completions: fix 4 correctness bugs in zsh/bash/fish scripts - #36515
completions: fix 4 correctness bugs in zsh/bash/fish scripts#36515robobun wants to merge 7 commits into
Conversation
- zsh: move closing bracket inside the quote for the -i optspec in _bun_run_completion so _arguments sees a single argument (#31665) - zsh: print the history array instead of executing its first element in _bun_add_param_package_completion (#34062) - bash: drop dead OR arm that referenced undeclared re_comp_word_script in _read_scripts_in_package_json (#28744) - fish: add frozen-lockfile to bun_install_boolean_flags and fix the dry-run description so flags and descriptions stay in lockstep (#29364) Adds test/cli/shell-completion-scripts.test.ts which asserts on the embedded scripts that `bun completions` emits to stdout. Co-authored-by: Agusti F. <6601142+agustif@users.noreply.github.com> Co-authored-by: Yahia EL IDRISSI <yahiaelidev@gmail.com> Co-authored-by: Jakub Pečenka <jakubpecenka99@gmail.com> Co-authored-by: Lucas Garron <code@garron.net>
|
Warning Review limit reached
Next review available in: 18 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 (4)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
…the guard The removed OR arm matched against an undeclared variable, which under GNU regex is always-true (empty pattern matches), so the block has always run unconditionally. Dropping only the OR arm turned the guard genuinely conditional and regressed `bun <entrypoint> <TAB>` on GNU bash from 42 completions to 0. Running the block unconditionally instead preserves the shipped GNU behaviour and still removes the BSD-regex error on macOS. Also: switch tests to test.concurrent and add a functional probe that drives _bun_completions for `bun somefile.js <TAB>`.
|
Updated 3:08 AM PT - Jul 31st, 2026
✅ @robobun, your commit 66b7669bc181937b897417f720272b96ec761e57 passed in 🧪 To try this PR locally: bunx bun-pr 36515That installs a local version of the PR into your bun-36515 --bun |
There was a problem hiding this comment.
LGTM — the four completion-script fixes are byte-level and well-scoped, and every concern from the earlier review rounds (bash guard semantics, test hermeticity, bash 3.2 gating, test.concurrent) has landed. The one remaining inline nit is non-blocking.
Extended reasoning...
Overview
Four independent one-line-class fixes to the embedded shell completion scripts under completions/, plus a new test file at test/cli/shell-completion-scripts.test.ts:
bun.zsh:486— swap']→]'so the-ioptspec is well-formed for_arguments.bun.zsh:994— replace$($inexact | grep …)with$(print -l -- $inexact | grep …)so history-scraped package names are printed rather than executed.bun.bash:56-70— remove the[[ … =~ ${re_comp_word_script} ]]guard (undeclared variable → always-true on GNU, error on BSD) and make the script-filter block unconditional, preserving the shipped GNU behaviour byte-for-byte and eliminating the macOS "empty (sub)expression" error.bun.fish:35-36— insertfrozen-lockfileand its description at matching positions in the two parallel arrays; correct thedry-rundescription.
No src/ code is touched; the scripts reach the binary via include_bytes! in shell_completions.rs.
Security risks
The zsh $($inexact | …) line was itself a latent arbitrary-command-execution hazard (first history token run as a program on bun add <TAB>); the fix is the canonical zsh idiom and strictly reduces attack surface. Nothing else in the diff touches auth, crypto, permissions, or untrusted input handling.
Level of scrutiny
Low-to-moderate. Completion scripts are UX affordances with no runtime-correctness impact; worst-case regression is degraded tab completion, which is the status quo the PR is fixing. The one non-mechanical change — the bash guard — has already been through two review rounds here: my first-pass concern (unconditional→conditional behaviour change) was verified by the author with a COMPREPLY probe, the fix was reworked to keep the block unconditional, and a functional regression test (gated on bash≥4) now drives _bun_completions for the bun somefile.js <TAB> case and asserts the global flag list is still offered.
Other factors
All four prior review threads (behaviour-preservation of the bash guard, test.concurrent, HOME/BUN_INSTALL hermeticity, macOS bash 3.2 gate) are resolved and visible in commits e4b3ff2, 26844d1, and 205ef24. The new test file asserts on the emitted script bytes for each shell, verifies bash -n cleanliness, drives the bash completion function directly, and checks the fish flag/description arrays stay length-aligned with positional spot-checks. The sole open finding this run — bash -n test not skipIf-gated on bash presence — is a nit: every current POSIX CI lane has bash, and there is repo precedent for ungated bash spawns. Not a merge blocker.
There was a problem hiding this comment.
LGTM — all four completion-script fixes are minimal and behaviour-preserving, and every earlier review point has been addressed.
What was reviewed:
- bash: confirmed the now-unconditional filter block matches shipped GNU behaviour byte-for-byte (traced
bun <TAB>,bun run <TAB>,bun somefile.js <TAB>through_read_scripts_in_package_json→*)consumer); BSD-regex error path is gone. - fish: verified the two parallel arrays stay index-aligned (12/12) with
frozen-lockfileand the updateddry-rundescription at the right positions. - zsh:
print -l -- $inexactreplaces the array-executed-as-command — strictly a hazard removal;-ioptspec bracket fix is a pure quoting correction. - tests: hermetic (
HOME/BUN_INSTALLcleared),bashMajorgates both bash-spawning tests, all concurrent.
Extended reasoning...
Overview
Rollup of four independent, externally-contributed correctness fixes to the embedded shell completion scripts (completions/bun.{zsh,bash,fish}), plus a new test file that spawns bun completions with piped stdout to assert on the emitted bytes. No src/ change — the scripts are include_bytes!'d, so the diff under completions/ is what ships.
Security risks
None introduced. The zsh _bun_add_param_package_completion fix actually removes a latent hazard: $($inexact | grep ...) previously executed the first history-scraped package name as a command. The other three changes are quoting/list corrections with no security surface.
Level of scrutiny
Low. Completion scripts are developer-UX only, run in the user's own interactive shell, and cannot affect runtime correctness. The one change with any behavioural subtlety (bash guard removal) went through an earlier review round: the initial approach (dropping the OR arm) was flagged as a GNU behaviour change, and the author corrected it in e4b3ff2 to make the block unconditional — I re-traced the *) consumer against replaced_script for the direct-invocation, bun run, and bare-bun cases and it matches what GNU-linked bash has been shipping. The run) case also calls _read_scripts_in_package_json, and its post-filter behaviour is likewise unchanged (scripts added then filtered — pre-existing, out of scope here).
Other factors
All four of my earlier review threads (behaviour-preserving bash fix, test.concurrent, HOME/BUN_INSTALL hermeticity, bashMajor gating for macOS bash 3.2 and bash-absent hosts) are resolved in the current head (66b7669). The new test file follows harness conventions: tempDir, await using, Promise.all pipe drain, stderr asserted before exit code, skipIf on unavailable system binaries, no network. No CODEOWNERS entry for completions/. The bug-hunting system found nothing this round.
|
CI on 66b7669 (build 86125): the new Locally: 4 of 6 tests fail on the released binary and all 6 pass on the debug build. The two that pass on both are the This is ready for review. |
|
Closing while triaging the open completion PRs. Each of the four changes here is already open as its own contributor PR, and those are the ones to merge:
The bash change here should not land in this form. Making the block unconditional does preserve what GNU bash did on main, but what it did was strip the scripts The last row is the case this PR set out to preserve; #36629 replaces that arm with flags plus file completion, so it is covered there instead. Comparison notes are in #26743 (comment). |
Rolls up four independent correctness fixes to the shell completion scripts, each originally submitted as its own PR by an external contributor.
Closes #31665. Closes #34062. Closes #28744. Closes #29364. Closes #26743.
Fixes #24847.
Related: #18407 (this fixes the command-execution half of
_bun_add_param_package_completion; thehistory -n bun/fc: event not founderror on the line above it is separate).Fixes
zsh:
-ioptspec has its closing]outside the quote (#31665, @agustif)completions/bun.zshline 486, inside_bun_run_completion:'-i[Automatically install ... equivalent to --install=fallback'] \The
]sits after the closing', so_argumentsreceives the optspec without a closing bracket plus a stray literal]as the next word. The fix moves the bracket inside the quote so the optspec is well-formed:'-i[Automatically install ... equivalent to --install=fallback]' \zsh:
bun addhistory completion executes history as a command (#34062, @yahiaelidev)completions/bun.zshline 994, inside_bun_add_param_package_completion:$inexactis an array of package names scraped from shell history.$($inexact | grep ...)expands the array as a command line, so the first history entry is executed as a program with the remaining entries as its arguments, and its stdout is piped to grep. The intent was to filter the array, so the fix prints it line-per-element instead:bash: guard in
_read_scripts_in_package_jsonmatches against an undeclared variable (#28744, @JakubPecenka)completions/bun.bashlines 58-61, inside_read_scripts_in_package_json:[[ ( "${COMPREPLY[*]}" =~ ${re_prev_script} && -n "${COMP_WORDS[2]}" ) || \ ( "${COMPREPLY[*]}" =~ ${re_comp_word_script} ) ]] && {re_comp_word_scriptis never declared anywhere in the file, so the second arm expands to"${COMPREPLY[*]}" =~with an empty pattern. Behaviour diverges by regex library:bun <tab>printsinvalid regular expression '': empty (sub)expression(bash_completion bug (macos bash) #24847; the workaround users have been applying inserts areturnright above this block).#28744 and #26743 both propose dropping the OR arm, which cures macOS but is a behaviour change on GNU: the guard becomes genuinely conditional, so
replaced_scriptis no longer set forbun <entrypoint> <TAB>and the downstream consumer at*)falls through tounset COMPREPLY(zero completions where there were 42 global flags). The fix here instead removes the guard entirely so the block runs unconditionally, matching the long-standing GNU behaviour byte-for-byte while removing the BSD error:A functional test drives
_bun_completionswithCOMP_WORDS=(bun somefile.js "")and asserts--help/--versionare still offered.#21778 takes the alternative approach of actually populating
re_comp_word_script(plus an unrelated[A-Za_z]typo fix) and is not superseded here.fish:
--frozen-lockfilemissing from install flag list (#29364, @lgarron)completions/bun.fishline 35 lists the boolean flags forbun install/add/remove/updatebut omitsfrozen-lockfileeven though zsh and bash both offer it. The fix inserts it and, to keep the two parallel arrays the same length, adds the matching description (taken from zsh: "Disallow changes to lockfile"). The original PR also corrected thedry-rundescription from "Don't install anything" to "Perform a dry run without making changes", which is kept here.Why this is the right fix
The two zsh changes and the fish change are minimal byte-level corrections to lines that were already written to do the thing the fix makes them do; no behaviour is added. The bash change is behaviour-preserving on GNU bash (probed by driving
_bun_completionsdirectly and diffingCOMPREPLYagainstmain) and removes the noisy error on macOS bash. The completion scripts areinclude_bytes!'d into the binary (src/runtime/cli/shell_completions.rs), so these ship with the next build without anysrc/change.Verification
New
test/cli/shell-completion-scripts.test.tsspawnsbun completionswith$SHELLset to each ofzsh/bash/fish(stdout piped, which triggers the "write embedded script to stdout" path ininstall_completions_command.rs) and asserts on the emitted bytes:bash -n completions/bun.bashis also clean.Note for the reviewer: the only diff is under
completions/andtest/; the embedded scripts are compiled in viainclude_bytes!, so the gate'ssrc/-stash step cannot reproduce the fail-before state (thecompletions/edits stay applied either side of the stash). The fail-before is demonstrated above against the released binary.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.