fix(review): the review keeps the findings that are right

voyage-lekerepo end-to-end trial (2026-10-06), report points 1, 5, 11: - engine-findings: recommended_action is "Fix <file:line> so this no longer happens: <failure scenario>", not "Resolve: <title>" (8 of 8 engine findings were set aside as actionability:empty); the coordinator never sets an engine finding aside for Actionability. - findings-schema --repo-root: a cited line past the end of an existing file is FINDING_LINE_OUT_OF_RANGE, so Phase 5's bounded re-ask fixes it before the coordinator sees it. The reviewers' instruction said "the actual line number from the diff"; it now says the file's own line, never the patch's. - fill-finding-ids.mjs: the coordinator (no shell) writes @@ID_<n>@@ and trekreview Phase 7 computes them over (file, line, rule_key); an unresolved placeholder or a wrong 40-hex id fails. The 4-field id with `title` the docs named is gone; the code always used three. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-08 06:48:13 +02:00 · 2026-10-08 06:48:13 +02:00 · 609eb35015
commit 609eb35015
parent c56f852ab6
10 changed files with 391 additions and 29 deletions
--- a/agents/brief-conformance-reviewer.md
+++ b/agents/brief-conformance-reviewer.md
@ -256,8 +256,8 @@ parsers find it by reading the last `json` code fence.
  is allowed for `detail` only when severity is SUGGESTION; never for
  BLOCKER/MAJOR.
 - `id` is a placeholder — emit a 40-char lowercase hex string (any
-  unique value works; the coordinator/finding-id parser will recompute
-  the canonical SHA1 from `(file, line, rule_key, title)`).
+  unique value works; `/trekreview` computes the canonical SHA1 over
+  `(file, line, rule_key)` with `lib/review/fill-finding-ids.mjs`).
 - `line` is an integer; use `0` when the finding is file-scoped without
  a specific line (e.g., MISSING_TEST for an entire file).
 - `rule_key` MUST be in the catalogue. Reviewers that emit unknown rule
@ -269,7 +269,7 @@ parsers find it by reading the last `json` code fence.
  `brief_ref`, except SCOPE_CREEP_BUILT (which traces to "no anchor").
 - **Cite, don't speculate.** Every finding includes a `file:line`
  citation taken from the diff. No "this might break" without quoted
-  evidence.
+  evidence. `line` is the line number in the file itself as it stands after the change (count from the hunk header's `+start`, or Read the file) — never the line number in the diff or patch file. A line past the end of the file fails the schema check and you are asked again.
 - **A check you did not get is not a check that passed.** Quote the exit
  code from the supplied results when you call a criterion verified or
  broken. If the results block is absent or says no criterion was checked,
--- a/agents/code-correctness-reviewer.md
+++ b/agents/code-correctness-reviewer.md
@ -245,8 +245,8 @@ parsers find it by reading the last `json` code fence.
 - `id` is a placeholder — emit a 40-char lowercase hex string (any
  unique value works; the coordinator/finding-id parser will recompute
  the canonical SHA1).
- `line` is an integer ≥ 0; use the actual line number from the diff,
-  or `0` for file-scoped findings.
+- `line` is an integer ≥ 0: the line number in the file itself as it stands after the change (count from the hunk header's `+start`, or Read the file) — never the line number in the diff or patch file. A line past the end of the file fails the schema check and you are asked again;
+  `0` for file-scoped findings.
 - `rule_key` MUST be in the catalogue. Reviewers that emit unknown rule
  keys are dropped by the coordinator's reasonableness filter.

--- a/agents/review-coordinator.md
+++ b/agents/review-coordinator.md
@ -86,9 +86,8 @@ identical. When findings collide:
 - Concatenate the kept finding's `detail` with a one-line note: "Also
  flagged by {other reviewer}: {their title}." This preserves
  attribution without duplicating the row.
- Recompute the finding `id` using the canonical SHA1 algorithm
-  (`finding-id.mjs`) over `(file, line, rule_key, title)`. Do not
-  carry over the placeholder hex from the reviewer.
+- Do not carry over the reviewer's id: write the placeholder `@@ID_<n>@@`
+  (see *Recompute IDs* under Hard rules).

 Findings with `line: 0` are file-scoped. Two file-scoped findings with
 identical `(file, rule_key)` and `line == 0` collide.
@ -104,6 +103,13 @@ is load-bearing** — see *Suppression is two-valued* below:
 | Accuracy | `file` resolves under the repo root AND `line` is plausible (≥ 0; ≤ file line count when known) | Path traversal escape, negative line, or impossibly large line number | **dropped** (`accuracy:refuted`) |
 | Actionability | `recommended_action` is non-empty AND begins with an imperative verb | Empty action, "consider …" hedges, or restating the title | `unverified` (`actionability:empty`) |

+A finding that carries `engine` (`code-review` / `security-review`) came
+from Claude Code's own review through `lib/review/engine-findings.mjs`, which
+writes its action from the engine's failure scenario; it is
+never set aside for Actionability. (Before this rule the converter wrote
+`Resolve: <title>`, and 8 of 8 engine findings were set aside in the
+voyage-lekerepo trial, 2026-10-06.)
+
 Succinctness and Actionability read the finding's *packaging*; neither
 examines the claim, so neither can establish the finding is unreal. Accuracy
 does: a citation that escapes the repo root refutes the finding as a claim
@ -347,10 +353,15 @@ for the ID list.
  tiebreak uses the catalogue tier, not the reviewer's emitted value.
 - **Block-style YAML for findings list.** The frontmatter parser
  (`lib/util/frontmatter.mjs`) does not support flow-style arrays.
- **Recompute IDs.** The reviewers emit placeholder hex IDs. Recompute
-  the canonical 40-char SHA1 from `(file, line, rule_key, title)` using
-  the algorithm in `lib/parsers/finding-id.mjs`. The frontmatter
-  `findings:` list and the JSON block IDs must match.
+- **IDs are computed by code, not by you.** You have no shell, so you
+  cannot compute the SHA1. Wherever finding n's id belongs — the frontmatter
+  `findings:` list, its `### ` heading, `**Source finding:**`, a `Verify:`
+  line and its `id` in the JSON block — write the placeholder `@@ID_<n>@@`,
+  where n is its 1-based position in the JSON block's `findings` array.
+  `/trekreview` Phase 7 runs `lib/review/fill-finding-ids.mjs`, which
+  replaces each placeholder with the id computed over `(file, line,
+  rule_key)` (`lib/parsers/finding-id.mjs`) and refuses a placeholder with
+  no finding behind it.
 - **Suppressed findings are accountable.** When you remove a finding via
  Pass 2 or Pass 3, log it in `## Suppressed Findings` with the reason and
  its fate (`[dropped]` / `[unverified]`), and list every `[unverified]` one
--- a/commands/trekreview.md
+++ b/commands/trekreview.md
@ -395,13 +395,16 @@ Collect each reviewer's trailing JSON block and **validate it against the
 reviewer-output schema** rather than merely parsing it. Run:

 ```bash
-node ${CLAUDE_PLUGIN_ROOT}/lib/review/findings-schema.mjs --json <reviewer-output-file>
+node ${CLAUDE_PLUGIN_ROOT}/lib/review/findings-schema.mjs --json --repo-root "$(git rev-parse --show-toplevel)" <reviewer-output-file>
 ```

 `validateReviewerOutput` in `lib/review/findings-schema.mjs` extracts the
 last fenced `json` block, parses it, and schema-checks every finding
 (load-bearing fields: `file`, `rule_key` ∈ catalogue, `severity` ∈ enum,
-`line` integer ≥ 0). Parse failure and schema failure surface through the
+`line` integer ≥ 0). With `--repo-root`, a cited line past the end of a file
+that exists is `FINDING_LINE_OUT_OF_RANGE`: the reviewer cited the line in the
+diff, not in the file, and is re-asked like any other schema failure (the
+voyage-lekerepo trial, 2026-10-06, lost its only brief-anchored MAJOR that way). Parse failure and schema failure surface through the
 same stable error codes (`FINDINGS_NO_JSON_BLOCK`, `FINDINGS_PARSE_ERROR`,
 `FINDING_*`).

@ -544,7 +547,20 @@ Write the coordinator's output verbatim to:
 ```

 Create parent directories if they do not exist. Atomic write pattern:
-write to a temp file, then rename. The frontmatter `findings:` field
+write to a temp file, then rename.
+
+**Then compute the finding ids** — the coordinator has no shell and writes
+`@@ID_<n>@@` placeholders (`agents/review-coordinator.md`, *IDs are computed
+by code*):
+
+```bash
+node ${CLAUDE_PLUGIN_ROOT}/lib/review/fill-finding-ids.mjs {project_dir}/review.md
+```
+
+Exit 0 = every placeholder filled (or none were left); 1 = a placeholder with
+no finding behind it, or a JSON id that is not the computed one — re-ask the
+coordinator once with the errors quoted, then stop and report; 2 = could not
+run (no JSON block) — stop. Never fill an id by hand. The frontmatter `findings:` field
 must use **block-style YAML** (one ID per line, `  - ` prefix). The
 parser at `lib/util/frontmatter.mjs` does not support flow-style arrays.

--- a/docs/architecture.md
+++ b/docs/architecture.md
@ -9,7 +9,7 @@ Imported from `CLAUDE.md` via pointer.
 - `lib/util/{frontmatter,result,atomic-write,autonomy-gate}.mjs` — shared YAML-frontmatter parser + Result helpers + `atomicWriteJson(path, obj)` for tmp+rename writes + autonomy-gate state machine (v3.4.0)
 - `lib/parsers/{plan-schema,manifest-yaml,project-discovery,arg-parser,bash-normalize,jaccard,finding-id}.mjs` — pure parsers (no I/O), unit-tested. `manifest-yaml` extended in v3.4.0 with additive `skip_commit_check` + `memory_write` flags (forward-compat: unknown keys ignored)
 - `lib/review/{rule-catalogue,plan-review-dedup}.mjs` — version-pinned rule catalogue (14 keys) + Phase 9 inline dedup helpers (v3.4.0)
- `lib/review/{verdict-gate,engine-findings,recurrence}.mjs` — the review gate (K7: 0 ALLOW/WARN · 1 BLOCK · 2 never green), Claude Code `/code-review` + `/security-review` findings → reviewer payload, recurring finding classes → operator-approved project-memory lines (review_version 1.1)
+- `lib/review/{verdict-gate,engine-findings,recurrence}.mjs` — the review gate (K7: 0 ALLOW/WARN · 1 BLOCK · 2 never green), Claude Code `/code-review` + `/security-review` findings → reviewer payload, recurring finding classes → operator-approved project-memory lines (review_version 1.1); `lib/review/fill-finding-ids.mjs` computes the coordinator's `@@ID_<n>@@` placeholders over `(file, line, rule_key)`
 - `lib/stats/event-emit.mjs` — single-source stats event emitter for autonomy-gate transitions and main-merge-gate (v3.4.0)
 - `lib/validators/{brief,research,plan,progress,session-state}-validator.mjs` — schema validators with CLI shims (`node lib/validators/X.mjs --json <path>`)
 - `lib/validators/architecture-discovery.mjs` — drift-WARN external-contract discovery for `architecture/overview.md`
--- a/lib/review/coordinator-contract.mjs
+++ b/lib/review/coordinator-contract.mjs
@ -11,10 +11,10 @@
 //     uses LLM judgement here; a verb-list approximation would DIVERGE from the
 //     contract being mirrored, so only the deterministic "recommended_action is
 //     present-and-non-empty when supplied" half is kept.
-//   - The doc's 4-tuple `(file,line,rule_key,title)` id recompute — the shipped
-//     `computeFindingId` is 3-arg `(file,line,rule_key)`; this module follows
-//     the shipped code and flags the doc divergence (the 4-tuple id is not
-//     producible by the current helper).
+//   - Nothing about ids beyond the triplet: `computeFindingId(file, line,
+//     rule_key)` is the one id. The LLM coordinator writes `@@ID_<n>@@`
+//     placeholders and lib/review/fill-finding-ids.mjs computes them (the
+//     old doc named a 4-tuple with `title` the code never computed).
 //
 // What IS implemented, purely: Pass 1 (triplet dedup → highest-severity-wins
 // survivor + conformance tiebreak + detail concat + raised_by provenance),
--- a/lib/review/engine-findings.mjs
+++ b/lib/review/engine-findings.mjs
@ -97,6 +97,8 @@ export function toReviewerPayload(input, opts = {}) {
    const narrower = typeof f.rule_key === 'string' && RULE_KEYS.has(f.rule_key)
      && tier(f.rule_key) <= tier(spec.rule_key);
    const rule_key = narrower ? f.rule_key : spec.rule_key;
+    const scenario = String(f.failure_scenario ?? '').trim();
+    const loc = line > 0 ? `${file}:${line}` : file;
    const out = {
      id: computeFindingId(file, line, rule_key),
      severity: getRule(rule_key).severity,
@ -107,7 +109,12 @@ export function toReviewerPayload(input, opts = {}) {
      title: summary,
      detail: [String(f.failure_scenario ?? '').trim(), f.category ? `(engine category: ${f.category})` : '']
        .filter(Boolean).join(' ') || summary,
-      recommended_action: `Resolve: ${summary}`,
+      // An action, not the title again: `Resolve: <title>` read as "restating
+      // the title" to the coordinator's Actionability filter, and 8 of 8
+      // engine findings were set aside (voyage-lekerepo trial, 2026-10-06).
+      recommended_action: scenario
+        ? `Fix ${loc} so this no longer happens: ${scenario}`
+        : `Fix the defect at ${loc} that ${input.engine === 'security-review' ? '/security-review' : '/code-review'} reported`,
      engine: input.engine,
    };
    if (scope && !scope.has(file)) out_of_scope.push(out);
--- a/lib/review/fill-finding-ids.mjs
+++ b/lib/review/fill-finding-ids.mjs
@ -0,0 +1,101 @@
+// lib/review/fill-finding-ids.mjs
+// Finding ids are computed by code, never by the coordinator.
+//
+// The review-coordinator agent has Read/Glob/Grep and no shell, so it cannot
+// compute the SHA1 a finding id is. In the voyage-lekerepo trial (2026-10-06)
+// it wrote `@@ID_1@@` placeholders and the orchestrator filled them by hand, a
+// step no document named. Now the contract says it: the coordinator writes
+// `@@ID_<n>@@` wherever finding n's id belongs (n = 1-based position in the
+// trailing JSON block's `findings` array), and this tool replaces every
+// placeholder with computeFindingId(file, line, rule_key) of that finding.
+//
+// Fail-closed: a placeholder with no JSON finding behind it, or a 40-hex id
+// in the JSON block that is not the computed one, is an error -- never a guess.
+// A review that is already correct passes unchanged (idempotent).
+//
+// CLI: node lib/review/fill-finding-ids.mjs <review.md>
+//   exit 0 = filled (file rewritten in place) or already correct;
+//   1 = unresolved placeholder / wrong id (errors on stderr, file untouched);
+//   2 = could not run (unreadable file, no JSON block).
+
+import { readFileSync, writeFileSync, renameSync, realpathSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
+import { computeFindingId } from '../parsers/finding-id.mjs';
+import { extractFindingsBlock } from './findings-schema.mjs';
+
+const PLACEHOLDER = /@@ID_(\d+)@@/g;
+const HEX40 = /^[0-9a-f]{40}$/;
+
+/**
+ * @param {string} text - review.md content
+ * @returns {{ ok: boolean, text: string, errors: string[], fatal?: boolean }}
+ */
+export function fillFindingIds(text) {
+  const block = extractFindingsBlock(text);
+  if (block === null) return { ok: false, fatal: true, text, errors: ['no trailing ```json block: nothing to compute ids from'] };
+  let parsed;
+  try {
+    parsed = JSON.parse(block);
+  } catch (e) {
+    return { ok: false, fatal: true, text, errors: [`the JSON block did not parse: ${e.message}`] };
+  }
+  const findings = Array.isArray(parsed?.findings) ? parsed.findings : [];
+  const errors = [];
+  const ids = findings.map((f, i) => {
+    try {
+      return computeFindingId(f.file, f.line, f.rule_key);
+    } catch (e) {
+      errors.push(`findings[${i}]: cannot compute an id (${e.message})`);
+      return null;
+    }
+  });
+  findings.forEach((f, i) => {
+    if (typeof f.id === 'string' && HEX40.test(f.id) && ids[i] && f.id !== ids[i]) {
+      errors.push(`findings[${i}].id ${f.id} does not match the id computed over (file, line, rule_key): ${ids[i]}`);
+    }
+  });
+  const out = text.replace(PLACEHOLDER, (m, n) => {
+    const id = ids[Number(n) - 1];
+    if (!id) {
+      errors.push(`${m} has no JSON finding ${n} behind it (${findings.length} findings)`);
+      return m;
+    }
+    return id;
+  });
+  if (errors.length > 0) return { ok: false, text, errors };
+  return { ok: true, text: out, errors: [] };
+}
+
+function isMain() {
+  try {
+    return realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url));
+  } catch {
+    return false;
+  }
+}
+
+if (isMain()) {
+  const path = process.argv[2];
+  if (!path) {
+    process.stderr.write('Usage: fill-finding-ids.mjs <review.md>\n');
+    process.exit(2);
+  }
+  let text;
+  try {
+    text = readFileSync(path, 'utf-8');
+  } catch (e) {
+    process.stderr.write(`fill-finding-ids: cannot read ${path}: ${e.message}\n`);
+    process.exit(2);
+  }
+  const r = fillFindingIds(text);
+  if (!r.ok) {
+    for (const e of r.errors) process.stderr.write(`fill-finding-ids: ${e}\n`);
+    process.exit(r.fatal ? 2 : 1);
+  }
+  if (r.text !== text) {
+    const tmp = `${path}.tmp-${process.pid}`;
+    writeFileSync(tmp, r.text);
+    renameSync(tmp, path);
+  }
+  process.stdout.write(`fill-finding-ids: ok ${path}\n`);
+}
--- a/lib/review/findings-schema.mjs
+++ b/lib/review/findings-schema.mjs
@ -21,8 +21,9 @@
 //
 // 3-layer pattern (Content → Raw-text → CLI shim) mirroring the other validators.

-import { readFileSync, existsSync, realpathSync } from 'node:fs';
+import { readFileSync, existsSync, realpathSync, statSync } from 'node:fs';
 import { fileURLToPath } from 'node:url';
+import { resolve, sep } from 'node:path';
 import { issue, fail } from '../util/result.mjs';
 import { RULE_KEYS, SEVERITY_VALUES } from './rule-catalogue.mjs';

@ -102,7 +103,42 @@ function validateFinding(finding, index, errors) {
 * @param {unknown} payload
 * @returns {import('../util/result.mjs').Result}
 */
-export function validateFindings(payload) {
+// A reviewer that reads the diff cites the line in the PATCH, not in the file:
+// the voyage-lekerepo trial (2026-10-06) cited tests/cli.test.mjs:194 for a
+// 124-line file, the coordinator then set the only brief-anchored MAJOR aside,
+// and the claim itself was right. With a repo root, a line past the end of a
+// file that exists is an error, so Phase 5's bounded re-ask fixes it before the
+// coordinator ever sees it. A file absent from the tree is not judged here.
+function lineCount(repoRoot, file) {
+  const root = resolve(repoRoot);
+  const abs = resolve(root, file);
+  if (abs !== root && !abs.startsWith(root + sep)) return null;
+  try {
+    if (!statSync(abs).isFile()) return null;
+    const text = readFileSync(abs, 'utf-8');
+    if (text === '') return 0;
+    return text.endsWith('\n') ? text.split('\n').length - 1 : text.split('\n').length;
+  } catch {
+    return null;
+  }
+}
+
+function checkLineRange(finding, index, repoRoot, errors) {
+  if (!finding || typeof finding.file !== 'string' || !Number.isInteger(finding.line) || finding.line <= 0) return;
+  const n = lineCount(repoRoot, finding.file);
+  if (n === null || finding.line <= n) return;
+  errors.push(issue(
+    'FINDING_LINE_OUT_OF_RANGE',
+    `findings[${index}] cites ${finding.file}:${finding.line}, but the file has ${n} lines`,
+    'Cite the line number in the file itself (Read it), not the line in the diff or patch.',
+  ));
+}
+
+/**
+ * @param {unknown} payload
+ * @param {{ repoRoot?: string }} [opts] - with a repo root, cited lines are checked against the files
+ */
+export function validateFindings(payload, opts = {}) {
  if (payload === null || typeof payload !== 'object' || Array.isArray(payload)) {
    return fail(issue('FINDINGS_NOT_OBJECT', `Reviewer output must be a JSON object, got ${Array.isArray(payload) ? 'array' : typeof payload}`));
  }
@ -121,6 +157,7 @@ export function validateFindings(payload) {

  for (let i = 0; i < payload.findings.length; i++) {
    validateFinding(payload.findings[i], i, errors);
+    if (opts.repoRoot) checkLineRange(payload.findings[i], i, opts.repoRoot, errors);
  }

  return { valid: errors.length === 0, errors, warnings, parsed: payload };
@ -133,7 +170,7 @@ export function validateFindings(payload) {
 * @param {string} rawText
 * @returns {import('../util/result.mjs').Result}
 */
-export function validateReviewerOutput(rawText) {
+export function validateReviewerOutput(rawText, opts = {}) {
  const block = extractFindingsBlock(rawText);
  if (block === null) {
    return fail(issue(
@ -148,7 +185,7 @@ export function validateReviewerOutput(rawText) {
  } catch (e) {
    return fail(issue('FINDINGS_PARSE_ERROR', `Reviewer JSON block did not parse: ${e.message}`));
  }
-  return validateFindings(parsed);
+  return validateFindings(parsed, opts);
 }

 // ---- CLI shim ----------------------------------------------------------------
@ -163,16 +200,20 @@ function isMain() {

 if (isMain()) {
  const args = process.argv.slice(2);
-  const filePath = args.find((a) => !a.startsWith('--'));
-  if (!filePath) {
-    process.stderr.write('Usage: findings-schema.mjs [--json] <reviewer-output.txt|.md>\n');
+  const rootIdx = args.indexOf('--repo-root');
+  const repoRoot = rootIdx >= 0 ? args[rootIdx + 1] : undefined;
+  // Without the flag nothing is skipped (rootIdx + 1 would be 0: the file).
+  const skipIdx = rootIdx >= 0 ? rootIdx + 1 : -1;
+  const filePath = args.find((a, i) => !a.startsWith('--') && i !== skipIdx);
+  if (!filePath || (rootIdx >= 0 && !repoRoot)) {
+    process.stderr.write('Usage: findings-schema.mjs [--json] [--repo-root <dir>] <reviewer-output.txt|.md>\n');
    process.exit(2);
  }
  if (!existsSync(filePath)) {
    process.stderr.write(`findings-schema: file not found: ${filePath}\n`);
    process.exit(2);
  }
-  const r = validateReviewerOutput(readFileSync(filePath, 'utf-8'));
+  const r = validateReviewerOutput(readFileSync(filePath, 'utf-8'), { repoRoot });
  if (args.includes('--json')) {
    process.stdout.write(JSON.stringify({ valid: r.valid, errors: r.errors, warnings: r.warnings }, null, 2) + '\n');
  } else {
--- a/tests/lib/review-findings-kept.test.mjs
+++ b/tests/lib/review-findings-kept.test.mjs
@ -0,0 +1,186 @@
+// tests/lib/review-findings-kept.test.mjs
+// The review keeps the findings that are right (voyage-lekerepo end-to-end
+// trial, 2026-10-06, report points 1, 5 and 11):
+//   1. every /code-review finding was set aside as `actionability:empty`,
+//      because the converter wrote `Resolve: <title>` — the title again;
+//   5. the only brief-anchored MAJOR was lost: the reviewer cited the line in
+//      the diff patch (194) for a 124-line file, and the schema let it through;
+//  11. the coordinator has no shell, so it cannot compute finding ids; the
+//      orchestrator filled `@@ID_1@@` placeholders by hand, undocumented, and
+//      the coordinator's doc named a 4-field id the code never computes.
+
+import { test } from 'node:test';
+import { strict as assert } from 'node:assert';
+import { mkdtempSync, writeFileSync, readFileSync, rmSync, mkdirSync } from 'node:fs';
+import { join, dirname } from 'node:path';
+import { tmpdir } from 'node:os';
+import { fileURLToPath } from 'node:url';
+import { spawnSync } from 'node:child_process';
+import { toReviewerPayload } from '../../lib/review/engine-findings.mjs';
+import { validateFindings, validateReviewerOutput } from '../../lib/review/findings-schema.mjs';
+import { computeFindingId } from '../../lib/parsers/finding-id.mjs';
+import { fillFindingIds } from '../../lib/review/fill-finding-ids.mjs';
+
+const HERE = dirname(fileURLToPath(import.meta.url));
+const ROOT = join(HERE, '..', '..');
+const read = (p) => readFileSync(join(ROOT, p), 'utf8');
+
+// --- 1. an engine finding carries an action, not its title again -------------
+
+test('engine finding: recommended_action is an imperative built from the failure scenario, never the title again', () => {
+  const r = toReviewerPayload({
+    engine: 'code-review',
+    findings: [{ file: 'bin/slug.mjs', line: 12, summary: 'trailing --max is read as text', failure_scenario: '`slug "x" --max` prints `x---max` instead of exiting 2' }],
+  });
+  assert.equal(r.ok, true, r.errors?.join('\n'));
+  const f = r.payload.findings[0];
+  assert.doesNotMatch(f.recommended_action, /^Resolve:/);
+  assert.notEqual(f.recommended_action.replace(/^\w+\s*/, ''), f.title);
+  assert.match(f.recommended_action, /^Fix /);
+  assert.match(f.recommended_action, /bin\/slug\.mjs:12/);
+  assert.match(f.recommended_action, /prints `x---max`/);
+});
+
+test('engine finding without a failure scenario still gets an action that is not the title', () => {
+  const r = toReviewerPayload({ engine: 'security-review', findings: [{ file: 'a.mjs', line: 0, summary: 'token logged' }] });
+  const f = r.payload.findings[0];
+  assert.match(f.recommended_action, /^Fix /);
+  assert.notEqual(f.recommended_action, `Resolve: ${f.title}`);
+});
+
+test('coordinator: Actionability does not set an engine finding aside', () => {
+  const doc = read('agents/review-coordinator.md');
+  const row = doc.split('\n').find((l) => l.startsWith('| Actionability |'));
+  assert.ok(row, 'Actionability row');
+  assert.match(doc, /carries `engine`[^\n]*(?:\n[^\n]*){0,4}never set aside for Actionability/);
+});
+
+// --- 5. a line number is the file's, not the patch's --------------------------
+
+function repo() {
+  const dir = mkdtempSync(join(tmpdir(), 'voyage-lines-'));
+  mkdirSync(join(dir, 'tests'));
+  writeFileSync(join(dir, 'tests', 'cli.test.mjs'), Array.from({ length: 124 }, (_, i) => `// ${i + 1}`).join('\n') + '\n');
+  return dir;
+}
+
+const finding = (line, file = 'tests/cli.test.mjs') => ({
+  reviewer: 'brief-conformance-reviewer',
+  findings: [{ id: 'x', severity: 'MAJOR', rule_key: 'MISSING_TEST', file, line, title: 't', detail: 'd', recommended_action: 'Add a test' }],
+});
+
+test('schema with a repo root: a line past the end of the file is an error the reviewer is re-asked about', () => {
+  const dir = repo();
+  try {
+    const r = validateFindings(finding(194), { repoRoot: dir });
+    assert.equal(r.valid, false);
+    const e = r.errors.find((x) => x.code === 'FINDING_LINE_OUT_OF_RANGE');
+    assert.ok(e, JSON.stringify(r.errors));
+    assert.match(e.message, /194/);
+    assert.match(e.message, /124/);
+    assert.match(e.hint, /not the line in the diff/);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('schema with a repo root: the last line, line 0 and a file absent from the tree are not range errors', () => {
+  const dir = repo();
+  try {
+    assert.equal(validateFindings(finding(124), { repoRoot: dir }).valid, true);
+    assert.equal(validateFindings(finding(0), { repoRoot: dir }).valid, true);
+    assert.equal(validateFindings(finding(500, 'src/deleted.mjs'), { repoRoot: dir }).valid, true);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('schema without a repo root behaves as before', () => {
+  assert.equal(validateFindings(finding(194)).valid, true);
+});
+
+test('schema CLI: --repo-root reaches the range check', () => {
+  const dir = repo();
+  try {
+    const out = join(dir, 'reviewer.md');
+    writeFileSync(out, '```json\n' + JSON.stringify(finding(194)) + '\n```\n');
+    const r = spawnSync(process.execPath, [join(ROOT, 'lib/review/findings-schema.mjs'), '--json', '--repo-root', dir, out], { encoding: 'utf8' });
+    assert.equal(r.status, 1, r.stdout + r.stderr);
+    assert.match(r.stdout, /FINDING_LINE_OUT_OF_RANGE/);
+    assert.equal(validateReviewerOutput(readFileSync(out, 'utf8')).valid, true);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('reviewers are told to cite the file\'s own line, never the patch\'s', () => {
+  for (const a of ['agents/brief-conformance-reviewer.md', 'agents/code-correctness-reviewer.md']) {
+    assert.match(read(a), /never the line number in the diff/, a);
+  }
+  assert.match(read('commands/trekreview.md'), /findings-schema\.mjs --json --repo-root/);
+});
+
+// --- 11. ids are computed by code, not by the coordinator ---------------------
+
+function review(findings, extra = '') {
+  const ids = findings.map((_, i) => `  - @@ID_${i + 1}@@`).join('\n');
+  const sections = findings.map((f, i) => `### @@ID_${i + 1}@@\n- file: ${f.file}\n- line: ${f.line}\n- rule_key: ${f.rule_key}\n`).join('\n');
+  const json = { verdict: 'WARN', findings: findings.map((f, i) => ({ id: `@@ID_${i + 1}@@`, ...f })) };
+  return `---\ntype: trekreview\nverdict: WARN\nfindings:\n${ids}\n---\n\n## Findings (MAJOR)\n\n${sections}\n${extra}\n\`\`\`json\n${JSON.stringify(json, null, 2)}\n\`\`\`\n`;
+}
+
+const F1 = { file: 'tests/cli.test.mjs', line: 40, rule_key: 'MISSING_TEST', severity: 'MAJOR' };
+const F2 = { file: 'bin/slug.mjs', line: 12, rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR' };
+
+test('fill-finding-ids: every @@ID_n@@ becomes the id computed over (file, line, rule_key) of JSON finding n', () => {
+  const r = fillFindingIds(review([F1, F2]));
+  assert.equal(r.ok, true, r.errors.join('\n'));
+  const id1 = computeFindingId(F1.file, F1.line, F1.rule_key);
+  const id2 = computeFindingId(F2.file, F2.line, F2.rule_key);
+  assert.doesNotMatch(r.text, /@@ID_/);
+  assert.match(r.text, new RegExp(`findings:\\n  - ${id1}\\n  - ${id2}\\n`));
+  assert.match(r.text, new RegExp(`### ${id2}`));
+  assert.match(r.text, new RegExp(`"id": "${id1}"`));
+});
+
+test('fill-finding-ids: a placeholder with no JSON finding behind it is an error, not a guess', () => {
+  const r = fillFindingIds(review([F1], 'See also @@ID_7@@.'));
+  assert.equal(r.ok, false);
+  assert.match(r.errors.join('\n'), /@@ID_7@@/);
+});
+
+test('fill-finding-ids: a 40-hex id that is not the computed one is an error', () => {
+  const text = review([F1]).replaceAll('@@ID_1@@', 'a'.repeat(40));
+  const r = fillFindingIds(text);
+  assert.equal(r.ok, false);
+  assert.match(r.errors.join('\n'), /does not match/);
+});
+
+test('fill-finding-ids: an already-correct review passes unchanged', () => {
+  const once = fillFindingIds(review([F1, F2]));
+  const twice = fillFindingIds(once.text);
+  assert.equal(twice.ok, true, twice.errors.join('\n'));
+  assert.equal(twice.text, once.text);
+});
+
+test('fill-finding-ids CLI rewrites the file in place and exits 0; 1 on an unresolved placeholder', () => {
+  const dir = mkdtempSync(join(tmpdir(), 'voyage-ids-'));
+  try {
+    const p = join(dir, 'review.md');
+    writeFileSync(p, review([F1]));
+    const cli = join(ROOT, 'lib/review/fill-finding-ids.mjs');
+    assert.equal(spawnSync(process.execPath, [cli, p]).status, 0);
+    assert.doesNotMatch(readFileSync(p, 'utf8'), /@@ID_/);
+    writeFileSync(p, review([F1], '@@ID_3@@'));
+    assert.equal(spawnSync(process.execPath, [cli, p]).status, 1);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('the coordinator writes @@ID_n@@ placeholders and the doc names the three-field id the code computes', () => {
+  const doc = read('agents/review-coordinator.md');
+  assert.doesNotMatch(doc, /\(file, line, rule_key, title\)/);
+  assert.match(doc, /@@ID_<n>@@/);
+  assert.match(read('commands/trekreview.md'), /fill-finding-ids\.mjs/);
+});