test(deploy): pin the two retirement loops' messaging asymmetry (BLO-32122)

scripts/approve-paperclip-api-digest.test.js

@@ -1656,7 +1656,7 @@ function runReleaseWrite({ stderrText, attempts = 3, writeSucceeds = false }) {
   );
   // Heredoc body and terminator sit at column 0 deliberately; indenting them
   // here would make the generated bash unparseable rather than failing loudly.
-  // Flags match the shipping script's `set -euo pipefail` (`:82`) so what runs
+  // Flags match the shipping script's `set -euo pipefail` (`:86`) so what runs
   // here runs under production's error handling; all three cases behave
   // identically either way, so the fidelity is free. It is NOT extra mutation
   // coverage for the trailing-sleep guard below: bash exempts the left side of
@@ -1721,7 +1721,13 @@ release_in_flight_lock`;
   const sleeps = existsSync(sleepLog)
     ? readFileSync(sleepLog, "utf8").split("\n").filter(Boolean)
     : [];
-  return { status: r.status, stderr: r.stderr, writes, sleeps };
+  // stdout as well as stderr: the two loops' MESSAGING asymmetry -- this one
+  // silent on both success and exhaustion, retire-only mode chatty -- is the
+  // other half of what `:759-763` defends at length against a parity fix, and
+  // until both harnesses captured stdout it was asserted by nothing. Every
+  // direction of that fix is now a failure: adding an `echo` here on either
+  // path, or deleting retire-only's.
+  return { status: r.status, stdout: r.stdout, stderr: r.stderr, writes, sleeps };
 }
 
 test("a non-retriable retirement write bails once and reports the cause", () => {
@@ -1755,7 +1761,7 @@ test("a conflicting retirement write is still retried to exhaustion", () => {
   });
   assert.equal(r.writes, 3, "a conflict must consume every attempt, not bail on the first");
   assert.equal(r.status, 1, "exhausting the attempts still reports failure");
-  // FLAT, and asserted by argument rather than by count: `:747-756` spends ten
+  // FLAT, and asserted by argument rather than by count: `:751-760` spends ten
   // lines defending this flatness against the "helpful parity fix" that would
   // spell it `sleep "$attempt"` like retire-only mode's, and until the stub
   // recorded its argument that defence was enforced by nothing. ["1","1"] is
@@ -1771,9 +1777,20 @@ test("a conflicting retirement write is still retried to exhaustion", () => {
     /cannot retire the in-flight lock on/,
     "the non-retriable message must not fire on a retriable conflict",
   );
+  // The EXHAUSTION half of the silence `:761-762` requires, which the success
+  // test's assertion cannot reach: that comment forbids a chatty success *or
+  // exhaustion*, and exhaustion is where the temptation is stronger, because
+  // the loop has just given up and so has something to report. It reports it on
+  // stderr or not at all -- the operator guidance belongs to `cleanup_on_exit`,
+  // and a stdout line here would bury the deploy failure that ran the cleanup.
+  assert.equal(
+    r.stdout,
+    "",
+    "an exhausted retirement must stay silent on stdout too -- its caller prints the guidance, and a line here would bury the failure the cleanup is running after",
+  );
 });
 
-test("a retirement write that succeeds returns at the first attempt without sleeping", () => {
+test("a retirement write that succeeds returns at the first attempt, silently and without sleeping", () => {
   // The control for the two pacing assertions: they pin what an EXHAUSTED loop
   // spends, and would still pass if the sleep had migrated above the success
   // check and started charging every caller. A succeeding retirement is the
@@ -1782,6 +1799,20 @@ test("a retirement write that succeeds returns at the first attempt without slee
   assert.equal(r.writes, 1, "a succeeding write must not be repeated");
   assert.equal(r.status, 0, "a retired lock must report success");
   assert.deepEqual(r.sleeps, [], "the success path must not sleep");
+  // The SILENT half of the messaging asymmetry, and the more surprising one --
+  // a parity fix reading retire-only mode's three chatty success lines would
+  // "correct" this loop by ADDING output, which is the direction the retire-only
+  // assertion below cannot catch. `:759-763` argues this silence is required,
+  // not merely tolerated: the caller (`cleanup_on_exit`) prints the operator
+  // guidance itself, and this loop only ever runs after a deploy has already
+  // failed, so a success line here would bury the failure the cleanup exists to
+  // report. Asserted as exact emptiness rather than a `doesNotMatch` on today's
+  // wording, so any new success line fails regardless of how it is phrased.
+  assert.equal(
+    r.stdout,
+    "",
+    "this loop must stay silent on success -- its caller prints the guidance, and a success line here would bury the failure the cleanup is running after",
+  );
 });
 
 test("a retirement write with no stderr still explains itself", () => {
@@ -1895,7 +1926,10 @@ ${retireRegion}`;
   const sleeps = existsSync(sleepLog)
     ? readFileSync(sleepLog, "utf8").split("\n").filter(Boolean)
     : [];
-  return { status: r.status, stderr: r.stderr, writes, sleeps };
+  // stdout captured for the reason spelled out in `runReleaseWrite`: this is the
+  // chatty half of that messaging asymmetry, and the success lines asserted
+  // below are what the release harness's empty-stdout assertion is the mirror of.
+  return { status: r.status, stdout: r.stdout, stderr: r.stderr, writes, sleeps };
 }
 
 test("retire-only mode's non-retriable write bails once and reports the cause", () => {
@@ -1929,7 +1963,7 @@ test("retire-only mode's conflicting write is still retried to exhaustion", () =
   // arguments rather than the attempt count is what makes the two loops'
   // pacing a tested difference instead of a commented one: with a discarding
   // stub, spelling this `sleep 1` -- or `sleep 0` -- left the suite green.
-  // 3s total, the number `:751-753` names as the cost retire-only mode can
+  // 3s total, the number `:755-757` names as the cost retire-only mode can
   // afford because it has an operator at a terminal and no grace-period clock.
   assert.deepEqual(
     r.sleeps,
@@ -1948,7 +1982,7 @@ test("retire-only mode's conflicting write is still retried to exhaustion", () =
   );
 });
 
-test("retire-only mode's succeeding write exits 0 at the first attempt without sleeping", () => {
+test("retire-only mode's succeeding write exits 0 at the first attempt, telling the operator what it retired", () => {
   // Same control as the release path's. Retire-only mode `exit 0`s where the
   // release loop `return 0`s, so this also pins that the success exit survives
   // the slice -- a mode that retired the lock and then exited non-zero would
@@ -1957,6 +1991,24 @@ test("retire-only mode's succeeding write exits 0 at the first attempt without s
   assert.equal(r.writes, 1, "a succeeding write must not be repeated");
   assert.equal(r.status, 0, "a retired lock must exit 0");
   assert.deepEqual(r.sleeps, [], "the success path must not spend the backoff");
+  // The CHATTY half, and the output this mode exists to produce: an operator
+  // ran it by hand and the three lines at `:346-348` are the only report they
+  // get. Until the harness captured stdout, silencing all three "for parity
+  // with `release_in_flight_lock`" left the suite green.
+  assert.match(
+    r.stdout,
+    /Retired the in-flight approval lock on sha256:cafe \(owner owner-9\)/,
+    "the operator must be told WHICH lock was retired, by digest and owner",
+  );
+  // The consequence, not just the act: this is what tells the operator they can
+  // re-run a corrected plan without editing the ring by hand. Matched across the
+  // line wrap so a rewrap stays green and a deletion does not -- the two lines
+  // are one sentence and neither is separately meaningful.
+  assert.match(
+    r.stdout,
+    /The ring still lists that digest, so a corrected plan or a rollback is\s+admitted without an out-of-band edit\./,
+    "the operator must be told the retirement left the digest admissible, or they will assume they have to re-approve it",
+  );
 });
 
 test("retire-only mode's write with no stderr still explains itself", () => {
@@ -1978,3 +2030,128 @@ test("retire-only mode's write with no stderr still explains itself", () => {
     "the message must not end at the colon it promises to expand on",
   );
 });
+
+// The `:NNN` citations sprinkled through the comments above name lines in the
+// shipping script, and until now nothing checked them. That is not hypothetical
+// drift: when the BLO-31842 ReplicaSet work inserted four lines into the
+// script's header comment, all seven citations in this file went stale by
+// exactly +4 in one commit, silently. The worst of them then pointed a reader
+// at the "nothing to retire" branch's `exit 0` instead of the three success
+// lines the assertion beside it is actually about -- so the comment read as if
+// it were pinning a completely different code path.
+//
+// Every OTHER cross-file reference here is already mechanical -- the function
+// body by name via extractShellFunction(), the numeric defaults and limits by
+// pattern via shellDefault()/shellLimit() -- which left the line citations as
+// the one convention in this suite held together by nothing but care. This
+// stack's whole thesis is that its comments are checkable, so they are checked.
+//
+// Pinned by the range's FIRST LINE, matched against that one line rather than
+// searched for across the range. Searching a range is what a first attempt at
+// this did, and it is too weak twice over: a citation ten lines wide still
+// "contains" its anchor after a four-line shift, so it tolerates exactly the
+// drift this exists to catch; and a spanning `[\s\S]*` pattern happily matches
+// an earlier occurrence elsewhere in the script and then reports a confidently
+// wrong line number -- the very defect being fixed. An exact first-line match
+// admits neither. `contains` additionally holds the rest of the range to the
+// content the citing comment claims is there.
+const LINE_CITATIONS = [
+  {
+    cite: "86",
+    startsWith: /^set -euo pipefail$/,
+    claim: "the shipping script's error-handling flags, mirrored by the harness",
+  },
+  {
+    cite: "346-348",
+    startsWith: /^\s*echo "Retired the in-flight approval lock on /,
+    contains: /ring still lists that digest[\s\S]*without an out-of-band edit/,
+    claim: "retire-only mode's three operator-facing success lines",
+  },
+  {
+    cite: "751-760",
+    startsWith: /^\s*# Flat, where retire-only mode backs off linearly/,
+    claim: "the ten lines defending the release loop's flat backoff",
+  },
+  {
+    cite: "755-757",
+    startsWith: /^\s*# `trap 'exit 143' TERM`, so the runner's grace period/,
+    contains: /2s of total sleep beats 3s/,
+    claim: "the 3s figure retire-only mode can afford and the release loop cannot",
+  },
+  {
+    cite: "759-763",
+    startsWith: /^\s*# Their MESSAGING differences are deliberate too/,
+    claim: "the messaging-asymmetry defence against a parity fix",
+  },
+  {
+    cite: "761-762",
+    startsWith: /^\s*# guidance that retire-only mode prints itself, so a chatty success or/,
+    contains: /exhaustion here would bury/,
+    claim: "the clause forbidding a chatty success OR exhaustion",
+  },
+];
+
+const scriptLines = script.split("\n");
+
+function citedRange(cite) {
+  const [start, end = start] = cite.split("-").map(Number);
+  return scriptLines.slice(start - 1, end).join("\n");
+}
+
+test("every line citation in this file still points at the content it claims", () => {
+  // Reports EVERY drifted citation in one run, rather than failing on the
+  // first. Drift arrives all at once -- BLO-31842 shifted all seven of these by
+  // +4 in a single commit, changing nothing about them -- so failing one at a
+  // time would turn one mechanical repair into seven fix-and-rerun cycles.
+  const problems = [];
+  for (const { cite, startsWith, contains, claim } of LINE_CITATIONS) {
+    const start = Number(cite.split("-")[0]);
+    if (!startsWith.test(scriptLines[start - 1] ?? "")) {
+      // Report WHERE it moved to. Every matching line is listed rather than
+      // just the first: if the anchor is no longer unique, that ambiguity is
+      // itself the thing to fix, and silently naming one of several would be
+      // the same confidently-wrong answer this test exists to prevent.
+      const at = scriptLines
+        .map((line, i) => (startsWith.test(line) ? i + 1 : 0))
+        .filter(Boolean);
+      problems.push(
+        at.length === 0
+          ? `  :${cite} claims to point at ${claim}, which is no longer anywhere in the script -- it is dangling, so drop it or repoint it at whatever replaced that code`
+          : `  :${cite} no longer points at ${claim} -- that content now starts at :${at.join(" or :")}`,
+      );
+      continue;
+    }
+    if (contains && !contains.test(citedRange(cite))) {
+      problems.push(
+        `  :${cite} starts in the right place, but the rest of the range no longer holds ${claim} -- the citing comment describes content that is not there any more`,
+      );
+    }
+  }
+  assert.deepEqual(
+    problems,
+    [],
+    `line citations have drifted from the script:\n${problems.join("\n")}\nUpdate the comments that cite them, and this table, to match.`,
+  );
+});
+
+test("no line citation can be added to this file without being pinned", () => {
+  // The completeness half, and the one that makes the pinning durable: without
+  // it a NEW citation is unpinned by default and the convention rots again from
+  // the next comment onward. Scans this file's own source, so the two sets are
+  // derived from the same place a reader looks.
+  const testSource = readFileSync(fileURLToPath(import.meta.url), "utf8");
+  const found = new Set(
+    [...testSource.matchAll(/`:(\d+(?:-\d+)?)`/g)].map((m) => m[1]),
+  );
+  const pinned = new Set(LINE_CITATIONS.map((c) => c.cite));
+  assert.deepEqual(
+    [...found].filter((c) => !pinned.has(c)).sort(),
+    [],
+    "a `:NNN` citation was added without a LINE_CITATIONS entry -- add one naming the phrase it points at",
+  );
+  assert.deepEqual(
+    [...pinned].filter((c) => !found.has(c)).sort(),
+    [],
+    "a LINE_CITATIONS entry no longer matches any citation in the file -- remove it rather than leaving it pinning nothing",
+  );
+});