Skip to content

test: fail the Desktop drop fast instead of restarting the app host - #14076

Merged
teamleaderleo merged 5 commits into
mainfrom
test/bound-cloud-desktop-drop-wait
Sep 24, 2026
Merged

teamleaderleo merged 5 commits into
mainfrom
test/bound-cloud-desktop-drop-wait

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

CloudDesktopOpenFixture.drop() waits for a SurfaceCatalog notification with no deadline, and only for an exact projection count. On main's full-suite run 35862070143, shard 4's capturesClickDestination(hasRemoteView: false, menu: false) never saw that notification. The test ran into the suite's 60 s time limit, which restarted the app host. The restart marks the whole shard incomplete, so the known-failure catalog cannot tolerate it. The same suite passed in 1.2 s after the restart.

The wait now:

  • checks the count once right after the drop call, since the commit can land before the next notification;
  • accepts a count at or past the target instead of exactly equal;
  • gives up after 10 s with a named expectation failure ("the drop never committed a second Desktop projection").

A missed commit now fails as an ordinary test instead of taking the shard down with it.

Not verified: this is test-only code. It is compiled by macOS compile admission and has not been run here.

— Glitch g1 📚

🤖 Generated with Claude Code


Summary by cubic

Fixes CloudDesktopOpenFixture.drop() so a missed Desktop commit fails the test after 10 s instead of hitting the suite's 60 s time limit, which restarted the app host and marked the whole shard incomplete.

Bug Fixes

  • Checks the projection count once right after the drop, since the commit can land before the next catalog notification.
  • Accepts a count at or past the target instead of an exact match.
  • Records a normal expectation failure when the drop never commits a second Desktop projection.

Written for commit 7a6d769. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Tests
    • Desktop-drop checks now finish when the expected count is reached and fail after a 10-second timeout instead of waiting indefinitely.

CloudDesktopOpenFixture.drop() waited on a catalog notification with no
deadline and only on an exact count. On main's run 35862070143 the commit
never arrived, capturesClickDestination ran into the suite's 60 s limit,
and the resulting host restart made shard 4 incomplete for every test after
it; the retried suite then passed in 1.2 s. The wait now checks the count
right after the drop, accepts a count at or past the target, and records a
normal failure after 10 s.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@teamleaderleo teamleaderleo added the no-full-ci Records that skipping the macOS suite on a test-only diff is deliberate label Sep 24, 2026
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Advanced

Run ID: a5bb6255-59be-4d17-9bbe-053a15097037

📥 Commits

Reviewing files that changed from the base of the PR and between 00c10ad and 7a6d769.

📒 Files selected for processing (1)
  • cmuxTests/CloudDesktopOpenFixture.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 27aef7f5-8804-419a-977d-3f4e644729b9

📥 Commits

Reviewing files that changed from the base of the PR and between 02972b7 and 00c10ad.

📒 Files selected for processing (1)
  • cmuxTests/CloudDesktopOpenFixture.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The Desktop drop fixture now checks whether the projected count meets or exceeds the expected value. It also bounds the commit wait with a 10-second deadline before asserting that the drop committed.

Changes

Desktop drop confirmation

Layer / File(s) Summary
Projection check and bounded wait
cmuxTests/CloudDesktopOpenFixture.swift
The fixture checks the projected count after the drop and in the catalog notification callback. It replaces the unbounded wait with a 10-second deadline and asserts that the drop committed.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: austinywang

Merge Risk: ⚪ Minimal · up to 00c10

The Desktop drop test still detects extra projections and now fails after a bounded wait if the expected projection is not committed. No actionable merge-blocking risk is evident.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: making Desktop drop failures occur quickly instead of restarting the app host.
Description check ✅ Passed The description clearly explains the problem, the implementation, the expected behavior, and the testing status. It does not include the template's Demo Video, Review Trigger, or Checklist sections, b…
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 Cloud Persistent Session And Early Input ✅ Passed PASS: The pull request changes only cmuxTests/CloudDesktopOpenFixture.swift, replacing an unbounded Desktop projection wait with an immediate count check and a 10-second test deadline. It does not c…
Cmux Swift Actor Isolation ✅ Passed PASS: The review-scoped diff changes only cmuxTests/CloudDesktopOpenFixture.swift. The changed code is test fixture code, and it adds no production model, protocol, Sendable reference type, or UI-st…
Cmux Swift Blocking Runtime ✅ Passed PASS. The PR changes only cmuxTests/CloudDesktopOpenFixture.swift, which is test-only scaffolding. Although it adds Task.sleep(for: .seconds(10)), the rule explicitly allows deterministic sleeps i…
Cmux Browser Automation Off-Main ✅ Passed PASS. The pull request changes only cmuxTests/CloudDesktopOpenFixture.swift. The diff adds a bounded SurfaceCatalog wait and does not add or move any browser.* socket command, WebKit callback wa…
Cmux Expensive Synchronous Load ✅ Passed PASS. The authoritative diff changes only cmuxTests/CloudDesktopOpenFixture.swift, a test fixture. It adds a bounded async wait and projection-count checks in drop(), not an agent-history loader, …
Cmux Cache Substitution Correctness ✅ Passed PASS. The authoritative PR diff changes only cmuxTests/CloudDesktopOpenFixture.swift, so it is test-only and not a production Swift change. The changes add a bounded wait and adjust a test-side `Sur…
Cmux No Hacky Sleeps ✅ Passed PASS. The authoritative diff changes only cmuxTests/CloudDesktopOpenFixture.swift, which is Swift test code. The custom check applies to production non-Swift app/runtime changes. Its explicit test-o…
Cmux Algorithmic Complexity ✅ Passed PASS: The PR changes only cmuxTests/CloudDesktopOpenFixture.swift, a @testable test fixture used by CloudDesktopOpenActionTests. The change adds bounded waiting and repeats a projection count qu…
Cmux Swift Concurrency ✅ Passed PASS. The only new async construct is the test-only let deadline = Task { ... } in CloudDesktopOpenFixture.drop(). The task has a caller-owned local handle and is cancelled by `defer { deadline.ca…
Cmux Swift @Concurrent ✅ Passed PASS: The diff changes only CloudDesktopOpenFixture.drop, which remains isolated through its @MainActor class. The new Task performs only non-blocking Task.sleep and resolves a `@unchecked Sen…
Cmux Swift Package Boundaries ✅ Passed PASS. The authoritative diff changes only cmuxTests/CloudDesktopOpenFixture.swift (+12/-2). The changed code is inside CloudDesktopOpenFixture, a @MainActor test fixture imported by `CloudDeskto…
Cmux Swiftpm Lockfiles ✅ Passed PASS: The PR changes only cmuxTests/CloudDesktopOpenFixture.swift. The authoritative diff contains no Package.swift, Package.resolved, .gitignore, workflow, Xcode project, or dependency change…
Cmux Swift Logging ✅ Passed PASS. The only changed file is cmuxTests/CloudDesktopOpenFixture.swift, a test fixture. The diff adds no print, debugPrint, dump, NSLog, file/stdout logging, Logger, or sensitive-data logg…
Cmux User-Facing Error Privacy ✅ Passed PASS. The authoritative diff changes only cmuxTests/CloudDesktopOpenFixture.swift, which belongs to the cmuxTests test target. The added text is a test expectation message and developer-only comme…
Cmux Full Internationalization ✅ Passed PASS. The authoritative diff changes only cmuxTests/CloudDesktopOpenFixture.swift. The changes are test-fixture wait and expectation logic, with no production user-facing text, localization catalog,…
Cmux Swiftui State Layout ✅ Passed The pull request changes only cmuxTests/CloudDesktopOpenFixture.swift. The diff updates async drop-commit waiting and does not add or expand SwiftUI state, GeometryReader, lazy/list row store refe…
Cmux Architecture Rethink ✅ Passed PASS. The diff changes only cmuxTests/CloudDesktopOpenFixture.swift. It bounds an existing test-only notification wait with a 10-second Task.sleep, adds a direct post-drop count check, and changes…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: The pull request changes only cmuxTests/CloudDesktopOpenFixture.swift. The diff changes test fixture drop-wait logic and adds no NSWindow, NSPanel, NSWindowController, SwiftUI Window, …
Cmux Source Artifacts ✅ Passed The PR changes only cmuxTests/CloudDesktopOpenFixture.swift, a tracked hand-written Swift test fixture. The diff adds test wait logic and comments; it does not add logs, screenshots, recordings, cac…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The authoritative PR diff changes only cmuxTests/CloudDesktopOpenFixture.swift. No Swift file under a production Sources/ path changed, and the modified code is test fixture code. Therefore,…
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo teamleaderleo added unit-ci Run compile admission + app-host unit tests only, without full-ci's package/lag/Release lanes and removed no-full-ci Records that skipping the macOS suite on a test-only diff is deliberate labels Sep 24, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Independent review at 1a1c5a7

No blocking findings. This review read the code; nothing was compiled or run locally, and unit-ci now runs the suite.

  • Compiles cleanly as far as reading can tell. The fixture is @MainActor and CloudLinkFirstValue is @unchecked Sendable. cmuxTests builds as Swift 5, and Task.sleep(for:) is already used in the target, which deploys to macOS 14. A cancelled deadline's later resolve(false) does nothing once the value is set, and a test cancelled by its time limit resolves to nil and still fails.
  • >= hides nothing. The only caller checks count == 2 on the next line, so a double projection still fails there. The old == hung instead.
  • The body matches the log. In run 35862070143 the drop never committed (count → 1) == 2), so the deadline is the part that matters. It turns the 60 s time limit and the host restart about 13 s later into an ordinary failure.
  • Follow-up, not blocking: waitForOpen() has the same unbounded iterator.next() and is called in 11 places. It could still reach the time limit.

— Glitch g1 📚

@teamleaderleo teamleaderleo removed the unit-ci Run compile admission + app-host unit tests only, without full-ci's package/lag/Release lanes label Sep 24, 2026
@teamleaderleo
teamleaderleo merged commit bf13034 into main Sep 24, 2026
43 checks passed
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 24, 2026
ff6c6dc ci: stop restoring an iOS GhosttyKit cache nothing saves (manaflow-ai#14189)
2827231 ci: seed DerivedData on the 12 vCPU macOS 26 pool (manaflow-ai#14188)
ae46aa9 Let Computer Use toggles save past unrelated cmux.json issues (manaflow-ai#14183)
a785270 test: fail loudly when portal rendering authority denies a fixture's tab id (manaflow-ai#13937)
9a1dea0 test: pin which terminal tabs get an agent mark after manaflow-ai#14062 (manaflow-ai#14177)
91bcb28 test: run the change-area tests in parallel workers (manaflow-ai#14193)
6d203e8 test: await the geometry publish in the equalize-splits shortcut case (manaflow-ai#13916)
48f1adf ci: balance the guard legs the macOS gate waits on (manaflow-ai#14186)
38117cd test: settle the split's reparent-focus suppression before focus feedback (manaflow-ai#14049)
a12a0b8 ci: neutralize Swift sources without a per-character loop (manaflow-ai#14169)
2b6ca4c ci: stop counting queue time on cancelled jobs as runner minutes (manaflow-ai#14187)
2837f22 test: stop gating terminal focus on key status the app host cannot grant (manaflow-ai#13948)
23c0ce2 test: give each detect-step run its own cmux-ci scratch files (manaflow-ai#14185)
a13ea28 ci: skip Mac lanes that bundled scripts and guard-only lints cannot fail (manaflow-ai#14179)
b59f34f ci: restore Swift packages and a compilation cache for iOS uploads (manaflow-ai#14180)
bcca243 profiling: poll child processes every 0.1 s instead of every second (manaflow-ai#14170)
76d6176 refactor: move 45 leaf browser files into CmuxBrowser (manaflow-ai#14092)
bf13034 test: fail the Desktop drop fast instead of restarting the app host (manaflow-ai#14076)
51d486b ci: start guards and web beside Fast static checks (manaflow-ai#14176)

# Conflicts:
#	.github/workflows/ci-guards.yml
#	.github/workflows/ci.yml
#	.github/workflows/ios-appstore-upload.yml
#	.github/workflows/ios-testflight.yml
#	.github/workflows/nightly.yml
#	.github/workflows/seed-derived-data.yml
#	.github/workflows/test-ios.yml
teamleaderleo added a commit that referenced this pull request Sep 24, 2026
* ci: make a changed-suites run prove a known-failure fix

The app-host ratchet tolerates every test listed in
app-host-known-failures.json. In a PR's changed-suites run that makes a
fix unprovable: #14076 went green while the suite it fixed failed 4/4,
because the test was already listed.

In changed-suites mode (CMUX_APP_HOST_UNIT_SELECTORS set), a listed test
that passes now fails the run with RATCHET_KNOWN_NOW_PASSING and asks the
PR to drop the entry. Once it is dropped, the same run must pass on its
own merits. Main's full shards only report it, since a flaky entry can
pass on any one run.

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

* ci: keep naming tolerated known failures when a known entry passes

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo added a commit that referenced this pull request Sep 25, 2026
…#14304)

* test: give the Cloud Desktop fixture a routable window for pane drops

"A queued Desktop click retains its same-VM destination like a drop"
fails its drop on main (every case since run 35979983434): the catalog
keeps one projection and the commit wait times out after 10s.

The drop targets `.split(targetPane:)`. SurfacePaneFactory resolves that
pane's anchor through TerminalController.v2LocatePane, which walks
listMainWindowSummaries. That list skips any registered context without
a window, and VaultPaneAppFixture registers its context with
`window: nil`. So the lookup returns nil, the factory throws
paneNotFound, and handleSurfaceResourceDrop's Task swallows the error.
The click path is unaffected because it targets `.workspace(_, .split)`
and never locates a pane.

The same suite failed 4/4 run on its own in #14076's changed-suites lane
and hung once on main before that (run 35862070143), so it depended on
test order rather than on a product change.

Give the fixture's context a task-owned NSWindow that is never shown,
forget its route on close, and require the pane lookup before the drop
so a routing failure is reported where it happens.

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

* test: keep the Cloud Desktop fixture window hidden and never key

With a routable window, the opens and drops in this suite request focus,
and focusMainWindow now resolves the fixture window and would order it
front and make it key on a shared app-host runner. Stub the fixture
delegate's visibility controller with activation suppressed, as #13938
does, which needs the property to be internal instead of private, and
expect the window to stay hidden and not key around every action.

Also require the drop to be accepted and the located route to match the
fixture's manager and pane.

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

* ci: drop the fixed Desktop drop test from the known-failure catalog

With the fixture fixed, capturesClickDestination must pass instead of
being tolerated as a known main failure, so this PR's changed-suites
run proves the fix rather than passing either way.

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

* test: assert the fixture window is hidden after init completes

Swift rejects a method call on self before every stored property is set.

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant