fix(review): the evidence writer takes a verdict only for the branch head

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

lib/review/merge-evidence.mjs

@@ -14,10 +14,18 @@
 //   - verification: progress.json `plan_verification.summary` passed/total;
 //   - review: the verdict gate (`decideVerdictFile` with the repo, R6 first)
 //     on {project}/review.md, and its `scope_sha_end` (`reviewedSha`); a review
-//     the gate does not decide (exit 2) has a null verdict;
+//     the gate does not decide (exit 2) has a null verdict. The gate is asked
+//     only when `git rev-parse --verify HEAD` in --repo is the branch head (R6
+//     is HEAD-based); from any other HEAD the verdict is null ("not measured"
+//     on the question) and `review.sha` is still written;
 //   - door: the plan's `## Doors` list, else the CHANGELOG's first section when
 //     it holds exactly one door line, else null.
 // A field that cannot be derived is null, and the guard prints "not measured".
+// A branch name git would read as something else is refused before the head
+// is read ({"written": false}, "branch name <b> is ambiguous ..."): when
+// `refs/tags/<b>` or `refs/<b>` resolves, or an entry of any kind
+// `<git dir>/<b>` or `<git common dir>/<b>` exists (so `voyage`, the evidence
+// directory's parent, is refused). The guard's reader keeps the same rule.
 // Before writing, evidence files in that directory older than 30 days are
 // removed (regular files named `<hex>.json` only). The file is written
 // atomically (a temp file in the same directory, then rename).
@@ -100,6 +108,26 @@ function git(repo, args) {
   return spawnSync('git', args, { cwd: repo, encoding: 'utf8' });
 }
 
+/**
+ * True when git would read `branch` as something other than refs/heads/<branch>:
+ * `refs/tags/<b>` or `refs/<b>` resolves (argv only, never a shell), or an
+ * entry of any kind (file, directory, link) `<git dir>/<b>` or
+ * `<git common dir>/<b>` exists. The guard's reader
+ * (hooks/lib/merge-evidence.mjs) shows no evidence for the same names.
+ */
+function ambiguousName(repoDir, gitDir, common, branch) {
+  for (const ref of [`refs/tags/${branch}`, `refs/${branch}`]) {
+    if (git(repoDir, ['rev-parse', '--verify', '--quiet', ref]).status === 0) return true;
+  }
+  for (const dir of [gitDir, common]) {
+    try { lstatSync(join(dir, branch)); return true; } catch (e) {
+      // No entry is ENOENT/ENOTDIR; any other error cannot tell, so it counts as ambiguous.
+      if (e?.code !== 'ENOENT' && e?.code !== 'ENOTDIR') return true;
+    }
+  }
+  return false;
+}
+
 const readText = (p) => { try { return existsSync(p) ? readFileSync(p, 'utf8') : null; } catch { return null; } };
 
 /** Remove evidence files older than MAX_AGE_MS: regular files named `<hex>.json` only. */
@@ -131,13 +159,24 @@ export function writeEvidence({ repo, branch, projectDir, now = Date.now() } = {
   const repoDir = resolve(repo);
   const project = resolve(projectDir);
 
-  const head = git(repoDir, ['rev-parse', '--verify', '--quiet', `refs/heads/${branch}`]);
-  const sha = head.status === 0 ? head.stdout.trim() : '';
-  if (!SHA_RE.test(sha)) return { written: false, reason: `no branch ${branch} in ${repoDir}` };
   const cd = git(repoDir, ['rev-parse', '--git-common-dir']);
   const rawCommon = cd.status === 0 ? cd.stdout.trim() : '';
   if (!rawCommon) return { written: false, reason: `git rev-parse --git-common-dir failed in ${repoDir}` };
   const common = isAbsolute(rawCommon) ? rawCommon : resolve(repoDir, rawCommon);
+  const gd = git(repoDir, ['rev-parse', '--git-dir']);
+  const rawGitDir = gd.status === 0 ? gd.stdout.trim() : '';
+  if (!rawGitDir) return { written: false, reason: `git rev-parse --git-dir failed in ${repoDir}` };
+  const gitDir = isAbsolute(rawGitDir) ? rawGitDir : resolve(repoDir, rawGitDir);
+  if (ambiguousName(repoDir, gitDir, common, branch)) {
+    return { written: false, reason: `branch name ${branch} is ambiguous (a tag or another ref has that name)` };
+  }
+
+  const head = git(repoDir, ['rev-parse', '--verify', '--quiet', `refs/heads/${branch}`]);
+  const sha = head.status === 0 ? head.stdout.trim() : '';
+  if (!SHA_RE.test(sha)) return { written: false, reason: `no branch ${branch} in ${repoDir}` };
+  // The gate's R6 check is HEAD-based: it is asked only when HEAD is the branch head.
+  const at = git(repoDir, ['rev-parse', '--verify', 'HEAD']);
+  const headIsBranch = at.status === 0 && at.stdout.trim() === sha;
   const top = git(repoDir, ['rev-parse', '--show-toplevel']);
   const topDir = top.status === 0 && top.stdout.trim() ? top.stdout.trim() : repoDir;
 
@@ -152,7 +191,7 @@ export function writeEvidence({ repo, branch, projectDir, now = Date.now() } = {
   const reviewPath = join(project, 'review.md');
   const reviewText = readText(reviewPath);
   let verdict = null;
-  if (reviewText !== null) {
+  if (reviewText !== null && headIsBranch) {
     try {
       const gate = decideVerdictFile(reviewPath, { repo: repoDir });
       if (gate.exit === EXIT.PASS) verdict = gate.verdict ?? null;

tests/lib/merge-evidence-writer-head.test.mjs

@@ -0,0 +1,252 @@
+// tests/lib/merge-evidence-writer-head.test.mjs
+// [WHD] The evidence writer (lib/review/merge-evidence.mjs write) asks the
+// verdict gate only when the HEAD of --repo is the head of the branch being
+// merged. The gate's R6 check is HEAD-based: from a checkout whose HEAD is
+// still the reviewed commit while the branch moved on, the gate passes the
+// review of an older commit, and the evidence file named by the new head
+// would carry that verdict. So when HEAD is not the branch head, the file's
+// `review.verdict` is null and the guard's merge question reads "review not
+// measured".
+// [AMB] The writer refuses a branch name git would read as something else (a
+// tag `refs/tags/<b>`, a ref `refs/<b>`, an entry `<git dir>/<b>` or
+// `<git common dir>/<b>`), so it writes no evidence the guard's reader would
+// refuse to show (the reader keeps the same rule).
+// `keep:` cases hold at the base already and stay untagged.
+// The writer module loads through `import * as m`, so a missing export is a
+// failed assertion at the red base, never a crash. Every child gets
+// childEnv(); refs are made with `git update-ref`, never `git tag`.
+
+import { after, test } from 'node:test';
+import { strict as assert } from 'node:assert';
+import { spawnSync } from 'node:child_process';
+import { appendFileSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, realpathSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { dirname, join } from 'node:path';
+import { fileURLToPath } from 'node:url';
+import * as m from '../../lib/review/merge-evidence.mjs';
+import { runHookWithEnv } from '../helpers/hook-helper.mjs';
+import { childEnv } from '../helpers/child-env.mjs';
+
+const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
+const PRE_BASH = join(ROOT, 'hooks', 'scripts', 'pre-bash-executor.mjs');
+const WRITER = join(ROOT, 'lib', 'review', 'merge-evidence.mjs');
+
+const made = [];
+after(() => { for (const b of made) rmSync(b, { recursive: true, force: true }); });
+
+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();
+}
+
+const run = (args, opts = {}) => spawnSync(process.execPath, args, { encoding: 'utf8', env: childEnv(), ...opts });
+const parse = (r) => { try { return JSON.parse(r.stdout); } catch { return {}; } };
+
+/**
+ * A throwaway repo on `main` (README) with a branch `feat` one commit ahead
+ * (a.mjs), checked out on main. No hooks, no signing.
+ * (Copied from tests/hooks/merge-evidence.test.mjs `mevRepo`.)
+ */
+function whdRepo() {
+  const base = realpathSync(mkdtempSync(join(tmpdir(), 'merge-evidence-whd-')));
+  made.push(base);
+  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`);
+  writeFileSync(join(repo, 'README.md'), '# t\n');
+  g(repo, 'add', 'README.md');
+  g(repo, 'commit', '-q', '-m', 'init');
+  const mainSha = g(repo, 'rev-parse', 'HEAD');
+  g(repo, 'checkout', '-q', '-b', 'feat');
+  writeFileSync(join(repo, 'a.mjs'), 'export const a = 1;\n');
+  g(repo, 'add', 'a.mjs');
+  g(repo, 'commit', '-q', '-m', 'feat: the work');
+  const featSha = g(repo, 'rev-parse', 'HEAD');
+  g(repo, 'checkout', '-q', 'main');
+  const evDir = join(repo, '.git', 'voyage', 'merge-evidence');
+  return { base, repo, mainSha, featSha, evDir, evPath: (sha = featSha) => join(evDir, `${sha}.json`) };
+}
+
+/** A minimal ALLOW review of `sha` (copied from tests/hooks/merge-evidence.test.mjs `review`). */
+function review(sha) {
+  const id = '1'.padStart(40, 'a');
+  const json = JSON.stringify({
+    verdict: 'ALLOW',
+    counts: { BLOCKER: 0, MAJOR: 0, MINOR: 1, SUGGESTION: 0 },
+    allow_blocked_by: [],
+    findings: [{ id, severity: 'MINOR', rule_key: 'MISSING_TEST', file: 'a.mjs', line: 1, title: 't', detail: 'd', recommended_action: 'Add a test.' }],
+  }, null, 2);
+  return `---
+type: trekreview
+review_version: "1.1"
+task: "t"
+slug: s
+project_dir: .claude/projects/2026-11-02-s/
+brief_path: .claude/projects/2026-11-02-s/brief.md
+scope_sha_end: ${sha}
+reviewed_files_count: 1
+verdict: ALLOW
+findings:
+  - ${id}
+---
+
+# Review: t
+
+## Executive Summary
+
+x
+
+## Coverage
+
+| File | Treatment | Reason |
+|------|-----------|--------|
+| a.mjs | summary-only | default |
+
+## Remediation Summary
+
+x
+
+\`\`\`json
+${json}
+\`\`\`
+`;
+}
+
+/** A plan with an End-to-end step (copied from tests/hooks/merge-evidence.test.mjs `plan`, no doors). */
+function plan(e2e = '/bin/bash tests/e2e-x.sh') {
+  return `# Plan
+
+> Generated by trekplan v6.5.0 on 2026-11-02 — \`plan_version: 1.9\`
+
+## Implementation Plan
+
+### Step 1: the work
+
+- **Files:** \`a.mjs\`
+- **Verify:** \`node --test tests/a.test.mjs\` → expected: pass
+
+### Step 2: End to end
+
+- **End-to-end:** \`${e2e}\` → expected: \`e2e: ok\`, exit 0.
+
+## Verification
+
+\`\`\`bash
+${e2e}
+\`\`\`
+`;
+}
+
+const PROGRESS = {
+  schema_version: '1', status: 'completed', end_to_end: 'verified',
+  plan_verification: { status: 'passed', summary: { total: 7, passed: 7, failed: 0, blocked: 0, unrunnable: 0 }, failed_criteria: [] },
+};
+
+/** A project folder outside the repo with a plan, progress.json and a review of `sha` (copied from tests/hooks/merge-evidence.test.mjs `project`). */
+function project(fx, sha) {
+  const dir = mkdtempSync(join(fx.base, 'project-'));
+  writeFileSync(join(dir, 'plan.md'), plan());
+  writeFileSync(join(dir, 'progress.json'), JSON.stringify(PROGRESS, null, 2));
+  writeFileSync(join(dir, 'review.md'), review(sha));
+  return dir;
+}
+
+/** The evidence writer's CLI (copied from tests/hooks/merge-evidence.test.mjs `write`); the module must export writeEvidence. */
+function write(repo, branch, proj) {
+  assert.equal(typeof m.writeEvidence, 'function', 'lib/review/merge-evidence.mjs exports writeEvidence');
+  return run([WRITER, 'write', '--repo', repo, '--branch', branch, '--project', proj, '--json']);
+}
+
+/** The guard on a Bash call, run in `cwd` (copied from tests/hooks/merge-evidence.test.mjs `ask`). */
+async function ask(cwd, command = 'git merge --no-ff feat') {
+  const input = { hook_event_name: 'PreToolUse', tool_name: 'Bash', cwd, tool_input: { command } };
+  const r = await runHookWithEnv(PRE_BASH, input, {});
+  let out = null;
+  try { out = JSON.parse(String(r.stdout).trim()); } catch { out = null; }
+  return {
+    ...r,
+    decision: out?.hookSpecificOutput?.permissionDecision ?? null,
+    reason: String(out?.hookSpecificOutput?.permissionDecisionReason ?? ''),
+  };
+}
+
+const evidenceFiles = (fx) => (existsSync(fx.evDir) ? readdirSync(fx.evDir).filter((n) => n.endsWith('.json')) : []);
+
+// ---- [WHD] HEAD is not the branch head ---------------------------------------------
+
+test('[WHD] a write from a worktree whose HEAD is the reviewed commit while the branch moved on gives review verdict null', async () => {
+  const fx = whdRepo();
+  const reviewed = fx.featSha;
+  // A second worktree, detached at the reviewed commit.
+  const wt = join(fx.base, 'wt');
+  g(fx.repo, 'worktree', 'add', '-q', '--detach', wt, reviewed);
+  // The branch moves on in the first worktree.
+  g(fx.repo, 'checkout', '-q', 'feat');
+  writeFileSync(join(fx.repo, 'b.mjs'), 'export const b = 2;\n');
+  g(fx.repo, 'add', 'b.mjs');
+  g(fx.repo, 'commit', '-q', '-m', 'feat: more');
+  g(fx.repo, 'checkout', '-q', 'main');
+  const head = g(fx.repo, 'rev-parse', 'refs/heads/feat');
+  assert.notEqual(head, reviewed, 'Control: the branch moved on');
+  assert.equal(g(wt, 'rev-parse', 'HEAD'), reviewed, 'Control: the second worktree\'s HEAD is the reviewed commit');
+
+  const proj = project(fx, reviewed);
+  const w = write(wt, 'feat', proj);
+  assert.equal(w.status, 0, `writer: ${w.stdout}${w.stderr}`);
+  assert.equal(parse(w).written, true, w.stdout);
+  const file = JSON.parse(readFileSync(fx.evPath(head), 'utf8'));
+  assert.equal(file.branch_head_sha, head, 'Control: the file is named by and holds the branch head');
+  assert.equal(file.review.verdict, null, `the gate is not asked when HEAD is not the branch head: ${JSON.stringify(file.review)}`);
+  assert.equal(file.review.sha, reviewed, 'Control: review.sha stays the reviewed commit');
+
+  const r = await ask(fx.repo);
+  assert.equal(r.decision, 'ask', `Control: the merge into main is asked about: ${r.stdout}${r.stderr}`);
+  assert.ok(r.reason.includes(`review not measured at ${reviewed.slice(0, 8)}.`), `the question reads "review not measured":\n${r.reason}`);
+  assert.ok(!r.reason.includes('review ALLOW'), `no review verdict is shown:\n${r.reason}`);
+});
+
+// ---- [AMB] the writer refuses an ambiguous branch name ------------------------------
+
+test('[AMB] the writer refuses a branch name that is also a tag', () => {
+  const fx = whdRepo();
+  g(fx.repo, 'update-ref', 'refs/tags/feat', fx.mainSha);
+  g(fx.repo, 'checkout', '-q', 'feat');
+  const w = write(fx.repo, 'feat', project(fx, fx.featSha));
+  assert.equal(w.status, 1, `refused: ${w.stdout}${w.stderr}`);
+  const out = parse(w);
+  assert.equal(out.written, false, w.stdout);
+  assert.match(String(out.reason), /branch name feat is ambiguous/, w.stdout);
+  assert.deepEqual(evidenceFiles(fx), [], 'nothing is written');
+});
+
+test('[AMB] the writer refuses a branch name that is also refs/<name>', () => {
+  const fx = whdRepo();
+  g(fx.repo, 'update-ref', 'refs/feat', fx.mainSha);
+  g(fx.repo, 'checkout', '-q', 'feat');
+  const w = write(fx.repo, 'feat', project(fx, fx.featSha));
+  assert.equal(w.status, 1, `refused: ${w.stdout}${w.stderr}`);
+  const out = parse(w);
+  assert.equal(out.written, false, w.stdout);
+  assert.match(String(out.reason), /branch name feat is ambiguous/, w.stdout);
+  assert.deepEqual(evidenceFiles(fx), [], 'nothing is written');
+});
+
+// ---- keep: the normal path ---------------------------------------------------------
+
+test('keep: a write with HEAD at the branch head keeps review ALLOW', async () => {
+  const fx = whdRepo();
+  g(fx.repo, 'checkout', '-q', 'feat');
+  const w = write(fx.repo, 'feat', project(fx, fx.featSha));
+  assert.equal(w.status, 0, `writer: ${w.stdout}${w.stderr}`);
+  assert.equal(parse(w).written, true, w.stdout);
+  const file = JSON.parse(readFileSync(fx.evPath(), 'utf8'));
+  assert.deepEqual(file.review, { verdict: 'ALLOW', sha: fx.featSha });
+  g(fx.repo, 'checkout', '-q', 'main');
+  const r = await ask(fx.repo);
+  assert.equal(r.decision, 'ask', `Control: ${r.stdout}${r.stderr}`);
+  assert.ok(r.reason.includes(`review ALLOW at ${fx.featSha.slice(0, 8)}.`), r.reason);
+});