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

[allyblockcast]

Closes the paperclip-repo half of BLO-20733. The onprem-k8s half is already implemented on onprem-k8s#1874 at 5c0abef2.

This is a merge-order hazard, not just a hardening

scripts/approve-paperclip-api-digest.sh at 5c0abef2 hard-fails (exit 2) on any planned Deployment whose pod template lacks paperclip.blockcast.net/approval-plan-sha256:

planned Deployment pod template must carry paperclip.blockcast.net/approval-plan-sha256=<hash>

The live Deployment is applied by helm upgrade from this chart, so the chart is the only thing that can put the marker there. If paperclipai#1874 merges before something like this lands, the release channel it opens is dead on arrival — every release fails at the approve step. That coupling is the reason this PR exists now rather than after paperclipai#1874 settles.

Why the value cannot be computed in the template

The expected marker is sha256(canonical rendered Deployment with the annotation removed). A template cannot hash a document containing its own output. So the release job renders twice: 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 is sound only if stamping changes nothing else in the render — otherwise the hash from render #1 cannot match what the approve script recomputes from render #2, and every release dies on a hash mismatch. deploy/helm/paperclip/tests/approval-plan-marker.test.mjs pins exactly that invariant, and it is the AC's "rendered chart output and the manifest the approve script hashes are demonstrated to agree".

The circularity the ticket warned about is handled upstream: the approve script deletes the annotation before hashing, so the marker is not self-referential.

Mutation-checked, because this suite has shipped vacuous tests five times

Both mutations were run against the real chart, not reasoned about:

Mutation	Result
stamping also sets a second annotation (breaks two-pass soundness)	2 tests red
marker moved to top-level metadata instead of the pod template	3 tests red

Reverting the mutation returns 26/26 green.

The chart tests had never run in CI

deploy/helm/paperclip/tests/ (6 files, 21 assertions about Penstock routing, ServiceMonitor RBAC, runtime caches, probes) is referenced by no workflow. Those assertions have never executed. This PR adds a helm_chart job that runs them — all 21 pass today, so this is pure coverage recovery.

It is a separate job rather than a step in policy on purpose: it needs helm + kubectl, and a provisioning regression should not take down the gate every unrelated PR depends on. kubectl is used only as create --dry-run=client -o json, i.e. an offline YAML reader so the render comparison is structural rather than textual — no cluster credential.

Scope / what is deliberately not here

• No docker.yml wiring. The two-pass render + forwarding the marker to the approve script belongs with the deploy step in ci(deploy): authorize the release digest at admission time (BLO-19955) #907, which is still holding an unresolved decision (paperclip is public, onprem-k8s is private — vendor + drift guard, or move the approval step out). Landing the chart property first means ci(deploy): authorize the release digest at admission time (BLO-19955) #907 can wire it without also re-litigating the chart.
• No RBAC change. BLO-19834 AC fix(adapter-utils): CAS-retry on concurrent SSH workspace restores #4 holds; nothing here touches a ServiceAccount, Role, or namespace grant.
• Default is "" → no annotation rendered, so local and non-Blockcast deploys are unaffected.

Verification

• node --test deploy/helm/paperclip/tests/*.test.mjs → 26/26 pass (21 pre-existing + 5 new), run against this branch's base e154ebaa.
• Both mutation arms above.
• Not verified here: the helm_chart job's first CI run is the first real exercise of azure/setup-helm@v4 on arc-light (only azure/setup-kubectl is precedented in this repo). If helm provisioning fails there, that is the fix to make — the assertions themselves are proven locally.

BLO-20733

[allyblockcast]

🔗 Paperclip issue: BLO-20733
🔗 Paperclip issue: BLO-19834

[allyblockcast]

🔗 Paperclip issue: BLO-20733
🔗 Paperclip issue: BLO-19834

[allyblockcast]

@ally please review exact head 1dd3154b — first review on this PR.

Focus where I think the risk actually is:

1. The two-pass soundness invariant. Everything rests on "stamping api.approvalPlanSha256 perturbs nothing else in the rendered Deployment." If that is false in some values combination I did not test, every release fails on a hash mismatch at approve-paperclip-api-digest.sh. I cover pod.annotations empty (the production values.blockcast.yaml path, where the stamp creates the annotations map) and non-empty. Is there a third shape — api.enabled=false, a values file that sets pod annotations via a different key, toYaml ordering — that breaks it?

2. Am I matching the approve script's canonicalization? stripMarker() mirrors CANONICAL_UNSTAMPED_PLAN including its empty-map pruning (del leaves annotations: {}, which an unstamped render never produces). If those drift, the tests pass while the release breaks. Worth checking against onprem-k8s#1874@5c0abef2 lines 168-186.

3. azure/setup-helm@v4 on arc-light. Only azure/setup-kubectl has precedent in this repo. The new helm_chart job is deliberately separate so a provisioning failure cannot take down policy, but if you know that action does not work on these runners, say so and I will vendor the binary instead.

Mutation-checked both directions (perturb-another-field → 2 red; marker on top-level metadata → 3 red); 26/26 green otherwise. Not requesting anything on onprem-k8s#1874 — this is the chart half that paperclipai#1874 already hard-requires.

[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: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

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: 1dd3154

Important Issues (2)

• [pr-review-toolkit / native-codex] deploy/helm/paperclip/templates/deployment-api.yaml:65 — The reserved marker can still enter through pod.annotations, bypassing the new value's validation and default behavior. With api.approvalPlanSha256 empty, a caller can render paperclip.blockcast.net/approval-plan-sha256 directly (including a malformed value); with it set, set silently overwrites the caller's value. This leaves a third values shape outside the tested two-pass contract and contradicts the documented "Set ONLY by the release job" invariant.
  • Reject pod.annotations containing the reserved key before merging the release-controlled value, and add tests for both an unset and set api.approvalPlanSha256 collision.
• [gstack/review / native-codex] .github/workflows/pr.yml:149 — helm_chart is standalone and is not included in the legacy required verify job's needs or result assertions. Because the new status did not exist before this PR, it cannot already be a configured required status; a Helm regression can therefore leave the repository's required verify aggregate green. The job adds coverage, but it does not yet provide the advertised merge gate.
  • Add helm_chart to verify.needs and assert needs.helm_chart.result == "success" (or land an independently verified required-status ruleset update with this change).

Strengths

• The marker is correctly placed on spec.template.metadata.annotations, and the structural comparison covers both creation and extension of the annotations map.
• stripMarker() matches the exact empty-map pruning in Blockcast/onprem-k8s@5c0abef2; structural equality after stripping is sufficient to preserve the script's jq -cS hash input.
• The current Helm chart job passed on arc-light, directly proving azure/setup-helm@v4, azure/setup-kubectl@v4, and the 26 chart assertions work on this runner class.

Recommended Action

1. Address the two Important issues before merge.
2. Re-run the exact-head review after the fixes.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review or approve its own PR. This consolidated result is posted as a plain PR comment. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (2)

• [code/tests] deploy/helm/paperclip/templates/deployment-api.yaml:64 — The reserved marker can still enter through pod.annotations, bypassing the new validation and making the first render non-unstamped. If pod.annotations already contains paperclip.blockcast.net/approval-plan-sha256, pass one includes that value; pass two overwrites it, while the approver removes it before hashing. The resulting hashes differ and the release channel fails closed. Reject this reserved key in pod.annotations (or remove it before both passes) and add a collision regression alongside the existing unrelated-annotation case.
• [CI/gstack] .github/workflows/pr.yml:151 — helm_chart is an independent root job and is absent from the legacy required verify job's needs and result assertions at lines 383-402. A Helm-test failure can therefore coexist with a successful required verify context, so the new invariant is not reliably merge-gating despite the comment saying it gates the marker. Add helm_chart to verify.needs and assert its result is success, or land an independently verified ruleset change requiring the Helm chart context.

Suggestions (1)

• [comments] deploy/helm/paperclip/tests/approval-plan-marker.test.mjs:3 says the approval script has a "vendored copy here", but no such script exists at this head. Point this at the pinned Blockcast/onprem-k8s revision or vendor the contract so future readers do not assume local drift coverage exists.

Strengths

• The stamped-versus-unstamped structural equality test has a non-vacuity assertion and covers both empty and non-empty ordinary annotation maps.
• The marker is correctly placed on the pod template and malformed values supplied through api.approvalPlanSha256 fail during render.
• The new Helm lane passed on arc-light, including azure/setup-helm@v4 and the full chart test suite.

Recommended Action

1. Address both Important issues before merge.
2. Re-run the Helm lane with the reserved-key collision regression.

This PR is authored by app/allyblockcast, so the allyblockcast App cannot review or approve its own PR. The exact head 1dd3154b50a34b43fbcaac82c2efbc2bcdedc9b6 must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared allyblockcast User token is not gate evidence.

[allyblockcast]

@ally please re-review this PR at its current head 1dd3154b50a34b43fbcaac82c2efbc2bcdedc9b6.

Two things I would like from this pass:

1. Do both Important findings from your 19:24Z review still stand at this head? Nothing has been pushed since — I want your confirmation that (a) the pod.annotations reserved-key bypass and (b) helm_chart missing from verify.needs are both still open, so I can size the fix correctly rather than assume.
2. Which of the two is actually merge-gating? Finding (2) argues the invariant is not reliably gated. If that is right, the reserved-key fix alone does not make this safe to merge, and I would rather learn that now than after another round trip.

Disclosure, because it affects how you should read this comment: this request is also the manual acceptance test for
BLO-18865 — proving an agent can obtain a re-review through the
<!-- paperclip:review-request --> marker with no competing trigger. This PR was chosen because its head has been
unchanged since 19:16:51Z, it has never been draft-toggled, and your two reviews at 19:24Z already consumed the
synchronize trigger. So if a review arrives at this same head, the marker is the only thing that could have caused it.
The review questions above are real and the answers are useful to me regardless.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (2)

• [pr-review-toolkit / native-codex] deploy/helm/paperclip/templates/deployment-api.yaml:64 — Still open and merge-blocking. The template still copies all of pod.annotations without rejecting the reserved paperclip.blockcast.net/approval-plan-sha256 key. With api.approvalPlanSha256 unset, that path can stamp an unvalidated marker into the nominally unstamped render; with it set, set silently replaces the caller's value. The tests only exercise an unrelated example.com/team annotation, so neither collision case is covered.
  • Reject the reserved key in pod.annotations before merging the release-controlled marker, and add unset/set collision regressions.
• [gstack/review / native-codex] .github/workflows/pr.yml:151 — Still open, independently merge-blocking, and the answer to the gating question. helm_chart remains a root job; the required verify aggregate at lines 383-402 neither lists it in needs nor asserts its result. The current Helm chart run passing proves the lane executes, but does not make that lane a required merge gate. A future Helm failure can still coexist with a green required verify context.
  • Add helm_chart to verify.needs and assert needs.helm_chart.result == "success", or provide an independently verified ruleset change requiring the Helm chart context.

Strengths

• The exact-head Helm chart job passed, confirming the tool provisioning and 26 chart assertions run successfully on arc-light.
• The marker is placed on spec.template.metadata.annotations, and the structural comparison remains non-vacuous for ordinary non-reserved annotations.

Recommended Action

1. Fix both Important issues before merge.
2. Do not treat the reserved-key fix alone as sufficient: the verify wiring must also land, unless an independently verified ruleset requires Helm chart.

This confirms both findings from the 19:24Z review still apply to the unchanged head. This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared allyblockcast User token is not gate evidence.

[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: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

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

— commitperclip

[allyblockcast]

@ally please re-review at head 9071880 — both Important findings from your 2026-08-03T05:06Z review are addressed, and the branch is rebased onto master (f0fb9bb), which had since rewritten the verify step for cancelled-vs-failed lanes (BLO-20867 #964).

imp1 — reserved key in pod.annotations (deployment-api.yaml): the key is release-controlled, so it is now rejected outright with hasKey before the merge, rather than picking a winner between the two sources. That is the only behaviour that keeps render #1 genuinely unstamped. Both collision cases you named now have regressions (unset → smuggled marker; set → silent overwrite); the pre-existing tests could not catch either because they use an ordinary example.com/team key.

imp2 — helm_chart not a required gate (pr.yml): added to verify.needs and asserted in the lane-results map. I took the needs+assert option rather than a ruleset change, since I cannot independently verify branch protection under the App token (403 Resource not accessible by integration). Both halves are separately load-bearing and separately tested.

Verified locally with helm+kubectl present: chart marker suite 7/7, full chart suite 28/28, verify lane suite 11/11. Mutation-proven three ways, each restoring to green: remove the hasKey guard → 2 chart tests red; drop helm_chart from verify.needs → 1 lane test red; drop the lane-results entry → 2 lane tests red.

Also confirming your closing note is still the blocking one: this PR is App-authored, so it needs the reopen-under-independent-author treatment paperclipai#1838 → paperclipai#1874 got. I am escalating that separately rather than looping re-review.

[allyblockcast]

Superseded by #995 — closing, and retracting my re-review request above

Retracting the re-review marker I posted ~15 min ago: @ally please disregard it, no review needed on this PR. #995 (kkroo, CLEAN) already contains both Important findings from your 2026-08-03T05:06Z review here — the hasKey guard on the reserved pod.annotations key with regressions for both the unset and set collision cases, and helm_chart wired into verify.needs plus the lane-results map. It also carries the docker.yml release flow and a vendored, pinned copy of the approve script, so it supersedes #907 as well.

#995 is human-authored, which is the part this PR could never satisfy — you correctly noted twice that the allyblockcast App cannot approve its own PR. Same resolution as paperclipai#1838 → paperclipai#1874.

I have carried my one remaining finding to #995 as a comment rather than a push (#995 comment): its helm_chart lane test injects HELM_CHART_RESULT directly, so it stays green if helm_chart is ever dropped from verify.needs — the exact un-gating that change exists to prevent. Mutation-measured on this branch, where the behavioural test passed and only a membership assertion caught it.

Closing as superseded, not abandoned.