feat(release): gate brand platform build matrix - #429
Conversation
|
Your Claude subscription has hit its usage limit. It resets at 11am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| environment: release | ||
| env: | ||
| EXPO_TOKEN: ${{ secrets.EXPO_TOKEN }} | ||
| R2_ACCESS_KEY_ID: ${{ secrets[format('{0}_R2_ACCESS_KEY_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} |
| env: | ||
| EXPO_TOKEN: ${{ secrets.EXPO_TOKEN }} | ||
| R2_ACCESS_KEY_ID: ${{ secrets[format('{0}_R2_ACCESS_KEY_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} | ||
| R2_ACCOUNT_ID: ${{ secrets[format('{0}_R2_ACCOUNT_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} |
| EXPO_TOKEN: ${{ secrets.EXPO_TOKEN }} | ||
| R2_ACCESS_KEY_ID: ${{ secrets[format('{0}_R2_ACCESS_KEY_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} | ||
| R2_ACCOUNT_ID: ${{ secrets[format('{0}_R2_ACCOUNT_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} | ||
| R2_SECRET_ACCESS_KEY: ${{ secrets[format('{0}_R2_SECRET_ACCESS_KEY', matrix.distribution.desktop.credentialSecretPrefix)] }} |
| runs-on: ${{ vars.CI_RUNNER_LINUX || 'ubuntu-latest' }} | ||
| environment: release | ||
| env: | ||
| AWS_ACCESS_KEY_ID: ${{ secrets[format('{0}_R2_ACCESS_KEY_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} |
| environment: release | ||
| env: | ||
| AWS_ACCESS_KEY_ID: ${{ secrets[format('{0}_R2_ACCESS_KEY_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} | ||
| AWS_SECRET_ACCESS_KEY: ${{ secrets[format('{0}_R2_SECRET_ACCESS_KEY', matrix.distribution.desktop.credentialSecretPrefix)] }} |
| env: | ||
| AWS_ACCESS_KEY_ID: ${{ secrets[format('{0}_R2_ACCESS_KEY_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} | ||
| AWS_SECRET_ACCESS_KEY: ${{ secrets[format('{0}_R2_SECRET_ACCESS_KEY', matrix.distribution.desktop.credentialSecretPrefix)] }} | ||
| R2_ACCOUNT_ID: ${{ secrets[format('{0}_R2_ACCOUNT_ID', matrix.distribution.desktop.credentialSecretPrefix)] }} |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two changes recommended before this ships a real brand release.
Reviewed the full CODE-559 diff (18 files). Note the incremental range-diff for this push is almost entirely deletions because b54ea67 is a merge: sync master into code-559 and master had already absorbed #428 — so I reviewed the complete diff rather than the range-diff.
The design holds up well. The fail-closed posture in brand-matrix.cjs (exact key sets, cross-platform pin agreement, R2 prefix overlap detection, unique credential prefixes / EAS project IDs / ASC app IDs) and the create-once flag: 'wx' provenance with realpath escape guards are the right shape for a multi-tenant release train. Two things to fix, one note.
Findings — all three inline:
release-brand-matrix.yml:548— provenance sidecars get synced into the public update-feed prefix, disclosing private-repo commit SHAs.build-desktop.yml:210— thePOSTHOG_HOSTsecrets.→vars.migration is incomplete;ci.ymlstill reads the secret.release-brand-matrix.yml:193— note only: coupling to the composite action's private scratch layout.
Things I chased down and cleared (recording these so they aren't re-litigated):
- Mobile identities do not need
--brand-artifacts. The matrix passesbrand-artifacts: truefor desktop only, and the action gatesBRAND_ARTIFACTSinsideif [ "$APP" = desktop ]— which looked like it would starve the ~8 downstream steps that readapps/mobile/generated/brand-identity.{ios,android}.json. It doesn't:apps/mobile/scripts/render-config-bundle.mts:104writes both identities unconditionally, and the flag has no mobile counterpart. Correct as written. publish-desktop'sfor … in mac win linuxmatchesbuild-desktop.yml:138-145exactly, andbuild-mobile.yml:118-125really does producelinkcode-android.aab, sopublish-mobile's unconditional android submit is reachable. Removing the commented-out android rows from build-mobile's ownsubmitmatrix is inert here — the brand matrix runs its own publish job.release-inputs/mobile-release.jsoncan never be missing on the upload path.brand-matrix.cjs:232rejects a nulldistribution.mobilewhenbuild=true, and upload⇒sign⇒build, so the unconditionaljq -er .ios.ascAppIdis safe.verifyReleaseArtifactProvenancenot re-callingassertStoreComplianceis correct, even thoughdocs/RELEASE.mdsays publish "re-hashes … all immutable inputs". The declaration is fully derived from the bundle —disclosedFeaturesmust exactly equal thefeature./modules.key set, and the checklist must be an exact 5-key all-trueset — so it carries no information the hash-bound bundle doesn't already pin. Nothing to re-verify, and the--verifypath correctly takes no--compliance..github/scripts/**/*.test.mjsis in the root vitestinclude, so both new validators are actually exercised.- The
= { bundle:/lastIndexOf('};')parser inrelease-artifact-cli.mtslooks fragile but is sound: the marker precedes any JSON payload, nothing after the const terminator contains};, and the single.replacehits index 0 of the slice. CONFIG_PUBLISHER_TOKENstays out of persisted git config (per-command-c http.…extraheader) — good.
Recommendation: because this is the first run to exercise the reworked packaging and publish scripts end to end, drive it once with build/sign but upload: false (or release-desktop.yml's dry_run: true) before pointing it at a live R2 prefix.
Claude Opus ｜ 𝕏
| AWS_REQUEST_CHECKSUM_CALCULATION: WHEN_REQUIRED | ||
| AWS_RESPONSE_CHECKSUM_VALIDATION: WHEN_REQUIRED | ||
| run: | | ||
| aws s3 sync "artifacts/${{ matrix.brandId }}/" \ |
There was a problem hiding this comment.
aws s3 sync copies the whole artifacts/<brand>/ directory with no --exclude, and that directory provably contains the provenance sidecars — the step directly above asserts test -s "artifacts/${{ matrix.brandId }}/release-provenance.${platform}.json" for each of mac win linux.
The destination is the public electron-updater feed (distribution.desktop.r2Prefix, served at the validated updateUrl that clients fetch unauthenticated). So each release publishes, on a public CDN:
publisherGitSha— a commit SHA of the privateCONFIG_PUBLISHER_REPOsourceGitShaandconfigSnapshotSha256- the full per-artifact digest and size inventory
None of that is needed by the updater. If publishing provenance is intentional as a transparency artifact, docs/RELEASE.md should say so; otherwise add --exclude 'release-provenance.*.json' — the files are still uploaded as workflow artifacts and re-verified in publish-preflight, so nothing downstream loses them.
| MAIN_VITE_SENTRY_DSN: ${{ inputs.sign && secrets.SENTRY_DSN_DESKTOP || '' }} | ||
| RENDERER_VITE_POSTHOG_PROJECT_TOKEN: ${{ inputs.sign && secrets.POSTHOG_PROJECT_TOKEN || '' }} | ||
| RENDERER_VITE_POSTHOG_HOST: ${{ inputs.sign && secrets.POSTHOG_HOST || '' }} | ||
| RENDERER_VITE_POSTHOG_HOST: ${{ inputs.sign && vars.POSTHOG_HOST || '' }} |
There was a problem hiding this comment.
This PR moves POSTHOG_HOST from a secret to an Actions variable here, in build-mobile.yml:135,206, and in release-brand-matrix.yml:119 — and docs/ENVIRONMENT.md:112 now documents it as "Organization Actions variable".
But .github/workflows/ci.yml was not migrated and still reads the secret in two places:
ci.yml:224—VITE_POSTHOG_HOST: ${{ secrets.POSTHOG_HOST }}(webview, unconditional)ci.yml:255—EXPO_PUBLIC_POSTHOG_HOST: ${{ github.event_name != 'pull_request' && secrets.POSTHOG_HOST || '' }}
While both the secret and the var exist this is invisible. The moment the secret is retired to match the new docs, those two silently resolve to '' — and docs/ENVIRONMENT.md:84 states both webview PostHog vars are required, so this either hard-fails the build or ships a bundle pointed at no host.
Either migrate ci.yml to vars.POSTHOG_HOST in the same PR, or leave a line in docs/ENVIRONMENT.md recording that the secret must stay provisioned until ci.yml catches up.
| run: | | ||
| set -euo pipefail | ||
| mkdir release-inputs | ||
| cp "$RUNNER_TEMP/config-render-desktop/source/packages/config-structural/brands.manifest.yaml" release-inputs/ |
There was a problem hiding this comment.
Note, not a blocker — this works today (the composite action never removes $work).
This reaches into render-release-config's private scratch directory, which this same PR renamed from config-render to config-render-$APP. That rename is itself the evidence the layout isn't a stable interface: a second rename, or adding a cleanup step to the action, breaks this cp from a file that gives no signal it has a consumer.
Worth promoting to a real contract — add an outputs: entry on the composite action exposing the pinned brands.manifest.yaml path, and consume that step output here instead of the literal temp path.
There was a problem hiding this comment.
Important
One change in dab0be6 rests on an invariant that can't be checked from this repo, and the pipeline has no detector for the way it fails.
Reviewed changes — this round covers dab0be6 ("harden brand matrix trust") on top of the round-1 review:
- The release plan moved out of
vars.BRAND_BUILD_MATRIXinto a committed file selected by a newmatrix_fileinput. - A new ancestry gate requires the client ref to be an ancestor of
origin/master(with the matchingfetch-depth: 0). - The matrix bytes are now hashed and threaded end-to-end as
delivery_descriptor_sha256, written into provenance and re-verified at publish time. secrets: inheritwas dropped from both reusable-workflow calls.- Test coverage widened (byte-exact digest test, mismatched-descriptor verify test).
BRAND_BUILD_MATRIXis fully retired — the docs row is gone and there are zero remaining references.
The hardening direction is right, and the implementation of it is careful. matrix_file is regex-pinned to a single directory with no / and no traversal, git ls-tree is mode-checked to 100644 blob so symlinks and gitlinks are rejected, and the bytes are read via git cat-file from the verified ref rather than the working tree. matrix_json surviving as plan-only and being rejected when build=true is the right split. The new tests are not theatre: the digest test asserts the exact bytes including the trailing newline, so a CLI that re-serialized the JSON would fail it, and the verify test feeds mismatched descriptor bytes and asserts .rejects.toThrow('reviewed release matrix').
I also independently confirmed a few things that looked risky and aren't: actions/checkout at fetch-depth: 0 does populate refs/remotes/origin/*, so the merge-base --is-ancestor gate resolves; MATRIX_JSON / MATRIX_FILE are passed as env: rather than interpolated into shell, so there is no injection surface; and the empty-string default for delivery_descriptor_sha256 can never reach the CLI, because every provenance step is gated on inputs.rendered_artifact != '' and only the brand matrix sets that — always alongside the digest.
⚠️ The new "reviewed matrix" trust root is branch protection alone
The security value of this commit is "committed to master ⇒ reviewed". That premise has no enforcement in the repo: there is no CODEOWNERS file anywhere, .github/release/brand-matrices/ is not created by this PR, and AGENTS.md records that master merges sometimes go through gh pr merge --admin because the ruleset's review requirements can't always be satisfied. So the gate currently reduces to "someone with admin merged it", which is a weaker property than the commit message implies.
Technical details
Affected: .github/workflows/release-brand-matrix.yml:72-108, and the (not-yet-created) .github/release/brand-matrices/ directory.
Required outcome: a change to a release matrix cannot reach master without a review by someone entitled to authorize a branded release.
Suggested approach: add a CODEOWNERS entry for .github/release/brand-matrices/ naming the release owners, and make sure the master ruleset requires code-owner review for that path specifically — so an --admin merge of unrelated work doesn't also carry a matrix edit.
ℹ️ Nitpicks
vars.BRAND_BUILD_MATRIXnow has zero readers. Worth deleting the org/repo variable itself so a stale plan can't be mistaken for live configuration later.
Claude Opus ｜ 𝕏
| strategy: | ||
| fail-fast: false | ||
| matrix: ${{ fromJSON(needs.prepare.outputs.brands) }} | ||
| uses: ./.github/workflows/build-desktop.yml |
There was a problem hiding this comment.
Dropping secrets: inherit here (and at line 381) is safe only if every secrets.* reachable on the sign: true path is stored on the release environment. GitHub forwards repository/organization secrets to a called workflow solely via inherit, while environment secrets resolve at job level independently of the caller — so the two tiers behave oppositely under this change.
The repo currently asserts both readings at once. build-desktop.yml:10 now says callers need not forward secrets, but release-desktop.yml:35 still carries secrets: inherit # required so build-desktop.yml can read the signing secrets. One of those comments is wrong, and I can't tell which from inside the repo.
If any signing secret turns out to be repo/org-scoped, the macOS path degrades silently rather than failing: an empty MACOS_CSC_LINK skips the CSC_LINK export by design (build-desktop.yml:250-256) and electron-builder simply produces an unsigned package.
Technical details
Affected: .github/workflows/release-brand-matrix.yml:308, :381; .github/workflows/build-desktop.yml:10, :71-76, :135; .github/workflows/release-desktop.yml:35.
Required outcome: every secret read on the signing path resolves in the called job without inherit, and the repo's comments agree on why.
Suggested approach: confirm the storage tier of each signing secret in org settings, then run this workflow once with build: true, sign: true, upload: false and check the produced artifact is actually signed. Note build-desktop.yml:71-76 reads the AZURE_* values in a workflow-level env: block while the comment calls them environment secrets — that block is outside the job that declares environment: release.
Open question: are MACOS_CSC_LINK, MACOS_CSC_KEY_PASSWORD, the Apple API-key secrets, and AZURE_* stored on the release environment, or at repo/org level?
| --release-manifest release-inputs/release-manifest.desktop.json \ | ||
| --compliance release-inputs/compliance.desktop.json \ | ||
| --out "release-provenance.${{ matrix.platform }}.json" \ | ||
| ${{ inputs.sign && '--signed' || '' }} |
There was a problem hiding this comment.
--signed is derived from inputs.sign, so the provenance sidecar restates a workflow input rather than recording an observation about the bytes.
Nothing downstream closes that gap. verify-artifacts.mts (422 lines) checks arch coverage and feed sha512s but never invokes codesign / signtool / spctl, and publish-preflight's --verify … --signed only compares provenance.signed against the same flag (release-artifact.ts:319). An artifact whose signing silently no-opped is therefore attested signed: true and clears every publish gate — which is what turns the secrets: inherit question above from a loud failure into a quiet one.
Technical details
Affected: .github/workflows/build-desktop.yml:315, apps/desktop/scripts/verify-artifacts.mts, packages/foundation/common/src/node/release-artifact.ts:164, :319.
Required outcome: signed: true in provenance means a signature was observed on the artifact, not that a flag was set.
Suggested approach: when inputs.sign is true, have verify-artifacts.mts run codesign --verify --deep --strict on the macOS bundle and signtool verify /pa on the Windows installer, and fail the build if either reports no valid signature. That makes the failure mode loud at build time rather than discoverable only after publication.
3 participants
Summary
Implements Linear CODE-559 as a stack on #428 (
xuan/code-558@77f3fb89d4268ffa93a9cc70d5281b5496b7318e).releaseenvironment vars/secrets and least-privilege contractNo LinkCode HQ change is required: this workflow consumes the immutable CODE-558 publisher output without changing render semantics.
Verification
pnpm check:cipnpm test— 331 files passed, 2,806 tests passed, 1 skippedpackages/foundation/commonandapps/mobileyamldependency (actionlintwas unavailable locally)CI=1 EXPO_NO_TELEMETRY=1 pnpm -F @linkcode/mobile smoke:export— iOS and Android production Metro/Hermes bundles exportedNo production credentials were used. Nothing was signed, uploaded, store-submitted, or released. Android SDK was unavailable locally, so Android evidence is Hermes export + clean native prebuild rather than an APK/AAB; iOS is likewise unsigned and not an IPA.
Checklist
pnpm check:ciandpnpm testboth pass (no Rust changes)