node/types: replace the StringOrBuffer parser bool flags with enums - #39164
Conversation
StringOrBuffer::from_js_maybe_async, from_js_with_encoding_maybe_async,
from_js_with_encoding_maybe_async_into and
BlobOrStringOrBuffer::from_js_maybe_file_maybe_async took two bools each,
and several callers passed bare literals (`.., true, true`).
is_async is now node_fs's Flavor (moved into types.rs, node_fs re-exports
it), allow_string_object is StringObjects::{Allow, Reject}, and allow_file
is FileBlobs::{Allow, Reject}, named for what it does: the old `true` was
the value that rejects file-backed blobs. Every call passes the variant
equivalent to the literal it passed before; no behavior change.
Clears the bare_bool_args entry for this file in mordant-baseline.toml.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
WalkthroughThe PR replaces boolean argument-parsing controls with ChangesArgument parsing policy migration
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This refactor replaces ambiguous parser booleans with descriptive enums while preserving behavior, supported by targeted tests and lint validation; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Related rather than a duplicate: #37180 is a workspace-wide pass (539 files, currently conflicting) that retypes these same four functions with |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with every literal mapped to its equivalent variant.
What was reviewed:
- Traced each bool→enum mapping at every call site (
is_async↔Flavor,allow_string_object↔StringObjects,allow_file↔FileBlobswith the polarity fix) — all preserve behavior. - Confirmed the
Flavormove fromnode_fs.rstotypes.rskeepsConstParamTyand thepub usere-export, so externalnode::fs::Flavorcallers (Blob/fetch/SSLConfig/copy_file) are unaffected. - Grepped for all callers of the re-signatured functions and
PBKDF2::from_js— every one is in the diff; the Rust type system enforces no site was missed.
Extended reasoning...
Overview
This PR replaces three bare bool parameters on the StringOrBuffer/BlobOrStringOrBuffer argument parsers in src/runtime/node/types.rs with two-variant enums (Flavor::{Sync,Async}, StringObjects::{Allow,Reject}, FileBlobs::{Allow,Reject}), updates the ~9 files that call them, moves the pre-existing Flavor enum from node_fs.rs to types.rs (re-exported so external callers are untouched), and removes the corresponding bare_bool_args mordant baseline entry. Comments referencing the old parameter names are updated in place.
Security risks
None. This is a type-level refactor of internal function signatures with no change to validation, allocation, protection/unprotection, or thread-safety semantics. Every conditional in the function bodies maps one-to-one from the old bool test to the equivalent enum comparison.
Level of scrutiny
Low-to-medium. The change touches memory-safety-adjacent code (the Flavor::Async branches take protect() and pin buffers), but the refactor is purely mechanical and compiler-enforced: Rust's type system guarantees every call site was updated, and I verified each old-literal→new-variant mapping individually. The one non-obvious mapping is FileBlobs, where the old allow_file was named backwards (true meant reject); the diff maps true→Reject and false→Allow at every site (from_js/from_js_async and both valkey call sites), and the body test if allow_file && ... becomes if file_blobs == FileBlobs::Reject && ..., preserving behavior.
Other factors
The PR description documents extensive verification against existing tests (fs, pbkdf2/scrypt sync+async, zstd, transpiler, CryptoHasher) plus a manual A/B script against the released binary. No new test is warranted since the change is a compiler-verified renaming with no behavioral delta. The Flavor enum retains its ConstParamTy derive so const-generic uses in node_fs.rs continue to compile, and the pub use super::types::Flavor re-export keeps the four external node::fs::Flavor references resolving.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Bun.build's files option rejecting a Bun.file() blob (the async parser's file-blob setting) and writeFile/appendFile rejecting a String wrapper on both the sync and callback paths (the one site that turns String objects away) had no coverage; the other flag values already have tests.
|
Status: reproduced the four CI: the red lanes are main issues, not this diff, and each has been reported for main-break triage.
|
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with every call site mapped 1:1.
What was reviewed:
- Traced every
is_async/allow_string_object/allow_fileliteral at all 9 call-site files to its enum variant; each maps correctly, including theFileBlobspolarity fix (oldallow_file=true⇒ throw ⇒FileBlobs::Reject). Flavormoved fromnode_fs.rstotypes.rswith identical derives (ConstParamTypreserved for const-generic use) and re-exported, so the 4 externalfs::Flavorcallers are untouched.- The two new tests pin existing behavior (
StringObjects::RejectforwriteFile'snew String()rejection;FileBlobs::RejectforBun.buildfiles) rather than testing anything new.
Extended reasoning...
Overview
Replaces three bare bool parameters on the StringOrBuffer / BlobOrStringOrBuffer argument parsers in src/runtime/node/types.rs with two-variant enums (Flavor::{Sync,Async}, StringObjects::{Allow,Reject}, FileBlobs::{Allow,Reject}), updates all call sites across 9 source files, moves the pre-existing Flavor enum from node_fs.rs into types.rs (re-exported for compatibility), removes the corresponding bare_bool_args mordant baseline entry, and adds two tests that pin the flag values.
Security risks
None. This is a type-level refactor of internal function signatures with no change to validation, parsing, or thread-safety semantics. The Flavor::Async path still protect()s buffers exactly where is_async == true did.
Level of scrutiny
Medium — the parsers are on the hot path for node:fs/node:crypto/zstd/sockets/Redis, and a swapped variant would silently change protect/unprotect pairing or String-object acceptance. I traced each of the ~20 call-site literals individually against the old bool value: true/is_async→Flavor::Async, false→Flavor::Sync, allow_string_object=true→StringObjects::Allow, false→Reject, and the misleadingly-named allow_file=true (which threw on file blobs) →FileBlobs::Reject at from_js/from_js_async/valkey number branch, false→Allow at the valkey general branch. All match.
Other factors
Flavor's derive set (includingcore::marker::ConstParamTy, required by const-generic dispatch innode_fs.rs) is preserved verbatim in the move; thepub use super::types::Flavorre-export keeps the 4 externalfs::Flavorcallers (fetch.rs,Blob.rs,copy_file.rs,SSLConfig.rs) compiling unchanged.- Grep confirms no callers of the retyped functions outside the PR's changed files.
- The PR description documents extensive test verification across fs/crypto/zstd/transpiler/redis, plus a differential run against the released binary.
- The two added tests are well-formed (synchronous throws,
toThrowWithCodewith specific error class/code, hermetic tempdir) and pin behavior that would break if a future edit swapped a variant. - The overlap with #37180 is a coordination note, not a correctness concern for this change.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
On the comment-cop threads: the groups it flagged in |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — every bool→enum mapping at each call site checks out (is_async↔Flavor, allow_string_object↔StringObjects, and the polarity-corrected allow_file↔FileBlobs). A human look is still worthwhile: comment-cop left eight unaddressed inline flags on the new enum doc comments and on pre-existing comments this diff only reworded, and there's the overlap with #37180 to coordinate.
What was reviewed
- Traced every changed call site (types.rs internal forwarders, BunObject/JSTranspiler/PBKDF2/Scrypt/WriteFile/socket/valkey) — each old literal maps to the semantically identical variant.
- Confirmed
FlavorkeepsConstParamTyand thenode_fs/nodere-exports, so existingfs::Flavorconst-generic callers are unchanged; grepped for other callers of the retyped functions — all are in the diff. - Checked the two new tests use
tempDir, assert the negative contract (nothing created), and pin behavior this refactor preserves.
Extended reasoning...
Overview
This PR replaces three bare bool parameters on the StringOrBuffer/BlobOrStringOrBuffer argument parsers in src/runtime/node/types.rs with two-variant enums: is_async → Flavor::{Sync,Async} (moved from node_fs.rs and re-exported there), allow_string_object → StringObjects::{Allow,Reject}, and allow_file → FileBlobs::{Allow,Reject} (renaming to match what the flag actually does — the old allow_file=true was the value that rejected file blobs). It updates all callers in BunObject.rs, JSTranspiler.rs, PBKDF2.rs, node_crypto_binding.rs, node_fs.rs, socket_body.rs, and js_valkey_functions.rs; propagates Flavor through PBKDF2::from_js; updates doc-comment references in node_path.rs; removes the bare_bool_args:src/runtime/node/types.rs mordant baseline entry; and adds two behavior-pinning tests.
Security risks
None. This is a type-level refactor of internal function signatures; no user-facing surface, parsing logic, or trust boundary changes. The Flavor::Async branch that protect()s JS buffers for cross-thread use is reached under exactly the same conditions as before.
Level of scrutiny
Moderate. The parsers sit on the argument-handling path for node:fs, node:crypto, zstd, sockets, and the Redis client, and Flavor::Async gates whether JS-backed buffers are pinned/protected for the thread pool — a wrong mapping could cause a UAF or a leaked protect. I therefore traced every call site individually: each true/false literal maps to the variant that reproduces the old branch (if is_async → if flavor == Flavor::Async; if allow_file && .. → if file_blobs == FileBlobs::Reject && ..; if !allow_string_object && .. → if string_objects == StringObjects::Reject && ..). The valkey number path (FileBlobs::Reject, was true) and general path (FileBlobs::Allow, was false) preserve the one caller that permitted file blobs. Flavor retains its ConstParamTy derive so const-generic read_file<{Flavor}> callers still compile, and node_fs.rs re-exports it so node::fs::Flavor paths (used in Blob.rs, fetch.rs, SSLConfig.rs, etc.) resolve to the same type.
Other factors
Two things keep me from approving outright. First, the comment-cop bot left eight unaddressed inline comments — several are clearly noise (they flag pre-existing comments where this PR only swapped is_async=true for Flavor::Async), but the ones on the new enum doc comments are a style call the author or a maintainer should make. Second, #37180 covers the same functions repo-wide with different enum names (IsAsync/AllowFile via bool_enum!); a maintainer should decide which naming lands. The refactor itself is correct and well-tested (the description lists the full test matrix run, and the two new tests pin the previously-uncovered FileBlobs::Reject on from_js_async and StringObjects::Reject on both flavors).
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with every call site mapping 1:1 to its old literal.
What was reviewed:
- Traced every changed call site:
is_async→Flavor,allow_string_object→StringObjects,allow_file→FileBlobsall preserve semantics; theallow_filepolarity flip (true→Reject,false→Allow) matches the oldif allow_file && needs_to_read_file()check. Flavormove keepsConstParamTyand is re-exported fromnode_fs, so const-generic uses (read_file::<{Flavor::Sync}>) andnode::fs::Flavorpaths keep working.- comment-cop flags are false positives (enum docs and pre-existing comments with a token rename), already addressed in the thread.
Extended reasoning...
Overview
Replaces four bool parameters on StringOrBuffer/BlobOrStringOrBuffer parsers in src/runtime/node/types.rs with two-variant enums (Flavor::{Sync,Async}, StringObjects::{Allow,Reject}, FileBlobs::{Allow,Reject}), updates the ~15 call sites across BunObject.rs, JSTranspiler.rs, PBKDF2.rs, node_crypto_binding.rs, node_fs.rs, socket_body.rs, and js_valkey_functions.rs, moves Flavor from node_fs.rs to types.rs (re-exported), removes the mordant baseline entry, and adds two behavior-pinning tests.
Security risks
None. This is a type-level refactor of internal argument-parsing helpers; no new inputs are accepted, no validation is loosened, and no crypto/auth/permission logic changes.
Level of scrutiny
Medium — the change touches shared argument parsers used by fs/crypto/zstd/transpiler/socket/redis, so a mismapped literal would flip async-vs-sync buffer protection or file-blob acceptance. I verified each site individually: every true/false maps to the variant whose enum comparison reproduces the old branch. The one non-trivial mapping is allow_file (whose old name was inverted); from_js/from_js_async (old true) → FileBlobs::Reject, valkey's false → FileBlobs::Allow, and the guard if allow_file && ... → if file_blobs == FileBlobs::Reject && ... — behavior-identical.
Other factors
Flavorretains#[derive(ConstParamTy)]sonode_fs.rs's const-generic dispatch still compiles; the re-export keepsnode_fs::Flavor/node::fs::Flavorpaths intact.- Two new tests pin the previously-uncovered flag values (
FileBlobs::RejectviaBun.build({files}),StringObjects::RejectviawriteFile/appendFilesync+callback); both pass on main by design since this is behavior-preserving. - CI on the prior head was green except a known main-break (
blob.test.tsunder ASAN, tracked separately); mordant/clippy/format checks pass. - The comment-cop bot flagged 8 doc comments; robobun already responded — they're enum documentation and pre-existing comments with
is_async=true→Flavor::Asynctoken updates, not workaround justifications. Not blocking. - A larger overlapping sweep (#37180) exists but is a 539-file conflicting PR; landing order is a maintainer call, not a correctness concern for this diff.
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with every call site mapping verified.
What was reviewed:
- Traced every
is_async/allow_string_object/allow_fileliteral to its new enum variant across all 9 source files; each maps correctly, including the deliberateallow_filepolarity flip (true→FileBlobs::Reject, matching what the old code actually did). Flavormove preservesConstParamTyand is re-exported fromnode_fs, so const-generic andnode::fs::Flavorcallers are unaffected.- Checked the PBKDF2 error-path guard (
has_exception() && flavor == Async) and the sync-onlyfrom_typed_arrayrewrite still gate on the same conditions. - New tests pin existing behavior (String-wrapper rejection in fs.writeFile/appendFile;
Bun.file()rejection inBun.build({ files })) and usetempDirper harness conventions.
Extended reasoning...
Overview
This PR replaces four bool parameters on StringOrBuffer / BlobOrStringOrBuffer parsers in src/runtime/node/types.rs with two-variant enums (Flavor::{Sync,Async}, StringObjects::{Allow,Reject}, FileBlobs::{Allow,Reject}), updates every caller in BunObject.rs, JSTranspiler.rs, PBKDF2.rs, node_crypto_binding.rs, node_fs.rs, socket_body.rs, and js_valkey_functions.rs, and drops the corresponding bare_bool_args mordant baseline entry. Flavor is the existing node_fs.rs enum moved to types.rs and re-exported. Two behavior-pinning tests are added; a handful of doc comments referencing is_async=true are updated.
Security risks
None. The change is a type-level rename of function parameters. No validation logic, thread-safety gating, or protect/unprotect balancing changes shape — every if is_async becomes if flavor == Flavor::Async, every if !allow_string_object becomes if string_objects == StringObjects::Reject, and the allow_file guard becomes file_blobs == FileBlobs::Reject (semantically identical to the old allow_file == true, which was misnamed). No new inputs are accepted; no security checks are relaxed.
Level of scrutiny
Medium — the change touches shared argument-parsing helpers used by node:fs, node:crypto, zstd, Bun.Transpiler, sockets, and the Redis client, so a mis-mapped literal could flip async/sync buffer protection or silently start accepting/rejecting inputs. I traced every call site in the diff against its old bool literal:
types.rsinternal forwarders:from_js(false,true→Sync,Allow),from_js_async(true,true→Reject,Async),from_js_maybe_file(false→Sync),from_js_with_encoding{,_into}(false,true→Sync,Allow),from_js_with_encoding_value_allow_string_object(is_async=false→Sync),from_js_with_encoding_value_allow_request_response(true→Allow).BunObject.rszstd,JSTranspiler.rstransform:true,true→Async,Allow.PBKDF2::from_js:is_asyncparam →flavor; guardhas_exception() && is_async→&& flavor == Async;if !is_async→== Sync; bothfrom_js_maybe_asynccalls →(flavor, Allow). Callers passAsync/Syncfor oldtrue/false.- Scrypt: const-generic
IS_ASYNCmapped once toflavor, forwarded withAllow(wastrue). node_fs.rsWriteFile::from_js:will_be_asyncmapped toflavor,allow_string_object=false→Reject.socket_body.rs:true→Allow.js_valkey_functions.rs: number pathtrue→Reject, general pathfalse→Allow(matches the old inverted-name semantics).
All map correctly. The Flavor move keeps #[derive(ConstParamTy)], so node_fs.rs's const-generic <const F: Flavor> uses continue to compile, and the pub use super::types::Flavor re-export keeps node_fs::Flavor / node::fs::Flavor paths working.
Other factors
The two new tests are behavior-pinning (documented to pass on main), correctly use tempDir/toThrowWithCode, and assert the negative contract (readdirSync(dir) is empty). The comment-cop bot threads are all resolved and were doc comments on the new enums, not workaround justifications. CI red lanes are pre-existing main failures per the robobun status comment. The overlap with #37180 is flagged in the thread as a coordination note, not a blocker — this PR's approach (reusing Flavor, fixing the allow_file polarity name) is sound on its own.
2 participants
Problem
bare_bool_argsreports four functions insrc/runtime/node/types.rs(thebare_bool_args:src/runtime/node/types.rs = 4baseline entry):BlobOrStringOrBuffer::from_js_maybe_file_maybe_async(.., allow_file, is_async),StringOrBuffer::from_js_maybe_async(.., is_async, allow_string_object),from_js_with_encoding_maybe_async_into(..)andfrom_js_with_encoding_maybe_async(..)(same two bools). Each has a caller passing bare literals, e.g.from_js_maybe_file_maybe_async(global, value, true, true)andfrom_js_maybe_async(global, value, false, true), where nothing says which flag is which.BunObject.rsandJSTranspiler.rspasstrue, allow_string_object,PBKDF2::from_js(.., true)/(.., false)forward a bare bool, Scrypt passesIS_ASYNC, true.allow_fileis also named backwards:allow_file == trueis what makes a file-backedBlobthrow "File blob cannot be used here" (types.rs:109on main). Only the Redis client passesfalse.Fix
from_js_maybe_fileandfrom_js_with_encoding_value_allow_string_object) take the enums instead of bools:is_async->Flavor::{Sync, Async}. This isnode_fs.rs's existing enum for the same fact (is this call serviced inline or on the thread pool). It moves totypes.rsso the argument parsers can use it;node_fs.rsre-exports it, so thenode_fs::Flavor/node::fs::Flavorcallers are untouched.allow_string_object->StringObjects::{Allow, Reject}.allow_file->FileBlobs::{Allow, Reject}, named for what the flag does:from_js/from_js_asyncpassReject(wastrue), the Redis client passesAllow(wasfalse).from_js_maybe_async(global, value, Flavor::Async, StringObjects::Allow), and the bodies compare against a variant where they tested the bool. Every call maps its old literal to the equivalent variant, so there is no behavior change.PBKDF2::from_jstakesFlavoritself, since itsis_asynconly existed to be forwarded; Scrypt (const-genericIS_ASYNC) andWriteFile::from_js(arguments.will_be_async) convert their bool once.is_async=true) are updated, including theThreadSafe::adoptdoc insrc/jsc/node_path.rs.mordant-baseline.toml: thebare_bool_args:src/runtime/node/types.rsentry is removed. Checked with mordant onbun_runtime(cargo dylint --all -p bun_runtime) with the entry removed: the unfixed tree reports the four functions above as over the baseline, this tree reports nothing, andMORDANT_BASELINE_WRITE=1regenerates the[bun_runtime]section without the entry and without any new one. The regeneration also wanted to drop two entries this PR has nothing to do with,narrowed_two_ways:src/runtime/node/node_crypto_binding.rs(itsas usizecast was removed by crypto: store PBKDF2's key length as usize #37648, after the baseline was last regenerated) andalways_unwrapped_option:src/install/PackageInstall.rs, so the file is edited by hand here and those are left for the next full regeneration.bun-cryptohasher.test.tshasBun.file()rejected and in-memory Blob /new Stringinputs accepted by the static hashers,test-fs-write-file-sync.jshaswriteFileSync(new String(..))rejected,bundler_files.test.tshas Blob content accepted).test/bundler/bundler_files.test.ts:Bun.build({ files })rejects aBun.file()blob with "File blob cannot be used here" (FileBlobs::Rejecton thefrom_js_asyncsite).test/js/node/fs/fs.test.ts:writeFileSync,appendFileSync, callbackwriteFileandappendFilereject a String wrapper withERR_INVALID_ARG_TYPEand create nothing (StringObjects::Rejecton the one site that uses it, on both its sync and its thread-pool flavor; Node behaves the same for all four).bun bd:bun bd test test/js/node/fs/fs.test.ts: 511 pass (512 with the new case);bun bd test test/bundler/bundler_files.test.ts: 24 pass.test/js/node/test/parallel/test-fs-write-file-sync.js,test-fs-write-file.js,test-fs-write-file-buffer.js,test-fs-write-file-typedarrays.js,test-fs-append-file.js,test-fs-append-file-sync.js,test-fs-promises-write-optional-params.js,test-fs-write-optional-params.js,test-fs-write.js: all pass.bun bd test test/js/node/crypto/pbkdf2.test.ts test/js/node/crypto/scrypt.test.ts test/js/bun/util/bun-cryptohasher.test.ts test/js/bun/util/zstd.test.ts test/js/bun/transpiler/transpiler-tsconfig-uaf.test.ts: 535 pass (pbkdf2/scrypt sync and async,FileBlobs::RejectviaCryptoHasher, the async zstd andTranspiler.transformpaths).new Stringinputs toCryptoHasher,writeFileSync/fs.promises.writeFile, async zstd, asyncTranspiler.transform, pbkdf2/scrypt sync and async,net.Socket#writewith an encoding, and Redissetwith number / string / Buffer / Blob /Bun.file()arguments against a local server) prints identical output on this build and on the released binary. The valkey test files need the dockerredis_unifiedservice and are skipped locally.Background
StringOrBuffer/BlobOrStringOrBuffer(src/runtime/node/types.rs) parse the "string or Buffer (or Blob)" arguments ofnode:fs,node:crypto,Bun.Transpiler, the zlib/zstd functions, sockets and the Redis client. Parsing for a call that runs on the thread pool has to produce a value that survives leaving the JS thread: strings are copied or re-referenced thread-safely, JS buffers are pinned andprotect()ed until the job's owner callsunprotect(). That is whatis_asyncselected and whatFlavor::Asyncselects now.allow_string_objectexists because Node'sfs.writeFilefamily rejectsnew String("..")wrapper objects while every other caller unwraps them.mordant-baseline.tomlis the ratchet for the lint pack run bybun run rust:mordant: it records the per-(lint, file) finding counts that predate the job, so CI only fails on a new finding.bare_bool_argsfires on a crate-private function with two or moreboolparameters when some call passes baretrue/falsefor at least two of them; fixing a site means deleting (or decrementing) its entry.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts