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 factory and file-system helpers, and added comprehensive generator/installer tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as openspec (CLI)
participant Factory as CompletionFactory
participant Gen as CompletionGenerator (zsh/bash/fish/pwsh)
participant Installer as CompletionInstaller (per-shell)
participant FS as FileSystem (OS)
CLI->>Factory: createGenerator(selectedShell)
Factory->>Gen: instantiate specific generator
CLI->>Gen: generate(commands)
Gen-->>CLI: completion script
CLI->>Factory: createInstaller(selectedShell)
Factory->>Installer: instantiate specific installer
CLI->>Installer: install(completionScript)
Installer->>FS: resolve install path, check/backup existing
FS-->>Installer: path / backup info
Installer->>FS: write script, update profile/bashrc with 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 and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
| # Change Proposal: Extend Shell Completions | ||
|
|
||
| ## Why | ||
|
|
||
| Zsh completions provide an excellent developer experience, but many developers use bash, fish, or PowerShell. Extending completion support to these shells removes friction for the majority of developers who don't use Zsh. | ||
|
|
||
| ## What Changes | ||
|
|
||
| This change adds bash, fish, and PowerShell completion support following the same architectural patterns, documentation methodology, and testing rigor established for Zsh completions. | ||
|
|
||
| ## Deltas | ||
|
|
||
| - **Spec:** `cli-completion` | ||
| - **Operation:** MODIFIED | ||
| - **Description:** Extend completion generation, installation, and testing requirements to support bash, fish, and PowerShell while maintaining the existing Zsh implementation and architectural patterns No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing required "Impact" section per coding guidelines.
The proposal is missing the required "Impact" section that should list affected specs and code. Based on learnings, proposal.md must include sections: Why, What Changes (bullet list with breaking change markers), and Impact (affected specs and code).
🔎 Suggested addition
- **Description:** Extend completion generation, installation, and testing requirements to support bash, fish, and PowerShell while maintaining the existing Zsh implementation and architectural patterns
+
+## Impact
+
+- **Affected Specs:** `cli-completion`
+- **Affected Code:**
+ - `src/core/completions/generators/` (new bash, fish, powershell generators)
+ - `src/core/completions/installers/` (new bash, fish, powershell installers)
+ - `src/core/completions/factory.ts` (extended shell support)
+ - `src/utils/shell-detection.ts` (updated detection logic)🤖 Prompt for AI Agents
In openspec/changes/extend-shell-completions/proposal.md around lines 1 to 15,
the proposal lacks the required "Impact" section and missing breaking-change
markers in the What Changes bullets; update the file to include an "Impact"
section that enumerates all affected specs and code (e.g., cli-completion spec,
CLI generation code, install scripts, tests, and documentation), clearly
indicate whether each item is breaking or non-breaking, and amend the "What
Changes" bullets to include explicit breaking change markers where applicable so
the document contains Why, What Changes (with breaking-change annotations), and
Impact sections as per guidelines.
| 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
| ### Requirement: Completion Generation | ||
|
|
||
| The completion command SHALL generate completion scripts for all supported shells on demand. | ||
|
|
||
| #### Scenario: Generating Zsh completion | ||
|
|
||
| - **WHEN** user executes `openspec completion generate zsh` | ||
| - **THEN** output a complete Zsh completion script to stdout | ||
| - **AND** include completions for all commands: init, list, show, validate, archive, view, update, change, spec, completion | ||
| - **AND** include all command-specific flags and options | ||
| - **AND** use Zsh's `_arguments` and `_describe` built-in functions | ||
| - **AND** support dynamic completion for change and spec IDs | ||
|
|
||
| #### Scenario: Generating Bash completion | ||
|
|
||
| - **WHEN** user executes `openspec completion generate bash` | ||
| - **THEN** output a complete Bash completion script to stdout | ||
| - **AND** include completions for all commands and subcommands | ||
| - **AND** use `complete -F` with custom completion function | ||
| - **AND** populate `COMPREPLY` with appropriate suggestions | ||
| - **AND** support dynamic completion for change and spec IDs via `openspec __complete` | ||
|
|
||
| #### Scenario: Generating Fish completion | ||
|
|
||
| - **WHEN** user executes `openspec completion generate fish` | ||
| - **THEN** output a complete Fish completion script to stdout | ||
| - **AND** use `complete -c openspec` with conditions | ||
| - **AND** include command-specific completions with `--condition` predicates | ||
| - **AND** support dynamic completion for change and spec IDs via `openspec __complete` | ||
| - **AND** include descriptions for each completion option | ||
|
|
||
| #### Scenario: Generating PowerShell completion | ||
|
|
||
| - **WHEN** user executes `openspec completion generate powershell` | ||
| - **THEN** output a complete PowerShell completion script to stdout | ||
| - **AND** use `Register-ArgumentCompleter -CommandName openspec` | ||
| - **AND** implement scriptblock that handles command context | ||
| - **AND** support dynamic completion for change and spec IDs via `openspec __complete` | ||
| - **AND** return `[System.Management.Automation.CompletionResult]` objects |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the full spec file to see all requirements
cat -n "openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md"Repository: Fission-AI/OpenSpec
Length of output: 18077
openspec __complete command referenced but not formally specified as a requirement.
Lines 103, 111, and 120 reference openspec __complete for dynamic completion across shell generators, but this command lacks a dedicated requirement defining its interface and behavior. Either add a new requirement for the dynamic completion command or remove the reference and document dynamic completion purely through the architecture pattern described in the "Dynamic completion providers" scenario.
🤖 Prompt for AI Agents
In
openspec/changes/archive/2025-12-23-extend-shell-completions/specs/cli-completion/spec.md
around lines 83 to 121, the shell completion scenarios reference the helper
command "openspec __complete" for dynamic completions but there is no formal
requirement specifying that command's interface or behavior; add a new
Requirement section (or subsection) that defines the "openspec __complete"
command: its invocation signature, input/output format (e.g., args/context
passed, JSON or newline-separated suggestions), exit codes, security/permission
considerations, and expected behavior for completing change and spec IDs;
alternatively, remove the three references to "openspec __complete" and instead
update each shell scenario to reference the existing "Dynamic completion
providers" architecture section so the spec remains consistent.
- 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
| private generateFlagCompletion(flag: FlagDefinition, condition: string): string[] { | ||
| const lines: string[] = []; | ||
| const longFlag = `--${flag.name}`; | ||
| const shortFlag = flag.short ? `-${flag.short}` : undefined; | ||
|
|
||
| if (flag.takesValue && flag.values) { | ||
| // Flag with enum values | ||
| for (const value of flag.values) { | ||
| if (shortFlag) { | ||
| lines.push( | ||
| `complete -c openspec -n '${condition}' -s ${flag.short} -l ${flag.name} -a '${value}' -d '${this.escapeDescription(flag.description)}'` | ||
| ); | ||
| } else { | ||
| lines.push( | ||
| `complete -c openspec -n '${condition}' -l ${flag.name} -a '${value}' -d '${this.escapeDescription(flag.description)}'` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused variable and unescaped flag values.
- Line 119:
longFlagis declared but never used — it can be removed. - Lines 127/131: The
valuefromflag.valuesis interpolated directly into single quotes without escaping. If a value contains a single quote (e.g.,"don't"), the generated script will have broken syntax.
🔎 Proposed fix
private generateFlagCompletion(flag: FlagDefinition, condition: string): string[] {
const lines: string[] = [];
- const longFlag = `--${flag.name}`;
const shortFlag = flag.short ? `-${flag.short}` : undefined;
if (flag.takesValue && flag.values) {
// Flag with enum values
for (const value of flag.values) {
+ const escapedValue = this.escapeDescription(value);
if (shortFlag) {
lines.push(
- `complete -c openspec -n '${condition}' -s ${flag.short} -l ${flag.name} -a '${value}' -d '${this.escapeDescription(flag.description)}'`
+ `complete -c openspec -n '${condition}' -s ${flag.short} -l ${flag.name} -a '${escapedValue}' -d '${this.escapeDescription(flag.description)}'`
);
} else {
lines.push(
- `complete -c openspec -n '${condition}' -l ${flag.name} -a '${value}' -d '${this.escapeDescription(flag.description)}'`
+ `complete -c openspec -n '${condition}' -l ${flag.name} -a '${escapedValue}' -d '${this.escapeDescription(flag.description)}'`
);
}
}🤖 Prompt for AI Agents
In src/core/completions/generators/fish-generator.ts around lines 117 to 134,
remove the unused longFlag variable declaration at line 119, and when
interpolating values from flag.values at lines ~127 and ~131, escape any single
quotes so the generated single-quoted shell arguments remain valid (e.g.,
transform each value by replacing ' with '\'' or use a small helper like
escapeSingleQuotes(value) and use that escaped value in the -a argument); keep
using this.escapeDescription for descriptions and preserve the shortFlag
conditional logic.
|
!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.
| const script: string[] = []; | ||
|
|
||
| // Header comment | ||
| script.push('# Bash completion script for OpenSpec CLI'); |
There was a problem hiding this comment.
I think I missed the use of script.push here in the previous PR for zsh. My TS knowledge isn't great, but is this a common pattern for writing scripts to file? I would have assumed we could use something like a template literal and create one push rather than this?
generate(commands: CommandDefinition[]): string {
const commandList = commands.map(c => this.escape(c.name)).join(' ');
return `# Bash completion script for OpenSpec CLI
# Auto-generated - do not edit manually
_openspec_completion() {
local cur prev words cword
if declare -F _init_completion >/dev/null 2>&1; then
_init_completion || return
else
COMPREPLY=()
cur="\${COMP_WORDS[COMP_CWORD]}"
fi
if [[ $cword -eq 1 ]]; then
COMPREPLY=($(compgen -W "${commandList}" -- "$cur"))
return 0
fi
case "$cmd" in
${this.generateCommandCases(commands)}
esac
}
${this.generateDynamicHelpers()}
complete -F _openspec_completion openspec
`;
}
^ something like above (please verify)
LMK if there's a strong reasoning behind this approach.
There was a problem hiding this comment.
Same with lines.push down below too
|
|
||
| /** | ||
| * 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?
| // 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
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.