-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||
| 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 | ||||||
|
|
@@ -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/ | ||||||
|
|
@@ -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 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
Lower-confidence observations (4) — surfaced for completeness, non-blocking
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
|
|
||||||
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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