ci(docker): pin manual deploys to approved SHA (BLO-18108) - #796
Conversation
|
🔗 Paperclip issue: BLO-18108 |
1 similar comment
|
🔗 Paperclip issue: BLO-18108 |
|
@ally please review the immutable production dispatch contract, especially exact-SHA checkout in both jobs, the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0f53727
Critical Issues (0)
None.
Important Issues (1)
- [types / gstack / native-codex]
.github/workflows/docker.yml:51—target_shais not constrained to the documented full immutable commit SHA.actions/checkoutaccepts branches, tags, and abbreviated SHAs, while the later check validates only the commit that the ref happened to resolve to. A dispatch withmastertherefore succeeds, and because the build and deploy jobs resolve the input independently,masteradvancing between them can make the deploy target differ from the approved/build target. Require exactly 40 hexadecimal characters and verify the checked-outHEADequals that normalized input; alternatively resolve once and pass the full resolved SHA from the build job to deploy.
Suggestions (2)
- [gstack]
.github/workflows/docker.yml:388— The deploy job now depends ondocker buildx imagetools inspect, but this customarc-deployjob does not provision Buildx. Consider addingdocker/setup-buildx-actionor explicitly checkingdocker buildx versionin the tooling step so scheduled deploys do not rely on undeclared runner state. - [code / gstack]
.github/workflows/docker.yml:407— The summary resolves an immutable digest, but Helm still deploys the mutable seven-character tag. Consider deploying the recorded digest, or at least using a full-SHA tag, to make the deployed artifact match the audit record exactly.
Strengths
- Manual targets are checked for ancestry from
origin/master. - Manual dispatches no longer move
latest-k8s-vendoredbackward. - Build and deploy summaries record the full source commit and Harbor digest.
- Deployment serialization and pre-rollout artifact checks reduce rollout races.
Recommended Action
- Enforce and carry one full immutable SHA through both jobs before merge.
- Consider the two artifact/tooling hardening suggestions opportunistically.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally Please re-review exact head |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fde42ba
Critical Issues (0)
None.
Important Issues (1)
- [gstack / native-codex]
.github/workflows/docker.yml:212— The approved 40-character commit is still reduced to a mutable seven-character image tag, and the digest resolved at line 418 is only written to the summary; Helm deploys the tag again at line 437. A prefix collision or retag between inspection and image pull can therefore run an artifact other than the one recorded for the approved SHA, so the immutable dispatch contract stops at checkout rather than covering the deployed image.- Export
${{ steps.build.outputs.digest }}frombuild-and-push, verify that exact digest indeploy, and configure Helm to deploy the repository by digest. At minimum, use the full SHA in the tag, but digest pinning is what removes the inspect-to-pull race.
- Export
Suggestions (1)
- [tests]
scripts/check-docker-deploy-timeout.test.js:45— Extend the invariant test to require build-digest propagation and digest-based Helm configuration so the artifact binding cannot regress while the textual SHA plumbing remains intact.
Strengths
- Manual dispatch now rejects non-full SHAs before checkout and verifies the exact normalized commit in both jobs.
- Both jobs enforce reachability from
master, and manual rollback builds preservelatest-k8s-vendored. - Buildx is explicitly provisioned before artifact inspection, addressing the prior runner-state concern.
Recommended Action
- Bind the rollout to the digest produced by the approved-SHA build before merge.
- Add the digest-propagation invariant test in the same change.
|
@ally Addressed the important digest-binding finding in |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally Please complete exact-head review of |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1c2419f
Critical Issues (0)
None.
Important Issues (2)
- [code / gstack / native-codex]
.github/workflows/docker.yml:277— A manual rollback checks out the selected historical commit for both the image build and the Helm deployment. Any approvedmasterancestor from before this PR has a chart whosepaperclip.imagehelper ignoresimage.digest; Helm accepts the extra--set-stringvalue but renders the mutable tag, silently defeating the new immutable rollout guarantee for exactly the historical-SHA rollback case. Fail closed by rendering and asserting the resulting workload image equalsrepository@${DIGEST}, reject targets whose chart lacks digest support, or deploy with a trusted current chart while building the selected source SHA. - [errors / gstack]
.github/workflows/docker.yml:423— Digest comparison is skipped wheneverneeds.build-and-push.outputs.image_digestis empty. Empty is valid only for a scheduled run where the build job was skipped, but the same branch permits a successful push/manual build with a missing output to deploy whichever artifact the mutable seven-character tag resolves to. Checkneeds.build-and-push.result: require a syntactically valid, matchingEXPECTED_DIGESTwhenever the build job succeeded, and allow an empty expected digest only when that job was skipped forschedule.
Suggestions (0)
None.
Strengths
- Full-SHA validation, exact checkout verification, and the
masterancestry guard now bind both jobs to one approved source commit. - The normal build-backed path verifies Harbor's resolution against the Buildx digest and renders Kubernetes images by digest.
- Manual dispatch preserves
latest-k8s-vendored, and the chart retains tag fallback for non-CI consumers. - The added invariants cover cross-job SHA propagation, Buildx provisioning, digest propagation, and Helm helper use.
Recommended Action
- Make digest rendering fail closed for historical manual targets.
- Require the build digest on every build-backed deployment.
|
@ally Follow-up |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 36aa6d2
Critical Issues (0)
None.
Important Issues (1)
- [gstack / native-codex]
deploy/helm/paperclip/templates/_helpers.tpl:110— Rendering workloads asrepository@digestremoves thesha-<TIP_SHORT>text that the existing scheduled deploy gate uses to detect “already running tip.” After any immediate push deploy, the next schedule run sees a recent pod but can never satisfyRUNNING_IMG | grep -qF "sha-${TIP_SHORT}", so it performs a redundant rollout instead of skipping it. This restores the periodic worker churn and stranded-run risk that the six-hour debounce is intended to prevent.- Resolve the candidate tag to a digest before the schedule decision and compare the StatefulSet image with
harbor.blockcast.net/paperclip/paperclip@<digest>(or persist an equivalent digest identity for the gate). Add an invariant test covering a recent digest-pinned tip.
- Resolve the candidate tag to a digest before the schedule decision and compare the StatefulSet image with
Suggestions (0)
None.
Strengths
- Manual dispatches validate and propagate one full commit SHA through both jobs.
- Build-backed deploys fail closed when Harbor’s tag resolution differs from the Buildx digest.
- Helm renders and deploys the verified digest while preserving tag fallback for chart consumers.
- All 19 reported checks on this head completed without failure.
Recommended Action
- Update the scheduled debounce identity check for digest-based image references before merge.
|
@ally Please re-review head |
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 71266ec
Looks good. No Critical or Important issues found.
Strengths
- Manual dispatch validates and propagates one full approved commit SHA through build and deploy.
- Build-backed deploys require Harbor's tag resolution to match the Buildx digest, and Helm is pinned to that digest.
- Historical charts fail closed unless every Paperclip workload renders the expected digest reference.
- The scheduled debounce compares the running StatefulSet image to the resolved tip digest, preserving the six-hour no-op behavior.
- Invariant tests cover the SHA contract, Buildx provisioning, scheduled digest comparison, and Helm digest propagation.
Recommended Action
Merge after required CI checks pass.
Ally's review of #825 found the file contradicting itself: :100-106 said the review bot posts formal reviews "as the App", while :151-159 said its formal approvals come from the `allyblockcast` user seat. Both cannot be true. Evidence settles it in favour of the second. Ally's own instructions approve via PAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token (AGENTS.md:309-310) and fall back to `--comment` under the default App token. Observed on this repo, every APPROVED review is authored by `allyblockcast` (type User) — PRs #817, #813, #810, #803, #802, #797, #796, #791, #789. So: comment-mode reviews come from the App, formal approvals from the user seat. State that once, in the identity list, and drop the incorrect "as the App" claim plus the "only reason the user seat is in this workflow" assertion that was not supported by any of the above. This strengthens rather than weakens the prohibition: the approve path genuinely runs under the shared seat, so a review posted under it is byte-for-byte indistinguishable from the reviewer's own. Push/create/merge guidance unchanged. Manifest regenerated: sha256 b1cc7c35, 8573 bytes, verified against the file directly (the catalog suite passes with a stale manifest, so its green is not the signal here — see BLO-18955).
…n (BLO-18925) (#825) * docs(skills): forbid formal PR reviews under the user-seat merge token (BLO-18925) The github-pr-workflow bundled skill reaches every engineering agent (recommendedForRoles: [engineer]). It enumerated the user-seat token's sanctioned uses -- branch push, gh pr create, gh pr merge -- and routed "everything else" to the default App token, but never named formal reviews. An agent holding a credential GitHub accepts an APPROVE from, looking at a red review/ally-complete gate it needs green to merge, had a short path to posting one. Prohibition by omission is not a control. State it explicitly: no gh pr review under the user-seat token in any form, and no ally-verdict:/Reviewed head: marker under it. The reason is spelled out -- the seat is the same identity the reviewer's own approvals come from, so a review posted under it is indistinguishable from the reviewer's, clears the gate for a change nobody reviewed, and leaves an audit trail that cannot separate the two. The sanctioned move on a red gate is to get a review, not to post one. Sanctioned uses (push, create, merge) are unchanged, and the reviewer's own --approve path is untouched. Also regenerates generated/catalog.json, which pins per-file sha256 and sizeBytes. Note: no test or CI job asserts manifest/file consistency, so this regeneration is not covered by the suite -- filed as a follow-up. * docs(skills): state the reviewer identity model once (BLO-18925) Ally's review of #825 found the file contradicting itself: :100-106 said the review bot posts formal reviews "as the App", while :151-159 said its formal approvals come from the `allyblockcast` user seat. Both cannot be true. Evidence settles it in favour of the second. Ally's own instructions approve via PAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token (AGENTS.md:309-310) and fall back to `--comment` under the default App token. Observed on this repo, every APPROVED review is authored by `allyblockcast` (type User) — PRs #817, #813, #810, #803, #802, #797, #796, #791, #789. So: comment-mode reviews come from the App, formal approvals from the user seat. State that once, in the identity list, and drop the incorrect "as the App" claim plus the "only reason the user seat is in this workflow" assertion that was not supported by any of the above. This strengthens rather than weakens the prohibition: the approve path genuinely runs under the shared seat, so a review posted under it is byte-for-byte indistinguishable from the reviewer's own. Push/create/merge guidance unchanged. Manifest regenerated: sha256 b1cc7c35, 8573 bytes, verified against the file directly (the catalog suite passes with a stale manifest, so its green is not the signal here — see BLO-18955). * fix(skills): keep PR authors off the reviewer seat Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: CTO <cto@blockcast.network> Co-authored-by: Blockcast CTO <cto@blockcast.net> Co-authored-by: Omar Ramadan <omar@blockcast.net> Co-authored-by: Paperclip <noreply@paperclip.ing>
… (BLO-18997) (#832) * docs(skills): forbid formal PR reviews under the user-seat merge token (BLO-18925) The github-pr-workflow bundled skill reaches every engineering agent (recommendedForRoles: [engineer]). It enumerated the user-seat token's sanctioned uses -- branch push, gh pr create, gh pr merge -- and routed "everything else" to the default App token, but never named formal reviews. An agent holding a credential GitHub accepts an APPROVE from, looking at a red review/ally-complete gate it needs green to merge, had a short path to posting one. Prohibition by omission is not a control. State it explicitly: no gh pr review under the user-seat token in any form, and no ally-verdict:/Reviewed head: marker under it. The reason is spelled out -- the seat is the same identity the reviewer's own approvals come from, so a review posted under it is indistinguishable from the reviewer's, clears the gate for a change nobody reviewed, and leaves an audit trail that cannot separate the two. The sanctioned move on a red gate is to get a review, not to post one. Sanctioned uses (push, create, merge) are unchanged, and the reviewer's own --approve path is untouched. Also regenerates generated/catalog.json, which pins per-file sha256 and sizeBytes. Note: no test or CI job asserts manifest/file consistency, so this regeneration is not covered by the suite -- filed as a follow-up. * docs(skills): state the reviewer identity model once (BLO-18925) Ally's review of #825 found the file contradicting itself: :100-106 said the review bot posts formal reviews "as the App", while :151-159 said its formal approvals come from the `allyblockcast` user seat. Both cannot be true. Evidence settles it in favour of the second. Ally's own instructions approve via PAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token (AGENTS.md:309-310) and fall back to `--comment` under the default App token. Observed on this repo, every APPROVED review is authored by `allyblockcast` (type User) — PRs #817, #813, #810, #803, #802, #797, #796, #791, #789. So: comment-mode reviews come from the App, formal approvals from the user seat. State that once, in the identity list, and drop the incorrect "as the App" claim plus the "only reason the user seat is in this workflow" assertion that was not supported by any of the above. This strengthens rather than weakens the prohibition: the approve path genuinely runs under the shared seat, so a review posted under it is byte-for-byte indistinguishable from the reviewer's own. Push/create/merge guidance unchanged. Manifest regenerated: sha256 b1cc7c35, 8573 bytes, verified against the file directly (the catalog suite passes with a stale manifest, so its green is not the signal here — see BLO-18955). * docs(skills): author agent PRs under the App token, not the user seat (BLO-18997) The github-pr-workflow skill instructed, in bold, "When the user-seat token is mounted, author your PR under it". That instruction is self-defeating: the review bot's formal APPROVE is posted under that same `allyblockcast` user seat, so a seat-authored PR makes author == approver, GitHub refuses the approval, the bot degrades to comment-mode, and `review/ally-complete` maps a clean comment-mode review to `pending`. The skill routed every engineering agent into a gate that cannot go green. Why it was introduced (c7d580d, 2026-06-28): on the premise that the review bot posts as the App, so App-authored PRs could only ever get comment-mode. That premise was true when written — the App posted 12 formal Bot approvals on human-authored PRs between 2026-07-11 and 2026-07-16. Ally's approve path then moved to the user seat (no Bot approval after 2026-07-16) and the skill was never updated, inverting its own rationale. Evidence on Blockcast/paperclip: - all 10 approvals in the last 40 PRs are `allyblockcast/User` (the seat); - 9 of those PRs are App-authored, 1 human-authored; - all 3 seat-authored PRs in repo history (#792, #825, #826) have zero approvals — #792 and #825 got comment-mode only, #826 nothing; - merges: 49 `kkroo`, 9 `allyblockcast[bot]` (the App), 0 by the seat — so the seat is not needed for merge either. Also documents the recovery path for an already-seat-authored PR (close and re-create from the same branch under the App; author is fixed at creation), the `gh api user` identity check, and why seat-pushing is unsafe under `require_last_push_approval`. Stacked on the BLO-18925 branch, which rewrites the same section; its forged-review prohibition and rationale are preserved intact. Co-Authored-By: Claude <noreply@anthropic.com> * docs(skills): repin the catalog test to App-authoring; don't hardcode base (BLO-18997) Addresses both Important findings from Ally's review of c69bbfe. 1. `shipped-catalog.test.ts` still pinned the contract this PR inverts. The assertion `toContain('PAPERCLIP_GITHUB_TOKEN_FILE="$USER_TOKEN_FILE"')` (added by #718, when seat-authoring was the sanctioned path) failed once the seat-selection recipes were removed, so the catalog could not land. The test now asserts the *new* contract instead of the old one: - the App-authoring rule is present ("Author and push under the default App token."), - the old seat-authoring instruction ("author your PR under it") and the seat-selection recipe are both absent, - the formal-review prohibition on the seat is present. The `GH_TOKEN="$AUTHOR_TOKEN"` guard from #718 is kept — the wrapped `gh` still overrides GH_TOKEN from a token file, so setting it selects nothing. 2. The seat-authored-PR recovery recipe hardcoded `--base master`, which would silently re-create a stacked PR — or one in a repo with a different default branch — against the wrong base, changing both the diff and the check set. It now captures `headRefName`/`baseRefName`/`title`/`body` from the original PR *before* closing it and passes those exact values through, with a post-condition to confirm base and head match. Manifest regenerated for the new SKILL.md: 10643 bytes, sha256 477cf058…, contentHash sha256:6cbe8cec…. Hashes computed with the same algorithm as `buildContentHash` (catalog-builder.ts:769) and validated by reproducing the previous committed values exactly. `npx vitest run src/shipped-catalog.test.ts` → 11/11 pass. Co-Authored-By: Claude <noreply@anthropic.com> * docs(skills): make seat-PR recovery SHA-safe; guard credentials structurally (BLO-18997) Addresses both important findings from Ally's review of 3c16d95. Recovery recipe (SKILL.md): the sequence promised "same branch and SHA" but never captured headRefOid, and closed the original before the replacement existed. Two failure modes: a branch that moved between capture and re-create silently reopened on an unreviewed head, and a failed `gh pr create` left the review artifact closed with no replacement. Now captures headRefOid up front, re-validates it against the remote ref immediately before the close (aborting instead of closing on mismatch), and reopens the original on any post-close failure, including a replacement that lands on the wrong head or base. Regression test: the credential check rejected three exact string spellings, so seat authoring could return under a renamed variable or a literal token path. Replaced with a structural scan — extract executable shell fences, and for any fence running `git push` / `gh pr create|merge|review`, reject any credential selection (token-file assignment, literal seat-token path, GH_TOKEN / GITHUB_TOKEN assignment, `--with-token`, `gh auth login|switch`). Includes an anti-vacuity assertion so a drifting extractor or retagged fence fails loudly rather than passing on an empty scan. A third test pins the recovery invariants. Verified: 7 mutations each turn the suite red (token-file next to `gh pr merge`, renamed GH_TOKEN next to `gh pr create`, dropped headRefOid, dropped reopen path, validation moved after the close, and fences retagged non-executable). Recovery fence parses under `bash -n`. Manifest regenerated (SKILL.md now 12510 bytes, sha256 160083f0…). * docs(skills): make the seat-PR recovery fail closed and prove App identity (BLO-18997) Ally's review at ee9f9e3 found the recovery recipe could still destroy the review artifact on a failure it did not anticipate, and that the regression test could not have caught the recipe it exists to keep out. - Fail closed before the destructive close. Every captured field is validated — full 40-hex SHA, non-empty refs — so a failed `gh pr view` or a null field no longer leaves an empty ORIG_SHA that compares equal to an empty REMOTE_SHA and "passes" the guard on two blanks. `set -euo pipefail` makes an unhandled failure abort rather than fall through to the next destructive line. - Roll back on every unsuccessful exit, not two anticipated ones: an EXIT/INT/TERM trap armed before the close reopens the original and takes any replacement down with it, and says so loudly when the rollback itself fails. - Prove the actor is the App installation before starting, and verify the replacement's author after creating it. Previously the recipe would happily recreate under the seat — the exact defect being recovered from — and report success. The preflight asserts the App's 403 signature rather than the seat's absence, so a seat login, a network failure, or a broken `gh` all abort. (`PAPERCLIP_GITHUB_TOKEN_FILE` is exported by default pointing at the App token, so its mere presence cannot be the signal.) - Validate the created PR number, so a blank `gh pr create` output cannot send the verification step to `gh pr view ""`. - Disclose what the replacement does not carry: labels, assignees, reviewers, milestone, and the original's draft state. Tests: the recovery recipe is now extracted from the shipped skill and executed against a stub `gh` across 12 scenarios, asserting the actual `gh` argv sequence — that no pre-close failure ever reaches `gh pr close`, and that every post-close failure reopens the original. That replaces presence-only regex assertions. Each guard was mutation-tested: removing the trap fails 6 tests, removing the App preflight 2, hardcoding `--base master` 1, and dropping both SHA-format checks fails the blank-both case by reaching the destructive close. The `git push` detector now matches git's global-option forms, so the historical `git -c http.https://github.com/.extraheader= push` seat-authoring recipe is rejected; a fixture pins that it would be. Catalog regenerated; SKILL.md sha256 d79f6270…5311 / 16133 bytes verified against the file directly. * fix(skills): make seat PR recovery signal safe Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: CTO <cto@blockcast.network> Co-authored-by: Blockcast CTO <cto@blockcast.net> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Omar Ramadan <omar@blockcast.net> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: allyblockcast[bot] <allyblockcast[bot]@users.noreply.github.com>
Thinking Path
Production deploys must bind one approved source commit to one immutable runtime artifact across build and deploy jobs. A movable ref or image tag can resolve differently between stages, so the workflow validates a full SHA, exports the Buildx digest, verifies deploy-time tag resolution still matches that digest, and makes Helm render the repository by digest. This preserves a single auditable source-to-runtime identity while failing closed if registry state changes before deployment.
What Changed
target_shafor manual production deploy dispatchesmasterimage.digestsupport and deployrepository@sha256:...latest-k8s-vendoredrollback behaviorVerification
go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.12 ... .github/workflows/docker.ymlpassednode --test scripts/check-docker-deploy-timeout.test.jspassed (4/4)helm lint deploy/helm/paperclippassedhelm templaterendered the digest reference for every workload and retained tag fallbackRisks
Digest pinning adds an optional chart value but preserves existing tag behavior when
image.digestis empty. Scheduled deploys resolve the existing commit tag to a digest; build-backed deploys additionally require that resolution to equal Buildx output and fail closed on mismatch.Model Used
OpenAI GPT-5.6 Sol
Required workflow prerequisite for Blockcast/onprem-k8s#1653.
Tracking: BLO-18108