Skip to content

review: teach the correctness and caching lenses to flag unbounded reads #271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 2 commits into from
Jul 21, 2026
Merged

review: teach the correctness and caching lenses to flag unbounded reads #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
417c8b0
[review-unbounded-read-lens] review: teach the correctness and cachin…
jwbron Jul 20, 2026
64cfc64
[review-unbounded-read-lens] review: make the bounds question mandato…
jwbron Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/review-unbounded-read-lens.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"review": minor
---

Close the unbounded-read blind spot found by the 2026-07-20 drift triage: the reviewer missed `retention-unbounded-prune` in all 6 drift samples (a query loading an entire user-sized result set via `pageSize: "all"`), and its own suggested fix for an adjacent bug recommended the same unbounded-read pattern, so no lens was reasoning about memory-bounded reads at all. The correctness lens now asks a mandatory bounds question for every query, fetch, or bulk-read call the diff touches (what bounds the result size; pageSize "all", missing LIMIT, fetching a whole set to act on part of it), states the expected shape (page or batch it, so a deliberately bounded per-invocation read is the fix, not a further defect), and explicitly authorizes a second finding in the same statement; the first iteration (a taxonomy item in the defect-class list) measured 0/10 on the targeted powered run because the model reported only the off-by-one in the same query and read "fetch all rows to delete them" as intentional. The caching-resource specialist gains a matching review rule and an `unbounded-read-materialization` tri-state hunt. Recall-affecting by design; priced by the per-PR A/B and a targeted powered run on golden-retention-lifecycle-1.
23 changes: 21 additions & 2 deletions workflows/review/review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1778,8 +1778,21 @@ Do two things in one pass over the files in the list:
timing makes this line wrong? Look for logic errors (off-by-one, inverted
conditions, null/undefined access, races, wrong-but-type-checking code);
security issues (injection, XSS, unsafe deserialization, missing
authz/validation, SSRF, path traversal, committed secrets); and missing tests
for added/changed behavior (except pure docs or formatting).
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

@jeresig jeresig Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incredibly backend specific... we'll want to split this out, for sure!

@jwbron jwbron Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note in my planning doc

touches, ask one more question: what bounds the size of the result it
materializes? A read sized by user data with no bound (`pageSize: "all"`,
a missing LIMIT, fetching an entire set in order to act on part of it, an
unpaginated loop buffering everything before acting) is a finding in its
own right; ask what happens at 100x the data. "The code needs all the
rows to do its job" is the defect restated, not a justification: the
expected shape is to page or batch, so a bounded read that deliberately
processes one batch per invocation is the fix, never a further defect.
Report an unbounded read **even when the same statement carries another
defect**; two defects in one query (say, a wrong offset and an unbounded
page size) are two findings, each anchored at its own line.

**Removed-behavior audit.** Removed (`-`) lines are in scope, not just added
ones. For each removed line (or block), name the invariant it enforced: a
Expand Down Expand Up @@ -2848,6 +2861,9 @@ Skills index for this repo (read only the entries relevant to this lens's domain
- **No unbounded growth.** Caches and in-memory collections have an eviction policy /
size or TTL bound; a request-scoped accumulator is not promoted to unbounded lifetime.
- **No N+1 / accidental resource exhaustion** introduced on a hot path.
- **No unbounded reads.** A query or fetch sized by user data (`pageSize: "all"`,
missing LIMIT, whole-table scans to act on a subset) that materializes the entire
set in memory on a path where the set grows without bound; page or batch it.

### Incident-derived hunts (tri-state)
- **`cache-key-missing-identifier`** — a cached value keyed without a required user/
Expand All @@ -2857,6 +2873,9 @@ Skills index for this repo (read only the entries relevant to this lens's domain
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

@github-actions github-actions Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to caching-resource? If not, the specialist half of the two-layer design never fires on the class that motivated it. (In this repo caching-resource is 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).

@jwbron jwbron Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

result set into memory at once (no limit, no pagination, no batching). `found` when the
set's growth is unbounded and nothing bounds the read.

### Output
Return ONLY the finding-schema JSON object below, under disciplines
Expand Down
Loading