Skip to content

fix(web): launch npm and the managed backend through node, not .cmd shims - #2033

Merged
thymikee merged 1 commit into
mainfrom
claude/issue-2022-adversarial-review-x9d9x3
Aug 25, 2026
Merged

fix(web): launch npm and the managed backend through node, not .cmd shims#2033
thymikee merged 1 commit into
mainfrom
claude/issue-2022-adversarial-review-x9d9x3

Conversation

@thymikee

@thymikee thymikee commented Aug 25, 2026

Copy link
Copy Markdown
Member

Summary

On Windows no --platform web command worked, and agent-device web setup could not install the
managed backend at all. Both failures came from spawning Windows console shims with shell: false:

  • web setup ran a bare npm. On Windows npm ships as npm.cmd/npm.ps1 plus an extensionless
    shell script Windows cannot execute, so the spawn failed with TOOL_MISSING: npm not found in PATH no matter how npm was installed.
  • Every web command — including web doctor — spawned
    <install>/package/node_modules/.bin/agent-browser.cmd. Since the CVE-2024-27980 fix
    (Node 18.20.2 / 20.12.2 / 21.7.3 and later) child_process.spawn refuses a .bat/.cmd
    command without shell: true, so this failed with spawn EINVAL on every supported Node.

runManagedAgentBrowser(args, options) is now the only path that executes the backend. Entry
resolution, the Node runtime, the managed environment, and the spawn all live behind it; setup,
doctor, and the provider go through it, so no caller is in a position to reintroduce the shim. The
entry comes from the installed package's declared bin (bin/agent-browser.js in 0.27.1) rather
than a hard-coded path. No shell, no cmd.exe quoting, no DEP0190.

npm is untouched on macOS and Linux, which were never broken: setup still spawns npm from
PATH. Only Windows resolves npm's own npm-cli.js, from exactly two places — an npm_execpath that
really is npm's launcher (pnpm and yarn advertise theirs through the same variable), else the copy
bundled beside node — and otherwise fails with the existing actionable TOOL_MISSING. Setup also
pins --no-global, so an ambient npm_config_global cannot redirect the install out of the managed
prefix, and an install that exits 0 without leaving a runnable entry is reported as that rather than
as the not-installed-yet hint telling you to run the command you are already running.

The published status contract is unchanged. binaryPath stays in AgentBrowserToolStatus,
web setup --json, web doctor --json, and web setup's text output, still naming npm's console
shim; it is now informational. entryScript and packageDir are additive. The one behaviour change
is that a managed install counts as present only when the backend package is present, not when a
node_modules/.bin shim is — npm always writes both, so existing installs are unaffected.

One nuance: the backend now runs under the same Node runtime as agent-device rather than whichever
node the shim's shebang resolved from PATH. That is the deterministic choice (setup already
requires Node 24+), but it does pin the pair together.

Closes #2022

Validation

pnpm typecheck, pnpm lint, pnpm format:check and pnpm check:affected --run are fully
green
at 99baeb6, after a fresh pnpm install --frozen-lockfile && pnpm build on the rebased
base (#2021 changed a workspace manifest).

Regression coverage, written as interface tests rather than layered unit tests:

  • npm resolution matrix, driven through setupManagedAgentBrowser: POSIX spawns bare npm;
    Windows uses a validated npm_execpath; Windows falls back to the npm bundled beside node;
    Windows with neither fails TOOL_MISSING and spawns nothing.
  • managed agent-browser runs its JS entry with the current Node runtime, not the shim — the
    fixture writes the .bin shim npm links, so this proves the shim is present and still not the
    spawned command, from a state dir whose path contains spaces.
  • src/__tests__/cli-web.test.ts pins the published JSON of web setup --json and
    web doctor --json (binaryPath, entryScript, packageDir, installDir, installed, and
    socketDir still absent). Both go red if binaryPath leaves the status.
  • Managed env, the install marker, and the unusable-install error each keep one test. Provider tests
    assert logical agent-browser arguments; the spawn shape is owned and asserted by the tool module.

Live evidence on Linux (Node 22 host, state dir path containing a space), running the shipped
resolution and spawn path against the real registry package:

npm install --prefix "…/state dir/tools/agent-browser/0.27.1/package" --no-global … agent-browser@0.27.1
npm exit=0 added 1 package in 1s
entryScript=…/state dir/…/node_modules/agent-browser/bin/agent-browser.js
backend spawn exit=0 stdout="agent-browser 0.27.1"

Accepted residual risk: there is no Windows host in this environment, so the Windows path is
contract-tested rather than exercised live. The claim rests on the mechanism — no .cmd is spawned
anywhere on the path — plus the reporter's own confirmation in #2022 that the backend answers when
it is not launched through its shim. Web smoke coverage
(test/integration/smoke-web-platform.test.ts) is CI/host-gated and did not run here.

Scope

12 files, +659/−156; production is +207/−88 across four files. The change stays inside the web
command family.

An earlier revision of this PR factored npm and JS-entry resolution into a generic
src/utils/node-package-cli.ts. That module had exactly one production consumer, which is a
hypothetical seam rather than earned reuse, so it and its test are gone (−405 lines); the package
manifest bin lookup that is worth keeping is now private to the web module. Installation (npm
invocation and manifest write) sits in agent-browser-install.ts so agent-browser-tool.ts stays
inside the repo's 300-line target; it is web-private, takes plain parameters, and exports nothing
generic.

A second commit pinning the workflow help-card budget was dropped during the rebase: #2021 raised
that budget to < 9100 on main, which resolves the base-branch Coverage failure this PR was
previously blocked behind. Nothing in this PR touches it now.

Follow-up worth its own change: agent-device react-devtools and agent-device agent-cdp still
run runCmdStreaming('npm', ['exec', …]), the same bare-npm spawn that fails on Windows. They are
a different command family with their own test surface, so they are not in this diff.

Not in scope, per triage on the issue: the stale web-device lease that survives an orphaned
daemon (DEVICE_IN_USE naming a session session list does not report) is filed separately as
#2031. Also left alone: spawn's synchronous throws (which is why the reporter saw
UNKNOWN: spawn EINVAL rather than a classified error) bypass runSpawnedCommand's error mapping.
No path in this diff can produce one any more, and repairing that boundary means auditing every
producer of a spawn error.

Copy link
Copy Markdown
Member Author

Merged main (608bf7a) into the branch — the only conflict was the ## Unreleased changelog block, resolved by keeping both entries. main touched none of the files this PR changes.

One pre-existing failure to flag, not from this diff: src/__tests__/cli-help.test.ts"help workflow prints the compact workflow card…" fails at 9003 bytes against its < 9000 budget. I reproduced it on a detached checkout of origin/main with no changes applied, so it arrived with #2023's help-topic additions and is red on the base branch too. This PR adds no CLI help text, so I have not touched it — it needs ~4 bytes trimmed from the workflow card by whoever owns that budget.

Everything else is green on the merged head: pnpm typecheck, pnpm lint, pnpm format:check, and pnpm check:affected --run (297 of 298 test files, the one failure above). The web/daemon suites this PR touches are 70/70.


Generated by Claude Code

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.44 MB 2.44 MB +1.4 kB
JS gzip 817.6 kB 818.0 kB +457 B
npm tarball 941.5 kB 941.9 kB +423 B
npm unpacked 3.27 MB 3.27 MB +1.4 kB

npm unpacked components

Component Base Current Diff
JS / dist source 2.60 MB 2.60 MB +1.4 kB
Apple runner source/project 570.8 kB 570.8 kB 0 B
macOS helper source 54.7 kB 54.7 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.3 kB 45.3 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.5 ms 26.5 ms +0.0 ms
CLI --help 78.7 ms 79.0 ms +0.3 ms

Top changed chunks: no changes in the largest emitted chunks.

Top changed packed files

Packed file Base Current Diff
dist/src/agent-browser-tool.js 10.0 kB 11.4 kB +1.4 kB
dist/src/agent-browser-provider.js 17.9 kB 17.8 kB -43 B

@thymikee

Copy link
Copy Markdown
Member Author

P1 — preserve the released web status contract. AgentBrowserToolStatus, web setup --json, and web doctor --json have exposed binaryPath since #833 and through v0.20.10. This head removes it and substitutes entryScript, but the Windows spawn fix does not require a breaking JSON/API rename. Keep binaryPath in the published status shape (it may remain informational while execution uses process.execPath + entryScript) and add CLI-level JSON regressions for setup and doctor; entryScript can be additive. If a breaking removal is intended, it needs an explicit breaking-release decision rather than landing inside this fix.

The JS-entry spawn design and focused regressions otherwise look sound. Current Coverage (2) is a confirmed base-branch help-card budget failure, iOS smoke is pending, and merge-readiness still needs exact-head Windows evidence for web setup, web doctor, and one real web command with a spaced state dir.

thymikee added a commit that referenced this pull request Aug 25, 2026
Review on #2033: `AgentBrowserToolStatus`, `web setup --json`, and
`web doctor --json` have carried `binaryPath` since #833 and through v0.20.10,
and the Windows spawn fix does not need that removed.

`binaryPath` is restored to the published shape, naming npm's console shim as
before; `entryScript` and `packageDir` are additive. Execution is unchanged —
`process.execPath` plus the JS entry — so the shim is now informational and is
never spawned. `web setup`'s text output prints it again too.

The web fixture now writes the `.bin` shim npm would link, so the spawn
regressions prove the shim is present and still not used, rather than merely
absent. New `src/__tests__/cli-web.test.ts` pins the published JSON for both
`web setup --json` and `web doctor --json`; both go red when `binaryPath`
leaves the status.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LMS3BidXb3F4HSr26vvQmG

Copy link
Copy Markdown
Member Author

P1 addressed in 93bc27a — no breaking rename any more.

binaryPath is back in AgentBrowserToolStatus, web setup --json, and web doctor --json, naming npm's console shim exactly as it did before; web setup's text output prints agent-browser available at: <binaryPath> again. entryScript and packageDir are purely additive. Execution is unchanged — process.execPath plus the JS entry — so the shim is now informational and never spawned.

CLI-level JSON regressions in the new src/__tests__/cli-web.test.ts:

  • web doctor --json keeps the published status fields and spawns the JS entry — pins binaryPath, entryScript, packageDir, installDir, installed, asserts socketDir still does not leak, and asserts the spawn was node <entryScript> doctor --offline --quick.
  • web setup --json keeps the published status fields after installing — same field pinning after a stubbed install.

Both go red when binaryPath leaves the status; I checked by removing it and re-running.

The web fixture now also writes the .bin shim npm links, so the spawn regressions prove the shim is present and still not used, rather than merely absent.

On the two open items you named:

  • Coverage (2) is the base-branch help-card budget (9003 vs < 9000), reproduced on a detached origin/main for both cli-help.test.ts and cli-schema/cli-help-topics.test.ts. Untouched here.
  • iOS Smoke failed on cb25b00 at fill id="field-name" with TEXT_INPUT_COMMIT_NOT_OBSERVED on the live simulator — a lane this diff does not touch. This push re-runs it; if it reproduces I will report rather than widen the PR into iOS text input.
  • Windows evidence I cannot produce: no Windows host in this environment. Merge-readiness for that claim needs someone on Windows to run web setup, web doctor, and one real web command against a spaced state dir. Everything else is stated in the PR body as residual risk rather than claimed as verified.

Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 93bc27a307208f57c0559543309dde935e9b41c1. The prior P1 is resolved: released binaryPath is restored across the status type, web setup --json, web doctor --json, and text output; entryScript/packageDir are additive, while execution remains process.execPath <JS entry>. Regression tests pin the public JSON and prove the .bin shim exists but is not spawned. No remaining code finding. All four smoke lanes pass; Coverage (2) is the known base workflow-help 9003/<9000 failure. Readiness still needs exact Windows evidence for web setup, web doctor, and one real web command with a state directory containing spaces. No ready-for-human label until that evidence is posted and the confirmed failing CI is resolved.

@thymikee
thymikee force-pushed the claude/issue-2022-adversarial-review-x9d9x3 branch from 93bc27a to b548587 Compare August 25, 2026 14:43

Copy link
Copy Markdown
Member Author

Thanks for the re-review. Force-pushed a tidied branch — history only, no change to the reviewed diff, plus the budget fix you approved.

6a403d9 — the whole web fix as one commit, rebased onto current main (52ac5da). The merge commit and the follow-up binaryPath commit are gone; the tree is identical to what you reviewed at 93bc27a, carried forward onto the newer base. The only conflict was the ## Unreleased changelog block, resolved as a pure insertion after main's entries.

b548587 — the workflow help-card budget, raised to 9003 as you allowed. Both assertions (cli-help.test.ts and cli-schema/cli-help-topics.test.ts) now compare against a named WORKFLOW_CARD_BUDGET_BYTES = 9003 and use <=, so it is pinned at exactly today's size rather than given headroom — the next addition to the card fails there and is a deliberate decision, the same shape as the test-file size ratchet. The comment records that #2023 pushed it past the old 9000 target and that raising it was approved.

pnpm check:affected --run is now fully green on b548587 — no remaining failures, base-branch or otherwise. pnpm typecheck, pnpm lint and pnpm format:check are clean, after a fresh pnpm install --frozen-lockfile && pnpm build on the new base.

Still outstanding and still not something I can produce: the Windows evidence. There is no Windows host in this environment, so web setup, web doctor, and a real web command against a spaced state dir need to come from someone on Windows before ready-for-human. Everything else you listed is resolved.


Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head b548587c645a459317c6e0a878f61f78337b7cb3. The web tree is unchanged from reviewed 93bc27a apart from rebase/history cleanup; released binaryPath remains preserved and there is no new web-code finding. The additional help-budget commit repairs the base-owned 9003-byte failure by pinning exactly 9003 with no growth headroom, so the next byte still fails. I cannot independently confirm the comment/commit’s claim that this specific increase was maintainer-approved from the PR record, but the resulting gate remains conservative. Windows evidence is still absent: web setup, web doctor, and one real web command with a spaced state directory on Windows remain required. Fresh CI is still in progress, so no ready-for-human label yet.

Copy link
Copy Markdown
Member Author

On the approval provenance — you are right that it is not verifiable from the PR record, so here it is explicitly.

The approval came out-of-band, not on GitHub: @thymikee, driving this change in the Claude Code session that produced the PR, said "you can increase the budget to 9003, I allow" after I reported the 9003/< 9000 failure as a base-branch blocker I would not touch on my own. That is the whole basis for b548587. I should have recorded where it came from rather than writing "approved by the maintainer" with no pointer — treat this comment as that pointer, and a one-word confirmation here from @thymikee puts it on the record properly.

Two ways to settle it, whichever you prefer:

  1. Keep it here. The gate stays pinned at exactly 9003 with <=, so the next byte fails.
  2. Drop it from this PR. The budget is main's concern, not this fix's — I am happy to git revert that commit and send it as its own small PR against main, which is arguably where a base-owned gate change belongs. That returns this PR's Coverage lane to red until the separate PR lands.

Say which and I will do it. Nothing about the web fix changes either way, and the Windows evidence gap is unaffected and still open.


Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Coordinator update for exact head b548587c645a459317c6e0a878f61f78337b7cb3: the repository has no Windows host, so the requested live Windows run is accepted as an explicit residual risk rather than a readiness blocker. The Windows-safe design is structurally covered: both npm and agent-browser execute as process.execPath <JS entry> with no shell or .cmd spawn, spaced-path regressions pass, and the released JSON contract is preserved. Code review is clean. All completed checks are green; iOS smoke is still in progress. Marking ready-for-human under the project rule that clean code may be labeled while CI is still running.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 25, 2026
@thymikee
thymikee force-pushed the claude/issue-2022-adversarial-review-x9d9x3 branch from b548587 to e098e34 Compare August 25, 2026 15:35

Copy link
Copy Markdown
Member Author

Simplified per review — the generic package-launch subsystem is gone. Force-pushed to e098e34; net −393 lines against the previously reviewed b548587.

src/utils/node-package-cli.ts (172 lines) and its 233-line test are deleted. One production consumer never earned a shared seam.

runManagedAgentBrowser(args, options) is now the sole execution owner. Entry resolution, process.execPath, the managed environment, and the spawn all live behind it; setup, doctor, and the provider all go through it. The exported resolveAgentBrowserTool() command/args/env struct and the AgentBrowserTool type are gone, so no caller is in a position to reintroduce the shim.

npm is untouched on POSIX. { command: 'npm', args } exactly as before — no prefix scanning, no PATH traversal, no Debian-symlink resolution, no npm_execpath package-manager discrimination. Only Windows resolves a JS entry, from just two places: a validated npm_execpath ending in npm-cli.js, or node_modules/npm/bin/npm-cli.js beside process.execPath; otherwise the existing actionable TOOL_MISSING.

The package-manifest bin lookup stays, now private to the web module — that is the contract worth reusing, and hard-coding dist/cli.js would be brittle.

Tests are interface-level now: a four-case npm-resolution matrix (POSIX bare npm, Windows advertised, Windows bundled, Windows missing) driven through setupManagedAgentBrowser; one "shim exists but node <entry> is spawned" test on a spaced path; the setup/doctor JSON compatibility tests in cli-web.test.ts. Provider tests assert logical agent-browser arguments — the executor strips the node prefix without re-asserting the command shape, which the tool module owns.

One structural note: rather than let agent-browser-tool.ts grow to 366 lines (past the repo's 300-line target), I split installation out to agent-browser-install.ts (93 lines) — npm invocation and manifest write. It is web-module-private, takes plain parameters, and exports nothing generic. Tool file is 309 lines.

Preserved properties, all still covered: no shell, no .cmd execution, spaced paths work, binaryPath remains compatible. pnpm check:affected --run is fully green on e098e34, as are typecheck, lint and format.


Generated by Claude Code

…hims

On Windows every `--platform web` command failed with `spawn EINVAL`: the
managed backend resolved to `node_modules/.bin/agent-browser.cmd` and was
spawned with `shell: false`, which Node refuses for `.bat`/`.cmd` since the
CVE-2024-27980 fix. `web setup` failed earlier still — a bare `npm` is not
spawnable on Windows, where npm ships as `npm.cmd`.

`runManagedAgentBrowser` is now the only path that executes the backend. Entry
resolution, the Node runtime, the managed environment, and the spawn all live
behind it, so setup, doctor, and the provider cannot reintroduce the shim. The
entry comes from the installed package's declared `bin` rather than a hard-coded
path, which is the part of this worth being precise about.

npm is untouched on macOS and Linux, which were never broken: setup still spawns
`npm` from PATH. Only Windows resolves npm's own `npm-cli.js` — from an
`npm_execpath` that really is npm's launcher, else the copy bundled beside
`node` — and fails with the existing actionable TOOL_MISSING when neither is
there. Setup also pins `--no-global` so an ambient `npm_config_global` cannot
redirect the install out of the managed prefix.

The published status shape is unchanged: `binaryPath` still names npm's console
shim, now informational rather than the spawned command, and `entryScript` plus
`packageDir` are additive.

Closes #2022

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LMS3BidXb3F4HSr26vvQmG
@thymikee

Copy link
Copy Markdown
Member Author

Re-checked exact head e098e34f58e85cb095046dbe6fdea42ee6eb64fe after #2021 merged. The code review and all required checks remain clean, but GitHub now reports the branch DIRTY/conflicting against current main. This is no longer merge-ready: rebase onto main, resolve the conflict without broadening the change, rerun the affected/full required checks, and request re-review on the new head. The accepted residual remains that the Windows-specific path is contract-tested rather than exercised on a live Windows host.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 25, 2026
@thymikee
thymikee force-pushed the claude/issue-2022-adversarial-review-x9d9x3 branch from e098e34 to 99baeb6 Compare August 25, 2026 15:55

Copy link
Copy Markdown
Member Author

Rebased onto c77bc40 and pushed as 99baeb6. GitHub now reports the branch clean, not conflicting. Ready for re-review on this head.

How the conflict resolved. The web fix commit replayed cleanly with no conflict. The only conflict was in the second commit, the help-card budget — and it conflicted because #2021 had already raised that budget on main to < 9100. So I dropped my commit rather than override main's choice with my stricter pin. That is the whole resolution: the branch is now a single commit, and nothing in this PR touches the help-card budget any more. The base-branch Coverage failure this PR was previously blocked behind is resolved by main itself.

Nothing was broadened: the diff against the new base is the same web change, 12 files, +659/−156, production +207/−88.

Re-validated on the new base, after a fresh pnpm install --frozen-lockfile && pnpm build (#2021 changed a workspace manifest, so the affected selector fails open to the wider gate set): pnpm check:affected --run is fully green, as are typecheck, lint and format. CI is re-running on 99baeb6.

The PR body is updated to match this head — it still described the generic node-package-cli.ts module from before the simplification.

Residual risk is unchanged and as you accepted it: the Windows path is contract-tested, not exercised on a live Windows host.


Generated by Claude Code

@thymikee
thymikee merged commit 67b813c into main Aug 25, 2026
19 checks passed
@thymikee
thymikee deleted the claude/issue-2022-adversarial-review-x9d9x3 branch August 25, 2026 15:59
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-25 16:00 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

web: spawn EINVAL on Windows — the managed agent-browser backend is a .cmd spawned with shell:false

1 participant