diff --git a/docs/specs/dor-browser.md b/docs/specs/dor-browser.md index 7ad5133d..806c84ba 100644 --- a/docs/specs/dor-browser.md +++ b/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`. diff --git a/docs/specs/dor-cli.md b/docs/specs/dor-cli.md index df561528..83d73e89 100644 --- a/docs/specs/dor-cli.md +++ b/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 diff --git a/dor/bin/dor b/dor/bin/dor index a402b51e..2c2f39e1 100755 --- a/dor/bin/dor +++ b/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 diff --git a/dor/bin/dor.cmd b/dor/bin/dor.cmd index c5d8195d..95849510 100644 --- a/dor/bin/dor.cmd +++ b/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% ) diff --git a/dor/package.json b/dor/package.json index e7044caa..533f0c98 100644 --- a/dor/package.json +++ b/dor/package.json @@ -13,7 +13,7 @@ ], "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": { @@ -21,6 +21,7 @@ "typescript": "^6.0.3" }, "dependencies": { - "@stricli/core": "^1.2.7" + "@stricli/core": "^1.2.7", + "cross-spawn": "^7.0.6" } } diff --git a/dor/src/commands/agent-browser.ts b/dor/src/commands/agent-browser.ts index 3a55480f..dd70019b 100644 --- a/dor/src/commands/agent-browser.ts +++ b/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 { 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); }); }); } diff --git a/dor/src/node-runtime.d.ts b/dor/src/node-runtime.d.ts index 5400d95e..09781457 100644 --- a/dor/src/node-runtime.d.ts +++ b/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; } diff --git a/dor/test/cli-output.test.mjs b/dor/test/cli-output.test.mjs index 7393ae6d..bafea32f 100644 --- a/dor/test/cli-output.test.mjs +++ b/dor/test/cli-output.test.mjs @@ -1,7 +1,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { mkdir, readFile, writeFile } from 'node:fs/promises'; -import { dirname, join } from 'node:path'; +import { delimiter, dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { runCli } from '../dist/cli.js'; import { buildShellCommandForKind, shellCommandKind } from '../dist/commands/shell-quote.js'; @@ -159,14 +159,29 @@ function fakeAgentBrowser({ exitCode = 0, stdout = '✓ ok\n', stderr = '' } = { }; } +// Snapshots are authored in their Unix form, and CI compares against them exactly. +// On Windows, any path the CLI resolves through node:path comes back drive-prefixed +// with backslash separators (and JSON output escapes each backslash as `\\`), so e.g. +// `--cwd /Users/me/projects/site` renders as `C:\\Users\\me\\projects\\site`. Smudge +// such paths back to their Unix form as the output is captured — like a git smudge +// filter on the way in — so a snapshot regenerated on Windows still writes the Unix +// form, and the comparison below stays byte-exact rather than platform-aware. +// Deliberately narrow: it only fires on win32 and only rewrites a `X:\...` token, so +// it can't touch escapes like `\n` that share the backslash but aren't paths. +function smudgeWindowsPaths(text) { + if (process.platform !== 'win32') return text; + return text.replace(/[A-Za-z]:(?:\\{1,2}[^"\\]+)+/g, (path) => + path.slice(2).replace(/\\{1,2}/g, '/')); +} + async function snapshot(name, result) { - const actual = [ + const actual = smudgeWindowsPaths([ `exitCode: ${result.exitCode}`, 'stdout:', result.stdout, 'stderr:', result.stderr, - ].join('\n'); + ].join('\n')); const path = join(snapshotsDir, `${name}.snap`); if (updateSnapshots) { await mkdir(snapshotsDir, { recursive: true }); @@ -236,6 +251,9 @@ test('ensure text output', async () => { test('ensure sends command argv and caller cwd to the host', async () => { const client = fixtureClient(); await runCli(['ensure', '--', 'pnpm', 'dev'], { client, env: { PWD: '/work/site' } }); + // On win32 resolvePath drive-prefixes the POSIX PWD (`C:\work\site`); smudge it + // back so this expectation is written once in its Unix form, like the snapshots. + client.requests[0].request.cwd = smudgeWindowsPaths(client.requests[0].request.cwd); assert.deepEqual(client.requests, [{ method: 'ensureSurface', request: { @@ -560,14 +578,20 @@ test('agent-browser resolves the binary on PATH to an absolute binaryPath', asyn const { tmpdir } = await import('node:os'); const dir = await mkdtemp(join(tmpdir(), 'dor-ab-')); try { - const binPath = join(dir, 'agent-browser'); + // On Windows a bare name isn't executable and resolveBinaryPath walks + // PATHEXT (.cmd/.exe/.bat), so the on-disk shim must carry one of those + // extensions — mirroring how agent-browser actually installs there. + const ext = process.platform === 'win32' ? '.cmd' : ''; + const binPath = join(dir, `agent-browser${ext}`); await write(binPath, '#!/bin/sh\n', { mode: 0o755 }); const ab = fakeAgentBrowser(); const client = fixtureClient(); await runCli(['ab', 'snapshot'], { client, execAgentBrowser: ab.exec, - env: { PATH: `/nonexistent:${dir}` }, + // Join with the platform PATH delimiter (`;` on Windows, `:` elsewhere) — + // resolveBinaryPath splits on the same, so a POSIX-only `:` would hide dir. + env: { PATH: ['/nonexistent', dir].join(delimiter) }, }); assert.equal(client.requests[0].request.binaryPath, binPath); } finally { diff --git a/dor/test/snapshots/agent-browser-missing-binary.snap b/dor/test/snapshots/agent-browser-missing-binary.snap index a3f2eadc..8a88e60f 100644 --- a/dor/test/snapshots/agent-browser-missing-binary.snap +++ b/dor/test/snapshots/agent-browser-missing-binary.snap @@ -2,4 +2,12 @@ exitCode: 1 stdout: stderr: -Error: agent-browser was not found (looked for 'agent-browser'). Install it with: npm i -g agent-browser +Error: agent-browser is not installed. + +dor ab drives your own agent-browser binary, which Dormouse never bundles. +Install it, then re-run your command: + + npm i -g agent-browser + +More: https://agent-browser.dev +Already installed? Make sure it's on your PATH, or set DORMOUSE_AGENT_BROWSER_BIN to its full path. diff --git a/lib/package.json b/lib/package.json index 32d0a7da..0e5cf310 100644 --- a/lib/package.json +++ b/lib/package.json @@ -19,6 +19,7 @@ "@xterm/addon-unicode-graphemes": "0.5.0-beta.288", "@xterm/xterm": "6.1.0-beta.288", "clsx": "^2.1.1", + "cross-spawn": "^7.0.6", "dockview-react": "^5.1.0", "fflate": "0.8.3", "jsonc-parser": "3.3.1", diff --git a/lib/src/host/agent-browser-host.test.ts b/lib/src/host/agent-browser-host.test.ts index e643e07a..f8a8023f 100644 --- a/lib/src/host/agent-browser-host.test.ts +++ b/lib/src/host/agent-browser-host.test.ts @@ -9,8 +9,10 @@ type SpawnResult = { stdout?: string; stderr?: string; code?: number }; const spawnMock = vi.hoisted(() => vi.fn()); -vi.mock('child_process', () => ({ - spawn: spawnMock, +// The host spawns via cross-spawn (default export) for cross-platform `.cmd` +// resolution; mock that, not node:child_process. +vi.mock('cross-spawn', () => ({ + default: spawnMock, })); function enqueueSpawnResults(results: SpawnResult[]) { diff --git a/lib/src/host/agent-browser-host.ts b/lib/src/host/agent-browser-host.ts index bcdbbd8f..347f46fd 100644 --- a/lib/src/host/agent-browser-host.ts +++ b/lib/src/host/agent-browser-host.ts @@ -38,7 +38,14 @@ import * as os from 'os'; import * as path from 'path'; import { promises as fs } from 'fs'; -import { spawn } from 'child_process'; +// cross-spawn, not 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; +// the GUI host hits this even for the absolute `binaryPath` dor ab resolved. +// cross-spawn routes through cmd.exe with correct escaping and is a no-op on +// POSIX. See docs/specs/dor-cli.md → "Spawning External Binaries". +import spawn from 'cross-spawn'; import { randomBytes } from 'crypto'; import { type AgentBrowserTab, parseAgentBrowserTabs } from '../lib/agent-browser-tab'; import { @@ -66,6 +73,9 @@ const EDIT_SCRIPTS: Record = { const STREAM_PORT_READ_ATTEMPTS = 4; const STREAM_PORT_READ_DELAY_MS = 150; +// Grace for 'close' to fire after 'exit' before resolving anyway, so a daemon +// holding the inherited stdio pipes can't hang the spawn. See spawnAgentBrowser. +const CLOSE_GRACE_MS = 250; const delay = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); export interface AgentBrowserHostDeps { @@ -120,21 +130,43 @@ export function createAgentBrowserHost(deps: AgentBrowserHostDeps): AgentBrowser function spawnAgentBrowser(binary: string, args: string[]): Promise { return new Promise((resolve) => { - 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 = ''; + let settled = false; + let graceTimer: ReturnType | undefined; + const settle = (apply: () => void): void => { + if (settled) return; + settled = true; + if (graceTimer !== undefined) clearTimeout(graceTimer); + apply(); + }; child.stdout.on('data', (chunk) => { stdout += String(chunk); }); child.stderr.on('data', (chunk) => { stderr += String(chunk); }); - child.on('error', (err: NodeJS.ErrnoException) => { + child.on('error', (err: NodeJS.ErrnoException) => settle(() => { if (err.code === 'ENOENT') { resolve('ENOENT'); return; } log(`[agent-browser] spawn failed: ${err.message}`); resolve({ exitCode: 1, stdout: '', stderr: err.message }); - }); - child.on('close', (code) => { - resolve({ exitCode: code ?? 1, stdout, stderr }); + })); + const finish = (code: number | null, out: string, err: string): void => + settle(() => resolve({ exitCode: code ?? 1, stdout: out, stderr: err })); + // Resolve on 'close' (clean: process exited and stdio drained), but fall + // back to 'exit' because `agent-browser open` leaves a detached daemon that + // on Windows inherits these pipes, so they never reach EOF and 'close' never + // fires. The grace lets 'close' win first so normal commands keep full + // output. See dor/src/commands/agent-browser.ts for the matching rationale. + child.on('close', (code) => finish(code, stdout, stderr)); + child.on('exit', (code) => { + // Snapshot at exit so output the surviving daemon writes into the + // inherited pipes during the grace doesn't leak into the result. + const out = stdout; + const err = stderr; + graceTimer = setTimeout(() => finish(code, out, err), CLOSE_GRACE_MS); }); }); } diff --git a/package.json b/package.json index dc4624e6..b399491e 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "build": "pnpm run build:vscode && pnpm --filter dormouse-website build", "test": "pnpm -r run test", "dev:lib": "pnpm --filter dormouse-lib dev", - "dev:standalone": "pnpm --filter dormouse-standalone tauri dev", + "dev:standalone": "node scripts/free-dev-port.mjs && pnpm --filter dormouse-standalone tauri dev", "dev:standalone:ab": "pnpm --filter dormouse-standalone dev:agent-browser", "dev:website": "pnpm --filter dormouse-website dev", "build:vscode": "pnpm --filter dormouse-lib build && pnpm --filter dormouse build:frontend && pnpm --filter dormouse build", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1bda820a..35906962 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -17,6 +17,9 @@ importers: '@stricli/core': specifier: ^1.2.7 version: 1.2.8 + cross-spawn: + specifier: ^7.0.6 + version: 7.0.6 devDependencies: esbuild: specifier: ^0.28.0 @@ -42,6 +45,9 @@ importers: clsx: specifier: ^2.1.1 version: 2.1.1 + cross-spawn: + specifier: ^7.0.6 + version: 7.0.6 dockview-react: specifier: ^5.1.0 version: 5.2.0(react@19.2.7) @@ -155,6 +161,9 @@ importers: '@vitejs/plugin-react': specifier: ^6.0.2 version: 6.0.3(vite@8.1.0(esbuild@0.28.1)(jiti@2.7.0)) + cross-spawn: + specifier: ^7.0.6 + version: 7.0.6 esbuild: specifier: ^0.28.0 version: 0.28.1 @@ -2282,6 +2291,10 @@ packages: resolution: {integrity: sha512-ei8Aos7ja0weRpFzJnEA9UHJ/7XQmqglbRwnf2ATjcB9Wq874VKH9kfjjirM6UhU2/E5fFYadylyhFldcqSidQ==} engines: {node: '>=18'} + cross-spawn@7.0.6: + resolution: {integrity: sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==} + engines: {node: '>= 8'} + css-select@5.2.2: resolution: {integrity: sha512-TizTzUddG/xYLA3NXodFM0fSbNizXjOKhqiQQwvhlspadZokn1KDy0NZFS0wuEubIYAV5/c1/lAr0TaaFXEXzw==} @@ -2745,6 +2758,9 @@ packages: resolution: {integrity: sha512-PGEHtwMnKbZpeSEXW2Utx+/JWed7dp6DiH0WWg33vGSDA7RUvpUeJSVlLrVkQ1RCpvDOUc/eH9ql7VsdbBZ8pA==} engines: {node: '>=18'} + isexe@2.0.0: + resolution: {integrity: sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==} + istextorbinary@9.5.0: resolution: {integrity: sha512-5mbUj3SiZXCuRf9fT3ibzbSSEWiy63gFfksmGfdOzujPjW3k+z8WvIBxcJHBoQNlaZaiyB25deviif2+osLmLw==} engines: {node: '>=4'} @@ -3259,6 +3275,10 @@ packages: parse5@8.0.1: resolution: {integrity: sha512-z1e/HMG90obSGeidlli3hj7cbocou0/wa5HacvI3ASx34PecNjNQeaHNo5WIZpWofN9kgkqV1q5YvXe3F0FoPw==} + path-key@3.1.1: + resolution: {integrity: sha512-ojmeN0qd+y0jszEtoY48r0Peq5dwMEkIlCOu6Q5f41lfkswXuKtYrhgoTpLnyIcHm24Uhqx+5Tqm2InSwLhE6Q==} + engines: {node: '>=8'} + path-parse@1.0.7: resolution: {integrity: sha512-LDJzPVEEEPR+y48z93A0Ed0yXb8pAByGWo/k5YYdYgpY2/2EsOsksJrq7lOHxryrVOn1ejG6oAp8ahvOIQD8sw==} @@ -3495,6 +3515,14 @@ packages: resolution: {integrity: sha512-Ou9I5Ft9WNcCbXrU9cMgPBcCK8LiwLqcbywW3t4oDV37n1pzpuNLsYiAV8eODnjbtQlSDwZ2cUEeQz4E54Hltg==} engines: {node: ^18.17.0 || ^20.3.0 || >=21.0.0} + shebang-command@2.0.0: + resolution: {integrity: sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==} + engines: {node: '>=8'} + + shebang-regex@3.0.0: + resolution: {integrity: sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==} + engines: {node: '>=8'} + side-channel-list@1.0.1: resolution: {integrity: sha512-mjn/0bi/oUURjc5Xl7IaWi/OJJJumuoJFQJfDDyO46+hBWsfaVM65TBHq2eoZBhzl9EchxOijpkbRC8SVBQU0w==} engines: {node: '>= 0.4'} @@ -4001,6 +4029,11 @@ packages: when-exit@2.1.5: resolution: {integrity: sha512-VGkKJ564kzt6Ms1dbgPP/yuIoQCrsFAnRbptpC5wOEsDaNsbCB2bnfnaA8i/vRs5tjUSEOtIuvl9/MyVsvQZCg==} + which@2.0.2: + resolution: {integrity: sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA==} + engines: {node: '>= 8'} + hasBin: true + why-is-node-running@2.3.0: resolution: {integrity: sha512-hUrmaWBdVDcxvYqnyh09zunKzROWjbZTiNy8dBEjkS7ehEDQibXJ7XvlmtbwuTclUiIyN+CyXQD4Vmko8fNm8w==} engines: {node: '>=8'} @@ -5866,6 +5899,12 @@ snapshots: cookie@1.1.1: {} + cross-spawn@7.0.6: + dependencies: + path-key: 3.1.1 + shebang-command: 2.0.0 + which: 2.0.2 + css-select@5.2.2: dependencies: boolbase: 1.0.0 @@ -6315,6 +6354,8 @@ snapshots: isbot@5.1.44: {} + isexe@2.0.0: {} + istextorbinary@9.5.0: dependencies: binaryextensions: 6.11.0 @@ -6712,6 +6753,8 @@ snapshots: dependencies: entities: 8.0.0 + path-key@3.1.1: {} + path-parse@1.0.7: {} path-scurry@2.0.2: @@ -7023,6 +7066,12 @@ snapshots: '@img/sharp-win32-ia32': 0.34.5 '@img/sharp-win32-x64': 0.34.5 + shebang-command@2.0.0: + dependencies: + shebang-regex: 3.0.0 + + shebang-regex@3.0.0: {} + side-channel-list@1.0.1: dependencies: es-errors: 1.3.0 @@ -7459,6 +7508,10 @@ snapshots: when-exit@2.1.5: {} + which@2.0.2: + dependencies: + isexe: 2.0.0 + why-is-node-running@2.3.0: dependencies: siginfo: 2.0.0 diff --git a/scripts/free-dev-port.mjs b/scripts/free-dev-port.mjs new file mode 100644 index 00000000..63dfbe2d --- /dev/null +++ b/scripts/free-dev-port.mjs @@ -0,0 +1,64 @@ +// Clears the two Windows-only strays that block the next `tauri dev`: +// +// 1. An orphaned `vite` process squatting on the dev-server port — left by an +// interrupted `tauri dev` (closed window, Ctrl-C that doesn't propagate to +// the Vite child) — so the next run aborts with "Port 1420 is already in use". +// 2. An orphaned standalone sidecar `node.exe` running out of `target\debug`. +// Its JobObject leash should take it down with the app, but a force-quit or +// crash can leave it alive holding a lock on `target\debug\node.exe`; the next +// Rust rebuild then fails copying the sidecar binary with "Access is denied." +// +// Both are no-ops everywhere but Windows. +import { execFileSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +if (process.platform !== 'win32') { + process.exit(0); +} + +// Keep in sync with standalone/vite.config.ts (defaults to 1420). +const port = Number(process.env.DORMOUSE_BROWSER_DEV_VITE_PORT || 1420); + +// scripts/ sits at the repo root, so the sidecar's dev build lives at +// standalone/src-tauri/target/debug next to it. +const repoRoot = dirname(dirname(fileURLToPath(import.meta.url))); +const debugDir = join(repoRoot, 'standalone', 'src-tauri', 'target', 'debug'); + +// Single-quoted PS literals; double any apostrophe in the path so it can't break out. +const debugDirLiteral = debugDir.replace(/'/g, "''"); + +const script = ` +$ErrorActionPreference = 'SilentlyContinue' + +# 1. Orphaned Vite squatting on the dev-server port. +$conns = Get-NetTCPConnection -LocalPort ${port} -State Listen +foreach ($procId in ($conns.OwningProcess | Select-Object -Unique)) { + $proc = Get-Process -Id $procId + if ($proc -and $proc.ProcessName -eq 'node') { + Stop-Process -Id $procId -Force + Write-Output "[free-dev-port] killed orphaned node process $procId holding port ${port}" + } +} + +# 2. Orphaned sidecar node.exe locking the dev build output. +$debugDir = '${debugDirLiteral}' +$sidecars = Get-CimInstance Win32_Process -Filter "Name = 'node.exe'" | + Where-Object { $_.ExecutablePath -and $_.ExecutablePath -like "$debugDir\\*" } +foreach ($sidecar in $sidecars) { + Stop-Process -Id $sidecar.ProcessId -Force + Write-Output "[free-dev-port] killed orphaned sidecar node process $($sidecar.ProcessId) locking $debugDir" +} +`; + +try { + const out = execFileSync( + 'powershell.exe', + ['-NoProfile', '-NonInteractive', '-Command', script], + { encoding: 'utf8' }, + ).trim(); + if (out) console.log(out); +} catch { + // Best effort: if we can't inspect/kill the strays, let `tauri dev` surface + // the real error itself rather than failing the predev hook. +} diff --git a/standalone/package.json b/standalone/package.json index e5a1170b..362fd9de 100644 --- a/standalone/package.json +++ b/standalone/package.json @@ -6,12 +6,12 @@ "type": "module", "scripts": { "dev": "vite", - "dev:agent-browser": "pnpm stage && node scripts/dev-agent-browser.mjs", - "build": "pnpm stage && tsc -b && vite build", - "stage": "pnpm stage:dor-cli && pnpm stage:sidecar-proxy", + "dev:agent-browser": "pnpm run stage && node scripts/dev-agent-browser.mjs", + "build": "pnpm run stage && tsc -b && vite build", + "stage": "pnpm run stage:dor-cli && pnpm run stage:sidecar-proxy", "stage:dor-cli": "pnpm --filter dor build && node scripts/stage-dor-cli.mjs", "stage:sidecar-proxy": "node scripts/build-sidecar-proxy.mjs", - "tauri": "pnpm stage && tauri", + "tauri": "pnpm run stage && tauri", "test": "vitest run" }, "dependencies": { @@ -34,6 +34,7 @@ "@types/react": "^19.2.14", "@types/react-dom": "^19.2.3", "@vitejs/plugin-react": "^6.0.2", + "cross-spawn": "^7.0.6", "esbuild": "^0.28.0", "jsdom": "^29.1.1", "tailwindcss": "^4.3.0", diff --git a/standalone/scripts/dev-agent-browser.mjs b/standalone/scripts/dev-agent-browser.mjs index d37d06f5..59e7717c 100644 --- a/standalone/scripts/dev-agent-browser.mjs +++ b/standalone/scripts/dev-agent-browser.mjs @@ -4,7 +4,11 @@ import net from 'node:net'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { spawn } from 'node:child_process'; +// cross-spawn, not node:child_process: this script spawns `pnpm` and +// `agent-browser`, which are `.cmd` shims on Windows that a bare-name spawn +// can't resolve (ENOENT) and Node >=22 won't run directly (EINVAL). cross-spawn +// handles both and is a no-op on POSIX. See docs/specs/dor-cli.md. +import spawn from 'cross-spawn'; import { createInterface } from 'node:readline'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); diff --git a/standalone/src-tauri/src/lib.rs b/standalone/src-tauri/src/lib.rs index 0da077e6..c9581c30 100644 --- a/standalone/src-tauri/src/lib.rs +++ b/standalone/src-tauri/src/lib.rs @@ -638,7 +638,13 @@ fn resolve_sidecar_path(resource_dir: Option, manifest_dir: &Path) -> P for prefix in &["sidecar", "_up_/sidecar"] { let path = dir.join(prefix).join("main.js"); if path.is_file() { - return path; + // resource_dir() hands back a `\\?\` verbatim path in the + // bundled/dev layout. Normalize once here, at the boundary, so + // every consumer (the node script arg, the dor-cli paths derived + // from this path's parent) gets a plain path. cmd.exe can't + // execute a batch file via a verbatim path; Rust's APIs accept + // both, so stripping is always safe. + return strip_windows_verbatim_prefix(&path.to_string_lossy()).unwrap_or(path); } } } @@ -657,14 +663,6 @@ fn strip_windows_verbatim_prefix(path_string: &str) -> Option { None } -fn sidecar_script_arg_path(path: &Path) -> PathBuf { - if let Some(path) = strip_windows_verbatim_prefix(&path.to_string_lossy()) { - return path; - } - - path.to_path_buf() -} - fn resolve_node_binary_path() -> Result { let exe = env::current_exe().map_err(|e| format!("current_exe: {e}"))?; let dir = exe @@ -734,7 +732,6 @@ fn resolve_dor_cli_paths(sidecar_path: &Path, manifest_dir: &Path) -> DorCliPath fn start_sidecar(app: &AppHandle) -> Result { let manifest_dir = Path::new(env!("CARGO_MANIFEST_DIR")); let sidecar_path = resolve_sidecar_path(app.path().resource_dir().ok(), manifest_dir); - let sidecar_arg_path = sidecar_script_arg_path(&sidecar_path); let node_path = resolve_node_binary_path()?; let dor_cli_paths = resolve_dor_cli_paths(&sidecar_path, manifest_dir); let dor_control_socket = dor_control_socket_path(); @@ -743,10 +740,6 @@ fn start_sidecar(app: &AppHandle) -> Result { "[sidecar] resolved script: {}", sidecar_path.display() )); - append_log(format!( - "[sidecar] script argument: {}", - sidecar_arg_path.display() - )); append_log(format!("[sidecar] node binary: {}", node_path.display())); append_log(format!( "[dor] CLI bin dir: {}", @@ -759,7 +752,7 @@ fn start_sidecar(app: &AppHandle) -> Result { append_log(format!("[dor] control socket: {dor_control_socket}")); let mut wrap = CommandWrap::with_new(&node_path, |c| { - c.arg(&sidecar_arg_path) + c.arg(&sidecar_path) .env("DORMOUSE_NODE", &node_path) .env("DORMOUSE_CLI_BIN", &dor_cli_paths.bin_dir) .env("DORMOUSE_CLI_JS", &dor_cli_paths.entrypoint) @@ -1192,4 +1185,26 @@ mod tests { assert_eq!(resolved.bin_dir, dor_root.join("bin")); assert_eq!(resolved.entrypoint, dor_root.join("dist").join("dor.js")); } + + // resource_dir() hands us a `\\?\` verbatim path on Windows. resolve_sidecar_path + // is the single normalization boundary: it must strip the prefix so every + // downstream consumer (the node script arg, and the dor-cli paths derived from + // this path's parent) gets a plain path — otherwise cmd.exe can't launch + // `dor.cmd` reached through DORMOUSE_CLI_BIN on PATH. + #[test] + #[cfg(windows)] + fn resolve_sidecar_path_strips_verbatim_prefix() { + let resource_dir = TempDir::new("sidecar-verbatim"); + let sidecar_path = resource_dir.path().join("sidecar").join("main.js"); + fs::create_dir_all(sidecar_path.parent().unwrap()).expect("failed to create sidecar dir"); + fs::write(&sidecar_path, "console.log('sidecar');").expect("failed to create sidecar"); + + // A verbatim resource dir to the same real tree; is_file() still resolves it. + let verbatim_resource = PathBuf::from(format!(r"\\?\{}", resource_dir.path().display())); + let resolved = + resolve_sidecar_path(Some(verbatim_resource), Path::new("/repo/standalone/src-tauri")); + + assert_eq!(resolved, sidecar_path); + assert!(!resolved.to_string_lossy().contains(r"\\?\")); + } }