ci: authorize exact Paperclip deploy plans (BLO-19955)

[kkroo]

Thinking Path

• Production built the scoped-concurrency image successfully, but Helm admission rejected its digest because the release workflow did not participate in the durable approval channel.
• The cluster now exposes a least-privilege, exact-name approval ConfigMap and requires a deterministic plan marker before an approved digest may roll out.
• This change makes the protected workflow render one exact Deployment plan, authorize that same plan with trusted tooling and a separate credential, then give Helm the identical marker.

Linked Issues or Issue Description

• Refs Paperclip BLO-19955, BLO-20733, and BLO-20742.
• The failed production attempt is Docker run 30853113490.
• Replaces ci(deploy): authorize the release digest at admission time (BLO-19955) #907 and feat(chart): stamp the approval plan marker on the API pod template (BLO-20733) #973.

What Changed

• Stamp a deterministic approval-plan hash on the API pod template and reject generic pod.annotations injection of the reserved key.
• Vendor the reviewed plan-aware digest approval transaction from onprem-k8s.
• Execute approval tooling from immutable github.workflow_sha, separate from the operator-selected deployment target.
• Confine the release-approver kubeconfig to a fresh mode-0700 temporary directory and unset its raw secret before invoking child tooling.
• Render and validate the exact digest-pinned Deployment, approve that plan, and pass the same marker to Helm.
• Include Helm chart verification in the required aggregate verify check.

Verification

• 37 focused Node tests passed: 28 Helm/chart tests and 9 Docker workflow tests.
• Both reserved annotation collision cases passed.
• Workflow YAML parses with Ruby YAML.
• bash -n scripts/approve-paperclip-api-digest.sh passed.
• git diff --check passed.
• The extracted render step produced one namespaced, digest-pinned stamped Deployment plan.

Risks

• A workflow/chart/protocol mismatch can deny a production rollout. The two-pass render proves stamping changes only the reserved marker, and the approval script server-dry-runs the exact plan before retaining approval.
• Concurrent or failed releases can otherwise evict a still-needed digest. The vendored transaction uses resource-version guards, an in-flight lock, explicit abandonment identity, and a bounded three-entry ring.
• The approver credential has more write privilege than the deploy credential. It is scoped to one ConfigMap name, materialized with mode 0600 under a fresh temporary directory, and removed on every exit path.

Model Used

• OpenAI GPT-5 Codex with tool use and code execution.

Checklist

• I have included a thinking path that traces from project context to this change

• I have specified the model used

• I have linked the owning Paperclip issues

• I have run tests locally and they pass

• I have added or updated tests where applicable

• I have considered and documented risks

• All Paperclip CI gates are green

• Exact-head Ally review has no Critical or Important regression

• I have searched GitHub for duplicate or related PRs and linked them above

[allyblockcast]

🔗 Paperclip issue: BLO-19955
🔗 Paperclip issue: BLO-20733
🔗 Paperclip issue: BLO-20742

[allyblockcast]

🔗 Paperclip issue: BLO-19955
🔗 Paperclip issue: BLO-20733
🔗 Paperclip issue: BLO-20742

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[allyblockcast]

Hey @kkroo! 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

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

Important Issues (3)

• [pr-review-toolkit] .github/workflows/docker.yml:506 — Historical rollbacks now fail before approval. The job intentionally renders the operator-selected target_sha, but every chart revision before this marker was introduced ignores api.approvalPlanSha256 and therefore cannot satisfy this new annotation assertion. An emergency rollback to any older master commit aborts here even though the workflow explicitly supports historical targets. Preserve compatibility by supplying a trusted marker-aware render path for legacy targets, or add an explicit legacy rollback protocol that still binds approval to the exact historical plan.
• [native-codex] .github/workflows/docker.yml:567 — The workflow renders and deploys in configurable NS, but it does not pass that namespace to the approval script. The script consequently defaults PAPERCLIP_DEPLOY_NAMESPACE to paperclip; if PAPERCLIP_NAMESPACE is configured to anything else, it rejects the already-rendered plan and probes the wrong Deployment, so production rollout cannot proceed. Export PAPERCLIP_DEPLOY_NAMESPACE="${NS}" into this invocation and cover a non-default namespace in the workflow test.
• [gstack/review] scripts/approve-paperclip-api-digest.sh:684 — The plan hash is normalized with a server-side full-object kubectl replace --dry-run, while the actual deployment at .github/workflows/docker.yml:601 uses Helm's three-way patch. Live-only drift for fields where the old and new chart agree, such as an HPA-adjusted replica count or a rollout-restart annotation, is preserved by Helm but removed by the replacement probe. The rollout can succeed, yet the resulting canonical live hash can never match the stored server-plan hash, leaving the approval lock stuck and blocking every subsequent release until explicit abandonment. Compute and authorize the object produced by Helm's actual patch semantics, or deploy the exact normalized transaction that was approved.

Strengths

• Release tooling is loaded from immutable github.workflow_sha, independently of the operator-selected deployment target.
• Approver and deploy credentials are separated, narrowly scoped, written under a fresh mode-0700 directory, and removed on exit.
• ConfigMap rotation uses resource-version guards and unique lock ownership, and the chart rejects reserved-marker injection.
• Focused chart tests prove stamping changes only the reserved pod-template annotation, and Helm verification is included in the required aggregate check.

Recommended Action

1. Address the three Important issues before merge.
2. Re-run the focused chart and workflow tests with a legacy target and a non-default namespace.

[kkroo]

Addressed all three exact-head Ally findings in 138cf709621bd0bb2d3583f4509cbc06261b0649:

• historical target charts are stamped by a trusted post-renderer checked out from immutable github.workflow_sha
• the workflow passes PAPERCLIP_DEPLOY_NAMESPACE=${NS} to the approver and tests the non-default namespace wiring
• the approver emits the exact server-normalized Deployment whose hash holds the lock; the workflow applies that object before Helm and reconciles it again after Helm, preventing three-way merge drift from wedging lock completion

The focused 48-test set, YAML/shell checks, and an extracted two-pass digest-pinned render pass locally.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:c5d279c important 1 — fixed — .github/workflows/docker.yml:497 — the trusted post-renderer now stamps the rendered API Deployment independently of the target chart, preserving historical rollback compatibility.
• prior:c5d279c important 2 — fixed — .github/workflows/docker.yml:577 — the approval invocation now passes PAPERCLIP_DEPLOY_NAMESPACE="${NS}", binding probes to the configured deployment namespace.
• prior:c5d279c important 3 — fixed — .github/workflows/docker.yml:625 — the workflow applies the exact server-normalized Deployment before Helm and reconciles it again afterward, so Helm-preserved live drift cannot strand the approval lock.

Looks good. No Critical or Important issues remain on this head.

Strengths

• Release tooling is resolved from immutable github.workflow_sha, separate from the operator-selected historical deployment target.
• The approver credential is isolated from the deploy credential and removed from the environment before child tooling runs.
• Exact-plan admission, namespace propagation, optimistic concurrency, and post-Helm reconciliation now form a consistent fail-closed transaction.
• Helm chart verification is wired into the required aggregate check, including reserved-marker collision coverage.

Recommended Action

1. Merge after the remaining required CI checks pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:c5d279c important 1 — fixed — .github/workflows/docker.yml:497 — the trusted post-renderer now stamps the rendered API Deployment independently of the target chart, preserving historical rollback compatibility.
• prior:c5d279c important 2 — fixed — .github/workflows/docker.yml:577 — the approval invocation now passes PAPERCLIP_DEPLOY_NAMESPACE="${NS}", binding probes to the configured deployment namespace.
• prior:c5d279c important 3 — fixed — .github/workflows/docker.yml:625 — the workflow applies the exact server-normalized Deployment before Helm and reconciles it again afterward, so Helm-preserved live drift cannot strand the approval lock.

Looks good. No Critical or Important issues remain on this head.

Strengths

• Release tooling is resolved from immutable github.workflow_sha, separate from the operator-selected historical deployment target.
• The approver credential is isolated from the deploy credential and removed from the environment before child tooling runs.
• Exact-plan admission, namespace propagation, optimistic concurrency, and post-Helm reconciliation now form a consistent fail-closed transaction.
• Helm chart verification is wired into the required aggregate check, including reserved-marker collision coverage.

Recommended Action

1. Merge after the remaining required CI checks pass.

[allyblockcast]

#995 supersedes my #973 — closing mine. One small gap, and it is the kind that passes while asserting nothing

@kkroo — I opened #973 for the chart half of the BLO-20733 marker before seeing this. #995 does everything #973 does and more, it is human-authored so it can actually clear required review, and it is CLEAN. I am closing #973 as superseded and not pushing anything here — this branch's value is being yours, same as paperclipai#1838 → paperclipai#1874.

Two things you did better than my version, worth saying explicitly: the trusted post-renderer handles legacy charts that predate api.approvalPlanSha256, and vendoring approve-paperclip-api-digest.sh (pinned to onprem-k8s cedac5c6) resolves the public-repo/private-repo question I had flagged as needing a decision — paperclip is public and has no credential to read onprem-k8s, so vendor + pin was the available answer.

I also want to correct myself before it misleads anyone: I first read this diff as missing the pod.annotations collision regressions Ally asked for. It is not — the for (const releaseMarker of [undefined, SAMPLE]) loop covers both the unset and set cases. My grep only matched test( at the start of a line and skipped the loop body.

The gap: the test that proves helm_chart gates cannot detect it not gating

verify step treats Helm chart failure as a required lane failure injects HELM_CHART_RESULT straight into the step's env:

HELM_CHART_RESULT: results.helm_chart ?? "success",

At CI runtime that value does not come from the harness — it comes from ${{ needs.helm_chart.result }}, which renders empty if helm_chart is absent from verify.needs. So the two halves of your pr.yml change fail independently, and only one of them is covered:

mutation	your test suite	consequence in CI
drop [helm_chart]="$HELM_CHART_RESULT" from lane_results	catches it (red)	result collected, ignored
drop helm_chart from verify.needs	passes green	needs.helm_chart.result is empty ⇒ lane silently never gates

I mutation-tested exactly this on my branch, which had both your behavioural test and a membership assertion: removing helm_chart from needs left the behavioural test green and only the membership assertion went red (11 tests → 1 failure). So the second row is measured, not theorised.

That matters here specifically because un-gating is the failure mode #995's pr.yml change exists to prevent, and it would revert silently — the Helm chart lane would keep running and keep reporting, exactly as it does today, while no longer blocking a merge. This area has now shipped tests nothing ran six times; this is the same shape one level up.

Suggestion, not a blocker — one test, no production code:

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(", ")}`,
  );
});

The lazy group cannot cross a job boundary (a sibling job header is 2-space indented and matches neither alternative), so it reads verify's own needs and not the next job's.

One merge-ordering note

#957 (ci(pr): require verify_serialized_server through the verify aggregator, BLO-20869) edits the same verify.needs line to add a different lane, and is awaiting board sign-off 09c52231. It is already DIRTY, so whichever of the two lands second will need that line reconciled — the two additions are complementary, not competing.

Why this is worth merging promptly

onprem-k8s#1874 merged 2026-08-03 and the policy went live on the cluster at 21:22Z (paperclip-api-image-approval, bound via paramRef to paperclip-release-approvals/paperclip-api-approved-images, 2 digests, live prod on c8db29b4…). The vendored script hard-fails exit 2 on any plan whose pod template lacks the marker, and the deployed chart stamps nothing — I confirmed the live pod template carries six operator annotations and no marker. So until this lands, authorizing a fresh digest still means a human editing that ConfigMap by hand. #995 is what closes it.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] .github/workflows/pr.yml:7 — Enabling merge_group makes the existing lockfile guard run without a pull_request payload. On merge-group events, github.head_ref and github.event.pull_request.user.login are empty, so all trusted-automation exemptions evaluate true and the guard runs unconditionally. A permitted Dependabot, allyblockcast[bot], blockcast-ci-packages[bot], or chore/refresh-lockfile change can pass its PR run but then fail policy when queued because the merge-group diff contains pnpm-lock.yaml, preventing the required verify check from succeeding. Gate this guard to pull_request, or derive equivalent exemption evidence for merge-group events and test that behavior rather than only regex-checking the trigger and SHA expressions.

Strengths

• The merge-group base and head SHAs are explicitly selected and reused by each diff-based policy step.
• Full-history checkout preserves the merge-base needed by three-dot diffs.
• The concurrency key separates synthetic merge-group heads while retaining cancellation for superseded runs.
• The aggregate verify job remains present, so merge-queue required-check reporting has a stable target.

Recommended Action

1. Make the lockfile exemption merge-group-aware before merge.
2. Add a regression test for an exempt lockfile PR executing under a merge-group payload.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:479d250 important 1 — fixed — .github/workflows/pr.yml:47 — merge-group runs now skip the PR-only lockfile guard, so absent branch/author fields can no longer erase the trusted automation exemptions and fail an otherwise permitted queue head.

Important Issues (2)

• [gstack/review + native-codex] scripts/stamp-paperclip-api-approval-plan.sh:31 — The Helm post-renderer stamps the pre-approved marker onto any rendered Deployment/paperclip-api without checking that the marker-free Deployment hashes to the approved plan. The approval render uses helm template, while the later helm upgrade render can differ through .Release.IsUpgrade, revision, or cluster capabilities, especially for an operator-selected historical chart. Admission can therefore accept and briefly or permanently apply a plan that was never approved; if Helm fails before the final raw reconciliation, that unapproved plan remains live. Have the post-renderer canonicalize the Deployment with the marker removed and reject unless its SHA-256 equals the supplied marker, with a regression covering an upgrade-time render difference.
• [pr-review-toolkit] scripts/approve-paperclip-api-digest.sh:826 — The script disables its owner-checked EXIT cleanup before writing PAPERCLIP_APPROVED_SERVER_PLAN_OUT. If that write fails, the command exits non-zero but leaves the newly acquired in-flight lock active even though no deployable plan was handed to the workflow, blocking every later release until manual abandonment. Keep cleanup armed until the output write succeeds, then disarm immediately before the successful return.

Strengths

• Release tooling is resolved from immutable github.workflow_sha, independently of the operator-selected deployment target.
• Approver and deploy credentials are separated, narrowly scoped, and handled in a fresh mode-restricted directory.
• Approval-ring mutations use resource-version guards and unique lock ownership, and the chart rejects reserved marker injection.
• Focused chart tests prove that the trusted stamper changes only the reserved pod-template annotation for the tested offline render.

Recommended Action

1. Make the Helm post-renderer verify the exact plan before it stamps the approval marker.
2. Keep lock cleanup armed through successful server-plan output handoff.

[kkroo]

@ally please re-review the current head. The previous exact-head run was terminated by the external lifecycle stale watchdog before posting a verdict.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:96e1e80 important 1 — fixed — scripts/stamp-paperclip-api-approval-plan.sh:58 — the post-renderer now canonicalizes the marker-free API Deployment and rejects it at lines 72-75 unless its SHA-256 equals the approved marker before stamping.
• prior:96e1e80 important 2 — fixed — scripts/approve-paperclip-api-digest.sh:828 — the server-normalized plan is written while EXIT cleanup is still armed, and cleanup is disarmed only after the handoff succeeds at line 835.

Important Issues (3)

• [gstack/review] .github/workflows/docker.yml:631 — the workflow replaces Deployment/paperclip-api before Helm applies the rest of the release. A target chart can make that Deployment depend on a Secret, ConfigMap, ServiceAccount, PVC, or service change from the same revision; the replacement starts that rollout against the old dependency set, and a subsequent Helm failure leaves production on a partial release that Helm never recorded. Keep the pre-Helm phase side-effect-free, let Helm apply the release resources in dependency order, and perform exact-plan reconciliation only after Helm succeeds.
• [gstack/review] .github/workflows/docker.yml:621 — both reconciliations take the server-normalized plan and overwrite only metadata.resourceVersion before a full-object kubectl replace. Because the approved object was built from the rendered chart rather than merged with live top-level metadata, each replace deletes live operator annotations and labels omitted from the chart; the second replace also strips metadata restored or added during Helm. Preserve allowed live top-level metadata when constructing the server plan (excluding release-controlled keys) before hashing and replacing it.
• [native-codex] .github/workflows/docker.yml:649 — the success path checks rollout health but never proves the final live canonical Deployment still equals the server-plan hash held by the approval lock. A concurrent update or a non-deterministic admission mutation between the dry-run and either real replace can yield a healthy but different plan, so this run reports success while the next release cannot retire the lock. Fetch the live Deployment after rollout, canonicalize it with the approval script's exact projection, and require its SHA-256, marker, and images to match the approved server plan before reporting success.

Strengths

• Release tooling is pinned to immutable github.workflow_sha, independently of the operator-selected deployment target.
• The post-renderer now verifies the exact marker-free plan before stamping, including historical target charts.
• Approver and deploy credentials remain separated, and the approver credential is confined to a mode-restricted temporary directory.
• Approval-ring writes use resource-version guards and unique lock ownership, with focused regression coverage for marker drift and failed plan handoff.

Recommended Action

1. Remove the pre-Helm production mutation or make the whole release transaction dependency-safe.
2. Preserve permitted live top-level metadata across exact-plan reconciliation.
3. Verify the final live canonical plan before declaring the deployment successful.

[kkroo]

@ally please re-review exact head a9c45d35aacdeec911f54d25a3496ba57147b01f. This head addresses all three Important findings from your review of 05a59b99d: it removes the pre-Helm Deployment mutation, preserves current non-release top-level metadata while rebuilding release-controlled keys from the approved plan, and verifies the final live canonical server-plan hash plus marker/images before success. Local verification: approval marker 11/11, Docker deploy workflow 9/9, both shell syntax checks, and git diff --check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:05a59b9 important 1 — fixed — .github/workflows/docker.yml:654 — the pre-Helm phase is now side-effect-free, and the exact Deployment reconciliation runs only after Helm has applied same-release dependencies at line 674.
• prior:05a59b9 important 2 — fixed — .github/workflows/docker.yml:630 — reconciliation now merges current live metadata, preserves non-release labels and annotations, and carries the current resourceVersion through line 650.
• prior:05a59b9 important 3 — fixed — .github/workflows/docker.yml:677 — the success path now hashes the approved server plan and final live canonical Deployment, rejects a mismatch, and verifies the marker and digest through line 701.

Important Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] .github/workflows/docker.yml:675 — The 40-minute deploy job can spend up to 30 minutes in helm upgrade --wait and then another 30 minutes in kubectl rollout status after exact-plan reconciliation. A Helm rollout that succeeds late can therefore be terminated by the job deadline before final hash verification, reporting a false failure and leaving the approval lock active for manual recovery.
  • Budget the job timeout for both bounded waits plus cleanup margin, or replace the two independent waits with one shared deadline; extend the timeout regression to account for every sequential rollout wait.

Strengths

• The trusted release tooling is pinned to github.workflow_sha, independently of the operator-selected historical deployment target.
• The approval credential is isolated from the deploy credential and removed from the environment before child tooling runs.
• Exact-plan reconciliation now preserves permitted live metadata and proves the final live canonical Deployment matches the approved server plan.
• Focused tests cover marker integrity, namespace propagation, lock cleanup, and merge-queue verification wiring.

Recommended Action

1. Make the deploy timeout cover the full sequential rollout budget before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:a9c45d3 important 1 — fixed — .github/workflows/docker.yml:261 — the deploy job now budgets 70 minutes for the sequential 30-minute Helm and rollout waits plus ten minutes of surrounding work, and the regression test sums both waits.

Important Issues (1)

• [gstack/review + native-codex] .github/workflows/docker.yml:668 — Exact-plan enforcement still occurs only after helm upgrade --wait has applied and rolled Helm's three-way-merged Deployment. The post-renderer proves the rendered manifest matches the approved marker, but it cannot prove the live object produced by Helm's patch: live-only pod-template drift can be preserved and admitted under that marker. If Helm later times out or fails on this or another release resource, set -e skips reconciliation and leaves the unapproved intermediate Deployment live with the approval lock stranded. Apply the exact server-normalized Deployment as part of the Helm transaction before waiting for its rollout, while still ensuring same-revision dependencies are applied first; add a regression that starts with live-only template drift and exercises Helm failure after the Deployment update.

Strengths

• Approval and deployment tooling remain pinned to immutable github.workflow_sha, independently of the selected target revision.
• The pre-Helm Deployment mutation is gone, and current top-level metadata is carried through server-plan construction and reconciliation.
• Final canonical hash, marker, and image checks make successful completion fail closed against late drift.
• The timeout regression now accounts for every sequential bounded wait.

Recommended Action

1. Prevent Helm from rolling a three-way-merged API Deployment before exact reconciliation is guaranteed.

[kkroo]

@ally please re-review exact head d0f1e8d12749be02f0936a6627da247b9072267e. This head addresses the prior Important finding by running Helm in the background, reconciling the exact server-normalized API Deployment immediately when the approved marker appears, handling the Helm-exit race, and propagating Helm failure only after reconciliation. The new regression asserts marker-bearing live drift is reconciled before a later Helm failure is returned. Please submit an explicit exact-head verdict; merge remains gated on that verdict and green CI.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:318908a important 1 — still-present — .github/workflows/docker.yml:696 — the deterministic marker can already be live on an exact retry, so this loop can reconcile before the current Helm invocation applies its resources and then stop watching.

Important Issues (1)

• [prior:318908a important 1; pr-review-toolkit + gstack/review + native-codex] .github/workflows/docker.yml:696 — An exact retry can mistake a marker left by the prior attempt for evidence that this Helm process applied the Deployment. The loop reconciles immediately and breaks at line 699; Helm can then apply a three-way-merged Deployment and fail, while api_plan_reconciled=yes suppresses the post-exit reconciliation at line 710. The unapproved merged object can remain live, so the prior failure-stranding defect is not fully resolved.
  • Reconcile and canonically verify after wait regardless of an earlier observation, or require evidence that this Helm invocation advanced the Deployment before setting api_plan_reconciled. Add a behavioral regression with the same marker already live before Helm starts.

Strengths

• The background Helm wait lets the workflow react to the Deployment before unrelated resource failures are propagated.
• The EXIT trap prevents an orphaned Helm process when the workflow step exits.
• Exact-plan approval, immutable release tooling, credential separation, and final canonical verification remain fail-closed on the normal success path.

Recommended Action

1. Make exact retries distinguish this Helm invocation from a pre-existing identical marker, then re-run the focused workflow tests.

[kkroo]

@ally please re-review exact head 72e0f23970929f707726e0c6dad8435c7aadce02. This head fixes the exact-retry gap by unconditionally re-reading the live marker and reconciling the server-normalized Deployment after Helm exits, even if the polling loop already reconciled a pre-existing identical marker. Focused verification: approval marker 11/11, Docker deploy workflow 10/10, both shell syntax checks, git diff --check, and Graphify refresh all pass. Merge remains gated on an explicit exact-head verdict and green CI.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:d0f1e8d important 1 — fixed — .github/workflows/docker.yml:717 — the workflow now reconciles the marker-bearing Deployment unconditionally after the background Helm process exits, so an identical marker left by a prior attempt cannot suppress the post-Helm exact-plan reconciliation.

Important Issues (2)

• [pr-review-toolkit + gstack/review] .github/workflows/docker.yml:588 — the workflow rejects every approved create plan because it requires metadata.resourceVersion, although scripts/approve-paperclip-api-digest.sh:663-708 deliberately uses server-dry-run create when Deployment/paperclip-api is absent. Recovery after an accidental Deployment deletion therefore acquires and preserves the approval lock, then exits here before Helm can recreate the Deployment, blocking subsequent releases until explicit lock abandonment.
  • Accept a missing resourceVersion for create plans and retain the requirement only for update plans; the reconciliation helper can obtain the newly created live object's version after Helm applies it. Add a regression for an absent API Deployment.
• [gstack/review + native-codex] .github/workflows/docker.yml:652 — reconcile_approved_api_plan performs a GET-derived full-object kubectl replace once, with no retry for a stale resourceVersion. Deployment controller status updates advance that version during rollout, so a normal update between lines 621 and 652 produces a conflict, aborts the step under set -e, kills the background Helm process, and leaves the release lock active even though the approved plan is otherwise valid.
  • Retry conflicts with a fresh GET/rebuild/replace cycle and bounded backoff, while still failing closed on non-conflict errors. Cover a first-attempt conflict followed by success.

Strengths

• Release tooling is pinned to immutable github.workflow_sha, independently of the operator-selected deployment target.
• The App and deploy credentials are separated, and the higher-privilege approver credential is confined to a fresh mode-restricted directory.
• The trusted post-renderer verifies the marker-free Deployment hash before stamping, including historical chart targets.
• The current head closes the exact-retry race by reconciling again after Helm exits, and final canonical hash, marker, and image checks remain fail-closed.

Recommended Action

1. Support the approver's create-plan path through the workflow handoff.
2. Make exact Deployment reconciliation resilient to ordinary resource-version conflicts.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:72e0f23 important 1 — still-present — .github/workflows/docker.yml:640 — the handoff now accepts a create plan without resourceVersion, but reconciliation starts from the server-dry-run plan metadata and merges live metadata only after deleting the live uid; a provisional UID returned by dry-run create therefore survives into the full-object replace and does not match the Deployment Helm actually created.
• prior:72e0f23 important 2 — fixed — .github/workflows/docker.yml:624 — reconciliation now performs up to five fresh GET/rebuild/replace attempts, retries recognized resource-version conflicts with bounded backoff, and fails closed on non-conflict errors or exhaustion.

Important Issues (1)

• [prior:72e0f23 important 1; pr-review-toolkit + native-codex] .github/workflows/docker.yml:640 — Approved create plans can still fail during reconciliation. A server-side dry-run create may assign provisional identity fields such as metadata.uid; after Helm creates the real Deployment, this merge retains $plan.metadata.uid because uid is deleted from $live_metadata. kubectl replace then rejects the immutable UID mismatch as a non-conflict error, leaving the release failed with its approval lock active.
  • Strip server-generated identity fields from the approved plan before merging, or explicitly set the reconciled object's uid, creationTimestamp, and resourceVersion from the current live Deployment. Add a behavioral create-path regression that feeds a dry-run plan UID different from the newly created live UID and proves reconciliation succeeds.

Strengths

• Conflict retries re-read and rebuild the exact plan on every attempt rather than retrying a stale object.
• Retry classification remains fail-closed for non-conflict errors and has a bounded attempt count.
• The handoff validation now distinguishes create plans from malformed update plans without weakening digest or namespace checks.

Recommended Action

1. Preserve the newly created Deployment's live identity fields during exact-plan reconciliation before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:72e0f23 important 1 — fixed — .github/workflows/docker.yml:634 — reconciliation now removes provisional server-dry-run identity fields from the approved create plan, then overlays the real live Deployment metadata and resource version before replacement.

Looks good. No Critical or Important issues remain on this head.

Suggestions (1)

• [pr-review-toolkit] scripts/check-docker-deploy-timeout.test.js:198 — Consider supplementing the structural regex assertions with a fixture-based jq execution test that proves a provisional plan UID is replaced by the newly created live UID.

Strengths

• Create recovery now preserves the live Deployment identity while keeping release-controlled labels and annotations bound to the approved plan.
• Conflict retries rebuild from a fresh GET and remain bounded and fail-closed for non-conflict errors.
• Post-Helm reconciliation and final canonical hashing verify the live Deployment against the exact approved server plan.
• Trusted release tooling and approval credentials remain immutable, separated, and narrowly handled.

Recommended Action

1. Merge after the remaining required CI checks pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:72e0f23 important 1 — fixed — .github/workflows/docker.yml:634 — reconciliation now removes provisional server-dry-run identity fields from the approved create plan, then overlays the real live Deployment metadata and resource version before replacement.

Looks good. No Critical or Important issues remain on this head.

Suggestions (1)

• [pr-review-toolkit] scripts/check-docker-deploy-timeout.test.js:198 — Consider supplementing the structural regex assertions with a fixture-based jq execution test that proves a provisional plan UID is replaced by the newly created live UID.

Strengths

• Create recovery now preserves the live Deployment identity while keeping release-controlled labels and annotations bound to the approved plan.
• Conflict retries rebuild from a fresh GET and remain bounded and fail-closed for non-conflict errors.
• Post-Helm reconciliation and final canonical hashing verify the live Deployment against the exact approved server plan.
• Trusted release tooling and approval credentials remain immutable, separated, and narrowly handled.

Recommended Action

1. Merge after the remaining required CI checks pass.

[kkroo]

@ally please re-review exact head 0657fbaf59dddf1ff50d41c5383df4614ba6e4c4. This head preserves uid, creationTimestamp, and resourceVersion from the live Deployment during approved create-plan reconciliation, addressing the provisional dry-run UID mismatch from your review of 8467cef2. Focused verification: approval marker 11/11, Docker deploy workflow 11/11, both shell syntax checks, git diff --check, and Graphify refresh pass. Merge remains gated on an explicit exact-head verdict and green CI.

[kkroo]

@eyad-hussein @MohamedElmdary production image build for merged SHA f819d716e947f2d179fa5ba21c1e7bb0ce763f11 is complete and waiting on the paperclip-production environment gate. Please approve the deploy job: https://github.com/Blockcast/paperclip/actions/runs/30887245919