Repository navigation
Fix main macOS CI - #3643
Merged
Merged
Fix main macOS CI#3643
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCentralizes Zig installation into a new ChangesCI/CD Zig Installation Centralization
Keyboard Shortcut Focus Resolution & Test Isolation
CLI Socket Timeout Configuration
Sequence Diagram(s)sequenceDiagram
participant Shortcut as Shortcut Event
participant Window as NSWindow
participant Resolver as shortcutFocusedBrowserPanel(in:)
participant TabMgr as TabManager
participant Panel as BrowserPanel
Shortcut->>Window: event dispatched (shortcut, web inspector, etc.)
Window->>Resolver: resolve focused panel for this window
Resolver-->>Panel: returns focused BrowserPanel (if any)
alt Panel found
Resolver-->>Shortcut: dispatch action targeting Panel
else Panel not found
Window->>TabMgr: ask for global focused panel
TabMgr-->>Resolver: return focused BrowserPanel or nil
Resolver-->>Shortcut: dispatch action or return nil
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
Greptile SummaryThis PR fixes the red macOS CI lanes by centralising Zig installation into a verified script with mirror-fallback, stabilising CLI socket timeouts on reused connections, and isolating shortcut tests from the developer's on-disk settings.
Confidence Score: 5/5Safe to merge; all three change areas are well-scoped fixes with no new structural risk introduced. The socket timeout change eliminates the stale-timeout window on reused connections by tracking actual socket state. The shortcut routing refactor is a pure extraction with no new logic surface. CI and test isolation changes are infrastructure-only with no production runtime impact. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant SocketClient
participant Darwin
Note over SocketClient: connectOnce()
SocketClient->>Darwin: configureSocketWriteSafety(responseTimeout)
SocketClient->>Darwin: configureReceiveTimeout(responseTimeout)
Darwin-->>SocketClient: lastConfiguredReceiveTimeout = responseTimeout
Note over Caller,Darwin: First send()
Caller->>SocketClient: send(command)
SocketClient->>SocketClient: lastConfigured == initial? skip setsockopt
SocketClient->>Darwin: read()
Darwin-->>SocketClient: sawNewline -> currentTimeout = multilineIdle
SocketClient->>Darwin: configureReceiveTimeout(multilineIdle)
Darwin-->>SocketClient: EAGAIN -> complete
Note over Caller,Darwin: Second send() on reused socket
Caller->>SocketClient: send(command)
SocketClient->>SocketClient: lastConfigured(multilineIdle) != responseTimeout
SocketClient->>Darwin: configureReceiveTimeout(responseTimeout)
Darwin-->>SocketClient: timeout reset correctly
Reviews (3): Last reviewed commit: "Address CI review feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+56
to
+61
| if ! download_file "$ZIG_MIRROR_URL" "$ZIG_TAR"; then | ||
| echo "Mirror download failed; retrying from ${ZIG_OFFICIAL_URL}" >&2 | ||
| download_file "$ZIG_OFFICIAL_URL" "$ZIG_TAR" | ||
| fi | ||
| download_file "${ZIG_OFFICIAL_URL}.minisig" "$ZIG_SIG" | ||
| minisign -Vm "$ZIG_TAR" -x "$ZIG_SIG" -P "$ZIG_MINISIGN_PUBLIC_KEY" |
Contributor
There was a problem hiding this comment.
The script falls back to the official URL only when download_file itself fails (e.g., HTTP error, network timeout). If the mirror returns HTTP 200 but serves stale or wrong content — for example a cached 0.14.x tarball while CI expects 0.15.2 — minisign will fail and set -euo pipefail will abort the script with no further attempt. CI would then be stuck until someone overrides ZIG_MIRROR_URL manually.
Comment on lines
+1046
to
+1050
| var configuredReceiveTimeout = Self.responseTimeoutSeconds | ||
| if initialResponseTimeout != configuredReceiveTimeout { | ||
| try configureReceiveTimeout(initialResponseTimeout) | ||
| configuredReceiveTimeout = initialResponseTimeout | ||
| } |
Contributor
There was a problem hiding this comment.
configuredReceiveTimeout is initialised to Self.responseTimeoutSeconds, but that only matches the socket's actual state immediately after connectOnce(). For non-relay sockets the connection is reused across multiple send() calls; if a previous call entered the multiline-response phase it leaves the socket with Self.multilineResponseIdleTimeoutSeconds (0.12 s). The next call with responseTimeout == nil evaluates initialResponseTimeout == configuredReceiveTimeout as equal and skips the pre-loop setsockopt. The first read() then hits EAGAIN after 0.12 s instead of the expected initial timeout, and the error branch at !sawNewline throws "Command timed out" on a healthy socket.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
53-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI blocker: this file is over the enforced Swift length budget.
The pipeline is failing with
actual=5577vsbudget=5556. Please split part of this class (for example, move the isolated keyboard-shortcut settings harness and related tests into a companion test file) so CI can pass without budget override.Also applies to: 125-129
🤖 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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 53 - 106, This file exceeds the Swift length budget; extract the isolated keyboard-shortcut settings harness and its related tests into a new companion test file: move the temporarySettingsDirectoryURL property, makeKeyEvent(_:characters:charactersIgnoringModifiers:keyCode:), setUp() teardown/cleanup logic that manipulates executionTimeAllowance, actionsWithPersistedShortcut, savedShortcutsByAction, originalSettingsFileStore, and the code that assigns KeyboardShortcutSettings.settingsFileStore to a KeyboardShortcutSettingsFileStore into the new test file (also move any tests referencing those symbols), update imports and test class name accordingly, and ensure the new file contains the matching tearDown that restores KeyboardShortcutSettings.settingsFileStore and persisted UserDefaults; apply the same extraction for the code referenced at lines ~125-129 that belongs to this harness.
🤖 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 `@CLI/cmux.swift`:
- Around line 1046-1050: The local variable configuredReceiveTimeout in send()
can drift from the actual socket SO_RCVTIMEO across calls when relayEndpoint ==
nil; fix by ensuring the socket's timeout is restored at the start of every
send(): either unconditionally call
configureReceiveTimeout(initialResponseTimeout) at the top of send(), or add an
instance property (e.g., lastConfiguredReceiveTimeout) on SocketClient and
update/compare that instead of a local var so the guard reflects real socket
state; update configureReceiveTimeout and places that change the timeout
(including the multilineResponseIdleTimeoutSeconds adjustment) to maintain
lastConfiguredReceiveTimeout whenever the kernel socket option is modified.
In `@scripts/install-zig-ci.sh`:
- Around line 56-60: The script only falls back to the official URL for the
tarball but always downloads the .minisig from ZIG_OFFICIAL_URL; update the
.minisig download to mirror the tarball logic by first trying download_file
"$ZIG_MIRROR_URL".minisig to "$ZIG_SIG" and, on failure, echo a retry message
and call download_file "$ZIG_OFFICIAL_URL".minisig to "$ZIG_SIG" (use the same
retry/fallback pattern and the existing variables ZIG_MIRROR_URL,
ZIG_OFFICIAL_URL, ZIG_TAR, ZIG_SIG and the download_file function).
- Around line 7-9: The startup check currently uses a prefix regex (grep -q
"^${ZIG_REQUIRED}") which can falsely match longer versions; change the version
check to require exact equality of the output of `zig version` against the
ZIG_REQUIRED variable (e.g., compare the full version string or use grep -x for
exact match) so only the exact version is accepted; additionally, update the
minisig download logic (the same area that fetches the tarball/mirror fallback)
to try the mirror minisig URL first and fall back to the official minisig URL on
failure just like the tarball fetch does, ensuring both tarball and minisig
follow the same mirror fallback behavior (refer to the ZIG_REQUIRED variable,
the initial zig version check block, and the minisig/tarball download code
around the mirror fallback).
In `@Sources/KeyboardShortcutContext.swift`:
- Around line 133-141: The helper shortcutFocusedBrowserPanel(in:) wrongly falls
back to the global tabManager when a specific NSWindow was supplied but not
found in mainWindowContexts, allowing cross-window shortcut routing; change
shortcutFocusedBrowserPanel(in:) so that if a non-nil window is passed and no
matching context is found it returns nil (instead of returning
tabManager?.focusedBrowserPanel), otherwise keep existing behavior for nil
window by returning tabManager?.focusedBrowserPanel; update references to
mainWindowContexts, tabManager, and focusedBrowserPanel inside
shortcutFocusedBrowserPanel(in:) accordingly.
---
Outside diff comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 53-106: This file exceeds the Swift length budget; extract the
isolated keyboard-shortcut settings harness and its related tests into a new
companion test file: move the temporarySettingsDirectoryURL property,
makeKeyEvent(_:characters:charactersIgnoringModifiers:keyCode:), setUp()
teardown/cleanup logic that manipulates executionTimeAllowance,
actionsWithPersistedShortcut, savedShortcutsByAction, originalSettingsFileStore,
and the code that assigns KeyboardShortcutSettings.settingsFileStore to a
KeyboardShortcutSettingsFileStore into the new test file (also move any tests
referencing those symbols), update imports and test class name accordingly, and
ensure the new file contains the matching tearDown that restores
KeyboardShortcutSettings.settingsFileStore and persisted UserDefaults; apply the
same extraction for the code referenced at lines ~125-129 that belongs to this
harness.
🪄 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: fe35da4d-2513-447c-a7ee-3e20b1951783
📒 Files selected for processing (14)
.circleci/config.yml.github/workflows/build-ghosttykit.yml.github/workflows/ci-macos-compat.yml.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/perf-activation.yml.github/workflows/release.yml.github/workflows/test-depot.yml.github/workflows/test-e2e.ymlCLI/cmux.swiftSources/KeyboardShortcutContext.swiftcmuxTests/AppDelegateRenameShortcutContextTests.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftscripts/install-zig-ci.sh
This was referenced May 6, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the current red main macOS CI lanes at commit
65706d036f138628d5337188ca51f5f34acb566c.Changes:
cmux.jsonLocal verification:
bash -n scripts/install-zig-ci.sh.circleci/config.ymland.github/workflows/*.yml./tests/test_ci_self_hosted_guard.sh./tests/test_ci_create_dmg_pinned.shbash tests/test_ci_unit_test_spm_retry.shbash tests/test_ci_ghosttykit_checksum_verification.shbash tests/test_ci_ghosttykit_checksum_present.shAppDelegateShortcutRoutingTestsandAppDelegateRenameShortcutContextTests/tmp/cmux-mainci-releasederived dataSummary by cubic
Fixes failing macOS CI by introducing a verified Zig installer with mirror fallback and using it across all workflows. Also stabilizes CLI socket timeouts and tightens shortcut routing and tests to reduce flakes.
scripts/install-zig-ci.sh(minisign verification, retrying mirror fallback, arch autodetect) across CircleCI and all GitHub workflows.setsockoptcalls; include errno and reason in errors.KeyboardShortcutTestSettingsIsolation.installIsolatedTestFileStoreto isolate settings in tests; update keybinding expectation to Cmd+Option+E and wrap debug shortcut test with a temporary unbind to prevent flakes.Written for commit 618d619. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests
Chores