From 417c8b0046f7f02dff6681f3b1175fc935d807df Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Mon, 20 Jul 2026 10:21:21 -0700 Subject: [PATCH 1/2] [review-unbounded-read-lens] review: teach the correctness and caching lenses to flag unbounded reads --- .changeset/review-unbounded-read-lens.md | 5 +++++ workflows/review/review.md | 17 +++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 .changeset/review-unbounded-read-lens.md diff --git a/.changeset/review-unbounded-read-lens.md b/.changeset/review-unbounded-read-lens.md new file mode 100644 index 00000000..bdf7e8a8 --- /dev/null +++ b/.changeset/review-unbounded-read-lens.md @@ -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's line scan now names unbounded reads and accumulation as a defect class (materializing a result set that grows with user data; missing LIMIT, whole-table reads to act on a subset, unpaginated buffering) and states the expected shape (page or batch it, so a deliberately bounded per-invocation read is the fix, not a further defect). 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. diff --git a/workflows/review/review.md b/workflows/review/review.md index 3c7f522f..287371fd 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -1778,8 +1778,15 @@ 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); unbounded reads + and accumulation (a query, fetch, or scan that materializes an entire result + set whose size grows with user data: a `pageSize: "all"` or missing-LIMIT + read, loading a whole table to act on part of it, an unpaginated loop + buffering everything before writing; ask what happens at 100x the data, and + treat "page or batch it" as the expected shape, so a bounded read that + deliberately processes one batch per invocation is the fix, not a further + defect); and missing tests for added/changed behavior (except pure docs or + formatting). **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 +2855,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 +2867,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 + 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 From 64cfc64a08870b593fe0f2262f5f6f438ff83fb3 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Mon, 20 Jul 2026 11:25:56 -0700 Subject: [PATCH 2/2] [review-unbounded-read-lens] review: make the bounds question mandatory per read call, allow two findings in one statement --- .changeset/review-unbounded-read-lens.md | 2 +- workflows/review/review.md | 24 +++++++++++++++--------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/.changeset/review-unbounded-read-lens.md b/.changeset/review-unbounded-read-lens.md index bdf7e8a8..da733fac 100644 --- a/.changeset/review-unbounded-read-lens.md +++ b/.changeset/review-unbounded-read-lens.md @@ -2,4 +2,4 @@ "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's line scan now names unbounded reads and accumulation as a defect class (materializing a result set that grows with user data; missing LIMIT, whole-table reads to act on a subset, unpaginated buffering) and states the expected shape (page or batch it, so a deliberately bounded per-invocation read is the fix, not a further defect). 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. +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. diff --git a/workflows/review/review.md b/workflows/review/review.md index 287371fd..6bd6b562 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -1778,15 +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); unbounded reads - and accumulation (a query, fetch, or scan that materializes an entire result - set whose size grows with user data: a `pageSize: "all"` or missing-LIMIT - read, loading a whole table to act on part of it, an unpaginated loop - buffering everything before writing; ask what happens at 100x the data, and - treat "page or batch it" as the expected shape, so a bounded read that - deliberately processes one batch per invocation is the fix, not a further - defect); 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