review: teach the correctness and caching lenses to flag unbounded reads - #271
Merged
Conversation
…g lenses to flag unbounded reads
🦋 Changeset detectedLatest commit: 64cfc64 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 |
khan-actions-bot
requested review from
a team,
jaredly and
somewhatabstract
and removed request for
a team
July 20, 2026 17:22
Contributor
Review Guidancegithub-actions (1 file)
Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| cache it feeds. `found` when invalidation is missing. | ||
| - **`unbounded-cache-or-collection`** — a cache/collection with no eviction, TTL, or size | ||
| bound. `found` when growth is unbounded. | ||
| - **`unbounded-read-materialization`**: a read that loads an unbounded, user-data-sized |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): This hunt entry uses a colon separator, but every other tri-state hunt in the file uses a spaced em-dash (e.g. the sibling unbounded-cache-or-collection directly above). Cosmetic only — nothing parses these bullets.
Suggested change
| - **`unbounded-read-materialization`**: a read that loads an unbounded, user-data-sized | |
| - **`unbounded-read-materialization`** — a read that loads an unbounded, user-data-sized |
Lower-confidence observations (4) — surfaced for completeness, non-blocking
workflows/review/review.md:1786— Consider adding the negative-control case (trial-batch-delete-wrapper) to the powered A/B so the widened rule's precision is measured alongside recall. (On validation: that case's defect is a batch-write cap rather than an unbounded read, so it may not be exercised by the new language at all.)workflows/review/review.md:1786— The drift triage named two failure modes (missed detection and a review suggesting the unbounded pattern as a fix); the expected-shape guard lives only in the correctness lens, not in other lenses' suggestion paths.workflows/review/review.md:2858— Does the consuming repo's ROUTING route retention/prune PRs tocaching-resource? If not, the specialist half of the two-layer design never fires on the class that motivated it. (In this repocaching-resourceis not enabled at all.)workflows/review/review.md:2860— The explicit "a bounded batched read is the fix, not a further defect" carve-out lives only in the correctness lens; the caching-resource rule states only the defect shape (its hunt predicate "nothing bounds the read" already implicitly excludes batched reads).
Contributor
Author
There was a problem hiding this comment.
Keeping the colon deliberately (house style here avoids adding em dashes in new text, and nothing parses these bullets). On the lower-confidence items: ROUTING is confirmed not to route caching-resource anywhere today (checked this repo and webapp), so the correctness-lens rule is the operative change; the PR body now says so, and the measurement plan is retargeted for the post-#270 corpus (that stack retires retention-unbounded-prune, and this PR is the lens that makes that row fair again, so the plan is to re-add the spec here after rebasing on the stack and run the powered recipe against lifecycle-1 with the lifecycle-2/3 bounded-prune negative controls).
Contributor
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0c1680e66eae (9 cases).
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. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…ry per read call, allow two findings in one statement
Contributor
Author
|
Acceptance measurement complete, two iterations:
Reports are on the two dispatch runs' artifacts ( |
jeresig
approved these changes
Jul 21, 2026
| authz/validation, SSRF, path traversal, committed secrets); and missing | ||
| tests for added/changed behavior (except pure docs or formatting). | ||
|
|
||
| Additionally, for **every query, fetch, or bulk-read call** the diff |
Member
There was a problem hiding this comment.
This is incredibly backend specific... we'll want to split this out, for sure!
Contributor
Author
There was a problem hiding this comment.
Added a note in my planning doc
jwbron
added a commit
that referenced
this pull request
Jul 21, 2026
> De-scoped 2026-07-21 per review: the swap covers `correctness-reviewer` only. Now based on main (the corpus stack #270/#273 and the drift/budget PRs have landed); the de-scoped roster has its own powered A/B below. ## What Switches the review workflow's `correctness-reviewer` from `claude-opus-4-8` to `claude-fable-5` (effort high, unchanged). Bug-finding recall is the workflow's load-bearing metric, and both the original five-role bundle A/B and the de-scoped re-run concentrate their gain in correctness-adjacent rows. Deliberately NOT moved, per review (jeresig) and per the measurements: - **`claim-validator`** stays Opus at xhigh: the bundle's Fable validator did not improve the precision gate, and the de-scoped re-run confirms the noise increase never came from the validator (it reproduces without it; see below). - **`holistic` / `completeness` / `test-adequacy`** stay Opus: the bundle A/B could not attribute any gain to them, and both known consumers `enable` them in ROUTING, so they price into every full review. - The orchestrator, `thread-reconciler` / `skill-auditor` / `conventions`, and all specialist lenses stay Opus (the lenses deliberately: Fable's cyber safety classifiers can false-positive refuse benign security analysis, and a refused lens is a silent coverage hole). `pattern-triage` stays Sonnet 4.6; `first-principles` already ran Fable 5. Each stay-on-Opus reason is pinned in the agent definition in `review.md`. The weekly drift budget is sized from the de-scoped roster's measured rate (~$1.08/case-arm-run): 180 case-arm-runs ~ $194/week, cap 220. ## The de-scoped measurement (powered A/B) Nine pooled dispatches (runs 29855626692-29855643020), same shard-and-pool structure as the bundle run: 30 live cases x 3 repeats per arm; baseline = all-Opus roster, candidate = this branch. Aggregated with `eval/aggregate.ts`. | Metric | Opus 4.8 | Fable 5 correctness-only | (five-role bundle, superseded) | | --- | --- | --- | --- | | Must-catch recall | 82% (101/123) | **89% (110/123)** | 83% -> 90% | | True misses | 22 | **13** | 21 -> 12 | | Verdict agreement | 96% | 96% | 94% -> 97% | | Noise (unmatched posted) | 43% | 49% | 43% -> 49% | | Judge mean quality | 0.88 | 0.86 | 0.87 -> 0.86 | | Adversarial hard gate | — | PASSED (all shards) | PASSED | | Cost | $72.12 | $97.19 (**+35%**) | +73% | **The de-scope kept the entire recall gain at half the cost delta.** The improved rows are the same previously in-band/floor spec rows as the bundle run (`incident-sql-missing-index:dm-default-backfill` 1/3 -> 3/3, `trial-retention-deletion:erasure-default-limit-one` 0/3 -> 2/3, `:erasure-ignores-delete-helper` 1/3 -> 2/3, `:erasure-prune-error-swallowed` 0/3 -> 1/3, `golden-retention-fix-push:quota-exceeded-never-true` 2/3 -> 3/3, `golden-retention-lifecycle-2:retention-prune-unhandled-rejection` 2/3 -> 3/3), so the bundle's gain is now attributed: it was the correctness reviewer. **What the re-run disambiguated.** The noise increase (43% -> 49%) reproduces without the Fable validator or whole-change reviewers: it is the Fable correctness reviewer posting more companion findings (219 posted vs 178). Likewise the one repeated verdict regression: `golden-retention-lifecycle-3` (expected-clean re-review) 3/3 -> 1/3, where two candidate samples flag the deliberate single-batch `PRUNE_BATCH` prune as blocking. Note both arms in this run predate #271 (merged since), whose correctness-lens language explicitly names the bounded-batch prune as the fix shape, not a defect; the first drift run on merged main will show whether that attenuates this. If not, the corpus-side option is a bounded-prune must-not-flag trap on lifecycle-3. **Statistical honesty.** The pooled +7-point recall delta is z ~ 1.7 (suggestive, not conclusive at 95%; pooled intervals are optimistic per the eval README). As with the bundle run, the per-case pattern is the stronger evidence: six hard rows improved, none regressed on recall, true misses down 41%. The aggregate stamps the expected mixed-corpus-hash warning (three shard groups select different case subsets); within every shard both arms share the same ruler. The superseded five-role bundle measurement (runs 29783522498-29783549087) is preserved in the edit history and summarized in the table's last column. ## Known risks to watch in production - Occasional `refusal` stop on cyber-adjacent diffs hitting `correctness-reviewer` (surfaces as a missing agent result; none observed across ~380 live case-arm-runs to date). The weekly drift corpus is the soft-refusal detector: a silently refusing correctness reviewer craters must-catch recall on the security-adjacent cases. - Author-facing chattiness: the Fable correctness reviewer posts more companion findings (noise 49% vs 43% on the eval's strict matcher). The reviewer-side dedup gap (two comments per defect) is the known follow-up that would absorb most of this. - Over-blocking on expected-clean re-review pushes (lifecycle-3 above); watch the drift series now that #271's lens carve-out is on main. 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: #266
jwbron
added a commit
that referenced
this pull request
Jul 29, 2026
## Summary: A consuming repo may now define .github/aw/review/lenses/<lens>.md for any of the eleven specialist lenses, plus lenses/correctness.md for the always-on correctness-reviewer. Each file is runtime-imported (optional form) into a new 'Repo-specific rules and hunts' section of the matching reviewer prompt, carrying that repo's surface-specific rules and extra tri-state hunts. Lens names stay generic and shared; only payloads vary per repo. Behavior-neutral for every current consumer: no repo carries a payload file yet, and gh-aw's runtime resolves a missing optional import to nothing. correctness-checks.md stays imported as a deprecated alias for lenses/correctness.md (frontend carries one today); a repo should migrate the file and carry at most one of the two. Also in this change: - Eval alignment: resolveRuntimeImports now matches production for the optional form (missing resolves to empty, not the '(not configured for this eval case)' note), so corpus case trees can carry payloads. The required-form fallback note is unchanged and remains the one documented deviation (production fails the run). - disciplines.test.ts pins the payload import in every specialist lens section so the seam cannot be silently dropped. - README documents the lenses/ surface and the three-way contribution rule (shared skeleton vs lens payload vs skills), and fixes two stale claims in the consumer-config section: correctness-checks.md was never documented, and the optional import form was said to have been dropped. First planned payload use: migrating the #271 unbounded-reads hunt from the shared skeleton to server repos' correctness/caching payloads, and frontend's client-surface lenses/security-auth.md. ## Test plan: - `npx vitest run`: 43 files, 1058 tests pass, including the new resolveRuntimeImports optional-form test and the per-lens seam assertion added to disciplines.test.ts (pins the payload import in all eleven lens sections). - `pnpm typecheck` clean; eslint clean on the changed TS files. - Production semantics verified against the pinned gh-aw runtime (github/gh-aw-actions@v0.81.6, setup/js/runtime_import.cjs): a missing optional import inlines nothing and only warns; import paths under .github/ pass its path validation. - Behavior tripwire: this PR touches workflows/review/**, so the per-PR smoke A/B runs; with no payload files in any consumer, the prompt delta is the inert optional-import sections only, so the arms should match. Author: jwbron Reviewers: github-actions[bot], jwbron, jeresig, jaredly, somewhatabstract Required Reviewers: Approved By: github-actions[bot], jeresig Checks: ✅ 10 checks were successful Pull Request URL: #281
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.
What
Closes the one genuine reviewer blind spot from the 2026-07-20 drift triage (#265, follow-up to #269/#270):
retention-unbounded-prunewas 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:
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 becausegolden-retention-lifecycle-2/3'sPRUNE_BATCHprune is exactly that shape and must stay clean.unbounded-read-materializationtri-state hunt. The correctness lens is the always-on catch; the specialist adds depth when routed. (Note per review: no ROUTING routescaching-resourcetoday, 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:retention-unbounded-prunespec togolden-retention-lifecycle-1in this PR (spec text is in git history at review eval: fix floor-case ground truth, mine noise pool, mint mid-band cases #270's parent), as the measurement instrument for the lens: without it, no corpus row tracks flagging an undocumented unbounded read (review eval: fix floor-case ground truth, mine noise pool, mint mid-band cases #270's review noted the gap).Success: candidate arm catches
retention-unbounded-pruneat a real rate (baseline is 0/6 in the drift record) with lifecycle-2/3 clean on the boundedPRUNE_BATCHprune (no false flags), and no new flags onmutation-retention-batch-delete-limit's already-caught defect beyond the existing catch.