Skip to content

shell: keep variables in one table with an export attribute, and prefix assignments on their command - #45371

Open
robobun wants to merge 10 commits into
mainfrom
robobun/06b9ce56/shell-export-attribute
Open

robobun wants to merge 10 commits into
mainfrom
robobun/06b9ce56/shell-export-attribute

Conversation

@robobun

@robobun robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #32202

Problem

  • T=5; export T; printenv T prints an empty line. export PATH; uname and PATH=/x:$PATH; tool fail with bun: command not found. HOME=/tmp/zz env prints two HOME= lines.
  • Three maps hold values, and "exported" means "in export_env". export NAME writes "" there (builtin/export.rs:35), and NAME=v writes only shell_env (interpreter.rs:2008). Each spawn appends cmd_local_env, which no command clears (states/Cmd.rs:474).

Fix

  • One VarTable replaces shell_env and export_env. Each entry is Exported(value), Local(value) or ExportedUnset. An assignment keeps the export state, and export NAME keeps the value.
  • A NAME=v cmd prefix lives on its Cmd node and ends with it. VarTable::child_env gives the child one entry per name.
  • Verified: bunshell.test.ts and shell-worker-terminate-leak.test.ts (28 tests fail on main's source), then test/js/bun/shell. One test compares 168 sequences with sh.
  • Self-reviewed: 19 concerns raised, 19 addressed.

Background

Downsides

  • Behavior change: an assignment to an inherited name reaches later commands, and a prefix no longer does. On Windows path=dist now replaces Path for children and lookup.
  • Cost: .text +4,144 B, and +864 B per environment copy at 64 variables. A spawn costs 9 more instructions per unexported variable, and a prefix 4 more allocator calls.
  • Still wrong: which, cd and a later prefix (A=1 B=$A cmd) ignore a command's prefixes. A=1 $EMPTY assigns nothing.
Notes

Why the prefix store is in this PR. The first pushes left cmd_local_env alone. That left the second bug half fixed. With NODE_ENV inherited, NODE_ENV=test true; NODE_ENV=production; bun -e 'console.log(process.env.NODE_ENV)' printed test: the plain assignment reached the child, and the stale prefix of the earlier command came after it in the environment. bun and sh take the last of two entries, node and python the first. The prefix change is commits 07bcb8a and f51d531. This PR therefore covers #36513 (closed as stale on 2026-10-31) and the prefix part of #42049. The PATH lookup part of #42049 is not here.

History. This replaces #33549 (bare export NAME) and #33550 (assignment to an exported name). A sweep of stale pull requests closed both on 2026-09-13 with no maintainer comment. The review of #33550 found two shadow regressions in a row (FOO=a; export FOO=b; FOO=c; echo $FOO printed a), each repaired by one more "remove it from the other map" at one writer. The tests here reuse the cases of both. #32202 has four earlier attempts, all closed unmerged: #32203, #32205, #33364 and #36513. A later report in the chat thread named the most visible form of the bare export bug: export PATH; uname gives bun: command not found: uname, and export HOME && cd gives cd: HOME not set. The report says 1.3.14 ran both, because its command lookup did not read the shell's PATH. I checked 1.4.3-canary and this branch, not 1.3.14.

What changes for a user.

  • T=5; export T; cmd: the child gets T=5. It got T= before.
  • export NAME on an inherited name keeps the value. It blanked it before, for the child and for $NAME. So export PATH; cmd finds cmd, and export HOME && cd works.
  • export NAME on an unset name adds nothing to the environment. A later NAME=1 is exported.
  • NAME=value on an inherited or exported name reaches child processes, which, the command lookup on POSIX, the export listing, subshells, pipelines and $( ).
  • NAME=v cmd on an exported name gives the child one NAME=v entry. It got the old entry and the new one before.
  • NAME=v cmd1; cmd2: cmd2 does not get NAME=v. It did before.
  • PATH=x cmd1; cmd2 looks cmd2 up on the shell's PATH. A script that relied on the leaked prefix now gets command not found.
  • FOO=a; export FOO=b; echo $FOO prints b. PWD=junk; cd /tmp; echo $PWD prints /tmp.
  • A=1 export A; echo $A prints 1. A prefix on export assigns only a name that is an operand, as in bash.
  • export with no operand prints a bare NAME for an exported name that has no value.
  • On Windows, variable names are ASCII-case-insensitive, as in process.env: PATH, Path and path are one variable, so an assignment under any spelling changes what child processes receive. On main that was true for $NAME and not for the child. tmp=x now changes the inherited TMP, pwd=saved; cd sub; echo $pwd prints the new directory, and path=dist; node build.js fails with bun: command not found: node. PATH=dir; tool finds tool there: the command lookup matches the key by the same rule since 3ad5807 (name_eql), where it matched PATH byte for byte before. The alternative is a different design: upper-case the inherited names at startup and keep shell names case-sensitive, as Git Bash does.

Code removed. ShellExecEnv::shell_env, export_env and cmd_local_env, ShellExecEnv::assign_var, EnvMap::{iterator, iter, clone, init_with_capacity, ensure_total_capacity, memory_cost}, and the second fill_env call per spawn. EnvMap is now only the prefix map of one command.

Sequences against dash 0.5.12-12. A one-off measurement on 2026-10-16. Its generator is not in the tree. The check that stays in the tree is the test sequences of assignments, exports and prefixes give the result of sh: 84 sequences for an unset name and 84 for an inherited one, against the system sh. The matrix: every sequence of 1 to N operations on one name, observed through printenv in a child and through $NAME in the shell. "Before" is a release build of main a407eae. "After" is this branch at f51d531, release build.

Matrix Operations Before After
assignment and export, name unset or inherited A=1, A=2, A=, export A, export A=3, export A=, up to 4 2,230 of 3,108 0
cd, with PWD inherited PWD=junk, OLDPWD=junk, cd /tmp, cd /usr, export OLDPWD, up to 4 696 of 780 0
subshell, pipeline member, $( ) 7 operations, up to 3 624 of 798 0
prefix on export, shell view only A=1 export A, A=2 export A=3 A, A=4 true, export A, A=5, up to 4 1,026 of 1,560 0
prefix on a command, a builtin and export A=1 true, A=2 printenv A, A=3 export A, export A, A=5, export A=6, up to 4 3,074 of 3,108 0
  • The cd row holds only with PWD in the starting environment. Without it, 120 of the 780 differ, because $PWD is empty until the first cd (see "Not fixed here").
  • The matrices use one name. A prefix whose value reads another prefix (A=1 B=$A cmd) is outside them.
  • dash and bash disagree on 160 of the 780 cd sequences, for two reasons. bash marks OLDPWD exported at the start (26). The cd of bash copies the variable $PWD into OLDPWD, and dash stores the previous directory (134). On the other matrices the two agree.
  • Where bash and dash disagree, this PR keeps what main does: dash for cd, and bash for a prefix on export (only an operand is assigned). A test records each of the two.
  • A further 22 hand-written prefix scripts (pipeline member, subshell, $( ), redirect, &&, if, command not found, two prefixes of one name) give the dash result, except one that uses "$?", which Bun Shell does not expand. 20 of the 22 differ on main.

Tests.

  • 28 tests fail with git checkout --no-overlay origin/main -- src/ packages/ and pass with the branch's source (debug build, ASAN, after the merge of main 2596ac5). They are the exported variables block, export var, external command resolution uses the PATH from the shell environment and the LeakSanitizer case.
  • bunshell.test.ts is on the list of files that run with LeakSanitizer off (test/no-validate-leaksan.txt). The new case in shell-worker-terminate-leak.test.ts runs with it on. It takes each release path of the variable table and of a prefix map, and ends with a live command. With three deref calls deleted (in VarTable::set, EnvMap::insert and Export::assign_prefixes) it reports three leaked strings.
  • Windows: I cannot run Windows here. CI build 138615 ran the Windows tests at 3ad5807 on x64 and arm64, and all 16 test jobs passed: a name in another case is the same variable, cmd /d /c set NAME shows one entry for a name that a prefix replaces, and which and the command lookup find a tool after PATH=dir. The Windows target also type-checks (bun run rust:check-all x86_64-pc-windows-msvc).
  • bunshell.test.ts and shell-worker-terminate-leak.test.ts on the branch: 501 pass, 0 fail.
  • All 44 files of test/js/bun/shell in one bun bd test run: every failure on the branch is a timeout of a slow test on the debug build (fd leak, shell-hang, shell load, ls of node_modules, bun exec > it works2, and once commands/cat.test.ts, which passes alone 3 of 3).

Not fixed here.

Open pull requests in the same files.

Measurements. Taken on 2026-10-16, between main a407eae and this branch at f51d531, before the merge of main 2596ac5. Release builds with --lto=off, env -i with 64 pinned variables, bun run script.bun.sh. valgrind, perf, strace and bloaty are not installed in the build container, and perf_event_open is not permitted there. The counts come from gdb breakpoints on function entry addresses. A "per statement" number is the difference of a 200-statement and a 100-statement script, divided by 100.

  • release binary: .text +4,144 B (76,512,176 to 76,516,320), file size unchanged at 101,439,328 B (llvm-size, stripped).
  • environment fill per spawn, instructions from the start of the fill in Cmd::transition_to_exec to the call of bun_which::which: with 0 unexported variables main 18,812, PR 18,216. With 100: 18,782, 19,231. With 1,000: 18,790, 27,323. Slope: 9.1 instructions per unexported variable (main 0). The array of entry pointers is reserved once now. It grew by doubling before.
  • the same span for a command with one prefix: a new name, main 25,663, PR 26,526. A name that replaces an exported one, main 25,665, PR 27,615. Each exported name is compared with each prefix name.
  • a command with a prefix (A=1 true): main 15.03 allocator calls and 1,358 B per statement, PR 19.03 calls and 1,582 B. The prefix map of a command is a box and three arrays, made for that command. main reuses one map.
  • env copy at 64 variables: main 21.03 allocator calls / 4,685 B per (true); statement, PR 21.03 calls / 5,549 B. Per $ start (Bun.$ from a JS file, two runs each): main 373 calls / 27.1 to 27.4 KB, PR 373 calls / 28.1 KB.
  • sizes (debug builds, gdb): Var is 32 B, EnvStr is 16 B. ShellExecEnv: main 368 B, PR 192 B. Cmd and the state node Node: main 176 B, PR 184 B (the prefix map is an Option<Box<EnvMap>>).
  • first fresh assignment per environment: main 3 calls / 144 B (mi_malloc), PR 3 calls / 6,656 B (mi_realloc, the table grows from 64 to 128 entries). Later fresh assignments: 6.08 allocator calls per statement on both. Requested bytes per statement over the first 100: main 636 B, PR 823 B.
  • name hashes per $NAME (inherited): main 2, PR 1. Unset name: 2, 1. ~: 2, 1. Plain shell variable: 1, 1. Fresh NAME=v, reassignment, export NAME=v, which: 1 on both. cd: 2 on both. (Debug builds at be4b33c, breakpoint on EnvMapContext::hash. The later commits do not touch these paths.)
  • syscalls for A=1; export A; /usr/bin/true; (true); echo $(echo x), three runs each, at be4b33c: main 396, 396, 399. PR 394, 394, 399. Only thread and event-loop calls differ, between builds and between runs of one build. Host functions added: 0.
  • An earlier push kept the export state in two spare bits of the EnvStr tag (16 B entry, no extra bytes per copy). The first self-review rejected it: it is bit-packing where a plain enum is correct, and shell builtins: remove unsafe from rm/cp/yes/ls/mkdir and EnvStr #40262 and shell: replace EnvStr packed u128 with a niche enum over NonNull #37608 both replace the packed EnvStr.

Self-review. Two reviews. The first asked for a restructure, with two findings: no export state in EnvStr tag bits (done in be4b33c), and cmd_local_env was still a second store (done in 07bcb8a). The second read the final diff and said to keep this shape. It raised 19 concerns, none about the design: 9 about tests, Windows and claims in this description, and 10 smaller ones. All 19 are addressed: the tests and the docs sentence in 08e204f, the rest in this description and in a follow-up that is handed off. A bot review of the merged head then raised 7 findings. Two are fixed in 3ad5807 (the PATH key of the Windows lookup, and the reference shell run of the sequence test). The one on Windows name case is open for a maintainer. Four have replies: two are optional and two exist on main.


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/bun/shell/shell-worker-terminate-leak.test.ts, test/js/bun/shell/bunshell.test.ts

@robobun

robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:27 AM PT - Nov 8th, 2026

❌ @robobun, your commit 3ad5807 has some failures in Build #138615 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 45371

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

bun-45371 --bun

@robobun

robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status. Reproduced on the released 1.4.3-canary (367d939) and on builds of main, with dash 0.5.12 and bash 5.2.37 as the reference:

T=5; export T; printenv T          prints an empty line (sh: 5)
export PATH; uname                 bun: command not found: uname (sh runs it)
OUTER=changed; printenv OUTER      prints the inherited value (sh: changed)
PATH=/tmp/xbin:$PATH; mytool       bun: command not found: mytool (sh runs it)
HOME=/tmp/zz env                   prints two HOME= lines (sh: one)
OUTER=pre true; OUTER=changed; bun -e 'console.log(process.env.OUTER)'
                                   prints pre (sh: changed)

This pull request is ready for review. It has main 2596ac5 merged in.

CI build 138615 on 3ad5807: no test of this change failed, and 185 of 188 jobs passed, with every Linux, Windows and macOS arm64 lane. The build is red because of the macOS x64 lane. Two of its four test jobs expired after 7 hours without an agent. One stopped when the release directory with the binary under test disappeared on the agent (ENOENT ... lstat .../builds/darwin-x64-14-8-9-1/bun/bun/release).

Open for a maintainer: the three questions in #45371 (comment). The review thread on Windows name case is the second of them.

Comment thread src/runtime/shell/EnvMap.rs Resolved
Comment thread src/runtime/shell/EnvMap.rs Resolved
Comment thread src/runtime/shell/EnvMap.rs Resolved
Comment thread src/runtime/shell/EnvMap.rs Resolved
Comment thread src/runtime/shell/EnvMap.rs Resolved
Comment thread src/runtime/shell/EnvMap.rs Resolved
Comment thread src/runtime/shell/EnvStr.rs Resolved
Comment thread src/runtime/shell/interpreter.rs Resolved
Comment thread src/runtime/shell/interpreter.rs Resolved
Comment thread src/runtime/shell/subproc.rs Resolved
The prefixes of `NAME=value cmd` were in `cmd_local_env`, a map on the
shell environment that no command cleared. Every later command of the
same shell got them, and the spawn code appended them after the
exported variables, so a name in both reached the child twice.

The prefixes now live on the `Cmd` node (`prefix_env`) and go away with
it. `VarTable::child_env` yields each exported name once, and a prefix
replaces the exported value. `fill_env` takes that one iterator and
reserves the whole array, with the null that ends it, in one call.
…prefixes by operand

`VarTable::child_env` hashed every exported name to ask the prefix map
whether a prefix replaces it. A command has few prefixes, so it now
compares each exported name with the prefix names.

`export` looked for its own prefixes through the labels of the AST and
compared bytes. It now asks the prefix map for each operand name, which
compares names the way the variable table does.
@robobun robobun changed the title shell: keep variables in one table with an export attribute shell: keep variables in one table with an export attribute, and prefix assignments on their command Oct 16, 2026
…s and the release paths

- `cd`: the test reads PWD and OLDPWD in a child, with neither inherited.
- Two tests record a choice between shells: a prefix on `export` assigns
  only an operand (bash), and OLDPWD is exported from the first `cd` (dash).
- Windows: a name in another case is the same variable, and `cmd /c set`
  shows one entry for a name that a prefix replaces.
- The PATH test is two tests. `which` runs on every platform. The lookup
  is a todo on Windows, where it matches the key `PATH` byte for byte.
- A `PATH=` prefix does not decide where a later command is found.
- 168 sequences of assignments, exports and prefixes per name are compared
  with the system `sh`.
- A LeakSanitizer case in shell-worker-terminate-leak.test.ts takes each
  release path of the variable table and of a command's prefix map.
- docs: variable names are ASCII-case-insensitive on Windows.
…l-export-attribute

# Conflicts:
#	src/runtime/shell/ParsedShellScript.rs
#	src/runtime/shell/builtin/export.rs
#	src/runtime/shell/interpreter.rs
#	src/runtime/shell/states/Cmd.rs
@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

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

Learn how review limits work.

Review configuration:

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 2596ac5 and 3ad5807.

Files selected for processing: 12

  • docs/runtime/shell.mdx
  • src/runtime/shell/EnvMap.rs
  • src/runtime/shell/ParsedShellScript.rs
  • src/runtime/shell/builtin/export.rs
  • src/runtime/shell/builtin/which.rs
  • src/runtime/shell/interpreter.rs
  • src/runtime/shell/states/Assigns.rs
  • src/runtime/shell/states/Cmd.rs
  • src/runtime/shell/states/Expansion.rs
  • src/runtime/shell/subproc.rs
  • test/js/bun/shell/bunshell.test.ts
  • test/js/bun/shell/shell-worker-terminate-leak.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@robobun

robobun commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator Author

@Jarred-Sumner This is the one pull request for the two lost-value bugs from the thread (export NAME loses the value, and NAME=value on an exported variable does not reach a child). It is ready for review. Three decisions are yours.

1. The prefix change. Commits 07bcb8a and f51d531 move the NAME=v cmd prefixes onto their command. Without them the second bug stays half fixed: with NODE_ENV inherited, NODE_ENV=test true; NODE_ENV=production; bun -e 'console.log(process.env.NODE_ENV)' prints test. The change also fixes #32202, and it changes the export var test, which expected a prefix to reach the next command. If you want only the two bugs you named, I can take the prefix change out.

2. Windows names. The one table compares names without case on Windows, as process.env does. So path=dist now changes the inherited Path that a child gets. On main only $NAME ignored the case. The alternative is to upper-case the inherited names at startup and keep shell names case-sensitive, as Git Bash does.

3. Order against #40262. VarTable uses the Copy EnvStr of main. On your stack it needs a port of roughly 120 lines, in either merge order. The mapping is in a note on #40262.

@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.

Beyond the inline findings, I also traced the EnvStr refcounts through VarTable::set/export/clone/Drop and Export::assign_prefixes (the prefixes.get ref is consumed by the single deref after vars.set takes its own), and the new Cmd.prefix_env box is released by EnvMap's Drop when the node is dropped — I found no imbalance there. I did not build or run the test suite; the Windows case-folding and sh differential checks rest on reading only.

Extended reasoning...

The change replaces three EnvMaps in the Bun Shell interpreter with a single VarTable carrying an export attribute, moves per-command NAME=v prefixes onto the Cmd node, and updates export/which/cd, child env construction, docs and tests. No auth, crypto or network surface is touched; the sensitive part is manual EnvStr reference counting and the Windows case-insensitive env key handling. Seven confirmed findings (Windows case-fold collisions, Path spelling on Windows, lost prefix on empty argv, PATH not found when not inherited, test shape) already go inline, so a human look is needed; this note only records the refcount and ownership paths I checked and ruled out.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/runtime/shell/subproc.rs — Windows users who assign PATH=...; then run a tool get bun: command not found while the child they do manage to spawn now receives the new value; POSIX finds the tool. child_env filters the inherited Path case-insensitively (EnvMap.rs:225) and emits the stored key spelling, but fill_env updates self.path only when key == b"PATH" byte-exact at subproc.rs:1412, so argv[0] lookup keeps the event-loop PATH. Fix: compare the key with the same case rule as EnvMapContext.eql on Windows (or resolve lookup PATH via vars.get_exported(PATH)), so lookup, which and the child env agree; drop the .todoIf(isWindows) at bunshell.test.ts:1536. [also at: src/runtime/shell/subproc.rs:1414 - Windows users who assign PATH in a script, then run a tool that lives only there, get bun: command not found while the child environment they hand to other programs already says Path=<newdir>.]

    Why this was flagged

    Trigger: on Windows, a script does PATH=C:\tools;$PATH; mytool or export PATH=...; mytool where mytool is only on the new PATH, through the normal JS $ path where .env() is process.env (key spelled Path). VarTable::set at EnvMap.rs:158 keeps the existing key Path, so child_env at EnvMap.rs:225 yields (Path, newvalue); fill_env at subproc.rs:1412 compares the key byte-for-byte with PATH, so spawn_args.path stays the process PATH from SpawnArgs::default and bun_which::which at Cmd.rs:483 misses the tool. Meanwhile which mytool reports it found, so the user sees which and execution disagree. The base branch never delivered the assignment to the child at all; the PR now delivers it to the child but not to lookup, which is a new inconsistency the .todoIf(isWindows) at bunshell.test.ts:1536 only records. Remedy: match the key case-insensitively on Windows at subproc.rs:1412, or take the lookup PATH from vars.get_exported(PATH).

    Verification: Acknowledged in diff: test/js/bun/shell/bunshell.test.ts:1535-1536 with test.todoIf(isWindows). On Windows VarTable::set keeps the inherited key Path (EnvMap.rs:157), child_env yields (Path, newvalue), and fill_env only updates self.path when key == b"PATH" (src/runtime/shell/subproc.rs:1412-1414), so bun_which::which at Cmd.rs:483 fails. On the base the same byte-exact compare already answers "command not found".

value.ref_();
/// `NAME=value`: the name keeps its export attribute. Refs `value`, like `EnvMap::insert`.
pub(crate) fn set(&mut self, name: EnvStr, value: EnvStr) {
let slot = self.map.get_or_put(name).expect("OOM");

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.

🔴 Windows users whose script assigns a lowercase shell variable such as path=dist, os=$(uname) or tmp=... now hand that value to every later child process under the inherited Path/OS/TMP name, breaking those children; on the base branch the child kept the inherited value. VarTable::set at src/runtime/shell/EnvMap.rs:158 looks the name up with the ASCII-case-insensitive EnvMapContext, finds the inherited Exported entry and overwrites its value in place, so child_env emits it. Fix: a plain assignment whose spelling differs from an inherited name must not change what children receive for that name, with the same rule at export (EnvMap.rs:182) and the cd writes of PWD/OLDPWD (interpreter.rs:1981).

Why this was flagged

A Bun Shell script on Windows runs path=./dist; node build.js. The default env is process.env, so the table holds Path, OS, TMP as Var::Exported (src/runtime/shell/ParsedShellScript.rs:112). Assigns::child_done calls shell.vars.set(name, value_ref) (src/runtime/shell/states/Assigns.rs:144); set calls self.map.get_or_put(name) at src/runtime/shell/EnvMap.rs:158, and EnvMapContext::eql (EnvMap.rs:35) compares case-insensitively on Windows, so found_existing is true and the Var::Exported(old) arm at EnvMap.rs:170 replaces the inherited value. child_env (EnvMap.rs:224) then yields Path=./dist to fill_env and node is spawned with a one-entry Path, so anything it spawns (npm, cmd, git) fails with not-found. On the base branch the assignment went to the separate shell_env map (assign_var, old interpreter.rs:2008) and the child was built from export_env only, so it kept the inherited Path/OS. The test "a name in another case is the same variable on Windows only" (test/js/bun/shell/bunshell.test.ts) pins the new behaviour.

Verification: VarTable::set (EnvMap.rs:157-176) does self.map.get_or_put(name) at line 158, so path finds the existing Path entry, and lines 170-173 replace it in place with Var::Exported(value). child_env (EnvMap.rs:224-229) then emits Path=./dist to every later child. On the base, AssignCtx::Shell wrote only shell_env.insert(label, value), so shell_env entries never reached the child.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Kept for now, and it is a maintainer decision (question 2 in #45371 (comment)).

The shell's variable names are ASCII-case-insensitive on Windows on main too: EnvMapContext is the key rule of all three maps. So on main path=dist already changes what $PATH and $Path expand to in the shell. Only the child disagreed with the shell, and that disagreement is the bug this PR fixes.

A rule by spelling cannot keep the fix. The inherited key is Path, so PATH=C:\tools;$PATH is also "a spelling that differs from an inherited name". To keep path=dist local and let PATH=... reach the child, the shell needs another model: names upper-cased at startup and case-sensitive after that, as Cygwin does for a fixed list. That changes the names a child sees.

You are right about the cost, and after 3ad5807 it is larger: on Windows path=dist; node build.js now fails with bun: command not found: node, because the lookup reads the same variable. The description lists this under Downsides, the docs state the rule, and the test a name in another case is the same variable on Windows only records it. I leave this thread open for the maintainer.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

One precedent for the other model, for whoever decides. deno_task_shell upper-cases environment names on Windows and keeps shell variables case-sensitive (src/shell/types.rs: apply_env_var upper-cases the name, and EnvChange::SetShellVar updates an environment variable only when the name as written is a key of env_vars). There PATH=x updates the environment and path=dist stays a shell variable. The cost of that model is that a child gets the upper-cased names (PATH, not Path), and that Path=x is a shell variable too.

This PR keeps the rule that main already has for the shell on Windows (one name for every spelling), so the thread stays open as a design question, not as a defect I missed.

Comment thread src/runtime/shell/states/Cmd.rs Resolved
Comment thread test/js/bun/shell/bunshell.test.ts Resolved
Comment thread test/js/bun/shell/bunshell.test.ts Resolved
Comment thread src/runtime/shell/interpreter.rs Resolved
Comment thread src/runtime/shell/builtin/which.rs Resolved
…the variable table

`fill_env` took the lookup path from the entry whose key is `PATH` byte
for byte. On Windows the inherited key is `Path`, and the variable table
keeps that spelling, so after `PATH=dir` the child got the new value and
the lookup still used the PATH of the process. `name_eql` is now the one
rule for variable names (ASCII-case-insensitive on Windows), used by the
table, by `child_env` and by the lookup. The lookup test runs on Windows.

The sequence test runs the reference `sh` with `Bun.spawn`, beside the
Bun Shell run, and checks the exit code and stderr of both.
@robobun

robobun commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator Author

The finding outside the diff is fixed in 3ad5807 (Windows: PATH=dir; tool reached the child and not the command lookup). fill_env matched the key PATH byte for byte, and the variable table keeps the inherited spelling Path. name_eql in src/runtime/shell/EnvMap.rs is now the one rule for variable names, and the table, child_env and the lookup use it. The test a plain assignment to PATH is used by command lookup has no todo on Windows now.

I cannot run Windows here. The Windows target type-checks (bun run rust:check-all x86_64-pc-windows-msvc), and CI runs the test.

Each inline finding has a reply. Five threads are resolved. The thread on Windows name case stays open for the maintainer.

@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.

I re-reviewed the latest push (3ad5807) and found no new bugs; because this rewrites how Bun Shell stores and exports variables and codifies a Windows name-folding design choice, a human look is still worthwhile.

What was reviewed on this push:

  • name_eql now shared by EnvMapContext::eql, child_env and fill_env's PATH detection; the refcount arms of VarTable::set/export and Export::assign_prefixes balance on every path, and Cmd.prefix_env is released when free_node overwrites the node.
  • The sequence test now runs sh via Bun.spawn with concurrent drains and asserts its exit code and stderr, and the PATH command-lookup test is enabled on Windows.
  • Not done: no build or test run was performed; the sh differential and the LeakSanitizer case were checked by reading only.
Extended reasoning...

The diff replaces the shell's three environment maps with a single VarTable carrying an export attribute, moves NAME=value prefixes onto their Cmd node, and routes export, which, cd, subshell cloning and child-env construction through it, with tests and docs. It touches no auth, crypto or injection surface, but it changes user-visible shell semantics (what children inherit, PATH lookup, and on Windows which inherited variable a lowercase assignment overwrites, now asserted by a new Windows test). The latest commit addressed the spawnSync nit from the earlier review and unified PATH key matching; the hunt ran dry with no new findings. Deferring rather than approving because the change is large, alters behavior users depend on, and the Windows case-folding choice is a design decision the author themselves lists with an alternative.

Still open from earlier reviews (1):

  • 🔴 src/runtime/shell/EnvMap.rs:160 — Windows users whose script assigns a lowercase shell variable such as path=dist, os=$(uname) or tmp=... now hand that v…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

This branch has not been deployed

No deployments
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: prefix assignment of an existing env var duplicates the environ entry (child sees the old value)

2 participants