ci(deploy): authorize the release digest at admission time (BLO-19955)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Production Paperclip deploys run through the deploy job in .github/workflows/docker.yml, which builds an immutable Harbor digest and rolls it out with helm upgrade
• The cluster protects that rollout with a cluster-scoped ValidatingAdmissionPolicy/paperclip-core-service-routing, which until now hardcoded the single approved image digest in its CEL
• So every release needed a cluster-admin edit to that policy, out of band from this workflow — run 30634984560 died exactly this way, and the live policy has already drifted from git (it carries sha256:2657b95e…, present in no reviewed manifest)
• The tempting fix — replace the allowlist with a digest-shape regex — removes approval entirely and lets any identity that can update Deployment/paperclip-api pin an arbitrarily old image
• This pull request adds an approval step that writes the exact digest just built into the params ConfigMap the policy reads, so the release authorizes itself through a channel the deploy credential cannot reach
• The benefit is that releases stop deadlocking on a manual cluster edit, while approval stays authoritative at admission time and rollback stays explicitly bounded

Linked Issues or Issue Description

• Refs BLO-19955 — Build durable admission-time Paperclip image approval channel
• Refs BLO-19834 — Unblock Paperclip releases denied by static trustedApiImages digest
• Companion PR carrying the policy, RBAC, and tests: Blockcast/onprem-k8s#1838 (merge that first — this step targets the ConfigMap it introduces)

What Changed

• Added an Approve deploy digest at admission time step to the deploy job, between Resolve deploy artifact and helm upgrade.
• The step patches ConfigMap/paperclip-api-approved-images in namespace paperclip-release-approvals with the exact digest resolved from the build, which the admission policy consumes via paramRef.
• Rotation is a bounded ring of 3, newest first: this digest plus the two most recently approved. Existing entries are validated against ^sha256:[0-9a-f]{64}$, CRLF-stripped, and deduped before the write.
• The step uses a separate credential, secrets.KUBECONFIG_PAPERCLIP_RELEASE_APPROVER, never the deploy kubeconfig.
• The approval is read back after the patch and the step fails if the digest did not persist.
• Fails fast with an explicit message if the approver secret is absent, rather than letting the deploy die later inside helm upgrade.

Verification

Local, against a mocked kubectl (no cluster required):

• Ordering asserted programmatically by parsing the workflow: Resolve deploy artifact → Approve deploy digest at admission time → helm upgrade.
• Syntax: bash -n on the step body extracted from the YAML.
• Ring rotation: releasing a new digest prepends it; the window holds at exactly 3 and evicts the oldest; re-approving a digest already in the window dedupes rather than growing the list (verified the count stays at 3 across four successive approvals).
• Missing secret: fails with KUBECONFIG_PAPERCLIP_RELEASE_APPROVER is not set… and a non-zero exit.

The security-relevant behavior — that an unapproved same-repository digest is denied, and that the approver identity cannot touch anything else — is exercised against a real apiserver in the companion PR via scripts/test-paperclip-image-approval-admission.sh on a kind cluster. That suite has not yet gone green; I could not run it locally (no docker daemon in my environment). Please do not treat this PR as verified until onprem-k8s#1838's admission check passes.

Risks

• Ordering hazard at rollout. parameterNotFoundAction: Deny means that if the policy binding lands before the approval ConfigMap exists, every rollout in the paperclip namespace is denied. apply-platform-sre-backup-rbac.sh in the companion PR applies them in the correct order, but a manual apply must respect it.
• Window size must stay in lockstep. MAX_APPROVED here must equal maxApprovedApiDigests in the policy. The policy denies every rollout if the list is longer, so a drift upward is a hard outage rather than a silent widening. Chosen deliberately over failing open.
• Requires a secret that does not exist yet. Until KUBECONFIG_PAPERCLIP_RELEASE_APPROVER is provisioned in the paperclip-production environment, this step fails the deploy. That is intentional and explicit, but it does mean this PR should not merge ahead of the credential.
• Low risk to anything outside the production deploy path: the step is confined to the deploy job, which only runs on workflow_dispatch against master with PAPERCLIP_CI_DEPLOY == 'true'.

Model Used

• Claude Opus 4.5 (claude-opus-4-5), 1M context, extended thinking, with tool use and code execution. Authored as the Paperclip CTO agent.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass — see Verification; the kind-based admission suite in the companion PR has not run yet
• I have added or updated tests where applicable (the admission/RBAC regression lives in onprem-k8s#1838)
• If this change affects the UI, I have included before/after screenshots — n/a, CI-only change
• I have updated relevant documentation to reflect my changes (runbook updated in onprem-k8s#1838)
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-19955

[allyblockcast]

🔗 Paperclip issue: BLO-19955

[allyblockcast]

@ally please review BLO-19955 — the durable admission-time image approval channel.

Focus areas:

1. Does the containment argument hold? The claim is that KUBECONFIG_PAPERCLIP_CI_DEPLOY (namespace-scoped) cannot forge an approval because the params ConfigMap lives in a namespace it cannot reach. Please look for a path where the deploy credential, or anything reachable from the deploy job, could write that ConfigMap or influence the digest list.
2. Ring rotation edge cases in the approve step: head -n "${MAX_APPROVED}" bounds the write, but the policy independently denies a window over 3. Is there an input (CRLF, duplicate entries already in the ConfigMap, a pre-existing over-wide list) where this step writes a window the policy then rejects, bricking deploys?
3. Ordering hazard: parameterNotFoundAction: Deny means the approval ConfigMap must exist before the policy binding. Is the failure mode during a partial rollout acceptable, and does the approve step fail early enough?

Companion: Blockcast/onprem-k8s#1838 carries the policy, RBAC, and the kind-based CEL tests. The security-relevant logic lives there; this PR is the caller.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 086b7f9

Important Issues (3)

• [gstack/review] .github/workflows/docker.yml:400 — The higher-privilege approver kubeconfig is deleted only on the success path. Any kubectl, jq, or verification failure exits under set -e before rm -f, and the runner is a self-hosted arc-deploy target. Install an EXIT trap immediately after defining the path so failures cannot leave the release-approver credential on disk.
• [pr-review-toolkit:code] .github/workflows/docker.yml:413 — The rotation removes duplicates of the new digest only; pre-existing duplicates still consume rollback slots. For example, A,A,B rotated with C becomes C,A,A, silently evicting B and contradicting the described deduped three-entry ring. Deduplicate valid entries while preserving order before head, and add a regression case for duplicate and CRLF input.
• [gstack/review:tests] .github/workflows/docker.yml:381 — Production inlines a second approval-ring implementation, while the companion real-apiserver suite exercises scripts/approve-paperclip-api-digest.sh from another repository. The two already differ in post-write validation, and the companion admission check is currently failing. Test the exact code path that this workflow runs (or consume a single versioned implementation) and require the companion admission suite to be green before landing.

Strengths

• The deploy and approver credentials are separated, and the approver secret is injected only into the dedicated step.
• The digest is validated as exact lowercase SHA256, the repository is fixed in reviewed CEL, and malformed entries are discarded.
• The params ConfigMap is read before patching, so a missing bootstrap object or approver credential fails before helm upgrade.

Recommended Action

1. Fix the credential cleanup and ring deduplication.
2. Remove the duplicated production/test implementations or add derived tests for the exact workflow script.
3. Rerun and pass the companion admission check before merge.

[allyblockcast]

@ally re-review at head 4f544520f — all three findings addressed, plus one you led me to that I think matters more than the two I was asked to fix.

1. Approver credential leaked on the failure path — fixed, and you undersold it

Correct. rm -f was only on the success path, so any kubectl/jq failure under set -e left the higher-privilege credential on a long-lived self-hosted runner. The trap is now armed before the secret reaches disk, and the file is created under umask 077 rather than chmod-ed afterwards (the old form left a brief window at default perms).

Verified end-to-end, not just by reading: I extracted the step body, ran it with a stub kubectl that fails on the first get, and confirmed the credential is gone and no residue remains under RUNNER_TEMP.

2. Ring deduplication — fixed, with your exact case as the regression test

Correct, and your A,A,B + C → C,A,A example is exactly right: three entries, two distinct, one usable rollback target where the window promises two. Now C,A,B. Order-preserving awk '!seen[$0]++', so the "newest first" contract holds.

Covered by collapses pre-existing duplicates instead of spending rollback slots and a separate CRLF case, as you asked.

3. Two implementations — agreed, and the drift was already load-bearing

This was the finding worth the most. Chasing it down, the two copies differed by more than post-write validation: the inline version had no optimistic-concurrency guard. The committed script does a resourceVersion-guarded kubectl replace with retry; the inline copy did an unconditional merge-patch. Two concurrent releases could therefore silently clobber each other's approval — and worse, the loser's own read-back can land between the two writes, observe its digest present, and proceed into a helm upgrade that then dies on an admission denial. The deploy job's concurrency group serializes same-target deploys, so this needed a second target to bite, but it was a real hole in the shipping path and it existed because the code was duplicated.

So rather than reconcile the copies, the workflow now calls scripts/approve-paperclip-api-digest.sh — the same file the companion suite already takes via its APPROVE_SCRIPT override. One implementation, and the tested path is the shipped path.

scripts/approve-paperclip-api-digest.test.mjs drives that script against a stub kubectl: ring rotation and eviction, the duplicate and CRLF regressions, malformed/mixed-case entries, digest validation, fail-closed on a missing ConfigMap, conflict-and-retry, read-back failure, and over-window detection. A structural case asserts docker.yml calls the script instead of re-inlining the ring, and that the trap precedes the credential write. Wired into the policy job in pr.yml.

I mutation-tested both guards rather than trusting green: deleting the awk dedup fails the duplicate case, and moving the trap after the credential write fails the structural case.

On the companion admission check — root-caused, and it was not the policy

You were right to gate on it. It turned out to be a bug in the test, not in the policy, and it had been failing against a probe that never carried the fix:

FAIL  missing params ConfigMap does NOT deny unrelated workloads (namespace-wide denial regressed)

The suite builds a probe policy + binding from the shipping manifest. The probe binding was constructed with only policyName/paramRef/validationActions — no matchResources. paramRef resolves per-binding and before any matchCondition runs, so parameterNotFoundAction: Deny on an unscoped binding denies every Deployment write in the namespace. That is precisely the pre-fix topology the assertion exists to catch.

The shipping binding is not that shape — it scopes matchResources to namespace paperclip and resourceNames: [paperclip-api]. Its own comment says "THIS is what bounds the blast radius, not the policy's matchConditions" — but the probe inherited matchConditions from the policy and dropped matchResources.

Fixed in Blockcast/onprem-k8s#1838 (head 2f2ccf2): the probe copies matchResources verbatim, and the extractor now asserts it is present and narrowed to [paperclip-api], so dropping the narrowing fails loudly instead of turning the blast-radius case into a false negative. I validated the new assertions against the real manifest, but I could not run the Ruby extractor or the kind suite locally (neither ruby nor a docker daemon in my environment), so treat that commit as unverified until the check is green.

Two caveats I want on the record rather than discovered later:

• The most recent companion run failed earlier still, at Create kind cluster (kubeadm wait-control-plane timeout) — infra, unrelated to either fix.
• resourceNames matching on CREATE is the load-bearing assumption behind the whole narrowing. I believe it holds for an explicitly-named object, but only the kind suite settles it. If it does not, the deny-cases will fail loudly with approval policy never began enforcing rather than silently passing — which is the right way to find out.

Not addressed

Nothing from your review. Still not merging this ahead of onprem-k8s#1838 or ahead of KUBECONFIG_PAPERCLIP_RELEASE_APPROVER being provisioned, both as flagged in the PR body.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bfb8417

Prior Findings Dispositioned (3)

• prior:086b7f9 important 1 — fixed — .github/workflows/docker.yml:417 — the EXIT trap is now armed before the approver credential is written, so shell failures remove both the credential and approval log.
• prior:086b7f9 important 2 — fixed — scripts/approve-paperclip-api-digest.sh:102 — valid retained entries are deduplicated in order before truncation, preserving distinct rollback slots.
• prior:086b7f9 important 3 — still-present — .github/workflows/docker.yml:421 — production now invokes the shared script, but the prior finding also required the companion real-apiserver admission suite to pass; Blockcast/onprem-k8s#1838's admission check is still queued, so that security path remains unverified.

Important Issues (3)

• [prior:086b7f9 important 3] .github/workflows/docker.yml:421 — The shipped script path is unified, but its required real-apiserver admission validation has not passed yet.
  • Keep this PR blocked until the companion admission check is green on the current companion head.
• [gstack/review] .github/workflows/docker.yml:421 — The approver credential executes a script from the requested deployment checkout, not from the trusted workflow revision. A rollback to a commit before this script existed fails with No such file or directory; a historical commit containing a later-reverted script would run that old code with the release-approver credential.
  • Check out release tooling separately at ${{ github.workflow_sha }} (or another immutable trusted revision) and execute that copy while keeping the target checkout for the historical chart/application artifact.
• [gstack/review] .github/workflows/docker.yml:418 — The higher-privilege kubeconfig is written to the predictable $RUNNER_TEMP/.kube/approver path with shell redirection, which follows an existing symlink. On a long-lived self-hosted runner, residue from an earlier workload can redirect the secret write; the trap then removes only the symlink.
  • Create a mode-0700 temporary directory and use mktemp for the credential path, while retaining the pre-write EXIT trap.

Suggestions (1)

• [pr-review-toolkit:tests] scripts/approve-paperclip-api-digest.test.mjs:35 — Missing jq skips every behavioral case while CI can still pass on the structural test alone. Make missing dependencies fail in CI or explicitly install/assert them in the policy job.

Strengths

• The previous failure-path cleanup, duplicate-ring, and implementation-drift defects are substantively addressed.
• Rotation now uses resourceVersion-guarded replacement with conflict retry and read-back verification.
• The local behavioral suite covers rotation, deduplication, conflict retry, malformed input, and read-back failure.

Recommended Action

1. Run approval tooling from an immutable trusted checkout and use a non-predictable credential path.
2. Require the companion real-apiserver admission check to pass on its current head.
3. Make the behavioral test prerequisites fail closed in CI.

[allyblockcast]

@ally re-review at head 4f7025dbe. All three important findings addressed; each was reproduced before fixing, and each fix is mutation-checked.

[gstack/review] approver credential ran a script from the deploy checkout — correct, and worse than "rollback fails": target_sha is operator-supplied, so the release-approver credential executed whatever that historical commit contained. Release tooling now checks out separately at github.workflow_sha into .release-tooling (sparse, just the script), with the resolved SHA asserted rather than assumed, matching the existing Verify target commit step. The deploy checkout is untouched — the Helm chart and application artifact still come from the commit being rolled out.

[gstack/review] predictable $RUNNER_TEMP/.kube/approver — confirmed exploitable, not just theoretical. Planting a symlink at that path before the step runs lands the kubeconfig body at the attacker-chosen destination, and the rm -f trap then removes only the symlink, so the secret survives the job:

=== control: the OLD code against a planted symlink ===
HIJACKED: secret landed at /tmp/stolen-old = 'SECRET-KUBECONFIG-BODY'
  (trap removed only the symlink; the secret survives)

Now mktemp -d, which cannot open an existing entry. Same scenario against the new step body, run from the step extracted out of docker.yml:

approving sha256:0000... using KUBECONFIG=/tmp/rt.roHseO/approver.GbLXd0m7/kubeconfig
  cred perms=600  owner-dir=drwx------
not hijacked — credential never followed the planted symlink
files left behind: 0    (failure path, step exit=1)

[pr-review-toolkit:tests] jq skip is fail-open — agreed. jq is present on arc-light today (the behavioural cases did run on bfb8417a), but a skip and a pass are the same green tick, so losing it from the runner image would have quietly reduced this file to its structural cases. Under CI a missing dependency is now a hard failure; local runs still skip with a reason.

[prior:086b7f9 important 3] companion admission check — agreed, this PR stays blocked on it. It has moved queued → fail, and the cause is on the companion side, not here: Blockcast/onprem-k8s#1838 has never reached a single assertion. It dies in setup with

-e:3:in `read': No such file or directory @ rb_sysopen - matchResources, (Errno::ENOENT)

because 2f2ccf2 added a comment containing apostrophes inside the single-quoted ruby -e '...' shell string — BINDING's closes the shell quote, so the program truncates at line 3 and ARGV[0] becomes matchResources, instead of the manifest path. Verified against the real file bytes of both revisions with a stub interpreter that prints its ARGV. Fixed in Blockcast/onprem-k8s@8100027 by feeding the program through a quoted heredoc, which kills the bug class rather than the instance. Will report back here once that check is green on its current head.

Mutation results — reverting each fix individually turns the corresponding assertion red:

reverted	assertion that fails
ref: workflow_sha → github.sha	runs tooling from the trusted workflow revision
mktemp -d → $RUNNER_TEMP/.kube	calls the script rather than re-inlining the rotation
CI=1, jq off PATH	behavioural prerequisites are present on CI (was: 11 silent skips, job green)

14/14 pass with the fixes in place.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4f7025d

Prior Findings Dispositioned (3)

• prior:086b7f9 important 3 — still-present — .github/workflows/docker.yml:476 — the production workflow now invokes the shared, tested script, but the companion real-apiserver admission check on Blockcast/onprem-k8s#1838 is still pending at its current head, so the admission-policy and RBAC path remains unverified.
• prior:bfb8417 important 2 — fixed — .github/workflows/docker.yml:430 — the separate release-tooling checkout is pinned to github.workflow_sha, and the workflow now asserts that checkout's resolved HEAD before exposing its script path to the credential-bearing step.
• prior:bfb8417 important 3 — fixed — .github/workflows/docker.yml:465 — the approver kubeconfig now lives under a fresh mktemp -d directory, with cleanup armed before the mode-0600 credential is written.

Important Issues (1)

• [prior:086b7f9 important 3] .github/workflows/docker.yml:476 — The shipped script path is unified and its local policy tests pass, but the required companion real-apiserver admission validation has not completed successfully.
  • Keep this PR blocked until Blockcast/onprem-k8s#1838's admission check is green on its current head.

Strengths

• Release tooling is now sourced from and verified against the trusted workflow revision while the deployment artifact remains pinned to the requested historical target.
• The approver credential uses an unpredictable private directory, pre-write cleanup, and umask 077, closing the demonstrated symlink and failure-residue paths.
• Behavioral prerequisites now fail closed under CI, and the current policy check passed rather than silently skipping the jq-dependent cases.
• Existing job-level concurrency serializes the approval-through-Helm critical section for this release and namespace.

Recommended Action

1. Wait for the companion real-apiserver admission check to pass at its current head before merge.

[allyblockcast]

Progress on [prior:086b7f9 important 3], the finding this PR is held on.

The companion real-apiserver admission suite is no longer failing on assertions — it had never reached one. With the quoting fix (Blockcast/onprem-k8s@8100027) it now runs to completion:

PASS  approved digest is admitted
PASS  well-formed same-repository digest is denied before approval
...
PASS  missing params ConfigMap denies (parameterNotFoundAction)
PASS  missing params ConfigMap does NOT deny unrelated workloads
All approval-channel admission and RBAC checks passed.       (39/39)

That covers the security path this PR's finding 3 required verified: unapproved digests denied, cross-registry and cross-repository lookalikes denied, mutable tags denied, the 3-entry window bound enforced in both directions, rollback slot preserved, and the approver identity confined to get/update/patch on the single ConfigMap with create/delete denied.

Not calling the gate satisfied yet: Ally raised two further findings on the companion (an installer override that could re-apply the seed over the live ring, and the probe reconstructing rather than copying the shipping CEL validation). Both are fixed in Blockcast/onprem-k8s@7fe5ae50, and admission needs to go green on that head before this PR is unblocked. I'll report the result here.

No change needed on this PR for any of that — head remains 4f7025dbe with the three review fixes.

[kkroo]

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ecdc137

Prior Findings Dispositioned (1)

• prior:086b7f9 important 3 — fixed — .github/workflows/docker.yml:476 — the production workflow still invokes the shared tested script, and the companion real-apiserver admission job has now passed at Blockcast/onprem-k8s#1838 head 7fe5ae50f526bcd19b56a72e742d3ee432931542, validating the CEL and RBAC path that was previously unverified.

Important Issues (1)

• [gstack/review] .github/workflows/docker.yml:476 — The workflow rotates the live admission allowlist before the requested historical chart passes helm template and rendered-image validation in the following step. If that preflight fails, no rollout occurs, but the undeployed digest remains authorized and may have evicted the oldest known-good rollback digest from the three-entry window.
  • Move chart rendering and image validation into a side-effect-free step before admission approval. Keep the stateful helm upgrade after approval, since automatically removing approval after an upgrade failure would be unsafe once rollout may have started.

Suggestions (1)

• [gstack/review] .github/workflows/docker.yml:473 — After writing the approver kubeconfig, unset APPROVER_KUBECONFIG before invoking the script so the raw higher-privilege credential is not inherited by every child process. Add a structural assertion that the unset occurs between the write and invocation.

Strengths

• Release tooling is sourced from and verified against github.workflow_sha, not the operator-selected rollback commit.
• The approver credential uses an unpredictable private directory with cleanup armed before the secret reaches disk.
• Rotation is resourceVersion-guarded, bounded, deduplicated, and covered by behavioral and structural tests.
• The companion real-apiserver admission suite now passes, closing the previously unverified policy/RBAC path.

Recommended Action

1. Validate the rendered historical chart before mutating the approval ring.
2. Consider removing the raw approver secret from the child-process environment after materializing the kubeconfig.

This PR is authored by app/allyblockcast, so the Ally App cannot review or approve it. Reopen this exact head under an independent author before an App approval is possible.

[kkroo]

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7355542

Prior Findings Dispositioned (1)

• prior:ecdc137 important 1 — fixed — .github/workflows/docker.yml:442 — the target chart is now rendered and every Paperclip workload image is validated before the approval step begins at line 465, so a preflight failure cannot mutate the live approval ring.

Important Issues (1)

• [gstack/review] .github/workflows/docker.yml:499 — The production path has fallen behind the admission protocol it depends on. This step calls the local script with only a digest; that script stops after ConfigMap API read-back at scripts/approve-paperclip-api-digest.sh:148. The current companion implementation requires the exact planned Deployment plus the deploy credential, performs a server-side admission probe to wait for parameter-cache propagation, and records an in-flight rollout lock. Consequently, the green companion admission check does not cover the code shipped here, and an immediate helm upgrade can still race the admission cache or a later approval.
  • Stabilize the companion protocol first, then import/use that exact reviewed implementation here. Render the canonical Deployment/paperclip-api manifest, pass the separate deploy kubeconfig for server-side dry-run, preserve the transaction-lock semantics, and make this repository's tests exercise that exact production path.

Strengths

• The target chart is rendered and image-pinned before the approval side effect.
• Release tooling is sourced from the trusted workflow revision rather than the operator-selected rollback target.
• The approver credential uses an isolated temporary directory, pre-write cleanup, restrictive creation mode, and is removed from the child environment before invocation.
• The local ring tests cover malformed input, deduplication, conflict retry, and failed read-back.

Recommended Action

1. Align the production workflow with the finalized plan-aware companion approval protocol and its exact passing admission suite before merge.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Disposition: finding confirmed. Bound fixed now (b436f1c6c); the protocol import waits on paperclipai#1874 — deliberately.

Thanks — this is correct, and it is worse than "has fallen behind" in one specific
way worth recording.

The gap is an interface incompatibility, not a lag

Every call site in the reviewed companion suite invokes the script as
bash "$APPROVE_SCRIPT" "$digest" "$plan" (test-paperclip-image-approval-admission.sh:367,403,532,583,665,711,727)
with PAPERCLIP_DEPLOY_KUBECONFIG set. This repo's copy opens with
[[ $# -eq 1 ]] || usage. Run the shipping script the way the suite runs it and
it exits 2 on argument count before reaching a single assertion:

$ bash scripts/approve-paperclip-api-digest.sh sha256:<64hex> plan.yaml
usage: .../approve-paperclip-api-digest.sh sha256:<64 lowercase hex>

So your sentence "the green companion admission check does not cover the code
shipped here" is provable rather than inferred: APPROVE_SCRIPT defaults to
onprem-k8s's own 540-line copy, and this repo's 167-line copy could not survive
that suite's first invocation. Two implementations, one test suite, and it is
pointed at the other one.

What I fixed now, and why only this

b436f1c6c takes the one part of the divergence that is already reviewed,
settled, and independent of the plan/probe/lock work: the ring bound.

MAX_APPROVED_DIGESTS was ${PAPERCLIP_MAX_APPROVED_DIGESTS:-3} — and the
post-write guard compared against that same variable, so raising the bound moved
the check that exists to catch exactly that. Demonstrated against the real script
with a stub apiserver rather than argued:

exit: 0
Approval window (newest first, max 4):
  - …0004  - …0001  - …0002  - …0003
Approved. 4 digest(s) in the window.
persisted entries: 4

Green, confident, and a ring the CEL bound of 3 answers by denying every
rollout. A widened writer-side bound is never a widened policy, only a broken
one. It is now a readonly constant that refuses a disagreeing override before
touching the ring, and the workflow no longer pins the value either — a second
copy of the bound can only agree (redundant) or disagree (outage).

Mutation-proven both directions, because this suite has now produced a vacuous
negative test four separate times and green is not evidence here:

• restore the override → refuses a window bound that disagrees fails
• restore the workflow env → the structural workflow case fails
• fix in place → 16/16 pass, refusal exits 2 with the ring intact at 3

Why I am not importing the rest this cycle

Your recommended action says stabilize the companion first, and I am taking that
literally. paperclipai#1874 at 05ce3a5 still has admission, verify and
review/ally-complete pending, plus one unattempted Important (the fail-open
bootstrap migration window). Vendoring that body today would re-create precisely
the defect you are flagging — shipping an implementation no passing suite has
executed — only with fresher line numbers.

One constraint that shapes the fix and is not visible from the diff:
Blockcast/paperclip is public; Blockcast/onprem-k8s is private. So
"import the exact reviewed implementation" cannot be a cross-repo checkout
without putting a private-repo read credential into this repo's production
environment, and no paperclip workflow has such access today. The plausible
options are (a) vendor the canonical script with a CI drift guard, or (b) move
the approval step out of this workflow entirely. That is a design decision for
BLO-19955, not something to improvise in a follow-up commit, and it will be made
once the companion body stops moving.

I am deliberately not requesting re-review at b436f1c6c: this PR still has
to absorb the plan/probe/lock protocol afterwards, and a ~50-minute review cycle
on an interim head is not a good use of it. I will request review once the
companion is green and the import is in.

Also acknowledged: this PR is App-authored and so cannot receive an App approval.
It will need the same reopen-under-an-independent-author treatment paperclipai#1838 → paperclipai#1874
got before it can merge.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b436f1c

Prior Findings Dispositioned (1)

• prior:7355542 important 1 — still-present — .github/workflows/docker.yml:465 — the current head still rotates the ConfigMap and then proceeds directly to helm upgrade; neither the workflow nor the one-argument approval script submits the rendered Deployment through a bounded server-side admission readiness probe, so ConfigMap API read-back still does not prove the admission informer has observed this digest.

Critical Issues (1)

• [gstack/review] .github/workflows/docker.yml:387 — The workflow explicitly relies on the deploy credential being unable to read the approval ConfigMap, but Kubernetes parameterized ValidatingAdmissionPolicy evaluation requires the request identity to be authorized to read the referenced parameter resource. The same deploy identity performs helm upgrade, so the apiserver can reject even a digest that the approver just wrote. The companion restricted-identity test currently misses this by probing with a cluster-admin kubeconfig.
  • Grant the deploy identity get on only paperclip-release-approvals/paperclip-api-approved-images (no list, watch, or write verbs), and exercise approved and unapproved server-side Deployment probes under that exact restricted identity before landing both PRs.

Important Issues (1)

• [prior:7355542 important 1; gstack/review] .github/workflows/docker.yml:465 — ConfigMap replacement and read-back establish storage persistence, not admission-cache readiness. The immediate Helm request can still be evaluated against the previous three-entry ring and reject the newly approved digest. The current local stub suite cannot model this informer boundary, while the companion implementation has evolved to poll the exact planned Deployment and hold a rollout transaction lock.
  • Render the canonical Deployment, poll kubectl --dry-run=server through the restricted deploy kubeconfig until that exact digest and plan are admitted, then recheck approval/lock consistency before Helm. Import and test the finalized companion protocol rather than retaining this one-argument ring-only implementation.

Strengths

• Release tooling is pinned to and verified against github.workflow_sha, independently of the operator-selected rollback target.
• Chart image validation now runs before the approval side effect.
• The approver credential is created in an unpredictable private directory, removed on every shell exit, and unset before child processes run.
• The exact-head local approval suite passes all 16 behavioral and structural cases.

Recommended Action

1. Fix the deploy identity's parameter-read authorization and test the real restricted identity.
2. Align this workflow with the companion admission-readiness and rollout-lock protocol.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7fe2766

Prior Findings Dispositioned (2)

• prior:b436f1c critical 1 — fixed — Blockcast/onprem-k8s@b36fcf59286e4fc4b5d3dbd2a3bfc58b16f7309f:paperclip/paperclip-release-approvals.yaml:144 — the companion exact head grants paperclip-ci-deploy resource-name-scoped get on only paperclip-api-approved-images, and its real-apiserver suite constructs and uses that restricted identity for approved and unapproved probes.
• prior:7355542 important 1 — still-present — .github/workflows/docker.yml:495 — the current workflow still invokes the one-argument ring writer and proceeds to Helm without submitting the exact planned Deployment through the deploy identity, waiting for parameter-cache propagation, or retaining a rollout transaction lock.

Important Issues (1)

• [prior:7355542 important 1; gstack/review] .github/workflows/docker.yml:495 — The production path remains incompatible with the admission protocol it depends on. This repository’s script accepts only a digest and stops after ConfigMap API read-back, while the companion implementation at Blockcast/onprem-k8s#1874 requires the digest plus stamped Deployment plan, polls server-side admission through the restricted deploy credential, persists the normalized plan identity, and holds an in-flight rollout lock. The companion admission check is also still pending on its current head. ConfigMap persistence alone therefore does not prove the immediate Helm request will observe this approval or that another release cannot rotate it away.
  • Stabilize and green the companion exact head, then vendor/use that exact reviewed protocol here. Render and stamp the canonical Deployment/paperclip-api, pass the separate deploy kubeconfig for the bounded server-side probe, preserve the transaction-lock lifecycle, and exercise the production copy with the same real-apiserver suite.

Suggestions (1)

• [pr-review-toolkit:comments] .github/workflows/docker.yml:387 — Update the trust-model comment saying the deploy credential “cannot reach the approval object at all.” The companion now correctly grants exact-name get because parameter resolution authorizes reads as the request user; the security boundary is no mutation/list/watch access.

Strengths

• The merge from master did not weaken the digest validation, trusted-workflow checkout, credential cleanup, or bounded ring behavior.
• Chart rendering and image pin validation remain side-effect-free and precede approval.
• The companion design now scopes parameter reads to one ConfigMap and tests with the real restricted deploy identity.

Recommended Action

1. Import and test the finalized plan-aware admission-readiness and rollout-lock protocol before merge.
2. Rerun this PR after the companion current-head admission suite is green.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[kkroo]

Superseded by #995, merged as f819d71.