feat(trekplan): Regression and On failure step rules, and trial 2's minor fixes

The minor points (B8-B12, C13, C14, C16, C17) of the second voyage-lekerepo trial (2026-10-08), each verified against the code and started with a test that failed first. - K10: optional step field `Regression:` - one command the criteria runner allows, unpiped; plan-validator refuses one that can never run (PLAN_STEP_REGRESSION_NOT_RUNNABLE). Any plan_version, no bump. - K11: `On failure:` opens with revert | retry | skip | escalate, or the validator warns (PLAN_STEP_ON_FAILURE_UNKNOWN); onFailureAction in plan-schema. - B8 (operator decision 2026-10-08): minors open after the last critic round are listed numbered in the Phase 10 summary; the ones the operator picks are fixed and get one more round (--extra-round when the 3 are spent). - B11: plan-review dedup merges two reviewers' findings on the same file:line with the same content (trial pair: Jaccard 0.44, overlap 0.83); the Jaccard threshold for every other pair is unchanged. - B12: /trekbrief Step 4c says the reviewer runs in the background and is waited for, not "foreground, blocking". - C13: the Phase 4 size count includes .mjs, .cjs, .mts, .cts. - C14: --evidence shows the first output line on PASS rows. - C16: findings-schema checks path:line citations in finding prose (error) and in the prose around the block (warning) against the file. - C17: git-historian gets the brief, never proposes changing a frozen file, and names the remote from `git remote -v`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-08 21:42:21 +02:00 · 2026-10-08 21:42:21 +02:00 · 255b6b4bf8
commit 255b6b4bf8
parent 4ee711a6ac
15 changed files with 675 additions and 24 deletions
--- a/tests/lib/trial2-minor-prose-pins.test.mjs
+++ b/tests/lib/trial2-minor-prose-pins.test.mjs
@ -0,0 +1,130 @@
+// tests/lib/trial2-minor-prose-pins.test.mjs
+// Prose pins for the minor points of the second voyage-lekerepo trial
+// (2026-10-08, report parts B and C) whose fix is text a session reads.
+// Counted as doc pins by lib/util/test-census.mjs (the file name).
+//   B8  minors left after the last critic round have a way to be fixed
+//       (operator decision 2026-10-08: listed in the summary, the operator picks).
+//   B9  K10 `Regression:` is described where plans are written and reviewed.
+//   B10 K11 the four `On failure:` words are named where plans are written.
+//   B12 /trekbrief Step 4c no longer claims a foreground, blocking spawn.
+//   C13 the codebase-size count in /trekplan Phase 4 counts .mjs/.cjs/.mts/.cts.
+//   C17 git-historian leaves the brief's frozen files alone and names the
+//       remote from `git remote -v`.
+
+import { test } from 'node:test';
+import { strict as assert } from 'node:assert';
+import { readFileSync, mkdtempSync, writeFileSync, rmSync } 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 { findSection } from '../../lib/parsers/plan-schema.mjs';
+
+const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
+const read = (p) => readFileSync(join(ROOT, p), 'utf8');
+
+/** The text of a level-2 section, up to the next `## ` heading outside a fence. */
+function section(text, heading) {
+  const s = findSection(text, heading.replace(/^## /, ''), 2);
+  assert.ok(s, `missing ${heading}`);
+  return s.text;
+}
+
+// --- B8 ------------------------------------------------------------------------
+
+test('B8: a PASS with minors sends them to the Phase 10 summary, numbered, for the operator to pick', () => {
+  const plan = read('commands/trekplan.md');
+  const p9 = section(plan, '## Phase 9 — Adversarial review (bounded critic loop)');
+  assert.match(p9, /\*\*PASS\*\*[\s\S]{0,600}minors[\s\S]{0,300}Phase 10/i);
+  const p10 = section(plan, '## Phase 10 — Present and refine');
+  assert.match(p10, /\*\*Open minors:\*\*/);
+  assert.match(p10, /numbered/i);
+});
+
+test('B8: fixing picked minors after the last round runs one more round with --extra-round, the pick being the yes', () => {
+  const p10 = section(read('commands/trekplan.md'), '## Phase 10 — Present and refine');
+  assert.match(p10, /--extra-round/);
+  assert.match(p10, /pick(ed|s)?\s+(is|counts as)\s+(the|their)\s+yes/i);
+});
+
+// --- B9 / B10 --------------------------------------------------------------------
+
+test('B9: /trekplan Phase 8 names `Regression:` as an allowed optional field and says what it is for', () => {
+  const p8 = section(read('commands/trekplan.md'), '## Phase 8 — Deep planning');
+  assert.match(p8, /`Regression:`/);
+  assert.match(p8, /PLAN_STEP_REGRESSION_NOT_RUNNABLE/);
+  assert.match(p8, /\*\*Regression:\*\* `npm test`/, 'the canonical example shows the field');
+  assert.match(p8, /before (the|its) checkpoint/i);
+});
+
+test('B9: the plan template shows `Regression:` as optional', () => {
+  const t = read('templates/plan-template.md');
+  assert.match(t, /- \*\*Regression:\*\* `[^\n]*\*\(optional\)\*/);
+  assert.match(t, /`Regression:` \(ONE backticked command/);
+});
+
+test('B9: plan-critic knows the field and does not demand the suite in `Verify:`', () => {
+  const c = read('agents/plan-critic.md');
+  assert.match(c, /`Regression:`/);
+  assert.match(c, /(never|do\s+not)\s+(demand|require)\s+`npm test`\s+in\s+`Verify:`/i);
+});
+
+test('B9/B10: Handover 4 documents `Regression:` (K10) and the `On failure:` words (K11)', () => {
+  const h4 = section(read('docs/HANDOVER-CONTRACTS.md'), '## Handover 4 — `plan.md` → execute');
+  for (const code of ['PLAN_STEP_REGRESSION_NOT_RUNNABLE', 'PLAN_STEP_ON_FAILURE_UNKNOWN']) assert.match(h4, new RegExp(code));
+  assert.match(h4, /K10/);
+  assert.match(h4, /K11/);
+  assert.match(h4, /revert[^\n]*retry[^\n]*skip[^\n]*escalate/);
+});
+
+test('B10: /trekplan Phase 8 says `On failure:` opens with one of the four words', () => {
+  const p8 = section(read('commands/trekplan.md'), '## Phase 8 — Deep planning');
+  assert.match(p8, /On failure:\*\*[^\n]*(opens|starts) with/i);
+  assert.match(p8, /PLAN_STEP_ON_FAILURE_UNKNOWN/);
+});
+
+// --- B12 -------------------------------------------------------------------------
+
+test('B12: /trekbrief Step 4c does not call the brief-reviewer spawn foreground or blocking, and waits for it', () => {
+  const t = read('commands/trekbrief.md');
+  const start = t.indexOf('**Step 4c — Launch brief-reviewer**');
+  const end = t.indexOf('**Step 4d — Parse JSON scores**');
+  assert.ok(start !== -1 && end > start);
+  const step = t.slice(start, end);
+  assert.doesNotMatch(step, /foreground, blocking/);
+  assert.match(step, /wait\s+for\s+(its|the agent's)\s+(result|completion)/i);
+  assert.match(step, /(background|asynchronous)/i, 'says how the spawn actually runs');
+});
+
+// --- C13 -------------------------------------------------------------------------
+
+test('C13: the Phase 4 size count counts .mjs, .cjs, .mts and .cts files', () => {
+  const p4 = section(read('commands/trekplan.md'), '## Phase 4 — Codebase sizing');
+  const cmd = p4.match(/```\n(find [^\n]+)\n```/);
+  assert.ok(cmd, 'the find command stands in a fence of its own');
+  const dir = mkdtempSync(join(tmpdir(), 'voyage-sizing-'));
+  try {
+    for (const f of ['a.mjs', 'b.cjs', 'c.mts', 'd.cts', 'e.ts', 'skip.txt']) writeFileSync(join(dir, f), 'x\n');
+    const r = spawnSync('/bin/sh', ['-c', cmd[1]], { cwd: dir, encoding: 'utf8' });
+    assert.equal(r.status, 0, r.stderr);
+    assert.equal(r.stdout.trim(), '5');
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+// --- C17 -------------------------------------------------------------------------
+
+test('C17: git-historian reads the brief, never proposes changing a file it freezes', () => {
+  const g = read('agents/git-historian.md');
+  assert.match(g, /brief/i);
+  assert.match(g, /(never|do not) (propose|suggest|recommend)[^\n]*(change|edit)[^\n]*(frozen|unchanged)|(frozen|unchanged)[^\n]*(never|do not) (propose|suggest|recommend)/i);
+  const p5 = section(read('commands/trekplan.md'), '## Phase 5 — Parallel exploration (specialized agents + research)');
+  assert.match(p5, /\*\*git-historian\*\*[\s\S]{0,400}\{brief_path\}/, 'the spawn hands it the brief');
+});
+
+test('C17: git-historian names the remote from `git remote -v`, never by guess', () => {
+  const g = read('agents/git-historian.md');
+  assert.match(g, /`git remote -v`/);
+  assert.match(g, /local (path|repository|bare)/i);
+});
--- a/tests/lib/trial2-minor.test.mjs
+++ b/tests/lib/trial2-minor.test.mjs
@ -0,0 +1,253 @@
+// tests/lib/trial2-minor.test.mjs
+// The minor points of the second voyage-lekerepo trial (2026-10-08, report
+// parts B and C) that are behaviour, not prose. Each test was red before its
+// fix.
+//   B9  K10 — optional step field `Regression:`: one runnable command, or the
+//       validator refuses it.
+//   B10 K11 — `On failure:` opens with revert | retry | skip | escalate, or the
+//       validator warns.
+//   B11 two findings on the same file:line with the same content merge (the
+//       trial's pair measured Jaccard 0.44 against a 0.7 threshold).
+//   C14 `--evidence` shows the first output line on a PASS row, not "—".
+//   C16 a file:line cited in a reviewer's prose is checked against the file.
+
+import { test } from 'node:test';
+import { strict as assert } from 'node:assert';
+import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
+import { join } from 'node:path';
+import { tmpdir } from 'node:os';
+import { validatePlanContent } from '../../lib/validators/plan-validator.mjs';
+import { onFailureAction, ON_FAILURE_ACTIONS } from '../../lib/parsers/plan-schema.mjs';
+import { dedupFindings, DEFAULT_THRESHOLD } from '../../lib/review/plan-review-dedup.mjs';
+import { formatCriteriaEvidence } from '../../lib/verification/criteria-runner.mjs';
+import { validateFindings, validateReviewerOutput } from '../../lib/review/findings-schema.mjs';
+import { buildPlan } from '../helpers/plan-v18-builder.mjs';
+
+const codes = (list) => list.map((e) => e.code);
+
+// Step 1's Verify line in the builder's plan; the new field goes right under it.
+const STEP1_VERIFY = '- **Verify:** `node --test tests/mod1.test.mjs` → expected: pass';
+const withRegression = (line, plan = buildPlan()) => plan.replace(STEP1_VERIFY, `${STEP1_VERIFY}\n${line}`);
+const withOnFailure = (text, plan = buildPlan()) =>
+  plan.replace(/^- \*\*On failure:\*\* .*$/m, `- **On failure:** ${text}`);
+
+// --- B9: K10 `Regression:` ----------------------------------------------------
+
+test('K10: `Regression: npm test | tee x` is refused — one command, unpiped', () => {
+  const r = validatePlanContent(withRegression('- **Regression:** `npm test | tee x` → expected: exit 0'));
+  assert.equal(r.valid, false);
+  const e = r.errors.find((x) => x.code === 'PLAN_STEP_REGRESSION_NOT_RUNNABLE');
+  assert.ok(e, codes(r.errors).join(','));
+  assert.match(e.message, /Step 1/);
+  assert.match(e.message, /shell operator/);
+});
+
+test('K10: `Regression: npm test` is accepted', () => {
+  const r = validatePlanContent(withRegression('- **Regression:** `npm test` → expected: exit 0'));
+  assert.deepEqual(codes(r.errors), []);
+  assert.ok(!codes(r.warnings).includes('PLAN_STEP_REGRESSION_NOT_RUNNABLE'));
+});
+
+test('K10: without the field the plan validates exactly as before', () => {
+  const r = validatePlanContent(buildPlan());
+  assert.equal(r.valid, true, codes(r.errors).join(','));
+  assert.deepEqual(codes(r.warnings), []);
+});
+
+test('K10: `npm test && bash tests/e2e.sh`, a bare word, or no backticks is refused', () => {
+  for (const line of [
+    '- **Regression:** `npm test && bash tests/e2e.sh`',
+    '- **Regression:** `./run-all-tests`',
+    '- **Regression:** npm test',
+    '- **Regression:**',
+  ]) {
+    const r = validatePlanContent(withRegression(line));
+    assert.ok(codes(r.errors).includes('PLAN_STEP_REGRESSION_NOT_RUNNABLE'), `${line}: ${codes(r.errors).join(',')}`);
+  }
+});
+
+test('K10: --soft reports an unrunnable Regression as a warning, not an error', () => {
+  const r = validatePlanContent(withRegression('- **Regression:** `npm test | tee x`'), { strict: false });
+  assert.equal(r.valid, true, codes(r.errors).join(','));
+  assert.ok(codes(r.warnings).includes('PLAN_STEP_REGRESSION_NOT_RUNNABLE'));
+});
+
+// --- B10: K11 `On failure:` ---------------------------------------------------
+
+test('K11: `On failure: try again` warns, and the plan stays valid', () => {
+  const r = validatePlanContent(withOnFailure('try again'));
+  assert.equal(r.valid, true, codes(r.errors).join(','));
+  const w = r.warnings.find((x) => x.code === 'PLAN_STEP_ON_FAILURE_UNKNOWN');
+  assert.ok(w, codes(r.warnings).join(','));
+  assert.match(w.message, /Step 1/);
+  for (const word of ON_FAILURE_ACTIONS) assert.match(w.hint, new RegExp(word));
+});
+
+test('K11: `On failure: retry once, then escalate` gives no warning', () => {
+  const r = validatePlanContent(withOnFailure('retry once, then escalate'));
+  assert.ok(!codes(r.warnings).includes('PLAN_STEP_ON_FAILURE_UNKNOWN'), codes(r.warnings).join(','));
+});
+
+test('K11: the four words, in any case and with markdown around them, are read; a longer word is not', () => {
+  assert.deepEqual([...ON_FAILURE_ACTIONS], ['revert', 'retry', 'skip', 'escalate']);
+  assert.equal(onFailureAction('revert — `git checkout -- x`'), 'revert');
+  assert.equal(onFailureAction('Escalate: stop the run'), 'escalate');
+  assert.equal(onFailureAction('**skip** this step'), 'skip');
+  assert.equal(onFailureAction('`retry` once'), 'retry');
+  assert.equal(onFailureAction('retrying is fine'), null);
+  assert.equal(onFailureAction('try again'), null);
+  assert.equal(onFailureAction(''), null);
+});
+
+// --- B11: same file:line, same content ---------------------------------------
+
+// The two findings from trial 2's round 1, verbatim (plan.md:248).
+const PLAN_MD = '.claude/projects/2026-10-08-slug-sep-option/plan.md';
+const CRITIC_248 = {
+  severity: 'minor', rule_key: 'verification/contract-brief-form-missing', file: PLAN_MD, line: 248,
+  text: "Verification runs `bash tests/e2e.sh` in the brief's own form but not `bash tests/contract.sh`, the form brief SC4 names.",
+};
+const GUARDIAN_248 = {
+  severity: 'minor', rule_key: 'gap-verification-command-form', file: PLAN_MD, line: 248,
+  text: "Brief SC4 runs 'bash tests/contract.sh'; Verification lists only '/bin/bash tests/contract.sh' (unlike e2e, where both forms are listed) - add the brief's form",
+};
+const sources = (a, b) => [
+  { agent: 'plan-critic', payload: { findings: a } },
+  { agent: 'scope-guardian', payload: { findings: b } },
+];
+
+test('B11: the two trial-2 findings on plan.md:248 merge into one, raised by both', () => {
+  const r = dedupFindings(sources([CRITIC_248], [GUARDIAN_248]));
+  assert.equal(r.findings.length, 1, JSON.stringify(r.findings, null, 2));
+  assert.deepEqual(r.findings[0].raised_by, ['plan-critic', 'scope-guardian']);
+  assert.equal(r.dedup_stats.anchor_dups, 1);
+});
+
+test('B11: two different findings on the same line stay two', () => {
+  const other = {
+    severity: 'minor', rule_key: 'verification/no-negative-case', file: PLAN_MD, line: 248,
+    text: 'No criterion feeds an invalid separator such as a letter and checks the exit code.',
+  };
+  const r = dedupFindings(sources([CRITIC_248], [other]));
+  assert.equal(r.findings.length, 2);
+});
+
+test('B11: a short finding held by a longer one on the same line stays its own (independent review)', () => {
+  const at = (text, severity = 'minor') => ({ severity, rule_key: `k-${text.length}`, file: PLAN_MD, line: 40, text });
+  for (const [a, b] of [
+    ['The token is checked.', 'The token is not checked before the write.'],
+    ['Step 3 test is missing.', 'Step 3 is missing a checkpoint; the test file already exists.'],
+    ['Verify runs npm test.', 'Verify must not run npm test, it is green before the step.'],
+  ]) {
+    assert.equal(dedupFindings(sources([at(a)], [at(b)])).findings.length, 2, `${a} | ${b}`);
+  }
+});
+
+test('B11: one reviewer\'s two findings on one line are never merged by the anchor rule', () => {
+  const r = dedupFindings([{ agent: 'plan-critic', payload: { findings: [CRITIC_248, { ...GUARDIAN_248, rule_key: 'other' }] } }]);
+  assert.equal(r.findings.length, 2);
+  assert.equal(r.dedup_stats.anchor_dups, 0);
+});
+
+test('B11: the same content on another line is not merged by the anchor rule; the threshold elsewhere is unchanged', () => {
+  assert.equal(DEFAULT_THRESHOLD, 0.7);
+  const r = dedupFindings(sources([CRITIC_248], [{ ...GUARDIAN_248, line: 249 }]));
+  assert.equal(r.findings.length, 2);
+  const noLine = dedupFindings(sources([{ ...CRITIC_248, line: 0 }], [{ ...GUARDIAN_248, line: 0 }]));
+  assert.equal(noLine.findings.length, 2, 'line 0 is file-scoped, not an anchor');
+});
+
+// --- C14: --evidence on a PASS row ---------------------------------------------
+
+const evidenceRep = (results) => ({
+  source: 'brief.md',
+  heading: '## Success Criteria',
+  error: null,
+  results,
+  summary: { passed: 1, failed: 0, blocked: 0, unrunnable: 0, total: 1, ok: true },
+});
+
+test('C14: --evidence shows the first line of output on a passed criterion', () => {
+  const block = formatCriteriaEvidence(evidenceRep([
+    { label: 'SC1', text: 't', command: 'npm test', status: 'passed', exitCode: 0, output: '\n# tests 12\n# pass 12\n' },
+  ]));
+  assert.match(block, /\| SC1 \| `npm test` \| PASS \| exit 0 \| # tests 12 \|/);
+});
+
+test('C14: a passed criterion with no output still says "—"', () => {
+  const block = formatCriteriaEvidence(evidenceRep([
+    { label: 'SC1', text: 't', command: 'npm test', status: 'passed', exitCode: 0, output: '' },
+  ]));
+  assert.match(block, /\| SC1 \| `npm test` \| PASS \| exit 0 \| — \|/);
+});
+
+// --- C16: file:line in prose --------------------------------------------------
+
+function repoWith33() {
+  const dir = mkdtempSync(join(tmpdir(), 'voyage-prose-cite-'));
+  mkdirSync(join(dir, 'tests'));
+  writeFileSync(join(dir, 'tests', 'x.sh'), Array.from({ length: 33 }, (_, i) => `echo ${i + 1}`).join('\n') + '\n');
+  return dir;
+}
+
+const finding = (over = {}) => ({
+  id: '0123456789abcdef0123456789abcdef01234567',
+  severity: 'MAJOR', rule_key: 'UNIMPLEMENTED_CRITERION', file: 'tests/x.sh', line: 3,
+  title: 'A title', detail: 'A detail.', recommended_action: 'Do one thing.', ...over,
+});
+
+test('C16: a finding whose prose cites `tests/x.sh:119` in a 33-line file is refused', () => {
+  const dir = repoWith33();
+  try {
+    const r = validateFindings(
+      { reviewer: 'code-correctness-reviewer', findings: [finding({ detail: 'The check at `tests/x.sh:119` relies on git.' })] },
+      { repoRoot: dir },
+    );
+    assert.equal(r.valid, false);
+    const e = r.errors.find((x) => x.code === 'FINDING_PROSE_LINE_OUT_OF_RANGE');
+    assert.ok(e, JSON.stringify(r.errors));
+    assert.match(e.message, /tests\/x\.sh:119/);
+    assert.match(e.message, /33 lines/);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('C16: the same citation in prose outside the JSON block is a warning, never silent (a JSON-only re-ask cannot fix prose)', () => {
+  const dir = repoWith33();
+  try {
+    const raw = [
+      '### Risk noted but not raised as a finding',
+      '',
+      '`tests/x.sh:119` relies on `git` and on a fixed commit.',
+      '',
+      '```json',
+      JSON.stringify({ reviewer: 'code-correctness-reviewer', findings: [] }),
+      '```',
+    ].join('\n');
+    const r = validateReviewerOutput(raw, { repoRoot: dir });
+    assert.equal(r.valid, true, JSON.stringify(r.errors));
+    const w = r.warnings.find((x) => x.code === 'FINDING_PROSE_LINE_OUT_OF_RANGE');
+    assert.ok(w, JSON.stringify(r.warnings));
+    assert.match(w.message, /reviewer prose cites `?tests\/x\.sh:119/);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});
+
+test('C16: a line that exists, a range ending in the file, a file absent from the tree and no repo root all pass', () => {
+  const dir = repoWith33();
+  try {
+    const ok = (detail, opts = { repoRoot: dir }) =>
+      validateFindings({ reviewer: 'r', findings: [finding({ detail })] }, opts).valid;
+    assert.equal(ok('See tests/x.sh:33 and tests/x.sh:10-12.'), true);
+    assert.equal(ok('See src/absent.mjs:900.'), true);
+    assert.equal(ok('See tests/x.sh:119.', {}), true);
+    assert.equal(ok('See tests/x.sh:30-40.'), false, 'a range that runs past the end is out of range');
+    assert.equal(ok('Served on http://localhost:8080 and example.com:443.'), true);
+    assert.equal(ok('On Windows: C:\\work\\tests\\x.sh:119 and tests\\x.sh:119.'), true, 'a Windows path is not the root\'s file');
+    assert.equal(ok('See tests/x.sh:30\u201440.'), false, 'an em-dash range is read as a range');
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+});