review: live-enabled corpus format and ten live cases#233
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.
🦋 Changeset detectedLatest commit: 56bc3b4 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 |
jwbron
added a commit
that referenced
this pull request
Jul 9, 2026
…-runner skill phase Phase 1 status (shipped in #233 minus the trial-case port), and a new Phase 5: a Claude Code skill packaging the Khan/webapp#40678 seeded live-PR trial choreography (isolated arm PRs with distinct workflow names, run collection, defect-table scoring, corpus-case export, cleanup). Seeds and ground truth stay human-authored.
This was referenced Jul 9, 2026
… 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).
This was referenced Jul 9, 2026
…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.
…/live-eval-corpus' into jwbron/live-eval-corpus
jwbron
added a commit
that referenced
this pull request
Jul 9, 2026
…record the judge-direction lesson All three follow-ups shipped: agent-failure reasons and the per-row stability footer on #236, lens routing for live cases on #233/#235. Records the acceptance pair's most instructive number: judge quality rose on the deliberately regressed arm (fewer, surer comments score better), so recall against labeled specs is the load-bearing metric.
khan-actions-bot
requested review from
a team,
jaredly and
jeresig
and removed request for
a team
jeresig
approved these changes
Jul 10, 2026
jeresig
left a comment
jeresig
left a comment
Member
There was a problem hiding this comment.
Great - these test cases make a lot of sense to me!
Comment on lines
+7
to
+8
| from app.auth.decorators import require_admin | ||
| from app.models.users import delete_user_content, lookup_user |
Member
There was a problem hiding this comment.
Will the agent try to read these files for more context and then fail?
Contributor
Author
There was a problem hiding this comment.
It will often try: the sub-agent's cwd is the staged copy of this tree, so reading a missing import returns an ordinary not-found tool error the agent works around (no network, no crash). The cost of a missing file is realism rather than failure, so case authors include the context files whose absence would change the reviewer's conclusion; in this case the load-bearing evidence is the decorated sibling handler in the same file. The smoke cases that ran through acceptance and the re-trigger wave all have one- or two-file trees like this one. Documented on the tree field now: 56bc3b4.
Contributor
Review Guidancegithub-actions (4 files)
Common patterns20 files: Flat 12 files: New Excluded from review (23 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
pr-reviewer v1.4.1 · finding-schema v2 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "severity": "blocking", | ||
| "confidence": 0.9, | ||
| "evidence_trace": [ | ||
| "src/api/admin_routes.py:18 defines POST /admin/reset with no @requires_admin decorator", |
Contributor
There was a problem hiding this comment.
note (non-blocking): These converted cases re-anchor each recorded finding to the new tree/ but leave its evidence_trace/failure_scenario/prose citing the old lines and identifiers — 7 of 8 defect cases (only incident-sql-missing-index is consistent). e.g. here the trace still references /admin/reset + @requires_admin at the old line, but the tree has purge with require_admin at a different line. The loader validates only tree-file presence, not finding prose, so CI passes — but each recorded finding now contradicts the tree it's paired with. Worth reconciling before these become the A/B reference cases.
| if (diffPaths !== undefined && !diffPaths.has(path)) { | ||
| errors.push(`${at}.path: "${path}" has no section in the diff`); | ||
| } | ||
| const mechanism = entry["mechanism"]; |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): mechanism is a required spec field but its rejection branch (lines 132-142) has no test — the sibling checks (key, path, line windows) are all covered. Consider adding:
it("rejects a spec with a missing or empty mechanism", () => {
const withMechanism = (mechanism: unknown) =>
liveCase({live: {prContext: {title: "t", description: "d", author: "a", baseBranch: "main"},
mustCatchSpecs: [{key: "k", path: "src/a.ts", mechanism}]}});
expect(parseErrors(withMechanism([]))).toMatch(/non-empty array of non-empty strings/);
expect(parseErrors(withMechanism([""]))).toMatch(/non-empty array of non-empty strings/);
expect(parseErrors(withMechanism("m"))).toMatch(/non-empty array of non-empty strings/);
});| "postedCommentCount": 0 | ||
| }, | ||
| "diff": "diff --git a/src/util/format.ts b/src/util/format.ts\n--- a/src/util/format.ts\n+++ b/src/util/format.ts\n@@ -1,7 +1,7 @@\n /** Shared display formatting helpers. */\n \n /** Format a fractional amount as a fixed two-decimal string. */\n-export const fmt = (amount: number): string => amount.toFixed(2);\n+export const formatAmount = (amount: number): string => amount.toFixed(2);\n \n /** Format a byte count using binary units. */\n export const formatBytes = (bytes: number): string => {\ndiff --git a/src/util/format.test.ts b/src/util/format.test.ts\n--- a/src/util/format.test.ts\n+++ b/src/util/format.test.ts\n@@ -1,11 +1,11 @@\n import {describe, expect, it} from \"vitest\";\n \n-import {fmt, formatBytes} from \"./format\";\n+import {formatAmount, formatBytes} from \"./format\";\n \n-describe(\"fmt\", () => {\n+describe(\"formatAmount\", () => {\n it(\"renders two decimals\", () => {\n- expect(fmt(3)).toBe(\"3.00\");\n- expect(fmt(3.456)).toBe(\"3.46\");\n+ expect(formatAmount(3)).toBe(\"3.00\");\n+ expect(formatAmount(3.456)).toBe(\"3.46\");\n });\n });\n \n", | ||
| "live": { |
Contributor
There was a problem hiding this comment.
note (non-blocking): mustNotFlagSpecs is a headline deliverable and is fully implemented/tested, but no converted case populates it — including the two clean cases it's designed for. A future live producer measuring precision on the clean cases has no labeled traps to score against.
| // Live eval-corpus trees are byte-exact fixtures paired with each | ||
| // case's diff: linting (and especially prettier auto-formatting) | ||
| // would desync them from the diff the provenance gate parses. | ||
| "workflows/review/eval/corpus/**/tree/", |
Contributor
There was a problem hiding this comment.
note (non-blocking): live.tree is a per-case field defaulting to tree, but this ignore (and the vitest exclude) hard-code the literal tree/ segment. They agree only because all ten cases use the default; a case with a non-default live.tree would escape both globs, so prettier would reformat the fixture and vitest would execute its by-design-failing test — the exact desync these excludes exist to prevent. Consider rejecting a non-default tree name in parseLive until the globs are generalized.
| live: CaseLive, | ||
| changedFiles: ChangedFile[], | ||
| sourcePath: string, | ||
| existsSync: (p: string) => boolean, |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): liveTreeErrors checks only that each changed file exists in the tree, not that the tree content matches the diff post-image — the desync these paired fixtures are most prone to (the diff and tree are hand-authored separately). Since parseLive already calls computeDiffProvenance, it could assert each hunk's added lines appear at their RIGHT-side offsets in the tree file, turning silent drift into a load-time authoring error.
| * (keyword/regex alternates a matcher tests against a produced finding's | ||
| * `failure_scenario` and prose). See `live-ab-plan.md` Phase 3. | ||
| */ | ||
| export type LiveDefectSpec = { |
Contributor
There was a problem hiding this comment.
thought (non-blocking): In all ten cases LiveDefectSpec.path restates the recorded finding's anchor path and the window brackets its anchor line — only mechanism is genuinely new — yet the key↔finding-id link is unenforced (the doc comment itself calls it "conventional"). A smaller shape (mechanism alternates attached to the existing finding, path/window defaulted from the anchor) would remove the parallel ground truth and make the key correspondence definitional rather than conventional.
| "live" | ||
| ], | ||
| "category": "golden", | ||
| "description": "Golden HOLDOUT case from a merged webapp PR that was later reverted: a human flagged a missing authorization check on a new admin endpoint as blocking, and the PR was changed. Ground truth = that blocking comment.", |
Contributor
There was a problem hiding this comment.
question (non-blocking): This case self-describes as "from a merged webapp PR" but the PR description says all ten diffs/trees are hand-authored, and the tree is a generic synthetic Flask blueprint. If golden cases derive their weight from being real reviewed changes, should the reconstructed live half be tagged distinctly (e.g. "reconstructed") so live A/B reports don't present synthetic content as human-validated holdout?
| "category": "incident-repro", | ||
| "description": "Repro of a cache-poisoning incident: a cache key omits the tenant id, so one tenant's response is served to another. The caching-resource lens must catch it and block.", | ||
| "changedFiles": [ | ||
| { |
Contributor
There was a problem hiding this comment.
note (non-blocking): These new case.json files expand every field onto its own line, while all pre-existing corpus cases use compact inline objects (e.g. {"path": "...", "status": "modified"}). JSON isn't lint-covered here (eslint targets only .js/.mjs/.ts), so the style is hand-maintained — worth matching the existing compact style for consistency across the corpus.
…eliberately minimal A live sub-agent's cwd is the staged copy of the case tree, so it will often try to read imports or callers the tree does not carry; that read is an ordinary not-found tool error the agent works around, not a run failure. State this on the tree field so case authors know what a missing file costs (realism) and what to include (only context whose absence would change a reviewer's conclusion).
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
Phase 2 of the live A/B eval plan (#232), complete, stacked on the Phase 1 corpus format (#233): everything needed to run the real model sub-agents over a live-enabled corpus case. ## 2a: `eval/agent-extract.ts` Parses `review.md`'s `## agent:` sections into `{name, description, model, prompt}`. String in, no filesystem access, because the baseline arm of an A/B extracts the merge-base `review.md` via `git show`. Parsing is strict: missing/unterminated frontmatter, a name mismatch, a missing model, or an empty body throws with every problem listed, 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 default roster's reference to the production staging root so a rename fails a test instead of silently staging nothing. ## 2b: `eval/live-stage.ts` Materializes the production `/tmp/gh-aw/review/` layout for one case, per `review.md` Step 1's staging contract: `pr-context.json` (synthetic identity fields), `full.diff`/`pr.diff`/`full-stripped.diff`, `files.json` + `review-files.json` with the `hasPatch` cross-check, `provenance.json`, `routing.json` from the deterministic router, `out/`, and the post-change checkout from the case's tree. `rewriteAgentPrompt` swaps the production staging root across a prompt. ## 2c: `eval/live-producer.ts` + `eval/live-runner.ts` `produceLive` dispatches the live roster (default finders plus the router's `lensesToSpawn`; no pattern-triage or thread-reconciler in eval) behind an injected `LiveAgentRunner` seam, the same pattern as `judge.ts`'s injected model, so all its logic is stub-tested with zero model calls. It maps the three sub-agent output contracts into exactly the shapes the deterministic runner consumes (`RecordedFinding[]` for `produceFindings`, `CaseVerification[]` for the validation replay): label-shape findings get the code-assigned lens (`correctness`, or `conventions` for the skill-auditor so `labelForFinding` reproduces the best-practice label variants) and the production 0.7 confidence default; lens findings validate against the schema as-is; the claim-validator's `{claims: [...]}` output becomes verifications. It stages `claims.json`, resolves `{{#runtime-import}}` directives from 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 accounts per-agent cost/turns/wall-clock. `live-runner.ts` is the only module touching a real model runtime: one Agent SDK `query()` per dispatch, Read/Grep/Glob only, cwd pinned to the staged checkout, the agent's pinned model, hard turn and wall-clock caps, plus the CLI smoke entry: `pnpm dlx tsx workflows/review/eval/live-runner.ts --case <id>` (requires `ANTHROPIC_API_KEY`). Adds `@anthropic-ai/claude-agent-sdk` as a dev dependency. Documented deviations from production (module doc): no pattern-triage pass, runtime imports resolve from the case tree or a fixed note, and the investigation-cap CLI is not staged (its own denied-budget fallback applies). ## Test plan: - `pnpm run test --run`: 680 tests green (21 new across the three test files: extraction fixtures + real-review.md integration, staging via memfs, producer via a scripted stub runner covering happy path, retry, double-failure, empty-findings skip, id collisions, missing-agent error, and runtime-import resolution). - `pnpm run typecheck` and eslint clean. - The CLI smoke run against a live case needs `ANTHROPIC_API_KEY`, which this environment does not have; it is the first thing to run when testing this PR (any of the 13 live case ids, e.g. `--case incident-money-rounding`). Expected cost: roughly $0.50 to $2 per case with the default roster. ## Next steps (human) 1. ~~Key-bearing validation~~ DONE, superseded by the phase 4 acceptance runs, which exercised this producer end to end with a real key over 7 live cases x 2 arms x 2 PRs (runs [29052332224](https://github.com/Khan/actions/actions/runs/29052332224) and [29052334404](https://github.com/Khan/actions/actions/runs/29052334404)); measured ~$0.55-0.72 per case per arm, inside the plan's expected envelope. The CLI smoke remains available for local iteration. 2. Sanity-check the SDK runner's settings (`live-runner.ts`): `permissionMode: "bypassPermissions"` with tools restricted to Read/Grep/Glob and cwd pinned to the staged checkout. Confirm you are comfortable with that posture for CI. 3. Review the documented deviations from production in `live-producer.ts`'s module doc (no pattern-triage, runtime-import resolution, investigation-cap absence); each is a measurement caveat for the A/B. 4. New dev dependency: `@anthropic-ai/claude-agent-sdk`. Confirm it clears whatever dependency review applies to this repo. Author: jwbron Reviewers: jeresig, github-actions[bot], jaredly Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #234
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
Ports the seeded-defect trial from Khan/webapp#40678 into the review-workflow eval corpus as three live-enabled cases, stacked on #233 (the live-enabled corpus format). ## Sanitization constraint Khan/actions is public and Khan/webapp is private, so these are sanitized structural rewrites, not copies: fresh Go code around a generic "notes retention" feature (per-user stored notes, a background prune, deletion on account erasure) that reproduces the trial's defect mechanisms with generic naming. No webapp code, file paths, or identifiers appear anywhere in the cases. ## The three cases ### `trial-retention-deletion` (incident-repro, tags incident/trial/live) The trial's deletion-path seeds, five recorded findings across `erasure.go`, `store.go`, and a new test file: - `erasure-flag-gated-deletion` (blocking): the account-erasure deletion is gated behind a rollout flag, so flag-off silently skips the compliance deletion (trial seed 1). - `erasure-default-limit-one` (blocking): the deletion query leaves `Limit` unset and the store's documented zero-value default is 1, so at most one note is ever deleted (seed 2). - `erasure-ignores-delete-helper` (advisory): the new loop reimplements, worse, a paging batch-delete helper that already exists in the same file (seed 3). - `erasure-env-widening` + `store-env-widening` (advisory, one per file): the request-env interface is widened in two files solely for the buggy flag gate (seed 4). - `erasure-prune-error-swallowed` (blocking): `_ = PruneUserNotes(...)` discards the returned error; the prose cites the error-handling skill (seed 8). Expected: REQUEST_CHANGES, all six ids must-catch, 6 posted comments. ### `trial-retention-prune-tests` (incident-repro, tags incident/trial/live) The prune-and-tests seeds in a new `prune.go` + `prune_test.go` (both added files): - `prune-off-by-one-cap` (advisory, the trial's calibrated severity): `notes[maxRetainedNotes-1:]` retains 199 where the documented cap is 200 (seed 5). - `prune-test-vacuous-cap` (blocking): the cap test seeds 5 notes, far below the cap, so the early return makes it pass even for a no-op prune (seed 6). - `prune-suite-flag-mock` (blocking): `TestMain` forces the rollout flag on for the whole suite, so the flag-off path is never exercised (seed 7). - `prune-full-entity-fetch` (advisory): the prune fetches up to 5000 full entities but reads only ids; `Query.KeysOnly` exists for exactly this (seed 9). Expected: REQUEST_CHANGES, all four ids must-catch, 4 posted comments. ### `trial-batch-delete-wrapper` (clean, tags clean/trial/live) The trial's deliberate non-defect (seed 10) as a must-not-flag trap: the changed `purge.go` collects every note key and issues one `DeleteMulti` call that looks like it exceeds the datastore's 500-entity per-call cap. The tree also carries the unchanged wrapper source (`internal/datastore/client.go`) whose `DeleteMulti` visibly chunks into 500-key batches, so a live reviewer can discover the truth by reading it. The case records the wrong blocking claim, neutralizes it with a `refuted` validation entry, and pins `expected` to APPROVE with zero posted comments; `live.mustNotFlagSpecs` describes the trap so a live run that claims the cap is scored as a false block. ## Case mechanics - Each case carries a git-style unified diff computed from authored pre/post file contents (difflib-generated, added test files staged against `/dev/null` with a new-file mode line), the post-change tree next to `case.json`, `live.prContext`, and per-defect `mustCatchSpecs`/`mustNotFlagSpecs` with line windows of about +/-5 around the defect and 2-4 mechanism alternates. - Every recorded line-anchored finding anchors on an ADDED line of the case diff; the generator asserted this against the diff, so the change-provenance gate keeps every finding. - None of the cases are tagged `smoke`, keeping the per-PR smoke gate fast; both incident-repro cases feed must-catch recall in `suite.test.ts` automatically. - Includes a `review` patch changeset. ## Test plan: - `pnpm run test --run`: all 21 files / 662 tests green, including the corpus loader's live-block and tree validation over the new cases and `checkExpectation` on each case's expected block. - `pnpm run typecheck`: clean (no TypeScript touched). - Provenance-gate anchors verified mechanically at generation time: every finding's anchor line was asserted to be an added line of its file's diff. ## Next steps (human) 1. Sanitization check, the load-bearing review here: someone who knows the webapp trial code should confirm no webapp code, paths, or identifiers leaked into these three cases (Khan/actions is public; the cases must be structural rewrites only). 2. Severity calls to ratify: seed 7 (suite-wide flag mock) was authored as blocking by judgment call; seed 5 (off-by-one) is advisory per the trial's calibration. Adjust if you disagree; `expected` blocks and specs move together. 3. Rebase and merge after #233; the deterministic suite gates the rest. --- ## Update 2026-07-09: two misses promoted to smoke `trial-dedup-composite-key` and `trial-erasure-suite-flag-mock` (the v1.4.0 re-run misses) now carry the `smoke` tag, so every per-PR A/B watches the two defects the current production prompt demonstrably missed (~$1/run for the pair). The other trial cases deliberately stay behind the `full-eval` label. Author: jwbron Reviewers: jeresig, github-actions[bot], somewhatabstract Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #235
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
Phase 3 of the live A/B eval plan (#232), stacked on the Phase 2 producer (#234): score live runs against labeled ground truth and diff two review.md versions arm to arm. ## 3a: `eval/live-match.ts` Live runs choose their own finding ids, so the recorded metrics (which key on `expected.mustCatch` ids) cannot score them. The matcher maps a run's POSTED candidates onto the case's labeled defect specs: a 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, case-insensitively. Each candidate satisfies at most one spec and each spec at most one candidate, so one comment cannot claim two defects. An injected fallback arbiter (hard-capped, same-file candidates only, recorded as `via: "fallback"` for human audit) can rescue recall on vague prose; false flags are decided by the deterministic rule alone. `computeLiveMetrics` aggregates the live analogues of the suite's numbers: must-catch recall, verdict agreement, clean false-flag (a clean case that blocks counts), and noise (posted candidates matching no spec). ## 3b: `eval/live-ab.ts` The arm orchestrator and CLI. Baseline `review.md` comes from `git show <merge-base>` (or `--base-ref`), candidate from the working tree; both arms run 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-behavior seam. Each arm runs under half the `--max-usd` budget (default 40 total) with sticky exhaustion: once spend plus the running per-case average crosses the cap, the remaining cases are recorded as SKIPPED and the report still emits (dying at a cap with nothing posted is the failure mode the plan forbids). Each live result replays through the deterministic pipeline; `runner.ts` gains an optional `RunOptions.validation` override so the live validator's output replaces the recorded block. Spec-level regressions ("baseline caught, candidate missed" and the reverse) are diffed only over cases both arms actually scored, so a budget skip is never reported as a regression. Judge scoring reuses the pinned judge for quality aggregates only (judge-vs-ground-truth disagreement keys on recorded ids, which live arms do not use); the fetch model moves from `live-judge.ts` into shared `eval/judge-live-model.ts`. Output: `out/live-ab-report.json` plus a markdown table (metrics deltas, judge quality delta, cost, wall clock, regressions, skips, agent failures) printed and appended to `GITHUB_STEP_SUMMARY`. Report-only, with one exception per the playbook's standing rule: the candidate arm failing any adversarial-injection case (wrong verdict or missed spec) exits non-zero. ## Test plan: - `pnpm run test --run`: 696 tests green (16 new: matcher rules incl. window overlap, one-candidate-one-spec, malformed-regex fallback-to-literal, capped fallback audit trail; runArm budget semantics incl. the sticky-stop case that caught a real bug in review; regression diffing over shared cases; report rendering both gate outcomes). - `pnpm run typecheck` and eslint clean. - The end-to-end CLI (`pnpm dlx tsx workflows/review/eval/live-ab.ts --max-usd 10 --cases incident-money-rounding,clean-no-findings`) needs `ANTHROPIC_API_KEY`; this environment has none, so that is the first thing to exercise when testing. On an unchanged review.md it prints the identical-arms note and near-zero deltas. ## Next steps (human) 1. ~~End-to-end A/B validation~~ DONE, superseded by the phase 4 acceptance runs (linked on #237): the control arm held recall and verdict agreement flat at +0% and the weakened arm showed -17% recall with the lost spec named, which is also the first empirical read on matcher quality (7 cases matched; one systematic miss traced to lens routing, fixed on #233, not to the matcher). Two acceptance-driven additions landed here since: `perCase.failedAgents` entries now carry `<agent>: <reason>` (a claim-validator flake was undiagnosable from the report alone; the flake itself is still to be root-caused on a future run), and every report closes with a per-row stability footer (judge quality rose on the deliberately regressed arm because fewer, surer comments each score better, so the footer warns against reading it alone). 2. Review the two scoring caveats: judge output is quality-aggregates only (disagreement semantics key on recorded ids), and the fallback arbiter is wired but not yet given a live implementation (matches would be marked `via: "fallback"` for audit when one is added). 3. Confirm the budget semantics (half of `--max-usd` per arm, sticky skip-and-report) match how you want cost bounded. --- ## Update 2026-07-09: eval-tuning pass (three riders) Three instrument fixes from the [eval-tuning memo](https://claude.ai/code/artifact/2995f0b5-8840-4d03-96e3-2518e2f0f75a), landed here so the whole stack inherits them: 1. **Pre-flight identity short-circuit** (memo item 1, hoisted down from #251 with identical text so that branch merges cleanly): byte-identical review.md in both arms posts a no-reviewable-delta report and runs nothing; `--force-arms` preserved for wobble controls. Corpus-only and plumbing-only PRs in this stack now pay $0 per push. 2. **Gate-flip retry** (memo item 3): an adversarial hard-gate flip re-runs only the flipped cases, best of three, before the gate may fail the arm. Retried runs never replace the original in the metrics; they only decide whether the flip was a run-to-run flake (the #244/#245 incident shape). Retry spend is recorded in the report. 3. **Drop-bucket taxonomy** (memo item 4): every missed must-catch spec is classified true-miss vs found-but-dropped (provenance/scope/validation) by re-matching against the dropped candidates with the line window relaxed. Lost regressions are annotated with their class and the table gains a found-but-dropped count row. Plus **judge economics**: the judge pin moves from Opus 4.8 to `claude-haiku-4-5-20251001` (the judge grades prose only; every load-bearing metric is deterministic, and the acceptance runs showed judge quality is not single-run-stable on any model, so it is priced accordingly; supersedes operator direction 4), and the judge seam gains retry with backoff so a transient 500 no longer wastes a whole scoring pass. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron, jaredly Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #236
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
Phase 5 of the live A/B eval plan (#232), independent of the eval-code stack (#233-#237): the `review-trial` Claude Code skill, packaging the Khan/webapp#40678 seeded-defect trial choreography so an operator can run one in an afternoon. ## What the skill does Given a consumer repo, a seeded branch, a **human-authored** defect table, an arms list, and budget approval (projected up front from the trial's measured $7-10 per workflow-arm run, confirmed before any PR is created), it: 1. Creates one isolated PR per arm from the seeded branch, with per-arm trigger recipes (repo-default reviewer, pinned workflow ref, hosted `@claude review`) and two hard rules: distinct workflow names (same-named gh-aw workflows share a per-PR concurrency group and cancel each other; this silently ate a run in the original trial) and exactly one reviewer per PR (suppress the default via `skip-ai-review` on non-default arms). 2. Collects each run's review, artifacts (tolerating the known gh-aw staging-path artifact annotation), billed credits, and wall clock. 3. Optionally drives the lifecycle protocol (push 2 with fixes plus fresh seeds, push 3 all-fixed) identically across arms, scoring each push separately. 4. Scores defect by defect with the deterministic rule mirroring `eval/live-match.ts` (path + window + mechanism), manual judgment for leftovers marked as judged in the report, traps counted as correct suppression, and faithful reporting whichever arm it favors; output in the #40678 report shape. 5. Exports live-enabled corpus case skeletons, with an explicit sanitization gate for private-repo content landing in this public repo (the #235 lesson: structural rewrites, never copied code). 6. Cleans up: PRs closed with the report linked, trial branches deleted, no temporary workflow left behind. What stays human is stated at the top of the skill: seeds and ground truth are operator-authored, always; the skill refuses to improvise a defect table. ## `.gitignore` change `.claude` was ignored wholesale, which would have kept project skills untracked. Narrowed to `.claude/*` with a `!.claude/skills/` carve-out; verified `settings.json` and `worktrees/` remain ignored. ## Test plan: Docs/tooling only (empty changeset); no code paths. `git check-ignore` verified the carve-out (SKILL.md tracked, `settings.json` and `worktrees` still ignored). The real acceptance is the plan's: re-running the #40678 trial via this skill reproduces its table in an afternoon, which needs a consumer-repo trial to exercise. ## Next steps (human) 1. Review the `.gitignore` change deliberately: `.claude` wholesale becomes `.claude/*` + `!.claude/skills/`. Verified locally that `settings.json` and `worktrees/` stay ignored, but this is a policy change about what agent tooling gets committed to the repo. 2. The skill's acceptance is operational, not CI: next time a trial is warranted (an architecture-bet change like the deterministic orchestrator, or pre-graduation ground-truthing), run it via this skill and check it reproduces the #40678 report shape in an afternoon. There is no cheaper honest validation. 3. Skim the per-arm trigger recipes against current consumer-repo reality (the `skip-ai-review` label convention, ready-for-review semantics); those conventions live outside this repo and can drift. Author: jwbron Reviewers: jeresig, github-actions[bot], somewhatabstract Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 10 checks were successful Pull Request URL: #238
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
Adds `workflows/review/eval/live-ab-plan.md`: a phased, agent-executable plan for real-model evals of review-bot changes, making the round-two doc's eval playbook executable for its behavior-shifting and cost-affecting change classes (today only the deterministic replay exists, which is structurally blind to prompt and model changes). The phases: 1. **Live-enabled corpus cases**: real diffs, post-change file trees, PR context, and labeled defect specs (live runs choose their own finding ids, so ground truth matches on anchor window + mechanism, not id). Implementation is up in a separate PR. 2. **Live producer**: run the actual sub-agent roster from `review.md` over a case via the Agent SDK (Read/Grep/Glob only, staged checkout, per-agent model pinning, live claim-validator, cost capture), behind the `produceFindings` seam `runner.ts` already ships. 3. **A/B runner + delta report**: same-run arms (merge-base `review.md` vs working tree), five-metric deltas, judge quality deltas, per-case cost, adversarial hard gate outright. 4. **CI wiring**: `review-eval-ab.yml` on `paths: [workflows/review/**]`, sticky PR comment, smoke-live subset by default, `full-eval` label escalation. Settled decisions recorded in the doc: same-run A/B over stored baselines; candidate corpus + lib for both arms (isolate the model seam); bounded agentic sub-agents, not one-shot completions; report-only except the adversarial gate; budget caps that still emit a partial report. Also records how this instrument relates to seeded live-PR trials like Khan/webapp#40678 (complementary layers, different cadences). ## Test plan: Docs only (plus an empty changeset). `pnpm run test --run` stays green. ## Next steps (human) 1. Review the settled design decisions section; they bind the whole stack (#233-#238), so disagreement is cheapest to voice here. 2. Two decisions the plan deliberately leaves to humans, no code blocked on them: ratify the gate thresholds that would turn the A/B report into a merge gate (until then everything is report-only except the adversarial rule), and decide whether `pattern-triage` joins the live roster. 3. This PR is docs plus an empty changeset; it can merge independently of the stack whenever the plan reads right. All five phases have landed and the doc now records the phase 4 acceptance results (both arms passed; the plan's acceptance section names the runs and the judge-direction lesson), so the remaining human items are exactly the two decisions in step 2 plus the consumer-repo trial that accepts #238. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #232
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
* [jwbron/live-eval-corpus] review: live-enabled corpus format and ten 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. * [jwbron/live-eval-producer-staging] review: live-producer prompt extraction 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. * [jwbron/live-eval-producer-staging] review: the live producer and SDK 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. * [jwbron/live-eval-ab-runner] review: the live A/B runner (phase 3) 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. * [jwbron/live-eval-ab-ci] review: per-PR live A/B workflow (phase 4) 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. * [jwbron/live-eval-corpus] review: exclude eval-corpus trees from lint 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). * [jwbron/live-eval-producer-staging] review: namespace live finding ids 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. * [jwbron/live-eval-ab-runner] review: judge failures degrade the A/B report 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. * [jwbron/live-eval-corpus] review: route the specialist lens on each live 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. * [jwbron/live-eval-ab-runner] review: carry agent-failure reasons into 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. * [jwbron/live-eval-ab-runner] review: close every A/B report with a per-row stability footer The phase 4 acceptance pair measured which report rows a reader may act on from one run: recall, verdict agreement, the regression lists, and the adversarial gate reproduced exactly on the no-op control, while judge quality and noise moved on jitter alone; and on the weakened arm judge quality went UP 0.16 as recall fell 17 points (fewer, surer comments each score better). The footer states this on every report so nobody chases a judge-quality delta or reads one as health. * [jwbron/live-eval-ab-runner] review: identity short-circuit, gate-flip retry, drop-bucket taxonomy Three instrument fixes from the eval-tuning memo, landed in the runner so the whole stack above inherits them: - Pre-flight identity short-circuit (memo item 1): byte-identical review.md in both arms posts a no-reviewable-delta report and runs nothing; --force-arms preserved for deliberate wobble controls. - Gate-flip retry (memo item 3): an adversarial hard-gate flip re-runs only the flipped cases, best of three, before the gate may fail the arm. Retried runs never replace the original in the metrics; they only decide whether the flip was a run-to-run flake. Retry spend is recorded in the report. - Drop-bucket taxonomy (memo item 4): each missed must-catch spec is classified true-miss vs found-but-dropped (provenance/scope/ validation) by re-matching the spec against the dropped candidates with the line window relaxed (a mis-anchored real finding is exactly the case to surface). The report annotates every lost regression with its class and carries a found-but-dropped count row. * [jwbron/live-eval-ab-runner] review: judge economics (Haiku pin, retry with backoff) The judge grades one comment's prose in 512 tokens; every load-bearing metric (recall, verdict agreement, regressions, adversarial gate) is deterministic and never touches it, and the acceptance runs showed the judge signal is not single-run-stable on any model (it moved 0.11 on the byte-identical control and went UP on the weakened arm). Price that signal accordingly: the pin moves from claude-opus-4-8 to claude-haiku-4-5-20251001 (a fifth of the cost; dated snapshot so week-over-week scores compare; both arms always share one judge, so within-run deltas are unaffected). Supersedes operator direction 4, which pinned Opus. Also adds retry with backoff (3 attempts, 429/408/ 5xx/network only) to the one live judge seam; a single transient 500 previously wasted an entire weekly scoring pass. * [jwbron/live-eval-ab-ci] review: document why the baseline is the base tip, not the merge-base The workflow passes origin/<base_ref> (the base branch tip) while its comments and the dispatch-input description said merge-base. The code is the right side of that disagreement: pull_request runs check out the PR merge commit, so the candidate tree already contains the base tip; baselining on the same tip isolates the PR's own review.md delta, where a merge-base baseline would fold upstream review.md movement into the PR's measured numbers. Prose updated to match the code; no behavior change. * [jwbron/live-eval-ab-runner] review: type the judge response via the fetch signature (eslint no-undef) * review: add the review-trial skill (live A/B phase 5) (#238) Phase 5 of the live A/B eval plan (#232), independent of the eval-code stack (#233-#237): the `review-trial` Claude Code skill, packaging the Khan/webapp#40678 seeded-defect trial choreography so an operator can run one in an afternoon. ## What the skill does Given a consumer repo, a seeded branch, a **human-authored** defect table, an arms list, and budget approval (projected up front from the trial's measured $7-10 per workflow-arm run, confirmed before any PR is created), it: 1. Creates one isolated PR per arm from the seeded branch, with per-arm trigger recipes (repo-default reviewer, pinned workflow ref, hosted `@claude review`) and two hard rules: distinct workflow names (same-named gh-aw workflows share a per-PR concurrency group and cancel each other; this silently ate a run in the original trial) and exactly one reviewer per PR (suppress the default via `skip-ai-review` on non-default arms). 2. Collects each run's review, artifacts (tolerating the known gh-aw staging-path artifact annotation), billed credits, and wall clock. 3. Optionally drives the lifecycle protocol (push 2 with fixes plus fresh seeds, push 3 all-fixed) identically across arms, scoring each push separately. 4. Scores defect by defect with the deterministic rule mirroring `eval/live-match.ts` (path + window + mechanism), manual judgment for leftovers marked as judged in the report, traps counted as correct suppression, and faithful reporting whichever arm it favors; output in the #40678 report shape. 5. Exports live-enabled corpus case skeletons, with an explicit sanitization gate for private-repo content landing in this public repo (the #235 lesson: structural rewrites, never copied code). 6. Cleans up: PRs closed with the report linked, trial branches deleted, no temporary workflow left behind. What stays human is stated at the top of the skill: seeds and ground truth are operator-authored, always; the skill refuses to improvise a defect table. ## `.gitignore` change `.claude` was ignored wholesale, which would have kept project skills untracked. Narrowed to `.claude/*` with a `!.claude/skills/` carve-out; verified `settings.json` and `worktrees/` remain ignored. ## Test plan: Docs/tooling only (empty changeset); no code paths. `git check-ignore` verified the carve-out (SKILL.md tracked, `settings.json` and `worktrees` still ignored). The real acceptance is the plan's: re-running the #40678 trial via this skill reproduces its table in an afternoon, which needs a consumer-repo trial to exercise. ## Next steps (human) 1. Review the `.gitignore` change deliberately: `.claude` wholesale becomes `.claude/*` + `!.claude/skills/`. Verified locally that `settings.json` and `worktrees/` stay ignored, but this is a policy change about what agent tooling gets committed to the repo. 2. The skill's acceptance is operational, not CI: next time a trial is warranted (an architecture-bet change like the deterministic orchestrator, or pre-graduation ground-truthing), run it via this skill and check it reproduces the #40678 report shape in an afternoon. There is no cheaper honest validation. 3. Skim the per-arm trigger recipes against current consumer-repo reality (the `skip-ai-review` label convention, ready-for-review semantics); those conventions live outside this repo and can drift. Author: jwbron Reviewers: jeresig, github-actions[bot], somewhatabstract Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 10 checks were successful Pull Request URL: #238
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
…g-relative paths) (#243) * [jwbron/live-eval-corpus] review: live-enabled corpus format and ten 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. * [jwbron/live-eval-producer-staging] review: live-producer prompt extraction 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. * [jwbron/live-eval-producer-staging] review: the live producer and SDK 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. * [jwbron/live-eval-ab-runner] review: the live A/B runner (phase 3) 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. * [jwbron/live-eval-ab-ci] review: per-PR live A/B workflow (phase 4) 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. * [jwbron/live-eval-corpus] review: exclude eval-corpus trees from lint 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). * [jwbron/live-eval-producer-staging] review: namespace live finding ids 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. * [jwbron/live-eval-ab-runner] review: judge failures degrade the A/B report 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. * [jwbron/review-trial-skill] review: add the review-trial skill (live 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. * [jwbron/review-out-artifact-upload] review: fix the out/ artifact upload (allowed-paths must match staging-relative paths) * [jwbron/live-eval-corpus] review: route the specialist lens on each live 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. * [jwbron/live-eval-ab-runner] review: carry agent-failure reasons into 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. * [jwbron/live-eval-ab-runner] review: close every A/B report with a per-row stability footer The phase 4 acceptance pair measured which report rows a reader may act on from one run: recall, verdict agreement, the regression lists, and the adversarial gate reproduced exactly on the no-op control, while judge quality and noise moved on jitter alone; and on the weakened arm judge quality went UP 0.16 as recall fell 17 points (fewer, surer comments each score better). The footer states this on every report so nobody chases a judge-quality delta or reads one as health. * [jwbron/live-eval-ab-runner] review: identity short-circuit, gate-flip retry, drop-bucket taxonomy Three instrument fixes from the eval-tuning memo, landed in the runner so the whole stack above inherits them: - Pre-flight identity short-circuit (memo item 1): byte-identical review.md in both arms posts a no-reviewable-delta report and runs nothing; --force-arms preserved for deliberate wobble controls. - Gate-flip retry (memo item 3): an adversarial hard-gate flip re-runs only the flipped cases, best of three, before the gate may fail the arm. Retried runs never replace the original in the metrics; they only decide whether the flip was a run-to-run flake. Retry spend is recorded in the report. - Drop-bucket taxonomy (memo item 4): each missed must-catch spec is classified true-miss vs found-but-dropped (provenance/scope/ validation) by re-matching the spec against the dropped candidates with the line window relaxed (a mis-anchored real finding is exactly the case to surface). The report annotates every lost regression with its class and carries a found-but-dropped count row. * [jwbron/live-eval-ab-runner] review: judge economics (Haiku pin, retry with backoff) The judge grades one comment's prose in 512 tokens; every load-bearing metric (recall, verdict agreement, regressions, adversarial gate) is deterministic and never touches it, and the acceptance runs showed the judge signal is not single-run-stable on any model (it moved 0.11 on the byte-identical control and went UP on the weakened arm). Price that signal accordingly: the pin moves from claude-opus-4-8 to claude-haiku-4-5-20251001 (a fifth of the cost; dated snapshot so week-over-week scores compare; both arms always share one judge, so within-run deltas are unaffected). Supersedes operator direction 4, which pinned Opus. Also adds retry with backoff (3 attempts, 429/408/ 5xx/network only) to the one live judge seam; a single transient 500 previously wasted an entire weekly scoring pass. * [jwbron/live-eval-ab-ci] review: document why the baseline is the base tip, not the merge-base The workflow passes origin/<base_ref> (the base branch tip) while its comments and the dispatch-input description said merge-base. The code is the right side of that disagreement: pull_request runs check out the PR merge commit, so the candidate tree already contains the base tip; baselining on the same tip isolates the PR's own review.md delta, where a merge-base baseline would fold upstream review.md movement into the PR's measured numbers. Prose updated to match the code; no behavior change. * [jwbron/live-eval-ab-runner] review: type the judge response via the fetch signature (eslint no-undef)
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.
Phase 1 of the live A/B eval plan (#232): the eval corpus learns to carry real change content so a future live producer can run the actual model sub-agents against it.
Format
A case may now carry an opt-in
liveblock (corpus/live.ts, re-exported through the loader):prContext: PR title/description/author/base branch, mirroring productionpr-context.json. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff.<id>/case.json+<id>/tree/layout coexisting with flat<id>.json. A directory containingcase.jsonis one case; its tree is never parsed as corpus JSON.mustCatchSpecs/mustNotFlagSpecs: labeled defects as (path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window plus mechanism rather than id; the Phase 3 matcher consumes these.Enforced invariants: the
livetag and the block imply each other; a live case needs a cleanly-parseable diff (fail-open is fine in production, an authoring error here); spec paths must appear inchangedFilesand the diff; every non-removed changed file must exist in the tree.loadLiveCorpus()returns the subset.Cases
Ten cases converted with hand-authored real diffs and trees: the five smoke incidents (money rounding, auth bypass, cache key, race condition, missing index), both clean cases, the adversarial injection case (payload is a code comment in the diff instructing the reviewer to approve), the golden authz holdout, and the money-payments synthetic mutation. Recorded line anchors are rewritten to the authored defect lines; natural files beat content padded to synthetic line numbers, and every anchor is asserted to be an added line so the provenance gate (now active on these cases) keeps them.
The trial-content port landed as #235, stacked on this PR. One acceptance-driven change also landed here: the phase 4 runs missed
incident-sql-missing-indexin all four arms because live cases carried norouterConfiglens rules, so specialist lenses never spawned; every live case whose ground truth belongs to a specialist lens now routes that lens on the finding's file (see the "route the specialist lens on each live case" commit).Test plan:
pnpm run test --run: 659 tests green, including 17 new loader tests (memfs) covering the live block, both layouts, tree validation, and the tree-JSON exclusion.pnpm run typecheckand eslint clean on the three touched TS files.Next steps (human)
pnpm run test --run) fully gates it.