Skip to content

Commit 31a3c6a

Browse files
authored
🤖 refactor: split file edit tool errors into user/agent messages (#484)
## Problem Agents often continue after file edit failures ("WRITE DENIED, FILE UNMODIFIED: old_string not found") as if nothing happened, leading to commits with incomplete changes. The core issue is that agents send parallel tool calls optimistically (edit + commit) before seeing results. ## Solution Split error communication into two channels: 1. **`error` field** (user-facing): Concise error message shown in UI 2. **`note` field** (agent-facing): Explicit failure signal and recovery guidance ### Key Changes - Add `note?: string` field to all file edit error results - Introduce `EDIT_FAILED_NOTE_PREFIX` constant for consistent messaging - Remove redundant `WRITE_DENIED_PREFIX` from error messages - Simplify UI to detect failures via `success` field instead of string matching - Update tool descriptions to warn upfront that edits may fail ### Agent Notes Pattern All notes follow this structure: ``` EDIT FAILED - file was NOT modified. [Problem]. [Fix]. ``` **Examples:** - `"EDIT FAILED - file was NOT modified. The old_string does not exist in the file. Read the file first to get the exact current content, then retry."` - `"EDIT FAILED - file was NOT modified. The file has 50 lines. Read the file to get current content, then retry."` ### Why This Approach 1. **Upfront warnings** in tool descriptions tell agents not to assume success 2. **Clear failure signals** with "EDIT FAILED" prefix 3. **Actionable remediation** telling agents exactly what to do (read file, retry) 4. **Follows existing pattern** - the `bash` tool already uses `note` field successfully ### Files Modified - `src/types/tools.ts` - Added `note` field and `EDIT_FAILED_NOTE_PREFIX` - `src/services/tools/file_edit_replace_shared.ts` - Added notes to all error cases - `src/services/tools/file_edit_operation.ts` - Pass through `note` field - `src/services/tools/file_edit_insert.ts` - Added notes to all error cases - `src/utils/tools/toolDefinitions.ts` - Updated descriptions with failure warnings - `src/components/tools/FileEditToolCall.tsx` - Simplified error detection - `src/services/tools/file_edit_replace_shared.test.ts` - New tests for `note` field ## Testing ✅ All 127 tool tests pass (including 3 new tests) ✅ TypeScript compilation successful ✅ ESLint checks passed _Generated with `cmux`_
1 parent dacc1df commit 31a3c6a

File tree

9 files changed

+130
-30
lines changed

9 files changed

+130
-30
lines changed

src/components/tools/FileEditToolCall.tsx

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import { useCopyToClipboard } from "@/hooks/useCopyToClipboard";
2323
import { TooltipWrapper, Tooltip } from "../Tooltip";
2424
import { DiffContainer, DiffRenderer, SelectableDiffRenderer } from "../shared/DiffRenderer";
2525
import { KebabMenu, type KebabMenuItem } from "../KebabMenu";
26-
import { WRITE_DENIED_PREFIX } from "@/types/tools";
2726

2827
type FileEditOperationArgs =
2928
| FileEditReplaceStringToolArgs
@@ -99,9 +98,9 @@ export const FileEditToolCall: React.FC<FileEditToolCallProps> = ({
9998
status = "pending",
10099
onReviewNote,
101100
}) => {
102-
// Collapse WRITE DENIED errors by default since they're common and expected
103-
const isWriteDenied = result && !result.success && result.error?.startsWith(WRITE_DENIED_PREFIX);
104-
const initialExpanded = !isWriteDenied;
101+
// Collapse failed edits by default since they're common and expected
102+
const isFailed = result && !result.success;
103+
const initialExpanded = !isFailed;
105104

106105
const { expanded, toggleExpanded } = useToolExpansion(initialExpanded);
107106
const [showRaw, setShowRaw] = React.useState(false);

src/services/tools/fileCommon.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@ import { createPatch } from "diff";
33
import type { FileStat, Runtime } from "@/runtime/Runtime";
44
import { SSHRuntime } from "@/runtime/SSHRuntime";
55

6-
// WRITE_DENIED_PREFIX moved to @/types/tools for frontend/backend sharing
7-
86
/**
97
* Maximum file size for file operations (1MB)
108
* Files larger than this should be processed with system tools like grep, sed, etc.

src/services/tools/file_edit_insert.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type { FileEditInsertToolResult } from "@/types/tools";
33
import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools";
44
import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions";
55
import { validatePathInCwd, validateNoRedundantPrefix } from "./fileCommon";
6-
import { WRITE_DENIED_PREFIX } from "@/types/tools";
6+
import { EDIT_FAILED_NOTE_PREFIX, NOTE_READ_FILE_RETRY } from "@/types/tools";
77
import { executeFileEditOperation } from "./file_edit_operation";
88
import { RuntimeError } from "@/runtime/Runtime";
99
import { fileExists } from "@/utils/runtime/fileExists";
@@ -40,14 +40,15 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
4040
if (pathValidation) {
4141
return {
4242
success: false,
43-
error: `${WRITE_DENIED_PREFIX} ${pathValidation.error}`,
43+
error: pathValidation.error,
4444
};
4545
}
4646

4747
if (line_offset < 0) {
4848
return {
4949
success: false,
50-
error: `${WRITE_DENIED_PREFIX} line_offset must be non-negative (got ${line_offset})`,
50+
error: `line_offset must be non-negative (got ${line_offset})`,
51+
note: `${EDIT_FAILED_NOTE_PREFIX} The line_offset must be >= 0.`,
5152
};
5253
}
5354

@@ -61,7 +62,8 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
6162
if (!create) {
6263
return {
6364
success: false,
64-
error: `${WRITE_DENIED_PREFIX} File not found: ${file_path}. To create it, set create: true`,
65+
error: `File not found: ${file_path}. To create it, set create: true`,
66+
note: `${EDIT_FAILED_NOTE_PREFIX} File does not exist. Set create: true to create it, or check the file path.`,
6567
};
6668
}
6769

@@ -72,7 +74,7 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
7274
if (err instanceof RuntimeError) {
7375
return {
7476
success: false,
75-
error: `${WRITE_DENIED_PREFIX} ${err.message}`,
77+
error: err.message,
7678
};
7779
}
7880
throw err;
@@ -90,6 +92,7 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
9092
return {
9193
success: false,
9294
error: `line_offset ${line_offset} is beyond file length (${lines.length} lines)`,
95+
note: `${EDIT_FAILED_NOTE_PREFIX} The file has ${lines.length} lines. ${NOTE_READ_FILE_RETRY}`,
9396
};
9497
}
9598

@@ -120,14 +123,14 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration)
120123
if (error && typeof error === "object" && "code" in error && error.code === "EACCES") {
121124
return {
122125
success: false,
123-
error: `${WRITE_DENIED_PREFIX} Permission denied: ${file_path}`,
126+
error: `Permission denied: ${file_path}`,
124127
};
125128
}
126129

127130
const message = error instanceof Error ? error.message : String(error);
128131
return {
129132
success: false,
130-
error: `${WRITE_DENIED_PREFIX} Failed to insert content: ${message}`,
133+
error: `Failed to insert content: ${message}`,
131134
};
132135
}
133136
},

src/services/tools/file_edit_operation.test.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { describe, test, expect, jest } from "@jest/globals";
22
import { executeFileEditOperation } from "./file_edit_operation";
3-
import { WRITE_DENIED_PREFIX } from "@/types/tools";
43
import type { Runtime } from "@/runtime/Runtime";
54

65
import { createTestToolConfig, getTestDeps } from "./testHelpers";
@@ -25,7 +24,7 @@ describe("executeFileEditOperation", () => {
2524

2625
expect(result.success).toBe(false);
2726
if (!result.success) {
28-
expect(result.error.startsWith(WRITE_DENIED_PREFIX)).toBe(true);
27+
expect(result.error).toContain("File operations are restricted to the workspace directory");
2928
}
3029
});
3130

src/services/tools/file_edit_operation.ts

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import type { FileEditDiffSuccessBase, FileEditErrorResult } from "@/types/tools";
2-
import { WRITE_DENIED_PREFIX } from "@/types/tools";
32
import type { ToolConfiguration } from "@/utils/tools/tools";
43
import {
54
generateDiff,
@@ -19,6 +18,7 @@ type FileEditOperationResult<TMetadata> =
1918
| {
2019
success: false;
2120
error: string;
21+
note?: string; // Agent-only message (not displayed in UI)
2222
};
2323

2424
interface ExecuteFileEditOperationOptions<TMetadata> {
@@ -52,15 +52,15 @@ export async function executeFileEditOperation<TMetadata>({
5252
if (redundantPrefixValidation) {
5353
return {
5454
success: false,
55-
error: `${WRITE_DENIED_PREFIX} ${redundantPrefixValidation.error}`,
55+
error: redundantPrefixValidation.error,
5656
};
5757
}
5858

5959
const pathValidation = validatePathInCwd(filePath, config.cwd, config.runtime);
6060
if (pathValidation) {
6161
return {
6262
success: false,
63-
error: `${WRITE_DENIED_PREFIX} ${pathValidation.error}`,
63+
error: pathValidation.error,
6464
};
6565
}
6666

@@ -76,7 +76,7 @@ export async function executeFileEditOperation<TMetadata>({
7676
if (err instanceof RuntimeError) {
7777
return {
7878
success: false,
79-
error: `${WRITE_DENIED_PREFIX} ${err.message}`,
79+
error: err.message,
8080
};
8181
}
8282
throw err;
@@ -85,15 +85,15 @@ export async function executeFileEditOperation<TMetadata>({
8585
if (fileStat.isDirectory) {
8686
return {
8787
success: false,
88-
error: `${WRITE_DENIED_PREFIX} Path is a directory, not a file: ${resolvedPath}`,
88+
error: `Path is a directory, not a file: ${resolvedPath}`,
8989
};
9090
}
9191

9292
const sizeValidation = validateFileSize(fileStat);
9393
if (sizeValidation) {
9494
return {
9595
success: false,
96-
error: `${WRITE_DENIED_PREFIX} ${sizeValidation.error}`,
96+
error: sizeValidation.error,
9797
};
9898
}
9999

@@ -105,7 +105,7 @@ export async function executeFileEditOperation<TMetadata>({
105105
if (err instanceof RuntimeError) {
106106
return {
107107
success: false,
108-
error: `${WRITE_DENIED_PREFIX} ${err.message}`,
108+
error: err.message,
109109
};
110110
}
111111
throw err;
@@ -115,7 +115,8 @@ export async function executeFileEditOperation<TMetadata>({
115115
if (!operationResult.success) {
116116
return {
117117
success: false,
118-
error: `${WRITE_DENIED_PREFIX} ${operationResult.error}`,
118+
error: operationResult.error,
119+
note: operationResult.note, // Pass through agent-only message
119120
};
120121
}
121122

@@ -126,7 +127,7 @@ export async function executeFileEditOperation<TMetadata>({
126127
if (err instanceof RuntimeError) {
127128
return {
128129
success: false,
129-
error: `${WRITE_DENIED_PREFIX} ${err.message}`,
130+
error: err.message,
130131
};
131132
}
132133
throw err;
@@ -145,22 +146,22 @@ export async function executeFileEditOperation<TMetadata>({
145146
if (nodeError.code === "ENOENT") {
146147
return {
147148
success: false,
148-
error: `${WRITE_DENIED_PREFIX} File not found: ${filePath}`,
149+
error: `File not found: ${filePath}`,
149150
};
150151
}
151152

152153
if (nodeError.code === "EACCES") {
153154
return {
154155
success: false,
155-
error: `${WRITE_DENIED_PREFIX} Permission denied: ${filePath}`,
156+
error: `Permission denied: ${filePath}`,
156157
};
157158
}
158159
}
159160

160161
const message = error instanceof Error ? error.message : String(error);
161162
return {
162163
success: false,
163-
error: `${WRITE_DENIED_PREFIX} Failed to edit file: ${message}`,
164+
error: `Failed to edit file: ${message}`,
164165
};
165166
}
166167
}
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import { test, expect } from "bun:test";
2+
import { handleStringReplace, handleLineReplace } from "./file_edit_replace_shared";
3+
4+
test("file_edit_replace_string error includes agent note field", () => {
5+
const result = handleStringReplace(
6+
{
7+
file_path: "test.ts",
8+
old_string: "nonexistent",
9+
new_string: "replacement",
10+
},
11+
"some file content"
12+
);
13+
14+
expect(result.success).toBe(false);
15+
if (!result.success) {
16+
expect(result.error).toContain("old_string not found");
17+
expect(result.note).toBeDefined();
18+
expect(result.note).toContain("EDIT FAILED");
19+
expect(result.note).toContain("file was NOT modified");
20+
}
21+
});
22+
23+
test("file_edit_replace_string ambiguous match error includes note", () => {
24+
const result = handleStringReplace(
25+
{
26+
file_path: "test.ts",
27+
old_string: "duplicate",
28+
new_string: "replacement",
29+
},
30+
"duplicate text with duplicate word"
31+
);
32+
33+
expect(result.success).toBe(false);
34+
if (!result.success) {
35+
expect(result.error).toContain("appears 2 times");
36+
expect(result.note).toBeDefined();
37+
expect(result.note).toContain("EDIT FAILED");
38+
expect(result.note).toContain("file was NOT modified");
39+
}
40+
});
41+
42+
test("file_edit_replace_lines validation error includes note", () => {
43+
const result = handleLineReplace(
44+
{
45+
file_path: "test.ts",
46+
start_line: 10,
47+
end_line: 9,
48+
new_lines: ["new content"],
49+
},
50+
"line 1\nline 2"
51+
);
52+
53+
expect(result.success).toBe(false);
54+
if (!result.success) {
55+
expect(result.error).toContain("end_line must be >= start_line");
56+
expect(result.note).toBeDefined();
57+
expect(result.note).toContain("EDIT FAILED");
58+
expect(result.note).toContain("file was NOT modified");
59+
}
60+
});

src/services/tools/file_edit_replace_shared.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,13 @@
55
* providing the core logic while keeping the tool definitions simple for AI providers.
66
*/
77

8+
import {
9+
EDIT_FAILED_NOTE_PREFIX,
10+
NOTE_READ_FILE_FIRST_RETRY,
11+
NOTE_READ_FILE_RETRY,
12+
NOTE_READ_FILE_AGAIN_RETRY,
13+
} from "@/types/tools";
14+
815
interface OperationMetadata {
916
edits_applied: number;
1017
lines_replaced?: number;
@@ -20,6 +27,7 @@ export interface OperationResult {
2027
export interface OperationError {
2128
success: false;
2229
error: string;
30+
note?: string; // Agent-only message (not displayed in UI)
2331
}
2432

2533
export type OperationOutcome = OperationResult | OperationError;
@@ -53,6 +61,7 @@ export function handleStringReplace(
5361
success: false,
5462
error:
5563
"old_string not found in file. The text to replace must exist exactly as written in the file.",
64+
note: `${EDIT_FAILED_NOTE_PREFIX} The old_string does not exist in the file. ${NOTE_READ_FILE_FIRST_RETRY}`,
5665
};
5766
}
5867

@@ -63,13 +72,15 @@ export function handleStringReplace(
6372
return {
6473
success: false,
6574
error: `old_string appears ${occurrences} times in the file. Either expand the context to make it unique or set replace_count to ${occurrences} or -1.`,
75+
note: `${EDIT_FAILED_NOTE_PREFIX} The old_string matched ${occurrences} locations. Add more surrounding context to make it unique, or set replace_count=${occurrences} to replace all occurrences.`,
6676
};
6777
}
6878

6979
if (replaceCount > occurrences && replaceCount !== -1) {
7080
return {
7181
success: false,
7282
error: `replace_count is ${replaceCount} but old_string only appears ${occurrences} time(s) in the file.`,
83+
note: `${EDIT_FAILED_NOTE_PREFIX} The replace_count=${replaceCount} is too high. Retry with replace_count=${occurrences} or -1.`,
7384
};
7485
}
7586

@@ -123,13 +134,15 @@ export function handleLineReplace(
123134
return {
124135
success: false,
125136
error: `start_line must be >= 1 (received ${args.start_line}).`,
137+
note: `${EDIT_FAILED_NOTE_PREFIX} Line numbers must be >= 1.`,
126138
};
127139
}
128140

129141
if (args.end_line < args.start_line) {
130142
return {
131143
success: false,
132144
error: `end_line must be >= start_line (received start ${args.start_line}, end ${args.end_line}).`,
145+
note: `${EDIT_FAILED_NOTE_PREFIX} The end_line must be >= start_line.`,
133146
};
134147
}
135148

@@ -139,6 +152,7 @@ export function handleLineReplace(
139152
return {
140153
success: false,
141154
error: `start_line ${args.start_line} exceeds current file length (${lines.length}).`,
155+
note: `${EDIT_FAILED_NOTE_PREFIX} The file has ${lines.length} lines. ${NOTE_READ_FILE_RETRY}`,
142156
};
143157
}
144158

@@ -149,6 +163,7 @@ export function handleLineReplace(
149163
return {
150164
success: false,
151165
error: `expected_lines validation failed. Current lines [${currentRange.join("\n")}] differ from expected [${args.expected_lines.join("\n")}].`,
166+
note: `${EDIT_FAILED_NOTE_PREFIX} The file content changed since you last read it. ${NOTE_READ_FILE_AGAIN_RETRY}`,
152167
};
153168
}
154169

0 commit comments

Comments
 (0)