diff --git a/packages/bcode-browser/skills/browser-execute/SKILL.md b/packages/bcode-browser/skills/browser-execute/SKILL.md index 217f877ed..44d8202d0 100644 --- a/packages/bcode-browser/skills/browser-execute/SKILL.md +++ b/packages/bcode-browser/skills/browser-execute/SKILL.md @@ -197,6 +197,7 @@ console.log(JSON.stringify(titles)) ## Guardrails - Top-level `import` statements inside the snippet body are not allowed. Use `await import(...)` instead. - No CPU-bound infinite loops without `await` — they ignore the timeout. Insert `await new Promise(r => setTimeout(r, 0))` to yield. +- `browser_execute` defaults to 60s (max 600s). For longer work, set the tool's top-level `timeout`; inner CDP timeouts do not extend it. Keep batches small and log progress — timeout errors return recent logs, and a timeout resets the CDP session (reconnect in the next snippet). ## Console - `console.log`, `console.error`, `console.warn`, `console.info`, `console.debug` are all captured and streamed to the user. Treat them as your stdout. Other `console.*` methods write to bcode's stderr without being captured into the tool result. diff --git a/packages/bcode-browser/src/browser-execute.ts b/packages/bcode-browser/src/browser-execute.ts index 230bacbd9..ddf756ef9 100644 --- a/packages/bcode-browser/src/browser-execute.ts +++ b/packages/bcode-browser/src/browser-execute.ts @@ -32,10 +32,13 @@ // // Cancellation: JS Promises are not preemptively cancellable. A snippet // without `await` yield-points (e.g. `for (let i = 0; i < 1e9; i++) {}`) -// runs to completion before our timeout fiber observes it. `Effect.timeoutOrElse` -// fails the surrounding fiber but the orphan Promise keeps running until it -// finishes. This matches the `uv run` subprocess case (SIGTERM only after -// the Python signal handler yields). Document, don't fix. +// runs to completion before our timeout fiber observes it. When a yielding +// snippet times out, its Promise keeps running as an orphan — so on timeout +// we retire the exact Session object the snippet received (rejects future +// connect/_call, closes the socket) and evict it from SessionStore. The +// orphan can finish local work but cannot keep driving the browser, and the +// next tool call gets a fresh Session instead of sharing a socket with it. +// The timeout error carries the console output captured so far. // // Level 1 per decisions.md §1c — substantial implementation lives here. The // Level-2 hook in packages/opencode is a thin adapter. @@ -48,6 +51,18 @@ import { Skills } from "./skills" const DEFAULT_TIMEOUT_MS = 60 * 1000 const MAX_TIMEOUT_MS = 10 * 60 * 1000 +const MAX_TIMEOUT_OUTPUT_BYTES = 8 * 1024 +const TIMEOUT_OUTPUT_TRUNCATED = "[partial console output truncated; showing final bytes]\n" + +// Tail-cap the captured output for the timeout error: last 8 KiB, snapped +// forward to a UTF-8 sequence start so multibyte characters survive the cut. +const timeoutOutput = (output: string) => { + const bytes = Buffer.from(output, "utf8") + if (bytes.length <= MAX_TIMEOUT_OUTPUT_BYTES) return output + let start = bytes.length - (MAX_TIMEOUT_OUTPUT_BYTES - Buffer.byteLength(TIMEOUT_OUTPUT_TRUNCATED)) + while (start < bytes.length && (bytes[start]! & 0xc0) === 0x80) start++ + return TIMEOUT_OUTPUT_TRUNCATED + bytes.subarray(start).toString("utf8") +} // Field order matters: providers stream tool-call args in schema-declared // order, so the model commits to whichever field comes first. `code` is the @@ -157,9 +172,16 @@ const serialize = (v: unknown): string => { export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string) { const skillsDir = yield* Effect.promise(() => Skills.resolveSkillsDir(dataDir)) + // Effect values are re-runnable, so per-run state lives inside the suspend + // thunk: each run resolves its own Session (the timeout handler retires + // exactly that object) and its own capture buffer (a re-run after a timeout + // must not inherit a retired Session or a frozen capture). const execute = (args: Parameters, ctx: ExecuteContext) => - Effect.gen(function* () { - const session = SessionStore.get(ctx.sessionID) + Effect.suspend(() => { + const session = SessionStore.get(ctx.sessionID) + const captured = { active: true, output: "" } + const timeout = Math.min(args.timeout ?? DEFAULT_TIMEOUT_MS, MAX_TIMEOUT_MS) + return Effect.gen(function* () { yield* Effect.promise(() => fs.mkdir(ctx.workspaceDir, { recursive: true })) const wrapped = yield* Effect.try({ @@ -167,10 +189,10 @@ export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string) catch: (err) => new Error(`syntax error in browser_execute snippet: ${err}`), }) - let output = "" const tee = (...a: unknown[]) => { - output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n" - if (ctx.onChunk) Effect.runFork(ctx.onChunk(output)) + if (!captured.active) return + captured.output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n" + if (ctx.onChunk) Effect.runFork(ctx.onChunk(captured.output)) } // Prototype-chain to the real `console` so uncommon methods (`debug`, // `dir`, `trace`, `table`, `group`, …) don't throw when a snippet calls @@ -223,14 +245,34 @@ export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string) catch: (err) => new Error(`browser_execute snippet threw: ${err instanceof Error ? err.stack ?? err.message : String(err)}`), }).pipe(Effect.ensuring(Effect.sync(() => unsubscribe()))) - return { output, result: serialize(ran), screenshots } satisfies ExecuteResult + return { output: captured.output, result: serialize(ran), screenshots } satisfies ExecuteResult }).pipe( Effect.scoped, Effect.timeoutOrElse({ - duration: Math.min(args.timeout ?? DEFAULT_TIMEOUT_MS, MAX_TIMEOUT_MS), - orElse: () => Effect.fail(new Error("browser_execute timed out")), + duration: timeout, + orElse: () => + Effect.suspend(() => { + captured.active = false + const output = timeoutOutput(captured.output) + const error = new Error( + [ + `browser_execute timed out after ${timeout} ms; CDP session was reset — reconnect in the next snippet`, + output.trim() ? `Partial console output before timeout:\n${output.trimEnd()}` : "", + ] + .filter(Boolean) + .join("\n\n"), + ) + // Always retires this snippet's Session; the identity check + // inside only guards the store delete, so a successor Session is + // never evicted. A concurrent same-sessionID call would share the + // retired object — acceptable for v1, opencode serializes tool + // calls within an assistant message. + SessionStore.invalidate(ctx.sessionID, session, error) + return Effect.fail(error) + }), }), ) + }) return { parameters, execute, skillsDir } }) diff --git a/packages/bcode-browser/src/cdp/session.ts b/packages/bcode-browser/src/cdp/session.ts index 3ea544996..b19713c5e 100644 --- a/packages/bcode-browser/src/cdp/session.ts +++ b/packages/bcode-browser/src/cdp/session.ts @@ -43,6 +43,7 @@ export type DetectedBrowser = { export class Session implements Transport { private ws?: WebSocket; + private invalidatedError?: Error; private nextId = 1; private pending = new Map(); private activeSessionId: string | undefined; @@ -79,6 +80,7 @@ export class Session implements Transport { * and we connect directly to the supplied endpoint. */ async connect(opts: ConnectOptions = {}): Promise { + if (this.invalidatedError) throw this.invalidatedError; const timeoutMs = opts.timeoutMs ?? 5_000; if (opts.wsUrl || opts.profileDir) { const wsUrl = await resolveWsUrl(opts, timeoutMs); @@ -103,6 +105,7 @@ export class Session implements Transport { await this.openWs(b.wsUrl, timeoutMs); return; } catch (e) { + if (this.invalidatedError) throw this.invalidatedError; const msg = e instanceof Error ? e.message : String(e); errors.push(` ${b.name} @ ${b.wsUrl}: ${msg}`); } @@ -113,6 +116,10 @@ export class Session implements Transport { } private openWs(wsUrl: string, timeoutMs: number): Promise { + // Re-checked here (not only in connect) because connect awaits resolver/ + // detection steps first — an invalidation landing during those must not + // open a late socket for a retired Session. + if (this.invalidatedError) return Promise.reject(this.invalidatedError); return new Promise((res, rej) => { const ws = new WebSocket(wsUrl); let done = false; @@ -124,26 +131,46 @@ export class Session implements Transport { else res(); }; const timer = setTimeout(() => finish(new Error(`timed out after ${timeoutMs}ms`)), timeoutMs); - ws.addEventListener('open', () => finish()); + ws.addEventListener('open', () => finish(this.invalidatedError)); ws.addEventListener('error', (e) => finish(new Error(`WS error: ${(e as any)?.message ?? 'connect failed (likely 403, permission not granted, or port closed)'}`))); ws.addEventListener('message', (e) => this.onMessage(String(e.data))); ws.addEventListener('close', () => { - for (const [, p] of this.pending) p.reject(new Error('CDP socket closed')); + for (const [, p] of this.pending) p.reject(this.invalidatedError ?? new Error('CDP socket closed')); this.pending.clear(); - finish(new Error('WS closed before open (likely 403 or port closed)')); + finish(this.invalidatedError ?? new Error('WS closed before open (likely 403 or port closed)')); }); this.ws = ws; }); } isConnected(): boolean { - return this.ws?.readyState === WebSocket.OPEN; + return !this.invalidatedError && this.ws?.readyState === WebSocket.OPEN; } close(): void { this.ws?.close(); } + /** + * Permanently retire this Session object. + * + * `browser_execute` timeouts cannot preempt the snippet's Promise — the + * orphan keeps running and would otherwise share this object (and its + * socket) with the next tool call, interleaving two authors on one + * transport. Invalidation rejects all future `connect`/`_call` attempts + * and closes the socket (the close handler rejects in-flight calls); + * `SessionStore.invalidate` removes the entry so the next call gets a + * fresh Session. + */ + invalidate(error: Error): void { + if (this.invalidatedError) return; + this.invalidatedError = error; + const ws = this.ws; + this.ws = undefined; + this.activeSessionId = undefined; + try { ws?.close(); } catch { /* ignore */ } + } + /** * Pick a target and make subsequent calls auto-route to it. * Uses Target.attachToTarget with flatten:true (single-WS, sessionId-on-message). @@ -239,6 +266,7 @@ export class Session implements Transport { // Transport implementation. Called by the generated domain bindings. _call(method: string, params: unknown = {}): Promise { + if (this.invalidatedError) return Promise.reject(this.invalidatedError); if (!this.ws || this.ws.readyState !== WebSocket.OPEN) { return Promise.reject(new Error('Not connected. Call session.connect(...) first.')); } diff --git a/packages/bcode-browser/src/session-store.ts b/packages/bcode-browser/src/session-store.ts index 8be05462c..4d911a5fa 100644 --- a/packages/bcode-browser/src/session-store.ts +++ b/packages/bcode-browser/src/session-store.ts @@ -27,6 +27,15 @@ export const get = (sessionID: string): Session => { return fresh } +// Retire a specific Session object after a browser_execute timeout. The +// expected Session is always invalidated; the identity check only guards the +// map delete, so a stale caller can never evict a successor Session that a +// newer call is already using. +export const invalidate = (sessionID: string, expected: Session, error: Error): void => { + if (sessions.get(sessionID) === expected) sessions.delete(sessionID) + expected.invalidate(error) +} + export const evict = async (sessionID: string): Promise => { const entry = sessions.get(sessionID) if (!entry) return diff --git a/packages/bcode-browser/test/browser-execute.test.ts b/packages/bcode-browser/test/browser-execute.test.ts index 5b1ca9eb4..5c62b65e0 100644 --- a/packages/bcode-browser/test/browser-execute.test.ts +++ b/packages/bcode-browser/test/browser-execute.test.ts @@ -224,6 +224,130 @@ test("console.debug is captured; uncommon methods fall through without throwing" await Promise.all([data, ws].map((d) => fs.rm(d, { recursive: true, force: true }))) }) +// Timeout isolation: a timed-out snippet keeps running as an orphan (JS +// Promises are not preemptible), so the tool must retire the Session object +// the snippet received and surface captured output in the error. No Chrome +// required — the snippets sleep without touching the browser. +const runTimeout = async (id: string, code: string, timeout: number, onChunk?: (o: string) => Effect.Effect) => { + const data = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-to-")) + const ws = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-to-ws-")) + const err = await Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + const impl = yield* BrowserExecute.make(data) + return yield* impl.execute( + { description: "timeout test", code, timeout }, + { sessionID: id, workspaceDir: ws, onChunk }, + ) + }), + ), + ).then( + () => { throw new Error("expected timeout") }, + (e: unknown) => String(e), + ) + await Promise.all([data, ws].map((d) => fs.rm(d, { recursive: true, force: true }))) + return err +} + +test("timeout returns partial output and retires the session", async () => { + const id = "timeout-isolation-test" + const before = SessionStore.get(id) + const err = await runTimeout( + id, + `console.log("progress-marker"); + await new Promise((r) => setTimeout(r, 60_000));`, + 100, + ) + expect(err).toContain("timed out after 100 ms") + expect(err).toContain("Partial console output before timeout:") + expect(err).toContain("progress-marker") + // The orphan's Session is permanently dead... + expect(before.isConnected()).toBe(false) + await expect(before.connect({ wsUrl: "ws://127.0.0.1:9/nope" })).rejects.toThrow(/timed out after 100 ms/) + await expect(before.domains.Runtime.evaluate({ expression: "1" })).rejects.toThrow(/timed out after 100 ms/) + // ...and the next tool call gets a fresh one. + expect(SessionStore.get(id)).not.toBe(before) + await SessionStore.evict(id) +}) + +test("console capture and onChunk stop after timeout", async () => { + const chunks: string[] = [] + const err = await runTimeout( + "timeout-capture-test", + `console.log("early"); + await new Promise((r) => setTimeout(r, 250)); + console.log("late");`, + 100, + (o) => Effect.sync(() => { chunks.push(o) }), + ) + expect(err).toContain("early") + // Let the orphan's late log fire, then confirm it was not captured. + await new Promise((r) => setTimeout(r, 400)) + expect(chunks.some((c) => c.includes("early"))).toBe(true) + expect(chunks.some((c) => c.includes("late"))).toBe(false) + await SessionStore.evict("timeout-capture-test") +}) + +test("re-running the execute effect after a timeout gets fresh state", async () => { + const data = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-rerun-")) + const ws = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-rerun-ws-")) + const impl = await Effect.runPromise(BrowserExecute.make(data)) + // One Effect value, run twice. Each run must resolve its own Session and + // capture buffer — the second run's error must carry its own partial + // output, not inherit the first run's frozen capture or retired Session. + // onChunk deliveries discriminate: a run that inherited a frozen capture + // buffer never tees, so it produces zero chunks (the frozen buffer still + // *contains* run 1's text, which is why asserting on the error message + // alone cannot catch this). + const chunks: string[] = [] + const eff = impl.execute( + { + description: "rerun test", + code: `console.log("progress-marker"); + await new Promise((r) => setTimeout(r, 60_000));`, + timeout: 100, + }, + { sessionID: "rerun-test", workspaceDir: ws, onChunk: (o) => Effect.sync(() => { chunks.push(o) }) }, + ) + const run = () => Effect.runPromise(eff).then(() => "resolved", (e: unknown) => String(e)) + const first = await run() + const afterFirst = chunks.length + const second = await run() + expect(first).toContain("progress-marker") + expect(second).toContain("progress-marker") + expect(afterFirst).toBeGreaterThan(0) + expect(chunks.length).toBeGreaterThan(afterFirst) + await SessionStore.evict("rerun-test") + await Promise.all([data, ws].map((d) => fs.rm(d, { recursive: true, force: true }))) +}) + +test("invalidate retires the expected Session even after replacement", async () => { + const id = "invalidate-replaced-test" + const s1 = SessionStore.get(id) + await SessionStore.evict(id) + const s2 = SessionStore.get(id) + SessionStore.invalidate(id, s1, new Error("retired stale session")) + // The successor entry is untouched, but the stale object is still dead. + expect(SessionStore.get(id)).toBe(s2) + await expect(s1.connect({ wsUrl: "ws://127.0.0.1:9/nope" })).rejects.toThrow(/retired stale session/) + await SessionStore.evict(id) +}) + +test("timeout output is tail-capped to valid UTF-8", async () => { + // ~25 KiB of multibyte lines, all logged before the sleep. + const err = await runTimeout( + "timeout-truncate-test", + `for (let i = 0; i < 300; i++) console.log("é".repeat(40) + "-line-" + i); + await new Promise((r) => setTimeout(r, 60_000));`, + 100, + ) + expect(err).toContain("[partial console output truncated; showing final bytes]") + expect(err).toContain("-line-299") + expect(err).not.toContain("-line-0\n") + expect(err).not.toContain("\uFFFD") + await SessionStore.evict("timeout-truncate-test") +}) + // Concurrency safety: two overlapping execute() calls (different sessionIDs) // must each capture their own console output without leaking into each other // or into the real global console. No Chrome required — the snippets never diff --git a/packages/bcode-browser/test/cdp-session.test.ts b/packages/bcode-browser/test/cdp-session.test.ts index 722627a7c..c5b916b4e 100644 --- a/packages/bcode-browser/test/cdp-session.test.ts +++ b/packages/bcode-browser/test/cdp-session.test.ts @@ -77,3 +77,30 @@ test("waitFor throws on a positional timeout rather than silently using the 30s await expect(session.waitFor("Test.never", { timeoutMs: 50 })).rejects.toThrow(/Timeout waiting for/) expect(Date.now() - started).toBeLessThan(1_000) }) + +// Retirement guarantee under in-flight connects: an invalidation landing +// while connect() is between awaits must not leave a usable or open socket. +test("invalidate before the socket exists rejects the in-flight connect", async () => { + const s = new Session() + // connect() awaits resolveWsUrl before openWs, so invalidate() runs while + // no socket exists yet — openWs must refuse to create one afterwards. + const connecting = s.connect({ wsUrl: `ws://127.0.0.1:${server.port}/`, timeoutMs: 1_000 }) + s.invalidate(new Error("retired by test")) + await expect(connecting).rejects.toThrow("retired by test") + expect(s.isConnected()).toBe(false) +}) + +test("invalidate while the socket is connecting closes it and rejects", async () => { + const s = new Session() + const connecting = s.connect({ wsUrl: `ws://127.0.0.1:${server.port}/`, timeoutMs: 1_000 }) + // Yield one macrotask so openWs has created the WebSocket, then retire. + await Bun.sleep(0) + s.invalidate(new Error("retired by test")) + // Depending on whether the open event won the race, connect either rejects + // or resolved just before retirement — in both cases the Session must end + // dead with no usable transport. + await connecting.catch(() => {}) + expect(s.isConnected()).toBe(false) + await expect(s._call("Runtime.evaluate", { expression: "1" })).rejects.toThrow("retired by test") + await expect(s.connect({ wsUrl: `ws://127.0.0.1:${server.port}/` })).rejects.toThrow("retired by test") +})