Repository navigation
Fail CI on xcodebuild hard failures - #4523
lawrencecchen wants to merge 19 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 a Bash CI guard that scans persisted unit-test logs for hard-failure signatures, integrates the guard into multiple GitHub Actions workflows to run after initial and retry test runs, refactors skip-testing argument generation and timeout fallback, and adds a regression test validating guard behavior across simulated failure logs. ChangesCI Unit-Test Hard-Failure Guard Validation
Sequence DiagramsequenceDiagram
participant GitHubActions
participant GuardScript as scripts/ci-unit-test-output-guard.sh
participant XCTestParser as XCTestSummaryParser
GitHubActions->>GuardScript: run(EXIT_CODE, /tmp/test-output.txt)
GuardScript-->>GitHubActions: exit 0 or exit 1 (reasons)
GitHubActions->>XCTestParser: parse XCTest summaries (only if exit 0)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 `@tests/test_ci_unit_test_output_guard.sh`:
- Around line 21-24: The test currently only checks presence of the guard string
"./scripts/ci-unit-test-output-guard.sh \"$EXIT_CODE\" /tmp/test-output.txt" but
not its position relative to the XCTest parsing step; change the test to assert
ordering by using grep -n (or awk) to capture the line number of the guard
invocation and the line number of the XCTest summary parsing command (e.g., the
parsing script or step name used for "XCTest summaries"), then compare the
numeric line numbers and fail if the guard's line number is not less than the
parser's line number; update the check that currently uses grep -Fq to instead
compute and compare these two line numbers and emit the same failure message if
ordering is wrong.
🪄 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
Run ID: 95724706-04ba-406f-8bcf-225458e570dd
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/test_ci_unit_test_output_guard.sh
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — the guard script, all three workflow integrations, and the Swift test-isolation changes are self-contained and well-tested. The guard script is narrow in scope and correctly uses fixed-string and extended-regex grep with an explicit count guard on the reasons array. Both previous review concerns (empty-array fallback and uncovered log-only timeout path) are resolved in the current head. The Swift changes gate window bootstrap during unit tests and centralise stale-surface liveness checks, with synthetic-pointer injection replacing unsafe freed-memory reuse in the test helper. Workflow changes are additive and symmetric across all three files. No files require special attention. Important Files Changed
Reviews (19): Last reviewed commit: "fix: mark socket-backed UI test launch" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for existing in "${reasons[@]:-}"; do | ||
| if [ "$existing" = "$reason" ]; then | ||
| return | ||
| fi | ||
| done |
There was a problem hiding this comment.
:- fallback
"${reasons[@]:-}" when reasons=() expands to a single empty-string word "" rather than zero words, so the for loop iterates once with existing="". Any reason that is itself an empty string "" would be incorrectly seen as already-present and silently dropped on the first call. All current callers use hardcoded non-empty strings so this can't trigger today, but the idiom is fragile. On bash 4+, an empty declared array doesn't trigger set -u; "${reasons[@]+"${reasons[@]}"}" is the portable form that expands to zero words when empty and to all elements otherwise.
There was a problem hiding this comment.
Fixed by removing the empty-array fallback and only iterating reasons when the array has elements.
— Claude Code
There was a problem hiding this comment.
Already addressed in current head: add_reason now checks the array count before iterating, so there is no empty-string fallback path left.
— Claude Code
| write_log timeout-after-summary \ | ||
| "Executed 1 test, with 1 failure (0 unexpected) in 0.1 seconds" \ | ||
| "xcodebuild unit test timeout after 900s; terminating" \ | ||
| "** BUILD INTERRUPTED **" | ||
| expect_fail 124 timeout-after-summary "xcodebuild watchdog timeout" |
There was a problem hiding this comment.
The timeout-after-summary case passes exit code 124, which hits the [ "$EXIT_CODE" = "124" ] branch in the guard before the grep -Fq "xcodebuild unit test timeout after" branch runs. Both branches add the same reason and deduplication masks the second. There's no test that sends a non-124 exit code with the timeout string in the log, so the grep-only path for timeout detection is untested. A CI scenario where a wrapper script exits non-124 but still injects the timeout string into the log would silently pass the guard with current coverage.
There was a problem hiding this comment.
Fixed by adding a timeout-string-only case with a non-124 exit code, so the log pattern is covered independently of the process timeout status.
— Claude Code
There was a problem hiding this comment.
Already addressed in current head: tests/test_ci_unit_test_output_guard.sh includes timeout-string-only with exit code 65, which exercises timeout detection from log text without relying on exit code 124.
— Claude Code
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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 @.github/workflows/ci.yml:
- Around line 278-279: Add an inline TODO comment next to the two skipped test
entries
(-skip-testing:cmuxTests/TerminalNotificationSocketActionTests/testNotificationJumpToUnreadOpensLatestUnreadAndNoOpsWhenNoneRemain
and
-skip-testing:cmuxTests/TerminalNotificationSocketActionTests/testNotificationJumpToUnreadPayloadMatchesOpenedFallbackNotification)
that includes a tracking issue or link (e.g. GH issue/bug ID) and a clear
removal condition (what must be fixed or verified before removing the skip), so
the skip is not left permanently; place the TODO immediately above or beside
each skip line and use a consistent format like "TODO(quarantine): track
<issue-link> — remove when <condition>".
🪄 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
Run ID: 2a60b76f-d5c2-43ef-9a87-c71d228937f1
📒 Files selected for processing (5)
.github/workflows/ci-macos-compat.yml.github/workflows/ci.yml.github/workflows/test-depot.ymlscripts/ci-unit-test-output-guard.shtests/test_ci_unit_test_output_guard.sh
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
6f73965 to
8791a6a
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
4c82021 to
bbcd11d
Compare
bbcd11d to
abe763a
Compare
abe763a to
ff0ef74
Compare
ff0ef74 to
b290fa9
Compare
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
scripts/ci-unit-test-output-guard.shto fail macOS unit-test jobs on xcodebuild watchdog timeout,BUILD INTERRUPTED, Swift crash output, or Swift interactive backtrace prompts.(0 unexpected)XCTest summary parser inci.yml,ci-macos-compat.yml, andtest-depot.yml.(0 unexpected).Testing
./tests/test_ci_self_hosted_guard.sh && ./tests/test_ci_create_dmg_pinned.sh && ./tests/test_ci_unit_test_spm_retry.sh && ./tests/test_ci_unit_test_output_guard.sh && ./tests/test_ci_scheme_testaction_debug.sh && ./tests/test_ci_ghosttykit_checksum_verification.sh && node scripts/release_asset_guard.test.js && ./tests/test_ci_ghosttykit_checksum_present.sh && ./tests/test_ci_swift_warning_budget.sh && ./tests/test_ci_swift_file_length_budget.sh && ./tests/test_ci_auxiliary_window_close_shortcuts.sh./tests/test_ci_unit_test_output_guard.shbefore the guard script exists.Issues
Note
Low Risk
Low risk: a single UI test now sets an additional launch environment flag, with no production code changes and minimal behavioral impact outside that test.
Overview
Updates the
testEscapeDismissesCommandPaletteOpenedByCmdShiftPUI test to launch the app withCMUX_UI_TEST_MODE=1, aligning the test environment with expected command-palette/debug-state behavior when verifying Escape dismissal.Reviewed by Cursor Bugbot for commit e946810. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Tests
Chores