From 04ddf404579fe769a5cbc78d8ffb55192be9fe34 Mon Sep 17 00:00:00 2001 From: Irving Popovetsky Date: Thu, 29 Oct 2026 18:31:40 -0700 Subject: [PATCH 1/3] Add local reproduction harness for collect's dependency stage review/tools/dep_repro.py reads two trees and the changed blobs through the REST API as data and runs collect.dependency_facts and the prefilter against the live registries, then prints keys, bytes, wall time, failed lookups and every hard block and signal. Developer tool only: not imported by production code, not run in CI. Co-Authored-By: Claude Opus 5.5 --- review/tools/dep_repro.py | 197 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 review/tools/dep_repro.py diff --git a/review/tools/dep_repro.py b/review/tools/dep_repro.py new file mode 100644 index 0000000..3852519 --- /dev/null +++ b/review/tools/dep_repro.py @@ -0,0 +1,197 @@ +"""Local reproduction harness for collect's dependency stage (developer tool). + + cd review + uv run python tools/dep_repro.py --repo OWNER/NAME --head HEAD_SHA --base MERGE_BASE_SHA + +It reads the two trees and the changed blobs through the GitHub REST API (the +token comes from `gh auth token`, or GITHUB_TOKEN), as data only: it never +checks out, installs, builds or runs anything from the repo. Then it runs the +same code as a collect run on those blobs: `collect.dependency_facts` (same +parser, delta, lookup code, limits and worker count) against the live +registries, and `prefilter.run_prefilter` on the result. It prints the keys, +unique names, decoded and wire bytes of the registry responses, the wall +time, each failed lookup log line, and every hard block and signal with a +count. + +Pass the merge base as `--base`, as collect does. The PR title, body and +author association are not read, so the checks that use them see empty text. +Not imported by production code and not run in CI: it makes live calls. +""" + +from __future__ import annotations + +import argparse +import contextlib +import io +import os +import re +import subprocess +import sys +import threading +import time +import urllib.request +from collections import Counter +from collections.abc import Sequence +from datetime import UTC, datetime +from pathlib import Path +from typing import Any + +_SRC = Path(__file__).resolve().parents[1] / "src" +if str(_SRC) not in sys.path: + sys.path.insert(0, str(_SRC)) + +from oc_review import collect, deps, prefilter # noqa: E402 +from oc_review.snapshot import diff_trees # noqa: E402 + +_SHA_RE = re.compile(r"^[0-9a-f]{40}$") +_MB = 1_000_000 + + +class _Meter: + """Registry traffic of one run: fetches, decoded and wire bytes.""" + + def __init__(self) -> None: + self.lock = threading.Lock() + self.fetches = 0 + self.decoded = 0 + self.wire = 0 + self.errors: Counter[str] = Counter() + + +class _CountingResponse: + """Wraps a urllib response and counts the bytes read from the socket.""" + + def __init__(self, resp: Any, meter: _Meter) -> None: + self._resp = resp + self._meter = meter + + def read(self, n: int = -1) -> bytes: + data = self._resp.read(n) + with self._meter.lock: + self._meter.wire += len(data) + return data + + def __enter__(self) -> _CountingResponse: + self._resp.__enter__() + return self + + def __exit__(self, *exc: Any) -> None: + self._resp.__exit__(*exc) + + def __getattr__(self, name: str) -> Any: + return getattr(self._resp, name) + + +class _CountingOpener: + def __init__(self, meter: _Meter) -> None: + self._opener = urllib.request.build_opener(deps._NoRedirect) + self._meter = meter + + def open(self, request: Any, timeout: float) -> _CountingResponse: + return _CountingResponse(self._opener.open(request, timeout=timeout), self._meter) + + +def _metered_fetch(meter: _Meter) -> deps.Fetcher: + opener = _CountingOpener(meter) + + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + with meter.lock: + meter.fetches += 1 + try: + result = deps.default_fetch(url, max_bytes, timeout, opener=opener) + except Exception as exc: + with meter.lock: + meter.errors[type(exc).__name__] += 1 + raise + with meter.lock: + meter.decoded += len(result.body) + return result + + return fetch + + +def _token() -> str: + token = os.environ.get("GITHUB_TOKEN") + if token: + return token + return subprocess.run(["gh", "auth", "token"], check=True, capture_output=True, text=True).stdout.strip() + + +def _sha(value: str) -> str: + if not _SHA_RE.match(value): + raise argparse.ArgumentTypeError("not a 40-character lowercase hex SHA") + return value + + +def run(repo: str, head: str, base: str) -> int: + owner, name = repo.split("/") + repo_path = f"/repos/{owner}/{name}" + client = collect.GitHubClient(_token()) + base_tree = collect._read_tree(client, repo_path, base) + head_tree = collect._read_tree(client, repo_path, head) + diff = diff_trees(base_tree.response, head_tree.response) + if not diff.complete: + print("tree diff is incomplete (truncated tree); collect would hard block") + return 1 + sizes = {**base_tree.sizes, **head_tree.sizes} + blobs, unread = collect._read_blobs( + client, repo_path, collect._blob_shas(diff), sizes, + max_blob_bytes=prefilter.MAX_BLOB_BYTES, max_reads=collect.MAX_BLOB_READS, + ) + + meter = _Meter() + log = io.StringIO() + started = time.monotonic() + with contextlib.redirect_stderr(log): + facts = collect.dependency_facts(diff, blobs, fetch=_metered_fetch(meter)) + wall = time.monotonic() - started + result = prefilter.run_prefilter( + diff, blobs, head_paths=head_tree.paths, now=datetime.now(UTC), + dependencies=facts, unread_reasons=unread, + ) + + keys = sorted({k for ch in facts.changes if (k := prefilter.registry_key(ch)) is not None}) + missing = [k for k in keys if k not in facts.registry] + files = [c.path for c in diff.files if deps.family_for_path(c.path) is not None] + print(f"repo {repo} head {head} base {base}") + print(f"code: collect.MAX_REGISTRY_BYTES={collect.MAX_REGISTRY_BYTES} " + f"concurrency={collect.REGISTRY_LOOKUP_CONCURRENCY} " + f"time budget={collect.REGISTRY_LOOKUP_BUDGET_SECONDS}s " + f"npm cap={deps.NPM_MAX_RESPONSE_BYTES}") + print(f"changed files {len(diff.files)}, dependency files {len(files)}: {', '.join(files) or '-'}") + print(f"package changes {len(facts.changes)}, registry keys {len(keys)}, " + f"unique names {len({k[:2] for k in keys})}") + print(f"registry fetches {meter.fetches}, decoded {meter.decoded / _MB:.1f} MB, " + f"wire {meter.wire / _MB:.1f} MB, lookup wall time {wall:.1f} s") + if meter.errors: + print("fetch exceptions: " + ", ".join(f"{k} {v}" for k, v in sorted(meter.errors.items()))) + print(f"keys with registry data {len(keys) - len(missing)}, keys without {len(missing)}") + + lines = [line for line in log.getvalue().splitlines() if line] + by_class = Counter(m.group(1) for line in lines if (m := re.search(r"': (\w+): ", line))) + print(f"\nfailed lookup log lines: {len(lines)}" + ( + " (" + ", ".join(f"{k} {v}" for k, v in sorted(by_class.items())) + ")" if by_class else "")) + for line in lines: + print(f" {line}") + + for kind, checks in (("hard blocks", result.hard_blocks), ("signals", result.signals)): + counts = Counter((c.id, c.detail) for c in checks) + print(f"\n{kind}: {len(checks)}") + for (cid, detail), n in sorted(counts.items()): + print(f" {n:4d} {cid}: {detail}") + return 0 + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__.split("\n", 1)[0]) + parser.add_argument("--repo", required=True, help="OWNER/NAME") + parser.add_argument("--head", required=True, type=_sha, help="head commit SHA") + parser.add_argument("--base", required=True, type=_sha, help="merge base commit SHA") + args = parser.parse_args(argv) + if not collect._REPO_RE.match(args.repo): + parser.error("--repo must be OWNER/NAME") + return run(args.repo, args.head, args.base) + + +if __name__ == "__main__": + sys.exit(main()) From 20a4af0a0144564cb5fe897cf18d0a0201d535a2 Mon Sep 17 00:00:00 2001 From: Irving Popovetsky Date: Thu, 29 Oct 2026 18:37:58 -0700 Subject: [PATCH 2/3] Count received registry bytes instead of reserving each fetch's cap (K-20) Each registry fetch reserved min(128 MiB, room) before it started, so 8 fetches in flight held the whole 1 GiB budget by themselves. Once about 373 MiB had arrived, the next reservation failed at once and that worker failed every queued key in a row: front-end PR 2027 got a false "over the download byte budget" hard block on 42 names while it needed only 578 MB decoded. Now a fetch starts with its full cap while the decoded bytes received are under the budget; the overshoot is at most 8 response caps. The budget is 8 GiB decoded, a fail-closed backstop behind the per-response cap (memory) and the 240 s lookup budget (time). The harness lists the subjects of each check. Co-Authored-By: Claude Opus 5.5 --- docs/claude-review-design.md | 9 +- review/src/oc_review/collect.py | 47 +++++----- review/tests/unit/test_collect.py | 149 +++++++++++++++++++++++++++--- review/tools/dep_repro.py | 6 ++ 4 files changed, 172 insertions(+), 39 deletions(-) diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index c5bb740..766ae81 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -291,7 +291,7 @@ 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 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. + - `MAX_REGISTRY_BYTES = 8 GiB` (K-18, K-19, K-20): decoded response bytes for all lookups of a run. Only bytes received count, and nothing is reserved: a fetch starts, with its full response cap, only while fewer than 8 GiB have arrived, so a key fails with the byte-budget reason only when the budget is really spent. The fetches in flight can pass the budget by at most `REGISTRY_LOOKUP_CONCURRENCY` response caps (8 x 128 MiB, 1 GiB). A fetch that fails before a body arrives (timeout, network error, 5xx) counts nothing; a response read past its cap, or one that fails to decode, counts as its cap. 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). It is a backstop: memory is bounded per response by the cap and time by `REGISTRY_LOOKUP_BUDGET_SECONDS`, which binds first at the observed rate. 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. @@ -694,6 +694,7 @@ token lifetime: 10 minutes review.md, security.md, supply-chain.md schema/findings.schema.json schema/envelope.schema.json + tools/dep_repro.py developer tool: collect's dependency stage on two real commits, live registries tests/ unit/ no network, no API calls (unit/fixtures/eval-smoke: runner smoke case) fixtures/ the live eval cases: bundle.json, expected.json, source/ @@ -714,6 +715,7 @@ token lifetime: 10 minutes - `hard_blocks`: exactly the prefilter hard block check IDs of the bundle, each listed once. - `summary_excludes`: strings that must not appear in the rendered summary, as written or with the publisher's Markdown escapes removed. A mismatch names the index, never the string. - **Fixture builder:** a fixture's `bundle.json` is the trusted input for the gate's hard blocks, so it is never written by hand. `review/tests/eval_fixtures.py` (trusted code, offline) builds each bundle from the case's `source/` files: it synthesizes the GitHub and registry responses, runs the real `collect.collect()` with a fixed time and run ID, and writes `bundle.json`. A unit test rebuilds every case and fails when a committed bundle differs, so the hard blocks, signals and redaction in a fixture come from the real prefilter and collect. Unit tests also prove, with fake pass records and no API, that each hard-block case fails the gate whatever Claude returns, and that each case with `hard_blocks: []` has no hard block. The smoke case `review/tests/unit/fixtures/eval-smoke` is for the unit tests only and is not part of a live run (P2-3). +- **Dependency stage reproduction (local, manual):** `cd review && uv run python tools/dep_repro.py --repo OWNER/NAME --head HEAD_SHA --base MERGE_BASE_SHA` reads the two trees and the changed blobs through the REST API as data (never a checkout, install or build), runs `collect.dependency_facts` and the prefilter with the production limits against the live registries, and prints the keys, unique names, decoded and wire bytes, the lookup wall time, each failed lookup and every hard block and signal with a count. It is not imported by production code and not run in CI (K-20). - **Fixture evals (manual `eval.yml`, costs money):** the cases in `review/tests/fixtures`. Run on the PR branch after each prompt or model change, with owner approval of the `eval` environment. Passing evals gate the merge of a prompt or model change, and they also gate any move of a ruleset pin: a pin moves only to a SHA whose tree has passed the evals (a merge SHA with the same tree as the tested candidate counts). No required check enforces this gate, so the owner holds the merge until the evals pass. Record the results in that PR. The pass bar is one full run with every case matched (P2-4). A miss is fixed in the prompt or the code, then the full set runs again. An expectation changes only with a written reason in the PR. Minimum set, with the part that code decides (unit tests check it, and the live run again) and the part that the model decides (only the live run checks it): - clean small refactor -> `pass`, no high findings (`gate: pass`, `max_severity: medium`, no hard block). Code: no hard block. Model: the gate and the severities. - real bug (`if (user.role = "admin")`) -> verified `bug` finding (`bug` `low` or higher, `file`, `side: head`, `verified: true`). Code: no hard block, and `verified` comes from the citation check. Model: that the finding exists. @@ -991,8 +993,9 @@ 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. -- 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. +- 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, and a key past it fails with its own reason (K-20 replaced the up-front reservation of each fetch's cap with a count of received bytes and raised the budget to 8 GiB). 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 then, 8 GiB since K-20) 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. +- K-20. Byte budget counts received bytes, not reservations (pilot false hard block, front-end PR 2027 run 40985217145, 2026-10-29). After K-19 the PR still got "registry lookup failed: over the download byte budget" for the last 42 of its 206 names (sorted), all logged within 25 ms, although the run needed only 577.6 MB decoded (71.5 MB gzip) of the 1 GiB budget. Cause, reproduced locally with `review/tools/dep_repro.py` at `cf19216` (3, 0 and 2 false failures in three runs on the same blobs; the count depends on timing) and deterministically in a unit test (28 of 40 keys failed): each fetch reserved min(128 MiB, room) before it started, so 8 fetches in flight held the whole 1 GiB by themselves; once about 373 MiB had arrived only a few fetches fit, the next worker's reservation failed at once, and that worker then failed every queued key in a row while the others still held their reservations. A race, not an overspend. Now nothing is reserved: a fetch starts with its full cap while the decoded bytes received are under the budget, and a refusal means the budget is really spent. The overshoot is bounded by the fetches in flight, at most `REGISTRY_LOOKUP_CONCURRENCY` x `NPM_MAX_RESPONSE_BYTES` (1 GiB). `MAX_REGISTRY_BYTES` is 8 GiB decoded: about 14x PR 2027 (about 1 GiB of gzip wire at the observed 8x ratio), and more than the 1 GiB that 8 workers can have in flight. Memory does not grow with it, because each response is parsed and dropped (the per-response cap bounds memory), and time is bounded by the 240 s lookup budget, which binds first at the observed rate of about 64 MB/s decoded (about 15 GB in 240 s), so the byte budget is a fail-closed backstop against abuse. The budget keeps its own reason, the per-response cap, gzip handling and failure logging are unchanged, and a response over its cap or with a bad encoding still counts as its cap. The other run limits have no such cascade: the count cap is fixed before any lookup starts, and the time budget refuses only at or near the deadline, when no time is left. Validated with the harness after the fix: PR 2027 had no hard block in four runs (206 of 206 keys, five signals: four registry `prepare` scripts and one long line), and controls on `back-end`, `operationcode-pybot`, `claude-review-test` and three `front-end` lockfile changes had no lookup failure. 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 f1a200a..7c2971e 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -90,11 +90,15 @@ 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 -# 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 6.4x its body). -MAX_REGISTRY_BYTES = 1024 * 1024 * 1024 +# Decoded response bytes for all lookups of a run (K-18, K-19, K-20). Only +# bytes received count: a fetch starts only while fewer than this have +# arrived, so a run can pass it by at most the fetches in flight, each at most +# its response cap (8 x 128 MiB). It is a backstop on download volume: memory +# is bounded per response by the cap (a parsed packument is about 6.4x its +# body) and time by the lookup time budget, which binds first at the observed +# rate (about 64 MB/s decoded, so about 15 GB in 240 s). 8 GiB is about 14x +# the largest measured legitimate run (front-end PR 2027, 578 MB decoded). +MAX_REGISTRY_BYTES = 8 * 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" @@ -682,7 +686,9 @@ def lookup_registry( 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. + is logged with its exception class. Only bytes received count against + `byte_budget`: no fetch starts once they reach it, and the fetches in + flight may pass it by at most `concurrency` response caps (K-20). 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 @@ -700,41 +706,36 @@ 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 + received = 0 # decoded response bytes of finished fetches def remaining() -> float: return deadline - clock() def bounded_fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: - nonlocal reserved + nonlocal received left = remaining() if left <= 0: raise _BudgetSpent("lookup time budget exceeded") + # No reservation (K-20): a fetch starts with its full cap while fewer + # than `byte_budget` bytes have arrived, so a refusal means the + # budget is really spent, never that fetches in flight hold it. with state: - room = byte_budget - reserved - if room <= 0: + if received >= byte_budget: raise _BytesSpent("download byte budget exceeded") - grant = min(max_bytes, room) - reserved += grant # 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. + # counts. A response that was read past its cap, or a body that did + # not decode, was a real download: it counts as its cap. got = 0 try: - result = fetch(url, grant, min(timeout, left)) + result = fetch(url, max_bytes, 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 + except (deps.ResponseTooLargeError, deps.ResponseEncodingError): + got = max_bytes raise finally: with state: - reserved -= grant - got - if grant < max_bytes and got > grant: - raise _BytesSpent("download byte budget exceeded") + received += got return result def bounded_sleep(seconds: float) -> None: diff --git a/review/tests/unit/test_collect.py b/review/tests/unit/test_collect.py index 6e4c6e5..64be4ed 100644 --- a/review/tests/unit/test_collect.py +++ b/review/tests/unit/test_collect.py @@ -10,6 +10,7 @@ import subprocess import sys import threading +from collections.abc import Callable from datetime import datetime from pathlib import Path from urllib.parse import urlsplit @@ -1182,6 +1183,14 @@ def test_lookup_limit_constants(): assert c.REGISTRY_LOOKUP_BUDGET_SECONDS <= 600 * 0.4 +def test_byte_budget_constant(): + # K-20: between 4 and 8 GiB decoded, well over the 578 MB of front-end PR + # 2027, and more than the bytes 8 workers can have in flight at their cap. + assert 4 * 1024**3 <= c.MAX_REGISTRY_BYTES <= 8 * 1024**3 + assert c.MAX_REGISTRY_BYTES >= 10 * 577_600_000 + assert c.MAX_REGISTRY_BYTES > c.REGISTRY_LOOKUP_CONCURRENCY * deps.NPM_MAX_RESPONSE_BYTES + + 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()) @@ -1221,13 +1230,13 @@ 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 (the cap) is cut to what is left, and - # the third response does not fit. + # 2500 bytes: the third fetch starts with 2000 received and may pass the + # budget (K-20); the fourth starts with 3000 received and is refused. registry, failed = c.lookup_registry( - _keys("a", "b", "c"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=1, byte_budget=2500 + _keys("a", "b", "c", "d"), 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} + assert sorted(k[1] for k in registry) == ["a", "b", "c"] + assert failed == {("npm", "d", "1.0.0"): c.LOOKUP_BYTES_BUDGET} def test_byte_budget_spent_before_a_fetch_starts(): @@ -1272,10 +1281,9 @@ 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). + # Budget 2500, cap 1000 (patched): each oversize download counts as its + # cap, so after three of them the fourth key finds the budget spent and + # gets the byte-budget reason, with no fetch. with pytest.MonkeyPatch.context() as mp: mp.setattr(deps, "NPM_MAX_RESPONSE_BYTES", 1000) _, failed = c.lookup_registry( @@ -1283,10 +1291,124 @@ def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: ) 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", "c", "1.0.0")] == c.LOOKUP_RESPONSE_TOO_LARGE assert failed[("npm", "d", "1.0.0")] == c.LOOKUP_BYTES_BUDGET +def _named_fetch(size: int, gate: Callable[[int], None] = lambda n: None): + """A fetch that returns a packument of `size` bytes for every name, after + `gate(call number)` returns, and records the most fetches in flight.""" + body = _sized_packument("a", size) + lock = threading.Lock() + stats = {"in_flight": 0, "peak": 0, "calls": 0} + + def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: + with lock: + stats["in_flight"] += 1 + stats["calls"] += 1 + stats["peak"] = max(stats["peak"], stats["in_flight"]) + n = stats["calls"] + try: + gate(n) + return deps.FetchResult(200, body.replace(b'"a"', b'"%s"' % url.rsplit("/", 1)[1].encode(), 1), url) + finally: + with lock: + stats["in_flight"] -= 1 + + return fetch, stats + + +def _names(n: int) -> list[str]: + return [f"p{i:03d}" for i in range(n)] + + +def _first_8_together(barrier: threading.Barrier) -> Callable[[int], None]: + def gate(n: int) -> None: + if n <= 8: + barrier.wait(timeout=5) + + return gate + + +def test_concurrent_large_fetches_under_the_budget_all_succeed(): + # 8 fetches near the cap are in flight at once and together need less + # than the budget, but less than 8 caps. Reserving the cap up front + # refused the 6th and failed it (pilot, front-end PR 2027). + fetch, stats = _named_fetch(900, _first_8_together(threading.Barrier(8))) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(deps, "NPM_MAX_RESPONSE_BYTES", 1000) + registry, failed = c.lookup_registry( + _keys(*_names(8)), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=8, byte_budget=7500 + ) + assert failed == {} + assert len(registry) == 8 and stats["peak"] == 8 + + +def test_a_busy_budget_does_not_cascade_through_the_queue(): + # 40 small keys need 4,000 of 5,000 bytes. The first 8 fetches are in + # flight together, so 8 caps (8,000) would pass the budget: with a + # reservation the 6th worker was refused at once and then failed every + # queued key in a row (pilot: the last 42 names within 25 ms). Only + # bytes received count, so nothing is refused and every key succeeds. + fetch, stats = _named_fetch(100, _first_8_together(threading.Barrier(8))) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(deps, "NPM_MAX_RESPONSE_BYTES", 1000) + registry, failed = c.lookup_registry( + _keys(*_names(40)), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=8, byte_budget=5000 + ) + assert failed == {} + assert len(registry) == 40 and stats["calls"] == 40 and stats["peak"] == 8 + + +def test_budget_exceeded_fails_only_keys_after_the_limit_with_bounded_overshoot(): + # Budget 1 byte: the first fetch of each worker starts with nothing + # received, so 8 fetches run and finish (overshoot at most 8 caps); every + # later key fails closed with the budget reason, and none starts a fetch. + fetch, stats = _named_fetch(900, _first_8_together(threading.Barrier(8))) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(deps, "NPM_MAX_RESPONSE_BYTES", 1000) + registry, failed = c.lookup_registry( + _keys(*_names(30)), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=8, byte_budget=1 + ) + assert len(registry) == 8 and stats["calls"] == 8 + assert stats["calls"] * 900 <= 1 + 8 * 1000 # received stays within budget + concurrency x cap + assert len(failed) == 22 and set(failed.values()) == {c.LOOKUP_BYTES_BUDGET} + + +def test_gzip_bomb_through_lookup_fails_closed_with_the_cap_reason(): + import gzip + + bomb = gzip.compress(b"\0" * 5_000_000) + + class Resp: + status = 200 + headers = {"Content-Encoding": "gzip"} + + def __init__(self, url): + self.url, self.chunks = url, [bomb] + + 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()) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(deps, "NPM_MAX_RESPONSE_BYTES", 100_000) + registry, failed = c.lookup_registry(_keys("a"), fetch=fetch, sleep=_no_sleep, clock=FakeClock()) + assert registry == {} and 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") @@ -1657,10 +1779,11 @@ def open(self, request, timeout): 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. + # Decoded size 4000 each: after two, 8000 decoded bytes have arrived, so + # an 8000 byte budget refuses the third, 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 + _keys("a", "b", "c"), fetch=fetch, sleep=_no_sleep, clock=FakeClock(), concurrency=1, byte_budget=8000 ) assert len(registry) + len(failed) == 3 assert list(failed.values()) == [c.LOOKUP_BYTES_BUDGET] diff --git a/review/tools/dep_repro.py b/review/tools/dep_repro.py index 3852519..4ff64b0 100644 --- a/review/tools/dep_repro.py +++ b/review/tools/dep_repro.py @@ -179,6 +179,12 @@ def run(repo: str, head: str, base: str) -> int: print(f"\n{kind}: {len(checks)}") for (cid, detail), n in sorted(counts.items()): print(f" {n:4d} {cid}: {detail}") + subjects = sorted({f"{c.file}: {c.subject}" if c.subject else str(c.file) + for c in checks if (c.id, c.detail) == (cid, detail)}) + for s in subjects[:20]: + print(f" {s!r}") + if len(subjects) > 20: + print(f" ... {len(subjects) - 20} more") return 0 From f11bea8a0204237b47e0debaa83506af93f753c6 Mon Sep 17 00:00:00 2001 From: Irving Popovetsky Date: Thu, 29 Oct 2026 18:51:02 -0700 Subject: [PATCH 3/3] Count partial registry reads and keep bounded script text (K-21) Review of K-20 found two gaps. A fetch that read part of a body and then failed (reset, timeout) counted 0 bytes, so repeated partial responses could pass the 8 GiB budget while the counter stayed at 0. default_fetch now sets received_bytes on the exception it raises, and collect counts it once per attempt; the failure keeps its own reason. A failure before any body still counts 0. Each result also kept the full text of its lifecycle scripts for the whole run and the bundle, so memory could grow with the received-byte budget. lookup_npm now keeps at most 1,000 characters of each script and notes the cut; every script name, the signal and the delta stay. The design doc also corrects which run limit binds first: at 64 MB/s decoded the byte budget arrives before the time budget. The harness meter now counts partial decoded bytes. Co-Authored-By: Claude Opus 5.5 --- docs/claude-review-design.md | 9 +-- review/src/oc_review/collect.py | 17 +++-- review/src/oc_review/deps.py | 36 ++++++++-- review/tests/unit/test_collect.py | 109 ++++++++++++++++++++++++++++++ review/tests/unit/test_deps.py | 47 +++++++++++++ review/tools/dep_repro.py | 1 + 6 files changed, 204 insertions(+), 15 deletions(-) diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 766ae81..52b5fd0 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -284,14 +284,14 @@ The review covers one immutable comparison pair: the merge base commit and the h - **Supported files:** `package.json`, `package-lock.json` (lockfile v2 and v3), `yarn.lock` (v1 and berry), `pnpm-lock.yaml` (v6 and later, one YAML document or the pnpm 12 pair of env and main documents, K-16), `pyproject.toml`, `poetry.lock`, `uv.lock`, `requirements*.txt`. The parser for each file family has fixtures for each supported version. - **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`). +- **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`). Each script keeps its name and at most its first 1,000 characters (`NPM_SCRIPT_TEXT_CHARS`), with a note of how many were cut (K-21). - **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, 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 = 8 GiB` (K-18, K-19, K-20): decoded response bytes for all lookups of a run. Only bytes received count, and nothing is reserved: a fetch starts, with its full response cap, only while fewer than 8 GiB have arrived, so a key fails with the byte-budget reason only when the budget is really spent. The fetches in flight can pass the budget by at most `REGISTRY_LOOKUP_CONCURRENCY` response caps (8 x 128 MiB, 1 GiB). A fetch that fails before a body arrives (timeout, network error, 5xx) counts nothing; a response read past its cap, or one that fails to decode, counts as its cap. 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). It is a backstop: memory is bounded per response by the cap and time by `REGISTRY_LOOKUP_BUDGET_SECONDS`, which binds first at the observed rate. + - `MAX_REGISTRY_BYTES = 8 GiB` (K-18, K-19, K-20, K-21): decoded response bytes for all lookups of a run. Only bytes received count, and nothing is reserved: a fetch starts, with its full response cap, only while fewer than 8 GiB have arrived, so a key fails with the byte-budget reason only when the budget is really spent. The fetches in flight can pass the budget by at most `REGISTRY_LOOKUP_CONCURRENCY` response caps (8 x 128 MiB, 1 GiB). Each attempt counts once. A fetch that fails before a body arrives (5xx, refused connection) counts nothing; one that fails after part of a body arrived (reset, timeout) counts the decoded bytes it read (K-21); a response read past its cap, or one that fails to decode, counts as its cap. 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). It is a fail-closed backstop on download volume. Which run limit is reached first depends on response sizes and throughput: at the observed 64 MB/s decoded, 8 GiB arrives in about 134 s, before the 240 s time budget. Memory does not grow with the budget: it holds at most `REGISTRY_LOOKUP_CONCURRENCY` responses and their parses at once, plus the results, whose only text from a response is the cut script text (K-21). 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. @@ -993,9 +993,10 @@ 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, and a key past it fails with its own reason (K-20 replaced the up-front reservation of each fetch's cap with a count of received bytes and raised the budget to 8 GiB). 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-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 (with the retained results, which K-21 bounds by the key count). 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, and a key past it fails with its own reason (K-20 replaced the up-front reservation of each fetch's cap with a count of received bytes and raised the budget to 8 GiB). 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 then, 8 GiB since K-20) 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. -- K-20. Byte budget counts received bytes, not reservations (pilot false hard block, front-end PR 2027 run 40985217145, 2026-10-29). After K-19 the PR still got "registry lookup failed: over the download byte budget" for the last 42 of its 206 names (sorted), all logged within 25 ms, although the run needed only 577.6 MB decoded (71.5 MB gzip) of the 1 GiB budget. Cause, reproduced locally with `review/tools/dep_repro.py` at `cf19216` (3, 0 and 2 false failures in three runs on the same blobs; the count depends on timing) and deterministically in a unit test (28 of 40 keys failed): each fetch reserved min(128 MiB, room) before it started, so 8 fetches in flight held the whole 1 GiB by themselves; once about 373 MiB had arrived only a few fetches fit, the next worker's reservation failed at once, and that worker then failed every queued key in a row while the others still held their reservations. A race, not an overspend. Now nothing is reserved: a fetch starts with its full cap while the decoded bytes received are under the budget, and a refusal means the budget is really spent. The overshoot is bounded by the fetches in flight, at most `REGISTRY_LOOKUP_CONCURRENCY` x `NPM_MAX_RESPONSE_BYTES` (1 GiB). `MAX_REGISTRY_BYTES` is 8 GiB decoded: about 14x PR 2027 (about 1 GiB of gzip wire at the observed 8x ratio), and more than the 1 GiB that 8 workers can have in flight. Memory does not grow with it, because each response is parsed and dropped (the per-response cap bounds memory), and time is bounded by the 240 s lookup budget, which binds first at the observed rate of about 64 MB/s decoded (about 15 GB in 240 s), so the byte budget is a fail-closed backstop against abuse. The budget keeps its own reason, the per-response cap, gzip handling and failure logging are unchanged, and a response over its cap or with a bad encoding still counts as its cap. The other run limits have no such cascade: the count cap is fixed before any lookup starts, and the time budget refuses only at or near the deadline, when no time is left. Validated with the harness after the fix: PR 2027 had no hard block in four runs (206 of 206 keys, five signals: four registry `prepare` scripts and one long line), and controls on `back-end`, `operationcode-pybot`, `claude-review-test` and three `front-end` lockfile changes had no lookup failure. No gate rule changes, and nothing lowers risk. +- K-20. Byte budget counts received bytes, not reservations (pilot false hard block, front-end PR 2027 run 40985217145, 2026-10-29). After K-19 the PR still got "registry lookup failed: over the download byte budget" for the last 42 of its 206 names (sorted), all logged within 25 ms, although the run needed only 577.6 MB decoded (71.5 MB gzip) of the 1 GiB budget. Cause, reproduced locally with `review/tools/dep_repro.py` at `cf19216` (3, 0 and 2 false failures in three runs on the same blobs; the count depends on timing) and deterministically in a unit test (28 of 40 keys failed): each fetch reserved min(128 MiB, room) before it started, so 8 fetches in flight held the whole 1 GiB by themselves; once about 373 MiB had arrived only a few fetches fit, the next worker's reservation failed at once, and that worker then failed every queued key in a row while the others still held their reservations. A race, not an overspend. Now nothing is reserved: a fetch starts with its full cap while the decoded bytes received are under the budget, and a refusal means the budget is really spent. The overshoot is bounded by the fetches in flight, at most `REGISTRY_LOOKUP_CONCURRENCY` x `NPM_MAX_RESPONSE_BYTES` (1 GiB). `MAX_REGISTRY_BYTES` is 8 GiB decoded: about 14x PR 2027 (about 1 GiB of gzip wire at the observed 8x ratio), and more than the 1 GiB that 8 workers can have in flight. Time is bounded separately by the 240 s lookup budget. Which limit is reached first depends on response sizes and throughput: at the observed rate of about 64 MB/s decoded, 8 GiB arrives in about 134 s (under 151 s with the 1 GiB overshoot), before the time budget (corrected by K-21; this text first said the time budget binds first). The byte budget is a fail-closed backstop against abuse. Memory does not grow with the budget only because the results keep bounded text: see K-21 (this text first said that each response is parsed and dropped, which missed the script text kept in the results). The budget keeps its own reason, the per-response cap, gzip handling and failure logging are unchanged, and a response over its cap or with a bad encoding still counts as its cap. The other run limits have no such cascade: the count cap is fixed before any lookup starts, and the time budget refuses only at or near the deadline, when no time is left. Validated with the harness after the fix: PR 2027 had no hard block in four runs (206 of 206 keys, five signals: four registry `prepare` scripts and one long line), and controls on `back-end`, `operationcode-pybot`, `claude-review-test` and three `front-end` lockfile changes had no lookup failure. No gate rule changes, and nothing lowers risk. +- K-21. Partial reads count, and results keep bounded script text (review of K-20, 2026-10-29). Three corrections to K-20. (1) A fetch that read part of a body and then failed (connection reset, timeout) counted 0 bytes, because the fetcher's decoded count was lost with the exception. Each of the 3 attempts of a key could read almost 128 MiB, so 24 names that each sent 127 MiB and then reset used about 9,144 MiB, past the 8 GiB budget and its 1 GiB overshoot, while the counter stayed at 0. Now `default_fetch` sets `received_bytes` on the exception it raises to the decoded bytes read, and collect counts them once per attempt. The failure keeps its own reason; a later key then gets "over the download byte budget". A failure before any body still counts 0. (2) K-20 said memory does not grow with the budget because each response is parsed and dropped. But each result kept the full text of its lifecycle scripts, the results stay for the run, and the bundle repeats them, so many valid packuments with long scripts could grow memory and the bundle toward the received-byte budget. Now `lookup_npm` keeps at most `NPM_SCRIPT_TEXT_CHARS` (1,000) characters of each script, then " ... [N more characters cut]". It keeps every script name, so the signal "registry manifest has install scripts" and the delta are unchanged, and the supply-chain pass still sees how each script starts. Memory bound: at most 8 responses and their parses at once (K-19, about 6.4 GiB worst case), plus the results: at most 3,000 keys x 7 scripts x about 1,030 characters, about 22 million characters (under 90 MB as Python strings), plus their copies in the bundle, at most 7 x about 1,030 characters of script text per changed package. Nothing else from a response is kept (publish time, wheel flag). (3) K-20 also said the time budget binds first; at 64 MB/s decoded the byte budget does (about 134 s for 8 GiB); which one binds depends on response sizes and throughput. Validated with the harness on PR 2027 (two runs) and the K-20 controls (one run each); results unchanged. 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 7c2971e..954a7b4 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -687,8 +687,9 @@ def lookup_registry( 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. Only bytes received count against - `byte_budget`: no fetch starts once they reach it, and the fetches in - flight may pass it by at most `concurrency` response caps (K-20). + `byte_budget`, including the part of a body a failed fetch read (K-21): + no fetch starts once they reach it, and the fetches in flight may pass it + by at most `concurrency` response caps (K-20). 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 @@ -722,10 +723,11 @@ def bounded_fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: with state: if received >= byte_budget: raise _BytesSpent("download byte budget exceeded") - # Bytes that count against the budget. A fetch that fails before a - # body arrives (timeout, network error, 5xx) downloaded nothing that - # counts. A response that was read past its cap, or a body that did - # not decode, was a real download: it counts as its cap. + # Bytes that count against the budget, once per attempt. A fetch that + # fails before a body arrives (5xx, refused connection) counts 0; one + # that fails after part of a body (reset, timeout) counts what it + # read (K-21). A response that was read past its cap, or a body that + # did not decode, was a real download: it counts as its cap. got = 0 try: result = fetch(url, max_bytes, min(timeout, left)) @@ -733,6 +735,9 @@ def bounded_fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: except (deps.ResponseTooLargeError, deps.ResponseEncodingError): got = max_bytes raise + except Exception as exc: + got = deps.received_bytes(exc) + raise finally: with state: received += got diff --git a/review/src/oc_review/deps.py b/review/src/oc_review/deps.py index 507940d..608b563 100644 --- a/review/src/oc_review/deps.py +++ b/review/src/oc_review/deps.py @@ -57,6 +57,9 @@ # checks alike: the four install scripts, plus the three npm also runs on # install. NPM_LIFECYCLE_SCRIPTS = ("preinstall", "install", "postinstall", "prepare", "preprepare", "postprepare", "prepublish") +# A lookup keeps at most this many characters of each registry lifecycle +# script, so the results of a run stay small whatever the byte budget (K-21). +NPM_SCRIPT_TEXT_CHARS = 1000 FAMILIES = ( "package.json", @@ -1481,10 +1484,27 @@ class FetchResult: # fetch(url, max_bytes, timeout_seconds) -> FetchResult. It raises OSError, -# TimeoutError or http.client.HTTPException on a network failure. +# TimeoutError or http.client.HTTPException on a network failure. A failure +# after part of the body arrived sets `received_bytes` on the exception to the +# decoded bytes read (K-21). Fetcher = Callable[[str, int, float], FetchResult] +def received_bytes(exc: BaseException) -> int: + """Decoded body bytes a fetch read before it raised `exc`; 0 when the + fetcher did not set them.""" + value = getattr(exc, "received_bytes", 0) + return value if isinstance(value, int) and value > 0 else 0 + + +def _with_received(exc: BaseException, size: int) -> BaseException: + try: + exc.received_bytes = size # type: ignore[attr-defined] + except AttributeError: + pass + return exc + + class _NoRedirect(urllib.request.HTTPRedirectHandler): def redirect_request(self, *args: Any, **kwargs: Any) -> None: # noqa: D401 return None @@ -1511,6 +1531,7 @@ def default_fetch( headers={"Accept": "application/json", "Accept-Encoding": "gzip", "User-Agent": "oc-claude-review"}, ) deadline = clock() + timeout + size = 0 # decoded bytes; a failure reports them as `received_bytes` try: with opener.open(request, timeout=timeout) as resp: encoding = (resp.headers.get("Content-Encoding") or "identity").strip().lower() @@ -1519,7 +1540,6 @@ def default_fetch( # 31 = gzip wrapper only (no zlib or raw deflate). inflater = zlib.decompressobj(31) if encoding != "identity" else None chunks: list[bytes] = [] - size = 0 # decoded bytes wire = 0 # encoded bytes while size <= max_bytes: if clock() > deadline: @@ -1554,7 +1574,9 @@ def default_fetch( except urllib.error.HTTPError as exc: return FetchResult(status=exc.code, body=b"", url=url) except http.client.HTTPException as exc: - raise OSError(f"{url}: {type(exc).__name__}: {exc}") from exc + raise _with_received(OSError(f"{url}: {type(exc).__name__}: {exc}"), size) from exc + except Exception as exc: + raise _with_received(exc, size) @dataclass(frozen=True) @@ -1653,9 +1675,13 @@ def lookup_npm( found = {} for key in NPM_LIFECYCLE_SCRIPTS: if key in scripts: - if not isinstance(scripts[key], str): + text = scripts[key] + if not isinstance(text, str): raise RegistryError(f"{url}: script {key} is not a string") - found[key] = scripts[key] + if len(text) > NPM_SCRIPT_TEXT_CHARS: + cut = len(text) - NPM_SCRIPT_TEXT_CHARS + text = f"{text[:NPM_SCRIPT_TEXT_CHARS]} ... [{cut} more characters cut]" + found[key] = text return RegistryInfo( ecosystem="npm", name=name, diff --git a/review/tests/unit/test_collect.py b/review/tests/unit/test_collect.py index 64be4ed..224f673 100644 --- a/review/tests/unit/test_collect.py +++ b/review/tests/unit/test_collect.py @@ -1295,6 +1295,115 @@ def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: assert failed[("npm", "d", "1.0.0")] == c.LOOKUP_BYTES_BUDGET +class _ResetOpener: + """An opener for default_fetch: each response sends `sent` bytes, then + the connection resets. With `sent` None, the connection fails before any + body arrives. Records each URL opened.""" + + def __init__(self, sent: int | None) -> None: + self.sent = sent + self.opened: list[str] = [] + + def open(self, request, timeout): + self.opened.append(request.full_url) + if self.sent is None: + raise ConnectionRefusedError("connection refused") + chunks = [b"x" * self.sent] + + class Resp: + status = 200 + url = request.full_url + headers: dict = {} + + def read(self, n): + if not chunks: + raise ConnectionResetError("connection reset by peer") + return chunks.pop() + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + return Resp() + + +def test_partial_reads_that_fail_spend_the_byte_budget(): + # K-21: each attempt of "a" reads 100 bytes, then the connection resets. + # Its three attempts count 300 bytes, past the 250 byte budget, so "a" + # keeps the generic reason and "b" gets the byte-budget reason, with no + # fetch. Before K-21 the partial bytes counted 0 and "b" was fetched. + opener = _ResetOpener(100) + + def fetch(url, max_bytes, timeout): + return deps.default_fetch(url, max_bytes, timeout, opener=opener) + + registry, failed = c.lookup_registry( + _keys("a", "b"), fetch=fetch, sleep=lambda s: None, clock=FakeClock(), concurrency=1, byte_budget=250 + ) + assert registry == {} + assert failed == {("npm", "a", "1.0.0"): c.LOOKUP_FAILED, ("npm", "b", "1.0.0"): c.LOOKUP_BYTES_BUDGET} + assert opener.opened == ["https://registry.npmjs.org/a"] * 3 + + +def test_a_failure_before_any_body_spends_no_bytes(): + opener = _ResetOpener(None) + + def fetch(url, max_bytes, timeout): + return deps.default_fetch(url, max_bytes, timeout, opener=opener) + + _, failed = c.lookup_registry( + _keys("a", "b"), fetch=fetch, sleep=lambda s: None, clock=FakeClock(), concurrency=1, byte_budget=1 + ) + assert set(failed.values()) == {c.LOOKUP_FAILED} + assert len(opener.opened) == 6 + + +def test_registry_results_keep_bounded_script_text(): + # K-21: lifecycle scripts of 1 MB each, in 20 packages that run in + # several successive groups. Each result keeps at most + # NPM_SCRIPT_TEXT_CHARS of each script, so the results and the bundle + # do not grow with the response sizes; every script name, the signal + # and the start of each script stay. + big = "node setup.js && " + "x" * 1_000_000 + names = _names(20) + lock = json.dumps({ + "name": "x", "lockfileVersion": 3, "requires": True, + "packages": { + "": {"name": "x", "dependencies": {n: "1.0.0" for n in names}}, + **{ + f"node_modules/{n}": {"version": "1.0.0", "resolved": f"https://registry.npmjs.org/{n}/-/{n}-1.0.0.tgz"} + for n in names + }, + }, + }) + fx = _snapshot({}, {"package-lock.json": ("100644", lock)}) + fx["registry"] = { + f"https://registry.npmjs.org/{n}": {"status": 200, "json": { + "name": n, + "time": {"1.0.0": "2018-01-01T00:00:00.000Z"}, + "versions": {"1.0.0": {"name": n, "scripts": {"postinstall": big, "prepare": "tsc", "test": big}}}, + }} + for n in names + } + col, _, _ = run(fx) + changes = col.envelope["payload"]["dependency_delta"]["changes"] + assert len(changes) == 20 + limit = deps.NPM_SCRIPT_TEXT_CHARS + 50 + for ch in changes: + scripts = ch["registry"]["install_scripts"] + assert set(scripts) == {"postinstall", "prepare"} + assert scripts["prepare"] == "tsc" + assert scripts["postinstall"].startswith("node setup.js && xxx") + assert len(scripts["postinstall"]) <= limit + assert len(col.data) < 20 * 2 * limit + 100_000 + signals = col.envelope["payload"]["prefilter"]["signals"] + details = [s["detail"] for s in signals if s["id"] == prefilter.INSTALL_SCRIPT_DEP] + assert details == ["registry manifest has install scripts: postinstall, prepare"] * 20 + assert col.envelope["payload"]["prefilter"]["hard_blocks"] == [] + + def _named_fetch(size: int, gate: Callable[[int], None] = lambda n: None): """A fetch that returns a packument of `size` bytes for every name, after `gate(call number)` returns, and records the most fetches in flight.""" diff --git a/review/tests/unit/test_deps.py b/review/tests/unit/test_deps.py index e37e938..d0c29e2 100644 --- a/review/tests/unit/test_deps.py +++ b/review/tests/unit/test_deps.py @@ -807,6 +807,19 @@ def test_lookup_npm_keeps_every_install_lifecycle_script(): assert info.install_scripts == {"preprepare": "a", "postprepare": "b", "prepublish": "c"} +def test_lookup_npm_keeps_bounded_script_text(): + # K-21: a long script keeps its first NPM_SCRIPT_TEXT_CHARS characters and + # a note of the cut; a script at the limit is unchanged. + n = deps.NPM_SCRIPT_TEXT_CHARS + scripts = {"postinstall": "curl x | sh; " + "y" * 5000, "prepare": "z" * n} + doc = {"name": "a", "time": {"1.0.0": "2024-01-01T00:00:00Z"}, "versions": {"1.0.0": {"scripts": scripts}}} + info = deps.lookup_npm("a", "1.0.0", FakeFetch(ok(doc, "https://registry.npmjs.org/a"))) + assert set(info.install_scripts) == {"postinstall", "prepare"} + cut = len(scripts["postinstall"]) - n + assert info.install_scripts["postinstall"] == scripts["postinstall"][:n] + f" ... [{cut} more characters cut]" + assert info.install_scripts["prepare"] == scripts["prepare"] + + @pytest.mark.parametrize("stamp", ["2020-01-01T00:00:00", "2020-01-01T00:00:00.000", "2020-01-01"]) def test_lookup_npm_time_without_time_zone_is_incomplete(stamp): doc = {"name": "a", "time": {"1.0.0": stamp}, "versions": {"1.0.0": {}}} @@ -1285,6 +1298,40 @@ def test_default_fetch_total_deadline_and_httplib_errors(): ) +class _ResetResp(_Resp): + """Sends its chunks, then the connection resets.""" + + def read(self, n): + if not self.chunks: + raise ConnectionResetError("connection reset by peer") + return super().read(n) + + +def test_default_fetch_failure_reports_the_decoded_bytes_read(): + # K-21: a failure after part of a body reports the decoded bytes read, + # for identity and gzip bodies; a failure before any body reports 0. + with pytest.raises(ConnectionResetError) as reset: + deps.default_fetch("https://registry.npmjs.org/a", 100, 5.0, opener=_Opener(_ResetResp([b"abcd", b"ef"]))) + assert deps.received_bytes(reset.value) == 6 + body = _gz(b"x" * 400) + with pytest.raises(ConnectionResetError) as reset: + deps.default_fetch( + "https://registry.npmjs.org/a", 1000, 5.0, + opener=_Opener(_ResetResp([body[:-8]], headers={"Content-Encoding": "gzip"})), + ) + assert deps.received_bytes(reset.value) == 400 + ticks = iter([0.0, 1.0, 99.0]) + with pytest.raises(TimeoutError) as late: + deps.default_fetch( + "https://registry.npmjs.org/a", 100, 5.0, opener=_Opener(_Resp([b"abc", b"d"])), clock=lambda: next(ticks) + ) + assert deps.received_bytes(late.value) == 3 + with pytest.raises(ConnectionRefusedError) as refused: + deps.default_fetch("https://registry.npmjs.org/a", 100, 5.0, opener=_Opener(ConnectionRefusedError("no"))) + assert deps.received_bytes(refused.value) == 0 + assert deps.received_bytes(ValueError("other fetcher")) == 0 + + def _gz(data: bytes) -> bytes: import gzip diff --git a/review/tools/dep_repro.py b/review/tools/dep_repro.py index 4ff64b0..5c63207 100644 --- a/review/tools/dep_repro.py +++ b/review/tools/dep_repro.py @@ -102,6 +102,7 @@ def fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult: except Exception as exc: with meter.lock: meter.errors[type(exc).__name__] += 1 + meter.decoded += deps.received_bytes(exc) raise with meter.lock: meter.decoded += len(result.body)