fix: build macOS x64/arm64 separately to prevent Intel architecture contamination#720
Conversation
…tamination The macOS build ran on an ARM64 runner but built both architectures in a single electron-builder invocation. Since npmRebuild was disabled, native modules (node-pty, better-sqlite3) were only compiled for the host arch (arm64), causing the x64 package to ship arm64 binaries. This mirrors the Linux fix from #116: build each architecture separately with explicit native module rebuild and verification before packaging. Closes #719
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRelease workflow split macOS packaging into per-architecture steps, moved native rebuild/verification into shared scripts, simplified mac build targets in package.json, normalized release notes formatting, and updated a test to assert the new scripts are referenced. Changes
Sequence Diagram(s)sequenceDiagram
participant Actions as GitHub Actions (release.yml)
participant Script as rebuild-and-verify-native.sh
participant Rebuild as electron-rebuild
participant Verifier as verify-native-arch.sh
participant Builder as electron-builder
participant NodeMods as node_modules
Actions->>Script: invoke ./.github/scripts/rebuild-and-verify-native.sh <arch>
Script->>NodeMods: remove build/prebuilds for native modules
Script->>Rebuild: run npx electron-rebuild --arch=<arch> --force
Rebuild-->>NodeMods: produce rebuilt native binaries
Script->>Verifier: call verify-native-arch.sh <arch> (or run file checks)
Verifier->>NodeMods: inspect binary files with file command
Verifier-->>Script: success/failure
Script-->>Actions: exit status
Actions->>Builder: run electron-builder --<arch> --config.extraMetadata.version=...
Builder-->>Actions: packaged artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 fixes Intel Mac terminal failures caused by arm64 native binaries being packaged into the x64 DMG. The root cause was that the macOS CI runner (arm64) built both architectures in a single Key changes:
Issues found:
Confidence Score: 4/5Safe to merge — the fix correctly addresses the root cause; two minor issues (missing The architecture separation pattern mirrors the already-working Linux approach and includes solid pre-packaging verification gates. The only actionable issues are that
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[macOS CI Runner\nmacos-latest / arm64] --> B[Checkout + npm ci]
B --> C[Build Application\nnpm run build]
C --> D[Import Apple Certificate]
D --> E["Package for macOS x64"]
E --> E1["Clean node-pty + better-sqlite3\nbuild dirs"]
E1 --> E2["electron-rebuild --arch=x64 --force"]
E2 --> E3{"Verify pty.node\ncontains x86_64?"}
E3 -->|No| E4["❌ Exit 1"]
E3 -->|Yes| E5{"Verify better_sqlite3.node\ncontains x86_64?"}
E5 -->|No| E4
E5 -->|Yes| E6["electron-builder --mac --x64\n→ DMG + ZIP artifacts"]
D --> F["Package for macOS arm64"]
F --> F1["Clean node-pty + better-sqlite3\nbuild dirs"]
F1 --> F2["electron-rebuild --arch=arm64 --force"]
F2 --> F3{"Verify pty.node\ncontains arm64?"}
F3 -->|No| F4["❌ Exit 1"]
F3 -->|Yes| F5{"Verify better_sqlite3.node\ncontains arm64?"}
F5 -->|No| F4
F5 -->|Yes| F6["electron-builder --mac --arm64\n→ DMG + ZIP artifacts"]
E6 --> G[Upload Release Assets]
F6 --> G
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
.github/workflows/release.yml
Outdated
|
|
||
| # Rebuild native modules explicitly for x64 | ||
| echo "Rebuilding native modules for x64..." | ||
| npx electron-rebuild --arch=x64 --force |
There was a problem hiding this comment.
-w module filter in electron-rebuild
The postinstall script intentionally limits the rebuild to only the two native modules that need architecture-specific binaries (-w node-pty,better-sqlite3):
electron-rebuild -f -w node-pty,better-sqlite3
The new macOS x64 step calls electron-rebuild without a -w filter, which will attempt to rebuild all native modules found in node_modules. If any native module (e.g., fsevents or other platform-specific addons) doesn't cleanly support cross-compilation from arm64 → x64, the step will fail.
The same issue exists in the arm64 step at line 414.
| npx electron-rebuild --arch=x64 --force | |
| npx electron-rebuild --arch=x64 --force -w node-pty,better-sqlite3 |
.github/workflows/release.yml
Outdated
|
|
||
| # Rebuild native modules explicitly for arm64 | ||
| echo "Rebuilding native modules for arm64..." | ||
| npx electron-rebuild --arch=arm64 --force |
There was a problem hiding this comment.
-w module filter in electron-rebuild (arm64 step)
Same issue as the x64 step — no -w filter means all native modules will be rebuilt. For consistency with postinstall and to reduce risk of unrelated native modules failing cross-compilation, scope the rebuild to just the two required modules.
| npx electron-rebuild --arch=arm64 --force | |
| npx electron-rebuild --arch=arm64 --force -w node-pty,better-sqlite3 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
330-451: Consider: Both macOS architectures built sequentially on same runner.The sequential x64→arm64 build on a single runner (macos-latest, which is arm64) works correctly because each step cleans and rebuilds native modules. However, this approach has trade-offs:
Pros:
- Uses only one runner/job
- Certificate import happens once
Trade-off: If the x64 step fails after
electron-builderpartially completes, the arm64 step could succeed, but the release artifacts might include partial x64 files. Theif-no-files-found: erroron artifact upload (line 692) should catch missing expected files.This is acceptable for the current design. Just noting it for visibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 330 - 451, Add an explicit post-package verification after the "Package for macOS x64" step (the block that runs npx electron-builder --mac --x64 ...) to confirm the expected x64 artifact (e.g., .dmg/.zip/.app in the dist output) exists and fail early if missing; keep the existing cleanup/rebuild logic but add a simple file-existence check and exit 1 with a clear message if the artifact is not found so the subsequent arm64 build doesn't proceed with partial x64 outputs (this complements the existing if-no-files-found: error behavior used during artifact upload).docs/releases.md (1)
202-202: Optional: Correct "Github" capitalization.The official brand name is "GitHub" with a capital "H". As per static analysis hints, line 202 uses "Github Worktree" instead of "GitHub Worktree".
📝 Suggested fix
-🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`. +🌳 GitHub Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 202, Replace the incorrect "Github Worktree" capitalization with the official "GitHub Worktree" in the release note text (the string containing "Github Worktree was added. Any agent bound to a Git repository...") so the brand name is correctly rendered as "GitHub" throughout that sentence; ensure any other occurrences of "Github" in the same paragraph are updated to "GitHub".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 330-451: Add an explicit post-package verification after the
"Package for macOS x64" step (the block that runs npx electron-builder --mac
--x64 ...) to confirm the expected x64 artifact (e.g., .dmg/.zip/.app in the
dist output) exists and fail early if missing; keep the existing cleanup/rebuild
logic but add a simple file-existence check and exit 1 with a clear message if
the artifact is not found so the subsequent arm64 build doesn't proceed with
partial x64 outputs (this complements the existing if-no-files-found: error
behavior used during artifact upload).
In `@docs/releases.md`:
- Line 202: Replace the incorrect "Github Worktree" capitalization with the
official "GitHub Worktree" in the release note text (the string containing
"Github Worktree was added. Any agent bound to a Git repository...") so the
brand name is correctly rendered as "GitHub" throughout that sentence; ensure
any other occurrences of "Github" in the same paragraph are updated to "GitHub".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79096e01-b485-4668-8144-ef407dd6f8b9
📒 Files selected for processing (3)
.github/workflows/release.ymldocs/releases.mdpackage.json
|
Thanks for the contribution, @pedramamini! 🙌 Reviewed the changes — the approach is solid. Splitting the macOS build into separate x64/arm64 steps with per-architecture rebuild + verification gates correctly addresses the root cause (arm64 runner contaminating x64 packages). The pattern mirrors the existing Linux fix from #116, which is great for consistency. The LGTM — approving. |
Deduplicates ~260 lines of copy-pasted clean/rebuild/verify logic across macOS x64, macOS arm64, Linux x64, and Linux ARM64 packaging steps into two reusable scripts: - rebuild-and-verify-native.sh: clean + rebuild + verify (used before packaging) - verify-native-arch.sh: verify only (used after initial postinstall) Also drops the unrelated docs/releases.md whitespace-only changes that inflated the original PR diff, and adds --ignore-unknown to prettier in lint-staged so shell scripts and other non-parseable files don't fail the pre-commit hook.
1 participant
Summary
electron-rebuild --arch=<target>) and architecture verification before packagingarcharrays from macOS targets inpackage.json, letting CLI flags (--x64/--arm64) control what gets built — mirroring the Linux fix from ARM pty.node shipping with non ARM Linux releases... #116filechecks forpty.nodeandbetter_sqlite3.nodeon macOS (previously only Linux had these safeguards)Root cause: The macOS CI ran on an ARM64 runner and built both architectures in a single
electron-builderinvocation. WithnpmRebuild: false, native modules were only compiled for the host arch (arm64), so the x64 package shipped arm64 binaries — breaking terminal on Intel Macs.Closes #719
Test plan
fileonpty.nodeinside each packaged app shows the correct architectureSummary by CodeRabbit