From b74d5da07692e8899cfef049ff7d1e671df4f376 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Wed, 5 Aug 2026 23:19:15 +0530 Subject: [PATCH] feat(tools): add a persistent repl, background processes and image reading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Across the 16 benchmark trials recorded in jobs/, 1,243 of 1,533 tool calls were run_terminal, and 1,017 of those were inline python3/node scripts. Every one of them rebuilt its state from nothing: the video-processing trial opened the same MP4 63 times across 74 scripts, gates.txt was re-parsed 26 times, input.tex 35 times. That is paid twice, in iterations and in tokens, because each script body stays in the conversation for the rest of the run. Two trials hit the 200-iteration ceiling and scored 0. repl keeps an interpreter alive for the turn, so data is loaded once and then queried. It drives a small framed driver rather than python3 -i, because an interactive prompt has no marker saying a statement finished and any reader has to guess. process_start/_output/_stop cover what run_terminal structurally cannot: it refuses an unquoted & and kills at its timeout, leaving no way to run a server or a watcher. These outlive the turn on purpose and end at dispose. read_image exists because the same trials show 131 cv2 and 128 numpy calls inferring what a frame contained, on a task that scored 0 in three of four trials. Tool.execute stays Promise: only Anthropic accepts an image inside a tool result, so the image rides on a following user message, which is the one shape all three providers take. The path is stored rather than the bytes, keeping sessions small at the cost of a file that may vanish — which is reported to the model rather than silently dropped. Deliberately not added: multi_edit or apply_patch. edit_file was called 5 times in 1,533; more editing surface is prompt weight the model already routes around. Commands now run under bash rather than sh. sh is dash on Debian, and a recorded run lost six commands to "Syntax error: Bad for loop variable" with nothing in the message pointing at the shell. Four defects found by reading the diff and probing, not by the suite: plan mode could not see repl source, so a write passed the second gate; background ids were reused after a stop, turning a stale id into a mix-up rather than an error; process_stop killed the shell but not its children; and attached images counted as conversation turns in recentMessages, evicting the user's actual question. --- commands/agentController.ts | 11 + config/config.ts | 20 +- config/toolSchema.ts | 4 + config/types.ts | 37 ++ .../tests/providers/imageAttachments.test.ts | 214 ++++++++++ .../tests/tools/process.integration.test.ts | 221 ++++++++++ .../tests/tools/readImage.integration.test.ts | 185 ++++++++ packages/tests/tools/repl.integration.test.ts | 210 +++++++++ .../tests/tools/terminal.integration.test.ts | 43 +- providers/anthropicClient.ts | 28 +- providers/client.ts | 15 +- providers/images.ts | 71 ++++ providers/openaiClient.ts | 25 +- runtime/codeEffects.test.ts | 188 +++++++++ runtime/loop.ts | 35 ++ runtime/planMode.ts | 13 +- runtime/toolEffects.ts | 82 ++++ runtime/turnState.ts | 9 +- site/src/docs/surface.json | 96 ++++- tools/approval.ts | 49 ++- tools/command.ts | 39 +- tools/index.ts | 8 + tools/process.ts | 296 +++++++++++++ tools/readImage.ts | 179 ++++++++ tools/repl.ts | 118 ++++++ tools/replSession.ts | 398 ++++++++++++++++++ tools/terminal.ts | 14 +- tui/src/types.ts | 2 +- 28 files changed, 2583 insertions(+), 27 deletions(-) create mode 100644 packages/tests/providers/imageAttachments.test.ts create mode 100644 packages/tests/tools/process.integration.test.ts create mode 100644 packages/tests/tools/readImage.integration.test.ts create mode 100644 packages/tests/tools/repl.integration.test.ts create mode 100644 providers/images.ts create mode 100644 runtime/codeEffects.test.ts create mode 100644 tools/process.ts create mode 100644 tools/readImage.ts create mode 100644 tools/repl.ts create mode 100644 tools/replSession.ts diff --git a/commands/agentController.ts b/commands/agentController.ts index 49b5043..dc3f8f9 100644 --- a/commands/agentController.ts +++ b/commands/agentController.ts @@ -22,6 +22,7 @@ import { type SessionRecord, } from "../config/sessions"; import { agentLoop } from "../runtime/loop"; +import { stopAllProcesses } from "../tools/process"; import { PLAN_MODE_PROMPT } from "../config/systemPrompt"; import { nextSessionMode, @@ -463,6 +464,16 @@ export class AgentController { } async dispose() { + // Background processes are the one thing here that outlives a turn by + // design — a server started while answering one question has to still be + // up for the next. This is where that ends: both the TUI's exit handler + // and the headless path come through dispose, so a session cannot end + // leaving a spawned process holding its port. + // + // Before the persist below rather than after, because persisting can fail + // and must not be what decides whether the processes are cleaned up. + stopAllProcesses(); + if (this.wasCancelled) { this.pendingAssistantText = null; this.removePendingUserMessage(); diff --git a/config/config.ts b/config/config.ts index 85273b6..96ae1f8 100644 --- a/config/config.ts +++ b/config/config.ts @@ -431,6 +431,24 @@ export async function buildRepositoryContext() { ); } +/** + * Is this a real turn of the conversation, or an image the loop attached? + * + * `read_image` returns text and the loop follows it with a user message + * carrying the picture, because that is the only shape all three providers + * accept. Those messages are not turns — nobody typed them — and counting them + * as turns silently shortens the window to the point of losing the question: + * with `MAX_TURNS` at 6, an agent that reads five frames of a video keeps one + * real user turn and five copies of "The image requested above:". + * + * That is the case `read_image` exists for, so it is not an edge case. The + * message is still *kept* when it falls inside the window — only the counting + * skips it. + */ +function isConversationTurn(message: Message | undefined): boolean { + return message?.role === "user" && !message.images?.length; +} + export function recentMessages( message: Message[], maxTurns: number, @@ -443,7 +461,7 @@ export function recentMessages( let startIndex = 0; for (let i = message.length - 1; i >= 0; i--) { - if (message[i]?.role === "user") { + if (isConversationTurn(message[i])) { userTurn++; if (userTurn == maxTurns) { diff --git a/config/toolSchema.ts b/config/toolSchema.ts index 55ad0e1..d647dd1 100644 --- a/config/toolSchema.ts +++ b/config/toolSchema.ts @@ -38,6 +38,10 @@ export function parameterSchema(parameter: ToolParameter): JsonSchema { const type: JsonSchemaType = parameter.type ?? "string"; const schema: JsonSchema = { type, description: parameter.description }; + // Emitted for any type, including array, where it constrains the array's own + // value rather than its elements — the element case is `items[].enum` below. + if (parameter.enum) schema.enum = parameter.enum; + if (type !== "array") return schema; // An array has to state what it holds. Objects when the parameter describes diff --git a/config/types.ts b/config/types.ts index 67db1ec..ef2d8c2 100644 --- a/config/types.ts +++ b/config/types.ts @@ -148,10 +148,38 @@ export interface IterationUsage { durationMs: number; } +/** + * An image carried by a user message. + * + * The path is stored, not the bytes. Each provider client loads the file when + * it renders the request, so a session on disk stays small and readable and + * compaction keeps moving short strings rather than megabytes of base64. The + * cost of that choice is that the file can change or disappear between turns, + * which every client handles by falling back to text rather than failing the + * request — a stale frame is worth less than a turn that cannot be sent. + */ +export interface ImageAttachment { + /** Absolute, already resolved through `resolveWorkspacePath`. */ + path: string; + /** An IANA type the providers accept: image/png, image/jpeg, image/gif, image/webp. */ + mediaType: string; +} + export type Message = | { role: "user"; content: string; + /** + * Images shown alongside the text. + * + * On a user message rather than on the tool result that produced them + * because that is the one shape all three providers accept. Anthropic + * takes an image inside a `tool_result`, but Gemini's `functionResponse` + * wants a JSON object and OpenAI's `function_call_output` is a string, so + * a tool that returned an image directly would work on one provider and + * silently degrade on two. + */ + images?: ImageAttachment[]; } | { role: "assistant"; @@ -186,6 +214,15 @@ export interface ToolParameter { description: string; required: boolean; type?: "string" | "number" | "boolean" | "array"; + /** + * Allowed values for the parameter itself, enforced by the provider. + * + * The same guarantee `ToolItemProperty.enum` already gave the objects inside + * an array, for the scalar case: an invalid value is rejected on their side + * and never costs a round trip. A parameter with a small closed set of values + * should say so here rather than only in prose, which the model may ignore. + */ + enum?: string[]; /** * The properties of each object in an array parameter. * diff --git a/packages/tests/providers/imageAttachments.test.ts b/packages/tests/providers/imageAttachments.test.ts new file mode 100644 index 0000000..fe66cfd --- /dev/null +++ b/packages/tests/providers/imageAttachments.test.ts @@ -0,0 +1,214 @@ +import { describe, test, expect, afterAll } from "bun:test"; +import { mkdirSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { buildContents } from "../../../providers/client"; +import { buildAnthropicMessages } from "../../../providers/anthropicClient"; +import { buildOpenAIInput } from "../../../providers/openaiClient"; +import { imageParts, imageText } from "../../../providers/images"; +import { recentMessages } from "../../../config/config"; +import type { Message } from "../../../config/types"; + +/** + * An attached image has to reach all three providers, in each one's own shape. + * + * Driven per provider rather than through one client, because this is exactly + * the kind of change that lands on one and is forgotten on the others — the + * repository has the scar: a provider client that read `toolRegistry` directly + * instead of its offered-tools parameter, merged without a textual conflict. + * + * The images are stored as paths and loaded when the request is built, so the + * cases that matter are the ordinary one and the one where the file has gone. + */ + +const fixtures = join(process.cwd(), `.test-attachments-${crypto.randomUUID()}`); +mkdirSync(fixtures, { recursive: true }); + +const png = join(fixtures, "frame.png"); +await Bun.write(png, new Uint8Array([0x89, 0x50, 0x4e, 0x47, 1, 2, 3, 4])); + +afterAll(() => { + rmSync(fixtures, { recursive: true, force: true }); +}); + +const withImage: Message[] = [ + { role: "user", content: "Look at this", images: [{ path: png, mediaType: "image/png" }] }, +]; + +const withMissingImage: Message[] = [ + { + role: "user", + content: "Look at this", + images: [{ path: join(fixtures, "gone.png"), mediaType: "image/png" }], + }, +]; + +const plain: Message[] = [{ role: "user", content: "No image here" }]; + +describe("imageParts", () => { + test("loads a file as base64 with its media type", () => { + const [loaded] = imageParts([{ path: png, mediaType: "image/png" }]); + expect(loaded!.mediaType).toBe("image/png"); + expect(loaded!.base64).toBe(Buffer.from([0x89, 0x50, 0x4e, 0x47, 1, 2, 3, 4]).toString("base64")); + }); + + test("drops a file that has gone rather than throwing", () => { + // The turn must still be sendable. A screenshot tidied away between turns + // is worth less than the conversation that refers to it. + expect(imageParts([{ path: join(fixtures, "gone.png"), mediaType: "image/png" }])).toEqual( + [], + ); + }); + + test("returns nothing for a message with no images", () => { + expect(imageParts(undefined)).toEqual([]); + expect(imageParts([])).toEqual([]); + }); +}); + +describe("imageText", () => { + test("leaves the text alone when every image loaded", () => { + expect(imageText("Look", 1, [{ mediaType: "image/png", base64: "x" }])).toBe("Look"); + }); + + test("leaves the text alone when there were no images", () => { + expect(imageText("Hello", 0, [])).toBe("Hello"); + }); + + test("names a single dropped image and tells the model not to describe it", () => { + const text = imageText("Look", 1, []); + expect(text).toContain("image is no longer readable"); + expect(text).toContain("Do not describe it"); + }); + + test("counts several dropped images", () => { + expect(imageText("Look", 3, [{ mediaType: "image/png", base64: "x" }])).toContain( + "2 images are no longer readable", + ); + }); +}); + +describe("Gemini", () => { + test("sends the image as an inlineData part after the text", () => { + const [content] = buildContents(withImage) as any[]; + expect(content.parts[0].text).toBe("Look at this"); + expect(content.parts[1].inlineData.mimeType).toBe("image/png"); + expect(content.parts[1].inlineData.data.length).toBeGreaterThan(0); + }); + + test("a message with no image is unchanged", () => { + const [content] = buildContents(plain) as any[]; + expect(content.parts).toHaveLength(1); + expect(content.parts[0].text).toBe("No image here"); + }); + + test("a missing file leaves one part, noting the image is gone", () => { + const [content] = buildContents(withMissingImage) as any[]; + expect(content.parts).toHaveLength(1); + expect(content.parts[0].text).toContain("no longer readable"); + }); +}); + +describe("Anthropic", () => { + test("sends the image as a base64 image block after the text", () => { + const [message] = buildAnthropicMessages(withImage, new Map()) as any[]; + expect(message.content[0]).toEqual({ type: "text", text: "Look at this" }); + expect(message.content[1].type).toBe("image"); + expect(message.content[1].source.media_type).toBe("image/png"); + expect(message.content[1].source.type).toBe("base64"); + }); + + test("a message with no image stays a plain string", () => { + // Not a one-element content array: the string form is what every message + // in this client has always been, and changing it for all of them would + // move the cache prefix for no gain. + const [message] = buildAnthropicMessages(plain, new Map()) as any[]; + expect(message.content).toBe("No image here"); + }); + + test("a missing file falls back to a string that says the image is gone", () => { + // Silence here is the dangerous option: the loop's text is "The image + // requested above:", so a message arriving with no image and no note + // invites the model to describe something it was never shown. + const [message] = buildAnthropicMessages(withMissingImage, new Map()) as any[]; + expect(message.content).toContain("Look at this"); + expect(message.content).toContain("no longer readable"); + }); +}); + +describe("OpenAI", () => { + test("sends the image as an input_image data URL after the text", () => { + const [message] = buildOpenAIInput(withImage, new Map()) as any[]; + expect(message.content[0]).toEqual({ type: "input_text", text: "Look at this" }); + expect(message.content[1].type).toBe("input_image"); + expect(message.content[1].image_url).toStartWith("data:image/png;base64,"); + }); + + test("a message with no image stays a plain string", () => { + const [message] = buildOpenAIInput(plain, new Map()) as any[]; + expect(message.content).toBe("No image here"); + }); + + test("a missing file falls back to a string that says the image is gone", () => { + const [message] = buildOpenAIInput(withMissingImage, new Map()) as any[]; + expect(message.content).toContain("Look at this"); + expect(message.content).toContain("no longer readable"); + }); +}); + +describe("the history window", () => { + /** + * An attached image is not a turn of the conversation. + * + * `recentMessages` keeps the last N *user* turns, and the loop follows every + * read_image with a user message carrying the picture. Counting those as + * turns shortens the window until the question itself falls out of it — with + * MAX_TURNS at 6, an agent reading five frames of a video kept one real turn + * and five copies of "The image requested above:". That is precisely the + * task read_image was added for, so it is the expected case, not an edge one. + */ + const attachment = (path: string): Message => ({ + role: "user", + content: "The image requested above:", + images: [{ path, mediaType: "image/png" }], + }); + + const conversation: Message[] = [ + { role: "user", content: "question one" }, + { role: "assistant", content: "answer one" }, + { role: "user", content: "question two" }, + attachment("/a.png"), + attachment("/b.png"), + attachment("/c.png"), + ]; + + const realTurns = (messages: Message[]) => + messages.filter((m) => m.role === "user" && !m.images?.length).length; + + test("attachments do not consume the window's turns", () => { + expect(realTurns(recentMessages(conversation, 2))).toBe(2); + }); + + test("one turn of window still keeps a real question", () => { + expect(realTurns(recentMessages(conversation, 1))).toBe(1); + }); + + test("attachments inside the window are still kept", () => { + // Skipped when counting, never dropped when slicing — the picture has to + // travel with the message that refers to it. + const windowed = recentMessages(conversation, 1); + expect(windowed.filter((m) => m.role === "user" && m.images?.length).length).toBe(3); + }); + + test("a conversation with no attachments is windowed as before", () => { + const plainTalk: Message[] = [ + { role: "user", content: "one" }, + { role: "assistant", content: "a" }, + { role: "user", content: "two" }, + { role: "assistant", content: "b" }, + ]; + expect(recentMessages(plainTalk, 1)).toEqual([ + { role: "user", content: "two" }, + { role: "assistant", content: "b" }, + ]); + }); +}); diff --git a/packages/tests/tools/process.integration.test.ts b/packages/tests/tools/process.integration.test.ts new file mode 100644 index 0000000..afcb762 --- /dev/null +++ b/packages/tests/tools/process.integration.test.ts @@ -0,0 +1,221 @@ +import { test, expect, describe, beforeEach, afterEach, afterAll } from "bun:test"; +import { + processOutputTool, + processStartTool, + processStopTool, + trackedProcessIds, + stopAllProcesses, +} from "../../../tools/process"; +import { store } from "../../../tui/src/store/ui-store"; + +/** + * INTEGRATION TESTS for the background process tools. + * + * Real processes. A mock would defeat the purpose: what is being tested is that + * something keeps running after the call returns, which is a property of the + * process table and not of the code around it. + * + * Every test stops what it started. A leaked process holds its pipes open and + * the test runner waits on them at exit. + */ + +/** Waits for a predicate, polling. Never longer than the bound. */ +async function until(predicate: () => boolean, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (predicate()) return true; + await Bun.sleep(25); + } + return predicate(); +} + +/** The id out of process_start's confirmation. */ +function idOf(result: string): string { + const match = result.match(/Started (bg\d+):/); + if (!match) throw new Error(`no id in: ${result}`); + return match[1]!; +} + +describe("process Tools - Integration Tests", () => { + const originalSetPendingCommand = store.setPendingCommand; + + beforeEach(() => { + store.setPendingCommand = async () => true; + }); + + afterEach(() => { + store.setPendingCommand = originalSetPendingCommand; + stopAllProcesses(); + }); + + afterAll(() => { + stopAllProcesses(); + }); + + describe("Argument validation", () => { + test("process_start rejects a call with no arguments", async () => { + expect(processStartTool.execute({})).rejects.toThrow(/command is required/); + }); + + test("process_output rejects a call with no arguments", async () => { + expect(processOutputTool.execute({})).rejects.toThrow(/id is required/); + }); + + test("process_stop rejects a call with no arguments", async () => { + expect(processStopTool.execute({})).rejects.toThrow(/id is required/); + }); + + test("an unknown id says so and lists what exists", async () => { + expect(processOutputTool.execute({ id: "bg99" })).rejects.toThrow( + /No background process bg99.*None have been started/s, + ); + }); + + test("a cwd outside the workspace is refused", async () => { + expect( + processStartTool.execute({ command: "echo hi", cwd: "/etc" }), + ).rejects.toThrow(/escapes the workspace/); + }); + }); + + describe("Starting and reading", () => { + test("returns immediately with an id while the process runs on", async () => { + const started = await processStartTool.execute({ command: "sleep 5" }); + const id = idOf(started); + + expect(trackedProcessIds()).toContain(id); + expect(await processOutputTool.execute({ id })).toContain("running for"); + }); + + test("collects output produced after the call returned", async () => { + const id = idOf(await processStartTool.execute({ command: "sleep 0.3; echo late" })); + + // The point of the tool: nothing was waited for, so the output cannot + // have existed when process_start returned. + expect(await processOutputTool.execute({ id })).toContain("No new output"); + + await Bun.sleep(1200); + expect(await processOutputTool.execute({ id })).toContain("late"); + }); + + test("output already returned is not repeated", async () => { + const id = idOf(await processStartTool.execute({ command: "echo once" })); + await until(() => trackedProcessIds().includes(id)); + await Bun.sleep(500); + + expect(await processOutputTool.execute({ id })).toContain("once"); + expect(await processOutputTool.execute({ id })).toContain("No new output"); + }); + + test("captures stderr as well as stdout", async () => { + const id = idOf( + await processStartTool.execute({ command: "echo to-err 1>&2" }), + ); + await Bun.sleep(500); + expect(await processOutputTool.execute({ id })).toContain("to-err"); + }); + + test("reports a process that has exited", async () => { + const id = idOf(await processStartTool.execute({ command: "exit 3" })); + await Bun.sleep(500); + expect(await processOutputTool.execute({ id })).toContain("exited with code 3"); + }); + }); + + describe("Stopping", () => { + test("stops a running process and forgets the id", async () => { + const id = idOf(await processStartTool.execute({ command: "sleep 30" })); + + const stopped = await processStopTool.execute({ id }); + expect(stopped).toContain(`Stopped ${id}`); + expect(trackedProcessIds()).not.toContain(id); + }); + + test("returns output that was never read", async () => { + const id = idOf(await processStartTool.execute({ command: "echo parting; sleep 30" })); + await Bun.sleep(500); + + expect(await processStopTool.execute({ id })).toContain("parting"); + }); + + test("stopping twice reports the id is unknown rather than hanging", async () => { + const id = idOf(await processStartTool.execute({ command: "sleep 30" })); + await processStopTool.execute({ id }); + + expect(processStopTool.execute({ id })).rejects.toThrow(/No background process/); + }); + + test("stops the whole tree, not just the shell", async () => { + // The leak this tool exists to prevent. Killing only the spawned shell + // leaves its children holding ports and files — measured before the fix: + // two processes before the stop, one still alive after it. + // + // `; true` keeps bash from exec-ing into python, so there is a real + // parent and a real child rather than one process wearing both hats. + const marker = `wooptest${crypto.randomUUID().replace(/-/g, "")}`; + const alive = () => + new TextDecoder() + .decode(Bun.spawnSync({ cmd: ["pgrep", "-f", marker] }).stdout) + .trim() + .split("\n") + .filter(Boolean); + + const id = idOf( + await processStartTool.execute({ + command: `python3 -c "import time; time.sleep(120)" ${marker} ; true`, + }), + ); + + expect(await until(() => alive().length > 0)).toBe(true); + + await processStopTool.execute({ id }); + + const gone = await until(() => alive().length === 0); + // Kill anything the fix failed to, so a failure here does not leak a + // sleeping process into the rest of the run. + for (const pid of alive()) { + try { + process.kill(Number(pid), 9); + } catch { + // Already gone between the listing and the signal. + } + } + + expect(gone).toBe(true); + }); + + test("an id is never reused after its process is stopped", async () => { + // A model holds the whole transcript, so it always might quote an old id. + // Reusing `bg1` would make that a silent mix-up — reading or killing a + // different process and getting a plausible answer — instead of an error. + const first = idOf(await processStartTool.execute({ command: "sleep 30" })); + await processStopTool.execute({ id: first }); + + const second = idOf(await processStartTool.execute({ command: "sleep 30" })); + expect(second).not.toBe(first); + expect(processOutputTool.execute({ id: first })).rejects.toThrow( + /No background process/, + ); + }); + + test("stopAllProcesses ends everything", async () => { + await processStartTool.execute({ command: "sleep 30" }); + await processStartTool.execute({ command: "sleep 30" }); + expect(trackedProcessIds().length).toBe(2); + + stopAllProcesses(); + expect(trackedProcessIds()).toEqual([]); + }); + }); + + describe("Approval", () => { + test("a rejected command starts nothing", async () => { + store.setPendingCommand = async () => false; + + const result = await processStartTool.execute({ command: "sleep 30" }); + + expect(result).toContain("rejected by user"); + expect(trackedProcessIds()).toEqual([]); + }); + }); +}); diff --git a/packages/tests/tools/readImage.integration.test.ts b/packages/tests/tools/readImage.integration.test.ts new file mode 100644 index 0000000..65cef6a --- /dev/null +++ b/packages/tests/tools/readImage.integration.test.ts @@ -0,0 +1,185 @@ +import { test, expect, describe, beforeEach, afterAll } from "bun:test"; +import { mkdirSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { readImageTool, takePendingImages } from "../../../tools/readImage"; + +/** + * INTEGRATION TESTS for the read_image tool. + * + * Real files, and inside the workspace: the tool routes through + * `resolveWorkspacePath`, which refuses anything outside it, so a fixture in + * `tmpdir()` would be rejected before any of this was exercised. + * + * The fixtures are built headers rather than photographs. That is what the tool + * reads — it identifies the format from magic bytes and takes the dimensions + * from the header, and never decodes a pixel — so a crafted header exercises + * exactly the code under test. The bytes are real ones in the real layout. + * + * UUID in the directory name because `Date.now()` collides between concurrent + * runs, and the loser has its fixtures deleted mid-test. + */ + +const fixtures = join(process.cwd(), `.test-images-${crypto.randomUUID()}`); +mkdirSync(fixtures, { recursive: true }); + +afterAll(() => { + rmSync(fixtures, { recursive: true, force: true }); +}); + +/** A PNG whose IHDR declares the given size. */ +async function writePng(name: string, width: number, height: number): Promise { + const bytes = new Uint8Array(33); + bytes.set([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a], 0); + const view = new DataView(bytes.buffer); + view.setUint32(8, 13); + bytes.set([0x49, 0x48, 0x44, 0x52], 12); // "IHDR" + view.setUint32(16, width); + view.setUint32(20, height); + + const path = join(fixtures, name); + await Bun.write(path, bytes); + return path; +} + +/** A JPEG with a single SOF0 segment declaring the given size. */ +async function writeJpeg(name: string, width: number, height: number): Promise { + const bytes = new Uint8Array(20); + const view = new DataView(bytes.buffer); + bytes.set([0xff, 0xd8, 0xff], 0); // SOI, then the first marker + bytes[3] = 0xc0; // SOF0 + view.setUint16(4, 11); // segment length + bytes[6] = 8; // sample precision + view.setUint16(7, height); + view.setUint16(9, width); + + const path = join(fixtures, name); + await Bun.write(path, bytes); + return path; +} + +/** The path as the tool is given it: relative to the workspace. */ +const relative = (absolute: string) => absolute.slice(process.cwd().length + 1); + +describe("read_image Tool - Integration Tests", () => { + beforeEach(() => { + // A queued image from an earlier test would be attributed to this one. + takePendingImages(); + }); + + describe("Argument validation", () => { + test("rejects a call with no arguments", async () => { + expect(readImageTool.execute({})).rejects.toThrow(/File path is required/); + }); + + test("rejects an empty path", async () => { + expect(readImageTool.execute({ path: " " })).rejects.toThrow( + /File path is required/, + ); + }); + + test("names a missing file rather than reporting ENOENT", async () => { + expect(readImageTool.execute({ path: "no-such-image.png" })).rejects.toThrow( + /no-such-image\.png does not exist/, + ); + }); + + test("refuses a path outside the workspace", async () => { + expect(readImageTool.execute({ path: "../../../etc/hosts" })).rejects.toThrow( + /escapes the workspace/, + ); + }); + + test("queues nothing when the call is rejected", async () => { + await readImageTool.execute({ path: "missing.png" }).catch(() => {}); + expect(takePendingImages()).toEqual([]); + }); + }); + + describe("Format detection", () => { + test("identifies a PNG and reports its dimensions", async () => { + const path = await writePng("shot.png", 1920, 1080); + const result = await readImageTool.execute({ path: relative(path) }); + + expect(result).toContain("1920x1080"); + expect(result).toContain("image/png"); + }); + + test("identifies a JPEG and reports its dimensions", async () => { + const path = await writeJpeg("frame.jpg", 640, 480); + const result = await readImageTool.execute({ path: relative(path) }); + + expect(result).toContain("640x480"); + expect(result).toContain("image/jpeg"); + }); + + test("judges the format by its bytes, not its extension", async () => { + // A PNG named .jpg. Trusting the extension would report the wrong media + // type to the provider, which rejects the request rather than guessing. + const path = await writePng("actually-a-png.jpg", 8, 8); + const result = await readImageTool.execute({ path: relative(path) }); + + expect(result).toContain("image/png"); + }); + + test("refuses a file that is not an image, and says what to use", async () => { + const path = join(fixtures, "notes.txt"); + await Bun.write(path, "just text"); + + expect(readImageTool.execute({ path: relative(path) })).rejects.toThrow( + /not a PNG, JPEG, GIF or WebP.*read_file/s, + ); + }); + + test("refuses an empty file", async () => { + const path = join(fixtures, "empty.png"); + await Bun.write(path, ""); + + expect(readImageTool.execute({ path: relative(path) })).rejects.toThrow(/is empty/); + }); + + test("refuses an image too large for a provider, with a way forward", async () => { + const path = join(fixtures, "huge.png"); + const bytes = new Uint8Array(4 * 1024 * 1024); + bytes.set([0x89, 0x50, 0x4e, 0x47], 0); + await Bun.write(path, bytes); + + expect(readImageTool.execute({ path: relative(path) })).rejects.toThrow( + /over the.*Resize or crop/s, + ); + }); + }); + + describe("Queueing", () => { + test("queues the resolved path and media type for the loop to attach", async () => { + const path = await writePng("queued.png", 4, 4); + await readImageTool.execute({ path: relative(path) }); + + const pending = takePendingImages(); + expect(pending).toHaveLength(1); + expect(pending[0]!.mediaType).toBe("image/png"); + // Absolute and resolved, so a later cwd change cannot strand it. + expect(pending[0]!.path).toBe(path); + }); + + test("taking the queue empties it", async () => { + const path = await writePng("once.png", 4, 4); + await readImageTool.execute({ path: relative(path) }); + + expect(takePendingImages()).toHaveLength(1); + expect(takePendingImages()).toEqual([]); + }); + + test("two reads queue two images in order", async () => { + const first = await writePng("one.png", 2, 2); + const second = await writeJpeg("two.jpg", 3, 3); + + await readImageTool.execute({ path: relative(first) }); + await readImageTool.execute({ path: relative(second) }); + + expect(takePendingImages().map((image) => image.mediaType)).toEqual([ + "image/png", + "image/jpeg", + ]); + }); + }); +}); diff --git a/packages/tests/tools/repl.integration.test.ts b/packages/tests/tools/repl.integration.test.ts new file mode 100644 index 0000000..a499d06 --- /dev/null +++ b/packages/tests/tools/repl.integration.test.ts @@ -0,0 +1,210 @@ +import { test, expect, describe, beforeEach, afterEach, afterAll } from "bun:test"; +import { replTool } from "../../../tools/repl"; +import { closeReplSessions, openReplLanguages } from "../../../tools/replSession"; +import { store } from "../../../tui/src/store/ui-store"; + +/** + * INTEGRATION TESTS for the repl tool. + * + * Real interpreters, spawned for real. The whole point of the tool is that a + * process outlives the call, and nothing about that survives being mocked — + * a fake that returned canned output would pass while the sessions leaked. + * + * Only the approval prompt is faked, because there is no human here. + */ + +describe("repl Tool - Integration Tests", () => { + const originalSetPendingCommand = store.setPendingCommand; + + beforeEach(() => { + store.setPendingCommand = async () => true; + }); + + afterEach(() => { + store.setPendingCommand = originalSetPendingCommand; + // Each test starts from no interpreter. Without this a session from an + // earlier test answers the next one with variables it never set — which is + // exactly the bug the per-turn scope exists to prevent, and a test suite + // that leaked it would be unable to detect it. + closeReplSessions(); + }); + + afterAll(() => { + closeReplSessions(); + }); + + describe("Argument validation", () => { + test("rejects a call with no arguments", async () => { + expect(replTool.execute({})).rejects.toThrow(/language must be one of/); + }); + + test("rejects an unknown language", async () => { + expect(replTool.execute({ language: "ruby", code: "1" })).rejects.toThrow( + /language must be one of/, + ); + }); + + test("rejects empty code", async () => { + expect(replTool.execute({ language: "python", code: " " })).rejects.toThrow( + /code is required/, + ); + }); + + test("rejects a non-positive timeout", async () => { + expect( + replTool.execute({ language: "python", code: "1", timeout: 0 }), + ).rejects.toThrow(/timeout must be a positive number/); + }); + + test("validates before spawning anything", async () => { + await replTool.execute({ language: "nope" }).catch(() => {}); + expect(openReplLanguages()).toEqual([]); + }); + }); + + describe("Python sessions", () => { + test("evaluates and prints a trailing expression", async () => { + const result = await replTool.execute({ language: "python", code: "2 + 3" }); + expect(result).toContain("5"); + }); + + test("state survives between calls", async () => { + await replTool.execute({ language: "python", code: "values = [1, 2, 3, 4]" }); + const result = await replTool.execute({ language: "python", code: "sum(values)" }); + expect(result).toContain("10"); + }); + + test("reports an assignment that printed nothing as a success", async () => { + const result = await replTool.execute({ language: "python", code: "x = 1" }); + expect(result).toMatch(/produced no output/); + }); + + test("captures stdout in order with the expression value", async () => { + const result = await replTool.execute({ + language: "python", + code: "print('first')\n'second'", + }); + expect(result.indexOf("first")).toBeLessThan(result.indexOf("second")); + }); + + test("returns a traceback without killing the session", async () => { + const failed = await replTool.execute({ language: "python", code: "1 / 0" }); + expect(failed).toContain("ZeroDivisionError"); + + const after = await replTool.execute({ language: "python", code: "'alive'" }); + expect(after).toContain("alive"); + }); + + test("handles unicode and emoji", async () => { + const result = await replTool.execute({ + language: "python", + code: "'héllo 🌍 世界'", + }); + expect(result).toContain("🌍"); + expect(result).toContain("世界"); + }); + + test("restart discards the previous state", async () => { + await replTool.execute({ language: "python", code: "kept = 99" }); + const result = await replTool.execute({ + language: "python", + code: "'kept' in dir()", + restart: true, + }); + expect(result).toContain("False"); + }); + }); + + describe("Node sessions", () => { + test("evaluates an expression", async () => { + const result = await replTool.execute({ language: "node", code: "6 * 7" }); + expect(result).toContain("42"); + }); + + test("a top-level var survives between calls", async () => { + await replTool.execute({ language: "node", code: "var total = 8" }); + const result = await replTool.execute({ language: "node", code: "total + 1" }); + expect(result).toContain("9"); + }); + + test("reports a thrown error without killing the session", async () => { + const failed = await replTool.execute({ + language: "node", + code: "throw new Error('boom')", + }); + expect(failed).toContain("boom"); + + const after = await replTool.execute({ language: "node", code: "'alive'" }); + expect(after).toContain("alive"); + }); + }); + + describe("Session lifetime", () => { + test("python and node are separate sessions", async () => { + await replTool.execute({ language: "python", code: "shared = 'py'" }); + await replTool.execute({ language: "node", code: "var shared = 'node'" }); + + expect(await replTool.execute({ language: "python", code: "shared" })).toContain("py"); + expect(await replTool.execute({ language: "node", code: "shared" })).toContain("node"); + expect(openReplLanguages().sort()).toEqual(["node", "python"]); + }); + + test("closeReplSessions ends every session", async () => { + await replTool.execute({ language: "python", code: "1" }); + await replTool.execute({ language: "node", code: "1" }); + expect(openReplLanguages().length).toBe(2); + + closeReplSessions(); + expect(openReplLanguages()).toEqual([]); + }); + + test("state is gone after the sessions are closed", async () => { + await replTool.execute({ language: "python", code: "carried = 5" }); + closeReplSessions(); + + const result = await replTool.execute({ + language: "python", + code: "'carried' in dir()", + }); + expect(result).toContain("False"); + }); + }); + + describe("Timeouts", () => { + test("a runaway evaluation ends and discards the session", async () => { + const result = await replTool.execute({ + language: "python", + code: "import time; time.sleep(30)", + timeout: 1, + }); + + expect(result).toContain("timed out after 1 seconds"); + expect(openReplLanguages()).toEqual([]); + }); + + test("the next call after a timeout starts a working session", async () => { + await replTool.execute({ + language: "python", + code: "import time; time.sleep(30)", + timeout: 1, + }); + + const result = await replTool.execute({ language: "python", code: "'recovered'" }); + expect(result).toContain("recovered"); + }); + }); + + describe("Approval", () => { + test("a rejected call runs nothing and starts no session", async () => { + store.setPendingCommand = async () => false; + + const result = await replTool.execute({ + language: "python", + code: "import subprocess; subprocess.run(['true'])", + }); + + expect(result).toContain("rejected by user"); + expect(openReplLanguages()).toEqual([]); + }); + }); +}); diff --git a/packages/tests/tools/terminal.integration.test.ts b/packages/tests/tools/terminal.integration.test.ts index c19cbac..7dbdc0c 100644 --- a/packages/tests/tools/terminal.integration.test.ts +++ b/packages/tests/tools/terminal.integration.test.ts @@ -26,6 +26,45 @@ describe("terminal Tool - Integration Tests", () => { const stdoutOf = (result: string) => result.split("STDOUT:\n")[1]?.split("\n\nSTDERR:")[0] ?? ""; + describe("The shell commands run under", () => { + /** + * Commands used to run under `sh`, which is dash on Debian — and the + * benchmark containers are Debian. A recorded run lost six commands to + * `sh: 1: Syntax error: Bad for loop variable` and nothing in the message + * told the model the shell was the problem rather than its command. + * + * These are bash constructs dash does not have. They are skipped where + * bash is genuinely absent, which is a real configuration rather than a + * failure — the fallback to sh is deliberate. + */ + const hasBash = Bun.which("bash") !== null; + + test.skipIf(!hasBash)("runs a C-style for loop", async () => { + const result = await terminalTool.execute({ + command: "for ((i=0; i<3; i++)); do echo n$i; done", + }); + + expect(result).toContain("Exit code: 0"); + expect(stdoutOf(result)).toBe("n0\nn1\nn2\n"); + }); + + test.skipIf(!hasBash)("runs a [[ ]] test", async () => { + const result = await terminalTool.execute({ + command: '[[ "abc" == a* ]] && echo matched', + }); + + expect(stdoutOf(result)).toBe("matched\n"); + }); + + test.skipIf(!hasBash)("expands an array", async () => { + const result = await terminalTool.execute({ + command: "arr=(one two three); echo ${arr[1]}", + }); + + expect(stdoutOf(result)).toBe("two\n"); + }); + }); + describe("Basic Execution", () => { test("executes simple command", async () => { const result = await terminalTool.execute({ @@ -296,7 +335,9 @@ describe("terminal Tool - Integration Tests", () => { }); test("rejects unquoted background operators without rejecting && or redirection", async () => { - await expect(terminalTool.execute({ command: "echo ready &" })).resolves.toContain("Background processes"); + // Still refused, and now pointed at the tool that does this instead of + // told to give up — process_start is what a trailing & was reaching for. + await expect(terminalTool.execute({ command: "echo ready &" })).resolves.toContain("process_start"); await expect(terminalTool.execute({ command: "echo ready && echo done" })).resolves.toContain("done"); await expect(terminalTool.execute({ command: "echo warning >&2" })).resolves.toContain("warning"); }); diff --git a/providers/anthropicClient.ts b/providers/anthropicClient.ts index 3b7e754..0e4fe74 100644 --- a/providers/anthropicClient.ts +++ b/providers/anthropicClient.ts @@ -5,6 +5,7 @@ import { thinkingBudget } from "./client"; import { defaultModelForProvider } from "./modelCatalog"; import type { Message, ProviderClient, StreamEvent, TokenUsage, Tool } from "../config/types"; import { toolInputSchema } from "../config/toolSchema"; +import { imageParts, imageText } from "./images"; import { classifyFailure, delay, maxAttempts } from "../runtime/retry"; /** Only the surface this client uses, so a test can supply a fake. */ @@ -411,9 +412,32 @@ export function buildAnthropicMessages( emitted.add(i); switch (message.role) { - case "user": - rendered.push({ role: "user", content: message.content }); + case "user": { + const images = imageParts(message.images); + const text = imageText(message.content, message.images?.length ?? 0, images); + rendered.push( + images.length === 0 + ? { role: "user", content: text } + : { + role: "user", + content: [ + { type: "text", text }, + ...images.map( + (image) => + ({ + type: "image", + source: { + type: "base64", + media_type: image.mediaType as "image/png", + data: image.base64, + }, + }) as const, + ), + ], + }, + ); break; + } case "assistant": // A turn that streamed nothing but tool calls leaves an empty assistant diff --git a/providers/client.ts b/providers/client.ts index a6a513b..1d0a7b5 100644 --- a/providers/client.ts +++ b/providers/client.ts @@ -4,6 +4,7 @@ import { SYSTEM_PROMPT } from "../config/systemPrompt"; import type { Message, ProviderClient, StreamEvent, TokenUsage, Tool } from "../config/types"; import { toolInputSchema, type JsonSchema, type JsonSchemaType } from "../config/toolSchema"; import { unsupportedProviderMessage } from "./providerRegistry"; +import { imageParts, imageText } from "./images"; import { classifyFailure, delay, maxAttempts } from "../runtime/retry"; import { DEFAULT_MODEL_ID, @@ -372,9 +373,19 @@ export function buildContents(messages: Message[]) { const message = messages[i]!; switch (message.role) { - case "user": - contents.push({ role: "user", parts: [{ text: message.content }] }); + case "user": { + const images = imageParts(message.images); + contents.push({ + role: "user", + parts: [ + { text: imageText(message.content, message.images?.length ?? 0, images) }, + ...images.map((image) => ({ + inlineData: { mimeType: image.mediaType, data: image.base64 }, + })), + ], + }); break; + } case "assistant": contents.push({ role: "model", parts: [{ text: message.content }] }); diff --git a/providers/images.ts b/providers/images.ts new file mode 100644 index 0000000..1458634 --- /dev/null +++ b/providers/images.ts @@ -0,0 +1,71 @@ +/** + * Turning an attached image into bytes, once, for all three clients. + * + * `ImageAttachment` stores a path rather than the bytes, so that a saved + * session stays small and compaction keeps moving short strings. The cost of + * that is this step: the file has to be read at the moment a request is built, + * and by then it may have changed or gone — the agent extracts a frame to a + * scratch path, and nothing stops a later turn overwriting it. + * + * So a file that cannot be read is dropped, not raised. A turn that fails + * outright because a screenshot from four messages ago was tidied away is worse + * than a turn that proceeds without it, and the text of the message already + * says what the image was. The three render sites each add their own text note + * in the provider's own shape. + * + * Read synchronously because all three request builders are synchronous + * functions over `Message[]`, and making them async to load a file would ripple + * through every caller and their tests. `readFileSync` is the sanctioned form + * here; the rule the gate enforces is against the `fs/promises` read/write pair. + */ + +import { readFileSync } from "node:fs"; +import type { ImageAttachment } from "../config/types"; + +export interface LoadedImage { + mediaType: string; + base64: string; +} + +export function imageParts(images: ImageAttachment[] | undefined): LoadedImage[] { + if (!images?.length) return []; + + const loaded: LoadedImage[] = []; + for (const image of images) { + try { + loaded.push({ + mediaType: image.mediaType, + base64: readFileSync(image.path).toString("base64"), + }); + } catch { + // Gone or unreadable since it was attached. Reported by `imageText` + // rather than raised; a dropped image must not cost the turn. + } + } + + return loaded; +} + +/** + * The message text, with a note when an image could not be loaded. + * + * Silence here is the dangerous option. The text the loop writes is "The image + * requested above:", so a message that arrives with the image dropped and the + * text untouched invites the model to describe something it was never shown — + * and it has every reason to believe it was shown one. Saying the file is gone + * turns a hallucination into a retry. + */ +export function imageText( + content: string, + attached: number, + loaded: LoadedImage[], +): string { + const missing = attached - loaded.length; + if (missing <= 0) return content; + + const subject = missing === 1 ? "image is" : `${missing} images are`; + return ( + `${content}\n\n[${subject} no longer readable and could not be attached. ` + + `Do not describe ${missing === 1 ? "it" : "them"}; read the file again if it still matters.]` + ); +} diff --git a/providers/openaiClient.ts b/providers/openaiClient.ts index cd558c5..0631a01 100644 --- a/providers/openaiClient.ts +++ b/providers/openaiClient.ts @@ -5,6 +5,7 @@ import { thinkingBudget } from "./client"; import { defaultModelForProvider } from "./modelCatalog"; import type { Message, ProviderClient, StreamEvent, TokenUsage, Tool } from "../config/types"; import { toolInputSchema } from "../config/toolSchema"; +import { imageParts, imageText } from "./images"; import { classifyFailure, delay, maxAttempts } from "../runtime/retry"; /** Only the surface this client uses, so a test can supply a fake. */ @@ -355,9 +356,29 @@ export function buildOpenAIInput( emitted.add(i); switch (message.role) { - case "user": - input.push({ role: "user", content: message.content }); + case "user": { + const images = imageParts(message.images); + const text = imageText(message.content, message.images?.length ?? 0, images); + input.push( + images.length === 0 + ? { role: "user", content: text } + : { + role: "user", + content: [ + { type: "input_text", text }, + // A data URL rather than an uploaded file id: the client is + // stateless by choice (`store: false`), so there is nothing + // to hang an uploaded file's lifetime on. + ...images.map((image) => ({ + type: "input_image" as const, + image_url: `data:${image.mediaType};base64,${image.base64}`, + detail: "auto" as const, + })), + ], + }, + ); break; + } case "assistant": // A turn that streamed nothing but tool calls leaves an empty assistant diff --git a/runtime/codeEffects.test.ts b/runtime/codeEffects.test.ts new file mode 100644 index 0000000..63b1fe8 --- /dev/null +++ b/runtime/codeEffects.test.ts @@ -0,0 +1,188 @@ +import { describe, test, expect } from "bun:test"; +import { classifyCode, classifyInvocation, codeShellsOut } from "./toolEffects"; +import { blockedInPlanMode } from "./planMode"; + +/** + * The REPL's half of plan mode's second gate. + * + * Both directions everywhere, for the reason `planMode.test.ts` states: a gate + * tested only on what it blocks passes just as well when it blocks everything. + * + * The refusals here are the ones that matter. `repl` carries its source in a + * `code` argument, which `commandOf` does not read and `classifyCommand`'s + * inline-script pattern does not match — so before `classifyInvocation` existed + * a REPL call opening a file for writing was not seen by the gate at all, and + * plan mode allowed it through while reporting nothing had changed. + */ + +describe("classifyCode — what interpreter source does", () => { + describe("writes", () => { + test("a Python file opened for writing", () => { + expect(classifyCode("open('notes.txt', 'w').write('x')").writes).toBe(true); + }); + + test("a Python file opened for appending", () => { + expect(classifyCode("f = open('log.txt', 'a')").writes).toBe(true); + }); + + test("pathlib's write_text", () => { + expect(classifyCode("Path('out.txt').write_text('x')").writes).toBe(true); + }); + + test("os.remove and friends", () => { + expect(classifyCode("os.remove('gone.txt')").writes).toBe(true); + expect(classifyCode("os.makedirs('a/b')").writes).toBe(true); + expect(classifyCode("shutil.move('a', 'b')").writes).toBe(true); + }); + + test("Node's writeFileSync", () => { + expect(classifyCode("fs.writeFileSync('out.js', src)").writes).toBe(true); + }); + + test("Bun.write", () => { + expect(classifyCode("Bun.write('out.txt', data)").writes).toBe(true); + }); + }); + + describe("does not write", () => { + test("a file opened for reading", () => { + expect(classifyCode("data = open('input.txt').read()").writes).toBe(false); + }); + + test("readFileSync", () => { + expect(classifyCode("const src = fs.readFileSync('vm.js', 'utf8')").writes).toBe(false); + }); + + test("arithmetic that shifts right", () => { + // The reason this file exists. Under the shell rules `>>` is a redirect + // and this reads as writing to a file called `16` — and the benchmark + // trials are full of exactly this, walking ELF headers and packing ints. + expect(classifyCode("value = 0x4395e4 >> 16").writes).toBe(false); + }); + + test("a comparison", () => { + expect(classifyCode("if width > 100: print('wide')").writes).toBe(false); + }); + + test("statements separated by semicolons", () => { + // `;` is a segment separator to the shell classifier, and `rm` at the + // head of a segment is a writing command — but here it is a variable. + expect(classifyCode("rm = 5; total = rm * 2").writes).toBe(false); + }); + + test("a bitwise or", () => { + expect(classifyCode("flags = a | b").writes).toBe(false); + }); + + test("printing to stdout", () => { + expect(classifyCode("print('hello')").writes).toBe(false); + expect(classifyCode("process.stdout.write('hi')").writes).toBe(false); + }); + + test("empty source", () => { + expect(classifyCode("")).toEqual({ writes: false, verifies: false }); + expect(classifyCode(" ")).toEqual({ writes: false, verifies: false }); + }); + }); + + describe("shelling out is treated as writing", () => { + test.each([ + ["subprocess.run(['make'])", "subprocess"], + ["os.system('make')", "os.system"], + ["execSync('make')", "execSync"], + ["require('child_process').spawn('sh')", "child_process"], + ])("%s", (code) => { + // Its argument is built at runtime, so there is nothing here to read. + // Unrecognised means destructive, as everywhere else in this codebase. + expect(codeShellsOut(code)).toBe(true); + expect(classifyCode(code).writes).toBe(true); + }); + + test("ordinary source does not", () => { + expect(codeShellsOut("total = sum(values)")).toBe(false); + }); + }); + + describe("verifies", () => { + test("an assertion counts as a check", () => { + expect(classifyCode("assert total == 42").verifies).toBe(true); + }); + + test("arithmetic does not", () => { + expect(classifyCode("total = 1 + 1").verifies).toBe(false); + }); + }); +}); + +describe("classifyInvocation — one entry point for both shapes", () => { + test("reads a code argument as source", () => { + expect(classifyInvocation({ code: "value >> 16" }).writes).toBe(false); + expect(classifyInvocation({ code: "open('f','w')" }).writes).toBe(true); + }); + + test("reads a command argument as a shell line", () => { + expect(classifyInvocation({ command: "sed -i s/a/b/ f.c" }).writes).toBe(true); + expect(classifyInvocation({ command: "grep -r foo ." }).writes).toBe(false); + }); + + test("an argumentless call changes nothing", () => { + expect(classifyInvocation({})).toEqual({ writes: false, verifies: false }); + }); +}); + +describe("plan mode gates a repl call on its source", () => { + test("refuses source that writes a file", () => { + expect(blockedInPlanMode("repl", { language: "python", code: "open('x','w')" })).toBe( + true, + ); + }); + + test("refuses source that shells out", () => { + expect( + blockedInPlanMode("repl", { language: "python", code: "subprocess.run(['rm','x'])" }), + ).toBe(true); + }); + + test("allows source that only reads and computes", () => { + // The permission half. Inspection is most of what planning is, and a REPL + // that could not be used to look at anything would be withheld in the one + // mode it is most useful in. + expect( + blockedInPlanMode("repl", { + language: "python", + code: "data = open('gates.txt').read()\nlen(data)", + }), + ).toBe(false); + }); + + test("allows arithmetic containing a right shift", () => { + expect(blockedInPlanMode("repl", { language: "node", code: "v >> 16" })).toBe(false); + }); + + test("still refuses a run_terminal write", () => { + expect(blockedInPlanMode("run_terminal", { command: "cat > f.txt" })).toBe(true); + }); +}); + +describe("plan mode and the background process tools", () => { + test("refuses starting a command that writes", () => { + expect(blockedInPlanMode("process_start", { command: "rm -rf build" })).toBe(true); + }); + + test("allows starting a server", () => { + expect(blockedInPlanMode("process_start", { command: "python3 -m http.server" })).toBe( + false, + ); + }); + + test("allows reading and stopping, which change nothing", () => { + expect(blockedInPlanMode("process_output", { id: "bg1" })).toBe(false); + expect(blockedInPlanMode("process_stop", { id: "bg1" })).toBe(false); + }); +}); + +describe("plan mode and read_image", () => { + test("allows it — looking at a file changes nothing", () => { + expect(blockedInPlanMode("read_image", { path: "shot.png" })).toBe(false); + }); +}); diff --git a/runtime/loop.ts b/runtime/loop.ts index 51441f3..0409508 100644 --- a/runtime/loop.ts +++ b/runtime/loop.ts @@ -1,4 +1,6 @@ import { getTool, toolRegistry } from "../tools"; +import { closeReplSessions } from "../tools/replSession"; +import { takePendingImages } from "../tools/readImage"; import { blockedInPlanMode, planModeRefusal, planModeTools } from "./planMode"; import { isRetryableError } from "./retry"; import { compactToolHistory, toolHistoryBudget } from "./compaction"; @@ -519,6 +521,23 @@ async function executeToolCall( }); pushToolResult(messages, toolCall, toolResult); + + // Images ride on a user message after the tool result, because that is the + // only shape all three providers accept — see `ImageAttachment`. Pushed here, + // after the result and before the next request, so the model sees the tool's + // description of the file and the file itself in the order it asked for them. + const images = takePendingImages(); + if (images.length > 0) { + messages.push({ + role: "user", + content: + images.length === 1 + ? "The image requested above:" + : `The ${images.length} images requested above:`, + images, + }); + } + return { kind: "continue" }; } @@ -787,6 +806,22 @@ export async function agentLoop( callbacks.onError?.(agentError); throw agentError; } finally { + // Interpreter sessions are scoped to the turn, and this is the only place + // that runs on every one of its exits — completion, cancellation, an + // exhausted budget, a provider failure. A session that outlived its turn + // would answer the next one with variables nobody in that conversation set. + // + // Background processes deliberately do not end here: a server started this + // turn has to still be up for the user in the next one, so `process_stop` + // and session exit are what end those. + closeReplSessions(); + + // An image read on the last call before a cancellation is never attached, + // because the path that attaches them returns before reaching it. Dropping + // it here is what stops it arriving in the next turn, where it would be + // introduced as "the image requested above" with no such request in sight. + takePendingImages(); + // Every exit is a turn that ended and is worth a record: a normal // completion, a rejected edit, cancellation, an exhausted budget, a // provider failure. A finally is what makes that exactly one record per diff --git a/runtime/planMode.ts b/runtime/planMode.ts index 04a2c7d..110d8b4 100644 --- a/runtime/planMode.ts +++ b/runtime/planMode.ts @@ -24,7 +24,7 @@ */ import type { Tool } from "../config/types"; -import { classifyCommand, commandOf, toolEffect } from "./toolEffects"; +import { classifyInvocation, toolEffect } from "./toolEffects"; /** Which mode a session is in. Cycled with Tab; never written to config. */ export type SessionMode = "build" | "plan"; @@ -98,10 +98,13 @@ export function planModeTools(registry: readonly Tool[]): Tool[] { /** * Would this call change the repository? * - * Judged from the effects table for a tool, and from the command itself for a - * shell tool — the same `classifyCommand` the runtime already uses to tell an + * Judged from the effects table for a tool, and from the call itself for a + * shell tool — the same `classifyInvocation` the runtime already uses to tell an * edit from a verification, so there is no second table of command names to keep - * in step with the first. + * in step with the first. That covers `repl` as well as `run_terminal`: a + * `code` argument is read as source and a `command` argument as a shell line, + * because a persistent interpreter reaches the disk exactly as `python3 -c` + * does and this gate is the only one that can see the difference. */ export function blockedInPlanMode( name: string, @@ -109,7 +112,7 @@ export function blockedInPlanMode( ): boolean { const effect = toolEffect(name); - if (effect === "shell") return classifyCommand(commandOf(args)).writes; + if (effect === "shell") return classifyInvocation(args).writes; return !PLANNABLE_EFFECTS.has(effect); } diff --git a/runtime/toolEffects.ts b/runtime/toolEffects.ts index f45553e..fdbdada 100644 --- a/runtime/toolEffects.ts +++ b/runtime/toolEffects.ts @@ -26,6 +26,17 @@ export const TOOL_EFFECTS: Record> = edit_file: "write", run_terminal: "shell", run_tests: "shell", + // Runs arbitrary source in a live interpreter, which reaches the disk exactly + // as `python3 -c` does. Graded as shell so that plan mode judges it per call + // from the code rather than letting the tool name decide. + repl: "shell", + process_start: "shell", + // Reading a started process's output and stopping it change nothing. They are + // still shell rather than read: both name a process this session spawned, and + // a mode that refuses to start one has no reason to allow steering it. + process_output: "shell", + process_stop: "shell", + read_image: "read", ask_user: "ask", // Records the agent's own task list. It reaches the UI and nothing else — no // file, no command — which is why it is neither a write nor a read of the @@ -213,3 +224,74 @@ export function commandOf(args: Record): string { } return ""; } + +/** The argument a REPL call carries interpreter source in. */ +export function codeOf(args: Record): string { + const value = args.code; + return typeof value === "string" ? value : ""; +} + +/** + * What a block of interpreter source does. + * + * Separate from `classifyCommand` because shell syntax read over Python or + * JavaScript is actively wrong, not merely imprecise. `segmentsOf` splits on + * `;` and `|`, which are statement and bitwise-or in both languages, and the + * redirect test `/>>?\s*[^&\s>]/` matches `value >> 16` — so `int.from_bytes` + * arithmetic would be classified as writing to a file. The benchmark run is + * full of exactly that shift: judging it with the shell rules would refuse + * half of plan mode's reads. + * + * So only two things are asked of source, both failing closed: + * + * - Does it call a file-writing API? `INLINE_WRITE` is the same pattern the + * shell path already uses for `python3 -c`, which is what a REPL call is a + * longer-lived version of. + * - Does it shell out? A `subprocess.run`, `os.system` or `execSync` can run + * anything at all, and the argument is usually built at runtime, so there is + * nothing here to read. Unrecognised means destructive, as everywhere else. + */ +const CODE_SUBPROCESS = + /(\bsubprocess\b|\bos\.system\s*\(|\bos\.popen\s*\(|\bchild_process\b|\bexecSync\s*\(|\bspawnSync\s*\(|Bun\.\$)/; + +/** + * Source that checks something works. + * + * `assert` counts: in a REPL it is how a check is written, and a turn that + * verified its edit that way should not be reported as unverified. + */ +const CODE_VERIFIES = /(\bassert\b|\bunittest\b|\bpytest\b)/; + +/** + * Does this source hand work to another program? + * + * Asked separately from `classifyCode` because approval grades it differently + * from an ordinary write. `open(p, "w")` writes one named file and reads as a + * workspace write; `subprocess.run(argv)` builds its argument at runtime and + * can do anything at all, so it is graded destructive and asked about. + */ +export function codeShellsOut(code: string): boolean { + return CODE_SUBPROCESS.test(code); +} + +export function classifyCode(code: string): CommandEffect { + if (!code.trim()) return { writes: false, verifies: false }; + + return { + writes: INLINE_WRITE.test(code) || CODE_SUBPROCESS.test(code), + verifies: CODE_VERIFIES.test(code), + }; +} + +/** + * What a tool call does to the workspace, judged from whatever it carries. + * + * One entry point because there are two callers — plan mode's second gate and + * the turn's write/verify marks — and a REPL that only one of them understood + * would either be refused while planning or silently allowed through it. + */ +export function classifyInvocation(args: Record): CommandEffect { + const code = codeOf(args); + if (code) return classifyCode(code); + return classifyCommand(commandOf(args)); +} diff --git a/runtime/turnState.ts b/runtime/turnState.ts index f7c2d1a..fc05e63 100644 --- a/runtime/turnState.ts +++ b/runtime/turnState.ts @@ -8,7 +8,7 @@ * site that needed them. */ -import { classifyCommand, commandOf, toolEffect } from "./toolEffects"; +import { classifyInvocation, toolEffect } from "./toolEffects"; import type { TurnSummary } from "../config/types"; export class TurnState { @@ -91,11 +91,12 @@ export class TurnState { break; case "shell": { - // Judged from the command, not the tool name. A benchmark run showed + // Judged from the call, not the tool name. A benchmark run showed // the agent doing its real editing through run_terminal — `sed -i`, // `cat >> file` — which name-only classification recorded as - // verification, the opposite of what it is. - const { writes, verifies } = classifyCommand(commandOf(args)); + // verification, the opposite of what it is. A `repl` call is judged + // from its source for the same reason. + const { writes, verifies } = classifyInvocation(args); // Order matters when a command does both: `sed -i f.c && make` edited // and then checked, and the check has to land afterwards for the edit diff --git a/site/src/docs/surface.json b/site/src/docs/surface.json index 3e7402c..138c092 100644 --- a/site/src/docs/surface.json +++ b/site/src/docs/surface.json @@ -1,6 +1,6 @@ { "counts": { - "tools": 14, + "tools": 19, "commands": 15, "approvalModes": 4 }, @@ -290,6 +290,100 @@ "description": "The complete task list, in the order the steps will be done. Replaces the previous list." } ] + }, + { + "name": "repl", + "description": "Runs code in an interpreter that stays alive between calls, so variables, imports and loaded data persist.\n\nUse it instead of `run_terminal` with `python3 -c` or `node -e` whenever the work takes more than one step over the same data. Load a file, parse an archive or decode a video once, then keep querying what is already in memory — re-reading the same input on every call is the single most expensive habit available here.\n\nState lasts for the current turn and is discarded when the turn ends.\n\npython: the value of a trailing expression is printed, as in a notebook.\nnode: a top-level `var` persists between calls; `const` and `let` are scoped to the single call, so assign to `globalThis` for anything that must outlive it.\n\nThis runs real code. It can write files and shell out, and is subject to the same approval and plan-mode rules as run_terminal.", + "effect": "shell", + "gate": "Gated by the approval mode.", + "parameters": [ + { + "name": "language", + "type": "string", + "required": true, + "description": "Which interpreter to use." + }, + { + "name": "code", + "type": "string", + "required": true, + "description": "Source to evaluate in the session. May be several statements; it runs in the same namespace as previous calls." + }, + { + "name": "restart", + "type": "boolean", + "required": false, + "description": "Discard the existing session and start an empty one before running this code. Use after leaving the interpreter in a bad state, not routinely — restarting throws away the loaded data that makes this tool worth using." + }, + { + "name": "timeout", + "type": "number", + "required": false, + "description": "Seconds this evaluation may run (default: 120). Exceeding it discards the session and its variables." + } + ] + }, + { + "name": "process_start", + "description": "Starts a command in the background and returns immediately with an id.\n\nUse it for anything that does not exit on its own — a development server, a watcher, a long build you want to follow. For a command that finishes and whose output you need, use run_terminal instead; it waits and returns the result in one call.\n\nThe process keeps running after this turn ends. Read what it has printed with process_output and end it with process_stop.", + "effect": "shell", + "gate": "Gated by the approval mode.", + "parameters": [ + { + "name": "command", + "type": "string", + "required": true, + "description": "Command to start" + }, + { + "name": "cwd", + "type": "string", + "required": false, + "description": "Directory to run it in. Defaults to the project root." + } + ] + }, + { + "name": "process_output", + "description": "Returns what a background process has printed since the last time this was called for it, along with whether it is still running. Output already returned is not repeated.", + "effect": "shell", + "gate": "Gated by the approval mode.", + "parameters": [ + { + "name": "id", + "type": "string", + "required": true, + "description": "The id returned by process_start" + } + ] + }, + { + "name": "process_stop", + "description": "Stops a background process and returns any output it had not yet shown. Use it as soon as a process is no longer needed — one left running holds its port and its files.", + "effect": "shell", + "gate": "Gated by the approval mode.", + "parameters": [ + { + "name": "id", + "type": "string", + "required": true, + "description": "The id returned by process_start" + } + ] + }, + { + "name": "read_image", + "description": "Shows you an image. Use it to look at a screenshot, a diagram, a rendered figure or a video frame rather than inferring their contents from pixel statistics.\n\nThe image arrives with the next message, so describe what you are looking for and then read what you actually see. To inspect a frame of a video, extract it to a file first (ffmpeg, or cv2 in the repl) and read that file.\n\nAccepts PNG, JPEG, GIF and WebP.", + "effect": "read", + "gate": "Runs without asking. Reads only; never changes the workspace.", + "parameters": [ + { + "name": "path", + "type": "string", + "required": true, + "description": "Path to the image file" + } + ] } ], "commands": [ diff --git a/tools/approval.ts b/tools/approval.ts index d15f5e8..0a304af 100644 --- a/tools/approval.ts +++ b/tools/approval.ts @@ -1,5 +1,6 @@ import { getApprovalMode } from "../config/config"; import { CommandRisk, classifyCommand, createApprovalPolicy } from "../runtime/approval"; +import { classifyCode, codeShellsOut } from "../runtime/toolEffects"; import { store } from "../tui/src/store/ui-store"; export interface CommandApprovalResult { @@ -19,9 +20,53 @@ export interface CommandApprovalResult { */ export async function requestCommandApproval( command: string, - toolName: "run_terminal" | "run_tests", + toolName: ApprovedToolName, +): Promise { + return decide(command, toolName, classifyCommand(command)); +} + +/** The tools that clear something to run through this module. */ +export type ApprovedToolName = "run_terminal" | "run_tests" | "repl" | "process_start"; + +/** + * The same decision for interpreter source rather than a shell command. + * + * The shell classifier cannot be reused here: it reads `;` and `|` as command + * separators and `>` as a redirect, which in Python and JavaScript are a + * statement separator, bitwise-or and a comparison. Running it over source + * grades ordinary arithmetic as destructive, so the model would be asked to + * approve `value >> 16`. + * + * Three grades, failing closed at the one that cannot be read: + * + * - Source that shells out is DESTRUCTIVE. `subprocess.run(argv)` builds its + * command at runtime, so there is nothing here to inspect, and unrecognised + * means destructive everywhere else in this codebase. + * - Source that writes a named file is WORKSPACE_WRITE. + * - Anything else is READ_ONLY. A REPL is mostly arithmetic over data already + * read, and grading that as risky would train the user to approve blindly. + */ +export async function requestCodeApproval( + code: string, + language: string, +): Promise { + const risk = codeShellsOut(code) + ? CommandRisk.DESTRUCTIVE + : classifyCode(code).writes + ? CommandRisk.WORKSPACE_WRITE + : CommandRisk.READ_ONLY; + + // Shown to the user as what it is: source for an interpreter, not a command + // line. Without the prefix a multi-line Python block renders in the approval + // dialog as though it were about to be handed to a shell. + return decide(`${language}:\n${code}`, "repl", risk); +} + +async function decide( + command: string, + toolName: ApprovedToolName, + risk: CommandRisk, ): Promise { - const risk = classifyCommand(command); const policy = createApprovalPolicy(await getApprovalMode()); if (!policy.requiresApproval(risk)) { diff --git a/tools/command.ts b/tools/command.ts index 99961ba..86042a0 100644 --- a/tools/command.ts +++ b/tools/command.ts @@ -12,6 +12,38 @@ const EXIT_GRACE_MS = 750; /** How long SIGTERM is given to work before SIGKILL follows. */ const FORCE_KILL_AFTER_MS = 500; +/** + * The shell commands run under. + * + * `sh` is dash on Debian, which is what the benchmark containers are, and dash + * has no `for ((i=0;;))`, no `[[`, no arrays and no process substitution. A + * benchmark run failed six commands on that alone — + * `sh: 1: Syntax error: Bad for loop variable` — and the model has no way to + * learn from the message that the shell is the problem rather than its command. + * + * Resolved once: `Bun.which` stats the PATH, and this is on the path of every + * command the agent runs. Falls back to `sh`, which is the only shell POSIX + * guarantees, so a container without bash still runs commands rather than none. + */ +export const SHELL = Bun.which("bash") ?? "sh"; + +/** + * The argv that runs a command in its own process group where possible. + * + * Shared with `tools/process.ts` rather than copied, because getting it wrong + * is invisible until something is left running: a `bash -c "npm run dev"` that + * is killed without its group takes down the shell and leaves the server + * holding the port. macOS has no `setsid` and falls back to the descendant walk + * in `terminateProcessTree`, which is why both halves have to travel together. + */ +export function shellArgv(command: string): { cmd: string[]; processGroup: boolean } { + const setsid = Bun.which("setsid"); + return { + cmd: setsid ? [setsid, SHELL, "-c", command] : [SHELL, "-c", command], + processGroup: Boolean(setsid), + }; +} + function childPids(pid: number): number[] { // `Bun.spawnSync` throws outright when the executable is missing, and `pgrep` // is not installed everywhere — a minimal container is enough to lose it. This @@ -134,7 +166,7 @@ function signalEverything( * running. Killing is best-effort by nature; failing to kill must stay a * best-effort failure rather than becoming a hang. */ -function terminateProcessTree(proc: ReturnType, processGroup: boolean) { +export function terminateProcessTree(proc: ReturnType, processGroup: boolean) { if (proc.exitCode !== null) return; try { @@ -166,10 +198,9 @@ export async function runCommand( // On platforms with setsid, give the command a dedicated process group so // all children can be stopped together. macOS uses the tree-kill fallback. - const setsid = Bun.which("setsid"); - const processGroup = Boolean(setsid); + const { cmd, processGroup } = shellArgv(command); const proc = Bun.spawn({ - cmd: processGroup ? [setsid!, "sh", "-c", command] : ["sh", "-c", command], + cmd, stdout: "pipe", stderr: "pipe", }); diff --git a/tools/index.ts b/tools/index.ts index a5bb04f..e683769 100644 --- a/tools/index.ts +++ b/tools/index.ts @@ -13,6 +13,9 @@ import { webSearchTool } from "./webSearch"; import { webFetchTool } from "./webFetch"; import { questionTool } from "./question"; import { todoWriteTool } from "./todo"; +import { replTool } from "./repl"; +import { processStartTool, processOutputTool, processStopTool } from "./process"; +import { readImageTool } from "./readImage"; export const toolRegistry: Tool[] = [ listFilesTool, @@ -29,6 +32,11 @@ export const toolRegistry: Tool[] = [ webFetchTool, questionTool, todoWriteTool, + replTool, + processStartTool, + processOutputTool, + processStopTool, + readImageTool, ]; // Providers occasionally vary casing or use the singular form while selecting a diff --git a/tools/process.ts b/tools/process.ts new file mode 100644 index 0000000..d36c74a --- /dev/null +++ b/tools/process.ts @@ -0,0 +1,296 @@ +/** + * Commands that outlive the call that started them. + * + * `run_terminal` refuses an unquoted `&` outright and kills anything still + * running at its timeout, which is right for it — a tool that waits for output + * cannot wait forever. But it left the agent with no way to start a server, a + * watcher or a build it wants to keep an eye on, and the recorded benchmark + * trials show it hitting that wall: background launches refused, and the model + * told in the timeout message to give up and ask the user to test by hand. + * + * Three tools rather than a flag on `run_terminal`, because the shapes differ. + * A backgrounded command has no exit code to return and no output to wait for, + * so `process_start` answers with a handle, and reading and stopping are calls + * of their own against that handle. + * + * Unlike the REPL, these deliberately survive the turn. A development server + * started while answering one question has to still be up for the next; ending + * it with the turn would make the tool useless for the thing it exists for. + * They end at `process_stop`, at session exit, or when the process itself dies. + */ + +import type { Tool } from "../config/types"; +import { requestCommandApproval } from "./approval"; +import { shellArgv, terminateProcessTree } from "./command"; +import { resolveWorkspacePath } from "./workspace"; + +/** + * Output kept per process, in characters. + * + * A ring rather than an unbounded buffer: a watcher left running for an hour + * produces more than anything here could use, and the interesting part of a + * server's output is almost always the newest — a stack trace it just printed, + * not the banner from startup. When the cap is passed the oldest is dropped and + * the reader is told, so a gap is never silently presented as the whole story. + */ +const MAX_BUFFERED_OUTPUT = 64 * 1024; + +/** Characters of output returned by a single `process_output` call. */ +const MAX_OUTPUT_RESULT = 16 * 1024; + +interface BackgroundProcess { + id: string; + command: string; + proc: ReturnType; + /** Output not yet handed to `process_output`. */ + unread: string; + /** Characters dropped from the front of `unread` to stay under the cap. */ + dropped: number; + exitCode: number | null; + startedAt: number; + /** Whether the command got a process group, which decides how it is killed. */ + processGroup: boolean; +} + +const processes = new Map(); + +/** + * Short and readable: this id is quoted back by the model on every later call. + * + * Monotonic, and never reused even after the process it named is gone. Picking + * the lowest free number instead would hand `bg1` to a second process once the + * first was stopped, and a model still holding the old id — it has the whole + * transcript, so it always might — would read or kill the wrong one and get a + * plausible answer back. A counter that only goes up makes a stale id an error + * rather than a mix-up. + */ +let issued = 0; + +function nextId(): string { + issued++; + return `bg${issued}`; +} + +function collect(record: BackgroundProcess, stream: ReadableStream | null): void { + if (!stream) return; + + void (async () => { + const decoder = new TextDecoder(); + const reader = stream.getReader(); + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + record.unread += decoder.decode(value, { stream: true }); + if (record.unread.length > MAX_BUFFERED_OUTPUT) { + const excess = record.unread.length - MAX_BUFFERED_OUTPUT; + record.unread = record.unread.slice(excess); + record.dropped += excess; + } + } + } catch { + // The process was killed mid-read, which is a normal end for this. + } + })(); +} + +function describe(record: BackgroundProcess): string { + if (record.exitCode !== null) return `exited with code ${record.exitCode}`; + const seconds = Math.round((Date.now() - record.startedAt) / 1000); + return `running for ${seconds}s`; +} + +/** Drains what has been buffered, bounding what is handed back. */ +function drain(record: BackgroundProcess): string { + let output = record.unread; + record.unread = ""; + + const dropped = record.dropped; + record.dropped = 0; + + if (output.length > MAX_OUTPUT_RESULT) { + const excess = output.length - MAX_OUTPUT_RESULT; + // The newest is kept: the tail is what says what the process is doing now. + output = output.slice(excess); + return ( + `... ${excess + dropped} earlier characters dropped ...\n${output}` + ); + } + + return dropped > 0 ? `... ${dropped} earlier characters dropped ...\n${output}` : output; +} + +function requireProcess(args: Record): BackgroundProcess { + const id = args.id; + if (typeof id !== "string" || id.trim() === "") { + throw Error("id is required and must be the string returned by process_start"); + } + + const record = processes.get(id.trim()); + if (!record) { + const known = [...processes.keys()]; + throw Error( + `No background process ${id}. ` + + (known.length + ? `Started processes: ${known.join(", ")}.` + : "None have been started in this session."), + ); + } + + return record; +} + +export const processStartTool: Tool = { + name: "process_start", + description: `Starts a command in the background and returns immediately with an id. + +Use it for anything that does not exit on its own — a development server, a watcher, a long build you want to follow. For a command that finishes and whose output you need, use run_terminal instead; it waits and returns the result in one call. + +The process keeps running after this turn ends. Read what it has printed with process_output and end it with process_stop.`, + + parameters: [ + { name: "command", description: "Command to start", required: true }, + { + name: "cwd", + description: "Directory to run it in. Defaults to the project root.", + required: false, + }, + ], + + async execute(args) { + const command = args.command; + if (typeof command !== "string" || command.trim() === "") { + throw Error("command is required and must be a non-empty string"); + } + + let cwd: string | undefined; + if (args.cwd !== undefined) { + if (typeof args.cwd !== "string") { + throw Error("cwd must be a string path"); + } + cwd = await resolveWorkspacePath(args.cwd, { mustExist: true }); + } + + const { approved } = await requestCommandApproval(command, "process_start"); + if (!approved) { + return "Command rejected by user. Nothing was started."; + } + + const id = nextId(); + // Its own process group, so stopping it stops what it started. Without + // this the kill reaches the shell and nothing below it: measured, a + // `python3 ... ; true` left its interpreter running after process_stop + // returned, which is exactly the leak this tool claims to prevent. + const { cmd, processGroup } = shellArgv(command); + const proc = Bun.spawn({ + cmd, + cwd, + stdout: "pipe", + stderr: "pipe", + }); + + const record: BackgroundProcess = { + id, + command, + proc, + unread: "", + dropped: 0, + exitCode: null, + startedAt: Date.now(), + processGroup, + }; + processes.set(id, record); + + collect(record, proc.stdout as ReadableStream | null); + collect(record, proc.stderr as ReadableStream | null); + void proc.exited.then((code) => { + record.exitCode = code; + }); + + // Nothing is waited for, so there is nothing to report but the handle. The + // model is told what to call next, because a bare id reads as a dead end. + return ( + `Started ${id}: ${command}\n\n` + + `It is running in the background. Call process_output with id "${id}" to read ` + + `what it has printed, and process_stop to end it. It is not waited for, so ` + + `give it a moment before expecting output.` + ); + }, +}; + +export const processOutputTool: Tool = { + name: "process_output", + description: + "Returns what a background process has printed since the last time this was called for it, along with whether it is still running. Output already returned is not repeated.", + + parameters: [ + { name: "id", description: "The id returned by process_start", required: true }, + ], + + async execute(args) { + const record = requireProcess(args); + const output = drain(record); + const status = `${record.id} (${record.command}) — ${describe(record)}`; + + // An empty read is a real answer and has to say so: a server that started + // cleanly and is waiting for a request prints nothing, and a bare empty + // string reads as a broken tool. + if (output === "") { + return `${status}\n\nNo new output since the last read.`; + } + + return `${status}\n\n${output}`; + }, +}; + +export const processStopTool: Tool = { + name: "process_stop", + description: + "Stops a background process and returns any output it had not yet shown. Use it as soon as a process is no longer needed — one left running holds its port and its files.", + + parameters: [ + { name: "id", description: "The id returned by process_start", required: true }, + ], + + async execute(args) { + const record = requireProcess(args); + const alreadyExited = record.exitCode !== null; + + if (!alreadyExited) { + // The whole tree, not just the shell — see the note at the spawn. + terminateProcessTree(record.proc, record.processGroup); + // Bounded: a process that ignores the signal must not hold the turn. + await Promise.race([record.proc.exited, Bun.sleep(2000)]); + } + + processes.delete(record.id); + + const trailing = drain(record); + const outcome = alreadyExited + ? `${record.id} had already ${describe(record)}.` + : `Stopped ${record.id} (${record.command}).`; + + return trailing === "" ? outcome : `${outcome}\n\n${trailing}`; + }, +}; + +/** + * Ends every background process. + * + * For session exit, and for tests, which would otherwise leave a spawned + * process behind and hang the runner waiting on its handles. + */ +export function stopAllProcesses(): void { + for (const record of [...processes.values()]) { + processes.delete(record.id); + if (record.exitCode === null) { + terminateProcessTree(record.proc, record.processGroup); + record.proc.unref(); + } + } +} + +/** The ids alive right now. Exists for tests. */ +export function trackedProcessIds(): string[] { + return [...processes.keys()]; +} diff --git a/tools/readImage.ts b/tools/readImage.ts new file mode 100644 index 0000000..f2cc70d --- /dev/null +++ b/tools/readImage.ts @@ -0,0 +1,179 @@ +/** + * Lets the model look at an image instead of computing what is in it. + * + * The recorded benchmark trials show why. In the video-processing task the + * agent made 131 `cv2` and 128 `numpy` calls building histograms, frame + * differences and bounding boxes to infer what a frame contained — and scored 0 + * in three of four trials. Every model this drives can see; nothing was + * offering them the pixels. + * + * The tool returns text and queues the image, which the agent loop attaches to + * a following user message. That indirection is not decoration: of the three + * providers only Anthropic accepts an image inside a tool result, so returning + * one directly would work on one provider and quietly degrade on the other two. + * A user message carrying image parts is the shape all three accept. + */ + +import type { ImageAttachment, Tool } from "../config/types"; +import { resolveWorkspacePath } from "./workspace"; + +/** + * The largest file that is sent. + * + * Providers reject a request whose base64 payload is over roughly 5MB, and + * base64 is a third larger than the bytes it encodes. Refusing here with an + * instruction the model can act on is better than a 400 it cannot read. + */ +const MAX_IMAGE_BYTES = 3.5 * 1024 * 1024; + +/** Magic bytes, because an extension is a claim and a header is evidence. */ +const SIGNATURES: { mediaType: string; test: (bytes: Uint8Array) => boolean }[] = [ + { + mediaType: "image/png", + test: (b) => b[0] === 0x89 && b[1] === 0x50 && b[2] === 0x4e && b[3] === 0x47, + }, + { mediaType: "image/jpeg", test: (b) => b[0] === 0xff && b[1] === 0xd8 && b[2] === 0xff }, + { + mediaType: "image/gif", + test: (b) => b[0] === 0x47 && b[1] === 0x49 && b[2] === 0x46, + }, + { + mediaType: "image/webp", + test: (b) => + b[0] === 0x52 && b[1] === 0x49 && b[2] === 0x46 && b[3] === 0x46 && + b[8] === 0x57 && b[9] === 0x45 && b[10] === 0x42 && b[11] === 0x50, + }, +]; + +function detectMediaType(bytes: Uint8Array): string | null { + return SIGNATURES.find((signature) => signature.test(bytes))?.mediaType ?? null; +} + +/** + * Width and height, read from the header. + * + * Reported because it is what the model needs to ask for the right crop next, + * and because it is the one property worth stating that the image itself does + * not make obvious. Only PNG and JPEG are parsed; the others return null and + * the result simply omits the dimensions rather than guessing at them. + */ +function readDimensions(bytes: Uint8Array, mediaType: string): { width: number; height: number } | null { + const view = new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength); + + if (mediaType === "image/png" && bytes.length >= 24) { + // IHDR is always the first chunk, so width and height sit at a fixed offset. + return { width: view.getUint32(16), height: view.getUint32(20) }; + } + + if (mediaType === "image/jpeg") { + // Walk the segment chain to the start-of-frame marker, which is the only + // place the size is recorded. Skipping by declared length rather than + // scanning for the marker avoids matching one inside entropy-coded data. + let offset = 2; + while (offset + 9 < bytes.length) { + if (bytes[offset] !== 0xff) break; + const marker = bytes[offset + 1]!; + const length = view.getUint16(offset + 2); + + // SOF0..SOF15, excluding the four that are not frame headers. + if (marker >= 0xc0 && marker <= 0xcf && marker !== 0xc4 && marker !== 0xc8 && marker !== 0xcc) { + return { width: view.getUint16(offset + 7), height: view.getUint16(offset + 5) }; + } + + offset += 2 + length; + } + } + + return null; +} + +/** + * Images read this turn and not yet attached to a message. + * + * Drained by the agent loop immediately after the tool returns, so at most one + * call's worth is ever waiting. A queue rather than a return value because + * `Tool.execute` returns a string — a contract the whole registry and its + * sweep rest on, and not one worth reshaping for a single tool. + */ +let pending: ImageAttachment[] = []; + +export function takePendingImages(): ImageAttachment[] { + const taken = pending; + pending = []; + return taken; +} + +export const readImageTool: Tool = { + name: "read_image", + description: `Shows you an image. Use it to look at a screenshot, a diagram, a rendered figure or a video frame rather than inferring their contents from pixel statistics. + +The image arrives with the next message, so describe what you are looking for and then read what you actually see. To inspect a frame of a video, extract it to a file first (ffmpeg, or cv2 in the repl) and read that file. + +Accepts PNG, JPEG, GIF and WebP.`, + + parameters: [ + { + name: "path", + description: "Path to the image file", + required: true, + }, + ], + + async execute(args) { + const requestedPath = args.path; + + if (typeof requestedPath !== "string" || requestedPath.trim() === "") { + throw Error("File path is required"); + } + + let path: string; + try { + path = await resolveWorkspacePath(requestedPath, { mustExist: true }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + throw Error(`File ${requestedPath} does not exist`); + } + throw error; + } + + const file = Bun.file(path); + if (!(await file.exists())) { + throw Error(`File ${requestedPath} does not exist`); + } + + if (file.size === 0) { + throw Error(`File ${requestedPath} is empty, so there is no image to show.`); + } + + if (file.size > MAX_IMAGE_BYTES) { + throw Error( + `Image ${requestedPath} is ${Math.round(file.size / 1024)}KB, over the ` + + `${Math.round(MAX_IMAGE_BYTES / 1024)}KB a provider will accept. Resize or ` + + `crop it to a smaller file and read that.`, + ); + } + + // Only the header is needed to identify it; the bytes themselves are read + // again by whichever provider client renders the request. + const header = new Uint8Array(await file.slice(0, 64 * 1024).arrayBuffer()); + const mediaType = detectMediaType(header); + + if (!mediaType) { + throw Error( + `${requestedPath} is not a PNG, JPEG, GIF or WebP image. ` + + `Use read_file for text, or convert it to a supported format first.`, + ); + } + + pending.push({ path, mediaType }); + + const dimensions = readDimensions(header, mediaType); + const size = `${Math.round(file.size / 1024)}KB`; + const shape = dimensions ? `${dimensions.width}x${dimensions.height}, ` : ""; + + return ( + `${requestedPath} (${shape}${mediaType}, ${size}) is attached to the next ` + + `message. Describe what it shows before drawing conclusions from it.` + ); + }, +}; diff --git a/tools/repl.ts b/tools/repl.ts new file mode 100644 index 0000000..aeac245 --- /dev/null +++ b/tools/repl.ts @@ -0,0 +1,118 @@ +import type { Tool } from "../config/types"; +import { requestCodeApproval } from "./approval"; +import { + DEFAULT_EVAL_TIMEOUT_SECONDS, + MAX_REPL_OUTPUT, + ReplUnavailableError, + evaluate, + type ReplLanguage, +} from "./replSession"; + +const LANGUAGES: ReplLanguage[] = ["python", "node"]; + +function isLanguage(value: unknown): value is ReplLanguage { + return typeof value === "string" && LANGUAGES.includes(value as ReplLanguage); +} + +export const replTool: Tool = { + name: "repl", + description: `Runs code in an interpreter that stays alive between calls, so variables, imports and loaded data persist. + +Use it instead of \`run_terminal\` with \`python3 -c\` or \`node -e\` whenever the work takes more than one step over the same data. Load a file, parse an archive or decode a video once, then keep querying what is already in memory — re-reading the same input on every call is the single most expensive habit available here. + +State lasts for the current turn and is discarded when the turn ends. + +python: the value of a trailing expression is printed, as in a notebook. +node: a top-level \`var\` persists between calls; \`const\` and \`let\` are scoped to the single call, so assign to \`globalThis\` for anything that must outlive it. + +This runs real code. It can write files and shell out, and is subject to the same approval and plan-mode rules as run_terminal.`, + + parameters: [ + { + name: "language", + description: "Which interpreter to use.", + required: true, + enum: LANGUAGES, + }, + { + name: "code", + description: + "Source to evaluate in the session. May be several statements; it runs in the same namespace as previous calls.", + required: true, + }, + { + name: "restart", + description: + "Discard the existing session and start an empty one before running this code. Use after leaving the interpreter in a bad state, not routinely — restarting throws away the loaded data that makes this tool worth using.", + required: false, + type: "boolean", + }, + { + name: "timeout", + description: `Seconds this evaluation may run (default: ${DEFAULT_EVAL_TIMEOUT_SECONDS}). Exceeding it discards the session and its variables.`, + required: false, + type: "number", + }, + ], + + async execute(args, signal) { + const language = args.language; + const code = args.code; + + // Validated before anything is spawned: the contract sweep calls this with + // no arguments at all. + if (!isLanguage(language)) { + throw Error( + `language must be one of ${LANGUAGES.join(", ")}, got ${JSON.stringify(args.language)}`, + ); + } + + if (typeof code !== "string" || code.trim() === "") { + throw Error("code is required and must be a non-empty string"); + } + + const timeout = args.timeout; + if (timeout !== undefined && (typeof timeout !== "number" || !(timeout > 0))) { + throw Error(`timeout must be a positive number of seconds, got ${JSON.stringify(timeout)}`); + } + + const { approved } = await requestCodeApproval(code, language); + if (!approved) { + return "Code rejected by user. It was not run, and the session is unchanged."; + } + + let output: string; + let started: boolean; + try { + ({ output, started } = await evaluate(language, code, { + restart: args.restart === true, + timeoutSeconds: timeout, + signal, + })); + } catch (error) { + if (error instanceof ReplUnavailableError) throw error; + // A lost session is returned as a result rather than thrown so the model + // can rebuild its state and carry on; the message says what was lost. + const message = error instanceof Error ? error.message : String(error); + return `Error: ${message}`; + } + + // An evaluation that assigns a variable prints nothing, which is a success + // and has to read as one — an empty result looks like a tool that failed. + if (output === "") { + return started + ? `Started a ${language} session. The code ran and produced no output.` + : "The code ran and produced no output."; + } + + if (output.length > MAX_REPL_OUTPUT) { + return ( + `${output.slice(0, MAX_REPL_OUTPUT)}\n\n` + + `... Output truncated: showing the first ${MAX_REPL_OUTPUT} of ${output.length} characters. ` + + `The session still holds the full result — print a slice or a summary of it instead.` + ); + } + + return output; + }, +}; diff --git a/tools/replSession.ts b/tools/replSession.ts new file mode 100644 index 0000000..871dfea --- /dev/null +++ b/tools/replSession.ts @@ -0,0 +1,398 @@ +/** + * A live interpreter that outlasts a single tool call. + * + * The problem this exists for is measurable. Across the recorded benchmark + * trials the agent made 1,017 inline `python3 -c` / `node -e` calls, and every + * one of them started from nothing: in the video-processing trial 74 scripts + * opened the same MP4 63 times, re-decoding it each call because there was + * nowhere to keep the decoded frames. `gates.txt` was re-parsed 26 times, + * `input.tex` 35 times. That is paid twice — once in wall clock and iterations, + * and once in tokens, because every one of those script bodies stays in the + * conversation for the rest of the run. + * + * Kept apart from `repl.ts` the way `textEdit.ts` is kept apart from + * `editFile.ts`: "keep a subprocess alive and talk to it" is a question about + * processes, not about tools, and it is the part worth testing on its own. + * + * ## Why a driver and not `python3 -i` + * + * An interactive interpreter is built for a terminal, not a protocol. Output + * arrives interleaved with prompts (`>>> `, `... `), continuation state depends + * on blank lines, and there is no marker saying a statement finished — so a + * reader has to guess, and guesses wrongly on any code that prints something + * prompt-shaped. Instead each interpreter runs a small driver that speaks a + * framed protocol: one JSON-encoded string of source per line in, the captured + * output followed by a per-session sentinel out. Nothing has to be guessed. + * + * The sentinel is a UUID generated per session rather than a fixed string, so + * source that happens to print the delimiter cannot end a read early. + */ + +import { randomUUID } from "node:crypto"; + +export type ReplLanguage = "python" | "node"; + +/** Characters of output kept from a single evaluation. */ +export const MAX_REPL_OUTPUT = 16 * 1024; + +/** How long one evaluation may run before the session is considered lost. */ +export const DEFAULT_EVAL_TIMEOUT_SECONDS = 120; + +/** + * Python's side of the protocol. + * + * `exec` into one persistent globals dict is what makes state survive. The + * `ast` dance around the final statement is what makes the session usable as a + * REPL rather than as a script runner: `frames[0].shape` on its own line should + * print, and under a plain `exec` it evaluates and discards silently, which + * reads to the model as a tool that returned nothing. + * + * stdout and stderr are captured into one buffer so a traceback arrives in the + * same result as the output that preceded it, in the order they happened. + * `BaseException` rather than `Exception` so a `SystemExit` from library code + * is reported instead of killing the driver and taking the session with it. + */ +const PYTHON_DRIVER = String.raw` +import sys, json, io, ast, traceback + +_globals = {"__name__": "__main__"} +_sentinel = sys.argv[1] + +def _run(source): + block = ast.parse(source, "", "exec") + if not block.body: + return + last = block.body[-1] + if isinstance(last, ast.Expr): + head = ast.Module(body=block.body[:-1], type_ignores=[]) + exec(compile(head, "", "exec"), _globals) + value = eval(compile(ast.Expression(last.value), "", "eval"), _globals) + if value is not None: + print(repr(value)) + else: + exec(compile(block, "", "exec"), _globals) + +for _line in sys.stdin: + _line = _line.strip() + if not _line: + continue + _buffer = io.StringIO() + _out, _err = sys.stdout, sys.stderr + sys.stdout = sys.stderr = _buffer + try: + _run(json.loads(_line)) + except BaseException: + traceback.print_exc(file=_buffer) + finally: + sys.stdout, sys.stderr = _out, _err + _out.write(_buffer.getvalue()) + _out.write("\n" + _sentinel + "\n") + _out.flush() +`; + +/** + * Node's side of the same protocol. + * + * `runInThisContext` rather than a fresh context per call, because a fresh one + * is what loses the state this whole file exists to keep. It also decides the + * rule the tool description has to state: a top-level `var` becomes a property + * of the global object and survives, while `const` and `let` are scoped to the + * single script and do not. That is Node's semantics, not a choice made here, + * and pretending otherwise by rewriting declarations would break any source + * that shadows a name deliberately. + * + * `console` is redirected rather than the process's stdout, so that the + * sentinel frame is written by this driver alone and cannot be interleaved + * with evaluated output. + */ +const NODE_DRIVER = String.raw` +const vm = require("vm"); +const util = require("util"); +// argv[1], not [2]: with -e there is no script path, so the first trailing +// argument sits where a filename normally would. Both node and bun agree. +const sentinel = process.argv[1]; + +let buffer = ""; +const write = (...args) => { + buffer += args + .map((a) => (typeof a === "string" ? a : util.inspect(a, { depth: 4 }))) + .join(" ") + "\n"; +}; +console.log = write; +console.error = write; +console.warn = write; +console.info = write; + +let pending = ""; +process.stdin.setEncoding("utf8"); +process.stdin.on("data", async (chunk) => { + pending += chunk; + let newline; + while ((newline = pending.indexOf("\n")) >= 0) { + const line = pending.slice(0, newline); + pending = pending.slice(newline + 1); + if (!line.trim()) continue; + buffer = ""; + try { + let value = vm.runInThisContext(JSON.parse(line), { filename: "" }); + if (value && typeof value.then === "function") value = await value; + if (value !== undefined) write(util.inspect(value, { depth: 4 })); + } catch (error) { + buffer += (error && error.stack) || String(error); + buffer += "\n"; + } + process.stdout.write(buffer + "\n" + sentinel + "\n"); + } +}); +`; + +interface Driver { + /** Looked up on PATH; the first that resolves wins. */ + readonly candidates: readonly string[]; + readonly source: string; + /** The flag that makes the interpreter read the driver from an argument. */ + readonly flag: string; +} + +const DRIVERS: Record = { + // `-u` because the driver's framing is only useful if it is not sitting in a + // block-buffered pipe waiting for more. + python: { candidates: ["python3", "python"], source: PYTHON_DRIVER, flag: "-u" }, + node: { candidates: ["node", "bun"], source: NODE_DRIVER, flag: "-e" }, +}; + +export class ReplUnavailableError extends Error {} + +/** + * All three streams piped, stated rather than inferred. + * + * `ReturnType` is the shape for the *default* options, where + * stdin is ignored — so a session typed that way has a `stdin` of `number` and + * no `write` on it, which is the opposite of what this file needs. + */ +type PipedProcess = Bun.Subprocess<"pipe", "pipe", "pipe">; + +/** + * The stream is consumed through its async iterator rather than a reader. + * + * Bun's `ReadableStreamDefaultReader.read` takes a buffer to fill, so the + * zero-argument DOM form does not type-check against it. The iterator hands + * back the chunk instead, which is all this needs, and it is still one held + * cursor across many evaluations — the property that matters, since a reader + * acquired per call would drop whatever had already been buffered. + */ +type StreamCursor = AsyncIterator; + +interface Session { + proc: PipedProcess; + cursor: StreamCursor; + sentinel: string; + /** Output read past the last sentinel, belonging to no evaluation yet. */ + pending: string; + /** Set when a timeout or a crash makes further evaluation meaningless. */ + broken: boolean; +} + +const sessions = new Map(); + +function spawnSession(language: ReplLanguage): Session { + const driver = DRIVERS[language]; + const interpreter = driver.candidates + .map((candidate) => Bun.which(candidate)) + .find((resolved): resolved is string => resolved !== null); + + if (!interpreter) { + throw new ReplUnavailableError( + `No ${language} interpreter is available on this machine ` + + `(looked for ${driver.candidates.join(", ")}). Use run_terminal instead.`, + ); + } + + const sentinel = `__woopcode_repl_${randomUUID()}__`; + const args = + language === "python" + ? [driver.flag, "-c", driver.source, sentinel] + : [driver.flag, driver.source, sentinel]; + + const proc: PipedProcess = Bun.spawn({ + cmd: [interpreter, ...args], + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + + return { + proc, + cursor: proc.stdout[Symbol.asyncIterator]() as StreamCursor, + sentinel, + pending: "", + broken: false, + }; +} + +/** + * Reads until the session's sentinel arrives. + * + * Three ways this ends badly, and all three have to leave the session dead + * rather than merely return an error: a timeout means the driver is still + * evaluating and will write its output into the *next* read, a closed stream + * means the interpreter is gone, and a cancellation means the user is no longer + * waiting. A session left alive after any of them answers the following call + * with the previous call's output. + */ +async function readFrame( + session: Session, + timeoutSeconds: number, + signal?: AbortSignal, +): Promise { + const decoder = new TextDecoder(); + const deadline = Date.now() + timeoutSeconds * 1000; + + // One timer for the whole read, not one per chunk. Created inside the loop it + // was a fresh `Bun.sleep` on every chunk received, none of them cancelled + // when the race was won by the stream — so reading a large result left one + // live timer per chunk, each pending for the rest of the timeout. A single + // promise raced repeatedly settles once and costs one timer. + let expired = false; + const timeout = Bun.sleep(timeoutSeconds * 1000).then(() => { + expired = true; + return "timeout" as const; + }); + + while (true) { + const marker = session.pending.indexOf(session.sentinel); + if (marker !== -1) { + const frame = session.pending.slice(0, marker); + session.pending = session.pending.slice(marker + session.sentinel.length); + // Newlines only, at both ends. The driver writes a newline before the + // sentinel and another after it, so an untrimmed frame carries the + // previous call's trailing byte at its front. Trimming whitespace + // generally would eat the indentation of output that begins with it. + return frame.replace(/^\n+/, "").replace(/\n+$/, ""); + } + + if (signal?.aborted) { + session.broken = true; + throw new Error("Evaluation cancelled"); + } + + if (expired || Date.now() >= deadline) { + session.broken = true; + throw new Error( + `Evaluation timed out after ${timeoutSeconds} seconds. The session was ` + + `discarded, so its variables are gone; the next call starts a fresh one.`, + ); + } + + // Raced rather than awaited outright: `cursor.next()` on a process that is + // busy evaluating never settles, so without this the timeout above is + // unreachable and a runaway loop hangs the turn instead of ending it. + const chunk = await Promise.race([session.cursor.next(), timeout]); + + if (chunk === "timeout") continue; + if (chunk.done) { + session.broken = true; + throw new Error( + "The interpreter exited. Its state is gone; the next call starts a fresh one.", + ); + } + + session.pending += decoder.decode(chunk.value as Uint8Array, { stream: true }); + } +} + +/** + * Ends one session. + * + * stdin is closed before the kill, and that ordering is the whole of it. Both + * drivers loop until their input ends, so closing stdin is what lets them + * return normally; `kill` alone left the pipe open, and Bun kept the process + * handle alive waiting on a writer that never went away — a probe that had + * already printed every result sat for two minutes before exiting. The kill + * stays as the backstop for a driver wedged inside an evaluation, which will + * never reach its read of stdin to notice the close. + */ +function discard(language: ReplLanguage): void { + const session = sessions.get(language); + if (!session) return; + sessions.delete(language); + + session.cursor.return?.(undefined)?.catch(() => { + // The process is being killed regardless; a cursor that will not release + // is not a reason to leave the interpreter running. + }); + + try { + session.proc.stdin.end(); + } catch { + // Already closed, or the process is gone. The kill below covers both. + } + + session.proc.kill(); + session.proc.unref(); +} + +export interface EvalOptions { + restart?: boolean; + timeoutSeconds?: number; + signal?: AbortSignal; +} + +export interface EvalResult { + output: string; + /** True when this call started the interpreter rather than reusing it. */ + started: boolean; +} + +export async function evaluate( + language: ReplLanguage, + code: string, + options: EvalOptions = {}, +): Promise { + const { restart = false, timeoutSeconds = DEFAULT_EVAL_TIMEOUT_SECONDS, signal } = options; + + if (restart) discard(language); + + const existing = sessions.get(language); + // A broken session is replaced rather than reported: the model asked for an + // evaluation, and the fact that the previous one timed out has already been + // reported to it as that call's error. + if (existing?.broken) discard(language); + + let session = sessions.get(language); + const started = session === undefined; + if (!session) { + session = spawnSession(language); + sessions.set(language, session); + } + + // One line, so the driver's line-oriented read frames it. JSON.stringify is + // what makes that safe for source containing newlines, quotes or backslashes. + session.proc.stdin.write(`${JSON.stringify(code)}\n`); + session.proc.stdin.flush(); + + try { + const output = await readFrame(session, timeoutSeconds, signal); + return { output, started }; + } catch (error) { + discard(language); + throw error; + } +} + +/** + * Ends every session. + * + * Called from the agent loop's `finally`, which is the only place that runs on + * all of a turn's exits. Per-turn scope is deliberate: an interpreter that + * outlived its turn would answer the next one with variables nobody in that + * conversation set, and the model has no way to see that history exists. + */ +export function closeReplSessions(): void { + for (const language of [...sessions.keys()]) discard(language); +} + +/** The languages with a session alive right now. Exists for tests. */ +export function openReplLanguages(): ReplLanguage[] { + return [...sessions.keys()]; +} diff --git a/tools/terminal.ts b/tools/terminal.ts index 6c59599..3da982f 100644 --- a/tools/terminal.ts +++ b/tools/terminal.ts @@ -77,7 +77,12 @@ export const terminalTool: Tool = { } if (startsBackgroundProcess(command)) { - return "Error: Background processes (&) are not supported. Use run_terminal for quick commands only (tests, builds, installs), not for starting servers."; + return ( + "Error: this tool waits for the command to finish, so a trailing & has " + + "nothing to return. Use process_start to run it in the background — it " + + "returns an id you can read with process_output and end with process_stop. " + + "Keep run_terminal for commands that exit on their own." + ); } try { @@ -87,7 +92,12 @@ export const terminalTool: Tool = { return "Command cancelled before completion."; } if (error instanceof Error && error.message.includes("timed out")) { - return `Error: ${error.message}\n\nNote: For long-running processes like servers, the agent cannot verify them. Just create/edit the code and inform the user to test manually.`; + return ( + `Error: ${error.message}\n\nIf this command was never going to exit on ` + + `its own — a server, a watcher — start it with process_start instead and ` + + `read it with process_output. If it was simply slow, run it again with a ` + + `larger timeout.` + ); } throw error; } diff --git a/tui/src/types.ts b/tui/src/types.ts index 76e238f..49c91c2 100644 --- a/tui/src/types.ts +++ b/tui/src/types.ts @@ -80,7 +80,7 @@ export interface PendingEdit { export interface PendingCommand { id: string; command: string; - toolName: "run_terminal" | "run_tests"; + toolName: "run_terminal" | "run_tests" | "repl" | "process_start"; /** Why it needs approval, from the classifier. */ risk?: CommandRisk; }