Skip to content

Fix browse daemon crash-loop diagnostics + a shell-injection bug found along the way - #2461

Closed
phuttimatebenchanakatkul wants to merge 1 commit into
garrytan:mainfrom
phuttimatebenchanakatkul:fix-daemon-crash-loop-diagnostics
Closed

phuttimatebenchanakatkul wants to merge 1 commit into
garrytan:mainfrom
phuttimatebenchanakatkul:fix-daemon-crash-loop-diagnostics

Conversation

@phuttimatebenchanakatkul

Copy link
Copy Markdown
Contributor

Summary

  • The detached browse daemon's stdout/stderr were wired to 'ignore' on every platform, so every console.error('[browse] FATAL: ...') from a crash (Chromium disconnect, uncaughtException, unhandledRejection) went nowhere — a restart was indistinguishable from any other cause of a dropped session, with nothing on disk recording why.
  • cli.ts: both the Windows and macOS/Linux spawn paths now redirect stdout/stderr to <stateDir>/browse-daemon.log (append mode, accumulates across the daemon's full lifetime and every respawn).
  • That immediately surfaced two real, previously invisible bugs, both fixed here:
    1. spawn bun ENOENT — bun-polyfill.cjs's Bun.spawn/spawnSync polyfill called Node's child_process.spawn with a bare command name ('bun'), which Windows can't resolve without PATHEXT lookup.
    2. browser?.process is not a function — resolveDisconnectCause() (the crash classifier itself) threw on some browser/connection-mode shapes, and being an unhandled fire-and-forget rejection, took the whole daemon down a second, harder-to-diagnose way.

A note on how the fix for #1 evolved

My first attempt used shell:true with hand-rolled double-quote escaping to fix the PATHEXT resolution. I verified empirically (not just reasoned about it) that this does not neutralize cmd.exe metacharacters (&|^%<>), and is reachable via browser-skill-commands.ts's ...opts.skillArgs passthrough from $B skill run <name> --arg k=v. Spawning a resolved .cmd path with an arg containing & echo INJECTED > proof.txt actually created the file.

Replaced it with cross-spawn — already resolved in node_modules transitively via @modelcontextprotocol/sdk, now added as a direct dependency — the established library for correct Windows PATHEXT resolution + argument escaping together. Re-verified the same injection payload no longer executes, and normal resolution (bun --version) still works.

Test plan

  • Killed the live Chromium child under a running daemon — log now shows a single clean FATAL: Chromium process crashed or was killed, terminal-agent respawns successfully, daemon auto-recovers to a healthy new PID
  • Re-ran the same test after reverting each fix individually to confirm each is load-bearing (not just cosmetically present)
  • Confirmed the shell-injection payload (& echo INJECTED > proof.txt) no longer executes after the cross-spawn fix
  • Confirmed normal resolution (bun --version via both spawn and spawnSync) still works, including paths/args containing spaces
  • 24/24 pre-existing browser-manager-unit tests pass
  • bun build --compile succeeds for both cli.ts and the Node server bundle

🤖 Generated with Claude Code

Root cause: the detached daemon's stdout/stderr were wired to 'ignore'
on every platform, so every console.error('[browse] FATAL: ...') from
a crash -- Chromium disconnect, uncaughtException, unhandledRejection --
went nowhere. A restart was indistinguishable from any other cause of a
dropped session; nothing on disk ever recorded why.

cli.ts: both the Windows and macOS/Linux spawn paths now redirect
stdout/stderr to <stateDir>/browse-daemon.log (append mode, accumulates
across the daemon's full lifetime and every respawn).

That immediately surfaced two real, previously invisible bugs:

1. "[browse] FATAL uncaught exception: spawn bun ENOENT" --
   bun-polyfill.cjs's Bun.spawn/spawnSync polyfill called Node's
   child_process.spawn with a bare command name ('bun'), which Windows
   can't resolve without PATHEXT lookup. First attempt used shell:true
   with hand-rolled double-quote escaping; verified empirically that this
   does NOT neutralize cmd.exe metacharacters (&|^%<>) and is exploitable
   via browser-skill-commands.ts's `...opts.skillArgs` passthrough --
   spawning a resolved .cmd path with an arg containing
   `& echo INJECTED > proof.txt` created the file. Replaced with
   cross-spawn (already resolved in node_modules transitively via
   @modelcontextprotocol/sdk, now a direct dependency) -- the established
   library for correct Windows PATHEXT resolution + argument escaping
   together. Re-verified: same injection payload no longer executes,
   normal resolution (bun --version) still works.

2. "[browse] FATAL unhandled rejection: browser?.process is not a
   function" -- resolveDisconnectCause() (the crash classifier itself)
   threw on some browser/connection-mode shapes, and that throw was
   unhandled (fire-and-forget void call from the 'disconnected' event),
   taking the whole daemon down a second, harder-to-diagnose way. Now
   wrapped in try/catch, logging the caught error (so a future different
   failure here stays diagnosable) and defaulting to the existing
   conservative 'crash' classification.

Verified end-to-end multiple times: killed the live Chromium child under
a running daemon, confirmed the log shows a single clean
"FATAL: Chromium process crashed or was killed", the terminal-agent
respawns successfully, and the daemon auto-recovers to a healthy new
PID. 24/24 pre-existing browser-manager-unit tests pass.
@trunk-io

trunk-io Bot commented Aug 5, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@garrytan

Copy link
Copy Markdown
Owner

Thank you — this was absorbed on main (credited in the v1.6x CHANGELOG entries; roster in PR #2604). Closing.

@garrytan garrytan closed this Aug 17, 2026
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.

2 participants