Skip to content

C11-332: un-quarantine the host test classes that guard 1.0 fixes - #588

Merged
BenevolentFutures merged 6 commits into
mainfrom
followup/C11-332-unquarantine
Oct 6, 2026
Merged

BenevolentFutures merged 6 commits into
mainfrom
followup/C11-332-unquarantine

Conversation

@BenevolentFutures

@BenevolentFutures BenevolentFutures commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

What

The hourly host gate (ci-hourly.yml, HOST_TEST_QUARANTINE) skipped seven whole classes from C11-109. Several of them hold the regression tests for c11 1.0 fixes, so CI never ran those tests. This PR removes all seven class exclusions. Every remaining exclusion is a single method with a one-line reason. Tests, CI and notes only: no Sources/ change.

Classes

Class Before Now 1.0 fixes it guards
GhosttyConfigTests class skipped; 29/29 already pass Un-quarantined C11-311 B083 (divider darkening)
NotificationBurstCoalescerTests class skipped; passes Un-quarantined none
TerminalNotificationDirectInteractionTests class skipped; passes Un-quarantined none
WorkspaceManagerReopenClosedBrowserFocusTests class skipped; passes Un-quarantined none
BrowserDeveloperToolsVisibilityPersistenceTests class skipped; 8 of 17 fail Un-quarantined, 8 stale tests repaired C11-287: the two inspector-replacement tests in this class
BrowserPanelHostContainerViewTests class skipped; 7 of 14 fail Un-quarantined, 7 stale fixtures repaired none
AppDelegateShortcutRoutingTests class skipped; 10 tests fail (the known 17 assertions) Un-quarantined, in its own host invocation. 8 repaired, 2 method-quarantined for a product bug, 2 new routing guards C11-250 (close guard), C11-266 (quick view), B050 (quit flush)
C11-253 method exclusions (8) grouped comments Still quarantined, one-line reason each unchanged

Repairs (all stale tests, not product bugs)

  • DevTools persistence.
    • The fake inspector's attach() counted a show, which gave the "2 != 1" failures. Production attaches before the first show.
    • Several tests acted inside the 0.15 s show transition. They now wait for it to settle.
    • One fixture leaked a detached "Web Inspector test fixture" window into the next test.
  • Browser host container. Manual divider routing applies only after promotion into HostedInspectorSideDockContainerView (upstream 06c5cac4df). The fixtures now promote through the production path, using a shared helper.
  • AppDelegate shortcut routing.
    • Fixtures assumed one pane per new workspace. The default 2x2 grid and the welcome quad are now turned off in setUp.
    • Cmd+N and a remapped New Workspace shortcut now open the New Workspace picker instead of creating on keyDown. The tests assert the picker and the retargeted active window.
    • Close tests held strong window references, so they now check that the window became invisible and lost its context.
    • The real hidden command-palette overlay shadowed the fixture's.
    • Cmd+W aimed at a non-main window intentionally passes through. The test now covers both phases.
    • A fresh shell needs a prompt-idle report to close without the confirmation, so the tests send one.
    • A Cmd+D active-window assertion raced the menu's key-window resync.

Still quarantined: product bug

testAddWorkspaceInPreferredMainWindowPrunesOrphanedContextWithoutLiveWindow and testCustomCmdTNewWorkspacePrunesOrphanedContextWithoutLiveWindow are restored unchanged and skipped by method. Workspace creation never prunes an orphaned main-window context. Every candidate in preferredMainWindowContextForWorkspaceCreation requires a resolved window, so the discard after a failed resolution (AppDelegate.swift, addWorkspaceInPreferredMainWindow) is unreachable. The orphan stays in mainWindowContexts and in window summaries. Two new tests guard the reachable half: an orphan never receives a workspace.

Host isolation

In one host process, AppDelegateShortcutRoutingTests made GhosttySurfaceOverlayTests fail. A probe after the class found:

  • 70 closed main windows still alive;
  • 84 registered terminal surfaces;
  • a backlogged main queue: the first main.async did not fire within 50 ms.

The class's tearDown now closes what each test opened and frees its surfaces: 149 of 149 terminals freed, where it freed none before. The closed windows remain reachable from outside the test code, so the gate runs this class in a second xcodebuild invocation. Whether production retains closed main windows is unverified; it is noted in notes/c11-253-host-test-triage.md as a one-step QA check.

Evidence (Atlas, scripts/remote-build.sh --mode test)

  • Baseline at main 6e2c1dc47f (invocation b2dae594): 138 tests in the seven classes, 50 failed assertions.
  • Control at main 6e2c1dc47f, full gate with main's quarantine (b44c471d): 1595 tests, 0 failures, both passes.
  • Seven classes at the repair head (78768165): 138/138, ** TEST SUCCEEDED **, both passes.
  • Full hourly host gate at this head 2f9513a67e (clean): first invocation ffe0294f, 1665 tests, 0 failures, both passes. Second invocation, AppDelegate class only, ffa5a2f0: 68 tests, 0 failures, both passes. Every 1.0 guard named above executed and passed. The two quarantined orphan methods did not execute.

After merging main (C11-337 R8 panel rename, #587), merge head 859d3210f2 (clean), same two hourly selections: c11Tests minus AppDelegate 079274d5, 1676 tests, 0 failures, both passes; AppDelegateShortcutRoutingTests alone cb1764ee, 68 tests, 0 failures, both passes. The 25 repaired methods that the panel-rename attribution lists among the 31 quarantined failures pass there; the 6 C11-253 methods stay quarantined.

The merge changed only names: TabLifecycleTests -> PanelLifecycleTests in the skip list, BrowserTabTests.swift -> BrowserPanelTests.swift with BrowserTab* -> BrowserPanel*, TabAttention* -> PanelAttention*, and updateTabShellActivityState -> updatePanelShellActivityState.

GitHub-hosted hourly CI has not run since 06:07Z today because the account's billing is blocking macOS runners. The Atlas runs above mirror the hourly step's selection exactly.

🤖 Generated with Claude Code

Atin and others added 5 commits October 5, 2026 12:48
Retire all seven C11-109 class-level exclusions from the hourly host gate.
Four classes already passed; three failed on stale fixtures, now repaired
in test code only:

- AppDelegateShortcutRoutingTests: fixtures assumed one pane per new
  workspace (default grid/welcome quad), Cmd+N creating on keyDown (it opens
  the picker), visible closes behind strong window refs, a fake palette
  overlay shadowed by the real one, and an idle shell without a prompt-idle
  report. Guards C11-250, C11-266 and B050.
- BrowserDeveloperToolsVisibilityPersistenceTests: fake attach() counted a
  show, tests acted inside the show transition, and one fixture leaked a
  detached-inspector window. Guards C11-287.
- BrowserTabHostContainerViewTests: fixtures now promote into the side-dock
  container that manual divider routing requires.

Two orphan-prune methods stay quarantined for a product bug: workspace
creation never prunes an orphaned window context. Every remaining exclusion
is a single method with a one-line reason. No Sources/ change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each test opened main windows whose Ghostty terminals stayed alive after
close (176 login shells started, none freed), loading the main thread so
GhosttySurfaceOverlayTests missed its deferred overlay mounts. tearDown
now closes windows the test added, frees surfaces it created and restores
the active window pointers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closed main windows from this class stay alive in the test host (70 after
the class, with a main-queue backlog), which starves later classes'
deferred-mount waits. The hourly host gate now runs the class in a second
xcodebuild invocation after the rest of c11Tests.

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

The overlay mounts on a main-queue hop; a fixed 50 ms wait failed under
the larger un-quarantined host suite. Wait (bounded) for the mount.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the drawbridge:review Drawbridge: routed to maintainer review label Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Deep review — C11-332: un-quarantine the host test classes that guard 1.0 fixes

Summary

This removes all seven class-level exclusions from the hourly host gate (ci-hourly.yml, HOST_TEST_QUARANTINE), repairs the stale fixtures that made three of those classes fail, and keeps only single-method exclusions — each with a one-line reason. It's test + CI + notes only; there is no Sources/ change. The net effect is that the regression guards for several c11 1.0 fixes (C11-311 B083, C11-250, C11-266, B050, the C11-287 inspector-replacement tests) now actually execute in CI instead of being skipped wholesale. The write-up, the baseline/control/repair-head evidence, and the honest "still a product bug" disclosure are exactly what makes this reviewable from the notification alone — thank you for that.

Alignment

Squarely in scope. Restoring real CI coverage for shipped 1.0 fixes is core to a publicly-released product with live users, and the change respects every house rule: no tenant-config writes, no source changes, no skill surface touched (none applies — the CLI/socket/surface model is untouched). The repairs are all fixture-side (stale attach() counting a show, acting inside the 0.15 s DevTools transition, assuming one pane per workspace before the 2x2 grid / welcome quad, strong window refs masking closes, promoting through the real side-dock path). Tests continue to verify runtime behavior through executable paths (window routing, hitTest, divider drags) — no source-grep / plist / signature assertions, so the test-quality policy holds.

I confirmed against the trusted checkout: every production symbol the new/changed tests call exists and is correctly DEBUG-gated where the test guards it — releaseSurfaceForTesting (GhosttyTerminalView.swift:4831), debugHasSearchOverlay (:9169), debugUnregisterMainWindow (AppDelegate.swift:6527), ensureLocalInlineSlotView / promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded / normalizeHostedInspectorLayoutIfNeeded (BrowserTabView.swift:4862/5019/5073), CreateWorkspaceSheet, WelcomeSettings/DefaultGridSettings. I also verified the "product bug" claim: preferredMainWindowContextForWorkspaceCreation (AppDelegate.swift:7469) only ever returns a context whose window resolves (key/main/ordered/fallback each gate on resolvedWindow != nil), so the discardOrphanedMainWindowContext branch in addWorkspaceInPreferredMainWindow (:7296) is reachable only through the event-addressed path — the two quarantined non-event orphan tests are genuinely unreachable as written. Leaving them method-quarantined with new reachable-half guards is the right call for a test/CI PR.

Findings

No correctness, security, or scope defects in the changed code. The items below are minor/nit polish on the CI script and one follow-up to keep on the radar.

  1. minor — host-window retention is a real, unverified product question. The reason AppDelegate tests need their own xcodebuild invocation is that closed main windows stay alive in NSApp.windows (70 observed) with a backlogged main queue — the same shape as the autoreleasepool/leak pitfalls this repo has hit before (C11-211). The notes correctly flag it as "unverified… worth one check" (notes/c11-253-host-test-triage.md). That's the right disclosure, but it means a possible production leak is now documented and parked. Recommend filing/keeping the one-step QA check (open+close a second window, confirm the window and its terminals deallocate) as an explicit follow-up rather than letting it live only in a notes paragraph. Not blocking for this PR.

  2. nit — redundant AppDelegate skip in the first invocation (ci-hourly.yml, the host_tests -only-testing:c11Tests -skip-testing:c11Tests/AppDelegateShortcutRoutingTests call). SKIP_ARGS already contains -skip-testing for the two AppDelegateShortcutRoutingTests methods, which the inline whole-class skip then subsumes. Harmless to xcodebuild, but a one-line comment that the method skips are there for the second invocation would save a future reader the double-take.

  3. nit — set -euo pipefail short-circuits the AppDelegate invocation on a first-invocation failure. If the first host_tests call fails, the step exits before the second runs, so a failing run gives no signal on AppDelegateShortcutRoutingTests. That's acceptable for a gate (the job fails either way), but if you ever want both results on a red run, collect the two exit codes and fail at the end instead.

Recommendation

Merge as-is. It's a test/CI/notes-only change that restores genuine CI coverage of shipped 1.0 regression guards, every repaired fixture tracks real production behavior, the one unfixed product bug is disclosed and appropriately quarantined, and the Atlas evidence mirrors the exact gate selection. Worth confirming the hourly host gate goes green on GitHub-hosted CI once the macOS-runner billing block clears (the PR modifies that gate, and mergeable_state is currently unstable), and keeping finding #1's QA check on the board.


🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.

Resolution: keep C11-332's quarantine list with R8's PanelLifecycleTests
name; BrowserTabTests.swift changes carried into BrowserPanelTests.swift
with R8's BrowserTab* -> BrowserPanel* names; the flag-banner wait uses
PanelAttentionIndex/PanelAttentionSnapshot; updateTabShellActivityState ->
updatePanelShellActivityState in the routing tests.

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

Copy link
Copy Markdown
Contributor Author

Cairn: exact-head attestation for 859d321. The merge of main (C11-337 panel rename) changed only names (TabLifecycleTests to PanelLifecycleTests, BrowserTab* to BrowserPanel*, PanelAttention*, updatePanelShellActivityState); the effective diff against main matches the Astra-reviewed ff6b4ad otherwise. Atlas at 859d321: 1676 + 68 host tests, 0 failures. Merging.

@BenevolentFutures
BenevolentFutures marked this pull request as ready for review October 6, 2026 21:14
@BenevolentFutures
BenevolentFutures merged commit 3fdb7e2 into main Oct 6, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drawbridge:review Drawbridge: routed to maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant