Repository navigation
fix(surface): accept surface_ref and surface aliases in send/read RPCs (related #2045) - #3032
aryateja2106 wants to merge 1 commit into
Conversation
Related manaflow-ai#2045. `surface.send_text`, `surface.send_key`, `surface.read_text`, and `surface.clear_history` only inspected `params["surface_id"]` when resolving the target surface; if absent, they silently fell back to `ws.focusedPanelId` (the caller's own pane). Callers using `surface_ref` (short refs like `surface:12`) or `surface` keys had their target silently ignored — the RPC then operated on the wrong pane. This is the same class as manaflow-ai#2045: the targeting parameter is asymmetric between request inputs and response outputs. Responses already include both `surface_id` and `surface_ref`; inputs should accept both forms. Replace the `params["surface_id"]` check in all four handlers with a fallback chain through `surface_id`, `surface_ref`, and `surface`, using the existing `v2UUIDAny` helper (which already accepts UUIDs and short-form handle refs via `v2ResolveHandleRef`). Update the error message to reflect all three accepted keys. Reproducer of the silent retargeting bug that this fixes: $ cmux new-pane --type terminal --direction right --id-format both OK surface:12 pane:11 workspace:5 $ cmux rpc surface.send_text '{"surface":"surface:12","text":"x\n"}' # response surface_id is the CALLER's surface (e.g. 9E1F1A31...), # not the requested surface:12. Text was injected into the caller's # own input box, which in a Claude Code / OMC session ends up # submitted as user input. After this change the request honors `surface`/`surface_ref`/`surface_id` identically.
|
@aryateja2106 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughUpdated multiple V2 surface-targeting handlers in TerminalController to accept flexible surface identifier sources ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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)
Sources/TerminalController.swift (1)
5666-5674: Extract the repeated surface-key fallback into a helper.This exact 7-line block (
rawSurfacechain +v2UUIDAnyresolve +not_foundguard +focusedPanelIdfallback) is duplicated verbatim acrossv2SurfaceSendText,v2SurfaceSendKey,v2SurfaceClearHistory, andv2SurfaceReadText. Any future change (e.g., switching toinvalid_params, enriching error data, adding a fourth alias, logging) has to be mirrored in four places and is easy to drift.Consider extracting once, e.g.:
♻️ Suggested helper
/// Resolves an optional surface identifier from any of "surface_id", "surface_ref", "surface". /// Returns: /// - .some(.some(uuid)) if a key was present and resolved /// - .some(.none) if a key was present but could not be resolved (caller should error) /// - .none if no key was provided (caller should fall back to focused panel) private func v2ResolveOptionalSurfaceId(_ params: [String: Any]) -> UUID?? { guard let raw = params["surface_id"] ?? params["surface_ref"] ?? params["surface"] else { return .none } return .some(v2UUIDAny(raw)) }Call sites collapse to:
let surfaceId: UUID? switch v2ResolveOptionalSurfaceId(params) { case .some(.some(let id)): surfaceId = id case .some(.none): result = .err(code: "not_found", message: "Surface not found for the given surface_id/surface_ref/surface", data: nil) return case .none: // existing focusedPanelId fallback ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 5666 - 5674, Extract the repeated seven-line surface-id resolution into a single helper (suggested name v2ResolveOptionalSurfaceId) that checks params["surface_id"] ?? params["surface_ref"] ?? params["surface"], returns .none if no key was provided, .some(.none) if a key was present but v2UUIDAny(raw) failed, or .some(.some(uuid)) when resolved; then update v2SurfaceSendText, v2SurfaceSendKey, v2SurfaceClearHistory and v2SurfaceReadText to call this helper, switch on its three outcomes to either set surfaceId, emit the existing .err(...) with the same message when .some(.none) is returned, or fall back to focusedPanelId when .none is returned. Ensure you reference and reuse v2UUIDAny and preserve the exact error code/message and existing focusedPanelId fallback logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 5667-5673: The handler currently returns .err(code: "not_found")
when a provided surface key (rawSurface) is present but v2UUIDAny(rawSurface)
fails to resolve; change this to return .err(code: "invalid_params") when
rawSurface != nil but surfaceId == nil, and include the offending value in the
data payload (e.g. data: ["surface_id": rawSurface]) so clients can recover
programmatically; apply the same change for the three sibling handlers that call
v2UUIDAny for pane/pane_ref/related keys and ensure the error message and code
use "invalid_params" rather than "not_found".
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 5666-5674: Extract the repeated seven-line surface-id resolution
into a single helper (suggested name v2ResolveOptionalSurfaceId) that checks
params["surface_id"] ?? params["surface_ref"] ?? params["surface"], returns
.none if no key was provided, .some(.none) if a key was present but
v2UUIDAny(raw) failed, or .some(.some(uuid)) when resolved; then update
v2SurfaceSendText, v2SurfaceSendKey, v2SurfaceClearHistory and v2SurfaceReadText
to call this helper, switch on its three outcomes to either set surfaceId, emit
the existing .err(...) with the same message when .some(.none) is returned, or
fall back to focusedPanelId when .none is returned. Ensure you reference and
reuse v2UUIDAny and preserve the exact error code/message and existing
focusedPanelId fallback logic.
🪄 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: 7bfe0087-795c-4952-80a2-0f1b6e2b1e6c
📒 Files selected for processing (1)
Sources/TerminalController.swift
| let rawSurface = params["surface_id"] ?? params["surface_ref"] ?? params["surface"] | ||
| if rawSurface != nil { | ||
| surfaceId = v2UUIDAny(rawSurface) | ||
| guard surfaceId != nil else { | ||
| result = .err(code: "not_found", message: "Surface not found for the given surface_id", data: nil) | ||
| result = .err(code: "not_found", message: "Surface not found for the given surface_id/surface_ref/surface", data: nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
Consider invalid_params instead of not_found when a surface key is present but unresolvable.
When rawSurface is provided but v2UUIDAny cannot resolve it (malformed UUID / unknown handle ref), the handler returns not_found. Per established convention for v2 endpoints that accept surface_id/pane_id/etc., a present-but-unresolvable key should yield invalid_params rather than not_found — not_found is typically reserved for well-formed IDs that don't match any live surface. This same concern applies to the three sibling handlers at lines 5728-5734, 5771-5777, and 5832-5838.
Also worth noting: the data payload is nil, so clients cannot programmatically recover which of the three keys was supplied or what value failed to resolve. Consider including data: ["surface_id": <rawSurface string>] (or similar) to keep parity with the existing error-data conventions exercised by cmuxTests/TerminalControllerSocketSecurityTests.swift.
Based on learnings: "TerminalController.v2UUID(params:key) resolves either a raw UUID string or a ref-style handle ... when the key is present but unresolvable, prefer returning invalid_params instead of falling back."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 5667 - 5673, The handler
currently returns .err(code: "not_found") when a provided surface key
(rawSurface) is present but v2UUIDAny(rawSurface) fails to resolve; change this
to return .err(code: "invalid_params") when rawSurface != nil but surfaceId ==
nil, and include the offending value in the data payload (e.g. data:
["surface_id": rawSurface]) so clients can recover programmatically; apply the
same change for the three sibling handlers that call v2UUIDAny for
pane/pane_ref/related keys and ensure the error message and code use
"invalid_params" rather than "not_found".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes an asymmetric input/output schema bug in four surface RPC handlers (
Confidence Score: 4/5Safe to merge for the common case, but has a regression when any surface key is present with an explicit JSON null value. One P1 finding: the raw-Any ?? chain does not fall through NSNull(), which is a regression compared to both pre-PR behavior (where null surface_id fell back to focused pane) and the established codebase pattern. The fix is trivially the existing idiom from line 2707. Sources/TerminalController.swift — specifically the four identical surface-resolution blocks. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[RPC params received] --> B{params contains surface_id?}
B -- "present (any value)" --> C["rawSurface = params[surface_id]"]
B -- "absent" --> D{params contains surface_ref?}
D -- "present" --> E["rawSurface = params[surface_ref]"]
D -- "absent" --> F{params contains surface?}
F -- "present" --> G["rawSurface = params[surface]"]
F -- "absent" --> H[rawSurface = nil]
C --> I{rawSurface != nil?}
E --> I
G --> I
H --> J[surfaceId = ws.focusedPanelId]
I -- "yes" --> K["v2UUIDAny(rawSurface)"]
I -- "no" --> J
K -- "resolved UUID" --> L[surfaceId = UUID]
K -- "nil (NSNull trap!)" --> M["err: not_found — surface_ref never tried"]
L --> N[ws.terminalPanel lookup]
J --> N
|
|
Reviewed in the context of a session-mining sweep on 2026-04-23 where we surfaced the exact symptom this PR fixes: The patch looks right to me. A few observations from the code-review side:
What I'd like to see before merge (nothing blocking, just flagging):
From the cmuxlayer side (the TypeScript MCP wrapper that sits above this), this unblocks — Context: session-mining 2026-04-23 surfaced three bugs in this class; this PR covers Bug 1 directly. |
teamleaderleo
added
S2: major
This branch has not been deployed
3 participants
Related #2045 (same class of asymmetric targeting bug).
Problem
surface.send_text,surface.send_key,surface.read_text, andsurface.clear_historyonly inspectedparams[\"surface_id\"]when resolving the target surface. If absent, they fell back tows.focusedPanelId(the caller's own pane).This makes the input/output schemas asymmetric: responses already include both
surface_id(UUID) andsurface_ref(short ref likesurface:12), but inputs only acceptsurface_id. Callers passingsurface_reforsurfacehad their target silently ignored — the RPC operated on the wrong pane and the bug was easy to miss because no error fired.Reproducer (the silent retargeting that this fixes)
```
$ cmux new-pane --type terminal --direction right --id-format both
OK surface:12 pane:11 workspace:5
$ cmux rpc surface.send_text '{"surface":"surface:12","text":"x\\n"}'
{
"workspace_ref" : "workspace:5",
"surface_ref" : "surface:7", ← caller's own surface, not surface:12
"surface_id" : "9E1F1A31-...", ← caller's own UUID
...
}
```
In practice this hits OMC / Claude Code orchestration sessions hard: an orchestrator trying to drive a worker pane via RPC ends up typing into its own prompt box, which then gets submitted as user input on the next newline. The bug is invisible in the response shape because the response always echoes a valid surface_id+surface_ref pair (just the wrong one).
Fix
Replace the
params[\"surface_id\"]check in all four handlers (v2SurfaceSendText,v2SurfaceSendKey,v2SurfaceClearHistory,v2SurfaceReadText) with a fallback chain:```swift
let rawSurface = params["surface_id"] ?? params["surface_ref"] ?? params["surface"]
if rawSurface != nil {
surfaceId = v2UUIDAny(rawSurface)
guard surfaceId != nil else {
result = .err(code: "not_found", message: "Surface not found for the given surface_id/surface_ref/surface", data: nil)
return
}
} else {
surfaceId = ws.focusedPanelId
}
```
Uses the existing
v2UUIDAnyhelper (line 3136), which already accepts UUIDs and short-form handle refs viav2ResolveHandleRef. No new helpers, no new dependencies.Compatibility
surface_idcallers: behavior unchanged (still hit the chain first).surface_ref/surfacecallers: now resolve correctly instead of silently retargeting.tab_idand other parameters: untouched.Files changed
Sources/TerminalController.swiftVerification
I do not have an Xcode build chain locally, so this PR has been code-reviewed but not Xcode-built. The change is a mechanical pattern substitution using a helper (
v2UUIDAny) that already exists and is already used elsewhere in the file (e.g. lines 3136–3144). I'd appreciate a maintainer applying the existing build/test pipeline. The companion test additions fortmux_split_ref_test.go-style coverage are in scope for a follow-up — happy to draft them once the approach here is validated.Related
__tmux-compat display-messageshort-form bug (tmux-compat display-message: short format variables (#S, #I, #P) not substituted #2750) — together these two fixes unblock `cmux omc team 1:codex,1:gemini "..."` end-to-end.Summary by CodeRabbit
Summary by cubic
Accepts
surface_refandsurfaceinsurface.send_text,surface.send_key,surface.read_text, andsurface.clear_historyso RPCs target the correct surface instead of falling back to the caller’s pane. Aligns inputs with outputs and addresses the asymmetric targeting bug (related to #2045).surface_id→surface_ref→surface, resolved viav2UUIDAny.not_foundwhen the ref is invalid; stop defaulting tows.focusedPanelId.surface_idcallers are unaffected.Written for commit cf02ce7. Summary will update on new commits.