* 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 <noreply@anthropic.com> * 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 <review_incorporation_contract> 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * docs(#3806): backfill PR #4345 into changeset and ADR Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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 <review_incorporation_contract> 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)', () => {
|
||||
|
||||
Reference in New Issue
Block a user