Repository navigation
fix(socket): return not_found when surface_id is provided but unresolvable - #2150
Conversation
…nresolvable Previously, v2SurfaceSendText, v2SurfaceSendKey, v2SurfaceClearHistory, and v2SurfaceReadText would silently fall back to ws.focusedPanelId when a caller supplied a surface_id that could not be resolved (e.g. a stale ref or an ordinal whose mapping had not yet been registered). This caused two distinct bugs: - manaflow-ai#2042: Commands like `cmux send --surface surface:9999` would succeed (exit 0) and deliver input to the focused pane instead of returning an error, making automation that targets specific surfaces unreliable. - manaflow-ai#2045: When the fallback landed on a browser panel, the subsequent ws.terminalPanel(for:) check failed and returned "Surface is not a terminal", making valid terminal surfaces appear broken when addressed by ref. The fix adds an explicit check: if params["surface_id"] is present but v2UUID() returns nil (resolution failure), we immediately return a not_found error instead of falling back to the focused pane. When surface_id is absent, the existing focused-pane fallback is preserved for backward compatibility. Fixes manaflow-ai#2042, Fixes manaflow-ai#2045 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@anthhub 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 (1)
📝 WalkthroughWalkthroughThis change modifies four v2 terminal surface operations in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 a silent-fallback bug in four socket surface commands: when Key changes:
Notes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Socket command received\nwith params] --> B{params has\nsurface_id?}
B -- Yes --> C[v2UUID resolves\nsurface_id]
C --> D{UUID resolved?}
D -- No --> E[return not_found\n'Surface not found for\nthe given surface_id']
D -- Yes --> F[surfaceId = resolved UUID]
B -- No --> G[surfaceId = ws.focusedPanelId\nfallback preserved]
F --> H{surfaceId != nil?}
G --> H
H -- No --> I[return not_found\n'No focused surface']
H -- Yes --> J[ws.terminalPanel lookup]
J --> K{Is terminal\npanel?}
K -- No --> L[return invalid_params\n'Surface is not a terminal']
K -- Yes --> M[Execute command\nsend_text / send_key /\nclear_history / read_text]
Reviews (1): Last reviewed commit: "fix(socket): return not_found error when..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard surfaceId != nil else { | ||
| result = .err(code: "not_found", message: "Surface not found for the given surface_id", data: nil) | ||
| return | ||
| } | ||
| } else { | ||
| surfaceId = ws.focusedPanelId | ||
| } |
There was a problem hiding this comment.
The guard surfaceId != nil pattern leaves surfaceId as a UUID? even after the guard passes, which is why the outer guard let surfaceId is still needed. The more idiomatic Swift approach combines both into a single binding and eliminates the double lookup:
if params["surface_id"] != nil {
guard let resolvedId = v2UUID(params, "surface_id") else {
result = .err(code: "not_found", message: "Surface not found for the given surface_id", data: nil)
return
}
surfaceId = resolvedId
} else {
surfaceId = ws.focusedPanelId
}This avoids the guard <optional> != nil compiler-warning-prone pattern and makes the flow clearer. The same refactor applies to the identical blocks in v2SurfaceSendKey (line ~5344), v2SurfaceClearHistory (line ~5387), and v2SurfaceReadText (line ~5447).
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!
| let surfaceId: UUID? | ||
| if params["surface_id"] != nil { | ||
| surfaceId = v2UUID(params, "surface_id") | ||
| guard surfaceId != nil else { | ||
| result = .err(code: "not_found", message: "Surface not found for the given surface_id", data: nil) | ||
| return | ||
| } | ||
| } else { | ||
| surfaceId = ws.focusedPanelId | ||
| } |
There was a problem hiding this comment.
CLAUDE.md specifies a two-commit regression-test policy for bug fixes (failing test first, then fix). The Python socket test infrastructure in tests_v2/ is capable of exercising this exact path — e.g., calling surface.send_text with a bogus surface_id like "surface:9999" and asserting the response code is not_found.
Both #2042 and #2045 are testable end-to-end through the existing cmux.py client. If a test is impractical for some infra reason, that should be stated explicitly per the policy.
This same observation applies to all four affected functions.
| if params["surface_id"] != nil { | ||
| surfaceId = v2UUID(params, "surface_id") | ||
| guard surfaceId != nil else { | ||
| result = .err(code: "not_found", message: "Surface not found for the given surface_id", data: nil) |
There was a problem hiding this comment.
surface_id in data
The not_found error response here omits the input value in data: nil. Downstream callers and CLI error output would benefit from knowing which surface_id string failed to resolve. All other surface-related errors in these functions (e.g., invalid_params: Surface is not a terminal) include the resolved UUID. Including the raw string here would aid debugging:
result = .err(
code: "not_found",
message: "Surface not found for the given surface_id",
data: ["surface_id": params["surface_id"] as? String ?? ""]
)The same applies to all four occurrences (lines ~5345, ~5388, ~5448).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Thank you for the contribution! |
…nresolvable (manaflow-ai#2150) Previously, v2SurfaceSendText, v2SurfaceSendKey, v2SurfaceClearHistory, and v2SurfaceReadText would silently fall back to ws.focusedPanelId when a caller supplied a surface_id that could not be resolved (e.g. a stale ref or an ordinal whose mapping had not yet been registered). This caused two distinct bugs: - manaflow-ai#2042: Commands like `cmux send --surface surface:9999` would succeed (exit 0) and deliver input to the focused pane instead of returning an error, making automation that targets specific surfaces unreliable. - manaflow-ai#2045: When the fallback landed on a browser panel, the subsequent ws.terminalPanel(for:) check failed and returned "Surface is not a terminal", making valid terminal surfaces appear broken when addressed by ref. The fix adds an explicit check: if params["surface_id"] is present but v2UUID() returns nil (resolution failure), we immediately return a not_found error instead of falling back to the focused pane. When surface_id is absent, the existing focused-pane fallback is preserved for backward compatibility. Fixes manaflow-ai#2042, Fixes manaflow-ai#2045 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
surface_idis explicitly provided in a socket command but fails to resolve (stale ref, unknown ordinal, etc.), the four affected functions now return anot_founderror instead of silently falling back tows.focusedPanelId.surface_idis not provided, the existing focused-pane fallback is preserved unchanged (backward compatible).v2SurfaceSendText,v2SurfaceSendKey,v2SurfaceClearHistory,v2SurfaceReadText(all inSources/TerminalController.swift).Root Cause
v2UUID(params, "surface_id")callsv2ResolveHandleRef()internally. If the ref is unresolvable, it returnsnil. The previous patternv2UUID(params, "surface_id") ?? ws.focusedPanelIdsilently promoted thisnilto the focused panel, causing two distinct failure modes:ws.terminalPanel(for:)guard fails and returnsinvalid_params: Surface is not a terminal, making valid terminal surfaces appear broken when addressed by ref.Fix
Test Plan
cmux send --surface surface:9999 "hello"→ERROR: not_found: Surface not found for the given surface_id(non-zero exit)cmux send-key --surface surface:9999 Enter→ same errorcmux read-screen --surface surface:9999→ same errorcmux send "hello"(no--surface) → operates on focused pane (backward compat preserved)cmux send --surface surface:1 "hello"with a valid ref → succeeds as beforecmux read-screen --surface surface:29with a valid terminal ref → returns screen contentFixes #2042, Fixes #2045
🤖 Generated with Claude Code
Summary by cubic
Return
not_foundwhen a socket command includes an unresolvablesurface_id, instead of silently targeting the focused pane. Behavior with nosurface_idis unchanged. Fixes #2042 and #2045.v2SurfaceSendText,v2SurfaceSendKey,v2SurfaceClearHistory, andv2SurfaceReadTextto error withnot_foundif the providedsurface_idcannot be resolved.surface_idis not provided, continue falling back tows.focusedPanelId(backward compatible).Written for commit 4c977f1. Summary will update on new commits.
Summary by CodeRabbit