completions: name the bunx symlink path after the symlink parameter it feeds - #39117
Conversation
…t feeds install_bunx_symlink_posix builds the path of the symlink it is about to create and passes it as the second argument of bun_sys::symlink(target, link), but called it `target`, which is the name of the first parameter (the path the link points at, `exe` here). The calls are in the right order: `bun completions` creates <dir>/bunx -> <path to bun>. Rename the local to link_path (and its buffer to link_buf) so the name matches the parameter, and drop the four arg_named_like_other_param entries this file had in mordant-baseline.toml.
|
Updated 10:33 AM PT - Aug 15th, 2026
⏳ @robobun, your commit 1c6d66b is still building in
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 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 (2)
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 (2)
💤 Files with no reviewable changes (1)
WalkthroughChangesThe POSIX completion installer now reuses one Completion symlink cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure local-variable rename to clear a mordant lint, no behavior change.
What was reviewed:
- Confirmed
bun_sys::symlink(target, link)signature at src/sys/lib.rs:2602 — the second arg is the link path, solink_pathis the correct name andexeremains the target. - All four call sites and the shared buffer are renamed consistently; argument order is unchanged.
- Baseline entry removal matches the count (4) and leaves the rest of the
[bun_runtime]section intact.
Extended reasoning...
Overview
This PR renames a local variable target → link_path (and its backing buffer target_buf → link_buf) at four sites in install_bunx_symlink_posix in src/runtime/cli/install_completions_command.rs, and deletes the corresponding arg_named_like_other_param entry (count 4) from mordant-baseline.toml. The mordant lint flagged these because a local named target was being passed as the link parameter of bun_sys::symlink(target, link), which reads like a transposed call. The calls were already correct — only the local name was misleading.
Security risks
None. This is a variable rename with identical codegen; no new inputs, no changed control flow, no path handling changes.
Level of scrutiny
Low. Pure mechanical rename in a POSIX-only helper of the bun completions CLI command. I verified against src/sys/lib.rs:2602 that symlink's second parameter is indeed link, so the new name matches the parameter it feeds and no longer collides with the first parameter's name — which is exactly what the lint keys on. The baseline delta is the minimal correct edit (delete one line whose count was 4, matching the four fixed sites).
Other factors
No test is expected per the repo's own guidance — nothing observable changes, and the PR description documents a manual before/after check with both release and debug builds. The diff is small, self-contained, and the bug hunting system found nothing.
Runs a private hardlink (or copy) of the executable so the first candidate location is inside the temporary directory, then checks each of the four locations install_bunx_symlink_posix tries in turn: next to the executable, $BUN_INSTALL/bin, $HOME/.bun/bin and $HOME/.local/bin. Each must end up holding a symlink whose target is the executable.
There was a problem hiding this comment.
LGTM — pure variable rename to satisfy the arg_named_like_other_param lint, plus new test coverage for the bunx-symlink fallback chain. One inline nit on the test's undrained stdout pipe; not blocking since nothing writes to stdout on the Shell::Unknown path.
What was reviewed:
- Confirmed
bun_sys::symlink(target, link)signature atsrc/sys/lib.rs:2602— the second arg is the link path, so the rename is correct and argument order is unchanged. - Baseline entry removal matches the 4 fixed sites; other
bun_runtimeentries are untouched. - Test is hermetic (temp dir, private exe copy, empty PATH, unset SHELL/BUN_INSTALL) and asserts each of the four fallback locations with
readlinkSync.
Extended reasoning...
Overview
This PR renames the local target/target_buf to link_path/link_buf at four sites in install_bunx_symlink_posix in src/runtime/cli/install_completions_command.rs. The old name was misleading: the variable holds the link path (second argument to bun_sys::symlink(target, link)), not the symlink target, and it triggered mordant's arg_named_like_other_param lint because it matched the name of a different parameter. The corresponding baseline entry (count 4) is removed from mordant-baseline.toml. A new test in test/cli/bun.test.ts exercises the four-directory fallback chain (<exe dir>, $BUN_INSTALL/bin, $HOME/.bun/bin, $HOME/.local/bin) and asserts the created symlink points at the executable's realpath.
Security risks
None. This is a variable rename with no change to argument order or values; I verified against the bun_sys::symlink signature at src/sys/lib.rs:2602. The new test is hermetic (temp dir, no network) and skipped on Windows where the code path differs.
Level of scrutiny
Low. The Rust diff is a mechanical identifier rename inside a single function — every bun_sys::symlink(exe, target) becomes bun_sys::symlink(exe, link_path) with the arguments in the same positions. The baseline change is a one-line deletion consistent with the four resolved findings. The test is additive coverage for pre-existing behavior.
Other factors
The bug hunter flagged one nit: installBunx sets stdout: "pipe" but only drains stderr. Traced the SHELL-unset path in exec() — it writes only to stderr (Output::err_generic) and exits before the piped-completions branch, so stdout stays empty and cannot deadlock. It's a convention nit worth fixing but not a correctness issue. The test otherwise follows harness conventions (tempDir, bunEnv spread, await using, exit code asserted last) and correctly isolates the first candidate location by hardlinking/copying the executable into the temp dir. No prior reviews from me on this PR.
There was a problem hiding this comment.
LGTM — pure local rename, and the earlier stdout-drain nit is now addressed in 1c6d66b.
What was reviewed:
- Confirmed
bun_sys::symlink(target, link)atsrc/sys/lib.rs:2602— the four calls pass(exe, link_path), so argument order is unchanged and the rename only fixes the misleading local name. - Traced the new test's four
installBunxcalls againstinstall_bunx_symlink_posix: each run's env forces the intended fallback (EEXIST onbin/, then$BUN_INSTALL/bin,$HOME/.bun/bin,$HOME/.local/bin), andreadlinkSync(...) === exeRealpathpins the argument order. installBunxnow drains stdout in the samePromise.alland asserts it is empty;SHELL: undefinedkeeps the child on theShell::Unknown→ stderr →exit(1)path so no shell files are written outside the temp dir.
Extended reasoning...
Overview
Three files: src/runtime/cli/install_completions_command.rs renames the local target/target_buf to link_path/link_buf at the four bun_sys::symlink call sites in install_bunx_symlink_posix; mordant-baseline.toml drops the now-unneeded arg_named_like_other_param entry (count 4) for that file; and test/cli/bun.test.ts adds a POSIX-only completions test that exercises all four fallback locations and asserts each resulting symlink points at the executable's realpath.
Security risks
None. This is a variable rename with no control-flow or data-flow change. The new test is hermetic — it hardlinks/copies the executable into a tempDir, sets PATH to an empty directory inside that temp dir, and overrides HOME/BUN_INSTALL to temp-dir subpaths, so no writes escape the temp directory and no network is touched.
Level of scrutiny
Low. The Rust diff is a mechanical identifier rename; I verified against src/sys/lib.rs:2602 that symlink's signature is (target, link) and that each call still passes exe as target and the newly-named link_path as link, so behavior is byte-identical. The baseline removal matches: the lint keys on the argument's name colliding with another parameter's name, and link_path collides with nothing. The test is the only non-mechanical addition and I walked through each of its four steps against the Rust fallback chain — the first run creates bin/bunx, subsequent runs hit EEXIST there and fall through to the next configured directory.
Other factors
My prior review flagged that installBunx piped stdout without draining it; commit 1c6d66b added proc.stdout.text() to the Promise.all and asserts stdout is empty, matching the file's other Bun.spawn calls. The test intentionally passes on both old and new code (the PR description states this) because there is no behavior change — it exists to pin the argument order against future regressions, so REVIEW.md's "must fail on system bun" rule does not apply here. The test is skipped on Windows with a comment explaining why (hardlink/.cmd shim instead of symlink), uses tempDir/bunEnv/await using, and clears SHELL so the child exits right after the symlink step without touching shell rc files.
There was a problem hiding this comment.
LGTM — the earlier stdout-drain nit is addressed in 1c6d66b.
What was reviewed:
- The Rust change is a pure local rename (
target/target_buf→link_path/link_buf);bun_sys::symlink(target, link)at src/sys/lib.rs:2602 confirms the second arg is the link path, so the new name is accurate and argument order is unchanged. - The baseline entry removal matches: all four
arg_named_like_other_paramsites in this file were renamed, leaving nothing to baseline. - The new test is hermetic (temp dir, controlled PATH/HOME/BUN_INSTALL/SHELL, private hardlinked exe), drains both pipes, and
readlinkSync(...) === exeRealpathpins the symlink direction at each of the four fallback locations.
Extended reasoning...
Overview
This PR touches three files to clear a mordant arg_named_like_other_param lint finding:
src/runtime/cli/install_completions_command.rs: renames the localtarget→link_pathand its buffertarget_buf→link_bufat all fourbun_sys::symlinkcall sites ininstall_bunx_symlink_posix. No control flow, argument order, or values change — verified by inspecting each hunk and confirmingbun_sys::symlink(target: &ZStr, link: &ZStr)atsrc/sys/lib.rs:2602takes the link path second.mordant-baseline.toml: deletes the now-zeroarg_named_like_other_param:.../install_completions_command.rs = 4entry.test/cli/bun.test.ts: adds a POSIX-onlycompletionstest that exercises all four symlink fallback locations and asserts the link target viareadlinkSync.
Security risks
None. This is a variable rename in a CLI utility path plus a test; no auth, crypto, parsing of untrusted input, or permission-sensitive code is touched.
Level of scrutiny
Low for the Rust and baseline changes — a mechanical identifier rename with no behavior change, of the same shape as several recent merged PRs (#39118, #39129, #39130, #39137). Medium for the test, which is new code; I traced it against install_bunx_symlink_posix and exec() to confirm the setup (empty PATH → which misses, unset SHELL → exit(1) after the symlink step) actually reaches the code under test, and that each subsequent run falls through because the prior link now EEXISTs.
Other factors
- My previous review flagged that
installBunxpiped stdout without draining it; the author fixed this in 1c6d66b (nowPromise.all([stdout, stderr, exited])withexpect(stdout).toBe("")), and the thread is resolved. The current diff reflects the fix. - The test follows harness conventions:
tempDir,bunEnvspread,describe.skipIf(isWindows)with a reason comment,await usingfor the subprocess, exit-code asserted last. - The one CI failure so far (
setInterval.test.json x64-asan) is unrelated to this change. - No outstanding human reviewer comments.
2 participants
Problem
arg_named_like_other_paramreports the fourbun_sys::symlinkcalls insrc/runtime/cli/install_completions_command.rs(lines 52, 62, 76, 94): a local namedtargetis passed assymlink'slinkparameter, andsymlinkalso has atargetparameter of the same type, so it reads like a transposed call.bun_sys::symlink(target, link)(src/sys/lib.rs:2602) is libcsymlink(target, linkpath); the calls passsymlink(exe, <dir>/bunx), which creates<dir>/bunxpointing at the bun executable. Only the local's name is wrong: it holds the path of the link being created, not what the link points at.Fix
link_pathand its buffer tolink_bufat all four sites. No behavior change.arg_named_like_other_paramentry (count 4) frommordant-baseline.toml. The lint keys on the argument's name matching another parameter's name;link_pathnames no parameter ofsymlink, so the file has nothing left to baseline. Thebun_runtimesection keeps its other entries, so this is the same result regenerating the baseline would give.test/cli/bun.test.tsgains acompletionstest covering what these four calls do: it runs a private hardlink (or copy) of the executable so the first candidate directory is inside the temp dir, and checks thatbun completionscreates a symlink to the executable next to it, then, as each earlier location is taken or missing, in$BUN_INSTALL/bin,$HOME/.bun/binand$HOME/.local/bin. This pins the argument order; since the code was already correct it passes before and after the rename (checked with the 1.4.0 release binary and with a debug build of this branch; about 1s under the debug build).Background
bun completionsalso installs abunxsymlink next tobun(falling back to the other directories above) sobunxworks after a manual install;install_bunx_symlink_posixis that step. Debug builds name the linkbunx-debug, which is why the test reads the name fromisDebug.bun run rust:mordantin the Rust lints workflow.mordant-baseline.tomlis a ratchet: per (lint, file) counts of pre-existing findings, so a PR fails the job only when it adds one. Fixing findings means deleting their 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/cli/bun.test.ts