docs(architecture): map WorkHub decisions and authority - #4819
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for doing the ablation before opening this; 249 → 101 is the right instinct, and I checked every row of the module map against main: all six files exist, the resolver really has no create_new (workhub-session-resolver.ts:83-92), creation really is decided in workhub-route-policy.ts:48-61, and the Gate really re-reads Host state through listSessions / claimAction. But the ablation needs one more pass, because what is left is still a third copy of two existing authorities.
Line by line: invariants 1–5 are the ADR at workhub-coordination-session-adr.md:42, :67-69, :89-90 and :194-202; the gate-semantics paragraph is :58-61 and :89-96; the closing "not designed here" sentence is :53-56. The five stage names and their one-line responsibilities are the definitions at workhub-domain-language.md:70-105, including "never returns create_new" (:76) and "creation is a policy decision" (:84); the canonical flow is those five entries read in order. docs/README.md:110 says it directly: prefer updating an existing authority over adding a parallel document. A file that has to stay in sync with the ADR, the domain language and the source at once raises the maintenance surface without adding a fact.
The one thing here that is not written anywhere else is the term → file column, and that is worth keeping. Proposal: drop the new file and add that table as a short section after the terms in workhub-domain-language.md, one line per stage with the path. That keeps the mapping next to the definitions it maps, and the README / ADR link lines become unnecessary.
Two corrections to carry into the table:
- The diagram puts Action Intent and Session Resolver inside
subgraph Renderer. Both live inpackages/core, the domain language calls the resolver "shared" (:74), and the Gate itself imports the intent module on the Host side (workhub-coordination-action-gate.ts:42-45) to re-read creation and correction intent. For a document whose purpose is a source map, that is a wrong mapping. - "Existing targets use opaque
candidateRefvalues" is true fordelegate_existingandreplace, butstop_workcarries a realtargetSessionIdin its preconditions (protocol/workhub-coordination.ts:151-158). Say "delegate and replace usecandidateRef; stop names the resolved Session".
Also, if the file were to stay, it self-describes as an implementation map but is listed under ## Current contracts in the README.
Evidence boundary: static read against main; I did not render the mermaid or read the full 1254-line Gate, only its effects interface and imports.
AI-assisted review: drafted with Maka; I verified the README rule, the ADR overlap, the Host-side intent import and the stop preconditions myself.
|
Agreed. I ran the deletion test again and folded the only unique content into the existing authority. The parallel architecture document and its README / ADR / domain-language cross-links are gone. The PR now adds only a six-row Stage → Current module table after the terms in Both corrections are incorporated: Action Intent and Session Resolver map to All six mapped files exist at head |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 69c24a1f1fafd51c6a1b437ef99cac256b7eac23 against current main 12ebc45e6fe43b2eebd7dfe61f5c1fa92c0c873a. The ablation is a substantial improvement: the parallel architecture document and its temporary cross-links cancel out of the final diff, leaving only an 11-line implementation map in the existing domain-language authority.
I found one P2 in that remaining map. The Projection row points only to the Coordination Session projection adapter, while the document's own definition and the accepted ADR cover projections derived from both Coordination and ordinary Session facts. The ordinary Session summaries, recent turns, and delegated execution feedback are implemented in a separate module, so the current row is incomplete. Details are inline.
Validation: all six linked files exist; the removed architecture document has no remaining repository references; git diff --check, the ASF header audit, the exact-head hosted test check, and a clean synthetic merge with current main passed. The synthetic merge preserves the PR patch exactly. Markdown is excluded by the repository's Biome configuration, so no formatter check applies to this file.
Unable to determine: this documentation-only review did not execute the complete WorkHub behavioral test matrix; implementation claims were checked by reading the mapped modules and their composition paths.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at a95b384 against main (70d1516a0b). Eleven lines, one file, and every one of them is a fact that exists nowhere else: the six stage → module rows sit directly under the term definitions they map, all seven linked paths exist on main, Action Intent and Session Resolver now point at packages/core, and the Projection row names both owners, workhub-coordination-port.ts for coordination turns and active delegations and workhub-session-port.ts for ordinary Session facts and delegation feedback, which is what the term above it says a projection is built from. Leaving the controller out is right; it composes those two and owns no projection of its own. The parallel architecture document, the README index line and the ADR cross-links are gone, so nothing has to be kept in sync with the ADR any more.
Nothing to fix. One thing to know: there is no check in the repo that relative links under docs/ resolve, so a rename will silently break a row until someone reads the page. Not worth a script for six rows.
Evidence boundary: static read; verified the seven paths and the projection functions named in the inline thread against main.
AI-assisted review: drafted with Maka; I verified the paths and the projection split myself.
Summary
Ablation result
Corrections incorporated
packages/core, not the rendererValidation
git diff --checkThis PR is documentation-only.