voyage/lib/review/findings-schema.mjs

// lib/review/findings-schema.mjs
// Reviewer-output JSON schema contract for /trekreview Phase 5 (NW1).
//
// brief-conformance-reviewer and code-correctness-reviewer each emit a trailing
// fenced `json` block of shape:
//
//   { "reviewer": "<name>", "findings": [ { id, severity, rule_key, file, line,
//                                           brief_ref, title, detail,
//                                           recommended_action }, ... ] }
//
// This module codifies that contract so main can VALIDATE each reviewer's JSON
// (not merely JSON.parse it) and re-ask on *schema* failure as well as parse
// failure, replacing the fragile "parse the last json block" contract that used
// to live in commands/trekreview.md (the :202–204 prose).
//
// Load-bearing fields (the downstream dedup triplet + verdict severity) are hard
// errors: file, rule_key, severity, line. Unknown rule_keys are errors too — the
// catalogue is the contract. Descriptive fields (title/detail/recommended_action/
// brief_ref) and unknown top-level keys are tolerated (forward-compat, mirroring
// review-validator.mjs).
//
// 3-layer pattern (Content → Raw-text → CLI shim) mirroring the other validators.

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

// The fields main + the coordinator depend on. Descriptive fields are not here
// on purpose: a missing recommended_action should not trigger a re-ask.
export const FINDING_REQUIRED_FIELDS = Object.freeze([
  'severity',
  'rule_key',
  'file',
  'line',
]);

// Last fenced ```json … ``` block in a reviewer's output. The contract pins the
// JSON block as the LAST fence so prose above it never confuses the parser.
// Both fences stand on a line of their own, as in Markdown and in the verdict
// gate's reader: a JSON string cannot hold a line break, so three backticks
// inside one ("a fenced ```bash block") never end the block. Ending it at the
// first triple backtick anywhere made fill-finding-ids exit 2 (release review
// of 6.3.0, found while writing round 7's review.md; present in 6.2.0).
const JSON_FENCE_GLOBAL = /^[ \t]*```json[ \t]*\r?\n([\s\S]*?)^[ \t]*```[ \t]*\r?$/gim;

/**
 * Extract the inner body of the LAST fenced `json` block in `text`.
 * @param {string} text
 * @returns {string|null} the JSON source, or null if no json fence is present.
 */
export function extractFindingsBlock(text) {
  if (typeof text !== 'string') return null;
  JSON_FENCE_GLOBAL.lastIndex = 0;
  let last = null;
  let m;
  while ((m = JSON_FENCE_GLOBAL.exec(text)) !== null) {
    last = m[1];
  }
  return last;
}

function validateFinding(finding, index, errors) {
  const loc = `findings[${index}]`;
  if (finding === null || typeof finding !== 'object' || Array.isArray(finding)) {
    errors.push(issue('FINDING_NOT_OBJECT', `${loc} is not an object`, undefined, loc));
    return;
  }

  if (typeof finding.file !== 'string' || finding.file.length === 0) {
    errors.push(issue('FINDING_MISSING_FILE', `${loc}.file must be a non-empty string`, undefined, loc));
  }

  if (typeof finding.rule_key !== 'string' || finding.rule_key.length === 0) {
    errors.push(issue('FINDING_MISSING_RULE_KEY', `${loc}.rule_key must be a non-empty string`, undefined, loc));
  } else if (!RULE_KEYS.has(finding.rule_key)) {
    errors.push(issue(
      'FINDING_UNKNOWN_RULE_KEY',
      `${loc}.rule_key "${finding.rule_key}" is not in the rule catalogue`,
      'Use a rule_key from lib/review/rule-catalogue.mjs',
      loc,
    ));
  }

  if (typeof finding.severity !== 'string' || !SEVERITY_VALUES.includes(finding.severity)) {
    errors.push(issue(
      'FINDING_BAD_SEVERITY',
      `${loc}.severity must be one of ${SEVERITY_VALUES.join('|')}, got ${JSON.stringify(finding.severity)}`,
      undefined,
      loc,
    ));
  }

  if (typeof finding.line !== 'number' || !Number.isInteger(finding.line) || finding.line < 0) {
    errors.push(issue(
      'FINDING_BAD_LINE',
      `${loc}.line must be an integer ≥ 0, got ${JSON.stringify(finding.line)}`,
      'Use 0 for file-scoped findings without a specific line.',
      loc,
    ));
  }
}

/**
 * Validate an already-parsed reviewer-output payload against the schema.
 * Accumulates every error (so a re-ask can name all problems at once).
 * @param {unknown} payload
 * @returns {import('../util/result.mjs').Result}
 */
// 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 readLineCount(root, file) {
  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;
  }
}

/**
 * Line counts for one validation, each file read once however often it is
 * cited: a reviewer that cited the 3,000-line commands/trekexecute.md 40
 * times read it 40 times, again on every re-ask (release review of 6.3.0,
 * round 7). The map lives for one call, so an edited file is read afresh.
 */
function lineCounter(repoRoot) {
  const root = resolve(repoRoot);
  const counts = new Map();
  return (file) => {
    if (!counts.has(file)) counts.set(file, readLineCount(root, file));
    return counts.get(file);
  };
}

function checkLineRange(finding, index, lineCount, errors) {
  if (!finding || typeof finding.file !== 'string' || !Number.isInteger(finding.line) || finding.line <= 0) return;
  const n = lineCount(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.',
  ));
}

// The same check for a `path:line` (or `path:a-b`) a reviewer writes in prose:
// a finding's title/detail/recommended_action/brief_ref, and the text around
// the JSON block. code-correctness-reviewer wrote "`tests/contract.sh:119`
// relies on git" about a 33-line file, and nothing looked (voyage-lekerepo
// trial 2, 2026-10-08). In a finding's fields a line past the end of a file
// that exists is an error (the re-ask can fix the block); in the prose around
// the block it is a warning (see validateReviewerOutput). A path absent from
// the tree is not judged. The path needs an extension, so `localhost:8080` and
// `host.example:443` are no citation of anything in the tree; a path glued to
// a `\` or `:` (a Windows path) is not read as one relative to the root.
const PROSE_CITE = /(?<![\w./\\:-])((?:\.{0,2}\/)?[\w.-]+(?:\/[\w.-]+)*\.[A-Za-z0-9]+):(\d+)(?:[-–—](\d+))?(?![\w])/g;
const PROSE_FIELDS = Object.freeze(['title', 'detail', 'recommended_action', 'brief_ref']);

function checkProseCitations(text, where, lineCount, errors, seen) {
  if (typeof text !== 'string' || text === '') return;
  PROSE_CITE.lastIndex = 0;
  let m;
  while ((m = PROSE_CITE.exec(text)) !== null) {
    const line = Math.max(Number(m[2]), m[3] === undefined ? 0 : Number(m[3]));
    if (line <= 0) continue;
    const n = lineCount(m[1]);
    if (n === null || line <= n) continue;
    const key = `${where}\u0000${m[0]}`;
    if (seen.has(key)) continue;
    seen.add(key);
    errors.push(issue(
      'FINDING_PROSE_LINE_OUT_OF_RANGE',
      `${where} cites ${m[0]}, but ${m[1]} has ${n} lines`,
      'Cite the line number in the file itself (Read it), not the line in the diff or patch. A line that is no ' +
        'longer in the file is cited with its revision, `<rev>:<path>:<line>` (`v6.2.0:hooks/x.mjs:120`), which ' +
        'is not judged against today\'s file.',
    ));
  }
}

function checkFindingProse(finding, index, lineCount, errors, seen) {
  if (!finding || typeof finding !== 'object') return;
  for (const field of PROSE_FIELDS) {
    checkProseCitations(finding[field], `findings[${index}].${field}`, lineCount, errors, seen);
  }
}

/**
 * @param {unknown} payload
 * @param {{ repoRoot?: string }} [opts] - with a repo root, cited lines are checked against the files:
 *   each finding's `file:line`, and every `path:line` in its prose fields
 */
export function validateFindings(payload, opts = {}) {
  return validatePayload(payload, opts.repoRoot ? lineCounter(opts.repoRoot) : null);
}

// One line counter for one validation: the JSON block and the prose around it share it.
function validatePayload(payload, lineCount) {
  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}`));
  }

  const errors = [];
  const warnings = [];

  if (typeof payload.reviewer !== 'string' || payload.reviewer.length === 0) {
    warnings.push(issue('FINDINGS_MISSING_REVIEWER', 'Reviewer output should carry a non-empty "reviewer" name'));
  }

  if (!Array.isArray(payload.findings)) {
    errors.push(issue('FINDINGS_NOT_ARRAY', `Field "findings" must be an array, got ${typeof payload.findings}`));
    return { valid: false, errors, warnings, parsed: payload };
  }

  const seen = new Set();
  for (let i = 0; i < payload.findings.length; i++) {
    validateFinding(payload.findings[i], i, errors);
    if (lineCount) {
      checkLineRange(payload.findings[i], i, lineCount, errors);
      checkFindingProse(payload.findings[i], i, lineCount, errors, seen);
    }
  }

  return { valid: errors.length === 0, errors, warnings, parsed: payload };
}

/**
 * Validate a reviewer's raw output text: extract the last json fence, parse it,
 * then schema-validate. Parse-stage failures get stable codes so they flow
 * through the same bounded re-ask path as schema failures.
 * @param {string} rawText
 * @returns {import('../util/result.mjs').Result}
 */
export function validateReviewerOutput(rawText, opts = {}) {
  const block = extractFindingsBlock(rawText);
  if (block === null) {
    return fail(issue(
      'FINDINGS_NO_JSON_BLOCK',
      'No trailing fenced ```json block found in reviewer output',
      'Reviewers must end their output with a single ```json findings block.',
    ));
  }
  let parsed;
  try {
    parsed = JSON.parse(block);
  } catch (e) {
    return fail(issue('FINDINGS_PARSE_ERROR', `Reviewer JSON block did not parse: ${e.message}`));
  }
  const lineCount = opts.repoRoot ? lineCounter(opts.repoRoot) : null;
  const r = validatePayload(parsed, lineCount);
  if (!lineCount) return r;
  // The prose around the block (the text outside the LAST json fence; the
  // block itself was checked field by field above) is a WARNING, not an error:
  // the bounded re-ask asks for the JSON block only, which cannot correct that
  // prose, and nothing downstream reads it — an error there could only stop a
  // review over text no one uses (independent review, 2026-10-08).
  const at = rawText.lastIndexOf(block);
  const found = [];
  checkProseCitations(rawText.slice(0, at) + rawText.slice(at + block.length), 'reviewer prose', lineCount, found, new Set());
  return found.length === 0 ? r : { ...r, warnings: [...r.warnings, ...found] };
}

// ---- CLI shim ----------------------------------------------------------------

function isDir(p) {
  try { return statSync(p).isDirectory(); } catch { return false; }
}

if (isMain(import.meta.url)) {
  const args = process.argv.slice(2);
  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);
  // A missing value, a flag in its place, or a root that is not a directory
  // would switch the line check off without a word: a usage error instead.
  const badRoot = rootIdx >= 0 && (!repoRoot || repoRoot.startsWith('--') || !isDir(repoRoot));
  if (!filePath || badRoot) {
    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'), { repoRoot });
  if (args.includes('--json')) {
    process.stdout.write(JSON.stringify({ valid: r.valid, errors: r.errors, warnings: r.warnings }, null, 2) + '\n');
  } else {
    process.stdout.write(`findings-schema: ${r.valid ? 'PASS' : 'FAIL'} ${filePath}\n`);
    for (const e of r.errors) process.stderr.write(`  ERROR  [${e.code}] ${e.message}\n`);
    for (const w of r.warnings) process.stderr.write(`  WARN   [${w.code}] ${w.message}\n`);
  }
  process.exit(r.valid ? 0 : 1);
}