Skip to content

port #29984 to v2.2.12-opt: Cmd-Q with local terminals asks how to quit (QUIT P0) - #30182

Merged
lawrencecchen merged 7 commits into
cp/dogfood-v2-2-12-optfrom
fix/p0-quit-reopen-dogfood
Nov 2, 2026
Merged

lawrencecchen merged 7 commits into
cp/dogfood-v2-2-12-optfrom
fix/p0-quit-reopen-dogfood

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

What

Port of #29984 (QUIT P0) to the dogfood line cp/dogfood-v2-2-12-opt: plain cherry-picks (-x) of its commits, no other change.

After these, the quit files are byte-identical to feat 88dcb89, except: feat's CmuxNextDaemonService/QuitRules.swift (six types) is split here one file per type with the same code, because this line's gate refuses a new multi-type file. scripts/cmux-next/quit-launch-bound.py does not exist on this line, so its hunks are dropped. readiness-soak.py keeps this line's own quit retry body and gets only feat's endEverything flag.

Evidence

  • Journey quit-reopen-restore (kit cp/journey-quit-reopen-restore@a1f077f5641, independent review SHIP; a REAL Cmd-Q by CuaDriver, whole-host lease) on the bench build 504c12edfe12f668361ba19e of 467695a (the first two commits): PASS 5/5 (cmux19 x4, cmux23). On the base dogfood build 9276e456 (be22bd3): RED 3/3 (no dialog).
  • Journey quit-reopen-handoff (prior build 4b3bd8a2, a real version handoff at the reopen): PASS 3/3 on 504c12ed.
  • Speed A/B (kit 7a10c35, daemon builds measured per cycle) base 9276e456 vs 504c12ed: no change in reopen speed (STATUS 3097); first saved frame p95 176 / 201 ms (1 / 20 workspaces), interactive p95 <= 401 / 459 ms.
  • Compile of the earlier head aed37b9: 6d218e67a29afbd6910fd0fe passed; suites 57ef5b811f7f4b2a53ff62bf there: QuitChoiceTests 6/6, CmuxDialogButtonFitTests 3/3.
  • Compile of this head 1df98d8: cfbc740aa8b48a8f435fc21a passed (CmuxNext, app, cmux-tui). Suites QuitChoiceTests + CmuxDialogButtonFitTests + QuitEndEverythingGuardTests: step 6f86569664a29a0d8e7efad3 passed on aws-m4pro-1 with CMUX_TEST_REQUIRE_GUI=1: QuitChoiceTests 6/6, CmuxDialogButtonFitTests 3/3, QuitEndEverythingGuardTests 5/5.
  • The guard's red commit c0e623a does not compile (its test uses confirmsEndEverything, which the fix adds), the same as on feat: it is not a behavioral red. The guard's behavior is shown by QuitEndEverythingGuardTests passing at the head, plus feat's own red/green for b52ff17.

Risk

risk:normal: Swift UI logic, copy, tests; no new delete (End Everything's delete is existing code, and the guard makes it harder to reach). Dogfood line, never merged into feat. Independent review: SHIP at 1df98d8 (#30182 (comment)).

Changelog

  • Changed: Cmd-Q with running terminals asks whether to keep them running, end them and keep the layout, or end everything.

Observed / Not run

  • Observed: the journey rounds and the speed A/B above on 504c12ed; the prepush gate on this head.
  • Not run on this exact head: a journey round with the dialog-fit and guard commits (the journeys ran on 467695a), and a screenshot of the dialog. No capture: the journey's check frames show the dialog on 504c12ed; a capture of the stacked layout follows.

…eep Sessions the default (QUIT P0, red)

(cherry picked from commit 8316ec3)
… (default), End Sessions Keep Layout, End Everything, Cancel; Don't ask again writes app.quitBehavior (QUIT P0)

Lawrence 2026-11-01: "cmd q needs to prompt for what type of quit". Takes back
aba72a3 (cx-7vu, 2026-10-11, Lawrence 2026-10-07 "no confirm dialogs"),
which made QuitPolicy.decide quit with Keep and no question unless incognito
programs ran, so app.quitBehavior "ask" (the default) never asked.

(cherry picked from commit 6bfec69)
…stack inside it, the default on top (QUIT P0 review, red)

(cherry picked from commit 325b2a3)
…n top, each the card's width (the quit choice's four buttons); the quit scripts answer the quit dialog's Keep (QUIT P0 review)

(cherry picked from commit 1ac555c)
… the second-quit check waits for the dialog; stacked dialog buttons tab top to bottom (review FIX on #29984)

(cherry picked from commit 132fa6e)
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 1, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-review-subagent-30182): FIX at aed37b9
reviewer-session: 2486d921-c9de-4d7e-9220-1532ab863a0b
author-session: a8c03bd88d9f5386c
R9: pass
perf claim: none

Scope: git diff $(git merge-base origin/cp/dogfood-v2-2-12-opt HEAD) HEAD (merge base 535b6a3, 5 commits, 12 files). I read the code only. I did not build or run tests. The two-dot diff against the base tip also shows base-only files (ghostty-next, LayoutRootView+Scroll, etc.). The cause is that the base moved after the branch point (a516f14). git merge-tree merges cleanly, and no base change touches a file of this PR.

Blocking

B1. Without b52ff17, "Don't ask again" + End Everything makes every later Cmd-Q delete all workspaces with no question

This PR is the first change on this line that gives the quit dialog an End Everything button next to a "Don't ask again" check box:

  • QuitAlertContent.main (session choice): buttons [.keep, .endKeepLayout, .endEverything, .cancel], showsSuppression: true.
  • QuitAlert.answered: .endEverything -> finish(.quit(.endEverything, remember: remembers)).
  • QuitCompletion.run: remember -> QuitPolicy.remembered(.endEverything) = .endEverything, which is written to app.quitBehavior.
  • QuitPolicy.decide on this line: remembered = .endEverything, so offersSessionChoice = false. With no incognito programs, the guard returns .quit(.endEverything) for an interactive quit. A scripted quit with no flag returns .quit(remembered ?? .keep), which is also End Everything.
  • DaemonService.endSessionsAndStop -> endSessionsAndStop(deletingWorkspaces: choice == .endEverything). Nothing between these steps confirms.

So one checked box makes every later accidental Cmd-Q (and every cmux app quit with no flag) delete every local workspace with no dialog. This is the exact incident that b52ff17 fixed on feat (2026-10-31 18:46, 900 workspaces, 796 agent chats and 490 terminals closed). The #29984 review closed this point on feat only because b52 was merged in (review of b3c6d56: "Round-1 N2 is now closed ... feat's guard then asks"). This PR does not port b52, so on the dogfood line the hazard comes back, and a one-click path is added. Before this PR, this line could persist End Everything only through Settings, because the incognito dialog had no End Everything button. QuitChoiceTests.aRememberedChoiceQuitsWithNoQuestion (the 8316ec3 version) also asserts .endEverything -> .quit(.endEverything) as the intended behavior. feat dropped that case.

I checked whether b52 applies here: git show b52ff179c27 | git apply --check fails on QuitAlert, QuitAlertContent, QuitPolicy and readiness-soak.py. It depends on the QuitRules move. Fix options:

  • Port the guard in a form that fits this line's QuitPolicy. An interactive quit with behavior == .endEverything asks (a confirmation with Return = Keep, Escape = Cancel, End Everything with no key, no suppression box). With alreadyAsked it falls back to .endKeepLayout. A scripted quit with no flag maps End Everything to End Sessions, Keep Layout. Update QuitChoiceTests to match, as b3c6d56 did, and add a red/green test for this case.
  • The smallest stopgap: never remember End Everything from the dialog (remember: false for .endEverything in QuitAlert.answered, or hide the check box effect for that button). Also make QuitPolicy.decide ask for an interactive quit with a remembered End Everything. The Settings path is otherwise open.

Checked and correct

  • Faithful port. For 2834021, 467695a and ea70cfb, every +/- line is identical to 8316ec3, 6bfec69 and 325b2a3. 9430407 and aed37b9 differ from 1ac555c and 132fa6e only by the dropped scripts/cmux-next/quit-launch-bound.py hunks. All 5 carry (cherry picked from commit ...). At HEAD, all 11 PR files other than the plan are byte-identical to 132fa6e's tree. quit-persistence.md differs only in base content.
  • Dropped items verified. scripts/cmux-next/quit-launch-bound.py is absent on the base and at HEAD. b52ff17 and b3c6d56 are not ancestors of HEAD.
  • The quit rule here, apart from B1.
    • Ask + local terminals -> asks (offersSessionChoice, default Keep).
    • Remembered keep / end-keep-layout -> quits at once.
    • scripted -> remembered ?? keep. signal and powerOff -> keep. explicit -> as given.
    • alreadyAsked (unsaved question) -> no second question.
    • Incognito with a remembered choice -> Quit/Cancel confirmation, no suppression box. Incognito with Ask and terminals -> folds into the session choice (line "incognito closes").
    • Incognito-only (0 terminals) with Ask -> Quit/Cancel. .quit -> prompt.defaultChoice = keep.
  • APIs on this line. QuitPrompt has offersSessionChoice and defaultChoice. QuitFacts.none and QuitPolicy.remembered exist. CmuxDialogView.buttonViews and focusables are private(set) (readable through @testable). CmuxDialogButtonView.button and intrinsicContentSize (title + key hint, set in init through refresh()) exist. Metrics.space2 and CmuxDialogMetrics are present. This line's update(_:) rebuilds through buildContents, so a spec update also goes through buttonRow. This line's focusInitial focuses the dialog itself, not a button, so a stacked row does not change it.
  • Compile. 6d218e67a29afbd6910fd0fe at aed37b9: CmuxNext ok (the log shows CmuxNextAppTests and CmuxNextDesignTests compiling), app ok, cmux-tui ok.
  • Callers on this line.
    • debug.quit {press: "quit"} maps to keep when there is no Quit button (file-pages-e2e, file-pages-r96-e2e).
    • press: <prompt.default> = "keep" (journeys/driver.py, restore-journey.py, quit-hidden-chromium-live.py).
    • press: "keep" (dialogs-undo-live, fleet-quit-persistence, quit-leaves-no-helpers).
    • press: "end-everything" is now a real button (picker-e2e, render-scenes cleanup), with the same intent.
    • The old ids confirm-quit-everything and quit-everything map to end-keep-layout.
    • The second-Cmd-Q variant waits for asking, then requestQuit(.interactive) -> answerDefault() -> keep.
    • The update relaunch records .explicit(.keep), so it never asks.
  • Localization. quit.button.endKeepLayout has all 21 locales in this line's Quit.xcstrings (the same file as feat). The removed quit.button.quitEverything is not used anywhere.
  • Fleet evidence matches the files.
    • 504c12edfe12f668361ba19e.jsonl: quit-reopen-restore passed 5/5 (cmux19 x4 a1-a4, cmux23 a5). quit-reopen-handoff passed 3/3. All on commit 467695a with kit a1f077f.
    • 9276e456da1ae1b93bfabd86.jsonl (be22bd3, one commit behind the merge base, with no quit or dialog change between them): the current-kit rounds fail at step 23 quit_asked ... never held (restore 3/3, handoff 2/2).

Non-blocking

  • N1. scripts/cmux-next/chromium-host-bench.py ae_quit step (this line only). feat removed this step. Here it sends the Quit Apple event, which is interactive in QuitOriginTracker, and waits 20 s for exit. If the bench app has a local terminal (the bench types into one), the session dialog now shows. Then cmux_quit is false, the step SIGKILLs the app and ae_quit.pass is false. Answer debug.quit with the default, as quit-hidden-chromium-live.py does.
  • N2. Evidence gap. The journey rounds cover 467695a only. The stacked-button layout (9430407, aed37b9) has no journey round and no capture on this line. This is a native UI change, and the PR body has neither a capture nor a No capture: line. QuitChoiceTests and CmuxDialogButtonFitTests were compiled here but not run on this line (they passed on feat's a9e83b6).
  • N3. The branch is behind its base tip by several merges. They merge cleanly with no file overlap, but the fleet compile is of the branch, not of the merged tree.

…md-Q (red)

2026-10-31 18:46 a Cmd-Q typed mid-sentence, with app.quitBehavior
end-everything set, closed all 900 local workspaces with no dialog.

(cherry picked from commit a0b0c7e)
…y workspace (port of feat b52ff17 to v2.2.12-opt)

On 2026-10-31 18:46 a Cmd-Q typed mid-sentence, with app.quitBehavior
end-everything set, quit with no dialog and closed all 900 local
workspaces. With the quit question of #29984 a person can store End
Everything from the dialog, so this line needs the guard too (review FIX on
#30182).

- An interactive quit with End Everything remembered asks "Quit and close
  every workspace?" (Return keeps sessions running, Escape cancels, End
  Everything has no key). After the unsaved-changes question it ends the
  sessions but keeps the workspaces.
- A scripted quit with no flag maps End Everything to End Sessions, Keep
  Layout. --end-everything and the File menu item run as before.
- The files are feat's at 88dcb89 (b52ff17 + #29984): QuitPolicy,
  QuitAlert, QuitAlertContent, QuitStrings, Quit.xcstrings, QuitChoice,
  readiness-soak.py, update-e2e.py, QuitChoiceTests, QuitEndEverythingGuardTests.
- Port difference: feat's CmuxNextDaemonService/QuitRules.swift holds six
  types; here they are one file per type (QuitProgram, QuitAgentFacts,
  QuitFacts, QuitPrompt, QuitDecision, QuitRules), byte-identical code, so
  no new multi-type file.

(cherry picked from commit b52ff17, files split)
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-review-subagent-30182): SHIP at 1df98d8
reviewer-session: 2486d921-c9de-4d7e-9220-1532ab863a0b
author-session: a8c03bd88d9f5386c
R9: pass
perf claim: none

Scope: the full PR over merge base 535b6a3 (7 commits). This round I looked closely at c0e623a (red) and 1df98d8 (the guard port), which were added on top of aed37b9. I read the code only. I did not build or run tests.

Blocking

None.

Round-1 B1 is fixed: a remembered End Everything can no longer delete every workspace with no question

  • Interactive quit. With app.quitBehavior = end-everything, QuitRules.decide returns .ask with confirmsEndEverything: true and defaultChoice: .keep. This happens before the session-choice rule and for any number of terminals. QuitAlertContent.main shows "Quit and close every workspace?" with buttons [.keep, .cancel, .endEverything] and no suppression box. Return presses Keep (.default) and Escape presses Cancel. End Everything is .destructive and has no key. Because showsSuppression is false, remembers is false for every answer. A second Cmd-Q while the dialog shows calls answerDefault(), which presses Keep. SIGTERM answers Keep.
  • After the unsaved-changes question, the quit gives .endKeepLayout (one dialog per quit). It never gives .endEverything.
  • Scripted quit with no flag: unattended(.endEverything) gives .endKeepLayout.
  • Explicit --end-everything, the File menu item and debug.quit fixture_quit end-everything still give .endEverything. Signal and power off give Keep.
  • The path from the dialog is closed. "Don't ask again" + End Everything in the session choice still writes end-everything, and the next Cmd-Q then asks with the guard dialog.
  • The rest of the rule is unchanged from round 1. Ask with local terminals asks (Keep is the default). Remembered Keep or End Sessions, Keep Layout quits at once. The unsaved-changes question stays the only question. The incognito rules are as before.

The port is faithful to feat 88dcb89

  • These files are byte-identical to feat 88dcb89: QuitPolicy, QuitAlert, QuitAlertContent, QuitStrings, Quit.xcstrings, QuitChoice, QuitChoiceTests, QuitEndEverythingGuardTests, update-e2e.py, CmuxDialogButtonFitTests, dialogs-undo-live.py and fleet-quit-persistence.sh.
  • c0e623a has the same patch as a0b0c7e.
  • The split of QuitRules.swift. I concatenated QuitProgram, QuitAgentFacts, QuitFacts, QuitPrompt, QuitDecision and QuitRules and compared the result with feat's QuitRules.swift. Only the doc comments differ: the file-header comment moved onto QuitRules, and QuitDecision got a one-line doc comment. The code is identical. All public inits and QuitFacts.none / QuitAgentFacts.zero exist.
  • The removed app types. The app's QuitFacts.swift and QuitPrompt.swift are deleted. QuitPolicy.busiest and busiestLimit are referenced nowhere. QuitFactsReader, QuitAgents and QuitCoordinator use the public CmuxNextDaemonService types. The CmuxNextApp module already used CmuxNextDaemonService types such as QuitSessionsChoice and QuitOrigin. Both test files import CmuxNextDaemonService.
  • readiness-soak.py. Only the quit call changed: it adds "args": {"endEverything": True}. The line's own retry body is kept. scripts/tests/test_readiness_soak.py reads only params["action"], so it is unaffected. update-e2e.py's final quit passes the flag.
  • Localization. quit.endEverything.title, quit.endEverything.line and quit.button.endEverything each have 21 locales. quit.button.endKeepLayout has 21 too, as in round 1.
  • Compile. cfbc740aa8b48a8f435fc21a at 1df98d8: CmuxNext ok (943 s, 0 error: lines), app ok, cmux-tui ok. The log shows QuitChoiceTests.swift and QuitEndEverythingGuardTests.swift compiling.
  • All points that passed in round 1 still hold: the five cherry-picks, the dropped quit-launch-bound.py, the callers of the button ids and debug.quit, and the stacked-button layout.

Non-blocking

  • N1 (chromium-host-bench.py step 3, ae_quit): still non-blocking.
    • On this line, the Quit Apple event is interactive. If the bench's cmux has a local terminal, the session dialog now shows. cmux then does not quit in 20 s, and ae_quit.pass is false.
    • The step expects this case: "When cmux does not quit (a confirmation, ...), SIGKILL to cmux checks the EOF end instead". So the host-EOF check (cmux_killed) still runs.
    • The bench is for the helper prototype (default off, Debug only), and feat removed this step.
    • Fix this when someone runs the bench on this line: answer debug.quit with prompt.default after the event, as quit-hidden-chromium-live.py does.
  • N2. The PR body is out of date. "Not ported: ... the End Everything guard (b52ff17), so the session rule stays in QuitPolicy.swift" is no longer true. Add c0e623a <- a0b0c7e and 1df98d8 <- b52ff17 (split into one file per type). Also add the guard to the changelog line and to Observed / Not run.
  • N3. The red at c0e623a will not compile, the same as feat's a0b0c7e. It uses confirmsEndEverything and QuitStrings.endEverythingTitle, which do not exist before 1df98d8. A compile red does not prove behavior. The behavior is shown on the base by reading the code: round 1 traced the .quit(.endEverything) path, and the base QuitChoiceTests asserted it. Say this in the PR, and do not report it as a behavioral red.
  • N4. Tests not run yet. Suite step 6f86569664a29a0d8e7efad3 (QuitChoiceTests, CmuxDialogButtonFitTests, QuitEndEverythingGuardTests at 1df98d8) was still queued when I checked. The journey rounds cover 467695a only. No capture yet (from round 1).
  • N5. The branch is still behind its base tip. The merge is clean and no files overlap.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Checks at 1df98d8:

  • plan (changed Cargo workspaces) and its wrapper rust workspaces: base finding. The error is "cmux-tui/tools/async-blocking-scan is a Cargo workspace with no entry in .github/rust-workspaces.toml". The base cp/dogfood-v2-2-12-opt has that directory and no entry; this PR touches no Cargo or workflow file. The same check is red on P0 port: keys typed after Cmd-N then Ctrl-` reach the new terminal (#30827, #29530 keys part) #30271 on this base.
  • Fleet evidence for this head: compile cfbc740aa8b48a8f435fc21a passed; suites 6f86569664a29a0d8e7efad3 passed (QuitChoiceTests 6/6, CmuxDialogButtonFitTests 3/3, QuitEndEverythingGuardTests 5/5, CMUX_TEST_REQUIRE_GUI=1).
  • Pending checks (cmux-tui tree preflight, ios-tests, GhosttyKit release check) do not hold the PR.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 1, 2026
@lawrencecchen
lawrencecchen merged commit 42a5c2f into cp/dogfood-v2-2-12-opt Nov 2, 2026
94 of 100 checks passed
@lawrencecchen
lawrencecchen deleted the fix/p0-quit-reopen-dogfood branch November 2, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant