review: exact --cases selection, fail-fast on unknown ids - #257
Merged
Conversation
…live cases Phase 1 of the live A/B eval plan (#232). The corpus gains an opt-in live block carrying what a real model run needs and the deterministic replay does not: - prContext (PR title/description/author/base; the description is untrusted author text, so an adversarial case can carry its payload there or in the diff), - a post-change file tree on disk next to the case, via a new <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json (a directory containing case.json is one case; its tree is never parsed as corpus JSON), and - labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window + mechanism rather than id. The loader enforces the invariants: the live tag and the live block imply each other, a live case needs a cleanly-parseable diff, spec paths must appear in changedFiles and the diff, and every non-removed changed file must exist in the tree. loadLiveCorpus() returns the subset. The live half lives in corpus/live.ts; loader.ts re-exports it as the single public surface. Ten cases are converted with hand-authored real diffs and trees: five smoke incidents, both clean cases, one adversarial injection whose payload is a code comment in the diff, one golden holdout, and one synthetic mutation. Their recorded line anchors are rewritten to the authored defect lines (natural files beat content padded to synthetic line numbers), which activates the provenance gate on these cases in the deterministic suite; all expectations hold unchanged.
…action and case staging Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1 corpus format (#233). agent-extract.ts turns review.md's '## agent:' sections into data (name, description, pinned model, prompt body). It takes the markdown as a string with no fs access, because the baseline arm of an A/B reads the merge-base review.md via git show. Parsing is strict; a malformed or model-less section throws listing every problem, since a silently dropped agent would skew an eval arm without failing it. An integration test extracts the real review.md (21 agents) and pins the staging-root reference so a future rename fails a test instead of silently staging nothing. live-stage.ts materializes the production staging layout for one live-enabled corpus case: pr-context.json (review.md Step 1's shape, synthetic identity fields), full.diff / pr.diff / full-stripped.diff (all the case diff; corpus diffs carry no generated files and no pattern-triage pass runs), files.json + review-files.json with the hasPatch cross-check derived from the diff parse, provenance.json, routing.json from the deterministic router, an out/ directory, and the post-change checkout copied from the case tree. rewriteAgentPrompt swaps the production staging root for the staged context dir. Everything sits behind an injected-fs seam and is memfs-tested. Model dispatch (phase 2c) is deliberately absent; it needs the Agent SDK dependency decision and arrives separately.
… runner (phase 2c)
live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.
live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.
live-match.ts scores a live run against a case's labeled defect specs: a posted candidate satisfies a spec when its anchor agrees with the spec's path (and line window when both carry one) and any mechanism alternate matches the finding's failure_scenario or prose; each candidate satisfies at most one spec and vice versa. An injected fallback arbiter (hard-capped, same-file only, recorded as via: fallback for audit) can rescue recall on vague prose; false flags are decided by the deterministic rule alone. computeLiveMetrics aggregates recall, verdict agreement, clean false-flag (including a clean case that blocks), and noise. live-ab.ts is the arm orchestrator and CLI: baseline review.md from git show <merge-base>, candidate from the working tree, both arms over the same live corpus with everything else (corpus, lib, runner, metrics, judge) from the candidate, per the plan's settled decision to isolate the model seam. Each arm runs under half the --max-usd budget with sticky exhaustion: once spend plus the running per-case average crosses the cap, remaining cases are recorded skipped and the report still emits. Spec-level regressions are diffed only over cases both arms scored. Judge scoring reuses the pinned judge (quality aggregates only; judge-vs-ground-truth disagreement keys on recorded ids a live arm does not use); the fetch model moves to judge-live-model.ts and live-judge.ts now imports it. runner.ts gains an optional RunOptions.validation override so a live validator's output replaces the recorded block. Report-only except the standing rule: adversarial-injection failures on the candidate arm exit non-zero.
Review Eval A/B runs on every non-draft PR touching workflows/review/** (and on workflow_dispatch): both review.md arms over the live corpus via live-ab.ts, with the delta report posted as a sticky PR comment (hidden-marker upsert), appended to the job summary, and uploaded as an artifact even on partial or gate-failing runs. Per-PR scope is the smoke-tagged live subset; the full-eval label or dispatch input lifts it, skip-live-eval opts out, drafts wait until ready, the changeset-release branch is excluded (it matches the path filter via package.json but changes no behavior), secretless runs skip green so fork PRs never fail, and per-PR concurrency cancels superseded runs. The workflow name is distinct from every gh-aw workflow per the operational-floor rule about shared concurrency groups. live-ab.ts gains --smoke-only and writes out/live-ab-report.md alongside the JSON for the comment step.
… and vitest Tree directories are byte-exact case fixtures paired with each case's diff: prettier auto-formatting one would silently desync it from the diff the provenance gate parses, and a tree may carry a *.test.ts whose tests fail by design (test-adequacy cases), so vitest must not execute them either. CI's lint job caught the first drift (prettier wanting to rewrap a fixture ternary).
…rpus' into jwbron/live-eval-producer-staging
…staging' into jwbron/live-eval-ab-runner
…to jwbron/live-eval-ab-ci
…s with the case id Live agents choose their own finding ids, so every case's first correctness finding was live-correctness-reviewer-1; ids were unique within a case but collided across cases, and judge.ts's score join requires arm-global uniqueness. Caught by the first real A/B run (both arms completed, then judge aggregation threw). Ids are now <caseId>:<id> from the moment of parsing, so claims.json, the validator round-trip, the matcher, and the judge all see the same namespaced id.
…staging' into jwbron/live-eval-ab-runner
…eport instead of killing it The first real A/B run spent both arms' budgets and then died in judge aggregation, writing no report: the exact everything-spent-nothing-posted failure mode the plan forbids. Judge scoring is additive, so a per-arm failure is now caught, recorded as judgeError on the arm, rendered as a degradation note in the report, and the run proceeds to write JSON + markdown and evaluate the adversarial gate as usual.
…to jwbron/live-eval-ab-ci
…A/B phase 5) Packages the seeded-defect live-trial pattern (Khan/webapp#40678) as a Claude Code skill so a trial costs an operator an afternoon instead of a week of hand choreography. The skill collects required inputs (seeded branch, human-authored defect table, arms, budget approval) and refuses to improvise ground truth; sets up one isolated PR per arm with per-arm trigger recipes and the distinct-workflow-name rule (same-named gh-aw workflows share a per-PR concurrency group and cancel each other); collects reviews, artifacts, and costs per run with the known gh-aw artifact-bug tolerance; drives optional lifecycle pushes; scores defect by defect with the deterministic rule mirroring eval/live-match.ts plus audited manual judgment; exports live-enabled corpus case skeletons with a sanitization gate for private-repo content landing in this public repo; and cleans up trial PRs, branches, and temporary workflows. Trials remain the architecture-bet instrument; per-change evals belong to the corpus A/B. .gitignore narrows from .claude to .claude/* with a !.claude/skills/ carve-out: local agent state (settings, worktrees) stays untracked, project skills are shared tooling and are committed.
…orpus' into tmp-refresh
…roducer-staging' into tmp-refresh
…b-runner' into tmp-refresh
…b-ci' into tmp-refresh
…ity, code-rendered from the reconciler keep-list
… instead of dropping them
…oad (allowed-paths must match staging-relative paths)
…untability' into jwbron/review-out-of-lane
…into one staged disciplines file
…ive case The first live acceptance runs missed incident-sql-missing-index:dm-missing-index-1 in all four arms: the recorded finding is a data-migrations LENS finding, but live cases carried no routerConfig lens rules, so the live roster never spawned the specialist that catches it. Every live case whose recorded finding belongs to a specialist lens now routes that lens on the finding's file (seven cases across five lenses), mirroring how a consumer ROUTING file would route the same paths in production.
…rpus' into jwbron/live-eval-producer-staging
…staging' into jwbron/live-eval-ab-runner
… the A/B report The acceptance runs surfaced a claim-validator failure that the report could only name, not explain (perCase carried agent names only, and the PerAgentReport.failed detail never reached the markdown). perCase failedAgents entries are now '<agent>: <reason>', so the next failure is diagnosable from the sticky comment alone.
…' into jwbron/rereview-live-corpus
…powered runs, drift watch, sql-index case fix The eval instrument's percentage deltas sit below the one-case noise floor; this lands the memo's measurement items. eval/aggregate.ts pools N report artifacts into per-case pass rates with Wilson intervals and pooled rows (reproduces the 07-10 cumulative numbers exactly); live-ab.ts --repeats runs one powered dispatch with a strict-majority adversarial gate; the weekly review-eval-drift.yml workflow runs full corpus x3 on main and publishes the aggregate; identical-arm pools render noise-floor bands as data. incident-sql-missing-index diagnosed against all 28 recorded arm-runs: the reviewer found the missing index every single time; the 8/16 catch rate was the spec accepting only the migration-file anchor while the reviewer anchored the same blocking finding at the hot query (11 runs) or had it provenance- dropped on a mis-anchor (2 runs). Specs gain altLocations; the case accepts the query-site anchor and its residual misses now classify found-but-dropped (the anchor-snap defect class), not recall.
…rbiter on the pinned Haiku snapshot Fills the MatchFallback seam in live-match.ts: when a spec stays unmatched after the deterministic pass and a posted candidate shares its file, a pinned claude-haiku-4-5-20251001 answers one yes/no question (same defect, same root cause?), biased to no since a false yes inflates recall. All seam guard rails inherited: capped per case, same-file only, matches recorded via 'fallback' for audit. API failures degrade to non-matches through onError; both arms share the matcher so the A/B delta stays unbiased. On by default in live-ab.ts; --no-match-arbiter restores deterministic-only matching. This targets the 54-69% 'unmatched posted' readings, implausibly high as true noise.
…nderers into live-ab-report.ts CI's max-lines (1000) caught live-ab.ts at 1011 after the repeats and arbiter work; this worktree cannot run eslint locally (dot-dir ignore). The report types (ArmRunReport, AbReport, MultiAbReport, gate types) and both markdown renderers move to a leaf module with no runner dependency; live-ab.ts re-exports them so the import surface is unchanged.
…t after every repeat A multi-repeat run carries tens of dollars of spend but wrote its artifact only at the end, so a crash or cancellation on repeat n forfeited repeats 1..n-1 (the exact dies-with-nothing-emitted failure mode the plan forbids; the workflow's always() artifact upload had nothing to grab). Each completed repeat now overwrites the out path with the accumulated partial payload, which aggregate.ts already pools via its repeats-field handling.
…or in the report footer Bought per the memo's item 6: run 29069228968 (--force-arms --repeats 3, full 14-case corpus, $58.71 under the $60 clamp) gives 6 arm-samples of one review.md. Measured bands: must-catch recall 54-86%, verdict agreement 75-100%, noise 50-60%, judge quality 0.82-0.86. Every single-run report now renders these as data with provenance, replacing prose guesswork; a delta whose arms both sit inside a band is wobble, and the footer points at --repeats for smaller effects. Pre-arbiter measurement; the weekly drift run re-measures with the arbiter active.
…budget to $85 The noise-floor dispatch showed the live corpus is now 14 cases (the #251 golden re-review set), and the $60 default trimmed case-runs on both arms (38 and 36 of 42; $58.71 spent at the clamp). $85 covers 6 full arm-runs at the measured ~$10/arm-run with landing headroom, so the weekly aggregate never carries budget-skip asymmetry.
…, and noise floor in the changeset
…port opens a visibility PR workflows/review/eval/README.md is the guide an agent or engineer needs to run the eval system cold: the three tiers, every live-ab flag and CI entry point, powered-run recipes with real costs, corpus-growth rules (target the 20-80% band), report-reading guidance anchored on the measured noise floor, model pins, and the harness's historical limits (nothing before the 2026-07-08 structured-agent architecture runs under it). The weekly drift run now also opens a PR committing the report markdown and compact aggregate under .github/review-eval/drift/ (changeset-gate-excluded, outside the A/B path filter): a job summary nobody opens is not a drift watch, and merged report PRs accumulate an in-repo time series.
…rom the main README
…-floor statistics, rolling repeat budgets Findings from an adversarial pass over the instrument's own claims: Reports now stamp their ruler (matcher configuration + a content hash of the loaded corpus); the aggregate prints it, warns when a pool mixes rulers, and the drift series stays interpretable across instrument upgrades like the arbiter default or corpus growth, which move every rate without the reviewer changing. Noise-floor bands gain SD (min/max are extreme-value statistics that only widen as samples accumulate; mean +/- sd is the stable band) and a loud case-asymmetry warning: the 2026-07-10 measurement had budget skips, so its published v1 bands fold case-mix variance in on top of wobble; the footer constants and provenance string now say so. Repeat budgets roll: each arm-run's slice is the remaining budget over the remaining arm-runs, so cheap early arms donate headroom forward instead of stranding it (fixed slices caused the 38/36-of-42 skip asymmetry that contaminated the noise-floor run). README gains a statistical-honesty section: clustered-interval optimism, the repeats-gate relaxation, and the arbiter's uncalibrated refuse bias.
…hange-provenance gate The reviewer produces right-file, right-mechanism findings at wrong line numbers (it appears to sometimes count unified-diff text lines instead of file lines: observed anchors at line 24 of an 18-line file and line 8 of a 3-line file), and the provenance gate then drops them; since only surviving blocking labels feed the computed verdict, a correct blocking finding dying at the gate can flip REQUEST_CHANGES to APPROVE. Observed on adversarial-injection-approve, golden-request-changes-authz, the sql-index replay, and production main's own baseline arm. Before setting a line-anchored finding aside, the gate now snaps it to the nearest changed line in the same file under two windows: a 3-line near-miss window (the unified diff's context width) and an overflow window for anchors past every shown line by no more than the file's diff-text overhead (exactly the counting mis-anchor's overshoot bound). Ties break toward the lower line. Snapped findings keep their severity, post at the snapped anchor, and are recorded for audit (out/snapped.json in production, snappedByProvenance in eval results); findings outside both windows keep today's set-aside behavior. provenance.json carries a precomputed per-file snap lookup so review.md's gate stays a dictionary lookup. The live A/B emulates each arm's own review.md gate version, keyed on the anchor-snap marker, so the baseline arm replays the pre-snap gate and the powered run prices this change.
…istic smoke case provenance-anchor-snap-rescued replays the observed production anatomy in the per-push CI gate: a correct blocking finding anchored at the diff-text line (past the end of a short file, at the exact overflow edge) snaps to the nearest changed line, posts there, and drives REQUEST_CHANGES, while a far-anchored pre-existing observation in the same run is still set aside unposted. Deterministic and smoke-tagged only, so the live ruler (corpus stamp over live-tagged cases) is untouched and the in-flight powered run stays comparable. Also documents how to audit snap records (from/to distance separates the near-miss and overflow classes).
🦋 Changeset detectedLatest commit: e0b347d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
khan-actions-bot
requested review from
a team,
jaredly and
somewhatabstract
and removed request for
a team
July 10, 2026 20:44
…st on unknown ids The smoke scope filtered the corpus before the case filter, so a powered dispatch naming a non-smoke case silently dropped it: the anchor-snap pricing run named two cases and the paid report covered one without saying so. --cases is now an exact selection (bypasses --smoke-only, preserves order, dedupes) and any id matching no live case throws before a single model call. The workflow needs no change; it already passes both flags and the case list now wins.
jwbron
force-pushed
the
jwbron/eval-case-selection
branch
from
July 10, 2026 20:44
a910f21 to
820c735
Compare
jeresig
approved these changes
Jul 13, 2026
…n' into jwbron/eval-case-selection # Conflicts: # .changeset/review-anchor-snap.md # .github/workflows/review-eval-ab.yml # workflows/review/eval/README.md # workflows/review/eval/aggregate.test.ts # workflows/review/eval/corpus/live.ts # workflows/review/eval/corpus/loader.test.ts # workflows/review/eval/corpus/loader.ts # workflows/review/eval/corpus/smoke/provenance-anchor-snap-rescued.json # workflows/review/eval/live-ab.test.ts # workflows/review/eval/live-ab.ts # workflows/review/eval/live-match.ts # workflows/review/eval/live-stage.ts # workflows/review/lib/disciplines.test.ts # workflows/review/lib/provenance.test.ts # workflows/review/lib/provenance.ts # workflows/review/lib/rereview-mode.ts # workflows/review/review.md
Contributor
Review Guidancegithub-actions (1 file)
Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note: holistic review not assessed this run (shed for run budget on this low-risk change).
Note: first-principles review not assessed this run (shed for run budget on this low-risk change).
Note: conventions review not assessed this run (shed for run budget on this low-risk change).
Note: claim validation not assessed this run (the single non-blocking candidate was posted unvalidated under run budget).
| options: {smokeOnly: boolean; caseFilter?: string[]}, | ||
| ): CorpusCase[] => { | ||
| const {caseFilter} = options; | ||
| if (caseFilter !== undefined) { |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): The empty-caseFilter ([]) branch is untested. It is distinct from undefined — it takes this if branch, bypasses the smoke scope, and returns [] — and is reachable in practice, since main()'s new --cases trim/filter turns --cases "" or --cases ,, into []. A future refactor that treated [] as "no filter" and fell back to the full corpus would defeat the fail-fast intent, and no current test would catch it. Consider adding to the selectCases block:
it("an empty case list selects nothing, even under a smoke scope", () => {
expect(selectCases(corpus(), {smokeOnly: true, caseFilter: []})).toEqual([]);
});
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instrument fix, stacked on #255 (parallel to #256), from a footgun the anchor-snap eval gate hit live.
What happened: the #255 powered dispatch (
cases=adversarial-injection-approve,golden-request-changes-authz repeats=10) followed the memo's recipe, which omitsfull=true. The workflow applies--smoke-onlybefore--cases, so the non-smokegolden-request-changes-authzwas silently filtered out of a paid measurement: the report covered one case with nothing anywhere saying the other was requested and dropped. A typo'd case id would shrink a measurement the same way. Re-measuring cost a second dispatch.The fix: an explicit
--caseslist is an exact selection.--smoke-only, which now scopes unscoped runs only (naming a case means selecting it; a tag filter should not second-guess an explicit list).Selection is extracted to a pure
selectCaseswith unit tests (smoke scope on/off, exact selection past the scope, order/dedup, unknown-id failure). The workflow yml needs no change: it already passes both flags, and the case list now wins. The memo's powered-run recipe works as written after this.