Refresh architecture overview to match current command slices#166
Conversation
There was a problem hiding this comment.
QA Review — PR #166 (Ref: #165)
Decision: APPROVED
What I verified
- PR body first line is
Ref: #165✅ - Head branch is
issues/orfe-165✅ - Single file changed:
docs/architecture/overview.md✅ (pure docs change, no code touched) - Reviewed the full diff against the live
src/commands/tree
Command surface accuracy (verified against src/commands/index.ts and filesystem):
All 18 registered commands were confirmed present in the new "Current built-in command surface" section:
help(top-level,topLevel: true,execution: 'runtime',runtimeHandlerin definition) ✅auth token✅issue: get, create, comment, update, validate, set-state (6 slices) ✅pr: get, get-or-create, comment, reply, submit-review, update, validate (7 slices — previously the old doc omittedupdateandvalidate) ✅project: get-status, set-status ✅runtime info(execution: 'runtime', definition-only,runtimeHandler, no handler.ts) ✅
Module map accuracy:
auth/,issue/,pr/,project/,runtime/vertical directories all confirmed ✅help/top-level with onlydefinition.ts+ test files confirmed ✅runtime/info/with onlydefinition.tsconfirmed ✅pr/shared/:github-errors.ts,github-response.ts,review.ts— conceptual label "group-local helpers" is accurate ✅issue/shared/:github-errors.ts,github-response.ts,state.ts— same ✅project/shared/GraphQL helpers confirmed ✅
Structural pattern accuracy:
execution: 'runtime'withruntimeHandlerdescribes bothhelpandruntime infodefinition-only slices correctly ✅*.test.tsnaming matches actual slice-local test files (cli.test.ts,core.test.ts,tool.test.ts,definition.test.ts) ✅- Registry
execution: 'github'for dispatched commands vsexecution: 'runtime'for definition-owned runtime handlers correctly reflected ✅
Issue acceptance criteria met:
- No misleadingly stale slice/layout examples remain ✅
- Module map aligned with current
src/commands/structure ✅ - A contributor reading the overview gets an accurate picture of the current slice architecture and ownership boundaries ✅
Blockers
None.
Important
Greg reported npm test, npm run lint, npm run typecheck, and npm run build as "not run". This is a documentation-only PR (single .md file changed) — none of those checks act on markdown content and none would surface or catch doc accuracy issues. The accuracy has been independently verified above against source code. That said, reporting all four as "not run" on a PR verification checklist is poor practice even for docs changes. Future doc PRs should at minimum run the test suite to confirm the base codebase health hasn't regressed before opening a PR.
Nice-to-have
-
Slightly awkward framing in updated intro paragraph: The sentence "Slices such as
issue validate,issue update,pr validate,pr update,project set-status, andruntime infoare first-class peers of the older get/create/comment paths" is slightly odd —runtime infobelongs to its own distinctruntimevertical and isn't a natural "peer" of theissue/prdomain families. A small rewording like listing it separately or dropping it from that sentence would improve clarity. Non-blocking. -
Stale running binary (out of scope): The live
orfe helpplugin output returned only 6 PR commands and excludedpr update, suggesting the installed binary predates that command's addition. The source code is correct (7 PR commands inindex.ts). This is unrelated to this PR but worth tracking — the build may need refreshing.
Docs / invariants
This PR is a direct docs update for docs/architecture/invariants.md's documentation invariant:
docs/README.mdis the entrypoint for durable product and architecture memory.
The overview.md update is exactly what that invariant calls for when the source drifts. ✅
No ADR or debt update is needed — the PR body correctly notes that existing invariants and ADRs already cover the architecture, and no new durable debt was found.
Ref: #165
Summary
docs/architecture/overview.mdso the documented command architecture matches the current built-insrc/commands/surface, including top-levelhelp,runtime, and newer validate/update/status slicesVerification
npm testnot runnpm run lintnot runnpm run typechecknot runnpm run buildnot runDocs / ADR / debt
docs/architecture/overview.md; existing invariants and ADRs already covered the architecture, and no new durable debt was foundRisks / follow-ups