diff --git a/src/components/tools/FileEditToolCall.tsx b/src/components/tools/FileEditToolCall.tsx index cfb3d0034..bb40ee70e 100644 --- a/src/components/tools/FileEditToolCall.tsx +++ b/src/components/tools/FileEditToolCall.tsx @@ -23,7 +23,6 @@ import { useCopyToClipboard } from "@/hooks/useCopyToClipboard"; import { TooltipWrapper, Tooltip } from "../Tooltip"; import { DiffContainer, DiffRenderer, SelectableDiffRenderer } from "../shared/DiffRenderer"; import { KebabMenu, type KebabMenuItem } from "../KebabMenu"; -import { WRITE_DENIED_PREFIX } from "@/types/tools"; type FileEditOperationArgs = | FileEditReplaceStringToolArgs @@ -99,9 +98,9 @@ export const FileEditToolCall: React.FC = ({ status = "pending", onReviewNote, }) => { - // Collapse WRITE DENIED errors by default since they're common and expected - const isWriteDenied = result && !result.success && result.error?.startsWith(WRITE_DENIED_PREFIX); - const initialExpanded = !isWriteDenied; + // Collapse failed edits by default since they're common and expected + const isFailed = result && !result.success; + const initialExpanded = !isFailed; const { expanded, toggleExpanded } = useToolExpansion(initialExpanded); const [showRaw, setShowRaw] = React.useState(false); diff --git a/src/services/tools/fileCommon.ts b/src/services/tools/fileCommon.ts index 7b726d33a..c7a608f47 100644 --- a/src/services/tools/fileCommon.ts +++ b/src/services/tools/fileCommon.ts @@ -3,8 +3,6 @@ import { createPatch } from "diff"; import type { FileStat, Runtime } from "@/runtime/Runtime"; import { SSHRuntime } from "@/runtime/SSHRuntime"; -// WRITE_DENIED_PREFIX moved to @/types/tools for frontend/backend sharing - /** * Maximum file size for file operations (1MB) * Files larger than this should be processed with system tools like grep, sed, etc. diff --git a/src/services/tools/file_edit_insert.ts b/src/services/tools/file_edit_insert.ts index 340dc838a..6f8efc7dc 100644 --- a/src/services/tools/file_edit_insert.ts +++ b/src/services/tools/file_edit_insert.ts @@ -3,7 +3,7 @@ import type { FileEditInsertToolResult } from "@/types/tools"; import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; import { validatePathInCwd, validateNoRedundantPrefix } from "./fileCommon"; -import { WRITE_DENIED_PREFIX } from "@/types/tools"; +import { EDIT_FAILED_NOTE_PREFIX, NOTE_READ_FILE_RETRY } from "@/types/tools"; import { executeFileEditOperation } from "./file_edit_operation"; import { RuntimeError } from "@/runtime/Runtime"; import { fileExists } from "@/utils/runtime/fileExists"; @@ -40,14 +40,15 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) if (pathValidation) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${pathValidation.error}`, + error: pathValidation.error, }; } if (line_offset < 0) { return { success: false, - error: `${WRITE_DENIED_PREFIX} line_offset must be non-negative (got ${line_offset})`, + error: `line_offset must be non-negative (got ${line_offset})`, + note: `${EDIT_FAILED_NOTE_PREFIX} The line_offset must be >= 0.`, }; } @@ -61,7 +62,8 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) if (!create) { return { success: false, - error: `${WRITE_DENIED_PREFIX} File not found: ${file_path}. To create it, set create: true`, + error: `File not found: ${file_path}. To create it, set create: true`, + note: `${EDIT_FAILED_NOTE_PREFIX} File does not exist. Set create: true to create it, or check the file path.`, }; } @@ -72,7 +74,7 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) if (err instanceof RuntimeError) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${err.message}`, + error: err.message, }; } throw err; @@ -90,6 +92,7 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) return { success: false, error: `line_offset ${line_offset} is beyond file length (${lines.length} lines)`, + note: `${EDIT_FAILED_NOTE_PREFIX} The file has ${lines.length} lines. ${NOTE_READ_FILE_RETRY}`, }; } @@ -107,14 +110,14 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) if (error && typeof error === "object" && "code" in error && error.code === "EACCES") { return { success: false, - error: `${WRITE_DENIED_PREFIX} Permission denied: ${file_path}`, + error: `Permission denied: ${file_path}`, }; } const message = error instanceof Error ? error.message : String(error); return { success: false, - error: `${WRITE_DENIED_PREFIX} Failed to insert content: ${message}`, + error: `Failed to insert content: ${message}`, }; } }, diff --git a/src/services/tools/file_edit_operation.test.ts b/src/services/tools/file_edit_operation.test.ts index d65bd4238..3bc198dcf 100644 --- a/src/services/tools/file_edit_operation.test.ts +++ b/src/services/tools/file_edit_operation.test.ts @@ -1,6 +1,5 @@ import { describe, test, expect, jest } from "@jest/globals"; import { executeFileEditOperation } from "./file_edit_operation"; -import { WRITE_DENIED_PREFIX } from "@/types/tools"; import type { Runtime } from "@/runtime/Runtime"; import { createTestToolConfig, getTestDeps } from "./testHelpers"; @@ -25,7 +24,7 @@ describe("executeFileEditOperation", () => { expect(result.success).toBe(false); if (!result.success) { - expect(result.error.startsWith(WRITE_DENIED_PREFIX)).toBe(true); + expect(result.error).toContain("File operations are restricted to the workspace directory"); } }); diff --git a/src/services/tools/file_edit_operation.ts b/src/services/tools/file_edit_operation.ts index 6ed67fde2..e84e23647 100644 --- a/src/services/tools/file_edit_operation.ts +++ b/src/services/tools/file_edit_operation.ts @@ -1,5 +1,4 @@ import type { FileEditDiffSuccessBase, FileEditErrorResult } from "@/types/tools"; -import { WRITE_DENIED_PREFIX } from "@/types/tools"; import type { ToolConfiguration } from "@/utils/tools/tools"; import { generateDiff, @@ -19,6 +18,7 @@ type FileEditOperationResult = | { success: false; error: string; + note?: string; // Agent-only message (not displayed in UI) }; interface ExecuteFileEditOperationOptions { @@ -52,7 +52,7 @@ export async function executeFileEditOperation({ if (redundantPrefixValidation) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${redundantPrefixValidation.error}`, + error: redundantPrefixValidation.error, }; } @@ -60,7 +60,7 @@ export async function executeFileEditOperation({ if (pathValidation) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${pathValidation.error}`, + error: pathValidation.error, }; } @@ -76,7 +76,7 @@ export async function executeFileEditOperation({ if (err instanceof RuntimeError) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${err.message}`, + error: err.message, }; } throw err; @@ -85,7 +85,7 @@ export async function executeFileEditOperation({ if (fileStat.isDirectory) { return { success: false, - error: `${WRITE_DENIED_PREFIX} Path is a directory, not a file: ${resolvedPath}`, + error: `Path is a directory, not a file: ${resolvedPath}`, }; } @@ -93,7 +93,7 @@ export async function executeFileEditOperation({ if (sizeValidation) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${sizeValidation.error}`, + error: sizeValidation.error, }; } @@ -105,7 +105,7 @@ export async function executeFileEditOperation({ if (err instanceof RuntimeError) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${err.message}`, + error: err.message, }; } throw err; @@ -115,7 +115,8 @@ export async function executeFileEditOperation({ if (!operationResult.success) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${operationResult.error}`, + error: operationResult.error, + note: operationResult.note, // Pass through agent-only message }; } @@ -126,7 +127,7 @@ export async function executeFileEditOperation({ if (err instanceof RuntimeError) { return { success: false, - error: `${WRITE_DENIED_PREFIX} ${err.message}`, + error: err.message, }; } throw err; @@ -145,14 +146,14 @@ export async function executeFileEditOperation({ if (nodeError.code === "ENOENT") { return { success: false, - error: `${WRITE_DENIED_PREFIX} File not found: ${filePath}`, + error: `File not found: ${filePath}`, }; } if (nodeError.code === "EACCES") { return { success: false, - error: `${WRITE_DENIED_PREFIX} Permission denied: ${filePath}`, + error: `Permission denied: ${filePath}`, }; } } @@ -160,7 +161,7 @@ export async function executeFileEditOperation({ const message = error instanceof Error ? error.message : String(error); return { success: false, - error: `${WRITE_DENIED_PREFIX} Failed to edit file: ${message}`, + error: `Failed to edit file: ${message}`, }; } } diff --git a/src/services/tools/file_edit_replace_shared.test.ts b/src/services/tools/file_edit_replace_shared.test.ts new file mode 100644 index 000000000..a149f726f --- /dev/null +++ b/src/services/tools/file_edit_replace_shared.test.ts @@ -0,0 +1,60 @@ +import { test, expect } from "bun:test"; +import { handleStringReplace, handleLineReplace } from "./file_edit_replace_shared"; + +test("file_edit_replace_string error includes agent note field", () => { + const result = handleStringReplace( + { + file_path: "test.ts", + old_string: "nonexistent", + new_string: "replacement", + }, + "some file content" + ); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("old_string not found"); + expect(result.note).toBeDefined(); + expect(result.note).toContain("EDIT FAILED"); + expect(result.note).toContain("file was NOT modified"); + } +}); + +test("file_edit_replace_string ambiguous match error includes note", () => { + const result = handleStringReplace( + { + file_path: "test.ts", + old_string: "duplicate", + new_string: "replacement", + }, + "duplicate text with duplicate word" + ); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("appears 2 times"); + expect(result.note).toBeDefined(); + expect(result.note).toContain("EDIT FAILED"); + expect(result.note).toContain("file was NOT modified"); + } +}); + +test("file_edit_replace_lines validation error includes note", () => { + const result = handleLineReplace( + { + file_path: "test.ts", + start_line: 10, + end_line: 9, + new_lines: ["new content"], + }, + "line 1\nline 2" + ); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("end_line must be >= start_line"); + expect(result.note).toBeDefined(); + expect(result.note).toContain("EDIT FAILED"); + expect(result.note).toContain("file was NOT modified"); + } +}); diff --git a/src/services/tools/file_edit_replace_shared.ts b/src/services/tools/file_edit_replace_shared.ts index 0e9c80037..4ab833c85 100644 --- a/src/services/tools/file_edit_replace_shared.ts +++ b/src/services/tools/file_edit_replace_shared.ts @@ -5,6 +5,13 @@ * providing the core logic while keeping the tool definitions simple for AI providers. */ +import { + EDIT_FAILED_NOTE_PREFIX, + NOTE_READ_FILE_FIRST_RETRY, + NOTE_READ_FILE_RETRY, + NOTE_READ_FILE_AGAIN_RETRY, +} from "@/types/tools"; + interface OperationMetadata { edits_applied: number; lines_replaced?: number; @@ -20,6 +27,7 @@ export interface OperationResult { export interface OperationError { success: false; error: string; + note?: string; // Agent-only message (not displayed in UI) } export type OperationOutcome = OperationResult | OperationError; @@ -53,6 +61,7 @@ export function handleStringReplace( success: false, error: "old_string not found in file. The text to replace must exist exactly as written in the file.", + note: `${EDIT_FAILED_NOTE_PREFIX} The old_string does not exist in the file. ${NOTE_READ_FILE_FIRST_RETRY}`, }; } @@ -63,6 +72,7 @@ export function handleStringReplace( return { success: false, 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.`, + 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.`, }; } @@ -70,6 +80,7 @@ export function handleStringReplace( return { success: false, error: `replace_count is ${replaceCount} but old_string only appears ${occurrences} time(s) in the file.`, + note: `${EDIT_FAILED_NOTE_PREFIX} The replace_count=${replaceCount} is too high. Retry with replace_count=${occurrences} or -1.`, }; } @@ -123,6 +134,7 @@ export function handleLineReplace( return { success: false, error: `start_line must be >= 1 (received ${args.start_line}).`, + note: `${EDIT_FAILED_NOTE_PREFIX} Line numbers must be >= 1.`, }; } @@ -130,6 +142,7 @@ export function handleLineReplace( return { success: false, error: `end_line must be >= start_line (received start ${args.start_line}, end ${args.end_line}).`, + note: `${EDIT_FAILED_NOTE_PREFIX} The end_line must be >= start_line.`, }; } @@ -139,6 +152,7 @@ export function handleLineReplace( return { success: false, error: `start_line ${args.start_line} exceeds current file length (${lines.length}).`, + note: `${EDIT_FAILED_NOTE_PREFIX} The file has ${lines.length} lines. ${NOTE_READ_FILE_RETRY}`, }; } @@ -149,6 +163,7 @@ export function handleLineReplace( return { success: false, error: `expected_lines validation failed. Current lines [${currentRange.join("\n")}] differ from expected [${args.expected_lines.join("\n")}].`, + note: `${EDIT_FAILED_NOTE_PREFIX} The file content changed since you last read it. ${NOTE_READ_FILE_AGAIN_RETRY}`, }; } diff --git a/src/types/tools.ts b/src/types/tools.ts index 3c95bef07..5cd5f6c9a 100644 --- a/src/types/tools.ts +++ b/src/types/tools.ts @@ -66,6 +66,7 @@ export interface FileEditDiffSuccessBase { export interface FileEditErrorResult { success: false; error: string; + note?: string; // Agent-only message (not displayed in UI) } export interface FileEditReplaceStringToolArgs { @@ -125,6 +126,26 @@ export type FileEditInsertToolResult = FileEditDiffSuccessBase | FileEditErrorRe */ export const WRITE_DENIED_PREFIX = "WRITE DENIED, FILE UNMODIFIED:"; +/** + * Prefix for edit failure notes (agent-only messages). + * This prefix signals to the agent that the file was not modified. + */ +export const EDIT_FAILED_NOTE_PREFIX = "EDIT FAILED - file was NOT modified."; + +/** + * Common note fragments for DRY error messages + */ +export const NOTE_READ_FILE_RETRY = "Read the file to get current content, then retry."; +export const NOTE_READ_FILE_FIRST_RETRY = + "Read the file first to get the exact current content, then retry."; +export const NOTE_READ_FILE_AGAIN_RETRY = "Read the file again and retry."; + +/** + * Tool description warning for file edit tools + */ +export const TOOL_EDIT_WARNING = + "Always check the tool result before proceeding with other operations."; + export type FileEditToolArgs = | FileEditReplaceStringToolArgs | FileEditReplaceLinesToolArgs diff --git a/src/utils/tools/toolDefinitions.ts b/src/utils/tools/toolDefinitions.ts index 2e106a011..9fa2af7c0 100644 --- a/src/utils/tools/toolDefinitions.ts +++ b/src/utils/tools/toolDefinitions.ts @@ -12,6 +12,7 @@ import { BASH_MAX_LINE_BYTES, BASH_MAX_TOTAL_BYTES, } from "@/constants/toolLimits"; +import { TOOL_EDIT_WARNING } from "@/types/tools"; import { zodToJsonSchema } from "zod-to-json-schema"; @@ -68,7 +69,8 @@ export const TOOL_DEFINITIONS = { }, file_edit_replace_string: { description: - "Apply one or more edits to a file by replacing exact text matches. All edits are applied sequentially. Each old_string must be unique in the file unless replace_count > 1 or replace_count is -1.", + "Apply one or more edits to a file by replacing exact text matches. All edits are applied sequentially. Each old_string must be unique in the file unless replace_count > 1 or replace_count is -1. " + + `IMPORTANT: Edits may fail if old_string is not found or not unique. ${TOOL_EDIT_WARNING}`, schema: z.object({ file_path: z.string().describe("The absolute path to the file to edit"), old_string: z @@ -88,7 +90,8 @@ export const TOOL_DEFINITIONS = { }, file_edit_replace_lines: { description: - "Replace a range of lines in a file. Use this for line-based edits when you know the exact line numbers to modify.", + "Replace a range of lines in a file. Use this for line-based edits when you know the exact line numbers to modify. " + + `IMPORTANT: Edits may fail if line numbers are invalid or file content has changed. ${TOOL_EDIT_WARNING}`, schema: z.object({ file_path: z.string().describe("The absolute path to the file to edit"), start_line: z.number().int().min(1).describe("1-indexed start line (inclusive) to replace"), @@ -106,7 +109,8 @@ export const TOOL_DEFINITIONS = { }, file_edit_insert: { description: - "Insert content at a specific line position in a file. Line offset is 1-indexed: 0 inserts at the top, 1 inserts after line 1, etc.", + "Insert content at a specific line position in a file. Line offset is 1-indexed: 0 inserts at the top, 1 inserts after line 1, etc. " + + `IMPORTANT: Edits may fail if line_offset is invalid or file doesn't exist. ${TOOL_EDIT_WARNING}`, schema: z.object({ file_path: z.string().describe("The absolute path to the file to edit"), line_offset: z