docs(release): correct rationale comment on macos-latest runner#11446
Conversation
The previous comment attributed the darwin SEA crashes to ldid producing bad page hashes, but the upstream minimal `node --build-sea` + `codesign` repro (nodejs/node#62893) shows codesign-signed binaries crash too. The bug is in LIEF's Mach-O surgery during --build-sea, not in signing. Rewrite the comment to state the actual reasons the job runs on macOS (native codesign avoids building ldid; macos-latest is Apple Silicon so verify-binary.mjs can smoke-test the darwin-arm64 SEA) and explicitly note that this does NOT fix the darwin-x64 crash. Comment-only change. No behaviour change.
📝 WalkthroughWalkthroughThe PR updates the macOS job comments in the release workflow, replacing outdated notes about Linux cross-signing with clarifications about native macOS ChangesWorkflow Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the explanatory comment in the release workflow to reflect the current understanding of why the release job runs on macos-latest. It keeps the workflow behavior unchanged while correcting the rationale around macOS signing and SEA verification.
Changes:
- Replaces the outdated
ldid/page-hash explanation with the current rationale for usingmacos-latest. - Clarifies that native
codesignand in-placedarwin-arm64verification are the reasons for the macOS runner choice. - Notes that the
darwin-x64crash remains an upstream Node.js SEA issue rather than something this workflow setting fixes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release.yml (2)
14-17: ⚡ Quick winMake the
macos-latest/arm64 assumption more future-proof (and linkable).This comment is likely correct, but it’s currently stated without a pointer. GitHub’s docs map
macos-latestto an arm64 (M1) runner label (Intel is available via explicitmacos-*-intellabels), so consider referencing that (or rephrasing to emphasize “as of now,macos-latestis arm64”). (docs.github.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 14 - 17, Update the comment around the macOS runner block to qualify the `macos-latest`/arm64 assumption: reference GitHub docs (https://docs.github.com/en/actions/reference/github-hosted-runners-reference) or rephrase to say “as of [date]/currently `macos-latest` maps to an arm64 runner” and mention the Intel labels (`macos-*-intel`) for contrast; keep notes about why `macos-latest` is chosen for ad-hoc `codesign` and for running `verify-binary.mjs` on the darwin-arm64 SEA, but add the doc link or “as of” qualifier to make the comment future-proof and linkable.
18-19: ⚡ Quick winReplace the vague “see pack-app docs” with a concrete reference.
“see pack-app docs” is hard to act on later—please point to the exact doc URL or repo path/section that explains the upstream SEA
darwin-x64crash vs signing. Also, yournodejs/node#62893reference aligns with the upstream report: Intel Mac segfault afternode --build-sea ...followed bycodesign --sign -, while arm64 doesn’t repro. (github.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 18 - 19, Replace the vague "see pack-app docs" text with a concrete reference: update the comment that currently reads "see pack-app docs" to include the exact pack-app docs URL or repo path/section that explains the upstream SEA darwin-x64 crash vs signing (replace the placeholder text "see pack-app docs" with that link/path), add the upstream Node.js issue URL (https://github.com/nodejs/node/issues/62893) and a one-line parenthetical summary like "(upstream SEA darwin-x64 crash with codesign, see linked issue and docs for details)"; ensure the new comment still mentions nodejs/node#62893 and provides both the authoritative pack-app docs location and the upstream issue link for future reference.
🤖 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 14-17: Update the comment around the macOS runner block to qualify
the `macos-latest`/arm64 assumption: reference GitHub docs
(https://docs.github.com/en/actions/reference/github-hosted-runners-reference)
or rephrase to say “as of [date]/currently `macos-latest` maps to an arm64
runner” and mention the Intel labels (`macos-*-intel`) for contrast; keep notes
about why `macos-latest` is chosen for ad-hoc `codesign` and for running
`verify-binary.mjs` on the darwin-arm64 SEA, but add the doc link or “as of”
qualifier to make the comment future-proof and linkable.
- Around line 18-19: Replace the vague "see pack-app docs" text with a concrete
reference: update the comment that currently reads "see pack-app docs" to
include the exact pack-app docs URL or repo path/section that explains the
upstream SEA darwin-x64 crash vs signing (replace the placeholder text "see
pack-app docs" with that link/path), add the upstream Node.js issue URL
(https://github.com/nodejs/node/issues/62893) and a one-line parenthetical
summary like "(upstream SEA darwin-x64 crash with codesign, see linked issue and
docs for details)"; ensure the new comment still mentions nodejs/node#62893 and
provides both the authoritative pack-app docs location and the upstream issue
link for future reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa14b6c9-e883-44ac-8308-1e9694b5fdba
📒 Files selected for processing (1)
.github/workflows/release.yml
The previous comment attributed the darwin SEA crashes to ldid producing bad page hashes, but the upstream minimal `node --build-sea` + `codesign` repro (nodejs/node#62893) shows codesign-signed binaries crash too. The bug is in LIEF's Mach-O surgery during --build-sea, not in signing. Rewrite the comment to state the actual reasons the job runs on macOS (native codesign avoids building ldid; macos-latest is Apple Silicon so verify-binary.mjs can smoke-test the darwin-arm64 SEA) and explicitly note that this does NOT fix the darwin-x64 crash. Comment-only change. No behaviour change.
Summary
The release workflow's comment block on the
macos-latestrunner choice (added in #11415) attributed darwin SEA crashes toldidproducing bad code-signature page hashes. That theory turned out to be wrong:node --build-sea+codesign --sign -+ run, with no pnpm and noldidinvolved. It still crashes on Intel Mac.codesignproduces correct page hashes by definition, so if signing were the bug, this repro would work. It doesn't.--build-seafor x64 — chained-fixup chain entries get rewritten incorrectly when the SEA segment is inserted, and dyld dereferences the unprocessed chain values at startup.The Node.js team has opted not to fix this on the grounds that x64 macOS is being phased out (related: nodejs/node#59553).
What this changes
Comment-only update to
.github/workflows/release.yml. No behaviour change. The job still runs onmacos-latest, just for the actually-correct reasons:codesignis on the runner — no need to build/installldid.macos-latestis Apple Silicon, soverify-binary.mjscan execute and smoke-test thedarwin-arm64SEA in place.The new comment also explicitly notes that this does not fix the
darwin-x64crash — that's an upstream Node.js bug, separately documented in thepack-appdocs (pnpm/pnpm.io@36d962f6).Related
Test plan
git diff origin/mainshows only the comment change;attestations: writeand theAttest build provenancestep from ci(release): attest build provenance for release artifacts #11441 are preserved.Summary by CodeRabbit