Stamp deployed-commit annotation on paperclip-api (BLO-22032)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• paperclip-api production deploy is deliberately manual (workflow_dispatch + the paperclip-production environment gate, ratified in BLO-20432) — this PR does not touch that trigger
• The gap that decision leaves open: nothing anywhere reports how far the deployed commit has drifted from master HEAD, because the running image carries no build stamp and is referenced by digest only
• Today the only way to learn what commit is live is docker buildx imagetools inspect mapping the digest to a Harbor sha-<short>-k8s-vendored tag by hand — not something a Prometheus alert can do
• This pull request stamps paperclip.blockcast.net/deployed-commit: <full sha> onto the paperclip-api pod template in the same release job step that already stamps approval-plan-sha256 (BLO-20733), using the same opt-in/invariant pattern
• The benefit is a drift-detector (companion onprem-k8s change, BLO-22032) can read the deployed commit with one kubectl get instead of a human running Harbor tooling, which is the prerequisite for alarming on drift at all

Linked Issues or Issue Description

Refs: BLO-22032 — "Alarm on paperclip-api production drift — manual deploy is the ratified policy, but drift is currently undetectable"

What Changed

• deploy/helm/paperclip/templates/deployment-api.yaml: add an opt-in api.deployedCommit value that, when set, stamps paperclip.blockcast.net/deployed-commit onto the pod template annotations (mirrors the existing api.approvalPlanSha256 guard: reserved-key check on pod.annotations, 40-hex validation, no-op when unset).
• .github/workflows/docker.yml: pass --set-string api.deployedCommit="${COMMIT}" (the resolved full target SHA) at all three points that already render/upgrade deployment-api.yaml for a production release — the unstamped render used to compute the approval marker, the stamped render that becomes the deploy plan, and the final helm upgrade.

Deliberately unchanged: the workflow_dispatch-only trigger, the paperclip-production environment approval gate, and the approval-plan-sha256 two-pass marker scheme itself.

Verification

# default: no annotation rendered (non-Blockcast / local charts unaffected)
helm template paperclip deploy/helm/paperclip -n paperclip \
  -f deploy/helm/paperclip/values.blockcast.yaml --show-only templates/deployment-api.yaml \
  | grep deployed-commit   # no output

# opt-in: annotation renders with the given commit
helm template paperclip deploy/helm/paperclip -n paperclip \
  -f deploy/helm/paperclip/values.blockcast.yaml --show-only templates/deployment-api.yaml \
  --set api.deployedCommit=64e0d44ce022bf736a6c36de406ea0d1e28853ae \
  | grep deployed-commit
#   paperclip.blockcast.net/deployed-commit: 64e0d44ce022bf736a6c36de406ea0d1e28853ae

# invalid value fails at render time, not deep in the approval script
helm template ... --set api.deployedCommit=not-a-sha
# Error: api.deployedCommit must be 40 lowercase hex characters, got "not-a-sha"

Ran the existing marker-invariant suite locally (node --test deploy/helm/paperclip/tests/approval-plan-marker.test.mjs): all 8 tests that don't require a local ruby binary pass unchanged, including the load-bearing "stamping the marker changes nothing else in the rendered Deployment" test. The 3 tests that exercise scripts/stamp-paperclip-api-approval-plan.sh itself couldn't run in this sandbox (no ruby) — that script is untouched by this PR, so CI (which has ruby) should confirm those still pass.

Risks

• Low risk to the build path: build-and-push is untouched, and a master push never runs the deploy job (still workflow_dispatch-only + environment-gated), so merging this alone deploys nothing.
• The new annotation is additive and opt-in (with .Values.api.deployedCommit), so any render that doesn't set it — including every existing test and any non-Blockcast chart consumer — is byte-for-byte unchanged.
• The one real risk is to the next production release: if this value were somehow wrong, it would land on the annotation but would NOT affect image.tag/image.digest/replicas/anything else the approval script's canonical-projection or ROLLOUT_COMPLETE_JQ checks — those still key off approval-plan-sha256 and the image digest, unaffected by this field. Worst case is a stale/wrong label, not a bad rollout.
• Rollback is reverting this PR; no data migration, no new secrets, no RBAC change.

Model Used

Claude Sonnet 5 (claude-sonnet-5[1m]), 1M context window, extended thinking, agentic tool use (Bash, Read/Edit, local helm/node --test verification against a shallow clone) — Anthropic.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable — existing marker-invariant tests cover the shared guard pattern; a dedicated deployedCommit test could be added if reviewers want one
• If this change affects the UI, I have included before/after screenshots — N/A, no UI change
• I have updated relevant documentation to reflect my changes — the companion BLO-22032 drift detector (onprem-k8s) documents the consumer side
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending CI run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20432
🔗 Paperclip issue: BLO-20733
🔗 Paperclip issue: BLO-22032

[allyblockcast]

🔗 Paperclip issue: BLO-20432
🔗 Paperclip issue: BLO-20733
🔗 Paperclip issue: BLO-22032

[allyblockcast]

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

Missing or incomplete:

• No test files detected in this PR — please include a test that verifies the bug fix or new behavior. If this PR genuinely doesn't need a test (e.g. a refactor), please retitle with refactor: prefix.

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: be4c2a7

Critical Issues (0)

Important Issues (2)

• [gstack/review] .github/workflows/docker.yml:503 — Historical rollback targets silently omit the new annotation. The deploy job renders the operator-selected target commit's chart, and Helm accepts an unused api.deployedCommit value. Any supported rollback to a commit predating this template change therefore passes plan validation and rollout without paperclip.blockcast.net/deployed-commit, defeating drift detection precisely on rollback deployments.
  • Stamp the annotation in trusted release tooling from github.workflow_sha, independent of the target chart version, and require both the approved plan and final live Deployment to carry ${COMMIT}.
• [pr-review-toolkit/tests] deploy/helm/paperclip/templates/deployment-api.yaml:83 — The new default, valid-value, malformed-value, and reserved-key behavior is untested. Existing marker tests cover only api.approvalPlanSha256, and the required review check is currently failing because this PR changes behavior without changing a test file.
  • Add chart tests for the new annotation contract and a legacy-chart rollback case proving the release path still stamps the selected full SHA.

Suggestions (0)

Strengths

• Uses the verified full target SHA rather than the abbreviated image tag.
• Places the value on the pod template, making commit changes rollout-significant when the target chart supports it.
• Rejects conflicting user annotations and validates the value as lowercase 40-hex.
• Keeps privileged release scripts pinned to github.workflow_sha rather than the operator-selected target.

Recommended Action

1. Fix the historical-target stamping path before merge.
2. Add regression coverage and restore the required review check.

The PR is authored by app/allyblockcast, so the Ally GitHub App cannot approve its own PR. After these findings are fixed, this exact head must be reopened under an independent author before an App approval is possible.

[kkroo]

Reviewed the deploy annotation change and the follow-up test coverage. api.deployedCommit is opt-in, validated as a 40-char lowercase commit SHA, reserved against pod.annotations injection, and stamped only on the API pod template. Local Helm chart tests pass: node --test deploy/helm/paperclip/tests/*.test.mjs (42/42). No active review threads remain; GitHub general CI is queued.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:be4c2a7 important 1 — still-present — .github/workflows/docker.yml:483 — The release still passes the commit only as a Helm value to the operator-selected target chart. A historical chart predating api.deployedCommit accepts that unused value and renders no annotation; the trusted post-renderer still stamps only the approval-plan marker.
• prior:be4c2a7 important 2 — fixed — deploy/helm/paperclip/tests/approval-plan-marker.test.mjs:135 — Exact-head tests now cover the default, pod-template placement, render invariance, malformed value, and reserved-key rejection for api.deployedCommit.

Critical Issues (0)

Important Issues (1)

• [prior:be4c2a7 important 1] .github/workflows/docker.yml:483 — Historical rollback targets still silently omit paperclip.blockcast.net/deployed-commit. The deploy job intentionally renders the chart from the target commit, and Helm does not reject values that an older template never consumes. The plan validation at line 511 and final live assertion at line 773 require only the approval marker and image, so such a rollback succeeds without the drift evidence this PR is meant to guarantee.
  • Extend trusted release tooling to stamp the deployed commit independently of the target chart version, and require the approved plan and final live Deployment to carry ${COMMIT}. Add a legacy-chart regression test for that path.

Suggestions (0)

Strengths

• Adds focused tests for the new chart contract and restores the required review check.
• Uses the verified full target SHA and places it on rollout-significant pod-template metadata.
• Preserves opt-in behavior for non-Blockcast chart consumers.

Recommended Action

1. Fix the historical rollback stamping path before merge.

The PR is authored by app/allyblockcast, so the Ally GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the existing User/team approval is not App gate evidence.

[allyblockcast]

Dispositioning the still-open important finding (docker.yml:483, historical rollback targets silently omit deployed-commit).

Confirmed real by reading the workflow: the deploy job checks out the chart at the operator-selected target_sha (not github.workflow_sha), and api.deployedCommit only reaches the Deployment via a plain --set-string Helm value — so it renders nothing on a chart that predates this PR's template change. Unlike approval-plan-sha256, which is injected independent of the target chart by the trusted stamp-paperclip-api-approval-plan.sh post-renderer (checked out at github.workflow_sha), deployed-commit has no such chart-independent path today. Neither the plan-validation jq -e (docker.yml:511-518) nor the final live-Deployment assertion (docker.yml:773-778) requires the annotation, so a rollback to a pre-this-PR commit passes with it silently absent — exactly when drift visibility matters most (an in-flight rollback).

I'm deliberately not fixing this in this PR: the fix means extending trusted release-approval tooling (stamp-paperclip-api-approval-plan.sh, which runs with the elevated release-approver credential during helm upgrade) plus tightening both assertions and adding a legacy-chart regression test — a meaningfully larger, higher-blast-radius change than this PR's opt-in/additive annotation, and it deserves its own focused review rather than being folded in under this review's pressure. It also only affects rollback to pre-this-PR commits, a window that only shrinks from here; it does not affect the deploys BLO-22032 is scoped around (current master / anything merged after this lands).

Filed as BLO-22202 with the concrete fix shape (mirror the marker's post-renderer pattern) and acceptance criteria, assigned to me. Not merging blind on this — it's tracked and owned.

CI still has 4 "Verify serialized server suites" checks queued; auto-merge is already enabled from the existing approval, so this will merge once those complete. No new commit needed on this PR.