From b149538d43c98583cc3cc7f4e040f7af32cc35e5 Mon Sep 17 00:00:00 2001 From: Kjell Tore Guttormsen Date: Thu, 18 Jun 2026 14:06:07 +0200 Subject: [PATCH] =?UTF-8?q?feat(voyage):=20S9=20=E2=80=94=20NW1=20reviewer?= =?UTF-8?q?-output=20schema=20contract=20(TDD,=20ungated)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codify the /trekreview Phase 5 reviewer JSON contract as a validated schema so main validates each reviewer's output instead of merely JSON.parse-ing it, and re-asks on schema failure (not just parse failure). Retires the fragility at trekreview.md:202-204. No Workflow dependency (S8 tier 1, ships regardless). New lib/review/findings-schema.mjs (3-layer Content -> Raw-text -> CLI shim, reusing result.mjs error-shape + rule-catalogue RULE_KEYS/SEVERITY_VALUES): - validateFindings(payload): hard errors on load-bearing fields the dedup triplet + verdict depend on — file, rule_key (in catalogue), severity (enum), line (integer >= 0); accumulates all errors; per-finding location. - extractFindingsBlock(text): last fenced ```json block (the :202 contract). - validateReviewerOutput(text): extract + parse + schema, unifying parse and schema failures under one bounded re-ask path. Stable codes: FINDINGS_NOT_OBJECT/_NOT_ARRAY/_NO_JSON_BLOCK/_PARSE_ERROR (top), FINDING_MISSING_FILE/_MISSING_RULE_KEY/_UNKNOWN_RULE_KEY/_BAD_SEVERITY/_BAD_LINE (per-finding). Descriptive fields + unknown keys tolerated (forward-compat). Phase 5 prose: replace "parse last json block; on parse error re-emit" with "validate against findings-schema; on failure re-ask conforming JSON, bounded N=2; never feed unvalidated findings to the coordinator". TDD: 27 failing tests first, then minimal code to pass. Suite 606 -> 647 (645 pass / 2 skip / 0 fail). claude plugin validate clean (only the known root-CLAUDE.md warning). Plan: docs/W1-narrow-wins-plan.md S9. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01LqBYc8Ltrk7LipyJmGxXiB --- commands/trekreview.md | 22 ++- lib/review/findings-schema.mjs | 175 +++++++++++++++++++++ tests/lib/findings-schema.test.mjs | 244 +++++++++++++++++++++++++++++ 3 files changed, 438 insertions(+), 3 deletions(-) create mode 100644 lib/review/findings-schema.mjs create mode 100644 tests/lib/findings-schema.test.mjs diff --git a/commands/trekreview.md b/commands/trekreview.md index 4fd24b3..288072d 100644 --- a/commands/trekreview.md +++ b/commands/trekreview.md @@ -199,9 +199,25 @@ Each reviewer prompt includes: - **Brief path** — `{brief_path}` (read on demand; do not inline). - **Rule catalogue** — reference to `lib/review/rule-catalogue.mjs`. -Collect each reviewer's trailing JSON block (last fenced `json` block in -their output). Parse with `JSON.parse()`. On parse error, ask the agent -to re-emit the JSON only. +Collect each reviewer's trailing JSON block and **validate it against the +reviewer-output schema** rather than merely parsing it. Run: + +```bash +node ${CLAUDE_PLUGIN_ROOT}/lib/review/findings-schema.mjs --json +``` + +`validateReviewerOutput` in `lib/review/findings-schema.mjs` extracts the +last fenced `json` block, parses it, and schema-checks every finding +(load-bearing fields: `file`, `rule_key` ∈ catalogue, `severity` ∈ enum, +`line` integer ≥ 0). Parse failure and schema failure surface through the +same stable error codes (`FINDINGS_NO_JSON_BLOCK`, `FINDINGS_PARSE_ERROR`, +`FINDING_*`). + +On any failure, re-ask **that reviewer** to re-emit a conforming JSON +block only — quote the reported error codes/locations so the fix is +targeted. **Bounded retries: N=2.** If the output still fails after 2 +re-asks, stop and report which reviewer produced non-conforming output; +do not feed unvalidated findings to the coordinator. In `quick` mode, launch only `code-correctness-reviewer`. The Executive Summary will note the brief-conformance pass was skipped. diff --git a/lib/review/findings-schema.mjs b/lib/review/findings-schema.mjs new file mode 100644 index 0000000..fb09c72 --- /dev/null +++ b/lib/review/findings-schema.mjs @@ -0,0 +1,175 @@ +// 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": "", "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 } from 'node:fs'; +import { issue, fail } from '../util/result.mjs'; +import { RULE_KEYS, SEVERITY_VALUES } from './rule-catalogue.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. +const JSON_FENCE_GLOBAL = /```json[ \t]*\r?\n([\s\S]*?)```/gi; + +/** + * 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} + */ +export function validateFindings(payload) { + 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 }; + } + + for (let i = 0; i < payload.findings.length; i++) { + validateFinding(payload.findings[i], i, errors); + } + + 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) { + 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}`)); + } + return validateFindings(parsed); +} + +// ---- CLI shim ---------------------------------------------------------------- + +if (import.meta.url === `file://${process.argv[1]}`) { + const args = process.argv.slice(2); + const filePath = args.find((a) => !a.startsWith('--')); + if (!filePath) { + process.stderr.write('Usage: findings-schema.mjs [--json] \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')); + 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); +} diff --git a/tests/lib/findings-schema.test.mjs b/tests/lib/findings-schema.test.mjs new file mode 100644 index 0000000..e94a7c6 --- /dev/null +++ b/tests/lib/findings-schema.test.mjs @@ -0,0 +1,244 @@ +import { test } from 'node:test'; +import { strict as assert } from 'node:assert'; +import { + validateFindings, + extractFindingsBlock, + validateReviewerOutput, + FINDING_REQUIRED_FIELDS, +} from '../../lib/review/findings-schema.mjs'; + +// ---- helpers ---------------------------------------------------------------- + +function validFinding(over = {}) { + return { + id: '0123456789abcdef0123456789abcdef01234567', + severity: 'BLOCKER', + rule_key: 'UNIMPLEMENTED_CRITERION', + file: 'lib/foo.mjs', + line: 0, + brief_ref: 'SC2 — exact quoted criterion text', + title: 'Short imperative title', + detail: 'Multi-sentence explanation citing concrete diff evidence', + recommended_action: 'Imperative, single-step recommendation', + ...over, + }; +} + +function validPayload(findings = [validFinding()]) { + return { reviewer: 'brief-conformance-reviewer', findings }; +} + +function codes(result) { + return result.errors.map((e) => e.code); +} + +// ---- exports ---------------------------------------------------------------- + +test('module exports the validator surface', () => { + assert.equal(typeof validateFindings, 'function'); + assert.equal(typeof extractFindingsBlock, 'function'); + assert.equal(typeof validateReviewerOutput, 'function'); + assert.ok(Array.isArray(FINDING_REQUIRED_FIELDS)); + for (const f of ['severity', 'rule_key', 'file', 'line']) { + assert.ok(FINDING_REQUIRED_FIELDS.includes(f), `${f} should be a required field`); + } +}); + +// ---- validateFindings: happy path ------------------------------------------ + +test('validateFindings — accepts a well-formed findings array', () => { + const r = validateFindings(validPayload()); + assert.equal(r.valid, true, JSON.stringify(r.errors)); + assert.deepEqual(r.errors, []); +}); + +test('validateFindings — accepts both reviewer example shapes', () => { + const conformance = validateFindings(validPayload([validFinding()])); + const correctness = validateFindings({ + reviewer: 'code-correctness-reviewer', + findings: [validFinding({ rule_key: 'SECURITY_INJECTION', file: 'lib/exec.mjs', line: 23 })], + }); + assert.equal(conformance.valid, true); + assert.equal(correctness.valid, true); +}); + +test('validateFindings — accepts an empty findings array', () => { + const r = validateFindings(validPayload([])); + assert.equal(r.valid, true); +}); + +test('validateFindings — accepts line: 0 (file-scoped finding)', () => { + const r = validateFindings(validPayload([validFinding({ line: 0 })])); + assert.equal(r.valid, true); +}); + +test('validateFindings — tolerates unknown top-level keys (forward-compat)', () => { + const payload = { ...validPayload(), future_field: 'whatever' }; + const r = validateFindings(payload); + assert.equal(r.valid, true); +}); + +// ---- validateFindings: the four named malformations ------------------------ + +test('validateFindings — rejects missing rule_key → FINDING_MISSING_RULE_KEY', () => { + const f = validFinding(); + delete f.rule_key; + const r = validateFindings(validPayload([f])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_MISSING_RULE_KEY'), codes(r).join(',')); +}); + +test('validateFindings — rejects unknown rule_key → FINDING_UNKNOWN_RULE_KEY', () => { + const r = validateFindings(validPayload([validFinding({ rule_key: 'NOT_A_REAL_RULE' })])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_UNKNOWN_RULE_KEY'), codes(r).join(',')); +}); + +test('validateFindings — rejects bad severity enum → FINDING_BAD_SEVERITY', () => { + const r = validateFindings(validPayload([validFinding({ severity: 'CRITICAL' })])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_BAD_SEVERITY'), codes(r).join(',')); +}); + +test('validateFindings — rejects missing severity → FINDING_BAD_SEVERITY', () => { + const f = validFinding(); + delete f.severity; + const r = validateFindings(validPayload([f])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_BAD_SEVERITY')); +}); + +test('validateFindings — rejects non-numeric line → FINDING_BAD_LINE', () => { + const r = validateFindings(validPayload([validFinding({ line: '23' })])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_BAD_LINE'), codes(r).join(',')); +}); + +test('validateFindings — rejects non-integer line → FINDING_BAD_LINE', () => { + const r = validateFindings(validPayload([validFinding({ line: 23.5 })])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_BAD_LINE')); +}); + +test('validateFindings — rejects negative line → FINDING_BAD_LINE', () => { + const r = validateFindings(validPayload([validFinding({ line: -1 })])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_BAD_LINE')); +}); + +test('validateFindings — rejects missing file → FINDING_MISSING_FILE', () => { + const f = validFinding(); + delete f.file; + const r = validateFindings(validPayload([f])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_MISSING_FILE'), codes(r).join(',')); +}); + +test('validateFindings — rejects empty-string file → FINDING_MISSING_FILE', () => { + const r = validateFindings(validPayload([validFinding({ file: '' })])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_MISSING_FILE')); +}); + +// ---- validateFindings: top-level shape ------------------------------------- + +test('validateFindings — rejects non-object payload → FINDINGS_NOT_OBJECT', () => { + for (const bad of [null, undefined, 42, 'x', []]) { + const r = validateFindings(bad); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDINGS_NOT_OBJECT'), `${JSON.stringify(bad)} → ${codes(r)}`); + } +}); + +test('validateFindings — rejects missing/non-array findings → FINDINGS_NOT_ARRAY', () => { + assert.ok(codes(validateFindings({ reviewer: 'x' })).includes('FINDINGS_NOT_ARRAY')); + assert.ok(codes(validateFindings({ reviewer: 'x', findings: {} })).includes('FINDINGS_NOT_ARRAY')); +}); + +test('validateFindings — rejects non-object finding element → FINDING_NOT_OBJECT', () => { + const r = validateFindings(validPayload(['not-an-object'])); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_NOT_OBJECT')); +}); + +test('validateFindings — missing reviewer is a non-blocking warning', () => { + const r = validateFindings({ findings: [] }); + assert.equal(r.valid, true); + assert.ok(r.warnings.some((w) => w.code === 'FINDINGS_MISSING_REVIEWER')); +}); + +// ---- validateFindings: error shape + accumulation -------------------------- + +test('validateFindings — every error has stable {code, message} + per-finding location', () => { + const r = validateFindings(validPayload([validFinding({ file: '', severity: 'NOPE' })])); + assert.equal(r.valid, false); + for (const e of r.errors) { + assert.ok(typeof e.code === 'string' && e.code.length > 0); + assert.ok(typeof e.message === 'string' && e.message.length > 0); + assert.ok(typeof e.location === 'string' && e.location.includes('findings[0]')); + } +}); + +test('validateFindings — accumulates errors across multiple bad findings', () => { + const r = validateFindings(validPayload([ + validFinding({ rule_key: undefined }), + validFinding({ line: 'x' }), + ])); + assert.equal(r.valid, false); + assert.ok(r.errors.some((e) => e.code === 'FINDING_MISSING_RULE_KEY' && e.location.includes('[0]'))); + assert.ok(r.errors.some((e) => e.code === 'FINDING_BAD_LINE' && e.location.includes('[1]'))); +}); + +// ---- extractFindingsBlock --------------------------------------------------- + +test('extractFindingsBlock — extracts the LAST json fence', () => { + const text = [ + '## Prose', + '```json', + '{"reviewer":"early","findings":[]}', + '```', + 'more prose', + '```json', + '{"reviewer":"last","findings":[]}', + '```', + ].join('\n'); + const block = extractFindingsBlock(text); + assert.ok(block.includes('"last"')); + assert.ok(!block.includes('"early"')); +}); + +test('extractFindingsBlock — returns null when no json fence present', () => { + assert.equal(extractFindingsBlock('just prose, no fence'), null); + assert.equal(extractFindingsBlock(''), null); + assert.equal(extractFindingsBlock(123), null); +}); + +// ---- validateReviewerOutput (raw text → extract → parse → schema) ----------- + +test('validateReviewerOutput — valid raw output round-trips to valid', () => { + const raw = `## Review\n\nprose here\n\n\`\`\`json\n${JSON.stringify(validPayload(), null, 2)}\n\`\`\`\n`; + const r = validateReviewerOutput(raw); + assert.equal(r.valid, true, JSON.stringify(r.errors)); +}); + +test('validateReviewerOutput — no json block → FINDINGS_NO_JSON_BLOCK', () => { + const r = validateReviewerOutput('prose with no fenced json'); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDINGS_NO_JSON_BLOCK')); +}); + +test('validateReviewerOutput — malformed JSON → FINDINGS_PARSE_ERROR', () => { + const raw = '```json\n{"reviewer":"x","findings":[],}\n```'; // trailing comma + const r = validateReviewerOutput(raw); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDINGS_PARSE_ERROR')); +}); + +test('validateReviewerOutput — well-formed JSON but schema-invalid surfaces schema code', () => { + const bad = { reviewer: 'x', findings: [{ severity: 'BLOCKER', file: 'a.mjs', line: 1 }] }; // no rule_key + const raw = `\`\`\`json\n${JSON.stringify(bad)}\n\`\`\``; + const r = validateReviewerOutput(raw); + assert.equal(r.valid, false); + assert.ok(codes(r).includes('FINDING_MISSING_RULE_KEY'), + 'parse and schema validation must flow through the same result'); +});