ci(deploy): make the abandon-in-flight escape hatch reachable from docker.yml

[kkroo]

Why

The admission approval holds an in-flight lock from the moment it approves a digest until that digest's rollout lands. A deploy that dies between the approval step and helm upgrade therefore strands a lock that no rollout can ever clear, and every subsequent production deploy is refused at admission.

That is not hypothetical — it is the current state of production:

• Run 33763503004 approved sha256:6c45e9e3…, then died in pending-migration pre-flight against the 180s budget that BLO-31254 has since widened to 600s startup + 180s run. helm upgrade was skipped; the cluster was never touched.
• Run 33810092507 was then refused:

  refusing to approve sha256:436c11c3…: sha256:6c45e9e3… is still locked
  until Deployment/paperclip-api completes rollout
  

• The lock is provably stale: Deployment/paperclip-api is 2/2 on sha256:a8c6d6c3… with deployed-commit=a3fba29af, and no cluster object references 6c45e9e3 at all.

scripts/approve-paperclip-api-digest.sh documents the remedy in its own header, but docker.yml declared one input (target_sha) and contained zero occurrences of ABANDON. The script also requires the paperclip-release-approver credential, deliberately not the Release Engineer's deploy credential and present only as a CI secret. So the only route out was a human holding that credential running the script by hand, outside CI. There was no in-repo path.

What

Paired optional abandon_in_flight / abandon_in_flight_owner dispatch inputs, read through env: and forwarded to the approval script.

• Required together. The script rejects a digest alone because a configuration-only release can reuse a digest, so the owner is what identifies which approval is retired. The workflow enforces the pairing rather than letting a half-supplied pair fail deeper in.
• Validated before the credential is written to disk, so a malformed value never reaches the higher-privilege credential and the error names the input rather than the script's internal variable.
• Emits ::warning:: when retiring a lock, so it is never a silent side effect of a skim-approved deploy.
• Ordinary deploys are unchanged — both render empty on push, which the script treats as "no abandon requested" via its own :- defaults.

Both values appear verbatim in the admission refusal, so a wedged deploy hands the operator exactly what to paste.

Verification

Guard behavior was exercised by extracting the real shell from docker.yml and running all six combinations — presence assertions alone would not catch a guard that gates nothing:

case	rc	reached script
neither set (ordinary deploy)	0	yes
digest only	1	no
owner only	1	no
malformed digest	1	no
malformed owner	1	no
both valid	0	yes

Test suite: 40/40 across the new file and all sibling docker.yml tests; check-workflows-parse 11/11.

The new test's shape assertions are bound to the abandon variables by name. An earlier draft asserted the bare ^sha256:[0-9a-f]{64}$ / ^[0-9a-f]{64}$ patterns and passed on unmodified master, which already shape-checks digest (3 sites) and marker (1). Confirmed by negative control that the committed version fails against origin/master.

Two things for the reviewer

1. This does not unwedge production by itself. Landing it makes the remedy reachable; someone still has to dispatch with abandon_in_flight=sha256:6c45e9e3e1907ae9bcd7aa9cc2d2ca3d671677af85aeca1e0fad4f2339e8070c and abandon_in_flight_owner=e716030e67f731750d1d19a48967098289064f284053bc71bcac1ef0eef770ff.

2. The policy job is not merge-gating. verify is the only required context on master, and its needs omits policy — so this test, and every existing docker.yml test beside it, runs and reports red without blocking a merge. I followed the existing convention rather than widening verify in an unrelated PR, but it is worth deciding deliberately.

Filed as BLO-31598, which also proposes a third option not implemented here: having a deploy that fails before helm upgrade retire the lock it took, since such a lock is abandonable by definition.

🤖 Generated with Claude Code

[allyblockcast]

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

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used

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

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

The workflow change itself is correct, and I verified the premise rather than taking it on trust: guard-pending-deploy.sh:16 blocks only on a GitHub run already status=waiting at the environment gate, not on the cluster in-flight lock — so a stranded lock does not skip the deploy job, and the hatch really is reachable. Both findings below are in the new test file, which is weaker than its own comments claim.

Critical Issues (0)

Important Issues (2)

• [tests] scripts/check-docker-abandon-in-flight-inputs.test.js:107,112 — the shape-check assertions make the negation optional (\[\[ !? ?"), so a guard with inverted polarity passes them. Executed against both variants:

  real guard      matches: true
  INVERTED guard  matches: true   <-- polarity flip still passes
  owner real: true  owner INVERTED: true
  

  An inverted guard rejects every well-formed digest, i.e. the escape hatch fails closed at exactly the moment an incident needs it, while malformed values fall through to the credential-write path. This is the same class of hole the comment at :96-103 documents about the earlier draft — a pattern loose enough to pass on code that does not have the property.

  • Drop the optionality: require the ! literally (\[\[ ! "\$\{ABANDON_IN_FLIGHT\}" =~ …). Nothing in the step legitimately renders without it.

• [tests] scripts/check-docker-abandon-in-flight-inputs.test.js:91 — the pairing assertion matches only the message text, not the condition that emits it. I neutered the guard to if false; then and left the echo in place; the assertion still passes:

  pairing-message assertion still passes on gutted guard: true
  

  The stated security property of this guard — reject before the approver credential reaches disk (docker.yml:786-788) — can therefore lapse silently while the test stays green, leaving approve-paperclip-api-digest.sh:121 as the only enforcement, after the credential is already written.

  • Assert the condition (\[ -z "\$\{ABANDON_IN_FLIGHT\}" \] \|\| \[ -z "\$\{ABANDON_IN_FLIGHT_OWNER\}" \]), not the prose it prints.

Suggestions (2)

• [code] .github/workflows/docker.yml:794,798 — the two shape patterns are now duplicated from approve-paperclip-api-digest.sh:110,116. The test deliberately reads the env var names out of the script so a rename fails loudly; the patterns have no such binding. If the script's accepted shape ever widens, the workflow silently becomes the stricter gate and rejects values the script would accept. Same technique as the existing name check would close it.
• [tests] The six-case rc matrix in the PR description is the thing that actually catches both findings above, and it is not in the diff. Extracting the step's run: body and executing it — as you already did locally — would pin behavior rather than rendering, and would subsume the two regex fixes.

Strengths

• Injection-safe by construction, and in the right order: inputs arrive via env: and are never interpolated into the script body, and the ::warning:: at docker.yml:803 is reached only after both anchored regexes, so no operator-supplied text can carry a :: workflow command. The ordering is load-bearing and the comments say why.
• Validating before (umask 077; … > "${approver_kubeconfig}") is the right call — a malformed value never reaches the higher-privilege credential.
• Reading the env var names out of the approve script instead of restating them (:28-40) is the correct coupling direction for a two-file contract.
• The pr.yml step matches its neighbours exactly — if: ${{ !cancelled() }}, timeout-minutes: 1, node --test — no drift.
• required: false on both, so ordinary push deploys are genuinely untouched; 3 dispatch inputs total, well inside GitHub's 10-input cap.

On your question 2: confirmed — pr.yml:922-932 lists verify's needs and policy is absent, so this test reports red without gating merge. Following the existing convention here rather than widening verify in an unrelated PR is the right call; the gating decision is worth its own change.

Recommended Action

1. No Critical issues — nothing blocks merge.
2. Tighten the two assertions in check-docker-abandon-in-flight-inputs.test.js this cycle; both are one-line regex edits, and the file's whole purpose is regression detection.
3. Consider committing the six-case matrix and binding the shape patterns to the script opportunistically.

[kkroo]

@ally both Important findings are fixed in 76c96b3, and I fixed them by mutation-testing rather than by re-reading.

Finding 1 (inverted polarity). You were right that !? ? accepted a flipped guard, and right that it is the dangerous direction — an inverted check rejects every well-formed digest, so the hatch fails closed exactly during the incident it exists for, while malformed values reach the credential write. The optionality was there to accommodate the implementation's if [[ ! ... ]] form, which was the wrong way to resolve that mismatch. The ! is now required literally.

Finding 2 (message vs condition). Confirmed your if false; then result. The assertion now tests [ -z "${ABANDON_IN_FLIGHT}" ] || [ -z "${ABANDON_IN_FLIGHT_OWNER}" ] directly.

Suggestion 2, taken — it is the real fix. The six-case matrix now lives in the diff instead of the PR description. It extracts the step's real shell from docker.yml and executes it, so both findings are caught behaviorally regardless of what any regex says. The two accepting rows are deliberate: an always-failing guard would "gate" every bad case and still be broken.

Suggestion 1, taken. Both shape patterns are now read out of approve-paperclip-api-digest.sh and asserted against docker.yml, matching how the env var names were already bound.

Mutation results:

mutation	outcome
invert digest polarity	tests 1 (matrix) + 7 (shape) fail
gut pairing to if false; then	test 6 fails
unmutated	8/8 pass, docker.yml byte-identical

42/42 across this file and the sibling docker.yml tests; check-workflows-parse 11/11.

Not done, deliberately: this still does not unwedge production by itself, and I did not widen verify's needs to make policy gating — thanks for confirming that read; it belongs in its own change.

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used

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

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Both prior Important findings are fixed. I dispositioned them by re-running your mutations myself against the tree at this head rather than re-reading the assertions, since "the regex looks stricter now" is exactly the evidence that failed last round. Unmutated: 8/8 pass.

Prior Findings Dispositioned (2)

• prior:34d53ed important 1 — fixed — scripts/check-docker-abandon-in-flight-inputs.test.js:187,192 — the !? ? optionality is gone; the negation is now a required literal and bound to the abandon variables by name. Independently mutated docker.yml to drop the ! from the digest check and re-ran: tests 1 and 7 both fail (pass 6 / fail 2), so an inverted guard can no longer reach master. The executable matrix at :83-88 catches it a second, regex-independent way — a flipped =~ rejects the both valid row.
• prior:34d53ed important 2 — fixed — scripts/check-docker-abandon-in-flight-inputs.test.js:170 — the assertion now tests [ -z "${ABANDON_IN_FLIGHT}" ] || [ -z "${ABANDON_IN_FLIGHT_OWNER}" ] instead of the prose. Re-ran the if false; then gut: test 6 fails. Worth knowing precisely which test caught it — see suggestion 2.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [tests] .github/workflows/docker.yml:808 — the ::warning:: ordering is the one property in this guard that no test pins. I moved the echo above both shape checks and 8/8 still pass. With that ordering, an operator-supplied abandon_in_flight is echoed unvalidated, so ::stop-commands::/::add-mask:: in the input become live workflow commands. Impact is genuinely low — reaching this step already requires paperclip-production environment approval, so the actor is an authorized deployer and it is log manipulation, not escalation — which is why this is a suggestion and not a blocker. But it is the same shape as the two findings you just fixed: a documented property that can lapse green. An indexOf ordering assertion (warning index > both regex indices) closes it in one line.
  • Also a correction to my own prior review: I wrote that "the comments say why" the ordering is load-bearing. Re-reading :806-807, the comment explains why the warning exists ("never a silent side effect"), not why it must come last. The property was mine, not yours — so this is a gap in my previous claim as much as in the test.
• [tests] scripts/check-docker-abandon-in-flight-inputs.test.js:83-88 — the matrix asserts exit code and reached, but not which guard did the rejecting, so it cannot distinguish "rejected for the stated reason" from "rejected by accident". Concretely: under the gutted-pairing mutation the digest only and owner only rows still return code 1 — the empty counterpart incidentally fails the shape check — which is why test 6 rather than the matrix caught that mutation. Your table reports this honestly; I am noting the mechanism. Asserting a stderr fragment per rejecting row would make the matrix self-sufficient rather than leaning on the string-match test.

Strengths

• The matrix is the right instrument and it is built correctly. Extracting the step's real shell means polarity and condition mutations are caught behaviorally, independent of what any regex claims — and the two accepting rows are what make it more than a rejection test. An always-failing guard would pass a rejection-only suite while making the hatch unusable during the incident it exists for; both valid is what forecloses that, and it also catches a swap of the two shape checks.
• extractGuard fails loudly rather than silently testing nothing: both anchors assert.notEqual(..., -1), so moving either block throws instead of quietly extracting an empty string. That is the failure mode most "extract the real code" helpers get wrong.
• The coupling test at :197 binds in the correct direction — it reads the accepted shapes out of approve-paperclip-api-digest.sh:110,116 and asserts docker.yml contains them, so widening the script fails here rather than letting the workflow silently become the stricter gate. Same technique as the env-var-name binding, now applied to the patterns; suggestion 1 from last round, taken properly.
• I checked the set -u hazard rather than assuming: the step runs set -euo pipefail (:779) and both new vars render empty on push. That is safe, and the precedent is in this very step — APPROVER_KUBECONFIG (:760) can also render empty and is read as [ -z ... ] at :780, so empty-renders-as-set-but-empty is already load-bearing on the pre-existing path. No regression to the ordinary push deploy.
• Injection-safety is still structural, not incidental: inputs arrive via env: and are never interpolated into the script body, and validation precedes (umask 077; … > "${approver_kubeconfig}") so a malformed value never reaches the higher-privilege credential. test 1's REACHED_CREDENTIAL_WRITE sentinel pins exactly that.
• required: false on both inputs, 3 dispatch inputs total, and the pr.yml step matches its neighbours exactly (if: ${{ !cancelled() }}, timeout-minutes: 1, node --test) — no drift.

Recommended Action

1. No Critical or Important issues — this is good to merge.
2. Both suggestions are test-hardening on properties that currently hold; take them opportunistically, or fold them into whichever change next touches this guard.
3. Agreed on the two deliberate omissions: this does not unwedge production by itself, and making policy gate verify belongs in its own change.

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used

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

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Merge-only synchronize — no reviewable change since 76c96b3e, and I established that by measurement rather than by reading the commit subject. All three PR files are byte-identical across the two heads (blob SHAs 9055e754bf, 214b20e08c, 66f92aa791 on both), and 76c96b3e...006e1646 touches only vendor/paperclip-adapter-claude-k8s/ — three files that arrived from master, not from you.

Two things worth checking on a merge that a file-identity check alone would miss, both clean:

• The merge reverted nothing. master...006e1646 is exactly +55/-0, +14/-0, +215/-0 — the PR's own additions and nothing else, so master's side of the merge was not silently dropped.
• The test's cross-file dependency did not move underneath it. check-docker-abandon-in-flight-inputs.test.js reads approve-paperclip-api-digest.sh at :197 to bind the accepted shapes; that script is blob 74d6a92b21 at both heads and at master, so the coupling test is asserting against the same contract it was.

Re-ran the suite against the tree at this head: 8/8 pass. I also re-confirmed the regression detection is live here rather than inheriting last round's result — inverting the digest guard's polarity in docker.yml:798 gives 6 pass / 2 fail.

I re-derived the two-file contract independently rather than trusting my own prior review: approve-paperclip-api-digest.sh:107-108 reads both values through :- defaults, so the empty render on push is genuinely inert; and the shape patterns at docker.yml:798,802 are character-identical to the script's at :110,116.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [ci] The one red check, review, is not about this code — it is the commitperclip PR-description gate asking for four literal headings (## Thinking Path, ## What Changed, ## Risks, ## Model Used). Your body has ## Why / ## What / ## Verification, which covers the same ground substantively; the gate matches heading text, not content. It has fired identically at all three heads, so it is not a merge regression. mergeable_state is blocked and this is the check holding it — a heading rename is the whole fix.
• [tests] Both suggestions from the previous round are still open, which is expected given the files are untouched; neither blocks. Restating only the mechanism, not the argument: the ::warning:: at docker.yml:806 must be emitted after both shape checks or an operator-supplied value reaches the log unvalidated, and nothing pins that ordering; and the rc matrix asserts that a row was rejected, not which guard rejected it. Fold them into whichever change next touches this guard.

Strengths

• The property that matters most at 3am survives mutation: the escape hatch fails closed on a malformed value and open on a well-formed one, and the suite catches a polarity flip in either direction. An inverted guard would reject exactly the well-formed digest an operator pastes from the refusal message — precisely when the hatch is the only way out — and that can no longer reach master.
• Validation still precedes (umask 077; … > "${approver_kubeconfig}"), so a malformed value never reaches the higher-privilege credential. Ordering is structural, not incidental.
• Injection-safety is by construction: both inputs arrive via env: and are never interpolated into the script body, so no operator-supplied text can carry a :: workflow command.
• Merging master in rather than force-pushing kept the reviewed tree addressable — the previously attested head is still reachable, which is what let me prove file-identity instead of re-reviewing 284 lines from scratch.

Recommended Action

1. No Critical or Important issues. The code carries an approval from the previous head and nothing reviewable changed; this re-attests the current one.
2. Rename the four PR-body headings to clear the review gate — that is the only thing standing between this and merge.
3. The two test-hardening suggestions remain opportunistic.

[allyblockcast]

review is blocking this PR, and it will never go green on its own

Flagging because the standing read on BLO-31598 is that this PR is "a wait for policy". policy went green at 006e1646. The thing actually holding it is review, and it is not a wait — it needs a one-time edit that no push will produce.

Why it fails. review is the commitperclip PR-body gate, not a verdict on the code. Its log is just One or more quality gates failed. See commitperclip comment on the PR for details.; the comment at 03:01:00Z names four literally-matched headings this body does not have:

## Thinking Path · ## What Changed · ## Risks · ## Model Used

The content is already here — only the heading names differ. ## Why covers Thinking Path and ## What covers What Changed. So this is a rename plus two short additions, not new writing.

The self-perpetuating part: the gate re-runs on every push and re-reads the same body, so it fails identically each time. It ran at 00:36, 01:47 and 03:01 and returned the same four items. The master merge at 02:59 re-ran it and changed nothing. It is the same shape as the stale lock this PR exists to fix: a state that cannot clear itself, that everyone is waiting on.

Evidence it is genuinely merge-gating, since this body states the opposite ("verify is the only required context on master"):

• I scanned the last 40 merged PRs: review was success at head on 40 of 40, zero exceptions.
• The merge-queue ruleset (20487141) is grouping_strategy: ALLGREEN.
• There is no verify context at this head at all — commits/006e1646/status returns exactly one: review/ally-comment.

Not proof: branches/master/protection returns 403 to the App token, so I cannot read the required-contexts list directly and am reasoning from behaviour. But 40/40 with no counter-example is the strongest available signal, and the cost of acting on it is one body edit.

Ally's APPROVED at 006e1646 is not stale — it survived the master merge, so nothing here needs re-review.

Ready-to-paste remedy

Rename ## Why → ## Thinking Path and ## What → ## What Changed, then add:

## Risks
Low for ordinary deploys: both inputs render empty on `push`, which the script
treats as "no abandon requested" via its own `:-` defaults, so the unmodified
deploy path is unchanged.

The hazard this is shaped around is a casually-passed abandon: retiring the lock
of a rollout that is genuinely in flight would reopen the approval ring
underneath it. That is why the two values are required together (a config-only
release can reuse a digest, so the owner is what identifies *which* approval is
retired), why both are validated before the approver credential is written to
disk, and why retiring emits `::warning::` rather than passing silently.

## Model Used
- <model + version you used>

I have deliberately not edited the body myself: ## Model Used is an attestation about how you worked and is not mine to author. Say the word and I will apply the rename and the ## Risks block and leave ## Model Used for you.

One caveat on mechanics: commitperclip's own comment says "push a new commit and these checks will re-run", so a body edit alone may not re-trigger it — after editing, re-run the review job (or fold it in with your next push).

— Release Engineer, 2026-09-04

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used

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

— commitperclip