review: fix the corpus defects behind the drift run's stable misses - #269
Closed
jwbron wants to merge 1 commit into
Closed
review: fix the corpus defects behind the drift run's stable misses#269jwbron wants to merge 1 commit into
jwbron wants to merge 1 commit into
Conversation
…e drift run's stable misses
🦋 Changeset detectedLatest commit: 44a75b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
khan-actions-bot
requested review from
a team,
jeresig and
kevinb-khan
and removed request for
a team
July 20, 2026 16:59
Contributor
Common patterns6 files: Replaced the fire-and-forget - void pruneNotes(db, note.userId);
+ pruneNotes(db, note.userId).catch(() => {});Excluded from review (5 files)Not individually reviewed — generated, formatting-only, or fully explained by the common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This was referenced Jul 20, 2026
jwbron
added a commit
that referenced
this pull request
Jul 21, 2026
…res and review fixes - Port the drift-run regression fixtures for the dedup-window spec from the parallel triage branch (PR #269): the real test-file coverage finding must match deterministically and the arbiter's false-accepted off-by-one must never match. - Add the (test|coverage).{0,80}dedup mechanism alternate; the drift artifact replays 4/6 deterministic (one sample phrases the gap as 'test does not exercise ... dedup', which the order-sensitive alternates missed). Both pinned fixtures still hold. - Widen the batch-delete-limit mustCatch window to cover the fire-and-forget .catch line: it is part of the defect's causal chain, and mustCatch claims run before the swallow trap, so a correct diagnosis anchored there scores as a catch instead of a false flag. - Correct the drift budget sizing comment: 23 live cases (both new mutation cases are live-tagged), 138 case-arm-runs ~$113, and note the thin headroom against the budget-skewed per-run mean. - Lead the new cases' tags with synthetic-mutation to match siblings.
Contributor
Author
|
Closing in favor of the #270 stack: this PR and #270 are two triages of the same drift run that reached conflicting ground truths for the same files. Both fix lifecycle-3's This PR's unique contributions were ported to #270 rather than lost:
The unbounded-prune spec this branch preserved is planned to return via #271 (the lens change that makes it a fair row); see that PR's measurement plan. |
jwbron
added a commit
that referenced
this pull request
Jul 21, 2026
…lse accepts (#272) ## What The tuning memo's item 7 (calibrate the match arbiter's false-positive rate), seeded from production evidence instead of synthetic pairs. The 2026-07-20 drift run (29724668102) recorded 10 fallback accepts; hand-auditing them for the stable-miss triage found **4 of the 10 were wrong**: - 3x: the cap off-by-one finding at `retention.ts:38` accepted for `retention-dedup-window-untested` - 1x: the unreachable-quotaExceeded finding accepted for `quota-cache-shared-key` (previously unnoticed; found while labeling this set) All four share the same shape: same file, plausible adjacency, different defect; exactly the class the prompt's refuse bias ("not merely a nearby, related, or different issue in the same file") was supposed to prevent. Each false accept silently inflates must-catch recall, the load-bearing metric. ## Contents - `eval/arbiter-calibration.json`: the 10 recorded decision points, verbatim finding text and spec mechanisms, hand labels (`match`/`mismatch`) with per-pair rationale. Not under `corpus/` (the loader parses every JSON there as a case). - `eval/arbiter-calibration.ts`: replays the pairs through the real arbiter seam (`haikuMatchArbiter`) and reports per-pair yes-rates plus false-accept/false-reject rates. Live run costs ~$0.02 at the default 3 samples/pair: `ANTHROPIC_API_KEY=... pnpm dlx tsx workflows/review/eval/arbiter-calibration.ts` - `eval/arbiter-calibration.test.ts`: deterministic invariants; 10 pairs, exactly 4 mismatches, no pair matches deterministically (else it would never reach the fallback), fixtures drive the real prompt builder. ## Deliberately not here The arbiter prompt is unchanged. Production behavior on this set was 100% yes by construction; the harness measures whether that reproduces and at what rate, and a prompt fix (e.g. requiring the arbiter to quote which mechanism alternate the comment satisfies) should be justified by these numbers, land as a separate PR, and be treated as a ruler change (provenance re-stamp plus noise-floor re-baseline). Note the rev-4 memo decision: the calibration set is also the instrument for deciding the arbiter's model tier. Note: after #269, the dedup-window spec matches its real finding deterministically (altLocations), which shrinks this spec's arbiter exposure but does not fix the arbiter; the false-accept class is generic, as the quota pair shows. Author: jwbron Reviewers: github-actions[bot], jwbron, jeresig, kevinb-khan Required Reviewers: Approved By: github-actions[bot], jeresig Checks: ✅ 10 checks were successful Pull Request URL: #272
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.
What
Corpus fixes for three of the four stable misses in the 2026-07-20 drift report (#265). The triage (replaying the run 29724668102 artifact against the specs and the deterministic matcher) found that only one of the four was a genuine reviewer miss; the other three were defects in the corpus itself. This is a deliberate ruler change: catch rates will move because the ground truth moved, not the reviewer. The corpus sha stamp marks the break; see "Measurement" below.
1. Lifecycle trees:
void pruneNotes(...)becomespruneNotes(...).catch(() => {})golden-retention-lifecycle-3expects a 0-comment APPROVE and documents the fire-and-forget prune as a deliberate must-not-flag trap, but the trap was mis-specified:voiddoes not deliver the comment's own contract ("a failing prune must not fail the save"); it leaves an unhandled promise rejection. The reviewer's blocking flag on that line was technically correct and fired in every one of the 6 drift samples, forcing the wrong verdict on lifecycle-3 and injecting one guaranteed noise finding into every lifecycle-1 and lifecycle-2 run. The fix (applied to all three lifecycle trees, their diffs, and the embedded prior-push diffs, line counts preserved) makes the trap sound while keeping its intent: a documented deliberate pattern the reviewer must not flag. The trap spec gains a "swallow/empty catch" mechanism alternate so the must-not-flag guard still recognizes the new phrasing.2.
retention-dedup-window-untestedspec: altLocation + mechanism alternatesReported 3/6, wrong in both directions. All 3 recorded catches were Haiku-arbiter false accepts (each was the unrelated cap off-by-one finding at retention.ts:38, recorded
via: "fallback"); meanwhile the reviewer surfaced the real dedup-coverage gap in 4 of 6 samples, anchored onretention.test.ts, which the spec's single path made ineligible for both deterministic and fallback matching. The spec now carries an altLocation on the test file plus two mechanism alternates covering the observed phrasing. Replayed deterministically over the recorded artifact: 0/6 as written, 4/6 fixed, with the prefix-collision spec still claiming its own finding first in all samples.Both recorded findings are pinned as regression fixtures in
live-match.test.ts: the real coverage finding must match deterministically, and the off-by-one finding the arbiter wrongly accepted must not match, guarding the widened mechanism against over-matching. The three recorded false accepts are also direct evidence that the arbiter's false-positive rate is uncalibrated; auditing its prompt is follow-up work, not attempted here.3.
trial-dedup-eventual-consistency: expected verdict APPROVE -> REQUEST_CHANGESThe case was internally inconsistent: it requires catching a defect that defeats dedup for exactly the retry/double-submit traffic the feature exists for, and both drift samples that scored it labeled the catch blocking, which mechanically forces REQUEST_CHANGES; the expected APPROVE was unreachable for a compliant reviewer. The recorded fixture moves to severity blocking at confidence 0.8, matching the trial-amplified-default-limit pattern, so the deterministic replay computes the expected verdict.
Not fixed here:
retention-unbounded-prune(0/6)The one genuine reviewer miss. No sample ever flags the prune query loading the entire stale set (
pageSize: "all"), and the reviewer's suggested fix for a different bug itself recommends the unbounded-read pattern. That is a review.md gap (no lens reasons about memory-bounded reads), left for a prompt change that the per-PR A/B can price.Measurement
Expected effects on the next drift run, all corpus-side: lifecycle-3 verdict agreement recoverable to in-band, dedup-window catch rate to roughly 4/6 with
via: "deterministic", noise down by one guaranteed finding per lifecycle-1/2 run, trial-dedup verdict agreement to in-band. This PR changes the corpus content hash, so the aggregate will (correctly) refuse to treat pre- and post-fix runs as one pool and warns on mixed rulers. Plan per the drift-report discussion: land this, then one manual drift dispatch re-baselines the noise-floor bands and starts the new series segment.Testing
pnpm typecheckand prettier clean.