A fake WKNavigation was killing the test host, hiding a whole suite

[ejc3]

What was happening

BrowserDiscardRestorePolicyCancelTests logged that it started and then produced no verdict at all. That is a dead test host, not a failing test, and it is worse than a red suite: the host takes down every suite batched with it, so any batched measurement including this one was untrustworthy. Reading the eight tests one by one finds nothing wrong with them, which is exactly what you would expect — the suite does not fail, it dies.

Cause, and a second one behind it

The suite constructs WKNavigation() directly. WebKit builds that object's embedded C++ API::Navigation itself, so a bare instance carries unconstructed storage. Allocating one is harmless; releasing it is not. Reproduced outside the test bundle, deterministically: EXC_BREAKPOINT inside CFRetain from -[WKNavigation dealloc] with WebKit initialised, and SIGSEGV through WebCoreObjCScheduleDeallocateOnMainRunLoop from the same dealloc without it. The earliest death needs no window: the fake is stored as the pending restore navigation, the next call clears that reference, and the release traps mid-assertion. WKNavigation() appears nowhere else in the repo.

That also explains why no output survives: with stdout on a pipe, the crash discards the buffer, so even the line printed immediately before it never reaches the log.

The bookkeeping under test only ever compares these by identity, so the fakes are now minted through a helper that keeps them retained for the run.

Fixing that got the suite far enough to run and pass several tests, which exposed a second killer in the same file: one test builds an NSWindow, holds it through ARC, and closes it in a defer without disabling AppKit's close-time release, so the last retain goes away underneath live references. About forty other closing sites in cmuxTests already guard against this, and the same defect was separately confirmed in another suite.

Verification

Measured on a macOS builder, the suite run alone in both arms:

arm	result
before	started, then no suite verdict at all
this branch	passing, 8 tests

No assertion changed.

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

---

Summary by cubic

Prevents xctest host crashes by replacing fake WKNavigation() with a retained stub and disabling NSWindow close-time release in one test. The BrowserDiscardRestorePolicyCancelTests suite now runs and passes instead of killing the host.

• Bug Fixes
  • Added BrowserDiscardRestoreNavigationStub.make() to create and retain WKNavigation instances for the test run.
  • Replaced direct WKNavigation() calls with the stub to avoid WebKit dealloc traps.
  • Set window.isReleasedWhenClosed = false in the window-close test to prevent double release.
  • Result: suite passes (8 tests); no more dead host or missing logs.

^{Written for commit 34c2460. Summary will update on new commits.}

Summary by CodeRabbit

• Tests
  • Improved browser discard and restore test stability.
  • Added safer handling for navigation state during stale and current navigation scenarios.
  • Prevented test crashes when windows remain open during deferred closing.

[coderabbitai]

📝 Walkthrough

Walkthrough

Browser discard/restore tests now retain stubbed WKNavigation instances and explicitly preserve a test-owned window during deferred close in the stale insecure-HTTP prompt scenario.

Changes

Browser discard/restore test stability

Layer / File(s)	Summary
Retained navigation and window ownership cmuxTests/BrowserDiscardRestoreHealPredicateTests.swift	Adds a retained navigation stub for discard/restore scenarios, updates stale and current navigation setup, and disables automatic window release for one deferred-close test.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

• manaflow-ai/cmux#8832: Addresses test-host window lifecycle behavior involving NSWindow release during test teardown.

Suggested reviewers: austinywang

🚥 Pre-merge checks | ✅ 25

✅ Passed checks (25 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation	✅ Passed	Only a test file changed, and the added @MainActor stub/window tweak is confined to tests, so no production actor-isolation debt was introduced.
Cmux Swift Blocking Runtime	✅ Passed	The diff is test-only and adds no semaphores, waits, sleeps, sync calls, or locks; it only retains WKNavigation objects and disables NSWindow close-time release.
Cmux Browser Automation Off-Main	✅ Passed	HEAD only changed a test file; no browser automation routing or policy files were modified, so the off-main WebKit wait rule isn’t implicated.
Cmux Expensive Synchronous Load	✅ Passed	Test-only diff adds a retained WKNavigation stub and window close guard; it does not add or move any agent-history load or other expensive sync path.
Cmux Cache Substitution Correctness	✅ Passed	PASS: The diff is test-only; it retains WKNavigation objects for identity checks and doesn't replace any authoritative read with a cache in a persistence/history/undo path.
Cmux No Hacky Sleeps	✅ Passed	The change is Swift test-only scaffolding; no sleep/delay/timer/polling was introduced, and the rule explicitly excludes Swift.
Cmux Algorithmic Complexity	✅ Passed	Only a test file changed; the new helper just appends a few retained WKNavigation instances and adds no scans or hot-path work.
Cmux Swift Concurrency	✅ Passed	Diff only adds an AppKit window-release guard and retained WKNavigation test stub; no new DispatchQueue, Combine, completion-handler, or fire-and-forget Task patterns.
Cmux Swift @Concurrent	✅ Passed	PASS — the only Swift change is a synchronous NSWindow close-release guard; no new async functions, @concurrent annotations, or actor hops were introduced.
Cmux Swift Package Boundaries	✅ Passed	Diff is confined to cmuxTests and adds test-only retained-navigation/window-close fixture code, which the rule explicitly allows.
Cmux Swiftpm Lockfiles	✅ Passed	Only cmuxTests/BrowserDiscardRestoreHealPredicateTests.swift changed; no Package.swift, .gitignore, workflow, xcodeproj, or Package.resolved files were modified.
Cmux Swift Logging	✅ Passed	The only changed Swift file is a test file and adds no print/debugPrint/dump/NSLog/Logger changes or sensitive logging.
Cmux User-Facing Error Privacy	✅ Passed	Change is confined to cmuxTests; the rule explicitly allows tests, and added text is internal test commentary, not user-facing copy.
Cmux Full Internationalization	✅ Passed	Only a test Swift file changed, with one test-only AppKit guard and comments; no user-facing localization/catalog/web text was added or modified.
Cmux Swiftui State Layout	✅ Passed	PASS: The diff only touches a test-only AppKit/WebKit suite; no SwiftUI views, state wrappers, GeometryReader, or render-time state writes are introduced.
Cmux Architecture Rethink	✅ Passed	PASS: The change is a test-only lifetime guard (retained WKNavigation stub) plus a standard AppKit window-release override; no sleeps, polling, duplicate wiring, or lifecycle split.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	Only test fixtures changed in cmuxTests; no production window identifiers or close-shortcut routing were added or modified, and test-only windows are allowed.
Cmux Source Artifacts	✅ Passed	Only changed path is a hand-written Swift test file; no logs, caches, build output, or scratch artifacts appear in the diff.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	Only cmuxTests/BrowserDiscardRestoreHealPredicateTests.swift changed; no Sources/ production file added any test/debug seam.
Cmux No Ambient Global State	✅ Passed	PASS: only a test file changed; the rule targets production Swift, and the private stub/helper stays local to cmuxTests with no exported ambient API.
Title check	✅ Passed	The title clearly matches the main fix: retaining fake WKNavigation objects to stop the test host from crashing.
Description check	✅ Passed	The description covers what changed, why, and verification with concrete results; only non-critical template sections are missing.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[ejc3]

Measured on a combined build at c59bbdf (current main + the build fix in #8857 + this):

BrowserDiscardRestorePolicyCancelTests   Test run with 8 tests in 1 suite passed
                                         ** TEST SUCCEEDED **   restarts=0  crashlines=0

On unfixed main the same suite gives 1 host restart, 3 crash lines and zero tests executed — no verdict at all, which is why it never showed up as a failure.

Also correcting the record on how this sat open. I re-derived the cause later in the day by running grep WKNavigation cmuxTests/BrowserDiscardRestorePolicyCancelTests.swift, got "no such file", and concluded the constructions lived in a sibling suite and the segfault was unexplained. Both suites live in BrowserDiscardRestoreHealPredicateTests.swift and all five bare constructions are inside the crashing one. A suite name is not a file name here, and a filename-derived search is not evidence that retracts a diagnosis.

[greptile-apps]

Greptile Summary

This PR fixes two distinct xctest host crashes in BrowserDiscardRestorePolicyCancelTests that were silently killing every suite sharing the same test host rather than producing test failures.

• Navigation stub: Introduces BrowserDiscardRestoreNavigationStub.make() to retain WKNavigation instances for the entire test run, preventing the -[WKNavigation dealloc] trap that fires when a bare WKNavigation() (carrying unconstructed C++ API::Navigation storage) is released. All four raw WKNavigation() call sites in the affected test suite are replaced.
• Window release guard: Adds window.isReleasedWhenClosed = false before the deferred window.close() in the window-close test, matching the ~70 other guarded sites in cmuxTests, so AppKit does not drop the last retain while ARC still holds references.

Confidence Score: 5/5

Safe to merge — all changes are confined to the test target, fixing crashes that previously swallowed an entire test suite without a failure verdict.

The two fixes are surgical, well-documented, and match established patterns already used dozens of times in this test file. No production code is touched, no assertions are changed, and the suite now passes all eight tests where it previously killed the host.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
cmuxTests/BrowserDiscardRestoreHealPredicateTests.swift	Adds WKNavigation retention stub and NSWindow isReleasedWhenClosed guard to prevent two distinct test-host crashes in BrowserDiscardRestorePolicyCancelTests; all changes are test-only and well-documented.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Test calls BrowserDiscardRestoreNavigationStub.make()"] --> B["WKNavigation() allocated"]
    B --> C["Appended to @MainActor static retained array"]
    C --> D["WKNavigation reference returned to test"]
    D --> E["Test uses navigation by identity only"]
    E --> F["Test completes — retained array keeps object alive"]
    F --> G["No dealloc during test run → host survives"]

    H["NSWindow created in test"] --> I["isReleasedWhenClosed = false set"]
    I --> J["defer: window.close() runs"]
    J --> K["AppKit does not release — ARC still holds ref"]
    K --> L["No double-free → host survives"]

Loading

_{Reviews (2): Last reviewed commit: "cmuxTests: the same suite closes a test-..." | Re-trigger Greptile}

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.