voyage/tests/lib/trial2-fixes.test.mjs

// tests/lib/trial2-fixes.test.mjs
// The voyage-lekerepo second end-to-end trial (2026-10-08), report section A:
// faults that make a result wrong or misleading. The operator decided they are
// fixed before the next release. Each test fails on main 5f9faee.

import { test } from 'node:test';
import { strict as assert } from 'node:assert';
import { spawnSync } from 'node:child_process';
import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';
import { toReviewerPayload } from '../../lib/review/engine-findings.mjs';
import { runContract } from '../../lib/review/coordinator-contract.mjs';
import { getRule } from '../../lib/review/rule-catalogue.mjs';
import { parseSuccessCriteria } from '../../lib/verification/criteria-runner.mjs';
import { validateBriefContent } from '../../lib/validators/brief-validator.mjs';

const HERE = dirname(fileURLToPath(import.meta.url));
const ROOT = join(HERE, '..', '..');
const read = (p) => readFileSync(join(ROOT, p), 'utf8');
const codes = (list) => list.map((e) => e.code);

const engineFinding = (extra = {}) => ({
  file: 'src/slug.mjs', line: 9,
  summary: 'A two-step detour where one split/filter/join would do',
  failure_scenario: 'slug("a--b") walks the string twice',
  ...extra,
});

// --- A1. an improvement from /code-review is not a MAJOR defect --------------

test('A1: a /code-review simplification or efficiency finding is MINOR, a correctness finding stays MAJOR', () => {
  const r = toReviewerPayload({ engine: 'code-review', findings: [
    engineFinding({ category: 'simplification' }),
    engineFinding({ line: 10, category: 'efficiency' }),
    engineFinding({ line: 11, category: 'correctness' }),
    engineFinding({ line: 12 }),
  ] });
  assert.equal(r.ok, true, r.errors.join('; '));
  const sev = r.payload.findings.map((f) => f.severity);
  assert.deepEqual(sev, ['MINOR', 'MINOR', 'MAJOR', 'MAJOR']);
  for (const f of r.payload.findings) assert.equal(f.severity, getRule(f.rule_key).severity, 'severity is fixed by rule_key');
});

test('A1: /security-review keeps its tier whatever the category', () => {
  const r = toReviewerPayload({ engine: 'security-review', findings: [engineFinding({ category: 'simplification' })] });
  assert.equal(r.payload.findings[0].severity, 'BLOCKER');
});

// --- A2. recommended_action is a remedy, not the failure scenario -----------

test('A2: the engine\'s own fix text becomes the recommended action', () => {
  const r = toReviewerPayload({ engine: 'code-review', findings: [engineFinding({ fix: 'Use one split/filter/join pass' })] });
  assert.equal(r.payload.findings[0].recommended_action, 'Use one split/filter/join pass');
});

test('A2: without a fix text the action is a step: reproduce the scenario in a test, then change the location', () => {
  const r = toReviewerPayload({ engine: 'code-review', findings: [engineFinding()] });
  const a = r.payload.findings[0].recommended_action;
  assert.doesNotMatch(a, /so this no longer happens/);
  assert.match(a, /^Write a test that reproduces/);
  assert.match(a, /src\/slug\.mjs:9/);
});

test('A2: /trekreview asks the transcription for the engine\'s fix text and forbids inventing one', () => {
  const md = read('commands/trekreview.md');
  const p = md.slice(md.indexOf('## Phase 4.7'), md.indexOf('## Phase 5'));
  assert.match(p, /the\s+fix\s+the\s+engine\s+proposes\s+\(`fix`\)/);
  assert.match(p, /never\s+write\s+one\s+the\s+engine\s+did\s+not\s+give/);
});

// --- A3. an out-of-range /code-review finding does not lock the verdict -----

test('A3: a small /code-review finding outside the exact range it was given is declined, and does not hold ALLOW back', () => {
  const out = { ...engineFinding({ file: 'src/util.mjs', line: 4 }), engine: 'code-review', rule_key: 'CODE_REVIEW_IMPROVEMENT', severity: 'MINOR', title: 'Simplify the helper' };
  const c = runContract([], { engineOutOfScope: [out] });
  assert.equal(c.verdict, 'ALLOW', JSON.stringify(c.allow_blocked_by));
  assert.equal(c.declined_to_judge.length, 1);
  assert.equal(c.declined_to_judge[0].reason, 'engine:out-of-scope');
});

test('A3 (independent review): a MAJOR /code-review defect cited at an unchanged file still forbids ALLOW — a cross-file break is cited there', () => {
  const out = { ...engineFinding({ file: 'tests/cli.test.mjs', line: 1 }), engine: 'code-review', rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR', title: 'No CLI test for --sep' };
  const c = runContract([], { engineOutOfScope: [out] });
  assert.notEqual(c.verdict, 'ALLOW');
  assert.equal(c.declined_to_judge.length, 1);
});

test('A3: a /security-review finding outside the range still forbids ALLOW (its base is wider and not ours)', () => {
  const out = { ...engineFinding({ file: 'lib/auth.mjs' }), engine: 'security-review', rule_key: 'SECURITY_REVIEW_VULNERABILITY', severity: 'BLOCKER', title: 'Token logged' };
  const c = runContract([], { engineOutOfScope: [out] });
  assert.notEqual(c.verdict, 'ALLOW');
});

// --- A4. the manifest's must_contain check survives a pattern that starts with "-"

test('A4: the must_contain recipe passes the pattern with -e and ends options with --', () => {
  const md = read('commands/trekexecute.md');
  assert.match(md, /`grep -E -e "\{pattern\}" -- \{path\}`/);
  assert.doesNotMatch(md, /`grep -E "\{pattern\}" \{path\}`/);
  const dir = mkdtempSync(join(tmpdir(), 'voyage-grep-'));
  try {
    writeFileSync(join(dir, 'README.md'), 'Use --sep to choose the separator.\n');
    const r = spawnSync('/bin/bash', ['-c', 'grep -E -e "--sep" -- README.md'], { cwd: dir, encoding: 'utf8' });
    assert.equal(r.status, 0, r.stderr);
  } finally { rmSync(dir, { recursive: true, force: true }); }
});

// --- A5. sub-points belong to their criterion -------------------------------

function brief(sections) {
  return [
    '---', 'type: trekbrief', 'brief_version: "2.2"', 'task: t', 'slug: s',
    'research_topics: 0', 'research_status: skipped', 'framing: new-direction', 'phase_signals_partial: true', '---', '',
    '## TL;DR', '', 'A toy.', '', '## Intent', '', 'Why.', '', '## Goal', '', 'What.', '',
    ...sections,
  ].join('\n');
}

const NESTED = [
  '## Success Criteria', '',
  '- `npm test` exits 0.',
  '- The separator is honoured:',
  '  - `-` joins words',
  '  - `_` joins words',
  '  - `.` joins words',
  '- `bash tests/e2e.sh` exits 0.', '',
];

test('A5: indented sub-points are part of their criterion, not criteria of their own', () => {
  const sc = parseSuccessCriteria(brief(NESTED));
  assert.deepEqual(sc.map((c) => c.label), ['SC1', 'SC2', 'SC3']);
  assert.equal(sc[1].subpoints, 3);
  assert.equal(sc[2].command, 'bash tests/e2e.sh');
});

test('A5: a criterion with sub-points and no command of its own is a warning that names it', () => {
  const r = validateBriefContent(brief(NESTED));
  const w = r.warnings.filter((x) => x.code === 'BRIEF_SC_SUBPOINTS');
  assert.equal(w.length, 1, JSON.stringify(r.warnings));
  assert.match(w[0].message, /SC2/);
});

// --- A6. a bash 3.2 constraint is measured under bash 3.2 --------------------

const BASH32 = ['## Constraints', '', '- Scripts must run under bash 3.2 (macOS system bash).', ''];

test('A6: a brief that requires bash 3.2 and runs a criterion with plain `bash` gets a warning pointing at /bin/bash', () => {
  const r = validateBriefContent(brief([...BASH32, '## Success Criteria', '', '- `bash tests/contract.sh` exits 0.', '']));
  const w = r.warnings.filter((x) => x.code === 'BRIEF_SC_BASH_NOT_PINNED');
  assert.equal(w.length, 1, JSON.stringify(r.warnings));
  assert.match(w[0].hint, /\/bin\/bash tests\//);
});

test('A6: `/bin/bash`, or no bash 3.2 constraint, raises no such warning', () => {
  const pinned = validateBriefContent(brief([...BASH32, '## Success Criteria', '', '- `/bin/bash tests/contract.sh` exits 0.', '']));
  assert.ok(!codes(pinned.warnings).includes('BRIEF_SC_BASH_NOT_PINNED'));
  const free = validateBriefContent(brief(['## Success Criteria', '', '- `bash tests/contract.sh` exits 0.', '']));
  assert.ok(!codes(free.warnings).includes('BRIEF_SC_BASH_NOT_PINNED'));
});

test('A6: /trekbrief proposes /bin/bash when the brief requires bash 3.2', () => {
  const md = read('commands/trekbrief.md');
  assert.match(md, /requires\s+bash\s+3\.2[^`]*`\/bin\/bash tests\/<script>\.sh`/);
  assert.match(md, /BRIEF_SC_BASH_NOT_PINNED/);
  assert.match(md, /BRIEF_SC_SUBPOINTS/);
});

// --- A7. the plan template's checkpoint stages what it commits --------------

test('A7: every checkpoint the plan template and /trekplan\'s example show stages the step\'s files first', () => {
  const lines = [
    ...read('templates/plan-template.md').split('\n'),
    ...read('commands/trekplan.md').split('\n'),
  ].filter((l) => /\*\*Checkpoint:\*\*/.test(l) && /`git [^`]*commit -m /.test(l));
  assert.ok(lines.length >= 3, `found ${lines.length}`);
  for (const l of lines) assert.match(l, /`git add [^`]+ && git commit -m /, l);
});

// --- the independent review of these fixes (2026-10-08) ----------------------

test('A1 (review): a `reuse` finding is an improvement too', () => {
  const r = toReviewerPayload({ engine: 'code-review', findings: [engineFinding({ category: 'Reuse ' })] });
  assert.equal(r.payload.findings[0].severity, 'MINOR');
});

test('A5 (review): a sub-bullet that carries its own command stays a criterion; only command-less sub-points fold', () => {
  const sc = parseSuccessCriteria(brief(['## Success Criteria', '',
    '- Behaviour:', '  - `npm test` exits 0', '  - `bash tests/e2e.sh` exits 0', '  - reads well', '']));
  assert.deepEqual(sc.map((c) => c.command), [null, 'npm test', 'bash tests/e2e.sh']);
  assert.equal(sc[0].subpoints, 1);
});

test('A5 (review): an indented first bullet does not turn later top-level bullets into sub-points', () => {
  const sc = parseSuccessCriteria(brief(['## Success Criteria', '',
    '  - The first, indented by accident.', '- The second:', '  - a sub-point of the second', '- `npm test` exits 0.', '']));
  assert.equal(sc.length, 3, JSON.stringify(sc.map((c) => c.text)));
  assert.equal(sc[1].subpoints, 1);
});

test('A6 (review): a negated bash 3.2 constraint, an env prefix and `bash -c` are handled', () => {
  const neg = validateBriefContent(brief(['## Constraints', '', '- bash 3.2 is not required here.', '',
    '## Success Criteria', '', '- `bash tests/contract.sh` exits 0.', '']));
  assert.ok(!codes(neg.warnings).includes('BRIEF_SC_BASH_NOT_PINNED'), 'negated constraint');
  const env = validateBriefContent(brief([...BASH32, '## Success Criteria', '', '- `LC_ALL=C bash tests/contract.sh` exits 0.', '']));
  assert.ok(codes(env.warnings).includes('BRIEF_SC_BASH_NOT_PINNED'), 'env-prefixed bash');
  const dashC = validateBriefContent(brief([...BASH32, '## Success Criteria', '', '- `bash -c true` exits 0.', '']));
  for (const w of dashC.warnings.filter((x) => x.code === 'BRIEF_SC_BASH_NOT_PINNED')) {
    assert.doesNotMatch(w.hint, /\/bin\/bash -c/, 'never suggests a form the runner refuses');
  }
});

test('A7 (review): the session-spec template stages before it commits too', () => {
  const l = read('templates/session-spec-template.md').split('\n').filter((x) => /\*\*Checkpoint:\*\*/.test(x) && /`git [^`]*commit -m /.test(x));
  assert.ok(l.length >= 1);
  for (const x of l) assert.match(x, /`git add [^`]+ && git commit -m /, x);
});

test('docs (review): the counts and the Declined-to-Judge rule match the code', () => {
  assert.match(read('docs/architecture.md'), /rule catalogue \(15 keys\)/);
  assert.match(read('docs/HANDOVER-CONTRACTS.md'), /three engine rule keys/);
  assert.doesNotMatch(read('lib/review/engine-findings.mjs'), /it forbids ALLOW and is listed under/);
  assert.match(read('agents/review-coordinator.md'), /every `\[unverified\]` one, and every\s+out-of-scope engine finding/);
});