diff --git a/src/client/app.js b/src/client/app.js index e5ff503..f249243 100644 --- a/src/client/app.js +++ b/src/client/app.js @@ -1248,6 +1248,14 @@ import { mountSidebar } from "./sidebar.js"; return sink; } // Pretty value for a tool's args/result (JSON, or a string as-is). + // A sandbox tool that promoted files returns { output, artifacts } instead of + // the bare text; the documents render from the event, so only the text belongs + // in the step's body. + function unwrapDelivery(output) { + return output && typeof output === "object" && typeof output.output === "string" + ? output.output + : output; + } function toolValue(v) { if (v == null) return ""; if (typeof v === "string") return v; @@ -2370,7 +2378,7 @@ import { mountSidebar } from "./sidebar.js"; function defaultResult(t, output) { var out = document.createElement("div"); out.className = "tout"; - out.textContent = toolValue(output); + out.textContent = toolValue(unwrapDelivery(output)); t.body.appendChild(out); } // A shell run as a terminal card: the command on a `$` prompt line, its output @@ -2381,11 +2389,7 @@ import { mountSidebar } from "./sidebar.js"; return i < 0 ? s : s.slice(0, i) + " …"; } function renderShellResult(t, output) { - // A command that promoted files returns { output, artifacts } instead of the - // bare transcript; the documents render from the event, so only the text - // belongs in the terminal card. - if (output && typeof output === "object" && typeof output.output === "string") - output = output.output; + output = unwrapDelivery(output); var term = document.createElement("div"); term.className = "term"; if (t.input && t.input.command) { diff --git a/src/tools.ts b/src/tools.ts index f42e338..32444cf 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -425,10 +425,7 @@ function runShell(executor: Executor, ctx: ToolContext) { const result = formatExecResult( await executor.run({ command, session, timeoutMs }, abortSignal), ); - const artifacts = session ? await promoteOutputs(executor, ctx, session, abortSignal) : []; - // Shape only shifts when there IS something to carry, so the ordinary - // command keeps returning the plain transcript it always did. - return artifacts.length ? { output: result, artifacts } : result; + return deliver(executor, ctx, result, abortSignal); }, }); } @@ -521,7 +518,8 @@ export function viewFile(executor: Executor, session: string) { }); } -export function writeFile(executor: Executor, session: string) { +export function writeFile(executor: Executor, ctx: ToolContext) { + const session = ctx.conversationId as string; return tool({ description: "Write a text file in the sandbox, creating parent directories and replacing anything " + @@ -546,14 +544,17 @@ export function writeFile(executor: Executor, session: string) { if (before.kind === "directory") return `${full} is a directory, not a file.`; await executor.putFile(session, full, new TextEncoder().encode(content), abortSignal); const lines = content === "" ? 0 : content.split("\n").length; - return before.kind === "file" - ? `Overwrote ${full} (${lines} lines, ${content.length} bytes).` - : `Created ${full} (${lines} lines, ${content.length} bytes).`; + const said = + before.kind === "file" + ? `Overwrote ${full} (${lines} lines, ${content.length} bytes).` + : `Created ${full} (${lines} lines, ${content.length} bytes).`; + return deliver(executor, ctx, said, abortSignal); }, }); } -export function editFile(executor: Executor, session: string) { +export function editFile(executor: Executor, ctx: ToolContext) { + const session = ctx.conversationId as string; return tool({ description: "Change part of a text file in the sandbox by exact find-and-replace. old_string must " + @@ -597,7 +598,8 @@ export function editFile(executor: Executor, session: string) { const delta = out.text.split("\n").length - read.text.split("\n").length; const shape = delta === 0 ? "same line count" : delta > 0 ? `+${delta} lines` : `${delta} lines`; - return `Edited ${full} — ${out.replaced} replacement${out.replaced === 1 ? "" : "s"}, ${shape}.`; + const said = `Edited ${full} — ${out.replaced} replacement${out.replaced === 1 ? "" : "s"}, ${shape}.`; + return deliver(executor, ctx, said, abortSignal); }, }); } @@ -606,7 +608,28 @@ export function editFile(executor: Executor, session: string) { const DEFAULT_VIEW_LINES = 200; /** - * Drain /workspace/outputs into blobs after a command. + * A sandbox tool's reply, plus whatever it left in the outbox. + * + * Promotion follows the outbox, not the tool that filled it: a report written + * with write_file is as finished as one a shell command copied there, and the + * model shouldn't have to run a stray `ls` to make it arrive. The shape only + * shifts when there IS something to carry, so an ordinary call keeps returning + * the plain string it always did. + */ +async function deliver( + executor: Executor, + ctx: ToolContext, + output: string, + signal?: AbortSignal, +): Promise { + const session = ctx.conversationId; + if (!session) return output; + const artifacts = await promoteOutputs(executor, ctx, session, signal); + return artifacts.length ? { output, artifacts } : output; +} + +/** + * Drain /workspace/outputs into blobs. * * The spec's promotion rule, in one place: if it wasn't promoted, it was * scratch. Auto-harvesting means a model that writes a chart to the outbox gets @@ -1030,7 +1053,7 @@ const REGISTRY: Array<{ executor: "sandbox", create: (ctx) => { const e = getExecutor(); - return e && ctx.conversationId ? writeFile(e, ctx.conversationId) : null; + return e && ctx.conversationId ? writeFile(e, ctx) : null; }, }, { @@ -1038,7 +1061,7 @@ const REGISTRY: Array<{ executor: "sandbox", create: (ctx) => { const e = getExecutor(); - return e && ctx.conversationId ? editFile(e, ctx.conversationId) : null; + return e && ctx.conversationId ? editFile(e, ctx) : null; }, }, ]; diff --git a/tests/filetools.test.ts b/tests/filetools.test.ts index 28765d4..196fdf3 100644 --- a/tests/filetools.test.ts +++ b/tests/filetools.test.ts @@ -1,4 +1,9 @@ import { expect, test } from "bun:test"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { FsBlobStore } from "../src/blobs"; +import type { ArtifactRef } from "../src/events"; import type { ExecResult, Executor, HarvestedFile, ReadResult } from "../src/executor"; import { editFile, viewFile, writeFile } from "../src/tools"; @@ -40,8 +45,18 @@ class FakeSandbox implements Executor { if (text === undefined) return { kind: "missing" }; return { kind: "file", text, bytes: text.length }; } + /** Drains the outbox, like the real one: what's taken is gone. */ async harvest(): Promise { - return []; + const out: HarvestedFile[] = []; + for (const [path, text] of this.files) { + if (!path.startsWith("/workspace/outputs/")) continue; + out.push({ + path: path.slice("/workspace/outputs/".length), + bytes: new TextEncoder().encode(text), + }); + this.files.delete(path); + } + return out; } disposeSession(): void {} } @@ -94,7 +109,7 @@ test("each way a read fails names a different fix", async () => { test("writing reports whether it created or replaced, and writes it verbatim", async () => { const box = new FakeSandbox(); - const write = writeFile(box, "c1"); + const write = writeFile(box, { conversationId: "c1" }); // Content that would need escaping through a shell, passed through untouched. const tricky = "line 'one'\n$(echo hi) `date`\n\"two\"\n"; @@ -111,7 +126,7 @@ test("writing reports whether it created or replaced, and writes it verbatim", a test("an edit writes back exactly the replacement it computed", async () => { const box = new FakeSandbox(); box.files.set("/workspace/app.py", "def main():\n return 1\n"); - const edit = editFile(box, "c1"); + const edit = editFile(box, { conversationId: "c1" }); const out = (await call(edit, { path: "app.py", @@ -127,7 +142,7 @@ test("a failed edit changes nothing and explains itself", async () => { const box = new FakeSandbox(); const before = "a = 1\nb = 2\na = 1\n"; box.files.set("/workspace/c.py", before); - const edit = editFile(box, "c1"); + const edit = editFile(box, { conversationId: "c1" }); const ambiguous = (await call(edit, { path: "c.py", @@ -158,7 +173,7 @@ test("a failed edit changes nothing and explains itself", async () => { test("editing a file that isn't there says so instead of creating it", async () => { const box = new FakeSandbox(); - const out = (await call(editFile(box, "c1"), { + const out = (await call(editFile(box, { conversationId: "c1" }), { path: "ghost.py", old_string: "a", new_string: "b", @@ -166,3 +181,32 @@ test("editing a file that isn't there says so instead of creating it", async () expect(out).toContain("No file at /workspace/ghost.py"); expect(box.files.size).toBe(0); }); + +test("a file written into the outbox is promoted, whichever tool put it there", async () => { + // The model is told to prefer write_file over a heredoc, so the outbox has to + // drain after a write too — a report that arrives only if a later `ls` happens + // to run is a report that usually doesn't arrive. + const dir = mkdtempSync(join(tmpdir(), "kloe-ft-")); + const box = new FakeSandbox(); + const ctx = { conversationId: "c1", blobs: new FsBlobStore(join(dir, "blobs")) }; + + const wrote = (await call(writeFile(box, ctx), { + path: "outputs/report.md", + content: "# Findings\n", + })) as { output: string; artifacts: ArtifactRef[] }; + expect(wrote.output).toContain("Created"); + expect(wrote.artifacts).toMatchObject([{ name: "report.md", mime: "text/markdown" }]); + + // A file kept in the workspace is scratch: writing it promotes nothing. + expect(await call(writeFile(box, ctx), { path: "notes.md", content: "x" })).toBeString(); + + // An edit delivers too — the finished version is the one worth handing over. + box.files.set("/workspace/outputs/chart.html", "

old

"); + const edited = (await call(editFile(box, ctx), { + path: "outputs/chart.html", + old_string: "old", + new_string: "new", + })) as { output: string; artifacts: ArtifactRef[] }; + expect(edited.artifacts).toMatchObject([{ name: "chart.html", mime: "text/html" }]); + expect(await (await ctx.blobs.get(edited.artifacts[0]!.sha256))?.text()).toBe("

new

"); +});