Fail a unit shard that loses verdicts to a dead app host (rebase of #8833) - #9574
austinywang wants to merge 43 commits into
Conversation
When the app host dies mid-run, xcodebuild relaunches it and its summary then covers only the last launch. Verdicts pending in the dead host are absent from the totals rather than failed, so the shard reports success. Run 29684286662 on main concluded success with 35 of those deaths across its four app-host jobs. The unit shard now fails on four conditions: the runner's own restart line, a shard that executed no tests, a crash report that appeared during the run, and a failure of the crash scan itself. The restart line misses a crash with nothing left to relaunch, such as one during teardown after the last verdict has printed. scripts/crash-reports-since.py covers that by listing the crash directory before the run and subtracting afterwards, since macOS gives no way to redirect that directory per run. It copies each match into a per-run directory that is uploaded with the rest of the evidence, because macOS prunes the original. Zero reports is not proof that nothing crashed — ReportCrash writes asynchronously — so this backs the restart check up rather than replacing it. The list of tests that started and never reported matched only swift-testing's output shape, so it printed nothing for a shard of XCTest suites, which is most of them. An empty list reads as "nothing was lost" at the exact moment something was. It now matches both. Measured against a real log whose host died: the old form printed nothing, the new one names MarkdownPanelTests.testOpenMarkdownPanelReloadsWhenFileChangesOnDisk. RemoteTmuxRectPublicationTests force-unwrapped the published layout and the observed pane in nine places. Each traps exactly when the expectation around it is already failing, turning a reportable failure into a dead process. They record the nil and continue. The shard-packing comment said BrowserDeveloperToolsVisibilityPersistenceTests reliably crash-restarts the host. That is fixed elsewhere, so the comment now says so, but the separation stays: that suite drives real WebKit inspector attach and detach and may still starve a live-navigation suite sharing its shard. Removing it needs a timing measurement. The crash scan self-tests against fixtures whose answers are known in advance, eight cases including a non-cmux crash in the same directory, a truncated report, a pre-existing crash that must not be attributed to the run, and a missing snapshot. That self-test runs in CI.
Two findings from Greptile, both correct. cmux_reports called header_of(path) twice per report, once for app_name and once for procName, so every file in the directory got two reads and two JSON parses. One assignment. The docstring on new_since_snapshot said a missing snapshot means no reports are attributable. The code does the opposite and returns every report, and the self-test asserts that as the intended conservative behaviour. The shard writes the snapshot before the run, so its absence means the scan went wrong rather than that the run was clean, and the caller should see the reports. The docstring now says that. Also corrected the shard-packing comment, which claimed the host crash was fixed by the suite's windows clearing isReleasedWhenClosed. That was one proposed fix and it is not the one being taken. It now points at #8832, which stops test windows releasing themselves on close, and does not claim the fix is already in the tree.
…pshot Two findings from CodeRabbit, both correct. The scan read `app_name or procName`, which only consults procName when app_name is empty. A report whose app_name is some host process and whose procName is cmux was therefore skipped, so neither scan path would report or copy that crash. Both fields are now checked independently through one predicate used by both paths. `--new-since` returned an empty list and exited 0 when the snapshot file was absent. The shard writes that snapshot before the run, so its absence means the write failed — and if the crash directory also happens to be empty, an empty list plus exit 0 reads as "nothing crashed". That is the same silent-pass shape this gate exists to close. The command now exits 1 and says why, which the shard already treats as a failure. The library function keeps returning everything, which is what its caller and its self-test expect. Self-test covers both: a report named only by procName is found, and the command exits non-zero for a missing snapshot. Ten cases, all passing. Verified against the gate's own shell as ci.yml runs it — a missing snapshot fails the shard, a present one proceeds.
The probe used a fixed name in the shared temp directory and deleted it first. An earlier run that left the file behind, or a concurrent run creating it between the delete and the subprocess call, would make the probe assert nothing while still printing ok. The path now lives inside a fresh TemporaryDirectory and is never created, so "absent" is guaranteed rather than arranged. Mutation-checked: replacing the CLI's missing-snapshot guard with `if False` makes the self-test fail, so the case tests the refusal rather than describing it.
Review asked what happens to the crash scan when two shards share a machine, since ~/Library/Logs/DiagnosticReports cannot be scoped per job. The comment now says what the attribution assumes and why it holds: the macOS runners are ephemeral per-job Tart VMs (docs/ci-runners.md), so each shard has the directory to itself. If that ever changes, a foreign cmux crash inside the window fails this shard too — a false failure, never a false pass, because a report that predates the snapshot is excluded and one that appears during the run is always attributed.
|
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:
📝 WalkthroughWalkthroughAdds streaming unit-test diagnostics, snapshot-based crash scanning, stronger CI shard validation, retained evidence artifacts, launch-mode-aware app bootstrap, and more resilient macOS test fixtures and assertions. ChangesCrash-aware unit-test CI
Launch isolation and test reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 1019-1043: Update the crash-detection flow around the crash-report
scan and final shard decision so it waits for a reliable CrashReporter
completion signal or equivalent finalization protocol before treating an empty
CRASHES result as clean. Do not rely on the single invocation of
scripts/crash-reports-since.py after test exit; preserve the existing scan
failure handling and crash-report failure path while ensuring teardown crashes
are included before success.
- Around line 1055-1061: Update the SWIFT_TESTS calculation near TESTS_SEEN so
it excludes “Test run” summary lines and counts only individual Swift Testing
result records, ensuring a zero-test summary cannot satisfy the nonzero guard.
Add or update the relevant fixture coverage to verify that a zero-test Swift
Testing run causes the shard to fail.
🪄 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: 8b96b761-efcc-466b-9b56-55b875461db0
📒 Files selected for processing (4)
.github/workflows/ci.ymlcmuxTests/RemoteTmuxRectPublicationTests.swiftscripts/ci/cmux_unit_test_shard.pyscripts/crash-reports-since.py
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 (1)
.github/workflows/ci.yml (1)
993-1011: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse one streaming parser for lost-test diagnostics.
Lines 1002-1010 scan
$TEST_OUTPUTfour times and sort both full name collections. The collection is the complete xcodebuild shard log. It can grow with test count and diagnostic output. This path costsO(4L + S log S + C log C)time.Replace the two
grepandsortpipelines with one streaming parser. Store started and completed names in sets. Emit the first 20 unmatched names after EOF. This reduces the work toO(L + S + C)without sorting.As per coding guidelines, “Avoid repeated full scans, sorting, filtering, or per-item nested scans over scalable collections in production code.”
🤖 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 @.github/workflows/ci.yml around lines 993 - 1011, Replace the four grep/sort pipelines in the host-death diagnostics with a single sequential read of TEST_OUTPUT. In that parser, record each Swift Testing or XCTest started name and remove or mark each corresponding completed name in constant-time sets; after reaching EOF, emit up to the first 20 started names still unmatched. Preserve the existing name normalization and diagnostic output while avoiding repeated scans and sorting.Source: Coding guidelines
🤖 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/executed_test_count.py`:
- Around line 11-24: Refactor the executed_test_count function to replace the
SUMMARY_PATTERNS tuple with a single regex pattern using alternation (pipe
operator) to combine both patterns. Update the implemented logic to scan the log
once using finditer on this combined pattern and compute the maximum count
during that single pass, rather than collecting all matches into a list before
finding the max. Preserve the default return value of 0 when no matches are
found.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 993-1011: Replace the four grep/sort pipelines in the host-death
diagnostics with a single sequential read of TEST_OUTPUT. In that parser, record
each Swift Testing or XCTest started name and remove or mark each corresponding
completed name in constant-time sets; after reaching EOF, emit up to the first
20 started names still unmatched. Preserve the existing name normalization and
diagnostic output while avoiding repeated scans and sorting.
🪄 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: 434190b1-6f58-4a48-903a-c943cd3ed3ab
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/ci/executed_test_count.pytests/test_ci_executed_test_count.py
|
Addressed the outside-diff lost-test diagnostic finding in 793a3ac. The host-death path no longer runs four full-log grep/sort pipelines. It invokes Added four behavioral fixtures covering Swift Testing, XCTest, a later restart of an already-completed name, and deterministic limit/order behavior. Both parser suites (9 tests total), Ruff, Python compilation, YAML parsing, and diff checks pass. |
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 (1)
.github/workflows/ci.yml (1)
995-1005: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve crash evidence before failing for host death.
At Line 1005,
exit 1skips Lines 1033-1045. Therefore, a shard with a runner restart never runs--copy-to "$CRASH_KEPT". The always-run artifact then has the log and selectors but no copied crash reports.Record the host-death result and defer the final exit until after the crash scan. This retains available crash reports for restart failures.
Proposed fix
HOST_DEATHS=$(grep -c "Restarting after unexpected exit, crash, or test timeout" "$TEST_OUTPUT" || true) + HOST_DIED=0 if [ "${HOST_DEATHS:-0}" -ne 0 ]; then echo "::error::app host died ${HOST_DEATHS} time(s); every verdict pending in those launches is lost" echo "Tests that started but never reported (the last one named is where a host died):" python3 scripts/ci/lost_test_verdicts.py "$TEST_OUTPUT" --limit 20 - exit 1 + HOST_DIED=1 fi @@ if [ -n "$CRASHES" ]; then echo "::error::a cmux process crashed during this shard, even if every verdict printed" echo "$CRASHES" | sed 's/^/ /' exit 1 fi + if [ "$HOST_DIED" -ne 0 ]; then + exit 1 + fi🤖 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 @.github/workflows/ci.yml around lines 995 - 1005, In the host-death branch around HOST_DEATHS, replace the immediate exit with a recorded failure status so execution continues through the crash scan and its --copy-to "$CRASH_KEPT" handling. After that scan completes, perform the final nonzero exit using the recorded status, preserving the existing host-death diagnostics and success behavior.
🤖 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/lost_test_verdicts.py`:
- Around line 23-31: Update the pending-event tracking in the loop over
EVENT_PATTERN matches to retain every started invocation rather than
deduplicating by test name. Use an occurrence-preserving structure keyed by test
name, have completion remove only the latest matching pending start, and leave
earlier unmatched starts available for the lost-verdict diagnostic; add a
fixture covering repeated started records followed by one completion.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 995-1005: In the host-death branch around HOST_DEATHS, replace the
immediate exit with a recorded failure status so execution continues through the
crash scan and its --copy-to "$CRASH_KEPT" handling. After that scan completes,
perform the final nonzero exit using the recorded status, preserving the
existing host-death diagnostics and success 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: 00dc74c2-b41a-4ab0-b4fc-6b60f27d4367
📒 Files selected for processing (4)
.github/workflows/ci.ymlscripts/ci/executed_test_count.pyscripts/ci/lost_test_verdicts.pytests/test_ci_lost_test_verdicts.py
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. |
|
Addressed the outside-diff crash-evidence finding in 9bd3da9. A restart now records Both parser suites (10 fixtures total), Ruff, Python compilation, YAML parsing, and diff checks pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
995-1005: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftUse one streaming shard-log analysis pass.
Line 995 scans the full
TEST_OUTPUTafter Line 971 already reads the full file intoOUTPUT. The normal path then scans it again inexecuted_test_count.py. The host-death path scans it again inlost_test_verdicts.py.This is O(n) work per pass over an unbounded xcodebuild log. A retry adds further full passes. Make one parser own restart detection, verdict diagnostics, and executed-test counting. Emit the required bounded status fields from that parser. Retain the raw log only for the artifact.
As per coding guidelines, avoid repeated full scans over scalable collections in production code.
🤖 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 @.github/workflows/ci.yml around lines 995 - 1005, Refactor the CI shard log handling so a single streaming parser owns restart detection, lost-verdict diagnostics, and executed-test counting, replacing the separate full-log scans involving OUTPUT, executed_test_count.py, and lost_test_verdicts.py. Have that parser emit the required bounded status fields, while retaining TEST_OUTPUT only for artifact upload and preserving the existing host-death reporting behavior and limit.Source: Coding guidelines
🤖 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.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 995-1005: Refactor the CI shard log handling so a single streaming
parser owns restart detection, lost-verdict diagnostics, and executed-test
counting, replacing the separate full-log scans involving OUTPUT,
executed_test_count.py, and lost_test_verdicts.py. Have that parser emit the
required bounded status fields, while retaining TEST_OUTPUT only for artifact
upload and preserving the existing host-death reporting behavior and limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43f6271b-52ec-4e6e-aa0f-4d52349bb963
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/ci/lost_test_verdicts.pytests/test_ci_lost_test_verdicts.py
|
Addressed the outside-diff single-owner finding architecturally in 29cff25.
It emits only four fixed integer assignments (safe for the workflow to source) and a bounded 20-name pending-verdict file. The workflow no longer materializes The unified executable fixture suite has 10 cases under both Python 3.9 and the current Python, including zero-test Swift Testing output, nested XCTest summaries, repeated starts, both harnesses’ lost verdicts, retry/restart detection, bounded order, and singular/plural expected-failure summaries. Ruff, Python compilation, YAML parsing, sharding guards, the crash scanner self-test, and diff checks pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
957-994: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail if
teecannot retainTEST_OUTPUT.Line 979 treats
TEST_OUTPUTas the shard decision source. Both capture pipelines save onlyPIPESTATUS[0]. Withset +e, a nonzeroteestatus is not fatal and is then discarded. Ifteefails afterrun_unit_testsexits zero, the parser can consume a partial log with a positive test count and allow a shard without complete evidence.Capture both pipeline statuses. Fail before analysis when
teefails. Apply the same helper to the retry at Line 994.Proposed fix
analyze_test_output() { python3 scripts/ci/unit_test_log_summary.py \ "$TEST_OUTPUT" \ --env-output "$TEST_ANALYSIS" \ --lost-output "$LOST_TESTS" \ --lost-limit 20 source "$TEST_ANALYSIS" } + capture_unit_test_output() { + set +e + run_unit_tests | tee "$TEST_OUTPUT" + UNIT_TEST_CAPTURE_STATUS=("${PIPESTATUS[@]}") + set -e + + EXIT_CODE="${UNIT_TEST_CAPTURE_STATUS[0]}" + if [ "${UNIT_TEST_CAPTURE_STATUS[1]}" -ne 0 ]; then + echo "::error::failed to retain complete unit-test output" + exit 1 + fi + } ... - set +e - run_unit_tests | tee "$TEST_OUTPUT" - EXIT_CODE=${PIPESTATUS[0]} - set -e + capture_unit_test_output analyze_test_output ... - set +e - run_unit_tests | tee "$TEST_OUTPUT" - EXIT_CODE=${PIPESTATUS[0]} - set -e + capture_unit_test_output analyze_test_output🤖 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 @.github/workflows/ci.yml around lines 957 - 994, Update both run_unit_tests capture pipelines in the shard workflow to record the tee status separately from PIPESTATUS[0] and fail immediately before analyze_test_output when tee cannot write TEST_OUTPUT. Apply identical handling to the retry pipeline, while preserving EXIT_CODE from the test command for existing retry and verdict logic.
🤖 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.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 957-994: Update both run_unit_tests capture pipelines in the shard
workflow to record the tee status separately from PIPESTATUS[0] and fail
immediately before analyze_test_output when tee cannot write TEST_OUTPUT. Apply
identical handling to the retry pipeline, while preserving EXIT_CODE from the
test command for existing retry and verdict logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5dc8c47d-1454-42f0-86c6-4522b7adfc71
📒 Files selected for processing (4)
.github/workflows/ci.ymlscripts/ci/unit_test_log_summary.pytests/test_ci_unit_test_log_summary.pytests/test_ci_unit_test_spm_retry.sh
💤 Files with no reviewable changes (1)
- tests/test_ci_unit_test_spm_retry.sh
|
Addressed the two latest top-level CodeRabbit findings.
The crash-scanner self-test, all 10 unified parser fixtures, Ruff, Python compilation, and git diff --check pass. The warning budget is unchanged. |
|
Reworked the latest renderer-lifetime fixture repair in c3b5e59 so it no longer adds a DEBUG/test hook to Sources/AppDelegate.swift. The headless interception now lives entirely in cmuxTests/AppDelegateMainWindowTestingSupport.swift. It listens to the existing main-window registration notification, swaps newly registered content before orderFront, and uses a task-local scope so concurrent app-host suites creating unrelated windows are not affected. RemoteTmuxMirrorCloseDetachTests wraps both direct and socket-driven dedicated-window creation in that scope; production AppDelegate code is byte-identical to main again. Local non-Xcode proof: 10 unit_test_log_summary fixtures, the crash-report scanner self-test, YAML parsing, pbxproj test-wiring lint, diff checks, and the unchanged Swift warning budget all pass. Per task policy, the focused suite and full CI are running only in GitHub Actions. |
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. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 245,275 of the 240,000 allowed lines of code this month. Reviews resume on 1 September 2026 (in 28 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.github/workflows/ci.yml:
- Around line 969-981: Update capture_unit_test_output and the SwiftPM retry
flow so an attempt with nonzero HOST_DEATHS is not retried; otherwise preserve
and aggregate host-death and lost-verdict state across attempts instead of
truncating TEST_OUTPUT or resetting parser state. Keep the existing later crash
scan before final failure.
In `@cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift`:
- Around line 250-264: Update the defer cleanup in the test around
controller.cacheConnection(connection) to unconditionally detach the cached
(host, sessionName) tuple, removing the sessionMirror existence guard so cleanup
also runs when mirroring fails or returns false.
In `@scripts/ci/unit_test_log_summary.py`:
- Line 20: Update scripts/ci/unit_test_log_summary.py at lines 20 and 48-50 so
EXPECTED_FAILURE_SUMMARY captures both reported failure counts, and
expected_failures_only is set only when failures are positive and unexpected
failures equal zero. Add a zero-failure XCTest fixture and assert
EXPECTED_FAILURES_ONLY is 0 in tests/test_ci_unit_test_log_summary.py lines
142-156.
In `@scripts/crash-reports-since.py`:
- Around line 74-76: Update crashes_since() to parse --since and each report
timestamp as offset-aware datetimes, comparing them in UTC or otherwise
preserving their offsets instead of truncating to the local-time prefix. Add a
fall-back DST fixture covering repeated local times with different offsets and
asserting the correct chronological ordering.
In `@Sources/AppDelegate.swift`:
- Around line 7372-7374: Gate the existing Debug no-window recovery block around
openNewMainWindow(nil) with MacAppLaunchMode.current(environment:
env).shouldAutomaticallyCreateMainWindow, so unit-app hosts do not create a
window automatically. Add a host-launch regression test verifying no window
exists until the test explicitly opens one, without materially changing the
existing asyncAfter timing path.
🪄 Autofix
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: 8d257ff5-3431-4c9c-9530-757f6972fb3a
📒 Files selected for processing (17)
.github/workflows/ci.ymlSources/App/MacAppLaunchMode.swiftSources/App/MacSentryStartupPolicy.swiftSources/AppDelegate+CmuxSSHURL.swiftSources/AppDelegate.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateMainWindowTestingSupport.swiftcmuxTests/CLISSHPTYAttachTransientBridgeFailureExitCodeTests.swiftcmuxTests/MacSentryStartupPolicyTests.swiftcmuxTests/RemoteTmuxMirrorCloseDetachTests.swiftcmuxTests/RemoteTmuxRectPublicationTests.swiftcmuxTests/TabManagerUnitTests.swiftscripts/ci/unit_test_log_summary.pyscripts/crash-reports-since.pytests/test_ci_unit_test_log_summary.pytests/test_ci_unit_test_spm_retry.sh
💤 Files with no reviewable changes (1)
- tests/test_ci_unit_test_spm_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. |
lawrencecchen
added
the
stale-revisit
4 participants
Summary
fix/unit-shard-loses-verdictsbranch because the original PR is backed byejc3/cmuxand this takeover is prohibited from pushing to that fork.teecannot retain the complete decision log.RemoteTmuxRectPublicationTestsso assertion failures remain ordinary test verdicts instead of terminating the shared app host..github/swift-warning-budget.tsvis unchanged.The grading invariant is the reason for the change: after an app host dies, xcodebuild can relaunch it and print a successful summary for only the final launch. Verdicts pending in the dead launch are absent, not green.
Testing
python3 -m unittest tests/test_ci_unit_test_log_summary.py— 10 behavioral fixtures pass.python3 scripts/crash-reports-since.py --self-test— snapshot, attribution, copy, malformed-report, process-name, and missing-snapshot cases pass.Run unit testsBash syntax,git diff --check, and the deterministic cmux policy gate pass.teestatus fails the step, while a test-command exit of 42 remainsEXIT_CODE=42.RemoteTmuxRectPublicationTestsworkflow run executed all 18 tests without a host restart or crash; the final full CI workflow will run after prerequisite cmuxTests: repair three red suites, one of which was killing its own test host #9572 lands.Demo Video
Not applicable: the production Swift change is gated to embedded unit-app-host launches; normal and UI-test app runtime/UI behavior is unchanged.
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fail unit-test shards when the app host dies, runs zero tests, a new crash report appears, or the crash scan fails, and keep logs plus copied crash reports so lost verdicts don’t read as passes. App‑host unit launches now boot headless (no automatic main window) while starting only headless services; Sentry stays off.
scripts/ci/unit_test_log_summary.pyhandles Swift Testing and XCTest; detects host restarts, zero-test shards, and “expected failures only”; exportsPACKAGE_RESOLUTION_FAILED,HOST_DEATHS,TESTS_SEEN,EXPECTED_FAILURES_ONLY; lists up to 20 started‑but‑no‑verdict tests (both harnesses); fails ifteeloses output; uploads shard logs/analysis/args and copied crash reports (14 days).scripts/crash-reports-since.pysnapshots before the run; matches new reports byapp_nameorprocName; exits non‑zero on a missing snapshot; copies matches to a per‑run dir; parses crash timestamps on Python 3.9 (handles local/DST forms); CI self‑tests are timezone‑independent.MacAppLaunchMode; skip automatic window bootstrap, start only the socket listener and mobile host services, return false on reopen; renamedscheduleInitialMainWindowBootstraptoscheduleAutomaticLaunchBootstrap.HeadlessMainWindowInterceptoracross dedicated‑window and socket paths; unmount panels before teardown; cache/detach connections; keep the socket worker actor‑safe./usr/bin/sshand the bundled CLI; model-G/-O checkcontrol‑path probes; emit realistic stderr on second auth to enforce fakes.TerminalSurfaceRuntimeTeardownCoordinator; keepTabManager.focusHistoryNow@Sendable; skip history capture for the synthetic teardown token.AgentNotificationMutationBoundaryTeststo stabilize the fixture..exitedwithout dropping the snapshot); compare session files by resolved URL to ignore/varvs/private/varaliases; expect wrapper shim invocation in resume bindings and env pass‑through.MacAppLaunchMode; test ensures the embedded unit app‑host test bundle prevents Sentry from starting.Written for commit e63036d. Summary will update on new commits.
Summary by CodeRabbit
CI Improvements
Bug Fixes
Testing