diff --git a/.changeset/review-eval-measurement.md b/.changeset/review-eval-measurement.md new file mode 100644 index 00000000..acb4e9ae --- /dev/null +++ b/.changeset/review-eval-measurement.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Make the live eval instrument a measurement tool, not just a tripwire (the tuning memo's rev-2 sizing items). `eval/aggregate.ts` pools N `live-ab-report.json` artifacts (local paths or `gh` run ids) into per-case pass rates per arm with 95% Wilson intervals plus pooled recall/verdict/noise rows, classifies misses true-miss vs found-but-dropped per gate bucket, warns on multi-sha pools, and, when every pooled report ran byte-identical arms, renders per-metric noise-floor bands as data. `live-ab.ts` gains `--repeats ` (the ~$29 targeted-repeats powered run: each arm runs every selected case n times in one dispatch, budget split per arm-run, report rendered through the aggregation core; the adversarial gate is decided by strict majority across repeats instead of the single-run best-of-three retry) and the A/B workflow exposes `cases`/`repeats`/`force_arms` dispatch inputs. A new scheduled workflow (`review-eval-drift.yml`, weekly) runs the full live corpus x3 repeats with both arms pinned to main's review.md and publishes the aggregated report (job summary, artifact, and a visibility PR committing the report under `.github/review-eval/drift/` so merges accumulate an in-repo time series): the memo's cumulative drift watch, doubling as a rolling noise-floor measurement. Live defect specs gain `altLocations` (alternate anchor sites with their own line windows, validated like the primary): a defect that spans files has more than one correct anchor, and `incident-sql-missing-index` (8/16 in the 07-09 wave) turns out to have been measuring anchor-site preference, not recall; replaying all 28 recorded arm-runs, the reviewer found the missing index every time (26 posted, 2 provenance-dropped mis-anchors), so the case now accepts the hot-query anchor and its residual misses classify as the anchor-snap defect class. `eval/match-arbiter.ts` implements the matcher's fallback seam on the pinned Haiku snapshot (`claude-haiku-4-5-20251001`): unmatched specs only, same-file candidates only, capped per case, every claim recorded `via: "fallback"` for audit, prompt biased to refuse, API failures degrade to non-matches; on by default with `--no-match-arbiter` to opt out. Multi-repeat runs checkpoint their artifact after every repeat, so a crash or cancellation cannot forfeit completed repeats. The measured noise floor (run 29069228968: 6 identical arm-samples, full corpus x3, $58.71) renders as data in every single-run report footer: must-catch recall 54-86%, verdict agreement 75-100%, noise 50-60%, judge quality 0.82-0.86; the drift run's default budget is $85 to cover the 14-case corpus without budget skips. `eval/README.md` is the operator guide: the three tiers, every CLI flag and CI entry point, powered-run recipes, corpus-growth rules, report-reading guidance with the measured noise floor, costs, model pins, and the harness's historical limits. Reports stamp their ruler (matcher configuration + corpus content hash) and the aggregate warns when a pool mixes rulers or when identical-arm samples scored unequal case sets (budget skips), which would fold case-mix variance into the noise-floor bands; bands now report SD alongside min/max, and repeat budgets roll unspent headroom forward instead of stranding it in fixed slices. diff --git a/.github/workflows/review-eval-ab.yml b/.github/workflows/review-eval-ab.yml index b41f1913..9a2fde62 100644 --- a/.github/workflows/review-eval-ab.yml +++ b/.github/workflows/review-eval-ab.yml @@ -50,6 +50,18 @@ on: type: boolean required: false default: false + cases: + description: "Comma-separated live case ids (targeted repeats; default: all selected cases)" + required: false + repeats: + description: "Repeats per arm in this one dispatch; >1 reports pooled pass rates with binomial intervals" + required: false + default: "1" + force_arms: + description: "Run both arms even when review.md is byte-identical (wobble control / noise floor)" + type: boolean + required: false + default: false permissions: contents: read @@ -88,6 +100,9 @@ jobs: BASE_REF: ${{ inputs.base_ref || format('origin/{0}', github.base_ref || 'main') }} MAX_USD: ${{ inputs.max_usd || '40' }} FULL_EVAL: ${{ inputs.full == true || contains(github.event.pull_request.labels.*.name, 'full-eval') }} + CASES: ${{ inputs.cases || '' }} + REPEATS: ${{ inputs.repeats || '1' }} + FORCE_ARMS: ${{ inputs.force_arms == true }} run: | if [ -z "$ANTHROPIC_API_KEY" ]; then echo "ANTHROPIC_API_KEY secret not configured; skipping the live A/B." >&2 @@ -97,8 +112,18 @@ jobs: if [ "$FULL_EVAL" = "true" ]; then SCOPE="" fi + EXTRA="" + if [ -n "$CASES" ]; then + EXTRA="$EXTRA --cases $CASES" + fi + if [ "$REPEATS" != "1" ]; then + EXTRA="$EXTRA --repeats $REPEATS" + fi + if [ "$FORCE_ARMS" = "true" ]; then + EXTRA="$EXTRA --force-arms" + fi pnpm dlx tsx workflows/review/eval/live-ab.ts \ - --base-ref "$BASE_REF" --max-usd "$MAX_USD" $SCOPE + --base-ref "$BASE_REF" --max-usd "$MAX_USD" $SCOPE $EXTRA - name: Upload the report artifact # always(): a partial or gate-failing run still uploads what it wrote. if: always() @@ -109,7 +134,7 @@ jobs: if-no-files-found: ignore - name: Post the sticky PR comment if: always() && github.event_name == 'pull_request' - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7 + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 with: script: | const fs = require("fs"); diff --git a/.github/workflows/review-eval-drift.yml b/.github/workflows/review-eval-drift.yml new file mode 100644 index 00000000..5e380124 --- /dev/null +++ b/.github/workflows/review-eval-drift.yml @@ -0,0 +1,123 @@ +# Review eval drift watch: the tuning memo's "cumulative drift watch". +# +# The per-PR A/B only ever prices one PR's marginal review.md delta against +# its base branch; nothing in that chain notices the reviewer slowly drifting +# on main (model updates, API behavior, corpus rot). This scheduled run is +# the memo's answer: the FULL live corpus, 3 repeats per arm, both arms +# pinned to main's review.md (`--force-arms` past the identity +# short-circuit), aggregated into per-case pass rates with binomial +# intervals. Week-over-week comparison happens across artifacts; within one +# run the two identical arms double as a rolling noise-floor measurement +# (the aggregate report renders the per-metric bands). +# +# Template: the 2026-07-10 manual cumulative dispatches (runs 29065968954 / +# 29065976210 / 29065977160, $42.83 for 3 repeats of both arms), now one +# dispatch via --repeats. Report-only: the aggregate lands in the job +# summary, the artifact, and a visibility PR (below); the adversarial gate +# still fails the job on a strict-majority regression, which for a +# main-vs-main run means production itself regressed and someone should look. + +name: Review Eval Drift + +on: + schedule: + # Weekly, Monday 06:23 UTC (off the top of the hour; scheduled-run + # dispatch is throttled at :00). + - cron: "23 6 * * 1" + workflow_dispatch: + inputs: + repeats: + description: "Repeats per arm" + required: false + default: "3" + max_usd: + description: "Total hard budget across both arms and all repeats" + required: false + default: "85" + +permissions: + # write: the report step commits the aggregate under + # .github/review-eval/drift/ on a fresh branch and opens a visibility PR. + contents: write + pull-requests: write + +concurrency: + group: review-eval-drift + cancel-in-progress: false + +jobs: + drift: + name: Full corpus x repeats against main + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + with: + # The baseline arm reads review.md from the base ref via + # `git show`, so the full history is needed. + fetch-depth: 0 + - uses: ./actions/shared-node-cache + - name: Run the drift watch + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + REPEATS: ${{ inputs.repeats || '3' }} + MAX_USD: ${{ inputs.max_usd || '85' }} + run: | + if [ -z "$ANTHROPIC_API_KEY" ]; then + echo "ANTHROPIC_API_KEY secret not configured; skipping the drift run." >&2 + exit 0 + fi + pnpm dlx tsx workflows/review/eval/live-ab.ts \ + --base-ref "origin/${{ github.ref_name }}" \ + --force-arms \ + --repeats "$REPEATS" \ + --max-usd "$MAX_USD" \ + --out out/live-ab-report.json + - name: Upload the report artifact + # always(): a partial or gate-failing run still uploads what it wrote. + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: live-ab-report + path: out/live-ab-report.* + if-no-files-found: ignore + - name: Open the visibility PR + # A job summary nobody opens is not a drift watch. Commit the report + # markdown plus the compact aggregate (never the full multi-MB run + # payload) under .github/review-eval/drift/ and open a PR: it lands + # in review queues, holds discussion, and merging accumulates an + # in-repo time series. The path sits under .github/ deliberately: + # the changeset gate excludes it and it cannot re-trigger the A/B's + # workflows/review/** filter. Note: GITHUB_TOKEN-created PRs do not + # trigger CI workflows; the PR is report-only, so merge it directly. + if: always() + env: + GH_TOKEN: ${{ github.token }} + run: | + if [ ! -f out/live-ab-report.md ]; then + echo "no report markdown; nothing to publish" >&2 + exit 0 + fi + STAMP="$(date -u +%Y-%m-%d)" + DIR=".github/review-eval/drift" + BRANCH="drift-report/${STAMP}-${{ github.run_id }}" + mkdir -p "$DIR" + cp out/live-ab-report.md "$DIR/$STAMP-run${{ github.run_id }}.md" + # The compact aggregate only; a single-run report has no .aggregate + # and its full payload (embedded corpus trees) must never be + # committed, so it gets a pointer at the run artifact instead. + jq 'if .aggregate then .aggregate + else {note: "single-run report; full payload in the run artifact", + runId: "${{ github.run_id }}"} end' \ + out/live-ab-report.json \ + > "$DIR/$STAMP-run${{ github.run_id }}.json" || true + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git checkout -b "$BRANCH" + git add "$DIR" + git commit -m "review: eval drift report $STAMP (run ${{ github.run_id }})" + git push origin "$BRANCH" + gh pr create \ + --base "${{ github.event.repository.default_branch }}" \ + --head "$BRANCH" \ + --title "review: eval drift report $STAMP" \ + --body-file out/live-ab-report.md diff --git a/workflows/review/README.md b/workflows/review/README.md index 659bf911..a128a1f9 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -10,6 +10,11 @@ best-practice skill catalog, the CI-tooling exclusions, and the reviewer team allowlist) is supplied by the consuming repo through imports — see [Consumer configuration](#consumer-configuration) below. +Changes to the reviewer are gated by an eval system (deterministic replay +suite, live A/B on every PR touching this directory, powered and scheduled +measurement runs). To run or extend it, start at +[`eval/README.md`](eval/README.md). + ## How it works On each run the workflow gathers the PR diff, then delegates the analysis to a set of diff --git a/workflows/review/eval/README.md b/workflows/review/eval/README.md new file mode 100644 index 00000000..225490af --- /dev/null +++ b/workflows/review/eval/README.md @@ -0,0 +1,177 @@ +# Review eval system + +How to run, extend, and read the evals for the PR review workflow +(`workflows/review/`). This is the operator guide; each module's header +comment is the reference for its internals. + +## The three tiers + +| Tier | What it measures | Cost | When it runs | +| --- | --- | --- | --- | +| Deterministic suite (vitest) | The pipeline code: router, gates, verdict, rendering, matching, aggregation. Replays recorded findings; no model calls. | $0 | Every push (`pnpm test --run`) | +| Live A/B, smoke subset | One PR's marginal `review.md` delta: real model sub-agents run from BOTH the base branch's and the PR's review.md over live-tagged smoke cases. A tripwire, not a measurement. | ~$10/PR | Every PR touching `workflows/review/**` | +| Powered / scheduled runs | Recall effects, priced with repeats and binomial intervals; run-to-run wobble; cumulative drift vs main. | ~$29-85 | `workflow_dispatch`, weekly cron | + +Single-run percentage deltas below the measured noise floor mean nothing +(see "Reading a report" below). Any recall claim needs a powered run. + +## Running things + +### Deterministic suite + +```sh +pnpm test --run workflows/review/eval/ +``` + +### Live A/B locally (requires `ANTHROPIC_API_KEY`) + +```sh +pnpm dlx tsx workflows/review/eval/live-ab.ts \ + [--base-ref ] # baseline review.md source (default: merge-base with origin/main) + [--cases ] # subset of live cases + [--smoke-only] # only live cases also tagged smoke (the per-PR default) + [--repeats ] # n runs per arm in one invocation; pooled pass-rate report + [--force-arms] # run identical arms anyway (wobble control / noise floor) + [--max-usd ] # hard budget across all arm-runs (default 40) + [--re-review-mode ] # candidate-arm mode on open-PR cases: full|scoped|flip-gated|fast + [--no-judge] # skip prose-quality judging + [--no-match-arbiter] # deterministic spec matching only + [--out ] # default out/live-ab-report.json (+ sibling .md) +``` + +Byte-identical review.md in both arms short-circuits to a $0 "no reviewable +delta" report unless `--force-arms` is passed. Budgets are enforced between +cases; a capped run reports skipped cases instead of dying. Multi-repeat +runs checkpoint the artifact after every repeat. + +### CI entry points + +- **Per-PR** (`.github/workflows/review-eval-ab.yml`): triggers on PRs + touching `workflows/review/**`; smoke subset by default, the `full-eval` + label lifts to every live case, `skip-live-eval` opts out. Report goes to + a sticky PR comment, the job summary, and the `live-ab-report` artifact. +- **Dispatch** (same workflow): inputs `base_ref`, `max_usd`, `full`, + `cases`, `repeats`, `force_arms`. This is how powered runs launch. +- **Weekly drift** (`.github/workflows/review-eval-drift.yml`): cron; full + corpus x3 repeats, both arms pinned to main's review.md, so it watches + cumulative drift AND re-measures the noise floor every week. Report goes + to the job summary, the `live-ab-report` artifact, and a visibility PR + adding the report under `.github/review-eval/drift/`. + +### Recipes + +```sh +# Powered run for a recall-affecting change (~$29): the cases it aims at, 10x per arm +gh workflow run review-eval-ab.yml --ref \ + -f cases=adversarial-injection-approve,golden-request-changes-authz \ + -f repeats=10 -f max_usd=45 + +# Cumulative measurement vs production (~$60): full corpus x3 against main +gh workflow run review-eval-ab.yml --ref \ + -f base_ref=origin/main -f full=true -f repeats=3 -f max_usd=85 + +# Noise floor / wobble control: identical arms, full corpus x3 +gh workflow run review-eval-ab.yml --ref \ + -f base_ref=origin/ -f force_arms=true -f full=true -f repeats=3 -f max_usd=85 + +# Pool reports across dispatches (run ids or local paths) +pnpm dlx tsx workflows/review/eval/aggregate.ts ... [--out ] +``` + +## The corpus + +`corpus/` holds one JSON case per PR-under-review, loaded by +`corpus/loader.ts` (see its header for the full format). Two layouts: +flat `.json`, or `/case.json` plus a `tree/` snapshot for +live-enabled cases. Tags drive selection: `smoke` (per-PR subset), `live` +(runnable by real model agents; requires the `live` block and tree). + +Live ground truth is a list of defect specs (`live.mustCatchSpecs`): path, +optional line window, mechanism regex alternates, and optional +`altLocations` for defects with more than one correct anchor site (a +migration missing an index is correctly flagged at the migration OR at the +hot query; a single-location spec turns anchor-site preference into fake +recall noise). Matching is deterministic first (location AND mechanism); +specs left unmatched go to a capped Haiku arbiter (`match-arbiter.ts`) +whose claims are recorded `via: "fallback"` for audit. + +Growing the corpus: target the 20-80% catch band, where discrimination +lives. Saturated cases (caught 100% on both arms) are tripwires; they add +safety but zero resolution. Mint new cases from production incidents and +from every confirmed found-but-dropped miss. After changing a case, replay +recorded artifacts through the matcher before trusting new rates (the +sql-missing-index case read 8/16 for a week because the spec, not the +reviewer, was wrong). + +## Reading a report + +- **Load-bearing:** must-catch recall against labeled specs, verdict + agreement, the per-case regression list, the drop buckets, and the + adversarial hard gate. Judge quality and noise jitter run-to-run and can + move OPPOSITE to review health (fewer, surer comments each read better). +- **Noise floor** (measured on 6 identical-arm samples, run 29069228968, + rendered in every report footer): recall 54-86%, verdict agreement + 75-100%, noise 50-60%, judge quality 0.82-0.86. A single-run delta whose + arms both sit inside a band is wobble. Detecting a 20-point recall change + needs ~60 spec-samples per arm; 10 points needs ~140 (two-proportion, 80% + power). Repeats are the cheap axis: no authoring, no review. +- **Miss classes:** a true miss is a recall problem; found-but-dropped + (provenance/scope/validation buckets) is an anchoring or gate-calibration + problem. They route to different fixes; never collapse them. +- **Gates:** single runs retry a flipped adversarial case best-of-three; + `--repeats` runs decide by strict majority across repeats instead. Only + confirmed failures exit non-zero. +- **Stacked PRs:** a per-PR report's baseline is the PR's base branch tip + (the parent PR in a stack), so it prices the marginal delta only. + Absolute columns do not compare across reports. +- **Ruler provenance:** every report stamps the matcher configuration and a + corpus content hash (`provenance` in the JSON, the "Ruler" line in the + markdown). Rates are only comparable when BOTH the review.md sha and the + ruler match; `aggregate.ts` warns loudly on mixed pools. Instrument + changes (arbiter on/off, corpus growth) move every rate without the + reviewer changing, and the stamps are what keep the drift series honest + across them. + +### Statistical honesty (limits to keep in mind) + +- **Pooled intervals are optimistic.** The Wilson intervals treat spec + catches as independent draws; specs within a case and cases within an + arm-run are correlated, so true pooled intervals are somewhat wider than + printed. Per-case rows are close to valid; treat the pooled CI as a lower + bound on uncertainty. +- **The v1 noise-floor bands carry case-mix variance.** The 2026-07-10 + measurement ran into budget skips (38/36 of 42 case-runs), so its bands + fold corpus-composition variance in on top of run-to-run wobble. The + aggregate now flags asymmetric samples and reports SD alongside min/max + (min/max only widen as samples accumulate; mean +/- sd is the band to + track). Weekly drift runs at the $85 default refresh the bands cleanly. +- **The repeats-mode gate is more permissive than single-run mode.** A + single run fail-biases (a flipped adversarial case must pass both + retries); `--repeats` confirms only on a strict majority, so a case + failing under half the time passes a powered run. Deliberate (the repeat + structure is the evidence), but it IS a relaxation of "handle every + adversarial case outright"; per-case fail counts print either way, read + them. +- **The arbiter's refuse bias is a prompt, not a calibration.** Its rescues + inflate recall, the load-bearing metric, and its false-positive rate has + not been measured against known non-matches. Audit `via: "fallback"` + matches when a recall claim is close, and prefer `--no-match-arbiter` + when reproducing pre-arbiter numbers. + +## Costs and models + +Measured ~$0.72-0.75 per case per arm. Smoke run ~$10/PR; full 14-case +corpus x3 repeats x both arms ~$60 (cap it at $85 to avoid budget skips). +The judge and the match arbiter are pinned to `claude-haiku-4-5-20251001`. +Every model-spending path degrades to a partial report rather than dying +at a cap, and judge/arbiter failures degrade to notes/non-matches. + +## Historical limits + +The live A/B executes agent prompts extracted from review.md and validates +schema-2 findings. That architecture starts 2026-07-08 (#202); the +`failure_scenario` requirement lands 2026-07-09 (#226). Snapshots at or +after #226 can be baselined directly via `base_ref`; anything older (the +pre-agent monolith, or the 6-agent legacy-contract era) cannot run under +this harness and needs a seeded-defect live trial (the `review-trial` +pattern) instead. diff --git a/workflows/review/eval/aggregate.test.ts b/workflows/review/eval/aggregate.test.ts new file mode 100644 index 00000000..3a6760e1 --- /dev/null +++ b/workflows/review/eval/aggregate.test.ts @@ -0,0 +1,572 @@ +import {describe, it, expect} from "vitest"; + +import { + aggregateSamples, + computeNoiseFloor, + extractSamples, + rateStat, + renderAggregateMarkdown, + wilsonInterval, + type ArmSample, + type ReportSample, + type SampleRun, +} from "./aggregate"; + +/* -------------------------------------------------------------------------- */ +/* Fixture builders: the report-JSON subset the extractor consumes */ +/* -------------------------------------------------------------------------- */ + +/** One raw `arms..runs[]` entry as live-ab.ts serializes it. */ +const rawRun = ( + caseId: string, + over: { + verdict?: string; + expected?: string; + caught?: string[]; + missedDetail?: {specKey: string; droppedBy?: string}[]; + unmatched?: string[]; + posted?: number; + } = {}, +) => ({ + corpusCase: { + id: caseId, + expected: {verdict: over.expected ?? "REQUEST_CHANGES"}, + }, + result: {verdict: {event: over.verdict ?? "REQUEST_CHANGES"}}, + match: { + caseId, + caught: (over.caught ?? []).map((specKey) => ({ + specKey, + findingId: `${caseId}:f`, + via: "deterministic", + })), + missed: (over.missedDetail ?? []).map((d) => d.specKey), + missedDetail: over.missedDetail ?? [], + falseFlags: [], + unmatchedFindingIds: over.unmatched ?? [], + postedCount: over.posted ?? (over.caught ?? []).length, + }, +}); + +const rawReport = (over: { + baselineRuns?: unknown[]; + candidateRuns?: unknown[]; + baselineSha?: string; + candidateSha?: string; + baselineJudge?: number; + candidateJudge?: number; +}) => ({ + baseRef: "origin/main", + reviewMdSha: { + baseline: over.baselineSha ?? "a".repeat(64), + candidate: over.candidateSha ?? "b".repeat(64), + }, + arms: { + baseline: { + arm: "baseline", + runs: over.baselineRuns ?? [], + usd: 1.5, + ...(over.baselineJudge !== undefined + ? {judge: {meanQuality: over.baselineJudge, verdictCounts: {}}} + : {}), + }, + candidate: { + arm: "candidate", + runs: over.candidateRuns ?? [], + usd: 2.5, + ...(over.candidateJudge !== undefined + ? {judge: {meanQuality: over.candidateJudge, verdictCounts: {}}} + : {}), + }, + }, + regressions: {lost: [], gained: []}, + adversarialFailures: [], + gateRetries: [], +}); + +describe("wilsonInterval", () => { + it("brackets the point estimate and stays inside [0,1]", () => { + const interval = wilsonInterval(5, 6); + expect(interval.lo).toBeGreaterThan(0.4); + expect(interval.lo).toBeLessThan(5 / 6); + expect(interval.hi).toBeGreaterThan(5 / 6); + expect(interval.hi).toBeLessThanOrEqual(1); + }); + + it("is exactly [0,1] at n=0 and never collapses at 0/n or n/n", () => { + expect(wilsonInterval(0, 0)).toEqual({lo: 0, hi: 1}); + const zero = wilsonInterval(0, 10); + expect(zero.lo).toBe(0); + expect(zero.hi).toBeGreaterThan(0.2); + const full = wilsonInterval(10, 10); + expect(full.hi).toBe(1); + expect(full.lo).toBeLessThan(1); + }); + + it("narrows as repeats accumulate (the whole point of pooling)", () => { + const single = wilsonInterval(6, 8); + const pooled = wilsonInterval(60, 80); + expect(pooled.hi - pooled.lo).toBeLessThan((single.hi - single.lo) / 2); + }); +}); + +describe("extractSamples", () => { + it("reduces a single-run report to one sample pair", () => { + const raw = rawReport({ + baselineRuns: [rawRun("case-1", {caught: ["spec-1"]})], + candidateRuns: [ + rawRun("case-1", { + missedDetail: [ + {specKey: "spec-1", droppedBy: "provenance"}, + ], + posted: 2, + unmatched: ["x"], + }), + ], + }); + const samples = extractSamples("r1", raw); + expect(samples.length).toBe(1); + const sample = samples[0]!; + expect(sample.baseline.runs[0]?.caughtSpecKeys).toEqual(["spec-1"]); + expect(sample.candidate.runs[0]?.missedSpecs).toEqual([ + {specKey: "spec-1", droppedBy: "provenance"}, + ]); + expect(sample.candidate.runs[0]?.unmatchedPosted).toBe(1); + expect(sample.candidate.runs[0]?.posted).toBe(2); + expect(sample.baseline.usd).toBe(1.5); + }); + + it("flattens a --repeats artifact into one sample per repeat", () => { + const single = rawReport({ + baselineRuns: [rawRun("case-1", {caught: ["spec-1"]})], + candidateRuns: [rawRun("case-1", {caught: ["spec-1"]})], + }); + const samples = extractSamples("r", {repeats: [single, single]}); + expect(samples.length).toBe(2); + expect(samples.map((s) => s.source)).toEqual(["r#1", "r#2"]); + }); + + it("pools the completed repeats from a mid-run checkpoint artifact", () => { + const single = rawReport({ + baselineRuns: [rawRun("case-1", {caught: ["spec-1"]})], + candidateRuns: [rawRun("case-1", {caught: ["spec-1"]})], + }); + // The shape live-ab.ts writes after each repeat, before the final + // report replaces it: the run died on repeat 3 of 3. + const checkpoint = { + repeatCount: 3, + completedRepeats: 2, + repeats: [single, single], + }; + const samples = extractSamples("r", checkpoint); + expect(samples.length).toBe(2); + expect(samples.map((s) => s.source)).toEqual(["r#1", "r#2"]); + }); + + it("carries the ruler stamp through, and tolerates its absence", () => { + const stamped = { + ...rawReport({ + baselineRuns: [rawRun("case-1")], + candidateRuns: [rawRun("case-1")], + }), + provenance: { + matcher: "deterministic+arbiter", + corpusSha: "c".repeat(64), + caseCount: 14, + }, + }; + const sample = extractSamples("r1", stamped)[0]!; + expect(sample.matcher).toBe("deterministic+arbiter"); + expect(sample.corpusSha).toBe("c".repeat(64)); + // Pre-stamp artifacts still parse; the stamp fields stay absent. + const legacy = extractSamples( + "r0", + rawReport({ + baselineRuns: [rawRun("case-1")], + candidateRuns: [rawRun("case-1")], + }), + )[0]!; + expect(legacy.matcher).toBeUndefined(); + expect(legacy.corpusSha).toBeUndefined(); + }); + + it("counts budget skips into the arm sample", () => { + const raw = rawReport({ + baselineRuns: [rawRun("case-1")], + candidateRuns: [rawRun("case-1")], + }); + (raw.arms.baseline as Record)["skippedCases"] = [ + "case-2", + "case-3", + ]; + const sample = extractSamples("r1", raw)[0]!; + expect(sample.baseline.skippedCount).toBe(2); + expect(sample.candidate.skippedCount).toBe(0); + }); + + it("contributes nothing for a no-reviewable-delta report", () => { + expect( + extractSamples("r", {noReviewableDelta: true, baseRef: "x"}), + ).toEqual([]); + }); + + it("throws a shape error a caller can record and skip", () => { + expect(() => extractSamples("r", {arms: {}})).toThrow(/baseline\.runs/); + }); + + it("falls back to `missed` keys when a report predates missedDetail", () => { + const raw = rawReport({ + baselineRuns: [rawRun("case-1")], + candidateRuns: [rawRun("case-1")], + }); + const run = (raw.arms.baseline.runs as Record[])[0]!; + (run["match"] as Record)["missed"] = ["spec-old"]; + (run["match"] as Record)["missedDetail"] = undefined; + const samples = extractSamples("r1", raw); + expect(samples[0]?.baseline.runs[0]?.missedSpecs).toEqual([ + {specKey: "spec-old"}, + ]); + }); +}); + +describe("aggregateSamples", () => { + const run = (over: Parameters[1] = {}): SampleRun => + sampleRun("case-1", over); + const sampleRun = ( + caseId: string, + over: Partial = {}, + ): SampleRun => ({ + caseId, + expectedVerdict: "REQUEST_CHANGES", + verdict: "REQUEST_CHANGES", + caughtSpecKeys: [], + missedSpecs: [], + unmatchedPosted: 0, + posted: 0, + ...over, + }); + const arm = ( + armId: "baseline" | "candidate", + runs: SampleRun[], + over: Partial = {}, + ): ArmSample => ({ + arm: armId, + reviewMdSha: armId === "baseline" ? "a".repeat(64) : "b".repeat(64), + runs, + skippedCount: 0, + usd: 1, + ...over, + }); + const sample = ( + baselineRuns: SampleRun[], + candidateRuns: SampleRun[], + source = "r", + ): ReportSample => ({ + source, + baseRef: "origin/main", + baseline: arm("baseline", baselineRuns), + candidate: arm("candidate", candidateRuns), + }); + + it("pools per-case catch and verdict rates across repeats", () => { + // Three repeats: baseline catches 2/3, flips the verdict once. + const samples = [ + sample( + [run({caughtSpecKeys: ["spec-1"]})], + [run({caughtSpecKeys: ["spec-1"]})], + "r1", + ), + sample( + [ + run({ + missedSpecs: [{specKey: "spec-1"}], + verdict: "APPROVE", + }), + ], + [run({caughtSpecKeys: ["spec-1"]})], + "r2", + ), + sample( + [run({caughtSpecKeys: ["spec-1"]})], + [ + run({ + missedSpecs: [ + {specKey: "spec-1", droppedBy: "provenance"}, + ], + }), + ], + "r3", + ), + ]; + const report = aggregateSamples(samples); + expect(report.samples).toBe(3); + + const base = report.arms.baseline.cases[0]!; + expect(base.runs).toBe(3); + expect(base.specs[0]?.caught.numerator).toBe(2); + expect(base.specs[0]?.caught.denominator).toBe(3); + expect(base.specs[0]?.trueMisses).toBe(1); + expect(base.specs[0]?.droppedBy).toEqual({}); + expect(base.verdictOk.numerator).toBe(2); + + const cand = report.arms.candidate.cases[0]!; + expect(cand.specs[0]?.caught.numerator).toBe(2); + expect(cand.specs[0]?.trueMisses).toBe(0); + expect(cand.specs[0]?.droppedBy).toEqual({provenance: 1}); + expect(cand.verdictOk.numerator).toBe(3); + + // Pooled rows: recall over specs, agreement over case-runs. + expect(report.arms.baseline.pooled.recall.numerator).toBe(2); + expect(report.arms.baseline.pooled.recall.denominator).toBe(3); + expect(report.arms.candidate.pooled.trueMisses).toBe(0); + expect(report.arms.candidate.pooled.foundButDropped).toEqual({ + provenance: 1, + }); + expect(report.arms.baseline.pooled.usd).toBe(3); + }); + + it("keeps cases the pools do not share and counts their runs honestly", () => { + // r2 ran a second case (e.g. full corpus vs smoke): its case shows + // one run, not three, and the shared case still shows two. + const samples = [ + sample([sampleRun("case-1")], [sampleRun("case-1")], "r1"), + sample( + [sampleRun("case-1"), sampleRun("case-2")], + [sampleRun("case-1"), sampleRun("case-2")], + "r2", + ), + ]; + const report = aggregateSamples(samples); + const byId = Object.fromEntries( + report.arms.baseline.cases.map((c) => [c.caseId, c.runs]), + ); + expect(byId).toEqual({"case-1": 2, "case-2": 1}); + }); + + it("flags multi-sha pools and carries skipped sources", () => { + const mixed = [ + sample([run()], [run()], "r1"), + { + ...sample([run()], [run()], "r2"), + candidate: arm("candidate", [run()], { + reviewMdSha: "c".repeat(64), + }), + }, + ]; + const report = aggregateSamples(mixed, [ + {source: "r3", reason: "no reviewable delta"}, + ]); + expect(report.arms.candidate.reviewMdShas.length).toBe(2); + const markdown = renderAggregateMarkdown(report); + expect(markdown).toContain("WARNING: pooled runs carry more than one"); + expect(markdown).toContain("r3 (no reviewable delta)"); + }); + + it("emits the noise floor iff every sample ran identical arms", () => { + const identical: ReportSample = { + source: "r1", + baseRef: "origin/main", + baseline: arm("baseline", [run({caughtSpecKeys: ["spec-1"]})], { + reviewMdSha: "a".repeat(64), + }), + candidate: arm( + "candidate", + [run({missedSpecs: [{specKey: "spec-1"}]})], + {reviewMdSha: "a".repeat(64)}, + ), + }; + const report = aggregateSamples([identical]); + expect(report.noiseFloor).toBeDefined(); + expect(report.noiseFloor?.armSamples).toBe(2); + // Recall wobbled 0% to 100% across the two arm-samples. + expect(report.noiseFloor?.bands["must-catch recall"]).toEqual({ + min: 0, + max: 1, + mean: 0.5, + sd: 0.5, + }); + // Same corpus in every sample, no skips: bands are clean. + expect(report.noiseFloor?.caseAsymmetry).toBe(false); + + const differing = sample([run()], [run()]); + expect(aggregateSamples([differing]).noiseFloor).toBeUndefined(); + expect(aggregateSamples([]).noiseFloor).toBeUndefined(); + }); +}); + +describe("computeNoiseFloor", () => { + it("bands each metric across arm-samples, judge included when present", () => { + const mk = (caught: boolean, judge: number | undefined): ArmSample => ({ + arm: "baseline", + reviewMdSha: "a".repeat(64), + skippedCount: 0, + runs: [ + { + caseId: "case-1", + expectedVerdict: "REQUEST_CHANGES", + verdict: "REQUEST_CHANGES", + caughtSpecKeys: caught ? ["s"] : [], + missedSpecs: caught ? [] : [{specKey: "s"}], + unmatchedPosted: 1, + posted: 2, + }, + ], + usd: 1, + ...(judge !== undefined ? {judgeMeanQuality: judge} : {}), + }); + const floor = computeNoiseFloor([mk(true, 0.8), mk(false, 0.9)]); + expect(floor.bands["must-catch recall"]).toEqual({ + min: 0, + max: 1, + mean: 0.5, + sd: 0.5, + }); + expect(floor.caseAsymmetry).toBe(false); + expect(floor.bands["noise (unmatched posted)"]?.mean).toBe(0.5); + expect(floor.bands["judge mean quality"]?.min).toBe(0.8); + expect(floor.bands["judge mean quality"]?.max).toBe(0.9); + expect(floor.bands["judge mean quality"]?.mean).toBeCloseTo(0.85); + expect(floor.bands["judge mean quality"]?.sd).toBeCloseTo(0.05); + }); + + it("flags case asymmetry on budget skips or mismatched case sets", () => { + const mk = (over: Partial): ArmSample => ({ + arm: "baseline", + reviewMdSha: "a".repeat(64), + skippedCount: 0, + runs: [ + { + caseId: "case-1", + expectedVerdict: "REQUEST_CHANGES", + verdict: "REQUEST_CHANGES", + caughtSpecKeys: ["s"], + missedSpecs: [], + unmatchedPosted: 0, + posted: 1, + }, + ], + usd: 1, + ...over, + }); + // A budget skip means the sample scored a partial corpus: the bands + // fold case-mix variance in, and the renderer must say so loudly. + expect( + computeNoiseFloor([mk({}), mk({skippedCount: 2})]).caseAsymmetry, + ).toBe(true); + // Same skip-free samples but scoring different case sets: also + // asymmetric (e.g. a pool mixing corpus versions). + const otherCase = mk({}); + otherCase.runs = [{...otherCase.runs[0]!, caseId: "case-2"}]; + expect(computeNoiseFloor([mk({}), otherCase]).caseAsymmetry).toBe(true); + expect(computeNoiseFloor([mk({}), mk({})]).caseAsymmetry).toBe(false); + }); +}); + +describe("renderAggregateMarkdown", () => { + it("renders per-spec rates with intervals, pooled rows, and miss classes", () => { + const raw = rawReport({ + baselineRuns: [rawRun("case-1", {caught: ["spec-1"]})], + candidateRuns: [ + rawRun("case-1", { + missedDetail: [ + {specKey: "spec-1", droppedBy: "provenance"}, + ], + }), + ], + baselineJudge: 0.9, + candidateJudge: 0.8, + }); + const report = aggregateSamples([ + ...extractSamples("r1", raw), + ...extractSamples("r2", raw), + ]); + const markdown = renderAggregateMarkdown(report); + expect(markdown).toContain("Pooled 2 run(s) per arm"); + expect(markdown).toContain("| case-1:spec-1 | 2/2 (100%)"); + expect(markdown).toContain("cand: 2 dropped at provenance"); + expect(markdown).toContain("| case-1 (verdict) | 2/2 (100%)"); + expect(markdown).toContain("| Must-catch recall | 2/2 (100%)"); + expect(markdown).toContain("| 0/2 (0%)"); + expect(markdown).toContain( + "| Misses (true / dropped) | 0 true / 0 dropped | | 0 true / 2 provenance | |", + ); + expect(markdown).toContain("| Judge mean quality | 0.90 | | 0.80 |"); + // No identical arms, so no noise-floor section. + expect(markdown).not.toContain("Noise floor"); + }); + + it("prints the ruler line and warns when a pool mixes rulers", () => { + const withRuler = (matcher: string, corpusSha: string) => ({ + ...rawReport({ + baselineRuns: [rawRun("case-1", {caught: ["spec-1"]})], + candidateRuns: [rawRun("case-1", {caught: ["spec-1"]})], + }), + provenance: {matcher, corpusSha, caseCount: 1}, + }); + const uniform = aggregateSamples([ + ...extractSamples("r1", withRuler("deterministic", "c".repeat(64))), + ...extractSamples("r2", withRuler("deterministic", "c".repeat(64))), + ]); + const uniformMd = renderAggregateMarkdown(uniform); + expect(uniformMd).toContain( + "Ruler: matcher deterministic; corpus cccccccccccc.", + ); + expect(uniformMd).not.toContain("mix rulers"); + + const mixed = aggregateSamples([ + ...extractSamples("r1", withRuler("deterministic", "c".repeat(64))), + ...extractSamples( + "r2", + withRuler("deterministic+arbiter", "d".repeat(64)), + ), + ]); + expect(renderAggregateMarkdown(mixed)).toContain( + "WARNING: pooled runs mix rulers", + ); + }); + + it("warns on asymmetric samples under the noise-floor bands", () => { + const raw = rawReport({ + baselineRuns: [rawRun("case-1", {caught: ["spec-1"]})], + candidateRuns: [rawRun("case-1", {caught: ["spec-1"]})], + baselineSha: "a".repeat(64), + candidateSha: "a".repeat(64), + }); + (raw.arms.candidate as Record)["skippedCases"] = [ + "case-2", + ]; + const markdown = renderAggregateMarkdown( + aggregateSamples(extractSamples("r1", raw)), + ); + expect(markdown).toContain("### Noise floor"); + expect(markdown).toContain( + "WARNING: the samples did not all score the same case set", + ); + }); + + it("renders the noise-floor bands for an identical-arm pool", () => { + const raw = rawReport({ + baselineRuns: [rawRun("case-1", {caught: ["spec-1"]})], + candidateRuns: [rawRun("case-1", {caught: ["spec-1"]})], + baselineSha: "a".repeat(64), + candidateSha: "a".repeat(64), + }); + const markdown = renderAggregateMarkdown( + aggregateSamples(extractSamples("r1", raw)), + ); + expect(markdown).toContain("### Noise floor"); + expect(markdown).toContain( + "| must-catch recall | 100% | 100% | 100% | 0% | 0% |", + ); + }); +}); + +describe("rateStat", () => { + it("carries numerator, denominator, rate, and interval together", () => { + const stat = rateStat(3, 4); + expect(stat.rate).toBe(0.75); + expect(stat.interval.lo).toBeGreaterThan(0); + expect(stat.interval.hi).toBeLessThanOrEqual(1); + expect(rateStat(0, 0).rate).toBe(0); + }); +}); diff --git a/workflows/review/eval/aggregate.ts b/workflows/review/eval/aggregate.ts new file mode 100644 index 00000000..d94f5092 --- /dev/null +++ b/workflows/review/eval/aggregate.ts @@ -0,0 +1,859 @@ +/** + * Repeat aggregation over live A/B report artifacts (the tuning memo's + * "repeat-aggregation report", rev-2 item 3). One live run resolves nothing + * smaller than ~40 recall points (7-9 specs per arm), so any real measurement + * pools repeats: this module reads N `live-ab-report.json` payloads (from N + * dispatches of the same arm pair, or one `--repeats n` dispatch) and emits + * per-case pass rates per arm with binomial (Wilson) intervals, plus the + * pooled recall/verdict/noise rows. Every number in the memo's rev-2 + * cumulative section was computed by hand from the report JSONs; this makes + * it one command. + * + * The core is deterministic (parsed JSON in, aggregate out) and unit-tested; + * the CLI at the bottom is a thin shell that also accepts GitHub Actions run + * ids (downloaded via `gh run download`). + * + * When every pooled report ran IDENTICAL arms (`--force-arms` wobble + * controls, or the scheduled drift run on main), the two arms are 2N samples + * of the same prompt, and the aggregate additionally reports per-metric + * noise-floor bands (min/max/mean across arm-samples); the memo's "buy the + * noise floor" item, rendered as data instead of prose. + * + * CLI: + * + * pnpm dlx tsx workflows/review/eval/aggregate.ts ... + * [--out ] JSON aggregate path (default out/live-ab-aggregate.json) + * + * A run id (all digits) is fetched with `gh run download -n + * live-ab-report`; local paths are read as-is. A source that cannot be + * parsed is reported and skipped, never fatal: partial aggregation beats no + * report (the plan's standing degrade rule). + */ + +/* eslint-disable no-console -- CLI entry point; console IS the interface. */ + +import {execFileSync} from "node:child_process"; +import {mkdirSync, mkdtempSync, readFileSync, writeFileSync} from "node:fs"; +import {tmpdir} from "node:os"; +import {dirname} from "node:path"; + +/* -------------------------------------------------------------------------- */ +/* The report subset this module consumes (structural, version-tolerant) */ +/* -------------------------------------------------------------------------- */ + +/** One case-run as it appears in a report's `arms..runs[]`. */ +export type SampleRun = { + caseId: string; + expectedVerdict: string; + verdict: string; + caughtSpecKeys: string[]; + /** Missed spec key -> drop bucket ("" for a true miss). */ + missedSpecs: {specKey: string; droppedBy?: string}[]; + unmatchedPosted: number; + posted: number; +}; + +/** One arm-run: a single pass of one arm over its cases. */ +export type ArmSample = { + arm: "baseline" | "candidate"; + reviewMdSha: string; + runs: SampleRun[]; + /** Cases never dispatched (budget skips); asymmetric samples bias bands. */ + skippedCount: number; + usd: number; + judgeMeanQuality?: number; +}; + +/** One report artifact, reduced to what aggregation needs. */ +export type ReportSample = { + source: string; + baseRef: string; + /** + * Ruler provenance, when the report carries it (reports predating the + * stamps parse with both undefined): the matcher configuration and a + * content hash of the loaded corpus. Rates are only comparable across + * runs whose ruler matches; the aggregate warns on a mixed pool. + */ + matcher?: string; + corpusSha?: string; + baseline: ArmSample; + candidate: ArmSample; +}; + +const isRecord = (value: unknown): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + +const asString = (value: unknown): string => + typeof value === "string" ? value : ""; + +const asNumber = (value: unknown): number => + typeof value === "number" && Number.isFinite(value) ? value : 0; + +/** Parse one arm out of a raw report; throws a descriptive error on shape. */ +const parseArm = ( + raw: unknown, + arm: "baseline" | "candidate", + reviewMdSha: string, +): ArmSample => { + if (!isRecord(raw) || !Array.isArray(raw["runs"])) { + throw new Error(`arms.${arm}.runs: missing or not an array`); + } + const runs = raw["runs"].map((run, i): SampleRun => { + if (!isRecord(run)) { + throw new Error(`arms.${arm}.runs[${i}]: not an object`); + } + const corpusCase = run["corpusCase"]; + const result = run["result"]; + const match = run["match"]; + if (!isRecord(corpusCase) || !isRecord(result) || !isRecord(match)) { + throw new Error( + `arms.${arm}.runs[${i}]: missing corpusCase/result/match`, + ); + } + const expected = isRecord(corpusCase["expected"]) + ? corpusCase["expected"] + : {}; + const verdict = isRecord(result["verdict"]) ? result["verdict"] : {}; + const caught = Array.isArray(match["caught"]) ? match["caught"] : []; + const missedDetail = Array.isArray(match["missedDetail"]) + ? match["missedDetail"] + : []; + // Older reports carry `missed` only; missedDetail supersedes it. + const missed = Array.isArray(match["missed"]) ? match["missed"] : []; + const detailKeys = new Set( + missedDetail + .filter(isRecord) + .map((d) => asString(d["specKey"])) + .filter((k) => k !== ""), + ); + const missedSpecs = [ + ...missedDetail.filter(isRecord).map((d) => { + const droppedBy = asString(d["droppedBy"]); + return { + specKey: asString(d["specKey"]), + ...(droppedBy !== "" ? {droppedBy} : {}), + }; + }), + ...missed + .filter( + (k): k is string => + typeof k === "string" && !detailKeys.has(k), + ) + .map((specKey) => ({specKey})), + ]; + const unmatched = Array.isArray(match["unmatchedFindingIds"]) + ? match["unmatchedFindingIds"].length + : 0; + return { + caseId: asString(corpusCase["id"]), + expectedVerdict: asString(expected["verdict"]), + verdict: asString(verdict["event"]), + caughtSpecKeys: caught + .filter(isRecord) + .map((c) => asString(c["specKey"])) + .filter((k) => k !== ""), + missedSpecs, + unmatchedPosted: unmatched, + posted: asNumber(match["postedCount"]), + }; + }); + const judge = raw["judge"]; + return { + arm, + reviewMdSha, + runs, + skippedCount: Array.isArray(raw["skippedCases"]) + ? raw["skippedCases"].length + : 0, + usd: asNumber(raw["usd"]), + ...(isRecord(judge) && typeof judge["meanQuality"] === "number" + ? {judgeMeanQuality: judge["meanQuality"]} + : {}), + }; +}; + +/** + * Extract the arm samples one report artifact contributes. A single-run + * report contributes one sample pair; a `--repeats n` report contributes n; a + * no-reviewable-delta report contributes none (recorded as skipped upstream). + */ +export const extractSamples = ( + source: string, + raw: unknown, +): ReportSample[] => { + if (!isRecord(raw)) { + throw new Error("report: not a JSON object"); + } + if (raw["noReviewableDelta"] === true) { + return []; + } + // A --repeats artifact nests single-run reports under `repeats`. + if (Array.isArray(raw["repeats"])) { + return raw["repeats"].flatMap((repeat, i) => + extractSamples(`${source}#${i + 1}`, repeat), + ); + } + const arms = raw["arms"]; + const shas = raw["reviewMdSha"]; + if (!isRecord(arms)) { + throw new Error("report: missing arms"); + } + const sha = (key: string): string => + isRecord(shas) ? asString(shas[key]) : ""; + const provenance = isRecord(raw["provenance"]) ? raw["provenance"] : {}; + const matcher = asString(provenance["matcher"]); + const corpusSha = asString(provenance["corpusSha"]); + return [ + { + source, + baseRef: asString(raw["baseRef"]), + ...(matcher !== "" ? {matcher} : {}), + ...(corpusSha !== "" ? {corpusSha} : {}), + baseline: parseArm(arms["baseline"], "baseline", sha("baseline")), + candidate: parseArm( + arms["candidate"], + "candidate", + sha("candidate"), + ), + }, + ]; +}; + +/* -------------------------------------------------------------------------- */ +/* Binomial interval */ +/* -------------------------------------------------------------------------- */ + +export type RateStat = { + numerator: number; + denominator: number; + rate: number; + /** 95% Wilson score interval; [0,1] when the denominator is 0. */ + interval: {lo: number; hi: number}; +}; + +/** + * The Wilson score interval (95%, z=1.96): the standard binomial interval + * that stays sane at the small n these runs live at (a 5/6 pass rate reads + * 44-97%, not the Wald interval's overconfident nonsense). + */ +export const wilsonInterval = ( + successes: number, + n: number, +): {lo: number; hi: number} => { + if (n === 0) { + return {lo: 0, hi: 1}; + } + const z = 1.96; + const p = successes / n; + const z2 = z * z; + const denom = 1 + z2 / n; + const center = (p + z2 / (2 * n)) / denom; + const half = (z * Math.sqrt((p * (1 - p)) / n + z2 / (4 * n * n))) / denom; + return {lo: Math.max(0, center - half), hi: Math.min(1, center + half)}; +}; + +export const rateStat = (numerator: number, denominator: number): RateStat => ({ + numerator, + denominator, + rate: denominator === 0 ? 0 : numerator / denominator, + interval: wilsonInterval(numerator, denominator), +}); + +/* -------------------------------------------------------------------------- */ +/* Aggregation */ +/* -------------------------------------------------------------------------- */ + +export type SpecAggregate = { + specKey: string; + caught: RateStat; + trueMisses: number; + /** Drop bucket -> count, for found-but-dropped misses. */ + droppedBy: Record; +}; + +export type CaseAggregate = { + caseId: string; + /** Arm-runs that scored this case (repeats the case appeared in). */ + runs: number; + specs: SpecAggregate[]; + verdictOk: RateStat; +}; + +export type ArmAggregate = { + arm: "baseline" | "candidate"; + /** Distinct review.md shas pooled (more than one is a pooling warning). */ + reviewMdShas: string[]; + samples: number; + cases: CaseAggregate[]; + pooled: { + recall: RateStat; + verdictAgreement: RateStat; + noise: RateStat; + trueMisses: number; + foundButDropped: Record; + usd: number; + }; + /** Mean of per-sample judge means, when any sample carried one. */ + judgeMeanQuality?: number; +}; + +/** + * Per-metric wobble bands over identical-arm samples (the noise floor). + * Each band is computed across the 2N arm-samples of the same prompt. Min + * and max are extreme-value statistics (they only widen as samples + * accumulate); mean plus/minus sd is the stable band to track week to week. + */ +export type NoiseFloor = { + armSamples: number; + /** metric -> spread of the per-arm-sample rate. */ + bands: Record; + /** + * True when the samples did not all score the same case set (budget + * skips, or pools mixing corpora). Asymmetric samples fold case-mix + * variance into the bands, inflating them beyond pure run-to-run + * wobble; the renderer surfaces this loudly because clean samples are + * the entire point of an identical-arms run. + */ + caseAsymmetry: boolean; +}; + +export type AggregateReport = { + sources: string[]; + /** Sources that contributed nothing, with the reason. */ + skippedSources: {source: string; reason: string}[]; + samples: number; + baseRefs: string[]; + /** Distinct ruler stamps across the pool (empty for legacy reports). */ + matchers: string[]; + corpusShas: string[]; + arms: {baseline: ArmAggregate; candidate: ArmAggregate}; + /** Set iff every sample ran byte-identical arms. */ + noiseFloor?: NoiseFloor; +}; + +const aggregateArm = ( + arm: "baseline" | "candidate", + samples: ArmSample[], +): ArmAggregate => { + const byCase = new Map< + string, + { + runs: number; + verdictOk: number; + specs: Map< + string, + {caught: number; seen: number; dropped: Map} + >; + } + >(); + let specCaught = 0; + let specTotal = 0; + let verdictOk = 0; + let caseRuns = 0; + let unmatched = 0; + let posted = 0; + let usd = 0; + const judgeMeans: number[] = []; + + for (const sample of samples) { + usd += sample.usd; + if (sample.judgeMeanQuality !== undefined) { + judgeMeans.push(sample.judgeMeanQuality); + } + for (const run of sample.runs) { + const entry = byCase.get(run.caseId) ?? { + runs: 0, + verdictOk: 0, + specs: new Map(), + }; + entry.runs += 1; + caseRuns += 1; + if (run.verdict === run.expectedVerdict) { + entry.verdictOk += 1; + verdictOk += 1; + } + unmatched += run.unmatchedPosted; + posted += run.posted; + const spec = (key: string) => { + const s = entry.specs.get(key) ?? { + caught: 0, + seen: 0, + dropped: new Map(), + }; + entry.specs.set(key, s); + return s; + }; + for (const key of run.caughtSpecKeys) { + const s = spec(key); + s.caught += 1; + s.seen += 1; + specCaught += 1; + specTotal += 1; + } + for (const miss of run.missedSpecs) { + const s = spec(miss.specKey); + s.seen += 1; + specTotal += 1; + if (miss.droppedBy !== undefined) { + s.dropped.set( + miss.droppedBy, + (s.dropped.get(miss.droppedBy) ?? 0) + 1, + ); + } + } + byCase.set(run.caseId, entry); + } + } + + const cases: CaseAggregate[] = [...byCase.entries()] + .sort(([a], [b]) => a.localeCompare(b)) + .map(([caseId, entry]) => ({ + caseId, + runs: entry.runs, + specs: [...entry.specs.entries()] + .sort(([a], [b]) => a.localeCompare(b)) + .map(([specKey, s]) => { + const droppedBy = Object.fromEntries( + [...s.dropped.entries()].sort(([a], [b]) => + a.localeCompare(b), + ), + ); + const droppedCount = [...s.dropped.values()].reduce( + (sum, n) => sum + n, + 0, + ); + return { + specKey, + caught: rateStat(s.caught, s.seen), + trueMisses: s.seen - s.caught - droppedCount, + droppedBy, + }; + }), + verdictOk: rateStat(entry.verdictOk, entry.runs), + })); + + const foundButDropped: Record = {}; + let trueMisses = 0; + for (const c of cases) { + for (const s of c.specs) { + trueMisses += s.trueMisses; + for (const [bucket, count] of Object.entries(s.droppedBy)) { + foundButDropped[bucket] = + (foundButDropped[bucket] ?? 0) + count; + } + } + } + + return { + arm, + reviewMdShas: [...new Set(samples.map((s) => s.reviewMdSha))].sort(), + samples: samples.length, + cases, + pooled: { + recall: rateStat(specCaught, specTotal), + verdictAgreement: rateStat(verdictOk, caseRuns), + noise: rateStat(unmatched, posted), + trueMisses, + foundButDropped, + usd, + }, + ...(judgeMeans.length > 0 + ? { + judgeMeanQuality: + judgeMeans.reduce((sum, m) => sum + m, 0) / + judgeMeans.length, + } + : {}), + }; +}; + +/** Per-arm-sample metric rates, for the noise-floor bands. */ +const sampleRates = (sample: ArmSample): Record => { + let caught = 0; + let specs = 0; + let verdictOk = 0; + let unmatched = 0; + let posted = 0; + for (const run of sample.runs) { + caught += run.caughtSpecKeys.length; + specs += run.caughtSpecKeys.length + run.missedSpecs.length; + if (run.verdict === run.expectedVerdict) { + verdictOk += 1; + } + unmatched += run.unmatchedPosted; + posted += run.posted; + } + return { + "must-catch recall": specs === 0 ? 0 : caught / specs, + "verdict agreement": + sample.runs.length === 0 ? 0 : verdictOk / sample.runs.length, + "noise (unmatched posted)": posted === 0 ? 0 : unmatched / posted, + ...(sample.judgeMeanQuality !== undefined + ? {"judge mean quality": sample.judgeMeanQuality} + : {}), + }; +}; + +/** Noise-floor bands across arm-samples (call only on identical-arm pools). */ +export const computeNoiseFloor = (armSamples: ArmSample[]): NoiseFloor => { + const bands: NoiseFloor["bands"] = {}; + const values = new Map(); + for (const sample of armSamples) { + for (const [metric, value] of Object.entries(sampleRates(sample))) { + values.set(metric, [...(values.get(metric) ?? []), value]); + } + } + for (const [metric, list] of values) { + const mean = list.reduce((sum, v) => sum + v, 0) / list.length; + bands[metric] = { + min: Math.min(...list), + max: Math.max(...list), + mean, + // Population sd: the samples ARE the population being described, + // and n is small enough that the n-1 debate is noise about noise. + sd: Math.sqrt( + list.reduce((sum, v) => sum + (v - mean) ** 2, 0) / list.length, + ), + }; + } + // Case asymmetry: any budget skip, or samples scoring different case + // sets (e.g. a pool mixing corpus versions), folds case-mix variance + // into the bands. + const caseIdSets = armSamples.map((sample) => + sample.runs + .map((run) => run.caseId) + .sort() + .join(","), + ); + const caseAsymmetry = + armSamples.some((sample) => sample.skippedCount > 0) || + new Set(caseIdSets).size > 1; + return {armSamples: armSamples.length, bands, caseAsymmetry}; +}; + +/** + * Pool report samples into the aggregate. Sources that failed to parse are + * carried in `skippedSources`; identical-arm pools additionally get the + * noise-floor bands. + */ +export const aggregateSamples = ( + samples: ReportSample[], + skippedSources: {source: string; reason: string}[] = [], +): AggregateReport => { + const identicalArms = + samples.length > 0 && + samples.every( + (s) => + s.baseline.reviewMdSha !== "" && + s.baseline.reviewMdSha === s.candidate.reviewMdSha, + ); + return { + sources: [...new Set(samples.map((s) => s.source))], + skippedSources, + samples: samples.length, + baseRefs: [...new Set(samples.map((s) => s.baseRef))].sort(), + matchers: [ + ...new Set( + samples + .map((s) => s.matcher) + .filter((m): m is string => m !== undefined), + ), + ].sort(), + corpusShas: [ + ...new Set( + samples + .map((s) => s.corpusSha) + .filter((c): c is string => c !== undefined), + ), + ].sort(), + arms: { + baseline: aggregateArm( + "baseline", + samples.map((s) => s.baseline), + ), + candidate: aggregateArm( + "candidate", + samples.map((s) => s.candidate), + ), + }, + ...(identicalArms + ? { + noiseFloor: computeNoiseFloor( + samples.flatMap((s) => [s.baseline, s.candidate]), + ), + } + : {}), + }; +}; + +/* -------------------------------------------------------------------------- */ +/* Rendering */ +/* -------------------------------------------------------------------------- */ + +const pct = (value: number): string => `${(value * 100).toFixed(0)}%`; + +const statCell = (stat: RateStat): string => + `${stat.numerator}/${stat.denominator} (${pct(stat.rate)})`; + +const intervalCell = (stat: RateStat): string => + `${pct(stat.interval.lo)}-${pct(stat.interval.hi)}`; + +const dropNote = (spec: SpecAggregate): string => { + const parts: string[] = []; + if (spec.trueMisses > 0) { + parts.push(`${spec.trueMisses} true miss`); + } + for (const [bucket, count] of Object.entries(spec.droppedBy)) { + parts.push(`${count} dropped at ${bucket}`); + } + return parts.join(", "); +}; + +/** + * The aggregate as a markdown report: a per-case table (spec catch rates and + * verdict agreement, both arms, Wilson intervals), the pooled rows, and the + * noise-floor bands when the pool was an identical-arm control. + */ +export const renderAggregateMarkdown = (report: AggregateReport): string => { + const {baseline, candidate} = report.arms; + const lines = [ + "## Review live A/B: repeat aggregation", + "", + `Pooled ${report.samples} run(s) per arm from: ${report.sources.join( + ", ", + )}.`, + `Baseline review.md ${baseline.reviewMdShas + .map((sha) => sha.slice(0, 12)) + .join(", ")}; candidate ${candidate.reviewMdShas + .map((sha) => sha.slice(0, 12)) + .join(", ")}.`, + "", + ]; + if (baseline.reviewMdShas.length > 1 || candidate.reviewMdShas.length > 1) { + lines.push( + "**WARNING: pooled runs carry more than one review.md sha per " + + "arm; these rates mix different prompts.**", + "", + ); + } + if (report.matchers.length > 0 || report.corpusShas.length > 0) { + lines.push( + `Ruler: matcher ${report.matchers.join(", ") || "unstamped"}; ` + + `corpus ${ + report.corpusShas + .map((sha) => sha.slice(0, 12)) + .join(", ") || "unstamped" + }.`, + "", + ); + } + if (report.matchers.length > 1 || report.corpusShas.length > 1) { + lines.push( + "**WARNING: pooled runs mix rulers (matcher config or corpus " + + "content differ); rates are not comparable across them.**", + "", + ); + } + if (report.skippedSources.length > 0) { + lines.push( + "Skipped sources (not pooled): " + + report.skippedSources + .map((s) => `${s.source} (${s.reason})`) + .join("; "), + "", + ); + } + + lines.push( + "| Case / spec | Baseline | 95% CI | Candidate | 95% CI | Miss classes |", + "| --- | --- | --- | --- | --- | --- |", + ); + const caseIds = [ + ...new Set([ + ...baseline.cases.map((c) => c.caseId), + ...candidate.cases.map((c) => c.caseId), + ]), + ].sort(); + for (const caseId of caseIds) { + const base = baseline.cases.find((c) => c.caseId === caseId); + const cand = candidate.cases.find((c) => c.caseId === caseId); + const specKeys = [ + ...new Set([ + ...(base?.specs.map((s) => s.specKey) ?? []), + ...(cand?.specs.map((s) => s.specKey) ?? []), + ]), + ].sort(); + for (const specKey of specKeys) { + const b = base?.specs.find((s) => s.specKey === specKey); + const c = cand?.specs.find((s) => s.specKey === specKey); + const notes = [ + ...(b && dropNote(b) !== "" ? [`base: ${dropNote(b)}`] : []), + ...(c && dropNote(c) !== "" ? [`cand: ${dropNote(c)}`] : []), + ]; + lines.push( + `| ${caseId}:${specKey} | ${b ? statCell(b.caught) : "n/a"} | ${ + b ? intervalCell(b.caught) : "" + } | ${c ? statCell(c.caught) : "n/a"} | ${ + c ? intervalCell(c.caught) : "" + } | ${notes.join("; ")} |`, + ); + } + lines.push( + `| ${caseId} (verdict) | ${ + base ? statCell(base.verdictOk) : "n/a" + } | ${base ? intervalCell(base.verdictOk) : ""} | ${ + cand ? statCell(cand.verdictOk) : "n/a" + } | ${cand ? intervalCell(cand.verdictOk) : ""} | |`, + ); + } + + const pooledRow = ( + label: string, + pick: (arm: ArmAggregate) => RateStat, + ): string => + `| ${label} | ${statCell(pick(baseline))} | ${intervalCell( + pick(baseline), + )} | ${statCell(pick(candidate))} | ${intervalCell(pick(candidate))} |`; + const dropSummary = (arm: ArmAggregate): string => { + const buckets = Object.entries(arm.pooled.foundButDropped) + .map(([bucket, count]) => `${count} ${bucket}`) + .join(", "); + return `${arm.pooled.trueMisses} true / ${ + buckets === "" ? "0 dropped" : buckets + }`; + }; + lines.push( + "", + "### Pooled", + "", + "| Metric | Baseline | 95% CI | Candidate | 95% CI |", + "| --- | --- | --- | --- | --- |", + pooledRow("Must-catch recall", (a) => a.pooled.recall), + pooledRow("Verdict agreement", (a) => a.pooled.verdictAgreement), + pooledRow("Noise (unmatched posted)", (a) => a.pooled.noise), + `| Misses (true / dropped) | ${dropSummary( + baseline, + )} | | ${dropSummary(candidate)} | |`, + ...(baseline.judgeMeanQuality !== undefined && + candidate.judgeMeanQuality !== undefined + ? [ + `| Judge mean quality | ${baseline.judgeMeanQuality.toFixed( + 2, + )} | | ${candidate.judgeMeanQuality.toFixed(2)} | |`, + ] + : []), + `| Cost | $${baseline.pooled.usd.toFixed( + 2, + )} | | $${candidate.pooled.usd.toFixed(2)} | |`, + "", + ); + + if (report.noiseFloor !== undefined) { + lines.push( + "### Noise floor (identical arms: every sample ran the same prompt)", + "", + `Bands across ${report.noiseFloor.armSamples} arm-samples of one review.md; ` + + "any A/B delta inside a band is indistinguishable from " + + "run-to-run wobble. Min/max only widen as samples accumulate; " + + "mean +/- sd is the band to track week to week.", + "", + ...(report.noiseFloor.caseAsymmetry + ? [ + "**WARNING: the samples did not all score the same " + + "case set (budget skips or mixed corpora), so " + + "these bands fold case-mix variance in on top of " + + "run-to-run wobble. Re-run with a budget that " + + "clears the full corpus before trusting them.**", + "", + ] + : []), + "| Metric | Min | Mean | Max | SD | Spread |", + "| --- | --- | --- | --- | --- | --- |", + ...Object.entries(report.noiseFloor.bands).map( + ([metric, band]) => + `| ${metric} | ${pct(band.min)} | ${pct(band.mean)} | ${pct( + band.max, + )} | ${pct(band.sd)} | ${pct(band.max - band.min)} |`, + ), + "", + ); + } + return lines.join("\n"); +}; + +/* -------------------------------------------------------------------------- */ +/* CLI */ +/* -------------------------------------------------------------------------- */ + +const argValue = (argv: string[], flag: string): string | undefined => { + const index = argv.indexOf(flag); + return index === -1 ? undefined : argv[index + 1]; +}; + +/** Resolve one CLI source (path or run id) to a parsed report payload. */ +const readSource = (source: string): unknown => { + if (/^\d+$/.test(source)) { + const dir = mkdtempSync(`${tmpdir()}/live-ab-agg-`); + execFileSync( + "gh", + ["run", "download", source, "-n", "live-ab-report", "-D", dir], + {stdio: ["ignore", "inherit", "inherit"]}, + ); + return JSON.parse(readFileSync(`${dir}/live-ab-report.json`, "utf8")); + } + return JSON.parse(readFileSync(source, "utf8")); +}; + +const main = (): void => { + const argv = process.argv.slice(2); + const outPath = argValue(argv, "--out") ?? "out/live-ab-aggregate.json"; + const sources = argv.filter( + (arg, i) => !arg.startsWith("--") && argv[i - 1] !== "--out", + ); + if (sources.length === 0) { + throw new Error( + "usage: aggregate.ts ... [--out ]", + ); + } + const samples: ReportSample[] = []; + const skipped: {source: string; reason: string}[] = []; + for (const source of sources) { + try { + const raw = readSource(source); + const extracted = extractSamples(source, raw); + if (extracted.length === 0) { + skipped.push({source, reason: "no reviewable delta"}); + } + samples.push(...extracted); + } catch (error) { + skipped.push({ + source, + reason: String(error instanceof Error ? error.message : error), + }); + } + } + const report = aggregateSamples(samples, skipped); + const markdown = renderAggregateMarkdown(report); + mkdirSync(dirname(outPath), {recursive: true}); + writeFileSync(outPath, JSON.stringify(report, null, 2)); + writeFileSync(outPath.replace(/\.json$/, ".md"), `${markdown}\n`); + console.log(markdown); + const summaryPath = process.env["GITHUB_STEP_SUMMARY"]; + if (summaryPath !== undefined && summaryPath !== "") { + writeFileSync(summaryPath, `${markdown}\n`, {flag: "a"}); + } + if (samples.length === 0) { + console.error("no report contributed any samples"); + process.exit(1); + } +}; + +// CLI entry point (mirrors live-ab.ts): run when executed, not imported. +if (process.argv[1]?.endsWith("aggregate.ts")) { + try { + main(); + } catch (error) { + console.error(error); + process.exit(1); + } +} diff --git a/workflows/review/eval/corpus/live.ts b/workflows/review/eval/corpus/live.ts index bf13fdae..672ae57b 100644 --- a/workflows/review/eval/corpus/live.ts +++ b/workflows/review/eval/corpus/live.ts @@ -37,6 +37,16 @@ export type LivePrContext = { baseBranch: string; }; +/** One anchor location a spec accepts (see {@link LiveDefectSpec}). */ +export type LiveSpecLocation = { + /** Changed-file path (must appear in the diff). */ + path: string; + /** First line of the window a matching finding may anchor in (1-based). */ + lineStart?: number; + /** Last line of the window (inclusive). Required iff lineStart is. */ + lineEnd?: number; +}; + /** * One labeled defect (or non-defect trap) in a live-enabled case. Live model * runs choose their own finding ids, so ground truth cannot key on ids the way @@ -62,6 +72,16 @@ export type LiveDefectSpec = { mechanism: string[]; /** Producing lens, when the defect is lens-specific (advisory only). */ lens?: string; + /** + * Alternate anchor locations the spec ALSO accepts. A defect that spans + * files has more than one correct anchor site (a migration missing an + * index surfaces at the migration AND at the hot query that needs it), + * and a spec that names only one turns the reviewer's anchor-site choice + * into recall noise (incident-sql-missing-index read 8/16 in the 07-09 + * wave; every "miss" was the same finding anchored at the query). The + * mechanism alternates still have to agree wherever the finding anchors. + */ + altLocations?: LiveSpecLocation[]; }; /** @@ -226,6 +246,74 @@ const parseDefectSpecs = ( errors.push(`${at}.lens: must be a non-empty string when present`); return; } + const rawAlt = entry["altLocations"]; + let altLocations: LiveSpecLocation[] | undefined; + if (rawAlt !== undefined) { + if (!Array.isArray(rawAlt) || rawAlt.length === 0) { + errors.push( + `${at}.altLocations: must be a non-empty array when present`, + ); + return; + } + altLocations = []; + let altBroken = false; + rawAlt.forEach((alt, j) => { + const altAt = `${at}.altLocations[${j}]`; + if (!isRecord(alt)) { + errors.push(`${altAt}: must be an object`); + altBroken = true; + return; + } + const altPath = alt["path"]; + if (!isNonEmptyString(altPath)) { + errors.push(`${altAt}.path: required non-empty string`); + altBroken = true; + return; + } + if (!changedPaths.has(altPath)) { + errors.push( + `${altAt}.path: "${altPath}" is not in changedFiles`, + ); + } + if (diffPaths !== undefined && !diffPaths.has(altPath)) { + errors.push( + `${altAt}.path: "${altPath}" has no section in the diff`, + ); + } + const altStart = alt["lineStart"]; + const altEnd = alt["lineEnd"]; + if ((altStart === undefined) !== (altEnd === undefined)) { + errors.push( + `${altAt}: lineStart and lineEnd must be set together`, + ); + altBroken = true; + return; + } + if (altStart !== undefined) { + if ( + !Number.isInteger(altStart) || + !Number.isInteger(altEnd) || + (altStart as number) < 1 || + (altEnd as number) < (altStart as number) + ) { + errors.push( + `${altAt}: lineStart/lineEnd must be positive integers with lineStart <= lineEnd`, + ); + altBroken = true; + return; + } + } + const location: LiveSpecLocation = {path: altPath}; + if (altStart !== undefined) { + location.lineStart = altStart as number; + location.lineEnd = altEnd as number; + } + altLocations?.push(location); + }); + if (altBroken) { + return; + } + } const spec: LiveDefectSpec = { key: specKey, path, @@ -238,6 +326,9 @@ const parseDefectSpecs = ( if (isNonEmptyString(lens)) { spec.lens = lens; } + if (altLocations !== undefined) { + spec.altLocations = altLocations; + } specs.push(spec); }); return specs; diff --git a/workflows/review/eval/corpus/loader.test.ts b/workflows/review/eval/corpus/loader.test.ts index 566b9f4e..4099e765 100644 --- a/workflows/review/eval/corpus/loader.test.ts +++ b/workflows/review/eval/corpus/loader.test.ts @@ -131,6 +131,56 @@ describe("parseCase: the live block", () => { expect(parsed.live?.mustNotFlagSpecs?.[0]?.lineStart).toBeUndefined(); }); + it("parses and validates altLocations like the primary location", () => { + const withAlt = (altLocations: unknown) => + liveCase({ + changedFiles: [ + {path: "src/a.ts", status: "modified"}, + {path: "src/only-listed.ts", status: "modified"}, + ], + live: { + prContext: { + title: "t", + description: "d", + author: "a", + baseBranch: "main", + }, + mustCatchSpecs: [ + { + key: "k", + path: "src/a.ts", + mechanism: ["m"], + altLocations, + }, + ], + }, + }); + const parsed = parseCase( + withAlt([{path: "src/a.ts", lineStart: 1, lineEnd: 2}]), + "test://case", + ); + expect(parsed.live?.mustCatchSpecs?.[0]?.altLocations).toEqual([ + {path: "src/a.ts", lineStart: 1, lineEnd: 2}, + ]); + expect(parseErrors(withAlt([]))).toMatch( + /altLocations: must be a non-empty array/, + ); + expect(parseErrors(withAlt([{path: "src/nope.ts"}]))).toMatch( + /altLocations\[0\]\.path: "src\/nope\.ts" is not in changedFiles/, + ); + expect(parseErrors(withAlt([{path: "src/only-listed.ts"}]))).toMatch( + /no section in the diff/, + ); + expect( + parseErrors(withAlt([{path: "src/a.ts", lineStart: 3}])), + ).toMatch(/must be set together/); + expect( + parseErrors( + withAlt([{path: "src/a.ts", lineStart: 5, lineEnd: 3}]), + ), + ).toMatch(/lineStart <= lineEnd/); + }); + it("requires a diff on a live case", () => { const raw = liveCase(); delete (raw as Record)["diff"]; diff --git a/workflows/review/eval/corpus/loader.ts b/workflows/review/eval/corpus/loader.ts index 35b9e572..b520a32f 100644 --- a/workflows/review/eval/corpus/loader.ts +++ b/workflows/review/eval/corpus/loader.ts @@ -45,6 +45,7 @@ export type { CaseRereview, LiveDefectSpec, LivePrContext, + LiveSpecLocation, RereviewPriorThread, } from "./live"; diff --git a/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json b/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json index 2acd619c..cc333498 100644 --- a/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json +++ b/workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json @@ -7,7 +7,7 @@ "live" ], "category": "incident-repro", - "description": "Repro of a production incident: a migration adds a column filtered by a hot query but no index, causing a table scan under load. The data-migrations lens must catch it and block.", + "description": "Repro of a production incident: a migration adds a column filtered by a hot query but no index, causing a table scan under load. The data-migrations lens must catch it and block. The defect spans two files, so the spec accepts an anchor at the migration OR at the hot query in order.ts (the 07-09 wave read 8/16 on this case, and every 'miss' was the same blocking finding anchored at the query site; a single-location spec measured anchor-site preference, not recall).", "changedFiles": [ { "path": "db/migrations/20260601_add_status.sql", @@ -76,7 +76,14 @@ ], "lens": "data-migrations", "lineStart": 1, - "lineEnd": 5 + "lineEnd": 5, + "altLocations": [ + { + "path": "src/models/order.ts", + "lineStart": 10, + "lineEnd": 18 + } + ] } ] }, diff --git a/workflows/review/eval/live-ab-report.ts b/workflows/review/eval/live-ab-report.ts new file mode 100644 index 00000000..14d036cd --- /dev/null +++ b/workflows/review/eval/live-ab-report.ts @@ -0,0 +1,444 @@ +/** + * The live A/B report shapes and their markdown rendering, split out of + * `live-ab.ts` (which keeps the arm execution, gates, and CLI). This module + * is a leaf: it depends only on types from the matcher/producer layers and + * on the aggregation core, so the runner, its tests, and any future report + * consumer can share the shapes without importing the CLI. + */ + +import {renderAggregateMarkdown, type AggregateReport} from "./aggregate"; +import type { + CaseVerification, + CorpusCase, + RecordedFinding, +} from "./corpus/loader"; +import type {LiveCaseRun, LiveMetricsReport} from "./live-match"; +import type {LiveReconciliation, PerAgentReport} from "./live-producer"; +import type {RereviewCaseScore, RereviewMetricsReport} from "./rereview-match"; + +export type ArmId = "baseline" | "candidate"; + +/** What an arm's producer must return per case (the produceLive subset). */ +export type ArmProduceResult = { + findings: RecordedFinding[]; + validation: CaseVerification[]; + perAgent: PerAgentReport[]; + /** The reconciler's decision, for open-PR (rereview) cases. */ + reconciliation?: LiveReconciliation; +}; + +export type ArmProduce = (corpusCase: CorpusCase) => Promise; + +export type ArmRunReport = { + arm: ArmId; + runs: LiveCaseRun[]; + metrics: LiveMetricsReport; + /** Case ids never dispatched because the budget ran out. */ + skippedCases: string[]; + usd: number; + wallMs: number; + perCase: { + caseId: string; + usd: number; + verdict: string; + expected: string; + caught: number; + missed: string[]; + /** `: ` per failed agent (diagnosable from the report). */ + failedAgents: string[]; + /** Present iff the case is an open-PR (rereview) case. */ + rereview?: RereviewCaseScore; + }[]; + /** Aggregated re-review scoring, when the corpus carried rereview cases. */ + rereview?: RereviewMetricsReport; + judge?: {meanQuality: number; verdictCounts: Record}; + /** Fixed-format note when judge scoring failed; metrics still stand. */ + judgeError?: string; +}; + +export type GateRetryAttempt = { + pass: boolean; + /** Gate failures this attempt produced (empty when pass). */ + failures: string[]; + usd: number; +}; + +export type GateRetry = { + caseId: string; + /** Re-run attempts, in order; the second is skipped once majority-fail is settled. */ + attempts: GateRetryAttempt[]; + /** True when 2 of 3 runs (original + retries) passed: a run-to-run flake. */ + settledPass: boolean; +}; + +/** One case's adversarial-gate outcome across a repeated run. */ +export type GateMajority = { + caseId: string; + failedRepeats: number; + repeats: number; + /** Strict majority of repeats failed: a confirmed regression, not a flake. */ + confirmed: boolean; +}; + +/** + * The ruler this report was scored with. Two runs are only comparable when + * BOTH the prompt (reviewMdSha) and the ruler match: a matcher-config change + * (arbiter on/off) or a corpus change moves every rate without the reviewer + * changing at all. aggregate.ts warns when a pool mixes rulers, which is + * what keeps the weekly drift series honest across instrument upgrades. + */ +export type ReportProvenance = { + /** Matcher configuration: `deterministic` or `deterministic+arbiter`. */ + matcher: string; + /** Content hash of the loaded corpus cases this run was scored against. */ + corpusSha: string; + caseCount: number; +}; + +export type AbReport = { + baseRef: string; + reviewMdSha: {baseline: string; candidate: string}; + /** Absent only on artifacts predating the ruler stamp. */ + provenance?: ReportProvenance; + arms: {baseline: ArmRunReport; candidate: ArmRunReport}; + regressions: {lost: string[]; gained: string[]}; + /** Confirmed failures only: flips settled as flakes by retry are removed. */ + adversarialFailures: string[]; + /** Best-of-three re-runs of the cases that flipped the hard gate. */ + gateRetries: GateRetry[]; +}; + +/** + * The `--repeats n` report: every repeat's full single-run report (so any + * one repeat stays diagnosable and re-aggregatable), the pooled aggregate, + * and the majority-decided gate. `aggregate.ts` recognises the `repeats` + * field, so a multi-run artifact pools across dispatches like any other. + */ +export type MultiAbReport = { + repeatCount: number; + repeats: AbReport[]; + aggregate: AggregateReport; + gate: GateMajority[]; + /** Cases failing the candidate gate in a strict majority of repeats. */ + adversarialFailures: string[]; +}; + +export const renderMultiMarkdownReport = (report: MultiAbReport): string => { + const first = report.repeats[0]; + const lines = [ + `## Review live A/B: ${report.repeatCount} repeats`, + "", + ...(first !== undefined + ? [ + `Baseline: \`${ + first.baseRef + }\` (review.md ${first.reviewMdSha.baseline.slice( + 0, + 12, + )}); candidate: working tree (review.md ${first.reviewMdSha.candidate.slice( + 0, + 12, + )}).`, + "", + ] + : []), + renderAggregateMarkdown(report.aggregate), + "", + ]; + if (report.gate.length === 0) { + lines.push( + "Adversarial hard gate: PASSED on the candidate arm in every repeat.", + "", + ); + } else { + lines.push( + "### Adversarial hard gate (strict majority over repeats)", + "", + ...report.gate.map( + (g) => + `- ${g.caseId}: failed ${g.failedRepeats}/${ + g.repeats + } repeats: ${ + g.confirmed + ? "FAILURE CONFIRMED" + : "minority flip, treated as run-to-run flake" + }`, + ), + "", + ); + } + return lines.join("\n"); +}; + +/** `caseId:specKey` -> drop bucket, for every found-but-dropped miss. */ +const dropClassByKey = (arm: ArmRunReport): Map => { + const map = new Map(); + for (const {corpusCase, match} of arm.runs) { + for (const detail of match.missedDetail) { + if (detail.droppedBy !== undefined) { + map.set(`${corpusCase.id}:${detail.specKey}`, detail.droppedBy); + } + } + } + return map; +}; + +const pct = (value: number): string => `${(value * 100).toFixed(0)}%`; + +/** + * Which report rows a reader may act on from ONE run. Measured on the phase 4 + * acceptance pair: on the no-op control, recall, verdict agreement, the + * regression lists, and the adversarial gate reproduced exactly while judge + * quality moved 0.11 and noise 5 points on live-agent jitter alone; on the + * weakened-reviewer arm, judge quality went UP 0.16 while recall fell 17 + * points (fewer, surer comments each score better). So judge quality is not + * just jittery, it can move opposite to review health; recall against the + * labeled specs is the load-bearing metric. + */ +const STABILITY_FOOTER = + "*Single-run-stable rows: recall, verdict agreement, regressions, " + + "adversarial gate. Judge quality and noise are not: they jitter " + + "run-to-run at this corpus size, and a regressed reviewer can score " + + "HIGHER on judge quality (fewer, surer comments each read better). " + + "Recall against the labeled specs is the load-bearing metric.*"; + +/** + * The measured run-to-run wobble of each report row, from IDENTICAL arms: + * gh run 29069228968 (2026-07-10), `--force-arms --repeats 3` over the full + * 14-case live corpus, i.e. 6 arm-samples of one review.md + * (e55e2ace5c95..., deterministic matcher, pre-arbiter). Rendered into every + * single-run report so a reader prices a delta against measured wobble, not + * prose. The weekly drift run re-measures these bands (with the arbiter + * active); update the constants when they move materially. + */ +export const MEASURED_NOISE_FLOOR = { + provenance: + "identical arms, run 29069228968, 2026-07-10, 6 arm-samples, " + + "full corpus x3, pre-arbiter; budget skips left the samples on " + + "unequal case sets, so these v1 bands also carry case-mix variance", + bands: [ + {metric: "must-catch recall", min: 0.54, max: 0.86, sd: 0.1}, + {metric: "verdict agreement", min: 0.75, max: 1.0, sd: 0.09}, + {metric: "noise (unmatched posted)", min: 0.5, max: 0.6, sd: 0.03}, + {metric: "judge mean quality", min: 0.82, max: 0.86, sd: 0.02}, + ], +} as const; + +const NOISE_FLOOR_FOOTER = + "*Measured noise floor (" + + MEASURED_NOISE_FLOOR.provenance + + "): " + + MEASURED_NOISE_FLOOR.bands + .map((b) => `${b.metric} ${pct(b.min)}-${pct(b.max)} (sd ${pct(b.sd)})`) + .join(", ") + + ". A single-run delta whose arms both sit inside a band is " + + "indistinguishable from run-to-run wobble; use `--repeats` to resolve " + + "smaller effects.*"; + +export const renderMarkdownReport = (report: AbReport): string => { + const {baseline, candidate} = report.arms; + const row = ( + label: string, + base: string, + cand: string, + delta = "", + ): string => `| ${label} | ${base} | ${cand} | ${delta} |`; + const metric = ( + label: string, + pick: (arm: ArmRunReport) => number, + format: (v: number) => string = pct, + ): string => + row( + label, + format(pick(baseline)), + format(pick(candidate)), + (pick(candidate) - pick(baseline) >= 0 ? "+" : "") + + format(pick(candidate) - pick(baseline)), + ); + + const lines = [ + "## Review live A/B", + "", + `Baseline: \`${ + report.baseRef + }\` (review.md ${report.reviewMdSha.baseline.slice(0, 12)}); ` + + `candidate: working tree (review.md ${report.reviewMdSha.candidate.slice( + 0, + 12, + )}).`, + "", + ...(report.provenance !== undefined + ? [ + `Ruler: matcher ${report.provenance.matcher}; corpus ` + + `${report.provenance.corpusSha.slice(0, 12)} ` + + `(${report.provenance.caseCount} cases).`, + "", + ] + : []), + "| Metric | Baseline | Candidate | Delta |", + "| --- | --- | --- | --- |", + metric("Must-catch recall", (a) => a.metrics.mustCatchRecall.rate), + metric("Verdict agreement", (a) => a.metrics.verdictAgreement.rate), + metric("Noise (unmatched posted)", (a) => a.metrics.noise.rate), + row( + "Clean false flags", + String(baseline.metrics.cleanFalseFlag.count), + String(candidate.metrics.cleanFalseFlag.count), + ), + ...(baseline.judge && candidate.judge + ? [ + row( + "Judge mean quality", + baseline.judge.meanQuality.toFixed(2), + candidate.judge.meanQuality.toFixed(2), + (candidate.judge.meanQuality - + baseline.judge.meanQuality >= + 0 + ? "+" + : "") + + ( + candidate.judge.meanQuality - + baseline.judge.meanQuality + ).toFixed(2), + ), + ] + : []), + row( + "Cost", + `$${baseline.usd.toFixed(2)}`, + `$${candidate.usd.toFixed(2)}`, + ), + row( + "Wall clock", + `${Math.round(baseline.wallMs / 1000)}s`, + `${Math.round(candidate.wallMs / 1000)}s`, + ), + row( + "Cases run / skipped", + `${baseline.runs.length} / ${baseline.skippedCases.length}`, + `${candidate.runs.length} / ${candidate.skippedCases.length}`, + ), + ...(baseline.rereview || candidate.rereview + ? [ + row( + "Re-review thread resolution", + baseline.rereview + ? pct(baseline.rereview.resolutionAccuracy) + : "n/a", + candidate.rereview + ? pct(candidate.rereview.resolutionAccuracy) + : "n/a", + ), + row( + "Re-review flip-gate wrong / dup comments", + baseline.rereview + ? `${baseline.rereview.flipGateWrongCases.length} / ${baseline.rereview.duplicateComments}` + : "n/a", + candidate.rereview + ? `${candidate.rereview.flipGateWrongCases.length} / ${candidate.rereview.duplicateComments}` + : "n/a", + ), + ] + : []), + row( + "Misses found-but-dropped", + String(dropClassByKey(baseline).size), + String(dropClassByKey(candidate).size), + ), + "", + ]; + + // A regression that was PRODUCED and then died at a gate is a different + // defect class (anchoring discipline, gate calibration) than a true miss + // (recall); annotate each lost spec so it routes to the right fix. + const candidateDrops = dropClassByKey(candidate); + if (report.regressions.lost.length > 0) { + lines.push( + "### Regressions (baseline caught, candidate missed)", + "", + ...report.regressions.lost.map((key) => { + const bucket = candidateDrops.get(key); + return bucket === undefined + ? `- ${key} (not found)` + : `- ${key} (found but dropped at the ${bucket} gate)`; + }), + "", + ); + } + if (report.regressions.gained.length > 0) { + lines.push( + "### Improvements (candidate caught, baseline missed)", + "", + ...report.regressions.gained.map((key) => `- ${key}`), + "", + ); + } + lines.push( + report.adversarialFailures.length === 0 + ? "Adversarial hard gate: PASSED on the candidate arm." + : [ + "### Adversarial hard gate: FAILED on the candidate arm", + "", + ...report.adversarialFailures.map((f) => `- ${f}`), + ].join("\n"), + "", + ); + if (report.gateRetries.length > 0) { + lines.push( + "### Gate flips retried (best of three, flipped cases only)", + "", + ...report.gateRetries.map((retry) => { + const passes = retry.attempts.filter((a) => a.pass).length; + const usd = retry.attempts.reduce((sum, a) => sum + a.usd, 0); + const outcome = retry.settledPass + ? "settled as a run-to-run flake; the gate does not fail on this case" + : "failure confirmed"; + return `- ${retry.caseId}: original run failed, ${passes}/${ + retry.attempts.length + } retries passed; ${outcome} ($${usd.toFixed(2)} retry spend)`; + }), + "", + ); + } + const skipped = [ + ...baseline.skippedCases.map((id) => `baseline:${id}`), + ...candidate.skippedCases.map((id) => `candidate:${id}`), + ]; + if (skipped.length > 0) { + lines.push( + "### SKIPPED (budget exhausted before dispatch)", + "", + ...skipped.map((s) => `- ${s}`), + "", + ); + } + const judgeErrors = [ + ...(baseline.judgeError !== undefined + ? [`baseline: ${baseline.judgeError}`] + : []), + ...(candidate.judgeError !== undefined + ? [`candidate: ${candidate.judgeError}`] + : []), + ]; + if (judgeErrors.length > 0) { + lines.push( + "### Judge scoring failed (metrics above still stand)", + "", + ...judgeErrors.map((e) => `- ${e}`), + "", + ); + } + const failedAgents = [...baseline.perCase, ...candidate.perCase].flatMap( + (c) => c.failedAgents.map((agent) => `${c.caseId}: ${agent} failed`), + ); + if (failedAgents.length > 0) { + lines.push( + "### Agent failures", + "", + ...failedAgents.map((f) => `- ${f}`), + "", + ); + } + lines.push(STABILITY_FOOTER, "", NOISE_FLOOR_FOOTER, ""); + return lines.join("\n"); +}; diff --git a/workflows/review/eval/live-ab.test.ts b/workflows/review/eval/live-ab.test.ts index aca111ca..a8a1f21d 100644 --- a/workflows/review/eval/live-ab.test.ts +++ b/workflows/review/eval/live-ab.test.ts @@ -1,15 +1,19 @@ import {describe, it, expect} from "vitest"; +import {aggregateSamples, extractSamples} from "./aggregate"; import {parseCase, type CorpusCase} from "./corpus/loader"; import { adversarialGateFailures, diffRegressions, + majorityGateFailures, renderMarkdownReport, + renderMultiMarkdownReport, retryGateFlips, runArm, type AbReport, type ArmProduce, type ArmRunReport, + type MultiAbReport, } from "./live-ab"; const DIFF = [ @@ -267,6 +271,133 @@ describe("retryGateFlips", () => { }); }); +describe("majorityGateFailures", () => { + const adversarial = liveCase("adv-1", { + category: "adversarial-injection", + }); + + it("confirms a failure only on a strict majority of repeats", async () => { + const fail = await runArm("candidate", [adversarial], produceMiss, { + maxUsd: 100, + }); + const pass = await runArm("candidate", [adversarial], produceHit(1), { + maxUsd: 100, + }); + // 1/3 failed: a flake, not a confirmed failure. + expect(majorityGateFailures([fail, pass, pass])).toEqual([ + {caseId: "adv-1", failedRepeats: 1, repeats: 3, confirmed: false}, + ]); + // 2/3 failed: confirmed. + expect(majorityGateFailures([fail, fail, pass])[0]?.confirmed).toBe( + true, + ); + // Exactly half (1/2) is NOT a strict majority. + expect(majorityGateFailures([fail, pass])[0]?.confirmed).toBe(false); + // No failures anywhere: nothing to report. + expect(majorityGateFailures([pass, pass])).toEqual([]); + }); +}); + +describe("renderMultiMarkdownReport", () => { + const multiReport = async ( + produces: ArmProduce[], + ): Promise => { + const cases = [liveCase("case-1")]; + const reports: AbReport[] = []; + for (const produce of produces) { + const baseline = await runArm("baseline", cases, produceHit(1), { + maxUsd: 100, + }); + const candidate = await runArm("candidate", cases, produce, { + maxUsd: 100, + }); + reports.push({ + baseRef: "origin/main", + reviewMdSha: { + baseline: "a".repeat(64), + candidate: "b".repeat(64), + }, + arms: {baseline, candidate}, + regressions: diffRegressions(baseline, candidate), + adversarialFailures: [], + gateRetries: [], + }); + } + const gate = majorityGateFailures(reports.map((r) => r.arms.candidate)); + return { + repeatCount: reports.length, + repeats: reports, + aggregate: aggregateSamples( + reports.flatMap((r, i) => extractSamples(`repeat-${i + 1}`, r)), + ), + gate, + adversarialFailures: gate + .filter((g) => g.confirmed) + .map( + (g) => + `${g.caseId}: failed ${g.failedRepeats}/${g.repeats} repeats`, + ), + }; + }; + + it("renders pooled pass rates with intervals instead of single-run deltas", async () => { + const multi = await multiReport([produceHit(1), produceMiss]); + const markdown = renderMultiMarkdownReport(multi); + expect(markdown).toContain("## Review live A/B: 2 repeats"); + // The candidate caught the spec in 1 of 2 repeats: a pass RATE row. + expect(markdown).toContain("| case-1:bug | 2/2 (100%)"); + expect(markdown).toContain("| 1/2 (50%)"); + expect(markdown).toContain("### Pooled"); + expect(markdown).toContain( + "Adversarial hard gate: PASSED on the candidate arm in every repeat.", + ); + }); + + it("reports minority gate flips as flakes and majorities as confirmed", async () => { + const adversarial = liveCase("adv-1", { + category: "adversarial-injection", + }); + const armFor = async (produce: ArmProduce): Promise => + runArm("candidate", [adversarial], produce, {maxUsd: 100}); + const fail = await armFor(produceMiss); + const pass = await armFor(produceHit(1)); + const gate = majorityGateFailures([fail, pass, pass]); + const markdown = renderMultiMarkdownReport({ + repeatCount: 3, + repeats: [], + aggregate: aggregateSamples([]), + gate, + adversarialFailures: [], + }); + expect(markdown).toContain( + "- adv-1: failed 1/3 repeats: minority flip, treated as run-to-run flake", + ); + const confirmed = majorityGateFailures([fail, fail, pass]); + expect( + renderMultiMarkdownReport({ + repeatCount: 3, + repeats: [], + aggregate: aggregateSamples([]), + gate: confirmed, + adversarialFailures: ["adv-1: failed 2/3 repeats"], + }), + ).toContain("- adv-1: failed 2/3 repeats: FAILURE CONFIRMED"); + }); + + it("round-trips a MultiAbReport through the aggregate extractor", async () => { + // The --repeats artifact must stay poolable across dispatches: the + // aggregate CLI reads the `repeats` field of a multi-run report. + const multi = await multiReport([produceHit(1), produceHit(1)]); + const samples = extractSamples( + "multi.json", + JSON.parse(JSON.stringify(multi)), + ); + expect(samples.length).toBe(2); + const pooled = aggregateSamples(samples); + expect(pooled.arms.candidate.pooled.recall.numerator).toBe(2); + }); +}); + describe("renderMarkdownReport", () => { it("renders the arm table, regressions, gate status, and skips", async () => { const cases = [liveCase("case-1"), liveCase("case-2")]; @@ -294,9 +425,17 @@ describe("renderMarkdownReport", () => { expect(markdown).toContain("Adversarial hard gate: PASSED"); expect(markdown).toContain("SKIPPED (budget exhausted"); expect(markdown).toContain("- candidate:case-2"); - // The stability footer closes every report, so a reader always sees - // which rows are single-run signals and which are cross-run trends. - expect(markdown.trimEnd().endsWith("load-bearing metric.*")).toBe(true); + // The stability footer plus the measured noise floor close every + // report, so a reader always sees which rows are single-run signals + // and prices any delta against measured wobble, not prose. + expect(markdown).toContain("load-bearing metric.*"); + expect(markdown).toContain( + "Measured noise floor (identical arms, run 29069228968", + ); + expect(markdown).toContain("must-catch recall 54%-86% (sd 10%)"); + expect(markdown.trimEnd().endsWith("resolve smaller effects.*")).toBe( + true, + ); }); it("marks a failed adversarial gate", () => { diff --git a/workflows/review/eval/live-ab.ts b/workflows/review/eval/live-ab.ts index ea7ab14e..8360bca4 100644 --- a/workflows/review/eval/live-ab.ts +++ b/workflows/review/eval/live-ab.ts @@ -23,6 +23,9 @@ * default in CI; a full-eval label lifts it) * [--max-usd ] total hard budget across both arms (default 40) * [--no-judge] skip judge quality scoring + * [--no-match-arbiter] deterministic spec matching only (skip the + * capped Haiku fallback arbiter on unmatched + * specs; see match-arbiter.ts) * [--stage-root ] staging root (default: a fresh temp dir) * [--out ] JSON report path (default out/live-ab-report.json) * [--re-review-mode ] re-review mode for the CANDIDATE arm on open-PR @@ -34,6 +37,16 @@ * byte-identical (a deliberate wobble control); * without it, identical arms short-circuit to a * "no reviewable delta" report at zero cost. + * [--repeats ] run every selected case n times per arm in this + * one dispatch and report pooled per-case pass + * rates with binomial intervals (aggregate.ts) + * instead of single-run percentages; the memo's + * "targeted repeats" powered run (e.g. --cases + * --repeats 10). + * The adversarial gate is decided by strict + * majority across repeats (the repeat structure + * replaces the single-run best-of-three retry). + * Default 1 (single-run behavior, unchanged). */ /* eslint-disable no-console -- CLI entry point; console IS the interface. */ @@ -45,70 +58,53 @@ import {tmpdir} from "node:os"; import {dirname} from "node:path"; import {extractAgents} from "./agent-extract"; +import {aggregateSamples, extractSamples} from "./aggregate"; import {SMOKE_TAG, loadLiveCorpus, type CorpusCase} from "./corpus/loader"; import {aggregate, buildCorpusRequests} from "./judge"; import {liveJudgeModel} from "./judge-live-model"; +import { + renderMarkdownReport, + renderMultiMarkdownReport, + type AbReport, + type ArmId, + type ArmProduce, + type ArmRunReport, + type GateMajority, + type GateRetry, + type GateRetryAttempt, + type MultiAbReport, +} from "./live-ab-report"; import { computeLiveMetrics, matchCase, type LiveCaseRun, - type LiveMetricsReport, + type MatchOptions, } from "./live-match"; -import { - produceLive, - type LiveReconciliation, - type PerAgentReport, -} from "./live-producer"; +import {produceLive} from "./live-producer"; import {sdkRunner} from "./live-runner"; +import {haikuMatchArbiter} from "./match-arbiter"; import { computeRereviewMetrics, scoreRereview, type RereviewCaseScore, - type RereviewMetricsReport, } from "./rereview-match"; import {runCase} from "./runner"; -import type {CaseVerification, RecordedFinding} from "./corpus/loader"; import type {ReReviewMode} from "../lib/routing-config"; -export type ArmId = "baseline" | "candidate"; - -/** What an arm's producer must return per case (the produceLive subset). */ -export type ArmProduceResult = { - findings: RecordedFinding[]; - validation: CaseVerification[]; - perAgent: PerAgentReport[]; - /** The reconciler's decision, for open-PR (rereview) cases. */ - reconciliation?: LiveReconciliation; -}; - -export type ArmProduce = (corpusCase: CorpusCase) => Promise; - -export type ArmRunReport = { - arm: ArmId; - runs: LiveCaseRun[]; - metrics: LiveMetricsReport; - /** Case ids never dispatched because the budget ran out. */ - skippedCases: string[]; - usd: number; - wallMs: number; - perCase: { - caseId: string; - usd: number; - verdict: string; - expected: string; - caught: number; - missed: string[]; - /** `: ` per failed agent (diagnosable from the report). */ - failedAgents: string[]; - /** Present iff the case is an open-PR (rereview) case. */ - rereview?: RereviewCaseScore; - }[]; - /** Aggregated re-review scoring, when the corpus carried rereview cases. */ - rereview?: RereviewMetricsReport; - judge?: {meanQuality: number; verdictCounts: Record}; - /** Fixed-format note when judge scoring failed; metrics still stand. */ - judgeError?: string; -}; +// The report shapes and renderers live in ./live-ab-report; re-exported so +// existing consumers keep one import surface for the runner. +export {renderMarkdownReport, renderMultiMarkdownReport}; +export type { + AbReport, + ArmId, + ArmProduce, + ArmProduceResult, + ArmRunReport, + GateMajority, + GateRetry, + GateRetryAttempt, + MultiAbReport, +} from "./live-ab-report"; /* -------------------------------------------------------------------------- */ /* One arm */ @@ -125,7 +121,7 @@ export const runArm = async ( arm: ArmId, cases: CorpusCase[], produce: ArmProduce, - options: {maxUsd: number}, + options: {maxUsd: number; match?: MatchOptions}, ): Promise => { const started = Date.now(); const runs: LiveCaseRun[] = []; @@ -154,7 +150,7 @@ export const runArm = async ( produceFindings: () => produced.findings, validation: produced.validation, }); - const match = await matchCase(corpusCase, result); + const match = await matchCase(corpusCase, result, options.match); runs.push({corpusCase, result, match}); // Open-PR (rereview) cases: score the reconciler's decision and the @@ -267,25 +263,41 @@ export const adversarialGateFailures = ( return failures; }; +/** + * The adversarial gate over a repeated run: per case, how many repeats' + * candidate arms failed, confirmed by STRICT majority. One flip among n + * repeats is the run-to-run flake the single-run path spends a best-of-three + * retry on; with repeats the evidence is already bought, so no retry runs and + * the gate fails only when more repeats failed a case than passed it. + */ +export const majorityGateFailures = ( + candidates: Pick[], +): GateMajority[] => { + const failCounts = new Map(); + for (const candidate of candidates) { + const failedCases = new Set( + adversarialGateFailures(candidate).map((f) => + f.slice(0, f.indexOf(":")), + ), + ); + for (const caseId of failedCases) { + failCounts.set(caseId, (failCounts.get(caseId) ?? 0) + 1); + } + } + return [...failCounts.entries()] + .sort(([a], [b]) => a.localeCompare(b)) + .map(([caseId, failedRepeats]) => ({ + caseId, + failedRepeats, + repeats: candidates.length, + confirmed: failedRepeats * 2 > candidates.length, + })); +}; + /* -------------------------------------------------------------------------- */ /* Gate-flip retry */ /* -------------------------------------------------------------------------- */ -export type GateRetryAttempt = { - pass: boolean; - /** Gate failures this attempt produced (empty when pass). */ - failures: string[]; - usd: number; -}; - -export type GateRetry = { - caseId: string; - /** Re-run attempts, in order; the second is skipped once majority-fail is settled. */ - attempts: GateRetryAttempt[]; - /** True when 2 of 3 runs (original + retries) passed: a run-to-run flake. */ - settledPass: boolean; -}; - /** * Best-of-three retry over the cases that flipped the adversarial hard gate, * and ONLY those cases (the tuning memo's flake policy: retry the flip, not @@ -299,6 +311,7 @@ export const retryGateFlips = async ( candidate: ArmRunReport, cases: CorpusCase[], produceForAttempt: (attempt: number) => ArmProduce, + match?: MatchOptions, ): Promise => { const failures = adversarialGateFailures(candidate); const failingIds = [ @@ -318,9 +331,9 @@ export const retryGateFlips = async ( produceFindings: () => produced.findings, validation: produced.validation, }); - const match = await matchCase(corpusCase, result); + const matched = await matchCase(corpusCase, result, match); const attemptFailures = adversarialGateFailures({ - runs: [{corpusCase, result, match}], + runs: [{corpusCase, result, match: matched}], }); attempts.push({ pass: attemptFailures.length === 0, @@ -340,253 +353,6 @@ export const retryGateFlips = async ( return retries; }; -/* -------------------------------------------------------------------------- */ -/* Report rendering */ -/* -------------------------------------------------------------------------- */ - -export type AbReport = { - baseRef: string; - reviewMdSha: {baseline: string; candidate: string}; - arms: {baseline: ArmRunReport; candidate: ArmRunReport}; - regressions: {lost: string[]; gained: string[]}; - /** Confirmed failures only: flips settled as flakes by retry are removed. */ - adversarialFailures: string[]; - /** Best-of-three re-runs of the cases that flipped the hard gate. */ - gateRetries: GateRetry[]; -}; - -/** `caseId:specKey` -> drop bucket, for every found-but-dropped miss. */ -const dropClassByKey = (arm: ArmRunReport): Map => { - const map = new Map(); - for (const {corpusCase, match} of arm.runs) { - for (const detail of match.missedDetail) { - if (detail.droppedBy !== undefined) { - map.set(`${corpusCase.id}:${detail.specKey}`, detail.droppedBy); - } - } - } - return map; -}; - -const pct = (value: number): string => `${(value * 100).toFixed(0)}%`; - -/** - * Which report rows a reader may act on from ONE run. Measured on the phase 4 - * acceptance pair: on the no-op control, recall, verdict agreement, the - * regression lists, and the adversarial gate reproduced exactly while judge - * quality moved 0.11 and noise 5 points on live-agent jitter alone; on the - * weakened-reviewer arm, judge quality went UP 0.16 while recall fell 17 - * points (fewer, surer comments each score better). So judge quality is not - * just jittery, it can move opposite to review health; recall against the - * labeled specs is the load-bearing metric. - */ -const STABILITY_FOOTER = - "*Single-run-stable rows: recall, verdict agreement, regressions, " + - "adversarial gate. Judge quality and noise are not: they jitter " + - "run-to-run at this corpus size, and a regressed reviewer can score " + - "HIGHER on judge quality (fewer, surer comments each read better). " + - "Recall against the labeled specs is the load-bearing metric.*"; - -export const renderMarkdownReport = (report: AbReport): string => { - const {baseline, candidate} = report.arms; - const row = ( - label: string, - base: string, - cand: string, - delta = "", - ): string => `| ${label} | ${base} | ${cand} | ${delta} |`; - const metric = ( - label: string, - pick: (arm: ArmRunReport) => number, - format: (v: number) => string = pct, - ): string => - row( - label, - format(pick(baseline)), - format(pick(candidate)), - (pick(candidate) - pick(baseline) >= 0 ? "+" : "") + - format(pick(candidate) - pick(baseline)), - ); - - const lines = [ - "## Review live A/B", - "", - `Baseline: \`${ - report.baseRef - }\` (review.md ${report.reviewMdSha.baseline.slice(0, 12)}); ` + - `candidate: working tree (review.md ${report.reviewMdSha.candidate.slice( - 0, - 12, - )}).`, - "", - "| Metric | Baseline | Candidate | Delta |", - "| --- | --- | --- | --- |", - metric("Must-catch recall", (a) => a.metrics.mustCatchRecall.rate), - metric("Verdict agreement", (a) => a.metrics.verdictAgreement.rate), - metric("Noise (unmatched posted)", (a) => a.metrics.noise.rate), - row( - "Clean false flags", - String(baseline.metrics.cleanFalseFlag.count), - String(candidate.metrics.cleanFalseFlag.count), - ), - ...(baseline.judge && candidate.judge - ? [ - row( - "Judge mean quality", - baseline.judge.meanQuality.toFixed(2), - candidate.judge.meanQuality.toFixed(2), - (candidate.judge.meanQuality - - baseline.judge.meanQuality >= - 0 - ? "+" - : "") + - ( - candidate.judge.meanQuality - - baseline.judge.meanQuality - ).toFixed(2), - ), - ] - : []), - row( - "Cost", - `$${baseline.usd.toFixed(2)}`, - `$${candidate.usd.toFixed(2)}`, - ), - row( - "Wall clock", - `${Math.round(baseline.wallMs / 1000)}s`, - `${Math.round(candidate.wallMs / 1000)}s`, - ), - row( - "Cases run / skipped", - `${baseline.runs.length} / ${baseline.skippedCases.length}`, - `${candidate.runs.length} / ${candidate.skippedCases.length}`, - ), - ...(baseline.rereview || candidate.rereview - ? [ - row( - "Re-review thread resolution", - baseline.rereview - ? pct(baseline.rereview.resolutionAccuracy) - : "n/a", - candidate.rereview - ? pct(candidate.rereview.resolutionAccuracy) - : "n/a", - ), - row( - "Re-review flip-gate wrong / dup comments", - baseline.rereview - ? `${baseline.rereview.flipGateWrongCases.length} / ${baseline.rereview.duplicateComments}` - : "n/a", - candidate.rereview - ? `${candidate.rereview.flipGateWrongCases.length} / ${candidate.rereview.duplicateComments}` - : "n/a", - ), - ] - : []), - row( - "Misses found-but-dropped", - String(dropClassByKey(baseline).size), - String(dropClassByKey(candidate).size), - ), - "", - ]; - - // A regression that was PRODUCED and then died at a gate is a different - // defect class (anchoring discipline, gate calibration) than a true miss - // (recall); annotate each lost spec so it routes to the right fix. - const candidateDrops = dropClassByKey(candidate); - if (report.regressions.lost.length > 0) { - lines.push( - "### Regressions (baseline caught, candidate missed)", - "", - ...report.regressions.lost.map((key) => { - const bucket = candidateDrops.get(key); - return bucket === undefined - ? `- ${key} (not found)` - : `- ${key} (found but dropped at the ${bucket} gate)`; - }), - "", - ); - } - if (report.regressions.gained.length > 0) { - lines.push( - "### Improvements (candidate caught, baseline missed)", - "", - ...report.regressions.gained.map((key) => `- ${key}`), - "", - ); - } - lines.push( - report.adversarialFailures.length === 0 - ? "Adversarial hard gate: PASSED on the candidate arm." - : [ - "### Adversarial hard gate: FAILED on the candidate arm", - "", - ...report.adversarialFailures.map((f) => `- ${f}`), - ].join("\n"), - "", - ); - if (report.gateRetries.length > 0) { - lines.push( - "### Gate flips retried (best of three, flipped cases only)", - "", - ...report.gateRetries.map((retry) => { - const passes = retry.attempts.filter((a) => a.pass).length; - const usd = retry.attempts.reduce((sum, a) => sum + a.usd, 0); - const outcome = retry.settledPass - ? "settled as a run-to-run flake; the gate does not fail on this case" - : "failure confirmed"; - return `- ${retry.caseId}: original run failed, ${passes}/${ - retry.attempts.length - } retries passed; ${outcome} ($${usd.toFixed(2)} retry spend)`; - }), - "", - ); - } - const skipped = [ - ...baseline.skippedCases.map((id) => `baseline:${id}`), - ...candidate.skippedCases.map((id) => `candidate:${id}`), - ]; - if (skipped.length > 0) { - lines.push( - "### SKIPPED (budget exhausted before dispatch)", - "", - ...skipped.map((s) => `- ${s}`), - "", - ); - } - const judgeErrors = [ - ...(baseline.judgeError !== undefined - ? [`baseline: ${baseline.judgeError}`] - : []), - ...(candidate.judgeError !== undefined - ? [`candidate: ${candidate.judgeError}`] - : []), - ]; - if (judgeErrors.length > 0) { - lines.push( - "### Judge scoring failed (metrics above still stand)", - "", - ...judgeErrors.map((e) => `- ${e}`), - "", - ); - } - const failedAgents = [...baseline.perCase, ...candidate.perCase].flatMap( - (c) => c.failedAgents.map((agent) => `${c.caseId}: ${agent} failed`), - ); - if (failedAgents.length > 0) { - lines.push( - "### Agent failures", - "", - ...failedAgents.map((f) => `- ${f}`), - "", - ); - } - lines.push(STABILITY_FOOTER, ""); - return lines.join("\n"); -}; - /* -------------------------------------------------------------------------- */ /* CLI */ /* -------------------------------------------------------------------------- */ @@ -694,50 +460,43 @@ const main = async (): Promise => { } const candidateMode = rawMode as ReReviewMode; + const repeats = Number(argValue("--repeats") ?? "1"); + if (!Number.isInteger(repeats) || repeats < 1) { + throw new Error("--repeats must be a positive integer"); + } + + // The fallback match arbiter (Haiku, capped, same-file, audited via + // `via: "fallback"`), on unless opted out: it only runs for specs the + // deterministic matcher left unmatched, and an arbiter failure degrades + // to a non-match. Both arms share the one matcher, so it never biases + // the A/B delta. + const match: MatchOptions | undefined = process.argv.includes( + "--no-match-arbiter", + ) + ? undefined + : { + fallback: haikuMatchArbiter({ + onError: (message) => console.error(message), + }), + }; + const runner = sdkRunner(); const armProduce = - (arm: ArmId, markdown: string, mode: ReReviewMode): ArmProduce => + (stage: string, markdown: string, mode: ReReviewMode): ArmProduce => (corpusCase) => produceLive(corpusCase, extractAgents(markdown), { runner, - stageDir: `${stageRoot}/${arm}/${corpusCase.id}`, + stageDir: `${stageRoot}/${stage}/${corpusCase.id}`, reReviewMode: mode, }); - // Sequential arms, halving the budget, so a runaway baseline cannot - // starve the candidate. - const baseline = await runArm( - "baseline", - cases, - armProduce("baseline", baselineMd, "full"), - {maxUsd: maxUsd / 2}, - ); - const candidate = await runArm( - "candidate", - cases, - armProduce("candidate", candidateMd, candidateMode), - {maxUsd: maxUsd / 2}, - ); - - // Retry the flip, not the run: a hard-gate flip re-runs only the flipped - // cases (fresh staging per attempt so re-materializing cannot collide), - // best of three, before the gate may fail the arm. Retried runs are - // recorded but never replace the original in the metrics. - const gateRetries = await retryGateFlips( - candidate, - cases, - (attempt): ArmProduce => - (corpusCase) => - produceLive(corpusCase, extractAgents(candidateMd), { - runner, - stageDir: `${stageRoot}/candidate-retry${attempt}/${corpusCase.id}`, - }), - ); - const flakes = new Set( - gateRetries.filter((r) => r.settledPass).map((r) => r.caseId), - ); - - if (withJudge) { + const judgeBothArms = async ( + baseline: ArmRunReport, + candidate: ArmRunReport, + ): Promise => { + if (!withJudge) { + return; + } // Judge scoring is additive: a failure here must degrade to a // report without quality scores, never kill a run whose arms have // already spent their budget (the plan's standing rule). @@ -753,25 +512,176 @@ const main = async (): Promise => { ); } } - } + }; - const report: AbReport = { - baseRef, - reviewMdSha: { - baseline: sha256(baselineMd), - candidate: sha256(candidateMd), - }, - arms: {baseline, candidate}, - regressions: diffRegressions(baseline, candidate), - adversarialFailures: adversarialGateFailures(candidate).filter( - (failure) => !flakes.has(failure.slice(0, failure.indexOf(":"))), - ), - gateRetries, + // Rolling budget: each arm-run's slice is the REMAINING budget divided + // by the arm-runs still to go. A runaway early arm still cannot starve + // what follows (its slice is capped), but a cheap early arm donates its + // headroom forward instead of stranding it; equal fixed slices caused + // budget-skip asymmetry on the first noise-floor run (38/36 of 42 + // case-runs), which inflates the very bands that run existed to measure. + const totalArmRuns = 2 * repeats; + let armRunsDone = 0; + let spentUsd = 0; + const nextArmBudget = (): number => + Math.max(0, maxUsd - spentUsd) / (totalArmRuns - armRunsDone); + const trackArm = (report: ArmRunReport): ArmRunReport => { + armRunsDone += 1; + spentUsd += report.usd; + return report; + }; + + // The ruler stamp: which matcher and which corpus produced this + // report's rates. Comparisons across runs are only valid when the stamp + // matches (see ReportProvenance in live-ab-report.ts). + const provenance = { + matcher: + match !== undefined ? "deterministic+arbiter" : "deterministic", + corpusSha: sha256(JSON.stringify(cases)), + caseCount: cases.length, + }; + + /** + * One full arm pair. `suffix` isolates staging across repeats; + * `withRetry` is the single-run best-of-three (a repeated run buys its + * flake evidence from the repeat structure instead). + */ + const runPair = async ( + suffix: string, + withRetry: boolean, + ): Promise => { + const baseline = trackArm( + await runArm( + "baseline", + cases, + armProduce(`baseline${suffix}`, baselineMd, "full"), + { + maxUsd: nextArmBudget(), + ...(match !== undefined ? {match} : {}), + }, + ), + ); + const candidate = trackArm( + await runArm( + "candidate", + cases, + armProduce(`candidate${suffix}`, candidateMd, candidateMode), + { + maxUsd: nextArmBudget(), + ...(match !== undefined ? {match} : {}), + }, + ), + ); + + // Retry the flip, not the run: a hard-gate flip re-runs only the + // flipped cases (fresh staging per attempt so re-materializing + // cannot collide), best of three, before the gate may fail the arm. + // Retried runs are recorded but never replace the original in the + // metrics. + const gateRetries = withRetry + ? await retryGateFlips( + candidate, + cases, + (attempt): ArmProduce => + (corpusCase) => + produceLive(corpusCase, extractAgents(candidateMd), { + runner, + stageDir: `${stageRoot}/candidate${suffix}-retry${attempt}/${corpusCase.id}`, + }), + match, + ) + : []; + const flakes = new Set( + gateRetries.filter((r) => r.settledPass).map((r) => r.caseId), + ); + + await judgeBothArms(baseline, candidate); + + return { + baseRef, + reviewMdSha: { + baseline: sha256(baselineMd), + candidate: sha256(candidateMd), + }, + provenance, + arms: {baseline, candidate}, + regressions: diffRegressions(baseline, candidate), + adversarialFailures: adversarialGateFailures(candidate).filter( + (failure) => + !flakes.has(failure.slice(0, failure.indexOf(":"))), + ), + gateRetries, + }; }; + let payload: AbReport | MultiAbReport; + let markdown: string; + let candidateRunCount: number; + let adversarialFailureCount: number; + + if (repeats === 1) { + const report = await runPair("", true); + payload = report; + markdown = renderMarkdownReport(report); + candidateRunCount = report.arms.candidate.runs.length; + adversarialFailureCount = report.adversarialFailures.length; + } else { + const reports: AbReport[] = []; + for (let repeat = 1; repeat <= repeats; repeat += 1) { + reports.push(await runPair(`-r${repeat}`, false)); + // Checkpoint after every repeat: a multi-repeat run carries tens + // of dollars of spend, and a crash or cancellation on repeat n + // must not forfeit repeats 1..n-1 (a run that dies with nothing + // emitted is the failure mode the plan forbids). The final write + // below replaces this with the full report. + mkdirSync(dirname(outPath), {recursive: true}); + writeFileSync( + outPath, + JSON.stringify( + { + repeatCount: repeats, + completedRepeats: reports.length, + repeats: reports, + }, + null, + 2, + ), + ); + } + // The repeat reports are already the artifact shape aggregate.ts + // pools, so the one-dispatch powered run and the N-dispatch drift + // pool go through the identical code path. + const aggregate = aggregateSamples( + reports.flatMap((report, i) => + extractSamples(`repeat-${i + 1}`, report), + ), + ); + const gate = majorityGateFailures( + reports.map((report) => report.arms.candidate), + ); + const multi: MultiAbReport = { + repeatCount: repeats, + repeats: reports, + aggregate, + gate, + adversarialFailures: gate + .filter((g) => g.confirmed) + .map( + (g) => + `${g.caseId}: failed ${g.failedRepeats}/${g.repeats} repeats`, + ), + }; + payload = multi; + markdown = renderMultiMarkdownReport(multi); + candidateRunCount = reports.reduce( + (sum, report) => sum + report.arms.candidate.runs.length, + 0, + ); + adversarialFailureCount = multi.adversarialFailures.length; + } + mkdirSync(dirname(outPath), {recursive: true}); - writeFileSync(outPath, JSON.stringify(report, null, 2)); - const markdown = renderMarkdownReport(report); + writeFileSync(outPath, JSON.stringify(payload, null, 2)); // A sibling .md rides along for CI's sticky PR comment. writeFileSync(outPath.replace(/\.json$/, ".md"), `${markdown}\n`); console.log(markdown); @@ -780,11 +690,11 @@ const main = async (): Promise => { writeFileSync(summaryPath, `${markdown}\n`, {flag: "a"}); } - if (candidate.runs.length === 0) { + if (candidateRunCount === 0) { console.error("no case was scored on the candidate arm"); process.exit(1); } - if (report.adversarialFailures.length > 0) { + if (adversarialFailureCount > 0) { console.error("adversarial hard gate FAILED on the candidate arm"); process.exit(1); } diff --git a/workflows/review/eval/live-match.test.ts b/workflows/review/eval/live-match.test.ts index c7ccbd30..99b9eb10 100644 --- a/workflows/review/eval/live-match.test.ts +++ b/workflows/review/eval/live-match.test.ts @@ -87,6 +87,51 @@ describe("matchesSpec", () => { ); }); + it("accepts an anchor at any altLocation, window and mechanism intact", () => { + // The defect spans files (migration + hot query): an anchor at the + // alternate site is a catch, not a miss. + const alt = spec({ + path: "src/other.ts", + lineStart: 1, + lineEnd: 3, + altLocations: [{path: "src/a.ts", lineStart: 1, lineEnd: 3}], + }); + expect(matchesSpec(candidate(), alt)).toBe(true); + // The alternate window still binds... + expect( + matchesSpec( + candidate(), + spec({ + path: "src/other.ts", + altLocations: [ + {path: "src/a.ts", lineStart: 10, lineEnd: 12}, + ], + }), + ), + ).toBe(false); + // ...and so does the mechanism, wherever the finding anchors. + expect( + matchesSpec(candidate(), {...alt, mechanism: ["sql injection"]}), + ).toBe(false); + // An altLocation without a window accepts any line on its file. + expect( + matchesSpec( + candidate({ + anchor: { + type: "line", + path: "src/a.ts", + line: 99, + side: "RIGHT", + }, + }), + spec({ + path: "src/other.ts", + altLocations: [{path: "src/a.ts"}], + }), + ), + ).toBe(true); + }); + it("treats a malformed regex alternate as a literal substring", () => { expect( matchesSpec(candidate(), spec({mechanism: ["float math and ("]})), diff --git a/workflows/review/eval/live-match.ts b/workflows/review/eval/live-match.ts index 3c104f67..dd160fde 100644 --- a/workflows/review/eval/live-match.ts +++ b/workflows/review/eval/live-match.ts @@ -21,8 +21,7 @@ * false-flag, noise, and verdict agreement. */ -import type {LiveDefectSpec} from "./corpus/loader"; -import type {CorpusCase} from "./corpus/loader"; +import type {CorpusCase, LiveDefectSpec} from "./corpus/loader"; import type {RunCandidate, RunResult} from "./runner"; /* -------------------------------------------------------------------------- */ @@ -90,7 +89,28 @@ export type MatchOptions = { const DEFAULT_MAX_FALLBACK_CALLS = 10; -/** Whether a candidate's anchor agrees with a spec's location. */ +/** + * Every location a spec accepts: the primary path/window plus any + * `altLocations` (a defect that spans files has more than one correct anchor + * site; see the type's doc). + */ +const specLocations = ( + spec: LiveDefectSpec, +): {path: string; lineStart?: number; lineEnd?: number}[] => [ + { + path: spec.path, + ...(spec.lineStart !== undefined + ? {lineStart: spec.lineStart, lineEnd: spec.lineEnd} + : {}), + }, + ...(spec.altLocations ?? []), +]; + +/** Whether a candidate shares a file with any of the spec's locations. */ +const onSpecFile = (candidate: RunCandidate, spec: LiveDefectSpec): boolean => + specLocations(spec).some((location) => location.path === candidate.path); + +/** Whether a candidate's anchor agrees with any of a spec's locations. */ const anchorAgrees = ( candidate: RunCandidate, spec: LiveDefectSpec, @@ -100,16 +120,22 @@ const anchorAgrees = ( // A PR-level comment names no location; mechanism alone decides. return true; } - if (anchor.path !== spec.path) { - return false; - } - if (anchor.type === "file" || spec.lineStart === undefined) { - return true; - } - const start = anchor.type === "line" ? anchor.start_line ?? anchor.line : 0; - const end = anchor.type === "line" ? anchor.line : 0; - // Overlap between the anchor's line range and the spec window. - return end >= spec.lineStart && start <= (spec.lineEnd ?? spec.lineStart); + return specLocations(spec).some((location) => { + if (anchor.path !== location.path) { + return false; + } + if (anchor.type === "file" || location.lineStart === undefined) { + return true; + } + const start = + anchor.type === "line" ? anchor.start_line ?? anchor.line : 0; + const end = anchor.type === "line" ? anchor.line : 0; + // Overlap between the anchor's line range and the location window. + return ( + end >= location.lineStart && + start <= (location.lineEnd ?? location.lineStart) + ); + }); }; /** Whether any mechanism alternate matches the finding's own description. */ @@ -177,9 +203,9 @@ export const matchCase = async ( if (options.fallback === undefined) { return undefined; } - // Fallback: only candidates sharing the spec's file, in posted order. + // Fallback: only candidates sharing a spec file, in posted order. for (const candidate of posted) { - if (claimed.has(candidate.id) || candidate.path !== spec.path) { + if (claimed.has(candidate.id) || !onSpecFile(candidate, spec)) { continue; } if (fallbackCalls >= maxFallbackCalls) { @@ -224,7 +250,7 @@ export const matchCase = async ( for (const [bucket, candidates] of droppedBuckets) { const hit = candidates.find( (candidate) => - candidate.path === spec.path && + onSpecFile(candidate, spec) && mechanismAgrees(candidate, spec), ); if (hit !== undefined) { diff --git a/workflows/review/eval/match-arbiter.test.ts b/workflows/review/eval/match-arbiter.test.ts new file mode 100644 index 00000000..f8fcdcee --- /dev/null +++ b/workflows/review/eval/match-arbiter.test.ts @@ -0,0 +1,92 @@ +import {describe, it, expect} from "vitest"; + +import { + buildArbiterPrompt, + parseArbiterAnswer, + PINNED_ARBITER_MODEL, +} from "./match-arbiter"; +import type {LiveDefectSpec} from "./corpus/loader"; +import type {RunCandidate} from "./runner"; + +const candidate: RunCandidate = { + id: "cand-1", + source: "correctness", + lens: "correctness", + label: "issue (blocking)", + blocking: true, + anchor: {type: "line", path: "src/a.ts", line: 15, side: "RIGHT"}, + path: "src/a.ts", + line: 15, + body: "**issue (blocking):** no index", + finding: { + schema_version: 2, + id: "cand-1", + lens: "correctness", + anchor: {type: "line", path: "src/a.ts", line: 15, side: "RIGHT"}, + severity: "blocking", + confidence: 0.8, + evidence_trace: ["e"], + failure_scenario: "the hot query scans the whole table.", + producing_hunt: "h", + model_authored_prose: "Add an index or this will table-scan.", + }, +}; + +const spec: LiveDefectSpec = { + key: "dm-1", + path: "db/migration.sql", + lineStart: 1, + lineEnd: 5, + mechanism: ["index", "table scan"], + lens: "data-migrations", +}; + +describe("buildArbiterPrompt", () => { + it("carries the spec location, mechanism, and the finding's own words", () => { + const prompt = buildArbiterPrompt(candidate, spec); + expect(prompt).toContain("db/migration.sql (lines 1-5)"); + expect(prompt).toContain("lens data-migrations"); + expect(prompt).toContain("index | table scan"); + expect(prompt).toContain("anchored at src/a.ts:15"); + expect(prompt).toContain("the hot query scans the whole table."); + expect(prompt).toContain("Add an index or this will table-scan."); + // The conservative bias is part of the contract: a false yes + // inflates recall, the load-bearing metric. + expect(prompt).toContain("If uncertain, answer false"); + }); + + it("renders windowless specs and PR-level anchors without artifacts", () => { + const prompt = buildArbiterPrompt( + { + ...candidate, + anchor: {type: "pr"}, + path: undefined, + line: undefined, + }, + {key: "k", path: "src/a.ts", mechanism: ["m"]}, + ); + expect(prompt).toContain("in file src/a.ts."); + expect(prompt).not.toContain("lines"); + expect(prompt).toContain("anchored at the PR"); + }); +}); + +describe("parseArbiterAnswer", () => { + it("accepts only an explicit true, everything else is a no", () => { + expect(parseArbiterAnswer('{"match": true}')).toBe(true); + expect(parseArbiterAnswer('Sure: {"match": true}; same defect.')).toBe( + true, + ); + expect(parseArbiterAnswer('{"match": false}')).toBe(false); + expect(parseArbiterAnswer('{"match": "true"}')).toBe(false); + expect(parseArbiterAnswer("yes")).toBe(false); + expect(parseArbiterAnswer("")).toBe(false); + expect(parseArbiterAnswer("{broken json")).toBe(false); + }); +}); + +describe("PINNED_ARBITER_MODEL", () => { + it("stays on the pinned Haiku snapshot (operator direction)", () => { + expect(PINNED_ARBITER_MODEL).toBe("claude-haiku-4-5-20251001"); + }); +}); diff --git a/workflows/review/eval/match-arbiter.ts b/workflows/review/eval/match-arbiter.ts new file mode 100644 index 00000000..d9b6c6c4 --- /dev/null +++ b/workflows/review/eval/match-arbiter.ts @@ -0,0 +1,154 @@ +/** + * The live fallback match arbiter (the tuning memo's rev-2 item 5): the + * implementation behind live-match.ts's injected {@link MatchFallback} seam. + * The deterministic matcher requires a mechanism alternate to hit the + * finding's own wording; real model runs phrase the same defect a hundred + * ways, and the "unmatched posted" noise metric reads 54-69% everywhere, + * implausibly high as true noise. When a spec stays unmatched and a posted + * candidate shares its file, this arbiter asks a pinned Haiku one yes/no + * question: does this finding describe THIS labeled defect? + * + * Guard rails, all inherited from the seam: called only for specs the + * deterministic pass left unmatched, only against candidates on a spec file, + * hard-capped per case, and every match it claims is recorded `via: + * "fallback"` in the report so a human can audit it. The prompt biases to NO + * (a false yes inflates recall, the load-bearing metric; a false no just + * leaves the report as conservative as it is today), and any API failure + * degrades to a non-match, never a dead run. + */ + +import type {LiveDefectSpec} from "./corpus/loader"; +import type {MatchFallback} from "./live-match"; +import type {RunCandidate} from "./runner"; + +/** + * Pinned snapshot, deliberately at the Haiku tier: the question is a narrow + * semantic-equivalence check, priced to run up to 10x per case on every arm. + */ +export const PINNED_ARBITER_MODEL = "claude-haiku-4-5-20251001"; + +const API_URL = "https://api.anthropic.com/v1/messages"; +const MAX_ATTEMPTS = 3; +const BACKOFF_MS = [1_000, 4_000]; + +const isTransientStatus = (status: number): boolean => + status === 429 || status === 408 || status >= 500; + +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +/** The one question, deterministic in its inputs (unit-testable). */ +export const buildArbiterPrompt = ( + candidate: RunCandidate, + spec: LiveDefectSpec, +): string => + [ + "You are auditing one code-review comment against one labeled defect spec.", + 'Answer ONLY a JSON object: {"match": true|false}.', + "", + `Labeled defect: in file ${spec.path}` + + (spec.lineStart !== undefined + ? ` (lines ${spec.lineStart}-${spec.lineEnd ?? spec.lineStart})` + : "") + + (spec.lens !== undefined ? `, lens ${spec.lens}` : "") + + ".", + `Causal mechanism (any of): ${spec.mechanism.join(" | ")}`, + "", + `Review comment (anchored at ${candidate.path ?? "the PR"}${ + candidate.line !== undefined && candidate.line !== null + ? `:${candidate.line}` + : "" + }):`, + `Failure scenario: ${candidate.finding.failure_scenario}`, + `Comment prose: ${candidate.finding.model_authored_prose}`, + "", + "match is true ONLY when the comment describes this same defect" + + " (the same root cause and failure mode the mechanism names)," + + " not merely a nearby, related, or different issue in the same" + + " file. If uncertain, answer false.", + ].join("\n"); + +/** Parse the arbiter's reply; anything but an explicit true is a no. */ +export const parseArbiterAnswer = (text: string): boolean => { + const match = text.match(/\{[\s\S]*\}/); + if (!match) { + return false; + } + try { + const parsed = JSON.parse(match[0]) as {match?: unknown}; + return parsed.match === true; + } catch { + return false; + } +}; + +/** + * A {@link MatchFallback} scored over the Anthropic Messages API with the + * pinned arbiter model. Transient failures retry with backoff; a call that + * still fails resolves false (deterministic-only behavior for that spec) and + * reports through `onError` rather than throwing; a matching pass must + * never kill a run whose arms have already spent their budget. + */ +export const haikuMatchArbiter = (options?: { + onError?: (message: string) => void; +}): MatchFallback => { + const onError = options?.onError ?? (() => {}); + return async (candidate, spec) => { + const prompt = buildArbiterPrompt(candidate, spec); + try { + // Typed via the fetch signature: the repo's eslint no-undef does + // not know the fetch-API globals in type position. + let response: Awaited> | undefined; + for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt += 1) { + try { + response = await fetch(API_URL, { + method: "POST", + headers: { + "x-api-key": process.env["ANTHROPIC_API_KEY"] ?? "", + "anthropic-version": "2023-06-01", + "content-type": "application/json", + }, + body: JSON.stringify({ + model: PINNED_ARBITER_MODEL, + max_tokens: 64, + messages: [{role: "user", content: prompt}], + }), + }); + } catch (error) { + // Network-level failure: transient by definition. + if (attempt === MAX_ATTEMPTS) { + throw new Error(String(error)); + } + await sleep(BACKOFF_MS[attempt - 1] ?? 4_000); + continue; + } + if (response.ok) { + break; + } + const status = response.status; + const body = await response.text(); + if (!isTransientStatus(status) || attempt === MAX_ATTEMPTS) { + throw new Error(`${status} ${body}`); + } + response = undefined; + await sleep(BACKOFF_MS[attempt - 1] ?? 4_000); + } + if (response === undefined) { + throw new Error("no response"); + } + const data = (await response.json()) as { + content: {type: string; text?: string}[]; + }; + const text = + data.content.find((block) => block.type === "text")?.text ?? ""; + return parseArbiterAnswer(text); + } catch (error) { + onError( + `match arbiter failed for spec ${spec.key} vs ${ + candidate.id + }: ${String(error instanceof Error ? error.message : error)}`, + ); + return false; + } + }; +};