Conversation
|
Updated 11:06 PM PT - May 4th, 2026
❌ @robobun, your commit 30d9121 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 28818That installs a local version of the PR into your bun-28818 --bun |
WalkthroughParses and filters Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bun.zig`:
- Around line 2067-2070: NodeOptionKind's two-state enum is too coarse and lets
required-value flags (e.g., --require, --import, --title, --dns-result-order) be
treated like bare boolean flags; change NodeOptionKind to at least three states
(bool_flag, optional_value, required_value), update the parsing logic used by
appendNodeOptionsEnv and the related token-consumption code so that
required_value flags are dropped unless they include their value inline in
NODE_OPTIONS (do not consume the next argv token as the flag's value), while
optional_value flags may still be emitted bare; ensure all checks that
previously matched NodeOptionKind::option are revised to distinguish required vs
optional so bare required-value flags are never injected.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9dbe2e56-8a97-4feb-b8bf-ce2a88587390
📒 Files selected for processing (4)
src/bun.zigsrc/env_var.zigsrc/js/node/dns.tstest/regression/issue/28817.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28817.test.ts`:
- Around line 1-3: In the regression test test/regression/issue/28817.test.ts
remove the two extra comment lines "// Bun should honor
NODE_OPTIONS=--dns-result-order and other Node-compatible" and "// flags set via
the NODE_OPTIONS environment variable." so only the single-line GitHub issue URL
comment on Line 1 remains; keep the file name and test intact and do not modify
any test logic or other content.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82c0a90d-2c3c-4ed0-81d6-9296d27625bc
📒 Files selected for processing (1)
test/regression/issue/28817.test.ts
Node.js honors flags passed via the NODE_OPTIONS env var. Bun ignored
it entirely, so things like NODE_OPTIONS=--dns-result-order=ipv4first
were dropped and 'bun install' would hang on hosts with broken IPv6.
Parse NODE_OPTIONS (quote- and escape-aware, like Node) and inject the
tokens into argv before clap parses them. Only flags on an allowlist
of Bun-supported Node-compatible options are honored; positionals and
unknown flags are dropped so the env var can't be used to inject
scripts or change the entrypoint.
Also fixes dns.getDefaultResultOrder() to return the order string
("verbatim"/"ipv4first"/"ipv6first") instead of the internal
function.
Fixes #28817
- Track NODE_OPTIONS-injected flags in bun_options_argc so standalone compiled binaries compute the correct passthrough offset and clap parses the full injected window. - Drop bare required-value flags (e.g. NODE_OPTIONS="--require") when no value follows in the env var itself. Previously the flag was emitted bare and clap bound the user's entrypoint as the missing value — reopening the entrypoint-hijack vector this filter closes. - Preserve empty quoted values (NODE_OPTIONS='--title ""') instead of dropping them. - Backslash has no special meaning inside single quotes (POSIX), so it no longer eats the closing apostrophe. - Add regression tests for bare required-value flag handling. - Drop the prose comment from the test file per review.
d8a014f to
30d9121
Compare
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
What
Parse and honor the
NODE_OPTIONSenvironment variable, the same way Node.js does.Why
Node honors flags like
NODE_OPTIONS=--dns-result-order=ipv4firstthat set runtime options via the environment. Bun ignoredNODE_OPTIONSentirely, which meant tools likebun installwould hang in environments with broken IPv6 even when the user tried the Node-compatible workaround.Reported in #28817:
NODE_OPTIONS="--dns-result-order=ipv4first" bun installstalled at🔍 Resolving.How
src/bun_core/util.rs—argv_view_initalready splicesBUN_OPTIONStokens into argv after argv[0]. Right after that, do the same forNODE_OPTIONSvia a newappend_node_options_env:--flag ""preserves the empty value).node_option_kind) of Bun-supported flags that are safe to set via the environment. Unknown flags and positional args are dropped, so the env var cannot inject a script or change the entrypoint.--require,--dns-result-order) that appear bare with no following value are dropped entirely, so a bareNODE_OPTIONS="--require"cannot bind the user's entrypoint as the missing value.Injected tokens count toward
bun_options_argc(alongsideBUN_OPTIONS) so standalone compiled binaries compute the correct passthrough offset.src/bun_core/env_var.rs— register theNODE_OPTIONSaccessor.The allowlist covers the flags Bun implements that Node allows in
NODE_OPTIONS:--dns-result-order,--conditions/-C,--import,--require/-r,--preserve-symlinks,--preserve-symlinks-main,--title,--max-http-header-size,--inspect/--inspect-brk/--inspect-wait,--expose-gc,--no-addons,--use-system-ca/--use-openssl-ca/--use-bundled-ca,--no-deprecation/--throw-deprecation,--zero-fill-buffers,--unhandled-rejections,--cpu-prof*, and--heap-prof*.Note on this PR's history
This PR was originally written against the Zig tree.
mainhas since migrated the runtime to Rust, so the branch was reset onto currentmainand the feature reimplemented in Rust (src/bun_core/util.rs+src/bun_core/env_var.rs). All the earlier review feedback is carried over in the Rust port:bun_options_argcincludes NODE_OPTIONS-injected args (correct passthrough offset for standalone binaries).--flag "") are preserved.The
dns.getDefaultResultOrder()return-value fix that was part of the original PR is already onmain(merged via #28949), so it is no longer part of this diff.Verification
Tests in
test/regression/issue/28817.test.tscover--dns-result-order(both--flag=valueand--flag valueforms, quoted values), default verbatim behavior, unknown-flag and positional-arg dropping,--evalsafety,--expose-gc,--title, and the bare required-value flag cases. 8 tests pass on the debug build; 6 fail on stock bun (the 2 that pass are safety cases that trivially hold when NODE_OPTIONS is ignored).Review follow-ups (Node parity)
ParseNodeOptionsEnvVarexactly: single quotes are literal, backslash only escapes inside double quotes (unquoted Windows paths keep their backslashes), and only ASCII space separates args. Verified against node v26.-Cfrom the allowlist (Bun has no-Cshort form for--conditions; clap would reject it).--inspect/--inspect-brk/--inspect-waitare classified as optional-value so bareNODE_OPTIONS=--inspectis kept rather than dropped.Fixes #28817