fix(test): stop --test-force-exit truncation, fix leaked handles at root cause (lr-795882) - #395
Conversation
… watchdog clears (lr-795882)
…Watchdog (lr-795882)
…sh socket (lr-795882)
…nt floor (lr-795882)
|
PEACHES — blocking (1 defect) test/app-boot-esm-graph-load-lr-4c58ae.test.js:223–234 — amos.code-craft.1 — Duplicate comment block. Lines 166–177 and 178–189 contain identical 12-line lr-795882 explanation (WebSocket onopen fix). Remove one copy. Other findings: clean. Production changes (lib/) are correct and safe. Guard mechanism (check-test-count.js) is robust. Test fixes properly scoped. 8 consecutive stable runs prove the fix eliminates original 1-in-4 truncation. |
|
BOBBIE clean. Audit scope: PR #395, base 0cf8d6b..head 2ff4fda, 7 commits / 8 files. This PRs real subject is the integrity of the merge gate itself (MILLER proved --test-force-exit silently dropped 21 XSS regression tests with exit 0); audited accordingly, not as routine test tooling. Findings (0 blocking, 3 nit): scripts/check-test-count.js:43 -- bobbie.uncat.1 -- count floor (1300) is well below true count (about 1407); self-disclosed as unable to catch the actual 21-test truncation that motivated this fix (1385 > 1300). scripts/check-test-count.js:67 -- bobbie.uncat.1 -- TAP-summary regex matches the FIRST hash-tests-N line anywhere in captured stdout; a test printing a matching line before Nodes real summary could spoof the floor check; no test in this diff does this but the parser has no defense against it. lib/project-sessions.js:28 -- bobbie.uncat.1 -- production code now branches on NODE_TEST_CONTEXT (an inheritable env var); if leaked into a prod launch, agentsModule.refresh() is silently skipped at boot. Traced the consequence: lib/agents.js real tool-ENFORCEMENT path readAgentToolsFromFile() reads agent frontmatter synchronously from disk per-request, independent of the refresh-populated cache -- worst case is a stale/empty agent picker list, not a tool-grant enforcement bypass; flagged as a pattern (test-runner env var branching prod code) rather than an active exploit. Verified clean: lib/smtp.js:194-203 OTP cleanup interval unref d, matches the precedented wsTicketSweepInterval pattern in lib/server-auth.js; verifyOtp() (smtp.js:138) performs its own lazy expiry check independent of the interval sweep, so an unfired interval cannot cause an expired OTP to be accepted. check-test-count.js spawns node --test via an argv array not a shell string, so no injection surface from its glob args (Q5). No process.exit or swallowed-rejection alternate truncation path found in the diff (Q3). Both modified test files and test/security.test.js diff hunks use os.tmpdir()/mkdtempSync only, no write to real operator state in this diff (Q4). gitleaks and trufflehog clean on base..head; semgrep 1 INFO hit is pre-existing code outside this diffs hunks; osv-scanner advisories are pre-existing transitive deps, package.json/package-lock.json dependency entries unchanged by this PR. scanners_run: gitleaks(ok,clean) trufflehog(ok,clean) semgrep(ok,1-info-out-of-diff) osv-scanner(ok,pre-existing-no-new-deps). |
|
PEACHES — clean (1 nit) Remediation confirmed: Duplicate 12-line comment block has been removed from test/app-boot-esm-graph-load-lr-4c58ae.test.js. Per-file completion check mechanism: The new design replaces the loose 1300 floor with a per-file boundary enforced via Node report API. Key assessment: a) Motivating failure caught: YES. Detects if any file produces zero test events — exact MILLER mode (lr-a7b03e). b) Stated limits accurate: BOTH CORRECT.
c) Node reporter-API file field durability: MODERATE RISK, MITIGATED. If future Node removes event.data.file, silent empty string fallback. Current design acceptable if Node API stability trusted; loud-fail would be more robust. Not blocking. d) Script factoring: CLEAN. check-test-count.js and test-file-completion-reporter.js have clear separation. e) Per-file isolation rejection + lr-2fcf59 flake: CORRECT. Isolation exposes unrelated millisecond-tie-break race. Verification should not introduce new risk. f) Production-code changes: CONFIRMED CLEAN. lib/project-sessions.js and lib/smtp.js unchanged from prior assessment. g) 5 consecutive runs @ 1407/1406/1/0: SUFFICIENT. Consistency confirmed; 107-test buffer healthy. h) Conventional Commits: VALID. nit.1 — peaches.nit.observation — Silent fallback on missing event.data.file improves durability with explicit loud-fail. |
|
Re-audit at new head c536019 (base 0cf8d6b). All three prior nits verified resolved. Prior nit 1 (floor too loose): replaced. check-test-count.js now attaches test-file-completion-reporter.js alongside tap and gates on a hard per-file boundary (every file on the glob must report >=1 test:pass/test:fail event via Node reporter API event.data.file), not just a total-count floor. The 1300 floor is retained only as a secondary net for large in-file drops -- documented as such in the script header. Prior nit 2 (regex spoofable by printed text): resolved by construction. Counts derive from the reporter event stream (RESULT <pass|fail> lines on a stream separate from TAP stdout), not from parsing human-readable text. Traced the one remaining regex in check-test-count.js (/^RESULT (pass|fail) (.+)$/ against stderr lines) -- this parses the tool-emitted stream, not test-authored stdout, so a test printing arbitrary text cannot forge a RESULT line onto stderr. Verified no other path lets printed text influence the verdict. Prior nit 3 (NODE_TEST_CONTEXT branch): confirmed. Read lib/agents.js in full -- readAgentToolsFromFile is a synchronous fs.readFileSync with zero reference to _cache/_refreshPromise. Read lib/project-sessions.js in full -- exactly 4 getAll() call sites (list_agents, its background-refresh callback, refresh_agents, toggle_agent_favorite), all send agents_list payloads to picker/listing UI, never an enforcement decision. Matches the PR body documentation exactly. Question (a) -- reporter as merge-gate control: traced the fail-open risk directly. If event.data.file is ever absent/renamed by a Node upgrade, the reporter emits RESULT pass (trailing space, empty file), which does NOT satisfy the check-test-count.js regexs required (.+) capture group -- the line falls through unparsed. resultsByFile then stays empty for every file, so ALL files land in missingFiles and the gate fails loudly (exit 1) rather than silently passing. Same fail-closed path covers a reporter that throws on load/iteration (Node treats a broken --test-reporter as fatal) and a spawnSync error or null exit status (both handled with explicit exit 1). No silent-pass path found for any of the scenarios asked about. Question (b) -- per-file check evidence: the guard scripts themselves (check-test-count.js, test-file-completion-reporter.js) have no committed regression test in this diff -- AMoS's verification of the MILLER failure shape was an ad-hoc manual simulation, not a checked-in test. Noting as a nit: a future edit to either script has no automated regression coverage, which is the same uncovered-meta-risk class this PR closes for node --test itself, one layer up. Question (c) -- injection surface: check-test-count.js uses spawnSync(process.execPath, [...].concat(files), {...}) -- array-form argv, no shell, so glob-expanded file arguments cannot achieve shell injection regardless of content. test-file-completion-reporter.js calls no spawn/exec at all. No injection surface introduced. Question (d) -- real-home-write: confirmed for the new files. check-test-count.js has zero fs calls; test-file-completion-reporter.js has zero fs/path calls. The three touched test files' new hunks add no new fs.mkdtempSync/writeFileSync/rmSync calls -- only timer-cleanup and WebSocket-stub logic. Question (e) -- scanners: gitleaks found 6 pre-existing hits, all in worktree fixture certs and a WebSocket test nonce, none in this PR's changed lines. semgrep found 2 INFO/WARNING hits, both outside this diff's changed hunks or on non-attacker-controlled input (path.resolve(f) where f comes from the shell-expanded npm test glob, not network/user input) -- dropped per Pre-Report Gate. osv-scanner flagged pre-existing lockfile CVEs; package-lock.json is untouched by this diff and package.json's only change is the test script line -- no new dependency introduced, out of scope. Question (f) -- Conventional Commits: PR title fix(test): stop --test-force-exit truncation, fix leaked handles at root cause (lr-795882) is Conventional-Commits-compliant. The read-only GitHub tool available to this audit does not expose a per-commit list separate from the PR title/diff, so per-commit conformance across the full commit history could not be independently re-verified from this tool -- flagging as unverified-from-this-tool rather than asserting pass. No blocking findings. One nit: the two new merge-gate scripts (scripts/check-test-count.js, scripts/test-file-completion-reporter.js) have no committed automated test of their own per-file-completion logic in this diff. scanners_run: gitleaks (6 pre-existing hits, none in diff), semgrep (2 hits, both out of scope), osv-scanner (pre-existing lockfile findings, no new deps in diff) |
|
Merged via clagentic-loadout v0.2.0
|
|
Merged via clagentic-loadout v0.2.0
|
|
This issue has been resolved in version 1.9.0-beta.2. To update, run: |
Removes --test-force-exit from npm test and fixes the leaked handles it was papering over (agents SDK subprocess spawn at module load, an un-unref-ed smtp OTP cleanup interval, a fake WebSocket that never resolved onopen leaving a real reconnect-watchdog timer chain, an uncompleted loop iteration leaving a 10-minute watchdog armed, and two leaks in security.test.js: an unstopped loop-registry scheduler interval and a real un-awaited outbound HTTPS call to skills.sh).
Run-invariant check, v2 after BOBBIE/PEACHES review: scripts/check-test-count.js no longer trusts a loose total-count floor. It now runs the whole suite in ONE node --test invocation (same single-process timing/ordering as always -- no isolation change) with a custom reporter (scripts/test-file-completion-reporter.js) attached alongside the normal tap reporter. Node reporter API delivers a file field on every test:pass/test:fail event regardless of what TAP text shows, so this gets real per-file attribution without isolating files into separate processes. The check fails if ANY file named on the glob reports zero results -- the literal MILLER failure mode -- and keeps a secondary 1300 floor as a coarser net for a large in-file drop. Stated plainly, per review ask: this CATCHES a whole test file silently vanishing (a hard per-file boundary, not probabilistic); it does NOT catch a handful of tests dropped from within an otherwise-reporting file, which would need a checked-in expected-name list per file -- rejected as disproportionate maintenance for this bug class. Verified the new mechanism actually catches the motivating case: simulated MILLER exact failure shape (a file whose tests genuinely ran but were dropped from reporting) via the reporter and confirmed check-test-count.js fails correctly, independent of node --test own exit code.
Why a second design (v1, single combined-run TAP-total floor of 1300 against a live count of ~1407) was replaced: BOBBIE and PEACHES independently flagged that 107 tests of slack cannot catch a 21-test drop (MILLER own repro number) -- correct, fixed. An intermediate v1.5 (isolate each file into its own node --test invocation to get per-file signal) was technically correct but changed timing/jitter enough to expose an unrelated PRE-EXISTING flake in test/project-connection-hydrate-session-model-lr-041af8.test.js (a millisecond tie-break race in lib/project-connection.js findRestoredActiveSession, unrelated to any change in this PR, confirmed via git blame and isolated repro) -- rejected because the verification mechanism itself should never introduce new failure risk. Filed as lr-2fcf59, left alone in this diff per instruction. Final v2 design (single-process + custom reporter) avoids that risk entirely.
Also fixed: removed a verbatim-duplicated 12-line comment block in test/app-boot-esm-graph-load-lr-4c58ae.test.js (PEACHES blocking finding, confirmed) and anchored the TAP-summary parsing concern (BOBBIE nit) by construction -- v2 no longer parses a spoofable printed line at all; counts come from the reporter own structured event stream.
Confirmed per explicit request: lib/agents.js readAgentToolsFromFile (the real tool-enforcement path, lib/sdk-bridge.js:1760) reads on-disk per-request and is fully independent of refresh() populated _cache -- verified directly (no reference to _cache/_refreshPromise anywhere in that function) and via a second independent call-site audit of every getAll() use (all 4, in lib/project-sessions.js, are agent-picker/listing UI payloads only, never a tool-restriction decision). Skipping refresh() under NODE_TEST_CONTEXT therefore bounds to a stale/empty agent PICKER in a test context, never a security/enforcement bypass -- BOBBIE reading confirmed accurate.
Confirmed per explicit request: nothing in the shipped diff writes outside an isolated temp home. scripts/test-file-completion-reporter.js and scripts/check-test-count.js contain zero fs calls (spawn + stream-only). Every fs.mkdtempSync/writeFileSync/rmSync touched or added across the full PR diff (security.test.js, project-loop-message-lr-e31b.test.js, app-boot-esm-graph-load-lr-4c58ae.test.js) uses os.tmpdir()/mkdtempSync exclusively -- confirmed by direct re-read of every changed hunk. The stray ~/.clagentic/console/user-presence.json write disclosed earlier came only from my own ad-hoc diagnostic probe file, never committed, deleted before the PR was opened -- not present in the shipped diff at all.
Re-verified after all four fixes: ran the full suite 5 consecutive times, all --test-force-exit-free, all through the new v2 check-test-count.js. Every run: 1407 registered, 1406 pass, 1 skip (pre-existing intentional), 0 fail, exit 0. No hang, no truncation, no flake (the 041af8 race never triggers in this design since we never isolate files).
Unchanged from original PR body: task items 1-3 root-cause analysis (the five leaked handles), ESM-split evaluation (still not warranted), and fold-ins (gitignore for test/.scratch-*, users.js/user-presence.js vector ruled out). See prior PR body revision for full detail; this update layers the review-response changes on top without altering that analysis.
Not in scope, left alone per instruction: un-skipping test/project-connection-ownership-claim-lr-768c9e.test.js:153 (lr-a7b03e, blocked on this task by design); physically deleting the pre-existing untracked test/.scratch-* pile (gitignore entry is sufficient).
TASK: lr-795882