fix(review): an answer after a stopped prepare no longer locks the rounds

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-11-04 10:05:04 +01:00
commit d81bd39754
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 479 additions and 11 deletions

View file

@ -0,0 +1,427 @@
// tests/lib/review-rounds-stopped-prepare.test.mjs
// [RSP] A round-2 `prepare` that stopped (the archive of round 1 is written,
// review.md is still round 1), then an answer recorded on review.md, then
// `prepare` again: review.md now differs from its archive by the `## Answers`
// row (and, for `fixed`, its R-step), not by its findings. `prepare`
// recognises the archive as this review's own (same round number, same
// trailing JSON block, same scope_sha_end), refreshes it with review.md and
// returns round 2, so the answer is not counted as an extra round and is
// carried through the refreshed archive. An archive that cannot be read is
// "not measured", never a crash.
// `keep:` cases hold at the base already and stay untagged.
// Modules load through `import * as m`, so a missing export is a failed
// assertion at the red base, never a crash. Every child process gets
// childEnv(). The fixtures are copied from tests/lib/review-rounds.test.mjs
// (`rndReview`, `rndRepo`, `cryFlow`), which earlier red wrappers list.
import { after, test } from 'node:test';
import { strict as assert } from 'node:assert';
import { spawnSync } from 'node:child_process';
import {
appendFileSync, chmodSync, copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync,
} from 'node:fs';
import { tmpdir } from 'node:os';
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';
import * as rounds from '../../lib/review/review-rounds.mjs';
import { computeFindingIds } from '../../lib/parsers/finding-id.mjs';
import { childEnv } from '../helpers/child-env.mjs';
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
const ROUNDS = join(ROOT, 'lib', 'review', 'review-rounds.mjs');
const FINALIZE = join(ROOT, 'lib', 'review', 'review-finalize.mjs');
const ANSWERS = join(ROOT, 'lib', 'review', 'review-answers.mjs');
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
const PROJECT_REL = '.claude/projects/2026-11-03-s';
const TRAILER = 'Co-Authored-By: Claude Test <noreply@example.invalid>';
const CCR = 'code-correctness-reviewer';
const REASON = 'the parser keeps the old contract on purpose';
const made = [];
const locked = [];
after(() => {
for (const p of locked) { try { chmodSync(p, 0o644); } catch { /* gone */ } }
for (const b of made) rmSync(b, { recursive: true, force: true });
});
function g(cwd, ...args) {
const r = spawnSync('git', args, { cwd, encoding: 'utf8', env: childEnv() });
if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`);
return r.stdout.trim();
}
function putIn(dir, rel, text) {
mkdirSync(dirname(join(dir, rel)), { recursive: true });
writeFileSync(join(dir, rel), text);
}
const parse = (r) => { try { return JSON.parse(r.stdout); } catch { return {}; } };
const run = (args, opts = {}) => spawnSync(process.execPath, args, { encoding: 'utf8', env: childEnv(), ...opts });
// Ten lines `export const <tag><n> = <n>;`; `changes` maps a line number to new text.
const ten = (tag, changes = {}) => Array.from({ length: 10 }, (_, i) => (i + 1 in changes ? changes[i + 1] : `export const ${tag}${i + 1} = ${i + 1};`)).join('\n') + '\n';
const jsonOf = (text) => JSON.parse(text.slice(text.lastIndexOf('```json\n') + 8, text.lastIndexOf('```')));
/** A finding as a reviewer writes it (copied from tests/lib/review-rounds.test.mjs `finding`). */
function finding(f, i) {
return {
...(f.id ? { id: f.id } : {}),
severity: f.severity, rule_key: f.rule_key ?? 'MISSING_TEST', ...(f.engine ? { engine: f.engine } : {}),
file: f.file, line: f.line, brief_ref: 'SC1', title: f.title ?? `title ${f.file} ${f.line} ${i + 1}`,
detail: f.detail ?? `detail ${f.file} ${f.line} ${i + 1}`, recommended_action: f.action ?? 'Add a test.',
...(f.carried_from ? { carried_from: f.carried_from } : {}),
};
}
/** A review_version 1.2 review of `start..end` (copied from tests/lib/review-rounds.test.mjs `rndReview`). */
function rndReview({ start, end, findings, verdict }) {
const list = findings.map(finding);
const ids = computeFindingIds(list);
list.forEach((f, i) => { if (!f.id) f.id = ids[i]; });
const ordered = list.map(({ id, ...rest }) => ({ id, ...rest }));
const counts = { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
for (const f of ordered) counts[f.severity] += 1;
const v = verdict ?? (counts.BLOCKER > 0 ? 'BLOCK' : counts.MAJOR > 0 ? 'WARN' : 'ALLOW');
const json = JSON.stringify({ verdict: v, counts, allow_blocked_by: [], findings: ordered }, null, 2);
const sections = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'].map((sev) => {
const of = ordered.filter((f) => f.severity === sev);
if (of.length === 0) return '';
return `## Findings (${sev})\n\n${of.map((f) => `### ${f.id}\n\n- file: ${f.file}\n- line: ${f.line}\n- rule_key: ${f.rule_key}\n- title: ${f.title}\n`).join('\n')}\n`;
}).join('');
return {
ids: ordered.map((f) => f.id),
text: `---
type: trekreview
review_version: "1.2"
task: "t"
slug: s
project_dir: ${PROJECT_REL}/
brief_path: ${PROJECT_REL}/brief.md
scope_sha_start: ${start}
scope_sha_end: ${end}
reviewed_files_count: 2
verdict: ${v}
findings:
${ordered.length ? ordered.map((f) => ` - ${f.id}`).join('\n') : ' []'}
---
# Review: t
## Executive Summary
x
## Coverage
| File | Treatment | Reason |
|------|-----------|--------|
| a.mjs | deep-review | default |
${sections}## Declined to Judge
None.
Trimmed: 0
## Remediation Steps
None.
## Remediation Summary
x
\`\`\`json
${json}
\`\`\`
`.replace('findings:\n []', 'findings: []'),
};
}
/**
* A throwaway repo (copied from tests/lib/review-rounds.test.mjs `rndRepo`):
* start, c1 (round 1's work), c2 (round 2's work); HEAD is c2. The project
* folder is excluded from git, as a local artifact.
*/
function rndRepo() {
const base = mkdtempSync(join(tmpdir(), 'review-rounds-stopped-'));
made.push(base);
const repo = join(base, 'repo');
mkdirSync(repo, { recursive: true });
mkdirSync(join(base, 'hooks-none'));
g(repo, 'init', '-q', '-b', 'main');
appendFileSync(join(repo, '.git', 'config'), '[user]\n\tname = Test\n\temail = test@example.invalid\n'
+ `[commit]\n\tgpgsign = false\n[core]\n\thooksPath = "${join(base, 'hooks-none')}"\n`);
appendFileSync(join(repo, '.git', 'info', 'exclude'), '.claude/projects/\n');
const commit = (files, msg) => {
for (const [rel, text] of Object.entries(files)) putIn(repo, rel, text);
g(repo, 'add', '-A');
g(repo, 'commit', '-q', '-m', msg);
return g(repo, 'rev-parse', 'HEAD');
};
const start = commit({ 'a.mjs': ten('a'), 'b.mjs': ten('b') }, 'chore: start');
const c1 = commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;' }), 'b.mjs': ten('b', { 3: 'export const b3 = 33;' }) }, 'feat: round 1');
const c2 = commit({ 'a.mjs': ten('a', { 3: 'export const a3 = 33;', 5: 'export const a5 = 55;' }) }, 'feat: round 2');
const project = join(repo, ...PROJECT_REL.split('/'));
mkdirSync(project, { recursive: true });
const reviewPath = join(project, 'review.md');
const fx = {
base, repo, start, c1, c2, project, reviewPath, run: start.slice(0, 12),
write: (text, p = reviewPath) => writeFileSync(p, text),
read: (p = reviewPath) => readFileSync(p, 'utf8'),
archive: (n) => join(project, `review-round-${start.slice(0, 12)}-${n}.md`),
archives: () => readdirSync(project).filter((n) => n.startsWith('review-round-')).sort(),
prepare: (extra = []) => run([ROUNDS, 'prepare', '--project', project, '--since', start, '--json', ...extra], { cwd: repo }),
finalize: (round, extra = []) => run([FINALIZE, '--review', reviewPath, '--repo', repo, '--project', project,
'--round', String(round), ...extra], { cwd: repo }),
finalize2: () => fx.finalize(2, ['--round-base', c1, '--previous', fx.archive(1)]),
gate: () => run([GATE, '--json', reviewPath, '--repo', repo]),
record: (id, answer) => run([ANSWERS, 'record', '--review', reviewPath, '--finding', id, '--answer', answer, '--repo', repo,
'--round', '1', '--store', 'local', '--trailer', TRAILER], { input: REASON }),
carry: () => run([ANSWERS, 'carry', '--review', reviewPath, '--previous', fx.archive(1), '--repo', repo]),
out: (reviewer, previous = [], findings = []) => writeFileSync(join(project, `.review-out-${reviewer}.md`),
`Reviewer prose.\n\n\`\`\`json\n${JSON.stringify({ reviewer, findings, previous }, null, 2)}\n\`\`\`\n`),
};
return fx;
}
const MAJ_A3 = { severity: 'MAJOR', file: 'a.mjs', line: 3, title: 'Parser drops the last token', detail: 'parse() stops one token early' };
/** Round 1 finalized (round: 1), then a round-2 prepare that archived it and stopped. Returns the MAJOR's id. */
function stoppedRoundTwo(fx) {
fx.write(rndReview({ start: fx.start, end: fx.c1, findings: [MAJ_A3] }).text);
fx.out(CCR);
const f1 = fx.finalize(1);
assert.equal(f1.status, 0, `round-1 finalize: ${f1.stdout}${f1.stderr}`);
assert.match(fx.read(), /^round: 1$/m, 'finalize wrote round: 1');
const p = fx.prepare();
assert.equal(p.status, 0, `first prepare: ${p.stdout}${p.stderr}`);
assert.equal(parse(p).round, 2, p.stdout);
assert.equal(fx.read(fx.archive(1)), fx.read(), 'the first prepare archived review.md byte for byte');
return jsonOf(fx.read()).findings[0].id;
}
/** The review with `## Remediation Steps` holding one R-step whose source is `id`. */
function withRStep(text, id) {
const head = '## Remediation Steps\n\n';
const at = text.indexOf(head);
assert.ok(at >= 0, 'the review has a ## Remediation Steps section');
const end = text.indexOf('\n## ', at + head.length);
const step = `### Step R1: fix the parser\n\n- **Files:** \`a.mjs\`\n- **Source finding:** \`${id}\`\n`
+ '- **Test first:** a test that fails today\n- **Verify:** re-run /trekreview\n- **On failure:** escalate\n';
return `${text.slice(0, at + head.length)}${step}${text.slice(end)}`;
}
/** The review with its JSON block's findings changed by `fn` (ids kept). */
function editFindings(text, fn) {
const at = text.lastIndexOf('```json\n') + 8;
const end = text.lastIndexOf('```');
const payload = JSON.parse(text.slice(at, end));
payload.findings = payload.findings.map(fn);
return `${text.slice(0, at)}${JSON.stringify(payload, null, 2)}\n${text.slice(end)}`;
}
/** `text` with one prose line added under `## Executive Summary` (outside the JSON block and the frontmatter). */
function withNote(text) {
const head = '## Executive Summary\n\n';
assert.ok(text.includes(head), 'the review has a ## Executive Summary section');
return text.replace(head, `${head}A note added later.\n\n`);
}
/** `text` without its trailing fenced JSON block. */
function withoutBlock(text) {
const at = text.lastIndexOf('```json\n');
assert.ok(at >= 0, 'the review has a trailing JSON block');
const out = text.slice(0, at);
assert.equal(out.includes('```'), false, 'no fenced block is left');
return out;
}
/** Import statements of `text` that name review-answers (static, side-effect or dynamic). */
function answersImports(text) {
const re = /(?:\bfrom\s*|\bimport\s*\(?\s*)(['"`])[^'"`]*review-answers(?:\.mjs)?\1/g;
return [...String(text).matchAll(re)].map((m) => m[0]);
}
test('[RSP] prepare, stop, record an accepted answer, prepare again gives round 2', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
const id = stoppedRoundTwo(fx);
const rec = fx.record(id, 'accepted');
assert.equal(rec.status, 0, `record: ${rec.stdout}${rec.stderr}`);
assert.match(fx.read(), /^## Answers$/m, 'record wrote the ## Answers row into review.md');
assert.notEqual(fx.read(fx.archive(1)), fx.read(), 'review.md now differs from its archive');
const p = fx.prepare();
assert.equal(p.status, 0, `prepare after the answer: ${p.stdout}${p.stderr}`);
assert.equal(parse(p).round, 2, p.stdout);
assert.equal(parse(p).previous, `review-round-${fx.run}-1.md`, p.stdout);
assert.equal(parse(p).archived, true, `the own archive was refreshed: ${p.stdout}`);
const archived = fx.read(fx.archive(1));
assert.match(archived, /^## Answers$/m, 'the archive now holds the ## Answers row');
assert.ok(archived.includes(`| ${id} | accepted | ${REASON} |`), archived);
assert.equal(archived, fx.read(), 'the archive is review.md as it is now');
assert.deepEqual(fx.archives(), [`review-round-${fx.run}-1.md`], 'no second archive');
});
test('[RSP] the carried answer binds through the refreshed archive', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
const oldId = stoppedRoundTwo(fx);
const rec = fx.record(oldId, 'accepted');
assert.equal(rec.status, 0, `record: ${rec.stdout}${rec.stderr}`);
const record = parse(rec).record;
const p = fx.prepare();
assert.equal(p.status, 0, `prepare after the answer: ${p.stdout}${p.stderr}`);
assert.equal(parse(p).round, 2, p.stdout);
// Round 2 re-raises the MAJOR; finalize carries it; carry writes the row from the archive.
fx.write(rndReview({ start: fx.start, end: fx.c2, findings: [MAJ_A3] }).text);
fx.out(CCR, []);
const f2 = fx.finalize2();
assert.equal(f2.status, 0, `round-2 finalize: ${f2.stdout}${f2.stderr}`);
const c = fx.carry();
assert.equal(c.status, 0, `carry: ${c.stdout}${c.stderr}`);
const text = fx.read();
const newF = jsonOf(text).findings[0];
assert.equal(newF.carried_from, oldId, text);
assert.deepEqual(parse(c).written, [newF.id], `carry wrote the row: ${c.stdout}`);
assert.ok(text.includes(`| ${newF.id} | accepted | ${REASON} | ${record} | review-round-${fx.run}-1.md#${oldId} |`), text);
const gate = fx.gate();
assert.equal(gate.status, 0, `the carried row is bound: ${gate.stdout}${gate.stderr}`);
});
test('[RSP] a fixed answer with its R-step after the stop still gives round 2', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
const id = stoppedRoundTwo(fx);
// /trekreview writes the R-step first; record --answer fixed refuses without it.
fx.write(withRStep(fx.read(), id));
const rec = fx.record(id, 'fixed');
assert.equal(rec.status, 0, `record fixed: ${rec.stdout}${rec.stderr}`);
assert.match(fx.read(), /^### Step R1: fix the parser$/m);
const p = fx.prepare();
assert.equal(p.status, 0, `prepare after the fixed answer: ${p.stdout}${p.stderr}`);
assert.equal(parse(p).round, 2, p.stdout);
const archived = fx.read(fx.archive(1));
assert.match(archived, /^### Step R1: fix the parser$/m, 'the archive holds the R-step');
assert.ok(archived.includes(`| ${id} | fixed | ${REASON} |`), archived);
assert.deepEqual(fx.archives(), [`review-round-${fx.run}-1.md`], 'no second archive');
});
test('[RSP] an unreadable archive is not measured, not a crash', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
stoppedRoundTwo(fx);
const rec = fx.record(jsonOf(fx.read()).findings[0].id, 'accepted');
assert.equal(rec.status, 0, `record: ${rec.stdout}${rec.stderr}`);
const before = fx.read();
chmodSync(fx.archive(1), 0o000);
locked.push(fx.archive(1));
let readable = true;
try { readFileSync(fx.archive(1)); } catch { readable = false; }
assert.equal(readable, false, 'mode 000 does not stop a read here (running as root?): this case cannot be shown, so it fails');
const p = fx.prepare();
assert.equal(p.status, 2, `prepare: ${p.stdout}${p.stderr}`);
const reason = String(parse(p).reason);
assert.match(reason, new RegExp(`archive review-round-${fx.run}-1\\.md is not readable`), p.stdout);
assert.doesNotMatch(reason, /crashed/, p.stdout);
assert.match(reason, /not measured/, p.stdout);
assert.equal(fx.read(), before, 'review.md is untouched');
});
test('keep: an archive whose findings differ is still counted as an earlier round', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
stoppedRoundTwo(fx);
const own = fx.read(fx.archive(1));
// review.md edited elsewhere: a finding's title changed in the JSON block.
fx.write(editFindings(fx.read(), (x) => ({ ...x, title: 'Another title' })));
const r = fx.prepare();
assert.equal(r.status, 2, r.stdout + r.stderr);
assert.match(String(parse(r).reason), /round count does not match the archives — not measured/, r.stdout);
assert.equal(fx.read(fx.archive(1)), own, 'the archive is left alone');
// review.md with another scope_sha_end (same JSON block).
fx.write(own.replace(`scope_sha_end: ${fx.c1}`, `scope_sha_end: ${fx.c2}`));
assert.notEqual(fx.read(), own);
const s = fx.prepare();
assert.equal(s.status, 2, s.stdout + s.stderr);
assert.match(String(parse(s).reason), /round count does not match the archives — not measured/, s.stdout);
assert.equal(fx.read(fx.archive(1)), own, 'the archive is left alone');
});
test('keep: review-rounds imports nothing from review-answers', () => {
const real = readFileSync(ROUNDS, 'utf8');
assert.deepEqual(answersImports(real), [], 'lib/review/review-rounds.mjs imports review-answers');
// The scanner sees a planted import in a temp copy.
const base = mkdtempSync(join(tmpdir(), 'review-rounds-imports-'));
made.push(base);
const copy = join(base, 'review-rounds.mjs');
copyFileSync(ROUNDS, copy);
writeFileSync(copy, real.replace("import { isMain } from '../util/is-main.mjs';",
"import { isMain } from '../util/is-main.mjs';\nimport { findingsBlockSha } from './review-answers.mjs';"));
assert.equal(answersImports(readFileSync(copy, 'utf8')).length, 1, 'the planted import is found');
for (const planted of ["const m = await import('./review-answers.mjs');", "import './review-answers.mjs';"]) {
assert.equal(answersImports(`${real}\n${planted}\n`).length, 1, `the planted ${planted} is found`);
}
// The clean copy passes again.
copyFileSync(ROUNDS, copy);
assert.deepEqual(answersImports(readFileSync(copy, 'utf8')), [], 'the clean copy passes');
});
test('keep: two equal archives still give the same round as before', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
stoppedRoundTwo(fx);
// A second archive with review.md's bytes: byte-equal archives are never counted.
copyFileSync(fx.reviewPath, fx.archive(2));
const before = Object.fromEntries(fx.archives().map((n) => [n, fx.read(join(fx.project, n))]));
const r = fx.prepare();
assert.equal(r.status, 0, r.stdout + r.stderr);
assert.deepEqual([parse(r).round, parse(r).previous, parse(r).archived], [2, `review-round-${fx.run}-1.md`, false], r.stdout);
const now = Object.fromEntries(fx.archives().map((n) => [n, fx.read(join(fx.project, n))]));
assert.deepEqual(now, before, 'both archives are kept as they were');
assert.ok(existsSync(fx.archive(2)));
});
test('keep: an archive under another number is counted although its block and scope_sha_end match', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
stoppedRoundTwo(fx);
// Archive 2: review.md's text plus one prose line; same JSON block, same scope_sha_end, other bytes.
fx.write(withNote(fx.read()), fx.archive(2));
assert.notEqual(fx.read(fx.archive(2)), fx.read());
assert.equal(jsonOf(fx.read(fx.archive(2))).findings[0].id, jsonOf(fx.read()).findings[0].id);
assert.match(fx.read(fx.archive(2)), new RegExp(`^scope_sha_end: ${fx.c1}$`, 'm'));
const before = Object.fromEntries(fx.archives().map((n) => [n, readFileSync(join(fx.project, n))]));
assert.deepEqual(Object.keys(before), [`review-round-${fx.run}-1.md`, `review-round-${fx.run}-2.md`]);
const r = fx.prepare();
assert.equal(r.status, 2, r.stdout + r.stderr);
assert.match(String(parse(r).reason), /round count does not match the archives — not measured/, r.stdout);
const now = Object.fromEntries(fx.archives().map((n) => [n, readFileSync(join(fx.project, n))]));
assert.deepEqual(now, before, 'both archives are left unchanged, byte for byte');
});
test('keep: a same-number archive is not the own archive without a JSON block or a commit scope_sha_end', () => {
assert.equal(typeof rounds.prepare, 'function', 'review-rounds.mjs exports prepare');
const fx = rndRepo();
stoppedRoundTwo(fx);
const finalized = fx.read();
// No trailing JSON block in review.md nor in archive 1; archive 1 has one more prose line.
const bare = withoutBlock(finalized);
fx.write(bare);
fx.write(withNote(bare), fx.archive(1));
const own1 = readFileSync(fx.archive(1));
const r = fx.prepare();
assert.equal(r.status, 2, r.stdout + r.stderr);
assert.match(String(parse(r).reason), /round count does not match the archives — not measured/, r.stdout);
assert.deepEqual(readFileSync(fx.archive(1)), own1, 'the archive is left unchanged');
// The JSON block kept, but scope_sha_end is not a commit id in either file.
const noEnd = finalized.replace(`scope_sha_end: ${fx.c1}`, 'scope_sha_end: HEAD');
assert.notEqual(noEnd, finalized);
fx.write(noEnd);
fx.write(withNote(noEnd), fx.archive(1));
const own2 = readFileSync(fx.archive(1));
const s = fx.prepare();
assert.equal(s.status, 2, s.stdout + s.stderr);
assert.match(String(parse(s).reason), /round count does not match the archives — not measured/, s.stdout);
assert.deepEqual(readFileSync(fx.archive(1)), own2, 'the archive is left unchanged');
assert.deepEqual(fx.archives(), [`review-round-${fx.run}-1.md`], 'no other archive');
});