diff --git a/.changeset/clever-cats-wake.md b/.changeset/clever-cats-wake.md new file mode 100644 index 000000000..35b6b8014 --- /dev/null +++ b/.changeset/clever-cats-wake.md @@ -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) diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index 97d2eb4df..7d983f44e 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -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-.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") diff --git a/tests/review-parallel-lanes.test.cjs b/tests/review-parallel-lanes.test.cjs index 393239984..abcefd090 100644 --- a/tests/review-parallel-lanes.test.cjs +++ b/tests/review-parallel-lanes.test.cjs @@ -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, {