fix(completions): complete files after a script arg in bash & zsh - #30387
Closed
robobun wants to merge 2 commits into
Closed
fix(completions): complete files after a script arg in bash & zsh#30387robobun wants to merge 2 commits into
robobun wants to merge 2 commits into
Claude / Claude Code Review
completed
May 8, 2026 in 21m 34s
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | test/cli/completions.test.ts:108-112 |
zsh test uses fixed sleep 0.5 instead of wait-for-condition |
| 🟡 Nit | test/cli/completions.test.ts:131 |
Process-spawning tests not marked .concurrent |
| 🟡 Nit | test/cli/completions.test.ts:80-90 |
zpty -r misuses 0.05/0.2 as timeout — it is actually the pattern argument |
Annotations
Check failure on line 112 in test/cli/completions.test.ts
claude / Claude Code Review
zsh test uses fixed sleep 0.5 instead of wait-for-condition
The post-TAB read uses a fixed `sleep 0.5` followed by a blind drain, which violates test/CLAUDE.md's explicit "never wait for time to pass in tests — always wait for the condition" rule and will flake on loaded CI runners. You already have `wait_for()` in this function — pass the expected substring (e.g. `foo-`) into `zshCompleteLine` and reuse that poll-until-seen pattern (with a generous upper-bound timeout) instead of `sleep` + drain.
Check warning on line 131 in test/cli/completions.test.ts
claude / Claude Code Review
Process-spawning tests not marked .concurrent
nit: per `test/CLAUDE.md` ("Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent"), these describe blocks should be `describe.concurrent` — every test here spawns its own bash/zsh subprocess in its own `tempDir`, so there's no shared state. The zsh tests in particular each run `compinit` + `sleep 0.5`, so running them sequentially roughly doubles wall time for no reason. If you do make the zsh ones concurrent, sw
Check warning on line 90 in test/cli/completions.test.ts
claude / Claude Code Review
zpty -r misuses 0.05/0.2 as timeout — it is actually the pattern argument
The trailing `0.05` / `0.2` arguments to `zpty -r -t S chunk …` aren't timeouts — `zpty -r`'s syntax is `name [param [pattern]]`, so these occupy the *pattern* slot. The reads happen to work because `-t` makes them non-blocking and the pattern never matches, but the intended per-read delay doesn't exist (so `wait_for()` busy-spins a core when no data is pending). Drop the bogus arg and add an explicit `else sleep 0.05` if you want a poll interval; same for the two drain loops at lines ~101 and ~
Loading