fix(dor): spawn agent-browser by resolved path, not bare name

docs/specs/dor-cli.md

@@ -97,6 +97,35 @@ tab/eval/screenshot commands, and anything added later. It is the only code
   escaping, and passes through untouched on POSIX. **Never forward an argument
   containing a literal `%VAR%`** — `cmd.exe` expands it through a `.cmd` shim,
   an unavoidable batch limitation; today's forwarded arguments carry none.
+- **`dor ab` spawns the `PATH`-resolved absolute path, never the bare name** —
+  cross-spawn resolves a bare name through `which`, which searches the cwd
+  before `PATH` on Windows (rationale). The host's candidate list still ends in a
+  bare name (`## Future`).
+- **Within the `PATH` directories, and only those, the walk must select the file
+  `which` would** — a divergence either runs a different binary or reports a
+  present install as missing. **Never extend the search to the cwd**, which is
+  the one place `which` looks and the rule above exists to exclude. Inside that
+  scope: skip a directory or a non-executable file rather than returning it, and
+  on Windows take the extension list from `PATHEXT` **or**, when that is unset
+  *or empty*, from `which`'s own hardcoded `.EXE;.CMD;.BAT;.COM` — npm's order,
+  not `cmd.exe`'s — trying the empty extension first when the name already
+  carries one.
+- **A bare name the walk cannot resolve is a missing install, reported before
+  the spawn** — including when there is no `PATH` to search at all, since the
+  spawn's own fallback is the bare name. That leaves `?? binary` unreachable on
+  the real path, and `isMissingBinaryError` covering only a binary that
+  disappears between the walk and the spawn.
+
+  Both Windows-only rules are pinned through an `isWindows` argument rather than
+  a `process.platform` read, because CI runs this suite on Linux only and a
+  platform-gated assertion would assert an unenforced claim. The one assertion
+  that cannot be written that way is the POSIX executable bit, since
+  `accessSync(X_OK)` reports every readable file as executable on Windows.
+
+  Source of truth: `binaryCandidateNames`, `isExecutableFile`, `resolveBinaryPath`
+  and `agentBrowserIsMissing` in `dor/src/commands/agent-browser.ts`; `getPathInfo`
+  in `which/which.js` is what they mirror; pinned in
+  `dor/test/cli-output.test.mjs`.
 - **`windowsHide`.** Without it every `.cmd` shim flashes a focus-stealing
   console window, once per screenshot stream-frame pulse (rationale).
 - **Resolve on `exit`, not `close`, with an exit-time snapshot** — the
@@ -684,6 +713,13 @@ Source of truth: `toolCommand` in `dor/src/commands/tool.ts`; `openCommand` in `
 
 ## Future
 
+- **Resolve the host's agent-browser candidates on `PATH` too.**
+  `runWithBinaryFallback` still ends its list with the bare
+  `DEFAULT_AGENT_BROWSER_BIN`, so on Windows the extension-host or Tauri-app
+  working directory is searched first — narrower than `dor ab`'s case, since a
+  user does not clone into it. Sharing `resolveBinaryPath` means moving it to
+  `dor-lib-common` beside `spawnAndCapture`.
+
 - **Surface a dead control channel in the UI.** A lost bind leaves one
   `[dor-control]` line on the host's stderr, and all a user sees is `dor`
   reporting "Dormouse control endpoint is not available in this terminal yet" —

docs/specs/dor-cli.rationale.md

@@ -22,6 +22,42 @@ The file viewer needs Node types, but the renderer imports CLI protocol and shel
 
 **The two Windows spawn failures cross-spawn absorbs.** Node's `spawn` does not consult `PATHEXT`, so a bare `agent-browser` ENOENTs instead of resolving the `agent-browser.cmd` shim npm/vfox installs (on POSIX that file is a real executable with a shebang); and Node ≥22 refuses to spawn `.cmd`/`.bat` without a shell (the CVE-2024-27980 hardening), so the resolved absolute `.cmd` EINVALs too. Neither failure has a POSIX counterpart.
 
+**Why the bare name is a code-execution primitive on Windows.** cross-spawn
+delegates resolution to `which`, whose Windows branch prepends `process.cwd()`
+to the search path (`which@2.0.2/which.js:19-21`, comment *"windows always
+checks the cwd first"*), and its `.cmd`-shim path re-emits the **bare** name
+into `cmd.exe` (`cross-spawn@7.0.6/lib/parse.js:36,48-59`), which resolves the
+cwd first as well. `dor` inherits the pane's cwd, so before this rule a cloned
+repository containing `agent-browser.cmd` executed on the next `dor ab` — and
+`dor skill` mandates `dor ab` for every page view. The 2026-09-19 security audit
+([run 35432996343](https://github.com/diffplug/dormouse/actions/runs/35432996343))
+raised it as its one BLOCKER: the hijack needs a *legitimate* install present,
+because `agentBrowserIsMissing` refuses to spawn when the PATH walk finds
+nothing. The same audit named three sidecar sites spawning system binaries by
+bare name (`standalone/sidecar/pty-core.js`, `standalone/sidecar/clipboard-ops.js`)
+whose cwd is the app directory rather than a user's repository;
+`pty-core.js`'s `%SystemRoot%\System32` join is the pattern those should follow.
+
+**What promoting the walk to the spawn target changed.** Before it, the walk
+only proved the install present and travelled to the host as a hint, so its
+divergence from `which` was inert: a fixed `.cmd`/`.exe`/`.bat` order and a bare
+`existsSync`. Once it decides what runs, both diverge observably — a directory
+holding `agent-browser.exe` and `agent-browser.cmd` would switch which one runs,
+and a non-executable file or directory named `agent-browser` earlier on `PATH`
+would fail EACCES/EISDIR where `which` walked past it to the real install (that
+one is not Windows-specific). A second round found that `which@2`'s fallback list
+is npm's `.EXE;.CMD;.BAT;.COM` rather than `cmd.exe`'s `.COM;.EXE;.BAT;.CMD`, so
+copying the shell's order inverts `.com`-vs-`.exe` and `.bat`-vs-`.cmd`; that
+`which` uses `||` rather than `??` for it, so an empty `PATHEXT` falls back
+instead of yielding no candidates; and that it unshifts an empty extension when
+the command contains a `.`, so `agent-browser.exe` is searched as itself. All
+four are `getPathInfo` in `which/which.js`, read at 2.0.2. A third round caught
+the invariant stating the opposite of the fix in the case that motivated it —
+`which`'s Windows branch prepends `process.cwd()`, so an unscoped "select the
+file `which` would" licenses the hijack — and that the X_OK probe was unpinned
+because `statSync().isFile()` already rejected the directory the test shadowed
+with. All three rounds were review findings on the fix, before it merged.
+
 **What a missing `windowsHide` looks like.** cross-spawn routes `.cmd` shims through `cmd.exe`, which owns a real console window, and the browser panel's screenshot loop spawns one per stream-frame pulse — a live page flickers focus-stealing windows several times a second.
 
 **Why none of the `exit`-vs-`close` trouble surfaced on macOS.** The `agent-browser` daemon double-forks and detaches from the inherited fds, so `close` fires normally; only on Windows, where the daemon holds the parent's stdout/stderr pipes for its whole life, does a `close`-only wait hang forever.

dor/src/commands/agent-browser.ts

@@ -14,7 +14,7 @@ import {
   AGENT_BROWSER_BIN_ENV,
   DEFAULT_AGENT_BROWSER_BIN,
 } from 'dor-lib-common';
-import { existsSync } from 'node:fs';
+import { accessSync, constants, existsSync, statSync } from 'node:fs';
 import type {
   CliEnv,
   AgentBrowserExecResult,
@@ -35,9 +35,14 @@ import {
 const INSTALL_HINT = 'npm i -g agent-browser';
 const INSTALL_DOCS = 'https://agent-browser.dev';
 
-// 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'];
+// Extensions a bare command name can carry on Windows, and the order to try
+// them in. This is `which@2`'s own hardcoded fallback — npm's list, deliberately
+// NOT cmd.exe's `.COM;.EXE;.BAT;.CMD` — because `resolveBinaryPath` now picks
+// the file that gets spawned and has to choose the same one cross-spawn's
+// `which` would (docs/specs/dor-cli.md → "Spawning External Binaries").
+// Source: `getPathInfo` in `which/which.js`. Shared by resolveBinaryPath (PATH
+// walk) and existsCandidate (explicit path, where order only affects reporting).
+const WINDOWS_BIN_EXTS = ['.EXE', '.CMD', '.BAT', '.COM'];
 
 /**
  * Clear, multi-line guidance shown when the user's agent-browser binary is
@@ -238,13 +243,28 @@ export async function runAgentBrowserCli(args: string[], options: CliOptions): P
   const binary = env[AGENT_BROWSER_BIN_ENV] || DEFAULT_AGENT_BROWSER_BIN;
   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.
+  // Resolve the binary to an absolute path once: it proves the install present
+  // (below), is what we spawn (see `execTarget`), 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);
 
+  // Spawn the resolved path, never the bare name: cross-spawn resolves a bare
+  // name through `which`, which checks `process.cwd()` *before* PATH on Windows
+  // (and re-emits the bare name into cmd.exe for a `.cmd` shim, which does the
+  // same). Since `dor` inherits the pane's cwd, a bare-name spawn would let an
+  // `agent-browser.cmd` sitting in a cloned repository win the race against the
+  // real install — repo content executing with no gate, which
+  // docs/specs/dor-tool.md -> Trust treats as a boundary.
+  //
+  // The `?? binary` branch is unreachable on the real path and is a type-level
+  // belt only: agentBrowserIsMissing already ends the call whenever binaryPath is
+  // undefined, and an explicit path comes back from resolveBinaryPath verbatim.
+  // Only a stub exec (tests), which skips that check, reaches it.
+  // See docs/specs/dor-cli.md -> "Spawning External Binaries".
+  const execTarget = binaryPath ?? binary;
+
   // 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
@@ -257,7 +277,7 @@ export async function runAgentBrowserCli(args: string[], options: CliOptions): P
 
   let result: AgentBrowserExecResult;
   try {
-    result = await exec(binary, ['--session', session, ...rest]);
+    result = await exec(execTarget, ['--session', session, ...rest]);
   } catch (error) {
     if (isMissingBinaryError(error)) {
       return fail(missingBinaryMessage(binary));
@@ -272,7 +292,7 @@ export async function runAgentBrowserCli(args: string[], options: CliOptions): P
     // passthrough rather than nagging about the missing surface.
     if (!(client instanceof Error)) {
       try {
-        const status = await exec(binary, streamStatusArgs(session));
+        const status = await exec(execTarget, streamStatusArgs(session));
         const wsPort = parseStreamPort(status.stdout);
         // Pass the absolute path resolved above so the host (which may not share
         // this terminal's PATH) can run host-side tab/close commands.
@@ -399,42 +419,92 @@ function shouldManageSurface(exitCode: number, rest: string[]): boolean {
   return subcommand !== undefined && subcommand !== 'close';
 }
 
+/**
+ * Whether `candidate` is a file this platform would actually run. `which` (and
+ * so cross-spawn) skips a directory or a non-executable file and keeps walking;
+ * since the walk's answer is now the spawn target, a laxer test here would turn
+ * a `PATH` entry `which` ignored into an EACCES/EISDIR failure. On Windows the
+ * extension decides executability, so being a regular file is the whole test —
+ * taken as an argument, like `binaryCandidateNames`, so both branches are
+ * reachable from a Linux-only CI.
+ */
+export function isExecutableFile(candidate: string, isWindows: boolean): boolean {
+  try {
+    if (!statSync(candidate).isFile()) return false;
+    if (isWindows) return true;
+    accessSync(candidate, constants.X_OK);
+    return true;
+  } catch {
+    return false;
+  }
+}
+
+/**
+ * The filenames to try for a bare `binary`, in order — `which`'s extension logic,
+ * which the walk has to reproduce because its answer is what gets spawned. Takes
+ * `isWindows` rather than reading `process.platform` so the Windows ordering is
+ * testable off Windows: every rule here is Windows-only, and a Linux-only CI
+ * that could not exercise them would be asserting an unenforced claim.
+ *
+ * Mirrors `getPathInfo` in `which/which.js` on three points a hand-rolled walk
+ * gets wrong: `||` (not `??`), so an *empty* PATHEXT falls back rather than
+ * yielding no candidates; the fallback list is npm's, not `cmd.exe`'s; and an
+ * empty extension comes first when the name already carries one, so
+ * `agent-browser.exe` is tried as itself and not only as `agent-browser.exe.EXE`.
+ */
+export function binaryCandidateNames(binary: string, env: CliEnv, isWindows: boolean): string[] {
+  if (!isWindows) return [binary];
+  // No `.filter(Boolean)`: `getPathInfo` splits without one, so a trailing
+  // separator — ordinary on Windows — leaves a final empty extension that tries
+  // the name unsuffixed. Nothing runnable lives there, but dropping it would make
+  // the walk report missing where `which` returned a path.
+  const exts = (env.PATHEXT || WINDOWS_BIN_EXTS.join(';')).split(';');
+  if (binary.includes('.')) exts.unshift('');
+  return exts.map((ext) => `${binary}${ext}`);
+}
+
 export function resolveBinaryPath(binary: string, env: CliEnv): string | undefined {
   if (binary.includes('/') || binary.includes('\\')) return binary;
   const pathVar = env.PATH;
   if (!pathVar) return undefined;
   const isWindows = process.platform === 'win32';
-  const names = isWindows ? WINDOWS_BIN_EXTS.map((ext) => `${binary}${ext}`) : [binary];
+  const names = binaryCandidateNames(binary, env, isWindows);
   for (const dir of pathVar.split(isWindows ? ';' : ':')) {
     if (!dir) continue;
     for (const name of names) {
       const candidate = `${dir}${isWindows ? '\\' : '/'}${name}`;
-      if (existsSync(candidate)) return candidate;
+      if (isExecutableFile(candidate, isWindows)) return candidate;
     }
   }
   return undefined;
 }
 
+// Narrow, now that an unresolvable name never reaches the spawn: this catches a
+// binary that disappeared between the PATH walk and the spawn, plus a stub exec's
+// injected ENOENT.
 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.
+ * `resolveBinaryPath` already produced for it. Every "not found" answer ends the
+ * call here rather than at the spawn, because the spawn's own fallback is the
+ * bare name and cross-spawn resolves that against the cwd first on Windows.
  */
-function agentBrowserIsMissing(binary: string, env: CliEnv, resolvedPath: string | undefined): boolean {
+export 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;
+  // Bare name: resolvedPath is the PATH walk's result. With no PATH to search
+  // there is nowhere the binary could legitimately be, and falling through to
+  // the spawn would hand cross-spawn a bare name — whose `which` searches the
+  // cwd first on Windows, the one thing `execTarget` exists to prevent. So an
+  // absent PATH is "missing", not "ambiguous".
+  if (!env.PATH) return true;
   return resolvedPath === undefined;
 }