Address PR review feedback (#1669)

- watchdog: escalate forwarded SIGINT/SIGTERM/SIGHUP to SIGKILL after grace so
  external cancellation can't hang for the full budget (coderabbit major)
- watchdog: route onProcExit through signalGroup for injection consistency (greptile)
- watchdog: add cwd option; test-changed passes rootDir so pnpm runs from repo
  root regardless of invocation cwd (coderabbit major — preserved original run() cwd)
- dashboard runner: validate/clamp FUSION_RUN_VITEST_* env so a malformed value
  can't NaN-disable the watchdog (coderabbit)
- tests: verify exit-listener cleanup, forwarded-signal escalation, cwd passthrough
- plan doc: per-class-ceiling fallback wording (not median); label Output Structure fence

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs/plans/2026-06-13-001-fix-test-timeout-failures-plan.md

@@ -128,7 +128,7 @@ flowchart TD
 
 New/changed shared infrastructure (illustrative — per-unit `Files` lists are authoritative):
 
-```
+```text
 scripts/
   lib/
     run-vitest-watchdog.mjs        # NEW (U1) — shared bounded-invocation runner + process-group killer;
@@ -170,7 +170,7 @@ scripts/lib/test-quarantine.json                # MODIFIED (U3) — rescue/delet
 - `.github/workflows/full-suite.yml` (modify) — add `timeout-minutes` to `test-shards`, `test-slow`, `test-inventory-guard`.
 - `scripts/__tests__/run-vitest-watchdog.test.mjs` (new).
 
-**Approach:** Extract the dashboard killer's process-group lifecycle into the shared async helper, parameterized by command, env, heap flag, and budget. Budget = `max(perClassFloor, min(perClassCeiling, expectedDurationMs × multiplier))` per KTD-2 — the per-class floor/ceiling (shard / changed-file / dashboard-lane) are the safety net; the timings term (aggregated across all packages in a multi-package `plain` command, median fallback when absent, multiplier 3-4×) only tightens within the band, and only when the snapshot is fresh. **Refresh `test-timings.json` before deriving budgets.** CI `timeout-minutes` must exceed the worst-case L2 ceiling so L2 always fires first; document the ordering in a comment. Forwards external signals; cleans up on exit/SIGINT/SIGTERM like the existing runners. Note the two runners import each other and are imported by tests — verify the async conversion doesn't break any synchronous-import caller.
+**Approach:** Extract the dashboard killer's process-group lifecycle into the shared async helper, parameterized by command, env, heap flag, and budget. Budget = `max(perClassFloor, min(perClassCeiling, expectedDurationMs × multiplier))` per KTD-2 — the per-class floor/ceiling (shard / changed-file / dashboard-lane) are the safety net; the timings term (aggregated across all packages in a multi-package `plain` command, multiplier 3-4×) only tightens within the band, and only when the snapshot is fresh; when timings are absent or stale, `deriveBudgetMs` falls back to the per-class **ceiling** (never a median). **Refresh `test-timings.json` before deriving budgets.** CI `timeout-minutes` must exceed the worst-case L2 ceiling so L2 always fires first; document the ordering in a comment. Forwards external signals; cleans up on exit/SIGINT/SIGTERM like the existing runners. Note the two runners import each other and are imported by tests — verify the async conversion doesn't break any synchronous-import caller.
 
 **Execution note:** Start with a failing test for the watchdog contract (spawns a deliberately-hanging child, asserts `SIGTERM`-then-`SIGKILL` and exit 124 within budget) before extracting the helper.
 

packages/dashboard/scripts/run-vitest-with-heap.mjs

@@ -18,8 +18,21 @@ if (vitestArgs.length === 0) {
 const nodeOptions = [`--max-old-space-size=${heapMb}`, process.env.NODE_OPTIONS || ""]
   .join(" ")
   .trim();
-const timeoutMs = Number.parseInt(process.env.FUSION_RUN_VITEST_TIMEOUT_MS || "900000", 10);
-const graceMs = Number.parseInt(process.env.FUSION_RUN_VITEST_KILL_GRACE_MS || "5000", 10);
+// Clamp to the default on a missing/malformed value. A bad env value must never
+// produce NaN — the watchdog only arms when budgetMs is finite and > 0, so a
+// NaN here would silently disable the killer and bring back the very hang this
+// wrapper exists to prevent.
+function positiveIntEnv(name, fallback) {
+  const raw = process.env[name];
+  if (raw == null || raw === "") return fallback;
+  const parsed = Number.parseInt(raw, 10);
+  if (Number.isInteger(parsed) && parsed > 0) return parsed;
+  console.error(`[dashboard-vitest] ignoring invalid ${name}=${JSON.stringify(raw)}; using ${fallback}`);
+  return fallback;
+}
+
+const timeoutMs = positiveIntEnv("FUSION_RUN_VITEST_TIMEOUT_MS", 900000);
+const graceMs = positiveIntEnv("FUSION_RUN_VITEST_KILL_GRACE_MS", 5000);
 
 function resolveSpawnCommand() {
   const override = process.env.FUSION_RUN_VITEST_SPAWN_OVERRIDE;

scripts/__tests__/run-vitest-watchdog.test.mjs

@@ -166,7 +166,8 @@ test("runWithWatchdog: child error rejects", async () => {
 });
 
 test("runWithWatchdog: removes its process listeners after settling", async () => {
-  const before = process.listenerCount("SIGTERM");
+  const beforeTerm = process.listenerCount("SIGTERM");
+  const beforeExit = process.listenerCount("exit");
   const child = makeFakeChild();
   const p = runWithWatchdog({
     command: "fake",
@@ -179,5 +180,52 @@ test("runWithWatchdog: removes its process listeners after settling", async () =
   });
   child.emit("close", 0, null);
   await p;
-  assert.equal(process.listenerCount("SIGTERM"), before);
+  assert.equal(process.listenerCount("SIGTERM"), beforeTerm);
+  assert.equal(process.listenerCount("exit"), beforeExit);
+});
+
+test("runWithWatchdog: forwarded signal escalates to SIGKILL after grace", async () => {
+  const child = makeFakeChild();
+  const killed = [];
+  const p = runWithWatchdog({
+    command: "pnpm",
+    args: [],
+    budgetMs: 10_000,
+    graceMs: 15,
+    heartbeatMs: 1000,
+    label: "cancel",
+    log: () => {},
+    spawn: fakeSpawn(child),
+    killGroup: (sig) => {
+      killed.push(sig);
+      // The child ignores SIGHUP; only SIGKILL takes it down.
+      if (sig === "SIGKILL") child.emit("close", null, "SIGKILL");
+    },
+  });
+  // Simulate external cancellation (Ctrl-C / CI cancel) reaching the wrapper.
+  process.emit("SIGHUP");
+  await new Promise((resolve) => setTimeout(resolve, 50));
+  await p;
+  assert.deepEqual(killed, ["SIGHUP", "SIGKILL"]);
+});
+
+test("runWithWatchdog: passes cwd through to spawn when provided", async () => {
+  let capturedOpts = null;
+  const child = makeFakeChild();
+  const p = runWithWatchdog({
+    command: "pnpm",
+    args: ["test"],
+    cwd: "/tmp/repo-root",
+    budgetMs: 10_000,
+    label: "cwd",
+    log: () => {},
+    spawn: (_cmd, _args, opts) => {
+      capturedOpts = opts;
+      return child;
+    },
+    killGroup: () => {},
+  });
+  child.emit("close", 0, null);
+  await p;
+  assert.equal(capturedOpts.cwd, "/tmp/repo-root");
 });

scripts/lib/run-vitest-watchdog.mjs

@@ -139,6 +139,8 @@ export function captureHangDiagnostics({ label, command, args, budgetMs, started
  * @param {string} [opts.label]
  * @param {(msg: string) => void} [opts.log]
  * @param {object} opts.spawn      injected spawn (node:child_process spawn); required for testability
+ * @param {string} [opts.cwd]       working directory for the spawned child (preserves callers that
+ *   ran the test command from a fixed root, e.g. test-changed.mjs's rootDir)
  * @param {() => number} [opts.now] injected clock (defaults to Date.now)
  * @param {(signal: string) => void} [opts.killGroup] injected group-signaller
  *   (defaults to a process-group `process.kill(-pid)` with child.kill fallback);
@@ -148,6 +150,7 @@ export function runWithWatchdog({
   command,
   args,
   env = process.env,
+  cwd = null,
   budgetMs,
   graceMs = DEFAULT_GRACE_MS,
   heartbeatMs = DEFAULT_HEARTBEAT_MS,
@@ -171,7 +174,12 @@ export function runWithWatchdog({
 
     // process-supervisor-allowlist: foreground wrapper signals the whole vitest
     // process group on death/timeout; not a background daemon.
-    const child = spawn(command, args, { detached: true, stdio: "inherit", env });
+    const child = spawn(command, args, {
+      detached: true,
+      stdio: "inherit",
+      env,
+      ...(cwd ? { cwd } : {}),
+    });
 
     const heartbeat = setInterval(() => {
       lastHeartbeatAt = now();
@@ -195,6 +203,20 @@ export function runWithWatchdog({
     }
     const signalGroup = typeof killGroup === "function" ? killGroup : defaultSignalGroup;
 
+    // Arm the SIGTERM→SIGKILL grace ladder once. Used by BOTH the timeout path
+    // and external-cancellation forwarding so a child that ignores SIGTERM can't
+    // keep the wrapper pending until the full budget (the original handlers
+    // suppressed Node's default exit behavior, so Ctrl-C / CI cancellation could
+    // otherwise hang for the whole per-command ceiling).
+    function armForceKill(triggerSignal) {
+      if (forceKillTimer) return;
+      forceKillTimer = setTimeout(() => {
+        log(`[watchdog] grace expired after ${triggerSignal}; SIGKILL: ${label}`);
+        signalGroup("SIGKILL");
+      }, Math.max(1, graceMs));
+      forceKillTimer.unref?.();
+    }
+
     const watchdog =
       Number.isFinite(budgetMs) && budgetMs > 0
         ? setTimeout(() => {
@@ -210,11 +232,7 @@ export function runWithWatchdog({
             });
             log(diagnostics);
             signalGroup("SIGTERM");
-            forceKillTimer = setTimeout(() => {
-              log(`[watchdog] grace expired; SIGKILL: ${label}`);
-              signalGroup("SIGKILL");
-            }, Math.max(1, graceMs));
-            forceKillTimer.unref?.();
+            armForceKill("timeout");
           }, budgetMs)
         : null;
     watchdog?.unref?.();
@@ -225,6 +243,7 @@ export function runWithWatchdog({
       const handler = () => {
         log(`[watchdog] received ${sig}; forwarding to group: ${label}`);
         signalGroup(sig);
+        armForceKill(sig);
       };
       signalHandlers.set(sig, handler);
       process.on(sig, handler);
@@ -232,8 +251,9 @@ export function runWithWatchdog({
 
     function onProcExit() {
       // Best-effort: don't leave an orphaned group if the wrapper itself dies.
+      // Route through signalGroup so the injection contract holds everywhere.
       try {
-        process.kill(-child.pid, "SIGTERM");
+        signalGroup("SIGTERM");
       } catch {
         /* group already gone */
       }

scripts/test-changed.mjs

@@ -213,6 +213,9 @@ async function runWatchedTest(command, commandArgs, { env, budgetMs, label } = {
     command,
     args: commandArgs,
     env: env ?? process.env,
+    // Preserve the original `run`'s fixed working directory; pnpm must execute
+    // from the repo root regardless of where test-changed was invoked.
+    cwd: rootDir,
     budgetMs,
     label: label ?? `${command} ${commandArgs.join(" ")}`,
     log: console.error,