From af97e6be030463c1c1a9d6229dba4f7542074500 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 16 Aug 2026 21:40:27 +0800 Subject: [PATCH] Fix delegation link loss across interrupt/resume; type NewTaskTool provider call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem A/C — delegation link lost across interrupt/resume When a delegated child is interrupted (cancelTask / evictCurrentTask), the parent's `awaitingChildId` link is preserved only while the parent is still `delegated`. After a crash or resume cycle the parent can be left `active` with no `awaitingChildId`, so when the user resumes the child, AttemptCompletionTool refuses to route its completion back (it requires `parent.awaitingChildId === this child`). The result: a resumed subtask's result is silently stranded and never reported to the parent. Fix: re-establish the link in `createTaskWithHistoryItem` (the common funnel for every resume path) when resuming an *interrupted* child. New private helper `reestablishDelegationLinkOnResume` transitions a demoted `active` parent back to `delegated`, gated so it: - skips children whose delegation was intentionally severed (`cancelledDelegationChildIds`), - never clobbers a live delegation to a different child, - only performs the legal `active -> delegated` transition. Non-fatal: any failure is logged and the resume proceeds without the link. Because AttemptCompletionTool already accepts an `active` parent whose `awaitingChildId` matches, routing (Problem C) works automatically once the link is restored. ## Problem D — remove `as any` in NewTaskTool `task.providerRef` is a `WeakRef`, so after the null-check `provider` is already typed `ClineProvider`. The `(provider as any).delegateParentAndOpenChild(...)` cast was unnecessary; it is now a typed method call. This removes the last `as any` in NewTaskTool.ts, so its stale `no-explicit-any` suppression entry is dropped from eslint-suppressions.json (count never increases). ## Tests Added 4 focused regression tests for `reestablishDelegationLinkOnResume` covering: link restored on resume of an interrupted child with a demoted parent; no-op when already delegated to the same child; never clobbers a live delegation to another child; and no reattach for intentionally-severed children. --- src/core/tools/NewTaskTool.ts | 6 +- src/core/webview/ClineProvider.ts | 72 +++++++++ .../ClineProvider.flicker-free-cancel.spec.ts | 152 ++++++++++++++++++ src/eslint-suppressions.json | 5 - 4 files changed, 228 insertions(+), 7 deletions(-) diff --git a/src/core/tools/NewTaskTool.ts b/src/core/tools/NewTaskTool.ts index 68240bb21d..369c83de8d 100644 --- a/src/core/tools/NewTaskTool.ts +++ b/src/core/tools/NewTaskTool.ts @@ -157,8 +157,10 @@ export class NewTaskTool extends BaseTool<"new_task"> { return } - // Delegate parent and open child as sole active task - const child = await (provider as any).delegateParentAndOpenChild({ + // Delegate parent and open child as sole active task. + // `provider` is already narrowed to ClineProvider above (WeakRef deref + null check), + // so this is a typed method call — no cast needed. + const child = await provider.delegateParentAndOpenChild({ parentTaskId: task.taskId, message: unescapedMessage, initialTodos: todoItems, diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index bb74660d82..be6d4e45d5 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -691,6 +691,68 @@ export class ClineProvider } } + /** + * Re-establish a parent→child delegation link when an interrupted child is resumed. + * + * The interrupt path (cancelTask / evictCurrentTask) preserves the link only while the + * parent is still "delegated". After a crash or resume cycle the parent can be left + * "active" with no awaitingChildId, so AttemptCompletionTool refuses to route the child's + * completion back (it requires parent.awaitingChildId === this child). Restoring the link + * here — the common funnel for every resume path — lets a resumed child report back. + * + * Safe by construction: + * - only called for children whose delegation was NOT intentionally severed + * (cancelledDelegationChildIds), + * - never clobbers a live delegation to a different child, + * - and only transitions an "active" parent to "delegated" (the sole legal path). + * Non-fatal: any failure is logged and the resume proceeds without the link. + */ + private async reestablishDelegationLinkOnResume(childTaskId: string, parentTaskId: string): Promise { + // A child whose delegation was intentionally severed (abandonSubtask) or that failed a + // cancel must NOT be reattached — its parentTaskId may still point at the old parent. + if (this.cancelledDelegationChildIds.has(childTaskId)) { + return + } + + try { + await this.taskHistoryStore.atomicReadAndUpdate(parentTaskId, (parent) => { + // Already linked to this child — nothing to do. + if (parent.status === "delegated" && parent.awaitingChildId === childTaskId) { + return parent + } + + // Never clobber a live delegation to a different child. + if (parent.awaitingChildId && parent.awaitingChildId !== childTaskId) { + const otherStatus = this.taskHistoryStore.get(parent.awaitingChildId)?.status + if (otherStatus === "active" || otherStatus === "delegated") { + return parent + } + } + + // Only an "active" parent can legally become "delegated"; any other status is left untouched. + if (parent.status !== "active") { + return parent + } + + const childIds = Array.from(new Set([...(parent.childIds ?? []), childTaskId])) + this.log( + `[reestablishDelegationLinkOnResume] Restored link: parent ${parentTaskId} → child ${childTaskId}`, + ) + return { + ...parent, + status: "delegated" as const, + awaitingChildId: childTaskId, + delegatedToId: childTaskId, + childIds, + } + }) + } catch (err) { + this.log( + `[reestablishDelegationLinkOnResume] Failed to restore link for parent ${parentTaskId} → child ${childTaskId}: ${err instanceof Error ? err.message : String(err)}`, + ) + } + } + /** * Cancel cascade: interrupt every LIVE child of the given parent task. * @@ -1186,6 +1248,16 @@ export class ClineProvider await this.evictCurrentTask() } + // Re-establish the parent→child delegation link when resuming an interrupted child. + // The interrupt path preserves the link only while the parent is still "delegated"; + // after a crash or resume cycle the parent can be left "active" with no awaitingChildId, + // which would prevent the resumed child's completion from routing back (AttemptCompletionTool + // requires parent.awaitingChildId === this child). Restoring it here — before the child Task + // instance starts writing its own history — lets a resumed child report back to its parent. + if (historyItem.parentTaskId && historyItem.status === "interrupted") { + await this.reestablishDelegationLinkOnResume(historyItem.id, historyItem.parentTaskId) + } + // If the history item has a saved mode, restore it and its associated API configuration. if (historyItem.mode) { // Validate that the mode still exists diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 3513bd3bd5..27f9e6e629 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -872,6 +872,158 @@ describe("ClineProvider flicker-free cancel", () => { expect(provider.getTaskWithId).not.toHaveBeenCalled() }) + describe("reestablishDelegationLinkOnResume (Problem A: link lost across interrupt/resume)", () => { + // The outer afterEach calls provider.dispose(), which needs the REAL store. We swap in a + // minimal fake per test and restore the original here so dispose() sees it. + let originalStore: unknown + beforeEach(() => { + originalStore = provider["taskHistoryStore"] + }) + afterEach(() => { + setStore(originalStore) + }) + + // taskHistoryStore is `public readonly`, so a direct assignment fails type-checking. + // Object.assign sidesteps the compile-time readonly check (it's an own data property at + // runtime) so we can swap in a minimal fake; the real store is restored before dispose(). + function setStore(store: unknown): void { + Object.assign(provider, { taskHistoryStore: store }) + } + + // Minimal Map-backed store mirroring the real TaskHistoryStore's atomicReadAndUpdate/get + // semantics closely enough to exercise the provider's restore logic in isolation. + function makeFakeStore(items: Record) { + const cache = new Map(Object.entries(items)) + return { + get: (id: string) => cache.get(id), + // provider.dispose() calls this; no-op for the fake. + dispose: () => {}, + atomicReadAndUpdate: async (taskId: string, updater: (current: HistoryItem) => HistoryItem) => { + const current = cache.get(taskId) + if (!current) throw new Error(`not found in cache: ${taskId}`) + const snapshot = structuredClone(current) + const updated = updater(snapshot) + if (updated.id !== taskId) throw new Error("updater changed task id") + cache.set(taskId, updated) + return [updated] + }, + } + } + + it("restores the parent link when an interrupted child is resumed and the parent was demoted to active", async () => { + // The exact Problem A state: parent left "active" with no awaitingChildId after a + // crash/resume cycle, while the child (interrupted) still points at it. + const store = makeFakeStore({ + ["parent-1"]: { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/test/workspace", + status: "active", + } as HistoryItem, + }) + setStore(store) + + await provider["reestablishDelegationLinkOnResume"]("child-1", "parent-1") + + const parent = store.get("parent-1")! + expect(parent.status).toBe("delegated") + expect(parent.awaitingChildId).toBe("child-1") + expect(parent.delegatedToId).toBe("child-1") + expect(parent.childIds).toContain("child-1") + }) + + it("is a no-op when the parent is already delegated to this child", async () => { + const store = makeFakeStore({ + ["parent-1"]: { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "child-1", + delegatedToId: "child-1", + } as HistoryItem, + }) + setStore(store) + + await provider["reestablishDelegationLinkOnResume"]("child-1", "parent-1") + + const parent = store.get("parent-1")! + expect(parent.status).toBe("delegated") + expect(parent.awaitingChildId).toBe("child-1") + }) + + it("never clobbers a live delegation to a different child", async () => { + const store = makeFakeStore({ + ["parent-1"]: { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/test/workspace", + status: "delegated", + awaitingChildId: "other-child", + delegatedToId: "other-child", + } as HistoryItem, + ["other-child"]: { + id: "other-child", + number: 3, + task: "other child", + ts: Date.now(), + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/test/workspace", + status: "active", + } as HistoryItem, + }) + setStore(store) + + await provider["reestablishDelegationLinkOnResume"]("child-1", "parent-1") + + const parent = store.get("parent-1")! + // Still awaiting the live other-child — child-1 must not have displaced it. + expect(parent.awaitingChildId).toBe("other-child") + }) + + it("does not reattach a child whose delegation was intentionally severed", async () => { + const store = makeFakeStore({ + ["parent-1"]: { + id: "parent-1", + number: 1, + task: "parent task", + ts: Date.now(), + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/test/workspace", + status: "active", + } as HistoryItem, + }) + setStore(store) + // abandonSubtask / a failed cancel adds the child to this fail-closed set. + provider["cancelledDelegationChildIds"].add("child-1") + + await provider["reestablishDelegationLinkOnResume"]("child-1", "parent-1") + + const parent = store.get("parent-1")! + expect(parent.status).toBe("active") + expect(parent.awaitingChildId).toBeUndefined() + }) + }) + afterAll(() => { vi.restoreAllMocks() }) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 569c846c29..4af5d9c17f 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -904,11 +904,6 @@ "count": 1 } }, - "core/tools/NewTaskTool.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 1 - } - }, "core/tools/ReadFileTool.ts": { "@typescript-eslint/no-explicit-any": { "count": 4