Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions docs/claude-review-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<encoded-name>` and `https://pypi.org/pypi/<name>/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/<encoded-name>` and `https://pypi.org/pypi/<name>/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.
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, 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 <ecosystem> '<name>' '<version>': <exception class>: <short reason>`. 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.
Expand Down Expand Up @@ -990,6 +991,8 @@ 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: <reason>") 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[<version>]` 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.

Expand Down
Loading
Loading