fix(scripts): flush check-run annotation via writeSync before exit (lr-e551b9)

scripts/check-test-count.js

@@ -61,11 +61,14 @@
 //
 var TEST_COUNT_FLOOR = 1300;
 
+var fs = require("fs");
 var path = require("path");
 var { spawnSync } = require("child_process");
 
 // ---------------------------------------------------------------------------
-// CI legibility (lr-e551b9, MILLER re-diagnosis of PR #400 / lr-4e1242 seq 5).
+// CI legibility (lr-e551b9, MILLER re-diagnosis of PR #400 / lr-4e1242 seq 5;
+// re-diagnosed again post-merge, same task, when the shipped fix turned out
+// not to survive a real CI pipe — see DELIVERY below).
 //
 // GitHub Actions does not surface arbitrary stderr text anywhere a crew
 // agent can reach it: the raw job log requires following a redirect to a
@@ -77,7 +80,100 @@ var { spawnSync } = require("child_process");
 // agent can actually read it, without changing which conditions fail the
 // run (see the module header above: every condition that fails today must
 // keep failing).
+//
+// DELIVERY (lr-e551b9 reopen, MILLER third-pass diagnosis, confidence 0.93,
+// reproduced locally and in CI on PR #400's rebased head): the first version
+// of this function used `process.stdout.write`. When stdout is a PIPE
+// (always true in CI, and in any captured run), that write is ASYNCHRONOUS —
+// it queues in userspace once the 64KB kernel pipe buffer fills. The
+// multi-MB TAP dump this script also writes to stdout vastly exceeds that
+// buffer, so by the time emitAnnotation() ran, its own write was queued
+// BEHIND the still-draining TAP blob. process.exit() then tore the process
+// down without draining the queue, so the annotation reached the Writable
+// stream object and never reached the file descriptor: exit code 1 with NO
+// annotation, indistinguishable from a genuine test failure at the very
+// point this script exists to make that distinguishable.
+//
+// Fixed by writing the annotation with fs.writeSync(1, ...) instead of
+// process.stdout.write(...). writeSync is a direct, synchronous syscall to
+// the fd — it is not subject to the Writable stream's internal async queue
+// at all, so there is no buffer to race process.exit() against. This is
+// belt: it holds even if something upstream changes how/when the process
+// terminates. See below for suspenders (annotation ordered before the large
+// stdout dump; process.exitCode instead of process.exit()).
 // ---------------------------------------------------------------------------
+// writeFullySync(fd, data) — fs.writeSync performs exactly ONE write(2)
+// syscall attempt and returns the number of bytes actually written; it does
+// NOT loop to guarantee the whole buffer lands, and a pipe fd can legitimately
+// short-write under backpressure once the payload is large (BOBBIE, PR #402
+// comment 5360992190). The single unlooped call this replaced discarded that
+// return value entirely, so a short write silently truncated its output —
+// exactly the class of silent-truncation defect this whole task (lr-e551b9)
+// exists to eliminate, now against the raw TAP evidence trail instead of the
+// verdict. This loops until every byte is confirmed written.
+//
+// `data` may be a Buffer or a string; if it is a string it is converted to a
+// Buffer ONCE up front, and the loop advances over that Buffer by byte
+// offset. This matters because fs.writeSync's offset/length/return-value
+// semantics are BYTE-indexed always, but a JS string is indexed by UTF-16
+// code unit — slicing a string by a byte count returned from a short write
+// would misalign mid multi-byte UTF-8 sequence. Converting once avoids that
+// mismatch entirely rather than trying to reconcile the two index spaces on
+// every partial-write iteration.
+//
+// Any thrown error (e.g. EPIPE/EAGAIN) propagates uncaught — deliberately not
+// swallowed here. This function is called from FAIL-path code whose only job
+// is to make a non-zero exit legible; the exit code itself is already
+// determined by classifyRun() before either write site runs (see
+// process.exitCode below), so an uncaught throw here can only ever turn an
+// already-nonzero process into a hard crash (still non-zero), never a FAIL
+// verdict into exit 0. Swallowing the error here would risk exactly that.
+//
+// NON-ADVANCING WRITE GUARD (lr-e551b9 fold-in, PEACHES PR #402 BLOCKING).
+// fs.writeSync's return value is the ONLY thing that advances `offset`; the
+// Node docs do not rule out a 0-byte return (distinct from throwing EAGAIN),
+// and this loop's sole invariant is that it always terminates. An unguarded
+// `while (offset < buffer.length)` against a 0-byte return spins forever —
+// on this specific script, the merge-gate wrapper whose entire purpose is
+// turning a FAIL into a legible signal, an infinite loop here is strictly
+// worse than the truncation bug it replaced: truncation still failed fast
+// with a usable exit code, while a hang burns the runner until the workflow
+// timeout kills it and produces NO verdict and NO annotation at all.
+//
+// A single 0-byte return is not necessarily fatal — a transient EAGAIN-like
+// stall on a pipe under backpressure could plausibly resolve on the very
+// next attempt without ever surfacing as a thrown error. But an unbounded
+// RUN of 0-byte returns with no forward progress is exactly the spin this
+// guard exists to rule out. MAX_CONSECUTIVE_ZERO_WRITES bounds that run:
+// each 0-byte return increments a counter; any return >0 resets it; hitting
+// the bound throws. Throwing (not returning/silently giving up) matches the
+// existing EPIPE/EAGAIN posture directly above — an uncaught throw always
+// exits non-zero, so it can never produce a false green, and it surfaces
+// immediately in CI as a hard crash rather than a silent short-delivery.
+var MAX_CONSECUTIVE_ZERO_WRITES = 100;
+
+function writeFullySync(fd, data) {
+  var buffer = Buffer.isBuffer(data) ? data : Buffer.from(String(data), "utf8");
+  var offset = 0;
+  var consecutiveZeroWrites = 0;
+  while (offset < buffer.length) {
+    var written = fs.writeSync(fd, buffer, offset, buffer.length - offset);
+    if (written === 0) {
+      consecutiveZeroWrites += 1;
+      if (consecutiveZeroWrites >= MAX_CONSECUTIVE_ZERO_WRITES) {
+        throw new Error(
+          "writeFullySync: fs.writeSync returned 0 " + consecutiveZeroWrites +
+          " times in a row with no forward progress (delivered " + offset + " of " +
+          buffer.length + " bytes) — aborting instead of spinning forever."
+        );
+      }
+      continue;
+    }
+    consecutiveZeroWrites = 0;
+    offset += written;
+  }
+}
+
 function emitAnnotation(message) {
   // GitHub Actions workflow-command syntax. `%`, CR and LF must be escaped
   // in the message text per GitHub's documented encoding for `::error::` —
@@ -86,7 +182,7 @@ function emitAnnotation(message) {
     .replace(/%/g, "%25")
     .replace(/\r/g, "%0D")
     .replace(/\n/g, "%0A");
-  process.stdout.write("::error::" + escaped + "\n");
+  writeFullySync(1, "::error::" + escaped + "\n");
 }
 
 // classifyRun() is the pure decision core of this script: given the raw
@@ -203,7 +299,11 @@ function classifyRun(result, files, resultsByFile, totalTests, floor) {
   return { ok: true, exitCode: 0, kind: "ok", reason: null };
 }
 
-module.exports = { classifyRun: classifyRun, emitAnnotation: emitAnnotation };
+module.exports = {
+  classifyRun: classifyRun,
+  emitAnnotation: emitAnnotation,
+  writeFullySync: writeFullySync,
+};
 
 // Everything below only runs when this file is executed directly (`node
 // scripts/check-test-count.js <file...>`), not when required as a module by
@@ -227,8 +327,6 @@ if (require.main === module) {
     maxBuffer: 64 * 1024 * 1024,
   });
 
-  if (result.stdout) process.stdout.write(result.stdout);
-
   // The custom reporter's RESULT lines are the only thing routed to stderr —
   // forward everything else Node itself wrote to stderr (real errors,
   // warnings) so a normal `npm test` run still surfaces them, then parse the
@@ -255,6 +353,14 @@ if (require.main === module) {
   var totalTests = passCount + failCount;
   var verdict = classifyRun(result, files, resultsByFile, totalTests, TEST_COUNT_FLOOR);
 
+  // DELIVERY ORDER (lr-e551b9 reopen). The annotation is emitted BEFORE the
+  // large TAP dump below, not after: emitAnnotation() now uses
+  // fs.writeSync(1, ...), a direct synchronous syscall that bypasses the
+  // stdout Writable's async queue entirely, so this ordering is not load-
+  // bearing for the annotation's own delivery — but it also means the
+  // annotation is never at risk of landing "behind" a dump that is itself
+  // subject to the async-pipe race (see below), belt-and-suspenders rather
+  // than relying on ordering alone.
   if (!verdict.ok) {
     var line = "[check-test-count] FAIL (" + verdict.kind + "): " + verdict.reason;
     process.stderr.write(line + "\n");
@@ -265,5 +371,27 @@ if (require.main === module) {
     emitAnnotation(line);
   }
 
-  process.exit(verdict.exitCode);
+  // The raw TAP dump is written with writeFullySync too, for the same reason
+  // as the annotation above: process.stdout.write() queues asynchronously
+  // once a pipe's 64KB kernel buffer fills, and a ~1400-test TAP blob is
+  // multi-MB — far larger than that buffer. Losing the raw output is a
+  // real, independent problem even though the annotation above no longer
+  // depends on it: a developer reading a captured `npm test` run should
+  // still see the actual TAP text, not a truncation artifact. A single
+  // unlooped fs.writeSync call here would carry the same short-write risk
+  // as the annotation write did (BOBBIE, PR #402) — writeFullySync loops
+  // until the whole multi-MB buffer is confirmed delivered.
+  if (result.stdout) writeFullySync(1, result.stdout);
+
+  // process.exitCode (not process.exit()) lets the event loop drain
+  // naturally instead of tearing the process down immediately — the second
+  // half of MILLER's recommended (a)+(b) combination. Nothing below this
+  // line can reset process.exitCode or call process.exit(0): this is the
+  // last statement in the script, so there is no later code path that could
+  // turn a red run green. The FAIL-OPEN CONSTRAINT (lr-795882, engram
+  // 7613980) requires this be verified, not assumed — every exit-1
+  // condition above still sets verdict.exitCode to 1 or the child's own
+  // non-zero status; process.exitCode is set from that value unconditionally
+  // and nothing else touches it.
+  process.exitCode = verdict.exitCode;
 }

test/check-test-count-delivery-lr-e551b9.test.js

@@ -0,0 +1,165 @@
+"use strict";
+// Regression test for the lr-e551b9 REOPEN (MILLER third-pass diagnosis,
+// confidence 0.93, reproduced locally and in CI on PR #400's rebased head
+// d7dd88ae). The PR #401 fix made classifyRun() name the right failure
+// reason, but its DELIVERY was broken: check-test-count.js wrote the
+// ::error:: annotation with process.stdout.write() and then called
+// process.exit() before that write could flush. process.stdout.write is
+// ASYNCHRONOUS when stdout is a real OS pipe (always true in CI); a
+// multi-MB preceding TAP dump exceeds the 64KB kernel pipe buffer, so the
+// remainder — including the annotation queued behind it — sits in a
+// userspace buffer that process.exit() tears down without draining. The
+// annotation reached the Writable stream object and never reached the fd.
+//
+// test/check-test-count-signal-legibility-lr-e551b9.test.js already covers
+// classifyRun()'s verdict/precedence logic and emitAnnotation()'s message
+// FORMAT — that coverage is correct for what it tests and is kept unchanged.
+// It stubs process.stdout.write with a function that pushes to an array and
+// returns true, which is exactly why it could not catch this: a stub that
+// always "succeeds" cannot observe flush, backpressure, or the interaction
+// between an async queue and process.exit(). It proves the message is
+// FORMATTED correctly; it cannot prove the message is DELIVERED. A write is
+// not a delivery. This file is the delivery test PEACHES, BOBBIE and NAOMI's
+// prior review passes did not have, because none of them exercised a real
+// pipe.
+//
+// METHOD: spawn the actual scripts/check-test-count.js as a CHILD PROCESS
+// (not require()'d, not stubbed) with stdio: ["ignore", "pipe", "pipe"] — a
+// real OS pipe, matching CI exactly — against a fixture file
+// (test/fixtures/check-test-count-padding-lr-e551b9.fixture.js) that emits
+// ~2000 trivial passing tests, comfortably exceeding a 64KB TAP dump, plus a
+// deliberately nonexistent second file to force the real missing-files FAIL
+// path (a genuine wrapper-level condition, not a stub). The parent reads the
+// child's stdout the way a CI log collector does — accumulate chunks,
+// nothing more — and asserts the captured bytes contain the ::error:: line.
+//
+// DEMONSTRATED-FAILURE VERIFICATION (lr-4e1242 convention), STRONG FORM:
+// this exact test file, run via `npx node --test
+// test/check-test-count-delivery-lr-e551b9.test.js`, was executed against
+// the pre-fix script at commit cf4b734a (git show cf4b734a:scripts/check-
+// test-count.js, restored to a scratch copy since the working tree already
+// carries the fix) BEFORE the delivery fix in this diff was written. It
+// failed with a genuine wrong-content assertion — not a missing-symbol or
+// spawn error:
+//
+//   AssertionError [ERR_ASSERTION]: the ::error:: annotation must be present
+//   in the child's captured stdout — the wrapper must not lose it to an
+//   unflushed pipe
+//   + actual: false
+//   - expected: true
+//
+// and the captured stdout in that pre-fix run measured 65536 bytes (exactly
+// one 64KB pipe high-water mark) with zero occurrences of "::error::",
+// reproducing MILLER's probe finding (mid-word truncation, no annotation)
+// exactly. After the fix in this diff (fs.writeSync for both the annotation
+// and the TAP dump, plus process.exitCode instead of process.exit()), the
+// same test passes: the annotation is present regardless of TAP dump size.
+// This is a real absent-annotation failure against production code, not a
+// missing-symbol error — the strong form PEACHES requires.
+var test = require("node:test");
+var assert = require("node:assert/strict");
+var path = require("path");
+var { spawn } = require("child_process");
+
+var WRAPPER_PATH = path.join(__dirname, "..", "scripts", "check-test-count.js");
+var PADDING_FIXTURE = path.join(__dirname, "fixtures", "check-test-count-padding-lr-e551b9.fixture.js");
+var MISSING_FIXTURE = path.join(__dirname, "fixtures", "check-test-count-DOES-NOT-EXIST-lr-e551b9.test.js");
+
+// Runs the real wrapper as a child process with a genuine OS pipe on stdout
+// and stderr, and resolves with the full captured output plus exit code.
+// Deliberately does NOT force any flush/drain on the parent side — a CI log
+// collector does not either; if the child loses data to an unflushed queue,
+// this harness must observe that loss, not paper over it.
+function runWrapperThroughRealPipe(args) {
+  return new Promise(function (resolve, reject) {
+    // This test file itself runs under `node --test`, which sets
+    // NODE_TEST_CONTEXT in its own process.env and — because child_process
+    // inherits the parent's env by default — that value leaks into the
+    // spawned child below. The wrapper we're spawning ALSO runs `node
+    // --test` internally, and Node's test runner treats an inherited
+    // NODE_TEST_CONTEXT as "I am a nested test-runner child", which trips
+    // its own recursion guard and makes it skip actually running the
+    // fixture file (silently reported here as "missing-files", NOT the
+    // production defect under test). Stripping it is required for this
+    // harness to observe the real wrapper's behavior instead of Node's own
+    // nested-test-runner guard.
+    var childEnv = Object.assign({}, process.env);
+    delete childEnv.NODE_TEST_CONTEXT;
+
+    var child = spawn(process.execPath, [WRAPPER_PATH].concat(args), {
+      stdio: ["ignore", "pipe", "pipe"],
+      env: childEnv,
+    });
+
+    var stdoutChunks = [];
+    var stderrChunks = [];
+
+    child.stdout.on("data", function (chunk) {
+      stdoutChunks.push(chunk);
+    });
+    child.stderr.on("data", function (chunk) {
+      stderrChunks.push(chunk);
+    });
+    child.on("error", reject);
+    child.on("close", function (code) {
+      resolve({
+        exitCode: code,
+        stdout: Buffer.concat(stdoutChunks).toString("utf8"),
+        stderr: Buffer.concat(stderrChunks).toString("utf8"),
+      });
+    });
+  });
+}
+
+test(
+  "lr-e551b9 delivery: a fail path (missing-files) behind a large preceding TAP dump still delivers the ::error:: annotation through a real pipe",
+  { timeout: 30000 },
+  async function () {
+    var result = await runWrapperThroughRealPipe([PADDING_FIXTURE, MISSING_FIXTURE]);
+
+    // Sanity: this must genuinely be the missing-files fail path, not some
+    // other failure mode — assert the exit code and the reason line, not
+    // just the annotation, so a future regression that changes WHICH path
+    // fires is also caught here.
+    assert.equal(result.exitCode, 1, "the missing-files condition must still fail the run (fail-open constraint, lr-795882)");
+    assert.ok(
+      result.stderr.indexOf("[check-test-count] FAIL (missing-files)") !== -1,
+      "stderr must still carry the plain-text FAIL line; got: " + result.stderr.slice(-500)
+    );
+
+    // The load-bearing assertion: the annotation must be present in the
+    // CHILD'S ACTUAL CAPTURED stdout, past a TAP dump large enough to
+    // exceed a 64KB pipe buffer. This is exactly the assertion that fails
+    // against the pre-fix script (see the file header's demonstrated-
+    // failure verification) and passes against the fix in this diff.
+    assert.ok(
+      result.stdout.indexOf("::error::") !== -1,
+      "the ::error:: annotation must be present in the child's captured stdout — the wrapper must not lose it to an unflushed pipe"
+    );
+    assert.ok(
+      result.stdout.indexOf("missing-files") !== -1,
+      "the delivered annotation must name the actual verdict kind, not just any ::error:: text"
+    );
+
+    // The raw TAP dump must also survive intact — MILLER/HOLDEN both noted
+    // that losing the annotation's ride-along blob (the raw test output) is
+    // an independent real problem, not just the annotation. A truncated
+    // capture ends abruptly with no TAP summary footer; assert the footer
+    // is present as a proxy for "the dump was not cut off mid-stream".
+    assert.ok(
+      result.stdout.indexOf("# duration_ms") !== -1,
+      "the TAP summary footer must survive intact — a truncated dump would be missing it, same as MILLER's probe finding"
+    );
+  }
+);
+
+test(
+  "lr-e551b9 delivery: a clean run (no fail path) behind the same large TAP dump emits no annotation and exits 0",
+  { timeout: 30000 },
+  async function () {
+    var result = await runWrapperThroughRealPipe([PADDING_FIXTURE]);
+
+    assert.equal(result.exitCode, 0, "a genuinely clean run must still pass — the delivery fix must not introduce a new false FAIL");
+    assert.ok(result.stdout.indexOf("::error::") === -1, "no annotation should be emitted when nothing failed");
+  }
+);