fix: address review feedback — hoist globalStoragePath, read-merge-write disk fallback, add delegation retry tests

hannesrudolph · hannesrudolph · commit 67e1d8a1bb11 · 2026-02-11T17:16:02.000-07:00
- P3: Hoist redundant globalStoragePath computation in getTaskWithId (ClineProvider.ts)
- Fix AttemptCompletionTool disk fallback blind overwrite: add readDelegationMeta to
  DelegationProvider interface and use read-merge-write pattern to preserve
  completedByChildId, completionResultSummary, and merge childIds
- Add readDelegationMeta convenience wrapper on ClineProvider
- P2: Add 5 tests for delegateToParent retry-once and parent-repair fallback:
  - First attempt fails, retry succeeds
  - Both attempts fail, parent repaired to active
  - Disk fallback with existing meta preserves all fields
  - Disk fallback with no existing meta uses null defaults
  - Approval denied skips delegation
diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts
@@ -34,6 +34,7 @@ interface DelegationProvider {
 	}): Promise<void>
 	updateTaskHistory(item: HistoryItem, options?: { broadcast?: boolean }): Promise<HistoryItem[]>
 	persistDelegationMeta(taskId: string, meta: DelegationMeta): Promise<void>
+	readDelegationMeta(taskId: string): Promise<DelegationMeta | null>
 }
 
 export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
@@ -221,14 +222,21 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
 					childIds,
 				})
 			} catch (repairError) {
-				// Disk-only fallback when parent is missing from globalState
+				// Disk-only fallback when parent is missing from globalState.
+				// Uses read-merge-write to preserve fields like completedByChildId
+				// and completionResultSummary that may exist from prior delegations.
 				if (repairError instanceof Error && repairError.message === "Task not found") {
 					try {
+						const existingMeta = await provider.readDelegationMeta(parentTaskId)
 						await provider.persistDelegationMeta(parentTaskId, {
 							status: "active",
 							awaitingChildId: null,
-							delegatedToId: null,
-							childIds: [childTaskId],
+							delegatedToId: existingMeta?.delegatedToId ?? null,
+							childIds: existingMeta?.childIds
+								? Array.from(new Set([...existingMeta.childIds, childTaskId]))
+								: [childTaskId],
+							completedByChildId: existingMeta?.completedByChildId ?? null,
+							completionResultSummary: existingMeta?.completionResultSummary ?? null,
 						})
 						console.warn(
 							`[AttemptCompletionTool] Repaired parent ${parentTaskId} via disk fallback (not in globalState)`,
diff --git a/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts b/src/core/tools/__tests__/attemptCompletionDelegation.spec.ts
@@ -0,0 +1,308 @@
+// npx vitest core/tools/__tests__/attemptCompletionDelegation.spec.ts
+
+import type { HistoryItem } from "@roo-code/types"
+import type { DelegationMeta } from "../../task-persistence/delegationMeta"
+
+// Mock vscode
+vi.mock("vscode", () => ({
+	workspace: {
+		getConfiguration: vi.fn(() => ({
+			get: vi.fn(),
+		})),
+	},
+	env: {
+		uriScheme: "vscode",
+	},
+}))
+
+vi.mock("../../../shared/package", () => ({
+	Package: {
+		name: "roo-cline",
+		publisher: "RooVeterinaryInc",
+		version: "1.0.0",
+		outputChannel: "Roo-Code",
+	},
+}))
+
+vi.mock("../../prompts/responses", () => ({
+	formatResponse: {
+		toolError: vi.fn((msg: string) => `Tool Error: ${msg}`),
+		toolResult: vi.fn((text: string) => text),
+	},
+}))
+
+vi.mock("../../../i18n", () => ({
+	t: vi.fn((key: string) => key),
+}))
+
+vi.mock("@roo-code/telemetry", () => ({
+	TelemetryService: {
+		instance: {
+			captureTaskCompleted: vi.fn(),
+		},
+	},
+}))
+
+import { AttemptCompletionTool } from "../AttemptCompletionTool"
+
+/** Helper: minimal HistoryItem for a child task. */
+function childHistoryItem(overrides: Partial<HistoryItem> = {}): HistoryItem {
+	return {
+		id: "child-1",
+		number: 2,
+		ts: Date.now(),
+		task: "child task",
+		tokensIn: 0,
+		tokensOut: 0,
+		totalCost: 0,
+		status: "active",
+		...overrides,
+	}
+}
+
+/** Helper: minimal HistoryItem for a parent task. */
+function parentHistoryItem(overrides: Partial<HistoryItem> = {}): HistoryItem {
+	return {
+		id: "parent-1",
+		number: 1,
+		ts: Date.now(),
+		task: "parent task",
+		tokensIn: 0,
+		tokensOut: 0,
+		totalCost: 0,
+		status: "delegated",
+		awaitingChildId: "child-1",
+		delegatedToId: "mode-1",
+		childIds: ["child-1"],
+		completedByChildId: "old-child-0",
+		completionResultSummary: "old result summary",
+		...overrides,
+	}
+}
+
+function createMockProvider() {
+	return {
+		getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistoryItem() }),
+		reopenParentFromDelegation: vi.fn().mockResolvedValue(undefined),
+		updateTaskHistory: vi.fn().mockResolvedValue([]),
+		persistDelegationMeta: vi.fn().mockResolvedValue(undefined),
+		readDelegationMeta: vi.fn().mockResolvedValue(null),
+	}
+}
+
+function createMockTask(provider: ReturnType<typeof createMockProvider>) {
+	return {
+		taskId: "child-1",
+		parentTaskId: "parent-1",
+		consecutiveMistakeCount: 0,
+		recordToolError: vi.fn(),
+		say: vi.fn().mockResolvedValue(undefined),
+		ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }),
+		emitFinalTokenUsageUpdate: vi.fn(),
+		emit: vi.fn(),
+		getTokenUsage: vi.fn().mockReturnValue({}),
+		toolUsage: {},
+		clineMessages: [],
+		providerRef: { deref: () => provider },
+		sayAndCreateMissingParamError: vi.fn().mockResolvedValue("missing param"),
+	}
+}
+
+function createCallbacks(overrides: Record<string, unknown> = {}) {
+	return {
+		askApproval: vi.fn(),
+		handleError: vi.fn(),
+		pushToolResult: vi.fn(),
+		askFinishSubTaskApproval: vi.fn().mockResolvedValue(true),
+		toolDescription: () => "attempt_completion tool",
+		...overrides,
+	}
+}
+
+describe("AttemptCompletionTool delegation retry and parent-repair", () => {
+	let tool: AttemptCompletionTool
+
+	beforeEach(() => {
+		vi.useFakeTimers()
+		tool = new AttemptCompletionTool()
+	})
+
+	afterEach(() => {
+		vi.useRealTimers()
+	})
+
+	it("should succeed on retry when first delegation attempt fails", async () => {
+		const provider = createMockProvider()
+		const task = createMockTask(provider)
+		const callbacks = createCallbacks()
+
+		// First call fails, second succeeds
+		provider.reopenParentFromDelegation
+			.mockRejectedValueOnce(new Error("transient race"))
+			.mockResolvedValueOnce(undefined)
+
+		// Child task status is "active"
+		provider.getTaskWithId.mockResolvedValue({
+			historyItem: childHistoryItem(),
+		})
+
+		const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any)
+
+		// Advance past the 500ms retry delay
+		await vi.advanceTimersByTimeAsync(600)
+		await executePromise
+
+		// Verify retry happened (2 calls to reopenParentFromDelegation)
+		expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(2)
+
+		// Verify delegation succeeded — pushToolResult called with empty string
+		expect(callbacks.pushToolResult).toHaveBeenCalledWith("")
+
+		// Verify no parent repair was attempted
+		expect(provider.updateTaskHistory).not.toHaveBeenCalled()
+	})
+
+	it("should repair parent to active when both delegation attempts fail", async () => {
+		const provider = createMockProvider()
+		const task = createMockTask(provider)
+		const callbacks = createCallbacks()
+
+		// Both delegation calls fail
+		provider.reopenParentFromDelegation
+			.mockRejectedValueOnce(new Error("first failure"))
+			.mockRejectedValueOnce(new Error("second failure"))
+
+		// First call: child status lookup; second call: parent lookup for repair
+		provider.getTaskWithId
+			.mockResolvedValueOnce({ historyItem: childHistoryItem() })
+			.mockResolvedValueOnce({ historyItem: parentHistoryItem() })
+
+		const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any)
+
+		await vi.advanceTimersByTimeAsync(600)
+		await executePromise
+
+		// Both attempts were made
+		expect(provider.reopenParentFromDelegation).toHaveBeenCalledTimes(2)
+
+		// Parent was repaired: updateTaskHistory called with status: "active"
+		expect(provider.updateTaskHistory).toHaveBeenCalledWith(
+			expect.objectContaining({
+				id: "parent-1",
+				status: "active",
+				awaitingChildId: undefined,
+				childIds: ["child-1"],
+			}),
+		)
+
+		// persistDelegationMeta called with status: "active" and null awaitingChildId
+		expect(provider.persistDelegationMeta).toHaveBeenCalledWith("parent-1", {
+			status: "active",
+			awaitingChildId: null,
+			delegatedToId: "mode-1",
+			childIds: ["child-1"],
+		})
+
+		// Error tool result was pushed
+		expect(callbacks.pushToolResult).toHaveBeenCalledWith(
+			expect.stringContaining("Delegation to parent task failed"),
+		)
+	})
+
+	it("should use read-merge-write disk fallback when parent not in globalState", async () => {
+		const provider = createMockProvider()
+		const task = createMockTask(provider)
+		const callbacks = createCallbacks()
+
+		// Both delegation attempts fail
+		provider.reopenParentFromDelegation
+			.mockRejectedValueOnce(new Error("first failure"))
+			.mockRejectedValueOnce(new Error("second failure"))
+
+		// Child status lookup succeeds, parent lookup fails (not in globalState)
+		provider.getTaskWithId
+			.mockResolvedValueOnce({ historyItem: childHistoryItem() })
+			.mockRejectedValueOnce(new Error("Task not found"))
+
+		// Existing delegation meta on disk
+		const existingMeta: DelegationMeta = {
+			status: "delegated",
+			delegatedToId: "mode-1",
+			awaitingChildId: "child-1",
+			childIds: ["child-1"],
+			completedByChildId: "prev-child",
+			completionResultSummary: "previous result",
+		}
+		provider.readDelegationMeta.mockResolvedValue(existingMeta)
+
+		const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any)
+
+		await vi.advanceTimersByTimeAsync(600)
+		await executePromise
+
+		// readDelegationMeta was called for the parent
+		expect(provider.readDelegationMeta).toHaveBeenCalledWith("parent-1")
+
+		// persistDelegationMeta preserves existing fields via read-merge-write
+		expect(provider.persistDelegationMeta).toHaveBeenCalledWith("parent-1", {
+			status: "active",
+			awaitingChildId: null,
+			delegatedToId: "mode-1",
+			childIds: ["child-1"],
+			completedByChildId: "prev-child",
+			completionResultSummary: "previous result",
+		})
+	})
+
+	it("should use defaults when disk fallback has no existing meta", async () => {
+		const provider = createMockProvider()
+		const task = createMockTask(provider)
+		const callbacks = createCallbacks()
+
+		// Both delegation attempts fail
+		provider.reopenParentFromDelegation
+			.mockRejectedValueOnce(new Error("first failure"))
+			.mockRejectedValueOnce(new Error("second failure"))
+
+		// Child status lookup succeeds, parent lookup fails
+		provider.getTaskWithId
+			.mockResolvedValueOnce({ historyItem: childHistoryItem() })
+			.mockRejectedValueOnce(new Error("Task not found"))
+
+		// No existing delegation meta on disk
+		provider.readDelegationMeta.mockResolvedValue(null)
+
+		const executePromise = tool.execute({ result: "completed work" }, task as any, callbacks as any)
+
+		await vi.advanceTimersByTimeAsync(600)
+		await executePromise
+
+		// persistDelegationMeta uses null defaults when no existing meta
+		expect(provider.persistDelegationMeta).toHaveBeenCalledWith("parent-1", {
+			status: "active",
+			awaitingChildId: null,
+			delegatedToId: null,
+			childIds: ["child-1"],
+			completedByChildId: null,
+			completionResultSummary: null,
+		})
+	})
+
+	it("should not attempt delegation when approval is denied", async () => {
+		const provider = createMockProvider()
+		const task = createMockTask(provider)
+		const callbacks = createCallbacks({
+			askFinishSubTaskApproval: vi.fn().mockResolvedValue(false),
+		})
+
+		// Child task status is "active"
+		provider.getTaskWithId.mockResolvedValue({
+			historyItem: childHistoryItem(),
+		})
+
+		await tool.execute({ result: "completed work" }, task as any, callbacks as any)
+
+		// No delegation attempted
+		expect(provider.reopenParentFromDelegation).not.toHaveBeenCalled()
+	})
+})
diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts
@@ -485,6 +485,7 @@ describe("attemptCompletionTool", () => {
 			reopenParentFromDelegation: ReturnType<typeof vi.fn>
 			updateTaskHistory: ReturnType<typeof vi.fn>
 			persistDelegationMeta: ReturnType<typeof vi.fn>
+			readDelegationMeta: ReturnType<typeof vi.fn>
 		}
 
 		let block: AttemptCompletionToolUse
@@ -498,6 +499,7 @@ describe("attemptCompletionTool", () => {
 				reopenParentFromDelegation: vi.fn(),
 				updateTaskHistory: vi.fn(),
 				persistDelegationMeta: vi.fn(),
+				readDelegationMeta: vi.fn().mockResolvedValue(null),
 			}
 
 			Object.defineProperty(mockTask, "parentTaskId", { value: "parent-123", writable: true, configurable: true })
@@ -611,6 +613,8 @@ describe("attemptCompletionTool", () => {
 				awaitingChildId: null,
 				delegatedToId: null,
 				childIds: ["task_1"],
+				completedByChildId: null,
+				completionResultSummary: null,
 			})
 			expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegation to parent task failed"))
 		})
diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts
