Repository navigation
Route blocking v2 socket methods off main - #3340
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:
📝 WalkthroughWalkthroughRemoves the JSON-parsing gatekeeper for VM socket commands and exposes the VM response handler; centralizes V2 socket dispatch in TerminalController by parsing typed V2 requests, classifying execution policy, and routing calls to either a socket-worker path or the main actor, rejecting misrouted methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Socket)
participant Dispatch as Socket Dispatch
participant Parser as V2 Request Parser
participant Policy as Execution Policy
participant SWHandler as Socket Worker
participant MainActor as Main Actor
participant CloudVM as Cloud VM Handler
Client->>Dispatch: send raw V2 JSON line
Dispatch->>Parser: parseV2SocketRequest(line)
Parser-->>Dispatch: { method, id, params }
Dispatch->>Policy: executionPolicy(forV2Method:)
Policy-->>Dispatch: (.socketWorker or .mainActor)
alt .socketWorker
Dispatch->>SWHandler: socketWorkerV2Response(method,id,params)
alt method == vm.*
SWHandler->>CloudVM: socketWorkerCloudVMResponse(method,id,params)
CloudVM-->>SWHandler: vm result
SWHandler-->>Client: response
else known worker method
SWHandler-->>Client: response
else unknown
SWHandler-->>Client: method_not_found
end
else .mainActor
Dispatch->>MainActor: processV2Command / v2MainSync
MainActor-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 consolidates the three scattered socket-worker bypass functions (
Confidence Score: 4/5Safe to merge; the refactoring is logically sound and the only finding is unreachable dead code left in the main-actor switch. All findings are P2 (dead code, no runtime impact). The execution-policy routing is consistent across both the socket path and the in-process handleSocketLine path, and the defensive guard in processV2Command provides a correct safety net. Sources/TerminalController.swift — the auth.* and vm.* cases inside processV2Command's switch should be removed. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Socket line received] --> B[authResponseIfNeeded]
B -- "auth required response" --> Z[Return response]
B -- "pass" --> C[processCommandUsingSocketExecutionPolicy]
C --> D{socketWorkerV2ResponseIfNeeded}
D -- "executionPolicy == .socketWorker" --> E[withSocketCommandPolicy socket worker thread]
E --> F[socketWorkerV2Response switch]
F --> F1["auth.status / auth.begin_sign_in / auth.sign_out semaphore to @MainActor"]
F --> F2["feedback.submit / feed.push / feed.*.reply"]
F --> F3["vm.* to socketWorkerCloudVMResponse"]
F --> F4["default to method_not_found"]
F1 & F2 & F3 & F4 --> Z
D -- "executionPolicy == .mainActor or non-v2 command" --> G[v2MainSync to processCommand @MainActor dispatch]
G --> H[processV2Command]
H --> I{executionPolicy guard}
I -- ".socketWorker blocked" --> J[invalid_dispatch error]
I -- ".mainActor ok" --> K[main-actor switch system.* / feed.jump / etc.]
K --> Z
|
![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
🤖 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 1841-1857: Detect and short‑circuit socket-worker requests made
from the main thread: in handleSocketLine(_:) check Thread.isMainThread and call
socketWorkerV2ResponseIfNeeded(for: line) (or equivalent predicate) and if it
returns non‑nil, return a fast error response (e.g. "socket worker requests
cannot be made from main thread") instead of dispatching to
processCommandUsingSocketExecutionPolicy(_:); leave other paths unchanged and
add a TODO comment to route these into an async/v2MainSync path later so v2
handlers that need `@MainActor` work use v2MainSync rather than blocking the main
thread.
🪄 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: bc7f627b-d763-4e93-8220-59a04f4d58b3
📒 Files selected for processing (2)
Sources/Cloud/VMClientSocketCommands.swiftSources/TerminalController.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2597d88b01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard command.hasPrefix("{"), | ||
| let data = command.data(using: .utf8), | ||
| let dict = (try? JSONSerialization.jsonObject(with: data, options: [])) as? [String: Any] else { |
There was a problem hiding this comment.
handleSocketLine now routes through processCommandUsingSocketExecutionPolicy, but parseV2SocketRequest only accepts strings that literally start with {. If an in-process caller passes a valid JSON-RPC line with leading whitespace (which previously worked because processCommand trimmed first), worker-only methods like feed.push/auth.status skip worker dispatch and then hit processV2Command's new invalid_dispatch guard. This creates a behavior regression for formatted or indented JSON input on the public in-process entry point.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in the current branch: parseV2SocketRequest trims leading whitespace before JSON parsing, and the dogfood stress run included leading-whitespace worker RPCs successfully.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1408-1412: Make executionPolicy(forV2Method:) the authoritative
classifier for "system.top" by treating methods equal to "system.top" (in
addition to vm.* and socketWorkerV2Methods.contains) as returning .socketWorker;
then remove or stop special-casing "system.top" in
socketWorkerSystemTopResponseIfNeeded so it no longer bypasses the
executionPolicy logic and the invalid_dispatch checks in the dispatch paths
become effective.
🪄 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: 76149363-5785-4520-b20d-4cd9e17b27b2
📒 Files selected for processing (1)
Sources/TerminalController.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TerminalController.swift (1)
1408-1502:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFold
system.topinto the worker policy.
system.topstill bypasses the new execution-policy gate: it is excluded fromsocketWorkerV2Methods, handled bysocketWorkerSystemTopResponseIfNeeded, and still treated as.mainActorinprocessV2Command(_:). That leaves the blocking path outside the off-main routing this refactor is supposed to enforce.As per coding guidelines, "If adding a new socket command, default to off-main handling; require an explicit reason in code comments when main-thread execution is necessary."
Suggested consolidation
private nonisolated static let socketWorkerV2Methods: Set<String> = [ "auth.status", "auth.begin_sign_in", "auth.sign_out", "feedback.submit", "feed.push", "feed.permission.reply", "feed.question.reply", "feed.exit_plan.reply", + "system.top", ] private nonisolated func socketWorkerV2Response(_ request: V2SocketRequest) -> String { switch request.method { @@ case "feed.exit_plan.reply": return v2Result(id: request.id, v2FeedExitPlanReply(params: request.params)) + case "system.top": + return v2Result(id: request.id, v2SystemTop(params: request.params)) case let method where method.hasPrefix("vm."): return socketWorkerCloudVMResponse(method: method, id: request.id, params: request.params) default: return v2Error(id: request.id, code: "method_not_found", message: "Unknown method") } } - - private nonisolated func socketWorkerSystemTopResponseIfNeeded(for command: String) -> String? { - guard let request = parseV2SocketRequest(command), - request.method == "system.top" else { - return nil - } - - return withSocketCommandPolicy(commandKey: request.method, isV2: true) { - v2Result(id: request.id, v2SystemTop(params: request.params)) - } - }Also applies to: 2273-2279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 1408 - 1502, The review points out that "system.top" is still handled on the main actor instead of being folded into the off-main worker policy; update the routing so "system.top" is treated like other socketWorker V2 methods. Add "system.top" to the socketWorkerV2Methods set (or change executionPolicy(forV2Method:) to treat request.method == "system.top" as .socketWorker), remove or retire socketWorkerSystemTopResponseIfNeeded and any direct .mainActor handling in processV2Command(_:) so v2 system.top requests are dispatched via withSocketCommandPolicy and handled by v2SystemTop through the same worker path; keep an explicit code comment only if you intentionally want main-thread execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Around line 1408-1502: The review points out that "system.top" is still
handled on the main actor instead of being folded into the off-main worker
policy; update the routing so "system.top" is treated like other socketWorker V2
methods. Add "system.top" to the socketWorkerV2Methods set (or change
executionPolicy(forV2Method:) to treat request.method == "system.top" as
.socketWorker), remove or retire socketWorkerSystemTopResponseIfNeeded and any
direct .mainActor handling in processV2Command(_:) so v2 system.top requests are
dispatched via withSocketCommandPolicy and handled by v2SystemTop through the
same worker path; keep an explicit code comment only if you intentionally want
main-thread execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80273261-bc52-45db-a429-f1975f2ad10a
📒 Files selected for processing (1)
Sources/TerminalController.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1458-1464: Validate and reject non-positive timeout_seconds before
calling AuthManager.shared.beginSignInAndAwait: parse the incoming value from
request.params using the v2 numeric-parsing helper (instead of blindly casting
to Double into timeoutSeconds), check that the resulting timeout is > 0, and if
not return an invalid_params error (or clamp to a minimum positive value) rather
than passing it to beginSignInAndAwait; update the logic around timeoutSeconds
and the Task that calls beginSignInAndAwait to only run when the validated
timeout is positive.
🪄 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: 1f7b39b2-4158-494a-a5c0-7eacd5634312
📒 Files selected for processing (1)
Sources/TerminalController.swift
| let timeoutSeconds = (request.params["timeout_seconds"] as? Double) ?? 300 | ||
| let semaphore = DispatchSemaphore(value: 0) | ||
| nonisolated(unsafe) var signedIn = false | ||
| Task { @MainActor in | ||
| signedIn = await AuthManager.shared.beginSignInAndAwait( | ||
| timeout: timeoutSeconds | ||
| ) |
There was a problem hiding this comment.
Reject non-positive timeout_seconds values.
A missing timeout falls back to 300, but a present 0 or negative value still gets passed into beginSignInAndAwait. For a socket-exposed timeout parameter, that should return invalid_params (or at least be clamped) instead of letting malformed input change auth behavior.
Suggested fix
- let timeoutSeconds = (request.params["timeout_seconds"] as? Double) ?? 300
+ let timeoutSeconds: TimeInterval
+ if let rawTimeout = request.params["timeout_seconds"] {
+ guard let seconds = rawTimeout as? NSNumber, seconds.doubleValue > 0 else {
+ return v2Error(
+ id: request.id,
+ code: "invalid_params",
+ message: "auth.begin_sign_in requires `timeout_seconds` > 0"
+ )
+ }
+ timeoutSeconds = seconds.doubleValue
+ } else {
+ timeoutSeconds = 300
+ }Based on learnings: In Swift request/JSON handlers in Sources/TerminalController.swift, prefer the v2 numeric-parsing helpers and clamp/validate timeout inputs as appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 1458 - 1464, Validate and
reject non-positive timeout_seconds before calling
AuthManager.shared.beginSignInAndAwait: parse the incoming value from
request.params using the v2 numeric-parsing helper (instead of blindly casting
to Double into timeoutSeconds), check that the resulting timeout is > 0, and if
not return an invalid_params error (or clamp to a minimum positive value) rather
than passing it to beginSignInAndAwait; update the logic around timeoutSeconds
and the Task that calls beginSignInAndAwait to only run when the validated
timeout is positive.
…tch-crash # Conflicts: # Sources/TerminalController.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TerminalController.swift (1)
1458-1464:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
timeout_secondsbefore calling auth wait.Line 1458 currently defaults missing values, but it still accepts present
0/negative values and forwards them. Please reject invalid values withinvalid_params(or clamp to a minimum positive value) beforebeginSignInAndAwait.Suggested fix
- case "auth.begin_sign_in": - let timeoutSeconds = (request.params["timeout_seconds"] as? Double) ?? 300 + case "auth.begin_sign_in": + let timeoutSeconds: TimeInterval + if let rawTimeout = request.params["timeout_seconds"] { + guard let n = rawTimeout as? NSNumber, n.doubleValue > 0 else { + return v2Error( + id: request.id, + code: "invalid_params", + message: "auth.begin_sign_in requires `timeout_seconds` > 0" + ) + } + timeoutSeconds = n.doubleValue + } else { + timeoutSeconds = 300 + } let semaphore = DispatchSemaphore(value: 0) nonisolated(unsafe) var signedIn = false Task { `@MainActor` in signedIn = await AuthManager.shared.beginSignInAndAwait( timeout: timeoutSeconds ) semaphore.signal() }Based on learnings: In Swift request/JSON handlers in
Sources/TerminalController.swift, prefer v2 numeric parsing helpers and validate/clamp timeout inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 1458 - 1464, Validate and reject or clamp the timeout_seconds value before calling AuthManager.shared.beginSignInAndAwait: parse timeout_seconds from request.params using the v2 numeric parsing helper, ensure it is a positive nonzero value (or clamp to a minimum like 1.0), and if invalid respond with an invalid_params error instead of forwarding 0/negative to beginSignInAndAwait; update the code around the timeoutSeconds assignment and the Task { `@MainActor` ... beginSignInAndAwait(...) } invocation to use the validated/clamped value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Around line 1458-1464: Validate and reject or clamp the timeout_seconds value
before calling AuthManager.shared.beginSignInAndAwait: parse timeout_seconds
from request.params using the v2 numeric parsing helper, ensure it is a positive
nonzero value (or clamp to a minimum like 1.0), and if invalid respond with an
invalid_params error instead of forwarding 0/negative to beginSignInAndAwait;
update the code around the timeoutSeconds assignment and the Task { `@MainActor`
... beginSignInAndAwait(...) } invocation to use the validated/clamped value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe46626d-d3d7-49aa-9eee-39feae200055
📒 Files selected for processing (1)
Sources/TerminalController.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16aa5f9fea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let rawWorkspaceId = event.workspaceId, | ||
| let workspaceId = v2UUID(["workspace_id": rawWorkspaceId], "workspace_id"), | ||
| let tabManager = v2ResolveWorkspaceOwner(workspaceId) | ||
| let workspaceId = UUID(uuidString: rawWorkspaceId.trimmingCharacters(in: .whitespacesAndNewlines)) |
There was a problem hiding this comment.
v2ApplyPromptSubmitSideEffects now parses event.workspaceId with UUID(uuidString:), so feed.push UserPromptSubmit events that carry a workspace handle/ref are ignored. This is a regression from the previous v2UUID(...) path, which also resolved handle refs, and it breaks prompt-submit side effects (message preview + iMessage-mode reordering) for valid feed payloads that provide workspace_ref/handle-style IDs (see feedWorkspaceId accepting workspace_ref in CLI/cmux.swift).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ff5280c. Prompt-submit side effects now resolve workspace handles/refs inside the main-actor bridge before touching TabManager.
— Claude Code
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch was successfully deployed
1 participant
Summary
Verification
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination platform=macOS -derivedDataPath /tmp/cmux-fdcrsh-check build CODE_SIGNING_ALLOWED=NO./scripts/reload.sh --tag fdcrsh/tmp/cmux-debug-fdcrsh.sock; RPC returned{ "status": "timed_out" }, app process stayed alive, and no newcmux DEVcrash report was written.Summary by cubic
Unifies V2 socket routing behind a single execution policy and runs blocking methods on the socket worker (including
system.top). Also trims inputs and fixes feed prompt workspace resolution to keep main-actor dispatch safe.Refactors
auth.*,feedback.submit,feed.push,feed.*.reply, allvm.*, andsystem.topto the socket worker; main dispatch rejects these withinvalid_dispatch.parseV2SocketRequest(trims input),socketWorkerV2ResponseIfNeeded,socketWorkerV2Response, andprocessCommandUsingSocketExecutionPolicy;handleSocketLineisnonisolated.socketWorkerCloudVMResponse(method:id:params:).Bug Fixes
system.topoff the main thread; long-running RPCs no longer block the app and repro still returns{"status":"timed_out"}.AppDelegate.tabManagerFor; feed push/reply handlers arenonisolatedto run on the worker.Written for commit 7c282c4. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Refactor
Bug Fixes