Repository navigation
fix(coding-agent): keep Bun console output out of the RPC/JSON stdout stream - #284
Conversation
… stream On Bun, console.log/info/debug/dir/dirxml/table/trace/count/group and console.write print to fd 1 without calling a replaced process.stdout.write (oven-sh/bun#47982), so an extension's console.log corrupted the JSON stream in RPC, JSON, ACP and MCP modes. While stdout is taken over, route them through a node:console Console over process.stdout, which goes to stderr as on Node; restore them after.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32ec94a0a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
![opensec-code-review[bot]](https://avatars.githubusercontent.com/u/263740616?s=60&v=4){kind=small}
There was a problem hiding this comment.
OpenSec security review
✅ No security findings in this change.
The change makes takeOverStdout() replace Bun's global console stdout methods with a node:console Console over process.stdout, so console output from extensions and dependencies goes to stderr instead of the JSON protocol stream in RPC/JSON/ACP/MCP/service mode, restores the originals in restoreStdout(), and adds a Bun spawn regression test. I reviewed the console patch/restore ordering, the new sinks (stderr writes, trace stack output, console.write byte conversion), the list of methods covered, and whether any trusted-actor or untrusted-actor path gains reach, data, or availability; nothing introduced by the change survives verification.
Notes (uncertain or pre-existing, not counted)
- Pre-existing and not introduced here: in RPC/JSON/ACP/MCP/service mode, in-process code that bypasses process.stdout.write (Bun's console.assert, Bun.stdout.write, direct fd 1 writes) can still inject lines into the JSON stream; the change leaves those paths open (extensions are an explicit SECURITY.md exclusion).
- The Bun console rerouting is installed while stdoutTakeoverState is still unassigned (output-guard.ts:110-111), so any console output during Console construction would reach the real stdout; no such write exists in that window, so this is a latent ordering hazard, not a reachable path.
- packages/coding-agent/test/fixtures/rpc-console-session.ts and test/suite/regressions/rpc-console-stdout.test.ts are test-only and out of review scope.
3 candidates rejected by verification
- Bun console methods outside the replaced list still write to the protocol stream on stdout: The writer would be a user-installed extension/dependency, which SECURITY.md trusts and which can already emit arbitrary protocol JSON in-process; the remaining gaps (console.assert, Bun.stdout, fd 1 writes) are pre-existing Bun behavior the diff neither adds nor newly exposes.
- Extension console output, including full stack traces, is now persisted in stderr logs: The bytes are produced by the trusted extension and go to the process's own stderr, whose reader is the operator; the change narrows where they go and matches long-standing Node behavior.
- Synchronous throw from the replacement Console on a broken stderr pipe reaches extension code: Only the parent/supervisor that owns the stderr pipe can break it, a broken stderr already breaks every log write, and the same synchronous path already existed on Node before this change.
Range 72981bda4ac8..32ec94a0a55a, 245 changed lines. Review by OpenSec Code Review.
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
OpenSec security review
✅ No security findings in this change.
The change fixes a Bun-only bug where extension or dependency console output went straight to fd 1 and corrupted the JSON/JSON-RPC stream in RPC, JSON, ACP, MCP and service modes: while stdout is taken over, the global console stdout methods are pointed at a node:console Console over the replaced process.stdout so the output lands on stderr, and restoreStdout puts them back. It was checked against the trust model (extensions and project resources are trusted once installed or approved) for new attacker reach into sinks, terminal output, buffer handling and resource limits, and no untrusted-attacker gain was found.
Notes (uncertain or pre-existing, not counted)
- restoreStdout (output-guard.ts:152-162) writes back stored method values, so a console method absent on some Bun build would be restored as undefined; a robustness nit, not attacker reachable.
- After restoreStdout in JSON/print mode, console writes go to real fd 1 again (main.ts:1127); behaviour unchanged by this PR and only on the shutdown path.
- Change reviewed for other classes: no new entry points, no path/shell/query/template sinks, no dependency, CI or release changes; the test fixture and test file are test-only.
2 candidates rejected by verification
- Terminal escape-sequence injection via stderr for extension/dependency console output: Node already routed console.log to stderr during stdout takeover (Node's global console calls the replaced process.stdout.write, output-guard.ts:96-108); this change only makes Bun match that, so nothing moves safe to unsafe and console.warn/error already reached stderr.
- Memory contents exposure through the Bun console.write replacement (Buffer.from over chunk.byteOffset/byteLength): Buffer.from(chunk.buffer, chunk.byteOffset, chunk.byteLength) confines the view to exactly the chunk's own bytes (output-guard.ts:139-146); pooled-buffer neighbours are never serialized.
Range 72981bda4ac8..befba935ae9c, 246 changed lines. Review by OpenSec Code Review.
With stdout taken over in -p mode, an extension's console.log now goes to stderr on Bun as it does on Node.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
OpenSec security review
✅ No security findings in this change.
The change hardens stream separation on the Bun runtime: while stdout is taken over (RPC, JSON, ACP, MCP and print modes), the global console methods that Bun writes straight to fd 1 are replaced by a node:console Console over process.stdout/process.stderr, so extension and dependency logging goes to stderr as it already does on Node, and restoreStdout puts the originals back. I read output-guard.ts at head with its callers (main.ts, rpc-mode, print-mode, ACP and MCP modes) and the project trust model, and checked the three candidate issues with an independent verifier. All were rejected: no removed check, no new entry point, no new sink reachable by an actor outside the trust model, and protocol output still flows through the pre-captured raw stdout write. No confirmed findings.
Notes (uncertain or pre-existing, not counted)
- output-guard.ts:107 evaluates routeBunConsoleThroughStdoutWrite() inside the state object literal, so a throw there would leave the global console partly mutated with stdoutTakeoverState unset; robustness nit on a non-attacker-controlled path, not exploitable.
- output-guard.ts:158-161 restores console methods unconditionally; a method replaced by another party between takeover and restore would be overwritten. Process-lifetime only.
- The change reduces exposure: on Bun it stops extension console output from entering the JSON/RPC/ACP/MCP stdout stream (oven-sh/bun#47982) and keeps print-mode answers on stdout via writeRawStdout, which stays sanitized.
- scripts/windows-selftest.ps1 now asserts the extension marker on stderr; consistent with Node, where the redirected process.stdout.write already sent console output there. No check weakened.
- packages/coding-agent/test/fixtures/rpc-console-session.ts and the new regression test are test-only; the fixture runs under a spawned bun in tests and touches no shipped path.
3 candidates rejected by verification
- Extension/dependency console output now reaches stderr unsanitized, allowing terminal escape injection: The same bytes already reached stderr before the change (console.error and the redirected process.stdout.write in takeOverStdout), the actor is an extension/model/MCP server the user configured (explicitly out of scope in SECURITY.md), and the old Bun path put them in the machine-parsed fd 1 stream instead.
- routeBunConsoleThroughStdoutWrite can throw partway, leaving process.stdout.write replaced with stdoutTakeoverState unset: Only fixed node:console method names are touched, no attacker input reaches it, and the outcome is a startup crash of the user's own process with no gain.
- Console replacement lets a caller clobber console methods or redirect protocol JSON off stdout: Protocol output goes through writeRawStdout/getRawStdoutWrite, whose rawStdoutWrite is bound before any console mutation; the only code that could call restoreStdout or pre-clobber console is an extension running with the user's permissions, which the trust model excludes.
Range 72981bda4ac8..27d6744e1e38, 249 changed lines. Review by OpenSec Code Review.
…ole prints output-guard.ts imported both at the top (opensec-git#284), and main.ts imports output-guard at every start, so every `pi -p` and TUI start evaluated them: +3.5 M instructions in module init and ~1 MB of peak RSS. The Bun console redirect now makes its Node Console at the first console call, through process.getBuiltinModule, so a run that never prints through the console never loads them. Output is unchanged.
Problem: on Bun,
console.log/info/debug/dir/dirxml/table/trace/count/groupand Bun'sconsole.writeprint to fd 1 without calling a replacedprocess.stdout.write(oven-sh/bun#47982, fix oven-sh/bun#47986 still open). Node's console calls the replaced write, sotakeOverStdout()sends it to stderr there. On Pi-Bolt, an extension's or a dependency'sconsole.login RPC, JSON, ACP or MCP mode (andpi-bolt service, the Android engine) went into the JSON stream.Repro (Bun 1.4.2 and the Pi-Bolt 1.4.3 runtime): an extension that calls
console.log("extension loaded", { a: 1 })in--mode rpcprinted that line, unformatted Bun-style, before the first JSON response on stdout.Fix: while stdout is taken over on Bun, those methods are replaced by the methods of a
node:consoleConsoleoverprocess.stdout/process.stderr, which calls the replaced write as Node's global console does, so the output goes to stderr with Node's formatting.traceis built fromutil.format+Error.captureStackTracebecause Bun'sConsole.tracedrops theTrace: <message>line;console.writeis routed throughprocess.stdout.write.restoreStdout()puts the originals back. Node and interactive mode are unchanged (no takeover there / not on Bun).Tests:
test/suite/regressions/rpc-console-stdout.test.ts(faux provider):console.log/inforeach stderr, stdout is only JSON lines.bunis not installed):test/fixtures/rpc-console-session.tsruns a full RPC session under Bun with an extension calling every method above; stdout must be JSON lines only and stderr must have each line. It fails without the fix.npm run checkclean.