install: hash patch contents with SHA-1 instead of Wyhash11

[robobun]

Fixes the patchedDependencies cache-poisoning site from #32741, and adds regression coverage for the SQL prepared-statement collision.

patchedDependencies contents_hash

The shared-cache folder for a patched package is named <pkg>@<ver>_patch_hash=<hex>, and the installed tree is verified by a .bun-patch-hash-<hex> tag file. Both were the Wyhash11(seed=0) of the patch file's raw bytes, so two projects with different patches that collide under Wyhash11 shared one cache folder and the second project observed the first project's patched package instead of applying its own patch:

patch A (AAAAAAAA) and patch B (BBBBBBBB) both hash to 0x429d7ca64c60f3d1
-> is-even@1.0.0@@@1_patch_hash=429d7ca64c60f3d1 (one dir)
-> projB/node_modules/is-even/m.js == ...AAAAAAAA...

Fix: derive the u64 from SHA-1 instead of Wyhash11. The u64 interface (folder suffix, tag filename, in-memory PatchedDep) is unchanged, and a constructed second preimage now costs 2^64 work. Existing node_modules re-patch once because the tag file name differs. There is no in-memory map to re-key here: the identity is a directory name derived from file contents, so the hash itself has to be collision resistant.

Hash the whole file

The chunked read loop (pre-existing, inherited from the Wyhash11 version) called File::read_fill_buf, which always reads from file offset 0. Each iteration therefore returned the same leading bytes, and the loop fed that first chunk into the hasher repeatedly until it had counted the file's size. Any two patch files with an identical first chunk hashed equal no matter what followed, which defeats the point of a collision-resistant hash. The loop now uses pread_all at an explicit running offset, 64 KiB at a time. A second test covers it: two patches sharing an 80 KiB identical prefix and differing only in the final line must land in distinct cache entries (fails on the released binary, where project B receives project A's patched package).

calc_hash was the only caller of bun_sys::File::read_fill_buf, so that helper is deleted in the same change.

SQL prepared-statement cache (regression test only)

The MySQL/Postgres prepared-statement caches were keyed on wyhash(signature.name) with identity equality, so two distinct queries whose names collided under that hash shared one server-side statement. That fix — re-keying the caches on the signature.name bytes via StringHashMap so the map's own equality distinguishes colliding queries — landed independently on main while this PR was in review. This PR keeps the regression test (test/js/sql/sql-statement-cache-hash-collision.test.ts): it builds wyhash-colliding query pairs for MySQL and Postgres and asserts each query returns its own result, plus a Postgres case where the colliding query fails at Parse. The tests pass against main's name-keyed caches and guard the invariant against a future regression to hash-keying.

Rebase note

Rebased onto main. Main had independently landed the SQL prepared-statement name-keying fix (both MySQL and Postgres, via StringHashMap<*mut Statement>), so all of this PR's SQL source changes were superseded and dropped in the rebase; only the patch contents_hash source change and the two regression tests remain. The earlier transpiler-cache changes were already reverted at Jarred's request before the rebase.

Verification

bun bd test test/cli/install/bun-install-patch.test.ts -t 'collided under the old wyhash'
bun bd test test/js/sql/sql-statement-cache-hash-collision.test.ts

The patch-hash test installs two projects with a shared BUN_INSTALL_CACHE_DIR plus a non-colliding control project; it fails before the SHA-1 swap (USE_SYSTEM_BUN=1) and passes after. The SQL tests pass against main's name-keyed caches.

---

no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-patch.test.ts

[robobun]

^{Updated 5:21 PM PT - Aug 11th, 2026}

✅ @robobun, your commit 05b6dc36dafc030ee165c9d3ac224261966183d8 passed in Build #92633! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 32749

That installs a local version of the PR into your bun-32749 executable, so you can run:

bun-32749 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Patch hashing now derives a u64 from SHA-1, MySQL and Postgres prepared-statement caches key entries by signature name bytes, and new tests cover cache-collision behavior for install and SQL paths.

Changes

Collision-resistant cache keys

Layer / File(s)	Summary
SHA-1 patch hash src/install/patch_install.rs, test/cli/install/bun-install-patch.test.ts	PatchTask::calc_hash now streams patch bytes into SHA-1 and truncates the digest to u64; the install test checks separate cache entries for colliding patch contents.
MySQL signature-name key src/sql_jsc/mysql/MySQLConnection.rs, src/sql_jsc/mysql/JSMySQLConnection.rs, src/sql_jsc/mysql/MySQLQuery.rs	MySQL prepared-statement cache aliases and lookup helpers use signature.name bytes as the key, and run_prepared_query calls the renamed getter.
Postgres signature-name key src/sql_jsc/postgres/PostgresSQLConnection.rs, src/sql_jsc/postgres/PostgresSQLQuery.rs	Postgres prepared-statement caches use signature.name bytes directly, and query/error failure paths remove entries by name instead of by hashed signature.
SQL collision tests test/js/sql/sql-statement-cache-hash-collision.test.ts	The SQL regression suite builds colliding query pairs for MySQL and Postgres, checks each query returns its own marker, and covers the Postgres parse-failure cleanup path.

Possibly related PRs

• oven-sh/bun#32209: Nearby prepared-statement hash handling changed in the same area, including removal of Signature::hash().

Suggested reviewers

• Jarred-Sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the primary source change: replacing Wyhash11 with SHA-1 for patch-content hashing.
Description check	✅ Passed	The description explains the changes, scope, regression coverage, verification commands, and platform-specific test limitation.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/install/patch_install.rs`:
- Around line 701-706: Shorten the rationale comment in the patch hash section
so it fits the 3-line comment limit while preserving the key invariant that
SHA-1 truncated to 64 bits replaces Wyhash11 to avoid cache collisions. Update
the existing comment near the patch hash logic in patch_install.rs to be
concise, and keep the longer explanation only in the PR discussion. Focus on the
comment block around the patch hash/cache folder explanation, not the
surrounding code.

In `@src/jsc/RuntimeTranspilerCache.rs`:
- Around line 619-624: Trim the source-hash comment in RuntimeTranspilerCache to
match the repo’s 3-line comment style by keeping only the cache-key invariant
and the issue reference; update the nearby comment around the hash
computation/`input_hash` logic so it briefly states that the hash must prevent
distinct source files from sharing a cache entry, and retain the
oven-sh/bun#32741 reference without the extended rationale.

In `@src/sql_jsc/mysql/MySQLQuery.rs`:
- Around line 341-345: The new MySQL cache comments in MySQLQuery should be
shortened to comply with the 3-line comment limit while preserving the key
invariant and ownership summary. Update the explanatory blocks around the cache
lookup and prepare path so they are collapsed into brief inline comments,
keeping the important collision/mismatch behavior tied to the query cache logic
in MySQLQuery::prepare and the related lookup code.

In `@src/sql_jsc/postgres/PostgresSQLQuery.rs`:
- Around line 644-650: Shorten the collision comment in PostgresSQLQuery so it
fits the 3-line comment guideline by keeping only the key invariant and the
action taken. Update the explanatory block near the cached prepare logic to
mention that wyhash(signature.name) can collide, that a stored-name mismatch
must be treated as a cache miss, and that execution should fall through to the
uncached prepare path without changing the existing map entry.

In `@test/cli/install/bun-install-patch.test.ts`:
- Around line 1049-1074: The install helper in bun-install-patch.test.ts still
resolves dependencies against the public npm registry because only
BUN_INSTALL_CACHE_DIR is overridden. Update mkProject/install to use the same
dummy local registry fixture as the other install tests so the is-odd@3.0.1
resolution is fully isolated and deterministic, while keeping the sharedCache
for cache isolation.

In `@test/cli/install/wyhash-std-collision.ts`:
- Around line 95-100: The helper comment in wyhash-std-collision should be
shortened to fit the 3-line comment limit while still documenting the layout
invariant. Update the comment near the collision-array builder logic to keep
only the essential explanation of the byte-array form, the collision goal under
std.Wyhash(seed), and the purpose of tailMinLen/freeOffset, using a compact
3-line version.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7ac45246-8d99-46b2-907b-42b7acd0dc28

📥 Commits

Reviewing files that changed from the base of the PR and between 0589548 and 36a6a58.

📒 Files selected for processing (8)

• src/install/patch_install.rs
• src/jsc/RuntimeTranspilerCache.rs
• src/sql_jsc/mysql/MySQLQuery.rs
• src/sql_jsc/postgres/PostgresSQLQuery.rs
• test/cli/install/bun-install-patch.test.ts
• test/cli/install/wyhash-std-collision.ts
• test/cli/run/transpiler-cache.test.ts
• test/js/sql/sql-statement-cache-hash-collision.test.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/jsc/RuntimeTranspilerCache.rs (1)

44-45: 🗄️ Data Integrity & Integration | 🟡 Minor

Align the cache version constants. EXPECTED_VERSION is 23, but RUNTIME_TRANSPILER_CACHE_VERSION is 20, so newly written cache entries will be treated as stale immediately.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/jsc/RuntimeTranspilerCache.rs` around lines 44 - 45, Align the cache
version constants used by RuntimeTranspilerCache so they match. Update the
version value referenced by EXPECTED_VERSION and the
RUNTIME_TRANSPILER_CACHE_VERSION logic together, and make sure the code paths
that read/write the cache use the same version number so entries written by the
transpiler are not immediately invalidated.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/jsc/RuntimeTranspilerCache.rs`:
- Around line 44-45: Align the cache version constants used by
RuntimeTranspilerCache so they match. Update the version value referenced by
EXPECTED_VERSION and the RUNTIME_TRANSPILER_CACHE_VERSION logic together, and
make sure the code paths that read/write the cache use the same version number
so entries written by the transpiler are not immediately invalidated.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1b629f28-2e19-4e22-851b-2c77d1c30bef

📥 Commits

Reviewing files that changed from the base of the PR and between 36a6a58 and e3ecf6c.

📒 Files selected for processing (6)

• src/install/patch_install.rs
• src/jsc/RuntimeTranspilerCache.rs
• src/sql_jsc/mysql/MySQLQuery.rs
• src/sql_jsc/postgres/PostgresSQLQuery.rs
• test/cli/install/wyhash-std-collision.ts
• test/js/sql/sql-statement-cache-hash-collision.test.ts

[robobun]

Re the out-of-diff CodeRabbit note about RUNTIME_TRANSPILER_CACHE_VERSION (20) vs EXPECTED_VERSION (23): addressed in 252ef48, but the stated consequence ("newly written cache entries will be treated as stale immediately") does not hold.

The only code that encodes the on-disk .pile header and the only code that validates cache_version are both the Metadata in src/jsc/RuntimeTranspilerCache.rs, and both use EXPECTED_VERSION, so write and read were always self-consistent. The bundler-side RuntimeTranspilerCacheMetadata is a mirror whose cache_version never reaches the file: its constant has exactly one consumer (its own Default impl), and nothing outside cache.rs references the struct. The drift also predates this PR (20 vs 22 on main) with no effect, which is why the existing transpiler-cache tests that assert a cache hit on a second run pass.

Since the constant's own comment declares it a mirror of EXPECTED_VERSION, leaving it un-mirrored after bumping the real one would be sloppy, so I aligned it to 23 and pointed the comment at the constant the format actually checks.

[claude]

Thanks — the 3a13cc7 fix to the ErrorResponse handler addresses the UAF I flagged and the added regression test covers it. I'm not auto-approving because this is security-hardening that reworks unsafe intrusive ref-counting across the MySQL/Postgres statement caches and changes on-disk cache-key hashing, so it warrants a human sign-off.

Extended reasoning...

Overview

This PR hardens three hash-keyed caches against constructed wyhash collisions (#32741): the MySQL/Postgres per-connection prepared-statement cache (adds a stored-name comparison and an uncached fall-through path, plus a pointer-identity guard in the Postgres ErrorResponse eviction), the patchedDependencies contents hash (Wyhash11 → SHA-1/64), and the runtime transpiler cache input_hash (Wyhash seed-42 → SHA-1/64, with a cache-version bump mirrored in src/bundler/cache.rs). It extends the test-side wyhash collision helper and adds regression tests for all three.

Security risks

The change is itself the security fix; the residual risk is in the new control-flow it introduces. The Postgres/MySQL paths add a state where an uncached ref_count=1 statement coexists with a different cached statement under the same wyhash key — my earlier review found a UAF in the Postgres ErrorResponse handler on exactly that state, which the author confirmed and fixed in 3a13cc7 by comparing the map entry's pointer to the failing statement before evicting/derefing. The SHA-1/64 swaps are straightforward and the cache-version bump invalidates stale entries; I see no new exposure there.

Level of scrutiny

High. This is security-relevant hardening that edits unsafe intrusive ref-counting and raw-pointer eviction logic in two database drivers, changes the on-disk identity of two persistent caches, and already required one correctness fix during review. None of that is mechanical.

Other factors

All prior review feedback (CodeRabbit comment-length nits, the registry-fixture note, my 🟡 named-statement leak note, and my 🔴 UAF) is resolved; the author enumerated the remaining statements map accesses to bound the UAF class and added a Parse-failure regression test that exercises the fixed eviction path under await using teardown. The robobun build-status comment still shows failures on the latest commit, which a human reviewer should check before merging.

[robobun]

One correction to the note above so nobody goes looking for it: there is no robobun build-status comment reporting failures on this PR, and as of now CI on 3a13cc7 has no failing checks (55 passed, 9 still running, 0 failed). The only red CI this PR has had was on the previous commit, where the darwin-aarch64 test lane timed out downloading the build artifact before running any tests; 3a13cc7 superseded that build.

Current state for whoever picks this up: every review finding is addressed and every thread is resolved, including the real use-after-free claude flagged in the Postgres ErrorResponse eviction (fixed in 3a13cc7 with a regression test). Agreed this wants a human sign-off given it touches intrusive ref-counting in the statement caches and changes two on-disk cache keys.

[Jarred-Sumner]

Remove the runtime transpiler cache changes - that's not in scope.

For the other changes, use a trait/HashMap equality to do this instead of one-off implementations in each call site.

[robobun]

Done, both points.

Transpiler cache: reverted entirely in 9e75379 (the hash change, the cache version bump, the test, and the collision-helper additions that only existed for that test). I also corrected my comment on #32741 so it no longer claims that PoC is addressed by this PR, and rewrote the title and description to match the narrower scope.

Prepared statements: 8fef8fb re-keys both caches by the signature name itself, type PreparedStatementsMap = StringArrayHashMap<*mut Statement>, so the byte comparison is the map's own equality (ArrayHashContext<[u8]>) rather than something each call site has to remember. All the one-off name comparisons from the earlier commits are gone, and so is the pointer-identity check I had added to the Postgres ErrorResponse eviction: with the map keyed by name, a remove keyed by the failing statement's name can only match that statement, so the existing SAFETY invariant there holds with no extra code. Net diff for the SQL half is now about one line shorter than main; the existing tests (including the Postgres parse-failure one) pass unchanged on it.

The patchedDependencies contents_hash stays as the SHA-1 swap since there is no map to re-key there: the identity is a cache folder name derived from the patch file bytes on disk, so the hash itself has to be collision resistant. Happy to rework that one too if you had something else in mind.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/sql_jsc/mysql/MySQLConnection.rs`:
- Around line 1728-1730: Remove the issue reference from the runtime invariant
comment in MySQLConnection around the cached statement map. Keep the durable
explanation about keying by signature.name and collision behavior, but delete
the bug-history reference so the comment in this area only describes the
invariant; update the adjacent comment text if needed without changing the
logic.

In `@src/sql_jsc/postgres/PostgresSQLConnection.rs`:
- Around line 56-58: The cache invariant comment in PostgresSQLConnection should
keep the durable explanation but remove the issue reference text, since bug
history should not live in source comments. Update the comment near the
statement cache map to preserve the statement-signature and hash-collision
invariant wording while dropping the “#32741” mention.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a6acd847-c77a-4a63-9cca-9f934e003ddc

📥 Commits

Reviewing files that changed from the base of the PR and between 3a13cc7 and 8fef8fb.

📒 Files selected for processing (5)

• src/sql_jsc/mysql/JSMySQLConnection.rs
• src/sql_jsc/mysql/MySQLConnection.rs
• src/sql_jsc/mysql/MySQLQuery.rs
• src/sql_jsc/postgres/PostgresSQLConnection.rs
• src/sql_jsc/postgres/PostgresSQLQuery.rs

[robobun]

CI status for the final head (eaad76e): build #65112 finished, and none of its failures involve this PR's code or tests. The complete failing set from the build's annotations:

• test/js/node/tls/node-tls-connect.test.ts: asserts cert.infoAccess["OCSP - URI"] is defined for the live bun.sh certificate. The current certificate carries no OCSP URI in its Authority Information Access (Let's Encrypt stopped including one), so this assertion fails on every platform lane and for any branch until that test is updated. This one failure is why all the test-bun lanes report red.
• test/integration/next-pages/test/dev-server-ssr-100.test.ts: error: Fail extracting tarball for "next" during the fixture's bun install (registry fetch).
• test/js/node/test/parallel/test-tls-client-destroy-soon.js: vendored Node TLS test, byte-count mismatch (2097152 vs 2048000).
• test/regression/issue/20965.test.ts: pre-existing streaming-abort test, timed out at 90s on darwin aarch64.
• Flaky-retry warnings for jsonwebtoken/async_sign.test.js and bun-pm-why.test.ts (passed on retry).

The tests added here (test/js/sql/sql-statement-cache-hash-collision.test.ts, the patch case in test/cli/install/bun-install-patch.test.ts) appear in no failure annotation, and the failing areas (live TLS certificate contents, registry tarball extraction, TLS streaming, HTTP response streaming) are outside this diff (SQL prepared-statement cache keying, patchedDependencies content hash). I am not pushing a retrigger: the node-tls-connect assertion fails deterministically against the real certificate, so a re-run cannot come back green regardless of this PR's contents.

[robobun]

Rebased onto main to clear the merge conflicts. Notable: the SQL prepared-statement name-keying fix landed on main independently while this was in review (both MySQL and Postgres now use StringHashMap<*mut Statement> keyed on signature.name), so all of this PR's SQL source changes were superseded and dropped during the rebase. What remains: the patchedDependencies contents_hash SHA-1 swap (main still uses Wyhash11 there), plus the patch-cache and SQL prepared-statement regression tests. The SQL test now guards main's name-keyed implementation and passes against it (mysql + postgres, including the Parse-failure case). Rebuilt and both test files pass; force-pushed. Title and description updated to match the reduced scope.

[claude]

The remaining source change is small and looks correct (Wyhash11 → SHA-1/64 in PatchTask::calc_hash, API verified against bun_sha_hmac::sha::hashers::SHA1), but since it's a cache-poisoning hardening that also changes the on-disk _patch_hash= cache key for every user, I'd like a human to confirm the algorithm choice and the one-time re-patch cost are acceptable.

Extended reasoning...

Overview

After the rebase, this PR now touches only three files: a ~10-line hash-algorithm swap in src/install/patch_install.rs (PatchTask::calc_hash now streams into BoringSSL SHA-1 and truncates the digest to a u64 instead of using Wyhash11(seed=0)), a new regression test in test/cli/install/bun-install-patch.test.ts with a fixed pair of Wyhash11-colliding patch files, and a new test/js/sql/sql-statement-cache-hash-collision.test.ts that guards the SQL prepared-statement invariant whose source fix already landed on main. All of the SQL source changes I previously commented on (PostgresSQLQuery.rs / PostgresSQLConnection.rs / MySQL*) were dropped in the rebase and are no longer in this diff; every prior inline thread is resolved.

Security risks

The change fixes a security issue rather than introducing one: under Wyhash11 an adversarially-constructed patch file could share a _patch_hash= cache folder with a different patch and cause the second project to observe the first project's patched package. Truncated SHA-1 makes that a ~2^64 second-preimage problem. I verified the SHA1 hasher API (init()/update()/r#final(&mut [u8; 20]), DIGEST = 20) matches the call site, and bun_sha_hmac is already a dependency of the install crate. The u64 interface (folder suffix, .bun-patch-hash- tag file, in-memory PatchedDep) is preserved. I don't see a way this weakens anything.

Level of scrutiny

Even though the diff is now mechanically trivial, it is a security hardening in the package manager and it changes the derivation of an on-disk cache identity for every user of patchedDependencies — existing installs will re-patch once because the tag filename differs. That's the kind of user-visible behavior/algorithm choice (SHA-1 truncated to 64 bits vs. e.g. full digest hex) that I think a maintainer should explicitly sign off on rather than have a bot approve. The PR also went through substantial scope surgery (transpiler-cache revert at Jarred's request, SQL re-keying, then SQL source dropped entirely on rebase), so a human confirming the final shape matches intent seems worthwhile.

Other factors

No bugs were found by the bug-hunting system on this revision. All CodeRabbit and prior claude[bot] threads are resolved. The new install test follows the file-local convention (public registry, tempDirWithFiles, early {stdout,stderr,exitCode} guard before reading generated files). The SQL test is regression-only against a fix already on main and gates on the container harness. The bun_wyhash::Wyhash11 import is removed but bun_wyhash remains a live dependency of the install crate (still used elsewhere), so no Cargo.toml change is needed here.

[robobun]

CI status for the rebased head (5b06fe4), build #69386: 250 lanes passed (including the Linux docker lanes that run this PR's bun-install-patch and SQL collision tests); the red/expired lanes are all infra, none involving this diff.

• :darwin: 26 aarch64 - test-bun: Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun' — the lane never downloaded the binary and ran zero tests. This exact timeout has recurred on every build of this PR.
• A batch of expired jobs across darwin/windows/linux lanes (agent never picked them up within the scheduling window).

The 3-file diff (patch contents_hash SHA-1 swap + two regression tests) is green everywhere it actually executed. I'm not pushing a retrigger: the darwin artifact-download timeout is a persistent condition in that agent pool, not a transient flake a re-run would clear.

[Jarred-Sumner]

@robobun rebase + make sure we hash the whole patch file instead of just the first 16 KB. We can read it in chunks of say 64 KB though.

[robobun]

Done, both parts.

Rebased onto current main (clean, single commit).

The hashing was actually worse than only covering the first chunk: File::read_fill_buf preads from offset 0 on every call, so the loop re-read and re-hashed the same leading 16 KiB until its byte counter reached the file size. Any two patches with an identical first chunk hashed equal regardless of what followed. calc_hash now reads with pread_all at an explicit running offset in 64 KiB chunks (a337310). Added a regression test: two patches sharing an 80 KiB identical prefix that differ only in the last line must get distinct cache entries. It fails on the released binary (project B receives project A's patched package) and passes with the fix; the full bun-install-patch.test.ts file is 20/20. PR description updated with a section on this.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/install/patch_install.rs:707 — This PR rewires calc_hash from file.read_fill_buf(...) to file.pread_all(...), which was the sole caller of bun_sys::File::read_fill_buf — repo-wide grep now shows only its definition at src/sys/file.rs:232 and this comment. Per REVIEW.md ("Delete dead code in the same PR that makes it dead … helpers whose last caller you rewired … Public items escape dead-code lints — grep for callers manually"), delete read_fill_buf alongside; it's also the platform-divergent footgun (POSIX pread from offset 0 vs Windows cursor read()) that caused the first-chunk-only bug fixed here.

  Extended reasoning...

  What

  Commit a337310 ("install: hash the whole patch file, not just its first chunk") in this PR rewires PatchTask::calc_hash from file.read_fill_buf(&mut stack[..]) to file.pread_all(&mut chunk[..], offset). That was the only caller of bun_sys::File::read_fill_buf in the repository. After this change, rg read_fill_buf across the whole tree returns exactly two hits:

  • src/sys/file.rs:232 — the pub fn read_fill_buf<'b>(&self, buf: &'b mut [u8]) -> Maybe<&'b mut [u8]> definition
  • src/install/patch_install.rs:701 — the change-narration comment ("read_fill_buf always reads from file offset 0, so looping over it re-hashes the first chunk")

  No other call sites, re-exports, or macro-generated references exist.

  Why it belongs in this PR

  REVIEW.md is explicit that this is required scope, not scope creep:

  Delete dead code in the same PR that makes it dead (required scope — name the deletions in the description): superseded implementations, helpers whose last caller you rewired, fields nothing reads … Public items escape dead-code lints — grep for callers manually.

  read_fill_buf is exactly "a helper whose last caller you rewired", and it is pub fn, so rustc's dead_code lint will not flag it — the manual grep the guidelines call for is what surfaces it. This PR already followed the same rule for the bun_wyhash dependency in src/sql_jsc/Cargo.toml (dropped in eaad76e when its last use was removed); read_fill_buf is the same case one layer deeper.

  Why it's worth deleting rather than leaving in the API

  Beyond hygiene, read_fill_buf is a footgun. Looking at src/sys/file.rs:235-240:

  // POSIX uses pread() from offset 0 so a pre-advanced cursor
  // doesn't truncate; Windows falls back to read().
  #[cfg(unix)]
  let rc = pread(self.handle, &mut buf[read_amount..], read_amount as i64);
  #[cfg(not(unix))]
  let rc = read(self.handle, &mut buf[read_amount..]);

  On POSIX, every call preads starting at absolute file offset 0 (the offset argument is read_amount, which resets to 0 on each call to the function). On Windows, it read()s from the current file cursor. So calling this in a loop — as the pre-PR calc_hash did — has divergent semantics: POSIX re-reads the same first buf.len() bytes on every iteration, while Windows advances through the file. That divergence is precisely what caused the pre-existing "only the first chunk is hashed" bug this PR's HEAD commit fixes (and covers with the new "patches that differ only after the first 64 KiB" test). Leaving the helper callable invites the next caller to hit the same trap.

  Step-by-step proof

  1. Base branch: calc_hash at src/install/patch_install.rs called file.read_fill_buf(&mut stack[..]) in a while (read as u64) < size loop.
  2. PR diff: that call is replaced by file.pread_all(&mut chunk[..], offset) with an explicit offset accumulator.
  3. rg -n read_fill_buf on the PR head → src/sys/file.rs:232 (definition) + src/install/patch_install.rs:701 (comment). No .rs call sites.
  4. read_fill_buf is pub fn → escapes #[warn(dead_code)] → CI will not catch it.
  5. Therefore the helper became dead in this PR and should be deleted here.

  Impact

  None at runtime — this is dead-code hygiene. The function compiles into libbun_rust.a unused (or is stripped by the linker), and no behavior changes either way.

  Fix

  Delete pub fn read_fill_buf (src/sys/file.rs:230-248, including its doc comment). Optionally also drop the change-narration comment at src/install/patch_install.rs:701-702, since once the helper is gone there's nothing to contrast against — the explicit offset accumulator speaks for itself (and comment-cop already flagged that block).

[robobun]

Addressed the three follow-up findings in 33a9059 (plus 4d50023, which had already landed before that review posted):

• read_fill_buf: agreed it became dead here, since calc_hash was its only caller. Deleted from bun_sys::File. Worth removing on its own merits too, given the POSIX/Windows split (offset 0 vs. cursor) is what made looping over it hash only the first chunk on POSIX. Full build links cleanly without it.
• SQL test gating: removed the outer isDockerEnabled() || BUN_TEST_SERVICE_* wrappers and the import; describeWithContainer gates itself in the documented env -> coordinator -> docker order. All three cases still run and pass.
• The two comments in calc_hash were already removed in 4d50023; the review ran against the earlier snapshot.

Net diff is now patch_install.rs (SHA-1, whole-file read), the read_fill_buf removal, and the two test files.

[claude]

I reviewed this PR and didn't find any bugs. Because it swaps the hash function that names the shared _patch_hash= cache folders (a security-relevant collision-resistance choice) and invalidates every existing patched-package cache entry on upgrade, a human look would still be worthwhile.

What was reviewed:

• calc_hash's new pread_all loop — offset advances by the returned byte count, so the whole file feeds SHA-1; n == 0 breaks on short files.
• bun_sha_hmac::sha::hashers::SHA1 API (init/update/r#final/DIGEST) matches the call sites; bun_sha_hmac was already a dependency of bun_install.
• read_fill_buf has no remaining callers under src/ after the deletion.
• The SQL regression test's constructStdCollision import resolves (test/cli/install/wyhash-std-collision.ts exists) and describeWithContainer is called unconditionally per the harness invariant.

Extended reasoning...

Overview

Four files: src/install/patch_install.rs swaps calc_hash from Wyhash11(seed=0) to SHA-1-truncated-to-u64 and rewrites the read loop from read_fill_buf (which restarted at file offset 0 each call on POSIX, so only the first chunk was ever hashed) to pread_all at an explicit running offset; src/sys/file.rs deletes the now-dead read_fill_buf; two test files add regression coverage for the wyhash-collision patch-cache poisoning and (separately) for the SQL prepared-statement cache collision whose source fix already landed on main.

Security risks

The whole point of the change is security: the _patch_hash=<hex> suffix on the shared install-cache folder and the .bun-patch-hash-<hex> tag file were derived from a non-cryptographic hash of attacker-influenceable file bytes, so a constructed collision let two projects share one patched-package folder. Moving to SHA-1/64 raises the second-preimage cost to 2^64. The truncation to u64 keeps the on-disk interface unchanged. I don't see a new risk introduced by the diff itself — the read loop is bounds-correct and the digest buffer is exactly SHA1::DIGEST bytes — but the choice (SHA-1 truncated to 64 bits, rather than e.g. full SHA-256 hex in the folder name) is a design call a maintainer should sign off on.

Level of scrutiny

Higher than a mechanical refactor. This is a hash-function selection for a shared on-disk cache identity (crypto-adjacent per the "do not approve security-sensitive" guideline), it changes behavior for every existing install with a patchedDependencies entry (all cached patched packages re-patch once because the tag filename differs), and a maintainer has already been actively shaping this PR's scope (the transpiler-cache portion was reverted at Jarred's request per the description). Those all point at human review.

Other factors

All of my prior inline comments on this PR were nits (comment length, redundant test gating, dead Cargo dep, helper duplication) and are marked resolved with follow-up commits through 05b6dc3. No human review is on the timeline. The bug-hunting pass on the current head found nothing. The 16 KiB stack buffer became a 64 KiB heap Vec, which is fine for a once-per-patch-file cold path. pread_all's offset: u64 → i64 internal cast is a non-issue at patch-file sizes.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes the hash used for a security-relevant cache identity (patch cache poisoning from #32741) and deletes a public bun_sys::File helper, a human sign-off would still be worthwhile.

What was reviewed:

• calc_hash: SHA-1 API (init/update/r#final/DIGEST=20) matches; digest[0..8] unwrap is infallible; bun_sha_hmac was already an install crate dep.
• pread_all loop: offset advances by n, breaks on n == 0, bounded by size; the 80 KiB-prefix test guards the old first-chunk-only bug.
• read_fill_buf removal: no remaining callers repo-wide; the removed Wyhash11 import was the file's only bun_wyhash reference.
• SQL regression test: constructStdCollision helper exists at the imported path; describeWithContainer self-gates so the tests skip cleanly without docker.

Extended reasoning...

Overview

The PR touches four files: src/install/patch_install.rs (swap Wyhash11 → truncated SHA-1 for the patch contents_hash, and fix the read loop to hash the whole file via pread_all at a running offset instead of read_fill_buf which restarted at offset 0 on POSIX), src/sys/file.rs (delete now-dead read_fill_buf), and two test files (two new patch-hash regression tests in bun-install-patch.test.ts; a new SQL prepared-statement collision regression test whose source fix already landed on main).

Security risks

The change is itself the security fix: it hardens the shared-cache folder / .bun-patch-hash tag identity against constructed second-preimage collisions (Wyhash11 → SHA-1/64). No new attack surface is introduced — the u64 interface (folder suffix, tag filename, lockfile field) is unchanged, and the input is a local patch file the user already controls. The read-loop fix is a strict correctness improvement (previously any two patches sharing an identical first ~16 KiB hashed equal on POSIX). I checked that the digest truncation is well-defined and that bun_sha_hmac was already a dependency of the install crate.

Level of scrutiny

Medium-high. The Rust source change is small (~30 lines) and mechanical, but it selects a hash function for a security-relevant cache identity and deletes a public method from bun_sys::File. I verified read_fill_buf has zero remaining callers repo-wide, that the removed Wyhash11 import was the file's only bun_wyhash reference, and that the SHA1 hasher's init/update/r#final/DIGEST API matches the call sites. The pread_all signature (&mut [u8], u64 → Maybe<usize>) matches the loop's usage.

Other factors

CI is green on the current head (Build #92633). Every prior round of feedback — mine (stale test prose, dead bun_wyhash Cargo dep, redundant isDockerEnabled() wrappers, comment-cop narration comments, duplicated mkProject/install helpers), coderabbitai's, and the comment-cop bot's — is addressed and resolved. No CODEOWNERS entries cover the touched paths. The PR went through significant scope reduction (transpiler-cache reverted at maintainer request; SQL source fix landed independently on main and was dropped in the rebase), so a human confirming the final scope matches maintainer intent is the remaining value-add.