Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/components/tools/FileEditToolCall.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -99,9 +98,9 @@ export const FileEditToolCall: React.FC<FileEditToolCallProps> = ({
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);
Expand Down
2 changes: 0 additions & 2 deletions src/services/tools/fileCommon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 10 additions & 7 deletions src/services/tools/file_edit_insert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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.`,
};
}

Expand All @@ -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.`,
};
}

Expand All @@ -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;
Expand All @@ -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}`,
};
}

Expand All @@ -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}`,
};
}
},
Expand Down
3 changes: 1 addition & 2 deletions src/services/tools/file_edit_operation.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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");
}
});

Expand Down
25 changes: 13 additions & 12 deletions src/services/tools/file_edit_operation.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -19,6 +18,7 @@ type FileEditOperationResult<TMetadata> =
| {
success: false;
error: string;
note?: string; // Agent-only message (not displayed in UI)
};

interface ExecuteFileEditOperationOptions<TMetadata> {
Expand Down Expand Up @@ -52,15 +52,15 @@ export async function executeFileEditOperation<TMetadata>({
if (redundantPrefixValidation) {
return {
success: false,
error: `${WRITE_DENIED_PREFIX} ${redundantPrefixValidation.error}`,
error: redundantPrefixValidation.error,
};
}

const pathValidation = validatePathInCwd(filePath, config.cwd, config.runtime);
if (pathValidation) {
return {
success: false,
error: `${WRITE_DENIED_PREFIX} ${pathValidation.error}`,
error: pathValidation.error,
};
}

Expand All @@ -76,7 +76,7 @@ export async function executeFileEditOperation<TMetadata>({
if (err instanceof RuntimeError) {
return {
success: false,
error: `${WRITE_DENIED_PREFIX} ${err.message}`,
error: err.message,
};
}
throw err;
Expand All @@ -85,15 +85,15 @@ export async function executeFileEditOperation<TMetadata>({
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}`,
};
}

const sizeValidation = validateFileSize(fileStat);
if (sizeValidation) {
return {
success: false,
error: `${WRITE_DENIED_PREFIX} ${sizeValidation.error}`,
error: sizeValidation.error,
};
}

Expand All @@ -105,7 +105,7 @@ export async function executeFileEditOperation<TMetadata>({
if (err instanceof RuntimeError) {
return {
success: false,
error: `${WRITE_DENIED_PREFIX} ${err.message}`,
error: err.message,
};
}
throw err;
Expand All @@ -115,7 +115,8 @@ export async function executeFileEditOperation<TMetadata>({
if (!operationResult.success) {
return {
success: false,
error: `${WRITE_DENIED_PREFIX} ${operationResult.error}`,
error: operationResult.error,
note: operationResult.note, // Pass through agent-only message
};
}

Expand All @@ -126,7 +127,7 @@ export async function executeFileEditOperation<TMetadata>({
if (err instanceof RuntimeError) {
return {
success: false,
error: `${WRITE_DENIED_PREFIX} ${err.message}`,
error: err.message,
};
}
throw err;
Expand All @@ -145,22 +146,22 @@ export async function executeFileEditOperation<TMetadata>({
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}`,
};
}
}

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}`,
};
}
}
60 changes: 60 additions & 0 deletions src/services/tools/file_edit_replace_shared.test.ts
Original file line number Diff line number Diff line change
@@ -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");
}
});
15 changes: 15 additions & 0 deletions src/services/tools/file_edit_replace_shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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}`,
};
}

Expand All @@ -63,13 +72,15 @@ 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.`,
};
}

if (replaceCount > occurrences && replaceCount !== -1) {
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.`,
};
}

Expand Down Expand Up @@ -123,13 +134,15 @@ 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.`,
};
}

if (args.end_line < args.start_line) {
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.`,
};
}

Expand All @@ -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}`,
};
}

Expand All @@ -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}`,
};
}

Expand Down
Loading