fix: require signed and notarized mac releases#62
Open
felippe-alves wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR implements macOS app notarization support with dual authentication methods. The notarization script handles Apple ID and App Store Connect API key credentials, validates release builds, manages temporary key files, and integrates into the build workflow with pre-flight secret validation and environment variable provisioning. ChangesmacOS Notarization Feature
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
79-119: ⚡ Quick winMove secret validation ahead of
pnpm build.On tag builds, missing signing/notarization secrets still burn a full macOS build before this step fails. Running this check right after dependency installation keeps the new fail-fast behavior while saving runner time.
🤖 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/build.yml around lines 79 - 119, The "Verify mac signing secrets" step currently runs after the macOS build and should be moved to run earlier to fail-fast; relocate the step named "Verify mac signing secrets" so it executes immediately after dependency installation (i.e., right after the workflow step that runs "pnpm install" or the job step that installs dependencies) and before the "pnpm build" (or any macOS build) step; keep the exact env and validation logic intact but change its position in the job sequence so tag builds validate MAC_CSC_LINK / MAC_CSC_KEY_PASSWORD and either the APPLE_ID/APPLE_APP_SPECIFIC_PASSWORD/APPLE_TEAM_ID trio or APPLE_API_KEY/APPLE_API_KEY_ID/APPLE_API_ISSUER trio before running the build.
🤖 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/notarize.js`:
- Around line 71-73: The current if-block uses the wrong condition and omits
APPLE_TEAM_ID: update the incomplete-credentials check (the if near the throw
that references appleIdEnv) to treat APPLE_TEAM_ID as part of the required set
and to throw only when there is a partial configuration (i.e., at least one of
appleIdEnv is set but not all). Concretely, include "APPLE_TEAM_ID" in
appleIdEnv if not already, compute missing = appleIdEnv.filter(k =>
!process.env[k]) and if (missing.length > 0 && appleIdEnv.some(k =>
process.env[k])) throw an Error listing the missing keys (use appleIdEnv.join or
missing.join for the message) so partial configs are flagged.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 79-119: The "Verify mac signing secrets" step currently runs after
the macOS build and should be moved to run earlier to fail-fast; relocate the
step named "Verify mac signing secrets" so it executes immediately after
dependency installation (i.e., right after the workflow step that runs "pnpm
install" or the job step that installs dependencies) and before the "pnpm build"
(or any macOS build) step; keep the exact env and validation logic intact but
change its position in the job sequence so tag builds validate MAC_CSC_LINK /
MAC_CSC_KEY_PASSWORD and either the
APPLE_ID/APPLE_APP_SPECIFIC_PASSWORD/APPLE_TEAM_ID trio or
APPLE_API_KEY/APPLE_API_KEY_ID/APPLE_API_ISSUER trio before running the build.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1cc0f8a-a2c0-4850-b3e4-b272c4b077f6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/build.ymlpackage.jsonscripts/notarize.js
Comment on lines
+71
to
+73
| if (process.env.APPLE_ID || process.env.APPLE_APP_SPECIFIC_PASSWORD) { | ||
| throw new Error(`Incomplete Apple ID notarization credentials. Required: ${appleIdEnv.join(", ")}`); | ||
| } |
There was a problem hiding this comment.
Treat APPLE_TEAM_ID as part of the incomplete Apple ID check.
With only APPLE_TEAM_ID set, this falls through as “no credentials” instead of flagging a partial Apple ID configuration. That makes local misconfigurations easy to miss and downgrades tagged builds to the generic error path.
Proposed fix
- if (process.env.APPLE_ID || process.env.APPLE_APP_SPECIFIC_PASSWORD) {
+ if (hasSomeEnv(appleIdEnv)) {
throw new Error(`Incomplete Apple ID notarization credentials. Required: ${appleIdEnv.join(", ")}`);
}🤖 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 `@scripts/notarize.js` around lines 71 - 73, The current if-block uses the
wrong condition and omits APPLE_TEAM_ID: update the incomplete-credentials check
(the if near the throw that references appleIdEnv) to treat APPLE_TEAM_ID as
part of the required set and to throw only when there is a partial configuration
(i.e., at least one of appleIdEnv is set but not all). Concretely, include
"APPLE_TEAM_ID" in appleIdEnv if not already, compute missing =
appleIdEnv.filter(k => !process.env[k]) and if (missing.length > 0 &&
appleIdEnv.some(k => process.env[k])) throw an Error listing the missing keys
(use appleIdEnv.join or missing.join for the message) so partial configs are
flagged.
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.
Summary
Verification
Summary by CodeRabbit