Fail app-host unit-test shards on hidden assertion failures - #9513
Closed
austinywang wants to merge 4 commits into
Closed
austinywang wants to merge 4 commits into
austinywang wants to merge 4 commits into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot 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. |
📝 WalkthroughWalkthroughThe app-host test wrapper now detects assertion failures in every xcodebuild attempt. CI no longer retries those failures as SwiftPM errors or treats nonzero app-host exits with clean summaries as successful. Regression tests cover XCTest, Swift Testing, retries, and workflow execution. ChangesApp-host failure accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant AppHostWrapper
participant Xcodebuild
participant RegressionTests
CIWorkflow->>AppHostWrapper: run app-host tests
AppHostWrapper->>Xcodebuild: execute test attempt
Xcodebuild-->>AppHostWrapper: return log and exit status
AppHostWrapper->>AppHostWrapper: detect assertion failures
AppHostWrapper-->>CIWorkflow: return failure without retry
RegressionTests->>CIWorkflow: verify failure and retry behavior
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci/run-app-host-xcodebuild.sh`:
- Around line 91-94: Update the assertion-failure branch around
log_contains_test_assertion_failure to remove the raw grep output from CI logs.
Instead, calculate and print only a sanitized count of matching test assertion
failures, while retaining raw diagnostics exclusively through the existing
restricted artifact or internal logging mechanism if available; preserve the
exit 1 behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16e2ae23-ba97-43ff-b443-c8edd83794f8
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/ci/run-app-host-xcodebuild.shtests/test_ci_app_host_xcodebuild_retry.sh
Bugbot is paused — on-demand spend limit reachedBugbot 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/ci/run-app-host-xcodebuild.sh (1)
86-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve and test the app-host exit-code contract.
The assertion path currently normalizes every marker hit to
1. The regression checks only require a nonzero result, so this status loss is not detected.
scripts/ci/run-app-host-xcodebuild.sh#L86-L97: preserve the captured nonzerostatus; use1only when markers prove failure withstatus=0.tests/test_ci_app_host_xcodebuild_retry.sh#L76-L90: assert the expected status for the fixture that exits65.tests/test_ci_app_host_xcodebuild_retry.sh#L228-L234: assert the expected propagated status for the workflow fixture that exits65.Based on the PR objective to propagate the actual unit-test exit status, preserve and verify the exact status contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/run-app-host-xcodebuild.sh` around lines 86 - 97, Preserve the captured nonzero exit status in the assertion-failure path of scripts/ci/run-app-host-xcodebuild.sh lines 86-97, using status 1 only when failure markers are found with status 0. Update tests/test_ci_app_host-xcodebuild_retry.sh lines 76-90 and 228-234 to assert that fixtures exiting 65 propagate status 65 through both direct and workflow paths.tests/test_ci_app_host_xcodebuild_retry.sh (2)
76-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin the exit-code contract in the regression checks.
The first fixture exits
65at Line [64]. The workflow fixture exits65at Line [206]. Both checks only reject0, so a regression that changes the propagated status to another nonzero value still passes.Assert the documented propagated status at both layers. If the contract intentionally maps assertion failures to
1, assert1and document that mapping.Based on the PR objective to propagate the actual unit-test exit status, test the exact contract instead of only testing failure.
Also applies to: 228-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_ci_app_host_xcodebuild_retry.sh` around lines 76 - 90, Update both regression checks around the assertion-retry fixture and workflow fixture to require the documented propagated exit status, rather than merely rejecting zero. Assert the exact status produced by the first fixture and workflow path (or explicitly use and document 1 if assertion failures are intentionally mapped), while preserving the existing failure reporting and regression count behavior.
54-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a communication-failure-only retry case.
This fixture emits both an XCTest failure and
Failed to establish communication with the test runnerat Lines [61-63]. It proves that an assertion stops a retry. It does not prove that a communication-only failure still retries.Add a first attempt with
0 failures (0 unexpected)plus the communication marker. Add a clean second attempt. Assert two invocations and a successful result.Based on the PR objective to retain bounded retries for infrastructure failures, add this regression case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_ci_app_host_xcodebuild_retry.sh` around lines 54 - 73, Add a regression case in tests/test_ci_app_host_xcodebuild_retry.sh covering a communication-only failure: make the first xcodebuild attempt output 0 failures and 0 unexpected alongside the communication marker, then make the second attempt clean. Assert that the command is invoked twice and ultimately succeeds, while preserving the existing assertion-failure case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_ci_app_host_xcodebuild_retry.sh`:
- Around line 92-96: Add a negative assertion in the test block around the
sanitized marker-count check to ensure representative raw xcodebuild diagnostic
lines are absent from assertion-retry-output.log before accepting the test. Keep
the existing marker-count assertion and failure accounting, and use the
fixture’s known raw diagnostic lines as the rejection criteria.
---
Outside diff comments:
In `@scripts/ci/run-app-host-xcodebuild.sh`:
- Around line 86-97: Preserve the captured nonzero exit status in the
assertion-failure path of scripts/ci/run-app-host-xcodebuild.sh lines 86-97,
using status 1 only when failure markers are found with status 0. Update
tests/test_ci_app_host-xcodebuild_retry.sh lines 76-90 and 228-234 to assert
that fixtures exiting 65 propagate status 65 through both direct and workflow
paths.
In `@tests/test_ci_app_host_xcodebuild_retry.sh`:
- Around line 76-90: Update both regression checks around the assertion-retry
fixture and workflow fixture to require the documented propagated exit status,
rather than merely rejecting zero. Assert the exact status produced by the first
fixture and workflow path (or explicitly use and document 1 if assertion
failures are intentionally mapped), while preserving the existing failure
reporting and regression count behavior.
- Around line 54-73: Add a regression case in
tests/test_ci_app_host_xcodebuild_retry.sh covering a communication-only
failure: make the first xcodebuild attempt output 0 failures and 0 unexpected
alongside the communication marker, then make the second attempt clean. Assert
that the command is invoked twice and ultimately succeeds, while preserving the
existing assertion-failure case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f2146c8-f434-4e1f-9d8c-1b80ba8119f6
📒 Files selected for processing (2)
scripts/ci/run-app-host-xcodebuild.shtests/test_ci_app_host_xcodebuild_retry.sh
Contributor
Author
|
Review audit:
Focused hosted verification passed on HEAD 1f7639c: https://github.com/manaflow-ai/cmux/actions/runs/30883702668/job/91910231046 |
This was referenced Aug 4, 2026
Open
5 of 9 tasks
lawrencecchen
added
the
stale-revisit
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
3 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
(0 unexpected)pass fallback and propagate the real unit-test exit statusTesting
994a62023a):gh workflow run ci.yml --ref issue-7471-ci-app-host-unit-test-job-swallows-real— workflow-guard-tests failed at the app-host retry regression.5b01f3c9cb): the same workflow dispatch — workflow-guard-tests passed, including the app-host retry regression.(0 unexpected)by the actual workflow step.1f7639c964): hosted workflow guards passed, including exact exit-status propagation, communication-only retry, and sanitized diagnostics coverage../scripts/reload-cloud.sh --tag sym7471succeeded remotely onblacksmith-6vcpu-macos-26(run 30882874657); no local build fallback was used. This is CI-only behavior with no applicable debug-socket runtime path. The app was not launched, and cleanup confirmed nosym7471process remained.Fixes #7471
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Make app-host unit-test shards fail on real assertion failures by scanning every
xcodebuildattempt log and stopping retries, while still retrying transient infra errors. Removes the "(0 unexpected)" pass fallback, surfaces a counted failure-marker diagnostic, and preserves the real exit code through the workflow (fixes #7471).xcodebuildexit code in CI; drop summary-based tolerance and fail on any non-zero status.Written for commit 1f7639c. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests