test(deploy): assert the two retirement loops' retry pacing (BLO-32109) - #1671
test(deploy): assert the two retirement loops' retry pacing (BLO-32109)#1671allyblockcast[bot] wants to merge 2 commits into
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
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