Repository navigation
Ensure shell integrations always dispatch claude through the bundled wrapper - #2465
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughDefines a shell-level Claude dispatch in bash and zsh integration: locate the bundled Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UserShell as "User Shell"
participant ShellFunc as "claude() (shell function)"
participant Bundled as "Resources/bin/claude (wrapper)"
participant RealBin as "real claude (PATH)"
UserShell->>ShellFunc: User runs `claude arg1 arg2`
ShellFunc->>Bundled: exec "$_CMUX_CLAUDE_WRAPPER" "$@"
Bundled->>RealBin: (may forward to real binary or handle)
RealBin-->>Bundled: exit/result
Bundled-->>ShellFunc: exit/result
ShellFunc-->>UserShell: exit/result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…-2448-cmux-claude-wrapper # Conflicts: # Resources/shell-integration/cmux-bash-integration.bash
Greptile SummaryThis PR fixes issue #2448 by installing a Key points:
Confidence Score: 5/5Safe to merge — the fix is correct, well-tested, and all remaining findings are P2 style suggestions. All identified issues are P2: the bash subshell export gap is a pre-existing limitation (not a regression), and the unhandled TimeoutExpired in the test is a minor robustness concern. The core fix logic — installing a shell function that bypasses PATH — is sound and correctly verified by the new regression test. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Shell starts (zsh / bash)"] --> B["Integration sourced\n.zshenv → cmux-zsh-integration.zsh\n--norc + source cmux-bash-integration.bash"]
B --> C["_cmux_install_claude_wrapper()"]
C --> D{"CMUX_SHELL_INTEGRATION_DIR\nset?"}
D -- No --> E["return 0 (no-op)"]
D -- Yes --> F["Derive bundle_dir\n= integration_dir%/shell-integration"]
F --> G{"bundle_dir/bin/claude\nexecutable?"}
G -- No --> E
G -- Yes --> H["Set _CMUX_CLAUDE_WRAPPER\n= bundle_dir/bin/claude"]
H --> I["unalias claude\ndefine claude() function"]
I --> J["User later mutates PATH\ne.g. prepends real-bin/"]
J --> K["User types: claude ..."]
K --> L["Shell function intercepts"]
L --> M["Executes _CMUX_CLAUDE_WRAPPER\n= bundled wrapper ✓"]
J --> N["Without this fix:\ncommand claude → PATH lookup"]
N --> O["Finds real-bin/claude ✗"]
Reviews (1): Last reviewed commit: "Merge branch 'main' of https://github.co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| result = subprocess.run( | ||
| ["zsh", "-d", "-i", "-c", 'PATH="$CMUX_TEST_REAL_BIN:$PATH"; claude zsh-case'], | ||
| env=env, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=8, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
TimeoutExpired not caught — surfaces as stack trace instead of clean FAIL
subprocess.run(..., timeout=8) raises subprocess.TimeoutExpired if the shell hangs. This exception is not caught anywhere in run_zsh or run_bash, so a timeout would crash the test script with an unhandled exception rather than appending a message to failures and returning 1. The same applies to run_bash at line 85–98.
| result = subprocess.run( | |
| ["zsh", "-d", "-i", "-c", 'PATH="$CMUX_TEST_REAL_BIN:$PATH"; claude zsh-case'], | |
| env=env, | |
| capture_output=True, | |
| text=True, | |
| timeout=8, | |
| check=False, | |
| ) | |
| try: | |
| result = subprocess.run( | |
| ["zsh", "-d", "-i", "-c", 'PATH="$CMUX_TEST_REAL_BIN:$PATH"; claude zsh-case'], | |
| env=env, | |
| capture_output=True, | |
| text=True, | |
| timeout=8, | |
| check=False, | |
| ) | |
| except subprocess.TimeoutExpired: | |
| return 1, "zsh timed out", [] |
| _CMUX_CLAUDE_WRAPPER="$wrapper_path" | ||
| unalias claude >/dev/null 2>&1 || true | ||
| claude() { | ||
| "$_CMUX_CLAUDE_WRAPPER" "$@" | ||
| } | ||
| } |
There was a problem hiding this comment.
claude function and _CMUX_CLAUDE_WRAPPER not exported to subshells
The claude() function reads $_CMUX_CLAUDE_WRAPPER dynamically at call time, but neither the variable nor the function itself is exported. Any bash subshell launched from the interactive session (bash -c 'claude ...', inline shell scripts, etc.) will lose the wrapper and fall back to PATH lookup. The same gap exists in the zsh integration (zsh has no export -f equivalent, so nothing can be done there).
For bash, adding both exports after the function definition would extend the fix to subshells:
_CMUX_CLAUDE_WRAPPER="$wrapper_path"
unalias claude >/dev/null 2>&1 || true
claude() {
"$_CMUX_CLAUDE_WRAPPER" "$@"
}
export _CMUX_CLAUDE_WRAPPER
export -f claudeNote: this was also broken before this PR for the GHOSTTY_BIN_DIR-unset case (since _cmux_fix_path was a no-op), so this is not a regression introduced here — it's a pre-existing gap that this fix partially addresses.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/shell-integration/cmux-zsh-integration.zsh`:
- Around line 69-72: The current function definition uses the shorthand
"claude() { ... }" which allows zsh alias expansion at parse time and can
accidentally expand a pre-existing "alias claude" before the "builtin unalias
claude >/dev/null 2>&1 || true" runs; change the declaration to the explicit
"function claude { ... }" form to suppress alias expansion during sourcing,
keeping the body calling "$_CMUX_CLAUDE_WRAPPER" "$@" unchanged so the function
still delegates to the wrapper.
In `@tests/test_issue_2448_shell_claude_wrapper_dispatch.py`:
- Around line 78-83: The test's env inherits os.environ so bash can still source
BASH_ENV; modify the env dict used by run_bash (the local variable env created
in the snippet) to remove/clear the "BASH_ENV" key before launching bash (e.g.
env.pop("BASH_ENV", None) or set env["BASH_ENV"] = ""), ensuring run_bash
invocation will not leak host shell configuration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bb79833-85aa-4bd4-9da4-2e2d9e285708
📒 Files selected for processing (3)
Resources/shell-integration/cmux-bash-integration.bashResources/shell-integration/cmux-zsh-integration.zshtests/test_issue_2448_shell_claude_wrapper_dispatch.py
| env = dict(os.environ) | ||
| env["CMUX_SHELL_INTEGRATION_DIR"] = str(shell_dir) | ||
| env["CMUX_TEST_LOG"] = str(log_path) | ||
| env["CMUX_TEST_REAL_BIN"] = str(real_bin) | ||
| env["PATH"] = f"{real_bin}:/usr/bin:/bin" | ||
| env.pop("GHOSTTY_BIN_DIR", None) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
env BASH_ENV=/dev/fd/3 bash --noprofile --norc -c 'true' 3<<<'printf "BASH_ENV_RAN\n"'Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
cat -n tests/test_issue_2448_shell_claude_wrapper_dispatch.py | sed -n '70,100p'Repository: manaflow-ai/cmux
Length of output: 1302
🏁 Script executed:
#!/bin/bash
# Test if BASH_ENV is sourced with --noprofile --norc -c
# Create a temp file and set BASH_ENV to it
tmpfile=$(mktemp)
printf 'export BASH_ENV_SOURCED=yes\n' > "$tmpfile"
# Test 1: with BASH_ENV set
output=$(env BASH_ENV="$tmpfile" bash --noprofile --norc -c 'echo "${BASH_ENV_SOURCED:-no}"')
echo "Test 1 (BASH_ENV set): $output"
# Test 2: without BASH_ENV
output=$(bash --noprofile --norc -c 'echo "${BASH_ENV_SOURCED:-no}"')
echo "Test 2 (BASH_ENV unset): $output"
# Test 3: check if os.environ typically includes BASH_ENV
python3 << 'EOF'
import os
print("BASH_ENV in os.environ:", "BASH_ENV" in os.environ)
if "BASH_ENV" in os.environ:
print(" Value:", os.environ["BASH_ENV"])
EOF
rm -f "$tmpfile"Repository: manaflow-ai/cmux
Length of output: 144
🏁 Script executed:
grep -n "subprocess.run" tests/test_issue_2448_shell_claude_wrapper_dispatch.py | head -20Repository: manaflow-ai/cmux
Length of output: 123
🏁 Script executed:
cat -n tests/test_issue_2448_shell_claude_wrapper_dispatch.py | sed -n '50,75p'Repository: manaflow-ai/cmux
Length of output: 1166
🏁 Script executed:
cat -n tests/test_issue_2448_shell_claude_wrapper_dispatch.py | sed -n '44,100p'Repository: manaflow-ai/cmux
Length of output: 2521
🏁 Script executed:
grep -n "^def " tests/test_issue_2448_shell_claude_wrapper_dispatch.pyRepository: manaflow-ai/cmux
Length of output: 432
🏁 Script executed:
grep -r "bash.*--noprofile.*--norc" tests/ --include="*.py" -nRepository: manaflow-ai/cmux
Length of output: 231
🏁 Script executed:
cat -n tests/test_issue_1138_sidebar_pr_polling.py | sed -n '415,435p'Repository: manaflow-ai/cmux
Length of output: 884
🏁 Script executed:
# Check if there are other functions in the codebase that also copy os.environ and run bash
rg "subprocess\.run.*bash.*--noprofile" -A 5 -B 5 tests/ --type py | head -60Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg "subprocess\.run" tests/ --type py -A 3 | grep -A 3 "bash"Repository: manaflow-ai/cmux
Length of output: 541
Clear BASH_ENV before launching bash.
run_bash() inherits os.environ, and bash --noprofile --norc -c still sources the file named by BASH_ENV. This can leak host configuration into the test and make it flaky. Clear it defensively:
env["CMUX_TEST_REAL_BIN"] = str(real_bin)
env["PATH"] = f"{real_bin}:/usr/bin:/bin"
env.pop("GHOSTTY_BIN_DIR", None)
+ env.pop("BASH_ENV", None)📝 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.
| env = dict(os.environ) | |
| env["CMUX_SHELL_INTEGRATION_DIR"] = str(shell_dir) | |
| env["CMUX_TEST_LOG"] = str(log_path) | |
| env["CMUX_TEST_REAL_BIN"] = str(real_bin) | |
| env["PATH"] = f"{real_bin}:/usr/bin:/bin" | |
| env.pop("GHOSTTY_BIN_DIR", None) | |
| env = dict(os.environ) | |
| env["CMUX_SHELL_INTEGRATION_DIR"] = str(shell_dir) | |
| env["CMUX_TEST_LOG"] = str(log_path) | |
| env["CMUX_TEST_REAL_BIN"] = str(real_bin) | |
| env["PATH"] = f"{real_bin}:/usr/bin:/bin" | |
| env.pop("GHOSTTY_BIN_DIR", None) | |
| env.pop("BASH_ENV", None) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_issue_2448_shell_claude_wrapper_dispatch.py` around lines 78 - 83,
The test's env inherits os.environ so bash can still source BASH_ENV; modify the
env dict used by run_bash (the local variable env created in the snippet) to
remove/clear the "BASH_ENV" key before launching bash (e.g. env.pop("BASH_ENV",
None) or set env["BASH_ENV"] = ""), ensuring run_bash invocation will not leak
host shell configuration.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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="Resources/shell-integration/cmux-bash-integration.bash">
<violation number="1" location="Resources/shell-integration/cmux-bash-integration.bash:57">
P2: This early return skips installing the bundled `claude` wrapper whenever a user alias/function exists, which defeats the goal of forcing wrapper dispatch. Remove the guard so the wrapper is always installed (and the existing alias is unaliased).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| local wrapper_path="$bundle_dir/bin/claude" | ||
| [[ -x "$wrapper_path" ]] || return 0 | ||
|
|
||
| existing_type="$(type -t claude 2>/dev/null || true)" |
There was a problem hiding this comment.
P2: This early return skips installing the bundled claude wrapper whenever a user alias/function exists, which defeats the goal of forcing wrapper dispatch. Remove the guard so the wrapper is always installed (and the existing alias is unaliased).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-bash-integration.bash, line 57:
<comment>This early return skips installing the bundled `claude` wrapper whenever a user alias/function exists, which defeats the goal of forcing wrapper dispatch. Remove the guard so the wrapper is always installed (and the existing alias is unaliased).</comment>
<file context>
@@ -46,13 +46,21 @@ _cmux_restore_scrollback_once
local wrapper_path="$bundle_dir/bin/claude"
[[ -x "$wrapper_path" ]] || return 0
+ existing_type="$(type -t claude 2>/dev/null || true)"
+ case "$existing_type" in
+ alias|function)
</file context>
This branch was successfully deployed
1 participant
Fixes #2448.
Summary
claudestill resolves to the bundled wrapper whenGHOSTTY_BIN_DIRis unset andPATHlater prefers another binaryclaudeshell function in the zsh and bash integrations using the wrapper path derived fromCMUX_SHELL_INTEGRATION_DIRVerification
./scripts/setup.sh./scripts/reload.sh --tag issue-2448zsh -n Resources/shell-integration/cmux-zsh-integration.zshbash -n Resources/shell-integration/cmux-bash-integration.bashpython3 -m py_compile tests/test_issue_2448_shell_claude_wrapper_dispatch.pyNotes
Summary by cubic
Ensure zsh routes
claudethrough the bundled wrapper and bash does unless a user alias/function exists; also pin Claude hooks to the bundledcmuxso PATH changes or unsetGHOSTTY_BIN_DIRcan't break dispatch. Fixes #2448.claudeshell function fromCMUX_SHELL_INTEGRATION_DIRthat stores_CMUX_CLAUDE_WRAPPERand stays ahead of later PATH changes; zsh unaliases and installs, bash preserves existingclaudealiases/functions.Resources/bin/claudeby exportingCMUX_CLAUDE_HOOK_CMUX_BIN(fromCMUX_BUNDLED_CLI_PATH, wrapper dir, or PATH) and referencing it in the hooks JSON; add tests for hook pinning and zsh/bash alias/function cases.Written for commit adfb7ab. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests