Skip to content

Commit 922f4e6

Browse files
mbelinkymbelinky
authored
fix(agents): harden edit tool recovery (#52516)
Merged via squash. Prepared head SHA: e23bde8 Co-authored-by: mbelinky <[email protected]> Co-authored-by: mbelinky <[email protected]> Reviewed-by: @mbelinky
1 parent 60cd98a commit 922f4e6

9 files changed

Lines changed: 525 additions & 150 deletions

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,10 @@ Docs: https://docs.openclaw.ai
407407
- 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.
408408
- 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.
409409

410+
### Fixes
411+
412+
- 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.
413+
410414
## 2026.3.13
411415

412416
### Changes

‎src/agents/pi-embedded-subscribe.handlers.tools.test.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,66 @@ describe("handleToolExecutionEnd cron.add commitment tracking", () => {
178178
});
179179
});
180180

181+
describe("handleToolExecutionEnd mutating failure recovery", () => {
182+
it("clears edit failure when the retry succeeds through common file path aliases", async () => {
183+
const { ctx } = createTestContext();
184+
185+
await handleToolExecutionStart(
186+
ctx as never,
187+
{
188+
type: "tool_execution_start",
189+
toolName: "edit",
190+
toolCallId: "tool-edit-1",
191+
args: {
192+
file_path: "/tmp/demo.txt",
193+
old_string: "beta stale",
194+
new_string: "beta fixed",
195+
},
196+
} as never,
197+
);
198+
199+
await handleToolExecutionEnd(
200+
ctx as never,
201+
{
202+
type: "tool_execution_end",
203+
toolName: "edit",
204+
toolCallId: "tool-edit-1",
205+
isError: true,
206+
result: { error: "Could not find the exact text in /tmp/demo.txt" },
207+
} as never,
208+
);
209+
210+
expect(ctx.state.lastToolError?.toolName).toBe("edit");
211+
212+
await handleToolExecutionStart(
213+
ctx as never,
214+
{
215+
type: "tool_execution_start",
216+
toolName: "edit",
217+
toolCallId: "tool-edit-2",
218+
args: {
219+
file: "/tmp/demo.txt",
220+
oldText: "beta",
221+
newText: "beta fixed",
222+
},
223+
} as never,
224+
);
225+
226+
await handleToolExecutionEnd(
227+
ctx as never,
228+
{
229+
type: "tool_execution_end",
230+
toolName: "edit",
231+
toolCallId: "tool-edit-2",
232+
isError: false,
233+
result: { ok: true },
234+
} as never,
235+
);
236+
237+
expect(ctx.state.lastToolError).toBeUndefined();
238+
});
239+
});
240+
181241
describe("handleToolExecutionEnd exec approval prompts", () => {
182242
it("emits a deterministic approval payload and marks assistant output suppressed", async () => {
183243
const { ctx } = createTestContext();

‎src/agents/pi-tools.create-openclaw-coding-tools.adds-claude-style-aliases-schemas-without-dropping-f.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,37 @@ describe("createOpenClawCodingTools", () => {
3939
}
4040
});
4141

42+
it("accepts broader file/edit alias variants", async () => {
43+
const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-alias-broad-"));
44+
try {
45+
const tools = createOpenClawCodingTools({ workspaceDir: tmpDir });
46+
const { readTool, writeTool, editTool } = expectReadWriteEditTools(tools);
47+
48+
await writeTool?.execute("tool-alias-broad-1", {
49+
file: "broad-alias.txt",
50+
content: "hello old value",
51+
});
52+
53+
await editTool?.execute("tool-alias-broad-2", {
54+
filePath: "broad-alias.txt",
55+
old_text: "old",
56+
newString: "new",
57+
});
58+
59+
const result = await readTool?.execute("tool-alias-broad-3", {
60+
file: "broad-alias.txt",
61+
});
62+
63+
const textBlocks = result?.content?.filter((block) => block.type === "text") as
64+
| Array<{ text?: string }>
65+
| undefined;
66+
const combinedText = textBlocks?.map((block) => block.text ?? "").join("\n");
67+
expect(combinedText).toContain("hello new value");
68+
} finally {
69+
await fs.rm(tmpDir, { recursive: true, force: true });
70+
}
71+
});
72+
4273
it("coerces structured content blocks for write", async () => {
4374
const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-structured-write-"));
4475
try {

‎src/agents/pi-tools.host-edit.ts‎

Lines changed: 150 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,128 @@
1-
import fs from "node:fs/promises";
21
import os from "node:os";
32
import path from "node:path";
43
import type { AgentToolResult, AgentToolUpdateCallback } from "@mariozechner/pi-agent-core";
54
import type { AnyAgentTool } from "./pi-tools.types.js";
65

7-
/** Resolve path for host edit: expand ~ and resolve relative paths against root. */
8-
function resolveHostEditPath(root: string, pathParam: string): string {
6+
type EditToolRecoveryOptions = {
7+
root: string;
8+
readFile: (absolutePath: string) => Promise<string>;
9+
};
10+
11+
type EditToolParams = {
12+
pathParam?: string;
13+
oldText?: string;
14+
newText?: string;
15+
};
16+
17+
const EDIT_MISMATCH_MESSAGE = "Could not find the exact text in";
18+
const EDIT_MISMATCH_HINT_LIMIT = 800;
19+
20+
/** Resolve path for edit recovery: expand ~ and resolve relative paths against root. */
21+
function resolveEditPath(root: string, pathParam: string): string {
922
const expanded =
1023
pathParam.startsWith("~/") || pathParam === "~"
1124
? pathParam.replace(/^~/, os.homedir())
1225
: pathParam;
1326
return path.isAbsolute(expanded) ? path.resolve(expanded) : path.resolve(root, expanded);
1427
}
1528

29+
function readStringParam(record: Record<string, unknown> | undefined, ...keys: string[]) {
30+
for (const key of keys) {
31+
const value = record?.[key];
32+
if (typeof value === "string") {
33+
return value;
34+
}
35+
}
36+
return undefined;
37+
}
38+
39+
function readEditToolParams(params: unknown): EditToolParams {
40+
const record =
41+
params && typeof params === "object" ? (params as Record<string, unknown>) : undefined;
42+
return {
43+
pathParam: readStringParam(record, "path", "file_path", "file"),
44+
oldText: readStringParam(record, "oldText", "old_string", "old_text", "oldString"),
45+
newText: readStringParam(record, "newText", "new_string", "new_text", "newString"),
46+
};
47+
}
48+
49+
function normalizeToLF(value: string): string {
50+
return value.replace(/\r\n?/g, "\n");
51+
}
52+
53+
function removeExactOccurrences(content: string, needle: string): string {
54+
return needle.length > 0 ? content.split(needle).join("") : content;
55+
}
56+
57+
function didEditLikelyApply(params: {
58+
originalContent?: string;
59+
currentContent: string;
60+
oldText?: string;
61+
newText: string;
62+
}) {
63+
const normalizedCurrent = normalizeToLF(params.currentContent);
64+
const normalizedNew = normalizeToLF(params.newText);
65+
const normalizedOld =
66+
typeof params.oldText === "string" && params.oldText.length > 0
67+
? normalizeToLF(params.oldText)
68+
: undefined;
69+
const normalizedOriginal =
70+
typeof params.originalContent === "string" ? normalizeToLF(params.originalContent) : undefined;
71+
72+
if (normalizedOriginal !== undefined && normalizedOriginal === normalizedCurrent) {
73+
return false;
74+
}
75+
76+
if (normalizedNew.length > 0 && !normalizedCurrent.includes(normalizedNew)) {
77+
return false;
78+
}
79+
80+
const withoutInsertedNewText =
81+
normalizedNew.length > 0
82+
? removeExactOccurrences(normalizedCurrent, normalizedNew)
83+
: normalizedCurrent;
84+
if (normalizedOld && withoutInsertedNewText.includes(normalizedOld)) {
85+
return false;
86+
}
87+
88+
return true;
89+
}
90+
91+
function buildEditSuccessResult(pathParam: string): AgentToolResult<unknown> {
92+
return {
93+
isError: false,
94+
content: [
95+
{
96+
type: "text",
97+
text: `Successfully replaced text in ${pathParam}.`,
98+
},
99+
],
100+
details: { diff: "", firstChangedLine: undefined },
101+
} as AgentToolResult<unknown>;
102+
}
103+
104+
function shouldAddMismatchHint(error: unknown) {
105+
return error instanceof Error && error.message.includes(EDIT_MISMATCH_MESSAGE);
106+
}
107+
108+
function appendMismatchHint(error: Error, currentContent: string): Error {
109+
const snippet =
110+
currentContent.length <= EDIT_MISMATCH_HINT_LIMIT
111+
? currentContent
112+
: `${currentContent.slice(0, EDIT_MISMATCH_HINT_LIMIT)}\n... (truncated)`;
113+
const enhanced = new Error(`${error.message}\nCurrent file contents:\n${snippet}`);
114+
enhanced.stack = error.stack;
115+
return enhanced;
116+
}
117+
16118
/**
17-
* When the upstream edit tool throws after having already written (e.g. generateDiffString fails),
18-
* the file may be correctly updated but the tool reports failure. This wrapper catches errors and
19-
* if the target file on disk contains the intended newText, returns success so we don't surface
20-
* a false "edit failed" to the user (fixes #32333, same pattern as #30773 for write).
119+
* Recover from two edit-tool failure classes without changing edit semantics:
120+
* - exact-match mismatch errors become actionable by including current file contents
121+
* - post-write throws are converted back to success only if the file actually changed
21122
*/
22-
export function wrapHostEditToolWithPostWriteRecovery(
123+
export function wrapEditToolWithRecovery(
23124
base: AnyAgentTool,
24-
root: string,
125+
options: EditToolRecoveryOptions,
25126
): AnyAgentTool {
26127
return {
27128
...base,
@@ -31,50 +132,54 @@ export function wrapHostEditToolWithPostWriteRecovery(
31132
signal: AbortSignal | undefined,
32133
onUpdate?: AgentToolUpdateCallback<unknown>,
33134
) => {
135+
const { pathParam, oldText, newText } = readEditToolParams(params);
136+
const absolutePath =
137+
typeof pathParam === "string" ? resolveEditPath(options.root, pathParam) : undefined;
138+
let originalContent: string | undefined;
139+
140+
if (absolutePath && newText !== undefined) {
141+
try {
142+
originalContent = await options.readFile(absolutePath);
143+
} catch {
144+
// Best-effort snapshot only; recovery should still proceed without it.
145+
}
146+
}
147+
34148
try {
35149
return await base.execute(toolCallId, params, signal, onUpdate);
36150
} catch (err) {
37-
const record =
38-
params && typeof params === "object" ? (params as Record<string, unknown>) : undefined;
39-
const pathParam = record && typeof record.path === "string" ? record.path : undefined;
40-
const newText =
41-
record && typeof record.newText === "string"
42-
? record.newText
43-
: record && typeof record.new_string === "string"
44-
? record.new_string
45-
: undefined;
46-
const oldText =
47-
record && typeof record.oldText === "string"
48-
? record.oldText
49-
: record && typeof record.old_string === "string"
50-
? record.old_string
51-
: undefined;
52-
if (!pathParam || !newText) {
151+
if (!absolutePath) {
53152
throw err;
54153
}
154+
155+
let currentContent: string | undefined;
55156
try {
56-
const absolutePath = resolveHostEditPath(root, pathParam);
57-
const content = await fs.readFile(absolutePath, "utf-8");
58-
// Only recover when the replacement likely occurred: newText is present and oldText
59-
// is no longer present. This avoids false success when upstream threw before writing
60-
// (e.g. oldText not found) but the file already contained newText (review feedback).
61-
const hasNew = content.includes(newText);
62-
const stillHasOld =
63-
oldText !== undefined && oldText.length > 0 && content.includes(oldText);
64-
if (hasNew && !stillHasOld) {
65-
return {
66-
content: [
67-
{
68-
type: "text",
69-
text: `Successfully replaced text in ${pathParam}.`,
70-
},
71-
],
72-
details: { diff: "", firstChangedLine: undefined },
73-
} as AgentToolResult<unknown>;
74-
}
157+
currentContent = await options.readFile(absolutePath);
75158
} catch {
76-
// File read failed or path invalid; rethrow original error.
159+
// Fall through to the original error if readback fails.
160+
}
161+
162+
if (typeof currentContent === "string" && newText !== undefined) {
163+
if (
164+
didEditLikelyApply({
165+
originalContent,
166+
currentContent,
167+
oldText,
168+
newText,
169+
})
170+
) {
171+
return buildEditSuccessResult(pathParam ?? absolutePath);
172+
}
77173
}
174+
175+
if (
176+
typeof currentContent === "string" &&
177+
err instanceof Error &&
178+
shouldAddMismatchHint(err)
179+
) {
180+
throw appendMismatchHint(err, currentContent);
181+
}
182+
78183
throw err;
79184
}
80185
},

0 commit comments

Comments
 (0)