fix(review): R6 checks every parent of a commit after the review
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
03c9f527a3
commit
57e7eba0a0
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 278 additions and 41 deletions
|
|
@ -8,7 +8,11 @@
|
|||
//
|
||||
// - the reviewed commit is HEAD, or an ancestor of HEAD;
|
||||
// - every commit between it and HEAD (side branches included, each diffed
|
||||
// against its first parent, `--no-relative --ignore-submodules=none`) changes only
|
||||
// against every parent it has, `--no-relative --ignore-submodules=none`;
|
||||
// some path of parent edges leads from the reviewed commit to HEAD, so
|
||||
// when every edge changes only the allowed paths, HEAD's tree outside
|
||||
// them is the reviewed tree — a merge whose first parent comes from
|
||||
// before the review cannot restore the tree from before it) changes only
|
||||
// `{prefix}.claude/rules/voyage-review-lessons.md`, `{prefix}CLAUDE.md`
|
||||
// and the approval log `.claude/voyage/approvals.jsonl` (LOG_RELPATH, at
|
||||
// the git top level, never under {prefix}), where {prefix} is the
|
||||
|
|
@ -267,49 +271,53 @@ export function checkReviewedCommit({ repo, sha } = {}) {
|
|||
const root = lines.find((f) => f.length === 1);
|
||||
if (root) return stale(`commit ${short(root[0])} has no parent`);
|
||||
|
||||
for (const [c] of lines) {
|
||||
// `--no-relative` and `--ignore-submodules=none`: neither `diff.relative`
|
||||
// nor an `ignore =` setting (config or .gitmodules) may hide a changed path.
|
||||
const names = git(repo, ['diff', '--no-relative', '--ignore-submodules=none', '--name-only', '-z', '--no-renames', `${c}^1`, c]);
|
||||
if (names.status !== 0) return gitFailed(`diff ${short(c)}`, names);
|
||||
const paths = names.stdout.split('\0').filter(Boolean);
|
||||
// The approval log is the top-level one: `--no-relative` paths are from the top level.
|
||||
const other = paths.find((p) => p !== lessons && p !== claude && p !== LOG_RELPATH);
|
||||
if (other !== undefined) return stale(`${other} changed after the review (commit ${short(c)})`);
|
||||
for (const [c, ...parents] of lines) {
|
||||
// Every parent edge, not only the first: a merge whose first parent is
|
||||
// from before the review could otherwise bring the pre-review tree back.
|
||||
for (const p of parents) {
|
||||
// `--no-relative` and `--ignore-submodules=none`: neither `diff.relative`
|
||||
// nor an `ignore =` setting (config or .gitmodules) may hide a changed path.
|
||||
const names = git(repo, ['diff', '--no-relative', '--ignore-submodules=none', '--name-only', '-z', '--no-renames', p, c]);
|
||||
if (names.status !== 0) return gitFailed(`diff ${short(c)}`, names);
|
||||
const paths = names.stdout.split('\0').filter(Boolean);
|
||||
// The approval log is the top-level one: `--no-relative` paths are from the top level.
|
||||
const other = paths.find((q) => q !== lessons && q !== claude && q !== LOG_RELPATH);
|
||||
if (other !== undefined) return stale(`${other} changed after the review (commit ${short(c)})`);
|
||||
|
||||
for (const path of paths) {
|
||||
const before = blobAt(repo, `${c}^1`, path);
|
||||
if (!before.ok) return before.fail;
|
||||
const after = blobAt(repo, c, path);
|
||||
if (!after.ok) return after.fail;
|
||||
if (before.mode === SYMLINK_MODE || after.mode === SYMLINK_MODE) {
|
||||
return stale(`${path} is a symlink (commit ${short(c)}) — re-run /trekreview on this commit`);
|
||||
}
|
||||
if (path === LOG_RELPATH) {
|
||||
// 7. The approval log: only appended review answers (R6).
|
||||
const was = bytesAt(repo, `${c}^1`, path, before);
|
||||
if (!was.ok) return was.fail;
|
||||
const now = bytesAt(repo, c, path, after);
|
||||
if (!now.ok) return now.fail;
|
||||
// A regular, non-executable file after the commit, on creation too.
|
||||
if (after.mode !== LOG_MODE || !onlyAppendedReviewRecords(was.bytes, now.bytes)) {
|
||||
return stale(`${path} changed other than by appended review answers (commit ${c.slice(0, 8)})`);
|
||||
for (const path of paths) {
|
||||
const before = blobAt(repo, p, path);
|
||||
if (!before.ok) return before.fail;
|
||||
const after = blobAt(repo, c, path);
|
||||
if (!after.ok) return after.fail;
|
||||
if (before.mode === SYMLINK_MODE || after.mode === SYMLINK_MODE) {
|
||||
return stale(`${path} is a symlink (commit ${short(c)}) — re-run /trekreview on this commit`);
|
||||
}
|
||||
} else if (path === lessons) {
|
||||
// 5. Only the managed block may change.
|
||||
if (outsideBlock(before.text).trimEnd() !== outsideBlock(after.text).trimEnd()) {
|
||||
return stale(`${path} changed outside the managed block (commit ${short(c)})`);
|
||||
if (path === LOG_RELPATH) {
|
||||
// 7. The approval log: only appended review answers (R6).
|
||||
const was = bytesAt(repo, p, path, before);
|
||||
if (!was.ok) return was.fail;
|
||||
const now = bytesAt(repo, c, path, after);
|
||||
if (!now.ok) return now.fail;
|
||||
// A regular, non-executable file after the commit, on creation too.
|
||||
if (after.mode !== LOG_MODE || !onlyAppendedReviewRecords(was.bytes, now.bytes)) {
|
||||
return stale(`${path} changed other than by appended review answers (commit ${c.slice(0, 8)})`);
|
||||
}
|
||||
} else if (path === lessons) {
|
||||
// 5. Only the managed block may change.
|
||||
if (outsideBlock(before.text).trimEnd() !== outsideBlock(after.text).trimEnd()) {
|
||||
return stale(`${path} changed outside the managed block (commit ${short(c)})`);
|
||||
}
|
||||
// 5b. What the block gains is lesson lines, or the header applyLesson
|
||||
// writes with the first lesson; free text there is an instruction (m4).
|
||||
const free = addedBlockLines(before.text, after.text)
|
||||
.find((l) => l !== BLOCK_HEADER && !validateLessonLine(l).valid);
|
||||
if (free !== undefined) {
|
||||
return stale(`free text added inside the lessons block (commit ${short(c)})`);
|
||||
}
|
||||
} else if (after.text !== before.text && after.text !== withPointer(before.text)) {
|
||||
// 6. CLAUDE.md: unchanged, or exactly the pointer block added.
|
||||
return stale(`${path} changed other than the review-lessons pointer (commit ${short(c)})`);
|
||||
}
|
||||
// 5b. What the block gains is lesson lines, or the header applyLesson
|
||||
// writes with the first lesson; free text there is an instruction (m4).
|
||||
const free = addedBlockLines(before.text, after.text)
|
||||
.find((l) => l !== BLOCK_HEADER && !validateLessonLine(l).valid);
|
||||
if (free !== undefined) {
|
||||
return stale(`free text added inside the lessons block (commit ${short(c)})`);
|
||||
}
|
||||
} else if (after.text !== before.text && after.text !== withPointer(before.text)) {
|
||||
// 6. CLAUDE.md: unchanged, or exactly the pointer block added.
|
||||
return stale(`${path} changed other than the review-lessons pointer (commit ${short(c)})`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
229
tests/lib/reviewed-commit-parents.test.mjs
Normal file
229
tests/lib/reviewed-commit-parents.test.mjs
Normal file
|
|
@ -0,0 +1,229 @@
|
|||
// tests/lib/reviewed-commit-parents.test.mjs
|
||||
// [RMP] R6 (lib/review/reviewed-commit.mjs) checks every parent of each commit
|
||||
// after the review, not only its first parent (fix round 4.5b2 part B, Step 13).
|
||||
// A merge whose first parent comes from before the review and whose tree is
|
||||
// the pre-review tree changes nothing against that parent, but against the
|
||||
// reviewed side it takes the reviewed change back out: HEAD no longer holds
|
||||
// what was reviewed, so the gate must say review-stale.
|
||||
// Every case runs the CLI `lib/review/verdict-gate.mjs --json <review.md>
|
||||
// --repo <repo>` on an ALLOW review of the reviewed commit R, in throwaway git
|
||||
// repos with no hooks. R sits on a pre-review commit P (`R^` is P).
|
||||
// The `keep:` cases hold before this step too and must stay so.
|
||||
|
||||
import { after, test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, rmSync, appendFileSync, cpSync, readFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join, dirname } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { MARK_START, MARK_END, POINTER_START, POINTER_LINE, POINTER_END } from '../../lib/review/recurrence.mjs';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:30-72 (review), never imported.
|
||||
function review({ verdict, jsonVerdict, severities = [], allowBlockedBy = [], scopeShaEnd = 'abc' }) {
|
||||
const ids = severities.map((_, i) => String(i + 1).padStart(40, 'a'));
|
||||
const fmVerdict = verdict === undefined ? '' : `verdict: ${verdict}\n`;
|
||||
const findings = severities.map((s, i) => ({
|
||||
id: ids[i], severity: s, rule_key: 'MISSING_TEST', file: 'a.mjs', line: i + 1, title: 't', detail: 'd', recommended_action: 'Add a test.',
|
||||
}));
|
||||
const counts = { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
|
||||
for (const s of severities) counts[s] += 1;
|
||||
const json = JSON.stringify({ verdict: jsonVerdict ?? verdict, counts, allow_blocked_by: allowBlockedBy, findings }, null, 2);
|
||||
return `---
|
||||
type: trekreview
|
||||
review_version: "1.1"
|
||||
task: "t"
|
||||
slug: s
|
||||
project_dir: .claude/projects/2026-10-06-s/
|
||||
brief_path: .claude/projects/2026-10-06-s/brief.md
|
||||
scope_sha_end: ${scopeShaEnd}
|
||||
reviewed_files_count: 1
|
||||
${fmVerdict}findings:
|
||||
${ids.map((id) => ` - ${id}`).join('\n')}
|
||||
---
|
||||
|
||||
# Review: t
|
||||
|
||||
## Executive Summary
|
||||
|
||||
x
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| a.mjs | summary-only | default |
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
x
|
||||
|
||||
\`\`\`json
|
||||
${json}
|
||||
\`\`\`
|
||||
`;
|
||||
}
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:255-261 (the R6 fixture texts), never imported.
|
||||
const LESSONS = '.claude/rules/voyage-review-lessons.md';
|
||||
const POINTER = `${POINTER_START}\n${POINTER_LINE}\n${POINTER_END}\n`;
|
||||
const LESSONS_BEFORE = `# Review lessons\n\n${MARK_START}\nEach line: a finding class.\n${MARK_END}\n\nKept by hand.\n`;
|
||||
const LESSON = '- [MISSING_TEST] (check) Mistake: m. · Fix: f. · How to check: c.';
|
||||
const LESSONS_AFTER = `# Review lessons\n\n${MARK_START}\nEach line: a finding class.\n${LESSON}\n${MARK_END}\n\nKept by hand.\n`;
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:263-267 (g), never imported.
|
||||
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();
|
||||
}
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:269-273 (putIn), never imported.
|
||||
function putIn(dir, rel, text) {
|
||||
mkdirSync(dirname(join(dir, rel)), { recursive: true });
|
||||
writeFileSync(join(dir, rel), text);
|
||||
}
|
||||
|
||||
const seeds = new Map();
|
||||
after(() => {
|
||||
for (const seed of seeds.values()) rmSync(seed.base, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:281-301 (seedRepo), never imported. Changed:
|
||||
// a pre-review commit P (a.mjs at `export const a = 0;`) comes first, so the
|
||||
// reviewed commit R changes a.mjs on top of it and is not the root commit.
|
||||
function seedRepo() {
|
||||
const base = mkdtempSync(join(tmpdir(), 'reviewed-commit-parents-seed-'));
|
||||
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`);
|
||||
putIn(repo, 'a.mjs', 'export const a = 0;\n');
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'feat: before the review');
|
||||
const pre = g(repo, 'rev-parse', 'HEAD');
|
||||
putIn(repo, 'a.mjs', 'export const a = 1;\n');
|
||||
putIn(repo, LESSONS, LESSONS_BEFORE);
|
||||
putIn(repo, 'CLAUDE.md', '# Project\n');
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', 'feat: the work');
|
||||
return { base, pre, reviewed: g(repo, 'rev-parse', 'HEAD') };
|
||||
}
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:310-339 (gateRepo), never imported. Changed:
|
||||
// one variant only (no sub, claudeLink, objectFormat), and it also returns P as `pre`.
|
||||
function gateRepo() {
|
||||
if (!seeds.has('')) seeds.set('', seedRepo());
|
||||
const seed = seeds.get('');
|
||||
const base = mkdtempSync(join(tmpdir(), 'reviewed-commit-parents-'));
|
||||
cpSync(seed.base, base, { recursive: true, verbatimSymlinks: true });
|
||||
const repo = join(base, 'repo');
|
||||
const config = join(repo, '.git', 'config');
|
||||
writeFileSync(config, readFileSync(config, 'utf8').split(seed.base).join(base));
|
||||
const { reviewed, pre } = seed;
|
||||
const commit = (msg, files) => {
|
||||
for (const [rel, text] of Object.entries(files)) putIn(repo, rel, text);
|
||||
g(repo, 'add', '-A');
|
||||
g(repo, 'commit', '-q', '-m', msg);
|
||||
};
|
||||
const gate = ({ verdict = 'ALLOW', sha = reviewed } = {}) => {
|
||||
const p = join(base, 'review.md');
|
||||
writeFileSync(p, review({ verdict, severities: ['MINOR'], scopeShaEnd: sha }));
|
||||
return spawnSync(process.execPath, [GATE, '--json', p, '--repo', repo], { encoding: 'utf-8', env: childEnv() });
|
||||
};
|
||||
return {
|
||||
base, repo, reviewed, pre, commit, gate,
|
||||
git: (...args) => g(repo, ...args),
|
||||
cleanup: () => rmSync(base, { recursive: true, force: true }),
|
||||
};
|
||||
}
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:341-344 (lessonsCommit), never imported.
|
||||
function lessonsCommit(fx, claudeAfter = `# Project\n\n${POINTER}`) {
|
||||
fx.commit('docs(voyage): review lessons (round 1)', { [LESSONS]: LESSONS_AFTER, 'CLAUDE.md': claudeAfter });
|
||||
}
|
||||
|
||||
// Copied from tests/lib/verdict-gate.test.mjs:346-350 (assertStale), never imported.
|
||||
function assertStale(r, re = /review-stale/) {
|
||||
assert.equal(r.status, 2, r.stdout + r.stderr);
|
||||
assert.match(r.stdout, /review-stale/);
|
||||
assert.match(r.stdout, re);
|
||||
}
|
||||
|
||||
test('[RMP] a merge whose first parent is the reviewed commit\'s parent and whose tree is that parent\'s tree, the reviewed commit as second parent → exit 2 review-stale naming a.mjs', () => {
|
||||
const fx = gateRepo();
|
||||
try {
|
||||
const tree = fx.git('rev-parse', `${fx.pre}^{tree}`);
|
||||
const merge = fx.git('commit-tree', tree, '-p', fx.pre, '-p', fx.reviewed, '-m', 'merge: back to before the review');
|
||||
fx.git('reset', '-q', '--hard', merge);
|
||||
// Control: HEAD is the merge, its first parent P, its second parent R, its tree P's tree.
|
||||
assert.equal(fx.git('rev-parse', 'HEAD^1'), fx.pre);
|
||||
assert.equal(fx.git('rev-parse', 'HEAD^2'), fx.reviewed);
|
||||
assert.equal(fx.git('rev-parse', 'HEAD^{tree}'), tree);
|
||||
assertStale(fx.gate(), /a\.mjs/);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('[RMP] a side branch from before the review, with one empty commit, merged over the reviewed history with -s ours → exit 2 review-stale', () => {
|
||||
const fx = gateRepo();
|
||||
try {
|
||||
fx.git('checkout', '-q', '-b', 'side', fx.pre);
|
||||
fx.git('commit', '-q', '--allow-empty', '-m', 'chore: nothing');
|
||||
const empty = fx.git('rev-parse', 'HEAD');
|
||||
fx.git('merge', '-q', '-s', 'ours', '--no-edit', 'main');
|
||||
// Control: HEAD is the merge on the side branch, first parent the empty commit, tree the pre-review tree.
|
||||
assert.equal(fx.git('rev-parse', 'HEAD^1'), empty);
|
||||
assert.equal(fx.git('rev-parse', 'HEAD^2'), fx.reviewed);
|
||||
assert.equal(fx.git('rev-parse', 'HEAD^{tree}'), fx.git('rev-parse', `${fx.pre}^{tree}`));
|
||||
assertStale(fx.gate(), /a\.mjs/);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('[RMP] an octopus merge whose first parent is the reviewed commit\'s parent and whose tree is that parent\'s tree → exit 2 review-stale', () => {
|
||||
const fx = gateRepo();
|
||||
try {
|
||||
fx.git('checkout', '-q', '-b', 'side', fx.pre);
|
||||
fx.git('commit', '-q', '--allow-empty', '-m', 'chore: nothing');
|
||||
const x = fx.git('rev-parse', 'HEAD');
|
||||
fx.git('checkout', '-q', 'main');
|
||||
const tree = fx.git('rev-parse', `${fx.pre}^{tree}`);
|
||||
const merge = fx.git('commit-tree', tree, '-p', fx.pre, '-p', x, '-p', fx.reviewed, '-m', 'merge: octopus back to before the review');
|
||||
fx.git('reset', '-q', '--hard', merge);
|
||||
// Control: three parents P, X, R.
|
||||
assert.deepEqual(fx.git('rev-list', '--parents', '-n', '1', 'HEAD').split(' ').slice(1), [fx.pre, x, fx.reviewed]);
|
||||
assertStale(fx.gate(), /a\.mjs/);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('keep: a merge after the review whose main side adds a lesson line and whose side branch from R adds only the CLAUDE.md pointer → exit 0', () => {
|
||||
const fx = gateRepo();
|
||||
try {
|
||||
fx.git('checkout', '-q', '-b', 'side');
|
||||
fx.commit('docs(voyage): pointer', { 'CLAUDE.md': `# Project\n\n${POINTER}` });
|
||||
fx.git('checkout', '-q', 'main');
|
||||
fx.commit('docs(voyage): review lessons (round 1)', { [LESSONS]: LESSONS_AFTER });
|
||||
fx.git('merge', '-q', '--no-ff', '--no-edit', 'side');
|
||||
// Control: HEAD is a merge of the two.
|
||||
assert.equal(fx.git('rev-list', '--parents', '-n', '1', 'HEAD').split(' ').length, 3);
|
||||
const r = fx.gate();
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('keep: a merge after the review that brings in a source change from a side branch started at R → exit 2 review-stale', () => {
|
||||
const fx = gateRepo();
|
||||
try {
|
||||
fx.git('checkout', '-q', '-b', 'side');
|
||||
fx.commit('fix: side work', { 'a.mjs': 'export const a = 3;\n' });
|
||||
fx.git('checkout', '-q', 'main');
|
||||
lessonsCommit(fx);
|
||||
fx.git('merge', '-q', '--no-ff', '--no-edit', 'side');
|
||||
assertStale(fx.gate(), /a\.mjs/);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue