fix(verdict-gate): a failed git call is reported as not measured, not stale

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-26 14:00:42 +01:00
commit 77544841d7
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
7 changed files with 233 additions and 13 deletions

View file

@ -2610,7 +2610,10 @@ Exit 2 with a reason that starts `review-stale` is closed with the decline
reason `review-stale` and the line "re-run /trekreview on this commit": the
review is of another commit than this one, or something other than the
review lessons and the `CLAUDE.md` pointer changed after it. Any other exit 2
is `review-not-measured`.
is `review-not-measured`. Exit 2 with a reason that starts `review-not-measured:`
(the `--repo` path is missing, it is no git repository, or a git call failed)
is reported as "not measured", never green, and the merge stays locked with
the decline reason `review-not-measured`.
The merge runs only on exit 0 and a `completed` result. **Closed:** emit
`main-merge-declined` with the reason `review-block`, `review-stale`,

View file

@ -431,7 +431,7 @@ missing `exit_code` (C15, `PROGRESS_FAILED_CRITERIA_KEY`).
**Consumer:** `/trekplan` Phase 1 when `--brief review.md` is supplied and the consumer detects `type: trekreview` in frontmatter. The plan command branches into a remediation-plan path: BLOCKER + MAJOR findings become plan goals, the produced `plan.md` carries a `source_findings: [<id>, ...]` frontmatter list as the audit trail back to the consumed findings. MINOR + SUGGESTION are skipped for plan-input. Since 1.1 every BLOCKER is also written as a numbered `### Step R<n>` under `## Remediation Steps` (Files · Source finding · Test first · Verify · On failure), so a consumer can append it as a plan step without rewording it.
**Second consumer — the verdict gate (K7, 1.1):** `lib/review/verdict-gate.mjs <review.md>` reads the frontmatter `verdict` and exits 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide. Exit 2 is never green: a missing file, a missing or unknown verdict, two verdict lines, a trailing JSON block that is not the last fence or lacks `verdict`/`counts`/`findings`, and a file whose JSON block, `## Findings (BLOCKER)` section or finding-id list contradicts a greener verdict all land there — and so does a WARN that exists because a reviewer or engine did not run (`missing-reviewer:` / `unattributable-payload` in the JSON block's `allow_blocked_by`, which a 1.1 review must carry): a review not performed in full stops a merge (operator decision 2026-10-06). Exactly one file per call. With `--repo <dir>` (`verdict-gate.mjs --json <review.md> --repo <dir>`, R6) an ALLOW/WARN review must also be of the commit being merged in `<dir>`: its `scope_sha_end` is HEAD, or an ancestor of HEAD whose later commits change only the review lessons (`.claude/rules/voyage-review-lessons.md`, inside its managed block) and the `CLAUDE.md` pointer (`lib/review/reviewed-commit.mjs`); otherwise exit 2 with a reason that starts `review-stale:`, which `/trekexecute`'s merge lock (Phase 7.2 step 4 and Phase 8, both with `--repo .`) maps to the decline reason `review-stale`. A `--repo` with no value is exit 2 as well; BLOCK stays exit 1. Whatever stops a merge or a release on a review reads this exit code, never a summary. Borrowed: the verdict goes to a file and a command gates on it (Kiro `feature-pipeline`); the executor reads the verdict but never writes it (Spec Kit).
**Second consumer — the verdict gate (K7, 1.1):** `lib/review/verdict-gate.mjs <review.md>` reads the frontmatter `verdict` and exits 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide. Exit 2 is never green: a missing file, a missing or unknown verdict, two verdict lines, a trailing JSON block that is not the last fence or lacks `verdict`/`counts`/`findings`, and a file whose JSON block, `## Findings (BLOCKER)` section or finding-id list contradicts a greener verdict all land there — and so does a WARN that exists because a reviewer or engine did not run (`missing-reviewer:` / `unattributable-payload` in the JSON block's `allow_blocked_by`, which a 1.1 review must carry): a review not performed in full stops a merge (operator decision 2026-10-06). Exactly one file per call. With `--repo <dir>` (`verdict-gate.mjs --json <review.md> --repo <dir>`, R6) an ALLOW/WARN review must also be of the commit being merged in `<dir>`: its `scope_sha_end`, read only as a full 40- or 64-hex commit id (a short id or a ref name is stale), is HEAD, or an ancestor of HEAD whose later commits change only the review lessons (`.claude/rules/voyage-review-lessons.md`, inside its managed block) and the `CLAUDE.md` pointer (`lib/review/reviewed-commit.mjs`); otherwise exit 2 with a reason that starts `review-stale:`, which `/trekexecute`'s merge lock (Phase 7.2 step 4 and Phase 8, both with `--repo .`) maps to the decline reason `review-stale`. When the gate cannot look (the `--repo` path is missing, it is no git repository, or a git call failed), the exit is 2 with a reason that starts `review-not-measured:`, never `review-stale:`: the merge lock reports it as not measured, never green. A `--repo` with no value is exit 2 as well; BLOCK stays exit 1. Whatever stops a merge or a release on a review reads this exit code, never a summary. Borrowed: the verdict goes to a file and a command gates on it (Kiro `feature-pipeline`); the executor reads the verdict but never writes it (Spec Kit).
**Path conventions:**
- Project-dir mode (recommended): `{project_dir}/review.md` (one per review iteration; subsequent runs overwrite atomically).
@ -442,12 +442,12 @@ missing `exit_code` (C15, `PROGRESS_FAILED_CRITERIA_KEY`).
| Field | Type | Required | Allowed values | Notes |
|---|---|---|---|---|
| `type` | string | yes | `trekreview` | Hard-coded discriminator |
| `review_version` | string | yes | `"1.0"` (current) | Bump on schema change |
| `review_version` | string | yes | `"1.1"` (current) | Bump on schema change |
| `task` | string | yes | one-line description | Mirrors brief task |
| `slug` | string | yes | URL-safe slug | Used in project_dir |
| `project_dir` | string | yes | `.claude/projects/{date}-{slug}/` | |
| `brief_path` | string | yes | path to consumed `brief.md` | Audit trail back to brief |
| `scope_sha_end` | string | yes | git sha of HEAD at review time | Defines "after" boundary |
| `scope_sha_end` | string | yes | git sha of HEAD at review time, as a full 40- or 64-hex commit id | Defines "after" boundary. The verdict gate reads only a full 40- or 64-hex commit id (M11): a short id or a ref name can also be a branch, which resolves to wherever it points now |
| `reviewed_files_count` | number | yes | ≥ 0 | From triage gate Coverage |
| `findings` | list | yes | block-style YAML list of 40-char hex IDs; `[]` when empty | Flat array; full objects in body. A bare `findings:` is null and fails |
| `verdict` | string | yes (1.1; strict at write, warning at read) | `BLOCK \| WARN \| ALLOW` | Coordinator output; equals the trailing JSON block's `verdict` (K7) |

View file

@ -20,11 +20,20 @@
// target shows as a changed path).
//
// No CLI of its own: the command is `lib/review/verdict-gate.mjs --repo <dir>`,
// which exits 2 (never green) with the reason `review-stale: …` when
// `checkReviewedCommit` is not ok. Every git call is an argv array with no
// shell; a git call that fails is a stale review (fails closed), never ok.
// which exits 2 (never green) with the reason when `checkReviewedCommit` is
// not ok. Two kinds of reason (M4):
//
// - `review-stale: …` — the gate looked, and the review is of another
// commit (or something else changed after it);
// - `review-not-measured: …` — the gate could not look: the `--repo` path
// is missing, it is no git repository, or a git call failed. That is not
// a finding about the review, so it is never reported as stale.
//
// Every git call is an argv array with no shell; a failed git call fails
// closed (not measured), never ok.
import { spawnSync } from 'node:child_process';
import { existsSync } from 'node:fs';
import { LESSONS_FILE_NAME, MARK_START, MARK_END, withPointer } from './recurrence.mjs';
/**
@ -59,14 +68,19 @@ function stale(reason) {
return { ok: false, reason: `review-stale: ${reason}` };
}
/** The gate could not look (M4): exit 2 like a stale review, but never called one. */
function notMeasured(reason) {
return { ok: false, reason: `review-not-measured: ${reason}` };
}
/** What a git call said: its spawn error, else the first line of its stderr. */
function gitNote(r) {
return r.error?.message ?? String(r.stderr ?? '').trim().split('\n')[0];
}
/** A failed git call as a stale reason: fails closed. */
/** A failed git call: not measured, fails closed. */
function gitFailed(what, r) {
return stale(`git ${what} failed (${gitNote(r) || `exit ${r.status}`})`);
return notMeasured(`git ${what} failed (${gitNote(r) || `exit ${r.status}`})`);
}
const short = (c) => c.slice(0, 12);
@ -102,17 +116,26 @@ function blobAt(repo, rev, path) {
* Is the review of `sha` a review of what HEAD holds in `repo`?
* @param {{ repo: string, sha: string }} args
* @returns {{ ok: true } | { ok: false, reason: string }} reason starts `review-stale:`
* (the review is of another commit) or `review-not-measured:` (the gate could not look)
*/
export function checkReviewedCommit({ repo, sha } = {}) {
if (typeof repo !== 'string' || repo === '') return stale('no repository given');
if (!existsSync(repo)) return notMeasured(`--repo ${repo} does not exist`);
const given = String(sha ?? '');
const notCommit = `reviewed commit ${given} is not a commit in this repository`;
// 1. Full hex only, so a value like `--output=/x` never reaches git as an option.
if (!FULL_SHA_RE.test(given)) return stale(notCommit);
const resolved = git(repo, ['rev-parse', '--verify', '--quiet', `${given}^{commit}`]);
// `--quiet` makes an unknown commit exit 1 with nothing on stderr: that is
// stale. A spawn error, any other exit, or stderr text (`not a git
// repository`, a broken git) means the gate could not look.
if (resolved.status === 1 && !resolved.error && String(resolved.stderr ?? '').trim() === '') {
return stale(notCommit);
}
if (resolved.status !== 0) {
const note = gitNote(resolved);
return stale(note ? `${notCommit} (${note})` : notCommit);
if (/not a git repository/.test(note)) return notMeasured(`${repo} is not a git repository (${note})`);
return gitFailed('rev-parse --verify', resolved);
}
const full = resolved.stdout.trim();
// A ref named like an id (64 hex in a SHA-1 repository) also resolves: the

View file

@ -19,7 +19,11 @@
// review of another commit than the one being merged (reason
// `review-stale: …`: its `scope_sha_end` is not HEAD, or an ancestor
// of HEAD whose later commits change only the review lessons and the
// CLAUDE.md pointer; lib/review/reviewed-commit.mjs, R6).
// CLAUDE.md pointer; lib/review/reviewed-commit.mjs, R6), or, with
// `--repo <dir>`, a check that could not look (reason
// `review-not-measured: …`: the path is missing, it is no git
// repository, or a git call failed; M4) — printed as it is, never
// as `review-stale`.
// Exit 2 is NEVER green.
//
// Fail-closed reading: a missing `verdict` is exit 2, never ALLOW. A verdict

View file

@ -22,7 +22,7 @@
| trekbrief-stats | ts, task, slug, mode, interview_turns, review_iterations, brief_quality, research_topics, auto_research, auto_result, intent_approved, project_dir, order_id, order_claimed_at, order_reason | commands/trekbrief.md (orchestrator-emit Phase 7; order_* added by lib/stats/stats-append.mjs) | trekbrief.md:1141-1165 | profile, phase_models, profile_source | none |
| trekresearch-stats | ts, question, mode, scope, engine, slug, project_dir, brief_path, dimensions, dimensions_baseline, dimensions_baseline_preserved, effort, conv_turns, empty_turns, unique_sources, agents_local, agents_external, gemini_used, confidence, contradictions, open_questions | lib/stats/stats-append.mjs (trekresearch.md Stats tracking) | trekresearch.md:822-855 | profile, phase_models, parallel_agents, external_research_enabled, profile_source | none |
| trekplan-stats | ts, task, mode, slug, brief_path, project_dir, codebase_size, codebase_files, agents_deployed, deep_dives, research_briefs_used, research_scout_used, critic_verdict, guardian_verdict, outcome | lib/stats/stats-append.mjs (trekplan.md Phase 12) | trekplan.md:1307-1335 | profile, phase_models, parallel_agents, profile_source | none |
| trekexecute-stats (Phase 9 record) | ts, plan, plan_type, mode, result, steps_total, steps_passed, steps_failed, steps_skipped, failed_at_step | lib/stats/stats-append.mjs (trekexecute.md Phase 9) | trekexecute.md:2969-2991 | profile, phase_models, profile_source | none |
| trekexecute-stats (Phase 9 record) | ts, plan, plan_type, mode, result, steps_total, steps_passed, steps_failed, steps_skipped, failed_at_step | lib/stats/stats-append.mjs (trekexecute.md Phase 9) | trekexecute.md:2972-2994 | profile, phase_models, profile_source | none |
| trekexecute-stats (autonomy events) | ts, event, known_event, payload | lib/stats/event-emit.mjs `emit()` | event-emit.mjs:78-99 | payload.profile, payload.phase_models, payload.profile_source | none |
| trekexecute-stats (PostToolUse Bash) | ts, session_id, command_excerpt, duration_ms, success | hooks/scripts/post-bash-stats.mjs (Bash PostToolUse) | post-bash-stats.mjs:42-54 | none (hook is plugin-level, not profile-aware) | command_excerpt (CWE-212) |
| trekreview-stats | ts, slug, verdict, counts (BLOCKER/MAJOR/MINOR/SUGGESTION), reviewed_files_count, mode, duration_ms | lib/stats/stats-append.mjs (trekreview.md Phase 8) | trekreview.md:640-690 | profile, phase_models, profile_source | none |

View file

@ -0,0 +1,103 @@
// tests/lib/fix-round-4-5b-prose.test.mjs
// Prose pins for fix round 4-5b (plan
// .claude/projects/2026-10-25-fix-round-4-5b-findings): the fixes that are
// text a session or a helper reads. The tags in brackets are 4.5b ids from the
// round's brief (for example `[M4]`, `[M11]`); each tagged test fails on an
// assertion at be0b631, the round's start point. A sibling's test is tagged
// `[<parent id>] sibling:`.
// M4 A git call the verdict gate cannot complete is reported as
// `review-not-measured:`, never as a stale review: Handover 6 names the
// prefix and lists review_version 1.1 as current, and /trekexecute's
// main-merge gate maps it to "not measured" with the merge locked.
// M11 Handover 6 states that only a full 40- or 64-hex reviewed commit is read.
// No static import of a new export: the file must load at be0b631.
// Fix the SOURCE a pin guards, not the pin.
import { test } from 'node:test';
import { strict as assert } from 'node:assert';
import { existsSync, readFileSync } from 'node:fs';
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
const readCache = new Map();
/** A repo file's text; the file must exist. Read from disk once per run. */
function read(rel) {
if (!readCache.has(rel)) {
assert.ok(existsSync(join(ROOT, rel)), `missing file ${rel}`);
readCache.set(rel, readFileSync(join(ROOT, rel), 'utf8'));
}
return readCache.get(rel);
}
/** Whitespace (line breaks too) collapsed to one space. */
const flat = (s) => s.replace(/\s+/g, ' ').trim();
/** The text from the line holding `start` up to (not including) the next line holding `stop`; both must exist. */
function between(rel, start, stop) {
const text = read(rel);
const s = text.indexOf(start);
assert.ok(s !== -1, `${rel} must hold "${start}"`);
const e = text.indexOf(stop, s + start.length);
assert.ok(e !== -1, `${rel} must hold "${stop}" after "${start}"`);
return text.slice(s, e);
}
/** Handover 6 of the handover contracts, up to Handover 7. */
const handover6 = () => between('docs/HANDOVER-CONTRACTS.md', '## Handover 6 ', '## Handover 7 ');
/** The one table row of Handover 6 whose first cell is `field`. */
function fieldRow(field) {
const rows = handover6().split('\n').filter((l) => l.startsWith(`| \`${field}\` |`));
assert.equal(rows.length, 1, `Handover 6 must hold exactly one \`${field}\` row; found ${rows.length}`);
return rows[0];
}
/** The verdict-gate paragraph of Handover 6 (the "Second consumer" one), flattened. */
function gateParagraph() {
const para = handover6().split('\n').filter((l) => l.startsWith('**Second consumer — the verdict gate'));
assert.equal(para.length, 1, `Handover 6 must hold one verdict-gate paragraph; found ${para.length}`);
return flat(para[0]);
}
/** Every sentence of `text` (flattened) that holds `needle`. */
function sentencesWith(text, needle) {
return flat(text).split(/(?<=\.)\s+(?=[A-Z`"*(])/).filter((s) => s.includes(needle));
}
// --- M4 ----------------------------------------------------------------------------
test('[M4] Handover 6 names the review-not-measured reason', () => {
const hits = sentencesWith(gateParagraph(), '`review-not-measured:`');
assert.ok(hits.length > 0, 'the verdict-gate paragraph of Handover 6 does not name the `review-not-measured:` reason prefix');
assert.ok(hits.some((s) => /\bgit\b/.test(s) && /never green|not measured/.test(s)),
`no sentence ties \`review-not-measured:\` to a failed git call read as not measured: ${hits.join(' | ')}`);
});
test('[M4] Handover 6 lists review_version 1.1 as current', () => {
const row = fieldRow('review_version');
assert.match(row, /"1\.1"`? \(current\)/, `the review_version row does not list 1.1 as current: ${row}`);
assert.doesNotMatch(row, /"1\.0"`? \(current\)/, `the review_version row still lists 1.0 as current: ${row}`);
});
test('[M4] the main merge gate maps review-not-measured to not measured', () => {
const gate = between('commands/trekexecute.md', '**1. The verdict.**', '**2. Open:');
const hits = sentencesWith(gate, '`review-not-measured:`');
assert.ok(hits.length > 0, 'the main-merge gate (Phase 8, 1. The verdict) does not name the `review-not-measured:` reason prefix');
const ok = hits.find((s) => {
const rest = s.split('`review-not-measured:`').join('');
return /"not measured"|\bnot measured\b/.test(rest) && /never green/.test(rest) && /merge stays (locked|closed)/.test(rest);
});
assert.ok(ok, `no sentence says a \`review-not-measured:\` reason is reported as "not measured", never green, with the merge locked: ${hits.join(' | ')}`);
});
// --- M11 ---------------------------------------------------------------------------
test('[M11] Handover 6 states the full 40- or 64-hex reviewed commit', () => {
const row = fieldRow('scope_sha_end');
assert.match(row, /full 40- or 64-hex/, `the scope_sha_end row does not state the full 40- or 64-hex commit id: ${row}`);
assert.match(gateParagraph(), /`scope_sha_end`[^.]*full 40- or 64-hex/,
'the verdict-gate paragraph of Handover 6 does not state that only a full 40- or 64-hex scope_sha_end is read');
});

View file

@ -8,16 +8,20 @@
// 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.
// [M4] A git call that fails, a --repo path that is missing and a --repo that
// is no repository are exit 2 `review-not-measured: ...`, never review-stale:
// the gate could not look, so it cannot say the review is of another commit.
import { after, test } from 'node:test';
import { strict as assert } from 'node:assert';
import { spawnSync } from 'node:child_process';
import { mkdtempSync, mkdirSync, writeFileSync, rmSync, existsSync, symlinkSync, appendFileSync, cpSync, readFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join, dirname } from 'node:path';
import { join, dirname, delimiter } 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';
import { pathWith } from '../helpers/bin-path.mjs';
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
@ -690,3 +694,86 @@ test('[M6] --json is accepted while --jsonx is refused', () => {
assert.match(bad.stderr, /unknown option --jsonx/);
});
});
// ---- M4: a git that fails, or a --repo it cannot look in, is not measured ----
/**
* A PATH whose first `git` fails the subcommand `sub` (exit 128, one stderr
* line) and runs the real git for every other one. The real git is reached
* through bin-path.mjs's pass-through wrapper, so this file never guesses
* where git lives.
*/
function failingGit(sub) {
const real = pathWith(['git']);
const dir = mkdtempSync(join(tmpdir(), 'verdict-gate-m4-git-'));
writeFileSync(join(dir, 'git'),
`#!/bin/sh\nif [ "$1" = "${sub}" ]; then echo "fatal: fake git: ${sub} fails" >&2; exit 128; fi\nexec '${join(real.path, 'git')}' "$@"\n`,
{ mode: 0o755 });
return {
path: `${dir}${delimiter}${process.env.PATH ?? ''}`,
cleanup: () => { real.cleanup(); rmSync(dir, { recursive: true, force: true }); },
};
}
/** The gate on an ALLOW review of the fixture's reviewed commit, with `env` and `repo` given. */
function gateWith(fx, { env = {}, repo = fx.dir } = {}) {
const p = join(fx.base, 'review.md');
writeFileSync(p, review({ verdict: 'ALLOW', severities: ['MINOR'], scopeShaEnd: fx.reviewed }));
return spawnSync(process.execPath, [GATE, '--json', p, '--repo', repo], { encoding: 'utf-8', env: childEnv(env) });
}
function assertNotMeasured(r, re) {
assert.equal(r.status, 2, r.stdout + r.stderr);
assert.doesNotMatch(r.stdout, /review-stale/);
const { reason } = JSON.parse(r.stdout);
assert.ok(String(reason).startsWith('review-not-measured:'), `the reason must start review-not-measured: ${reason}`);
if (re) assert.match(reason, re);
}
test('[M4] a git that cannot run rev-parse is not measured', () => {
const fx = gateRepo();
const git = failingGit('rev-parse');
try {
assertNotMeasured(gateWith(fx, { env: { PATH: git.path } }), /rev-parse/);
} finally { git.cleanup(); fx.cleanup(); }
});
test('[M4] a git that fails rev-list after the commit resolves is not measured', () => {
const fx = gateRepo();
const git = failingGit('rev-list');
try {
lessonsCommit(fx);
// Control: with the real git the same review opens the gate, so the run below gets past rev-parse.
const control = gateWith(fx);
assert.equal(control.status, 0, control.stdout + control.stderr);
assertNotMeasured(gateWith(fx, { env: { PATH: git.path } }), /rev-list/);
} finally { git.cleanup(); fx.cleanup(); }
});
test('[M4] a git that fails diff after the commit resolves is not measured', () => {
const fx = gateRepo();
const git = failingGit('diff');
try {
lessonsCommit(fx);
const control = gateWith(fx);
assert.equal(control.status, 0, control.stdout + control.stderr);
assertNotMeasured(gateWith(fx, { env: { PATH: git.path } }), /diff/);
} finally { git.cleanup(); fx.cleanup(); }
});
test('[M4] --repo with a missing path is not measured', () => {
const fx = gateRepo();
try {
assertNotMeasured(gateWith(fx, { repo: join(fx.base, 'no-such-dir') }), /no-such-dir/);
} finally { fx.cleanup(); }
});
test('[M4] --repo that is no repository is not measured', () => {
const fx = gateRepo();
try {
const plain = join(fx.base, 'plain');
mkdirSync(plain);
// The ceiling stops git from finding a repository above the fixture.
assertNotMeasured(gateWith(fx, { repo: plain, env: { GIT_CEILING_DIRECTORIES: fx.base } }), /not a git repository/);
} finally { fx.cleanup(); }
});