Repository navigation
CmuxControlSocket stage 3c: extract the RPC dispatch half of TerminalController into ControlCommandCoordinator (14 domains) - #5816
Conversation
Extract the window RPC domain (window.list/current/focus/create/close/displays/ display) out of TerminalController into a new @mainactor @observable ControlCommandCoordinator in CmuxControlSocket, behind the read-only ControlCommandContext seam (app target conforms; package never imports the app target). The coordinator owns the kind:N ControlHandleRegistry (RPC selection state per the decomposition plan); TerminalController delegates its ensureRef/ resolveRef/removeRef to it so refs stay consistent across moved and not-yet- moved domains. Faithful lift: the window bodies build ControlCallResult/JSONValue payloads whose Foundation object is identical to the legacy [String: Any] dictionaries, so the encoded wire bytes match. Dispatch runs on the main actor inside the existing withSocketCommandPolicy scope, so the per-read v2MainSync hops the legacy bodies used become plain in-isolation calls and disappear. window.current preserves both distinct legacy errors (unavailable vs not_found) via ControlCurrentWindowResolution. TerminalController.swift 22074 -> 21921 (budget ratcheted). 17 new package tests (128 total) drive every window method through a fake context, asserting byte-identical payloads, ref minting, routing-selector parsing, and the two window.current failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds ControlCommandCoordinator, domain context protocols/DTOs, typed param/ref helpers, full domain handlers (window/pane/feed/notification/workspace-group/mobile/app-focus), TerminalController conformances and V2 dispatch wiring, tests, and Xcode source registrations. ChangesControlCommandCoordinator & Integration
Sequence DiagramsequenceDiagram
participant V2Client as V2 RPC Client
participant TermController as TerminalController
participant CmdCoord as ControlCommandCoordinator
participant Handles as ControlHandleRegistry
participant Context as ControlCommandContext
participant App as AppDelegate/TabManager/Stores
V2Client->>TermController: send ControlRequest
TermController->>CmdCoord: handle(request)
alt coordinator handles method
CmdCoord->>CmdCoord: parse params, routingSelectors
CmdCoord->>Handles: resolveRef / ensureRef
CmdCoord->>Context: control*(...)
Context->>App: query/mutate state
App-->>Context: typed result
Context-->>CmdCoord: typed result
CmdCoord-->>TermController: ControlCallResult
TermController-->>V2Client: encoded response
else fallback
CmdCoord-->>TermController: nil
TermController->>TermController: legacy dispatch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In
`@Packages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWindowTests.swift`:
- Around line 202-212: The test windowCloseOkAndNotFound currently only checks
the success path; add a second assertion for the failure/not-found path by
toggling context.closeResult to false, calling
coordinator.handle(request("window.close", ["window_id":
.string(windowID.uuidString)])) again and asserting the returned value is the
not_found response and that context.closedID was not set (or cleared). Make the
change inside the same test after the existing success assertions, referencing
windowCloseOkAndNotFound, coordinator.handle(...), context.closeResult and
context.closedID to locate the code to update.
In `@Sources/TerminalController.swift`:
- Around line 4004-4020: The current routing logic falls back to a default
tabManager when an explicit selector (routing.groupID, routing.workspaceID,
routing.surfaceID, routing.paneID) is present but fails to resolve; change the
control flow so each explicit selector is authoritative: for each of
routing.groupID, routing.workspaceID, routing.surfaceID, and routing.paneID,
first check if the selector is non-nil and then attempt resolution (use
v2LocateTabManager(forGroupId:), AppDelegate.shared?.tabManagerFor(tabId:),
AppDelegate.shared?.locateSurface(surfaceId:)? .tabManager,
v2LocatePane(paneId)? .tabManager respectively); if resolution succeeds return
the found tab manager, but if the selector was present and resolution fails
return nil (do not fall through to tabManager or currentScriptableMainWindow());
keep the existing window_id behavior on line 4000 unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa3331bd-cded-4e7c-8e88-d4dd737d30ad
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (12)
Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+Window.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCurrentWindowResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlDisplayInfo.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlMoveAllWindowsResult.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlRoutingSelectors.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWindowSummary.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWindowTests.swiftSources/TerminalController.swiftSources/TerminalControllerControlCommandContext.swiftcmux.xcodeproj/project.pbxproj
Greptile SummaryThis PR extracts the window, pane, workspace-group, notification, app-focus, feed, and mobile-host RPC domains from
Confidence Score: 5/5Safe to merge; the change is a well-scoped, byte-faithful lift of existing RPC domain logic into the package coordinator with no behavioral changes on any moved path. The refactor preserves existing wire bytes and error shapes across all moved domains, actor isolation is correct (@mainactor throughout), no new blocking primitives or timing hacks were introduced, and the coordinator + seam pattern is clean and consistently applied. The one finding is a workspace-group string fallback returning empty strings instead of English defaults, which only affects a pre-wiring state that does not occur in production. No files require special attention beyond the workspaceGroupStrings() fallback in ControlCommandCoordinator+WorkspaceGroup.swift. Important Files Changed
Sequence DiagramsequenceDiagram
participant Socket as Socket Worker
participant TC as TerminalController
participant Coord as ControlCommandCoordinator
participant Ctx as ControlCommandContext(seam)
participant App as AppDelegate / TabManager
Socket->>TC: processV2Command(request)
TC->>Coord: handle(request)
alt "window.* / pane.* / workspace.group.* / notification.* / app.focus / feed / mobile.*"
Coord->>Ctx: controlXxx(routing:, ...)
Ctx->>App: live state read/write
App-->>Ctx: result
Ctx-->>Coord: typed resolution enum
Coord-->>TC: ControlCallResult
TC-->>Socket: encoded response
else legacy domains (not yet moved)
Coord-->>TC: nil
TC->>TC: legacy switch (unchanged)
TC-->>Socket: encoded response
end
Reviews (5): Last reviewed commit: "stage 3c: fix int/double param helpers t..." | Re-trigger Greptile |
Restructure the seam into a per-domain protocol umbrella (ControlCommandContext: ControlWindowContext, ...) so each domain can be built in its own files, and port the shared TerminalControllerV2ParamParsingSupport pure helpers + ref minting (workspaceRefs/tabRef/workspacePaneAndSurfaceRefs) into the coordinator as JSONValue twins. Foundation for moving the remaining RPC domains. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the app-focus (app.focus_override.set, app.simulate_active), main-actor feed (feed.jump, feed.list), and notification (create/create_for_surface/create_for_target/ list/dismiss/mark_read/open/jump_to_unread/clear) domains into the coordinator behind their per-domain seams (ControlAppFocusContext/ControlFeedContext/ ControlNotificationContext), composed into the ControlCommandContext umbrella. The core handle(_:) now chains per-domain handleX dispatchers. Worker-lane methods stay app-side: feed.push/permission.reply/question.reply/ exit_plan.reply, and notification.create_for_caller (its own resolver). Faithfulness: byte-identical payloads/errors (live socket sweep on ctl3c1 confirms every result + error shape). Notification localized strings are resolved in the app conformance (app bundle) and passed through ControlNotificationStrings, because String(localized:) inside the package would bind to the package bundle and silently drop the Japanese translations — a wire change for non-English locales. Test fakes get benign defaults for non-window seams via ControlCommandContextTestStubs so each fake implements only the domain it exercises (128 package tests still green). TerminalController.swift 21952 -> 21522 (budget ratcheted). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+Window.swift (1)
115-115: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSimplify the nested Int conversion.
The
Int64(Int($0))pattern convertsUInt32? -> Int -> Int64. SincedisplayIDis effectively aUInt32, you can convert directly toInt64($0)without the intermediateIntcast.♻️ Proposed simplification
-"display_id": display.displayID.map { JSONValue.int(Int64(Int($0))) } ?? .null, +"display_id": display.displayID.map { JSONValue.int(Int64($0)) } ?? .null,🤖 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 `@Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator`+Window.swift at line 115, The JSON construction uses a nested cast for display.displayID ("display_id": display.displayID.map { JSONValue.int(Int64(Int($0))) } ?? .null); simplify by removing the unnecessary intermediate Int cast and convert the UInt32 (or optional UInt32) directly to Int64 (i.e., use Int64($0)) so the mapping becomes JSONValue.int(Int64($0)) inside the display.displayID.map closure.
🤖 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.
Inline comments:
In
`@Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlAppFocusContext.swift`:
- Line 1: The Foundation import in ControlAppFocusContext.swift is unused;
remove the line "internal import Foundation" from the file and ensure no other
Foundation symbols are referenced by the protocol signatures or comments; if the
comments reference AppKit types like NSApp or
NSApplication.didBecomeActiveNotification, either update the comments to avoid
implying Foundation or, if AppKit APIs are actually needed, replace the
Foundation import with the correct AppKit import.
---
Outside diff comments:
In
`@Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator`+Window.swift:
- Line 115: The JSON construction uses a nested cast for display.displayID
("display_id": display.displayID.map { JSONValue.int(Int64(Int($0))) } ??
.null); simplify by removing the unnecessary intermediate Int cast and convert
the UInt32 (or optional UInt32) directly to Int64 (i.e., use Int64($0)) so the
mapping becomes JSONValue.int(Int64($0)) inside the display.displayID.map
closure.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6eaa018-6d38-4ca9-ad8e-be7b2f6bec9c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (22)
Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlAppFocusContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+AppFocus.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+Feed.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+Notification.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+Window.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlFeedContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationCreateResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationDismissResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationMarkReadResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationOpenResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationSnapshot.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationStrings.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlNotificationTargetedDeliveryResolution.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftSources/TerminalController+ControlAppFocusContext.swiftSources/TerminalController+ControlFeedContext.swiftSources/TerminalController+ControlNotificationContext.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
Move workspace.group.* (17 methods), pane.* (9 methods), and mobile.host.status/ mobile.workspace.list/mobile.terminal.* (+terminal.* aliases) into the coordinator behind ControlWorkspaceGroupContext/ControlPaneContext/ControlMobileHostContext, composed into the umbrella; core handle(_:) chains the new handlers. Workspace Groups + Pane are full lifts (bodies deleted, payloads rebuilt as JSONValue, localized group strings routed app-side via ControlWorkspaceGroupStrings). Mobile Host is a faithful pass-through: its 8 bodies are SHARED with the mobile data-plane (mobileHostHandleRPC) so they stay in TerminalController (relaxed private->internal); the coordinator decouples via the seam and the conformance bridges V2CallResult. Pane folds the resize support helpers (kept app-side: Bonsplit-coupled); v2SurfaceMove relaxed private->internal for pane.join forwarding. Live socket sweep on ctl3c1 confirms faithful payloads + errors (group create/list, pane list/create split, mobile host status). TerminalController.swift 21522 -> 20296. 128 package tests green. Two new Pane files >500 lines get budget entries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlMobileHostContext.swift`:
- Line 1: ControlMobileHostContext.swift currently uses "internal import
Foundation" which is inconsistent with sibling seam protocol files (e.g.,
ControlPaneContext, ControlWindowContext) that use public visibility; update the
file to match the intended public seam visibility by changing the import
visibility to "public import Foundation" and ensure any protocol/type
declarations in this file (e.g., the ControlMobileHostContext protocol/type
declarations) have public access modifiers to match the other Control*Context
seam protocols—alternatively, if this file is intentionally internal, add a
concise comment explaining why it differs and update other files to the intended
internal visibility for consistency.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b57e6a0-7224-4e10-b15f-3133f08acaa2
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (36)
Packages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+MobileHost.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+Pane.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator+WorkspaceGroup.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlMobileHostContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneBreakResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneCreateInputs.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneCreateResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneFocusResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneGridSize.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneJoinResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneLastResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneListSnapshot.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPanePixelFrame.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneResizeInputs.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneResizeResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneSummary.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneSurfaceSummary.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneSurfacesSnapshot.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlPaneSwapResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupAddResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupContext.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupCreateResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupFocusResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupListResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupNewWorkspaceResolution.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupSnapshot.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlWorkspaceGroupStrings.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftSources/TerminalController+ControlMobileHostContext.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController+ControlWorkspaceGroupContext.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
Regression found by the no-regression code review of the moved domains: the ported int() did Int(value) on a JSON double, which TRAPS (crashes) on overflow/ NaN — reachable via pane.resize amount or workspace.group.move to_index with e.g. 1e30 — whereas legacy v2Int went through (params[key] as? NSNumber).intValue, which clamps. Also int()/double() didn't coerce a JSON boolean to a number the way the legacy as? NSNumber path did. Both now route doubles/bools through NSNumber.intValue/.doubleValue, matching v2Int/v2Double exactly (truncate-toward-zero, clamp out-of-range, bool->1/0). 5 regression tests cover truncation, overflow/NaN no-trap, and bool coercion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Workspace (21 methods incl. remote.*) and Surface (25 methods + debug.terminals) move into ControlCommandCoordinator behind ControlWorkspaceContext/ ControlSurfaceContext. ~2640 lines deleted from TerminalController.swift (20296 -> ~17650). Worker-lane workspace.remote.pty_* stay app-side. Two shared bodies the drafting agents wrongly flagged for deletion were RESTORED (internal/private): v2WorkspaceCreate(params:tabManager:) is still driven by the mobile data-plane v2MobileWorkspaceCreate; workspaceCloseProtectedMessage() by the v1 close path. surface.move + debug.terminals forward to the still-shared v2SurfaceMove/v2DebugTerminals (relaxed internal), like pane.join. Relaxed to internal for the conformances: tabManager, socketFastPathState, orderedPanels, readTerminalTextRawSnapshot. Live socket sweep on ctl3c1 confirms faithful payloads + errors across both domains (workspace list/current/create/rename/select/next/close, surface list/ current/health/send_text+read_text round-trip/resume.get, error shapes). 133 package tests green. KNOWN FOLLOW-UPS: workspace.create logic is duplicated (conformance reimplements + restored shared body) — dedupe by forwarding; the 2 Workspace files >500 lines (budget entries added) should be split; adversarial code-review verification of these 2 domains still pending (8 prior domains verified clean). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coordinator/ had grown to 105 files. Move each domain's coordinator extension, seam protocol, and value/resolution/snapshot types into a per-domain subfolder (Window/AppFocus/Feed/Notification/Pane/Surface/Workspace/WorkspaceGroup/ MobileHost). The 4 shared core files stay at the Coordinator/ root: ControlCommandContext (umbrella), ControlCommandCoordinator (core dispatch + handle registry), ControlCommandCoordinator+Params (shared param/ref helpers), ControlRoutingSelectors. SwiftPM globs sources recursively, so this is purely organizational — no Package.swift/import changes. Budget paths updated for the moved Pane/Workspace coordinator files; TC.swift budget corrected to 17680 (the two restored shared bodies grew it after the last bump). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Workspace lift had reimplemented workspace.create logic in the conformance while the original v2WorkspaceCreate(params:tabManager:) was restored for the mobile data-plane caller -- two copies that could diverge. Replace the typed reimplementation with a passthrough that forwards to the single shared v2WorkspaceCreate (relaxed private->internal) and bridges its Foundation result, exactly like surface.move/debug.terminals/mobile. Deletes the now-unused ControlWorkspaceCreateInputs/ControlWorkspaceCreateResolution. One source of truth, byte-identical wire output. Comprehensive socket sweep on ctl3c1 (all 10 domains, 38 ok + 13 expected validation errors, zero crashes) confirms no regression: workspace.create happy path + its cwd/layout validation errors preserved; pane.resize amount=1e30 now clamps (invalid_state) instead of trapping (the int/double NSNumber fix). 133 package tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view Surface (4): surface.clear_history with a present-but-invalid surface_id silently cleared the FOCUSED surface instead of returning not_found (wrong-target side effect; hasSurfaceIDParam now crosses the seam like send_text); surface.split with an unrecognized direction returned unavailable instead of invalid_params 'Missing or invalid direction (left|right|up|down)' (coordinator now validates the parseSplitDirection token set + a drift-safe .invalidDirection case); surface.split error precedence restored (direction -> agent-session -> divider; the agent-session token check moved before divider parsing); surface.resume.* explicit target restored to surface_id ?? tab_id ONLY (terminal_id is a general routing alias but was never a resume target) and the window branch now requires a RESOLVABLE window_id like origin. Workspace (4): select/close/rename get the routing precheck so unresolvable routing returns unavailable before param validation (legacy TabManager-first order, matching reorder); workspace.current with a stale selectedTabId returns .ok with workspace:null again instead of not_found. Dead code removed (JSONValue.isControlNull, surfaceIDForInput). All confirmed by live socket sweep on the rebuilt ctl3c1 (each previously-wrong response now byte-matches origin). 133 package tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…3c-1 # Conflicts: # .github/swift-file-length-budget.tsv
…ce conformance The tests-build-and-lag job failed solely on the Swift WARNING budget: the Workspace conformance's controlWorkspaceRemotePTYAttachEnd declared 'sessionID sessionID: String' (extraneous duplicate). Behavior identical; the job's build and lag phases were green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s (drafts integrated) Five domains drafted by the orchestrator's agents (handed off), repaired (browserNavContext accessor, allocateElementRef state call, v1 handlers unhooked from the v2 chain), wired into the umbrella + dispatch, with test stubs completed. 140 package tests green. App-side surgery follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e coordinator TerminalController.swift 18,033 -> 10,748 (-7,285). The five remaining domains now dispatch through ControlCommandCoordinator: System (identify/tree/auth.login/ session.restore/settings.open/feedback.open/extension snapshot/workspace.action/ tab.action/drag_to_split/split_off), Project (project.* + markdown.open + file.open), Debug (39 debug.* methods), Sidebar v1 (44 verbs via a new handleSidebarV1 hook ahead of the v1 switch), Browser panel v1 (8 verbs), and all 89 main-actor browser.* methods. Browser per-surface state moved off the controller: ControlBrowserAutomationState (package) + dialog responders keyed by dialogID app-side (the Sendable V2BrowserPendingDialog redesign); cleanupSurfaceState purges the new state, faithfully mirroring the legacy eviction. Two conformances the drafts never included (ControlBrowserContext, ControlBrowserPanelContext) were authored byte-faithfully from the legacy bodies. Shared bodies kept + relaxed to internal (v2Identify, v2WorkspaceAction, v2SurfaceSplitOff, v2FileOpen, the 18 v1-debug impls, the JS pump, the worker-lane browser.download.wait cluster). Deliberate deltas (documented): controlFeedbackOpen drops the deprecated .activateIgnoringOtherApps activation option (documented no-op on macOS 14+, the project floor; keeping it fails the new-file warning budget); a sequence id bridges Int64->Int (lossless on arm64). Gates: package swift build + 140/140 tests; tagged app build BUILD SUCCEEDED; live socket sweep green across all domains (system.tree, auth.login parity, browser.open_split -> get.title returns the real page title end-to-end, project validation errors, v1 set-status via the new hook, debug.terminals, plus regression of the ten prior domains); zero new warnings; both budgets pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
39d111b to
f731c27
Compare
The System+Project adversarial review found file.open had been reimplemented in the coordinator/conformance while the original v2FileOpen stayed behind (it is driven directly by FilePreviewReviewFeedbackTests and MarkdownPanelTests) - two copies that could drift, and a stale dispatcher comment claiming forwarding. file.open now forwards to the single shared body and bridges its result, like workspace.create; the reimplementation and its now-unused ControlFileOpenResolution/ControlFileOpenSurface types are deleted. Review verdicts so far: System+Project all faithful (this was the only finding, not a behavior bug); Debug (39 verbs) + Sidebar v1 (44) + Browser-panel v1 (8) all faithful, zero divergences, #if DEBUG gating verified end-to-end. 140 package tests green; app build green; live probe of file.open through the shared body (happy path + both error shapes) byte-faithful. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h residue The Browser adversarial review (87/89 faithful) found its only two divergences share one root cause: focus_mode.set and zoom.set validated mode/direction BEFORE the TabManager/handle guards (legacy order: guards first). The shared browserFocusedAction helper gains a post-guard validate step; both methods' validation moves there. Live-verified: double-fault now returns unavailable/'TabManager not available', single-fault the mode/direction error. Residue: socketFastPathState drops its 'nonisolated' (after the cutover its only callers are the @mainactor sidebar/surface conformances; the worker-thread fast path retired with the legacy dispatcher). ServerEventTarget's @unchecked Sendable and the V2CallResult/V2SocketRequest twins stay deliberately: they serve the worker-lane and kept-shared bodies, which move in a later wave (the target itself dissolves with TerminalControlComposition in Wave 5). Verification totals for the five stacked domains: 143 methods/verbs reviewed per-method vs the pre-deletion originals; 141 faithful as-lifted, 2 fixed here. 140 package tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stage 3c (stacked): final five domains — System/Project/Debug/Sidebar/Browser
…est, unused imports) - @ObservationIgnored on the coordinator's handles registry: it is a struct mutated by ref() on nearly every response, so tracking it would invalidate any observer on every socket command (greptile). - windowCloseOkAndNotFound now also asserts the not_found branch (coderabbit). - Drop unused Foundation imports from ControlAppFocusContext and ControlMobileHostContext (coderabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ert the coordinator's v2 browser.* domain PR 5778 moved the JS-evaluating browser.* methods onto a nonisolated socket-worker lane while this branch had lifted the (pre-5778) browser domain into the @mainactor coordinator. The two designs are incompatible and main's is the behavioral reference, so this merge takes main's browser implementation wholesale and deletes the coordinator's browser-v2 domain (package files, app conformances, umbrella members, tests). The v1 browser-panel and sidebar handlers and the other 13 coordinator domains are untouched by main and stay. mobile.terminal.paste (new in PR 5876) dispatches from the legacy v2 switch. The browser domain gets re-lifted in a follow-up against the worker-lane architecture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g into ControlCommandCoordinator Main's CmuxControlSocket stage 3c (manaflow-ai#5816) moved the v1/v2 RPC dispatch out of TerminalController into the ControlCommandCoordinator package, deleting the handler bodies that carried this branch's remote-tmux accepted-routing contract. This merge re-expresses that contract in the new architecture: - ControlSurfaceSplitResolution / ControlSurfaceCreateResolution / ControlPaneCreateResolution gain mirrorUnsupportedOptions + routedToRemote cases; ControlTabActionResolution.Extras gains routedToRemote; ControlSidebarNewSurfaceResolution gains routedToRemote; the v1 controlSidebarCreatePaneSplit seam returns a new ControlSidebarPaneSplitResolution instead of UUID?. - ControlCommandCoordinator+MirrorRouting.swift renders the shared accepted payload (accepted: true, routed: "remote-tmux", null surface ids) and the pre-mutation invalid_params rejection — byte-compatible with the previous app-side v2RemoteRoutedCreationResult / v2MirrorUnsupportedOptionsError. - The app-side context witnesses (TerminalController+ControlSurfaceContext2, +ControlPaneContext, +ControlSystemContext2, +ControlSidebarContext3) run the mirror guard and switch on newTerminalSplitOutcome / newTerminalSurfaceOutcome, exactly as the pre-refactor handlers did. - v1 new_split stays app-side (main kept it in TerminalController); its mirror handling is unchanged, with v1MirrorDirectionError re-homed next to it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…browser CLI, pairing QR, iOS PRs included: - manaflow-ai#5816 ControlCommandCoordinator extraction (package coordinator skeleton; fork keeps legacy v2* dispatchers) - manaflow-ai#5859 sidebar perf - manaflow-ai#5857 RendererRealization (added as SurfaceHibernation adapter) - manaflow-ai#5867 in-process custom sidebars - manaflow-ai#5778 browser CLI / system-proxy bypass - manaflow-ai#5872 minimal pairing QR - iOS pairing/manual-entry stack - 30+ hot fixes Fork-side adjustments: - Skip 21 TerminalController+Control* extension files (PR manaflow-ai#5816 architecture refactor not adopted) - Add Sources/App/RendererRealizationSettingsAdapter.swift to bridge new RendererRealizationSettings to fork's existing SurfaceHibernationSettings - Restore v2SurfaceDragToSplit shim removed by upstream - Add SettingsNavigationTarget.customSidebars case - Stub ghostty_surface_set_renderer_realized callsites pending GhosttyKit rebuild (zig 0.15.2 required, host has 0.16.0) - Update ghostty submodule to 44b2baa81 (cherry-pick the 3 renderer commits onto fork's manaflow-ai#5128 link-fix pointer) - Keep fork's CMUXSessionDaemon module pbxproj refs and SurfaceHibernation settings
Adopts upstream PR manaflow-ai#5816's incremental coordinator architecture without disturbing the fork's legacy v2 dispatcher path: * Adds 21 TerminalController+Control*Context.swift extensions plus the umbrella TerminalControllerControlCommandContext.swift, all copied byte-for-byte from upstream/main. * TerminalController now constructs ControlCommandCoordinator() and binds context = self in init (mirrors upstream). * processV2Command and the v1 dispatcher try controlCommandCoordinator.handle / handleSidebarV1 / handleBrowserPanelV1 first; fork-only domains continue to fall through to the legacy v2*/v1 switch cases unchanged. * Adds Sources/TerminalController+ControlContextSupport.swift with resolveTabManager(routing:), the typed routing seam the upstream extensions assume. * Relaxes a handful of fork-only "private" v2*/helper accessors to internal (responderChainContains, orderedPanels, setShortcut family, etc.) so the new extension files can call them. v2BrowserDownload* and v2ResolveRemotePTY* keep their original visibility because their signatures expose private types. This keeps fork's 175 fork-only v2* methods on the legacy path while matching upstream's coordinator seam, so future upstream merges that move additional domains land cleanly.
TLDR
Stage 3c of the
CmuxControlSocketrefactor (continues 3b, merged in #5719). Extracts the RPC dispatch half ofTerminalControllerinto a@MainActor @Observable ControlCommandCoordinatorin the package, behind per-domain read-only seam protocols under aControlCommandContextumbrella. The package never imports the app target.Sources/TerminalController.swiftgoes from 22,073 to 14,774 lines.What moved
14 command domains now dispatch in
ControlCommandCoordinator.handle(_:)and return typedControlCallResults: window, app-focus, feed, notification, workspace-group, pane, mobile-host, workspace, surface, system, project, debug, sidebar (v2 plus the v1 string commands), and the v1 browser-panel commands.processCommand/processV2Commanddelegate to the coordinator first and fall through to the legacy switch for everything else, so unmoved and future methods keep working unchanged.Each domain has its own seam protocol file and its own app-conformance file (
TerminalController+Control<Domain>Context.swift);ControlCommandContextis an empty umbrella inheriting all of them. The coordinator owns thekind:NControlHandleRegistry; the app routesensureRef/resolveRef/removeRefthrough it so refs stay consistent across moved and unmoved domains.Genuinely shared or irreducibly app-coupled bodies stay app-side and are reached by forwarding, never duplicated:
v2WorkspaceCreate(shared with the mobile RPC path),v2FileOpen,v2SurfaceMove,v2DebugTerminals,v2WorkspaceAction,v2Identify, the worker-lane methods (vm.*,auth.*,system.top/memory,browser.download.wait, remote PTY), and the browser JS pump.The v2 browser.* domain is deliberately NOT in this PR
The browser domain was lifted and verified, then #5778 landed on main moving every JS-evaluating
browser.*method onto a nonisolated socket-worker lane (a starvation-deadlock fix for unmounted webviews). That architecture cannot live behind the@MainActorcoordinator seam, so the merge of main into this branch adopts main's browser implementation wholesale and reverts the coordinator's browser-v2 domain (package files, conformances, tests). The v1 browser-panel handlers stay in the coordinator (main did not touch them). Re-lifting browser against the worker-lane architecture is a follow-up.Faithfulness and verification
Every moved method was adversarially reviewed against its pre-deletion original (
git showof the pre-cutover tree), with byte-identical wire output as the bar:JSONValuepayloads whosefoundationObjectequals the legacy[String: Any], every error code/message/data preserved, side-effect order preserved. Divergences found in review (NSNumber coercion for numeric params,clear_historywrong-target side effect, workspace select/close/rename precheck order, focus_mode/zoom error precedence, stale-selectionworkspace.current) were fixed and live-verified over the socket. Localized strings resolve app-side so Japanese is not dropped by the package bundle.Live gates on the final tree: 90-command v2 socket sweep plus v1 sidebar/browser-panel round-trips on a tagged build, 135 package tests in 25 suites, full app build, file-length budget, CI.
Tests
Package tests drive every moved domain through fake seam conformances (per-domain test-stub defaults keep fakes small). Regression tests cover the NSNumber numeric-param coercion and the
window.closenot-found branch.