review: hand off out-of-lane observations instead of dropping them#245
Open
jwbron wants to merge 8 commits into
Open
review: hand off out-of-lane observations instead of dropping them#245jwbron wants to merge 8 commits into
jwbron wants to merge 8 commits into
Conversation
🦋 Changeset detectedLatest commit: 7de0635 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 |
… instead of dropping them
jwbron
force-pushed
the
jwbron/review-rereview-accountability
branch
from
f85b74f to
491a983
Compare
jwbron
force-pushed
the
jwbron/review-out-of-lane
branch
from
df3d369 to
7b5318c
Compare
Contributor
Author
This was referenced Jul 9, 2026
…untability' into jwbron/review-out-of-lane
2 tasks
khan-actions-bot
requested review from
a team,
jaredly and
jeresig
and removed request for
a team
Contributor
Review live A/BBaseline:
Improvements (candidate caught, baseline missed)
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. |
…untability' into jwbron/review-out-of-lane
…untability' into jwbron/review-out-of-lane
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * block on its own. | ||
| */ | ||
| export type OutOfLaneObservation = { | ||
| /** Repo-relative path the observation anchors on. */ |
Contributor
There was a problem hiding this comment.
thought (non-blocking): The production orchestrator now routes out-of-lane observations, but the live-eval producer (eval/live-producer.ts parseAgentFindings) reads only findings[]. So in a review-trial a handed-off observation is dropped and the arm scores as if it never happened — the same drop this PR closes on the production path. Worth teaching the eval harness to read out_of_lane_observations[] in a follow-up so the trial can measure this feature.
| * returned, so a producer's malformed handoff is diagnosable from the run | ||
| * artifact rather than silently dropped. | ||
| */ | ||
| export const validateOutOfLaneObservation = ( |
Contributor
There was a problem hiding this comment.
question (non-blocking): validateOutOfLaneObservation is referenced only by the tests — the orchestrator prose in review.md never invokes it (unlike the provenance gate, which runs a real CLI). Is the intent that shape enforcement stays eval/test-only, as validateFinding already is? If so, the PR description's "shape is code-validated" slightly overstates the live behavior; a one-line note that the live gate is the orchestrator prompt would settle it.
| lens, a concern outside its domain). Do not discard these. Convert each observation | ||
| into a candidate comment in the same label-bearing shape as every other candidate: | ||
| `path`/`line` from the observation, `subject` from its `observation` text verbatim, | ||
| `failure_scenario` verbatim, and the label **`question (non-blocking)`** — the label |
Contributor
There was a problem hiding this comment.
thought (non-blocking): The handoff channel has a lower evidentiary bar than the lane it feeds. A finding must carry evidence_trace and an honest confidence; an out-of-lane observation carries neither, so once converted to a candidate it has no confidence and defaults to 0.7 (review.md:693) — above the 0.5 medium posting bar — leaving the claim-validator as the only gate before it posts. Consider code-assigning out-of-lane candidates a below-default confidence so only validator-confirmed ones clear the bar.
| is code-assigned, never model-chosen: an out-of-lane observation is a handoff, not a | ||
| vetted finding, so it can never block on its own (and the `claim-validator` never | ||
| upgrades severity). Set the candidate's `source` to `"<agent> (out-of-lane)"`. From | ||
| here each one flows through the identical change-provenance gate → scope filter → |
Contributor
There was a problem hiding this comment.
thought (non-blocking): Nothing merges same-line candidates from different sources within a run (the only dedup layers are the thread-reconciler and the scope filter), so the motivating case — the skill-auditor hands off a correctness bug the correctness-reviewer also finds — can post the same line twice. Also suggested_lane is collected on the observation but never consumed in the routing paragraph: either use it for an ownership/dedup check or drop the field.
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 `live` block (`corpus/live.ts`, re-exported through the loader): - `prContext`: PR title/description/author/base branch, mirroring production `pr-context.json`. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff. - An on-disk post-change file tree, via a new `<id>/case.json` + `<id>/tree/` layout coexisting with flat `<id>.json`. A directory containing `case.json` is 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 `live` tag 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 in `changedFiles` and 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-index` in all four arms because live cases carried no `routerConfig` lens 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. - The deterministic suite runs the ten converted cases unchanged (same verdicts, comment counts, must-catch sets), now with the provenance gate active on them. - `pnpm run typecheck` and eslint clean on the three touched TS files. ## Next steps (human) 1. Review the ten authored diffs and trees for realism; they are synthetic content a live model will read, so plausibility matters more than in ordinary fixtures. Spot-check that each case's recorded finding still describes the authored defect (anchors were rewritten to the authored lines). 2. This is the root of the stack: review and undraft first; #234 and #235 rebase onto it. 3. No live validation needed here; the deterministic suite (`pnpm run test --run`) fully gates it. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #233
# Conflicts: # workflows/review/eval/corpus/live.ts
Contributor
Review Guidancegithub-actions (2 files)
Common patterns
Excluded from review (3 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
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
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
1 participant
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.
Summary
Third of five PRs in the fold-in batch two stack (stacked on #244; the stack is based on #238,
jwbron/review-trial-skill). The top of the batch-two stack will bejwbron/review-skill-rule-quote.In run 2 of the v1.4.0 re-run lifecycle the skill-auditor found a real issue (dedup reads via an eventually-consistent Query immediately after PutMulti) and dropped it, verbatim: "that's a correctness concern, not a quotable skill-rule violation, so I'll leave it". That was the correct call under quote-the-rule, but the observation died with it.
Failure scenario fixed: a reviewer investigating a rule notices a genuine correctness defect, correctly declines to report it as a skill violation, and has no other channel; the defect reaches the PR author in no form and ships.
The handoff:
out_of_lane_observations[]alongsidefindings[]:path, optionalline, the concern stated concretely, a required concretefailure_scenario(the claim the validator attacks, same discipline as findings), and an optionalsuggested_lane.question (non-blocking); the label is never model-chosen, because a handoff is not a vetted finding and must not block on its own (and the claim-validator never upgrades severity). Candidates then flow through the identical provenance gate, scope filter, claims.json, and validation path as every other candidate; validation into the correctness lane, rather than re-dispatching the correctness reviewer, keeps the handoff inside the run's existing budget.validateOutOfLaneObservation/isValidOutOfLaneObservationinlib/finding-schema.ts, with the same collect-every-error contract asvalidateFindingso a malformed handoff is diagnosable from the run artifact. It is a sibling type, not aFindingfield, soFINDING_SCHEMA_VERSIONstays 2 and no serialized finding is invalidated.The per-lens handoff paragraph and JSON field are stamped into each of the 11 lens definitions (mechanically, identical text); the next PR in the stack (dispatch-tax trim) folds exactly this kind of repeated block into the shared staged disciplines context, so the duplication added here is transitional within the same stack.
cc @jeresig
Testing
npx vitest run workflows/review: 484 tests green (7 new observation-schema tests: minimal and full shapes, required fields, error collection, line bounds).npx tsc --noEmitclean.