Make dor / dor ab work on Windows (#188)

Author: nedtwigg

docs/specs/dor-browser.md

@@ -191,6 +191,12 @@ The binary is resolved from `DORMOUSE_AGENT_BROWSER_BIN` or `PATH`. If present,
 `dor ab` resolves an absolute `binaryPath` and passes it to the host because GUI
 hosts may not share the terminal's shell PATH.
 
+Both `dor ab` and the host spawn `agent-browser` through `cross-spawn`, never raw
+`child_process` — on Windows it ships as a `.cmd` shim that a bare-name spawn
+can't find (ENOENT) and Node ≥22 won't run directly (EINVAL), so even the
+absolute `binaryPath` must go through it. See docs/specs/dor-cli.md → "Spawning
+External Binaries".
+
 Managed identity:
 
 - Default is `--key default`.

docs/specs/dor-cli.md

@@ -60,6 +60,56 @@ Public PTY env:
 `DORMOUSE_CLI_BIN` is host-internal spawn configuration. Terminals should rely
 on `PATH`, not on that variable.
 
+On Windows, `DORMOUSE_CLI_BIN` and `DORMOUSE_CLI_JS` must be plain paths, never
+`\\?\` verbatim paths. The standalone host derives them from Tauri's
+`resource_dir()`, which returns a verbatim prefix in the bundled/dev layout; the
+host strips it once at the boundary (`resolve_sidecar_path`), so every derived
+path is plain. `dor.cmd` is reached through
+`DORMOUSE_CLI_BIN` on `PATH`, and cmd.exe cannot execute a batch file via a
+verbatim path — it fails with "The system cannot find the path specified."
+
+## Spawning External Binaries
+
+Any time Dormouse spawns an external/user-installed binary — `dor ab` driving
+`agent-browser`, the agent-browser host running tab/eval/screenshot commands, dev
+harnesses launching `pnpm`/`agent-browser` — it goes through **`cross-spawn`**,
+never raw `node:child_process` `spawn`. This is mandatory for correctness on
+Windows, where two distinct failures bite a naive spawn:
+
+- **ENOENT on a bare name.** Node's `spawn` does not consult `PATHEXT`, so a bare
+  `agent-browser` never resolves the `agent-browser.cmd` PATH shim that npm/vfox
+  installs. (`agent-browser` works from a POSIX shell only because the file there
+  is a real executable with a shebang; on Windows it is a `.cmd`.)
+- **EINVAL on a `.cmd` even by full path.** Node ≥22 refuses to spawn `.cmd`/
+  `.bat` files without a shell (the CVE-2024-27980 hardening), so resolving the
+  absolute `.cmd` path and spawning it directly still fails.
+
+`cross-spawn` resolves the command via `PATH`/`PATHEXT` and routes `.cmd`/`.bat`
+through `cmd.exe` with correct argument escaping, and is a transparent passthrough
+on POSIX. Use it with the same `(command, args, options)` signature as
+`child_process.spawn`; it is bundled into `dist/dor.js` and the sidecar `.cjs`
+by esbuild.
+
+Caveat: a literal `%VAR%` inside an argument can still be expanded by `cmd.exe`
+when it passes through a `.cmd` shim — an unavoidable Windows batch limitation, not
+something `cross-spawn` (or any wrapper) can fully prevent. Our forwarded
+arguments (URLs, selectors, and the host's hardcoded `eval` scripts) contain no
+`%VAR%` patterns, so this does not arise in practice.
+
+### Resolve on `exit`, not `close`
+
+When buffering a spawned command's output, resolve on the child's **`exit`**
+event, not `close`. `agent-browser open` launches a long-lived per-session daemon
+that on Windows inherits the parent's stdout/stderr pipes; those pipes never reach
+EOF while the daemon lives, so `close` (which waits for stdio to drain) never
+fires and the spawn hangs forever. `exit` fires when the foreground process ends
+regardless of the lingering pipe. The two spawn helpers
+(`dor/src/commands/agent-browser.ts`, `lib/src/host/agent-browser-host.ts`) wait
+for `close` but fall back to `exit` after a short grace (`CLOSE_GRACE_MS`), so a
+normal command's full output still flushes while the daemon case can't hang.
+(POSIX dodges this because the daemon double-forks and detaches from the inherited
+fds, closing the pipe — which is why this never surfaced on macOS.)
+
 ## Host Plumbing
 
 ### Standalone

dor/bin/dor

@@ -1,5 +1,12 @@
 #!/bin/sh
 if [ -n "${DORMOUSE_NODE:-}" ] && [ -n "${DORMOUSE_CLI_JS:-}" ]; then
+  # DORMOUSE_NODE is the editor's Electron binary, which only behaves as Node
+  # when ELECTRON_RUN_AS_NODE is set. Set it here rather than relying on the
+  # ambient env to carry it: terminals routinely strip it (so Electron apps
+  # launched from a shell don't misbehave), and without it Electron launches its
+  # GUI, ignores the script, and exits 0 — so `dor` would silently do nothing.
+  ELECTRON_RUN_AS_NODE=1
+  export ELECTRON_RUN_AS_NODE
   exec "$DORMOUSE_NODE" "$DORMOUSE_CLI_JS" "$@"
 fi
 

dor/bin/dor.cmd

@@ -1,6 +1,11 @@
 @echo off
 setlocal
 if not "%DORMOUSE_NODE%"=="" if not "%DORMOUSE_CLI_JS%"=="" (
+  rem DORMOUSE_NODE is the editor's Electron binary; it only behaves as Node when
+  rem ELECTRON_RUN_AS_NODE is set. Set it here rather than relying on the ambient
+  rem env to carry it: without it Electron launches its GUI, ignores the script,
+  rem and exits 0 — so `dor` would silently do nothing.
+  set "ELECTRON_RUN_AS_NODE=1"
   "%DORMOUSE_NODE%" "%DORMOUSE_CLI_JS%" %*
   exit /b %ERRORLEVEL%
 )

dor/package.json

@@ -13,14 +13,15 @@
   ],
   "scripts": {
     "prebuild": "node ../scripts/generate-dor-version.mjs",
-    "build": "tsc -p tsconfig.json && esbuild src/dor.ts --bundle --format=esm --platform=node --outfile=dist/dor.js",
+    "build": "tsc -p tsconfig.json && esbuild src/dor.ts --bundle --format=esm --platform=node --outfile=dist/dor.js --banner:js=\"import { createRequire } from 'module'; const require = createRequire(import.meta.url);\"",
     "test": "pnpm run build && node --test test/*.test.mjs"
   },
   "devDependencies": {
     "esbuild": "^0.28.0",
     "typescript": "^6.0.3"
   },
   "dependencies": {
-    "@stricli/core": "^1.2.7"
+    "@stricli/core": "^1.2.7",
+    "cross-spawn": "^7.0.6"
   }
 }

dor/src/commands/agent-browser.ts

@@ -16,7 +16,13 @@
  */
 
 import { buildCommand } from '@stricli/core';
-import { spawn } from 'node:child_process';
+// cross-spawn, not node:child_process — on Windows a bare command name never
+// resolves a `.cmd`/`.bat` PATH shim (Node spawn ignores PATHEXT → ENOENT), and
+// Node >=22 refuses to spawn a `.cmd` directly even by full path (EINVAL, the
+// CVE-2024-27980 hardening). agent-browser ships as a `.cmd` shim, so both bite.
+// cross-spawn routes through cmd.exe with correct escaping and is a no-op
+// passthrough on POSIX. See docs/specs/dor-cli.md → "Spawning External Binaries".
+import spawn from 'cross-spawn';
 import { existsSync } from 'node:fs';
 import type {
   CliEnv,
@@ -33,6 +39,33 @@ import { fail, requireControlClient, stringParser } from './shared.js';
 const WORKSPACE_ID = '1';
 
 const INSTALL_HINT = 'npm i -g agent-browser';
+const INSTALL_DOCS = 'https://agent-browser.dev';
+const BIN_ENV = 'DORMOUSE_AGENT_BROWSER_BIN';
+
+// Extensions a bare command name can carry on Windows, in PATH-search order.
+// Shared by resolveBinaryPath (PATH walk) and existsCandidate (explicit path).
+const WINDOWS_BIN_EXTS = ['.cmd', '.exe', '.bat'];
+
+/**
+ * Clear, multi-line guidance shown when the user's agent-browser binary is
+ * absent. `binary` is named only when it differs from the default, so a custom
+ * DORMOUSE_AGENT_BROWSER_BIN that points nowhere still tells the user what was
+ * looked for.
+ */
+function missingBinaryMessage(binary: string): string {
+  const lookedFor = binary === 'agent-browser' ? '' : ` (looked for '${binary}')`;
+  return [
+    `agent-browser is not installed${lookedFor}.`,
+    '',
+    'dor ab drives your own agent-browser binary, which Dormouse never bundles.',
+    'Install it, then re-run your command:',
+    '',
+    `    ${INSTALL_HINT}`,
+    '',
+    `More: ${INSTALL_DOCS}`,
+    `Already installed? Make sure it's on your PATH, or set ${BIN_ENV} to its full path.`,
+  ].join('\n');
+}
 
 // agent-browser session names become filesystem paths (socket dir), so `/` is
 // not usable as a namespace separator — the daemon fails to start. Dots keep
@@ -160,12 +193,29 @@ export async function runAgentBrowserCli(args: string[], options: CliOptions): P
   const binary = env.DORMOUSE_AGENT_BROWSER_BIN || 'agent-browser';
   const exec = options.execAgentBrowser ?? execAgentBrowserProcess;
 
+  // Resolve the binary to an absolute path once: it both proves the install
+  // present (below) and travels to the host as `binaryPath` (a GUI host may not
+  // share this terminal's PATH). undefined means "not found on PATH" — or, for
+  // an explicit path, simply "returned verbatim", which agentBrowserIsMissing
+  // re-checks on disk.
+  const binaryPath = resolveBinaryPath(binary, env);
+
+  // Detect a missing install deterministically, before spawning. A failed spawn
+  // on Windows emits BOTH 'error' (ENOENT) and 'close' (a libuv error code); if
+  // 'close' wins that race the process resolves with a bogus exit code and no
+  // output, so `dor ab` would print nothing at all. Checking the filesystem
+  // ourselves sidesteps that ordering. Skipped when a stub exec is injected
+  // (tests), which supplies its own ENOENT behavior via the catch below.
+  if (options.execAgentBrowser === undefined && agentBrowserIsMissing(binary, env, binaryPath)) {
+    return fail(missingBinaryMessage(binary));
+  }
+
   let result: AgentBrowserExecResult;
   try {
     result = await exec(binary, ['--session', session, ...rest]);
   } catch (error) {
     if (isMissingBinaryError(error)) {
-      return fail(`agent-browser was not found (looked for '${binary}'). Install it with: ${INSTALL_HINT}`);
+      return fail(missingBinaryMessage(binary));
     }
     return fail(error instanceof Error ? error.message : String(error));
   }
@@ -179,11 +229,8 @@ export async function runAgentBrowserCli(args: string[], options: CliOptions): P
       try {
         const status = await exec(binary, ['--session', session, 'stream', 'status', '--json']);
         const wsPort = parseStreamPort(status.stdout);
-        // The Dormouse host (e.g. a GUI-launched VS Code extension host) may
-        // not share this terminal's PATH, so resolve the binary to an
-        // absolute path here, where the user's environment is authoritative,
-        // and pass it along for host-side tab/close commands.
-        const binaryPath = resolveBinaryPath(binary, env);
+        // Pass the absolute path resolved above so the host (which may not share
+        // this terminal's PATH) can run host-side tab/close commands.
         await client.agentBrowserSurface({
           key,
           session,
@@ -217,7 +264,7 @@ export function resolveBinaryPath(binary: string, env: CliEnv): string | undefin
   const pathVar = env.PATH;
   if (!pathVar) return undefined;
   const isWindows = process.platform === 'win32';
-  const names = isWindows ? [`${binary}.cmd`, `${binary}.exe`, `${binary}.bat`] : [binary];
+  const names = isWindows ? WINDOWS_BIN_EXTS.map((ext) => `${binary}${ext}`) : [binary];
   for (const dir of pathVar.split(isWindows ? ';' : ':')) {
     if (!dir) continue;
     for (const name of names) {
@@ -242,19 +289,83 @@ function isMissingBinaryError(error: unknown): boolean {
   return !!error && typeof error === 'object' && (error as { code?: unknown }).code === 'ENOENT';
 }
 
+/**
+ * Whether the binary can be proven absent without spawning it, given the path
+ * `resolveBinaryPath` already produced for it. Returns true only when the absence
+ * is certain; ambiguous cases (no PATH to search) fall through to the spawn,
+ * which still rejects with ENOENT.
+ */
+function agentBrowserIsMissing(binary: string, env: CliEnv, resolvedPath: string | undefined): boolean {
+  // Explicit path (e.g. a DORMOUSE_AGENT_BROWSER_BIN override): resolveBinaryPath
+  // hands such a path back verbatim without touching disk, so check it (and
+  // Windows launcher extensions) directly.
+  if (binary.includes('/') || binary.includes('\\')) {
+    return !existsCandidate(binary, process.platform === 'win32');
+  }
+  // Bare name: resolvedPath is the PATH walk's result. Without a PATH to search
+  // we can't prove anything, so let the spawn decide.
+  if (!env.PATH) return false;
+  return resolvedPath === undefined;
+}
+
+function existsCandidate(path: string, isWindows: boolean): boolean {
+  if (existsSync(path)) return true;
+  if (!isWindows) return false;
+  return WINDOWS_BIN_EXTS.some((ext) => existsSync(`${path}${ext}`));
+}
+
 // agent-browser talks to a daemon, so forwarded commands return quickly;
 // buffering output until exit keeps this transport-agnostic with runCli's
 // captured stdout/stderr at the cost of not streaming long-running output.
+//
+// Grace window for 'close' to win after 'exit' before we resolve anyway. See
+// execAgentBrowserProcess: long enough that a normal command's stdio drains
+// (its output was written before the process exited), short enough that the
+// daemon-holds-the-pipe case doesn't feel like a hang.
+const CLOSE_GRACE_MS = 250;
+
 function execAgentBrowserProcess(binary: string, args: string[]): Promise<AgentBrowserExecResult> {
   return new Promise((resolve, reject) => {
-    const child = spawn(binary, args, { stdio: ['ignore', 'pipe', 'pipe'] });
+    // windowsHide: cross-spawn runs `.cmd` shims through cmd.exe; without this
+    // each spawn flashes a console window that steals focus. No-op off Windows.
+    const child = spawn(binary, args, { stdio: ['ignore', 'pipe', 'pipe'], windowsHide: true });
     let stdout = '';
     let stderr = '';
+    // A failed spawn races 'error' against the exit events; latch on the first so
+    // the loser can't overwrite the outcome (e.g. a stray exit code swallowing an
+    // ENOENT). clearTimeout drops the grace timer so it can't keep the event loop
+    // alive after we've already settled.
+    let settled = false;
+    let graceTimer: number | undefined;
+    const settle = (apply: () => void): void => {
+      if (settled) return;
+      settled = true;
+      if (graceTimer !== undefined) clearTimeout(graceTimer);
+      apply();
+    };
     child.stdout.on('data', (chunk: unknown) => { stdout += String(chunk); });
     child.stderr.on('data', (chunk: unknown) => { stderr += String(chunk); });
-    child.on('error', reject);
-    child.on('close', (code: number | null) => {
-      resolve({ exitCode: code ?? 1, stdout, stderr });
+    child.on('error', (error: Error) => settle(() => reject(error)));
+    const finish = (code: number | null, out: string, err: string): void =>
+      settle(() => resolve({ exitCode: code ?? 1, stdout: out, stderr: err }));
+    // 'close' is the clean path: the process exited AND its stdio reached EOF, so
+    // all output is captured. But `agent-browser open` leaves a detached daemon
+    // that on Windows inherits our stdout/stderr pipes — they never reach EOF and
+    // 'close' never fires, so waiting on it alone hangs forever. Fall back to
+    // 'exit' (which fires when the foreground process ends regardless of the
+    // lingering pipe), giving 'close' a short grace to win first so a normal
+    // command's full output is still flushed before we resolve.
+    child.on('close', (code: number | null) => finish(code, stdout, stderr));
+    child.on('exit', (code: number | null) => {
+      // Snapshot now: the foreground command has produced all its output. The
+      // surviving daemon keeps our inherited pipes open and may scribble into
+      // them during the grace below (this is how `dor ab open` printed a burst
+      // of blank lines on Windows) — resolve with the exit-time snapshot so that
+      // post-command noise is excluded. 'close', if it wins, still uses the live
+      // buffers since without a lingering daemon there's nothing extra to drop.
+      const out = stdout;
+      const err = stderr;
+      graceTimer = setTimeout(() => finish(code, out, err), CLOSE_GRACE_MS);
     });
   });
 }

dor/src/node-runtime.d.ts

@@ -26,11 +26,24 @@ declare module 'node:child_process' {
     stdout: ChildProcessStream;
     stderr: ChildProcessStream;
     on(event: 'error', listener: (error: Error) => void): void;
+    on(event: 'exit', listener: (code: number | null) => void): void;
     on(event: 'close', listener: (code: number | null) => void): void;
   }
 
   export function spawn(command: string, args: readonly string[], options: {
     stdio: readonly ['ignore', 'pipe', 'pipe'];
+    windowsHide?: boolean;
+  }): ChildProcess;
+}
+
+// cross-spawn ships no types and dor avoids @types/node, so declare the one call
+// shape we use. Drop-in for the node:child_process spawn above; returns the same
+// minimal ChildProcess.
+declare module 'cross-spawn' {
+  import type { ChildProcess } from 'node:child_process';
+  export default function spawn(command: string, args: readonly string[], options: {
+    stdio: readonly ['ignore', 'pipe', 'pipe'];
+    windowsHide?: boolean;
   }): ChildProcess;
 }