ci(release): require GAIA_HUB_PUBLISH_URL for hub publish, drop dead fallback - #3032
Conversation
Verdict: ApproveThis makes the hub publish URL a hard requirement: instead of quietly falling back to the public Nothing blocking. Two small things worth a pass before merge:
Real-world evidenceN/A — release-workflow change with no evidence bundle present. A release workflow can't be exercised without cutting a real release, and the two workflows that do have a dry-run mode skip the new guards on purpose (a dry run doesn't publish, so requiring the variable there would be wrong). The verdict rests on static review of the diff plus a check that every publish call site is guarded and that nothing else still reads the removed variable. 🔍 Technical detailsVerification performed
🟢 Minor — misleading remediation hint in the new error message ( The parenthetical was copied from the Two smaller points in the same message: "would silently target a hardcoded fallback URL" describes the behaviour this PR just deleted, so it reads as a counterfactual to anyone debugging after the merge; and the 300-character string is duplicated verbatim five times — a composite action under 🟢 Minor — skill doc may not have landed ( The diff removes the "Unset → uploads fall back to the custom domain and 403" sentence, but the file in the checked-out PR tree still contains it (and shows as locally modified). If the edit is genuinely on the branch, ignore this; if not, the skill still tells a maintainer the fallback exists, which is the one contradiction this PR is trying to erase. 🟢 Minor — chat header now documents an unused output ( The header gains "downloads + the lock baseUrl" for Strengths
|
There was a problem hiding this comment.
Thanks for this — the assert steps themselves are nicely done, and the sweep across all seven call sites is thorough. I approved it earlier and then went back for a closer look at one thing that was nagging me, so apologies for the reversal.
The thing I got wrong is upstream of this PR: #2937's framing. GAIA has only one Agent Hub. hub.amd-gaia.ai and the workers.dev URL are two front doors onto the same Worker and the same R2 bucket, so the fallback being removed here couldn't actually publish "to the wrong place" — it would reach the same bucket through a door whose WAF rejects large bodies, which shows up as a 403 rather than a silent misroute. This PR implemented exactly what the issue asked for; the issue just described the failure mode inaccurately, and that description has now been carried into four workflow headers.
I'd still like to keep the behaviour change. Requiring the publish origin explicitly gives a much better error than a 403 from a WAF, and that's a real improvement. It's just the explanation around it I'd like to correct before merging, so the next person reading those headers doesn't come away thinking there are two hubs.
One related thing worth mentioning: the fallback that actually reaches users is one this PR reasonably left alone. GAIA_HUB_BASE_URL turned out to be set nowhere — not at repository level, not in any environment — so the download and lock URLs were quietly resolving off a hardcoded default, including the ones baked into the binaries.lock.json we ship. I've set it as a repository variable, so that chain now resolves from configuration. No action needed from you on that, just closing the loop since it's the same variable.
Two small things and I'm happy to merge:
- Adjust the rationale in the PR body and the four workflow headers — the failure mode is "uploads through the WAF-fronted origin get a 403" rather than "publishes to the wrong place".
- Repoint the remediation hint. The new error messages send the operator to
workers/agent-hub/README.md, which doesn't currently mention the publish URL variable or the two-origin split. Either point somewhere that covers it, or add a short note there — the latter would be genuinely useful to have.
🔍 Technical details
One Worker, one bucket, two origins — workers/agent-hub/wrangler.toml:12-26:
name = "agent-hub"
routes = [{ pattern = "hub.amd-gaia.ai", custom_domain = true }]
workers_dev = true
[[r2_buckets]]
binding = "BUCKET"
bucket_name = "gaia-hub"release_agent_chat.yml:59-62 notes the same thing: "the Worker endpoint is the same one either way." So for large binaries the old chain already failed loudly at the WAF, and for a small wheel it would have reached the correct bucket — which is why "wrong place" overstates it.
The fallback that does ship to users, correctly left in place here:
release_agent_gaia.yml:228and:736,release_agent_chat.yml:263,release_agent_email.yml:510—base_url=${GAIA_HUB_BASE_URL:-https://hub.amd-gaia.ai}/${HUB_PREFIX}/${VERSION}release_agent_gaia.yml:912—gen_binaries_lock.py --sidecar-base-url "${{ steps.ver.outputs.base_url }}"
Variable scope. Set at repository level, deliberately not environment level:
| value | |
|---|---|
GAIA_HUB_BASE_URL |
https://hub.amd-gaia.ai |
GAIA_HUB_PUBLISH_URL |
https://agent-hub.kalin-bb5.workers.dev |
release_agent_gaia.yml's version job has no environment:, and release_components.yml's deploy-worker runs on worker-deploy rather than agent-publish — an environment-scoped variable would resolve empty in both and silently fall back to the hardcode. release_components.yml:230 already makes this point. A URL that gets printed in logs and baked into lock files also doesn't want to be a secret.
Remediation hint. GAIA_HUB_PUBLISH_URL and workers.dev don't appear anywhere under workers/ except the wrangler.toml comment above, including in the README the six new assert steps cite.
Nothing wrong with the asserts themselves — each sits in the same job as the publish step it guards, mirrors the existing token asserts, and passes values through env: rather than inlining them. That's the right pattern.
80061a3 to
77260ad
Compare
|
Thanks for the thorough review and the correction — you're right that the framing overreached: one Worker, one R2 bucket, two doors. Both points are addressed: 1. Rationale corrected (PR body + workflow headers). The failure mode is now described as what it actually is: the WAF-fronted 2. Remediation hint repointed. Also rebased onto latest |
|
@itomek both points look addressed now, so this is only waiting on a re-review to clear the changes-requested state. The rationale is corrected everywhere it appeared — PR body plus all four workflow headers now describe the WAF 403 on the upload door rather than a misroute, and they say explicitly that there is one Worker and one bucket behind two front doors. The remediation hint is now accurate too: Thanks for catching that the issue's framing was wrong rather than just the implementation — that distinction is what kept it from propagating into four more headers. |
…fallback
The publish steps picked their target URL from a three-level chain
${GAIA_HUB_PUBLISH_URL:-${GAIA_HUB_BASE_URL:-https://hub.amd-gaia.ai}}.
GAIA_HUB_BASE_URL exists nowhere in the repo (dead middle branch), and
the hardcoded URL was a latent fallback: if GAIA_HUB_PUBLISH_URL is ever
unset or renamed, a release would silently publish to the hardcoded
address instead of failing.
Drop the dead branch and hardcoded default from every publish --base-url,
and add a fail-loudly assert for GAIA_HUB_PUBLISH_URL mirroring the
existing 'Require the publish token' / 'Assert hub publish token present'
steps: a missing variable is now a startup-time error naming what is
missing and how to set it, not a silent wrong-origin publish.
GAIA_HUB_BASE_URL is kept where it is the documented download/lock origin
(the ver steps' base_url output, GET-verify steps) — only the publish
target is now explicit and required.
Closes amd#2937
The 'an environment-scoped one resolves empty here' hint was copied from the worker-deploy job (environment: worker-deploy), where a variable scoped to agent-publish genuinely resolves empty. The publish jobs declare environment: agent-publish, where an environment-scoped variable does resolve, so the hint sent maintainers looking in the wrong place. Point to both valid locations instead. Also trim the chat workflow header: chat publishes a wheel with no binaries.lock.json, so GAIA_HUB_BASE_URL is a downloads-origin only there.
…doors The publish URL is not a second hub: hub.amd-gaia.ai and the workers.dev URL are two front doors onto the same Worker + R2 bucket. The managed WAF fronting the custom domain 403s large uploads, so POST /publish goes through the workers.dev origin; a publish through the WAF-fronted door fails loudly with a 403, it does not land somewhere else. Rewrite the GAIA_HUB_PUBLISH_URL variable docs in the three agent release workflow headers to say this, and add a 'Publishing origins (one Worker, two doors)' section to workers/agent-hub/README.md covering both variables and why they are repository-level.
77260ad to
7bc1cf7
Compare
|
Both points from the review are addressed, and the branch is rebased onto current main (no conflicts):
Also rebased onto upstream main (78bbc30) — the merge conflict from #3035 is resolved. YAML parse check passes on all 4 workflows. |
|
🟡 The email and gaia assert steps are missing the dry-run gate that the chat and components workflows have, causing dry runs to fail unnecessarily when In 🔍 Technical details
- name: Assert hub publish URL present
shell: bash
env:
GAIA_HUB_PUBLISH_URL: ${{ vars.GAIA_HUB_PUBLISH_URL }}
run: |Should be: - name: Assert hub publish URL present
if: steps.dry.outputs.dry_run == 'false'
shell: bash
Compare: |
|
Thanks for the review — I checked both files on the updated head ( Evidence (head
So the step is intentionally unconditional: every run of these two workflows publishes, and a missing
|
Both asks landed after this review: 7bc1cf7 corrects the rationale, 8189110 repoints the remediation hint. Verified at head 39eb0c6 — no stale "publishes to the wrong place" framing remains in the four workflow headers or SKILL.md, and workers/agent-hub/README.md now documents the two-origin split the new error messages point operators at. Dismissing so the PR isn't blocked on a stale review.
kovtcharov-amd
left a comment
There was a problem hiding this comment.
Both review asks are in, and the change does what it says.
The rationale is corrected everywhere it was wrong — all four workflow headers and SKILL.md now describe the real failure mode (a WAF-fronted origin 403s large uploads) rather than implying there are two hubs. And the six new error messages now point at a workers/agent-hub/README.md that actually explains the two-origin split.
I also checked the thing most likely to bite in a release: deleting an env: line while the script below it still reads that variable would abort a publish mid-run under set -euo pipefail. There are no orphans, and the download/lock URL chain is untouched.
One nit, non-blocking — release_agent_gaia.yml still declares GAIA_HUB_BASE_URL in the publish step's env: after the body stopped reading it. The same line was dropped from the other three. Dead declaration, no functional impact.
Worth knowing for whoever merges: no PR check exercises these files — they only fire on agent-pkg-* tags. A workflow_dispatch dry-run with the variable deliberately unset is the only way to see the new assert actually fire.
🔍 Technical details
Rationale sweep — zero hits at head 39eb0c60:
grep -niE "wrong place|falls back to (the )?(custom domain|GAIA_HUB_BASE_URL)|uploads will 403" \
-- .github/workflows/ .claude/skills/ workers/
→ (no matches)
Orphaned-variable check — every surviving ${GAIA_HUB_BASE_URL} still has its declaration in the same step:
| File | env: |
used at |
|---|---|---|
release_agent_chat.yml |
237 | 265 |
release_agent_email.yml |
496, 770 | 514, 780 |
release_agent_gaia.yml |
208, 734 | 239, 751 |
release_components.yml |
— | — (all removed together) |
The if: guard asymmetry is correct, not a bug. release_agent_chat.yml:290 guards the assert with dry_run == 'false'; email and gaia don't. Those two publish jobs have no dry-run path — they're gated by environment: agent-publish on a tag push, so there's nothing to guard against.
Nit location: release_agent_gaia.yml:894.
Summary
Removes the dead
GAIA_HUB_BASE_URLmiddle branch and the hardcodedhttps://hub.amd-gaia.aidefault from every Hub publish--base-url, and requiresGAIA_HUB_PUBLISH_URLexplicitly via new fail-loudly assertion steps.Why
The publish steps pick their target URL from a three-level fallback chain:
--base-url "${GAIA_HUB_PUBLISH_URL:-${GAIA_HUB_BASE_URL:-https://hub.amd-gaia.ai}}"GAIA_HUB_BASE_URLexists nowhere in the repository (verified against repo + environment variable lists), so the middle branch is dead. The remaining hardcoded default is the WAF-fronted custom domain. There is exactly one Agent Hub Worker and one R2 bucket:hub.amd-gaia.aiand theworkers.devURL are two front doors onto the same Worker (workers/agent-hub/wrangler.toml). The managed WAF fronting the custom domain 403s large uploads (thePOST /publishpath), so CI must publish through theworkers.devorigin — same Worker + bucket, just the door that isn't WAF-blocked.With the old chain, an unset/renamed
GAIA_HUB_PUBLISH_URLwould silently fall back to the custom domain and every large upload would fail mid-release with a WAF 403, after smaller artifacts were already stored immutably. A missing required value should be a startup-time error naming what is missing and how to set it, not a 403 partway through the release. (Publishing through the wrong door fails loudly at the WAF — it does not land in a different place.)The same files already have the right pattern: the "Require the publish token" / "Assert hub publish token present" steps fail loudly on a missing value. This change mirrors them for the URL.
Linked issue
Closes #2937
Changes
--base-urlsimplified to${GAIA_HUB_PUBLISH_URL}; the deploy-worker "Verify the deployed build is live" step now checksGAIA_HUB_PUBLISH_URLonly (drops the dead second branch).dry_run == 'false'guard as the token assert); publish--base-urlsimplified; header documents the variable as required.--base-urlsimplified in the active publish step and the disabled whole-package-zip step; inline comment updated (no longer "falls back to the public origin").--base-urlsimplified; header updated.GAIA_HUB_PUBLISH_URLdocumented as required (no silent fallback).GAIA_HUB_BASE_URL/GAIA_HUB_PUBLISH_URL, why they are repository-level variables, and why the WAF-fronted door 403s uploads rather than misrouting them.GAIA_HUB_BASE_URLis intentionally kept where it is the documented download/lock origin (theversteps'base_urloutput used bygen_binaries_lock.py, GET-verify steps,upload_to_r2.shmanual path) — only the publish target is now explicit and required.Test plan
python3 -c "import yaml,glob; [yaml.safe_load(open(f)) for f in glob.glob('.github/workflows/release_*.yml')]"— all 4 workflows parse.grep -rn 'GAIA_HUB_PUBLISH_URL:-' .github/— only the new-z "${GAIA_HUB_PUBLISH_URL:-}"assertion checks remain; zero publish fallback chains.actionlinton all 4 workflows — only the two pre-existingif: falsedisabled-step warnings (untouched).🤖 This PR was authored by Kagura, an AI agent.