Skip to content

shell: stop field splitting command substitution in export operands and assignments - #40764

Merged
Jarred-Sumner merged 3 commits into
mainfrom
farm/528d448f/shell-export-field-split
Aug 29, 2026
Merged

Jarred-Sumner merged 3 commits into
mainfrom
farm/528d448f/shell-export-field-split

Conversation

@robobun

@robobun robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #40763.

Problem

  • export NAME=$(echo "hello world") sets NAME=hello and exports a stray empty variable world. bash and dash give NAME=hello world.
  • The expansion of a command argument field splits unquoted command substitution output into several argv words (src/runtime/shell/states/Cmd.rs, the ExpandingArgs arm of child_done). The expansion does not know the word is an assignment operand of export.
  • A plain assignment NAME=$(cmd) hides the same split: Assigns::child_done re-joins the words with single spaces, so runs of whitespace and newlines collapse.

Fix

  • Add an assign_ctx flag to Expansion. When set, the command substitution output is not field split (the same path as a quoted "$(...)"). POSIX 2.9.1: an assignment word undergoes no field splitting.
  • Cmd sets the flag for an operand of export (the only declaration builtin) that starts with a literal NAME= prefix where NAME is a valid identifier. A word whose name comes from an expansion, for example export $(echo "A=a b"), still splits, like in bash.
  • Assigns sets the flag for every assignment value, so VAR=$(echo a && echo b) now keeps the newline instead of collapsing to a b.
  • Verified: three new tests in test/js/bun/shell/bunshell.test.ts fail on current bun and pass with the fix. The full test/js/bun/shell/ suite passes except pre-existing environment failures (root-user permission tests, ASAN timeouts), which fail the same way without the change.

Background

  • The shell interpreter is a tree of state nodes. A Cmd expands each argv atom through an Expansion child, which returns a buffer plus bounds, the offsets that split the buffer into argv words.
  • An unquoted $(...) runs post_subshell_expansion, which turns newlines into spaces and splits on space runs. A quoted "$(...)" skips that and only trims trailing whitespace. assign_ctx reuses the quoted path.
  • Glob and brace expansion of assignment values are unchanged. Only the field split of command substitution output is suppressed.
Notes
  • Repro: bun -e 'await Bun.$export NAME=$(echo "hello world"); echo "NAME=[$NAME]"' prints NAME=[hello] before, NAME=[hello world] after.
  • With shell: export rejects invalid identifiers #40710 (identifier validation in export), the split also turns into a hard error when a split word is not a valid identifier, for example export NAME=$(echo "a b-c"). This fix removes the split, so the two compose.
  • CondExpr passes assign_ctx: false to keep its behavior unchanged. Bash also suppresses splitting inside [[ ]], but that is a separate concern.
  • Variable expansion ($X) was never field split in the Bun shell, so only command substitution output was affected.
  • Known divergence: the AST folds quoted and unquoted text into the same SimpleAtom::Text, so export "NAME="$(cmd) also gets assignment treatment, while bash splits there. Restoring the quoting bit needs a parser AST change. The effect is benign: export already treats any operand with = as an assignment at runtime, so the flag only stops split words from becoming stray exported variables. zsh does not split here either.

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts

An unquoted $(cmd) in an assignment value was field split into several
words. A plain assignment re-joined the words with spaces, but an
export operand like export NAME=$(cmd) kept the split and exported
the extra words as separate variables.

Mark the expansion of an assignment value with assign_ctx, which skips
the field split of command substitution output. Cmd applies it to a
declaration-builtin operand that starts with a literal NAME= prefix.
Assigns applies it to every assignment value, so interior whitespace
is now preserved instead of collapsed.
@coderabbitai

coderabbitai Bot commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file.

Or wait 15 minutes for your next included review.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 638aa228-736d-490d-8574-5053936bfeda

📥 Commits

Reviewing files that changed from the base of the PR and between 69c6138 and dcfe3d8.

📒 Files selected for processing (5)
  • src/runtime/shell/states/Assigns.rs
  • src/runtime/shell/states/Cmd.rs
  • src/runtime/shell/states/CondExpr.rs
  • src/runtime/shell/states/Expansion.rs
  • test/js/bun/shell/bunshell.test.ts

Comment @coderabbitai help to get the list of available commands.

Comment thread src/runtime/shell/states/Cmd.rs Outdated
Comment thread src/runtime/shell/states/Cmd.rs Outdated
Comment thread src/runtime/shell/states/Cmd.rs Outdated
Comment thread src/runtime/shell/states/Expansion.rs Outdated
@robobun

robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:16 AM PT - Aug 28th, 2026

❌ @robobun, your commit dcfe3d8 has 1 failures in Build #107654 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 40764

That installs a local version of the PR into your bun-40764 executable, so you can run:

bun-40764 --bun

Comment thread src/runtime/shell/states/Cmd.rs
Comment thread src/runtime/shell/states/Cmd.rs
Comment thread src/runtime/shell/states/Expansion.rs
Comment thread src/runtime/shell/states/Cmd.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

CI on dcfe3d8: no failure touches this diff. The shell suites passed on every lane. test/js/web/url/url.test.ts fails on darwin x64 on main as well (ICU IDNA table test). The other failures passed on retry or in isolation (hot reload, bun init, install, streams, net, http2, fetch leak, napi worker tests).

@robobun

robobun commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

#40710 (identifier validation in export) makes a split word that is not a valid identifier fail the command, for example export DATE=$(date) reports 28 and 12:37:27 and exits 1. #40710 now says to land this PR first or together with it.

If this PR lands second, consider a test with a split word that is not an identifier, for example export MSG=$(echo "a b-c") && echo "[$MSG]" with stdout [a b-c], empty stderr, and exit 0. That pins the combination of the two changes. Both PRs add tests after exported vars 2 in bunshell.test.ts, so the second one to land needs a small rebase there.

@Jarred-Sumner
Jarred-Sumner merged commit 0fbdf0d into main Aug 29, 2026
9 of 10 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/528d448f/shell-export-field-split branch August 29, 2026 05:19
Jarred-Sumner added a commit that referenced this pull request Aug 29, 2026
… captured stderr through the env's RefCell; Expansion::init callers pass assign_ctx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bun shell: export NAME=$(cmd) field-splits the command substitution output

2 participants