fix: address CodeRabbit review - guard active-only in markDelegatedChildInterrupted, evict on disposal, fix delegateParentAndOpenChild TOCTOU, translate i18n keys

Author: edelauna

apps/vscode-e2e/src/fixtures/subtasks.ts

@@ -470,6 +470,7 @@ export function addSubtaskFixtures(mock: InstanceType<typeof LLMock>) {
 		match: {
 			predicate: (req: ChatCompletionRequest) =>
 				requestContains(req, [SUBTASK_ABANDON_CHILD_MARKER]) &&
+				!requestContains(req, [SUBTASK_ABANDON_PARENT_MARKER]) &&
 				!requestContains(req, ["call_abandon_child_followup_001"]) &&
 				!requestContains(req, [`<user_message>\\n${SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER}\\n</user_message>`]),
 		},

src/__tests__/removeClineFromStack-delegation.spec.ts

@@ -537,15 +537,18 @@ describe("ClineProvider.evictCurrentTask() — active delegated child path", ()
 		expect(markDelegatedChildInterrupted).not.toHaveBeenCalled()
 	})
 
-	it("swallows markDelegatedChildInterrupted errors and logs them", async () => {
+	it("propagates markDelegatedChildInterrupted errors (method swallows internally, not caller)", async () => {
+		// evictCurrentTask no longer has a caller-level .catch(); errors propagate
+		// from markDelegatedChildInterrupted directly. The real implementation swallows
+		// inside its own try/catch (after the guard reads); a mock that rejects bypasses
+		// that catch and exercises the propagation path.
 		const childTask = {
 			taskId: "child-err",
 			instanceId: "inst-1",
 			emit: vi.fn(),
 			abortTask: vi.fn().mockResolvedValue(undefined),
 		}
 
-		const log = vi.fn()
 		const markDelegatedChildInterrupted = vi.fn().mockRejectedValue(new Error("lock contention"))
 
 		const provider = makeProviderStub({
@@ -556,12 +559,12 @@ describe("ClineProvider.evictCurrentTask() — active delegated child path", ()
 				get: vi.fn(() => ({ id: "child-err", status: "active", parentTaskId: "parent-1" })),
 			},
 			markDelegatedChildInterrupted,
-			log,
+			log: vi.fn(),
 		})
 
-		await expect((ClineProvider.prototype as any).evictCurrentTask.call(provider)).resolves.not.toThrow()
-
-		expect(log).toHaveBeenCalledWith(expect.stringContaining("markDelegatedChildInterrupted failed"))
+		await expect((ClineProvider.prototype as any).evictCurrentTask.call(provider)).rejects.toThrow(
+			"lock contention",
+		)
 	})
 })
 

src/core/webview/ClineProvider.ts

@@ -546,11 +546,7 @@ export class ClineProvider
 			await this.markDelegatedChildInterrupted({
 				childTaskId: storedHistory.id,
 				parentTaskId: storedHistory.parentTaskId,
-			}).catch((err) =>
-				this.log(
-					`[evictCurrentTask] markDelegatedChildInterrupted failed (non-fatal): ${err instanceof Error ? err.message : String(err)}`,
-				),
-			)
+			})
 		}
 	}
 
@@ -597,10 +593,12 @@ export class ClineProvider
 				const childHistory =
 					this.taskHistoryStore.get(childTaskId) ?? (await this.getTaskWithId(childTaskId)).historyItem
 
-				// Re-check inside the lock to close the TOCTOU window with cancelTask().
-				if (childHistory?.status === "interrupted") {
+				// Re-check inside the lock to close the TOCTOU window with cancelTask() or
+				// a concurrent completion. Only proceed when the child is still "active";
+				// any other terminal status (interrupted, completed) must not be overwritten.
+				if (childHistory?.status !== "active") {
 					this.log(
-						`[markDelegatedChildInterrupted] Child ${childTaskId} already interrupted (in-lock check) — skipping`,
+						`[markDelegatedChildInterrupted] Child ${childTaskId} is no longer active (status=${childHistory?.status}) — skipping`,
 					)
 					return
 				}
@@ -680,7 +678,12 @@ export class ClineProvider
 		this._disposed = true
 		this.log("Disposing ClineProvider...")
 
-		// Clear all tasks from the stack.
+		// Clear all tasks from the stack. The first pop goes through evictCurrentTask()
+		// so an active delegated child is marked interrupted before the extension shuts down,
+		// rather than being left persisted as "active" across the reload.
+		if (this.clineStack.length > 0) {
+			await this.evictCurrentTask()
+		}
 		while (this.clineStack.length > 0) {
 			await this.removeClineFromStack()
 		}
@@ -3596,23 +3599,24 @@ export class ClineProvider
 		//    If the parent is already "delegated" to a previous interrupted child (the user
 		//    navigated back to the parent and continued working), we implicitly sever the old
 		//    link here (delegated → active → delegated) so no explicit Abandon step is needed.
-		//    We snapshot the old awaited child's status BEFORE entering the updater (which is
-		//    synchronous) so the guard can verify the child is actually interrupted before
-		//    severing. An active child must never be silently detached.
-		const existingParent = this.taskHistoryStore.get(parentTaskId)
-		const existingAwaitedChildStatus = existingParent?.awaitingChildId
-			? this.taskHistoryStore.get(existingParent.awaitingChildId)?.status
-			: undefined
+		//    The old awaited child's status is re-read INSIDE the updater (which runs
+		//    synchronously under the store lock) so a concurrent abandon or completion cannot
+		//    slip between the status snapshot and the write. An active child must never be
+		//    silently detached.
 		try {
 			await this.taskHistoryStore.atomicReadAndUpdate(parentTaskId, (historyItem) => {
 				let base = historyItem
 				if (historyItem.status === "delegated") {
+					// Re-read the awaited child's current status under the store lock.
+					const awaitedChildStatus = historyItem.awaitingChildId
+						? this.taskHistoryStore.get(historyItem.awaitingChildId)?.status
+						: undefined
 					// Only sever the stale link when the old child is confirmed interrupted.
 					// If it is still active, throw so the rollback path cleans up the new child
 					// rather than silently detaching a live task.
-					if (existingAwaitedChildStatus !== "interrupted") {
+					if (awaitedChildStatus !== "interrupted") {
 						throw new Error(
-							`[delegateParentAndOpenChild] Cannot re-delegate: existing child ${historyItem.awaitingChildId} is ${existingAwaitedChildStatus}, not interrupted`,
+							`[delegateParentAndOpenChild] Cannot re-delegate: existing child ${historyItem.awaitingChildId} is ${awaitedChildStatus}, not interrupted`,
 						)
 					}
 					// Implicit sever of the stale interrupted-child link.

src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts

@@ -791,10 +791,9 @@ describe("ClineProvider Task History Synchronization", () => {
 					listeners[event] = listeners[event] ?? []
 					listeners[event].push(fn)
 				},
-				emit: (event: string, ...args: unknown[]) => {
-					for (const fn of listeners[event] ?? []) {
-						fn(...args)
-					}
+				// Returns a promise that resolves when all async listeners have settled.
+				emit: async (event: string, ...args: unknown[]) => {
+					await Promise.all((listeners[event] ?? []).map((fn) => Promise.resolve(fn(...args))))
 				},
 			}
 		}
@@ -806,8 +805,7 @@ describe("ClineProvider Task History Synchronization", () => {
 			const fakeTask = makeFakeTask("task-cb-1")
 			;(provider as any).taskCreationCallback(fakeTask)
 
-			fakeTask.emit(RooCodeEventName.TaskCompleted, "task-cb-1", {}, {})
-			await new Promise((r) => setTimeout(r, 10))
+			await fakeTask.emit(RooCodeEventName.TaskCompleted, "task-cb-1", {}, {})
 
 			const stored = provider.taskHistoryStore.get("task-cb-1")
 			expect(stored?.status).toBe("completed")
@@ -822,8 +820,7 @@ describe("ClineProvider Task History Synchronization", () => {
 			const fakeTask = makeFakeTask("task-cb-2")
 			;(provider as any).taskCreationCallback(fakeTask)
 
-			fakeTask.emit(RooCodeEventName.TaskCompleted, "task-cb-2", {}, {})
-			await new Promise((r) => setTimeout(r, 10))
+			await fakeTask.emit(RooCodeEventName.TaskCompleted, "task-cb-2", {}, {})
 
 			// updateTaskHistory is called initially to store the item, but should NOT be
 			// called again by onTaskCompleted since it's already completed.
@@ -845,8 +842,7 @@ describe("ClineProvider Task History Synchronization", () => {
 			const fakeTask = makeFakeTask("task-cb-3")
 			;(provider as any).taskCreationCallback(fakeTask)
 
-			fakeTask.emit(RooCodeEventName.TaskCompleted, "task-cb-3", {}, {})
-			await new Promise((r) => setTimeout(r, 10))
+			await fakeTask.emit(RooCodeEventName.TaskCompleted, "task-cb-3", {}, {})
 
 			expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[onTaskCompleted] Failed to write"))
 		})