feat: Phase 7b - LockGuardedToolExecutor for write tool lock integration (#12330)

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<ToolName> = new Set<ToolName>([
+	"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, unknown>): 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<string, unknown>,
+		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")
+	}
+}

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")
+		})
+	})
+})

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"