Repository navigation
Fix explicit surface routing for read-screen and send - #6605
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 selected for processing (1)
📝 WalkthroughWalkthroughSeven CLI subcommand parsing paths now delegate workspace-inheritance logic to a new private helper ChangesExplicit surface workspace inheritance fix and validation
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Socket as Unix Socket
participant Server as Mock Server
participant CLI as cmux CLI
rect rgba(100, 149, 237, 0.5)
Note over Test,CLI: String surface ref (surface:11)
Test->>Socket: bind temp socket
Test->>Server: start accept loop
Test->>CLI: spawn with --surface surface:11, CMUX_WORKSPACE_ID set
CLI->>Socket: connect and send JSON-RPC request
Server->>Server: record request (no workspace_id, no window_id)
Server-->>CLI: v2Response with surface_id
CLI-->>Test: exit 0
Test->>Server: assert workspace_id absent, surface_id == surface:11
end
rect rgba(144, 238, 144, 0.5)
Note over Test,CLI: Numeric surface handle (--surface 5)
Test->>Socket: bind temp socket
Test->>Server: start accept loop (handles surface.list and surface.read_text)
Test->>CLI: spawn with --surface 5, CMUX_WORKSPACE_ID set
CLI->>Socket: surface.list request (workspace_id present)
Server-->>CLI: list response with UUID entry
CLI->>Socket: surface.read_text request (workspace_id present)
Server-->>CLI: read_text response
CLI-->>Test: exit 0
Test->>Server: assert workspace_id present, surface_id == expected UUID
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (22 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 |
Greptile SummaryThis PR fixes a routing bug where
Confidence Score: 5/5The fix is safe to merge: it narrows the workspace-inheritance path to only the cases that genuinely need it (absent or numeric surface handles) and leaves the explicit-ref and UUID paths completely unscoped, matching the intended API contract. The helper The inline Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI command invoked\n(read-screen / send / send-key /\ncapture-pane / pipe-pane /\nsend-panel / send-key-panel)"] --> B{explicit --workspace\nor --window given?}
B -- yes --> C["Use supplied workspace/window\n(unchanged path)"]
B -- no --> D["callerWorkspaceForSurfaceHandle\n(sfArg, windowRaw)"]
D --> E{surface arg is\nempty or Int?}
E -- "yes / nil" --> F["workspaceArg = CMUX_WORKSPACE_ID\n(inherit caller workspace)"]
E -- "no / explicit ref or UUID" --> G["workspaceArg = nil\n(no workspace scoping)"]
F --> H["normalizeWorkspaceHandle\nreturns callerWorkspaceId"]
G --> I["normalizeWorkspaceHandle\nreturns nil\n(allowCurrent defaults false)"]
H --> J{surface arg\nis numeric?}
J -- yes --> K["surface.list scoped to\ncaller workspace\n→ resolve surface_id"]
J -- no --> L["surface ref passed directly\nwith workspace_id"]
I --> M["surface ref or UUID passed\nwith no workspace_id\nor window_id"]
%%{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["CLI command invoked\n(read-screen / send / send-key /\ncapture-pane / pipe-pane /\nsend-panel / send-key-panel)"] --> B{explicit --workspace\nor --window given?}
B -- yes --> C["Use supplied workspace/window\n(unchanged path)"]
B -- no --> D["callerWorkspaceForSurfaceHandle\n(sfArg, windowRaw)"]
D --> E{surface arg is\nempty or Int?}
E -- "yes / nil" --> F["workspaceArg = CMUX_WORKSPACE_ID\n(inherit caller workspace)"]
E -- "no / explicit ref or UUID" --> G["workspaceArg = nil\n(no workspace scoping)"]
F --> H["normalizeWorkspaceHandle\nreturns callerWorkspaceId"]
G --> I["normalizeWorkspaceHandle\nreturns nil\n(allowCurrent defaults false)"]
H --> J{surface arg\nis numeric?}
J -- yes --> K["surface.list scoped to\ncaller workspace\n→ resolve surface_id"]
J -- no --> L["surface ref passed directly\nwith workspace_id"]
I --> M["surface ref or UUID passed\nwith no workspace_id\nor window_id"]
Reviews (5): Last reviewed commit: "fix: route explicit surface CLI I/O glob..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let read = try runCase( | ||
| name: "read-explicit-surface", | ||
| arguments: ["read-screen", "--surface", targetSurfaceRef, "--lines", "5"], | ||
| expectedMethod: "surface.read_text" | ||
| ) | ||
| XCTAssertFalse(read.timedOut, read.stderr) | ||
| XCTAssertEqual(read.status, 0, read.stderr) | ||
| XCTAssertEqual(read.stdout, "agent screen\n\n") | ||
|
|
||
| let send = try runCase( | ||
| name: "send-explicit-surface", | ||
| arguments: ["send", "--surface", targetSurfaceRef, "hello"], | ||
| expectedMethod: "surface.send_text", | ||
| expectedText: "hello" | ||
| ) | ||
| XCTAssertFalse(send.timedOut, send.stderr) | ||
| XCTAssertEqual(send.status, 0, send.stderr) | ||
| XCTAssertTrue(send.stdout.contains("OK"), send.stdout) | ||
| } | ||
|
|
||
| func testPipePaneWindowWorkspaceOmittedSurfaceDoesNotUseSelectedWorkspaceSurface() throws { | ||
| let cliPath = try bundledCLIPath() |
There was a problem hiding this comment.
send-key, send-panel, and send-key-panel
All five subcommands received the same workspace-inheritance fix, but the new regression test only exercises read-screen and send. send-key, send-panel, and send-key-panel have no coverage for the "explicit surface/panel ref must not inherit CMUX_WORKSPACE_ID" invariant. A future refactor that accidentally reverts those three cases would go undetected by CI. The test also doesn't include a positive control verifying that a bare numeric surface index (e.g. "5") still does inherit the caller workspace — the two-sided contract is only half-verified.
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!
There was a problem hiding this comment.
Expanded the regression to cover send-key, send-panel, send-key-panel, and the numeric-index positive control. The test now verifies explicit surface refs skip caller workspace while bare numeric indexes still inherit it.
— Claude Code
There was a problem hiding this comment.
Addressed in the current head: CLIExplicitSurfaceRoutingTests now covers read-screen, send, send-key, send-panel, send-key-panel, capture-pane, pipe-pane, plus the numeric surface index positive control.
— Claude Code
66cd663 to
06df20b
Compare
06df20b to
b9ebed5
Compare
b9ebed5 to
ab50e06
Compare
ab50e06 to
395029f
Compare
This branch was successfully deployed
1 participant
Summary
read-screen,send,send-key, and panel send aliases from inheriting the caller workspace when an explicit surface ref/UUID is supplied.surface:NCLI routing from a caller surface with ambientCMUX_WORKSPACE_ID.Reproduction
cmux read-screen --surface surface:11 --lines 5failed withinvalid_params: Surface is not a terminalwhen run from another workspace.cmux read-screen --workspace workspace:3 --surface surface:11 --lines 5succeeded, confirming the explicit surface was being scoped to the wrong caller workspace.cmux send --surface surface:11 " "hit the same invalid terminal error.Verification
Fixes #6598
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fix incorrect CLI routing when an explicit surface or panel ref/UUID is provided.
read-screen,capture-pane,pipe-pane,send,send-key,send-panel, andsend-key-panelnow route globally to the target instead of inheriting the caller workspace, fixing #6598.workspace_idonly for numeric or omitted handles; for explicit refs/UUIDs, omitworkspace_idandwindow_idin RPCs.surface.listwithin the caller workspace; do not passwindow_idtosurface.list.read-screen.Written for commit 395029f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
read-screen,send,send-key,send-panel, andsend-key-panel.