Fix: Bun bytecode extraction — fall back to npm source for patchable JS#655
Open
LeonFedotov wants to merge 4 commits intoPiebald-AI:mainfrom
Open
Fix: Bun bytecode extraction — fall back to npm source for patchable JS#655LeonFedotov wants to merge 4 commits intoPiebald-AI:mainfrom
LeonFedotov wants to merge 4 commits intoPiebald-AI:mainfrom
Conversation
When the native binary contains Bun bytecode (// @Bun @bytecode prefix), function bodies are compiled and can't be regex-patched. This affects all tweakcc patches on recent native installs. Fix: after extracting the claude module, detect the bytecode prefix. If found, fetch the matching version's readable cli.js from the npm registry via `npm pack`. When repacking, clear the bytecode field so Bun uses the patched source JS instead of stale compiled bytecode. This fixes Piebald-AI#628, Piebald-AI#629, Piebald-AI#635, Piebald-AI#645, Piebald-AI#651 — all reporting patch failures on native binary installs. Changes: - nativeInstallation.ts: Add fetchNpmSource(), bytecode detection in extraction, clear bytecode on repack when contents are replaced - nativeInstallationLoader.ts: Pass version parameter through - patches/index.ts: Pass ccInstInfo.version to extraction
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Bun bytecode detection and an npm-pack fallback when native extraction yields Bun bytecode; changes extraction/repack APIs to return/accept a clearBytecode flag and optional version, and threads that flag through repacking and higher-level content I/O paths. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Tool
participant NativeBin as Native Binary
participant NPM as npm (shell)
participant FS as Filesystem
participant Repacker as Repack Logic
CLI->>NativeBin: extractClaudeJsFromNativeInstallation(path, version?)
NativeBin-->>CLI: { data: Buffer|null, clearBytecode: boolean }
alt data is Bun bytecode and version provided
CLI->>NPM: npm pack `@anthropic-ai/claude-code`@version
NPM-->>FS: write tarball
FS-->>NPM: extract package/cli.js -> Buffer
NPM-->>CLI: cli.js Buffer
CLI->>Repacker: repackNativeInstallation(binPath, modifiedJs, outPath, clearBytecode=true)
else data is readable JS or no bytecode clear needed
CLI->>Repacker: repackNativeInstallation(binPath, modifiedJs, outPath, clearBytecode=false)
end
Repacker->>FS: write repacked binary
FS-->>CLI: repack completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🧹 Nitpick comments (1)
src/nativeInstallation.ts (1)
707-714: Please drop the new explanatory comments in this file.These additions mostly restate the control flow and go against the repo's comment-light rule.
As per coding guidelines, "Do not add comments unless explicitly requested".
Also applies to: 724-724, 732-732, 752-756, 802-803, 878-879
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nativeInstallation.ts` around lines 707 - 714, Remove the new explanatory comments that were added to nativeInstallation.ts (specifically the block comment starting "/** Fetches the readable cli.js source from the npm package..." and the subsequent short explanatory lines added at the other listed locations). Delete those comment blocks/lines so the code follows the repo's comment-light rule; do not change the implementation or behavior of the function(s) (leave the function body and any identifiers intact).
🤖 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 761-764: The function extractClaudeJsFromNativeInstallation
currently returns a Buffer|null but loses the information whether we purposely
cleared bytecode (npm fallback) or are returning the original bytecode; thread a
clearBytecode flag through the API so callers can distinguish fallback vs
original. Specifically, change extractClaudeJsFromNativeInstallation (and the
related code around the other occurrence at 812-827) to propagate a
boolean/metadata (e.g., {data: Buffer|null, clearBytecode: boolean}) or
accept/return a clearBytecode parameter; update nativeInstallationLoader.ts,
patches/index.ts, and rebuildBunData() to accept and forward that flag, and when
fetchNpmSource() returns null or version is missing ensure clearBytecode is
false (preserve original module.bytecode), while when npm fallback is used set
clearBytecode true so callers know to strip module.bytecode.
- Around line 719-737: Replace the two execSync calls with execFileSync using
argument arrays to avoid shell interpolation: call execFileSync('npm', ['pack',
`@anthropic-ai/claude-code@${version}`, '--pack-destination', tmpDir], {
stdio:'pipe', timeout:30000, cwd: tmpDir }) and call execFileSync('tar', ['xzf',
path.join(tmpDir, tgz), 'package/cli.js'], { stdio:'pipe', timeout:30000, cwd:
tmpDir }); keep the same options (stdio, timeout, cwd), import/ensure path is
available for path.join, and reference the tmpDir, version and tgz variables
used in the original block so no shell interpolation is performed.
---
Nitpick comments:
In `@src/nativeInstallation.ts`:
- Around line 707-714: Remove the new explanatory comments that were added to
nativeInstallation.ts (specifically the block comment starting "/** Fetches the
readable cli.js source from the npm package..." and the subsequent short
explanatory lines added at the other listed locations). Delete those comment
blocks/lines so the code follows the repo's comment-light rule; do not change
the implementation or behavior of the function(s) (leave the function body and
any identifiers intact).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 396b1fd5-d0bc-4202-ac5b-0d3462e38f75
📒 Files selected for processing (3)
src/nativeInstallation.tssrc/nativeInstallationLoader.tssrc/patches/index.ts
- Use execFileSync with arg arrays instead of execSync (no shell) - Thread clearBytecode flag so bytecode is only cleared on npm fallback - Remove explanatory comments (repo style) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/content.ts (1)
30-44:⚠️ Potential issue | 🟠 MajorThread the
clearBytecodeflag fromreadContenttowriteContentto prevent stale bytecode from persisting.The
readContentfunction receivesclearBytecodefromextractClaudeJsFromNativeInstallationbut only destructures thedatafield, discarding the flag. Consequently,writeContentalways passesfalsetorepackNativeInstallation, which prevents bytecode clearing even when needed (when npm fallback is used).Since these functions are always paired in the read-modify-write flow (e.g.,
src/commands.tslines 403–451, 474–534, 600–674, 692–745), the flag must be threaded through. Otherwise, a native binary with Bun bytecode will retain stale compiled code after patching.Solution: Return
clearBytecodefromreadContentand pass it towriteContent:
- Modify
readContentto return both content and flag- Accept
clearBytecodeparameter inwriteContentand pass it torepackNativeInstallationCompare with
src/nativeInstallation.tswhich correctly handles this flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/content.ts` around lines 30 - 44, readContent currently discards the clearBytecode flag returned by extractClaudeJsFromNativeInstallation; change readContent to return both the content string and the clearBytecode boolean (e.g., { content, clearBytecode }) when installation.kind === 'native', and update callers accordingly. Update writeContent to accept a clearBytecode parameter and forward that boolean to repackNativeInstallation instead of always passing false. Use the symbols readContent, writeContent, extractClaudeJsFromNativeInstallation, and repackNativeInstallation to locate the changes and mirror the handling in nativeInstallation.ts so the read-modify-write flow preserves the clearBytecode flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/content.ts`:
- Around line 30-44: readContent currently discards the clearBytecode flag
returned by extractClaudeJsFromNativeInstallation; change readContent to return
both the content string and the clearBytecode boolean (e.g., { content,
clearBytecode }) when installation.kind === 'native', and update callers
accordingly. Update writeContent to accept a clearBytecode parameter and forward
that boolean to repackNativeInstallation instead of always passing false. Use
the symbols readContent, writeContent, extractClaudeJsFromNativeInstallation,
and repackNativeInstallation to locate the changes and mirror the handling in
nativeInstallation.ts so the read-modify-write flow preserves the clearBytecode
flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0d2564c-d041-45a9-9868-7ae3b4ab068f
📒 Files selected for processing (6)
src/installationDetection.tssrc/lib/content.tssrc/nativeInstallation.tssrc/nativeInstallationLoader.tssrc/patches/index.tssrc/tests/config.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nativeInstallationLoader.ts
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tests/content.test.ts (1)
25-42: Add an assertion thatinstallation.versionis passed to extraction.These tests validate return shape well, but they don’t verify the call contract that drives npm-version fallback selection.
✅ Suggested assertion
const result = await readContent(installation); expect(result).toEqual({ content: jsContent, clearBytecode: true }); + expect( + nativeInstallation.extractClaudeJsFromNativeInstallation + ).toHaveBeenCalledWith('/usr/bin/claude', '1.0.0');As per coding guidelines, "Test edge cases and error conditions in test files".
Also applies to: 44-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/content.test.ts` around lines 25 - 42, The tests for readContent need to assert that installation.version is passed through to the native extraction call: update the two native-installation tests (the one using nativeInstallation.extractClaudeJsFromNativeInstallation at lines shown) to include an expectation like vi.mocked(nativeInstallation.extractClaudeJsFromNativeInstallation).toHaveBeenCalledWith(expect.objectContaining({ version: installation.version })) or an equivalent call-arg assertion after invoking readContent; ensure you reference the same installation object used in the test and assert the mock was called with that version so the npm-version fallback contract is verified.
🤖 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/commands.ts`:
- Line 403: The unpack→repack flow drops the clearBytecode signal (readContent
currently assigned at const { content } = await readContent(installation)) and
later hardcodes false, letting bytecode-native installs keep executing embedded
bytecode; modify readContent usage to destructure and preserve the clearBytecode
flag (e.g., const { content, clearBytecode } = await readContent(installation))
and pass that flag through to the repack/write step instead of hardcoding false
so the repack operation removes embedded bytecode when clearBytecode is true
(ensure the repack/pack/write function that currently receives false uses the
propagated clearBytecode value).
---
Nitpick comments:
In `@src/tests/content.test.ts`:
- Around line 25-42: The tests for readContent need to assert that
installation.version is passed through to the native extraction call: update the
two native-installation tests (the one using
nativeInstallation.extractClaudeJsFromNativeInstallation at lines shown) to
include an expectation like
vi.mocked(nativeInstallation.extractClaudeJsFromNativeInstallation).toHaveBeenCalledWith(expect.objectContaining({
version: installation.version })) or an equivalent call-arg assertion after
invoking readContent; ensure you reference the same installation object used in
the test and assert the mock was called with that version so the npm-version
fallback contract is verified.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5952118-9163-40d2-ba02-03be08304607
📒 Files selected for processing (4)
src/commands.tssrc/lib/content.tssrc/lib/index.tssrc/tests/content.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/content.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Fixes patch failures on native binary installs where the extracted JS contains Bun bytecode (
// @bun @bytecodeprefix). Function bodies in bytecode are compiled and can't be regex-patched, causing most tweakcc patches to silently fail.Fix: When bytecode is detected after extraction, fetch the matching version's readable
cli.jsfrom the npm registry. When repacking, clear the bytecode field so Bun uses the patched source JS.Fixes #628, #629, #635, #645, #651.
How it works
Why bytecode appears
Recent Claude Code native binaries (2.1.81+) are compiled with Bun's bytecode format. The module graph stores:
contents: source wrapped with// @bun @bytecodeprefix — function bodies are compiled bytecode, not readable JSbytecode: the separate compiled bytecode blobWhen repacking with patched source, the stale
bytecodefield must be cleared (zeroed) so Bun falls back to interpreting the source JS instead of executing the outdated bytecode.Changes
src/nativeInstallation.tsfetchNpmSource()— downloads cli.js from npm vianpm pack; bytecode detection inextractClaudeJsFromNativeInstallation; clear bytecode on repacksrc/nativeInstallationLoader.tsversionparameter through to extractionsrc/patches/index.tsccInstInfo.versionto extraction callEdge cases
cli.jsinstallationDetection.ts,lib/content.ts): Don't pass version, so no npm fetch is triggered — they only need the version string from comments, which is readable even in bytecodeTesting
pnpm lintpasses (typecheck + ESLint)pnpm run testpasses (221 total, 0 failures)pnpm prettier --check srcpassesSummary by CodeRabbit
Bug Fixes
New Features
Tests