fix(scripts): flush check-run annotation via writeSync before exit (lr-e551b9) - #402
Conversation
…r-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 d7dd88a that no ::error:: annotation reached the check-run despite the run going red. TASK: lr-e551b9
…(lr-e551b9) 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 cf4b734 (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
…lr-e551b9) 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
|
PEACHES — clean (0 blocking findings) The PR successfully fixes the delivery hole identified by MILLER's reopen of lr-e551b9. The prior fix (PR #401) correctly identified which conditions cause the run to fail, but the annotation delivery itself was broken — the annotation was lost to an unflushed stdout pipe race. This PR fixes the delivery via three coordinated changes:
Delivered-failure verification (strong form): The new delivery test (test/check-test-count-delivery-lr-e551b9.test.js) was run against pre-fix code and failed with the assertion: the ::error:: annotation must be present in the child's captured stdout... actual: false, expected: true. This is a genuine data-loss failure, not a missing-symbol error. The captured output measured exactly 65536 bytes (one 64KB kernel pipe boundary), confirming MILLER's probe finding. After the fix, the same test passes. Test quality assessment: The delivery test spawns the real wrapper as a subprocess with real OS pipes (stdio: [ignore, pipe, pipe]), matching CI exactly. No stubs, no mocks. It does not force flush/drain on the parent side. The assertion measures actual bytes received, not a stubbed return value. This is genuine e2e. Existing test retarget: Two tests that verify emitAnnotation's FORMAT (the ::error:: prefix and GitHub workflow-command escaping) were updated to spy on fs.writeSync instead of stubbing process.stdout.write. The spy still captures the exact string passed to fd 1, so FORMAT coverage is preserved. Fail-open constraint: All five FAIL conditions (spawn-error, missing-files, signal-death, below-floor, test-failure) still return exitCode: 1. The final line in the main block (process.exitCode = verdict.exitCode) is unconditional and terminal. Traced all paths: no exception handling, no bypass. The constraint holds. Brand strings: No bare clagentic in user-facing output. The annotation prefix is [check-test-count], which is correct. Conventional commit: fix(scripts): flush check-run annotation via writeSync before exit (lr-e551b9) follows conventional-commit format correctly. |
|
BOBBIE — clean (0 blocking, 1 nit) MERGE-GATE INTEGRITY (highest-value check, verified independently, not inherited from PEACHES): process.exitCode = verdict.exitCode at scripts/check-test-count.js:317 is the textually final statement in the require.main block. verdict.exitCode is produced entirely inside classifyRun() (0 only on the terminal ok branch; 1 or result.status, itself non-zero, on every other branch). No process.exit() call, no signal handler, no exitCode reset exists anywhere else in this file, and no third-party module load in its dependency chain (fs/path/child_process are core builtins) can touch it. The two fs.writeSync calls that run before this assignment (annotation at line 113, TAP dump at line 305) can throw on EPIPE/EAGAIN, but an uncaught throw still exits non-zero (Node uncaught-exception default), never 0 — no path from a computed FAIL verdict to exit code 0 exists in this diff. INJECTION SURFACE: emitAnnotation escaping (percent, CR, LF -> percent25/percent0D/percent0A) at check-test-count.js:109-112 is byte-for-byte unchanged from the version previously audited clean on PR 401; only the sink changed (process.stdout.write -> fs.writeSync(1, ...)). No regression. FIXTURE/GLOB: package.json test script is node scripts/check-test-count.js test/*.test.js (non-recursive shell glob). test/fixtures/check-test-count-padding-lr-e551b9.fixture.js is excluded on two independent grounds — it lives in a subdirectory the glob does not descend into, and its filename does not end in .test.js. It cannot be picked up by the real suite invocation, cannot inflate totalTests against TEST_COUNT_FLOOR (1300), and is never in the per-file files list checked for missing-files. It is only exercised by test/check-test-count-delivery-lr-e551b9.test.js own explicit child-process spawn. No suite pollution. NIT (bobbie.uncat.1): check-test-count.js:305 if (result.stdout) fs.writeSync(1, result.stdout); is a single, unlooped writeSync call on a buffer this PR own comments describe as multi-MB, with the return value (actual bytes written) discarded. fs.writeSync performs exactly one write(2) attempt and does not loop to guarantee full delivery; a pipe fd can legitimately short-write under backpressure on a buffer this size. This does not defeat the merge gate — the ::error:: annotation for FAIL paths is small, written separately, and first, so verdict/exit-code integrity is unaffected — but it reintroduces a milder version of the exact silent-truncation failure mode this task exists to eliminate, this time against the raw TAP evidence trail rather than the verdict. The delivery test duration_ms footer assertion gives empirical, not structural, coverage (it would catch this specific fixture size truncating in this CI shape, but nothing loops fs.writeSync to guarantee it holds as the suite grows or under slower-reader backpressure). Justification for bobbie.uncat.1: no bobbie.sast.* rule covers an unlooped-write data-integrity gap on a large buffer to a pipe fd; this is a real, citable, non-gate-defeating exposure of the exact evidence-loss class lr-e551b9/lr-795882 exist to prevent. Recommend wrapping both writeSync calls in a until-fully-written loop. SCANNERS: gitleaks detect (range cf4b734..4bf735f) — no leaks found. semgrep --config auto (scripts/check-test-count.js) — 1 WARNING (path-join-resolve-traversal, line 149, LOW confidence) on pre-existing path.resolve(f) code unchanged by this diff (f derives from process.argv, a CI-controlled test-file glob, not attacker input) — false positive, dropped. osv-scanner — skipped, no dependency manifest changed in this diff. scanners_run: gitleaks(clean) semgrep(1 FP reviewed/dropped) osv-scanner(skipped, no dep changes) |
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
…teSync return (lr-e551b9) 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
|
PEACHES — clean Re-review of PR #402 at new head 0e54e61. KEY VERIFICATIONS (from dispatch lr-e551b9):
All dispatch verification points pass. Changes are minimal, focused, tested. No hardcoded paths, no new dependencies, no SDK direct calls. |
|
Re-audit of PR #402 at head 0e54e61 (prior verdict was bound to 4bf735f and is superseded). MERGE-GATE INTEGRITY (verified independently, not inherited from PEACHES). No process.on(uncaughtException) handler and no process.exit(0)/exitCode-reset path exist anywhere in scripts/check-test-count.js. The final statement is the unconditional process.exitCode = verdict.exitCode (line 396), reached only after both writeFullySync call sites (emitAnnotation at line 371, TAP dump at line 384). If writeFullySync throws via MAX_CONSECUTIVE_ZERO_WRITES, that throw fires strictly before process.exitCode is ever assigned; Node uncaught-exception handling then terminates the process with a non-zero exit code by default, unmodified by this script. A throw here can only convert an already-determined FAIL into a hard crash (still non-zero) - never a false green. lr-795882 / engram 7613980 constraint holds. TERMINATION. Traced writeFullySync directly: any fs.writeSync return greater than 0 both resets consecutiveZeroWrites to 0 and advances offset, same branch, confirming PEACHES claim. A 0 return only increments the zero-counter without touching offset. Alternating 1-byte/0-byte sequences reset the counter on every real write and make monotonic progress, bounded at 2x buffer.length iterations. A run of zeros after partial progress is capped at exactly 100 non-advancing attempts before throwing, independent of prior progress. Every path terminates. PARTIAL-ANNOTATION EXPOSURE (new this round). emitAnnotation passes the whole error line as one writeFullySync call; if it throws after exhausting the zero-write bound, a prefix of the annotation may already be on fd 1 without its trailing newline. Assessed for injection: nothing else writes to stdout after a thrown writeFullySync in this script, and the uncaught-exception output goes to stderr not stdout, so there is no subsequent content to splice into the unterminated line. The stream ends mid-line, treated as inert by the Actions log parser. Real but narrow, gated behind an already-degenerate 100-consecutive-zero-write condition. nit, not blocking. INJECTION SURFACE. The percent/CR/LF escaping in emitAnnotation is byte-for-byte unchanged from the pre-PR version; only the write call changed from process.stdout.write to writeFullySync(1, ...). TEST GLOB / FIXTURE / FLOOR. package.json test script remains test/*.test.js (non-recursive). The three new test files (delivery, short-write, zero-write) live directly under test/ and are correctly collected. test/fixtures/check-test-count-padding-lr-e551b9.fixture.js sits in a subdirectory with a .fixture.js suffix - matches neither the glob path depth nor its filename suffix, confirmed non-collected; its 2000 padding tests run only when spawned as a child by the delivery test, never polluting TEST_COUNT_FLOOR accounting for the real suite. SCANNERS. gitleaks: 6 hits, all outside this PR diff (pre-existing agent-worktree cert fixtures, a pre-existing WebSocket-nonce test string). semgrep: 2 LOW-confidence path-traversal warnings on path.resolve/path.join in check-test-count.js line 221 (pre-existing, unchanged by this diff) and a signal-legibility test file - the argument iterates a CI-supplied fixed test-file list from packages own glob, not attacker-controlled input; not a real exposure. osv-scanner: pre-existing dependency findings, no package.json/package-lock.json change in this PR diff - out of scope. No blocking findings. Every claim verified directly against the diff and checked-out HEAD, not inherited from PEACHES re-review. |
|
Merged via clagentic-loadout v0.2.0
|
What changed
check-test-count.js (the npm test entrypoint) surfaces its FAIL reason as a GitHub Actions ::error:: annotation, but that annotation never reached the check-run in CI. This PR fixes delivery. It does not change branch logic or verdict computation (classifyRun), which PR #401 shipped correctly and which stay as-is.
Why
lr-e551b9 was closed on PR #401, then reopened by MILLER (confidence 0.93, reproduced locally and in CI) and confirmed by HOLDEN empirically on PR #400 rebased head d7dd88a: the run went red and only the two generic GitHub-native annotations were present, zero ::error:: lines.
Root cause: check-test-count.js wrote the annotation with process.stdout.write() and then called process.exit(). process.stdout.write is asynchronous when stdout is a pipe (always true in CI). The ~1400-test TAP dump this script also writes to stdout is multi-MB, vastly exceeding the 64KB kernel pipe buffer, so the tail of that dump — and the annotation queued behind it — sat in a userspace buffer that process.exit() tore down without draining. The annotation reached the Writable stream object and never reached the file descriptor.
Why PR #401s tests did not catch it: test/check-test-count-signal-legibility-lr-e551b9.test.js stubbed process.stdout.write with a function that pushes to an array and returns true. Against that stub the annotation always arrives — the test verifies message FORMAT and is structurally incapable of observing flush, backpressure, or process.exit() interaction. A write is not a delivery.
Fix
Fail-open constraint (lr-795882, engram 7613980)
Every exit-1 condition in classifyRun() is unchanged; only delivery of the reason changed. Verified by re-running the full suite (1443 tests, all pass, exit 0) and by the through-a-real-pipe delivery test asserting exitCode 1 is preserved on the missing-files path.
Through-a-real-pipe verification (the acceptance bar per this tasks brief)
Added test/check-test-count-delivery-lr-e551b9.test.js: spawns the real scripts/check-test-count.js as a CHILD PROCESS with stdio: ["ignore", "pipe", "pipe"] (a genuine OS pipe, matching CI), against test/fixtures/check-test-count-padding-lr-e551b9.fixture.js (~2000 trivial tests, pushing TAP output to ~860KB, comfortably past the 64KB pipe buffer), plus a deliberately nonexistent second file to force a real missing-files FAIL path. Asserts the ::error:: annotation is present in the childs actual captured stdout.
Demonstrated-failure verification, strong form: this exact test file was run (npx node --test) against the pre-fix script restored from commit cf4b734 into the working tree, before the delivery fix existed. It failed with a genuine wrong-content assertion, not a missing-symbol error:
AssertionError: the ::error:: annotation must be present in the childs captured stdout — the wrapper must not lose it to an unflushed pipe
actual: false / expected: true
After restoring the fix, the same test passes. The existing classifyRun() precedence/verdict tests (test/check-test-count-signal-legibility-lr-e551b9.test.js) are kept — two of them needed retargeting from a process.stdout.write stub to an fs.writeSync spy, since emitAnnotation no longer calls process.stdout.write at all; same assertions, same message-format coverage, just tracking the real implementation.
Not chased in this PR
HOLDEN/MILLER flagged that the pre-fix instrumentation destroyed the name of whatever node --test failure produced local verdict test-failure. Post-fix, the full suite (npm test, 1443 tests) now runs clean on this branch — no test-failure surfaced. This is expected: fixing delivery lets any future red run be attributed by its own annotation instead of an opaque exit code. Reported here per the tasks instruction, not chased further.
Tests
npm test: 1443/1443 pass, exit 0.
TASK: lr-e551b9