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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqBYc8Ltrk7LipyJmGxXiB
244 lines
9.4 KiB
JavaScript
244 lines
9.4 KiB
JavaScript
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');
|
|
});
|