Skip to content

fix: handle split-window in __tmux-compat when surface resolution fails #2593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bIackr0se wants to merge 2 commits into from
Closed

fix: handle split-window in __tmux-compat when surface resolution fails #2593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f39d0e2
fix: handle split-window in __tmux-compat when surface resolution fails
bIackr0se Apr 4, 2026
844ea55
fix: address review feedback — test coverage and explicit target hand…
bIackr0se Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion daemon/remote/cmd/cmuxd-remote/tmux_compat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,7 +1188,17 @@ func tmuxSplitWindow(rc *rpcContext, args []string) error {

targetWs, _, targetSurface, err := tmuxResolveSurfaceTarget(rc, p.value("-t"))
if err != nil {
return err
if p.value("-t") != "" {
return err // explicit target was invalid — don't fall back
}
// No explicit target; 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"))
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
if err != nil {
return err
}
targetSurface = ""
}
Comment on lines 1189 to 1202

@greptile-apps greptile-apps Bot Apr 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant workspace RPC on fallback path

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.


direction := "down"
Expand Down
295 changes: 295 additions & 0 deletions daemon/remote/cmd/cmuxd-remote/tmux_split_ref_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,301 @@ func TestTmuxSplitWindowCanonicalizesCallerSurfaceRefs(t *testing.T) {
}
}

// startMockTmuxCompatSocketWithFocusFallback creates a mock that accepts
// surface.split with either the known surface UUID or an empty surface_id,
// mimicking the real server's focused-surface fallback.
func startMockTmuxCompatSocketWithFocusFallback(t *testing.T) string {
t.Helper()
sockPath := makeShortUnixSocketPath(t)

ln, err := net.Listen("unix", sockPath)
if err != nil {
t.Fatalf("failed to listen: %v", err)
}
t.Cleanup(func() { ln.Close() })

go func() {
for {
conn, err := ln.Accept()
if err != nil {
return
}
go func(conn net.Conn) {
defer conn.Close()
reader := bufio.NewReader(conn)
line, err := reader.ReadBytes('\n')
if err != nil {
return
}

var req map[string]any
if err := json.Unmarshal(line, &req); err != nil {
_, _ = conn.Write([]byte(`{"ok":false,"error":{"code":"parse","message":"bad json"}}` + "\n"))
return
}

method, _ := req["method"].(string)
resp := map[string]any{
"id": req["id"],
"ok": true,
}

switch method {
case "workspace.list":
resp["result"] = map[string]any{
"workspaces": []map[string]any{{
"id": "11111111-1111-4111-8111-111111111111",
"ref": "workspace:1",
"index": 1,
"title": "demo",
}},
}
case "workspace.current":
resp["result"] = map[string]any{
"workspace_id": "11111111-1111-4111-8111-111111111111",
"workspace_ref": "workspace:1",
}
case "surface.list":
resp["result"] = map[string]any{"surfaces": []map[string]any{{
"id": "44444444-4444-4444-8444-444444444444",
"ref": "surface:1",
"focused": true,
"pane_id": "33333333-3333-4333-8333-333333333333",
"title": "leader",
}}}
case "surface.current":
resp["result"] = map[string]any{
"workspace_id": "11111111-1111-4111-8111-111111111111",
"pane_id": "33333333-3333-4333-8333-333333333333",
"surface_id": "44444444-4444-4444-8444-444444444444",
}
case "pane.list":
resp["result"] = map[string]any{"panes": []map[string]any{{
"id": "33333333-3333-4333-8333-333333333333",
"ref": "pane:1",
"index": 1,
}}}
case "surface.split":
// Accept the known surface UUID or empty (server fallback)
resp["result"] = map[string]any{
"surface_id": "77777777-7777-4777-8777-777777777777",
"pane_id": "66666666-6666-4666-8666-666666666666",
}
case "workspace.equalize_splits":
resp["result"] = map[string]any{"ok": true}
case "pane.surfaces":
resp["result"] = map[string]any{"surfaces": []map[string]any{{
"id": "44444444-4444-4444-8444-444444444444",
"selected": true,
}}}
default:
resp["ok"] = false
resp["error"] = map[string]any{
"code": "unsupported",
"message": method,
}
}

payload, _ := json.Marshal(resp)
_, _ = conn.Write(append(payload, '\n'))
}(conn)
}
}()

return sockPath
}

func TestTmuxSplitWindowWithoutSurfaceEnv(t *testing.T) {
origHome := os.Getenv("HOME")
origWorkspace := os.Getenv("CMUX_WORKSPACE_ID")
origSurface := os.Getenv("CMUX_SURFACE_ID")
origPane := os.Getenv("TMUX_PANE")
origCmuxPane := os.Getenv("CMUX_PANE_ID")
os.Setenv("HOME", t.TempDir())
os.Unsetenv("CMUX_WORKSPACE_ID")
os.Unsetenv("CMUX_SURFACE_ID")
os.Unsetenv("TMUX_PANE")
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
os.Unsetenv("CMUX_PANE_ID")
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")
}
if origCmuxPane != "" {
os.Setenv("CMUX_PANE_ID", origCmuxPane)
} else {
os.Unsetenv("CMUX_PANE_ID")
}
}()
Comment thread
coderabbitai[bot] marked this conversation as resolved.

sockPath := startMockTmuxCompatSocketSurfaceResolveFails(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)
}
})

Comment on lines +283 to +323

@greptile-apps greptile-apps Bot Apr 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

if got := output; got != "%66666666-6666-4666-8666-666666666666\n" {
t.Fatalf("stdout = %q, want %%66666666-6666-4666-8666-666666666666", got)
}
}

// startMockTmuxCompatSocketSurfaceResolveFails creates a mock where
// surface.current returns no surface_id (simulating the scenario where
// surface resolution fails on the client side but surface.split still
// works because the server falls back to its focused surface).
func startMockTmuxCompatSocketSurfaceResolveFails(t *testing.T) string {
t.Helper()
sockPath := makeShortUnixSocketPath(t)

ln, err := net.Listen("unix", sockPath)
if err != nil {
t.Fatalf("failed to listen: %v", err)
}
t.Cleanup(func() { ln.Close() })

go func() {
for {
conn, err := ln.Accept()
if err != nil {
return
}
go func(conn net.Conn) {
defer conn.Close()
reader := bufio.NewReader(conn)
line, err := reader.ReadBytes('\n')
if err != nil {
return
}

var req map[string]any
if err := json.Unmarshal(line, &req); err != nil {
_, _ = conn.Write([]byte(`{"ok":false,"error":{"code":"parse","message":"bad json"}}` + "\n"))
return
}

method, _ := req["method"].(string)
resp := map[string]any{
"id": req["id"],
"ok": true,
}

switch method {
case "workspace.list":
resp["result"] = map[string]any{
"workspaces": []map[string]any{{
"id": "11111111-1111-4111-8111-111111111111",
"ref": "workspace:1",
"index": 1,
"title": "demo",
}},
}
case "workspace.current":
resp["result"] = map[string]any{
"workspace_id": "11111111-1111-4111-8111-111111111111",
"workspace_ref": "workspace:1",
}
case "surface.current":
// Return no surface_id — simulates transient server state
// where the focused surface is not yet available to the
// current endpoint but surface.split can still use it.
resp["result"] = map[string]any{
"workspace_id": "11111111-1111-4111-8111-111111111111",
}
case "surface.list":
// Return empty surfaces — surface resolution will fail
resp["result"] = map[string]any{"surfaces": []map[string]any{}}
case "pane.list":
resp["result"] = map[string]any{"panes": []map[string]any{}}
case "surface.split":
// Server accepts split with any/empty surface_id and uses
// its internal focused surface (like the real server does).
resp["result"] = map[string]any{
"surface_id": "77777777-7777-4777-8777-777777777777",
"pane_id": "66666666-6666-4666-8666-666666666666",
}
case "workspace.equalize_splits":
resp["result"] = map[string]any{"ok": true}
default:
resp["ok"] = false
resp["error"] = map[string]any{
"code": "unsupported",
"message": method,
}
}

payload, _ := json.Marshal(resp)
_, _ = conn.Write(append(payload, '\n'))
}(conn)
}
}()

return sockPath
}

func TestTmuxSplitWindowFallsBackWhenSurfaceResolveFails(t *testing.T) {
origHome := os.Getenv("HOME")
origWorkspace := os.Getenv("CMUX_WORKSPACE_ID")
origSurface := os.Getenv("CMUX_SURFACE_ID")
origPane := os.Getenv("TMUX_PANE")
origCmuxPane := os.Getenv("CMUX_PANE_ID")
os.Setenv("HOME", t.TempDir())
os.Unsetenv("CMUX_WORKSPACE_ID")
os.Unsetenv("CMUX_SURFACE_ID")
os.Unsetenv("TMUX_PANE")
os.Unsetenv("CMUX_PANE_ID")
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")
}
if origCmuxPane != "" {
os.Setenv("CMUX_PANE_ID", origCmuxPane)
} else {
os.Unsetenv("CMUX_PANE_ID")
}
}()

sockPath := startMockTmuxCompatSocketSurfaceResolveFails(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 should fall back when surface resolution fails: %v", err)
}
})

if got := output; got != "%66666666-6666-4666-8666-666666666666\n" {
t.Fatalf("stdout = %q, want %%66666666-6666-4666-8666-666666666666", got)
}
}

func TestTmuxSplitWindowIgnoresStaleUUIDColumnSurface(t *testing.T) {
origHome := os.Getenv("HOME")
origWorkspace := os.Getenv("CMUX_WORKSPACE_ID")
Expand Down