From 922f4e66ea1af8963ec9c16b4c116c22bc5014be Mon Sep 17 00:00:00 2001 From: Mariano <132747814+mbelinky@users.noreply.github.com> Date: Tue, 24 Mar 2026 13:19:16 +0100 Subject: [PATCH] fix(agents): harden edit tool recovery (#52516) Merged via squash. Prepared head SHA: e23bde893a73b150cc821b62c195928e67560985 Co-authored-by: mbelinky <132747814+mbelinky@users.noreply.github.com> Co-authored-by: mbelinky <132747814+mbelinky@users.noreply.github.com> Reviewed-by: @mbelinky --- CHANGELOG.md | 4 + ...-embedded-subscribe.handlers.tools.test.ts | 60 +++++ ...aliases-schemas-without-dropping-f.test.ts | 31 +++ src/agents/pi-tools.host-edit.ts | 195 ++++++++++---- src/agents/pi-tools.params.ts | 50 ++-- .../pi-tools.read.host-edit-recovery.test.ts | 243 +++++++++++++----- src/agents/pi-tools.read.ts | 14 +- src/agents/tool-mutation.test.ts | 16 ++ src/agents/tool-mutation.ts | 60 +++-- 9 files changed, 524 insertions(+), 149 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 056a417a7ee..f3ab0e32573 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -407,6 +407,10 @@ Docs: https://docs.openclaw.ai - Slack/embedded delivery: suppress transcript-only `delivery-mirror` assistant messages before embedded re-delivery and raise the default Slack chunk fallback so messages just over 4000 characters stay in a single post. (#45489) Thanks @theo674. - Slack/embedded delivery: suppress transcript-only `delivery-mirror` assistant messages before embedded re-delivery and raise the default Slack chunk fallback so messages just over 4000 characters stay in a single post. (#45489) Thanks @theo674. +### Fixes + +- Agents/edit tool: accept common path/text alias spellings, show current file contents on exact-match failures, and avoid false edit failures after successful writes. (#52516) thanks @mbelinky. + ## 2026.3.13 ### Changes diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts index ec9b52aa6c5..c41dba3b9fb 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts @@ -178,6 +178,66 @@ describe("handleToolExecutionEnd cron.add commitment tracking", () => { }); }); +describe("handleToolExecutionEnd mutating failure recovery", () => { + it("clears edit failure when the retry succeeds through common file path aliases", async () => { + const { ctx } = createTestContext(); + + await handleToolExecutionStart( + ctx as never, + { + type: "tool_execution_start", + toolName: "edit", + toolCallId: "tool-edit-1", + args: { + file_path: "/tmp/demo.txt", + old_string: "beta stale", + new_string: "beta fixed", + }, + } as never, + ); + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "edit", + toolCallId: "tool-edit-1", + isError: true, + result: { error: "Could not find the exact text in /tmp/demo.txt" }, + } as never, + ); + + expect(ctx.state.lastToolError?.toolName).toBe("edit"); + + await handleToolExecutionStart( + ctx as never, + { + type: "tool_execution_start", + toolName: "edit", + toolCallId: "tool-edit-2", + args: { + file: "/tmp/demo.txt", + oldText: "beta", + newText: "beta fixed", + }, + } as never, + ); + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "edit", + toolCallId: "tool-edit-2", + isError: false, + result: { ok: true }, + } as never, + ); + + expect(ctx.state.lastToolError).toBeUndefined(); + }); +}); + describe("handleToolExecutionEnd exec approval prompts", () => { it("emits a deterministic approval payload and marks assistant output suppressed", async () => { const { ctx } = createTestContext(); diff --git a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts index c1aba0b928e..d2b5620e9d7 100644 --- a/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts +++ b/src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts @@ -39,6 +39,37 @@ describe("createOpenClawCodingTools", () => { } }); + it("accepts broader file/edit alias variants", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-alias-broad-")); + try { + const tools = createOpenClawCodingTools({ workspaceDir: tmpDir }); + const { readTool, writeTool, editTool } = expectReadWriteEditTools(tools); + + await writeTool?.execute("tool-alias-broad-1", { + file: "broad-alias.txt", + content: "hello old value", + }); + + await editTool?.execute("tool-alias-broad-2", { + filePath: "broad-alias.txt", + old_text: "old", + newString: "new", + }); + + const result = await readTool?.execute("tool-alias-broad-3", { + file: "broad-alias.txt", + }); + + const textBlocks = result?.content?.filter((block) => block.type === "text") as + | Array<{ text?: string }> + | undefined; + const combinedText = textBlocks?.map((block) => block.text ?? "").join("\n"); + expect(combinedText).toContain("hello new value"); + } finally { + await fs.rm(tmpDir, { recursive: true, force: true }); + } + }); + it("coerces structured content blocks for write", async () => { const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-structured-write-")); try { diff --git a/src/agents/pi-tools.host-edit.ts b/src/agents/pi-tools.host-edit.ts index f58d391de76..bc8ab0c2054 100644 --- a/src/agents/pi-tools.host-edit.ts +++ b/src/agents/pi-tools.host-edit.ts @@ -1,11 +1,24 @@ -import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import type { AgentToolResult, AgentToolUpdateCallback } from "@mariozechner/pi-agent-core"; import type { AnyAgentTool } from "./pi-tools.types.js"; -/** Resolve path for host edit: expand ~ and resolve relative paths against root. */ -function resolveHostEditPath(root: string, pathParam: string): string { +type EditToolRecoveryOptions = { + root: string; + readFile: (absolutePath: string) => Promise; +}; + +type EditToolParams = { + pathParam?: string; + oldText?: string; + newText?: string; +}; + +const EDIT_MISMATCH_MESSAGE = "Could not find the exact text in"; +const EDIT_MISMATCH_HINT_LIMIT = 800; + +/** Resolve path for edit recovery: expand ~ and resolve relative paths against root. */ +function resolveEditPath(root: string, pathParam: string): string { const expanded = pathParam.startsWith("~/") || pathParam === "~" ? pathParam.replace(/^~/, os.homedir()) @@ -13,15 +26,103 @@ function resolveHostEditPath(root: string, pathParam: string): string { return path.isAbsolute(expanded) ? path.resolve(expanded) : path.resolve(root, expanded); } +function readStringParam(record: Record | undefined, ...keys: string[]) { + for (const key of keys) { + const value = record?.[key]; + if (typeof value === "string") { + return value; + } + } + return undefined; +} + +function readEditToolParams(params: unknown): EditToolParams { + const record = + params && typeof params === "object" ? (params as Record) : undefined; + return { + pathParam: readStringParam(record, "path", "file_path", "file"), + oldText: readStringParam(record, "oldText", "old_string", "old_text", "oldString"), + newText: readStringParam(record, "newText", "new_string", "new_text", "newString"), + }; +} + +function normalizeToLF(value: string): string { + return value.replace(/\r\n?/g, "\n"); +} + +function removeExactOccurrences(content: string, needle: string): string { + return needle.length > 0 ? content.split(needle).join("") : content; +} + +function didEditLikelyApply(params: { + originalContent?: string; + currentContent: string; + oldText?: string; + newText: string; +}) { + const normalizedCurrent = normalizeToLF(params.currentContent); + const normalizedNew = normalizeToLF(params.newText); + const normalizedOld = + typeof params.oldText === "string" && params.oldText.length > 0 + ? normalizeToLF(params.oldText) + : undefined; + const normalizedOriginal = + typeof params.originalContent === "string" ? normalizeToLF(params.originalContent) : undefined; + + if (normalizedOriginal !== undefined && normalizedOriginal === normalizedCurrent) { + return false; + } + + if (normalizedNew.length > 0 && !normalizedCurrent.includes(normalizedNew)) { + return false; + } + + const withoutInsertedNewText = + normalizedNew.length > 0 + ? removeExactOccurrences(normalizedCurrent, normalizedNew) + : normalizedCurrent; + if (normalizedOld && withoutInsertedNewText.includes(normalizedOld)) { + return false; + } + + return true; +} + +function buildEditSuccessResult(pathParam: string): AgentToolResult { + return { + isError: false, + content: [ + { + type: "text", + text: `Successfully replaced text in ${pathParam}.`, + }, + ], + details: { diff: "", firstChangedLine: undefined }, + } as AgentToolResult; +} + +function shouldAddMismatchHint(error: unknown) { + return error instanceof Error && error.message.includes(EDIT_MISMATCH_MESSAGE); +} + +function appendMismatchHint(error: Error, currentContent: string): Error { + const snippet = + currentContent.length <= EDIT_MISMATCH_HINT_LIMIT + ? currentContent + : `${currentContent.slice(0, EDIT_MISMATCH_HINT_LIMIT)}\n... (truncated)`; + const enhanced = new Error(`${error.message}\nCurrent file contents:\n${snippet}`); + enhanced.stack = error.stack; + return enhanced; +} + /** - * When the upstream edit tool throws after having already written (e.g. generateDiffString fails), - * the file may be correctly updated but the tool reports failure. This wrapper catches errors and - * if the target file on disk contains the intended newText, returns success so we don't surface - * a false "edit failed" to the user (fixes #32333, same pattern as #30773 for write). + * Recover from two edit-tool failure classes without changing edit semantics: + * - exact-match mismatch errors become actionable by including current file contents + * - post-write throws are converted back to success only if the file actually changed */ -export function wrapHostEditToolWithPostWriteRecovery( +export function wrapEditToolWithRecovery( base: AnyAgentTool, - root: string, + options: EditToolRecoveryOptions, ): AnyAgentTool { return { ...base, @@ -31,50 +132,54 @@ export function wrapHostEditToolWithPostWriteRecovery( signal: AbortSignal | undefined, onUpdate?: AgentToolUpdateCallback, ) => { + const { pathParam, oldText, newText } = readEditToolParams(params); + const absolutePath = + typeof pathParam === "string" ? resolveEditPath(options.root, pathParam) : undefined; + let originalContent: string | undefined; + + if (absolutePath && newText !== undefined) { + try { + originalContent = await options.readFile(absolutePath); + } catch { + // Best-effort snapshot only; recovery should still proceed without it. + } + } + try { return await base.execute(toolCallId, params, signal, onUpdate); } catch (err) { - const record = - params && typeof params === "object" ? (params as Record) : undefined; - const pathParam = record && typeof record.path === "string" ? record.path : undefined; - const newText = - record && typeof record.newText === "string" - ? record.newText - : record && typeof record.new_string === "string" - ? record.new_string - : undefined; - const oldText = - record && typeof record.oldText === "string" - ? record.oldText - : record && typeof record.old_string === "string" - ? record.old_string - : undefined; - if (!pathParam || !newText) { + if (!absolutePath) { throw err; } + + let currentContent: string | undefined; try { - const absolutePath = resolveHostEditPath(root, pathParam); - const content = await fs.readFile(absolutePath, "utf-8"); - // Only recover when the replacement likely occurred: newText is present and oldText - // is no longer present. This avoids false success when upstream threw before writing - // (e.g. oldText not found) but the file already contained newText (review feedback). - const hasNew = content.includes(newText); - const stillHasOld = - oldText !== undefined && oldText.length > 0 && content.includes(oldText); - if (hasNew && !stillHasOld) { - return { - content: [ - { - type: "text", - text: `Successfully replaced text in ${pathParam}.`, - }, - ], - details: { diff: "", firstChangedLine: undefined }, - } as AgentToolResult; - } + currentContent = await options.readFile(absolutePath); } catch { - // File read failed or path invalid; rethrow original error. + // Fall through to the original error if readback fails. } + + if (typeof currentContent === "string" && newText !== undefined) { + if ( + didEditLikelyApply({ + originalContent, + currentContent, + oldText, + newText, + }) + ) { + return buildEditSuccessResult(pathParam ?? absolutePath); + } + } + + if ( + typeof currentContent === "string" && + err instanceof Error && + shouldAddMismatchHint(err) + ) { + throw appendMismatchHint(err, currentContent); + } + throw err; } }, diff --git a/src/agents/pi-tools.params.ts b/src/agents/pi-tools.params.ts index 9dda99a2a86..4bd2aeed9ac 100644 --- a/src/agents/pi-tools.params.ts +++ b/src/agents/pi-tools.params.ts @@ -13,20 +13,20 @@ function parameterValidationError(message: string): Error { } export const CLAUDE_PARAM_GROUPS = { - read: [{ keys: ["path", "file_path"], label: "path (path or file_path)" }], + read: [{ keys: ["path", "file_path", "filePath", "file"], label: "path alias" }], write: [ - { keys: ["path", "file_path"], label: "path (path or file_path)" }, + { keys: ["path", "file_path", "filePath", "file"], label: "path alias" }, { keys: ["content"], label: "content" }, ], edit: [ - { keys: ["path", "file_path"], label: "path (path or file_path)" }, + { keys: ["path", "file_path", "filePath", "file"], label: "path alias" }, { - keys: ["oldText", "old_string"], - label: "oldText (oldText or old_string)", + keys: ["oldText", "old_string", "old_text", "oldString"], + label: "oldText alias", }, { - keys: ["newText", "new_string"], - label: "newText (newText or new_string)", + keys: ["newText", "new_string", "new_text", "newString"], + label: "newText alias", allowEmpty: true, }, ], @@ -91,20 +91,22 @@ export function normalizeToolParams(params: unknown): Record | } const record = params as Record; const normalized = { ...record }; - // file_path → path (read, write, edit) - if ("file_path" in normalized && !("path" in normalized)) { - normalized.path = normalized.file_path; - delete normalized.file_path; - } - // old_string → oldText (edit) - if ("old_string" in normalized && !("oldText" in normalized)) { - normalized.oldText = normalized.old_string; - delete normalized.old_string; - } - // new_string → newText (edit) - if ("new_string" in normalized && !("newText" in normalized)) { - normalized.newText = normalized.new_string; - delete normalized.new_string; + const aliasPairs: Array<{ original: string; alias: string }> = [ + { original: "path", alias: "file_path" }, + { original: "path", alias: "filePath" }, + { original: "path", alias: "file" }, + { original: "oldText", alias: "old_string" }, + { original: "oldText", alias: "old_text" }, + { original: "oldText", alias: "oldString" }, + { original: "newText", alias: "new_string" }, + { original: "newText", alias: "new_text" }, + { original: "newText", alias: "newString" }, + ]; + for (const { original, alias } of aliasPairs) { + if (alias in normalized && !(original in normalized)) { + normalized[original] = normalized[alias]; + } + delete normalized[alias]; } // Some providers/models emit text payloads as structured blocks instead of raw strings. // Normalize these for write/edit so content matching and writes stay deterministic. @@ -132,8 +134,14 @@ export function patchToolSchemaForClaudeCompatibility(tool: AnyAgentTool): AnyAg const aliasPairs: Array<{ original: string; alias: string }> = [ { original: "path", alias: "file_path" }, + { original: "path", alias: "filePath" }, + { original: "path", alias: "file" }, { original: "oldText", alias: "old_string" }, + { original: "oldText", alias: "old_text" }, + { original: "oldText", alias: "oldString" }, { original: "newText", alias: "new_string" }, + { original: "newText", alias: "new_text" }, + { original: "newText", alias: "newString" }, ]; for (const { original, alias } of aliasPairs) { diff --git a/src/agents/pi-tools.read.host-edit-recovery.test.ts b/src/agents/pi-tools.read.host-edit-recovery.test.ts index ebe41b26f72..297e47aa604 100644 --- a/src/agents/pi-tools.read.host-edit-recovery.test.ts +++ b/src/agents/pi-tools.read.host-edit-recovery.test.ts @@ -1,95 +1,216 @@ -/** - * Tests for edit tool post-write recovery: when the upstream library throws after - * having already written the file (e.g. generateDiffString fails), we catch and - * if the file on disk contains the intended newText we return success (#32333). - */ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import type { EditToolOptions } from "@mariozechner/pi-coding-agent"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; +import { wrapEditToolWithRecovery } from "./pi-tools.host-edit.js"; +import type { AnyAgentTool } from "./pi-tools.types.js"; +import type { SandboxFsBridge, SandboxFsStat } from "./sandbox/fs-bridge.js"; -const mocks = vi.hoisted(() => ({ - executeThrows: true, -})); +function createInMemoryBridge(root: string, files: Map): SandboxFsBridge { + const resolveAbsolute = (filePath: string, cwd?: string) => + path.isAbsolute(filePath) ? path.resolve(filePath) : path.resolve(cwd ?? root, filePath); + + const readStat = (absolutePath: string): SandboxFsStat | null => { + const content = files.get(absolutePath); + if (typeof content !== "string") { + return null; + } + return { + type: "file", + size: Buffer.byteLength(content, "utf8"), + mtimeMs: 0, + }; + }; -vi.mock("@mariozechner/pi-coding-agent", async (importOriginal) => { - const actual = await importOriginal(); return { - ...actual, - createEditTool: (cwd: string, options?: EditToolOptions) => { - const base = actual.createEditTool(cwd, options); + resolvePath: ({ filePath, cwd }) => { + const absolutePath = resolveAbsolute(filePath, cwd); return { - ...base, - execute: async (...args: Parameters) => { - if (mocks.executeThrows) { - throw new Error("Simulated post-write failure (e.g. generateDiffString)"); - } - return base.execute(...args); - }, + hostPath: absolutePath, + relativePath: path.relative(root, absolutePath), + containerPath: absolutePath, }; }, + readFile: async ({ filePath, cwd }) => { + const absolutePath = resolveAbsolute(filePath, cwd); + const content = files.get(absolutePath); + if (typeof content !== "string") { + throw new Error(`ENOENT: ${absolutePath}`); + } + return Buffer.from(content, "utf8"); + }, + writeFile: async ({ filePath, cwd, data }) => { + const absolutePath = resolveAbsolute(filePath, cwd); + files.set(absolutePath, typeof data === "string" ? data : Buffer.from(data).toString("utf8")); + }, + mkdirp: async () => {}, + remove: async ({ filePath, cwd }) => { + files.delete(resolveAbsolute(filePath, cwd)); + }, + rename: async ({ from, to, cwd }) => { + const fromPath = resolveAbsolute(from, cwd); + const toPath = resolveAbsolute(to, cwd); + const content = files.get(fromPath); + if (typeof content !== "string") { + throw new Error(`ENOENT: ${fromPath}`); + } + files.set(toPath, content); + files.delete(fromPath); + }, + stat: async ({ filePath, cwd }) => readStat(resolveAbsolute(filePath, cwd)), }; -}); +} -let createHostWorkspaceEditTool: typeof import("./pi-tools.read.js").createHostWorkspaceEditTool; - -describe("createHostWorkspaceEditTool post-write recovery", () => { +describe("edit tool recovery hardening", () => { let tmpDir = ""; - beforeEach(async () => { - vi.resetModules(); - ({ createHostWorkspaceEditTool } = await import("./pi-tools.read.js")); - mocks.executeThrows = true; - }); - afterEach(async () => { - mocks.executeThrows = true; if (tmpDir) { await fs.rm(tmpDir, { recursive: true, force: true }); tmpDir = ""; } }); - it("returns success when upstream throws but file has newText and no longer has oldText", async () => { + function createRecoveredEditTool(params: { + root: string; + readFile: (absolutePath: string) => Promise; + execute: AnyAgentTool["execute"]; + }) { + const base = { + name: "edit", + execute: params.execute, + } as unknown as AnyAgentTool; + return wrapEditToolWithRecovery(base, { + root: params.root, + readFile: params.readFile, + }); + } + + it("adds current file contents to exact-match mismatch errors", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-edit-recovery-")); - const filePath = path.join(tmpDir, "MEMORY.md"); - const oldText = "# Memory"; - const newText = "Blog Writing"; - await fs.writeFile(filePath, `\n\n${newText}\n`, "utf-8"); + const filePath = path.join(tmpDir, "demo.txt"); + await fs.writeFile(filePath, "actual current content", "utf-8"); - const tool = createHostWorkspaceEditTool(tmpDir); - const result = await tool.execute("call-1", { path: filePath, oldText, newText }, undefined); - - expect(result).toBeDefined(); - const content = Array.isArray((result as { content?: unknown }).content) - ? (result as { content: Array<{ type?: string; text?: string }> }).content - : []; - const textBlock = content.find((b) => b?.type === "text" && typeof b.text === "string"); - expect(textBlock?.text).toContain("Successfully replaced text"); + const tool = createRecoveredEditTool({ + root: tmpDir, + readFile: (absolutePath) => fs.readFile(absolutePath, "utf-8"), + execute: async () => { + throw new Error( + "Could not find the exact text in demo.txt. The old text must match exactly including all whitespace and newlines.", + ); + }, + }); + await expect( + tool.execute( + "call-1", + { path: filePath, oldText: "missing", newText: "replacement" }, + undefined, + ), + ).rejects.toThrow(/Current file contents:\nactual current content/); }); - it("rethrows when file on disk does not contain newText", async () => { + it("recovers success after a post-write throw when CRLF output contains newText and oldText is only a substring", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-edit-recovery-")); - const filePath = path.join(tmpDir, "other.md"); - await fs.writeFile(filePath, "unchanged content", "utf-8"); + const filePath = path.join(tmpDir, "demo.txt"); + await fs.writeFile(filePath, 'const value = "foo";\r\n', "utf-8"); - const tool = createHostWorkspaceEditTool(tmpDir); + const tool = createRecoveredEditTool({ + root: tmpDir, + readFile: (absolutePath) => fs.readFile(absolutePath, "utf-8"), + execute: async () => { + await fs.writeFile(filePath, 'const value = "foobar";\r\n', "utf-8"); + throw new Error("Simulated post-write failure (e.g. generateDiffString)"); + }, + }); + const result = await tool.execute( + "call-1", + { + path: filePath, + oldText: 'const value = "foo";\n', + newText: 'const value = "foobar";\n', + }, + undefined, + ); + + expect(result).toMatchObject({ isError: false }); + expect(result.content[0]).toMatchObject({ + type: "text", + text: `Successfully replaced text in ${filePath}.`, + }); + }); + + it("does not recover false success when the file never changed", async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-edit-recovery-")); + const filePath = path.join(tmpDir, "demo.txt"); + await fs.writeFile(filePath, "replacement already present", "utf-8"); + + const tool = createRecoveredEditTool({ + root: tmpDir, + readFile: (absolutePath) => fs.readFile(absolutePath, "utf-8"), + execute: async () => { + throw new Error("Simulated post-write failure (e.g. generateDiffString)"); + }, + }); await expect( - tool.execute("call-1", { path: filePath, oldText: "x", newText: "never-written" }, undefined), + tool.execute( + "call-1", + { path: filePath, oldText: "missing", newText: "replacement already present" }, + undefined, + ), ).rejects.toThrow("Simulated post-write failure"); }); - it("rethrows when file still contains oldText (pre-write failure; avoid false success)", async () => { + it("recovers deletion edits when the file changed and oldText is gone", async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-edit-recovery-")); - const filePath = path.join(tmpDir, "pre-write-fail.md"); - const oldText = "replace me"; - const newText = "new content"; - await fs.writeFile(filePath, `before ${oldText} after ${newText}`, "utf-8"); + const filePath = path.join(tmpDir, "demo.txt"); + await fs.writeFile(filePath, "before delete me after\n", "utf-8"); - const tool = createHostWorkspaceEditTool(tmpDir); - await expect( - tool.execute("call-1", { path: filePath, oldText, newText }, undefined), - ).rejects.toThrow("Simulated post-write failure"); + const tool = createRecoveredEditTool({ + root: tmpDir, + readFile: (absolutePath) => fs.readFile(absolutePath, "utf-8"), + execute: async () => { + await fs.writeFile(filePath, "before after\n", "utf-8"); + throw new Error("Simulated post-write failure (e.g. generateDiffString)"); + }, + }); + const result = await tool.execute( + "call-1", + { path: filePath, oldText: "delete me", newText: "" }, + undefined, + ); + + expect(result).toMatchObject({ isError: false }); + expect(result.content[0]).toMatchObject({ + type: "text", + text: `Successfully replaced text in ${filePath}.`, + }); + }); + + it("applies the same recovery path to sandboxed edit tools", async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-edit-recovery-")); + const filePath = path.join(tmpDir, "demo.txt"); + const files = new Map([[filePath, "before old text after\n"]]); + + const bridge = createInMemoryBridge(tmpDir, files); + const tool = createRecoveredEditTool({ + root: tmpDir, + readFile: async (absolutePath: string) => + (await bridge.readFile({ filePath: absolutePath, cwd: tmpDir })).toString("utf8"), + execute: async () => { + files.set(filePath, "before new text after\n"); + throw new Error("Simulated post-write failure (e.g. generateDiffString)"); + }, + }); + const result = await tool.execute( + "call-1", + { path: filePath, oldText: "old text", newText: "new text" }, + undefined, + ); + + expect(result).toMatchObject({ isError: false }); + expect(result.content[0]).toMatchObject({ + type: "text", + text: `Successfully replaced text in ${filePath}.`, + }); }); }); diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index 91951fe2c10..c78a865f52e 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -14,7 +14,7 @@ import { detectMime } from "../media/mime.js"; import { sniffMimeFromBase64 } from "../media/sniff-mime-from-base64.js"; import type { ImageSanitizationLimits } from "./image-sanitization.js"; import { toRelativeWorkspacePath } from "./path-policy.js"; -import { wrapHostEditToolWithPostWriteRecovery } from "./pi-tools.host-edit.js"; +import { wrapEditToolWithRecovery } from "./pi-tools.host-edit.js"; import { CLAUDE_PARAM_GROUPS, assertRequiredParams, @@ -607,7 +607,12 @@ export function createSandboxedEditTool(params: SandboxToolParams) { const base = createEditTool(params.root, { operations: createSandboxEditOperations(params), }) as unknown as AnyAgentTool; - return wrapToolParamNormalization(base, CLAUDE_PARAM_GROUPS.edit); + const withRecovery = wrapEditToolWithRecovery(base, { + root: params.root, + readFile: async (absolutePath: string) => + (await params.bridge.readFile({ filePath: absolutePath, cwd: params.root })).toString("utf8"), + }); + return wrapToolParamNormalization(withRecovery, CLAUDE_PARAM_GROUPS.edit); } export function createHostWorkspaceWriteTool(root: string, options?: { workspaceOnly?: boolean }) { @@ -621,7 +626,10 @@ export function createHostWorkspaceEditTool(root: string, options?: { workspaceO const base = createEditTool(root, { operations: createHostEditOperations(root, options), }) as unknown as AnyAgentTool; - const withRecovery = wrapHostEditToolWithPostWriteRecovery(base, root); + const withRecovery = wrapEditToolWithRecovery(base, { + root, + readFile: (absolutePath: string) => fs.readFile(absolutePath, "utf-8"), + }); return wrapToolParamNormalization(withRecovery, CLAUDE_PARAM_GROUPS.edit); } diff --git a/src/agents/tool-mutation.test.ts b/src/agents/tool-mutation.test.ts index ab618f8da5a..9ae7f7148e2 100644 --- a/src/agents/tool-mutation.test.ts +++ b/src/agents/tool-mutation.test.ts @@ -37,6 +37,22 @@ describe("tool mutation helpers", () => { expect(readFingerprint).toBeUndefined(); }); + it("treats coding-tool path aliases as the same stable target", () => { + const filePathFingerprint = buildToolActionFingerprint("edit", { + file_path: "/tmp/demo.txt", + old_string: "before", + new_string: "after", + }); + const fileAliasFingerprint = buildToolActionFingerprint("edit", { + file: "/tmp/demo.txt", + oldText: "before", + newText: "after again", + }); + + expect(filePathFingerprint).toBe("tool=edit|path=/tmp/demo.txt"); + expect(fileAliasFingerprint).toBe("tool=edit|path=/tmp/demo.txt"); + }); + it("exposes mutation state for downstream payload rendering", () => { expect( buildToolMutationState("message", { action: "send", to: "telegram:1" }).mutatingAction, diff --git a/src/agents/tool-mutation.ts b/src/agents/tool-mutation.ts index a88bbadfd21..33bff88c8d8 100644 --- a/src/agents/tool-mutation.ts +++ b/src/agents/tool-mutation.ts @@ -82,6 +82,23 @@ function normalizeFingerprintValue(value: unknown): string | undefined { return undefined; } +function appendFingerprintAlias( + parts: string[], + record: Record | undefined, + label: string, + keys: string[], +): boolean { + for (const key of keys) { + const value = normalizeFingerprintValue(record?.[key]); + if (!value) { + continue; + } + parts.push(`${label}=${value}`); + return true; + } + return false; +} + export function isLikelyMutatingToolName(toolName: string): boolean { const normalized = toolName.trim().toLowerCase(); if (!normalized) { @@ -152,25 +169,30 @@ export function buildToolActionFingerprint( parts.push(`action=${action}`); } let hasStableTarget = false; - for (const key of [ - "path", - "filePath", - "oldPath", - "newPath", - "to", - "target", - "messageId", - "sessionKey", - "jobId", - "id", - "model", - ]) { - const value = normalizeFingerprintValue(record?.[key]); - if (value) { - parts.push(`${key.toLowerCase()}=${value}`); - hasStableTarget = true; - } - } + hasStableTarget = + appendFingerprintAlias(parts, record, "path", [ + "path", + "file_path", + "filePath", + "filepath", + "file", + ]) || hasStableTarget; + hasStableTarget = + appendFingerprintAlias(parts, record, "oldpath", ["oldPath", "old_path"]) || hasStableTarget; + hasStableTarget = + appendFingerprintAlias(parts, record, "newpath", ["newPath", "new_path"]) || hasStableTarget; + hasStableTarget = + appendFingerprintAlias(parts, record, "to", ["to", "target"]) || hasStableTarget; + hasStableTarget = + appendFingerprintAlias(parts, record, "messageid", ["messageId", "message_id"]) || + hasStableTarget; + hasStableTarget = + appendFingerprintAlias(parts, record, "sessionkey", ["sessionKey", "session_key"]) || + hasStableTarget; + hasStableTarget = + appendFingerprintAlias(parts, record, "jobid", ["jobId", "job_id"]) || hasStableTarget; + hasStableTarget = appendFingerprintAlias(parts, record, "id", ["id"]) || hasStableTarget; + hasStableTarget = appendFingerprintAlias(parts, record, "model", ["model"]) || hasStableTarget; const normalizedMeta = meta?.trim().replace(/\s+/g, " ").toLowerCase(); // Meta text often carries volatile details (for example "N chars"). // Prefer stable arg-derived keys for matching; only fall back to meta