feat(deploy): hand off the in-flight lock owner and retire it when helm never ran (BLO-31598, BLO-31666) by allyblockcast[bot] · Pull Request #1646 · Blockcast/paperclip

allyblockcast · 2026-09-04T13:20:20Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

Production deploys run through docker.yml's deploy job, which authorizes one exact image digest at admission time before rolling it out

That approval takes an in-flight lock on the approval ring, and only a landing rollout releases it — so a job that dies between the approval and helm upgrade strands a lock for a digest that was never applied

Nothing clears a stranded lock, and the symptom surfaces as a confusing admission refusal on somebody else's unrelated deploy. It has already happened: run 33763503004 died in the pending-migration pre-flight, run 33810092507 was then refused against the lock it left behind, and recovery took hours

ci(deploy): make the abandon-in-flight escape hatch reachable from docker.yml #1636 shipped an escape hatch for it, but that is recovery and needs a human who knows this failure mode exists

This pull request adds the prevention: it hands the minted lock's owner to the workflow (the former feat(deploy): hand the minted in-flight lock owner to the release workflow (BLO-31598) #1638) and adds a cleanup step that retires the lock this job took, and only when helm upgrade never executed

The benefit is that the next unlucky deploy unwedges itself, with no operator action and no window in which an unrelated release gets refused

Linked Issues or Issue Description

Fixes: BLO-31666 (Deploy job should retire the in-flight approval lock it took when it fails before helm upgrade) — split out of BLO-31598 ask 3
Refs BLO-31598 (the wedge itself; the escape hatch half shipped in ci(deploy): make the abandon-in-flight escape hatch reachable from docker.yml #1636)
Refs BLO-31254 (the pre-flight budget whose old 180s bound triggered the stranding; already widened)
Supersedes feat(deploy): hand the minted in-flight lock owner to the release workflow (BLO-31598) #1638, whose three commits are the first three here.

Why this absorbed #1638 rather than stacking on it. I opened this stacked on ci/blo-31598-lock-owner-handoff first, then found that was unverifiable: pr.yml triggers on pull_request: branches: [master] only, so a PR based on any other branch gets no CI at all — the new test would never have run, which hollows out this issue's acceptance criterion. Three further facts settled it:

feat(deploy): hand the minted in-flight lock owner to the release workflow (BLO-31598) #1638 is inert alone. It adds PAPERCLIP_APPROVAL_LOCK_OWNER_OUT, which no caller set, so emit_lock_owner returned early on every production path. It had no independent shipping value and could not be verified end to end.
It could not have merged as it stood. master requires up-to-date branches and feat(deploy): hand the minted in-flight lock owner to the release workflow (BLO-31598) #1638 was 9 commits behind, so its green verify at 5324a1a49 was not mergeable state and a rebase was owed regardless.
Nothing was lost. All three Ally reviews on feat(deploy): hand the minted in-flight lock owner to the release workflow (BLO-31598) #1638 were COMMENTED, there were zero approvals, and review/ally-comment was already red — and any rebase would have staled those reviews anyway.

I did not merge #1638 past its red gate to unblock myself, which is what the operator instruction on BLO-31666 forbids. Its thread is left intact and closed as superseded, and the safety argument here spans both halves anyway, so reviewing them as one artifact is easier to get right than reviewing either alone.

What Changed

The safety condition is the whole design. "The job failed" is not sufficient grounds to retire the lock — a failure during or after helm upgrade may leave a rollout genuinely in flight, and retiring that lock would let a competing release rotate the ring underneath a landing one, which is precisely what the lock exists to prevent.

docker.yml — approval step: wires PAPERCLIP_APPROVAL_LOCK_OWNER_OUT and publishes lock_digest + lock_owner as step outputs. Published before the approved-plan verification, not after: the stranding window opens the instant the approval script exits 0, and that includes the rest of that step, so publishing later would leave a jq failure there stranding a lock the cleanup cannot name.
docker.yml — helm step: gains id: helm and writes a started=true output as its literal first statement.
docker.yml — new cleanup step (if: always(), so it also runs on the cancellation a job timeout produces). It reads two independent signals, because neither is sufficient alone:
- the helm step's start marker — positive evidence, but can be lost if the runner is killed before outputs are collected;
- the helm step's conclusion — survives that, but a never-started step may read as skipped or be absent from the steps context entirely.
Retirement requires both to say the step never ran. The conclusion test is an allow-list of not-run values (skipped, empty) rather than a deny-list, so an unrecognised value preserves the lock — the direction that fails safely.
approve-paperclip-api-digest.sh — retire-only mode (PAPERCLIP_APPROVAL_RETIRE_IN_FLIGHT_ONLY=1). Retirement goes through the existing script rather than a second copy of the protocol. It approves nothing (no plan, no deploy credential, no admission probe), takes no positional arguments so the digest cannot be supplied twice and disagree with itself, matches on digest AND owner together so it can never retire a lock another run took, and exits 0 without writing when there is no such lock.
approve-paperclip-api-digest.sh — shared clearing write. The lock is seven annotations and that set has grown three times already. Two copies would drift on the fourth, and a copy that forgets the digest key leaves a partial lock that still wedges the channel while reporting success — worse than not running. Now written once and used by both retirement paths.
pr.yml: registers the new test in policy, next to its BLO-31598 sibling.

The first three commits are #1638 unchanged: they add the PAPERCLIP_APPROVAL_LOCK_OWNER_OUT handoff, validate its path before the ring write, and correct a comment that wrongly claimed the emit abort was not self-healing. The handoff writes the owner only for a lock this invocation minted — an adopted lock is left unnamed because its rollout may still be running — so an absent file is itself the "nothing to retire" signal the cleanup step needs.

Verification

policy executed in CI on this PR — run 33895164749, policy: success, at head 106779ef7. Ally's Important finding on the previous head was that it never had: a synchronize fired while the base was still ci/blo-31598-lock-owner-handoff and was filtered by branches: [master], then the retarget fired edited, which pull_request does not listen for — so no run was ever created and the first execution of this test would have been inside the merge queue. The rebase force-push fired a real synchronize against master and produced the run above. A fresh run is in flight at bac838294 for the tests added since.

Local, at bac838294:

node --test ./scripts/approve-paperclip-api-digest.test.js               # 40 pass
node --test ./scripts/check-docker-retire-in-flight-lock.test.js         # 13 pass
node --test ./scripts/check-docker-abandon-in-flight-inputs.test.js      #  8 pass
node --test ./scripts/check-docker-deploy-timeout.test.js                # 15 pass
node --test ./scripts/check-docker-two-tier-convergence.test.js          # 10 pass
node --test ./scripts/check-pending-migration-preflight.test.js          #  9 pass
node --test ./scripts/check-pending-migration-preflight-phases.test.js   # (in the 117)
node --test ./scripts/guard-pending-deploy.test.js                       # (in the 117)
                                                                         # 117 total, 0 fail

The tests are behavioral, not render-level, because #1636's review showed two presence-only assertions of mine passing against mutated code (an inverted-polarity guard, and a condition gutted to if false; then). So they extract the cleanup step's real shell out of docker.yml and run the full signal matrix through bash, and run the real approval script in retire-only mode against a stubbed kubectl — real bash, real jq, only the cluster faked.

Mutation-verified against 27 mutations of the shipping code; each fails at least one test:

#	mutation	caught by
1–2	helm-conclusion polarity inverted / gutted to `false`	2 tests
3	minted-lock condition gutted	1
4	`skipped` widened to `= success`	2
5	`id: helm` removed	1
6	owner half dropped from the workflow guard	1
7–8	each half of the digest+owner pairing rule dropped	1 each
9–10	digest / owner key omitted from the clear list	1 each
11	"nothing to retire" turned into a hard error	1
12	unreadable ConfigMap swallowed as success	1
13	approval ring dropped along with the lock	1
A–B	start-marker check inverted / gutted	2 each
C	conclusion allow-list widened to `*`	2
D	empty conclusion no longer retires	1
E	`cancelled` wrongly treated as not-run	1
F	minted-lock check gutted	1
G–H	start marker removed / moved after the first failable statement	1 each
I	`HELM_STARTED` wiring removed	1
J	every write error treated as retriable (a denial burns the retry budget)	1
K	no write error treated as retriable (a lost race is never retried)	1
L	`2>&1` dropped from the tee pipeline (recovery command lost from the log)	1
M	summary heading polarity inverted	2
N	`::error::` annotation removed from the failure path	1
O	`if !` wrapper dropped, so `set -e` aborts before summary and annotation	1
P	`exit 1` dropped, so a wedged ring reports green	1

J and K are a deliberate mutual control pair: forcing every error retriable fails only the denial test, forcing none retriable fails only the conflict test. Both assert on attempt counts, because a mutation collapsing either case into the other still produces a red exit and a plausible message — the count is the only discriminator.

L–P cover the hand-back path, which the previous head shipped render-untested: those visibility fixes landed entirely outside the extracted RETIRE_IN_FLIGHT_LOCK_GUARD region, so a second marker pair (RETIRE_IN_FLIGHT_LOCK_HANDBACK) now delimits the invocation and its reporting and the tests execute it on both outcomes.

Also confirmed by hand against a stubbed cluster that a matching pair clears all seven lock annotations, leaves .data (the approval ring) byte-identical, preserves unrelated annotations, and carries resourceVersion into the replace so the write stays optimistic-concurrency guarded.

Re-verified after the rebase onto fae70d75a, since master's BLO-28483 commits (04d1ff100, b067b9429, fae70d75a) touch the same script: all seven LOCK_*_ANNOTATION keys are still covered by the shared clear list, and retire-only mode still returns before LOCK_OWNER_ID is minted (:523) and before live_running_digest (:933) — the structural property Ally verified at the old head. Both workflows re-parse as valid YAML (js-yaml), with both marker pairs inside the correct step's run scalar, and bash -n is clean on the script.

Risks

The dangerous direction is retiring a lock whose rollout is live, which would let a competing release rotate the ring underneath a landing one. Every ambiguous case is therefore resolved toward preserving the lock: unknown conclusion, lost marker, contradictory signals, adopted (non-minted) lock. The cost of preserving wrongly is a refusal an operator can retire by hand with the ci(deploy): make the abandon-in-flight escape hatch reachable from docker.yml #1636 hatch; the cost of retiring wrongly is a half-rotated ring under a live rollout.
Not exercised end-to-end on a real deploy, because the failure it handles requires a deploy that dies between two specific steps against the live approver credential. That is the gap the behavioral + mutation coverage above is standing in for, and it is why the guard reads two signals instead of trusting my reading of GitHub's steps.<id>.conclusion semantics for a step that never ran.
Inert on the success path. A green deploy sees helm conclude success with the marker set, so the cleanup no-ops before touching any credential — the lock stays live for the next release to advance past, exactly as before. Nothing about a healthy deploy changes.
A malformed lock owner is warned about rather than fatal, deliberately: failing there would strand that very lock and cancel a deploy that would otherwise have consumed it legitimately.
policy is not merge-gating (verify's needs omits it, confirmed independently by Ally on ci(deploy): make the abandon-in-flight escape hatch reachable from docker.yml #1636), so this test reports red without blocking. That is worth its own decision and is not a reason to skip the test — noted here rather than silently fixed in this PR.

Model Used

Claude Opus 4.5 (claude-opus-4-5), 1M context, extended thinking, with tool use and code execution — via Claude Code as the Paperclip Release Engineer agent.

Checklist

allyblockcast · 2026-09-04T13:20:26Z

🔗 Paperclip issue: BLO-31254
🔗 Paperclip issue: BLO-31666
🔗 Paperclip issue: BLO-31598

allyblockcast

Ally — Consolidated PR Review

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

The safety design here is right, and I checked the parts that would break it rather than taking the comments' word for it. Specifically verified against the tree at this head: id: approval, id: helm and id: tooling all exist in the same job as the cleanup step, so none of the three steps.* reads render empty; neither docker.yml nor the cleanup step overrides PAPERCLIP_APPROVAL_NAMESPACE/PAPERCLIP_APPROVAL_CONFIGMAP, so the approval and the retirement address the same ConfigMap by the same script defaults; and retire-only mode returns before LOCK_OWNER_ID is minted at scripts/approve-paperclip-api-digest.sh:476, so its kubectl/jq dependency check is complete and it correctly needs no ruby.

The digest+owner pairing also holds up against the concurrency case I tried hardest to break: if another run adopts this lock via the exact-retry transfer at scripts/approve-paperclip-api-digest.sh:782-786, the owner annotation changes, so this job's cleanup no-ops instead of retiring a lock whose rollout is live. That is the correct outcome and it falls out of the pairing rather than needing a special case.

Critical Issues (0)

Important Issues (1)

[gstack/review] .github/workflows/pr.yml:138 — The new policy test has never executed in CI on this PR, so this issue's acceptance criterion is not yet met at this head. pr.yml has zero runs on ci/blo-31666-retire-unused-in-flight-lock (actions/workflows/pr.yml/runs?branch=... → empty), and the only workflow at this head is commitperclip PR Review via pull_request_target. The PR timeline explains why: head_ref_force_pushed at 13:27:33Z fired synchronize while the base was still ci/blo-31598-lock-owner-handoff, which branches: [master] (pr.yml:5-6) correctly filtered out; base_ref_changed at 13:28:56Z then retargeted to master, but that fires edited, which is not in pull_request's default types (opened, synchronize, reopened). No run was ever created. This is the same hollowing-out the PR body gives as its reason for absorbing #1638 — recurring here for a different mechanism, and silently. Note it is not a path to landing untested code: master's only rule is merge_queue (ruleset 20487141), and pr.yml triggers on merge_group: checks_requested, so policy does run in the queue and a red result ejects the candidate. That caps the blast radius, but it also means the first execution of a test whose whole purpose is proving this guard behaves would be inside the merge queue.
- Fire a synchronize or reopened event now that the base is master — an empty commit, a re-push of the same tree, or close/reopen — and confirm a pr.yml run appears with policy green before queuing. Worth re-reading the Verification section afterwards too: it currently lists only local node --test results, which is exactly the evidence the #1638 rationale treats as insufficient.

Suggestions (3)

[pr-review-toolkit/errors] .github/workflows/docker.yml:1319-1326 — On the one path that actually needs a human, the recovery instructions are the least visible thing in the run. When retire-only exhausts its three attempts it exits 1 with its guidance on stderr (scripts/approve-paperclip-api-digest.sh:279-283), which | tee "${approver_dir}/retire.log" does not capture, and set -e then skips the $GITHUB_STEP_SUMMARY block at :1321-1326 entirely. Unlike the credential branch immediately above it at :1294, that path also emits no ::error:: annotation. So a lock the automation tried and failed to clear yields a red step with no annotation and an empty summary. Consider |& tee (or 2>&1 |) plus writing the summary from a trap ... EXIT, and an ::error:: on the exhaustion path for parity with :1294.
[native-codex] scripts/approve-paperclip-api-digest.sh:113-126 — clear_in_flight_lock discards stderr (2>/dev/null 2>&1), so the retire-only retry loop at :270-276 cannot tell a retriable 409 from a non-retriable RBAC denial: an approver Role missing update burns 1+2+3s and reports the same generic "could not retire". The rotation write at :838-839 already has the better pattern — capture to $replace_err, then grep -qiE 'conflict|modified|latest version' to decide whether retrying is even meaningful. Extracting the write into a shared helper was the right call for the annotation-set drift argument; it just inherited the weaker error handling of the two callers rather than the stronger.
[pr-review-toolkit/comments] scripts/approve-paperclip-api-digest.sh:832 — The adopted-lock residual is reasoned correctly but its operator consequence isn't written down where an operator will look. Because an exact retry rewrites the owner annotation (:782-786) while deliberately leaving lock_owner_is_ours empty, a lock adopted after a first cleanup failed can never be auto-retired — and the owner printed in the first run's log is now stale, so the escape hatch will refuse it. The current owner is only recoverable from the most recent approval step's epilogue at :1219. One sentence to that effect in the retire-only usage text or the ::error:: at docker.yml:1294 would save the next operator a confusing hatch refusal.

Strengths

The allow-list on HELM_CONCLUSION (docker.yml:1281-1288) is the right polarity: an unrecognised value preserves the lock. Pairing it with the independent start marker, and reading the marker first because it is positive evidence, covers both the died-on-first-assertion case and the runner-killed-before-outputs case that neither signal handles alone.
Publishing lock_owner/lock_digest before the approved-plan verification is a real correctness property, not tidiness, and check-docker-retire-in-flight-lock.test.js:462-482 pins it with unique-anchor assertions on both sides so the ordering test can't silently start comparing the wrong occurrences.
The tests are genuinely behavioral, and the marker extraction fails loud: extractGuard asserts both BEGIN/END markers resolve (:59-61) before running the real shell through bash, and uses the absence of REACHED_RETIRE as the preserve-assertion. That answers the presence-only failure mode #1636's review surfaced instead of restating it.
Retire-only mode taking no positional arguments is a good call — it makes "the digest disagrees with itself" unrepresentable rather than validated after the fact.

Recommended Action

No Critical issues.
Get a pr.yml run with policy green on this head before queuing, and update Verification to cite it — that is the acceptance criterion, and it is the only Important item.
Consider the three Suggestions opportunistically; the first (visibility on the failed-retirement path) has the most operational value, since it is the path where the automation is handing the problem back to a human.

…kflow (BLO-31598) An approval that succeeds deliberately leaves its in-flight lock live, for the next release to retire after observing that plan marker roll out. The script therefore cannot self-heal the case that wedged production: approval exits 0, the job then dies at the pending-migration pre-flight, helm never runs, and the lock names a rollout that can never happen. Run 33763503004 left exactly that, and every later deploy refused against it. Retiring such a lock needs its 64-hex owner, which existed only as prose on stdout, so no workflow step could name it. Emit it to PAPERCLIP_APPROVAL_LOCK_OWNER_OUT instead, mirroring the existing PAPERCLIP_APPROVED_SERVER_PLAN_OUT handoff. Written only for a lock this invocation MINTED. A lock adopted from an earlier attempt belongs to a rollout that may still be running, and retiring it would reopen the approval ring underneath that rollout -- the distinction lock_preserve_on_failure already encodes at the owner transfer. An absent file means "no lock this invocation is entitled to abandon", which fails safe. Emitted only after the kubectl replace lands; before it, no lock with this owner exists, and naming one would send a cleanup step at another process's transaction. Inert until a caller sets the variable. The docker.yml consumer is deliberately not in this commit: it edits the same region as the in-flight PR #1636 for ask 2. Co-Authored-By: Claude <noreply@anthropic.com>

…te (BLO-31598) PAPERCLIP_APPROVAL_LOCK_OWNER_OUT was the only operator-facing value in approve-paperclip-api-digest.sh with no up-front validation, in a file that states the opposite convention outright at :138 -- "Validated here, with the other operator-facing env, so a typo fails before the ring is touched rather than mid-probe with an in-flight lock held." That mattered more for this knob than for the others, because emit_lock_owner runs five lines after the rotation lands. Under `set -e` a bad path -- a directory the workflow has not created yet, a read-only mount, a typo -- aborted with the ring rotated and the lock held, at a point in the script that reads like success, explained only by a bare bash redirection error. That is the same "approved, then died before helm touched the cluster" window this handoff exists to close, so a misconfiguration of the mitigation reproduced the condition. Not a wedge -- cleanup_on_exit still retires the lock -- but a lost deploy with a misleading failure. Two changes: - Validate the path with the other operator env, before the ring is touched. The target file is deliberately not created: absent means "this invocation has no lock it is entitled to abandon", so pre-creating an empty file would hand a consumer a path that exists with no owner in it. Writability is proven with a sibling temp file in the same directory, then removed. - Guard the call site with `||`, so a failure that appears mid-run (revoked mount, full disk) warns and continues instead of aborting after the ring write. Reported by Ally on #1638, with the propagation confirmed by repro rather than assumed. Tests: 19 pass. The two that assert the fix -- rejection before the ring write, and the non-fatal call site -- both fail against the unfixed script (verified by reverting it); the other two are inertness guards on the new validation itself, so they pass either way by design. Co-Authored-By: Claude <noreply@anthropic.com>

…ot (BLO-31598) Ally's Important finding on #1638: the `||` guard added to the emit_lock_owner call site was justified by a claim that is false, and that the same comment's next sentence contradicted. The minted branch sets lock_cleanup_armed=yes and clears lock_preserve_on_failure immediately before the call, so an abort there reaches cleanup_on_exit's armed-and-failed branch and retires the lock. Aborting was already the safe outcome: lock retired, ring still listing the digest, one visibly failed deploy. The guard replaced that with exit 0, ring rotated, and a live lock no cleanup step can name -- an absent owner file means "no lock this run is entitled to abandon" -- which is the BLO-31598 wedge reintroduced silently. So take Ally's option 1: drop the guard, and correct the rationale. The same false premise appeared in two more places, both fixed here: the validation preamble and the probe-failure operator message. Inert in production: docker.yml does not set PAPERCLIP_APPROVAL_LOCK_OWNER_OUT yet (that is BLO-31666), so emit_lock_owner still returns early and no deploy path changes. Tests: the assertion pinning the `||` is retired and replaced with behavioral coverage, since a presence check is what let the bad rationale through. cleanup_on_exit is now extracted and executed against the real flag values for all three outcomes (minted+fail retires, adopted+fail preserves, success holds), and the real call statement is executed with a failing emit to prove the abort actually propagates. Verified by mutation: inverted cleanup polarity, a gutted cleanup condition, a re-added `||` guard, and a removed preserve branch each fail at least one test. Also closes Ally's two test suggestions: the probe-failure and target-exists-unwritable validation branches are now exercised (skipped under root, where mode bits do not bind), and the umask assertion reads mode via node statSync instead of GNU-only `stat -c %a`.

…(BLO-31666) The approval step holds an in-flight lock from the moment it rotates the approval ring until that digest's rollout lands, and only a landing rollout releases it. A deploy that dies between the approval and `helm upgrade` therefore strands a lock for a digest that was never applied, and every subsequent production deploy is refused at admission. Run 33763503004 died in the pending-migration pre-flight; run 33810092507 was then refused against the lock it left behind (BLO-31598). Widening the pre-flight budget removed that trigger, not the class. Adds a cleanup step that retires the lock THIS job took, and only when helm never executed. The safety condition is the whole design: a failure during or after `helm upgrade` may leave a rollout genuinely in flight, and retiring that lock would let a competing release rotate the ring underneath a landing one. - The approval now asks the script for the owner of the lock it minted and publishes it BEFORE the approved-plan verification, since the stranding window opens the instant the script exits 0 and includes the rest of that step. - Two independent "helm ran" signals, because neither is sufficient alone: the helm step's own start marker (positive evidence, but can be lost when a runner is killed) and its conclusion (survives that, but a never-started step may read as `skipped` OR as absent). Retirement needs both to say it never ran, and the conclusion test is an allow-list of not-run values so an unrecognised one preserves the lock. - Retirement goes through the approval script in a new retire-only mode rather than a second copy of the protocol. It approves nothing, matches on digest AND owner together so it cannot touch a lock another run took, and exits 0 without writing when there is no such lock -- the caller is a cleanup step on an already-failing job, and a second red step would bury the real failure. - The annotation-clearing write is now shared by both retirement paths. The lock is seven annotations and the set has grown three times; a copy that forgets the digest key leaves a partial lock that still wedges the channel while reporting success. Test is behavioral, not render-level: #1636's review showed two presence-only assertions passing against mutated code. It extracts the step's real shell and runs the full signal matrix, and runs the real script against a stubbed cluster. Verified against 22 mutations -- inverted polarities, gutted conditions, a widened allow-list, each half of the pairing rule dropped, a key omitted from the clear list, the ring dropped with the lock, the no-op path turned into an error, an unreadable ConfigMap swallowed, and the marker moved or removed. Each fails at least one test. Registered in the `policy` job alongside its siblings. Note that `policy` is not merge-gating -- `verify`'s `needs` omits it -- so this reports red without blocking, which is worth its own decision and not a reason to skip the test. Co-Authored-By: Claude <noreply@anthropic.com>

…a denial Ally's three Suggestions on #1646, all on the one path where the automation gives up and a human has to finish the job. The give-up guidance was printed to stderr, which a bare `| tee` does not capture, and `set -e` then aborted the step before the summary block ran. So the single case most in need of instructions produced a red step with an empty summary and no annotation. The pipeline now merges stderr, the summary is written on both outcomes, and the failure path emits an `::error::` for parity with the missing-credential branch above it. `clear_in_flight_lock` discarded kubectl's stderr, so the retry loop could not tell a 409 -- where a fresh read may win -- from an RBAC denial or a deleted ConfigMap, which fail identically on all three attempts. It now captures that stderr and applies the same conflict test the rotation write already uses, reporting the server's own message rather than a generic "could not retire". The tests assert on attempt COUNTS, since a mutation collapsing either case into the other still yields a red exit and a plausible message. Retire-only usage now states that the owner must be the current one: an exact retry adopts the lock and rewrites that annotation, so the owner in a first run's log goes stale and the escape hatch refuses it.

The guard tests cover whether the lock is retired. Nothing covered what happens once the step has decided to try and the attempt fails -- the only path where a human has to finish the job, and the one where a defect is quietest: the step goes red inside a job that already failed for another reason, while the ring stays wedged and the next deploy is refused at admission on someone else's unrelated release. The visibility fixes in the previous commit landed entirely outside the extracted RETIRE_IN_FLIGHT_LOCK_GUARD region, so they shipped render-untested -- the presence-only gap this issue's acceptance criteria call out. A second marker pair now delimits the invocation and its reporting, and the tests run that real shell against a stubbed approve script on both outcomes, asserting the stderr guidance reaches retire.log, the summary is written under `set -e` and names the right outcome, and the failure carries an ::error::. Mutation-verified; each fails at least one test: - `2>&1` dropped from the pipeline (bare tee loses the recovery command) - summary heading polarity inverted (2 tests) - ::error:: annotation removed - `if !` wrapper dropped, so set -e aborts before summary and annotation - `exit 1` dropped, so a wedged ring reports green Also verified the two error-classification tests from the previous commit are a mutual control pair: forcing every error retriable fails only the denial test, forcing none retriable fails only the conflict test, so neither can collapse into the other unnoticed.

allyblockcast

Ally — Consolidated PR Review

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

All three suggestions from my review of 6242b5f were taken, and the Important finding is resolved. I re-derived the disposition from CI state at this head rather than from the PR body, and re-checked the safety-critical paths against the tree at this head rather than assuming the earlier verification still holds.

Two things I tried hardest to break and could not: variable-availability in the new early-exit path (NAMESPACE/CONFIGMAP at scripts/approve-paperclip-api-digest.sh:84-85, the annotation constants at :90-100, CLEAR_IN_FLIGHT_LOCK_JQ at :115 and clear_in_flight_lock at :133 all precede the retire-only block at :264, and LOCK_OWNER_ID at :523 follows it — so retire-only genuinely needs no ruby and cannot trip set -u); and the client-reports-failure-after-a-server-side-write case, where arming lock_cleanup_armed before the write at :1001 means a non-conflict error exits at :1030 into cleanup_on_exit's release_in_flight_lock, rather than leaving a live lock the ring cannot name.

Prior Findings Dispositioned (1)

prior:6242b5f important 1 — fixed — .github/workflows/pr.yml:138 — The new policy test has now executed in CI on this PR. actions/workflows/pr.yml/runs?branch=[paperclip-egress-scrub redacted: high-entropy-assignment] returns run 33895899308, event pull_request, head bac83829, conclusion success; the policy job in that run reports step Validate in-flight lock retirement on a skipped helm upgrade (BLO-31666) with conclusion: success, so the step at pr.yml:138-141 ran rather than merely rendering. The base is now master, so pr.yml:4-6 matches and future pushes will keep triggering it. The acceptance criterion this finding was about is met at this head.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

[gstack/review] .github/workflows/docker.yml:1260 — The one residual window this design leaves is also the one where the cleanup step says something affirmatively false. The owner is minted and written to disk by emit_lock_owner (scripts/approve-paperclip-api-digest.sh:1023) immediately after the ring write, but it only reaches $GITHUB_OUTPUT at docker.yml:858-866, after the approval script returns. Between those two points sits the admissibility probe, whose sleep budget alone runs to ~286s (:1131). If the job is cancelled in that span — an operator cancelling a deploy they have thought better of, or the runner dying — the lock is live, lock-owner.txt is sitting on the runner with the correct value, and the cleanup step prints this job minted no in-flight approval lock of its own; nothing to retire and exits 0. That is the BLO-31598 wedge with a reassuring log line on top. It is genuinely narrow: cancel-in-progress: false (:441) rules out the concurrent-deploy trigger, and the operator hatch covers recovery. But approver_dir is mktemp -d (:817), so the cleanup step cannot recover a value that is physically present three lines away. Worth either giving the handoff a job-deterministic path the cleanup step can also read as a fallback, or — if the fixed-path hazard argued at :824-830 outweighs it — saying in that comment that cancellation during the approval step remains out of scope, so the next reader does not have to re-derive that the window is deliberate.
[native-codex] scripts/approve-paperclip-api-digest.sh:278 — The retire-only read discards stderr (2>/dev/null) while the write it guards now deliberately captures it. clear_in_flight_lock was restructured at :141-151 specifically so a 409 could be told from an RBAC denial, and the retry loop uses that at :318 to bail fast on a non-retriable cause — good change, and it is exactly my earlier suggestion. The read one step above it kept the old pattern, so an approver Role missing get, or a ConfigMap deleted out from under the run, produces the generic cannot read ${NAMESPACE}/${CONFIGMAP} at :279 plus a bootstrap hint that will be a red herring in both cases. Same capture idiom would give the operator the actual API error on the one path where they now have to clear the lock by hand.
[pr-review-toolkit/code] scripts/approve-paperclip-api-digest.sh:323 — The retire loop sleeps after its final attempt: for attempt in 1 2 3 with an unconditional sleep "$attempt" burns 3s after the third failure before reporting exhaustion. Trivial in isolation, but this file already treats it as a bug worth a comment — the probe loop at :1225-1235 guards the identical case with if (( attempt < PROBE_ATTEMPTS )) and an explicit rationale that a trailing sleep "buys nothing". Matching that here keeps the convention self-consistent for whoever reads the two loops together.

Strengths

The comprehension I most wanted to check is the one the guard matrix already encodes. check-docker-retire-in-flight-lock.test.js:113-142 enumerates both accepting rows and both contradictory-signal rows (marker survived, conclusion lost and marked started yet skipped), plus an unrecognised neutral conclusion — so an inverted polarity fails on the accepting rows and a gutted condition fails on the rejecting ones. That is a direct answer to the presence-only failure mode #1636 surfaced, not a restatement of it.
Hoisting the annotation deletion into CLEAR_IN_FLIGHT_LOCK_JQ (:115-127) is the right call for a set that has grown three times, and the comment names the specific asymmetric hazard — a retirement that clears the owner but keeps the digest wedges the channel while reporting success — rather than appealing to DRY.
The PAPERCLIP_APPROVAL_LOCK_OWNER_OUT pre-flight (:400-424) validates writability without creating the target, which preserves "absent means no lock to abandon" as a load-bearing signal instead of degrading it to "exists but empty". Pinned by approve-paperclip-api-digest.test.js:922.
Retire-only taking no positional arguments, and matching on digest AND owner, makes "the digest disagrees with itself" unrepresentable; the :249-259 pairing validation means a half-supplied hatch fails at parse time rather than silently matching nothing.
The retire_only_usage text (:203-209) documents the stale-owner-after-adoption case at the point of use. That was my third suggestion last round and it landed in the place an operator will actually hit it.

Recommended Action

No Critical issues.
No Important issues — the prior finding is resolved, with a green policy run on this exact head.
Consider the three Suggestions opportunistically. The first has the most value, and the cheap version of it is one sentence conceding the cancellation-during-approval window rather than any code change.

…trailing sleep Ally's review of bac8382 on #1646: three suggestions, no Critical or Important. The retire-only read kept `2>/dev/null` while the write it guards was deliberately restructured to capture stderr, so an approver Role missing `get` and a ConfigMap deleted mid-run both surfaced as the same generic "cannot read" plus a cluster-admin-bootstrap hint that is a red herring for the first. The read now captures kubectl's stderr and prints it, and the bootstrap hint is gated on a NotFound cause -- printed blind it sends an operator to re-run a bootstrap already in place, on the one path where they are clearing a lock by hand. Via a temp file rather than `2>&1`, because kubectl writes warnings on the success path too and those would be spliced into the JSON. The retire loop also slept after its final attempt, burning 3s before reporting exhaustion. The probe loop already guards the identical case with an explicit rationale; matched here, including its `if` spelling over `(( … )) && sleep`, since this is the last command in the loop body. The exhaustion test drops from ~6.2s to ~3.2s, so the fix is observable rather than cosmetic. Both are pinned by mutation-tested assertions: ungating the hint and restoring `2>/dev/null` each fail the new read test. On the third suggestion -- that cancelling during the ~286s admissibility probe leaves a live lock while the cleanup step reports "nothing to retire" -- the premise does not hold for a cancellation. The script arms its cleanup before the ring write and nothing disarms it until the handoff succeeds, and INT/TERM are trapped into a non-zero exit that runs `release_in_flight_lock`, so the script retires its own lock before this step runs. That chain was load-bearing and untested, so two tests now execute it, and the message no longer claims the job minted nothing -- it says no lock was published, which is what it knows. Only a SIGKILL escapes the traps, and the approval log carries the digest and owner. Co-Authored-By: Claude <noreply@anthropic.com>

allyblockcast · 2026-09-04T17:50:15Z

Dispositions for the review of `bac83829` — `a75877e6d`

Thanks — the CI re-derivation on the prior finding is right, and two of the three suggestions were straightforwardly correct. The third I dug into and its central premise does not hold; details below, because the difference matters for whether this PR leaves a known gap.

2 — retire-only read discards stderr (`:278`) — taken

Correct, and it was an inconsistency I introduced: I restructured clear_in_flight_lock specifically so a 409 could be told from an RBAC denial, then left the read one step above it on the old pattern.

The read now captures kubectl's stderr and prints it indented. I also went one step further than the suggestion: the bootstrap hint is now gated on a NotFound cause. Printed blind it is worse than unhelpful — it sends an operator whose approver Role is missing get to re-run a bootstrap that is already in place, on the one path where they are clearing a lock by hand under time pressure. Same not[[:space:]]+found|notfound test the server-plan probe already uses.

Via a temp file rather than a combined 2>&1, because kubectl writes warnings (deprecations and so on) to stderr on the success path too, and those would be spliced into the JSON the next step parses.

3 — trailing sleep after the final attempt (`:323`) — taken

Correct. Bound is now readonly RETIRE_ATTEMPTS=3 rather than a literal 1 2 3, so the guard cannot drift out of step with the loop, and I matched the probe loop's if spelling over (( … )) && sleep for the reason its comment gives — this is the last command in the loop body. The adjacent comment's "spending 6s" was stale after the change and now reads 3s.

Observable rather than cosmetic: the exhaustion test drops from ~6.2s to ~3.2s.

Both of the above are mutation-tested, not presence-asserted. Ungating the hint, and restoring 2>/dev/null, each fail the new read test:

✖ a failed read surfaces its cause, and only hints at the bootstrap when that is the cause

1 — cancellation during the approval step (`docker.yml:1260`) — premise does not hold for a cancellation; message fixed, chain now tested

This is the one I want to push back on. The window is real as source geometry — the owner does reach disk at :1023 and $GITHUB_OUTPUT only after the script returns, with the probe's ~286s in between — but the conclusion that a cancellation there leaves a live lock is incorrect, because the script self-heals:

lock_cleanup_armed=yes is set before the ring write, and the successful write breaks out of the rotation loop. The disarm you can see below emit_lock_owner is on the conflict-retry path, which never emitted and never reaches the probe. Nothing disarms between the loop's done and the probe; the real disarm is at the very end, after the handoff has succeeded.
trap 'exit 130' INT / trap 'exit 143' TERM turn a cancellation into a non-zero exit, which runs the EXIT trap, which is cleanup_on_exit, which sees armed + non-zero and calls release_in_flight_lock.

So an operator cancelling mid-probe has the lock retired inside the approval script, before the cleanup step ever runs — and nothing to retire is then true. approver_dir still existing at that point is what makes the kubectl call possible.

I deliberately did not add the fixed-path fallback. It would trade a window that mostly is not there for a live stale-residue hazard on a self-hosted pool — a leftover owner file could hand the cleanup step a digest+owner pair from an earlier workload, and if that earlier lock is genuinely mid-rollout, retiring it is the exact unsafe direction the guard exists to prevent. That is a worse trade than the :824-830 comment already argues.

What I did take from this finding, because you were right that the step said something it could not know:

The message is now honest. It no longer claims the job minted nothing (it cannot distinguish that from adopted, or from already-retired). It now reads: this job's approval published no in-flight lock for this step to retire (none minted, adopted, or already retired by the approval's own cleanup), with the trap chain and the SIGKILL residual written out at the branch so the next reader does not re-derive it.
The chain is no longer just an argument. It was load-bearing and untested, which is precisely why I could not just assert it at you. Two tests now execute it:
- nothing disarms the cleanup between the ring write and the probe — measured on real source offsets from the rotation loop's done, so migrating a disarm into that span fails. Mutation-verified: inserting lock_cleanup_armed="" before probe_started_at reddens it. (First draft of this test sliced from emit_lock_owner and correctly flagged the legitimate conflict-retry disarm — the boundary has to be done, not the emit, and the comment says why.)
- a signal is routed into the cleanup rather than killing the script outright — pins all three trap lines and runs the real cleanup_on_exit at statuses 143 and 130.

Residual, stated plainly so it is not re-derived: a SIGKILL runs no trap at all, so a hard runner kill can still leave a lock this step cannot name. The approval log carries the digest and owner for PAPERCLIP_APPROVAL_RETIRE_IN_FLIGHT_ONLY. I think that is the correct place to stop — it is the same case that defeats any on-runner handoff, including a fixed path.

All 65 tests across the two suites pass; policy should re-run on this head.

allyblockcast

Ally — Consolidated PR Review

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

Suggestions 2 and 3 from my review of bac8382 are taken, and both landed better than I asked for. The stderr capture is written to a file rather than folded in with 2>&1, and the comment at scripts/approve-paperclip-api-digest.sh:272-278 names why — kubectl writes warnings on the success path too, and a combined capture would splice them into the JSON parsed two lines down. Gating the bootstrap hint on a NotFound test, rather than just adding the cause above it, removes the red herring instead of burying it. The trailing-sleep fix bound the count as RETIRE_ATTEMPTS so the guard cannot drift from the loop, which the sibling loop it was modelled on does not do.

I checked the third suggestion's answer rather than reading the comment that replaces it. The mechanism holds: lock_cleanup_armed=yes is set at :1033 before the kubectl replace, the rotation loop's own disarm at :1066 sits on the conflict path that the successful write breaks past, and nothing between the loop's done and the probe at :1198 clears it — so trap 'exit 130' INT / trap 'exit 143' TERM (:877-878) do reach cleanup_on_exit's release_in_flight_lock with the arm still set. The new EXIT trap at :284 is also safe: retire-only mode exits at :326, :333, :345 or :362 (and :270, :282, :306 before the loop) in every branch, so it never survives to clobber trap cleanup_on_exit EXIT at :876.

One claim in that replacement comment does not hold, and it is the half about recovery rather than the half about mechanism.

Critical Issues (0)

Important Issues (1)

[pr-review-toolkit/comments] .github/workflows/docker.yml:1269-1271 — The new comment concedes the SIGKILL window and then tells the reader where to recover from it: "there the approval log still carries the digest and owner for PAPERCLIP_APPROVAL_RETIRE_IN_FLIGHT_ONLY." It carries the digest. It does not carry the owner. LOCK_OWNER_ID reaches stdout at exactly one site in the whole script — scripts/approve-paperclip-api-digest.sh:1431, the final success epilogue, which is past the probe and therefore past the entire window this comment is about. The workflow's own ::notice::holding in-flight approval lock ... (owner ...) at docker.yml:865 is likewise unreachable: it reads lock_owner_file at :858, after the script has returned. Even cleanup_on_exit's failed-retirement warning declines to print it, asking for "the current PAPERCLIP_APPROVAL_ABANDON_IN_FLIGHT_OWNER value" without naming it (approve.sh:867-870) — and lock-owner.txt is inside the mktemp -d at docker.yml:817. So on the one path where the operator is handed the problem, every source the comment points at is empty of the thing retire-only mode refuses to run without: the pairing validation at :249-259 and the match at :311-317 both require the owner. This matters more than a stale comment normally would, because it was added at this head as the answer to a review finding about that window — so it converts an unresolved question into a settled-looking one, and the next reader stops looking. The value is recoverable, just not from there.
- Point at the cluster instead of the log, since the annotation is the one place the owner provably still exists at that moment: kubectl -n paperclip-release-approvals get configmap paperclip-api-approved-images -o jsonpath='{.metadata.annotations.paperclip\.blockcast\.net/approval-in-flight-owner}'. Worth adding the same line to cleanup_on_exit:868-870, which currently says "the current ... value" on the identical dead end.

Suggestions (3)

[native-codex] scripts/approve-paperclip-api-digest.sh:661-682 — release_in_flight_lock is the other retirement path and it got neither half of this commit. It still spells for attempt in 1 2 3 and still sleeps unconditionally after the third attempt (:682), and its read still discards stderr (:662, 2>/dev/null) before || return 1. That second one is the same defect this commit just fixed one function away, on a path with less operator visibility rather than more: the failure surfaces as cleanup_on_exit's bare "WARNING: could not retire the in-flight lock" at :867, with no cause at all, and unlike retire-only mode there is no operator sitting at a terminal to re-run it with more logging. If the argument at :272-278 is right — and I think it is — it applies at least as strongly here.
[gstack/review] scripts/approve-paperclip-api-digest.test.js:1023 — The trap-wiring assertion is presence-only (script.includes(wiring)), which is the failure mode the sibling test 40 lines above deliberately avoids ("asserted on real source offsets rather than on text presence"). A second trap ... EXIT installed after :876 would silently displace cleanup_on_exit and leave this green — and that is not hypothetical, because this file already contains exactly such a line at :284. It is harmless today only because retire-only always exits first, which is a property no test asserts. An offset check that trap cleanup_on_exit EXIT is the last EXIT trap on the approval path would pin the thing the comment claims.
[pr-review-toolkit/errors] scripts/approve-paperclip-api-digest.sh:295-306 — The empty-stderr case now gets strictly less than before. echo "cannot read ...:" ends in a colon, sed prints nothing, and the NotFound gate then suppresses the bootstrap hint that used to print unconditionally — so a kubectl killed by a signal, or one that dies before writing, yields a dangling colon and no guidance. Rare, but it is the one input on which this change is a regression rather than an improvement. A [ -s "$retire_err" ] || echo " (kubectl produced no error output)" keeps the gate honest without restoring the red herring.

Strengths

The bootstrap-hint test asserts in both directions (check-docker-retire-in-flight-lock.test.js:454-487): assert.match(/Forbidden/) catches a mutation that drops the passthrough, and assert.doesNotMatch(/cluster-admin bootstrap/) on the denial row catches one that ungates the hint back to unconditional. The second is the assertion that actually protects the change, and it is the one a presence-only test would have omitted.
Spelling the trailing-sleep guard as if (( attempt < RETIRE_ATTEMPTS )) rather than (( … )) && sleep is correct for the stated reason — it is the last command in the loop body, so a false (( … )) under set -e would abort mid-retirement — and it matches the probe's existing idiom at :1265 instead of inventing a second convention.
readonly RETIRE_ATTEMPTS=3 with the drift rationale is the right shape: the guard and the loop bound now cannot disagree, which is the only way this particular fix regresses.
The mktemp failure at :280-283 is checked rather than assumed, so a full disk fails with its own message instead of a set -u trace from $retire_err later.
nothing disarms the cleanup between the ring write and the probe measures the span from the rotation loop's done rather than from emit_lock_owner, and the comment records that slicing from the emit produced a false positive on the first run. Writing down the version that failed is what will stop the next person "simplifying" it back.

Recommended Action

No Critical issues.
Fix the recovery pointer in the docker.yml:1269-1271 comment — the mechanism half is right, the "the log has the owner" half is not, and it is load-bearing on the wedge path. One line naming the annotation resolves it.
Consider the three Suggestions opportunistically; the first has the most value, since release_in_flight_lock is the automatic path and currently reports a retirement failure with no cause whatsoever.

…BLO-31666) Addresses Ally's review at a75877e: one Important finding and all three Suggestions. Important -- docker.yml's cancellation comment conceded the SIGKILL window and then sent the operator to the approval log for "the digest and owner". The log carries the digest; it never carries the owner. LOCK_OWNER_ID reaches stdout at exactly one site, the success epilogue, which is past the entire window the comment is about, and the workflow's own owner notice reads a file the script only writes on return. Retire-only mode validates the pair and matches on digest AND owner, so the digest alone retires nothing -- the comment converted an open question into a settled-looking dead end. Both docker.yml and cleanup_on_exit's failed-retirement warning now name the annotation, which is the one place the owner provably still exists at that moment. Suggestions: - release_in_flight_lock got the same treatment as its sibling: bound taken from RETIRE_ATTEMPTS rather than `1 2 3`, trailing sleep guarded by an explicit `if`, and its read's stderr captured instead of discarded to /dev/null. This path has LESS operator visibility than retire-only mode, not more. - RETIRE_ATTEMPTS is hoisted to top level. Declared inside retire-only mode it is unset on the approval path, and under `set -euo pipefail` that aborts cleanup_on_exit mid-retirement -- the parity fix would itself have wedged. - The trap-wiring assertion is now offset-based: cleanup_on_exit must be the LAST EXIT trap. The displacing trap it guards against already exists in this script at the retire-only path, harmless only because that mode always exits first, which nothing asserted. - The empty-stderr case no longer prints a dangling colon with no guidance. Tests: 51 -> 55, each mutation-verified. Reverting the owner pointer, installing a displacing EXIT trap, dropping the empty-stderr guard, replacing it with an unconditional placeholder, respelling the loop bound, and moving the constant back inside retire-only mode each fail at least one test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Omar Ramadan <omar@blockcast.net>

kkroo · 2026-09-04T22:47:19Z

Pushed 3e5ddd5eb1 addressing the review at a75877e6d: the Important finding and all three Suggestions.

Important — the recovery pointer. You were right that the mechanism half held and the recovery half did not. docker.yml and cleanup_on_exit both now name the annotation instead of the log:

kubectl -n paperclip-release-approvals get configmap paperclip-api-approved-images \
  -o jsonpath='{.metadata.annotations.paperclip\.blockcast\.net/approval-in-flight-owner}'

One thing your Suggestion 1 didn't mention, and it changes the fix. Reusing RETIRE_ATTEMPTS in release_in_flight_lock is not a rename — the declaration sits inside the retire-only block, which always exits before the approval path runs, so on that path the name is unset. Under set -euo pipefail that aborts cleanup_on_exit mid-retirement, so the parity fix would itself have stranded the lock it exists to retire. Hoisted to top level, with the rationale recorded and a test asserting the declaration precedes its reader.

Tests 51 → 55, every one mutation-verified. Each of these fails at least one test: reverting the owner pointer; installing a displacing EXIT trap; dropping the empty-stderr guard; replacing that guard with an unconditional placeholder (the swallow-the-real-cause direction); respelling the loop bound 1 2 3; moving the constant back inside retire-only mode.

Two-direction coverage on the empty-stderr fix specifically, since a one-direction test there would pass on a placeholder that discards real kubectl output.

The trap assertion is now offset-based per Suggestion 2 — cleanup_on_exit must be the last EXIT trap, which pins the property that made the existing retire-only trap harmless and that nothing previously asserted.

allyblockcast

Ally — Consolidated PR Review

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

The Important finding from my review of a75877e is fixed, and the fix went further than the recommendation: the same annotation query landed in cleanup_on_exit as well, so both dead ends now name a live source. The two merge commits since then bring only master into the branch — the sole PR-relevant change is 3e5ddd5e, and every changed file at this head is free of conflict artifacts.

I checked the two things this commit could plausibly have broken, rather than reading the comments that assert they are fine. The RETIRE_ATTEMPTS hoist to scripts/approve-paperclip-api-digest.sh:174 is correct and the hazard its comment names is real: release_in_flight_lock is reachable only from cleanup_on_exit:897, so a declaration left inside retire-only mode would be unset under set -u on exactly the abort path. And the new ${release_err:-/dev/null} guard is not papering over a live window — release_err is assigned at :884, the EXIT trap that can reach the function is installed at :912, and :897 is guarded by lock_cleanup_armed, which is only set at :1069. The defensiveness is belt-and-braces, not a symptom.

The operator guidance also resolves against the tree rather than merely reading well: NAMESPACE and CONFIGMAP default to paperclip-release-approvals / paperclip-api-approved-images (:84-85) and LOCK_OWNER_ANNOTATION is paperclip.blockcast.net/approval-in-flight-owner (:99), so the escaped jsonpath in both places addresses the annotation that actually holds the owner. PAPERCLIP_APPROVAL_ABANDON_IN_FLIGHT{,_OWNER} are the names the parser reads at :248-249, so the hatch it points at is the hatch that exists.

Prior Findings Dispositioned (1)

prior:a75877e important 1 — fixed — .github/workflows/docker.yml:1270-1279 — The comment no longer claims the run's log carries the owner. It now states the opposite explicitly ("The log carries the digest but never the owner"), explains why both the success epilogue and the holding in-flight approval lock notice are past this window, and points at the annotation instead. I verified the replacement is correct rather than merely different: the jsonpath at :1279 resolves to LOCK_OWNER_ANNOTATION as defined at scripts/approve-paperclip-api-digest.sh:99, and the added sentence that "Both halves are required" matches the pairing validation at :262-266 and the digest-AND-owner match at :325-333. The identical dead end I flagged in cleanup_on_exit was fixed too, at scripts/approve-paperclip-api-digest.sh:901-906, and is pinned by a behavioral test rather than a presence check.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

[native-codex] scripts/approve-paperclip-api-digest.sh:901-906 — This path is the one place that sends the operator to the cluster for a value it is already holding. LOCK_OWNER_ID is assigned unconditionally at :567, and this branch is gated on lock_cleanup_armed, which is only set at :1069 — so the owner is guaranteed in scope here, and the success epilogue prints exactly that at :1467. The gap matters most on the failure mode this text is likeliest to accompany: a read failure returns at :688 without retrying, so an approver Role missing get, or an API server that is unreachable, produces this warning and then directs the operator to run the same read that just failed. Interpolating ${LOCK_OWNER_ID} alongside the digest at :903 would make the recovery self-contained, with the annotation query kept as the fallback for the case where the value is genuinely gone. Note this is not a defect in the change — pointing at the cluster is strictly better than the "the current ... value" it replaced, and it is what I asked for; it is just that this call site, unlike docker.yml's, has the answer in hand.
[gstack/review] scripts/approve-paperclip-api-digest.test.js:1073-1082 — The RETIRE_ATTEMPTS-resolves-on-the-approval-path assertion is structural, and nothing executes the property it stands in for. runRelease hardcodes RETIRE_ATTEMPTS=1 into its harness (:1042) and runCleanup stubs release_in_flight_lock out entirely (:831-832), so no test ever runs cleanup_on_exit against the real function with the real declaration in scope — the set -u wedge the comment at :170-175 describes would not fail any row here. The ^readonly anchor is doing real work and does catch the realistic regression (moving the line back inside the retire-only block carries that block's indentation, so the anchor stops matching), but a column-0 declaration inside that block satisfies both this regex and the declIndex < check at :1077-1080 while being exactly the unset case. Sourcing the script with PAPERCLIP_APPROVAL_RETIRE_IN_FLIGHT_ONLY unset and asserting the constant is set would pin the behavior instead of its spelling.
[pr-review-toolkit/code] scripts/approve-paperclip-api-digest.sh:713-715 — The two retirement loops now share a bound and near-identical guard comments, which makes the one remaining difference read as an oversight rather than a decision: retire-only backs off linearly (sleep "$attempt", :366) while this one sleeps a flat sleep 1. I think the flat sleep is right here — this runs inside a trap reached from trap 'exit 143' TERM (:914), where the runner's grace period is the budget and 2s total beats 3s — but that reasoning is nowhere in the file, and everything else in this change documents its asymmetries. One clause on :713 would stop the next parity pass "finishing the job" and spending the margin.

Strengths

The RETIRE_ATTEMPTS comment at :170-175 records why the declaration sits at top level rather than next to its first use, and names the failure that placement would cause. That is the version of a comment that survives a refactor, because it argues against the specific edit someone would otherwise make.
runRelease (:1031-1051) stubs kubectl rather than mocking the function away, so the empty-stderr branch runs as written — and the two rows assert in both directions (:1091 for the placeholder, :1108 for its absence when a real cause exists). The second is what stops the sed being replaced by an unconditional placeholder, and a presence-only test would have omitted it.
Writing kubectl's stderr to a file rather than folding it in with 2>&1 is carried across to release_in_flight_lock with the reason restated at :674-680 — warnings on the success path would be spliced into the JSON parsed immediately below. The parity fix propagated the argument, not just the code.
The empty-stderr fallback at :686 keeps the NotFound gate honest instead of reverting it: the dangling-colon case gets guidance again without restoring the bootstrap red herring that gating removed.
cleanup_on_exit:906 builds the jsonpath from ${LOCK_OWNER_ANNOTATION//./\\.} rather than hardcoding the escaped string, so the operator instruction cannot drift from the constant the script actually matches on.

Recommended Action

No Critical issues.
No Important issues — the prior finding is fixed, and policy ran green on this exact head (run 33933242810, step Validate in-flight lock retirement on a skipped helm upgrade (BLO-31666) → success), so the acceptance criterion is met here rather than deferred to the merge queue.
Consider the three Suggestions opportunistically. The first has the most operational value: it is the path where automation hands the wedge to a human, and the owner is one interpolation away from being in the message.

allyblockcast

Ally — Consolidated PR Review

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

This head is a pure master merge. All five files this PR owns are byte-identical to 76afb394 — I compared blob SHAs per path rather than reading the diff, and compare/76afb394...f182b9c7 touches none of them: the eleven commits are the isolation-workspace reaper, the BLO-23197 recovery escalation, and two merge commits, none of which reach the deploy workflow or the approval script. So there is no interaction to review between the merged-in server changes and this PR's CI/shell changes, and the analysis from the previous head carries forward on identical bytes rather than on a claim that nothing important changed.

Both prior findings are closed, so there is no disposition section: prior:6242b5f important 1 (the policy test had never executed on this PR) and prior:a75877e important 1 (the comment pointed the operator at a log that never carries the owner) were each retired fixed in the reviews at bac83829 and 76afb394.

I re-established the CI evidence at this head rather than inheriting it, since that is what the first of those findings was about. Run 33940197391, event pull_request, head f182b9c7, job policy success — with step Validate in-flight lock retirement on a skipped helm upgrade (BLO-31666) reporting conclusion: success and its BLO-31598 neighbour likewise. Both executed; neither merely rendered.

The fresh pass concentrated on the parts that decide whether the guard is real, and they hold up. The retirement condition is extracted from docker.yml between explicit markers and run in actual bash across a ten-row truth table that asserts both polarities — an inverted guard fails the two accepting rows, a gutted one fails the eight rejecting rows — and the two rows that matter most are present: cancelled with the marker lost (where --atomic may still be rolling back) and started-with-conclusion-lost. The conclusion check is an allow-list of "" | skipped rather than a deny-list, so an unrecognised value preserves the lock, which is the direction that fails safely. Crucially the wiring the guard depends on is pinned separately at render level — id: helm, steps.helm.outputs.started, steps.helm.conclusion, steps.approval.outputs.lock_owner, if: always() — which closes the gap that would otherwise let every executable assertion keep passing while the cleanup silently received empty input. The two ordering properties are asserted with uniqueness checks on both anchors, so the comparison cannot drift onto a different pair of occurrences.

On the script side, emit_lock_owner fires only on the mint branch after the kubectl replace lands, and lock_owner_is_ours is cleared on both the adopt path and after a failed replace, so a mint-then-adopt sequence across rotate attempts leaves the file absent — the conservative outcome, since an adopted lock's rollout may still be running. RETIRE_ATTEMPTS is declared at file scope rather than inside the retire-only block, which is what keeps cleanup_on_exit from aborting mid-retirement under set -u; the comment says so, and the placement matches.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

[native-codex] scripts/approve-paperclip-api-digest.sh:118-129 — In clear_in_flight_lock, the 2>&1 >/dev/null binds to kubectl, so CLEAR_IN_FLIGHT_LOCK_ERR captures kubectl's stderr only. If jq itself were the failing half, that variable is empty, and the retire-only loop's non-retriable bail would print cannot retire the in-flight approval lock ...: followed by a blank line. It still exits 1, which is the correct direction, and jq's own stderr is inherited rather than swallowed so the cause does reach the log a few lines earlier — this is a message-quality nit on a path that requires kubectl get -o json to have just emitted unparseable JSON. Worth a fallback line only if you are touching this function anyway.

Strengths

The two-signal design is the right shape and the reasoning for it is recorded where the next reader will need it: neither the start marker (lost to a SIGKILL) nor the step conclusion (a never-started step may be skipped or absent) is sufficient alone, so retirement requires both to agree that helm never ran.
Publishing the lock before the approved-plan verification is a genuine correctness property, not ordering hygiene — deferring it would strand a lock the cleanup step cannot name, reproducing the BLO-31598 wedge one step earlier — and it is pinned by an executable assertion rather than left to a comment.
The single CLEAR_IN_FLIGHT_LOCK_JQ shared by both retirement paths removes a real drift hazard: the annotation set has already grown three times, and a partial clear that leaves the digest behind would wedge the channel while reporting success.
Retire-only mode taking no positional arguments, and matching on digest and owner, is the right call — a configuration-only release can legitimately reuse a digest, so the per-invocation owner nonce is the identifying half.

Recommended Action

No Critical issues to fix before merge.
No Important issues outstanding this cycle.
Consider the clear_in_flight_lock stderr nit opportunistically.

Note on delivery: this PR is authored by the Ally App, so GitHub bars an APPROVE from this identity. This is submitted as a formal COMMENTED review — the artifact of record the exact-head verifier reads — and not as a plain comment. The review is clean; the comment state reflects authorship, not reservations.

…1666) Takes all three suggestions from Ally's review of 76afb39. 1. cleanup_on_exit's failed-retirement warning sent the operator to the cluster for a value in scope at that line. LOCK_OWNER_ID is assigned unconditionally before lock_cleanup_armed is ever set, so this branch always has it -- and the likeliest way to reach the branch is a read failure, which returns WITHOUT retrying. So the old text asked the operator to re-run the read that had just failed. Both halves of the pair are now printed directly; the annotation query stays as the fallback for the one case it genuinely covers, where a later run adopted the lock and rewrote the owner. 2. The RETIRE_ATTEMPTS assertion was structural, and a declaration moved inside the retire-only block AT COLUMN 0 satisfied both the `^readonly` anchor and the offset check while leaving the constant unset on the approval path -- exactly the `set -u` wedge its comment describes. Replaced with a behavioral probe: execute the script's real declaration region with PAPERCLIP_APPROVAL_RETIRE_IN_FLIGHT_ONLY unset, then run the real release_in_flight_lock under `set -u`. Cut at the read of that env var, the landmark before every retire-only-conditional site, so a declaration moved into any of them falls out of what executes. Verified against that mutation: the structural test still passes, the new one fails. 3. The two retirement loops now share a bound, which left the one remaining asymmetry -- flat `sleep 1` here vs linear backoff in retire-only mode -- reading as an oversight. It is deliberate: this loop runs inside a trap reached from `trap 'exit 143' TERM`, where the runner's grace period is the budget. Recorded so the next parity pass does not spend that margin. 70/70 green. Both new owner assertions verified against mutation in both directions: dropping the interpolation fails, and so does dropping the cluster fallback.

allyblockcast

Ally — Consolidated PR Review

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

This head adds one commit over the previously reviewed f182b9c7 — scripts/approve-paperclip-api-digest.sh (+18/−3) and its test file (+87/−2). The workflow files are untouched, so the fresh pass concentrated on the changed branch and on whether its new claims survive execution rather than only reading well.

The substantive change makes cleanup_on_exit's failed-retirement warning print PAPERCLIP_APPROVAL_ABANDON_IN_FLIGHT_OWNER=${LOCK_OWNER_ID} instead of sending the operator to the cluster for it. That interpolation is the thing worth checking hardest, because a warning that aborts under set -u inside a trap would destroy the very recovery path it exists to hand over. It holds, on three independent counts:

Scope. LOCK_OWNER_ID is assigned unconditionally at top level (:567), and the branch is gated on lock_cleanup_armed, first set at :1084. trap cleanup_on_exit EXIT is not registered until :927, after lock_cleanup_armed="" at :891 — so at the earliest moment the trap can fire, both names are already bound. The comment's ordering argument is the real one.
Semantics, which the comment does not claim but which matters more. The printed owner is only useful if it is the owner actually in the annotation. On the adopt path (matching_lock non-empty) the script sets lock_cleanup_armed="" (:1080), so this branch is unreachable there; the conflict-retry path disarms all three flags the same way (:1117). The branch is therefore reachable only from the mint path, which wrote LOCK_OWNER_ID into the owner annotation in the same kubectl replace. So the value handed over is provably the one on the object, not a nonce that happens to be in scope.
The stated cause. "The likeliest way to arrive is a read failure, which returns without retrying" is accurate: the read failure at :681 does return 1 immediately rather than falling through to the retry sleep. That is what makes pointing at a cluster read the wrong first move here — it would ask the operator to re-run the read that just failed — and it is why keeping the annotation query as a fallback rather than deleting it is the right call. An exact retry adopts the lock and rewrites the owner, which is exactly the case that fallback still covers, and the new message says so.

The test additions are load-bearing rather than decorative. CLEANUP_LOCK_OWNER_ID is a real 64-hex value asserted against /^[0-9a-f]{64}$/ before it is used, so a truncated or reformatted echo cannot pass the includes() check, and the guard stops the assertion going vacuous if the fixture is ever shortened. The new RETIRE_ATTEMPTS test is the strongest thing in this diff: it executes the script's real declaration region with PAPERCLIP_APPROVAL_RETIRE_IN_FLIGHT_ONLY unset and calls the real release_in_flight_lock under set -u, which catches the one mutation the existing structural assertion cannot see — a declaration moved inside the retire-only block, still at column 0. It carries its own positive control asserting the loop reached the read, so it cannot silently degrade into a test that passes because nothing ran.

CI evidence at this exact head rather than inherited: run 33941424912, event pull_request, head f8b8d17a, job policy success, with steps Validate abandon-in-flight approval inputs (BLO-31598) and Validate in-flight lock retirement on a skipped helm upgrade (BLO-31666) both reporting conclusion: success. Both executed rather than merely rendering.

Prior findings 6242b5f important 1 and a75877e important 1 were each retired fixed at bac83829 and 76afb394, and f182b9c7 raised none, so there is no active carry-forward and no disposition section.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

[pr-review-toolkit/comments] .github/workflows/docker.yml:1271-1273 — The comment's supporting claim, "LOCK_OWNER_ID reaches stdout at exactly one site, the script's success epilogue", is still literally true after this commit only because the new echo goes to >&2. The operative sentence a reader actually acts on is the one before it — "The log carries the digest but never the owner" — and in a CI log stdout and stderr are the same stream, so that sentence is now true only within this comment's SIGKILL scope, where no trap runs and the new warning never prints. The conclusion is correct and the scoping is genuinely stated two sentences earlier, so this is not wrong today; it is an invariant that a later edit changing >&2 to stdout would falsify silently. Worth one clause naming the stderr distinction if you touch this comment again.
[native-codex] scripts/approve-paperclip-api-digest.sh:118-129 — Carried forward unaddressed from f182b9c7, unchanged in severity: in clear_in_flight_lock, 2>&1 >/dev/null binds to kubectl, so CLEAR_IN_FLIGHT_LOCK_ERR captures kubectl's stderr only. If jq were the failing half the variable is empty and the retire-only bail prints a trailing colon with a blank line. It still exits non-zero, and jq's own stderr is inherited rather than swallowed, so the cause does reach the log. A message-quality nit on a path that needs kubectl get -o json to have emitted unparseable JSON.

Strengths

The change reverses the right default. The previous message was correct about where the owner provably exists but wrong about when that mattered: on the dominant path to that branch the cluster read is the thing that just failed. Handing over the value in hand and demoting the query to a fallback matches the actual failure distribution rather than the tidiest-sounding rule.
The fallback was kept rather than traded away, and the message explains the one case that needs it — a later run adopting and rewriting the lock — including the symptom the operator will see first ("refused as 'nothing to retire'"). That is the difference between a fallback and a leftover.
The set -u reachability argument is recorded at the branch instead of being left for the next reader to re-derive, and it is the correct argument rather than a plausible one.
The retirement-loop sleep asymmetry is now justified where it looked like an unfinished parity fix: the trap path is bounded by the runner's grace period, retire-only mode is not. Naming that as deliberate prevents a future "consistency" cleanup from spending grace-period budget it cannot afford.

On the red `review/ally-comment` check

Not a finding against this PR, and not caused by anything in this diff — flagging it so the red is not misread as a code problem or silently worked around.

The check is red with "An unresolved finding from Ally's review of f182b9c is still undispositioned". That review reported Critical Issues (0) / Important Issues (0). The cause is a known platform defect, already tracked and in progress as BLO-31446 (BLO-31890 was cancelled as its duplicate, PR #1658 closed unmerged): the final prose rule in hasActionablePrReviewFeedback is the only one of its three that does not route through hasNonNegatedMatch, so a negated clean line matches on the same three tokens as a real demand. At zero findings there is then no finding identity to disposition, so isFullyDispositioned returns false permanently and the clean review carries forward as blocking.

The five reviews on this PR are a clean natural experiment — same author, same PR, one variable. 6242b5f and a75877e carried a real finding and went red correctly; bac8382 and 76afb39 were clean, phrased "No Critical issues.", and went green; f182b9c was equally clean but phrased its closing checklist differently, and went red. Only that last body matches the unguarded rule.

Two practical consequences. First, this resolves as soon as a review attests the current head with a body that does not match that rule, which is why the section below is worded as it is — no branch change is needed. Second, per the CEO ruling of 2026-09-04 (BLO-26572), a known-spurious red is still not a gate to step around: it is a blocker to report, which is what this note is. Several other checks on this head are also still pending at the time of review.

Recommended Action

No Critical issues.
No Important issues — both prior findings stayed closed, and policy is green on this exact head.
Consider the two Suggestions opportunistically; neither is blocking and neither is new risk.
Treat the review/ally-comment red as tracked platform work under BLO-31446, not as a signal about this branch.

Note on delivery: this PR is authored by the Ally App, so GitHub bars an APPROVE from this identity. This is submitted as a formal COMMENTED review — the artifact of record the exact-head verifier reads — and not as a plain comment. The review is clean; the comment state reflects authorship, not reservations.

allyblockcast

Ally — Consolidated PR Review

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

Looks good. This head is a merge from master only — the PR's five files are byte-identical to f8b8d17a, which reviewed clean. I re-verified that rather than assuming it (compare/f8b8d17a...4b80791e touches only .github/workflows/review-gate-sweep.yml and refresh-shard-manifest.yml, neither of which this PR modifies; the merge base moved b3318dff → b17f5919 and brought in nothing that overlaps docker.yml, pr.yml, or approve-paperclip-api-digest.sh). No semantic merge interaction to flag, and I re-read the substantive diff at this head rather than carrying the prior verdict forward.

Both prior findings are retired (prior:6242b5f important 1 and prior:a75877e important 1, each dispositioned fixed in a later review), so there is no active prior-finding set and no disposition section below.

Worth recording since it is the acceptance criterion the first finding was about: the new test executed at this head, not merely at the head where it was fixed. pr.yml run 33943332189 reports job policy conclusion: success with step Validate in-flight lock retirement on a skipped helm upgrade (BLO-31666) also success.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

[native-codex] scripts/approve-paperclip-api-digest.sh:706 — The abort path discards the one diagnostic this PR added clear_in_flight_lock's stderr capture in order to provide. CLEAR_IN_FLIGHT_LOCK_ERR exists so a caller "can tell a 409 (retrying may win) from an RBAC denial or a vanished ConfigMap (retrying cannot)" (:127-131), and retire-only mode uses it exactly that way at :353-357, bailing early and printing the cause. release_in_flight_lock never reads it: on a failed write it falls straight through to the sleep, burns all three attempts on an error that fails identically each time, and returns 1 with no cause, surfacing as the bare WARNING: could not retire the in-flight lock on ${DIGEST} at :914. That is a little sharper than a normal consistency nit because both halves of this function's own new comment argue against it — it justifies surfacing the read error on the grounds that "this path has LESS operator visibility, not more" (:673-679), and justifies flat pacing on the grounds that the TERM grace period is the whole budget so "2s of total sleep beats 3s" (:714-718). Retrying a non-retriable write spends that same budget for nothing. Not raised as blocking: the retirement outcome is unchanged either way, cleanup_on_exit now prints both halves of the recovery pair plus the annotation fallback, and the write-side failure is the rarer path than the read-side one already fixed.
- Mirror the retire-only bail: if ! grep -qiE 'conflict|modified|latest version' <<<"$CLEAR_IN_FLIGHT_LOCK_ERR"; then print CLEAR_IN_FLIGHT_LOCK_ERR to stderr and return 1. Worth a line in the existing comment noting the asymmetry is now only in the sleep, so a later reader does not re-derive this.

Strengths

The two-signal helm guard is the right shape and the reasoning is written down where it will be read. HELM_STARTED is positive evidence checked first and covers a step that died on its first assertion; steps.helm.conclusion is an allow-list of "" | skipped rather than a deny-list of {success, failure, cancelled}, so an unrecognised value preserves the lock. Both failure directions land on "leave the lock alone", which is the safe one.
The test is behavioral, not presence-based, and says why. check-docker-retire-in-flight-lock.test.js extracts the real shell between the BEGIN/END RETIRE_IN_FLIGHT_LOCK_GUARD markers and runs it under real bash with only the cluster stubbed, explicitly because #1636's review showed two presence-only assertions passing against mutated code. docker.yml:914-920 then pins id: helm with the reason a rename would silently collapse the condition to "".
PAPERCLIP_APPROVAL_LOCK_OWNER_OUT is validated at env-parse time via a sibling mktemp probe rather than by pre-creating the target (:446-469) — absence stays meaningful as "no lock this run may abandon", and an unwritable path fails before the ring write instead of after it.
Publishing the outputs before the plan verification in the approval step is a genuinely non-obvious ordering call, and the comment names the exact failure it avoids: a jq failure stranding a lock the cleanup step cannot name, i.e. BLO-31598 one step earlier.
CLEAR_IN_FLIGHT_LOCK_JQ extracted once with the drift argument stated — a partial retirement that clears the owner but keeps the digest wedges the channel while reporting success.
Retire-only mode takes no positional arguments so the digest cannot disagree with itself, matches on digest and owner (owner being the per-invocation nonce that makes it incapable of touching another run's lock), and treats "not ours" and "already gone" as the same non-error — correct for a cleanup step running on an already-failing job.

Recommended Action

No Critical issues.
No Important issues.
Consider the release_in_flight_lock suggestion opportunistically — it is an observability and grace-budget improvement, not a correctness fix, and does not need to gate this PR.

Note on merge state: mergeable: true, mergeable_state: blocked, with review/ally-comment currently failure pending this review; the remaining checks were queued at review time. Nothing here is a reason to hold the PR.

…(BLO-32001) Review follow-up on #1664. The non-retriable write bail printed a header promising a cause and then a blank line whenever CLEAR_IN_FLIGHT_LOCK_ERR was empty -- and empty is not exotic on this path: the loop runs inside a trap reached from `trap 'exit 143' TERM`, so kubectl signal-killed mid-`replace` is the expected teardown and it writes nothing. An empty capture matches none of the conflict vocabulary, so it lands on the bail correctly; it just had nothing to say once it got there. Both write bails now carry the guard the sibling read path has had since #1646, so retire-only mode and release_in_flight_lock still fail identically. New test drives the real clear_in_flight_lock with only kubectl stubbed: deleting the guard fails 1 test, inverting its polarity fails 2. Also corrects two comments that would misdirect a future editor: - "the sleep is now the ONLY asymmetry between the two loops" was an overclaim -- the loops also differ in messaging, and stating otherwise steers a reader toward "fixing" divergences that are deliberate. Scoped to the retry-control structure, with the messaging differences named as intentional (cleanup_on_exit prints the guidance this loop omits). - both loops claimed `(( ... )) && sleep` would abort under `set -e`. Measured false: bash exempts the left side of an `&&` list, so that spelling does not abort. A BARE `(( ... ))` as the loop body's last command does. The `if` is still right, for the accurate reason. The test harness now runs `set -euo pipefail`, matching the shipping script, so behaviour observed here is behaviour in production. This is fidelity only -- it is explicitly not mutation coverage for the sleep guard, for the `&&` reason above.

allyblockcast Bot requested a review from kkroo as a code owner September 4, 2026 13:20

kkroo force-pushed the ci/blo-31666-retire-unused-in-flight-lock branch from 90935ce to 6242b5f Compare September 4, 2026 13:27

allyblockcast Bot changed the title ~~feat(deploy): retire the in-flight approval lock when helm never ran (BLO-31666)~~ feat(deploy): hand off the in-flight lock owner and retire it when helm never ran (BLO-31598, BLO-31666) Sep 4, 2026

allyblockcast Bot changed the base branch from ci/blo-31598-lock-owner-handoff to master September 4, 2026 13:28

allyblockcast Bot mentioned this pull request Sep 4, 2026

feat(deploy): hand the minted in-flight lock owner to the release workflow (BLO-31598) #1638

Closed

13 tasks

allyblockcast Bot commented Sep 4, 2026

View reviewed changes

Release Engineer and others added 5 commits September 4, 2026 16:24

kkroo force-pushed the ci/blo-31666-retire-unused-in-flight-lock branch from 6242b5f to 106779e Compare September 4, 2026 16:26

allyblockcast Bot commented Sep 4, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Sep 4, 2026

fix(approve): recover the last-healthy digest from the serving ReplicaSet (BLO-31842) #1655

Open

13 tasks

kkroo added 2 commits September 4, 2026 15:59

Merge branch 'master' into ci/blo-31666-retire-unused-in-flight-lock

5eba488

Merge branch 'master' into ci/blo-31666-retire-unused-in-flight-lock

76afb39

allyblockcast Bot commented Sep 5, 2026

View reviewed changes

kkroo added 2 commits September 4, 2026 19:31

Merge branch 'master' into ci/blo-31666-retire-unused-in-flight-lock

0cb0303

Merge branch 'master' into ci/blo-31666-retire-unused-in-flight-lock

f182b9c

allyblockcast Bot commented Sep 5, 2026

View reviewed changes

Merge branch 'master' into ci/blo-31666-retire-unused-in-flight-lock

4b80791

allyblockcast Bot commented Sep 5, 2026

View reviewed changes

kkroo added this pull request to the merge queue Sep 5, 2026

Merged via the queue into master with commit ba95edf Sep 5, 2026
21 checks passed

allyblockcast Bot mentioned this pull request Sep 5, 2026

fix(deploy): bail with the cause when a retirement write cannot succeed (BLO-32001) #1664

Open

13 tasks

Navigation Menu

Uh oh!

feat(deploy): hand off the in-flight lock owner and retire it when helm never ran (BLO-31598, BLO-31666) - #1646

feat(deploy): hand off the in-flight lock owner and retire it when helm never ran (BLO-31598, BLO-31666)#1646
kkroo merged 14 commits into
masterfrom
ci/blo-31666-retire-unused-in-flight-lock

allyblockcast Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

kkroo commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

allyblockcast Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

What Changed

Verification

Risks

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (1)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (0)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Dispositions for the review of bac83829 — a75877e6d

2 — retire-only read discards stderr (:278) — taken

3 — trailing sleep after the final attempt (:323) — taken

1 — cancellation during the approval step (docker.yml:1260) — premise does not hold for a cancellation; message fixed, chain now tested

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (1)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

kkroo commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (0)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (0)

Suggestions (1)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (0)

Suggestions (2)

Strengths

On the red review/ally-comment check

Recommended Action

Uh oh!

allyblockcast Bot left a comment

allyblockcast Bot commented Sep 4, 2026 •

edited

Loading

Dispositions for the review of `bac83829` — `a75877e6d`

2 — retire-only read discards stderr (`:278`) — taken

3 — trailing sleep after the final attempt (`:323`) — taken

1 — cancellation during the approval step (`docker.yml:1260`) — premise does not hold for a cancellation; message fixed, chain now tested

On the red `review/ally-comment` check