diff --git a/.changeset/clever-moles-rest.md b/.changeset/clever-moles-rest.md new file mode 100644 index 000000000..969c7c2cb --- /dev/null +++ b/.changeset/clever-moles-rest.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4170 +--- +**/gsd-review no longer down-weights plan-grounded reviews for a citation shape the prompt made impossible** — each plan in the review prompt now carries a repo-relative #### path header, and the budget copies are named after their source plan id instead of a bare index, so source-grounded lanes can cite plans in a form the consensus step resolves. (#3959) diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index cd2c2f15c..93e292c28 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -208,7 +208,8 @@ Provide structured feedback on plan quality, completeness, and risks. {research if present} ### Plans to Review -{all PLAN.md contents} +For each `*-PLAN.md` in the phase directory, in glob order, include its full content preceded by a `####` header carrying the plan's **repo-relative path** (e.g. `#### .planning/phases//-PLAN.md`). The path header is the citable anchor for findings about the plan itself — cite it as `:` (name the heading in prose beside the citation if it helps the reader); reserve `path:line` for repo files the plan references. +{per-plan: `#### ` + full plan contents} ## Review Instructions @@ -261,12 +262,15 @@ RUN_DIR="{run_dir}" # from gather_context cp "$INSTRUCTIONS_BLOCK_FILE" "${RUN_DIR}/gsd-review-instructions.md" cp "$ROADMAP_SECTION_FILE" "${RUN_DIR}/gsd-review-roadmap.md" -# Plan files: copy each PLAN.md to a predictable numbered path -PLAN_INDEX=0 +# Plan files: copy each PLAN.md to a predictable path named after its source +# plan id (#3959: a bare padded index discards provenance — the budget tool's +# per-plan `### ` header then renders a run-dir artifact name no reviewer +# or consensus step can resolve. The plan id keeps the gsd-review-plan-*.md glob +# prepare_trimmed_prompt_for_reviewer consumes.) for PLAN_FILE in "${PHASE_DIR}"/*-PLAN.md; do - PADDED_IDX=$(printf '%02d' "$PLAN_INDEX") - cp "$PLAN_FILE" "${RUN_DIR}/gsd-review-plan-${PADDED_IDX}.md" - PLAN_INDEX=$((PLAN_INDEX + 1)) + PLAN_BASENAME=$(basename "$PLAN_FILE") + PLAN_ID="${PLAN_BASENAME%-PLAN.md}" + cp "$PLAN_FILE" "${RUN_DIR}/gsd-review-plan-${PLAN_ID}.md" done # #3301: plan coverage manifest — tell reviewers exactly which plan ids exist and diff --git a/tests/review-build-prompt-optional-sections.test.cjs b/tests/review-build-prompt-optional-sections.test.cjs index ef2ece3c3..f59e7fa94 100644 --- a/tests/review-build-prompt-optional-sections.test.cjs +++ b/tests/review-build-prompt-optional-sections.test.cjs @@ -227,8 +227,11 @@ describe('#3300 build_prompt optional-section guards under nullglob', () => { 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'); + // The always-on parts of the block still did their job (#3959: the copy + // is named after the source plan id, not the bare padded index). + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-plan-01.md')), 'plan\n'); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-plan-00.md')), null, + 'the bare-index copy name is gone (#3959)'); }); test(`[${shell.name}] present optional sources: output matches the source exactly`, (t) => { @@ -291,3 +294,40 @@ describe('#3300 build_prompt optional-section guards under nullglob', () => { ); }); }); + +// ─── #3959: plan copies carry source provenance, prompt carries path anchors ── + +describe('#3959 plan-copy provenance and path anchors', () => { + for (const shell of SHELLS) { + test(`[${shell.name}] plan copies are named after their source plan id, not a bare index`, (t) => { + const fx = buildFixture({ + '01-PLAN.md': 'plan one\n', + '02-PLAN.md': 'plan two\n', + }); + t.after(() => cleanup(fx.root)); + const res = runBlock(shell, extractBuildPromptBlock(), fx); + assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`); + // Source-named copies: reviewers and the budget tool's `### ` + // header (src/prompt-budget.cts) see a resolvable plan identity, and + // the prepare_trimmed_prompt_for_reviewer glob still matches. + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-plan-01.md')), 'plan one\n'); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-plan-02.md')), 'plan two\n'); + assert.strictEqual(readIfPresent(path.join(fx.runDir, 'gsd-review-plan-00.md')), null, + 'bare-index copy names must not survive (#3959)'); + }); + } + + test('Plans to Review template carries per-plan repo-relative path headers', () => { + // source-text-is-the-product: the workflow markdown IS the assembled + // prompt's contract. #3959: plan bodies inlined with no path anchor leave + // a source-grounded lane nothing citable that SOURCE_CITATION_RE accepts. + const content = readWorkflowCombined(REVIEW_WORKFLOW); + const anchorIdx = content.indexOf('### Plans to Review'); + assert.notEqual(anchorIdx, -1, '### Plans to Review section not found in review.md (+steps)'); + const section = content.slice(anchorIdx, anchorIdx + 600); + assert.ok( + /####.*path/i.test(section) || section.includes('####