From 24f81017eb21d0652a7d4c152c4dae9ecd4b791f Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 15:04:59 -0400 Subject: [PATCH 1/3] fix(scripts): make check-test-count.js failure reasons legible (lr-e551b9) Every exit-1 path in check-test-count.js fired with zero individual test failures and, worst of all, the signal-death path (spawnSync result.status === null when node --test is killed by a signal, e.g. OOM) wrote nothing at all before exiting 1 -- indistinguishable from a real test failure at the exit-code level. This is what blocked four agents (NAOMI, AMoS, HOLDEN, MILLER) from attributing PR #400's red check (lr-4e1242 comment seq 5). Extracts the post-spawn decision logic into a pure classifyRun() function (exported for unit testing) that preserves every existing FAIL condition and its exact ordering unchanged -- spawn-error, missing-files, below-floor, then the previously-silent signal-death branch, then an ordinary test-failure passthrough. No exit-1 condition is softened or removed; this only makes the reason legible, per lr-795882's fail-open constraint. Also adds emitAnnotation(), which writes a GitHub Actions ::error:: workflow-command line for any FAIL verdict. This turns the reason into a check-run annotation reachable via the GitHub API, unlike raw job-log text which is structurally unreachable to the crew (crew-manifest lr-90a3e1). --- scripts/check-test-count.js | 241 ++++++++++++++++++++++++++---------- 1 file changed, 176 insertions(+), 65 deletions(-) diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js index 18e24273..e1bbc03a 100644 --- a/scripts/check-test-count.js +++ b/scripts/check-test-count.js @@ -64,78 +64,189 @@ var TEST_COUNT_FLOOR = 1300; var path = require("path"); var { spawnSync } = require("child_process"); -var files = process.argv.slice(2); -if (files.length === 0) { - process.stderr.write("[check-test-count] no test files given (expected: node scripts/check-test-count.js )\n"); - process.exit(1); +// --------------------------------------------------------------------------- +// CI legibility (lr-e551b9, MILLER re-diagnosis of PR #400 / lr-4e1242 seq 5). +// +// 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 +// blob store that loadout-git-host-api correctly refuses (crew-manifest +// lr-90a3e1). A `::error::`-prefixed line, by contrast, is turned into a +// check-run ANNOTATION by the Actions runner, which IS reachable via the +// GitHub API. emitAnnotation() below exists so every FAIL path +// in this script — not just the final exit code — lands somewhere a crew +// 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). +// --------------------------------------------------------------------------- +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::` — + // https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions + var escaped = String(message) + .replace(/%/g, "%25") + .replace(/\r/g, "%0D") + .replace(/\n/g, "%0A"); + process.stdout.write("::error::" + escaped + "\n"); } -var REPORTER_PATH = path.join(__dirname, "test-file-completion-reporter.js"); +// classifyRun() is the pure decision core of this script: given the raw +// spawnSync() result plus the bucketed RESULT-line data, decide whether the +// run passes and, if not, exactly WHY. Pulled out of the top-level script +// body (which is otherwise unavoidably I/O-coupled — it shells out to a +// real `node --test`) specifically so the signal-death path (the highest- +// value item in lr-e551b9) is exercisable by a fast, deterministic unit +// test against a hand-built `result` object, without spawning a real child +// process or running the real suite. +// +// Returns { ok: bool, exitCode: number, reason: string|null, kind: string }. +// `kind` names which of this script's distinct FAIL routes produced the +// verdict (see the module header's WHAT THIS DOES AND DOES NOT CATCH / +// MECHANISM sections for the full list) — 'ok', 'spawn-error', +// 'missing-files', 'below-floor', 'signal-death', or 'test-failure' (a +// genuine node --test non-zero exit with no wrapper-level condition +// tripped — i.e. an ordinary named test failure, not a wrapper failure). +function classifyRun(result, files, resultsByFile, totalTests, floor) { + if (result.error) { + return { + ok: false, + exitCode: 1, + kind: "spawn-error", + reason: "failed to spawn node --test: " + result.error.message, + }; + } -var result = spawnSync(process.execPath, [ - "--test", - "--test-reporter=tap", "--test-reporter-destination=stdout", - "--test-reporter=" + REPORTER_PATH, "--test-reporter-destination=stderr", -].concat(files), { - stdio: ["inherit", "pipe", "pipe"], - encoding: "utf8", - maxBuffer: 64 * 1024 * 1024, -}); + var missingFiles = files.filter(function (f) { + var abs = path.resolve(f); + return !resultsByFile[abs]; + }); -if (result.stdout) process.stdout.write(result.stdout); + if (missingFiles.length > 0) { + return { + ok: false, + exitCode: 1, + kind: "missing-files", + reason: ( + missingFiles.length + " test file(s) reported ZERO test results — " + + "treated as a truncated/incomplete run, not a pass, regardless of the overall exit code. This is " + + "exactly the failure class lr-795882 fixed (MILLER, lr-a7b03e): a file whose tests silently vanish " + + "while the overall run still exits 0.\n " + missingFiles.join("\n ") + ), + }; + } -if (result.error) { - process.stderr.write("[check-test-count] failed to spawn node --test: " + result.error.message + "\n"); - process.exit(1); -} + if (totalTests < floor) { + return { + ok: false, + exitCode: 1, + kind: "below-floor", + reason: ( + "total executed test count " + totalTests + + " is below the floor of " + floor + " even though every file reported at least one result — " + + "likely a large in-file test drop. See this script's header for what the floor does and does not " + + "catch, and how to update it for a deliberate suite reduction." + ), + }; + } -// 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 -// RESULT lines separately below. -var stderrLines = (result.stderr || "").split("\n"); -var resultsByFile = Object.create(null); -var passCount = 0; -var failCount = 0; - -stderrLines.forEach(function (line) { - var match = /^RESULT (pass|fail) (.+)$/.exec(line); - if (!match) { - if (line) process.stderr.write(line + "\n"); - return; + if (result.status === null) { + // The child `node --test` ORCHESTRATOR process itself (not an + // individual per-file worker/subprocess it manages internally — Node's + // own test runner already reports THOSE as ordinary test:fail events + // with a `signal` field, which is why they show up as RESULT lines + // above and never reach this branch) was killed by a signal rather than + // exiting normally. status is null in exactly this case (Node's + // child_process docs: exactly one of status/signal is non-null). + // Previously this exited 1 with ZERO explanatory output — indistinguishable + // from a genuine test failure at the exit-code level. lr-e551b9. + return { + ok: false, + exitCode: 1, + kind: "signal-death", + reason: ( + "node --test was killed by " + result.signal + " — likely OOM or an external kill (e.g. CI job " + + "timeout/cancellation); no test failure was reported because the process did not exit normally. " + + "This is NOT the same as a per-file worker crash (Node's test runner already reports those as a " + + "named test:fail with a signal field); this is the top-level orchestrator process itself dying." + ), + }; + } + + if (result.status !== 0) { + return { + ok: false, + exitCode: result.status, + kind: "test-failure", + reason: ( + "node --test exited " + result.status + " with a named test failure above (see the TAP `not ok` " + + "line(s)) — this is an ordinary test failure, not a wrapper-level condition." + ), + }; } - var kind = match[1]; - var file = match[2]; - if (!resultsByFile[file]) resultsByFile[file] = 0; - resultsByFile[file] += 1; - if (kind === "pass") passCount += 1; - else failCount += 1; -}); - -var missingFiles = files.filter(function (f) { - var abs = path.resolve(f); - return !resultsByFile[abs]; -}); - -if (missingFiles.length > 0) { - process.stderr.write( - "[check-test-count] FAIL: " + missingFiles.length + " test file(s) reported ZERO test results — " + - "treated as a truncated/incomplete run, not a pass, regardless of the overall exit code. This is " + - "exactly the failure class lr-795882 fixed (MILLER, lr-a7b03e): a file whose tests silently vanish " + - "while the overall run still exits 0.\n " + missingFiles.join("\n ") + "\n" - ); - process.exit(1); -} -var totalTests = passCount + failCount; -if (totalTests < TEST_COUNT_FLOOR) { - process.stderr.write( - "[check-test-count] FAIL: total executed test count " + totalTests + - " is below the floor of " + TEST_COUNT_FLOOR + " even though every file reported at least one result — " + - "likely a large in-file test drop. See this script's header for what the floor does and does not " + - "catch, and how to update it for a deliberate suite reduction.\n" - ); - process.exit(1); + return { ok: true, exitCode: 0, kind: "ok", reason: null }; } -process.exit(result.status === null ? 1 : result.status); +module.exports = { classifyRun: classifyRun, emitAnnotation: emitAnnotation }; + +// Everything below only runs when this file is executed directly (`node +// scripts/check-test-count.js `), not when required as a module by +// a unit test. +if (require.main === module) { + var files = process.argv.slice(2); + if (files.length === 0) { + process.stderr.write("[check-test-count] no test files given (expected: node scripts/check-test-count.js )\n"); + process.exit(1); + } + + var REPORTER_PATH = path.join(__dirname, "test-file-completion-reporter.js"); + + var result = spawnSync(process.execPath, [ + "--test", + "--test-reporter=tap", "--test-reporter-destination=stdout", + "--test-reporter=" + REPORTER_PATH, "--test-reporter-destination=stderr", + ].concat(files), { + stdio: ["inherit", "pipe", "pipe"], + encoding: "utf8", + 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 + // RESULT lines separately below. + var stderrLines = (result.stderr || "").split("\n"); + var resultsByFile = Object.create(null); + var passCount = 0; + var failCount = 0; + + stderrLines.forEach(function (line) { + var match = /^RESULT (pass|fail) (.+)$/.exec(line); + if (!match) { + if (line) process.stderr.write(line + "\n"); + return; + } + var kind = match[1]; + var file = match[2]; + if (!resultsByFile[file]) resultsByFile[file] = 0; + resultsByFile[file] += 1; + if (kind === "pass") passCount += 1; + else failCount += 1; + }); + + var totalTests = passCount + failCount; + var verdict = classifyRun(result, files, resultsByFile, totalTests, TEST_COUNT_FLOOR); + + if (!verdict.ok) { + var line = "[check-test-count] FAIL (" + verdict.kind + "): " + verdict.reason; + process.stderr.write(line + "\n"); + // Surfaced as a check-run annotation too (see emitAnnotation's own + // comment) — this is what makes the reason reachable without the raw + // job log, for every FAIL route including the previously-silent + // signal-death path. + emitAnnotation(line); + } + + process.exit(verdict.exitCode); +} From 9f8b3d6c8bb17a102b73e8daeb425dc66d7e9157 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 15:05:10 -0400 Subject: [PATCH 2/3] test(scripts): regression coverage for check-test-count.js legibility (lr-e551b9) Demonstrated-failure verified per lr-4e1242: against the unmodified (pre-lr-e551b9) script, check-test-count.js exported no classifyRun/ emitAnnotation surface at all, so every test in this file failed outright (require.main === module ran the old top-level body instead, producing 'test failed' with no assertions ever reached). Verified via git stash on scripts/check-test-count.js only, re-running this file through npm's own node --test. Covers: signal-death now reports the actual signal (SIGKILL and SIGTERM cases, not a hardcoded string), a genuine test failure is labeled distinctly from a wrapper failure, missing-files/below-floor/spawn-error stay exactly as strict as before, a clean run still passes, missing-files is checked before signal-death when both conditions co-occur (the realistic shape: a worker dies mid-file), and emitAnnotation's ::error:: framing plus its %/CR/LF escaping per GitHub's documented workflow-command encoding. The true top-level node --test orchestrator signal-death cannot be reproduced by a test running inside that same process tree without killing itself -- confirmed empirically during this task's investigation that a per-file worker crash is already caught and reported as an ordinary test:fail event by Node's own test runner, one layer below the boundary this fix covers. classifyRun() being pure and exported is what makes the actual boundary (the orchestrator's spawnSync result) testable at all. --- ...-count-signal-legibility-lr-e551b9.test.js | 197 ++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 test/check-test-count-signal-legibility-lr-e551b9.test.js diff --git a/test/check-test-count-signal-legibility-lr-e551b9.test.js b/test/check-test-count-signal-legibility-lr-e551b9.test.js new file mode 100644 index 00000000..fa222bce --- /dev/null +++ b/test/check-test-count-signal-legibility-lr-e551b9.test.js @@ -0,0 +1,197 @@ +"use strict"; +// Regression tests for lr-e551b9: scripts/check-test-count.js exits 1 +// without naming a cause on several distinct wrapper-level FAIL routes, +// worst of all the signal-death path (result.status === null when the +// spawned `node --test` orchestrator is killed by a signal, e.g. OOM) which +// previously wrote NOTHING before exiting 1 — indistinguishable from a real +// test failure. See lr-4e1242 comment seq 5 (MILLER) for the full diagnosis +// and lr-795882 for why every one of these FAIL conditions must keep +// failing the run (a guard that looks like protection without providing it +// is worse than none — engram 7613980). +// +// classifyRun()/emitAnnotation() are exported by check-test-count.js +// specifically so this file can drive the decision logic directly with a +// hand-built spawnSync()-shaped result object, rather than needing to +// actually spawn `node --test` and kill its process tree with a real +// signal (the file-level "kill a test from inside itself" experiment run +// during this task's investigation showed Node's OWN test runner already +// intercepts a per-file worker crash and reports it as a normal test:fail +// event with a `signal` field — the true signal-death path this task fixes +// is one layer up, the top-level `node --test` orchestrator process dying, +// which is not something a `node --test`-run unit test can reproduce from +// inside that same process tree without killing itself). +// +// DEMONSTRATED-FAILURE VERIFICATION (lr-4e1242 convention): every test below +// was run against the pre-fix script (git stash) before this file existed. +// Pre-fix, classifyRun did not exist at all — check-test-count.js had no +// exported surface, so EVERY test in this file fails outright on +// `require("../scripts/check-test-count.js").classifyRun is not a +// function` against unmodified main. That is the demonstrated failure for +// the whole file: there was no unit-testable decision surface to assert +// against before this change, which is itself the defect lr-e551b9 fixes +// (the logic was unreachable except by paying for a full real `node --test` +// spawn). Per-test behavioral detail below where the mapping is not obvious +// from the function name alone. + +var test = require("node:test"); +var assert = require("node:assert/strict"); +var path = require("path"); +var checkTestCount = require("../scripts/check-test-count.js"); + +var FLOOR = 1300; + +function absFile(name) { + return path.resolve(name); +} + +test("lr-e551b9: signal-death (result.status === null) is no longer silent — reports the signal and fails", function () { + var files = ["test/some-file.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/some-file.test.js")] = 5; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5 + FLOOR, FLOOR); + + assert.equal(verdict.ok, false, "a null-status (signal-killed) run must fail, not pass"); + assert.equal(verdict.exitCode, 1); + assert.equal(verdict.kind, "signal-death"); + assert.ok(verdict.reason, "the signal-death path must produce a non-empty reason (previously: nothing)"); + assert.ok(verdict.reason.indexOf("SIGKILL") !== -1, + "the reason must name the actual signal, not just say 'killed'; got: " + verdict.reason); +}); + +test("lr-e551b9: signal-death names whichever signal fired, not a hardcoded one", function () { + var files = ["test/some-file.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/some-file.test.js")] = 5; + var result = { status: null, signal: "SIGTERM", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5 + FLOOR, FLOOR); + + assert.equal(verdict.kind, "signal-death"); + assert.ok(verdict.reason.indexOf("SIGTERM") !== -1, "must report SIGTERM, not a different signal name"); +}); + +test("lr-e551b9: a genuine test failure (non-null, non-zero status) is distinguished from a wrapper failure", function () { + var files = ["test/some-file.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/some-file.test.js")] = 5; + var result = { status: 1, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5 + FLOOR, FLOOR); + + assert.equal(verdict.ok, false); + assert.equal(verdict.exitCode, 1); + assert.equal(verdict.kind, "test-failure", + "a plain node --test failure must be labeled distinctly from signal-death/missing-files/below-floor"); +}); + +test("lr-e551b9: missing-files path is preserved unchanged (still fails, still names the files)", function () { + var files = ["test/present.test.js", "test/absent.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 3; + var result = { status: 0, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 3, FLOOR); + + assert.equal(verdict.ok, false, "a file with zero RESULT lines must still fail the run"); + assert.equal(verdict.kind, "missing-files"); + assert.ok(verdict.reason.indexOf("test/absent.test.js") !== -1, + "must name the specific missing file; got: " + verdict.reason); + assert.ok(verdict.reason.indexOf("test/present.test.js") === -1, + "must NOT name the file that did report results"); +}); + +test("lr-e551b9: below-floor path is preserved unchanged (still fails when every file reported but total is low)", function () { + var files = ["test/a.test.js", "test/b.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/a.test.js")] = 1; + resultsByFile[absFile("test/b.test.js")] = 1; + var result = { status: 0, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 2, FLOOR); + + assert.equal(verdict.ok, false, "every file reporting at least one result must not mask a total below the floor"); + assert.equal(verdict.kind, "below-floor"); + assert.ok(verdict.reason.indexOf("2") !== -1 && verdict.reason.indexOf(String(FLOOR)) !== -1, + "must report both the actual total and the floor; got: " + verdict.reason); +}); + +test("lr-e551b9: spawn-error path is preserved unchanged (spawnSync itself failing to launch the child)", function () { + var files = ["test/a.test.js"]; + var resultsByFile = {}; + var result = { status: null, signal: null, error: new Error("ENOENT: node not found") }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 0, FLOOR); + + assert.equal(verdict.ok, false); + assert.equal(verdict.kind, "spawn-error"); + assert.ok(verdict.reason.indexOf("ENOENT") !== -1, "must surface the underlying spawn error message"); +}); + +test("lr-e551b9: a clean run (every file reported, floor met, status 0) still passes", function () { + var files = ["test/a.test.js", "test/b.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/a.test.js")] = FLOOR; + resultsByFile[absFile("test/b.test.js")] = 1; + var result = { status: 0, signal: null, error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, FLOOR + 1, FLOOR); + + assert.equal(verdict.ok, true, "a genuinely clean run must still pass — this fix must not introduce a new false FAIL"); + assert.equal(verdict.exitCode, 0); + assert.equal(verdict.kind, "ok"); + assert.equal(verdict.reason, null); +}); + +test("lr-e551b9: check ORDER — missing-files is checked before signal-death (a truncated run with both conditions reports the more specific cause)", function () { + // If node --test died by signal AND a file has zero results (the common + // real-world shape: a worker died mid-file, so that file never reported), + // missing-files should win — it names the specific file, which is more + // actionable than a bare signal name. + var files = ["test/present.test.js", "test/killed-mid-run.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 5; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 5, FLOOR); + + assert.equal(verdict.kind, "missing-files"); + 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 () { + var chunks = []; + var originalWrite = process.stdout.write; + process.stdout.write = function (chunk) { + chunks.push(chunk); + return true; + }; + try { + checkTestCount.emitAnnotation("[check-test-count] FAIL (signal-death): node --test was killed by SIGKILL"); + } finally { + process.stdout.write = originalWrite; + } + + var written = chunks.join(""); + assert.ok(written.indexOf("::error::") === 0, "annotation must start with the GitHub Actions ::error:: workflow command"); + assert.ok(written.indexOf("SIGKILL") !== -1, "the annotated message must carry the actual failure reason"); +}); + +test("lr-e551b9: emitAnnotation escapes %, CR and LF per GitHub's documented workflow-command encoding", function () { + var chunks = []; + var originalWrite = process.stdout.write; + process.stdout.write = function (chunk) { + chunks.push(chunk); + return true; + }; + try { + checkTestCount.emitAnnotation("100% failure\r\nline two"); + } finally { + process.stdout.write = originalWrite; + } + + var written = chunks.join(""); + assert.ok(written.indexOf("100%25 failure%0D%0Aline two") !== -1, + "%, CR, LF must be percent-escaped or the annotation body truncates/misparses; got: " + written); +}); From 2b0967a1d14986fcff5944da281fed10bfc081eb Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Thu, 20 Aug 2026 15:16:23 -0400 Subject: [PATCH 3/3] fix(scripts): check signal-death before below-floor in classifyRun (lr-e551b9) PEACHES BLOCKING finding on PR #401: below-floor was checked before signal-death, so an OOM-killed orchestrator whose files all reported at least one RESULT but with totalTests < TEST_COUNT_FLOOR reported kind 'below-floor' instead of 'signal-death'. An OOM-killed run IS a truncated run, so the two conditions fire together in the ordinary case this task exists to make legible, and the wrong cause was reported precisely when the cause matters most. Reordered: spawn-error > missing-files > signal-death > below-floor > test-failure > ok. missing-files still wins over signal-death (unchanged, per the existing regression test). Adds two mixed-condition tests: signal-death+below-floor together (the PEACHES probe), and missing-files+signal-death+below-floor three-way precedence. Both verified the strong (demonstrated-failure) way: run in isolation via node --test against the pre-fix implementation, the signal-death-vs-below-floor test failed with AssertionError expected 'signal-death' actual 'below-floor' (a wrong-verdict failure, not a missing-symbol error); after the reorder the same isolated run passes 12/12. Full suite: 1441/1441. TASK: lr-e551b9 --- scripts/check-test-count.js | 53 ++++++++++------ ...-count-signal-legibility-lr-e551b9.test.js | 62 +++++++++++++++++++ 2 files changed, 97 insertions(+), 18 deletions(-) diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js index e1bbc03a..b331c9db 100644 --- a/scripts/check-test-count.js +++ b/scripts/check-test-count.js @@ -101,10 +101,16 @@ function emitAnnotation(message) { // Returns { ok: bool, exitCode: number, reason: string|null, kind: string }. // `kind` names which of this script's distinct FAIL routes produced the // verdict (see the module header's WHAT THIS DOES AND DOES NOT CATCH / -// MECHANISM sections for the full list) — 'ok', 'spawn-error', -// 'missing-files', 'below-floor', 'signal-death', or 'test-failure' (a -// genuine node --test non-zero exit with no wrapper-level condition -// tripped — i.e. an ordinary named test failure, not a wrapper failure). +// MECHANISM sections for the full list). Checked in this precedence order +// when multiple conditions are live at once (lr-e551b9 fold-in, PR #401): +// 'spawn-error' > 'missing-files' > 'signal-death' > 'below-floor' > +// 'test-failure' (a genuine node --test non-zero exit with no wrapper-level +// condition tripped — i.e. an ordinary named test failure, not a wrapper +// failure) > 'ok'. missing-files outranks signal-death because it names a +// specific file, the most actionable cause available; signal-death outranks +// below-floor because a signal-killed run IS the truncated run that +// produces a below-floor count in the first place — see the check itself +// for the full reasoning. function classifyRun(result, files, resultsByFile, totalTests, floor) { if (result.error) { return { @@ -134,20 +140,6 @@ function classifyRun(result, files, resultsByFile, totalTests, floor) { }; } - if (totalTests < floor) { - return { - ok: false, - exitCode: 1, - kind: "below-floor", - reason: ( - "total executed test count " + totalTests + - " is below the floor of " + floor + " even though every file reported at least one result — " + - "likely a large in-file test drop. See this script's header for what the floor does and does not " + - "catch, and how to update it for a deliberate suite reduction." - ), - }; - } - if (result.status === null) { // The child `node --test` ORCHESTRATOR process itself (not an // individual per-file worker/subprocess it manages internally — Node's @@ -158,6 +150,17 @@ function classifyRun(result, files, resultsByFile, totalTests, floor) { // child_process docs: exactly one of status/signal is non-null). // Previously this exited 1 with ZERO explanatory output — indistinguishable // from a genuine test failure at the exit-code level. lr-e551b9. + // + // Checked BEFORE below-floor (PEACHES PR #401 finding, lr-e551b9 + // fold-in): an OOM-killed/externally-signaled orchestrator IS a + // truncated run, and a truncated run is exactly what produces a + // below-floor total — the two conditions fire TOGETHER in the ordinary + // case this whole check exists to make legible. Reporting below-floor + // here would name the wrong cause precisely when the cause matters + // most (the run still fails either way — exitCode stays 1 — but the + // diagnostic would mislabel it). missing-files is still checked above + // this because it names a more specific, actionable culprit (an actual + // file) when both conditions are live. return { ok: false, exitCode: 1, @@ -171,6 +174,20 @@ function classifyRun(result, files, resultsByFile, totalTests, floor) { }; } + if (totalTests < floor) { + return { + ok: false, + exitCode: 1, + kind: "below-floor", + reason: ( + "total executed test count " + totalTests + + " is below the floor of " + floor + " even though every file reported at least one result — " + + "likely a large in-file test drop. See this script's header for what the floor does and does not " + + "catch, and how to update it for a deliberate suite reduction." + ), + }; + } + if (result.status !== 0) { return { ok: false, 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 fa222bce..4a0bb436 100644 --- a/test/check-test-count-signal-legibility-lr-e551b9.test.js +++ b/test/check-test-count-signal-legibility-lr-e551b9.test.js @@ -32,6 +32,27 @@ // (the logic was unreachable except by paying for a full real `node --test` // spawn). Per-test behavioral detail below where the mapping is not obvious // from the function name alone. +// +// STRONG-FORM DEMONSTRATED FAILURE (PEACHES PR #401 fold-in): the "missing +// symbol" failure above proves classifyRun is new, not that its VERDICTS +// are correct — a test that would still pass against a WRONG verdict is not +// a behavioral guard. PEACHES found exactly that gap: the below-floor +// branch (~line 137, pre-fold-in) was checked before signal-death, so a +// signal-killed run with a below-floor total (the common real shape — an +// OOM-killed run IS a truncated run) reported kind "below-floor" instead of +// "signal-death". The "signal-death is checked before below-floor" test +// below was verified the strong way: run via `npx node --test +// test/check-test-count-signal-legibility-lr-e551b9.test.js` against the +// mis-ordered implementation BEFORE the reorder landed, it failed with +// `AssertionError [ERR_ASSERTION]: ... expected: 'signal-death', actual: +// 'below-floor'` (test 9 of 12, `code: 'ERR_ASSERTION'`) — a genuine +// wrong-verdict assertion failure, not a missing-symbol error. After +// reordering signal-death ahead of below-floor in classifyRun, the same +// isolated run passed 12/12. The "missing-files still wins ... full +// three-way precedence" test alongside it already passed pre-fix (missing- +// files was already checked first), which is the expected shape: that test +// pins a precedence relationship the reorder must NOT disturb, not one it +// fixes. var test = require("node:test"); var assert = require("node:assert/strict"); @@ -160,6 +181,47 @@ test("lr-e551b9: check ORDER — missing-files is checked before signal-death (a assert.ok(verdict.reason.indexOf("test/killed-mid-run.test.js") !== -1); }); +test("lr-e551b9: check ORDER — signal-death is checked before below-floor (PEACHES PR #401 finding: an OOM-killed run IS a truncated run, so these two conditions fire TOGETHER in the ordinary case this task exists to make legible, and the wrong cause must not win)", function () { + // All argv files reported at least one RESULT (so missing-files does NOT + // fire), the orchestrator was signal-killed, AND the total is below the + // floor — the common real shape for an OOM/CI-timeout kill mid-run: every + // file that got to run at least once emitted partial results before the + // process died, so nothing is individually "missing", but the aggregate + // total is far short of a real run. signal-death must win here: it is the + // more actionable, more specific cause (names the actual signal) and is + // the true cause of the truncation, not a coincidental byproduct of it. + var files = ["test/present.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 2; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 2, FLOOR); + + assert.equal(verdict.kind, "signal-death", + "a signal-killed run with a below-floor total must report signal-death, not below-floor — " + + "otherwise the diagnostic reports the wrong cause precisely when the cause matters most; got: " + verdict.kind); + assert.ok(verdict.reason.indexOf("SIGKILL") !== -1, + "must name the actual signal even when below-floor also applies; got: " + verdict.reason); +}); + +test("lr-e551b9: check ORDER — missing-files still wins over signal-death even when below-floor would also apply (full three-way precedence)", function () { + // Reaffirms the existing missing-files-before-signal-death precedence + // (line ~147 above) but with a total that is ALSO below the floor, so + // all three conditions are live at once. missing-files must still win — + // reordering signal-death ahead of below-floor must not disturb the + // higher-precedence missing-files check. + var files = ["test/present.test.js", "test/killed-mid-run.test.js"]; + var resultsByFile = {}; + resultsByFile[absFile("test/present.test.js")] = 2; + var result = { status: null, signal: "SIGKILL", error: undefined }; + + var verdict = checkTestCount.classifyRun(result, files, resultsByFile, 2, FLOOR); + + assert.equal(verdict.kind, "missing-files", + "missing-files must win over both signal-death and below-floor when all three are live; got: " + verdict.kind); + 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 () { var chunks = []; var originalWrite = process.stdout.write;