Close only the workspaces a tab manager actually owns

[ejc3]

closeWorkspace tears a workspace down before it checks that the workspace belongs to this manager, so
handing it a workspace from another window destroys that workspace while leaving it on screen.

What the user sees

Two windows open. Something asks window A's tab manager to close a workspace that lives in window B.
Window B's workspace keeps its tab and stays visible, but its terminals are gone: the panels were torn
down, the Ghostty surfaces freed, and freeing a surface SIGHUPs the child processes. A workspace-closed
event is published for a workspace that is still open, so anything listening downstream is told about a
close that did not happen, and owningTabManager is left nil.

Why it happens

func closeWorkspace(_ workspace: Workspace, recordHistory: Bool = true) {
    guard tabs.count > 1 else { return }

That is the only precondition. Everything destructive then runs unconditionally:
workspace.teardownAllPanels(), workspace.teardownRemoteConnection(), owningTabManager = nil, and
publishCmuxWorkspaceClosed(workspace). teardownAllPanels runs discardClosedPanelLifecycleState
per panel, which calls panel?.close() and removes the entry from panels and panelTitles.

Membership in tabs is only enforced at the very end, when the array element is removed. The
recordHistory block does look the index up earlier, but only to decide where in the history to
record, and it does not stop the teardown.

The guard was there, and a merge dropped it

#889 ("Fix orphaned child processes when closing workspace tabs") added teardownAllPanels() and,
directly above it, a tabs.firstIndex(where:) guard — the destructive step and its precondition landed
in the same commit, along with the test that covers this. git log -L on those lines shows the guard
removed, restored by a revert, then removed again by a "Reapply" merge commit that kept the teardown.

Two call sites already make this check themselves rather than relying on closeWorkspace: AppDelegate
re-checks sourceManager.tabs.contains before closing a source workspace, and TerminalController
records existedBefore and skips candidates that fail it. Both predate #889, so they are not
compensating for the lost guard — they are evidence that callers have always needed this precondition
and have been paying for it individually.

One path does change behavior. Workspace.swift resolves a manager as
owningTabManager ?? AppDelegate.shared?.tabManagerFor(tabId:) ?? AppDelegate.shared?.tabManager, and
that last fallback is reached precisely when no manager owns the workspace. Previously such a call tore
the workspace down through an unrelated manager; now it returns early. That is the intent of the guard,
but it is a real change rather than a no-op, so it is worth a reviewer's attention.

Test plan

Measured on 4253cc2884, one GUI test host at a time, same checkout and DerivedData for both arms:

xcodebuild test -scheme cmux-unit -configuration Debug -destination 'platform=macOS' \
  -only-testing:cmuxTests/WorkspacePullRequestSidebarTests \
  -only-testing:cmuxTests/TabManagerPullRequestProbeTests \
  -only-testing:cmuxTests/TabManagerWorkspaceOwnershipTests \
  -only-testing:cmuxTests/CLICodexHookTimeoutRegressionTests

arm	XCTest failures across a 36-test run	distinct tests red
main unchanged	15 of 36	9
this branch	13 of 36	8

testCloseWorkspaceIgnoresWorkspaceNotOwnedByManager is the test that flips: it hands the manager a
foreign workspace and checks that the workspace keeps its panel, which is the terminal that would
otherwise be killed. On main it fails twice, XCTAssertEqual failed: ("0") is not equal to ("1") for
the panel count and ("[:]") for the panel titles, which is the foreign workspace being emptied.

No host restarts in either arm, and CLICodexHookTimeoutRegressionTests reports Test run with 8 tests in 1 suite passed in both.

Pull-request CI on this repo runs review bots and security scanners, not the test suite, so the arms
above are the only test evidence this carries.

The eight tests still red on this branch are test-side failures in the same suites: two git-index
fixtures describing a state git cannot produce, a scoped socket report sent to an unresolvable manager,
a stub watching a subprocess the product no longer spawns, and four waits that resolve to a blocking
helper.

Summary by CodeRabbit

• Bug Fixes
  • Prevented unintended workspace shutdown actions when attempting to close a workspace that isn’t currently managed by the active tab manager.
  • Avoided unwanted focus/selection changes and suppressed the workspace-closed event when an invalid close request is received.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 955c1361-8e43-4fa3-b293-bc8357ee3890

📥 Commits

Reviewing files that changed from the base of the PR and between 1bc4fe4 and 89acd5f.

📒 Files selected for processing (1)

• Sources/TabManager.swift

---

📝 Walkthrough

Walkthrough

TabManager.closeWorkspace now verifies that the workspace belongs to the manager before performing teardown, publishing events, or mutating state.

Changes

Workspace closure

Layer / File(s)	Summary
Guard workspace ownership Sources/TabManager.swift	closeWorkspace returns early when the workspace ID is absent from tabs, preventing subsequent closure side effects.

Estimated code review effort: 2 (Simple) | ~5 minutes

Suggested reviewers: austinywang, lawrencecchen, azooz2003-bit

🚥 Pre-merge checks | ✅ 25

✅ Passed checks (25 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely matches the ownership-guard change in closeWorkspace.
Description check	✅ Passed	The description covers what changed, why, and how it was tested, with only a few noncritical template sections left out.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation	✅ Passed	TabManager and Workspace are already @MainActor; the diff only adds an ownership guard inside @MainActor closeWorkspace, with no new isolation or Sendable debt.
Cmux Swift Blocking Runtime	✅ Passed	The diff only adds an ownership guard in closeWorkspace; it introduces no waits, sleeps, sync calls, locks, or polling.
Cmux Browser Automation Off-Main	✅ Passed	Diff only adds an ownership guard in TabManager.closeWorkspace; no browser socket command routing or WebKit wait handling changed, and the rule targets different files.
Cmux Expensive Synchronous Load	✅ Passed	The diff only adds an ownership guard before the existing closeWorkspace history path; no new or moved RestorableAgentSessionIndex.load() on the main-actor close path was introduced.
Cmux Cache Substitution Correctness	✅ Passed	Diff only adds an ownership guard before teardown; no fresh authoritative read was replaced by a stale cache in a persistence/history path.
Cmux No Hacky Sleeps	✅ Passed	PASS: PR only changes Sources/TabManager.swift (Swift) and adds an ownership guard; no non-Swift runtime code or sleep/delay workaround was introduced.
Cmux Algorithmic Complexity	✅ Passed	The PR only adds a single tabs.contains(where:) ownership guard to closeWorkspace; that’s one linear scan on an infrequent close action, with no nested or batch rescans.
Cmux Swift Concurrency	✅ Passed	Diff only adds a synchronous ownership guard in TabManager.closeWorkspace; it introduces no Dispatch/Task/Combine/completion-handler concurrency pattern.
Cmux Swift @Concurrent	✅ Passed	Diff only adds a synchronous membership guard in an @MainActor class; no new async helpers or @concurrent annotations were introduced.
Cmux Swift Package Boundaries	✅ Passed	PASS: The change is confined to app-target Sources/TabManager.swift and adds an ownership guard inside closeWorkspace, which is app/window lifecycle glue, not reusable domain logic.
Cmux Swiftpm Lockfiles	✅ Passed	Only Sources/TabManager.swift changed; no Package.resolved, Package.swift, .gitignore, workflow, or dependency diffs, so the SwiftPM lockfile rule isn’t triggered.
Cmux Swift Logging	✅ Passed	Diff only adds an ownership guard and comments; it introduces no print/debugPrint/dump/NSLog or ad hoc file/stdout logging, and existing Logger usage is unchanged.
Cmux User-Facing Error Privacy	✅ Passed	Diff only adds an internal guard comment in TabManager.swift; no user-facing error, alert, or error-body text was introduced.
Cmux Full Internationalization	✅ Passed	The PR only adds an internal guard and developer comments in TabManager.swift; no user-facing text or locale assets changed.
Cmux Swiftui State Layout	✅ Passed	Only an ownership guard was added in existing TabManager logic; no new SwiftUI state, geometry, list-row store refs, or render-time mutations were introduced.
Cmux Architecture Rethink	✅ Passed	The diff is a small ownership guard in closeWorkspace; it names tabs as source of truth and adds no timing, observers, or split lifecycle ownership.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	PR only adds an ownership guard in TabManager.closeWorkspace for existing workspace tabs; no NSWindow/NSPanel/WindowGroup or cmuxAuxiliaryWindowIdentifiers changes.
Cmux Source Artifacts	✅ Passed	Only Sources/TabManager.swift changed, and it’s hand-written source; the diff adds a guard/comment, not generated output or scratch artifacts.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	Diff only adds a production ownership guard in closeWorkspace; no new DEBUG/test-only seam, symbol, or visibility change appears.
Cmux No Ambient Global State	✅ Passed	HEAD adds only a guard inside existing TabManager.closeWorkspace; no new file-scope func/var, namespace, or singleton was introduced.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR restores a membership guard to closeWorkspace in TabManager.swift that was lost to a merge conflict. Without it, calling closeWorkspace with a workspace owned by a different window would teardown that workspace's Ghostty surfaces (SIGHUPing child processes), publish a spurious workspace-closed event, and nil out owningTabManager, while leaving the workspace visible on screen.

• Adds guard tabs.contains(where: { $0.id == workspace.id }) else { return } immediately after the existing count guard, short-circuiting all destructive work for any workspace this manager does not own.
• Intentionally changes one previously-reachable code path: a workspace resolved only via the global fallback (AppDelegate.shared?.tabManager) will now no longer be torn down through an unrelated manager — this is the correct behavior described in the PR.

Confidence Score: 5/5

Safe to merge. The change is a single early-return guard that prevents destructive teardown from running on a workspace this manager does not own.

The guard closes a narrow, well-documented re-regression: without it, closeWorkspace runs teardownAllPanels, teardownRemoteConnection, and a spurious close event against a workspace it has no business touching. The new guard uses the same UUID-identity comparison that the rest of the function already performs, is ordered correctly relative to the count guard, and has no interaction with the main-actor execution model that would introduce a TOCTOU window. The one deliberate behavior change (the global-fallback path now returns early) is correctly reasoned through in the PR description and is the intended outcome.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
Sources/TabManager.swift	Adds ownership guard to closeWorkspace preventing teardown of workspaces belonging to another tab manager. Guard placement, UUID comparison, and interaction with the three subsequent tabs.firstIndex calls are all correct assuming main-actor execution.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant TabManagerA as TabManager (Window A)
    participant WorkspaceB as Workspace (Window B)

    Note over Caller,WorkspaceB: Before this PR — foreign workspace destroyed

    Caller->>TabManagerA: closeWorkspace(workspaceB)
    TabManagerA->>TabManagerA: "guard tabs.count > 1 ✓"
    TabManagerA->>WorkspaceB: teardownAllPanels() 💀
    TabManagerA->>WorkspaceB: teardownRemoteConnection()
    TabManagerA->>WorkspaceB: "owningTabManager = nil"
    TabManagerA->>TabManagerA: publishCmuxWorkspaceClosed(workspaceB) 📣

    Note over Caller,WorkspaceB: After this PR — foreign workspace untouched

    Caller->>TabManagerA: closeWorkspace(workspaceB)
    TabManagerA->>TabManagerA: "guard tabs.count > 1 ✓"
    TabManagerA->>TabManagerA: "guard tabs.contains(id == workspaceB.id) ✗"
    TabManagerA-->>Caller: return (no-op)

Loading

_{Reviews (3): Last reviewed commit: "Close only the workspaces a tab manager ..." | Re-trigger Greptile}

[austinywang]

Fresh current-main verification passed: https://github.com/manaflow-ai/cmux/actions/runs/30895315961 checked out 36a2b6ee4da606bf3f4f3890d6d0fe4b22a576a4 and executed cmuxTests/TabManagerWorkspaceOwnershipTests (2 tests, 0 failures), including testCloseWorkspaceIgnoresWorkspaceNotOwnedByManager. The PR remains conflict-free and mergeable.