completions: fix 4 correctness bugs in zsh/bash/fish scripts - #36515
Closed
robobun wants to merge 7 commits into
Closed
completions: fix 4 correctness bugs in zsh/bash/fish scripts#36515robobun wants to merge 7 commits into
robobun wants to merge 7 commits into
Claude / Claude Code Review
completed
Jul 31, 2026 in 25m 14s
Code review found 2 potential issues
Found 3 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | completions/bun.bash:58 |
bash: removed OR arm was always-true, not dead — behavior change, not dead-code removal |
| 🟡 Nit | test/cli/shell-completion-scripts.test.ts:22 |
Tests spawn independent subprocesses serially — use test.concurrent |
Annotations
Check warning on line 58 in completions/bun.bash
claude / Claude Code Review
bash: removed OR arm was always-true, not dead — behavior change, not dead-code removal
Heads-up: the removed OR arm was not dead — in bash, `[[ str =~ ${undeclared} ]]` is **true** (the empty regex matches at position 0), so this block previously ran unconditionally and now runs only when `${re_prev_script}` matches *and* `COMP_WORDS[2]` is non-empty. That is a behavior change, not a no-op cleanup: e.g. `bun somefile.js <TAB>` now leaves `replaced_script` unset and falls through to `unset COMPREPLY` (zero completions) where it used to offer the global flags. Probably fine to keep
Check warning on line 22 in test/cli/shell-completion-scripts.test.ts
claude / Claude Code Review
Tests spawn independent subprocesses serially — use test.concurrent
nit: All 5 tests spawn independent subprocesses (`bun completions` / `bash -n`) with no shared state, so per test/CLAUDE.md ("make them concurrent with `test.concurrent` or `describe.concurrent`") and REVIEW.md ("`test.concurrent` for independent subprocess suites") these should run concurrently — e.g. swap each `test(` for `test.concurrent(`. Very low impact since `bun completions` just dumps `include_bytes!` to stdout, but it's a one-token change to match the harness convention.
Loading