diff --git a/docs/T2-bakeoff-results.md b/docs/T2-bakeoff-results.md new file mode 100644 index 0000000..7729cec --- /dev/null +++ b/docs/T2-bakeoff-results.md @@ -0,0 +1,90 @@ +# T2 / NW2 — prose-vs-Workflow bake-off results + +**Status (S10):** Build complete + **smoke run (1 run/arm) done**. Full ≥3-runs/arm +measurement is **pending operator go/no-go** (operator posture: "build + smoke, then pause"). +**This document records the smoke; it is NOT the full T2 §5 verdict.** + +Resolves: the build + de-risk half of `docs/W1-narrow-wins-plan.md §S10`. + +--- + +## Setup + +| Item | Value | +|------|-------| +| Fixture | `tests/fixtures/bakeoff/` — real diff of commit `b149538` (NW1) + brief reconstructed from `docs/W1-narrow-wins-plan.md §S9` | +| Delivered diff | 3 files, +438/-3 (`commands/trekreview.md`, `lib/review/findings-schema.mjs`, `tests/lib/findings-schema.test.mjs`) | +| Triage | all 3 files `summary-only` (deterministic Phase-4 classifier) | +| Model | `opus` (reviewer + coordinator agent defaults) | +| Arm A (prose) | reviewers spawned via Agent tool, prose trailing-JSON contract (validated by NW1 `findings-schema`) | +| Arm B (Workflow) | `scripts/trekreview-armB.workflow.mjs` via Workflow tool: `parallel([conformance, correctness])` schema-forced → JS dedup-by-triplet → `agent(review-coordinator)` verdict schema | +| Fidelity metric | `lib/review/fidelity-diff.mjs` `fidelityDiffStructured` (verdict + jaccard over `(file,line,rule_key)`-IDs + severity/rule cross-check) | + +## Smoke results (single run per arm) + +| | Arm A (prose) | Arm B (Workflow) | +|---|---|---| +| Reviewers run | 2 | 2 | +| Raw findings | 0 | 1 | +| After coordinator | 0 (coordinator moot — 0 findings) | 0 (coordinator dropped the 1 finding) | +| **Verdict** | **ALLOW** | **ALLOW** | +| Agents | 2 | 3 (2 reviewers + coordinator) | +| Subagent tokens | ~72.3k (34.3k + 38.0k; no coordinator) | ~100.9k (incl. coordinator) | +| Wall-time | ~70 s (parallel reviewers) | ~157 s (full pipeline) | + +### PRIMARY metric — output fidelity: **EQUIVALENT** + +``` +fidelityDiffStructured(ArmA, ArmB) = + { verdictMatch: true, jaccard: 1, countA: 0, countB: 0, + severityMismatches: [], ruleKeyMismatches: [], equivalent: true } +``` + +⚠ **Caveat — thin finding surface.** Both arms returned **0 final findings** on +this clean, TDD'd fixture, so fidelity is confirmed only at the **verdict** level +(ALLOW ≡ ALLOW); the finding-*set* fidelity is trivially equal at zero and was +**not stressed**. A reviewer-level divergence *did* appear (Arm B raised 1 raw +finding, its coordinator filtered it; Arm A raised 0) — masked at the verdict +level. Quantifying that divergence is exactly what the full run on a +richer-finding-surface fixture must do. + +### Secondary metrics (smoke, single-run — not medians) + +- **JSON-robustness (the F2 win):** Arm B's reviewers were **schema-forced** + (StructuredOutput) — typed findings, zero `JSON.parse`; the 1 raw finding + + the coordinator verdict both conformed with no re-ask. Arm A's trailing-JSON + validated clean via NW1 `findings-schema`. Win demonstrated structurally; the + parse-error/re-ask delta needs a fixture that actually provokes malformed JSON. +- **Classifier interference: 0.** Arm B's 2-agent fan-out + coordinator (3 agents) + ran with no denied/missing spawns. Confirms S8 F4 for trekreview's small + fan-out under the default mode. (`auto`/`bypass` still to be checked in the full run.) +- **Token cost:** preliminary and **not yet comparable** — Arm B ran a coordinator + (on its 1 finding) that Arm A did not. Single run; no medians. +- **Control/visibility:** Arm B runs in the background; intermediate findings are + visible in the workflow transcript + `/workflows`. Operator-gate (the review.md + write) is unaffected — both arms return structured `{verdict, findings}` and + Phase 7 rendering stays shared/prose. + +## Smoke verdict + +**SMOKE PASS — machinery validated.** Arm B (Workflow substrate) runs the full +Phase 5–6 pipeline end-to-end, fidelity-**equivalent** to Arm A at the verdict +level, with **zero classifier interference**. The build is sound: NW1 schema, +fidelity-diff, fixture, and the Arm B port all work together. + +This is **not** the T2 §5 POSITIVE/NEGATIVE verdict — that needs the full +≥3-runs/arm measurement with a finding-rich fixture. + +## Go / no-go recommendation (operator decides) + +**Recommend: proceed to the full ≥3-runs/arm run (S10 part B)** with two changes: +1. **Use a richer-finding-surface fixture** (a larger real voyage commit, or seed + the fixture with a few genuine issues) so finding-*set* fidelity is actually + stressed — the smoke only proved verdict fidelity at 0 findings. +2. **Match the arms' coordinator path** (run Arm A's coordinator too, even at low + finding counts) so the token/wall-time comparison is apples-to-apples, and + add the `auto`/`bypass` classifier-interference check (F4). + +If the operator prefers, S11 can instead record "port built + smoke-validated; +full measurement deferred" and integrate behind the opt-in `--workflow` flag on +the smoke evidence alone — weaker, but the substrate is demonstrably functional. diff --git a/lib/review/fidelity-diff.mjs b/lib/review/fidelity-diff.mjs new file mode 100644 index 0000000..aa48c90 --- /dev/null +++ b/lib/review/fidelity-diff.mjs @@ -0,0 +1,173 @@ +// lib/review/fidelity-diff.mjs +// Fidelity comparison of two review.md artifacts — the PRIMARY metric of the +// NW2 (S10) prose-vs-Workflow bake-off (T2 §5). +// +// A substrate swap (prose Arm A → Workflow Arm B) passes the gate only if it +// produces a fidelity-equivalent review.md: SAME verdict and an equivalent +// finding set (IDs / severities / rule_keys). This module computes that diff +// from two rendered review.md texts, reusing the determinism-pipeline +// primitives (jaccard over finding-IDs + frontmatter parse + the NW1 trailing- +// block extractor). +// +// Pure JS, zero deps beyond existing lib modules — unit-testable without any +// live LLM run. + +import { parseDocument } from '../util/frontmatter.mjs'; +import { jaccardSimilarity } from '../parsers/jaccard.mjs'; +import { computeFindingId } from '../parsers/finding-id.mjs'; +import { extractFindingsBlock } from './findings-schema.mjs'; + +export const DEFAULT_JACCARD_TOLERANCE = 0.7; + +/** + * Parse a rendered review.md into its comparable shape. + * - verdict + finding-ID list come from frontmatter (the validated contract). + * - per-finding severity / rule_key / file / line come from the trailing JSON + * block. Both the `rule_key` (real reviewer output) and `rule` (fixtures) + * keys are accepted. + * @param {string} text + * @returns {{ verdict: string|null, findingIds: string[], + * details: Array<{id, severity, rule_key, file, line}> }} + */ +export function parseReviewArtifact(text) { + const doc = parseDocument(text); + const fm = (doc.valid && doc.parsed && doc.parsed.frontmatter) || {}; + const verdict = typeof fm.verdict === 'string' ? fm.verdict : null; + const findingIds = Array.isArray(fm.findings) ? fm.findings.filter((x) => typeof x === 'string') : []; + + let details = []; + const block = extractFindingsBlock(text); + if (block !== null) { + try { + const parsed = JSON.parse(block); + const arr = Array.isArray(parsed) ? parsed : (Array.isArray(parsed.findings) ? parsed.findings : []); + details = arr.map((f) => ({ + id: f.id ?? null, + severity: f.severity ?? null, + rule_key: f.rule_key ?? f.rule ?? null, + file: f.file ?? null, + line: f.line ?? null, + })); + } catch { + details = []; + } + } + + return { verdict, findingIds, details }; +} + +function detailMap(details) { + const m = new Map(); + for (const d of details) { + if (d.id) m.set(d.id, d); + } + return m; +} + +/** + * Core comparison over two normalized artifacts: {verdict, findingIds, details}. + * Shared by fidelityDiff (review.md text) and fidelityDiffStructured (arm output). + */ +function compareArtifacts(a, b, opts = {}) { + const tol = typeof opts.jaccardTolerance === 'number' ? opts.jaccardTolerance : DEFAULT_JACCARD_TOLERANCE; + const verdictMatch = a.verdict === b.verdict; + const jaccard = jaccardSimilarity(a.findingIds, b.findingIds); + + // Cross-check severity + rule_key on findings present in BOTH arms. + const mapA = detailMap(a.details); + const mapB = detailMap(b.details); + const severityMismatches = []; + const ruleKeyMismatches = []; + for (const [id, da] of mapA) { + const db = mapB.get(id); + if (!db) continue; + if (da.severity !== db.severity) severityMismatches.push({ id, a: da.severity, b: db.severity }); + if (da.rule_key !== db.rule_key) ruleKeyMismatches.push({ id, a: da.rule_key, b: db.rule_key }); + } + + const equivalent = + verdictMatch && + jaccard >= tol && + severityMismatches.length === 0 && + ruleKeyMismatches.length === 0; + + return { + verdictA: a.verdict, + verdictB: b.verdict, + verdictMatch, + jaccard, + countA: a.findingIds.length, + countB: b.findingIds.length, + severityMismatches, + ruleKeyMismatches, + equivalent, + }; +} + +/** + * Compute the fidelity diff between two rendered review.md artifacts. + * @param {string} textA — baseline (Arm A — prose) + * @param {string} textB — candidate (Arm B — Workflow) + * @param {{ jaccardTolerance?: number }} [opts] + */ +export function fidelityDiff(textA, textB, opts = {}) { + return compareArtifacts(parseReviewArtifact(textA), parseReviewArtifact(textB), opts); +} + +/** + * Normalize a structured arm output ({verdict, findings:[{severity,rule_key, + * file,line}]}) into the comparable shape, recomputing canonical finding-IDs + * from the (file, line, rule_key) triplet. Findings missing file/rule_key are + * dropped from the ID set (they cannot dedupe), but counted is by valid IDs. + */ +export function normalizeArmOutput(arm) { + const verdict = arm && typeof arm.verdict === 'string' ? arm.verdict : null; + const findings = (arm && Array.isArray(arm.findings)) ? arm.findings : []; + const findingIds = []; + const details = []; + for (const f of findings) { + const file = f.file; + const rule_key = f.rule_key ?? f.rule ?? null; + const line = f.line; + let id = null; + if (typeof file === 'string' && file.length > 0 && rule_key && line !== null && line !== undefined) { + try { id = computeFindingId(file, line, rule_key); } catch { id = null; } + } + if (id) findingIds.push(id); + details.push({ id, severity: f.severity ?? null, rule_key, file: file ?? null, line: line ?? null }); + } + return { verdict, findingIds, details }; +} + +/** + * Fidelity diff between two structured arm outputs (the bake-off comparison — + * avoids rendering review.md for each run). + * @param {{verdict, findings}} armA + * @param {{verdict, findings}} armB + * @param {{ jaccardTolerance?: number }} [opts] + */ +export function fidelityDiffStructured(armA, armB, opts = {}) { + return compareArtifacts(normalizeArmOutput(armA), normalizeArmOutput(armB), opts); +} + +// ---- CLI shim ---------------------------------------------------------------- + +if (import.meta.url === `file://${process.argv[1]}`) { + const { readFileSync } = await import('node:fs'); + const args = process.argv.slice(2); + const files = args.filter((x) => !x.startsWith('--')); + if (files.length !== 2) { + process.stderr.write('Usage: fidelity-diff.mjs [--json] \n'); + process.exit(2); + } + const d = fidelityDiff(readFileSync(files[0], 'utf-8'), readFileSync(files[1], 'utf-8')); + if (args.includes('--json')) { + process.stdout.write(JSON.stringify(d, null, 2) + '\n'); + } else { + process.stdout.write(`fidelity-diff: ${d.equivalent ? 'EQUIVALENT' : 'DIVERGENT'}\n`); + process.stdout.write(` verdict: ${d.verdictA} vs ${d.verdictB} (match=${d.verdictMatch})\n`); + process.stdout.write(` jaccard: ${d.jaccard.toFixed(4)} (findings ${d.countA} vs ${d.countB})\n`); + process.stdout.write(` severity mismatches: ${d.severityMismatches.length}; rule_key mismatches: ${d.ruleKeyMismatches.length}\n`); + } + process.exit(d.equivalent ? 0 : 1); +} diff --git a/scripts/trekreview-armB.workflow.mjs b/scripts/trekreview-armB.workflow.mjs new file mode 100644 index 0000000..495a2ed Binary files /dev/null and b/scripts/trekreview-armB.workflow.mjs differ diff --git a/tests/fixtures/bakeoff/README.md b/tests/fixtures/bakeoff/README.md new file mode 100644 index 0000000..b05a1e6 --- /dev/null +++ b/tests/fixtures/bakeoff/README.md @@ -0,0 +1,50 @@ +# NW2 bake-off fixture (S10) + +Fixed, committable input for the prose-vs-Workflow `/trekreview` bake-off +(T2 §5). Both arms review **the same delivered diff against the same brief**, so +any difference in `review.md` is attributable to the orchestration substrate +(prose Arm A vs Workflow Arm B), not to the input. + +## What's here + +- `brief.md` — the contract. Reconstructed faithfully from the **real** NW1 + contract (`docs/W1-narrow-wins-plan.md §S9`): goal, SC1–SC4, NG1–NG3, + constraints, NFRs. +- `delivered.diff` — the **real** delivered diff of commit `b149538` + (`git diff b149538^ b149538`): the NW1 implementation + (`lib/review/findings-schema.mjs` + tests + `commands/trekreview.md` Phase 5). + 3 files, +438/-3. + +## Why this commit + +`b149538` (NW1/S9) is a genuine delivered-diff + brief pair already in this +repo's history — real code, real contract, fully self-contained (no external +deps), and small/cheap (ideal for the de-risking smoke run). It is not +finding-free (a strict correctness reviewer has surface to flag), so the +fidelity comparison is informative. A larger commit can be substituted for the +full ≥3-runs/arm measurement if more finding surface is wanted. + +## Triage map (deterministic, pinned) + +Per the Phase-4 path-pattern classifier, none of the 3 files match `skip` or +`deep-review` patterns → all `summary-only`: + +``` +commands/trekreview.md → summary-only +lib/review/findings-schema.mjs → summary-only +tests/lib/findings-schema.test.mjs → summary-only +``` + +## How it's consumed + +The bake-off pins Phases 1–4 (brief + diff + triage above) and passes them to +both arms: + +- **Arm A (prose):** current `/trekreview` Phase 5–6 behaviour. +- **Arm B (Workflow):** `scripts/trekreview-armB.workflow.mjs` via the Workflow + tool, `args = { brief, diff, triage }`. + +Each arm yields a structured `{verdict, findings}`; the PRIMARY metric is +`fidelityDiffStructured` (`lib/review/fidelity-diff.mjs`) — same verdict + +equivalent finding set (IDs / severities / rule_keys). +``` diff --git a/tests/fixtures/bakeoff/brief.md b/tests/fixtures/bakeoff/brief.md new file mode 100644 index 0000000..fa92e9c --- /dev/null +++ b/tests/fixtures/bakeoff/brief.md @@ -0,0 +1,78 @@ +--- +type: trekbrief +brief_version: "2.1" +slug: nw1-reviewer-output-schema +task: Codify the /trekreview Phase 5 reviewer-output JSON as a validated schema contract +research_topics: 0 +research_status: complete +brief_quality: ready +created: 2026-06-18 +--- + +# NW1 — reviewer-output schema contract + +## Intent + +`/trekreview` Phase 5 launches two reviewer agents (`brief-conformance-reviewer`, +`code-correctness-reviewer`) that each emit a trailing fenced `json` block of +findings. The main context used to "collect the last fenced json block and +`JSON.parse()` it; on parse error, ask the agent to re-emit" — a fragile +contract (`trekreview.md:202–204`) that catches *parse* failure but not +*schema* failure: a reviewer can emit valid JSON with a missing `rule_key`, a +bad `severity`, or a non-numeric `line`, and it flows unchecked into the +coordinator's dedup-by-`(file,line,rule_key)` triplet. + +## Goal + +Codify the reviewer-output JSON as a validated schema. Main validates each +reviewer's output against the schema (not merely parses it) and re-asks on +schema failure as well as parse failure, with bounded retries. The load-bearing +fields the downstream dedup triplet + verdict depend on — `file`, `rule_key`, +`severity`, `line` — are hard errors; descriptive fields and unknown keys are +tolerated (forward-compatible). No Workflow dependency — the substrate stays +prose (this is the ungated, ship-regardless win). + +## Success Criteria + +- **SC1** — A findings-schema validator accepts a well-formed findings array and + rejects each malformation (missing `rule_key`, unknown `rule_key`, bad + `severity` enum, non-numeric `line`, missing `file`) with a stable error code. +- **SC2** — `lib/review/findings-schema.mjs` implements the schema + validator, + reusing the `lib/util/result.mjs` issue/result shape and the + `lib/review/rule-catalogue.mjs` `RULE_KEYS` / `SEVERITY_VALUES`. +- **SC3** — `commands/trekreview.md` Phase 5 prose replaces the + `JSON.parse`-only contract with "validate each reviewer's JSON against the + findings schema; on schema failure re-ask for conforming JSON (bounded + retries N=2); never feed unvalidated findings to the coordinator." +- **SC4** — `node --test tests/lib/findings-schema.test.mjs` is green; the full + `node --test` suite still passes; `claude plugin validate` is clean (modulo the + pre-existing root-CLAUDE.md warning). + +## Non-Goals + +- **NG1** — Do NOT introduce a Workflow-tool dependency. NW1 is the prose-only + tier; the substrate stays prose. +- **NG2** — Do NOT change the `review.md` output schema or + `lib/validators/review-validator.mjs`. This is the *reviewer-output* contract + (Phase 5 input), not the review.md artifact contract (Phase 7 output). +- **NG3** — Do NOT touch NW2 (Workflow port) or NW3 (synthesis-agent) scope. + +## Constraints + +- Zero runtime dependencies — Node stdlib only (project invariant). +- 3-layer module pattern (Content → Raw-text → CLI shim) mirroring the other + `lib/` validators. +- TDD (Iron Law): failing tests first, then minimal code to pass. + +## Assumptions + +- Reviewers continue to emit the documented JSON shape (`{reviewer, findings:[…]}`) + with the per-finding fields shown in the agent specs. +- The 12-key rule catalogue and 4-tier severity enum are the contract surface + for `rule_key` / `severity` validation. + +## NFRs + +- The schema validator adds no measurable latency to Phase 5 (pure in-process + validation). +- Error codes are stable strings so a bounded re-ask can quote them. diff --git a/tests/fixtures/bakeoff/delivered.diff b/tests/fixtures/bakeoff/delivered.diff new file mode 100644 index 0000000..0587b17 --- /dev/null +++ b/tests/fixtures/bakeoff/delivered.diff @@ -0,0 +1,464 @@ +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'); ++}); diff --git a/tests/lib/fidelity-diff.test.mjs b/tests/lib/fidelity-diff.test.mjs new file mode 100644 index 0000000..2595977 --- /dev/null +++ b/tests/lib/fidelity-diff.test.mjs @@ -0,0 +1,165 @@ +import { test } from 'node:test'; +import { strict as assert } from 'node:assert'; +import { readFileSync } from 'node:fs'; +import { join, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { + parseReviewArtifact, + fidelityDiff, + fidelityDiffStructured, + normalizeArmOutput, +} from '../../lib/review/fidelity-diff.mjs'; +import { computeFindingId } from '../../lib/parsers/finding-id.mjs'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const ROOT = join(HERE, '..', '..'); + +function fixture(name) { + return readFileSync(join(ROOT, 'tests/fixtures/trekreview', name), 'utf-8'); +} + +const RUN_A = fixture('review-run-A.md'); +const RUN_B = fixture('review-run-B.md'); + +// ---- parseReviewArtifact ---------------------------------------------------- + +test('parseReviewArtifact — extracts verdict + finding IDs from frontmatter', () => { + const a = parseReviewArtifact(RUN_A); + assert.equal(a.verdict, 'WARN'); + assert.ok(Array.isArray(a.findingIds)); + assert.equal(a.findingIds.length, 5); + for (const id of a.findingIds) assert.match(id, /^[0-9a-f]{40}$/); +}); + +test('parseReviewArtifact — extracts per-finding details from trailing JSON', () => { + const a = parseReviewArtifact(RUN_A); + assert.ok(Array.isArray(a.details)); + assert.equal(a.details.length, 5); + const blocker = a.details.find((d) => d.severity === 'BLOCKER'); + assert.ok(blocker); + // fixtures use the key "rule"; real reviewer output uses "rule_key" — both accepted + assert.equal(blocker.rule_key, 'UNIMPLEMENTED_CRITERION'); + assert.equal(blocker.file, 'lib/handlers/login.mjs'); + assert.equal(blocker.line, 23); +}); + +test('parseReviewArtifact — tolerates a missing trailing JSON block (details = [])', () => { + const noJson = '---\ntype: trekreview\nverdict: ALLOW\nfindings: []\n---\n\n# Review\n\nNo findings.\n'; + const r = parseReviewArtifact(noJson); + assert.equal(r.verdict, 'ALLOW'); + assert.deepEqual(r.findingIds, []); + assert.deepEqual(r.details, []); +}); + +// ---- fidelityDiff ----------------------------------------------------------- + +test('fidelityDiff — identical artifact is fully equivalent (jaccard 1.0)', () => { + const d = fidelityDiff(RUN_A, RUN_A); + assert.equal(d.verdictMatch, true); + assert.equal(d.jaccard, 1); + assert.equal(d.equivalent, true); + assert.equal(d.severityMismatches.length, 0); + assert.equal(d.ruleKeyMismatches.length, 0); +}); + +test('fidelityDiff — A vs B (A ⊂ B): same verdict, jaccard 5/6, equivalent at default tolerance', () => { + const d = fidelityDiff(RUN_A, RUN_B); + assert.equal(d.verdictMatch, true); + assert.ok(Math.abs(d.jaccard - 5 / 6) < 1e-9, `jaccard=${d.jaccard}`); + assert.equal(d.equivalent, true); // default jaccardTolerance 0.7 +}); + +test('fidelityDiff — divergent verdict makes it non-equivalent even at jaccard 1.0', () => { + const a = parseReviewArtifact(RUN_A); + const flipped = RUN_A.replace('verdict: WARN', 'verdict: BLOCK'); + const d = fidelityDiff(RUN_A, flipped); + assert.equal(d.verdictMatch, false); + assert.equal(d.jaccard, 1); + assert.equal(d.equivalent, false); + void a; +}); + +test('fidelityDiff — tolerance gate: jaccard below tolerance is non-equivalent', () => { + const dLoose = fidelityDiff(RUN_A, RUN_B, { jaccardTolerance: 0.7 }); + const dStrict = fidelityDiff(RUN_A, RUN_B, { jaccardTolerance: 0.95 }); + assert.equal(dLoose.equivalent, true); + assert.equal(dStrict.equivalent, false); +}); + +test('fidelityDiff — flags severity mismatch on a shared finding ID', () => { + // Flip the BLOCKER severity in B's trailing JSON for the shared login.mjs finding. + const sharedId = '763d174e6c519fafbadcba5d1706708479e36e61'; + const tampered = RUN_B.replace( + `"id": "${sharedId}", "severity": "BLOCKER"`, + `"id": "${sharedId}", "severity": "MINOR"`, + ); + const d = fidelityDiff(RUN_A, tampered); + assert.ok(d.severityMismatches.some((m) => m.id === sharedId), + `expected a severity mismatch for ${sharedId}; got ${JSON.stringify(d.severityMismatches)}`); + assert.equal(d.equivalent, false); +}); + +test('fidelityDiff — exposes finding counts for both arms', () => { + const d = fidelityDiff(RUN_A, RUN_B); + assert.equal(d.countA, 5); + assert.equal(d.countB, 6); +}); + +// ---- structured arm-output path (the bake-off comparison) ------------------- + +const ARM = (verdict, findings) => ({ verdict, findings }); +const F = (severity, rule_key, file, line) => ({ severity, rule_key, file, line }); + +test('normalizeArmOutput — recomputes canonical IDs from the (file,line,rule_key) triplet', () => { + const arm = ARM('WARN', [F('BLOCKER', 'SECURITY_INJECTION', 'lib/exec.mjs', 23)]); + const n = normalizeArmOutput(arm); + assert.equal(n.verdict, 'WARN'); + assert.equal(n.findingIds.length, 1); + assert.equal(n.findingIds[0], computeFindingId('lib/exec.mjs', 23, 'SECURITY_INJECTION')); +}); + +test('normalizeArmOutput — drops findings missing file/rule_key from the ID set', () => { + const arm = ARM('WARN', [F('MAJOR', 'MISSING_TEST', '', 0), { severity: 'MINOR', line: 5 }]); + const n = normalizeArmOutput(arm); + assert.equal(n.findingIds.length, 0); + assert.equal(n.details.length, 2); // still recorded, just not ID-keyed +}); + +test('fidelityDiffStructured — identical arms are equivalent (jaccard 1.0)', () => { + const findings = [ + F('BLOCKER', 'UNIMPLEMENTED_CRITERION', 'a.mjs', 1), + F('MAJOR', 'MISSING_TEST', 'b.mjs', 0), + ]; + const d = fidelityDiffStructured(ARM('BLOCK', findings), ARM('BLOCK', findings)); + assert.equal(d.equivalent, true); + assert.equal(d.jaccard, 1); + assert.equal(d.verdictMatch, true); +}); + +test('fidelityDiffStructured — divergent verdict fails even with identical findings', () => { + const findings = [F('BLOCKER', 'UNIMPLEMENTED_CRITERION', 'a.mjs', 1)]; + const d = fidelityDiffStructured(ARM('BLOCK', findings), ARM('WARN', findings)); + assert.equal(d.verdictMatch, false); + assert.equal(d.equivalent, false); +}); + +test('fidelityDiffStructured — extra finding in Arm B lowers jaccard', () => { + const a = ARM('WARN', [F('MAJOR', 'MISSING_TEST', 'a.mjs', 10)]); + const b = ARM('WARN', [ + F('MAJOR', 'MISSING_TEST', 'a.mjs', 10), + F('MINOR', 'PLACEHOLDER_IN_CODE', 'b.mjs', 14), + ]); + const d = fidelityDiffStructured(a, b); + assert.ok(Math.abs(d.jaccard - 1 / 2) < 1e-9, `jaccard=${d.jaccard}`); + assert.equal(d.countA, 1); + assert.equal(d.countB, 2); +}); + +test('fidelityDiffStructured — same triplet, different severity → severity mismatch', () => { + const a = ARM('BLOCK', [F('BLOCKER', 'UNIMPLEMENTED_CRITERION', 'a.mjs', 1)]); + const b = ARM('BLOCK', [F('MAJOR', 'UNIMPLEMENTED_CRITERION', 'a.mjs', 1)]); + const d = fidelityDiffStructured(a, b); + assert.equal(d.jaccard, 1); // same triplet → same ID + assert.equal(d.severityMismatches.length, 1); + assert.equal(d.equivalent, false); +});