Close safety: ask before closing windows and real workspaces; no silent fallbacks

[BenevolentFutures]

Why

Follow-up to #456 and #458. Atin approved audit items 1 to 4 plus removing the socket close_others. Overlapping findings from surface:180's deeper audit are folded in (last-window session deletion, stale refs on destructive commands, close_others, Return-confirms defaults).

What

1. Windows ask before closing on every operator path: red close button, File > Close All, Cmd+Ctrl+W. A guard delegate in front of each main window's delegate answers windowShouldClose; c11's own closes (confirmed workspace close, last shell exiting, socket and AppleScript closes, quit) use closeMainWindowWithoutPrompt. The prompt is a sheet; Cancel is the default.
2. Closing the last window keeps the session snapshot. Before, it deleted the file, so nothing came back on relaunch. The last window's full session is written as it closes (conversations suspended), and a later quit with no windows counts as a clean shutdown. Limitation: opening a new window before quitting replaces the kept snapshot; snapshot history is surface:180's follow-up.
3. Workspace close asks unless it is a single idle terminal. Pinned, several surfaces, browser or markdown, or anything running: all ask. Cmd+W on a last tab and the cascade into a window close follow the same rule.
4. No reflex confirms. Destructive pane confirm cards and the legacy close alerts default to Cancel (standard agent pane.confirm questions keep their confirm default). Cmd+D (Split Right) no longer accepts any close; it still submits text-input cards.
5. Socket/CLI: workspace.action close_others removed. Destructive verbs (surface/workspace/window close, surface/tab/workspace actions, clear_history) reject an empty or unresolvable ref instead of falling back to the focused target. The CLI refuses an explicitly empty --surface/--workspace/... on those commands. Skill api.md updated.

Validation (tagged build, AppleScript/System Events + tagged CLI)

• Red button → sheet (Cancel, Close); Cancel keeps the window; clicking Close closes it. Quit afterwards with no windows → sentinel .clean; normal quit → .clean. File > Close All → sheet (before the guard delegate it closed silently).
• Close Workspace: lone idle terminal closes at once; 4-pane workspace and pinned idle workspace both stay pending on the prompt.
• Socket close-window closes with no prompt; afterwards the session file is still on disk with the window's layout.
• Stale surface:9999 / workspace:99 on close-surface, workspace-action, tab-action, clear-history → not_found, nothing closed. Empty --surface '' / --workspace '' → CLI error. workspace-action close-others → unknown action.
• Not run locally (no heavy work on the laptop): unit and UI suites, left to CI. Cmd+D and Return on live overlays not synthesized, to keep keystrokes off the production c11. Unit tests cover the Cancel default and the workspace rule.

Behavior notes (CHANGELOG material)

• An agent whose C11_WORKSPACE_ID points at a closed workspace now gets not_found from destructive commands instead of acting on the selected workspace.
• The ref check covers every workspace.action / tab.action / surface.action, including rename and pin, and treats a non-string ref as empty_ref.
• Closing a workspace now asks unless it is a single idle terminal, so Cmd+Shift+W prompts more often.

Fresh-context review: nothing blocking; its fixes are in the second commit.

🤖 Generated with Claude Code

[github-actions]

Deep review — #469 "Close safety: ask before closing windows and real workspaces; no silent fallbacks"

Thank you for this — it's a genuinely careful safety pass, and the write-up (per-path validation, the explicit "Behavior notes", the clean split between confirmed and pre-confirmed close paths) made it a pleasure to review. This is a re-review after your rebase onto #467: I re-traced every close path and the ref-rejection machinery against the trusted base, and the earlier round's two nits are both addressed (the windowWillClose must-stay-last comment is in, and the window/workspace asymmetry is confirmed intentional). No prompt-injection or anything suspicious in the diff or discussion.

Alignment

Squarely in scope. Socket/CLI targeting correctness plus operator-safety UX for the operator:agent pair — the same "no silent fallback stomps a peer's target" family #456/#458 established. No tenant-config writes, no display-link/draw-loop, no gratuitous upstream divergence, and the skills/c11/references/api.md update keeps the skill contract honest about the new strict-targeting rule. Good citizenship.

What I verified (holds up)

• v2RejectUnresolvedTargetRefs is correct and closes the right hole. For every present, non-empty ref it resolves through v2UUID (which parses raw UUIDs and handle refs) inside v2MainSync, then confirms liveness via the matching locator (tabManagerFor(windowId:), tabManagerFor(tabId:), v2LocatePane, locateSurface). A stale handle (surface:999) makes v2UUID return nil → not_found; an empty/non-string ref → empty_ref; an absent ref is skipped so the documented default still applies. This is exactly the misroute the handlers' own ?? focusedPanelId/?? focusedPanelId chains would otherwise allow (e.g. v2TabAction line 71). The pre-filter only ever adds rejections the handler would itself turn into not_found — it can't accept something the handler would misroute.
• No deadlock and no hang path. v2MainSync short-circuits when already on main, so its use inside v2SurfaceClearHistory's Task { @MainActor } is safe, and the switch-level uses run on the socket worker. Every socket close verb (window.close/workspace.close/surface.close) now routes through closeMainWindowWithoutPrompt / closeWorkspace, and confirmCloseMainWindow is a non-blocking beginSheetModal — no runModal() is reachable from a socket path. mainWindowsClosingWithoutPrompt is only mutated on main (AppleScript is main; socket goes through v2MainSync), so no race on that set.
• Role-gating on pane.confirm is right. initialSelection(for:) returns .confirm for .standard and .cancel for .destructive, and both present/advance route through it; the moveConfirmSelection/acceptSelectedConfirm defaults flip to .cancel defensively. makeConfirm is .destructive, so testReturnOnAFreshConfirmCancels and testStandardConfirmStartsOnItsConfirmButton lock both halves down. "No reflex confirms" lands on the destructive cards, not the generic socket primitive.
• No dead code / no compile break from the Cmd+D removal. acceptActiveWorkspaceCloseInteractionInKeyWorkspace() and its sole caller are deleted together; acceptActivePaneInteractionInKeyWorkspace gains includingDestructiveConfirms: with its one caller updated in lockstep; findButton(in:titled:) is removed along with its only use.
• close_others removal is surgical. Dropped from WorkspaceHandlers.supportedActions, its switch case, the CLI help, and the skill — while the separate tab.action/surface.action close_others (close-other-tabs-in-pane) in MiscHandlers is correctly retained.
• Last-window snapshot + clean-shutdown wiring is coherent. shouldRemoveSnapshotWhenNoWindowsRemainOnWindowUnregister now returns false; persistLastWindowSnapshot writes synchronously as the last context unregisters; didPersistLastWindowSnapshot is reset when a fresh context registers; and the new mainWindowContexts.isEmpty && didPersistLastWindowSnapshot branch in persistCleanShutdownSnapshot promotes to .clean without a redundant rewrite. The rebase guard (closesUnansweredResumePicker) correctly skips the save when the window closing is Session history; the resume picker never costs the previous session #467's unanswered resume picker, so its snapshot guarantee holds.
• Localization intact. Both dialog buttons stay String(localized:); the change only reorders them (Cancel first) and marks the close button .hasDestructiveAction. New socket/CLI error strings are diagnostic, not UI.

Findings

No blockers, no majors. Three small items:

1. (nit) Rejection message says "destructive commands" on now-non-destructive verbs. The ref check is intentionally applied to every tab.action/surface.action/workspace.action, including pin, mark_read, and rename (per your Behavior notes). But v2RejectUnresolvedTargetRefs's messages read "destructive commands need a concrete ref / never fall back to the focused target" (Sources/TerminalController.swift). For a stale-ref pin that wording is a touch misleading. Consider softening to "this command needs a concrete ref …" so the agent-facing error matches the broader scope you actually shipped.

2. (nit / heads-up) Switch-level reject calls don't v2RefreshKnownRefs() first, unlike the send/clear-history paths. surface.send and v2SurfaceClearHistory refresh the handle map before resolving; the other verbs call v2RejectUnresolvedTargetRefs at the dispatch switch without it. This is not a live bug — those handlers (v2WorkspaceClose, v2WindowClose, …) don't refresh either, so the reject filter resolves against the same map the handler uses and can't false-reject anything the handler would have accepted; and CLI callers pre-resolve handles to UUIDs client-side. Worth a one-line comment noting the intentional asymmetry (raw-UUID callers don't need the refresh) so a future reader doesn't "fix" it by adding a refresh or, worse, remove the refresh from send thinking it's redundant.

3. (nit, addressed-in-spirit) SwiftUI-delegate-swap fragility. The guard covers performClose routes only while window.delegate === closeGuard; if SwiftUI re-sets the delegate after installMainWindowCloseGuard, File > Close All / Cmd+Ctrl+W would bypass the prompt, leaving the close-button override as the sole backstop. Your validation shows all three paths prompt today, so registration is landing after SwiftUI's delegate is set — just flagging that the guarantee rests on that ordering. No change needed.

Process notes (not blocking)

• Sync the installed skill. Per the repo HARD RULE, editing skills/c11/references/api.md in source doesn't refresh an already-installed copy — whoever merges should run scripts/sync-installed-skills.sh c11 on their machine.
• Confirm CI is green before merge. The PR is unstable and leaves the unit/UI suites to CI, so the build gate is the real check on the new needsConfirmCloseWorkspace tests, the flipped SessionPersistenceTests assertion, and the reworked close-window / Cmd+D UI tests.

Recommendation

Merge after fixes — and "fixes" here is really just nit 1's wording and (optionally) nit 2's clarifying comment; nits 2 and 3 are both safe as-is. The core close-safety machinery is sound, the strict-targeting rejection is complete and consistent across all the destructive verbs plus the folded-in action verbs, the last-window snapshot path is coherent with #467, and the behavioral tests are the right kind. Land it once CI is green and the skill copy is synced.

---

🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.

---

🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.

[BenevolentFutures]

Rebased onto #467 (session history). Resolved two conflicts in AppDelegate: both clean-shutdown branches kept (quit with the resume picker open; quit with no windows), and the last-window save is skipped when the window closing is the unanswered resume picker's, so #467's guarantee holds. Drawbridge nit 1 addressed with a must-stay-last comment; nit 2 is intentional (a lone idle workspace closes without asking, and its window cascade follows). Re-validated on a tagged build: Close All prompts, stale refs rejected, close-others gone, Close on the window sheet closes, zero-window quit leaves the sentinel clean.