control socket: move CLI command handling off the main thread (tranches A-E of #5757) - #7357
Merged
Merged
Conversation
…-hop timing Commit A of the CLI off-main migration (#5757). Dispatch infrastructure only; no command bodies move lanes and all responses stay byte-identical. - Parse once per v2 line: processCommandUsingSocketExecutionPolicy strict-parses on the socket-worker thread and hands the parsed ControlRequest to the worker lane or into the main hop; processV2Command no longer re-parses the same line on the main thread. runV2CommandLine keeps its parse-on-calling-thread contract for in-process callers. - Encode off main for the v2 coordinator path: the single v2MainSync hop returns the coordinator's typed ControlCallResult (or the legacy switch's already-encoded string), and JSON bridging/serialization runs on the worker after the hop. Legacy switch cases keep encoding inline for now (TODO in v2LegacyMainActorResponse). - v1 worker lane plumbing: ControlCommandExecutionPolicy.init(forV1Command:) with a ping-only socketWorkerV1Commands set (+ mainThreadCallable twin), socketWorkerV1ResponseIfHandled mirroring the v2 worker entry, and the main-thread invalid-dispatch guard extended to v1 (plain ERROR string form). - Per-hop timing in v2MainSync: queue-wait and body duration per hop, emitted as a com.cmux.socket "main-hop" os_signpost interval keyed by the active command, and accumulated per command (DEBUG) so socket.command.end slow logs show total-vs-main-hop time. String formatting stays behind the enabled checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Commit B1 of the CLI off-main migration (#5757), on top of the tranche-A dispatch plumbing. v1 verbs migrated (all policy socketWorker, mainThreadCallable): set_status, report_meta, report_meta_block, clear_status, clear_meta, clear_meta_block, list_status, list_meta, list_meta_blocks, set_agent_pid, set_agent_lifecycle, agent_hibernation, clear_agent_pid, log, clear_log, list_log, set_progress, clear_progress, report_git_branch, clear_git_branch, report_pr, report_review, clear_pr, report_pr_action, report_ports, clear_ports, report_pwd, report_shell_state, report_tty, ports_kick. v2 twins migrated: surface.report_pwd, surface.report_shell_state, surface.report_tty, surface.ports_kick. Shape: the coordinator's telemetry bodies become nonisolated (handleSidebarTelemetryV1), shared verbatim by the socket worker lane and the main-actor handleSidebarV1 dispatch, with the seam threaded as a parameter. Parse/tokenize/validate/format run on the connection thread; deferred mutations keep their ordered TerminalMutationBus enqueues (now via nonisolated Schedule* witnesses, zero main hops on the scoped hot paths); each resolution-dependent command crosses to the main actor exactly once via the new controlSidebarOnMain hop primitive (v2MainSync underneath, inline on main). report_pwd/report_meta_block/report_shell_state select their reply inside that single hop over precomputed parse results so the legacy TabManager-availability-before-parse-error precedence is byte-identical. set_agent_lifecycle's vault-registry disk IO moves to the worker thread; only the tab/panel-directory allowlist snapshot hops. clear_meta_block keeps its sync hop so the "OK" vs "OK (key not found)" reply distinction is unchanged. list_* return existing Sendable snapshots from one hop and format off-main. report_tty ordering: the v1 scoped path stays a bus enqueue ordered FIFO with scoped ports_kick on the same bus; the v1 fallback and the v2 surface.report_tty relay path (the deliberately-synchronous first relay report) keep their registration inside the synchronous hop, so the reply is written only after the registration is visible to later commands on any connection. The v2 twins run socketWorkerV2Response's new coordinator-hop branch: one v2MainSync around the shared v2MainActorResponse (known-ref refresh + coordinator body), with JSON encode on the worker — byte-identical replies by construction. All migrated verbs are mainThreadCallable: every body is non-blocking end-to-end when run inline on the main thread (bus enqueues plus hops that collapse inline), which keeps in-process main-thread callers and the cmuxTests that drive handleSocketLine on the main actor (AgentHibernationTests, WorkspacePullRequestSidebarTests, TerminalAndGhosttyTests) on their previous inline semantics. swift test --package-path Packages/macOS/CmuxControlSocket: 186 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…title onto the worker lane Commit B2 of the CLI off-main migration (#5757), on top of B1. v1 verbs migrated (policy socketWorker, mainThreadCallable): notify, notify_surface, notify_target, notify_target_async, list_notifications, clear_notifications. v2 methods migrated: notification.create, notification.create_for_surface, notification.create_for_target, notification.create_for_caller (app-side legacy-switch resolver), and workspace.set_auto_title. notification.reconcile is left alone with a comment: it is a mobile-host data-plane verb (v2MobileDispatch), not a control-socket method, so no execution policy applies. v1 shape: the bodies become nonisolated on TerminalController. Payload/ argument parsing (parseNotificationPayload, the split/UUID parses, parseOptions/parseSidebarMutationTabTarget/parseOptionalPanelIdOption) runs on the connection thread. notify_target_async and clear_notifications are pure lock-guarded TerminalMutationBus enqueues with ZERO main hops (hooks nohup them and ignore the reply; the bus already returned OK before apply). notify/notify_surface/notify_target/list_notifications keep exactly ONE v2MainSync hop; the legacy guard order (TabManager availability before the usage errors) runs inside that hop over precomputed parse results, so multi-error requests report the same error byte-for-byte, and each reply is written only after the synchronous delivery — notify replies were never fire-and-forget and stay that way. notify_target's two former per-branch hops collapse into one (the branches were mutually exclusive per request). list_notifications snapshots the store + tab titles in the hop and does ISO8601/percent-escape formatting and the join on the worker. v2 shape: the five methods route through socketWorkerV2Response's coordinator-hop branch from B1 — one v2MainSync around the shared v2MainActorResponse (known-ref refresh + coordinator + legacy switch), encode on the worker. Byte-identical replies by construction; the synchronous hop preserves create-then-list read-your-write ordering (the create reply, which echoes resolved workspace/surface/window ids, lands only after the store mutation) and set_auto_title's apply-then-reply contract. All migrated verbs are mainThreadCallable: every body is non-blocking end-to-end when run inline on main (bus enqueues plus inline-collapsing hops), which keeps main-actor cmuxTests callers (TerminalNotificationClearAllTests clear_notifications, SetAutoTitleSocketTests workspace.set_auto_title) on their previous inline semantics; no test changes were needed. swift test --package-path Packages/macOS/CmuxControlSocket: 188 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Commit C of the CLI off-main migration (#5757), on top of tranche B. Reimplements the prior-art prototype 415f5af ("Run surface.read_text off the main actor to fix heavy-load beachball") on today's dispatch plumbing: read-text formatting was ~135/289 of main-thread busy samples under agent load, the single biggest main-thread cost in the socket path. surface.read_text (policy socketWorker, NOT mainThreadCallable): the method moves OUT of the @mainactor ControlCommandCoordinator — its dispatch case, handler, ControlSurfaceContext witness, ControlSurfaceReadTextResolution enum, and test stub are dropped — into the app-side worker body TerminalController.v2SurfaceReadText, because the coordinator seam cannot host a capture-on-main/format-off-main split. The body takes ONE minimal v2MainSync hop: known-ref refresh, routing-selector resolution (registry reads), TabManager guard, the lines>0 validation (kept AFTER the TabManager guard so multi-error requests keep the legacy error precedence), the global-dock vs workspace target resolution (including the dock branch the witness grew after the prototype), the Ghostty ghostty_surface_read_text capture, and the success-path ref minting in the payload's literal order (workspace, surface, window). The scrollback tail/merge, candidate scoring, base64 encode, and reply encode run on the socket-worker thread. Response shape, error codes, and routing precedence are byte-faithful; the main-lane comment now points at the worker body, and runV2CommandLine (zero in-repo callers) answers method_not_found for it, as in the prior art. v1 read_screen (new terminalReadV1Commands policy set, NOT mainThreadCallable): same split. parseReadScreenArgs and the surface-arg trim run on the connection thread; the selected-tab/panel resolution, liveSurfaceForGhosttyAccess check, and raw capture take one hop (the legacy main-actor tabManager guard moves inside it, same reply order); the tail/merge/scoring plus the base64 encode->trim->decode round-trip are kept verbatim off-main so replies are byte-identical to the legacy readTerminalTextBase64 pipeline. The processCommand case stays and shares the same nonisolated body. Neither verb is mainThreadCallable: running multi-MB formatting inline on a main-thread caller is exactly the stall this lane move removes, and no in-process main-thread caller exists (audited cmuxTests: the only surface.read_text reference, CmuxEventBusTests, drives the event mapper, not dispatch; no test calls read_screen). Both invalid_dispatch guards are pinned in the new policy tests, and the exact-set v1 pin test now carries the read_screen exception. New behavioral test testSurfaceReadTextIsServicedOnTheWorkerLane (cmuxTests/TerminalControllerSocketSecurityTests.swift): released-surface workspace, main-thread invalid_dispatch pins for both verbs, then background round-trips asserting the byte-exact legacy error replies — which also catch a policy/worker-switch drift loudly (the "has no worker handler" backstop and a method_not_found re-lift both fail the assert). Unlike the set_status worker-lane proof, the round-trip runs with the main actor free: a read's reply legitimately requires its one capture hop, so "reply while main is blocked" cannot hold for this lane. swift test --package-path Packages/macOS/CmuxControlSocket: 191 tests green. swift-file-length-budget.tsv refreshed via --write-budget (absorbs this commit plus the tranche A/B growth that had not been re-baselined). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Commit D of the CLI off-main migration (#5757), on top of tranche C. These are the implicit handle-normalization reads nearly every CLI invocation pays 1-3 of. v2 methods migrated (policy socketWorker, all mainThreadCallable): surface.list, surface.current, workspace.list, workspace.current, window.list, window.current, window.displays, pane.list, pane.surfaces, system.identify, system.tree. v1 twins migrated (new resolutionReadV1Commands set, all mainThreadCallable): list_windows, current_window, list_workspaces, list_surfaces, current_workspace. v2 shape: the coordinator bodies become nonisolated, shared verbatim by the socket worker lane (the new nonisolated ControlCommandCoordinator.handleSocketWorkerV2 entrypoint, dispatched from socketWorkerV2Response(handling:)) and the main-actor handle() dispatch (where the hop collapses inline, keeping runV2CommandLine and main-thread in-process callers byte-identical). Each body takes ONE hop via the new ControlCommandContext.controlResolveOnMain primitive — the app conformer forwards to v2MainSync and runs v2RefreshKnownRefs FIRST, mirroring the main-lane v2MainActorResponse preamble byte-for-byte — and inside that hop does the routing-selector resolution (ControlHandleRegistry reads; the registry stays main-confined), the EXISTING Sendable snapshot witness call, and a ref mint pass in the payload's exact literal order (per-row structs so a missing ref is impossible by construction; the refresh has already minted every live-topology id, so ordinals cannot drift). The JSON row build and the encode (same ControlResponseEncoder as the main lane) run on the worker. workspaceSummaryPayload and the four system.tree node payload builders become nonisolated over pre-minted refs; orNull / string / surfaceResumeBindingPayload become nonisolated (pure). system.identify keeps its whole resolution — focused window, caller context validation, ref minting — inside the single hop, so the payload is the same one-snapshot read the main lane produced; only the encode leaves the main thread. system.tree (the widest read) snapshots the full window/workspace/pane/surface tree, runs its legacy-ordered error selection (workspace_id parse error, then the three routing invalid_params shapes, then window-not-found, then workspace-not-found with its in-hop-minted ref), and mints the parallel ref tree inside the hop; the full tree-to-JSON mapping happens off-main. Its unwired-context (nil seam) flow keeps the legacy inline main-actor behavior instead of inventing an error, with a loud fallback for the impossible off-main-nil case. window.list / window.displays keep their legacy nil-context ok-with-empty replies. v1 shape: the five TerminalController bodies become nonisolated; the main-actor tabManager guard moves inside the single v2MainSync hop with the snapshot (same reply-selection order), and line formatting/joins run on the worker. The legacy processCommand cases keep calling the same bodies. All 16 verbs are mainThreadCallable: non-blocking single-hop snapshot reads whose hop collapses inline, and cmuxTests drive them through handleSocketLine on the main actor (AppDelegateIssue2907RoutingTests: workspace.list/current, window.list/current, surface.list/current, pane.list, system.tree; MobileHostAuthorizationTests + TerminalAndGhostty- Tests: workspace.list) — those tests keep their previous inline semantics, so no test changes were needed. Known deliberate cost: the zero-caller main-lane path (runV2CommandLine) now refreshes known refs twice for these verbs (once in v2MainActorResponse, once in the inline-collapsing hop); the refresh is idempotent. Package policy pin tests updated: exact-set v1 pin gains the resolution-read family, the v2/v1 defaults tests drop the migrated names (and now pin the focus-intent verbs to the main lane), and two new tests pin all 16 verbs to socketWorker(mainThreadCallable: true). The package test stub gains the controlResolveOnMain default (inline main hop, no refresh — package fakes have no app topology, matching the pre-migration coordinator tests whose refresh also lived app-side). swift test --package-path Packages/macOS/CmuxControlSocket: 193 tests green. swiftc -parse on all touched files. swift-file-length-budget.tsv refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Commit E of the CLI off-main migration (#5757), on top of tranche D. v2 methods migrated (policy socketWorker, both mainThreadCallable): surface.send_text, surface.send_key. v1 twins migrated (new terminalSendV1Commands set, all mainThreadCallable): send, send_key, send_surface, send_key_surface, and the DEBUG-only send_workspace. v2 shape: surfaceSendText/surfaceSendKey become nonisolated coordinator bodies shared by the worker lane (handleSocketWorkerV2 gains the two cases) and the main-actor handle() dispatch. Text/key extraction runs on the worker (pure param reads); the single controlResolveOnMain hop does the known-ref refresh, routing resolution, the TabManager guard, the missing-text/missing-key check (selected AFTER the guard so multi-error requests keep the legacy precedence), the existing controlSurfaceSendText/SendKey witness (target resolve + main-bound Ghostty input injection + forceRefresh, including the global-dock branch), and the success-ref minting in the payload's literal order (workspace, surface, window; error resolutions mint nothing, like the legacy in-payload build). surfaceSendResult becomes nonisolated over the pre-minted refs, and the controlSurfaceInputStrings witness becomes nonisolated (a pure bundle lookup) so the localized error-string selection also leaves the main thread. The reply is load-bearing (queued/input_queue_full/process_exited drive caller retry), so the hop stays synchronous — no fire-and-forget. surface.send_text MUST be mainThreadCallable: AppDelegate's handleFeedRequestSendText (the feed send-text path, AppDelegate.swift:9249) drives it through handleSocketLine on the main thread; on that path the policy routes to the worker branch inline and the hop collapses, i.e. the exact legacy main-lane execution. Verified by reading the call site. surface.send_key shares the identical non-blocking body shape, so it carries the same policy rather than an asymmetric guard. v1 shape: the five bodies become nonisolated on TerminalController. The target/text splits, the \n->\r/\t unescaping, and send_workspace's UUID parse run on the worker; ONE v2MainSync hop keeps the legacy evaluation order (the main-actor tabManager guard first, then the usage/UUID parse errors over precomputed results, then resolveTerminalPanel / focused-terminal / cross-window workspace resolution, then sendInputResult/sendNamedKeyResult + the legacy per-verb forceRefresh reasons); the reply mapping — "OK", the usage strings, "Unknown key", and the localized terminal*SocketError statics (now nonisolated computed lookups) — runs on the worker over a shared V1SendHopOutcome enum. Per-connection input ordering is preserved: one worker thread per connection stays serial, and each send's synchronous hop serializes with every other main-actor mutation exactly as the legacy main-lane FIFO did. send_workspace's worker case is #if DEBUG; in Release it replies the legacy unknown-command string (the debug.sidebar.simulate_drag precedent), since the policy set is compiled unconditionally. All seven verbs are mainThreadCallable (narrow non-blocking hop, no semaphores or cross-thread waits): required for surface.send_text (feed path) and send_workspace (TerminalAndGhosttyTests' testDaemonSendWorkspaceQueuesColdControlInputInsteadOfReportingDroppedOK drives handleSocketLine on the main actor and still gets its inline "OK" + bus-deferred queue semantics); no cmuxTests changes were needed (audited: no other main-thread callers of send/send_key/send_surface/ send_key_surface/surface.send_key exist). Package pin tests updated: exact-set v1 pin gains the terminal-send family (callable set = worker set minus read_screen), the v1 defaults test drops send/send_key (replaced with workspace lifecycle verbs), and two new tests pin the send policies with the caller rationale. swift test --package-path Packages/macOS/CmuxControlSocket: 195 tests green. swiftc -parse on all touched files. swift-file-length-budget.tsv refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes on top of tranche E (issue #5757): - v2SurfaceReadText: reject unformattable snapshots BEFORE minting refs. terminalTextPayload's only failure predicate is snapshot shape (screen/ history/active all nil with scrollback, viewport nil without), so the worker body now applies that exact predicate in-hop and mints refs only when a success reply is guaranteed. The legacy build minted nothing on this error path, and dock-hosted ids are first-minted by the mint pass (not the refresh), so an error-path mint would have shifted kind:N ordinals for every later reply on the instance. - controlResolveOnMain docs (protocol + app conformer): the known-ref refresh covers only main-window workspace topology; dock-hosted surfaces are first-minted by each body's in-hop mint pass, so mint passes must preserve payload literal mint order for ordinal parity. - systemTree: the off-main nil-context backstop now fails loudly (assertionFailure + distinct internal_error) instead of a generic "unavailable" reply that made lane drift indistinguishable from routine TabManager unavailability. Unreachable in-app (the worker lane always passes its live seam). - v2MainActorResponse: LOCKSTEP comment tying the main-lane dispatch preamble to its worker-lane mirror controlResolveOnMain, plus a comment correcting the worker-lane read_text main-entry reply (invalid_dispatch from the policy guard, not method_not_found). swift test --package-path Packages/macOS/CmuxControlSocket: 195 green. Full app target compiled on the fleet builder (reload-cloud tag climn). swift-file-length-budget.tsv refreshed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hand-resolved against two main-side changes that landed since the branch base: - PR #7129 (agent-notification gating): the v1 notify family keeps its worker-lane shape; the 4-tuple parseNotificationPayload and the shouldDeliverAgentNotification gate read run on the socket-worker thread (both nonisolated; the settings read goes through the documented thread-safe DefaultsKey.value(in:) seam), and the gate verdict applies inside each hop at main's exact guard position so reply precedence is byte-identical. - PR #7144 (per-window docks): v2SurfaceReadText's dock branch ports from globalDockForRouting to windowDockForRouting + dockResultWindowId, mirroring the coordinator send witnesses' post-#7144 shape; the deleted controlSurfaceReadText witness stays deleted (its replacement carries the update). swift test --package-path Packages/macOS/CmuxControlSocket: 195 green. swift-file-length-budget.tsv regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds main-thread resolution hops for command and sidebar contexts, routes pane/surface/system/window/workspace handlers through those hops, removes ChangesWorker-lane dispatch migration
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 passed)
✨ 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 |
Contributor
Author
|
@codex review |
Contributor
Greptile SummaryThis PR moves control-socket command handling off the main thread to fix the blocking-main-thread issue under heavy agent load (#5757). Parsing, JSON encoding, and large reply formatting now run on the per-connection socket-worker thread; each migrated verb keeps at most one narrow
Confidence Score: 5/5Safe to merge. The threading contracts are sound: every migrated verb narrows to at most one synchronous main hop, ref minting stays inside the hop in payload order, and the package-test policy suite pins the exact worker/mainThreadCallable sets so future handler drift is caught at compile time. The architectural invariants are enforced mechanically — the policy tests pin exact command sets and the loud backstops catch any policy/handler drift before it reaches clients. The v2MainSync signposting infrastructure is properly guarded by #if DEBUG and .dynamicTracing, so Release builds pay no overhead. The coalescing logic in enqueueReplacingMainActorMutation follows the same established removeAll pattern as existing notification coalescing. Two concerns flagged in earlier review rounds (backstop error wording and v2SurfaceReadText lines/TabManager precedence) are the only open items and do not affect correctness for the common path. No files require special attention beyond the two items noted in prior review threads. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as CLI Client
participant SW as Socket Worker Thread
participant Policy as ControlCommandExecutionPolicy
participant Main as Main Actor (v2MainSync)
participant Coord as ControlCommandCoordinator
CLI->>SW: raw socket line
SW->>SW: parse once (v2Parser / v1 tokenize)
SW->>Policy: classify method/command
alt Worker lane (resolution reads, sends, sidebar telemetry)
Policy-->>SW: .socketWorker(mainThreadCallable:)
SW->>Coord: handleSocketWorkerV2 / handleSidebarTelemetryV1
Coord->>Main: controlResolveOnMain (ONE hop: ref refresh + snapshot + ref mint)
Main-->>Coord: Sendable snapshot + pre-minted refs
Coord-->>SW: ControlCallResult (payload build off-main)
SW-->>CLI: encoded response
else surface.read_text / read_screen (formatting off-main)
Policy-->>SW: .socketWorker(mainThreadCallable: false)
SW->>Main: v2MainSync (ONE hop: routing + Ghostty FFI capture)
Main-->>SW: ReadTextCaptureOutcome (raw snapshot + refs)
SW->>SW: terminalTextPayload (line tail / base64 / format)
SW-->>CLI: encoded response
else Coordinator hop methods
Policy-->>SW: .socketWorker(mainThreadCallable: true)
SW->>Main: "v2MainSync { v2MainActorResponse }"
Main-->>SW: V2MainHopOutcome
SW-->>CLI: encoded response
else Main lane (focus verbs, legacy mutations)
Policy-->>SW: .mainActor
SW->>Main: "v2MainSync { processCommand }"
Main-->>SW: result
SW-->>CLI: encoded response
end
%%{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"}}}%%
sequenceDiagram
participant CLI as CLI Client
participant SW as Socket Worker Thread
participant Policy as ControlCommandExecutionPolicy
participant Main as Main Actor (v2MainSync)
participant Coord as ControlCommandCoordinator
CLI->>SW: raw socket line
SW->>SW: parse once (v2Parser / v1 tokenize)
SW->>Policy: classify method/command
alt Worker lane (resolution reads, sends, sidebar telemetry)
Policy-->>SW: .socketWorker(mainThreadCallable:)
SW->>Coord: handleSocketWorkerV2 / handleSidebarTelemetryV1
Coord->>Main: controlResolveOnMain (ONE hop: ref refresh + snapshot + ref mint)
Main-->>Coord: Sendable snapshot + pre-minted refs
Coord-->>SW: ControlCallResult (payload build off-main)
SW-->>CLI: encoded response
else surface.read_text / read_screen (formatting off-main)
Policy-->>SW: .socketWorker(mainThreadCallable: false)
SW->>Main: v2MainSync (ONE hop: routing + Ghostty FFI capture)
Main-->>SW: ReadTextCaptureOutcome (raw snapshot + refs)
SW->>SW: terminalTextPayload (line tail / base64 / format)
SW-->>CLI: encoded response
else Coordinator hop methods
Policy-->>SW: .socketWorker(mainThreadCallable: true)
SW->>Main: "v2MainSync { v2MainActorResponse }"
Main-->>SW: V2MainHopOutcome
SW-->>CLI: encoded response
else Main lane (focus verbs, legacy mutations)
Policy-->>SW: .mainActor
SW->>Main: "v2MainSync { processCommand }"
Main-->>SW: result
SW-->>CLI: encoded response
end
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
1400
to
+1413
| case let method where method.hasPrefix("remotes."): | ||
| return socketWorkerRemotesResponse(method: method, id: request.id, params: request.params) | ||
| default: | ||
| return v2Error(id: request.id, code: "method_not_found", message: "Unknown method") | ||
| #if !DEBUG | ||
| // debug.sidebar.simulate_drag stays policy-listed in Release but | ||
| // its worker case above is compiled out; the Release main lane | ||
| // answers method_not_found for debug verbs, so mirror that reply | ||
| // instead of the internal-error backstop below. | ||
| if request.method == "debug.sidebar.simulate_drag" { | ||
| return v2Error(id: request.id, code: "method_not_found", message: "Unknown method") | ||
| } | ||
| #endif | ||
| // Only reachable when a method is added to the policy's | ||
| // socketWorkerMethods but omitted from both |
Contributor
There was a problem hiding this comment.
Three sentinel error messages in this PR surface routing and threading internals to API consumers. Under the user-facing error messages rule, API error bodies and command output must not include implementation details.
- In
socketWorkerV2Response:"v2 worker method '\(request.method)' has no worker handler"leaks "v2 worker method" and "worker handler" terminology. - In
socketWorkerV1ResponseIfHandled(same file):"ERROR: internal: v1 worker command '\(cmd)' has no worker handler"leaks "v1 worker command" and "worker handler". - In
ControlCommandCoordinator+System.swift(the nil-context branch ofsystemTree):"system.tree dispatched off-main without a context seam"leaks dispatch topology.
These messages are visible to any CLI client or shell script that receives the error. A phrase like "Internal routing error — please report this" would give the user an actionable signal without exposing the two-lane architecture.
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!
Comment on lines
+5385
to
+5398
| self.v2RefreshKnownRefs() | ||
| let routing = ControlRoutingSelectors( | ||
| hasWindowIDParam: self.v2HasNonNullParam(params, "window_id"), | ||
| windowID: self.v2UUID(params, "window_id"), | ||
| groupID: self.v2UUID(params, "group_id"), | ||
| workspaceID: self.v2UUID(params, "workspace_id"), | ||
| surfaceID: self.v2UUID(params, "surface_id") | ||
| ?? self.v2UUID(params, "terminal_id") | ||
| ?? self.v2UUID(params, "tab_id"), | ||
| paneID: self.v2UUID(params, "pane_id") | ||
| ) | ||
| guard let tabManager = self.resolveTabManager(routing: routing) else { | ||
| return .finished(.err(code: "unavailable", message: "TabManager not available", data: nil)) | ||
| } |
Contributor
There was a problem hiding this comment.
lines vs TabManager error precedence changed from legacy ordering
The PR claims responses from v2SurfaceReadText are byte-identical to the main lane, but the evaluation order for simultaneous parameter errors has shifted. In the old coordinator dispatch, lines > 0 was validated before the TabManager check, so a request with both lines: -1 and an unavailable TabManager returned invalid_params. In the new in-hop layout the TabManager guard fires first, returning unavailable.
This is a narrow edge case, but the "byte-identical" documentation in the PR description and function doc is inaccurate for this path. If any downstream test or shell integration drives both a bad lines value and an unavailable session simultaneously, it would see a different error code.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The warning-budget check failed on the new hop-timing preamble:
wantsTiming is constant true in DEBUG, so the guard's bare
DispatchQueue.main.sync fallback was dead code ('will never be
executed', budget 0 for TerminalController.swift). Make the early
return #if !DEBUG and guard it directly on signpostingActive, which is
exactly what wantsTiming reduced to outside DEBUG. Behavior identical
in both configurations; the warning is gone from the build log.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding on the worker-lane move: report_shell_state's dedupe CAS now runs at drain time (required for record-order == apply-order across concurrent connections), which meant every report enqueued onto TerminalMutationBus's unbounded pending array before duplicates were discarded — and the worker lane replies without waiting for main, so a looping client could grow the backlog for as long as the main actor stayed blocked. Fix at the bus boundary: enqueueReplacingMainActorMutation removes any still-pending mutation with the same TerminalMutationReplaceKey before appending (the existing notification-coalescing pattern applied to .perform mutations). Shell-state reports key on (workspace, panel, .shellActivity), so pending holds at most one shell-state entry per surface regardless of drain starvation — a strictly tighter bound than the pre-worker-lane path, which enqueued every state change. The CAS at drain time stays authoritative, preserving the ordering invariant the witness documents. Covered by testReplacingMainActorMutationKeepsOnlyNewestEntryPerKey: same-key enqueues coalesce to the newest closure, distinct keys and non-keyed mutations are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hell state Second review round flagged the siblings of the shell-state fix: report_git_branch (and the other scoped schedulers) still appended an unbounded bus mutation per report while the worker lane replies immediately, so a blocked main actor turns telemetry loops into unbounded pending growth. Extend the replace-key coalescing to every scoped last-write-wins scheduler: git branch update/clear (shared .gitBranch key, newest write wins in either order), directory (report_pwd), tty, and ports_kick keyed additionally by reason (idempotent trigger; same-reason duplicates collapse, distinct reasons each run). PR metadata mutations intentionally stay non-coalesced: shouldReplacePullRequest applies an ordering guard at drain, so collapsing an update chain could drop an update the guard would have accepted, and report_pr is poller-cadence traffic. Unscoped fallback paths also stay non-coalesced; they resolve targets at drain and serve manual invocations. Budget TSV: track the three grown files at their new exact counts and tighten TerminalController.swift by the 3 lines the dead-code fix removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…head of queued kicks Review round 3: replace-key coalescing on the scoped TTY path used remove-and-append, so report_tty A / ports_kick / report_tty A while main is blocked would drain the kick before any registration. PortScanner.kick silently no-ops for unregistered TTYs and the coordinator documents that a kick enqueued after a TTY report drains after the registration, so the scan would be lost. Revert the scoped TTY scheduler to a plain ordered enqueue and drop the unused .tty kind. report_tty fires once per shell start, not per prompt, so boundedness is not a practical concern there. The other coalesced kinds have no queued dependents (branch/directory/shell state feed display state only; a kick moving later preserves its only dependency, registration-before-kick). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/TerminalController+ControlSidebarContext2.swift (1)
27-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t coalesce git-branch reports when
isDirtyis omitted.
isDirty == nilis state-dependent: the existing path preserves the current dirty bit when the branch is unchanged (Lines 62-67). With replacement,dirty=truefollowed bydirty=nilwhile main is blocked drops the first mutation, so the drained state can become clean instead of preserving dirty. Keep omitted-dirty updates ordered, or merge pending dirty state before replacing.One localized fix option
- TerminalMutationBus.shared.enqueueReplacingMainActorMutation( - replaceKey: TerminalMutationReplaceKey( - tabId: scope.workspaceID, - surfaceId: scope.panelID, - kind: .gitBranch - ) - ) { + let enqueueMutation: (`@escaping` `@MainActor` () -> Void) -> Void = { mutation in + if isDirty == nil { + TerminalMutationBus.shared.enqueueMainActorMutation(mutation) + } else { + TerminalMutationBus.shared.enqueueReplacingMainActorMutation( + replaceKey: TerminalMutationReplaceKey( + tabId: scope.workspaceID, + surfaceId: scope.panelID, + kind: .gitBranch + ), + mutation + ) + } + } + enqueueMutation {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalController`+ControlSidebarContext2.swift around lines 27 - 33, The git-branch mutation path in TerminalController+ControlSidebarContext2 is incorrectly coalescing updates when isDirty is omitted, which can drop a prior dirty=true state before the main actor drains. Adjust the enqueueing logic around TerminalMutationBus.shared.enqueueReplacingMainActorMutation and the .gitBranch replace key so omitted-dirty updates are not replaced out of order, or ensure pending dirty state is merged into the replacement payload before coalescing. Keep the existing state-preserving behavior in the git-branch handling code so isDirty == nil continues to retain the current dirty bit when the branch is unchanged.Sources/TerminalNotificationQueue.swift (1)
35-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMark
TerminalMutationReplaceKeyasnonisolated.TerminalMutationReplaceKeyandKindare pureSendablevalue types used from worker-lane scheduling paths; leaving them actor-isolated re-couples those call sites to the main actor and can trigger Swift 6 isolation diagnostics.Suggested fix
-nonisolated struct TerminalMutationReplaceKey: Hashable, Sendable { - nonisolated enum Kind: Hashable, Sendable { +nonisolated struct TerminalMutationReplaceKey: Hashable, Sendable { + nonisolated enum Kind: Hashable, Sendable {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalNotificationQueue.swift` around lines 35 - 44, Make TerminalMutationReplaceKey nonisolated so this Sendable value type and its nested Kind enum can be used from worker-lane scheduling paths without inheriting main-actor isolation. Update the TerminalMutationReplaceKey declaration in TerminalNotificationQueue and keep its stored properties and Kind cases as plain value types so Swift 6 isolation diagnostics are avoided.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/TerminalController`+ControlSidebarContext2.swift:
- Around line 27-33: The git-branch mutation path in
TerminalController+ControlSidebarContext2 is incorrectly coalescing updates when
isDirty is omitted, which can drop a prior dirty=true state before the main
actor drains. Adjust the enqueueing logic around
TerminalMutationBus.shared.enqueueReplacingMainActorMutation and the .gitBranch
replace key so omitted-dirty updates are not replaced out of order, or ensure
pending dirty state is merged into the replacement payload before coalescing.
Keep the existing state-preserving behavior in the git-branch handling code so
isDirty == nil continues to retain the current dirty bit when the branch is
unchanged.
In `@Sources/TerminalNotificationQueue.swift`:
- Around line 35-44: Make TerminalMutationReplaceKey nonisolated so this
Sendable value type and its nested Kind enum can be used from worker-lane
scheduling paths without inheriting main-actor isolation. Update the
TerminalMutationReplaceKey declaration in TerminalNotificationQueue and keep its
stored properties and Kind cases as plain value types so Swift 6 isolation
diagnostics are avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3029a4f-8738-4c47-827e-04b99ebaf1c7
📒 Files selected for processing (2)
Sources/TerminalController+ControlSidebarContext2.swiftSources/TerminalNotificationQueue.swift
# Conflicts: # .github/swift-file-length-budget.tsv
azooz2003-bit
added a commit
that referenced
this pull request
Jul 6, 2026
… authority, #7324 Vault) The big one is #7357 (tranches A-E of the off-main CLI command program): 11 both-diverged files resolved keeping HEAD's coordinator/extraction structure while porting main's off-main dispatch semantics, ControlCommandExecutionPolicy expansion, TerminalNotificationQueue changes, and the new socket-security tests. The relocated codex resume/fork routing (AgentResumeCommandBuilder) and the agent-notification gating were preserved (no duplicates resurrected). #6712's Codex session-restore authority folded into RestorableAgentSession. pbxproj union-dedup + normalize; budget regenerated; wiring/conventions lints green. One deliberate TODO(7357-merge): the DEBUG v1 sleepy_mode command maps into extracted ControlDebugContext/help files in a follow-up commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
azooz2003-bit
added a commit
that referenced
this pull request
Jul 6, 2026
…identify The #7357 conflict resolution wired systemIdentify to a nonexistent ControlSystemContext.controlSystemIdentify seam. HEAD's refactor already owns the identify payload build as ControlCommandCoordinator.identify(params:) (ControlCommandCoordinator+Identify.swift, backed by the identifyContext seam). Call self.identify(params:) inside the controlResolveOnMain hop, matching #7357's off-main dispatch (payload built on the main actor, only JSON encode leaves the thread) and the sibling systemTree pattern (self.systemTreeHopBody inside the same hop). CmuxControlSocket now builds standalone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
azooz2003-bit
added a commit
that referenced
this pull request
Jul 6, 2026
…d.push helper Two #7357 merge artifacts in withSocketCommandPolicy / socketWorkerV2Response: - The defer manually popped the socket-command focus-allowance stack via main's former thread-dictionary statics (currentSocketCommandFocusAllowanceStack / setter). HEAD's refactor replaced that process-wide mechanism with the instance socketCommandFocusAllowance (ControlSocketFocusAllowanceStack), whose withPolicy() pushes on entry and pops in its own defer (proven by its package test). HEAD's method top correctly no longer pushes the allowance manually, so the merged-in manual pop popped a frame it never pushed and referenced two now-removed statics. Dropped those 5 lines; the command-key signpost stack pop stays. - Restored the pure feedPushWaitTimeoutSeconds(params:) static that the merged feed.push handler calls (validates wait_timeout_seconds in 0...120). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 7, 2026
This was referenced Jul 8, 2026
Closed
This was referenced Jul 13, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves the control-socket CLI paths off the main thread so heavy agent load stops serializing on the main actor (the beachball in #5757: 1030 threads, 446 client handlers, main thread pegged in
v2MainSyncformatting scrollback).Parse, encode, formatting, and reply shaping now run on the per-connection socket-worker thread; each migrated verb keeps at most ONE narrow
v2MainSynchop for the main-confined snapshot or mutation witness. Replies, error precedence, andkind:Nref ordinals are byte-identical to the main lane (per-commit contracts in the messages):surface.read_text+ v1read_screen— the largest single main-thread consumer (multi-MB scrollback formatting) now formats off-main; NOT main-thread-callable, guarded byinvalid_dispatch.controlResolveOnMainsingle-hop primitive.surface.send_text/send_key+ 5 v1 send twins), synchronous hop kept because the queued/full/exited replies drive caller retry.Policy is pinned by exact-set package tests (
ControlCommandExecutionPolicyTests): growing a worker lane fails tests until the author decides themainThreadCallableflag; a policy-listed verb with no worker handler answers a loudinternal_errorinstead of a plausiblemethod_not_found.Out of scope (follow-ups on #5757): the mutations tranche (focus verbs stay main-lane), thread-per-connection pooling, read-text coalescing/backpressure.
Verification:
swift test --package-path Packages/macOS/CmuxControlSocket195 green at every commit; full app builds via cloud builder; dogfood build on tagclimn.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large architectural change to control-socket threading and ordering for automation/CLI paths; incorrect hops or policy drift could change reply timing, ref ordinals, or main-thread safety under load.
Overview
Offloads control-socket CLI work from the main actor so heavy agent load no longer stalls the UI in
v2MainSyncscrollback formatting (#5757). Parse, JSON encode, and reply shaping run on the per-connection socket-worker thread; migrated verbs use at most one narrow main hop (controlResolveOnMain/controlSidebarOnMain) for snapshots, ref minting, or input injection while keeping wire bytes and error order unchanged.CmuxControlSocket gains
handleSocketWorkerV2for tranche-D/E v2 reads and sends (surface.*,workspace.*,window.*,pane.*,system.identify/tree,surface.send_text/send_key), ref minting before off-main payload build, and nonisolated sidebar telemetry bodies threaded with explicitcontext.ControlCommandExecutionPolicynow classifies v1 commands too (sidebar telemetry, notifications,read_screen, resolution reads, sends) with explicitmainThreadCallablesets;surface.read_textstays app-side on the worker (coordinator witness removed).TerminalControllerdispatches worker-lane v2 through the coordinator and v1 telemetry throughhandleSidebarTelemetryV1, with loud errors when policy lists a verb but no handler exists.Reviewed by Cursor Bugbot for commit fb03849. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Moves control-socket CLI parsing/encoding and most command handling off the main thread to remove UI stalls under load. Reads and sends now run on the per-connection worker thread with at most one short main-actor hop, keeping replies byte-identical and preserving per-connection ordering.
Refactors
surface/workspace/window list/current,window.displays,pane.list/surfaces,system.identify/tree(plus v1 twins).surface.send_text/surface.send_keyand v1 send twins; keep a synchronous hop for routing/input and ordering; main-thread-callable.controlResolveOnMain/controlSidebarOnMain; addControlCommandExecutionPolicyfor v2 and v1 withmainThreadCallablepins.surface.read_textand v1read_screento capture with one hop and format off-main; not main-thread-callable by design.Bug Fixes
v2MainSynctiming preamble outside DEBUG; behavior unchanged, warning eliminated.ports_kick(by reason).report_ttystays non-coalesced to keep registration ahead of queued kicks.mainThreadCallablepolicy;read_textrejects unformattable snapshots before minting refs.read_textdock branch updated for per-window docks.Written for commit fb03849. Summary will update on new commits.
Summary by CodeRabbit
New Features
surface.read_textnow runs on the socket-worker lane.Bug Fixes
Tests