mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
feat: wire LockGuardedToolExecutor into tool runner and Task.dispose()
- Integrate file lock guards into presentAssistantMessage.ts tool dispatch - Acquire locks before write tool execution, release in finally block - Block concurrent writes with LLM-friendly conflict error messages - Wire releaseAllLocks into Task.dispose() for stale lock cleanup - Add singleton getters (getFileLockManager, getLockGuardedToolExecutor) - Add 14 integration tests covering lifecycle, conflicts, and cleanup Phase 7b tool runner integration for parallel agent file coordination.
This commit is contained in:
Roo Code
parent
77fe7cf6f7
commit
363436c442
4 changed files with 536 additions and 229 deletions
|
|
@ -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<string, unknown>,
|
||||
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
|
||||
|
||||
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)
|
||||
// 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 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)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<TaskEvents> 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) => {
|
||||
|
|
|
|||
220
src/services/file-lock/__tests__/tool-runner-integration.spec.ts
Normal file
220
src/services/file-lock/__tests__/tool-runner-integration.spec.ts
Normal file
|
|
@ -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"))
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue