Repository navigation
Workspace god-model split: CmuxWorkspaceCore + Wave-3 value vocabulary (stack B) - #5991
Merged
Merged
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>
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>
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>
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
… PTY-bridge DTO leaf Wave-2 remote drain, first slice. Moves the remote-configuration value vocabulary out of the app target into a new CmuxCore leaf package and seeds the CmuxRemoteDaemonCore DTO/protocol leaf: - CmuxCore (new, depends on CmuxFoundation): WorkspaceRemoteConfiguration (+ SSH option normalization scoped onto the type, formerly the file-private WorkspaceRemoteSSHOptionFilter namespace enum), WorkspaceRemoteTransport, SessionRemoteWorkspaceSnapshot, WorkspaceRemoteWebSocketDaemonEndpoint, WorkspaceRemoteDaemonManifest (read side), RemoteDropUploadError, BrowserProxyEndpoint. - CmuxRemoteDaemonCore (new, no deps): RemotePTYBridgeEvent, RemotePTYBridgeAttachment, RemotePTYBridgeRPCClient seam, RemoteDaemonCapability wire strings, RemoteDaemonStrings (localized text resolved app-side and passed through the seam). - App target keeps localization and app-coupled pieces: RemoteDropUploadError+LocalizedError (app bundle string resolution), SSHPTYAttachStartupCommandBuilder (uses RemoteInteractiveShellBootstrapBuilder), SessionRemoteWorkspaceSnapshot+Restore. - Sources/WorkspaceRemoteConfiguration.swift (627 lines) deleted; Workspace.swift 19955 -> 19906 lines. Tests: 20 new CmuxCore tests + 4 new CmuxRemoteDaemonCore tests, all green via swift test. Tagged app build green. Net app-target delta -699/+~80 app lines; package source +716 lines (incl. DocC). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xtensions Prerequisite for lifting the remote daemon RPC client out of Workspace.swift: its pipe readers must live in a package. The app-target ProcessPipeReader namespace enum becomes CmuxFoundation FileHandle extensions per the no-namespace-enum/no-free-function conventions: - FileHandle.readAvailableData/readAvailableDataOrEndOfFile/ readToEndOfFileCapturingError/readDataToEndOfFileOrEmpty/ copyDataToEndOfFile(to:) with identical read/poll/EINTR semantics and the same os.Logger subsystem/category. - Raw descriptor loops scoped onto ProcessPipeAvailableRead (internal); injectable drain core kept public as ProcessPipeEndRead.reading for the partial-data-on-failure contract test. - 48 call sites across app + cmux-cli updated mechanically; the descriptor-level regression tests moved into CmuxFoundationTests (swift-testing); the app-coupled ProcessOutputCollector regression test stays in cmuxTests. Tests: CmuxFoundation 57 green (6 new). Tagged app build green. Net: app target -265 lines (file deleted), package +331 source lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…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>
…mmand composition Checkpoint 2 of the Wave-2 remote drain (PR #5896 plan): moves the remaining remote value vocabulary out of the app target into CmuxCore. - WorkspaceRemoteConnectionState / WorkspaceRemoteDaemonState / WorkspaceRemoteDaemonStatus (payload() wire shape pinned by tests) move from Sources/Workspace.swift to CmuxCore/Remote. - RemoteLoopbackProxyAlias moves from Sources/ to CmuxCore/Remote, now also owning normalizeHost (byte-identical lift of BrowserInsecureHTTPSettings.normalizeHost, which forwards to it; single source of truth). - WorkspaceRemoteSSHBatchCommandBuilder's namespace-enum statics become WorkspaceRemoteConfiguration instance methods (argv lists element-for- element identical, first-match ssh-option semantics pinned by tests); Sources/WorkspaceRemoteSSHBatchCommandBuilder.swift retired. - CmuxCore added to the ci.yml headless package-test matrix. Tests: CmuxCore 37 (was 20, +17 behavior tests for the alias, status values, and batch argv composition). Net: +682/-326 (tests dominate the adds); Sources/Workspace.swift 19907 -> 19860. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…opback rewriters, proxy tunnel, PTY bridge server Checkpoint 3 of the Wave-2 remote drain (PR #5896 plan): - New CmuxRemoteDaemon package: RemoteDaemonRPCClient (faithful lift of the private WorkspaceRemoteDaemonRPCClient; stdio/socket-forward/websocket transports, hello capability handshake, framing, every NSError domain/code/message and timeout pinned), RemoteDaemonPendingCallRegistry, websocket delegate, stream/PTY event values, capability strings, and the RemotePTYBridge DTO/protocol seam. Folds the CmuxRemoteDaemonCore leaf package in (its only consumers also need the client, so the two-package boundary did not survive the "why two packages?" rule); CmuxRemoteDaemonCore is deleted. - New CmuxRemoteWorkspace package: the three RemoteLoopbackHTTP rewriters (pure transforms), RemoteDaemonProxyTunnel + per-connection RemoteDaemonProxySession (SOCKS5/HTTP-CONNECT handshake parsing pinned), and RemotePTYBridgeServer + Session (newline-JSON handshake, ready/error status lines, buffer caps, half-close semantics pinned). Localization stays app-side via RemoteDaemonStrings.appLocalized and AppRemotePTYBridgeStrings (identical keys + defaultValues). The two DispatchQueue.asyncAfter timeouts became injected-clock tasks (RemoteProxyRetryClock, 30s delays unchanged, idempotent guards absorb stale fires). - Sources/Workspace.swift drops the whole lifted block (registry, DTOs, protocol, RPC client, rewriters, tunnel, PTY bridge server) and now composes the packages; remaining broker/relay/session-controller code is rewired to the package types. - Both packages added to the ci.yml headless package-test matrix. Tests: CmuxRemoteDaemon 16, CmuxRemoteWorkspace 21 (rewriter wire shapes, pending-call registry semantics, capability handshake requirements, and an end-to-end loopback PTY bridge handshake/pump/error-mapping suite); CmuxCore 37 still green; existing cmuxTests PTY-bridge/registry/rewriter suites retargeted to the package types. Net: Sources/Workspace.swift 19860 -> 16800 (-3060); app target total -3708 lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…yCommandRewriting seam Checkpoint 4 of the Wave-2 remote drain (PR #5896 plan): - WorkspaceRemoteCLIRelayServer moves to CmuxRemoteWorkspace as RemoteCLIRelayServer + Session (Relay/ subfolder). Wire behavior pinned: the cmux-relay-auth challenge JSON, HMAC-SHA256 auth message bytes, constant-time MAC compare, ok:true/ok:false newline frames, 16 KiB frame cap, unix-socket round-trip semantics, and every NSError domain/code/message. - The relay's call into Workspace.rewriteRemoteRelayCommandLine inverts behind the new RemoteRelayCommandRewriting protocol; the app conforms via WorkspaceRemoteRelayCommandRewriter (composition site unchanged: ensureCLIRelayServerLocked). - The anti-timing-oracle minimum failure delay (50ms) moves from DispatchQueue.asyncAfter to the injected RemoteProxyRetryClock (ceil to ms keeps the floor a true minimum); phase/isClosed guards unchanged. - Listener startup reuses ListenerStartupState (same semaphore contract). Tests: CmuxRemoteWorkspace 24 (+3: end-to-end challenge -> HMAC auth -> rewritten command round trip against a fake unix socket server with alias maps observed; wrong-MAC ok:false + close; invalid token hex rejected). Net: Sources/Workspace.swift 16800 -> 16292 (-508). Co-Authored-By: Claude Fable 5 <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>
Lift the embedded sidebar git/PR subsystem out of TabManager into the new CmuxSidebarGit services package: - @mainactor SidebarGitMetadataService (behind SidebarGitMetadataServing): local probe state machine, retry walk over the preserved offsets [0, 0.5, 1.5, 3, 6, 10]s, per-directory snapshot dedupe behind the injected process-wide WorkspaceGitMetadataProbeLimiter, RecursivePathWatcher wiring, and the 5-minute fallback re-poll. - @mainactor PullRequestPollService (behind PullRequestProbing): PR poll deadlines (max(0.25, ...) floor, 10s/60s +-10% jitter, 15-min terminal sweeps, batch limit 3), repo cache pruning, transient-failure staleness, command-hint reconciliation. - TabManager keeps thin forwarders plus a SidebarGitHosting conformance (synchronous read/write seam so apply-turn interleavings stay identical); gh/git argv, GitHub request shapes, cache keys, and badge transitions are unchanged byte-for-byte. - GitPollClock moves into the package with the code that sleeps on it; the probe limiter singleton becomes constructor injection (process-wide instance at the composition root). TabManager.swift 9992 -> 8366 lines (-1626); package +~2900 lines incl. 16 behavior tests (virtual-clock probe scheduling, projection, poll-deadline floor, command hints, limiter). Package swift build + swift test green; full app build green; pbxproj normalized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
"main" is a skip-lookup branch, so the poll-deadline floor test cleared the badge without ever starting a refresh and then waited forever for a poll timer that was never armed. Probe/PR suites now use feature/x where a real refresh is required; the dedicated skip-lookup test keeps main. 20 tests in 4 suites green. Net +5/-4 lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
… and the daemon manifest repository Checkpoint 5a+5b of the remote drain on #5896. RemoteProxyBroker (was the file-private WorkspaceRemoteProxyBroker in Workspace.swift): per-transport shared-tunnel registry, lease refcounting, exponential restart backoff. The `static let shared` singleton is gone: TerminalController constructs one instance at the app hub and injects it into each WorkspaceRemoteSessionController init behind the new RemoteProxyBrokering protocol (composition-root ownership moves with the planned RemoteSessionCoordinator). The broker now depends on the tunnel through new RemoteProxyTunneling + RemoteProxyTunnelProviding seams; the production provider carries the app-resolved strings so localization stays app-side. RemoteDaemonManifestRepository: live-manifest fetch, binary download with checksum verify (including the stale-embedded-manifest live-manifest fallback), and cache validation/atomic placement under the shared CmuxStateDirectory cache the CLI reads. The embedded-manifest decode moved onto the CmuxCore value (WorkspaceRemoteDaemonManifest init(infoDictionary:) + infoDictionaryKey); Bundle.main stays app-side. The Workspace manifest static forwarders are deleted (tests retarget to the package types); the dev-only go-build fallback stays in the controller. Deliberate deltas (who could observe them): 1. The restart backoff's queue.asyncAfter + DispatchWorkItem became an injected-clock Task (RemoteProxyRetryClock) whose wakeup is guarded by a per-entry restart token; delays identical (3s doubling, 60s cap, whole seconds convert to ms exactly). Cancellation is guard-absorbed. 2. The checksum-fallback DEBUG log line is now emitted by the controller after downloadBinary returns (flag on the result) instead of mid-download; DEBUG-log ordering only, text identical. Drive-by: fix the `sessionID sessionID:` duplicate parameter label in TerminalController+ControlWorkspaceContext.swift (warning-budget failure on newer toolchains; no callers change). Tests: CmuxRemoteWorkspace 24 -> 41 (broker state machine via fake provider + manual clock: transport dedupe, remotePath restart, backoff escalation with pinned retry suffixes, stale-wakeup token guard, lease teardown, PTY forwarding incl. the pinned code-40 error; manifest repository against a loopback HTTP server: fetch statuses, download happy path with 0755 + atomic install, pinned code 25/26/28 errors, live-manifest checksum fallback, cache validation), CmuxCore 37 -> 39. Workspace.swift 16292 -> 15823 (-469); net app-target -520 lines, package source +~830 (incl. DocC), package tests +~700. App build, package builds/tests, pbxproj check, warning + file-length budgets, and tagged build (remotews) all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…note CI's Swift 6.1 flags 'capture of self with non-sendable type WorkspaceRemoteSessionController?' now that the broker's acquire callback is @sendable (the local Swift 6.3 toolchain accepts the same capture silently; warning budget gates the CI toolchain). The controller is queue-confined (every mutable property behind the private serial queue via the *Locked methods) and its callbacks have always crossed queues by contract, so the annotation states the existing invariant rather than changing behavior; same essay as the lifted tunnel/broker. Workspace.swift +6 comment lines (budget 15823 -> 15829, still -463 net this checkpoint). Co-Authored-By: Claude Fable 5 <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>
…convergence surface Adds the synchronous typed-settings seam the TabManager Wave-3 drain reads through: SettingsReading/SettingsWriting protocols plus the UserDefaultsSettingsClient conformer (same DefaultsKey + SettingCodable primitives as the actor store, no parallel mechanism). Adds the two missing TabManager keys as a workspaceGroups catalog section (anchorCloseSuppressed, newWorkspacePlacement with the tolerant WorkspaceGroupNewPlacement value enum) and folds the legacy indicator-style string mapping (rail/border/wash/lift/typography/washRail/blueWashColorRail) into WorkspaceIndicatorStyle's SettingCodable decode so converged reads keep resolving values written by earlier builds. Tests: 83 package tests green (19 new: client round-trip/reset/absence, legacy indicator decode matrix, tolerant group-placement parse). Net: +371 lines (package only; app cutover follows). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
stage 3c (stacked): final five domains — System/Project/Debug/Sidebar/Browser
…ls + tests; app cutover follows) Three Wave-3 feature-domain packages from the TabManager blueprint, each a zero-dependency leaf with swift-tools 6.0 / macOS 14 / Swift 6 language mode: - CmuxSidebar: SidebarMultiSelectionModel (@mainactor @observable) owning the sidebar multi-selection set plus typed SidebarMultiSelectionDidHideEvent / ShouldCollapseEvent wrappers (NotificationCenter delivery and userInfo wire shape byte-identical to the legacy stringly keys); SidebarWorkspaceAuxiliaryDetailVisibility lifted faithfully and SidebarWorkspaceDetailVisibility binding the two legacy SidebarWorkspaceDetailSettings resolvers into one value. - CmuxBrowser: BrowserModel (@mainactor @observable) behind BrowserManaging, holding the bounded RecentlyClosedBrowserStack (faithful LIFO/capacity/ workspace-purge semantics), generic over BrowserPanelRestoreSnapshot because the full snapshot payload is Workspace-owned and migrates with the Workspace decomposition. - CmuxNotifications: NotificationDismissalModel (@mainactor @observable) behind NotificationDismissing, a faithful lift of TabManager's dismissal decision flow (context policy enum, pending-selection context, suppress-focus-flash latch) over a synchronous two-way NotificationDismissalHosting seam (same isolation rationale as CmuxSidebarGit: one-turn read/write interleavings must not gain suspension points). Tests: 7 (CmuxSidebar) + 8 (CmuxBrowser) + 10 (CmuxNotifications) new package tests green, covering selection mutations, event wire shape, stack bounds, and the dismissal-state transition matrix incl. side-effect ordering. Net: +1147 lines (packages only). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… drains via CmuxRemote* packages Stacks the Workspace remote/cloud lift (CmuxCore + CmuxRemoteDaemon + CmuxRemoteWorkspace + CmuxRemoteSession, PR 5896 by azooz2003-bit) under this branch instead of re-lifting the same ~7.5k lines in parallel. Workspace.swift: 19,552 -> 11,972 lines. Conflict resolutions (verified by machine-diff: zero novel lines vs either parent): - Workspace.swift hunk 1: drop both stale halves (remote types lifted by 5896, sidebar vocabulary lifted by this branch); keep the BrowserPanelRestoreSnapshot conformance line from HEAD. - Hunks 2-3: keep this branch's surfaceTTYNames / panelShellActivityStates forwarding accessors (SurfaceRegistryModel), take 5896's RemoteSessionCoordinator property type and DEBUG process-runner seam. - Import unions in ContentView, TerminalController(+ControlWorkspaceContext), GhosttyConfigTests (package imports hoisted above canImport(cmux_DEV)). - pbxproj: keep this branch's SplitEqualizer.swift removal (lifted into CmuxPanes), take 5896's SSHPTYAttachStartupCommandBuilder.swift wiring; normalized, checked, no duplicate definition IDs. - swift-file-length-budget.tsv regenerated from the merged tree. Gates: CmuxCore/CmuxRemoteDaemon/CmuxRemoteWorkspace/CmuxRemoteSession swift build + swift test green (39/16/41/18 tests); lint-ios-package- conventions OK; budget OK; app xcodebuild Debug build succeeded. PR 5991 now depends on PR 5896 landing (or carries its diff); noted in the PR body. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…muxPanes Tranche 7 (behavior follows tranche-5 storage): the detach-transaction state transitions move onto SplitLayoutModel as six choreography verbs, each body the verbatim legacy mutation: - markDetaching / cancelDetach (detachingTabIds insert / rollback remove + pending discard) - openDetachCloseTransaction / closeDetachCloseTransaction (+= 1 / max(0, n-1) clamp) - consumeDetachingMark (remove != nil || isDetachingCloseTransaction, legacy short-circuit preserved) - storeDetachedTransfer / takeDetachedTransfer (pending subscript-assign / removeValue) Workspace.detachSurface and the close pipeline call the verbs 1:1; the bonsplit closeTab seam, forceCloseTabIds close-bookkeeping family, publish, and remote cleanup stay app-side (the close family remains whole for the future SplitLifecycleCoordinator lift, per the tranche-5 residue note). The now-mutation-free forwarding accessors narrow to read-only and the unused detachingTabIds accessor is deleted (compile-proven: no remaining users). Package tests pin the verb semantics: success path, rollback, transaction-open consume for unmarked tabs, and the max(0) clamp. Gates: CmuxPanes swift build + swift test green (24 tests); lint OK; budget OK; app xcodebuild Debug build succeeded. Adversarial diff: every removed Workspace line maps 1:1 to a verb whose body is the moved mutation; the DetachedSurfaceTransfer argument list is byte-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resync the Workspace-decomposition slice (PR 5991) onto main after the TabManager-decomposition (PR 5921) merged via squash. Squash-lineage: the slice carried 5921's original commits; main now has the squashed equivalent, so TabManager + Cmux* package overlaps resolved by keeping unified content (5921's merged decomposition + 5991's Workspace sub-models / SurfaceRegistryModel / SplitLayoutModel / detach choreography), no duplication. Conflict resolution highlights: - AppDelegate: dropped slice's local runMultiWindowRouteCLI re-add (lifted to CmuxIPCService on main). - TabManager: kept main's workspaceGroupsPublisher bridge (5921), the publisher MobileWorkspaceListObserver consumes. - Workspace.swift: took the slice's package decomposition (~8k lines relocated); ported main's WorkspaceRemoteSessionController fixes into the relocated CmuxRemoteSession package (remotePlatformProbeScript + version sanitization, marker-stripped user-facing stdout, armv7 arch). Auto-merged main's Workspace.createdAt. - Silent cross-file breaks fixed: main's extracted NotificationSoundSettings.swift rewired to CmuxFoundation (readDataToEndOfFileOrEmpty instance method); deleted main's orphaned WorkspaceRemotePlatformProbeScript.swift (extended the slice-deleted app-target WorkspaceRemoteSessionController) and its pbxproj entries. - Restored import CmuxSidebar where the slice moved SidebarPullRequestState / SidebarStatusEntry into CmuxSidebar (TabManager+SidebarGitHosting, FeedCoordinator). - pbxproj: union of package/file refs, dropped relocated RemoteLoopbackProxyAlias app-target ref; normalized + check-pbxproj + xcodebuild -list pass. - SurfaceKind: new main lint rule (namespace-types) flags the slice's all-static frozen-wire-string namespace; added reviewed inline lint:allow (rawValue enum would change persistence-boundary semantics at ~58 String call sites). - swift-file-length-budget.tsv regenerated. Local: xcodebuild app build SUCCEEDED; budget/lint/pbxproj gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The app-target cmuxTests/WorkspaceRemotePlatformProbeTests.swift (from PR 6056's OpenWrt BusyBox probe fix) referenced WorkspaceRemoteSessionController, which this slice deleted from the app target (logic lifted into the CmuxRemoteSession RemoteSessionCoordinator extension). Only the cmux-unit `tests` job compiles cmuxTests, so the local app build missed it. Retargeted onto the lifted package type (WorkspaceRemoteSessionController -> RemoteSessionCoordinator) with @testable import CmuxRemoteSession, same pattern as the existing RemoteReconnectPolicyTests/RemoteSessionProcessRunnerTests in this package. Assertions unchanged; coverage of the probe script, version sanitization, and marker-stripped user-facing stdout preserved. Removed the app-target test file and its four pbxproj entries; package test target compiles + links. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to main 5991 carried #5896's ORIGINAL CmuxRemote* commits, predating the squash-merge of #5896 (and its follow-up fixes #6056 OpenWrt BusyBox probe, @suite(.serialized) test serialization) into main. Merging as-is would have re-added the entire CmuxRemote* package set and reverted main's newer remote fixes. Resolution: take MAIN's version for everything under Packages/CmuxRemoteDaemon, Packages/CmuxRemoteSession, Packages/CmuxRemoteWorkspace, and Packages/CmuxCore/ Sources/CmuxCore/Remote, so 5991 owns ZERO remote-package changes. Removed 5991's own RemotePlatformProbeTests.swift (replaced by main's RemotePlatformProbeScriptTests.swift) and the deleted-both-sides cmuxTests/WorkspaceRemotePlatformProbeTests.swift. Workspace.swift conflicts resolved to 5991's workspace-decomposition form (surfaceTTYNames / panelShellActivityStates delegate to surfaceRegistry sub-model; Sidebar* value types now live in the CmuxSidebar package). GhosttyConfigTests import order taken from main. Budget tsv regenerated from the merged tree. Verified: git diff origin/main HEAD over the three remote packages is empty; CmuxRemote* name-only diff count is 0; remaining diff vs main is workspace-only. 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.
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 `@Sources/Workspace.swift`:
- Around line 2561-2564: The current `get`/`set` forwarding accessors on the
`panels` property (and similarly on `surfaceIdToPanelId`, `surfaceTTYNames`, and
`panelShellActivityStates`) trigger unnecessary copy-in/copy-out mutations when
performing subscript assignments or removals, breaking Swift's copy-on-write
optimization for dictionaries. Replace the `get` and `set` accessors with
`_read` and `_modify` accessors respectively on all four properties to enable
in-place mutations and preserve unique reference semantics. The `_read` accessor
should yield the dictionary, and the `_modify` accessor should yield a mutable
reference to allow mutations like subscript assignment and removal to occur
in-place without triggering copies.
🪄 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: b0d14be5-18b5-4e08-ae62-75b484eac302
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (54)
Packages/CmuxNotifications/Sources/CmuxNotifications/RestoredPanelUnreadIndicator.swiftPackages/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swiftPackages/CmuxPanes/Sources/CmuxPanes/Model/PaneTreeHosting.swiftPackages/CmuxPanes/Sources/CmuxPanes/Model/PaneTreeModel.swiftPackages/CmuxPanes/Sources/CmuxPanes/Model/SplitLayoutModel.swiftPackages/CmuxPanes/Tests/CmuxPanesTests/PaneTreeModelTests.swiftPackages/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swiftPackages/CmuxPanes/Tests/CmuxPanesTests/SplitLayoutModelTests.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Git/SidebarBranchOrdering.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Git/SidebarGitBranchState.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Git/SidebarPullRequestState.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Git/SidebarPullRequestStatus.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Git/String+SidebarBranchName.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Status/SidebarLogEntry.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Status/SidebarLogLevel.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Status/SidebarMetadataBlock.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Status/SidebarMetadataFormat.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Status/SidebarProgressState.swiftPackages/CmuxSidebar/Sources/CmuxSidebar/Status/SidebarStatusEntry.swiftPackages/CmuxSidebar/Tests/CmuxSidebarTests/SidebarValueVocabularyTests.swiftPackages/CmuxWorkspaceCore/Package.swiftPackages/CmuxWorkspaceCore/Sources/CmuxWorkspaceCore/Model/SurfaceRegistryModel.swiftPackages/CmuxWorkspaceCore/Sources/CmuxWorkspaceCore/Values/FocusPanelTrigger.swiftPackages/CmuxWorkspaceCore/Sources/CmuxWorkspaceCore/Values/PanelShellActivityState.swiftPackages/CmuxWorkspaceCore/Sources/CmuxWorkspaceCore/Values/PendingNonFocusSplitFocusReassert.swiftPackages/CmuxWorkspaceCore/Sources/CmuxWorkspaceCore/Values/SurfaceKind.swiftPackages/CmuxWorkspaceCore/Sources/CmuxWorkspaceCore/Values/WorkspacePendingTerminalInputReason.swiftPackages/CmuxWorkspaceCore/Tests/CmuxWorkspaceCoreTests/SurfaceRegistryModelTests.swiftPackages/CmuxWorkspaceCore/Tests/CmuxWorkspaceCoreTests/WorkspaceCoreValueTests.swiftSources/AppDelegate.swiftSources/Feed/FeedCoordinator.swiftSources/Mobile/MobileWorkspaceListObserver.swiftSources/TabManager+SidebarGitHosting.swiftSources/TabManager.swiftSources/TerminalController+ControlSidebarContext.swiftSources/TerminalController+ControlSidebarContext2.swiftSources/TerminalController+ControlSurfaceContext4.swiftSources/TerminalController+ControlWorkspaceContext.swiftSources/TerminalController.swiftSources/TerminalNotificationStore.swiftSources/Workspace+DetachedSurfaceTransfer.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace.swiftSources/WorkspacePortalPaneDrop.swiftSources/WorkspaceSidebarObservation.swiftcmux.xcodeproj/project.pbxprojcmux.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedcmuxTests/BrowserConfigTests.swiftcmuxTests/SidebarOrderingTests.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/TerminalNotificationClearAllTests.swiftcmuxTests/WorkspacePullRequestSidebarTests.swiftcmuxTests/WorkspaceSidebarObservationTests.swiftcmuxTests/WorkspaceUnitTests.swift
Comment on lines
+2561
to
+2564
| var panels: [UUID: any Panel] { | ||
| get { paneTree.panels } | ||
| set { paneTree.panels = newValue } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift, when mutating a copy-on-write collection through a computed property that only has getandset, does the compiler use copy-in/copy-out semantics, and is _modify the correct way to preserve in-place mutation for Dictionary/Array/Set-backed properties?
💡 Result:
When you access a Copy-on-Write (COW) collection through a computed property with only a get and set accessor, the Swift compiler treats the interaction as a get-modify-set sequence [1][2]. This process creates a temporary copy of the collection, which breaks the unique reference required for in-place mutation, thereby triggering an unnecessary structural copy of the backing buffer [1][3][2]. While the language semantics for in-out parameters are described as copy-in/copy-out, the compiler optimizes them for performance; however, this optimization cannot bridge the gap for computed properties because the setter is conceptually an assignment of a new value, not a modification of the original storage [4][5][1]. The _modify accessor (and similar yielding accessors like borrow/mutate) is the correct way to preserve in-place mutation for collection-backed properties [1][6][3]. By using a yielding accessor, you allow the compiler to lend the underlying storage directly to the caller, maintaining the unique reference and avoiding the overhead of creating temporary copies [1][6][2]. Key considerations for _modify include: - It is a non-public, underscored language feature, meaning its implementation or availability could theoretically change, although it is widely used throughout the Swift standard library and collection packages [7][3]. - Proper usage involves yielding the collection with & (e.g., yield &self._storage) to provide a mutable, in-place reference [8]. - When using _modify, it is often beneficial to ensure the accessor can be inlined by the compiler, as this is critical for achieving optimal performance and avoiding extra allocations [7][9]. - Emerging language features, such as borrow and mutate accessors proposed in the Swift Evolution process, are intended to provide official, stable alternatives to these underscored accessors [6].
Citations:
- 1: https://github.com/swiftlang/swift-evolution/blob/eab7cae4fb066dd6b954a761cb8341b6cc89d148/proposals/0474-yielding-accessors.md
- 2: https://github.com/apple/swift/blob/main/docs/proposals/Accessors.rst
- 3: https://forums.swift.org/t/array-copying-and-computed-properties/63903
- 4: https://www.cosmiclearn.com/swift/in_out_parameters.php
- 5: https://stackoverflow.com/questions/46210631/accessing-a-var-passed-as-inout-is-undefined-behavior
- 6: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0507-borrow-accessors.md
- 7: Force-inline _modify accessors to work around a performance issue apple/swift-collections#165
- 8: https://forums.swift.org/t/reducing-allocations-in-modify-accessors/70471
- 9: Implement generalized accessors using yield-once coroutines swiftlang/swift#18156
🏁 Script executed:
# Check the exact implementation at the flagged lines
sed -n '2561,2564p' Sources/Workspace.swift
echo "---"
sed -n '2733,2736p' Sources/Workspace.swift
echo "---"
sed -n '2779,2782p' Sources/Workspace.swift
echo "---"
sed -n '3495,3498p' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 692
🏁 Script executed:
# Count mutation patterns for the forwarded CoW collections
echo "=== panels mutations ==="
rg 'panels\[|panels\.remove|panels\.removeAll' Sources/Workspace.swift -c
echo ""
echo "=== surfaceIdToPanelId mutations ==="
rg 'surfaceIdToPanelId\[|surfaceIdToPanelId\.remove|surfaceIdToPanelId\.removeAll' Sources/Workspace.swift -c
echo ""
echo "=== surfaceTTYNames mutations ==="
rg 'surfaceTTYNames\[|surfaceTTYNames\.remove|surfaceTTYNames\.removeAll' Sources/Workspace.swift -c
echo ""
echo "=== panelShellActivityStates mutations ==="
rg 'panelShellActivityStates\[|panelShellActivityStates\.remove|panelShellActivityStates\.removeAll' Sources/Workspace.swift -cRepository: manaflow-ai/cmux
Length of output: 214
🏁 Script executed:
# Check if paneTree is mutable and what's the backing storage type
rg 'var paneTree|let paneTree' Sources/Workspace.swift -A 2Repository: manaflow-ai/cmux
Length of output: 176
🏁 Script executed:
# Look at actual mutation patterns in context (sample a few)
echo "=== Sample panels mutations ==="
rg 'panels\[' Sources/Workspace.swift -B 1 -A 1 | head -40Repository: manaflow-ai/cmux
Length of output: 1889
🏁 Script executed:
# Check if PaneTreeModel.panels is a Dictionary (CoW type)
rg -A 5 'var panels.*Panel\]' Sources/ -t swift | head -50Repository: manaflow-ai/cmux
Length of output: 813
🏁 Script executed:
# Check if this is a newly-introduced pattern or pre-existing by looking at git diff context
# Search for whether these forwarded properties existed before
git log --oneline -20Repository: manaflow-ai/cmux
Length of output: 138
🏁 Script executed:
# Search for the actual definition of PaneTreeModel to understand the backing storage
fd -t f 'PaneTreeModel' Sources/ && rg 'struct PaneTreeModel|class PaneTreeModel' Sources/ -A 10Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Search for PaneTreeModel more broadly
rg 'PaneTreeModel' Sources/ -lRepository: manaflow-ai/cmux
Length of output: 84
🏁 Script executed:
# Look at the actual definition if it exists
rg 'class PaneTreeModel|struct PaneTreeModel' -A 20Repository: manaflow-ai/cmux
Length of output: 4623
🏁 Script executed:
# Check if panels property on PaneTreeModel is mutable
rg 'var panels.*Dictionary|var panels.*\[.*\]' -A 2 | head -40Repository: manaflow-ai/cmux
Length of output: 2461
🏁 Script executed:
# Determine how panels is actually mutated - are these subscript assigns or batch operations?
rg 'panels\s*\[.*\]\s*=' Sources/Workspace.swift | head -20Repository: manaflow-ai/cmux
Length of output: 1013
🏁 Script executed:
# Check if these forwarding properties existed before the feat-workspace-model branch
# by looking at what changed in this commit
git show HEAD:Sources/Workspace.swift | grep -A 3 'var panels.*Panel\]' | head -20Repository: manaflow-ai/cmux
Length of output: 177
🏁 Script executed:
# Get the actual mutation patterns to understand the scale of the problem
rg 'panels\[.*\]\s*=' Sources/Workspace.swift | wc -l
rg 'panels\.remove|panels\.removeAll' Sources/Workspace.swift | wc -lRepository: manaflow-ai/cmux
Length of output: 66
🏁 Script executed:
# Check the structure of how mutations happen - are they in loops or hot paths?
rg 'panels\[.*\]\s*=' Sources/Workspace.swift -B 3 -A 1 | grep -E '(for|while|func|panels\[)' | head -30Repository: manaflow-ai/cmux
Length of output: 1119
🏁 Script executed:
# Check the parent of this commit to see what changed
git log --oneline -5Repository: manaflow-ai/cmux
Length of output: 138
🏁 Script executed:
# Check if the feat-workspace-model branch introduced these forwarding properties
# by comparing against main
git diff main..HEAD -- Sources/Workspace.swift | grep -A 5 -B 5 'var panels\|var surfaceIdToPanelId\|var surfaceTTYNames\|var panelShellActivityStates' | head -80Repository: manaflow-ai/cmux
Length of output: 2888
🏁 Script executed:
# Alternatively, check which lines were actually added/modified in this PR
git diff main..HEAD -- Sources/Workspace.swift | grep -E '^\+.*var (panels|surfaceIdToPanelId|surfaceTTYNames|panelShellActivityStates)'Repository: manaflow-ai/cmux
Length of output: 254
🏁 Script executed:
# Look at the actual mutations - are any in loops?
rg 'for.*panels|while.*panels' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 1528
Avoid get/set forwarding for the hot CoW registries—use _modify/_read instead.
These forwarding properties introduce a newly-added regression: panels[id] = …, surfaceIdToPanelId[tab] = …, surfaceTTYNames[id] = …, and panelShellActivityStates[id] = … now trigger copy-in/copy-out mutations instead of in-place updates. The original code used @Published var panels: [UUID: any Panel] = [:] (mutable backing), but the refactor changed it to computed properties with only get and set accessors, which breaks the unique-reference requirement for Dictionary copy-on-write optimization.
This affects 38+ mutations per workflow pass on panels alone (22 subscript assignments, 16 removals), with similar counts for the other three properties. Many occur in loop contexts (e.g., for panelId in panels.keys, for panel in panels.values), which compounds the cost during split/restore/close choreography.
♻️ Suggested fix pattern
- var panels: [UUID: any Panel] {
- get { paneTree.panels }
- set { paneTree.panels = newValue }
- }
+ var panels: [UUID: any Panel] {
+ _read { yield paneTree.panels }
+ _modify { yield &paneTree.panels }
+ }Apply the same _read / _modify pattern to surfaceIdToPanelId, surfaceTTYNames, and panelShellActivityStates.
🤖 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/Workspace.swift` around lines 2561 - 2564, The current `get`/`set`
forwarding accessors on the `panels` property (and similarly on
`surfaceIdToPanelId`, `surfaceTTYNames`, and `panelShellActivityStates`) trigger
unnecessary copy-in/copy-out mutations when performing subscript assignments or
removals, breaking Swift's copy-on-write optimization for dictionaries. Replace
the `get` and `set` accessors with `_read` and `_modify` accessors respectively
on all four properties to enable in-place mutations and preserve unique
reference semantics. The `_read` accessor should yield the dictionary, and the
`_modify` accessor should yield a mutable reference to allow mutations like
subscript assignment and removal to occur in-place without triggering copies.
Source: Coding guidelines
# Conflicts: # .github/swift-file-length-budget.tsv # cmux.xcodeproj/project.pbxproj
# Conflicts: # .github/swift-file-length-budget.tsv
4 tasks
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.
Stacked diff on purpose: this PR targets
feat-tabmanager-models(#5921), the TabManager slice it builds on. Do not retarget to main. As of tranche 6 this branch also mergesfeat-remote-workspace(#5896), so the diff below includes 5896's CmuxRemote lift until 5896 lands on main; merge 5896 first (or accept that this PR carries it).*Stack B of the modular refactor: decomposition of the
Workspacegod model (Sources/Workspace.swift) perWorkspace.plan.md. At this PR's base the file is 19,992 lines; after tranches 1-3 it is 19,491; tranches 4-5 (storage moves below) bring it to 19,552. The +61 from tranches 4-5 is the forwarding-accessor tax of a pure storage move (every moved property keeps a byte-compatible accessor onWorkspace, so zero call sites churn); the net win lands when the behavior that mutates this state follows the storage into the sub-models.Tranches landed
CmuxWorkspaceCore bootstrap + lifted vocabulary (new package, future home of
WorkspaceModel):SurfaceKind(namespace enum -> value-typed namespace struct, wire strings frozen + test-pinned),PanelShellActivityState(control-socket/session-snapshot raw values test-pinned),FocusPanelTrigger,WorkspacePendingTerminalInputReason(absorbs the case-lessWorkspacePendingTerminalInputPolicy; the timeout lookup becomes a property on the reason).RestoredPanelUnreadIndicator->CmuxNotificationsper plan. All call sites converged, no compat shims.Sidebar value vocabulary -> CmuxSidebar + CmuxPanes (plan Wave 3 p01/p06):
SidebarStatusEntry,SidebarMetadataBlock,SidebarMetadataFormat,SidebarLogLevel,SidebarLogEntry,SidebarProgressState,SidebarGitBranchState,SidebarPullRequestStatus,SidebarPullRequestStateinto CmuxSidebar (Status/, Git/ role folders); the free functionnormalizedSidebarBranchNamebecomesString.normalizedSidebarBranchName.SidebarBranchOrdering(case-less namespace enum) becomes an instantiated Sendable value,WorkspaceReorderPlannerprecedent, bodies verbatim. The two pane-tree spatial-order derivations move onto their natural receiver asExternalTreeNodeextensions in CmuxPanes (which already owns the Bonsplit edge; CmuxSidebar stays Bonsplit-free) next to the existing+SplitGeometryprecedent.PaneTreeModel (CmuxPanes) — the deferred handoff from the TabManager slice: Workspace's pane-tree stored state (
panels,paneLayoutVersion,surfaceIdToPanelId,lastOrderedPanelIds) moves into@MainActor @Observable PaneTreeModel<Panel>(app bindsany Panel). Published-parity viaPaneTreeHostingwillSet hooks re-emittingobjectWillChange+CurrentValueSubjectbridges (panelsPublisher,paneLayoutVersionPublisher) at exact legacy timing (new-value-at-willSet, replay-on-subscribe, fires-on-equal-assignment, pinned by package tests). All 16$panels/$paneLayoutVersionsubscriber sites moved to the bridges after per-site analysis (all signal-only, count-mapping, or CombineLatest inputs — subject-compatible). Workspace keeps byte-compatible forwarding accessors; zero call-site churn outside the observation seams.SurfaceRegistryModel (CmuxWorkspaceCore) — plan Wave-4 p07 sub-model, storage move:
pendingTabSelection,isApplyingTabSelection,pendingNonFocusSplitFocusReassert,nonFocusSplitFocusReassertGeneration,surfaceTTYNames,panelShellActivityStatesmove into@MainActor @Observable SurfaceRegistryModel<TabSelectionRequest>.PendingNonFocusSplitFocusReassertlifts to CmuxWorkspaceCore as a Sendable value (fields verbatim);PendingTabSelectionRequeststays app-side (it carriesGhosttySurfaceScrollView/PanelFocusIntentreferences) and binds the generic parameter, the same shape asPaneTreeModel<any Panel>. The registry queries move ontoPaneTreeModelaspanelId(forSurfaceId:)/surfaceId(forPanelId:)(bodies verbatim, including the dictionary-order-arbitrary reverse lookup);Workspace.panelIdFromSurfaceId/surfaceIdFromPanelIdbecome forwarders. None of the moved properties were@Published, so unlike tranche 3 there is no observer contract to preserve: no hooks, no bridges, no$sites (asserted by grep across Sources/ and CLI/).SplitLayoutModel (CmuxPanes) — plan Wave-4 p07 sub-model, storage move:
isProgrammaticSplit,detachingTabIds,pendingDetachedSurfaces,activeDetachCloseTransactionsmove into@MainActor @Observable SplitLayoutModel<Transfer>;isDetachingCloseTransactionmoves as the derived flag (body verbatim). Lives in CmuxPanes because the state is keyed by BonsplitTabIDand CmuxPanes owns that edge (tranche-2/3 precedent);DetachedSurfaceTransferstays app-side (panel references + app-domain snapshots) and binds the generic parameter. None of the moved properties were@Published; no observer seams exist.Remote/cloud half via PR 5896 merge (tranche 6). Instead of re-lifting the ~7.5k-line remote half in parallel with the in-flight CmuxRemote* family: extract Workspace remote/cloud-VM connectivity (transport, daemon RPC, proxy/relay, session coordinator) #5896 (same domain, file-for-file collision), this branch merges
origin/feat-remote-workspace(commit 5d63306). That brings inCmuxCore,CmuxRemoteDaemon,CmuxRemoteWorkspace,CmuxRemoteSessionand drains all socket/RPC/SSH/PTY/port-scan code from Workspace.swift: 19,552 -> 11,972 lines. Conflict resolution was machine-diff verified: zero lines in the merged Workspace.swift that are not attributable to exactly one parent (3 hand-resolved hunks: dual stale-vocabulary drop, and this branch's SurfaceRegistryModel forwarding accessors combined with 5896'sRemoteSessionCoordinatorproperty + DEBUG process-runner seam). Budget tsv regenerated; pbxproj keeps this branch's SplitEqualizer removal + 5896's SSHPTYAttachStartupCommandBuilder wiring. Gates: all four incoming packagesswift build+swift testgreen (39/16/41/18), lint OK, app build green.SplitLayoutModel detach choreography verbs (tranche 7). Behavior follows the tranche-5 storage: the detach-transaction state transitions move onto
SplitLayoutModelas six verbs (markDetaching,openDetachCloseTransaction,closeDetachCloseTransaction,cancelDetach,consumeDetachingMark,storeDetachedTransfer/takeDetachedTransfer), each body the verbatim legacy mutation including themax(0, n-1)clamp and theremove != nil || isDetachingCloseTransactionshort-circuit.Workspace.detachSurfaceand the close pipeline call them 1:1; the bonsplitcloseTabseam, theforceCloseTabIdsclose-bookkeeping family, publish, and remote cleanup stay app-side so the close family remains whole for the futureSplitLifecycleCoordinatorlift. Mutation-free forwarding accessors narrowed to read-only; the unuseddetachingTabIdsaccessor deleted (compile-proven). Five new package tests pin the verb semantics.Verification
Per tranche: package
swift build+swift testgreen (CmuxWorkspaceCore 7, CmuxNotifications 10, CmuxSidebar 12, CmuxPanes 20); appxcodebuildgreen;swift_file_length_budget.py(--write-budget ratchet in cutover commits) andlint-ios-package-conventions.shexit 0; pbxproj untouched in tranches 4-5 (no new app-target files). Adversarial machine-diff per tranche: tranche 1 member lists/cases/bodies byte-identical modulopublic/DocC; tranche 2SidebarBranchOrderingbody differs only on the type-declaration line after normalizing visibility/static spelling (tree-walk bodies normalized only at the receiver seam); tranche 3 is a storage move with hook parity pinned by tests and a 16-line one-token$sitediff; tranches 4-5 move only property declarations plus three one-expression bodies (panelIdFromSurfaceId,surfaceIdFromPanelId,isDetachingCloseTransaction), each verbatim modulo receiver/visibility, with defaults pinned by package tests. Wire formats (enum raw values, SurfaceKind strings) pinned by package tests; defaults keys/codecs untouched.Boundary justifications
CmuxWorkspaceCoreis the plan-assigned home of the decomposedWorkspaceModel; this PR bootstraps it with the p07 vocabulary (stack-ACmuxWorkspacesbootstrap precedent). The plan'sCmuxCoreleaf was not created for the two pending-terminal-input values: a new package holding two enums fails scope sufficiency; they live in CmuxWorkspaceCore and can lift further when more cross-domain values accumulate.SurfaceRegistryModelis in CmuxWorkspaceCore (per plan) and stays Bonsplit-free: everything it stores is keyed by workspace-sideUUIDs, and the one Bonsplit-typed concern (the surface-id map and its queries) stays inPaneTreeModel. CmuxPanes deliberately does NOT depend on CmuxWorkspaceCore: the futureWorkspaceModelin CmuxWorkspaceCore will composePaneTreeModel, so that edge must point CmuxWorkspaceCore -> CmuxPanes, never the reverse.SplitLayoutModelis in CmuxPanes, not CmuxWorkspaceCore, because its state isTabID-keyed (same reasoning as tranche 3).Remaining (stated, per plan)
The remote/cloud halfDone via tranche 6 (merge of CmuxRemote* family: extract Workspace remote/cloud-VM connectivity (transport, daemon RPC, proxy/relay, session coordinator) #5896; this PR now depends on 5896 landing first or carries its diff). Original note: the remote/cloud half of Workspace.swift (~7.5k lines) was being drained in CmuxRemote* family: extract Workspace remote/cloud-VM connectivity (transport, daemon RPC, proxy/relay, session coordinator) #5896 (author azooz2003-bit, based on main, checks green as of 2026-06-12), which creates the plan's Wave-2CmuxRemoteDaemon/CmuxRemoteWorkspace/CmuxRemoteSessionpackages plusCmuxCore. Stack B deliberately does not touch the remote half: a parallel lift here would collide file-for-file with that in-flight PR (the ledger's parallel-redesign lesson). Once 5991 and 5896 both land on main, the remote sub-model wiring (RemoteSessionCoordinatorownership inside the futureWorkspaceModel) is the follow-up.Workspace.init+ session-restoreconfigureRemoteConnectionsequencing) belongs toSessionRestoreCoordinator(CmuxAgentRuntime, Wave 3, package does not exist yet) and the full god-class move (Wave 4 p07); both are later-sequenced per the plan. TheWorkspaceModelrename is likewise not attempted: the plan sequences it with the Wave-4 p07 completion after the Wave-3 feature coordinators exist.WorkspaceModelrename/final shape: the plan sequences it with the full god-class move (Wave 4 p07 completion), after the remote half and the feature coordinators exist; not attempted this round.SessionSnapshotRepository: per the plan and the 5921 residue note, the encode/decode + file write (SessionPersistence.swift) is AppDelegate-fleet work; the Workspace-side session-restore extension stays an app-target shim untilSessionRestoreCoordinator(CmuxAgentRuntime) lands. Left in place deliberately.forceCloseTabIds,pendingCloseConfirmTabIds,explicitUserCloseTabIds, close-history sets,postCloseSelectTabId, pane-close pending maps): plan-assigned toSplitLifecycleCoordinator(the BonsplitDelegate inversion), which is a behavior lift, not a storage move; deferred to that tranche rather than inventing an intermediate model.@Publishedtmux mirror quartet (tmuxLayoutSnapshot,tmuxWorkspaceFlash*): not assigned to a sub-model by the plan; needs the tranche-3 hook pattern when its domain home (tmux mirroring) is decided.e2e
NewBrowserWorkspaceShortcutUITestshas now been dispatched 6x and cancelled every time by runner-pool preemption (latest: run 27450502997, 2026-06-13). Noting and moving on per the cancellation policy; the suite is unrelated to any tranche here (no browser-workspace-shortcut code touched) and should be re-run once the warp runner pool has headroom.BonsplitTabDragUITests(drag-detach choreography, the path tranches 5-7 touch) passed on this head: run 27450503785.🤖 Generated with Claude Code
Summary by CodeRabbit