From a5a44df1ecba4b0afae353ea6df265060120ba0d Mon Sep 17 00:00:00 2001 From: Irving Popovetsky Date: Thu, 29 Oct 2026 12:30:10 -0700 Subject: [PATCH 1/3] Raise npm response cap to 80 MiB, add byte budget and distinct lookup failure reasons (K-18) vite's full packument (37.2 MiB) was over the 32 MiB cap and gave a false hard block on front-end PR 2027. Collect now logs each failed lookup. Co-Authored-By: Claude Sonnet 5.5 --- docs/claude-review-design.md | 6 +- review/src/oc_review/collect.py | 69 ++++++++++++++++++-- review/src/oc_review/deps.py | 11 +++- review/tests/unit/test_collect.py | 105 +++++++++++++++++++++++++++++- review/tests/unit/test_deps.py | 19 +++++- 5 files changed, 198 insertions(+), 12 deletions(-) diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index ee3cda7..49d192a 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -286,12 +286,13 @@ The review covers one immutable comparison pair: the merge base commit and the h - **Supported versions:** `package-lock.json` lockfile v2 and v3; `yarn.lock` v1 and berry metadata versions 4 to 8; `pnpm-lock.yaml` 6.0, 6.1 and 9.0, in each document of a two-document file; `poetry.lock` 1.1, 2.0 and 2.1; `uv.lock` version 1. Any other version is unknown. Pip `-r` and `-c` includes are not followed, and an unknown pip option is a parse failure. `npm-shrinkwrap.json` is not parsed; it is an unsupported dependency file. - **Delta:** parse each changed supported file at the merge base and at the head (both from blobs by SHA) and compare. For each added or changed package: name, old and new version, resolved source (normalized), registry publish time of the new version, `hasInstallScript` (npm lockfile v2 and v3), and for npm direct deps the install lifecycle scripts from the registry manifest (`preinstall`, `install`, `postinstall`, `prepare`, `preprepare`, `postprepare`, `prepublish`). - **Alternatives:** a `pyproject.toml` dependency can list alternatives, split by markers: a list under `[tool.uv.sources]`, or a list of Poetry constraints. The code reads every alternative, whatever its marker, as its own package with its own version and source, so each one gets the source check and the 72-hour rule. A uv alternative with no known source key, or an empty Poetry list, is a parse failure. -- **Registry lookups:** only fixed routes built from validated names: `https://registry.npmjs.org/` and `https://pypi.org/pypi//json`. No redirects to other hosts, a response byte cap, and a timeout. Never fetch a URL from the PR, including a lockfile `resolved` field. The code refuses every redirect. Caps: 16 MiB for one dependency file, 32 MiB for an npm response (the full packument, for per-version publish times), 16 MiB for a PyPI response. Each lookup has a 20-second timeout and at most 2 retries, for 429, 5xx and network errors only. +- **Registry lookups:** only fixed routes built from validated names: `https://registry.npmjs.org/` and `https://pypi.org/pypi//json`. No redirects to other hosts, a response byte cap, and a timeout. Never fetch a URL from the PR, including a lockfile `resolved` field. The code refuses every redirect. Caps: 16 MiB for one dependency file, 80 MiB for an npm response (the full packument, for per-version publish times; K-18), 16 MiB for a PyPI response. Each lookup has a 20-second timeout and at most 2 retries, for 429, 5xx and network errors only. - **Lookup limits per run** (K-11): a large lockfile, or a migration whose base counts as empty (K-10), can need thousands of lookups. One by one they outlast the 10-minute job timeout, and the run fails with no bundle and no reason. So collect deduplicates the lookups by registry key (ecosystem, name, version) before any starts, and limits them with three constants in `collect.py`: - `REGISTRY_LOOKUP_CONCURRENCY = 8`: at most 8 packages are looked up at once. The versions of one package run in order on one worker, so its packument is fetched once. This is safe because lookups go only to the two fixed registry hosts. - `REGISTRY_LOOKUP_BUDGET_SECONDS = 240`: total wall-clock time for all lookups, well under the job timeout, so collect still has time to build and write the bundle. A fetch never gets more time than is left, and no fetch or retry starts after the deadline. Collect does not wait for a lookup that is still running at the deadline. - `MAX_REGISTRY_LOOKUPS = 3,000`: unique keys per run, in sorted order. A typical app lockfile has 1,000 to 2,000 unique names. - A key that is past the count cap or has no result at the deadline is a failed lookup, with the reason "registry lookup failed: over the lookup count cap" or "registry lookup failed: lookup time budget exceeded". The data is incomplete, so the hard block applies, and collect still writes the bundle. + - `MAX_REGISTRY_BYTES = 1 GiB` (K-18): response bytes for all lookups of a run. Each fetch reserves its response cap, or what is left of this budget, before it starts, so received plus reserved bytes never pass it. + A key that is past the count cap or has no result at the deadline is a failed lookup, with the reason "registry lookup failed: over the lookup count cap" or "registry lookup failed: lookup time budget exceeded". A key past the byte budget has "registry lookup failed: over the download byte budget". A response over its size cap has "registry lookup failed: response over the size cap". Any other failure keeps "registry lookup failed". A file with several reasons shows one, in that order: time budget, count cap, byte budget, size cap, other. The data is incomplete, so the hard block applies, and collect still writes the bundle. Collect also logs one line per failed lookup to the job log: `collect: lookup failed for '' '': : `. The reason is cut to 160 characters and escaped, and never holds a response body. - **Official sources:** `registry.npmjs.org` and `registry.yarnpkg.com` for npm, `pypi.org` and `files.pythonhosted.org` for PyPI, over https with no port and no userinfo, and only for the ecosystem of the entry. An official npm tarball URL must have the path of that package and version. A lockfile entry with no resolved source is an unknown source (foreign), unless it is a workspace link or a bundled dependency. Local sources (file, workspace, bundled, patch) are not foreign. - **Failures:** a supported file at the head that does not parse, an unknown lockfile version at the head, or a registry lookup that still fails after retries or is not done within the lookup limits is the hard block "incomplete dependency data". It clears on a re-run when the cause is gone. Each file gets at most one failure entry. - **Merge base that does not parse** (K-10): a supported file whose merge-base side does not parse or has an unknown version (for example an npm `lockfileVersion: 1` lockfile that the PR migrates to v3) does not block by itself. Its base side counts as empty: every head entry counts as added and gets every check (source, registry lookup, 72-hour rule), under the same per-lookup caps and run limits (`REGISTRY_LOOKUP_CONCURRENCY`, `REGISTRY_LOOKUP_BUDGET_SECONDS`, `MAX_REGISTRY_LOOKUPS`, K-11). So an entry that was already at the merge base counts as added: an existing git or tarball dependency, or any other foreign source, in that file now blocks. If a lookup fails, goes over a byte cap, is past the count cap or has no result within the time budget, or the head side does not parse, the data is incomplete and the hard block applies. A deleted file with such a base has no head entries, so it does not block. The base failure is a signal (context only). A base blob that could not be read is still the hard block. A base symlink or submodule holds no dependency data and counts as absent. @@ -990,6 +991,7 @@ P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, - P3-7. Release M4, the comment redesign (owner and coordinator, 2026-10-28). M4 is `9e2795b97b22869b9e5c7a777e71a2afa8366eeb` on main (code from PR 12 `a410ddd`, docs PR 13). There is no separate candidate branch: the merge SHA was validated directly. Unit tests: `test.yml` at M4, [run 40710532524](https://github.com/OperationCode/.github/actions/runs/40710532524), success. Release evals: `eval.yml` at M4, [run 40713029193](https://github.com/OperationCode/.github/actions/runs/40713029193), 13 of 13 matched, success; the coordinator approved the run under the owner's delegation through phase 4. Console: the owner edited R1 `claude-review-prod-r1` (`fdrl_01N2ZktTiVKqUvvXwywBFfys`) so that `job_workflow_ref` is `OperationCode/.github/.github/workflows/claude-review.yml@9e2795b97b22869b9e5c7a777e71a2afa8366eeb`. The owner reported "rule updated" and shared a screenshot of the rules list; the rule detail value was not read back. TR (org ruleset 25835887) moved from M3 `2c63f49b461feac74c2c70c4f86ca90040df6a0a` to M4; read back: `repository_id` 1432342572, path `.github/workflows/claude-review.yml`, `sha` `9e2795b`, no `ref`, bypass list empty, enforcement active. Live checks in claude-review-test: PR 28 reopened, [run 40713506554](https://github.com/OperationCode/claude-review-test/actions/runs/40713506554) at M4, success, all four jobs `ok`, summary comment in the new layout (green PASS heading, TIP callout, checks strip). PR 40 reopened, [run 40713856249](https://github.com/OperationCode/claude-review-test/actions/runs/40713856249), failure, red FAIL heading, CAUTION callout, the hard block row (bidi) first, merged duplicate rows with "Found by" listing both passes. Both PRs were closed again. Console rules in the owner's screenshot: `claude-review-p3-r2`, `claude-review-prod-r1`, `eval-federation-rule`, `claude-reviewer-test`; the coordinator advised archiving `claude-review-p3-r2`. Deviation 1: no new federation rule for this release; R1 was edited in place, because the merged code names R1 (same as the phase 3 release, where R1 moved from S1 to M3). Cost: M3 can no longer get a token, so a rollback to M3 needs another Console edit. Deviation 2: the denied exchange at M4 before the Console edit (release step 5a) was NOT RUN, because the owner made the edit before the check. M4 is the current production SHA and the current pin. - K-16. pnpm 12 lockfiles with two YAML documents (ruling on front-end PR 2027, 2026-10-29). pnpm 12 writes `pnpm-lock.yaml` as two YAML documents: first the env lockfile (one importer `.` with `configDependencies` and `packageManagerDependencies`, and the `packages` and `snapshots` of pnpm itself), then the main lockfile. The parser read one document only, so every front-end PR that changed the lockfile got the false hard block "incomplete dependency data" (does not parse), at the head and at the merge base. The parser now reads every document with the same safe loader that refuses duplicate keys. The loader expands YAML merge keys before that check, so a merged key that repeats another key in the same mapping is a parse failure too, in every YAML lockfile. It accepts one document, as before, or exactly two in that order. Each document must have a supported `lockfileVersion`. The first may hold only `lockfileVersion`, `importers`, `packages` and `snapshots`, and its only importer is `.` with only the two env keys. An env `packages` entry with its own `name` or `version` must match its key, and each env dependency must be a package that the parser emits from that document, so no env dependency can skip a check under another identity. The second may hold only the top-level keys of pnpm 12's main lockfile: `lockfileVersion`, `settings`, `catalogs`, `overrides`, `packageExtensionsChecksum`, `pnpmfileChecksum`, `ignoredOptionalDependencies`, `patchedDependencies`, `importers`, `packages`, `snapshots` and `time`. A one-document file keeps the old check, which ignores other keys. Packages of both documents go through every dependency check; pnpm and its platform binaries are dependencies too, and an env dependency counts as direct. Any other document count, an empty document, another env or main shape, or a bare document after a `...` end marker with no `---` is still the hard block. A malformed two-document merge-base file counts as empty, as in K-10. The size gate is unchanged. - K-17. Anchored patch for long files changed near both ends (pilot false skip, 2026-10-29). PR 2027 in `OperationCode/front-end` (2 files, `package.json` and a `pnpm-lock.yaml` of about 16,000 lines, GitHub +1,131/-1,153) got "too large for automatic review", so a routine dependency update got no model review. Cause: the changes span both ends of the lockfile, so its middle is about 16,000 by 16,000 lines, past `DIFF_BUDGET` (25,000,000). The patch fell back to one hunk with every base line removed and every head line added (+16,069/-16,091). Rendered with line numbers, the bundle estimated 451,544 tokens against the 150,000 limit. A plain difflib diff of that middle needs about 82 million counted steps, past the 5 million per-file budget, so raising `DIFF_BUDGET` alone does not help. Now collect splits such a middle at lines that occur once on each side and diffs only the gaps, within the same budgets ("Coverage limits"). Result for PR 2027 at the same blobs: +1,127/-1,149, estimate 93,786 tokens with the main-branch parser (the head lockfile does not parse there, so the delta is small), 115,961 with the K-16 parser and registry data for all 229 changed packages. Rejected: (b) counting lockfiles at a fine-diff or capped size for the estimate, because the gate must measure what Claude is sent: Claude would still get the 450k-token rendering, and a path rule would let a PR shrink the estimate of any content by its file name. (c) Sending a code-built package delta instead of the lockfile patch: it needs a path or parse rule to pick the files, so content in a lockfile-named file could be hidden from the model passes, and the supply-chain pass would lose the raw lines it reads today. The anchored patch reads no path, never drops a changed line, and leaves the size gate fail-closed: a full rewrite has no anchors and is still one hunk. It changes no gate rule and lowers no risk. +- K-18. npm response cap and lookup failure visibility (pilot false hard block, front-end PR 2027, 2026-10-29). `vite` 8.3.5 is a real, old enough version, but its full packument is 38,999,531 bytes (37.2 MiB), past the 32 MiB cap, so the lookup failed and the PR got the hard blocks "registry lookup failed" and "no registry data for a new version". Collect hid the cause: every lookup error became the plain reason and no log line named it. Measured full packuments on 2026-10-29 with the code's `Accept: application/json` (bytes): `vite` 38,999,531; `next` 31,483,965 (1.1 MiB under the old cap); `typescript` 15,865,004; `@types/node` 11,183,996; `aws-sdk` 10,595,791; `react-dom` 9,584,246; `webpack` 5,246,798; `electron` 4,301,537; `@aws-sdk/client-s3` 4,251,556; `@types/react` 2,642,195; `eslint` 2,099,986; `@babel/core` 810,569. Now `NPM_MAX_RESPONSE_BYTES` is 80 MiB (2.15x the largest). The body is still read in 64 KiB chunks and never past the cap plus one byte, and a response over the cap still fails closed. Aggregate check: 8 workers run at once and a parsed packument takes about 10x its body in memory (vite: 373 MB peak for the 39 MB body), so the worst case is 8 x 80 MiB x 10, about 6.4 GiB, only if 8 packuments sit at the cap at once; the 8 largest real packuments total about 137 MB, about 1.4 GB parsed. A hosted runner has 16 GB (7 GB on a private repo), and the per-response cap is the memory bound. Time is bounded by the 240 s budget and the 20 s fetch timeout, both unchanged. New `MAX_REGISTRY_BYTES = 1 GiB` bounds download volume per run: each fetch reserves its cap or the rest of the budget first, and a key past it fails with its own reason. Failure reasons are now distinct for "response over the size cap" and "over the download byte budget", and each failed lookup logs its exception class and a short, escaped reason, never a body. The prefilter still reports one failed lookup twice, deliberately: the file row ("dependency data incomplete: ") names the cause, and the per-package row ("no registry data for a new version") names each package the 72-hour rule could not check; merging them would drop one of the two. No gate rule changes, and nothing lowers risk. P4: phase 4 pilot start, 2026-10-28 and 2026-10-29 (owner and coordinator). The pilot ruleset was created and enabled on 2026-10-29. Where a P4- line changes an earlier line, it wins. diff --git a/review/src/oc_review/collect.py b/review/src/oc_review/collect.py index 25f7c09..ee9bbef 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -90,12 +90,25 @@ REGISTRY_LOOKUP_BUDGET_SECONDS = 240.0 # Unique keys looked up per run; past the cap a key fails without a lookup. MAX_REGISTRY_LOOKUPS = 3000 +# Response bytes for all lookups of a run (K-18). Each fetch reserves its cap +# (or what is left) before it starts, so reserved plus received bytes never +# pass this. It bounds download volume; memory per worker is bounded by the +# per-response cap (a parsed packument is about 10x its body). +MAX_REGISTRY_BYTES = 1024 * 1024 * 1024 # Fixed failure reasons ("incomplete dependency data"), in the order a file # with several picks its one reason. LOOKUP_BUDGET_EXCEEDED = "registry lookup failed: lookup time budget exceeded" LOOKUP_COUNT_CAP = "registry lookup failed: over the lookup count cap" +LOOKUP_BYTES_BUDGET = "registry lookup failed: over the download byte budget" +LOOKUP_RESPONSE_TOO_LARGE = "registry lookup failed: response over the size cap" LOOKUP_FAILED = "registry lookup failed" -_LOOKUP_REASON_ORDER = (LOOKUP_BUDGET_EXCEEDED, LOOKUP_COUNT_CAP, LOOKUP_FAILED) +_LOOKUP_REASON_ORDER = ( + LOOKUP_BUDGET_EXCEEDED, + LOOKUP_COUNT_CAP, + LOOKUP_BYTES_BUDGET, + LOOKUP_RESPONSE_TOO_LARGE, + LOOKUP_FAILED, +) SIZE_GATE_TOKENS = 150_000 CHARS_PER_TOKEN = 4 @@ -622,7 +635,25 @@ class _BudgetSpent(deps.RegistryError): """No time is left in the run's lookup budget for a fetch or a retry.""" +class _BytesSpent(deps.RegistryError): + """No bytes are left in the run's download budget for a fetch.""" + + _LOOKUP_WAIT_SLICE_SECONDS = 0.05 +_LOG_REASON_CHARS = 160 + + +def _log_lookup_failure(key: RegistryKey, exc: BaseException) -> None: + """One job-log line per failed lookup: key, exception class and a short + reason. Never a response body; the text is escaped and cut, so it cannot + start a workflow command.""" + reason = str(exc).encode("ascii", "backslashreplace").decode("ascii") + reason = "".join(ch if ch.isprintable() else "?" for ch in reason)[:_LOG_REASON_CHARS] + ecosystem, name, version = key + print( + f"collect: lookup failed for {ecosystem} {name!r} {version!r}: {type(exc).__name__}: {reason}", + file=sys.stderr, + ) def lookup_registry( @@ -634,6 +665,7 @@ def lookup_registry( budget_seconds: float = REGISTRY_LOOKUP_BUDGET_SECONDS, concurrency: int = REGISTRY_LOOKUP_CONCURRENCY, max_lookups: int = MAX_REGISTRY_LOOKUPS, + byte_budget: int = MAX_REGISTRY_BYTES, ) -> tuple[dict[RegistryKey, deps.RegistryInfo], dict[RegistryKey, str]]: """Registry data by key, and a fixed failure reason by key for every key that has no data (ruling R-LOOKUPS, K-11). @@ -645,7 +677,10 @@ def lookup_registry( `budget_seconds` of the start: a fetch never gets more time than is left, no lookup, fetch or retry starts after the deadline, and a result that completes at or after the deadline is not taken. A key with no result at - the deadline fails. Any error in a lookup fails that key only. + the deadline fails. Any error in a lookup fails that key only, with a + reason that tells a response over its size cap, and a run past its + `byte_budget` download budget, from other failures (K-18); each failure + is logged with its exception class. Collect does not wait for a lookup past the deadline. The workers are daemon threads, so a hung lookup also does not hold the process open @@ -663,15 +698,32 @@ def lookup_registry( results: dict[RegistryKey, deps.RegistryInfo | str] = {} finished = 0 # groups whose worker is done with them closed = False # set at the deadline or when all groups finish + reserved = 0 # response bytes received or reserved by fetches in flight def remaining() -> float: return deadline - clock() def bounded_fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + nonlocal reserved left = remaining() if left <= 0: raise _BudgetSpent("lookup time budget exceeded") - return fetch(url, max_bytes, min(timeout, left)) + with state: + room = byte_budget - reserved + if room <= 0: + raise _BytesSpent("download byte budget exceeded") + grant = min(max_bytes, room) + reserved += grant + got = grant # a failed fetch keeps its whole reservation + try: + result = fetch(url, grant, min(timeout, left)) + got = len(result.body) + finally: + with state: + reserved -= grant - got + if grant < max_bytes and got > grant: + raise _BytesSpent("download byte budget exceeded") + return result def bounded_sleep(seconds: float) -> None: if seconds >= remaining(): @@ -702,7 +754,14 @@ def run_group(group: list[RegistryKey]) -> None: result = lookup(name, version, cached, sleep=bounded_sleep) except _BudgetSpent: result = LOOKUP_BUDGET_EXCEEDED - except Exception: # noqa: BLE001 - any error is a failed lookup, never a lost bundle + except _BytesSpent as exc: + _log_lookup_failure(key, exc) + result = LOOKUP_BYTES_BUDGET + except deps.ResponseTooLargeError as exc: + _log_lookup_failure(key, exc) + result = LOOKUP_RESPONSE_TOO_LARGE + except Exception as exc: # noqa: BLE001 - any error is a failed lookup, never a lost bundle + _log_lookup_failure(key, exc) result = LOOKUP_FAILED if not commit(key, result): return @@ -754,6 +813,7 @@ def dependency_facts( budget_seconds: float = REGISTRY_LOOKUP_BUDGET_SECONDS, concurrency: int = REGISTRY_LOOKUP_CONCURRENCY, max_lookups: int = MAX_REGISTRY_LOOKUPS, + byte_budget: int = MAX_REGISTRY_BYTES, ) -> prefilter.DependencyFacts: """Parse each changed supported file at the merge base and the head, and look up every new exact registry version (`prefilter.registry_key`). @@ -822,6 +882,7 @@ def dependency_facts( budget_seconds=budget_seconds, concurrency=concurrency, max_lookups=max_lookups, + byte_budget=byte_budget, ) # One fixed reason per file, the run-limit reason first; the delta shows # which entries have no registry data. diff --git a/review/src/oc_review/deps.py b/review/src/oc_review/deps.py index 2e0944f..530f494 100644 --- a/review/src/oc_review/deps.py +++ b/review/src/oc_review/deps.py @@ -36,8 +36,9 @@ # Byte cap on one manifest or lockfile, applied before parsing. MAX_DEP_FILE_BYTES = 16 * 1024 * 1024 -# Byte caps on registry responses, applied before parsing. -NPM_MAX_RESPONSE_BYTES = 32 * 1024 * 1024 +# Byte caps on registry responses, applied before parsing. The npm cap is +# 2x the largest full packument measured on 2026-10-29 (vite, 37.2 MiB), K-18. +NPM_MAX_RESPONSE_BYTES = 80 * 1024 * 1024 PYPI_MAX_RESPONSE_BYTES = 16 * 1024 * 1024 REGISTRY_TIMEOUT_SECONDS = 20.0 REGISTRY_RETRIES = 2 @@ -127,6 +128,10 @@ class RegistryError(DepsError): """A registry lookup failed or its response did not validate.""" +class ResponseTooLargeError(RegistryError): + """A registry response is over its byte cap.""" + + # --------------------------------------------------------------------------- # Data model # --------------------------------------------------------------------------- @@ -1573,7 +1578,7 @@ def _fetch_json( if result.status != 200: raise RegistryError(f"{url}: HTTP {result.status}") if len(result.body) > max_bytes: - raise RegistryError(f"{url}: response is over the {max_bytes} byte cap") + raise ResponseTooLargeError(f"{url}: response is over the {max_bytes} byte cap") try: doc = json.loads(result.body) except (ValueError, RecursionError) as exc: diff --git a/review/tests/unit/test_collect.py b/review/tests/unit/test_collect.py index ca85f9b..f056fb1 100644 --- a/review/tests/unit/test_collect.py +++ b/review/tests/unit/test_collect.py @@ -1074,11 +1074,11 @@ def test_lockfile_migration_over_a_lookup_cap_is_a_hard_block(monkeypatch): monkeypatch.setattr(deps, "NPM_MAX_RESPONSE_BYTES", 2048) col, _, _ = run(_migration(registry=big)) assert _dep_blocks(col) == { - ("package-lock.json", "dependency data incomplete: registry lookup failed"), + ("package-lock.json", "dependency data incomplete: registry lookup failed: response over the size cap"), ("package-lock.json", "no registry data for a new version"), } assert col.envelope["payload"]["dependency_delta"]["failures"] == [ - {"path": "package-lock.json", "reason": "registry lookup failed"} + {"path": "package-lock.json", "reason": c.LOOKUP_RESPONSE_TOO_LARGE} ] @@ -1182,6 +1182,107 @@ def test_lookup_limit_constants(): assert c.REGISTRY_LOOKUP_BUDGET_SECONDS <= 600 * 0.4 +def _sized_packument(name: str, size: int) -> bytes: + doc = {"name": name, "time": {"1.0.0": "2026-01-01T00:00:00Z"}, "versions": {"1.0.0": {}}, "pad": ""} + pad = size - len(json.dumps(doc).encode()) + doc["pad"] = "x" * pad + body = json.dumps(doc).encode() + assert len(body) == size + return body + + +def test_npm_cap_fits_the_largest_measured_sized_packument(): + # vite on 2026-10-29: 38,999,531 bytes; the cap keeps 2x headroom (K-18). + assert deps.NPM_MAX_RESPONSE_BYTES >= 2 * 38_999_531 + + +def test_packument_just_under_and_over_the_cap_through_the_default_fetch_path(): + cap = 4096 + + def fetch_of(size: int): + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + # What default_fetch does: read at most max_bytes + 1. + return deps.FetchResult(200, _sized_packument("a", size)[: max_bytes + 1] if size > max_bytes else _sized_packument("a", size), url) + return fetch + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(deps, "NPM_MAX_RESPONSE_BYTES", cap) + registry, failed = c.lookup_registry(_keys("a"), fetch=fetch_of(cap), sleep=_no_sleep, clock=FakeClock()) + assert failed == {} and list(registry) == _keys("a") + registry, failed = c.lookup_registry(_keys("a"), fetch=fetch_of(cap + 1), sleep=_no_sleep, clock=FakeClock()) + assert registry == {} and failed == {("npm", "a", "1.0.0"): c.LOOKUP_RESPONSE_TOO_LARGE} + + +def test_byte_budget_fails_closed_with_its_own_reason(): + body = _sized_packument("a", 1000) + + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + return deps.FetchResult(200, body.replace(b'"a"', b'"%s"' % url.rsplit("/", 1)[1].encode(), 1), url) + + # 2500 bytes: the cap reservation (80 MiB) is cut to what is left, and + # the third response does not fit. + registry, failed = c.lookup_registry( + _keys("a", "b", "c"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=1, byte_budget=2500 + ) + assert sorted(k[1] for k in registry) == ["a", "b"] + assert failed == {("npm", "c", "1.0.0"): c.LOOKUP_BYTES_BUDGET} + + +def test_byte_budget_spent_before_a_fetch_starts(): + fetch = CountingFetch() + registry, failed = c.lookup_registry( + _keys("a"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), byte_budget=0 + ) + assert registry == {} and failed == {("npm", "a", "1.0.0"): c.LOOKUP_BYTES_BUDGET} + assert fetch.calls == [] + + +def test_response_over_the_cap_is_not_a_byte_budget_failure(): + # Budget left is more than the cap: the cap reason applies. + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + return deps.FetchResult(200, b"x" * (max_bytes + 1), url) + + _, failed = c.lookup_registry(_keys("a"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), byte_budget=10**9) + assert failed == {("npm", "a", "1.0.0"): c.LOOKUP_RESPONSE_TOO_LARGE} + + +def test_reason_order_puts_run_limits_before_the_size_cap(): + assert c._LOOKUP_REASON_ORDER == ( + c.LOOKUP_BUDGET_EXCEEDED, + c.LOOKUP_COUNT_CAP, + c.LOOKUP_BYTES_BUDGET, + c.LOOKUP_RESPONSE_TOO_LARGE, + c.LOOKUP_FAILED, + ) + reasons = {c.LOOKUP_BUDGET_EXCEEDED, c.LOOKUP_COUNT_CAP, c.LOOKUP_BYTES_BUDGET, c.LOOKUP_RESPONSE_TOO_LARGE, c.LOOKUP_FAILED} + assert len(reasons) == 5 and all(r.startswith(c.LOOKUP_FAILED) for r in reasons) + + +def test_failed_lookup_is_logged_with_class_and_reason_but_no_body(capsys): + secret = b'{"token": "SECRET-BODY-TEXT"}' + + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + if url.endswith("/big"): + return deps.FetchResult(200, b"x" * (max_bytes + 1), url) + return deps.FetchResult(200, secret, url) # a JSON object that is for no package + + _, failed = c.lookup_registry(_keys("big", "odd"), fetch=fetch, sleep=_no_sleep, clock=FakeClock()) + err = capsys.readouterr().err + assert "collect: lookup failed for npm 'big' '1.0.0': ResponseTooLargeError:" in err + assert "byte cap" in err + assert "collect: lookup failed for npm 'odd' '1.0.0': RegistryError:" in err + assert "SECRET-BODY-TEXT" not in err and "xxxx" not in err + assert failed[("npm", "big", "1.0.0")] == c.LOOKUP_RESPONSE_TOO_LARGE + assert failed[("npm", "odd", "1.0.0")] == c.LOOKUP_FAILED + + +def test_failure_log_line_is_escaped_and_short(capsys): + c._log_lookup_failure(("npm", "a", "1.0.0"), OSError("line\n::error::boom " + "y" * 500)) + err = capsys.readouterr().err + assert err.count("\n") == 1 # one line, ends the log record + assert "::error::boom" in err and len(err) < 300 + + def test_lookup_budget_fails_every_remaining_key(): clock = FakeClock() fetch = CountingFetch(clock, step=115.0) diff --git a/review/tests/unit/test_deps.py b/review/tests/unit/test_deps.py index f9750cd..924cc63 100644 --- a/review/tests/unit/test_deps.py +++ b/review/tests/unit/test_deps.py @@ -885,10 +885,27 @@ def test_redirect_status_is_rejected(): def test_oversized_response_is_rejected_before_parsing(): body = b"x" * (deps.NPM_MAX_RESPONSE_BYTES + 1) resp = deps.FetchResult(status=200, body=body, url="https://registry.npmjs.org/esbuild") - with pytest.raises(RegistryError, match="byte cap"): + with pytest.raises(deps.ResponseTooLargeError, match="byte cap"): deps.lookup_npm("esbuild", "0.19.0", FakeFetch(resp)) +def test_npm_cap_boundary(): + # Exactly at the cap parses; one byte over is refused. 39,000,000 bytes + # (vite today) fits with 2x headroom. + assert deps.NPM_MAX_RESPONSE_BYTES == 80 * 1024 * 1024 + pad = "x" * 100 + body = json.dumps({**NPM_DOC, "pad": pad}).encode() + ok_resp = deps.FetchResult(200, body, "https://registry.npmjs.org/esbuild") + assert deps.lookup_npm("esbuild", "0.19.0", FakeFetch(ok_resp)).name == "esbuild" + over = deps.FetchResult(200, b" " * (deps.NPM_MAX_RESPONSE_BYTES + 1), "https://registry.npmjs.org/esbuild") + with pytest.raises(deps.ResponseTooLargeError): + deps.lookup_npm("esbuild", "0.19.0", FakeFetch(over)) + at_cap = deps.FetchResult( + 200, body + b" " * (deps.NPM_MAX_RESPONSE_BYTES - len(body)), "https://registry.npmjs.org/esbuild" + ) + assert deps.lookup_npm("esbuild", "0.19.0", FakeFetch(at_cap)).name == "esbuild" + + @pytest.mark.parametrize( "doc", [ From 1572a5b317add501e3a95394df5b08fc271b5948 Mon Sep 17 00:00:00 2001 From: Irving Popovetsky Date: Thu, 29 Oct 2026 12:43:06 -0700 Subject: [PATCH 2/3] Raise npm cap to 128 MiB, request gzip and inflate within the decoded cap (K-19) renovate's full packument is about 74 MB and grows. The fetcher now sends Accept-Encoding: gzip, inflates in bounded steps, counts decoded bytes against the cap and the run budget, and fails closed on other encodings or bad gzip. Co-Authored-By: Claude Sonnet 5.5 --- docs/claude-review-design.md | 5 +- review/src/oc_review/collect.py | 9 ++- review/src/oc_review/deps.py | 51 +++++++++++++--- review/tests/unit/test_collect.py | 60 +++++++++++++++++-- review/tests/unit/test_deps.py | 99 ++++++++++++++++++++++++++++++- 5 files changed, 207 insertions(+), 17 deletions(-) diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 49d192a..d8dc77c 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -286,12 +286,12 @@ The review covers one immutable comparison pair: the merge base commit and the h - **Supported versions:** `package-lock.json` lockfile v2 and v3; `yarn.lock` v1 and berry metadata versions 4 to 8; `pnpm-lock.yaml` 6.0, 6.1 and 9.0, in each document of a two-document file; `poetry.lock` 1.1, 2.0 and 2.1; `uv.lock` version 1. Any other version is unknown. Pip `-r` and `-c` includes are not followed, and an unknown pip option is a parse failure. `npm-shrinkwrap.json` is not parsed; it is an unsupported dependency file. - **Delta:** parse each changed supported file at the merge base and at the head (both from blobs by SHA) and compare. For each added or changed package: name, old and new version, resolved source (normalized), registry publish time of the new version, `hasInstallScript` (npm lockfile v2 and v3), and for npm direct deps the install lifecycle scripts from the registry manifest (`preinstall`, `install`, `postinstall`, `prepare`, `preprepare`, `postprepare`, `prepublish`). - **Alternatives:** a `pyproject.toml` dependency can list alternatives, split by markers: a list under `[tool.uv.sources]`, or a list of Poetry constraints. The code reads every alternative, whatever its marker, as its own package with its own version and source, so each one gets the source check and the 72-hour rule. A uv alternative with no known source key, or an empty Poetry list, is a parse failure. -- **Registry lookups:** only fixed routes built from validated names: `https://registry.npmjs.org/` and `https://pypi.org/pypi//json`. No redirects to other hosts, a response byte cap, and a timeout. Never fetch a URL from the PR, including a lockfile `resolved` field. The code refuses every redirect. Caps: 16 MiB for one dependency file, 80 MiB for an npm response (the full packument, for per-version publish times; K-18), 16 MiB for a PyPI response. Each lookup has a 20-second timeout and at most 2 retries, for 429, 5xx and network errors only. +- **Registry lookups:** only fixed routes built from validated names: `https://registry.npmjs.org/` and `https://pypi.org/pypi//json`. No redirects to other hosts, a response byte cap, and a timeout. Never fetch a URL from the PR, including a lockfile `resolved` field. The code refuses every redirect. Caps: 16 MiB for one dependency file, 128 MiB for an npm response (the full packument, for per-version publish times; K-18, K-19), 16 MiB for a PyPI response. Caps count decoded bytes. The fetcher sends `Accept-Encoding: gzip`, accepts a gzip or identity response, inflates in bounded steps and stops when the decoded size passes the cap, so a decompression bomb never inflates in full. Any other `Content-Encoding`, and a gzip body that is corrupt, truncated or has trailing data, fails the lookup with its own reason. Each lookup has a 20-second timeout and at most 2 retries, for 429, 5xx and network errors only. - **Lookup limits per run** (K-11): a large lockfile, or a migration whose base counts as empty (K-10), can need thousands of lookups. One by one they outlast the 10-minute job timeout, and the run fails with no bundle and no reason. So collect deduplicates the lookups by registry key (ecosystem, name, version) before any starts, and limits them with three constants in `collect.py`: - `REGISTRY_LOOKUP_CONCURRENCY = 8`: at most 8 packages are looked up at once. The versions of one package run in order on one worker, so its packument is fetched once. This is safe because lookups go only to the two fixed registry hosts. - `REGISTRY_LOOKUP_BUDGET_SECONDS = 240`: total wall-clock time for all lookups, well under the job timeout, so collect still has time to build and write the bundle. A fetch never gets more time than is left, and no fetch or retry starts after the deadline. Collect does not wait for a lookup that is still running at the deadline. - `MAX_REGISTRY_LOOKUPS = 3,000`: unique keys per run, in sorted order. A typical app lockfile has 1,000 to 2,000 unique names. - - `MAX_REGISTRY_BYTES = 1 GiB` (K-18): response bytes for all lookups of a run. Each fetch reserves its response cap, or what is left of this budget, before it starts, so received plus reserved bytes never pass it. + - `MAX_REGISTRY_BYTES = 1 GiB` (K-18, K-19): decoded response bytes for all lookups of a run. Each fetch reserves its response cap, or what is left of this budget, before it starts, so received plus reserved bytes never pass it. A key that is past the count cap or has no result at the deadline is a failed lookup, with the reason "registry lookup failed: over the lookup count cap" or "registry lookup failed: lookup time budget exceeded". A key past the byte budget has "registry lookup failed: over the download byte budget". A response over its size cap has "registry lookup failed: response over the size cap". Any other failure keeps "registry lookup failed". A file with several reasons shows one, in that order: time budget, count cap, byte budget, size cap, other. The data is incomplete, so the hard block applies, and collect still writes the bundle. Collect also logs one line per failed lookup to the job log: `collect: lookup failed for '' '': : `. The reason is cut to 160 characters and escaped, and never holds a response body. - **Official sources:** `registry.npmjs.org` and `registry.yarnpkg.com` for npm, `pypi.org` and `files.pythonhosted.org` for PyPI, over https with no port and no userinfo, and only for the ecosystem of the entry. An official npm tarball URL must have the path of that package and version. A lockfile entry with no resolved source is an unknown source (foreign), unless it is a workspace link or a bundled dependency. Local sources (file, workspace, bundled, patch) are not foreign. - **Failures:** a supported file at the head that does not parse, an unknown lockfile version at the head, or a registry lookup that still fails after retries or is not done within the lookup limits is the hard block "incomplete dependency data". It clears on a re-run when the cause is gone. Each file gets at most one failure entry. @@ -992,6 +992,7 @@ P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, - K-16. pnpm 12 lockfiles with two YAML documents (ruling on front-end PR 2027, 2026-10-29). pnpm 12 writes `pnpm-lock.yaml` as two YAML documents: first the env lockfile (one importer `.` with `configDependencies` and `packageManagerDependencies`, and the `packages` and `snapshots` of pnpm itself), then the main lockfile. The parser read one document only, so every front-end PR that changed the lockfile got the false hard block "incomplete dependency data" (does not parse), at the head and at the merge base. The parser now reads every document with the same safe loader that refuses duplicate keys. The loader expands YAML merge keys before that check, so a merged key that repeats another key in the same mapping is a parse failure too, in every YAML lockfile. It accepts one document, as before, or exactly two in that order. Each document must have a supported `lockfileVersion`. The first may hold only `lockfileVersion`, `importers`, `packages` and `snapshots`, and its only importer is `.` with only the two env keys. An env `packages` entry with its own `name` or `version` must match its key, and each env dependency must be a package that the parser emits from that document, so no env dependency can skip a check under another identity. The second may hold only the top-level keys of pnpm 12's main lockfile: `lockfileVersion`, `settings`, `catalogs`, `overrides`, `packageExtensionsChecksum`, `pnpmfileChecksum`, `ignoredOptionalDependencies`, `patchedDependencies`, `importers`, `packages`, `snapshots` and `time`. A one-document file keeps the old check, which ignores other keys. Packages of both documents go through every dependency check; pnpm and its platform binaries are dependencies too, and an env dependency counts as direct. Any other document count, an empty document, another env or main shape, or a bare document after a `...` end marker with no `---` is still the hard block. A malformed two-document merge-base file counts as empty, as in K-10. The size gate is unchanged. - K-17. Anchored patch for long files changed near both ends (pilot false skip, 2026-10-29). PR 2027 in `OperationCode/front-end` (2 files, `package.json` and a `pnpm-lock.yaml` of about 16,000 lines, GitHub +1,131/-1,153) got "too large for automatic review", so a routine dependency update got no model review. Cause: the changes span both ends of the lockfile, so its middle is about 16,000 by 16,000 lines, past `DIFF_BUDGET` (25,000,000). The patch fell back to one hunk with every base line removed and every head line added (+16,069/-16,091). Rendered with line numbers, the bundle estimated 451,544 tokens against the 150,000 limit. A plain difflib diff of that middle needs about 82 million counted steps, past the 5 million per-file budget, so raising `DIFF_BUDGET` alone does not help. Now collect splits such a middle at lines that occur once on each side and diffs only the gaps, within the same budgets ("Coverage limits"). Result for PR 2027 at the same blobs: +1,127/-1,149, estimate 93,786 tokens with the main-branch parser (the head lockfile does not parse there, so the delta is small), 115,961 with the K-16 parser and registry data for all 229 changed packages. Rejected: (b) counting lockfiles at a fine-diff or capped size for the estimate, because the gate must measure what Claude is sent: Claude would still get the 450k-token rendering, and a path rule would let a PR shrink the estimate of any content by its file name. (c) Sending a code-built package delta instead of the lockfile patch: it needs a path or parse rule to pick the files, so content in a lockfile-named file could be hidden from the model passes, and the supply-chain pass would lose the raw lines it reads today. The anchored patch reads no path, never drops a changed line, and leaves the size gate fail-closed: a full rewrite has no anchors and is still one hunk. It changes no gate rule and lowers no risk. - K-18. npm response cap and lookup failure visibility (pilot false hard block, front-end PR 2027, 2026-10-29). `vite` 8.3.5 is a real, old enough version, but its full packument is 38,999,531 bytes (37.2 MiB), past the 32 MiB cap, so the lookup failed and the PR got the hard blocks "registry lookup failed" and "no registry data for a new version". Collect hid the cause: every lookup error became the plain reason and no log line named it. Measured full packuments on 2026-10-29 with the code's `Accept: application/json` (bytes): `vite` 38,999,531; `next` 31,483,965 (1.1 MiB under the old cap); `typescript` 15,865,004; `@types/node` 11,183,996; `aws-sdk` 10,595,791; `react-dom` 9,584,246; `webpack` 5,246,798; `electron` 4,301,537; `@aws-sdk/client-s3` 4,251,556; `@types/react` 2,642,195; `eslint` 2,099,986; `@babel/core` 810,569. Now `NPM_MAX_RESPONSE_BYTES` is 80 MiB (2.15x the largest). The body is still read in 64 KiB chunks and never past the cap plus one byte, and a response over the cap still fails closed. Aggregate check: 8 workers run at once and a parsed packument takes about 10x its body in memory (vite: 373 MB peak for the 39 MB body), so the worst case is 8 x 80 MiB x 10, about 6.4 GiB, only if 8 packuments sit at the cap at once; the 8 largest real packuments total about 137 MB, about 1.4 GB parsed. A hosted runner has 16 GB (7 GB on a private repo), and the per-response cap is the memory bound. Time is bounded by the 240 s budget and the 20 s fetch timeout, both unchanged. New `MAX_REGISTRY_BYTES = 1 GiB` bounds download volume per run: each fetch reserves its cap or the rest of the budget first, and a key past it fails with its own reason. Failure reasons are now distinct for "response over the size cap" and "over the download byte budget", and each failed lookup logs its exception class and a short, escaped reason, never a body. The prefilter still reports one failed lookup twice, deliberately: the file row ("dependency data incomplete: ") names the cause, and the per-package row ("no registry data for a new version") names each package the 72-hour rule could not check; merging them would drop one of the two. No gate rule changes, and nothing lowers risk. +- K-19. npm cap raised to 128 MiB, gzip requested (owner decision, 2026-10-29, after two independent analyses of large packuments). The 80 MiB cap of K-18 had 1.1x headroom over the largest legitimate packument found later: `renovate`, about 74 MB, growing about 22 MB per year. The owner chose to raise the cap now and stream later. `NPM_MAX_RESPONSE_BYTES` is 128 MiB of decoded bytes: about 1.7x headroom over `renovate`, while 8 parallel full `json.loads` parses (about 6.4x the document size each, so about 6.4 GiB in the worst case) stay within a GitHub-hosted runner's memory. The fetcher now sends `Accept-Encoding: gzip` and inflates with stdlib `zlib` (`decompressobj` with `max_length` and `unconsumed_tail`), at most the room left under the cap plus one byte per step, so it aborts as soon as the decoded size passes the cap. The encoded size is also held to the cap. An identity response is accepted. Any other `Content-Encoding` fails closed with the reason "unsupported or corrupt response encoding", as does a corrupt or truncated gzip body or data after the gzip stream; none is retried. The cap and the per-run byte budget (`MAX_REGISTRY_BYTES`, 1 GiB) both count decoded bytes: memory and parse time follow the decoded size, the fetch interface is unchanged, and decoded bytes are never lower than encoded bytes, so the budget stays an upper bound on download volume. The HTTPS, host and no-redirect checks are unchanged. Deferred: a streaming extractor (bounded, grouped `ijson` extraction of `time[]` and the lifecycle scripts of the wanted versions, from the design in the second analysis) would remove the memory multiple. Trigger: switch when a legitimate packument passes about 100 MiB or runner memory becomes a problem. No gate rule changes, and nothing lowers risk. P4: phase 4 pilot start, 2026-10-28 and 2026-10-29 (owner and coordinator). The pilot ruleset was created and enabled on 2026-10-29. Where a P4- line changes an earlier line, it wins. diff --git a/review/src/oc_review/collect.py b/review/src/oc_review/collect.py index ee9bbef..e38f1f8 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -90,10 +90,10 @@ REGISTRY_LOOKUP_BUDGET_SECONDS = 240.0 # Unique keys looked up per run; past the cap a key fails without a lookup. MAX_REGISTRY_LOOKUPS = 3000 -# Response bytes for all lookups of a run (K-18). Each fetch reserves its cap +# Decoded response bytes for all lookups of a run (K-18, K-19). Each fetch reserves its cap # (or what is left) before it starts, so reserved plus received bytes never # pass this. It bounds download volume; memory per worker is bounded by the -# per-response cap (a parsed packument is about 10x its body). +# per-response cap (a parsed packument is about 6.4x its body). MAX_REGISTRY_BYTES = 1024 * 1024 * 1024 # Fixed failure reasons ("incomplete dependency data"), in the order a file # with several picks its one reason. @@ -101,12 +101,14 @@ LOOKUP_COUNT_CAP = "registry lookup failed: over the lookup count cap" LOOKUP_BYTES_BUDGET = "registry lookup failed: over the download byte budget" LOOKUP_RESPONSE_TOO_LARGE = "registry lookup failed: response over the size cap" +LOOKUP_BAD_ENCODING = "registry lookup failed: unsupported or corrupt response encoding" LOOKUP_FAILED = "registry lookup failed" _LOOKUP_REASON_ORDER = ( LOOKUP_BUDGET_EXCEEDED, LOOKUP_COUNT_CAP, LOOKUP_BYTES_BUDGET, LOOKUP_RESPONSE_TOO_LARGE, + LOOKUP_BAD_ENCODING, LOOKUP_FAILED, ) @@ -760,6 +762,9 @@ def run_group(group: list[RegistryKey]) -> None: except deps.ResponseTooLargeError as exc: _log_lookup_failure(key, exc) result = LOOKUP_RESPONSE_TOO_LARGE + except deps.ResponseEncodingError as exc: + _log_lookup_failure(key, exc) + result = LOOKUP_BAD_ENCODING except Exception as exc: # noqa: BLE001 - any error is a failed lookup, never a lost bundle _log_lookup_failure(key, exc) result = LOOKUP_FAILED diff --git a/review/src/oc_review/deps.py b/review/src/oc_review/deps.py index 530f494..507940d 100644 --- a/review/src/oc_review/deps.py +++ b/review/src/oc_review/deps.py @@ -27,6 +27,7 @@ import urllib.error import urllib.parse import urllib.request +import zlib from collections.abc import Callable, Iterable, Mapping from dataclasses import dataclass, field, replace from datetime import UTC, datetime @@ -36,9 +37,10 @@ # Byte cap on one manifest or lockfile, applied before parsing. MAX_DEP_FILE_BYTES = 16 * 1024 * 1024 -# Byte caps on registry responses, applied before parsing. The npm cap is -# 2x the largest full packument measured on 2026-10-29 (vite, 37.2 MiB), K-18. -NPM_MAX_RESPONSE_BYTES = 80 * 1024 * 1024 +# Byte caps on registry responses (decoded bytes), applied before parsing. The +# npm cap is about 1.7x the largest legitimate full packument measured on +# 2026-10-29 (renovate, about 74 MB), K-18 and K-19. +NPM_MAX_RESPONSE_BYTES = 128 * 1024 * 1024 PYPI_MAX_RESPONSE_BYTES = 16 * 1024 * 1024 REGISTRY_TIMEOUT_SECONDS = 20.0 REGISTRY_RETRIES = 2 @@ -132,6 +134,11 @@ class ResponseTooLargeError(RegistryError): """A registry response is over its byte cap.""" +class ResponseEncodingError(RegistryError): + """A registry response has a Content-Encoding the fetcher does not accept, + or a gzip body that is corrupt or truncated.""" + + # --------------------------------------------------------------------------- # Data model # --------------------------------------------------------------------------- @@ -1499,20 +1506,50 @@ def default_fetch( if parts.scheme != "https" or parts.hostname not in (NPM_REGISTRY_HOST, PYPI_REGISTRY_HOST): raise RegistryError("default_fetch refuses a URL outside the fixed registry hosts") opener = opener or urllib.request.build_opener(_NoRedirect) - request = urllib.request.Request(url, headers={"Accept": "application/json", "User-Agent": "oc-claude-review"}) + request = urllib.request.Request( + url, + headers={"Accept": "application/json", "Accept-Encoding": "gzip", "User-Agent": "oc-claude-review"}, + ) deadline = clock() + timeout try: with opener.open(request, timeout=timeout) as resp: + encoding = (resp.headers.get("Content-Encoding") or "identity").strip().lower() + if encoding not in ("identity", "gzip", "x-gzip"): + raise ResponseEncodingError(f"{url}: unsupported Content-Encoding") + # 31 = gzip wrapper only (no zlib or raw deflate). + inflater = zlib.decompressobj(31) if encoding != "identity" else None chunks: list[bytes] = [] - size = 0 + size = 0 # decoded bytes + wire = 0 # encoded bytes while size <= max_bytes: if clock() > deadline: raise TimeoutError(f"{url}: over the {timeout}s total deadline") chunk = resp.read(min(65536, max_bytes + 1 - size)) if not chunk: break - chunks.append(chunk) - size += len(chunk) + if inflater is None: + chunks.append(chunk) + size += len(chunk) + continue + wire += len(chunk) + if wire > max_bytes: + raise ResponseTooLargeError(f"{url}: encoded response is over the {max_bytes} byte cap") + try: + # Inflate in bounded steps: never more than the room left + # under the cap plus one byte, so a bomb stops early. + data = inflater.decompress(chunk, max_bytes + 1 - size) + while True: + chunks.append(data) + size += len(data) + if not inflater.unconsumed_tail or size > max_bytes: + break + data = inflater.decompress(inflater.unconsumed_tail, max_bytes + 1 - size) + except zlib.error as exc: + raise ResponseEncodingError(f"{url}: gzip body is corrupt ({exc})") from exc + if inflater.eof and inflater.unused_data: + raise ResponseEncodingError(f"{url}: data after the end of the gzip stream") + if inflater is not None and size <= max_bytes and not inflater.eof: + raise ResponseEncodingError(f"{url}: gzip body is truncated") return FetchResult(status=resp.status, body=b"".join(chunks), url=resp.url) except urllib.error.HTTPError as exc: return FetchResult(status=exc.code, body=b"", url=url) diff --git a/review/tests/unit/test_collect.py b/review/tests/unit/test_collect.py index f056fb1..ac14dd4 100644 --- a/review/tests/unit/test_collect.py +++ b/review/tests/unit/test_collect.py @@ -1192,8 +1192,10 @@ def _sized_packument(name: str, size: int) -> bytes: def test_npm_cap_fits_the_largest_measured_sized_packument(): - # vite on 2026-10-29: 38,999,531 bytes; the cap keeps 2x headroom (K-18). + # vite on 2026-10-29: 38,999,531 bytes; renovate about 74 MB. The cap keeps + # about 1.7x headroom over renovate (K-18, K-19). assert deps.NPM_MAX_RESPONSE_BYTES >= 2 * 38_999_531 + assert deps.NPM_MAX_RESPONSE_BYTES >= int(1.7 * 74_000_000) def test_packument_just_under_and_over_the_cap_through_the_default_fetch_path(): @@ -1219,7 +1221,7 @@ def test_byte_budget_fails_closed_with_its_own_reason(): def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: return deps.FetchResult(200, body.replace(b'"a"', b'"%s"' % url.rsplit("/", 1)[1].encode(), 1), url) - # 2500 bytes: the cap reservation (80 MiB) is cut to what is left, and + # 2500 bytes: the cap reservation (the cap) is cut to what is left, and # the third response does not fit. registry, failed = c.lookup_registry( _keys("a", "b", "c"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=1, byte_budget=2500 @@ -1246,16 +1248,25 @@ def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: assert failed == {("npm", "a", "1.0.0"): c.LOOKUP_RESPONSE_TOO_LARGE} +def test_bad_encoding_has_its_own_reason(): + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + raise deps.ResponseEncodingError("x: unsupported Content-Encoding") + + _, failed = c.lookup_registry(_keys("a"), fetch=fetch, sleep=_no_sleep, clock=FakeClock()) + assert failed == {("npm", "a", "1.0.0"): c.LOOKUP_BAD_ENCODING} + + def test_reason_order_puts_run_limits_before_the_size_cap(): assert c._LOOKUP_REASON_ORDER == ( c.LOOKUP_BUDGET_EXCEEDED, c.LOOKUP_COUNT_CAP, c.LOOKUP_BYTES_BUDGET, c.LOOKUP_RESPONSE_TOO_LARGE, + c.LOOKUP_BAD_ENCODING, c.LOOKUP_FAILED, ) - reasons = {c.LOOKUP_BUDGET_EXCEEDED, c.LOOKUP_COUNT_CAP, c.LOOKUP_BYTES_BUDGET, c.LOOKUP_RESPONSE_TOO_LARGE, c.LOOKUP_FAILED} - assert len(reasons) == 5 and all(r.startswith(c.LOOKUP_FAILED) for r in reasons) + reasons = set(c._LOOKUP_REASON_ORDER) + assert len(reasons) == 6 and all(r.startswith(c.LOOKUP_FAILED) for r in reasons) def test_failed_lookup_is_logged_with_class_and_reason_but_no_body(capsys): @@ -1573,3 +1584,44 @@ def test_hung_lookup_does_not_hold_the_process_open(tmp_path): assert json.loads(proc.stdout) == [{"path": "package-lock.json", "reason": c.LOOKUP_BUDGET_EXCEEDED}] data = (tmp_path / c.BUNDLE_FILE_NAME).read_bytes() envelope.validate_envelope(envelope.parse_json(data)) + + +def test_byte_budget_counts_decoded_bytes_of_gzip_responses(): + import gzip + + body = _sized_packument("a", 4000) + assert len(gzip.compress(body)) < 1000 # the encoded size is far below the decoded size + + class Resp: + status = 200 + url = "https://registry.npmjs.org/a" + headers = {"Content-Encoding": "gzip"} + + def __init__(self, url): + self.url = url + named = body.replace(b'"a"', b'"%s"' % url.rsplit("/", 1)[1].encode(), 1) + self.chunks = [gzip.compress(named)] + + def read(self, n): + return self.chunks.pop(0) if self.chunks else b"" + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + class Opener: + def open(self, request, timeout): + return Resp(request.full_url) + + def fetch(url, max_bytes, timeout): + return deps.default_fetch(url, max_bytes, timeout, opener=Opener()) + + # Decoded size 4000 each: a 9000 byte budget fits two, not three, even + # though three encoded bodies would fit easily. + registry, failed = c.lookup_registry( + _keys("a", "b", "c"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=1, byte_budget=9000 + ) + assert len(registry) + len(failed) == 3 + assert list(failed.values()) == [c.LOOKUP_BYTES_BUDGET] diff --git a/review/tests/unit/test_deps.py b/review/tests/unit/test_deps.py index 924cc63..e37e938 100644 --- a/review/tests/unit/test_deps.py +++ b/review/tests/unit/test_deps.py @@ -892,7 +892,7 @@ def test_oversized_response_is_rejected_before_parsing(): def test_npm_cap_boundary(): # Exactly at the cap parses; one byte over is refused. 39,000,000 bytes # (vite today) fits with 2x headroom. - assert deps.NPM_MAX_RESPONSE_BYTES == 80 * 1024 * 1024 + assert deps.NPM_MAX_RESPONSE_BYTES == 128 * 1024 * 1024 pad = "x" * 100 body = json.dumps({**NPM_DOC, "pad": pad}).encode() ok_resp = deps.FetchResult(200, body, "https://registry.npmjs.org/esbuild") @@ -1228,8 +1228,9 @@ def test_lookup_pypi_matches_normalized_version(): class _Resp: - def __init__(self, chunks, status=200, url="https://registry.npmjs.org/a"): + def __init__(self, chunks, status=200, url="https://registry.npmjs.org/a", headers=None): self.chunks, self.status, self.url = list(chunks), status, url + self.headers = headers or {} def read(self, n): return self.chunks.pop(0)[:n] if self.chunks else b"" @@ -1282,3 +1283,97 @@ def test_default_fetch_total_deadline_and_httplib_errors(): deps.default_fetch( "https://registry.npmjs.org/a", 10, 5.0, opener=_Opener(http.client.IncompleteRead(b"x")) ) + + +def _gz(data: bytes) -> bytes: + import gzip + + return gzip.compress(data) + + +def _gz_resp(payload: bytes, step: int = 100, **headers): + chunks = [payload[i : i + step] for i in range(0, len(payload), step)] + return _Resp(chunks, headers={"Content-Encoding": "gzip", **headers}) + + +def _fetch_with(resp, cap=1000): + return deps.default_fetch("https://registry.npmjs.org/a", cap, 5.0, opener=_Opener(resp)) + + +def test_default_fetch_requests_gzip(): + seen = {} + + class Op(_Opener): + def open(self, request, timeout): + seen.update({k.lower(): v for k, v in request.header_items()}) + return super().open(request, timeout) + + deps.default_fetch("https://registry.npmjs.org/a", 10, 5.0, opener=Op(_Resp([b"ab"]))) + assert seen["accept-encoding"] == "gzip" + + +def test_default_fetch_inflates_gzip_under_the_cap(): + body = b'{"a": 1}' * 50 + got = _fetch_with(_gz_resp(_gz(body)), cap=len(body)) + assert got.body == body and got.status == 200 # exactly at the cap is fine + + +def test_default_fetch_gzip_over_the_decoded_cap_stops_early(): + # A small compressed payload that inflates far past the cap. + bomb = _gz(b"\0" * 50_000_000) + assert len(bomb) < 100_000 + resp = _gz_resp(bomb, step=1000) + got = _fetch_with(resp, cap=1000) + assert len(got.body) == 1001 # the cap plus one byte, never the full inflate + assert resp.chunks # stopped before reading the whole stream + + +def test_default_fetch_gzip_one_byte_over_the_cap(): + body = b"x" * 1001 + assert len(_fetch_with(_gz_resp(_gz(body)), cap=1000).body) == 1001 + + +def test_default_fetch_gzip_large_chunk_inflates_in_bounded_steps(): + body = b"y" * 5000 + got = _fetch_with(_gz_resp(_gz(body), step=10_000), cap=10_000) + assert got.body == body + + +def test_default_fetch_identity_body_with_and_without_header(): + assert _fetch_with(_Resp([b"abc"], headers={"Content-Encoding": "identity"})).body == b"abc" + assert _fetch_with(_Resp([b"abc"])).body == b"abc" + + +def test_default_fetch_rejects_unknown_encodings(): + for enc in ("br", "deflate", "zstd", "gzip, br", "compress"): + with pytest.raises(deps.ResponseEncodingError, match="Content-Encoding"): + _fetch_with(_Resp([b"abc"], headers={"Content-Encoding": enc})) + + +def test_default_fetch_corrupt_and_truncated_gzip_fail_closed(): + good = _gz(b'{"a": 1}' * 100) + with pytest.raises(deps.ResponseEncodingError, match="corrupt"): + _fetch_with(_gz_resp(b"not gzip at all, just text" * 4), cap=10_000) + with pytest.raises(deps.ResponseEncodingError, match="truncated"): + _fetch_with(_gz_resp(good[: len(good) // 2]), cap=10_000) + with pytest.raises(deps.ResponseEncodingError, match="corrupt"): + _fetch_with(_gz_resp(good[:-8] + b"\x00" * 8), cap=10_000) # bad CRC and size + with pytest.raises(deps.ResponseEncodingError, match="after the end"): + _fetch_with(_gz_resp(good + good), cap=10_000) + + +def test_default_fetch_encoded_body_past_the_cap_is_refused(): + import os + + noise = os.urandom(5000) # does not compress: encoded size is past the cap + with pytest.raises(deps.ResponseTooLargeError): + _fetch_with(_gz_resp(_gz(noise), step=500), cap=1000) + + +def test_lookup_npm_accepts_a_gzip_response_through_default_fetch(): + import json as _json + + doc = _json.dumps(NPM_DOC).encode() + resp = _gz_resp(_gz(doc), step=64, url="https://registry.npmjs.org/esbuild") + info = deps.lookup_npm("esbuild", "0.19.0", lambda url, mb, t: deps.default_fetch(url, mb, t, opener=_Opener(resp))) + assert info.name == "esbuild" From 4df72dcad804b3a33cd58f097e1abb118a2acd5b Mon Sep 17 00:00:00 2001 From: Irving Popovetsky Date: Thu, 29 Oct 2026 12:47:16 -0700 Subject: [PATCH 3/3] Release the byte-budget reservation when a fetch fails before a body arrives (K-19 fix 1) A timeout or network error no longer spends the run budget; oversize and bad-encoding downloads still count. Near the budget an over-grant response maps to the byte-budget reason. Docs: encoding reason in the reason order, note on decoded-byte budget. Co-Authored-By: Claude Sonnet 5.5 --- docs/claude-review-design.md | 4 ++-- review/src/oc_review/collect.py | 12 +++++++++- review/tests/unit/test_collect.py | 39 +++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index d8dc77c..c5bb740 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -291,8 +291,8 @@ The review covers one immutable comparison pair: the merge base commit and the h - `REGISTRY_LOOKUP_CONCURRENCY = 8`: at most 8 packages are looked up at once. The versions of one package run in order on one worker, so its packument is fetched once. This is safe because lookups go only to the two fixed registry hosts. - `REGISTRY_LOOKUP_BUDGET_SECONDS = 240`: total wall-clock time for all lookups, well under the job timeout, so collect still has time to build and write the bundle. A fetch never gets more time than is left, and no fetch or retry starts after the deadline. Collect does not wait for a lookup that is still running at the deadline. - `MAX_REGISTRY_LOOKUPS = 3,000`: unique keys per run, in sorted order. A typical app lockfile has 1,000 to 2,000 unique names. - - `MAX_REGISTRY_BYTES = 1 GiB` (K-18, K-19): decoded response bytes for all lookups of a run. Each fetch reserves its response cap, or what is left of this budget, before it starts, so received plus reserved bytes never pass it. - A key that is past the count cap or has no result at the deadline is a failed lookup, with the reason "registry lookup failed: over the lookup count cap" or "registry lookup failed: lookup time budget exceeded". A key past the byte budget has "registry lookup failed: over the download byte budget". A response over its size cap has "registry lookup failed: response over the size cap". Any other failure keeps "registry lookup failed". A file with several reasons shows one, in that order: time budget, count cap, byte budget, size cap, other. The data is incomplete, so the hard block applies, and collect still writes the bundle. Collect also logs one line per failed lookup to the job log: `collect: lookup failed for '' '': : `. The reason is cut to 160 characters and escaped, and never holds a response body. + - `MAX_REGISTRY_BYTES = 1 GiB` (K-18, K-19): decoded response bytes for all lookups of a run. Each fetch reserves its response cap, or what is left of this budget, before it starts, so received plus reserved bytes never pass it. A fetch that fails before a body arrives (timeout, network error, 5xx) releases its reservation; a response read past its cap, or one that fails to decode, keeps it. The budget counts decoded bytes, so it is a tighter limit on wire volume than its name suggests (wire bytes are often 5 to 10 times smaller); raise it if a pilot run hits it. + A key that is past the count cap or has no result at the deadline is a failed lookup, with the reason "registry lookup failed: over the lookup count cap" or "registry lookup failed: lookup time budget exceeded". A key past the byte budget has "registry lookup failed: over the download byte budget". A response over its size cap has "registry lookup failed: response over the size cap". A response with an unsupported or corrupt encoding has "registry lookup failed: unsupported or corrupt response encoding". Any other failure keeps "registry lookup failed". A file with several reasons shows one, in that order: time budget, count cap, byte budget, size cap, encoding, other. The data is incomplete, so the hard block applies, and collect still writes the bundle. Collect also logs one line per failed lookup to the job log: `collect: lookup failed for '' '': : `. The reason is cut to 160 characters and escaped, and never holds a response body. - **Official sources:** `registry.npmjs.org` and `registry.yarnpkg.com` for npm, `pypi.org` and `files.pythonhosted.org` for PyPI, over https with no port and no userinfo, and only for the ecosystem of the entry. An official npm tarball URL must have the path of that package and version. A lockfile entry with no resolved source is an unknown source (foreign), unless it is a workspace link or a bundled dependency. Local sources (file, workspace, bundled, patch) are not foreign. - **Failures:** a supported file at the head that does not parse, an unknown lockfile version at the head, or a registry lookup that still fails after retries or is not done within the lookup limits is the hard block "incomplete dependency data". It clears on a re-run when the cause is gone. Each file gets at most one failure entry. - **Merge base that does not parse** (K-10): a supported file whose merge-base side does not parse or has an unknown version (for example an npm `lockfileVersion: 1` lockfile that the PR migrates to v3) does not block by itself. Its base side counts as empty: every head entry counts as added and gets every check (source, registry lookup, 72-hour rule), under the same per-lookup caps and run limits (`REGISTRY_LOOKUP_CONCURRENCY`, `REGISTRY_LOOKUP_BUDGET_SECONDS`, `MAX_REGISTRY_LOOKUPS`, K-11). So an entry that was already at the merge base counts as added: an existing git or tarball dependency, or any other foreign source, in that file now blocks. If a lookup fails, goes over a byte cap, is past the count cap or has no result within the time budget, or the head side does not parse, the data is incomplete and the hard block applies. A deleted file with such a base has no head entries, so it does not block. The base failure is a signal (context only). A base blob that could not be read is still the hard block. A base symlink or submodule holds no dependency data and counts as absent. diff --git a/review/src/oc_review/collect.py b/review/src/oc_review/collect.py index e38f1f8..f1a200a 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -716,10 +716,20 @@ def bounded_fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: raise _BytesSpent("download byte budget exceeded") grant = min(max_bytes, room) reserved += grant - got = grant # a failed fetch keeps its whole reservation + # Bytes that count against the budget. A fetch that fails before a + # body arrives (timeout, network error, 5xx) downloaded nothing that + # counts, so it keeps none of its reservation. A response that was + # read past its cap, or a body that did not decode, was a real + # download: it keeps the whole reservation. + got = 0 try: result = fetch(url, grant, min(timeout, left)) got = len(result.body) + except (deps.ResponseTooLargeError, deps.ResponseEncodingError) as exc: + got = grant + if grant < max_bytes and isinstance(exc, deps.ResponseTooLargeError): + raise _BytesSpent("download byte budget exceeded") from exc + raise finally: with state: reserved -= grant - got diff --git a/review/tests/unit/test_collect.py b/review/tests/unit/test_collect.py index ac14dd4..6e4c6e5 100644 --- a/review/tests/unit/test_collect.py +++ b/review/tests/unit/test_collect.py @@ -1248,6 +1248,45 @@ def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: assert failed == {("npm", "a", "1.0.0"): c.LOOKUP_RESPONSE_TOO_LARGE} +def test_failed_fetches_do_not_spend_the_byte_budget(): + body = _sized_packument("a", 1000) + state = {"fail": True} + + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + if state["fail"] and not url.endswith("/z"): + raise TimeoutError("registry blip") + return deps.FetchResult(200, body.replace(b'"a"', b'"%s"' % url.rsplit("/", 1)[1].encode(), 1), url) + + # Many failing attempts (3 per key) would spend 5000 bytes at the cap + # each if reservations stayed; the later valid lookup must still fit. + registry, failed = c.lookup_registry( + _keys("a", "b", "c", "d", "e", "f", "z"), + fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=1, byte_budget=5000, + ) + assert list(registry) == _keys("z") + assert set(failed.values()) == {c.LOOKUP_FAILED} + assert len(failed) == 6 + + +def test_oversize_downloads_still_spend_the_byte_budget(): + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + raise deps.ResponseTooLargeError(f"{url}: response is over the {max_bytes} byte cap") + + # Budget 2500, grant 1000 per call (cap patched): the real oversize + # downloads keep their reservations, so the third key finds the budget + # cut and gets the byte-budget reason; near the budget an over-grant + # response is a byte-budget failure, not a cap failure (M1). + with pytest.MonkeyPatch.context() as mp: + mp.setattr(deps, "NPM_MAX_RESPONSE_BYTES", 1000) + _, failed = c.lookup_registry( + _keys("a", "b", "c", "d"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=1, byte_budget=2500 + ) + assert failed[("npm", "a", "1.0.0")] == c.LOOKUP_RESPONSE_TOO_LARGE + assert failed[("npm", "b", "1.0.0")] == c.LOOKUP_RESPONSE_TOO_LARGE + assert failed[("npm", "c", "1.0.0")] == c.LOOKUP_BYTES_BUDGET + assert failed[("npm", "d", "1.0.0")] == c.LOOKUP_BYTES_BUDGET + + def test_bad_encoding_has_its_own_reason(): def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: raise deps.ResponseEncodingError("x: unsupported Content-Encoding")