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

[dormouse-bot]

dor ab resolved agent-browser to an absolute path on PATH, forwarded that
path to the host, and then spawned the bare name anyway at both of its call
sites. spawnAndCapture uses cross-spawn, whose which searches
process.cwd() before PATH on Windows — and dor inherits the pane's
working directory. So an agent-browser.cmd committed to a repository the user
cloned executed on their next dor ab, which dor skill mandates for every
page view.

docs/specs/security.md -> "What is not defended" accepts "a process running as
you", and this is not that: nothing in the cloned repository was running.
docs/specs/dor-tool.md -> Trust already treats repo-controlled content as a
boundary, keeping dormouse.yml inert until granted; this path crossed it with
no gate.

What the branch does

0be792c3 spawns the path already computed fifteen lines earlier. Three review
rounds then closed the consequences of promoting that path from a hint to the
thing that executes — its resolution semantics now matter, and they diverged
from the resolver they replaced:

Commit
0be792c3	Spawn binaryPath at both call sites.
ce8ffd25	Skip a directory or non-executable file rather than returning it, and take the Windows extension list from PATHEXT.
1618c9ae	Mirror which@2's getPathInfo exactly — npm's .EXE;.CMD;.BAT;.COM fallback (not cmd.exe's order), `
9aa2c665	Scope the parity invariant to the PATH directories — which itself searches the cwd, so an unscoped rule licenses the hijack — and pin the executability test's three halves separately.

Verification

• node --test dor/test/cli-output.test.mjs — 146 pass, 1 skipped (the skip
  is a process.platform-gated Windows case, and is not the pin for any rule).

• Every rule mutation-checks red on Linux, which took a redesign: the Windows
  ordering rules were Windows-only and this suite runs on Linux in CI, so the
  first attempt at pinning them passed vacuously. binaryCandidateNames and
  isExecutableFile take isWindows as an argument rather than reading
  process.platform, so both branches are reachable from the suite that runs.

  Mutation	Result
  exec(binary, …) — spawn the bare name	143 / 2
  drop statSync(candidate).isFile()	144 / 2
  drop accessSync(candidate, X_OK)	144 / 2
  drop the isWindows early return	145 / 1
  ?? instead of `
  cmd.exe's .COM-first order	144 / 1
  drop the empty-extension unshift	144 / 1
  absent PATH reported as ambiguous	144 / 1

• node scripts/spec-lint.mjs reports only the two pre-existing
  standalone/sidecar/node_modules path hits every dependency-free checkout
  shows. docs/specs/dor-cli.md re-baselined 6250 -> 6550 across the four
  commits.

Scope

The host's runWithBinaryFallback (lib/src/host/agent-browser-host.ts) still
ends its candidate list with the bare DEFAULT_AGENT_BROWSER_BIN. Its cwd is
the extension-host or Tauri-app directory rather than a directory users clone
into, and sharing the resolver means moving it from dor to dor-lib-common,
so it is recorded under docs/specs/dor-cli.md -> ## Future rather than folded
in here. The audit also named bare-name spawns of system binaries in
standalone/sidecar/pty-core.js and clipboard-ops.js; those sit outside this
spec section's "external/user-installed binary" scope, and pty-core.js:373,419's
%SystemRoot%\System32 join is the pattern they want.

Provenance: how this reached a PR instead of the audit issue

Found by the repo's own security-audit run
35432996343 as
CA-13, the single BLOCKER deciding that run's VERDICT: FAIL. It is not
readable on #598: the merged
report was 315,730 characters, clamp-issue-body.mjs keeps the head, and
audit-application.md is both last in AUDIT_FRAGMENTS and 90% of the body —
so the comment's own "read that domain's section first" points at a section
truncation removed. Recovered from the run's audit-transcript artifact.

Each link of the chain re-verified against this checkout before writing the fix:
exec(binary, ...) at dor/src/commands/agent-browser.ts:260,275 (pre-fix
line numbers) against binaryPath at :245; the walk reading env.PATH only;
spawnAndCapture -> cross-spawn at dor-lib-common/src/spawn.ts:1,43;
getPathInfo in which/which.js at 2.0.2 for the cwd-first comment and the
whole extension contract; and agentBrowserIsMissing refusing to spawn when the
walk finds nothing, which is why the hijack needs a legitimate install rather
than being blocked by one.

Opened by the daily review-runs sweep; the reporting-path defect is recorded
on #511 and described on
#598.

[cloudflare-workers-and-pages]

Deploying mouseterm with    Cloudflare Pages

Latest commit:	4af22b0
Status:	✅  Deploy successful!
Preview URL:	https://7fe440f5.mouseterm.pages.dev
Branch Preview URL:	https://daily-review-runs-3544178685.mouseterm.pages.dev

View logs

[dormouse-bot]

Draft feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

The spawn target moves from cross-spawn's resolution to resolveBinaryPath's hand-rolled PATH walk, and those two don't resolve the same file. The walk tests candidates with existsSync in a fixed WINDOWS_BIN_EXTS order (.cmd, .exe, .bat), while the bare-name spawn it replaces went through cross-spawn to which, which requires an executable file and orders Windows candidates by PATHEXT (default .COM;.EXE;.BAT;.CMD;…). Two observable differences: a PATH directory holding both agent-browser.exe and agent-browser.cmd used to run the .exe and now runs the .cmd (and a customised PATHEXT is ignored outright); and a non-executable file — or a directory — named agent-browser earlier on PATH used to be skipped by which in favour of the real install further along, where now it becomes execTarget and the spawn fails with a raw EACCES/EISDIR instead. The second one is not Windows-specific.

That divergence was inert while binaryPath only proved the install present and travelled to the host as a hint; promoting it to the thing that executes is what makes it matter, and neither the spec rule nor the rationale mentions that resolution semantics moved with it. Worth either matching which more closely in the walk (honour env.PATHEXT when set, and check executability rather than mere existence) or stating in docs/specs/dor-cli.md that dor ab resolves by its own fixed extension order — as written, a reader takes "the PATH-resolved absolute path" to mean the path PATH resolution would have produced anyway.

[dormouse-bot]

Taken, both halves — the divergence is real and the second one isn't
Windows-specific, which is the sharper half.

ce8ffd25 makes the walk agree with which rather than documenting that it
doesn't:

• Skip anything that is not an executable regular file. A directory or a
  non-executable agent-browser earlier on PATH is now walked past to the real
  install, where before this commit it became execTarget and the spawn failed
  EACCES/EISDIR. On Windows the extension decides executability, so being a
  regular file is the whole test there.
• Order Windows candidates by PATHEXT when set, else by cmd.exe's own
  default (.com, .exe, .bat, .cmd) — which is also which's fallback, so
  the .exe-vs-.cmd case resolves as it did before the fix. The old fixed list
  put .cmd first, which was the inversion you found.

Pinned by skips a PATH entry that is not an executable file, mutation-checked
in the direction it points: reverting isExecutableFile to existsSync takes
the suite to 142 pass / 1 fail. 143/143 as shipped.

docs/specs/dor-cli.md now carries the agreement as its own rule beside the
spawn rule, rather than leaving a reader to infer that "PATH-resolved" means
what PATH resolution would have produced — your point about the reader's
reading. The why went to the rationale.

[dormouse-bot]

Draft feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

The executability test lands, and so does the PATHEXT-when-set half. The
fallback half doesn't: which@2.0.2 hardcodes '.EXE;.CMD;.BAT;.COM' — its
getPathInfo reads opt.pathExt || process.env.PATHEXT || '.EXE;.CMD;.BAT;.COM'
— which is npm's own list, not cmd.exe's .COM;.EXE;.BAT;.CMD. So the
constant's comment, the spec bullet, and the commit message each assert an
agreement that doesn't hold, and two orderings invert against the resolver being
replaced: a directory holding both agent-browser.com and agent-browser.exe
runs the .com where cross-spawn ran the .exe, and .bat-vs-.cmd flips the
same way. The .exe-vs-.cmd case from the last round does land right — both
lists put .exe first — so that conclusion was correct and the reason given for
it wasn't.

Two more on the same expression:

• env.PATHEXT ?? … keeps an empty PATHEXT, so exts is [], the walk
  resolves nothing, and dor ab fails with "agent-browser is not installed"
  while the install sits on PATH. which uses || there and falls back to
  its default.
• A bare name that already carries an extension — DORMOUSE_AGENT_BROWSER_BIN=agent-browser.exe,
  no separator, so resolveBinaryPath walks instead of returning it verbatim —
  is searched as agent-browser.exe.EXE, agent-browser.exe.CMD, … and never
  as itself, giving the same false "not installed". which unshifts '' onto
  pathExt when the command contains a . (its cmd.indexOf('.') !== -1
  branch). That one predates this PR; what's new is the rule claiming it can't
  happen.

Narrower, at the spawn itself: agentBrowserIsMissing returns false when
env.PATH is unset — "ambiguous, let the spawn decide" — and binaryPath ?? binary then spawns the bare name, which on Windows is the cwd-first path this
PR closes rather than the ENOENT the spec bullet says the fallback is for.
Treating "no PATH to search" as missing would make the fallback match its
stated purpose; today it is the one route back to the primitive.

[dormouse-bot]

All four taken, and the first one was the important correction — I verified
getPathInfo in which/which.js at 2.0.2 rather than reasoning from the shell,
and you're right on every point:

const pathExtExe = isWindows
  ? opt.pathExt || process.env.PATHEXT || '.EXE;.CMD;.BAT;.COM'
  : ''
const pathExt = isWindows ? pathExtExe.split(colon) : ['']
if (isWindows) {
  if (cmd.indexOf('.') !== -1 && pathExt[0] !== '')
    pathExt.unshift('')
}

So in 1618c9ae: the list is npm's .EXE;.CMD;.BAT;.COM, read with || so an
empty PATHEXT falls back, with an empty extension unshifted when the name
carries a .. The constant's comment, the spec bullet and the commit message now
say npm's list and cite getPathInfo instead of claiming cmd.exe parity.

The agentBrowserIsMissing hole is the one I'd least have wanted to ship: an
unresolvable bare name is now a missing install on every route, including no
PATH at all, so the fallback can no longer be the way back to the primitive.

The part worth flagging back

Your three extension findings were unpinnable as I'd written them — every rule is
Windows-only, this suite runs on Linux in CI, and I had just added a test that
passed vacuously there. I checked before claiming otherwise: mutating each of
the three left the suite at 145/0.

So the ordering logic is now an exported binaryCandidateNames(binary, env, isWindows) and the test drives the Windows branch through the argument. Each of
the six rules on this branch mutation-checks red on Linux:

Mutation	Result
exec(binary, …) — spawn the bare name	143 / 2
existsSync instead of the executability test	144 / 1
?? instead of `
cmd.exe's .COM-first order	144 / 1
drop the empty-extension unshift	144 / 1
absent PATH reported as ambiguous	144 / 1

Baseline 145 pass / 1 skipped (the remaining skip is a genuinely
process.platform-gated case-sensitivity check, and it is not the pin for any
rule). CI green at 1618c9ae.

[dormouse-bot]

Draft feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

The which mirror holds on the extension rules now. Three things left, two of them in the prose rather than the code.

The new invariant states the opposite of the fix, in the one case that motivated it. "The walk must select the file which would" — which's Windows branch prepends process.cwd() to the search path, so the file it would select is the agent-browser.cmd in the cloned repository. An editor who takes the rule literally and makes the walk agree reintroduces exactly the primitive the bullet above it closes, and the spec is what they would be reading. Scope it to the PATH directories. The suggestion below is word-neutral against the 6500 budget when paired with the next one (the file is at 6496 today, so an unpaired addition needs a re-ratchet).

The bare-name fallback is unreachable now, so the spec and the comment both describe a path that cannot be taken. Making an unresolvable bare name "missing" on every route — including no PATH at all — means agentBrowserIsMissing returns true before the spawn in every case where binaryPath is undefined, and an explicit path comes back from resolveBinaryPath verbatim so it is never undefined. binaryPath ?? binary can only take the binary branch when a test injects execAgentBrowser and skips the check. Keeping the ?? as a type-level belt is fine; "absence still surfaces as ENOENT" and "which is the ENOENT path below" are not — absence surfaces as missingBinaryMessage, and isMissingBinaryError now only catches a binary that disappears between the walk and the spawn.

Half the executability rule is unpinned. The mutation table reads existsSync instead of the executability test → 144/1, but the directory shadow alone accounts for that: statSync(...).isFile() already rejects a directory. Delete only accessSync(candidate, constants.X_OK) and the suite stays green — so the non-executable-file half of "skips a directory or a non-executable file", the half that is not Windows-specific, is the unenforced claim that binaryCandidateNames's isWindows parameter exists to avoid. A second POSIX-only shadow in the same test pins it.

Same shape, smaller: isExecutableFile's Windows branch reads process.platform directly, so "on Windows the extension decides executability, so being a regular file is the whole test" is equally unreachable from Linux CI — the same isWindows parameter would enforce it.

[dormouse-bot]

All three taken in 9aa2c665, and the first is the one that mattered most —
a spec bullet that licenses the vulnerability is worse than no bullet.

Scope. which's Windows branch prepends process.cwd(), so "select the file
which would" read literally is an instruction to reintroduce the primitive. The
rule is now scoped to the PATH directories, with Never extend the search to
the cwd stated as its own imperative rather than left to be inferred from the
bullet above.

The unreachable fallback. Correct, and I traced it the same way: with the
walk's failure reported before the spawn, binaryPath is undefined only when
agentBrowserIsMissing has already ended the call, and an explicit path comes
back verbatim. So neither the spec nor the comment claims ENOENT any more — the
?? is documented as a type-level belt reachable only through an injected stub,
and isMissingBinaryError is annotated with what it actually still catches (a
binary that disappears between the walk and the spawn).

The unpinned half. You were right that my table over-claimed: statSync().isFile()
alone accounted for the 144/1, so the X_OK probe was load-bearing in the prose
only. isExecutableFile now takes isWindows like binaryCandidateNames, a unit
test drives both branches, and the walk test gained a POSIX non-executable-file
shadow so the probe is load-bearing end-to-end too. The three halves separate
cleanly:

Mutation	Result
drop accessSync(candidate, X_OK)	144 / 2
drop the isWindows early return	145 / 1
drop statSync(candidate).isFile()	144 / 2

Baseline 146 pass / 1 skipped; the skip is the Windows-only
already-carries-an-extension case and is not the pin for any rule. The one
assertion that genuinely cannot be platform-free is the POSIX executable bit,
since accessSync(X_OK) reports every readable file as executable on Windows —
the spec says so rather than leaving it as an unexplained gate.

CI green at 9aa2c665. Three rounds in, each one found something real, so I'd
rather hand it over here than keep iterating on my own PR: the branch is a
security fix plus its which parity, and nedtwigg should see it before it
grows further. Still a draft.

[dormouse-bot]

Draft feedback on work in progress, not a merge verdict.

Two on the prose; the three halves of the executability rule do separate cleanly, and I found nothing to add on the code.

isExecutableFile is now a rule-bearer and isn't in the pointer list. This commit exported it, gave it the isWindows parameter and its own test, so skips a directory or a non-executable file lives in it rather than in resolveBinaryPath — but the Source of truth: beside that rule still names only the other three, leaving a reader one hop from the code the bullet describes. Suggestion below; it costs one word against a budget the file currently sits 14 under.

The walk drops PATHEXT entries which keeps. binaryCandidateNames ends in .filter(Boolean), while the getPathInfo you quoted splits with no filter — so PATHEXT=.COM;.EXE;.BAT;.CMD;, with the trailing separator that is ordinary on Windows, hands which a final empty extension and the walk none, and which then tries the extension-less agent-browser as a last candidate where the walk never does. Nothing runs differently: Windows won't execute an extension-less file, so the divergence only makes the walk report missing where which would have returned something unrunnable. But the bullet states the parity without qualification, and every other divergence on this branch is written down — so it wants a half-clause, or the filter dropped.

[dormouse-bot]

Both taken in 4af22b0e, and this is my last round on my own PR.

The PATHEXT one is the better catch of the two: I quoted getPathInfo and then
filtered where it doesn't. Dropped the .filter(Boolean) rather than qualifying
the invariant — the trailing separator is ordinary on Windows and keeping the
empty extension is what which does, so parity is now literal instead of
annotated. Pinned by a PATHEXT: '.EXE;' case; re-adding the filter takes the
suite to 145/1. isExecutableFile is in the Source of truth: list.

146 pass / 1 skipped, and the ten mutations on this branch each still land red.

[dormouse-bot]

Draft feedback on work in progress, not a merge verdict.

The trailing-empty-extension fix reads right. Two divergences from getPathInfo/whichSync are still unlisted, and the first has the user-visible consequence the invariant names.

which strips a quoted PATH entry; the walk doesn't. Both which and whichSync run const pathPart = /^".*"$/.test(ppRaw) ? ppRaw.slice(1, -1) : ppRaw on every entry before joining, ungated by platform. resolveBinaryPath joins dir verbatim, so a PATH written as C:\Windows\system32;"C:\Program Files\nodejs" — quoting an entry with spaces is ordinary on Windows, which is why which handles it — makes every candidate under that entry "C:\Program Files\nodejs"\agent-browser.cmd. A quote is not a legal filename character, statSync throws, isExecutableFile returns false, and the directory is skipped. If the only install lives there, agentBrowserIsMissing returns true and dor ab reports "agent-browser is not installed" against a working install that every other tool on the machine resolves. This one predates the branch, the same way the already-carries-an-extension case did; what's new is the invariant asserting the walk selects the file which would within the PATH directories, with reports a present install as missing named as the failure it rules out.

4af22b0e made the unshift diverge in the case the filter used to hide. getPathInfo guards it — if (cmd.indexOf('.') !== -1 && pathExt[0] !== '') pathExt.unshift('') — and with .filter(Boolean) in place exts[0] could never be empty, so the guard was moot. Without the filter, a leading separator (PATHEXT=';.EXE') plus a name carrying a . yields the empty extension twice where which yields it once. Nothing resolves differently; it costs one duplicate statSync and it makes the candidate list — which is asserted by exact equality in the Windows candidate list mirrors which(1) on every edge — no longer the list getPathInfo produces. Worth closing while the parity is literal rather than annotated.

Both fixes are below. Neither is pinned by the suite as it stands: the quoted-entry one wants a PATH entry wrapped in quotes in the walk test, and the guard one a PATHEXT: ';.EXE' case beside the '.EXE;' one this commit added — the same mutation-in-the-direction-it-points check as the other ten.

[dormouse-bot]

Suggested change

	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;
	}
	}
	for (const dir of pathVar.split(isWindows ? ';' : ':')) {
	if (!dir) continue;
	// `which` strips a surrounding pair of double quotes from a PATH entry
	// (`/^".*"$/`, in both its async and sync walks, ungated by platform):
	// quoting an entry that contains spaces is ordinary on Windows, and a quote
	// is not legal in a filename, so keeping them stats every candidate ENOENT.
	const entry = /^".*"$/.test(dir) ? dir.slice(1, -1) : dir;
	for (const name of names) {
	const candidate = `${entry}${isWindows ? '\\' : '/'}${name}`;
	if (isExecutableFile(candidate, isWindows)) return candidate;
	}
	}

[dormouse-bot]

Suggested change

	if (binary.includes('.')) exts.unshift('');
	// `which` skips the unshift when PATHEXT already begins with an empty entry
	// (`pathExt[0] !== ''`), so a leading separator yields the empty extension
	// once, not twice.
	if (binary.includes('.') && exts[0] !== '') exts.unshift('');