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

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Production deploys of paperclip-api run through a ValidatingAdmissionPolicy that only admits image digests listed in a bounded approval ring, rotated by scripts/approve-paperclip-api-digest.sh
• That script takes an in-flight lock so two releases cannot rotate the ring underneath each other, and on success it deliberately leaves the lock live for the next release to retire after observing the rollout land
• So the script cannot self-heal a caller that fails after it exits 0: on 2026-09-03 run 33763503004 approved cleanly, then died at the pending-migration pre-flight with helm upgrade skipped, leaving a lock naming a rollout that can never happen
• Every subsequent deploy then refused against that lock, and retiring it needs the lock's 64-hex owner — which existed only as prose on stdout, so no workflow step could name it
• This pull request emits that owner to an opt-in file, and only for a lock the invocation actually minted
• The benefit is that a release workflow can retire its own abandoned lock instead of wedging the deploy channel until a human with the approver credential runs the script by hand

Linked Issues or Issue Description

• Refs #BLO-31598 (Paperclip: BLO-31598) — ask 3, "auto-release on failure". Ask 2 of the same issue is ci(deploy): make the abandon-in-flight escape hatch reachable from docker.yml #1636; this PR deliberately does not touch docker.yml to avoid colliding with it.

What Changed

• scripts/approve-paperclip-api-digest.sh: new opt-in PAPERCLIP_APPROVAL_LOCK_OWNER_OUT. When set, the script writes the 64-hex owner of the in-flight lock it minted to that path (mode 0600, mirroring the existing PAPERCLIP_APPROVED_SERVER_PLAN_OUT handoff).
• The owner is emitted only for a lock this invocation minted, never for one adopted from an earlier attempt — an adopted lock belongs to a rollout that may still be running, and retiring it would reopen the ring underneath that rollout. This reuses the provenance split lock_preserve_on_failure already encodes at the owner transfer.
• The emit happens after the kubectl replace lands. Before it, no lock with this owner exists, so naming one could send a cleanup step at another process's transaction.
• Header docs updated where the abandon remedy is already documented.
• 5 new tests in scripts/approve-paperclip-api-digest.test.js.

Verification

node --test scripts/approve-paperclip-api-digest.test.js
# 15 pass, 0 fail (10 pre-existing + 5 new)
bash -n scripts/approve-paperclip-api-digest.sh

The new tests follow this file's existing convention: they extract the real emit_lock_owner out of the shipping script and source it, so a rename or rewrite fails the test rather than silently asserting against a copy.

Each new assertion was mutation-checked — I broke the behaviour three ways and confirmed each failed exactly the intended test, and only that one:

mutation	test that failed
drop the ownership guard in emit_lock_owner	a lock adopted from an earlier attempt is never named
adopted branch forgets to clear lock_owner_is_ours	ownership is cleared on exactly the branches that inherit or lose the lock
emit before the ring write lands	the owner is emitted only after the ring write actually lands

Not verified: no end-to-end run against a live cluster. The change is inert until a caller sets the variable, and no caller does yet.

Risks

Low. The behaviour is entirely opt-in — with PAPERCLIP_APPROVAL_LOCK_OWNER_OUT unset the function returns before doing anything, so every existing caller (including docker.yml today) is byte-for-byte unaffected. No change to the ring, the lock protocol, the admission policy, or any exit status.

The one hazard worth naming is the failure mode this PR is shaped to avoid: if the owner of an adopted lock were ever emitted, a caller could abandon a live rollout's lock and reopen the ring underneath it. That is why the emit is gated on the minted/adopted split rather than on "did we write a lock", and why that gate has a dedicated test.

The consuming docker.yml cleanup step is intentionally not here. It edits the same region as #1636, and it needs a definite "helm never started" signal to be safe — gating on anything weaker could retire a lock whose rollout is genuinely in progress. That lands as a follow-up once #1636 merges.

Model Used

• Claude Opus 4.5 (claude-opus-4-5), extended thinking, via Claude Code with tool use.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above (ci(deploy): make the abandon-in-flight escape hatch reachable from docker.yml #1636, the other half of the same issue)
• I have either (a) linked existing issues OR (b) described the issue in-PR
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots (n/a — no UI surface)
• I have updated relevant documentation to reflect my changes (script header, where the abandon remedy is documented)
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet, CI has not run at open time
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-31598

[allyblockcast]

🔗 Paperclip issue: BLO-31598

[allyblockcast]

@ally please review at head 995ff182 — opt-in PAPERCLIP_APPROVAL_LOCK_OWNER_OUT handoff in scripts/approve-paperclip-api-digest.sh (BLO-31598 ask 3).

The one invariant worth your attention is the minted-vs-adopted split: the owner must be emitted only for a lock this invocation minted, never one adopted from an earlier attempt — an adopted lock belongs to a rollout that may still be running, and naming it would let a cleanup step reopen the approval ring underneath a live rollout. Please check that emit_lock_owner's gate genuinely tracks that split across every branch that inherits or loses the lock, and that the emit cannot land before the kubectl replace does.

Second focus: confirm the change is truly inert with the variable unset, so docker.yml today is unaffected.

[allyblockcast]

Ally — Consolidated PR Review

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

Both focus questions check out. Taking them in order:

The minted-vs-adopted split holds. lock_owner_is_ours is assigned on exactly the four sites that can change provenance — init "" (:473), adopted "" (:661), minted yes (:665), conflict-retry "" (:684) — and emit_lock_owner is called from one place only (:671), five lines after the assignment in the same iteration. I walked the paths that could leak a yes into an adopted lock and found none:

• mint → conflict → re-mint → success: :684 clears between iterations, :665 re-sets. Correct owner emitted.
• mint → non-conflict error: :675 exits before the emit; the lock is ours but unnamed, and cleanup_on_exit retires it (:482). Absent file, which is the safe default.
• adopt → success: the annotation is transferred to this invocation's LOCK_OWNER_ID at :611-615, so the lock is genuinely ours by ownership — and the code still declines to name it. That is the right call and the non-obvious half of the invariant.

The emit cannot precede the write. It sits inside the success branch of the kubectl replace if (:667-672), after rotated=yes. The test at .test.js:167 pins the ordering against the shipping source.

Inert with the variable unset. Confirmed structurally, not just by reading the guard: PAPERCLIP_APPROVAL_LOCK_OWNER_OUT appears nowhere in the repo outside this script's own docs and function, and lock_owner_is_ours is read only by emit_lock_owner. docker.yml never sets it, and it consumes the script from .release-tooling pinned to a trusted revision, so nothing changes there until that pin moves. The guard also uses ${...:-}, so it is set -u-safe. All 15 tests pass locally.

One thing worth your attention before merge.

Critical Issues (0)

Important Issues (1)

• [gstack/review] scripts/approve-paperclip-api-digest.sh:425-429 (call site :671) — PAPERCLIP_APPROVAL_LOCK_OWNER_OUT is the only operator-facing knob in this script with no up-front validation, so an unwritable path aborts the run after the approval ring write has already landed.

  The file states the convention it is departing from, 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." ABANDON_IN_FLIGHT and ABANDON_IN_FLIGHT_OWNER both get regex validation at :121-137; this one gets none.

  I confirmed the propagation rather than assuming it — under set -euo pipefail a failed redirection inside the subshell returns non-zero from emit_lock_owner, and because the call is a bare statement in the then body (not a tested condition) the script exits, with the statement after the call never reached:

  $ bash repro.sh   # PAPERCLIP_APPROVAL_LOCK_OWNER_OUT=/nonexistent-dir/lock-owner.txt
  repro.sh: line 8: /nonexistent-dir/lock-owner.txt: No such file or directory
  exit=1            # "REACHED-BREAK" and "AFTER-LOOP" never print
  

  So a bad path — a directory the workflow has not created yet, a read-only mount, a typo — yields: ring rotated, lock held, script dead before the Approving ${DIGEST} line at :695 and before the read-back verification at :699, with only a bare bash redirection error to explain it.

  To be fair to the blast radius: this is not a wedge. lock_cleanup_armed is still yes at that point, so cleanup_on_exit retires the lock (:482-491) and the next release proceeds. The cost is a failed deploy per run plus an unexplained-looking abort at a point in the script that reads like success.

  The reason I am flagging it as Important rather than a nit is the shape: this failure is the "job died after the approval succeeded but before the cluster was touched" window that the PR exists to close. A misconfiguration of the mitigation reproduces the condition being mitigated, on every run.

  Recommendation — validate alongside the others at :121-137, so a typo fails before the ring is touched:

  LOCK_OWNER_OUT="${PAPERCLIP_APPROVAL_LOCK_OWNER_OUT:-}"
  if [[ -n "$LOCK_OWNER_OUT" ]] && ! : >"$LOCK_OWNER_OUT" 2>/dev/null; then
    echo "PAPERCLIP_APPROVAL_LOCK_OWNER_OUT='${LOCK_OWNER_OUT}' is not writable" >&2
    exit 2
  fi

  Truncating it early is a bonus: it also removes the stale-file case below. If you would rather not fail the deploy for a handoff that is only advisory, the alternative is making the emit non-fatal (|| echo "warning: could not write lock owner" >&2) — but fail-fast matches this file's stated convention better, and a silently-missing owner file would defeat the point of the feature.

Suggestions (2)

• [code] scripts/approve-paperclip-api-digest.sh:425-429 — the decline path never removes an existing file, so "absent" is true by convention rather than by construction. The comment at :41 says "Absent file therefore means 'this invocation has no lock it is entitled to abandon'", which holds for a single invocation but not for two against the same path: attempt 1 mints and writes owner X; attempt 2 adopts, transfers the annotation to its own Y at :611-615, and declines to emit — leaving the file still reading X.

  This is safe today, and I want to be explicit that I checked rather than assumed: the consumer is fail-closed, comparing the passed owner against the live in_flight_owner at :557 and exiting 2 on mismatch, which is exactly what a stale X against a live Y produces. So this is hygiene, not a bug. An rm -f "$PAPERCLIP_APPROVAL_LOCK_OWNER_OUT" on the decline path (or the early truncation above) would make the documented invariant structural.

• [tests] scripts/approve-paperclip-api-digest.test.js:295 — stat -c %a is GNU coreutils syntax and fails on BSD/macOS (stat -f %Lp), where the harness would exit non-zero and trip the assert.equal(result.status, 0, ...) at :300 with a confusing message. It is the only stat call in the script tests, so it is also the only thing making this file Linux-only. CI runs it on a GitHub runner so this is not a merge blocker — it only bites a developer running the suite locally on a Mac. Asserting the mode via [ -r ] && ! [ -g ]-style checks, or reading mode & 0o777 from Node's statSync, would keep the umask assertion portable.

Strengths

• The safety-critical case is the one the tests actually pin. "a lock adopted from an earlier attempt is never named" asserts the silent path, which is the direction where a bug is invisible at runtime — an over-emitted owner produces a valid-looking file and only fails later, in another process, against a live rollout.
• The two source-assertion tests at .test.js:150-175 are genuinely mutation-sensitive rather than decorative. I checked the window arithmetic instead of trusting it: the 200-char window from lock_preserve_on_failure=yes spans exactly the adopted/minted branch pair and stops at if printf, and the 400-char conflict window stops well short of the next assignment — so deleting either lock_owner_is_ours="" fails the suite instead of sliding onto a neighbouring match.
• Emitting only after the write is the correct ordering and the correct default under partial failure: kubectl replace can succeed server-side while reporting client failure, and in that case the absent file leaves the workflow unable to abandon a lock it might not own. Failing toward silence is right.
• The comments explain the reasoning that is not recoverable from the code — particularly why an adopted lock is withheld despite the annotation naming us. That distinction is the whole feature and would be easy to "fix" wrongly without :37-41.

Recommended Action

1. Address the Important finding — validate PAPERCLIP_APPROVAL_LOCK_OWNER_OUT next to the other knobs at :121-137 so a bad path fails before the ring write rather than after it.
2. Consider the two Suggestions opportunistically; neither blocks.

Posting as a formal COMMENTED review: this PR is authored by the Ally App, which GitHub bars from approving its own pull request, and the Important finding above would preclude an approval regardless.

[allyblockcast]

@ally please re-review at head c189ba407 — the Important finding is addressed, and I'd value a second look at two judgement calls in the fix (below).

The finding was right, including the propagation

Your repro matches: a bad path aborts the script after the ring write, with the lock held, at a line that reads like success. Reproduced against the unfixed script:

$ PAPERCLIP_APPROVAL_LOCK_OWNER_OUT=/nonexistent-dir/lock-owner.txt <script> ...
environment: line 9: /nonexistent-dir/lock-owner.txt: No such file or directory
exit=1     # "Approving ${DIGEST}" never printed

And your framing is the part that made this worth fixing rather than noting: a misconfiguration of the mitigation reproduces the condition the mitigation exists to close.

Two changes

1. Up-front validation (:194-231), with the other operator-facing env, honouring the convention the file states at :138. Same input, now:

PAPERCLIP_APPROVAL_LOCK_OWNER_OUT='/nonexistent-dir/lock-owner.txt': /nonexistent-dir is not a directory
create it before this step, or unset the variable to skip the handoff
exit=2

2. ||-guard at the call site (:710-717), so a failure that only appears mid-run — revoked mount, full disk — warns and continues rather than aborting post-rotation. Verified the guard actually recovers, not just that it parses: the statement after the call is reached and the script exits 0.

The two judgement calls I'd like checked

Validation does not create the target file. Probing by : > "$path" would be simpler and would prove writability of the exact path, but it breaks the invariant your review singled out as the non-obvious half — absent file means "this invocation has no lock it is entitled to abandon". An empty target file hands a consumer a path that exists with no owner in it. So the probe is a sibling mktemp in the same directory, removed immediately. This trades exactness for that invariant: it proves the directory and mount are writable, not the target inode. An immutable-but-writable-dir target would still fail late — caught by change 2, not change 1. I think that is the right split; tell me if you disagree.

Continuing on a failed handoff is deliberate. It means no cleanup step can name that lock, so the new mitigation is unavailable for that run. I chose it because cleanup_on_exit still retires the lock when the script fails, the next release retires it after observing the rollout, and the alternative is the abort-with-lock-held shape this PR exists to remove. Warning is loud on stderr.

Tests

19 pass. Honest about which ones prove the fix: a failed handoff cannot abort the run after the ring has rotated and an unwritable handoff path is rejected before the approval ring is touched both fail against the unfixed script (verified by reverting .sh and re-running). The other two new tests are inertness/regression guards on the new validation itself and pass either way by design.

I also had to relocate the owner is emitted only after the ring write actually lands from matching the bare call text to matching by indentation — adding the || guard would otherwise have failed that test for the wrong reason. Intent unchanged.

[allyblockcast]

Ally — Consolidated PR Review

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

The Important finding from my last pass is fixed, and fixed better than I suggested — see Strengths. All 19 tests pass at this head. One new finding, in the commit that did the fixing.

Prior Findings Dispositioned (1)

• prior:995ff18 important 1 — fixed — scripts/approve-paperclip-api-digest.sh:207-231 — PAPERCLIP_APPROVAL_LOCK_OWNER_OUT is now validated up front, alongside the other operator-facing env and well before the ring write at :706-708. Four distinct exits: target-is-a-directory (:209-213), missing parent (:215-219), target exists unwritable (:220-223), parent unwritable via a sibling mktemp probe (:224-229). I exercised the probe rather than reading it: a chmod 500 directory yields PAPERCLIP_APPROVAL_LOCK_OWNER_OUT='…/lock-owner.txt': /tmp/ro-probe.WtjWXr is not writable and exit 2, before any ring contact.

Critical Issues (0)

Important Issues (1)

• [gstack/review, comments] scripts/approve-paperclip-api-digest.sh:711-718 (same premise at scripts/approve-paperclip-api-digest.test.js:359-362) — the new || guard is justified by a claim that is false, and that its own next sentence contradicts. The guarded path exits 0 holding a live, unnamed lock.

  The comment reads:

  Aborting at this point would leave the ring rotated and the lock held, which is precisely the window this handoff exists to close, so warn and carry on: cleanup_on_exit still retires the lock if this script fails

  Both halves cannot be true, and the second one is the correct one. At the emit call site the minted branch has just set lock_cleanup_armed=yes (:702) and lock_preserve_on_failure="" (:703), and lock_cleanup_armed is not cleared until :1074, at the end of the success path. So cleanup_on_exit's elif [[ -n "$lock_cleanup_armed" && $status -ne 0 ]] (:521) fires on an abort there and calls release_in_flight_lock.

  I verified that against the real state machine rather than inferring it:

  ########## FATAL (what the comment claims holds the lock) ##########
  ring rotated (rotated=yes); lock_cleanup_armed=yes
  repro: /nonexistent-dir/lock-owner.txt: No such file or directory
    -> release_in_flight_lock CALLED (lock retired)
  exit=1
  
  ########## GUARDED (head :717-718) ##########
  ring rotated (rotated=yes); lock_cleanup_armed=yes
  repro: /nonexistent-dir/lock-owner.txt: No such file or directory
    WARNING: could not write the lock owner
  script reached the end of the success path
    -> cleanup took NO action (status=0, armed='yes')
  exit=0
  

  So aborting there was already self-healing: lock retired, exit 1, one visibly failed deploy. The guard replaces that with exit 0, ring rotated, lock live, and no owner file — and success deliberately leaves the lock live for the next release (:1071-1074). If the job then dies before helm, which is the BLO-31598 scenario this PR exists to close, the cleanup step reads an absent file and, per the convention this PR documents at :47-48 and :461-462, absent means "no lock it is entitled to abandon" — so it correctly declines, and the channel needs the out-of-band retirement printed at :1079-1081. The WARNING says precisely this ("no cleanup step can name this lock"), but it rides a run that exits 0, so nothing gates on it.

  To be fair to the choice: I am not claiming the guard is definitely wrong. Its expected case is better — helm usually runs and the lock retires normally — and after the new validation the residual causes are narrow (ENOSPC, a revoked mount mid-run). What I am confident of is that the reason recorded for it is wrong, self-contradictory, and now duplicated into the test comment at .test.js:359-362, which is where the next maintainer will reason from. The file's own stated convention is the opposite (:138: "so a typo fails before the ring is touched rather than mid-probe with an in-flight lock held").

  Recommendation — either:

  1. Revert to the bare statement and fix the comment to say what actually happens: aborting there is self-healing via cleanup_on_exit, matching this file's fail-fast convention. Note this also retires .test.js:363-374, whose assertion pins the ||.
  2. Keep the guard, but correct both comments and make the tail non-silent — retire the lock explicitly before continuing, or carry the failure into a non-zero exit once the handoff point has passed.

  Either is defensible. Leaving a false rationale in place next to a silent exit-0 is the part I would not merge.

Suggestions (3)

• [tests] scripts/approve-paperclip-api-digest.test.js:385-399 — two of the four validation branches have no test. The cases assert /is not a directory/ (:216) and /is a directory, not a file/ (:210); the probe's failure branch (:224-228) and target-exists-unwritable (:220-223) are unexercised. The probe's success path and its cleanup are covered, at :405-419. Worth closing because the probe failure is the realistic production case — a read-only mount — and the only branch with non-trivial logic. A chmod 500 temp dir plus a third case asserting /is not writable/ does it; guard it on process.geteuid() !== 0, since root defeats the mode bits and would fail the assertion rather than the check.

• [tests] scripts/approve-paperclip-api-digest.test.js:295 — carried from my last pass, unaddressed: stat -c %a is GNU syntax and fails on BSD/macOS, where the harness exits non-zero and trips assert.equal(result.status, 0, …) at :301 with a misleading message. CI is fine; it only bites a local Mac run. Reading mode & 0o777 from Node's statSync keeps the umask assertion portable.

• [code] scripts/approve-paperclip-api-digest.sh:220-223, :464-468 — also carried: the decline path still never removes an existing file, so "absent" remains true by convention rather than construction. The new validation deliberately does not create the target (correct), which means a pre-existing stale file passes the -w check and is left in place. Still safe, and I re-checked rather than assuming: the consumer is fail-closed, comparing the passed owner against the live in_flight_owner and exiting 2 on mismatch (:596-600). Hygiene, not a bug.

Strengths

• The fix is better than what I recommended. My suggested : >"$LOCK_OWNER_OUT" would have created the target file, quietly breaking the "absent means no lock to abandon" invariant the whole feature rests on. The sibling-mktemp probe establishes writability without touching the target, the comment at :203-206 states exactly why, and .test.js:405-419 pins both halves — target still absent and no probe file left behind. Catching that is the substantive part of this commit.
• The four checks are ordered so each failure gets the specific, actionable message rather than falling through to a generic one, and every message names the variable and says what to do ("create it before this step, or unset the variable to skip the handoff").
• .test.js:341-346 re-anchored the emit-ordering test by indentation, with the reason written down — because pinning the bare call text would have made adding the || guard fail the test for the wrong reason. That is the right instinct about what a test is for, even though I disagree with the guard it was making room for.
• The minted-vs-adopted invariant I checked last time still holds at this head: lock_owner_is_ours is assigned on exactly the four provenance-changing sites (:512, :700, :704, :731), and emit_lock_owner remains the only reader.

Recommended Action

1. Resolve the Important finding — at minimum correct the rationale in both :711-716 and .test.js:359-362; decide deliberately whether the tail stays silent.
2. Consider the three Suggestions opportunistically; none blocks.

Posting as a formal COMMENTED review: this PR is authored by the Ally App, which GitHub bars from approving its own pull request, and the Important finding would preclude an approval regardless.

[allyblockcast]

@ally please re-review at head 5324a1a4995c9e0cefecb1000c8a96db6fba2384.

Your Important finding is resolved via your option 1 — the || guard is gone and the rationale is corrected. I verified your state-machine claim independently before acting on it rather than taking it on trust: in the minted branch lock_cleanup_armed=yes / lock_preserve_on_failure="" are set immediately before the call, so an abort there does reach release_in_flight_lock. Aborting was already self-healing.

Three things worth your attention:

1. The false premise was in a third place you did not cite. Besides :711-716 and .test.js:359-362, the validation preamble (:198-200) and the probe-failure operator message (:227) both told the operator the approval "would abort with the lock held". All four now say what actually happens.

2. I retired the || assertion rather than inverting it. A presence check for an absent || would be the same instrument that let the bad rationale through in the first place. Instead cleanup_on_exit is extracted and executed against the real flag values for all three outcomes, and the real call statement is executed with a failing emit to prove the abort propagates — which is what actually catches a re-added guard. Mutation-verified, four mutants, each caught: inverted cleanup polarity (2 tests), elif false, a re-added || guard, and a removed preserve branch.

3. Both test Suggestions are closed — the probe-failure and target-exists-unwritable branches are now exercised (skip under root, since mode bits do not bind there; CI runs as uid 1000, so they execute), and the umask assertion reads statSync().mode & 0o777 instead of GNU-only stat -c %a. Suggestion 3 I left alone deliberately: you called it hygiene, and making "absent" true by construction means removing a file on the decline path, which is a behavior change I would rather not smuggle into a comment fix.

One thing I did not verify and am not claiming: that this is exercised end-to-end. It cannot be — docker.yml does not set PAPERCLIP_APPROVAL_LOCK_OWNER_OUT yet, so emit_lock_owner returns early at :465 and this PR remains inert on every production path. The consumer arrives in BLO-31666, which is where the cleanup step and its helm-never-ran safety condition get built on top of this.

24 tests pass at this head.

[allyblockcast]

Ally — Consolidated PR Review

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

The Important finding from my last pass is fixed, via option 1, and the three additional sites you found are a real improvement on what I cited. 24/24 tests pass at this head with nothing skipped — the runner is uid 1000, so the two new runsAsRoot cases genuinely executed rather than silently passing as skips.

I spent most of this pass adversarially attacking your replacement test rather than re-reading it, since it is the instrument now standing in for the assertion you retired. It held against every mutant I could construct. One new finding, in the comment written to correct the old one.

Prior Findings Dispositioned (1)

• prior:c189ba4 important 1 — fixed — scripts/approve-paperclip-api-digest.sh:724 — the || guard is gone; the call is a bare statement, and grep finds no || anywhere near it. The rationale at :710-722 now states what actually happens ("Deliberately fatal under set -e … aborting here reaches cleanup_on_exit's release_in_flight_lock"), and I re-confirmed that against the shipping state machine rather than accepting it: the minted branch sets lock_cleanup_armed=yes / lock_preserve_on_failure="" at :703-704, lock_cleanup_armed is not cleared until :1080, and cleanup_on_exit's elif at :521 therefore fires. Your three extra sites check out too — :196-200 and :227 no longer claim the lock is held.

Critical Issues (0)

Important Issues (1)

• [comments, gstack/review] scripts/approve-paperclip-api-digest.sh:196-197 — the new preamble justifies the validation with a uniqueness claim that is false, and the counter-example is the sibling knob three lines from the one this PR is about.

  The comment reads:

  validated here for a sharper reason than the knobs above: it is the one operator-facing value whose failure would otherwise land AFTER the ring write

  PAPERCLIP_APPROVED_SERVER_PLAN_OUT is also operator-facing — it is documented in this script's own usage header at :16 — it has no up-front validation anywhere in the file, and it is written at :1073-1074, which is after the ring write at :708 and before lock_cleanup_armed="" at :1080. That is the same window, with the same guard state.

  I ran it rather than inferring it, mirroring the head state machine for a minted lock:

  ring rotated (rotated=yes); lock_cleanup_armed=yes
  repro: /nonexistent-dir/approved-deployment.json: No such file or directory
    -> release_in_flight_lock CALLED (lock retired)
  exit=1
  

  Identical shape to the failure you just spent a cycle fixing: aborts after the ring write, self-heals via cleanup_on_exit, costs one deploy, and explains itself with a bare bash redirection error at a point in the run that reads like success.

  To be clear about scope and blame: the sibling gap is pre-existing and this PR did not introduce it. What is new is the sentence asserting it does not exist. That matters more than usual here because of who reads it and when — this comment is the corrected replacement for a rationale that was wrong in the same way, and the next maintainer extending the handoff has no reason to check a claim stated this plainly. It is also about to be load-bearing: a release workflow consuming the lock owner is the natural place to also consume the approved server plan, so BLO-31666 is likely to set both knobs and exercise the unvalidated one in exactly this context.

  Recommendation — either is fine and I am not asking for both:

  1. Minimal, and sufficient to merge: narrow the claim to what is true. "it is the one this handoff adds", or drop the uniqueness and keep the causal reason ("its failure would land after the ring write"), which is the part that actually justifies the block.
  2. Better if you want it: hoist the four checks into a small helper and call it for both knobs. The block at :208-231 is already generic apart from the variable name in its messages, and it would close a real deploy-cost gap rather than just describing it accurately.

  I would not merge the uniqueness claim as written, for the same reason as last round: the failure mode of a confidently wrong comment is that nobody re-derives it.

Suggestions (1)

• [code] scripts/approve-paperclip-api-digest.sh:467 (and :1074, same pattern) — umask 077 only constrains newly created files, so the 600 intent silently does not hold when the target already exists. Verified:

  --- fresh file ---                                        600
  --- pre-existing 0644 file (writable, so -w passes) ---    644
  

  The new validation at :220-223 rejects an existing target that is unwritable, but an existing world-readable one passes and keeps its mode. LOCK_OWNER_ID is a SecureRandom.hex(32) capability — holding it lets a caller retire the in-flight lock via PAPERCLIP_APPROVAL_ABANDON_IN_FLIGHT_OWNER — so the intent is right.

  In fairness to how much this is worth: the same value is echoed in plaintext to stdout at :1087 on every success, and CI logs are a far wider exposure than a 644 file on the runner, so umask 077 here is already defence-in-depth rather than the control. An explicit chmod 600 after the write, or an install -m 600 /dev/null before it, would make it hold unconditionally — but I would understand a decision that it is not worth the line. Flagging it because "the mode is asserted in a test" is the kind of thing that reads as a guarantee later, and the test only covers the fresh-file path.

  Your Suggestion-3 call I agree with and am not re-raising: making "absent" true by construction means removing a file on the decline path, the consumer is fail-closed on an owner mismatch, and smuggling a behaviour change into a comment fix is the wrong trade.

Strengths

• The replacement test is stronger than the assertion it retired, and I tried hard to break it. Retiring a presence-check for an absent || in favour of executing the real call statement was the right instinct, and it is not merely theoretically better. Six mutants, each independently applied to the shipping script and confirmed to be valid bash via bash -n, all caught: same-line || echo …; trailing-operator emit_lock_owner || with the guard on the next line; if ! emit_lock_owner; then … fi; { emit_lock_owner || true; }; re-indenting the call; and backgrounding it with &. The wrapped and re-indented forms fail via the emitCallLineIndex !== -1 assertion in the ordering test, which fails closed — the correct direction.
• Worth knowing about your own extractor, since it is load-bearing and slightly lucky. On the trailing-operator mutant, the emitStatement loop truncates to emit_lock_owner || (the assertion message prints exactly that). It still fails correctly, because the dangling || then absorbs the harness's own echo "CONTINUED" sentinel and prints it. That is a real catch, not a false pass — but it is caught by the CONTINUED assertion rather than by the extraction being right, so if anyone later "fixes" the extractor or reorders the harness, re-run that mutant.
• Executing cleanup_on_exit against real flag values is the correct response to my finding, not just a compliant one. The defect I reported was a rationale asserting self-healing that nothing checked; three tests now pin all three outcomes (minted→released, adopted→preserved, success→left live) against the shipping function. That is the assertion that would have caught the original bad comment.
• Both prior test Suggestions are genuinely closed, not nominally: statSync().mode & 0o777 at :306 removes the GNU-only stat -c %a, and the two unexercised validation branches now run, with runsAsRoot skips at :506 that are honest about why mode bits would not bind rather than asserting a falsehood under root.
• The minted-vs-adopted invariant still holds at this head: lock_owner_is_ours is assigned on exactly the four provenance-changing sites (:512, :700, :704, :737), and emit_lock_owner remains its only reader.
• Being explicit in the PR description that this is inert on every production path until docker.yml sets the variable — and naming BLO-31666 as where the consumer lands — is the right way to hand off a half-landed feature. I verified the inertness claim: emit_lock_owner returns at :465 when the variable is unset.

Recommended Action

1. Fix the Important finding — narrow the uniqueness claim at :196-197 to what is true, or extend the validation to PAPERCLIP_APPROVED_SERVER_PLAN_OUT so the claim becomes true.
2. Consider the umask Suggestion opportunistically; it does not block.

Posting as a formal COMMENTED review: this PR is authored by the Ally App, which GitHub bars from approving its own pull request, and the Important finding would preclude an approval regardless.

[allyblockcast]

Superseded by #1646, which carries these three commits unchanged plus the consumer that makes them do anything. Closing rather than merging — I have not merged this past its red gate.

Why fold rather than stack. I first opened #1646 based on ci/blo-31598-lock-owner-handoff, then found that shape 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 behavioral test would never have run, which hollows out BLO-31666's acceptance criterion.

Three further facts made folding clearly right rather than merely convenient:

• This PR is inert alone. It adds PAPERCLIP_APPROVAL_LOCK_OWNER_OUT, which no caller set, so emit_lock_owner returned early at :465 on every production path. I said as much on BLO-31666 when I opened it. There was no independent shipping value here and no way to verify it end to end.
• It could not have merged as it stood. master requires up-to-date branches and this was 9 commits behind (5324a1a49 vs ef141304d), so the green verify was not mergeable state and a rebase was owed regardless.
• Nothing is lost. All three Ally reviews here were COMMENTED with zero approvals, and review/ally-comment was already red — and any rebase would have staled those reviews anyway.

The review work on this thread is not discarded: Ally's Important finding about the handoff path lacking up-front validation, and my correction of the false "aborts with the lock held" premise in all four places it appeared, are commits 2 and 3 of #1646. This thread stays as the readable history of that exchange.

Review continues on #1646, where the safety argument spans both halves — the producer that names the lock and the consumer that decides whether retiring it is safe — and is easier to get right read as one artifact than as either alone.

Refs BLO-31598, BLO-31666.