feature/bash_fish_power_shells_completions #401
Conversation
📝 WalkthroughWalkthroughExpanded CLI completions from Zsh-only to multi‑shell (zsh, bash, fish, powershell): added per‑shell generators and installers, centralized InstallationResult, updated CompletionFactory and CLI installer output/confirmation behavior, added shell templates, adjusted file-system writability logic, and added comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as OpenSpec CLI
participant Factory as CompletionFactory
participant Gen as CompletionGenerator
participant Installer as CompletionInstaller
participant FS as FileSystem
CLI->>Factory: createGenerator(selectedShell)
Factory->>Gen: instantiate shell-specific generator
CLI->>Gen: generate(commands)
Gen-->>CLI: completion script
CLI->>Factory: createInstaller(selectedShell)
Factory->>Installer: instantiate shell-specific installer
CLI->>Installer: install(completionScript)
Installer->>FS: resolve path / check existing / backup
FS-->>Installer: path & backup info
Installer->>FS: write script / update shell config/profile (markers)
FS-->>Installer: write result
Installer-->>CLI: InstallationResult(success, installedPath, backupPath, instructions, warnings)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/completion.ts (1)
189-193: Confirmation prompt hardcodes Zsh-specific path.The uninstall confirmation message references
~/.zshrcregardless of which shell is being used. For Bash, Fish, or PowerShell, this message would be misleading to users.🔎 Suggested fix
if (!skipConfirmation) { + const configFiles: Record<string, string> = { + zsh: '~/.zshrc', + bash: '~/.bashrc', + fish: '~/.config/fish/config.fish', + powershell: '$PROFILE', + }; const confirmed = await confirm({ - message: 'Remove OpenSpec configuration from ~/.zshrc?', + message: `Remove OpenSpec ${shell} completion configuration${configFiles[shell] ? ` from ${configFiles[shell]}` : ''}?`, default: false, });
🧹 Nitpick comments (6)
src/core/completions/generators/bash-generator.ts (1)
132-155: Consider using the union type fromCommandDefinitionfor type safety.The
positionalTypeparameter is typed asstring | undefined, butCommandDefinitiondefines it as a specific union type. Using the exact type would provide compile-time exhaustiveness checking.🔎 Proposed fix
- private generatePositionalCompletion(positionalType: string | undefined, indent: string): string[] { + private generatePositionalCompletion(positionalType: CommandDefinition['positionalType'], indent: string): string[] {test/core/completions/installers/fish-installer.test.ts (1)
220-227: Consider whether empty completion scripts should be allowed.The test verifies that an empty string can be installed as a completion script. While this works, it may indicate a bug in the caller. Consider whether the installer should validate that the script content is non-empty, or if this is intentional for testing purposes.
test/core/completions/installers/powershell-installer.test.ts (1)
1-1: Unused import:vifrom vitest.The
viimport is not used anywhere in this test file. All tests use real filesystem operations with temp directories rather than mocking.🔎 Proposed fix
-import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest';src/core/completions/installers/bash-installer.ts (3)
31-50: Consider checking user-local bash-completion directory.The check only looks at system-level paths. Users who installed bash-completion via Homebrew or in user-local directories might have it at
~/.local/share/bash-completion. However, since this is only used to display a warning (not to block installation), the current implementation is acceptable.
189-192: Misleading comment: code removes leading lines, not trailing.The comment says "Remove trailing empty lines" but
lines.shift()removes from the front (leading lines). This appears intentional since marker blocks are typically at the beginning of the file, but the comment is incorrect.🔎 Proposed fix
- // Remove trailing empty lines + // Remove leading empty lines after marker block removal while (lines.length > 0 && lines[0].trim() === '') { lines.shift(); }
258-269: Warning message is macOS-specific.The bash-completion installation instructions mention
brew installand/opt/homebrewpaths which are macOS-specific. Linux users would typically useapt install bash-completionor similar package managers. Consider providing platform-specific instructions or more generic guidance.🔎 Proposed fix
if (!hasBashCompletion) { warnings.push( '⚠️ Warning: bash-completion package not detected', '', 'The completion script requires bash-completion to function.', - 'Install it with:', - ' brew install bash-completion@2', + 'Install it with your package manager:', + ' macOS: brew install bash-completion@2', + ' Debian/Ubuntu: sudo apt install bash-completion', + ' Fedora: sudo dnf install bash-completion', '', - 'Then add to your ~/.bash_profile:', - ' [[ -r "/opt/homebrew/etc/profile.d/bash_completion.sh" ]] && . "/opt/homebrew/etc/profile.d/bash_completion.sh"' + 'Then follow your distribution\'s instructions to enable it in your shell.' ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
openspec/changes/extend-shell-completions/proposal.mdopenspec/changes/extend-shell-completions/specs/cli-completion/spec.mdopenspec/changes/extend-shell-completions/tasks.mdsrc/commands/completion.tssrc/core/completions/factory.tssrc/core/completions/generators/bash-generator.tssrc/core/completions/generators/fish-generator.tssrc/core/completions/generators/powershell-generator.tssrc/core/completions/generators/zsh-generator.tssrc/core/completions/installers/bash-installer.tssrc/core/completions/installers/fish-installer.tssrc/core/completions/installers/powershell-installer.tssrc/core/completions/installers/zsh-installer.tstest/commands/completion.test.tstest/core/completions/generators/bash-generator.test.tstest/core/completions/generators/fish-generator.test.tstest/core/completions/generators/powershell-generator.test.tstest/core/completions/installers/bash-installer.test.tstest/core/completions/installers/fish-installer.test.tstest/core/completions/installers/powershell-installer.test.ts
💤 Files with no reviewable changes (1)
- src/core/completions/generators/zsh-generator.ts
🧰 Additional context used
📓 Path-based instructions (4)
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/extend-shell-completions/specs/cli-completion/spec.mdopenspec/changes/extend-shell-completions/proposal.mdopenspec/changes/extend-shell-completions/tasks.md
openspec/changes/**/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/spec.md: Use## ADDED|MODIFIED|REMOVED|RENAMED Requirementsheaders in spec delta files
Include at least one#### Scenario:per requirement in spec delta files
Use#### Scenario: Nameformat (4 hashtags) for scenario headers, not bullets or bold text
Use## ADDED Requirementsfor new orthogonal capabilities that can stand alone; use## MODIFIED Requirementsfor behavior changes of existing requirements
When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Files:
openspec/changes/extend-shell-completions/specs/cli-completion/spec.md
openspec/changes/*/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Ensure
proposal.mdincludes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Files:
openspec/changes/extend-shell-completions/proposal.md
openspec/changes/*/tasks.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Ensure
tasks.mdcontains implementation checklist with numbered sections and checkbox items
Files:
openspec/changes/extend-shell-completions/tasks.md
🧠 Learnings (5)
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` headers in spec delta files
Applied to files:
openspec/changes/extend-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
openspec/changes/extend-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Include at least one `#### Scenario:` per requirement in spec delta files
Applied to files:
openspec/changes/extend-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/proposal.md : Ensure `proposal.md` includes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Applied to files:
openspec/changes/extend-shell-completions/proposal.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/tasks.md : Ensure `tasks.md` contains implementation checklist with numbered sections and checkbox items
Applied to files:
openspec/changes/extend-shell-completions/tasks.md
🧬 Code graph analysis (11)
test/core/completions/generators/bash-generator.test.ts (2)
src/core/completions/generators/bash-generator.ts (1)
BashGenerator(7-192)src/core/completions/types.ts (1)
CommandDefinition(36-72)
test/core/completions/installers/bash-installer.test.ts (2)
scripts/postinstall.js (1)
installer(84-84)src/core/completions/installers/bash-installer.ts (1)
BashInstaller(11-364)
src/core/completions/generators/powershell-generator.ts (2)
src/core/completions/types.ts (2)
CompletionGenerator(77-90)CommandDefinition(36-72)scripts/postinstall.js (1)
script(81-81)
test/core/completions/installers/powershell-installer.test.ts (2)
scripts/postinstall.js (1)
installer(84-84)src/core/completions/installers/powershell-installer.ts (1)
PowerShellInstaller(11-326)
src/core/completions/generators/bash-generator.ts (2)
src/core/completions/types.ts (1)
CommandDefinition(36-72)scripts/postinstall.js (1)
script(81-81)
test/core/completions/generators/powershell-generator.test.ts (3)
scripts/postinstall.js (2)
generator(80-80)script(81-81)src/core/completions/generators/powershell-generator.ts (1)
PowerShellGenerator(7-224)src/core/completions/types.ts (1)
CommandDefinition(36-72)
src/core/completions/factory.ts (1)
src/utils/shell-detection.ts (1)
SupportedShell(4-4)
test/core/completions/installers/fish-installer.test.ts (3)
scripts/postinstall.js (1)
installer(84-84)src/core/completions/installers/fish-installer.ts (1)
FishInstaller(10-152)src/utils/file-system.ts (1)
fileExists(76-86)
src/core/completions/generators/fish-generator.ts (1)
src/core/completions/types.ts (3)
CompletionGenerator(77-90)CommandDefinition(36-72)FlagDefinition(6-31)
src/core/completions/installers/bash-installer.ts (2)
src/utils/file-system.ts (1)
FileSystemUtils(44-187)src/core/completions/factory.ts (1)
InstallationResult(15-27)
src/core/completions/installers/fish-installer.ts (1)
src/core/completions/factory.ts (1)
InstallationResult(15-27)
🪛 GitHub Actions: CI
test/core/completions/installers/powershell-installer.test.ts
[error] 153-153: configureProfile: should create profile with markers when file does not exist (expected true, received false)
[error] 243-243: ENOENT: no such file or directory, open '/tmp/openspec-powershell-test-6b713d6e-a264-40d9-b000-4b0111399956/.config/powershell/Microsoft.PowerShell_profile.ps1'
🪛 GitHub Check: Test
test/core/completions/installers/powershell-installer.test.ts
[failure] 243-243: test/core/completions/installers/powershell-installer.test.ts > PowerShellInstaller > configureProfile > should generate correct PowerShell syntax in config
Error: ENOENT: no such file or directory, open '/tmp/openspec-powershell-test-6b713d6e-a264-40d9-b000-4b0111399956/.config/powershell/Microsoft.PowerShell_profile.ps1'
❯ test/core/completions/installers/powershell-installer.test.ts:243:23
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -2, code: 'ENOENT', syscall: 'open', path: '/tmp/openspec-powershell-test-6b713d6e-a264-40d9-b000-4b0111399956/.config/powershell/Microsoft.PowerShell_profile.ps1' }
[failure] 153-153: test/core/completions/installers/powershell-installer.test.ts > PowerShellInstaller > configureProfile > should create profile with markers when file does not exist
AssertionError: expected false to be true // Object.is equality
- Expected
- Received
- true
- false
❯ test/core/completions/installers/powershell-installer.test.ts:153:22
🔇 Additional comments (30)
src/core/completions/generators/fish-generator.ts (1)
1-230: LGTM!The FishGenerator implementation correctly follows Fish shell completion conventions:
- Proper use of
complete -cwith conditions- Helper functions for subcommand detection are well-structured
- Dynamic completion helpers integrate with the CLI's
__completeinterface- Correct escaping of single quotes for Fish syntax
src/core/completions/generators/powershell-generator.ts (1)
1-224: LGTM!The PowerShellGenerator implementation correctly follows PowerShell completion patterns:
- Proper use of
Register-ArgumentCompleterwith scriptblock- Token parsing and command/subcommand detection logic is sound
- Dynamic completion helpers use appropriate PowerShell constructs
- Double-quote escaping is correct for PowerShell string literals
src/commands/completion.ts (1)
153-159: LGTM!The warning display logic correctly handles the new
warningsfield inInstallationResult. The null check and length check prevent unnecessary output, and the blank line provides visual separation.test/commands/completion.test.ts (4)
80-87: LGTM!Test correctly updated to use
tcshas an example of an unsupported shell, and the expected error message properly lists all four now-supported shells.
137-144: LGTM!Install subcommand test correctly updated to match the new supported shell list.
186-193: LGTM!Uninstall subcommand test correctly updated to match the new supported shell list.
247-257: LGTM!Shell detection integration test correctly updated to simulate detection of an unsupported shell (
tcsh) and verify the appropriate error message.test/core/completions/generators/fish-generator.test.ts (1)
1-429: LGTM!Comprehensive test suite covering:
- Interface compliance verification
- Script header and helper function generation
- Flag handling (short/long, boolean vs value-taking, enum values)
- Subcommand structures
- All positional argument types
- Dynamic completion helpers
- Edge cases (empty commands, escaping)
test/core/completions/generators/bash-generator.test.ts (1)
1-419: LGTM!Comprehensive test suite with good coverage of:
- Interface compliance
- Bash-specific constructs (
_init_completion,compgen,COMPREPLY)- All flag and positional argument variations
- Dynamic completion helpers
- Script registration verification
- Edge cases
src/core/completions/installers/zsh-installer.ts (1)
5-5: No circular dependency issue. The change centralizes theInstallationResulttype infactory.ts, which is the source of truth for this interface. All installers (bash, fish, powershell, zsh) correctly import this shared type from the factory. SinceInstallationResultis only a type definition, not a runtime class, TypeScript strips these imports during compilation. This pattern is safe and follows TypeScript best practices.src/core/completions/generators/bash-generator.ts (1)
166-188: LGTM!The dynamic completion helpers are well-structured. They correctly invoke the
openspec __completecommand, redirect stderr to suppress errors, and usecut -f1to extract the first field (ID) from tab-separated output.src/core/completions/factory.ts (2)
12-27: LGTM!The centralized
InstallationResultinterface with shell-specific optional fields (isOhMyZsh,zshrcConfigured,bashrcConfigured,profileConfigured) is a clean approach that allows each installer to use only the relevant fields while maintaining a unified interface.
51-64: LGTM!The factory methods correctly handle all four supported shells with proper switch-case exhaustiveness. The default case throwing an error provides a safety net for any future type mismatches.
Also applies to: 73-86
test/core/completions/installers/fish-installer.test.ts (2)
12-20: LGTM!Good test isolation pattern using
randomUUID()for unique temp directories and proper cleanup inafterEach.
195-208: LGTM!Good testing pattern for permission errors: restoring permissions before assertions ensures proper cleanup even if assertions fail.
src/core/completions/installers/powershell-installer.ts (2)
32-46: LGTM!Good cross-platform profile path resolution with
$PROFILEenvironment variable check and sensible fallbacks for Windows and Unix-like systems.
85-92: LGTM!The
Test-Pathcheck before sourcing the script is a safe pattern that prevents errors if the script is removed while the profile config remains.test/core/completions/generators/powershell-generator.test.ts (2)
1-10: LGTM!Comprehensive test suite with good coverage of interface compliance, script generation, flag handling, subcommands, positional arguments, dynamic helpers, and edge cases.
347-365: LGTM!Good test for PowerShell quote escaping - verifying that double quotes are escaped by doubling (
"") which is the correct PowerShell convention.openspec/changes/extend-shell-completions/tasks.md (1)
1-49: LGTM!The
tasks.mdfollows the expected format with numbered sections (phases) and checkbox items. The implementation checklist is comprehensive, covering foundation work, shell-specific implementations, documentation, testing, and validation. All tasks are marked complete, aligning with the code changes in this PR.Based on learnings, this file correctly implements the expected format.
src/core/completions/installers/fish-installer.ts (2)
1-15: LGTM!Clean implementation with minimal dependencies. The constructor pattern with optional
homeDirparameter enables testability.
52-115: LGTM!The install flow correctly handles:
- Idempotence (same content detection)
- Directory creation with
recursive: true- Backup on updates
- Appropriate messaging for different scenarios
Fish's auto-loading behavior means no profile configuration is needed, keeping this installer simpler than Bash/PowerShell.
test/core/completions/installers/bash-installer.test.ts (3)
12-22: LGTM!Good test isolation pattern with unique temp directories and proper cleanup.
109-139: LGTM!Proper environment variable handling - saves the original value, sets the test value, and restores it in all code paths (including implicit exception paths via the finally-like pattern).
260-326: LGTM!Thorough testing of marker-based
.bashrcconfiguration including:
- Creating new file with markers
- Prepending to existing file without markers
- Updating existing markers with new config
- Preserving user content outside markers
openspec/changes/extend-shell-completions/specs/cli-completion/spec.md (1)
1-328: Spec delta file follows coding guidelines correctly.The file properly uses:
## MODIFIED Requirementsheader format### Requirement:for requirement blocks#### Scenario:format (4 hashtags) for all scenarios- Each requirement contains at least one scenario with WHEN/THEN/AND structure
The multi-shell specification is comprehensive and well-structured.
test/core/completions/installers/powershell-installer.test.ts (1)
375-554: Comprehensive install tests with good coverage.The install test suite covers:
- First-time installation
- Directory creation
- Content verification
- Idempotent behavior (already installed with same content)
- Update scenarios with backup
- Profile configuration (auto and manual)
- Paths with spaces
- Permission errors
- Edge cases (empty script, special characters)
Good test design using real filesystem operations with temporary directories.
src/core/completions/installers/bash-installer.ts (3)
1-24: Clean class structure following established patterns.The
BashInstallerclass follows the same structure as other installers (PowerShell, Zsh) with:
- Dependency injection for
homeDir(testability)- Consistent marker format for config management
- Proper TypeScript typing with
InstallationResultimport from factory
57-63: Method works correctly for user-level installation.Always using
~/.local/share/bash-completion/completions/openspecis the right choice for user-level installation without requiring root permissions. The comment on line 58 ("Try user-local bash-completion directory first") is slightly misleading since no alternatives are tried, but the implementation is correct.
333-363: Uninstall method correctly delegates confirmation to command layer.The
options.yesparameter is accepted but not used internally, as the JSDoc indicates confirmation is handled by the command layer. This matches the interface contract and allows for consistent behavior across installers.
| script.push('_openspec_completion() {'); | ||
| script.push(' local cur prev words cword'); | ||
| script.push(' _init_completion || return'); | ||
| script.push(''); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the bash-generator.ts file to understand the complete context
fd -t f bash-generator.ts -x wc -l {}Repository: Fission-AI/OpenSpec
Length of output: 118
🏁 Script executed:
# Search for _init_completion and bash-completion references
rg -n "_init_completion|bash-completion" --type=ts -A 2 -B 2Repository: Fission-AI/OpenSpec
Length of output: 7773
🏁 Script executed:
# Check if there are other shell generators for comparison
fd -t f -path "*/generators/*" ".*-generator\.ts$"Repository: Fission-AI/OpenSpec
Length of output: 231
🏁 Script executed:
# Look for any documentation about dependencies or setup
fd -t f "README|INSTALL|SETUP|package\.json" -x grep -l "bash-completion" {} \;Repository: Fission-AI/OpenSpec
Length of output: 45
🏁 Script executed:
# Let's look at the full context of bash-generator.ts around lines 25-28
sed -n '1,50p' src/core/completions/generators/bash-generator.tsRepository: Fission-AI/OpenSpec
Length of output: 1791
🏁 Script executed:
# Check the bash-installer.ts warnings about bash-completion
sed -n '210,275p' src/core/completions/installers/bash-installer.tsRepository: Fission-AI/OpenSpec
Length of output: 2577
🏁 Script executed:
# Look at the beginning of bash-generator.ts to see if there's any documentation
head -n 30 src/core/completions/generators/bash-generator.tsRepository: Fission-AI/OpenSpec
Length of output: 1115
Document bash-completion requirement in generated script header.
The _init_completion function depends on the bash-completion package. While the BashInstaller already checks for this dependency and warns users during installation if it's missing, the generated script header could document this requirement to make the dependency explicit in the script itself.
Consider adding a comment in the generated script header (e.g., # Requires: bash-completion package) for clarity.
🤖 Prompt for AI Agents
In src/core/completions/generators/bash-generator.ts around lines 25 to 28, the
generated completion script calls _init_completion but does not document that
this requires the bash-completion package; add a one-line comment at the top of
the generated script header such as "# Requires: bash-completion package" (or
similar) immediately before or after the function declaration to make the
dependency explicit in the output script.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/specs/cli-completion/spec.md (1)
345-346: Outdated error message contradicts the updated spec.Line 345's scenario header states users may request bash, fish, or powershell completions, but the error message at line 346 claims "Currently supported: zsh" only. This conflicts with the expanded multi-shell support throughout this spec.
Update the error message to reflect all four supported shells:
"Shell '<name>' is not supported. Supported shells: zsh, bash, fish, powershell"Alternatively, verify whether this scenario describes a genuinely unsupported shell (e.g., ksh) versus shells now explicitly supported by this PR.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openspec/specs/cli-completion/spec.md
🧰 Additional context used
📓 Path-based instructions (1)
openspec/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Files:
openspec/specs/cli-completion/spec.md
🔇 Additional comments (1)
openspec/specs/cli-completion/spec.md (1)
8-8: Multi-shell support spec is comprehensive and well-structured.The expansion from Zsh-only to bash, fish, and powershell is consistently applied across detection, generation, installation, uninstallation, and architecture patterns. Normative requirements use "SHALL" correctly per spec guidelines. Scenarios follow WHEN/THEN format clearly, with explicit shell-specific behaviors (e.g., single vs. double TAB triggers, completion syntaxes). Factory pattern and interface definitions are precise for implementers.
Also applies to: 18-47, 70-94, 98-135, 170-221, 232-271, 280-338
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/commands/completion.ts (1)
152-158: Consider adding visual distinction for warning messages.Warnings are currently printed as plain text without any prefix or icon. For better user experience, consider prefixing warnings with a visual indicator (e.g.,
⚠orWarning:) to distinguish them from regular instructions, similar to how success messages use✓and errors use✗.🔎 Suggested enhancement
// Display warnings if present if (result.warnings && result.warnings.length > 0) { console.log(''); for (const warning of result.warnings) { - console.log(warning); + console.log(`⚠ ${warning}`); } }openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md (2)
123-184: Installation scenarios are comprehensive but lack marker-based update definition.The scenarios reference "marker-based updates" (lines 133, 141, 151, 167) for shell config modifications, but the marker pattern itself is not formally defined in the spec. Ensure this pattern is documented (e.g., using begin/end markers like
# BEGIN openspec-completion/# END openspec-completion) either in a separate requirement or as implementation guidance.
233-292: Architecture patterns have implementation-specific details that should be generalized.
Line 272 — Caching TTL is overly specific: Specifying "2-second TTL" is a fine-grained implementation choice; the spec should instead state something like "implement caching to avoid redundant filesystem operations" and let implementation decide appropriate duration based on trade-offs.
Line 282 — positionalType values are underspecified: The spec defines
positionalType: stringbut doesn't enumerate valid values. The example mentions "change-id, spec-id, path, shell" — these should be formally listed as a union type or enum.🔎 Suggested refinements
- **AND** implement caching with 2-second TTL using class properties + **AND** implement caching to avoid redundant filesystem operations during rapid successive calls - **AND** `positionalType: string` - Type of positional (change-id, spec-id, path, shell) + **AND** `positionalType: 'change-id' | 'spec-id' | 'path' | 'shell'` - Type of positional argument
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openspec/changes/archive/2025-12-23-extend-shell-completions/proposal.mdopenspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.mdopenspec/changes/archive/2025-12-23-extend-shell-completions/tasks.mdsrc/commands/completion.tssrc/utils/file-system.tstest/utils/file-system.test.ts
✅ Files skipped from review due to trivial changes (1)
- openspec/changes/archive/2025-12-23-extend-shell-completions/proposal.md
🧰 Additional context used
📓 Path-based instructions (2)
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.mdopenspec/changes/archive/2025-12-23-extend-shell-completions/tasks.md
openspec/changes/**/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/spec.md: Use## ADDED|MODIFIED|REMOVED|RENAMED Requirementsheaders in spec delta files
Include at least one#### Scenario:per requirement in spec delta files
Use#### Scenario: Nameformat (4 hashtags) for scenario headers, not bullets or bold text
Use## ADDED Requirementsfor new orthogonal capabilities that can stand alone; use## MODIFIED Requirementsfor behavior changes of existing requirements
When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Files:
openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md
🧠 Learnings (2)
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` headers in spec delta files
Applied to files:
openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/tasks.md : Ensure `tasks.md` contains implementation checklist with numbered sections and checkbox items
Applied to files:
openspec/changes/archive/2025-12-23-extend-shell-completions/tasks.md
🔇 Additional comments (11)
openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md (4)
5-46: Shell-specific scenarios are well-formed.The Native Shell Behavior Integration requirement correctly delineates shell-specific UX patterns with WHEN/THEN structure. The negation scenario (lines 41–46) is a good boundary-setting constraint.
48-81: Shell detection scenarios are comprehensive and well-specified.The detection logic appropriately handles platform-specific environment variables ($SHELL for Unix shells, $PSModulePath for PowerShell). Scenarios are clear and the error message provides useful context.
185-231: Uninstallation scenarios are well-structured and symmetric with installation.The scenarios properly mirror installation flows with appropriate confirmations and error handling. Marker-based removal pattern (referenced in lines 196, 204, 218) shares the definition concern flagged in the Installation section above.
294-327: Testing support scenarios provide appropriate guidance for multi-shell coverage.The scenarios correctly emphasize dependency injection, temporary test directories, and cross-shell consistency validation. This requirement appears entirely new (not a modification of existing testing), which supports the earlier recommendation to separate ADDED vs MODIFIED requirements.
openspec/changes/archive/2025-12-23-extend-shell-completions/tasks.md (1)
1-49: Implementation tasks correctly reference existing codebase artifacts and follow required structure.The tasks.md file uses proper scaffolding with numbered phases (1–5) and checkbox items as required. Verification confirms all referenced implementation artifacts exist: generators and installers for bash, fish, and PowerShell; the CompletionFactory and interfaces; updated SupportedShell type; and corresponding test files. The structure is sound and complete.
Likely an incorrect or invalid review comment.
src/utils/file-system.ts (2)
88-121: LGTM! Well-structured directory traversal logic.The new helper correctly walks up the directory tree to find the first existing directory. The infinite loop is safe due to explicit exit conditions (lines 100, 104, 111), and the root detection check at line 109 (
parentDir === currentDir) properly prevents infinite traversal. Edge cases are appropriately handled with debug logging.
140-156: Robust handling of non-existent file paths.The updated ENOENT handling correctly uses the new helper to traverse up to the first existing directory, then checks its write permissions. This properly handles deeply nested non-existent paths that the previous implementation may not have covered.
Note: There's an inherent TOCTOU (time-of-check to time-of-use) limitation where permissions could change between this check and actual write operations, but this is acceptable for the method's purpose of checking current state.
test/utils/file-system.test.ts (4)
227-236: Excellent edge case test for file-in-path scenario.This test cleverly validates the behavior when an intermediate path component is a file rather than a directory. The implementation correctly handles this:
findFirstExistingDirectorywill find the file, detect it's not a directory (line 99 in the implementation), and returnnull, causingcanWriteFileto returnfalse. This prevents false positives in edge cases where filesystem structure is unusual.
238-252: Good coverage of symlinks and cross-platform paths.The symlink test correctly validates that
canWriteFilefollows symbolic links (sincefs.statdereferences symlinks by default). The platform-specific separator test usingjoinPathensures the method works correctly with platform-appropriate path construction.
199-210: This test's cross-platform reliability is uncertain due to Windows directory permission semantics.The test uses
fs.chmod(readOnlyDir, 0o555)to create a read-only directory. On Windows, this sets the read-only file attribute, but for directories, this attribute has different semantics than Unix permissions—it may not prevent writes in the way the test expects. Although thecanWriteFile()implementation correctly usesfs.access()withW_OKinstead of relying onstats.mode, the test setup itself may not reliably simulate the intended read-only condition on Windows since the test runs in CI onwindows-latest.Consider either: (1) adding a skip condition for Windows (
skipIf(process.platform === 'win32')), or (2) verifying thatfs.access(W_OK)on thereadOnlyDiractually returns false afterchmod(0o555)on Windows before finalizing this test.
165-197: Comprehensive test coverage for basic scenarios.The tests for writable files (lines 165-171), non-existent files in writable directories (lines 185-190), and deeply nested non-existent paths (lines 192-197) provide excellent coverage and will validate the new
findFirstExistingDirectoryhelper.The read-only file test (lines 173-183) is properly cross-platform compatible. The
canWriteFileimplementation usesfs.access()withW_OKto check write permissions rather than relying onstats.mode, which handles both Unix chmod-based permissions and Windows NTFS attributes correctly. The test will work as expected on all platforms.
openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md
Show resolved
Hide resolved
openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md
Show resolved
Hide resolved
- Add _init_completion fallback for systems without bash-completion - Fix command injection escaping in Fish/PowerShell generators - Add Bash command name escaping for security - Add comprehensive security tests for all generators - Fix test placement issues in bash/powershell test files
…ompletions' into feature/bash_fish_power_shells_completions
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/core/completions/installers/powershell-installer.test.ts (2)
147-159: Test fails because parent directory is not created before callingconfigureProfile.The test needs to create the parent directory structure before calling
configureProfile. Other tests in this block correctly create the directory withawait fs.mkdir(path.dirname(profilePath), { recursive: true })before the test.🔎 Proposed fix
it('should create profile with markers when file does not exist', async () => { delete process.env.OPENSPEC_NO_AUTO_CONFIG; const profilePath = installer.getProfilePath(); + await fs.mkdir(path.dirname(profilePath), { recursive: true }); const result = await installer.configureProfile(mockScriptPath);
237-248: Same issue: missing parent directory creation causes test failure.This test also needs to create the parent directory before calling
configureProfile. The subsequent file read operation fails because the profile cannot be created without its parent directory.🔎 Proposed fix
it('should generate correct PowerShell syntax in config', async () => { delete process.env.OPENSPEC_NO_AUTO_CONFIG; const profilePath = installer.getProfilePath(); + await fs.mkdir(path.dirname(profilePath), { recursive: true }); await installer.configureProfile(mockScriptPath);
🧹 Nitpick comments (2)
src/core/completions/generators/fish-generator.ts (1)
178-180: Consider deriving shell list from a central source.The hardcoded list
'zsh bash fish powershell'could become out of sync if new shells are added. This is acceptable for now, but consider importing from a shared constant (e.g., fromtypes.tswhereSupportedShellis defined) for consistency.src/core/completions/installers/bash-installer.ts (1)
259-269: Consider making bash-completion installation instructions cross-platform.The warning currently provides macOS/Homebrew-specific instructions. While helpful, users on Linux distributions would need different commands (e.g.,
apt install bash-completionoryum install bash-completion).This is acceptable as-is since the warning alerts users to the dependency, but could be enhanced to detect the platform and provide appropriate instructions for the user's system.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/core/completions/generators/bash-generator.tssrc/core/completions/generators/fish-generator.tssrc/core/completions/generators/powershell-generator.tssrc/core/completions/installers/bash-installer.tssrc/core/completions/installers/powershell-installer.tstest/core/completions/generators/bash-generator.test.tstest/core/completions/generators/fish-generator.test.tstest/core/completions/generators/powershell-generator.test.tstest/core/completions/installers/powershell-installer.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/completions/generators/bash-generator.ts
- test/core/completions/generators/bash-generator.test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
test/core/completions/generators/fish-generator.test.ts (3)
scripts/postinstall.js (2)
generator(80-80)script(81-81)src/core/completions/generators/fish-generator.ts (1)
FishGenerator(7-234)src/core/completions/types.ts (1)
CommandDefinition(36-72)
src/core/completions/installers/bash-installer.ts (1)
src/core/completions/factory.ts (1)
InstallationResult(15-27)
src/core/completions/installers/powershell-installer.ts (2)
src/utils/file-system.ts (1)
FileSystemUtils(44-252)src/core/completions/factory.ts (1)
InstallationResult(15-27)
src/core/completions/generators/powershell-generator.ts (2)
src/core/completions/types.ts (2)
CompletionGenerator(77-90)CommandDefinition(36-72)scripts/postinstall.js (1)
script(81-81)
src/core/completions/generators/fish-generator.ts (1)
src/core/completions/types.ts (3)
CompletionGenerator(77-90)CommandDefinition(36-72)FlagDefinition(6-31)
test/core/completions/installers/powershell-installer.test.ts (1)
src/core/completions/installers/powershell-installer.ts (1)
PowerShellInstaller(11-326)
🔇 Additional comments (9)
test/core/completions/generators/fish-generator.test.ts (1)
1-531: Comprehensive test suite for FishGenerator - LGTM!The test file provides excellent coverage including:
- Interface compliance verification
- Command and flag generation with various configurations
- Positional argument handling for different types
- Dynamic completion helpers
- Security-focused tests for shell metacharacter escaping
The security tests (lines 431-531) are particularly valuable, ensuring that command injection vectors like
$(), backticks, and other shell metacharacters are properly escaped.src/core/completions/generators/fish-generator.ts (2)
16-63: Well-structured generate method.The main
generatemethod is well-organized with clear sections for header, helper functions, top-level commands, and command-specific completions. The use ofscript.join('\n')for output construction is clean and maintainable.
227-233: Escape method correctly handles shell metacharacters.The ordering is correct (backslashes first to avoid double-escaping), and the key metacharacters for Fish single-quoted strings are covered. This aligns well with the security tests in the test file.
test/core/completions/generators/powershell-generator.test.ts (1)
448-531: Excellent security test coverage!The security tests comprehensively validate escaping of PowerShell metacharacters that could lead to command injection vulnerabilities. Testing $() subexpressions, backticks, dollar signs, and double quotes together (as in the final test) ensures the escaping logic handles real-world dangerous inputs.
src/core/completions/installers/powershell-installer.ts (2)
100-130: Well-designed profile configuration with proper safeguards.The implementation correctly:
- Respects the
OPENSPEC_NO_AUTO_CONFIGflag for user control- Checks write permissions before attempting modifications
- Uses marker-based updates for idempotent configuration
- Fails gracefully without breaking the installation
194-263: Robust installation implementation with excellent UX.The install method demonstrates good engineering:
- Detects and handles both fresh installs and updates appropriately
- Creates backups before modifying existing installations
- Attempts auto-configuration with graceful fallback to manual instructions
- Returns comprehensive status information including paths, backup location, and user guidance
src/core/completions/generators/powershell-generator.ts (2)
221-226: Security-critical escaping is correctly implemented.The escape sequence order is correct for PowerShell:
- Backticks first (escape character itself)
- Dollar signs (prevents variable expansion and
$()injection)- Double quotes (string delimiters)
This prevents command injection while preserving description text. The comprehensive test coverage validates this logic.
187-216: Well-structured dynamic completion helpers.The helper functions correctly:
- Shell out to the CLI's
__completecommand for dynamic data- Parse the tab-delimited output to extract IDs
- Suppress errors with
2>$nullto avoid polluting completion output- Return properly formatted arrays for PowerShell's completion system
src/core/completions/installers/bash-installer.ts (1)
117-147: Consistent and well-structured configuration management.The
.bashrcconfiguration follows the same reliable pattern as the PowerShell installer:
- Respects user opt-out via
OPENSPEC_NO_AUTO_CONFIG- Checks write permissions before modifications
- Uses markers for safe, idempotent updates
- Fails gracefully without breaking installation
|
!reviewfast |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
There was a problem hiding this comment.
I think the main comment I have is we mention zsh only in a lot of places even though we're now supporting multiple shells.
From the AI Code review I ran to check this:
Spec drift still claims Zsh-only in a few places: purpose (openspec/specs/cli-completion/spec.md:4), command list (openspec/specs/cli-completion/spec.md:58), unsupported-shell message (openspec/specs/cli-completion/spec.md:346), and “non‑Zsh shell” wording (openspec/specs/cli-completion/spec.md:364).
Zsh generator completes shell positional as zsh only (src/core/completions/generators/zsh-generator.ts:296); should list zsh|bash|fish|powershell like the other generators.
Completion registry omits completion uninstall --yes/-y even though the CLI supports it (src/core/completions/command-registry.ts:283, compare src/cli/index.ts:294).
UX is still zsh-centric:
- Install “next steps” only print for zshrcConfigured (src/commands/completion.ts:166) so bash/profile auto-config can end with no reload hint.
- Verbose output only mentions ~/.zshrc (src/commands/completion.ts:147).
- Uninstall prompt always says ~/.zshrc (src/commands/completion.ts:191), even for bash/fish/powershell.
|
|
||
| /** | ||
| * Generate dynamic completion helper functions for change and spec IDs | ||
| */ |
There was a problem hiding this comment.
minor don't need to fix
lol I just realised there's a tab completion for openspec completion generate <TAB> that was added as part of generatePositionalSpec . <- This only returns zsh
If we're supporting more shells would make sense to add more in ... but this is obviously a little awkward as the tab completion only works if the user already has a completion installed.
| } | ||
| } | ||
|
|
||
| // Display warnings if present |
There was a problem hiding this comment.
We have a bunch of zshrcConfigured checks in this file, do we need to extend for other shells here too?
There was a problem hiding this comment.
Similarly I think there's a few places where we focus on zsh heavily. For example there's a restart hint for zsh but not for other shells.
There was a problem hiding this comment.
I think even when uninstalling we mention zshrc
| * @returns true if bash-completion directories exist | ||
| */ | ||
| async isBashCompletionInstalled(): Promise<boolean> { | ||
| const paths = [ |
There was a problem hiding this comment.
How are these paths determined? Does it work if you use homebrew too for example?
There was a problem hiding this comment.
I have static paths to check for, for example:
'/usr/share/bash-completion',
'/usr/local/share/bash-completion',
'/etc/bash_completion.d'
I'll also add these for homebrew:
'/opt/homebrew/etc/bash_completion.d',
'/usr/local/etc/bash_completion.d',
| // Fall back to platform-specific defaults | ||
| if (process.platform === 'win32') { | ||
| // Windows: Documents/PowerShell/Microsoft.PowerShell_profile.ps1 | ||
| return path.join(this.homeDir, 'Documents', 'PowerShell', 'Microsoft.PowerShell_profile.ps1'); |
There was a problem hiding this comment.
Documents/PowerShell/ is the profile location for PowerShell Core (6+). Windows PowerShell 5.1 uses Documents/WindowsPowerShell/ instead. Since the spec says "support both Windows PowerShell 5.1 and PowerShell Core 7+", this needs to handle both paths
Extract static template literals from generators into separate template files.
Standardize naming to {SHELL}_STATIC_HELPERS and {SHELL}_DYNAMIC_HELPERS.
- Create bash/fish/powershell/zsh template files
- Rename constants: BASH_HELPERS → BASH_DYNAMIC_HELPERS,
FISH_HELPER_FUNCTIONS → FISH_STATIC_HELPERS,
POWERSHELL_HELPERS → POWERSHELL_DYNAMIC_HELPERS
- Update generator imports
- Remove ~99 lines of boilerplate from generators
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/core/completions/generators/zsh-generator.ts (1)
247-247: Update shell completions to include newly supported shells.The shell completion only offers
zsh, but this PR adds support forbash,fish, andpowershell. Update the completion list to reflect all supported shells.🔎 Proposed fix
case 'shell': - return "'*:shell:(zsh)'"; + return "'*:shell:(zsh bash fish powershell)'"; default:src/core/completions/generators/powershell-generator.ts (1)
168-172: Hard-coded shell list creates maintenance burden.This is the same issue as in bash-generator.ts line 156. The shell list is duplicated here, creating a maintenance risk if new shells are added.
See the refactoring suggestion in bash-generator.ts (lines 155-157) to centralize the shell list in types.ts. Apply the same pattern here:
+import { SUPPORTED_SHELLS } from '../types.js'; ... case 'shell': - lines.push(`${indent}$shells = @("zsh", "bash", "fish", "powershell")`); + lines.push(`${indent}$shells = @(${SUPPORTED_SHELLS.map(s => `"${s}"`).join(', ')})`); lines.push(`${indent}$shells | Where-Object { $_ -like "$wordToComplete*" } | ForEach-Object {`);
🧹 Nitpick comments (4)
src/core/completions/templates/zsh-templates.ts (1)
1-4: Clarify documentation - these are dynamic helpers, not static.The comment describes these as "static template strings" and "helper functions that never change," but
ZSH_DYNAMIC_HELPERScontains functions that dynamically invokeopenspec __completecommands. Consider renaming or clarifying the comment to reflect that while the template structure is static, the helpers perform dynamic completion.🔎 Proposed documentation fix
/** - * Static template strings for Zsh completion scripts. - * These are Zsh-specific helper functions that never change. + * Dynamic helper template for Zsh completion scripts. + * These Zsh-specific functions dynamically fetch completion data from openspec. */src/core/completions/templates/powershell-templates.ts (1)
1-4: Clarify documentation - these are dynamic helpers, not static.The comment describes these as "static template strings" and "helper functions that never change," but the helpers dynamically execute
openspec __completecommands. This same issue appears in bash-templates.ts and zsh-templates.ts. Consider updating the documentation to clarify that while the template is static, the functions perform dynamic operations.🔎 Proposed documentation fix
/** - * Static template strings for PowerShell completion scripts. - * These are PowerShell-specific helper functions that never change. + * Dynamic helper template for PowerShell completion scripts. + * These PowerShell-specific functions dynamically fetch completion data from openspec. */src/core/completions/templates/bash-templates.ts (1)
1-4: Clarify documentation - these are dynamic helpers, not static.Same documentation inconsistency as in zsh-templates.ts and powershell-templates.ts: the comment says "static template strings" and "never change," but these are dynamic helpers that execute commands.
🔎 Proposed documentation fix
/** - * Static template strings for Bash completion scripts. - * These are Bash-specific helper functions that never change. + * Dynamic helper template for Bash completion scripts. + * These Bash-specific functions dynamically fetch completion data from openspec. */src/core/completions/generators/bash-generator.ts (1)
155-157: Hard-coded shell list creates maintenance burden.The shell list
"zsh bash fish powershell"is hard-coded here and duplicated in powershell-generator.ts (line 169). If a new shell is added to the project, both locations must be updated manually, risking inconsistency.🔎 Centralize shell list in types.ts
In
src/core/completions/types.ts, add:export const SUPPORTED_SHELLS = ['zsh', 'bash', 'fish', 'powershell'] as const; export const SUPPORTED_SHELLS_STRING = SUPPORTED_SHELLS.join(' ');Then import and use in this file:
+import { SUPPORTED_SHELLS_STRING } from '../types.js'; ... case 'shell': - lines.push(`${indent}local shells="zsh bash fish powershell"`); + lines.push(`${indent}local shells="${SUPPORTED_SHELLS_STRING}"`); lines.push(`${indent}COMPREPLY=($(compgen -W "$shells" -- "$cur"))`);Apply the same change in powershell-generator.ts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/core/completions/generators/bash-generator.tssrc/core/completions/generators/fish-generator.tssrc/core/completions/generators/powershell-generator.tssrc/core/completions/generators/zsh-generator.tssrc/core/completions/templates/bash-templates.tssrc/core/completions/templates/fish-templates.tssrc/core/completions/templates/powershell-templates.tssrc/core/completions/templates/zsh-templates.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/completions/generators/fish-generator.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/core/completions/generators/bash-generator.ts (2)
src/core/completions/types.ts (2)
CompletionGenerator(77-90)CommandDefinition(36-72)src/core/completions/templates/bash-templates.ts (1)
BASH_DYNAMIC_HELPERS(6-24)
src/core/completions/generators/zsh-generator.ts (1)
src/core/completions/templates/zsh-templates.ts (1)
ZSH_DYNAMIC_HELPERS(6-36)
src/core/completions/generators/powershell-generator.ts (2)
src/core/completions/types.ts (2)
CompletionGenerator(77-90)CommandDefinition(36-72)src/core/completions/templates/powershell-templates.ts (1)
POWERSHELL_DYNAMIC_HELPERS(6-25)
🔇 Additional comments (12)
src/core/completions/templates/fish-templates.ts (1)
6-21: LGTM!The static helper functions are well-structured and follow Fish shell conventions correctly.
src/core/completions/templates/zsh-templates.ts (1)
6-36: LGTM!The Zsh helper functions are correctly implemented with proper process substitution, tab-separated parsing, and
_describeusage.src/core/completions/generators/zsh-generator.ts (1)
17-81: LGTM!The refactoring to template-based assembly with explicit array building improves readability and maintainability. The use of
ZSH_DYNAMIC_HELPERSfrom the template module properly separates concerns.src/core/completions/templates/bash-templates.ts (1)
6-24: LGTM!The Bash helper functions are correctly implemented using standard Bash completion patterns with
compgen, proper command substitution, and tab-delimited field extraction viacut. The inclusion of all three helper functions (changes, specs, and items) maintains consistency with Fish and Zsh templates.src/core/completions/generators/bash-generator.ts (4)
17-28: Code structure addresses prior feedback.The use of
push()for loop-based construction (lines 22-27) followed by template literal assembly (lines 34-75) is a reasonable pattern for complex logic with loops and conditionals. While template literals throughout (as TabishB suggested) could work, the current approach offers better clarity when building conditional blocks programmatically.
81-107: LGTM!The command case generation logic correctly handles both subcommand scenarios and direct argument completion. The recursive delegation to
generateArgumentCompletion()for subcommands is clean and maintainable.
112-137: LGTM!The argument completion logic properly prioritizes flag completion (when
curstarts with-) before falling back to positional completions. This matches standard Bash completion behavior.
171-174: LGTM!The escape logic correctly handles Bash shell metacharacters (
",$,`,\) to prevent command injection. The replacement pattern is secure and comprehensive.src/core/completions/generators/powershell-generator.ts (4)
17-68: LGTM!The PowerShell completion script structure is well-organized. The use of
push()for loop-based construction followed by template literal assembly provides good clarity. The script correctly usesRegister-ArgumentCompleterand handles the top-level command/subcommand flow with appropriate token parsing.
73-106: LGTM!The command case generation logic correctly handles subcommand scenarios with proper indentation and delegation. The nested switch statement for subcommands (lines 91-99) is clean and matches PowerShell conventions.
111-143: LGTM!The argument completion logic properly handles flags (checking for
-*prefix) and includes both long and short flag forms when available. The delegation to positional completion is clean.
185-190: LGTM!The escape logic correctly handles PowerShell-specific special characters:
- Backticks (PowerShell's escape character)
- Dollar signs (prevent variable expansion)
- Double quotes (string delimiters)
The escaping is secure and comprehensive for PowerShell string contexts.
| export const POWERSHELL_DYNAMIC_HELPERS = `# Dynamic completion helpers | ||
| function Get-OpenSpecChanges { | ||
| $output = openspec __complete changes 2>$null | ||
| if ($output) { | ||
| $output | ForEach-Object { | ||
| ($_ -split "\\t")[0] | ||
| } | ||
| } | ||
| } | ||
| function Get-OpenSpecSpecs { | ||
| $output = openspec __complete specs 2>$null | ||
| if ($output) { | ||
| $output | ForEach-Object { | ||
| ($_ -split "\\t")[0] | ||
| } | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
Add a combined items function for consistency with other shells.
All other shell templates (Bash, Fish, and Zsh) provide a combined function that returns both changes and specs (e.g., _openspec_complete_items, __fish_openspec_items). PowerShell is missing this combined helper, which creates an inconsistency in the multi-shell completion architecture.
🔎 Proposed combined items function
function Get-OpenSpecSpecs {
$output = openspec __complete specs 2>$null
if ($output) {
$output | ForEach-Object {
($_ -split "\\t")[0]
}
}
}
+
+function Get-OpenSpecItems {
+ $changes = openspec __complete changes 2>$null
+ $specs = openspec __complete specs 2>$null
+ $combined = @()
+ if ($changes) {
+ $combined += $changes | ForEach-Object { ($_ -split "\\t")[0] }
+ }
+ if ($specs) {
+ $combined += $specs | ForEach-Object { ($_ -split "\\t")[0] }
+ }
+ $combined
+}
`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const POWERSHELL_DYNAMIC_HELPERS = `# Dynamic completion helpers | |
| function Get-OpenSpecChanges { | |
| $output = openspec __complete changes 2>$null | |
| if ($output) { | |
| $output | ForEach-Object { | |
| ($_ -split "\\t")[0] | |
| } | |
| } | |
| } | |
| function Get-OpenSpecSpecs { | |
| $output = openspec __complete specs 2>$null | |
| if ($output) { | |
| $output | ForEach-Object { | |
| ($_ -split "\\t")[0] | |
| } | |
| } | |
| } | |
| `; | |
| export const POWERSHELL_DYNAMIC_HELPERS = `# Dynamic completion helpers | |
| function Get-OpenSpecChanges { | |
| $output = openspec __complete changes 2>$null | |
| if ($output) { | |
| $output | ForEach-Object { | |
| ($_ -split "\\t")[0] | |
| } | |
| } | |
| } | |
| function Get-OpenSpecSpecs { | |
| $output = openspec __complete specs 2>$null | |
| if ($output) { | |
| $output | ForEach-Object { | |
| ($_ -split "\\t")[0] | |
| } | |
| } | |
| } | |
| function Get-OpenSpecItems { | |
| $changes = openspec __complete changes 2>$null | |
| $specs = openspec __complete specs 2>$null | |
| $combined = @() | |
| if ($changes) { | |
| $combined += $changes | ForEach-Object { ($_ -split "\\t")[0] } | |
| } | |
| if ($specs) { | |
| $combined += $specs | ForEach-Object { ($_ -split "\\t")[0] } | |
| } | |
| $combined | |
| } | |
| `; |
🤖 Prompt for AI Agents
In src/core/completions/templates/powershell-templates.ts around lines 6 to 25,
PowerShell is missing a combined items helper (like other shells) that returns
both changes and specs; add a new function Get-OpenSpecItems that calls
Get-OpenSpecChanges and Get-OpenSpecSpecs, merges their outputs (preserving
order or deduplicating as needed), and writes the combined list to stdout so the
completion logic can consume a single unified items helper consistent with
Bash/Fish/Zsh.
Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
@noameron Could you check the 2 critical coderabbit errors highlighted |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/specs/cli-completion/spec.md (1)
4-41: Spec: tighten normative language + avoid overly absolute “double TAB” claim for Bash.
Peropenspec/specs/**/spec.mdguidelines, the new normative bullets should consistently use SHALL/MUST (several say “support …” / “leverage …” without SHALL). Also, “completions SHALL trigger on double TAB” for Bash is not reliably true (single TAB can complete unambiguous tokens; double TAB typically lists options).Example wording adjustments
- - **AND** completions SHALL trigger on double TAB (standard Bash behavior) + - **AND** completions SHALL follow standard Bash behavior (TAB completes; a second TAB may list options) - - **AND** support both bash-completion v1 and v2 patterns + - **AND** completions SHALL support both bash-completion v1 and v2 patterns - - **AND** leverage Fish's built-in caching automatically + - **AND** completions SHALL leverage Fish's built-in caching automatically
🤖 Fix all issues with AI agents
In @openspec/specs/cli-completion/spec.md:
- Around line 70-95: Summary: The spec's validation/error bullets are
non-normative; change them to mandatory normative language. Update the
enumerated bullets and scenario outcomes (e.g., the lines that currently read
“validate the shell is one of: `zsh`, `bash`, `fish`, `powershell`” and “throw
an error if the shell is not supported”, and the scenario THEN/THROW lines such
as "THEN throw error: "Shell '<name>' is not supported. Supported shells: zsh,
bash, fish, powershell"") to use SHALL/MUST wording (e.g., "The system SHALL
validate that the shell is one of …" and "The system SHALL throw an error 'Shell
'<name>' is not supported. Supported shells: zsh, bash, fish, powershell' when
not supported"), and apply the same normative phrasing to each scenario outcome
for Bash, Fish, and PowerShell detection so the behavior is unambiguous and
mandatory.
In @src/core/completions/generators/bash-generator.ts:
- Around line 79-105: generateCommandCase currently, when cmd.subcommands
exists, only completes subcommands at cword == 2 and never offers parent-level
flags; modify generateCommandCase so that before emitting the subcommands list
for cword == 2 it also builds and offers parent flags (cmd.flags) combined with
subcommands (or offers flags first), by including cmd.flags (properly escaped)
in the local completions string or by calling generateArgumentCompletion(cmd,
indent) for the parent at that cword check; ensure you reference cmd.flags and
reuse escapeCommandName and generateArgumentCompletion so parent flags like
--scope are offered at word 2 while keeping the per-subcommand case block
intact.
🧹 Nitpick comments (2)
src/core/completions/command-registry.ts (1)
287-293: Good addition:completion uninstall --yes/-ymakes non-interactive flows scriptable.
Small consistency nit: there are multiple ad-hoc{ name: 'yes', short: 'y' }flag definitions in this registry (e.g.,archive,config reset, now here). Consider promoting aCOMMON_FLAGS.yesto avoid drift.src/core/completions/generators/bash-generator.ts (1)
18-29: Consider escaping/quoting command names consistently in Bashcasepatterns.
Top-level command list usesescapeCommandName()(Line 19), butcaseLines.push(\${cmd.name})`)(Line 24) uses raw names, which become glob patterns incase`. Probably safe with your current fixed command set, but easy to harden by escaping or using a literal match pattern.Also applies to: 63-66, 169-172
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openspec/specs/cli-completion/spec.mdsrc/core/completions/command-registry.tssrc/core/completions/generators/bash-generator.tssrc/core/completions/generators/zsh-generator.ts
🧰 Additional context used
📓 Path-based instructions (1)
openspec/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Files:
openspec/specs/cli-completion/spec.md
🧬 Code graph analysis (2)
src/core/completions/generators/zsh-generator.ts (1)
src/core/completions/templates/zsh-templates.ts (1)
ZSH_DYNAMIC_HELPERS(6-36)
src/core/completions/generators/bash-generator.ts (2)
src/core/completions/types.ts (2)
CompletionGenerator(77-90)CommandDefinition(36-72)src/core/completions/templates/bash-templates.ts (1)
BASH_DYNAMIC_HELPERS(6-24)
🔇 Additional comments (2)
src/core/completions/generators/zsh-generator.ts (2)
246-248:shellpositional completion updated correctly to include all supported shells.
2-80: Clarify: top-level dispatch is correct, but subcommand dispatch may use wrong word index.The top-level dispatcher correctly uses
case $words[1] in(line 70)—$words[1]is the first argument afteropenspec(the subcommand name), notopenspecitself. However, in subcommand dispatch (line 122), the generated code also usescase $words[1]when it should likely use$words[2]for nested subcommands. When a user typesopenspec change show <TAB>,$words[1]refers to "change", but the code tries to match against subcommand names like "show". Tests do not validate actual zsh completion execution, only that strings are generated; this could be a "works locally but fails under real zsh" scenario.
| private generateCommandCase(cmd: CommandDefinition, indent: string): string[] { | ||
| const lines: string[] = []; | ||
|
|
||
| // Handle subcommands | ||
| if (cmd.subcommands && cmd.subcommands.length > 0) { | ||
| lines.push(`${indent}if [[ $cword -eq 2 ]]; then`); | ||
| lines.push(`${indent} local subcommands="` + cmd.subcommands.map(s => this.escapeCommandName(s.name)).join(' ') + '"'); | ||
| lines.push(`${indent} COMPREPLY=($(compgen -W "$subcommands" -- "$cur"))`); | ||
| lines.push(`${indent} return 0`); | ||
| lines.push(`${indent}fi`); | ||
| lines.push(''); | ||
| lines.push(`${indent}case "$subcmd" in`); | ||
|
|
||
| for (const subcmd of cmd.subcommands) { | ||
| lines.push(`${indent} ${subcmd.name})`); | ||
| lines.push(...this.generateArgumentCompletion(subcmd, indent + ' ')); | ||
| lines.push(`${indent} ;;`); | ||
| } | ||
|
|
||
| lines.push(`${indent}esac`); | ||
| } else { | ||
| // No subcommands, just complete arguments | ||
| lines.push(...this.generateArgumentCompletion(cmd, indent)); | ||
| } | ||
|
|
||
| return lines; | ||
| } |
There was a problem hiding this comment.
Bug: parent flags are not completable for commands with subcommands (e.g., openspec config --scope ...).
generateCommandCase() enters the “has subcommands” branch (Line 83) and, at cword -eq 2, only offers subcommands (Line 84-88). It never offers cmd.flags at the parent level in that flow. This undermines “same commands/flags across shells” expectations for commands like config that have both flags and subcommands.
Proposed fix (complete parent flags at word 2 before subcommand)
private generateCommandCase(cmd: CommandDefinition, indent: string): string[] {
const lines: string[] = [];
// Handle subcommands
if (cmd.subcommands && cmd.subcommands.length > 0) {
lines.push(`${indent}if [[ $cword -eq 2 ]]; then`);
+ if (cmd.flags.length > 0) {
+ const flags = cmd.flags.map(f => {
+ const parts: string[] = [];
+ if (f.short) parts.push(`-${f.short}`);
+ parts.push(`--${f.name}`);
+ return parts.join(' ');
+ }).join(' ');
+ lines.push(`${indent} if [[ "$cur" == -* ]]; then`);
+ lines.push(`${indent} local flags="${flags}"`);
+ lines.push(`${indent} COMPREPLY=($(compgen -W "$flags" -- "$cur"))`);
+ lines.push(`${indent} return 0`);
+ lines.push(`${indent} fi`);
+ }
lines.push(`${indent} local subcommands="` + cmd.subcommands.map(s => this.escapeCommandName(s.name)).join(' ') + '"');
lines.push(`${indent} COMPREPLY=($(compgen -W "$subcommands" -- "$cur"))`);
lines.push(`${indent} return 0`);
lines.push(`${indent}fi`);
lines.push('');
lines.push(`${indent}case "$subcmd" in`);Also applies to: 110-135
🤖 Prompt for AI Agents
In @src/core/completions/generators/bash-generator.ts around lines 79 - 105,
generateCommandCase currently, when cmd.subcommands exists, only completes
subcommands at cword == 2 and never offers parent-level flags; modify
generateCommandCase so that before emitting the subcommands list for cword == 2
it also builds and offers parent flags (cmd.flags) combined with subcommands (or
offers flags first), by including cmd.flags (properly escaped) in the local
completions string or by calling generateArgumentCompletion(cmd, indent) for the
parent at that cword check; ensure you reference cmd.flags and reuse
escapeCommandName and generateArgumentCompletion so parent flags like --scope
are offered at word 2 while keeping the per-subcommand case block intact.
Fish's echo doesn't interpret escape sequences, so \t outputs literally instead of as a tab character. Use printf for proper tab-separated completion output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/core/completions/installers/powershell-installer.ts (6)
4-4: Unused import:FileSystemUtilsThe
FileSystemUtilsis imported but never used in this file. UnlikeBashInstallerwhich usesFileSystemUtils.canWriteFile()andFileSystemUtils.updateFileWithMarkers(), this installer implements its own logic directly withfs.🔧 Suggested fix
import { promises as fs } from 'fs'; import path from 'path'; import os from 'os'; -import { FileSystemUtils } from '../../../utils/file-system.js'; import { InstallationResult } from '../factory.js';
109-116: Unused private method:generateProfileConfigThis method generates a profile configuration block but is never called anywhere in the class. The
configureProfilemethod at lines 148-154 constructs a different block inline instead.🔧 Either use this method or remove it
Option 1 - Remove the unused method:
- /** - * Generate PowerShell profile configuration content - * - * @param scriptPath - Path to the completion script - * @returns Configuration content - */ - private generateProfileConfig(scriptPath: string): string { - return [ - '# OpenSpec shell completions configuration', - `if (Test-Path "${scriptPath}") {`, - ` . "${scriptPath}"`, - '}', - ].join('\n'); - }Option 2 - Use the method in
configureProfilefor consistency with the generated instructions.
141-154: Inconsistency between marker check and marker block format.The check at line 143 looks for
. "${scriptPath}", but the block inserted at lines 148-154 includes surrounding markers and comments. If a user manually adds the source line without the markers, subsequent installs would skip configuration (line 144), butremoveProfileConfigwouldn't find the markers to clean up. This creates an inconsistent state.Consider checking for the markers themselves rather than just the script line:
🔧 Suggested approach
// Check if already configured - const scriptLine = `. "${scriptPath}"`; - if (profileContent.includes(scriptLine)) { + if (profileContent.includes(this.PROFILE_MARKERS.start)) { continue; // Already configured, skip }
159-162: Bareconsole.warnmay not align with the project's logging strategy.The installer uses
console.warndirectly here and at line 199, 213. If the project has a structured logging utility, consider using it for consistency. If not, this is acceptable for user-facing warnings.
248-251: Debug logging usesconsole.debugwhich may not be visible by default.The
console.debugcall at line 250 logs when the file can't be read, which is expected for new installations. Consider removing this debug statement or ensuring debug output is consistently handled across all installers.
320-357: Theoptionsparameter is unused in theuninstallmethod.The
options?.yesparameter is documented but never used within the method body. The confirmation prompt is handled at the command layer, making this parameter redundant here.🔧 Consider removing unused parameter or documenting why it exists
If the parameter is kept for interface consistency with other installers, add a comment explaining that confirmation is handled by the command layer. Otherwise, simplify:
- async uninstall(options?: { yes?: boolean }): Promise<{ success: boolean; message: string }> { + async uninstall(): Promise<{ success: boolean; message: string }> {src/core/completions/installers/bash-installer.ts (1)
335-365: Unusedoptionsparameter inuninstallmethod.Same issue as in
PowerShellInstaller- theoptions?.yesparameter is documented but not used. Confirmation is handled at the command layer.src/commands/completion.ts (2)
147-160: Duplicate code:configWasUpdatedlogic is repeated.The same
configWasUpdatedcheck andconfigPathsmapping appears at lines 149-159 and again at lines 178-191. Consider extracting to a helper.🔧 Extract shared logic
private getConfigInfo(shell: SupportedShell, result: InstallationResult): { updated: boolean; path: string } { const configPaths: Record<string, string> = { zsh: '~/.zshrc', bash: '~/.bashrc', fish: '~/.config/fish/config.fish', powershell: '$PROFILE', }; return { updated: !!(result.zshrcConfigured || result.bashrcConfigured || result.profileConfigured), path: configPaths[shell] || 'config file', }; }
171-171: Stale comment references only zsh.The comment says "only shown if .zshrc wasn't auto-configured" but the code now handles multiple shells. Update for clarity.
🔧 Update comment
- // Print instructions (only shown if .zshrc wasn't auto-configured) + // Print instructions (only shown if shell config wasn't auto-configured)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands/completion.tssrc/core/completions/installers/bash-installer.tssrc/core/completions/installers/powershell-installer.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/commands/completion.ts (1)
scripts/postinstall.js (3)
result(57-57)result(85-85)shell(128-128)
src/core/completions/installers/powershell-installer.ts (1)
src/core/completions/factory.ts (1)
InstallationResult(15-27)
🔇 Additional comments (9)
src/core/completions/installers/powershell-installer.ts (2)
26-46: Profile path resolution looks correct for cross-platform support.The logic correctly prioritizes the
$PROFILEenvironment variable (set when running inside PowerShell) and falls back to platform-specific defaults for Windows and Unix systems.
83-101: Backup implementation is clean and handles missing files gracefully.The timestamp-based backup naming and error handling for non-existent files follows best practices.
src/core/completions/installers/bash-installer.ts (4)
31-52: Good coverage of bash-completion paths including Homebrew locations.The detection includes both Intel (
/usr/local/...) and Apple Silicon (/opt/homebrew/...) Homebrew paths, aligning with the commit message "fix: add Homebrew paths for bash-completion detection."
59-65:getInstallationPathalways returns user-local path regardless of bash-completion availability.The method ignores system-wide bash-completion directories and always installs to
~/.local/share/bash-completion/completions/openspec. This is a safe choice for user installations without requiring elevated privileges.
119-149: Good use ofFileSystemUtilsfor marker-based config management.Unlike the PowerShell installer which implements its own logic, this correctly delegates to shared utilities, reducing code duplication and ensuring consistent behavior.
179-186: No issue found - marker format is consistent between write and removal operations.The removal logic at lines 179-186 will correctly match markers.
FileSystemUtils.updateFileWithMarkerswrites markers on their own lines (# OPENSPEC:START\n), and the removal method splits by newlines then trims each line before comparison. Since the marker string# OPENSPEC:STARTmatches exactly after split and trim, the search will succeed reliably.src/commands/completion.ts (3)
216-227: Shell-specific config paths for uninstall prompt look good.The mapping correctly handles all supported shells with appropriate paths. The Fish entry notes that it "doesn't modify profile, just removes script file" which is helpful context.
185-193: Reload commands are shell-appropriate.The mapping provides correct reload commands for each shell (
exec zsh,exec bash,exec fish,. $PROFILE), with a sensible fallback for unknown shells.
163-169: Warnings are displayed correctly.Each warning is printed on its own line after a blank separator, which improves readability for multi-line warning messages like the bash-completion warning.
Add -n : option to _init_completion to prevent colons from being treated as word separators. This is important for spec/change IDs that may contain colons. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Updated bash-generator test to expect `-n :` flag in _init_completion call. Updated powershell-installer tests to match refactored implementation that supports both PowerShell Core and Windows PowerShell 5.1. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/core/completions/installers/powershell-installer.test.ts (4)
157-172: Misleading test name: implementation appends, not prepends.The test name says "should prepend markers and config when file exists without markers", but the implementation actually appends the OpenSpec block to the end of the existing profile content (
profileContent + openspecBlock).Consider renaming to "should append markers and config when file exists without markers" for accuracy.
138-256: Consider adding test coverage for Windows dual-profile configuration.The implementation's
getAllProfilePaths()returns both PowerShell Core and Windows PowerShell 5.1 profile paths on Windows (as seen in relevant snippets). However, there's no test that verifiesconfigureProfilecorrectly updates both profiles on Windows. This is important functionality that should be tested.Suggested test case
it('should configure both PowerShell Core and Windows PowerShell 5.1 profiles on Windows', async () => { delete process.env.PROFILE; Object.defineProperty(process, 'platform', { value: 'win32', }); const coreProfilePath = path.join(testHomeDir, 'Documents', 'PowerShell', 'Microsoft.PowerShell_profile.ps1'); const legacyProfilePath = path.join(testHomeDir, 'Documents', 'WindowsPowerShell', 'Microsoft.PowerShell_profile.ps1'); const result = await installer.configureProfile(mockScriptPath); expect(result).toBe(true); // Verify both profiles are configured const coreContent = await fs.readFile(coreProfilePath, 'utf-8'); expect(coreContent).toContain('# OPENSPEC:START'); expect(coreContent).toContain(`. "${mockScriptPath}"`); const legacyContent = await fs.readFile(legacyProfilePath, 'utf-8'); expect(legacyContent).toContain('# OPENSPEC:START'); expect(legacyContent).toContain(`. "${mockScriptPath}"`); });
365-537: Unnecessary environment variable cleanup throughout install tests.Many tests delete
process.env.OPENSPEC_NO_AUTO_CONFIG(lines 375, 385, 395, 404, 415, 427, 442, 455, 474, 517, 527), but the comment at lines 451-452 explicitly states: "Note: OPENSPEC_NO_AUTO_CONFIG support was removed from PowerShell installer."Since this environment variable is no longer used by the implementation, these cleanup lines can be removed to reduce noise and improve test clarity.
539-645: Unnecessary environment variable cleanup throughout uninstall tests.Similar to the install tests, many uninstall tests delete
process.env.OPENSPEC_NO_AUTO_CONFIG(lines 546, 556, 567, 586, 596, 618), but this environment variable is no longer used by the PowerShell installer implementation.These cleanup lines can be removed for consistency with the comment at lines 451-452.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/completions/generators/bash-generator.tstest/core/completions/generators/bash-generator.test.tstest/core/completions/installers/powershell-installer.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/completions/generators/bash-generator.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/core/completions/installers/powershell-installer.test.ts (2)
scripts/postinstall.js (1)
installer(84-84)src/core/completions/installers/powershell-installer.ts (1)
PowerShellInstaller(11-358)
test/core/completions/generators/bash-generator.test.ts (3)
scripts/postinstall.js (2)
generator(80-80)script(81-81)src/core/completions/generators/bash-generator.ts (1)
BashGenerator(8-175)src/core/completions/types.ts (1)
CommandDefinition(36-72)
🔇 Additional comments (7)
test/core/completions/generators/bash-generator.test.ts (6)
1-10: LGTM: Clean test setup.The imports and test setup follow standard Vitest patterns with proper ESM paths and a
beforeEachhook for generator initialization.
12-20: LGTM: Interface compliance checks.These tests ensure the generator properly implements the
CompletionGeneratorinterface with the correct shell identifier and required methods.
22-289: Excellent coverage of script generation scenarios.This comprehensive test suite validates:
- Header and initialization (including the critical
_init_completion -n :on line 37, which aligns with the PR's colon-handling fix)- Command list generation
- Flag variations (boolean, value-taking, with/without short options, enum values)
- Subcommands
- All positional argument types (change-id, spec-id, change-or-spec-id, shell, path)
The tests appropriately use
.toContain()assertions to verify key script fragments without being overly rigid about formatting.
291-344: LGTM: Dynamic completion helper validation.These tests properly verify that:
- Dynamic completion helpers are generated for change-id, spec-id, and change-or-spec-id types
- Helpers invoke
openspec __completecommands with appropriate arguments- Generated functions follow Bash completion conventions (COMPREPLY, command substitution, error suppression)
346-418: LGTM: Edge cases and complex scenarios covered.These tests validate:
- Complex nested subcommands with flags and positional arguments work together correctly
- The script properly ends with
complete -F _openspec_completion openspecregistration (line 393 uses.trim().endsWith()for robust checking)- Edge cases like empty command lists and commands with no flags are handled gracefully
421-524: Excellent defense-in-depth security testing.This comprehensive security suite validates proper escaping of shell metacharacters in both command and subcommand names:
- Dollar signs (
$)- Backticks (
`)- Double quotes (
")- Backslashes (
\)The comment on line 439 appropriately notes that while command names are likely validated upstream, the escaping provides defense-in-depth protection against potential injection attacks. This is a best practice for shell script generation.
test/core/completions/installers/powershell-installer.test.ts (1)
1-647: Excellent comprehensive test coverage for PowerShellInstaller!This test suite is well-structured and thoroughly exercises the PowerShellInstaller across multiple dimensions:
- ✅ Platform-specific behavior (Windows, macOS, Linux)
- ✅ Environment variable handling (PROFILE env var)
- ✅ Edge cases (spaces in paths, special characters, empty scripts)
- ✅ Error scenarios (permission errors, missing files, invalid marker placement)
- ✅ Installation flows (first install, updates, backups)
- ✅ Profile configuration and cleanup
- ✅ Proper test isolation with temp directories and cleanup
The test organization is clear with descriptive test names (with one minor exception noted), and the setup/teardown properly isolates tests. Great work on comprehensive coverage!
TabishB
left a comment
TabishB
left a comment
There was a problem hiding this comment.
Thanks, I think this looks good! :) Keen to try it out!
fs.chmod() on directories doesn't restrict write access on Windows since Windows uses ACLs that Node.js doesn't control. Additionally, admin users and CI runners can bypass read-only attributes. Skip these tests on Windows and use platform-specific invalid paths in cross-platform tests. Fixes #401 (bash/pwsh completion commit breaking Windows e2e tests).
fs.chmod() on directories doesn't restrict write access on Windows since Windows uses ACLs that Node.js doesn't control. Additionally, admin users and CI runners can bypass read-only attributes. Skip these tests on Windows and use platform-specific invalid paths in cross-platform tests. Fixes #401 (bash/pwsh completion commit breaking Windows e2e tests).
2 participants
Added support for auto-completion for bash/fish/powershell
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.