From 6eea00b707e8e33b6e2c645c7ee7c52e3a591ac7 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 30 Aug 2026 14:25:24 -0400 Subject: [PATCH] enhance(#3301): tell reviewers the plan ids and total count, grade coverage (#4084) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3301): add failing-first plan coverage manifest tests Failing-first regression tests for the plan-id manifest, the updated Review Instructions, and the mechanical per-reviewer coverage check, ahead of the review.md implementation. RED baseline before the fix lands. * test(#3301): raise allow-test-rule-refs unverified ceiling for new marker Adding tests/review-plan-coverage-manifest.test.cjs's source-text-is-the-product marker grows the unverified-exemption pool by one (282 -> 283), the same documented growth path scripts/lint-allow-test-rule-refs.cjs's own failure output names. Confirmed clean via 'npm run lint:allow-test-rule-refs' locally. * feat(#3301): tell reviewers the plan ids and total count, grade coverage build_prompt now derives a plan-id manifest from each *-PLAN.md filename (stripping the -PLAN.md suffix) and appends it, with the total plan count, to both gsd-review-instructions.md and gsd-review-prompt.md. The Review Instructions prose requires one heading-verbatim section per id before any cross-plan or overall-risk content. write_reviews grades each dispatched lane's real (non-stub, non-empty) review against that same manifest and records an optional plan_coverage: frontmatter block, present only when a lane is incomplete. The match escapes regex metacharacters in the id and excludes a preceding/trailing hyphen or word character as a boundary, closing the two traps named in the issue (a decimal phase like 12.6 satisfied by 12X6-01; a threat id like T-04-07 registering as coverage of plan 04-07). CodeRabbit is exempt, since it never receives the source-grounding prompt carrying the manifest. This closes the gap where a review that silently covers only some plans in a multi-plan phase is indistinguishable from one that covers all of them. * docs(#3301): add changeset fragment * test(#3301): use t.after() instead of try/finally for cleanup CONTRIBUTING.md bans try/finally inside test bodies. Code review caught this in the new coverage-manifest test file; switch every fixture-cleanup site to the approved t.after() pattern. * test: use t.after() instead of try/finally in #3300's build_prompt tests Pre-existing try/finally-for-cleanup pattern in this file (landed for #3300) violates CONTRIBUTING.md's explicit ban on try/finally inside test bodies. Surfaced incidentally while reviewing #3301's diff, which cites this file as its extraction-pattern precedent; fixed inline per the no-defer rule rather than deferred to a separate PR. * test(#3301): anchor coverage-check extraction on the fence line, not prose `.plans-manifest.md` also appears in write_reviews' own prose ahead of the ```bash fence, so indexOf found that occurrence first and the backward-walk-to-fence-open landed on the earlier, unrelated gate-check block instead. gsd-test caught this: coverage-check tests expecting a real verdict got null, because the wrong block ran and never writes .plan-coverage-.json. Anchor on the fence-only bash assignment line instead. Emitted-Drift-Ack-Growth: review.md — #3301 adds the plan-coverage manifest and mechanical coverage check to build_prompt/write_reviews. * fix(#3301): route id escaping through the canonical pattern seam ADR-3212 (epic #3212) consolidated ~44 hand-rolled regex-escape copies into one owner, src/pattern.cts's escapeRegex, specifically to stop this exact class of duplication. My coverage-check node -e script hand-rolled the identical metachar-escape regex — invisible to eslint-rules/no-adhoc-regex-escape.cjs only because it lives inside a workflow markdown file, not a .cts/.cjs source file the shape-matching guard scans. Require the compiled seam (gsd-core/bin/lib/pattern.cjs) instead, matching the established node -e-requires-a-compiled-lib idiom already used elsewhere in this workflow (code-review.md's code-review-flags.cjs/code-review-depth.cjs calls). Verified both named traps from the issue still resolve correctly under escapeRegex's RegExp.escape-backed implementation, which differs in escaped-text shape (hex-escapes hyphens/leading chars) but not match result. * test(#3301): run coverage-check block with cwd at the repo root The block's node -e now requires ./gsd-core/bin/lib/pattern.cjs, a path relative to the repo root (correct for production, which always runs from there). The test harness ran it with cwd at the fixture's own temp dir instead, so the require failed. Add an optional cwd param to runScript (default: root, unchanged for the plan-copy-block tests) and pass the real repo root for every coverage-check call site. Manually verified end-to-end before spending another remote run: the extracted block now produces the expected {complete:true} verdict. * docs(#3301): backfill changeset pr number (pr:0 -> pr:4084) --------- Co-authored-by: sim --- .changeset/bold-sloths-dance.md | 5 + docs/COMMANDS.md | 2 + gsd-core/workflows/review.md | 129 ++++++ ...low-test-rule-refs.unverified-ceiling.json | 2 +- ...ew-build-prompt-optional-sections.test.cjs | 97 ++--- tests/review-plan-coverage-manifest.test.cjs | 372 ++++++++++++++++++ 6 files changed, 550 insertions(+), 57 deletions(-) create mode 100644 .changeset/bold-sloths-dance.md create mode 100644 tests/review-plan-coverage-manifest.test.cjs diff --git a/.changeset/bold-sloths-dance.md b/.changeset/bold-sloths-dance.md new file mode 100644 index 000000000..7db5f8cce --- /dev/null +++ b/.changeset/bold-sloths-dance.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 4084 +--- +**`/gsd-review` now tells every reviewer the exact plan ids and total count, and grades coverage against them** — a review that silently covers only some of a multi-plan phase is no longer indistinguishable from one that covered every plan. (#3301) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 4eeff01db..79245ff8b 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -1811,6 +1811,8 @@ Reviewers reached through `--all` or `review.default_reviewers` behave different Its frontmatter records the model each reviewer resolved to, as `models:` (the model id, or `unknown`, with a `(reasoning=)` suffix when GSD applied a reasoning effort to that lane) and `model_sources:` (how each value was determined — `pinned`, `served`, `requested`, `banner`, `transcript`, or `unknown`). See [Resolved model recording](CONFIGURATION.md#resolved-model-recording-2295). +**Plan coverage manifest (#3301):** the assembled prompt tells every prompt-fed reviewer exactly which plan ids exist in this review and the total count, and asks for one heading-verbatim section per id before any cross-plan or overall-risk content — so a review that silently stops partway through a multi-plan phase is no longer indistinguishable from one that covered every plan. A mechanical check grades each reviewer's output against that same id list and records an optional `plan_coverage:` frontmatter block, present only when a reviewer's output does not mention every id (diagnostic only — it never blocks the run). CodeRabbit is not graded — it is a diff-only reviewer that never receives the prompt carrying the manifest. + ```bash # set project default reviewers for no-flag /gsd-review runs gsd config-set review.default_reviewers '["gemini","codex"]' diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 636c48907..1b553d7b5 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -220,6 +220,13 @@ Provide structured feedback on plan quality, completeness, and risks. Findings citing `file:line` evidence are weighted far more heavily than impressionistic ones; a review that only restates the plan's own claims has low value. +**Plan coverage is mandatory (#3301).** The exact list of plan ids and the total plan count for +this review are given in the "## Plan Coverage Manifest" section below. Give **every** listed id +its own `##`-level section headed with that id **verbatim** (e.g. `## 12.6-01`) before writing any +cross-plan comparison, an overall risk assessment, or a consensus-style summary. A review that +stops before every id has its own section is an incomplete review, not a summary — if you must +stop early, say so explicitly and name which ids you did not reach. + Analyze each plan and provide: 1. **Summary** — One-paragraph assessment @@ -262,6 +269,42 @@ for PLAN_FILE in "${PHASE_DIR}"/*-PLAN.md; do PLAN_INDEX=$((PLAN_INDEX + 1)) done +# #3301: plan coverage manifest — tell reviewers exactly which plan ids exist and +# how many there are, so a review that silently covers 6 of 7 plans is no longer +# indistinguishable from one that covers all 7. The id is the plan file's own +# basename with the `-PLAN.md` suffix stripped (e.g. `12.6-01-PLAN.md` -> +# `12.6-01`) — NOT the `plan:` frontmatter key, which holds only the bare +# in-phase sequence number ("01") and can never reconstruct the phase-qualified +# id reviewers need to cite. The filename is guaranteed present for every copied +# plan, so no plan is ever dropped from the manifest for lacking a key. +# A plain string accumulator, not an array: zsh and bash disagree on array +# indexing and this block runs under both (see the nullglob/NULL_GLOB pairing +# above). +PLAN_IDS="" +for PLAN_FILE in "${PHASE_DIR}"/*-PLAN.md; do + PLAN_BASENAME=$(basename "$PLAN_FILE") + PLAN_IDS="$PLAN_IDS ${PLAN_BASENAME%-PLAN.md}" +done +PLAN_IDS="${PLAN_IDS# }" +PLAN_COUNT=0 +for _ in $PLAN_IDS; do PLAN_COUNT=$((PLAN_COUNT + 1)); done + +# Named to avoid BOTH existing RUN_DIR globs: `gsd-review-*.md` (reviewer +# reports, invoke_reviewers) and `gsd-review-plan-*.md` (the plan copies just +# above) — a manifest matching either would be picked up as a report or as a +# plan to review. +{ + echo "" + echo "## Plan Coverage Manifest" + echo "" + echo "Total plans in this review: ${PLAN_COUNT}" + echo "" + echo "Plan ids (give each one its own \`##\`-level section, headed verbatim):" + for PLAN_ID in $PLAN_IDS; do + echo "- ${PLAN_ID}" + done +} > "${RUN_DIR}/.plans-manifest.md" + # Optional section files (only if content was included in the combined prompt) if [ -f ".planning/PROJECT.md" ]; then cp .planning/PROJECT.md "${RUN_DIR}/gsd-review-project.md" @@ -277,6 +320,15 @@ fi if [ -f ".planning/REQUIREMENTS.md" ]; then cp .planning/REQUIREMENTS.md "${RUN_DIR}/gsd-review-requirements.md" fi + +# #3301: append the manifest to BOTH files reviewers actually read — the +# per-lane budget-trimmed instructions file (descriptor lanes get +# `--instructions-file`) and the full combined prompt (combined-prompt lanes +# read the whole file). The `instructions` fragment is in prompt-budget's +# `minimumFor` floor set and is never trimmed, so this survives per-lane +# budget trimming intact. +cat "${RUN_DIR}/.plans-manifest.md" >> "${RUN_DIR}/gsd-review-instructions.md" +cat "${RUN_DIR}/.plans-manifest.md" >> "${RUN_DIR}/gsd-review-prompt.md" ``` Note: `INSTRUCTIONS_BLOCK_FILE`, `ROADMAP_SECTION_FILE`, and `PHASE_DIR` come from prompt assembly; `RUN_DIR` is the run-scoped dir from `gather_context` (#2358) re-assigned from `{run_dir}` above. Copy the temp files written during prompt assembly to these section paths (or write each section here if the prompt was built inline). @@ -536,6 +588,79 @@ fi their `.err`/stub files preserved under `.review-diagnostics/` by `present_results`) and stop. - **Otherwise** (at least one lane produced a result — R1, unchanged): proceed exactly as below. +**#3301: plan coverage check.** For each dispatched lane that produced a *real* review (not a +stub, not budget-skipped, not empty), check whether its output mentions every plan id from +`.plans-manifest.md` — the same manifest `build_prompt` gave the reviewer, so the expected-id list +here can never diverge from what the reviewer was actually told. This is diagnostic only: it never +blocks the workflow, never fails a lane, and never changes the `TOTAL_LANE_FAILURE`/ +`ALL_LANES_SKIPPED` gate above. + +CodeRabbit is excluded — it is a diff-only lane that never receives the source-grounding prompt +(and therefore never receives the manifest or the per-id section instruction either), the same fact +that already excludes it from grounded-review weighting in the Consensus Summary below. + +The match is intentionally lenient about *where* an id appears (a `##`-headed section is asked for, +but plain prose mentioning the id still counts as coverage — grading only the letter of the +formatting instruction would produce false INCOMPLETE verdicts against a reviewer that cited real +evidence correctly). It is strict about *what* counts as a match: the id is regex-escaped (a +decimal phase like `12.6` must not let `12X6-01` satisfy `12.6-01` through an unescaped `.`), and a +`-`/word character immediately before or after the candidate match does not count as a boundary (so +a threat id like `T-04-07` elsewhere in the review must not register as covering plan `04-07`). + +```bash +RUN_DIR="{run_dir}" +MANIFEST="$RUN_DIR/.plans-manifest.md" + +# Recompute — a shell variable does not survive across separate fenced blocks +# (each is its own process), so DISPATCH_SLUGS from the gate-check block above +# cannot be assumed to still be set here. Same recomputation as that block and +# as invoke_reviewers. +DISPATCH_SLUGS="" +for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do + case " $DISPATCH_SLUGS " in + *" $SLUG "*) continue ;; + esac + DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG" +done + +for SLUG in $DISPATCH_SLUGS; do + [ "$SLUG" = "coderabbit" ] && continue + REVIEW_FILE="$RUN_DIR/gsd-review-$SLUG.md" + [ -f "$REVIEW_FILE" ] || continue + [ -s "$REVIEW_FILE" ] || continue + grep -q "review skipped: prompt budget" "$REVIEW_FILE" 2>/dev/null && continue + grep -q "failed or returned empty output" "$REVIEW_FILE" 2>/dev/null && continue + + node -e ' + const fs = require("fs"); + const { escapeRegex } = require("./gsd-core/bin/lib/pattern.cjs"); + const manifest = fs.readFileSync(process.argv[1], "utf8"); + const review = fs.readFileSync(process.argv[2], "utf8"); + const ids = manifest.split("\n") + .filter((l) => l.startsWith("- ")) + .map((l) => l.slice(2).trim()) + .filter(Boolean); + const missing = ids.filter((id) => { + const re = new RegExp("(? "$RUN_DIR/.plan-coverage-$SLUG.json" +done +``` + +Each `${RUN_DIR}/.plan-coverage-.json` carries `{complete, missing_ids, total}` for one +graded lane. Collect these into a `plan_coverage` frontmatter block — **only** when at least one +graded lane has `complete: false` (mirrors the existing `trimmed_reviewers` precedent: present +only when there is something to report): + +```yaml +plan_coverage: # only present if at least one graded lane is incomplete + : + total: 7 + missing: ["12.6-07"] +``` + Combine all review responses into `{phase_dir}/{padded_phase}-REVIEWS.md`: After all reviewers complete, collect trim metadata files written during the run. For each reviewer that was trimmed (i.e. a `.metadata.json` file exists and `hardFailed` or `omitted` is non-empty, or `projectMdShrunk` is true, or `planTruncationPct > 0`), include a `trimmed_reviewers` block in the frontmatter. Omit the key entirely if no reviewer was trimmed. @@ -580,6 +705,10 @@ trimmed_reviewers: # only present if at least one reviewer was trimmed plan_truncation_pct: 22 hard_failed: false note_injected: true +plan_coverage: # only present if at least one graded lane is incomplete (#3301) + ollama: + total: 7 + missing: ["12.6-07"] --- # Cross-AI Plan Review — Phase {N} diff --git a/scripts/lint-allow-test-rule-refs.unverified-ceiling.json b/scripts/lint-allow-test-rule-refs.unverified-ceiling.json index afdf1f882..ef98fecb9 100644 --- a/scripts/lint-allow-test-rule-refs.unverified-ceiling.json +++ b/scripts/lint-allow-test-rule-refs.unverified-ceiling.json @@ -1,3 +1,3 @@ { - "maxFiles": 282 + "maxFiles": 283 } diff --git a/tests/review-build-prompt-optional-sections.test.cjs b/tests/review-build-prompt-optional-sections.test.cjs index 7057e460e..01c7e15db 100644 --- a/tests/review-build-prompt-optional-sections.test.cjs +++ b/tests/review-build-prompt-optional-sections.test.cjs @@ -198,83 +198,68 @@ describe('#3300 build_prompt optional-section guards under nullglob', () => { }); for (const shell of SHELLS) { - test(`[${shell.name}] absent optional sources: no context/research output file is created at all`, () => { + test(`[${shell.name}] absent optional sources: no context/research output file is created at all`, (t) => { const fx = buildFixture({ '01-PLAN.md': 'plan\n' }); // the common case: PLAN only - try { - const res = runBlock(shell, extractBuildPromptBlock(), fx); - assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); - // Row 1/2 of the matrix — the failing-first core. Pre-fix these exist - // as 0-byte files (the `>` redirect creates them before cat blocks or - // EOFs); post-fix they must not exist at all. - assert.strictEqual( - readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), - null, - 'gsd-review-context.md must NOT be created when no *-CONTEXT.md exists', - ); - assert.strictEqual( - readIfPresent(path.join(fx.runDir, 'gsd-review-research.md')), - null, - 'gsd-review-research.md must NOT be created when no *-RESEARCH.md exists', - ); - // The always-on parts of the block still did their job. - assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-plan-00.md')), 'plan\n'); - } finally { - cleanup(fx.root); - } + t.after(() => cleanup(fx.root)); + const res = runBlock(shell, extractBuildPromptBlock(), fx); + assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); + // Row 1/2 of the matrix — the failing-first core. Pre-fix these exist + // as 0-byte files (the `>` redirect creates them before cat blocks or + // EOFs); post-fix they must not exist at all. + assert.strictEqual( + readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), + null, + 'gsd-review-context.md must NOT be created when no *-CONTEXT.md exists', + ); + assert.strictEqual( + readIfPresent(path.join(fx.runDir, 'gsd-review-research.md')), + null, + 'gsd-review-research.md must NOT be created when no *-RESEARCH.md exists', + ); + // The always-on parts of the block still did their job. + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-plan-00.md')), 'plan\n'); }); - test(`[${shell.name}] present optional sources: output matches the source exactly`, () => { + test(`[${shell.name}] present optional sources: output matches the source exactly`, (t) => { const fx = buildFixture({ '01-PLAN.md': 'plan\n', '01-CONTEXT.md': 'ctx\n', '01-RESEARCH.md': 'research\n', }); - try { - const res = runBlock(shell, extractBuildPromptBlock(), fx); - assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); - assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), 'ctx\n'); - assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-research.md')), 'research\n'); - } finally { - cleanup(fx.root); - } + t.after(() => cleanup(fx.root)); + const res = runBlock(shell, extractBuildPromptBlock(), fx); + assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), 'ctx\n'); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-research.md')), 'research\n'); }); - test(`[${shell.name}] research-only phase: context output stays absent`, () => { + test(`[${shell.name}] research-only phase: context output stays absent`, (t) => { const fx = buildFixture({ '01-PLAN.md': 'plan\n', '01-RESEARCH.md': 'research\n' }); - try { - const res = runBlock(shell, extractBuildPromptBlock(), fx); - assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); - assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), null); - assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-research.md')), 'research\n'); - } finally { - cleanup(fx.root); - } + t.after(() => cleanup(fx.root)); + const res = runBlock(shell, extractBuildPromptBlock(), fx); + assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), null); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-research.md')), 'research\n'); }); - test(`[${shell.name}] multiple matches concatenate in glob order (original cat semantics)`, () => { + test(`[${shell.name}] multiple matches concatenate in glob order (original cat semantics)`, (t) => { const fx = buildFixture({ '01-PLAN.md': 'plan\n', '01-CONTEXT.md': 'A\n', '02-CONTEXT.md': 'B\n', }); - try { - const res = runBlock(shell, extractBuildPromptBlock(), fx); - assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); - assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), 'A\nB\n'); - } finally { - cleanup(fx.root); - } + t.after(() => cleanup(fx.root)); + const res = runBlock(shell, extractBuildPromptBlock(), fx); + assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-context.md')), 'A\nB\n'); }); - test(`[${shell.name}] open, unconsumed stdin pipe: block completes within ${STDIN_BOUND_MS}ms`, async () => { + test(`[${shell.name}] open, unconsumed stdin pipe: block completes within ${STDIN_BOUND_MS}ms`, async (t) => { const fx = buildFixture({ '01-PLAN.md': 'plan\n' }); - try { - const res = await runBlockWithOpenStdin(shell, extractBuildPromptBlock(), fx); - assert.ok(!res.timedOut, `block blocked on stdin (killed at ${STDIN_BOUND_MS}ms) — cat ran operand-less`); - assert.strictEqual(res.code, 0, `block exited ${res.code}${res.signal ? ` (signal ${res.signal})` : ''}`); - } finally { - cleanup(fx.root); - } + t.after(() => cleanup(fx.root)); + const res = await runBlockWithOpenStdin(shell, extractBuildPromptBlock(), fx); + assert.ok(!res.timedOut, `block blocked on stdin (killed at ${STDIN_BOUND_MS}ms) — cat ran operand-less`); + assert.strictEqual(res.code, 0, `block exited ${res.code}${res.signal ? ` (signal ${res.signal})` : ''}`); }); } diff --git a/tests/review-plan-coverage-manifest.test.cjs b/tests/review-plan-coverage-manifest.test.cjs new file mode 100644 index 000000000..bd4931b96 --- /dev/null +++ b/tests/review-plan-coverage-manifest.test.cjs @@ -0,0 +1,372 @@ +// allow-test-rule: source-text-is-the-product (#3301) +// The workflow markdown IS the installed orchestration contract; these rows +// extract the real shipped bash and execute it, never a hand-copied duplicate. + +'use strict'; + +/** + * #3301 — reviewers in the cross-AI plan-review workflow are never told the + * plan ids or the total plan count, so a review that silently covers 6 of 7 + * plans is indistinguishable from one that covers all 7. + * + * Two behavioral seams, both extracted from the REAL shipped bash in + * gsd-core/workflows/review.md (the same pattern as + * tests/review-build-prompt-optional-sections.test.cjs): + * + * 1. build_prompt's plan-copy block — must derive a `.plans-manifest.md` + * (plan ids + total count) from the `*-PLAN.md` filenames and append it + * to both gsd-review-instructions.md and gsd-review-prompt.md. + * 2. write_reviews' plan-coverage-check block — must grade each dispatched + * lane's review file against that manifest, honoring two named regex + * traps (escaped ids, hyphen-boundary exclusion) and skipping stub/empty/ + * CodeRabbit lanes. + * + * Script transport is temp-FILE based, never `bash -c