feature/oh-my-zsh-completions #289
Conversation
WalkthroughAdds a Zsh-first, plugin-based shell completion subsystem: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI User
participant Cmd as CompletionCommand
participant Detect as detectShell()
participant Factory as CompletionFactory
participant Gen as ZshGenerator
participant Registry as COMMAND_REGISTRY
participant Provider as CompletionProvider
participant Installer as ZshInstaller
Note over User,Cmd: User runs "openspec completion install [shell?]"
User->>Cmd: completion install [shell?]
Cmd->>Detect: detectShell()
Detect-->>Cmd: 'zsh'
Cmd->>Factory: createGenerator('zsh')
Factory-->>Gen: instantiate
Cmd->>Gen: generate(COMMAND_REGISTRY)
Gen->>Registry: read command metadata
Gen->>Provider: fetch dynamic IDs (changes/specs)
Provider-->>Gen: {changeIds, specIds}
Gen-->>Cmd: zsh completion script
Cmd->>Factory: createInstaller('zsh')
Factory-->>Installer: instantiate
Cmd->>Installer: install(script)
Installer->>Installer: detect OMZ / ensure dirs / backup
Installer->>Installer: write completion file
Installer->>Installer: optionally modify ~/.zshrc (markers)
Installer-->>Cmd: InstallationResult
Cmd-->>User: success message & instructions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (2)
src/core/completions/installers/zsh-installer.ts (2)
180-227: Consider extracting marker removal to FileSystemUtils for consistency.The
configureZshrcmethod usesFileSystemUtils.updateFileWithMarkers(Line 145) whileremoveZshrcConfigimplements custom removal logic. This creates potential for inconsistency if the marker handling in FileSystemUtils changes.Additionally, the logic at lines 215-217 unconditionally removes leading empty lines, but the comment suggests this should only happen "if the markers were at the top." The code doesn't actually verify the markers were at the top before removing these lines.
Consider:
- Add a
removeMarkerSectionmethod toFileSystemUtilsto centralize marker management- Clarify the empty line removal logic - either add a condition to check if
startIndex === 0, or update the comment if unconditional removal is intentionalExample for the empty line fix:
- // Remove trailing empty lines at the start if the markers were at the top - while (lines.length > 0 && lines[0].trim() === '') { - lines.shift(); - } + // Remove leading empty lines if the markers were at the top of the file + if (startIndex === 0) { + while (lines.length > 0 && lines[0].trim() === '') { + lines.shift(); + } + }
235-278: Consider validating the completion script parameter.The method writes
completionScriptdirectly without validating that it's non-empty or contains valid content. While the caller should provide valid input, defensive validation would prevent installing corrupt completions.Consider adding validation before writing:
async install(completionScript: string): Promise<InstallationResult> { try { + if (!completionScript || completionScript.trim().length === 0) { + return { + success: false, + isOhMyZsh: false, + message: 'Invalid completion script: content is empty', + }; + } + const { path: targetPath, isOhMyZsh } = await this.getInstallationPath();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
openspec/changes/archive/2025-11-06-add-shell-completions/design.md(1 hunks)openspec/changes/archive/2025-11-06-add-shell-completions/proposal.md(1 hunks)openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.md(1 hunks)openspec/changes/archive/2025-11-06-add-shell-completions/tasks.md(1 hunks)openspec/specs/cli-completion/spec.md(1 hunks)package.json(2 hunks)scripts/postinstall.js(1 hunks)scripts/test-postinstall.sh(1 hunks)src/cli/index.ts(2 hunks)src/commands/completion.ts(1 hunks)src/core/completions/command-registry.ts(1 hunks)src/core/completions/completion-provider.ts(1 hunks)src/core/completions/factory.ts(1 hunks)src/core/completions/generators/zsh-generator.ts(1 hunks)src/core/completions/installers/zsh-installer.ts(1 hunks)src/core/completions/types.ts(1 hunks)src/utils/shell-detection.ts(1 hunks)test/commands/completion.test.ts(1 hunks)test/core/completions/completion-provider.test.ts(1 hunks)test/core/completions/generators/zsh-generator.test.ts(1 hunks)test/core/completions/installers/zsh-installer.test.ts(1 hunks)test/utils/shell-detection.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
openspec/changes/*/**
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Choose a unique kebab-case, verb-led change-id (add-, update-, remove-, refactor-) for openspec/changes//
Files:
openspec/changes/archive/2025-11-06-add-shell-completions/design.mdopenspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.mdopenspec/changes/archive/2025-11-06-add-shell-completions/tasks.mdopenspec/changes/archive/2025-11-06-add-shell-completions/proposal.md
openspec/changes/archive/**
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Archive completed changes under openspec/changes/archive/YYYY-MM-DD-/ and run 'openspec validate --strict' post-archive; update specs/ if capabilities changed
Files:
openspec/changes/archive/2025-11-06-add-shell-completions/design.mdopenspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.mdopenspec/changes/archive/2025-11-06-add-shell-completions/tasks.mdopenspec/changes/archive/2025-11-06-add-shell-completions/proposal.md
openspec/{specs/**/spec.md,changes/*/specs/**/spec.md}
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/{specs/**/spec.md,changes/*/specs/**/spec.md}: Write requirements using SHALL/MUST for normative wording
Every requirement must include at least one scenario, formatted exactly with a '#### Scenario: ' header (no bullets or bold)
Under ADDED, introduce new capabilities as standalone requirements rather than altering existing ones
Files:
openspec/specs/cli-completion/spec.md
openspec/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Keep current, deployed specs in openspec/specs//spec.md (one focused capability per directory)
Files:
openspec/specs/cli-completion/spec.md
🧠 Learnings (8)
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/{specs/**/spec.md,changes/*/specs/**/spec.md} : Under ADDED, introduce new capabilities as standalone requirements rather than altering existing ones
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.mdopenspec/specs/cli-completion/spec.mdopenspec/changes/archive/2025-11-06-add-shell-completions/proposal.md
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/changes/archive/** : Archive completed changes under openspec/changes/archive/YYYY-MM-DD-<name>/ and run 'openspec validate --strict' post-archive; update specs/ if capabilities changed
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/changes/*/{proposal.md,tasks.md,design.md,specs/**/spec.md} : Scaffold each change with proposal.md, tasks.md, optional design.md, and delta spec files under openspec/changes/<change-id>/specs/<capability>/spec.md
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.mdopenspec/changes/archive/2025-11-06-add-shell-completions/tasks.mdopenspec/changes/archive/2025-11-06-add-shell-completions/proposal.md
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/specs/**/spec.md : Keep current, deployed specs in openspec/specs/<capability>/spec.md (one focused capability per directory)
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/changes/*/specs/**/spec.md : Use ADDED, MODIFIED, REMOVED, or RENAMED sections when authoring deltas; place changed requirements under the correct operation header
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/{specs/**/spec.md,changes/*/specs/**/spec.md} : Write requirements using SHALL/MUST for normative wording
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/changes/*/specs/**/spec.md : For MODIFIED requirements, paste the full existing requirement block (header through scenarios) and ensure the header text matches exactly (whitespace-insensitive)
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.md
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/changes/*/tasks.md : Track implementation steps in tasks.md and mark each completed item with '- [x]' once finished
Applied to files:
openspec/changes/archive/2025-11-06-add-shell-completions/tasks.md
🧬 Code graph analysis (14)
test/commands/completion.test.ts (2)
src/commands/completion.ts (1)
CompletionCommand(22-185)src/core/completions/installers/zsh-installer.ts (1)
ZshInstaller(23-412)
test/utils/shell-detection.test.ts (1)
src/utils/shell-detection.ts (2)
detectShell(11-48)SupportedShell(4-4)
scripts/postinstall.js (3)
src/utils/shell-detection.ts (1)
detectShell(11-48)src/core/completions/factory.ts (1)
CompletionFactory(21-74)src/core/completions/command-registry.ts (1)
COMMAND_REGISTRY(35-291)
src/core/completions/command-registry.ts (1)
src/core/completions/types.ts (2)
FlagDefinition(6-31)CommandDefinition(36-72)
src/cli/index.ts (2)
scripts/postinstall.js (1)
shell(127-127)src/commands/completion.ts (1)
CompletionCommand(22-185)
src/core/completions/completion-provider.ts (1)
src/utils/item-discovery.ts (1)
getActiveChangeIds(4-23)
src/commands/completion.ts (4)
scripts/postinstall.js (5)
shell(127-127)generator(79-79)script(80-80)installer(83-83)result(84-84)src/utils/shell-detection.ts (2)
SupportedShell(4-4)detectShell(11-48)src/core/completions/factory.ts (1)
CompletionFactory(21-74)src/core/completions/command-registry.ts (1)
COMMAND_REGISTRY(35-291)
src/core/completions/generators/zsh-generator.ts (2)
src/core/completions/types.ts (3)
CompletionGenerator(77-90)CommandDefinition(36-72)FlagDefinition(6-31)scripts/postinstall.js (1)
script(80-80)
test/core/completions/completion-provider.test.ts (1)
src/core/completions/completion-provider.ts (1)
CompletionProvider(16-128)
src/core/completions/factory.ts (3)
src/core/completions/installers/zsh-installer.ts (2)
InstallationResult(9-17)ZshInstaller(23-412)src/utils/shell-detection.ts (1)
SupportedShell(4-4)src/core/completions/types.ts (1)
CompletionGenerator(77-90)
test/core/completions/generators/zsh-generator.test.ts (3)
scripts/postinstall.js (2)
generator(79-79)script(80-80)src/core/completions/generators/zsh-generator.ts (1)
ZshGenerator(7-373)src/core/completions/types.ts (1)
CommandDefinition(36-72)
src/core/completions/types.ts (1)
src/utils/shell-detection.ts (1)
SupportedShell(4-4)
test/core/completions/installers/zsh-installer.test.ts (2)
scripts/postinstall.js (3)
installer(83-83)result(84-84)stat(44-44)src/core/completions/installers/zsh-installer.ts (2)
ZshInstaller(23-412)isInstalled(378-386)
src/core/completions/installers/zsh-installer.ts (2)
src/core/completions/factory.ts (1)
InstallationResult(15-15)src/utils/file-system.ts (1)
FileSystemUtils(44-187)
🔇 Additional comments (22)
openspec/changes/archive/2025-11-06-add-shell-completions/specs/cli-completion/spec.md (1)
1-301: LGTM! Well-structured specification.The specification follows all coding guidelines:
- Properly archived under the correct date-based path format
- Uses SHALL/MUST normative language throughout
- Organizes new capabilities under ADDED section
- Scenarios follow WHEN/THEN/AND structure
- Comprehensive coverage of requirements including native shell behavior, installation flows, error handling, and testing
The architecture patterns defined here (factory, generators, installers, command registry) align well with the implementation files in this PR.
Based on learnings
package.json (1)
48-51: LGTM! Postinstall integration follows best practices.The postinstall hook enables automatic shell completion installation after package install. The implementation (scripts/postinstall.js) includes:
- CI detection to skip in automated environments
OPENSPEC_NO_COMPLETIONS=1opt-out mechanism- Graceful failure that never breaks npm install
- Silent skips when dist/ doesn't exist (dev scenarios)
This aligns with the specification requirements for installation automation.
test/commands/completion.test.ts (3)
1-96: LGTM! Comprehensive test coverage for generate subcommand.The test suite properly covers:
- Explicit shell parameter
- Auto-detection scenarios
- Error cases (unsupported shells, detection failures)
- Case-insensitive shell handling
Mocking strategy is appropriate with shell-detection and ZshInstaller properly isolated.
98-153: LGTM! Install subcommand tests cover key scenarios.Tests validate:
- Explicit and auto-detected shell installation
- Verbose output mode
- Error handling for detection failures
- Unsupported shell errors
- Installation instruction display
196-268: LGTM! Error handling and integration tests are thorough.The tests cover:
- Installation/uninstallation failure scenarios with appropriate error messages
- Shell detection fallback behavior
- Explicit shell parameter precedence over auto-detection
Good use of dynamic imports and mock re-implementation for failure scenarios.
src/cli/index.ts (1)
16-16: LGTM! CLI integration follows consistent patterns.The completion command integration:
- Follows the same structure as existing commands (change, spec, validate)
- Properly passes options (shell, verbose) to CompletionCommand methods
- Uses consistent error handling with ora and process.exit(1)
- Subcommands align with specification requirements
Also applies to: 254-300
scripts/postinstall.js (1)
1-146: LGTM! Postinstall script follows best practices.The implementation demonstrates excellent postinstall hygiene:
- ✓ Never fails npm install (all errors caught, exits 0)
- ✓ Skips silently in CI environments
- ✓ Respects opt-out via
OPENSPEC_NO_COMPLETIONS=1- ✓ Guards against missing dist/ (dev setup scenario)
- ✓ Gracefully handles detection and installation failures
- ✓ Provides helpful tips when auto-install isn't possible
- ✓ Dynamic imports prevent crashes when modules unavailable
The success messages appropriately vary by installation type (Oh My Zsh vs. standard Zsh vs. manual configuration needed).
src/utils/shell-detection.ts (1)
1-48: LGTM! Shell detection logic handles common scenarios correctly.The implementation:
- Detects Unix shells (zsh, bash, fish) via $SHELL environment variable
- Handles PowerShell detection via PSModulePath (works for WSL scenarios)
- Returns undefined for unsupported shells (cmd.exe, tcsh, etc.)
- Uses case-insensitive matching to handle variations
The substring matching approach (e.g.,
shellName.includes('zsh')) is appropriate given standard shell path conventions.test/utils/shell-detection.test.ts (1)
1-158: LGTM! Comprehensive test coverage with proper isolation.The test suite thoroughly validates:
- Detection for zsh, bash, fish across multiple common paths
- Case-insensitive detection
- PowerShell detection via PSModulePath on both Windows and non-Windows
- Undefined behavior for unsupported shells and missing configuration
- Platform-specific logic (Windows vs. Unix-like)
Environment restoration (lines 10-43) ensures test isolation. The use of
Object.definePropertyfor mockingprocess.platformis appropriate.scripts/test-postinstall.sh (1)
1-57: LGTM! Well-designed integration test harness.The test script validates three critical postinstall scenarios:
- Normal installation flow
- CI environment skip (silent)
- Opt-out flag skip (silent)
Good practices:
set -efor strict error handling- Environment preservation and restoration
- Clear output with headers and expected behavior notes
- Tests the actual behavior rather than implementation details
src/core/completions/command-registry.ts (1)
1-291: LGTM! Well-structured command registry.The centralized registry follows clean architecture principles and provides excellent type safety. The COMMON_FLAGS pattern effectively reduces duplication.
test/core/completions/completion-provider.test.ts (1)
1-288: Excellent test coverage!The test suite comprehensively validates caching behavior, TTL expiration, cache invalidation, and data retrieval. The use of temporary directories with proper cleanup is good practice.
src/core/completions/types.ts (1)
1-90: LGTM! Clean and well-documented type definitions.The interfaces provide strong typing for the completion system with clear documentation. The positionalType union ensures type safety for dynamic completions.
src/core/completions/completion-provider.ts (1)
1-128: LGTM! Solid caching implementation.The CompletionProvider implements efficient 2-second TTL caching to balance performance with freshness during tab completion. The parallel fetching in
getAllIds()usingPromise.allis a good optimization.src/core/completions/factory.ts (1)
1-74: LGTM! Clean factory pattern with good extensibility.The factory pattern makes it straightforward to add support for additional shells in future PRs. The defensive copy in
getSupportedShells()and the type guard inisSupported()follow good practices.test/core/completions/generators/zsh-generator.test.ts (1)
1-381: Excellent test coverage!The test suite thoroughly validates the ZshGenerator implementation with comprehensive coverage of:
- Interface compliance
- Script structure and syntax
- Command, flag, and subcommand handling
- Positional argument types
- Special character escaping
- Edge cases and complex nested structures
src/core/completions/installers/zsh-installer.ts (6)
1-17: LGTM! Clean interface definition.The imports are appropriate and the
InstallationResultinterface is well-structured with clear optional fields for different installation scenarios.
23-36: LGTM! Good testability design.The constructor allows dependency injection of
homeDirfor testing while defaulting toos.homedir(). The marker-based configuration approach is a standard pattern for managing auto-generated config sections.
43-95: LGTM! Appropriate path detection and backup strategy.The path detection correctly distinguishes between Oh My Zsh and standard Zsh installations. The timestamped backup approach prevents overwriting existing backups and the graceful failure handling is appropriate for this use case.
128-157: LGTM! Robust configuration with appropriate safeguards.Good defensive programming with permission checks and the
OPENSPEC_NO_AUTO_CONFIGopt-out mechanism. The graceful failure approach ensures installation doesn't break if.zshrcconfiguration fails.
287-315: LGTM! Clear and actionable user instructions.The instructions appropriately differ between Oh My Zsh and standard Zsh setups, providing users with exactly the steps needed to enable completions.
322-411: LGTM! Comprehensive uninstallation and status checking.The uninstall logic correctly handles partial installations (script exists but no config, or vice versa) and provides accurate feedback. The status checking methods offer a clean API for querying installation state.
| - **WHEN** user executes `openspec completion --help` | ||
| - **THEN** display available subcommands: | ||
| - `zsh` - Generate Zsh completion script | ||
| - `install [shell]` - Install completion for Zsh (auto-detects or requires explicit shell) | ||
| - `uninstall [shell]` - Remove completion for Zsh (auto-detects or requires explicit shell) |
There was a problem hiding this comment.
Fix the documented subcommand list.
The CLI exposes completion generate/install/uninstall, not a zsh subcommand. The requirement should match the shipped interface to keep the spec authoritative. Please update the scenario accordingly.
[suggested diff]
- - `zsh` - Generate Zsh completion script
- - `install [shell]` - Install completion for Zsh (auto-detects or requires explicit shell)
- - `uninstall [shell]` - Remove completion for Zsh (auto-detects or requires explicit shell)
+ - `generate [shell]` - Output the completion script (defaults to detected shell)
+ - `install [shell]` - Install completion for Zsh (auto-detects or requires explicit shell)
+ - `uninstall [shell]` - Remove completion for Zsh (auto-detects or requires explicit shell)🤖 Prompt for AI Agents
In openspec/specs/cli-completion/spec.md around lines 31 to 35, the scenario
lists a non-existent `zsh` subcommand; update the expected subcommand list to
reflect the actual CLI interface `completion generate/install/uninstall` (and
describe their purposes, e.g., `generate` - produce completion script, `install
[shell]` - install for a shell, `uninstall [shell]` - remove for a shell).
Ensure the wording replaces `zsh` with `generate` and updates the bullet
descriptions to match the shipped commands.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/completions/generators/zsh-generator.ts (1)
328-343: Consider using SUPPORTED_SHELLS constant for shell completion.Line 339 hardcodes
'*:shell:(zsh)', but the PR description mentions the architecture is prepared for additional shells (bash, fish, PowerShell). Consider referencing theSUPPORTED_SHELLSconstant fromfactory.tsto keep the completion list in sync with supported shells.Example refactor:
import { SUPPORTED_SHELLS } from '../factory.js'; // In generatePositionalSpec: case 'shell': const shells = SUPPORTED_SHELLS.join(' '); return `'*:shell:(${shells})'`;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/completions/generators/zsh-generator.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/completions/generators/zsh-generator.ts (2)
src/core/completions/types.ts (3)
CompletionGenerator(77-90)CommandDefinition(36-72)FlagDefinition(6-31)scripts/postinstall.js (1)
script(80-80)
🔇 Additional comments (9)
src/core/completions/generators/zsh-generator.ts (9)
1-8: LGTM!The class correctly implements the
CompletionGeneratorinterface, and the const assertion on theshellproperty provides proper type safety.
16-83: Excellent fix from previous review!The completion script structure correctly follows Zsh conventions. The critical issue from the previous review has been properly addressed—line 79 now uses
compdef _openspec openspecfor registration instead of invoking_openspec "$@"during load, which prevents errors when sourcing the completion file.
94-126: LGTM!The helper correctly generates Zsh completion functions with proper handling of single and multi-line command arrays.
131-173: LGTM!The dynamic completion helpers correctly use command substitution with
openspec listand awk to format change and spec IDs with descriptions. The Zsh parameter expansion syntax${(f)"..."}properly splits the output into array elements.
178-257: LGTM with minor edge case.The function correctly handles both commands with and without subcommands, properly generating Zsh completion logic. The trailing backslash removal (lines 240-242) is necessary for valid Zsh syntax.
Edge case: If a command has neither flags nor positionals, the result would be
_argumentswith no arguments, which is invalid Zsh syntax. However, this is unlikely in practice as commands typically have at least one flag.
262-288: LGTM!The subcommand function generation follows the same pattern as command functions with consistent handling of flags and positionals.
293-323: LGTM!The flag specification generation correctly implements Zsh syntax for both mutually exclusive short/long forms and value completion.
348-365: LGTM!The escaping methods correctly handle special characters for Zsh completion syntax, with backslash properly escaped first to avoid double-escaping issues.
370-372: LGTM!The function correctly sanitizes command names for use in Zsh function identifiers by replacing dashes with underscores.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Add __complete endpoint with tab-separated output for safe parsing - Replace brittle awk parsing with __complete in completion helpers - Add uninstall confirmation prompt with --yes flag to skip - Prefer $ZSH env var for Oh My Zsh detection before dir check - Add fpath verification guidance for OMZ installations - Update cli-completion spec to document generate subcommand
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/item-discovery.ts (1)
4-65: Consider extracting common discovery logic to reduce duplication.All three functions (
getActiveChangeIds,getSpecIds,getArchivedChangeIds) share nearly identical logic—they differ only in the base path, validation file name, and filter criteria. Consider refactoring into a parameterized helper function:async function discoverItems( basePath: string, validationFile: string, excludeDirs: string[] = [] ): Promise<string[]> { try { const entries = await fs.readdir(basePath, { withFileTypes: true }); const result: string[] = []; for (const entry of entries) { if (!entry.isDirectory() || entry.name.startsWith('.') || excludeDirs.includes(entry.name)) continue; const filePath = path.join(basePath, entry.name, validationFile); try { await fs.access(filePath); result.push(entry.name); } catch { // skip directories without validation file } } return result.sort(); } catch { return []; } } export async function getActiveChangeIds(root: string = process.cwd()): Promise<string[]> { return discoverItems( path.join(root, 'openspec', 'changes'), 'proposal.md', ['archive'] ); } export async function getSpecIds(root: string = process.cwd()): Promise<string[]> { return discoverItems( path.join(root, 'openspec', 'specs'), 'spec.md' ); } export async function getArchivedChangeIds(root: string = process.cwd()): Promise<string[]> { return discoverItems( path.join(root, 'openspec', 'changes', 'archive'), 'proposal.md' ); }This would also help consolidate similar logic found in
src/commands/change.ts(getActiveChangesmethod).src/core/completions/installers/zsh-installer.ts (1)
186-233: Consider using FileSystemUtils for marker removal.The
removeZshrcConfigmethod manually implements marker-based content removal, which duplicates some logic fromFileSystemUtils.updateFileWithMarkers. If FileSystemUtils provides a method to remove marker-enclosed content (or ifupdateFileWithMarkerscan be extended to handle removal by passing empty content), consider using it to reduce code duplication and ensure consistent marker handling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/cli/index.ts(2 hunks)src/commands/completion.ts(1 hunks)src/core/completions/generators/zsh-generator.ts(1 hunks)src/core/completions/installers/zsh-installer.ts(1 hunks)src/utils/item-discovery.ts(1 hunks)test/commands/completion.test.ts(1 hunks)test/core/completions/generators/zsh-generator.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/core/completions/generators/zsh-generator.test.ts
- src/commands/completion.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-22T06:05:06.615Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-10-22T06:05:06.615Z
Learning: Applies to openspec/changes/archive/** : Archive completed changes under openspec/changes/archive/YYYY-MM-DD-<name>/ and run 'openspec validate --strict' post-archive; update specs/ if capabilities changed
Applied to files:
src/utils/item-discovery.ts
📚 Learning: 2025-10-21T03:05:07.856Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-21T03:05:07.856Z
Learning: Always open @/openspec/AGENTS.md when the request is ambiguous and the authoritative spec is needed before coding
Applied to files:
src/core/completions/generators/zsh-generator.ts
🧬 Code graph analysis (5)
src/utils/item-discovery.ts (2)
src/commands/change.ts (1)
getActiveChanges(241-259)src/core/view.ts (1)
getChangesData(71-110)
src/cli/index.ts (2)
scripts/postinstall.js (1)
shell(127-127)src/commands/completion.ts (1)
CompletionCommand(30-254)
src/core/completions/generators/zsh-generator.ts (2)
src/core/completions/types.ts (3)
CompletionGenerator(77-90)CommandDefinition(36-72)FlagDefinition(6-31)scripts/postinstall.js (1)
script(80-80)
src/core/completions/installers/zsh-installer.ts (2)
src/core/completions/factory.ts (1)
InstallationResult(15-15)src/utils/file-system.ts (1)
FileSystemUtils(44-187)
test/commands/completion.test.ts (2)
src/commands/completion.ts (1)
CompletionCommand(30-254)src/core/completions/installers/zsh-installer.ts (1)
ZshInstaller(23-442)
🔇 Additional comments (1)
src/utils/item-discovery.ts (1)
46-65: LGTM! Logic is correct and consistent.The function correctly discovers archived change IDs by scanning the archive directory, validating the presence of
proposal.md, and returning a sorted list. The error handling and filtering logic align perfectly with the existinggetActiveChangeIdsandgetSpecIdsfunctions.
|
@TabishB Hi, this PR is abit big partly due to the specs used |
- Return structured result from detectShell() with shell and detected name - Detect already-installed completions and skip reinstall - Add update detection with automatic backup of previous version - Add debug logging to silent catch blocks for diagnostics - Quote fpath directories to handle paths with spaces - Verify Oh My Zsh fpath configuration and add to .zshrc if needed - Show helpful error for detected but unsupported shells - Update all tests for new detection API
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/core/completions/installers/zsh-installer.test.ts (1)
177-194: Consider usingbeforeEach/afterEachfor environment variable management.The manual save/restore pattern for
OPENSPEC_NO_AUTO_CONFIGworks but is error-prone if an assertion fails before restoration. Consider extracting this to a helper or using nesteddescribeblocks with dedicated setup/teardown for tests that modify environment variables.+ describe('with OPENSPEC_NO_AUTO_CONFIG=1', () => { + let originalEnv: string | undefined; + + beforeEach(() => { + originalEnv = process.env.OPENSPEC_NO_AUTO_CONFIG; + process.env.OPENSPEC_NO_AUTO_CONFIG = '1'; + }); + + afterEach(() => { + if (originalEnv === undefined) { + delete process.env.OPENSPEC_NO_AUTO_CONFIG; + } else { + process.env.OPENSPEC_NO_AUTO_CONFIG = originalEnv; + } + }); + + it('should include fpath instructions for standard Zsh when auto-config is disabled', async () => { + const result = await installer.install(testScript); + + expect(result.instructions).toBeDefined(); + expect(result.instructions!.join('\n')).toContain('fpath'); + expect(result.instructions!.join('\n')).toContain('.zshrc'); + expect(result.instructions!.join('\n')).toContain('compinit'); + }); + });test/utils/shell-detection.test.ts (2)
40-42: Addconfigurable: trueto platform property descriptor.Without
configurable: true, subsequentObject.definePropertycalls onprocess.platformmay fail in some Node.js versions. This affects both the restore inafterEachand any later tests that mock the platform.Object.defineProperty(process, 'platform', { value: originalPlatform, + configurable: true, });Apply the same change to all other
Object.definePropertycalls forprocess.platform(lines 137-139, 154-156, 163-165).
179-184: Limited value in type test.This test only verifies array length, which doesn't test the TypeScript type constraint at runtime. Consider removing this test or adding a compile-time type assertion test using
expectTypeOffrom vitest if available.describe('SupportedShell type', () => { - it('should accept valid shell types', () => { - const shells: SupportedShell[] = ['zsh', 'bash', 'fish', 'powershell']; - expect(shells).toHaveLength(4); - }); + it('should accept valid shell types', () => { + // Type-level test: this will fail at compile time if types don't match + const shells: SupportedShell[] = ['zsh', 'bash', 'fish', 'powershell']; + expect(shells).toEqual(['zsh', 'bash', 'fish', 'powershell']); + }); });scripts/postinstall.js (2)
24-36: Consider extending CI detection.The current CI detection checks only
CI=trueandCI=1. Many CI systems setCIto other truthy values or use different environment variables.function shouldSkipInstallation() { // Skip in CI environments - if (process.env.CI === 'true' || process.env.CI === '1') { + if (process.env.CI) { return { skip: true, reason: 'CI environment detected' }; }This matches common practice where any non-empty
CIvalue indicates a CI environment.
89-99: Hardcoded Zsh-specific messages may be misleading for future shells.The success messages (lines 91-98) are hardcoded for Zsh (
exec zsh,~/.zsh/completions/,~/.zshrc). When support for other shells is added, these messages will be incorrect. Consider using theresultobject's data or theshellparameter to generate appropriate messages.if (result.success) { // Show success message based on installation type if (result.isOhMyZsh) { console.log(`✓ Shell completions installed`); - console.log(` Restart shell: exec zsh`); + console.log(` Restart shell: exec ${shell}`); } else if (result.zshrcConfigured) { console.log(`✓ Shell completions installed and configured`); - console.log(` Restart shell: exec zsh`); + console.log(` Restart shell: exec ${shell}`); } else { - console.log(`✓ Shell completions installed to ~/.zsh/completions/`); - console.log(` Add to ~/.zshrc: fpath=(~/.zsh/completions $fpath)`); - console.log(` Then: exec zsh`); + console.log(`✓ Shell completions installed to ${result.installedPath}`); + if (result.instructions) { + result.instructions.forEach(i => console.log(` ${i}`)); + } }src/commands/completion.ts (1)
181-186: Hardcoded Zsh reference in confirmation prompt.The confirmation message mentions
~/.zshrcspecifically, which won't be accurate when other shells are supported.if (!skipConfirmation) { const confirmed = await confirm({ - message: 'Remove OpenSpec configuration from ~/.zshrc?', + message: `Remove OpenSpec ${shell} completion configuration?`, default: false, });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
scripts/postinstall.js(1 hunks)src/commands/completion.ts(1 hunks)src/core/completions/installers/zsh-installer.ts(1 hunks)src/utils/shell-detection.ts(1 hunks)test/commands/completion.test.ts(1 hunks)test/core/completions/installers/zsh-installer.test.ts(1 hunks)test/utils/shell-detection.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/commands/completion.test.ts
- src/core/completions/installers/zsh-installer.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/utils/shell-detection.test.ts (1)
src/utils/shell-detection.ts (2)
detectShell(21-62)SupportedShell(4-4)
scripts/postinstall.js (3)
src/utils/shell-detection.ts (1)
detectShell(21-62)src/core/completions/factory.ts (1)
CompletionFactory(21-74)src/core/completions/command-registry.ts (1)
COMMAND_REGISTRY(35-291)
src/commands/completion.ts (4)
src/core/completions/completion-provider.ts (1)
CompletionProvider(16-128)src/utils/shell-detection.ts (2)
SupportedShell(4-4)detectShell(21-62)src/core/completions/command-registry.ts (1)
COMMAND_REGISTRY(35-291)src/utils/item-discovery.ts (1)
getArchivedChangeIds(46-65)
test/core/completions/installers/zsh-installer.test.ts (1)
src/core/completions/installers/zsh-installer.ts (2)
ZshInstaller(23-507)isInstalled(473-481)
🔇 Additional comments (16)
test/core/completions/installers/zsh-installer.test.ts (6)
1-22: Good test setup with isolated temp directories.The
beforeEach/afterEachpattern properly creates and cleans up isolated test directories usingrandomUUID(), preventing test pollution.
196-204: LGTM for error handling test.Testing with an inaccessible path to verify graceful failure is appropriate. The implementation correctly returns
{ success: false }without throwing.
246-269: Good coverage for paths with spaces.This test properly validates that paths containing spaces are quoted correctly in
.zshrcconfiguration, which is important for shell compatibility.
272-313: Comprehensive uninstall test coverage.Tests properly verify both successful removal and failure cases, including Oh My Zsh-specific path handling.
389-512: Thorough.zshrcconfiguration tests.Good coverage of marker-based configuration management, including creation, updates, and preservation of user content outside markers.
585-594: Good defensive test for invalid marker placement.Testing the case where END marker appears before START marker ensures the implementation handles corrupted configurations gracefully.
test/utils/shell-detection.test.ts (3)
10-43: Good environment isolation pattern.The
beforeEach/afterEachpattern properly saves and restores all relevant environment variables andprocess.platform, preventing test pollution.
53-67: Good coverage of shell path variations.Testing multiple common installation paths for each shell ensures the detection works across different system configurations.
129-144: PowerShell detection coverage looks good.Tests properly verify both
PSModulePathpresence and Windows platform scenarios.scripts/postinstall.js (1)
143-147: Appropriate fail-safe pattern for postinstall.Using
main().catch(() => process.exit(0))ensures the npm install never fails due to completion installation issues, which is the correct behavior for an optional enhancement.src/utils/shell-detection.ts (3)
1-14: Clean type definitions.The
SupportedShelltype andShellDetectionResultinterface provide good type safety for shell detection consumers.
25-36: Shell detection usingincludes()is pragmatic.The substring matching approach works for common shell paths. Edge cases like
/usr/bin/zsh-5.8(versioned binaries) will correctly match. The approach is reasonable for the expected use cases.
45-59: Windows detection logic is sound.Using
PSModulePathas the primary PowerShell indicator is reliable. The fallback toCOMSPECfor cmd.exe detection properly handles the unsupported case.src/commands/completion.ts (3)
43-75: Past review feedback addressed.The
resolveShellOrExitmethod now properly distinguishes between:
- Detected but unsupported shell (lines 54-58): Shows "Shell '' is not supported yet"
- Could not detect shell (lines 60-65): Shows "Could not auto-detect shell"
This addresses the previous reviewer's feedback about providing better messaging for detected-but-unsupported shells.
219-254: Clean machine-readable output for shell completions.The
complete()method properly outputs tab-separated data and silently fails with exit code 1 for invalid types, which is appropriate for shell completion scripts that shouldn't produce error messages.
125-172: Well-structured installation flow with good UX.The installation method provides clear feedback via spinner, supports verbose output for debugging, and handles success/failure cases appropriately with actionable instructions.
2 participants
Add Zsh Shell Completions
Adds tab completion support for the OpenSpec CLI with Zsh (including Oh My Zsh).
New Command
openspec completion install # Auto-installs Zsh completions
openspec completion uninstall # Removes completions
openspec completion generate # Outputs completion script
Key Files
Core Implementation
Zsh-Specific
/.oh-my-zsh/custom/completions/) or standard Zsh (/.zsh/completions/)Command Interface
Features
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.