From e07fe7a6e2e975ccc6f8d27581118217acf6a178 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 6 Sep 2026 04:35:57 +0000 Subject: [PATCH 1/2] test(deploy): stop both write harnesses fabricating `undefined` as kubectl stderr `runReleaseWrite` and `runRetireOnlyWrite` destructured `stderrText` with no default. `JSON.stringify(undefined)` returns the value undefined, not a string, so the failing branch interpolated the bare six-character token into the stub and shipped it as kubectl's stderr: the release path's message read cannot retire the in-flight lock on sha256:deadbeef (owner owner-nonce-1): undefined a cause no kubectl ever produced. Unreachable today, and checked rather than assumed -- every failing-path caller passes the text explicitly, and the callers that omit it pass writeSucceeds, which takes the `return 0` arm and never reads the value. So no current test is wrong. The cost was in front of us: this harness has taken new cases from four issues in two days, and the next failing case that forgot the argument would have asserted against that fabricated value, passing or failing on the harness rather than on the script. Defaulted to "" at both destructures, routing an omission onto the empty-capture path both harnesses already defend, and pinned it as EQUIVALENCE to an explicit "" rather than as the absence of /undefined/ -- the negative alone still passes if the default becomes some other invented string (measured: "(none)" leaves it green and trips only the deepEqual). Behaviour-preserving: a JS default fires only on undefined, so every explicit call site keeps its stub text byte-for-byte, verified against all three values the file passes. Test-only; scripts/approve-paperclip-api-digest.sh is byte-identical to master. Suite 79 -> 80, 0 fail. Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.test.js | 52 +++++++++++++++++++- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index 06ad4cc38e39..10fa789eef62 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -1639,7 +1639,7 @@ test("a signal is routed into the cleanup rather than killing the script outrigh // cause, on the path with the LEAST operator visibility -- the caller prints // only the bare "could not retire the in-flight lock" and nobody is at a // terminal to re-run it with more logging. -function runReleaseWrite({ stderrText, attempts = 3, writeSucceeds = false }) { +function runReleaseWrite({ stderrText = "", attempts = 3, writeSucceeds = false }) { const dir = mkdtempSync(path.join(tmpdir(), "paperclip-release-write-")); const countFile = path.join(dir, "replace_count"); const sleepLog = path.join(dir, "sleeps"); @@ -1851,7 +1851,7 @@ test("a retirement write with no stderr still explains itself", () => { // rather than by presetting CLEAR_IN_FLIGHT_LOCK_ERR, so the variable is proven // populated by the script's own `2>&1 >/dev/null` capture -- the same standard // the release harness sets, for the same reason. -function runRetireOnlyWrite({ stderrText, attempts = 3, writeSucceeds = false }) { +function runRetireOnlyWrite({ stderrText = "", attempts = 3, writeSucceeds = false }) { const dir = mkdtempSync(path.join(tmpdir(), "paperclip-retire-write-")); const countFile = path.join(dir, "replace_count"); const sleepLog = path.join(dir, "sleeps"); @@ -2155,3 +2155,51 @@ test("no line citation can be added to this file without being pinned", () => { "a LINE_CITATIONS entry no longer matches any citation in the file -- remove it rather than leaving it pinning nothing", ); }); + +// Both write harnesses default `stderrText` to "" rather than destructuring it +// bare, and this pins that default. Ally's suggestion 1 on #1671: +// `JSON.stringify(undefined)` returns the VALUE undefined, not a string, so a +// bare destructure interpolated the six-character token `undefined` into the +// stub and shipped it as kubectl's stderr -- the release path's message read +// `cannot retire the in-flight lock on sha256:deadbeef (owner owner-nonce-1):` +// followed by an indented `undefined`, a cause no kubectl ever produced. +// +// No call site reached it: every failing-path caller passes the text +// explicitly, and the callers that omit it pass writeSucceeds, which takes the +// `return 0` arm and never reads the value. So the whole cost was in front of +// us rather than behind -- this harness has taken new cases from four issues in +// two days, and the next failing case that forgets the argument would have +// asserted against that fabricated value, passing or failing on the harness +// rather than on the script. That is the quiet-wrong-answer mode, which is why +// it is worth a test despite being unreachable on the day it was written. +// +// Asserted as EQUIVALENCE to an explicit "", not merely as the absence of +// /undefined/. The negative alone would still pass if the default were later +// changed to some other invented string -- measured, not supposed: defaulting +// to "(none)" instead leaves the negative green and trips only this deepEqual. +// The equivalence pins it to the one value that already carries a tested +// meaning on this path -- kubectl produced no output -- so a forgotten argument +// lands on the empty-capture branch both harnesses defend directly above, +// instead of on a new and unexamined one. +// +// Both harnesses in one test because it is one shared invariant, and because +// the pair is the point: the defect was identical in both, so a per-harness +// split would let a future divergence read as an unrelated single failure. +test("omitting stderrText on a failing write is exactly an empty capture, never the token `undefined`", () => { + for (const [mode, run] of [ + ["release", runReleaseWrite], + ["retire-only", runRetireOnlyWrite], + ]) { + const omitted = run({}); + assert.doesNotMatch( + omitted.stderr, + /undefined/, + `${mode} mode fabricated a cause: the stub emitted the bare token \`undefined\` as kubectl's stderr, so any assertion about the cause would be measuring the harness rather than the script`, + ); + assert.deepEqual( + omitted, + run({ stderrText: "" }), + `${mode} mode: omitting stderrText must be indistinguishable from passing "", so a failing case that forgets it lands on the tested empty-capture path`, + ); + } +}); From 3ffe44e32119e8dcbef238c9815b062c284c1aea Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 6 Sep 2026 05:09:47 +0000 Subject: [PATCH 2/2] test(deploy): default the parameter object too, and pin it in the same chain Ally's suggestion 1 on #1682. The `stderrText = ""` defaults landed one level in: they defend an omitted *property*, but the parameter object itself still had no default, so a bare `runReleaseWrite()` died with `TypeError: Cannot read properties of undefined (reading 'stderrText')` before ever reaching them. That is a loud failure rather than the quiet one the PR closes, which is why Ally called it polish -- but it is the same omission family, so it is pinned as an extension of the existing equivalence rather than given a test of its own: `run()` === `run({})` === `run({ stderrText: "" })`. Removing either `= {}` throws past the chain and fails the new assertion. Suite 80 pass / 0 fail. `scripts/approve-paperclip-api-digest.sh` remains byte-identical to master -- test-only. Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.test.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index 10fa789eef62..a3465111d721 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -1639,7 +1639,7 @@ test("a signal is routed into the cleanup rather than killing the script outrigh // cause, on the path with the LEAST operator visibility -- the caller prints // only the bare "could not retire the in-flight lock" and nobody is at a // terminal to re-run it with more logging. -function runReleaseWrite({ stderrText = "", attempts = 3, writeSucceeds = false }) { +function runReleaseWrite({ stderrText = "", attempts = 3, writeSucceeds = false } = {}) { const dir = mkdtempSync(path.join(tmpdir(), "paperclip-release-write-")); const countFile = path.join(dir, "replace_count"); const sleepLog = path.join(dir, "sleeps"); @@ -1851,7 +1851,7 @@ test("a retirement write with no stderr still explains itself", () => { // rather than by presetting CLEAR_IN_FLIGHT_LOCK_ERR, so the variable is proven // populated by the script's own `2>&1 >/dev/null` capture -- the same standard // the release harness sets, for the same reason. -function runRetireOnlyWrite({ stderrText = "", attempts = 3, writeSucceeds = false }) { +function runRetireOnlyWrite({ stderrText = "", attempts = 3, writeSucceeds = false } = {}) { const dir = mkdtempSync(path.join(tmpdir(), "paperclip-retire-write-")); const countFile = path.join(dir, "replace_count"); const sleepLog = path.join(dir, "sleeps"); @@ -2185,6 +2185,15 @@ test("no line citation can be added to this file without being pinned", () => { // Both harnesses in one test because it is one shared invariant, and because // the pair is the point: the defect was identical in both, so a per-harness // split would let a future divergence read as an unrelated single failure. +// +// The third assertion pins the `= {}` on the parameter object itself, which is +// the same omission family one level out: with a bare `{ ... }` destructure the +// defaults above are unreachable for a caller who passes nothing at all, and +// `run()` dies with `TypeError: Cannot read properties of undefined (reading +// 'stderrText')`. That is a loud failure rather than the quiet one this test +// exists for, so it is pinned here as an extension of the same chain -- +// `run()` === `run({})` === `run({ stderrText: "" })` -- rather than given a +// test of its own. Ally's suggestion 1 on #1682. test("omitting stderrText on a failing write is exactly an empty capture, never the token `undefined`", () => { for (const [mode, run] of [ ["release", runReleaseWrite], @@ -2201,5 +2210,10 @@ test("omitting stderrText on a failing write is exactly an empty capture, never run({ stderrText: "" }), `${mode} mode: omitting stderrText must be indistinguishable from passing "", so a failing case that forgets it lands on the tested empty-capture path`, ); + assert.deepEqual( + run(), + omitted, + `${mode} mode: omitting the argument object entirely must reach the same defaults as passing {}, not throw past them`, + ); } });