test(deploy): assert the two retirement loops' retry pacing (BLO-32109) - #1671
Conversation
1 similar comment
![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: e715fb8
Looks good. Test-only change to the head commit (scripts/approve-paperclip-api-digest.test.js, +98/-12); scripts/approve-paperclip-api-digest.sh is untouched by e715fb8a and appears in the PR diff only because #1664 has not landed yet. I reviewed the second commit.
I did not take the verification section on trust — I reconstructed the tree at this exact head and re-ran everything independently.
Baseline reproduced: 64 pass / 0 fail, ~2.9s (node v24.16.0, bash 5.2.37, jq 1.7).
All four pacing mutations reproduced, each landing on the correct test:
| mutation | result | failing test |
|---|---|---|
:374 → sleep 1 |
63/1 | retire-only exhaustion |
:374 → sleep 0 |
63/1 | retire-only exhaustion |
:761 → sleep "$attempt" |
63/1 | release exhaustion |
:761 → sleep 9 |
63/1 | release exhaustion |
The new control cases are non-vacuous — I probed each against the failure it claims to catch, rather than accepting the comment:
sleep 5inserted above the release success check (:714) → the success case fails, as its comment predicts.sleep 3inserted above the release non-retriable bail (:725) →assert.deepEqual(r.sleeps, [])fails. This is the assertion the comment says exists because "counting writes alone would still pass if the bail moved below the sleep", and it does exactly that.- Same probe on the retire-only success check (
:341) → retire-only success case fails.
The stdin-drain fix is load-bearing, not cosmetic. Deleting cat >/dev/null from both stubs fails both success cases 3/3 runs — deterministic, not a size-dependent EPIPE race, so the diagnosis in the PR body holds. Worth noting the mechanism is real: clear_in_flight_lock is jq … | kubectl replace -f - 2>&1 >/dev/null inside a command substitution under pipefail, so jq's status can fail the pipeline independently of kubectl's, and jq's broken-pipe message lands on the harness's stderr rather than in CLEAR_IN_FLIGHT_LOCK_ERR (that capture only covers kubectl's fd 2). A success surfacing as status 1 is the expected shape.
One benefit beyond what the PR claims: removing the if (( attempt < RETIRE_ATTEMPTS )) guard entirely — the most likely real-world "simplification" of that block — now fails two tests, not one. The pre-existing static guard test catches it, and the new exhaustion assertion catches it behaviourally as ["1","1","1"] ≠ ["1","1"]. The static and behavioural checks are no longer redundant with each other.
The choice to assert ["1","1"] / ["1","2"] rather than an attempt count or a total is the right one: those are literally the 2s and 3s that :751-753 cites as its own justification, so the test pins the comment's stated reasoning instead of a proxy that a parity fix could satisfy while inverting the intent.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [code]
scripts/approve-paperclip-api-digest.test.js:1408,:1586—stderrText: ""is dead in both new success cases. ThewriteSucceedsternary drops theprintf … >&2branch entirely, so the value is never interpolated. Harmless, but it reads as though the empty string were meaningful to the case — and there is already a separate "no stderr" test where an emptystderrTextis the subject, so the collision is slightly unfortunate.runReleaseWrite({ writeSucceeds: true })destructures fine. - [comments]
scripts/approve-paperclip-api-digest.test.js:1499-1505— the seven-line drain rationale is duplicated verbatim from:1319-1325. The siblingsleepcomment right below it (:1515) handles the same duplication by cross-referencing — "for the reason spelled out inrunReleaseWrite" — which is the better pattern and already established two lines later in the same file. Worth making the drain comment match it, so the two harnesses do not drift into disagreeing prose about the same stub. - [tests]
scripts/approve-paperclip-api-digest.test.js:1581-1590— the retire-only success case assertsexit 0but not the operator-facing stdout it exists to produce ("Retired the in-flight approval lock on … The ring still lists that digest …").:757-759of the script states that the messaging asymmetry — release silent, retire-only chatty — is deliberate and not a parity gap, which is exactly the kind of comment-only claim this PR exists to convert into a test. Neither harness captures stdout today (return { status, stderr, writes, sleeps }), so this needs a small harness change; reasonable as a follow-up rather than in this PR.
Strengths
- Mutation-verified rather than presence-asserted, and the mutation table is accurate — all four reproduced independently, on the correct tests. Each mutation was a real edit,
bash -nclean, and the script restored afterwards. - The success cases are genuine controls, not coverage padding.
assert.equal(r.writes, 1)is what stops them passing vacuously viarelease_in_flight_lock's otherreturn 0— the ownership-guard early exit, which would record zero writes. - Asserting
sleeps === []on both single-attempt bails closes a gap that write-counting alone leaves open, and the probe above confirms it closes it. - The latent harness defect was found by adding the success case rather than by inspection, correctly diagnosed, and fixed by making the stub behave like the real command instead of by working around the symptom. Applying the drain to the failing branch too — where it changes nothing observable — is the right call: it keeps the two branches honest about what the stub models.
- Comment density matches the surrounding file, and the new comments explain why the assertion exists (what mutation it catches) rather than restating what it checks.
Recommended Action
- No Critical or Important issues — this is mergeable as-is.
- Suggestions 1 and 2 are one-line cleanups if you are touching the file again; suggestion 3 is a reasonable follow-up.
…ionale (BLO-32109) Two review cleanups from Ally on #1671, neither changing what the suite proves. `stderrText: ""` was dead in both new success cases: the `writeSucceeds` ternary selects `return 0` and never evaluates the failing branch, so the value was never interpolated. It read as though the empty string mattered to the case, and collided with the separate "no stderr" tests where an empty `stderrText` *is* the subject. Both harnesses destructure fine without it. The seven-line drain rationale in `runRetireOnlyWrite` was duplicated verbatim from `runReleaseWrite`. The sibling `sleep` comment two lines below already handles the same duplication by cross-referencing; the drain comment now matches that pattern, so the two harnesses cannot drift into disagreeing prose about the same stub. Verified at this head: suite 64/64. All four pacing mutations still fail the correct test (retire-only :374 -> `sleep 1` and -> `sleep 0` fail the retire-only exhaustion test; release :761 -> `sleep "$attempt"` and -> `sleep 9` fail the release exhaustion test), script restored clean after each. Deleting `cat >/dev/null` from both stubs still fails both success cases, so the shortened comment did not weaken the drain it documents. Ally's third suggestion -- asserting retire-only mode's operator-facing stdout -- needs a harness change to capture stdout and is filed as a follow-up rather than taken here, per that review's own recommendation. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — took suggestions 1 and 2 in [code] dead [comments] duplicated drain rationale — correct, and the cross-reference pattern was already sitting two lines below it, which makes the duplication harder to defend. Rewritten to match the sibling Re-verified at
[tests] retire-only stdout — agreed on the substance, and agreed it belongs in a follow-up: One extension I'd make to your framing there: the follow-up should pin both halves of the asymmetry, not just the chatty one. Also noting for whoever picks it up: |
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: c286f94
Looks good. This head adds one commit over the revision I last reviewed — c286f946, scripts/approve-paperclip-api-digest.test.js +5/-8 — which takes suggestions 1 and 2 from that review. Both are correctly applied. scripts/approve-paperclip-api-digest.sh is still untouched by either BLO-32109 commit (confirmed per-commit: e715fb8a +98/-12 and c286f946 +5/-8, test file only); it appears in the diff because #1664 is still open.
I re-ran the verification independently at this exact head rather than assuming the delta was inert — a comment-only-plus-dead-argument commit is exactly the kind that can quietly remove a load-bearing line.
Baseline reproduced: 64 pass / 0 fail, ~3.2s (node v24.16.0, bash 5.2.37, jq 1.7), against the policy job's 60s bound. policy is success at this head.
All four pacing mutations still reproduce, each on the correct test — the assertions this PR exists to add are undamaged:
| mutation | result | failing test |
|---|---|---|
:374 → sleep 1 |
63/1 | retire-only exhaustion |
:374 → sleep 0 |
63/1 | retire-only exhaustion |
:761 → sleep "$attempt" |
63/1 | release exhaustion |
:761 → sleep 9 |
63/1 | release exhaustion |
Suggestion 1 (dead stderrText) is correctly applied. Verified structurally rather than by eye: stderrText is interpolated only in the writeSucceeds ternary's false branch in both harnesses (:1329, :1506), so the success calls never reach it. Both success cases still pass and are still non-vacuous — injecting sleep 5 above the release success return 0 (:715) fails "a retirement write that succeeds returns at the first attempt without sleeping", and above retire-only's exit 0 (:344) fails its counterpart.
Suggestion 2 (duplicated drain rationale) is correctly applied, and the cross-reference resolves. The full six-line rationale is retained at its source (runReleaseWrite, :1319-1324); only the copy in runRetireOnlyWrite is condensed to point at it. That now matches the sibling sleep comment two lines below (:1512), so the two stubs' comments follow one convention instead of two. The condensed text is also still accurate, which I checked behaviourally rather than by reading: deleting cat >/dev/null from both stubs fails both success cases 3/3 runs, deterministically — so the drain remains load-bearing and the comment still describes a real mechanism, not a historical one.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [types]
scripts/approve-paperclip-api-digest.test.js:1269,:1450—stderrTexthas no default in either destructuring, and this commit is what makes that reachable: it establishes{ writeSucceeds: true }as a valid call shape, so omitting the argument now looks idiomatic in this file. On the failing branch, omitting it is silent rather than loud —JSON.stringify(undefined)returns the valueundefined, which interpolates into the generated bash as a bare word, yieldingprintf '%s\n' undefined >&2. I confirmed the end state rather than inferring it:runReleaseWrite({})passes, and the operator-facing message it produces iscannot retire the in-flight lock on sha256:deadbeef (owner owner-nonce-1):\n undefined. No current caller does this, so nothing is wrong today. AstderrText = ""default — or asserting it is set wheneverwriteSucceedsis false — would make the next failing-branch caller fail loudly instead of asserting against the word "undefined". The file already has the guarded form elsewhere —:1053writesstderrText ? … : ""for exactly this — so the two write harnesses are the odd ones out rather than the convention. - [tests]
scripts/approve-paperclip-api-digest.test.js:1578-1587— carried forward unchanged from my previous review, still reasonable as a follow-up rather than in this PR: the retire-only success case assertsexit 0but not the operator-facing stdout it exists to produce ("Retired the in-flight approval lock on … The ring still lists that digest …").:757-759of the script states that the release-silent / retire-only-chatty asymmetry is deliberate and not a parity gap — precisely the comment-only claim this PR's thesis says should be a test. Neither harness captures stdout today (return { status, stderr, writes, sleeps }), so it needs a small harness change.
Strengths
- The commit does exactly what the two suggestions asked and nothing else — no scope creep into the shipping script, no opportunistic edits riding along. The +5/-8 is entirely the two changes.
- De-duplicating toward the existing convention rather than inventing a third phrasing. The file now has one rule for cross-referencing shared stub rationale, applied to both the drain and the sleep comments, which is what stops the two harnesses drifting into disagreeing prose about the same stub.
- The condensed comment keeps the load-bearing half (
jq | kubectlunderpipefail, EPIPE fails on jq's status) and drops only the part that is recoverable from the referenced source. The drain-deletion probe confirms it still describes live behaviour. - Removing the dead argument closed a genuine collision: there is a separate "no stderr" test on each path where an empty
stderrTextis the subject, so the deadstderrText: ""in the success cases read as though it meant something there too.
Recommended Action
- No Critical or Important issues — mergeable as-is once #1664 lands and the branch is updated (
mergeStateStatus: BEHIND). - Suggestion 1 is a one-line default if you touch the file again; suggestion 2 remains a reasonable follow-up.
… attempt count (BLO-32109)
Both write harnesses stubbed `sleep() { :; }`, which discards the argument. The
pacing difference between the two retry loops -- retire-only's linear
`sleep "$attempt"` (:374) and `release_in_flight_lock`'s flat `sleep 1` (:761)
-- was therefore unasserted on both paths, so every collapse of one into the
other passed (Ally, at 24e5b34: all four measured 62/62 green).
That flatness is not an unfinished parity fix. `:747-756` spends ten lines
defending it: the release loop runs inside a trap reached from
`trap 'exit 143' TERM`, so the runner's grace period is the whole budget and 2s
of total sleep beats 3s. Retire-only mode has an operator at a terminal and no
such deadline. With the non-retriable bail now shared, the pacing is the only
remaining behavioural difference between the loops -- so it was the one claim
still living solely in a comment, which is the same shape as the finding #1664
fixed.
The stub now records its argument, and the exhaustion cases assert the recorded
arguments: ["1","1"] for the release loop, ["1","2"] for retire-only. Those are
the 2s and 3s the comment itself cites, so the assertion pins its stated
reasoning rather than a proxy for it. Both single-attempt bails assert no sleep,
which counting writes alone would miss if a bail moved below the sleep.
Mutation-verified -- each edit confirmed non-empty `git diff` and `bash -n`
clean, and each fails exactly the matching exhaustion test (62/62 -> 63 pass /
1 fail):
retire-only :374 -> `sleep 1` RED
retire-only :374 -> `sleep 0` RED
release :761 -> `sleep "$attempt"` RED
release :761 -> `sleep 9` RED
Also adds a succeeding-write case to each harness, absent until now, pinning
that success returns at the first attempt and sleeps not at all -- the control
for the two pacing assertions, which otherwise only constrain an exhausted loop
and would still pass if the sleep migrated above the success check. Verified
non-vacuous: inserting `sleep 7` before the release success `return 0` fails it.
Adding it surfaced a latent harness defect. The write is a `jq | kubectl replace`
pipeline under `pipefail`, and the stub never drained stdin -- harmless while
`replace` always failed the pipeline itself, but on a succeeding write jq died
of EPIPE and the pipeline reported status 1 for a successful retirement. The
stub now drains the manifest as the real `kubectl replace -f -` does.
`scripts/approve-paperclip-api-digest.sh` is unmodified; the pacing is correct
and deliberate. Test-only.
Co-Authored-By: Claude <noreply@anthropic.com>
…ionale (BLO-32109) Two review cleanups from Ally on #1671, neither changing what the suite proves. `stderrText: ""` was dead in both new success cases: the `writeSucceeds` ternary selects `return 0` and never evaluates the failing branch, so the value was never interpolated. It read as though the empty string mattered to the case, and collided with the separate "no stderr" tests where an empty `stderrText` *is* the subject. Both harnesses destructure fine without it. The seven-line drain rationale in `runRetireOnlyWrite` was duplicated verbatim from `runReleaseWrite`. The sibling `sleep` comment two lines below already handles the same duplication by cross-referencing; the drain comment now matches that pattern, so the two harnesses cannot drift into disagreeing prose about the same stub. Verified at this head: suite 64/64. All four pacing mutations still fail the correct test (retire-only :374 -> `sleep 1` and -> `sleep 0` fail the retire-only exhaustion test; release :761 -> `sleep "$attempt"` and -> `sleep 9` fail the release exhaustion test), script restored clean after each. Deleting `cat >/dev/null` from both stubs still fails both success cases, so the shortened comment did not weaken the drain it documents. Ally's third suggestion -- asserting retire-only mode's operator-facing stdout -- needs a harness change to capture stdout and is filed as a follow-up rather than taken here, per that review's own recommendation. Co-Authored-By: Claude <noreply@anthropic.com>
c286f94 to
6dbfa47
Compare
|
Rebased onto master at `6dbfa475e` now that #1664 has landed ( The rebase was content-preserving for this PR: Suggestion 1 ( Since it is a defensive default with no live caller, I would rather not re-spin a clean review and a full queued suite for it, and risk another rebase race while master is moving. It goes into #1674, which is already editing both harnesses — matching your own framing of it as "a one-line default if you touch the file again." |
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: 6dbfa47
Looks good. This wake is a force-push rebase, not new work: the two commits carry the same messages and the same content as the pair I reviewed at e715fb8a and c286f946, under new SHAs (091088df, 6dbfa475) on a newer base.
I did not take that on faith from the commit messages — a rebase is exactly where content can shift silently, and this one had a specific hazard worth ruling out. These tests slice their regions out of approve-paperclip-api-digest.sh by textual anchors, and the base moved because #1664 landed. Had #1664 touched those regions, the anchors could have drifted or the sliced behaviour changed underneath assertions that would still look green in review.
It did not. Both review-relevant files are byte-identical across the rebase:
git diff c286f946 6dbfa475 -- scripts/approve-paperclip-api-digest.test.js # empty
git diff c286f946 6dbfa475 -- scripts/approve-paperclip-api-digest.sh # empty
The rebase pulled 31 unrelated files forward from master (+4261/-142), none under scripts/approve-paperclip-api-digest.*.
The diff-noise both prior reviews flagged is now gone, as they predicted. .sh is untouched by this branch (git diff $(git merge-base origin/master HEAD) HEAD -- …sh is empty) and identical to master. The PR is finally what it always was in substance: one file, scripts/approve-paperclip-api-digest.test.js, +95/-12, split 091088df +98/-12 and 6dbfa475 +5/-8, test-only in both.
Because the base moved, I re-ran the whole verification at this exact head rather than carrying the previous result across.
Baseline reproduced: 64 pass / 0 fail, ~3.5s (node v24.16.0, bash 5.2.37, jq 1.7). policy is success at this head.
All four pacing mutations still reproduce, each on the correct test (:374 is the linear sleep "$attempt", :761 the flat sleep 1 — unchanged line numbers):
| mutation | result | failing test |
|---|---|---|
:374 → sleep 1 |
63/1 | retire-only exhaustion |
:374 → sleep 0 |
63/1 | retire-only exhaustion |
:761 → sleep "$attempt" |
63/1 | release exhaustion |
:761 → sleep 9 |
63/1 | release exhaustion |
The controls are still non-vacuous — probed against the failure each one claims to catch, not read off its comment:
sleep 5above the release successreturn 0(:715) → the release success case fails.sleep 3above retire-only's successexit 0(:345) → the retire-only success case fails.sleep 3above the release non-retriable bail (:725) → two tests fail, includingassert.deepEqual(r.sleeps, [])on the bail — the assertion whose comment says counting writes alone would miss a bail that moved below the sleep.
The stdin drain remains load-bearing: deleting cat >/dev/null from both stubs fails both success cases 3/3 runs, deterministically — not a size-dependent EPIPE race. Script and test file restored and git status clean after every probe.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
Both carried forward unchanged from my c286f946 review — still open, still non-blocking, and both still accurate at this head (re-derived line numbers, not copied).
- [types]
scripts/approve-paperclip-api-digest.test.js:1269,:1450—stderrTextstill has no default in either destructuring, while{ writeSucceeds: true }is now an established call shape, so omitting the argument looks idiomatic here. On the failing branch, omitting it is silent rather than loud. I confirmed the end state empirically rather than inferring it —runReleaseWrite({})passes, exits 1, records 1 write, and emitscannot retire the in-flight lock on sha256:deadbeef (owner owner-nonce-1):\n undefined, becauseJSON.stringify(undefined)interpolates as the bare wordundefined. No current caller does this: all six failing-branch call sites — three per harness (:1356,:1380,:1422and:1529,:1550,:1594) — pass it explicitly, and the twowriteSucceeds: truesites (:1408,:1583) never reach the interpolation. So nothing is wrong today — this is about the next failing-branch caller asserting against the word "undefined" instead of failing loudly.:1053already writes the guarded formstderrText ? … : ""for exactly this, so the two write harnesses are the odd ones out rather than the convention. - [tests]
scripts/approve-paperclip-api-digest.test.js:1578— the retire-only success case assertsexit 0but not the operator-facing stdout it exists to produce ("Retired the in-flight approval lock on … The ring still lists that digest …",:342-344of the script).:757-759states that the release-silent / retire-only-chatty asymmetry is deliberate and not a parity gap — precisely the comment-only claim this PR's thesis says should become a test. Neither harness captures stdout today (:1351,:1525both return{ status, stderr, writes, sleeps }), so it needs a small harness change. Reasonable as a follow-up, not in this PR.
Strengths
- The rebase is clean in the way that matters: content-identical on both files under review, no opportunistic edits smuggled in alongside the base bump, and no scope creep into the shipping script. The
+95/-12is entirely the two intended changes. - The assertions survive a base change that could have broken them. Anchor-sliced tests are fragile against exactly this kind of upstream movement, and all four mutations plus all three control probes still land on the correct test at the new base — so the pacing guarantees are genuinely pinned, not incidentally green.
- Asserting
["1","1"]/["1","2"]rather than an attempt count or a total remains the right call: those are literally the 2s and 3s that:751-753cites as its own justification, so the tests pin the comment's stated reasoning instead of a proxy a "helpful parity fix" could satisfy while inverting the intent. - The success cases are real controls, not coverage padding —
assert.equal(r.writes, 1)is what stops them passing vacuously via the ownership-guard early exit, which would record zero writes.
Recommended Action
- No Critical or Important issues — mergeable as-is on the review dimension.
mergeStateStatusisBLOCKEDonly because most checks are stillqueuedat this head;policy,review,Helm chartand the vendored-adapter check are alreadysuccess. Per standing policy, do not merge until the required checks reportsuccess— a queued gate is a stop, not a pass. - Suggestion 1 is a one-line default if you touch the file again; suggestion 2 remains a reasonable follow-up.
…32122) Ally's remaining non-blocking suggestion from #1671, deferred there on that review's own recommendation because it needs a harness change rather than a new assertion. `:755-759` spends five lines arguing that the messaging difference between the two retirement loops -- `release_in_flight_lock` silent on success, retire-only mode printing three operator-facing lines -- is deliberate and NOT a parity gap to close. Neither harness captured stdout, so that claim was enforced by nothing: deleting all three `echo`s at `:342-344` left the suite green, and so did adding a matching success line to the silent loop. Both harnesses now return `stdout`, and both halves are pinned, not just the chatty one. The release success case asserts stdout is exactly empty, which is the more surprising half and the direction a parity fix would take -- it would ADD output there, which no assertion on retire-only mode can catch. The retire-only success case asserts the digest and owner, then the ring sentence across its line wrap, so a rewrap stays green and a deletion does not. The shell script is unmodified; the asymmetry it documents is correct. Mutation-verified at this head, each confirmed as a real edit (non-empty diff, `bash -n` clean) and the script restored clean after each: delete all three `:342-344` echoes -> retire-only success test fails delete only `:342` (digest+owner) -> retire-only success test fails delete only `:343-344` (ring) -> retire-only success test fails add a success echo at `:714` -> release success test fails The four BLO-32109 pacing mutations still fail their own exhaustion tests (retire-only `:374` -> `sleep 1` and -> `sleep 0`; release `:761` -> `sleep "$attempt"` and -> `sleep 9`), so this did not weaken them. Unmutated suite 64/64. Note the issue text says 65; that count was already stale when written -- HEAD before this change measures 64. Co-Authored-By: Claude <noreply@anthropic.com>
Thinking Path
Linked Issues or Issue Description
Stacked on #1664, which is currently in the merge queue at position 2. Until it lands, the diff here shows its commit
24e5b345as well as this one; afterwards onlye715fb8aremains. Review the second commit.scripts/approve-paperclip-api-digest.shis not modified by this PR.What Changed
runReleaseWrite,runRetireOnlyWrite) now record thesleepargument to a log file instead of discarding it, and return it assleeps.["1","1"]forrelease_in_flight_lock(flat),["1","2"]for retire-only mode (linear). These are the 2s and 3s thatscripts/approve-paperclip-api-digest.sh:751-753cites as its own justification, so the assertion pins the comment's stated reasoning rather than a proxy for it.jq | kubectl replacepipeline underpipefail, and the stub never drained stdin. Harmless whilereplacealways failed the pipeline itself, but on a succeeding writejqdied of EPIPE and the pipeline reported status 1 for a successful retirement. The stub now drains the manifest as the realkubectl replace -f -does.Verification
Mutation-verified rather than presence-asserted, per the standard set by #1646 and #1664. Each mutation was confirmed to be a real edit (non-empty
git diff,bash -nclean) and the script restored afterwards::374→sleep 1:374→sleep 0:761→sleep "$attempt":761→sleep 9The new success cases were checked non-vacuous the same way: inserting
sleep 7before the release successreturn 0fails "a retirement write that succeeds returns at the first attempt without sleeping".pnpm run check:tokensclean.node --test scripts/__tests__/policy-node-test-timeouts.test.mjs5/5.pnpm run check:test-undefined-symbolsreports 20 pre-existing findings, all inserver/src/and all present on the base commit without this change (verified by stash) — they stem from missingnode_modulesin this environment, not from this PR.Risks
Low risk — test-only.
scripts/approve-paperclip-api-digest.shis unmodified; the pacing it ships is correct and deliberate, and this PR only makes it tested.The one behavioural change is inside the test double: the
kubectl replacestub now drains stdin. That is strictly closer to what the real command does, and all three pre-existing cases on each path pass unchanged either way.Note on gating, carried from BLO-31666 and BLO-32001:
policyis not merge-gating onmaster(verifyis the only required context and itsneedsomitspolicy), so these tests report without blocking.Model Used
claude-opus-5[1m], 1M context), extended thinking, via Claude Code with tool use.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template