Add universal macOS release artifact guard - #4744
austinywang wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. 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 reusable Bash verifier for macOS universal app bundles, integrates it into CI workflows and the build script, and adds integration tests validating success and multiple failure cases. ChangesUniversal Binary Verification Script
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (17 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 |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
Greptile SummaryCentralizes macOS universal-binary enforcement into a single post-build verifier (
Confidence Score: 5/5Changes are confined to CI/CD guards, build scripts, and test scaffolding with no runtime app logic touched; all release paths now enforce the shared SDK+arch contract. The verifier script is well-structured with robust argument guards. All four release paths now call it with --require-sdk-prefix "26.", previously flagged gaps are closed, and the fixture-based test suite covers both the success and failure branches. The one noted gap — no automated check that nightly.yml continues to pass --require-sdk-prefix — represents minor test coverage debt, not a correctness risk in this PR. No files require special attention. The comment in tests/test_ci_release_sdk_lane.sh overstates nightly coverage but the nightly workflow itself is correct as shipped. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[xcodebuild universal build\nARCHS=arm64 x86_64\nONLY_ACTIVE_ARCH=NO] --> B[.app artifact produced]
B --> C[verify-universal-macos-app.sh]
C --> D{Info.plist readable?}
D -- Yes --> E[Read CFBundleExecutable]
D -- No --> F[Fallback: basename .app]
E --> G[verify_binary_archs: app binary]
F --> G
G --> H[verify_binary_archs: CLI binary]
H --> I[verify_binary_archs: Ghostty helper]
I --> J{--require-sdk-prefix set?}
J -- No --> K[PASS]
J -- Yes --> L[otool LC_BUILD_VERSION sdk check]
L --> M{prefix matches?}
M -- Yes --> K
M -- No --> N[FAIL: wrong SDK]
subgraph callers [Callers]
P1[release.yml\n--require-sdk-prefix 26.]
P2[nightly.yml\n--require-sdk-prefix 26.]
P3[ci.yml release-build\n--require-sdk-prefix 26.]
P4[build-sign-upload.sh\n--require-sdk-prefix 26.]
end
callers --> C
Reviews (8): Last reviewed commit: "fix: address release verifier review fee..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 `@scripts/verify-universal-macos-app.sh`:
- Around line 17-20: Guard the shift in the --label case so the script doesn't
"shift count out of range" when the flag is the last argument: in the --label)
branch (referencing LABEL and shift 2), check whether a second argument exists
(e.g. test $# -ge 2 or test -n "${2:-}") and only then set LABEL from $2 and
shift 2; otherwise set LABEL to empty (or leave as default) and shift 1 to
consume the --label token so the later "Missing value for --label" validation
can run.
🪄 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: 134ca6f9-c2b8-4747-8f79-7b9e8c8ea4f8
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.ymlscripts/build-sign-upload.shscripts/verify-universal-macos-app.shtests/test_verify_universal_macos_app.sh
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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/ci.yml:
- Around line 10-13: The workflow-wide permissions block currently grants broad
write rights via the actions: write entry; remove the actions: write line
(leaving contents: read) or instead move and scope actions: write to only the
specific job that needs it by adding a per-job permissions block; update the
permissions block to the minimal scope required and ensure any job-level
permission overrides use actions: write only where strictly necessary.
🪄 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: b7336aa3-7f87-4232-a117-928e6bd68767
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.yml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 10-13: The workflow-wide permissions block currently grants broad
write rights via the actions: write entry; remove the actions: write line
(leaving contents: read) or instead move and scope actions: write to only the
specific job that needs it by adding a per-job permissions block; update the
permissions block to the minimal scope required and ensure any job-level
permission overrides use actions: write only where strictly necessary.
🪄 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: b7336aa3-7f87-4232-a117-928e6bd68767
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.yml
🛑 Comments failed to post (1)
.github/workflows/ci.yml (1)
10-13:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow workflow token scope at Line 12 (
actions: write).
actions: writeat workflow scope is broader than required by the visible CI steps and increases blast radius if any CI step is compromised. Keep least privilege by removing it (or scoping write only to the specific job that truly needs it).Suggested minimal hardening
permissions: contents: read - actions: write + actions: read📝 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.permissions: contents: read actions: read🧰 Tools
🪛 zizmor (1.25.2)
[error] 12-12: overly broad permissions (excessive-permissions): actions: write is overly broad at the workflow level
(excessive-permissions)
[warning] 12-12: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 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/ci.yml around lines 10 - 13, The workflow-wide permissions block currently grants broad write rights via the actions: write entry; remove the actions: write line (leaving contents: read) or instead move and scope actions: write to only the specific job that needs it by adding a per-job permissions block; update the permissions block to the minimal scope required and ensure any job-level permission overrides use actions: write only where strictly necessary.Source: Linters/SAST tools
| echo "Ghostty theme picker helper not found at $HELPER_PATH" >&2 | ||
| exit 1 | ||
| fi | ||
| ./scripts/verify-universal-macos-app.sh "$APP_PATH" --label "Release app" |
There was a problem hiding this comment.
--require-sdk-prefix "26.", which means an app accidentally built against macOS 15 (or any non-26 SDK) would pass this gate and continue through signing, notarization, and upload. Every other release path — release.yml, nightly.yml, and CI release-build — all pass --require-sdk-prefix "26.". The PR explicitly lists this script as one of the hardened paths.
| ./scripts/verify-universal-macos-app.sh "$APP_PATH" --label "Release app" | |
| ./scripts/verify-universal-macos-app.sh "$APP_PATH" --label "Release app" --require-sdk-prefix "26." |
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 121-124: The current check uses a substring match on the variable
section which can match comments or unrelated text; change the condition to
match a command token boundary so it only passes when the verifier is actually
invoked. Replace the glob test on section with a word-boundary regex using
Bash's =~ operator, e.g. test that section matches
'(^|[[:space:]])\./scripts/verify-universal-macos-app.sh([[:space:]]|$)' so the
script name is a standalone command token (allowing trailing args) when checking
in tests/test_ci_self_hosted_guard.sh where variable section is evaluated.
🪄 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: cf0f0cfb-a9ec-455d-8de3-f4425ce1f4e6
📒 Files selected for processing (1)
tests/test_ci_self_hosted_guard.sh
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 de49750. Configure here.
|
cmux-reconcile: partly-useful Usefulness verdict: Evaluate only guard consolidation and test value; the absence of architecture checks is no longer a valid premise. The diff introduces a shared Reviewed patch head: Older issue/PR tracking index — remaining scope and competing implementations are recorded there. |
|
Mac fleet instructions for head JOB_JSON=$(~/.local/bin/cmux-ci submit --kind cmux --command 'CMUX_FLEET_BUILD_TAG=pr-4744-9822afe4 /Users/Shared/cmux-build-fleet/recipes/cmux.sh https://github.com/manaflow-ai/cmux.git 9822afe475cac02d681aaca76b6c6ade7c7e855b' --artifact artifacts/cmux.app.zip --workspace https://github.com/manaflow-ai/cmux/pull/4744 --source-digest 9822afe475cac02d681aaca76b6c6ade7c7e855b --cache-key cmux:pr-4744 --min-free-bytes 268435456000 --label cmux --label ram48)
JOB_ID=$(python3 -c 'import json,sys; print(json.load(sys.stdin)["id"])' <<<"$JOB_JSON")
~/.local/bin/cmux-ci wait "$JOB_ID" --receipt artifacts/fleet/$JOB_ID.json
~/.local/bin/cmux-ci publish-hq "$JOB_ID"Use an existing campaign job ID if one is already posted; do not submit a duplicate. A wait timeout leaves the remote job running. Published results will include an exact-head artifact link and timing/disk receipt. This recipe validates the macOS app only, not iOS or tests. Never use maclease or put credentials in a PR comment. |
|
Universal macOS artifact checks are now part of the current release pipeline; this PR is superseded by the main CI contract in 8dd69c0. |
This branch was successfully deployed
3 participants
Summary
Refs #293.
This is a scoped first increment for Intel Mac support durability. Current main already builds Release and Nightly macOS artifacts as universal binaries; this PR turns that into one shared artifact-level contract so release paths cannot silently ship an arm64-only app, embedded CLI, or Ghostty helper.
scripts/verify-universal-macos-app.shto verify the app executable, bundledcmuxCLI, and bundledghosttyhelper all containarm64andx86_64slices.release-build, and the manualscripts/build-sign-upload.shpath.ARCHS="arm64 x86_64",ONLY_ACTIVE_ARCH=NO, andgeneric/platform=macOS.lipoover fixture app artifacts.Architecture note
The build system had multiple owners for the same invariant: Xcode project Release settings, release/nightly workflow inline
lipochecks, CI release-build, the Ghostty helper run script, and the manual signing script. The stronger boundary is a single verifier that inspects the produced.appartifact after Xcode has run. That makes the bad state observable at the artifact boundary, not inferred from workflow text or project settings.Remaining outside this increment: a real Intel-hardware launch smoke. This PR verifies the shipped Mach-O slices for the app, CLI, and helper; it does not provision an Intel Mac or add an x86 runtime UI test.
Test plan
Not run locally per task and repo instructions. CI should run the new workflow guard test and the macOS
release-buildartifact verification.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Changes are build/CI guardrails and permission tightening around release artifacts, not runtime app logic or auth/data paths.
Overview
Introduces
scripts/verify-universal-macos-app.shas the single post-build contract for Release macOS.appartifacts: the main executable, bundledcmuxCLI, andghosttyhelper must each include arm64 and x86_64 slices, with an optional--require-sdk-prefix(e.g.26.) on the app binary.Replaces duplicated inline
lipo/otoolchecks in CIrelease-build, nightly, and release workflows with calls to that script.scripts/build-sign-upload.shnow forces a universalxcodebuild(ARCHS,ONLY_ACTIVE_ARCH=NO, generic macOS destination) and runs the verifier before signing.CI hygiene: workflow default permissions drop
actions: write;release-ghostty-cli-helper/release-buildget scoped job permissions.workflow-guard-testsgainstests/test_verify_universal_macos_app.sh; existing lane/self-hosted guard tests assert workflows and the manual script use the verifier with--require-sdk-prefix "26.".Reviewed by Cursor Bugbot for commit 9822afe. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a universal macOS artifact verifier to ensure the app, bundled
cmux, andghosttyhelper are universal (arm64 + x86_64) and built with the macOS 26 SDK. Centralizes checks across nightly/release/CI and the manual signing path, and tightens CI permissions (refs #293).New Features
scripts/verify-universal-macos-app.sh(readsInfo.plist, supports--labeland--require-sdk-prefix) to verify app, CLI, helper slices and SDK.--require-sdk-prefix "26."; CI runstests/test_verify_universal_macos_app.sh.scripts/build-sign-upload.shnow builds universal via generic macOS destination and runs the verifier before signing.Bug Fixes
--labeland--require-sdk-prefixrequire values with clear errors.release-builduses the universal verifier and that workflows/manual script pass--require-sdk-prefix "26.".Written for commit 9822afe. Summary will update on new commits.
Summary by CodeRabbit
Chores
Tests