feat(fish): add clrc command for stable claude remote-control - #1204
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request modifies the OpenClaw configuration hydration logic by replacing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Mesa DescriptionTL;DRAdds a What changed?
Description generated by Mesa. Update settings |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the development environment by introducing a robust Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
d0ac220 to
3347d35
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new clrc fish command for running Claude remote-control in a way that's resilient to updates, which is a nice enhancement. The implementation is straightforward and includes tests. The PR also includes some good refactoring of Nix files to use pkgs.writeText instead of pkgs.replaceVars, which aligns with modern Nix practices and avoids warnings. Additionally, a fix to a test for _ssh_add_github makes the test more robust. My main feedback is on the new test file for clrc, where the test structure could be simplified and made more robust. Overall, these are solid improvements.
| # ── basic: resolves symlink and runs node with remote-control ── | ||
| set log1 (mktemp) | ||
| set fake_cli (mktemp) | ||
|
|
||
| function which; echo $fake_cli; end | ||
| function realpath; echo $argv[1]; end | ||
| function node; echo "node" $argv >> $log1; end | ||
|
|
||
| _clrc_function | ||
|
|
||
| @test "calls node directly (not claude symlink)" (grep -c "^node" $log1) -ge 1 | ||
| @test "passes remote-control subcommand" (grep -c "remote-control" $log1) -ge 1 | ||
|
|
||
| # ── with args: passes through extra args ────────────────────── | ||
| set log2 (mktemp) | ||
| function node; echo "node" $argv >> $log2; end | ||
|
|
||
| _clrc_function --name mysession | ||
|
|
||
| @test "passes extra args through" (grep -c -- "--name" $log2) -ge 1 | ||
| @test "still includes remote-control with args" (grep -c "remote-control" $log2) -ge 1 | ||
|
|
||
| rm -f $log1 $log2 $fake_cli |
There was a problem hiding this comment.
The test structure can be improved for better isolation and clarity. Redefining the node mock and using separate log files for each test case makes the test suite more complex and potentially brittle. A cleaner approach is to use a single set of mocks and a single log file, clearing the log between test cases. This improves readability and maintainability.
Additionally, the tests can be made more specific by checking for an exact count (-eq 1) instead of a minimum count (-ge 1), and it's good practice to clean up mocked functions after the tests complete.
# ── setup ─────────────────────────────────────────────────────
set -l log (mktemp)
set -l fake_cli (mktemp)
# Mock dependencies
function which; echo $fake_cli; end
function realpath; echo $argv[1]; end
function node; echo "node" $argv >> $log; end
# ── basic: resolves symlink and runs node with remote-control ──
_clrc_function
@test "calls node directly (not claude symlink)" (grep -c "^node" $log) -eq 1
@test "passes remote-control subcommand" (grep -c "remote-control" $log) -eq 1
> $log # Clear log for next test
# ── with args: passes through extra args ──────────────────────
_clrc_function --name mysession
@test "passes extra args through" (grep -c -- "--name" $log) -eq 1
@test "still includes remote-control with args" (grep -c "remote-control" $log) -eq 1
# ── teardown ──────────────────────────────────────────────────
functions -e which realpath node
rm -f $log $fake_cli
23716c3 to
5d7c8b8
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/programs/fish/functions/_clrc_function.fish">
<violation number="1" location="home-manager/programs/fish/functions/_clrc_function.fish:7">
P2: Resolve `claude` with a path-only lookup. `which` can be shadowed by a fish function, causing `realpath` to receive non-path output and breaking `clrc`.</violation>
</file>
<file name="spec/fish/_clrc_function_test.fish">
<violation number="1" location="spec/fish/_clrc_function_test.fish:9">
P2: This test never distinguishes `realpath (which claude)` from plain `which claude`, so it can pass even if the symlink-resolution behavior regresses.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # keeps its reference to the old inode and is unaffected. | ||
| # Usage: clrc [<claude remote-control args...>] | ||
|
|
||
| set -l claude_real (realpath (which claude)) |
There was a problem hiding this comment.
P2: Resolve claude with a path-only lookup. which can be shadowed by a fish function, causing realpath to receive non-path output and breaking clrc.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/fish/functions/_clrc_function.fish, line 7:
<comment>Resolve `claude` with a path-only lookup. `which` can be shadowed by a fish function, causing `realpath` to receive non-path output and breaking `clrc`.</comment>
<file context>
@@ -0,0 +1,9 @@
+ # keeps its reference to the old inode and is unaffected.
+ # Usage: clrc [<claude remote-control args...>]
+
+ set -l claude_real (realpath (which claude))
+ node $claude_real remote-control $argv
+end
</file context>
| set fake_cli (mktemp) | ||
|
|
||
| function which; echo $fake_cli; end | ||
| function realpath; echo $argv[1]; end |
There was a problem hiding this comment.
P2: This test never distinguishes realpath (which claude) from plain which claude, so it can pass even if the symlink-resolution behavior regresses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/fish/_clrc_function_test.fish, line 9:
<comment>This test never distinguishes `realpath (which claude)` from plain `which claude`, so it can pass even if the symlink-resolution behavior regresses.</comment>
<file context>
@@ -0,0 +1,26 @@
+set fake_cli (mktemp)
+
+function which; echo $fake_cli; end
+function realpath; echo $argv[1]; end
+function node; echo "node" $argv >> $log1; end
+
</file context>
Resolves bun-update staleness by invoking node directly on the resolved cli.js inode, so running sessions survive bun replacing the binary. Also fixes _ssh_add_github test to use PATH filtering instead of erasing a fish function (which didn't hide the real keychain binary on PATH).
5d7c8b8 to
45acd9d
Compare
There was a problem hiding this comment.
Pull request overview
Adds a Fish helper for running claude remote-control via a stable file handle (for resilience across bun updates), and updates Fish tests + Nix hydration scripts to reduce warnings and fix a test edge case.
Changes:
- Add
_clrc_functionFish function and a new spec to validate argument passthrough and node invocation. - Fix
_ssh_add_githubtest setup by filteringkeychainout ofPATHrather than trying to shadow/erase a function. - Replace
pkgs.replaceVarswithwriteText+builtins.replaceStringsfor hydration scripts; suppress a Home Manager manual warning.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/fish/_ssh_add_github_test.fish | Adjusts test setup to remove keychain from PATH reliably. |
| spec/fish/_clrc_function_test.fish | Adds coverage for _clrc_function behavior (node invocation + arg passthrough). |
| home-manager/programs/fish/functions/_clrc_function.fish | Implements _clrc_function to resolve claude’s real path and invoke node ... remote-control. |
| home-manager/default.nix | Disables Home Manager manpages generation (comment says options.json warning suppression). |
| config/openclaw/default.nix | Reworks hydration script generation using writeText + replaceStrings. |
| config/ccs/default.nix | Reworks hydration script generation using writeText + replaceStrings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| function which; echo $fake_cli; end | ||
| function realpath; echo $argv[1]; end | ||
| function node; echo "node" $argv >> $log1; end |
|
|
||
| # ── with args: passes through extra args ────────────────────── | ||
| set log2 (mktemp) | ||
| function node; echo "node" $argv >> $log2; end |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/fish/_clrc_function_test.fish (1)
14-15: Strengthen the “not claude symlink” assertion to match intent.Current checks only prove
noderan andremote-controlis present. Add an explicit assertion that the resolved path ($fake_cli) is passed tonode.✅ Suggested test additions
`@test` "calls node directly (not claude symlink)" (grep -c "^node" $log1) -ge 1 `@test` "passes remote-control subcommand" (grep -c "remote-control" $log1) -ge 1 +@test "passes resolved claude path to node" (grep -F -c -- "$fake_cli" $log1) -ge 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/fish/_clrc_function_test.fish` around lines 14 - 15, Update the "calls node directly (not claude symlink)" test to assert that the resolved CLI path ($fake_cli) is passed to node: instead of only checking for a "^node" line in $log1, add/replace an assertion that the node invocation in $log1 contains the literal $fake_cli (e.g., grep for $fake_cli in $log1) so the test ensures node was invoked with the resolved $fake_cli path; keep the existing "passes remote-control subcommand" check unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/programs/fish/functions/_clrc_function.fish`:
- Around line 7-8: The current _clrc_function.fish computes claude_real with
"set -l claude_real (realpath (which claude))" then blindly runs "node
$claude_real remote-control $argv", which will call node without a script if
which/realpath fail; update _clrc_function.fish to guard after computing
claude_real with "if test -z \"$claude_real\"; echo 'Error: claude not found'
>&2; return 1; end" (or similar) so you early-return on missing/empty
claude_real, keeping the descriptive stderr message; also add spec tests
covering both failure modes (simulate missing claude and failing realpath)
asserting the function returns non-zero and emits the error message.
---
Nitpick comments:
In `@spec/fish/_clrc_function_test.fish`:
- Around line 14-15: Update the "calls node directly (not claude symlink)" test
to assert that the resolved CLI path ($fake_cli) is passed to node: instead of
only checking for a "^node" line in $log1, add/replace an assertion that the
node invocation in $log1 contains the literal $fake_cli (e.g., grep for
$fake_cli in $log1) so the test ensures node was invoked with the resolved
$fake_cli path; keep the existing "passes remote-control subcommand" check
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fc83306-061d-4d1e-aab0-685bc0dcd85d
📒 Files selected for processing (4)
config/openclaw/default.nixhome-manager/programs/fish/functions/_clrc_function.fishspec/fish/_clrc_function_test.fishspec/fish/_ssh_add_github_test.fish
| set -l claude_real (realpath (which claude)) | ||
| node $claude_real remote-control $argv |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect current clrc resolution flow and related tests.
rg -n -C3 'which claude|realpath|node \$claude_real remote-control' home-manager/programs/fish/functions/_clrc_function.fish
rg -n -C3 '_clrc_function|remote-control|which|realpath|node' spec/fish/_clrc_function_test.fishRepository: shunkakinoki/dotfiles
Length of output: 1200
🏁 Script executed:
#!/bin/bash
# Verify Fish shell behavior for failed command substitution
# Check if there are similar error-handling patterns elsewhere in the codebase
# 1. Test Fish behavior when which fails
fish -c 'which nonexistent_command_xyz; echo "Exit code: $status"'
# 2. Look for other error handling patterns in Fish functions
fd -t f '\.fish$' home-manager/programs/fish/functions/ -x grep -l 'if.*test' {} \; | head -5
rg -n 'if.*test.*-z|if.*which|if.*realpath' home-manager/programs/fish/functions/ -A 2 | head -30
# 3. Check spec file for error case tests
cat spec/fish/_clrc_function_test.fishRepository: shunkakinoki/dotfiles
Length of output: 3156
Add guards for which and realpath failures before invoking node.
If which claude or realpath fail, $claude_real becomes empty and Line 8 executes node remote-control $argv (missing the script path). The codebase establishes a consistent error-handling pattern elsewhere (e.g., _ocxeh_function.fish, _tsk_function.fish) using if test -z "$var" checks with early returns. Apply the same pattern here. Additionally, the spec file has no tests for error cases; the proposed fix should include tests for missing claude and realpath failure scenarios.
Proposed fix
- set -l claude_real (realpath (which claude))
- node $claude_real remote-control $argv
+ set -l claude_bin (which claude 2>/dev/null)
+ if test -z "$claude_bin"
+ echo "clrc: claude not found in PATH" >&2
+ return 127
+ end
+
+ set -l claude_real (realpath $claude_bin 2>/dev/null)
+ if test -z "$claude_real"
+ echo "clrc: failed to resolve claude path: $claude_bin" >&2
+ return 1
+ end
+
+ node $claude_real remote-control $argv📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set -l claude_real (realpath (which claude)) | |
| node $claude_real remote-control $argv | |
| set -l claude_bin (which claude 2>/dev/null) | |
| if test -z "$claude_bin" | |
| echo "clrc: claude not found in PATH" >&2 | |
| return 127 | |
| end | |
| set -l claude_real (realpath $claude_bin 2>/dev/null) | |
| if test -z "$claude_real" | |
| echo "clrc: failed to resolve claude path: $claude_bin" >&2 | |
| return 1 | |
| end | |
| node $claude_real remote-control $argv |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/fish/functions/_clrc_function.fish` around lines 7 - 8,
The current _clrc_function.fish computes claude_real with "set -l claude_real
(realpath (which claude))" then blindly runs "node $claude_real remote-control
$argv", which will call node without a script if which/realpath fail; update
_clrc_function.fish to guard after computing claude_real with "if test -z
\"$claude_real\"; echo 'Error: claude not found' >&2; return 1; end" (or
similar) so you early-return on missing/empty claude_real, keeping the
descriptive stderr message; also add spec tests covering both failure modes
(simulate missing claude and failing realpath) asserting the function returns
non-zero and emits the error message.
Summary
clrcfish abbreviation that runsclaude remote-controlby invokingnodedirectly on the resolvedcli.jsinode (viarealpath $(which claude))_ssh_add_github_test.fishwherekeychainwas shadowed by erasing a fish function, but the real binary on PATH was still found bycommand -v; fix uses PATH filtering insteadTest plan
make fish-testpasses 202/202clrc— runs remote-control with a stable binary handleclrc --name mysession --spawn worktree— args pass through correctlySummary by cubic
Adds a
clrcfish command that runsclaude remote-controlvianodeon the resolvedclaudepath to keep sessions stable acrossbunupgrades. Also updates the OpenClaw Nix hydrate script to avoid context warnings and formats fortreefmt.New Features
clrc: runsnode (realpath (which claude)) remote-control ...for stability; tests cover basic run and arg passthrough.Bug Fixes
_ssh_add_github_test.fish: filterPATHsokeychainis correctly treated as missing.config/openclaw/default.nix: rewrite hydrate script withwriteText/replaceStringsto avoid Nix context warnings; applytreefmtformatting.Written for commit 44ad1a8. Summary will update on new commits.