Repository navigation
shell: return which's PATH and cwd lookup as a struct instead of a tuple - #39131
Conversation
path_and_cwd returned (Vec<u8>, Vec<u8>) and both callers unpacked it as
(path_env, cwd) before handing the pair to resolve(path_env, cwd, arg),
where the two same-typed slices could be swapped without a compile error.
Fold both helpers into a SearchEnv { path_env, cwd } struct with a
resolve(&self, arg) method so the names live in the type. No behavior
change.
Drop the cleared tuple_wants_struct entry for which.rs from
mordant-baseline.toml.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical tuple→struct refactor to clear a mordant lint.
What was reviewed:
SearchEnv::loadis byte-for-byte the oldpath_and_cwdbody (including theEnvMap::getref/deref balance) andresolvepassespath_env, cwdtobun_which::whichin the same order.- Both call sites preserve their prior load frequency: once per command in
start's sync loop, once per arg innext. - The baseline drop removes exactly the one
tuple_wants_struct:…/which.rsentry the change clears; the two stale entries the description mentions are intentionally left for main.
Extended reasoning...
Overview
This PR refactors src/runtime/shell/builtin/which.rs to replace a (Vec<u8>, Vec<u8>)-returning helper (Which::path_and_cwd) and its positional consumer (Which::resolve(path_env, cwd, arg)) with a file-private SearchEnv { path_env, cwd } struct that carries load and resolve methods. The only other change is deleting the corresponding tuple_wants_struct line from mordant-baseline.toml. The Which::arg helper moves a few lines up but is unchanged.
Security risks
None. This is a type-shape refactor of a shell builtin's internal helper; no parsing, no boundary crossing, no new inputs. The values ($PATH, cwd, arg bytes) flow to bun_which::which exactly as before.
Level of scrutiny
Low. The diff is a pure behavior-preserving reshape driven by a lint ratchet. I diffed each moved block against its original: SearchEnv::load reproduces the export_env.get(b"PATH") → .slice().to_vec() → s.deref() → unwrap_or_default() sequence and the shell.cwd().to_vec() verbatim; resolve still acquires a path buffer from the pool and calls bun_which::which(buf, path_env, cwd, arg) in the same argument order. The two callers in start (sync loop, load once) and next (streaming, load per arg) keep their prior semantics — start binds search before the loop and reuses it, matching the old single (path_env, cwd) destructure.
Other factors
Existing test coverage (test/js/bun/shell/commands/which.test.ts and the which cases in bunshell.test.ts) exercises both the found and not-found paths, and the PR description reports both suites passing under bun bd. No new test is expected for a no-behavior-change lint fix. The baseline edit is minimal and scoped to exactly the entry this change clears.
…names Two fixture directories hold an executable of the same name; one is put on the shell's PATH and the other is made its cwd. `which tool ./tool` has to print them in that order, on both the captured-stdout path and the streamed path, so feeding the two SearchEnv members to bun_which::which in the wrong order shows up as the directories swapping in the output. The existing which tests pass with the members transposed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical tuple→struct refactor with no behavior change.
What was reviewed:
SearchEnv::load/resolvepreserve the exact$PATH/cwd lookup andbun_which::which(buf, path, cwd, bin)argument order from the oldpath_and_cwd/resolvepair, including theEnvMap::getref/deref balance.- Both call sites (
startsync loop,nextper-arg) keep their original load frequency;Which::argis unchanged, only relocated. - New tests follow harness conventions (
tempDir+using,bunEnv/bunExe, concurrent pipe drain, stdout asserted before exit code, Windows.exebranch); the comment-cop feedback was addressed in 8acff7e.
Extended reasoning...
Overview
This PR clears one tuple_wants_struct mordant finding in the shell which builtin. Which::path_and_cwd returned (Vec<u8>, Vec<u8>) and Which::resolve took the two slices positionally; both are replaced by a file-private SearchEnv { path_env, cwd } with load (same body as path_and_cwd) and resolve(&self, arg) (same call to bun_which::which). The two call sites in start and next are updated in place, the baseline entry is removed from mordant-baseline.toml, and two tests are added that pin down which input feeds bare-name vs ./name lookups on both the captured and streamed stdout paths.
Security risks
None. No new inputs, no parsing changes, no permission or filesystem semantics touched — the same bytes reach bun_which::which in the same order as before. The struct is private to the file.
Level of scrutiny
Low. This is a type-shape refactor the lint tool asked for; I diffed the old and new helper bodies line-by-line and confirmed against src/which/lib.rs:100 that the (path, cwd, bin) positional order is preserved. Which::arg and the BuiltinState-provided state_mut are untouched. The mordant baseline change is the single expected deletion.
Other factors
The one piece of automated feedback (comment-cop on a long doc comment) was resolved by dropping the comment in 8acff7e; the thread is marked resolved. The new tests use tempDir/using, test.concurrent, Promise.all for pipe draining, assert stdout before exit code, and branch on isWindows for the fixture name and chmod — all in line with the repo's test guidance. No CODEOWNERS cover these paths. No prior claude[bot] review exists on this PR.
There was a problem hiding this comment.
LGTM — mechanical tuple→struct refactor that clears the tuple_wants_struct mordant finding with no behavior change.
What was reviewed:
SearchEnv::load/resolvepreserve the exact body of the oldpath_and_cwd/resolve, including theEnvMap::getref/deref balance and thebun_which::which(buf, path, cwd, bin)argument order (checked againstsrc/which/lib.rs:100).- Both callers keep their prior load cadence: once per command in the sync
startpath, once per arg in the streamingnextpath. - New tests follow harness conventions (
tempDirwithusing,test.concurrent, concurrent pipe drain, stdout asserted before exit code) and are hermetic; the comment-cop feedback on the struct doc comment was already addressed and resolved.
Extended reasoning...
Overview
This PR replaces Which::path_and_cwd() -> (Vec<u8>, Vec<u8>) and Which::resolve(path_env, cwd, arg) in src/runtime/shell/builtin/which.rs with a file-private SearchEnv { path_env, cwd } struct exposing load(interp, cmd) and resolve(&self, arg). It removes the corresponding tuple_wants_struct:src/runtime/shell/builtin/which.rs entry from mordant-baseline.toml and adds two tests to test/js/bun/shell/commands/which.test.ts that pin down which of the two byte slices ($PATH vs cwd) each lookup reads.
Security risks
None. This is an internal refactor of a shell builtin's helper functions with no change to input handling, no new syscalls, and no change to how bun_which::which is invoked. The tests write only to per-test temp directories.
Level of scrutiny
Low. The Rust change is a pure shape refactor: the body of SearchEnv::load is byte-for-byte the old path_and_cwd (including the s.deref() refcount balance) with a struct return instead of a tuple, and resolve passes &self.path_env, &self.cwd to bun_which::which in the same order the old positional call did — verified against the signature at src/which/lib.rs:100 (which(buf, path, cwd, bin)). Both call sites (start loading once outside the loop, next loading per-arg) keep their prior semantics. The baseline file change removes exactly the one cleared entry and nothing else.
Other factors
The new tests are well-constructed per REVIEW.md: hermetic fixture dirs via tempDir with using, chmodSync gated on !isWindows, test.concurrent for the independent subprocess case, all three pipes drained concurrently via Promise.all, and stdout/stderr asserted before the exit code. They cover both the captured-stdout (sync start) and streamed-stdout (async next) code paths, and the PR description explains they fail if the two slices are swapped — so they guard the exact property the struct is meant to make un-transposable. The one prior bot comment (comment-cop about a paragraph-long doc comment on the struct) was addressed in a follow-up commit and the thread is resolved. No prior reviews from me on this PR.
Problem
tuple_wants_structflagsWhich::path_and_cwdinsrc/runtime/shell/builtin/which.rs: it returned(Vec<u8>, Vec<u8>), and both callers (Which::startandWhich::next) unpacked it as(path_env, cwd)and passed the pair straight toWhich::resolve(path_env, cwd, arg).resolvecall) compiles. The names existed at every call site but not in the type.whichtests only look up a bare name on the real$PATH, and they still pass with the two members swapped.Fix
SearchEnv { path_env, cwd }struct:SearchEnv::load(interp, cmd)does the same$PATH/ cwd lookup (including theEnvMap::getref/deref balance), andSearchEnv::resolve(&self, arg)wrapsbun_which::whichso the two slices are never passed positionally again.SearchEnv::load(..)+.resolve(&arg). The sync path instartstill loads once per command and the streaming path innextstill loads once per argument, as before. No behavior change.tuple_wants_struct:src/runtime/shell/builtin/which.rsentry frommordant-baseline.toml.bun run rust:mordant:baselineon this branch drops that entry and adds none, so the new struct does not trade one finding for another. It also drops two entries that are already stale on main and unrelated to this change (always_unwrapped_option:src/install/PackageInstall.rs,narrowed_two_ways:src/runtime/node/node_crypto_binding.rs, the latter cleared by crypto: store PBKDF2's key length as usize #37648); those are left as they are here.test/js/bun/shell/commands/which.test.ts): two fixture directories each hold an executable namedtool; one is the shell'sPATH, the other its cwd, andwhich tool ./toolmust print them in that order. One case captures stdout (thestartpath), the other streams it from a child process (thenextpath). Because the change is a refactor the test passes before and after it; what it pins down is the role of each member: with the two slices passed tobun_which::whichin the other order, both cases fail with the two directories swapped in the output, while the pre-existing tests stay green.bun bd test test/js/bun/shell/commands/which.test.ts(5 pass, and 2 fail as described with the members deliberately swapped);bun bd test test/js/bun/shell/bunshell.test.ts -t which(2 pass); the new cases also pass on Windows (tool.exefixture) with the current canary.Background
whichbuiltin has two output paths: when stdout is captured it resolves every argument synchronously instart; when stdout needs real IO it resolves one argument pernextcall and waits for each write. Both need$PATH(from the shell's export env) and the shell's cwd, whichbun_which::which(buf, path, cwd, bin)takes as two positional byte slices: a bare name is looked up inpathonly, a name containing a slash is resolved againstcwdonly.mordant-baseline.tomlis the ratchet for the mordant lint pack: it records the per-(lint, file) finding counts that predate the CI job, so CI only fails on new findings. Fixing a site means deleting (or decrementing) its entry.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/shell/commands/which.test.ts