fix(scripts): make check-test-count.js failure reasons legible (lr-e551b9)

[clagentic-builder]

What

Makes scripts/check-test-count.js failure reasons legible. It has four distinct exit-1 paths that previously fired with zero individual test failures, and the worst one -- the signal-death path (spawnSync result.status === null when the spawned node --test orchestrator 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.

The post-spawn decision logic is extracted into a pure, exported classifyRun() function that preserves every existing FAIL condition and its exact ordering: spawn-error -> missing-files -> below-floor -> (new) signal-death -> ordinary test-failure passthrough -> ok. No exit-1 condition is softened, removed, or reordered relative to a stricter check -- this is a legibility change only, per lr-795882 fail-open constraint (engram 7613980: a guard that looks like protection without providing it is worse than none).

Also adds emitAnnotation(), which writes a GitHub Actions ::error:: workflow-command line for any FAIL verdict. That turns the wrapper 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, refused by loadout-git-host-api 302-to-blob-store refusal by design).

Why

Surfaced by MILLER re-diagnosing PR #400 red test check (lr-4e1242 comment seq 5): four agents (NAOMI, AMoS, HOLDEN, MILLER) could not attribute the failure because the check-run annotation carried only Process completed with exit code 1, no test name, and no wrapper diagnostic -- because none of check-test-count.js own FAIL messages ever reach anywhere CI-reachable, and the signal-death path had no message to begin with.

Step 1 -- CI re-run diagnostic (requested alongside this task)

No write-capable path to trigger a re-run of PR #400 failed job (run 32305669339, job 96237771055) was available. loadout-git-host-api is Forgejo-only (confirmed via its own --help: no GitHub Actions endpoints, GET-focused, no re-run verb). loadout-push is push/PR-create/update only, no Actions API surface. gh is not on AMoS Bash allowlist (guard-bash denied it outright). Per the task explicit instruction, no empty commit was pushed as a workaround. PR #400 remains blocked pending an operator-triggered re-run or manual log read; this PR does not touch it.

Empirical verification during this task

Confirmed the pre-fix defect is real, not just a plausible reading of the code: stashed the fix, ran npm test against the unmodified script -- the FAIL(below-floor)/FAIL(missing-files) messages already existed in the old code and do fire, but the signal-death branch (process.exit(result.status === null ? 1 : result.status)) genuinely emitted nothing.

Also discovered, empirically, something MILLER diagnosis did not have room to establish: killing a test FROM WITHIN a node --test file (process.kill(process.pid, SIGKILL)) does NOT reproduce the silent signal-death path, because Node own test runner isolates each file into its own subprocess and already reports a killed file as an ordinary test:fail event carrying a signal field -- visible proof: not ok 1 ... signal: SIGKILL ... code: ERR_TEST_FAILURE. The true silent path is one layer up: the top-level node --test orchestrator process itself (the one spawnSync in check-test-count.js directly manages) dying by signal, which a test running inside that same process tree cannot reproduce by killing itself. This is why the regression tests exercise classifyRun() directly with a hand-built result object rather than attempting an end-to-end signal-kill through the real suite -- documented explicitly in the test file header, per this task instruction to say so plainly if a test is not achievable end-to-end.

Tests

Demonstrated-failure verified per lr-4e1242: git stash on scripts/check-test-count.js only, re-ran the new test file through the (then-unmodified) script. Every test in the file failed outright, because the unmodified script exports no classifyRun/emitAnnotation surface at all (require.main === module ran the old top-level body instead) -- require(...).classifyRun is not a function, surfaced as the whole test file crashing. That is the demonstrated failure for the file: there was no unit-testable decision surface before this change, which is itself part of the defect.

npm test: 1439/1439 pass (was 1429 pre-existing + 10 new), 0 fail, exit 0.

Task: lr-e551b9

[clagentic-reviewer]

PEACHES — blocking

scripts/check-test-count.js:137–151 — amos.code-craft.2 (amos.path-choice) — below-floor branch checked before signal-death, masking signal-death verdicts in mixed-condition edge cases.

When node --test orchestrator is killed by SIGKILL/SIGTERM AND every argv file emitted at least one RESULT but total test count < 1300, classifyRun() returns kind: "below-floor" instead of kind: "signal-death". The signal-death branch at line 151 is unreachable in this case because below-floor (line 137) was checked first and returned. This defeats lr-e551b9's main goal: making signal-death causes legible in CI.

The hard constraint (user prompt): verify every exit-1 condition preserves exactly — no subtle inversion or reordered branch. This is a subtle inversion: the exit code still fails (correct), but the reason is masked by a different condition (incorrect). Codex probe confirms: classifyRun({status: null, signal: "SIGKILL"}, files, {all-files-present}, totalTests: 2, floor: 1300) returns kind: "below-floor" not signal-death.

The test file does not cover this mixed case; test passes but does not exercise the ordering bug.

Fix: move result.status === null branch before totalTests < floor branch (keeping missing-files first, as task mentions missing-files should win when both conditions fire together).

{"reviewer": "peaches", "review_status": "blocking", "head_sha": "9f8b3d6c8bb17a102b73e8daeb425dc66d7e9157", "pr_number": 401}

[clagentic-reviewer]

PEACHES — clean (0 nits)

Re-review of HEAD 2b0967a after response to prior blocking verdict. The reorder is correct and complete.

Precedence Order Verification:
The classifyRun() function now checks conditions in the documented order: spawn-error > missing-files > signal-death > below-floor > test-failure > ok. Verified by line inspection (signal-death check at line 143 precedes below-floor at line 177) and all tests pass.

No Regression to Passing:
All FAIL branches still return { ok: false, exitCode: 1 }: spawn-error (line 115–121), missing-files (line 129–140), signal-death (line 143–174), below-floor (line 177–188), test-failure (line 191–200). Confirmed lr-795882 fail-open constraint is maintained.

Test Quality:
All 12 tests pass (test 9 specifically verifies the signal-death/below-floor ordering). Tests 8 and 10 guard against other precedence inversions by checking missing-files precedence is preserved even when reordering signal-death ahead of below-floor. Strong-form demonstrated-failure verification confirmed: test 9 was run against the mis-ordered implementation before the reorder landed and failed with the exact wrong-verdict assertion (expected: 'signal-death', actual: 'below-floor'), then passed after the reorder. Tests would catch re-introduction of the bug.

Brand Strings:
No violations detected. No bare "clagentic" appears as a product name; only "[check-test-count]" prefix is used in user-visible stderr output (correct).

Annotation Legibility:
The ::error:: line is actionable. Example: "::error::[check-test-count] FAIL (signal-death): node --test was killed by SIGKILL — likely OOM or an external kill (e.g. CI job timeout/cancellation)..." Names the condition, signal, and provides context without requiring the raw job log.

No findings.

{"reviewer": "peaches", "review_status": "clean", "head_sha": "2b0967a1d14986fcff5944da281fed10bfc081eb", "pr_number": 401}

[clagentic-security]

BOBBIE audited PR #401 (scripts/check-test-count.js legibility fix, lr-e551b9) against base..head (998cdef..2b0967a), scope confirmed to the two files named in the assignment.

Findings: none.

Verification performed beyond accepting the prior PEACHES clean verdict:

1. Merge-gate weakening check (governing constraint lr-795882, engram 7613980): traced every exit-1 path in classifyRun() against the pre-refactor code. All five verdict kinds (spawn-error, missing-files, signal-death, below-floor, test-failure) map to the identical exitCode the original code produced for that condition. The only behavioral change is that signal-death, previously silent (process.exit(result.status === null ? 1 : result.status) wrote nothing), now emits a named reason. No condition that failed the run before can pass it now. The signal-death-before-below-floor precedence fix at HEAD (2b0967a) changes only which reason is reported when both conditions are live, never whether the run fails.

2. Workflow-command injection: independently re-derived rather than accepting the escaping claim. emitAnnotation() applies percent/CR/LF escaping to the fully-composed message string as the last step before the single stdout write of the ::error:: line -- the escaping wraps the whole payload, not just the interpolated pieces, so no embedded control character in any interpolated value (signal name, file path, counts, spawn error message) can break the line or inject a second workflow command. Percent is escaped before CR/LF are converted, so the percent characters introduced by that conversion are not themselves re-escaped -- no double-escaping bug, no under-escaping gap.

3. Interpolated-value provenance: result.signal is a Node-runtime enum, not attacker text. files (missingFiles, path.resolve) is process.argv.slice(2), sourced from package.jsons fixed test script glob -- repo-controlled filenames, not external runtime input. spawnSync still receives an argv array, never a shell string -- no new argument-injection surface.

4. semgrep (auto + p/javascript + p/security-audit) flagged 2 LOW-confidence path-traversal warnings on path.resolve(f) inside the missing-files check -- this call is unchanged verbatim from the pre-PR code and f is a repo-controlled filename. Not a new exposure, not cited as a finding.

5. gitleaks (commit-range scoped) and trufflehog (base..head range) both report zero secrets. No dependency changes in this diff; osv-scanner run for completeness, not diff-relevant.

scanners_run: gitleaks (clean), trufflehog (clean), semgrep auto+p/javascript+p/security-audit (2 low-confidence non-findings, reasoned through), osv-scanner (no dep changes, informational).

{"reviewer": "bobbie", "review_status": "clean", "head_sha": "2b0967a1d14986fcff5944da281fed10bfc081eb", "pr_number": 401}

[clagentic-merger]

Merged via clagentic-loadout v0.2.0

Field	Value
Gated HEAD SHA	2b0967a1d14986fcff5944da281fed10bfc081eb
Merged SHA	2b0967a1d14986fcff5944da281fed10bfc081eb
Reviews	clagentic-reviewer[bot], clagentic-security[bot]
CI status	no-runner-by-design (0 commit-status entries at HEAD)
task_id	lr-e551b9