fix: add ELF-native extraction/repack and improve error reporting for NixOS/Bun#589
Closed
y0usaf wants to merge 3 commits intoPiebald-AI:mainfrom
Closed
fix: add ELF-native extraction/repack and improve error reporting for NixOS/Bun#589y0usaf wants to merge 3 commits intoPiebald-AI:mainfrom
y0usaf wants to merge 3 commits intoPiebald-AI:mainfrom
Conversation
On NixOS with Bun, node-lief fails to load because libstdc++.so.6 is not on the system library path. Previously this produced a generic "Could not extract JS from native binary" error with no hint about the cause or how to fix it. Now nativeInstallationLoader.ts tracks the load error, and installationDetection.ts surfaces it with a NixOS-specific suggestion to use Node.js instead (nix shell nixpkgs#nodejs -c npx tweakcc).
Contributor
📝 WalkthroughWalkthroughAdds ELF-aware extraction and repacking for Bun-based native installations, exposes native module load failure details via a new getter, enriches error messages with platform-specific hints and load-error text, and updates tests to mock and assert the new error-path behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Installer
participant Detector as installationDetection
participant Loader as nativeInstallationLoader
participant NativeMod as nativeInstallation
participant FS as FileSystem
participant NativeBin as Native Binary
CLI->>Detector: request extract/repack
Detector->>Loader: invoke native extraction
Loader->>FS: read native binary bytes
FS-->>Loader: binary bytes
alt ELF binary
Loader->>NativeMod: extractBunDataFromELFRaw (parse ELF overlay)
NativeMod-->>Loader: bun data + offsets
else non-ELF binary
Loader->>NativeMod: use LIEF-based extraction
NativeMod-->>Loader: bun data
end
Loader->>Loader: set/clear loadError on dynamic load attempt
Loader-->>Detector: result or throw (includes getNativeModuleLoadError())
Detector->>CLI: return result or enriched error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/installationDetection.ts`:
- Around line 388-399: The heuristic that sets nixNote is too broad because
loadErr?.includes('No such file') can match generic ENOENT errors; tighten it by
detecting Linux shared-library loader messages instead (e.g., look for 'cannot
open shared object file' or 'undefined symbol' combined with 'ld.so'/'libc' or
other ELF/shared-object indicators) when computing nixNote in the block that
builds loadErr/nixNote; update the logic around getNativeModuleLoadError,
nixNote, and the thrown Error that references binaryPath so the Bun/NixOS hint
is only appended for true shared-library loading failures rather than any "No
such file" ENOENT.
In `@src/nativeInstallationLoader.ts`:
- Line 24: The stored loadError string (loadError) is not cleared when a later
retry successfully imports the native module, so getNativeModuleLoadError() can
return a stale error; update the retry/success paths (where the native module is
assigned—e.g., in the function that attempts the import such as
loadNativeInstallation()/tryLoadNativeInstallation() or wherever nativeModule is
set) to set loadError = null on successful load, and ensure any other
success-return paths also clear loadError so getNativeModuleLoadError() reliably
returns null after a successful import.
In `@src/tests/config.test.ts`:
- Around line 32-36: The test mock for getNativeModuleLoadError in
src/tests/config.test.ts always returns null, so the new enriched error branches
(the "Reason: node-lief failed to load..." suffix and the NixOS/Bun workaround
text) are never exercised; update the mock to return representative non-null
error strings in targeted tests (e.g., use vi.fn().mockReturnValueOnce(...) or
override the mock in specific test cases) so you add two assertions: one where
getNativeModuleLoadError returns a message containing "node-lief failed to load"
and one returning the NixOS/Bun workaround text, ensuring code paths that build
the enriched error message run (reference the mocked getNativeModuleLoadError in
../nativeInstallationLoader and the tests in src/tests/config.test.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d72b39a5-601c-482a-85a2-db86870ece43
📒 Files selected for processing (3)
src/installationDetection.tssrc/nativeInstallationLoader.tssrc/tests/config.test.ts
LIEF crashes (SIGSEGV) when parsing large ELF Bun binaries (~237MB). On Linux/NixOS, Bun stores its data as a plain file overlay — no ELF section parsing needed. This adds raw file-based extraction and repacking for ELF, skipping LIEF entirely for that format. LIEF is still used for Mach-O and PE where it reads actual binary sections that can't be found by tail-reading alone.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/nativeInstallation.ts (1)
507-514: Drop or rewrite this new ELF docblock.It says the trailing
u64 totalByteCountis the source of truth, butextractBunDataFromELFRaw()never reads that footer and instead usesBunOffsets.byteCount. Leaving the comment as-is makes this path easy to cargo-cult incorrectly.As per coding guidelines, "Do not add comments unless explicitly requested".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nativeInstallation.ts` around lines 507 - 514, The docblock describing ELF layout is incorrect: it claims the trailing u64 totalByteCount is the source of truth but extractBunDataFromELFRaw() does not read that footer and relies on BunOffsets.byteCount; update or remove the docblock so it matches the implementation. Either (A) delete the block entirely, or (B) rewrite it to accurately state that extractBunDataFromELFRaw() uses BunOffsets.byteCount (from the Bun offsets structure) as the overlay size and does not read a trailing u64 footer, and clarify where the offsets are read and why the footer is not used; ensure any remaining comment cannot be misinterpreted to suggest the code reads the u64 footer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nativeInstallation.ts`:
- Around line 495-500: isELFFile currently opens a file descriptor and reads 4
bytes but can leak the fd on read errors and misclassify very short files due to
using Buffer.allocUnsafe without checking bytes read; modify isELFFile to ensure
the fd is always closed (use try/finally around fs.openSync/readSync/closeSync
or use fs.readFileSync) and validate the number of bytes returned from readSync
before inspecting the magic bytes (return false for reads < 4), preserving the
existing behavior and signature.
- Around line 1267-1274: repackELFRaw currently computes the ELF prefix using
the trailing footer (readBigUInt64LE) which can be unreliable; change it to use
the parsed offsets block (BunOffsets.byteCount or the existing offsets object
used elsewhere) to determine the overlay start, i.e. compute elfSize from
fileSize minus offsets.byteCount (not the footer), preserve the same bounds
checks and error message, and ensure this aligns with the early-return behavior
in repackNativeInstallation so we don't rewrite from the wrong offset.
---
Nitpick comments:
In `@src/nativeInstallation.ts`:
- Around line 507-514: The docblock describing ELF layout is incorrect: it
claims the trailing u64 totalByteCount is the source of truth but
extractBunDataFromELFRaw() does not read that footer and relies on
BunOffsets.byteCount; update or remove the docblock so it matches the
implementation. Either (A) delete the block entirely, or (B) rewrite it to
accurately state that extractBunDataFromELFRaw() uses BunOffsets.byteCount (from
the Bun offsets structure) as the overlay size and does not read a trailing u64
footer, and clarify where the offsets are read and why the footer is not used;
ensure any remaining comment cannot be misinterpreted to suggest the code reads
the u64 footer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c605b479-7375-4224-a9b4-76bb3d12d56e
📒 Files selected for processing (1)
src/nativeInstallation.ts
- isELFFile: use try/finally to prevent fd leak; validate bytesRead - extractBunDataFromELFRaw: correct docblock (uses BunOffsets.byteCount, not footer) - repackELFRaw: derive elfSize from BunOffsets.byteCount instead of the unreliable trailing u64 footer - nativeInstallationLoader: clear loadError on successful module load - installationDetection: tighten NixOS hint to only trigger on shared-object errors (drop the over-broad 'No such file' match) - config.test: add two tests covering enriched error message paths
y0usaf
changed the title
y0usaf
added a commit
to y0usaf/tweakcc
that referenced
this pull request
Mar 6, 2026
- Tighten NixOS/Bun heuristic: use regex + platform check instead of broad 'No such file' substring match - Add test coverage for enriched error branches (load reason suffix and NixOS/Bun hint text)
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Superseded by #633