review: eval drift report 2026-07-20 - #265
Merged
Merged
Conversation
|
jwbron
approved these changes
Jul 20, 2026
This was referenced Jul 20, 2026
jwbron
added a commit
that referenced
this pull request
Jul 21, 2026
…and cases (#270) Improves the discrimination power of the review eval corpus, working from the 2026-07-20 drift run (PR #265, run 29724668102). Every claim below was verified by replaying that run's recorded artifacts through the deterministic matcher at $0 (`--no-match-arbiter` semantics); the deterministic suite passes before and after (346 -> 348 tests). ## 1. Floor-case triage (replay evidence) | Case / row | Recorded | Deterministic replay | Verdict | | --- | --- | --- | --- | | `golden-retention-lifecycle-3` (verdict) | 0/6 | 0/6, trap false-flagged 6/6 | **Ground truth bug.** The "documented deliberate" `void pruneNotes(...)` was a real defect: `void` discards the promise but not its rejection, so a failing prune is an unhandledRejection that can crash the process, the opposite of the comment's stated intent. The reviewer blocked it, correctly, in every sample (`retention.ts:26`, blocking, conf 0.7, all 6). Fix: the tree now implements the intent (`.catch(() => {})`), same line count, diff updated; the must-not-flag trap now also covers the swallow phrasing. Needs one live confirmation run (recorded artifacts reviewed the old tree). | | `trial-dedup-eventual-consistency` (verdict) | 0/2 | 2/2 after fix | **Ground truth bug.** Expected APPROVE contradicted the case's own premise: the PR claims to stop double-submit duplicates and the eventually consistent read fails for exactly that traffic. Both samples caught the defect (spec 2/2) and blocked; flipped expected verdict to REQUEST_CHANGES, recorded fixture raised to blocking. | | `golden-retention-lifecycle-1:retention-unbounded-prune` | 0/6 | 0/6, no dropped-bucket rescue | **Genuine miss, unfair spec.** No posted finding in any sample describes the mechanism, so the matcher is fine; but the facade documents no `deleteMulti` batch limit, so the "defect" was not evidenceable in-tree (the reviewer would have had to invent the constraint). Spec retired; the mechanism returns as `mutation-retention-batch-delete-limit` with the limit documented (below). | Two instrument bugs surfaced by the same replay (the `sql-missing-index` precedent, twice): - `golden-retention-lifecycle-2:retention-dedup-window-untested`, the corpus's only "in-band" row (3/6), was **entirely Haiku-arbiter false positives**: all three recorded catches were `via: "fallback"` claims of the overcorrected off-by-one finding at `retention.ts:38`, a different defect. Deterministic rate was 0/6. The bare `"boundary"` mechanism alternate (shown to the arbiter as ground truth) was the false-positive vector. Fixed: dedup/window-specific mechanisms plus an `altLocations` entry for the test file; replays an honest **3/6 deterministic**, matched findings verified to be the dedup-coverage gap. - `trial-dedup-composite-key:save-dedup-composite-key`: reviewer caught it 5/5, deterministic matcher 1/5 (order-sensitive regexes); the arbiter was papering over it. Broadened alternates replay **5/5 deterministic**. ## 2. Noise-pool mining (all 126 unmatched findings classified) The 6-sample noise pool (55 baseline + 61 candidate after replay, 52-55% reported) decomposes into 36 (case, path, line) clusters: | Class | Instances | Disposition | | --- | --- | --- | | Real defect, unannotated | 43 | Annotated: 4 new specs (below) | | Duplicate report of an already-matched defect | ~35 | Stays noise (it is real author-facing chattiness; the two-comments-per-defect pattern in auth-bypass, cache-missing-key, authz, mutation-money, race-condition, money-rounding is a review-side dedup gap worth its own fix) | | Re-review duplicates of KEPT prior threads | 12 | Stays noise by design: lifecycle-2's overcorrected off-by-one and still-vacuous test map to threads with `expect: keep`; fresh findings there are what re-review dup-scoring penalizes. Deliberately NOT annotated (reasons recorded in the case description) | | Companion test-gap advisories, contract-hardening suggestions, design commentary | ~30 | Chattiness; not annotated | | Injection attempt surfaced as advisory (`adversarial-injection-approve` `handler.ts:8`, 5/6) | 5 | Desired behavior, not a defect; left unannotated to avoid touching the hard-gate case. Candidate spec if we later want to pin "surface the injection" as recall | | lifecycle-3 advisory cluster (purge test gap etc.) | ~8 | Advisory-grade on an expected-clean re-review case; stays noise | New specs from the "real defect" class, all replay-verified: - `incident-sql-missing-index:dm-default-backfill` (5/6): `DEFAULT 'pending'` backfills every fulfilled historical order into the picker queue; `altLocations` accepts the migration or the query anchor. - `golden-retention-fix-push:quota-exceeded-never-true` (6/6): remaining is clamped to zero then compared `< 0`, so the quota is never enforced; introduced by this push's own hunk. - `golden-retention-lifecycle-1` and `-2` `retention-prune-unhandled-rejection` (6/6 each): the void-prune crash, flagged blocking in every sample of both pushes. lifecycle-1 keeps four planted defects (the retired unbounded-prune slot is taken by this real one, fixture and all); lifecycle-3's fixed tree closes the arc. No saturated case's existing specs, trees, or expectations were touched. ## 3. New mid-band cases (validation pending) Both target the demonstrated weak class (multi-file retention/lifecycle reasoning), graded easier than the retired floor specs: the evidence chain exists in-tree but crosses files. - **`mutation-retention-batch-delete-limit`**: unchanged `purge-user-data.ts` documents the datastore's 100-key `deleteMulti` limit and chunks accordingly; the new prune passes an entire launch backlog to one call, the documented fire-and-forget catch swallows the rejection, and the backlog test passes only because the in-memory fake enforces no limit. Carries the same must-not-flag swallow trap as lifecycle-3. - **`mutation-retention-window-units`**: window constant in days (`config.ts`), `createdAt` documented as epoch milliseconds (`db.ts`), the `since` arithmetic omits the `*1000`, so the "30-day" window is about 43 minutes and dedup silently no-ops for the traffic it names. Neither is smoke-tagged, so the per-PR A/B on this PR does not exercise them. **Validation is done**: the identical-arm calibration (run 29765059892) and the 2026-07-20 pooled A/B (runs 29783522498-29783549087) both saturated them (batch-delete-limit 2/2 then 6/6 across arms, window-units 2/2 then 5/5). Consistent with the eval README's synthetic-saturation finding, they stay live as family tripwires rather than discriminators; both case descriptions record the outcome and run ids. The pooled A/B also exposed a trap over-match (two candidate samples posted a non-blocking "log instead of an empty catch" companion advisory that scored as a false flag next to the deterministic catch): specs now support `blockingOnly`, set on both swallow traps, so only a blocking condemnation of the documented fire-and-forget can hit them. ## 4. Budget The 2026-07-20 run spent $84.33 of its $85 cap and budget-skipped the corpus tail (composite-key 5/6 samples, dedup-eventual-consistency 2/6, erasure-suite-flag-mock 1/6), which is why the noise-floor bands carry the case-mix warning. Measured ~$0.82 per case-arm-run; the 23-case corpus (both new cases are live-tagged) at 3 repeats x 2 arms is 138 case-arm-runs, ~$113 projected. Default `max_usd` raised to 120. Rejected: 2 repeats (~$62, halves the power axis) and per-case repeat tiering (~$58, needs live-ab changes and makes every noise-floor sample a different case mix, the exact contamination the aggregate warns about). ## Corpus shape, before -> after Spec rows per drift run (recorded 2026-07-20 ruler vs this branch replayed on the same artifacts): | Band | Before (recorded) | After (deterministic replay) | | --- | --- | --- | | Floor (0-20%) | 3 rows: unbounded-prune 0/6, lifecycle-3 verdict 0/6, dedup-EC verdict 0/2 (+ window-untested 0/6 in truth) | 1 row: erasure-suite-flag-mock verdict 0/1 (1 sample; out of scope here, same severity-calibration family as dedup-EC, worth watching next run) | | In-band (20-80%) | 1 illusory row (window-untested 3/6, all arbiter FPs) | 1 honest row (window-untested 3/6 deterministic) + 2 new cases pending validation | | Ceiling (80-100%) | 15 spec rows + 16 verdict rows | 21 spec rows + 18 verdict rows (4 mined specs and 2 fixture rows are deliberate: they clean the noise metric and add tripwires) | Unmatched-posted findings on the same artifacts drop 126 -> 96 (noise ~52/55% -> ~44/45%), all of it from mislabeled ground truth, none from loosening the matcher's location-AND-mechanism rule. Note for the drift series: the corpus content hash changes, so the report's Ruler line will flag the discontinuity; week-over-week rows for untouched saturated cases remain comparable. Author: jwbron Reviewers: github-actions[bot], jwbron, jeresig, jaredly, somewhatabstract Required Reviewers: Approved By: github-actions[bot], jeresig Checks: ⏭️ 1 check has been skipped, ✅ 9 checks were successful Pull Request URL: #270
jwbron
added a commit
that referenced
this pull request
Jul 21, 2026
…rift budget (#267) ## What Two follow-ups from the first scheduled drift report (#265): 1. **Identical-arm eval reports no longer read as an A/B.** When both arms ran the same review.md (only possible under `--force-arms`: the weekly drift watch and manual wobble controls), the single-run, multi-repeat, and aggregate renderers now retitle themselves "Review wobble control (identical arms)", relabel Baseline/Candidate to Arm A/Arm B, and state up front that between-arm deltas are run-to-run wobble, not a prompt effect. The aggregate additionally leads with the noise-floor bands: on an identical-arm pool the bands are the product and the per-case table is the raw material. The aggregate keys the relabel off `noiseFloor` being present, which is computed iff every pooled sample ran identical arms. 2. **A contaminated noise floor is a red run, not a footnote.** The 2026-07-20 run spent $84.33 of its $85 cap and budget-skipped cases, which the report flags as contaminating the noise-floor bands with case-mix variance; but the flag lives inside the report body, so the degradation persists silently until someone reads it. The drift job now fails when the aggregate reports `caseAsymmetry` (after the artifact upload and visibility PR, both `if: always()`), so a corpus outgrowing the budget surfaces as a red run. The budget resize itself (85 -> 120 -> 240) moved to the corpus-growth stack (#270/#266), which owns the sizing; three open PRs were editing the same lines. ## Why the relabel matters #265 renders a "Baseline vs Candidate" per-case table with small deltas between two runs of the same prompt; that framing invites reading wobble as a result. The drift run's whole point is that within-run deltas are noise, so the report should say so in its own headline. ## Testing - `pnpm test --run workflows/review/eval/` (new cases cover the multi-repeat, single-run, and aggregate relabels, the noise-floor-first ordering, the identical-arm drop-note prefix, and that A/B reports keep the Baseline/Candidate framing) - `pnpm typecheck`, prettier clean on changed files Author: jwbron Reviewers: github-actions[bot], jwbron, jeresig, kevinb-khan Required Reviewers: Approved By: github-actions[bot], jeresig Checks: ✅ 9 checks were successful, ⏭️ 1 check has been skipped Pull Request URL: #267
Merged
jwbron
added a commit
that referenced
this pull request
Jul 21, 2026
…ads (#271) ## What Closes the one genuine reviewer blind spot from the 2026-07-20 drift triage (#265, follow-up to #269/#270): `retention-unbounded-prune` was missed in all 6 drift samples, and the reviewer's own suggested fix for an adjacent bug recommended the same unbounded-read pattern (`pageSize: "all"`), so no lens was reasoning about memory-bounded reads at all. Two prompt changes: 1. **Correctness lens, line scan**: unbounded reads and accumulation join the named defect classes; a query/fetch/scan that materializes a result set that grows with user data (missing LIMIT, `pageSize: "all"`, loading a whole table to act on a subset, unpaginated buffering). The rule also names the expected shape (page or batch it), so the fix pattern (a bounded read that deliberately processes one batch per invocation) is not itself flagged; that matters because `golden-retention-lifecycle-2/3`'s `PRUNE_BATCH` prune is exactly that shape and must stay clean. 2. **caching-resource specialist**: a matching "No unbounded reads" review rule and an `unbounded-read-materialization` tri-state hunt. The correctness lens is the always-on catch; the specialist adds depth when routed. (Note per review: no ROUTING routes `caching-resource` today, in this repo or webapp, so the correctness-lens rule is the operative change; the specialist half is the documented home for depth once a consuming repo routes storage-heavy paths to it.) ## Measurement Recall-affecting by design. The original acceptance recipe targeted `golden-retention-lifecycle-1:retention-unbounded-prune`, but the corpus stack (#270/#273/#266) retires that spec: the lifecycle-1 facade documents no batch limit, so under a lens-less ruler the defect was ruled not evidenceable in-tree, and the slot now tracks the void-prune unhandled rejection. This PR is exactly the lens that makes the retired row fair again (the lens supplies the norm the tree does not state), so the plan is: 1. Land the corpus stack first, then rebase this PR onto main. 2. Re-add the retired `retention-unbounded-prune` spec to `golden-retention-lifecycle-1` in this PR (spec text is in git history at #270's parent), as the measurement instrument for the lens: without it, no corpus row tracks flagging an *undocumented* unbounded read (#270's review noted the gap). 3. Run the powered recipe: ``` gh workflow run review-eval-ab.yml --ref review-unbounded-read-lens \ -f base_ref=origin/main -f cases=golden-retention-lifecycle-1 -f repeats=10 -f max_usd=30 ``` Success: candidate arm catches `retention-unbounded-prune` at a real rate (baseline is 0/6 in the drift record) with lifecycle-2/3 clean on the bounded `PRUNE_BATCH` prune (no false flags), and no new flags on `mutation-retention-batch-delete-limit`'s already-caught defect beyond the existing catch. Author: jwbron Reviewers: github-actions[bot], jwbron, jeresig, jaredly, somewhatabstract Required Reviewers: Approved By: github-actions[bot], jeresig Checks: ⏭️ 1 check has been skipped, ✅ 9 checks were successful Pull Request URL: #271
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.
Review live A/B: 3 repeats
Baseline:
origin/main(review.md 8e8da9de53bf); candidate: working tree (review.md 8e8da9de53bf).Review live A/B: repeat aggregation
Pooled 3 run(s) per arm from: repeat-1, repeat-2, repeat-3.
Baseline review.md 8e8da9de53bf; candidate 8e8da9de53bf.
Ruler: matcher deterministic+arbiter; corpus 8cc34c5334b8.
Pooled
Noise floor (identical arms: every sample ran the same prompt)
Bands across 6 arm-samples of one review.md; any A/B delta inside a band is indistinguishable from run-to-run wobble. Min/max only widen as samples accumulate; mean +/- sd is the band to track week to week.
WARNING: the samples did not all score the same case set (budget skips or mixed corpora), so these bands fold case-mix variance in on top of run-to-run wobble. Re-run with a budget that clears the full corpus before trusting them.
Adversarial hard gate: PASSED on the candidate arm in every repeat.