fix: close the gaps the review of the trial-5 fixes found

- park-attempt-files: parks in the git dir (outside the work tree, so Sub-step C.2 never registers a parked file again), reads a path with a space, names a new file under a header it cannot read, parks a symlink, never overwrites a parked file, and exits 1 while a file the attempt made is still in the tree; the upward diff is saved in one form whatever the git config.

- brief-validator: BRIEF_SC_NOT_RUNNABLE only when a later backticked span is a command; a criterion that only names a flag or a path stays prose (Handover 1 says so now).

- review ids: counted over the final list (runContract), as fill-finding-ids counts review.md; trimmed_count matches rows by id; engine-findings and fidelity-diff use computeFindingIds; the --workflow dedup keeps one source's different findings apart, the decision-23 put-back stays one copy per triplet.

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

commands/trekbrief.md

@@ -820,7 +820,8 @@ If the validator returns errors, report them to the user and offer to
 re-enter Phase 4 with the validator's hints in scope. If only warnings,
 note them in the final report — except `BRIEF_SC_NOT_RUNNABLE`: a success
 criterion whose command `/trekreview` will never run (the product's own
-binary, `jq`, `grep`, a pipe). Name each one to the user in plain words and
+binary, `jq`, `grep`, a pipe, or a command that stands after a flag or path
+in backticks). Name each one to the user in plain words and
 offer to rewrite it as a script under `tests/` that the criterion runs
 (`bash tests/<script>.sh` exits 0) before the brief is approved; if the user
 keeps it, it stays a criterion a human judges, and the report says so. The

commands/trekexecute.md

@@ -1527,13 +1527,16 @@ the earlier attempt's new files, which the revert left on disk untracked, so a
 stale file can never pass the revised step's checks (voyage-lekerepo trial 5,
 finding 4):
 ```bash
-node ${CLAUDE_PLUGIN_ROOT}/lib/util/park-attempt-files.mjs --diff {report dir}/step-{N}-upward.diff --to {report dir}/step-{N}-attempt-{attempts}
+node ${CLAUDE_PLUGIN_ROOT}/lib/util/park-attempt-files.mjs --diff {report dir}/step-{N}-upward.diff --step {N} --attempt {attempts}
 ```
-It moves every file the attempt created that is still untracked into that
-directory under its own path: moved, never deleted. Name the `parked` paths
-and every `kept` path with its reason in the step report; exit 2 is a stop
-(the tree was not checked). Then launch a fresh `build` helper for the
-revised step.
+It moves every file the attempt created that is still untracked to the git
+dir (`voyage/parked/step-{N}-attempt-{attempts}/`, outside the work tree, so
+Sub-step C.2 never registers it again): moved, never deleted, never over a file
+already parked. Name the `parked` paths and every `kept` path with its reason
+in the step report. Exit 1 means a file the attempt made is still in the tree
+(a header it could not read, a move that failed): stop and name it; exit 2 is
+a stop too (the tree was not checked). Then launch a fresh `build` helper for
+the revised step.
 
 - **Never pass the Agent tool's `name` parameter.** A named spawn becomes an
   `in_process_teammate`: it does the work, but its result never reaches this
@@ -1849,7 +1852,7 @@ session's effort, Sub-step E).
 | 4 | the plan | **After three failed fix rounds the question is the architecture, not a fourth fix.** The step goes back to planning (below). Sub-step B sends three answers here directly, with no fix round: a `"not-run"` caused by the `Verify:` command, a second `plan-unreadable` (the executor's own call still wrong after one correction), and a `not-red-at-base` with `tests_only: true` (a step of only tests). Stop class 4 |
 | 5 | the operator | A step already sent to the plan (`fix_rounds` 4) that fails its control again after re-planning goes to the operator. The executor never builds this step again: on a later `--resume` it only runs the checks (B, D1, D2, D3, D4, E) on whatever the operator changed. If they fail again the step stays at 5 — there is no sixth round, `fix_rounds` never exceeds 5 — and the run stops again. Stop class 4 |
 
-**Back to planning** (`fix_rounds = 4`): save `git diff {start_sha}` to
+**Back to planning** (`fix_rounds = 4`): save `git diff --no-ext-diff --no-renames --src-prefix=a/ --dst-prefix=b/ {start_sha}` (one form whatever the user's git config) to
 `{report dir}/step-{N}-upward.diff` and write `{report dir}/step-{N}-upward.md`:
 the step verbatim, every round's findings and verdicts, and the question for
 the planner — which assumption in the step (its Changes, its Interface, its
@@ -1873,7 +1876,7 @@ directly (Sub-step B), and so does a second `plan-unreadable` after one correcte
   ```
   The new files stay on disk, untracked, and `step-{N}-upward.md` names them —
   nothing the step made is deleted, and none of it is left registered. A
-  `--resume` after re-planning parks them in the report dir before the fresh
+  `--resume` after re-planning parks them in the git dir before the fresh
   build (Sub-step C).
   When the guard refuses the restore (it is destructive git: in the permission
   mode that shows no prompts the guard refuses it, exit 2, "no one to ask"), or

docs/HANDOVER-CONTRACTS.md

@@ -100,7 +100,7 @@ Optional but standard sections: `## Non-Goals`, `## Constraints`, `## Preference
 | **v5.1 sequencing gate** | every read | `brief_version ≥ 2.1` requires `phase_signals` (list) OR `phase_signals_partial: true` — error `BRIEF_V51_MISSING_SIGNALS` on miss. Validator-only enforcement; commands surface, don't re-enforce. |
 | **v5.5 framing gate** | every read | `framing` enum-checked on any version when present (`BRIEF_INVALID_FRAMING`); at `brief_version ≥ 2.2` a missing `framing` → `BRIEF_MISSING_FRAMING` and a missing `## TL;DR` → `BRIEF_MISSING_SECTION` (strict) / warning (soft). `trekreview` briefs are exempt. |
 | Body sections | strict only | All `BRIEF_BODY_SECTIONS` present (`## TL;DR` added at ≥ 2.2) |
-| Runnable criteria | every read | A `## Success Criteria` bullet whose first backticked span is a command the review's criteria runner never runs (the product's own binary, `jq`, `grep`, a pipe) → warning `BRIEF_SC_NOT_RUNNABLE`, never an error: a criterion a human judges stays allowed. Prose-only criteria are not flagged. `trekreview` briefs are exempt. No schema change. Indented sub-bullets belong to the criterion above them (counted, never criteria of their own); a criterion with sub-points and no command → warning `BRIEF_SC_SUBPOINTS`. A brief whose Constraints or NFRs require bash 3.2 and a criterion that runs plain `bash` → warning `BRIEF_SC_BASH_NOT_PINNED` (use `/bin/bash`). |
+| Runnable criteria | every read | A `## Success Criteria` bullet whose first backticked span is a command the review's criteria runner never runs (the product's own binary, `jq`, `grep`, a pipe), or whose first span is not a command while a later span is one (the runner reads only the first, voyage-lekerepo trial 5) → warning `BRIEF_SC_NOT_RUNNABLE`, never an error: a criterion a human judges stays allowed. Prose-only criteria, also those that only name a flag or a path, are not flagged. `trekreview` briefs are exempt. No schema change. Indented sub-bullets belong to the criterion above them (counted, never criteria of their own); a criterion with sub-points and no command → warning `BRIEF_SC_SUBPOINTS`. A brief whose Constraints or NFRs require bash 3.2 and a criterion that runs plain `bash` → warning `BRIEF_SC_BASH_NOT_PINNED` (use `/bin/bash`). |
 
 **State machine** detail: a brief that says it has research topics but skipped them must explicitly admit it (via `brief_quality: partial`). This is the most common failure mode the validator catches.
 

lib/review/coordinator-contract.mjs

@@ -331,8 +331,8 @@ export function dedupByTriplet(findings) {
       ...(raised ? { pass1_raised_from: survivor.severity } : {}),
     });
   }
-  // The groups of one triplet sit next to each other, so the k-th row on a
-  // triplet here is the k-th in review.md, where fill-finding-ids counts the same way.
+  // One id per row here; runContract counts the written ids again over the
+  // final list (after Pass 2 and 3), the way fill-finding-ids counts review.md.
   const ids = computeFindingIds(out);
   return out.map((f, i) => ({ ...f, id: ids[i] }));
 }
@@ -600,8 +600,13 @@ export function runContract(reviewerPayloads, options = {}) {
     reason: f.suppressed_reason,
   }));
   // A trimmed finding that Pass 3 later removed is accounted for there, not here.
-  const keptSet = new Set(reasoned.kept.map((f) => tripletKey(f)));
-  const trimmed_count = judged.trimmed.filter((f) => keptSet.has(tripletKey(f))).length;
+  // Matched on Pass 1's id, which is one per row (a triplet can hold two rows).
+  const keptSet = new Set(reasoned.kept.map((f) => f.id));
+  const trimmed_count = judged.trimmed.filter((f) => keptSet.has(f.id)).length;
+  // The ids written are counted over the final list, as fill-finding-ids counts
+  // review.md's JSON: a row whose triplet sibling was set aside gets the plain id.
+  const finalIds = computeFindingIds(reasoned.kept);
+  const kept = reasoned.kept.map((f, i) => ({ ...f, id: finalIds[i] }));
 
   // A reviewer counts as REPORTED only when a payload carrying its name
   // validated. `validateFindings` merely warns on a missing `reviewer`, so a
@@ -633,7 +638,7 @@ export function runContract(reviewerPayloads, options = {}) {
   return {
     verdict,
     counts,
-    findings: reasoned.kept,
+    findings: kept,
     suppressed: [...judgedDropped, ...judgedUnverified, ...reasoned.dropped, ...reasoned.unverified],
     unverified,
     skipped,

lib/review/engine-findings.mjs

@@ -44,7 +44,7 @@
 //   exit 0 = payload on stdout; 1 = input invalid (errors on stderr); 2 = could not run.
 
 import { readFileSync, existsSync } from 'node:fs';
-import { computeFindingId } from '../parsers/finding-id.mjs';
+import { computeFindingIds } from '../parsers/finding-id.mjs';
 import { RULE_KEYS, getRule, SEVERITY_VALUES } from './rule-catalogue.mjs';
 import { validateFindings } from './findings-schema.mjs';
 import { isMain } from '../util/is-main.mjs';
@@ -117,7 +117,7 @@ export function toReviewerPayload(input, opts = {}) {
     const fix = String(f.fix ?? '').trim();
     const loc = line > 0 ? `${file}:${line}` : file;
     const out = {
-      id: computeFindingId(file, line, rule_key),
+      id: null,
       severity: getRule(rule_key).severity,
       rule_key,
       file,
@@ -140,6 +140,10 @@ export function toReviewerPayload(input, opts = {}) {
   });
   if (errors.length > 0) return { ok: false, errors };
 
+  // Two defects the engine put on one line under one key keep an id each, the
+  // second over `rule_key#2` (computeFindingIds; voyage-lekerepo trial 5, finding 7).
+  const ids = computeFindingIds([...findings, ...out_of_scope]);
+  [...findings, ...out_of_scope].forEach((f, i) => { f.id = ids[i]; });
   const payload = { reviewer: spec.reviewer, findings };
   const v = validateFindings(payload);
   if (!v.valid) return { ok: false, errors: v.errors.map((e) => `${e.code}: ${e.message}`) };

lib/review/fidelity-diff.mjs

@@ -14,7 +14,7 @@
 
 import { parseDocument } from '../util/frontmatter.mjs';
 import { jaccardSimilarity } from '../parsers/jaccard.mjs';
-import { computeFindingId } from '../parsers/finding-id.mjs';
+import { computeFindingIds } from '../parsers/finding-id.mjs';
 import { extractFindingsBlock } from './findings-schema.mjs';
 import { isMain } from '../util/is-main.mjs';
 
@@ -126,14 +126,13 @@ export function normalizeArmOutput(arm) {
   const findings = (arm && Array.isArray(arm.findings)) ? arm.findings : [];
   const findingIds = [];
   const details = [];
-  for (const f of findings) {
+  // The k-th row on one triplet gets the id over `rule_key#k`, as review.md does.
+  const ids = computeFindingIds(findings.map((f) => ({ file: f.file, line: f.line, rule_key: f.rule_key ?? f.rule ?? null })), () => {});
+  for (const [i, f] of findings.entries()) {
     const file = f.file;
     const rule_key = f.rule_key ?? f.rule ?? null;
     const line = f.line;
-    let id = null;
-    if (typeof file === 'string' && file.length > 0 && rule_key && line !== null && line !== undefined) {
-      try { id = computeFindingId(file, line, rule_key); } catch { id = null; }
-    }
+    const id = ids[i];
     if (id) findingIds.push(id);
     details.push({ id, severity: f.severity ?? null, rule_key, file: file ?? null, line: line ?? null });
   }

lib/util/park-attempt-files.mjs

@@ -9,81 +9,101 @@
 //
 // Before the fresh build the executor parks them: every file the attempt
 // created (a `new file mode` entry in step-{N}-upward.diff) that is still
-// untracked on disk moves into the report dir under its own path. Moved,
-// never deleted. A file tracked since then, already gone, or outside the
-// repository is kept where it is and named.
+// untracked on disk moves to <git dir>/voyage/parked/step-<N>-attempt-<k>/
+// under its own path. The git dir, like the test lock's, is outside the work
+// tree, so a parked file never comes back as a new file of the next build.
+// Moved, never deleted, and never over a file already parked there.
 //
 // Usage:
-//   node lib/util/park-attempt-files.mjs --diff <step-N-upward.diff> --to <dir> [--cwd <repo>]
-// Prints {"parked": [paths], "kept": [{"path", "reason"}]} on stdout.
-// Exit: 0 = done (also when nothing was parked), 2 = could not run (bad usage,
-// unreadable diff, not a repository). 2 never means the tree is clean.
+//   node lib/util/park-attempt-files.mjs --diff <step-N-upward.diff> --step <N> --attempt <k> [--cwd <repo>]
+// Prints {"parked": [paths], "kept": [{"path", "reason", "blocking"}], "to": dir}.
+// Exit: 0 = no file the attempt made is left in the tree; 1 = one still is
+// (a header it could not read, a move that failed, a target already taken):
+// the run stops; 2 = could not run (bad usage, unreadable diff, not a
+// repository). Neither 1 nor 2 is ever "the tree is clean".
 
-import { readFileSync, existsSync, mkdirSync, renameSync, lstatSync } from 'node:fs';
+import { readFileSync, mkdirSync, renameSync, lstatSync } from 'node:fs';
 import { join, dirname, resolve, relative, isAbsolute } from 'node:path';
 import { spawnSync } from 'node:child_process';
 import { isMain } from './is-main.mjs';
 
-/** The paths a unified git diff creates (`new file mode` after its `diff --git` line). */
+/**
+ * What a unified git diff creates: `paths` read from a `diff --git a/X b/X`
+ * header followed by `new file mode`, and `unread`, the raw headers of new
+ * files whose header is not that form (another prefix, a quoted path).
+ */
 export function newFilesInDiff(diffText) {
   const paths = [];
-  let current = null;
+  const unread = [];
+  let header = null;
   for (const line of String(diffText).split('\n')) {
-    const head = line.match(/^diff --git a\/(\S+) b\/(\S+)$/);
-    if (head) {
-      current = head[2];
-      continue;
-    }
     if (line.startsWith('diff --git ')) {
-      current = null; // a quoted or spaced path: not read, so never moved
+      header = line;
       continue;
     }
-    if (current !== null && /^new file mode \d+$/.test(line)) {
-      paths.push(current);
-      current = null;
+    if (header !== null && /^new file mode \d+$/.test(line)) {
+      // Both names are the same for a new file, so a space in it is read too.
+      const m = header.match(/^diff --git a\/(.+) b\/\1$/);
+      if (m) paths.push(m[1]);
+      else unread.push(header);
+      header = null;
     }
   }
-  return paths;
+  return { paths, unread };
 }
 
-function tracked(cwd, path) {
-  return spawnSync('git', ['ls-files', '--error-unmatch', '--', path], { cwd, encoding: 'utf8' }).status === 0;
+function lexists(abs) {
+  try {
+    lstatSync(abs);
+    return true;
+  } catch {
+    return false;
+  }
 }
 
 /**
- * @param {{ diffText: string, to: string, cwd?: string }} args
- * @returns {{ parked: string[], kept: Array<{path: string, reason: string}> }}
+ * @param {{ diffText: string, step: string|number, attempt: string|number, cwd?: string }} args
+ * @returns {{ parked: string[], kept: Array<{path: string, reason: string, blocking: boolean}>, to: string }}
  */
-export function parkAttemptFiles({ diffText, to, cwd = process.cwd() }) {
+export function parkAttemptFiles({ diffText, step, attempt, cwd = process.cwd() }) {
   const root = resolve(cwd);
-  const dest = resolve(root, to);
+  const gitDir = spawnSync('git', ['rev-parse', '--absolute-git-dir'], { cwd: root, encoding: 'utf8' }).stdout.trim();
+  const to = join(gitDir, 'voyage', 'parked', `step-${step}-attempt-${attempt}`);
+  const trackedPaths = new Set(
+    spawnSync('git', ['ls-files', '-z'], { cwd: root, encoding: 'utf8' }).stdout.split('\0').filter(Boolean),
+  );
+  const { paths, unread } = newFilesInDiff(diffText);
   const parked = [];
-  const kept = [];
-  for (const path of newFilesInDiff(diffText)) {
+  const kept = unread.map((h) => ({ path: h, reason: 'diff header not read: move this file out by hand', blocking: true }));
+  for (const path of paths) {
     const abs = resolve(root, path);
     const rel = relative(root, abs);
     if (isAbsolute(path) || rel === '' || rel.startsWith('..') || isAbsolute(rel) || rel.split(/[\\/]/)[0] === '.git') {
-      kept.push({ path, reason: 'outside the repository: refused' });
+      kept.push({ path, reason: 'outside the repository: refused', blocking: false });
       continue;
     }
-    if (!existsSync(abs) || !lstatSync(abs).isFile()) {
-      kept.push({ path, reason: 'not on disk as a file: nothing to park' });
+    if (!lexists(abs)) {
+      kept.push({ path, reason: 'not on disk: nothing to park', blocking: false });
       continue;
     }
-    if (tracked(root, rel)) {
-      kept.push({ path, reason: 'tracked since the attempt: not the executor\'s to move' });
+    if (trackedPaths.has(rel.split('\\').join('/'))) {
+      kept.push({ path, reason: 'tracked since the attempt: not the executor\'s to move', blocking: false });
+      continue;
+    }
+    const target = join(to, rel);
+    if (lexists(target)) {
+      kept.push({ path, reason: `already parked at ${target}: never overwritten, move it by hand`, blocking: true });
       continue;
     }
-    const target = join(dest, rel);
     try {
       mkdirSync(dirname(target), { recursive: true });
       renameSync(abs, target);
       parked.push(rel);
     } catch (e) {
-      kept.push({ path, reason: `could not move: ${e.message}` });
+      kept.push({ path, reason: `could not move: ${e.message}`, blocking: true });
     }
   }
-  return { parked, kept };
+  return { parked, kept, to };
 }
 
 if (isMain(import.meta.url)) {
@@ -93,10 +113,11 @@ if (isMain(import.meta.url)) {
     return i === -1 ? undefined : argv[i + 1];
   };
   const diff = opt('--diff');
-  const to = opt('--to');
+  const step = opt('--step');
+  const attempt = opt('--attempt');
   const cwd = opt('--cwd') ?? process.cwd();
-  if (!diff || !to) {
-    process.stderr.write('Usage: park-attempt-files.mjs --diff <step-N-upward.diff> --to <dir> [--cwd <repo>]\n');
+  if (!diff || !/^[0-9]+$/.test(step ?? '') || !/^[0-9]+$/.test(attempt ?? '')) {
+    process.stderr.write('Usage: park-attempt-files.mjs --diff <step-N-upward.diff> --step <N> --attempt <k> [--cwd <repo>]\n');
     process.exit(2);
   }
   let diffText;
@@ -110,5 +131,7 @@ if (isMain(import.meta.url)) {
     process.stderr.write(`park-attempt-files: ${cwd} is not a git repository\n`);
     process.exit(2);
   }
-  process.stdout.write(JSON.stringify(parkAttemptFiles({ diffText, to, cwd }), null, 2) + '\n');
+  const result = parkAttemptFiles({ diffText, step, attempt, cwd });
+  process.stdout.write(JSON.stringify(result, null, 2) + '\n');
+  process.exit(result.kept.some((k) => k.blocking) ? 1 : 0);
 }

lib/validators/brief-validator.mjs

@@ -10,7 +10,7 @@ import { readFileSync, existsSync } from 'node:fs';
 import { parseDocument } from '../util/frontmatter.mjs';
 import { issue, ok, fail } from '../util/result.mjs';
 import { BASE_ALLOWED_MODELS } from './profile-validator.mjs';
-import { parseSuccessCriteria, runnableCheck } from '../verification/criteria-runner.mjs';
+import { parseSuccessCriteria, runnableCheck, firstCommand } from '../verification/criteria-runner.mjs';
 import { isMain } from '../util/is-main.mjs';
 
 export const BRIEF_REQUIRED_FRONTMATTER = ['type', 'brief_version', 'task', 'slug', 'research_topics', 'research_status'];
@@ -286,17 +286,22 @@ export function validateBriefContent(text, opts = {}) {
             'or write each sub-point as its own criterion with its own command.',
         ));
       }
-      // A backticked span that is not a command (`--stdin` named before the
-      // command) is what the runner reads first and reports unrunnable; only
-      // a criterion with no backticked span at all is prose (trial 5, finding 1).
+      // A criterion whose first backticked span is not a command but whose
+      // later span is one (`--stdin` named before `/bin/bash tests/contract.sh`):
+      // the runner reads only the first span and reports it unrunnable. A
+      // criterion that only names a flag or a path stays prose and unflagged
+      // (HANDOVER-CONTRACTS Handover 1; voyage-lekerepo trial 5, finding 1).
       if (!c.command && c.reason === 'not-a-command' && !c.subpoints) {
-        const span = (c.text.match(/`([^`]+)`/) ?? [])[1] ?? '';
-        warnings.push(issue(
-          'BRIEF_SC_NOT_RUNNABLE',
-          `Success criterion ${c.label} opens with \`${span}\`, which is not a command, so the review's criteria runner will never run it (not-a-command)`,
-          'The runner runs the first backticked span: put the command first and name anything else in plain words ' +
-            '(voyage:writing-criteria), or it is reported NOT RUN at review.',
-        ));
+        const spans = [...c.text.matchAll(/`([^`]+)`/g)].map((m) => m[1]);
+        const later = spans.slice(1).find((span) => firstCommand(`\`${span}\``).command);
+        if (later !== undefined) {
+          warnings.push(issue(
+            'BRIEF_SC_NOT_RUNNABLE',
+            `Success criterion ${c.label}'s first backticked span is \`${spans[0]}\`, not a command, so the review's criteria runner never reaches \`${later}\` (not-a-command)`,
+            'The runner runs the first backticked span: put the command first and name anything else in plain words ' +
+              '(voyage:writing-criteria), or it is reported NOT RUN at review.',
+          ));
+        }
       }
       if (!c.command) continue;
       const bare = c.command.replace(/^(?:[A-Za-z_][A-Za-z0-9_]*=\S*\s+)+/, '');

scripts/trekreview-armB.workflow.mjs

@@ -126,16 +126,33 @@ const VERDICT_SCHEMA = {
 // the workflow has no conformance rule, so otherwise the first-seen one stays.
 // The survivor carries the highest severity any finding in its group asserted,
 // so an engine's mark never lowers what a reviewer agent asserted.
-function dedupByTriplet(findings) {
-  const groups = new Map();
+// As in coordinator-contract Pass 1, one source never duplicates itself: its
+// second finding on a triplet under another title opens a group of its own
+// (voyage-lekerepo trial 5, finding 7). `sourceOf` names the reviewer or engine.
+const sameTitle = (a, b) => a.title === undefined || b.title === undefined ||
+  String(a.title).trim().toLowerCase() === String(b.title).trim().toLowerCase();
+const sourceOf = new Map();
+function dedupByTriplet(findings, { perSource = true } = {}) {
+  const byTriplet = new Map();
   for (const f of findings) {
     const key = tripletOf(f);
-    if (!groups.has(key)) groups.set(key, []);
-    groups.get(key).push(f);
+    if (!byTriplet.has(key)) byTriplet.set(key, []);
+    const subs = byTriplet.get(key);
+    const src = sourceOf.get(f) ?? null;
+    const fits = (g) => g.every((m) => (sourceOf.get(m) ?? null) !== src || sameTitle(m, f));
+    let sub = !perSource ? subs[0]
+      : subs.find((g) => fits(g) && g.some((m) => m.title !== undefined && f.title !== undefined && sameTitle(m, f)))
+        ?? subs.find(fits);
+    if (!sub) {
+      sub = [];
+      subs.push(sub);
+    }
+    sub.push(f);
   }
+  const groups = [...byTriplet.values()].flat();
   const tier = (f) => sevRank(tierOf(f.rule_key) ?? f.severity);
   const out = [];
-  for (const group of groups.values()) {
+  for (const group of groups) {
     let survivor = group[0];
     for (const f of group.slice(1)) {
       const tf = tier(f);
@@ -248,9 +265,15 @@ const withEngine = (f, eng) => {
   const { engine: _dropped, ...rest } = f;
   return eng ? { ...rest, engine: eng } : rest;
 };
+const tagged = (f, src) => {
+  if (f && typeof f === 'object') sourceOf.set(f, src);
+  return f;
+};
 const allFindings = [
-  ...reviewerResults.flatMap((r) => (Array.isArray(r?.findings) ? r.findings : []).map((f) => withEngine(f, null))),
-  ...enginePayloads.flatMap((p) => (Array.isArray(p?.findings) ? p.findings : []).map((f) => withEngine(f, engineOf(p?.reviewer)))),
+  ...reviewerResults.flatMap((r, i) => (Array.isArray(r?.findings) ? r.findings : [])
+    .map((f) => tagged(withEngine(f, null), `reviewer:${r?.reviewer ?? i}`))),
+  ...enginePayloads.flatMap((p) => (Array.isArray(p?.findings) ? p.findings : [])
+    .map((f) => tagged(withEngine(f, engineOf(p?.reviewer)), `engine:${p?.reviewer}`))),
 ];
 const merged = dedupByTriplet(allFindings);
 log(`Arm B: ${reviewerResults.length} reviewer(s); ${allFindings.length} raw → ${merged.length} after triplet-dedup`);
@@ -333,9 +356,10 @@ for (const f of returned) {
 }
 // (c) the put-back (decision 23).
 const returnedTriplets = new Set(returned.filter((f) => f && typeof f === 'object').map(tripletOf));
-// Engine findings only, so dedupByTriplet keeps the higher severity, else the first seen.
+// Engine findings only, so dedupByTriplet keeps the higher severity, else the
+// first seen: one copy per triplet (decision 23), so not split per source here.
 const putBack = dedupByTriplet(allFindings.filter((f) =>
-  f && typeof f === 'object' && f.engine && !returnedTriplets.has(tripletOf(f))));
+  f && typeof f === 'object' && f.engine && !returnedTriplets.has(tripletOf(f))), { perSource: false });
 for (const f of putBack) {
   const { brief_lowered_from: _lowered, ...copy } = f;
   returned.push(copy);

tests/fixtures/jsonl-schemas.md

@@ -19,10 +19,10 @@
 
 | schema_id | fields | writer_path | line_ref | v4.1 additive | PII |
 |-----------|--------|-------------|----------|---------------|-----|
-| 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:1127-1151 | profile, phase_models, profile_source | none |
+| 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:1128-1152 | 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:782-808 | 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:1277-1305 | 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:2904-2926 | 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:2907-2929 | 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:636-686 | profile, phase_models, profile_source | none |

tests/lib/coordinator-contract.test.mjs

@@ -427,3 +427,15 @@ test('the coordinator prose says one reviewer never duplicates itself, as dedupB
   assert.match(pass1, /different titles are two\s+findings/);
   assert.match(prose, /k-th row on one triplet\s+gets it over `rule_key#k`/);
 });
+
+test('runContract — the #k id counts rows of one triplet in the final list, so a row whose sibling was set aside gets the plain id (review of b47cabd)', () => {
+  const long = 'x'.repeat(120);
+  const result = runContract([{ reviewer: 'claude-code-review', findings: [
+    { file: 'bin/slug.mjs', line: 46, rule_key: 'MISSING_ERROR_HANDLING', severity: 'MINOR', title: `EAGAIN ${long}`, detail: 'd', recommended_action: '' },
+    { file: 'bin/slug.mjs', line: 46, rule_key: 'MISSING_ERROR_HANDLING', severity: 'MINOR', title: `Lone CR ${long}`, detail: 'd', recommended_action: 'Split on CR too.' },
+  ] }]);
+  assert.equal(result.findings.length, 1, JSON.stringify(result.findings.map((f) => f.title)));
+  assert.match(result.findings[0].title, /^Lone CR/);
+  assert.equal(result.findings[0].id, computeFindingId('bin/slug.mjs', 46, 'MISSING_ERROR_HANDLING'), 'what fill-finding-ids computes for a review.md holding this row alone');
+  assert.equal(result.trimmed_count, 1, 'the trimmed row that Pass 2 set aside is not counted');
+});

tests/lib/fidelity-diff.test.mjs

@@ -164,3 +164,12 @@ test('fidelityDiffStructured — same triplet, different severity → severity m
   assert.equal(d.severityMismatches.length, 1);
   assert.equal(d.equivalent, false);
 });
+
+test('normalizeArmOutput — two rows on one triplet keep two ids, the second over #2, as review.md writes them (review of b47cabd)', () => {
+  const row = { file: 'bin/slug.mjs', line: 46, rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR' };
+  const n = normalizeArmOutput({ verdict: 'WARN', findings: [row, { ...row }] });
+  assert.deepEqual(n.findingIds, [
+    computeFindingId('bin/slug.mjs', 46, 'CODE_REVIEW_DEFECT'),
+    computeFindingId('bin/slug.mjs', 46, 'CODE_REVIEW_DEFECT#2'),
+  ]);
+});

tests/lib/park-attempt-files.test.mjs

@@ -5,7 +5,7 @@
 
 import { test } from 'node:test';
 import { strict as assert } from 'node:assert';
-import { mkdtempSync, writeFileSync, readFileSync, existsSync, mkdirSync, rmSync } from 'node:fs';
+import { mkdtempSync, writeFileSync, readFileSync, existsSync, mkdirSync, rmSync, symlinkSync } from 'node:fs';
 import { join, dirname } from 'node:path';
 import { tmpdir } from 'node:os';
 import { fileURLToPath } from 'node:url';
@@ -57,12 +57,15 @@ const DIFF = [
   '',
 ].join('\n');
 
-function run(dir, extra = []) {
-  writeFileSync(join(dir, 'upward.diff'), DIFF);
-  return spawnSync(process.execPath, [CLI, '--diff', join(dir, 'upward.diff'), '--to', join(dir, 'parked'), '--cwd', dir, ...extra], { encoding: 'utf8' });
+function run(dir, diff = DIFF) {
+  writeFileSync(join(dir, '.git', 'upward.diff'), diff);
+  return spawnSync(process.execPath, [CLI, '--diff', join(dir, '.git', 'upward.diff'), '--step', '1', '--attempt', '1', '--cwd', dir], { encoding: 'utf8' });
 }
 
-test('park: the step\'s untracked new files move into --to with their paths; tracked files stay', () => {
+// Parked outside the work tree, in the git dir, so a later `git ls-files --others` never lists them.
+const parkedPath = (dir, path) => join(dir, '.git', 'voyage', 'parked', 'step-1-attempt-1', path);
+
+test('park: the step\'s untracked new files move into the git dir with their paths; tracked files stay', () => {
   const dir = repo();
   try {
     mkdirSync(join(dir, 'tests'));
@@ -74,7 +77,8 @@ test('park: the step\'s untracked new files move into --to with their paths; tra
     const out = JSON.parse(r.stdout);
     assert.deepEqual(out.parked, ['tests/batch.sh', 'tests/cli-stdin.test.mjs']);
     assert.equal(existsSync(join(dir, 'tests/batch.sh')), false, 'the stale file is out of the tree');
-    assert.equal(readFileSync(join(dir, 'parked/tests/batch.sh'), 'utf8'), 'stale | cat\n', 'and kept, never deleted');
+    assert.equal(readFileSync(parkedPath(dir, 'tests/batch.sh'), 'utf8'), 'stale | cat\n', 'and kept, never deleted');
+    assert.equal(git(dir, 'ls-files', '--others', '--exclude-standard').trim(), '', 'nothing parked shows up as a new file');
     assert.equal(readFileSync(join(dir, 'bin/slug.mjs'), 'utf8'), 'new\n', 'a tracked file is not the helper\'s to move');
   } finally {
     rmSync(dir, { recursive: true, force: true });
@@ -104,9 +108,7 @@ test('park: a new file that is tracked now, or already gone, is kept and named,
 test('park: a path that leaves the repository is refused, never moved', () => {
   const dir = repo();
   try {
-    const evil = DIFF.replaceAll('tests/batch.sh', '../outside.sh');
-    writeFileSync(join(dir, 'upward.diff'), evil);
-    const r = spawnSync(process.execPath, [CLI, '--diff', join(dir, 'upward.diff'), '--to', join(dir, 'parked'), '--cwd', dir], { encoding: 'utf8' });
+    const r = run(dir, DIFF.replaceAll('tests/batch.sh', '../outside.sh'));
     assert.equal(r.status, 0, r.stderr);
     const out = JSON.parse(r.stdout);
     assert.ok(out.kept.some((k) => k.path === '../outside.sh' && /outside the repository/.test(k.reason)), r.stdout);
@@ -115,10 +117,66 @@ test('park: a path that leaves the repository is refused, never moved', () => {
   }
 });
 
+test('park: a path with a space is read and parked', () => {
+  const dir = repo();
+  try {
+    mkdirSync(join(dir, 'tests'));
+    writeFileSync(join(dir, 'tests/my batch.sh'), 'stale\n');
+    const r = run(dir, DIFF.replaceAll('tests/batch.sh', 'tests/my batch.sh'));
+    assert.equal(r.status, 0, r.stderr);
+    assert.ok(JSON.parse(r.stdout).parked.includes('tests/my batch.sh'), r.stdout);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('park: a new file under a header it cannot read is named, and exit 1 stops the run (review of 3238327)', () => {
+  const dir = repo();
+  try {
+    mkdirSync(join(dir, 'tests'));
+    writeFileSync(join(dir, 'tests/batch.sh'), 'stale\n');
+    const r = run(dir, DIFF.replace('diff --git a/tests/batch.sh b/tests/batch.sh', 'diff --git c/tests/batch.sh w/tests/batch.sh'));
+    assert.equal(r.status, 1, r.stdout);
+    const out = JSON.parse(r.stdout);
+    assert.ok(out.kept.some((k) => /header not read/.test(k.reason)), r.stdout);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('park: an attempt file still on disk because its target exists is exit 1, and the parked copy is never overwritten', () => {
+  const dir = repo();
+  try {
+    mkdirSync(join(dir, 'tests'));
+    writeFileSync(join(dir, 'tests/batch.sh'), 'second\n');
+    mkdirSync(dirname(parkedPath(dir, 'tests/batch.sh')), { recursive: true });
+    writeFileSync(parkedPath(dir, 'tests/batch.sh'), 'first\n');
+    const r = run(dir);
+    assert.equal(r.status, 1, r.stdout);
+    assert.equal(readFileSync(parkedPath(dir, 'tests/batch.sh'), 'utf8'), 'first\n');
+    assert.equal(existsSync(join(dir, 'tests/batch.sh')), true);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('park: a symlink the attempt made is parked, not called absent', () => {
+  const dir = repo();
+  try {
+    mkdirSync(join(dir, 'tests'));
+    symlinkSync('nowhere', join(dir, 'tests/batch.sh'));
+    const r = run(dir, DIFF.replace('new file mode 100755', 'new file mode 120000'));
+    assert.equal(r.status, 0, r.stdout);
+    assert.ok(JSON.parse(r.stdout).parked.includes('tests/batch.sh'), r.stdout);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
 test('park: an unreadable diff or missing arguments exit 2 and move nothing', () => {
   const dir = repo();
   try {
-    const r = spawnSync(process.execPath, [CLI, '--diff', join(dir, 'nope.diff'), '--to', join(dir, 'parked'), '--cwd', dir], { encoding: 'utf8' });
+    const r = spawnSync(process.execPath, [CLI, '--diff', join(dir, 'nope.diff'), '--step', '1', '--attempt', '1', '--cwd', dir], { encoding: 'utf8' });
     assert.equal(r.status, 2);
     assert.equal(spawnSync(process.execPath, [CLI], { encoding: 'utf8' }).status, 2);
   } finally {
@@ -130,8 +188,9 @@ test('trekexecute: a re-planned step parks the earlier attempt\'s new files befo
   const md = readFileSync(join(ROOT, 'commands/trekexecute.md'), 'utf8');
   const at = md.indexOf('A step at `fix_rounds` 4 on `--resume` was re-planned');
   assert.ok(at > 0);
-  const para = md.slice(at, at + 1200);
+  const para = md.slice(at, at + 1600);
   assert.match(para, /lib\/util\/park-attempt-files\.mjs/);
   assert.match(para, /step-\{N\}-upward\.diff/);
   assert.match(para, /never deleted/);
+  assert.match(para, /Exit 1 means a file the attempt made is still in the tree/);
 });

tests/lib/review-findings-kept.test.mjs

@@ -197,3 +197,17 @@ test('the coordinator writes @@ID_n@@ placeholders and the doc names the three-f
   assert.match(doc, /@@ID_<n>@@/);
   assert.match(read('commands/trekreview.md'), /fill-finding-ids\.mjs/);
 });
+
+test('engine findings: two different defects on one line under the catch-all key get two ids, the second over #2 (review of b47cabd)', () => {
+  const r = toReviewerPayload({
+    engine: 'code-review',
+    findings: [
+      { file: 'bin/slug.mjs', line: 46, summary: 'EAGAIN on a non-blocking stdin' },
+      { file: 'bin/slug.mjs', line: 46, summary: 'A lone CR does not split lines' },
+    ],
+  });
+  assert.equal(r.ok, true, r.errors?.join('\n'));
+  const [a, b] = r.payload.findings;
+  assert.equal(a.id, computeFindingId('bin/slug.mjs', 46, a.rule_key));
+  assert.equal(b.id, computeFindingId('bin/slug.mjs', 46, `${b.rule_key}#2`));
+});

tests/scripts/trekreview-armb-workflow.test.mjs

@@ -627,3 +627,13 @@ test('decision 23 — Handover 6 states the dedup rank on both paths, the put-ba
   assert.match(h6, PUT_BACK);
   assert.match(h6, FLOOR_B);
 });
+
+test('armB — two different /code-review findings on one triplet stay two rows for the coordinator (trial 5, finding 7)', async () => {
+  const c = await floorCase({
+    codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR', { title: 'EAGAIN on stdin' }), F('CODE_REVIEW_DEFECT', 'MAJOR', { title: 'A lone CR does not split lines' })],
+    reviewerFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR', { title: 'EAGAIN on stdin' })],
+    returned: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
+    verdict: 'WARN',
+  });
+  assert.deepEqual(c.merged.map((f) => f.title).sort(), ['A lone CR does not split lines', 'EAGAIN on stdin']);
+});

tests/validators/runnable-checks.test.mjs

@@ -199,12 +199,20 @@ test('brief: a criterion whose first backticked span is not a command is a warni
   assert.equal(r.valid, true, JSON.stringify(r.errors));
   const w = r.warnings.filter((x) => x.code === 'BRIEF_SC_NOT_RUNNABLE');
   assert.equal(w.length, 1, JSON.stringify(r.warnings));
-  assert.match(w[0].message, /SC1/);
-  assert.match(w[0].message, /--stdin/);
+  assert.match(w[0].message, /SC1's first backticked span is `--stdin`/);
+  assert.match(w[0].message, /\/bin\/bash tests\/contract\.sh/);
   assert.match(w[0].message, /not-a-command/);
   assert.match(w[0].hint, /first backticked span/);
 });
 
+test('brief: a prose criterion that names a flag or a path, with no command after it, stays unflagged (HANDOVER-CONTRACTS)', () => {
+  const r = validateBriefContent(brief([
+    '- The `--verbose` flag is documented.',
+    '- `docs/install.md` explains the install steps.',
+  ]));
+  assert.ok(!codes(r.warnings).includes('BRIEF_SC_NOT_RUNNABLE'), JSON.stringify(r.warnings));
+});
+
 test('brief: runnable and prose-only criteria raise no runnability warning', () => {
   const r = validateBriefContent(brief(['- `npm test` exits 0.', '- `bash tests/e2e.sh` exits 0.', '- The output reads well.']));
   assert.ok(!codes(r.warnings).includes('BRIEF_SC_NOT_RUNNABLE'), JSON.stringify(r.warnings));