diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cc6aeb7..5ba1c0e 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index b26fe75..b99629c 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/configuration.md b/docs/configuration.md index 2ea14a7..dc454b5 100644 --- a/docs/configuration.md +++ b/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 diff --git a/src/exec.test.ts b/src/exec.test.ts index 7b8168d..1a1e9f7 100644 --- a/src/exec.test.ts +++ b/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(); + 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) => { + 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; +} diff --git a/src/exec.ts b/src/exec.ts index 11001b8..8ed2fb6 100644 --- a/src/exec.ts +++ b/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(); const abortHandlers = new Map 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 { 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 { 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 { +export async function taskkillTree(pid: number, timeoutMs = taskkillTimeoutMs()): Promise { await new Promise((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(); + }); }); }