Skip to content

Commit 8199718

Browse files
author
bcode
committed
browser_execute: per-call console arg, drop global monkey-patch
Fixes cubic P2 finding on PR #41. Two overlapping execute() calls clobbered each other's global console.log/error/warn/info patches and corrupted the originals process-wide via a stale 'finally' restore. Cases this hits: server-mode multi-session, sub-agent sessions, parallel browser_execute tool calls in one assistant turn (AI SDK runs those via Promise.all), TUI tabs sharing a daemon. Fix: bind a per-call { log, error, warn, info } object as the second AsyncFunction argument. JS scope chain resolves console.log() in the snippet to the function parameter before reaching the global, so existing snippets keep working byte-identically and the global console is never mutated. Also concurrency-safe by construction -- no shared state between calls. Test 'overlapping execute calls do not clobber each other's console capture' is a regression guard: verified to fail on the old impl (empty output captures + 'bye from B' leaking to stderr) and pass on the new one.
1 parent c5ea60c commit 8199718

2 files changed

Lines changed: 68 additions & 29 deletions

File tree

packages/bcode-browser/src/browser-execute.ts

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@
33
// Executes a JavaScript snippet in-process against a per-opencode-session
44
// `Session` (the CDP transport from `./cdp/session.ts`). No subprocess, no
55
// daemon, no Unix socket, no `uv` — we wrap the snippet with
6-
// `new AsyncFunction("session", code)` and run it.
6+
// `new AsyncFunction("session", "console", code)` and run it.
77
//
88
// Snippet scope (Phase H hard rule #3 — workspace-as-plain-code):
99
// `session` — the live CDP `Session`, persistent across calls.
10+
// `console` — per-call capture object shadowing the global. Same
11+
// `{log, error, warn, info}` API as the real console.
1012
// standard JS globals.
1113
//
1214
// Nothing is auto-loaded. To reuse code from a previous snippet the agent
@@ -16,13 +18,17 @@
1618
// wrapper supplies `ctx.workspaceDir` so `.ts` files written under it can be
1719
// addressed by absolute path; this resolver creates the dir on first use.
1820
//
19-
// Output capture: console.log calls inside the snippet stream via a
20-
// monkey-patch around `console.log`/`console.error`/`console.warn`/
21-
// `console.info`. The originals are restored in a `finally` block — even if
22-
// the snippet throws, even on timeout. See
23-
// `memory/browsercode/phase_h_eval_feasibility_findings.md` for the verified
24-
// pattern (compiled-mode `bun build --compile` works on Linux x64; AsyncFunction
25-
// + dynamic import survive bunfs).
21+
// Output capture: a per-call `console` object (`{log, error, warn, info}`)
22+
// is bound into the snippet's lexical scope as the second AsyncFunction
23+
// argument. JavaScript's scope chain resolves `console.log(...)` to the
24+
// function parameter before reaching the global, so existing snippets keep
25+
// working byte-identically while the global `console` stays untouched.
26+
// This is concurrency-safe: two overlapping `execute` calls (different
27+
// opencode sessions in the same process, parallel tool calls within one
28+
// session, etc.) each get their own capture buffer with no global state to
29+
// clobber. See `memory/browsercode/phase_h_eval_feasibility_findings.md`
30+
// for the verified eval pattern (compiled-mode `bun build --compile` works
31+
// on Linux x64; AsyncFunction + dynamic import survive bunfs).
2632
//
2733
// Cancellation: JS Promises are not preemptively cancellable. A snippet
2834
// without `await` yield-points (e.g. `for (let i = 0; i < 1e9; i++) {}`)
@@ -44,7 +50,8 @@ const MAX_TIMEOUT_MS = 10 * 60 * 1000
4450

4551
export const parameters = Schema.Struct({
4652
code: Schema.String.annotate({
47-
description: "JavaScript source. Wrapped in an async function with `session` (CDP Session) bound.",
53+
description:
54+
"JavaScript source. Wrapped in an async function with `session` (CDP Session) and `console` (per-call capture; same `log/error/warn/info` API) bound.",
4855
}),
4956
timeout: Schema.optional(Schema.Number).annotate({
5057
description: `Timeout in milliseconds. Default ${DEFAULT_TIMEOUT_MS}, max ${MAX_TIMEOUT_MS}.`,
@@ -117,37 +124,21 @@ export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string)
117124
yield* Effect.promise(() => fs.mkdir(ctx.workspaceDir, { recursive: true }))
118125

119126
const wrapped = yield* Effect.try({
120-
try: () => new AsyncFunction("session", args.code),
127+
try: () => new AsyncFunction("session", "console", args.code),
121128
catch: (err) => new Error(`syntax error in browser_execute snippet: ${err}`),
122129
})
123130

124131
let output = ""
125-
const realLog = console.log
126-
const realErr = console.error
127-
const realWarn = console.warn
128-
const realInfo = console.info
129132
const tee = (...a: unknown[]) => {
130133
output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
131134
if (ctx.onChunk) Effect.runFork(ctx.onChunk(output))
132135
}
133-
console.log = tee
134-
console.error = tee
135-
console.warn = tee
136-
console.info = tee
136+
const snippetConsole = { log: tee, error: tee, warn: tee, info: tee }
137137

138138
const ran = yield* Effect.tryPromise({
139-
try: () => wrapped(session),
139+
try: () => wrapped(session, snippetConsole),
140140
catch: (err) => new Error(`browser_execute snippet threw: ${err instanceof Error ? err.stack ?? err.message : String(err)}`),
141-
}).pipe(
142-
Effect.ensuring(
143-
Effect.sync(() => {
144-
console.log = realLog
145-
console.error = realErr
146-
console.warn = realWarn
147-
console.info = realInfo
148-
}),
149-
),
150-
)
141+
})
151142

152143
return { output, result: serialize(ran) } satisfies ExecuteResult
153144
}).pipe(

packages/bcode-browser/test/browser-execute.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,3 +130,51 @@ test.skipIf(!enabled)("syntax error in snippet surfaces a clean failure", async
130130
),
131131
).rejects.toThrow(/syntax error/)
132132
})
133+
134+
// Concurrency safety: two overlapping execute() calls (different sessionIDs)
135+
// must each capture their own console output without leaking into each other
136+
// or into the real global console. No Chrome required — the snippets never
137+
// touch `session`. Regression guard for the global-monkey-patch bug fixed
138+
// by the per-call `console` argument shadowing the global.
139+
test("overlapping execute calls do not clobber each other's console capture", async () => {
140+
const realLogBefore = console.log
141+
const aWorkspace = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-conc-a-"))
142+
const bWorkspace = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-conc-b-"))
143+
const aData = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-data-a-"))
144+
const bData = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-data-b-"))
145+
146+
const run = (label: string, dataDirX: string, workspace: string) =>
147+
Effect.runPromise(
148+
Effect.scoped(
149+
Effect.gen(function* () {
150+
const impl = yield* BrowserExecute.make(dataDirX)
151+
return yield* impl.execute(
152+
{
153+
// Yield once so both snippets' bodies are mid-execution at the same
154+
// time; under the old global-patch impl, B's tee would shadow A's
155+
// and the `finally` chain would corrupt both captures + the global.
156+
code: `await new Promise((r) => setTimeout(r, 50));
157+
console.log("hello from ${label}");
158+
await new Promise((r) => setTimeout(r, 50));
159+
console.log("bye from ${label}");
160+
return ${JSON.stringify(label)};`,
161+
},
162+
{ sessionID: `concurrency-${label}`, workspaceDir: workspace },
163+
)
164+
}),
165+
),
166+
)
167+
168+
const [a, b] = await Promise.all([run("A", aData, aWorkspace), run("B", bData, bWorkspace)])
169+
170+
expect(a.output).toBe("hello from A\nbye from A\n")
171+
expect(b.output).toBe("hello from B\nbye from B\n")
172+
expect(JSON.parse(a.result)).toBe("A")
173+
expect(JSON.parse(b.result)).toBe("B")
174+
// Global console must be untouched.
175+
expect(console.log).toBe(realLogBefore)
176+
177+
await Promise.all(
178+
[aWorkspace, bWorkspace, aData, bData].map((d) => fs.rm(d, { recursive: true, force: true })),
179+
)
180+
})

0 commit comments

Comments
 (0)