fix(#4097): evidence preservation no longer sweeps the run's own input copies into .review-diagnostics/ (#4329)
* test(#4097): failing-first regression — run input copies swept into .review-diagnostics The present_results preserve+cleanup glob `_DIAG_MD=( "$RUN_DIR"/gsd-review-*.md )` matches both lane outputs and the run's own assembled input copies (prompt, instructions, roadmap, per-plan copies, project/context/research/requirements, per-lane trimmed prompts) because both share the gsd-review- prefix. Seed the full input-copy set into the existing runWriteReviewsFlow fixture and assert (a) the diagnostics dir holds exactly the lane report + .err sidecar and no input basenames, and (b) an inputs-only run creates no diagnostics dir at all and still cleans up. Both RED against the shipped block. * fix(#4097): preserve lane output only — exclude the run's input copies from evidence The preserve+cleanup glob treated every gsd-review-*.md in RUN_DIR as lane evidence, but the workflow itself writes the run's assembled INPUTS there under the same prefix (prompt, instructions, roadmap, per-plan copies, project/ context/research/requirements, per-lane trimmed prompts). Filter _DIAG_MD by basename against that closed input set instead. Direct glob iteration + case filter — identical under bash and zsh (#4099/ #4109), nullglob-safe (#2962), and the exclusion list is closed and owned in this step: a future input basename cannot silently rejoin the evidence set. Lane reports, diagnostic stubs and non-empty .err sidecars are unchanged. Emitted-Drift-Ack-Growth: review.md — #4097 narrows the present_results evidence-preservation glob: the closed input-basename exclusion list (case filter) plus its rationale note are deliberate additions so a future input basename cannot silently rejoin the evidence set. * changeset(#4097): fixed — review diagnostics no longer sweep run input copies * changeset(#4097): backfill PR number 4329 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/clever-cats-wake.md
Normal file
5
.changeset/clever-cats-wake.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4329
|
||||
---
|
||||
**`/gsd-review` no longer sweeps the run's own prompt/plan copies into `.review-diagnostics/`** — after a review, the preserved diagnostics folder is now dominated by actual evidence (reviewer reports and stderr sidecars) instead of byte-identical duplicates of the prompt, instructions, roadmap, and every plan under review. (#4097)
|
||||
@@ -845,6 +845,19 @@ NOT a preservation failure. This copy is deliberately NOT part of the commit abo
|
||||
step names only `{padded_phase}-REVIEWS.md` explicitly, never a directory glob, so
|
||||
`.review-diagnostics/` is never swept into it.
|
||||
|
||||
**#4097: preserve lane OUTPUT, never the run's own input copies.** `RUN_DIR` holds not only
|
||||
lane outputs — prompt assembly (the `gather_context`/section-copy step above) also writes the
|
||||
run's assembled INPUTS there under the same `gsd-review-` prefix: the combined prompt, the
|
||||
instructions/roadmap sections, a copy of every plan under review, the project/context/research/
|
||||
requirements sections, and the per-lane trimmed prompts. Those are byte-identical duplicates of
|
||||
files already committed under `.planning/`; sweeping them into `.review-diagnostics/` buries
|
||||
the actual evidence under plan duplicates and grows the phase directory on every run. The
|
||||
exclusion list below is CLOSED and owned here: this workflow itself writes every input
|
||||
basename at prompt-assembly time, so a future input file CANNOT silently join the evidence
|
||||
set — adding one means adding its stem to this list consciously. Lane slugs never begin with
|
||||
any excluded stem (`prompt`, `instructions`, `plan-`, `project`, `roadmap`, `context`,
|
||||
`research`, `requirements`), so a lane report can never be excluded by accident.
|
||||
|
||||
Preservation and cleanup MUST run in the same fenced block below (a shell variable cannot
|
||||
survive across separate fences — each is its own process). `mkdir -p` and every `cp` are
|
||||
exit-status checked; `rm -rf "$RUN_DIR"` runs ONLY if nothing was preserved (nothing to
|
||||
@@ -858,7 +871,20 @@ shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null
|
||||
RUN_DIR="{run_dir}"
|
||||
DIAG_DIR="{phase_dir}/.review-diagnostics"
|
||||
|
||||
_DIAG_MD=( "$RUN_DIR"/gsd-review-*.md )
|
||||
# #4097: `gsd-review-*.md` matches BOTH lane outputs (reports, diagnostic stubs) and the
|
||||
# run's own assembled input copies (see the #4097 note above). Filter by basename against
|
||||
# the closed input set this workflow itself writes — direct glob iteration with a `case`
|
||||
# filter, no string accumulator, identical under bash and zsh (#4099/#4109), and the
|
||||
# `nullglob` set at the top of this fence keeps an empty RUN_DIR an empty array (#2962).
|
||||
# `gsd-review-prompt*` deliberately covers BOTH the combined prompt (`gsd-review-prompt.md`)
|
||||
# and the per-lane trimmed prompts (`gsd-review-prompt-<slug>.md`).
|
||||
_DIAG_MD=()
|
||||
for f in "$RUN_DIR"/gsd-review-*.md; do
|
||||
case "$(basename "$f")" in
|
||||
gsd-review-prompt*|gsd-review-instructions*|gsd-review-plan-*|gsd-review-project*|gsd-review-roadmap*|gsd-review-context*|gsd-review-research*|gsd-review-requirements*) ;;
|
||||
*) _DIAG_MD+=("$f") ;;
|
||||
esac
|
||||
done
|
||||
_DIAG_ERR=()
|
||||
for f in "$RUN_DIR"/gsd-review-*.err; do
|
||||
[ -s "$f" ] && _DIAG_ERR+=("$f")
|
||||
|
||||
@@ -708,6 +708,14 @@ function runWriteReviewsFlow(t, opts) {
|
||||
fs.writeFileSync(path.join(phaseDir, '.review-diagnostics'), 'blocking file, not a directory\n');
|
||||
}
|
||||
|
||||
// #4097: stage the run's OWN input copies exactly as prompt assembly
|
||||
// (review.md's section-copy fence) writes them, so the preserve+cleanup
|
||||
// block can be observed deciding input vs. evidence. `name` is a bare
|
||||
// basename written into the fixture run dir.
|
||||
for (const [name, content] of Object.entries(opts.seedFiles || {})) {
|
||||
fs.writeFileSync(path.join(runDir, name), content);
|
||||
}
|
||||
|
||||
if (opts.jsonlLines && opts.jsonlLines.length > 0) {
|
||||
fs.writeFileSync(
|
||||
path.join(runDir, 'gsd-review-lane-results.jsonl'),
|
||||
@@ -997,6 +1005,85 @@ describe('#3885 failed preservation leaves run_dir intact (no silent swallow)',
|
||||
});
|
||||
});
|
||||
|
||||
describe('#4097 the run\'s own input copies are not preserved as diagnostics', () => {
|
||||
// The preserve+cleanup glob assumes every `gsd-review-*.md` in RUN_DIR is
|
||||
// lane OUTPUT. But the workflow itself writes the run's assembled INPUTS
|
||||
// into RUN_DIR under the same prefix at prompt-assembly time (review.md's
|
||||
// section-copy fence): the combined prompt, instructions, roadmap, one copy
|
||||
// of every plan under review, project/context/research/requirements
|
||||
// sections, and the per-lane trimmed prompts. On a multi-plan phase those
|
||||
// byte-identical `.planning/` duplicates bury the actual evidence (a
|
||||
// handful of reviewer reports and `.err` sidecars) and grow the phase
|
||||
// directory on every review run (#4097).
|
||||
const INPUT_COPIES_4097 = {
|
||||
'gsd-review-prompt.md': 'combined reviewer prompt\n',
|
||||
'gsd-review-instructions.md': 'instructions section\n',
|
||||
'gsd-review-roadmap.md': 'roadmap section\n',
|
||||
'gsd-review-project.md': 'PROJECT.md copy\n',
|
||||
'gsd-review-context.md': 'CONTEXT.md concatenation\n',
|
||||
'gsd-review-research.md': 'RESEARCH.md concatenation\n',
|
||||
'gsd-review-requirements.md': 'REQUIREMENTS.md copy\n',
|
||||
'gsd-review-plan-12.6-01.md': 'plan 12.6-01 duplicate\n',
|
||||
'gsd-review-plan-12.6-02.md': 'plan 12.6-02 duplicate\n',
|
||||
// Per-lane trimmed prompt written by prepare_trimmed_prompt_for_reviewer
|
||||
// — also a run input, also matched by the issue's `gsd-review-prompt*`
|
||||
// exclusion prefix.
|
||||
'gsd-review-prompt-claude.md': 'budget-trimmed prompt for lane claude\n',
|
||||
};
|
||||
|
||||
test('inputCopiesAreNotSweptIntoDiagnostics_4097', (t) => {
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'claude',
|
||||
jsonlLines: [{ slug: 'claude' }],
|
||||
seedFiles: INPUT_COPIES_4097,
|
||||
lanes: {
|
||||
claude: { md: '# Claude review\nok\n', err: 'mild stderr warning\n' },
|
||||
},
|
||||
});
|
||||
|
||||
assert.equal(result.outcome, 'exited');
|
||||
assert.equal(result.runDirExists, false, 'successful preservation must still clean up the run dir');
|
||||
assert.equal(result.diagDirExists, true, 'real lane evidence must still be preserved');
|
||||
|
||||
const preserved = fs.existsSync(result.diagDir)
|
||||
? fs.readdirSync(result.diagDir).sort()
|
||||
: [];
|
||||
// Only the lane's own output belongs in the diagnostics folder.
|
||||
assert.deepEqual(
|
||||
preserved,
|
||||
['gsd-review-claude.err', 'gsd-review-claude.md'],
|
||||
`diagnostics must hold exactly the lane report and stderr sidecar, not the run's input copies; got: ${preserved.join(', ')}`,
|
||||
);
|
||||
assert.equal(
|
||||
fs.readFileSync(path.join(result.diagDir, 'gsd-review-claude.md'), 'utf-8'),
|
||||
'# Claude review\nok\n',
|
||||
'the lane report must be preserved byte-identically',
|
||||
);
|
||||
for (const inputName of Object.keys(INPUT_COPIES_4097)) {
|
||||
assert.equal(
|
||||
fs.existsSync(path.join(result.diagDir, inputName)),
|
||||
false,
|
||||
`input copy ${inputName} must NOT be swept into .review-diagnostics/ (#4097)`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('inputsOnlyRunLeavesNoDiagnosticsDir_4097', (t) => {
|
||||
// Inputs only, no lane artifacts at all: inputs are not evidence, so
|
||||
// this is the "nothing to preserve" branch — no diagnostics directory is
|
||||
// created and cleanup proceeds (#4097 narrowing of #3352's glob).
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'claude',
|
||||
jsonlLines: [],
|
||||
seedFiles: INPUT_COPIES_4097,
|
||||
lanes: {},
|
||||
});
|
||||
|
||||
assert.equal(result.runDirExists, false, 'nothing to preserve is not a failure — run dir must still be removed');
|
||||
assert.equal(result.diagDirExists, false, 'a run that produced only input copies has no evidence to preserve');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3352 preserved evidence is never swept into the commit (N6)', () => {
|
||||
test('preservedEvidenceIsNotSweptIntoTheCommit', (t) => {
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
|
||||
Reference in New Issue
Block a user