ci: harden release workflow secrets - #993
Conversation
|
Warning Review limit reached
More reviews will be available in 18 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ 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 |
There was a problem hiding this comment.
Suggested priority: P3 (only low-risk paths changed (.github/workflows/build.yml, packages/opencode/test/github/build-workflow.test.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request updates the release workflow tests in build-workflow.test.ts to verify new steps, including dependency installation, certificate importing, Apple API key cleanup, and notarization submission/finalization. It also refines the assertions for job permissions. The review feedback suggests ensuring that steps like installDependenciesStep, submitNotarizationStep, and finalizeNotarizationStep are explicitly asserted as defined before checking their index order to prevent silent test gaps or unclear failures. Additionally, a redundant assertion checking that the submit step's condition does not contain 'full' should be removed as it is already covered by an exact-match assertion.
|
Review follow-up:
|
Summary
Harden the #977 PR-C release workflow slice: reduce the default
build.ymltoken permissions, keep write scopes only on release-writing jobs, move macOS certificate import after dependency install/build checks, and clean up the Apple API key file after the last notarization use.Why
The release workflow previously granted
contents: writeat the top level, so read-only selector work inherited a broader token than needed. It also imported the macOS signing certificate beforebun install,bun run build, and the runtime import guard, widening the unlocked-keychain window around dependency and build-time code.Related Issue
Closes part of #977.
Human Review Status
Pending
Review Focus
Please focus on GitHub Actions release semantics: job-level permissions replacement,
actions: readremaining onbuild-electronfor cross-run artifact download, certificate import still running duringfinalizefor DMG signing, and the phase gates on Apple API key cleanup.Risk Notes
build-electronstill carriescontents: writefor all phases because the job owns submit/finalize/full publishing paths; splitting the job by phase is intentionally out of scope for PR-C.finalize, cert import still runs before the notarization wait because the same shared job path is used; narrowing that further would require splitting cert import/re-import or keychain cleanup around the DMG signing step.finalizeintentionally still imports the signing certificate. The app re-sign is skipped with--config.mac.sign=./script/skip-mac-sign.cjs, butelectron-builder.config.tskeepsdmg.sign: true, so the DMG packaging step still needs a signing identity.dev-dep-auditcurrently fails on existing lockfile advisories (34 vulnerabilities: 1 critical, 33 high). This is the known non-required visibility check added by PR ci: harden dependency audit checks #990; the livedev-merge-gateruleset does not includedev-dep-audit.How To Verify
Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.