From 1db726ebbf92a698abb875a99fa03fd009d088d7 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 5 Sep 2026 18:50:27 -0400 Subject: [PATCH] feat(#3806): canonize the Review Dispositions Ledger contract (#4345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3806): add parity tests for the Review Dispositions Ledger contract Failing-first: asserts references/planner-reviews.md, workflows/plan-phase.md, and agents/gsd-plan-checker.md agree on a single canonical "Review Dispositions Ledger" heading, its round-scoping, L##@{sha} anchor format, and append-only supersession rule. These fail until the canon and its two references are added. Co-Authored-By: Claude Sonnet 5 * feat(#3806): canonize the Review Dispositions Ledger contract Promote the existing planner-reviews.md Step 4 return-payload tables (Review Feedback Addressed/Deferred) into a canonical `## Review Dispositions Ledger` PLAN.md section, stated once in planner-reviews.md and referenced (not restated) from plan-phase.md's and gsd-plan-checker.md's Review Incorporation dimension. Adds round-scoping (`### Round {N} — {REVIEWS_sha}`), a `L##@{sha}` line-anchor format so a REVIEWS.md reference survives the file being rewritten each round, and an append-only supersession rule. Scoped to part 1 only per the maintainer's approved-feature verdict — the deterministic lint/check verb (part 2) is explicitly deferred to a follow-up. Also: ADR-3806 recording the decision, a docs/features/ fragment (FEATURES.md is generated), and a changeset fragment. Closes #3806 Co-Authored-By: Claude Sonnet 5 * fix(#3806): fenced-example count bug and lint findings from review - tests/plan-review-convergence.test.cjs: the "heading exactly once" test counted the canonical heading text globally, so it also matched the illustrative fenced-code example in planner-reviews.md that shows the same heading as sample content, always failing 2 !== 1. Rewritten as a bounded line scanner that skips fenced blocks (found by an isolated adversarial review pass). Also bounded an unbounded regex quantifier over readFileSync content flagged by local/no-unbounded-quantifier. - docs/features/review-dispositions-ledger.md: match house fragment style (bold-lead paragraphs, not #### headings) per the Standards-axis review; regenerated docs/FEATURES.md. Co-Authored-By: Claude Sonnet 5 * fix(#3806): fit reference-cite fix within size hard caps; ack growth Trims the plan-phase.md / gsd-plan-checker.md reference-cite text to a single short clause pointing at gsd-core/references/planner-reviews.md (also fixes the bare `references/planner-reviews.md` cite the #3576 shipped-reference-cites gate rejects), bringing both files back under their SIZE hard caps and the plan-phase.md phase6 shrink-only baseline. Both files still grow slightly versus origin/next, acknowledged below per ADR-2719's emitted-drift-ack contract. Emitted-Drift-Ack-Growth: gsd-plan-checker.md — adds a short pointer (in the existing Review Incorporation bullet) to the canonical Review Dispositions Ledger location (#3806); stays within the LARGE hard cap. Emitted-Drift-Ack-Growth: plan-phase.md — adds a short pointer (in the existing review_incorporation_contract bullet) to the canonical Review Dispositions Ledger location (#3806); stays under the XL hard cap and the phase6 shrink-only baseline. Co-Authored-By: Claude Sonnet 5 * fix(#3806): correct malformed Emitted-Drift-Ack-Growth trailer block The previous commit's two Emitted-Drift-Ack-Growth trailers were separated from the Co-Authored-By trailer by a blank line, so git's own trailer parser (which tests/helpers/emitted-runtime.cjs reads via `%(trailers:key=...)`) only recognized the last contiguous block (Co-Authored-By) and treated the Ack-Growth lines as ordinary body text — invisible to the emitted-attribution gate, not malformed data. Restating them here as one contiguous trailer block, git log over the PR range aggregates trailers from every commit, so this is additive. Emitted-Drift-Ack-Growth: gsd-plan-checker.md — adds a short pointer (in the existing Review Incorporation bullet) to the canonical Review Dispositions Ledger location (#3806); stays within the LARGE hard cap. Emitted-Drift-Ack-Growth: plan-phase.md — adds a short pointer (in the existing review_incorporation_contract bullet) to the canonical Review Dispositions Ledger location (#3806); stays under the XL hard cap and the phase6 shrink-only baseline. Co-Authored-By: Claude Sonnet 5 * fix(#3806): isolate the ack-trailer paragraph as its own trailer block Git's trailer parser requires the trailer paragraph to be the message's final paragraph, preceded by a blank line, and to contain nothing but trailer-shaped lines. The prior commit's blank line before the trailer lines was missing, which folded the leading Emitted-Drift-Ack-Growth lines into an ordinary prose paragraph. Emitted-Drift-Ack-Growth: gsd-plan-checker.md — adds a short pointer (in the existing Review Incorporation bullet) to the canonical Review Dispositions Ledger location (#3806); stays within the LARGE hard cap. Emitted-Drift-Ack-Growth: plan-phase.md — adds a short pointer (in the existing review_incorporation_contract bullet) to the canonical Review Dispositions Ledger location (#3806); stays under the XL hard cap and the phase6 shrink-only baseline. Co-Authored-By: Claude Sonnet 5 * docs(#3806): backfill PR #4345 into changeset and ADR Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: sim Co-authored-by: Claude Sonnet 5 --- .changeset/nimble-tigers-forage.md | 5 + agents/gsd-plan-checker.md | 2 +- docs/FEATURES.md | 38 ++++++ docs/adr/3806-review-dispositions-ledger.md | 139 +++++++++++++++++++ docs/adr/README.md | 1 + docs/features/review-dispositions-ledger.md | 38 ++++++ gsd-core/references/planner-reviews.md | 47 +++++++ gsd-core/workflows/plan-phase.md | 2 +- tests/plan-review-convergence.test.cjs | 143 ++++++++++++++++++++ 9 files changed, 413 insertions(+), 2 deletions(-) create mode 100644 .changeset/nimble-tigers-forage.md create mode 100644 docs/adr/3806-review-dispositions-ledger.md create mode 100644 docs/features/review-dispositions-ledger.md diff --git a/.changeset/nimble-tigers-forage.md b/.changeset/nimble-tigers-forage.md new file mode 100644 index 000000000..5d42e05df --- /dev/null +++ b/.changeset/nimble-tigers-forage.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 4345 +--- +**Reviews-mode disposition records now have a canonical shape** — planning a phase with `/gsd-plan-phase --reviews` writes accepted/deferred review findings into PLAN.md under one `## Review Dispositions Ledger` section instead of each planner run improvising its own format. Entries are grouped per review round and cite REVIEWS.md lines as `L##@{sha}` so a reference still resolves after the next round rewrites the file. (#3806) diff --git a/agents/gsd-plan-checker.md b/agents/gsd-plan-checker.md index 637954d88..8828e6ae4 100644 --- a/agents/gsd-plan-checker.md +++ b/agents/gsd-plan-checker.md @@ -89,7 +89,7 @@ REVIEWS.md is audit trail and feedback input, not a hidden execution contract. / - Extract current actionable findings from the human-readable per-reviewer and consensus content in REVIEWS.md. Do NOT look for a `CYCLE_SUMMARY: current_high= current_actionable=` line or `## Current HIGH Concerns` / `## Current Actionable Non-HIGH Concerns` section headers — those machine-readable fields exist only in the convergence orchestrator's return message, never in REVIEWS.md (which contains only human-readable review content). - Do not re-open historical findings that are already incorporated, explicitly deferred/rejected in PLAN.md, or marked fully resolved. -- Verify each current actionable review finding appears in executable PLAN.md content: a task, ``, ``, ``, `must_haves`, threat model, artifact list, stale-path correction, or explicit deferral/rejection rationale. +- Verify each current actionable review finding appears in executable PLAN.md content: a task, ``, ``, ``, `must_haves`, threat model, artifact list, stale-path correction, or explicit deferral/rejection rationale using the Review Dispositions Ledger in `gsd-core/references/planner-reviews.md`. - If a current actionable finding remains only in REVIEWS.md and would be invisible to /gsd:execute-phase, return `## ISSUES FOUND`. Use WARNING by default; use BLOCKER when the missing incorporation can prevent the phase goal, create unsafe execution, or invalidate verification. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index e8fca97fc..438d5afc5 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -25,6 +25,7 @@ - [Freeform Routing](#12-freeform-routing) - [Note Capture](#13-note-capture) - [Auto-Advance (Next)](#14-auto-advance-next) + - [Review Dispositions Ledger](#3806-review-dispositions-ledger) - [Quick Batch Mode](#4015-quick-batch-mode) - [Quality Assurance Features](#quality-assurance-features) - [Nyquist Validation](#15-nyquist-validation) @@ -609,6 +610,43 @@ --- +### 3806. Review Dispositions Ledger + +**Purpose:** Reviews-mode planning (`/gsd-plan-phase {N} --reviews`) has required every current +actionable REVIEWS.md finding to be incorporated into PLAN.md or explicitly deferred/rejected +there since v1.5.0 (#724/#728). Nothing canonized *where* in PLAN.md, *what shape*, or how a +REVIEWS.md line reference survives the next round rewriting the file wholesale. Two +independently-invented, mutually incompatible disposition formats were observed across two +consecutive rounds of the same phase, each written by a different planner subagent instance +improvising from prose alone. + +**Behavior:** The existing return-payload tables from `references/planner-reviews.md` Step 4 — +`### Review Feedback Addressed` / `### Review Feedback Deferred` — are now the canonical +**Review Dispositions Ledger**, promoted verbatim in shape into the affected PLAN.md itself under +a `## Review Dispositions Ledger` heading. Each reviews-mode round gets its own +`### Round {N} — {REVIEWS_sha}` subsection, where `{REVIEWS_sha}` is the commit that wrote that +round's REVIEWS.md snapshot (`workflows/review.md` already commits REVIEWS.md as its own commit). +A REVIEWS.md line reference cites `L##@{REVIEWS_sha}`; a bare line number is non-conforming. The +ledger is append-only — a later round adds a new row naming what it supersedes rather than editing +or deleting an earlier round's tables. + +The contract is stated once, in `references/planner-reviews.md`; `workflows/plan-phase.md`'s +`` and `agents/gsd-plan-checker.md`'s Review Incorporation dimension +both reference it by name rather than restating it, guarded by a parity test +(`tests/plan-review-convergence.test.cjs`) that fails if the three drift apart. + +`{Concern}`/`{Reason}` stay free text — the reviewer roster is capability-owned and open to +third-party additions, so no closed reviewer/severity enum is introduced. + +**Known limits:** No lint or check verb enforces this shape yet — a follow-up (tracked as part 2 +of #3806) will add deterministic enforcement once a migration story for the two pre-existing ad-hoc +formats already in the wild is decided. Legacy PLAN.md content written before this convention is +not migrated or flagged. + +**Reference:** [ADR-3806](adr/3806-review-dispositions-ledger.md) · [Cross-AI Peer Review](#42-cross-ai-peer-review) + +--- + ### 4015. Quick Batch Mode **Command:** `/gsd-quick-batch [--file ] [--jobs auto|N] [--validate] [--research] [--resume ]` diff --git a/docs/adr/3806-review-dispositions-ledger.md b/docs/adr/3806-review-dispositions-ledger.md new file mode 100644 index 000000000..798484872 --- /dev/null +++ b/docs/adr/3806-review-dispositions-ledger.md @@ -0,0 +1,139 @@ +# Review Dispositions Ledger canonizes where and how reviews-mode records incorporate/defer decisions in PLAN.md + +- **Status:** Accepted +- **Date:** 2026-09-05 +- **Issue:** #3806 +- **Implementation:** PR #4345 + +## Context + +Since v1.5.0 (#724/#728), reviews-mode planning requires every current actionable REVIEWS.md +finding to be either incorporated into executable PLAN.md content or explicitly +deferred/rejected with a rationale recorded in that PLAN.md +(`gsd-core/workflows/plan-phase.md` ``; +`agents/gsd-plan-checker.md` Review Incorporation dimension). That content requirement has held up +well. What it never specified is *where in PLAN.md*, *in what shape*, or *how a REVIEWS.md line +reference survives the next round* — `gsd-core/workflows/review.md`'s `/gsd:review` step rewrites +each phase's `-REVIEWS.md` wholesale on every cycle, so a bare line-number citation from round 1 +resolves against different content by round 3. + +In practice, this produced exactly the failure an unspecified format invites: across two +consecutive `/gsd-review` → `/gsd-plan-phase {N} --reviews` rounds of the same phase, two +different planner subagent instances each independently improvised a disposition format. Round 1 +invented `## Review Dispositions (developer-ruled)` with `[REVIEW DISPOSITION] …` lines; round 2 +invented a second, incompatible `## Review Scope Disposition (requester lock)` with +`AUTHORIZED`/`REJECTED` bullets citing bare `R2-L32`-style line numbers. Both now coexist in the +same PLAN files. Neither is self-sufficient: entries reference conversation-only context (a +mandated "ten fixes" that exists nowhere on disk) and bare line numbers into a file that has since +been rewritten twice more. + +The canon gap is real, not a one-off misuse: `gsd-core/references/planner-reviews.md` Step 4 +already defines a return-payload shape for this exact information — + +```markdown +### Review Feedback Addressed + +| Concern | Severity | How Addressed | +|---------|----------|---------------| +| {concern} | HIGH | Plan {N}, Task {M}: {how} | + +### Review Feedback Deferred +| Concern | Reason | +|---------|--------| +| {concern} | {why — out of scope, disagree, etc.} | +``` + +— but it was scoped to the planner's *return message to the orchestrator*, never promoted into +the PLAN.md file content the content requirement actually governs. Each fresh planner subagent +therefore had nothing on disk to imitate and improvised its own shape, twice. + +## Decision + +Promote the existing Step 4 tables, verbatim in shape, into a canonical `## Review Dispositions +Ledger` section that reviews-mode planners write **into the affected PLAN.md itself** — not a new +line grammar. The ledger groups entries by round: one `### Round {N} — {REVIEWS_sha}` subsection +per reviews-mode cycle that touched the plan, where `{REVIEWS_sha}` is the commit that wrote that +round's REVIEWS.md snapshot (`workflows/review.md` already commits REVIEWS.md as its own commit — +`git log -1 --format=%h -- /-REVIEWS.md` gives a real, addressable sha). Any +REVIEWS.md line reference cites `L##@{REVIEWS_sha}`; a bare line number is non-conforming, because +it silently resolves against whatever REVIEWS.md happens to contain by the time someone reads it. +The ledger is append-only: a later round never edits or deletes an earlier round's tables, and +overturning a prior verdict means adding a new row that names what it supersedes. + +The contract is stated **once**, in `gsd-core/references/planner-reviews.md` (the stable seam — 3 +commits total on `next`, versus 51 and 57 on `plan-phase.md` and `gsd-plan-checker.md` +respectively). `gsd-core/workflows/plan-phase.md`'s `` and +`agents/gsd-plan-checker.md`'s Review Incorporation dimension both *reference* the canonical +section by name and point at `planner-reviews.md` for its shape, rather than restating it — each +keeps only the workflow/checker-specific logic that genuinely belongs to it (when a finding counts +as current-actionable, BLOCKER vs. WARNING severity). A parity test +(`tests/plan-review-convergence.test.cjs`, describe block `'plan-review-convergence reviews-mode +ledger canonicalization (#3806)'`) extracts the live heading text from `planner-reviews.md` and +asserts both referencing files still name it, and that neither restates a competing `##`-level +heading of the same name — so a rename in the canon that isn't mirrored in the references fails +the build instead of drifting silently. + +`{Concern}`/`{Reason}` stay free text. The reviewer roster is capability-owned — twelve +capabilities each declare a `reviewer` block with `reviewsSection`, and third-party capabilities +can add reviewers — so a closed enum for the reviewer or severity field would be wrong by +construction the moment a new capability ships one. + +## What stays OUTSIDE this decision + +- **A deterministic lint/check verb enforcing this shape.** Two ad-hoc, mutually incompatible + formats already exist in the wild (the round-1/round-2 improvisations above). A hard-failing + lint shipped today would redden every existing PLAN.md carrying either of them before a + legacy-migration story (warn-then-fail, or scope enforcement to post-adoption entries) has been + decided. That is real, separate design work, explicitly deferred to a follow-up. +- **Changing what reviews-mode *requires*.** The #724/#728 content contract — every current + actionable finding must be incorporated or explicitly deferred/rejected in PLAN.md — is + unchanged. This decision only canonizes the shape of that existing requirement's rejection + records. +- **`/gsd:execute-phase`'s consumption of PLAN.md.** The ledger remains audit trail and feedback + input, exactly as REVIEWS.md itself is (`workflows/plan-phase.md`'s existing framing); the + executor does not read or depend on it. +- **Migrating or flagging legacy PLAN.md content.** The two ad-hoc formats already produced by + prior reviews-mode rounds are untouched by this decision. + +## Consequences + +- Reviews-mode planners across separate subagent instances and separate rounds now have a single, + concrete, on-disk shape to imitate instead of improvising one from prose alone — closing the gap + that produced two incompatible formats in the first reproduction. +- A REVIEWS.md line reference is addressable independent of how many times `/gsd:review` has + rewritten the file since, because it is pinned to the commit that produced the round it came + from. +- **Duplication risk is named, not just avoided.** `plan-phase.md` and `gsd-plan-checker.md` + already carried near-identical prose about "explicitly document a deferral/rejection rationale" + before this change; both now point at one canonical source instead of each independently + describing the shape, and the parity test is the mechanical guard against the two drifting apart + again the way the underlying prose already had. +- **This is a documentation/prompt-contract change with no runtime behavior change.** No `src/**` + code is touched; no new CLI verb exists yet. The follow-up lint (out of scope here) is what would + eventually make the shape mechanically enforced rather than convention-only. +- A stale `Proposed` never applies here: the decision and its full implementation (the canon plus + both references plus the parity test) land in this same PR, matching the precedent set by + [ADR-766](766-claude-code-plugin-manifest-module.md) ("this ADR + the hand-authored manifest land + first"). + +## Open questions + +- Should the deferred lint (#3806 part 2) hard-fail new entries immediately while only + warning on legacy ones, or gate on an adoption date? Left to that follow-up's own design. +- Should the ledger's per-round subsections eventually be machine-summarizable (e.g. a `check` + verb reporting "N concerns still open across M rounds")? Same follow-up. + +## References + +- Issue #3806 — original report, with the two reproduced disposition formats and the maintainer's + Go-with-conditions verdict scoping this decision to part 1 (canon only). +- `gsd-core/references/planner-reviews.md` — the canonical statement of the ledger contract. +- `gsd-core/workflows/plan-phase.md` `` — references the canon. +- `agents/gsd-plan-checker.md` Review Incorporation dimension — references the canon. +- `gsd-core/workflows/review.md` — commits each round's REVIEWS.md snapshot, the anchor the + `L##@{sha}` format points at. +- `tests/plan-review-convergence.test.cjs` — parity test locking the three seams together, and the + pre-existing `#724` contract tests this decision does not change. +- #724 / #728 — established the content requirement this decision only gives a canonical shape to. +- [ADR-766](766-claude-code-plugin-manifest-module.md) — precedent for an ADR and its full + implementation landing in the same PR. diff --git a/docs/adr/README.md b/docs/adr/README.md index a01b9ce82..b759a7f92 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -266,6 +266,7 @@ These govern the system as it stands. Cite these. | [ADR-3625](3625-vetted-spawn-library-evaluation.md) | The platform seam keeps its own Windows binary resolution rather than adopting a spawn library | Accepted | — | | [ADR-3626](3626-context-md-seam-claim-gate.md) | CONTEXT.md seam claims carry a checkable enforcement pointer | Accepted | — | | [ADR-3660](3660-runtime-artifact-layout-module.md) | Runtime Artifact Layout Module owns per-runtime artifact placement | Accepted | [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) | +| [ADR-3806](3806-review-dispositions-ledger.md) | Review Dispositions Ledger canonizes where and how reviews-mode records incorporate/defer decisions in PLAN.md | Accepted | — | ### Proposed diff --git a/docs/features/review-dispositions-ledger.md b/docs/features/review-dispositions-ledger.md new file mode 100644 index 000000000..89b7324c5 --- /dev/null +++ b/docs/features/review-dispositions-ledger.md @@ -0,0 +1,38 @@ +--- +id: 3806 +title: Review Dispositions Ledger +group: Planning Features +--- + +**Purpose:** Reviews-mode planning (`/gsd-plan-phase {N} --reviews`) has required every current +actionable REVIEWS.md finding to be incorporated into PLAN.md or explicitly deferred/rejected +there since v1.5.0 (#724/#728). Nothing canonized *where* in PLAN.md, *what shape*, or how a +REVIEWS.md line reference survives the next round rewriting the file wholesale. Two +independently-invented, mutually incompatible disposition formats were observed across two +consecutive rounds of the same phase, each written by a different planner subagent instance +improvising from prose alone. + +**Behavior:** The existing return-payload tables from `references/planner-reviews.md` Step 4 — +`### Review Feedback Addressed` / `### Review Feedback Deferred` — are now the canonical +**Review Dispositions Ledger**, promoted verbatim in shape into the affected PLAN.md itself under +a `## Review Dispositions Ledger` heading. Each reviews-mode round gets its own +`### Round {N} — {REVIEWS_sha}` subsection, where `{REVIEWS_sha}` is the commit that wrote that +round's REVIEWS.md snapshot (`workflows/review.md` already commits REVIEWS.md as its own commit). +A REVIEWS.md line reference cites `L##@{REVIEWS_sha}`; a bare line number is non-conforming. The +ledger is append-only — a later round adds a new row naming what it supersedes rather than editing +or deleting an earlier round's tables. + +The contract is stated once, in `references/planner-reviews.md`; `workflows/plan-phase.md`'s +`` and `agents/gsd-plan-checker.md`'s Review Incorporation dimension +both reference it by name rather than restating it, guarded by a parity test +(`tests/plan-review-convergence.test.cjs`) that fails if the three drift apart. + +`{Concern}`/`{Reason}` stay free text — the reviewer roster is capability-owned and open to +third-party additions, so no closed reviewer/severity enum is introduced. + +**Known limits:** No lint or check verb enforces this shape yet — a follow-up (tracked as part 2 +of #3806) will add deterministic enforcement once a migration story for the two pre-existing ad-hoc +formats already in the wild is decided. Legacy PLAN.md content written before this convention is +not migrated or flagged. + +**Reference:** [ADR-3806](adr/3806-review-dispositions-ledger.md) · [Cross-AI Peer Review](#42-cross-ai-peer-review) diff --git a/gsd-core/references/planner-reviews.md b/gsd-core/references/planner-reviews.md index da8f52088..ee1775d2f 100644 --- a/gsd-core/references/planner-reviews.md +++ b/gsd-core/references/planner-reviews.md @@ -40,3 +40,50 @@ Use standard PLANNING COMPLETE return format, adding a reviews section: |---------|--------| | {concern} | {why — out of scope, disagree, etc.} | ``` + +### Step 5: Write the ledger into PLAN.md (#3806) + +The two tables above are not only the planner's return payload — they are also the **canonical +Review Dispositions Ledger**, and they belong in the affected PLAN.md itself, in this exact shape. +`gsd-core/workflows/plan-phase.md` (``) and +`agents/gsd-plan-checker.md` (Review Incorporation dimension) both point back to this section for +the ledger's shape rather than restating it — this is the one place it is defined. + +## Review Dispositions Ledger + +Add or extend a `## Review Dispositions Ledger` section in the affected PLAN.md, containing one +`### Round {N} — {REVIEWS_sha}` subsection per reviews-mode round that touched this plan, where +`{REVIEWS_sha}` is the commit that wrote the REVIEWS.md snapshot being ruled on (the short sha from +`git log -1 --format=%h -- /-REVIEWS.md`, after `workflows/review.md`'s REVIEWS.md +commit step). Under each round heading, use the two tables from Step 4 above, unchanged in shape: + +```markdown +## Review Dispositions Ledger + +### Round 1 — a1b2c3d + +### Review Feedback Addressed +| Concern | Severity | How Addressed | +|---------|----------|---------------| +| {concern} | HIGH | Plan {N}, Task {M}: {how} | + +### Review Feedback Deferred +| Concern | Reason | +|---------|--------| +| {concern} | {why — out of scope, disagree, etc.} | +``` + +**Anchoring.** Any reference to a specific REVIEWS.md line cites `L##@{REVIEWS_sha}` (e.g. +`L32@a1b2c3d`) — a bare line number is meaningless once the next round rewrites REVIEWS.md +wholesale. `{Concern}` and `{Reason}` stay free text; do not invent a reviewer/severity enum — the +reviewer roster is capability-owned and open to third-party additions (see each capability's +`reviewer.reviewsSection`). + +**Append-only.** A later round never edits or deletes a prior round's tables. To overturn a prior +round's verdict, add a new row in the current round's table whose Reason/How Addressed names the +round and concern it supersedes (e.g. "Supersedes Round 1 Deferred: {concern} — now addressed in +Plan 3"). + +**Out of scope for this contract.** A deterministic lint/check verb that mechanically enforces this +shape is a separate, later addition (#3806 part 2) — this section defines the format only. Legacy +PLAN.md content written before this convention existed is not migrated or flagged by it. diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index 3516370dc..0b4db2a13 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -785,7 +785,7 @@ ${AGENT_SKILLS_PLANNER} For each current actionable finding in REVIEWS.md, the planner MUST either: - incorporate it into a PLAN.md task, ``, ``, ``, `must_haves`, threat model, or artifact list; or -- explicitly document a deferral/rejection rationale in the relevant PLAN.md so the executor and reviewer can see the decision. +- explicitly document a deferral/rejection rationale in the relevant PLAN.md, using the Review Dispositions Ledger in `gsd-core/references/planner-reviews.md`. Historical findings already incorporated, explicitly deferred/rejected in PLAN.md, or marked fully resolved do not require new plan changes. diff --git a/tests/plan-review-convergence.test.cjs b/tests/plan-review-convergence.test.cjs index 099fb28eb..888b5554e 100644 --- a/tests/plan-review-convergence.test.cjs +++ b/tests/plan-review-convergence.test.cjs @@ -43,6 +43,7 @@ const CONFIG_DOC_PATH = path.join(__dirname, '..', 'docs', 'CONFIGURATION.md'); const PLAN_PHASE_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-phase.md'); const PLANNER_REVIEWS_PATH = path.join(__dirname, '..', 'gsd-core', 'references', 'planner-reviews.md'); const PLAN_CHECKER_PATH = path.join(__dirname, '..', 'agents', 'gsd-plan-checker.md'); +const WORKFLOW_REVIEW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'review.md'); // #2315: the workflow's reviewer-resolution block pipes through `jq`, which is // a documented production dependency (review.md:244 "install jq if missing") @@ -1018,6 +1019,148 @@ describe('plan-review-convergence reviews-mode incorporation contract (#724)', ( }); }); +// ─── Reviews-mode ledger canonicalization (#3806) ────────────────────────── +// +// #3806: reviews-mode requires actionable findings to be incorporated or +// explicitly deferred/rejected IN PLAN.md (#724/#728), but nothing canonized +// WHERE in PLAN.md, WHAT SHAPE, or how a line reference survives REVIEWS.md +// being rewritten wholesale every round. Two independently-invented, mutually +// incompatible disposition formats were observed across two consecutive +// rounds of the same phase. The fix promotes the existing Step-4 return- +// payload tables (`### Review Feedback Addressed` / `### Review Feedback +// Deferred`) into a canonical `## Review Dispositions Ledger` PLAN.md +// section, stated ONCE in planner-reviews.md and referenced — not restated — +// from plan-phase.md and gsd-plan-checker.md. These tests are the parity +// assertion the maintainer's verdict required (condition 3): they fail if +// the three seams diverge. + +describe('plan-review-convergence reviews-mode ledger canonicalization (#3806)', () => { + const plannerReviews = fs.readFileSync(PLANNER_REVIEWS_PATH, 'utf8'); + const planPhase = fs.readFileSync(PLAN_PHASE_PATH, 'utf8'); + const planChecker = fs.readFileSync(PLAN_CHECKER_PATH, 'utf8'); + const reviewWorkflow = fs.readFileSync(WORKFLOW_REVIEW_PATH, 'utf8'); + + const LEDGER_HEADING_RE = /^##\s+(Review Dispositions Ledger[^\r\n]{0,200})$/m; + + test('planner-reviews.md defines the canonical "Review Dispositions Ledger" heading exactly once', () => { + // Counts only the REAL heading occurrence, skipping any fenced code-block + // example that happens to show the same heading text as sample content + // (planner-reviews.md's worked example does this deliberately, so a plan- + // writing agent has a full copyable sample including its own top heading). + const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); + let inFence = false; + let realHeadingCount = 0; + for (const line of splitLines(plannerReviews)) { + if (line.trimStart().startsWith('```')) { + inFence = !inFence; + continue; + } + if (inFence) continue; + if (line.trim() === '## Review Dispositions Ledger') realHeadingCount += 1; + } + assert.equal( + realHeadingCount, + 1, + 'references/planner-reviews.md must define the real (non-fenced-example) "## Review Dispositions Ledger" heading exactly once — this is the single canonical statement of the ledger contract (#3806)' + ); + }); + + test('plan-phase.md references the canonical ledger instead of restating its shape', () => { + const headingMatch = plannerReviews.match(LEDGER_HEADING_RE); + assert.ok(headingMatch, 'precondition: canonical heading must exist in planner-reviews.md'); + const canonicalHeading = headingMatch[1].trim(); + + assert.match( + planPhase, + /references\/planner-reviews\.md/, + 'plan-phase.md must point at references/planner-reviews.md for the ledger shape (#3806)' + ); + assert.ok( + planPhase.includes(canonicalHeading), + `plan-phase.md must name the live canonical heading ("${canonicalHeading}") — if planner-reviews.md renames it without updating this reference, this fails (#3806 parity)` + ); + assert.doesNotMatch( + planPhase, + /^##\s+Review Dispositions Ledger[^\r\n]*$/m, + 'plan-phase.md must NOT define its own competing "## Review Dispositions Ledger" heading — the contract is stated once, in planner-reviews.md, never restated (#3806)' + ); + }); + + test('gsd-plan-checker.md references the canonical ledger instead of restating its shape', () => { + const headingMatch = plannerReviews.match(LEDGER_HEADING_RE); + assert.ok(headingMatch, 'precondition: canonical heading must exist in planner-reviews.md'); + const canonicalHeading = headingMatch[1].trim(); + + assert.match( + planChecker, + /references\/planner-reviews\.md/, + 'gsd-plan-checker.md Review Incorporation dimension must point at references/planner-reviews.md for the ledger shape (#3806)' + ); + assert.ok( + planChecker.includes(canonicalHeading), + `gsd-plan-checker.md must name the live canonical heading ("${canonicalHeading}") — if planner-reviews.md renames it without updating this reference, this fails (#3806 parity)` + ); + assert.doesNotMatch( + planChecker, + /^##\s+Review Dispositions Ledger[^\r\n]*$/m, + 'gsd-plan-checker.md must NOT define its own competing "## Review Dispositions Ledger" heading — the contract is stated once, in planner-reviews.md, never restated (#3806)' + ); + }); + + test('planner-reviews.md documents round-scoping and the L##@{sha} anchor format', () => { + assert.match( + plannerReviews, + /###\s+Round\s*\{?N\}?/, + 'canonical ledger must define a per-round subsection (e.g. "### Round {N} — ...") so successive reviews-mode rounds do not collide or overwrite each other (#3806)' + ); + assert.match( + plannerReviews, + /L##@\{?REVIEWS_sha\}?/, + 'canonical ledger must define the L##@{REVIEWS_sha} line-anchor format — a bare line number is meaningless once REVIEWS.md is rewritten wholesale next round (#3806)' + ); + }); + + test('planner-reviews.md documents append-only supersession, not in-place edits', () => { + assert.match( + plannerReviews, + /append-only/i, + 'canonical ledger must state the append-only rule: a later round never edits or deletes a prior round\'s tables (#3806)' + ); + assert.match( + plannerReviews, + /supersed/i, + 'canonical ledger must define how a later round overturns an earlier verdict (a new row naming what it supersedes), not by editing history (#3806)' + ); + }); + + test('planner-reviews.md keeps the ledger\'s Concern/Reason fields free text (no closed enum introduced)', () => { + // Condition 4 of the maintainer's verdict: the reviewer roster is + // capability-owned and third parties can add reviewers, so `{reviewer}` + // must never become a closed enum. Guard against the ledger promotion + // accidentally introducing one (e.g. "must be one of: Codex, Claude, ..."). + assert.ok( + !/reviewer\s+must\s+be\s+one\s+of/i.test(plannerReviews), + 'the ledger must not introduce a closed reviewer/severity enum — the field stays free text (#3806 condition 4)' + ); + }); + + test('workflows/review.md still commits REVIEWS.md as its own commit (anchor precondition)', () => { + // The L##@{sha} anchor format is only meaningful if REVIEWS.md snapshots + // are actually addressable by commit. If this ever stops being true, the + // anchoring half of the #3806 contract silently becomes unfulfillable. + assert.match( + reviewWorkflow, + /REVIEWS\.md/, + 'workflows/review.md must still reference REVIEWS.md as a committed artifact — the #3806 ledger anchors against its commit sha' + ); + assert.match( + reviewWorkflow, + /commit["\s]/, + 'workflows/review.md must still run a commit step for REVIEWS.md — without it, {REVIEWS_sha} has nothing to point at' + ); + }); +}); + // ─── Local model reviewer support ──────────────────────────────────────── describe('plan-review-convergence local model reviewer flags (#2306-local)', () => {