From 2aeb6e078b3fc07b57ec612bcb2b06e16329c960 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 15:47:00 -0400 Subject: [PATCH 1/5] fix(scripts): flush check-run annotation via writeSync before exit (lr-e551b9) check-test-count.js emitted the ::error:: annotation with process.stdout.write() and then called process.exit() before the write could flush. process.stdout.write is asynchronous when stdout is a pipe (always true in CI); the multi-MB TAP dump this script also writes to stdout exceeds the 64KB kernel pipe buffer, so the annotation queued behind it and process.exit() discarded the queue without draining it. Exit code 1, zero annotation. Fixed with fs.writeSync(1, ...) for both the annotation and the raw TAP dump (writeSync bypasses the async Writable queue entirely), the annotation ordered before the large dump, and process.exitCode instead of process.exit() so the event loop drains naturally. Every exit-1 condition in classifyRun() is unchanged; only delivery of the reason changed (fail-open constraint, lr-795882, engram 7613980). MILLER (lr-e551b9 reopen, confidence 0.93) diagnosed this after HOLDEN verified empirically on PR #400's rebased head d7dd88ae that no ::error:: annotation reached the check-run despite the run going red. TASK: lr-e551b9 --- scripts/check-test-count.js | 59 +++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js index b331c9db..4bb6deb9 100644 --- a/scripts/check-test-count.js +++ b/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,6 +80,27 @@ 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()). // --------------------------------------------------------------------------- function emitAnnotation(message) { // GitHub Actions workflow-command syntax. `%`, CR and LF must be escaped @@ -86,7 +110,7 @@ function emitAnnotation(message) { .replace(/%/g, "%25") .replace(/\r/g, "%0D") .replace(/\n/g, "%0A"); - process.stdout.write("::error::" + escaped + "\n"); + fs.writeSync(1, "::error::" + escaped + "\n"); } // classifyRun() is the pure decision core of this script: given the raw @@ -227,8 +251,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 +277,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 +295,24 @@ if (require.main === module) { emitAnnotation(line); } - process.exit(verdict.exitCode); + // The raw TAP dump is written with fs.writeSync 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. + if (result.stdout) fs.writeSync(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; } From 4abe580a85ae4046ef3b26423bf4ffd2870a9ddd Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 15:47:10 -0400 Subject: [PATCH 2/5] test(scripts): real-pipe delivery regression for check-test-count.js (lr-e551b9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test/check-test-count-signal-legibility-lr-e551b9.test.js stubs process.stdout.write with a function that pushes to an array and returns true, so it can only verify annotation message FORMAT, not delivery through a real pipe — it is structurally incapable of observing flush, backpressure, or the process.exit() race that actually lost the annotation in production. This adds a delivery test that spawns the real scripts/check-test-count.js as a child process with stdio: ["ignore", "pipe", "pipe"] (a genuine OS pipe, matching CI), against a fixture (test/fixtures/check-test-count-padding-lr-e551b9.fixture.js, ~2000 trivial tests) that pushes TAP output past the 64KB pipe buffer, plus a deliberately missing file to force a real missing-files FAIL path. It asserts the ::error:: annotation is present in the child's actual captured stdout, and that a clean run still emits none and exits 0. Demonstrated-failure verification (lr-4e1242 convention), strong form: this test file, run against the pre-fix script at cf4b734a (restored to a scratch copy before the delivery fix in this branch existed), failed with a genuine wrong-content assertion — "the ::error:: annotation must be present in the child's captured stdout ... actual: false" — not a missing-symbol or spawn error. After the delivery fix, the same test passes. Keeps the existing classifyRun() verdict/precedence tests unchanged; this adds delivery coverage alongside, it does not replace them. TASK: lr-e551b9 --- ...heck-test-count-delivery-lr-e551b9.test.js | 165 ++++++++++++++++++ ...ck-test-count-padding-lr-e551b9.fixture.js | 23 +++ 2 files changed, 188 insertions(+) create mode 100644 test/check-test-count-delivery-lr-e551b9.test.js create mode 100644 test/fixtures/check-test-count-padding-lr-e551b9.fixture.js diff --git a/test/check-test-count-delivery-lr-e551b9.test.js b/test/check-test-count-delivery-lr-e551b9.test.js new file mode 100644 index 00000000..372b9e63 --- /dev/null +++ b/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"); + } +); diff --git a/test/fixtures/check-test-count-padding-lr-e551b9.fixture.js b/test/fixtures/check-test-count-padding-lr-e551b9.fixture.js new file mode 100644 index 00000000..bd6f627e --- /dev/null +++ b/test/fixtures/check-test-count-padding-lr-e551b9.fixture.js @@ -0,0 +1,23 @@ +"use strict"; +// Fixture for test/check-test-count-delivery-lr-e551b9.test.js — NOT itself +// collected by `npm test` (package.json only globs test/*.test.js; this file +// lives under test/fixtures/ and does not match that pattern, and does not +// end in .test.js). +// +// Emits enough trivial passing node:test cases that the resulting TAP output +// comfortably exceeds a 64KB OS pipe buffer, which is what the delivery test +// needs to force process.stdout.write's async-queue behavior when stdout is +// a real pipe (see that test file for why this matters — lr-e551b9 reopen). +var test = require("node:test"); +var assert = require("node:assert/strict"); + +for (var i = 0; i < 2000; i++) { + test( + "lr-e551b9 padding test number " + i + " — exists only to bulk up TAP " + + "output past a 64KB pipe buffer so the delivery test can force the " + + "async-write race this fixture is built to exercise", + function () { + assert.equal(1, 1); + } + ); +} From 4bf735fcba210f8463f12b54ecbe861f29392cd0 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 15:47:18 -0400 Subject: [PATCH 3/5] test(scripts): retarget emitAnnotation format tests at fs.writeSync (lr-e551b9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit emitAnnotation() now calls fs.writeSync(1, ...) instead of process.stdout.write() (see the prior commit in this branch). The two message-format tests in this file previously stubbed process.stdout.write, so they silently stopped observing anything — fs.writeSync is a distinct code path a process.stdout.write stub cannot intercept, and the tests started failing outright once the implementation changed underneath them. Retargets both tests to spy on fs.writeSync (fd 1) instead, which is the primitive emitAnnotation actually calls today. Same assertions, same coverage of the ::error:: prefix and the %/CR/LF escaping — only the spy target changed to stay truthful about the implementation. TASK: lr-e551b9 --- ...-count-signal-legibility-lr-e551b9.test.js | 44 ++++++++++++++----- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/test/check-test-count-signal-legibility-lr-e551b9.test.js b/test/check-test-count-signal-legibility-lr-e551b9.test.js index 4a0bb436..fae0cc18 100644 --- a/test/check-test-count-signal-legibility-lr-e551b9.test.js +++ b/test/check-test-count-signal-legibility-lr-e551b9.test.js @@ -222,17 +222,35 @@ test("lr-e551b9: check ORDER — missing-files still wins over signal-death even assert.ok(verdict.reason.indexOf("test/killed-mid-run.test.js") !== -1); }); -test("lr-e551b9: emitAnnotation writes a ::error:: prefixed workflow command to stdout", function () { +// NOTE (lr-e551b9 reopen): emitAnnotation() is now implemented with +// fs.writeSync(1, ...) rather than process.stdout.write(...) — see +// check-test-count.js's DELIVERY comment above emitAnnotation for why: a +// process.stdout.write-based stub, exactly like the two tests below used to +// use, cannot observe the async-pipe/process.exit() race that lost the +// annotation in production (that is precisely the gap MILLER's reopen +// diagnosis found — see test/check-test-count-delivery-lr-e551b9.test.js for +// the through-a-real-pipe regression test that covers actual delivery). +// These two tests still verify message FORMAT — the ::error:: prefix and the +// GitHub workflow-command escaping — which is a real, narrower thing worth +// unit-testing fast and in isolation; they now spy on fs.writeSync (the +// primitive the implementation actually calls) instead of stubbing +// process.stdout.write (a primitive it no longer calls), so the spy remains +// truthful about what emitAnnotation does today. +test("lr-e551b9: emitAnnotation writes a ::error:: prefixed workflow command to fd 1", function () { + var fs = require("fs"); var chunks = []; - var originalWrite = process.stdout.write; - process.stdout.write = function (chunk) { - chunks.push(chunk); - return true; + var originalWriteSync = fs.writeSync; + fs.writeSync = function (fd, chunk) { + if (fd === 1) { + chunks.push(chunk); + return chunk.length; + } + return originalWriteSync.apply(fs, arguments); }; try { checkTestCount.emitAnnotation("[check-test-count] FAIL (signal-death): node --test was killed by SIGKILL"); } finally { - process.stdout.write = originalWrite; + fs.writeSync = originalWriteSync; } var written = chunks.join(""); @@ -241,16 +259,20 @@ test("lr-e551b9: emitAnnotation writes a ::error:: prefixed workflow command to }); test("lr-e551b9: emitAnnotation escapes %, CR and LF per GitHub's documented workflow-command encoding", function () { + var fs = require("fs"); var chunks = []; - var originalWrite = process.stdout.write; - process.stdout.write = function (chunk) { - chunks.push(chunk); - return true; + var originalWriteSync = fs.writeSync; + fs.writeSync = function (fd, chunk) { + if (fd === 1) { + chunks.push(chunk); + return chunk.length; + } + return originalWriteSync.apply(fs, arguments); }; try { checkTestCount.emitAnnotation("100% failure\r\nline two"); } finally { - process.stdout.write = originalWrite; + fs.writeSync = originalWriteSync; } var written = chunks.join(""); From 1f1cdd468160e8fadfb66a57a986e7f19d9d4b10 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 16:02:05 -0400 Subject: [PATCH 4/5] fix(scripts): loop writeSync calls to survive short writes (lr-e551b9) BOBBIE PR #402 nit, folded in per task instruction: both writeSync call sites in check-test-count.js (the ::error:: annotation and the raw TAP dump) used a single, unlooped fs.writeSync with the return value discarded. fs.writeSync performs exactly one write(2) attempt; a pipe fd can legitimately short-write under backpressure at multi-MB size. This reintroduced a milder form of the exact silent-truncation class lr-e551b9 exists to eliminate, now against the raw TAP evidence trail. Adds a shared writeFullySync(fd, data) helper that loops until every byte is confirmed written, converting a string input to a Buffer once up front so a short write's byte-count return value never misaligns against UTF-16 code-unit indices on a multi-byte UTF-8 sequence. Both call sites now route through it. Errors (EPIPE/EAGAIN) still propagate uncaught, preserving the fail-open constraint (lr-795882): the exit code is already determined by classifyRun() before either write site runs, so an uncaught throw here can only escalate an already-nonzero exit, never reset it to 0. Adds a unit test exercising writeFullySync directly with an injected fs.writeSync stub that deliberately returns partial byte counts, covering the short-write path structurally rather than empirically (the existing delivery test only proves the loop happens to work at today's fixture size). Verified this test fails with a genuine wrong-content assertion against the pre-fix unlooped shape and passes against the fix. TASK: lr-e551b9 --- scripts/check-test-count.js | 51 +++++++- ...k-test-count-short-write-lr-e551b9.test.js | 116 ++++++++++++++++++ 2 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 test/check-test-count-short-write-lr-e551b9.test.js diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js index 4bb6deb9..35b2495a 100644 --- a/scripts/check-test-count.js +++ b/scripts/check-test-count.js @@ -102,6 +102,40 @@ var { spawnSync } = require("child_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. +function writeFullySync(fd, data) { + var buffer = Buffer.isBuffer(data) ? data : Buffer.from(String(data), "utf8"); + var offset = 0; + while (offset < buffer.length) { + offset += fs.writeSync(fd, buffer, offset, buffer.length - offset); + } +} + 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::` — @@ -110,7 +144,7 @@ function emitAnnotation(message) { .replace(/%/g, "%25") .replace(/\r/g, "%0D") .replace(/\n/g, "%0A"); - fs.writeSync(1, "::error::" + escaped + "\n"); + writeFullySync(1, "::error::" + escaped + "\n"); } // classifyRun() is the pure decision core of this script: given the raw @@ -227,7 +261,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 `), not when required as a module by @@ -295,14 +333,17 @@ if (require.main === module) { emitAnnotation(line); } - // The raw TAP dump is written with fs.writeSync too, for the same reason + // 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. - if (result.stdout) fs.writeSync(1, result.stdout); + // 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 diff --git a/test/check-test-count-short-write-lr-e551b9.test.js b/test/check-test-count-short-write-lr-e551b9.test.js new file mode 100644 index 00000000..3980c2fe --- /dev/null +++ b/test/check-test-count-short-write-lr-e551b9.test.js @@ -0,0 +1,116 @@ +"use strict"; +// Regression test for the lr-e551b9 fold-in (BOBBIE, PR #402 comment +// 5360992190). Both writeSync call sites in scripts/check-test-count.js +// (the ::error:: annotation and the raw TAP dump) used a SINGLE, UNLOOPED +// fs.writeSync call with the return value discarded. fs.writeSync performs +// exactly one write(2) syscall attempt and does not loop to guarantee full +// delivery; a pipe fd can legitimately short-write under backpressure once +// the payload is large enough — the exact condition the multi-MB TAP dump +// this script also writes is built to hit. The existing delivery test +// (test/check-test-count-delivery-lr-e551b9.test.js) gives EMPIRICAL +// coverage — it happens to pass at that fixture's size in this CI shape — +// but nothing in it guarantees the loop as the suite grows or under a +// slower-reader backpressure shape that hits a mid-write short-write instead +// of a single flush boundary. This file unit-tests the write HELPER itself +// (writeFullySync, exported alongside classifyRun/emitAnnotation) with an +// injected write function that deliberately returns partial byte counts, so +// the short-write path is covered structurally, not just empirically. +// +// DEMONSTRATED-FAILURE VERIFICATION (lr-4e1242 convention): this exact +// short-write scenario (a 300KB buffer against a stub that always returns a +// 64KB-capped partial count, mirroring an OS pipe's kernel buffer) was run +// against a PRE-FIX single-unlooped-call shape (`if (buffer.length) +// writeSyncFn(fd, buffer, 0, buffer.length);`, discarding the return value — +// exactly what scripts/check-test-count.js's two writeSync call sites did +// before this diff) in an isolated scratch harness before this file was +// written. It failed with a genuine wrong-content assertion: +// +// AssertionError [ERR_ASSERTION]: delivered length must equal input length +// 65536 !== 300000 +// +// only 65536 of 300000 bytes were ever handed to the stub — the rest was +// silently discarded, reproducing the exact truncation class this task +// exists to eliminate. Against the post-fix looped shape (identical to +// writeFullySync below), the same scenario delivered all 300000 bytes +// exactly. This test below exercises the REAL exported writeFullySync +// function directly (not a reimplementation), so it fails against the +// current working tree if the loop is ever removed or short-circuited. +var test = require("node:test"); +var assert = require("node:assert/strict"); +var checkTestCount = require("../scripts/check-test-count.js"); + +test( + "lr-e551b9 short-write: writeFullySync keeps calling fs.writeSync until a short-write-prone destination has received every byte", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var delivered = []; + var callCount = 0; + var CHUNK_CAP = 65536; // mirrors a 64KB OS pipe kernel buffer short-write + + fs.writeSync = function (fd, buffer, offset, length) { + callCount += 1; + var n = Math.min(CHUNK_CAP, length); + delivered.push(Buffer.from(buffer.slice(offset, offset + n))); + return n; + }; + + var input = Buffer.alloc(300000, 65); // 300KB, comfortably > one chunk cap + + try { + checkTestCount.writeFullySync(1, input); + } finally { + fs.writeSync = originalWriteSync; + } + + var result = Buffer.concat(delivered); + + assert.ok( + callCount > 1, + "the stub must have been invoked more than once for this to be a real short-write test; got " + callCount + " call(s)" + ); + assert.equal( + result.length, input.length, + "writeFullySync must keep looping until the full buffer is delivered, not stop after the first short write" + ); + assert.ok( + result.equals(input), + "the concatenated delivered bytes must equal the input buffer exactly, byte for byte" + ); + } +); + +test( + "lr-e551b9 short-write: writeFullySync converts a string input to a Buffer once up front, so byte offsets from a short write never misalign against UTF-16 code-unit indices", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var delivered = []; + // Force a short write partway through a multi-byte UTF-8 sequence: "e" + // followed by a 3-byte euro sign, encoded as UTF-8 that is 4 bytes total + // (1 + 3). A cap of 2 forces the split to land INSIDE the euro sign's + // byte sequence on the first call, which only a byte-indexed Buffer + // offset (not a JS string character slice) can resume correctly. + var CHUNK_CAP = 2; + + fs.writeSync = function (fd, buffer, offset, length) { + var n = Math.min(CHUNK_CAP, length); + delivered.push(Buffer.from(buffer.slice(offset, offset + n))); + return n; + }; + + var input = "e€"; // "e" + EURO SIGN, 4 bytes UTF-8, 2 UTF-16 code units + + try { + checkTestCount.writeFullySync(1, input); + } finally { + fs.writeSync = originalWriteSync; + } + + var result = Buffer.concat(delivered); + assert.equal( + result.toString("utf8"), input, + "the reassembled bytes must decode back to the exact original string, even when a short write splits a multi-byte character" + ); + } +); From 0e54e615cb45b842e1a45088279d10a5c286e1e4 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 16:12:16 -0400 Subject: [PATCH 5/5] fix(scripts): bound writeFullySync against a non-advancing 0-byte writeSync return (lr-e551b9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PEACHES PR #402 BLOCKING: the writeFullySync loop added to fix short writes only advances offset by fs.writeSync's return value. A 0-byte return (distinct from a thrown EAGAIN/EPIPE) does not advance offset, so 'while (offset < buffer.length)' spins forever. On this specific script — the merge-gate wrapper whose entire purpose is turning a FAIL verdict into a legible signal — a hang here is strictly worse than the truncation bug it replaced: the job burns the runner until the workflow timeout kills it, producing NO verdict and NO annotation at all, versus a truncated log that still fails fast with a usable exit code. Adds MAX_CONSECUTIVE_ZERO_WRITES (100): a single 0-byte return does not throw immediately (a transient pipe stall is plausible and need not be fatal), but an unbounded run of consecutive 0-byte returns with no forward progress throws. Any return >0 resets the counter. Throwing matches the existing EPIPE/EAGAIN posture directly above it in the same function: an uncaught throw always exits non-zero, so it can never produce a false green, and it surfaces the failure immediately rather than as a silent short delivery. Adds a unit test exercising the 0-byte path directly against an injected fs.writeSync stub, using a bounded call-count safety valve (not a wall-clock timeout — node --test's own timeout mechanism cannot interrupt a synchronous, non-yielding while loop) so a regression to the unguarded spin fails fast with an assertable error instead of hanging the suite. Covers both an immediate 0-byte return and a mixed sequence (two real partial writes, then a run of zeros) so the guard is proven to apply after real progress has already been made, not only on a first-call zero. Verified the demonstrated failure the strong way: swapped in the pre-guard unguarded loop (git show 1f1cdd4:scripts/check-test-count.js) in the working tree, ran the new test file — both tests failed with genuine bounded assertion errors (the stub's own safety-valve tripped at 501 calls without the loop ever throwing its own error), not a hang and not a missing-symbol error. Restored the guarded version; both tests then passed. Full existing coverage (short-write, delivery, signal-legibility) and the full project suite (npm test, and a direct node --test run of test/*.test.js) still pass. TASK: lr-e551b9 --- scripts/check-test-count.js | 40 ++++- ...ck-test-count-zero-write-lr-e551b9.test.js | 142 ++++++++++++++++++ 2 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 test/check-test-count-zero-write-lr-e551b9.test.js diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js index 35b2495a..ee9c58cc 100644 --- a/scripts/check-test-count.js +++ b/scripts/check-test-count.js @@ -128,11 +128,49 @@ var { spawnSync } = require("child_process"); // 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) { - offset += fs.writeSync(fd, buffer, offset, buffer.length - offset); + 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; } } diff --git a/test/check-test-count-zero-write-lr-e551b9.test.js b/test/check-test-count-zero-write-lr-e551b9.test.js new file mode 100644 index 00000000..2647ef34 --- /dev/null +++ b/test/check-test-count-zero-write-lr-e551b9.test.js @@ -0,0 +1,142 @@ +"use strict"; +// Regression test for the lr-e551b9 fold-in (PEACHES PR #402 BLOCKING). +// writeFullySync (test/check-test-count-short-write-lr-e551b9.test.js) +// advances `offset` ONLY by fs.writeSync's return value: +// offset += fs.writeSync(fd, buffer, offset, buffer.length - offset); +// If fs.writeSync ever returns 0, offset does not advance and +// `while (offset < buffer.length)` spins FOREVER. On this specific script — +// the merge-gate wrapper whose entire purpose is turning a FAIL verdict into +// a legible signal — an infinite loop here is strictly worse than the +// truncation bug it replaced: the job burns its runner until the workflow +// timeout kills it, producing NO verdict and NO annotation at all, versus a +// truncated log that still fails fast with a usable exit code. +// +// This file covers the 0-byte-return path directly: assert the guard +// terminates (does not hang) and surfaces the failure as a throw rather than +// looping, via a BOUNDED call-count assertion rather than a real wall-clock +// timeout — a regression to the unguarded loop calls the stub an unbounded +// number of times with a 0 return and never returns control to the test +// runner at all, so a real-time `{ timeout }` on the test itself would only +// convert a spin into a hung *suite* (node --test's own timeout mechanism +// cannot interrupt a synchronous, non-yielding while loop — there is no +// event-loop turn for it to fire on). The stub instead throws once its own +// call count exceeds a bound comfortably above the guard's configured +// threshold, so an unguarded loop fails with a bounded, assertable stub +// error instead of hanging the process, and CI still reports a fast FAIL +// rather than stalling on a spinning regression (the exact "a test that +// hangs on regression is nearly as bad as the bug" trap named in this +// task's dispatch). +var test = require("node:test"); +var assert = require("node:assert/strict"); +var checkTestCount = require("../scripts/check-test-count.js"); + +// Comfortably above writeFullySync's own MAX_CONSECUTIVE_ZERO_WRITES bound +// (100) so the stub never trips before the guard has a chance to act, but +// still small enough that an unguarded regression fails near-instantly +// instead of iterating meaningfully long. +var STUB_CALL_BOUND = 500; + +test( + "lr-e551b9 zero-write: writeFullySync throws instead of spinning forever when fs.writeSync returns 0", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var callCount = 0; + + fs.writeSync = function () { + callCount += 1; + if (callCount > STUB_CALL_BOUND) { + // Safety valve for a REGRESSION to the unguarded loop: without this, + // an unguarded writeFullySync would call this stub forever and the + // test process would never yield back to node --test's own result + // reporting — the exact "hangs on regression" failure mode this test + // exists to avoid inflicting on CI. Throwing here still fails the + // test (the throw propagates out of writeFullySync, uncaught by the + // test body below, and node --test reports it as a failure) — it + // just fails FAST instead of hanging. + throw new Error( + "writeFullySync called the 0-byte-returning stub " + callCount + + " times without throwing its own bounded-retry error — the " + + "non-advancing-write guard is missing or broken (regression to " + + "the unguarded spin this test exists to catch)." + ); + } + return 0; + }; + + var input = Buffer.from("hello world", "utf8"); + + try { + assert.throws( + function () { + checkTestCount.writeFullySync(1, input); + }, + function (err) { + return err instanceof Error && !/non-advancing-write guard/.test(err.message); + }, + "writeFullySync must throw its OWN bounded-retry error, not exhaust the test's safety-valve bound first" + ); + } finally { + fs.writeSync = originalWriteSync; + } + + assert.ok( + callCount <= STUB_CALL_BOUND, + "writeFullySync must give up well before " + STUB_CALL_BOUND + " zero-byte returns; got " + callCount + " calls" + ); + } +); + +test( + "lr-e551b9 zero-write: a mixed sequence (partial, partial, then a run of zeros) still terminates via the guard, not just a first-call zero", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var callCount = 0; + var progressCalls = 0; + + fs.writeSync = function (fd, buffer, offset, length) { + callCount += 1; + if (callCount > STUB_CALL_BOUND) { + throw new Error( + "writeFullySync called the stub " + callCount + " times without " + + "throwing its own bounded-retry error after real progress was " + + "already made — the guard must apply AFTER progress, not only " + + "on an immediate first-call zero." + ); + } + // First two calls make real, partial progress (proving the guard does + // not just special-case "the very first call returned 0" — it must + // keep counting consecutive zeros correctly even after offset has + // already advanced past 0). + if (progressCalls < 2) { + progressCalls += 1; + var n = Math.min(2, length); + return n; + } + // From here on, every call returns 0 — the non-advancing run the + // guard must catch. + return 0; + }; + + var input = Buffer.from("hello world", "utf8"); // 11 bytes; 2+2 real progress, then stuck + + try { + assert.throws( + function () { + checkTestCount.writeFullySync(1, input); + }, + /writeFullySync: fs\.writeSync returned 0/, + "the guard must fire (and its own error message must be observable) after real progress was already made, not only on an immediate first-call zero" + ); + } finally { + fs.writeSync = originalWriteSync; + } + + assert.ok( + callCount <= STUB_CALL_BOUND, + "the mixed sequence must still terminate well before " + STUB_CALL_BOUND + " calls; got " + callCount + ); + assert.equal(progressCalls, 2, "the stub must have been allowed to make its two real partial-progress calls before the zero run began"); + } +);