fix(subprocess): harden process spawning on Windows (MSYS paths + process-tree kill)

src/core/subprocess.ts

@@ -6,6 +6,8 @@
  * (~64 KB on macOS) while the parent blocks on `proc.exited`.
  */
 
+import { existsSync } from "node:fs"
+
 export interface SubprocessResult {
   exitCode: number
   stdout: string
@@ -26,6 +28,53 @@ export interface SubprocessOptions {
   env?: Record<string, string | undefined>
 }
 
+/**
+ * On Windows under Git Bash / MSYS, `which <name>` returns MSYS drive paths
+ * (`/d/...`) that `Bun.spawn` cannot resolve (it expects `D:\\...` or `D:/...`).
+ * Adapter `tierGlobal` resolvers feed that path straight back as `cmd[0]`,
+ * producing `ENOENT uv_spawn`. Convert MSYS drive paths to Windows paths and,
+ * if the bare path doesn't exist, try `.exe` (e.g. `pi` → `pi.exe`, the
+ * bun-compiled binary in `node_modules/.bin`). No-op on non-win32 and for bare
+ * command names (`bash`, `docker`, ...) that Bun resolves via PATH.
+ */
+function resolveCmd0ForSpawn(cmd0: string): string {
+  if (process.platform !== "win32") return cmd0
+  const m = /^\/([a-zA-Z])\/(.*)$/.exec(cmd0)
+  if (!m) return cmd0
+  const win = `${m[1]!.toUpperCase()}:/${m[2]}`
+  if (existsSync(win)) return win
+  if (existsSync(win + ".exe")) return win + ".exe"
+  return win
+}
+
+/**
+ * Forcibly kill a process AND its descendants. Adapter wrappers (pi.exe,
+ * opencode) spawn grandchildren that survive a plain `proc.kill()` (SIGTERM
+ * to the wrapper only) and keep the stdout pipe open, so `proc.exited` never
+ * resolves and timeouts never fire. On Windows `taskkill /T /F <pid>` takes
+ * down the whole tree; elsewhere we try SIGKILL on the process group.
+ * Synchronous + best-effort: by the time a timeout fires we want the process
+ * gone, not a graceful shutdown that might itself hang.
+ */
+function killProcessTree(pid: number): void {
+  try {
+    if (process.platform === "win32") {
+      // /T = tree, /F = force. Shell not needed; Bun.spawn resolves taskkill
+      // via PATH. Ignore exit code — the process may already be exiting.
+      const result = Bun.spawnSync(["taskkill", "/PID", String(pid), "/T", "/F"], {
+        stdout: "ignore", stderr: "ignore",
+      })
+      if (result.exitCode !== 0) throw new Error(`taskkill exited ${result.exitCode}`)
+    } else {
+      process.kill(-pid, "SIGKILL")
+    }
+  } catch {
+    // Best-effort. If the group kill fails (e.g. not a group leader), fall
+    // back to a direct SIGKILL on the pid itself.
+    try { process.kill(pid, "SIGKILL") } catch { /* already gone */ }
+  }
+}
+
 export async function runSubprocess(
   cmd: string[],
   opts?: SubprocessOptions,
@@ -34,26 +83,66 @@ export async function runSubprocess(
     ? mergeEnv(process.env, opts.env)
     : process.env
   const start = Date.now()
-  const proc = Bun.spawn(cmd, {
+  const spawnCmd = cmd.length > 0
+    ? [resolveCmd0ForSpawn(cmd[0]!), ...cmd.slice(1)]
+    : cmd
+  const proc = Bun.spawn(spawnCmd, {
     cwd: opts?.cwd,
     stdout: "pipe",
     stderr: "pipe",
     env,
+    // A negative-pid kill reaches descendants only when the child leads its
+    // own process group. Windows uses taskkill /T instead.
+    detached: process.platform !== "win32",
   })
 
   let timedOut = false
   let timer: ReturnType<typeof setTimeout> | undefined
+  // Readers are kept so a timeout can cancel them. Without cancellation,
+  // `Response(stream).text()` blocks until EOF — and on Windows/MSYS a
+  // killed wrapper's grandchild (e.g. `sleep` spawned by bash) can keep the
+  // stdout pipe open, so the timeout fires the kill but runSubprocess never
+  // returns. Cancelling the reader lets us return promptly after the kill.
+  const stdoutReader = proc.stdout.getReader()
+  const stderrReader = proc.stderr.getReader()
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  const readAll = async (reader: any): Promise<string> => {
+    const decoder = new TextDecoder()
+    let output = ""
+    try {
+      for (;;) {
+        const { done, value } = await reader.read()
+        if (done) break
+        if (value) output += decoder.decode(value, { stream: true })
+      }
+    } catch {
+      // reader cancelled (timeout) — return what we have so far
+    } finally {
+      output += decoder.decode()
+    }
+    return output
+  }
   if (opts?.timeoutMs) {
     timer = setTimeout(() => {
       timedOut = true
-      proc.kill()
+      // Kill the whole process tree, not just the direct child. Adapter
+      // targets (pi.exe, opencode) are wrappers that spawn grandchildren
+      // (node, bash, the LLM agent loop); proc.kill() only signals the
+      // wrapper, which (a) may ignore SIGTERM and (b) leaves grandchildren
+      // holding the stdout pipe — so proc.exited never resolves and the
+      // timeout never actually fires. On Windows use taskkill /T /F to
+      // forcibly take down the tree; elsewhere SIGKILL the group. Then cancel
+      // the pipe readers so this function returns promptly with partial output.
+      killProcessTree(proc.pid)
+      stdoutReader.cancel().catch(() => {})
+      stderrReader.cancel().catch(() => {})
     }, opts.timeoutMs)
   }
 
   const [exitCode, stdout, stderr] = await Promise.all([
     proc.exited.then((code) => { if (timer) clearTimeout(timer); return code }),
-    new Response(proc.stdout).text(),
-    new Response(proc.stderr).text(),
+    readAll(stdoutReader),
+    readAll(stderrReader),
   ])
   return { exitCode, stdout, stderr, durationMs: Date.now() - start, timedOut }
 }

test/core/subprocess.test.ts

@@ -1,44 +1,60 @@
 import { test, expect, describe } from "bun:test"
 import { runSubprocess } from "../../src/core/subprocess.ts"
 
+function bunEval(source: string, executable = process.execPath): string[] {
+  return [executable, "-e", source]
+}
+
 describe("runSubprocess: exit + output", () => {
   test("captures stdout/stderr and exit code 0 on success", async () => {
-    const r = await runSubprocess(["sh", "-c", "echo out; echo err >&2"])
+    const r = await runSubprocess(bunEval(
+      'process.stdout.write("out\\n"); process.stderr.write("err\\n")',
+    ))
     expect(r.exitCode).toBe(0)
     expect(r.stdout.trim()).toBe("out")
     expect(r.stderr.trim()).toBe("err")
     expect(r.timedOut).toBe(false)
   })
 
   test("propagates a non-zero exit code", async () => {
-    const r = await runSubprocess(["sh", "-c", "exit 3"])
+    const r = await runSubprocess(bunEval("process.exit(3)"))
     expect(r.exitCode).toBe(3)
     expect(r.timedOut).toBe(false)
   })
 
   test("reports a plausible durationMs", async () => {
-    const r = await runSubprocess(["sh", "-c", "sleep 0.1"])
+    const r = await runSubprocess(bunEval("await Bun.sleep(100)"))
     expect(r.durationMs).toBeGreaterThanOrEqual(50)
   })
 
   test("drains output larger than the OS pipe buffer without deadlock", async () => {
     // ~256 KB of stdout; without concurrent draining the child blocks on a
     // full pipe (~64 KB on macOS) while the parent waits on proc.exited.
-    const r = await runSubprocess(["sh", "-c", 'head -c 262144 /dev/zero | tr "\\0" a'])
+    const r = await runSubprocess(bunEval('process.stdout.write("a".repeat(262144))'))
     expect(r.exitCode).toBe(0)
     expect(r.stdout.length).toBe(262144)
   })
+
+  test("decodes a UTF-8 character split across stdout chunks", async () => {
+    const r = await runSubprocess(bunEval([
+      "process.stdout.write(new Uint8Array([0xe4]))",
+      "await Bun.sleep(50)",
+      "process.stdout.write(new Uint8Array([0xbd, 0xa0]))",
+    ].join("; ")))
+
+    expect(r.stdout).toBe("你")
+  })
 })
 
 describe("runSubprocess: timeout", () => {
   test("returns timedOut=true when the subprocess is killed by the timer", async () => {
-    const r = await runSubprocess(["sleep", "10"], { timeoutMs: 200 })
+    const r = await runSubprocess(bunEval("await Bun.sleep(10_000)"), { timeoutMs: 200 })
     expect(r.timedOut).toBe(true)
     expect(r.exitCode).not.toBe(0)
   })
 
   test("returns timedOut=false on natural completion", async () => {
-    const r = await runSubprocess(["sh", "-c", "echo ok"], { timeoutMs: 5000 })
+    const r = await runSubprocess(bunEval('process.stdout.write("ok\\n")'), { timeoutMs: 5000 })
     expect(r.timedOut).toBe(false)
     expect(r.exitCode).toBe(0)
     expect(r.stdout.trim()).toBe("ok")
@@ -47,10 +63,12 @@ describe("runSubprocess: timeout", () => {
 
 describe("runSubprocess: env overlay", () => {
   test("merges the overlay over process.env", async () => {
-    const r = await runSubprocess(["sh", "-c", 'echo "$SKVM_SUBPROC_TEST:$HOME"'], {
+    const r = await runSubprocess(bunEval(
+      'process.stdout.write(JSON.stringify([process.env.SKVM_SUBPROC_TEST, process.env.HOME ?? ""]))',
+    ), {
       env: { SKVM_SUBPROC_TEST: "overlay-value" },
     })
-    const [overlaid, home] = r.stdout.trim().split(":")
+    const [overlaid, home] = JSON.parse(r.stdout) as [string, string]
     expect(overlaid).toBe("overlay-value")
     // Inherited variables survive the merge.
     expect(home).toBe(process.env.HOME ?? "")
@@ -59,7 +77,9 @@ describe("runSubprocess: env overlay", () => {
   test("an undefined overlay value removes the variable from the child env", async () => {
     process.env.SKVM_SUBPROC_DELETED = "should-not-survive"
     try {
-      const r = await runSubprocess(["sh", "-c", 'echo "${SKVM_SUBPROC_DELETED:-unset}"'], {
+      const r = await runSubprocess(bunEval(
+        'process.stdout.write(process.env.SKVM_SUBPROC_DELETED ?? "unset")',
+      ), {
         env: { SKVM_SUBPROC_DELETED: undefined, SKVM_SUBPROC_KEEP: "1" },
       })
       expect(r.stdout.trim()).toBe("unset")
@@ -71,10 +91,49 @@ describe("runSubprocess: env overlay", () => {
   test("no env option inherits process.env unchanged", async () => {
     process.env.SKVM_SUBPROC_INHERIT = "inherited"
     try {
-      const r = await runSubprocess(["sh", "-c", 'echo "$SKVM_SUBPROC_INHERIT"'])
+      const r = await runSubprocess(bunEval(
+        'process.stdout.write(process.env.SKVM_SUBPROC_INHERIT ?? "")',
+      ))
       expect(r.stdout.trim()).toBe("inherited")
     } finally {
       delete process.env.SKVM_SUBPROC_INHERIT
     }
   })
 })
+
+const windowsTest = process.platform === "win32" ? test : test.skip
+
+describe("runSubprocess: Windows process handling", () => {
+  windowsTest("resolves an MSYS drive path and its implicit .exe suffix", async () => {
+    const withoutExe = process.execPath.replace(/\.exe$/i, "")
+    const normalized = withoutExe.replace(/\\/g, "/")
+    const match = /^([a-zA-Z]):\/(.*)$/.exec(normalized)
+    expect(match).not.toBeNull()
+    const msysPath = `/${match![1]!.toLowerCase()}/${match![2]}`
+
+    const r = await runSubprocess(bunEval(
+      'process.stdout.write("msys-ok")',
+      msysPath,
+    ))
+
+    expect(r.exitCode).toBe(0)
+    expect(r.stdout).toBe("msys-ok")
+  })
+
+  windowsTest("kills a wrapper and its grandchild on timeout", async () => {
+    const wrapper = [
+      'const child = Bun.spawn([process.execPath, "-e", "await Bun.sleep(30_000)"],',
+      '  { stdout: "ignore", stderr: "ignore" });',
+      'process.stdout.write(String(child.pid) + "\\n");',
+      "await child.exited;",
+    ].join("\n")
+
+    const r = await runSubprocess(bunEval(wrapper), { timeoutMs: 500 })
+    const grandchildPid = Number(r.stdout.trim())
+
+    expect(r.timedOut).toBe(true)
+    expect(r.durationMs).toBeLessThan(5000)
+    expect(Number.isInteger(grandchildPid)).toBe(true)
+    expect(() => process.kill(grandchildPid, 0)).toThrow()
+  })
+})