golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 3 Packages Projects Releases Wiki Activity

enhance(#3301): tell reviewers the plan ids and total count, grade coverage (#4084)

Browse Source 
* 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-<slug>.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 <sim@local>
This commit is contained in:
Tom Boucher
2026-08-30 14:25:24 -04:00
committed by GitHub
parent 86452da7cb
commit 6eea00b707
6 changed files with 550 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

129
gsd-core/workflows/review.md
View File

@@ -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("(?<![\\w-])" + escapeRegex(id) + "(?![\\w-])");
return !re.test(review);
});
process.stdout.write(JSON.stringify({ complete: missing.length === 0, missing_ids: missing, total: ids.length }));
' "$MANIFEST" "$REVIEW_FILE" > "$RUN_DIR/.plan-coverage-$SLUG.json"
done
```
Each `${RUN_DIR}/.plan-coverage-<slug>.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
<slug>:
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}