test(deploy): make the temp-file cleanup cases actually able to fail (BLO-32395) - #1696
Conversation
…(BLO-32395)
Both `leaves no temp file behind` cases passed with the cleanup they claim to
pin DELETED, so neither was evidence. Measured on master before this change:
removing `rm -f "$jq_err"` from the shipping reader left the suite at 96/96,
and removing the pre-existing `rm -f "$rs_err"` sibling did too.
The cause is that both stderr captures are minted INSIDE $state_dir, and
$state_dir is removed on both paths -- by the reader itself when it minted an
ephemeral one, by the harness teardown when the caller owns it. So by the time
readdirSync(tmpRoot) runs, the file is gone whether or not the reader's `rm -f`
reclaimed it first. The assertion could only ever see the directory, never its
contents.
Fixed by listing the caller-owned directory's own contents BEFORE teardown --
the one moment they are observable -- and asserting the exact residue. Both
cases now fail when either cleanup is deleted:
rm -f "$jq_err" deleted: ['jq-err', 'warned-jq'] vs ['warned-jq']
['jq-err'] vs []
rm -f "$rs_err" deleted: ['list-err', 'warned-jq'] vs ['warned-jq']
['list-err', 'warned-list'] vs ['warned-list']
The residue is asserted as an exact set rather than as "does not include
jq-err", because a not-includes check would still pass if the reader started
leaving some other file behind. The leftoverTempFiles assertions are KEPT
alongside it: on the no-caller path they are the only thing that catches a
reader which mints an ephemeral state_dir and never removes it. Two different
claims, so both are asserted rather than one standing in for the other.
A caller-owned directory the reader has wrongly removed reads as an explicit
sentinel rather than crashing the harness under `set -e`; verified by making
the reader's teardown unconditional, which reports the sentinel by name.
No change to the shipping script: both `rm -f` lines remain and both temp
files are still reclaimed. No change to reader behaviour -- election order, the
decline/abort split from #1691, warn-once semantics, and degrade-never-fail are
untouched. Suite stays at 96/96 with the reader unmodified.
Follow-up to BLO-32267, raised by Ally as a non-blocking Suggestion on #1691.
Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
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 |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally please review at head The Please ignore the stale comment above. It cannot self-clear: Review focus: the diff is test-only ( |
![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: 65e4526
Looks good. Test-only diff (+69/−1), and I verified the central claim rather than taking it on the comments' word: the two cases really were unable to fail before, and really can now.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [native-codex]
scripts/approve-paperclip-api-digest.test.js:547—( cd "$rs_state_dir" && ls -A )can bels -A "$rs_state_dir":lsalready prints bare names for a single directory operand, so the subshell and thecdbuy nothing. Not a bug today — thecdwould only echo to stdout if the operand resolved viaCDPATH, and POSIX forbids consultingCDPATHfor a path beginning with/, which$rs_state_diralways does (mktemp -d "${TMPDIR}/…"withTMPDIRabsolute). Purely simplification, plus it removes the need for the reader to know that argument is absolute. - [gstack/review]
scripts/approve-paperclip-api-digest.test.js:1356— Worth one line on the stakes of the invariant, so a future failure is not over-escalated.jq_err/rs_errare fixed names (${state_dir}/jq-err), re-truncated by: >on each entry, so a missingrm -fcannot accumulate across rotations — and the caller'sEXITtrap (rm -rf "$rs_state_dir", reader.sh:1227) reclaims the directory at process exit regardless. So what the residue assertion protects is the reader's own prompt-cleanup contract, not a leak that would grow. That is a fine thing to regression-lock and this is the only view that can fail when it breaks; it just means a failure here is hygiene, not a resource bug. - [pr-review-toolkit/tests]
scripts/approve-paperclip-api-digest.test.js:1388— The residue assertion is guarded byif (callerOwnedStateDir)inside a two-value loop, so half of each loop asserts nothing about residue. That is correct and the comments explain why, but consider hoisting the caller-owned case out of the loop (or assertingrun.stateDirResidue === nullin thefalsearm) so the split is structural rather than a reader having to infer it from the guard.
Strengths
- The claim is verifiable and I verified it.
unguardedCoercionReaderSourceis a narrow textual replace of the shippingreplicaSetReaderSource(test.js:450, anchored on the guard's own text), not a restated copy — so the shippingrm -f "$jq_err"/rm -f "$rs_err"do propagate into the mutated variant. Deleting either from the real reader is therefore genuinely caught by the new assertion. The pre-existingleftoverTempFilesview could not catch it, exactly as the comment says: both files live insidestate_dir, which is gone byreaddirSync(tmpRoot)on both paths. - The residue expectations are right on all three branches, checked against reader.sh:1001–1107. Forbidden list →
warned-listonly (rm -f "$rs_err"at :1103 sits outside thers_statusbranch, so it runs on the failure path too). Empty list →[], because an empty$servingis a decline the jq program expresses by succeeding. jq abort →warned-jqonly, with both captures reclaimed at :1101 and :1103. - The new assertion also self-protects against anchor drift. If the
UNGUARDED_COERCION_ANCHORstops matching, the mutation no-ops, jq succeeds, no marker is written, and residue is[]— sodeepEqual(residue, ["warned-jq"])fails loudly. TheleftoverTempFilesassertion in the same test would have passed. That is a second defect class caught for free. stateDirResidue: nullrather than[]in the no-caller mode is the right call — asserting residue in the wrong mode fails instead of matching an empty array for the wrong reason.- The
<caller-owned-state-dir-removed-by-reader>sentinel turns "reader wrongly removed the caller's directory" into a named assertion failure instead of a bare non-zero exit underset -e.rm -rfon the already-gone path still exits 0, so the harness reaches the assertion. - Keeping
leftoverTempFilesis two claims, not redundancy — this was the author's own open question and it holds up. On the no-caller path it is the only thing that catches a reader minting an ephemeralstate_dirand never removing it; even on the caller-owned path it still covers a file minted directly inTMPDIRrather than insidestate_dir. - No unguarded
readFileSync(residueLog)hazard: theassert.equal(result.status, 0, …)above the return means a harness that aborted before writing the log fails with the reader-must-succeed message first, so ENOENT is unreachable. Additive field, so the other twocallerOwnedStateDir: truecases (:1331, :1444) are unaffected. - The
warned-selectorbranch needs no residue pin: it creates no capture files at all, so there is no gap there.
Recommended Action
- No Critical or Important issues — mergeable as-is.
- Suggestions are optional; the second one (a line on the invariant's stakes) is the only one I'd bother with, and it is a comment-only change.
- All 20 check-runs are
successat this head, including theGeneral testsshards that run this file,verify, andreview.
Submitted as a formal COMMENTED review: this PR is authored by app/allyblockcast, and GitHub bars a PR's author from APPROVE. Zero Critical and zero Important findings — the comment state reflects the authorship constraint, not a reservation about the change.
Thinking Path
Refs: BLO-32395. Follow-up to BLO-32267, raised by Ally as a non-blocking Suggestion on #1691.
The defect
Both
leaves no temp file behindcases inscripts/approve-paperclip-api-digest.test.jspassed with the cleanup they claim to pin deleted, so neither was evidence.Both stderr captures are minted inside
$state_dir, and$state_diris removed on both paths — by the reader itself when it minted an ephemeral one, by the harness teardown when the caller owns it. So by the timereaddirSync(tmpRoot)runs, the file is gone whether or not the reader'srm -freclaimed it first. The assertion could only ever see the directory, never its contents.What Changed
scripts/approve-paperclip-api-digest.test.jsonly (+69/−1). No shipping code in this diff.leaves no temp file behindcases now list the caller-ownedstate_dir's own contents before harness teardown — the one moment they are observable — and assert the exact residue.jq-err": a not-includes check would still pass if the reader started leaving some other file behind.leftoverTempFilesassertions are kept alongside the new ones. On the no-caller path they are the only thing that catches a reader which mints an ephemeralstate_dirand never removes it. Two different claims, so both are asserted rather than one standing in for the other.<caller-owned-state-dir-removed-by-reader>) is reported if the reader ever removes the caller's directory, so that failure mode surfaces by name instead of crashing the harness underset -e.Verification
node --test ./scripts/approve-paperclip-api-digest.test.js, gated in CI at.github/workflows/pr.yml(jobpolicy). 96/96 with the reader unmodified — the same count as baseline, since this repairs two existing cases rather than adding or removing any.Negative controls were run, not assumed. Baseline, shipping reader unmodified: 96/96 pass.
rm -f "$jq_err"deleted → 2 fail:rm -f "$rs_err"deleted → 2 fail:Before this change both of those deletions left the suite at 96/96 — reproducing Ally's measurement, including on the pre-existing
rs_errsibling. The two are fixed together; neither is left inert.The sentinel branch was verified rather than asserted: making the reader's
rm -rf "$state_dir"unconditional (so it removes the caller's directory) reports<caller-owned-state-dir-removed-by-reader>by name instead of crashing the harness.Risks
Low risk, and confined to test code. This diff touches no shipping file:
rm -f "$jq_err"andrm -f "$rs_err"both remain at their original lines inscripts/approve-paperclip-api-digest.sh, and both temp files are still reclaimed. Reader behaviour is untouched — election order, the decline/abort split shipped in #1691, warn-once semantics, and degrade-never-fail all unchanged.The one behavioural change a reviewer should weigh is that the two repaired cases are now sensitive to the reader's residue inside a caller-owned
state_dir, so a future change that legitimately starts leaving a new file there will fail them. That is the intended property — it is what makes them evidence — but it is a real maintenance obligation rather than a free win.Model Used
claude-opus-5[1m] (Anthropic), via Claude Code.
🤖 Generated with Claude Code