From 62701d740dc9d877adc684aabf61711c96087951 Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 14:03:34 +1100 Subject: [PATCH 01/11] fix(exec): record a run that hits its time limit as a timeout (#106) A timed-out tool run was logged as a generic failure. ToolRun now carries status "timeout", timedOut: true and the timeoutMs that was exceeded, so every runner's provenance log tells a slow tool from a failing one. recordToolRun lets a runner that executes a command itself (to kill a whole process tree) land its run in the same log. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/runners/exec.test.ts | 17 +++++++++++++++++ src/runners/exec.ts | 29 +++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/runners/exec.test.ts b/src/runners/exec.test.ts index 4e0bdca..960e249 100644 --- a/src/runners/exec.test.ts +++ b/src/runners/exec.test.ts @@ -71,6 +71,23 @@ describe("tool run provenance", () => { expect(r.exitCode).toBe(2); }); + it("records a timeout as a timeout, not as a failure (#106)", () => { + startToolRecording(); + const result = run("sleep 5", "/tmp", 200); + const [r] = takeToolRuns(); + expect(result.ok).toBe(false); + expect(r).toMatchObject({ status: "timeout", timedOut: true, timeoutMs: 200, ok: false, notFound: false }); + }); + + it("does not mark an ordinary failure as timed out", () => { + startToolRecording(); + run("sh -c 'exit 1'", "/tmp"); + const [r] = takeToolRuns(); + expect(r.status).toBe("failed"); + expect(r.timedOut).toBeUndefined(); + expect(r.timeoutMs).toBeUndefined(); + }); + it("names the package rather than npx for delegated tools", () => { startToolRecording(); run("npx --yes knip --reporter json", "/tmp", 1); diff --git a/src/runners/exec.ts b/src/runners/exec.ts index 603b849..4d8b26b 100644 --- a/src/runners/exec.ts +++ b/src/runners/exec.ts @@ -22,8 +22,12 @@ export interface ToolRun { analyzer?: string; projectId?: string; projectPath?: string; - status: "success" | "failed"; + status: "success" | "failed" | "timeout"; exitCode: number | null; + /** True when the run was killed for exceeding its time limit. */ + timedOut?: boolean; + /** The time limit that was exceeded; set only when `timedOut`. */ + timeoutMs?: number; ok: boolean; durationMs: number; /** Combined output, trimmed and capped so reports stay a sane size. */ @@ -134,12 +138,14 @@ export function run(cmd: string, cwd: string, timeout = 60_000, context: ToolRun return { stdout, ok: true }; } catch (e: any) { const output = outputOf(e); + const timedOut = e?.code === "ETIMEDOUT"; record({ tool: toolNameOf(cmd), command: cmd, cwd, ...runContext, - status: "failed", + status: timedOut ? "timeout" : "failed", + ...(timedOut ? { timedOut: true, timeoutMs: timeout } : {}), exitCode: typeof e?.status === "number" ? e.status : null, ok: false, durationMs: Date.now() - started, @@ -150,6 +156,25 @@ export function run(cmd: string, cwd: string, timeout = 60_000, context: ToolRun } } +/** Record a run executed outside `run()` (e.g. by a runner that must kill a + * whole process tree), so it lands in the same provenance log. */ +export function recordToolRun( + cmd: string, + cwd: string, + result: Pick, + context: ToolRunContext = {}, +): void { + record({ + tool: toolNameOf(cmd), + command: cmd, + cwd, + ...normalizedContext({ ...defaultContext, ...context }), + ...result, + output: result.output.trim().slice(0, MAX_OUTPUT), + notFound: !result.ok && !result.timedOut && /not found|ENOENT|command not found/i.test(result.output), + }); +} + export function runJSON(cmd: string, cwd: string, timeout = 60_000, context: ToolRunContext = {}): T | null { const { stdout } = run(cmd, cwd, timeout, context); try { From 6fbb0747eebdff4238ebd86156f551f9960e4174 Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 14:03:34 +1100 Subject: [PATCH 02/11] fix(testing): treat a test-run timeout as not run, not failed (#106) A test run that hit the fixed 120 s limit raised an error, scored 0/20 for execution (worse than --skip-tests), could score a previous run's coverage file, and left npx/vitest/workers running because only /bin/sh was killed. - Per-project status "timeout" ("timed out after N s") and a warning test-run-timeout instead of the command-failed error; execution gets the same 10/20 as --skip-tests; no coverage is read for a timed-out root, nor the repo-level fallback. Check-level status derivation is unchanged. - The limit is configurable: --test-timeout or .vcqa.json checks.testing.settings.timeoutMs (flag wins; default 120000). The effective value and its source are recorded in the check details. - Test runs go through a small supervisor that starts the command in its own process group and, on timeout, kills the whole group and waits for it to be gone before returning. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/cli.ts | 9 ++ src/config.ts | 9 +- src/core.ts | 13 ++- src/runners/exec-tree.ts | 108 ++++++++++++++++++ src/runners/test-timeout.test.ts | 168 ++++++++++++++++++++++++++++ src/runners/testing.ts | 182 +++++++++++++++++++++++++------ 6 files changed, 451 insertions(+), 38 deletions(-) create mode 100644 src/runners/exec-tree.ts create mode 100644 src/runners/test-timeout.test.ts diff --git a/src/cli.ts b/src/cli.ts index ab01e3a..e025a2c 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -31,6 +31,7 @@ interface ParsedFlags { jsonOnly: boolean; ciMode: boolean; skipTests: boolean; + testTimeoutMs: number | null; watchMode: boolean; badgeMode: boolean; sarifMode: boolean; @@ -64,6 +65,11 @@ function parseFlags(): ParsedFlags { const topN = parseValueFlag("--top", 5) ?? 0; const failUnder = parseValueFlag("--fail-under"); + const testTimeoutMs = parseValueFlag("--test-timeout"); + if (args.includes("--test-timeout") && !testTimeoutMs) { + console.error("--test-timeout needs a positive whole number of milliseconds, e.g. --test-timeout 300000"); + process.exit(2); + } let diffBase: string | null = null; const diffIdx = args.indexOf("--diff"); @@ -84,6 +90,7 @@ function parseFlags(): ParsedFlags { jsonOnly: flags.has("--json"), ciMode: flags.has("--ci"), skipTests: flags.has("--skip-tests"), + testTimeoutMs, watchMode: flags.has("--watch"), badgeMode: flags.has("--badge"), sarifMode: flags.has("--sarif"), @@ -122,6 +129,7 @@ function printHelp(): void { \x1b[1mFlags:\x1b[0m --skip-tests Skip test execution (faster scan) + --test-timeout MS Per-project test run limit in ms (default: 120000) --ci CI mode (exit 1 if score < 60) --fail-under N Exit 1 if score below N (e.g. --fail-under 80) --json Output JSON only (no terminal UI) @@ -633,6 +641,7 @@ async function main() { // Run scan using core API with progress output const report = await scan(cwd, { skipTests, + testTimeoutMs: flags.testTimeoutMs ?? undefined, config, onProgress: quietMode ? undefined diff --git a/src/config.ts b/src/config.ts index 033359d..dc5c2f3 100644 --- a/src/config.ts +++ b/src/config.ts @@ -5,7 +5,7 @@ import { join } from "node:path"; export interface VcqaConfig { /** Disable or configure individual checks */ - checks?: Record; + checks?: Record }>; /** Extra glob patterns to ignore, merged with the versioned default exclusion policy. */ ignore?: string[]; /** Default fail-under threshold (overridden by --fail-under flag) */ @@ -49,6 +49,13 @@ export function getCheckIgnore(config: VcqaConfig, checkName: string): string[] return config.checks?.[checkName]?.ignore; } +/** Get per-check settings (`checks..settings`). Values are unvalidated — + * the check that reads a setting validates it and falls back to its default. */ +export function getCheckSettings(config: VcqaConfig, checkName: string): Record { + const settings = config.checks?.[checkName]?.settings; + return settings && typeof settings === "object" && !Array.isArray(settings) ? settings : {}; +} + function validate(raw: Record): VcqaConfig { const config: VcqaConfig = {}; diff --git a/src/core.ts b/src/core.ts index 7c9fd9d..02f289d 100644 --- a/src/core.ts +++ b/src/core.ts @@ -10,7 +10,7 @@ import { readFileSync } from "node:fs"; import { resolve } from "node:path"; import { buildAnalyzerSnapshots } from "./analyzer-snapshot.js"; import { CHECK_META, type CheckMeta, getCheckMeta } from "./check-meta.js"; -import { getCheckIgnore, isCheckEnabled, loadConfig, type VcqaConfig } from "./config.js"; +import { getCheckIgnore, getCheckSettings, isCheckEnabled, loadConfig, type VcqaConfig } from "./config.js"; import { detectRepoUrl, detectStack, detectWorkspace } from "./detect.js"; import { buildFileInventory } from "./file-inventory.js"; import { setGlobalIgnore, setGlobalIgnoreNames, setGlobalScanPolicy, setGlobalSrcRoots } from "./fs-utils.js"; @@ -80,6 +80,8 @@ type ScoreMode = "available-scored" | "available-unscored" | "not-applicable" | export interface ScanOptions { /** Skip test execution (faster scan). Default: false */ skipTests?: boolean; + /** Time limit for each test run, in ms. Overrides `checks.testing.settings.timeoutMs`. Default: 120000 */ + testTimeoutMs?: number; /** Only run these checks (by name). Default: all checks */ checks?: string[]; /** Override config (instead of loading from .vcqa.json). */ @@ -150,7 +152,14 @@ export async function scan(cwd: string, options: ScanOptions = {}): Promise runEnvValidation(resolvedCwd) }, { name: "git-hygiene", fn: () => runGitHygiene(resolvedCwd, fileInventory) }, { name: "memory-safety", fn: () => runMemorySafety(resolvedCwd, workspace, fileInventory) }, - { name: "testing", fn: () => runTesting(resolvedCwd, stack, skipTests, srcRoots, workspace, fileInventory) }, + { + name: "testing", + fn: () => + runTesting(resolvedCwd, stack, skipTests, srcRoots, workspace, fileInventory, { + timeoutMs: options.testTimeoutMs, + settings: getCheckSettings(config, "testing"), + }), + }, { name: "secrets", fn: () => runSecrets(resolvedCwd, fileInventory) }, { name: "security", fn: () => runSecurity(resolvedCwd, fileInventory) }, { name: "dependencies", fn: () => runDependencies(resolvedCwd, stack) }, diff --git a/src/runners/exec-tree.ts b/src/runners/exec-tree.ts new file mode 100644 index 0000000..7cf07f8 --- /dev/null +++ b/src/runners/exec-tree.ts @@ -0,0 +1,108 @@ +/** Run a command with a time limit that kills its whole process tree. + * + * `execSync`'s timeout signals only the `/bin/sh` it spawned. Everything that + * shell started — `npx` → `vitest` → worker pools — is reparented and keeps + * running after the scan has given up on it (vibecodeqa/cli#106). + * + * The runners are synchronous and `spawnSync` cannot start a detached child, + * so the command runs under a tiny Node supervisor: it starts the command in + * its own process group (`detached`), and on timeout signals the group — + * SIGTERM, then SIGKILL after a grace period — and waits until no member of + * the group is left before exiting. When this function returns, nothing the + * command started is still alive. The supervisor reports the timeout on fd 3, + * so it cannot be confused with anything the command prints or exits with. + */ + +import { spawnSync } from "node:child_process"; +import { recordToolRun, type ToolRunContext } from "./exec.js"; + +const MAX_BUFFER = 64 * 1024 * 1024; +const TIMEOUT_MARKER = "vcqa:timed-out"; + +// Runs as `node -e`. argv: [cmd, timeoutMs]. Plain JS — it is not compiled. +const SUPERVISOR = ` +const { spawn, execFileSync } = require("node:child_process"); +const { writeSync } = require("node:fs"); +const [cmd, timeoutArg] = process.argv.slice(1); +const timeoutMs = Number(timeoutArg); +const win = process.platform === "win32"; +const child = spawn(cmd, { shell: true, detached: !win, stdio: ["ignore", "inherit", "inherit"] }); +let timedOut = false; +function signalTree(sig) { + try { + if (win) execFileSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" }); + else process.kill(-child.pid, sig); + } catch {} +} +function groupAlive() { + if (win) return false; + try { process.kill(-child.pid, 0); return true; } catch (e) { return e.code === "EPERM"; } +} +function finish(code) { + if (timedOut) { try { writeSync(3, "${TIMEOUT_MARKER}"); } catch {} } + process.exit(code); +} +const timer = setTimeout(() => { + timedOut = true; + signalTree("SIGTERM"); + const started = Date.now(); + let killed = false; + const poll = setInterval(() => { + if (!groupAlive()) { clearInterval(poll); finish(124); return; } + const waited = Date.now() - started; + if (!killed && waited >= 2000) { killed = true; signalTree("SIGKILL"); } + if (waited >= 10000) { clearInterval(poll); finish(124); } + }, 20); +}, timeoutMs); +child.on("error", (e) => { clearTimeout(timer); process.stderr.write(String(e && e.message)); process.exit(127); }); +child.on("exit", (code, signal) => { + if (timedOut) return; + clearTimeout(timer); + // Grandchildren that outlive the command (a stray watcher) go too. + signalTree("SIGKILL"); + process.exit(code ?? (signal ? 128 : 1)); +}); +`; + +export interface TreeRunResult { + stdout: string; + ok: boolean; + timedOut: boolean; +} + +/** Like `run()` from exec.ts, but a timeout kills every process the command + * started, and is recorded as `status: "timeout"`. */ +export function runWithTreeKill(cmd: string, cwd: string, timeoutMs: number, context: ToolRunContext = {}): TreeRunResult { + const started = Date.now(); + const res = spawnSync(process.execPath, ["-e", SUPERVISOR, cmd, String(timeoutMs)], { + cwd, + encoding: "utf-8", + maxBuffer: MAX_BUFFER, + stdio: ["ignore", "pipe", "pipe", "pipe"], + // Backstop only: the supervisor normally exits well before this. + timeout: timeoutMs + 30_000, + killSignal: "SIGKILL", + }); + const stdout = typeof res.stdout === "string" ? res.stdout : ""; + const stderr = typeof res.stderr === "string" ? res.stderr : ""; + const marker = typeof res.output?.[3] === "string" ? res.output[3] : ""; + const timedOut = marker.includes(TIMEOUT_MARKER) || (res.error as NodeJS.ErrnoException | undefined)?.code === "ETIMEDOUT"; + const ok = !timedOut && !res.error && res.status === 0; + const combined = `${stdout}${stdout && stderr ? "\n" : ""}${stderr}`.trim() || (res.error ? String(res.error) : ""); + const output = ok ? stdout : combined; + + recordToolRun( + cmd, + cwd, + { + status: ok ? "success" : timedOut ? "timeout" : "failed", + exitCode: ok ? 0 : timedOut ? null : (res.status ?? null), + ok, + durationMs: Date.now() - started, + output, + ...(timedOut ? { timedOut: true, timeoutMs } : {}), + }, + context, + ); + return { stdout: output, ok, timedOut }; +} diff --git a/src/runners/test-timeout.test.ts b/src/runners/test-timeout.test.ts new file mode 100644 index 0000000..27cac93 --- /dev/null +++ b/src/runners/test-timeout.test.ts @@ -0,0 +1,168 @@ +/** vibecodeqa/cli#106 — a test run that hits its time limit is "not run", not + * "failed", and leaves nothing behind. + * + * The fixture's `vitest` is a local fake (node_modules/vitest), so `npx vitest` + * resolves it without a download. It starts a grandchild (`sleep`), records + * both pids, then waits the number of ms in `fake-sleep-ms` before printing a + * passing vitest JSON report and a fresh coverage summary. A stale coverage + * summary is on disk before the run. + */ + +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { scan } from "../core.js"; +import type { ToolRun } from "./exec.js"; +import { resolveTestTimeout } from "./testing.js"; + +const FAKE_VITEST = `#!/usr/bin/env node +const { spawn } = require("node:child_process"); +const { mkdirSync, readFileSync, writeFileSync } = require("node:fs"); +const sleepMs = Number(readFileSync("fake-sleep-ms", "utf-8")); +const grandchild = spawn("sleep", ["60"], { stdio: "ignore" }); +writeFileSync("pids.json", JSON.stringify({ fake: process.pid, grandchild: grandchild.pid })); +setTimeout(() => { + grandchild.kill("SIGKILL"); + mkdirSync("coverage", { recursive: true }); + const pct = { pct: 90 }; + writeFileSync("coverage/coverage-summary.json", JSON.stringify({ total: { statements: pct, lines: pct, branches: pct, functions: pct } })); + console.log(JSON.stringify({ numTotalTests: 1, numPassedTests: 1, numFailedTests: 0, testResults: [] })); +}, sleepMs); +`; + +const STALE_PCT = { pct: 42 }; +const STALE_COVERAGE = { total: { statements: STALE_PCT, lines: STALE_PCT, branches: STALE_PCT, functions: STALE_PCT } }; + +const dirs: string[] = []; +afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +function makeFixture(sleepMs: number, settings?: Record): string { + const dir = mkdtempSync(join(tmpdir(), "vcqa-timeout-")); + dirs.push(dir); + writeFileSync(join(dir, "package.json"), JSON.stringify({ name: "timeout-fixture", devDependencies: { vitest: "1.0.0" } })); + if (settings) writeFileSync(join(dir, ".vcqa.json"), JSON.stringify({ checks: { testing: { settings } } })); + mkdirSync(join(dir, "src")); + writeFileSync(join(dir, "src/app.ts"), "export const x = 1;\n"); + writeFileSync( + join(dir, "src/app.test.ts"), + "import { expect, it } from 'vitest';\nimport { x } from './app';\nit('works as expected', () => { expect(x).toBe(1); });\n", + ); + mkdirSync(join(dir, "coverage")); + writeFileSync(join(dir, "coverage/coverage-summary.json"), JSON.stringify(STALE_COVERAGE)); + writeFileSync(join(dir, "fake-sleep-ms"), String(sleepMs)); + mkdirSync(join(dir, "node_modules/vitest"), { recursive: true }); + mkdirSync(join(dir, "node_modules/.bin"), { recursive: true }); + writeFileSync( + join(dir, "node_modules/vitest/package.json"), + JSON.stringify({ name: "vitest", version: "1.0.0", bin: { vitest: "cli.js" } }), + ); + writeFileSync(join(dir, "node_modules/vitest/cli.js"), FAKE_VITEST); + chmodSync(join(dir, "node_modules/vitest/cli.js"), 0o755); + symlinkSync("../vitest/cli.js", join(dir, "node_modules/.bin/vitest")); + return dir; +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (e) { + return (e as NodeJS.ErrnoException).code === "EPERM"; + } +} + +async function scanTesting(dir: string, testTimeoutMs?: number) { + const report = await scan(dir, { checks: ["testing"], testTimeoutMs }); + const check = report.checks.find((c) => c.name === "testing"); + if (!check) throw new Error("testing check missing"); + const details = check.details as Record; + return { check, details, toolRuns: (details.toolRuns ?? []) as ToolRun[] }; +} + +describe("test run timeout (#106)", () => { + it.skipIf(process.platform === "win32")( + "a run past the configured limit is a warning, scores like --skip-tests, reads no coverage and leaves no process", + async () => { + const dir = makeFixture(30_000, { timeoutMs: 2000 }); + const { check, details, toolRuns } = await scanTesting(dir); + + const run = toolRuns.find((r) => r.tool === "vitest"); + expect(run).toMatchObject({ status: "timeout", timedOut: true, timeoutMs: 2000, ok: false }); + expect(run?.durationMs).toBeLessThan(10_000); + + expect(details.testTimeoutMs).toBe(2000); + expect(details.testTimeoutSource).toBe("config"); + expect(details.executionStatus).toBe("timeout"); + expect(details.testProjects).toHaveLength(1); + expect(details.testProjects[0]).toMatchObject({ + status: "timeout", + statusLabel: "timed out after 2 s", + timeoutMs: 2000, + coverageStatus: "not-reported", + coverage: null, + }); + // The stale 42% summary on disk was not read. + expect(details.coverage).toBeUndefined(); + + const timeoutIssues = check.issues.filter((i) => i.rule === "test-run-timeout"); + expect(timeoutIssues).toHaveLength(1); + expect(timeoutIssues[0].severity).toBe("warning"); + expect(check.issues.some((i) => i.rule === "test-run-command-failed")).toBe(false); + + // Execution scored as for --skip-tests: same fixture, same static + // points, and --skip-tests gets 10/20 for execution plus whatever the + // stale coverage file is worth (which the timed-out scan must not get). + const skipped = await scan(dir, { checks: ["testing"], skipTests: true }); + const skippedCheck = skipped.checks.find((c) => c.name === "testing"); + const avgStale = 42; + const staleCoveragePoints = Math.round((avgStale / 100) * 20); + expect(check.score).toBe((skippedCheck?.score ?? 0) - staleCoveragePoints); + + // Nothing from the timed-out run survives scan(). + const pids = JSON.parse(readFileSync(join(dir, "pids.json"), "utf-8")) as { fake: number; grandchild: number }; + expect(isAlive(pids.fake)).toBe(false); + expect(isAlive(pids.grandchild)).toBe(false); + }, + 30_000, + ); + + it.skipIf(process.platform === "win32")( + "the same fixture passes with a larger limit from --test-timeout", + async () => { + const dir = makeFixture(2500, { timeoutMs: 2000 }); + const { check, details, toolRuns } = await scanTesting(dir, 15_000); + + const run = toolRuns.find((r) => r.tool === "vitest"); + expect(run).toMatchObject({ status: "success", ok: true }); + expect(run?.timedOut).toBeUndefined(); + expect(details.testTimeoutMs).toBe(15_000); + expect(details.testTimeoutSource).toBe("flag"); + expect(details.testProjects[0]).toMatchObject({ status: "passed", coverageStatus: "reported" }); + expect(details.testProjects[0].timeoutMs).toBeUndefined(); + expect(details.coverage).toMatchObject({ stmts: 90 }); + expect(check.issues.some((i) => i.rule === "test-run-timeout")).toBe(false); + expect(existsSync(join(dir, "pids.json"))).toBe(true); + }, + 30_000, + ); +}); + +describe("resolveTestTimeout", () => { + it("defaults to 120 s", () => { + expect(resolveTestTimeout()).toEqual({ ms: 120_000, source: "default" }); + }); + + it("prefers the flag over the setting", () => { + expect(resolveTestTimeout({ timeoutMs: 5000, settings: { timeoutMs: 9000 } })).toEqual({ ms: 5000, source: "flag" }); + expect(resolveTestTimeout({ settings: { timeoutMs: 9000 } })).toEqual({ ms: 9000, source: "config" }); + }); + + it("falls back to the default for an invalid setting and says so", () => { + expect(resolveTestTimeout({ settings: { timeoutMs: "fast" } })).toEqual({ ms: 120_000, source: "default", invalidSetting: "fast" }); + expect(resolveTestTimeout({ settings: { timeoutMs: -1 } })).toMatchObject({ ms: 120_000, source: "default" }); + expect(resolveTestTimeout({ settings: { timeoutMs: 1.5 } })).toMatchObject({ ms: 120_000, source: "default" }); + }); +}); diff --git a/src/runners/testing.ts b/src/runners/testing.ts index 7dac626..a520d6c 100644 --- a/src/runners/testing.ts +++ b/src/runners/testing.ts @@ -16,7 +16,7 @@ import { inventorySourceFiles } from "../file-inventory.js"; import { getProductionFiles, normalizeToolPath, readDeps } from "../fs-utils.js"; import type { AnalyzerMetric, CheckResult, Issue, StackInfo, WorkspaceInfo } from "../types.js"; import { gradeFromScore } from "../types.js"; -import { run } from "./exec.js"; +import { runWithTreeKill } from "./exec-tree.js"; // ── Types ── @@ -48,7 +48,49 @@ interface CoverageData { } type TestCaseStatus = "passed" | "failed" | "skipped" | "todo" | "unknown"; -type TestProjectRunStatus = "passed" | "failed" | "partial" | "no-tests-matched" | "parse-failed" | "command-failed" | "skipped"; +type TestProjectRunStatus = + | "passed" + | "failed" + | "partial" + | "no-tests-matched" + | "parse-failed" + | "command-failed" + | "timeout" + | "skipped"; + +/** Default per-project test run limit. Override with `--test-timeout ` or + * `.vcqa.json` `checks.testing.settings.timeoutMs`. */ +export const DEFAULT_TEST_TIMEOUT_MS = 120_000; + +export interface TestingOptions { + /** From `--test-timeout`; wins over the config setting. */ + timeoutMs?: number; + /** `checks.testing.settings` from `.vcqa.json` / package.json#vcqa. */ + settings?: Record; +} + +export interface EffectiveTestTimeout { + ms: number; + source: "flag" | "config" | "default"; + /** Set when `settings.timeoutMs` was present but not a positive integer. */ + invalidSetting?: unknown; +} + +function isValidTimeout(value: unknown): value is number { + return typeof value === "number" && Number.isInteger(value) && value > 0; +} + +export function resolveTestTimeout(options: TestingOptions = {}): EffectiveTestTimeout { + const configured = options.settings?.timeoutMs; + const invalid = configured !== undefined && !isValidTimeout(configured) ? { invalidSetting: configured } : {}; + if (isValidTimeout(options.timeoutMs)) return { ms: options.timeoutMs, source: "flag", ...invalid }; + if (isValidTimeout(configured)) return { ms: configured, source: "config" }; + return { ms: DEFAULT_TEST_TIMEOUT_MS, source: "default", ...invalid }; +} + +function formatSeconds(ms: number): string { + return `${Number((ms / 1000).toFixed(1))} s`; +} type TestCoverageStatus = "reported" | "not-reported"; interface TestCaseReport { @@ -93,6 +135,8 @@ interface NormalizedTestProjectReport extends TestExecutionReport { reportParsed: boolean; coverageStatus: TestCoverageStatus; coverage: CoverageData | null; + /** Set when the run was killed for exceeding this limit (ms). */ + timeoutMs?: number; } // ── Classification rules ── @@ -482,7 +526,7 @@ function commandForTestTarget(target: TestRunTarget): string { : "npx jest --json --coverage --coverageReporters=json-summary"; } -function statusLabel(status: TestProjectRunStatus): string { +function statusLabel(status: TestProjectRunStatus, timeoutMs?: number): string { switch (status) { case "passed": return "passed"; @@ -496,6 +540,8 @@ function statusLabel(status: TestProjectRunStatus): string { return "report could not be parsed"; case "command-failed": return "command failed"; + case "timeout": + return timeoutMs ? `timed out after ${formatSeconds(timeoutMs)}` : "timed out"; case "skipped": return "skipped"; } @@ -516,8 +562,14 @@ export function normalizeTestProjectReport(input: { executionOk: boolean | null; coverage: CoverageData | null; skipped?: boolean; + /** The limit the run exceeded; marks the project as timed out. */ + timedOutAfterMs?: number; }): NormalizedTestProjectReport { - const status = input.skipped ? "skipped" : reportStatus(input.report, input.executionOk); + const status: TestProjectRunStatus = input.skipped + ? "skipped" + : input.timedOutAfterMs !== undefined + ? "timeout" + : reportStatus(input.report, input.executionOk); const base = input.report ?? { runner: input.target.runner, cwd: input.target.cwd, @@ -541,11 +593,12 @@ export function normalizeTestProjectReport(input: { path: input.target.projectPath ?? ".", command: input.command, status, - statusLabel: statusLabel(status), + statusLabel: statusLabel(status, input.timedOutAfterMs), executionOk: input.executionOk, reportParsed: Boolean(input.report), coverageStatus: input.coverage ? "reported" : "not-reported", coverage: input.coverage, + ...(status === "timeout" ? { timeoutMs: input.timedOutAfterMs } : {}), }; } @@ -556,6 +609,7 @@ export function summarizeTestProjectStatus(projects: Pick p.status === "passed" || p.status === "partial")) { return projects.every((p) => p.status === "passed") ? "passed" : "partial"; } + if (projects.some((p) => p.status === "timeout")) return "timeout"; if (projects.every((p) => p.status === "no-tests-matched")) return "no-tests-matched"; return "parse-failed"; } @@ -563,8 +617,9 @@ export function summarizeTestProjectStatus(projects: Pick 0) { - issues.push({ - severity: "warning", - message: `${report.path} has ${staticTestCount} discovered test file${staticTestCount === 1 ? "" : "s"}, but ${report.runner} matched 0 tests`, - rule: "test-run-no-tests-matched", - }); - } - if (report.status === "parse-failed" || report.status === "command-failed") { - issues.push({ - severity: report.status === "command-failed" ? "error" : "warning", - message: `${report.path} ${report.runner} ${report.statusLabel}`, - rule: `test-run-${report.status}`, - }); - } - } + issues.push(...projectRunIssues(testReports, testFiles)); if (coverage) { if (coverage.branches < 50) { @@ -944,14 +1011,7 @@ export function runTesting( score += pyramidScore; // Execution (20 points) - if (execution) { - const passRate = execution.total > 0 ? execution.passed / execution.total : 0; - score += Math.round(passRate * 20); - } else if (!skipExec) { - // Couldn't run tests - } else { - score += 10; // partial credit when skipped - } + score += executionPoints(execution, skipExec, testReports); // Coverage (20 points) if (coverage) { @@ -1001,6 +1061,13 @@ export function runTesting( } : {}), executionStatus: summarizeTestProjectStatus(testReports), + ...(skipExec + ? {} + : { + testTimeoutMs: testTimeout.ms, + testTimeoutSource: testTimeout.source, + ...("invalidSetting" in testTimeout ? { testTimeoutInvalidSetting: testTimeout.invalidSetting } : {}), + }), ...(execution ? { passed: execution.passed, failed: execution.failed, total: execution.total } : {}), ...(testReports.length > 0 ? { testReports } : {}), ...(testReports.length > 0 @@ -1012,6 +1079,7 @@ export function runTesting( command: report.command, status: report.status, statusLabel: report.statusLabel, + ...(report.timeoutMs !== undefined ? { timeoutMs: report.timeoutMs } : {}), reportParsed: report.reportParsed, executionOk: report.executionOk, passed: report.passed, @@ -1138,3 +1206,47 @@ function staticTestFilesForProject(projectPath: string, testFiles: TestFile[]): const prefix = `${projectPath.replace(/^\/+|\/+$/g, "")}/`; return testFiles.filter((file) => file.path.startsWith(prefix)).length; } + +function projectRunIssues(testReports: NormalizedTestProjectReport[], testFiles: TestFile[]): Issue[] { + const issues: Issue[] = []; + for (const report of testReports) { + const staticTestCount = staticTestFilesForProject(report.path, testFiles); + if (report.status === "no-tests-matched" && staticTestCount > 0) { + issues.push({ + severity: "warning", + message: `${report.path} has ${staticTestCount} discovered test file${staticTestCount === 1 ? "" : "s"}, but ${report.runner} matched 0 tests`, + rule: "test-run-no-tests-matched", + }); + } + if (report.status === "timeout") { + issues.push({ + severity: "warning", + message: `${report.path} ${report.runner} ${report.statusLabel} — tests not scored; raise --test-timeout or checks.testing.settings.timeoutMs`, + rule: "test-run-timeout", + }); + } + if (report.status === "parse-failed" || report.status === "command-failed") { + issues.push({ + severity: report.status === "command-failed" ? "error" : "warning", + message: `${report.path} ${report.runner} ${report.statusLabel}`, + rule: `test-run-${report.status}`, + }); + } + } + return issues; +} + +function executionPoints( + execution: { passed: number; failed: number; total: number } | null, + skipExec: boolean, + testReports: NormalizedTestProjectReport[], +): number { + if (execution) { + const passRate = execution.total > 0 ? execution.passed / execution.total : 0; + return Math.round(passRate * 20); + } + // Partial credit when not run: skipped, or stopped at the time limit. A + // timeout says how fast the machine is, not whether the tests pass. + if (skipExec || testReports.some((report) => report.status === "timeout")) return 10; + return 0; // couldn't run tests +} From 32bf0270f531e47f651159bbc128495565f5e75a Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 19:22:07 +1100 Subject: [PATCH 03/11] refactor(exec): build every recorded tool run in one helper (#106) run() and recordToolRun() each assembled ToolRun entries on their own: tool name, command, cwd, context, capped output, the not-found guess. Both now go through a single toolRunEntry() builder, with run() recording via recordToolRun(), so a run executed outside run() lands in the log in exactly the same shape. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/runners/exec.test.ts | 34 +++++++++++++++++++++- src/runners/exec.ts | 62 +++++++++++++++++----------------------- 2 files changed, 60 insertions(+), 36 deletions(-) diff --git a/src/runners/exec.test.ts b/src/runners/exec.test.ts index 960e249..27d7ae1 100644 --- a/src/runners/exec.test.ts +++ b/src/runners/exec.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { filterToolRuns, run, startToolRecording, type ToolRun, takeToolRuns } from "./exec.js"; +import { filterToolRuns, recordToolRun, run, startToolRecording, type ToolRun, takeToolRuns } from "./exec.js"; /** Provenance is the mechanism that makes a report auditable — without it a * clean score cannot be distinguished from a tool that never ran, or one that @@ -118,6 +118,38 @@ describe("tool run provenance", () => { expect(r.output.length).toBeLessThanOrEqual(8000); }); + it("records an externally executed run exactly as run() would (#106)", () => { + const noisy = "node -e \"process.stdout.write('x'.repeat(20000))\""; + startToolRecording({ analyzerId: "testing" }); + run(noisy, "/tmp", 60_000, { projectId: "app" }); + const viaRun = takeToolRuns()[0]; + startToolRecording({ analyzerId: "testing" }); + recordToolRun( + noisy, + "/tmp", + { status: "success", exitCode: 0, ok: true, durationMs: viaRun.durationMs, output: `${"x".repeat(20000)}\n` }, + { projectId: "app" }, + ); + const viaRecord = takeToolRuns()[0]; + expect(viaRecord).toEqual(viaRun); + expect(viaRecord.output.length).toBeLessThan(20000); + }); + + it("does not call a timed-out run whose output mentions ENOENT a missing binary", () => { + startToolRecording(); + recordToolRun("npx vitest run", "/tmp", { + status: "timeout", + exitCode: null, + ok: false, + durationMs: 5, + output: "ENOENT: no such file", + timedOut: true, + timeoutMs: 5, + }); + const [r] = takeToolRuns(); + expect(r).toMatchObject({ tool: "vitest", status: "timeout", notFound: false }); + }); + it("filters tool runs by analyzer and project for page-specific logs", () => { const runs: ToolRun[] = [ { diff --git a/src/runners/exec.ts b/src/runners/exec.ts index 4d8b26b..112c728 100644 --- a/src/runners/exec.ts +++ b/src/runners/exec.ts @@ -114,7 +114,6 @@ export function filterToolRuns(runs: ToolRun[], filter: ToolRunFilter = {}): Too export function run(cmd: string, cwd: string, timeout = 60_000, context: ToolRunContext = {}): { stdout: string; ok: boolean } { const started = Date.now(); - const runContext = normalizedContext({ ...defaultContext, ...context }); try { const stdout = execSync(cmd, { cwd, @@ -123,48 +122,34 @@ export function run(cmd: string, cwd: string, timeout = 60_000, context: ToolRun maxBuffer: MAX_BUFFER, stdio: ["pipe", "pipe", "pipe"], }); - record({ - tool: toolNameOf(cmd), - command: cmd, - cwd, - ...runContext, - status: "success", - exitCode: 0, - ok: true, - durationMs: Date.now() - started, - output: stdout.trim().slice(0, MAX_OUTPUT), - notFound: false, - }); + recordToolRun(cmd, cwd, { status: "success", exitCode: 0, ok: true, durationMs: Date.now() - started, output: stdout }, context); return { stdout, ok: true }; } catch (e: any) { const output = outputOf(e); const timedOut = e?.code === "ETIMEDOUT"; - record({ - tool: toolNameOf(cmd), - command: cmd, + recordToolRun( + cmd, cwd, - ...runContext, - status: timedOut ? "timeout" : "failed", - ...(timedOut ? { timedOut: true, timeoutMs: timeout } : {}), - exitCode: typeof e?.status === "number" ? e.status : null, - ok: false, - durationMs: Date.now() - started, - output: output.trim().slice(0, MAX_OUTPUT), - notFound: /not found|ENOENT|command not found/i.test(output), - }); + { + status: timedOut ? "timeout" : "failed", + ...(timedOut ? { timedOut: true, timeoutMs: timeout } : {}), + exitCode: typeof e?.status === "number" ? e.status : null, + ok: false, + durationMs: Date.now() - started, + output, + }, + context, + ); return { stdout: output, ok: false }; } } -/** Record a run executed outside `run()` (e.g. by a runner that must kill a - * whole process tree), so it lands in the same provenance log. */ -export function recordToolRun( - cmd: string, - cwd: string, - result: Pick, - context: ToolRunContext = {}, -): void { - record({ +type ToolRunResult = Pick; + +/** The one place a ToolRun entry is built: command, cwd, context, capped + * output and the not-found guess. Every recorded run goes through it. */ +function toolRunEntry(cmd: string, cwd: string, result: ToolRunResult, context: ToolRunContext): ToolRun { + return { tool: toolNameOf(cmd), command: cmd, cwd, @@ -172,7 +157,14 @@ export function recordToolRun( ...result, output: result.output.trim().slice(0, MAX_OUTPUT), notFound: !result.ok && !result.timedOut && /not found|ENOENT|command not found/i.test(result.output), - }); + }; +} + +/** Record a finished run in the provenance log. `run()` records through this, + * and so does a runner that executes its command another way (e.g. one that + * must kill a whole process tree). */ +export function recordToolRun(cmd: string, cwd: string, result: ToolRunResult, context: ToolRunContext = {}): void { + record(toolRunEntry(cmd, cwd, result, context)); } export function runJSON(cmd: string, cwd: string, timeout = 60_000, context: ToolRunContext = {}): T | null { From ef3db8665080df23a178e47e506ca7a2babc73dc Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 19:29:36 +1100 Subject: [PATCH 04/11] fix(exec): tear the test process group down however the run ends (#106) Running the test command in its own process group took it out of the terminal's reach: Ctrl-C killed the CLI and the supervisor, and left the test run alive under PID 1. The supervisor now tears the group down (SIGTERM, then SIGKILL, waiting until it is empty) on every abnormal end: - SIGINT, SIGTERM or SIGHUP to the supervisor, so Ctrl-C works again; - the parent dying, seen as EOF on a pipe only the parent holds open; - the parent giving up on its output (maxBuffer) or hitting the backstop: spawnSync now sends SIGTERM rather than SIGKILL, so the handler runs, and the relay also stops on a broken pipe. The command's stdout/stderr are now relayed by the supervisor instead of inherited. A process outside the group that keeps the pipe open (a detached helper) no longer holds a finished run open until the backstop and turns it into a "timeout": the supervisor stops relaying shortly after the command exits, and reports the exit code on fd 3, which wins over the backstop's ETIMEDOUT. A limit above 2^31-1 ms, which setTimeout would fire after 1 ms, is clamped to that maximum. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/runners/exec-tree.test.ts | 148 +++++++++++++++++++++++++++++++++ src/runners/exec-tree.ts | 151 ++++++++++++++++++++++++++-------- 2 files changed, 265 insertions(+), 34 deletions(-) create mode 100644 src/runners/exec-tree.test.ts diff --git a/src/runners/exec-tree.test.ts b/src/runners/exec-tree.test.ts new file mode 100644 index 0000000..5d4f16e --- /dev/null +++ b/src/runners/exec-tree.test.ts @@ -0,0 +1,148 @@ +/** vibecodeqa/cli#106 — the supervisor behind runWithTreeKill must leave no + * process behind however the run ends: timeout, Ctrl-C, a signal to the + * supervisor, the parent dying, or the parent giving up on its output. */ + +import { type ChildProcess, spawn } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { startToolRecording, takeToolRuns } from "./exec.js"; +import { MAX_TIMER_MS, runWithTreeKill, supervisorSpawn } from "./exec-tree.js"; + +const posix = process.platform !== "win32"; + +// A separate parent process running the exact supervisor runWithTreeKill uses, +// so the test can signal or kill the parent (or the supervisor) mid-run. +const PARENT = ` +const { spawnSync } = require("node:child_process"); +const { writeFileSync } = require("node:fs"); +const spec = JSON.parse(process.argv[1]); +const res = spawnSync(spec.file, spec.args, { ...spec.options, cwd: spec.cwd }); +writeFileSync(spec.cwd + "/result.json", JSON.stringify({ status: res.status, signal: res.signal, marker: res.output && res.output[3] })); +`; + +const dirs: string[] = []; +const leftovers: number[] = []; +afterEach(() => { + for (const pid of leftovers.splice(0)) { + try { + process.kill(pid, "SIGKILL"); + } catch {} + } + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +function tempDir(): string { + const dir = mkdtempSync(join(tmpdir(), "vcqa-tree-")); + dirs.push(dir); + return dir; +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (e) { + return (e as NodeJS.ErrnoException).code === "EPERM"; + } +} + +async function waitFor(cond: () => boolean, ms = 5000): Promise { + const until = Date.now() + ms; + while (Date.now() < until) { + if (cond()) return true; + await new Promise((r) => setTimeout(r, 25)); + } + return cond(); +} + +/** Start a parent in its own process group (like a CLI in a terminal) running + * `cmd` under the supervisor; resolves once `cmd` has written its pids. */ +async function startParent(dir: string, cmd: string): Promise<{ parent: ChildProcess; pids: number[] }> { + const spec = { ...supervisorSpawn(cmd, 60_000), cwd: dir }; + const parent = spawn(process.execPath, ["-e", PARENT, JSON.stringify(spec)], { detached: true, stdio: "ignore" }); + if (parent.pid) leftovers.push(parent.pid); + const pidFile = join(dir, "pids"); + expect(await waitFor(() => existsSync(pidFile) && readFileSync(pidFile, "utf-8").trim().split(/\s+/).length >= 2)).toBe(true); + const pids = readFileSync(pidFile, "utf-8").trim().split(/\s+/).map(Number); + leftovers.push(...pids); + return { parent, pids }; +} + +// `$$` is the shell, which execs into `sleep`; `$PPID` is the supervisor. +const SLEEPER = "echo $$ $PPID > pids; exec sleep 300"; + +describe.skipIf(!posix)("runWithTreeKill", () => { + it("returns the output and exit status of a normal run", () => { + const dir = tempDir(); + expect(runWithTreeKill("echo hello", dir, 10_000)).toEqual({ stdout: "hello\n", ok: true, timedOut: false }); + startToolRecording(); + const failed = runWithTreeKill("echo oops >&2; exit 3", dir, 10_000); + const [r] = takeToolRuns(); + expect(failed).toMatchObject({ ok: false, timedOut: false, stdout: "oops" }); + expect(r).toMatchObject({ status: "failed", exitCode: 3, ok: false }); + }); + + it("on timeout kills the whole group and records a timeout", () => { + const dir = tempDir(); + startToolRecording(); + const res = runWithTreeKill("sleep 60 & echo $! > pids; exec sleep 60", dir, 300); + const [r] = takeToolRuns(); + expect(res).toMatchObject({ ok: false, timedOut: true }); + expect(r).toMatchObject({ status: "timeout", timedOut: true, timeoutMs: 300, exitCode: null }); + expect(isAlive(Number(readFileSync(join(dir, "pids"), "utf-8")))).toBe(false); + }); + + it("is not held open by a process outside the group that keeps stdout", () => { + const dir = tempDir(); + const helper = + "node -e \"const c=require('child_process').spawn('sleep',['45'],{detached:true,stdio:['ignore','inherit','inherit']});require('fs').writeFileSync('helper',String(c.pid));c.unref()\""; + const started = Date.now(); + const res = runWithTreeKill(`${helper}; echo report`, dir, 10_000); + const elapsed = Date.now() - started; + leftovers.push(Number(readFileSync(join(dir, "helper"), "utf-8"))); + expect(res).toEqual({ stdout: "report\n", ok: true, timedOut: false }); + expect(elapsed).toBeLessThan(5000); + }); + + it("clamps a limit too large for a timer instead of timing out at once", () => { + const dir = tempDir(); + expect(supervisorSpawn("true", 3e9).limit).toBe(MAX_TIMER_MS); + expect(runWithTreeKill("sleep 0.2; echo done", dir, 3e9)).toEqual({ stdout: "done\n", ok: true, timedOut: false }); + }); + + it("kills the group when the parent gives up on its output (maxBuffer)", () => { + const dir = tempDir(); + const res = runWithTreeKill("echo $$ > pids; exec yes", dir, 60_000); + expect(res).toMatchObject({ ok: false, timedOut: false }); + expect(isAlive(Number(readFileSync(join(dir, "pids"), "utf-8")))).toBe(false); + }, 30_000); + + it("Ctrl-C (SIGINT to the parent's process group) leaves nothing running", async () => { + const dir = tempDir(); + const { parent, pids } = await startParent(dir, SLEEPER); + process.kill(-(parent.pid as number), "SIGINT"); + expect(await waitFor(() => !isAlive(pids[0]))).toBe(true); + expect(await waitFor(() => !isAlive(pids[1]))).toBe(true); + }); + + it("a SIGTERM to the supervisor kills the group and is reported as a failure", async () => { + const dir = tempDir(); + const { pids } = await startParent(dir, SLEEPER); + process.kill(pids[1], "SIGTERM"); + expect(await waitFor(() => existsSync(join(dir, "result.json")))).toBe(true); + expect(isAlive(pids[0])).toBe(false); + const result = JSON.parse(readFileSync(join(dir, "result.json"), "utf-8")); + expect(result.status).toBe(143); + expect(result.marker).not.toContain("vcqa:timed-out"); + }); + + it("kills the group when the parent dies outright", async () => { + const dir = tempDir(); + const { parent, pids } = await startParent(dir, SLEEPER); + process.kill(parent.pid as number, "SIGKILL"); + expect(await waitFor(() => !isAlive(pids[0]))).toBe(true); + expect(await waitFor(() => !isAlive(pids[1]))).toBe(true); + }); +}); diff --git a/src/runners/exec-tree.ts b/src/runners/exec-tree.ts index 7cf07f8..b25df60 100644 --- a/src/runners/exec-tree.ts +++ b/src/runners/exec-tree.ts @@ -5,12 +5,27 @@ * running after the scan has given up on it (vibecodeqa/cli#106). * * The runners are synchronous and `spawnSync` cannot start a detached child, - * so the command runs under a tiny Node supervisor: it starts the command in - * its own process group (`detached`), and on timeout signals the group — - * SIGTERM, then SIGKILL after a grace period — and waits until no member of - * the group is left before exiting. When this function returns, nothing the - * command started is still alive. The supervisor reports the timeout on fd 3, - * so it cannot be confused with anything the command prints or exits with. + * so the command runs under a tiny Node supervisor. It starts the command in + * its own process group (`detached`) and tears the group down — SIGTERM, then + * SIGKILL after a grace period, waiting until no member is left — whenever the + * run ends for any reason: + * + * - the time limit passes; + * - the supervisor is signalled (SIGINT/SIGTERM/SIGHUP). Ctrl-C at a terminal + * reaches the supervisor, which sits in the CLI's process group, but not the + * detached command, so the supervisor must pass it on; + * - the parent goes away: the supervisor holds one end of a pipe (fd 4) whose + * other end only the parent holds, and sees EOF on it when the parent dies; + * - the parent gives up on it (output past `maxBuffer`, or the backstop + * timeout): `spawnSync` then sends SIGTERM, which is handled as above. + * + * The command's output is relayed through the supervisor rather than inherited, + * so a process outside the group that keeps the command's stdout open (a + * `detached` helper) cannot hold the run open: the supervisor stops relaying + * shortly after the command exits, and its exit closes the parent's pipes. + * + * The supervisor reports on fd 3 — a timeout marker, or the command's exit + * code — so the outcome cannot be confused with anything the command prints. */ import { spawnSync } from "node:child_process"; @@ -18,6 +33,9 @@ import { recordToolRun, type ToolRunContext } from "./exec.js"; const MAX_BUFFER = 64 * 1024 * 1024; const TIMEOUT_MARKER = "vcqa:timed-out"; +const EXIT_MARKER = "vcqa:exit:"; +/** Largest delay `setTimeout` honours; anything above fires after 1 ms. */ +export const MAX_TIMER_MS = 2 ** 31 - 1; // Runs as `node -e`. argv: [cmd, timeoutMs]. Plain JS — it is not compiled. const SUPERVISOR = ` @@ -26,8 +44,16 @@ const { writeSync } = require("node:fs"); const [cmd, timeoutArg] = process.argv.slice(1); const timeoutMs = Number(timeoutArg); const win = process.platform === "win32"; -const child = spawn(cmd, { shell: true, detached: !win, stdio: ["ignore", "inherit", "inherit"] }); -let timedOut = false; +const child = spawn(cmd, { shell: true, detached: !win, stdio: ["ignore", "pipe", "pipe"] }); +let ending = false; +function report(text) { try { writeSync(3, text); } catch {} } +function writeAll(fd, buf) { + let off = 0; + while (off < buf.length) { + try { off += writeSync(fd, buf, off); } + catch (e) { if (e.code !== "EAGAIN") throw e; } + } +} function signalTree(sig) { try { if (win) execFileSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" }); @@ -38,32 +64,89 @@ function groupAlive() { if (win) return false; try { process.kill(-child.pid, 0); return true; } catch (e) { return e.code === "EPERM"; } } -function finish(code) { - if (timedOut) { try { writeSync(3, "${TIMEOUT_MARKER}"); } catch {} } - process.exit(code); -} -const timer = setTimeout(() => { - timedOut = true; +// Tear the group down and exit. The single exit path for every abnormal end. +function stop(code, timedOut) { + if (ending) return; + ending = true; + clearTimeout(timer); signalTree("SIGTERM"); const started = Date.now(); let killed = false; + const finish = () => { if (timedOut) report("${TIMEOUT_MARKER}"); process.exit(code); }; + if (!child.pid) finish(); const poll = setInterval(() => { - if (!groupAlive()) { clearInterval(poll); finish(124); return; } + if (!groupAlive()) { clearInterval(poll); finish(); return; } const waited = Date.now() - started; if (!killed && waited >= 2000) { killed = true; signalTree("SIGKILL"); } - if (waited >= 10000) { clearInterval(poll); finish(124); } + if (waited >= 10000) { clearInterval(poll); finish(); } }, 20); -}, timeoutMs); -child.on("error", (e) => { clearTimeout(timer); process.stderr.write(String(e && e.message)); process.exit(127); }); +} +const relay = (fd) => (chunk) => { + try { writeAll(fd, chunk); } catch { stop(129, false); } // parent stopped reading +}; +child.stdout.on("data", relay(1)); +child.stderr.on("data", relay(2)); +const timer = setTimeout(() => stop(124, true), timeoutMs); +for (const [sig, code] of [["SIGINT", 130], ["SIGTERM", 143], ["SIGHUP", 129]]) process.on(sig, () => stop(code, false)); +if (!win) { + try { + const watch = new (require("node:net").Socket)({ fd: 4, readable: true, writable: false }); + const gone = () => stop(129, false); + watch.on("end", gone); watch.on("close", gone); watch.on("error", gone); + } catch {} +} +child.on("error", (e) => { + if (ending) return; + ending = true; + clearTimeout(timer); + try { writeAll(2, Buffer.from(String(e && e.message))); } catch {} + report("${EXIT_MARKER}127"); + process.exit(127); +}); child.on("exit", (code, signal) => { - if (timedOut) return; + if (ending) return; + ending = true; clearTimeout(timer); // Grandchildren that outlive the command (a stray watcher) go too. signalTree("SIGKILL"); - process.exit(code ?? (signal ? 128 : 1)); + const status = code ?? (signal ? 128 : 1); + // Relay what is still buffered, but do not wait on a process outside the + // group that holds the pipes open: stop once they close, go quiet, or 2 s pass. + const done = () => { report("${EXIT_MARKER}" + status); process.exit(status); }; + const streams = [child.stdout, child.stderr].filter((s) => !s.closed); + if (streams.length === 0) done(); + let open = streams.length; + for (const s of streams) s.on("close", () => { if (--open === 0) done(); }); + let quietTimer = setTimeout(done, 200); + const busy = () => { clearTimeout(quietTimer); quietTimer = setTimeout(done, 200); }; + child.stdout.on("data", busy); + child.stderr.on("data", busy); + setTimeout(done, 2000); }); `; +/** How the supervisor is started: `spawnSync(file, args, options)`. Exposed so + * tests can run the exact same supervisor from a separate parent process. */ +export function supervisorSpawn(cmd: string, timeoutMs: number) { + const limit = Math.min(Math.max(1, Math.floor(timeoutMs)), MAX_TIMER_MS); + return { + limit, + file: process.execPath, + args: ["-e", SUPERVISOR, cmd, String(limit)], + options: { + encoding: "utf-8" as const, + maxBuffer: MAX_BUFFER, + // fd 3: the supervisor's report. fd 4: held open by this process only, + // so the supervisor sees EOF on it if this process dies. + stdio: ["ignore", "pipe", "pipe", "pipe", "pipe"] as Array<"ignore" | "pipe">, + // Backstop only: the supervisor normally exits well before this. SIGTERM, + // not SIGKILL, so the supervisor still tears the group down. + timeout: limit + 30_000, + killSignal: "SIGTERM" as const, + }, + }; +} + export interface TreeRunResult { stdout: string; ok: boolean; @@ -71,23 +154,23 @@ export interface TreeRunResult { } /** Like `run()` from exec.ts, but a timeout kills every process the command - * started, and is recorded as `status: "timeout"`. */ + * started, and is recorded as `status: "timeout"`. A `timeoutMs` above + * {@link MAX_TIMER_MS} is clamped to it. */ export function runWithTreeKill(cmd: string, cwd: string, timeoutMs: number, context: ToolRunContext = {}): TreeRunResult { const started = Date.now(); - const res = spawnSync(process.execPath, ["-e", SUPERVISOR, cmd, String(timeoutMs)], { - cwd, - encoding: "utf-8", - maxBuffer: MAX_BUFFER, - stdio: ["ignore", "pipe", "pipe", "pipe"], - // Backstop only: the supervisor normally exits well before this. - timeout: timeoutMs + 30_000, - killSignal: "SIGKILL", - }); + const { limit, file, args, options } = supervisorSpawn(cmd, timeoutMs); + const res = spawnSync(file, args, { ...options, cwd }); const stdout = typeof res.stdout === "string" ? res.stdout : ""; const stderr = typeof res.stderr === "string" ? res.stderr : ""; const marker = typeof res.output?.[3] === "string" ? res.output[3] : ""; - const timedOut = marker.includes(TIMEOUT_MARKER) || (res.error as NodeJS.ErrnoException | undefined)?.code === "ETIMEDOUT"; - const ok = !timedOut && !res.error && res.status === 0; + const exited = new RegExp(`${EXIT_MARKER}(\\d+)`).exec(marker); + const errorCode = (res.error as NodeJS.ErrnoException | undefined)?.code; + // The supervisor's own report wins; the backstop's ETIMEDOUT counts only + // when the supervisor never said how the command ended. + const timedOut = marker.includes(TIMEOUT_MARKER) || (!exited && errorCode === "ETIMEDOUT"); + const status = exited ? Number(exited[1]) : res.status; + const failedToRun = Boolean(res.error) && !(exited && errorCode === "ETIMEDOUT"); + const ok = !timedOut && !failedToRun && status === 0; const combined = `${stdout}${stdout && stderr ? "\n" : ""}${stderr}`.trim() || (res.error ? String(res.error) : ""); const output = ok ? stdout : combined; @@ -96,11 +179,11 @@ export function runWithTreeKill(cmd: string, cwd: string, timeoutMs: number, con cwd, { status: ok ? "success" : timedOut ? "timeout" : "failed", - exitCode: ok ? 0 : timedOut ? null : (res.status ?? null), + exitCode: ok ? 0 : timedOut ? null : (status ?? null), ok, durationMs: Date.now() - started, output, - ...(timedOut ? { timedOut: true, timeoutMs } : {}), + ...(timedOut ? { timedOut: true, timeoutMs: limit } : {}), }, context, ); From 6769e147f693f71eadde1bee78bbe13d71debfcc Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 19:34:00 +1100 Subject: [PATCH 05/11] fix(testing): reject a test timeout no timer can hold (#106) Node fires a setTimeout above 2^31-1 ms after 1 ms, so a huge --test-timeout or checks.testing.settings.timeoutMs (someone meaning "no limit") marked every project as timed out at once. Both now accept a whole number of ms from 1 to 2147483647 (~24.8 days): the flag exits 2 with that range, and the setting falls back to the default with the existing invalid-setting info issue. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/cli.test.ts | 12 ++++++++++++ src/cli.ts | 5 +++-- src/runners/test-timeout.test.ts | 6 ++++++ src/runners/testing.ts | 19 ++++++++++++------- 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/cli.test.ts b/src/cli.test.ts index 1289d35..3e480eb 100644 --- a/src/cli.test.ts +++ b/src/cli.test.ts @@ -96,6 +96,18 @@ describe("CLI flags", () => { } }, 30_000); + it("--test-timeout rejects a value a timer cannot hold, or no value (#106)", () => { + for (const value of ["3000000000", "0", ""]) { + try { + execSync(`node ${CLI} --json --test-timeout ${value} .`, { encoding: "utf-8", timeout: 30_000, cwd: TMP, stdio: "pipe" }); + expect.unreachable(`--test-timeout ${value} should exit 2`); + } catch (e: any) { + expect(e.status).toBe(2); + expect(String(e.stderr)).toContain("from 1 to 2147483647"); + } + } + }, 30_000); + it("--fail-under does not exit when score is above threshold", () => { const out = run("--skip-tests --json --fail-under 0 ."); const report = JSON.parse(out); diff --git a/src/cli.ts b/src/cli.ts index e025a2c..8ae0991 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -16,6 +16,7 @@ import { detectStack, detectWorkspace } from "./detect.js"; import { postPRComment } from "./pr-comment.js"; import { generatePages } from "./report/html.js"; import { buildReportHistorySnapshot, withFreshAnalyzerSnapshots } from "./report-contract.js"; +import { isValidTestTimeout, MAX_TEST_TIMEOUT_MS } from "./runners/testing.js"; import { computeTrend, formatTrend, type TrendDelta } from "./trend.js"; import type { VibeReport, WorkspaceInfo } from "./types.js"; import { buildReportUploadPayload, currentGitSha } from "./upload.js"; @@ -66,8 +67,8 @@ function parseFlags(): ParsedFlags { const topN = parseValueFlag("--top", 5) ?? 0; const failUnder = parseValueFlag("--fail-under"); const testTimeoutMs = parseValueFlag("--test-timeout"); - if (args.includes("--test-timeout") && !testTimeoutMs) { - console.error("--test-timeout needs a positive whole number of milliseconds, e.g. --test-timeout 300000"); + if (args.includes("--test-timeout") && !isValidTestTimeout(testTimeoutMs)) { + console.error(`--test-timeout needs a whole number of milliseconds from 1 to ${MAX_TEST_TIMEOUT_MS}, e.g. --test-timeout 300000`); process.exit(2); } diff --git a/src/runners/test-timeout.test.ts b/src/runners/test-timeout.test.ts index 27cac93..494695b 100644 --- a/src/runners/test-timeout.test.ts +++ b/src/runners/test-timeout.test.ts @@ -165,4 +165,10 @@ describe("resolveTestTimeout", () => { expect(resolveTestTimeout({ settings: { timeoutMs: -1 } })).toMatchObject({ ms: 120_000, source: "default" }); expect(resolveTestTimeout({ settings: { timeoutMs: 1.5 } })).toMatchObject({ ms: 120_000, source: "default" }); }); + + it("accepts up to 2^31-1 ms and rejects anything a timer would fire at once", () => { + expect(resolveTestTimeout({ settings: { timeoutMs: 2 ** 31 - 1 } })).toEqual({ ms: 2 ** 31 - 1, source: "config" }); + expect(resolveTestTimeout({ settings: { timeoutMs: 3e9 } })).toEqual({ ms: 120_000, source: "default", invalidSetting: 3e9 }); + expect(resolveTestTimeout({ timeoutMs: 2 ** 31, settings: { timeoutMs: 9000 } })).toEqual({ ms: 9000, source: "config" }); + }); }); diff --git a/src/runners/testing.ts b/src/runners/testing.ts index a520d6c..a74a575 100644 --- a/src/runners/testing.ts +++ b/src/runners/testing.ts @@ -16,7 +16,7 @@ import { inventorySourceFiles } from "../file-inventory.js"; import { getProductionFiles, normalizeToolPath, readDeps } from "../fs-utils.js"; import type { AnalyzerMetric, CheckResult, Issue, StackInfo, WorkspaceInfo } from "../types.js"; import { gradeFromScore } from "../types.js"; -import { runWithTreeKill } from "./exec-tree.js"; +import { MAX_TIMER_MS, runWithTreeKill } from "./exec-tree.js"; // ── Types ── @@ -76,15 +76,20 @@ export interface EffectiveTestTimeout { invalidSetting?: unknown; } -function isValidTimeout(value: unknown): value is number { - return typeof value === "number" && Number.isInteger(value) && value > 0; +/** Largest accepted test timeout: 2^31-1 ms (~24.8 days), the most a Node + * timer can wait. Larger values are rejected, not clamped. */ +export const MAX_TEST_TIMEOUT_MS = MAX_TIMER_MS; + +/** A test timeout is a whole number of ms from 1 to {@link MAX_TEST_TIMEOUT_MS}. */ +export function isValidTestTimeout(value: unknown): value is number { + return typeof value === "number" && Number.isInteger(value) && value > 0 && value <= MAX_TEST_TIMEOUT_MS; } export function resolveTestTimeout(options: TestingOptions = {}): EffectiveTestTimeout { const configured = options.settings?.timeoutMs; - const invalid = configured !== undefined && !isValidTimeout(configured) ? { invalidSetting: configured } : {}; - if (isValidTimeout(options.timeoutMs)) return { ms: options.timeoutMs, source: "flag", ...invalid }; - if (isValidTimeout(configured)) return { ms: configured, source: "config" }; + const invalid = configured !== undefined && !isValidTestTimeout(configured) ? { invalidSetting: configured } : {}; + if (isValidTestTimeout(options.timeoutMs)) return { ms: options.timeoutMs, source: "flag", ...invalid }; + if (isValidTestTimeout(configured)) return { ms: configured, source: "config" }; return { ms: DEFAULT_TEST_TIMEOUT_MS, source: "default", ...invalid }; } @@ -948,7 +953,7 @@ export function runTesting( if ("invalidSetting" in testTimeout) { issues.push({ severity: "info", - message: `checks.testing.settings.timeoutMs must be a positive integer (ms); using ${testTimeout.ms} ms`, + message: `checks.testing.settings.timeoutMs must be a whole number of ms from 1 to ${MAX_TEST_TIMEOUT_MS}; using ${testTimeout.ms} ms`, rule: "invalid-test-timeout-setting", }); } From 293f88e7db9d030643e9ae65bd7a17fb490f15cd Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 19:37:41 +1100 Subject: [PATCH 06/11] fix(testing): give timeout credit only when every project timed out (#106) Execution got 10/20 whenever no project reported results and any project had timed out, so a monorepo with one timed-out project and one whose test command failed scored 10 where it used to score 0. The 10 points now need every non-skipped project to have timed out; otherwise the previous rule stands (pass rate over the projects that reported, else 0), and the failed command keeps its error. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/runners/test-timeout.test.ts | 61 +++++++++++++++++++++++++++++--- src/runners/testing.ts | 17 +++++---- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/src/runners/test-timeout.test.ts b/src/runners/test-timeout.test.ts index 494695b..64ba03e 100644 --- a/src/runners/test-timeout.test.ts +++ b/src/runners/test-timeout.test.ts @@ -13,13 +13,16 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { scan } from "../core.js"; +import { detectStack } from "../detect.js"; import type { ToolRun } from "./exec.js"; -import { resolveTestTimeout } from "./testing.js"; +import { resolveTestTimeout, runTesting, testExecutionPoints } from "./testing.js"; const FAKE_VITEST = `#!/usr/bin/env node const { spawn } = require("node:child_process"); const { mkdirSync, readFileSync, writeFileSync } = require("node:fs"); -const sleepMs = Number(readFileSync("fake-sleep-ms", "utf-8")); +const mode = readFileSync("fake-sleep-ms", "utf-8").trim(); +if (mode === "fail") { console.error("boom"); process.exit(1); } +const sleepMs = Number(mode); const grandchild = spawn("sleep", ["60"], { stdio: "ignore" }); writeFileSync("pids.json", JSON.stringify({ fake: process.pid, grandchild: grandchild.pid })); setTimeout(() => { @@ -39,9 +42,10 @@ afterEach(() => { for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); -function makeFixture(sleepMs: number, settings?: Record): string { - const dir = mkdtempSync(join(tmpdir(), "vcqa-timeout-")); - dirs.push(dir); +function makeFixture(sleepMs: number | "fail", settings?: Record, into?: string): string { + const dir = into ?? mkdtempSync(join(tmpdir(), "vcqa-timeout-")); + if (!into) dirs.push(dir); + mkdirSync(dir, { recursive: true }); writeFileSync(join(dir, "package.json"), JSON.stringify({ name: "timeout-fixture", devDependencies: { vitest: "1.0.0" } })); if (settings) writeFileSync(join(dir, ".vcqa.json"), JSON.stringify({ checks: { testing: { settings } } })); mkdirSync(join(dir, "src")); @@ -172,3 +176,50 @@ describe("resolveTestTimeout", () => { expect(resolveTestTimeout({ timeoutMs: 2 ** 31, settings: { timeoutMs: 9000 } })).toEqual({ ms: 9000, source: "config" }); }); }); + +describe("timeout scoring across projects (#106)", () => { + const timedOut = { status: "timeout" as const }; + + it("gives 10/20 only when every project that ran timed out", () => { + expect(testExecutionPoints(null, false, [timedOut])).toBe(10); + expect(testExecutionPoints(null, false, [timedOut, timedOut, { status: "skipped" }])).toBe(10); + expect(testExecutionPoints(null, false, [timedOut, { status: "command-failed" }])).toBe(0); + expect(testExecutionPoints(null, false, [timedOut, { status: "parse-failed" }])).toBe(0); + expect(testExecutionPoints(null, false, [])).toBe(0); + expect(testExecutionPoints(null, true, [])).toBe(10); + }); + + it("scores the projects that reported by pass rate, timed-out ones aside", () => { + expect(testExecutionPoints({ passed: 1, failed: 1, total: 2 }, false, [timedOut, { status: "failed" }])).toBe(10); + expect(testExecutionPoints({ passed: 0, failed: 3, total: 3 }, false, [timedOut, { status: "failed" }])).toBe(0); + }); + + it.skipIf(process.platform === "win32")( + "a timed-out project does not hide another project's failed command", + () => { + const run = (second: number | "fail") => { + const dir = mkdtempSync(join(tmpdir(), "vcqa-timeout-mono-")); + dirs.push(dir); + writeFileSync(join(dir, "package.json"), JSON.stringify({ name: "mono", private: true })); + makeFixture(30_000, undefined, join(dir, "packages/slow")); + makeFixture(second, undefined, join(dir, "packages/other")); + // Keep coverage out of the comparison: a failed run's coverage file is + // still read, as before #106, and would differ between the two runs. + rmSync(join(dir, "packages/other/coverage"), { recursive: true }); + const srcRoots = ["packages/slow/src", "packages/other/src"]; + return runTesting(dir, detectStack(dir), false, srcRoots, undefined, undefined, { timeoutMs: 1500 }); + }; + const bothTimedOut = run(30_000); + const oneFailed = run("fail"); + const statuses = (check: typeof oneFailed) => + (check.details as Record).testProjects.map((p: { status: string }) => p.status).sort(); + + expect(statuses(bothTimedOut)).toEqual(["timeout", "timeout"]); + expect(statuses(oneFailed)).toEqual(["command-failed", "timeout"]); + expect(oneFailed.issues.some((i) => i.rule === "test-run-command-failed" && i.severity === "error")).toBe(true); + // Same static points; only execution differs: 10 when all timed out, 0 here. + expect(oneFailed.score).toBe(bothTimedOut.score - 10); + }, + 30_000, + ); +}); diff --git a/src/runners/testing.ts b/src/runners/testing.ts index a74a575..00701f5 100644 --- a/src/runners/testing.ts +++ b/src/runners/testing.ts @@ -1016,7 +1016,7 @@ export function runTesting( score += pyramidScore; // Execution (20 points) - score += executionPoints(execution, skipExec, testReports); + score += testExecutionPoints(execution, skipExec, testReports); // Coverage (20 points) if (coverage) { @@ -1241,17 +1241,22 @@ function projectRunIssues(testReports: NormalizedTestProjectReport[], testFiles: return issues; } -function executionPoints( +/** Execution points (of 20). Pass rate over the projects that reported; 10 + * when tests were not run at all — `--skip-tests`, or EVERY project that ran + * stopped at the time limit (a timeout says how fast the machine is, not + * whether the tests pass); otherwise 0. A timeout never lifts a project whose + * command failed: one failed project and one timed-out project score 0. */ +export function testExecutionPoints( execution: { passed: number; failed: number; total: number } | null, skipExec: boolean, - testReports: NormalizedTestProjectReport[], + testReports: Pick[], ): number { if (execution) { const passRate = execution.total > 0 ? execution.passed / execution.total : 0; return Math.round(passRate * 20); } - // Partial credit when not run: skipped, or stopped at the time limit. A - // timeout says how fast the machine is, not whether the tests pass. - if (skipExec || testReports.some((report) => report.status === "timeout")) return 10; + if (skipExec) return 10; + const ran = testReports.filter((report) => report.status !== "skipped"); + if (ran.length > 0 && ran.every((report) => report.status === "timeout")) return 10; return 0; // couldn't run tests } From 5530a7c78daf0873a9bc54faa2258ce721838b1d Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 19:41:19 +1100 Subject: [PATCH 07/11] fix(monitor): let the scan outlast the configured test timeout (#106) The monitor killed its scan subprocess after a fixed 120 s, so a checks.testing.settings.timeoutMs above that could never take effect there: the whole scan died first. When tests run, the limit is now 120 s plus the effective test timeout for each test project (capped at 2^31-1 ms); with --skip-tests it stays 120 s. Killing the scan no longer orphans a test run either, since the supervisor sees its parent go. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/monitor.tsx | 17 ++++++++++++++++- src/runners/test-timeout.test.ts | 14 +++++++++++++- src/runners/testing.ts | 11 +++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/monitor.tsx b/src/monitor.tsx index 85fc622..4cd4f45 100644 --- a/src/monitor.tsx +++ b/src/monitor.tsx @@ -13,8 +13,10 @@ import { watch, existsSync, mkdirSync, writeFileSync, readFileSync } from "node: import { execFile, execSync } from "node:child_process"; import { fileURLToPath } from "node:url"; +import { getCheckSettings, loadConfig } from "./config.js"; import { detectStack, detectWorkspace } from "./detect.js"; import { loadHistory } from "./history.js"; +import { resolveTestTimeout, SCAN_BASE_TIMEOUT_MS, scanTimeoutWithTests, testRunTargets } from "./runners/testing.js"; import type { CheckResult } from "./types.js"; const VERSION = JSON.parse(readFileSync(new URL("../package.json", import.meta.url), "utf-8")).version; @@ -163,6 +165,19 @@ function getGitChanges(cwd: string): GitChange[] { // ── Scan via child process — UI never freezes ── +/** The scan subprocess's limit: it must outlast the configured test timeout + * (checks.testing.settings.timeoutMs) for every test project, or the scan is + * killed before that setting can take effect. */ +function scanTimeoutMs(cwd: string, skipTests: boolean): number { + if (skipTests) return SCAN_BASE_TIMEOUT_MS; + try { + const { ms } = resolveTestTimeout({ settings: getCheckSettings(loadConfig(cwd), "testing") }); + return scanTimeoutWithTests(ms, testRunTargets(cwd, undefined, detectWorkspace(cwd)).length); + } catch { + return scanTimeoutWithTests(resolveTestTimeout().ms, 1); + } +} + function runScanProcess( cwd: string, skipTests: boolean, @@ -172,7 +187,7 @@ function runScanProcess( if (skipTests) args.unshift("--skip-tests"); execFile(process.execPath, [CLI_PATH, ...args], { - timeout: 120_000, + timeout: scanTimeoutMs(cwd, skipTests), maxBuffer: 10 * 1024 * 1024, env: { ...process.env, VCQA_NO_UPDATE_CHECK: "1" }, }, (err, stdout) => { diff --git a/src/runners/test-timeout.test.ts b/src/runners/test-timeout.test.ts index 64ba03e..3847c9c 100644 --- a/src/runners/test-timeout.test.ts +++ b/src/runners/test-timeout.test.ts @@ -15,7 +15,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { scan } from "../core.js"; import { detectStack } from "../detect.js"; import type { ToolRun } from "./exec.js"; -import { resolveTestTimeout, runTesting, testExecutionPoints } from "./testing.js"; +import { resolveTestTimeout, runTesting, scanTimeoutWithTests, testExecutionPoints } from "./testing.js"; const FAKE_VITEST = `#!/usr/bin/env node const { spawn } = require("node:child_process"); @@ -223,3 +223,15 @@ describe("timeout scoring across projects (#106)", () => { 30_000, ); }); + +describe("scanTimeoutWithTests (monitor scan limit)", () => { + it("leaves room for the full test limit of every project", () => { + expect(scanTimeoutWithTests(120_000, 1)).toBe(240_000); + expect(scanTimeoutWithTests(600_000, 3)).toBe(120_000 + 1_800_000); + expect(scanTimeoutWithTests(600_000, 0)).toBe(720_000); + }); + + it("never exceeds what a timer can hold", () => { + expect(scanTimeoutWithTests(2 ** 31 - 1, 2)).toBe(2 ** 31 - 1); + }); +}); diff --git a/src/runners/testing.ts b/src/runners/testing.ts index 00701f5..7195460 100644 --- a/src/runners/testing.ts +++ b/src/runners/testing.ts @@ -93,6 +93,17 @@ export function resolveTestTimeout(options: TestingOptions = {}): EffectiveTestT return { ms: DEFAULT_TEST_TIMEOUT_MS, source: "default", ...invalid }; } +/** Base allowance for a whole scan, test runs aside. */ +export const SCAN_BASE_TIMEOUT_MS = 120_000; + +/** Time limit for a whole scan that runs tests (e.g. the monitor's scan + * subprocess): the base allowance plus one full test limit per test project, + * capped at what a timer can hold. A tighter limit would kill the scan before + * a configured test timeout could take effect. */ +export function scanTimeoutWithTests(testTimeoutMs: number, testProjects: number): number { + return Math.min(SCAN_BASE_TIMEOUT_MS + testTimeoutMs * Math.max(1, testProjects), MAX_TIMER_MS); +} + function formatSeconds(ms: number): string { return `${Number((ms / 1000).toFixed(1))} s`; } From 40b1884c08b5f2365c4e360a5addc3979b369c94 Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 19:41:25 +1100 Subject: [PATCH 08/11] docs: list --test-timeout in the README and CLAUDE.md flags (#106) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- CLAUDE.md | 2 +- README.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1193533..9cde598 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -208,7 +208,7 @@ Tries dedicated tools first, falls back to built-in: ## Flags -`--skip-tests`, `--ci`, `--fail-under N`, `--json`, `--badge`, `--sarif`, `--upload`, `--top [N]`, `--diff [base]`, `--watch`, `-v`, `-h` +`--skip-tests`, `--test-timeout MS`, `--ci`, `--fail-under N`, `--json`, `--badge`, `--sarif`, `--upload`, `--top [N]`, `--diff [base]`, `--watch`, `-v`, `-h` ## Testing diff --git a/README.md b/README.md index 4357a9a..79e51a2 100644 --- a/README.md +++ b/README.md @@ -254,6 +254,7 @@ Auto-detects: TypeScript/JavaScript/Dart, React/Vue/Svelte/Flutter, Vite/Webpack | Flag | Description | |------|-------------| | `--skip-tests` | Skip test execution (fast mode) | +| `--test-timeout MS` | Per-project test run limit in ms, 1–2147483647 (default 120000; overrides `checks.testing.settings.timeoutMs` in `.vcqa.json`) | | `--ci` | CI mode (exit 1 if score < 60) | | `--fail-under N` | Exit 1 if score < N | | `--json` | JSON output | From 1037bd3c69f582c98b6815afafc7e0693a9add1a Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 21:34:13 +1100 Subject: [PATCH 09/11] fix(exec): trap SIGQUIT; record signal kills as 128 + signal (#106) Ctrl-\ killed the supervisor by default action before it could signal the test group, leaving the run orphaned. SIGQUIT is now trapped like SIGINT, SIGTERM and SIGHUP (exit 131). A command killed by a signal was recorded as a flat 128; it is now 128 + the signal number (137 for SIGKILL, 143 for SIGTERM), so an OOM kill is distinguishable from a crash. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/runners/exec-tree.test.ts | 19 +++++++++++++++++++ src/runners/exec-tree.ts | 4 ++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/runners/exec-tree.test.ts b/src/runners/exec-tree.test.ts index 5d4f16e..b807503 100644 --- a/src/runners/exec-tree.test.ts +++ b/src/runners/exec-tree.test.ts @@ -138,6 +138,25 @@ describe.skipIf(!posix)("runWithTreeKill", () => { expect(result.marker).not.toContain("vcqa:timed-out"); }); + it("Ctrl-\\ (SIGQUIT to the supervisor) kills the group too", async () => { + const dir = tempDir(); + const { pids } = await startParent(dir, SLEEPER); + process.kill(pids[1], "SIGQUIT"); + expect(await waitFor(() => existsSync(join(dir, "result.json")))).toBe(true); + expect(isAlive(pids[0])).toBe(false); + expect(JSON.parse(readFileSync(join(dir, "result.json"), "utf-8")).status).toBe(131); + }); + + it("records a command killed by a signal as 128 + its signal number", () => { + const dir = tempDir(); + startToolRecording(); + runWithTreeKill("kill -9 $$", dir, 10_000); + runWithTreeKill("kill -TERM $$", dir, 10_000); + const [killed, terminated] = takeToolRuns(); + expect(killed).toMatchObject({ status: "failed", exitCode: 137, ok: false }); + expect(terminated).toMatchObject({ status: "failed", exitCode: 143, ok: false }); + }); + it("kills the group when the parent dies outright", async () => { const dir = tempDir(); const { parent, pids } = await startParent(dir, SLEEPER); diff --git a/src/runners/exec-tree.ts b/src/runners/exec-tree.ts index b25df60..67674fe 100644 --- a/src/runners/exec-tree.ts +++ b/src/runners/exec-tree.ts @@ -87,7 +87,7 @@ const relay = (fd) => (chunk) => { child.stdout.on("data", relay(1)); child.stderr.on("data", relay(2)); const timer = setTimeout(() => stop(124, true), timeoutMs); -for (const [sig, code] of [["SIGINT", 130], ["SIGTERM", 143], ["SIGHUP", 129]]) process.on(sig, () => stop(code, false)); +for (const [sig, code] of [["SIGINT", 130], ["SIGTERM", 143], ["SIGHUP", 129], ["SIGQUIT", 131]]) process.on(sig, () => stop(code, false)); if (!win) { try { const watch = new (require("node:net").Socket)({ fd: 4, readable: true, writable: false }); @@ -109,7 +109,7 @@ child.on("exit", (code, signal) => { clearTimeout(timer); // Grandchildren that outlive the command (a stray watcher) go too. signalTree("SIGKILL"); - const status = code ?? (signal ? 128 : 1); + const status = code ?? (signal ? 128 + (require("node:os").constants.signals[signal] ?? 0) : 1); // Relay what is still buffered, but do not wait on a process outside the // group that holds the pipes open: stop once they close, go quiet, or 2 s pass. const done = () => { report("${EXIT_MARKER}" + status); process.exit(status); }; From 128a7705bbd98d1df5eb6bb891dfad5e45354a21 Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 21:34:13 +1100 Subject: [PATCH 10/11] test(testing): give must-finish runs headroom under load (#106) The single-project timeout test used a 2 s limit and the two-project one 1.5 s; on a loaded machine the fake runner could miss writing its pid file, or the failing project could time out instead of failing. Both use 5 s; the 30 s sleepers still time out. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/runners/test-timeout.test.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/runners/test-timeout.test.ts b/src/runners/test-timeout.test.ts index 3847c9c..7fee031 100644 --- a/src/runners/test-timeout.test.ts +++ b/src/runners/test-timeout.test.ts @@ -90,21 +90,23 @@ describe("test run timeout (#106)", () => { it.skipIf(process.platform === "win32")( "a run past the configured limit is a warning, scores like --skip-tests, reads no coverage and leaves no process", async () => { - const dir = makeFixture(30_000, { timeoutMs: 2000 }); + // 5 s, not less: the fake must start and write pids.json before the limit, + // even on a loaded CI runner. + const dir = makeFixture(30_000, { timeoutMs: 5000 }); const { check, details, toolRuns } = await scanTesting(dir); const run = toolRuns.find((r) => r.tool === "vitest"); - expect(run).toMatchObject({ status: "timeout", timedOut: true, timeoutMs: 2000, ok: false }); - expect(run?.durationMs).toBeLessThan(10_000); + expect(run).toMatchObject({ status: "timeout", timedOut: true, timeoutMs: 5000, ok: false }); + expect(run?.durationMs).toBeLessThan(15_000); - expect(details.testTimeoutMs).toBe(2000); + expect(details.testTimeoutMs).toBe(5000); expect(details.testTimeoutSource).toBe("config"); expect(details.executionStatus).toBe("timeout"); expect(details.testProjects).toHaveLength(1); expect(details.testProjects[0]).toMatchObject({ status: "timeout", - statusLabel: "timed out after 2 s", - timeoutMs: 2000, + statusLabel: "timed out after 5 s", + timeoutMs: 5000, coverageStatus: "not-reported", coverage: null, }); @@ -207,7 +209,9 @@ describe("timeout scoring across projects (#106)", () => { // still read, as before #106, and would differ between the two runs. rmSync(join(dir, "packages/other/coverage"), { recursive: true }); const srcRoots = ["packages/slow/src", "packages/other/src"]; - return runTesting(dir, detectStack(dir), false, srcRoots, undefined, undefined, { timeoutMs: 1500 }); + // One limit for both projects: generous enough that the failing one + // always finishes under load; the 30 s sleeper still times out. + return runTesting(dir, detectStack(dir), false, srcRoots, undefined, undefined, { timeoutMs: 5000 }); }; const bothTimedOut = run(30_000); const oneFailed = run("fail"); @@ -220,7 +224,7 @@ describe("timeout scoring across projects (#106)", () => { // Same static points; only execution differs: 10 when all timed out, 0 here. expect(oneFailed.score).toBe(bothTimedOut.score - 10); }, - 30_000, + 60_000, ); }); From adacab20c0cb8c4c70b37451c216669dd6d5899a Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Tue, 6 Oct 2026 09:08:33 +1100 Subject: [PATCH 11/11] refactor(exec): record tool runs through the shared recorder (#106) toolRunEntry passed a trimmed, capped copy of the output to record(), which now does its own processing and capping. Command and output are handed over whole, so runs recorded by runWithTreeKill are stored exactly like runs recorded by run(). Tests cover a successful run, a timed-out run, and output where a recognised value straddles the 8000-character cap. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- src/runners/exec-tree.test.ts | 49 ++++++++++++++++++++++++++++++++++- src/runners/exec.ts | 7 ++--- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/runners/exec-tree.test.ts b/src/runners/exec-tree.test.ts index b807503..26fcf16 100644 --- a/src/runners/exec-tree.test.ts +++ b/src/runners/exec-tree.test.ts @@ -3,7 +3,7 @@ * supervisor, the parent dying, or the parent giving up on its output. */ import { type ChildProcess, spawn } from "node:child_process"; -import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -165,3 +165,50 @@ describe.skipIf(!posix)("runWithTreeKill", () => { expect(await waitFor(() => !isAlive(pids[1]))).toBe(true); }); }); + +describe.skipIf(!posix)("runWithTreeKill records through the shared recorder", () => { + // A token-shaped value, assembled at runtime so no literal sits in the source. + const alnum = "ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz23456789"; + const body = Array.from({ length: 36 }, (_, i) => alnum[(i * 7 + 3) % alnum.length]).join(""); + const value = ["gh", "p_", body].join(""); + const windows = Array.from({ length: body.length - 4 }, (_, i) => body.slice(i, i + 5)); + const expectAbsent = (text: string) => { + for (const w of windows) expect(text).not.toContain(w); + }; + + it("processes the output of a successful run", () => { + const dir = tempDir(); + writeFileSync(join(dir, "v.txt"), `before ${value} after\n`); + startToolRecording(); + const res = runWithTreeKill("cat v.txt", dir, 10_000); + const [r] = takeToolRuns(); + expect(res.ok).toBe(true); + expect(r).toMatchObject({ status: "success", ok: true }); + expect(r.output).toContain("before"); + expectAbsent(r.output); + expectAbsent(r.command); + }); + + it("processes the output of a timed-out run", () => { + const dir = tempDir(); + writeFileSync(join(dir, "v.txt"), `${value}\n`); + startToolRecording(); + const res = runWithTreeKill("cat v.txt; exec sleep 60", dir, 500); + const [r] = takeToolRuns(); + expect(res.timedOut).toBe(true); + expect(r).toMatchObject({ status: "timeout", timedOut: true }); + expectAbsent(r.output); + }); + + it("processes a value that straddles the output cap", () => { + const dir = tempDir(); + // The value starts 10 characters before the 8000-character cap. + writeFileSync(join(dir, "v.txt"), `${"x".repeat(7990)}${value}${"y".repeat(100)}\n`); + startToolRecording(); + runWithTreeKill("cat v.txt", dir, 10_000); + const [r] = takeToolRuns(); + expect(r.output.length).toBeLessThanOrEqual(8000); + expect(r.output.startsWith("xxxx")).toBe(true); + expectAbsent(r.output); + }); +}); diff --git a/src/runners/exec.ts b/src/runners/exec.ts index fe8fd31..43772ec 100644 --- a/src/runners/exec.ts +++ b/src/runners/exec.ts @@ -151,8 +151,9 @@ export function run(cmd: string, cwd: string, timeout = 60_000, context: ToolRun type ToolRunResult = Pick; -/** The one place a ToolRun entry is built: command, cwd, context, capped - * output and the not-found guess. Every recorded run goes through it. */ +/** The one place a ToolRun entry is built: command, cwd, context and the + * not-found guess. Command and output are passed through whole — `record()` + * processes and caps them, so every recorded run is stored the same way. */ function toolRunEntry(cmd: string, cwd: string, result: ToolRunResult, context: ToolRunContext): ToolRun { return { tool: toolNameOf(cmd), @@ -160,7 +161,7 @@ function toolRunEntry(cmd: string, cwd: string, result: ToolRunResult, context: cwd, ...normalizedContext({ ...defaultContext, ...context }), ...result, - output: result.output.trim().slice(0, MAX_OUTPUT), + output: result.output, notFound: !result.ok && !result.timedOut && /not found|ENOENT|command not found/i.test(result.output), }; }