open/voyage
1
0
Fork 0
Code Issues Releases 13 Activity

fix(execute): the step loop survives an adversarial review

Browse source 
An independent review of abc3083 found 15 defects, the two worst verified
in scratch repos. Each fix started with a test that failed.

- Proof of red could be "proven" by the environment: a start point
  unpacked with git archive had no node_modules and no .git, so any test
  failed there. The start point is now a git clone --shared checked out at
  start_sha, gitignored top-level entries are linked in, and a control run
  (start point + the change) must pass, or the result is not-run.
- The Test first file was never committed (the template lists it apart
  from Files:) and leaked into the next step's diff. The step's scope is
  now Files: + test files, staged at the checkpoint.
- New files outside Files: were invisible to every check. Every untracked
  file is registered with --intent-to-add; a revert unregisters them.
- The floor guard paired renames (foo.test.js -> .bak read as clean) and
  missed skipIf and node's t.skip/t.todo; prose files no longer count as
  stubs.
- Phase 7.5 audited from the last step's start_sha; it now uses the run's
  session_start_sha.
- The done line is written after the checkpoint and checked with --commit.
- fix_rounds never passes 5; attempts are never reset; re-control keeps
  the spec/quality verdicts and earlier floor-guard rulings; the closed
  stop list maps every existing stop; failed vs stopped made consistent;
  the controller's effort is said, not assumed.

Tests: 1470 (1468 pass, 0 fail, 2 skipped).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-06 20:17:39 +02:00
commit 44dcc779a9
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
11 changed files with 466 additions and 148 deletions
Download patch file Download diff file Expand all files Collapse all files

28
lib/verification/done-line.mjs
View file

@ -9,9 +9,11 @@
// --step, a command outside the test-runner allowlist, a command that cannot
// start, a working tree that is not a git repo.
//
// `--check --step <N> --report <file>`: exit 0 when the report carries a
// well-formed line for step N, 1 when it does not. A hand-typed "DONE" is not
// the script's line.
// `--check --step <N> --report <file> [--commit <sha>]`: exit 0 when the report
// carries a well-formed line for step N (at exactly that commit, with
// --commit), 1 when it does not. /trekexecute writes the line right after the
// step's checkpoint and checks it with --commit, so the line is bound to the
// commit the test passed on.
//
// Adapted from obra/superpowers@8ca22db (MIT, (c) 2025 Jesse Vincent),
// skills/executing-plans/scripts/task-done: the completion line is appended
@ -24,13 +26,20 @@ import { fileURLToPath } from 'node:url';
import { allowedCommand, screenCommand } from './criteria-runner.mjs';
import { commandEnv, DEFAULT_TIMEOUT_MS, repoRoot } from './red-proof.mjs';
export function doneLineRe(step) {
return new RegExp(`^DONE step ${step}: \`.+\` exit 0 at [0-9a-f]{40} \\d{4}-\\d{2}-\\d{2}T\\S+$`, 'm');
export function doneLineRe(step, commit) {
const sha = commit ? String(commit).replace(/[^0-9a-f]/g, '') : '[0-9a-f]{40}';
return new RegExp(`^DONE step ${step}: \`.+\` exit 0 at ${sha} \\d{4}-\\d{2}-\\d{2}T\\S+$`, 'm');
}
export function hasDoneLine(reportPath, step) {
/**
* With `commit` (the step's checkpoint sha, full 40 hex), the line must name
* exactly that commit. A regex cannot tell a typed line from a written one; the
* commit binds the line to the state the test ran on (/trekexecute writes the
* line right after the checkpoint, so HEAD is the step's own commit).
*/
export function hasDoneLine(reportPath, step, commit) {
if (!existsSync(reportPath)) return false;
return doneLineRe(step).test(readFileSync(reportPath, 'utf8'));
return doneLineRe(step, commit).test(readFileSync(reportPath, 'utf8'));
}
/** @returns {{status: 'written'|'failed'|'not-run', reason: string, line?: string, exit?: number|null}} */
@ -58,7 +67,7 @@ export function writeDoneLine({ cwd, step, cmd, report, timeoutMs = DEFAULT_TIME
const USAGE =
'usage: done-line.mjs --step <N> --cmd "<test command>" --report <file> [--cwd <dir>] [--json]\n' +
' done-line.mjs --check --step <N> --report <file>';
' done-line.mjs --check --step <N> --report <file> [--commit <sha>]';
export function main(argv) {
const o = {};
@ -72,12 +81,13 @@ export function main(argv) {
else if (a === '--cwd' && argv[i + 1]) o.cwd = resolve(argv[++i]);
else if (a === '--timeout' && argv[i + 1]) o.timeoutMs = Number(argv[++i]);
else if (a === '--check') check = true;
else if (a === '--commit' && argv[i + 1]) o.commit = argv[++i];
else if (a === '--json') json = true;
else { process.stderr.write(`done-line: unknown argument ${a}\n${USAGE}\n`); return 2; }
}
if (check) {
if (!/^\d+$/.test(String(o.step ?? '')) || !o.report) { process.stderr.write(`done-line: --check needs --step and --report\n${USAGE}\n`); return 2; }
const ok = hasDoneLine(o.report, o.step);
const ok = hasDoneLine(o.report, o.step, o.commit);
process.stdout.write(json ? `${JSON.stringify({ step: Number(o.step), done: ok })}\n` : `step ${o.step}: ${ok ? 'DONE line present' : 'no DONE line - not done'}\n`);
return ok ? 0 : 1;
}

149
lib/verification/red-proof.mjs
View file

@ -1,25 +1,38 @@
// lib/verification/red-proof.mjs
// Proof of red (P9 (1)). A step's test is run by a command twice:
// Proof of red (P9 (1)). A step's test is run by a command:
//
// 1. against the START POINT (the step's recorded start_sha, with the step's
// test files copied in): it must FAIL - otherwise the test proves nothing
// about the change;
// 2. against the RESULT (the working tree as the step left it): it must PASS.
// 2. against the RESULT (the working tree as the step left it): it must PASS;
// 3. CONTROL: the start-point copy with the step's whole change laid on top
// must PASS too. A copy that cannot go green even with the change failed
// step 1 for an environment reason (a missing dependency, a path, a tool),
// not because the change was missing - that is "not-run", never proven.
//
// `--red-only` runs step 1 alone: the gate the step helper passes BEFORE it
// builds (and locks the test). The executor's own check afterwards is the full
// two-sided run.
// run, and only the full run is authoritative.
//
// Exit 0 = proven, 1 = rejected (`not-red-at-base` or `not-green-at-result`),
// 2 = could not run (`not-run`: no test files, an unknown start point, a command
// outside the test-runner allowlist, a command that cannot start). 2 is never a
// pass and never "proven".
// outside the test-runner allowlist, a command that cannot start, a control run
// that stays red). 2 is never a pass and never "proven".
//
// The start point is materialized with `git archive <start_sha>` into a temp
// directory, never by checking anything out in the working tree: the step's
// work is not touched, and no worktree is registered in the repo. Tracked files
// only - pass `--link node_modules` (comma list) to symlink an untracked
// dependency directory from the working tree into the start-point copy.
// The start point is a temp `git clone --shared` of the repo checked out at
// start_sha - a real git checkout, so a git-reading command behaves as it does
// in the repo, and nothing in the working tree or the repo's worktree list is
// touched. Every gitignored top-level entry of the working tree (node_modules,
// .venv, a build cache) is symlinked in, so the copy has the same dependencies;
// `--link a,b` adds deeper ones. Measured 2026-10-06 (review): with a bare
// `git archive` copy, a test that needs node_modules "proved red" with no change.
// Known limit, failing safe: a dependency that points back INTO the working
// tree (an editable `pip install -e .`, a workspace symlink) makes the start
// point see the new code, and a correct step reads `not-red-at-base`.
//
// Everything runs at the root of --cwd's repository: plan paths and commands
// are root-relative, and `git archive`/`clone` paths from a subdirectory are
// not (measured 2026-10-06).
//
// The command must be ONE test-runner invocation without shell operators
// (criteria-runner.mjs `allowedCommand`), and it is screened through the
@ -27,8 +40,8 @@
// node never passes through the Bash tool, so the hook cannot fire by itself.
//
// When the output is TAP (`--test-reporter=tap`, pytest-tap, ...), the counts
// `# tests N` / `# fail N` are reported for both runs. They show WHAT the red
// was: "fail 1 of 1" on a node test file is usually a load error (an import the
// `# tests N` / `# fail N` are reported per run. They show WHAT the red was:
// "fail 1 of 1" on a node test file is usually a load error (an import the
// start point lacks), not named failing tests - see
// tests/fixtures/red-first/02243c6-always-allow-shim.mjs, re-measured through
// this tool in tests/lib/red-proof.test.mjs.
@ -36,7 +49,7 @@
// Borrowed: GitHub Spec Kit (MIT, github/spec-kit@2e941fe) - its test-first
// rule is an instruction there; here a command proves it.
import { cpSync, existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync } from 'node:fs';
import { cpSync, existsSync, lstatSync, mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync } from 'node:fs';
import { spawnSync } from 'node:child_process';
import { tmpdir } from 'node:os';
import { dirname, isAbsolute, join, resolve } from 'node:path';
@ -61,6 +74,11 @@ export function commandEnv(env = process.env) {
return rest;
}
export function repoRoot(dir) {
const top = spawnSync('git', ['rev-parse', '--show-toplevel'], { cwd: dir, encoding: 'utf8' });
return top.status === 0 ? top.stdout.trim() : null;
}
function run(cmd, cwd, timeoutMs) {
const r = spawnSync('/bin/sh', ['-c', cmd], { cwd, env: commandEnv(), encoding: 'utf8', timeout: timeoutMs, maxBuffer: 64 * 1024 * 1024 });
const output = `${r.stdout ?? ''}${r.stderr ?? ''}`;
@ -78,35 +96,71 @@ function unrunnable(r) {
return null;
}
/**
* @param {{cwd: string, base: string, cmd: string, tests: string[], link?: string[], timeoutMs?: number}} o
* @returns {{status: 'proven'|'rejected'|'not-run', reason: string, base?: object, result?: object}}
*/
export function repoRoot(dir) {
const top = spawnSync('git', ['rev-parse', '--show-toplevel'], { cwd: dir, encoding: 'utf8' });
return top.status === 0 ? top.stdout.trim() : null;
const git = (cwd, args) => spawnSync('git', args, { cwd, encoding: 'utf8', maxBuffer: 256 * 1024 * 1024 });
const lines = (s) => String(s ?? '').split('\n').filter(Boolean);
function safeRel(p) {
return p && !isAbsolute(p) && !p.split('/').includes('..');
}
function copyIn(from, to, rel) {
mkdirSync(dirname(join(to, rel)), { recursive: true });
rmSync(join(to, rel), { recursive: true, force: true });
cpSync(join(from, rel), join(to, rel), { recursive: true, verbatimSymlinks: true });
}
/** Symlink every gitignored top-level entry of the working tree, plus `extra`, into `tree`. */
function linkDependencies(root, tree, extra) {
const linked = [];
const ignored = lines(git(root, ['ls-files', '--others', '--ignored', '--exclude-standard', '--directory']).stdout)
.map((p) => p.replace(/\/$/, '').split('/')[0]);
for (const l of new Set([...ignored, ...extra])) {
if (!safeRel(l) || l === '.git') continue;
if (existsSync(join(root, l)) && !existsSync(join(tree, l))) {
mkdirSync(dirname(join(tree, l)), { recursive: true });
symlinkSync(join(root, l), join(tree, l));
linked.push(l);
}
}
return linked;
}
/** Lay the working tree's change since `base` onto `tree` (tracked edits, deletions, untracked files). */
function applyResult(root, tree, base) {
const status = lines(git(root, ['diff', '--name-status', '--no-renames', base]).stdout);
for (const s of status) {
const [code, rel] = s.split('\t');
if (!safeRel(rel)) continue;
if (code.startsWith('D')) rmSync(join(tree, rel), { recursive: true, force: true });
else if (existsSync(join(root, rel)) || lstatSync(join(root, rel), { throwIfNoEntry: false })) copyIn(root, tree, rel);
}
for (const rel of lines(git(root, ['ls-files', '--others', '--exclude-standard']).stdout)) {
if (safeRel(rel)) copyIn(root, tree, rel);
}
}
/**
* @param {{cwd: string, base: string, cmd: string, tests: string[], link?: string[], redOnly?: boolean, timeoutMs?: number}} o
* @returns {{status: 'proven'|'rejected'|'not-run', reason: string, base?: object, result?: object, control?: object}}
*/
export function proveRed(o) {
const given = resolve(o.cwd ?? process.cwd());
const timeoutMs = o.timeoutMs ?? DEFAULT_TIMEOUT_MS;
const tests = (o.tests ?? []).filter(Boolean);
if (!existsSync(given)) return notRun(`--cwd ${given} does not exist`);
// Plan paths and commands are relative to the repo root, and `git archive`
// run from a subdirectory packs only that subdirectory (measured
// 2026-10-06). So everything below runs at the root of --cwd's repo.
const cwd = repoRoot(given);
if (!cwd) return notRun(`--cwd ${given} is not inside a git repository`);
if (tests.length === 0) return notRun('no test files given: a step without its test cannot prove red');
for (const t of tests) {
if (isAbsolute(t) || t.split('/').includes('..')) return notRun(`test path must be repo-relative: ${t}`);
if (!safeRel(t)) return notRun(`test path must be repo-relative: ${t}`);
if (!existsSync(join(cwd, t))) return notRun(`test file not found in the working tree: ${t}`);
}
for (const l of o.link ?? []) if (!safeRel(l)) return notRun(`--link must be repo-relative: ${l}`);
const allowed = allowedCommand(o.cmd, { cwd });
if (!allowed.allowed) return notRun(`command not run: ${allowed.reason}`);
const screen = screenCommand(o.cmd);
if (!screen.allowed) return notRun(`command blocked: ${screen.rule}`);
const sha = spawnSync('git', ['rev-parse', '--verify', '--quiet', `${o.base}^{commit}`], { cwd, encoding: 'utf8' });
const sha = git(cwd, ['rev-parse', '--verify', '--quiet', `${o.base}^{commit}`]);
if (sha.status !== 0) return notRun(`cannot resolve the start point ${o.base} in ${cwd}`);
const baseSha = sha.stdout.trim();
@ -122,32 +176,29 @@ export function proveRed(o) {
const tmp = mkdtempSync(join(tmpdir(), 'voyage-red-proof-'));
try {
const tree = join(tmp, 'tree');
mkdirSync(tree);
const tar = join(tmp, 'start.tar');
const a = spawnSync('git', ['archive', '--format=tar', '-o', tar, baseSha], { cwd, encoding: 'utf8' });
if (a.status !== 0) return notRun(`git archive ${baseSha} failed: ${String(a.stderr).trim()}`, { result });
const x = spawnSync('tar', ['-xf', tar, '-C', tree], { encoding: 'utf8' });
if (x.status !== 0) return notRun(`tar could not unpack the start point: ${String(x.stderr).trim()}`, { result });
for (const t of tests) {
mkdirSync(dirname(join(tree, t)), { recursive: true });
cpSync(join(cwd, t), join(tree, t));
}
for (const l of o.link ?? []) {
if (!l || isAbsolute(l) || l.split('/').includes('..')) return notRun(`--link must be repo-relative: ${l}`, { result });
if (existsSync(join(cwd, l)) && !existsSync(join(tree, l))) {
mkdirSync(dirname(join(tree, l)), { recursive: true });
symlinkSync(join(cwd, l), join(tree, l));
}
}
const c = git(tmp, ['clone', '--quiet', '--shared', '--no-checkout', cwd, tree]);
if (c.status !== 0) return notRun(`could not clone the start point: ${String(c.stderr).trim()}`, { result });
const co = git(tree, ['checkout', '--quiet', '--detach', baseSha]);
if (co.status !== 0) return notRun(`could not check out ${baseSha}: ${String(co.stderr).trim()}`, { result });
for (const t of tests) copyIn(cwd, tree, t);
const linked = linkDependencies(cwd, tree, o.link ?? []);
const base = run(o.cmd, tree, timeoutMs);
const baseBroken = unrunnable(base);
if (baseBroken) return notRun(`at the start point: ${baseBroken}`, { base, result });
const out = { base_sha: baseSha, base, result };
if (baseBroken) return notRun(`at the start point: ${baseBroken}`, { base, result, linked });
const out = { base_sha: baseSha, linked, base, result };
if (base.exit === 0) return { status: 'rejected', reason: 'not-red-at-base', ...out };
if (o.redOnly) return { status: 'proven', reason: 'red at the start point (--red-only: the result was not run)', base_sha: baseSha, base };
if (o.redOnly) return { status: 'proven', reason: 'red at the start point (--red-only: the result was not run)', base_sha: baseSha, linked, base };
if (result.exit !== 0) return { status: 'rejected', reason: 'not-green-at-result', ...out };
return { status: 'proven', reason: 'red at the start point, green at the result', ...out };
// Control: the same copy with the whole change laid on. Red here means the
// copy itself cannot run the test green, so its red proved nothing.
applyResult(cwd, tree, baseSha);
const control = run(o.cmd, tree, timeoutMs);
if (control.exit !== 0) {
return notRun('the control run (start-point copy + the change) stayed red: the start point failed for an environment reason, not for the missing change', { ...out, control });
}
return { status: 'proven', reason: 'red at the start point, green at the result, green in the control', ...out, control };
} finally {
rmSync(tmp, { recursive: true, force: true });
}
@ -173,7 +224,7 @@ export function main(argv) {
else if (a === '--red-only') o.redOnly = true;
else if (a === '--timeout' && argv[i + 1]) o.timeoutMs = Number(argv[++i]);
else if (a === '--json') json = true;
else { process.stderr.write(`red-proof: unknown argument ${a}\n${USAGE}\n`); return 2; }
else { process.stderr.write(`red-proof: unknown argument ${a} (--tests takes ONE comma-separated list)\n${USAGE}\n`); return 2; }
}
if (!o.base || !o.cmd) { process.stderr.write(`red-proof: --base and --cmd are required\n${USAGE}\n`); return 2; }
if (o.timeoutMs !== undefined && !Number.isFinite(o.timeoutMs)) { process.stderr.write(`red-proof: --timeout must be a number\n${USAGE}\n`); return 2; }
@ -186,6 +237,8 @@ export function main(argv) {
process.stdout.write(`red proof: ${r.status.toUpperCase()} - ${r.reason}\n`);
if (r.base) process.stdout.write(` start point ${r.base_sha ?? o.base}: exit ${r.base.exit}${counts(r.base)}\n`);
if (r.result) process.stdout.write(` result: exit ${r.result.exit}${counts(r.result)}\n`);
if (r.control) process.stdout.write(` control: exit ${r.control.exit}${counts(r.control)}\n`);
if (r.linked?.length) process.stdout.write(` linked into the start point: ${r.linked.join(', ')}\n`);
}
return r.status === 'proven' ? 0 : r.status === 'rejected' ? 1 : 2;
}

15
lib/verification/test-integrity.mjs
View file

@ -45,6 +45,7 @@ export function isTestPath(p) {
}
const isCheckPath = (p) => isTestPath(p) || CHECK_CONFIG_RE.test(p);
const PROSE_RE = /\.(md|markdown|mdx|txt|rst|adoc)$/i;
// --- the patterns -------------------------------------------------------------
@ -55,7 +56,7 @@ const TEST_DECL_RE =
/^\s*(test|it|describe|suite)(\.\w+)?\s*\(|^\s*(async\s+)?def\s+test_\w*|^\s*func\s+Test\w*|^\s*#\[test\]|@Test\b/;
const SKIP_RE =
/\b(test|it|describe|suite)\.(skip|todo|only)\s*\(|\bx(it|describe|test)\s*\(|\bf(it|describe)\s*\(|\{\s*(skip|todo)\s*:\s*(true|['"`])|@pytest\.mark\.(skip|skipif|xfail)\b|\bpytest\.skip\s*\(|@unittest\.skip|\bt\.Skip(Now|f)?\s*\(|#\[ignore\]|@(Disabled|Ignore)\b/;
/\b(test|it|describe|suite)\.(skip|skipIf|todo|only)\s*\(|\bt\.(skip|todo)\s*\(|\bx(it|describe|test)\s*\(|\bf(it|describe)\s*\(|\{\s*(skip|todo)\s*:\s*(true|['"`])|@pytest\.mark\.(skip|skipif|xfail)\b|\bpytest\.skip\s*\(|@unittest\.skip|\bt\.Skip(Now|f)?\s*\(|#\[ignore\]|@(Disabled|Ignore)\b/;
// Inline suppressions silence a check wherever they appear.
const SUPPRESS_RE =
@ -183,6 +184,9 @@ export function analyzeDiff(text) {
const removed = all.filter((l) => l.type === '-');
const added = all.filter((l) => l.type === '+');
const test = isTestPath(f.path);
// Prose (docs, notes) describes code; a "not implemented" or an
// eslint-disable written in it is text, not a stub or a muted check.
const prose = PROSE_RE.test(f.path);
if (test) {
const lostAsserts = count(removed, ASSERT_RE) - count(added, ASSERT_RE);
@ -196,7 +200,7 @@ export function analyzeDiff(text) {
for (const l of addedNotRemoved(removed, added, SKIP_RE)) findings.push(finding('skipped-test', f.path, l));
}
for (const l of addedNotRemoved(removed, added, SUPPRESS_RE)) findings.push(finding('muted-check', f.path, l));
if (!prose) for (const l of addedNotRemoved(removed, added, SUPPRESS_RE)) findings.push(finding('muted-check', f.path, l));
if (isCheckPath(f.path)) {
for (const l of addedNotRemoved(removed, added, MUTE_RE)) findings.push(finding('muted-check', f.path, l));
for (const h of f.hunks) {
@ -211,7 +215,7 @@ export function analyzeDiff(text) {
}
}
if (!test) for (const l of addedNotRemoved(removed, added, STUB_RE)) findings.push(finding('stub', f.path, l));
if (!test && !prose) for (const l of addedNotRemoved(removed, added, STUB_RE)) findings.push(finding('stub', f.path, l));
}
return { status: findings.length ? 'findings' : 'clean', findings };
}
@ -227,7 +231,10 @@ function diffFromGit(cwd, base, head) {
const okHead = spawnSync('git', ['rev-parse', '--verify', '--quiet', `${head}^{commit}`], { cwd, encoding: 'utf8' });
if (okHead.status !== 0) return { error: `cannot resolve --head ${head} in ${cwd}` };
}
const args = ['diff', '--no-color', '--no-ext-diff', '--find-renames', base, ...(head ? [head] : [])];
// --no-renames: a rename is read as a deletion plus an addition. Paired as a
// rename, `foo.test.js -> foo.test.js.bak` showed no removed line at all and
// switched a test off silently (review, 2026-10-06).
const args = ['diff', '--no-color', '--no-ext-diff', '--no-renames', base, ...(head ? [head] : [])];
const r = spawnSync('git', args, { cwd, encoding: 'utf8', maxBuffer: 256 * 1024 * 1024 });
if (r.status !== 0) return { error: `git diff failed: ${String(r.stderr).trim()}` };
return { text: r.stdout };