feat(chart): stamp the approval plan marker on the API pod template (BLO-20733)

.github/workflows/pr.yml

@@ -141,6 +141,42 @@ jobs:
           retention-days: 1
           if-no-files-found: error
 
+  # The chart tests under deploy/helm/paperclip/tests/ were never wired into any
+  # workflow -- 21 assertions about the manifests we actually ship (Penstock
+  # routing, ServiceMonitor RBAC, runtime caches, probes) that no CI run has ever
+  # executed. This job runs them, and gates the BLO-20733 approval-plan marker:
+  # the release channel's completion check is bound to a marker computed from an
+  # unstamped render, which is only sound while stamping perturbs nothing else.
+  #
+  # Deliberately its own job rather than a step in `policy`: it needs helm and
+  # kubectl, and a tooling regression in provisioning those should not take down
+  # the gate that every unrelated PR depends on.
+  helm_chart:
+    name: Helm chart
+    runs-on: arc-light
+    timeout-minutes: 10
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v6
+
+      - name: Setup Node.js
+        uses: actions/setup-node@v6
+        with:
+          node-version: 24
+
+      - name: Install helm
+        uses: azure/setup-helm@v4
+
+      # Only ever invoked as `kubectl create --dry-run=client -o json`, i.e. as
+      # an offline YAML reader so the render comparisons are structural rather
+      # than textual. No cluster credential is used or needed.
+      - name: Install kubectl
+        uses: azure/setup-kubectl@v4
+
+      - name: Test Helm chart renders
+        run: node --test ./deploy/helm/paperclip/tests/*.test.mjs
+
   typecheck_release_registry:
     name: Typecheck + Release Registry
     needs: [policy]
@@ -351,7 +387,7 @@ jobs:
     # Preserve the legacy required-check name while the underlying work runs in parallel.
     name: verify
     if: ${{ always() }}
-    needs: [typecheck_release_registry, general_tests, worktree_install, build]
+    needs: [typecheck_release_registry, general_tests, worktree_install, build, helm_chart]
     runs-on: arc-light
     timeout-minutes: 5
 
@@ -382,12 +418,14 @@ jobs:
           GENERAL_TESTS_RESULT: ${{ needs.general_tests.result }}
           WORKTREE_INSTALL_RESULT: ${{ needs.worktree_install.result }}
           BUILD_RESULT: ${{ needs.build.result }}
+          HELM_CHART_RESULT: ${{ needs.helm_chart.result }}
         run: |
           declare -A lane_results=(
             [typecheck_release_registry]="$TYPECHECK_RELEASE_REGISTRY_RESULT"
             [general_tests]="$GENERAL_TESTS_RESULT"
             [worktree_install]="$WORKTREE_INSTALL_RESULT"
             [build]="$BUILD_RESULT"
+            [helm_chart]="$HELM_CHART_RESULT"
           )
 
           cancelled_lanes=()

deploy/helm/paperclip/templates/deployment-api.yaml

@@ -41,7 +41,49 @@ spec:
         {{- with .Values.pod.labels }}
         {{- toYaml . | nindent 8 }}
         {{- end }}
-      {{- with .Values.pod.annotations }}
+      {{- /*
+      BLO-20733: stamp the approval plan marker onto the POD TEMPLATE, so it is
+      part of the rolled-out spec rather than top-level metadata a config-only
+      edit could carry. scripts/approve-paperclip-api-digest.sh refuses any plan
+      whose template lacks it, records it on the in-flight lock, and its
+      ROLLOUT_COMPLETE_JQ requires the live template to carry this exact value
+      before retiring that lock -- so a healthy rollout of a DIFFERENT plan that
+      happens to reuse the approved digest can no longer retire it.
+
+      The marker's value is the SHA-256 of the canonical rendered Deployment
+      with this annotation removed, which is why it cannot be computed inside
+      the template (it would hash a document containing itself). The deploy job
+      renders once unstamped, hashes, then re-renders with the value set.
+
+      That two-pass scheme is only sound if setting this value changes NOTHING
+      ELSE in the rendered output. Keeping the stamp confined to this one merged
+      key is what preserves it, and
+      deploy/helm/paperclip/tests/approval-plan-marker.test.mjs pins it by
+      diffing a stamped render against an unstamped one.
+
+      The marker key is RELEASE-CONTROLLED, so pod.annotations may not carry it.
+      Without this guard the key has two silent failure modes, both of which
+      defeat the binding above. With api.approvalPlanSha256 unset, a value from
+      pod.annotations would be copied straight through -- stamping an
+      unvalidated marker into the render the release job treats as "unstamped",
+      so the hash it computes is taken over a document that already contains a
+      marker and can never match what the approve script recomputes. With it
+      set, `set` would silently overwrite the caller's value, hiding the
+      conflict rather than reporting it. Rejecting the key outright is the only
+      behaviour that keeps render #1 genuinely unstamped.
+      */}}
+      {{- $markerKey := "paperclip.blockcast.net/approval-plan-sha256" }}
+      {{- $podAnnotations := deepCopy (.Values.pod.annotations | default dict) }}
+      {{- if hasKey $podAnnotations $markerKey }}
+      {{- fail (printf "pod.annotations must not set %s: it is release-controlled and stamped from api.approvalPlanSha256" $markerKey) }}
+      {{- end }}
+      {{- with .Values.api.approvalPlanSha256 }}
+      {{- if not (regexMatch "^[0-9a-f]{64}$" .) }}
+      {{- fail (printf "api.approvalPlanSha256 must be 64 lowercase hex characters, got %q" .) }}
+      {{- end }}
+      {{- $_ := set $podAnnotations $markerKey . }}
+      {{- end }}
+      {{- with $podAnnotations }}
       annotations:
         {{- toYaml . | nindent 8 }}
       {{- end }}

deploy/helm/paperclip/tests/approval-plan-marker.test.mjs

@@ -0,0 +1,192 @@
+// BLO-20733 — the approval plan marker the release channel binds completion to.
+//
+// scripts/approve-paperclip-api-digest.sh (Blockcast/onprem-k8s, and the
+// vendored copy here) refuses any planned Deployment whose POD TEMPLATE lacks
+// `paperclip.blockcast.net/approval-plan-sha256`, and computes the value it
+// expects as:
+//
+//   sha256( jq -cS 'del(.spec.template.metadata.annotations[marker])' <plan> )
+//
+// i.e. the canonical full Deployment with the marker itself removed. The marker
+// therefore cannot be produced inside the Helm template — it would have to hash
+// a document containing its own value. The release job instead renders the
+// chart TWICE: once unstamped to obtain the hash, then again with
+// `--set api.approvalPlanSha256=<hash>` to produce the manifest it both hands
+// to the approve script and deploys.
+//
+// That scheme is sound if and only if stamping changes NOTHING ELSE in the
+// rendered output. If it did, the hash taken from render #1 would not match the
+// hash the approve script recomputes from render #2, and every release would
+// die at "planned Deployment pod template must carry ...". These tests pin that
+// invariant — they are the "rendered chart output and the manifest the approve
+// script hashes are demonstrated to agree" acceptance criterion.
+import { execFileSync } from "node:child_process";
+import assert from "node:assert/strict";
+import { test } from "node:test";
+import path from "node:path";
+import { fileURLToPath } from "node:url";
+
+const repoRoot = path.resolve(
+  path.dirname(fileURLToPath(import.meta.url)),
+  "../../../..",
+);
+
+const MARKER = "paperclip.blockcast.net/approval-plan-sha256";
+// Any 64-hex string exercises the plumbing; the real value is a SHA-256 the
+// release job computes from the unstamped render.
+const SAMPLE = "a".repeat(64);
+
+function renderApiDeployment(extraArgs = []) {
+  const yaml = execFileSync(
+    "helm",
+    [
+      "template",
+      "paperclip",
+      "deploy/helm/paperclip",
+      "--namespace",
+      "paperclip",
+      "-f",
+      "deploy/helm/paperclip/values.blockcast.yaml",
+      "--show-only",
+      "templates/deployment-api.yaml",
+      ...extraArgs,
+    ],
+    { cwd: repoRoot, encoding: "utf8" },
+  );
+  // kubectl is the YAML->JSON reader here purely so the comparison below is
+  // structural rather than textual; --dry-run=client needs no cluster.
+  return JSON.parse(
+    execFileSync("kubectl", ["create", "--dry-run=client", "-o", "json", "-f", "-"], {
+      input: yaml,
+      encoding: "utf8",
+      stdio: ["pipe", "pipe", "ignore"],
+    }),
+  );
+}
+
+// Mirrors CANONICAL_UNSTAMPED_PLAN in approve-paperclip-api-digest.sh, including
+// its empty-map cleanup: after deleting the only annotation, jq's `del` leaves
+// `annotations: {}` behind, which is NOT what an unstamped render produces. The
+// script prunes it, so the equivalence asserted here is the one it actually
+// computes. Keep the two in step.
+function stripMarker(deployment) {
+  const stripped = structuredClone(deployment);
+  const templateMeta = stripped.spec?.template?.metadata;
+  if (!templateMeta?.annotations) return stripped;
+
+  delete templateMeta.annotations[MARKER];
+  if (Object.keys(templateMeta.annotations).length === 0) {
+    delete templateMeta.annotations;
+  }
+  if (Object.keys(templateMeta).length === 0) {
+    delete stripped.spec.template.metadata;
+  }
+  return stripped;
+}
+
+test("no marker is stamped by default (non-Blockcast deploys skip the approval channel)", () => {
+  const rendered = renderApiDeployment();
+
+  assert.equal(
+    rendered.spec.template.metadata.annotations?.[MARKER],
+    undefined,
+    "unset api.approvalPlanSha256 must render no marker annotation",
+  );
+});
+
+test("the marker lands on the POD TEMPLATE, not top-level Deployment metadata", () => {
+  const rendered = renderApiDeployment([`--set`, `api.approvalPlanSha256=${SAMPLE}`]);
+
+  assert.equal(
+    rendered.spec.template.metadata.annotations[MARKER],
+    SAMPLE,
+    "the marker must be on spec.template.metadata.annotations so it is part of the rolled-out pod spec",
+  );
+  // Top-level metadata survives a config-only edit that never rolls pods, which
+  // is exactly the evidence ROLLOUT_COMPLETE_JQ must not accept.
+  assert.equal(
+    rendered.metadata.annotations?.[MARKER],
+    undefined,
+    "the marker must NOT be stamped on top-level Deployment metadata",
+  );
+});
+
+// The load-bearing test: without this, a two-pass render is unsound.
+test("stamping the marker changes nothing else in the rendered Deployment", () => {
+  const unstamped = renderApiDeployment();
+  const stamped = renderApiDeployment([`--set`, `api.approvalPlanSha256=${SAMPLE}`]);
+
+  assert.notDeepEqual(
+    stamped,
+    unstamped,
+    "sanity: the stamped render must actually differ, or this test proves nothing",
+  );
+  assert.deepEqual(
+    stripMarker(stamped),
+    unstamped,
+    "stamped render minus the marker must equal the unstamped render, or the hash " +
+      "computed from render #1 cannot match what the approve script recomputes from render #2",
+  );
+});
+
+test("the invariant also holds when pod.annotations is already non-empty", () => {
+  // values.blockcast.yaml currently leaves pod.annotations empty, so the stamp
+  // is what creates the annotations map. Pin the other branch too: an operator
+  // adding a pod annotation later must not break the release channel, and
+  // toYaml's alphabetical ordering must not shift anything.
+  const existing = [`--set`, `pod.annotations.example\\.com/team=paperclip`];
+  const unstamped = renderApiDeployment(existing);
+  const stamped = renderApiDeployment([
+    ...existing,
+    `--set`,
+    `api.approvalPlanSha256=${SAMPLE}`,
+  ]);
+
+  assert.equal(
+    stamped.spec.template.metadata.annotations["example.com/team"],
+    "paperclip",
+    "stamping must not clobber pre-existing pod annotations",
+  );
+  assert.deepEqual(stripMarker(stamped), unstamped);
+});
+
+test("a malformed marker fails the render instead of shipping a plan the approver rejects", () => {
+  // Fail at render time, where the message names the value, rather than several
+  // steps later inside the approve script where it reads as a hash mismatch.
+  assert.throws(
+    () => renderApiDeployment([`--set`, `api.approvalPlanSha256=not-a-sha`]),
+    /approvalPlanSha256 must be 64 lowercase hex/,
+  );
+});
+
+// The marker key is release-controlled. Both collision cases below are silent
+// without the hasKey guard in deployment-api.yaml, and both break the two-pass
+// render in a way that only surfaces later, inside the approve script, as an
+// unexplained hash mismatch. The tests above use `example.com/team`, an
+// ordinary key, so they cannot catch either one.
+const podMarkerArg = [`--set`, `pod.annotations.${MARKER.replace(/\./g, "\\.")}=${SAMPLE}`];
+
+test("pod.annotations may not smuggle the marker in when api.approvalPlanSha256 is UNSET", () => {
+  // The dangerous case: this render is the one the release job hashes as
+  // "unstamped". A marker reaching it means the hash is taken over a document
+  // that already carries a marker, so the value the approve script recomputes
+  // from render #2 can never agree with it.
+  assert.throws(
+    () => renderApiDeployment(podMarkerArg),
+    /pod\.annotations must not set paperclip\.blockcast\.net\/approval-plan-sha256/,
+  );
+});
+
+test("pod.annotations may not silently lose to the release value when BOTH are set", () => {
+  // Previously `set` overwrote the caller's value and rendered successfully,
+  // reporting nothing. Conflicting intent must be an error, not a winner.
+  assert.throws(
+    () =>
+      renderApiDeployment([
+        ...podMarkerArg,
+        `--set`,
+        `api.approvalPlanSha256=${"b".repeat(64)}`,
+      ]),
+    /pod\.annotations must not set paperclip\.blockcast\.net\/approval-plan-sha256/,
+  );
+});

deploy/helm/paperclip/values.yaml

@@ -87,6 +87,16 @@ api:
   # -- Soft anti-affinity to spread API replicas across nodes. Falls back to
   # co-located if the cluster has only one schedulable paperclip node.
   spreadAcrossNodes: true
+  # -- Approval plan marker (BLO-20733), stamped onto the API pod template as
+  # `paperclip.blockcast.net/approval-plan-sha256`. 64 lowercase hex, or empty
+  # to render no annotation (the default -- local and non-Blockcast deploys do
+  # not go through the admission approval channel).
+  #
+  # Set ONLY by the release job, and never by hand: the value is the SHA-256 of
+  # the canonical rendered Deployment with this annotation removed, so it can
+  # only be obtained by rendering the chart unstamped first. A hand-set value
+  # makes `approve-paperclip-api-digest.sh` refuse the plan.
+  approvalPlanSha256: ""
 
 # -- Kubernetes Service exposing the HTTP UI/API.
 service:

scripts/__tests__/pr-verify-lane-outcome.test.mjs

@@ -35,6 +35,7 @@ function runVerifyStep(results) {
     GENERAL_TESTS_RESULT: results.general_tests ?? "success",
     WORKTREE_INSTALL_RESULT: results.worktree_install ?? "success",
     BUILD_RESULT: results.build ?? "success",
+    HELM_CHART_RESULT: results.helm_chart ?? "success",
   };
   return spawnSync("bash", ["-c", script], { env, encoding: "utf8" });
 }
@@ -107,3 +108,36 @@ test("verify step exits non-zero for an unrecognized result and treats it as a f
   assert.match(result.stdout, /::error title=verify: lane failure::/);
   assert.match(result.stdout, /general_tests/);
 });
+
+// BLO-20733 / Ally review of PR #973: the `Helm chart` lane exists and runs, but
+// running is not gating. `verify` is the required context, so a lane it neither
+// `needs` nor asserts can go red beside a green required check. Both halves are
+// load-bearing and fail independently:
+//   - absent from `needs`  => `needs.helm_chart.result` renders EMPTY, and the
+//     lane silently never gates (the failure mode being closed here).
+//   - absent from the script => the result is collected and ignored.
+test("verify declares helm_chart as a dependency, so its result is actually populated", () => {
+  const needsMatch = workflow.match(/\n {2}verify:\n(?: {4}.*\n| *\n)*? {4}needs: \[([^\]]*)\]/);
+  assert.notEqual(needsMatch, null, "verify job must declare a needs list");
+
+  const needs = needsMatch[1].split(",").map((lane) => lane.trim());
+  assert.ok(
+    needs.includes("helm_chart"),
+    `verify.needs must include helm_chart or its result is always empty; got: ${needs.join(", ")}`,
+  );
+});
+
+test("verify step exits non-zero when the Helm chart lane fails", () => {
+  const result = runVerifyStep({ helm_chart: "failure" });
+  assert.notEqual(result.status, 0);
+  assert.match(result.stdout, /::error title=verify: lane failure::/);
+  assert.match(result.stdout, /helm_chart/);
+});
+
+test("verify step reports a cancelled Helm chart lane as cancelled, not as a diff defect", () => {
+  const result = runVerifyStep({ helm_chart: "cancelled" });
+  assert.notEqual(result.status, 0);
+  assert.match(result.stdout, /::error title=verify: lane cancelled::/);
+  assert.match(result.stdout, /helm_chart/);
+  assert.doesNotMatch(result.stdout, /::error title=verify: lane failure::/);
+});