From 086b7f9c982cbefb93dd0d3cf4434ecefba4f62c Mon Sep 17 00:00:00 2001
From: kkroo <kkroo@paperclip.ai>
Date: Sat, 1 Aug 2026 06:30:16 +0000
Subject: [PATCH 1/6] ci(deploy): authorize the release digest at admission
 time (BLO-19955)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Production deploys currently fail in `helm upgrade` whenever the built digest
is not already in the cluster-scoped ValidatingAdmissionPolicy's hardcoded
allowlist, which required a cluster-admin edit per release.

Add an approval step between artifact resolution and rollout. It writes the
exact digest just built into the ConfigMap the policy reads via paramRef
(Blockcast/onprem-k8s#1838), so the release authorizes itself.

The step deliberately uses a separate credential from the deploy kubeconfig.
KUBECONFIG_PAPERCLIP_CI_DEPLOY is namespace-scoped and cannot reach the approval
object at all — that is what keeps a direct namespace write from forging an
approval. The approver credential is bound to a Role over exactly one ConfigMap
name and holds no cluster-scoped permission.

Rotation is a bounded ring of 3, newest first, so an immediate rollback stays
available while rolling back further remains an explicit act. The window size
must match maxApprovedApiDigests in the policy: the policy denies every rollout
if the list is longer, so this is a ceiling rather than a preference.

The approval is read back rather than trusted from the patch exit code, so a
write that did not persist fails here instead of surfacing later as a confusing
admission denial during helm upgrade.

Requires KUBECONFIG_PAPERCLIP_RELEASE_APPROVER in the paperclip-production
environment; the step fails with an explicit message if it is absent.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 .github/workflows/docker.yml | 74 ++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
index ab5dbc66b84e..d9440ed01fac 100644
--- a/.github/workflows/docker.yml
+++ b/.github/workflows/docker.yml
@@ -366,6 +366,80 @@ jobs:
             echo "- Image: \`${image}@${digest}\`"
           } >> "$GITHUB_STEP_SUMMARY"
 
+      # Authorize this exact digest at admission time before rolling it out
+      # (BLO-19955). The cluster-scoped ValidatingAdmissionPolicy
+      # paperclip-core-service-routing reads its approved-digest list from
+      # ConfigMap/paperclip-api-approved-images via paramRef, so without this
+      # step helm upgrade is denied.
+      #
+      # This uses a SEPARATE credential from the deploy kubeconfig on purpose.
+      # KUBECONFIG_PAPERCLIP_CI_DEPLOY is namespace-scoped to the deploy
+      # namespace and cannot reach the approval object at all — that is what
+      # stops a direct namespace write from forging an approval. The approver
+      # credential is bound to a Role over exactly one ConfigMap name and holds
+      # no cluster-scoped permission.
+      - name: Approve deploy digest at admission time
+        env:
+          APPROVER_KUBECONFIG: ${{ secrets.KUBECONFIG_PAPERCLIP_RELEASE_APPROVER }}
+          DIGEST: ${{ steps.artifact.outputs.digest }}
+          APPROVAL_NS: paperclip-release-approvals
+          APPROVAL_CM: paperclip-api-approved-images
+          # Must match maxApprovedApiDigests in the admission policy. The policy
+          # denies every rollout if the window is longer, so this is a hard
+          # ceiling rather than a preference.
+          MAX_APPROVED: '3'
+        run: |
+          set -euo pipefail
+          [[ "${DIGEST}" =~ ^sha256:[0-9a-f]{64}$ ]]
+          if [ -z "${APPROVER_KUBECONFIG}" ]; then
+            echo "KUBECONFIG_PAPERCLIP_RELEASE_APPROVER is not set in the paperclip-production environment." >&2
+            echo "Admission will deny this rollout until the approver credential is provisioned." >&2
+            exit 1
+          fi
+
+          approver_kubeconfig="$RUNNER_TEMP/.kube/approver"
+          printf '%s' "${APPROVER_KUBECONFIG}" > "${approver_kubeconfig}"
+          chmod 600 "${approver_kubeconfig}"
+
+          current=$(KUBECONFIG="${approver_kubeconfig}" kubectl -n "${APPROVAL_NS}" \
+            get configmap "${APPROVAL_CM}" -o jsonpath='{.data.approvedDigests}')
+
+          # Bounded ring, newest first: this digest plus the most recently
+          # approved ones. Rolling back past the window stays an explicit act,
+          # so a stale image cannot be re-pinned silently.
+          window=$(
+            {
+              printf '%s\n' "${DIGEST}"
+              printf '%s\n' "${current}" | tr -d '\r' \
+                | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \
+                | grep -E '^sha256:[0-9a-f]{64}$' \
+                | grep -Fxv "${DIGEST}" || true
+            } | head -n "${MAX_APPROVED}"
+          )
+
+          patch=$(jq -n --arg v "${window}" '{data: {approvedDigests: $v}}')
+          KUBECONFIG="${approver_kubeconfig}" kubectl -n "${APPROVAL_NS}" \
+            patch configmap "${APPROVAL_CM}" --type merge -p "${patch}" >/dev/null
+
+          # Read back rather than trusting the patch exit code. A 200 that did
+          # not persist would send helm upgrade into an admission denial with a
+          # far more confusing error than this one.
+          verify=$(KUBECONFIG="${approver_kubeconfig}" kubectl -n "${APPROVAL_NS}" \
+            get configmap "${APPROVAL_CM}" -o jsonpath='{.data.approvedDigests}')
+          if ! printf '%s\n' "${verify}" | tr -d '\r' \
+            | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' | grep -Fxq "${DIGEST}"; then
+            echo "Approval did not persist: ${DIGEST} absent from ${APPROVAL_NS}/${APPROVAL_CM}" >&2
+            exit 1
+          fi
+          rm -f "${approver_kubeconfig}"
+
+          {
+            echo "### Admission approval"
+            echo "- Approved digest: \`${DIGEST}\`"
+            echo "- Approval window (newest first):"
+            printf '%s\n' "${window}" | sed 's/^/  - `/; s/$/`/'
+          } >> "$GITHUB_STEP_SUMMARY"
+
       # Pin the manifest digest resolved above so the image inspected for the
       # approved commit is exactly the image Kubernetes pulls. The committed
       # tag remains the readable source reference for non-CI deploys.

From 4f544520fd63d216f52bd8be8a7b068f389cc1b8 Mon Sep 17 00:00:00 2001
From: "allyblockcast[bot]" <allyblockcast[bot]@users.noreply.github.com>
Date: Sat, 1 Aug 2026 16:22:18 +0000
Subject: [PATCH 2/6] ci(deploy): single tested approval-ring implementation
 (BLO-19955)

Addresses Ally's review on #907.

- Arm the approver-kubeconfig EXIT trap before the credential reaches
  disk. It was removed only on the success path, so any kubectl/jq
  failure under `set -e` left a higher-privilege credential on a
  long-lived self-hosted runner. Also create it under umask 077 rather
  than chmod-ing after the fact.

- Deduplicate the approval ring. Only duplicates of the incoming digest
  were dropped, so a window that already contained a repeat spent a
  rollback slot on it: rotating `A,A,B` with `C` yielded `C,A,A` and
  silently evicted B.

- Stop reimplementing the rotation inside docker.yml. The inline copy
  had already drifted from the committed script in onprem-k8s -- it
  lacked the optimistic-concurrency guard, so two concurrent releases
  could clobber each other's approval and send helm upgrade into a
  confusing admission denial. The workflow now invokes
  scripts/approve-paperclip-api-digest.sh, which is the same file the
  companion real-apiserver suite takes via its APPROVE_SCRIPT override.

scripts/approve-paperclip-api-digest.test.mjs drives that script against
a stub kubectl and covers the duplicate and CRLF regressions, malformed
entries, the conflict-retry path, fail-closed on a missing ConfigMap,
and read-back failure. A structural case asserts docker.yml calls the
script instead of re-inlining the ring, and that the trap precedes the
credential write. Both guards were mutation-tested.

Refs BLO-19955, BLO-19834
---
 .github/workflows/docker.yml                  |  66 ++---
 .github/workflows/pr.yml                      |   3 +
 scripts/approve-paperclip-api-digest.sh       | 164 ++++++++++++
 scripts/approve-paperclip-api-digest.test.mjs | 246 ++++++++++++++++++
 4 files changed, 438 insertions(+), 41 deletions(-)
 create mode 100755 scripts/approve-paperclip-api-digest.sh
 create mode 100644 scripts/approve-paperclip-api-digest.test.mjs

diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
index d9440ed01fac..5c204ef7b74d 100644
--- a/.github/workflows/docker.yml
+++ b/.github/workflows/docker.yml
@@ -367,11 +367,19 @@ jobs:
           } >> "$GITHUB_STEP_SUMMARY"
 
       # Authorize this exact digest at admission time before rolling it out
-      # (BLO-19955). The cluster-scoped ValidatingAdmissionPolicy
-      # paperclip-core-service-routing reads its approved-digest list from
+      # (BLO-19955). The ValidatingAdmissionPolicy paperclip-api-image-approval
+      # reads its approved-digest list from
       # ConfigMap/paperclip-api-approved-images via paramRef, so without this
       # step helm upgrade is denied.
       #
+      # The rotation itself lives in scripts/approve-paperclip-api-digest.sh and
+      # is deliberately NOT reimplemented here. An inline copy drifted from the
+      # committed script once already (it lacked the optimistic-concurrency
+      # guard, so two concurrent releases could silently clobber each other's
+      # approval), and only the script is covered by
+      # scripts/approve-paperclip-api-digest.test.mjs and by the real-apiserver
+      # suite in Blockcast/onprem-k8s. One implementation, one set of tests.
+      #
       # This uses a SEPARATE credential from the deploy kubeconfig on purpose.
       # KUBECONFIG_PAPERCLIP_CI_DEPLOY is namespace-scoped to the deploy
       # namespace and cannot reach the approval object at all — that is what
@@ -382,62 +390,38 @@ jobs:
         env:
           APPROVER_KUBECONFIG: ${{ secrets.KUBECONFIG_PAPERCLIP_RELEASE_APPROVER }}
           DIGEST: ${{ steps.artifact.outputs.digest }}
-          APPROVAL_NS: paperclip-release-approvals
-          APPROVAL_CM: paperclip-api-approved-images
           # Must match maxApprovedApiDigests in the admission policy. The policy
           # denies every rollout if the window is longer, so this is a hard
           # ceiling rather than a preference.
-          MAX_APPROVED: '3'
+          PAPERCLIP_MAX_APPROVED_DIGESTS: '3'
         run: |
           set -euo pipefail
-          [[ "${DIGEST}" =~ ^sha256:[0-9a-f]{64}$ ]]
           if [ -z "${APPROVER_KUBECONFIG}" ]; then
             echo "KUBECONFIG_PAPERCLIP_RELEASE_APPROVER is not set in the paperclip-production environment." >&2
             echo "Admission will deny this rollout until the approver credential is provisioned." >&2
             exit 1
           fi
 
+          mkdir -p "$RUNNER_TEMP/.kube"
           approver_kubeconfig="$RUNNER_TEMP/.kube/approver"
-          printf '%s' "${APPROVER_KUBECONFIG}" > "${approver_kubeconfig}"
-          chmod 600 "${approver_kubeconfig}"
-
-          current=$(KUBECONFIG="${approver_kubeconfig}" kubectl -n "${APPROVAL_NS}" \
-            get configmap "${APPROVAL_CM}" -o jsonpath='{.data.approvedDigests}')
+          approval_log="$RUNNER_TEMP/approval.log"
 
-          # Bounded ring, newest first: this digest plus the most recently
-          # approved ones. Rolling back past the window stays an explicit act,
-          # so a stale image cannot be re-pinned silently.
-          window=$(
-            {
-              printf '%s\n' "${DIGEST}"
-              printf '%s\n' "${current}" | tr -d '\r' \
-                | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \
-                | grep -E '^sha256:[0-9a-f]{64}$' \
-                | grep -Fxv "${DIGEST}" || true
-            } | head -n "${MAX_APPROVED}"
-          )
+          # Arm the cleanup BEFORE the credential reaches disk. This is the
+          # higher-privilege identity of the two the job handles, and the runner
+          # is a long-lived self-hosted host, so a kubectl/jq failure exiting
+          # under `set -e` must not leave it behind. umask instead of a
+          # follow-up chmod so the file is never briefly world-readable.
+          trap 'rm -f "${approver_kubeconfig}" "${approval_log}"' EXIT
+          (umask 077; printf '%s' "${APPROVER_KUBECONFIG}" > "${approver_kubeconfig}")
 
-          patch=$(jq -n --arg v "${window}" '{data: {approvedDigests: $v}}')
-          KUBECONFIG="${approver_kubeconfig}" kubectl -n "${APPROVAL_NS}" \
-            patch configmap "${APPROVAL_CM}" --type merge -p "${patch}" >/dev/null
-
-          # Read back rather than trusting the patch exit code. A 200 that did
-          # not persist would send helm upgrade into an admission denial with a
-          # far more confusing error than this one.
-          verify=$(KUBECONFIG="${approver_kubeconfig}" kubectl -n "${APPROVAL_NS}" \
-            get configmap "${APPROVAL_CM}" -o jsonpath='{.data.approvedDigests}')
-          if ! printf '%s\n' "${verify}" | tr -d '\r' \
-            | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' | grep -Fxq "${DIGEST}"; then
-            echo "Approval did not persist: ${DIGEST} absent from ${APPROVAL_NS}/${APPROVAL_CM}" >&2
-            exit 1
-          fi
-          rm -f "${approver_kubeconfig}"
+          KUBECONFIG="${approver_kubeconfig}" \
+            ./scripts/approve-paperclip-api-digest.sh "${DIGEST}" | tee "${approval_log}"
 
           {
             echo "### Admission approval"
-            echo "- Approved digest: \`${DIGEST}\`"
-            echo "- Approval window (newest first):"
-            printf '%s\n' "${window}" | sed 's/^/  - `/; s/$/`/'
+            echo '```'
+            cat "${approval_log}"
+            echo '```'
           } >> "$GITHUB_STEP_SUMMARY"
 
       # Pin the manifest digest resolved above so the image inspected for the
diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml
index 430682db5b9c..53720d606dc9 100644
--- a/.github/workflows/pr.yml
+++ b/.github/workflows/pr.yml
@@ -74,6 +74,9 @@ jobs:
       - name: Validate Docker deploy timeout margin
         run: node --test ./scripts/check-docker-deploy-timeout.test.js
 
+      - name: Test admission-approval digest rotation
+        run: node --test ./scripts/approve-paperclip-api-digest.test.mjs
+
       - name: Reject git push in adapter/runtime code
         run: node ./scripts/check-no-git-push.mjs
 
diff --git a/scripts/approve-paperclip-api-digest.sh b/scripts/approve-paperclip-api-digest.sh
new file mode 100755
index 000000000000..7b096ffd9d77
--- /dev/null
+++ b/scripts/approve-paperclip-api-digest.sh
@@ -0,0 +1,164 @@
+#!/usr/bin/env bash
+# Authorize one immutable Paperclip API image digest at admission time (BLO-19955).
+#
+# Rotates the bounded approval window in
+# ConfigMap/paperclip-api-approved-images (namespace paperclip-release-approvals),
+# which ValidatingAdmissionPolicy/paperclip-api-image-approval consumes via
+# paramRef. Run this with the paperclip-release-approver credential — NOT the
+# Release Engineer's namespace-scoped deploy credential, which by design cannot
+# reach this object.
+#
+# Usage:
+#   scripts/approve-paperclip-api-digest.sh sha256:<64 hex>
+#
+# This is the single implementation of the rotation. The production release
+# workflow (.github/workflows/docker.yml, "Approve deploy digest at admission
+# time") invokes this file rather than reimplementing the ring inline, so the
+# code path exercised by scripts/approve-paperclip-api-digest.test.mjs is the
+# code path that ships. The companion admission suite in Blockcast/onprem-k8s
+# takes the same file via its APPROVE_SCRIPT override.
+#
+# The window is a ring of at most MAX_APPROVED_DIGESTS entries, newest first:
+# the digest being released plus the most recently approved ones. That keeps an
+# immediate rollback available without accepting every historical digest. Rolling
+# back past the window is deliberately an explicit act — re-run this script
+# naming that digest.
+set -euo pipefail
+
+NAMESPACE="${PAPERCLIP_APPROVAL_NAMESPACE:-paperclip-release-approvals}"
+CONFIGMAP="${PAPERCLIP_APPROVAL_CONFIGMAP:-paperclip-api-approved-images}"
+DATA_KEY="approvedDigests"
+# Must stay in lockstep with the `maxApprovedApiDigests` CEL variable in
+# onprem-k8s paperclip/paperclip-public-tools.yaml. The policy denies everything
+# if the list is longer, so a drift here is a hard outage, not a silent widening.
+MAX_APPROVED_DIGESTS="${PAPERCLIP_MAX_APPROVED_DIGESTS:-3}"
+
+usage() {
+  echo "usage: $0 sha256:<64 lowercase hex>" >&2
+  exit 2
+}
+
+[[ $# -eq 1 ]] || usage
+DIGEST="$1"
+
+if [[ ! "$DIGEST" =~ ^sha256:[0-9a-f]{64}$ ]]; then
+  echo "refusing to approve '${DIGEST}': not a well-formed lowercase sha256 digest" >&2
+  echo "pass the bare digest only — the repository is fixed inside the admission policy" >&2
+  exit 2
+fi
+
+for dep in kubectl jq; do
+  command -v "$dep" >/dev/null 2>&1 || { echo "$dep is required" >&2; exit 2; }
+done
+
+# Rotation is a read-modify-write, so the write MUST be guarded by the version
+# that was read. With an unconditional merge-patch, two releases approving
+# concurrently silently clobber each other: A and B both read [x], A writes
+# [a,x], B writes [b,x], and `a` is gone. Worse, A's own read-back can land in
+# the window between the two writes and observe `a` present — so A proceeds to a
+# `helm upgrade` whose digest is no longer approved and dies on a confusing
+# admission denial. Carrying the observed resourceVersion into the write makes
+# the apiserver reject a stale write with 409 instead, and we retry from a fresh
+# read.
+#
+# `kubectl replace` maps to the `update` verb, which the approver Role grants on
+# exactly this one ConfigMap name. It cannot create the object (no `create`), so
+# the fail-closed behaviour below is preserved.
+MAX_ROTATE_ATTEMPTS="${PAPERCLIP_APPROVAL_ROTATE_ATTEMPTS:-5}"
+replace_err="$(mktemp "${TMPDIR:-/tmp}/paperclip-approve-err.XXXXXX")"
+trap 'rm -f "$replace_err"' EXIT
+
+rotated=""
+for attempt in $(seq 1 "$MAX_ROTATE_ATTEMPTS"); do
+  # `get` is the only read verb the approver holds, and it is scoped to this one
+  # name. A failure here means the credential is wrong or the ConfigMap was never
+  # installed by the cluster-admin bootstrap — both are fail-closed, so surface
+  # them rather than trying to create the object.
+  if ! current_json=$(kubectl -n "$NAMESPACE" get configmap "$CONFIGMAP" -o json 2>/dev/null); then
+    echo "cannot read ${NAMESPACE}/${CONFIGMAP}." >&2
+    echo "The approval ConfigMap is installed by the cluster-admin bootstrap" >&2
+    echo "(paperclip/paperclip-release-approvals.yaml); this script never creates it." >&2
+    exit 1
+  fi
+
+  current_raw="$(jq -r --arg key "$DATA_KEY" '.data[$key] // ""' <<<"$current_json")"
+
+  # Keep only well-formed digests, drop the one being approved wherever it
+  # already sits, then collapse any remaining repeats before prepending it.
+  #
+  # The `awk '!seen[$0]++'` is load-bearing and not merely tidy: without it only
+  # duplicates OF THE NEW DIGEST are removed, so a window that already contained
+  # a repeat spends a rollback slot on it. Rotating `A,A,B` with `C` would yield
+  # `C,A,A` and silently evict B — three entries, two distinct, one usable
+  # rollback target instead of two. Anything malformed is discarded here rather
+  # than carried forward: the policy would ignore it anyway, and leaving it in
+  # place would consume a slot the same way.
+  mapfile -t existing < <(
+    printf '%s\n' "$current_raw" \
+      | tr -d '\r' \
+      | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \
+      | grep -E '^sha256:[0-9a-f]{64}$' \
+      | grep -Fxv "$DIGEST" \
+      | awk '!seen[$0]++' \
+      || true
+  )
+
+  approved=("$DIGEST")
+  for entry in "${existing[@]:-}"; do
+    [[ -n "$entry" ]] || continue
+    (( ${#approved[@]} < MAX_APPROVED_DIGESTS )) || break
+    approved+=("$entry")
+  done
+
+  payload=$(printf '%s\n' "${approved[@]}")
+
+  # resourceVersion rides along inside current_json, so this write is rejected if
+  # anyone else rotated the ring since the read above.
+  if jq --arg key "$DATA_KEY" --arg value "$payload" '.data[$key] = $value' <<<"$current_json" \
+      | kubectl replace -f - >/dev/null 2>"$replace_err"; then
+    rotated=yes
+    break
+  fi
+
+  if ! grep -qiE 'conflict|modified|latest version' "$replace_err"; then
+    echo "failed to rotate ${NAMESPACE}/${CONFIGMAP}:" >&2
+    cat "$replace_err" >&2
+    exit 1
+  fi
+  echo "approval ring changed underneath us; retrying (${attempt}/${MAX_ROTATE_ATTEMPTS})" >&2
+  sleep $(( attempt ))
+done
+
+if [[ -z "$rotated" ]]; then
+  echo "could not rotate ${NAMESPACE}/${CONFIGMAP} after ${MAX_ROTATE_ATTEMPTS} attempts;" >&2
+  echo "another release is approving concurrently — serialize the release path" >&2
+  exit 1
+fi
+
+echo "Approving ${DIGEST} for harbor.blockcast.net/paperclip/paperclip"
+echo "Approval window (newest first, max ${MAX_APPROVED_DIGESTS}):"
+printf '  - %s\n' "${approved[@]}"
+
+# Read back rather than trusting the write's exit code. A 200 that did not
+# persist the digest would let `helm upgrade` run straight into an admission
+# denial with a confusing error, so verify the approval actually landed.
+verify_raw=$(kubectl -n "$NAMESPACE" get configmap "$CONFIGMAP" \
+  -o jsonpath="{.data.${DATA_KEY}}")
+verify_count=$(printf '%s\n' "$verify_raw" | tr -d '\r' \
+  | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \
+  | grep -Ec '^sha256:[0-9a-f]{64}$' || true)
+
+if ! printf '%s\n' "$verify_raw" | tr -d '\r' \
+  | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \
+  | grep -Fxq "$DIGEST"; then
+  echo "approval did not persist: ${DIGEST} is absent from ${NAMESPACE}/${CONFIGMAP}" >&2
+  exit 1
+fi
+
+if (( verify_count > MAX_APPROVED_DIGESTS )); then
+  echo "approval window is ${verify_count} entries, over the ${MAX_APPROVED_DIGESTS} the policy accepts;" >&2
+  echo "the admission policy will now deny every rollout until this is trimmed" >&2
+  exit 1
+fi
+
+echo "Approved. ${verify_count} digest(s) in the window."
diff --git a/scripts/approve-paperclip-api-digest.test.mjs b/scripts/approve-paperclip-api-digest.test.mjs
new file mode 100644
index 000000000000..09960d87a3b6
--- /dev/null
+++ b/scripts/approve-paperclip-api-digest.test.mjs
@@ -0,0 +1,246 @@
+// Behavioural tests for scripts/approve-paperclip-api-digest.sh — the exact
+// script the production release workflow runs (BLO-19955).
+//
+// The rotation used to be inlined into .github/workflows/docker.yml, which meant
+// the shipping code path had no test at all: the only coverage lived in
+// Blockcast/onprem-k8s against a copy that had already drifted from it. These
+// tests drive the real file with a stub `kubectl` so the ring semantics are
+// pinned here, in the repo that ships them, without needing a cluster.
+//
+// `jq` is a hard dependency of the script itself, so the behavioural cases are
+// skipped (visibly, with a reason) on a runner that lacks it. The structural
+// case at the bottom always runs — it is what stops the rotation from being
+// re-inlined into the workflow and drifting again.
+
+import assert from "node:assert/strict";
+import { execFileSync, spawnSync } from "node:child_process";
+import { chmodSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { test } from "node:test";
+
+const SCRIPT = new URL("./approve-paperclip-api-digest.sh", import.meta.url).pathname;
+const WORKFLOW = new URL("../.github/workflows/docker.yml", import.meta.url).pathname;
+
+function have(bin) {
+  try {
+    execFileSync("sh", ["-c", `command -v ${bin}`], { stdio: "ignore" });
+    return true;
+  } catch {
+    return false;
+  }
+}
+
+const missing = ["jq", "bash"].filter((b) => !have(b));
+const skip = missing.length ? `requires ${missing.join(" + ")} on PATH` : false;
+
+/** A 64-lowercase-hex digest, distinct per `n`. */
+const digest = (n) => `sha256:${n.toString(16).padStart(64, "0")}`;
+const [A, B, C, D] = [1, 2, 3, 4].map(digest);
+
+// A stub `kubectl` covering exactly the three invocations the script makes:
+//   get configmap -o json        -> the stored object
+//   replace -f -                 -> optimistic-concurrency write
+//   get configmap -o jsonpath=.. -> the read-back
+// State lives in a JSON file so it survives across the script's retry loop.
+const KUBECTL_STUB = `#!/usr/bin/env node
+import { readFileSync, writeFileSync } from "node:fs";
+const statePath = process.env.STUB_STATE;
+const state = JSON.parse(readFileSync(statePath, "utf8"));
+const argv = process.argv.slice(2);
+
+if (state.absent) {
+  process.stderr.write("Error from server (NotFound): configmaps not found\\n");
+  process.exit(1);
+}
+
+if (argv.includes("replace")) {
+  const body = JSON.parse(readFileSync(0, "utf8"));
+  // Burn a scripted conflict before accepting the write, to exercise retry.
+  if (state.conflictsRemaining > 0) {
+    state.conflictsRemaining -= 1;
+    state.resourceVersion = String(Number(state.resourceVersion) + 1);
+    writeFileSync(statePath, JSON.stringify(state));
+    process.stderr.write(
+      "Error from server (Conflict): the object has been modified; please apply your changes to the latest version\\n",
+    );
+    process.exit(1);
+  }
+  if (body.metadata.resourceVersion !== state.resourceVersion) {
+    process.stderr.write("Error from server (Conflict): the object has been modified\\n");
+    process.exit(1);
+  }
+  state.data = body.data;
+  state.resourceVersion = String(Number(state.resourceVersion) + 1);
+  // Simulate a write the apiserver accepted but that did not land as sent.
+  if (state.tamperOnWrite !== undefined) state.data.approvedDigests = state.tamperOnWrite;
+  writeFileSync(statePath, JSON.stringify(state));
+  process.exit(0);
+}
+
+const jsonpath = argv.find((a) => a.startsWith("jsonpath="));
+if (jsonpath) {
+  process.stdout.write(state.data.approvedDigests ?? "");
+  process.exit(0);
+}
+
+process.stdout.write(
+  JSON.stringify({
+    apiVersion: "v1",
+    kind: "ConfigMap",
+    metadata: {
+      name: "paperclip-api-approved-images",
+      namespace: "paperclip-release-approvals",
+      resourceVersion: state.resourceVersion,
+    },
+    data: state.data,
+  }),
+);
+`;
+
+/**
+ * Run the real script against a stub cluster.
+ * @returns {{status:number, stdout:string, stderr:string, window:string[]}}
+ */
+function approve(digestArg, { current, absent = false, conflicts = 0, tamperOnWrite } = {}) {
+  const dir = mkdtempSync(join(tmpdir(), "paperclip-approve-"));
+  const statePath = join(dir, "state.json");
+  const state = {
+    resourceVersion: "1",
+    data: current === undefined ? {} : { approvedDigests: current },
+    absent,
+    conflictsRemaining: conflicts,
+  };
+  if (tamperOnWrite !== undefined) state.tamperOnWrite = tamperOnWrite;
+  writeFileSync(statePath, JSON.stringify(state));
+
+  const stub = join(dir, "kubectl");
+  writeFileSync(stub, KUBECTL_STUB);
+  chmodSync(stub, 0o755);
+
+  const run = spawnSync("bash", [SCRIPT, digestArg], {
+    encoding: "utf8",
+    env: {
+      ...process.env,
+      PATH: `${dir}:${process.env.PATH}`,
+      STUB_STATE: statePath,
+      PAPERCLIP_APPROVAL_ROTATE_ATTEMPTS: "3",
+    },
+    stdio: ["ignore", "pipe", "pipe"],
+  });
+  const { status, stdout, stderr } = { status: run.status, stdout: run.stdout, stderr: run.stderr };
+
+  const persisted = JSON.parse(readFileSync(statePath, "utf8")).data.approvedDigests ?? "";
+  const window = persisted.split("\n").filter(Boolean);
+  return { status, stdout, stderr, window };
+}
+
+test("prepends the new digest and evicts the oldest at the 3-entry bound", { skip }, () => {
+  const { status, window } = approve(D, { current: [A, B, C].join("\n") });
+  assert.equal(status, 0);
+  assert.deepEqual(window, [D, A, B]);
+});
+
+// The regression this file was added for. Rotating `A,A,B` used to yield
+// `C,A,A` — three entries, two distinct — silently dropping B and leaving one
+// usable rollback target where the window promises two.
+test("collapses pre-existing duplicates instead of spending rollback slots", { skip }, () => {
+  const { status, window } = approve(C, { current: [A, A, B].join("\n") });
+  assert.equal(status, 0);
+  assert.deepEqual(window, [C, A, B]);
+  assert.equal(new Set(window).size, window.length, "window must hold distinct digests");
+});
+
+test("re-approving a digest already in the window moves it to the front", { skip }, () => {
+  const { status, window } = approve(B, { current: [A, B, C].join("\n") });
+  assert.equal(status, 0);
+  assert.deepEqual(window, [B, A, C]);
+});
+
+test("tolerates CRLF and surrounding whitespace in the stored window", { skip }, () => {
+  const { status, window } = approve(C, { current: `  ${A}  \r\n${B}\r\n` });
+  assert.equal(status, 0);
+  assert.deepEqual(window, [C, A, B]);
+});
+
+test("discards malformed and mixed-case entries rather than carrying them", { skip }, () => {
+  const current = ["not-a-digest", A, "", `SHA256:${"A".repeat(64)}`, `sha256:${"z".repeat(64)}`].join("\n");
+  const { status, window } = approve(C, { current });
+  assert.equal(status, 0);
+  assert.deepEqual(window, [C, A]);
+});
+
+test("seeds an empty window", { skip }, () => {
+  const { status, window } = approve(A, { current: "" });
+  assert.equal(status, 0);
+  assert.deepEqual(window, [A]);
+});
+
+test("refuses a digest that is not lowercase sha256", { skip }, () => {
+  for (const bad of [`sha256:${"A".repeat(64)}`, "sha256:abc", "harbor.example/x@sha256:" + "a".repeat(64), ""]) {
+    const { status, stderr } = approve(bad, { current: "" });
+    assert.equal(status, 2, `expected refusal for ${JSON.stringify(bad)}`);
+    assert.match(stderr, /well-formed lowercase sha256|usage:/);
+  }
+});
+
+test("fails closed when the approval ConfigMap is absent, without creating it", { skip }, () => {
+  const { status, stderr } = approve(A, { current: "", absent: true });
+  assert.equal(status, 1);
+  assert.match(stderr, /cannot read paperclip-release-approvals\/paperclip-api-approved-images/);
+  assert.match(stderr, /never creates it/);
+});
+
+test("retries a concurrent rotation instead of clobbering it", { skip }, () => {
+  const { status, window, stderr } = approve(C, { current: [A, B].join("\n"), conflicts: 1 });
+  assert.equal(status, 0);
+  assert.match(stderr, /changed underneath us; retrying/);
+  assert.deepEqual(window, [C, A, B]);
+});
+
+test("fails when the write is accepted but the digest did not persist", { skip }, () => {
+  const { status, stderr } = approve(C, { current: [A, B].join("\n"), tamperOnWrite: [A, B].join("\n") });
+  assert.equal(status, 1);
+  assert.match(stderr, /approval did not persist/);
+});
+
+test("fails when the persisted window is wider than the policy accepts", { skip }, () => {
+  const { status, stderr } = approve(C, {
+    current: [A, B].join("\n"),
+    tamperOnWrite: [C, A, B, D].join("\n"),
+  });
+  assert.equal(status, 1);
+  assert.match(stderr, /over the 3 the policy accepts/);
+});
+
+// Always runs: no external tooling, and it is the guard that keeps the workflow
+// and the tested script from diverging again.
+test("the deploy workflow calls the script rather than re-inlining the rotation", () => {
+  const workflow = readFileSync(WORKFLOW, "utf8");
+  const marker = "\n      - name: Approve deploy digest at admission time\n";
+  const start = workflow.indexOf(marker);
+  assert.notEqual(start, -1, "docker.yml must keep the admission-approval step");
+  const rest = workflow.slice(start + marker.length);
+  const next = rest.search(/^ {6}- name: /m);
+  const step = next === -1 ? rest : rest.slice(0, next);
+
+  assert.match(
+    step,
+    /\.\/scripts\/approve-paperclip-api-digest\.sh "\$\{DIGEST\}"/,
+    "the step must invoke the committed script",
+  );
+  assert.doesNotMatch(
+    step,
+    /kubectl .*(patch|replace) configmap|head -n "\$\{MAX_APPROVED\}"/,
+    "the step must not reimplement the rotation inline",
+  );
+
+  // The approver is the higher-privilege of the two credentials this job
+  // handles; on a self-hosted runner a mid-step failure must not leave it on
+  // disk, so the trap has to be armed before the secret is written.
+  const trapAt = step.indexOf("trap 'rm -f");
+  const writeAt = step.indexOf('printf \'%s\' "${APPROVER_KUBECONFIG}"');
+  assert.notEqual(trapAt, -1, "the step must install an EXIT trap for the approver kubeconfig");
+  assert.notEqual(writeAt, -1, "the step must write the approver kubeconfig");
+  assert.ok(trapAt < writeAt, "the EXIT trap must be armed before the credential reaches disk");
+});

From 4f7025dbef610168f5d67dde539868431ff87360 Mon Sep 17 00:00:00 2001
From: kkroo <kkroo@paperclip.ai>
Date: Sat, 1 Aug 2026 17:25:46 +0000
Subject: [PATCH 3/6] ci(deploy): run approval tooling from the trusted
 revision, on a private path (BLO-19955)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Ally review on bfb8417a. Two findings against the approval step, both real —
each reproduced against the step body extracted from docker.yml.

1. The approver credential executed a script from the DEPLOY checkout.

   That checkout is `target_sha`: operator-supplied, and for a rollback an
   arbitrary historical revision. So the release-approver credential ran
   whatever that commit happened to contain — nothing at all for a rollback to
   before the script existed (`No such file or directory`), or a
   later-reverted implementation running against the live approval object.

   Check the tooling out separately at github.workflow_sha, the revision the
   executing workflow file itself came from, so the script and the step
   invoking it are always the same revision and neither is chosen by the
   deploy requester. Assert the resolved SHA rather than assuming it, matching
   "Verify target commit". The deploy checkout stays exactly as it was: the
   Helm chart and application artifact must still come from the commit being
   rolled out.

2. The credential was written to the predictable $RUNNER_TEMP/.kube/approver
   with shell redirection, which follows a symlink already sitting there.

   Reproduced against the old code: 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. Now it goes in a mktemp -d directory, which cannot open an existing
   entry. Same scenario against the new step body: not hijacked, 0600 file in
   a 0700 dir, and the trap leaves nothing behind on either the success or the
   failure path.

3. Missing `jq` skipped every behavioural case while the policy job still went
   green, 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
   failure; local runs still skip with a reason.

Structural coverage extended to all three, and mutation-checked — reverting
each fix individually turns the corresponding assertion red:

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

14/14 pass with the fixes in place.

Refs BLO-19955, BLO-19834
---
 .github/workflows/docker.yml                  | 75 +++++++++++++---
 scripts/approve-paperclip-api-digest.test.mjs | 87 +++++++++++++++++--
 2 files changed, 144 insertions(+), 18 deletions(-)

diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
index b780b79960c6..fcb98a675b48 100644
--- a/.github/workflows/docker.yml
+++ b/.github/workflows/docker.yml
@@ -389,10 +389,61 @@ jobs:
       # stops a direct namespace write from forging an approval. The approver
       # credential is bound to a Role over exactly one ConfigMap name and holds
       # no cluster-scoped permission.
+      # The release tooling runs from the TRUSTED workflow revision, not from the
+      # checkout above. That checkout is `target_sha` — an operator-supplied
+      # commit, which for a rollback is an arbitrary historical revision of this
+      # repo. Executing a script out of it with the release-approver credential
+      # would mean the credential runs whatever that commit happened to contain:
+      # a rollback to before this script existed dies on `No such file or
+      # directory`, and a rollback onto a commit carrying a later-reverted
+      # version silently runs that old code against the live approval object.
+      #
+      # github.workflow_sha is the commit the currently-executing workflow file
+      # came from, so the script and the workflow step invoking it are always the
+      # same revision and neither is chosen by the deploy requester. The target
+      # checkout stays in place: the Helm chart and application artifact must
+      # still come from the historical commit being rolled out.
+      - name: Checkout release tooling at trusted revision
+        uses: actions/checkout@v5
+        with:
+          ref: ${{ github.workflow_sha }}
+          path: .release-tooling
+          fetch-depth: 1
+          sparse-checkout: scripts/approve-paperclip-api-digest.sh
+          sparse-checkout-cone-mode: false
+
+      - name: Verify release tooling revision
+        id: tooling
+        env:
+          WORKFLOW_SHA: ${{ github.workflow_sha }}
+        run: |
+          set -euo pipefail
+          # Assert rather than assume, matching "Verify target commit" above: if
+          # the context is ever empty or the checkout silently resolves
+          # elsewhere, fail here instead of approving a digest with tooling of
+          # unknown provenance.
+          expected=$(printf '%s' "${WORKFLOW_SHA}" | tr '[:upper:]' '[:lower:]')
+          if [[ ! "${expected}" =~ ^[0-9a-f]{40}$ ]]; then
+            echo "github.workflow_sha is not a full commit SHA: '${WORKFLOW_SHA}'" >&2
+            exit 1
+          fi
+          actual=$(git -C .release-tooling rev-parse HEAD | tr '[:upper:]' '[:lower:]')
+          if [ "${actual}" != "${expected}" ]; then
+            echo "Release tooling resolved to ${actual}, expected ${expected}" >&2
+            exit 1
+          fi
+          script="$(pwd)/.release-tooling/scripts/approve-paperclip-api-digest.sh"
+          if [ ! -f "${script}" ]; then
+            echo "Approval script missing from trusted revision ${expected}" >&2
+            exit 1
+          fi
+          echo "approve_script=${script}" >> "$GITHUB_OUTPUT"
+
       - name: Approve deploy digest at admission time
         env:
           APPROVER_KUBECONFIG: ${{ secrets.KUBECONFIG_PAPERCLIP_RELEASE_APPROVER }}
           DIGEST: ${{ steps.artifact.outputs.digest }}
+          APPROVE_SCRIPT: ${{ steps.tooling.outputs.approve_script }}
           # Must match maxApprovedApiDigests in the admission policy. The policy
           # denies every rollout if the window is longer, so this is a hard
           # ceiling rather than a preference.
@@ -405,20 +456,24 @@ jobs:
             exit 1
           fi
 
-          mkdir -p "$RUNNER_TEMP/.kube"
-          approver_kubeconfig="$RUNNER_TEMP/.kube/approver"
-          approval_log="$RUNNER_TEMP/approval.log"
+          # mktemp -d, not a fixed path: `> "$RUNNER_TEMP/.kube/approver"`
+          # follows a pre-existing symlink at that name, and arc-deploy is a
+          # self-hosted pool, so residue from an earlier workload could redirect
+          # this write — the higher-privilege credential of the two this job
+          # handles — somewhere it survives. mktemp creates a fresh 0700
+          # directory or fails; it never opens something that already exists.
+          approver_dir="$(mktemp -d "${RUNNER_TEMP}/approver.XXXXXXXX")"
+          approver_kubeconfig="${approver_dir}/kubeconfig"
+          approval_log="${approver_dir}/approval.log"
 
-          # Arm the cleanup BEFORE the credential reaches disk. This is the
-          # higher-privilege identity of the two the job handles, and the runner
-          # is a long-lived self-hosted host, so a kubectl/jq failure exiting
-          # under `set -e` must not leave it behind. umask instead of a
-          # follow-up chmod so the file is never briefly world-readable.
-          trap 'rm -f "${approver_kubeconfig}" "${approval_log}"' EXIT
+          # Arm the cleanup BEFORE the credential reaches disk, so a kubectl/jq
+          # failure exiting under `set -e` cannot leave it behind. umask instead
+          # of a follow-up chmod so the file is never briefly world-readable.
+          trap 'rm -rf "${approver_dir}"' EXIT
           (umask 077; printf '%s' "${APPROVER_KUBECONFIG}" > "${approver_kubeconfig}")
 
           KUBECONFIG="${approver_kubeconfig}" \
-            ./scripts/approve-paperclip-api-digest.sh "${DIGEST}" | tee "${approval_log}"
+            "${APPROVE_SCRIPT}" "${DIGEST}" | tee "${approval_log}"
 
           {
             echo "### Admission approval"
diff --git a/scripts/approve-paperclip-api-digest.test.mjs b/scripts/approve-paperclip-api-digest.test.mjs
index 09960d87a3b6..a8d3c42bd030 100644
--- a/scripts/approve-paperclip-api-digest.test.mjs
+++ b/scripts/approve-paperclip-api-digest.test.mjs
@@ -7,10 +7,13 @@
 // tests drive the real file with a stub `kubectl` so the ring semantics are
 // pinned here, in the repo that ships them, without needing a cluster.
 //
-// `jq` is a hard dependency of the script itself, so the behavioural cases are
-// skipped (visibly, with a reason) on a runner that lacks it. The structural
-// case at the bottom always runs — it is what stops the rotation from being
-// re-inlined into the workflow and drifting again.
+// `jq` is a hard dependency of the script itself. Locally the behavioural cases
+// skip (visibly, with a reason) on a machine that lacks it; in CI they must NOT,
+// because a skip and a pass are the same green tick — losing jq from the runner
+// image would silently reduce this file to its one structural case while the
+// policy job still reported success. Under CI a missing dependency is a failure.
+// The structural cases at the bottom always run: they are what stops the
+// rotation from being re-inlined into the workflow and drifting again.
 
 import assert from "node:assert/strict";
 import { execFileSync, spawnSync } from "node:child_process";
@@ -31,9 +34,22 @@ function have(bin) {
   }
 }
 
-const missing = ["jq", "bash"].filter((b) => !have(b));
+const REQUIRED_BINS = ["jq", "bash"];
+const missing = REQUIRED_BINS.filter((b) => !have(b));
 const skip = missing.length ? `requires ${missing.join(" + ")} on PATH` : false;
 
+// Fail closed on CI rather than skipping into a green run.
+test("behavioural prerequisites are present on CI", () => {
+  if (!process.env.CI) return;
+  assert.deepEqual(
+    missing,
+    [],
+    `CI runner is missing ${missing.join(", ")}; the behavioural cases below would ` +
+      "silently skip and this job would still pass. Install the dependency in the " +
+      "policy job (or drop it from the script) rather than letting coverage vanish.",
+  );
+});
+
 /** A 64-lowercase-hex digest, distinct per `n`. */
 const digest = (n) => `sha256:${n.toString(16).padStart(64, "0")}`;
 const [A, B, C, D] = [1, 2, 3, 4].map(digest);
@@ -226,7 +242,7 @@ test("the deploy workflow calls the script rather than re-inlining the rotation"
 
   assert.match(
     step,
-    /\.\/scripts\/approve-paperclip-api-digest\.sh "\$\{DIGEST\}"/,
+    /"\$\{APPROVE_SCRIPT\}" "\$\{DIGEST\}"/,
     "the step must invoke the committed script",
   );
   assert.doesNotMatch(
@@ -238,9 +254,64 @@ test("the deploy workflow calls the script rather than re-inlining the rotation"
   // The approver is the higher-privilege of the two credentials this job
   // handles; on a self-hosted runner a mid-step failure must not leave it on
   // disk, so the trap has to be armed before the secret is written.
-  const trapAt = step.indexOf("trap 'rm -f");
+  const trapAt = step.indexOf("trap 'rm -rf");
   const writeAt = step.indexOf('printf \'%s\' "${APPROVER_KUBECONFIG}"');
-  assert.notEqual(trapAt, -1, "the step must install an EXIT trap for the approver kubeconfig");
+  assert.notEqual(trapAt, -1, "the step must install an EXIT trap for the approver credential");
   assert.notEqual(writeAt, -1, "the step must write the approver kubeconfig");
   assert.ok(trapAt < writeAt, "the EXIT trap must be armed before the credential reaches disk");
+
+  // A fixed path is opened through any symlink already sitting at that name.
+  // arc-deploy is a self-hosted pool, so residue from an earlier workload could
+  // redirect the credential write; mktemp -d cannot open an existing entry.
+  assert.match(
+    step,
+    /mktemp -d "\$\{RUNNER_TEMP\}\/approver\.X{6,}"/,
+    "the approver credential must live under a mktemp -d directory",
+  );
+  assert.doesNotMatch(
+    step,
+    /approver_kubeconfig="\$\{?RUNNER_TEMP\}?\//,
+    "the approver credential path must not be a predictable literal",
+  );
+});
+
+// The approval step runs with a credential that can rewrite the live admission
+// allowlist. `target_sha` is operator-supplied and, for a rollback, an arbitrary
+// historical revision — so the tooling it executes must come from the workflow's
+// own revision instead. Without this the credential runs whatever that commit
+// contained: nothing (rollback to before the script existed) or a reverted
+// implementation.
+test("the approval step runs tooling from the trusted workflow revision", () => {
+  const workflow = readFileSync(WORKFLOW, "utf8");
+
+  const checkoutAt = workflow.indexOf("- name: Checkout release tooling at trusted revision");
+  const approveAt = workflow.indexOf("- name: Approve deploy digest at admission time");
+  assert.notEqual(checkoutAt, -1, "docker.yml must check out release tooling separately");
+  assert.ok(checkoutAt < approveAt, "the tooling checkout must precede the approval step");
+
+  const checkout = workflow.slice(checkoutAt, approveAt);
+  assert.match(
+    checkout,
+    /ref: \$\{\{ github\.workflow_sha \}\}/,
+    "release tooling must be pinned to github.workflow_sha, not the deploy target",
+  );
+  assert.match(
+    checkout,
+    /path: \.release-tooling/,
+    "release tooling must land beside, not on top of, the deploy checkout",
+  );
+  assert.match(
+    checkout,
+    /actual=\$\(git -C \.release-tooling rev-parse HEAD/,
+    "the resolved tooling revision must be asserted, not assumed",
+  );
+
+  // The deploy checkout is still what Helm renders — only the approval tooling
+  // moves to the trusted revision.
+  const approveStep = workflow.slice(approveAt);
+  assert.doesNotMatch(
+    approveStep.slice(0, approveStep.search(/^ {6}- name: /m) || undefined),
+    /\.\/scripts\/approve-paperclip-api-digest\.sh/,
+    "the approval step must not execute the script out of the deploy checkout",
+  );
 });

From ecdc1371aaa538120782418aa5e496bb3a7eaf27 Mon Sep 17 00:00:00 2001
From: Omar Ramadan <omar@blockcast.net>
Date: Sat, 1 Aug 2026 12:12:22 -0700
Subject: [PATCH 4/6] test(e2e): stabilize onboarding and MCP approval waits

Co-Authored-By: Paperclip <noreply@paperclip.ing>
---
 .../e2e/conference-room-typing-intro.spec.ts  | 27 +++++++++++++++++--
 tests/e2e/mcp-user-stories.spec.ts            | 22 +++++++++++----
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/tests/e2e/conference-room-typing-intro.spec.ts b/tests/e2e/conference-room-typing-intro.spec.ts
index aa40d81e77f7..8f120dfe35de 100644
--- a/tests/e2e/conference-room-typing-intro.spec.ts
+++ b/tests/e2e/conference-room-typing-intro.spec.ts
@@ -13,6 +13,8 @@ const MISSION = "Verify the dashboard launch survives the wizard handoff.";
 const FIRST_TASK_TITLE = "Hire your first engineer and create a hiring plan";
 
 test.describe("Dashboard launch after onboarding wizard", () => {
+  test.setTimeout(120_000);
+
   test("creates the first task and opens the dashboard", async ({
     page,
     baseURL,
@@ -73,13 +75,34 @@ test.describe("Dashboard launch after onboarding wizard", () => {
     await page
       .getByPlaceholder("What is your team trying to achieve?")
       .fill(MISSION);
-    await page.getByRole("button", { name: /Confirm mission/ }).click();
+    const [companyCreateRes, goalCreateRes] = await Promise.all([
+      page.waitForResponse(
+        (res) => {
+          const url = new URL(res.url());
+          return res.request().method() === "POST" && url.pathname === "/api/companies";
+        },
+        { timeout: 60_000 },
+      ),
+      page.waitForResponse(
+        (res) => {
+          const url = new URL(res.url());
+          return (
+            res.request().method() === "POST" &&
+            /^\/api\/companies\/[^/]+\/goals$/.test(url.pathname)
+          );
+        },
+        { timeout: 60_000 },
+      ),
+      page.getByRole("button", { name: /Confirm mission/ }).click(),
+    ]);
+    expect(companyCreateRes.ok(), await companyCreateRes.text()).toBe(true);
+    expect(goalCreateRes.ok(), await goalCreateRes.text()).toBe(true);
 
     // Step 3: lead name (prefilled) -> Step 4 creates the inert CEO via the
     // route above after the adapter probe is intercepted.
     await expect(
       page.getByRole("heading", { name: "Create your team lead" }),
-    ).toBeVisible({ timeout: 15_000 });
+    ).toBeVisible({ timeout: 30_000 });
     await expect(
       page.locator('input[placeholder="Chief of staff"]'),
     ).toHaveValue("Chief of staff");
diff --git a/tests/e2e/mcp-user-stories.spec.ts b/tests/e2e/mcp-user-stories.spec.ts
index 4e1de84d34fc..b0af23bf0ae1 100644
--- a/tests/e2e/mcp-user-stories.spec.ts
+++ b/tests/e2e/mcp-user-stories.spec.ts
@@ -240,10 +240,22 @@ async function testCall(
   }));
 }
 
-async function approveActionRequest(request: APIRequestContext, companyId: string, actionRequestId: string) {
-  return await json(await request.post(`/api/tool-gateway/action-requests/${actionRequestId}/approve`, {
+async function approveActionRequest(
+  request: APIRequestContext,
+  companyId: string,
+  actionRequestId: string,
+  options: { connectionId?: string } = {},
+) {
+  const response = await request.post(`/api/tool-gateway/action-requests/${actionRequestId}/approve`, {
     data: { companyId },
-  }));
+  });
+  if (response.status() === 409 && options.connectionId) {
+    const body = await response.json().catch(() => null) as { reasonCode?: string } | null;
+    if (body?.reasonCode === "action_not_pending") {
+      return await pollTestCall(request, options.connectionId, actionRequestId, "done");
+    }
+  }
+  return await json(response);
 }
 
 async function declineActionRequest(request: APIRequestContext, companyId: string, actionRequestId: string) {
@@ -315,7 +327,7 @@ test.describe.serial("MCP prod Phase 5a user-story harness", () => {
       await page.goto(`/${seed.prefix}/apps/${connectionId}/review`);
       await screenshot(page, "US-2", "01-review-pending");
 
-      await approveActionRequest(request, seed.companyId, pending.actionRequestId!);
+      await approveActionRequest(request, seed.companyId, pending.actionRequestId!, { connectionId });
       await pollTestCall(request, connectionId, pending.actionRequestId!, "done");
       expect(mock.captures.filter((capture) => capture.method === "tools/call" && capture.toolName === "sheets:update_cell")).toHaveLength(1);
       await expectAuditEvent(request, seed.companyId, { connectionId, agentId: scout.id, search: "sheets:update_cell" });
@@ -432,7 +444,7 @@ test.describe.serial("MCP prod Phase 5a user-story harness", () => {
         expect(pending.decision).toBe("ask_first");
         await page.goto(`/${seed.prefix}/apps/${connectionId}/review`);
         await screenshot(page, "US-9", `review-${value}`);
-        await approveActionRequest(request, seed.companyId, pending.actionRequestId!);
+        await approveActionRequest(request, seed.companyId, pending.actionRequestId!, { connectionId });
         await pollTestCall(request, connectionId, pending.actionRequestId!, "done");
       }
       expect(mock.captures.filter((capture) => capture.method === "tools/call" && capture.toolName === "sheets:update_cell")).toHaveLength(2);

From 61d24366ceadd32e9f1c9bc81c85a24386f7f121 Mon Sep 17 00:00:00 2001
From: Omar Ramadan <omar@blockcast.net>
Date: Sat, 1 Aug 2026 17:30:18 -0700
Subject: [PATCH 5/6] ci(deploy): validate chart before digest approval

Move the side-effect-free Helm render/image validation ahead of the live admission allowlist mutation, then leave the stateful helm upgrade after approval. Also keep the raw approver secret out of child-process environments and make the approval script work on the Bash 3 runtime available on macOS runners.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
---
 .github/workflows/docker.yml                  | 37 ++++++++++++-------
 scripts/approve-paperclip-api-digest.sh       |  5 ++-
 scripts/approve-paperclip-api-digest.test.mjs | 32 ++++++++++++++++
 3 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
index fcb98a675b48..4a69b158183c 100644
--- a/.github/workflows/docker.yml
+++ b/.github/workflows/docker.yml
@@ -439,6 +439,29 @@ jobs:
           fi
           echo "approve_script=${script}" >> "$GITHUB_OUTPUT"
 
+      - name: Render and validate deploy chart
+        env:
+          DIGEST: ${{ steps.artifact.outputs.digest }}
+          NS: ${{ vars.PAPERCLIP_NAMESPACE || 'paperclip' }}
+          RELEASE: ${{ vars.PAPERCLIP_HELM_RELEASE || 'paperclip' }}
+          TAG: sha-${{ steps.target.outputs.short }}-k8s-vendored
+        run: |
+          set -euo pipefail
+          [[ "${DIGEST}" =~ ^sha256:[0-9a-f]{64}$ ]]
+          expected_image="harbor.blockcast.net/paperclip/paperclip@${DIGEST}"
+          rendered=$(helm template "${RELEASE}" ./deploy/helm/paperclip \
+            -n "${NS}" \
+            -f deploy/helm/paperclip/values.blockcast.yaml \
+            --set image.tag="${TAG}" \
+            --set-string image.digest="${DIGEST}")
+          rendered_images=$(printf '%s\n' "${rendered}" | awk \
+            '$1 == "image:" { gsub(/["'\''"]/, "", $2); if ($2 ~ /^harbor\.blockcast\.net\/paperclip\/paperclip([:@]|$)/) print $2 }')
+          if [ -z "${rendered_images}" ] || [ -n "$(printf '%s\n' "${rendered_images}" | grep -Fvx "${expected_image}" || true)" ]; then
+            echo "Chart did not render every Paperclip workload as ${expected_image}:" >&2
+            printf '%s\n' "${rendered_images:-<none>}" >&2
+            exit 1
+          fi
+
       - name: Approve deploy digest at admission time
         env:
           APPROVER_KUBECONFIG: ${{ secrets.KUBECONFIG_PAPERCLIP_RELEASE_APPROVER }}
@@ -471,6 +494,7 @@ jobs:
           # of a follow-up chmod so the file is never briefly world-readable.
           trap 'rm -rf "${approver_dir}"' EXIT
           (umask 077; printf '%s' "${APPROVER_KUBECONFIG}" > "${approver_kubeconfig}")
+          unset APPROVER_KUBECONFIG
 
           KUBECONFIG="${approver_kubeconfig}" \
             "${APPROVE_SCRIPT}" "${DIGEST}" | tee "${approval_log}"
@@ -495,19 +519,6 @@ jobs:
           set -euo pipefail
           [[ "${DIGEST}" =~ ^sha256:[0-9a-f]{64}$ ]]
           echo "rolling out ${TAG}@${DIGEST} to ${NS}/${RELEASE}"
-          expected_image="harbor.blockcast.net/paperclip/paperclip@${DIGEST}"
-          rendered=$(helm template "${RELEASE}" ./deploy/helm/paperclip \
-            -n "${NS}" \
-            -f deploy/helm/paperclip/values.blockcast.yaml \
-            --set image.tag="${TAG}" \
-            --set-string image.digest="${DIGEST}")
-          rendered_images=$(printf '%s\n' "${rendered}" | awk \
-            '$1 == "image:" { gsub(/["'\''"]/, "", $2); if ($2 ~ /^harbor\.blockcast\.net\/paperclip\/paperclip([:@]|$)/) print $2 }')
-          if [ -z "${rendered_images}" ] || [ -n "$(printf '%s\n' "${rendered_images}" | grep -Fvx "${expected_image}" || true)" ]; then
-            echo "Chart did not render every Paperclip workload as ${expected_image}:" >&2
-            printf '%s\n' "${rendered_images:-<none>}" >&2
-            exit 1
-          fi
           # --timeout bumped 10m → 30m on 2026-05-23 after rev 316 + several
           # prior revs hit "context deadline exceeded" while the cluster was
           # in apiserver/kubelet-latency cascade (control-plane nodes at
diff --git a/scripts/approve-paperclip-api-digest.sh b/scripts/approve-paperclip-api-digest.sh
index 7b096ffd9d77..86ce5ab13ac1 100755
--- a/scripts/approve-paperclip-api-digest.sh
+++ b/scripts/approve-paperclip-api-digest.sh
@@ -93,7 +93,10 @@ for attempt in $(seq 1 "$MAX_ROTATE_ATTEMPTS"); do
   # rollback target instead of two. Anything malformed is discarded here rather
   # than carried forward: the policy would ignore it anyway, and leaving it in
   # place would consume a slot the same way.
-  mapfile -t existing < <(
+  existing=()
+  while IFS= read -r entry; do
+    existing+=("$entry")
+  done < <(
     printf '%s\n' "$current_raw" \
       | tr -d '\r' \
       | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \
diff --git a/scripts/approve-paperclip-api-digest.test.mjs b/scripts/approve-paperclip-api-digest.test.mjs
index a8d3c42bd030..235831e55e55 100644
--- a/scripts/approve-paperclip-api-digest.test.mjs
+++ b/scripts/approve-paperclip-api-digest.test.mjs
@@ -256,9 +256,15 @@ test("the deploy workflow calls the script rather than re-inlining the rotation"
   // disk, so the trap has to be armed before the secret is written.
   const trapAt = step.indexOf("trap 'rm -rf");
   const writeAt = step.indexOf('printf \'%s\' "${APPROVER_KUBECONFIG}"');
+  const unsetAt = step.indexOf("unset APPROVER_KUBECONFIG");
+  const invokeAt = step.indexOf('"${APPROVE_SCRIPT}" "${DIGEST}"');
   assert.notEqual(trapAt, -1, "the step must install an EXIT trap for the approver credential");
   assert.notEqual(writeAt, -1, "the step must write the approver kubeconfig");
   assert.ok(trapAt < writeAt, "the EXIT trap must be armed before the credential reaches disk");
+  assert.notEqual(unsetAt, -1, "the step must unset the raw approver secret after writing the kubeconfig");
+  assert.notEqual(invokeAt, -1, "the step must invoke the committed script");
+  assert.ok(writeAt < unsetAt, "the raw approver secret must stay available until the kubeconfig is materialized");
+  assert.ok(unsetAt < invokeAt, "the raw approver secret must not be inherited by the approval script");
 
   // A fixed path is opened through any symlink already sitting at that name.
   // arc-deploy is a self-hosted pool, so residue from an earlier workload could
@@ -285,9 +291,15 @@ test("the approval step runs tooling from the trusted workflow revision", () =>
   const workflow = readFileSync(WORKFLOW, "utf8");
 
   const checkoutAt = workflow.indexOf("- name: Checkout release tooling at trusted revision");
+  const preflightAt = workflow.indexOf("- name: Render and validate deploy chart");
   const approveAt = workflow.indexOf("- name: Approve deploy digest at admission time");
+  const helmAt = workflow.indexOf("- name: helm upgrade");
   assert.notEqual(checkoutAt, -1, "docker.yml must check out release tooling separately");
+  assert.notEqual(preflightAt, -1, "docker.yml must validate the rendered chart before approval");
+  assert.notEqual(helmAt, -1, "docker.yml must still perform the Helm rollout");
   assert.ok(checkoutAt < approveAt, "the tooling checkout must precede the approval step");
+  assert.ok(preflightAt < approveAt, "chart rendering must be validated before mutating the admission allowlist");
+  assert.ok(approveAt < helmAt, "Helm rollout must remain after admission approval");
 
   const checkout = workflow.slice(checkoutAt, approveAt);
   assert.match(
@@ -314,4 +326,24 @@ test("the approval step runs tooling from the trusted workflow revision", () =>
     /\.\/scripts\/approve-paperclip-api-digest\.sh/,
     "the approval step must not execute the script out of the deploy checkout",
   );
+
+  const preflightStep = workflow.slice(preflightAt, approveAt);
+  assert.match(
+    preflightStep,
+    /helm template "\$\{RELEASE\}" \.\/deploy\/helm\/paperclip/,
+    "the side-effect-free preflight must render the target chart",
+  );
+  assert.match(
+    preflightStep,
+    /Chart did not render every Paperclip workload as \$\{expected_image\}/,
+    "the preflight must validate rendered Paperclip images before approval",
+  );
+
+  const helmStep = workflow.slice(helmAt);
+  const helmStepBody = helmStep.slice(0, helmStep.search(/^ {6}- name: /m) || undefined);
+  assert.doesNotMatch(
+    helmStepBody,
+    /helm template "\$\{RELEASE\}" \.\/deploy\/helm\/paperclip/,
+    "chart rendering must not wait until after admission approval",
+  );
 });

From b436f1c6c34e382997cc553b4df8132186e7ee15 Mon Sep 17 00:00:00 2001
From: Paperclip CTO <cto@paperclip.blockcast.net>
Date: Sun, 2 Aug 2026 01:49:38 +0000
Subject: [PATCH 6/6] ci(deploy): make the approval-window bound a constant,
 not an override (BLO-19955)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Ally's review of 7355542b is right that this repo's approval script has fallen
behind the reviewed companion implementation in Blockcast/onprem-k8s#1874. The
gap is real and larger than a refactor: the companion suite invokes the script
as `"$digest" "$plan"` with PAPERCLIP_DEPLOY_KUBECONFIG set, and this copy
usage-errors on a second argument, so that green `admission` check has provably
never executed the code shipped here.

Closing the whole gap needs the companion protocol to be final; #1874 still has
admission/verify/review pending and one unattempted Important. This commit takes
the one piece that is already reviewed, settled, and independent of the
plan/probe/lock work — the ring bound — so it stops being an outage vector while
the rest waits.

`MAX_APPROVED_DIGESTS` was `${PAPERCLIP_MAX_APPROVED_DIGESTS:-3}`, and the
post-write guard compared against that same variable. So raising it moved the
check that exists to catch exactly that. Demonstrated against the real script
with a stub apiserver: with the override at 4 the script exits 0, prints
"Approved. 4 digest(s) in the window.", and persists 4 entries — while the CEL
bound stays 3, which makes the policy deny every rollout. A widened writer bound
is never a widened policy, only a broken one.

Now a readonly constant that refuses a disagreeing override before touching the
ring, matching the companion fix. 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:
- 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

Refs BLO-19955, BLO-19834
---
 .github/workflows/docker.yml                  | 10 +++--
 scripts/approve-paperclip-api-digest.sh       | 19 ++++++++-
 scripts/approve-paperclip-api-digest.test.mjs | 41 ++++++++++++++++++-
 3 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
index 4a69b158183c..b4229b6a6fc8 100644
--- a/.github/workflows/docker.yml
+++ b/.github/workflows/docker.yml
@@ -467,10 +467,12 @@ jobs:
           APPROVER_KUBECONFIG: ${{ secrets.KUBECONFIG_PAPERCLIP_RELEASE_APPROVER }}
           DIGEST: ${{ steps.artifact.outputs.digest }}
           APPROVE_SCRIPT: ${{ steps.tooling.outputs.approve_script }}
-          # Must match maxApprovedApiDigests in the admission policy. The policy
-          # denies every rollout if the window is longer, so this is a hard
-          # ceiling rather than a preference.
-          PAPERCLIP_MAX_APPROVED_DIGESTS: '3'
+          # The approval window size is deliberately NOT set here. It is a
+          # readonly constant inside the approval script, kept in lockstep with
+          # maxApprovedApiDigests in the admission policy; a workflow-side value
+          # could only ever disagree with the policy, and the script refuses a
+          # disagreeing PAPERCLIP_MAX_APPROVED_DIGESTS rather than writing a ring
+          # the policy answers by denying every rollout.
         run: |
           set -euo pipefail
           if [ -z "${APPROVER_KUBECONFIG}" ]; then
diff --git a/scripts/approve-paperclip-api-digest.sh b/scripts/approve-paperclip-api-digest.sh
index 86ce5ab13ac1..365e2523be71 100755
--- a/scripts/approve-paperclip-api-digest.sh
+++ b/scripts/approve-paperclip-api-digest.sh
@@ -31,7 +31,24 @@ DATA_KEY="approvedDigests"
 # Must stay in lockstep with the `maxApprovedApiDigests` CEL variable in
 # onprem-k8s paperclip/paperclip-public-tools.yaml. The policy denies everything
 # if the list is longer, so a drift here is a hard outage, not a silent widening.
-MAX_APPROVED_DIGESTS="${PAPERCLIP_MAX_APPROVED_DIGESTS:-3}"
+#
+# Deliberately a constant and not an override. This script runs with the
+# approver credential, which cannot read the cluster-scoped policy to check
+# itself, and the window guard at the bottom of this file compares against this
+# same number — so an override raises the bound *and* moves the guard that
+# exists to catch exactly that, reporting "Approved. 4 digest(s) in the window."
+# while leaving the ring in a state the policy answers by denying every rollout.
+# Raising the writer-side bound never widens the policy; it only breaks it.
+readonly MAX_APPROVED_DIGESTS=3
+
+if [[ -n "${PAPERCLIP_MAX_APPROVED_DIGESTS:-}" \
+      && "$PAPERCLIP_MAX_APPROVED_DIGESTS" != "$MAX_APPROVED_DIGESTS" ]]; then
+  echo "refusing to approve: PAPERCLIP_MAX_APPROVED_DIGESTS=${PAPERCLIP_MAX_APPROVED_DIGESTS} disagrees with the" >&2
+  echo "${MAX_APPROVED_DIGESTS}-entry window the admission policy enforces. Raising the writer-side bound does not" >&2
+  echo "widen the policy; it makes the policy deny every rollout. Change the maxApprovedApiDigests CEL" >&2
+  echo "variable in onprem-k8s paperclip/paperclip-public-tools.yaml and this constant together." >&2
+  exit 2
+fi
 
 usage() {
   echo "usage: $0 sha256:<64 lowercase hex>" >&2
diff --git a/scripts/approve-paperclip-api-digest.test.mjs b/scripts/approve-paperclip-api-digest.test.mjs
index 235831e55e55..14769623918b 100644
--- a/scripts/approve-paperclip-api-digest.test.mjs
+++ b/scripts/approve-paperclip-api-digest.test.mjs
@@ -118,7 +118,7 @@ process.stdout.write(
  * Run the real script against a stub cluster.
  * @returns {{status:number, stdout:string, stderr:string, window:string[]}}
  */
-function approve(digestArg, { current, absent = false, conflicts = 0, tamperOnWrite } = {}) {
+function approve(digestArg, { current, absent = false, conflicts = 0, tamperOnWrite, env = {} } = {}) {
   const dir = mkdtempSync(join(tmpdir(), "paperclip-approve-"));
   const statePath = join(dir, "state.json");
   const state = {
@@ -141,6 +141,7 @@ function approve(digestArg, { current, absent = false, conflicts = 0, tamperOnWr
       PATH: `${dir}:${process.env.PATH}`,
       STUB_STATE: statePath,
       PAPERCLIP_APPROVAL_ROTATE_ATTEMPTS: "3",
+      ...env,
     },
     stdio: ["ignore", "pipe", "pipe"],
   });
@@ -157,6 +158,33 @@ test("prepends the new digest and evicts the oldest at the 3-entry bound", { ski
   assert.deepEqual(window, [D, A, B]);
 });
 
+// The window size is enforced by CEL in the admission policy, which this script
+// cannot read. It used to be an override (`${PAPERCLIP_MAX_APPROVED_DIGESTS:-3}`)
+// that also fed the post-write guard, so raising it moved the check that exists
+// to catch exactly that: the script wrote a 4-entry ring, compared it against
+// the same raised 4, and reported "Approved. 4 digest(s) in the window." while
+// the policy — still bounded at 3 — answered by denying *every* rollout. A
+// widened writer bound is a hard outage, never a widened policy, so a
+// disagreeing value must be refused before the ring is touched.
+test("refuses a window bound that disagrees with the policy, leaving the ring intact", { skip }, () => {
+  const { status, stderr, window } = approve(D, {
+    current: [A, B, C].join("\n"),
+    env: { PAPERCLIP_MAX_APPROVED_DIGESTS: "4" },
+  });
+  assert.equal(status, 2, "a disagreeing bound must be refused, not honoured");
+  assert.match(stderr, /disagrees with the/);
+  assert.deepEqual(window, [A, B, C], "the approval ring must be untouched on refusal");
+});
+
+test("accepts a bound that agrees with the policy", { skip }, () => {
+  const { status, window } = approve(D, {
+    current: [A, B, C].join("\n"),
+    env: { PAPERCLIP_MAX_APPROVED_DIGESTS: "3" },
+  });
+  assert.equal(status, 0);
+  assert.deepEqual(window, [D, A, B]);
+});
+
 // The regression this file was added for. Rotating `A,A,B` used to yield
 // `C,A,A` — three entries, two distinct — silently dropping B and leaving one
 // usable rollback target where the window promises two.
@@ -251,6 +279,17 @@ test("the deploy workflow calls the script rather than re-inlining the rotation"
     "the step must not reimplement the rotation inline",
   );
 
+  // The bound belongs to the admission policy, so the workflow must not carry a
+  // second copy of it. A value here can only ever agree (redundant) or disagree
+  // (a ring the policy denies outright); the script owns it as a constant.
+  // Scoped to an actual env assignment so the step may still explain *why* it
+  // does not set one.
+  assert.doesNotMatch(
+    step,
+    /^\s*PAPERCLIP_MAX_APPROVED_DIGESTS\s*:/m,
+    "the workflow must not pin the approval window size; the script owns that constant",
+  );
+
   // The approver is the higher-privilege of the two credentials this job
   // handles; on a self-hosted runner a mid-step failure must not leave it on
   // disk, so the trap has to be armed before the secret is written.