Skip to content

Fix the npm byte-budget race; add a local dependency reproduction harness - #22

Merged
irvingpop merged 3 commits into
mainfrom
irvingpop/phase4-p
Oct 30, 2026
Merged

irvingpop merged 3 commits into
mainfrom
irvingpop/phase4-p

Conversation

@irvingpop

Copy link
Copy Markdown
Member

Fixes the false "over the download byte budget" blocks on front-end#2027 (42 packages at release cf19216), and adds the local check the owner asked for before any dependency release.

Cause (reproduced locally). bounded_fetch reserved the full 128 MiB per-response cap before each download. Eight workers held the whole 1 GiB budget while only about 390 MB had arrived; a refused reservation failed instantly, so one worker drained the rest of the queue in about 25 ms. A race, not an overspend: the PR needs 577.6 MB decoded (71.5 MB gzip), and lookups took 9 s of 240 s. Local harness at cf19216: 3, 0 and 2 false failures in three live runs (timing-dependent); a deterministic unit test fails 28 of 40 keys on the old code.

Fix.

  • No reservation: count decoded bytes actually received (including partial reads from failed attempts, once per attempt, under the lock); refuse a new fetch only when received >= budget. Overshoot bounded by workers x per-response cap.
  • MAX_REGISTRY_BYTES = 8 GiB decoded (about 14x this PR); the 240 s run budget and the per-response cap still bound time and memory.
  • Kept registry data per version holds at most 1,000 characters of each lifecycle script, visibly marked as cut for the model. Detection of new or changed install scripts uses script names and the full package.json text, not the cut text.
  • Design doc K-20, K-21; K-18/K-19 corrected.

New tool. review/tools/dep_repro.py runs the same collect dependency stage (same parser, delta, limits, worker count) on two lockfile blobs fetched through the REST API, against live registries. It never runs PR code. Use it before any dependency-check release.

Local validation. front-end#2027: 0 hard blocks in 6 harness runs (206/206 packages). Controls (back-end, operationcode-pybot, claude-review-test, front-end, a one-package change): no lookup failures; only true 72-hour fresh-publish blocks. Stress: real budget overrun, oversized document, gzip bomb and 8 large concurrent downloads behave as designed. Unit tests 2711 passed; release tests 2 passed. Eval not run.

Review. Adversarial review (other model family) found 2 Important + 1 Minor; all fixed with regression tests that fail on the previous commit; scoped re-review clean, including a trace of every use of the cut script text. Open minor: the cut marker is plain text inside the script string and the prompts do not explain it (it cannot lower risk).

Goes live only with a release.

🤖 Generated with Claude Code

irvingpop and others added 3 commits October 29, 2026 18:31
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@irvingpop
irvingpop requested a review from a team as a code owner October 30, 2026 01:55
@github-actions

Copy link
Copy Markdown

🔴 FAIL: OC Claude review did not complete

Caution

Incomplete file coverage blocks this PR. No waiver can clear it.
Next step for the author: fix the cause named in the row, then push. The review runs again on the new commit.

Severity Effect Where Finding Found by
🚫 Hard block Blocks, no waiver review/tests/unit/test_collect.py Incomplete file coverage: file content not scanned: unterminated-private-key, in review/tests/unit/test_collect.py File checks (code, no model)
⚠️ Not run Blocks, no waiver Review pass authentication failed. The pass returned no result. Workflow (code, no model)
⚠️ Not run Blocks, no waiver Security pass authentication failed. The pass returned no result. Workflow (code, no model)
⚠️ Not run Blocks, no waiver Supply chain pass authentication failed. The pass returned no result. Workflow (code, no model)
⚠️ Not run Blocks, no waiver Gate A required job did not succeed: dependency-review: failure Workflow (code, no model)
⚠️ Not run Blocks, no waiver Gate Dependency review did not succeed: job result failure Workflow (code, no model)
⚠️ Not run Blocks, no waiver Gate Dependency review did not succeed: step outcome failure Workflow (code, no model)
File checks Review Security Supply chain Dependency review Coverage
🚫 1 hard block, 1 signal ❌ failed ❌ failed ❌ failed ❌ failed 🚫 incomplete

Run 40988632863, attempt 1 for head f11bea8 into main. The required workflow in the merge box is the result that counts. This bot never approves a PR.

📋 Full review record: reasons, checks, signals, finding details, model summaries, token usage

Reviewed snapshot

Head SHA Merge base SHA Base branch Run
f11bea8a0204237b47e0debaa83506af93f753c6 cf192165e1306772370f64af96b0d6d2e53da0af main 40988632863, attempt 1

Reasons the gate failed

Reason Waivable? Detail
A required job did not succeed 🚫 no dependency-review: failure
Dependency review did not succeed 🚫 no job result failure
Dependency review did not succeed 🚫 no step outcome failure
A review pass failed 🚫 no pass review failed: authentication failed
A review pass failed 🚫 no pass security failed: authentication failed
A review pass failed 🚫 no pass supply-chain failed: authentication failed
Hard block from the file checks 🚫 no incomplete-coverage in review/tests/unit/test_collect.py
Hard block from the file checks 🚫 no incomplete-coverage (coverage record is not complete)

No waiver line is shown. At least one reason cannot be waived, so a waiver would not change the result.

Checks (code, no model)

Check Result Detail
Hard block 🚫 blocks Incomplete file coverage: file content not scanned: unterminated-private-key, in review/tests/unit/test_collect.py
Coverage 🚫 NOT complete files in comparison 6, files collected 6, trees complete
Unavailable file ⚠️ unavailable review/tests/unit/test_collect.py: unterminated-private-key
Dependency changes ✅ none
Dependency review job ❌ failed

Signals (context only, never block)

  • ℹ️ Very long line: lines longer than 500 characters, in docs/claude-review-design.md, lines 287 294 718 996 997 998 999

Dependency changes

None.

Finding details

Model output from untrusted input. Check it before you act on it. Verified means only that the quoted text is at that place. It does not mean the claim is true.

Blocking is decided by code from category and severity, never by the model. A finding in a blocking category blocks only at high or critical severity.

No findings from the passes that ran. The Review, Security, Supply chain pass did not run, so this is not a clean result.

Model summaries

Model output from untrusted input. Check it before you act on it. Verified means only that the quoted text is at that place. It does not mean the claim is true.

Pass Status Model Summary
Review ❌ failed No summary. Reason: authentication failed
Security ❌ failed No summary. Reason: authentication failed
Supply chain ❌ failed No summary. Reason: authentication failed

Token usage

Pass Input Output Cache write Cache read Attempts
Review not recorded not recorded not recorded not recorded not recorded
Security not recorded not recorded not recorded not recorded not recorded
Supply chain not recorded not recorded not recorded not recorded not recorded
Recorded total not recorded not recorded not recorded not recorded not recorded

@irvingpop
irvingpop merged commit 96f4966 into main Oct 30, 2026
3 of 5 checks passed
@irvingpop
irvingpop deleted the irvingpop/phase4-p branch October 30, 2026 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant