Skip to content

cmux next focus: no reclaim inside the grant of a page that still has focus - #25689

Merged
austinywang merged 8 commits into
feat-cmux-nextfrom
next-reclaim-inside-grant
Oct 27, 2026
Merged

austinywang merged 8 commits into
feat-cmux-nextfrom
next-reclaim-inside-grant

Conversation

@austinywang

@austinywang austinywang commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #25222 (follow-up): ChildPageFocusKeyChangeTests.aReapplyInsideTheGrantDoesNotGrantAgain() is red on feat-cmux-next. It has failed since #24913 added it, and it surfaced when #25222 cmux-next swift test run put the suite in its priority filter. Base pushes skip that job.

  • What went wrong: the test models a re-apply inside the grant that calls reclaim(into:when:) while the granted page still has focus. reclaim then gave the keys straight back to the cmux window: two key changes for one focus request (["pageB", "cmux"]), with the page still recorded as focused and no keys. No production path is known to reach this today (focusPage never reclaims, and blurChildWindowPage blurs first), so the fix is defensive: it makes ChildPageFocus honor the contract the test states.
  • Fix: ChildPageFocus.reclaim does nothing inside the grant of a page that still has focus. keysReturned(to:during:) already treats key changes inside a grant as the grant's own; reclaim now does the same.
  • Why scoped to a page that still has focus: in production reclaim runs from blurChildWindowPage, which blurs the page first. If focus moves away during a grant (the model moved on to a terminal), the keys still come back to the cmux window. A new test covers that case.

Changelog

  • Fixed: ChildPageFocus.reclaim no longer hands the keys back to the cmux window in the middle of a grant to a page that still has focus.

Tests

The command, on the fleet:

cmux-ci run --class light --script scripts/ci/package-test-lane.sh --ref <sha> --arg=suite --arg=Packages/macOS/CmuxNext --arg=ChildPageFocusKeyChangeTests --env CMUX_CI_XCODE_APP=/Applications/Xcode_26.6.app
  • aReapplyInsideTheGrantDoesNotGrantAgain(): the existing test, red on the base.
  • reclaimInsideTheGrantAfterThePageBlurredTakesTheKeysBack(): new; it guards the scoping.

Observed / Not run

Results for the command above at cc9622ea43e0c (tests only, expected red) and 6b40dd68f1c58 (fix, expected green) are added in a comment below.

No capture: there is no visible UI change; the fix only decides which window has the keys.

🤖 Generated with Claude Code

austinywang and others added 2 commits October 26, 2026 14:52
…eys back once the page blurred

aReapplyInsideTheGrantDoesNotGrantAgain (red on feat-cmux-next since #24913)
covers the other half: a reclaim inside the grant while the page still has focus.

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

The grant's own key change (the page window's didBecomeKey) could re-apply the
model, and its reclaim gave the keys straight back to the cmux window: two key
changes, and the page stayed focused with no keys. keysReturned already treats
key changes inside a grant as the grant's own; reclaim now does too, while the
granted page still has focus. After a blur inside the grant it still reclaims.

Fixes aReapplyInsideTheGrantDoesNotGrantAgain, red on feat-cmux-next since
#24913 and surfaced by
#25222 swift test run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@austinywang austinywang added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 26, 2026
@cursor

cursor Bot commented Oct 26, 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.

@teamleaderleo

Copy link
Copy Markdown
Collaborator

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

@austinywang

Copy link
Copy Markdown
Contributor Author

Review and test results on head 6b40dd68f1c5817ebb1796924d832056093587a5

Fleet suite runs (ChildPageFocusKeyChangeTests, Xcode 26.6, AWS Macs):

commit contents result
cc9622ea43e0c tests only ✘ aReapplyInsideTheGrantDoesNotGrantAgain(): (world.keys.changes → ["pageB", "cmux"]) == ["pageB"] (step a2f9a34a19f36acb8e279d77, aws-m4pro-6)
6b40dd68f1c58 fix ✔ 11 of 11 tests passed (step 5ebb2fa285f5ff65856e0f95, aws-m4pro-3)

Correctness review (subagent): APPROVE, no blocking findings.

  • The guard can't leave the keys on a page window when the target is elsewhere. Every cmux-window target goes through blurChildWindowPage(), which calls pageFocus.blurPage() before reclaim, so granting && page != nil is false there. A page target calls focus(other) and never reclaims. reclaimKeyAfterActivation returns early for page targets. DevTools paths set page = nil.
  • Finding 1 (minor, existing before this PR): focus(_:) uses defer { granting = false } rather than restoring the previous value. A nested grant therefore clears the flag while the outer grant is still running. No production caller reaches reclaim in that window. Resolution: leaving it as a follow-up note (restore the previous value, and add a nested-grant test). It is out of scope for this red fix.
  • Finding 2 (low): no production path is known to call reclaim inside a grant without blurring first, so this fix is defensive. Resolution: I corrected the PR body so it no longer claims a reproduced runtime bug.
  • Finding 3: the new test passes with or without the guard. It guards against the guard becoming too broad (for example, dropping page != nil). It is not a red-to-green test. Resolution: this is intended and noted.

PR check reds, not from this change:

  • link: a GitHub API rate limit (HTTP 403 when posting a comment).
  • post cmux-next/fast-gates: "no head or base sha to report", when the reporting job ran.

Dev build: cmux-ci job 0b6bc91559df084a6331c460, tag 25222-reclaim-inside-grant-fix, at this head.

There is no visible UI change, so no visual review was done.

@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 26, 2026
@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

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

Review baf8653: 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=pr25689 -f ref=next-reclaim-inside-grant -f platform=macos -f backend_mode=local

web and size checks: green

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (claude-subagent correctness review): SHIP at 6b40dd6

Findings and resolution: #25689 (comment)

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (claude-subagent correctness re-review): SHIP at 4e53838

The head is two plain merges of feat-cmux-next (by Leo) on top of the reviewed 6b40dd68f1c5817ebb1796924d832056093587a5. Re-reviewed on the exact head:

  • The net diff against the merge base (git diff 4e53838977461a8e5db9c841590df359048e8f50^2 4e53838977461a8e5db9c841590df359048e8f50) is still only ChildPageFocus.swift and ChildPageFocusKeyChangeTests.swift, byte-identical to 6b40dd6.
  • reclaim, focus(_:), keysReturned, blurPage and the caller in FocusEffectApplier are unchanged. The focus-adjacent changes that came in from the base (NoActivateKeyboardGuard, WindowActivation startsBehind, AppDelegate endRelaunchBehind, WindowManager+FullScreen) add no reclaim, page-focus or Chromium-window makeKey path. While a relaunch refuses activation, reclaim already returns early, because no cmux window is key.
  • No compile risk: every symbol the change and its test use is unchanged at the head.

No findings. The earlier findings and their resolution are at #25689 (comment).

@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 26, 2026
@blacksmith-sh

blacksmith-sh Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

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

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

@austinywang

Copy link
Copy Markdown
Contributor Author

Review (claude-subagent correctness re-review): SHIP at 169390e

Re-review on the exact head after the third feat-cmux-next merge (parents 4e53838 and 4b321c4). The reviewer read the diffs only and did not compile anything.

  • The net PR diff against the base parent is unchanged: two files, +19/−2. These are the !(granting && page != nil) guard in ChildPageFocus.reclaim and the test reclaimInsideTheGrantAfterThePageBlurredTakesTheKeysBack. The existing test aReapplyInsideTheGrantDoesNotGrantAgain still relies on the guard.
  • None of the 106 base files merged since 4e53838 touch CmuxNextFocusUI, CmuxNextApp/Focus, the CEF page-window code or any other makeKey path. The window changes nearby (windowDidResize/windowDidChangeScreen, placeLaunchWindow, chromiumWindowHoles) only save geometry or only read.
  • Every reclaim caller at the head comes through blurChildWindowPage(), which blurs the page first. The one other caller, reclaimKeyAfterActivation(), returns early for page targets. The merge changed none of the test helpers.

Findings:

  • Low / theoretical, no action needed (FocusEffectApplier.swift:262). Suppose didBecomeActive fires inside a grant while the target has already moved to a non-page target that has not been applied yet. reclaimKeyAfterActivation is then skipped. The keys don't get stuck, because the next apply of that target goes through blurChildWindowPage() after the grant ends.

Earlier findings and how they were resolved: #25689 (comment). Previous receipt (superseded by this head): #25689 (comment)

@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

Copy link
Copy Markdown
Contributor Author

Review (claude-subagent correctness re-review): SHIP at baf8653

This re-review covers the exact head after Leo's fourth feat-cmux-next merge. The parents are 169390e and 1aa243d. The reviewer read the code only and did not compile anything.

  • The PR's net diff is unchanged: the same two files, +19/−2. The guard !(granting && page != nil) is still correct. Outside a grant, and after the page has blurred, reclaim behaves as before. aReapplyInsideTheGrantDoesNotGrantAgain still pins the guard itself.
  • The 337 merged base files change zero lines in CmuxNextFocusUI/ or in the test file. None of them touch the test helpers, KeyWindowSwitching, FocusEffectApplier or any reclaim caller. The CEF files that changed only alter how long the runtime object is kept alive.

Findings:

  1. Note, no change made. reclaimInsideTheGrantAfterThePageBlurredTakesTheKeysBack also passes without the guard, so it doesn't catch a regression of the fix. It pins the opposite edge, that a blurred page inside a grant still gets the keys back. That is the case the first, over-broad version of this fix broke: the fleet run on cc9622e failed with ["pageB","cmux"] == ["pageB"]. The regression test for the guard is the existing aReapplyInsideTheGrantDoesNotGrantAgain.
  2. Low, informational, and older than this PR (ChildPageFocus.swift:55-56). A nested focus(other) inside a running grant lets the inner defer clear granting before the outer grant ends. No production path does this today. I'm leaving it as is because it is outside this PR's scope.

Earlier findings: #25689 (comment). This receipt supersedes #25689 (comment).

@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

Copy link
Copy Markdown
Contributor Author

Review (claude-subagent correctness re-review): SHIP at f8eb55e

Re-review on the new head after the fifth plain merge of feat-cmux-next (second parent 856aacf4dcbfa7022e4513d006087ff5d9d2308e). The net diff against the base is still the same two files.

  • Guard is correct. The !(granting && page != nil) guard in ChildPageFocus.reclaim(into:when:) blocks a reclaim only while a page is still focused inside its grant. aReapplyInsideTheGrantDoesNotGrantAgain pins that case. A page that blurs inside the grant (page == nil) still hands the keys back, which reclaimInsideTheGrantAfterThePageBlurredTakesTheKeysBack covers with ["pageB","cmux"]. Outside a grant nothing changes, because granting is reset by defer.
  • Merge doesn't touch the focus path. Between baf8653c718f7c7e0ce7c4a3af57c18042dfdffa and this head, nothing changed in CmuxNextFocusUI, CmuxNextApp/Focus (FocusEffectApplier.reclaimKeyFromPageWindow), the test file, or the mock browser helpers it uses.
  • One merged CEF change, unrelated to focus. It removes the crashed-tab reloadWhenShown. No focus or key-window code is involved.
  • No findings, so nothing to resolve.

Earlier reviews on previous heads: #25689 (comment) and #25689 (comment).

Known base red: release-notary / Guard status fails on feat-cmux-next too, because release-notes/next/highlights/cloud-start-session-chat-link.md has no docs: line. #25839 fixes it.

@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 merged commit 002c20e into feat-cmux-next Oct 27, 2026
145 of 151 checks passed
@austinywang
austinywang deleted the next-reclaim-inside-grant branch October 27, 2026 22:21
@github-actions

Copy link
Copy Markdown
Contributor

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/40594175819 at 002c20e failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at 5c2a75b. This pull request is one of 1 merged since: #25689.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 26341467 Deployed Oct 27, 2026 by teamleaderleo via wait for same-key tree publisher #63261
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.

2 participants