fix(browse): capture skill subprocess output via temp files, not pipes

[frederik-kaster-noygear]

Problem

$B skill test is flaky under load. In a full bun test browse/test/ run it intermittently returns only bun's version banner:

Expected substring or pattern: /13 pass|0 fail|tests passed/
Received: "bun test v1.3.12 (700fc117)\n"

The obvious reading is a read-ordering bug: handleTest awaited proc.exited before attaching readers, so it looked like it was reading a stream snapshot. That is not the cause. Fixing the ordering did not fix the flake.

Root cause

Under a loaded parent, the first piped spawn in a process intermittently yields an empty stderr even though the child wrote it and exited 0. Instrumenting a full suite run, spawning the same child three times in a row:

pipe#0 exit=0 outLen=28 errLen=0    <- lost
pipe#1 exit=0 outLen=28 errLen=76
pipe#2 exit=0 outLen=28 errLen=76

Reproduced in 4 of 4 instrumented runs. Two candidate workarounds were measured losing the same bytes in the same position, so neither is a fix:

• attaching readers before awaiting proc.exited and settling with Promise.all
• a manual stream.getReader() drain loop (what readCapped did)

Bun.spawnSync captures reliably, but blocking is not an option in this module: a spawned skill calls back into the same daemon on GSTACK_PORT, so a synchronous wait would deadlock it.

It surfaced in $B skill test because bun test splits its report across streams (banner to stdout, pass/fail summary to stderr) and handleTest returned stderr || stdout, so a dropped stderr degraded silently to the banner. Worth noting the capture was lossy on every run, not just failing ones: the banner was missing from successful captures too.

Fix

runToFiles() points the child's stdout/stderr at temp files via Bun.file(path), awaits exit, then reads the files. The kernel has flushed everything by the time the child exits, so the post-exit read is always complete. It also removes the pipe-buffer stall risk on chatty children.

spawnSkill had the same defect and now shares the helper rather than carrying a second copy of the fix. There the loss is worse than a test flake: a dropped read blanks a skill's JSON result on $B skill run while still reporting success.

Uses Bun.file(path) rather than raw fds from fs.openSync. Opening descriptors and closing them around a spawn puts the caller into Bun's fd bookkeeping and produced an intermittent EBADF: bad file descriptor, epoll_ctl on a later spawn, observed once in a full-suite run.

The tests passed for "<name>" fallback is removed. A passing bun test always prints a summary, so exit 0 with no output means the run was not captured; it now throws rather than returning an unverifiable success string. The old assertion's tests passed alternative matched that string, so it would have passed vacuously on an empty capture.

Tests

• The E2E assertion now checks both halves of the report (banner from stdout, summary from stderr) instead of a loose alternation.
• A static tripwire asserts runToFiles owns the module's only Bun.spawn, and that no site reads child output via stdout: 'pipe', new Response(proc.stdout), or getReader(). The data loss is load-dependent and silent, so a behavioural test cannot catch it reliably; the structure is what is pinned.

Verification

5 consecutive full runs of bun test browse/test/ (1970 tests each) with the target test passing in all five, and no EBADF.

The terminal-agent: PTY round-trip via real WebSocket failures in that suite are pre-existing and unrelated, confirmed by reproducing them on a clean tree (2 of 3 baseline runs).

Not addressed here

14 other stdout: 'pipe' capture sites share this pattern across browse/src/. The highest-value one is cookie-import-browser.ts, where getMacKeychainPassword returns stdout.trim() as a keychain password and the DPAPI path base64-decodes stdout into a decryption key, so a lost read yields a silent empty credential rather than a loud error. Left out to keep this reviewable.

Caveat

This establishes that the first piped spawn can drop a stream and that file-based capture is immune. It does not explain why Bun drops it. The fix is verified empirically against the reproduction, not derived from Bun internals.

🤖 Generated with Claude Code

[trunk-io]

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]

Thank you — this landed in v1.67.0.0 (credited in the CHANGELOG). Closing since the fix is on main. 🙏