fix(completion): make PowerShell tab completion work again#1374
Conversation
…erShell script An empty switch body is a parse error in PowerShell, and the generator emitted one for every command whose positionals are all path-typed (18 in the current registry). PowerShell parses the entire file before execution, so the whole completion script failed to load. Skip the positional-index block when no positional produces completions. Fixes #1293 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PowerShell generator now omits empty positional-index switches. Tests cover commands with no positional completions and the actual command registry, while a changeset documents the fix. ChangesPowerShell completion generation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alfred-openspec
left a comment
There was a problem hiding this comment.
Looks good. The fix is correctly scoped, preserves mixed positional indexing, and includes focused plus registry-wide regression coverage.
Status: LGTM — reproduced, fixed, and verified with a real PowerShell 7.5.4 parser (18 parse errors → 0) and live tab completion.
Fixes #1293
What was wrong
openspec completion installgenerated a PowerShell script that failed to parse, so completions never worked for any PowerShell user. The generator emittedswitch ($positionalIndex) { }with zero clauses for every command whose positional arguments are allpath-typed (PowerShell completes paths natively, so those cases produce no clauses) — 18 such blocks in the current registry. An empty switch body is a hard syntax error in PowerShell (Missing condition in switch statement clause), and PowerShell parses the whole file before running it, so one bad block aborted the entire script.How it was fixed
In
generateIndexedPositionalCompletion, the case clauses are now built first; when no positional produces any completion, the whole positional-index block (the token-counting loop plus the switch) is skipped — it existed only to feed the switch, so this is behavior-equivalent and leaves no dead code. Commands with real positional completions are unchanged.Replication / proof
Generate the script from the real command registry and parse it with PowerShell's own parser:
Dot-sourcing the fixed script in pwsh 7.5.4 and invoking
TabExpansion2returns real completions:Two regression tests added: a unit test for a path-only positional command, and a registry-wide test asserting the generated script never contains an empty
switch ($positionalIndex)block. Full suite: 1,860 passed; the only failures are the 17 known environment-onlyzsh-installerfailures on this machine (local oh-my-zsh; CI unaffected).Notes
switch ($command)would also be empty, but the registry is static and never empty, so it's unreachable in practice.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
switchblocks.Tests