Repository navigation
Issue 6194 session restore resume cwd - #6458
Merged
Merged
Conversation
…store-resume-cwd # Conflicts: # .github/swift-file-length-budget.tsv
…store-resume-cwd # Conflicts: # .github/swift-file-length-budget.tsv
When cmux is launched with a foreign CLAUDE_CONFIG_DIR (e.g. the .app is opened from a terminal whose agent set one), a restored `claude --resume <id>` resumes against the wrong config root and reports "No conversation found", dropping the user to a bare shell (#6194). This test fails until the wrapper self-heals CLAUDE_CONFIG_DIR to the config root that actually holds the transcript. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A restored `claude --resume <id>` only resolves a session under the current CLAUDE_CONFIG_DIR. When the cmux app inherits a foreign CLAUDE_CONFIG_DIR (e.g. the .app is opened by cmd-clicking a link from a terminal whose agent set one), it propagates that dir to every restored pane, so sessions created under a different config root resume against the wrong namespace and fail with "No conversation found" — leaving the user at a bare shell instead of their conversation (#6194). The wrapper now relocates CLAUDE_CONFIG_DIR to the config root that actually holds the transcript when resuming an explicit session id, and only when the current root lacks it (a correct resume is never repointed). Session ids are filename-token validated before any glob-walk. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…store-resume-cwd # Conflicts: # .github/swift-file-length-budget.tsv
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds validated TTY/agent caching to the ChangesClaude Wrapper Fallback, Resume Self-Healing, TTY Cache Validation & Transcript Cache Refactor
Sequence Diagram(s)sequenceDiagram
participant Shell as Shell (bash/zsh/fish)
participant Shim as claude shim
participant Wrapper as cmux-claude-wrapper
participant Extract as extract_claude_resume_session_id
participant HasSession as cmux_claude_config_dir_has_session
participant Reconcile as reconcile_claude_config_dir_for_resume
participant Claude as real claude
Shell->>Shim: claude --resume <sid>
alt cmux-claude-wrapper found (via CMUX_BUNDLED_CLI_PATH or cmux on PATH)
Shim->>Wrapper: exec cmux-claude-wrapper --resume <sid>
Wrapper->>Extract: argv
Extract-->>Wrapper: session_id
Wrapper->>HasSession: CLAUDE_CONFIG_DIR + session_id
HasSession-->>Wrapper: not found
Wrapper->>Reconcile: session_id
Reconcile->>HasSession: each known config root
HasSession-->>Reconcile: first match
Reconcile-->>Wrapper: updated CLAUDE_CONFIG_DIR
Wrapper->>Claude: exec real claude --resume <sid>
else no wrapper available
Shim->>Shim: strip cmux shim dirs from PATH
Shim->>Claude: exec claude --resume <sid>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (20 passed)
✨ 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 |
…store-resume-cwd # Conflicts: # .github/swift-file-length-budget.tsv # CLI/cmux.swift # Resources/bin/cmux-claude-wrapper # Sources/RestorableAgentSession.swift # cmuxTests/ClaudeHookSurfaceResolutionSwiftTests.swift # tests/test_claude_wrapper_hooks.py
Contributor
Greptile SummaryThis PR fixes session-restore CWD regression (issue 6194) by hardening three coupled subsystems: hook surface routing, wrapper/shim resolution, and transcript-lookup caching.
Confidence Score: 5/5The changes are well-scoped, thoroughly tested, and address genuine failure modes without introducing new runtime risks. All three touched subsystems have new or expanded test coverage in both Swift and Python. The wrapper positional-arg fix and shim fallback chain are exercised by the new prompt-text resume test, the inherited-shim-root skipping test, and stale-wrapper dispatch tests for zsh/bash/fish. The transcript cache refactoring is mechanical. The only structural note is that the 3-stage wrapper resolution is now emitted by four separate code-generation sites. The four parallel shim generators (TerminalSurface+StartupEnvironment.swift, cmux-bash-integration.bash, cmux-zsh-integration.zsh, fish/config.fish) now each embed the same wrapper-resolution logic; future edits to the fallback chain need to be applied in all four places. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User types claude ..."] --> B["Shell claude() function"]
B --> C{CMUX_CLAUDE_WRAPPER_SHIM executable?}
C -- yes --> D["exec shim"]
C -- no --> E{_CMUX_CLAUDE_WRAPPER / wrapper_path executable?}
E -- yes --> D
E -- no --> F["command claude (finds shim in PATH)"]
F --> D
D --> G{Original wrapper executable?}
G -- yes --> H["exec cmux-claude-wrapper"]
G -- no --> I{CMUX_BUNDLED_CLI_PATH-relative wrapper?}
I -- yes --> H
I -- no --> J{command -v cmux wrapper?}
J -- yes --> H
J -- no --> K["Strip cmux shim dirs; exec real claude"]
H --> L{CMUX_SURFACE_ID set?}
L -- yes --> M["Inject --settings/--session-id; exec real claude"]
L -- no --> N["Pass-through"]
M --> O["claudeHookSurfaceIsListed check"]
O --> P{TTY binding listed?}
P -- yes --> R["Use TTY binding"]
P -- no --> S{PID binding listed?}
S -- yes --> T["Use PID binding"]
S -- no --> U["callerTTYBindingCache = nil"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["User types claude ..."] --> B["Shell claude() function"]
B --> C{CMUX_CLAUDE_WRAPPER_SHIM executable?}
C -- yes --> D["exec shim"]
C -- no --> E{_CMUX_CLAUDE_WRAPPER / wrapper_path executable?}
E -- yes --> D
E -- no --> F["command claude (finds shim in PATH)"]
F --> D
D --> G{Original wrapper executable?}
G -- yes --> H["exec cmux-claude-wrapper"]
G -- no --> I{CMUX_BUNDLED_CLI_PATH-relative wrapper?}
I -- yes --> H
I -- no --> J{command -v cmux wrapper?}
J -- yes --> H
J -- no --> K["Strip cmux shim dirs; exec real claude"]
H --> L{CMUX_SURFACE_ID set?}
L -- yes --> M["Inject --settings/--session-id; exec real claude"]
L -- no --> N["Pass-through"]
M --> O["claudeHookSurfaceIsListed check"]
O --> P{TTY binding listed?}
P -- yes --> R["Use TTY binding"]
P -- no --> S{PID binding listed?}
S -- yes --> T["Use PID binding"]
S -- no --> U["callerTTYBindingCache = nil"]
Reviews (2): Last reviewed commit: "chore: refresh Swift file length budget ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+141
to
+159
| if [[ -x "$cmux_wrapper" ]]; then | ||
| exec "$cmux_wrapper" "$@" | ||
| fi | ||
| cmux_path_without_shim="" | ||
| cmux_old_ifs="$IFS" | ||
| IFS=: | ||
| for cmux_entry in ${PATH:-}; do | ||
| if [[ "$cmux_entry" == "$CMUX_CLAUDE_WRAPPER_SHIM_ROOT" || "$cmux_entry" == */cmux-cli-shims/* || "$cmux_entry" == */cmux-cli-shims ]]; then | ||
| continue | ||
| fi | ||
| if [[ -z "$cmux_path_without_shim" ]]; then | ||
| cmux_path_without_shim="$cmux_entry" | ||
| else | ||
| cmux_path_without_shim="$cmux_path_without_shim:$cmux_entry" | ||
| fi | ||
| done | ||
| IFS="$cmux_old_ifs" | ||
| export PATH="$cmux_path_without_shim" | ||
| exec claude "$@" |
Contributor
There was a problem hiding this comment.
claude binary exists in stripped PATH
The final fallback strips all cmux shim directories from PATH and runs exec claude "$@". If the user has claude installed exclusively via a cmux-managed path (a non-shim location that also got filtered out, or a system where the real binary only lives inside one of the skipped directories), the exec will fail with a shell "not found" error and no cmux-specific message. The same pattern is duplicated across the bash/zsh/fish shell integrations, so the silent failure applies everywhere. Adding a short diagnostic printf before the final exec would make this fallback diagnosable without a debugger.
Comment on lines
22914
to
22941
| func callerTTYBinding() -> CallerTerminalBinding? { | ||
| if !didResolveCallerTTYBinding { | ||
| didResolveCallerTTYBinding = true | ||
| callerTTYBindingCache = resolveCallerTerminalBindingByTTY( | ||
| let ttyBinding = resolveCallerTerminalBindingByTTY( | ||
| client: client, | ||
| includeAmbientTTY: workspaceArg == nil && surfaceArg == nil | ||
| ) ?? resolveAgentProcessTerminalBinding( | ||
| pid: claudeAgentPID(from: ProcessInfo.processInfo.environment), | ||
| socketPath: client.socketPath, | ||
| socketPassword: socketPassword | ||
| ) | ||
| if let ttyBinding, | ||
| claudeHookSurfaceIsListed(ttyBinding.surfaceId, workspaceId: ttyBinding.workspaceId, client: client) { | ||
| callerTTYBindingCache = ttyBinding | ||
| } else { | ||
| let processBinding = resolveAgentProcessTerminalBinding( | ||
| pid: claudeAgentPID(from: ProcessInfo.processInfo.environment), | ||
| socketPath: client.socketPath, | ||
| socketPassword: socketPassword | ||
| ) | ||
| if let processBinding, | ||
| claudeHookSurfaceIsListed( | ||
| processBinding.surfaceId, | ||
| workspaceId: processBinding.workspaceId, | ||
| client: client | ||
| ) { | ||
| callerTTYBindingCache = processBinding | ||
| } | ||
| } | ||
| } | ||
| return callerTTYBindingCache | ||
| } |
Contributor
There was a problem hiding this comment.
callerTTYBindingCache stays nil when TTY binding is stale and process binding is also not listed
The new code validates both ttyBinding and processBinding against claudeHookSurfaceIsListed. When the TTY binding exists but its surface is not listed (stale), the code falls through to processBinding. If processBinding is also non-nil but its surface is also not listed (e.g., the socket is transiently unavailable during the listing check), callerTTYBindingCache remains nil and the hook call is silently dropped. Previously the code would have used the stale TTY binding. The intentional "block fall-through on invalid surface" design is sound, but it introduces a new failure mode when claudeHookSurfaceIsListed has transient false negatives — and there's no observability (no log, no metric) to distinguish a correctly-blocked stale surface from a spurious socket-read failure.
Comment on lines
309
to
314
| fi | ||
| ;; | ||
| *) | ||
| return 1 | ||
| ;; | ||
| esac | ||
| index=$((index + 1)) |
Contributor
There was a problem hiding this comment.
return 1 in the *) case allows scanning past positional args — correct fix, but a gap in test coverage
The removal of the early return 1 from the positional-arg case in both extract_claude_resume_session_id and claude_explicit_session_flag_present is the correct fix for the "prompt before --resume" scenario. The existing --resume) handler's index + 1 < ${#args[@]} guard prevents extracting the wrong arg when --resume is the last token. However, there is no test for ["--resume", "session-id", "some-extra-positional"] to confirm that a positional after a valid --resume <id> pair does not interfere. Adding this case alongside the new test_live_socket_resume_after_prompt_text_does_not_inject_session_id would lock in both halves of the ordering invariant.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The #5989 merge added 7 lines to CLI/cmux.swift; regenerate the budget so the length check passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes Claude session restore so resumed sessions open in the original working directory and recover when
CLAUDE_CONFIG_DIRdoesn’t match the transcript location. Hardens hook routing and wrapper/shim resolution so Claude starts in the right workspace, recovers from stale wrappers/aliases in bash/zsh/fish, and avoids bare-shell drops. Addresses issue 6194.Bug Fixes
CLAUDE_CONFIG_DIRforclaude --resume <id>when the current root lacks the transcript; validates ids, bounds the search, preserves auth selection, and stops at prompt text without injecting a session id.cmux’s sibling; if missing or reaped, strip inheritedcmuxshim roots from PATH and exec the user’sclaude. Shellclaude()functions route through a shim resolver and recover after aliases/functions and late PATH changes (bash/zsh/fish).cmux-cli-shims/claudeas self to avoid recursion.Tests
cmuxCLI shim roots.claude()uses the shim resolver and falls back to the current bundle when the original wrapper is reaped.cmuxCLI shim roots.Written for commit 9ff87f1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests