voyage/lib/validators/review-validator.mjs
Kjell Tore Guttormsen a10018f9d3
fix: round 8 of the release review of 6.3.0, for 6.3.1
Round 8 (0f400d3..29b4ac9, BLOCK: 1 BLOCKER, 12 MAJOR, 1 MINOR). Each fix
began with a test that failed on 29b4ac9 (13 tests, all red there); suite
2221 (2219/0/2).

- BLOCKER: a backticked check whose trailing comment holds a quote, $,
  backtick or backslash is refused, not dropped. The comment was never
  screened, while the executor hands the Verify span as written to red
  proof, the test lock and the done line inside --cmd "...", where a $(...)
  runs and a quote ends the argument. trekplan's text and the reviewer's
  NOT RUN table say so.
- A span that is only a comment is not-a-command, not a placeholder. The
  dropComment text says the allowlist still refuses a word with a # in it.
- The criteria runner, comparison-gate and end-state-gate use isMain: they
  did nothing and exited 0 under a symlinked path. The test now refuses any
  process.argv[1] comparison outside lib/util/is-main.mjs.
- Files: in a field with backticked paths, a plain word counts only before
  the entry's description (a dash, or a colon after the path); a name with
  no slash only first in its entry or after and/or/+/&. "mocks
  fs.readFileSync", "runs on Node.js 22.x" or "no change to package.json"
  no longer make a tests-only step look like it builds something.
  Lowercase makefile, rakefile and justfile are build files. Measured on the
  520 Files: fields in 48 plans of the marketplace and the test repo:
  3 fields read differently (two notes no longer read as files, one
  duplicate name dropped), tests-only changed in 0.
- The test lock compares --cmd without its trailing comment, and the test
  lock and the done line read a stop with stoppedBy, the one reading.
  red-proof drops its second reading, spawnError.
- release-check reads a relative --review in --repo or, when only that one
  exists, from the current directory as 6.3.0 did; two different files are
  exit 2, never guessed. The runbook says so.
- One trailing-json reader: a closing fence may be longer than three
  backticks, in the block reader and the verdict gate alike, and the review
  validator and the synthesis digest use it, so three backticks inside a
  JSON string no longer hide a BLOCKER from the remediation-step check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-11 10:43:56 +02:00

200 lines
7.8 KiB
JavaScript

// lib/validators/review-validator.mjs
// Validate trekreview frontmatter + body invariants.
// 3-layer pattern (Content → File → CLI shim) mirroring brief-validator.
//
// Schema is forward-compatible: unknown top-level frontmatter keys are
// tolerated silently. Strict-key checks are intentionally avoided so new
// optional fields (jf. the source_findings precedent) can be added without
// a review_version bump.
//
// K7 (ordre 5, review_version 1.1): `verdict: ALLOW | WARN | BLOCK` is REQUIRED.
// Strict (write-time) mode rejects a review without it; soft (read-time) mode
// warns, so a 1.0 review can still seed a plan (N-1 window, Handover 6). The
// gate that decides on the verdict is lib/review/verdict-gate.mjs, which never
// reads a missing verdict as ALLOW.
import { readFileSync, existsSync } from 'node:fs';
import { parseDocument } from '../util/frontmatter.mjs';
import { issue, ok, fail } from '../util/result.mjs';
import { isMain } from '../util/is-main.mjs';
import { extractFindingsBlock } from '../review/findings-schema.mjs';
export const REVIEW_REQUIRED_FRONTMATTER = [
'type',
'review_version',
'task',
'slug',
'project_dir',
'brief_path',
'scope_sha_end',
'reviewed_files_count',
'findings',
];
export const REVIEW_BODY_SECTIONS = ['Executive Summary', 'Coverage', 'Remediation Summary'];
export const VERDICT_VALUES = Object.freeze(['ALLOW', 'WARN', 'BLOCK']);
const HEX_ID_RE = /^[0-9a-f]{40}$/;
/**
* IDs of BLOCKER findings in the trailing JSON block ([] when absent/unparseable).
* Read with the one block reader, so three backticks inside a JSON string do
* not end the block early and hide a BLOCKER (release review of 6.3.0, round 8).
*/
function blockerIdsFromBody(body) {
const last = extractFindingsBlock(body);
if (last === null) return [];
try {
const payload = JSON.parse(last);
return (Array.isArray(payload?.findings) ? payload.findings : [])
.filter((f) => f?.severity === 'BLOCKER' && typeof f.id === 'string')
.map((f) => f.id);
} catch {
return [];
}
}
/** Finding IDs named by `Source finding:` lines under `## Remediation Steps` step headings. */
function remediationStepSources(body) {
const start = body.search(/^##\s+Remediation Steps\b/m);
if (start === -1) return new Set();
const rest = body.slice(start).replace(/^##\s+Remediation Steps[^\n]*\n/, '');
const next = rest.search(/^##\s+/m);
const section = next === -1 ? rest : rest.slice(0, next);
const ids = new Set();
for (const step of section.split(/^###\s+Step\s+R\d+\b/m).slice(1)) {
const m = step.match(/Source finding:\**\s*`?([0-9a-f]{40})`?/);
if (m) ids.add(m[1]);
}
return ids;
}
export function validateReviewContent(text, opts = {}) {
const strict = opts.strict !== false;
const doc = parseDocument(text);
if (!doc.valid) return doc;
const fm = doc.parsed.frontmatter || {};
const body = doc.parsed.body || '';
const errors = [];
const warnings = [];
for (const k of REVIEW_REQUIRED_FRONTMATTER) {
if (!(k in fm)) {
errors.push(issue('REVIEW_MISSING_FIELD', `Required frontmatter field missing: ${k}`));
}
}
if (fm.type !== undefined && fm.type !== 'trekreview') {
errors.push(issue('REVIEW_WRONG_TYPE', `frontmatter.type must be "trekreview", got "${fm.type}"`));
}
if (fm.findings !== undefined) {
if (!Array.isArray(fm.findings)) {
errors.push(issue(
'REVIEW_BAD_FINDINGS_TYPE',
`Field "findings" must be an array of finding-IDs, got ${typeof fm.findings}`,
'Use block-style YAML: `findings:\\n - <id1>\\n - <id2>`',
));
} else {
for (let i = 0; i < fm.findings.length; i++) {
const id = fm.findings[i];
if (typeof id !== 'string' || !HEX_ID_RE.test(id)) {
errors.push(issue(
'REVIEW_BAD_FINDING_ID',
`findings[${i}] is not a 40-char hex ID: ${JSON.stringify(id)}`,
));
}
}
}
}
if (!('verdict' in fm) || fm.verdict === null || fm.verdict === '') {
const issueObj = issue(
'REVIEW_MISSING_VERDICT',
'Required frontmatter field missing: verdict (ALLOW | WARN | BLOCK)',
'Copy the coordinator verdict from the trailing JSON block into the frontmatter.',
);
if (strict) errors.push(issueObj);
else warnings.push(issueObj);
} else if (!VERDICT_VALUES.includes(fm.verdict)) {
errors.push(issue(
'REVIEW_BAD_VERDICT',
`frontmatter.verdict must be one of ${VERDICT_VALUES.join('|')}, got ${JSON.stringify(fm.verdict)}`,
));
}
// Every BLOCKER is written as a numbered remediation step that Handover 6 can
// append to a plan unchanged: `### Step R<n>: …` with `Source finding: <id>`.
// Borrowed: review gaps become appended, numbered tasks with a source-ref,
// never rewriting earlier ones (Spec Kit `templates/commands/converge.md:156-163`, MIT).
// 1.1+ only (N-1 window): a 1.0 review never carried the section.
const v = typeof fm.review_version === 'string' ? fm.review_version.match(/^(\d+)\.(\d+)$/) : null;
const atLeast11 = v && (Number(v[1]) > 1 || (Number(v[1]) === 1 && Number(v[2]) >= 1));
const blockerIds = atLeast11 ? blockerIdsFromBody(body) : [];
if (blockerIds.length > 0) {
const steps = remediationStepSources(body);
for (const id of blockerIds) {
if (!steps.has(id)) {
const issueObj = issue(
'REVIEW_MISSING_REMEDIATION_STEP',
`BLOCKER ${id} has no "### Step R<n>" with "Source finding: ${id}" under ## Remediation Steps`,
'Write one numbered remediation step per BLOCKER (Handover 6).',
);
if (strict) errors.push(issueObj);
else warnings.push(issueObj);
}
}
}
// 1.1+: what the coordinator set aside is listed, never left out (ordre 5).
if (atLeast11 && !/^##\s+Declined to Judge\b/m.test(body)) {
const issueObj = issue('REVIEW_MISSING_SECTION', 'Required body section missing: ## Declined to Judge (review_version ≥ 1.1)');
if (strict) errors.push(issueObj);
else warnings.push(issueObj);
}
for (const section of REVIEW_BODY_SECTIONS) {
const re = new RegExp(`^##\\s+${section}\\b`, 'm');
if (!re.test(body)) {
const issueObj = issue('REVIEW_MISSING_SECTION', `Required body section missing: ## ${section}`);
if (strict) errors.push(issueObj);
else warnings.push(issueObj);
}
}
if (typeof fm.review_version === 'string') {
const m = fm.review_version.match(/^(\d+)\.(\d+)$/);
if (!m) {
warnings.push(issue('REVIEW_VERSION_FORMAT', `review_version "${fm.review_version}" not in N.M form`));
}
}
return { valid: errors.length === 0, errors, warnings, parsed: { frontmatter: fm, body } };
}
export function validateReview(filePath, opts = {}) {
if (!existsSync(filePath)) return fail(issue('REVIEW_NOT_FOUND', `File not found: ${filePath}`));
let text;
try { text = readFileSync(filePath, 'utf-8'); }
catch (e) { return fail(issue('REVIEW_READ_ERROR', `Cannot read ${filePath}: ${e.message}`)); }
const r = validateReviewContent(text, opts);
return { ...r, parsed: { ...r.parsed, filePath } };
}
if (isMain(import.meta.url)) {
const args = process.argv.slice(2);
const strict = !args.includes('--soft');
const filePath = args.find(a => !a.startsWith('--'));
if (!filePath) {
process.stderr.write('Usage: review-validator.mjs [--soft] [--json] <review.md>\n');
process.exit(2);
}
const r = validateReview(filePath, { strict });
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(`review-validator: ${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);
}