* fix(#4351): run-scope preserved review evidence The preserve block copied lane output into a flat .review-diagnostics/ using each file's source basename. A lane slug is stable across runs, so the destination was stable across runs too -- and cp over an existing file is a success, so a second review of the same phase destroyed the first run's evidence with no error and no warning, in the one directory that exists to outlive the rm -rf beside it. Copy into one subdirectory per run instead. $RUN_DIR is mktemp -d, so its basename is already unique per run by construction; the UTC stamp in front is only a sort key and is omitted if date fails. Destination-only: nothing inside $RUN_DIR is renamed, because both prepare_trimmed_prompt_for_reviewer and the lane invocation resolver depend on those exact basenames. Verified by extracting the real fence and running it twice against one phase dir: before, one report survived and it was run 2's; after, both. Emitted-Drift-Ack-Growth: review.md — per-run diagnostics subdirectory plus the comment explaining why uniqueness cannot come from the clock Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#4351): cover repeated runs, resolve through the run subdir Adds the two-run rows the issue asks for: both runs' reports recoverable, and a lane failing identically twice leaving two stubs. Both assert on CONTENT, not a file count -- a clobber producing the same number of files would pass a count-only check, and the defect is that run 1's bytes were replaced. runWriteReviewsFlow gains an optional phaseDir so a caller can run the flow twice against one phase directory, which is the only arrangement that can observe the overwrite. Omitted, it mints a fresh one as before. The existing rows asserted a flat readdir of the diagnostics root, which the fix makes stale. They now resolve through preservedPath/preservedNames so they keep asserting WHICH files were preserved rather than silently becoming assertions about the layout; the layout is pinned once, explicitly, by oneRunSubdirectoryPerRun_4351. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4351): add changeset fragment Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4351): name the run, not the clock, in the changeset Adversarial review: the fragment said each run gets its own "timestamped subdirectory", which reads as though the timestamp provides the separation. It does not -- collision-safety is mktemp's random basename, and the stamp is a sort key that is dropped entirely when date fails. The code comment already said so; the user-facing text now agrees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4351): backfill the changeset PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/rapid-eagles-roam.md
Normal file
5
.changeset/rapid-eagles-roam.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4713
|
||||
---
|
||||
**Repeated reviews no longer overwrite each other's preserved evidence** — a second review of the same phase silently replaced the first run's lane reports and error stubs in `.review-diagnostics/`, because the copy used each file's lane-named basename and a lane slug is stable across runs. Each run's evidence now lands in its own subdirectory, named after that run, so repeat reviews accumulate diagnostics instead of clobbering them. (#4351)
|
||||
@@ -892,11 +892,28 @@ done
|
||||
|
||||
_PRESERVE_OK=true
|
||||
if [ ${#_DIAG_MD[@]} -gt 0 ] || [ ${#_DIAG_ERR[@]} -gt 0 ]; then
|
||||
if mkdir -p "$DIAG_DIR"; then
|
||||
if [ ${#_DIAG_MD[@]} -gt 0 ] && ! cp "${_DIAG_MD[@]}" "$DIAG_DIR/"; then
|
||||
# #4351: ONE SUBDIRECTORY PER RUN. A flat copy used each file's SOURCE basename,
|
||||
# and a lane slug is stable across runs — so a second review of the same phase
|
||||
# silently overwrote the first run's evidence for any lane that ran both times.
|
||||
# `cp` over an existing file is a success, so this lost data with no warning, in
|
||||
# the one directory whose entire purpose is to outlive the `rm -rf` below.
|
||||
#
|
||||
# $RUN_DIR is `mktemp -d .../gsd-review-XXXXXX`, so its basename is already
|
||||
# unique per run BY CONSTRUCTION — uniqueness never depends on the clock. The
|
||||
# UTC stamp is only a sort key in front of it, and is omitted entirely if `date`
|
||||
# fails. Lane basenames are unchanged INSIDE the subdirectory, so evidence still
|
||||
# correlates back to its lane, and nothing inside $RUN_DIR is renamed (both
|
||||
# `prepare_trimmed_prompt_for_reviewer` and the lane invocation resolver depend
|
||||
# on those exact basenames).
|
||||
_DIAG_STAMP="$(date -u +%Y%m%dT%H%M%SZ 2>/dev/null || true)"
|
||||
_DIAG_RUN_DIR="$DIAG_DIR/${_DIAG_STAMP:+${_DIAG_STAMP}-}$(basename "$RUN_DIR")"
|
||||
# `mkdir -p` still creates $DIAG_DIR itself, so a plain FILE sitting at
|
||||
# $DIAG_DIR still fails here and still skips the `rm -rf` (#3885).
|
||||
if mkdir -p "$_DIAG_RUN_DIR"; then
|
||||
if [ ${#_DIAG_MD[@]} -gt 0 ] && ! cp "${_DIAG_MD[@]}" "$_DIAG_RUN_DIR/"; then
|
||||
_PRESERVE_OK=false
|
||||
fi
|
||||
if [ ${#_DIAG_ERR[@]} -gt 0 ] && ! cp "${_DIAG_ERR[@]}" "$DIAG_DIR/"; then
|
||||
if [ ${#_DIAG_ERR[@]} -gt 0 ] && ! cp "${_DIAG_ERR[@]}" "$_DIAG_RUN_DIR/"; then
|
||||
_PRESERVE_OK=false
|
||||
fi
|
||||
else
|
||||
@@ -907,7 +924,7 @@ fi
|
||||
if [ "$_PRESERVE_OK" = "true" ]; then
|
||||
rm -rf "$RUN_DIR"
|
||||
else
|
||||
echo "WARNING: evidence preservation to $DIAG_DIR failed — leaving the un-preserved run directory intact at: $RUN_DIR" >&2
|
||||
echo "WARNING: evidence preservation to ${_DIAG_RUN_DIR:-$DIAG_DIR} failed — leaving the un-preserved run directory intact at: $RUN_DIR" >&2
|
||||
fi
|
||||
```
|
||||
</step>
|
||||
|
||||
@@ -694,14 +694,57 @@ function extractPresentResultsPreserveAndCleanupBash() {
|
||||
* "not a directory" for every caller, root included, because it is not a
|
||||
* permissions check at all.
|
||||
*/
|
||||
|
||||
// #4351: preserved evidence now lands in a PER-RUN SUBDIRECTORY under
|
||||
// `.review-diagnostics/`. A flat copy used each file's SOURCE basename, and a
|
||||
// lane slug is stable across runs, so a second review of the same phase silently
|
||||
// overwrote the first run's evidence.
|
||||
//
|
||||
// These helpers resolve THROUGH that subdirectory layer on purpose. Rows that
|
||||
// assert WHICH files were preserved should keep asserting exactly that; without
|
||||
// them each such row would quietly become an assertion about the directory
|
||||
// LAYOUT instead, and would have to be rewritten again the next time the layout
|
||||
// moves. The layout itself is pinned once, explicitly, by
|
||||
// `oneRunSubdirectoryPerRun_4351`.
|
||||
function preservedRunDirs(result) {
|
||||
if (!fs.existsSync(result.diagDir)) return [];
|
||||
return fs.readdirSync(result.diagDir)
|
||||
.map((name) => path.join(result.diagDir, name))
|
||||
.filter((p) => fs.statSync(p).isDirectory())
|
||||
.sort();
|
||||
}
|
||||
|
||||
function preservedNames(result) {
|
||||
return preservedRunDirs(result)
|
||||
.flatMap((dir) => fs.readdirSync(dir))
|
||||
.sort();
|
||||
}
|
||||
|
||||
// Resolves a preserved basename to its real path. When the file was NOT
|
||||
// preserved this deliberately returns a path that does not exist, so a negative
|
||||
// assertion (`existsSync(...) === false`) still reads naturally at the call site.
|
||||
function preservedPath(result, name) {
|
||||
for (const dir of preservedRunDirs(result)) {
|
||||
const candidate = path.join(dir, name);
|
||||
if (fs.existsSync(candidate)) return candidate;
|
||||
}
|
||||
return path.join(result.diagDir, name);
|
||||
}
|
||||
|
||||
function runWriteReviewsFlow(t, opts) {
|
||||
const scriptDir = createTempDir('gsd-3352-script-');
|
||||
const runDir = createTempDir('gsd-3352-rundir-');
|
||||
const phaseDir = createTempDir('gsd-3352-phasedir-');
|
||||
// #4351: an explicit phaseDir lets a caller run this flow TWICE against the
|
||||
// SAME phase directory, which is the only way to observe a second run
|
||||
// clobbering the first run's preserved evidence. Omitted (every pre-existing
|
||||
// caller) it mints a fresh one exactly as before.
|
||||
const phaseDir = opts.phaseDir || createTempDir('gsd-3352-phasedir-');
|
||||
t.after(() => {
|
||||
cleanup(scriptDir);
|
||||
cleanup(runDir);
|
||||
cleanup(phaseDir);
|
||||
// A caller-supplied phaseDir is owned by the caller — cleaning it here would
|
||||
// destroy run 1's evidence before run 2 could be asserted against it.
|
||||
if (!opts.phaseDir) cleanup(phaseDir);
|
||||
});
|
||||
|
||||
if (opts.blockDiagDirWithFile) {
|
||||
@@ -934,15 +977,15 @@ describe('#3352 per-lane evidence survives cleanup (R3)', () => {
|
||||
assert.equal(result.runDirExists, false, 'the run dir must still be destroyed');
|
||||
assert.equal(result.diagDirExists, true, 'diagnostics must have been preserved somewhere under phase_dir');
|
||||
|
||||
const codexMd = path.join(result.diagDir, 'gsd-review-codex.md');
|
||||
const codexErr = path.join(result.diagDir, 'gsd-review-codex.err');
|
||||
const claudeErr = path.join(result.diagDir, 'gsd-review-claude.err');
|
||||
const codexMd = preservedPath(result, 'gsd-review-codex.md');
|
||||
const codexErr = preservedPath(result, 'gsd-review-codex.err');
|
||||
const claudeErr = preservedPath(result, 'gsd-review-claude.err');
|
||||
assert.ok(fs.existsSync(codexMd), 'codex .md evidence must be preserved');
|
||||
assert.equal(fs.readFileSync(codexMd, 'utf-8'), LANE_FAILURE_MD('codex'));
|
||||
assert.ok(fs.existsSync(codexErr), 'codex non-empty .err evidence must be preserved');
|
||||
assert.ok(fs.existsSync(claudeErr), 'claude non-empty .err evidence must be preserved');
|
||||
|
||||
const geminiErr = path.join(result.diagDir, 'gsd-review-gemini.err');
|
||||
const geminiErr = preservedPath(result, 'gsd-review-gemini.err');
|
||||
assert.equal(fs.existsSync(geminiErr), false, 'an empty .err must not be copied as if it were real evidence');
|
||||
});
|
||||
});
|
||||
@@ -1045,9 +1088,7 @@ describe('#4097 the run\'s own input copies are not preserved as diagnostics', (
|
||||
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()
|
||||
: [];
|
||||
const preserved = preservedNames(result);
|
||||
// Only the lane's own output belongs in the diagnostics folder.
|
||||
assert.deepEqual(
|
||||
preserved,
|
||||
@@ -1055,13 +1096,13 @@ describe('#4097 the run\'s own input copies are not preserved as diagnostics', (
|
||||
`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'),
|
||||
fs.readFileSync(preservedPath(result, '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)),
|
||||
fs.existsSync(preservedPath(result, inputName)),
|
||||
false,
|
||||
`input copy ${inputName} must NOT be swept into .review-diagnostics/ (#4097)`,
|
||||
);
|
||||
@@ -1117,3 +1158,107 @@ describe('#3352 preserved evidence is never swept into the commit (N6)', () => {
|
||||
assert.equal(afterFiles.split(/\s+/).length, 1, `--files must carry exactly one path; got: ${afterFiles}`);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #4351: repeated runs must accumulate evidence, never clobber it ────────
|
||||
//
|
||||
// The preserve block copies lane output into `.review-diagnostics/` using each
|
||||
// file's SOURCE basename (`gsd-review-<slug>.md` / `.err`). A lane slug is
|
||||
// stable across runs, so before this fix the destination path was stable across
|
||||
// runs too — and `cp` over an existing file is a SUCCESS, so a second review of
|
||||
// the same phase destroyed the first run's evidence with no error and no
|
||||
// warning, in the one directory that exists to outlive `rm -rf "$RUN_DIR"`.
|
||||
//
|
||||
// These rows run the REAL extracted fence twice against ONE phase directory,
|
||||
// which is the only arrangement that can observe the overwrite.
|
||||
describe('#4351 repeated review runs accumulate evidence instead of clobbering it', () => {
|
||||
test('twoRunsOnOnePhaseBothPreserved_4351', (t) => {
|
||||
const sharedPhaseDir = createTempDir('gsd-4351-phasedir-');
|
||||
t.after(() => cleanup(sharedPhaseDir));
|
||||
|
||||
const first = runWriteReviewsFlow(t, {
|
||||
phaseDir: sharedPhaseDir,
|
||||
selected: 'codex',
|
||||
jsonlLines: [{ slug: 'codex' }],
|
||||
lanes: { codex: { md: '# run one report\n', err: 'run one stderr\n' } },
|
||||
});
|
||||
assert.equal(first.outcome, 'exited');
|
||||
assert.equal(first.diagDirExists, true, 'run 1 must preserve its evidence');
|
||||
|
||||
const second = runWriteReviewsFlow(t, {
|
||||
phaseDir: sharedPhaseDir,
|
||||
selected: 'codex',
|
||||
jsonlLines: [{ slug: 'codex' }],
|
||||
lanes: { codex: { md: '# run two report\n', err: 'run two stderr\n' } },
|
||||
});
|
||||
assert.equal(second.outcome, 'exited');
|
||||
|
||||
// Assert on CONTENT, not on a file count: a clobber that happened to produce
|
||||
// the same number of files would pass a count-only check, and the whole
|
||||
// defect is that run 1's bytes were replaced by run 2's.
|
||||
const runDirs = preservedRunDirs(second);
|
||||
assert.equal(
|
||||
runDirs.length, 2,
|
||||
`each run must own a distinct subdirectory; got ${runDirs.length}: ${runDirs.map((d) => path.basename(d)).join(', ')}`,
|
||||
);
|
||||
|
||||
const reports = runDirs
|
||||
.map((dir) => path.join(dir, 'gsd-review-codex.md'))
|
||||
.filter((p) => fs.existsSync(p))
|
||||
.map((p) => readFileNormalized(p));
|
||||
assert.deepEqual(
|
||||
reports.sort(),
|
||||
['# run one report\n', '# run two report\n'].sort(),
|
||||
'BOTH runs\' reports must be recoverable — before #4351 only the second survived',
|
||||
);
|
||||
});
|
||||
|
||||
test('identicalLaneFailureTwiceLeavesTwoStubs_4351', (t) => {
|
||||
// The 2nd acceptance criterion, and the worst case for a flat layout: the
|
||||
// SAME lane failing the SAME way twice produces byte-identical basenames.
|
||||
const sharedPhaseDir = createTempDir('gsd-4351-phasedir-err-');
|
||||
t.after(() => cleanup(sharedPhaseDir));
|
||||
|
||||
const opts = {
|
||||
phaseDir: sharedPhaseDir,
|
||||
selected: 'claude',
|
||||
jsonlLines: [],
|
||||
lanes: { claude: { md: '# lane failed\n', err: 'identical stack trace\n' } },
|
||||
};
|
||||
runWriteReviewsFlow(t, opts);
|
||||
const second = runWriteReviewsFlow(t, opts);
|
||||
|
||||
const stubs = preservedRunDirs(second)
|
||||
.map((dir) => path.join(dir, 'gsd-review-claude.err'))
|
||||
.filter((p) => fs.existsSync(p));
|
||||
assert.equal(
|
||||
stubs.length, 2,
|
||||
'two identical failures must leave two independently recoverable stubs',
|
||||
);
|
||||
for (const stub of stubs) {
|
||||
assert.equal(readFileNormalized(stub), 'identical stack trace\n');
|
||||
}
|
||||
});
|
||||
|
||||
test('oneRunSubdirectoryPerRun_4351', (t) => {
|
||||
// Pins the LAYOUT itself, once, so the helpers above stay honest: a single
|
||||
// run creates exactly one subdirectory, the lane basenames are unchanged
|
||||
// INSIDE it (nothing in $RUN_DIR was renamed — two consumers depend on those
|
||||
// names), and no evidence is left sitting flat in the diagnostics root.
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex',
|
||||
jsonlLines: [{ slug: 'codex' }],
|
||||
lanes: { codex: { md: '# only run\n', err: 'only stderr\n' } },
|
||||
});
|
||||
|
||||
const runDirs = preservedRunDirs(result);
|
||||
assert.equal(runDirs.length, 1, 'one run must create exactly one subdirectory');
|
||||
assert.deepEqual(
|
||||
fs.readdirSync(runDirs[0]).sort(),
|
||||
['gsd-review-codex.err', 'gsd-review-codex.md'],
|
||||
'lane basenames must be unchanged inside the run subdirectory',
|
||||
);
|
||||
const flat = fs.readdirSync(result.diagDir)
|
||||
.filter((name) => fs.statSync(path.join(result.diagDir, name)).isFile());
|
||||
assert.deepEqual(flat, [], 'no evidence may be left flat in the diagnostics root');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user