ci(hub): one approval per component release, not two - #3018
Conversation
Putting deploy-worker in the agent-publish environment cost a second approval prompt. GitHub raises one per batch of jobs that pend together: terminal-hub and agent-ui pend simultaneously and share a prompt, but the deploy has to finish before them, so it always lands in an earlier batch of its own. Dropped the environment from deploy-worker. The reviewer gate is there for the step that cannot be undone — an immutable R2 upload — and this deploy is neither irreversible (redeploying fixes it) nor unchecked (worker-check runs typecheck, the vitest suite, and a bundle before it). It also only runs on a real release someone dispatched, never on a dry run, and the publishes still refuse to start unless it succeeded. This moves CLOUDFLARE_API_TOKEN and CLOUDFLARE_ACCOUNT_ID from environment secrets to repository secrets, which widens their reach from one environment to every workflow in the repo. That is the real cost of the change; the error message and README now say which kind is required so a half-migrated setup fails by name rather than inside wrangler.
|
Verdict: Request changes — one settings change away from approvable. This drops the reviewer gate from the job that deploys the Agent Hub Worker so a component release only prompts a maintainer once instead of twice. The goal is right and the reasoning is written down where the next maintainer will find it, but the change does two things beyond removing a prompt, and neither is accounted for in the comment. 🔒 SECURITY CONCERN: this widens who can push code to the production Hub Worker, and where its Cloudflare credentials live. @kovtcharov-amd
Both are avoidable while still getting one approval: put the deploy job in a second environment that has no required reviewers but keeps the branch/tag restriction, and hold the Cloudflare secrets there. No extra prompt, credentials stay environment-scoped, and the release can still only deploy from a real release ref. Two smaller things: the freshness check that runs right after the deploy reads a repository variable that, until now, was only ever read from inside the gated environment — worth confirming it is defined repo-wide, or that step will quietly check the wrong origin. And this PR is not self-contained: the secrets must be re-created at repo level before the next release tag, or the whole component release fails. Real-world evidenceN/A — CI workflow and docs only; no evidence bundle was produced for this PR, and this code path can only run during an actual tagged release. The verdict rests on static review of the workflow. ( 🔍 Technical detailsIssues🔴 Deploy job loses the environment's ref allowlist, not just the approval (
Recommended shape — a non-gated environment that keeps the ref rule: (Requires creating 🟡 Every other reference to Confirm it is repo-level. Separately, per CLAUDE.md's no-silent-fallbacks rule, this step would be better failing loudly than silently verifying a different origin than the publish jobs upload to: 🟢 The comment overstates the trigger surface ( "It also only runs on a real release the maintainer dispatched, never on a dry run" — the workflow also fires on any 🟢 Migration is not self-contained The secrets must exist at repo level before the next Strengths
|
The review caught that dropping `environment:` threw away more than the second approval. agent-publish also carries the deployment ref allowlist — main, v*, agent-pkg-* — so without it anyone with write access could dispatch this workflow from an arbitrary branch with dry_run unchecked and push that branch's Worker straight to production. The Worker is the manifest validator and holds the R2 binding, so that is a supply-chain surface, and "redeploy fixes it" answers a bad deploy rather than a malicious one. It also moved the Cloudflare credentials from one environment to every workflow in the repo. deploy-worker now runs in a new worker-deploy environment: no required reviewers, so the release still prompts once, but the same ref allowlist as agent-publish and the Cloudflare secrets scoped to it. Also fixed the freshness check, which defaulted to hub.amd-gaia.ai when neither hub URL variable resolved. The publish jobs POST to workers.dev — the WAF fronts the custom domain — so that default verified an origin we do not upload to. It now fails loudly instead. (GAIA_HUB_PUBLISH_URL is in fact repo-level, so this was latent rather than live, but a silent wrong-origin check is exactly what the no-silent-fallbacks rule is for.)
|
Fixed — and the 🔴 was a genuine hole I introduced, not just a scoping preference. You're right that removing Took your recommended shape.
One prompt, credentials stay environment-scoped, and the deploy can still only run from a real release ref. 🔍 On the two smaller points
Migration. The Cloudflare secrets now need to live on Trigger surface. The comment no longer claims "only a real release the maintainer dispatched" — it now says what actually constrains the job, which is the ref allowlist. One thing to be aware of: I created the |
|
Verdict: Approve This is a clean CI security improvement. Moving One suggestion before this goes live operationally: 🟢 The README documents the two secrets the 🔍 Technical details
Set the **deployment branch and tag rules** on `worker-deploy` to match `agent-publish`:
`main`, `v*`, `agent-pkg-*`. Leave **Required reviewers** empty.The |
…n it The setup instructions listed the two secrets but not the environment settings, so the security property this change rests on — the deployment ref allowlist — existed only as a comment in the YAML. Anyone creating worker-deploy from the README would have got an environment with no reviewers and no ref restriction, which is strictly worse than the agent-publish arrangement it replaced. Both settings are now documented with why they matter: reviewers empty (the second prompt is the thing being avoided) and main / v* / agent-pkg-* (the only remaining restriction on a job that deploys the manifest validator).
|
Good catch — fixed in the latest push. You're right that the guarantee was only as good as the setup instructions, and those lived in a YAML comment rather than where someone creating the environment would look. A The README now documents both settings with the reason each matters — reviewers left empty because the second prompt is the whole point, and For what it's worth the live environment already matches; I created it and its three policies via the API while addressing the earlier round, so the config and the docs now agree. |
|
Verdict: Approve with suggestions The environment split is well-reasoned and the fail-loud base URL change is a clear CLAUDE.md compliance win. One security trade-off is worth a maintainer's explicit sign-off. 🟡 The Everything else looks clean:
🔍 Technical detailsSecurity trade-off (release_components.yml:187) Old flow: New flow: Attack surface: write access + push to This is a real change in the security boundary, even if the threat model (trusted contributors with write access) makes it acceptable. Worth a maintainer's explicit "yes, this is intended" before merge. Base URL check (release_components.yml:221-229) The new explicit The error message note ("an environment-scoped one resolves empty here") is accurate: |
…rker body cap (amd#3025) ## Why this matters The Agent UI installers have never been publishable to the hub, and the error blamed the wrong thing. Each one is 106–135 MiB, and **Cloudflare caps a Worker request body at 100 MB on Free/Pro**, so they are rejected with a `413` by the edge before the Worker executes. `MAX_ARTIFACT_BYTES` (250 MiB) was never consulted — the HTML error body is Cloudflare's, not ours — so raising it would have changed nothing. email and terminal-hub only work because they are small (43.5 MiB and under). Artifacts at or above 90 MiB now go straight into the same R2 bucket over the S3 API, which has no such cap, and are published **by reference**: the POST carries the artifact's coordinates instead of its bytes. ## Test plan - [ ] Add `R2_ACCESS_KEY_ID` / `R2_SECRET_ACCESS_KEY` repo secrets (Cloudflare → R2 → Manage API Tokens → Object Read & Write on `gaia-hub`) - [ ] `cd workers/agent-hub && npm test` — 214 pass, 10 of them new - [ ] `pytest hub/agents/email/python/tests/test_publish_to_r2_by_reference.py` — 7 pass - [ ] Dispatch `release_components.yml` with `dry_run=false` and confirm **agent-ui publishes** where it previously 413'd - [ ] Confirm terminal-hub and email still take the inline path unchanged <details> <summary>🔍 How the integrity guarantee survives</summary> The Worker no longer sees these bytes, so "the publisher said so" would be an easy accidental outcome. It isn't: before recording anything, the Worker heads the object and checks its size **and** SHA-256 against what R2 itself stored at PUT time. R2 keeps a whole-object SHA-256 only for **single-part** uploads. An object without one is refused (`artifact_unverifiable`) rather than trusted — so the uploader uses `put_object` with `ChecksumSHA256`, never `upload_file`, which would switch to multipart and silently drop the checksum. A test pins that call shape, because the failure mode is a green upload followed by a rejected publish, mid-release. **Immutability needed rethinking rather than reusing.** Inline, the object's presence in R2 *is* the record, so heading it is the right check. By reference the object always exists by the time the Worker is called, so that same check would 409 against the caller's own upload — the record is the agent manifest, which only lists artifacts this endpoint accepted. Both new-path tests were verified non-vacuous by mutating the verifier to trust the caller's claim; the hash-mismatch and missing-checksum cases fail as they should. **New secrets are R2 S3 credentials**, distinct from the `CLOUDFLARE_API_TOKEN` that deploys the Worker. Missing any of the three fails loudly naming all of them — never a fallback to the Worker path, which cannot work at these sizes. </details> Stacked on amd#2991 (merged) and independent of amd#3018. --------- Co-authored-by: Ovtcharov <kovtchar@amd.com>
Why this matters
Releasing hub components asks for approval twice. GitHub raises one prompt per batch of jobs that become pending together —
terminal-hubandagent-uipend simultaneously and share one, butdeploy-workerhas to finish before them, so it always forms an earlier batch of its own. Any environment-gated job placed before the publishes does this; it was a side effect of #2991 rather than a deliberate choice.Dropping the environment from
deploy-workerleaves exactly one gate, on the step that actually needs a human.Test plan
CLOUDFLARE_API_TOKENandCLOUDFLARE_ACCOUNT_IDto repository secrets (they are currently on theagent-publishenvironment, where this job can no longer read them)dry_run=false— exactly one approval prompt, raised afterdeploy-workerhas already rundeploy-workerstill gates the publishes: they must not start if it failsdry_run=true— no deploy, no approval,worker-checkstill runs🔍 The trade-off
The reviewer gate exists for the part that cannot be undone: an immutable R2 upload. The Worker deploy is neither irreversible (redeploying fixes it) nor unchecked —
worker-checkruns typecheck, the vitest suite, and a full bundle first, and the deploy only happens on a real release someone dispatched.The genuine cost is secret scope: repository secrets are readable by every workflow in the repo, where environment secrets were readable by one. That is a real widening and the reason to keep the token minimally scoped — Workers + R2 on the single account, plus the
amd-gaia.aizone for the custom-domain route.The alternative that keeps them environment-scoped is a second environment with the same reviewers, which trades the extra prompt for an extra environment to keep in sync. Happy to go that way instead if the wider secret scope is the bigger concern.