test(release): complete credential-free brand pilot - #436
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f4172178c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| gh api "repos/$GITHUB_REPOSITORY/environments/release" > "$environment" | ||
| if ! jq -e ' | ||
| ([.protection_rules[]?.type] | index("required_reviewers") != null) and | ||
| (.deployment_branch_policy != null) |
There was a problem hiding this comment.
When release uses a broad custom deployment policy such as *, this predicate still succeeds because it checks only that deployment_branch_policy is non-null and never inspects the permitted patterns. Signing can therefore proceed while the exact-ref restriction required by docs/RELEASE.md lines 184–186 is absent, leaving release credentials available to unrelated refs or workflows; fetch the environment's deployment-branch policies and validate the allowed patterns explicitly.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new release-environment-preflight gate does not actually block the desktop signing path — signing-inputs being force-skipped reports skipped, which the desktop job already accepts. Two other issues below are worth a look before this stops being a pilot branch.
Reviewed changes — initial review of the single commit on xuan/code-561 against base xuan/code-559. No product code is touched; both files are CI.
- New
credential-free-validationjob — unconditional (needs: prepare, noif:), no secrets and noenvironment:. Checksapps/desktop/e2e/fixtures/pilot-e2e-v1.jsonagainst a pinned sha256, runs the Electron config canary under Xvfb, then production Hermes export plus Android and iOSexpo prebuild, and uploadsruntime-validation.jsonrecordingdeploymentIdentity = {kind:"local-static-origin", providerDeploymentId:null}. - New
release-environment-preflightgate —if: inputs.sign,environment: release; reads thereleaseenvironment throughgh apiand fails unless it carriesrequired_reviewersand a non-nulldeployment_branch_policy.signing-inputsis rewired to need it, and workflowpermissionsgainsdeployments: read. - New per-brand
desktop-validationjob —inputs.build && !inputs.sign; downloadsbrand-render-<brand>, moves the rendered bundle aside, runs the canary, asserts the canary did not recreateapps/desktop/generated, restores it, then binds a fixedvalidation.desktop.jsonto the brand's bundle and identity viarelease-artifact-cli.mts. - Three source-contract tests — slice the workflow YAML between job keys with
indexOfand assert raw substrings, pinning the credential-free job's independence, the preflight's fail-closed shape, and the desktop job's evidence binding.
Verified along the way and not problems: the pinned fixture hash matches the file on disk; smoke:export and expo prebuild work without a rendered mobile config because apps/mobile/app.config.ts:24-36 only throws on a partial generated/ set (and ci.yml:256 already runs it that way); pnpm --dir apps/mobile exec is equivalent to mobile-validation's working-directory; and bare xvfb-run matches existing CI usage.
ℹ️ Per-brand desktop-validation fans out over a brand-independent workload
Moving apps/desktop/generated aside is forced, not sloppy — loadGeneratedConfigBundle throws when both a generated bundle and MAIN_VITE_CONFIG_BOOTSTRAP are present (apps/desktop/scripts/config-bundle.mts:101-106), so the canary cannot run with the rendered bundle in place. The consequence is that every matrix leg executes a byte-identical fixture run, and the evidence step then binds that brand-independent result to the brand's bundle and identity. mobile-validation is the opposite: it keeps the rendered config in place and validates it.
Technical details
# Per-brand desktop validation exercises no per-brand input
## Affected sites
- `.github/workflows/release-brand-matrix.yml:403-405` — `strategy.matrix` fans out over every brand.
- `.github/workflows/release-brand-matrix.yml:433` — `mv apps/desktop/generated "$rendered"` removes the only brand-specific input before the canary runs.
- `.github/workflows/release-brand-matrix.yml:447-460` — a fixed `validation.desktop.json` literal is bound to `apps/desktop/generated/{config-build-bundle,brand-identity}.json` after the bundle is restored.
## Required outcome
- Either the per-brand fan-out exercises something brand-specific, or the Electron canary runs once and the per-brand step is limited to the provenance binding.
- A reader of `brand-validation-desktop-<brand>` should not be able to mistake it for evidence that `<brand>`'s rendered config was exercised in Electron.
## Open questions for the human
- Is the fan-out deliberate (N Electron rebuilds for identical work, to keep the artifact shape symmetric with `mobile-validation`), or an oversight?
- Should the desktop evidence JSON record that the rendered bundle was deliberately withheld, alongside the `pilotFixtureSha256` it already records?ℹ️ Nitpicks
credential-free-validationcarries noif:, so it also runs on plan-only dispatches (matrix_jsonwithbuild=false) and on sign/upload runs, where it duplicatesdesktop-validationandmobile-validation. Validating a matrix JSON now costs a full Electron build plus two Expo prebuilds — intentional?expect(validation).not.toContain('environment: release')is raw-text, so the block formenvironment:\n name: releaseslips past it.
Claude Opus ｜ 𝕏
| expect(desktopValidation).toContain('inputs.build && !inputs.sign'); | ||
| expect(desktopValidation).toContain('xvfb-run -a pnpm -F @linkcode/desktop e2e:config-canary'); | ||
| expect(desktopValidation).toContain( | ||
| '54ce1fc855e12295a8dd1490463c9afac8e84a526f1e16340bcefe4f0fec8e39', |
There was a problem hiding this comment.
Nothing in the suite ties this literal to the actual fixture. The test asserts the workflow contains the hash, and the workflow asserts the file matches it — but that second check only runs on a manual release dispatch, which per the PR description is not happening yet. So editing apps/desktop/e2e/fixtures/pilot-e2e-v1.json stays green through pnpm test and pnpm check:ci and only surfaces months later as a red release job.
createHash and readFile are already imported at the top of this file, so closing it is two lines:
const fixture = await readFile(
new URL('../../apps/desktop/e2e/fixtures/pilot-e2e-v1.json', import.meta.url),
);
expect(createHash('sha256').update(fixture).digest('hex')).toBe(
'54ce1fc855e12295a8dd1490463c9afac8e84a526f1e16340bcefe4f0fec8e39',
);| expect(validation).toContain('"local-static-origin"'); | ||
| expect(validation).toContain('providerDeploymentId:null'); | ||
| expect(validation).not.toContain('environment: release'); | ||
| expect(validation).not.toContain('secrets.'); |
There was a problem hiding this comment.
not.toContain('secrets.') misses the indexed form, which this same workflow already uses at line 548 (secrets[format('{0}_R2_ACCESS_KEY_ID', …)]) — so a future edit could wire brand credentials into the credential-free job and this guard would stay green. expect(validation).not.toMatch(/secrets[.[]/) covers both spellings.
There was a problem hiding this comment.
Important
The committed pilot matrix cannot drive a build=true run — brand-matrix.cjs:232 rejects it — so none of the inputs.build-gated machinery this PR adds or hardens is reachable with it, and the new test that "pins" the matrix never exercises that path. Separately, the environment repoint has no way back to release.
Reviewed changes — delta between the prior pullfrog review (9f41721) and a7f15df, which reshapes the pilot from a single unconditional job into a per-target matrix and moves the whole workflow onto an isolated environment.
- Matrixed
credential-free-validation— now fans out overfromJSON(needs.prepare.outputs.targets)(brands ×desktop/ios/android), with platform-conditional steps, a per-legruntimefield, andcredential-free-<brand>-<platform>artifact names. - Rewrote
release-environment-preflight—if: inputs.sign→if: inputs.build, dropped its ownenvironment:, swappedgithub.tokenforsecrets.PILOT_ENVIRONMENT_ADMIN_TOKENwith an explicit empty-token failure, retargetedrelease→pilot-nonproduction, and tightened the jq assertion to require a non-emptyreviewersarray plus a concrete branch policy. - Gated
render-inputson the preflight — this is what actually closes the desktop signing bypass from the last review (thread resolved). - Repointed all eight
environment: releasesites inrelease-brand-matrix.ymltopilot-nonproduction, and threaded a newrelease_environmentworkflow_callinput (defaultrelease) throughbuild-desktop.ymlandbuild-mobile.yml. - Added
.github/release/brand-matrices/code-561-pilot.json— two canary brands, all three platforms,distribution.desktop/distribution.mobilebothnull. - Added three contract tests — the pilot matrix plan, the reusable-workflow environment threading, and the rewritten preflight shape.
Traced and confirmed not problems: the preflight gate is now airtight (a failed preflight skips render-inputs → render, so desktop's needs.render.result == 'success' blocks it and the 'skipped' escape hatch on signing-inputs no longer matters; build=false, sign=true is still killed in prepare); '${{ matrix.platform }}' inside run: blocks is not a shell-injection vector because platform comes from the fixed PLATFORMS const in brand-matrix.cjs:4; and release-desktop.yml calls build-desktop.yml without release_environment, so the production desktop train still resolves to release.
⚠️ The pilot environment repoint has no way back to release
All eight environment: release sites in release-brand-matrix.yml are now hardcoded to pilot-nonproduction — including publish-desktop (R2 sync) and publish-mobile (TestFlight / Play internal) — and both reusable-workflow call sites pass release_environment: pilot-nonproduction as a literal. Nothing parameterizes it, and brand-matrix.test.mjs:319 asserts expect(workflow).not.toContain('environment: release'), so restoring the production environment is now a test failure rather than a config change. The release_environment input added to build-desktop.yml / build-mobile.yml is exactly the right seam, but this workflow never uses it as a seam.
Technical details
# `release-brand-matrix.yml` is pinned to the pilot environment with no exit
## Affected sites
- `.github/workflows/release-brand-matrix.yml:235,252,289,569,649,725` — `environment: pilot-nonproduction` literals, covering render, signing-input validation, publish preflight, store submission, and R2 upload.
- `.github/workflows/release-brand-matrix.yml:418,557` — `release_environment: pilot-nonproduction` passed as a literal to `build-desktop.yml` / `build-mobile.yml`.
- `.github/workflows/release-brand-matrix.yml:219` — the preflight only ever inspects `pilot-nonproduction`.
- `.github/scripts/brand-matrix.test.mjs:319` — `expect(workflow).not.toContain('environment: release')` locks the repoint in.
## Required outcome
- Either the workflow can select its environment (so a real brand release is possible without editing YAML and a test), or the PR states explicitly that this file is pilot-only and names the commit/PR that reverts it.
- Whatever the answer, `pilot-nonproduction` must own every credential the eight repointed jobs read (`CONFIG_PUBLISHER_TOKEN`, the Apple/Azure signing set, `EXPO_TOKEN`, and the per-brand `*_R2_*` secrets) or those jobs fail at the first secret read rather than at a gate.
## Open questions for the human
- Is this a permanent split (brand matrix ships through a nonproduction environment by design) or a temporary pilot state that reverts before this reaches `master`?
- If it is meant to be selectable, should it be a `workflow_dispatch` input, or derived from `inputs.upload` so only real uploads demand the production environment?ℹ️ Nitpicks
credential-free-validationstill carries noif:, and the round-2 fan-out multiplies the cost noted last review: a plan-onlymatrix_jsondispatch now spendsbrands × 3heavyweight jobs (6 for the pilot matrix), including two byte-identical Electron runs and four fullpnpm install+ Expo cycles.build-desktop.yml:139's comment still reads "Signing is gated by thereleaseenvironment (secrets + tag policy)", but line 140 now resolves to whateverrelease_environmentthe caller passes.
Claude Opus ｜ 𝕏
| "distribution": { | ||
| "desktop": null, | ||
| "mobile": null |
There was a problem hiding this comment.
A null distribution makes this matrix unusable for any build=true dispatch — brand-matrix.cjs:232 fails with desktop and mobile delivery inputs are required when build=true, which I reproduced against this exact file. Since matrix_json is rejected for builds (release-brand-matrix.yml:77-80), this is the only committed matrix a build could use, so release-environment-preflight, render, desktop-validation and mobile-validation are all unreachable and only credential-free-validation ever runs.
Technical details
# The committed pilot matrix cannot drive any `build=true` job
## Affected sites
- `.github/release/brand-matrices/code-561-pilot.json:57-59,156-158` — `distribution.desktop` and `distribution.mobile` are `null` for both brands.
- `.github/scripts/brand-matrix.cjs:232-237` — `if (options.build && (distribution.desktop === null || distribution.mobile === null)) fail(...)`.
- `.github/workflows/release-brand-matrix.yml:77-80` — `matrix_json is plan-only; builds require a reviewed matrix_file`, so there is no alternative input.
## Reproduction
```
node .github/scripts/brand-matrix.cjs \
--matrix-file .github/release/brand-matrices/code-561-pilot.json \
--build true --sign false --upload false
# TypeError: matrix.brands[0].distribution: desktop and mobile delivery inputs are required when build=true
```
## Required outcome
- Either the pilot matrix carries real `distribution` blocks (pointing at pilot R2 prefixes and a pilot EAS project) so `build=true, sign=false` can exercise `render` + `desktop-validation` + `mobile-validation`, or the PR states that the pilot is plan-only and that the `build`-gated jobs are validated some other way.
## Open questions for the human
- Was `build=true, sign=false` ever run against this matrix, or was the Electron/Xvfb evidence in the PR description produced locally rather than through this workflow?| 'utf8', | ||
| ), | ||
| ); | ||
| const plan = buildMatrixPlan(pilot); |
There was a problem hiding this comment.
buildMatrixPlan(pilot) passes no options, so options.build is undefined and the build=true distribution check at brand-matrix.cjs:232 is never reached — the test goes green on a matrix that cannot actually build. Line 106 then asserts the all-null distribution as though it were the desired state. Adding expect(() => buildMatrixPlan(pilot, { build: true })).toThrow(/delivery inputs are required/) would at least make the constraint explicit and intentional.
| runtime=production-hermes+prebuild | ||
| fi | ||
| jq -cn \ | ||
| --arg brandId "$BRAND_ID" \ |
There was a problem hiding this comment.
This job never touches anything brand-specific: it checks out inputs.ref and runs a fixed fixture, and unlike desktop-validation it never downloads brand-render-<brand>. So credential-free-acme-desktop and credential-free-zenith-desktop will hold byte-identical evidence apart from this brandId string, and a reader of the artifact reasonably concludes that brand's build was exercised. That is the same class of overclaim the PR's own source-contract tests exist to prevent for provider identity.
Technical details
# Credential-free evidence claims a brand it never exercised
## Affected sites
- `.github/workflows/release-brand-matrix.yml:136` — `matrix: ${{ fromJSON(needs.prepare.outputs.targets) }}` fans out over `brandId × platform`.
- `.github/workflows/release-brand-matrix.yml:150-174` — every step operates on the checked-out tree and the pinned fixture; no brand input is consumed.
- `.github/workflows/release-brand-matrix.yml:177,189,194` — `brandId` is threaded into `runtime-validation.json`.
- `.github/workflows/release-brand-matrix.yml:198` — artifact name is per brand.
## Required outcome
- Either the matrix legs consume the brand's rendered inputs (making `brandId` a true claim), or the fan-out drops to `platform` only and the evidence stops naming a brand.
- `runtime-validation.json` should not assert a property the run did not establish; `deliveryDescriptorSha256` is already matrix-wide and does carry real meaning.
## Open questions for the human
- Is the per-brand fan-out here for artifact-shape symmetry with `desktop-validation` / `mobile-validation`, or was it meant to pick up per-brand rendered config?| steps: | ||
| - name: Require protected nonproduction release environment | ||
| env: | ||
| GH_TOKEN: ${{ secrets.PILOT_ENVIRONMENT_ADMIN_TOKEN }} |
There was a problem hiding this comment.
Because this job deliberately has no environment:, PILOT_ENVIRONMENT_ADMIN_TOKEN has to be a repository-level secret — readable by any job in any workflow on any branch, which is a wider blast radius than the gate it protects. Worth confirming it is a fine-grained PAT scoped to actions: read on this repo only (that is the permission category GitHub documents for GET /repos/{owner}/{repo}/environments/{name}), rather than something matching the ADMIN in its name.
There was a problem hiding this comment.
Important
e37d4a1 reverts round 2's isolated-pilot environment, which moves this PR's new fail-closed preflight onto the shared production release environment. Provisioning release the way the gate's own error message suggests would block the tag-triggered desktop release train, and the PR description still describes the pilot-nonproduction design that no longer exists anywhere in the tree.
Reviewed changes — delta between the prior pullfrog review (a7f15df) and e37d4a1, which undoes the environment repoint flagged last round.
- Reverted all six
environment: releasesites inrelease-brand-matrix.ymlfrompilot-nonproductionback torelease, and changed both reusable-workflow call sites to passrelease_environment: release(lines 418, 557). - Retargeted
release-environment-preflightat thereleaseenvironment — renamed its token tosecrets.RELEASE_ENVIRONMENT_ADMIN_TOKENand updated thegh apipath, the jq.nameassertion, the temp-file name, and both error messages. - Documented
RELEASE_ENVIRONMENT_ADMIN_TOKENindocs/ENVIRONMENT.md, including why it cannot be stored inside the environment it inspects. - Rewrote the environment assertions in
brand-matrix.test.mjs—not.toContain('environment: release')gave way to occurrence counts (sixenvironment: release, tworelease_environment: release) alongside the existing per-job identity checks onrender-inputsandsigning-inputs.
Traced and confirmed not problems: no reference to pilot-nonproduction or PILOT_ENVIRONMENT_ADMIN_TOKEN survives anywhere in the tree; the preflight gate stays airtight through the revert (a failed preflight skips render-inputs → render, so desktop's needs.render.result == 'success' blocks the signing path, and the signing-inputs == 'skipped' branch is only reachable on a legitimately unsigned run where build-desktop.yml:140 resolves environment to ''); and both new occurrence counts match the files exactly. The round-1 thread about deployments: read causing a 403 is resolved — the PAT switch means the workflow permissions: block no longer governs that gh api call.
⚠️ The pilot's fail-closed gate now applies to the production release environment
release-environment-preflight runs on if: inputs.build and hard-fails unless the repo carries RELEASE_ENVIRONMENT_ADMIN_TOKEN and the release environment has a non-empty required-reviewer rule plus a deployment branch policy. Last round that gate pointed at an environment nobody used; now it points at release, which release-desktop.yml, build-desktop.yml and publish-mobile-metadata.yml all share. The PR body says release is currently unprotected, so satisfying this gate means changing production release configuration — and adding required reviewers to release inserts a manual approval step into the tag-triggered desktop release train, which today runs unattended after release-please pushes the tag.
Technical details
# The credential-free pilot now gates the shared production `release` environment
## Affected sites
- `.github/workflows/release-brand-matrix.yml:203-228` — the preflight now inspects `release` and fails closed for every `build=true` dispatch.
- `.github/workflows/release-brand-matrix.yml:233,250` — `render-inputs` and `signing-inputs` both `needs: [prepare, release-environment-preflight]`, so nothing downstream runs until the environment is provisioned.
- `.github/workflows/release-desktop.yml:49` and `.github/workflows/build-desktop.yml:95,140` — the tag-triggered desktop train enters the same `release` environment.
- `.github/workflows/publish-mobile-metadata.yml:21` — also `environment: release`.
- PR description, "Live gate" section — still asserts the gate targets `pilot-nonproduction` and that `pilot-nonproduction` does not exist. Neither string appears in the tree after `e37d4a1`.
## Required outcome
- The PR description reflects what the branch actually does: the gate inspects `release`, and the new prerequisite is a repository-level `RELEASE_ENVIRONMENT_ADMIN_TOKEN` plus protection rules on `release`.
- The consequence for `release-desktop.yml` is stated and accepted before merge — either the desktop train is expected to wait on reviewer approval from now on, or the required-reviewer rule needs a scope that excludes it.
- `RELEASE_ENVIRONMENT_ADMIN_TOKEN` exists before this lands, otherwise the first `build=true` dispatch after merge fails at the preflight with `RELEASE_ENVIRONMENT_ADMIN_TOKEN is required to inspect environment protection`.
## Open questions for the human
- Was gating the shared `release` environment the intent of the revert, or was the intent only to stop hardcoding a nonexistent environment (in which case the preflight could target whatever `release_environment` the run resolves to)?
- Should `release-desktop.yml` be exempt from the required-reviewer rule, given it is the automated post-tag train described in `docs/RELEASE.md`?ℹ️ Nitpicks
- The
release_environmentseam is now inert: both call sites inrelease-brand-matrix.ymlpass the literal defaultrelease,release-desktop.ymlomits the input entirely, and noworkflow_dispatchinput feeds it — sobuild-desktop.ymlandbuild-mobile.ymlcarry aworkflow_callinput no caller varies, plus three test assertions pinning plumbing nothing exercises. Either wire it to a dispatch input or drop it. expect(workflow.split(' environment: release')).toHaveLength(7)(brand-matrix.test.mjs:317) pins a bare occurrence count, so moving the environment off one job and onto another stays green.render-inputsandsigning-inputsare additionally checked by identity, butrender,publish-preflight,publish-mobileandpublish-desktopare covered only by the total.
Claude Opus ｜ 𝕏
| .name == "release" and | ||
| ([.protection_rules[]? | select(.type == "required_reviewers") | .reviewers | length] | any(. > 0)) and | ||
| (.deployment_branch_policy != null) and | ||
| (.deployment_branch_policy.protected_branches == true or .deployment_branch_policy.custom_branch_policies == true) |
There was a problem hiding this comment.
Accepting protected_branches == true points an operator at the one policy shape that breaks the existing release train: GitHub's Protected branches only option refuses tag refs, and release-desktop.yml reaches environment: release from a v*.*.* tag push. An admin who satisfies this gate that way would leave every desktop release stuck on "not allowed to deploy", with nothing here or in the error message at line 226 hinting why.
Technical details
# The accepted branch policy can lock the tag-triggered desktop release out of `release`
## Affected sites
- `.github/workflows/release-brand-matrix.yml:224` — `protected_branches == true or custom_branch_policies == true`; the first disjunct is the trap.
- `.github/workflows/release-brand-matrix.yml:226` — the failure message says only "must require reviewers and a deployment branch policy", giving no shape.
- `.github/workflows/release-desktop.yml:9-12,49` — triggered by `push: tags: v*.*.*`, and its `release` job enters `environment: release`.
- `.github/workflows/build-desktop.yml:95,140` — reached from that same tag-triggered run with `sign: true`, both entering `release`.
## Background
GitHub's "Protected branches only" deployment branch policy admits only branches carrying protection rules; tag refs cannot deploy under it. Tags deploy only under custom branch policies with an explicit tag-name pattern (GitHub changelog, 2023-08-10, "Actions runs triggered from tags or forks with the same name as a protected branch will now be blocked").
## Required outcome
- Following this gate must not produce an environment configuration that blocks `release-desktop.yml`.
- Whichever policy shape is correct for this repo should be stated where an operator will read it — the `::error::` text, `docs/RELEASE.md`, or the `docs/ENVIRONMENT.md` row added by this PR.
## Suggested approach (optional)
Require `custom_branch_policies` and assert a tag pattern covering `v*.*.*`, rather than accepting either policy type.1 participant
Summary
releaseenvironment before render/build/sign/upload paths, and pass exactreleasethrough reusable Desktop/mobile workflowsRELEASE_ENVIRONMENT_ADMIN_TOKENfor pre-environment protection inspectionlocal-static-origin, no provider deployment ID) rather than claiming provider evidenceValidation
pnpm check:cipnpm test— 331 files passed, 1 skipped; 2,816 tests passed, 1 skippedRelease environment follow-up
Commit
e37d4a1edaaa87715f41f6834b25b13f326d8f56replaces the proposed separate pilot environment with exactrelease. The preflight still fails closed unlessreleasehas required reviewers and a non-null deployment branch policy. Every direct release job and both reusable workflow calls now use exactrelease; local credential-free validation still uses no environment and no secrets.Live gate
No signing, notarization, upload, store submission, or provider deployment was performed. The committed pilot matrix has
distribution: null. The existingreleaseenvironment currently has no protection rules, no deployment branch policy, and no configured release inputs. A live run therefore fails closed until an authorized owner protects and configures that existing environment plus the repository/org-scoped inspection token. Nopilot-nonproductionenvironment is required or created.Stacked on #429. Do not merge as part of CODE-561 execution.