diff --git a/.reviewgate/lore/review-output-schema-strict.md b/.reviewgate/lore/review-output-schema-strict.md index 7c72e59..18108dc 100644 --- a/.reviewgate/lore/review-output-schema-strict.md +++ b/.reviewgate/lore/review-output-schema-strict.md @@ -4,8 +4,8 @@ id: review-output-schema-strict status: canon anchors: - "src/providers/review-output.ts" -verified_at: 2026-07-10 -verified_tree: "3d6c09350b1a2dcede15eb0917e011310c0c327b409895dc00c469491a1a5068" +verified_at: 2026-08-06 +verified_tree: "5388991bd6aebe97b8fad3437f141401be49649d18cbc2a772761f301ef94487" tags: [] --- Why REVIEW_OUTPUT_SCHEMA is shaped the way it is: codex's `--output-schema` runs diff --git a/docs/superpowers/plans/2026-08-05-true-positive-hole.md b/docs/superpowers/plans/2026-08-05-true-positive-hole.md new file mode 100644 index 0000000..baab74c --- /dev/null +++ b/docs/superpowers/plans/2026-08-05-true-positive-hole.md @@ -0,0 +1,816 @@ +# True-Positive Hole Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop Reviewgate demoting a real security finding twice over — once by calling a +mis-anchored finding "almost certainly hallucinated", once by letting a lone critic push a WARN +security finding below the blocking boundary. + +**Architecture:** Two independent slices. **Slice A** teaches `validateFindingFacts` to +distinguish *mis-anchored* from *fabricated* by consulting the reviewer's own `evidence_line`, +and re-anchors instead of demoting when the quote is a real line of the cited file. Because that +pass runs pre-aggregation, the repaired line feeds clustering, which is what lets two detections +of the same bug merge and gain corroboration. **Slice B** extends the critic's security exemption +from CRITICAL-only to CRITICAL-and-WARN. + +**Tech Stack:** TypeScript on Bun. `bun test` (not jest/vitest), `biome` for lint, +`tsc --noEmit` for types. Zod schemas are the source of truth for every persisted artifact. + +## Global Constraints + +- Spec of record: `docs/superpowers/specs/2026-08-05-true-positive-hole-design.md`. +- Runtime is **Bun**. Use `bun`/`bunx`, never `npm`/`node`/`npx`. +- `bunx tsc --noEmit` and `bun run lint` must both be clean before any task is "done". +- `FindingSchema` changes in Task 1, so the **full** `bun test` runs from Task 1 onward. +- Every new schema field is `.optional()` — older persisted `pending.json` must still parse. +- No new config key, no `reviewgate.config.ts` change: both slices are always-on. Touching the + config would arm the control plane and demand a TTY approval. +- **Do NOT run `bun run build`.** It deploys to every repo via the `~/.local/bin/reviewgate` + symlink. The rebuild is step 2 of the pilot-03 sequencing, after all review gates pass. +- Never `git add -A` at the repo root — it tracks `.reviewgate/` runtime state. Add named paths. +- Guard tests that already pass against current code are **vacuous until mutation-checked**. + Every such test below carries an explicit mutation step: apply the mutation **in a copy of the + repo**, confirm RED, discard the copy, confirm `git diff` shows the original unchanged. + +--- + +## File Structure + +| File | Responsibility | Change | +|---|---|---| +| `src/core/fact-check.ts` | Deterministic finding fact-check | Add `reanchorByEvidence`; consult it before demoting | +| `src/schemas/finding.ts` | Persisted finding shape | Add `anchor_repaired` to the finding and to `members[]` | +| `src/core/aggregator.ts` | Dedup, merge, suppression passes, verdict | Propagate `anchor_repaired` across a merge; add the critic's WARN-security floor | +| `src/core/report-writer.ts` | Renders `pending.md` | One badge | +| `tests/unit/fact-check-reanchor.test.ts` | Slice A guards | **Create** | +| `tests/unit/aggregator-critic.test.ts` | Critic-pass guards | Append Slice B guards | +| `tests/unit/anchor-repair-cascade.test.ts` | End-to-end acceptance | **Create** | + +`src/core/orchestrator.ts` is deliberately **not** touched — the whole change sits inside a pass +it already calls. + +--- + +## Task 1: Slice A — re-anchor a mis-anchored finding instead of demoting it + +**Files:** +- Modify: `src/schemas/finding.ts:234` (beside `fact_invalid`) +- Modify: `src/core/fact-check.ts:116-119` (the demote decision) and add a helper above it +- Test: `tests/unit/fact-check-reanchor.test.ts` (create) + +**Interfaces:** +- Consumes: `normalizeLine(s: string): string` — already in `fact-check.ts:129`, a hoisted + function declaration, so it is callable from a function defined above it. It defangs injection + markers, collapses whitespace runs to one space, and trims. +- Consumes: `lineCount(text: string): number` — already in `fact-check.ts:32`. +- Produces: `Finding.anchor_repaired?: boolean` — Task 2 propagates it, Task 3 renders it. + +- [ ] **Step 1: Write the failing tests** + +Create `tests/unit/fact-check-reanchor.test.ts`: + +```ts +// tests/unit/fact-check-reanchor.test.ts +// +// pilot-02 turn 2: the fact-check demoted a 0.90 path-traversal finding as "almost certainly +// hallucinated" because it cited line 67 of a 27-line file — while the reviewer's OWN +// evidence_line matched line 26 of that file verbatim. Mis-anchored, not fabricated. These +// guards pin the distinction, and pin that the fabrication protection the pass was built for +// (a CRITICAL citing a line in an EMPTY file) is untouched. +import { describe, expect, it } from "bun:test"; +import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { validateFindingFacts } from "../../src/core/fact-check.ts"; +import type { Finding } from "../../src/schemas/finding.ts"; + +const EVIDENCE = " return readFileSync(`./templates/${name}`, 'utf8')"; + +function mkFinding(over: Partial = {}): Finding { + return { + id: "F-001", + signature: "sig1", + severity: "CRITICAL", + category: "security", + rule_id: "path-traversal", + file: "store.ts", + line_start: 67, + line_end: 67, + message: "Path traversal vulnerability in readTemplate.", + details: "details", + reviewer: { provider: "openrouter", model: "deepseek/deepseek-v3.2", persona: "security" }, + confidence: 0.9, + consensus: "singleton", + ...over, + }; +} + +function repo(content: string): string { + const dir = mkdtempSync(join(tmpdir(), "rg-reanchor-")); + mkdirSync(join(dir, "src"), { recursive: true }); + writeFileSync(join(dir, "store.ts"), content); + writeFileSync(join(dir, "empty.yaml"), ""); + return dir; +} + +// 5 lines, the quoted evidence at line 3. +const FIVE_LINES = ["const a = 1", "const b = 2", EVIDENCE, "const d = 4", "const e = 5"].join("\n") + "\n"; + +describe("validateFindingFacts — mis-anchored vs fabricated", () => { + // GUARD 1. WITHOUT the mechanism: severity INFO + fact_invalid -> 0 blocking. + // WITH it: CRITICAL kept at the quoted line 3 + anchor_repaired -> 1 blocking. + it("re-anchors an out-of-range finding whose evidence_line matches a real line", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ line_start: 67, line_end: 67, evidence_line: EVIDENCE })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("CRITICAL"); + expect(out[0]?.line_start).toBe(3); + expect(out[0]?.line_end).toBe(3); + expect(out[0]?.anchor_repaired).toBe(true); + expect(out[0]?.fact_invalid).toBeUndefined(); + expect(out[0]?.details).toContain("re-anchored"); + }); + + // GUARD 2 (passes on current code — MUTATION-CHECKED in Step 3). + // WITHOUT the evidence gate (repair unconditionally): fact_invalid absent. + // WITH it: fact_invalid true. This is the empty-file field-report case. + it("still demotes an out-of-range finding that carries NO evidence_line", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ file: "empty.yaml", line_start: 2, line_end: 2 })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("INFO"); + expect(out[0]?.fact_invalid).toBe(true); + expect(out[0]?.anchor_repaired).toBeUndefined(); + }); + + // GUARD 3 (passes on current code — MUTATION-CHECKED in Step 3). + // WITHOUT the match test: fact_invalid absent. WITH it: fact_invalid true. + it("still demotes when the evidence_line matches NO line of the cited file", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ line_start: 67, evidence_line: "this line is nowhere in the file" })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("INFO"); + expect(out[0]?.fact_invalid).toBe(true); + expect(out[0]?.anchor_repaired).toBeUndefined(); + }); + + // GUARD 4. WITHOUT the nearest-match rule (first match wins): line_start 2. + // WITH it: line_start 8. Deterministic either way, but only one is the rule. + it("resolves a multi-match to the occurrence NEAREST the cited line", () => { + const dup = ["a", EVIDENCE, "c", "d", "e", "f", "g", EVIDENCE, "i", "j"].join("\n") + "\n"; + const dir = repo(dup); + const out = validateFindingFacts( + [mkFinding({ line_start: 20, line_end: 20, evidence_line: EVIDENCE })], + dir, + new Set(), + ); + expect(out[0]?.line_start).toBe(8); + expect(out[0]?.anchor_repaired).toBe(true); + }); + + // GUARD 5 (passes on current code — MUTATION-CHECKED in Step 3). + // WITHOUT the range check first (repair before it): line_start moves to 3. + // WITH the correct order: line_start stays 1. The pass must never move a VALID anchor. + it("never touches a finding whose cited line is IN range", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ line_start: 1, line_end: 1, evidence_line: EVIDENCE })], + dir, + new Set(), + ); + expect(out[0]?.line_start).toBe(1); + expect(out[0]?.anchor_repaired).toBeUndefined(); + expect(out[0]?.fact_invalid).toBeUndefined(); + }); + + // Robustness: the match runs under normalizeLine, so indentation/whitespace differences in + // the reviewer's quote must not defeat it. WITHOUT normalization: no match -> demoted. + // WITH it: re-anchored to line 3. + it("matches the quote whitespace-insensitively", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ line_start: 67, evidence_line: "return readFileSync(`./templates/${name}`, 'utf8')" })], + dir, + new Set(), + ); + expect(out[0]?.line_start).toBe(3); + expect(out[0]?.anchor_repaired).toBe(true); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `bun test tests/unit/fact-check-reanchor.test.ts` + +Expected: guards 1, 4 and the whitespace test FAIL (the finding comes back `INFO` + +`fact_invalid`, `line_start` still 67). Guards 2, 3, 5 PASS — they pin behaviour that already +exists, and Step 3 proves they are not vacuous. + +- [ ] **Step 3: Mutation-check the three tests that already pass** + +Do this in a **copy**, never in the working repo: + +```bash +cp -R /Users/markus/Developer/reviewgate /private/tmp/claude-501/-Users-markus-Developer-reviewgate/4d33f15e-1cc6-4490-b511-a3a6d3b10443/scratchpad/mut1 +``` + +In the copy's `src/core/fact-check.ts`, apply each mutation and record the result: + +| Mutation | Expected RED | +|---|---| +| Delete the `if (f.line_start <= lines) return f;` early return (repair before the range check) | GUARD 5 red — `line_start` 3, expected 1 | +| Make `reanchorByEvidence` return a repaired finding even when `evidence_line` is absent (anchor to line 1) | GUARD 2 red — `fact_invalid` undefined, expected true | +| Make `reanchorByEvidence` skip the `normalizeLine` equality test and take line 1 | GUARD 3 red — `fact_invalid` undefined, expected true | + +Then: `rm -rf ` and `git -C /Users/markus/Developer/reviewgate diff --stat` → must show the +original untouched. + +(Steps 1–3 are written against the finished implementation; if a mutation cannot be applied +because the code does not exist yet, do Step 4 first and then Step 3.) + +- [ ] **Step 4: Add the schema field** + +In `src/schemas/finding.ts`, immediately after `fact_invalid: z.boolean().optional(),` (`:234`): + +```ts + // Slice A (pilot-02 turn 2): set true when the fact-check found the cited line OUT OF RANGE + // but the reviewer's own evidence_line matched a real line of that file — the finding was + // MIS-ANCHORED, not fabricated, so it is re-anchored to the quoted line instead of demoted. + // Severity is untouched; this is a provenance/render marker. Mutually exclusive with + // fact_invalid by construction (the repair returns before the demote). + anchor_repaired: z.boolean().optional(), +``` + +- [ ] **Step 5: Add the helper to `src/core/fact-check.ts`** + +Insert directly above `export function validateFindingFacts` (`:58`): + +```ts +/** + * A cited line that does not exist is NOT automatically a fabrication. When the reviewer quoted + * its own evidence and that quote is a real line of the cited file, the reviewer read real code + * and mis-numbered it — mis-anchored, not hallucinated. Demoting that as a hallucination is a + * false accusation against a finding we can PROVE is grounded (pilot-02 turn 2: a 0.90 path + * traversal, quote verbatim at line 26, cited at 67, demoted with "almost certainly + * hallucinated"). + * + * Returns the finding re-anchored to the quoted line, or null when nothing can be proven — in + * which case the caller demotes exactly as before. This can only ever be reached for an + * out-of-range anchor, so it can never move a valid one. + * + * Multiple matches resolve to the occurrence NEAREST the cited line, ties to the LOWER line + * number. Both are real occurrences of the reviewer's own quote, so this chooses among facts + * rather than inventing one — and it must be deterministic, because the aggregator's clustering + * is deliberately order-independent and keys on line_start. + */ +function reanchorByEvidence(f: Finding, text: string): Finding | null { + const ev = typeof f.evidence_line === "string" ? f.evidence_line : null; + if (ev === null || ev.length === 0) return null; + const evN = normalizeLine(ev); + if (evN.length === 0) return null; // quote was only whitespace/markers → no signal + const lines = text.split("\n"); + const cited = f.line_start - 1; + let best = -1; + for (let i = 0; i < lines.length; i++) { + const raw = lines[i]; + if (raw === undefined) continue; + if (normalizeLine(raw) !== evN) continue; + // Strict `<` with an ascending scan makes ties keep the LOWER line number. + if (best === -1 || Math.abs(i - cited) < Math.abs(best - cited)) best = i; + } + if (best === -1) return null; // quote matches no line → the fabrication signal stands + const line = best + 1; + const total = lineCount(text); + const note = `\n\n[reviewgate fact-check] the reviewer cited ${f.file}:${f.line_start}, which does not exist (the file has ${total} line${total === 1 ? "" : "s"}), but the evidence it quoted matches line ${line} verbatim — MIS-ANCHORED, not fabricated, so it has been re-anchored there. Treat the defect as real and check line ${line}.`; + return { + ...f, + line_start: line, + line_end: line, + anchor_repaired: true, + details: `${f.details.slice(0, 2000 - note.length)}${note}`, + }; +} +``` + +- [ ] **Step 6: Consult the helper before demoting** + +In `validateFindingFacts`, replace lines `116-119`: + +```ts + const lines = lineCount(text); + if (f.line_start <= lines) return f; // cited line exists → real finding, untouched + const note = `\n\n[reviewgate fact-check] cited location ${file}:${f.line_start} does not exist in the working tree (file has ${lines} line${lines === 1 ? "" : "s"}) — almost certainly hallucinated; demoted to advisory. Verify before treating as real.`; + return demote(f, note); +``` + +with: + +```ts + const lines = lineCount(text); + if (f.line_start <= lines) return f; // cited line exists → real finding, untouched + // Out of range. Before calling it a fabrication, consult the reviewer's OWN quoted evidence: + // a quote that matches a real line of THIS file proves the reviewer read the code and + // mis-numbered it, which is not what this pass exists to catch. + const repaired = reanchorByEvidence(f, text); + if (repaired !== null) return repaired; + const note = `\n\n[reviewgate fact-check] cited location ${file}:${f.line_start} does not exist in the working tree (file has ${lines} line${lines === 1 ? "" : "s"}) — almost certainly hallucinated; demoted to advisory. Verify before treating as real.`; + return demote(f, note); +``` + +- [ ] **Step 7: Run the tests to verify they pass** + +Run: `bun test tests/unit/fact-check-reanchor.test.ts tests/unit/fact-check.test.ts tests/unit/evidence-attestation.test.ts` +Expected: PASS, including the pre-existing fact-check and evidence suites. + +- [ ] **Step 8: Full gates** + +```bash +bunx tsc --noEmit +bun run lint +bun test +``` +Expected: all clean. `FindingSchema` changed, so the whole suite matters here. + +- [ ] **Step 9: Commit** + +```bash +git add src/schemas/finding.ts src/core/fact-check.ts tests/unit/fact-check-reanchor.test.ts +git commit -m "fix(fact-check): re-anchor a mis-anchored finding instead of calling it fabricated" +``` + +--- + +## Task 2: Carry `anchor_repaired` through a dedup merge + +**Why this task exists:** Slice A's whole point is that the repaired line lets two detections +merge. But `memberOf` (`aggregator.ts:295`) projects a member down to six fields and drops +everything else, and the representative is chosen by severity with **ties keeping the first**. In +the turn-2 shape both findings are WARN and the repaired one sorts second, so it becomes a +*member* — and the marker, the badge and pilot-03's count all vanish in exactly the case the +slice was built for. `demoted_from_critical` already solved this problem in the same code +(`:524-531`); this mirrors it. + +**Files:** +- Modify: `src/schemas/finding.ts:254` (inside the `members[]` object) +- Modify: `src/core/aggregator.ts:295-307` (`memberOf`) and `:529-538` (the dedupe push) +- Test: `tests/unit/anchor-repair-cascade.test.ts` (create; Task 5 adds the second test to it) + +**Interfaces:** +- Consumes: `Finding.anchor_repaired` from Task 1. +- Produces: a deduped finding whose `anchor_repaired` is `OR(representative, all members)`. + +- [ ] **Step 1: Write the failing test** + +Create `tests/unit/anchor-repair-cascade.test.ts`: + +```ts +// tests/unit/anchor-repair-cascade.test.ts +import { describe, expect, it } from "bun:test"; +import { aggregate } from "../../src/core/aggregator.ts"; +import type { Finding } from "../../src/schemas/finding.ts"; + +function fin(over: Partial): Finding { + return { + id: "F-x", + signature: "s", + severity: "WARN", + category: "security", + rule_id: "r", + file: "src/store.ts", + line_start: 25, + line_end: 25, + message: "m", + details: "d", + reviewer: { provider: "ollama", model: "glm-5.2:cloud", persona: "correctness" }, + confidence: 0.55, + consensus: "singleton", + ...over, + }; +} + +describe("anchor_repaired survives a dedup merge", () => { + // WITHOUT the OR-propagation: anchor_repaired is undefined on the merged finding (it lived on + // the member, and ties-keep-first made the UNrepaired finding the representative) -> the badge + // and pilot-03's count both disappear in exactly the cascade case. + // WITH it: anchor_repaired is true on the single merged finding. + it("OR-propagates anchor_repaired from a merged member to the representative", () => { + const r = aggregate({ + findings: [ + fin({ signature: "sigA", line_start: 25, line_end: 25, rule_id: "path-traversal-readtemplate" }), + fin({ + signature: "sigB", + line_start: 26, + line_end: 26, + rule_id: "path-traversal", + anchor_repaired: true, + reviewer: { provider: "openrouter", model: "deepseek/deepseek-v3.2", persona: "security" }, + confidence: 0.9, + }), + ], + reviewersTotal: 2, + }); + expect(r.dedupedFindings.length).toBe(1); + expect(r.dedupedFindings[0]?.anchor_repaired).toBe(true); + expect(r.dedupedFindings[0]?.consensus).toBe("majority"); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `bun test tests/unit/anchor-repair-cascade.test.ts` +Expected: FAIL — `anchor_repaired` is `undefined`, expected `true`. (`consensus` already passes; +the merge itself is existing behaviour.) + +- [ ] **Step 3: Add the field to the member schema** + +In `src/schemas/finding.ts`, inside the `members[]` object right after +`demoted_from_critical: z.boolean().optional(),` (`:254`): + +```ts + // Slice A: per-member mis-anchor provenance, so a repaired member merged under an + // unrepaired equal-severity representative (ties-keep-first) does not silently lose + // the marker — the badge and the pilot count both key on it. + anchor_repaired: z.boolean().optional(), +``` + +- [ ] **Step 4: Carry it in `memberOf`** + +In `src/core/aggregator.ts`, inside `memberOf` after the `demoted_from_critical` spread (`:305`): + +```ts + ...(f.anchor_repaired === true ? { anchor_repaired: true } : {}), +``` + +- [ ] **Step 5: OR it into the representative** + +In `src/core/aggregator.ts`, after the `demotedFromCritical` computation (`:529-531`): + +```ts + // Slice A mirror of the G0 OR above: the repaired finding is often NOT the representative + // (equal severity + ties-keep-first), and losing the marker would hide the mis-anchor in + // exactly the merge the repair made possible. + const anchorRepaired = + sample.anchor_repaired === true || members.some((m) => m.anchor_repaired === true); +``` + +and add to the `deduped.push({...})` object after the `demoted_from_critical` spread (`:538`): + +```ts + ...(anchorRepaired ? { anchor_repaired: true } : {}), +``` + +- [ ] **Step 6: Run tests to verify they pass** + +Run: `bun test tests/unit/anchor-repair-cascade.test.ts tests/unit/aggregator-dedup-category.test.ts` +Expected: PASS. + +- [ ] **Step 7: Gates and commit** + +```bash +bunx tsc --noEmit && bun run lint && bun test +git add src/schemas/finding.ts src/core/aggregator.ts tests/unit/anchor-repair-cascade.test.ts +git commit -m "fix(aggregator): carry anchor_repaired across a dedup merge" +``` + +--- + +## Task 3: Render the mis-anchor badge + +**Files:** +- Modify: `src/core/report-writer.ts:43` (beside the `fact_invalid` badge) +- Test: `tests/unit/anchor-repair-cascade.test.ts` (append) + +**Interfaces:** +- Consumes: `Finding.anchor_repaired` (Task 1), `findingBadges(f: Finding): string | null` + (`report-writer.ts:39`, already exported). + +- [ ] **Step 1: Write the failing test** + +Append to `tests/unit/anchor-repair-cascade.test.ts` (and add +`import { findingBadges } from "../../src/core/report-writer.ts";` to the imports): + +```ts +describe("anchor_repaired badge", () => { + // WITHOUT the badge: findingBadges returns null for a repaired finding -> the agent sees a + // silently corrected line number and no signal that a reviewer mis-anchored. + // WITH it: the badge text is present. + it("renders a badge naming the repair", () => { + const out = findingBadges(fin({ anchor_repaired: true })); + expect(out).toContain("re-anchored"); + }); + + it("renders no such badge for an ordinary finding", () => { + expect(findingBadges(fin({}))).toBeNull(); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `bun test tests/unit/anchor-repair-cascade.test.ts` +Expected: FAIL — `findingBadges` returns `null`, expected a string containing "re-anchored". + +- [ ] **Step 3: Add the badge** + +In `src/core/report-writer.ts`, directly after the `fact_invalid` badge (`:43`): + +```ts + // Slice A: the counterpart to the badge above — the cited line was wrong, but the reviewer's + // quoted evidence proved the finding is grounded, so it was moved rather than demoted. + if (f.anchor_repaired) + badges.push("⚑ reviewer cited a line that does not exist — re-anchored to the source line it quoted"); +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `bun test tests/unit/anchor-repair-cascade.test.ts tests/unit/report-writer.test.ts` +Expected: PASS — the new badge tests plus the existing report-writer suite. + +- [ ] **Step 5: Gates and commit** + +```bash +bunx tsc --noEmit && bun run lint && bun test +git add src/core/report-writer.ts tests/unit/anchor-repair-cascade.test.ts +git commit -m "feat(report): badge a finding whose anchor was repaired from its quoted evidence" +``` + +--- + +## Task 4: Slice B — the critic's WARN-security floor + +**Files:** +- Modify: `src/core/aggregator.ts:604` (add the floor), `:615` and `:619` (use it) +- Test: `tests/unit/aggregator-critic.test.ts` (append) + +**Interfaces:** +- Consumes: `touchesSecurityOrCorrectness(f: Finding): boolean` (`aggregator.ts:255`). +- Produces: nothing new — a protected finding takes the existing + `survivors.push({ ...f, critic_verdict: "keep" })` path at `:635`. + +- [ ] **Step 1: Write the failing tests** + +Append to `tests/unit/aggregator-critic.test.ts`: + +```ts +describe("critic — security/correctness floor (pilot-02 turn 2)", () => { + // GUARD 7. WITHOUT the floor: INFO + critic_verdict "likely_fp" -> 0 blocking. + // WITH it: WARN + critic_verdict "keep" -> 1 blocking. + it("does not demote a WARN security finding below the blocking boundary", () => { + const f = fin({ signature: "sigSecWarn", severity: "WARN", category: "security" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigSecWarn", { verdict: "likely_fp" }]]), + }); + expect(r.dedupedFindings[0]?.severity).toBe("WARN"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("keep"); + }); + + it("does not demote a WARN correctness finding below the blocking boundary", () => { + const f = fin({ signature: "sigCorrWarn", severity: "WARN", category: "correctness" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigCorrWarn", { verdict: "likely_fp" }]]), + }); + expect(r.dedupedFindings[0]?.severity).toBe("WARN"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("keep"); + }); + + // GUARD 8 (passes on current code — MUTATION-CHECKED in Step 3). + // WITH an "exempt at every severity" floor: criticDroppedCount 0. + // WITH the correct WARN floor: criticDroppedCount 1. The critic keeps its FP-filtering + // power exactly where reviewers are noisiest (low-confidence INFO security chatter). + it("still drops an already-INFO security likely_fp", () => { + const f = fin({ signature: "sigSecInfo", severity: "INFO", category: "security" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigSecInfo", { verdict: "likely_fp" }]]), + }); + expect(r.criticDroppedCount).toBe(1); + expect(r.dedupedFindings.length).toBe(0); + }); + + // GUARD 9 (passes on current code — MUTATION-CHECKED in Step 3). + // WITH a severity-only floor (all WARN exempt): the finding stays WARN -> 1 blocking. + // WITH the correct category-keyed floor: INFO -> 0 blocking. + it("still demotes a WARN quality finding", () => { + const f = fin({ signature: "sigQualWarn", severity: "WARN", category: "quality" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigQualWarn", { verdict: "likely_fp" }]]), + }); + expect(r.dedupedFindings[0]?.severity).toBe("INFO"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("likely_fp"); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `bun test tests/unit/aggregator-critic.test.ts` +Expected: the two "does not demote a WARN …" tests FAIL (severity comes back `INFO`, +`critic_verdict` `likely_fp`). Guards 8 and 9 PASS — Step 3 proves they are not vacuous. + +- [ ] **Step 3: Mutation-check guards 8 and 9** + +In a **copy** of the repo: + +| Mutation in `aggregator.ts` | Expected RED | +|---|---| +| `const isBlockingSecurity = touchesSecurityOrCorrectness(f);` (drop the severity test → exempt at every severity) | GUARD 8 red — `criticDroppedCount` 0, expected 1 | +| `const isBlockingSecurity = f.severity === "WARN";` (drop the category test) | GUARD 9 red — severity `WARN`, expected `INFO` | + +Then `rm -rf ` and confirm `git diff --stat` shows the original untouched. + +- [ ] **Step 4: Implement the floor** + +In `src/core/aggregator.ts`, replace line `604`: + +```ts + const isCriticalSecurity = f.severity === "CRITICAL" && touchesSecurityOrCorrectness(f); +``` + +with: + +```ts + const isCriticalSecurity = f.severity === "CRITICAL" && touchesSecurityOrCorrectness(f); + // pilot-02 turn 2: the exemption above is keyed to CRITICAL, so a WARN-severity security + // finding was demotable by a single adversarial critic — and WARN→INFO is the one demote + // that crosses the blocking boundary (isBlocking = CRITICAL || WARN). The sibling + // delta-scope pass already exempts security/correctness at ANY severity; mirror that floor + // here. An already-INFO security finding stays droppable, so the critic keeps its + // FP-filtering power where reviewers are noisiest. + const isBlockingSecurity = f.severity === "WARN" && touchesSecurityOrCorrectness(f); + const isSecurityProtected = isCriticalSecurity || isBlockingSecurity; +``` + +Then replace `!isCriticalSecurity` with `!isSecurityProtected` in **both** conditions — `:615` +(`if (!isCriticalSecurity && !isCorroborated && isProtected(f))`) and `:619` +(`if (!isCriticalSecurity && !isCorroborated)`). + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `bun test tests/unit/aggregator-critic.test.ts` +Expected: PASS. + +- [ ] **Step 6: Gates and commit** + +```bash +bunx tsc --noEmit && bun run lint && bun test +git add src/core/aggregator.ts tests/unit/aggregator-critic.test.ts +git commit -m "fix(aggregator): the critic may not demote a security finding below WARN" +``` + +--- + +## Task 5: End-to-end acceptance — the turn-2 cascade + +**Files:** +- Test: `tests/unit/anchor-repair-cascade.test.ts` (append) + +No production code changes. If this test does not pass on the strength of Tasks 1–4, one of them +is wrong. + +**Interfaces:** +- Consumes: `validateFindingFacts` (Task 1), `aggregate` (Tasks 2 and 4). + +**What it reconstructs:** turn 2's two findings with their real lines, categories, providers, +confidences and the actual `evidence_line`, over a temp-dir copy of the 27-line `src/store.ts` +taken from `rig/results/pilot-02/turns/2/diff.patch`. This is a **reconstruction, not a replay** — +the archived findings are post-aggregation, so the pre-aggregation severities are inferred from +the demotion markers, exactly as `ablate.ts` does. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/unit/anchor-repair-cascade.test.ts` (add `mkdirSync`, `mkdtempSync`, +`writeFileSync`, `tmpdir`, `join` and `validateFindingFacts` to the imports): + +```ts +// The 27-line src/store.ts exactly as turn 2's diff created it. Line 26 is the quoted evidence. +const STORE_TS = [ + "import { readFileSync } from 'node:fs'", + "", + "export interface KVStore {", + " get(key: K): V | undefined", + " set(key: K, value: V): void", + " has(key: K): boolean", + "}", + "", + "export function createStore(): KVStore {", + " const entries = new Map()", + "", + " return {", + " get(key) {", + " return entries.get(key)", + " },", + " set(key, value) {", + " entries.set(key, value)", + " },", + " has(key) {", + " return entries.has(key)", + " },", + " }", + "}", + "", + "export function readTemplate(name: string): string {", + " return readFileSync(`./templates/${name}`, 'utf8')", + "}", +].join("\n") + "\n"; + +describe("pilot-02 turn 2 — the full cascade", () => { + // WITHOUT the fix: F-002 is demoted as fabricated (INFO + fact_invalid) and stays 42 lines + // from F-001, so nothing merges, both stay `singleton`, and the critic demotes F-001 to INFO + // -> 2 findings, 0 blocking. This is what pilot-02 recorded. + // WITH the fix: F-002 re-anchors 67 -> 26, merges with F-001 at 25, consensus becomes + // `majority`, the critic is barred -> 1 finding, blocking WARN. + it("repairs the anchor, merges, corroborates, and stays blocking", () => { + const dir = mkdtempSync(join(tmpdir(), "rg-turn2-")); + mkdirSync(join(dir, "src"), { recursive: true }); + writeFileSync(join(dir, "src", "store.ts"), STORE_TS); + + const evidence = " return readFileSync(`./templates/${name}`, 'utf8')"; + const raw: Finding[] = [ + fin({ + signature: "sigF001", + rule_id: "path-traversal-readtemplate", + line_start: 25, + line_end: 27, + confidence: 0.55, + evidence_line: evidence, + message: "readTemplate interpolates 'name' directly into a filesystem path", + }), + fin({ + signature: "sigF002", + rule_id: "path-traversal", + line_start: 67, + line_end: 67, + confidence: 0.9, + evidence_line: evidence, + message: "Path traversal vulnerability in readTemplate.", + reviewer: { provider: "openrouter", model: "deepseek/deepseek-v3.2", persona: "security" }, + }), + ]; + + const checked = validateFindingFacts(raw, dir, new Set()); + expect(checked.find((f) => f.signature === "sigF002")?.line_start).toBe(26); + + const r = aggregate({ + findings: checked, + reviewersTotal: 2, + changedRanges: new Map([["src/store.ts", [[1, 27]] as Array<[number, number]>]]), + scopeToDiff: true, + // The critic called the weaker detection a likely FP, exactly as it did in the pilot. + critic: new Map([["sigF001", { verdict: "likely_fp" }]]), + }); + + expect(r.dedupedFindings.length).toBe(1); + expect(r.dedupedFindings[0]?.consensus).toBe("majority"); + expect(r.dedupedFindings[0]?.severity).toBe("WARN"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("keep"); + expect(r.dedupedFindings[0]?.anchor_repaired).toBe(true); + expect(r.dedupedFindings[0]?.scope_demoted).toBeUndefined(); + expect(r.dedupedFindings[0]?.fact_invalid).toBeUndefined(); + }); +}); +``` + +- [ ] **Step 2: Run it** + +Run: `bun test tests/unit/anchor-repair-cascade.test.ts` +Expected: PASS on the strength of Tasks 1–4. If it fails, do **not** patch the test — find which +task's behaviour is wrong. + +- [ ] **Step 3: Gates and commit** + +```bash +bunx tsc --noEmit && bun run lint && bun test +git add tests/unit/anchor-repair-cascade.test.ts +git commit -m "test: pilot-02 turn-2 cascade — repair, merge, corroborate, stay blocking" +``` + +--- + +## After the tasks + +1. **Review pipeline** — the post-implementation gate, two independent slots, both must return + `VERDICT: PASS`. Codex is quota-blocked until **2026-08-08T11:07Z**, so Slot A (the executing + reviewer) is `agy`/Gemini or a Claude reviewer subagent, and Slot B is a second, different + voice. Tell the reviewer explicitly to mutation-check the new guard tests in a copy. +2. **`rm -rf .review/`** before the final commit. +3. **Stop and ask before pushing.** Do not push to `origin`. +4. **Do not `bun run build` yet** — that is step 2 of the pilot-03 sequencing in the spec, and it + re-pins the binary for every repo on the machine. diff --git a/docs/superpowers/specs/2026-08-05-true-positive-hole-design.md b/docs/superpowers/specs/2026-08-05-true-positive-hole-design.md new file mode 100644 index 0000000..23ae56b --- /dev/null +++ b/docs/superpowers/specs/2026-08-05-true-positive-hole-design.md @@ -0,0 +1,338 @@ +# Closing the true-positive hole pilot-02 exposed + +_2026-08-05. Task (b) from `NEXT_SESSION.md`, following +`docs/dev/2026-08-05-pilot-02-result.md` and the measurement fixes in +`docs/superpowers/plans/2026-08-05-rig-measurement-fixes.md` (task (a), `5a1f94f`)._ + +> **Revision note.** A first version of this spec proposed a new anchor-validation pass in +> `aggregator.ts`. That was wrong: `src/core/fact-check.ts` already validates line anchors, it +> already fired on the finding in question, and the new pass would have silently reversed a +> deliberate, documented policy. The design below replaces it. What survives from the first +> version is Slice B, unchanged. + +## The evidence this design starts from + +pilot-02's turn 2 seeded a path traversal. The panel **detected it twice**. Both detections +ended at INFO, the turn recorded 0 blocking findings, and M3 scored a miss. + +Read the recorded findings yourself: + +```bash +bun -e 'const j = JSON.parse(await Bun.file("rig/results/pilot-02/turns/2/.reviewgate/pending.json").text()); +for (const f of j.findings) console.log(f.id, f.rule_id, f.severity, f.line_start, f.confidence, f.consensus, f.critic_verdict ?? "-", f.fact_invalid ?? "-", f.scope_demoted ?? "-")' +sed -n '1,10p' rig/results/pilot-02/turns/2/diff.patch +``` + +| | rule_id | line_start | conf | consensus | what demoted it | +|---|---|---|---:|---|---| +| F-001 | `path-traversal-readtemplate` | **25** | 0.55 | singleton | critic → `likely_fp` | +| F-002 | `path-traversal` | **67** | 0.90 | singleton | **`fact_invalid`**, then `scope_demoted` | + +`src/store.ts` is a **brand-new 27-line file** in that turn's diff (`@@ -0,0 +1,27 @@`). +F-002 is anchored at **line 67 — past EOF**. + +### The gate caught the bad anchor, and drew the wrong conclusion + +`validateFindingFacts` (`src/core/fact-check.ts`, called at `orchestrator.ts:2226`, before +aggregation) detected it exactly and demoted the finding, writing this into its details: + +``` +[reviewgate fact-check] cited location src/store.ts:67 does not exist in the working tree +(file has 27 lines) — almost certainly hallucinated; demoted to advisory. +``` + +That pass exists for good reason — two production field reports where a lone reviewer emitted a +0.97 CRITICAL citing content in an **empty** file. Its stated premise is: + +> _"if a file has 3 lines, a finding on line 99 is unambiguously fabricated"_ (`fact-check.ts:18`) + +**F-002 falsifies that premise.** It carries the reviewer's own quoted evidence: + +``` +evidence_line: " return readFileSync(`./templates/${name}`, 'utf8')" +``` + +which is **verbatim line 26** of that 27-line file. The reviewer read the real code, quoted it +correctly, and mis-numbered the line. The finding was **mis-anchored, not fabricated** — and the +gate told the agent the opposite, at 0.90 confidence, about a real path traversal. + +The discriminator already exists **in the same file**: `attestEvidence:154` computes +`lines.some((l) => normalizeLine(l) === evN)` — "does this quote match any line of the file". It +runs at `orchestrator.ts:2573`, render-only, *after* aggregation, and never feeds the demote. + +### Scale, measured across both pilots + +```bash +bun -e 'let tot=0,ev=0,fi=0; const g=new Bun.Glob("rig/results/pilot-0*/turns/*/reports/*-pending.json"); +for await (const p of g.scan(".")) { const j=JSON.parse(await Bun.file(p).text()); +for (const f of j.findings ?? []) { tot++; if (f.evidence_line) ev++; if (f.fact_invalid) fi++; } } +console.log({tot, ev, fi})' +``` + +| | | +|---|---| +| findings across both pilots | 68 | +| carrying `evidence_line` | **55 (81 %)** | +| distinct findings `fact_invalid` fired on, in 24 turns | **1** | +| ...whose evidence line matched a real line of the cited file | **1 of 1** | + +Running the command above prints `fi: 2`, not 1 — both numbers are right. It is the **same** +signature (`8deca571…`) recurring in `turns/3`, so the raw count is 2 rows but 1 distinct finding. +The one time this pass fired (on a distinct finding), it labelled a real security finding a +hallucination. **n = 1**: the change below is justified by the mechanism, not by that sample size. + +### Why this one anchor cost the whole turn + +The bad anchor caused all three failures the pilot-02 write-up attributes to independent layers: + +1. **`fact_invalid`** demoted F-002 to INFO as fabricated (above). +2. **The two findings never merged.** 25 and 67 are 42 lines apart, past both `REGION_WINDOW` + (5, `aggregator.ts:192`) and `WORDING_MERGE_MAX_LINE_DISTANCE` (25, `:229`). +3. **No merge meant no corroboration.** `computeConsensus(1, 2)` is `"singleton"`; merged it + would be `computeConsensus(2, 2)` → `"majority"` → `isCorroborated` (`:610`) → **the critic + could not have demoted F-001 at all.** + +So the handoff's candidate (2) — "merge same-file/same-category detections before the critic" — +targets a symptom. These findings did not need a broader merge rule; they needed the anchor +repaired, after which they are **one line apart** and merge under the existing window. + +The handoff's candidate (1) is a real defect independent of turn 2: the critic's exemption is +keyed to CRITICAL (`:604`) while the sibling `deltaScoped` pass exempts +`touchesSecurityOrCorrectness` at **any** severity (`:664`). That is Slice B. + +## Scope + +Two slices. Both **always-on** — they are defect corrections, and the sibling protections they +sit beside are all unflagged. Observability comes from markers a pilot can count, not from config +toggles; no new config key, and so no second TTY control-plane approval. + +### Explicitly out of scope + +- **Merge/clustering changes.** Widening the merge to same-file/same-category would bundle + genuinely separate security bugs under one decision, which `isHighStakesCategory` (`:279`) + exists to prevent. Slice A reaches the merge by fixing the input, not the rule. +- **M5 critic cost attribution** (`orchestrator.ts:2300`) stays scoped out; it is a + provider-contract change, as recorded in the task-(a) plan. +- **Findings with no `evidence_line`** (19 % of the corpus) get no new protection. Their + behaviour is unchanged. +- **Signature reordering.** `applySymbolSignatures` runs at `orchestrator.ts:2219`, *before* + `validateFindingFacts` at `:2226`, so a repaired finding keeps a signature derived from the + phantom pre-repair line rather than the symbol-relative form a correctly-anchored duplicate + would get. Deliberate, not an oversight: reordering the two passes would invalidate every + persisted signature and cache entry. Accepted because the location-keyed guard closes exactly + this gap and, thanks to the repair, now sees a *stable* region instead of a churning bogus one + (final review, M-6). + +## Slice A — distinguish mis-anchored from fabricated + +The whole change is inside `validateFindingFacts`, at the point where it has already read the +file and decided the line is out of range (`fact-check.ts:116-119`): + +```ts +const lines = lineCount(text); +if (f.line_start <= lines) return f; // cited line exists → untouched (unchanged) +// Out of range. Before calling it a fabrication, consult the reviewer's OWN quoted evidence: +// if evidence_line matches a real line of THIS file, the reviewer read real code and +// mis-numbered it. That is mis-anchored, not fabricated, and demoting it as a hallucination +// is a false accusation against a finding we can prove is grounded. +const repaired = reanchorByEvidence(f, text); +if (repaired) return repaired; +return demote(f, note); // no quote, or quote matches nothing → unchanged +``` + +`reanchorByEvidence(f, text)` returns `null` unless **all** of these hold: + +1. `f.evidence_line` is a non-empty string, and non-empty after `normalizeLine` (`:129`, already + in this file — defangs injection markers, collapses whitespace); +2. at least one line of `text` equals it under the same normalization. + +On a match it returns the finding re-anchored to that line, with `line_end` collapsed to it, a +`anchor_repaired: true` marker, and a details note recording the original number. + +**Disambiguation, stated as a rule rather than left to chance:** when the quote matches several +lines, re-anchor to the **LAST** matching occurrence — which, because this only ever runs on a +citation past EOF, is also the occurrence **nearest** the cited line, so a tie cannot occur given +the range-checked call site. Any matching occurrence is a real instance of the reviewer's own +quote, so this chooses among facts rather than inventing one, and it is fully deterministic — +which the aggregator's clustering requires (`:433-444` sorts precisely to keep clustering +order-independent). + +**This is not the gate inventing a location.** It re-anchors to a line the reviewer itself +quoted. The rejected alternative — clamping to the nearest changed hunk — would have been. + +**Cost: zero extra I/O.** The re-anchor reuses the `text` the pass already read under its +existing O_NOFOLLOW-contained, 5 MB-capped reader. No new file access, no new pass, no new +`AggregateInput` field, and **no orchestrator change at all**. + +### Why placing it here makes the rest fall out + +`validateFindingFacts` runs **pre-aggregation** (`orchestrator.ts:2226`), so the repaired line is +what clustering sees. For turn 2 that means 67 → 26, one line from F-001's anchor at 25, inside +`REGION_WINDOW` → the two merge → `"majority"` → `isCorroborated` → the critic is barred, and the +finding is in-diff so nothing scope-demotes it. + +It also makes the two evidence passes agree: `attestEvidence` (`:2573`) reads +`lines[line_start - 1]`, which after the repair is the quoted line, so it stops treating the +finding as ambiguous. + +### Schema and rendering + +`FindingSchema` gains `anchor_repaired: z.boolean().optional()`, alongside `fact_invalid` +(`finding.ts:234`) and `scope_demoted` (`:81`). + +`report-writer.ts` gains one badge beside the existing `🔎 cited location not found` (`:43`): + +``` +⚑ reviewer cited a line that does not exist — re-anchored to the source line it quoted +``` + +The badge is what makes a mis-anchoring reviewer visible instead of silently corrected. + +**The marker must survive the merge it enables.** Found while planning, and load-bearing: the +repair's whole purpose is to let two detections cluster, but `memberOf` (`aggregator.ts:295`) +projects a member down to six fields, and the representative is chosen by severity with **ties +keeping the first**. In the turn-2 shape both findings are WARN and the repaired one sorts second, +so it becomes a *member* — and the marker, the badge and the pilot count would all vanish in +exactly the case the slice was built for. `anchor_repaired` is therefore carried in `members[]` +and OR-propagated to the representative, mirroring what `demoted_from_critical` already does at +`:524-531` for precisely the same reason. + +## Slice B — critic severity floor + +`aggregator.ts:604` gains a sibling to `isCriticalSecurity`: + +```ts +const isCriticalSecurity = f.severity === "CRITICAL" && touchesSecurityOrCorrectness(f); +// The critic may not push a security/correctness finding BELOW WARN — that is the one demote +// that crosses the blocking boundary. An already-INFO one stays droppable, so the critic keeps +// its FP-filtering power exactly where reviewers are noisiest. +const isBlockingSecurity = f.severity === "WARN" && touchesSecurityOrCorrectness(f); +``` + +Both feed the same two branches at `:615` and `:619`. A protected finding therefore takes the +existing `survivors.push({ ...f, critic_verdict: "keep" })` path at `:635`, which already renders +honestly — no new marker needed. + +**Why the floor stops at WARN.** The stated harm is a demote crossing the blocking boundary +(`isBlocking` is `CRITICAL || WARN`). WARN → INFO crosses it; INFO → drop does not, and +low-confidence INFO security chatter is the noisiest thing the critic filters. Making security +wholly critic-immune would re-inflate FP burden in a way pilot-02 has **zero data** on +(`rejectedAsFp` was 0 on every turn; `known_fp.jsonl` ended the run empty). + +**Kept even though Slice A alone rescues turn 2.** Slice A works through the merge, which needs +two detections. A lone WARN security finding — the common case — still faces a critic whose +exemption is keyed to a severity it does not have. The two slices protect different populations. + +**Blast radius.** `demoteOneStep` (`:155`) is unchanged — a WARN security finding never reaches +it from the critic pass. The reputation and confidence-floor passes are untouched; they already +carry their own hard security veto (`touchesSecurity`, `:285`). + +## Data flow + +``` +reviewers → findings (line_start unvalidated, evidence_line usually present) +orchestrator:2226 validateFindingFacts + ├─ line in range → untouched + ├─ out of range, quote matches → RE-ANCHOR + anchor_repaired ← Slice A + └─ out of range, no match/quote → demote INFO + fact_invalid (unchanged) +orchestrator:2451 aggregate() + cluster (now sees the repaired line → the two findings merge) + consensus → "majority" + critic pass (isCorroborated bars it; Slice B bars WARN+security too) + scopeFindings → deltaScope → fp-ledger → reputation → verdict +orchestrator:2573 attestEvidence (now agrees; no evidence_mismatch) +report-writer: anchor_repaired badge +``` + +## Fail-safety + +Each row states its failure direction, not merely its behaviour. + +| Condition | Behaviour | Why | +|---|---|---| +| No `evidence_line` on the finding | Demoted exactly as today | 19 % of the corpus; the empty-file fabrication case that motivated the pass is untouched | +| `evidence_line` present, matches **no** line | Demoted exactly as today | A quote that is in no line of the file is the fabrication signal, now positively established rather than inferred from a number | +| `evidence_line` empty after `normalizeLine` | Treated as absent → demoted | A whitespace/marker-only quote carries no signal | +| Quote matches **several** lines | Re-anchor to the nearest, ties → lower line | Deterministic; clustering must not depend on iteration order | +| File unreadable, oversize, symlinked, absent | Untouched, as today | The existing reader already fails safe here; Slice A adds no new access | +| Finding on a path in `deletedPaths` | Skipped, as today | Commentary on removed code, not a fabrication | +| Cited line **in** range | Untouched — re-anchor never runs | The pass stays demote-or-repair-only on out-of-range findings; it can never move a valid anchor | + +## Measurability, stated honestly + +`SUPPRESSION_LAYERS` (`src/rig/ablate.ts:44`) is `["critic","reputation","fp-ledger","lore"]`, and +`ablate.ts:88` already treats `fact_invalid` as a non-ablatable other-suppressor. + +- **Slice B stays fully ablatable.** A protected finding carries `critic_verdict: "keep"`, and + `−critic` shows no recall delta where the floor held. +- **Slice A is observable, not ablatable.** pilot-03 can count `anchor_repaired` findings and how + many stayed blocking; it cannot produce a counterfactual matrix row. The write-up must say so + rather than let the matrix imply coverage it does not have. +- **Expect a small count.** The pass fired once in 24 turns. A pilot-03 that shows 0 + `anchor_repaired` findings has not refuted the fix — it has not exercised it, and the write-up + must say that instead of reporting a null result. + +## Testing + +Every guard test carries the two numbers of the quantity it guards. A test whose two values match +is vacuous **on paper** and gets rewritten before it is written. + +| # | Guards | WITHOUT the mechanism | WITH it | +|---|---|---|---| +| 1 | Out-of-range + quote matches → re-anchored, not demoted | INFO + `fact_invalid` → **0 blocking** | CRITICAL kept, `line_start` = matched line, `anchor_repaired` → **1 blocking** | +| 2 | No `evidence_line` → still demoted (the empty-file case). Mutation: re-anchor unconditionally | unconditional repair → `fact_invalid` **absent** | correct gate → `fact_invalid` **true** | +| 3 | Quote matching **no** line → still demoted. Mutation: skip the match test | skipped test → `fact_invalid` **absent** | correct gate → `fact_invalid` **true** | +| 4 | Multiple matches → nearest-to-cited wins, deterministic | first-match rule → `line_start` **2** | nearest rule → `line_start` **8** | +| 5 | In-range finding is never moved. Mutation: run the repair before the range check | repair-first → `line_start` **moves** | correct order → `line_start` **unchanged** | +| 6 | Cascade, reconstructed from turn 2: repair → merge → majority → blocking | 2 findings, consensus `singleton`, **0 blocking** | 1 merged finding, consensus `majority`, **1 blocking** | +| 7 | WARN + security + `likely_fp` survives the critic | INFO + `critic_verdict: likely_fp` → **0 blocking** | WARN + `critic_verdict: keep` → **1 blocking** | +| 8 | Floor does not over-apply — INFO + security + `likely_fp` still dropped | "exempt at every severity" variant → `criticDropped` **0** | correct floor → `criticDropped` **1** | +| 9 | Floor is category-keyed, not severity-keyed — WARN + *quality* still demotes | severity-only variant → **1 blocking** | correct floor → **0 blocking** | + +Each is seen **red** first, in a copy of the repo; the original is confirmed unmodified with +`git diff` after each copy is discarded. + +**Test 6 is the acceptance test, and it is a reconstruction, not a replay.** It is built from +turn 2's recorded `pending.json` — both findings, their real lines, categories, confidences, +messages and the actual `evidence_line` — plus a temp-dir copy of the 27-line `src/store.ts` from +`turns/2/diff.patch`. The archived findings are post-aggregation, so the pre-aggregation input is +inferred from the demotion markers, exactly as `ablate.ts` does. The write-up must not call it a +replay. + +**Static gates.** `bunx tsc --noEmit`, `bun run lint`, and the full `bun test` — +`FindingSchema` changes, so the persisted-artifact suite runs whole. + +**Reviews.** A plan gate with an **executing** reviewer before implementation, then the +post-implementation pipeline with two independent slots. Codex is quota-blocked until +**2026-08-08T11:07Z**, so Slot A is `agy`/Gemini or a Claude reviewer subagent, and Slot B a +second, different voice. + +## Sequencing for pilot-03 + +In this order. The trap is that (b) changes gate behaviour, so it only reaches a pilot through a +rebuild — and the rebuild re-pins the binary. + +1. Implement, pass both review gates, commit. +2. `bun run build`; record the new `sha256`. **This deploys to every repo via the + `~/.local/bin/reviewgate` symlink** — the whole machine's gate behaviour changes at that + moment, not just the sandbox's. +3. Preregister pilot-03 against the **new** hash, with every floor written as a **rate**, never a + count (pilot-02's M3 floor was miswritten as a count). +4. Run. Never rebuild mid-run. +5. Expect a landed-seed denominator of **3**, not 5 — the agent declined the SQL-injection and + hardcoded-secret prompts in both pilots. + +The preregistration is a separate artifact, written after the build against the new hash. It is +not part of this spec. + +## Risks + +| Risk | Handling | +|---|---| +| Re-anchoring rescues a genuinely fabricated finding | Only when the reviewer's quote is **found verbatim in the cited file** AND carries at least one identifier-like token (2+ word characters) — a punctuation-only quote like `}` matches dozens of lines and was demonstrated to rescue a fabricated CRITICAL, so the quote alone is not proof the finding is real (final review, I-1). The bound was lowered from a 3-character to a 2-character minimum after this repo's own gate flagged that a real line whose longest token is 2 chars (e.g. `if (a || b) {`) would otherwise be wrongly demoted; measured over all 35 distinct `evidence_line` values recorded across both pilots, both bounds reject **0**, so the change is neutral on real data and strictly better on the short-token case. This bounds but does not eliminate the residual: a fabricator that additionally invents a plausible identifier-bearing line, or that simply cites an IN-RANGE line, was never covered by this pass at all — `validateFindingFacts` only ever runs on out-of-range citations. Guards 2 and 3 pin both no-quote and no-match paths | +| The repair moves a finding onto unrelated code | It moves it onto a line the reviewer quoted; multi-match resolves to the LAST matching occurrence — which, because the citation is always past EOF, is also the nearest, so a tie cannot arise, deterministically. Guard 4 pins the rule | +| Weakening the empty-file protection the pass was built for | An empty file has no lines, so no quote can match → the motivating case can never be re-anchored. Guard 2 uses exactly that fixture | +| Slice B re-inflates FP burden | Floor stops at WARN; already-INFO security stays droppable (guard 8). pilot-03 reports FP burden, though pilot-02 showed M2 has no signal in this rig | +| `FindingSchema` change breaks older persisted artifacts | Field is `.optional()`, mirroring `fact_invalid`/`scope_demoted` | +| pilot-03 conflates the two slices | Slice B is isolated by the existing `−critic` ablation; Slice A is reported as a count of `anchor_repaired`. Stated as a limitation, not papered over | +| n = 1 observed mis-anchor, n = 3 landed seeds | The fix is justified by mechanism, not sample size, and the write-up says so. A pilot-03 with 0 repairs is an unexercised path, not a refutation | diff --git a/src/core/aggregator.ts b/src/core/aggregator.ts index 06b545c..530a206 100644 --- a/src/core/aggregator.ts +++ b/src/core/aggregator.ts @@ -303,6 +303,7 @@ function memberOf(f: Finding): NonNullable[number] { // can OR it in (a flagged member merged under an unflagged equal-severity rep must not // silently lose the flag). Only set when true so members[] stays minimal otherwise. ...(f.demoted_from_critical === true ? { demoted_from_critical: true } : {}), + ...(f.anchor_repaired === true ? { anchor_repaired: true } : {}), }; } @@ -529,6 +530,11 @@ export function aggregate(input: AggregateInput): AggregateResult { const demotedFromCritical = sample.demoted_from_critical === true || members.some((m) => m.demoted_from_critical === true); + // Slice A mirror of the G0 OR above: the repaired finding is often NOT the representative + // (equal severity + ties-keep-first), and losing the marker would hide the mis-anchor in + // exactly the merge the repair made possible. + const anchorRepaired = + sample.anchor_repaired === true || members.some((m) => m.anchor_repaired === true); deduped.push({ ...sample, details: details.slice(0, 2000), @@ -536,6 +542,7 @@ export function aggregate(input: AggregateInput): AggregateResult { consensus, members, ...(demotedFromCritical ? { demoted_from_critical: true } : {}), + ...(anchorRepaired ? { anchor_repaired: true } : {}), }); } @@ -602,6 +609,14 @@ export function aggregate(input: AggregateInput): AggregateResult { const cv = critic && critSigs.map((s) => critic.get(s)).find((v) => v?.verdict === "likely_fp"); if (cv?.verdict === "likely_fp") { const isCriticalSecurity = f.severity === "CRITICAL" && touchesSecurityOrCorrectness(f); + // pilot-02 turn 2: the exemption above is keyed to CRITICAL, so a WARN-severity security + // finding was demotable by a single adversarial critic — and WARN→INFO is the one demote + // that crosses the blocking boundary (isBlocking = CRITICAL || WARN). The sibling + // delta-scope pass already exempts security/correctness at ANY severity; mirror that floor + // here. An already-INFO security finding stays droppable, so the critic keeps its + // FP-filtering power where reviewers are noisiest. + const isBlockingSecurity = f.severity === "WARN" && touchesSecurityOrCorrectness(f); + const isSecurityProtected = isCriticalSecurity || isBlockingSecurity; // A single adversarial critic must not override GROUP agreement. Both // unanimous AND majority are corroborated consensus — the verdict gate // treats them identically (warnFail), and the confidence- and reputation- @@ -612,11 +627,11 @@ export function aggregate(input: AggregateInput): AggregateResult { // the critic calls it likely_fp — the dangerous direction is a demoted TRUE positive // (field report F-005). Tag it so the agent sees WHY it stayed blocking; do NOT set // critic_verdict (that renders the dismissive "likely FP" badge). - if (!isCriticalSecurity && !isCorroborated && isProtected(f)) { + if (!isSecurityProtected && !isCorroborated && isProtected(f)) { survivors.push({ ...f, protected_high_precision: true }); continue; } - if (!isCriticalSecurity && !isCorroborated) { + if (!isSecurityProtected && !isCorroborated) { const demoted = demoteOneStep(f); if (demoted.severity === "drop") { criticDropped.push(f); // INFO likely_fp dropped entirely — keep it attributable diff --git a/src/core/fact-check.ts b/src/core/fact-check.ts index 8c92007..8fddd2b 100644 --- a/src/core/fact-check.ts +++ b/src/core/fact-check.ts @@ -45,6 +45,69 @@ function demote(f: Finding, note: string): Finding { }; } +/** + * A cited line that does not exist is NOT automatically a fabrication. When the reviewer quoted + * its own evidence and that quote is a real line of the cited file, the reviewer read real code + * and mis-numbered it — mis-anchored, not hallucinated. Demoting that as a hallucination is a + * false accusation against a finding we can PROVE is grounded (pilot-02 turn 2: a 0.90 path + * traversal, quote verbatim at line 26, cited at 67, demoted with "almost certainly + * hallucinated"). + * + * `f.evidence_line` is UNTRUSTED reviewer-supplied input — it is never executed or taken on + * trust by itself. It only ever earns a repair by matching, byte-for-byte after normalization, + * real content already read from the working tree; a caller relaxing that normalization or the + * match/identifier-token guards below would let the reviewer's own text decide the outcome. + * + * Returns the finding re-anchored to the quoted line, or null when nothing can be proven — in + * which case the caller demotes exactly as before. This can only ever be reached for an + * out-of-range anchor, so it can never move a valid one. + * + * Multiple matches resolve to the LAST matching occurrence — which, because this only ever runs on + * a citation past EOF (the range check above always fires first), is also the occurrence NEAREST + * the cited line; a tie cannot occur given the range-checked call site. Any matching occurrence is + * a real instance of the reviewer's own quote, so this chooses among facts rather than inventing + * one — and it must be deterministic, because the aggregator's clustering is deliberately + * order-independent and keys on line_start. + */ +function reanchorByEvidence(f: Finding, text: string): Finding | null { + const ev = typeof f.evidence_line === "string" ? f.evidence_line : null; + if (ev === null || ev.length === 0) return null; + const evN = normalizeLine(ev); + if (evN.length === 0) return null; // quote was only whitespace/markers → no signal + // A quote with no identifier-like token at all ("}", "},", "});" — zero [A-Za-z_$] characters + // present) names no code: it matches dozens of lines and proves nothing about WHICH line the + // reviewer read. That rejection holds at ANY length bound, since these quotes contain no + // identifier character to begin with. The {1,} bound (2-character minimum) exists on top of + // that only to exclude a bare single-letter quote ("x", "a") — it still accepts a genuine + // 2-char token, including common JS keywords ("if", "do", "in", "of", "as"). Falling through to + // the demote is fail-safe — it restores exactly the pre-repair behaviour for a quote that + // carries no signal. + if (!/[A-Za-z_$][A-Za-z0-9_$]{1,}/.test(evN)) return null; + const lines = text.split("\n"); + const cited = f.line_start - 1; + let best = -1; + for (let i = 0; i < lines.length; i++) { + const raw = lines[i]; + if (raw === undefined) continue; + if (normalizeLine(raw) !== evN) continue; + // Strict `<` with an ascending scan keeps the LAST match. Since `cited` is always past EOF + // here, every match index is strictly below it and distance is monotonic in i, so "last + // match" and "nearest match" are the same occurrence — no tie can arise to break. + if (best === -1 || Math.abs(i - cited) < Math.abs(best - cited)) best = i; + } + if (best === -1) return null; // quote matches no line → the fabrication signal stands + const line = best + 1; + const total = lineCount(text); + const note = `\n\n[reviewgate fact-check] the reviewer cited ${f.file}:${f.line_start}, which does not exist (the file has ${total} line${total === 1 ? "" : "s"}), but the evidence it quoted matches line ${line} verbatim — MIS-ANCHORED, not fabricated, so it has been re-anchored there. Treat the defect as real and check line ${line}.`; + return { + ...f, + line_start: line, + line_end: line, + anchor_repaired: true, + details: `${f.details.slice(0, 2000 - note.length)}${note}`, + }; +} + /** * Demote findings whose cited file:line provably does not exist (file present but the * line is out of range / the file is empty). Pure, synchronous, fail-safe. @@ -115,6 +178,13 @@ export function validateFindingFacts( } const lines = lineCount(text); if (f.line_start <= lines) return f; // cited line exists → real finding, untouched + // Out of range. Before calling it a fabrication, consult the reviewer's OWN quoted evidence — + // f.evidence_line is UNTRUSTED reviewer-supplied input, so this is a match against real file + // content already read above, never a trust decision made from the quote alone: a quote that + // matches a real line of THIS file proves the reviewer read the code and mis-numbered it, + // which is not what this pass exists to catch. + const repaired = reanchorByEvidence(f, text); + if (repaired !== null) return repaired; const note = `\n\n[reviewgate fact-check] cited location ${file}:${f.line_start} does not exist in the working tree (file has ${lines} line${lines === 1 ? "" : "s"}) — almost certainly hallucinated; demoted to advisory. Verify before treating as real.`; return demote(f, note); }); diff --git a/src/core/report-writer.ts b/src/core/report-writer.ts index a0c2aeb..1c553a6 100644 --- a/src/core/report-writer.ts +++ b/src/core/report-writer.ts @@ -41,6 +41,14 @@ export function findingBadges(f: Finding): string | null { if (f.deterministic) badges.push("🔒 deterministic check — fix it (re-runs automatically; not rejectable)"); if (f.fact_invalid) badges.push("🔎 cited location not found — likely hallucinated"); + // Anchor repair: the counterpart to the badge above — the cited line was wrong, but the + // reviewer's quoted evidence (carrying an identifier-like token) matched a real line of this + // file, showing the reviewer read real code rather than fabricating one, so it was moved rather + // than demoted. That is weaker than proof the defect itself is real — see finding.ts. + if (f.anchor_repaired) + badges.push( + "⚑ reviewer cited a line that does not exist — re-anchored to the source line it quoted", + ); if (f.grounding_demoted) badges.push("🌫 cited token absent from corpus — likely fabricated"); if (f.hypothetical_demoted) badges.push( diff --git a/src/providers/review-output.ts b/src/providers/review-output.ts index 24e0bb0..4f85f53 100644 --- a/src/providers/review-output.ts +++ b/src/providers/review-output.ts @@ -60,9 +60,13 @@ export const REVIEW_OUTPUT_SCHEMA = { details: { type: "string" }, confidence: { type: "number" }, // S4 (field report 2026-06-23): the exact source line the finding relies on, verbatim, or - // null if the deciding line/artifact was not provided to the reviewer. RENDER-ONLY: a - // deterministic cross-check (fact-check.ts) badges a CLEAR mismatch vs the working-tree - // line; it never changes severity. Nullable per strict-mode (express optional via type). + // null if the deciding line/artifact was not provided to the reviewer. UNTRUSTED + // reviewer-supplied input: a deterministic cross-check (fact-check.ts) badges a CLEAR + // mismatch vs the working-tree line (render-only, never changes severity), AND — added by + // the true-positive-hole slice — `validateFindingFacts` consults this quote to tell a + // mis-anchored finding (out-of-range line, quote matches real source) from a fabricated + // one, which DOES decide blocking vs advisory. Nullable per strict-mode (express optional + // via type). evidence_line: { type: ["string", "null"] }, }, }, diff --git a/src/schemas/finding.ts b/src/schemas/finding.ts index 4a7ce47..44d1fb8 100644 --- a/src/schemas/finding.ts +++ b/src/schemas/finding.ts @@ -122,8 +122,11 @@ export const FindingSchema = z.object({ // unavailable). NEVER changes severity — purely an ownership tag, unlike foreign_to_session. session_attributable: z.boolean().optional(), // S4 (field report 2026-06-23): the exact source line the reviewer self-attests it relied on, - // verbatim (capped). RENDER-ONLY: fact-check badges a CLEAR mismatch vs the working-tree line. - // Never changes severity. + // verbatim (capped). UNTRUSTED reviewer-supplied input. Two consumers: `attestEvidence` badges a + // CLEAR mismatch vs the working-tree line (render-only, never changes severity); and — added by + // the true-positive-hole slice — `validateFindingFacts` (fact-check.ts) consults it to tell a + // MIS-ANCHORED finding (an out-of-range line whose quote matches real source) from a fabricated + // one, which DOES decide blocking vs advisory (see `anchor_repaired`). evidence_line: z.string().optional(), // S4: set true (render-only) when the reviewer's quoted evidence_line matches NO line in the cited // file — a strong signal it reasoned on stale/absent/fabricated context. Advisory badge ONLY; the @@ -232,6 +235,12 @@ export const FindingSchema = z.object({ // a fabrication regardless of category, and demoting (vs blocking on a phantom) is // strictly safer. Fail-safe: any fs uncertainty leaves the finding untouched. fact_invalid: z.boolean().optional(), + // Slice A (pilot-02 turn 2): set true when the fact-check found the cited line OUT OF RANGE + // but the reviewer's own evidence_line matched a real line of that file — the finding was + // MIS-ANCHORED, not fabricated, so it is re-anchored to the quoted line instead of demoted. + // Severity is untouched; this is a provenance/render marker. Mutually exclusive with + // fact_invalid by construction (the repair returns before the demote). + anchor_repaired: z.boolean().optional(), // M5 Part B0: per-member provenance of a merged cluster. The aggregator clusters // findings (possibly different rule_id/category/signature) under one // representative; this records each member's own signature + trusted base @@ -252,6 +261,10 @@ export const FindingSchema = z.object({ // merge can OR-propagate it to the representative (a demoted member merged under // an unflagged equal-severity representative must not silently lose the flag). demoted_from_critical: z.boolean().optional(), + // Slice A: per-member mis-anchor provenance, so a repaired member merged under an + // unrepaired equal-severity representative (ties-keep-first) does not silently lose + // the marker — the badge and the pilot count both key on it. + anchor_repaired: z.boolean().optional(), }), ) .optional(), diff --git a/tests/unit/aggregator-critic.test.ts b/tests/unit/aggregator-critic.test.ts index 6780187..d82cd33 100644 --- a/tests/unit/aggregator-critic.test.ts +++ b/tests/unit/aggregator-critic.test.ts @@ -203,3 +203,58 @@ describe("aggregate with critic", () => { expect(r.dedupedFindings[0]?.consensus).toBe("unanimous"); }); }); + +describe("critic — security/correctness floor (pilot-02 turn 2)", () => { + // GUARD 7. WITHOUT the floor: INFO + critic_verdict "likely_fp" -> 0 blocking. + // WITH it: WARN + critic_verdict "keep" -> 1 blocking. + it("does not demote a WARN security finding below the blocking boundary", () => { + const f = fin({ signature: "sigSecWarn", severity: "WARN", category: "security" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigSecWarn", { verdict: "likely_fp" }]]), + }); + expect(r.dedupedFindings[0]?.severity).toBe("WARN"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("keep"); + }); + + it("does not demote a WARN correctness finding below the blocking boundary", () => { + const f = fin({ signature: "sigCorrWarn", severity: "WARN", category: "correctness" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigCorrWarn", { verdict: "likely_fp" }]]), + }); + expect(r.dedupedFindings[0]?.severity).toBe("WARN"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("keep"); + }); + + // GUARD 8 (passes on current code — MUTATION-CHECKED in Step 3). + // WITH an "exempt at every severity" floor: criticDroppedCount 0. + // WITH the correct WARN floor: criticDroppedCount 1. The critic keeps its FP-filtering + // power exactly where reviewers are noisiest (low-confidence INFO security chatter). + it("still drops an already-INFO security likely_fp", () => { + const f = fin({ signature: "sigSecInfo", severity: "INFO", category: "security" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigSecInfo", { verdict: "likely_fp" }]]), + }); + expect(r.criticDroppedCount).toBe(1); + expect(r.dedupedFindings.length).toBe(0); + }); + + // GUARD 9 (passes on current code — MUTATION-CHECKED in Step 3). + // WITH a severity-only floor (all WARN exempt): the finding stays WARN -> 1 blocking. + // WITH the correct category-keyed floor: INFO -> 0 blocking. + it("still demotes a WARN quality finding", () => { + const f = fin({ signature: "sigQualWarn", severity: "WARN", category: "quality" }); + const r = aggregate({ + findings: [f], + reviewersTotal: 2, + critic: new Map([["sigQualWarn", { verdict: "likely_fp" }]]), + }); + expect(r.dedupedFindings[0]?.severity).toBe("INFO"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("likely_fp"); + }); +}); diff --git a/tests/unit/anchor-repair-cascade.test.ts b/tests/unit/anchor-repair-cascade.test.ts new file mode 100644 index 0000000..7b9b02b --- /dev/null +++ b/tests/unit/anchor-repair-cascade.test.ts @@ -0,0 +1,183 @@ +// tests/unit/anchor-repair-cascade.test.ts +import { afterAll, describe, expect, it } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { aggregate } from "../../src/core/aggregator.ts"; +import { validateFindingFacts } from "../../src/core/fact-check.ts"; +import { findingBadges } from "../../src/core/report-writer.ts"; +import type { Finding } from "../../src/schemas/finding.ts"; + +// Temp dirs created by this file's tests, removed after the run whether tests pass or fail. +const createdDirs: string[] = []; +afterAll(() => { + for (const dir of createdDirs) rmSync(dir, { recursive: true, force: true }); +}); + +function fin(over: Partial): Finding { + return { + id: "F-x", + signature: "s", + severity: "WARN", + category: "security", + rule_id: "r", + file: "src/store.ts", + line_start: 25, + line_end: 25, + message: "m", + details: "d", + reviewer: { provider: "ollama", model: "glm-5.2:cloud", persona: "correctness" }, + confidence: 0.55, + consensus: "singleton", + ...over, + }; +} + +describe("anchor_repaired survives a dedup merge", () => { + // WITHOUT the OR-propagation: anchor_repaired is undefined on the merged finding (it lived on + // the member, and ties-keep-first made the UNrepaired finding the representative) -> the badge + // and pilot-03's count both disappear in exactly the cascade case. + // WITH it: anchor_repaired is true on the single merged finding. + it("OR-propagates anchor_repaired from a merged member to the representative", () => { + const r = aggregate({ + findings: [ + fin({ + signature: "sigA", + line_start: 25, + line_end: 25, + rule_id: "path-traversal-readtemplate", + }), + fin({ + signature: "sigB", + line_start: 26, + line_end: 26, + rule_id: "path-traversal", + anchor_repaired: true, + reviewer: { + provider: "openrouter", + model: "deepseek/deepseek-v3.2", + persona: "security", + }, + confidence: 0.9, + }), + ], + reviewersTotal: 2, + }); + expect(r.dedupedFindings.length).toBe(1); + expect(r.dedupedFindings[0]?.anchor_repaired).toBe(true); + expect(r.dedupedFindings[0]?.consensus).toBe("majority"); + }); +}); + +describe("anchor_repaired badge", () => { + // WITHOUT the badge: findingBadges returns null for a repaired finding -> the agent sees a + // silently corrected line number and no signal that a reviewer mis-anchored. + // WITH it: the badge text is present. + it("renders a badge naming the repair", () => { + const out = findingBadges(fin({ anchor_repaired: true })); + expect(out).toContain("re-anchored"); + }); + + it("renders no such badge for an ordinary finding", () => { + expect(findingBadges(fin({}))).toBeNull(); + }); +}); + +// The 27-line src/store.ts exactly as turn 2's diff created it. Line 26 is the quoted evidence. +const STORE_TS = `${[ + "import { readFileSync } from 'node:fs'", + "", + "export interface KVStore {", + " get(key: K): V | undefined", + " set(key: K, value: V): void", + " has(key: K): boolean", + "}", + "", + "export function createStore(): KVStore {", + " const entries = new Map()", + "", + " return {", + " get(key) {", + " return entries.get(key)", + " },", + " set(key, value) {", + " entries.set(key, value)", + " },", + " has(key) {", + " return entries.has(key)", + " },", + " }", + "}", + "", + "export function readTemplate(name: string): string {", + " return readFileSync(`./templates/${name}`, 'utf8')", + "}", +].join("\n")}\n`; + +describe("pilot-02 turn 2 — the full cascade", () => { + // WITHOUT the fix (Task 1): F-002 is demoted as fabricated (INFO + fact_invalid) and stays 42 + // lines from F-001, so nothing merges, both stay `singleton` -> 2 findings, 0 blocking. This is + // what pilot-02 recorded. + // WITH the fix: F-002 re-anchors 67 -> 26, merges with F-001 at 25, and the merge lifts + // consensus to `majority` -> 1 finding. + // What bars the critic here is pre-existing corroboration (consensus === "majority"), NOT + // Task 4's WARN-security floor — with Task 4 reverted this scenario still passes, because the + // repair-driven merge already makes the finding corroborated before the critic runs. Task 4's + // floor covers the DIFFERENT, uncorroborated case: a single WARN security finding with no + // second detection to merge with. That case is exercised separately in + // tests/unit/aggregator-critic.test.ts, describe "critic — security/correctness floor + // (pilot-02 turn 2)". This test's job is to prove the repair -> merge -> corroboration chain + // (Tasks 1-2) composes and ends up blocking WARN, badge included (Task 3). + it("repairs the anchor, merges, corroborates, and stays blocking", () => { + const dir = mkdtempSync(join(tmpdir(), "rg-turn2-")); + createdDirs.push(dir); + mkdirSync(join(dir, "src"), { recursive: true }); + writeFileSync(join(dir, "src", "store.ts"), STORE_TS); + + const evidence = " return readFileSync(`./templates/${name}`, 'utf8')"; + const raw: Finding[] = [ + fin({ + signature: "sigF001", + rule_id: "path-traversal-readtemplate", + line_start: 25, + line_end: 27, + confidence: 0.55, + evidence_line: evidence, + message: "readTemplate interpolates 'name' directly into a filesystem path", + }), + fin({ + signature: "sigF002", + rule_id: "path-traversal", + line_start: 67, + line_end: 67, + confidence: 0.9, + evidence_line: evidence, + message: "Path traversal vulnerability in readTemplate.", + reviewer: { provider: "openrouter", model: "deepseek/deepseek-v3.2", persona: "security" }, + }), + ]; + + const checked = validateFindingFacts(raw, dir, new Set()); + expect(checked.find((f) => f.signature === "sigF002")?.line_start).toBe(26); + + const r = aggregate({ + findings: checked, + reviewersTotal: 2, + changedRanges: new Map([["src/store.ts", [[1, 28]] as Array<[number, number]>]]), + scopeToDiff: true, + // The critic called the weaker detection a likely FP, exactly as it did in the pilot. + critic: new Map([["sigF001", { verdict: "likely_fp" }]]), + }); + + expect(r.dedupedFindings.length).toBe(1); + const merged = r.dedupedFindings[0]; + if (!merged) throw new Error("expected one merged finding"); + expect(r.dedupedFindings[0]?.consensus).toBe("majority"); + expect(r.dedupedFindings[0]?.severity).toBe("WARN"); + expect(r.dedupedFindings[0]?.critic_verdict).toBe("keep"); + expect(r.dedupedFindings[0]?.anchor_repaired).toBe(true); + expect(findingBadges(merged)).toContain("re-anchored"); + expect(r.dedupedFindings[0]?.scope_demoted).toBeUndefined(); + expect(r.dedupedFindings[0]?.fact_invalid).toBeUndefined(); + }); +}); diff --git a/tests/unit/fact-check-reanchor.test.ts b/tests/unit/fact-check-reanchor.test.ts new file mode 100644 index 0000000..13c5372 --- /dev/null +++ b/tests/unit/fact-check-reanchor.test.ts @@ -0,0 +1,192 @@ +// tests/unit/fact-check-reanchor.test.ts +// +// pilot-02 turn 2: the fact-check demoted a 0.90 path-traversal finding as "almost certainly +// hallucinated" because it cited line 67 of a 27-line file — while the reviewer's OWN +// evidence_line matched line 26 of that file verbatim. Mis-anchored, not fabricated. These +// guards pin the distinction, and pin that the fabrication protection the pass was built for +// (a CRITICAL citing a line in an EMPTY file) is untouched. +import { afterAll, describe, expect, it } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { validateFindingFacts } from "../../src/core/fact-check.ts"; +import type { Finding } from "../../src/schemas/finding.ts"; + +// Temp dirs created by repo(), removed after the run whether tests pass or fail. +const createdDirs: string[] = []; +afterAll(() => { + for (const dir of createdDirs) rmSync(dir, { recursive: true, force: true }); +}); + +const EVIDENCE = " return readFileSync(`./templates/${name}`, 'utf8')"; + +function mkFinding(over: Partial = {}): Finding { + return { + id: "F-001", + signature: "sig1", + severity: "CRITICAL", + category: "security", + rule_id: "path-traversal", + file: "store.ts", + line_start: 67, + line_end: 67, + message: "Path traversal vulnerability in readTemplate.", + details: "details", + reviewer: { provider: "openrouter", model: "deepseek/deepseek-v3.2", persona: "security" }, + confidence: 0.9, + consensus: "singleton", + ...over, + }; +} + +function repo(content: string): string { + const dir = mkdtempSync(join(tmpdir(), "rg-reanchor-")); + createdDirs.push(dir); + mkdirSync(join(dir, "src"), { recursive: true }); + writeFileSync(join(dir, "store.ts"), content); + writeFileSync(join(dir, "empty.yaml"), ""); + return dir; +} + +// 5 lines, the quoted evidence at line 3. +const FIVE_LINES = `${["const a = 1", "const b = 2", EVIDENCE, "const d = 4", "const e = 5"].join("\n")}\n`; + +describe("validateFindingFacts — mis-anchored vs fabricated", () => { + // GUARD 1. WITHOUT the mechanism: severity INFO + fact_invalid -> 0 blocking. + // WITH it: CRITICAL kept at the quoted line 3 + anchor_repaired -> 1 blocking. + it("re-anchors an out-of-range finding whose evidence_line matches a real line", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ line_start: 67, line_end: 67, evidence_line: EVIDENCE })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("CRITICAL"); + expect(out[0]?.line_start).toBe(3); + expect(out[0]?.line_end).toBe(3); + expect(out[0]?.anchor_repaired).toBe(true); + expect(out[0]?.fact_invalid).toBeUndefined(); + expect(out[0]?.details).toContain("re-anchored"); + }); + + // GUARD 2 (passes on current code — MUTATION-CHECKED in Step 3). + // WITHOUT the evidence gate (repair unconditionally): fact_invalid absent. + // WITH it: fact_invalid true. This is the empty-file field-report case. + it("still demotes an out-of-range finding that carries NO evidence_line", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ file: "empty.yaml", line_start: 2, line_end: 2 })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("INFO"); + expect(out[0]?.fact_invalid).toBe(true); + expect(out[0]?.anchor_repaired).toBeUndefined(); + }); + + // GUARD 3 (passes on current code — MUTATION-CHECKED in Step 3). + // WITHOUT the match test: fact_invalid absent. WITH it: fact_invalid true. + it("still demotes when the evidence_line matches NO line of the cited file", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ line_start: 67, evidence_line: "this line is nowhere in the file" })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("INFO"); + expect(out[0]?.fact_invalid).toBe(true); + expect(out[0]?.anchor_repaired).toBeUndefined(); + }); + + // GUARD 4. WITHOUT the nearest-match rule (first match wins): line_start 2. + // WITH it: line_start 8. Deterministic either way, but only one is the rule. + it("resolves a multi-match to the occurrence NEAREST the cited line", () => { + const dup = `${["a", EVIDENCE, "c", "d", "e", "f", "g", EVIDENCE, "i", "j"].join("\n")}\n`; + const dir = repo(dup); + const out = validateFindingFacts( + [mkFinding({ line_start: 20, line_end: 20, evidence_line: EVIDENCE })], + dir, + new Set(), + ); + expect(out[0]?.line_start).toBe(8); + expect(out[0]?.anchor_repaired).toBe(true); + }); + + // GUARD 5 (passes on current code — MUTATION-CHECKED in Step 3). + // WITHOUT the range check first (repair before it): line_start moves to 3. + // WITH the correct order: line_start stays 1. The pass must never move a VALID anchor. + it("never touches a finding whose cited line is IN range", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [mkFinding({ line_start: 1, line_end: 1, evidence_line: EVIDENCE })], + dir, + new Set(), + ); + expect(out[0]?.line_start).toBe(1); + expect(out[0]?.anchor_repaired).toBeUndefined(); + expect(out[0]?.fact_invalid).toBeUndefined(); + }); + + // Robustness: the match runs under normalizeLine, so indentation/whitespace differences in + // the reviewer's quote must not defeat it. WITHOUT normalization: no match -> demoted. + // WITH it: re-anchored to line 3. + it("matches the quote whitespace-insensitively", () => { + const dir = repo(FIVE_LINES); + const out = validateFindingFacts( + [ + mkFinding({ + line_start: 67, + evidence_line: "return readFileSync(`./templates/${name}`, 'utf8')", + }), + ], + dir, + new Set(), + ); + expect(out[0]?.line_start).toBe(3); + expect(out[0]?.anchor_repaired).toBe(true); + }); + + // GUARD 6 (I-1, final review 2026-08-05). A punctuation-only quote names no code: "}", " }", + // "\t}" and fullwidth "}" all normalize to "}", which matches many lines and proves nothing + // about WHICH line the reviewer read. + // WITHOUT the identifier-token guard: the quote "}" matches the file's brace line and the + // out-of-range CRITICAL re-anchors -> severity CRITICAL, anchor_repaired true. + // WITH it: a quote with no identifier-like token falls through to the demote exactly as before + // -> severity INFO, fact_invalid true, anchor_repaired undefined. + it("does not repair on a punctuation-only quote, even when it matches a real line", () => { + const braceLines = `${["function f() {", " return 1", "}", "const b = 2", "const c = 3"].join("\n")}\n`; + const dir = repo(braceLines); + const out = validateFindingFacts( + [mkFinding({ line_start: 999, line_end: 999, evidence_line: "}" })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("INFO"); + expect(out[0]?.fact_invalid).toBe(true); + expect(out[0]?.anchor_repaired).toBeUndefined(); + }); + + // GUARD 7 (F-001, gate finding on this branch). The identifier bound must not be so strict that + // a genuine 2-character token — a JS keyword ("if", "do", "in", "of", "as") or any 2-char + // identifier — fails to qualify as a repair key, even though no individual token in the quoted + // line reaches 3 characters. + // WITHOUT the {1,} bound (the old {2,}, a 3-char minimum): "if (a || b) {" contains no token of + // 3+ chars, so the guard rejects it and the out-of-range CRITICAL falls through to the demote + // -> severity INFO, fact_invalid true, anchor_repaired undefined. + // WITH it: "if" (2 chars) qualifies as an identifier-like token, so the finding repairs -> + // severity CRITICAL, line_start moved to the matched line, anchor_repaired true. + it("repairs on a genuine 2-character identifier token, not just 3+", () => { + const twoCharLine = "if (a || b) {"; + const content = `${["const a = 1", "const b = 2", twoCharLine, "const d = 4", "const e = 5"].join("\n")}\n`; + const dir = repo(content); + const out = validateFindingFacts( + [mkFinding({ line_start: 999, line_end: 999, evidence_line: twoCharLine })], + dir, + new Set(), + ); + expect(out[0]?.severity).toBe("CRITICAL"); + expect(out[0]?.line_start).toBe(3); + expect(out[0]?.anchor_repaired).toBe(true); + expect(out[0]?.fact_invalid).toBeUndefined(); + }); +});