feat(voyage): S10 — NW2 part A (Workflow port + fidelity harness + smoke)
Build the prose-vs-Workflow bake-off machinery for /trekreview Phase 5-6 and
run a 1-run/arm smoke to de-risk before the full measurement (operator posture:
build + smoke, then pause for go/no-go on the full >=3-runs/arm run).
New:
- lib/review/fidelity-diff.mjs (+ tests) — the PRIMARY metric: parse two
review.md (or two structured arm outputs) and compare verdict + jaccard over
(file,line,rule_key)-IDs + per-finding severity/rule_key. Reuses jaccard +
frontmatter + NW1 findings-schema + finding-id. fidelityDiffStructured avoids
rendering review.md per run.
- scripts/trekreview-armB.workflow.mjs — Arm B: Phase 5-6 as a Workflow
(parallel([conformance, correctness]) schema-forced -> JS dedup-by-triplet ->
agent(review-coordinator) verdict schema). Path-based input via args (reviewers
carry Read). Inlines dedup + the 12-key rule_key enum (scripts have no imports).
- tests/fixtures/bakeoff/ — committable fixture: real diff of b149538 (NW1) +
brief reconstructed from plan S9. Both arms review the same pinned input.
- docs/T2-bakeoff-results.md — smoke results + verdict + go/no-go recommendation.
Smoke result: SMOKE PASS. Arm B runs the full pipeline (3 agents) with ZERO
classifier interference; fidelity EQUIVALENT to Arm A at the verdict level
(both ALLOW; jaccard 1.0). Caveat: the clean TDD'd fixture yielded ~0 findings,
so finding-SET fidelity was not stressed (only verdict fidelity proven). A
reviewer-level divergence appeared (Arm B raised 1 raw finding, coordinator
filtered it; Arm A raised 0) — to be quantified in the full run on a
richer-finding-surface fixture. NOT the T2 §5 POSITIVE/NEGATIVE verdict.
Suite 647 -> 662 (660 pass / 2 skip / 0 fail; +15 fidelity-diff). claude plugin
validate clean (known root-CLAUDE.md warning only). Plan: docs/W1-narrow-wins-plan.md S10.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LqBYc8Ltrk7LipyJmGxXiB
This commit is contained in:
parent
b149538d43
commit
869bf318d2
7 changed files with 1020 additions and 0 deletions
90
docs/T2-bakeoff-results.md
Normal file
90
docs/T2-bakeoff-results.md
Normal file
|
|
@ -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.
|
||||
173
lib/review/fidelity-diff.mjs
Normal file
173
lib/review/fidelity-diff.mjs
Normal file
|
|
@ -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] <review-A.md> <review-B.md>\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);
|
||||
}
|
||||
BIN
scripts/trekreview-armB.workflow.mjs
Normal file
BIN
scripts/trekreview-armB.workflow.mjs
Normal file
Binary file not shown.
50
tests/fixtures/bakeoff/README.md
vendored
Normal file
50
tests/fixtures/bakeoff/README.md
vendored
Normal file
|
|
@ -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).
|
||||
```
|
||||
78
tests/fixtures/bakeoff/brief.md
vendored
Normal file
78
tests/fixtures/bakeoff/brief.md
vendored
Normal file
|
|
@ -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.
|
||||
464
tests/fixtures/bakeoff/delivered.diff
vendored
Normal file
464
tests/fixtures/bakeoff/delivered.diff
vendored
Normal file
|
|
@ -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 <reviewer-output-file>
|
||||
+```
|
||||
+
|
||||
+`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": "<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 } 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] <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'));
|
||||
+ 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');
|
||||
+});
|
||||
165
tests/lib/fidelity-diff.test.mjs
Normal file
165
tests/lib/fidelity-diff.test.mjs
Normal file
|
|
@ -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);
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue