Skip to content

Part of #25852: guardrails --sites, the whole-tree main-blocking count - #25862

Open
austinywang wants to merge 5 commits into
feat-cmux-nextfrom
25852-threading
Open

austinywang wants to merge 5 commits into
feat-cmux-nextfrom
25852-threading

Conversation

@austinywang

@austinywang austinywang commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

scripts/cmux-next/guardrails.py --sites RULE prints every site of a guardrail rule in HEAD's whole tree (base debt included) and their count. It is diagnostic only and does not change push enforcement. --sites main-blocking gives the main-thread blocking debt left to pay down under #25852: 75 sites at feat-cmux-next 01facc1b131. threading-model.md records the command.

Scope change (base already landed the Swift part)

This PR first moved the agent panes' draft and scroll-place saves off the main thread through a StateFileWriter, and drained them at quit. feat-cmux-next has since landed the same goal through another design:

The merge at ca0091ab7c resolves the 12 conflicted files to the base. StateFileWriter, AgentPanePersistenceQuit, their tests, the App wiring and the app_lines +8 allowance are removed. That also resolves both blocking findings of the review at 6aaafd0a3a2 (#25862 (comment)):

  • The seal no longer refuses the quit's own late draft, because the base never seals.
  • No unbounded drain sits before CefShutdown, because the base's wait is bounded by the quit deadline.

Review note 5 is fixed: tree_sites diffs against git's built-in empty tree and writes no tree or commit object.

Verification

Head: 33c9938a8516bdc85895c60f675174c76ec8eae1. Base: 01facc1b131. The base was merged normally and the push was a plain fast-forward.

Observed / Not run

Observed:

  • python3 tests/test_cmux_next_guardrails_blocking.py: 9 tests OK, including test_sites_counts_the_whole_tree_base_debt_included and the red-first test_sites_fails_closed_when_the_rule_does_not_run (red at 377b591254: SystemExit not raised).
  • python3 scripts/cmux-next/guardrails.py --sites main-blocking --head origin/feat-cmux-next: 75 at 01facc1b131.
  • python3 scripts/cmux-next/prepush-gate.py: no new violations, and no architecture count rises.
  • git diff --check: clean.

Not run: no Swift build or test, because this head changes no Swift. The PR diff against the base is guardrails.py, its test and one doc line.

No capture: no visible change.

Changelog

none

🤖 Generated with Claude Code

@austinywang austinywang added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 27, 2026
@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 51c3b5d: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr25862 -f ref=25852-threading -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); base red: the same failure on the base 01facc1; does not block

Default changes

No defaults change at 51c3b5d2cdc4.

@github-actions github-actions Bot added risk:high Risk tier (docs/ci/risk-tiers.md) and removed risk:normal Risk tier (docs/ci/risk-tiers.md) labels Oct 27, 2026
@austinywang

Copy link
Copy Markdown
Contributor Author

Review 1 (correctness subagent) at fd82abe and how it was resolved at a561674:

  1. Blocking, fixed. StateFileWriterTests.theQuitsShutdownKeepsTheLastSave expected a save made after BackgroundWork.shutdown() to be refused. LaneScheduler.submit still accepts blockShutdown items after close(), so that save is written and the test would always fail. The test now asserts the save is written. The dropped: callback on BackgroundWork.post could never fire for this writer, so it was removed (BackgroundWork.swift now matches the base).
  2. Medium, fixed. The quit awaited the lanes before it armed the engine-stop deadline. A stalled blockShutdown item, such as the update rollback copy, could therefore hold the quit with no bound. The quit now awaits the lanes after QuitDeadline.engineStopLimit is armed.
  3. Low, fixed. The second quit signal's ForcedQuitExit flushed only Home's cache. It now drains the lanes alongside that flush, under the same 160 ms bound.
  4. Low, fixed. StateFileWriter re-drained a late save by calling itself recursively. It now uses a loop.
  5. Low, kept. guardrails.py --sites writes an empty-tree object and a dangling commit into the repo's object store. These are unreferenced and git gc removes them. It is a measurement tool and is not on the push path.

The review checked these and found them clean: the writer's state machine (no lost wakeup, no reordering), that no quit code uses services.work after the shutdown, Swift 6 isolation of the closures and of Entry, and the determinism of the tests.

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Bead cx-ms417 created for this PR by the cmux coordinator.

@blacksmith-sh

blacksmith-sh Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Found 6 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmp12qn2cqs/summary-fail.json (live):/summary-fail.json (live): View Logs
/tmp/tmple_at0xu/summary-missing.json (live):/summary-missing.json (live): View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows
View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/
[tabs] debug.tab_switch timed 0 of 40 switches
View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@austinywang

Copy link
Copy Markdown
Contributor Author

Review 2 (correctness subagent) at 140f6b5: SHIP. It verified all 4 review 1 fixes:

  • the post-shutdown test now matches LaneScheduler
  • the quit arms its deadline before it awaits the lanes
  • the forced exit drains the lanes inside its 160 ms bound
  • the drain is a loop, not a recursive call

No new correctness issue. Nits only: a hung .files write is bounded only by the quit deadline, by design. Fleet: red at 3afa9e4 (step 8e3dc3a3), all 7 suites green at the head (step b041598b), build job 148536fa published.

@austinywang austinywang 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 Oct 27, 2026
@austinywang

Copy link
Copy Markdown
Contributor Author

Merged origin/feat-cmux-next (93d1c8c) into the branch, no conflicts; pushed without force. New head 86a15ad. The PR's own diff is line-identical to the reviewed 140f6b5.

  • Fleet: step a83694c3a3eb0d34e17f6621 (aws-m4pro-3, Xcode 26.6) — StateFileWriterTests, UnsentDraftTests, ChatScrollRestoreTests, ChatScrollRestoreDefaultPathTests, RestoredNewTabPageTitleTests, BackgroundWorkTests, ForcedQuitExitTests all pass.
  • Review 3 (subagent, base interaction): SHIP at 86a15ad. The base's quit changes (cloud.prepareForTermination before QuitCompletion, onQuitRefused) leave the lane drain in stopBrowserEngines under the engine-stop deadline; no new callers of the drafts or scroll-anchor inits; no new blockShutdown posts.

@austinywang austinywang added ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds and removed ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds labels Oct 27, 2026
@austinywang austinywang removed the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Oct 29, 2026
@austinywang

austinywang commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor Author

Review (Independent correctness and impact reviewers): SHIP at 6384d68

Two fresh independent correctness-first reviews of the full diff against e6830bd1aee702961f4d41791b2341e0120e6fa3 found no verified defects. They traced state-writer ordering and flush races, state ownership, both quit seal/drain paths, late draft and both scroll-page refusals, consumers and tests. Merge integration retains the base upload-staging fix and both independently reviewed architecture allowances (+6 cloud automation and +8 quit composition). The reviewers assessed the changed scroll provider path and existing PageError.closed refusal.

The same focused fleet command executed 37 tests on both the before-fix commit 21bd10296be51a969bb689383581efd15190bfa1 and repaired pre-merge head bee085be7810ee5e2157b46ebcf2ef1a740ef50f: the two new late-draft/late-scroll regressions failed before the fix, and all 37 passed after it. The persistence/quit implementation and those tests are unchanged in this merge. Exact-current-head fleet app compilation passed (Debug): overflow run 41177371943, job 5c2af82b5fc1e176ef04884d. The compile, receipt upload and workflow all passed. Exact-current-head focused suites passed all 37 tests across 8 suites (step 72b45be3a16d7014c1339644), including both new late-save regressions. The required Release recipe-2 compile also passed (job 83b44e9daabecbecae038f84, 120 seconds). Current-head pre-push, Swift syntax and diff checks passed.

Source review only: no runtime execution by the reviewer. GUI quit/relaunch, slow-disk deadline expiry, sanitizer and macOS 14/15 behavior remain unverified. The existing bounded quit deadlines may expire behind slow work on the shared file lane.

Both reviewers independently confirmed that the +8 App allowance covers only imports, injection and quit composition, while the base cloud-automation +6 allowance remains separate.

@austinywang

Copy link
Copy Markdown
Contributor Author

Current-head check classification for 6384d680b20f61d4eec2796df9ed78924ac28fa5:

  • cmux-next crash-class rules: this head's job repeats the three missing NSTextView drag selector probes from feat-cmux-next at c61f351aa09ef165c752a19ed344433f2d180a55. This PR changes neither the omnibar source nor the checker/table relative to its merged base.
  • nightly-guards / workflow-guard-tests / release-notary: this head's job and merged-base e6830bd1aee702961f4d41791b2341e0120e6fa3 fail the same two RunsOnFallbackTests assertions and the same SigningHooksInStepWithGlaedaTests assertion for hook 49b5d9f59f582bed21bdc3014d35b187ab2ccc19f01d4eb53ba57dd5449fa3ef.
  • cmux-next app compile (build fleet): the wrapper timed out contacting the controller (exit 69), before a compile verdict. The accepted request is fleet job 5c2af82b5fc1e176ef04884d; no duplicate was submitted. GitHub refused a job-only rerun while the workflow is active. The existing overflow run is 41177371943.

The defaults guard passed after the fresh exact-head review receipt. Current-head focused suites are running in existing fleet job 72b45be3a16d7014c1339644.

@austinywang austinywang added ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds and removed ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds labels Oct 31, 2026
@austinywang

austinywang commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor Author

Review (Independent correctness and impact reviewers): SHIP at c034f6b

Two fresh independent reviews of the full diff against de23a474c46411b07f6b680163020b056388c8ff found no verified defects. They checked writer synchronization and coalescing, persistence ordering, model and page consumers, late-save refusal, normal and forced quit, CEF shutdown, regression tests and base integration. Both independently confirmed the +8 App composition allowance covers wiring, imports and comments, with persistence behavior in its domain packages; the separate base +6 allowance is retained.

This head normally merges the latest fetched base. It preserves the PR's persistence repair and includes base fixes for the previously failing daemon tests and automatic compile-receipt recovery. The remote head was verified before the plain push. Pre-push gate, push hook and git diff --check passed.

Source review only: the reviewers did not execute builds or tests. Current-head fleet Release recipe-2 app compile passed (job 83d7dd2c941dde06d5a16172, 150 seconds). Current-head focused fleet suites passed all 42 tests across ten suites (step fd06a462e36ebbac1301e054), including the late-save regressions and both previously failing daemon suites. The automatic gate also passed app compilation and the bundled Rust check; official cmux-next/compile is green. GUI quit/relaunch, slow-disk deadline expiry, sanitizer and macOS 14/15 behavior remain unverified. Existing bounded quit deadlines may expire behind slow work on the shared file lane.

@austinywang

Copy link
Copy Markdown
Contributor Author

Check classification at c034f6b28f0bc76e4ffd5e340b287d89139b0055:

  • The remaining nightly-guards / workflow-guard-tests / release-notary red is inherited. This head's job and base 7ad4616221fad7c703fa33e79c34919affcfc81d fail the same three tests: both RunsOnFallbackTests signing-label checks and SigningHooksInStepWithGlaedaTests.test_every_listed_hook_is_one_glaeda_released_with_the_check (missing hook 49b5d9f59f582bed21bdc3014d35b187ab2ccc19f01d4eb53ba57dd5449fa3ef). This PR changes none of those workflow, test or hook files relative to its merged base. Guard status is the wrapper.
  • The initial defaults-guard failure requested a fresh exact-head SHIP receipt. Both independent reviews passed; their receipt is now posted and the same-head guard rerun passed.
  • The exact-head manual fleet Release recipe-2 app compile passed, job 83d7dd2c941dde06d5a16172 (150 seconds). The controller's automatic compile receipt was missing before its recovery update deployed; it now exists for this exact head and is running as job e2756869df7e220d20c26b17. The official status publisher remains in progress. Focused fleet suites are queued as fd06a462e36ebbac1301e054; no pass is claimed for queued work.

No PR-caused source failure has been found. Current-head live quit/relaunch, sanitizer and macOS 14/15 behavior remain unverified, as disclosed in the description.

@cursor

cursor Bot commented Oct 31, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (Independent correctness and impact subagents; correctness first): SHIP at bb42033

Two fresh read-only reviews examined the full diff against base 1fd321e09671c45666ca2f53094f6a04cfdb1fdb, persistence callers, writer/quit interleavings, API integration, and base policy. No verified production defects remain. Both independently approve the +8 App composition allowance; the base's removal of the obsolete Cloud +6 allowance remains intact.

The impact review found a test-only race between separate suites that replace the shared scroll store across awaits. The current head nests the default-host suite under the existing serialized page-host suite; both reviews verified the shared serialization boundary. Standard substring filters remain valid; strict named-suite selection uses ChatScrollRestoreTests.ChatScrollRestoreDefaultPathTests.

Current-head automatic fleet app compile passed (job 626f70cac3a2ccf729349006, 96 seconds), and cmux-next/compile is green. Static pre-push checks and git diff --check passed. Focused fleet step aba02a093650c4ace44dfa62 is pending and runs both scroll suites together through ChatScrollRestore; ready-to-land remains off until that execution is verified.

Source review does not establish live quit/relaunch, disk-contention deadlines, sanitizers, macOS 14/15 behavior, or performance measurements; these were not run.

@austinywang

Copy link
Copy Markdown
Contributor Author

Check classification at bb4203379a4b6d4ffe57cb928cef5fb5c7350001:

  • defaults guard: rerun passed after the fresh exact-head review receipt.
  • nightly-guards / workflow-guard-tests / release-notary: inherited. The PR job log has the same three failures as the base job at 0d380b87c126ad2350c9c9b1c9b02297cfb6b5ad: both RunsOnFallbackTests signing-label tests and SigningHooksInStepWithGlaedaTests.test_every_listed_hook_is_one_glaeda_released_with_the_check (missing hook 49b5d9f59f582bed21bdc3014d35b187ab2ccc19f01d4eb53ba57dd5449fa3ef). This PR changes none of the workflows, signing-hook lists or tests involved.
  • nightly-guards / Guard status: wrapper for the inherited release-notary failure above.

The app compile and static checks are green. Fleet reproduction confirmed the reviewer-found fixture race on the old head: nine combined scroll tests ran, and the final-drain test failed both late-save refusal assertions (step 526889971814de64ebdb328d). The repaired-head run is in progress (step aba02a093650c4ace44dfa62).

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (Independent correctness and impact subagents; correctness first): SHIP at a8c14e7

Two fresh independent reviews examined the full diff against base 24b2581cb9ddd399c3e40397f8ffee1b0f54fe9c, the new Cloud first-send integration, quit admission and writer interleavings, API consumers, guardrail exceptions and architecture accounting. No verified defects remain. The base callback/handoff and dated guardrail-exception behavior remain intact.

Both independently approve the unchanged +8 App composition allowance. All base record fields and exceptions, including +485, are preserved. The +8 entry now sits between immutable applied entries; the consumer compares entries as multisets and sums allowances, so position has no semantic effect and future end insertions avoid the repeated conflict.

Current-head app compile passed (fleet job 44f1920f3169f937ed4add40, 68 seconds); official cmux-next/compile is green. Static gate and git diff --check passed. Previous head bb4203379a4b6d4ffe57cb928cef5fb5c7350001 executed and passed all 42 focused tests, including nine scroll tests together and all five nested default-host cases. Current-head fleet step 86d83e908185ca86955723a6 is queued; its execution will be recorded separately before ready-to-land.

Source review does not establish live quit/relaunch, disk-contention deadlines, sanitizers, macOS 14/15 behavior or light/STRESS performance; these were not run.

@austinywang

Copy link
Copy Markdown
Contributor Author

Check classification at a8c14e7ba96fb98dda0e1fc83508c03f49449024:

  • defaults guard: rerun passed after the fresh exact-head review receipt.
  • cmux-next crash-class rules: inherited. The PR job and exact merged-base run both report the CEF contract reviewed for cef-154.0.28-cmux.28 while the manifest pins cef-154.0.34-cmux.38, with the same SHA mismatch. The checker, contract and manifest are identical between base 24b2581cb9ddd399c3e40397f8ffee1b0f54fe9c and this head.
  • nightly-guards / workflow-guard-tests / release-notary: inherited. The PR job matches the base job: both signing-label RunsOnFallbackTests and the signing-hook allowlist test fail with the same missing hook. This PR changes none of their inputs.
  • nightly-guards / Guard status, if red, wraps that inherited nightly guard failure.

Compile, defaults guard and CI status are green. Focused fleet step d122999b65aac0cd95954400 verifies current-head persistence, quit and both scroll hosts together; ready-to-land is still off pending its result.

@austinywang

Copy link
Copy Markdown
Contributor Author

Ready-to-land handoff for a8c14e7ba96fb98dda0e1fc83508c03f49449024: fresh independent SHIP receipt, green current-head compile, defaults guard and static checks; no identified PR-caused red. Remaining inherited failures are classified with matching base logs.

The final merge-head focused rerun remains queued as fleet step d122999b65aac0cd95954400 and is not claimed as executed coverage. The last implementation head bb4203379a4b6d4ffe57cb928cef5fb5c7350001 passed all 42 focused tests, including the reproduced/fixed fixture race. The current merge preserves that repair and has fresh source review. Per the repository handoff rule, pending checks do not hold this reviewed head; the rerun continues.

@austinywang

Copy link
Copy Markdown
Contributor Author

Current-head verification update for a8c14e7ba96fb98dda0e1fc83508c03f49449024: focused fleet step d122999b65aac0cd95954400 passed all 42 tests across nine filter groups (ten suites). The combined ChatScrollRestore filter executed nine tests together, including all five nested default-host cases and the final-drain late-save refusal regression. The PR description now records this completed execution.

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (Independent impact/API reviewer): FIX at f647f95

P2: a pending tab creation can receive its real ID after AgentUnsentDrafts seals during quit. move then refuses the migration, leaving accepted draft text under the provisional ID while relaunch looks up the real ID. The creation reply/echo remains active during the final drain and CEF shutdown; relaunch wake state does not keep draft text as a fallback.

Readiness remains removed. The repair will settle accepted drafts' pending identity migrations before the final lane drain, under the existing quit deadline, and retain refusal of new edits. A focused regression will exercise the real tab-store rekey path. Source review established the finding; executable red/green results will follow.

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (Independent correctness and impact subagents; correctness first): FIX at 4b68a87

Both independent reviewers confirmed the quit/rekey persistence defect. The new regression exercises the production tab-store track/rekey path and disk restore by the real ID. It is running on the fleet before the fix (step 93ab3a9eb4ef069a66b16464). Readiness remains off. The repair also replaces iteration-count readiness polling with a clock deadline and checks durability before any cleanup drain.

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (Codex independent correctness reviewer): SHIP at 8a9fb26

Fresh read-only review of the full base-to-head diff found no verified correctness defects. The reviewer traced provisional registration, rekey, abandonment, restored draft lookup, normal and forced quit, overlapping drains, and writer ordering. Migration/removal is queued before identity waiters resume; new edits remain sealed. The reviewer independently confirmed exactly +8 physical App lines and preservation of every base exception.

This replaces the prior FIX verdict: the accepted draft now follows a pending tab to its real ID before the final drain. The strengthened regression inspects disk immediately after quit and covers abandonment and empty pending tabs.

Verification is separately owned by the primary agent: exact-head static gate and whitespace checks passed; official cmux-next/compile is green (fleet recipe-2 app Release compile 847e05ae711e6576fdc8ded3). Pre-fix fleet step 93ab3a9eb4ef069a66b16464 reproduced the real-ID/provisional-ID disk mismatch. Current-head focused step 1d09ccc1920090fc0069efea and the independent impact review are still running. No current-head live GUI quit/relaunch, sanitizers, macOS 14/15 or performance measurements were run. Readiness remains off pending the remaining verification.

PR: #25862; owner: austinywang; agent: Codex primary /root with independent correctness subagent /root/review_d22_correctness; base: 2dbca164fb4a06a95cf754a821b88b0672e684d9; head: 8a9fb26c1682c286845b9da29086cb67c34fe2ef.

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (Codex independent impact reviewer): SHIP at 8a9fb26

Fresh full-diff review found no surviving correctness defects. Both provisional creation paths register before page adoption. Reply/event rekey and failure/close release queue migration or deletion before settling waiters. Normal and forced quit retain their existing hard deadlines. The reviewer independently confirmed +8 App lines and preservation of every base allowance. This is a new exact-head review, with no inherited verdict.

The primary agent also classified the current nightly-guards / workflow-guard-tests / release-notary red as inherited: PR run 41196695363 and base run 41196909155 at 6a1571957057d02a53f2956909cf36a25cad11d4 both fail SigningHooksInStepWithGlaedaTests.test_every_listed_hook_is_one_glaeda_released_with_the_check on the same missing external hook df0e90b42c73838e01855a51636b7ed5c47fd918faf76ab815b5ce78aec54e57. Its test and hook list are unchanged by this PR. nightly-guards / Guard status is its wrapper. No source repair or rerun is needed here.

Source review only: the reviewer ran no builds, tests or live performance checks. The primary agent owns the green exact-head compile, static checks, red regression and running focused fleet verification. The defaults guard rerun now passes after the exact-head review receipt.

PR: #25862; owner: austinywang; agent: Codex primary /root with independent impact subagent /root/review_d22_impact; reviewed base: 2dbca164fb4a06a95cf754a821b88b0672e684d9; head: 8a9fb26c1682c286845b9da29086cb67c34fe2ef.

@austinywang

austinywang commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor Author

Verification complete at 8a9fb26.

Fleet step 1d09ccc1920090fc0069efea compiled and linked the macOS test bundle, then passed all 44 tests across nine focused filter groups (ten suites). UnsentDraftTests passed all five tests, including both arguments of the pending-tab regression (real-ID rekey and abandonment), and the empty pending-tab case. Disk restoration is checked immediately after quit, before cleanup. The combined ChatScrollRestore filter passed all nine tests, including all five default-host cases.

This completes red/green evidence for the identity repair: pre-fix step 93ab3a9eb4ef069a66b16464 at 4b68a875987a4214a7dbe9cf8a895e997051b40a reached the seal and failed precisely because the real-ID text was missing and the provisional-ID draft remained. The repaired head passes those assertions. The PR description contains the exact focused command, prior quit/scroll red-green evidence, the current compile and static checks, both independent SHIP receipts, and verification limits.

Both fresh full-diff reviews passed on this exact head. The only CI root red is the signing-hook assertion that also fails identically on feat-cmux-next; its matching log is linked above. Restoring ready-to-land after these receipts and results.

Autoland classification remains an external blocker. Its current verdict calls release-notary this PR's own red, although the base fails the same assertion. In the base watcher snapshot at 2026-10-31T11:48:35.598768+00:00, the nested release-notary entry has signed failure steps, but the older workflow-level cmux-next guards entry from run 41104078975 has steps: null. Replaying the current base_failures() function on that snapshot turns the signed nested entry into {"steps": null}. The watcher retains the old root entry because completed nested jobs settle only their nested keys. This prevents the classifier from recognizing the matching base failure. No shared service state was changed. Existing PR #29332 addresses the signing-hook drift; this PR's source remains limited to its own fixes.

The post-handoff autoland pass completed at 2026-10-31T12:34:43Z. It still says #25862 waits: its own red: cmux-next guards / nightly-guards / workflow-guard-tests / release-notary and separately reports that #29332 lacks a SHIP receipt. This PR retains its exact-head ready-to-land label; the service has not accepted it as landable.

PR: #25862; owner: austinywang; agent: Codex /root; reviewed base: 2dbca164fb4a06a95cf754a821b88b0672e684d9; head: 8a9fb26c1682c286845b9da29086cb67c34fe2ef. No live GUI quit/relaunch, sanitizers, macOS 14/15 or light/STRESS performance verification was run; no tagged app was produced.

@lawrencecchen

Copy link
Copy Markdown
Contributor

Review (claude-rereview): SHIP at 8a9fb26

I am an independent reviewer. I did not write this PR or its merges. I read the full diff of this head against its merge base 2dbca164fb4 (23 files). The head merges cleanly into the current feat-cmux-next tip 625b1ca07f9.

Findings

No blocking defects. Two notes for a follow-up. They do not hold this PR:

  1. Packages/macOS/CmuxNext/Sources/CmuxNextApp/AppDelegate.swift:45 and :529: after this PR, no production code writes through AgentPaneStateFile.write. Both state owners now save through StateFileWriter, so AgentPaneStateFile.flushAll() drains queues that are always empty. These two allowBlocking(.terminate) calls (and AgentPaneStateFile.write / flushAll, used only by AgentPaneStateFileTests) are now dead code. REVIEW.md "Dead code" asks to remove an API that a change makes unreachable. The PR body keeps the type as the read seam on purpose, so a follow-up is enough.
  2. Packages/macOS/CmuxNext/Sources/CmuxNextAgentPane/AgentPanePersistenceQuit.swift:23: identitiesSettled() has no limit of its own. If a pending tab with an accepted draft never gets its create reply, the normal quit uses the 15 s engine-stop deadline before CefShutdown runs, and the forced exit uses its 160 ms bound. Today the create retry ends after about 10 s plus one request timeout, and release(key) then frees the wait, so the deadline still bounds the quit. threading-model.md records this budget sharing. I note it only so that nobody later adds a create path that skips track/release.

What I checked

  • StateFileWriter (CmuxNextWakeups/StateFileWriter.swift): the admission and pending state is under one Mutex. Encoding and I/O run outside it. A save that arrives between the last take and finished() is drained again, not lost. A blockShutdown post is never refused after the lanes close (LaneScheduler keeps those jobs), so running cannot stay true. flushed() resumes only after the run that holds the save ends.
  • Quit sealing: finishSaving() runs for both owners in one main-actor turn before the first suspension. Late chat.unsentDraft and chat.scrollAnchor calls get a closed refusal on both paths: the WebKit bridge (AgentPaneScrollAnchors+Bridge.swift:59) and the page provider (AgentPageProvider+Scroll.swift:19). The page code catches the rejection (App.tsx postUnsentDraft, sendScrollAnchor).
  • Pending-tab identity: track registers provisional keys in both agent create paths (AgentTabs+Lifecycle.swift:99, AgentTabs+OptimisticWorkspace.swift:52). rekey -> move queues the real-ID snapshot, then releases the wait. A failed or cancelled create reaches release(key) -> abandonIdentity, which removes the provisional draft even though set("") is refused while sealed. finishSaving keeps only the identities that hold a draft. A second drain (forced exit after a normal quit) is safe.
  • Quit ordering: normal quit (QuitCoordinator.swift:226) drains after Home's last flush and after deadline.arm, before cef.shutdown(). The forced exit (AppDelegate.swift:50) drains after Home's flush under its 160 ms bound.
  • Repo rules: no try!. No new main-thread blocking: the blockingWait is only in a test on a lane thread, and each Mutex has a concurrency-allow reason. No UI, motion, dialog, font or shortcut changes, so no Ghostty-defaults trailer is needed. No BSL server directories are touched. The app_lines +8 arch-ratchet allowance has a reason, an owner, an expiry and a review receipt.
  • Red-first tests: the PR has red commits before each fix (3afa9e4846f, 642b8f4f04e/21bd10296be, 4b68a875987), and the body lists fleet step IDs for red and green. The tests check the disk state before cleanup, and they wait on signals, not on sleeps.
  • guardrails.py --sites (scripts/cmux-next/guardrails.py:2884) is diagnostic only and does not change push enforcement. It has a unit test.
  • Compile on this exact head: cmux-next fleet Release compile succeeded and cmux-next/compile passed (check-runs API, head_sha 8a9fb26c168). I did not run a new compile.

Checks on this head

  • nightly-guards / workflow-guard-tests / release-notary: base red. The test fails because glaeda hook df0e90b4... is not in glaeda-signing-hooks.txt. The same job fails on the feat-cmux-next tip 625b1ca07f9 (run 41206037511) and on the two tips before it. This PR does not touch that test or the hook list.
  • nightly-guards / Guard status: a wrapper of the red above.
  • next-autoland / verdict: it calls release-notary this PR's own red. The evidence above shows it is a base red.
  • pr-red-checks / comment: the job was cancelled (infra; it posts a comment and is not a test).
  • cmux-next swift test and cmux-next fast tier: pending.
  • 66 checks pass.

What I did not check

  • I did not run any Swift tests myself. I rely on the fleet steps in the PR body for the focused suites.
  • I did not do a live GUI quit and relaunch, a disk-contention timing test, macOS 14/15 runs, or light/STRESS measurements.
  • I did not review ChatScrollRestoreDefaultPathTests.swift line by line. It is mostly re-indentation under the serialized parent suite.

@lawrencecchen

Copy link
Copy Markdown
Contributor

Review (claude-review-subagent): BLOCK at 6aaafd0

Independent review of the full change against origin/feat-cmux-next (4f957ed) and of the merge 6aaafd0a3a2 (parents 780192f and 4f957ed). The textual conflict was resolved correctly: the merge keeps feat's cef.shutdown(timeout: QuitDeadline.engineCloseWait), the quit.engines_stopped and quit.replied marks, the QuitSteps(... limiter:timeLeft:) runner and the Home cache flush, and it puts the PR's drain at the same point as before. But the PR's quit model was not reconciled with the quit model that #22919 (QuitStepLimiter, new since the old merge base 3d28563) added. That causes the two blocking findings below.

Blocking

  1. The quit's own draft settle can be refused (data loss in a slow keep quit). QuitCoordinator.swift:235, AgentPanePersistenceQuit.swift:21-22, AgentUnsentDrafts.swift:63.
    On the old base, prepareWindows ran in sequence. So every page's pane.wakeState ask, which first settles the composer and posts chat.unsentDraft (schemas/agent-pane/wake-state.json, draftSettleMs 500, AgentPaneRelaunchWake.askLimit 1 s), finished before stopBrowserEngines sealed the owners. On feat, agent_composers is a limited save step (QuitSaveSteps.swift:87, 400 ms). Past its limit it keeps running, and QuitSteps.run waits for it only max(0, timeLeft - engineCloseWait) (QuitSteps.swift:91-92). That wait is 0 when a keep quit has used about 1.8 s of its 4 s deadline. Then stopBrowserEngines calls finishSaving() while the composer step still runs. The page's settled chat.unsentDraft arrives after the seal, set returns false, and the page gets closed. The new chat's last typed text is lost.
    On feat without this PR, the same late message is accepted, and its own utility queue writes it within milliseconds. This is the slow-quit case that cmux NEXT: a keep quit ends within 4 s whatever any step does; Restart to Update never leaves cmux invisible #22919 was built for. The PR body says that only "late page edits" are refused, but this is the quit's own requested flush.
    Possible fixes: (a) do not seal until the agent_composers step has ended or the deadline has passed (for example, the limiter finishes that named overrun first, under the remaining deadline), or (b) accept saves until the final drain starts, and refuse only after work.shutdown() returns.

  2. An unbounded wait now sits in front of CefShutdown and uses up the engine-close reserve. AgentPanePersistenceQuit.swift:23-24, QuitCoordinator.swift:235-238.
    Feat budgets the time before the engines so that "Chromium still closes its pages and CefShutdown flushes cookies and storage" (QuitSteps.swift:87-92, engineCloseWait 2 s). The PR adds two waits after that budget and before cef.shutdown. Neither wait has its own bound:

    • identitiesSettled() waits for every pending tab creation that holds a draft. A new-conversation-tab request that the daemon has not answered has no timeout (AgentTabs+Wiring.swift:421-430).
    • BackgroundWork.shutdown() waits for every blockShutdown item in the app. This includes the updater's rollback keep (clone plus prune signature reads, "seconds on a busy disk"; UpdaterService+Rollback.swift:49). Before this PR, nothing called shutdown().

    In both cases the quit now runs to the 3-4 s deadline _exit, and CefShutdown never runs. Feat ran CefShutdown at once. The PR's own threading-model.md text records this ("a slow one (the update rollback clone) can use up the engine-stop deadline before CefShutdown runs"), but it is a regression in quit time and in browser state durability compared with the base.
    Possible fix: bound the drain by deadline.timeLeft(margin:) - engineCloseWait, or await only the two writers' flushed() before CEF and call work.shutdown() after cef.shutdown, under the remaining deadline.

Non-blocking

  1. StateFileWriter.swift:65: drafts and scroll places now share the single serial .files bestEffort slot (LaneScheduler perPriority) with the rollback clone and other bestEffort file work. While that work runs, every draft save waits behind it. At quit, the last draft write can sit behind the clone until the deadline. AgentPaneStateFile gave each file its own queue. Consider a lane priority or a slot that a multi-second item cannot block.
  2. After work.shutdown(), the lanes refuse every non-blockShutdown item for the rest of the quit (the overruns that run beside the engine stop, and applicationWillTerminate). I found no current quit-path user of services.work: the save steps use Task.detached, and Cloud's step uses the lanes only to launch. This is safe today but easy to break. A comment at the call would help.
  3. guardrails.py:2892-2894: --sites writes an empty tree object and a commit object into the repository's object store (hash-object -w, commit-tree). This is harmless but leaves loose objects. git hash-object -t tree /dev/null without -w, or the well-known empty tree, would avoid it.

What I checked and found correct

  • StateFileWriter: latest-wins admission under the mutex, no I/O under the lock, ordered writes, flushed() waiter wake-up, and a throw that skips only that save. A save after a run ended starts a new run, with no lost wake-up.
  • Pending-identity flow: track registers provisional keys. finishSaving keeps only keys that hold drafts. rekey → move queues the real-ID snapshot before settledIdentity releases the waiter, so the write is counted in blocking before shutdown() runs. Failure, cancel and close go through release → abandonIdentity. A second tabCreated after the echo is a no-op.
  • Forced quit (AppDelegate.swift:43-51): the drain runs after Home's last batch, under the 160 ms bound. A second drain after a normal quit's drain is idempotent.
  • No try!. The main thread does no new disk work (both owners only snapshot values and post). Nothing on feat writes agent-unsent-drafts.json or agent-scroll-anchors.json outside the new writers. AgentPaneStateFile stays as the read seam. App line allowance: the merge added no App lines (+8 is unchanged).
  • I did not compile or run tests on this machine (repo rule). This verdict comes from reading the code.

@lawrencecchen

Copy link
Copy Markdown
Contributor

Merge of feat-cmux-next pushed as 6aaafd0 (one conflict, QuitCoordinator.swift: the drain now runs after feat's Home cache flush and before feat's cef.shutdown(timeout: QuitDeadline.engineCloseWait)). Fleet compile is running. I removed ready-to-land; I do not add it back.

The independent review of this head is BLOCK (#25862 (comment)). The cause is not the textual merge: #22919 (on feat after this branch's old base) gave each quit save step a time limit (QuitSaveSteps, QuitSteps.run: agent_composers 400 ms, then the wait is bounded by timeLeft - engineCloseWait). This PR's AgentPanePersistenceQuit.drain() has no limit and seals the drafts owner.

@austinywang, a decision for you (the fix changes this PR's quit model):

  1. Late drafts: on a slow keep quit, a page's chat.unsentDraft can arrive after the seal and is refused, so the last typed text of a new chat is lost (feat without this PR accepts it). Do you want the seal to wait for the agent_composers step's late answers, or to accept writes until cef.shutdown starts?
  2. Drain bound: identitiesSettled() and BackgroundWork.shutdown() (it also waits for the updater rollback copy) can use the 2 s CEF close reserve, so the process can end before CefShutdown flushes cookies. Should the drain be bounded by deadline.timeLeft(margin: QuitDeadline.engineCloseWait) (and give up on the rest), or should blockShutdown work outside agent-pane state stay out of this drain?

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (independent correctness subagent): SHIP at 33c9938

A fresh read-only subagent reviewed the full diff against feat-cmux-next 01facc1b131, the merge resolution ca0091ab7c, and the delta since 89189f9297.

Findings and how each was resolved

  1. Fixed at 33c9938a85. --sites failed open: measure() turns a crashing rule into a guardrails warning, and tree_sites dropped that warning, so it printed 0 and exited 0. That count would read as debt paid down (REVIEW.md "Fail closed"). It now exits non-zero when the diff read, the generic-signature context or this rule fails.
    • Red first: 377b591254 adds test_sites_fails_closed_when_the_rule_does_not_run. Against the old code it failed with AssertionError: SystemExit not raised.
    • Green: python3 -m unittest tests.test_cmux_next_guardrails_blocking passes all 9 tests at the head.
  2. Fixed. Warning-rule sites were not stripped while refusal sites were; both now use one format. Not changed (non-blocking): --sites ignores --base, and wins over --list when both are given.

What the reviewer checked

  • No PR Swift left. git diff origin/feat-cmux-next HEAD -- Packages is empty, and git grep "StateFileWriter\|AgentPanePersistenceQuit" finds nothing.
  • The base covers the PR's original goal. AgentPaneStateFile saves on one serial queue per file, and AgentPaneStateFiles gives each file one owner. AppServices.stateFilesDrained() runs under the armed quit deadline, both before and after the engines close, and on the second-signal exit. AgentPaneStateLoss records saves still queued at the deadline.
  • Earlier BLOCK resolved. The two blocking findings of Part of #25852: guardrails --sites, the whole-tree main-blocking count #25862 (comment) (a late draft refused by the seal, and an unbounded drain before CefShutdown) existed only in this PR's own seal-and-drain design, which is now removed. The base never seals, so a late draft or a provisional-tab draft rename is queued like any other save.
  • Empty tree. It comes from git hash-object -t tree --stdin without -w, so it suits a SHA-256 repository and writes no objects (loose-object count unchanged across a full run). Base-side reads come back missing and become empty texts.
  • Count. --sites main-blocking gives 75 at 01facc1b131, which matches the threading-model.md line.

Not run: no Swift build or test, because this head changes no Swift.

@austinywang

Copy link
Copy Markdown
Contributor Author

Check classification at 33c9938a8516bdc85895c60f675174c76ec8eae1:

@austinywang

Copy link
Copy Markdown
Contributor Author

Ready for review at 33c9938a8516bdc85895c60f675174c76ec8eae1. Mergeable; nothing red is caused by this PR.

Not merged, and ready-to-land is not added: it waits for the owner's decision.

austinywang and others added 5 commits November 3, 2026 22:04
…tree, base debt included (#25852)

`scripts/cmux-next/guardrails.py --sites main-blocking` judges what HEAD adds over an empty commit and prints every
site and the count, so threading item 1's slice (c) has one repeatable number: 79 at 101e904, 75 at this
branch. threading-model.md names the command. Test: test_sites_counts_the_whole_tree_base_debt_included
(python3 tests/test_cmux_next_guardrails_blocking.py: 7 tests OK, this Mac; Python only).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eproduces (79 at cec0a36)

Review 5 nit on #25862: --sites main-blocking reports 0 at 101e904, 79 at
747aac1 and cec0a36, 75 at this branch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Red on this commit: a crashing main-blocking rule makes tree_sites return
an empty list (count 0), which reads as debt paid down.
python3 -m unittest tests.test_cmux_next_guardrails_blocking.BlockingRuleTests.test_sites_fails_closed_when_the_rule_does_not_run
-> AssertionError: SystemExit not raised.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
measure() fails open per rule; tree_sites now exits non-zero with the
failure instead of printing a count that left the rule out (review of
89189f9, finding 1). Warning-rule sites are stripped like refusal sites.
Green: the same unittest command, and the whole file (9 tests) passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 51c3b5d2 Deployed Nov 4, 2026 by austinywang via wait for same-key tree publisher #110021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants