Bugfix/issue #122 fish shell support#125
Bugfix/issue #122 fish shell support#125stephenfeather wants to merge 6 commits intoparcadei:mainfrom
Conversation
…zard - Detect fish shell and set CLAUDE_OPC_DIR/LOOGLE_HOME using fish-compatible syntax - Added a verification step at the end of the wizard to check for CLAUDE_OPC_DIR - Updated math features guide with fish instructions Closes parcadei#122
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR adds YAML front matter metadata (name and description fields) to 17 skill definition files and refactors shell configuration handling in setup scripts. The wizard script introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
opc/scripts/setup/wizard.py (2)
876-881: Fallback message uses incorrect syntax for fish shell.When the fish config file doesn't exist (e.g., fresh fish install without
~/.config/fish/), the fallback showsexportsyntax which won't work in fish. Consider usingshell_typeto show the correct syntax.Proposed fix
elif sys.platform == "win32": console.print(" [yellow]NOTE[/yellow] Add to your environment:") console.print(f' set CLAUDE_OPC_DIR="{opc_dir}"') else: - console.print(" [yellow]NOTE[/yellow] Add to your shell config:") - console.print(f' export CLAUDE_OPC_DIR="{opc_dir}"') + if shell_type == "fish": + console.print(" [yellow]NOTE[/yellow] Add to your fish config:") + console.print(f' set -gx CLAUDE_OPC_DIR "{opc_dir}"') + else: + console.print(" [yellow]NOTE[/yellow] Add to your shell config:") + console.print(f' export CLAUDE_OPC_DIR="{opc_dir}"')
1235-1240: Same fallback syntax issue for fish shell.The fallback message at line 1240 shows
exportsyntax which won't work for fish users whose config directory doesn't exist yet. Apply the same fix as suggested for the CLAUDE_OPC_DIR fallback.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| shell_config, _ = get_shell_config() | ||
| if shell_config: | ||
| console.print(f" 1. Run: [bold]source {shell_config}[/bold]") |
There was a problem hiding this comment.
source doesn't work with fish shell. For fish, the instruction should be different (fish uses just the path without source, or can use . for POSIX scripts).
| console.print(f" 1. Run: [bold]source {shell_config}[/bold]") | |
| console.print(f" 1. Restart your terminal or reload config") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: opc/scripts/setup/wizard.py
Line: 1279:1279
Comment:
`source` doesn't work with fish shell. For fish, the instruction should be different (fish uses just the path without `source`, or can use `.` for POSIX scripts).
```suggestion
console.print(f" 1. Restart your terminal or reload config")
```
How can I resolve this? If you propose a fix, please make it concise.| if "fish" in shell: | ||
| # Standard location for fish config | ||
| return Path.home() / ".config" / "fish" / "config.fish", "fish" |
There was a problem hiding this comment.
Fish config directory ~/.config/fish/ may not exist. Should create it before attempting to write to config.fish, otherwise the write operation will fail.
Add before line 863 and 1222:
if shell_type == "fish" and shell_config:
shell_config.parent.mkdir(parents=True, exist_ok=True)Prompt To Fix With AI
This is a comment left during a code review.
Path: opc/scripts/setup/wizard.py
Line: 519:521
Comment:
Fish config directory `~/.config/fish/` may not exist. Should create it before attempting to write to `config.fish`, otherwise the write operation will fail.
Add before line 863 and 1222:
```python
if shell_type == "fish" and shell_config:
shell_config.parent.mkdir(parents=True, exist_ok=True)
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
@MauroToscano Please test
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.
Greptile Overview
Greptile Summary
This PR adds fish shell support to the setup wizard by:
$SHELLenvironment variable in newget_shell_config()functionset -gxinstead ofexport) forCLAUDE_OPC_DIRandLOOGLE_HOMEenvironment variablesCLAUDE_OPC_DIRis setCritical Issues Found:
sourcecommand which doesn't work correctly with fish shell'sconfig.fish~/.config/fish/before writing toconfig.fish(will fail on fresh fish installations)Recommendation:
The fish shell support implementation is on the right track but needs the two logic bugs fixed before merge. Additionally, tests should be added to verify shell detection and config file writing work correctly for fish, bash, and zsh.
Confidence Score: 2/5
sourcecommand for fish shell which won't work properly, and (2) missing directory creation will cause writes to fail on fresh fish installations. Additionally, no tests were provided despite explicit repository requirement.opc/scripts/setup/wizard.py- it contains the two critical bugs that need fixingImportant Files Changed
Sequence Diagram
sequenceDiagram participant User participant Wizard as setup/wizard.py participant ShellConfig as get_shell_config() participant FileSystem as Shell Config File User->>Wizard: Run setup wizard Wizard->>Wizard: Setup steps 1-8 Note over Wizard: Step 9: Set CLAUDE_OPC_DIR Wizard->>ShellConfig: Get shell config path & type ShellConfig->>ShellConfig: Check $SHELL env var alt Fish Shell ShellConfig-->>Wizard: (~/.config/fish/config.fish, "fish") else Zsh Shell ShellConfig-->>Wizard: (~/.zshrc, "zsh") else Bash Shell ShellConfig-->>Wizard: (~/.bashrc, "bash") else Unknown ShellConfig-->>Wizard: (None, "unknown") end alt Shell config exists Wizard->>FileSystem: Read config file FileSystem-->>Wizard: Current content alt Fish shell Wizard->>FileSystem: Append "set -gx CLAUDE_OPC_DIR <path>" else Bash/Zsh Wizard->>FileSystem: Append "export CLAUDE_OPC_DIR=<path>" end else Config missing Wizard->>User: Show manual setup instructions end Note over Wizard: Optional: Loogle Setup alt Loogle installed Wizard->>ShellConfig: Get shell config again Wizard->>FileSystem: Append LOOGLE_HOME (fish or bash/zsh syntax) end Note over Wizard: Verification Step Wizard->>Wizard: Check if CLAUDE_OPC_DIR in current env alt Not set in env Wizard->>User: WARNING - suggest source config (BUG: doesn't work for fish) end Wizard->>User: Setup complete!(4/5) You can add custom instructions or style guidelines for the agent here!