From f013cafcd18014c2a2fa38409af2337763a3112a Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Tue, 11 Aug 2026 17:00:41 -0400 Subject: [PATCH] feat: view, write and edit files in the sandbox --- src/client/app.js | 46 ++++++++ src/edits.ts | 202 +++++++++++++++++++++++++++++++++++ src/executor.ts | 59 +++++++++++ src/tools.ts | 226 +++++++++++++++++++++++++++++++++++++++- tests/edits.test.ts | 85 +++++++++++++++ tests/executor.test.ts | 34 ++++++ tests/filetools.test.ts | 168 +++++++++++++++++++++++++++++ tests/tools.test.ts | 13 +++ 8 files changed, 830 insertions(+), 3 deletions(-) create mode 100644 src/edits.ts create mode 100644 tests/edits.test.ts create mode 100644 tests/filetools.test.ts diff --git a/src/client/app.js b/src/client/app.js index 0418585..3c14487 100644 --- a/src/client/app.js +++ b/src/client/app.js @@ -34,6 +34,7 @@ import { EXT_ICON as ICON_EXT, GLOBE_ICON as ICON_GLOBE, PAGE_ICON as ICON_PAGE, + PENCIL_ICON as ICON_PENCIL, RESEARCH_ICON as ICON_RESEARCH, TERMINAL_ICON as ICON_TERMINAL, TOOL_ICON as ICON_TOOL, @@ -2227,7 +2228,52 @@ import { mountSidebar } from "./sidebar.js"; }, result: renderShellResult, }, + // The file tools name the file they touched, because that is the whole + // story of the step: which file, and what happened to it. The result body + // (numbered lines, or a one-line report) renders with the default. + view_file: { + icon: ICON_PAGE, + row: function (input) { + return (input && input.path) || "view_file"; + }, + summary: function (steps, active) { + var verb = active ? "Reading" : "Read"; + if (steps.length > 1) return verb + " " + steps.length + " files"; + return verb + " " + (baseName(lastInput(steps).path) || "a file"); + }, + result: defaultResult, + }, + write_file: { + icon: ICON_PAGE, + row: function (input) { + return (input && input.path) || "write_file"; + }, + summary: function (steps, active) { + var verb = active ? "Writing" : "Wrote"; + if (steps.length > 1) return verb + " " + steps.length + " files"; + return verb + " " + (baseName(lastInput(steps).path) || "a file"); + }, + result: defaultResult, + }, + edit_file: { + icon: ICON_PENCIL, + row: function (input) { + return (input && input.path) || "edit_file"; + }, + summary: function (steps, active) { + var verb = active ? "Editing" : "Edited"; + if (steps.length > 1) return verb + " " + steps.length + " files"; + return verb + " " + (baseName(lastInput(steps).path) || "a file"); + }, + result: defaultResult, + }, }; + /** The last path segment — a step reads better as "main.py" than as its path. */ + function baseName(p) { + if (!p) return ""; + var parts = String(p).split("/"); + return parts[parts.length - 1] || p; + } var DEFAULT_TOOL = { icon: ICON_TOOL, row: function (_input, name) { diff --git a/src/edits.ts b/src/edits.ts new file mode 100644 index 0000000..25a6e6c --- /dev/null +++ b/src/edits.ts @@ -0,0 +1,202 @@ +/** + * The text half of the file tools: numbering, slicing, and exact replacement. + * + * Kept apart from the sandbox on purpose. Reading and writing bytes is the + * executor's problem; deciding what a view shows and whether an edit is + * unambiguous is string work, and string work should be testable without a + * container running. + * + * The design follows Anthropic's text-editor tool (view with a line range, + * str_replace requiring a unique match) and Crush's `edit`, with one deliberate + * divergence: when `old_string` doesn't match, Crush will fall back to + * whitespace-normalized matching and silently re-indent the replacement. That + * rescues a call at the cost of the model never learning what the file actually + * contains. Here the same analysis runs, but its output is a DIAGNOSTIC — the + * real lines, with whitespace made visible — and the edit is refused. The model + * gets the byte-accurate text it was missing and can retry exactly. + */ + +/** A line's worth of margin, wide enough for a six-figure file. */ +function gutter(n: number): string { + const s = String(n); + return s.length >= 6 ? s : " ".repeat(6 - s.length) + s; +} + +/** Cap on one line's length, so a minified bundle can't fill the window. */ +export const MAX_LINE = 2000; + +export interface ViewSlice { + /** The requested lines, numbered, ready to hand back. */ + body: string; + /** 1-based line number of the first line shown. */ + from: number; + /** How many lines this slice holds. */ + shown: number; + /** How many lines the file has in total. */ + total: number; +} + +/** + * A window onto a file's lines, numbered from its real position. + * + * Numbers are not decoration: they are what makes a follow-up `offset` mean + * something, and what lets a person reading the transcript check the model's + * claim about line 40 against line 40. + */ +export function viewSlice(text: string, offset = 0, limit = 200): ViewSlice { + const lines = text.split("\n"); + // A trailing newline yields a final empty element that is not a line. + if (lines.length > 1 && lines[lines.length - 1] === "") lines.pop(); + const from = Math.max(0, offset); + const window = lines.slice(from, from + Math.max(1, limit)); + const body = window + .map((line, i) => { + const shown = line.length > MAX_LINE ? `${line.slice(0, MAX_LINE)}…[line truncated]` : line; + return `${gutter(from + i + 1)}|${shown}`; + }) + .join("\n"); + return { body, from: from + 1, shown: window.length, total: lines.length }; +} + +export type ReplaceOutcome = + | { ok: true; text: string; replaced: number } + | { ok: false; reason: string }; + +/** + * Exact find-and-replace, refusing anything ambiguous. + * + * A unique match is the whole safety property: "replace this text" is only a + * well-defined instruction when the text appears once. When it appears several + * times the model has to say which — with more context, or by asking for all of + * them — because guessing produces an edit nobody can review. + */ +export function replaceOnce( + content: string, + oldString: string, + newString: string, + replaceAll = false, +): ReplaceOutcome { + if (oldString === "") return { ok: false, reason: "old_string must not be empty." }; + if (oldString === newString) + return { ok: false, reason: "old_string and new_string are identical; nothing to do." }; + const first = content.indexOf(oldString); + if (first === -1) { + const hint = mismatchHint(content, oldString); + return { + ok: false, + reason: + "old_string was not found in the file. It must match exactly, including whitespace and " + + "line breaks." + + (hint ? `\n\n${hint}` : ""), + }; + } + const count = content.split(oldString).length - 1; + if (count > 1 && !replaceAll) { + return { + ok: false, + reason: + `old_string appears ${count} times, so which one to change is ambiguous. Include more ` + + "surrounding lines to make it unique, or pass replace_all to change every occurrence.", + }; + } + const text = replaceAll + ? content.split(oldString).join(newString) + : content.slice(0, first) + newString + content.slice(first + oldString.length); + return { ok: true, text, replaced: replaceAll ? count : 1 }; +} + +/** Whitespace runs collapsed, so two lines can be compared for their words. */ +function normalize(line: string): string { + return line.trim().split(/\s+/).join(" "); +} + +/** Tabs and spaces made visible, for a hint about text that looks identical. */ +function visualize(line: string): string { + return line.replace(/\t/g, "→").replace(/ /g, "·"); +} + +/** + * Why an exact match failed, when the answer is knowable. + * + * Two cases are worth the model's time. The text is there but its whitespace + * differs — invisible in a transcript, and the single most common way an edit + * misses. Or one line is very close, in which case showing the neighbourhood is + * more useful than repeating that nothing matched. + */ +export function mismatchHint(content: string, oldString: string): string | null { + const lines = content.split("\n"); + const want = oldString.split("\n"); + const wantNorm = want.map(normalize).join("\n"); + if (wantNorm.trim() === "") return null; + + // Whitespace-only difference: report the real lines, whitespace visible. + const norm = lines.map(normalize); + for (let i = 0; i + want.length <= lines.length; i++) { + if (norm.slice(i, i + want.length).join("\n") !== wantNorm) continue; + const actual = lines + .slice(i, i + want.length) + .map((l, k) => `${gutter(i + k + 1)}|${visualize(l)}`) + .join("\n"); + return ( + "The text is in the file, but its whitespace differs (tabs vs spaces, or a different " + + `indent). Lines ${i + 1}-${i + want.length} actually read:\n${actual}\n` + + "→ is a tab and · is a space. Copy exactly that." + ); + } + + // Otherwise, the closest single line — enough to locate the drift. + const target = normalize(want[0] ?? ""); + if (target === "") return null; + let bestAt = -1; + let bestScore = 0; + for (let i = 0; i < lines.length; i++) { + const score = similarity(norm[i] ?? "", target); + if (score > bestScore) { + bestScore = score; + bestAt = i; + } + } + // Half the trigrams in common. Two versions of one line with an identifier + // renamed land around 0.55-0.7, while genuinely unrelated lines sit well + // below — and a hint is advisory, so the cost of a loose threshold is a few + // wasted lines rather than a wrong edit. + if (bestAt < 0 || bestScore < 0.5) return null; + const from = Math.max(0, bestAt - 2); + const near = lines + .slice(from, bestAt + 3) + .map((l, k) => `${gutter(from + k + 1)}|${l}`) + .join("\n"); + return `The closest text in the file is around line ${bestAt + 1}:\n${near}`; +} + +/** + * Trigram overlap — enough to rank candidate lines, not a diff algorithm. + * + * Character trigrams rather than words, because the lines this has to tell + * apart are code: `def greet(name):` and `def greet(person):` share only one + * whitespace-delimited token out of two, which reads as unrelated, while their + * character runs are obviously the same line with one word changed. + */ +function similarity(a: string, b: string): number { + if (!a || !b) return 0; + if (a === b) return 1; + const grams = (s: string): Map => { + const m = new Map(); + const padded = ` ${s} `; + for (let i = 0; i + 3 <= padded.length; i++) { + const g = padded.slice(i, i + 3); + m.set(g, (m.get(g) ?? 0) + 1); + } + return m; + }; + const ga = grams(a); + const gb = grams(b); + let shared = 0; + let total = 0; + for (const [, n] of ga) total += n; + for (const [g, n] of gb) { + total += n; + shared += Math.min(n, ga.get(g) ?? 0); + } + return total === 0 ? 0 : (2 * shared) / total; +} diff --git a/src/executor.ts b/src/executor.ts index 44823fd..f32234d 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -57,6 +57,17 @@ export interface ExecResult { timedOut: boolean; } +/** + * The outcome of reading a path in the sandbox: what it turned out to be, and + * its text when it was a readable file. + */ +export type ReadResult = + | { kind: "file"; text: string; bytes: number } + | { kind: "missing" } + | { kind: "directory" } + | { kind: "too-large"; bytes: number } + | { kind: "binary" }; + /** A file drained out of the sandbox's outbox. */ export interface HarvestedFile { /** Path relative to the outbox root, e.g. "chart.png" or "data/out.csv". */ @@ -76,6 +87,14 @@ export interface Executor { * model can actually operate on. */ putFile(session: string, path: string, bytes: Uint8Array, signal?: AbortSignal): Promise; + /** + * Read one file back out, with enough of a verdict to explain a failure. + * + * The kind comes back with the bytes because "no such file" and "that's a + * directory" are different mistakes, and a tool that answers both with an + * empty string teaches the model nothing. + */ + readFile(session: string, path: string, signal?: AbortSignal): Promise; /** * Drain the outbox: every file under `dir`, returned and then removed. * @@ -99,6 +118,13 @@ function shq(s: string): string { return `'${s.replace(/'/g, "'\\''")}'`; } +/** + * Ceiling on a single file read, so a tool call can't pull a gigabyte of log + * through the socket and into a prompt. Generous next to any source file, and + * `run_shell` is still there for the cases that genuinely need the whole thing. + */ +const MAX_READ_BYTES = 256 * 1024; + /** What a killed command reports back. coreutils `timeout` uses 124 already. */ const TIMEOUT_EXIT = 124; @@ -462,6 +488,39 @@ export class LocalDockerExecutor implements Executor { if (code !== 0) throw new Error(`could not write ${path}: ${stderr.trim() || `exit ${code}`}`); } + /** + * Read a file out of the container, classifying it on the way. + * + * One `docker exec` rather than a stat followed by a cat: the shell decides + * what the path is and prints a verdict line, then the bytes. Two round trips + * would also be two moments — a file can be created between them — and this + * way the answer describes a single instant. + * + * Text only, by design. The transport decodes stdout as UTF-8, so bytes that + * aren't text arrive mangled; detecting that here and saying so is honest, + * where handing back replacement characters would look like content. + */ + async readFile(session: string, path: string, signal?: AbortSignal): Promise { + const s = this.ensureSession(session); + await s.ready; + const max = MAX_READ_BYTES; + const script = + `p=${shq(path)}; ` + + `if [ -d "$p" ]; then echo DIR; elif [ ! -e "$p" ]; then echo MISSING; else ` + + `n=$(wc -c < "$p"); if [ "$n" -gt ${max} ]; then echo "BIG $n"; else echo "OK $n"; cat "$p"; fi; fi`; + const r = await dockerRun(["exec", s.name, "sh", "-c", script], this.env, signal); + const nl = r.stdout.indexOf("\n"); + const verdict = (nl < 0 ? r.stdout : r.stdout.slice(0, nl)).trim(); + if (verdict === "DIR") return { kind: "directory" }; + if (verdict === "MISSING" || r.exitCode !== 0) return { kind: "missing" }; + if (verdict.startsWith("BIG ")) return { kind: "too-large", bytes: Number(verdict.slice(4)) }; + const text = nl < 0 ? "" : r.stdout.slice(nl + 1); + // A NUL or a decode failure means these bytes were never text. Editing them + // through a string API would corrupt the file, so refuse rather than try. + if (text.includes("") || text.includes("�")) return { kind: "binary" }; + return { kind: "file", text, bytes: Number(verdict.slice(3)) || text.length }; + } + async harvest( session: string, dir: string, diff --git a/src/tools.ts b/src/tools.ts index 02d9e06..f2f880c 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -1,7 +1,14 @@ import { jsonSchema, type LanguageModel, type Tool, type ToolSet, tool } from "ai"; import type { BlobStore } from "./blobs"; +import { replaceOnce, viewSlice } from "./edits"; import type { ArtifactRef } from "./events"; -import { type Executor, formatExecResult, getExecutor, type SandboxInfo } from "./executor"; +import { + type Executor, + formatExecResult, + getExecutor, + type ReadResult, + type SandboxInfo, +} from "./executor"; import { createFetchProvider, type FetchProvider } from "./fetch"; import { contextToText, @@ -296,7 +303,11 @@ function getAttachment(store: Store, blobs: BlobStore, executor: Executor, conve * busybox and little else. Better to say so and point at `command -v` than to * send the model chasing a python3 that isn't there. */ -export function sandboxDescription(info: SandboxInfo, hasAttachments: boolean): string { +export function sandboxDescription( + info: SandboxInfo, + hasAttachments: boolean, + hasFileTools = false, +): string { const secs = (ms: number) => Math.round(ms / 1000); const net = info.network ? "It HAS network access, so package installs work and persist for the rest of the chat — use the image's own " + @@ -332,6 +343,16 @@ export function sandboxDescription(info: SandboxInfo, hasAttachments: boolean): (hasAttachments ? " Files the user attached, and documents from earlier turns, arrive at /workspace/inputs/ via get_attachment." : ""), + ...(hasFileTools + ? [ + "", + "Files: reach for view_file, write_file and edit_file rather than doing it here. A " + + "heredoc has to survive the shell's quoting and a `sed` expression has to survive its " + + "own, and when that goes wrong the result is a mangled file rather than an error. " + + "This tool is still the right one for everything around the file: listing, searching, " + + "running it, moving it, installing what it needs.", + ] + : []), ].join("\n"); } @@ -346,7 +367,7 @@ function runShell(executor: Executor, ctx: ToolContext) { ); const maxSeconds = Math.round(executor.info.maxTimeoutMs / 1000); return tool({ - description: sandboxDescription(executor.info, hasAttachments), + description: sandboxDescription(executor.info, hasAttachments, Boolean(ctx.conversationId)), inputSchema: jsonSchema<{ command: string; timeout_seconds?: number }>({ type: "object", properties: { @@ -378,6 +399,178 @@ function runShell(executor: Executor, ctx: ToolContext) { }); } +/** + * Working with a file, without going through a shell. + * + * `run_shell` can already read and write files, and for one-liners it is the + * better tool. It falls down on exactly the operations a model does most: get a + * file's contents with line numbers you can then refer to, and change a few + * lines in the middle of it. Through a shell the second one is a heredoc or a + * `sed` expression, where the model's real content has to survive two levels of + * quoting — and the failure mode is not an error but a mangled file. + * + * So the bytes make one trip to this process, the edit happens in a real string + * API, and the bytes go back. What the model sends is what lands. + */ +// Exported for tests: the wrappers are exercised against a fake executor, so +// path handling and the phrasing of every failure are checkable without docker. +function describePaths(): string { + return ( + "Paths are inside the sandbox and may be relative to /workspace (so " + + "'notes.md' and '/workspace/notes.md' are the same file)." + ); +} + +/** Absolute, or resolved against the workspace root the sandbox starts in. */ +function sandboxPath(path: string): string { + const p = path.trim(); + if (!p) return ""; + return p.startsWith("/") ? p : `/workspace/${p.replace(/^\.\//, "")}`; +} + +/** A read that failed, phrased so the model knows what to do next. */ +function explainRead(result: ReadResult, path: string): string | null { + switch (result.kind) { + case "missing": + return `No file at ${path}. Check the path with run_shell (\`ls\`) — it may be somewhere else, or not written yet.`; + case "directory": + return `${path} is a directory, not a file. List it with run_shell (\`ls ${path}\`).`; + case "too-large": + return `${path} is ${result.bytes} bytes, too big to read in one piece. Use run_shell to slice the part you need (\`head\`, \`sed -n\`, \`grep -n\`).`; + case "binary": + return `${path} is not text, so it can't be viewed or edited this way. Handle it with run_shell, or promote it to /workspace/outputs to hand it to the user.`; + default: + return null; + } +} + +export function viewFile(executor: Executor, session: string) { + return tool({ + description: + "Read a text file from the sandbox, with line numbers. Prefer this over `cat`: the numbers " + + "are what later edits and your own explanations refer to, and long lines are truncated " + + "rather than flooding the conversation. Reads 200 lines from the top by default; pass " + + "offset and limit to walk a longer file. " + + describePaths(), + inputSchema: jsonSchema<{ path: string; offset?: number; limit?: number }>({ + type: "object", + properties: { + path: { type: "string", description: "The file to read, e.g. 'src/main.py'." }, + offset: { + type: "number", + description: "0-based line to start at. Omit to start at the top.", + }, + limit: { type: "number", description: "How many lines to read. Defaults to 200." }, + }, + required: ["path"], + additionalProperties: false, + }), + execute: async ({ path, offset, limit }, { abortSignal }) => { + const full = sandboxPath(path); + if (!full) return "path is required."; + const read = await executor.readFile(session, full, abortSignal); + const problem = explainRead(read, full); + if (problem) return problem; + if (read.kind !== "file") return problem ?? "Could not read that file."; + if (read.text === "") return `${full} is empty (0 bytes).`; + const slice = viewSlice(read.text, offset ?? 0, limit ?? DEFAULT_VIEW_LINES); + if (slice.shown === 0) { + return `${full} has ${slice.total} lines, so there is nothing at offset ${offset}.`; + } + const end = slice.from + slice.shown - 1; + const more = + end < slice.total + ? `\n\n[showing lines ${slice.from}-${end} of ${slice.total}. Pass offset: ${end} to continue.]` + : ""; + return `${full}\n${slice.body}${more}`; + }, + }); +} + +export function writeFile(executor: Executor, session: string) { + return tool({ + description: + "Write a text file in the sandbox, creating parent directories and replacing anything " + + "already there. This is the tool for a NEW file, or for a rewrite so extensive that " + + "quoting the old text would be pointless; to change part of a file, edit_file leaves the " + + "rest alone and is far harder to get wrong. Content is written exactly as given — no shell " + + "quoting, no escaping. " + + describePaths(), + inputSchema: jsonSchema<{ path: string; content: string }>({ + type: "object", + properties: { + path: { type: "string", description: "The file to write, e.g. 'src/main.py'." }, + content: { type: "string", description: "The file's complete new contents." }, + }, + required: ["path", "content"], + additionalProperties: false, + }), + execute: async ({ path, content }, { abortSignal }) => { + const full = sandboxPath(path); + if (!full) return "path is required."; + const before = await executor.readFile(session, full, abortSignal); + 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).`; + }, + }); +} + +export function editFile(executor: Executor, session: string) { + return tool({ + description: + "Change part of a text file in the sandbox by exact find-and-replace. old_string must " + + "appear EXACTLY once — include the surrounding lines that make it unique — or pass " + + "replace_all to change every occurrence. Copy old_string byte-for-byte from view_file, " + + "whitespace included; if it doesn't match, the reply shows you what the file really says. " + + "Nothing outside old_string is touched. " + + describePaths(), + inputSchema: jsonSchema<{ + path: string; + old_string: string; + new_string: string; + replace_all?: boolean; + }>({ + type: "object", + properties: { + path: { type: "string", description: "The file to edit." }, + old_string: { type: "string", description: "The exact text to replace." }, + new_string: { + type: "string", + description: "What to put in its place (may be empty to delete).", + }, + replace_all: { + type: "boolean", + description: "Replace every occurrence instead of requiring a unique one.", + }, + }, + required: ["path", "old_string", "new_string"], + additionalProperties: false, + }), + execute: async ({ path, old_string, new_string, replace_all }, { abortSignal }) => { + const full = sandboxPath(path); + if (!full) return "path is required."; + const read = await executor.readFile(session, full, abortSignal); + const problem = explainRead(read, full); + if (problem) return problem; + if (read.kind !== "file") return "Could not read that file."; + const out = replaceOnce(read.text, old_string, new_string, replace_all === true); + if (!out.ok) return out.reason; + await executor.putFile(session, full, new TextEncoder().encode(out.text), abortSignal); + 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}.`; + }, + }); +} + +/** Lines a view returns when the caller doesn't say. Anthropic's tool uses the same default. */ +const DEFAULT_VIEW_LINES = 200; + /** * Drain /workspace/outputs into blobs after a command. * @@ -526,6 +719,33 @@ const REGISTRY: Array<{ return e ? runShell(e, ctx) : null; }, }, + // The file tools need a conversation to be the sandbox session: a one-off + // container would be a fresh filesystem per call, so viewing a file you just + // wrote would find nothing. + { + name: "view_file", + executor: "sandbox", + create: (ctx) => { + const e = getExecutor(); + return e && ctx.conversationId ? viewFile(e, ctx.conversationId) : null; + }, + }, + { + name: "write_file", + executor: "sandbox", + create: (ctx) => { + const e = getExecutor(); + return e && ctx.conversationId ? writeFile(e, ctx.conversationId) : null; + }, + }, + { + name: "edit_file", + executor: "sandbox", + create: (ctx) => { + const e = getExecutor(); + return e && ctx.conversationId ? editFile(e, ctx.conversationId) : null; + }, + }, ]; // lard memory tools, bound to ONE kloe user's token (store + sub). Only offered diff --git a/tests/edits.test.ts b/tests/edits.test.ts new file mode 100644 index 0000000..06a7424 --- /dev/null +++ b/tests/edits.test.ts @@ -0,0 +1,85 @@ +import { expect, test } from "bun:test"; +import { MAX_LINE, mismatchHint, replaceOnce, viewSlice } from "../src/edits"; + +// The text half of the file tools (src/edits.ts): what a view shows, and when +// an edit is allowed to happen. No container involved — that's the point of +// keeping this apart from the executor. + +test("a view numbers lines from their real position and reports the whole", () => { + const text = "alpha\nbravo\ncharlie\ndelta\n"; + const all = viewSlice(text); + expect(all.total).toBe(4); // the trailing newline is not a fifth line + expect(all.from).toBe(1); + expect(all.body.split("\n")[0]).toBe(" 1|alpha"); + + const mid = viewSlice(text, 2, 2); + expect(mid.from).toBe(3); + expect(mid.shown).toBe(2); + expect(mid.body).toBe(" 3|charlie\n 4|delta"); +}); + +test("a view past the end shows nothing rather than pretending", () => { + expect(viewSlice("one\ntwo", 9, 10).shown).toBe(0); +}); + +test("a very long line is truncated, so one line can't fill the window", () => { + const long = "x".repeat(MAX_LINE + 500); + const body = viewSlice(long).body; + expect(body.length).toBeLessThan(MAX_LINE + 100); + expect(body).toContain("[line truncated]"); +}); + +test("an edit needs a unique match, and says how many it found", () => { + const content = "a = 1\nb = 2\na = 1\n"; + const ambiguous = replaceOnce(content, "a = 1", "a = 3"); + expect(ambiguous.ok).toBe(false); + if (!ambiguous.ok) { + expect(ambiguous.reason).toContain("appears 2 times"); + expect(ambiguous.reason).toContain("replace_all"); + } + + // More context makes it unique. + const unique = replaceOnce(content, "b = 2\na = 1", "b = 2\na = 3"); + expect(unique.ok).toBe(true); + if (unique.ok) expect(unique.text).toBe("a = 1\nb = 2\na = 3\n"); + + const all = replaceOnce(content, "a = 1", "a = 3", true); + expect(all.ok).toBe(true); + if (all.ok) { + expect(all.replaced).toBe(2); + expect(all.text).toBe("a = 3\nb = 2\na = 3\n"); + } +}); + +test("an empty new_string deletes, and a no-op edit is refused", () => { + const gone = replaceOnce("keep\ndrop\nkeep2\n", "drop\n", ""); + expect(gone.ok).toBe(true); + if (gone.ok) expect(gone.text).toBe("keep\nkeep2\n"); + + expect(replaceOnce("x", "", "y").ok).toBe(false); + expect(replaceOnce("x", "same", "same").ok).toBe(false); +}); + +test("a whitespace mismatch is diagnosed with the file's real bytes", () => { + // The file indents with a tab; the model wrote spaces. Invisible in a + // transcript, and the most common reason an exact edit misses. + const content = "def f():\n\treturn 1\n"; + const out = replaceOnce(content, "def f():\n return 1", "def f():\n return 2"); + expect(out.ok).toBe(false); + if (!out.ok) { + expect(out.reason).toContain("whitespace differs"); + expect(out.reason).toContain("→"); // the tab, made visible + expect(out.reason).toContain("Lines 1-2"); + } +}); + +test("a near miss points at the closest lines instead of just saying no", () => { + const content = ["import os", "", "def greet(name):", ' print("hi", name)', ""].join("\n"); + const hint = mismatchHint(content, 'def greet(person):\n print("hi", person)'); + expect(hint).toContain("line 3"); + expect(hint).toContain("def greet(name):"); +}); + +test("a hint is withheld when nothing in the file is close", () => { + expect(mismatchHint("nothing alike here\n", "completely different text")).toBeNull(); +}); diff --git a/tests/executor.test.ts b/tests/executor.test.ts index 0f7bd63..164788e 100644 --- a/tests/executor.test.ts +++ b/tests/executor.test.ts @@ -330,3 +330,37 @@ liveTest( }, 90_000, ); + +liveTest( + "readFile classifies what it found, not just whether it worked", + async () => { + const e = new LocalDockerExecutor(SANDBOX); + const session = "test-" + Math.random().toString(36).slice(2); + try { + await e.putFile(session, "/workspace/notes.md", new TextEncoder().encode("one\ntwo\n")); + expect(await e.readFile(session, "/workspace/notes.md")).toMatchObject({ + kind: "file", + text: "one\ntwo\n", + }); + + // The three ways a read fails are three different mistakes, and the tool + // above phrases a different fix for each. + expect((await e.readFile(session, "/workspace/nope.md")).kind).toBe("missing"); + expect((await e.readFile(session, "/workspace")).kind).toBe("directory"); + + // Bytes that were never text: refused rather than mangled into a string. + await e.putFile(session, "/workspace/blob.bin", new Uint8Array([0, 1, 2, 0, 255])); + expect((await e.readFile(session, "/workspace/blob.bin")).kind).toBe("binary"); + + // A file too big to inline is a different answer from a file that failed. + await e.run({ + command: "head -c 300000 /dev/zero | tr '\\0' 'x' > /workspace/big.txt", + session, + }); + expect((await e.readFile(session, "/workspace/big.txt")).kind).toBe("too-large"); + } finally { + e.disposeSession(session); + } + }, + 120_000, +); diff --git a/tests/filetools.test.ts b/tests/filetools.test.ts new file mode 100644 index 0000000..28765d4 --- /dev/null +++ b/tests/filetools.test.ts @@ -0,0 +1,168 @@ +import { expect, test } from "bun:test"; +import type { ExecResult, Executor, HarvestedFile, ReadResult } from "../src/executor"; +import { editFile, viewFile, writeFile } from "../src/tools"; + +/** + * The sandbox file tools, against a fake sandbox. + * + * The docker half (reading a real file, classifying a directory or a binary) is + * covered live in executor.test.ts. What matters here is everything the tools + * decide: where a relative path lands, what the model is told when a read + * fails, and that an edit writes back exactly what it computed. + */ +class FakeSandbox implements Executor { + readonly kind = "fake"; + readonly info = { + image: "alpine:3.20", + network: false, + defaultTimeoutMs: 30_000, + maxTimeoutMs: 300_000, + memory: "2g", + cpus: "2", + }; + files = new Map(); + dirs = new Set(["/workspace"]); + /** Every path the tools asked about, in order — the resolution record. */ + asked: string[] = []; + binary = new Set(); + + run(): Promise { + throw new Error("not used"); + } + async putFile(_session: string, path: string, bytes: Uint8Array): Promise { + this.files.set(path, new TextDecoder().decode(bytes)); + } + async readFile(_session: string, path: string): Promise { + this.asked.push(path); + if (this.dirs.has(path)) return { kind: "directory" }; + if (this.binary.has(path)) return { kind: "binary" }; + const text = this.files.get(path); + if (text === undefined) return { kind: "missing" }; + return { kind: "file", text, bytes: text.length }; + } + async harvest(): Promise { + return []; + } + disposeSession(): void {} +} + +type Exec = (input: never, opts: { toolCallId: string; messages: [] }) => Promise; +function call(t: { execute?: unknown }, input: unknown): Promise { + return (t.execute as Exec)(input as never, { toolCallId: "c", messages: [] }); +} + +test("a relative path lands in the workspace, an absolute one is left alone", async () => { + const box = new FakeSandbox(); + box.files.set("/workspace/notes.md", "hello\n"); + box.files.set("/etc/hosts", "127.0.0.1\n"); + const view = viewFile(box, "c1"); + + expect(await call(view, { path: "notes.md" })).toContain("hello"); + expect(await call(view, { path: "./notes.md" })).toContain("hello"); + expect(await call(view, { path: "/etc/hosts" })).toContain("127.0.0.1"); + expect(box.asked).toEqual(["/workspace/notes.md", "/workspace/notes.md", "/etc/hosts"]); +}); + +test("a view numbers lines and says how to see the rest", async () => { + const box = new FakeSandbox(); + box.files.set( + "/workspace/long.txt", + Array.from({ length: 500 }, (_, i) => `L${i + 1}`).join("\n"), + ); + const out = (await call(viewFile(box, "c1"), { path: "long.txt" })) as string; + + expect(out).toContain(" 1|L1"); + expect(out).toContain(" 200|L200"); + expect(out).not.toContain("|L201"); + expect(out).toContain("showing lines 1-200 of 500"); + expect(out).toContain("offset: 200"); +}); + +test("each way a read fails names a different fix", async () => { + const box = new FakeSandbox(); + box.binary.add("/workspace/a.png"); + const view = viewFile(box, "c1"); + + expect(await call(view, { path: "ghost.md" })).toContain("run_shell"); + expect(await call(view, { path: "ghost.md" })).toContain("No file at /workspace/ghost.md"); + expect(await call(view, { path: "/workspace" })).toContain("is a directory"); + expect(await call(view, { path: "a.png" })).toContain("not text"); + // An empty file is a fact about the file, not a failure to read it. + box.files.set("/workspace/empty.txt", ""); + expect(await call(view, { path: "empty.txt" })).toContain("is empty"); +}); + +test("writing reports whether it created or replaced, and writes it verbatim", async () => { + const box = new FakeSandbox(); + const write = writeFile(box, "c1"); + // Content that would need escaping through a shell, passed through untouched. + const tricky = "line 'one'\n$(echo hi) `date`\n\"two\"\n"; + + expect(await call(write, { path: "src/main.py", content: tricky })).toContain("Created"); + expect(box.files.get("/workspace/src/main.py")).toBe(tricky); + + const again = (await call(write, { path: "src/main.py", content: "replaced\n" })) as string; + expect(again).toContain("Overwrote"); + expect(box.files.get("/workspace/src/main.py")).toBe("replaced\n"); + + expect(await call(write, { path: "/workspace", content: "x" })).toContain("is a directory"); +}); + +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 out = (await call(edit, { + path: "app.py", + old_string: " return 1", + new_string: " return 2\n # changed", + })) as string; + expect(out).toContain("1 replacement"); + expect(out).toContain("+1 lines"); + expect(box.files.get("/workspace/app.py")).toBe("def main():\n return 2\n # changed\n"); +}); + +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 ambiguous = (await call(edit, { + path: "c.py", + old_string: "a = 1", + new_string: "a = 9", + })) as string; + expect(ambiguous).toContain("appears 2 times"); + expect(box.files.get("/workspace/c.py")).toBe(before); // untouched + + const missing = (await call(edit, { + path: "c.py", + old_string: "nowhere to be found", + new_string: "x", + })) as string; + expect(missing).toContain("not found"); + expect(box.files.get("/workspace/c.py")).toBe(before); + + // replace_all is the way through, and it says how many it changed. + const all = (await call(edit, { + path: "c.py", + old_string: "a = 1", + new_string: "a = 9", + replace_all: true, + })) as string; + expect(all).toContain("2 replacements"); + expect(box.files.get("/workspace/c.py")).toBe("a = 9\nb = 2\na = 9\n"); +}); + +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"), { + path: "ghost.py", + old_string: "a", + new_string: "b", + })) as string; + expect(out).toContain("No file at /workspace/ghost.py"); + expect(box.files.size).toBe(0); +}); diff --git a/tests/tools.test.ts b/tests/tools.test.ts index aa067d5..141ded4 100644 --- a/tests/tools.test.ts +++ b/tests/tools.test.ts @@ -81,3 +81,16 @@ test("sandboxDescription mentions get_attachment only when that tool is offered" expect(sandboxDescription(INFO, true)).toContain("get_attachment"); expect(sandboxDescription(INFO, false)).not.toContain("get_attachment"); }); + +test("the shell description points at the file tools only when they're offered", () => { + const withFiles = sandboxDescription(INFO, false, true); + expect(withFiles).toContain("view_file"); + expect(withFiles).toContain("edit_file"); + // The reason, not just the instruction: a model that knows WHY reaches for + // the right tool in cases this text didn't enumerate. + expect(withFiles).toContain("quoting"); + + // A one-off sandbox (no conversation) has no persistent filesystem to edit, + // so the tools aren't offered and must not be advertised. + expect(sandboxDescription(INFO, false, false)).not.toContain("view_file"); +}); -- 2.51.2