Repository navigation
Fail closed on app-host test outcomes and remove impossible route waits - #12173
Closed
austinywang wants to merge 60 commits into
Closed
austinywang wants to merge 60 commits into
austinywang wants to merge 60 commits into
Conversation
…ectations Three changes that a red-to-green run on a macOS builder proves together, covering two suites. The product bug: a BrowserPanel built with an initial URL and rendering deferred keeps the .newTab lifecycle state it is born with, so a restored-but-not-yet-loaded tab is classified as an empty new tab. That state feeds the omnibar's new-tab handling and is exported in top telemetry. The recompute is driven by shouldRenderWebView's didSet, and on this path the assignment writes false over a declared default of false, so the didSet guard on oldValue != newValue never fires and the initializer returns without recomputing. Both deferred branches had the same gap. Two tests written when .deferredURL was introduced already assert the right thing and were failing on it, so no test is added here. TerminalOffscreenStartupTests read the runtime-creation counter synchronously right after the initializer returns, but a startup surface now installs its per-surface claude wrapper shim on a detached task and only calls createSurface once that finishes. The tests wait for the attempt and keep the claim they were written for by asserting uiWindow is nil, which is non-nil only once the surface is in a real window rather than the hidden bootstrap one. The same suite's three attach-ticket tests waited on mobile.host.status reporting routes; that method is the unauthenticated probe and discloses none, so the wait was unsatisfiable rather than slow. They seed the public status cache instead, which is what the ticket path reads. The browser under-page fill tests asserted the raw translucent colour they posted, but the panel composites it over the window background and fills opaquely. The expectation is now derived from the panel's own theme helper, with a guard assertion so the derivation cannot go vacuous. Measured on a macOS builder, base eeb4866 against this tree: TerminalOffscreenStartupTests red with 10 failures -> passing. BrowserDeveloperToolsConfigurationTests red with 18 failures -> passing. That suite needs both the colour update and the product fix, since its three failures split across them.
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request updates app-host test classification, Swift Testing sharding, timeout handling, configuration-path validation, network determinism detection, terminal test isolation, and mobile capability ordering. ChangesApp-host CI execution
Test determinism detection
Terminal test isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant ConsoleSession
participant XcodebuildRunner
participant ResultClassifier
CIWorkflow->>ConsoleSession: forward timeout and Swift Testing settings
ConsoleSession->>XcodebuildRunner: run noninteractive xcodebuild
XcodebuildRunner->>ResultClassifier: provide exit status and output file
ResultClassifier-->>CIWorkflow: return classified result
Merge Risk: 🟡 Moderate · up to A total CI deadline can hide the specific test failure classification needed by app-host reporting, and the determinism gate concern remains unresolved. Resolve these before merge to keep CI results trustworthy. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 73689e1. Configure here.
This comment has been minimized.
This comment has been minimized.
# Conflicts: # Sources/Surfaces/CmuxTuiSurfaceProviders.swift # cmuxTests/CmuxTuiSurfaceProviderTests.swift
Contributor
|
Mac fleet instructions for head JOB_JSON=$(~/.local/bin/cmux-ci submit --kind cmux --command 'CMUX_FLEET_BUILD_TAG=pr-12173-09118b6f /Users/Shared/cmux-build-fleet/recipes/cmux.sh https://github.com/manaflow-ai/cmux.git 09118b6f3f4f5634df366ce34026fb77c9f50968' --artifact artifacts/cmux.app.zip --workspace https://github.com/manaflow-ai/cmux/pull/12173 --source-digest 09118b6f3f4f5634df366ce34026fb77c9f50968 --cache-key cmux:pr-12173 --min-free-bytes 268435456000 --label cmux --label ram48)
JOB_ID=$(python3 -c 'import json,sys; print(json.load(sys.stdin)["id"])' <<<"$JOB_JSON")
~/.local/bin/cmux-ci wait "$JOB_ID" --receipt artifacts/fleet/$JOB_ID.json
~/.local/bin/cmux-ci publish-hq "$JOB_ID"Use an existing campaign job ID if one is already posted; do not submit a duplicate. A wait timeout leaves the remote job running. Published results will include an exact-head artifact link and timing/disk receipt. This recipe validates the macOS app only, not iOS or tests. Never use maclease or put credentials in a PR comment. |
This was referenced Sep 20, 2026
Open
5 of 6 tasks
teamleaderleo
added
area: build-and-ci
Collaborator
|
Closing; reopen if you still want it. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
python3 tests/test_ci_change_areas.pybash tests/test_ci_app_host_xcodebuild_retry.shpython3 tests/test_ci_xcodebuild_noninteractive_helper.pybash tests/test_ci_app_host_identity.shbash tests/test_ci_app_host_processes.shbash tests/test_ci_app_host_home_cleanup.shpython3 scripts/check-test-determinism.py --self-testand--strictpython3 scripts/check-package-resolved-policy.pypython3 scripts/check-workspace-package-groups.pybash scripts/check-pbxproj.shbash scripts/lint-pbxproj-test-wiring.shpython3 scripts/swift_file_length_budget.pySeptember 10 resume, pushed HEAD
4ee30db3f4973982076c6dc609c3a15bdb217e3b: local CI-tooling regressions pass, including the complete change-area test runner, noninteractive helper, app-host retry/identity/process/home fixtures, sharding tests (2457 selectors), shared Swift lexer tests (14), determinism fixtures (150 positive + 90 negative), strict determinism scan (0 findings), and the policy/wiring checks above. New regression fixtures were committed separately and observed failing locally before their fixes. No local Xcode build or test was run.Final-head full CI run
34460787505is pending, not claimed green. E2E34458121381tested SHAfa74034a08and passed all 29 TerminalOffscreenStartupTests, including the three attach-ticket cases and the inventory-order regression. The app/runtime/test-source trees are unchanged between that SHA and HEAD. Hosted web-typecheck passed atda947bab7f; the web test files are unchanged in HEAD. The prior E2E attribution was corrected in comment5589141222: run34248807414tested branch SHA7eb9b54add, not main.Trade-offs
mainvia merge commits, preserving the existing commit provenance instead of rewriting the long PR stack.--force-with-leasefrom its stale pre-rebase SHA; the explicit lease prevented overwriting any intervening origin update.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Medium Risk
Changes affect CI gating for the full app-host unit suite and agent automatic-resume ownership rules; misclassification could hide real failures or leave stale bindings, though the direction is fail-closed with expanded regression coverage.
Overview
App-host CI now treats Swift Testing and wrapper exit codes as authoritative: sharding emits per-batch Swift Testing metadata, the noninteractive helper enforces total deadlines and reserved statuses (123–127), batches run with
-parallel-testing-enabled NO, andclassify-app-host-test-result.shonly tolerates ordinary XCTest failures when the log still shows(0 unexpected)—so a stale summary cannot green a missing or failed Swift Testing phase. Ghostty config validation is tightened with canonical path checks (aliases OK, traversal rejected).Product fixes include not marking agent resume bindings stale when the live index has no entry after a completed scan (only a observed non-live entry counts), rewriting loopback browser URLs with correct IPv6 bracket handling, and sorting the DEBUG mobile RPC inventory for release-gate parity.
Tests and tooling move private-URL and simulator-gesture coverage into dedicated suites, fix attach-ticket tests by seeding
MobileHostPublicStatusCacheinstead of waiting on the unauthenticated status probe, split UserDefaults superseded-source coverage, and extend the determinism scanner forProcess.run/curl-style invocations. Several snapshot/cloud/tree expectations were updated for tab-scoped port nodes and parser tab-ID behavior.Reviewed by Cursor Bugbot for commit 3d8c1be. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fails closed on app-host test outcomes so a tolerant XCTest summary can no longer pass a job with real Swift Testing failures, timeouts, or unknown exit statuses. Also removes impossible unauthenticated mobile-host route waits in
TerminalOffscreenStartupTestsand fixes related restore and release-gate bugs.CI hardening
classify-app-host-test-result.shonly tolerates ordinary XCTest statuses when the summary shows "(0 unexpected)" and no Swift Testing failure line is present.check-test-determinism.pyflagscurl/wgetinsideProcess.run,exec, and multiline shell strings, while allowing a wrapped deadline-timeout race.Bug fixes and test updates
MobileHostPublicStatusCachedirectly, headless startup tests tear down surfaces deterministically, and the mobile debug RPC inventory is sorted.vm.diagnosticsRPC.Written for commit 09118b6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores