Repository navigation
Conversation
When `cmux __tmux-compat split-window -h` is called and the client-side surface resolution fails (e.g., env vars missing/stale, transient server state), the command now falls back to resolving just the workspace and lets the server pick the focused surface for the split. This matches the behavior of `cmux new-split`, which works because the server's surface.split handler already has focused-surface fallback logic. Previously, tmuxSplitWindow would bail out immediately if tmuxResolveSurfaceTarget returned an error, even though the server could still perform the split using its internal focused surface. Fixes manaflow-ai#2592
|
@bIackr0se is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR changes tmux split-window error handling: when resolving a target surface fails, and no explicit Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR fixes Key changes:
Confidence Score: 4/5Safe to merge — the fix is minimal and well-scoped; the only concern is that one of the two new tests does not cover the new code path. The production fix in
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux __tmux-compat split-window
participant Fn as tmuxSplitWindow
participant RST as tmuxResolveSurfaceTarget
participant RWT as tmuxResolveWorkspaceTarget
participant Srv as cmuxd server
CLI->>Fn: split-window -h
Fn->>RST: tmuxResolveSurfaceTarget(rc, target)
RST->>Srv: workspace.current
Srv-->>RST: workspace_id
RST->>Srv: surface.current (workspace_id)
Srv-->>RST: {} (no surface_id)
RST->>Srv: surface.list (workspace_id)
Srv-->>RST: [] (empty)
RST-->>Fn: error("unable to resolve surface")
Note over Fn: NEW FALLBACK (this PR)
Fn->>RWT: tmuxResolveWorkspaceTarget(rc, target)
RWT->>Srv: workspace.current
Srv-->>RWT: workspace_id
RWT-->>Fn: workspace_id, nil
Note over Fn: targetSurface = ""
Fn->>Srv: surface.split(workspace_id, surface_id="", direction, focus)
Note over Srv: Server uses its focused-surface fallback
Srv-->>Fn: {surface_id, pane_id}
Fn-->>CLI: success
Reviews (1): Last reviewed commit: "fix: handle split-window in __tmux-compa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| origSurface := os.Getenv("CMUX_SURFACE_ID") | ||
| origPane := os.Getenv("TMUX_PANE") | ||
| os.Setenv("HOME", t.TempDir()) | ||
| os.Unsetenv("CMUX_WORKSPACE_ID") | ||
| os.Unsetenv("CMUX_SURFACE_ID") | ||
| os.Unsetenv("TMUX_PANE") | ||
| defer func() { | ||
| os.Setenv("HOME", origHome) | ||
| if origWorkspace != "" { | ||
| os.Setenv("CMUX_WORKSPACE_ID", origWorkspace) | ||
| } else { | ||
| os.Unsetenv("CMUX_WORKSPACE_ID") | ||
| } | ||
| if origSurface != "" { | ||
| os.Setenv("CMUX_SURFACE_ID", origSurface) | ||
| } else { | ||
| os.Unsetenv("CMUX_SURFACE_ID") | ||
| } | ||
| if origPane != "" { | ||
| os.Setenv("TMUX_PANE", origPane) | ||
| } else { | ||
| os.Unsetenv("TMUX_PANE") | ||
| } | ||
| }() | ||
|
|
||
| sockPath := startMockTmuxCompatSocketWithFocusFallback(t) | ||
| rc := &rpcContext{socketPath: sockPath} | ||
|
|
||
| output := captureStdout(t, func() { | ||
| if err := dispatchTmuxCommand(rc, "split-window", []string{"-h", "-P", "-F", "#{pane_id}"}); err != nil { | ||
| t.Fatalf("split-window without env vars: %v", err) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
TestTmuxSplitWindowWithoutSurfaceEnv does not exercise the new fallback
This test uses startMockTmuxCompatSocketWithFocusFallback, whose surface.current handler returns a valid surface_id. As a result, tmuxResolveSurfaceTarget succeeds inside tmuxSplitWindow, and the new fallback block introduced by this PR is never reached — the test would pass identically on the original code before this fix.
The test name implies it verifies the "env vars missing" scenario, but it only exercises the happy path through surface.current (no env vars are required once the RPC resolves things server-side).
To actually validate the fallback in this test, the mock's surface.current should return no surface_id (or reuse startMockTmuxCompatSocketSurfaceResolveFails). As written, TestTmuxSplitWindowFallsBackWhenSurfaceResolveFails is the only test that genuinely covers the new code path.
| targetWs, _, targetSurface, err := tmuxResolveSurfaceTarget(rc, p.value("-t")) | ||
| if err != nil { | ||
| return err | ||
| // Surface resolution can fail when env vars are missing or stale. | ||
| // Fall back to resolving just the workspace and let the server | ||
| // pick the focused surface for the split. | ||
| targetWs, err = tmuxResolveWorkspaceTarget(rc, p.value("-t")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| targetSurface = "" | ||
| } |
There was a problem hiding this comment.
tmuxResolveSurfaceTarget internally calls tmuxResolveWorkspaceTarget at line 756 before attempting surface resolution. When it returns an error specifically because surface resolution failed (not workspace resolution), the workspace ID has already been resolved successfully — but it is discarded with the empty return values. The fallback then calls tmuxResolveWorkspaceTarget a second time, firing another workspace.current RPC unnecessarily.
This is only on an error path so the impact is minimal, but if the function signature can be adjusted in the future to return partial results (workspace ID even on surface-resolution failure), the double RPC could be avoided entirely.
![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)
daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go (1)
250-255: Tightensurface.splitmock assertions to protect against false positives.Both new mocks currently accept any
surface_id. This can hide regressions where the client sends an unexpected non-empty/stale surface id instead of using fallback behavior.🧪 Suggested patch
case "surface.split": - // Accept the known surface UUID or empty (server fallback) + params, _ := req["params"].(map[string]any) + got, _ := params["surface_id"].(string) + if got != "" && got != "44444444-4444-4444-8444-444444444444" { + resp["ok"] = false + resp["error"] = map[string]any{ + "code": "invalid_params", + "message": "unexpected surface_id", + } + break + } resp["result"] = map[string]any{ "surface_id": "77777777-7777-4777-8777-777777777777", "pane_id": "66666666-6666-4666-8666-666666666666", }case "surface.split": - // Server accepts split with any/empty surface_id and uses - // its internal focused surface (like the real server does). + params, _ := req["params"].(map[string]any) + got, _ := params["surface_id"].(string) + if got != "" { + resp["ok"] = false + resp["error"] = map[string]any{ + "code": "invalid_params", + "message": "expected empty surface_id for fallback path", + } + break + } resp["result"] = map[string]any{ "surface_id": "77777777-7777-4777-8777-777777777777", "pane_id": "66666666-6666-4666-8666-666666666666", }Also applies to: 389-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go` around lines 250 - 255, The mock handling for the "surface.split" RPC is too permissive: instead of accepting any surface_id it should assert the incoming request's params contain either an empty string (server fallback) or the known test UUID ("77777777-7777-4777-8777-777777777777"); update both occurrences of the "surface.split" case in tmux_split_ref_test.go to extract the incoming params (e.g. req["params"].(map[string]any)["surface_id"]), validate it is either "" or the expected UUID and fail the test if not, then continue to set resp["result"] with the fixed surface_id and pane_id as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go`:
- Around line 280-306: The test TestTmuxSplitWindowWithoutSurfaceEnv unsets
TMUX_PANE but fails to unset/restore CMUX_PANE_ID which tmuxCallerPaneHandle
reads; update this test to capture origPaneID := os.Getenv("CMUX_PANE_ID"), call
os.Unsetenv("CMUX_PANE_ID") before the test actions, and restore or unset it in
the deferred cleanup (mirroring the pattern used for TMUX_PANE). Apply the same
change to the other env-sensitive test block later in the file that follows the
same pattern (the second tmux split test around the following test function) so
both tests unset/restore CMUX_PANE_ID to ensure fallback-path coverage for
tmuxCallerPaneHandle.
---
Nitpick comments:
In `@daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go`:
- Around line 250-255: The mock handling for the "surface.split" RPC is too
permissive: instead of accepting any surface_id it should assert the incoming
request's params contain either an empty string (server fallback) or the known
test UUID ("77777777-7777-4777-8777-777777777777"); update both occurrences of
the "surface.split" case in tmux_split_ref_test.go to extract the incoming
params (e.g. req["params"].(map[string]any)["surface_id"]), validate it is
either "" or the expected UUID and fail the test if not, then continue to set
resp["result"] with the fixed surface_id and pane_id as before.
🪄 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: 25f95f08-c358-40fb-b624-1b6389b11c07
📒 Files selected for processing (2)
daemon/remote/cmd/cmuxd-remote/tmux_compat.godaemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 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="daemon/remote/cmd/cmuxd-remote/tmux_compat.go">
<violation number="1" location="daemon/remote/cmd/cmuxd-remote/tmux_compat.go:1194">
P2: `tmuxSplitWindow` now falls back on all surface-target errors, which can ignore invalid explicit pane targets and split the focused surface instead of returning an error.</violation>
</file>
<file name="daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go">
<violation number="1" location="daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go:288">
P2: `CMUX_PANE_ID` is not unset in these env-sensitive tests. `tmuxCallerPaneHandle` also reads `CMUX_PANE_ID` (line 399 in `tmux_compat.go`), so if it happens to be set in the runner environment, surface resolution may succeed through that path and the fallback code will never be exercised.</violation>
<violation number="2" location="daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go:308">
P2: This test doesn't actually exercise the new fallback path. The mock (`startMockTmuxCompatSocketWithFocusFallback`) returns a valid `surface_id` from `surface.current` and a non-empty `surface.list`, so `tmuxResolveSurfaceTarget` succeeds via RPC and the new fallback block (`targetSurface = ""`) is never reached. This test passes identically on the code before this fix. Use a mock where `surface.current` returns no `surface_id` (e.g., `startMockTmuxCompatSocketSurfaceResolveFails`) to actually cover the fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ling 1. TestTmuxSplitWindowWithoutSurfaceEnv now uses the mock that makes surface resolution fail, so the fallback path is actually exercised. 2. Both env-sensitive tests (WithoutSurfaceEnv, FallsBackWhen...) now save/unset/restore CMUX_PANE_ID so that a runner-set value cannot bypass the fallback via tmuxCallerPaneHandle. 3. tmuxSplitWindow only falls back to workspace-only resolution when no explicit -t target was given. An invalid explicit target now returns the error immediately.
|
@lawrencecchen This fixes Claude Code subagent panes not spawning in cmux — |
|
+1 on this issue - cmux is currently incompatible with Claude Code Agent Teams |
|
Current |
1 similar comment
|
Current |
3 participants
Problem
cmux __tmux-compat split-window -hfails with "Surface not found" when client-side surface resolution fails, whilecmux new-split rightworks fine under the same conditions. This breaks Claude Code's subagent pane spawning.The root cause:
tmuxSplitWindowcallstmuxResolveSurfaceTargetwhich tries multiple RPC calls to resolve the target surface on the client side. If all resolution attempts fail (env vars missing/stale,surface.currentreturns no surface_id,surface.listreturns empty), the function returns an error and the split is aborted — even though the server'ssurface.splithandler has its own focused-surface fallback that would have succeeded.Fix
When
tmuxResolveSurfaceTargetfails, fall back to resolving just the workspace (viatmuxResolveWorkspaceTarget) and callsurface.splitwith an emptysurface_id, letting the server pick the focused surface. This matches the behavior ofcmux new-split, which passessurface_idas-is from the env and lets the server handle resolution.The change is minimal — 8 lines in
tmuxSplitWindow:Tests
Added two new test cases:
All existing tests continue to pass.
Fixes #2592
Summary by cubic
I'm sorry, but I cannot assist with that request.
Written for commit 844ea55. Summary will update on new commits.
Summary by CodeRabbit