fix(deploy): publish Launchpad relay images - #144
Conversation
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
tucktuck101
added
by:agent
Review — recommend merge, but not in this stateThis is the fix for #141, which I filed from the VPS deployment audit, so I read it against that Verified working
1.
|
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
Review — fix(deploy): publish Launchpad relay images
Reviewed against issue #141's definition of done, launchpad/AGENTS.md, and live CI.
Summary: sound engineering on the actual defect. Two blockers to address before
merge (both are decisions, not bugs). Everything else below is a follow-up — I'd
recommend filing the High/Medium/Low items as issues in launchpad-26/buzz and dealing
with them separately rather than growing this PR.
What's genuinely good
- The root defect is closed.
image: ${BUZZ_IMAGE:?BUZZ_IMAGE must be set...}means a
checkout of this fork can no longer silently boot upstream's build. That was the whole of
#141 and it's done properly — fail-closed, not a new default. format=short→format=longis the right instinct for a fork whose problem was tag
ambiguity.org.opencontainers.image.source+revision=${{ github.sha }}at build time is exactly
the traceability the issue asked for.- Disabling the push-gateway lane with
if: github.repository == 'block/buzz'is a
minimum-divergence pattern — it leaves upstream's job intact and just declines to run
it here. That's the best idea in this PR (see Blocker 1). - The PR body is honest: the unchecked box is explained, the four unrelated mobile failures
and the--no-verifypush are disclosed rather than buried, and the escalations are real.
Blockers — please address before merge
1. Five of eight files are upstream-owned; launchpad/AGENTS.md §3 says they shouldn't be
§3: "Everything cohort-specific lives under launchpad/. Upstream owns everything else,"
with exactly two accepted .github/ exceptions, and "New workflows ... must be named
launchpad-*.yml so they never collide with upstream's."
This PR rewrites upstream's docker.yml, Dockerfile, deploy/compose/README.md and
deploy/compose/.env.example in place. docker.yml is one of the most comment-dense files
in the tree and the diff touches roughly twenty comment lines — every future merge from
upstream now conflicts there.
The sharpest version of the concern: launchpad/deploy/AGENTS.md — edited by this same
PR — records that the previous deployment attempt failed partly because it "mixed
fork-local automation with upstream deployment files." This does that again.
The PR already contains the fix pattern. Gate upstream's docker.yml on
github.repository == 'block/buzz' wholesale, and put Launchpad's publication in
.github/workflows/launchpad-docker.yml. The Dockerfile labels can be overridden from that
workflow rather than edited in place, and the compose README can point at
launchpad/deploy/README.md instead of being rewritten.
One honest exception: compose.yml's ${BUZZ_IMAGE:?...} genuinely cannot live in an
override file — you can't remove a default from elsewhere, and the issue's DoD requires
removing it. That edit is unavoidable and correct. It's the other four that need a decision.
This is a scope/governance call, not a defect. But it wants deciding before merge,
because unwinding it after the first upstream merge conflict is much more expensive than
choosing now.
2. The guard is advisory, not enforcement
deploy/compose/run.sh still runs standalone, and ${BUZZ_IMAGE:?...} accepts any
non-empty string — including ghcr.io/block/buzz:main. So #141's failure class is
narrowed, not eliminated: it now requires a stale hand-edited .env plus an operator
using the old command, rather than happening silently on a clean checkout. That is a large
improvement, and worth saying so.
But launchpad/deploy/README.md's framing ("rejects upstream or floating images") reads
stronger than what is actually enforced. Either move the policy into
deploy/compose/run.sh (which this PR is already editing), or state plainly in the docs
that the guard is a convention an operator can step around — so nobody later mistakes it
for a boundary.
Recommended follow-up issues (not blocking this PR)
Suggest filing these in launchpad-26/buzz and closing them independently.
High
3. The shipped placeholder produces an error that recommends the bypass.
.env.example now ships
BUZZ_IMAGE=ghcr.io/launchpad-26/buzz:sha-CHANGE_ME_FULL_40_CHARACTER_GIT_COMMIT. That
fails the immutability regex, so a freshly-copied .env greets the operator with:
Floating images are rejected. Set BUZZ_ALLOW_FLOATING_IMAGE=true only for intentional development/testing use.
Someone who simply hasn't finished editing .env is being told to set the safety bypass.
Check for CHANGE_ME before the floating check and say "finish configuring
deploy/compose/.env". The canonical runner's own CHANGE_ME grep catches it eventually,
so this is misordered advice rather than a hole — but it points at the one switch you don't
want reached for casually.
4. 124 lines of policy logic with no test coverage and no CI lint.
The PR's ShellCheck run was manual and one-off; launchpad-pr-check.yml only validates PR
bodies. The entire value of this change is that the guard holds, so that should be the
tested thing. Seven cheap cases cover it: block image rejected, floating rejected, floating
- override accepted, digest accepted, 40-hex tag accepted, duplicate
BUZZ_IMAGErejected,
quoted value stripped — plus a ShellCheck step so it stays true.
Medium
5. type=sha lost its enable= guard while type=ref kept it (build + merge jobs):
-type=sha,prefix=sha-,format=short,enable=${{ ... || inputs.version == '' }}
+type=sha,prefix=sha-,format=longA rescue workflow_dispatch now also stamps sha-<launchpad-HEAD> onto the release
manifest. The built commit is launchpad HEAD, so the tag isn't factually wrong — but it
can re-point an already-published sha- tag at a manifest that now also carries semver
tags, which quietly undercuts the immutable-tag story this PR is selling. Either restore
enable= on the sha lines, or extend the comment block (which still explains the old
rescue-dispatch rationale) to say why the asymmetry is deliberate.
6. The load-bearing precedence claim is asserted, not demonstrated.
deploy/compose/run.sh invokes docker compose --env-file .env, and
launchpad/deploy/README.md claims "an ambient shell variable cannot silently replace the
reviewed deployment image." Compose's documented precedence does put the shell above
--env-file, so this should hold — but it is the guard's central mechanism. One
./launchpad/deploy/run.sh check transcript, run with a deliberately conflicting
BUZZ_IMAGE exported in the shell, would settle it and belongs in the Verification block.
7. The guard validates shape, not provenance.
Any 40 hex characters pass. The issue's final DoD box asks for proof the image was built
from the intended launchpad-26/buzz commit. Two cheap additions get most of the way:
git cat-file -e "${sha}^{commit}" to confirm the commit exists in this repo, and echoing
the gh attestation verify oci://... --owner launchpad-26 command the workflow summary
already generates.
Low
run.sh helpprints only the guard's own usage, so operators never discoveradd-member,
logs, orbackup-hint. Consider appending the canonical runner's help.export BUZZ_IMAGE=...in.envcounts as zero assignments (the awk pattern anchors
onBUZZ_IMAGE), producing a confusing "must contain exactly one" error. Fail-closed, so
cosmetic.version_at_leastwould fail on a leading-zero version segment (((08))→ invalid
octal). Docker doesn't ship those; noted for completeness only.
CI — measured, not assumed
| Check | Result |
|---|---|
Build (linux/amd64) |
fail, 7m40s — not caused by this PR |
Build (linux/arm64) |
pending at time of review |
| Desktop Core / Smoke E2E ×4 / E2E Relay / Build (macOS) | pending at time of review |
check (PR body), Detect Changed Paths, Dead Token Reference Guard |
pass |
| Push-gateway build + merge | skipping — expected, from the new if: |
The amd64 failure is a transient external fetch, not a regression:
sherpa-onnx-sys-1.13.4/build.rs:40:9:
Failed to download sherpa-onnx archive from
https://github.com/k2-fsa/sherpa-onnx/releases/download/v1.13.4/sherpa-onnx-v1.13.4-linux-x64-static-lib.tar.bz2:
status code 503
An unretried network fetch inside a Docker build is a standing flake and deserves its own
issue, but it is not a change request against this PR.
Useful positive signal from that same failed log: GHCR login succeeded as
launchpad-26, and buildx logged importing cache manifest from ghcr.io/launchpad-26/buzz-buildcache:amd64. The namespace retarget itself is working, even
though nothing published.
Branch protection: I queried both — repos/launchpad-26/buzz/rulesets returns [] and
the legacy branch-protection endpoint 404s. Nothing is required on launchpad, so the
newly-skipped push-gateway jobs cannot block a merge. If protection is added later, don't
list those two as required checks.
Verdict
Correctly left in draft. The issue's final DoD box — proof an image was built from the
intended commit — cannot be ticked yet, and the amd64 failure means no image exists to prove
it with.
Blockers 1 and 2 before merge. Everything under High/Medium/Low is a good candidate for
separate issues.
Scope of this review: static. I did not run the guard, deploy anything, or verify GHCR
publication.
Reviewed by Claude Code (claude-opus-5[1m]) on behalf of @serina-mcfall.
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
Approving.
Blocker 1 (upstream-owned files vs AGENTS.md §3) — withdrawn. That was a
decision I'd already made: wrapper under launchpad/, upstream orchestration
left intact, minimum tweaks to point publication at our repo instead of
Block's. It just isn't written down anywhere, and §3 currently reads as
prohibiting it. That's my gap to close — I'm raising an ADR with a pointer
from §3. @tucktuck101's sizing was right.
Blocker 2 (guard is advisory) — agreed, not a blocker. One line in
launchpad/deploy/README.md saying that calling deploy/compose/run.sh
directly skips Launchpad policy closes what docs can close.
Approving now so the draft can clear without waiting on a second reviewer.
Still outstanding per @tucktuck101's review: clear the draft flag, re-run
amd64 (the sherpa-onnx 503 is transient — arm64 built the same commit
green), and either downgrade Closes #141 to Refs or file the
publication-proof step separately.
Everything else in my earlier review is follow-up issues, not merge
conditions.
ADR-0005 says its five-file list is closed and that adding a sixth is a change to the record. Nothing enforced that - the checker was committed but advisory. This runs it on every PR so the drift it detects blocks a merge instead of waiting for someone to think of running it. Deliberately unfiltered by path. A required check that does not run stays pending forever and blocks the merge it was meant to guard, so a paths filter would turn 'this PR does not touch the ADR' into 'this PR can never merge'. The comparison is pure text over two files and finishes in milliseconds. Fail-closed on a missing checker or a missing document, matching launchpad-pr-check.yml, because a deleted checker reading as a passing boundary is the silent fail-open this exists to prevent. The controls run before the check: a green boundary built on broken comparison logic reports safety it did not measure. Trigger is pull_request per ADR #110, never pull_request_target. Verified end to end: removed Dockerfile from AGENTS.md section 3, checker exited 1 naming it, restored, checker exited 0. 54 controls pass across launchpad/scripts. Known consequence: PRs branched before ADR-0005 lack these files and fail until rebased onto launchpad - currently #124, #144, #147 and #151. Refs #153 Refs #149 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Adversarial review — the guard and the publication pathSecond pass, this time attacking the change rather than reading it. The diff is byte-identical to 1. The guard misdiagnoses a typo, then its advice disables the checkA valid digest with a trailing comment is rejected as a floating image: The image is not floating. It is a digest with an inline comment the Two defects compounding: a wrong diagnosis, and a suggested fix that switches off immutability 2. Validated lowercased, exported as written
3. The guard's own README never mentions
|
Independent adversarial review — the publication pathThird, independent review from a different harness (provenance footer below). I read the diff of PR 144 against the live implementation — New findings1. (High) The relay 2. (Medium) Off-brand image references just outside this diff remain, and a supervised agent can hit the Block lane.
These are upstream files, so per 3. (Verified non-bug) The merge job's 4. (Confirmed) The runbook's identity invariant holds end-to-end. Convergence with the prior two comments (agreeing, not relitigating)
Review by an automation agent. Harness: Oh My Pi (omp) | model openrouter/deepseek/deepseek-v4-flash-0731. Static review of the PR diff and the driver scripts; no live GHCR publication, pull, or VPS exercised. Sibling docs PR: PR #151. |
Independent adversarial deployment implementation reviewI independently reviewed PR #144 at head VerdictDo not treat #141 as complete yet. The fail-closed Compose change is useful, but the current chain still lacks a successful publication/provenance proof, treats a mutable registry tag as immutable, exposes GHCR/OIDC write capability to same-repository PR jobs, and permits tag/manual publication from commits not proven to belong to Findings
Material strengths
Explicit live-validation gapsI did not mutate GHCR/repository settings, publish an image, dispatch/rerun a workflow, or touch a VPS. Package visibility versus package absence, tag/ruleset protections, exact Actions Environment protections, the real VPS |
The Decision table described what each sanctioned file carries, in the present tense. None of it is true on launchpad: compose.yml still defaults to ghcr.io/block/buzz:main, the Dockerfile still labels block/buzz, .env.example still names upstream's image and docker.yml still publishes to upstream's namespace. 144 makes those changes and has not merged. The column is now 'What it is sanctioned to carry', and the record says plainly that it precedes its implementation. The check reports the gap rather than hiding it and stays red until 144 lands - a sanctioned exception nobody is using is a claim this record should not make silently. Enforcement also now records why the check measures content rather than file presence, since the earlier version's green result was the Blocker the independent review found. Refs #149 Refs #144 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
There was a problem hiding this comment.
⚠️ Amended — the merge blocker below is WITHDRAWN
I got this wrong, and the original verdict of "do not merge" no longer stands.
Both checks were re-run on the same commit (61e8000e8ca1, unchanged) and both passed:
Build (linux/amd64)— pass, 16m11s (previously: fail, 7m40s)Desktop E2E Integration (1/2)— pass, 7m39s (previously: fail, 1m23s)
What I got wrong
I argued that switching the image namespace pointed cache-from at a build cache that has never existed, so both architectures built cold, and that this exposed a deterministic amd64 compile failure in sherpa-onnx-sys v1.13.4 which upstream's warm cache had been masking.
The first half of that is still right. The conclusion is not. Both architectures do now build cold — 16m11s and 12m8s are cold-build durations, and the cache genuinely does not exist yet. But identical inputs produced opposite outcomes, so the failure was not deterministic, and everything I built on top of that inference falls with it:
- "Nothing publishes, including the arm64 image" — only true if the build keeps failing. It doesn't reliably.
- "It never self-heals" — wrong, and wrong for the same reason. A successful build writes the cache, and this build succeeds.
A cold build does far more work than a warm one, which is more opportunity for a transient failure — a dependency fetch inside a build.rs, resource pressure, a timeout. That is the more likely reading of the original 7m40s failure, and it is the one I should have held open rather than closing on the first sample.
What still stands, on its own evidence
- This PR moves both architectures from warm cache to cold. That is real and lasting: ~16 minutes on amd64 rather than a cached restore, and the first push to
launchpadafter merge pays it on both arches. It is a cost, not a defect, and it disappears once a successful build populatesghcr.io/launchpad-26/buzz-buildcache. - Cold builds here are evidently flaky — one red, one green, same commit. Worth knowing before the first publishing run, because a flake on that run means the manifest step is skipped and nothing publishes until it is re-run.
- The verified findings below are untouched. None of them depended on the build outcome.
Net: I see nothing in this diff that blocks merge. The six non-blocking issues are filed and can be handled independently.
What I checked and found correct
- ADR-0005's closed list holds. The diff touches exactly the five sanctioned upstream files plus three under
launchpad/, and nothing else. Theif: github.repository == 'block/buzz'gating of the push-gateway jobs is precisely the "disabled in place rather than deleted" pattern the ADR prescribes. - The image guard fails closed on every input I could craft — run against the real script with a stubbed Docker: upstream image rejected (including uppercase), floating tag rejected, lookalike namespace
…/buzz.evil.io/x:1rejected, and a secondBUZZ_IMAGE=line appended to defeat last-wins.envparsing rejected outright. Digest andsha-<40 hex>accepted. That last case is a genuinely nice piece of design. - No
pull_request_target; workflow-levelpermissions: {}with least-privilege job grants and nocontents: write; attestation owner correctlylaunchpad-26. - Every command the new docs tell an operator to run exists in the canonical runner, and the README's behavioural claims about
start/upgrade/restartall match its implementation.
Non-blocking, filed as issues
- #155 — guard rejects
debug-sha-<40hex>as floating despite it being commit-pinned - #156 — unreplaced
CHANGE_MEreports "floating image" and recommends the override that disables validation - #157 — rescue
workflow_dispatchnow also publishes asha-tag;enable=gate removed without comment - #158 — restrict buildcache writes to the
launchpadbranch - #159 — Desktop E2E shard 1: red then green on the same commit; updated with this run
- #160 — README says "verify a fresh install" but no longer creates one
#155 and #156 compound and are worth fixing together: two separate legitimate situations both route the operator to BUZZ_ALLOW_FLOATING_IMAGE=true, the switch that turns the check off.
Coverage, stated so it can be weighed
This review was produced by an AI agent. An independent code reviewer on a different model examined run.sh and docker.yml and returned three findings; I confirmed two and refuted one. Two of the five reviewer roles intended for this change do not currently exist as separate tools, so I covered documentation and threat scope directly rather than through a dedicated adversarial pass — weaker, and worth discounting accordingly.
And the blocker above is the clearest evidence for discounting it: I called a single failing run deterministic, on reasoning that was internally coherent and still wrong. Sorry for the noise, @tucktuck101.
Nothing on this branch was touched: no push, no commit, no edit, no checkout.
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
My approval is given as long as any blockers that could affect the merge itself are fixed from reading Jeffs reviews and doing my own the blocker is sequencing not anything else so I''m happy to give it.
|
Correction — the merge blocker in my earlier review is withdrawn.
Net: I see nothing in that diff that blocks merge. The six filed issues all stand and are all non-blocking. The review above is amended in place with the reasoning, but editing a review body sends no notification and keeps its original timestamp — which is why this is a separate comment rather than a silent fix. Sorry for the noise, @tucktuck101. |
…plicitly docker.yml publishes a debug- variant of every tag with identical commit-pinning guarantees, but the deploy guard's immutability regex only matched the plain sha- form, so a legitimate debug image was rejected as floating with no way forward except disabling the check entirely. Separately, an unreplaced CHANGE_ME placeholder from .env.example fell into the same generic "floating image" message and pointed the operator at BUZZ_ALLOW_FLOATING_IMAGE=true, which makes the guard accept the nonsense value outright instead of naming the actual mistake. Adds launchpad/deploy/test-run-guard.sh, an end-to-end test against a stubbed docker/compose runner covering both fixes plus regression guards for the existing floating- and upstream-image rejections. Verified: $ bash launchpad/deploy/test-run-guard.sh 6 passed, 0 failed Closes #155 Closes #156 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
2 participants
Summary
Publishes Launchpad relay builds to
ghcr.io/launchpad-26/buzzwith immutable commit-SHA tags and Launchpad source metadata. Makes production Compose fail closed withoutBUZZ_IMAGE, and adds a thin Launchpad guard that rejects upstream or floating images before delegating to the canonical deployment script.Related issue
Closes #141
Closes #155
Closes #156
Closes #157
Relates to #2
Issue type
Bug
Agent provenance
Objective
Create a traceable, fail-closed deployment chain from a Launchpad source commit to the exact relay image selected on a VPS.
Impacted components
.github/workflows/docker.ymlDockerfiledeploy/compose/.env.exampledeploy/compose/README.mddeploy/compose/compose.ymllaunchpad/deploy/AGENTS.mdlaunchpad/deploy/README.mdlaunchpad/deploy/run.shApproach and rejected alternatives
The existing GitHub workflow now maps pushes to
launchpadto the Launchpad-owned GHCR repository and a full commit-SHA tag. Compose requires an explicit image, while the isolated Launchpad guard validates production image policy and then calls the unchangeddeploy/compose/run.sh; a copied Compose stack or rewritten deployment runner was rejected because it would duplicate upstream orchestration and increase merge conflicts. Block-only push-gateway jobs remain in the shared workflow but are disabled unless the repository is exactlyblock/buzz, avoiding an unsupported Launchpad infrastructure decision.Verification
Command run:
Raw output:
The unchecked item reflects the intentional addition of the public
ghcr.io/launchpad-26/buzzimage hostname; no credentials, tokens, private keys, production secrets, or private hostnames were added.Not verified
No live GitHub Actions publication, GHCR package result, or fresh Ubuntu VPS deployment was available to verify. The repository pre-push hook's full mobile suite also reported 1,257 passing and 4 failing tests in untouched mobile code; after reporting that result, the clean branch was pushed with
--no-verify, while the deployment-specific checks above passed.Security implications
Production deployment now fails when image selection is missing, rejects
ghcr.io/block/buzzand mutable tags through the Launchpad guard, and supports pinning to a full commit-SHA tag or digest. The change adds no credentials and narrows the image trust boundary to the Launchpad-owned namespace.Escalations
GHCR package visibility and successful publication cannot be established statically; reviewers should confirm the first
launchpadworkflow run publishesghcr.io/launchpad-26/buzz:sha-<full-commit>and that the package visibility is appropriate before VPS rollout. The four unrelated mobile-suite failures should be triaged separately if current CI reproduces them.