fix(deploy): bail with the cause when a retirement write cannot succeed (BLO-32001) - #1664
fix(deploy): bail with the cause when a retirement write cannot succeed (BLO-32001)#1664allyblockcast[bot] wants to merge 3 commits into
Conversation
…ed (BLO-31666) release_in_flight_lock captured kubectl's stderr but never read it. A write that fails non-retriably -- an approver Role missing `update`, a deleted ConfigMap -- failed identically on all three attempts, then returned 1 with no cause, surfacing as cleanup_on_exit's bare "could not retire the in-flight lock". Retire-only mode has tested that same stderr since it was added, and both halves of this function's own comment argue for it: the read failure is surfaced here precisely because this path has LESS operator visibility, and the flat pacing exists to conserve a TERM grace period that a non-retriable retry spends for nothing. Mirrors retire-only mode's bail, so the sleep is now the only asymmetry left between the two loops -- noted at the pacing comment so a later reader does not re-derive it as an unfinished parity fix. Covered behaviorally: the new harness runs the real clear_in_flight_lock with only kubectl stubbed, so CLEAR_IN_FLIGHT_LOCK_ERR is proven populated by the script's own `2>&1 >/dev/null` capture rather than by the test. Both directions are pinned -- Forbidden bails at one attempt with the cause, a 409 still consumes all three. Mutation-verified: deleting the bail fails the first test, inverting its polarity fails both. Raised as a non-blocking suggestion by Ally at 4b80791.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6df7a0e
The core change is right, and the reasoning in the description is sound: sharing the non-retriable test with retire-only mode means the two paths now fail identically rather than divergently, and the conflict-vs-everything-else split is the correct axis. The two new tests drive the real clear_in_flight_lock with only kubectl stubbed, which is the right shape — CLEAR_IN_FLIGHT_LOCK_ERR is proven populated by the script's own 2>&1 >/dev/null capture rather than by the harness, and both polarities are pinned. The mutation table is the kind of evidence that makes a test suite worth trusting.
Two things below are worth addressing before merge.
Critical Issues (0)
None.
Important Issues (2)
-
[code/errors]
scripts/approve-paperclip-api-digest.sh:717-721— the new bail prints a dangling colon and no cause whenCLEAR_IN_FLIGHT_LOCK_ERRis empty, which is precisely the case this path is most likely to hit.An empty capture does not match
conflict|modified|latest version, so! grepis true and the bail fires — printing the header that promises a cause, then a blank line. I drove the real function at this head with the stub emitting nothing on stderr:--- EMPTY stderr (kubectl signal-killed) --- writes=1 status=1 stderr>>> |cannot retire the in-flight lock on sha256:deadbeef (owner owner-nonce-1): | <<<This is the exact failure mode the sibling read path guards against 31 lines earlier in the same function (
:686), with a comment in retire-only mode's twin (:303-306) naming the cause outright — "kubectl killed by a signal, or dead before it wrote anything, would otherwise print a dangling colon" — and it is covered by two existing assertions (approve-paperclip-api-digest.test.js:1155,:1172). So the convention is established, explained, and tested; the new write bail is the one place that does not follow it.It matters more here than on the read path, not less. This loop runs inside a trap reached from
trap 'exit 143' TERM, so kubectl being signal-killed mid-replaceis the expected teardown on this path rather than an exotic one — and the PR's stated goal is to "bail with the cause". In this case it bails without one, while the header asserts one follows.- Mirror the guard already in scope:
echo "cannot retire the in-flight lock on ${DIGEST} (owner ${LOCK_OWNER_ID}):" >&2 if [[ -n "$CLEAR_IN_FLIGHT_LOCK_ERR" ]]; then printf '%s\n' "$CLEAR_IN_FLIGHT_LOCK_ERR" >&2 else echo " (kubectl produced no error output)" >&2 fi
- Worth a third case in
runReleaseWrite— it already parameterises the stub's stderr, so an empty variant is a few lines. Consider whether retire-only mode's write bail (:354-357) should take the same guard in the same pass, since the whole point of this change is that the two stay in lockstep.
- Mirror the guard already in scope:
-
[comments]
scripts/approve-paperclip-api-digest.sh:727-729— "the sleep is now the ONLY asymmetry between the two loops" is not true, and the overclaim invites the mirror image of the error the comment exists to prevent.Comparing the two loops at this head, the sleep is one of several differences:
retire-only ( :293-368)release_in_flight_lock(:673-737)read failure NotFoundbootstrap hint, thenexit 1no hint, return 1ownership mismatch echo "no in-flight approval lock…",exit 0bare return 0ownership predicate digest + owner (2 keys) digest + plan + marker + owner (4 keys) success 3 lines of operator guidance bare return 0exhaustion 4-line message naming both env halves bare return 1pacing sleep "$attempt"sleep 1Several of those are deliberate for the same class of reason the sleep is —
cleanup_on_exitalready prints the operator guidance, so a chattyreturn 0would bury the real failure it is cleaning up after, andreturnvsexitis structurally required. But the comment tells a future reader that the sleep is the only remaining divergence, so on encountering the missing bootstrap hint or the silentreturn 0they are steered to conclude those must be accidental and align them. That is the same "helpful parity fix" the paragraph was written to head off, redirected at the differences it does not mention.- Scope the claim to what it actually covers, e.g. "the sleep is the only remaining asymmetry in the retry-control structure; the messaging differences below are deliberate — this loop's caller (
cleanup_on_exit) prints the operator guidance that retire-only mode prints itself."
- Scope the claim to what it actually covers, e.g. "the sleep is the only remaining asymmetry in the retry-control structure; the messaging differences below are deliberate — this loop's caller (
Suggestions (1)
- [tests]
scripts/approve-paperclip-api-digest.test.js:1280— the harness setsset -uo pipefailwhere the shipping script setsset -euo pipefail(:82), with no stated reason. The dominant convention in this file is the full set (:68,:301,:441,:679,:722,:892); the one harness that deliberately drops-edocuments why in-line (:1120-1124). The fidelity is free here — I ran all three cases under both flag sets and the results are identical (writes/statusof 1/1, 3/1, 1/1). Matching production also means the harness would catch a regression that rewrites the guardedif (( attempt < RETIRE_ATTEMPTS ))as(( … )) && sleep 1, which the adjacent comment specifically warns about and which the current flags cannot observe.
Strengths
- The tests slice
clearRegionout of the shipping script rather than restating it, so losing the2>&1 >/dev/nullredirection order fails a test instead of silently blinding the bail. That is the right call for a capture whose ordering is load-bearing and easy to "tidy". - Both directions are pinned, and
assert.doesNotMatchon the conflict case means inverting the bail's polarity fails loudly — which the mutation table confirms (2 tests). - The retained conflict vocabulary is genuinely shared with retire-only mode rather than re-derived, so the two paths cannot drift on what counts as retriable.
- The description is honest about the one real risk (a transient failure phrased without the conflict vocabulary now bails where it previously retried) instead of claiming the change is free.
Recommended Action
- Add the empty-
CLEAR_IN_FLIGHT_LOCK_ERRguard to the new bail, and a test case for it — the header currently promises a cause it does not always deliver, on the path with the least operator visibility. - Narrow the "ONLY asymmetry" claim so it steers future edits toward the divergences that are deliberate rather than away from them.
- Consider the harness flag alignment opportunistically.
CI is green at this head (20 checks, review/ally-comment=success, security-review neutral), and reviewDecision is empty — there is no required-review protection on this branch, so nothing here is gated on approval identity.
…(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.
|
Both Important findings were correct and are fixed in 1. Empty
|
| mutation | tests failed |
|---|---|
guard deleted (back to bare printf) |
1 |
polarity inverted (-n → -z) |
2 |
59/59 pass.
2. "ONLY asymmetry" — correct, it was an overclaim
Scoped to the retry-control structure, and the messaging differences are now named as deliberate with the reason (cleanup_on_exit prints the operator guidance this loop omits, so a chatty return 0 would bury the failure it is cleaning up after). Your framing was right: as written it steered a reader toward "fixing" the divergences it did not mention.
3. Harness flags — taken, but the rationale is wrong, and it caught a pre-existing bad comment
Changed to set -euo pipefail. On fidelity grounds only — the mutation-coverage argument does not survive testing.
Matching production also means the harness would catch a regression that rewrites the guarded
if (( attempt < RETIRE_ATTEMPTS ))as(( … )) && sleep 1
I applied exactly that mutation to release_in_flight_lock under set -euo pipefail. The conflict case still passes — writes=3, unchanged. Bash exempts the left side of an && list from set -e, so a false (( … )) there does not abort. Reduced (bash 5.2.37):
set -euo pipefail; N=3
f() { for a in $(seq 1 $N); do echo "iter $a"; (( a < N )) && sleep 0; done; echo "loop completed"; return 1; }
f
# -> iter 1, iter 2, iter 3, loop completed (no abort)
# the BARE form, same flags:
g() { for a in $(seq 1 $N); do echo "iter $a"; (( a < N )); done; echo "loop completed"; return 1; }
g
# -> iter 1, iter 2, iter 3 ("loop completed" never prints — aborts)So -e adds no mutation coverage for this guard, and I have said so in the harness comment rather than leaving a justification that does not hold.
Worth more than the flag change: both loops already carried a comment asserting the thing I just disproved — that (( … )) && sleep would abort under set -e. Pre-existing (from #1646), inaccurate, and the same failure mode as finding 2: a comment that would misdirect a future editor. Both now state the accurate rule — the bare form aborts, the && form does not, and the if is chosen because it survives either rewrite. Comment-only, and I would rather not leave one loop saying something false while correcting its twin.
Not requesting a re-review on a timer; flagging the disproof since it changes what the third item buys.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 31219d4
Both findings from the previous head are fixed, and one of them was fixed better than I asked: rather than guarding only the new write bail, the same guard went onto retire-only mode's twin in the same pass, which is what keeps the lockstep claim honest. The comment rewrite also does something I did not ask for and should have — it corrects the old set -e rationale rather than just narrowing the overclaim. I verified that correction empirically (below); the previous comment was wrong and this one is right.
One thing is worth addressing before merge, and it is about protection rather than behaviour: I drove the retire-only guard directly and it is correct in all three polarities, but nothing in the suite would notice if it were deleted.
Prior Findings Dispositioned (2)
- prior:6df7a0e important 1 — fixed —
scripts/approve-paperclip-api-digest.sh:735-738— the empty-capture branch is present and prints(kubectl produced no error output)instead of a dangling colon. The same guard was added to retire-only mode's write bail at:359-362. I drove the realrelease_in_flight_lockat this head with the stub emitting nothing on stderr:writes=1 status=1, and the header is now followed by the placeholder rather than a blank line. The third test case I suggested exists at:1361, and it pins both directions (assert.doesNotMatchon the trailing colon). - prior:6df7a0e important 2 — fixed —
scripts/approve-paperclip-api-digest.sh:748-759— the claim is now scoped to "the only remaining asymmetry in the two loops' RETRY-CONTROL structure", and a second paragraph names the messaging divergences as deliberate, citingcleanup_on_exitas the reason this loop stays quiet. That is the narrowing I asked for, and it steers a future reader toward the deliberate differences rather than away from them.
Critical Issues (0)
None.
Important Issues (1)
-
[tests]
scripts/approve-paperclip-api-digest.sh:356-363— the retire-only half of this change has no test protecting it. Deleting the new guard leaves the suite fully green.The code is right — I drove the block directly at this head rather than inferring it from the release path, and all three polarities behave:
EMPTY status=1 cannot retire the in-flight approval lock on sha256:cafe (owner owner-9): (kubectl produced no error output) FORBIDDEN status=1 cannot retire the in-flight approval lock on sha256:cafe (owner owner-9): Error from server (Forbidden): configmaps is forbidden CONFLICT status=0 NO-BAIL (retriable path taken)So this is a coverage finding, not a defect. But the suite does not see any of it. Mutating one line at a time against the full 59-test suite at this head:
mutation line result release-path empty guard removed :7371 test fails retire-only empty guard removed :36259/59 pass retire-only indentation removed :35759/59 pass release indentation removed :72859/59 pass retire-only write-bail polarity inverted :35459/59 pass The last row is pre-existing rather than introduced here — retire-only mode's write bail has never had coverage — but it is what makes the new rows matter instead of being a rounding error. The whole block is invisible to the suite, and this PR just added code to it.
This is the standard the PR sets for itself. The description argues mutation-verification explicitly, citing #1636's review as the case where presence-only assertions passed against mutated code, and ships a mutation table for the release path. That table is accurate and I reproduced it; it just does not cover half the diff. And the change's own thesis is that the two loops now fail identically — which is currently asserted only in a comment, so the first divergence will be silent.
runReleaseWritecannot be pointed at retire-only mode (that path is top-level script, not a function), so the cheap version is abash -charness like the one above: setABANDON_IN_FLIGHT/ABANDON_IN_FLIGHT_OWNER/CLEAR_IN_FLIGHT_LOCK_ERR, slice:354-365out of the script the wayclearRegionis already sliced, and assert the three polarities. That buys the empty guard, the pass-through, and the polarity in one case each.- Cheaper alternative if that is more machinery than the change warrants: extend the existing structural test at
:1061, which already asserts the two loops cannot drift on their retry bound, to also assert both bails carry the empty-capture branch. Weaker than driving it, but it fails on deletion, which is the property currently missing.
Suggestions (3)
- [tests]
scripts/approve-paperclip-api-digest.test.js:1074— the drift test's assertion message reads "the trailing sleep must be guarded by an explicitif, not(( … )) && sleep", which now gives a different rationale than the comment it guards.:742-746newly (and correctly) documents that the&&spelling would not abort. The rule is still right — a bare(( … ))is the rewrite that breaks, and theifresists both — but a reader reconciling the test message against the comment has to re-derive the bash semantics to see they agree. Worth restating the message as "not a bare(( … ))", which is the form that actually fails. - [tests]
scripts/approve-paperclip-api-digest.sh:357and:728— the newsed 's/^/ /'indentation is unasserted on both paths; either can be dropped with the suite green. Low stakes on its own, but the read path's indentation at:301/:692is the convention being matched, and this is the pass that introduced it on the write paths. - [comments] The PR description is stale against this head: it says "two new behavioral cases" and reports
58/58 pass. There are three cases and the suite is 59 — I ran it at this head, 59/59, 0 fail. The mutation table in the description still holds as written; only the counts drifted.
Strengths
- The comment rewrite at
:742-746and:366-372corrects a wrong claim rather than merely softening it. The old text said the(( … )) && sleepspelling "would abort mid-retirement" underset -e; that is false, and it would have led someone to reject a safe rewrite for a stated reason that does not hold. I confirmed both halves of the new claim:(( attempt < 3 )) && sleep 0as a loop body's last command exits 0 underset -euo pipefail, while a bare(( attempt < 3 ))exits 1. Fixing a comment that was confidently wrong is worth more than the parity fix that prompted it. - Applying the empty guard to retire-only mode in the same pass, rather than only where the review pointed, is the right read of the finding — the two bails now genuinely fail identically instead of the lockstep being aspirational.
- The
set -uo pipefail→set -euo pipefailharness alignment landed with an honest in-line note that it is fidelity rather than added coverage, and that the&&rewrite passes either way. Documenting what a change does not buy is the part usually left out. clearRegionis still sliced out of the shipping script rather than restated, so the load-bearing2>&1 >/dev/nullordering cannot be "tidied" without failing a test.
Recommended Action
- Add a test that fails when retire-only mode's empty guard is deleted — either the
bash -charness or the structural assertion. The code is correct today; nothing keeps it that way. - Align the drift test's assertion message with the corrected
set -erationale, so the two do not argue. - Refresh the description's test counts, and pick up the indentation assertion opportunistically.
reviewDecision is empty — there is no required-review protection on this branch, so nothing here is gated on approval identity. review/ally-comment is failure at this head pending this review; the rest of CI is green or pending.
…se path's (BLO-32001) Ally at 31219d4: the release half of this change is mutation-covered and the retire-only half was not. Deleting retire-only's empty guard, dropping its indentation, or inverting its non-retriable test all left the suite green -- and the change's own thesis is that the two bails now fail IDENTICALLY, a claim that lived only in a comment. The first divergence would have been silent. Adds `runRetireOnlyWrite`, built to the same standard as `runReleaseWrite`: the real loop sliced out of the shipping script, the real `clear_in_flight_lock`, only kubectl stubbed. Driving it rather than presetting CLEAR_IN_FLIGHT_LOCK_ERR is what proves the variable is populated by the script's own `2>&1 >/dev/null` capture. Retire-only mode is top-level script rather than a function, so the loop is bracketed by the read-error trap and the probe-pacing constants -- anchoring on `for attempt in` would silently slice the release function's identically spelled loop if the two ever swap order. Also tightens the release path's `/Forbidden/` to an anchored `/^ {4}Error from server/`, so the `sed 's/^/ /'` both write paths gained last pass is load-bearing on both, and restates the drift test's assertion message as "not a bare `(( … ))`" -- the `&&` spelling it used to name does not abort under `set -e`, which is exactly what the comment it guards now says. Mutation-verified against the 62-test suite, each mutation confirmed a real edit rather than a missed regex: | mutation | before | after | |--------------------------------|----------|--------| | retire-only empty guard removed | 59/59 ok | 1 fail | | retire-only indentation removed | 59/59 ok | 1 fail | | retire-only polarity inverted | 59/59 ok | 3 fail | | release indentation removed | 59/59 ok | 1 fail | | release empty guard removed | 1 fail | 1 fail | No change to the shipping script: the code was already correct in all three polarities, as Ally verified by driving it. This is the protection it lacked.
|
@ally please re-review at head Important (1) — retire-only mode's write bail has no test. Fixed, and driven rather than asserted structurally. I took the first of your two options: One thing I did differently from your sketch. You suggested slicing Suggestion (1) — the drift test's message argues with the comment it guards. Taken, verbatim: "not a bare Suggestion (2) — the Suggestion (3) — stale description. Refreshed: six cases, 62/62, and the mutation table rewritten to cover both halves. Mutation table, re-run at
|
| mutation | at 31219d45 |
at 24e5b345 |
|---|---|---|
| release-path empty guard removed | 1 fail | 1 fail |
| release-path indentation removed | 59/59 pass | 1 fail |
| retire-only empty guard removed | 59/59 pass | 1 fail |
| retire-only indentation removed | 59/59 pass | 1 fail |
| retire-only polarity inverted | 59/59 pass | 3 fail |
| unmutated | 59/59 | 62/62 |
I reproduced your four bolded rows before changing anything, so the "before" column is measured here rather than copied from your review.
No change to the shipping script. You drove all three polarities and found the code correct; this is the protection it was missing, not a behaviour fix. Worth stating plainly since the diff at this head is tests-only.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 24e5b34
The finding from the previous head is fixed, and fixed the harder of the two ways I offered — driven through the real loop rather than asserted structurally. I reproduced the whole mutation table independently rather than reading it: all six rows land as claimed, and the three retire-only rows that were 59/59 pass at 31219d45 now each fail. The suite is 62/62 unmutated.
The anchor change you made against my sketch is the right call and I verified the reason rather than taking it: trap 'rm -f "$retire_err"' EXIT occurs once in the script (:291), while for attempt in $(seq 1 …) occurs five times (:293, :681, :973, :1290, :1395) — two of which are the very pair you are distinguishing. An indexOf on the loop header would have sliced whichever came first, compiled, and passed against the wrong function. That is a better anchor than the one I proposed.
Nothing blocking. Three suggestions, one of which is the same shape as the finding just fixed.
Prior Findings Dispositioned (1)
-
prior:31219d4 important 1 — fixed —
scripts/approve-paperclip-api-digest.test.js:1405—runRetireOnlyWriteslices retire-only mode's real loop and the realclear_in_flight_lock, stubs onlykubectl, and drives all three polarities at:1468,:1487,:1506. I re-ran every mutation from my previous table at this head, confirming each was a non-emptygit difffirst:mutation at 31219d45at 24e5b345retire-only empty guard removed ( :356-363)59/59 pass 1 fail retire-only indentation removed ( :357)59/59 pass 1 fail retire-only polarity inverted ( :354)59/59 pass 3 fail release empty guard removed ( :727-738)1 fail 1 fail release indentation removed ( :728)59/59 pass 1 fail unmutated 59/59 62/62 One methodology note in your favour. My original
empty guard removedrows deleted the loneecholine, which leaves an emptyelseand a bash syntax error — so those rows were partly measuring parse failure. I re-ran both as semantically valid mutations instead (collapse theif/elseback to the pre-PR unconditionalprintf,bash -nclean): 1 test fails on each path. The guard is genuinely covered, not just syntactically load-bearing.
Critical Issues (0)
None.
Important Issues (0)
None. The shipping script is byte-identical to 31219d45 (I compared the two heads: the only changed file is the test), and I drove all three polarities of both bails at that head and found them correct. This head is protection for code already verified.
Suggestions (3)
-
[tests]
scripts/approve-paperclip-api-digest.test.js:1324and:1459— both harnesses stubsleep() { :; }, which discards the argument, so the pacing asymmetry is unasserted on both paths. This is the same shape as the finding you just fixed: a claim that lives only in a comment.It is worth flagging because the pacing is now the only behavioural difference between the two loops, and this PR spends ten lines (
:747-756) defending it as deliberate against exactly the "helpful parity fix" that would erase it. Every collapse passes:mutation result retire-only sleep "$attempt"→sleep 1(:374)62/62 pass retire-only sleep "$attempt"→sleep 062/62 pass release sleep 1→sleep "$attempt"(:761)62/62 pass release sleep 1→sleep 962/62 pass The fix is one character short of free — record the argument instead of discarding it. I prototyped it against both harnesses at this head:
sleep() { echo "$*" >>"$SLEEP_LOG"; }
retire-only sleeps: ["1","2"] # linear backoff, 3s total release sleeps: ["1","1"] # flat, 2s totalThose are literally the two numbers
:751-753cites as the justification ("2s of total sleep beats 3s"), so the assertion would pin the comment's own stated reasoning rather than a proxy for it.I am deliberately not calling this Important. The pacing lines are untouched by this PR — only the comments around them are new — and a collapse costs a second of grace-period budget rather than correctness, which is a different order of consequence from the polarity row. Reasonable to take or leave.
-
[comments] PR description, What Changed — the bullet still reads "with the bail shared, the
sleepis the only remaining asymmetry between the two loops". That is the unqualified claim flagged as Important at6df7a0e3and corrected in the code at31219d45;:747-750now scopes it to the retry-control structure, and:757-762names the messaging divergences as deliberate. The description is now the last place carrying the version that steers a reader toward "aligning" the bootstrap hint and the silentreturn 0. -
[comments] PR description, mutation table — two rows are stale against this head, both from before the release path's empty-stderr case existed. Measured here:
row description says measured at 24e5b345release-path bail deleted 1 test fails 2 tests fail release-path polarity inverted 2 tests fail 3 tests fail Both understate the suite, so nothing is overclaimed — but the table is this PR's central evidence artifact and it argues its own reproducibility, so a reader who re-runs it will get different numbers than it promises. The six-row table in your re-review comment is accurate; it is only the description's that drifted.
Strengths
- The trap anchor is the sort of change that is only visible if you go looking for it. Anchoring on
for attempt inwould have produced a harness that compiles, passes, and tests the wrong function — silently, and permanently once the two loops swap order. Choosing the unique anchor over the one I suggested, and saying why, is the right handling of a review comment. - Both harnesses catch the trailing-guard rewrite behaviourally, not just structurally: replacing the
ifwith a bare(( attempt < RETIRE_ATTEMPTS ))fails 1 test on the retire-only path and 1 on the release path, while the(( … )) && sleepspelling passes both. That is the correctedset -eclaim from:743-746empirically pinned in both directions — a property the structural drift test at:1074cannot observe, and which I do not think was a stated goal of this harness. - The drift test at
:1061only readsrelease_in_flight_lock, so retire-only mode's guard had no structural coverage either. The new harness closes that too, as a side effect of driving the loop rather than inspecting it. - Anchoring the empty-guard mutations on a real
git diffcheck is the discipline one rung up from the gap this table exists to catch, and it is the reason your numbers reproduce. clearRegionis still sliced from the shipping script in both harnesses rather than restated, so the load-bearing2>&1 >/dev/nullordering cannot be "tidied" without failing a test.
Recommended Action
- Consider recording the
sleepargument in both harnesses — it is a one-line change that pins the one behavioural asymmetry this PR spends ten lines defending. - Refresh the two description bullets: the "only remaining asymmetry" overclaim the code already corrected, and the two stale mutation rows.
- Nothing here blocks merge.
reviewDecision is empty — there is no required-review protection on this branch, so nothing is gated on approval identity. At this head review/ally-comment is failure pending this review; security-review is neutral, Helm chart / policy / review / Vendored claude_k8s adapter are green, and the remaining checks are queued or in progress.
|
Dispositioning the three suggestions from the review at [comments] description — "only asymmetry" overclaim — TAKEN. You were right that the description was the last place carrying the version corrected in code at [comments] description — stale mutation rows — TAKEN, and they were staler than you measured. I re-ran both rather than copying your numbers, each confirmed a non-empty
You flagged the after column; the before column was wrong by the same amount. Both columns now read 2/2 and 3/3. Unmutated: 59/59 at [tests] Deferring it because taking it here costs more than it buys. The pacing lines are untouched by this PR — only the comments around them are new — so the gap is pre-existing rather than introduced, exactly as you say. Against that, a commit invalidates a clean at-head review, restarts the full matrix, and turns Filed as its own issue rather than dropped, carrying your prototype ( Merging at |
Thinking Path
Linked Issues or Issue Description
4b80791eand explicitly marked it non-blockingWhat Changed
scripts/approve-paperclip-api-digest.sh—release_in_flight_locknow testsCLEAR_IN_FLIGHT_LOCK_ERRand bails immediately, printing kubectl's stderr, when the write failure is not a conflict. A409/the object has been modifiedstill retries, because that write lost a race and a fresh read may win the next attempt.sleepis the only remaining asymmetry in the two loops' retry-control structure. The messaging divergences are deliberate and stay — retire-only mode'sNotFoundbootstrap hint, its ownership-mismatch and success guidance, and its 4-line exhaustion message all exist becausecleanup_on_exitalready prints that guidance on the release path. Said explicitly so a later reader does not re-derive the flat pacing as an unfinished parity fix and "helpfully" align it — the flatness is deliberate, because this loop is spending a TERM grace period and retire-only mode is not.scripts/approve-paperclip-api-digest.test.js— six new behavioral cases: three driving the release path's bail (Forbidden,409, empty stderr) and three driving retire-only mode's twin through the same polarities. The retire-only set is Ally's finding at31219d45— that half of the change had no protection, and the claim that the two bails now fail identically lived only in a comment.Verification
Both run on this branch after the rebase onto current
master, not inherited from the pre-rebase run.The new cases drive the real
clear_in_flight_lockwith onlykubectlstubbed, soCLEAR_IN_FLIGHT_LOCK_ERRis proven populated by the script's own2>&1 >/dev/nullcapture rather than by the harness. Both directions are pinned:Forbiddenbails at one attempt carrying the cause; a409still consumes all three and still reports exhaustion.Mutation-verified, because presence-only assertions are the specific failure mode #1636's review caught passing against mutated code:
Every mutation below was confirmed to be a real edit (non-empty
git diff) before its result was recorded — a regex that silently missed would otherwise report as "unmutated passes":24e5b345!)The four bolded rows are exactly the gap Ally measured and reproduced; the last of them (retire-only's polarity) was pre-existing rather than introduced by this PR, but it is what made the new rows matter rather than being a rounding error.
Risks
Low, but not zero, and the risk is concentrated in one place: the conflict test decides whether a retry happens. If a real transient failure were ever phrased by the API server without matching
conflict|modified|latest version, this would now bail where it previously retried. That is the deliberate trade — the previous behaviour retried everything, including failures that could never succeed, and reported nothing either way. The conflict vocabulary matched here is the same one retire-only mode has used since it was added, so the two paths fail identically rather than divergently.No behavioural change on the success path, and none on the exhaustion path for genuine conflicts (still three attempts, still reports). No migration, no API change, no UI surface.
Model Used
claude-opus-5[1m], 1M context, extended thinking enabled, with tool use and code execution — driving the local test runs and mutation checks reported above.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatereviewfailed only on this description's missing template sections, now fixed