Improve macOS CI runner fallbacks - #4922
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughAdjusts macOS runner selection and cache keys across CI workflows, replaces fragile Xcode app globbing with find/sort pipelines, adds run naming and concurrency to the E2E workflow, ensures xcodebuild receives ONLY_TESTING as a single argument, and adds guard tests validating the E2E workflow metadata and runner fallbacks. ChangesCI & workflow updates
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 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 updates macOS CI across multiple workflows: it switches Xcode discovery from
Confidence Score: 5/5Safe to merge — changes are limited to GitHub Actions workflow YAML and a CI guard shell script with no impact on application runtime, auth, or data paths. All changes are CI-only: Xcode selection is made consistent and correct across every workflow, the new lipo gate catches silent architecture regressions in the release build, and the E2E concurrency fix prevents duplicate queued runs from interfering with each other. No production Swift, runtime, or data code is touched. No files require special attention. The lipo validation step in ci.yml is the most impactful new gate and is correctly placed after the full universal build in the release-build job. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR[Pull Request / Push Event] --> CI[ci.yml]
DISPATCH[workflow_dispatch] --> E2E[test-e2e.yml]
DISPATCH --> PERF[perf-activation.yml]
CI --> RB[release-build\nwarp-macos-26-arm64-6x]
CI --> TESTS[tests\nwarp-macos-15-arm64-6x]
CI --> LAG[tests-build-and-lag\nwarp-macos-15-arm64-6x]
RB --> XCODE_RB[Select Xcode\nfind/sort/tail]
XCODE_RB --> BUILD[Build universal app\nARCHS=arm64 x86_64]
BUILD --> LIPO[Validate Release slices\nlipo -verify_arch arm64 x86_64\napp + CLI + ghostty]
E2E --> CONCUR{concurrency group\nrunner + ref_name + filter\ncancel-in-progress}
CONCUR --> E2E_JOB[e2e job\ndepot-macos-latest\nor depot-macos-14]
E2E_JOB --> TCC[Grant TCC permission\nno continue-on-error]
TCC --> XCODE_E2E[Select Xcode\nfind/sort/tail]
XCODE_E2E --> ONLY[xcodebuild\nONLY_TESTING quoted]
PERF --> PERF_JOB[activation-session\nwarp or depot runner]
style LIPO fill:#d4edda,stroke:#28a745
style CONCUR fill:#cce5ff,stroke:#004085
style TCC fill:#fff3cd,stroke:#856404
Reviews (10): Last reviewed commit: "Harden release-build fallback on hosted ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| concurrency: | ||
| group: e2e-${{ inputs.runner || 'blacksmith-6vcpu-macos-15' }}-${{ inputs.ref || github.ref }}-${{ inputs.test_filter }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
inputs.ref is provided. When left blank (default ""), the fallback is github.ref which produces a full path like refs/heads/main. When a user fills in inputs.ref with a short name (main), the key becomes e2e-<runner>-main-<filter> — a different string than e2e-<runner>-refs/heads/main-<filter>. Two runs targeting the same branch but one with an explicit ref and one with the default would land in separate concurrency groups and not cancel each other, defeating the de-duplication goal.
| concurrency: | |
| group: e2e-${{ inputs.runner || 'blacksmith-6vcpu-macos-15' }}-${{ inputs.ref || github.ref }}-${{ inputs.test_filter }} | |
| cancel-in-progress: true | |
| concurrency: | |
| group: e2e-${{ inputs.runner || 'blacksmith-6vcpu-macos-15' }}-${{ inputs.ref || github.ref_name }}-${{ inputs.test_filter }} | |
| cancel-in-progress: true |
![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 @.github/workflows/test-e2e.yml:
- Around line 65-69: The XCODE_APP fallback currently uses the pipeline "find
... | sort | head -n 1" which picks the lexicographically first match rather
than the newest Xcode; update the pipeline used in the XCODE_APP assignment so
it selects the highest-sorted candidate (e.g., use a reverse or version-aware
sort and take the first result, or keep the current sort and take the last
result) so that Xcode_16.x wins over Xcode_15.x; locate the XCODE_APP assignment
and replace the "find ... | sort | head -n 1" portion with a reverse/version
sort + head (or use tail) to pick the newest Xcode app.
🪄 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: 88408ee1-0e96-4ace-baaf-b966fd02ca50
📒 Files selected for processing (2)
.github/workflows/test-e2e.ymltests/test_ci_self_hosted_guard.sh
deac174 to
0e01585
Compare
lawrencecchen
changed the title
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_self_hosted_guard.sh`:
- Around line 31-36: Update the AWK check so saw_run_name only sets true when
run-name contains a dynamic interpolation token instead of any value; replace
the /^run-name:/ test with a pattern that matches run-name: lines containing
interpolation (for example /^\s*run-name:\s*.*\${{.*}}/ or
/^\s*run-name:\s*.*\$\{/) so the saw_run_name flag is only set when the run-name
is dynamically generated; keep the rest of the logic using in_concurrency,
saw_cancel and saw_test_filter unchanged.
🪄 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: 0ca2399c-dd68-4c40-91fb-d9e999652ba6
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/perf-activation.yml.github/workflows/test-e2e.ymltests/test_ci_self_hosted_guard.sh
0e01585 to
a9ad2e8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/test_ci_self_hosted_guard.sh (1)
31-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden
run-namevalidation to require dynamic interpolation.At Line 31, any static
run-name:passes. The guard should require dynamic interpolation tokens (and ideally the expected fields) to prevent silent regression of run metadata.Proposed hardening
if ! awk ' - /^run-name:/ { saw_run_name=1 } + /^run-name:/ { + saw_run_name=1 + if ($0 ~ /inputs\.test_filter/ && ($0 ~ /inputs\.runner/ || $0 ~ /blacksmith-6vcpu-macos-15/) && ($0 ~ /inputs\.ref/ || $0 ~ /github\.ref_name/)) { + saw_run_name_dynamic=1 + } + } /^concurrency:/ { in_concurrency=1; next } in_concurrency && /^jobs:/ { in_concurrency=0 } in_concurrency && /cancel-in-progress:[[:space:]]*true/ { saw_cancel=1 } in_concurrency && /inputs\.test_filter/ { saw_test_filter=1 } - END { exit !(saw_run_name && saw_cancel && saw_test_filter) } + END { exit !(saw_run_name && saw_run_name_dynamic && saw_cancel && saw_test_filter) } ' "$E2E_FILE"; then🤖 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 `@tests/test_ci_self_hosted_guard.sh` around lines 31 - 36, The current guard only checks for a static "run-name:" key (pattern /^run-name:/ and saw_run_name) which allows non-dynamic names; update the validation to require dynamic interpolation by changing the /^run-name:/ check to match interpolation tokens (e.g., require patterns like /\brun-name:[[:space:]]*.*(\${{[^}]+}}|\$\{[^}]+\}|{{[^}]+}})/) and optionally assert presence of expected fields such as github.run_id or github.ref within the matched token; adjust the saw_run_name logic to only set when that interpolation regex matches so the END exit condition enforces a dynamic run-name instead of any static value..github/workflows/test-e2e.yml (1)
65-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSelect the newest Xcode fallback (current logic picks the lexicographically first match).
The fallback uses
find ... | sort | head -n 1, which selects the lexicographically smallestXcode*.app. With multiple installs this would pickXcode_15.4.appoverXcode_16.2.app. The other workflows (ci.yml, perf-activation.yml) correctly usetail -n 1.🔧 Proposed fix
XCODE_APP="$( find /Applications -maxdepth 1 -name 'Xcode*.app' -print 2>/dev/null \ | sort \ - | head -n 1 \ + | tail -n 1 \ || true )"🤖 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/test-e2e.yml around lines 65 - 69, The Xcode fallback selects the lexicographically first match due to using "head -n 1" and should pick the newest install; update the find pipeline that assigns XCODE_APP (the block starting with XCODE_APP="$( find /Applications -maxdepth 1 -name 'Xcode*.app' -print 2>/dev/null | sort | head -n 1 || true") to use "tail -n 1" instead of "head -n 1" so the sorted list returns the latest Xcode; keep the surrounding find/sort/|| true structure unchanged.
🤖 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 @.github/workflows/test-e2e.yml:
- Around line 65-69: The Xcode fallback selects the lexicographically first
match due to using "head -n 1" and should pick the newest install; update the
find pipeline that assigns XCODE_APP (the block starting with XCODE_APP="$( find
/Applications -maxdepth 1 -name 'Xcode*.app' -print 2>/dev/null | sort | head -n
1 || true") to use "tail -n 1" instead of "head -n 1" so the sorted list returns
the latest Xcode; keep the surrounding find/sort/|| true structure unchanged.
In `@tests/test_ci_self_hosted_guard.sh`:
- Around line 31-36: The current guard only checks for a static "run-name:" key
(pattern /^run-name:/ and saw_run_name) which allows non-dynamic names; update
the validation to require dynamic interpolation by changing the /^run-name:/
check to match interpolation tokens (e.g., require patterns like
/\brun-name:[[:space:]]*.*(\${{[^}]+}}|\$\{[^}]+\}|{{[^}]+}})/) and optionally
assert presence of expected fields such as github.run_id or github.ref within
the matched token; adjust the saw_run_name logic to only set when that
interpolation regex matches so the END exit condition enforces a dynamic
run-name instead of any static value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96193f7d-56e0-4bc8-83ab-da5ba7a680b6
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/perf-activation.yml.github/workflows/test-e2e.ymltests/test_ci_self_hosted_guard.sh
706b271 to
dbf346e
Compare
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_self_hosted_guard.sh`:
- Around line 39-42: The concurrency guard can falsely pass if the
concurrency.group drops the runner dimension, so add a check that the
concurrency block explicitly includes a runner dimension; inside the awk block
that sets in_concurrency (where symbols like in_concurrency, saw_run_name,
saw_run_name_dynamic, saw_cancel, saw_test_filter, saw_ref_name are used) add a
new pattern such as: in_concurrency && /runner/ { saw_runner=1 } (or a more
specific match like /github\.runner/ or /runs-on/ if present), and include
saw_runner in the END exit condition alongside the other saw_* variables so the
script fails unless the runner dimension is present.
🪄 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: d966fcfc-1c65-41d4-b4e9-b9a450189f19
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/perf-activation.yml.github/workflows/test-e2e.ymltests/test_ci_self_hosted_guard.sh
583fae4 to
be186de
Compare
be186de to
47479d1
Compare
6df1cc3 to
f926594
Compare
f926594 to
c123649
Compare
fd6535a to
e8a8546
Compare
This branch was successfully deployed
1 participant
Summary
Validation
Summary by CodeRabbit
Note
Low Risk
Changes are limited to CI YAML, shell guards, and actionlint config; no application runtime or auth logic is modified.
Overview
macOS CI workflows now pick the newest installed Xcode with
find+sort+tailinstead ofls/glob ordering, applied consistently across build, test, release, nightly, E2E, and compat jobs.Release signal:
ci.ymlrelease-buildadds a post-build step that checks the Release app, bundledcmuxCLI, andghosttyhelper are executable and universal (lipo -verify_arch arm64 x86_64).tests/test_ci_self_hosted_guard.shenforces that plus the Xcode-selection rule.E2E / manual runs:
test-e2e.ymlgets a dynamicrun-name, concurrency that cancels duplicate queued runs for the same runner/ref/test filter, Depot runner options stay, TCC screen-recording no longer usescontinue-on-error, andxcodebuildpasses-only-testingvia a quoted argument (same fix intest-depot.yml).Other: New
.github/actionlint.yamlwhitelists Warp/Depot self-hosted labels; nightly env exports are batched; minor comment/loop cleanups inci.yml.Reviewed by Cursor Bugbot for commit e8a8546. Bugbot is set up for automated code reviews on this repo. Configure here.