Repository navigation
Fail CI on unit test failures - #5701
lawrencecchen wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughCI workflows now fail immediately on non-zero unit-test exit codes and the default unit-test timeout was increased; a CI guard prevents masking XCTest failures. BrowserPanel lifecycle/observer/devtools logic was adjusted, focus-related and inspector tests were made deterministic, and a retrying cargo build helper was added. ChangesCI Unit Test Failure Masking Removal
BrowserPanel and Test Improvements
Build script
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning, 2 inconclusive)
✅ Passed checks (12 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 |
79cec9f to
e5e80f3
Compare
Greptile SummaryThis PR fixes CI masking of real XCTest failures by routing all three unit-test workflows through a shared
Confidence Score: 5/5Safe to merge — all changes are CI infrastructure, targeted BrowserPanel bug fixes, and test stabilization with no new production regressions found. The core change (shared failure gate + SIGTERM forwarding) is fully covered by three new end-to-end test scripts. The BrowserPanel production changes are narrow, isolated fixes for the lifecycle and DevTools state. The headless test helpers are test-only scaffolding. No new unsafe patterns were introduced in production Swift or shell code. No files require special attention; BrowserPanel.swift carries forward a pre-existing architectural concern in DevTools state management already tracked in a prior review thread. Important Files Changed
Sequence DiagramsequenceDiagram
participant CI as CI Workflow (YAML)
participant Gate as run-unit-tests-with-failure-gate.sh
participant NI as xcodebuild_noninteractive.py
participant XC as xcodebuild (process group)
CI->>Gate: invoke with SOURCE_PACKAGES_DIR / env vars
Gate->>NI: launch in background (PID file)
NI->>XC: fork+exec (new process group via setpgrp)
NI-->>Gate: write child PID to file
loop Watchdog every 5s
Gate->>Gate: check kill -0 xcodebuild_pid
alt timeout exceeded
Gate->>NI: SIGTERM
NI->>XC: killpg(pgid, SIGTERM)
Note over Gate: sleep 5
Gate->>NI: SIGKILL
Gate-->>CI: exit 124
end
end
XC-->>NI: exit (status)
NI-->>Gate: exit (status or 128+SIGTERM)
Gate->>Gate: check SwiftPM resolution failure
alt SwiftPM failure
Gate->>Gate: clear caches + retry once
end
Gate-->>CI: exit nonzero on failure
Reviews (9): Last reviewed commit: "Kill unit test process group on timeout" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func requestDeveloperToolsRefreshAfterNextAttach(reason: String) { | ||
| guard preferredDeveloperToolsVisible else { return } | ||
| guard preferredDeveloperToolsVisible || developerToolsLastKnownVisibleAt != nil else { return } | ||
| setPreferredDeveloperToolsVisible(true) | ||
| forceDeveloperToolsRefreshOnNextAttach = true |
There was a problem hiding this comment.
developerToolsLastKnownVisibleAt is a staleness-detection timestamp (used by consumeAttachedDeveloperToolsManualCloseIfNeeded and retry guards) — it is not a canonical "the user wants DevTools open" source of truth. Using it as a fallback to call setPreferredDeveloperToolsVisible(true) means any layout-transition reattach that fires this function while developerToolsLastKnownVisibleAt is still non-nil (e.g., if the timestamp was updated by noteDeveloperToolsHostAttached after the user closed DevTools but before the nil-write propagates) will silently re-enable DevTools against user intent. The invariant that preferredDeveloperToolsVisible is the authority for whether the panel should be visible is lost.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let ghosttyBackgroundObserver = NotificationCenter.default.addObserver( | ||
| forName: .ghosttyDefaultBackgroundDidChange, object: nil, queue: nil | ||
| ) { [weak self] notification in | ||
| MainActor.assumeIsolated { self?.applyWebViewBackground(color: GhosttyBackgroundTheme.color(from: notification)) } | ||
| } |
There was a problem hiding this comment.
queue: nil delivers the notification on whatever thread posts it. If any call site posts .ghosttyDefaultBackgroundDidChange from a non-main thread, MainActor.assumeIsolated will trap at runtime. Using queue: .main is the safe alternative — it guarantees main-queue delivery regardless of the posting thread and avoids the assumeIsolated precondition entirely.
| let ghosttyBackgroundObserver = NotificationCenter.default.addObserver( | |
| forName: .ghosttyDefaultBackgroundDidChange, object: nil, queue: nil | |
| ) { [weak self] notification in | |
| MainActor.assumeIsolated { self?.applyWebViewBackground(color: GhosttyBackgroundTheme.color(from: notification)) } | |
| } | |
| let ghosttyBackgroundObserver = NotificationCenter.default.addObserver( | |
| forName: .ghosttyDefaultBackgroundDidChange, object: nil, queue: .main | |
| ) { [weak self] notification in | |
| self?.applyWebViewBackground(color: GhosttyBackgroundTheme.color(from: notification)) | |
| } |
e5e80f3 to
0f3b506
Compare
![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 `@Sources/Panels/BrowserPanel.swift`:
- Around line 5276-5281: The observer created as ghosttyBackgroundObserver for
.ghosttyDefaultBackgroundDidChange currently calls MainActor.assumeIsolated
unnecessarily; make the main-threading explicit by either passing queue: .main
to NotificationCenter.default.addObserver or by wrapping the handler body in
Task { `@MainActor` in ... } and remove MainActor.assumeIsolated; keep the handler
calling self?.applyWebViewBackground(color: GhosttyBackgroundTheme.color(from:
notification)) and preserve the AnyCancellable removal logic to unregister the
observer.
🪄 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: be4e52ee-b4c0-468f-9e38-00cb0dcfe279
📒 Files selected for processing (6)
.github/workflows/ci-macos-compat.yml.github/workflows/ci.yml.github/workflows/test-depot.ymlSources/Panels/BrowserPanel.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/BrowserConfigTests.swift
0f3b506 to
88419de
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88419deb80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inspectorWindow.contentView?.addSubview( | ||
| WKInspectorProbeView(frame: inspectorWindow.contentView?.bounds ?? .zero) |
There was a problem hiding this comment.
In this close-button regression test, replacing the frontend WKInspectorProbeWebView with only a plain probe view leaves FakeInspector.inspectorWebView() returning nil because setFrontendWebView is no longer called. The close-action path checks BrowserPanel.detachedDeveloperToolsWindowBelongsToPanel, which requires webView.cmuxInspectorFrontendWebView() to be a descendant of this window before it will close the inspector, so NSApp.sendAction(...) returns false and inspector.closeCount stays 0. This re-breaks the unit-test gate this commit is trying to restore.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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 `@scripts/build-command-palette-nucleo-ffi.sh`:
- Around line 65-67: Remove the fixed sleep-based backoff (the sleep $((attempt
* 5)) line) and instead wait for an explicit readiness signal or remove
timing-based retry entirely: after the failing cargo build echo/retry block,
replace the fixed-delay retry mechanism with a loop that checks a concrete
readiness condition (for example, existence of the expected build artifact, a
readiness FIFO/lock file, or a blocking readiness command) and only retries when
that condition indicates the target is not yet ready; keep the retry counter
(attempt) and the echo for diagnostics but do not use fixed sleep for
synchronization.
- Around line 58-63: The script currently sets status=$? after the if...fi block
which can mask the real cargo build exit code; change the logic to capture the
exit code immediately after running cargo build (store it in status right after
the cargo build command returns), then test status to decide whether to return 0
or retry, and on the final attempt return "$status" so the actual cargo failure
code is preserved (use the existing variables CRATE_DIR, target, attempt, and
status to locate and update the cargo build branch).
🪄 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: 749668c6-1cb5-453f-a922-15e7a60cd5cf
📒 Files selected for processing (7)
.github/workflows/ci-macos-compat.yml.github/workflows/ci.yml.github/workflows/test-depot.ymlSources/Panels/BrowserPanel.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/BrowserConfigTests.swiftscripts/build-command-palette-nucleo-ffi.sh
88419de to
eb88dd6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/build-command-palette-nucleo-ffi.sh (2)
65-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFixed sleep-based retry backoff still violates the runtime synchronization policy (duplicate of past review).
The
sleep $((attempt * 5))on line 66 implements timing-based retry logic, which is explicitly disallowed for production/build shell scripts. As per coding guidelines, retry logic must not depend on wall-clock time. Consider immediate retry (remove sleep) or a readiness-based wait.🤖 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/build-command-palette-nucleo-ffi.sh` around lines 65 - 66, Remove the time-based backoff sleep in the retry loop that uses sleep $((attempt * 5)) — instead implement a non-wall-clock retry strategy: either retry immediately by deleting the sleep line, or replace it with a readiness check that polls a deterministic condition (e.g., test artifacts, cargo lock/state, or a specific command that indicates tool readiness) before the next attempt; update the loop around the cargo build invocation that references ${target} and ${attempt} to use this readiness-based check or immediate retry so no wall-clock sleep remains.Source: Coding guidelines
58-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStatus capture still occurs after the if statement (duplicate of past review).
The
status=$?on line 61 captures the exit code after theifcompound command completes. While this works in bash, capturing immediately aftercargo buildis more explicit and reliable across shell variations.🤖 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/build-command-palette-nucleo-ffi.sh` around lines 58 - 63, Move the exit-code capture so it happens immediately after running the cargo build command: run cargo build --manifest-path "${CRATE_DIR}/Cargo.toml" --release --target "$target" then immediately assign status=$? and use that status in the subsequent conditional checks (instead of relying on the compound if returning code); update the existing if/return logic that references status and attempt to use this captured value.
🤖 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.
Duplicate comments:
In `@scripts/build-command-palette-nucleo-ffi.sh`:
- Around line 65-66: Remove the time-based backoff sleep in the retry loop that
uses sleep $((attempt * 5)) — instead implement a non-wall-clock retry strategy:
either retry immediately by deleting the sleep line, or replace it with a
readiness check that polls a deterministic condition (e.g., test artifacts,
cargo lock/state, or a specific command that indicates tool readiness) before
the next attempt; update the loop around the cargo build invocation that
references ${target} and ${attempt} to use this readiness-based check or
immediate retry so no wall-clock sleep remains.
- Around line 58-63: Move the exit-code capture so it happens immediately after
running the cargo build command: run cargo build --manifest-path
"${CRATE_DIR}/Cargo.toml" --release --target "$target" then immediately assign
status=$? and use that status in the subsequent conditional checks (instead of
relying on the compound if returning code); update the existing if/return logic
that references status and attempt to use this captured value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28981568-68e5-47c1-8a1a-24fba48033b8
📒 Files selected for processing (7)
.github/workflows/ci-macos-compat.yml.github/workflows/ci.yml.github/workflows/test-depot.ymlSources/Panels/BrowserPanel.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/BrowserConfigTests.swiftscripts/build-command-palette-nucleo-ffi.sh
eb88dd6 to
bb59727
Compare
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5972755c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| status=$? | ||
| if [ "$attempt" -eq 3 ]; then | ||
| return "$status" |
There was a problem hiding this comment.
When cargo build fails on the final retry, this captures $? after the if ... fi has completed, not after cargo itself; Bash documents that an if with no true branch exits zero when no condition matched, which I confirmed with help if. In incremental Xcode/CI builds where target/<triple>/release/libcmux_command_palette_nucleo_ffi.dylib already exists from an earlier build, three failed cargo attempts will therefore return success and package the stale dylib instead of failing the build.
Useful? React with 👍 / 👎.
bb59727 to
90c226d
Compare
90c226d to
aa19914
Compare
aa19914 to
b32efc2
Compare
b32efc2 to
40dc885
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7b5c370. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb6d201f08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| check_unit_tests_do_not_mask_xctest_assertion_failures() { | ||
| local file | ||
| for file in "$CI_FILE" "$COMPAT_FILE" "$ROOT_DIR/.github/workflows/test-depot.yml"; do | ||
| if ! grep -Fq "scripts/ci/run-unit-tests-with-failure-gate.sh" "$file"; then |
There was a problem hiding this comment.
/workspace/cmux/AGENTS.md says not to add tests that only verify source text or grep-style patterns, but this new guard decides whether the workflow is correct by grepping YAML strings rather than exercising the unit-test workflow behavior. In CI this can fail or pass based on spelling/placement changes while missing regressions in the actual failure gate; please cover the behavior through the shared script/harness instead of source-text checks.
Useful? React with 👍 / 👎.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
(0 unexpected)assertion failures as a passVerification
tests/test_ci_self_hosted_guard.shEvidence
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
CI behavior change removes a broad pass override so previously masked unit failures will fail builds; BrowserPanel DevTools and lifecycle edits affect WebKit UI paths but are mostly test- and edge-case-driven.
Overview
CI unit tests now fail on real XCTest failures instead of treating runs with
(0 unexpected)as green.ci.yml,ci-macos-compat.yml, andtest-depot.ymlcallscripts/ci/run-unit-tests-with-failure-gate.sh, which runscmux-unitvia the noninteractive xcodebuild wrapper with a configurable timeout, one SwiftPM cache-clear retry, and no expected-failure pass override.tests/test_ci_self_hosted_guard.shand related scripts assert workflows cannot reintroduce that masking.Supporting CI hardening:
xcodebuild_noninteractive.pyputs the child in its own process group and forwards SIGTERM/SIGINT; the nucleo FFI build script retriescargo buildup to three times;tests-build-and-lagcreates the virtual display before the CoreAnimation regression.App and test fixes exposed once failures were no longer masked:
BrowserPanelrefreshes web-view lifecycle when initial navigation is skipped, observes Ghostty background on the main thread, and tightens DevTools visibility/refresh state;HeadlessWindowTestHelpers.swiftadds stable key-window and background-color waits; shortcut, clipboard, terminal search, and browser DevTools tests swap fixed sleeps forwaitUntil/waitForand safer WebKit teardown.Reviewed by Cursor Bugbot for commit fb6d201. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Make unit tests fail CI on real XCTest errors by routing all unit-test jobs through a shared failure gate with a watchdog that can kill the whole test process group on timeout and a one-time SwiftPM retry. Also tighten BrowserPanel/DevTools lifecycle, apply Ghostty background updates on the main thread, improve
xcodebuildsignal handling, and deflake headless tests.Bug Fixes
NotificationCenterobserver.HeadlessWindowTestHelpers.swiftadds a reliable make-key helper and background-color waits; retain WebKit windows and inspector frontend webviews; replace sleeps with deterministic waits.CI
scripts/ci/run-unit-tests-with-failure-gate.sh(watchdog with kill-on-timeout of the wrapper and child process group, one SwiftPM cache-clear retry, preserves nonzero exit).tests/test_ci_self_hosted_guard.shenforces gate usage and nonzero exit preservation;tests/test_ci_unit_test_spm_retry.shverifies the SwiftPM retry;tests/test_ci_xcodebuild_noninteractive_helper.pychecks SIGTERM forwarding.scripts/ci/xcodebuild_noninteractive.pyputsxcodebuildin its own process group, writes the child PID, forwards SIGTERM/SIGINT to the group, and returns nonzero on termination.scripts/build-command-palette-nucleo-ffi.shretriescargo buildup to 3 times.Written for commit fb6d201. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Chores