Skip to content

Fix stale Claude shim recursion - #6164

Closed
Satwik-Jain wants to merge 3 commits into
manaflow-ai:mainfrom
Satwik-Jain:fix/claude-shim-recursion
Closed

Satwik-Jain wants to merge 3 commits into
manaflow-ai:mainfrom
Satwik-Jain:fix/claude-shim-recursion

Conversation

@Satwik-Jain

@Satwik-Jain Satwik-Jain commented Jun 15, 2026 •

Copy link
Copy Markdown

Summary

  • reject every per-surface cmux-cli-shims/*/claude entry while resolving the real Claude binary
  • add a behavioral regression test with current and stale surface shim directories

Root cause

If cmux is launched from a cmux terminal, its process can inherit that surface's Claude shim in PATH. A new terminal prepends its own shim, but the wrapper previously excluded only the shim named by the current CMUX_CLAUDE_WRAPPER_SHIM variables. It could therefore select the inherited stale shim as REAL_CLAUDE, causing the wrappers to invoke each other until the growing argument list failed with E2BIG.

Validation

  • regression-only commit fails with expected user claude after stale shim, got 'stale-shim --version'
  • python3 tests/test_claude_wrapper_user_binary_resolution.py
  • python3 tests/test_claude_wrapper_hooks.py
  • python3 tests/test_issue_2448_shell_claude_wrapper_dispatch.py
  • bash -n Resources/bin/cmux-claude-wrapper
  • CMUX_ZIG=/opt/homebrew/opt/zig@0.15/bin/zig ./scripts/reload.sh --tag fix-stale-claude-shim

No user-facing strings or localization resources changed.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Prevents infinite recursion in the Claude wrapper when a terminal inherits a stale cmux shim. The wrapper now rejects any per-surface shims and aligns shim-path matching with the shell/Swift shims to always use the user’s real Claude.

  • Bug Fixes
    • Skip all cmux-cli-shims/*/claude candidates during REAL_CLAUDE resolution.
    • Add regression test covering stale and current shims; verifies the real user binary is chosen.

Written for commit 069a29a. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Updated the wrapper to recognize claude binaries under cmux-cli-shims as shim references, ensuring it skips stale shim paths when resolving the real executable.
  • Tests

    • Added a test that sets up both stale and current shim instances and verifies the wrapper resolves to the correct user-owned claude output.

@vercel

vercel Bot commented Jun 15, 2026

Copy link
Copy Markdown

@Satwik-Jain is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps

greptile-apps Bot commented Jun 15, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes an infinite-recursion bug (E2BIG argument-list exhaustion) that occurs when a cmux terminal inherits a stale surface shim in PATH from a parent cmux terminal. The wrapper previously only excluded the shim named by CMUX_CLAUDE_WRAPPER_SHIM / CMUX_CLAUDE_WRAPPER_SHIM_ROOT, so a leftover cmux-cli-shims/<stale-surface>/claude could be selected as REAL_CLAUDE and chain-invoke the wrapper indefinitely.

  • Adds a case branch in cmux_claude_wrapper_is_self_or_shim matching */cmux-cli-shims/*/claude so every per-surface shim directory is rejected during REAL_CLAUDE resolution, not just the active one.
  • Adds test_wrapper_skips_stale_cmux_shims which places a stale shim ahead of the current one in PATH and asserts the wrapper walks past both and selects the real user binary.

Confidence Score: 5/5

Safe to merge. The change is a 4-line addition to a case statement with no effect on any code path outside find_real_claude, and the regression test confirms the stale-shim scenario resolves to the correct binary.

The fix is minimal and surgical — a single glob pattern added to an existing case statement. The pattern /cmux-cli-shims//claude in a bash case correctly matches any depth prefix and covers every per-surface shim directory. Existing checks for CMUX_CLAUDE_WRAPPER_SHIM and CMUX_CLAUDE_WRAPPER_SHIM_ROOT remain in place as first-priority guards; the new branch adds defense-in-depth for inherited stale shims not reflected in those env vars. The accompanying regression test exercises the exact failure scenario described in the PR and confirms the right binary is selected.

No files require special attention.

Important Files Changed

Filename Overview
Resources/bin/cmux-claude-wrapper Adds a glob case branch to cmux_claude_wrapper_is_self_or_shim that rejects any */cmux-cli-shims/*/claude candidate, preventing stale inherited shims from being selected as REAL_CLAUDE and triggering infinite recursion.
tests/test_claude_wrapper_user_binary_resolution.py Adds test_wrapper_skips_stale_cmux_shims to simulate a stale surface shim ahead of the current one in PATH and verify the wrapper resolves to the real user binary; wired into main().

Reviews (2): Last reviewed commit: "Address shim resolution review feedback" | Re-trigger Greptile

Comment thread tests/test_claude_wrapper_user_binary_resolution.py Outdated
Comment thread Resources/bin/cmux-claude-wrapper
@coderabbitai

coderabbitai Bot commented Jun 15, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 71eb6c44-fc7b-4652-8499-e1f80400c436

📥 Commits

Reviewing files that changed from the base of the PR and between dedca03 and 069a29a.

📒 Files selected for processing (2)
  • Resources/bin/cmux-claude-wrapper
  • tests/test_claude_wrapper_user_binary_resolution.py
💤 Files with no reviewable changes (1)
  • tests/test_claude_wrapper_user_binary_resolution.py

📝 Walkthrough

Walkthrough

cmux_claude_wrapper_is_self_or_shim gains a new glob pattern that matches claude binaries under */cmux-cli-shims/*/claude, causing find_real_claude to skip them. A new test, test_wrapper_skips_stale_cmux_shims, verifies that the wrapper selects the user-owned claude binary and ignores stale shim entries.

Changes

Shim Exclusion and Test Coverage

Layer / File(s) Summary
Shim glob pattern in is_self_or_shim
Resources/bin/cmux-claude-wrapper
Adds a glob pattern for */cmux-cli-shims/*/claude inside cmux_claude_wrapper_is_self_or_shim, so find_real_claude treats those paths as shim references and skips them.
Stale shim skipping test
tests/test_claude_wrapper_user_binary_resolution.py
Adds test_wrapper_skips_stale_cmux_shims which sets up bundled, user, current-shim, and stale-shim executables in a temp directory, runs the wrapper via the current shim with CMUX_* env vars, and asserts the output matches the user real-claude --version; wires the test into main().

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • manaflow-ai/cmux#5721: Ensures resumed claude is routed via CMUX_CLAUDE_WRAPPER_SHIM token, while this PR updates the wrapper to skip stale shimmed paths during real executable selection.

Suggested reviewers

  • Ari4ka

Poem

🐇 A shim in the path? Not today, my friend!
The wrapper now checks where the fake claude ends.
cmux-cli-shims, you shall not pass through,
The real claude is found — the user's own brew.
Hop hop, stale shims skipped with glee! 🎉

🚥 Pre-merge checks | ✅ 20 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (20 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: fixing stale Claude shim recursion, which directly addresses the core issue in the PR.
Description check ✅ Passed The description includes a comprehensive summary with root cause analysis, comprehensive validation steps, and testing evidence, largely matching the template requirements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed No Swift production code changes in this PR. Changes are to a bash wrapper script and Python test file only.
Cmux Swift Blocking Runtime ✅ Passed PR contains no Swift code changes (only Bash wrapper script and Python test), so the Swift blocking runtime check is not applicable.
Cmux Expensive Synchronous Load ✅ Passed This PR modifies only bash script and Python test files. The check applies exclusively to production Swift changes, which this PR does not contain.
Cmux Cache Substitution Correctness ✅ Passed Check applies only to production Swift, TypeScript, JavaScript changes. PR modifies bash script and Python test only, with no cache substitution operations.
Cmux No Hacky Sleeps ✅ Passed No hacky sleeps, timers, polling, or fixed delays in production/test code. Changes only add pattern matching for shim detection and deterministic unit tests with temporary directories and subproces...
Cmux Algorithmic Complexity ✅ Passed Production code adds O(1) pattern matching in PATH iteration (O(n) bounded to ~10-20 entries), not a hot path; test code exempt. No nested scans, rescans, or unbounded collections.
Cmux Swift Concurrency ✅ Passed PR only modifies Resources/bin/cmux-claude-wrapper (shell script) and Python test file. No Swift code changes made, so Swift concurrency modernization check does not apply.
Cmux Swift @Concurrent ✅ Passed No Swift files were modified in this PR; changes are to bash script and Python test file only. The @concurrent annotation check applies only to Swift code.
Cmux Swift File And Package Boundaries ✅ Passed This PR contains no Swift file changes. The modifications are to a Bash shell script (Resources/bin/cmux-claude-wrapper) and Python test file (tests/test_claude_wrapper_user_binary_resolution.py)....
Cmux Swift Logging ✅ Passed PR modifies only Bash shell script (cmux-claude-wrapper) and Python test file; no Swift code changes present, so swift-logging.md rules do not apply.
Cmux User-Facing Error Privacy ✅ Passed PR adds internal wrapper logic and developer-only tests; no user-facing errors or sensitive material (vendor names, env vars, credentials) are exposed to end users.
Cmux Full Internationalization ✅ Passed No user-facing strings or localization changes. PR modifies operational shell wrapper and adds Python test code, both explicitly allowed categories under the full-internationalization rule.
Cmux Swiftui State Layout ✅ Passed PR contains no SwiftUI changes. Modified files are shell script (cmux-claude-wrapper) and Python test (test_claude_wrapper_user_binary_resolution.py), not Swift/SwiftUI code.
Cmux Architecture Rethink ✅ Passed Custom check for Swift architectural rethink is not applicable: PR modifies only Bash shell script and Python test file, not Swift code.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR contains no Swift changes (only bash wrapper and Python tests); auxiliary window close shortcut check is not applicable.
Cmux Source Artifacts ✅ Passed Both changed files are hand-written source code (shell script and Python test) intentionally part of the product and test suite; no generated artifacts, logs, caches, or build output included.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@teamleaderleo

Copy link
Copy Markdown
Collaborator

You had this first; main now rejects inherited cmux-cli-shims/*/claude entries and carries the regression in #15116. Closing this superseded PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants