From 69465905dd984981779f5d7936597e6abda511c4 Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 03:22:09 +0300 Subject: [PATCH 1/4] fix(tools): F37 a tool call carrying model control markers is refused, not run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live, Gemma 4 31B under the GBNF grammar: the first call of a turn was `os.fs.list {"path": ".}}]thought<|channel>thought---…"}`. The model opened another thought channel mid-call; the grammar admits those bytes only inside a JSON string, so the markers landed in `path`. The tool ran on the garbage (ENAMETOOLONG), the model read "the folder is empty" and overwrote the user's input file. - control-marker-guard: `findControlMarkers` walks every string argument (nested objects and arrays included) for Gemma 4, Qwen/ChatML and Gemma 3 markers plus the generic `<|name|>` form, one exported list with a comment per family. Paths, commands, patterns, URLs and ids are flagged on any occurrence; the writing tools' content arguments (`os.fs.write.content`, `os.fs.edit.oldString/newString`, `os.fs.patch.patch`) only when a marker starts a line — the line F24's write-time check draws — so source that mentions `` in a comment is not corrupted. A patch line's start sits after its `+`/`-` prefix. - batch-executor: a flagged non-terminal call never reaches the registry; its slot gets an error result naming the argument, marker, offset and a short excerpt, with `details.corrupted` and the hits, so the loop tracker counts it like any error and the trace row carries it. No parse-recovery budget is spent: the model reads the result on its next step. Terminals stay exempt — a reply is shown, not run. - build-grammar: the reasoning profiles' string body excludes `<|` and `|>` through a three-state rule over the last character (the naive `"<" [^|] | "|" [^>]` shape lets `<<|` and `||>` through); the plain profile's grammar stays byte-identical to the file. Pinned with a small test-side GBNF interpreter that reads the emitted grammar. --- src/agent/batch-executor.test.ts | 151 ++++++++++++ src/agent/batch-executor.ts | 60 ++++- src/llm/grammar/build-grammar.test.ts | 88 +++++++ src/llm/grammar/build-grammar.ts | 41 +++- src/llm/grammar/gbnf-test-helpers.ts | 324 +++++++++++++++++++++++++ src/tools/control-marker-guard.test.ts | 197 +++++++++++++++ src/tools/control-marker-guard.ts | 213 ++++++++++++++++ 7 files changed, 1060 insertions(+), 14 deletions(-) create mode 100644 src/llm/grammar/gbnf-test-helpers.ts create mode 100644 src/tools/control-marker-guard.test.ts create mode 100644 src/tools/control-marker-guard.ts diff --git a/src/agent/batch-executor.test.ts b/src/agent/batch-executor.test.ts index 89c08400..0a4c1780 100644 --- a/src/agent/batch-executor.test.ts +++ b/src/agent/batch-executor.test.ts @@ -15,6 +15,8 @@ import { type BatchLoopSignal, } from "./batch-executor.js"; import { LOOP_VETO_DENIED_REASON, ToolLoopTracker } from "./loop-detector.js"; +import { createTraceRecorder } from "../tracing/trace/trace-recorder.js"; +import type { TraceEvent } from "../tracing/trace/trace-event.js"; function ctx(signal: AbortSignal) { return { @@ -1438,3 +1440,152 @@ describe("executeBatch outcome-repeat detector (F25)", () => { ); }); }); + +describe("executeBatch refuses a corrupted call (F37)", () => { + /** The live Gemma 4 call: a thought channel opened inside `path`. */ + const LIVE_PATH = ".}}]thought<|channel>thought---"; + + it("does not run a call whose argument carries a control marker and answers with the error shape", async () => { + const run = vi.fn(async () => okResult("os.fs.list", "(empty)")); + const registry = buildRegistry({ "os.fs.list": run }); + const out = await executeBatch( + toBatchInputs([{ tool: "os.fs.list", args: { path: LIVE_PATH } }]), + registry, + ctx(new AbortController().signal), + ); + expect(run).not.toHaveBeenCalled(); + const result = out.results[0]!.compressed!; + expect(result.status).toBe("error"); + expect(result.summary).toBe( + 'corrupted tool call: argument `path` contains a model control marker (`` at char 4: ".}}]thought<|channel…"). The call was not run — re-emit it with clean arguments.', + ); + expect(result.details).toEqual({ + corrupted: true, + markers: [ + { + path: "path", + marker: "", + index: 4, + excerpt: ".}}]thought<|channel…", + }, + ], + }); + expect(out.cancelled).toBe(false); + }); + + it("lands details.corrupted on the tool_invocation trace row", async () => { + const registry = buildRegistry({ + "os.fs.list": async () => okResult("os.fs.list"), + }); + const events: TraceEvent[] = []; + const recorder = createTraceRecorder({ + sessionId: "s1", + emit: (event) => events.push(event), + now: () => 0, + }); + recorder.onAgentEvent({ type: "turn_started", turnIndex: 0 }); + recorder.onAgentEvent({ type: "step_started", stepIndex: 0 }); + const call = { tool: "os.fs.list", args: { path: LIVE_PATH } }; + recorder.onAgentEvent({ + type: "llm_event", + event: { type: "tool_call_parsed", call, batchIndex: 0, batchSize: 1 }, + }); + await executeBatch(toBatchInputs([call]), registry, { + ...ctx(new AbortController().signal), + onCallFinished: ({ result, batchIndex, batchSize }) => + recorder.onAgentEvent({ + type: "llm_event", + event: { type: "tool_call_executed", result, batchIndex, batchSize }, + }), + }); + const row = events.find((e) => e.type === "tool_invocation"); + expect(row).toMatchObject({ + type: "tool_invocation", + tool: "os.fs.list", + status: "error", + args: { path: LIVE_PATH }, + details: { corrupted: true }, + }); + }); + + it("counts toward the loop detector like any other error", async () => { + const run = vi.fn(async () => okResult("os.fs.list")); + const registry = buildRegistry({ "os.fs.list": run }); + const tracker = new ToolLoopTracker({ criticalThreshold: 3 }); + const signals: BatchLoopSignal[] = []; + for (let i = 0; i < 4; i += 1) { + const out = await executeBatch( + toBatchInputs([{ tool: "os.fs.list", args: { path: LIVE_PATH } }]), + registry, + { ...ctx(new AbortController().signal), tracker }, + ); + signals.push(...out.loopSignals); + } + expect(run).not.toHaveBeenCalled(); + // The same refused call, repeated, is a no-progress loop: the + // refusals were recorded as outcomes and the gate eventually vetoes. + expect(signals.some((s) => s.kind === "critical")).toBe(true); + }); + + it("runs a write whose content mentions a marker mid-line, refuses one whose line starts with it", async () => { + const run = vi.fn(async () => okResult("os.fs.write", "wrote")); + const registry = buildRegistry({ "os.fs.write": run }, false); + const clean = await executeBatch( + toBatchInputs([ + { + tool: "os.fs.write", + args: { path: "a.ts", content: "// wraps tags\nconst x = 1;" }, + }, + ]), + registry, + ctx(new AbortController().signal), + ); + expect(run).toHaveBeenCalledTimes(1); + expect(clean.results[0]!.compressed?.status).toBe("ok"); + + const corrupted = await executeBatch( + toBatchInputs([ + { + tool: "os.fs.write", + args: { path: "a.ts", content: "const x = 1;\n<|channel>thought\n" }, + }, + ]), + registry, + ctx(new AbortController().signal), + ); + expect(run).toHaveBeenCalledTimes(1); + expect(corrupted.results[0]!.compressed).toMatchObject({ + status: "error", + details: { corrupted: true, markers: [{ path: "content", marker: "<|channel>" }] }, + }); + }); + + it("refuses only the corrupted call of a batch; its siblings and the tail reply run", async () => { + const list = vi.fn(async () => okResult("os.fs.list")); + const read = vi.fn(async () => okResult("os.fs.read")); + const reply = vi.fn(async () => okResult("reply")); + const registry = buildRegistry({ + "os.fs.list": list, + "os.fs.read": read, + reply, + }); + const out = await executeBatch( + toBatchInputs([ + { tool: "os.fs.read", args: { path: "README.md" } }, + { tool: "os.fs.list", args: { path: LIVE_PATH } }, + { tool: "reply", args: { text: "the tag is spelled " } }, + ]), + registry, + ctx(new AbortController().signal), + ); + expect(read).toHaveBeenCalledTimes(1); + expect(list).not.toHaveBeenCalled(); + // A terminal's text is shown, not run; the turn must be able to close. + expect(reply).toHaveBeenCalledTimes(1); + expect(out.results.map((r) => r.compressed?.status)).toEqual([ + "ok", + "error", + "ok", + ]); + }); +}); diff --git a/src/agent/batch-executor.ts b/src/agent/batch-executor.ts index e5e00845..d63383db 100644 --- a/src/agent/batch-executor.ts +++ b/src/agent/batch-executor.ts @@ -12,6 +12,10 @@ import { import type { ToolRegistry } from "../tools/tool-registry.js"; import type { ToolRole } from "../tools/tool-roles.js"; import { describeArgumentError } from "../tools/argument-error-hint.js"; +import { + describeCorruptedCall, + findControlMarkers, +} from "../tools/control-marker-guard.js"; import { CancelledError } from "../llm/index.js"; import { isParallelWithinGroup, @@ -405,19 +409,12 @@ export async function executeBatch( const groups = planBatch(toInvoke); - const invokeOne = async (input: BatchCallInput): Promise => { - if (ctx.signal.aborted) { - slots[input.batchIndex] = { - ...slots[input.batchIndex]!, - cancelled: true, - }; - return; - } - ctx.onCallStarted?.({ batchIndex: input.batchIndex, batchSize }); - const startedAt = Date.now(); - let compressed: CompressedToolResult; + /** The registry call itself; a thrown error becomes an error result. */ + const invokeRegistry = async ( + input: BatchCallInput, + ): Promise => { try { - compressed = await registry.invoke(input.call.tool, input.call.args, { + return await registry.invoke(input.call.tool, input.call.args, { workingDir: ctx.workingDir, sessionId: ctx.sessionId, stepIndex: ctx.stepIndex, @@ -446,7 +443,7 @@ export async function executeBatch( args: input.call.args, message: cause.message, }); - compressed = compressToolResult({ + return compressToolResult({ tool: input.call.tool, status: "error", output: hint?.message ?? cause.message, @@ -461,6 +458,43 @@ export async function executeBatch( }, }); } + }; + + const invokeOne = async (input: BatchCallInput): Promise => { + if (ctx.signal.aborted) { + slots[input.batchIndex] = { + ...slots[input.batchIndex]!, + cancelled: true, + }; + return; + } + ctx.onCallStarted?.({ batchIndex: input.batchIndex, batchSize }); + const startedAt = Date.now(); + let compressed: CompressedToolResult; + // A call whose argument carries the model's own control markup (F37) + // never reaches the registry: a `path` holding `<|channel>` is a + // thought block that fell into the call, and the tool would run on + // the garbage (it listed an ENAMETOOLONG path as "empty" once, and + // the model overwrote the input file on that reading). The refusal + // is an ordinary error result — recorded in the loop tracker like + // any other, on the trace row via `details.corrupted` — that the + // model reads on its next step; no parse-recovery budget is spent. + // Terminals are exempt for the reason every gate exempts them: a + // reply's text is shown, not run, and the turn must be able to close. + const markers = + input.resourceClass === "terminal" + ? [] + : findControlMarkers(input.call.args, input.call.tool); + if (markers.length > 0) { + compressed = compressToolResult({ + tool: input.call.tool, + status: "error", + output: describeCorruptedCall(markers), + details: { corrupted: true, markers }, + }); + } else { + compressed = await invokeRegistry(input); + } const durationMs = Date.now() - startedAt; slots[input.batchIndex] = { ...slots[input.batchIndex]!, diff --git a/src/llm/grammar/build-grammar.test.ts b/src/llm/grammar/build-grammar.test.ts index d2ff990a..2286216a 100644 --- a/src/llm/grammar/build-grammar.test.ts +++ b/src/llm/grammar/build-grammar.test.ts @@ -1,3 +1,7 @@ +import { readFile } from "node:fs/promises"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + import { describe, expect, it } from "vitest"; import { @@ -10,6 +14,90 @@ import { buildGrammarForTools, grammarToolNames, } from "./build-grammar.js"; +import { gbnfAccepts } from "./gbnf-test-helpers.js"; + +const GRAMMAR_FILE = resolve( + dirname(fileURLToPath(import.meta.url)), + "../../../grammars/tool-call.gbnf", +); + +describe("string arguments under a reasoning profile (F37)", () => { + const CALL = (path: string): string => + `[{"tool":"os.fs.list","args":{"path":${JSON.stringify(path)}}}]`; + const GEMMA_COMPLETION = (path: string): string => + `<|channel>thought\nlist first\n${CALL(path)}`; + const LIVE_PATH = ".}}]thought<|channel>thought---"; + + it("keeps the plain profile's grammar byte-identical to the file", async () => { + const grammar = await buildGrammar(PLAIN_INSTRUCT_PROFILE); + expect(grammar).toBe(await readFile(GRAMMAR_FILE, "utf8")); + expect(grammar).not.toContain("str-neutral"); + }); + + it("the matcher reads the base grammar as llama.cpp would: a well-formed array in, a bare object out", async () => { + const plain = await buildGrammar(PLAIN_INSTRUCT_PROFILE); + expect(gbnfAccepts(plain, "root", CALL("."))).toBe(true); + expect(gbnfAccepts(plain, "root", '[{"tool":"os.fs.list","args":{"path":"a","n":1,"x":[true,null]}},{"tool":"reply","args":{"text":"ok"}}]')).toBe(true); + expect(gbnfAccepts(plain, "root", '{"tool":"os.fs.list","args":{}}')).toBe(false); + expect(gbnfAccepts(plain, "root", '[{"tool":"os.fs.nope","args":{}}]')).toBe(false); + // The plain string body admits the marker: that grammar is unchanged. + expect(gbnfAccepts(plain, "string", '"<|channel>"')).toBe(true); + }); + + it("rejects a string containing <|channel> and accepts a under gemma and qwen", async () => { + for (const profile of [GEMMA4_THINK_PROFILE, QWEN_THINK_PROFILE]) { + const grammar = await buildGrammar(profile); + expect(grammar, profile.id).toContain("chars ::= str-neutral"); + expect(grammar, profile.id).not.toContain("chars ::= char*"); + for (const bad of [ + '"<|channel>"', + '"x<|channel>"', + '""', + '"a<|"', + '"|>a"', + '"<<|channel>"', + '"||>"', + '"<|im_start|>"', + `"${LIVE_PATH.replace(/"/g, '\\"')}"`, + ]) { + expect(gbnfAccepts(grammar, "string", bad), `${profile.id} ${bad}`).toBe( + false, + ); + } + for (const good of [ + '""', + '"a"', + '"<>"', + '"|<"', + '"a<>c||d"', + '"<\\"|"', + '"a\\"b\\\\c\\n\\u003c"', + '"src/index.ts"', + ]) { + expect(gbnfAccepts(grammar, "string", good), `${profile.id} ${good}`).toBe( + true, + ); + } + } + }); + + it("makes the live corruption unemittable in a whole gemma completion while the clean call passes", async () => { + const gemma = await buildGrammar(GEMMA4_THINK_PROFILE); + expect(gbnfAccepts(gemma, "root", GEMMA_COMPLETION("."))).toBe(true); + expect(gbnfAccepts(gemma, "root", GEMMA_COMPLETION("ad"))).toBe(true); + expect(gbnfAccepts(gemma, "root", GEMMA_COMPLETION(LIVE_PATH))).toBe(false); + }); + + it("survives the per-request rewrite, which touches only the tool-name rule", async () => { + const gemma = await buildGrammar(GEMMA4_THINK_PROFILE); + const narrowed = buildGrammarForTools(gemma, ["os.fs.list"]); + expect(narrowed).toContain("chars ::= str-neutral"); + expect(gbnfAccepts(narrowed, "root", GEMMA_COMPLETION("."))).toBe(true); + expect(gbnfAccepts(narrowed, "root", GEMMA_COMPLETION(LIVE_PATH))).toBe(false); + }); +}); describe("buildGrammar", () => { it("keeps the plain instruct grammar pinned to the array-only root", async () => { diff --git a/src/llm/grammar/build-grammar.ts b/src/llm/grammar/build-grammar.ts index 7b9afbe3..ee9883a1 100644 --- a/src/llm/grammar/build-grammar.ts +++ b/src/llm/grammar/build-grammar.ts @@ -64,7 +64,9 @@ export async function buildGrammar( // This avoids the GBNF first-token bias toward `{` that small models // exhibit even when their `` block reasoned about parallelism. const rootRule = `root ::= ${ruleStem}-prelude tool-call-array`; - const withPreludeRoot = withMcp.replace(/^root ::= .*$/m, rootRule); + const withPreludeRoot = hardenStringRule( + withMcp.replace(/^root ::= .*$/m, rootRule), + ); // When the model emits its own reasoning open tag (Gemma 4 turn-framing), // the prelude must force that opener — the prompt no longer prefills it. const openSentinel = reasoningOpenEmittedByModel(profile) @@ -113,6 +115,43 @@ function buildUntilSentinelRules( ].join("\n"); } +/** The base grammar's string-body rule, `string ::= "\"" chars "\""`. */ +const CHARS_RULE_RE = /^chars ::= char\*$/m; + +/** + * The string body for a reasoning profile: valid JSON with the + * two-character sequences `<|` and `|>` excluded (F37). The model's own + * control markers open with one and close with the other (`<|channel>`, + * ``, `<|turn>`, ``, `<|im_start|>`), and the grammar + * admits those bytes nowhere but inside a JSON string — so a thought + * block that opens mid-call lands in an argument value, and the tool + * runs on it. With the sequences gone from the sampler's vocabulary the + * marker cannot be emitted there at all; `control-marker-guard.ts` + * still refuses whatever arrives another way (an escaped `<|`, a + * provider without grammars). + * + * A three-state automaton over the last character — neutral, after + * `<`, after `|` — rather than the tempting `"<" [^|] | "|" [^>]` + * alternatives: those let `<<|` and `||>` through (the first `<` pairs + * with the second, freeing the `|`). Every alternative is decided by + * one character, so the sampler carries one stack per string, and the + * right recursion is the same shape llama.cpp expands `char*` into. + * Only the reasoning profiles get it; the plain profile's grammar stays + * byte-identical to `grammars/tool-call.gbnf`. + */ +const HARDENED_STRING_RULES = [ + "chars ::= str-neutral", + 'str-neutral ::= ( str-plain str-neutral | "<" str-after-lt | "|" str-after-bar )?', + 'str-after-lt ::= ( str-plain str-neutral | "<" str-after-lt )?', + 'str-after-bar ::= ( str-plain-not-gt str-neutral | "<" str-after-lt | "|" str-after-bar )?', + 'str-plain ::= [^"\\\\\\x00-\\x1f<|] | "\\\\" escape', + 'str-plain-not-gt ::= [^"\\\\\\x00-\\x1f<|>] | "\\\\" escape', +].join("\n"); + +function hardenStringRule(grammar: string): string { + return grammar.replace(CHARS_RULE_RE, HARDENED_STRING_RULES); +} + /** The one rule a per-request grammar rewrites. */ const TOOL_NAME_RULE_RE = /^tool-name ::= .*$/m; diff --git a/src/llm/grammar/gbnf-test-helpers.ts b/src/llm/grammar/gbnf-test-helpers.ts new file mode 100644 index 00000000..135ef2e8 --- /dev/null +++ b/src/llm/grammar/gbnf-test-helpers.ts @@ -0,0 +1,324 @@ +/** + * A small GBNF interpreter for tests: does `rule` of `grammar` accept + * exactly `input`? The real thing is llama.cpp's sampler, which reads + * the same text; this lets a test pin the LANGUAGE a grammar edit + * produces (which strings it admits) rather than only its bytes. + * + * Covers the subset `grammars/tool-call.gbnf` and `buildGrammar` use: + * `"literals"` with escapes, `[character classes]`, `|`, `( groups )`, + * `?` `*` `+` `{m,n}` repetition, rule references and `#` comments. A + * set-of-end-positions matcher, so ambiguity and backtracking are free; + * inputs are short. Left recursion is guarded, not supported. + */ + +type Elem = + | { kind: "lit"; text: string } + | { kind: "class"; negate: boolean; ranges: Array<[number, number]> } + | { kind: "ref"; name: string } + | { kind: "group"; alts: Seq[] }; + +interface Item { + elem: Elem; + min: number; + max: number; +} + +type Seq = Item[]; +export type GbnfRules = Map; + +type Token = + | { kind: "name"; text: string } + | { kind: "assign" } + | { kind: "lit"; text: string } + | { kind: "class"; negate: boolean; ranges: Array<[number, number]> } + | { kind: "punct"; text: "(" | ")" | "|" | "?" | "*" | "+" } + | { kind: "repeat"; min: number; max: number }; + +/** True when `rule` of `grammar` matches all of `input`. */ +export function gbnfAccepts( + grammar: string, + rule: string, + input: string, +): boolean { + const rules = parseGbnf(grammar); + if (!rules.has(rule)) throw new Error(`no rule named ${rule}`); + return new Matcher(rules, input).rule(rule, 0).has(input.length); +} + +export function parseGbnf(grammar: string): GbnfRules { + const tokens = tokenize(grammar); + const rules: GbnfRules = new Map(); + let i = 0; + const peek = (): Token | undefined => tokens[i]; + const atRuleStart = (): boolean => + tokens[i]?.kind === "name" && tokens[i + 1]?.kind === "assign"; + + const parseAlternates = (): Seq[] => { + const alts = [parseSequence()]; + while (peek()?.kind === "punct" && (peek() as { text: string }).text === "|") { + i += 1; + alts.push(parseSequence()); + } + return alts; + }; + + const parseSequence = (): Seq => { + const items: Seq = []; + for (;;) { + const token = peek(); + if (token === undefined || atRuleStart()) break; + if (token.kind === "punct" && (token.text === ")" || token.text === "|")) + break; + let elem: Elem; + if (token.kind === "lit") { + elem = { kind: "lit", text: token.text }; + i += 1; + } else if (token.kind === "class") { + elem = { kind: "class", negate: token.negate, ranges: token.ranges }; + i += 1; + } else if (token.kind === "name") { + elem = { kind: "ref", name: token.text }; + i += 1; + } else if (token.kind === "punct" && token.text === "(") { + i += 1; + const alts = parseAlternates(); + const close = peek(); + if (close?.kind !== "punct" || close.text !== ")") + throw new Error("expected )"); + i += 1; + elem = { kind: "group", alts }; + } else { + throw new Error(`unexpected token ${JSON.stringify(token)}`); + } + let min = 1; + let max = 1; + const rep = peek(); + if (rep?.kind === "punct" && rep.text === "?") { + min = 0; + i += 1; + } else if (rep?.kind === "punct" && rep.text === "*") { + min = 0; + max = Infinity; + i += 1; + } else if (rep?.kind === "punct" && rep.text === "+") { + max = Infinity; + i += 1; + } else if (rep?.kind === "repeat") { + min = rep.min; + max = rep.max; + i += 1; + } + items.push({ elem, min, max }); + } + return items; + }; + + while (i < tokens.length) { + const name = tokens[i]; + if (name?.kind !== "name" || tokens[i + 1]?.kind !== "assign") + throw new Error(`expected a rule at token ${i}`); + i += 2; + rules.set(name.text, parseAlternates()); + } + return rules; +} + +function tokenize(grammar: string): Token[] { + const tokens: Token[] = []; + let i = 0; + const readEscape = (): number => { + // `i` is on the char after the backslash. + const c = grammar[i]!; + i += 1; + switch (c) { + case "n": + return 10; + case "r": + return 13; + case "t": + return 9; + case "x": + return readHex(2); + case "u": + return readHex(4); + case "U": + return readHex(8); + default: + return c.codePointAt(0)!; + } + }; + const readHex = (digits: number): number => { + const code = parseInt(grammar.slice(i, i + digits), 16); + i += digits; + return code; + }; + while (i < grammar.length) { + const c = grammar[i]!; + if (c === " " || c === "\t" || c === "\n" || c === "\r") { + i += 1; + } else if (c === "#") { + while (i < grammar.length && grammar[i] !== "\n") i += 1; + } else if (grammar.startsWith("::=", i)) { + tokens.push({ kind: "assign" }); + i += 3; + } else if (/[a-zA-Z]/.test(c)) { + const match = /^[a-zA-Z][a-zA-Z0-9-]*/.exec(grammar.slice(i))!; + tokens.push({ kind: "name", text: match[0] }); + i += match[0].length; + } else if (c === '"') { + i += 1; + let text = ""; + while (grammar[i] !== '"') { + if (i >= grammar.length) throw new Error("unterminated literal"); + if (grammar[i] === "\\") { + i += 1; + text += String.fromCodePoint(readEscape()); + } else { + text += grammar[i]; + i += 1; + } + } + i += 1; + tokens.push({ kind: "lit", text }); + } else if (c === "[") { + i += 1; + let negate = false; + if (grammar[i] === "^") { + negate = true; + i += 1; + } + const ranges: Array<[number, number]> = []; + const readOne = (): number => { + if (grammar[i] === "\\") { + i += 1; + return readEscape(); + } + const code = grammar.codePointAt(i)!; + i += String.fromCodePoint(code).length; + return code; + }; + while (grammar[i] !== "]") { + if (i >= grammar.length) throw new Error("unterminated class"); + const from = readOne(); + if (grammar[i] === "-" && grammar[i + 1] !== "]") { + i += 1; + ranges.push([from, readOne()]); + } else { + ranges.push([from, from]); + } + } + i += 1; + tokens.push({ kind: "class", negate, ranges }); + } else if (c === "{") { + const match = /^\{(\d+)(?:(,)(\d*))?\}/.exec(grammar.slice(i)); + if (match === null) throw new Error("bad repetition"); + const min = Number(match[1]); + const max = + match[2] === undefined + ? min + : match[3] === "" + ? Infinity + : Number(match[3]); + tokens.push({ kind: "repeat", min, max }); + i += match[0].length; + } else if ("()|?*+".includes(c)) { + tokens.push({ kind: "punct", text: c as "(" }); + i += 1; + } else { + throw new Error(`unexpected ${JSON.stringify(c)} at ${i}`); + } + } + return tokens; +} + +class Matcher { + private readonly memo = new Map>(); + private readonly active = new Set(); + + constructor( + private readonly rules: GbnfRules, + private readonly input: string, + ) {} + + rule(name: string, pos: number): Set { + const key = `${name}@${pos}`; + const cached = this.memo.get(key); + if (cached !== undefined) return cached; + // Left recursion would loop; the grammars here have none. + if (this.active.has(key)) return new Set(); + const alts = this.rules.get(name); + if (alts === undefined) throw new Error(`undefined rule ${name}`); + this.active.add(key); + const out = this.alts(alts, pos); + this.active.delete(key); + this.memo.set(key, out); + return out; + } + + private alts(alts: Seq[], pos: number): Set { + const out = new Set(); + for (const seq of alts) for (const end of this.seq(seq, pos)) out.add(end); + return out; + } + + private seq(seq: Seq, pos: number): Set { + let current = new Set([pos]); + for (const item of seq) { + const next = new Set(); + for (const p of current) for (const q of this.item(item, p)) next.add(q); + current = next; + if (current.size === 0) break; + } + return current; + } + + private item(item: Item, pos: number): Set { + const out = new Set(); + // Positions reached at a repetition count >= min: everything past + // them is already accounted for, so a repeat of one stops the walk. + const settled = new Set(); + let frontier = new Set([pos]); + if (item.min === 0) { + out.add(pos); + settled.add(pos); + } + for (let reps = 1; reps <= item.max && frontier.size > 0; reps += 1) { + const next = new Set(); + for (const p of frontier) for (const q of this.elem(item.elem, p)) next.add(q); + if (reps < item.min) { + frontier = next; + continue; + } + const fresh = new Set(); + for (const q of next) { + if (settled.has(q)) continue; + settled.add(q); + out.add(q); + fresh.add(q); + } + frontier = fresh; + } + return out; + } + + private elem(elem: Elem, pos: number): Set { + switch (elem.kind) { + case "lit": + return this.input.startsWith(elem.text, pos) + ? new Set([pos + elem.text.length]) + : new Set(); + case "class": { + if (pos >= this.input.length) return new Set(); + const code = this.input.codePointAt(pos)!; + const inRange = elem.ranges.some(([lo, hi]) => code >= lo && code <= hi); + return inRange !== elem.negate + ? new Set([pos + String.fromCodePoint(code).length]) + : new Set(); + } + case "ref": + return this.rule(elem.name, pos); + case "group": + return this.alts(elem.alts, pos); + } + } +} diff --git a/src/tools/control-marker-guard.test.ts b/src/tools/control-marker-guard.test.ts new file mode 100644 index 00000000..e4a09bcd --- /dev/null +++ b/src/tools/control-marker-guard.test.ts @@ -0,0 +1,197 @@ +import { describe, expect, it } from "vitest"; + +import { + CONTENT_ARGUMENTS, + CONTROL_MARKERS, + GENERIC_CONTROL_MARKER, + describeCorruptedCall, + findControlMarkers, +} from "./control-marker-guard.js"; + +/** The live call that motivated the guard: a thought channel opened mid-call. */ +const LIVE_PATH = + ".}}]thought<|channel>thought---"; + +describe("findControlMarkers", () => { + it("flags the live Gemma 4 failure: channel markers inside a path", () => { + const hits = findControlMarkers({ path: LIVE_PATH }, "os.fs.list"); + expect(hits).toEqual([ + { + path: "path", + marker: "", + index: 4, + excerpt: ".}}]thought<|channel…", + }, + ]); + }); + + it("flags every marker of every family anywhere in a non-content argument", () => { + for (const marker of CONTROL_MARKERS) { + const hits = findControlMarkers( + { command: `echo before ${marker} after` }, + "os.shell.run", + ); + expect(hits, marker).toHaveLength(1); + expect(hits[0]!.marker, marker).toBe(marker); + expect(hits[0]!.index, marker).toBe("echo before ".length); + } + }); + + it("flags the generic <|name|> form whatever the family", () => { + for (const marker of ["<|eot_id|>", "<|assistant|>", "<|end|>"]) { + expect(GENERIC_CONTROL_MARKER.test(marker), marker).toBe(true); + const hits = findControlMarkers({ url: `https://x/${marker}` }); + expect(hits.map((h) => h.marker), marker).toEqual([marker]); + } + }); + + it("walks nested objects and arrays and names the path of each hit", () => { + const hits = findControlMarkers({ + paths: ["clean", "<|im_start|>bad"], + options: { nested: { id: "x" } }, + count: 3, + flag: null, + }); + expect(hits.map((h) => [h.path, h.marker])).toEqual([ + ["paths[1]", "<|im_start|>"], + ["options.nested.id", ""], + ]); + }); + + it("reports one hit per value — the earliest marker — with a windowed excerpt", () => { + const value = `${"x".repeat(20)}middle${"y".repeat(40)}`; + const hits = findControlMarkers({ pattern: value }, "os.fs.grep"); + expect(hits).toHaveLength(1); + expect(hits[0]).toMatchObject({ marker: "", index: 20 }); + expect(hits[0]!.excerpt).toBe("…xxxxxxxxmiddleyy…"); + }); + + it("keeps the excerpt on one line", () => { + const hits = findControlMarkers({ path: "a\n\t<|turn>\r\nb" }); + expect(hits[0]!.excerpt).toBe("a\\n\\t<|turn>\\r\\nb"); + }); + + it("returns nothing for a clean call", () => { + expect( + findControlMarkers( + { path: "src/index.ts", offset: 1, tags: ["a"] }, + "os.fs.read", + ), + ).toEqual([]); + }); + + describe("file content: only a marker at the start of a line", () => { + it("lists the writing tools' content arguments", () => { + expect([...CONTENT_ARGUMENTS.entries()].map(([t, k]) => [t, [...k]])).toEqual([ + ["os.fs.write", ["content"]], + ["os.fs.edit", ["oldString", "newString"]], + ["os.fs.patch", ["patch"]], + ]); + }); + + it("does not flag source that mentions a marker mid-line", () => { + const content = [ + "// the model wraps reasoning in … tags", + "const open = '<|channel>';", + "if (a';", + "const chatml = `<|im_start|>user`;", + ].join("\n"); + expect(findControlMarkers({ path: "a.ts", content }, "os.fs.write")).toEqual( + [], + ); + expect( + findControlMarkers( + { path: "a.ts", oldString: "x y", newString: "x y" }, + "os.fs.edit", + ), + ).toEqual([]); + }); + + it("flags a line that starts with a marker — transcript markup, not content", () => { + const content = "const a = 1;\n<|channel>thought\nI should…\n"; + const hits = findControlMarkers({ path: "a.ts", content }, "os.fs.write"); + expect(hits).toEqual([ + { + path: "content", + marker: "<|channel>", + index: 13, + // 8 chars before the marker, 16 after it, cut marks where cut. + excerpt: "… a = 1;\\n<|channel>thought\\nI should…", + }, + ]); + expect( + findControlMarkers( + { path: "a.ts", oldString: "x", newString: "\nfoo" }, + "os.fs.edit", + ).map((h) => h.path), + ).toEqual(["newString"]); + // The first line is a line start too. + expect( + findControlMarkers({ path: "a.md", content: "hello" }, "os.fs.write"), + ).toHaveLength(1); + }); + + it("reads a unified diff's line start after its +/-/space prefix", () => { + const clean = [ + "--- a/x.ts", + "+++ b/x.ts", + "@@ -1,2 +1,2 @@", + " const keep = '';", + "-const old = 1; // <|turn>", + "+const now = 2; // ", + ].join("\n"); + expect(findControlMarkers({ patch: clean }, "os.fs.patch")).toEqual([]); + const corrupted = `${clean}\n+<|channel>thought\n`; + const hits = findControlMarkers({ patch: corrupted }, "os.fs.patch"); + expect(hits).toHaveLength(1); + expect(hits[0]).toMatchObject({ + path: "patch", + marker: "<|channel>", + index: corrupted.indexOf("<|channel>"), + }); + // A bare line start (no diff prefix) is caught as well. + expect( + findControlMarkers({ patch: `${clean}\n<|channel>x` }, "os.fs.patch"), + ).toHaveLength(1); + }); + + it("applies the line-start rule to the content argument only, not to the path beside it", () => { + const hits = findControlMarkers( + { path: "a<|channel>.ts", content: "mid line" }, + "os.fs.write", + ); + expect(hits.map((h) => h.path)).toEqual(["path"]); + }); + + it("checks the same key anywhere when the tool is not a writing tool", () => { + expect( + findControlMarkers({ content: "mid line" }, "memory.notes.store"), + ).toHaveLength(1); + expect(findControlMarkers({ content: "mid line" })).toHaveLength(1); + }); + }); +}); + +describe("describeCorruptedCall", () => { + it("names the argument, the marker, its offset and the excerpt, and says the call did not run", () => { + const hits = findControlMarkers({ path: LIVE_PATH }, "os.fs.list"); + expect(describeCorruptedCall(hits)).toBe( + 'corrupted tool call: argument `path` contains a model control marker (`` at char 4: ".}}]thought<|channel…"). The call was not run — re-emit it with clean arguments.', + ); + }); + + it("lists several arguments and counts the rest past three", () => { + const hits = findControlMarkers({ + a: "", + b: "", + c: "", + d: "", + e: "", + }); + const message = describeCorruptedCall(hits); + expect(message).toContain("argument `a` contains"); + expect(message).toContain("; argument `c` contains"); + expect(message).not.toContain("argument `d`"); + expect(message).toContain("; and 2 more argument(s). The call was not run"); + }); +}); diff --git a/src/tools/control-marker-guard.ts b/src/tools/control-marker-guard.ts new file mode 100644 index 00000000..8b68460e --- /dev/null +++ b/src/tools/control-marker-guard.ts @@ -0,0 +1,213 @@ +/** + * A tool call whose argument carries the model's own control markup did + * not come out of the model the way it meant it — and it must not run. + * + * Seen live (Gemma 4 31B on llama-server under the GBNF grammar): the + * first call of a turn was + * `os.fs.list {"path": ".}}]thought<|channel>thought---…"}`. + * The model tried to open another thought channel in the middle of the + * call; the grammar admits those bytes only inside a JSON string, so the + * channel markers landed in `path`. The tool ran with the garbage path + * (`ENAMETOOLONG`), the model read "the folder is empty" and overwrote + * the user's input file. `findControlMarkers` is what `executeBatch` + * asks before dispatch; a hit turns the call into an error result the + * model reads on its next step (see `describeCorruptedCall`). + * + * What is NOT flagged: file content that merely mentions a marker. A + * source file may say `` in a comment mid-line, so for the + * content arguments of the writing tools (`CONTENT_ARGUMENTS`) only a + * marker at the START of a line counts — the same line F24's write-time + * check (`fs-content-check.ts`) draws for transcript markup. Every other + * string argument (a path, a command, a pattern, a URL, an id) is + * flagged on any occurrence: none of those has a legitimate reason to + * carry one. + */ + +/** + * Marker text a model emits only to frame its own output — never as an + * argument value. Exact strings; the generic `<|name|>` shape is + * {@link GENERIC_CONTROL_MARKER}. + */ +export const CONTROL_MARKERS: readonly string[] = [ + // Gemma 4: turn framing, the thought channel, tool-call framing. The + // open form is `<|x>`, the close form `` (not the `<|x|>` shape). + "<|channel>", + "", + "<|turn>", + "", + "<|tool_call>", + "", + "<|tool_response>", + "", + // Qwen / ChatML: turn delimiters, native tool-call tags, think tags. + // The think tags are the reasoning prelude's own sentinels — only their + // appearance INSIDE an argument string is wrong, which is all this + // module ever looks at. + "<|im_start|>", + "<|im_end|>", + "", + "", + "", + "", + // Gemma 3: turn delimiters. + "", + "", +]; + +/** + * The generic `<|name|>` form (Llama 3 `<|eot_id|>`, Phi `<|assistant|>`, + * Mistral `<|im_end|>`-alikes): whatever the family, it is a control + * token, not an argument value. + */ +export const GENERIC_CONTROL_MARKER = /<\|[a-z_]+\|>/; + +/** + * Arguments that carry file content, keyed by tool. A marker there is + * suspect only at the start of a line: the file may legitimately quote + * one mid-line, while a line that STARTS with one is transcript markup. + * `oldString` sits with `newString` because it quotes the file as it is + * — a line the file already has mid-line must stay editable. + */ +export const CONTENT_ARGUMENTS: ReadonlyMap> = + new Map([ + ["os.fs.write", new Set(["content"])], + ["os.fs.edit", new Set(["oldString", "newString"])], + ["os.fs.patch", new Set(["patch"])], + ]); + +/** + * Where a file's line starts inside a unified diff: after the one-char + * `+` / `-` / space prefix of a body line. A bare line start still counts + * (the prefix is optional), so a corrupted patch is caught either way. + */ +const PATCH_LINE_PREFIX = "[+ -]?"; + +export interface ControlMarkerHit { + /** Argument path: `path`, `paths[1]`, `edits[0].newString`. */ + readonly path: string; + /** The marker text found (for the generic form, the actual token). */ + readonly marker: string; + /** 0-based char offset of the marker in the argument value. */ + readonly index: number; + /** A one-line window of the value around the marker, `…` where cut. */ + readonly excerpt: string; +} + +const EXCERPT_BEFORE = 8; +const EXCERPT_AFTER = 16; + +function escapeRegExp(text: string): string { + return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +const MARKER_ALTERNATION = [ + ...CONTROL_MARKERS.map(escapeRegExp), + GENERIC_CONTROL_MARKER.source, +].join("|"); + +/** A marker anywhere in the value. */ +const ANYWHERE = new RegExp(MARKER_ALTERNATION); +/** A marker at the start of a line. */ +const LINE_START = new RegExp(`^(?:${MARKER_ALTERNATION})`, "m"); +/** A marker at the start of a line of a unified-diff body. */ +const PATCH_LINE_START = new RegExp( + `^${PATCH_LINE_PREFIX}(?:${MARKER_ALTERNATION})`, + "m", +); + +/** + * Every argument value of `tool`'s call that carries a control marker, + * one hit per value (the earliest marker), in argument order. Empty + * when the call is clean. `tool` selects the content arguments that get + * the line-start rule; without it every string is checked anywhere. + */ +export function findControlMarkers( + args: Record, + tool?: string, +): ControlMarkerHit[] { + const hits: ControlMarkerHit[] = []; + const contentKeys = tool !== undefined ? CONTENT_ARGUMENTS.get(tool) : undefined; + for (const [key, value] of Object.entries(args)) { + const pattern = + contentKeys?.has(key) === true + ? tool === "os.fs.patch" + ? PATCH_LINE_START + : LINE_START + : ANYWHERE; + walk(value, key, pattern, hits); + } + return hits; +} + +function walk( + value: unknown, + path: string, + pattern: RegExp, + hits: ControlMarkerHit[], +): void { + if (typeof value === "string") { + const hit = scan(value, pattern); + if (hit !== null) hits.push({ path, ...hit }); + return; + } + if (Array.isArray(value)) { + value.forEach((item, i) => walk(item, `${path}[${i}]`, pattern, hits)); + return; + } + if (value !== null && typeof value === "object") { + for (const [key, nested] of Object.entries(value)) { + walk(nested, `${path}.${key}`, pattern, hits); + } + } +} + +function scan( + value: string, + pattern: RegExp, +): Omit | null { + const match = pattern.exec(value); + if (match === null) return null; + // The patch pattern's optional prefix is part of the match; the marker + // itself starts where the alternation matched. + const marker = match[0].replace(/^[+ -](?=<)/, ""); + const index = match.index + (match[0].length - marker.length); + return { marker, index, excerpt: excerptAround(value, index, marker) }; +} + +function excerptAround(value: string, index: number, marker: string): string { + const start = Math.max(0, index - EXCERPT_BEFORE); + const end = Math.min(value.length, index + marker.length + EXCERPT_AFTER); + const body = value + .slice(start, end) + .replace(/\n/g, "\\n") + .replace(/\r/g, "\\r") + .replace(/\t/g, "\\t"); + return `${start > 0 ? "…" : ""}${body}${end < value.length ? "…" : ""}`; +} + +/** Hits named in the message before the rest are counted. */ +const DESCRIBED_HITS_MAX = 3; + +/** + * The tool result a corrupted call gets instead of running: which + * argument, which marker, where, and what to do — re-emit the call. The + * value is quoted only as the short excerpt; the transcript already has + * the call in full. + */ +export function describeCorruptedCall( + hits: readonly ControlMarkerHit[], +): string { + const described = hits + .slice(0, DESCRIBED_HITS_MAX) + .map( + (hit) => + `argument \`${hit.path}\` contains a model control marker ` + + `(\`${hit.marker}\` at char ${hit.index}: "${hit.excerpt}")`, + ); + const rest = hits.length - described.length; + const more = rest > 0 ? `; and ${rest} more argument(s)` : ""; + return ( + `corrupted tool call: ${described.join("; ")}${more}. ` + + "The call was not run — re-emit it with clean arguments." + ); +} From c38d14b02ecef4afce31d506bd56b7c2221dcec0 Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 05:32:44 +0300 Subject: [PATCH 2/4] feat(tools): F36 replacing a pre-existing user file is announced and reversible (os.fs.restore) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live, Gemma 4 31B, 2026-09-15: the model's first step wrote `projects.json` over the user's data file without listing the folder — the result said "(replace)" and the model did not react — and after a corrupted listing it wrote a 9-row `sales.csv` over the user's 2,401-row dataset. Both files were named by the request as inputs; both were unrecoverable afterwards. Warn-only throughout: the write still lands. What changes is that the previous content is saved first and the result says what happened. - fs-restore-store: `/restore//` holds the copies (`-`, last 20 per session, files up to 5 MB) and a `manifest.json` with the copy index and the set of paths this session created (a write to a path that did not exist). On disk rather than in `SessionState` because the tools consult it BEFORE a write and a tool sees only its `ToolContext`; keyed by session id, so a resumed session still knows what it created. Built by `registerOsTools` from `stateDir` and shared by write / edit / patch / restore through `FsDangerousToolOptions.restore`; absent (embedders, tests) turns the guard off. - fs-replace-guard: before `os.fs.write` replaces a file that existed and was not created this session, the previous bytes are copied and a line is prepended to the result. Loud — `⚠ replaced the user's file \`sales.csv\` (2,401 lines → 10, header changed); the previous content is saved — \`os.fs.restore {"path":"sales.csv"}\` brings it back` — when the line count dropped by ≥ 80 % (integer: dropped/before ≥ 4/5) OR, for .csv .tsv .json .jsonl .md .txt .yaml .yml .toml .ini, the first line changed, OR the file was over 5 MB (announced, not read or saved). Any other replacement of a pre-existing user file gets the quiet `replaced the user's file \`x\` (N lines → M); previous content saved`. An empty file and a file the agent created are silent. `os.fs.edit` / `os.fs.patch` run the same guard on the shrink rule only (`shrank the user's file …`); a patch that creates a file marks it as the agent's. The "(replace)" wording now carries the counts: `(replace, 2,401 lines → 10)`, `(replace, new file, 10 lines)`. - os.fs.restore { path }: writes the newest saved copy back, approval- gated like a write on the fs ladder, and names the bytes and lines that came back; the copy stays so a second restore still works. Registered as the other os.fs tools are — descriptor (rare), args schema, grammar os-tool list, `approval_gated` + `FS_WRITE_CATEGORIES`, builder role, the stable-prefix solo list, the write-tool sets of the loop detector and the fusion worker report — and the `os.fs.write` descriptor says a replaced pre-existing file can be brought back. - fs-edit-diff / fs-patch-preview: the diff renderer and the dry-run half split out so both tools stay under the 300-line cap. --- grammars/tool-call.gbnf | 2 +- src/agent/loop-detector.ts | 1 + src/agent/tool-resource-class.test.ts | 1 + src/agent/tool-resource-class.ts | 3 + src/llm/grammar/build-grammar.test.ts | 19 + .../openai/strict-tool-schema.test.ts | 7 +- src/prompt/default-tool-args-schemas.ts | 1 + src/prompt/default-tool-descriptors-a.ts | 10 +- src/prompt/stable-prefix.ts | 4 +- src/tools/fusion/worker-result.ts | 1 + src/tools/os/fs-edit-diff.ts | 100 ++++ src/tools/os/fs-edit.ts | 154 ++--- src/tools/os/fs-patch-preview.ts | 129 +++++ src/tools/os/fs-patch.ts | 172 ++---- src/tools/os/fs-replace-guard.test.ts | 545 ++++++++++++++++++ src/tools/os/fs-replace-guard.ts | 253 ++++++++ src/tools/os/fs-require-approval.ts | 9 + src/tools/os/fs-restore-store.ts | 209 +++++++ src/tools/os/fs-restore.ts | 94 +++ src/tools/os/fs-write.ts | 111 +++- src/tools/os/index.ts | 36 +- src/tools/os/os-tools.test.ts | 1 + src/tools/tool-roles.test.ts | 2 + src/tools/tool-roles.ts | 3 + 24 files changed, 1598 insertions(+), 269 deletions(-) create mode 100644 src/tools/os/fs-edit-diff.ts create mode 100644 src/tools/os/fs-patch-preview.ts create mode 100644 src/tools/os/fs-replace-guard.test.ts create mode 100644 src/tools/os/fs-replace-guard.ts create mode 100644 src/tools/os/fs-restore-store.ts create mode 100644 src/tools/os/fs-restore.ts diff --git a/grammars/tool-call.gbnf b/grammars/tool-call.gbnf index c749222e..771eceb8 100644 --- a/grammars/tool-call.gbnf +++ b/grammars/tool-call.gbnf @@ -7,7 +7,7 @@ tool-call ::= "{" ws "\"tool\"" ws ":" ws tool-name ws "," ws "\"args\"" ws ":" tool-call-array ::= "[" ws tool-call ( ws "," ws tool-call ){0,15} ws "]" tool-name ::= browser-tool | os-tool | discovery-tool | memory-tool | tasks-tool | vision-tool | fusion-tool | verify-tool | mcp-native-tool | mcp-server-tool | "\"reply\"" | "\"finish\"" browser-tool ::= "\"browser." ( "navigate" | "click" | "type" | "read_aria" | "search" | "tabs" | "scroll" ) "\"" -os-tool ::= "\"os." ( "shell.run" | "fs.read" | "fs.read_document" | "fs.write" | "fs.trash" | "fs.list" | "fs.grep" | "fs.glob" | "fs.locate_project" | "fs.edit" | "fs.hash" | "fs.diff" | "fs.patch" | "fs.watch" | "fs.archive.list" | "fs.archive.read_entry" | "fs.archive.extract" | "http.request" | "web.search" | "web.fetch" | "git.status" | "git.log" | "git.diff" | "git.show" | "git.blame" | "git.branch" | "git.init" | "git.add" | "git.commit" | "git.checkout" | "git.clone" | "git.remote" | "git.fetch" | "git.pull" | "git.push" | "proc.list" | "proc.kill" | "clipboard.read" | "clipboard.write" | "window.list" | "window.focus" | "notify" | "email.inbox" | "email.send" ) "\"" +os-tool ::= "\"os." ( "shell.run" | "fs.read" | "fs.read_document" | "fs.write" | "fs.trash" | "fs.list" | "fs.grep" | "fs.glob" | "fs.locate_project" | "fs.edit" | "fs.hash" | "fs.diff" | "fs.patch" | "fs.restore" | "fs.watch" | "fs.archive.list" | "fs.archive.read_entry" | "fs.archive.extract" | "http.request" | "web.search" | "web.fetch" | "git.status" | "git.log" | "git.diff" | "git.show" | "git.blame" | "git.branch" | "git.init" | "git.add" | "git.commit" | "git.checkout" | "git.clone" | "git.remote" | "git.fetch" | "git.pull" | "git.push" | "proc.list" | "proc.kill" | "clipboard.read" | "clipboard.write" | "window.list" | "window.focus" | "notify" | "email.inbox" | "email.send" ) "\"" discovery-tool ::= "\"skill." ( "view" | "run_script" ) "\"" | "\"tool.view\"" memory-tool ::= "\"memory." ( "profile.set" | "profile.remove" | "profile.list" | "profile.history" | "notes.store" | "notes.recall" | "notes.forget" | "lessons.recall" | "procedures.recall" ) "\"" tasks-tool ::= "\"tasks." ( "schedule" | "cron" | "list" | "cancel" | "show" ) "\"" diff --git a/src/agent/loop-detector.ts b/src/agent/loop-detector.ts index 62e08b9e..bebae419 100644 --- a/src/agent/loop-detector.ts +++ b/src/agent/loop-detector.ts @@ -751,6 +751,7 @@ const WRITE_TOOLS: ReadonlySet = new Set([ "os.fs.write", "os.fs.edit", "os.fs.patch", + "os.fs.restore", ]); /** A write, edit or patch that landed — the reset event for the outcome-repeat detector. */ diff --git a/src/agent/tool-resource-class.test.ts b/src/agent/tool-resource-class.test.ts index 4889367b..75c83511 100644 --- a/src/agent/tool-resource-class.test.ts +++ b/src/agent/tool-resource-class.test.ts @@ -127,6 +127,7 @@ describe("tool-resource-class", () => { "os.fs.edit", "os.fs.trash", "os.fs.patch", + "os.fs.restore", "os.fs.archive.extract", "os.git.init", "os.git.add", diff --git a/src/agent/tool-resource-class.ts b/src/agent/tool-resource-class.ts index bba5d4e6..2afa188e 100644 --- a/src/agent/tool-resource-class.ts +++ b/src/agent/tool-resource-class.ts @@ -91,6 +91,7 @@ const TOOL_RESOURCE_CLASS: Record = { "os.fs.edit": "approval_gated", "os.fs.trash": "approval_gated", "os.fs.patch": "approval_gated", + "os.fs.restore": "approval_gated", "os.fs.archive.extract": "approval_gated", // os.git.* — read-only shell-outs @@ -297,6 +298,8 @@ const APPROVAL_CATEGORIES_BY_TOOL: Record = "os.fs.write": FS_WRITE_CATEGORIES, "os.fs.edit": FS_WRITE_CATEGORIES, "os.fs.patch": FS_WRITE_CATEGORIES, + // Puts a saved copy back: a write in every sense. + "os.fs.restore": FS_WRITE_CATEGORIES, "os.fs.trash": ["fs_trash", "trust_config", "other"], "os.fs.archive.extract": ["fs_write_home", "other"], // Local git writes ride the fs funnel against the repository root. diff --git a/src/llm/grammar/build-grammar.test.ts b/src/llm/grammar/build-grammar.test.ts index 2286216a..661d4c00 100644 --- a/src/llm/grammar/build-grammar.test.ts +++ b/src/llm/grammar/build-grammar.test.ts @@ -262,6 +262,25 @@ describe("os-tool names the local-model grammar admits", () => { } }); + it("includes os.fs.restore — the undo the replace guard's note tells the model to call", () => { + const { readFileSync } = require("node:fs") as typeof import("node:fs"); + const { resolve } = require("node:path") as typeof import("node:path"); + const grammar = readFileSync( + resolve(__dirname, "../../../grammars/tool-call.gbnf"), + "utf8", + ); + const osToolLine = + grammar.split("\n").find((l) => l.startsWith("os-tool ::=")) ?? ""; + expect(osToolLine).toContain('"fs.restore"'); + expect( + gbnfAccepts( + grammar, + "root", + '[{"tool":"os.fs.restore","args":{"path":"sales.csv"}}]', + ), + ).toBe(true); + }); + it("includes the git WRITE tools, not just the read half", () => { // Same class of bug as the missing `fusion.delegate`: the local-first // git tools were registered and described, and the grammar still only diff --git a/src/llm/provider/openai/strict-tool-schema.test.ts b/src/llm/provider/openai/strict-tool-schema.test.ts index eb3f79d9..0b619f74 100644 --- a/src/llm/provider/openai/strict-tool-schema.test.ts +++ b/src/llm/provider/openai/strict-tool-schema.test.ts @@ -556,10 +556,10 @@ describe("toStrictJsonSchema", () => { "vision.describe", ]); expect(converted).toBe(DEFAULT_TOOL_NAMES.length - refused.length); - // 84 registered schemas, 78 of them strict. Pinned as a number so + // 85 registered schemas, 79 of them strict. Pinned as a number so // the sample cannot quietly shrink. - expect(DEFAULT_TOOL_NAMES.length).toBe(84); - expect(converted).toBe(78); + expect(DEFAULT_TOOL_NAMES.length).toBe(85); + expect(converted).toBe(79); }); /** @@ -625,6 +625,7 @@ const DEFAULT_TOOL_NAMES: readonly string[] = [ "os.fs.hash", "os.fs.diff", "os.fs.patch", + "os.fs.restore", "os.fs.watch", "os.git.status", "os.git.log", diff --git a/src/prompt/default-tool-args-schemas.ts b/src/prompt/default-tool-args-schemas.ts index b65f3b20..baa1eb26 100644 --- a/src/prompt/default-tool-args-schemas.ts +++ b/src/prompt/default-tool-args-schemas.ts @@ -153,6 +153,7 @@ const DEFAULT_TOOL_ARGS_SCHEMAS: ReadonlyMap = new Map< ["path", "content"], ), ], + ["os.fs.restore", obj({ path: stringSchema }, ["path"])], ["os.fs.trash", obj({ paths: stringArraySchema }, ["paths"])], [ "os.fs.list", diff --git a/src/prompt/default-tool-descriptors-a.ts b/src/prompt/default-tool-descriptors-a.ts index 52657ec2..7cda9bb7 100644 --- a/src/prompt/default-tool-descriptors-a.ts +++ b/src/prompt/default-tool-descriptors-a.ts @@ -56,9 +56,17 @@ export const DEFAULT_TOOL_DESCRIPTORS_A: readonly ToolDescriptor[] = [ }, { name: "os.fs.write", - summary: "Write or append to a file (may require approval).", + summary: + "Write or append to a file (may require approval). The result says when it replaced a pre-existing file and with what line counts; a replaced pre-existing file can be brought back with os.fs.restore.", argsSchema: `{ path: string, content: string, mode?: "replace" | "append" }`, }, + { + name: "os.fs.restore", + summary: + "Bring back the previous content of a file that os.fs.write / os.fs.edit / os.fs.patch replaced or shrank this session — the result of that call said it was saved (may require approval).", + argsSchema: "{ path: string }", + tier: "rare", + }, { name: "os.fs.trash", summary: diff --git a/src/prompt/stable-prefix.ts b/src/prompt/stable-prefix.ts index 852027a7..49019648 100644 --- a/src/prompt/stable-prefix.ts +++ b/src/prompt/stable-prefix.ts @@ -316,7 +316,7 @@ export function buildStablePrefix(input: StablePrefixInput): string { ? [ `Call tools now, through the native function-calling interface (the \`tools\` your API request carries) — do NOT write tool-call JSON as text. The \`### tools\` catalog above is reference documentation for those same tools (tiers, examples, \`tool.view\`). For the final user-facing answer call \`reply\`, or answer in plain text.`, `PARALLEL: when you need multiple INDEPENDENT actions (e.g. read 3 different files, run 2 globs, look up 4 git logs), emit up to ${maxParallelToolCalls} tool calls in the SAME response — they run in parallel and cut wall time by ~Nx.`, - `Emit a single tool call (no others alongside) when: it is \`reply\`/\`finish\`, may need approval (\`os.shell.run\`, \`os.fs.write\`, \`os.fs.edit\`, \`os.fs.trash\`, \`os.fs.patch\`, \`os.fs.archive.extract\`, \`os.proc.kill\`, \`os.http.request\`, \`skill.run_script\`), or its args depend on a previous call's result.`, + `Emit a single tool call (no others alongside) when: it is \`reply\`/\`finish\`, may need approval (\`os.shell.run\`, \`os.fs.write\`, \`os.fs.edit\`, \`os.fs.trash\`, \`os.fs.patch\`, \`os.fs.restore\`, \`os.fs.archive.extract\`, \`os.proc.kill\`, \`os.http.request\`, \`skill.run_script\`), or its args depend on a previous call's result.`, ] : [ `Emit a JSON ARRAY of tool calls now. Always start with \`[\` and end with \`]\`, even for a single call. Use \`reply\` for natural-language answers to the user.`, @@ -324,7 +324,7 @@ export function buildStablePrefix(input: StablePrefixInput): string { ` - one call: [{"tool":"os.fs.read","args":{"path":"a.ts"}}]`, ` - parallel batch: [{"tool":"os.fs.read","args":{"path":"a.csv"}},{"tool":"os.fs.read","args":{"path":"b.csv"}},{"tool":"os.fs.read","args":{"path":"c.csv"}}]`, ` - reply: [{"tool":"reply","args":{"text":"..."}}]`, - `Keep a call solo (length-1 array) when: it is \`reply\`/\`finish\`, may need approval (\`os.shell.run\`, \`os.fs.write\`, \`os.fs.edit\`, \`os.fs.trash\`, \`os.fs.patch\`, \`os.fs.archive.extract\`, \`os.proc.kill\`, \`os.http.request\`, \`skill.run_script\`), or its args depend on a previous call's result.`, + `Keep a call solo (length-1 array) when: it is \`reply\`/\`finish\`, may need approval (\`os.shell.run\`, \`os.fs.write\`, \`os.fs.edit\`, \`os.fs.trash\`, \`os.fs.patch\`, \`os.fs.restore\`, \`os.fs.archive.extract\`, \`os.proc.kill\`, \`os.http.request\`, \`skill.run_script\`), or its args depend on a previous call's result.`, ]), ``, ].join("\n"); diff --git a/src/tools/fusion/worker-result.ts b/src/tools/fusion/worker-result.ts index 7aeeaa9b..09554660 100644 --- a/src/tools/fusion/worker-result.ts +++ b/src/tools/fusion/worker-result.ts @@ -58,6 +58,7 @@ export const FILE_WRITING_TOOLS: ReadonlySet = new Set([ "os.fs.write", "os.fs.edit", "os.fs.patch", + "os.fs.restore", ]); /** diff --git a/src/tools/os/fs-edit-diff.ts b/src/tools/os/fs-edit-diff.ts new file mode 100644 index 00000000..d8bc92ea --- /dev/null +++ b/src/tools/os/fs-edit-diff.ts @@ -0,0 +1,100 @@ +const DIFF_MAX_LINES = 40; +const PREVIEW_MAX_LEN = 800; + +/** + * Minimal unified diff, capped at DIFF_MAX_LINES total lines. We avoid a + * full LCS algorithm: the edit is a single substring replacement, so the + * diff is well approximated by a simple before/after line listing around + * the changed region. + */ +export function renderUnifiedDiff( + before: string, + after: string, + path: string, +): string { + const beforeLines = before.split(/\r?\n/); + const afterLines = after.split(/\r?\n/); + const firstDiff = findFirstDiffLine(beforeLines, afterLines); + if (firstDiff === -1) return ""; + const contextBefore = 2; + const contextAfter = 2; + const head = Math.max(0, firstDiff - contextBefore); + const lastDiffBefore = findLastDiffLine(beforeLines, afterLines); + const lastDiffAfter = findLastDiffLineFromEnd(beforeLines, afterLines); + const tailBefore = Math.min( + beforeLines.length - 1, + lastDiffBefore + contextAfter, + ); + + const segments: string[] = []; + segments.push(`--- a/${path}`); + segments.push(`+++ b/${path}`); + for ( + let i = head; + i <= Math.min(beforeLines.length - 1, tailBefore) && i < firstDiff; + i++ + ) { + segments.push(` ${beforeLines[i]}`); + } + for (let i = firstDiff; i <= lastDiffBefore; i++) { + segments.push(`-${beforeLines[i] ?? ""}`); + } + for (let i = firstDiff; i <= lastDiffAfter; i++) { + segments.push(`+${afterLines[i] ?? ""}`); + } + for ( + let i = Math.max(lastDiffBefore + 1, firstDiff); + i <= tailBefore && i < beforeLines.length; + i++ + ) { + segments.push(` ${beforeLines[i]}`); + } + if (segments.length > DIFF_MAX_LINES + 2) { + return ( + segments.slice(0, DIFF_MAX_LINES + 2).join("\n") + "\n… [diff truncated]" + ); + } + return segments.join("\n"); +} + +function findFirstDiffLine(before: string[], after: string[]): number { + const limit = Math.min(before.length, after.length); + for (let i = 0; i < limit; i++) { + if (before[i] !== after[i]) return i; + } + if (before.length !== after.length) return limit; + return -1; +} + +function findLastDiffLine(before: string[], after: string[]): number { + // Returns the last index in `before` that differs from `after` (walking + // from the end). Treats missing indices as different. + const len = Math.max(before.length, after.length); + for ( + let i = before.length - 1, j = after.length - 1; + i >= 0 && j >= 0; + i--, j-- + ) { + if (before[i] !== after[j]) return i; + } + if (before.length < after.length) return -1; + return before.length - 1 - (len - Math.max(before.length, after.length)); +} + +function findLastDiffLineFromEnd(before: string[], after: string[]): number { + for ( + let i = after.length - 1, j = before.length - 1; + i >= 0 && j >= 0; + i--, j-- + ) { + if (after[i] !== before[j]) return i; + } + if (after.length > before.length) return after.length - 1; + return -1; +} + +/** The approval-prompt preview: the diff, clipped. */ +export function clampDiffPreview(text: string): string { + if (text.length <= PREVIEW_MAX_LEN) return text; + return text.slice(0, PREVIEW_MAX_LEN - 15) + "\n… [truncated]"; +} diff --git a/src/tools/os/fs-edit.ts b/src/tools/os/fs-edit.ts index 7d7cefe6..719946a9 100644 --- a/src/tools/os/fs-edit.ts +++ b/src/tools/os/fs-edit.ts @@ -4,16 +4,19 @@ import { randomBytes } from "node:crypto"; import { compressToolResult } from "../../compressor/result-compressor.js"; import { resolveUserPath } from "./expand-home.js"; import { checkChangedFile } from "./fs-content-check.js"; +import { clampDiffPreview, renderUnifiedDiff } from "./fs-edit-diff.js"; import { withParseWarning } from "./fs-parse-check.js"; +import { + guardReplacedFile, + priorFromText, + withReplaceNotes, +} from "./fs-replace-guard.js"; import { requireFsApproval, type FsDangerousToolOptions, } from "./fs-require-approval.js"; import type { ToolDefinition } from "../tool-registry.js"; -const DIFF_MAX_LINES = 40; -const PREVIEW_MAX_LEN = 800; - interface EditArgs { path: string; oldString: string; @@ -53,7 +56,7 @@ export function buildOsFsEditTool( ? replaceAll(original, args.oldString, args.newString) : replaceOnce(original, args.oldString, args.newString); const diff = renderUnifiedDiff(original, updated, absolute); - const preview = clamp(diff, PREVIEW_MAX_LEN); + const preview = clampDiffPreview(diff); await requireFsApproval( options, { @@ -72,6 +75,20 @@ export function buildOsFsEditTool( await atomicWrite(absolute, updated); + // An edit that cut a user's file down by 80 % or more — the blind + // `replaceAll` that ate a dataset — saves what it replaced and says + // so (`fs-replace-guard.ts`); an ordinary edit is silent here. + const guard = await guardReplacedFile({ + store: options.restore, + sessionId: ctx.sessionId, + absolute, + display: args.path, + tool: "os.fs.edit", + change: "shrink", + prior: priorFromText(original), + after: updated, + }); + const replacedOccurrences = args.replaceAll ? occurrences : 1; // Judged after the write landed, against the file as it was before: // an edit that turns a parsing file into a broken one — the classic @@ -86,20 +103,24 @@ export function buildOsFsEditTool( replacedOccurrences, }); - return withParseWarning( - compressToolResult({ - tool: "os.fs.edit", - status: "ok", - output: diff.length > 0 ? diff : `(no textual diff — file rewritten)`, - details: { - path: absolute, - replacedOccurrences, - replaceAll: args.replaceAll, - sizeBefore: Buffer.byteLength(original, "utf8"), - sizeAfter: Buffer.byteLength(updated, "utf8"), - }, - }), - parseWarning, + return withReplaceNotes( + withParseWarning( + compressToolResult({ + tool: "os.fs.edit", + status: "ok", + output: + diff.length > 0 ? diff : `(no textual diff — file rewritten)`, + details: { + path: absolute, + replacedOccurrences, + replaceAll: args.replaceAll, + sizeBefore: Buffer.byteLength(original, "utf8"), + sizeAfter: Buffer.byteLength(updated, "utf8"), + }, + }), + parseWarning, + ), + [guard], ); }, }; @@ -191,100 +212,3 @@ async function atomicWrite(target: string, content: string): Promise { throw err; } } - -/** - * Minimal unified diff, capped at DIFF_MAX_LINES total lines. We avoid a - * full LCS algorithm: the edit is a single substring replacement, so the - * diff is well approximated by a simple before/after line listing around - * the changed region. - */ -function renderUnifiedDiff( - before: string, - after: string, - path: string, -): string { - const beforeLines = before.split(/\r?\n/); - const afterLines = after.split(/\r?\n/); - const firstDiff = findFirstDiffLine(beforeLines, afterLines); - if (firstDiff === -1) return ""; - const contextBefore = 2; - const contextAfter = 2; - const head = Math.max(0, firstDiff - contextBefore); - const lastDiffBefore = findLastDiffLine(beforeLines, afterLines); - const lastDiffAfter = findLastDiffLineFromEnd(beforeLines, afterLines); - const tailBefore = Math.min( - beforeLines.length - 1, - lastDiffBefore + contextAfter, - ); - - const segments: string[] = []; - segments.push(`--- a/${path}`); - segments.push(`+++ b/${path}`); - for ( - let i = head; - i <= Math.min(beforeLines.length - 1, tailBefore) && i < firstDiff; - i++ - ) { - segments.push(` ${beforeLines[i]}`); - } - for (let i = firstDiff; i <= lastDiffBefore; i++) { - segments.push(`-${beforeLines[i] ?? ""}`); - } - for (let i = firstDiff; i <= lastDiffAfter; i++) { - segments.push(`+${afterLines[i] ?? ""}`); - } - for ( - let i = Math.max(lastDiffBefore + 1, firstDiff); - i <= tailBefore && i < beforeLines.length; - i++ - ) { - segments.push(` ${beforeLines[i]}`); - } - if (segments.length > DIFF_MAX_LINES + 2) { - return ( - segments.slice(0, DIFF_MAX_LINES + 2).join("\n") + "\n… [diff truncated]" - ); - } - return segments.join("\n"); -} - -function findFirstDiffLine(before: string[], after: string[]): number { - const limit = Math.min(before.length, after.length); - for (let i = 0; i < limit; i++) { - if (before[i] !== after[i]) return i; - } - if (before.length !== after.length) return limit; - return -1; -} - -function findLastDiffLine(before: string[], after: string[]): number { - // Returns the last index in `before` that differs from `after` (walking - // from the end). Treats missing indices as different. - const len = Math.max(before.length, after.length); - for ( - let i = before.length - 1, j = after.length - 1; - i >= 0 && j >= 0; - i--, j-- - ) { - if (before[i] !== after[j]) return i; - } - if (before.length < after.length) return -1; - return before.length - 1 - (len - Math.max(before.length, after.length)); -} - -function findLastDiffLineFromEnd(before: string[], after: string[]): number { - for ( - let i = after.length - 1, j = before.length - 1; - i >= 0 && j >= 0; - i--, j-- - ) { - if (after[i] !== before[j]) return i; - } - if (after.length > before.length) return after.length - 1; - return -1; -} - -function clamp(text: string, maxLen: number): string { - if (text.length <= maxLen) return text; - return text.slice(0, maxLen - 15) + "\n… [truncated]"; -} diff --git a/src/tools/os/fs-patch-preview.ts b/src/tools/os/fs-patch-preview.ts new file mode 100644 index 00000000..24f3976d --- /dev/null +++ b/src/tools/os/fs-patch-preview.ts @@ -0,0 +1,129 @@ +import { readFile, stat } from "node:fs/promises"; +import { isAbsolute, resolve } from "node:path"; +import { applyPatch } from "diff"; +import type { StructuredPatch } from "diff"; + +/** + * Per-file outcome of applying a patch. Tracked per target so the tool can + * report "files 1/3 applied, 2/3 rejected" instead of silently dropping + * bad hunks. + */ +export interface FileOutcome { + path: string; + absolute: string; + applied: boolean; + reason?: string; + addedLines: number; + removedLines: number; +} + +export interface PreviewOutcome extends FileOutcome { + /** Raw source content at the time of preview (used again when we decide to write). */ + originalContent?: string; + /** Whether the target was on disk at preview time — a patch may create a file. */ + existed: boolean; +} + +/** Try one file of the patch against the disk without writing anything. */ +export async function dryRunFile( + hunkFile: StructuredPatch, + rootDir: string, + fuzzFactor: number, + stripComponents: number, +): Promise { + const targetRel = pickTargetPath(hunkFile, stripComponents); + const abs = isAbsolute(targetRel) ? targetRel : resolve(rootDir, targetRel); + const counts = countPatchLines(hunkFile); + + let originalContent = ""; + let existed = false; + try { + const info = await stat(abs); + if (!info.isFile()) { + return { + path: targetRel, + absolute: abs, + applied: false, + reason: "target is not a regular file", + addedLines: counts.added, + removedLines: counts.removed, + existed: true, + }; + } + originalContent = await readFile(abs, "utf8"); + existed = true; + } catch (err) { + const isMissing = (err as NodeJS.ErrnoException).code === "ENOENT"; + if (!isMissing) { + return { + path: targetRel, + absolute: abs, + applied: false, + reason: `cannot read target: ${(err as Error).message}`, + addedLines: counts.added, + removedLines: counts.removed, + existed: false, + }; + } + // Missing target is fine only if the patch creates the file from + // scratch (empty original). Let applyPatch decide. + } + + const patched = applyPatch(originalContent, hunkFile, { fuzzFactor }); + if (patched === false) { + return { + path: targetRel, + absolute: abs, + applied: false, + reason: `hunk(s) did not match (fuzzFactor=${fuzzFactor})`, + addedLines: counts.added, + removedLines: counts.removed, + originalContent, + existed, + }; + } + return { + path: targetRel, + absolute: abs, + applied: true, + addedLines: counts.added, + removedLines: counts.removed, + originalContent, + existed, + }; +} + +function pickTargetPath( + hunkFile: StructuredPatch, + stripComponents: number, +): string { + // Prefer the "new" side; fall back to the "old" side for pure deletions. + const raw = + typeof hunkFile.newFileName === "string" && + hunkFile.newFileName !== "/dev/null" + ? hunkFile.newFileName + : (hunkFile.oldFileName ?? ""); + return stripPathComponents(raw, stripComponents); +} + +function stripPathComponents(p: string, n: number): string { + if (n <= 0) return p; + // Drop `a/`, `b/` prefixes that `git diff` emits. + const parts = p.split(/[\\/]/); + return parts.slice(Math.min(n, parts.length - 1)).join("/"); +} + +function countPatchLines(hunkFile: StructuredPatch): { + added: number; + removed: number; +} { + let added = 0; + let removed = 0; + for (const hunk of hunkFile.hunks) { + for (const line of hunk.lines) { + if (line.startsWith("+")) added++; + else if (line.startsWith("-")) removed++; + } + } + return { added, removed }; +} diff --git a/src/tools/os/fs-patch.ts b/src/tools/os/fs-patch.ts index b7eebd9e..10872140 100644 --- a/src/tools/os/fs-patch.ts +++ b/src/tools/os/fs-patch.ts @@ -1,30 +1,25 @@ -import { readFile, stat, writeFile } from "node:fs/promises"; -import { isAbsolute, resolve, dirname, basename } from "node:path"; +import { readFile, writeFile } from "node:fs/promises"; +import { dirname, basename } from "node:path"; import { applyPatch, parsePatch } from "diff"; import type { StructuredPatch } from "diff"; import { compressToolResult } from "../../compressor/result-compressor.js"; import { resolveUserPath } from "./expand-home.js"; import { checkChangedFile } from "./fs-content-check.js"; import { withParseWarning } from "./fs-parse-check.js"; +import { dryRunFile, type PreviewOutcome } from "./fs-patch-preview.js"; +import { + guardReplacedFile, + priorFromText, + withReplaceNotes, + type ReplaceGuardOutcome, +} from "./fs-replace-guard.js"; import { requireFsApproval, type FsDangerousToolOptions, } from "./fs-require-approval.js"; import type { ToolDefinition } from "../tool-registry.js"; -/** - * Per-file outcome of applying a patch. Tracked per target so the tool can - * report "files 1/3 applied, 2/3 rejected" instead of silently dropping - * bad hunks. - */ -interface FileOutcome { - path: string; - absolute: string; - applied: boolean; - reason?: string; - addedLines: number; - removedLines: number; -} +export type { FileOutcome, PreviewOutcome } from "./fs-patch-preview.js"; interface PatchArgs { patch: string; @@ -90,6 +85,7 @@ export function buildOsFsPatchTool( } const parseWarnings: string[] = []; + const guards: ReplaceGuardOutcome[] = []; for (let i = 0; i < parsed.length; i++) { const hunkFile = parsed[i]; const outcome = previews[i]; @@ -105,6 +101,9 @@ export function buildOsFsPatchTool( ); } await writeFile(outcome.absolute, patched, "utf8"); + guards.push( + await guardAfterPatch(options, ctx.sessionId, outcome, patched), + ); const warning = parseWarningAfterPatch( outcome, patched, @@ -113,14 +112,49 @@ export function buildOsFsPatchTool( if (warning !== null) parseWarnings.push(warning); } - return withParseWarning( - buildResult(previews, "applied"), - parseWarnings.length > 0 ? parseWarnings.join("\n") : null, + return withReplaceNotes( + withParseWarning( + buildResult(previews, "applied"), + parseWarnings.length > 0 ? parseWarnings.join("\n") : null, + ), + guards, ); }, }; } +/** + * A patch that created the file marks it as the agent's; one that cut a + * user's file down by 80 % or more saves what it replaced and says so + * (`fs-replace-guard.ts`). The absolute path is what the note spells, + * since a patch path is relative to `rootDir`, not the working dir. + */ +async function guardAfterPatch( + options: FsDangerousToolOptions, + sessionId: string, + outcome: PreviewOutcome, + patched: string, +): Promise { + if (!outcome.existed) { + try { + await options.restore?.recordCreated(sessionId, outcome.absolute); + } catch { + // Best effort: the patch landed either way. + } + return { note: null }; + } + return guardReplacedFile({ + store: options.restore, + sessionId, + absolute: outcome.absolute, + display: outcome.absolute, + tool: "os.fs.patch", + change: "shrink", + prior: priorFromText(outcome.originalContent ?? ""), + after: patched, + }); +} + /** * The warnings for one applied file (see `fs-parse-check.ts` and * `fs-content-check.ts`), or null. A file the patch emptied is a @@ -200,108 +234,6 @@ function safeParsePatch(source: string): StructuredPatch[] { } } -interface PreviewOutcome extends FileOutcome { - /** Raw source content at the time of preview (used again when we decide to write). */ - originalContent?: string; -} - -async function dryRunFile( - hunkFile: StructuredPatch, - rootDir: string, - fuzzFactor: number, - stripComponents: number, -): Promise { - const targetRel = pickTargetPath(hunkFile, stripComponents); - const abs = isAbsolute(targetRel) ? targetRel : resolve(rootDir, targetRel); - const counts = countLines(hunkFile); - - let originalContent = ""; - try { - const info = await stat(abs); - if (!info.isFile()) { - return { - path: targetRel, - absolute: abs, - applied: false, - reason: "target is not a regular file", - addedLines: counts.added, - removedLines: counts.removed, - }; - } - originalContent = await readFile(abs, "utf8"); - } catch (err) { - const isMissing = (err as NodeJS.ErrnoException).code === "ENOENT"; - if (!isMissing) { - return { - path: targetRel, - absolute: abs, - applied: false, - reason: `cannot read target: ${(err as Error).message}`, - addedLines: counts.added, - removedLines: counts.removed, - }; - } - // Missing target is fine only if the patch creates the file from - // scratch (empty original). Let applyPatch decide. - } - - const patched = applyPatch(originalContent, hunkFile, { fuzzFactor }); - if (patched === false) { - return { - path: targetRel, - absolute: abs, - applied: false, - reason: `hunk(s) did not match (fuzzFactor=${fuzzFactor})`, - addedLines: counts.added, - removedLines: counts.removed, - originalContent, - }; - } - return { - path: targetRel, - absolute: abs, - applied: true, - addedLines: counts.added, - removedLines: counts.removed, - originalContent, - }; -} - -function pickTargetPath( - hunkFile: StructuredPatch, - stripComponents: number, -): string { - // Prefer the "new" side; fall back to the "old" side for pure deletions. - const raw = - typeof hunkFile.newFileName === "string" && - hunkFile.newFileName !== "/dev/null" - ? hunkFile.newFileName - : (hunkFile.oldFileName ?? ""); - return stripPathComponents(raw, stripComponents); -} - -function stripPathComponents(p: string, n: number): string { - if (n <= 0) return p; - // Drop `a/`, `b/` prefixes that `git diff` emits. - const parts = p.split(/[\\/]/); - return parts.slice(Math.min(n, parts.length - 1)).join("/"); -} - -function countLines(hunkFile: StructuredPatch): { - added: number; - removed: number; -} { - let added = 0; - let removed = 0; - for (const hunk of hunkFile.hunks) { - for (const line of hunk.lines) { - if (line.startsWith("+")) added++; - else if (line.startsWith("-")) removed++; - } - } - return { added, removed }; -} - function buildResult( previews: PreviewOutcome[], mode: "dry-run" | "applied" | "apply-refused", diff --git a/src/tools/os/fs-replace-guard.test.ts b/src/tools/os/fs-replace-guard.test.ts new file mode 100644 index 00000000..3d129e2b --- /dev/null +++ b/src/tools/os/fs-replace-guard.test.ts @@ -0,0 +1,545 @@ +import { existsSync } from "node:fs"; +import { mkdtemp, readFile, readdir, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { createPatch } from "diff"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + ApprovalGate, + type ApprovalRequest, +} from "../../approval/approval-gate.js"; +import { ToolRegistry, type ToolContext } from "../tool-registry.js"; +import { buildOsFsEditTool } from "./fs-edit.js"; +import { buildOsFsPatchTool } from "./fs-patch.js"; +import { countLines, firstLine, isShrink } from "./fs-replace-guard.js"; +import { buildOsFsRestoreTool } from "./fs-restore.js"; +import { + FileRestoreStore, + RESTORE_COPY_CAP, + RESTORE_MAX_BYTES, +} from "./fs-restore-store.js"; +import { buildOsFsWriteTool } from "./fs-write.js"; +import { registerOsTools } from "./index.js"; + +/** + * F36. Two live failures (Gemma 4 31B, 2026-09-15): the model wrote + * `projects.json` over the user's data file without listing the folder, + * and a 9-row `sales.csv` over a 2,401-row dataset. Both files were + * inputs the request named; both were gone. The rules pinned here: the + * write still lands (warn-only), the previous content is saved first, + * the result says so — loudly on a ≥ 80 % shrink or a changed header, + * quietly otherwise — and `os.fs.restore` brings the bytes back. + */ +describe("replace guard (F36)", () => { + let dir: string; + let stateDir: string; + let store: FileRestoreStore; + let prompts: ApprovalRequest[]; + let gate: ApprovalGate; + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "atomic-replace-guard-")); + stateDir = await mkdtemp(join(tmpdir(), "atomic-replace-state-")); + store = new FileRestoreStore(join(stateDir, "restore")); + prompts = []; + gate = new ApprovalGate({ + emit: (req) => { + prompts.push(req); + gate.resolve({ approvalId: req.approvalId, approved: true }); + }, + }); + }); + + afterEach(async () => { + await rm(dir, { recursive: true, force: true }); + await rm(stateDir, { recursive: true, force: true }); + }); + + function ctx(sessionId = "s-guard"): ToolContext { + return { + workingDir: dir, + sessionId, + stepIndex: 0, + signal: new AbortController().signal, + }; + } + + /** `null` builds the tools with no store wired — the embedder / plain-test shape. */ + function tools(restore: FileRestoreStore | null = store) { + const options = { + approvals: gate, + approvalRequired: true, + ...(restore === null ? {} : { restore }), + }; + return { + write: buildOsFsWriteTool(options), + edit: buildOsFsEditTool(options), + patch: buildOsFsPatchTool(options), + restore: buildOsFsRestoreTool(options), + }; + } + + function csv(rows: number, header = "id,name,amount"): string { + const lines = [header]; + for (let i = 1; i <= rows; i++) lines.push(`${i},row ${i},${i * 10}`); + return `${lines.join("\n")}\n`; + } + + describe("os.fs.write", () => { + it("announces a shrink with a changed header loudly, saves the copy, and names the restore call", async () => { + const before = csv(2401); + await writeFile(join(dir, "sales.csv"), before, "utf8"); + const after = csv(9, "sku,qty"); + const result = await tools().write.run( + { path: "sales.csv", content: after }, + ctx(), + ); + expect(result.status).toBe("ok"); + const [note, wrote] = result.summary.split("\n"); + expect(note).toBe( + '⚠ replaced the user\'s file `sales.csv` (2,402 lines → 10, header changed); the previous content is saved — `os.fs.restore {"path":"sales.csv"}` brings it back', + ); + // The "(replace)" wording carries the counts too. + expect(wrote).toBe( + `wrote ${after.length} bytes to ${join(dir, "sales.csv")} (replace, 2,402 lines → 10)`, + ); + expect(result.details.replaced).toMatchObject({ + path: join(dir, "sales.csv"), + linesBefore: 2402, + linesAfter: 10, + shrunk: true, + headerChanged: true, + saved: "saved", + copy: "1-sales.csv", + }); + expect(result.details.previousLines).toBe(2402); + expect(result.details.lines).toBe(10); + // The write landed anyway: warn-only. + expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(after); + const copy = join(stateDir, "restore", "s-guard", "1-sales.csv"); + expect(await readFile(copy, "utf8")).toBe(before); + }); + + it("is loud on a header change alone (a .json whose first line moved)", async () => { + await writeFile( + join(dir, "projects.json"), + '[{"id": 1, "name": "alpha"},\n{"id": 2, "name": "beta"}]\n', + "utf8", + ); + const result = await tools().write.run( + { + path: "projects.json", + content: '{"projects": [\n{"id": 1}]}\n', + }, + ctx(), + ); + expect(result.summary.split("\n")[0]).toBe( + '⚠ replaced the user\'s file `projects.json` (2 lines → 2, header changed); the previous content is saved — `os.fs.restore {"path":"projects.json"}` brings it back', + ); + }); + + it("is loud on a shrink alone, without a header clause for a non-header format", async () => { + const before = Array.from({ length: 100 }, (_, i) => `line ${i}`).join( + "\n", + ); + await writeFile(join(dir, "app.log"), before, "utf8"); + const result = await tools().write.run( + { path: "app.log", content: "line 0\nline 1\n" }, + ctx(), + ); + const note = result.summary.split("\n")[0] ?? ""; + expect(note.startsWith("⚠ replaced the user's file `app.log` (100 lines → 2); ")).toBe(true); + expect(note).not.toContain("header changed"); + }); + + it("keeps to a quiet one-liner for a same-size replacement", async () => { + await writeFile(join(dir, "a.py"), "x = 1\ny = 2\nz = 3\n", "utf8"); + const result = await tools().write.run( + { path: "a.py", content: "x = 10\ny = 20\nz = 30\n" }, + ctx(), + ); + const note = result.summary.split("\n")[0]; + expect(note).toBe( + "replaced the user's file `a.py` (3 lines → 3); previous content saved", + ); + expect(note).not.toContain("⚠"); + expect(result.details.replaced).toMatchObject({ + shrunk: false, + headerChanged: false, + saved: "saved", + }); + expect(existsSync(join(stateDir, "restore", "s-guard", "1-a.py"))).toBe( + true, + ); + }); + + it("says nothing about a file the agent created earlier this session, and still counts the lines", async () => { + const t = tools(); + const first = await t.write.run( + { path: "out/report.md", content: "# Report\n\nfirst\n" }, + ctx(), + ); + expect(first.summary).toBe( + `wrote 16 bytes to ${join(dir, "out", "report.md")} (replace, new file, 3 lines)`, + ); + expect(first.details.existed).toBe(false); + const second = await t.write.run( + { path: "out/report.md", content: "# Done\n" }, + ctx(), + ); + expect(second.summary).toBe( + `wrote 7 bytes to ${join(dir, "out", "report.md")} (replace, 3 lines → 1)`, + ); + expect(second.details.replaced).toBeUndefined(); + expect(await readdir(join(stateDir, "restore", "s-guard"))).toEqual([ + "manifest.json", + ]); + }); + + it("a file created by an append is the agent's too; an append never replaces", async () => { + const t = tools(); + await t.write.run( + { path: "notes.txt", content: "one\n", mode: "append" }, + ctx(), + ); + await writeFile(join(dir, "user.txt"), "a\nb\nc\n", "utf8"); + const appended = await t.write.run( + { path: "user.txt", content: "d\n", mode: "append" }, + ctx(), + ); + expect(appended.summary).toContain("(append)"); + expect(appended.details.replaced).toBeUndefined(); + const replaced = await t.write.run( + { path: "notes.txt", content: "" }, + ctx(), + ); + expect(replaced.details.replaced).toBeUndefined(); + }); + + it("remembers the created set across a resumed session (same id, new process)", async () => { + await tools().write.run( + { path: "fresh.log", content: csv(5) }, + ctx("s-resumed"), + ); + const resumed = tools(new FileRestoreStore(join(stateDir, "restore"))); + const result = await resumed.write.run( + { path: "fresh.log", content: "x\n" }, + ctx("s-resumed"), + ); + expect(result.details.replaced).toBeUndefined(); + // A different session does not inherit it: the file is the user's there. + const other = await resumed.write.run( + { path: "fresh.log", content: "y\n" }, + ctx("s-other"), + ); + expect(other.summary.split("\n")[0]).toBe( + "replaced the user's file `fresh.log` (1 line → 1); previous content saved", + ); + }); + + it("has nothing to say about an empty file, or when no store is wired", async () => { + await writeFile(join(dir, "empty.csv"), "", "utf8"); + const empty = await tools().write.run( + { path: "empty.csv", content: csv(3) }, + ctx(), + ); + expect(empty.details.replaced).toBeUndefined(); + expect(empty.summary).toContain("(replace, 0 lines → 4)"); + + await writeFile(join(dir, "sales.csv"), csv(50), "utf8"); + const bare = await tools(null).write.run( + { path: "sales.csv", content: csv(1, "a,b") }, + ctx(), + ); + expect(bare.details.replaced).toBeUndefined(); + expect(bare.summary).toBe( + `wrote ${csv(1, "a,b").length} bytes to ${join(dir, "sales.csv")} (replace, 51 lines → 2)`, + ); + }); + + it("announces a file over the size cap loudly without reading or saving it", async () => { + const big = Buffer.alloc(RESTORE_MAX_BYTES + 1, 0x61); + await writeFile(join(dir, "huge.bin"), big); + const result = await tools().write.run( + { path: "huge.bin", content: "tiny\n" }, + ctx(), + ); + expect(result.summary.split("\n")[0]).toBe( + "⚠ replaced the user's file `huge.bin` (5.0 MB → 1 line); the previous content was too large to save (over 5.0 MB)", + ); + expect(result.details.replaced).toMatchObject({ + saved: "too_large", + linesBefore: null, + }); + expect(result.summary).toContain("(replace, 5.0 MB → 1 line)"); + expect(await store.listCopies("s-guard")).toEqual([]); + }); + + it("spells the operator's retarget in the note when the write was moved", async () => { + await writeFile(join(dir, "moved.txt"), "a\nb\nc\nd\ne\n", "utf8"); + const moving = new ApprovalGate({ + emit: (req) => + moving.resolve({ + approvalId: req.approvalId, + approved: true, + pathOverride: join(dir, "moved.txt"), + }), + }); + const write = buildOsFsWriteTool({ + approvals: moving, + approvalRequired: true, + restore: store, + }); + const result = await write.run( + { path: "elsewhere.txt", content: "z\n" }, + ctx(), + ); + expect(result.summary.split("\n")[0]).toBe( + `⚠ replaced the user's file \`${join(dir, "moved.txt")}\` (5 lines → 1, header changed); the previous content is saved — \`os.fs.restore ${JSON.stringify({ path: join(dir, "moved.txt") })}\` brings it back`, + ); + }); + }); + + describe("os.fs.restore", () => { + it("brings the saved bytes back, approval-gated like a write, and names what came back", async () => { + const before = csv(2401); + await writeFile(join(dir, "sales.csv"), before, "utf8"); + const t = tools(); + await t.write.run({ path: "sales.csv", content: csv(9, "sku,qty") }, ctx()); + prompts.length = 0; + + const result = await t.restore.run({ path: "sales.csv" }, ctx()); + expect(result.status).toBe("ok"); + expect(result.summary).toBe( + `restored \`sales.csv\` from the copy saved before os.fs.write: ${(before.length / 1024).toFixed(1)} KB, 2,402 lines`, + ); + expect(result.details).toMatchObject({ + path: join(dir, "sales.csv"), + bytes: before.length, + lines: 2402, + savedBefore: "os.fs.write", + copy: "1-sales.csv", + }); + expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(before); + expect(prompts).toHaveLength(1); + expect(prompts[0]).toMatchObject({ + tool: "os.fs.restore", + category: "fs_write_workspace", + }); + expect(prompts[0]?.reason).toContain("2,402 lines"); + // The copy stays: a second restore still works. + await t.write.run({ path: "sales.csv", content: "gone\n" }, ctx()); + const again = await t.restore.run({ path: "sales.csv" }, ctx()); + expect(again.status).toBe("ok"); + expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(before); + }); + + it("restores the most recent copy of a path replaced twice", async () => { + await writeFile(join(dir, "a.txt"), "first\n", "utf8"); + const t = tools(); + await t.write.run({ path: "a.txt", content: "second\n" }, ctx()); + await t.write.run({ path: "a.txt", content: "third\n" }, ctx()); + await t.restore.run({ path: "a.txt" }, ctx()); + expect(await readFile(join(dir, "a.txt"), "utf8")).toBe("second\n"); + }); + + it("refuses when nothing was saved for the path, and when no store is wired", async () => { + await expect( + tools().restore.run({ path: "never.csv" }, ctx()), + ).rejects.toThrow(/nothing saved for `never.csv` in this session/); + await expect( + tools(null).restore.run({ path: "never.csv" }, ctx()), + ).rejects.toThrow(/keeps no restore copies/); + await expect(tools().restore.run({}, ctx())).rejects.toThrow( + /`path` must be a non-empty string/, + ); + }); + + it("is a registered, approval-gated os.fs tool", async () => { + const registry = new ToolRegistry(); + registerOsTools(registry, { + approvals: gate, + approvalRequired: true, + config: { + http: { + enabled: true, + approvalMode: "writes", + hostAllowlist: null, + maxResponseBytes: 1_048_576, + defaultTimeoutMs: 30_000, + }, + web: { + search: { + enabled: true, + provider: "duckduckgo", + maxResults: 8, + timeoutMs: 15_000, + cacheTtlMinutes: 15, + fallback: [], + searxng: { instanceUrl: null }, + exa: { + endpoint: "https://mcp.exa.ai/mcp", + apiEndpoint: "https://api.exa.ai/search", + apiKeyEnv: "EXA_API_KEY", + }, + brave: { apiKeyEnv: "BRAVE_SEARCH_API_KEY" }, + }, + }, + projects: { roots: [] }, + }, + listRecentSessionDirs: () => [], + stateDir, + }); + expect(registry.has("os.fs.restore")).toBe(true); + expect(registry.get("os.fs.restore").readonly).toBe(false); + // The store the registry wired lives under `/restore`. + await writeFile(join(dir, "user.csv"), csv(20), "utf8"); + await registry + .get("os.fs.write") + .run({ path: "user.csv", content: csv(1, "q") }, ctx("s-reg")); + expect( + existsSync(join(stateDir, "restore", "s-reg", "1-user.csv")), + ).toBe(true); + }); + }); + + describe("copy cap", () => { + it(`keeps the last ${RESTORE_COPY_CAP} copies per session, dropping the oldest file`, async () => { + const t = tools(); + for (let i = 1; i <= RESTORE_COPY_CAP + 1; i++) { + await writeFile(join(dir, `f${i}.txt`), `user ${i}\n`, "utf8"); + await t.write.run({ path: `f${i}.txt`, content: `agent ${i}\n` }, ctx()); + } + const copies = await store.listCopies("s-guard"); + expect(copies).toHaveLength(RESTORE_COPY_CAP); + expect(copies[0]?.n).toBe(2); + expect(copies.at(-1)?.n).toBe(RESTORE_COPY_CAP + 1); + const files = (await readdir(join(stateDir, "restore", "s-guard"))).sort(); + expect(files).not.toContain("1-f1.txt"); + expect(files).toContain("2-f2.txt"); + expect(files).toContain(`${RESTORE_COPY_CAP + 1}-f${RESTORE_COPY_CAP + 1}.txt`); + await expect( + t.restore.run({ path: "f1.txt" }, ctx()), + ).rejects.toThrow(/nothing saved/); + await t.restore.run({ path: "f2.txt" }, ctx()); + expect(await readFile(join(dir, "f2.txt"), "utf8")).toBe("user 2\n"); + }); + }); + + describe("os.fs.edit and os.fs.patch", () => { + it("edit: a replaceAll that cut the file by 80 % is announced and saved; a small edit is not", async () => { + const before = csv(100); + await writeFile(join(dir, "sales.csv"), before, "utf8"); + const t = tools(); + const small = await t.edit.run( + { path: "sales.csv", oldString: "row 1,", newString: "row one," }, + ctx(), + ); + expect(small.details.replaced).toBeUndefined(); + expect(small.summary.startsWith("--- a/")).toBe(true); + + // Every data row shares ",row " — replacing the whole tail of each + // line with nothing leaves the header and 100 empty lines... so + // make it a true shrink: collapse the newlines instead. + const shrink = await t.edit.run( + { path: "sales.csv", oldString: "\n", newString: " ", replaceAll: true }, + ctx(), + ); + const note = shrink.summary.split("\n")[0]; + expect(note).toBe( + '⚠ shrank the user\'s file `sales.csv` (101 lines → 1); the previous content is saved — `os.fs.restore {"path":"sales.csv"}` brings it back', + ); + expect(shrink.details.replaced).toMatchObject({ + linesBefore: 101, + linesAfter: 1, + shrunk: true, + saved: "saved", + copy: "1-sales.csv", + }); + await t.restore.run({ path: "sales.csv" }, ctx()); + expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe( + before.replace("row 1,", "row one,"), + ); + }); + + it("edit: a file the agent created is never announced, however much it shrinks", async () => { + const t = tools(); + await t.write.run({ path: "mine.txt", content: csv(50) }, ctx()); + const result = await t.edit.run( + { path: "mine.txt", oldString: "\n", newString: "", replaceAll: true }, + ctx(), + ); + expect(result.details.replaced).toBeUndefined(); + }); + + it("patch: a hunk that deletes most of a user's file is announced with the absolute path; a created file is the agent's", async () => { + const before = csv(40); + const after = csv(2); + await writeFile(join(dir, "data.csv"), before, "utf8"); + const t = tools(); + const shrink = await t.patch.run( + { patch: createPatch("data.csv", before, after), apply: true }, + ctx(), + ); + expect(shrink.status).toBe("ok"); + expect(shrink.summary.split("\n")[0]).toBe( + `⚠ shrank the user's file \`${join(dir, "data.csv")}\` (41 lines → 3); the previous content is saved — \`os.fs.restore ${JSON.stringify({ path: join(dir, "data.csv") })}\` brings it back`, + ); + expect(shrink.summary).toContain("patch applied:"); + expect(await readFile(join(dir, "data.csv"), "utf8")).toBe(after); + await t.restore.run({ path: join(dir, "data.csv") }, ctx()); + expect(await readFile(join(dir, "data.csv"), "utf8")).toBe(before); + + const created = await t.patch.run( + { patch: createPatch("new.csv", "", csv(30)), apply: true }, + ctx(), + ); + expect(created.status).toBe("ok"); + expect(created.details.replaced).toBeUndefined(); + expect(await store.wasCreated("s-guard", join(dir, "new.csv"))).toBe( + true, + ); + const overwritten = await t.write.run( + { path: "new.csv", content: "q\n" }, + ctx(), + ); + expect(overwritten.details.replaced).toBeUndefined(); + }); + + it("patch: a small change to a user's file is silent", async () => { + const before = csv(40); + const after = before.replace("row 2,", "row two,"); + await writeFile(join(dir, "data.csv"), before, "utf8"); + const result = await tools().patch.run( + { patch: createPatch("data.csv", before, after), apply: true }, + ctx(), + ); + expect(result.status).toBe("ok"); + expect(result.details.replaced).toBeUndefined(); + expect(result.summary.startsWith("patch applied:")).toBe(true); + }); + }); + + describe("rules", () => { + it("counts lines the way an editor does", () => { + expect(countLines("")).toBe(0); + expect(countLines("a")).toBe(1); + expect(countLines("a\n")).toBe(1); + expect(countLines("a\nb")).toBe(2); + expect(countLines("a\r\nb\r\n")).toBe(2); + expect(firstLine("h1,h2\r\n1,2\n")).toBe("h1,h2"); + expect(firstLine("solo")).toBe("solo"); + }); + + it("a shrink is 80 % or more of the lines gone, never on an empty file", () => { + expect(isShrink(2401, 10)).toBe(true); + expect(isShrink(10, 2)).toBe(true); + expect(isShrink(10, 3)).toBe(false); + expect(isShrink(5, 1)).toBe(true); + expect(isShrink(4, 1)).toBe(false); + expect(isShrink(1, 0)).toBe(true); + expect(isShrink(0, 0)).toBe(false); + expect(isShrink(0, 5)).toBe(false); + }); + }); +}); diff --git a/src/tools/os/fs-replace-guard.ts b/src/tools/os/fs-replace-guard.ts new file mode 100644 index 00000000..5af57ed0 --- /dev/null +++ b/src/tools/os/fs-replace-guard.ts @@ -0,0 +1,253 @@ +import { readFile, stat } from "node:fs/promises"; +import { extname } from "node:path"; +import type { CompressedToolResult } from "../../compressor/result-compressor.js"; +import { RESTORE_MAX_BYTES, type FileRestoreStore } from "./fs-restore-store.js"; + +/** + * The guard `os.fs.write` / `edit` / `patch` run before they replace a + * file the agent did not create this session. It never blocks: the write + * lands, the previous content is saved through `FileRestoreStore`, and a + * line is prepended to the tool result. Loud (`⚠`, with the restore call + * spelled out) when the replacement looks like a loss; a quiet one-liner + * for any other replacement of a pre-existing user file. + */ + +/** + * Formats whose first line is a header or the document's shape — a + * changed first line on one of these is announced loudly even when the + * size held (the 9-row `sales.csv` had new column names, too). + */ +export const HEADER_SENSITIVE_EXTENSIONS: ReadonlySet = new Set([ + ".csv", + ".tsv", + ".json", + ".jsonl", + ".md", + ".txt", + ".yaml", + ".yml", + ".toml", + ".ini", +]); + +/** + * What sat at the path before the write. `content` is null past + * `RESTORE_MAX_BYTES` — such a file is announced but not read or saved. + */ +export interface PriorFile { + bytes: number; + content: Buffer | null; + lines: number | null; + firstLine: string | null; +} + +export type ReplaceChange = "replace" | "shrink"; + +export interface ReplaceGuardInput { + store: FileRestoreStore | undefined; + sessionId: string; + absolute: string; + /** The path as the model should spell it in `os.fs.restore`. */ + display: string; + tool: string; + /** + * `replace`: a whole-file write — every pre-existing user file is + * noted, loudly on a shrink or a header change. `shrink`: an edit or a + * patch — noted only when the result shrank the file, always loudly. + */ + change: ReplaceChange; + prior: PriorFile; + after: string; +} + +export interface ReplacedFileDetails { + path: string; + bytesBefore: number; + linesBefore: number | null; + linesAfter: number; + shrunk: boolean; + headerChanged: boolean; + saved: "saved" | "too_large" | "failed"; + copy?: string; +} + +export interface ReplaceGuardOutcome { + note: string | null; + replaced?: ReplacedFileDetails; +} + +export const NO_REPLACE_NOTE: ReplaceGuardOutcome = { note: null }; + +/** Read what `absolute` holds now: null when nothing (or not a regular file). */ +export async function readPriorFile(absolute: string): Promise { + let size: number; + try { + const info = await stat(absolute); + if (!info.isFile()) return null; + size = info.size; + } catch { + return null; + } + if (size > RESTORE_MAX_BYTES) { + return { bytes: size, content: null, lines: null, firstLine: null }; + } + try { + const content = await readFile(absolute); + const text = content.toString("utf8"); + return { + bytes: content.byteLength, + content, + lines: countLines(text), + firstLine: firstLine(text), + }; + } catch { + return { bytes: size, content: null, lines: null, firstLine: null }; + } +} + +/** A `PriorFile` for content a tool already holds as text (edit, patch). */ +export function priorFromText(text: string): PriorFile { + const content = Buffer.from(text, "utf8"); + return { + bytes: content.byteLength, + content, + lines: countLines(text), + firstLine: firstLine(text), + }; +} + +/** Lines as an editor counts them: a trailing newline does not open one more. */ +export function countLines(text: string): number { + if (text.length === 0) return 0; + let newlines = 0; + for (let i = text.indexOf("\n"); i !== -1; i = text.indexOf("\n", i + 1)) { + newlines++; + } + return text.endsWith("\n") ? newlines : newlines + 1; +} + +export function firstLine(text: string): string { + const end = text.indexOf("\n"); + const line = end === -1 ? text : text.slice(0, end); + return line.endsWith("\r") ? line.slice(0, -1) : line; +} + +/** Did the line count drop by 80 % or more? Integer arithmetic: dropped/before ≥ 4/5. */ +export function isShrink(linesBefore: number, linesAfter: number): boolean { + return linesBefore > 0 && (linesBefore - linesAfter) * 5 >= linesBefore * 4; +} + +export async function guardReplacedFile( + input: ReplaceGuardInput, +): Promise { + const { prior, store } = input; + // No store (embedders, tests) means nothing could be brought back, so + // nothing is claimed; an empty file has nothing to lose. + if (store === undefined || prior.bytes === 0) return NO_REPLACE_NOTE; + if (await store.wasCreated(input.sessionId, input.absolute)) { + return NO_REPLACE_NOTE; + } + const linesAfter = countLines(input.after); + const shrunk = prior.lines !== null && isShrink(prior.lines, linesAfter); + const headerChanged = + input.change === "replace" && + prior.firstLine !== null && + HEADER_SENSITIVE_EXTENSIONS.has(extname(input.absolute).toLowerCase()) && + prior.firstLine !== firstLine(input.after); + if (input.change === "shrink" && !shrunk) return NO_REPLACE_NOTE; + // A file too large to read is a loud case: nothing about it is known + // except that it was the user's and it is gone. + const loud = shrunk || headerChanged || prior.lines === null; + + let saved: ReplacedFileDetails["saved"] = "too_large"; + let copyFile: string | undefined; + let failure = ""; + if (prior.content !== null && prior.bytes <= RESTORE_MAX_BYTES) { + try { + const copy = await store.saveCopy( + input.sessionId, + input.absolute, + prior.content, + { tool: input.tool, lines: prior.lines ?? 0 }, + ); + saved = "saved"; + copyFile = copy.file; + } catch (err) { + saved = "failed"; + failure = (err as Error).message; + } + } + + const counts = formatCounts(prior, linesAfter, headerChanged); + const verb = input.change === "replace" ? "replaced" : "shrank"; + const head = `${verb} the user's file \`${input.display}\` (${counts})`; + const tail = + saved === "saved" + ? loud + ? `the previous content is saved — \`os.fs.restore ${JSON.stringify({ path: input.display })}\` brings it back` + : "previous content saved" + : saved === "too_large" + ? `the previous content was too large to save (over ${formatBytes(RESTORE_MAX_BYTES)})` + : `the previous content could not be saved: ${failure}`; + const note = `${loud ? "⚠ " : ""}${head}; ${tail}`; + return { + note, + replaced: { + path: input.absolute, + bytesBefore: prior.bytes, + linesBefore: prior.lines, + linesAfter, + shrunk, + headerChanged, + saved, + ...(copyFile !== undefined ? { copy: copyFile } : {}), + }, + }; +} + +/** Prepend the guard's line(s) to the summary; a batch of outcomes (patch) keeps file order. */ +export function withReplaceNotes( + result: CompressedToolResult, + outcomes: readonly ReplaceGuardOutcome[], +): CompressedToolResult { + const noted = outcomes.filter((o) => o.note !== null); + if (noted.length === 0) return result; + const notes = noted.map((o) => o.note).join("\n"); + const replaced = noted.map((o) => o.replaced); + return { + ...result, + summary: + result.summary.length > 0 ? `${notes}\n${result.summary}` : notes, + details: { + ...result.details, + replaced: replaced.length === 1 ? replaced[0] : replaced, + }, + }; +} + +/** `2,401 lines → 10, header changed`, or `12.3 MB → 10 lines` when the file was never read. */ +function formatCounts( + prior: PriorFile, + linesAfter: number, + headerChanged: boolean, +): string { + const counts = + prior.lines === null + ? `${formatBytes(prior.bytes)} → ${formatLines(linesAfter)}` + : `${formatLines(prior.lines)} → ${formatNumber(linesAfter)}`; + return headerChanged ? `${counts}, header changed` : counts; +} + +export function formatLines(n: number): string { + return `${formatNumber(n)} ${n === 1 ? "line" : "lines"}`; +} + +export function formatNumber(n: number): string { + return n.toLocaleString("en-US"); +} + +export function formatBytes(n: number): string { + if (n >= 1024 * 1024) return `${(n / (1024 * 1024)).toFixed(1)} MB`; + if (n >= 1024) return `${(n / 1024).toFixed(1)} KB`; + return `${n} B`; +} diff --git a/src/tools/os/fs-require-approval.ts b/src/tools/os/fs-require-approval.ts index afc29681..f82f468b 100644 --- a/src/tools/os/fs-require-approval.ts +++ b/src/tools/os/fs-require-approval.ts @@ -7,6 +7,7 @@ import { categorizeFsMutation, type FsMutationKind, } from "./fs-approval-scope.js"; +import type { FileRestoreStore } from "./fs-restore-store.js"; /** * `DangerousToolOptions` plus the injected trust-config surface every @@ -23,6 +24,14 @@ export interface FsDangerousToolOptions extends DangerousToolOptions { * derived inside the tools layer. */ trustConfigPaths?: readonly string[]; + /** + * Where a replaced user file's previous content is kept and which + * files this session created (`fs-replace-guard.ts`). Built by + * `registerOsTools` from `stateDir`; omitted (embedders, tests) turns + * the replace guard off and leaves `os.fs.restore` with nothing to + * restore. + */ + restore?: FileRestoreStore; } /** diff --git a/src/tools/os/fs-restore-store.ts b/src/tools/os/fs-restore-store.ts new file mode 100644 index 00000000..9a78cbdb --- /dev/null +++ b/src/tools/os/fs-restore-store.ts @@ -0,0 +1,209 @@ +import { mkdir, readFile, rename, rm, writeFile } from "node:fs/promises"; +import { basename, join } from "node:path"; + +/** + * Where a replaced user file's previous content goes, and which files the + * agent itself created this session. + * + * Two live failures motivate this (Gemma 4 31B, 2026-09-15): the model's + * first step wrote `projects.json` over the user's data file without + * listing the folder — the result said "(replace)" and the model did not + * react — and after a corrupted listing it wrote a 9-row `sales.csv` over + * the user's 2,401-row dataset. Both were named by the request as inputs; + * both were unrecoverable afterwards. Warn-only by design: the write + * still lands, but its previous content is saved first and the result + * says so, loudly when the replacement looks like a loss. + * + * Everything lives under `/restore//`: the copies + * as `-` and a small `manifest.json` carrying the copy + * index and the created-path set. On disk rather than in `SessionState` + * because the tools consult it BEFORE a write, and a tool sees only its + * `ToolContext` (working dir, session id) — threading session state into + * every fs tool would be the invasive change. Keyed by session id, so a + * resumed session (same id, new process) still knows what it created. + */ + +/** Copies kept per session; the oldest is dropped when a new one lands. */ +export const RESTORE_COPY_CAP = 20; +/** Largest previous content a copy is taken of. Bigger is announced, not saved. */ +export const RESTORE_MAX_BYTES = 5 * 1024 * 1024; +/** Paths remembered as created by the agent; the oldest are forgotten past this. */ +const CREATED_PATHS_CAP = 5000; +const MANIFEST_FILE = "manifest.json"; +/** Keeps `-` under every filesystem's name limit. */ +const COPY_BASENAME_MAX = 200; + +export interface RestoreCopy { + /** Monotonic per session; the copy's file is `-`. */ + n: number; + /** Absolute path of the file whose previous content this is. */ + path: string; + /** File name of the copy inside the session's restore directory. */ + file: string; + bytes: number; + lines: number; + savedAt: number; + /** The tool whose call replaced the file. */ + tool: string; +} + +interface RestoreManifest { + version: 1; + next: number; + created: string[]; + copies: RestoreCopy[]; +} + +export class FileRestoreStore { + constructor(private readonly root: string) {} + + /** `/` — the session's copies and manifest. */ + sessionDir(sessionId: string): string { + return join(this.root, safeSegment(sessionId)); + } + + /** Did a tool of this session create `absolute` (write to a path that did not exist)? */ + async wasCreated(sessionId: string, absolute: string): Promise { + const manifest = await this.read(sessionId); + return manifest.created.includes(absolute); + } + + async recordCreated(sessionId: string, absolute: string): Promise { + const manifest = await this.read(sessionId); + if (manifest.created.includes(absolute)) return; + manifest.created.push(absolute); + if (manifest.created.length > CREATED_PATHS_CAP) { + manifest.created.splice(0, manifest.created.length - CREATED_PATHS_CAP); + } + await this.write(sessionId, manifest); + } + + /** + * Save `content` as the previous content of `absolute`. The caller has + * checked the size cap; the bytes are stored as given so a restore puts + * back exactly what was there. Past `RESTORE_COPY_CAP` the oldest copy + * of the session — whichever path it belonged to — is removed. + */ + async saveCopy( + sessionId: string, + absolute: string, + content: Uint8Array | string, + meta: { tool: string; lines: number }, + ): Promise { + const dir = this.sessionDir(sessionId); + await mkdir(dir, { recursive: true }); + const manifest = await this.read(sessionId); + const n = manifest.next; + const file = `${n}-${basename(absolute).slice(0, COPY_BASENAME_MAX)}`; + await writeFile(join(dir, file), content); + const copy: RestoreCopy = { + n, + path: absolute, + file, + bytes: + typeof content === "string" + ? Buffer.byteLength(content, "utf8") + : content.byteLength, + lines: meta.lines, + savedAt: Date.now(), + tool: meta.tool, + }; + manifest.next = n + 1; + manifest.copies.push(copy); + while (manifest.copies.length > RESTORE_COPY_CAP) { + const dropped = manifest.copies.shift(); + if (dropped !== undefined) { + await rm(join(dir, dropped.file), { force: true }); + } + } + await this.write(sessionId, manifest); + return copy; + } + + /** The newest saved copy for `absolute`, or null when none was ever taken (or it aged out). */ + async latestCopy( + sessionId: string, + absolute: string, + ): Promise { + const manifest = await this.read(sessionId); + for (let i = manifest.copies.length - 1; i >= 0; i--) { + const copy = manifest.copies[i]; + if (copy !== undefined && copy.path === absolute) return copy; + } + return null; + } + + async readCopy(sessionId: string, copy: RestoreCopy): Promise { + return readFile(join(this.sessionDir(sessionId), copy.file)); + } + + /** Every copy the session still holds, oldest first. */ + async listCopies(sessionId: string): Promise { + return (await this.read(sessionId)).copies; + } + + private async read(sessionId: string): Promise { + try { + const raw = await readFile( + join(this.sessionDir(sessionId), MANIFEST_FILE), + "utf8", + ); + return normalizeManifest(JSON.parse(raw)); + } catch { + return emptyManifest(); + } + } + + private async write( + sessionId: string, + manifest: RestoreManifest, + ): Promise { + const dir = this.sessionDir(sessionId); + await mkdir(dir, { recursive: true }); + const temp = join(dir, `${MANIFEST_FILE}.${process.pid}.tmp`); + await writeFile(temp, JSON.stringify(manifest), "utf8"); + await rename(temp, join(dir, MANIFEST_FILE)); + } +} + +function emptyManifest(): RestoreManifest { + return { version: 1, next: 1, created: [], copies: [] }; +} + +/** A manifest a previous build wrote, or a damaged one, never throws — it just remembers less. */ +function normalizeManifest(raw: unknown): RestoreManifest { + if (typeof raw !== "object" || raw === null) return emptyManifest(); + const record = raw as Partial; + const created = Array.isArray(record.created) + ? record.created.filter((p): p is string => typeof p === "string") + : []; + const copies = Array.isArray(record.copies) + ? record.copies.filter(isRestoreCopy) + : []; + const highest = copies.reduce((max, copy) => Math.max(max, copy.n), 0); + const next = + typeof record.next === "number" && Number.isInteger(record.next) + ? Math.max(record.next, highest + 1) + : highest + 1; + return { version: 1, next, created, copies }; +} + +function isRestoreCopy(value: unknown): value is RestoreCopy { + if (typeof value !== "object" || value === null) return false; + const copy = value as Partial; + return ( + typeof copy.n === "number" && + typeof copy.path === "string" && + typeof copy.file === "string" && + typeof copy.bytes === "number" && + typeof copy.lines === "number" && + typeof copy.savedAt === "number" && + typeof copy.tool === "string" + ); +} + +/** A session id is `s-` today; anything else is made a safe directory name. */ +function safeSegment(id: string): string { + const safe = id.replace(/[^A-Za-z0-9._-]/g, "_"); + return safe.length === 0 ? "_" : safe; +} diff --git a/src/tools/os/fs-restore.ts b/src/tools/os/fs-restore.ts new file mode 100644 index 00000000..2f339430 --- /dev/null +++ b/src/tools/os/fs-restore.ts @@ -0,0 +1,94 @@ +import { mkdir, writeFile } from "node:fs/promises"; +import { dirname } from "node:path"; +import { compressToolResult } from "../../compressor/result-compressor.js"; +import { resolveUserPath } from "./expand-home.js"; +import { formatBytes, formatLines } from "./fs-replace-guard.js"; +import { + requireFsApproval, + type FsDangerousToolOptions, +} from "./fs-require-approval.js"; +import type { ToolDefinition } from "../tool-registry.js"; + +const PREVIEW_MAX_LEN = 400; + +/** + * `os.fs.restore { path }` — put back the previous content that + * `os.fs.write` / `edit` / `patch` saved before replacing a user file this + * session (see `fs-replace-guard.ts`). A write in every sense, so it + * rides the same approval ladder; the copy stays in the store, so a + * second restore of the same path still works. + */ +export function buildOsFsRestoreTool( + options: FsDangerousToolOptions, +): ToolDefinition { + return { + name: "os.fs.restore", + description: + "Bring back the previous content of a file this session replaced or shrank (saved automatically by os.fs.write / os.fs.edit / os.fs.patch). Dangerous — always requires approval.", + readonly: false, + async run(rawArgs, ctx) { + const path = rawArgs.path; + if (typeof path !== "string" || path.length === 0) { + throw new Error("os.fs.restore: `path` must be a non-empty string"); + } + const store = options.restore; + if (store === undefined) { + throw new Error( + "os.fs.restore: this runtime keeps no restore copies (no state directory)", + ); + } + const absolute = resolveUserPath(path, ctx.workingDir); + const copy = await store.latestCopy(ctx.sessionId, absolute); + if (copy === null) { + throw new Error( + `os.fs.restore: nothing saved for \`${path}\` in this session — only a pre-existing file replaced by os.fs.write / edit / patch has a copy`, + ); + } + let content: Buffer; + try { + content = await store.readCopy(ctx.sessionId, copy); + } catch { + throw new Error( + `os.fs.restore: the saved copy of \`${path}\` is gone (${copy.file})`, + ); + } + + const text = content.toString("utf8"); + const preview = + text.length > PREVIEW_MAX_LEN + ? `${text.slice(0, PREVIEW_MAX_LEN)}…` + : text; + await requireFsApproval( + options, + { + kind: "write", + paths: [absolute], + sessionId: ctx.sessionId, + tool: "os.fs.restore", + reason: `restore ${absolute} to the ${formatBytes(copy.bytes)} (${formatLines(copy.lines)}) it held before ${copy.tool}`, + preview, + affectedResources: [absolute], + workingDir: ctx.workingDir, + trustConfigPaths: options.trustConfigPaths, + }, + ctx.signal, + ); + + await mkdir(dirname(absolute), { recursive: true }); + await writeFile(absolute, content); + return compressToolResult({ + tool: "os.fs.restore", + status: "ok", + output: `restored \`${path}\` from the copy saved before ${copy.tool}: ${formatBytes(copy.bytes)}, ${formatLines(copy.lines)}`, + details: { + path: absolute, + bytes: copy.bytes, + lines: copy.lines, + savedAt: copy.savedAt, + savedBefore: copy.tool, + copy: copy.file, + }, + }); + }, + }; +} diff --git a/src/tools/os/fs-write.ts b/src/tools/os/fs-write.ts index 481643b7..0cdc4cda 100644 --- a/src/tools/os/fs-write.ts +++ b/src/tools/os/fs-write.ts @@ -5,6 +5,18 @@ import { resolveUserPath } from "./expand-home.js"; import { categorizeFsMutation } from "./fs-approval-scope.js"; import { checkChangedFile } from "./fs-content-check.js"; import { PARSE_CHECK_MAX_CHARS, withParseWarning } from "./fs-parse-check.js"; +import { + NO_REPLACE_NOTE, + countLines, + formatBytes, + formatLines, + formatNumber, + guardReplacedFile, + readPriorFile, + withReplaceNotes, + type PriorFile, +} from "./fs-replace-guard.js"; +import type { FileRestoreStore } from "./fs-restore-store.js"; import { requireFsApproval, type FsDangerousToolOptions, @@ -101,6 +113,10 @@ export function buildOsFsWriteTool( if (nextCategory === outcome.category) break; } + // What is there now, read before it is gone: the line counts the + // result reports, and — for a user's file about to be replaced — + // the content the restore copy is taken from. + const prior = await readPriorFile(target); await mkdir(dirname(target), { recursive: true }); if (mode === "append") { const { appendFile } = await import("node:fs/promises"); @@ -108,36 +124,99 @@ export function buildOsFsWriteTool( } else { await writeFile(target, content, "utf8"); } + const guard = + prior === null + ? await noteCreated(options.restore, ctx.sessionId, target) + : mode === "replace" + ? await guardReplacedFile({ + store: options.restore, + sessionId: ctx.sessionId, + absolute: target, + display: target === absolute ? path : target, + tool: "os.fs.write", + change: "replace", + prior, + after: content, + }) + : NO_REPLACE_NOTE; const parseWarning = await parseWarningAfterWrite( target, mode, content, ctx.workingDir, ); + const linesAfter = countLines(content); // The path is echoed in `output` (not just `details`) so a model // that had its target moved reads where the file actually landed // and keeps working against the right path. - return withParseWarning( - compressToolResult({ - tool: "os.fs.write", - status: "ok", - output: - target === absolute - ? `wrote ${content.length} bytes to ${target} (${mode})` - : `wrote ${content.length} bytes to ${target} (${mode}); the operator moved this write from ${absolute}`, - details: { - path: target, - bytes: content.length, - mode, - ...(target === absolute ? {} : { requestedPath: absolute }), - }, - }), - parseWarning, + const wording = describeWrite(mode, prior, linesAfter); + return withReplaceNotes( + withParseWarning( + compressToolResult({ + tool: "os.fs.write", + status: "ok", + output: + target === absolute + ? `wrote ${content.length} bytes to ${target} (${wording})` + : `wrote ${content.length} bytes to ${target} (${wording}); the operator moved this write from ${absolute}`, + details: { + path: target, + bytes: content.length, + mode, + lines: linesAfter, + existed: prior !== null, + ...(prior === null ? {} : { previousBytes: prior.bytes }), + ...(prior?.lines === undefined || prior.lines === null + ? {} + : { previousLines: prior.lines }), + ...(target === absolute ? {} : { requestedPath: absolute }), + }, + }), + parseWarning, + ), + [guard], ); }, }; } +/** + * The parenthetical after "wrote N bytes to path": `(replace)` used to + * be all a model read when it overwrote a 2,401-line dataset with 10 + * lines, so the counts ride along — `(replace, 2,401 lines → 10)`, + * `(replace, new file, 10 lines)`, `(replace, 12.3 MB → 10 lines)` for + * a file too large to read. An append is judged on the chunk only. + */ +function describeWrite( + mode: "append" | "replace", + prior: PriorFile | null, + linesAfter: number, +): string { + if (mode === "append") return "append"; + if (prior === null) return `replace, new file, ${formatLines(linesAfter)}`; + if (prior.lines === null) { + return `replace, ${formatBytes(prior.bytes)} → ${formatLines(linesAfter)}`; + } + return `replace, ${formatLines(prior.lines)} → ${formatNumber(linesAfter)}`; +} + +/** Remember that this session created `target`, so replacing it later is not a loss. Best effort. */ +async function noteCreated( + store: FileRestoreStore | undefined, + sessionId: string, + target: string, +): Promise { + if (store !== undefined) { + try { + await store.recordCreated(sessionId, target); + } catch { + // The write already landed; a manifest that could not be written + // only means a later replacement is announced when it need not be. + } + } + return NO_REPLACE_NOTE; +} + /** * Check what now sits at `target` (see `fs-parse-check.ts` and * `fs-content-check.ts`). An append is judged on the whole file, not the diff --git a/src/tools/os/index.ts b/src/tools/os/index.ts index fd701c85..24559b35 100644 --- a/src/tools/os/index.ts +++ b/src/tools/os/index.ts @@ -1,3 +1,4 @@ +import { resolve } from "node:path"; import type { ToolRegistry } from "../tool-registry.js"; import type { DangerousToolOptions } from "../../approval/dangerous-tool.js"; import type { AtomicAgentConfig } from "../../config/index.js"; @@ -28,6 +29,8 @@ import { osNotifyTool } from "./notify.js"; import { osFsHashTool } from "./fs-hash.js"; import { osFsDiffTool } from "./fs-diff.js"; import { buildOsFsPatchTool } from "./fs-patch.js"; +import { buildOsFsRestoreTool } from "./fs-restore.js"; +import { FileRestoreStore } from "./fs-restore-store.js"; import { osFsWatchTool } from "./fs-watch.js"; import { osGitStatusTool, @@ -77,6 +80,8 @@ export { buildOsEmailInboxTool, buildOsEmailSendTool } from "./email.js"; export { osFsHashTool } from "./fs-hash.js"; export { osFsDiffTool } from "./fs-diff.js"; export { buildOsFsPatchTool } from "./fs-patch.js"; +export { buildOsFsRestoreTool } from "./fs-restore.js"; +export { FileRestoreStore } from "./fs-restore-store.js"; export { osFsWatchTool } from "./fs-watch.js"; export { osGitStatusTool, @@ -119,8 +124,11 @@ export interface RegisterOsToolsOptions extends DangerousToolOptions { /** * Absolute state directory (`config.paths.stateDir`), resolved by the * bootstrap and threaded into `os.web.search` so its result cache and - * provider cooldown can survive the process (#256). Omitted keeps both - * in-memory, which is what existing embedders and tests get. + * provider cooldown can survive the process (#256), and into the fs + * mutation tools as `/restore/` — where a replaced user + * file's previous content is kept for `os.fs.restore`. Omitted keeps + * the search cache in-memory and turns the replace guard off, which is + * what existing embedders and tests get. */ stateDir?: string; /** @@ -145,8 +153,19 @@ export function registerOsTools( : { shellPolicy: options.shellPolicy }), }), ); + // One option bag for every tool that replaces file content, so the + // write, the edit, the patch and the restore share the store that + // remembers what this session created and what it replaced. + const fsMutation = { + approvals: options.approvals, + approvalRequired: options.approvalRequired, + trustConfigPaths: options.trustConfigPaths, + ...(options.stateDir === undefined + ? {} + : { restore: new FileRestoreStore(resolve(options.stateDir, "restore")) }), + }; registry.register(osFsReadTool); - registry.register(buildOsFsWriteTool(options)); + registry.register(buildOsFsWriteTool(fsMutation)); registry.register(buildOsFsTrashTool(options)); registry.register(osFsListTool); registry.register(osFsGlobTool); @@ -157,7 +176,7 @@ export function registerOsTools( }), ); registry.register(buildOsFsGrepTool()); - registry.register(buildOsFsEditTool(options)); + registry.register(buildOsFsEditTool(fsMutation)); registry.register(buildOsFsReadDocumentTool()); registry.register(buildOsFsArchiveListTool()); registry.register(buildOsFsArchiveReadEntryTool()); @@ -201,13 +220,8 @@ export function registerOsTools( ); registry.register(osFsHashTool); registry.register(osFsDiffTool); - registry.register( - buildOsFsPatchTool({ - approvals: options.approvals, - approvalRequired: options.approvalRequired, - trustConfigPaths: options.trustConfigPaths, - }), - ); + registry.register(buildOsFsPatchTool(fsMutation)); + registry.register(buildOsFsRestoreTool(fsMutation)); registry.register(osFsWatchTool); registry.register(osGitStatusTool); registry.register(osGitLogTool); diff --git a/src/tools/os/os-tools.test.ts b/src/tools/os/os-tools.test.ts index 95b066be..5d7916c7 100644 --- a/src/tools/os/os-tools.test.ts +++ b/src/tools/os/os-tools.test.ts @@ -593,6 +593,7 @@ describe("registerOsTools", () => { "os.fs.patch", "os.fs.read", "os.fs.read_document", + "os.fs.restore", "os.fs.trash", "os.fs.watch", "os.fs.write", diff --git a/src/tools/tool-roles.test.ts b/src/tools/tool-roles.test.ts index 611f43ac..94068ab0 100644 --- a/src/tools/tool-roles.test.ts +++ b/src/tools/tool-roles.test.ts @@ -18,6 +18,7 @@ describe("tool roles", () => { "os.fs.write", "os.fs.edit", "os.fs.patch", + "os.fs.restore", "os.fs.list", "os.fs.glob", "os.fs.grep", @@ -82,6 +83,7 @@ describe("tool roles", () => { "os.fs.write", "os.fs.edit", "os.fs.patch", + "os.fs.restore", "os.shell.run", "skill.view", "mcp.resource.read", diff --git a/src/tools/tool-roles.ts b/src/tools/tool-roles.ts index 28c412c4..06acff5b 100644 --- a/src/tools/tool-roles.ts +++ b/src/tools/tool-roles.ts @@ -60,6 +60,9 @@ const FS_WRITE_TOOLS: readonly string[] = [ "os.fs.write", "os.fs.edit", "os.fs.patch", + // Undoes one of the three; a worker that shrank a user file must be + // able to put it back. + "os.fs.restore", ]; /** Long-term memory READS: the orchestrator plans against what is known. */ From c004d348d8f2eb4e229c5ad845e8872411e615be Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 08:19:55 +0300 Subject: [PATCH 3/4] fix(tools): F40 a tool call with unknown argument keys is refused, not run with them dropped --- src/agent/batch-executor.test.ts | 230 ++++++++++++++++++++--- src/agent/batch-executor.ts | 79 +++++--- src/tools/coerce-tool-args.ts | 7 +- src/tools/os/shell.test.ts | 63 ++++++- src/tools/os/shell.ts | 9 +- src/tools/unknown-argument-guard.test.ts | 148 +++++++++++++++ src/tools/unknown-argument-guard.ts | 178 ++++++++++++++++++ 7 files changed, 662 insertions(+), 52 deletions(-) create mode 100644 src/tools/unknown-argument-guard.test.ts create mode 100644 src/tools/unknown-argument-guard.ts diff --git a/src/agent/batch-executor.test.ts b/src/agent/batch-executor.test.ts index 0a4c1780..be386190 100644 --- a/src/agent/batch-executor.test.ts +++ b/src/agent/batch-executor.test.ts @@ -233,14 +233,16 @@ describe("executeBatch", () => { description: "click", readonly: false, run: async (args) => { - const idx = (args.idx as number) ?? -1; + // Fixtures carry their index under a key the tool's schema knows + // (`ref`, `offset`): an unknown key is refused before dispatch (F40). + const idx = (args.ref as number) ?? -1; return await make(idx)(args); }, }); const inputs = toBatchInputs([ - { tool: "browser.click", args: { idx: 0 } }, - { tool: "browser.click", args: { idx: 1 } }, - { tool: "browser.click", args: { idx: 2 } }, + { tool: "browser.click", args: { ref: 0 } }, + { tool: "browser.click", args: { ref: 1 } }, + { tool: "browser.click", args: { ref: 2 } }, ]); const ctrl = new AbortController(); const startedAt = Date.now(); @@ -261,7 +263,7 @@ describe("executeBatch", () => { readonly: true, run: async (args) => { await new Promise((r) => setTimeout(r, 60)); - reads.push((args.idx as number) ?? -1); + reads.push((args.offset as number) ?? -1); return okResult("os.fs.read"); }, }); @@ -271,15 +273,15 @@ describe("executeBatch", () => { readonly: false, run: async (args) => { await new Promise((r) => setTimeout(r, 60)); - clicks.push((args.idx as number) ?? -1); + clicks.push((args.ref as number) ?? -1); return okResult("browser.click"); }, }); const inputs = toBatchInputs([ - { tool: "os.fs.read", args: { idx: 0 } }, - { tool: "browser.click", args: { idx: 1 } }, - { tool: "os.fs.read", args: { idx: 2 } }, - { tool: "browser.click", args: { idx: 3 } }, + { tool: "os.fs.read", args: { offset: 0 } }, + { tool: "browser.click", args: { ref: 1 } }, + { tool: "os.fs.read", args: { offset: 2 } }, + { tool: "browser.click", args: { ref: 3 } }, ]); const ctrl = new AbortController(); const startedAt = Date.now(); @@ -332,8 +334,11 @@ describe("executeBatch", () => { throw new Error("os.fs.read: `path` must be a non-empty string"); }, }); + // A misspelt key (`patth`) no longer reaches the tool at all — F40 + // refuses it before dispatch — so the thrown path is exercised with + // a known key the tool rejects. const inputs = toBatchInputs([ - { tool: "os.fs.read", args: { patth: "secret-value.txt" } }, + { tool: "os.fs.read", args: { path: "" } }, ]); const out = await executeBatch( inputs, @@ -342,11 +347,10 @@ describe("executeBatch", () => { ); const result = out.results[0]!.compressed!; expect(result.status).toBe("error"); - expect(result.summary).toContain( - "os.fs.read: `path` must be a non-empty string — received keys: patth; expected: path, maxBytes, offset, limit, lineNumbers; did you mean `path` instead of `patth`?", + expect(result.summary).toBe( + "os.fs.read: `path` must be a non-empty string — received keys: path; expected: path, maxBytes, offset, limit, lineNumbers", ); - expect(result.summary).not.toContain("secret-value"); - expect(result.details.receivedKeys).toEqual(["patth"]); + expect(result.details.receivedKeys).toEqual(["path"]); expect(result.details.expectedKeys).toEqual([ "path", "maxBytes", @@ -386,15 +390,15 @@ describe("executeBatch", () => { // Fast call when idx==2, slow otherwise — verifies that result // ordering is by batchIndex regardless of completion order. await new Promise((r) => - setTimeout(r, (args.idx as number) === 2 ? 5 : 60), + setTimeout(r, (args.offset as number) === 2 ? 5 : 60), ); - return okResult("os.fs.read", `done-${args.idx}`); + return okResult("os.fs.read", `done-${args.offset}`); }, }); const inputs = toBatchInputs([ - { tool: "os.fs.read", args: { idx: 0 } }, - { tool: "os.fs.read", args: { idx: 1 } }, - { tool: "os.fs.read", args: { idx: 2 } }, + { tool: "os.fs.read", args: { offset: 0 } }, + { tool: "os.fs.read", args: { offset: 1 } }, + { tool: "os.fs.read", args: { offset: 2 } }, ]); const out = await executeBatch( inputs, @@ -809,16 +813,16 @@ describe("executeBatch", () => { readonly: false, run: async (args) => { await new Promise((r) => setTimeout(r, 30)); - if ((args.idx as number) === 0) { + if ((args.ref as number) === 0) { ctrl.abort(); } return okResult("browser.click"); }, }); const inputs = toBatchInputs([ - { tool: "browser.click", args: { idx: 0 } }, - { tool: "browser.click", args: { idx: 1 } }, - { tool: "browser.click", args: { idx: 2 } }, + { tool: "browser.click", args: { ref: 0 } }, + { tool: "browser.click", args: { ref: 1 } }, + { tool: "browser.click", args: { ref: 2 } }, ]); const out = await executeBatch(inputs, registry, ctx(ctrl.signal)); expect(out.cancelled).toBe(true); @@ -1589,3 +1593,181 @@ describe("executeBatch refuses a corrupted call (F37)", () => { ]); }); }); + +describe("executeBatch refuses a call with unknown argument keys (F40)", () => { + /** The live Gemma 4 worker call: the script under a flag used as a key. */ + const LIVE_CALL = { + tool: "os.shell.run", + args: { cmd: "python3", "-e": "import os\nos.rename('a', 'b')" }, + }; + + it("does not run the call and answers with the error shape", async () => { + const run = vi.fn(async () => okResult("os.shell.run", "$ python3\nexit: 0")); + const registry = buildRegistry({ "os.shell.run": run }, false); + const out = await executeBatch( + toBatchInputs([LIVE_CALL]), + registry, + ctx(new AbortController().signal), + ); + expect(run).not.toHaveBeenCalled(); + const result = out.results[0]!.compressed!; + expect(result.status).toBe("error"); + expect(result.summary).toBe( + 'unknown argument `-e` for os.shell.run (expected: cmd, args, cwd, timeoutMs; put the script in args: ["-c", "…"]) — the call was not run; re-emit it with the right keys', + ); + expect(result.summary).not.toContain("rename"); + expect(result.details).toEqual({ + unknownKeys: ["-e"], + expectedKeys: ["cmd", "args", "cwd", "timeoutMs"], + }); + expect(out.cancelled).toBe(false); + }); + + it("names the key the model most likely meant", async () => { + const run = vi.fn(async () => okResult("os.shell.run")); + const registry = buildRegistry({ "os.shell.run": run }, false); + const out = await executeBatch( + toBatchInputs([ + { + tool: "os.shell.run", + args: { cmd: "python3", "-args": ["-c", "print(1)"] }, + }, + ]), + registry, + ctx(new AbortController().signal), + ); + expect(run).not.toHaveBeenCalled(); + expect(out.results[0]!.compressed!.summary).toBe( + "unknown argument `-args` for os.shell.run (expected: cmd, args, cwd, timeoutMs; did you mean `args`?) — the call was not run; re-emit it with the right keys", + ); + }); + + it("lands details.unknownKeys on the tool_invocation trace row", async () => { + const registry = buildRegistry( + { "os.shell.run": async () => okResult("os.shell.run") }, + false, + ); + const events: TraceEvent[] = []; + const recorder = createTraceRecorder({ + sessionId: "s1", + emit: (event) => events.push(event), + now: () => 0, + }); + recorder.onAgentEvent({ type: "turn_started", turnIndex: 0 }); + recorder.onAgentEvent({ type: "step_started", stepIndex: 0 }); + recorder.onAgentEvent({ + type: "llm_event", + event: { + type: "tool_call_parsed", + call: LIVE_CALL, + batchIndex: 0, + batchSize: 1, + }, + }); + await executeBatch(toBatchInputs([LIVE_CALL]), registry, { + ...ctx(new AbortController().signal), + onCallFinished: ({ result, batchIndex, batchSize }) => + recorder.onAgentEvent({ + type: "llm_event", + event: { type: "tool_call_executed", result, batchIndex, batchSize }, + }), + }); + const row = events.find((e) => e.type === "tool_invocation"); + expect(row).toMatchObject({ + type: "tool_invocation", + tool: "os.shell.run", + status: "error", + details: { unknownKeys: ["-e"] }, + }); + }); + + it("counts toward the loop detector like any other error", async () => { + const run = vi.fn(async () => okResult("os.shell.run")); + const registry = buildRegistry({ "os.shell.run": run }, false); + const tracker = new ToolLoopTracker({ criticalThreshold: 3 }); + const signals: BatchLoopSignal[] = []; + for (let i = 0; i < 4; i += 1) { + const out = await executeBatch(toBatchInputs([LIVE_CALL]), registry, { + ...ctx(new AbortController().signal), + tracker, + }); + signals.push(...out.loopSignals); + } + expect(run).not.toHaveBeenCalled(); + expect(signals.some((s) => s.kind === "critical")).toBe(true); + }); + + it("runs a valid call untouched", async () => { + const run = vi.fn(async () => okResult("os.shell.run", "$ ls -la\nexit: 0")); + const registry = buildRegistry({ "os.shell.run": run }, false); + const out = await executeBatch( + toBatchInputs([ + { tool: "os.shell.run", args: { cmd: "ls", args: ["-la"], cwd: "." } }, + ]), + registry, + ctx(new AbortController().signal), + ); + expect(run).toHaveBeenCalledTimes(1); + expect(run).toHaveBeenCalledWith({ cmd: "ls", args: ["-la"], cwd: "." }); + expect(out.results[0]!.compressed?.status).toBe("ok"); + }); + + it("runs a tool without a registered schema whatever its keys", async () => { + const run = vi.fn(async () => okResult("mcp.srv.search")); + const registry = buildRegistry({ "mcp.srv.search": run }); + const out = await executeBatch( + toBatchInputs([ + { tool: "mcp.srv.search", args: { query: "x", "-e": "y" } }, + ]), + registry, + ctx(new AbortController().signal), + ); + expect(run).toHaveBeenCalledWith({ query: "x", "-e": "y" }); + expect(out.results[0]!.compressed?.status).toBe("ok"); + }); + + it("does not refuse a quoted key that F33 normalises at dispatch", async () => { + const run = vi.fn(async () => okResult("os.fs.read")); + const registry = buildRegistry({ "os.fs.read": run }); + const out = await executeBatch( + toBatchInputs([{ tool: "os.fs.read", args: { '"path"': "a.txt" } }]), + registry, + ctx(new AbortController().signal), + ); + // The registry's own normalisation renamed the key before the tool ran. + expect(run).toHaveBeenCalledWith({ path: "a.txt" }); + expect(out.results[0]!.compressed?.status).toBe("ok"); + }); + + it("refuses only the unknown-key call of a batch; its siblings and the tail reply run", async () => { + const list = vi.fn(async () => okResult("os.fs.list")); + const read = vi.fn(async () => okResult("os.fs.read")); + const reply = vi.fn(async () => okResult("reply")); + const registry = buildRegistry({ + "os.fs.list": list, + "os.fs.read": read, + reply, + }); + const out = await executeBatch( + toBatchInputs([ + { tool: "os.fs.read", args: { path: "README.md" } }, + { tool: "os.fs.list", args: { Path: "." } }, + { tool: "reply", args: { text: "done", extra: "shown, not run" } }, + ]), + registry, + ctx(new AbortController().signal), + ); + expect(read).toHaveBeenCalledTimes(1); + expect(list).not.toHaveBeenCalled(); + // A terminal is never gated: the turn must be able to close. + expect(reply).toHaveBeenCalledTimes(1); + expect(out.results.map((r) => r.compressed?.status)).toEqual([ + "ok", + "error", + "ok", + ]); + expect(out.results[1]!.compressed!.summary).toContain( + "unknown argument `Path` for os.fs.list", + ); + }); +}); diff --git a/src/agent/batch-executor.ts b/src/agent/batch-executor.ts index d63383db..000f4e16 100644 --- a/src/agent/batch-executor.ts +++ b/src/agent/batch-executor.ts @@ -16,6 +16,7 @@ import { describeCorruptedCall, findControlMarkers, } from "../tools/control-marker-guard.js"; +import { findUnknownArguments } from "../tools/unknown-argument-guard.js"; import { CancelledError } from "../llm/index.js"; import { isParallelWithinGroup, @@ -471,30 +472,15 @@ export async function executeBatch( ctx.onCallStarted?.({ batchIndex: input.batchIndex, batchSize }); const startedAt = Date.now(); let compressed: CompressedToolResult; - // A call whose argument carries the model's own control markup (F37) - // never reaches the registry: a `path` holding `<|channel>` is a - // thought block that fell into the call, and the tool would run on - // the garbage (it listed an ENAMETOOLONG path as "empty" once, and - // the model overwrote the input file on that reading). The refusal - // is an ordinary error result — recorded in the loop tracker like - // any other, on the trace row via `details.corrupted` — that the - // model reads on its next step; no parse-recovery budget is spent. - // Terminals are exempt for the reason every gate exempts them: a - // reply's text is shown, not run, and the turn must be able to close. - const markers = + // A call that is not what the model meant never reaches the + // registry — see `refuseBeforeDispatch`. Terminals are exempt for + // the reason every gate exempts them: a reply's text is shown, not + // run, and the turn must be able to close. + const refusal = input.resourceClass === "terminal" - ? [] - : findControlMarkers(input.call.args, input.call.tool); - if (markers.length > 0) { - compressed = compressToolResult({ - tool: input.call.tool, - status: "error", - output: describeCorruptedCall(markers), - details: { corrupted: true, markers }, - }); - } else { - compressed = await invokeRegistry(input); - } + ? null + : refuseBeforeDispatch(input.call); + compressed = refusal ?? (await invokeRegistry(input)); const durationMs = Date.now() - startedAt; slots[input.batchIndex] = { ...slots[input.batchIndex]!, @@ -635,6 +621,53 @@ export async function executeBatch( }; } +/** + * The error result a call gets instead of running when its arguments + * are not what the model meant, or `null` when the call is clean. + * + * Two checks, in this order. A value carrying the model's own control + * markup (F37): a `path` holding `<|channel>` is a thought block that + * fell into the call, and the tool would run on the garbage (it listed + * an ENAMETOOLONG path as "empty" once, and the model overwrote the + * input file on that reading). Then a top-level key the tool's schema + * does not know (F40): `os.shell.run {"cmd":"python3","-e":"