diff --git a/docs/AGENTS.md b/docs/AGENTS.md index 2d935dae7b..ead56e3459 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -200,31 +200,34 @@ This project uses **Make** as the primary build orchestrator. See `Makefile` for const config = env.config.loadConfigOrDefault(); config.projects.set(projectPath, { path: projectPath, workspaces: [] }); env.config.saveConfig(config); - - // ❌ BAD - Directly accessing services - const history = await env.historyService.getHistory(workspaceId); - await env.historyService.appendToHistory(workspaceId, message); ``` - **Correct approach (DO THIS):** +// ❌ BAD - Directly accessing services +const history = await env.historyService.getHistory(workspaceId); +await env.historyService.appendToHistory(workspaceId, message); - ```typescript - // ✅ GOOD - Use IPC to save config - await env.mockIpcRenderer.invoke(IPC_CHANNELS.CONFIG_SAVE, { - projects: Array.from(projectsConfig.projects.entries()), - }); - - // ✅ GOOD - Use IPC to interact with services - await env.mockIpcRenderer.invoke(IPC_CHANNELS.HISTORY_GET, workspaceId); - await env.mockIpcRenderer.invoke(IPC_CHANNELS.WORKSPACE_CREATE, projectPath, branchName); - ``` +```` + +**Correct approach (DO THIS):** + +```typescript +// ✅ GOOD - Use IPC to save config +await env.mockIpcRenderer.invoke(IPC_CHANNELS.CONFIG_SAVE, { + projects: Array.from(projectsConfig.projects.entries()), +}); - **Acceptable exceptions:** - - Reading context (like `env.config.loadConfigOrDefault()`) to prepare IPC call parameters - - Verifying filesystem state (like checking if files exist) after IPC operations complete - - Loading existing data to avoid expensive API calls in test setup +// ✅ GOOD - Use IPC to interact with services +await env.mockIpcRenderer.invoke(IPC_CHANNELS.HISTORY_GET, workspaceId); +await env.mockIpcRenderer.invoke(IPC_CHANNELS.WORKSPACE_CREATE, projectPath, branchName); +```` - If IPC is hard to test, fix the test infrastructure or IPC layer, don't work around it by bypassing IPC. +**Acceptable exceptions:** + +- Reading context (like `env.config.loadConfigOrDefault()`) to prepare IPC call parameters +- Verifying filesystem state (like checking if files exist) after IPC operations complete +- Loading existing data to avoid expensive API calls in test setup + +If IPC is hard to test, fix the test infrastructure or IPC layer, don't work around it by bypassing IPC. ## Command Palette (Cmd+Shift+P) @@ -486,3 +489,7 @@ should go through `log.debug()`. - If you're fixing via simplifcation, a new test case is generally not necessary. - If fixing through additional complexity, add a test case if an existing convenient harness exists. - Otherwise if creating complexity, propose a new test harness to contain the new tests. + +## Mode: Exec + +If a user requests `wait_pr_checks`, treat it as a directive to keep running that process and address failures continuously. Do not return to the user until the checks succeed or you encounter a blocker you cannot resolve alone. This mode signals that the user expects persistent execution without further prompting. If static checks fail remotely, reproduce the error locally with `make static-check` before responding. If formatting issues are flagged, run `make fmt` to fix them before retrying CI. diff --git a/scripts/wait_pr_checks.sh b/scripts/wait_pr_checks.sh index 795d3400a3..189261841a 100755 --- a/scripts/wait_pr_checks.sh +++ b/scripts/wait_pr_checks.sh @@ -10,15 +10,58 @@ if [ $# -eq 0 ]; then fi PR_NUMBER=$1 -echo "⏳ Waiting for PR #$PR_NUMBER checks to complete..." -# Warn if there are unstaged changes -if ! git diff --quiet 2>/dev/null; then - echo "⚠️ Warning: You have unstaged changes that are not pushed!" - echo " Run 'git status' to see what changes are pending." - echo "" +# Check for dirty working tree +if ! git diff-index --quiet HEAD --; then + echo "❌ Error: You have uncommitted changes in your working directory." >&2 + echo "" >&2 + git status --short >&2 + echo "" >&2 + echo "Please commit or stash your changes before checking PR status." >&2 + exit 1 +fi + +# Get current branch name +CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD) + +# Get remote tracking branch +REMOTE_BRANCH=$(git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null || echo "") + +if [[ -z "$REMOTE_BRANCH" ]]; then + echo "❌ Error: Current branch '$CURRENT_BRANCH' has no upstream branch." >&2 + echo "Set an upstream with: git push -u origin $CURRENT_BRANCH" >&2 + exit 1 +fi + +# Check if local and remote are in sync +LOCAL_HASH=$(git rev-parse HEAD) +REMOTE_HASH=$(git rev-parse "$REMOTE_BRANCH") + +if [[ "$LOCAL_HASH" != "$REMOTE_HASH" ]]; then + echo "❌ Error: Local branch is not in sync with remote." >&2 + echo "" >&2 + echo "Local: $LOCAL_HASH" >&2 + echo "Remote: $REMOTE_HASH" >&2 + echo "" >&2 + + # Check if we're ahead, behind, or diverged + if git merge-base --is-ancestor "$REMOTE_HASH" HEAD 2>/dev/null; then + AHEAD=$(git rev-list --count "$REMOTE_BRANCH"..HEAD) + echo "Your branch is $AHEAD commit(s) ahead of '$REMOTE_BRANCH'." >&2 + echo "Push your changes with: git push" >&2 + elif git merge-base --is-ancestor HEAD "$REMOTE_HASH" 2>/dev/null; then + BEHIND=$(git rev-list --count HEAD.."$REMOTE_BRANCH") + echo "Your branch is $BEHIND commit(s) behind '$REMOTE_BRANCH'." >&2 + echo "Pull the latest changes with: git pull" >&2 + else + echo "Your branch has diverged from '$REMOTE_BRANCH'." >&2 + echo "You may need to rebase or merge." >&2 + fi + + exit 1 fi +echo "⏳ Waiting for PR #$PR_NUMBER checks to complete..." echo "" while true; do @@ -83,6 +126,7 @@ while true; do if ! ./scripts/check_pr_reviews.sh "$PR_NUMBER" >/dev/null 2>&1; then echo "" echo "❌ Unresolved review comments found!" + echo " 👉 Tip: run ./scripts/check_pr_reviews.sh "$PR_NUMBER" to list them." ./scripts/check_pr_reviews.sh "$PR_NUMBER" exit 1 fi @@ -103,6 +147,7 @@ while true; do else echo "" echo "❌ Please resolve Codex comments before merging." + echo " 👉 Tip: use ./scripts/resolve_codex_comment.sh "$PR_NUMBER" to apply Codex suggestions from the CLI." exit 1 fi elif [ "$MERGE_STATE" = "BLOCKED" ]; then diff --git a/src/components/Messages/ToolMessage.tsx b/src/components/Messages/ToolMessage.tsx index 926712bf08..c6db6f5607 100644 --- a/src/components/Messages/ToolMessage.tsx +++ b/src/components/Messages/ToolMessage.tsx @@ -11,10 +11,12 @@ import type { BashToolResult, FileReadToolArgs, FileReadToolResult, - FileEditReplaceToolArgs, FileEditInsertToolArgs, - FileEditReplaceToolResult, FileEditInsertToolResult, + FileEditReplaceStringToolArgs, + FileEditReplaceStringToolResult, + FileEditReplaceLinesToolArgs, + FileEditReplaceLinesToolResult, ProposePlanToolArgs, ProposePlanToolResult, } from "@/types/tools"; @@ -37,9 +39,20 @@ function isFileReadTool(toolName: string, args: unknown): args is FileReadToolAr return TOOL_DEFINITIONS.file_read.schema.safeParse(args).success; } -function isFileEditReplaceTool(toolName: string, args: unknown): args is FileEditReplaceToolArgs { - if (toolName !== "file_edit_replace") return false; - return TOOL_DEFINITIONS.file_edit_replace.schema.safeParse(args).success; +function isFileEditReplaceStringTool( + toolName: string, + args: unknown +): args is FileEditReplaceStringToolArgs { + if (toolName !== "file_edit_replace_string") return false; + return TOOL_DEFINITIONS.file_edit_replace_string.schema.safeParse(args).success; +} + +function isFileEditReplaceLinesTool( + toolName: string, + args: unknown +): args is FileEditReplaceLinesToolArgs { + if (toolName !== "file_edit_replace_lines") return false; + return TOOL_DEFINITIONS.file_edit_replace_lines.schema.safeParse(args).success; } function isFileEditInsertTool(toolName: string, args: unknown): args is FileEditInsertToolArgs { @@ -79,13 +92,26 @@ export const ToolMessage: React.FC = ({ message, className, wo ); } - if (isFileEditReplaceTool(message.toolName, message.args)) { + if (isFileEditReplaceStringTool(message.toolName, message.args)) { + return ( +
+ +
+ ); + } + + if (isFileEditReplaceLinesTool(message.toolName, message.args)) { return (
diff --git a/src/components/TitleBar.tsx b/src/components/TitleBar.tsx index e2f4dc391b..31bc75057f 100644 --- a/src/components/TitleBar.tsx +++ b/src/components/TitleBar.tsx @@ -30,6 +30,20 @@ const BuildInfo = styled.div` cursor: default; `; +interface VersionMetadata { + buildTime: string; + git_describe?: unknown; +} + +function hasBuildInfo(value: unknown): value is VersionMetadata { + if (typeof value !== "object" || value === null) { + return false; + } + + const candidate = value as Record; + return typeof candidate.buildTime === "string"; +} + function formatUSDate(isoDate: string): string { const date = new Date(isoDate); const month = String(date.getUTCMonth() + 1).padStart(2, "0"); @@ -51,13 +65,31 @@ function formatExtendedTimestamp(isoDate: string): string { }); } +function parseBuildInfo(version: unknown) { + if (hasBuildInfo(version)) { + const { buildTime, git_describe } = version; + const gitDescribe = typeof git_describe === "string" ? git_describe : undefined; + + return { + buildDate: formatUSDate(buildTime), + extendedTimestamp: formatExtendedTimestamp(buildTime), + gitDescribe, + }; + } + + return { + buildDate: "unknown", + extendedTimestamp: "Unknown build time", + gitDescribe: undefined, + }; +} + export function TitleBar() { - const buildDate = formatUSDate(VERSION.buildTime); - const extendedTimestamp = formatExtendedTimestamp(VERSION.buildTime); + const { buildDate, extendedTimestamp, gitDescribe } = parseBuildInfo(VERSION satisfies unknown); return ( - cmux {VERSION.git_describe} + cmux {gitDescribe ?? "(dev)"} {buildDate} Built at {extendedTimestamp} diff --git a/src/components/tools/FileEditToolCall.tsx b/src/components/tools/FileEditToolCall.tsx index 73cc831ff3..e816c45bb8 100644 --- a/src/components/tools/FileEditToolCall.tsx +++ b/src/components/tools/FileEditToolCall.tsx @@ -2,10 +2,12 @@ import React from "react"; import styled from "@emotion/styled"; import { parsePatch } from "diff"; import type { - FileEditReplaceToolArgs, - FileEditReplaceToolResult, FileEditInsertToolArgs, FileEditInsertToolResult, + FileEditReplaceStringToolArgs, + FileEditReplaceStringToolResult, + FileEditReplaceLinesToolArgs, + FileEditReplaceLinesToolResult, } from "@/types/tools"; import { ToolContainer, @@ -177,12 +179,19 @@ const ButtonGroup = styled.div` margin-right: 8px; `; -type FileEditToolArgs = FileEditReplaceToolArgs | FileEditInsertToolArgs; -type FileEditToolResult = FileEditReplaceToolResult | FileEditInsertToolResult; +type FileEditOperationArgs = + | FileEditReplaceStringToolArgs + | FileEditReplaceLinesToolArgs + | FileEditInsertToolArgs; + +type FileEditToolResult = + | FileEditReplaceStringToolResult + | FileEditReplaceLinesToolResult + | FileEditInsertToolResult; interface FileEditToolCallProps { - toolName: "file_edit_replace" | "file_edit_insert"; - args: FileEditToolArgs; + toolName: "file_edit_replace_string" | "file_edit_replace_lines" | "file_edit_insert"; + args: FileEditOperationArgs; result?: FileEditToolResult; status?: ToolStatus; } @@ -262,7 +271,7 @@ export const FileEditToolCall: React.FC = ({ const [showRaw, setShowRaw] = React.useState(false); const [copied, setCopied] = React.useState(false); - const filePath = args.file_path; + const filePath = "file_path" in args ? args.file_path : undefined; const handleCopyPatch = async () => { if (result && result.success && result.diff) { diff --git a/src/services/streamManager.test.ts b/src/services/streamManager.test.ts index abef27314c..20c8dd186e 100644 --- a/src/services/streamManager.test.ts +++ b/src/services/streamManager.test.ts @@ -345,7 +345,7 @@ describe("StreamManager - Unavailable Tool Handling", () => { type: "tool-call", toolCallId: "test-call-1", toolName: "file_edit_replace", - input: { file_path: "/test", edits: [] }, + input: { file_path: "/test", old_string: "foo", new_string: "bar" }, }; // SDK emits tool-error when tool execution fails yield { diff --git a/src/services/tools/file_edit_insert.ts b/src/services/tools/file_edit_insert.ts index 6bc8502bf3..525f77f17e 100644 --- a/src/services/tools/file_edit_insert.ts +++ b/src/services/tools/file_edit_insert.ts @@ -1,16 +1,11 @@ import { tool } from "ai"; import * as fs from "fs/promises"; import * as path from "path"; -import writeFileAtomic from "write-file-atomic"; import type { FileEditInsertToolResult } from "@/types/tools"; import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; -import { - generateDiff, - validatePathInCwd, - validateFileSize, - WRITE_DENIED_PREFIX, -} from "./fileCommon"; +import { validatePathInCwd, WRITE_DENIED_PREFIX } from "./fileCommon"; +import { executeFileEditOperation } from "./file_edit_operation"; /** * File edit insert tool factory for AI assistant @@ -28,7 +23,6 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) create, }): Promise => { try { - // Validate that the path is within the working directory const pathValidation = validatePathInCwd(file_path, config.cwd); if (pathValidation) { return { @@ -37,99 +31,67 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) }; } - // Resolve path (but expect absolute paths) + if (line_offset < 0) { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} line_offset must be non-negative (got ${line_offset})`, + }; + } + const resolvedPath = path.isAbsolute(file_path) ? file_path : path.resolve(config.cwd, file_path); - // Check if file exists - let originalContent = ""; + let fileExists = await fs + .stat(resolvedPath) + .then((stats) => stats.isFile()) + .catch(() => false); - try { - const stats = await fs.stat(resolvedPath); - if (!stats.isFile()) { + if (!fileExists) { + if (!create) { return { success: false, - error: `${WRITE_DENIED_PREFIX} Path exists but is not a file: ${resolvedPath}`, + error: `${WRITE_DENIED_PREFIX} File not found: ${file_path}. To create it, set create: true`, }; } - // Validate file size - const sizeValidation = validateFileSize(stats); - if (sizeValidation) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} ${sizeValidation.error}`, - }; - } + const parentDir = path.dirname(resolvedPath); + await fs.mkdir(parentDir, { recursive: true }); + await fs.writeFile(resolvedPath, ""); + fileExists = true; + } + + return executeFileEditOperation({ + config, + filePath: file_path, + operation: (originalContent) => { + const lines = originalContent.split("\n"); - // Read file content - originalContent = await fs.readFile(resolvedPath, { encoding: "utf-8" }); - } catch (error) { - // If file doesn't exist and create is not true, return error with suggestion - if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") { - if (!create) { + if (line_offset > lines.length) { return { success: false, - error: `${WRITE_DENIED_PREFIX} File not found: ${file_path}. To create it, set create: true`, + error: `line_offset ${line_offset} is beyond file length (${lines.length} lines)`, }; } - // File doesn't exist but create is true, so we'll create it - // Ensure parent directory exists - const parentDir = path.dirname(resolvedPath); - await fs.mkdir(parentDir, { recursive: true }); - originalContent = ""; - } else { - // Re-throw other errors - throw error; - } - } - const lines = originalContent.split("\n"); + const newLines = [...lines.slice(0, line_offset), content, ...lines.slice(line_offset)]; + const newContent = newLines.join("\n"); - // Validate line_offset - if (line_offset < 0) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} line_offset must be non-negative (got ${line_offset})`, - }; - } - - if (line_offset > lines.length) { + return { + success: true, + newContent, + metadata: {}, + }; + }, + }); + } catch (error) { + if (error && typeof error === "object" && "code" in error && error.code === "EACCES") { return { success: false, - error: `${WRITE_DENIED_PREFIX} line_offset ${line_offset} is beyond file length (${lines.length} lines)`, + error: `${WRITE_DENIED_PREFIX} Permission denied: ${file_path}`, }; } - // Insert content at specified line - // line_offset = 0: insert at top (before line 1) - // line_offset = N: insert after line N - const newLines = [...lines.slice(0, line_offset), content, ...lines.slice(line_offset)]; - const newContent = newLines.join("\n"); - - // Write the modified content back to file atomically - await writeFileAtomic(resolvedPath, newContent, { encoding: "utf-8" }); - - // Generate diff - const diff = generateDiff(resolvedPath, originalContent, newContent); - - return { - success: true, - diff, - }; - } catch (error) { - // Handle specific errors - if (error && typeof error === "object" && "code" in error) { - if (error.code === "EACCES") { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} Permission denied: ${file_path}`, - }; - } - } - - // Generic error const message = error instanceof Error ? error.message : String(error); return { success: false, diff --git a/src/services/tools/file_edit_operation.test.ts b/src/services/tools/file_edit_operation.test.ts new file mode 100644 index 0000000000..81ac2f2900 --- /dev/null +++ b/src/services/tools/file_edit_operation.test.ts @@ -0,0 +1,24 @@ +import { describe, it, expect } from "bun:test"; +import { executeFileEditOperation } from "./file_edit_operation"; +import { WRITE_DENIED_PREFIX } from "./fileCommon"; + +const TEST_CWD = "/tmp"; + +function createConfig() { + return { cwd: TEST_CWD }; +} + +describe("executeFileEditOperation", () => { + it("should return error when path validation fails", async () => { + const result = await executeFileEditOperation({ + config: createConfig(), + filePath: "../../etc/passwd", + operation: () => ({ success: true, newContent: "", metadata: {} }), + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.startsWith(WRITE_DENIED_PREFIX)).toBe(true); + } + }); +}); diff --git a/src/services/tools/file_edit_operation.ts b/src/services/tools/file_edit_operation.ts new file mode 100644 index 0000000000..0a87c922e9 --- /dev/null +++ b/src/services/tools/file_edit_operation.ts @@ -0,0 +1,113 @@ +import * as fs from "fs/promises"; +import * as path from "path"; +import writeFileAtomic from "write-file-atomic"; +import type { FileEditDiffSuccessBase, FileEditErrorResult } from "@/types/tools"; +import type { ToolConfiguration } from "@/utils/tools/tools"; +import { + generateDiff, + validateFileSize, + validatePathInCwd, + WRITE_DENIED_PREFIX, +} from "./fileCommon"; + +type FileEditOperationResult = + | { + success: true; + newContent: string; + metadata: TMetadata; + } + | { + success: false; + error: string; + }; + +interface ExecuteFileEditOperationOptions { + config: ToolConfiguration; + filePath: string; + operation: ( + originalContent: string + ) => FileEditOperationResult | Promise>; +} + +/** + * Shared execution pipeline for file edit tools. + * Handles validation, file IO, diff generation, and common error handling. + */ +export async function executeFileEditOperation({ + config, + filePath, + operation, +}: ExecuteFileEditOperationOptions): Promise< + FileEditErrorResult | (FileEditDiffSuccessBase & TMetadata) +> { + try { + const pathValidation = validatePathInCwd(filePath, config.cwd); + if (pathValidation) { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} ${pathValidation.error}`, + }; + } + + const resolvedPath = path.isAbsolute(filePath) ? filePath : path.resolve(config.cwd, filePath); + + const stats = await fs.stat(resolvedPath); + if (!stats.isFile()) { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} Path exists but is not a file: ${resolvedPath}`, + }; + } + + const sizeValidation = validateFileSize(stats); + if (sizeValidation) { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} ${sizeValidation.error}`, + }; + } + + const originalContent = await fs.readFile(resolvedPath, { encoding: "utf-8" }); + + const operationResult = await Promise.resolve(operation(originalContent)); + if (!operationResult.success) { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} ${operationResult.error}`, + }; + } + + await writeFileAtomic(resolvedPath, operationResult.newContent, { encoding: "utf-8" }); + + const diff = generateDiff(resolvedPath, originalContent, operationResult.newContent); + + return { + success: true, + diff, + ...operationResult.metadata, + }; + } catch (error) { + if (error && typeof error === "object" && "code" in error) { + const nodeError = error as { code?: string }; + if (nodeError.code === "ENOENT") { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} File not found: ${filePath}`, + }; + } + + if (nodeError.code === "EACCES") { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} Permission denied: ${filePath}`, + }; + } + } + + const message = error instanceof Error ? error.message : String(error); + return { + success: false, + error: `${WRITE_DENIED_PREFIX} Failed to edit file: ${message}`, + }; + } +} diff --git a/src/services/tools/file_edit_replace.test.ts b/src/services/tools/file_edit_replace.test.ts index 3ab3cb19be..e8e672bfe8 100644 --- a/src/services/tools/file_edit_replace.test.ts +++ b/src/services/tools/file_edit_replace.test.ts @@ -2,8 +2,14 @@ import { describe, it, expect, beforeEach, afterEach } from "bun:test"; import * as fs from "fs/promises"; import * as path from "path"; import * as os from "os"; -import { createFileEditReplaceTool } from "./file_edit_replace"; -import type { FileEditReplaceToolArgs, FileEditReplaceToolResult } from "@/types/tools"; +import { createFileEditReplaceStringTool } from "./file_edit_replace_string"; +import { createFileEditReplaceLinesTool } from "./file_edit_replace_lines"; +import type { + FileEditReplaceStringToolArgs, + FileEditReplaceStringToolResult, + FileEditReplaceLinesToolArgs, + FileEditReplaceLinesToolResult, +} from "@/types/tools"; import type { ToolCallOptions } from "ai"; // Mock ToolCallOptions for testing @@ -21,444 +27,86 @@ const readFile = async (filePath: string): Promise => { return await fs.readFile(filePath, "utf-8"); }; -const executeReplace = async ( - tool: ReturnType, - filePath: string, - edits: FileEditReplaceToolArgs["edits"] -): Promise => { - const args: FileEditReplaceToolArgs = { file_path: filePath, edits }; - return (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; +const executeStringReplace = async ( + tool: ReturnType, + args: FileEditReplaceStringToolArgs +): Promise => { + return (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceStringToolResult; }; -describe("file_edit_replace tool", () => { +const executeLinesReplace = async ( + tool: ReturnType, + args: FileEditReplaceLinesToolArgs +): Promise => { + return (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceLinesToolResult; +}; + +describe("file_edit_replace_string tool", () => { let testDir: string; let testFilePath: string; beforeEach(async () => { - // Create a temporary directory for test files testDir = await fs.mkdtemp(path.join(os.tmpdir(), "fileEditReplace-test-")); testFilePath = path.join(testDir, "test.txt"); }); afterEach(async () => { - // Clean up test directory await fs.rm(testDir, { recursive: true, force: true }); }); it("should apply a single edit successfully", async () => { await setupFile(testFilePath, "Hello world\nThis is a test\nGoodbye world"); - const tool = createFileEditReplaceTool({ cwd: testDir }); - - const result = await executeReplace(tool, testFilePath, [ - { old_string: "Hello world", new_string: "Hello universe" }, - ]); - - expect(result.success).toBe(true); - if (result.success) { - expect(result.edits_applied).toBe(1); - } - - expect(await readFile(testFilePath)).toBe("Hello universe\nThis is a test\nGoodbye world"); - }); - - it("should apply multiple edits sequentially", async () => { - await setupFile(testFilePath, "foo bar baz"); - const tool = createFileEditReplaceTool({ cwd: testDir }); - - const result = await executeReplace(tool, testFilePath, [ - { old_string: "foo", new_string: "FOO" }, - { old_string: "bar", new_string: "BAR" }, - { old_string: "baz", new_string: "BAZ" }, - ]); - - expect(result.success).toBe(true); - if (result.success) { - expect(result.edits_applied).toBe(3); - } - - expect(await readFile(testFilePath)).toBe("FOO BAR BAZ"); - }); - - it("should rollback if later edit fails (first edit breaks second edit search)", async () => { - // Setup - This test demonstrates that multi-edit is a state machine: - // each edit operates on the OUTPUT of the previous edit, not the original file. - // If any edit fails, the entire operation is rolled back (file unchanged). - const initialContent = "foo bar baz"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "foo", - new_string: "FOO", - }, - { - // This edit will FAIL because "foo" was already replaced by the first edit - // The second edit operates on "FOO bar baz", not "foo bar baz" - old_string: "foo", - new_string: "qux", - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; + const tool = createFileEditReplaceStringTool({ cwd: testDir }); - // Assert - The operation should fail - expect(result.success).toBe(false); - if (!result.success) { - expect(result.error).toContain("Edit 2"); - expect(result.error).toContain("old_string not found"); - } - - // Critical assertion: File should remain UNCHANGED because edit failed - // The atomic write should not have occurred - const finalContent = await fs.readFile(testFilePath, "utf-8"); - expect(finalContent).toBe(initialContent); - expect(finalContent).toBe("foo bar baz"); // Still the original content - }); - - it("should replace all occurrences when replace_count is -1", async () => { - // Setup - const initialContent = "cat dog cat bird cat"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { + const payload: FileEditReplaceStringToolArgs = { file_path: testFilePath, - edits: [ - { - old_string: "cat", - new_string: "mouse", - replace_count: -1, - }, - ], + old_string: "Hello world", + new_string: "Hello universe", }; - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; + const result = await executeStringReplace(tool, payload); - // Assert - expect(result.success).toBe(true); - if (result.success) { - expect(result.edits_applied).toBe(3); - } - - const updatedContent = await fs.readFile(testFilePath, "utf-8"); - expect(updatedContent).toBe("mouse dog mouse bird mouse"); - }); - - it("should replace unique occurrence when replace_count defaults to 1", async () => { - // Setup - const initialContent = "cat dog bird"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "cat", - new_string: "mouse", - // replace_count omitted, defaults to 1 - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert expect(result.success).toBe(true); if (result.success) { expect(result.edits_applied).toBe(1); } - const updatedContent = await fs.readFile(testFilePath, "utf-8"); - expect(updatedContent).toBe("mouse dog bird"); - }); - - it("should fail when old_string is not found", async () => { - // Setup - const initialContent = "Hello world"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "nonexistent", - new_string: "replacement", - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(false); - if (!result.success) { - expect(result.error).toContain("old_string not found"); - } - - // File should remain unchanged - const unchangedContent = await fs.readFile(testFilePath, "utf-8"); - expect(unchangedContent).toBe(initialContent); - }); - - it("should fail when old_string appears multiple times with replace_count of 1", async () => { - // Setup - const initialContent = "cat dog cat bird cat"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "cat", - new_string: "mouse", - replace_count: 1, // Explicitly set to 1 - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(false); - if (!result.success) { - expect(result.error).toContain("appears 3 times"); - expect(result.error).toContain("expand the context to make it unique"); - expect(result.error).toContain("replace_count to 3 or -1"); - } - - // File should remain unchanged - const unchangedContent = await fs.readFile(testFilePath, "utf-8"); - expect(unchangedContent).toBe(initialContent); - }); - - it("should replace exactly N occurrences when replace_count is N", async () => { - // Setup - const initialContent = "cat dog cat bird cat"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "cat", - new_string: "mouse", - replace_count: 2, // Replace first 2 occurrences - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(true); - if (result.success) { - expect(result.edits_applied).toBe(2); - } - - const updatedContent = await fs.readFile(testFilePath, "utf-8"); - expect(updatedContent).toBe("mouse dog mouse bird cat"); - }); - - it("should fail when replace_count exceeds actual occurrences", async () => { - // Setup - const initialContent = "cat dog bird"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "cat", - new_string: "mouse", - replace_count: 5, // Only 1 occurrence exists - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(false); - if (!result.success) { - expect(result.error).toContain("replace_count is 5"); - expect(result.error).toContain("only appears 1 time(s)"); - } - - // File should remain unchanged - const unchangedContent = await fs.readFile(testFilePath, "utf-8"); - expect(unchangedContent).toBe(initialContent); - }); - - it("should fail when file does not exist", async () => { - // Setup - const nonExistentPath = path.join(testDir, "nonexistent.txt"); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: nonExistentPath, - edits: [ - { - old_string: "foo", - new_string: "bar", - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(false); - if (!result.success) { - expect(result.error).toContain("File not found"); - } + expect(await readFile(testFilePath)).toBe("Hello universe\nThis is a test\nGoodbye world"); }); +}); - it("should handle multiline edits", async () => { - // Setup - const initialContent = "line1\nline2\nline3\nline4"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "line2\nline3", - new_string: "REPLACED", - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(true); - if (result.success) { - expect(result.edits_applied).toBe(1); - } +describe("file_edit_replace_lines tool", () => { + let testDir: string; + let testFilePath: string; - const updatedContent = await fs.readFile(testFilePath, "utf-8"); - expect(updatedContent).toBe("line1\nREPLACED\nline4"); + beforeEach(async () => { + testDir = await fs.mkdtemp(path.join(os.tmpdir(), "fileEditReplace-test-")); + testFilePath = path.join(testDir, "test.txt"); }); - it("should handle empty string replacement", async () => { - // Setup - const initialContent = "Hello [DELETE_ME] world"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "[DELETE_ME] ", - new_string: "", - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(true); - if (result.success) { - expect(result.edits_applied).toBe(1); - } - - const updatedContent = await fs.readFile(testFilePath, "utf-8"); - expect(updatedContent).toBe("Hello world"); + afterEach(async () => { + await fs.rm(testDir, { recursive: true, force: true }); }); - it("should handle edits that depend on previous edits", async () => { - // Setup - const initialContent = "step1"; - await fs.writeFile(testFilePath, initialContent); + it("should replace a line range successfully", async () => { + await setupFile(testFilePath, "line1\nline2\nline3\nline4"); + const tool = createFileEditReplaceLinesTool({ cwd: testDir }); - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { + const payload: FileEditReplaceLinesToolArgs = { file_path: testFilePath, - edits: [ - { - old_string: "step1", - new_string: "step2", - }, - { - old_string: "step2", - new_string: "step3", - }, - ], + start_line: 2, + end_line: 3, + new_lines: ["LINE2", "LINE3"], }; - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; + const result = await executeLinesReplace(tool, payload); - // Assert expect(result.success).toBe(true); if (result.success) { - expect(result.edits_applied).toBe(2); + expect(result.lines_replaced).toBe(2); + expect(result.line_delta).toBe(0); } - const updatedContent = await fs.readFile(testFilePath, "utf-8"); - expect(updatedContent).toBe("step3"); - }); - - it("should return unified diff with context of 3", async () => { - // Setup - create a file with multiple lines - const initialContent = "line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9"; - await fs.writeFile(testFilePath, initialContent); - - const tool = createFileEditReplaceTool({ cwd: testDir }); - const args: FileEditReplaceToolArgs = { - file_path: testFilePath, - edits: [ - { - old_string: "line5", - new_string: "LINE5_MODIFIED", - }, - ], - }; - - // Execute - const result = (await tool.execute!(args, mockToolCallOptions)) as FileEditReplaceToolResult; - - // Assert - expect(result.success).toBe(true); - if (result.success) { - expect(result.diff).toBeDefined(); - expect(result.diff).toContain("--- " + testFilePath); - expect(result.diff).toContain("+++ " + testFilePath); - expect(result.diff).toContain("-line5"); - expect(result.diff).toContain("+LINE5_MODIFIED"); - - // Verify context of 3 - should include 3 lines before and after - expect(result.diff).toContain("line2"); - expect(result.diff).toContain("line3"); - expect(result.diff).toContain("line4"); - expect(result.diff).toContain("line6"); - expect(result.diff).toContain("line7"); - expect(result.diff).toContain("line8"); - - // Lines outside the context should not be in the diff (line1, line9) - // Note: This depends on the exact diff format, so we verify at least the key parts are present - } + expect(await readFile(testFilePath)).toBe("line1\nLINE2\nLINE3\nline4"); }); }); diff --git a/src/services/tools/file_edit_replace.ts b/src/services/tools/file_edit_replace.ts deleted file mode 100644 index 31198c2a5f..0000000000 --- a/src/services/tools/file_edit_replace.ts +++ /dev/null @@ -1,174 +0,0 @@ -import { tool } from "ai"; -import * as fs from "fs/promises"; -import * as path from "path"; -import writeFileAtomic from "write-file-atomic"; -import type { FileEditReplaceToolResult } from "@/types/tools"; -import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; -import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; -import { - generateDiff, - validatePathInCwd, - validateFileSize, - WRITE_DENIED_PREFIX, -} from "./fileCommon"; - -/** - * File edit replace tool factory for AI assistant - * Creates a tool that allows the AI to apply multiple edits to a file - * @param config Required configuration including working directory - */ -export const createFileEditReplaceTool: ToolFactory = (config: ToolConfiguration) => { - return tool({ - description: TOOL_DEFINITIONS.file_edit_replace.description, - inputSchema: TOOL_DEFINITIONS.file_edit_replace.schema, - execute: async ( - { file_path, edits }, - { abortSignal: _abortSignal } - ): Promise => { - // Note: abortSignal available but not used - file operations are fast and complete quickly - try { - // Validate that the path is within the working directory - const pathValidation = validatePathInCwd(file_path, config.cwd); - if (pathValidation) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} ${pathValidation.error}`, - }; - } - - // Resolve path (but expect absolute paths) - const resolvedPath = path.isAbsolute(file_path) - ? file_path - : path.resolve(config.cwd, file_path); - - // Check if file exists - const stats = await fs.stat(resolvedPath); - if (!stats.isFile()) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} Path exists but is not a file: ${resolvedPath}`, - }; - } - - // Validate file size - const sizeValidation = validateFileSize(stats); - if (sizeValidation) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} ${sizeValidation.error}`, - }; - } - - // Read file content - const originalContent = await fs.readFile(resolvedPath, { encoding: "utf-8" }); - let content = originalContent; - - // Apply each edit sequentially - // THE KEY INSIGHT: MultiEdit is a state machine where each edit operates on the - // output of the previous edit, not the original file. - // - // For example, if the file contains "foo bar baz": - // Edit 1: "foo" -> "FOO" transforms content to "FOO bar baz" - // Edit 2: "bar" -> "BAR" operates on "FOO bar baz", resulting in "FOO BAR baz" - // Edit 3: "foo" -> "qux" would FAIL because "foo" no longer exists in the current state - // - // If ANY edit fails, the entire operation is rolled back - the file remains unchanged - // because we only write to disk after all edits succeed. - let editsApplied = 0; - for (let i = 0; i < edits.length; i++) { - const edit = edits[i]; - const replaceCount = edit.replace_count ?? 1; // Default to 1 - - // Validate old_string exists in content - if (!content.includes(edit.old_string)) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} Edit ${i + 1}: old_string not found in file. The text to replace must exist exactly as written in the file.`, - }; - } - - // Count occurrences - const parts = content.split(edit.old_string); - const occurrences = parts.length - 1; - - // Check for uniqueness if replace_count is 1 - if (replaceCount === 1 && occurrences > 1) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} Edit ${i + 1}: old_string appears ${occurrences} times in the file. Either expand the context to make it unique or set replace_count to ${occurrences} or -1.`, - }; - } - - // Validate replace_count doesn't exceed occurrences (unless -1) - if (replaceCount > occurrences && replaceCount !== -1) { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} Edit ${i + 1}: replace_count is ${replaceCount} but old_string only appears ${occurrences} time(s) in the file.`, - }; - } - - // Apply the edit - if (replaceCount === -1) { - // Replace all occurrences - content = parts.join(edit.new_string); - editsApplied += occurrences; - } else { - // Replace the specified number of occurrences - let replacedCount = 0; - let currentContent = content; - - for (let j = 0; j < replaceCount; j++) { - const index = currentContent.indexOf(edit.old_string); - if (index !== -1) { - currentContent = - currentContent.substring(0, index) + - edit.new_string + - currentContent.substring(index + edit.old_string.length); - replacedCount++; - } else { - break; - } - } - - content = currentContent; - editsApplied += replacedCount; - } - } - - // Write the modified content back to file atomically - await writeFileAtomic(resolvedPath, content, { encoding: "utf-8" }); - - // Generate diff - const diff = generateDiff(resolvedPath, originalContent, content); - - return { - success: true, - edits_applied: editsApplied, - diff, - }; - } catch (error) { - // Handle specific errors - if (error && typeof error === "object" && "code" in error) { - if (error.code === "ENOENT") { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} File not found: ${file_path}`, - }; - } else if (error.code === "EACCES") { - return { - success: false, - error: `${WRITE_DENIED_PREFIX} Permission denied: ${file_path}`, - }; - } - } - - // Generic error - const message = error instanceof Error ? error.message : String(error); - return { - success: false, - error: `${WRITE_DENIED_PREFIX} Failed to edit file: ${message}`, - }; - } - }, - }); -}; diff --git a/src/services/tools/file_edit_replace_lines.ts b/src/services/tools/file_edit_replace_lines.ts new file mode 100644 index 0000000000..30be0a586d --- /dev/null +++ b/src/services/tools/file_edit_replace_lines.ts @@ -0,0 +1,51 @@ +import { tool } from "ai"; +import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; +import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; +import { executeFileEditOperation } from "./file_edit_operation"; +import { handleLineReplace, type LineReplaceArgs } from "./file_edit_replace_shared"; + +export interface FileEditReplaceLinesResult { + success: true; + diff: string; + edits_applied: number; + lines_replaced: number; + line_delta: number; +} + +export interface FileEditReplaceLinesError { + success: false; + error: string; +} + +export type FileEditReplaceLinesToolResult = FileEditReplaceLinesResult | FileEditReplaceLinesError; + +/** + * Line-based file edit replace tool factory + */ +export const createFileEditReplaceLinesTool: ToolFactory = (config: ToolConfiguration) => { + return tool({ + description: TOOL_DEFINITIONS.file_edit_replace_lines.description, + inputSchema: TOOL_DEFINITIONS.file_edit_replace_lines.schema, + execute: async (args: LineReplaceArgs): Promise => { + const result = await executeFileEditOperation({ + config, + filePath: args.file_path, + operation: (originalContent) => handleLineReplace(args, originalContent), + }); + + // handleLineReplace always returns lines_replaced and line_delta, + // so we can safely assert this meets FileEditReplaceLinesToolResult + if (result.success) { + return { + success: true, + diff: result.diff, + edits_applied: result.edits_applied, + lines_replaced: result.lines_replaced!, + line_delta: result.line_delta!, + }; + } + + return result; + }, + }); +}; diff --git a/src/services/tools/file_edit_replace_shared.ts b/src/services/tools/file_edit_replace_shared.ts new file mode 100644 index 0000000000..0e9c80037a --- /dev/null +++ b/src/services/tools/file_edit_replace_shared.ts @@ -0,0 +1,175 @@ +/** + * Shared implementation for file edit replace tools + * + * These helpers are used by both string-based and line-based replace tools, + * providing the core logic while keeping the tool definitions simple for AI providers. + */ + +interface OperationMetadata { + edits_applied: number; + lines_replaced?: number; + line_delta?: number; +} + +export interface OperationResult { + success: true; + newContent: string; + metadata: OperationMetadata; +} + +export interface OperationError { + success: false; + error: string; +} + +export type OperationOutcome = OperationResult | OperationError; + +export interface StringReplaceArgs { + file_path: string; + old_string: string; + new_string: string; + replace_count?: number; +} + +export interface LineReplaceArgs { + file_path: string; + start_line: number; + end_line: number; + new_lines: string[]; + expected_lines?: string[]; +} + +/** + * Handle string-based replacement + */ +export function handleStringReplace( + args: StringReplaceArgs, + originalContent: string +): OperationOutcome { + const replaceCount = args.replace_count ?? 1; + + if (!originalContent.includes(args.old_string)) { + return { + success: false, + error: + "old_string not found in file. The text to replace must exist exactly as written in the file.", + }; + } + + const parts = originalContent.split(args.old_string); + const occurrences = parts.length - 1; + + if (replaceCount === 1 && occurrences > 1) { + 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.`, + }; + } + + if (replaceCount > occurrences && replaceCount !== -1) { + return { + success: false, + error: `replace_count is ${replaceCount} but old_string only appears ${occurrences} time(s) in the file.`, + }; + } + + let newContent: string; + let editsApplied: number; + + if (replaceCount === -1) { + newContent = parts.join(args.new_string); + editsApplied = occurrences; + } else { + let replacedCount = 0; + let currentContent = originalContent; + + for (let i = 0; i < replaceCount; i++) { + const index = currentContent.indexOf(args.old_string); + if (index === -1) { + break; + } + + currentContent = + currentContent.substring(0, index) + + args.new_string + + currentContent.substring(index + args.old_string.length); + replacedCount++; + } + + newContent = currentContent; + editsApplied = replacedCount; + } + + return { + success: true, + newContent, + metadata: { + edits_applied: editsApplied, + }, + }; +} + +/** + * Handle line-range replacement + */ +export function handleLineReplace( + args: LineReplaceArgs, + originalContent: string +): OperationOutcome { + const startIndex = args.start_line - 1; + const endIndex = args.end_line - 1; + + if (args.start_line <= 0) { + return { + success: false, + error: `start_line must be >= 1 (received ${args.start_line}).`, + }; + } + + 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}).`, + }; + } + + const lines = originalContent.split("\n"); + + if (startIndex >= lines.length) { + return { + success: false, + error: `start_line ${args.start_line} exceeds current file length (${lines.length}).`, + }; + } + + const clampedEndIndex = Math.min(endIndex, lines.length - 1); + const currentRange = lines.slice(startIndex, clampedEndIndex + 1); + + if (args.expected_lines && !arraysEqual(currentRange, args.expected_lines)) { + return { + success: false, + error: `expected_lines validation failed. Current lines [${currentRange.join("\n")}] differ from expected [${args.expected_lines.join("\n")}].`, + }; + } + + const before = lines.slice(0, startIndex); + const after = lines.slice(clampedEndIndex + 1); + const updatedLines = [...before, ...args.new_lines, ...after]; + const linesReplaced = currentRange.length; + const totalDelta = args.new_lines.length - currentRange.length; + + return { + success: true, + newContent: updatedLines.join("\n"), + metadata: { + edits_applied: 1, + lines_replaced: linesReplaced, + line_delta: totalDelta, + }, + }; +} + +function arraysEqual(a: string[], b: string[]): boolean { + if (a.length !== b.length) return false; + return a.every((value, index) => value === b[index]); +} diff --git a/src/services/tools/file_edit_replace_string.ts b/src/services/tools/file_edit_replace_string.ts new file mode 100644 index 0000000000..8c255a0687 --- /dev/null +++ b/src/services/tools/file_edit_replace_string.ts @@ -0,0 +1,37 @@ +import { tool } from "ai"; +import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; +import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; +import { executeFileEditOperation } from "./file_edit_operation"; +import { handleStringReplace, type StringReplaceArgs } from "./file_edit_replace_shared"; + +export interface FileEditReplaceStringResult { + success: true; + diff: string; + edits_applied: number; +} + +export interface FileEditReplaceStringError { + success: false; + error: string; +} + +export type FileEditReplaceStringToolResult = + | FileEditReplaceStringResult + | FileEditReplaceStringError; + +/** + * String-based file edit replace tool factory + */ +export const createFileEditReplaceStringTool: ToolFactory = (config: ToolConfiguration) => { + return tool({ + description: TOOL_DEFINITIONS.file_edit_replace_string.description, + inputSchema: TOOL_DEFINITIONS.file_edit_replace_string.schema, + execute: async (args: StringReplaceArgs): Promise => { + return executeFileEditOperation({ + config, + filePath: args.file_path, + operation: (originalContent) => handleStringReplace(args, originalContent), + }); + }, + }); +}; diff --git a/src/types/tools.ts b/src/types/tools.ts index 11ae41c81d..2e7adb20cd 100644 --- a/src/types/tools.ts +++ b/src/types/tools.ts @@ -48,46 +48,81 @@ export type FileReadToolResult = error: string; }; -// File Edit Replace Tool Types -export interface FileEditReplaceEdit { +export interface FileEditDiffSuccessBase { + success: true; + diff: string; +} + +export interface FileEditErrorResult { + success: false; + error: string; +} + +export interface FileEditReplaceStringToolArgs { + file_path: string; old_string: string; new_string: string; - replace_count?: number; // Default: 1, -1 means replace all + replace_count?: number; } -export interface FileEditReplaceToolArgs { +export type FileEditReplaceStringToolResult = + | (FileEditDiffSuccessBase & { + edits_applied: number; + }) + | FileEditErrorResult; + +export interface FileEditReplaceLinesToolArgs { file_path: string; - edits: FileEditReplaceEdit[]; + start_line: number; + end_line: number; + new_lines: string[]; + expected_lines?: string[]; } -export type FileEditReplaceToolResult = - | { - success: true; +export type FileEditReplaceLinesToolResult = + | (FileEditDiffSuccessBase & { edits_applied: number; - diff: string; - } - | { - success: false; - error: string; - }; + lines_replaced: number; + line_delta: number; + }) + | FileEditErrorResult; + +export type FileEditSharedToolResult = + | FileEditReplaceStringToolResult + | FileEditReplaceLinesToolResult + | FileEditInsertToolResult; + +export const FILE_EDIT_TOOL_NAMES = [ + "file_edit_replace_string", + "file_edit_replace_lines", + "file_edit_insert", +] as const; + +export type FileEditToolName = (typeof FILE_EDIT_TOOL_NAMES)[number]; -// File Edit Insert Tool Types export interface FileEditInsertToolArgs { file_path: string; - line_offset: number; // 1-indexed line position (0 = insert at top, N = insert after line N) + line_offset: number; content: string; - create?: boolean; // If true, create the file if it doesn't exist (default: false) + create?: boolean; } -export type FileEditInsertToolResult = - | { - success: true; - diff: string; - } - | { - success: false; - error: string; - }; +export type FileEditInsertToolResult = FileEditDiffSuccessBase | FileEditErrorResult; + +export type FileEditToolArgs = + | FileEditReplaceStringToolArgs + | FileEditReplaceLinesToolArgs + | FileEditInsertToolArgs; + +export interface FileEditToolMessage { + toolName: FileEditToolName; + args: FileEditToolArgs; + result?: FileEditSharedToolResult; +} + +export function isFileEditToolName(value: string): value is FileEditToolName { + return (FILE_EDIT_TOOL_NAMES as readonly string[]).includes(value); +} // Propose Plan Tool Types export interface ProposePlanToolArgs { diff --git a/src/utils/main/tokenizer.ts b/src/utils/main/tokenizer.ts index 180c28dbdb..4c9f3c78aa 100644 --- a/src/utils/main/tokenizer.ts +++ b/src/utils/main/tokenizer.ts @@ -155,7 +155,8 @@ export function getToolDefinitionTokens(toolName: string, modelString: string): const fallbackSizes: Record = { bash: 65, file_read: 45, - file_edit_replace: 70, + file_edit_replace_string: 70, + file_edit_replace_lines: 80, file_edit_insert: 50, web_search: 50, google_search: 50, diff --git a/src/utils/messages/toolOutputRedaction.ts b/src/utils/messages/toolOutputRedaction.ts index 4c428845eb..8f2da0e7e9 100644 --- a/src/utils/messages/toolOutputRedaction.ts +++ b/src/utils/messages/toolOutputRedaction.ts @@ -10,7 +10,11 @@ * - Type safety: if a tool's result type changes, these redactors should fail type-checks. */ -import type { FileEditInsertToolResult, FileEditReplaceToolResult } from "@/types/tools"; +import type { + FileEditInsertToolResult, + FileEditReplaceStringToolResult, + FileEditReplaceLinesToolResult, +} from "@/types/tools"; // Tool-output from AI SDK is often wrapped like: { type: 'json', value: } // Keep this helper local so all redactors handle both wrapped and plain objects consistently. @@ -33,7 +37,9 @@ function rewrapJsonContainer(wrapped: boolean, value: unknown): unknown { } // Narrowing helpers for our tool result types -function isFileEditReplaceResult(v: unknown): v is FileEditReplaceToolResult { +function isFileEditResult( + v: unknown +): v is FileEditReplaceStringToolResult | FileEditReplaceLinesToolResult { return ( typeof v === "object" && v !== null && @@ -51,21 +57,29 @@ function isFileEditInsertResult(v: unknown): v is FileEditInsertToolResult { ); } -// Redactors per tool +// Redactors per tool - unified for both string and line replace function redactFileEditReplace(output: unknown): unknown { const unwrapped = unwrapJsonContainer(output); const val = unwrapped.value; - if (!isFileEditReplaceResult(val)) return output; // unknown structure, leave as-is + if (!isFileEditResult(val)) return output; // unknown structure, leave as-is - if (val.success) { - // Keep metadata but compact the diff to avoid huge context windows - const compact: FileEditReplaceToolResult = { + if (val.success && "edits_applied" in val) { + // Build base compact result + const compact: Record = { success: true, edits_applied: val.edits_applied, - // Replace the unified diff with a sentinel that tells the model how to fetch details if needed diff: "[diff omitted in context - call file_read on the target file if needed]", }; + + // Preserve line metadata for line-based edits (only if present) + if ("lines_replaced" in val) { + compact.lines_replaced = val.lines_replaced; + } + if ("line_delta" in val) { + compact.line_delta = val.line_delta; + } + return rewrapJsonContainer(unwrapped.wrapped, compact); } @@ -93,7 +107,8 @@ function redactFileEditInsert(output: unknown): unknown { // Public API - registry entrypoint. Add new tools here as needed. export function redactToolOutput(toolName: string, output: unknown): unknown { switch (toolName) { - case "file_edit_replace": + case "file_edit_replace_string": + case "file_edit_replace_lines": return redactFileEditReplace(output); case "file_edit_insert": return redactFileEditInsert(output); diff --git a/src/utils/tools/toolDefinitions.ts b/src/utils/tools/toolDefinitions.ts index 58d236c0f2..9e6eecfe23 100644 --- a/src/utils/tools/toolDefinitions.ts +++ b/src/utils/tools/toolDefinitions.ts @@ -66,31 +66,42 @@ export const TOOL_DEFINITIONS = { .describe("Number of lines to return from offset (optional, returns all if not specified)"), }), }, - file_edit_replace: { + 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.", schema: z.object({ file_path: z.string().describe("The absolute path to the file to edit"), - edits: z - .array( - z.object({ - old_string: z - .string() - .describe( - "The exact text to replace (must be unique in file if replace_count is 1). Include enough context (indentation, surrounding lines) to make it unique." - ), - new_string: z.string().describe("The replacement text"), - replace_count: z - .number() - .int() - .optional() - .describe( - "Number of occurrences to replace (default: 1). Use -1 to replace all occurrences. If 1, old_string must be unique in the file." - ), - }) - ) - .min(1) - .describe("Array of edits to apply sequentially"), + old_string: z + .string() + .describe( + "The exact text to replace (must be unique in file if replace_count is 1). Include enough context (indentation, surrounding lines) to make it unique." + ), + new_string: z.string().describe("The replacement text"), + replace_count: z + .number() + .int() + .optional() + .describe( + "Number of occurrences to replace (default: 1). Use -1 to replace all occurrences. If 1, old_string must be unique in the file." + ), + }), + }, + 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.", + 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"), + end_line: z.number().int().min(1).describe("1-indexed end line (inclusive) to replace"), + new_lines: z + .array(z.string()) + .describe("Replacement lines. Provide an empty array to delete the specified range."), + expected_lines: z + .array(z.string()) + .optional() + .describe( + "Optional safety check. When provided, the current lines in the specified range must match exactly." + ), }), }, file_edit_insert: { @@ -174,7 +185,8 @@ export function getAvailableTools(modelString: string): string[] { const baseTools = [ "bash", "file_read", - "file_edit_replace", + "file_edit_replace_string", + "file_edit_replace_lines", "file_edit_insert", "propose_plan", "compact_summary", diff --git a/src/utils/tools/toolPolicy.test.ts b/src/utils/tools/toolPolicy.test.ts index 56c3a99ff9..4782e9d4a4 100644 --- a/src/utils/tools/toolPolicy.test.ts +++ b/src/utils/tools/toolPolicy.test.ts @@ -14,9 +14,14 @@ const mockTools = { inputSchema: z.object({ path: z.string() }), execute: () => Promise.resolve({ content: "test" }), }), - file_edit_replace: tool({ - description: "Replace content in files", - inputSchema: z.object({ path: z.string() }), + file_edit_replace_string: tool({ + description: "Replace content in files using string matching", + inputSchema: z.object({ path: z.string(), old_string: z.string() }), + execute: () => Promise.resolve({ success: true }), + }), + file_edit_replace_lines: tool({ + description: "Replace content in files using line ranges", + inputSchema: z.object({ path: z.string(), start_line: z.number() }), execute: () => Promise.resolve({ success: true }), }), file_edit_insert: tool({ @@ -51,7 +56,8 @@ describe("applyToolPolicy", () => { expect(result.bash).toBeUndefined(); expect(result.file_read).toBeDefined(); - expect(result.file_edit_replace).toBeDefined(); + expect(result.file_edit_replace_string).toBeDefined(); + expect(result.file_edit_replace_lines).toBeDefined(); expect(result.file_edit_insert).toBeDefined(); expect(result.web_search).toBeDefined(); }); @@ -66,7 +72,8 @@ describe("applyToolPolicy", () => { expect(result.bash).toBeUndefined(); expect(result.web_search).toBeUndefined(); expect(result.file_read).toBeDefined(); - expect(result.file_edit_replace).toBeDefined(); + expect(result.file_edit_replace_string).toBeDefined(); + expect(result.file_edit_replace_lines).toBeDefined(); expect(result.file_edit_insert).toBeDefined(); }); }); @@ -76,7 +83,8 @@ describe("applyToolPolicy", () => { const policy: ToolPolicy = [{ regex_match: "file_edit_.*", action: "disable" }]; const result = applyToolPolicy(mockTools, policy); - expect(result.file_edit_replace).toBeUndefined(); + expect(result.file_edit_replace_string).toBeUndefined(); + expect(result.file_edit_replace_lines).toBeUndefined(); expect(result.file_edit_insert).toBeUndefined(); expect(result.bash).toBeDefined(); expect(result.file_read).toBeDefined(); @@ -95,7 +103,8 @@ describe("applyToolPolicy", () => { const result = applyToolPolicy(mockTools, policy); expect(result.file_read).toBeUndefined(); - expect(result.file_edit_replace).toBeUndefined(); + expect(result.file_edit_replace_string).toBeUndefined(); + expect(result.file_edit_replace_lines).toBeUndefined(); expect(result.file_edit_insert).toBeUndefined(); expect(result.bash).toBeDefined(); expect(result.web_search).toBeDefined(); @@ -112,19 +121,21 @@ describe("applyToolPolicy", () => { expect(result.bash).toBeDefined(); expect(result.file_read).toBeUndefined(); - expect(result.file_edit_replace).toBeUndefined(); + expect(result.file_edit_replace_string).toBeUndefined(); + expect(result.file_edit_replace_lines).toBeUndefined(); expect(result.file_edit_insert).toBeUndefined(); expect(result.web_search).toBeUndefined(); }); - test("disables file_edit_.* then enables file_edit_replace", () => { + test("disables file_edit_.* then enables file_edit_replace_string", () => { const policy: ToolPolicy = [ { regex_match: "file_edit_.*", action: "disable" }, - { regex_match: "file_edit_replace", action: "enable" }, + { regex_match: "file_edit_replace_string", action: "enable" }, ]; const result = applyToolPolicy(mockTools, policy); - expect(result.file_edit_replace).toBeDefined(); + expect(result.file_edit_replace_string).toBeDefined(); + expect(result.file_edit_replace_lines).toBeUndefined(); expect(result.file_edit_insert).toBeUndefined(); expect(result.bash).toBeDefined(); expect(result.file_read).toBeDefined(); @@ -149,7 +160,8 @@ describe("applyToolPolicy", () => { expect(result.file_read).toBeDefined(); expect(result.bash).toBeDefined(); - expect(result.file_edit_replace).toBeUndefined(); + expect(result.file_edit_replace_string).toBeUndefined(); + expect(result.file_edit_replace_lines).toBeUndefined(); expect(result.file_edit_insert).toBeUndefined(); }); @@ -158,7 +170,8 @@ describe("applyToolPolicy", () => { expect(result.bash).toBeDefined(); expect(result.file_read).toBeDefined(); - expect(result.file_edit_replace).toBeDefined(); + expect(result.file_edit_replace_string).toBeDefined(); + expect(result.file_edit_replace_lines).toBeDefined(); expect(result.file_edit_insert).toBeDefined(); }); @@ -172,7 +185,8 @@ describe("applyToolPolicy", () => { expect(result.bash).toBeDefined(); expect(result.file_read).toBeDefined(); - expect(result.file_edit_replace).toBeUndefined(); + expect(result.file_edit_replace_string).toBeUndefined(); + expect(result.file_edit_replace_lines).toBeUndefined(); expect(result.file_edit_insert).toBeUndefined(); expect(result.web_search).toBeUndefined(); }); @@ -202,7 +216,8 @@ describe("applyToolPolicy", () => { expect(result.bash).toBeDefined(); expect(Object.keys(result)).toHaveLength(1); expect(result.file_read).toBeUndefined(); - expect(result.file_edit_replace).toBeUndefined(); + expect(result.file_edit_replace_string).toBeUndefined(); + expect(result.file_edit_replace_lines).toBeUndefined(); expect(result.file_edit_insert).toBeUndefined(); expect(result.web_search).toBeUndefined(); }); @@ -210,7 +225,7 @@ describe("applyToolPolicy", () => { test("requires tool with regex pattern", () => { const policy: ToolPolicy = [{ regex_match: "file_.*", action: "require" }]; - // This should throw because multiple tools match (file_read, file_edit_replace, file_edit_insert) + // This should throw because multiple tools match (file_read, file_edit_replace_string, file_edit_replace_lines, file_edit_insert) expect(() => applyToolPolicy(mockTools, policy)).toThrow(/Multiple tools marked as required/); }); diff --git a/src/utils/tools/tools.ts b/src/utils/tools/tools.ts index c026e16ef4..b290bdb5c0 100644 --- a/src/utils/tools/tools.ts +++ b/src/utils/tools/tools.ts @@ -4,7 +4,8 @@ import { openai } from "@ai-sdk/openai"; import { google } from "@ai-sdk/google"; import { createFileReadTool } from "@/services/tools/file_read"; import { createBashTool } from "@/services/tools/bash"; -import { createFileEditReplaceTool } from "@/services/tools/file_edit_replace"; +import { createFileEditReplaceStringTool } from "@/services/tools/file_edit_replace_string"; +import { createFileEditReplaceLinesTool } from "@/services/tools/file_edit_replace_lines"; import { createFileEditInsertTool } from "@/services/tools/file_edit_insert"; import { createProposePlanTool } from "@/services/tools/propose_plan"; import { createCompactSummaryTool } from "@/services/tools/compact_summary"; @@ -43,7 +44,8 @@ export function getToolsForModel( const baseTools: Record = { // Use snake_case for tool names to match what seems to be the convention. file_read: createFileReadTool(config), - file_edit_replace: createFileEditReplaceTool(config), + file_edit_replace_string: createFileEditReplaceStringTool(config), + file_edit_replace_lines: createFileEditReplaceLinesTool(config), file_edit_insert: createFileEditInsertTool(config), bash: createBashTool(config), propose_plan: createProposePlanTool(config), diff --git a/tests/ipcMain/sendMessage.test.ts b/tests/ipcMain/sendMessage.test.ts index 30fd22bd5f..aafa011e22 100644 --- a/tests/ipcMain/sendMessage.test.ts +++ b/tests/ipcMain/sendMessage.test.ts @@ -1333,10 +1333,11 @@ These are general instructions that apply to all modes. e !== null && "type" in e && (e as any).type === "tool-call-end" && - (e as any).toolName === "file_edit_replace" + ((e as any).toolName === "file_edit_replace_string" || + (e as any).toolName === "file_edit_replace_lines") ) as any[]; - // Find the last successful file_edit_replace event (model may retry) + // Find the last successful file_edit_replace_* event (model may retry) const successfulEdits = allFileEditEvents.filter((e) => { const result = e?.result; const payload = result && result.value ? result.value : result;