fix(test): stop --test-force-exit truncation, fix leaked handles at root cause (lr-795882)

.gitignore

@@ -30,3 +30,11 @@ scripts/pr-body-*.txt
 scripts/smoke.sh
 test-results/
 
+# Same pattern as scripts/_* above, for the test/ tree — ad-hoc verification
+# logs, probe test files, and repro fixtures left behind by AMoS build
+# sessions while diagnosing/verifying a fix (lr-795882). Never part of the
+# shipped test suite; `test/*.test.js` (the npm test glob) already only
+# picks up real *.test.js files, so these are inert either way, but leaving
+# them untracked-and-unignored just accumulates as `git status` noise.
+test/.scratch-*
+

lib/project-sessions.js

@@ -11,9 +11,25 @@ var sessionActivity = require("./session-activity");
 // Kick off SDK agent discovery in the background at module load.
 // Errors are swallowed inside refresh(); the cache starts empty and fills
 // when the SDK subprocess completes initialization (~9s on this box).
-agentsModule.refresh().catch(function (e) {
-  console.error("[project-sessions] initial agent refresh failed:", e && e.message ? e.message : e);
-});
+//
+// lr-795882: skipped under `node --test` (NODE_TEST_CONTEXT is set natively
+// by the test runner itself — not a project-invented flag). This module is
+// required, directly or transitively via lib/project.js, by ~18 test files;
+// none of them exercise agent discovery, so this was spawning a real
+// @anthropic-ai/claude-agent-sdk subprocess purely as an unrequested
+// module-load side effect on every full suite run. refresh()'s SDK session
+// is aborted in a finally block (lib/agents.js) but the underlying process
+// exit is deferred by the SDK's own internal debounce, so the spawned
+// subprocess could still be alive — holding the event loop open — well
+// after the module that triggered it finished loading. This was the root
+// cause package.json's --test-force-exit was added to paper over (see
+// 5c17b6d) and was never actually fixed, only masked. Tests that need real
+// agent discovery call agentsModule.refresh() themselves explicitly.
+if (!process.env.NODE_TEST_CONTEXT) {
+  agentsModule.refresh().catch(function (e) {
+    console.error("[project-sessions] initial agent refresh failed:", e && e.message ? e.message : e);
+  });
+}
 
 // Format a user's answer to an ask_user_questions card as a plain user
 // message so the MCP path can feed it back to the agent on the next turn.

lib/smtp.js

@@ -183,7 +183,15 @@ function sendInviteEmail(email, inviteUrl, inviterName) {
 
 // --- Cleanup expired OTPs ---
 
-setInterval(function () {
+// lr-795882: unref()'d — matches the equivalent ws-ticket sweep in
+// server-auth.js (wsTicketSweepInterval). This is a housekeeping sweep, not
+// something that should keep the process alive on its own; leaving it
+// ref'd meant every test that required this module (or re-required it after
+// busting require.cache, as test/ws-ticket-auth-lr-de5fcb.test.js's
+// makeAuth() helper does once per test) left a real, permanently-running
+// setInterval handle on the event loop, blocking `node --test` from ever
+// reaching idle without --test-force-exit.
+var _otpCleanupInterval = setInterval(function () {
   var now = Date.now();
   var keys = Object.keys(otpStore);
   for (var i = 0; i < keys.length; i++) {
@@ -192,6 +200,7 @@ setInterval(function () {
     }
   }
 }, 60000);
+if (_otpCleanupInterval.unref) _otpCleanupInterval.unref();
 
 module.exports = {
   getSmtpConfig: getSmtpConfig,

package.json

@@ -12,7 +12,7 @@
     "dev": "node bin/cli.js --dev",
     "postinstall": "node scripts/postinstall.js",
     "prepack": "node scripts/write-build-sha.js",
-    "test": "node --test --test-force-exit test/*.test.js",
+    "test": "node scripts/check-test-count.js test/*.test.js",
     "install:local-test": "node scripts/install-local-test.js",
     "verify:installed-build": "node scripts/verify-installed-build.js",
     "semantic-release": "semantic-release"

scripts/check-test-count.js

@@ -0,0 +1,141 @@
+#!/usr/bin/env node
+"use strict";
+//
+// check-test-count.js — per-file completion + count floor for `npm test`
+// (lr-795882, hardened after PEACHES/BOBBIE PR #395 review).
+//
+// PROBLEM: `node --test` can exit 0 with a self-consistent-looking summary
+// even when a truncated run silently dropped an entire test file's worth of
+// tests (proved by MILLER during lr-a7b03e: 1406/1385/1406/1406 across four
+// runs on unmodified main, all exit 0, 1385 missing 21 real security tests
+// from test/xss-escape.test.js). A green `npm test` did not prove the full
+// suite ran. Fixing the underlying leaked handles (lr-795882) closed the
+// mechanism that caused THAT truncation, but nothing short of verifying
+// every file actually ran can catch the NEXT regression of this class — a
+// future leaked handle in a NEW test file would reproduce the exact same
+// silent-drop failure mode this script exists to catch.
+//
+// WHAT THIS DOES AND DOES NOT CATCH — state this plainly, not left for a
+// reader to work out (PR #395 review, andy):
+//   - CATCHES: any single test FILE that reports zero test:pass/test:fail
+//     events at all — the literal MILLER failure mode (one file's tests
+//     silently vanish while `node --test` still exits 0). This is a hard
+//     per-file boundary, not a probabilistic total: every file named on
+//     the command line is checked individually against the reporter's own
+//     event stream; there is no "close enough" combined count to hide
+//     behind, and — unlike an earlier version of this script — this
+//     mechanism does NOT need to isolate each file into its own process
+//     to get that per-file signal (see MECHANISM below), so it carries no
+//     risk of changing test timing/ordering behavior.
+//   - DOES NOT CATCH: a handful of tests silently dropped from WITHIN an
+//     otherwise-reporting file (e.g. 3 of a file's 40 tests vanish but the
+//     file still reports other passes and exits 0). Catching that would
+//     require a checked-in expected test-name list per file, which is far
+//     more maintenance than this bug class justifies. TEST_COUNT_FLOOR
+//     below is a coarser secondary net for a LARGE in-file drop, not a
+//     precise one.
+//
+// MECHANISM (v2 — replaces the v1 "single combined run + parse the shared
+// TAP summary's total" design, which BOBBIE/PEACHES correctly flagged as
+// too loose: a floor of 1300 against a live count of ~1407 has 107 tests
+// of slack, comfortably hiding MILLER's own 21-test drop; and a v1.5
+// per-file-isolated-process design, which was correct in principle but
+// exposed an unrelated pre-existing test-order flake
+// (project-connection-hydrate-session-model-lr-041af8.test.js's
+// millisecond tie-break) purely as a side effect of changing how files are
+// scheduled — rejected because the count-verification mechanism itself
+// should never be the thing introducing new failure risk):
+//
+//   Run ALL files in ONE node --test invocation, exactly as `npm test`
+//   always has (no per-file process isolation, no timing/ordering change),
+//   but attach a CUSTOM REPORTER (test-file-completion-reporter.js)
+//   alongside the normal `tap` reporter. Node's reporter API delivers a
+//   `file` field on every test:pass/test:fail event REGARDLESS of what
+//   TAP's own text output shows (TAP text has no per-file marker when
+//   multiple files share one process — that's what made v1 unable to do
+//   this without isolating files). The custom reporter emits one
+//   machine-readable "RESULT <pass|fail> <file>" line per test result to a
+//   separate destination stream; this script reads that stream, buckets
+//   results by file, and requires every file passed on argv to have at
+//   least one RESULT line.
+//
+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 <file...>)\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);
+
+if (result.error) {
+  process.stderr.write("[check-test-count] failed to spawn node --test: " + result.error.message + "\n");
+  process.exit(1);
+}
+
+// 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 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);
+}
+
+process.exit(result.status === null ? 1 : result.status);

scripts/test-file-completion-reporter.js

@@ -0,0 +1,43 @@
+"use strict";
+//
+// test-file-completion-reporter.js — custom `node --test` reporter for
+// scripts/check-test-count.js (lr-795882, hardened after PEACHES/BOBBIE PR
+// #395 review).
+//
+// WHY A CUSTOM REPORTER, NOT TAP TEXT PARSING: when `node --test` is given
+// multiple file arguments, all of them run in ONE shared process and their
+// results interleave into a single flat TAP stream with no per-file marker
+// in the human-readable text — there is no way to tell from TAP output
+// alone whether every file in the glob actually reported. An earlier
+// version of this check ran each file as its OWN `node --test` invocation
+// to get per-file attribution, but that changes timing/jitter
+// characteristics enough to expose an unrelated pre-existing flake
+// (test/project-connection-hydrate-session-model-lr-041af8.test.js's
+// millisecond-tie-break race in findRestoredActiveSession, filed
+// separately as a follow-up) — a mechanism change should not itself
+// introduce new failure risk. Node's reporter API (this file) receives a
+// `file` field on every test:pass/test:fail event regardless of TAP's own
+// text output, so this gets real per-file attribution from the SAME
+// single-process run `npm test` has always used, with no isolation/timing
+// change at all.
+//
+// CONTRACT: this reporter is used TOGETHER with the default `tap` reporter
+// (`node --test --test-reporter=tap --test-reporter-destination=stdout
+// --test-reporter=./test-file-completion-reporter.js
+// --test-reporter-destination=stderr`, see check-test-count.js) — Node
+// supports multiple --test-reporter flags, each with its own
+// --test-reporter-destination, so a developer running `npm test` still
+// sees normal TAP output on stdout; this reporter's machine-readable
+// RESULT lines go to a separate stream that scripts/check-test-count.js
+// parses. Output shape, one line per test:pass/test:fail event:
+//   RESULT <pass|fail> <absolute-file-path>
+
+module.exports = async function* fileCompletionReporter(source) {
+  for await (var event of source) {
+    if (event.type === "test:pass" || event.type === "test:fail") {
+      var file = (event.data && event.data.file) || "";
+      var kind = event.type === "test:pass" ? "pass" : "fail";
+      yield "RESULT " + kind + " " + file + "\n";
+    }
+  }
+};

test/app-boot-esm-graph-load-lr-4c58ae.test.js

@@ -163,9 +163,32 @@ function installBrowserGlobals() {
   });
   global.localStorage = { getItem: function () { return null; }, setItem: function () {}, removeItem: function () {} };
   global.sessionStorage = { getItem: function () { return null; }, setItem: function () {}, removeItem: function () {} };
+  // lr-795882: a real browser WebSocket always eventually calls onopen,
+  // onclose, or onerror. This stub previously never called any of them, so
+  // app-connection.js's connect()->openSocket() 3s "not connected yet" watchdog
+  // (connectTimeoutId, app-connection.js:297) never got cleared by a real
+  // onopen — it fired, tore the socket down, and called connect() again,
+  // which built a NEW real 3s Node timer, forever. Under `node --test`
+  // (no --test-force-exit) that unbounded setTimeout chain kept the process
+  // event loop alive indefinitely, and under --test-force-exit it silently
+  // truncated later test files in the same run instead of ever completing.
+  // Firing onopen asynchronously mirrors a real successful handshake, which
+  // reaches connect()'s real terminal state and clears its own timer via the
+  // production onopen handler — no test-side timer bookkeeping needed.
   global.WebSocket = function (url, protocols) {
+    var sock = {
+      send: function () {},
+      close: function () {},
+      onopen: null,
+      onclose: null,
+      onerror: null,
+      onmessage: null,
+    };
     wsConstructions.push({ url: url, protocols: protocols });
-    return { send: function () {}, close: function () {} };
+    setTimeout(function () {
+      if (typeof sock.onopen === "function") sock.onopen();
+    }, 0);
+    return sock;
   };
   global.lucide = { createIcons: function () {} };
   global.marked = { parse: function (s) { return s; }, setOptions: function () {}, use: function () {} };
@@ -216,3 +239,34 @@ test("connect() runs as part of app.js's top-level init sequence (WebSocket is c
     "loaded but boot did not reach the connection step"
   );
 });
+
+// Regression test for lr-795882: the fake WebSocket used to load app.js's
+// module graph above must resolve to a real terminal FSM state (connected),
+// not leave a live reconnect watchdog running. Before the fix, this file
+// left an unbounded self-rescheduling setTimeout chain
+// (app-connection.js:297's connectTimeoutId) running forever because the
+// fake WebSocket never called onopen/onclose/onerror — silently truncating
+// whatever test file happened to run after this one under
+// --test-force-exit, and hanging `node --test` outright without it.
+//
+// `store` is the same singleton app.js's own connect() call reads/writes —
+// asserting through it (rather than reaching into app-connection.js's
+// private connectTimeoutId closure variable) verifies the *effect* that
+// actually matters: the connection FSM reached a settled state and stopped
+// scheduling new watchdog timers, using only the module's public surface.
+test("connect()'s FSM reaches a settled connected state (no leaked reconnect watchdog, lr-795882)", async function () {
+  var storeModule = await import(pathToFileURL(
+    path.join(__dirname, "..", "lib", "public", "modules", "store.js")
+  ).href);
+  // The fake WebSocket's onopen fires via a real setTimeout(fn, 0) queued
+  // during app.js's module-load (see installBrowserGlobals above); yield to
+  // the event loop so that macrotask has a chance to run before asserting.
+  await new Promise(function (resolve) { setTimeout(resolve, 50); });
+  assert.equal(
+    storeModule.store.get("connected"), true,
+    "app.js's connect() FSM must reach 'connected' once the stubbed WebSocket " +
+    "opens — if this is false, the 3s connectTimeoutId watchdog in " +
+    "app-connection.js is still armed and will keep rescheduling itself " +
+    "(the exact leak lr-795882 fixed)"
+  );
+});

test/project-loop-message-lr-e31b.test.js

@@ -218,6 +218,16 @@ test("lr-e31b: runNextIteration() drains the queue into the next iteration promp
       return origSetTimeout(fn, delay);
     };
 
+    // lr-795882: runNextIteration() also arms a real, un-mocked 10-minute
+    // coderWatchdog setTimeout per iteration (project-loop.js's "Watchdog:
+    // if onQueryComplete hasn't fired after 10 minutes"). The global.setTimeout
+    // intercept above only special-cases delay === 1000 (the advance timer),
+    // so every coderWatchdog call passed straight through to the real timer
+    // — left armed for any iteration that isn't itself completed via
+    // onQueryComplete before the test ends. Keep the intercept installed
+    // (restored in the outer finally below) across BOTH iterations' full
+    // completion so neither iteration's coderWatchdog reaches the real event
+    // loop as a leaked handle.
     try {
       engine.startLoop({ maxIterations: 5 });
 
@@ -236,12 +246,25 @@ test("lr-e31b: runNextIteration() drains the queue into the next iteration promp
       ];
 
       // Complete iteration 1 with clean history (no error markers) so the
-      // simple-mode onQueryComplete path schedules iteration 2.
+      // simple-mode onQueryComplete path schedules iteration 2. This also
+      // clears iteration 1's coderWatchdog via the real production code path
+      // (project-loop.js:491).
       iter1Session.history.push({ type: "done", code: 0 });
       iter1Session.onQueryComplete(iter1Session);
 
       assert.ok(capturedAdvance, "iteration 1 completion should schedule the next iteration");
       capturedAdvance();
+
+      var iter2SessionForCleanup = ctx.sm.sessions.get(ls.currentSessionId);
+      assert.ok(iter2SessionForCleanup, "iteration 2 session should exist before assertions");
+
+      // Complete iteration 2 the same way — clears iteration 2's coderWatchdog
+      // via the same real onQueryComplete path. maxIterations is 5, so this
+      // itself schedules a THIRD 1000ms advance timer; the intercept above
+      // captures (rather than arms) it, and we simply never invoke it, so no
+      // iteration 3 ever starts and no further coderWatchdog is armed.
+      iter2SessionForCleanup.history.push({ type: "done", code: 0 });
+      iter2SessionForCleanup.onQueryComplete(iter2SessionForCleanup);
     } finally {
       global.setTimeout = origSetTimeout;
     }