diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index d3393d0961..0f2149e1b9 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -38,6 +38,11 @@ import { codebaseSearchTool } from "../tools/CodebaseSearchTool" import { formatResponse } from "../prompts/responses" import { sanitizeToolUseId } from "../../utils/tool-id" +import { + getLockGuardedToolExecutor, + LockGuardedToolExecutor, + type LockAcquisitionResult, +} from "../../services/file-lock" /** * Processes and presents assistant message content to the user interface. @@ -676,245 +681,275 @@ export async function presentAssistantMessage(cline: Task) { }) } - switch (block.name) { - case "write_to_file": - await checkpointSaveAndMark(cline) - await writeToFileTool.handle(cline, block as ToolUse<"write_to_file">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "update_todo_list": - await updateTodoListTool.handle(cline, block as ToolUse<"update_todo_list">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "apply_diff": - await checkpointSaveAndMark(cline) - await applyDiffToolClass.handle(cline, block as ToolUse<"apply_diff">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "edit": - case "search_and_replace": - await checkpointSaveAndMark(cline) - await editTool.handle(cline, block as ToolUse<"edit">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "search_replace": - await checkpointSaveAndMark(cline) - await searchReplaceTool.handle(cline, block as ToolUse<"search_replace">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "edit_file": - await checkpointSaveAndMark(cline) - await editFileTool.handle(cline, block as ToolUse<"edit_file">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "apply_patch": - await checkpointSaveAndMark(cline) - await applyPatchTool.handle(cline, block as ToolUse<"apply_patch">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "read_file": - // Type assertion is safe here because we're in the "read_file" case - await readFileTool.handle(cline, block as ToolUse<"read_file">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "list_files": - await listFilesTool.handle(cline, block as ToolUse<"list_files">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "codebase_search": - await codebaseSearchTool.handle(cline, block as ToolUse<"codebase_search">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "search_files": - await searchFilesTool.handle(cline, block as ToolUse<"search_files">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "execute_command": - await executeCommandTool.handle(cline, block as ToolUse<"execute_command">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "read_command_output": - await readCommandOutputTool.handle(cline, block as ToolUse<"read_command_output">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "use_mcp_tool": - await useMcpToolTool.handle(cline, block as ToolUse<"use_mcp_tool">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "access_mcp_resource": - await accessMcpResourceTool.handle(cline, block as ToolUse<"access_mcp_resource">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "ask_followup_question": - await askFollowupQuestionTool.handle(cline, block as ToolUse<"ask_followup_question">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "switch_mode": - await switchModeTool.handle(cline, block as ToolUse<"switch_mode">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "new_task": - await checkpointSaveAndMark(cline) - await newTaskTool.handle(cline, block as ToolUse<"new_task">, { - askApproval, - handleError, - pushToolResult, - toolCallId: block.id, - }) - break - case "attempt_completion": { - const completionCallbacks: AttemptCompletionCallbacks = { - askApproval, - handleError, - pushToolResult, - askFinishSubTaskApproval, - toolDescription, - } - await attemptCompletionTool.handle( - cline, - block as ToolUse<"attempt_completion">, - completionCallbacks, - ) + // Phase 7b: Acquire file locks for write tools before execution. + // This prevents concurrent tasks from writing to the same file simultaneously. + // Lock acquisition is only attempted for complete (non-partial) blocks with valid args. + let lockResult: LockAcquisitionResult | undefined + if (!block.partial && block.nativeArgs) { + const executor = getLockGuardedToolExecutor() + lockResult = executor.tryAcquireLocks( + block.name as ToolName, + block.nativeArgs as Record, + cline.taskId, + cline.cwd, + ) + if (!lockResult.success) { + const errorMsg = LockGuardedToolExecutor.formatLockConflictError(lockResult.conflicts) + pushToolResult(formatResponse.toolError(errorMsg)) break } - case "run_slash_command": - await runSlashCommandTool.handle(cline, block as ToolUse<"run_slash_command">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "skill": - await skillTool.handle(cline, block as ToolUse<"skill">, { - askApproval, - handleError, - pushToolResult, - }) - break - case "generate_image": - await checkpointSaveAndMark(cline) - await generateImageTool.handle(cline, block as ToolUse<"generate_image">, { - askApproval, - handleError, - pushToolResult, - }) - break - default: { - // Handle unknown/invalid tool names OR custom tools - // This is critical for native tool calling where every tool_use MUST have a tool_result - - // CRITICAL: Don't process partial blocks for unknown tools - just let them stream in. - // If we try to show errors for partial blocks, we'd show the error on every streaming chunk, - // creating a loop that appears to freeze the extension. Only handle complete blocks. - if (block.partial) { + } + + try { + switch (block.name) { + case "write_to_file": + await checkpointSaveAndMark(cline) + await writeToFileTool.handle(cline, block as ToolUse<"write_to_file">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "update_todo_list": + await updateTodoListTool.handle(cline, block as ToolUse<"update_todo_list">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "apply_diff": + await checkpointSaveAndMark(cline) + await applyDiffToolClass.handle(cline, block as ToolUse<"apply_diff">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "edit": + case "search_and_replace": + await checkpointSaveAndMark(cline) + await editTool.handle(cline, block as ToolUse<"edit">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "search_replace": + await checkpointSaveAndMark(cline) + await searchReplaceTool.handle(cline, block as ToolUse<"search_replace">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "edit_file": + await checkpointSaveAndMark(cline) + await editFileTool.handle(cline, block as ToolUse<"edit_file">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "apply_patch": + await checkpointSaveAndMark(cline) + await applyPatchTool.handle(cline, block as ToolUse<"apply_patch">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "read_file": + // Type assertion is safe here because we're in the "read_file" case + await readFileTool.handle(cline, block as ToolUse<"read_file">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "list_files": + await listFilesTool.handle(cline, block as ToolUse<"list_files">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "codebase_search": + await codebaseSearchTool.handle(cline, block as ToolUse<"codebase_search">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "search_files": + await searchFilesTool.handle(cline, block as ToolUse<"search_files">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "execute_command": + await executeCommandTool.handle(cline, block as ToolUse<"execute_command">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "read_command_output": + await readCommandOutputTool.handle(cline, block as ToolUse<"read_command_output">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "use_mcp_tool": + await useMcpToolTool.handle(cline, block as ToolUse<"use_mcp_tool">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "access_mcp_resource": + await accessMcpResourceTool.handle(cline, block as ToolUse<"access_mcp_resource">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "ask_followup_question": + await askFollowupQuestionTool.handle(cline, block as ToolUse<"ask_followup_question">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "switch_mode": + await switchModeTool.handle(cline, block as ToolUse<"switch_mode">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "new_task": + await checkpointSaveAndMark(cline) + await newTaskTool.handle(cline, block as ToolUse<"new_task">, { + askApproval, + handleError, + pushToolResult, + toolCallId: block.id, + }) + break + case "attempt_completion": { + const completionCallbacks: AttemptCompletionCallbacks = { + askApproval, + handleError, + pushToolResult, + askFinishSubTaskApproval, + toolDescription, + } + await attemptCompletionTool.handle( + cline, + block as ToolUse<"attempt_completion">, + completionCallbacks, + ) break } + case "run_slash_command": + await runSlashCommandTool.handle(cline, block as ToolUse<"run_slash_command">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "skill": + await skillTool.handle(cline, block as ToolUse<"skill">, { + askApproval, + handleError, + pushToolResult, + }) + break + case "generate_image": + await checkpointSaveAndMark(cline) + await generateImageTool.handle(cline, block as ToolUse<"generate_image">, { + askApproval, + handleError, + pushToolResult, + }) + break + default: { + // Handle unknown/invalid tool names OR custom tools + // This is critical for native tool calling where every tool_use MUST have a tool_result + + // CRITICAL: Don't process partial blocks for unknown tools - just let them stream in. + // If we try to show errors for partial blocks, we'd show the error on every streaming chunk, + // creating a loop that appears to freeze the extension. Only handle complete blocks. + if (block.partial) { + break + } - const customTool = stateExperiments?.customTools ? customToolRegistry.get(block.name) : undefined - - if (customTool) { - try { - let customToolArgs - - if (customTool.parameters) { - try { - customToolArgs = customTool.parameters.parse(block.nativeArgs || block.params || {}) - } catch (parseParamsError) { - const message = `Custom tool "${block.name}" argument validation failed: ${parseParamsError.message}` - console.error(message) - cline.consecutiveMistakeCount++ - await cline.say("error", message) - pushToolResult(formatResponse.toolError(message)) - break + const customTool = stateExperiments?.customTools + ? customToolRegistry.get(block.name) + : undefined + + if (customTool) { + try { + let customToolArgs + + if (customTool.parameters) { + try { + customToolArgs = customTool.parameters.parse( + block.nativeArgs || block.params || {}, + ) + } catch (parseParamsError) { + const message = `Custom tool "${block.name}" argument validation failed: ${parseParamsError.message}` + console.error(message) + cline.consecutiveMistakeCount++ + await cline.say("error", message) + pushToolResult(formatResponse.toolError(message)) + break + } } + + const result = await customTool.execute(customToolArgs, { + mode: mode ?? defaultModeSlug, + task: cline, + }) + + console.log( + `${customTool.name}.execute(): ${JSON.stringify(customToolArgs)} -> ${JSON.stringify(result)}`, + ) + + pushToolResult(result) + cline.consecutiveMistakeCount = 0 + } catch (executionError: any) { + cline.consecutiveMistakeCount++ + // Record custom tool error with static name + cline.recordToolError("custom_tool", executionError.message) + await handleError(`executing custom tool "${block.name}"`, executionError) } - const result = await customTool.execute(customToolArgs, { - mode: mode ?? defaultModeSlug, - task: cline, - }) - - console.log( - `${customTool.name}.execute(): ${JSON.stringify(customToolArgs)} -> ${JSON.stringify(result)}`, - ) - - pushToolResult(result) - cline.consecutiveMistakeCount = 0 - } catch (executionError: any) { - cline.consecutiveMistakeCount++ - // Record custom tool error with static name - cline.recordToolError("custom_tool", executionError.message) - await handleError(`executing custom tool "${block.name}"`, executionError) + break } + // Not a custom tool - handle as unknown tool error + const errorMessage = `Unknown tool "${block.name}". This tool does not exist. Please use one of the available tools.` + cline.consecutiveMistakeCount++ + cline.recordToolError(block.name as ToolName, errorMessage) + await cline.say("error", t("tools:unknownToolError", { toolName: block.name })) + // Push tool_result directly WITHOUT setting didAlreadyUseTool + // This prevents the stream from being interrupted with "Response interrupted by tool use result" + cline.pushToolResultToUserContent({ + type: "tool_result", + tool_use_id: sanitizeToolUseId(toolCallId), + content: formatResponse.toolError(errorMessage), + is_error: true, + }) break } - - // Not a custom tool - handle as unknown tool error - const errorMessage = `Unknown tool "${block.name}". This tool does not exist. Please use one of the available tools.` - cline.consecutiveMistakeCount++ - cline.recordToolError(block.name as ToolName, errorMessage) - await cline.say("error", t("tools:unknownToolError", { toolName: block.name })) - // Push tool_result directly WITHOUT setting didAlreadyUseTool - // This prevents the stream from being interrupted with "Response interrupted by tool use result" - cline.pushToolResultToUserContent({ - type: "tool_result", - tool_use_id: sanitizeToolUseId(toolCallId), - content: formatResponse.toolError(errorMessage), - is_error: true, - }) - break + } + } finally { + // Phase 7b: Release file locks acquired before tool execution. + if (lockResult?.success && lockResult.lockedPaths.length > 0) { + getLockGuardedToolExecutor().releaseLocks(lockResult.lockedPaths, cline.taskId) } } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 812d39ff41..7b8d5f74fe 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -103,6 +103,7 @@ import { restoreTodoListForTask } from "../tools/UpdateTodoListTool" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" import { RooProtectedController } from "../protect/RooProtectedController" +import { getFileLockManager } from "../../services/file-lock" import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message" import { NativeToolCallParser } from "../assistant-message/NativeToolCallParser" import { manageContext, willManageContext } from "../context-management" @@ -2361,6 +2362,14 @@ export class Task extends EventEmitter implements TaskLike { console.error("Error releasing terminals:", error) } + // Phase 7b: Release any file locks held by this task to prevent stale locks + // from blocking other tasks after this task is disposed/aborted. + try { + getFileLockManager().releaseAllLocks(this.taskId) + } catch (error) { + console.error("Error releasing file locks:", error) + } + // Cleanup command output artifacts getTaskDirectoryPath(this.globalStoragePath, this.taskId) .then((taskDir) => { diff --git a/src/services/file-lock/__tests__/tool-runner-integration.spec.ts b/src/services/file-lock/__tests__/tool-runner-integration.spec.ts new file mode 100644 index 0000000000..df7ed3ca71 --- /dev/null +++ b/src/services/file-lock/__tests__/tool-runner-integration.spec.ts @@ -0,0 +1,220 @@ +import path from "path" + +import { FileLockManager } from "../FileLockManager" +import { LockGuardedToolExecutor } from "../LockGuardedToolExecutor" +import { getFileLockManager, getLockGuardedToolExecutor, resetFileLockSingletons } from "../index" + +describe("tool-runner-integration", () => { + describe("singleton getters", () => { + afterEach(() => { + resetFileLockSingletons() + }) + + it("getFileLockManager returns a FileLockManager instance", () => { + const manager = getFileLockManager() + expect(manager).toBeInstanceOf(FileLockManager) + }) + + it("getFileLockManager returns the same instance on repeated calls", () => { + const a = getFileLockManager() + const b = getFileLockManager() + expect(a).toBe(b) + }) + + it("getLockGuardedToolExecutor returns a LockGuardedToolExecutor instance", () => { + const executor = getLockGuardedToolExecutor() + expect(executor).toBeInstanceOf(LockGuardedToolExecutor) + }) + + it("getLockGuardedToolExecutor returns the same instance on repeated calls", () => { + const a = getLockGuardedToolExecutor() + const b = getLockGuardedToolExecutor() + expect(a).toBe(b) + }) + + it("resetFileLockSingletons creates fresh instances", () => { + const before = getFileLockManager() + resetFileLockSingletons() + const after = getFileLockManager() + expect(before).not.toBe(after) + }) + + it("getLockGuardedToolExecutor uses getFileLockManager singleton", () => { + const executor = getLockGuardedToolExecutor() + const manager = getFileLockManager() + + // Acquire a lock through the manager, verify executor sees it + const absPath = path.resolve("/workspace", "test.ts") + manager.acquireLock(absPath, "task-1") + + const result = executor.tryAcquireLocks( + "write_to_file", + { path: "test.ts", content: "hello" }, + "task-2", + "/workspace", + ) + + expect(result.success).toBe(false) + if (!result.success) { + expect(result.conflicts[0].holdingTaskId).toBe("task-1") + } + }) + }) + + describe("lock guard lifecycle in tool execution flow", () => { + let lockManager: FileLockManager + let executor: LockGuardedToolExecutor + + beforeEach(() => { + lockManager = new FileLockManager() + executor = new LockGuardedToolExecutor(lockManager) + }) + + it("acquires and releases locks around a successful write_to_file", () => { + const cwd = "/workspace" + const params = { path: "src/app.ts", content: "code" } + const taskId = "task-1" + + // Acquire + const result = executor.tryAcquireLocks("write_to_file", params, taskId, cwd) + expect(result.success).toBe(true) + if (!result.success) return + + // Lock is held + const absPath = path.resolve(cwd, "src/app.ts") + expect(lockManager.getLockHolder(absPath)).toBe(taskId) + + // Simulate tool execution... + + // Release + executor.releaseLocks(result.lockedPaths, taskId) + + // Lock is released + expect(lockManager.getLockHolder(absPath)).toBeUndefined() + }) + + it("acquires and releases locks around a successful apply_patch with multiple files", () => { + const cwd = "/workspace" + const patch = ["*** Update File: src/a.ts", "some diff", "*** Add File: src/b.ts", "some content"].join( + "\n", + ) + const params = { patch } + const taskId = "task-1" + + const result = executor.tryAcquireLocks("apply_patch", params, taskId, cwd) + expect(result.success).toBe(true) + if (!result.success) return + expect(result.lockedPaths).toHaveLength(2) + + // Both locks held + expect(lockManager.getLockHolder(path.resolve(cwd, "src/a.ts"))).toBe(taskId) + expect(lockManager.getLockHolder(path.resolve(cwd, "src/b.ts"))).toBe(taskId) + + // Release + executor.releaseLocks(result.lockedPaths, taskId) + + // Both released + expect(lockManager.getLockHolder(path.resolve(cwd, "src/a.ts"))).toBeUndefined() + expect(lockManager.getLockHolder(path.resolve(cwd, "src/b.ts"))).toBeUndefined() + }) + + it("blocks a second task from writing to a locked file", () => { + const cwd = "/workspace" + const params = { path: "src/shared.ts", content: "x" } + + // Task 1 acquires + const result1 = executor.tryAcquireLocks("write_to_file", params, "task-1", cwd) + expect(result1.success).toBe(true) + + // Task 2 tries to write same file + const result2 = executor.tryAcquireLocks("write_to_file", params, "task-2", cwd) + expect(result2.success).toBe(false) + if (!result2.success) { + expect(result2.conflicts).toHaveLength(1) + expect(result2.conflicts[0].holdingTaskId).toBe("task-1") + } + + // Task 1 releases + if (result1.success) { + executor.releaseLocks(result1.lockedPaths, "task-1") + } + + // Task 2 can now acquire + const result3 = executor.tryAcquireLocks("write_to_file", params, "task-2", cwd) + expect(result3.success).toBe(true) + }) + + it("releaseAllLocks on dispose frees all locks for a task", () => { + const cwd = "/workspace" + + // Task 1 acquires locks on two files + executor.tryAcquireLocks("write_to_file", { path: "a.ts", content: "a" }, "task-1", cwd) + executor.tryAcquireLocks("write_to_file", { path: "b.ts", content: "b" }, "task-1", cwd) + + expect(lockManager.getLockHolder(path.resolve(cwd, "a.ts"))).toBe("task-1") + expect(lockManager.getLockHolder(path.resolve(cwd, "b.ts"))).toBe("task-1") + + // Simulate Task.dispose() calling releaseAllLocks + lockManager.releaseAllLocks("task-1") + + expect(lockManager.getLockHolder(path.resolve(cwd, "a.ts"))).toBeUndefined() + expect(lockManager.getLockHolder(path.resolve(cwd, "b.ts"))).toBeUndefined() + }) + + it("does not acquire locks for read-only tools", () => { + const cwd = "/workspace" + const result = executor.tryAcquireLocks("read_file", { path: "a.ts" }, "task-1", cwd) + expect(result.success).toBe(true) + if (result.success) { + expect(result.lockedPaths).toEqual([]) + } + }) + + it("formats lock conflict errors with useful information", () => { + const conflicts = [ + { + filePath: "/workspace/src/shared.ts", + holdingTaskId: "task-abc-123", + heldForMs: 8000, + }, + ] + const msg = LockGuardedToolExecutor.formatLockConflictError(conflicts) + + expect(msg).toContain("Cannot write") + expect(msg).toContain("/workspace/src/shared.ts") + expect(msg).toContain("task-abc-123") + expect(msg).toContain("8s") + expect(msg).toContain("Wait for the other task") + }) + + it("handles edit_file tool path extraction", () => { + const cwd = "/workspace" + const result = executor.tryAcquireLocks( + "edit_file", + { file_path: "src/utils.ts", old_string: "a", new_string: "b" }, + "task-1", + cwd, + ) + expect(result.success).toBe(true) + if (result.success) { + expect(result.lockedPaths).toHaveLength(1) + expect(result.lockedPaths[0]).toBe(path.resolve(cwd, "src/utils.ts")) + } + }) + + it("handles search_replace tool path extraction", () => { + const cwd = "/workspace" + const result = executor.tryAcquireLocks( + "search_replace", + { file_path: "src/config.ts", old_string: "x", new_string: "y" }, + "task-1", + cwd, + ) + expect(result.success).toBe(true) + if (result.success) { + expect(result.lockedPaths).toHaveLength(1) + expect(result.lockedPaths[0]).toBe(path.resolve(cwd, "src/config.ts")) + } + }) + }) +}) diff --git a/src/services/file-lock/index.ts b/src/services/file-lock/index.ts index ca893cd869..8d6896b83a 100644 --- a/src/services/file-lock/index.ts +++ b/src/services/file-lock/index.ts @@ -13,3 +13,46 @@ export { WRITE_TOOL_NAMES, type LockAcquisitionResult, } from "./LockGuardedToolExecutor" + +import { FileLockManager } from "./FileLockManager" +import { LockGuardedToolExecutor } from "./LockGuardedToolExecutor" + +/** + * Module-level singleton instances for the file lock subsystem. + * Lazily initialized on first access. + */ +let _fileLockManager: FileLockManager | undefined +let _lockGuardedToolExecutor: LockGuardedToolExecutor | undefined + +/** + * Get the singleton FileLockManager instance. + * Creates it on first call. + */ +export function getFileLockManager(): FileLockManager { + if (!_fileLockManager) { + _fileLockManager = new FileLockManager() + } + return _fileLockManager +} + +/** + * Get the singleton LockGuardedToolExecutor instance. + * Creates it (and the underlying FileLockManager) on first call. + */ +export function getLockGuardedToolExecutor(): LockGuardedToolExecutor { + if (!_lockGuardedToolExecutor) { + _lockGuardedToolExecutor = new LockGuardedToolExecutor(getFileLockManager()) + } + return _lockGuardedToolExecutor +} + +/** + * Reset singleton instances. Intended for testing only. + */ +export function resetFileLockSingletons(): void { + if (_fileLockManager) { + _fileLockManager.dispose() + } + _fileLockManager = undefined + _lockGuardedToolExecutor = undefined +}