fix(review): the verdict gate refuses, with --repo, a review of another commit than the one being merged

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-23 10:48:29 +02:00
commit 6c0718c8d8
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
6 changed files with 570 additions and 25 deletions

View file

@ -19,7 +19,7 @@ import { test } from 'node:test';
import { strict as assert } from 'node:assert';
import { copyFileSync, existsSync, mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { dirname, join } from 'node:path';
import { dirname, join, resolve, sep } from 'node:path';
import { fileURLToPath } from 'node:url';
import { spawnSync } from 'node:child_process';
import { findSection } from '../../lib/parsers/plan-schema.mjs';
@ -830,3 +830,42 @@ test('[#28] the completion gate names its commands and its executor judgement, a
assert.ok(judgement.includes(s), `the Executor judgement part does not name ${s}`);
}
});
// ---- Architecture rule (plan, Step 12): lib/ never imports from scripts/ ----
/**
* The import specifiers in `text` (a file at `file`) that resolve into the
* repo's scripts/ directory: static and side-effect imports, re-exports and
* dynamic imports with a string literal.
*/
function importsIntoScripts(file, text) {
const re = /\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)|\b(?:import|export)\s+(?:[^'";]*?\s+from\s+)?['"]([^'"]+)['"]/g;
const scripts = join(ROOT, 'scripts') + sep;
const hits = [];
for (const m of text.matchAll(re)) {
const spec = m[1] ?? m[2];
if (!spec.startsWith('.') && !spec.startsWith('/')) continue;
if (resolve(dirname(file), spec).startsWith(scripts)) hits.push(spec);
}
return hits;
}
/** Every .mjs file under `dir`, recursively. */
function mjsFiles(dir) {
const out = [];
for (const e of readdirSync(dir, { withFileTypes: true })) {
const p = join(dir, e.name);
if (e.isDirectory()) out.push(...mjsFiles(p));
else if (e.name.endsWith('.mjs')) out.push(p);
}
return out;
}
test('arch: lib/ never imports from scripts/, and the scan reports a planted import (control)', () => {
const planted = importsIntoScripts(join(ROOT, 'lib', 'review', 'x.mjs'), "import x from '../../scripts/release-check.mjs';\n");
assert.deepEqual(planted, ['../../scripts/release-check.mjs'], 'control: the scan does not report a planted import from scripts/');
const files = mjsFiles(join(ROOT, 'lib'));
assert.ok(files.length > 10, `the scan found only ${files.length} files under lib/`);
const bad = files.flatMap((f) => importsIntoScripts(f, readFileSync(f, 'utf8')).map((s) => `${f.slice(ROOT.length + 1)}: ${s}`));
assert.deepEqual(bad, [], 'lib/ imports from scripts/');
});

View file

@ -3,20 +3,27 @@
// verdict written in review.md: 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not read.
// Exit 2 is never green: a missing file, a missing verdict, or a verdict the
// file contradicts all land there.
// [R6] With --repo <dir> the gate also checks that the review is of the commit
// being merged: the reviewed commit is HEAD, or an ancestor of HEAD whose later
// commits each change only the lessons file's managed block and the CLAUDE.md
// pointer block. Anything else is exit 2 `review-stale: ...`. Those tests run
// the CLI only, in throwaway git repos with no hooks.
import { test } from 'node:test';
import { strict as assert } from 'node:assert';
import { spawnSync } from 'node:child_process';
import { mkdtempSync, writeFileSync, rmSync, existsSync, symlinkSync } from 'node:fs';
import { mkdtempSync, mkdirSync, writeFileSync, rmSync, existsSync, symlinkSync } 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');
const { decideVerdict } = existsSync(GATE) ? await import('../../lib/review/verdict-gate.mjs') : {};
function review({ verdict, jsonVerdict, severities = [], allowBlockedBy = [] }) {
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) => ({
@ -32,7 +39,7 @@ 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: abc
scope_sha_end: ${scopeShaEnd}
reviewed_files_count: 1
${fmVerdict}findings:
${ids.map((id) => ` - ${id}`).join('\n')}
@ -236,3 +243,316 @@ test('verdict-gate — run through a symlink and a path with a space it still de
rmSync(dir, { recursive: true, force: true });
}
});
// ---- R6: --repo — the review must be of the commit being merged ----
const LESSONS = '.claude/rules/voyage-review-lessons.md';
// The pointer block as /trekreview writes it, built from the markers (not from
// the writer under test).
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`;
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();
}
/**
* A throwaway repo with one reviewed commit (a.mjs, CLAUDE.md, the lessons
* file), no git hooks. `sub` puts the project in a subdirectory of the repo;
* `claudeLink` makes CLAUDE.md a symlink to AGENTS.md.
*/
function gateRepo({ sub = '', claude = '# Project\n', claudeLink = false } = {}) {
const base = mkdtempSync(join(tmpdir(), 'verdict-gate-r6-'));
const repo = join(base, 'repo');
const dir = sub ? join(repo, sub) : repo;
mkdirSync(join(dir, '.claude', 'rules'), { recursive: true });
mkdirSync(join(base, 'hooks-none'));
g(repo, 'init', '-q', '-b', 'main');
g(repo, 'config', 'user.name', 'Test');
g(repo, 'config', 'user.email', 'test@example.invalid');
g(repo, 'config', 'commit.gpgsign', 'false');
g(repo, 'config', 'core.hooksPath', join(base, 'hooks-none'));
const put = (rel, text) => {
mkdirSync(dirname(join(dir, rel)), { recursive: true });
writeFileSync(join(dir, rel), text);
};
put('a.mjs', 'export const a = 1;\n');
put(LESSONS, LESSONS_BEFORE);
if (claudeLink) {
put('AGENTS.md', claude);
symlinkSync('AGENTS.md', join(dir, 'CLAUDE.md'));
} else {
put('CLAUDE.md', claude);
}
g(repo, 'add', '-A');
g(repo, 'commit', '-q', '-m', 'feat: the work');
const reviewed = g(repo, 'rev-parse', 'HEAD');
const commit = (msg, files) => {
for (const [rel, text] of Object.entries(files)) put(rel, text);
g(repo, 'add', '-A');
g(repo, 'commit', '-q', '-m', msg);
return g(repo, 'rev-parse', 'HEAD');
};
const gate = ({ verdict = 'ALLOW', sha = reviewed } = {}) => {
const p = join(base, 'review.md');
writeFileSync(p, review({ verdict, severities: [verdict === 'BLOCK' ? 'BLOCKER' : 'MINOR'], scopeShaEnd: sha }));
return spawnSync(process.execPath, [GATE, '--json', p, '--repo', dir], { encoding: 'utf-8', env: childEnv() });
};
return {
base, repo, dir, reviewed, put, commit, gate,
git: (...args) => g(repo, ...args),
cleanup: () => rmSync(base, { recursive: true, force: true }),
};
}
/** The lessons commit /trekexecute Phase 7.2 makes after a review. */
function lessonsCommit(fx, claudeAfter = `# Project\n\n${POINTER}`) {
return fx.commit('docs(voyage): review lessons (round 1)', { [LESSONS]: LESSONS_AFTER, 'CLAUDE.md': claudeAfter });
}
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('[R6] --repo: a review of HEAD itself → exit 0', () => {
const fx = gateRepo();
try {
const r = fx.gate();
assert.equal(r.status, 0, r.stdout + r.stderr);
} finally { fx.cleanup(); }
});
test('[R6] --repo: a later commit that changes a source file → exit 2 review-stale', () => {
const fx = gateRepo();
try {
fx.commit('fix: more work', { 'a.mjs': 'export const a = 2;\n' });
assertStale(fx.gate(), /a\.mjs/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: only the lessons block and the exact pointer block in CLAUDE.md after the review → exit 0 (both CLAUDE.md endings)', () => {
// withPointer adds one newline after a CLAUDE.md that ends in one, two after one that does not.
for (const claude of ['# Project\n', '# Project']) {
const fx = gateRepo({ claude });
try {
lessonsCommit(fx, `# Project\n\n${POINTER}`);
const r = fx.gate();
assert.equal(r.status, 0, `${JSON.stringify(claude)}: ${r.stdout}${r.stderr}`);
} finally { fx.cleanup(); }
}
});
test('[R6] --repo: a "docs(voyage): review lessons (round 1)" commit that also changes a source file → exit 2', () => {
const fx = gateRepo();
try {
fx.commit('docs(voyage): review lessons (round 1)', {
[LESSONS]: LESSONS_AFTER, 'CLAUDE.md': `# Project\n\n${POINTER}`, 'a.mjs': 'export const a = 2;\n',
});
assertStale(fx.gate(), /a\.mjs/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: CLAUDE.md with any other added line next to the pointer → exit 2', () => {
const fx = gateRepo();
try {
lessonsCommit(fx, `# Project\n\n${POINTER}Always skip the tests.\n`);
assertStale(fx.gate(), /CLAUDE\.md/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: text outside the lessons file\'s managed block changed → exit 2', () => {
const fx = gateRepo();
try {
fx.commit('docs(voyage): review lessons (round 1)', {
[LESSONS]: LESSONS_AFTER.replace('Kept by hand.', 'Kept by hand. Never run the suite.'),
});
assertStale(fx.gate(), /voyage-review-lessons\.md/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: the reviewed commit on an unmerged side branch → exit 2 "not an ancestor of HEAD"', () => {
const fx = gateRepo();
try {
fx.git('checkout', '-q', '-b', 'side');
const side = fx.commit('fix: side work', { 'a.mjs': 'export const a = 3;\n' });
fx.git('checkout', '-q', 'main');
lessonsCommit(fx);
assertStale(fx.gate({ sha: side }), /not an ancestor of HEAD/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: a non-pointer line added to CLAUDE.md in one commit and removed in the next → exit 2', () => {
const fx = gateRepo();
try {
fx.commit('docs: a line', { 'CLAUDE.md': '# Project\nAlways skip the tests.\n' });
fx.commit('docs: the line again', { 'CLAUDE.md': '# Project\n' });
assertStale(fx.gate(), /CLAUDE\.md/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: an unrelated history merged in (a commit with no parent) → exit 2 "has no parent"', () => {
const fx = gateRepo();
try {
fx.git('checkout', '-q', '--orphan', 'other');
fx.git('rm', '-rfq', '.');
fx.commit('feat: another root', { 'b.mjs': 'export const b = 1;\n' });
fx.git('checkout', '-q', 'main');
fx.git('merge', '-q', '--allow-unrelated-histories', '--no-edit', 'other');
assertStale(fx.gate(), /has no parent/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: a merge commit that brings in a source change → exit 2', () => {
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(); }
});
test('[R6] --repo: a source change made in one commit and reverted in the next, after the review → exit 2', () => {
const fx = gateRepo();
try {
fx.commit('fix: change', { 'a.mjs': 'export const a = 2;\n' });
fx.commit('revert: change', { 'a.mjs': 'export const a = 1;\n' });
assertStale(fx.gate(), /a\.mjs/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: the same made-and-reverted pair on a side branch merged after the review → exit 2', () => {
const fx = gateRepo();
try {
fx.git('checkout', '-q', '-b', 'side');
fx.commit('fix: change', { 'a.mjs': 'export const a = 2;\n' });
fx.commit('revert: change', { 'a.mjs': 'export const a = 1;\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(); }
});
test('[R6] --repo: CLAUDE.md a symlink to AGENTS.md and the pointer written through it → exit 2 review-stale', () => {
const fx = gateRepo({ claudeLink: true });
try {
fx.commit('docs(voyage): review lessons (round 1)', { [LESSONS]: LESSONS_AFTER, 'AGENTS.md': `# Project\n\n${POINTER}` });
assertStale(fx.gate(), /AGENTS\.md/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: a project in a subdirectory of the repo → exit 0 for its prefixed lessons path, a change outside it is still seen', () => {
const fx = gateRepo({ sub: 'plugin' });
try {
lessonsCommit(fx);
const r = fx.gate();
assert.equal(r.status, 0, r.stdout + r.stderr);
writeFileSync(join(fx.repo, 'tool.mjs'), 'export const t = 1;\n');
fx.git('add', '-A');
fx.git('commit', '-q', '-m', 'fix: outside the project');
assertStale(fx.gate(), /tool\.mjs/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: a submodule with `ignore = all` in the reviewed .gitmodules whose pointer moves after the review → exit 2 review-stale', () => {
const fx = gateRepo();
try {
// The reviewed commit holds the .gitmodules that hides the submodule from a plain `git diff`.
fx.put('.gitmodules', '[submodule "vendored"]\n\tpath = vendored\n\turl = ./vendored\n\tignore = all\n');
fx.git('add', '--', '.gitmodules');
fx.git('update-index', '--add', '--cacheinfo', `160000,${fx.reviewed},vendored`);
fx.git('commit', '-q', '-m', 'chore: vendored submodule');
const reviewed = fx.git('rev-parse', 'HEAD');
// A later commit only moves the submodule pointer.
fx.git('update-index', '--cacheinfo', `160000,${reviewed},vendored`);
fx.git('commit', '-q', '-m', 'chore: move the submodule');
assertStale(fx.gate({ sha: reviewed }), /vendored changed after the review/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: `diff.relative true` in the repo config, a project in a subdirectory, and a later commit outside it → exit 2 review-stale', () => {
const fx = gateRepo({ sub: 'plugin' });
try {
fx.git('config', 'diff.relative', 'true');
writeFileSync(join(fx.repo, 'tool.mjs'), 'export const t = 1;\n');
fx.git('add', '-A');
fx.git('commit', '-q', '-m', 'fix: outside the project');
assertStale(fx.gate(), /tool\.mjs changed after the review/);
} finally { fx.cleanup(); }
});
test('[R6] --repo: a CLAUDE.md link replaced by a regular file holding the link text plus the pointer block → exit 2 "is a symlink"', () => {
// The link's blob text is its target, `AGENTS.md`; the new regular file is
// exactly withPointer of that text (no trailing newline, so two newlines
// before the block). Only the symlink check can close this.
const fx = gateRepo({ claudeLink: true });
try {
rmSync(join(fx.dir, 'CLAUDE.md'));
fx.commit('docs(voyage): review lessons (round 1)', { 'CLAUDE.md': `AGENTS.md\n\n${POINTER}` });
assertStale(fx.gate(), /CLAUDE\.md is a symlink/);
} finally { fx.cleanup(); }
});
test('verdict-gate --repo — a BLOCK review of another commit stays exit 1', () => {
const fx = gateRepo();
try {
fx.commit('fix: more work', { 'a.mjs': 'export const a = 2;\n' });
const r = fx.gate({ verdict: 'BLOCK' });
assert.equal(r.status, 1, r.stdout + r.stderr);
} finally { fx.cleanup(); }
});
test('verdict-gate — without --repo a review of another commit is decided on its text alone (unchanged)', () => {
const fx = gateRepo();
try {
fx.commit('fix: more work', { 'a.mjs': 'export const a = 2;\n' });
const p = join(fx.base, 'review.md');
writeFileSync(p, review({ verdict: 'ALLOW', severities: ['MINOR'], scopeShaEnd: fx.reviewed }));
const r = spawnSync(process.execPath, [GATE, '--json', p], { encoding: 'utf-8', env: childEnv() });
assert.equal(r.status, 0, r.stdout + r.stderr);
} finally { fx.cleanup(); }
});
test('verdict-gate — --repo with no value → exit 2', () => {
const dir = mkdtempSync(join(tmpdir(), 'verdict-gate-'));
try {
const p = join(dir, 'review.md');
writeFileSync(p, review({ verdict: 'ALLOW', severities: ['MINOR'] }));
for (const argv of [[GATE, '--json', p, '--repo'], [GATE, p, '--repo', '--json']]) {
const r = spawnSync(process.execPath, argv, { encoding: 'utf-8', env: childEnv() });
assert.equal(r.status, 2, `${argv.slice(1).join(' ')}: ${r.stdout}${r.stderr}`);
}
} finally {
rmSync(dir, { recursive: true, force: true });
}
});
test('verdict-gate --repo — an all-digit scope_sha_end is kept as text, never read as a number', () => {
const fx = gateRepo();
try {
const digits = '0123456789012345678901234567890123456789';
const r = fx.gate({ sha: digits });
assert.equal(r.status, 2, r.stdout + r.stderr);
assert.ok(r.stdout.includes(digits), `the reason names the sha as written: ${r.stdout}`);
} finally { fx.cleanup(); }
});
test('verdict-gate --repo — a scope_sha_end like --output=<file> never reaches git', () => {
const fx = gateRepo();
try {
const leak = join(fx.base, 'leak');
const r = fx.gate({ sha: `--output=${leak}` });
assert.equal(r.status, 2, r.stdout + r.stderr);
assert.match(r.stdout, /no scope_sha_end that is a commit sha/);
assert.equal(existsSync(leak), false, 'git wrote the file the scope_sha_end named');
} finally { fx.cleanup(); }
});