Repository navigation
Add latest macOS launch crash smoke test - #2765
lawrencecchen wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a conditional macOS UI launch smoke test and a CI startup-crash probe: new XCUITest source and Xcode project entries, CI matrix flags and conditional xcodebuild steps, plus a bash script that repeatedly probes app startup from Xcode DerivedData. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant CI as CI Job
participant XCB as xcodebuild
participant SCRIPT as startup-crash-probe script
participant XCT as XCTest Runner
participant APP as App Process
participant FS as Filesystem (isolated HOME)
GH->>CI: start macOS compatibility job (matrix lane)
CI-->GH: evaluate matrix flags
alt launch_smoke_ui == true
CI->>XCB: run xcodebuild test target (cmuxUITests/...LatestMacOSLaunchSmokeUITests)
XCB->>XCT: start XCTest runner
XCT->>FS: create isolated HOME + write settings.json
XCT->>APP: launch XCUIApplication (CMUX_TAG, HOME)
APP-->>XCT: report lifecycle (running/exit)
XCT->>APP: poll for running state and stability, then terminate
XCT->>FS: remove isolated HOME
end
alt direct_startup_probe == true
CI->>XCB: build cmux scheme (DerivedData)
CI->>SCRIPT: run startup-crash-probe (ITERATIONS, STABILITY_SECONDS)
SCRIPT->>FS: create temp HOME + write settings.json
SCRIPT->>APP: launch app binary (env CMUX_TAG, HOME, CMUX_SOCKET_MODE)
APP-->>SCRIPT: exit or remain running
SCRIPT->>FS: collect logs/diagnostics on failure, cleanup temp HOME
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Greptile SummaryThis PR adds Confidence Score: 5/5This PR is safe to merge — it adds a well-structured smoke test and a targeted CI lane with no changes to production code. All findings are P2 (style/hardening suggestions). The test logic correctly detects launch crashes via cmuxUITests/LatestMacOSLaunchSmokeUITests.swift — minor hardening opportunities around Important Files Changed
Sequence DiagramsequenceDiagram
participant CI as GitHub Actions (macOS 26 lane)
participant XB as xcodebuild test
participant Host as XCTest Runner
participant App as cmux.app
CI->>XB: xcodebuild -only-testing:LatestMacOSLaunchSmokeUITests test
XB->>Host: launch test host process
Host->>App: app.launch() (wrapped in XCTExpectFailure)
App-->>Host: state → runningForeground / runningBackground
Host->>Host: waitForAppToStart (poll 0.2s, up to 20s)
Host->>Host: waitForNoImmediateCrash (poll 0.2s for 10s)
alt app stays running
Host->>App: app.terminate()
Host-->>CI: test passed
else app crashes
App-->>Host: state → notRunning
Host-->>CI: XCTAssertTrue failure
end
Reviews (1): Last reviewed commit: "Add latest macOS launch crash smoke UI t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func isRunning(_ app: XCUIApplication) -> Bool { | ||
| app.state == .runningForeground || app.state == .runningBackground | ||
| } |
There was a problem hiding this comment.
runningBackgroundSuspended not covered
isRunning treats .runningBackgroundSuspended as "not running", so if macOS suspends the app during the 10-second stability window waitForNoImmediateCrash will return false and the test will report a false crash. While macOS rarely suspends apps, it is a valid XCUIApplicationState on the platform and including it keeps the test from flaking on heavily-loaded CI runners.
| private func isRunning(_ app: XCUIApplication) -> Bool { | |
| app.state == .runningForeground || app.state == .runningBackground | |
| } | |
| private func isRunning(_ app: XCUIApplication) -> Bool { | |
| app.state == .runningForeground | |
| || app.state == .runningBackground | |
| || app.state == .runningBackgroundSuspended | |
| } |
| private func launchAllowingHeadlessBackgroundState(_ app: XCUIApplication) { | ||
| // Some CI runners launch in background-only mode, which can emit an | ||
| // activation failure even when the process is healthy. | ||
| let options = XCTExpectedFailure.Options() | ||
| options.isStrict = false | ||
| XCTExpectFailure("App activation may fail on headless CI runners", options: options) { | ||
| app.launch() | ||
| } | ||
| } |
There was a problem hiding this comment.
XCTExpectFailure swallows all launch failures without a targeted matcher
No issueMatcher is supplied, so every failure recorded inside the block — including codesign errors, bundle-not-found, or sandbox violations — is silently suppressed. When any of those infrastructure issues occur, the test continues and ultimately fails with "Expected cmux to start" instead of the underlying cause, making CI failures harder to diagnose. Consider adding an issue matcher scoped to activation/connection errors:
| private func launchAllowingHeadlessBackgroundState(_ app: XCUIApplication) { | |
| // Some CI runners launch in background-only mode, which can emit an | |
| // activation failure even when the process is healthy. | |
| let options = XCTExpectedFailure.Options() | |
| options.isStrict = false | |
| XCTExpectFailure("App activation may fail on headless CI runners", options: options) { | |
| app.launch() | |
| } | |
| } | |
| private func launchAllowingHeadlessBackgroundState(_ app: XCUIApplication) { | |
| // Some CI runners launch in background-only mode, which can emit an | |
| // activation failure even when the process is healthy. | |
| let options = XCTExpectedFailure.Options() | |
| options.isStrict = false | |
| options.issueMatcher = { $0.compactDescription.contains("activation") || $0.compactDescription.contains("connection") } | |
| XCTExpectFailure("App activation may fail on headless CI runners", options: options) { | |
| app.launch() | |
| } | |
| } |
![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 the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci-macos-compat.yml:
- Line 20: Add an actionlint config to allowlist the custom self-hosted runner
labels so actionlint stops reporting unknown runners like "warp-macos-*" used in
runs-on; create a file named .github/actionlint.yaml that defines the
"self-hosted-runner" key with a labels list including "warp-macos-*" and
"depot-macos-*", ensuring any workflows that use runs-on: warp-macos-26-arm64-6x
(and similar labels) pass actionlint validation.
In `@cmuxUITests/LatestMacOSLaunchSmokeUITests.swift`:
- Around line 12-31: The test currently calls app.terminate() directly and can
skip that call if an assertion fails; wrap cleanup in an addTeardownBlock to
guarantee termination: capture the XCUIApplication instance `app` created in
`testAppLaunchDoesNotCrashOnStartup()` and add an `addTeardownBlock` that checks
`isRunning(app)` and calls `app.terminate()` so the app is always terminated
even on early assertion failures (remove or keep the inline terminate only if
you still have the teardown to ensure cleanup).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ef9468f-513c-4659-8294-fdc7c851e955
📒 Files selected for processing (3)
.github/workflows/ci-macos-compat.ymlGhosttyTabs.xcodeproj/project.pbxprojcmuxUITests/LatestMacOSLaunchSmokeUITests.swift
| smoke: true | ||
| skip_zig: false | ||
| launch_smoke_ui: false | ||
| - os: warp-macos-26-arm64-6x |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Workflow references =="
rg -n 'warp-macos-26-arm64-6x|warp-macos-15-arm64-6x|runs-on:' .github/workflows/ci-macos-compat.yml
echo
echo "== actionlint config files =="
fd -HI 'actionlint\.ya?ml$' || true
echo
echo "== actionlint label allowlist snippets =="
for f in $(fd -HI 'actionlint\.ya?ml$'); do
echo "--- $f"
rg -n 'label|labels|self-hosted|warp-macos|runner' "$f" || true
doneRepository: manaflow-ai/cmux
Length of output: 276
🏁 Script executed:
# Check for any actionlint references in the repo
echo "=== Searching for actionlint references ==="
rg -r 'actionlint' . --type yaml --type toml --type json --type yml 2>/dev/null | head -20
echo
echo "=== Checking for other workflow files ==="
fd -HI '\.ya?ml$' .github/workflows | head -20
echo
echo "=== Other workflows using warp runners ==="
for f in $(fd -HI '\.ya?ml$' .github/workflows); do
if rg -q 'warp-macos' "$f" 2>/dev/null; then
echo "Found in: $f"
rg -n 'warp-macos' "$f"
fi
done
echo
echo "=== Checking for actionlint in package.json or CI config ==="
fd -HI 'package.json|\.github' . | xargs -I {} sh -c 'test -f {} && grep -l actionlint {} 2>/dev/null || true'Repository: manaflow-ai/cmux
Length of output: 1160
🏁 Script executed:
# Check if actionlint is in any config files (including hidden/dotfiles)
echo "=== All files in .github directory ==="
find .github -type f | head -30
echo
echo "=== Check for CI/linting setup ==="
ls -la .github/ 2>/dev/null || trueRepository: manaflow-ai/cmux
Length of output: 970
🌐 Web query:
warp-macos-26-arm64-6x GitHub Actions runner
💡 Result:
warp-macos-26-arm64-6x is a WarpBuild (third-party) GitHub Actions cloud runner label for macOS 26 on Apple Silicon (arm64) with 6 vCPU, 22 GB RAM, 120 GB SSD (priced at $0.08/min per WarpBuild’s docs). [1]
Use it in your workflow like:
jobs:
build:
runs-on: warp-macos-26-arm64-6x
steps:
- uses: actions/checkout@v4If you meant GitHub-hosted runners instead, GitHub’s equivalent label is macos-26 (standard arm64) or macos-26-xlarge (larger arm64). [2]
Sources: WarpBuild runner tags/specs [1], GitHub macOS 26 runner labels [2], WarpBuild macOS runner overview [3].
🌐 Web query:
actionlint warp-macos runner validation
💡 Result:
actionlint validates runs-on against a built-in list of runner labels, so WarpBuild labels like warp-macos-latest-arm64-6x will be reported as “unknown” unless you allow them via config. WarpBuild’s macOS runner label example is warp-macos-latest-arm64-6x. [1]
Add an actionlint config at .github/actionlint.yaml and whitelist the Warp labels as self-hosted runner labels (glob patterns are supported):
self-hosted-runner:
labels:
- warp-macos-*This tells actionlint that warp-macos-* is an allowed custom runner label and stops the runner-label validation error. [2][3]
🏁 Script executed:
# Check if there are any comments in workflows about these runners or depot
echo "=== Comments about warp/depot runners ==="
rg -n 'depot|self.hosted|self-hosted|warp' .github/workflows/ || true
echo
echo "=== Check the specific ci-macos-compat.yml file context ==="
cat -n .github/workflows/ci-macos-compat.yml | head -30Repository: manaflow-ai/cmux
Length of output: 2424
Add actionlint config to allowlist WarpBuild runner labels and prevent CI validation failures.
actionlint validates runs-on against a built-in list of runner labels and will report warp-macos-* runners as unknown. Create .github/actionlint.yaml with:
self-hosted-runner:
labels:
- warp-macos-*
- depot-macos-*This affects multiple workflows using these runner labels across the repository.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 20-20: label "warp-macos-26-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-intel", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-macos-compat.yml at line 20, Add an actionlint config
to allowlist the custom self-hosted runner labels so actionlint stops reporting
unknown runners like "warp-macos-*" used in runs-on; create a file named
.github/actionlint.yaml that defines the "self-hosted-runner" key with a labels
list including "warp-macos-*" and "depot-macos-*", ensuring any workflows that
use runs-on: warp-macos-26-arm64-6x (and similar labels) pass actionlint
validation.
| func testAppLaunchDoesNotCrashOnStartup() { | ||
| let app = XCUIApplication() | ||
| app.launchEnvironment["CMUX_TAG"] = launchTag | ||
| app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" | ||
|
|
||
| launchAllowingHeadlessBackgroundState(app) | ||
|
|
||
| XCTAssertTrue( | ||
| waitForAppToStart(app, timeout: 20.0), | ||
| "Expected cmux to start on latest macOS. state=\(app.state.rawValue)" | ||
| ) | ||
|
|
||
| XCTAssertTrue( | ||
| waitForNoImmediateCrash(app, duration: 10.0), | ||
| "Expected cmux to remain running for startup stability window. state=\(app.state.rawValue)" | ||
| ) | ||
|
|
||
| if isRunning(app) { | ||
| app.terminate() | ||
| } |
There was a problem hiding this comment.
Ensure cleanup always runs even on early assertion failure.
If either assertion fails, execution can stop before the explicit terminate call, leaving the app alive for subsequent tests in the same run. Move termination into addTeardownBlock.
💡 Proposed fix
func testAppLaunchDoesNotCrashOnStartup() {
let app = XCUIApplication()
app.launchEnvironment["CMUX_TAG"] = launchTag
app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1"
+ addTeardownBlock {
+ if app.state == .runningForeground || app.state == .runningBackground {
+ app.terminate()
+ }
+ }
launchAllowingHeadlessBackgroundState(app)
@@
- if isRunning(app) {
- app.terminate()
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func testAppLaunchDoesNotCrashOnStartup() { | |
| let app = XCUIApplication() | |
| app.launchEnvironment["CMUX_TAG"] = launchTag | |
| app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" | |
| launchAllowingHeadlessBackgroundState(app) | |
| XCTAssertTrue( | |
| waitForAppToStart(app, timeout: 20.0), | |
| "Expected cmux to start on latest macOS. state=\(app.state.rawValue)" | |
| ) | |
| XCTAssertTrue( | |
| waitForNoImmediateCrash(app, duration: 10.0), | |
| "Expected cmux to remain running for startup stability window. state=\(app.state.rawValue)" | |
| ) | |
| if isRunning(app) { | |
| app.terminate() | |
| } | |
| func testAppLaunchDoesNotCrashOnStartup() { | |
| let app = XCUIApplication() | |
| app.launchEnvironment["CMUX_TAG"] = launchTag | |
| app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" | |
| addTeardownBlock { | |
| if app.state == .runningForeground || app.state == .runningBackground { | |
| app.terminate() | |
| } | |
| } | |
| launchAllowingHeadlessBackgroundState(app) | |
| XCTAssertTrue( | |
| waitForAppToStart(app, timeout: 20.0), | |
| "Expected cmux to start on latest macOS. state=\(app.state.rawValue)" | |
| ) | |
| XCTAssertTrue( | |
| waitForNoImmediateCrash(app, duration: 10.0), | |
| "Expected cmux to remain running for startup stability window. state=\(app.state.rawValue)" | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/LatestMacOSLaunchSmokeUITests.swift` around lines 12 - 31, The
test currently calls app.terminate() directly and can skip that call if an
assertion fails; wrap cleanup in an addTeardownBlock to guarantee termination:
capture the XCUIApplication instance `app` created in
`testAppLaunchDoesNotCrashOnStartup()` and add an `addTeardownBlock` that checks
`isRunning(app)` and calls `app.terminate()` so the app is always terminated
even on early assertion failures (remove or keep the inline terminate only if
you still have the teardown to ensure cleanup).
![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 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/LatestMacOSLaunchSmokeUITests.swift">
<violation number="1" location="cmuxUITests/LatestMacOSLaunchSmokeUITests.swift:39">
P2: `XCTExpectFailure` without an `issueMatcher` silently suppresses **all** failures inside the block — including codesign errors, bundle-not-found, and sandbox violations. When any of those occur, the test continues past `launch()` and ultimately fails at the "Expected cmux to start" assertion, hiding the real root cause. Scope the matcher to activation/connection errors only:
```swift
options.issueMatcher = { issue in
issue.compactDescription.contains("activation")
|| issue.compactDescription.contains("connection")
}
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // activation failure even when the process is healthy. | ||
| let options = XCTExpectedFailure.Options() | ||
| options.isStrict = false | ||
| XCTExpectFailure("App activation may fail on headless CI runners", options: options) { |
There was a problem hiding this comment.
P2: XCTExpectFailure without an issueMatcher silently suppresses all failures inside the block — including codesign errors, bundle-not-found, and sandbox violations. When any of those occur, the test continues past launch() and ultimately fails at the "Expected cmux to start" assertion, hiding the real root cause. Scope the matcher to activation/connection errors only:
options.issueMatcher = { issue in
issue.compactDescription.contains("activation")
|| issue.compactDescription.contains("connection")
}Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/LatestMacOSLaunchSmokeUITests.swift, line 39:
<comment>`XCTExpectFailure` without an `issueMatcher` silently suppresses **all** failures inside the block — including codesign errors, bundle-not-found, and sandbox violations. When any of those occur, the test continues past `launch()` and ultimately fails at the "Expected cmux to start" assertion, hiding the real root cause. Scope the matcher to activation/connection errors only:
```swift
options.issueMatcher = { issue in
issue.compactDescription.contains("activation")
|| issue.compactDescription.contains("connection")
}
```</comment>
<file context>
@@ -0,0 +1,69 @@
+ // activation failure even when the process is healthy.
+ let options = XCTExpectedFailure.Options()
+ options.isStrict = false
+ XCTExpectFailure("App activation may fail on headless CI runners", options: options) {
+ app.launch()
+ }
</file context>
![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: 0e81c1df9e
ℹ️ 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".
|
|
||
| let app = XCUIApplication() | ||
| app.launchEnvironment["CMUX_TAG"] = launchTag | ||
| app.launchEnvironment["HOME"] = launchHomeDirectory.path |
There was a problem hiding this comment.
This smoke test creates settings.json under a temporary directory and relies on app.launchEnvironment["HOME"] to redirect cmux there, but cmux resolves its primary settings file from FileManager.default.homeDirectoryForCurrentUser (Sources/KeyboardShortcutSettingsFileStore.swift:99-101). Because that path is not derived from the test’s HOME launch environment, the fixture may be ignored and the test can pass without exercising the managed app.appIcon startup path it is meant to guard. Please write the fixture to the path cmux actually reads (or add/use an explicit settings-path override consumed by the app).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f80c54edb
ℹ️ 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".
| log_file="$LOG_DIR/${iteration}-${mode}.log" | ||
|
|
||
| echo "--- Launch iteration=$iteration mode=$mode tag=$tag ---" | ||
| HOME="$probe_home" CMUX_TAG="$tag" CMUX_SOCKET_MODE=allowAll "$BINARY_PATH" >"$log_file" 2>&1 & |
There was a problem hiding this comment.
The probe launches cmux with HOME="$probe_home", but cmux resolves managed settings from FileManager.default.homeDirectoryForCurrentUser / .applicationSupportDirectory (Sources/KeyboardShortcutSettingsFileStore.swift:99-114), not from this launch env override. That means the fixtures written under $probe_home/.config/... and $probe_home/Library/Application Support/... are frequently ignored, so this job can pass without exercising the intended app.appIcon startup paths (automatic/light/dark) and miss the regression it is meant to catch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/startup-crash-probe-ci.sh`:
- Around line 104-107: The current shutdown sequence may hang because wait
"$app_pid" can block indefinitely if the process ignores TERM; update the block
that uses app_pid (the kill -0 check, kill "$app_pid", and wait "$app_pid") to
perform a bounded wait instead of an unbounded wait: either wrap the wait in a
timeout (e.g., use the timeout utility to limit wait to a few seconds) or
replace the single wait with a short polling loop that checks kill -0 "$app_pid"
with a sleep and a max-iteration/countdown to give up after a bounded interval,
then proceed (ensure you still attempt kill and consume exit status as before).
- Around line 11-15: The APP_PATH assignment uses a single find -print -quit
which can return an arbitrary/stale app; change it to collect all matching paths
and deterministically pick the most recent build (e.g., use find to list all
matches for "*/Build/Products/Debug/cmux DEV.app", then sort by modification
time and select the newest) before testing [[ -z "$APP_PATH" ]], updating the
APP_PATH variable assignment and keeping rest of the script intact (refer to the
APP_PATH variable and the existing find invocation).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76516cb7-9a74-41d1-9f0a-bdd861920006
📒 Files selected for processing (2)
.github/workflows/ci-macos-compat.ymlscripts/startup-crash-probe-ci.sh
| APP_PATH="$(find "$HOME/Library/Developer/Xcode/DerivedData" -path "*/Build/Products/Debug/cmux DEV.app" -print -quit 2>/dev/null || true)" | ||
| if [[ -z "$APP_PATH" ]]; then | ||
| echo "ERROR: could not find built cmux DEV.app in DerivedData" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Make app-bundle discovery deterministic.
Using find ... -print -quit can pick an arbitrary/stale DerivedData app when multiple builds exist, which can mask regressions in this probe.
Proposed fix
-APP_PATH="$(find "$HOME/Library/Developer/Xcode/DerivedData" -path "*/Build/Products/Debug/cmux DEV.app" -print -quit 2>/dev/null || true)"
+APP_PATH="$(
+ find "$HOME/Library/Developer/Xcode/DerivedData" \
+ -type d -path "*/Build/Products/Debug/cmux DEV.app" -print0 2>/dev/null \
+ | xargs -0 stat -f '%m %N' 2>/dev/null \
+ | sort -nr \
+ | head -n 1 \
+ | cut -d' ' -f2- || true
+)"
if [[ -z "$APP_PATH" ]]; then
echo "ERROR: could not find built cmux DEV.app in DerivedData" >&2
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| APP_PATH="$(find "$HOME/Library/Developer/Xcode/DerivedData" -path "*/Build/Products/Debug/cmux DEV.app" -print -quit 2>/dev/null || true)" | |
| if [[ -z "$APP_PATH" ]]; then | |
| echo "ERROR: could not find built cmux DEV.app in DerivedData" >&2 | |
| exit 1 | |
| fi | |
| APP_PATH="$( | |
| find "$HOME/Library/Developer/Xcode/DerivedData" \ | |
| -type d -path "*/Build/Products/Debug/cmux DEV.app" -print0 2>/dev/null \ | |
| | xargs -0 stat -f '%m %N' 2>/dev/null \ | |
| | sort -nr \ | |
| | head -n 1 \ | |
| | cut -d' ' -f2- || true | |
| )" | |
| if [[ -z "$APP_PATH" ]]; then | |
| echo "ERROR: could not find built cmux DEV.app in DerivedData" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/startup-crash-probe-ci.sh` around lines 11 - 15, The APP_PATH
assignment uses a single find -print -quit which can return an arbitrary/stale
app; change it to collect all matching paths and deterministically pick the most
recent build (e.g., use find to list all matches for
"*/Build/Products/Debug/cmux DEV.app", then sort by modification time and select
the newest) before testing [[ -z "$APP_PATH" ]], updating the APP_PATH variable
assignment and keeping rest of the script intact (refer to the APP_PATH variable
and the existing find invocation).
| if kill -0 "$app_pid" 2>/dev/null; then | ||
| kill "$app_pid" 2>/dev/null || true | ||
| wait "$app_pid" 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
Bound shutdown wait to avoid CI hangs.
wait "$app_pid" can block indefinitely if TERM is ignored, stalling the probe/job until global timeout.
Proposed fix
if kill -0 "$app_pid" 2>/dev/null; then
kill "$app_pid" 2>/dev/null || true
- wait "$app_pid" 2>/dev/null || true
+ for _ in {1..20}; do
+ if ! kill -0 "$app_pid" 2>/dev/null; then
+ break
+ fi
+ sleep 0.25
+ done
+ if kill -0 "$app_pid" 2>/dev/null; then
+ kill -9 "$app_pid" 2>/dev/null || true
+ fi
+ wait "$app_pid" 2>/dev/null || true
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/startup-crash-probe-ci.sh` around lines 104 - 107, The current
shutdown sequence may hang because wait "$app_pid" can block indefinitely if the
process ignores TERM; update the block that uses app_pid (the kill -0 check,
kill "$app_pid", and wait "$app_pid") to perform a bounded wait instead of an
unbounded wait: either wrap the wait in a timeout (e.g., use the timeout utility
to limit wait to a few seconds) or replace the single wait with a short polling
loop that checks kill -0 "$app_pid" with a sleep and a max-iteration/countdown
to give up after a bounded interval, then proceed (ensure you still attempt kill
and consume exit status as before).
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/startup-crash-probe-ci.sh">
<violation number="1" location="scripts/startup-crash-probe-ci.sh:106">
P2: `wait "$app_pid"` will block indefinitely if the process ignores SIGTERM, stalling the CI job until the global workflow timeout. Add a bounded poll after `kill` and fall back to `kill -9` before calling `wait`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if kill -0 "$app_pid" 2>/dev/null; then | ||
| kill "$app_pid" 2>/dev/null || true | ||
| wait "$app_pid" 2>/dev/null || true |
There was a problem hiding this comment.
P2: wait "$app_pid" will block indefinitely if the process ignores SIGTERM, stalling the CI job until the global workflow timeout. Add a bounded poll after kill and fall back to kill -9 before calling wait.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/startup-crash-probe-ci.sh, line 106:
<comment>`wait "$app_pid"` will block indefinitely if the process ignores SIGTERM, stalling the CI job until the global workflow timeout. Add a bounded poll after `kill` and fall back to `kill -9` before calling `wait`.</comment>
<file context>
@@ -0,0 +1,113 @@
+
+ if kill -0 "$app_pid" 2>/dev/null; then
+ kill "$app_pid" 2>/dev/null || true
+ wait "$app_pid" 2>/dev/null || true
+ fi
+
</file context>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
LatestMacOSLaunchSmokeUITests, a startup smoke test that launches cmux and verifies it stays alive through startup on latest macOS CI runners~/.config/cmux/settings.json(app.appIcon = automatic) before launch so the test exercises the same managed app-icon startup path reported in Instant crash on macOS 26.4.1 #2763.github/workflows/ci-macos-compat.ymlon the macOS 26 lane (latest 26.x)Testing
gh workflow run test-e2e.yml --repo manaflow-ai/cmux -f ref=task-latest-macos-launch-crash-smoke -f test_filter=LatestMacOSLaunchSmokeUITests -f runner=depot-macos-latest -f record_video=false -f test_timeout=120(pass): https://github.com/manaflow-ai/cmux/actions/runs/24219770310compat-tests (warp-macos-26-arm64-6x, 30, false, true, true)for latest commit: pending at https://github.com/manaflow-ai/cmux/actions/runs/24220149171/job/70709620736Issues
Summary by CodeRabbit
Tests
Chores