Skip to content

test: target local Undo at the editable responder and name the refusing focus gate - #13736

Closed
teamleaderleo wants to merge 1 commit into
manaflow-ai:fix/app-host-greenfrom
teamleaderleo:fix/app-host-focus-responder
Closed

teamleaderleo wants to merge 1 commit into
manaflow-ai:fix/app-host-greenfrom
teamleaderleo:fix/app-host-focus-responder

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Reconciled onto fix/app-host-green at 938dbab. Two things landed underneath this branch and the scope shrank accordingly.

Dropped: the TerminalNotificationDirectInteractionTests portal owner. a682c55 ("test: give standalone terminal fixtures a live portal authority") is the same fix for the same three tests — testKeyDownRecoveryDoesNotReplayFocusAfterResponderMovesAway, testVisibilityRestoreRefreshesSurfaceWhileTerminalIsInactive, testDirectFirstResponderFocusRefreshesCursorStateAfterForeignResponder — reaching the same Workspace.portalRenderingEnabled(for:) authority through a private makeLivePortalWorkspace() fixture instead of the shared TerminalPortalTestWorkspace. Two fixtures granting the same authority to the same three surfaces is one too many, and the base's is the one that shipped. This branch no longer touches cmuxTests/TerminalAndGhosttyTests.swift at all; that was the only conflicting file.

Kept: WindowKeyDownReplayGuardTests.terminalHostedEditableResponderKeepsLocalUndo. Its Cmd+Z menu item now targets the editable responder. A nil-targeted menu action resolves through NSApp.keyWindow, which is nil while the headless host is inactive, so performKeyEquivalent returned true while undo: reached no responder. The contract under test is cmux's routing — the menu handles Cmd+Z and the terminal sees no menu miss — not AppKit target resolution. The sibling terminalDeclinedUndoCommandFallsBackToTerminalKeyDownWithoutLocalUndo keeps its nil target and its undoCallCount == 0 expectation, so the negative case is unchanged. Nothing on the base touches this file.

Kept, with the premise restated: the focus-gate diagnostic. The original description built on #13672's key-status pin, and #13759's own reconciliation dropped that pin (a MainWindowKeyStatusPin swizzle of NSWindow.isKeyWindow) in favour of 87a7016's NSApp.activate(ignoringOtherApps:). That reads like it supersedes this work. It does not, for these three tests:

  • 87a7016's remedy lives inside AppDelegate.focusTerminalForTesting. None of automaticApplyDoesNotBypassHiddenTinyFirstResponderDeferral, findTerminalRestorePreservesHiddenTinyFirstResponderDeferral or testTerminalFirstResponderFeedbackPreservesActiveFocusTransaction calls that helper — they build a KeyStatusTestWindow directly, so key status was already pinned true for them before and after.
  • applyFirstResponderIfNeeded gates on isActive, surfaceView.isVisibleInUI, hidden/geometry, window.isKeyWindow, the focus target match, allowsTerminalKeyboardFocus and isCommandPaletteEffectivelyVisible. App activation bears on none of them; NSApp.isActive appears in that file only as a debugRenderStats field.
  • No commit on the base between 5a92d7f and 938dbab names any of the three.

So the cause is still unnamed and the assertions still need to say which gate refused. terminalFocusGateSummary now also prints windowIsKey and appActive, so the next run says whether key status was real or only KeyStatusTestWindow's override — the one place the base's activation change could plausibly change the answer.

Verified: scripts/check-test-determinism.py (0 findings), tests/test_ci_pbxproj_test_wiring.sh (18/18), scripts/ci/validate_test_execution_registry.py (223 tests), and swiftc -parse on all four changed files. No pbxproj change, so no normalization needed. Not verified: no app-host build or test run was performed locally — a full xcodebuild test of the cmux app target is not feasible here, so the claim that these tests now pass rests on CI, not on anything I ran.

🤖 Generated with Claude Code


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

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c2cf6cb3-605f-4299-911f-acd4189bbdd2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Triage note — no overlap found with any other open PR, but this branch is currently unmergeable against its own base (mergeable: false, dirty), so it needs a rebase onto fix/app-host-green before #13643 can pick it up.

Checked for duplicates because #13643, #13736, #13744, #13427 and #13414 all read as "app-host test wiring" from the outside. They are not competing:

One cross-reference worth having: #13744 names testMinimalModeToggleDoesNotReevaluateChromeHeavyBodies as sharing a prime suspect with its own visibilityToggleKeepsAppKitTableContainerMounted failure — the late first working-directory report arriving mid-measurement — and you list the same test as "likely shares its cause with the sidebar double-build: both started when 0929dc2 put the test window on screen". You have independently converged on the same commit and the same test from two branches. Neither of you is changing that test yet, which is the right call, but whoever gets CI signal back first should say so on the other PR rather than both waiting.

Also relevant to #13643 landing at all: macos / macOS status fails with invalid route compile_admitted='' on full-suite runs regardless of shard health (#13727). Even with every shard green, the macOS gate stays red until that lands.

…ng focus gate

Reconciled onto fix/app-host-green after a682c55 and manaflow-ai#13759 landed.

Dropped: the `TerminalNotificationDirectInteractionTests` portal-owner change.
a682c55 ("test: give standalone terminal fixtures a live portal authority")
is the same fix for the same three tests, through a `makeLivePortalWorkspace()`
fixture instead of `TerminalPortalTestWorkspace`. Two fixtures granting the same
portal authority to the same three surfaces is one too many, so this branch no
longer touches cmuxTests/TerminalAndGhosttyTests.swift at all.

Kept: `terminalHostedEditableResponderKeepsLocalUndo` targets its Cmd+Z menu
item at the editable responder. A nil-targeted menu action resolves through
`NSApp.keyWindow`, which is nil while the headless host is inactive, so
`performKeyEquivalent` returned true while `undo:` reached no responder. The
contract under test is cmux's routing -- the menu handles Cmd+Z and the terminal
sees no menu miss -- not AppKit target resolution. The sibling
`terminalDeclinedUndoCommandFallsBackToTerminalKeyDownWithoutLocalUndo` keeps
its nil target and its `undoCallCount == 0` expectation.

Kept: the focus-gate diagnostic. manaflow-ai#13759's reconciliation dropped its
`MainWindowKeyStatusPin` for 87a7016's `NSApp.activate(ignoringOtherApps:)`,
but that remedy lives inside `focusTerminalForTesting`, and none of
`automaticApplyDoesNotBypassHiddenTinyFirstResponderDeferral`,
`findTerminalRestorePreservesHiddenTinyFirstResponderDeferral` or
`testTerminalFirstResponderFeedbackPreservesActiveFocusTransaction` calls that
helper -- they build `KeyStatusTestWindow` directly. `applyFirstResponderIfNeeded`
gates on `isActive`, `isVisibleInUI`, hidden/geometry, `window.isKeyWindow`,
the focus target, the keyboard-focus coordinator and the command palette, and
on none of them does app activation bear. So the base has not addressed these
three, and their assertions still need to name the gate that refused. The
summary now also prints `windowIsKey` and `appActive`, so the next run says
whether key status was real or only `KeyStatusTestWindow`'s override.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@teamleaderleo
teamleaderleo force-pushed the fix/app-host-focus-responder branch from e44deeb to c711ef0 Compare September 23, 2026 00:08
@teamleaderleo teamleaderleo changed the title test: authorize direct-interaction portals, target local Undo, report focus gates test: target local Undo at the editable responder and name the refusing focus gate Sep 23, 2026
@teamleaderleo teamleaderleo added the full-ci EXPENSIVE: full macOS tests/builds; overrides selective PR routing. Not needed for normal checks. label Sep 23, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Parked — Thornquay 💠 (triage, 2026-09-23). Base fix/app-host-green (#13643) was squash-merged, so this needs retargeting to main; it then conflicts in cmuxTests/AppDelegateMainWindowTestingSupport.swift and cmuxTests/WorkspaceUnitTests.swift. Worth confirming how much scope is left after #13643 and #13759 before paying the Mac CI run.

Copy link
Copy Markdown
Collaborator Author

Closing this stale stack in favor of fresh-main replacements. The independent Cmd+Z harness fix is lifted as #13989. The hidden/tiny focus failures this branch instrumented are being repaired and re-run on the macOS 26 PR lane in #13988; that work uses the concrete appIsActive=false evidence from the prior macOS 26 run rather than carrying this branch's 286-commit-behind diagnostic layer. If #13988 exposes another focus gate, I'll add the diagnostic fresh on current main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full-ci EXPENSIVE: full macOS tests/builds; overrides selective PR routing. Not needed for normal checks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant