* test(#3959): plan copies carry source names, prompt carries path anchors Regression coverage: budget copies named gsd-review-plan-<plan-id>.md (not a bare padded index), no bare-index copy survives, and the Plans to Review template instructs a per-plan repo-relative #### path header. Also corrects the stale -00 assertion to the source-named copy. * fix(#3959): give review-prompt plans citable path anchors The Plans to Review template now instructs a per-plan repo-relative #### path header, and the budget copies are named gsd-review-plan-<plan-id>.md instead of a bare padded index, restoring provenance for prompt-budget's per-plan headers while keeping the trim glob intact. Emitted-Drift-Ack-Growth: review.md — per-plan path-header instruction in Plans to Review + provenance-named copy loop (#3959) * chore(#3959): backfill changeset pr number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/clever-moles-rest.md
Normal file
5
.changeset/clever-moles-rest.md
Normal file
@@ -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)
|
||||
@@ -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/<phase>/<NN>-PLAN.md`). The path header is the citable anchor for findings about the plan itself — cite it as `<repo-relative plan path>:<line>` (name the heading in prose beside the citation if it helps the reader); reserve `path:line` for repo files the plan references.
|
||||
{per-plan: `#### <repo-relative plan path>` + 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 `### <file>` 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
|
||||
|
||||
@@ -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 `### <file>`
|
||||
// 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('#### <repo-relative'),
|
||||
`Plans to Review must instruct a #### path header per plan; got: ${section.slice(0, 200)}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user