diff --git a/.gitignore b/.gitignore index 2efb43ad..d7adf624 100644 --- a/.gitignore +++ b/.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-* + diff --git a/lib/project-sessions.js b/lib/project-sessions.js index 68f2ceda..6d7df57d 100644 --- a/lib/project-sessions.js +++ b/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. diff --git a/lib/smtp.js b/lib/smtp.js index dfa2abde..cf048a21 100644 --- a/lib/smtp.js +++ b/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, diff --git a/package.json b/package.json index 468dcff0..32f6cae8 100644 --- a/package.json +++ b/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" diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js new file mode 100644 index 00000000..18e24273 --- /dev/null +++ b/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 " 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 )\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); diff --git a/scripts/test-file-completion-reporter.js b/scripts/test-file-completion-reporter.js new file mode 100644 index 00000000..15ceda94 --- /dev/null +++ b/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 + +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"; + } + } +}; diff --git a/test/app-boot-esm-graph-load-lr-4c58ae.test.js b/test/app-boot-esm-graph-load-lr-4c58ae.test.js index 67a2a5a8..ddca30a9 100644 --- a/test/app-boot-esm-graph-load-lr-4c58ae.test.js +++ b/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)" + ); +}); diff --git a/test/project-loop-message-lr-e31b.test.js b/test/project-loop-message-lr-e31b.test.js index 068ca439..5a826e6c 100644 --- a/test/project-loop-message-lr-e31b.test.js +++ b/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; } diff --git a/test/security.test.js b/test/security.test.js index 209f5de5..ab4811cd 100644 --- a/test/security.test.js +++ b/test/security.test.js @@ -910,22 +910,55 @@ test("skills proxy: rejects unauthenticated request with 401 in multi-user mode" assert.strictEqual(res403.status, 403, "authenticated user without skills permission gets 403"); // lr-ec2d: single-user mode removed; authenticated user with skills permission proceeds - var ctxOk = { - users: { - isMultiUser: function () { return true; }, - getEffectivePermissions: function () { return { skills: true }; }, - }, - osUsers: [], - getMultiUserFromReq: function () { return { id: "u2" }; }, + // + // lr-795882: the pass-through branch calls the REAL lib/server-skills.js + // handler, which (once past the permission gate this test exists to check) + // synchronously kicks off a real outbound https.get("https://skills.sh/...") + // — a genuine network dependency in a unit test, and the connection was + // never awaited, timed out, or closed, leaking a live TCP+TLS socket that + // kept `node --test` (run without --test-force-exit) from ever reaching + // idle for the rest of this suite. This test only asserts the auth gate + // was passed (see the pre-existing comment below), so stub https.get for + // the duration of this one call — scoped to this test, restored + // immediately after — instead of letting a real network call fire for an + // assertion that never reads its result. + var https = require("https"); + var origHttpsGet = https.get; + https.get = function (url, opts, cb) { + // Match Node's http.get behavior when only a callback is passed. + if (typeof opts === "function") { cb = opts; } + var fakeResp = { + statusCode: 200, + headers: {}, + on: function (event, handler) { + if (event === "end") setImmediate(handler); + return fakeResp; + }, + }; + if (typeof cb === "function") setImmediate(function () { cb(fakeResp); }); + var fakeReq = { on: function () { return fakeReq; } }; + return fakeReq; }; - var handlerOk = attachSkills(ctxOk).handleRequest; - var reqOk = makeReq("GET", "/api/skills?tab=all"); - reqOk.url = "/api/skills?tab=all"; - var resOk = makeRes(); - handlerOk(reqOk, resOk, "/api/skills"); - // Response is async (fetch); status is null now — just verify it's not 401/403 - assert.ok(resOk.status !== 401 && resOk.status !== 403, - "authenticated user with skills permission is not rejected (no 401 or 403)"); + try { + var ctxOk = { + users: { + isMultiUser: function () { return true; }, + getEffectivePermissions: function () { return { skills: true }; }, + }, + osUsers: [], + getMultiUserFromReq: function () { return { id: "u2" }; }, + }; + var handlerOk = attachSkills(ctxOk).handleRequest; + var reqOk = makeReq("GET", "/api/skills?tab=all"); + reqOk.url = "/api/skills?tab=all"; + var resOk = makeRes(); + handlerOk(reqOk, resOk, "/api/skills"); + // Response is async (fetch); status is null now — just verify it's not 401/403 + assert.ok(resOk.status !== 401 && resOk.status !== 403, + "authenticated user with skills permission is not rejected (no 401 or 403)"); + } finally { + https.get = origHttpsGet; + } }); // lr-30a5: sibling pre-auth 401 gates (lr-d857 B1 follow-up). Each test drives @@ -1455,10 +1488,11 @@ test("validateLoopId: accepts well-formed loop IDs", function () { test("loop_registry_files handler: sends loop_registry_error for invalid id via real handleLoopMessage", function () { var tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "loop-h-test-")); + var loop = null; try { var sent = []; var ctx = makeLoopCtx(tmpDir, function (msg) { sent.push(msg); }); - var loop = attachLoop(ctx); + loop = attachLoop(ctx); var attackIds = [ "../../../etc/passwd", @@ -1478,16 +1512,25 @@ test("loop_registry_files handler: sends loop_registry_error for invalid id via "error text must be invalid_loop_id for id=" + attackIds[i]); } } finally { + // lr-795882: attachLoop() unconditionally starts a real 30s + // setInterval (loopRegistry.startTimer(), project-loop.js -> scheduler.js) + // as part of attaching, independent of whether a loop is ever started. + // Left uncleared, that interval is a genuine leaked handle that keeps + // `node --test` (run without --test-force-exit) from ever reaching + // beforeExit/idle for the rest of this file's tests and every test file + // that runs after it in the same process. + if (loop) loop.stopTimer(); fs.rmSync(tmpDir, { recursive: true }); } }); test("loop_registry_save_files handler: sends loop_registry_error for invalid id via real handleLoopMessage", function () { var tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "loop-h-test-")); + var loop = null; try { var sent = []; var ctx = makeLoopCtx(tmpDir, function (msg) { sent.push(msg); }); - var loop = attachLoop(ctx); + loop = attachLoop(ctx); var attackIds = [ "../../../etc/passwd", @@ -1505,6 +1548,9 @@ test("loop_registry_save_files handler: sends loop_registry_error for invalid id "error text must be invalid_loop_id for id=" + attackIds[i]); } } finally { + // lr-795882: same leaked-timer class as the test above — see that + // comment for the full explanation. + if (loop) loop.stopTimer(); fs.rmSync(tmpDir, { recursive: true }); } });