fix(verification): the done line and the release check take only full commit ids

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-26 13:33:51 +01:00
commit e279f12131
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
4 changed files with 73 additions and 8 deletions

View file

@ -12,7 +12,8 @@
//
// `--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
// --commit), 1 when it does not, 2 when --commit is not a full 40- or 64-hex
// commit id (a short id can also be a branch name; never green). /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.
//
@ -26,17 +27,26 @@ import { resolve } from 'node:path';
import { DEFAULT_TIMEOUT_MS, allowedCommand, runCommand, screenCommand, stoppedBy } from './criteria-runner.mjs';
import { repoRoot } from './red-proof.mjs';
import { isMain } from '../util/is-main.mjs';
import { FULL_SHA_RE } from '../review/reviewed-commit.mjs';
/**
* A line names a full commit id: 40 hex (SHA-1) or 64 hex (SHA-256), never a
* short one (M11). A `commit` that is not a full id matches nothing; the CLI
* refuses it before it gets here.
*/
export function doneLineRe(step, commit) {
const sha = commit ? String(commit).replace(/[^0-9a-f]/g, '') : '[0-9a-f]{40}';
const sha = commit === undefined || commit === null || commit === ''
? '(?:[0-9a-f]{40}|[0-9a-f]{64})'
: FULL_SHA_RE.test(String(commit)) ? String(commit).toLowerCase() : '(?!)';
return new RegExp(`^DONE step ${step}: \`.+\` exit 0 at ${sha} \\d{4}-\\d{2}-\\d{2}T\\S+$`, 'm');
}
/**
* 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).
* With `commit` (the step's checkpoint sha, a full 40- or 64-hex id), 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;
@ -89,6 +99,11 @@ export function main(argv) {
}
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; }
// A short id can also be a branch name (DJ3): only a full id binds the line.
if (o.commit !== undefined && !FULL_SHA_RE.test(o.commit)) {
process.stderr.write(`done-line: --commit ${o.commit} is not a full commit id (40 or 64 hex)\n${USAGE}\n`);
return 2;
}
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;

View file

@ -40,7 +40,7 @@ import { spawnSync } from 'node:child_process';
import { dirname, isAbsolute, join, relative, resolve } from 'node:path';
import { fileURLToPath } from 'node:url';
import { decideVerdict } from '../lib/review/verdict-gate.mjs';
import { reviewedSha } from '../lib/review/reviewed-commit.mjs';
import { FULL_SHA_RE, reviewedSha } from '../lib/review/reviewed-commit.mjs';
import { commandEnv } from '../lib/util/command-env.mjs';
import { DEFAULT_TIMEOUT_MS, stoppedBy } from '../lib/verification/criteria-runner.mjs';
import { isMain } from '../lib/util/is-main.mjs';
@ -222,10 +222,13 @@ export { reviewedSha };
function checkCoverage(repo, reviewText) {
if (reviewText === null) return { id: 'coverage', status: 'skip', detail: 'not checked: no readable review' };
const sha = reviewedSha(reviewText);
if (!sha) return { id: 'coverage', status: 'fail', detail: 'review.md has no scope_sha_end that is a commit sha (7-40 hex characters) — cannot tell what it reviewed' };
// A full id only (M11): a short id, or a 64-hex name in a SHA-1 repository,
// can be a branch name that resolves to wherever the branch points now.
if (!sha || !FULL_SHA_RE.test(sha)) return { id: 'coverage', status: 'fail', detail: 'review.md has no scope_sha_end that is a commit sha (a full 40- or 64-hex commit id) — cannot tell what it reviewed' };
const resolved = git(repo, ['rev-parse', '--verify', '--quiet', `${sha}^{commit}`]);
if (resolved.status !== 0) return { id: 'coverage', status: 'fail', detail: `the reviewed commit ${sha} is not in the history of HEAD (unknown commit)` };
const full = resolved.stdout.trim();
if (full !== sha.toLowerCase()) return { id: 'coverage', status: 'fail', detail: `review.md's scope_sha_end ${sha} names a ref, not a commit (a full 40- or 64-hex commit id) — cannot tell what it reviewed` };
const anc = git(repo, ['merge-base', '--is-ancestor', full, 'HEAD']);
if (anc.status !== 0) return { id: 'coverage', status: 'fail', detail: `the reviewed commit ${full.slice(0, 12)} is not in the history of HEAD` };

View file

@ -91,3 +91,32 @@ test('done line: --check finds the line for that step only', () => {
assert.equal(run(['--check', '--step', '4', '--report', report]).status, 1);
} finally { rmSync(dir, { recursive: true, force: true }); }
});
// ---- M11 sibling: only a full 40- or 64-hex commit id ----
test('[M11] sibling: the done line accepts a 64-hex commit id', () => {
// A SHA-256 repository writes a 64-hex HEAD into the line; --check must read it.
const dir = mkdtempSync(join(tmpdir(), 'done-line-sha256-'));
const report = join(dir, 'step-3.report.md');
const sha = '0123abcd'.padEnd(64, 'e');
try {
writeFileSync(report, `# Step 3 report\nDONE step 3: \`node --test tests/ok.test.mjs\` exit 0 at ${sha} 2026-10-06T12:00:00.000Z\n`);
const plain = run(['--check', '--step', '3', '--report', report]);
assert.equal(plain.status, 0, `a 64-hex line is a done line: ${plain.stdout}${plain.stderr}`);
const bound = run(['--check', '--step', '3', '--report', report, '--commit', sha]);
assert.equal(bound.status, 0, `the line at that 64-hex commit: ${bound.stdout}${bound.stderr}`);
} finally { rmSync(dir, { recursive: true, force: true }); }
});
test('[M11] sibling: the done line refuses a 7-hex id given with --commit, naming 40 or 64 hex', () => {
// A short id can also be a branch name (DJ3); a line typed at a short id
// must not pass the check bound to that short id.
const dir = mkdtempSync(join(tmpdir(), 'done-line-short-'));
const report = join(dir, 'step-3.report.md');
try {
writeFileSync(report, '# Step 3 report\nDONE step 3: `node --test tests/ok.test.mjs` exit 0 at deadbee 2026-10-06T12:00:00.000Z\n');
const r = run(['--check', '--step', '3', '--report', report, '--commit', 'deadbee']);
assert.equal(r.status, 2, `a 7-hex --commit could not be checked (exit 2, never green): ${r.stdout}${r.stderr}`);
assert.match(r.stdout + r.stderr, /40 or 64 hex/, `the refusal names 40 or 64 hex: ${r.stdout}${r.stderr}`);
} finally { rmSync(dir, { recursive: true, force: true }); }
});

View file

@ -536,3 +536,21 @@ test('release-check r9 / bdd46799 — one review.md reached through a symlinked
assert.equal(r.status, 0, r.stdout + r.stderr);
} finally { rmSync(link, { force: true }); fx.cleanup(); }
});
// ==== fix round 4.5b: only a full 40- or 64-hex commit id (M11 sibling) ======
test('[M11] sibling: the release check refuses a 64-hex branch name in a SHA-1 repository with the reason "(a full 40- or 64-hex commit id)"', () => {
// In a SHA-1 repository a 64-hex name is no object id; git resolves it as a
// ref name to wherever the branch points, so coverage would pass a review
// of whatever the branch is moved to.
const name = 'c0ffee'.padEnd(64, '0');
const fx = fixture({ reviewSha: name });
try {
git(fx.repo, 'branch', name, 'HEAD~1');
const r = run(fx, ['--json']);
assert.equal(r.status, 1, r.stdout + r.stderr);
const coverage = JSON.parse(r.stdout).checks.find((c) => c.id === 'coverage');
assert.equal(coverage.status, 'fail', JSON.stringify(coverage));
assert.ok(coverage.detail.includes('(a full 40- or 64-hex commit id)'), `the reason names a full commit id: ${coverage.detail}`);
} finally { fx.cleanup(); }
});