fix: intercept fs.readFileSync/readFile for /$bunfs/root/ paths (2.1.251 -p crash) - #349
Conversation
…x -p crash on 2.1.251 upstream 2.1.251 introduced embedded text assets (plugin-eval docs) read via plain fs.readFileSync/fs.promises.readFile with /$bunfs/root/ virtual paths, bypassing the existing import.meta.require-only patch. Adds installFsBunfsInterception() with syncBuiltinESMExports() (required for ESM named-import bindings), zstd support in the Bun shim (node:zlib), engines/version-gate bump to Node >=23.8.0 (zstd landed at 22.15.0/23.8.0, not 23.5.0), and shared resolveBunfsPath() to dedupe path validation. G1(4x)/G2(2x) Go. Co-Authored-By: Claude Sonnet 5 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f16ab84724
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fsMod.readFile = function (p, ...rest) { | ||
| const real = typeof p === 'string' ? resolveOrRecover(p) : null; | ||
| return Reflect.apply(origReadFile, this, [real !== null ? real : p, ...rest]); |
There was a problem hiding this comment.
Preserve asynchronous readFile error delivery
When a /$bunfs/root/ asset is missing and recovery fails, resolveOrRecover() throws before the original callback-based fs.readFile() is invoked. This changes the API from reporting the error through its callback to throwing synchronously, so callers that rely on the normal fs.readFile(path, callback) contract cannot handle the error and may crash. Catch resolution failures here and invoke the supplied callback asynchronously with the error.
Useful? React with 👍 / 👎.
… exercise installFsBunfsInterception() G3 non-blocker fixes: the sync-necessity test previously hand-rolled a toy simulation instead of calling installFsBunfsInterception(), and the rollback test only wrote a subprocess script without ever executing it (spawnSync was never called). Both are now proper child-process integration tests against the real implementation, and both were verified to fail when the corresponding source behavior is deliberately broken. Co-Authored-By: Claude Sonnet 5 <[email protected]>
upstream 2.1.251 で
-pが100%決定論的にクラッシュする問題の修正。新機能 plugin-eval の埋め込みテキストアセット (zstd圧縮 .md) が素のfs.readFileSync/fs.promises.readFileで/$bunfs/root/仮想パスを直接読もうとし、既存のimport.meta.require専用パッチではカバーされずENOENTでクラッシュしていた。変更
bunfs-esm-loader.mjs:resolveBunfsPath()共有関数 +installFsBunfsInterception()(fs 3関数へのグローバルパッチ、syncBuiltinESMExports()必須、失敗時ロールバック付き)termux-run-claude-native.sh: 両 esmChunkedMain ブロックに Bun shim zstd メソッド追加 +installFsBunfsInterception()配線 (legacyCjsMain は対象外、影響なし)package.json/bin/claude: Node engines を>=23.8.0に (zstd は 22.15.0/23.8.0 で追加、23.5-23.7 は非対応)ゲート
G1 (4回)/G2 (2回) Go。テスト: bunfs-esm-loader.test.js 34/34 pass、termux-run-claude-native.test.js 53 pass/20 fail(既知ベースライン、subprocess timing、無関係)/1 skip。
🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 5 [email protected]