From 77fe7cf6f71d07bb508faf16717a9f7e12446a14 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 15:39:31 +0000 Subject: [PATCH] feat: Phase 7b - LockGuardedToolExecutor for write tool lock integration (#12330) --- .../file-lock/LockGuardedToolExecutor.ts | 229 +++++++++++++++ .../__tests__/LockGuardedToolExecutor.spec.ts | 277 ++++++++++++++++++ src/services/file-lock/index.ts | 7 + 3 files changed, 513 insertions(+) create mode 100644 src/services/file-lock/LockGuardedToolExecutor.ts create mode 100644 src/services/file-lock/__tests__/LockGuardedToolExecutor.spec.ts diff --git a/src/services/file-lock/LockGuardedToolExecutor.ts b/src/services/file-lock/LockGuardedToolExecutor.ts new file mode 100644 index 0000000000..8a3549560a --- /dev/null +++ b/src/services/file-lock/LockGuardedToolExecutor.ts @@ -0,0 +1,229 @@ +import path from "path" + +import type { ToolName } from "@roo-code/types" + +import { FileLockManager, type LockConflict } from "./FileLockManager" + +/** + * Set of tool names that perform file write operations and require lock guards. + */ +export const WRITE_TOOL_NAMES: ReadonlySet = new Set([ + "write_to_file", + "apply_diff", + "apply_patch", + "edit_file", + "search_replace", + "search_and_replace", +]) + +/** + * Patch file header markers used by apply_patch to specify file operations. + */ +const PATCH_FILE_MARKERS = ["*** Add File: ", "*** Delete File: ", "*** Update File: "] as const + +/** + * Extract target file paths from tool parameters based on tool name. + * + * Each write tool encodes the target file path differently: + * - write_to_file, apply_diff: `params.path` + * - edit_file, search_replace, search_and_replace: `params.file_path` + * - apply_patch: multiple paths embedded in the patch content + * + * @returns Array of relative file paths the tool intends to write to. + */ +export function extractWriteTargetPaths(toolName: ToolName, params: Record): string[] { + switch (toolName) { + case "write_to_file": + case "apply_diff": { + const p = params.path + if (typeof p === "string" && p.length > 0) { + return [p] + } + return [] + } + + case "edit_file": + case "search_replace": + case "search_and_replace": { + const p = params.file_path + if (typeof p === "string" && p.length > 0) { + return [p] + } + return [] + } + + case "apply_patch": { + return extractFilePathsFromPatch(params.patch) + } + + default: + return [] + } +} + +/** + * Extract file paths from apply_patch content. + * The patch format uses markers like "*** Add File: path", "*** Delete File: path", etc. + */ +function extractFilePathsFromPatch(patchContent: unknown): string[] { + if (typeof patchContent !== "string" || patchContent.length === 0) { + return [] + } + + const filePaths: string[] = [] + const lines = patchContent.split("\n") + + for (const line of lines) { + for (const marker of PATCH_FILE_MARKERS) { + if (line.startsWith(marker)) { + const filePath = line.substring(marker.length).trim() + if (filePath) { + filePaths.push(filePath) + } + break + } + } + } + + return filePaths +} + +/** + * Result of attempting to acquire locks for a tool execution. + */ +export type LockAcquisitionResult = + | { success: true; lockedPaths: string[] } + | { success: false; conflicts: LockConflict[]; lockedPaths: string[] } + +/** + * Orchestrates file lock acquisition and release around write tool executions. + * + * This executor is designed to be called by the tool execution layer before + * invoking a write tool. It: + * + * 1. Extracts target file paths from the tool's parameters + * 2. Attempts to acquire locks on all target files for the given task + * 3. If any lock fails, releases all locks acquired in this batch and returns conflicts + * 4. On success, the caller executes the tool, then calls `releaseLocks()` + * + * Usage: + * ```typescript + * const executor = new LockGuardedToolExecutor(fileLockManager) + * const result = executor.tryAcquireLocks("write_to_file", params, taskId, cwd) + * + * if (!result.success) { + * // Report conflicts to the LLM + * return formatLockConflictError(result.conflicts) + * } + * + * try { + * await tool.execute(params, task, callbacks) + * } finally { + * executor.releaseLocks(result.lockedPaths, taskId) + * } + * ``` + */ +export class LockGuardedToolExecutor { + constructor(private readonly lockManager: FileLockManager) {} + + /** + * Check if the given tool name is a write tool that requires lock guards. + */ + isWriteTool(toolName: ToolName): boolean { + return WRITE_TOOL_NAMES.has(toolName) + } + + /** + * Attempt to acquire file locks for all files a write tool targets. + * + * If the tool is not a write tool or has no extractable paths, returns + * success with an empty lockedPaths array (no locks needed). + * + * Uses all-or-nothing semantics: if any file can't be locked, all locks + * acquired in this batch are released and the conflicts are returned. + * + * @param toolName - The tool being executed + * @param params - The tool's parameters + * @param taskId - The ID of the task executing the tool + * @param cwd - The working directory for resolving relative paths + * @returns Lock acquisition result + */ + tryAcquireLocks( + toolName: ToolName, + params: Record, + taskId: string, + cwd: string, + ): LockAcquisitionResult { + if (!this.isWriteTool(toolName)) { + return { success: true, lockedPaths: [] } + } + + const relativePaths = extractWriteTargetPaths(toolName, params) + + if (relativePaths.length === 0) { + return { success: true, lockedPaths: [] } + } + + // Resolve to absolute paths for consistent locking + const absolutePaths = relativePaths.map((p) => path.resolve(cwd, p)) + + // Sort paths to prevent deadlocks when multiple tools lock multiple files + const sortedPaths = [...absolutePaths].sort() + + const lockedPaths: string[] = [] + const conflicts: LockConflict[] = [] + + for (const absPath of sortedPaths) { + const acquired = this.lockManager.acquireLock(absPath, taskId) + + if (acquired) { + lockedPaths.push(absPath) + } else { + // Collect conflict info + const conflict = this.lockManager.getLockConflict(absPath, taskId) + if (conflict) { + conflicts.push(conflict) + } + } + } + + // All-or-nothing: if any conflict, release everything we acquired + if (conflicts.length > 0) { + for (const locked of lockedPaths) { + this.lockManager.releaseLock(locked, taskId) + } + return { success: false, conflicts, lockedPaths: [] } + } + + return { success: true, lockedPaths } + } + + /** + * Release locks on the specified paths for a task. + * Should be called in a `finally` block after tool execution. + */ + releaseLocks(lockedPaths: string[], taskId: string): void { + for (const absPath of lockedPaths) { + this.lockManager.releaseLock(absPath, taskId) + } + } + + /** + * Format a human-readable error message for lock conflicts. + * This message is intended to be returned to the LLM so it can + * understand why the write was blocked and take corrective action. + */ + static formatLockConflictError(conflicts: LockConflict[]): string { + const lines = ["Cannot write to the following file(s) because they are locked by another task:", ""] + + for (const conflict of conflicts) { + const heldSec = Math.round(conflict.heldForMs / 1000) + lines.push(` - ${conflict.filePath} (locked by task ${conflict.holdingTaskId} for ${heldSec}s)`) + } + + lines.push("") + lines.push("Wait for the other task to finish writing, or work on a different file.") + + return lines.join("\n") + } +} diff --git a/src/services/file-lock/__tests__/LockGuardedToolExecutor.spec.ts b/src/services/file-lock/__tests__/LockGuardedToolExecutor.spec.ts new file mode 100644 index 0000000000..71f74a3895 --- /dev/null +++ b/src/services/file-lock/__tests__/LockGuardedToolExecutor.spec.ts @@ -0,0 +1,277 @@ +import path from "path" + +import { FileLockManager } from "../FileLockManager" +import { LockGuardedToolExecutor, extractWriteTargetPaths, WRITE_TOOL_NAMES } from "../LockGuardedToolExecutor" + +describe("extractWriteTargetPaths", () => { + it("extracts path from write_to_file params", () => { + expect(extractWriteTargetPaths("write_to_file", { path: "src/foo.ts", content: "hello" })).toEqual([ + "src/foo.ts", + ]) + }) + + it("extracts path from apply_diff params", () => { + expect(extractWriteTargetPaths("apply_diff", { path: "lib/bar.ts", diff: "---" })).toEqual(["lib/bar.ts"]) + }) + + it("extracts file_path from edit_file params", () => { + expect( + extractWriteTargetPaths("edit_file", { + file_path: "a/b.ts", + old_string: "x", + new_string: "y", + }), + ).toEqual(["a/b.ts"]) + }) + + it("extracts file_path from search_replace params", () => { + expect( + extractWriteTargetPaths("search_replace", { + file_path: "c/d.ts", + old_string: "x", + new_string: "y", + }), + ).toEqual(["c/d.ts"]) + }) + + it("extracts file_path from search_and_replace params", () => { + expect( + extractWriteTargetPaths("search_and_replace", { + file_path: "e/f.ts", + old_string: "x", + new_string: "y", + }), + ).toEqual(["e/f.ts"]) + }) + + it("extracts multiple paths from apply_patch params", () => { + const patch = [ + "*** Update File: src/a.ts", + "some diff content", + "*** Add File: src/b.ts", + "file content", + "*** Delete File: src/c.ts", + ].join("\n") + + expect(extractWriteTargetPaths("apply_patch", { patch })).toEqual(["src/a.ts", "src/b.ts", "src/c.ts"]) + }) + + it("returns empty array for apply_patch with no recognizable paths", () => { + expect(extractWriteTargetPaths("apply_patch", { patch: "random content" })).toEqual([]) + }) + + it("returns empty array for apply_patch with empty/missing patch", () => { + expect(extractWriteTargetPaths("apply_patch", { patch: "" })).toEqual([]) + expect(extractWriteTargetPaths("apply_patch", {})).toEqual([]) + }) + + it("returns empty array for missing path param", () => { + expect(extractWriteTargetPaths("write_to_file", {})).toEqual([]) + expect(extractWriteTargetPaths("write_to_file", { path: "" })).toEqual([]) + }) + + it("returns empty array for non-write tools", () => { + expect(extractWriteTargetPaths("read_file", { path: "foo.ts" })).toEqual([]) + expect(extractWriteTargetPaths("list_files", { path: "." })).toEqual([]) + }) +}) + +describe("WRITE_TOOL_NAMES", () => { + it("contains all expected write tools", () => { + expect(WRITE_TOOL_NAMES.has("write_to_file")).toBe(true) + expect(WRITE_TOOL_NAMES.has("apply_diff")).toBe(true) + expect(WRITE_TOOL_NAMES.has("apply_patch")).toBe(true) + expect(WRITE_TOOL_NAMES.has("edit_file")).toBe(true) + expect(WRITE_TOOL_NAMES.has("search_replace")).toBe(true) + expect(WRITE_TOOL_NAMES.has("search_and_replace")).toBe(true) + }) + + it("does not contain read tools", () => { + expect(WRITE_TOOL_NAMES.has("read_file")).toBe(false) + expect(WRITE_TOOL_NAMES.has("list_files")).toBe(false) + }) +}) + +describe("LockGuardedToolExecutor", () => { + let lockManager: FileLockManager + let executor: LockGuardedToolExecutor + const cwd = "/workspace" + + beforeEach(() => { + lockManager = new FileLockManager() + executor = new LockGuardedToolExecutor(lockManager) + }) + + afterEach(() => { + lockManager.dispose() + }) + + describe("isWriteTool", () => { + it("returns true for write tools", () => { + expect(executor.isWriteTool("write_to_file")).toBe(true) + expect(executor.isWriteTool("apply_diff")).toBe(true) + expect(executor.isWriteTool("apply_patch")).toBe(true) + expect(executor.isWriteTool("edit_file")).toBe(true) + expect(executor.isWriteTool("search_replace")).toBe(true) + }) + + it("returns false for non-write tools", () => { + expect(executor.isWriteTool("read_file")).toBe(false) + expect(executor.isWriteTool("list_files")).toBe(false) + expect(executor.isWriteTool("execute_command")).toBe(false) + }) + }) + + describe("tryAcquireLocks", () => { + it("returns success with empty lockedPaths for non-write tools", () => { + const result = executor.tryAcquireLocks("read_file", { path: "foo.ts" }, "task-1", cwd) + expect(result).toEqual({ success: true, lockedPaths: [] }) + }) + + it("returns success with empty lockedPaths when no paths extracted", () => { + const result = executor.tryAcquireLocks("write_to_file", {}, "task-1", cwd) + expect(result).toEqual({ success: true, lockedPaths: [] }) + }) + + it("acquires lock for a single file write", () => { + const result = executor.tryAcquireLocks( + "write_to_file", + { path: "src/foo.ts", content: "hello" }, + "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/foo.ts")) + } + + // Verify the lock is held in the manager + expect(lockManager.getLockHolder(path.resolve(cwd, "src/foo.ts"))).toBe("task-1") + }) + + it("acquires locks for multiple files in apply_patch", () => { + const patch = ["*** Update File: src/a.ts", "diff", "*** Add File: src/b.ts", "content"].join("\n") + + const result = executor.tryAcquireLocks("apply_patch", { patch }, "task-1", cwd) + + expect(result.success).toBe(true) + if (result.success) { + expect(result.lockedPaths).toHaveLength(2) + } + }) + + it("allows re-entrant lock by same task", () => { + // First acquire + const result1 = executor.tryAcquireLocks( + "write_to_file", + { path: "src/foo.ts", content: "a" }, + "task-1", + cwd, + ) + expect(result1.success).toBe(true) + + // Same task, same file -- should succeed (re-entrant) + const result2 = executor.tryAcquireLocks( + "write_to_file", + { path: "src/foo.ts", content: "b" }, + "task-1", + cwd, + ) + expect(result2.success).toBe(true) + }) + + it("fails when another task holds the lock", () => { + // Task 1 locks the file + lockManager.acquireLock(path.resolve(cwd, "src/foo.ts"), "task-1") + + // Task 2 tries to write + const result = executor.tryAcquireLocks( + "write_to_file", + { path: "src/foo.ts", content: "conflict" }, + "task-2", + cwd, + ) + + expect(result.success).toBe(false) + if (!result.success) { + expect(result.conflicts).toHaveLength(1) + expect(result.conflicts[0].holdingTaskId).toBe("task-1") + expect(result.lockedPaths).toEqual([]) + } + }) + + it("uses all-or-nothing semantics for multi-file patches", () => { + // Task 1 locks one of the files + lockManager.acquireLock(path.resolve(cwd, "src/b.ts"), "task-1") + + const patch = ["*** Update File: src/a.ts", "diff a", "*** Update File: src/b.ts", "diff b"].join("\n") + + const result = executor.tryAcquireLocks("apply_patch", { patch }, "task-2", cwd) + + expect(result.success).toBe(false) + if (!result.success) { + expect(result.conflicts).toHaveLength(1) + // a.ts should have been rolled back + expect(result.lockedPaths).toEqual([]) + } + + // Verify a.ts was NOT left locked by task-2 + expect(lockManager.getLockHolder(path.resolve(cwd, "src/a.ts"))).toBeUndefined() + }) + }) + + describe("releaseLocks", () => { + it("releases all specified locks", () => { + const absPath = path.resolve(cwd, "src/foo.ts") + lockManager.acquireLock(absPath, "task-1") + + executor.releaseLocks([absPath], "task-1") + + expect(lockManager.getLockHolder(absPath)).toBeUndefined() + }) + + it("is safe to call with empty array", () => { + expect(() => executor.releaseLocks([], "task-1")).not.toThrow() + }) + + it("is safe to call for locks not held", () => { + expect(() => executor.releaseLocks([path.resolve(cwd, "nonexistent.ts")], "task-1")).not.toThrow() + }) + }) + + describe("formatLockConflictError", () => { + it("formats a readable error message", () => { + const conflicts = [ + { + filePath: "/workspace/src/foo.ts", + holdingTaskId: "task-abc", + heldForMs: 5000, + }, + ] + + const msg = LockGuardedToolExecutor.formatLockConflictError(conflicts) + + expect(msg).toContain("Cannot write") + expect(msg).toContain("/workspace/src/foo.ts") + expect(msg).toContain("task-abc") + expect(msg).toContain("5s") + expect(msg).toContain("Wait for the other task") + }) + + it("formats multiple conflicts", () => { + const conflicts = [ + { filePath: "/workspace/a.ts", holdingTaskId: "t1", heldForMs: 2000 }, + { filePath: "/workspace/b.ts", holdingTaskId: "t2", heldForMs: 10000 }, + ] + + const msg = LockGuardedToolExecutor.formatLockConflictError(conflicts) + + expect(msg).toContain("a.ts") + expect(msg).toContain("b.ts") + expect(msg).toContain("t1") + expect(msg).toContain("t2") + }) + }) +}) diff --git a/src/services/file-lock/index.ts b/src/services/file-lock/index.ts index bd61c47d85..ca893cd869 100644 --- a/src/services/file-lock/index.ts +++ b/src/services/file-lock/index.ts @@ -6,3 +6,10 @@ export { type FileLockEvent, type FileLockEventListener, } from "./FileLockManager" + +export { + LockGuardedToolExecutor, + extractWriteTargetPaths, + WRITE_TOOL_NAMES, + type LockAcquisitionResult, +} from "./LockGuardedToolExecutor"