standalone: check every offset in the embedded module graph and reject a repeated module name

[alii]

A bun build --compile executable whose embedded module graph is damaged should report "Corrupted module graph: ..." and exit 1. StandaloneModuleGraph::from_bytes only checked one thing, entry_point_id > record count, and read every other offset in the graph through debug_asserts, so damage panicked (or in release builds read out of bounds) instead:

• an entry point id equal to the record count passed the > check and indexed one past the end in entry_point()
• two records with the same name collapsed into one entry of the name-keyed files map, so an id that was in range for the records indexed past the end the same way
• a name, contents, source map, bytecode, module info, bytecode origin path, argv or module list pointer reaching past the section was dereferenced as is

Now every offset the graph carries is checked against the section length (and NUL-terminated strings against their terminator) before it is read, through checked_range in the three slice helpers, which return Err(Corruption::...) naming the field. A repeated module name is rejected, which is what makes the record count a valid bound for entry_point_id (>=, checked before loading). to_bytes never writes a repeated name; the debug round-trip at the end of it already checks that. entry_point_id is private to the code that checks it.

Only a corrupted binary can reach any of this; it is error-path quality, not something a well-formed build hits.

Tested in test/bundler/bun-build-compile.test.ts by compiling an executable, patching the embedded graph one way per case (entry id == record count, repeated name, name pointer past the section, contents length past the section) and running it, expecting exit 1 with the matching message. Without this change the first case panics with index out of bounds: the len is 1 but the index is 1, the repeated-name case silently runs, and the pointer cases trip the old debug_asserts (a release build would read out of bounds). Runs on Linux and Windows; macOS is skipped because a patched Mach-O would need re-signing, and the parsing code is the same everywhere.

---

no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-compile.test.ts

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 091eca8d-8a70-4fdb-817a-bdaa23da203c

📥 Commits

Reviewing files that changed from the base of the PR and between dc59d3e and 29f98f5.

📒 Files selected for processing (3)

• src/standalone_graph/StandaloneModuleGraph.rs
• src/standalone_graph/error.rs
• test/bundler/bun-build-compile.test.ts

Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review.

---

Walkthrough

Standalone module graph deserialization now validates serialized ranges, pointers, NUL termination, entry-point IDs, and duplicate module names. It returns structured corruption errors. Regression tests cover corrupted standalone binaries.

Changes

Standalone module graph corruption handling

Layer / File(s)	Summary
Define structured corruption errors src/standalone_graph/error.rs	The public Corruption enum defines module graph failure categories. Error maps each category to its message and name.
Validate serialized module graph data src/standalone_graph/StandaloneModuleGraph.rs	Deserialization uses checked slices and structured errors for invalid ranges, pointers, NUL termination, entry-point IDs, and duplicate module names. The entry_point_id field is private.
Test corrupted standalone executables test/bundler/bun-build-compile.test.ts	Regression tests mutate executable metadata and module records. They verify exit code 1, empty stdout, and the expected corruption diagnostic.

Suggested reviewers: robobun, 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 summarizes the main changes: validating embedded module graph offsets and rejecting repeated module names.
Description check	✅ Passed	The description explains the changes and verification, including test coverage and platform limitations, although it omits the template headings.

---

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

[alii]

Added a test (compiled binary validity > corrupted entry_point_id is reported instead of crashing): it compiles a one-module app, sets entry_point_id in the trailer to the module count, and expects exit code 1 with the corruption error on stderr. It runs on Linux and Windows; macOS is skipped because the patched Mach-O would have to be re-signed with Bun's own ad-hoc signer (system codesign rejects the layout), and from_bytes is the same code on every platform.

Checked the scenario by hand on macOS with a debug build of this branch plus codesign: it prints Corrupted module graph: entry point ID is greater than module list count and exits 1. I couldn't run the same patched binary built from main locally (re-signing failed), so the "before" side is from the code: entry_point_id == count passes the old > check and entry_point() indexes files.values_mut()[count].

[coderabbitai]

Actionable comments posted: 1

🤖 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/standalone_graph/StandaloneModuleGraph.rs`:
- Around line 735-736: Update the
CorruptedModuleGraphEntryPointIDIsGreaterThanModuleListCount diagnostic in
src/standalone_graph/error.rs to state that entry-point IDs greater than or
equal to the loaded module count are invalid, and keep StandaloneModuleGraph’s
boundary check mapped to that error. In test/bundler/bun-build-compile.test.ts
lines 181-185, assert the complete corrected corruption diagnostic instead of
only the generic “Corrupted module graph” text.

🪄 Autofix

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: b40e8a2d-b388-4820-986f-f2f47a2bffd0

📥 Commits

Reviewing files that changed from the base of the PR and between 956746d and 3d556fd.

📒 Files selected for processing (2)

• src/standalone_graph/StandaloneModuleGraph.rs
• test/bundler/bun-build-compile.test.ts

[claude]

LGTM — thanks for adding the corrupted-trailer test.

What was reviewed

• The >= modules.count() bound matches how entry_point() indexes files.values_mut(); moving it after the insert loop covers the dedup-by-name case.
• Making entry_point_id private: grepped for external readers — only entry_point() in the same file uses it.
• Test offset math: Offsets is #[repr(C)] with usize + StringPointer{u32,u32} before entry_point_id, so trailer - 16 is correct; the toBe(0) pre-assert guards a mis-located trailer.
• macOS skipIf has a stated reason and from_bytes is platform-shared.

Extended reasoning...

Overview

Two-file change. In src/standalone_graph/StandaloneModuleGraph.rs, the entry_point_id bounds check inside from_bytes moves from before the module-record loop to after the StringArrayHashMap is populated, and tightens from > modules_list_count to >= modules.count(). The struct field goes pub → private. In test/bundler/bun-build-compile.test.ts, a new test compiles a single-module binary, byte-patches the trailer's entry_point_id from 0 to 1, and asserts the child prints the "Corrupted module graph" error and exits 1.

Security risks

None. The path is only reached when a bun build --compile output has been corrupted on disk; the change turns an index panic into a clean error return on that already-broken input. No new parsing, no new trust boundary.

Level of scrutiny

Low. The consumer of this field is a single line, files.values_mut()[entry_point_id] at StandaloneModuleGraph.rs:158, so the correct invariant (< files.count()) is directly readable. I verified the #[repr(C)] layout of Offsets (usize=8, StringPointer{u32,u32}=8, then u32 at +16; struct size 32) against the test's at - 16 patch offset, and confirmed to_bytes appends Offsets immediately before TRAILER (StandaloneModuleGraph.rs:1103-1104) so the backward scan lands on the payload copy. Field-visibility change: grep shows no reads of StandaloneModuleGraph::entry_point_id outside this file; cargo check was run per the description.

Other factors

My prior review asked for a test; the author added a spawned-fixture test that patches the crashing input directly, matching REVIEW.md's "crash fixes need the crashing input as a spawned fixture". The test drains stdout/stderr/exit concurrently, uses tempDir/bunEnv/using, and pre-asserts the un-patched value is 0 so a mis-located trailer would fail loudly rather than silently pass. The macOS skip carries a comment naming the re-signing constraint and notes the guarded code is platform-shared, which satisfies the "every skip needs a comment with the observed failure" rule.

[coderabbitai]

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (2)

src/standalone_graph/StandaloneModuleGraph.rs (2)

732-738: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Restore the early serialized-count check.

The post-insertion check is correct, but it runs after modules.reserve, all read_unaligned calls, all slice_to* calls, and every modules.put. Retain the earlier entry_point_id >= modules_list_count guard before the loop. Keep this modules.count() check as the second guard for records that collapse during insertion.

A malformed binary should not enter the unsafe deserialization path when its entry-point ID is already impossible.

Preserve both validation phases

 let modules_list_base = modules_list_bytes.as_ptr();

+if offsets.entry_point_id as usize >= modules_list_count {
+    return Err(crate::Error::CorruptedModuleGraphEntryPointIDOutOfRange);
+}
+
 let mut modules = StringArrayHashMap::<File>::new();

As per coding guidelines: “Validate untrusted input before processing, allocation, or side effects.”

🤖 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/standalone_graph/StandaloneModuleGraph.rs` around lines 732 - 738,
Restore the early entry-point validation before modules.reserve and the
deserialization/insertion loop, checking entry_point_id against the serialized
modules_list_count and returning CorruptedModuleGraphEntryPointIDOutOfRange when
invalid. Retain the existing post-insertion offsets.entry_point_id versus
modules.count() check to handle records that collapse during insertion.

Source: Coding guidelines

---

732-738: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject duplicate module names during deserialization.

StringArrayHashMap::put replaces duplicate keys without changing their ordinal. entry_point_id then can select the wrong File, or a valid serialized ordinal can fail the modules.count() check.

🤖 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/standalone_graph/StandaloneModuleGraph.rs` around lines 732 - 738, Update
the deserialization logic surrounding the entry-point validation to detect and
reject duplicate module names when inserting into the StringArrayHashMap, rather
than allowing put to replace an existing entry. Return the appropriate
corrupted-module-graph error immediately on a duplicate, while preserving valid
ordinal validation and entry_point() behavior for unique names.

🤖 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/standalone_graph/error.rs`:
- Around line 3-4: Update the corrupted module-graph error handling around
CorruptedModuleGraphEntryPointIDOutOfRange so the user-facing diagnostic
includes offsets.entry_point_id, modules.count(), and a hint to rebuild the
standalone executable. Modify the to_bytes() failure path to format the error
via Display rather than e.name(), while preserving Error::name() and its stable
ErrName value for callers.

---

Outside diff comments:
In `@src/standalone_graph/StandaloneModuleGraph.rs`:
- Around line 732-738: Restore the early entry-point validation before
modules.reserve and the deserialization/insertion loop, checking entry_point_id
against the serialized modules_list_count and returning
CorruptedModuleGraphEntryPointIDOutOfRange when invalid. Retain the existing
post-insertion offsets.entry_point_id versus modules.count() check to handle
records that collapse during insertion.
- Around line 732-738: Update the deserialization logic surrounding the
entry-point validation to detect and reject duplicate module names when
inserting into the StringArrayHashMap, rather than allowing put to replace an
existing entry. Return the appropriate corrupted-module-graph error immediately
on a duplicate, while preserving valid ordinal validation and entry_point()
behavior for unique names.

🪄 Autofix

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: de3c8092-7ffe-4019-9073-8a3397ea68d8

📥 Commits

Reviewing files that changed from the base of the PR and between 3d556fd and 4870846.

📒 Files selected for processing (3)

• src/standalone_graph/StandaloneModuleGraph.rs
• src/standalone_graph/error.rs
• test/bundler/bun-build-compile.test.ts

[claude]

LGTM — the earlier missing-test and error-wording feedback are both addressed; the remaining inline finding is a diagnostic-readability nit and non-blocking.

Extended reasoning...

Overview

This PR tightens the corrupted-trailer guard in StandaloneModuleGraph::from_bytes: the entry_point_id bound moves from > modules_list_count (checked before the map is built) to >= modules.count() (checked after insertion), the field is made module-private, and the error variant is renamed CorruptedModuleGraphEntryPointIDOutOfRange with an accurate message. A new regression test in test/bundler/bun-build-compile.test.ts compiles a one-module app, patches entry_point_id in the on-disk trailer to equal the module count, and asserts the binary exits 1 with the specific corruption message on stderr.

Prior feedback resolved

My earlier review flagged the absence of an automated test — that's now addressed with the spawned-fixture test (the shape I suggested). CodeRabbit flagged the misleading "greater than" wording — addressed by the rename to ...OutOfRange / "out of range for the module list", and the test asserts the full new message. Both threads are marked resolved. I confirmed no stale references to the old variant name remain anywhere in the tree.

Correctness verification

I checked the test's offset arithmetic against the source: Offsets is #[repr(C)] with byte_count: usize (8) + modules_ptr: StringPointer (8) placing entry_point_id at struct offset 16, and to_bytes writes the 32-byte Offsets immediately before TRAILER — so trailer_start - 16 is correct. The test also self-guards with expect(view.getUint32(at - 16, true)).toBe(0) before patching, so a future layout change fails the precondition rather than passing vacuously. The macOS skip is justified in a comment (patched Mach-O needs Bun's ad-hoc re-signer; from_bytes is platform-agnostic). I also re-checked the ruled-out raw_len == 0 early return — it constructs an empty graph with entry_point_id: 0, but that path is only taken when there's no embedded section at all, in which case entry_point() is never called on it; not a regression from this change.

Security risks

None. This path only runs when parsing a self-produced compiled binary's trailer, and the change strictly tightens validation (panic → clean error). Making entry_point_id private removes the only way to set it without going through the validated constructor.

Level of scrutiny & other factors

Low-risk error-path hardening: ~15 net lines, unreachable from any well-formed bun build --compile output. The one inline finding this run — collapsing stderr to a boolean in the combined-object assertion — is a legitimate test-diagnostic nit (the failure diff would show stderr: false instead of the actual output), but the assertion is not vacuous and passes/fails on the right condition. Worth a follow-up tweak to expect.stringContaining(...), not worth holding the merge.

[robobun]

^{Updated 10:58 PM PT - Aug 17th, 2026}

✅ @robobun, your commit 7e1e636d4da78013cf8182bc5144ab1adf6fe4d7 passed in Build #100416! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37639

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

bun-37639 --bun

[claude]

LGTM — thanks for addressing the earlier feedback. All three rounds are covered: the test now exists, the error variant/message name the bound it enforces, and the assertion keeps the raw stderr in the diff.

What was reviewed:

• Verified entry_point_id has no readers outside this module, so making it private is safe; the bundler .entry_point_id() hits are on a different type.
• Checked the #[repr(C)] Offsets layout (usize + {u32,u32} + u32 …) — at - 16 lands on entry_point_id, and the toBe(0) precondition guards the offset arithmetic.
• Confirmed the variant rename is complete (three sites) and no stale references to the old name remain.
• The unresolved CodeRabbit note about embedding the id/count in the diagnostic is a nice-to-have; the enum is Copy unit variants and enriching one is out of scope here.

Extended reasoning...

Overview

This PR tightens a bounds check in StandaloneModuleGraph::from_bytes: the entry_point_id from a compiled binary's trailer is now validated as >= modules.count() after the module map is populated, rather than > modules_list_count before it. This closes an off-by-one (== count passed the old check) and a dedup gap (the map is keyed by name, so it can hold fewer entries than the raw record count). The field is made private since its only consumer is entry_point() in the same file. The error variant is renamed from ...IsGreaterThanModuleListCount to ...OutOfRange with matching Display and name() text. A regression test compiles a one-module binary, patches entry_point_id in the trailer to equal the module count, and asserts exit 1 with the specific corruption message on stderr (skipped on macOS because a patched Mach-O needs re-signing; from_bytes is platform-shared).

Security risks

None. This is defensive validation of a compiled binary's own embedded trailer — reachable only when the executable itself is corrupted. The change makes a panic into a clean error return; it doesn't widen any input surface.

Level of scrutiny

Low-to-moderate. The Rust change is a five-line move-and-tighten of an existing guard on an error path no well-formed binary hits. The test is more involved (hand-computed byte offset into a #[repr(C)] struct across Linux and Windows), but it self-checks the offset via expect(view.getUint32(at - 16, true)).toBe(0) and asserts the exact diagnostic string, so a layout drift or wrong-occurrence match would fail loudly rather than pass vacuously.

Other factors

I reviewed this PR twice previously. Both of my findings (missing test; boolean-collapsed stderr in the assertion) have been addressed in commits 3d556fd and 07641b7. CodeRabbit's wording nit was also addressed in 4870846. The one remaining unresolved CodeRabbit comment suggests embedding the actual id and count values in the diagnostic — a reasonable enhancement, but the Error enum here is a Copy set of unit variants (matching the file's existing pattern for the other corruption cases), and restructuring it is orthogonal to this bounds fix. I grepped for external readers of .entry_point_id on StandaloneModuleGraph (none — the bundler hits are on Chunk::entry_point) and for stale references to the old variant name (none).

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/bundler/bun-build-compile.test.ts`:
- Around line 150-186: Update the corrupted-entry test to build an artifact
containing at least two serialized module records, then patch one record’s name
to duplicate another and set entry_point_id to modules.count() while keeping it
below modules_list_count. Assert the corruption error and nonzero exit, ensuring
the fixture reaches the post-insertion bound guard rather than the existing
pre-insertion check; keep the test anchored to the current Bun.build and
binary-patching flow.

🪄 Autofix

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: 8f6e605d-014f-45e2-8b1c-d7f5a9afbf19

📥 Commits

Reviewing files that changed from the base of the PR and between c4b0d91 and 71fa7fb.

📒 Files selected for processing (3)

• src/standalone_graph/StandaloneModuleGraph.rs
• src/standalone_graph/error.rs
• test/bundler/bun-build-compile.test.ts

[claude]

LGTM — the earlier feedback (add a regression test, keep raw stderr in the assertion, rename the error to reflect the inclusive bound) has all been addressed and the resulting change is small and self-contained.

What was reviewed:

• from_bytes now bounds entry_point_id by modules.count() after insertion with >=, matching the index in entry_point() (files.values_mut()[id]); making the field private is safe — its only reader is that method.
• Verified StringPointer is {u32,u32} and Offsets is 32 bytes, so the test's at - 16 locates entry_point_id; the getUint32(...) === 0 pre-check guards against future layout drift.
• No stale references to the old ...IsGreaterThanModuleListCount variant or message string remain in the tree.

Extended reasoning...

Overview

Three files: src/standalone_graph/StandaloneModuleGraph.rs moves the entry_point_id bounds check from before the module-record loop (> modules_list_count) to after the name-keyed map is built (>= modules.count()) and makes the struct field private; src/standalone_graph/error.rs renames the variant to CorruptedModuleGraphEntryPointIDOutOfRange and updates both the thiserror display string and name(); test/bundler/bun-build-compile.test.ts adds a test that compiles a one-module app, patches the trailer's entry_point_id to equal the module count, and asserts exit code 1 with the corruption message on stderr.

Security risks

None. This is defense-in-depth for a corrupted bun build --compile executable's own trailer — the input is the process's own image, not attacker-controlled network or file data, and the change turns an index-out-of-bounds panic into a typed error return. No auth, crypto, or permission surface is touched.

Level of scrutiny

Low. The code path is only reachable when a compiled standalone binary has been damaged; well-formed outputs never hit the guard. The fix is a two-line comparison change (> → >=, modules_list_count → modules.count()) placed after the map is populated, which is exactly what entry_point()'s files.values_mut()[self.entry_point_id as usize] requires. I confirmed entry_point() is the only reader of the now-private field, and grepped for the old variant name/message to confirm the rename is complete.

Other factors

All four prior review threads (mine and CodeRabbit's) are resolved: the test was added, the assertion now uses expect.stringContaining(...) so failure diffs surface the real stderr, the error wording was corrected for the inclusive bound, and the author's decision to keep the error fieldless (it's a Copy enum feeding ErrName) was accepted by CodeRabbit. The test's byte-offset arithmetic checks out against the #[repr(C)] Offsets layout (usize + {u32,u32} + u32 … = offset 16 in a 32-byte struct), and it self-guards by asserting the pre-patch value is 0 before overwriting. The macOS skip is justified (patched Mach-O needs re-signing) and from_bytes is platform-shared, so Linux/Windows coverage is sufficient.

[alii]

@robobun get this mergeable

[robobun]

Head is 29f98f5: the widened fix from 3a99fb3, plus a merge of main (the binary-size check on 100399 was comparing a branch 187 commits behind main, with per-target deltas from -2 MB to +0.5 MB) and the one review nit on the new source map exit. CI is running on it and no review threads are open.
Once it is green the remaining blocker is an approving review, which the author cannot give on their own PR.

[claude]

LGTM — the earlier feedback (missing test, boolean-collapsed stderr assertion) is addressed and the duplicate-name case is now covered.

What was reviewed:

• >= modules.count() after insertion vs. entry_point()'s index at line 158; entry_point_id made private with no external readers.
• Offsets layout (32 bytes, id at +16), CompiledModuleGraphFile (52 bytes), and TRAILER match the test's byte-patching constants.
• Error variant rename: only two reference sites, both updated.

Extended reasoning...

Overview

Tightens the corrupted-trailer guard in StandaloneModuleGraph::from_bytes: the entry_point_id bound moves from > modules_list_count (checked before the map is built) to >= modules.count() (checked after), closing both the off-by-one at == count and the case where duplicate module names collapse the map below the record count. entry_point_id is made module-private (its only reader is entry_point() at line 158). The error variant is renamed to CorruptedModuleGraphEntryPointIDOutOfRange with matching Display/name() text. A test.each in bun-build-compile.test.ts compiles a binary, byte-patches the trailer for each of the two failure modes, and asserts exit 1 with the corruption diagnostic on stderr.

Security risks

None. This is defensive validation of Bun's own embedded module graph — reachable only via a hand-corrupted bun build --compile output. The change strictly tightens a guard (panic → clean error) and does not touch auth, crypto, network, or user-controlled parsing paths.

Level of scrutiny

Low-to-moderate. The Rust change is a four-line guard relocation plus a visibility tweak and rename in an error path no well-formed binary reaches. I verified the struct layouts the test hard-codes against source (Offsets repr(C): usize + StringPointer + u32 + StringPointer + u32 = 32 bytes, entry_point_id at offset 16; CompiledModuleGraphFile = 6×StringPointer + 4×u8 = 52 bytes; TRAILER = b"\n---- Bun! ----\n"), confirmed the bundler's .entry_point_id() hits are on an unrelated Chunk type so the visibility change is safe, and confirmed the renamed variant has no other references.

Other factors

All four prior review threads are resolved: my request for a test (added), my note on the boolean-collapsed stderr assertion (now expect.stringContaining on the raw string), CodeRabbit's wording nit (message reworded to "out of range"), and CodeRabbit's request to exercise the post-insertion path (second test.each case duplicates a module name so entry_point_id = 1 < modules_list_count but >= modules.count()). robobun reports the prior build passed on all lanes except two cancelled darwin-26 jobs that are main-only. The macOS skip is justified in-comment (Mach-O re-signing) and from_bytes is platform-shared. The 60s per-test timeout is commented with the reason (~1 GB debug+ASAN executable read/rewritten).

[claude]

LGTM — the earlier feedback (add a regression test; keep raw stderr in the combined-object assertion; cover the duplicated-name dedup gap) has all been applied.

Checked the test's byte-offset arithmetic against the #[repr(C)] layouts in StandaloneModuleGraph.rs: Offsets is 32 bytes with entry_point_id at +16, CompiledModuleGraphFile is 52 bytes with name at +0, and the trailer string matches TRAILER. Confirmed entry_point_id's only reader is entry_point() in the same file, so narrowing it to private is safe. The renamed error variant is fully updated in both the #[error(...)] attribute and name().

Extended reasoning...

Overview

Tightens a corruption guard in StandaloneModuleGraph::from_bytes: the entry_point_id bound check moves from > modules_list_count (before the map is built) to >= modules.count() (after), closing an off-by-one and the case where duplicate module names collapse the map below the record count. The error variant is renamed from ...IsGreaterThanModuleListCount to ...OutOfRange with matching message text, and entry_point_id on the graph struct is made module-private since its only reader is entry_point(). A two-case regression test in test/bundler/bun-build-compile.test.ts compiles an executable, patches the trailer, and asserts the corruption error surfaces with exit code 1 instead of an index-out-of-bounds panic.

Security risks

None. The path validates Bun's own --compile output; the change turns a panic into a diagnostic on a corrupted trailer. It does not widen what is accepted, add parsing of external input, or touch auth/crypto/permissions.

Level of scrutiny

Low-to-medium. This is a small error-path hardening reachable only via a damaged compiled executable — well-formed binaries never hit the branch. The logic change is a two-line move-and-tighten; the rest is a rename and visibility narrowing. I verified the test's hard-coded offsets (OFFSETS_SIZE=32, MODULE_RECORD_SIZE=52, entryPointIdAt = offsetsAt+16, name pointer at record +0, trailer literal) against the #[repr(C)] structs and StringPointer {u32, u32} in source, and grepped for external readers of entry_point_id (none outside this file).

Other factors

All prior review threads are resolved: my request for a test and for expect.stringContaining on raw stderr were applied; CodeRabbit's request to also cover the post-insertion dedup gap was addressed with the second test.each case; the author declined to make the error carry the id/count with a stated reason (fieldless Copy enum feeding ErrName, numbers not actionable) and CodeRabbit withdrew. The test asserts its own precondition ({ moduleRecordCount, entryPointId: 0 }) before corrupting, drains all pipes concurrently, uses tempDir/bunEnv/await using, and justifies both the macOS skip and the 60s timeout in comments. robobun reports CI is rerunning after a clean merge of main and that only an approval remains.

[claude]

LGTM — both my earlier points (missing test, boolean-collapsed stderr assertion) are addressed, and CI is green on all lanes.

What was reviewed:

• The bounds check: >= modules.count() after the map is built covers both the off-by-one and the duplicate-name-dedup gap; entry_point() indexes files.values_mut()[id], so this is the right bound.
• Test byte-offset math against the repr(C) Offsets (32 bytes, entry_point_id at +16) and CompiledModuleGraphFile (52 bytes, name at +0) — matches the source structs.
• The renamed error variant has no remaining references to the old name; entry_point_id going private breaks no external readers.

Extended reasoning...

Overview

This PR tightens the corrupted-trailer guard in StandaloneModuleGraph::from_bytes (src/standalone_graph/StandaloneModuleGraph.rs). The old check entry_point_id > modules_list_count ran before the module map was built and had two gaps: it accepted id == count (off-by-one), and it compared against the raw record count rather than the number of files actually inserted into the name-keyed map (which can be smaller if records share a name). Either gap let a corrupted bun build --compile binary reach entry_point() and panic on an out-of-bounds index instead of returning the error the guard exists for. The fix moves the check after the map is built and uses >= modules.count(). The error variant is renamed to CorruptedModuleGraphEntryPointIDOutOfRange with matching message text, and entry_point_id on the graph struct is made module-private since its only reader is entry_point() in the same file. Two regression tests in test/bundler/bun-build-compile.test.ts compile a real executable, patch the trailer to hit each gap, and assert exit 1 with the corruption message on stderr.

Security risks

None introduced. This is defensive hardening on an error path that only fires for a damaged executable — the change strictly narrows what from_bytes accepts. No new parsing of untrusted data, no new allocation before validation, no auth/crypto/permissions surface.

Level of scrutiny

Low-to-medium. The Rust change is a one-condition bounds check moved and tightened, plus a mechanical variant rename; it only affects the error path for corrupted standalone binaries, never well-formed ones. I verified the test's hand-computed byte offsets against the repr(C) layouts in source: Offsets is 32 bytes with byte_count at +0, modules_ptr.{offset,length} at +8/+12, entry_point_id at +16; CompiledModuleGraphFile is 6×StringPointer (48) + 4 repr(u8) enums = 52 with name at +0. The duplicate-name case copies exactly the first record's 8-byte name pointer over the second record's, which is the right corruption to make the map dedup to one entry.

Other factors

Both of my prior review comments are addressed: the missing test was added (and expanded per the later CodeRabbit thread to cover the dedup gap with a second case), and the assertion now keeps raw stderr in the object with expect.stringContaining(...) on the expected side. All inline threads on the PR are resolved. The author declined to add dynamic id/count to the diagnostic with a sound reason (fieldless Copy enum feeding ErrName; the numbers don't help someone holding a damaged binary), and CodeRabbit withdrew that finding. robobun reports build 94851 passed on all 179 jobs. Grep confirms no remaining references to the old variant name and no external readers of the now-private entry_point_id field. The macOS skip is justified in a comment (patched Mach-O needs re-signing; from_bytes is platform-shared) and the 60s timeout is justified (compiles and rewrites a ~1GB debug+ASAN executable).

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[robobun]

#41360 takes the offset checks from this PR and extends them to the records that to_bytes now writes after the module table (#40201, #40407, #40509). This PR predates those records. Sentry BUN-4SPC shows release builds that segfault at the first read in that chain.

#41360 uses the same shape as this PR: checked_range in slice_to, slice_to_mut and slice_to_z, the Corruption enum, and Error::CorruptedModuleGraph(Corruption). Its first commit credits you as a co-author.

It also takes the byte_count check. It checks the entry point ID in another form: it compares the ID with the entries of files, after the loop that builds it. It does not take the repeated-name check. After #41360 lands, a rebase of this PR keeps that check, and its other hunks drop out.