[lawrencecchen]

Summary

Base red, priority zero (STATUS 2638, 2643). a8502be (#28704, "menus opened inside a native popup keep the popup open") breaks the feat-cmux-next tip twice:

• compile: NativePopupHost.swift:235:46 and :239:46 "sending 'menu' risks causing data races". The NSMenu read from the notification was sent into MainActor.assumeIsolated (tip-watcher job ec729ac24847cb52bde83a60).
• arch: assume_isolated_outside_adapters +3 and observer_sites +2. The cause is two raw NSMenu did{Begin,End}Tracking observers with assumeIsolated in the popup host, plus a DispatchQueue.main.async + assumeIsolated.

Takes back #28704: its menu observation moves into one seam adapter. The popup's behavior is unchanged: it still stays open while a menu it opened tracks, and it takes the keys back after.

Root fix (PATTERNS.md P2 and P3; no escape comment added):

• MenuTracking (new, in CmuxNextCompat) is the one seam for the two NSMenu tracking notices. It observes both through MainActorObservation and calls its subscribers on the main actor with .began(menu) / .ended(menu).

• MainActorObservation gains atOnce: (queue nil). With it, the handler runs in the poster's own call, as fix(next): menus opened inside a native popup keep the popup open (bookmark bubble's Folder picker) #28704 needs: a press that opens a menu sees it before its key-loss check. Only a post made on the main thread is delivered. The type's one reviewed main-delivery proof covers both modes. The NSMenu is read on the main actor inside that proof and is never sent across isolation.

• NativePopupHost keeps a MenuTracking while it watches and drops it in unwatch, as it did with the raw observers. Its deferred dismiss check is now a Task { @MainActor } instead of DispatchQueue.main.async + assumeIsolated.

• The two other NSMenu tracking observers use MenuTracking too, so those notices are observed in one place:

  • ChromiumWarmup's wait for a menu to end: a raw observer with assumeIsolated before.
  • TabMenuTracking's end of the strip's menu: it now filters the ended menu in its subscriber instead of by object:.

  Both now hear the end inside AppKit's post instead of right after it. CmuxNextChromiumWarmup declares its CmuxNextCompat dependency (core).

Testing

arch-ratchet.py against the merge base 07efced: assume_isolated_outside_adapters -5, observer_sites -2, no architecture count rises.

On the current tip 94cabb6, measured from the last green 6d639a7: this PR removes the two #28704 rows. Two rows stay red, and neither is from #28704:

• global_mutable_statics +1 in CmuxNextCompat, from a later commit;
• app_lines +351, from cloud.startChat: a Mac workspace keeps the Cloud machine it picked; workspaces reuse and share machines #29532, cmux NEXT agent chat: a relaunch never draws a black picture of a see-through page #29058 and later commits. Those are separate items.

defaults_guard.py check: exit 0. layer_gate.py: ok. The namespace lint: OK. prepush-gate.py: no new violations.

Observed / Not run

Observed: the checks above. Pending, posted as a comment when they finish:

• the fleet compile with tests (--app --package CmuxNext --recipe-version 2, job 19c8460528747fe4526b50dd);
• the suites on a display worker (step 9a2d7b42544ba82f7c7f09ef, cmux7s, at c3d8696): PASSED: NativePopupHostChildMenuTests (fix(next): menus opened inside a native popup keep the popup open (bookmark bubble's Folder picker) #28704's own), PagePopupMotionTests, CaptureBackdropTests, ChromiumWarmupTests, MainActorObservationTests, MainActorObservationReleaseTests. The step's exit 1 came only from my list entry TabStripMenuTests: no suite has that name (0 tests ran). The real suite for the tab-strip menu path (TabMenuTracking) is PaneTabBarControlsTests: display step 8e6349ea70b75456015500b6, result posted as a comment when it ends.

Not run: no local Swift build (fleet only).

Changelog

none

Proof

No capture: no visible change; the same popup behavior goes through one menu-tracking seam.

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

---

Summary by cubic

Takes back #28704: a popup stays open while a menu it opened tracks and then retakes the keys, without the compile error or the new assumeIsolated and observer rows it caused. NSMenu tracking notifications now flow through one seam adapter (MenuTracking) instead of raw observers with assumeIsolated.

Changes

• MenuTracking (new, in CmuxNextCompat) observes didBeginTrackingNotification and didEndTrackingNotification through MainActorObservation and calls subscribers on the main actor with .began(menu) / .ended(menu).
• MainActorObservation gains atOnce: so the handler runs in the poster's own call on the main thread; the menu object is read on the main actor inside the main-delivery proof and never crosses isolation.
• NativePopupHost holds one MenuTracking while it watches and drops it in unwatch; its deferred dismiss check becomes a Task { @MainActor }.
• ChromiumWarmup and TabMenuTracking use MenuTracking too and hear the menu end inside AppKit's post instead of right after it; CmuxNextChromiumWarmup declares its CmuxNextCompat dependency.

^{Written for commit 87682a6. Summary will update on new commits.}

[github-actions]

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

Review 87682a6: 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=pr29856 -f ref=base-red/menu-tracking-adapter -f platform=macos -f backend_mode=local

Shipped in:

• cmux NEXT nightly 1.0.0-nightly.4130052256501 (28aa638015)

[cubic-dev-ai]

1 issue found across 6 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="Packages/macOS/CmuxNext/Sources/CmuxNextCompat/MenuTracking.swift">

<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextCompat/MenuTracking.swift:9">
P3: `MenuTracking` has no unit tests. Add a `MenuTrackingTests` that posts `NSMenu.didBeginTrackingNotification` and `didEndTrackingNotification` on an injected `NotificationCenter`. It should check synchronous delivery, the `.began`/`.ended` mapping, and that releasing the tracker stops delivery.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[cubic-dev-ai]

P3: MenuTracking has no unit tests. Add a MenuTrackingTests that posts NSMenu.didBeginTrackingNotification and didEndTrackingNotification on an injected NotificationCenter. It should check synchronous delivery, the .began/.ended mapping, and that releasing the tracker stops delivery.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextCompat/MenuTracking.swift, line 9:

<comment>`MenuTracking` has no unit tests. Add a `MenuTrackingTests` that posts `NSMenu.didBeginTrackingNotification` and `didEndTrackingNotification` on an injected `NotificationCenter`. It should check synchronous delivery, the `.began`/`.ended` mapping, and that releasing the tracker stops delivery.</comment>

<file context>
@@ -0,0 +1,39 @@
+/// notices (plans/cmux-next/PATTERNS.md P2, P3): an owner keeps one while it follows menus and
+/// subscribes; releasing it stops observing.
+@MainActor
+public final class MenuTracking {
+    /// A menu began or ended tracking.
+    public enum Change {
</file context>

[lawrencecchen]

Review (claude-review-subagent): SHIP at c3d8696

Scope: git show c3d86964b02 against a8502be (#28704). I read the code only. I did not compile or run Swift on this machine.

Correctness and concurrency

• MainActorObservation (generic init): guard Thread.isMainThread comes before MainActor.assumeIsolated, so the main-delivery proof is sound for queue: nil. For the existing queue: .main callers the guard is always true, so their behavior does not change.
• read(note) runs inside the assumeIsolated closure, so the NSMenu is created on the main actor and goes to handler on the same delivery. No value crosses an isolation boundary, which was the cause of the fix(next): menus opened inside a native popup keep the popup open (bookmark bubble's Folder picker) #28704 "sending 'menu'" error. The captures (@MainActor function types, and note as nonisolated(unsafe)) are the same as in the init that compiles on base. atOnce ? nil : .main gets its type from the OperationQueue? parameter.
• MenuTracking: the class is @MainActor, send is main-actor isolated, and it calls each (Change) -> Void subscriber synchronously. The non-Sendable Change does not cross isolation, so I expect no sending diagnostic. The subscriber closures in the UI targets (MainActor by default) take the main-actor isolation of the place that creates them. This is the same pattern as onLostKeys and the NSEvent monitors in NativePopupHost, which compile on base.
• Release during send, in ChromiumWarmup removeMenuObserver() and TabMenuTracking end(): self?.send(...) keeps a strong temporary reference until the call returns, and for subscriber in subscribers iterates a copy of the array. The observer is removed inside its own block, as the old code also did, and NotificationCenter allows that. This is safe. armIdle only schedules a timer and does not create a new tracker again in the same call.

Behavior equivalence

• NativePopupHost: guard observers.isEmpty still guards menus, because observers always holds the 5 window/app observers after watch. unwatch clears both. The old filters (main thread only, object as? NSMenu) are kept: the guard plus read plus if let menu. The injected center is still used.
• Task { @MainActor } and DispatchQueue.main.async both enqueue on the main queue, so the check still runs after the press. NativePopupHostChildMenuTests posts synchronously on its own center and waits with Task.sleep(50 ms). The Task hop runs in that time, and the expectation #expect(host.isShown, "the check waits until the press is handled") still holds.
• TabMenuTracking: ended === menu with [weak menu] matches the old object: menu filter, including the case where the menu is deallocated.
• Informational: the commit message says ChromiumWarmup and TabMenuTracking "now hear the end in AppKit's post instead of right after it". That is not correct. With queue: .main, a post made on the main thread already runs the block inline: MainActorObservationTests expects runs == 2 directly after two synchronous posts. So nothing changes in timing. It also follows that atOnce only changes what happens to a post made off the main thread (it drops it instead of hopping to main). The change is safe, but it adds API surface for almost no effect.

Non-blocking follow-ups

• P3 says to observe a system notification "once per app". Now each owner makes its own MenuTracking, with 2 observers for each instance. An app-wide instance injected from AppServices (P1) would meet P3 completely. DebugHints still observes the NSMenu tracking notices directly, so "observed in one place" is not fully correct.
• No direct test covers atOnce (main post delivered, off-main post dropped) or the MenuTracking subscribe/release path. NativePopupHostChildMenuTests covers them indirectly.

Ratchets (run at the head over HEAD~1..HEAD)

• arch-ratchet: assume_isolated_outside_adapters -5, observer_sites -2, "no architecture count rises".
• guardrails: ok. One owner warning: ChromiumWarmup is owned by PL-cef-tab. Tell the owner.
• crash_ratchet: ok. One count went down.
• defaults_guard check: exit 0.
• prepush-gate --commits HEAD~1..HEAD: 0 add violations.
• check-layers: ok. Layer edge: ChromiumWarmup (service) -> Compat (core) points down.

PR body: it has Takes back #28704: <why>, the risk:normal label, ## Changelog none, ### Observed / Not run, and No capture:. The fleet compile and the display suites are still pending, as the body says.

[blacksmith-sh]

Found 1 test failure on Blacksmith runners:

Failure

Test	View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl ights_validate)/ test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl ights_validate)	View Logs

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[lawrencecchen]

Fleet evidence at c3d8696:

• Compile (cmux-ci compile-check --app --package CmuxNext --recipe-version 2, job 19c8460528747fe4526b50dd): app passed (89 s, "Build complete!"), cmux-tui passed.
• Display suites (step 9a2d7b42544ba82f7c7f09ef, cmux7s): NativePopupHostChildMenuTests, PagePopupMotionTests, CaptureBackdropTests, ChromiumWarmupTests, MainActorObservationTests, MainActorObservationReleaseTests all passed with tests run. The step exit 1 is only my list entry TabStripMenuTests, which names no suite (0 tests). Not a red of this change.
• The tab-strip menu path (TabMenuTracking) is covered by PaneTabBarControlsTests: display step 8e6349ea70b75456015500b6, result in a later comment.

[teamleaderleo]

For awareness: #29871 (merged as ac5389c) fixed the same NativePopupHost.swift:235/239 "sending 'menu'" compile red on feat-cmux-next. Its Release compile was green at the PR head. This PR also covers the assume_isolated/observer arch rows, so it may still be needed for those, but it will now conflict in NativePopupHost.swift.

[lawrencecchen]

PaneTabBarControlsTests (the tab-strip menu path through TabMenuTracking): display step 8e6349ea70b75456015500b6 on cmux7s at c3d8696: 8 tests in 1 suite passed. The #29856 evidence is complete; the later head bc6570c is only a cmux-bot merge of the base.

[teamleaderleo]

FYI: #29871 (merged ac5389c) already fixed the NativePopupHost compile red with nonisolated(unsafe) binding (base Release compile green at ebf5b25: https://github.com/manaflow-ai/cmux/actions/runs/41260253412/job/124712020722). This PR's seam adapter and arch rows may still be worth landing on their own; leaving it to you.

[lawrencecchen]

Review (claude-review-subagent-cw): SHIP at 87682a6

Merge resolution (87682a6, parents bc6570c + b85875a)

• git show --remerge-diff shows one conflict hunk only: Packages/macOS/CmuxNext/Sources/CmuxNextDesign/Overlay/NativePopupHost.swift:232-241. All other files merged with no conflict.
• The resolver kept the PR side (MenuTracking). This is correct. Feat's base red: NativePopupHost's menu-tracking observers bind the menu nonisolated(unsafe) before assumeIsolated #29871 (ac5389c) only changed the two raw observer blocks to bind the menu nonisolated(unsafe). The PR deletes those blocks. The same proof now lives one time in CmuxNextCompat/MainActorObservation.swift:73-79 (nonisolated(unsafe) let note inside the main-thread check). Thus the intent of feat (no "sending 'menu'" compile red) is kept, and the intent of the PR (one seam, no raw observer or assumeIsolated in the popup host) is kept.
• I compared the PR's own diff (b85875a4be9...bc6570cfe4b) with the net diff (b85875a4be9..87682a642ac). The added lines are identical. Only the removed lines are different: they are now feat's base red: NativePopupHost's menu-tracking observers bind the menu nonisolated(unsafe) before assumeIsolated #29871 version. No feat line is lost by accident.
• Semantic conflicts: none found. In the PR's six files, feat changed only NativePopupHost.swift. Feat adds no new MenuTracking name and no new raw didBegin/EndTrackingNotification observer. The only other user of the read: init, CmuxNextApp/Control/DebugHints.swift:21,24, still compiles: the new atOnce: Bool = false parameter has a default and comes before read:.

Net change over feat (correctness)

• MenuTracking.swift:20-29: the stored properties have defaults before [weak self] is captured in init, so init is valid. The MainActorObservation deinit removes the observers when the owner releases the tracker.
• Release during delivery is safe. TabMenuTracking.end() (TabMenuTracking.swift:31) and ChromiumWarmup.removeMenuObserver() (ChromiumWarmup.swift:256) set the tracker to nil inside its own send. self?.send(...) keeps a strong reference until the call returns, and the loop goes through a copy of the subscribers array. NotificationCenter allows an observer to be removed during its own post.
• NativePopupHost.swift:234-241,262: watch creates the tracker and unwatch releases it. This is the same lifetime as the old observers tokens. Tests post on the injected center from the main actor, so atOnce delivery is synchronous, as NativePopupHostChildMenuTests expects.
• NativePopupHost.swift:70-74: DispatchQueue.main.async + assumeIsolated changed to Task { @MainActor }, with a task-owner note. The check still runs after the press returns and still checks the generation. The tests wait 50 ms, so this timing is sufficient.

Notes (not blocking)

• TabMenuTracking.swift:21-24 now observes the end of every menu (object nil) and filters with ended === menu. Before, the observer filtered with object: menu. The behavior is the same and the cost is very small.
• ChromiumWarmup.swift:208-214 now runs its end handler inside the AppKit post, not after it. The handler only schedules the idle timer, so there is no change that the user can see.
• MainActorObservation.swift:74: the new Thread.isMainThread check is redundant on the queue: .main path, but it does no harm.

Observed / Not run

• I read the remerge diff and both one-sided diffs, and I searched the merged tree for all users. Not run: no Swift build or test on this machine. When I made this review, the CI compile and swift-test jobs on this head were still pending, and no check had failed.

[lawrencecchen]

Review (claude-review-subagent): SHIP at 87682a6

Correctness review of the whole diff against merge base b85875a (6 files). No compile or test run on this machine (fleet only).

Conflict resolution (NativePopupHost.swift). The merged file is the same as the earlier head bc6570c. It keeps everything #28704/#29871 intended:

• Delivery is synchronous. MenuTracking uses MainActorObservation(..., atOnce: true), which registers with queue: nil, so menuBegan runs inside AppKit's post, before the press's key-loss check. NativePopupHostChildMenuTests.thePickersMenuTakingTheKeysLeavesTheBubbleOpen asserts this ordering directly: it posts, then calls resignKey() with no runloop turn between them.
• Only main-thread posts are delivered. A post off the main thread is dropped by guard Thread.isMainThread before assumeIsolated, as base red: NativePopupHost's menu-tracking observers bind the menu nonisolated(unsafe) before assumeIsolated #29871 did. Nothing off the main thread reaches main-actor state. The NSMenu is read and used inside the one proof in MainActorObservation and never crosses isolation.
• .began goes to menuBegan and .ended goes to menuEnded. The nil-object filtering is the same as before.
• Cleanup: unwatch() sets menus = nil, and MainActorObservation.deinit removes both tokens.
• No retain cycle. The host holds MenuTracking, whose subscriber captures the host weakly. MenuTracking's observation handlers capture MenuTracking weakly. The tokens hold only those closures.
• The deferred dismiss changed from DispatchQueue.main.async + assumeIsolated to Task { @MainActor [weak self] }. It still runs after the press, with the same generation, isShown, hasOpenChild and isKeyWindow guards. The two async tests sleep 50 ms, which covers it.

Other files

• MainActorObservation atOnce: the default false keeps queue: .main. The new guard is a no-op there. Existing callers are unchanged.
• MenuTracking: all properties have defaults, so the init closures capture self only after full init. A subscriber that releases its MenuTracking during delivery is safe: the caller of self?.send holds a strong reference for the call, and the loop runs over a copy of the array. Removing a NotificationCenter observer inside its own delivery is supported.
• ChromiumWarmup and TabMenuTracking now get the menu end inside AppKit's post instead of on the next main-queue turn. Both handlers are idempotent and cheap: armIdle only schedules a timer, and the strip's later end() returns early on menuOpen == false. TabMenuTracking's ended === menu check with a weak menu matches the old object: menu filter, because the menu is alive while it posts its own end.
• Package.swift: CmuxNextChromiumWarmup declares CmuxNextCompat. CmuxNextDesign and CmuxNextTabs already depended on it. CmuxNextCompat already imports AppKit.

Arch ratchet (arch-ratchet.py --base origin/feat-cmux-next --head pr/29856, judged over the feat tip 33995df): assume_isolated_outside_adapters -5, observer_sites -2, assume_isolated_total -4 (report only), "no architecture count rises". The head also merges cleanly into 33995df.

Non-blocking notes

• The ChromiumWarmup and TabMenuTracking menu-end paths have no direct test. The suites in the description (ChromiumWarmupTests, PaneTabBarControlsTests) do not post didEndTrackingNotification.
• CmuxNextApp/Control/DebugHints.swift still observes both tracking notices through MainActorObservation directly. So "observed in one place" in the description is not fully true. This is a Debug-only path and is not a defect.
• The failed check nightly-guards / workflow-guard-tests / release-notary does not come from this PR, which changes no workflow, release or notary file.

[lawrencecchen]

Fleet compile at 87682a6 (job 5848fe428130370596cc942e, --app --package CmuxNext --recipe-version 2): CmuxNext passed (323 s), app passed (88 s), cmux-tui passed.

[lawrencecchen]

Conflict wave: merged feat-cmux-next b85875a into this branch (merge 87682a6). One conflict, NativePopupHost.swift: feat's #29871 (ac5389c) fixed the same compile red by keeping the two NSMenu observers; this PR replaces them with the MenuTracking seam adapter (its purpose), so I took this PR's side. Feat's other #29871 change is in MainActorObservation.swift and is kept.

Fleet evidence at 87682a6:

• Compile (cmux-ci compile-check --package CmuxNext --app, job 8e0e7507fff99c9e0019d0cc): CmuxNext passed (256 s), app passed (88 s).
• Display suites (step 7baa2efef169eaa3c99cbc2e, cmux7s, --display, CMUX_TEST_REQUIRE_GUI=1): NativePopupHostChildMenuTests 4, PagePopupMotionTests 7, CaptureBackdropTests 7, ChromiumWarmupTests 5, MainActorObservationTests 5, MainActorObservationReleaseTests 2, PaneTabBarControlsTests 8: all 7 suites passed, tests ran.
• Review: SHIP at this head (comment 6551966643).