fix(exec): timeout Windows taskkill on the timeout path

.github/workflows/ci.yml

@@ -26,3 +26,15 @@ jobs:
       - run: pnpm test
       - run: pnpm build
       - run: pnpm pack:smoke
+
+  windows-exec:
+    runs-on: windows-latest
+    steps:
+      - uses: actions/checkout@v7.0.1
+      - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6.0.10
+      - uses: actions/setup-node@v7
+        with:
+          node-version: 26
+          cache: pnpm
+      - run: pnpm install --frozen-lockfile
+      - run: pnpm test src/exec.test.ts

CHANGELOG.md

@@ -4,6 +4,8 @@
 
 - Updated pnpm, Node typings, formatter and linter tooling, Vitest/Vite, CodeQL, TruffleHog, and the release workflow's npm CLI.
 - Fixed `clawpatch open-pr` so a stalled `git push` or `gh pr create` times out instead of hanging the command, thanks @SebTardif.
+- Fixed Windows command timeouts so a hung `taskkill` cannot keep the CLI or its direct child running after the cleanup deadline, thanks @SebTardif.
+- Fixed Windows shell validation commands with quoted executable paths.
 - Bound npm trusted publishing to the `npm-release` GitHub environment and restored canonical package repository metadata.
 - Reworked the README around a verified install and quickstart path, with deeper command, mapper, provider, and safety details linked to the existing docs.
 - Updated transitive Vitest dependencies.

docs/configuration.md

@@ -75,10 +75,17 @@ Environment overrides:
 - `CLAWPATCH_CLAUDE_AUTH_CONTEXT` (`isolated` or `host`; default `isolated`)
 - `CLAWPATCH_GIT_PUSH_TIMEOUT_MS` (default `600000`, or 10 minutes)
 - `CLAWPATCH_GH_PR_CREATE_TIMEOUT_MS` (default `300000`, or 5 minutes)
+- `CLAWPATCH_TASKKILL_TIMEOUT_MS` (Windows cleanup deadline; default `5000`, or 5 seconds)
 
 The `open-pr` timeout overrides must be positive millisecond values. Invalid values fall back to
 their defaults.
 
+`CLAWPATCH_TASKKILL_TIMEOUT_MS` must be between `1` and `2147483647` milliseconds;
+invalid values fall back to 5 seconds. Fractional values are truncated. Each Windows
+process-tree cleanup attempt is bounded independently of the command deadline.
+If cleanup fails or times out, Clawpatch also terminates the direct child; descendant
+cleanup remains best effort when `taskkill` is unavailable or hung.
+
 `provider.codexConfig` passes primitive values to Codex as `-c key=value`.
 Only config loaded by `--config` or `CLAWPATCH_CONFIG` may set non-empty
 Codex passthrough config. Auto-discovered repository and state config files

src/exec.test.ts

@@ -1,8 +1,10 @@
-import { access, mkdtemp, writeFile } from "node:fs/promises";
+import childProcess, { spawn, type ChildProcess } from "node:child_process";
+import { access, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
-import { describe, expect, it } from "vitest";
-import { runCommand, runCommandArgs } from "./exec.js";
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { runCommand, runCommandArgs, taskkillTimeoutMs, taskkillTree } from "./exec.js";
+import { shellQuotePath } from "./shell.js";
 
 describe("runCommand", () => {
   it("runs a shell command and passes stdin", async () => {
@@ -15,7 +17,7 @@ describe("runCommand", () => {
     );
 
     const result = await runCommand(
-      `${JSON.stringify(process.execPath)} ${JSON.stringify(script)}`,
+      `${shellQuotePath(process.execPath)} ${shellQuotePath(script)}`,
       dir,
       "ok",
     );
@@ -28,7 +30,7 @@ describe("runCommand", () => {
     const dir = await mkdtemp(join(tmpdir(), "clawpatch-exec-shell-"));
     const script = join(dir, "large-output.mjs");
     await writeFile(script, "process.stdout.write('x'.repeat(9000));", "utf8");
-    const command = `${JSON.stringify(process.execPath)} ${JSON.stringify(script)}`;
+    const command = `${shellQuotePath(process.execPath)} ${shellQuotePath(script)}`;
 
     const trimmed = await runCommand(command, dir);
     const raw = await runCommand(command, dir, undefined, { trimOutput: false });
@@ -45,13 +47,13 @@ describe("runCommand", () => {
     await writeFile(hanging, "setInterval(() => {}, 1000);", "utf8");
 
     const bounded = await runCommand(
-      `${JSON.stringify(process.execPath)} ${JSON.stringify(noisy)}`,
+      `${shellQuotePath(process.execPath)} ${shellQuotePath(noisy)}`,
       dir,
       undefined,
       { trimOutput: false, maxOutputChars: 10_000 },
     );
     const timedOut = await runCommand(
-      `${JSON.stringify(process.execPath)} ${JSON.stringify(hanging)}`,
+      `${shellQuotePath(process.execPath)} ${shellQuotePath(hanging)}`,
       dir,
       undefined,
       { timeoutMs: 50 },
@@ -220,3 +222,113 @@ describe("runCommandArgs", () => {
     expect(JSON.parse(result.stdout)).toEqual(args);
   });
 });
+
+vi.mock("node:child_process", async (importOriginal) => {
+  const original = await importOriginal<typeof import("node:child_process")>();
+  return { ...original, spawn: vi.fn(original.spawn) };
+});
+
+const cleanupTimeoutMs = 1_000;
+
+describe("taskkillTree", () => {
+  afterEach(() => {
+    vi.unstubAllEnvs();
+    vi.mocked(spawn).mockImplementation(childProcess.spawn);
+  });
+
+  it("defaults to 5s and accepts supported millisecond overrides", () => {
+    vi.stubEnv("CLAWPATCH_TASKKILL_TIMEOUT_MS", undefined);
+    expect(taskkillTimeoutMs()).toBe(5_000);
+    vi.stubEnv("CLAWPATCH_TASKKILL_TIMEOUT_MS", "1234");
+    expect(taskkillTimeoutMs()).toBe(1_234);
+    vi.stubEnv("CLAWPATCH_TASKKILL_TIMEOUT_MS", "2147483647");
+    expect(taskkillTimeoutMs()).toBe(2_147_483_647);
+  });
+
+  it.each(["invalid", "", "0", "-1", "0.5", "Infinity", "2147483648"])(
+    "rejects unsupported timeout override %s",
+    (value) => {
+      vi.stubEnv("CLAWPATCH_TASKKILL_TIMEOUT_MS", value);
+      expect(taskkillTimeoutMs()).toBe(5_000);
+    },
+  );
+
+  it("bounds a verified hanging cleanup process", async () => {
+    const root = await mkdtemp(join(tmpdir(), "clawpatch-taskkill-"));
+    const marker = join(root, "killer.json");
+    const children = interceptTaskkill(marker);
+    try {
+      await taskkillTree(42_424, cleanupTimeoutMs);
+      expect(JSON.parse(await readFile(marker, "utf8"))).toEqual(["/pid", "42424", "/T", "/F"]);
+      expect(children).toHaveLength(1);
+      await expect
+        .poll(() => children[0]?.exitCode !== null || children[0]?.signalCode !== null)
+        .toBe(true);
+    } finally {
+      for (const child of children) child.kill("SIGKILL");
+      await rm(root, { recursive: true, force: true });
+    }
+  });
+
+  it.runIf(process.platform === "win32")(
+    "returns a timeout and kills the original child when taskkill hangs",
+    async () => {
+      const root = await mkdtemp(join(tmpdir(), "clawpatch-taskkill-caller-"));
+      const marker = join(root, "killer.json");
+      const children = interceptTaskkill(marker);
+      vi.stubEnv("CLAWPATCH_TASKKILL_TIMEOUT_MS", String(cleanupTimeoutMs));
+      let pid: number | undefined;
+      try {
+        const result = await runCommandArgs(
+          process.execPath,
+          ["-e", "console.log(process.pid); setInterval(() => {}, 1000)"],
+          root,
+          undefined,
+          { timeoutMs: 1_000 },
+        );
+        pid = Number(result.stdout.trim());
+        expect(pid).toBeGreaterThan(0);
+        expect(result.exitCode).toBe(124);
+        expect(result.stderr).toContain("command timed out after 1000ms");
+        expect(JSON.parse(await readFile(marker, "utf8"))).toEqual([
+          "/pid",
+          String(pid),
+          "/T",
+          "/F",
+        ]);
+        expect(children.length).toBeGreaterThan(0);
+        expect(() => process.kill(pid!, 0)).toThrow();
+      } finally {
+        if (pid) {
+          try {
+            process.kill(pid, "SIGKILL");
+          } catch {}
+        }
+        for (const child of children) child.kill("SIGKILL");
+        await rm(root, { recursive: true, force: true });
+      }
+    },
+  );
+});
+
+function interceptTaskkill(marker: string): ChildProcess[] {
+  const realSpawn = childProcess.spawn;
+  const children: ChildProcess[] = [];
+  vi.mocked(spawn).mockImplementation(((...params: Parameters<typeof spawn>) => {
+    const [program, args = [], options = {}] = params;
+    if (program !== "taskkill") return realSpawn(program, args, options);
+    const child = realSpawn(
+      process.execPath,
+      [
+        "-e",
+        "require('node:fs').writeFileSync(process.argv[1], JSON.stringify(process.argv.slice(2))); setInterval(() => {}, 1000)",
+        marker,
+        ...args,
+      ],
+      options,
+    );
+    children.push(child);
+    return child;
+  }) as typeof childProcess.spawn);
+  return children;
+}

src/exec.ts

@@ -10,11 +10,22 @@ type CommandOptions = {
   timeoutMs?: number;
   replaceEnv?: boolean;
   maxOutputChars?: number;
+  windowsVerbatimArguments?: boolean;
 };
 
 const abortSignals: NodeJS.Signals[] = ["SIGINT", "SIGTERM", "SIGHUP"];
 const abortableChildren = new Set<SpawnedChild>();
 const abortHandlers = new Map<NodeJS.Signals, () => void>();
+const defaultTaskkillTimeoutMs = 5_000;
+
+export function taskkillTimeoutMs(): number {
+  const configured = Number(
+    process.env["CLAWPATCH_TASKKILL_TIMEOUT_MS"] ?? String(defaultTaskkillTimeoutMs),
+  );
+  return Number.isFinite(configured) && configured >= 1 && configured <= 2_147_483_647
+    ? Math.trunc(configured)
+    : defaultTaskkillTimeoutMs;
+}
 
 export async function runCommand(
   command: string,
@@ -32,8 +43,13 @@ export async function runCommandRaw(
   options: CommandOptions = {},
 ): Promise<CommandResult> {
   const shell = process.platform === "win32" ? (process.env["ComSpec"] ?? "cmd.exe") : "/bin/sh";
-  const args = process.platform === "win32" ? ["/d", "/s", "/c", command] : ["-c", command];
-  const result = await runCommandArgs(shell, args, cwd, input, options);
+  const windows = process.platform === "win32";
+  // cmd.exe owns shell quoting; Node's executable argument escaping breaks quoted paths.
+  const args = windows ? ["/d", "/s", "/c", `"${command}"`] : ["-c", command];
+  const result = await runCommandArgs(shell, args, cwd, input, {
+    ...options,
+    windowsVerbatimArguments: windows,
+  });
   return { ...result, command };
 }
 
@@ -57,7 +73,8 @@ export async function runCommandArgs(
     detached: process.platform !== "win32" && options.timeoutMs !== undefined,
     shell: false,
     stdio: ["pipe", "pipe", "pipe"],
-    windowsVerbatimArguments: spawnSpec.windowsVerbatimArguments,
+    windowsVerbatimArguments:
+      options.windowsVerbatimArguments ?? spawnSpec.windowsVerbatimArguments,
   });
   const stdout = new OutputBuffer(options.maxOutputChars);
   const stderr = new OutputBuffer(options.maxOutputChars);
@@ -144,6 +161,10 @@ function terminateChild(child: SpawnedChild, onForceKill: () => void): NodeJS.Ti
 async function killChild(child: SpawnedChild, signal: NodeJS.Signals): Promise<void> {
   if (process.platform === "win32" && child.pid !== undefined) {
     await taskkillTree(child.pid);
+    // A failed or hung tree killer must not leave the direct child keeping the CLI alive.
+    try {
+      child.kill(signal);
+    } catch {}
     return;
   }
   try {
@@ -157,14 +178,33 @@ async function killChild(child: SpawnedChild, signal: NodeJS.Signals): Promise<v
   } catch {}
 }
 
-async function taskkillTree(pid: number): Promise<void> {
+export async function taskkillTree(pid: number, timeoutMs = taskkillTimeoutMs()): Promise<void> {
   await new Promise<void>((resolve) => {
     const killer = spawn("taskkill", ["/pid", String(pid), "/T", "/F"], {
       stdio: "ignore",
       windowsHide: true,
     });
-    killer.on("error", () => resolve());
-    killer.on("close", () => resolve());
+    let settled = false;
+    const finish = (): void => {
+      if (settled) {
+        return;
+      }
+      settled = true;
+      clearTimeout(timeout);
+      resolve();
+    };
+    const timeout = setTimeout(() => {
+      try {
+        killer.kill("SIGKILL");
+      } catch {}
+      finish();
+    }, timeoutMs);
+    killer.on("error", () => {
+      finish();
+    });
+    killer.on("close", () => {
+      finish();
+    });
   });
 }