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

// 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);
  // Trial 2 (2026-10-08): the action is a test-first step, not the scenario
  // dressed as a remedy (tests/lib/trial2-fixes.test.mjs, A2).
  assert.match(f.recommended_action, /^Write a test /);
  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, /^Write a test /);
  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/);
});