From 72e1216b696a0c43b93935d698c928da6e78569b Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 20 Sep 2026 14:00:20 +0000 Subject: [PATCH 1/5] fix: stop interrupted subtask delegation replay loops Allow approved delegation from interrupted tasks while preserving only current parent ownership. Persist a failed tool result before rollback restores a pending delegation, and stop restoration if recovery persistence fails. Add lifecycle model, provider rollback, history resume, and extension-host regression coverage for #1714. Amp-Thread-ID: https://ampcode.com/threads/T-01a0bf0f-6f0a-724b-8d1f-69c1f1bfe3fc --- apps/vscode-e2e/src/fixtures/subtasks.ts | 27 ++ apps/vscode-e2e/src/suite/subtasks.test.ts | 59 ++++ docs/architecture/task-lifecycle-model.md | 2 + scripts/check-task-lifecycle.ts | 26 +- .../ClineProvider.delegation.spec.ts | 285 ++++++++++++------ .../__tests__/taskLifecycle.spec.ts | 32 ++ src/core/task-persistence/taskLifecycle.ts | 13 +- .../task/__tests__/Task.persistence.spec.ts | 78 +++-- src/core/webview/ClineProvider.ts | 48 ++- src/eslint-suppressions.json | 2 +- 10 files changed, 443 insertions(+), 129 deletions(-) diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index ebfd94324e..1595bdf207 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -13,6 +13,9 @@ const SUBTASK_FAST_PARENT_MARKER = "SUBTASK_PARENT_IMMEDIATE_COMPLETION" const SUBTASK_FAST_CHILD_MARKER = "SUBTASK_CHILD_IMMEDIATE_COMPLETION" const SUBTASK_APPROVAL_RESTORE_PARENT_MARKER = "SUBTASK_PARENT_APPROVAL_RESTORE" const SUBTASK_APPROVAL_RESTORE_CHILD_MARKER = "SUBTASK_CHILD_APPROVAL_RESTORE" +export const SUBTASK_PENDING_REPLAY_ROOT = "SUBTASK_PENDING_REPLAY_ROOT: Create a child task." +const SUBTASK_PENDING_REPLAY_CHILD = "SUBTASK_PENDING_REPLAY_CHILD: Create a grandchild task." +const SUBTASK_PENDING_REPLAY_LEAF = "SUBTASK_PENDING_REPLAY_LEAF: Wait for user instructions." const SUBTASK_XPROFILE_PARENT_MARKER = "SUBTASK_PARENT_CROSS_PROFILE" const SUBTASK_XPROFILE_SAME_CHILD_MARKER = "SUBTASK_CHILD_SAME_PROFILE" const SUBTASK_XPROFILE_DIFFERENT_CHILD_MARKER = "SUBTASK_CHILD_DIFFERENT_PROFILE" @@ -127,6 +130,30 @@ const completionAfterAnswer = (followupId: string, completionId: string) => ({ }) export function addSubtaskFixtures(mock: InstanceType) { + for (const [prompt, childPrompt, id] of [ + [SUBTASK_PENDING_REPLAY_ROOT, SUBTASK_PENDING_REPLAY_CHILD, "call_pending_replay_root"], + [SUBTASK_PENDING_REPLAY_CHILD, SUBTASK_PENDING_REPLAY_LEAF, "call_pending_replay_child"], + ]) { + mock.addFixture({ + match: { userMessage: prompt, sequenceIndex: 0 }, + response: { + toolCalls: [{ name: "new_task", arguments: JSON.stringify({ mode: "ask", message: childPrompt }), id }], + }, + }) + } + mock.addFixture({ + match: { userMessage: SUBTASK_PENDING_REPLAY_LEAF }, + response: { + toolCalls: [ + { + name: "ask_followup_question", + arguments: JSON.stringify({ question: "What should I do next?", follow_up: [] }), + id: "call_pending_replay_wait", + }, + ], + }, + }) + mock.addFixture({ match: { userMessage: new RegExp(SUBTASK_APPROVAL_RESTORE_PARENT_MARKER), diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 857c8accc5..1a316217e6 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -27,6 +27,7 @@ import { SUBTASK_INTERRUPT_PARENT_PROMPT, SUBTASK_INTERRUPT_PARENT_RESULT, SUBTASK_PARENT_PROMPT, + SUBTASK_PENDING_REPLAY_ROOT, SUBTASK_XPROFILE_DIFFERENT_CHILD_RESULT, SUBTASK_XPROFILE_PARENT_PROMPT, SUBTASK_XPROFILE_PARENT_RESULT, @@ -260,6 +261,64 @@ suite("Roo Code Subtasks", function () { } }) + test("interrupted child replays pending new_task once with subtask auto-approval", async () => { + const api = globalThis.api + const asks: Record = {} + const delegations: Array<[string, string]> = [] + const onMessage = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (isCompletedAsk(message)) (asks[taskId] ??= []).push(message) + } + const onDelegated = (parentId: string, childId: string) => { + delegations.push([parentId, childId]) + } + const hasNewTaskAsk = (taskId: string) => + asks[taskId]?.some( + (message) => message.ask === "tool" && JSON.parse(message.text ?? "{}").tool === "newTask", + ) ?? false + api.on(RooCodeEventName.Message, onMessage) + api.on(RooCodeEventName.TaskDelegated, onDelegated) + try { + const rootId = await api.startNewTask({ + configuration: { + mode: "ask", + autoApprovalEnabled: true, + alwaysAllowSubtasks: false, + enableCheckpoints: false, + }, + text: SUBTASK_PENDING_REPLAY_ROOT, + }) + await waitFor(() => hasNewTaskAsk(rootId)) + await api.approveCurrentAsk() + await waitFor(() => delegations.length === 1) + assert.ok(delegations[0]) + const childId = delegations[0][1] + await waitFor(() => hasNewTaskAsk(childId)) + await api.clearCurrentTask() + const interrupted = await api.getTaskHistoryItem(childId) + assert.strictEqual(interrupted?.status, "interrupted") + assert.strictEqual(interrupted?.pendingAction?.kind, "create_subtask") + + await api.setConfiguration({ autoApprovalEnabled: true, alwaysAllowSubtasks: true }) + await api.resumeTask(childId) + await waitFor(() => delegations.length === 2) + assert.ok(delegations[1]) + const [delegatingId, grandchildId] = delegations[1] + assert.strictEqual(delegatingId, childId) + await waitFor(() => asks[grandchildId]?.some(({ ask }) => ask === "followup") ?? false) + const resumed = await api.getTaskHistoryItem(childId) + assert.strictEqual(resumed?.status, "delegated") + assert.strictEqual(resumed?.pendingAction, undefined) + assert.strictEqual(resumed?.parentTaskId, rootId) + assert.deepStrictEqual(resumed?.childIds, [grandchildId]) + assert.strictEqual(api.getCurrentTaskStack().at(-1), grandchildId) + assert.strictEqual(delegations.length, 2) + } finally { + api.off(RooCodeEventName.Message, onMessage) + api.off(RooCodeEventName.TaskDelegated, onDelegated) + while (api.getCurrentTaskStack().length > 0) await api.clearCurrentTask() + } + }) + // Smoke: child completing normally must resume the parent task. test("child task returns to parent after normal completion", async () => { const api = globalThis.api diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 355b08aa7e..eae57c16ad 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -55,6 +55,8 @@ The model has three fixed task slots, enough to cover competing siblings and a n Production completion also accepts a recovery-compatible `active` parent that still awaits the returning child, then clears the stale pointers. Normal model transitions never create that intermediate state, so it is covered by a focused reducer test rather than admitted as a generally valid reachable state. +An approved delegation can also resume an `interrupted` task directly into `delegated`. The `resume-delegate` action and detached-task-delegation landmark cover this path without allowing arbitrary message saves to reactivate interrupted tasks. The reducer retains the task's own parent link only when that parent still awaits it; otherwise it clears stale lineage instead of taking ownership back from a newer sibling. Provider rollback persists an error tool result before rehydrating a failed pending delegation, so history resume reconciles the action rather than auto-approving it again. Failed result persistence stops restoration. Provider and history-resume tests cover this persistence boundary; the subtask extension-host smoke test covers interrupted pending approval replay with auto-approval enabled (#1714). + ## Shared-store concurrency model The same `pnpm lifecycle:model-check` command also runs a second bounded explorer over two `TaskHistoryStore` hosts. It imports the production `computeHistoryDelta` and `mergeHistoryDelta` functions, so its semantics match the store rather than assuming coherent caches or transactional pair writes: diff --git a/scripts/check-task-lifecycle.ts b/scripts/check-task-lifecycle.ts index 73e9078366..005a3f6271 100644 --- a/scripts/check-task-lifecycle.ts +++ b/scripts/check-task-lifecycle.ts @@ -25,8 +25,12 @@ interface TraceStep { const MAX_DEPTH = 12 const MAX_STATES = 10_000 -const expectedActions = ["delegate", "interrupt", "complete", "abandon"] as const +const expectedActions = ["delegate", "resume-delegate", "interrupt", "complete", "abandon"] as const const semanticLandmarks = { + "detached-task-delegation": (state: ModelState) => + state["child-a"]?.status === "delegated" && + state["child-a"].parentTaskId === undefined && + state["child-a"].awaitingChildId === "child-b", "interrupted-child-redelegation": (state: ModelState) => state.parent?.status === "delegated" && state.parent.awaitingChildId === "child-b" && @@ -73,12 +77,17 @@ function transitions(state: ModelState): Transition[] { for (const childId of taskIds) { if (childId === parentId || state[childId]) continue const awaitedStatus = parent.awaitingChildId ? state[parent.awaitingChildId as TaskId]?.status : undefined - if (parent.status !== "active" && !(parent.status === "delegated" && awaitedStatus === "interrupted")) { + if ( + parent.status !== "active" && + parent.status !== "interrupted" && + !(parent.status === "delegated" && awaitedStatus === "interrupted") + ) { continue } - const delegated = delegateTaskToChild(parent, childId, awaitedStatus) + const owningParent = parent.parentTaskId ? state[parent.parentTaskId as TaskId] : undefined + const delegated = delegateTaskToChild(parent, childId, awaitedStatus, owningParent) result.push({ - name: `delegate(${parentId}, ${childId})`, + name: `${parent.status === "interrupted" ? "resume-delegate" : "delegate"}(${parentId}, ${childId})`, next: replace(state, delegated, task(childId, parentId)), }) } @@ -263,8 +272,17 @@ function runRepresentativeScenarios(): void { assert.throws(() => delegateTaskToChild(delegated, "child-b", "active"), /not interrupted/) const interruptedA = interruptDelegatedChild(delegated, childA) + const resumedA = delegateTaskToChild(interruptedA, "child-b", undefined, delegated) + assert.equal(resumedA.status, "delegated") + assert.equal(resumedA.parentTaskId, "parent") + const nestedReturn = completeDelegatedChild(resumedA, task("child-b", "child-a"), "nested result") + assert.equal(completeDelegatedChild(delegated, nestedReturn.parent, "resumed result").child.status, "completed") + const redelegated = delegateTaskToChild(delegated, "child-b", interruptedA.status) assert.throws(() => completeDelegatedChild(redelegated, interruptedA, "stale"), /not delegated to child/) + const detached = delegateTaskToChild(interruptedA, "new-grandchild", undefined, redelegated) + assert.equal(detached.parentTaskId, undefined) + assert.equal(detached.rootTaskId, undefined) const abandoned = abandonDelegatedChild(delegated, interruptedA) assert.throws(() => completeDelegatedChild(abandoned.parent, abandoned.child, "late"), /not delegated to child/) diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index 422c264e2c..3f0596c432 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -5,6 +5,13 @@ import type { HistoryItem } from "@roo-code/types" import { providerIdentifiers, RooCodeEventName } from "@roo-code/types" import { ClineProvider } from "../core/webview/ClineProvider" import { TaskScheduler } from "../core/task/TaskScheduler" +import { readApiMessages, saveApiMessages, type ApiMessage } from "../core/task-persistence/apiMessages" + +vi.mock("../core/task-persistence/apiMessages", async (importOriginal) => ({ + ...(await importOriginal()), + readApiMessages: vi.fn(), + saveApiMessages: vi.fn(), +})) const parentHistoryItem: HistoryItem = { id: "parent-1", @@ -90,49 +97,69 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(taskHistoryStore.atomicReadAndUpdate).not.toHaveBeenCalled() }) - it("clears a matching pending action when delegation commits", async () => { - const pendingAction = { - kind: "create_subtask" as const, - actionId: "create-action", - approvalText: "{}", - mode: "code", - message: "Do something", - todos: [], - } - let current: HistoryItem = { ...parentHistoryItem, status: "active", pendingAction } - const taskHistoryStore = { - invalidate: vi.fn().mockResolvedValue(undefined), - get: vi.fn(() => current), - atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { - current = updater(current) - return [current] - }), - } - const parentTask = makeParentTask() - const child = { taskId: "child-1", run: vi.fn().mockResolvedValue(undefined) } - const provider = { - taskScheduler: new TaskScheduler(), - emit: vi.fn(), - getCurrentTask: vi.fn(() => parentTask), - removeClineFromStack: vi.fn().mockResolvedValue(undefined), - createTask: vi.fn().mockResolvedValue(child), - handleModeSwitch: vi.fn().mockResolvedValue(undefined), - log: vi.fn(), - isViewLaunched: false, - taskHistoryStore, - } as unknown as ClineProvider - - await ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { - parentTaskId: "parent-1", - message: "Do something", - initialTodos: [], - mode: "code", - pendingActionId: "create-action", - }) + it.each(["active", "interrupted"] as const)( + "clears a matching pending action when %s delegation commits", + async (status) => { + const pendingAction = { + kind: "create_subtask" as const, + actionId: "create-action", + approvalText: "{}", + mode: "code", + message: "Do something", + todos: [], + } + let current: HistoryItem = { + ...parentHistoryItem, + status, + pendingAction, + parentTaskId: "root", + rootTaskId: "root", + } + const owner: HistoryItem = { + ...parentHistoryItem, + id: "root", + status: "delegated", + awaitingChildId: "parent-1", + } + const taskHistoryStore = { + invalidate: vi.fn().mockResolvedValue(undefined), + get: vi.fn((id: string) => (id === "root" ? owner : current)), + atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { + current = updater(current) + return [current] + }), + } + const parentTask = makeParentTask() + const child = { taskId: "child-1", run: vi.fn().mockResolvedValue(undefined) } + const provider = { + taskScheduler: new TaskScheduler(), + emit: vi.fn(), + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue(child), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + log: vi.fn(), + isViewLaunched: false, + taskHistoryStore, + } as unknown as ClineProvider - expect(current.pendingAction).toBeUndefined() - expect(current).toMatchObject({ status: "delegated", awaitingChildId: "child-1" }) - }) + await ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + pendingActionId: "create-action", + }) + + expect(current.pendingAction).toBeUndefined() + expect(current).toMatchObject({ status: "delegated", awaitingChildId: "child-1", parentTaskId: "root" }) + expect(provider.createTask).toHaveBeenCalledTimes(1) + await vi.waitFor(() => expect(child.run).toHaveBeenCalledTimes(1)) + if (status === "interrupted") { + expect(taskHistoryStore.invalidate).toHaveBeenCalledWith("root") + } + }, + ) it("rolls back when pending-action ownership changes before the atomic parent update", async () => { const pendingAction = { @@ -700,57 +727,129 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(durableParent.awaitingChildId).toBe("child-1") }) - it("rolls back the paused child and restores the parent when atomicReadAndUpdate fails", async () => { - const persistError = new Error("parent metadata persist failed") - const parentTask = makeParentTask() - const childRun = vi.fn().mockResolvedValue(undefined) - const removeClineFromStack = vi.fn().mockResolvedValue(undefined) - const deleteTaskWithId = vi.fn().mockResolvedValue(undefined) - const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) - const getTaskWithId = vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }) - - const taskHistoryStore = makeStoreStub({ - atomicReadAndUpdate: vi.fn().mockRejectedValue(persistError), - }) - - const child = { taskId: "child-1", start: vi.fn(), run: childRun } - // Before createTask: getCurrentTask returns parent (used by step 3 close). - // After createTask: returns child so the rollback guard passes and the child is popped. - const getCurrentTask = vi.fn().mockReturnValue(parentTask) - const createTask = vi.fn().mockImplementation(async () => { - getCurrentTask.mockReturnValue(child) - return child - }) - - const provider = { - taskScheduler: new TaskScheduler(), - emit: vi.fn(), - getCurrentTask, - removeClineFromStack, - createTask, - getTaskWithId, - handleModeSwitch: vi.fn().mockResolvedValue(undefined), - deleteTaskWithId, - createTaskWithHistoryItem, - log: vi.fn(), - isViewLaunched: false, - recentTasksCache: undefined, - taskHistoryStore, - } as unknown as ClineProvider + it.each(["legacy", "pending", "resolved", "read-failure", "write-failure"] as const)( + "rolls back without automatic action replay after a metadata failure (%s)", + async (scenario) => { + const persistError = new Error("parent metadata persist failed") + const parentTask = makeParentTask() + const childRun = vi.fn().mockResolvedValue(undefined) + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const deleteTaskWithId = vi.fn().mockResolvedValue(undefined) + const pendingAction: HistoryItem["pendingAction"] = + scenario === "legacy" + ? undefined + : { + kind: "create_subtask", + actionId: "failed-action", + approvalText: "{}", + mode: "code", + message: "Do something", + todos: [], + } + const historyItem: HistoryItem = { ...parentHistoryItem, status: "interrupted", pendingAction } + let durableMessages: ApiMessage[] = [ + { + role: "assistant", + content: [{ type: "tool_use", id: "failed-action", name: "new_task", input: {} }], + }, + ] + if (scenario === "resolved") { + durableMessages.push({ + role: "user", + content: [ + { + type: "tool_result", + tool_use_id: "failed-action", + content: "Already resolved", + is_error: true, + }, + ], + }) + } + vi.mocked(readApiMessages) + .mockReset() + .mockImplementation(async () => { + if (scenario === "read-failure") throw new Error("history read failed") + return durableMessages + }) + vi.mocked(saveApiMessages) + .mockReset() + .mockImplementation(async ({ messages }) => { + if (scenario === "write-failure") throw new Error("history write failed") + durableMessages = messages + return messages + }) + const createTaskWithHistoryItem = vi.fn(async () => { + if (pendingAction) { + // A rehydrated Task must find a durable result before it can replay the action. + expect(durableMessages.at(-1)).toMatchObject({ + role: "user", + content: [{ type: "tool_result", tool_use_id: pendingAction.actionId, is_error: true }], + }) + } + }) + const getTaskWithId = vi.fn().mockResolvedValue({ historyItem }) + + const taskHistoryStore = makeStoreStub({ + get: vi.fn().mockReturnValue(historyItem), + atomicReadAndUpdate: vi.fn().mockRejectedValue(persistError), + }) + + const child = { taskId: "child-1", start: vi.fn(), run: childRun } + // Before createTask: getCurrentTask returns parent (used by step 3 close). + // After createTask: returns child so the rollback guard passes and the child is popped. + const getCurrentTask = vi.fn().mockReturnValue(parentTask) + const createTask = vi.fn().mockImplementation(async () => { + getCurrentTask.mockReturnValue(child) + return child + }) - await expect( - (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { - parentTaskId: "parent-1", - message: "Do something", - initialTodos: [], - mode: "code", - }), - ).rejects.toThrow(persistError) + const provider = { + contextProxy: { globalStorageUri: { fsPath: "/tmp/delegation-rollback" } }, + taskScheduler: new TaskScheduler(), + emit: vi.fn(), + getCurrentTask, + removeClineFromStack, + createTask, + getTaskWithId, + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + deleteTaskWithId, + createTaskWithHistoryItem, + log: vi.fn(), + isViewLaunched: false, + recentTasksCache: undefined, + taskHistoryStore, + // This test substitutes only the provider boundaries used by delegation. + } as unknown as ClineProvider - expect(childRun).not.toHaveBeenCalled() - expect(removeClineFromStack).toHaveBeenNthCalledWith(1) - expect(removeClineFromStack).toHaveBeenNthCalledWith(2) - expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) - expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistoryItem) - }) + await expect( + ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + pendingActionId: pendingAction?.actionId, + }), + ).rejects.toThrow(persistError) + + expect(childRun).not.toHaveBeenCalled() + expect(removeClineFromStack).toHaveBeenNthCalledWith(1) + expect(removeClineFromStack).toHaveBeenNthCalledWith(2) + expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) + if (scenario === "read-failure" || scenario === "write-failure") { + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() + } else { + expect(createTaskWithHistoryItem).toHaveBeenCalledExactlyOnceWith(historyItem) + } + expect(saveApiMessages).toHaveBeenCalledTimes( + scenario === "pending" || scenario === "write-failure" ? 1 : 0, + ) + if (scenario === "pending") { + expect(durableMessages).toHaveLength(2) + expect(durableMessages[1]).toMatchObject({ + content: [{ content: "Subtask creation failed: parent metadata persist failed" }], + }) + } + }, + ) }) diff --git a/src/core/task-persistence/__tests__/taskLifecycle.spec.ts b/src/core/task-persistence/__tests__/taskLifecycle.spec.ts index fe415f09f8..4547ea54fc 100644 --- a/src/core/task-persistence/__tests__/taskLifecycle.spec.ts +++ b/src/core/task-persistence/__tests__/taskLifecycle.spec.ts @@ -33,6 +33,38 @@ describe("task lifecycle transitions", () => { }) }) + it.each(["owned", "released", "replaced", "missing"] as const)( + "resumes interrupted delegation with %s parent ownership", + (ownership) => { + const parent = item("parent", { + status: "interrupted", + parentTaskId: "root", + rootTaskId: "root", + childIds: ["older"], + }) + const owner = + ownership === "missing" + ? undefined + : item("root", { + status: ownership === "released" ? "active" : "delegated", + awaitingChildId: + ownership === "owned" ? "parent" : ownership === "replaced" ? "sibling" : undefined, + }) + const resumed = delegateTaskToChild(parent, "grandchild", undefined, owner) + + expect(resumed).toMatchObject({ + status: "delegated", + awaitingChildId: "grandchild", + delegatedToId: "grandchild", + childIds: ["older", "grandchild"], + parentTaskId: ownership === "owned" ? "root" : undefined, + rootTaskId: ownership === "owned" ? "root" : undefined, + }) + expect(parent.status).toBe("interrupted") + expect(parent.childIds).toEqual(["older"]) + }, + ) + it("treats a legacy unset status as active when delegating", () => { expect(delegateTaskToChild(item("parent", { status: undefined }), "child")).toMatchObject({ status: "delegated", diff --git a/src/core/task-persistence/taskLifecycle.ts b/src/core/task-persistence/taskLifecycle.ts index efd2e1148f..bb45561bab 100644 --- a/src/core/task-persistence/taskLifecycle.ts +++ b/src/core/task-persistence/taskLifecycle.ts @@ -6,7 +6,7 @@ export type HistoryItemStatus = NonNullable export const VALID_TASK_STATUS_TRANSITIONS: Readonly> = { active: ["delegated", "completed", "interrupted"], delegated: ["active"], - interrupted: ["completed"], + interrupted: ["delegated", "completed"], completed: [], } @@ -28,8 +28,19 @@ export function delegateTaskToChild( parent: HistoryItem, childId: string, awaitedChildStatus?: HistoryItemStatus, + owningParent?: HistoryItem, ): HistoryItem { let base = parent + // Approval to delegate resumes interrupted work. Keep its old lineage only + // while that parent still awaits it; startup repair or re-delegation may + // already have released ownership. Never steal another child's handoff. + if ( + parent.status === "interrupted" && + parent.parentTaskId && + (owningParent?.id !== parent.parentTaskId || owningParent.awaitingChildId !== parent.id) + ) { + base = { ...parent, parentTaskId: undefined, rootTaskId: undefined } + } if (parent.status === "delegated") { if (awaitedChildStatus !== "interrupted") { throw new LifecycleTransitionError( diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 8d3314a9a6..5e914e793f 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -1383,37 +1383,57 @@ describe("Task persistence", () => { expect(mockSaveTaskMessages).not.toHaveBeenCalled() }) - it("reconciles an already-persisted tool result before generic resume", async () => { - mockReadTaskMessages.mockResolvedValue([{ ts: 1, type: "say", say: "text", text: "Child" }]) - mockReadApiMessages.mockResolvedValue([ - { role: "user", content: [{ type: "tool_result", tool_use_id: "finish-action", content: "Denied" }] }, - ]) - mockProvider.clearPendingTaskAction = vi.fn().mockResolvedValue(true) - const task = new Task({ - provider: mockProvider, - apiConfiguration: mockApiConfig, - historyItem: { - id: "child-1", - number: 1, - ts: 1, - task: "Child", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - pendingAction, - }, - startTask: false, - }) - vi.spyOn(task, "ask").mockResolvedValue({ response: "noButtonClicked" }) - vi.spyOn(getTaskPersistenceAccess(task), "initiateTaskLoop").mockResolvedValue(undefined) - const replay = vi.spyOn(getTaskPersistenceAccess(task), "resumePendingTaskAction") + it.each(["finish_subtask", "create_subtask"] as const)( + "reconciles a durable %s error before generic resume", + async (kind) => { + const action: PendingTaskAction = + kind === "finish_subtask" + ? pendingAction + : { + kind, + actionId: "create-action", + approvalText: JSON.stringify({ tool: "newTask" }), + mode: "code", + message: "Delegate", + todos: [], + } + mockReadTaskMessages.mockResolvedValue([{ ts: 1, type: "say", say: "text", text: "Child" }]) + mockReadApiMessages.mockResolvedValue([ + { + role: "user", + content: [ + { type: "tool_result", tool_use_id: action.actionId, content: "Failed", is_error: true }, + ], + }, + ]) + mockProvider.clearPendingTaskAction = vi.fn().mockResolvedValue(true) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "child-1", + number: 1, + ts: 1, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + status: "interrupted", + pendingAction: action, + }, + startTask: false, + }) + vi.spyOn(task, "ask").mockResolvedValue({ response: "noButtonClicked" }) + vi.spyOn(getTaskPersistenceAccess(task), "initiateTaskLoop").mockResolvedValue(undefined) + const replay = vi.spyOn(getTaskPersistenceAccess(task), "resumePendingTaskAction") - await getTaskPersistenceAccess(task).resumeTaskFromHistory() + await getTaskPersistenceAccess(task).resumeTaskFromHistory() - expect(mockProvider.clearPendingTaskAction).toHaveBeenCalledWith("child-1", "finish-action") - expect(replay).not.toHaveBeenCalled() - expect(task.ask).toHaveBeenCalledWith("resume_task") - }) + expect(mockProvider.clearPendingTaskAction).toHaveBeenCalledWith("child-1", action.actionId) + expect(replay).not.toHaveBeenCalled() + expect(task.ask).toHaveBeenCalledWith("resume_task") + }, + ) it("clears pending metadata after the matching tool result is saved", async () => { mockProvider.clearPendingTaskAction = vi.fn().mockResolvedValue(true) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 86ce5d8e67..928b9e7ae0 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3881,6 +3881,9 @@ export class ClineProvider // waited on the shared lock. Refresh before mutating either task stack. await this.taskHistoryStore.invalidate(parentTaskId) const authoritativeParent = this.taskHistoryStore.get(parentTaskId) + if (authoritativeParent?.status === "interrupted" && authoritativeParent.parentTaskId) { + await this.taskHistoryStore.invalidate(authoritativeParent.parentTaskId) + } if (authoritativeParent?.status === "delegated") { const awaitedChildId = authoritativeParent.awaitingChildId if (!awaitedChildId) throw new Error("Cannot re-delegate a parent with no awaited child") @@ -4024,7 +4027,10 @@ export class ClineProvider const awaitedChildStatus = historyItem.awaitingChildId ? this.taskHistoryStore.get(historyItem.awaitingChildId)?.status : undefined - const delegated = delegateTaskToChild(historyItem, child.taskId, awaitedChildStatus) + const owningParent = historyItem.parentTaskId + ? this.taskHistoryStore.get(historyItem.parentTaskId) + : undefined + const delegated = delegateTaskToChild(historyItem, child.taskId, awaitedChildStatus, owningParent) return { ...delegated, pendingAction: @@ -4068,6 +4074,43 @@ export class ClineProvider } try { const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + if (pendingActionId && parentHistory.pendingAction?.actionId === pendingActionId) { + // Resolve the failed action durably BEFORE restoring the parent. Otherwise + // history resume auto-approves the same action and repeats this rollback. + // If this write fails, do not schedule a replacement task at all. + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + const messages = await readApiMessages({ taskId: parentTaskId, globalStoragePath }) + const hasResult = messages.some( + (message) => + message.role === "user" && + Array.isArray(message.content) && + message.content.some( + (block) => block.type === "tool_result" && block.tool_use_id === pendingActionId, + ), + ) + if (!hasResult) { + await saveApiMessages({ + taskId: parentTaskId, + globalStoragePath, + merge: true, + messages: [ + ...messages, + { + role: "user", + ts: Date.now(), + content: [ + { + type: "tool_result", + tool_use_id: pendingActionId, + content: `Subtask creation failed: ${err instanceof Error ? err.message : String(err)}`, + is_error: true, + }, + ], + }, + ], + }) + } + } await this.createTaskWithHistoryItem(parentHistory) } catch (rollbackError) { this.log( @@ -4075,6 +4118,9 @@ export class ClineProvider (rollbackError as Error)?.message ?? String(rollbackError) }`, ) + void vscode.window.showErrorMessage( + "Subtask creation failed and the parent could not be restored safely. Reopen the task to retry.", + ) } throw err } diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 93741e9174..f7835e512f 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -6,7 +6,7 @@ }, "__tests__/ClineProvider.delegation.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 9 + "count": 8 } }, "__tests__/ClineProvider.history-resume-delegation.spec.ts": { From 1867dbb307bfff44bbb0ee6b134cad1d28b1ea03 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 21 Sep 2026 18:21:37 +0000 Subject: [PATCH 2/5] Fix interrupted delegation ownership reads and rollback result ordering Amp-Thread-ID: https://ampcode.com/threads/T-01a0c52b-e493-76ea-aad5-70f0077cad14 --- .../ClineProvider.delegation.spec.ts | 81 ++++++++++++++++++- src/core/task-persistence/TaskHistoryStore.ts | 22 +++-- .../__tests__/TaskHistoryStore.spec.ts | 27 +++++++ src/core/webview/ClineProvider.ts | 46 +++++++---- 4 files changed, 146 insertions(+), 30 deletions(-) diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index 3f0596c432..f8ac4f8454 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -54,6 +54,41 @@ const makeParentTask = () => }) as any describe("ClineProvider.delegateParentAndOpenChild()", () => { + it("stops before delegation when refreshing the interrupted task's owner fails", async () => { + const parentTask = makeParentTask() + const historyItem: HistoryItem = { + ...parentHistoryItem, + status: "interrupted", + parentTaskId: "root", + rootTaskId: "root", + } + const taskHistoryStore = makeStoreStub({ get: vi.fn().mockReturnValue(historyItem) }) + taskHistoryStore.invalidate.mockImplementation(async (id: string) => { + if (id === "root") throw new Error("owner read failed") + }) + const provider = { + getCurrentTask: vi.fn(() => parentTask), + createTask: vi.fn(), + removeClineFromStack: vi.fn(), + taskHistoryStore, + // Only the boundaries reached before delegation are needed for this failure. + } as unknown as ClineProvider + + await expect( + ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + }), + ).rejects.toThrow("owner read failed") + expect(taskHistoryStore.invalidate).toHaveBeenNthCalledWith(2, "root") + expect(taskHistoryStore.atomicReadAndUpdate).not.toHaveBeenCalled() + expect(provider.createTask).not.toHaveBeenCalled() + expect(provider.removeClineFromStack).not.toHaveBeenCalled() + expect(historyItem).toMatchObject({ status: "interrupted", parentTaskId: "root", rootTaskId: "root" }) + }) + it("rejects a stale restored action before delegation side effects", async () => { const parentTask = makeParentTask() const removeClineFromStack = vi.fn() @@ -727,7 +762,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(durableParent.awaitingChildId).toBe("child-1") }) - it.each(["legacy", "pending", "resolved", "read-failure", "write-failure"] as const)( + it.each(["legacy", "pending", "flushed", "resolved", "read-failure", "write-failure"] as const)( "rolls back without automatic action replay after a metadata failure (%s)", async (scenario) => { const persistError = new Error("parent metadata persist failed") @@ -753,6 +788,21 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { content: [{ type: "tool_use", id: "failed-action", name: "new_task", input: {} }], }, ] + if (scenario === "flushed") { + durableMessages[0].content = [ + { type: "tool_use", id: "earlier-action", name: "read_file", input: {} }, + { type: "tool_use", id: "failed-action", name: "new_task", input: {} }, + ] + durableMessages.push({ + role: "user", + ts: 123, + messageId: "flushed-turn", + content: [ + { type: "tool_result", tool_use_id: "earlier-action", content: "File contents" }, + { type: "text", text: "Continue with the subtask" }, + ], + }) + } if (scenario === "resolved") { durableMessages.push({ role: "user", @@ -784,7 +834,14 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { // A rehydrated Task must find a durable result before it can replay the action. expect(durableMessages.at(-1)).toMatchObject({ role: "user", - content: [{ type: "tool_result", tool_use_id: pendingAction.actionId, is_error: true }], + content: expect.arrayContaining([ + { + type: "tool_result", + tool_use_id: pendingAction.actionId, + is_error: true, + content: expect.any(String), + }, + ]), }) } }) @@ -842,8 +899,26 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(createTaskWithHistoryItem).toHaveBeenCalledExactlyOnceWith(historyItem) } expect(saveApiMessages).toHaveBeenCalledTimes( - scenario === "pending" || scenario === "write-failure" ? 1 : 0, + scenario === "pending" || scenario === "flushed" || scenario === "write-failure" ? 1 : 0, ) + if (scenario === "flushed") { + expect(durableMessages).toHaveLength(2) + expect(durableMessages[1]).toEqual({ + role: "user", + ts: 123, + messageId: "flushed-turn", + content: [ + { type: "tool_result", tool_use_id: "earlier-action", content: "File contents" }, + { + type: "tool_result", + tool_use_id: "failed-action", + content: "Subtask creation failed: parent metadata persist failed", + is_error: true, + }, + { type: "text", text: "Continue with the subtask" }, + ], + }) + } if (scenario === "pending") { expect(durableMessages).toHaveLength(2) expect(durableMessages[1]).toMatchObject({ diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 3d4cc47604..a9b43bfb66 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -756,17 +756,13 @@ export class TaskHistoryStore { */ async invalidate(taskId: string): Promise { return this.withLock(async () => { - try { - const item = await this.readTaskFile(taskId) - if (item) { - this.cache.set(taskId, item) - } else { - this.cache.delete(taskId) - } - this.taskFileMtimes.delete(taskId) - } catch { + const item = await this.readTaskFile(taskId) + if (item) { + this.cache.set(taskId, item) + } else { this.cache.delete(taskId) } + this.taskFileMtimes.delete(taskId) }) } @@ -877,9 +873,11 @@ export class TaskHistoryStore { try { const raw = await fs.readFile(filePath, "utf8") const item: HistoryItem = JSON.parse(raw) - return item.id ? item : null - } catch { - return null + if (!item?.id) throw new Error(`Invalid task history record: ${filePath}`) + return item + } catch (error) { + if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") return null + throw error } } diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts index 3e277ac867..2d8aa27072 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts @@ -93,6 +93,33 @@ describe("TaskHistoryStore", () => { }) }) + describe("invalidate()", () => { + it.each(["malformed", "invalid-record", "read-error", "missing"] as const)( + "preserves cached ownership unless the record is missing (%s)", + async (scenario) => { + await store.initialize() + store.dispose() // Keep filesystem watcher reconciliation out of this explicit refresh test. + const owner = makeHistoryItem({ id: "owner", status: "delegated", awaitingChildId: "child" }) + await store.upsert(owner) + const filePath = path.join(tmpDir, "tasks", "owner", GlobalFileNames.historyItem) + if (scenario === "malformed" || scenario === "invalid-record") { + await fs.writeFile(filePath, scenario === "malformed" ? "{" : "{}") + } else { + await fs.unlink(filePath) + if (scenario === "read-error") await fs.mkdir(filePath) + } + + if (scenario === "missing") { + await expect(store.invalidate("owner")).resolves.toBeUndefined() + expect(store.get("owner")).toBeUndefined() + } else { + await expect(store.invalidate("owner")).rejects.toThrow() + expect(store.get("owner")).toEqual(owner) + } + }, + ) + }) + describe("pending action persistence", () => { it("persists set and clear operations across store reinitialization", async () => { await store.initialize() diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 928b9e7ae0..10d758a0ed 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4089,25 +4089,41 @@ export class ClineProvider ), ) if (!hasResult) { + const result: Anthropic.ToolResultBlockParam = { + type: "tool_result", + tool_use_id: pendingActionId, + content: `Subtask creation failed: ${err instanceof Error ? err.message : String(err)}`, + is_error: true, + } + const toolUseIndex = messages.findIndex( + (message) => + message.role === "assistant" && + Array.isArray(message.content) && + message.content.some( + (block) => block.type === "tool_use" && block.id === pendingActionId, + ), + ) + const nextMessage = toolUseIndex === -1 ? undefined : messages[toolUseIndex + 1] + if (nextMessage?.role === "user") { + const content = + typeof nextMessage.content === "string" + ? [{ type: "text" as const, text: nextMessage.content }] + : [...nextMessage.content] + const firstNonTool = content.findIndex((block) => block.type !== "tool_result") + content.splice(firstNonTool === -1 ? content.length : firstNonTool, 0, result) + messages[toolUseIndex + 1] = { ...nextMessage, content } + } else { + messages.splice(toolUseIndex === -1 ? messages.length : toolUseIndex + 1, 0, { + role: "user", + ts: Date.now(), + content: [result], + }) + } await saveApiMessages({ taskId: parentTaskId, globalStoragePath, merge: true, - messages: [ - ...messages, - { - role: "user", - ts: Date.now(), - content: [ - { - type: "tool_result", - tool_use_id: pendingActionId, - content: `Subtask creation failed: ${err instanceof Error ? err.message : String(err)}`, - is_error: true, - }, - ], - }, - ], + messages, }) } } From 1a67e404191adcf6567277482585c937e987513d Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 24 Sep 2026 21:11:02 +0200 Subject: [PATCH 3/5] fix(history): reject task records whose id does not match the task readTaskFile accepted any truthy id, so a history_item.json holding a non-string id or another task's id (e.g. a copied task directory) could replace the cached owner during invalidate(). Require a string id equal to the requested taskId; invalid records now throw and keep the cache. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/core/task-persistence/TaskHistoryStore.ts | 10 +++- .../__tests__/TaskHistoryStore.spec.ts | 59 +++++++++++-------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index a9b43bfb66..1f20ccb00c 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -872,9 +872,13 @@ export class TaskHistoryStore { try { const raw = await fs.readFile(filePath, "utf8") - const item: HistoryItem = JSON.parse(raw) - if (!item?.id) throw new Error(`Invalid task history record: ${filePath}`) - return item + const item: unknown = JSON.parse(raw) + // Reject records that belong to another task (e.g. a copied task directory) + // so they can never replace this task's cached ownership. + if (typeof item !== "object" || item === null || (item as { id?: unknown }).id !== taskId) { + throw new Error(`Invalid task history record: ${filePath}`) + } + return item as HistoryItem } catch (error) { if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") return null throw error diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts index 2d8aa27072..8c896b96df 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts @@ -94,30 +94,43 @@ describe("TaskHistoryStore", () => { }) describe("invalidate()", () => { - it.each(["malformed", "invalid-record", "read-error", "missing"] as const)( - "preserves cached ownership unless the record is missing (%s)", - async (scenario) => { - await store.initialize() - store.dispose() // Keep filesystem watcher reconciliation out of this explicit refresh test. - const owner = makeHistoryItem({ id: "owner", status: "delegated", awaitingChildId: "child" }) - await store.upsert(owner) - const filePath = path.join(tmpDir, "tasks", "owner", GlobalFileNames.historyItem) - if (scenario === "malformed" || scenario === "invalid-record") { - await fs.writeFile(filePath, scenario === "malformed" ? "{" : "{}") - } else { - await fs.unlink(filePath) - if (scenario === "read-error") await fs.mkdir(filePath) - } + const invalidRecords = { + malformed: "{", + "invalid-record": "{}", + "non-object-record": "null", + "non-string-id": JSON.stringify({ id: 42, status: "active" }), + "other-task-id": JSON.stringify({ id: "other-task", status: "active" }), + } as const + + it.each([ + "malformed", + "invalid-record", + "non-object-record", + "non-string-id", + "other-task-id", + "read-error", + "missing", + ] as const)("preserves cached ownership unless the record is missing (%s)", async (scenario) => { + await store.initialize() + store.dispose() // Keep filesystem watcher reconciliation out of this explicit refresh test. + const owner = makeHistoryItem({ id: "owner", status: "delegated", awaitingChildId: "child" }) + await store.upsert(owner) + const filePath = path.join(tmpDir, "tasks", "owner", GlobalFileNames.historyItem) + if (scenario in invalidRecords) { + await fs.writeFile(filePath, invalidRecords[scenario as keyof typeof invalidRecords]) + } else { + await fs.unlink(filePath) + if (scenario === "read-error") await fs.mkdir(filePath) + } - if (scenario === "missing") { - await expect(store.invalidate("owner")).resolves.toBeUndefined() - expect(store.get("owner")).toBeUndefined() - } else { - await expect(store.invalidate("owner")).rejects.toThrow() - expect(store.get("owner")).toEqual(owner) - } - }, - ) + if (scenario === "missing") { + await expect(store.invalidate("owner")).resolves.toBeUndefined() + expect(store.get("owner")).toBeUndefined() + } else { + await expect(store.invalidate("owner")).rejects.toThrow() + expect(store.get("owner")).toEqual(owner) + } + }) }) describe("pending action persistence", () => { From b4d593dd80c3c29e4abb658168f0859bf78a9a8a Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 28 Sep 2026 18:57:37 +0200 Subject: [PATCH 4/5] fix: refuse orphan tool_result during delegation rollback - Abort rollback before saving API history or restoring the parent when the failed pending action has no matching assistant tool_use. A standalone tool_result is invalid history that native tool-call providers reject; the parent stays available for an explicit retry. - Assert the specific error for each invalidate() failure scenario (EISDIR, SyntaxError, invalid record) instead of any rejection. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/__tests__/ClineProvider.delegation.spec.ts | 8 ++++++-- .../__tests__/TaskHistoryStore.spec.ts | 8 +++++++- src/core/webview/ClineProvider.ts | 11 +++++++++-- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index f8ac4f8454..e9cd5644c3 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -762,7 +762,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(durableParent.awaitingChildId).toBe("child-1") }) - it.each(["legacy", "pending", "flushed", "resolved", "read-failure", "write-failure"] as const)( + it.each(["legacy", "pending", "flushed", "resolved", "missing-tool-use", "read-failure", "write-failure"] as const)( "rolls back without automatic action replay after a metadata failure (%s)", async (scenario) => { const persistError = new Error("parent metadata persist failed") @@ -803,6 +803,10 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { ], }) } + if (scenario === "missing-tool-use") { + // No owning tool_use: a standalone tool_result would be rejected by native tool-call providers. + durableMessages = [] + } if (scenario === "resolved") { durableMessages.push({ role: "user", @@ -893,7 +897,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(removeClineFromStack).toHaveBeenNthCalledWith(1) expect(removeClineFromStack).toHaveBeenNthCalledWith(2) expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) - if (scenario === "read-failure" || scenario === "write-failure") { + if (scenario === "read-failure" || scenario === "write-failure" || scenario === "missing-tool-use") { expect(createTaskWithHistoryItem).not.toHaveBeenCalled() } else { expect(createTaskWithHistoryItem).toHaveBeenCalledExactlyOnceWith(historyItem) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts index 8c896b96df..5d8eee1cef 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts @@ -127,7 +127,13 @@ describe("TaskHistoryStore", () => { await expect(store.invalidate("owner")).resolves.toBeUndefined() expect(store.get("owner")).toBeUndefined() } else { - await expect(store.invalidate("owner")).rejects.toThrow() + await expect(store.invalidate("owner")).rejects.toMatchObject( + scenario === "read-error" + ? { code: "EISDIR" } + : scenario === "malformed" + ? { name: "SyntaxError" } + : { message: expect.stringContaining("Invalid task history record") }, + ) expect(store.get("owner")).toEqual(owner) } }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3fe267b88d..11eed1d33b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4049,7 +4049,14 @@ export class ClineProvider (block) => block.type === "tool_use" && block.id === pendingActionId, ), ) - const nextMessage = toolUseIndex === -1 ? undefined : messages[toolUseIndex + 1] + // A standalone tool_result with no owning tool_use is invalid API history that + // native tool-call providers reject; leave the parent for an explicit retry instead. + if (toolUseIndex === -1) { + throw new Error( + `[delegateParentAndOpenChild] Cannot resolve pending action ${pendingActionId}: no matching tool_use exists`, + ) + } + const nextMessage = messages[toolUseIndex + 1] if (nextMessage?.role === "user") { const content = typeof nextMessage.content === "string" @@ -4059,7 +4066,7 @@ export class ClineProvider content.splice(firstNonTool === -1 ? content.length : firstNonTool, 0, result) messages[toolUseIndex + 1] = { ...nextMessage, content } } else { - messages.splice(toolUseIndex === -1 ? messages.length : toolUseIndex + 1, 0, { + messages.splice(toolUseIndex + 1, 0, { role: "user", ts: Date.now(), content: [result], From 7e110806ec80a883493e28f6ef90f8f4a5f5f5cb Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 5 Oct 2026 19:23:33 +0200 Subject: [PATCH 5/5] Stop cancelled pending delegation before child execution Guard approval and handoff cancellation, retain interrupted committed children for recovery, and model terminal handoff cancellation. Amp-Thread-ID: https://ampcode.com/threads/T-01a10cc3-a401-7090-be6f-1ae696680827 Co-authored-by: Amp --- docs/architecture/task-lifecycle-model.md | 4 +- scripts/check-provider-handoff-scheduler.ts | 49 ++++++++- .../ClineProvider.delegation.spec.ts | 103 +++++++++++++++++- src/core/task/Task.ts | 3 + .../task/__tests__/Task.persistence.spec.ts | 16 +++ src/core/webview/ClineProvider.ts | 39 ++++++- 6 files changed, 209 insertions(+), 5 deletions(-) diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 11351f15a4..6ce140c466 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -99,7 +99,9 @@ The umbrella command also runs a separate bounded child model for in-memory abor Provider locking, paused-child/current-task publication, and semaphore admission/release are explicit model abstractions rather than imported production code. Focused provider and `TaskScheduler` tests cover those concrete adapters. Lifecycle commits and completion use the real reducers. Parent publication and its queued continuation share an explicit transition owner: the fixed policy retains that ownership through matching resume invocation, then models the resumed run settling outside transition ownership. This permits a new delegation generation to begin while the prior resumed run remains active without allowing a stale continuation to start across the newer transition. The fixed policy checks every successor for continuous publication, one child start and commit per generation, exact commit-before-start ownership, permit release before parent resume or redelegation, matching parent transition/continuation ownership at resume invocation, and consistent final child/parent publication. It also requires both resume phases, every other action, and named semantic landmarks to remain reachable and fails if the depth boundary has an unseen successor. -Six injected legacy transition policies must produce deterministic shortest counterexamples through the same explorer: start before commit, resume before permit release, redelegation before permit release, empty current-task publication, two stale provider commits from competing snapshots, and releasing parent-transition serialization immediately after publication. The last witness must causally include first-child completion and parent publication, a second-child commit, release of the first child's scheduler permit, and then the stale first-child continuation. The checker prints the distinct reachable-state count, complete scenario/action/landmark coverage, bounds, and each named counterexample trace. It deliberately does not add a WAL, global profile projection, or scheduler state to persisted `HistoryItem` records. +Seven injected legacy transition policies must produce deterministic shortest counterexamples through the same explorer: start after cancellation, start before commit, resume before permit release, redelegation before permit release, empty current-task publication, two stale provider commits from competing snapshots, and releasing parent-transition serialization immediately after publication. The last witness must causally include first-child completion and parent publication, a second-child commit, release of the first child's scheduler permit, and then the stale first-child continuation. The checker prints the distinct reachable-state count, complete scenario/action/landmark coverage, bounds, and each named counterexample trace. It deliberately does not add a WAL, global profile projection, or scheduler state to persisted `HistoryItem` records. + +Handoff cancellation is terminal for scheduling: a paused child is removed before commit, or interrupted with the production reducer after commit so the handoff remains recoverable. The model requires both cancellation landmarks and rejects subsequent child starts or retained active children. It abstracts successful cleanup as one step; focused adapter tests cover cancellation/disposal after approval, during parent flush/eviction, child creation, metadata commit, and publication. Intentional parent eviction is not treated as a cancellation of its own handoff. For #921, the execution-context matrix checks saved, unsaved, and locked profile selection at the handoff boundary. For that bounded matrix, it establishes only that delegation writes the requested task-local mode and cloned configuration into the child context. It does not prove that every downstream consumer reads that context. The checker retains a divergent-mode witness in which the child task mode differs from the shared provider mode so reader refinements can demonstrate that choosing the wrong source is observable. diff --git a/scripts/check-provider-handoff-scheduler.ts b/scripts/check-provider-handoff-scheduler.ts index e6c4c29a38..b34fdf980d 100644 --- a/scripts/check-provider-handoff-scheduler.ts +++ b/scripts/check-provider-handoff-scheduler.ts @@ -3,7 +3,11 @@ import assert from "node:assert/strict" import type { HistoryItem, ProviderSettings } from "@roo-code/types" import { selectHandoffExecutionContext } from "../src/core/task/providerHandoff" -import { completeDelegatedChild, delegateTaskToChild } from "../src/core/task-persistence/taskLifecycle" +import { + completeDelegatedChild, + delegateTaskToChild, + interruptDelegatedChild, +} from "../src/core/task-persistence/taskLifecycle" const PROVIDERS = ["a", "b"] as const type Provider = (typeof PROVIDERS)[number] @@ -18,9 +22,11 @@ type Policy = { emptyPublication?: boolean staleConcurrentCommits?: boolean releaseParentTransitionAfterPublication?: boolean + startAfterCancellation?: boolean } type ModelState = { + cancelled?: boolean generation: Generation parent: HistoryItem children: Partial> @@ -62,8 +68,11 @@ const EXPECTED_ACTIONS = [ "resume-parent", "settle-parent", "redelegate", + "cancel-handoff", ] as const const LANDMARKS = { + "cancelled-before-commit": (state: ModelState) => state.cancelled === true && state.commitOwner === undefined, + "cancelled-after-commit": (state: ModelState) => state.cancelled === true && state.commitOwner !== undefined, "competing-claims": (state: ModelState) => Object.keys(state.claims).length === 2, "prepared-before-commit": (state: ModelState) => state.prepared !== undefined && state.commitOwner === undefined, "committed-before-start": (state: ModelState) => @@ -84,6 +93,11 @@ const LANDMARKS = { const FIXED_POLICY: Policy = { name: "fixed" } const LEGACY_POLICIES: Array = [ + { + name: "start-after-cancellation", + startAfterCancellation: true, + expectedViolation: "child started after cancellation", + }, { name: "start-before-commit", startBeforeCommit: true, expectedViolation: "child started without exact commit" }, { name: "resume-before-permit-release", @@ -251,6 +265,33 @@ function explore( function transitions(state: ModelState, policy: Policy): Transition[] { const result: Transition[] = [] + if (state.cancelled) { + if (policy.startAfterCancellation && state.commitOwner) { + return [ + action("start-cancelled-child", "start", state, (next) => { + next.startedProviders = [state.commitOwner!] + next.childPermit = "held" + }), + ] + } + return result + } + if (state.startedProviders.length === 0 && !state.parentQueued) { + result.push( + action("cancel-handoff", "cancel-handoff", state, (next) => { + next.cancelled = true + next.publishedTask = undefined + if (state.commitOwner) { + const childId = childIdFor(state.generation, state.commitOwner) + next.children[childId] = interruptDelegatedChild(state.parent, state.children[childId]!) + } + if (state.prepared) { + delete next.children[childIdFor(state.generation, state.prepared.provider)] + next.prepared = undefined + } + }), + ) + } for (const provider of PROVIDERS) { if (!state.parentQueued && !state.claims[provider] && state.commitOwner === undefined) { result.push( @@ -407,7 +448,11 @@ function transitions(state: ModelState, policy: Policy): Transition[] { function invariantViolations(state: ModelState): string[] { const violations: string[] = [] - if (!state.publishedTask) violations.push("observable current task is empty") + if (state.cancelled && state.startedProviders.length > 0) violations.push("child started after cancellation") + if (state.cancelled && Object.values(state.children).some((child) => child?.status === "active")) { + violations.push("cancelled handoff retained an active child") + } + if (!state.publishedTask && !state.cancelled) violations.push("observable current task is empty") if (state.startedProviders.length > 1) violations.push("multiple child starts for one parent generation") if (state.committedProviders.length > 1) violations.push("multiple provider commits for one parent generation") for (const provider of state.parentQueued ? [] : state.startedProviders) { diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index 2309e5e55d..c53d317904 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -11,6 +11,7 @@ import { ClineProvider } from "../core/webview/ClineProvider" import { TaskScheduler } from "../core/task/TaskScheduler" import { LifecycleTransitionError } from "../core/task-persistence" import { readApiMessages, saveApiMessages, type ApiMessage } from "../core/task-persistence/apiMessages" +import { makeProviderStub } from "./helpers/provider-stub" vi.mock("../core/task-persistence/apiMessages", async (importOriginal) => ({ ...(await importOriginal()), @@ -59,6 +60,105 @@ const makeParentTask = () => }) as any describe("ClineProvider.delegateParentAndOpenChild()", () => { + it.each([ + ["entry-abort", false, false], + ["entry-abandon", false, false], + ["entry-dispose", false, false], + ["flush-abort", false, false], + ["flush-dispose", false, false], + ["evict-dispose", false, false], + ["create-dispose", true, false], + ["create-abort", true, false], + ["commit-dispose", true, true], + ["commit-abort", true, true], + ["publish-dispose", true, true], + ] as const)("stops cancelled handoff at %s", async (phase, created, committed) => { + const parent = makeParentTask() + const child = { + taskId: "child-1", + abort: false, + abandoned: false, + run: vi.fn(), + dispose: vi.fn().mockResolvedValue(undefined), + } + let current: { taskId: string } | undefined = parent + const records = new Map([["parent-1", { ...parentHistoryItem, status: "interrupted" }]]) + const cancel = () => { + if (phase.endsWith("abort")) { + if (created) child.abort = true + else parent.abort = true + } else if (phase.endsWith("abandon")) parent.abandoned = true + else provider["_disposed"] = true + } + const store = { + invalidate: vi.fn().mockResolvedValue(undefined), + get: (id: string) => records.get(id), + atomicReadAndUpdate: vi.fn(async (id: string, update: (item: HistoryItem) => HistoryItem) => { + records.set(id, update(records.get(id)!)) + if (id === "parent-1" && phase.startsWith("commit")) cancel() + return [...records.values()] + }), + } + const schedule = vi.fn() + const restore = vi.fn() + const create = vi.fn(async () => { + current = child + records.set(child.taskId, { + ...parentHistoryItem, + id: child.taskId, + status: "active", + parentTaskId: "parent-1", + }) + if (phase.startsWith("create")) cancel() + return child + }) + const provider = makeProviderStub({ + taskHistoryStore: store, + taskScheduler: { schedule }, + createTask: create, + createTaskWithHistoryItem: restore, + getCurrentTask: () => current, + isViewLaunched: true, + emit: vi.fn(), + postMessageToWebview: vi.fn(async () => { + if (phase.startsWith("publish")) cancel() + }), + deleteTaskWithId: vi.fn(async (id: string) => { + records.delete(id) + }), + removeClineFromStack: vi.fn(async () => { + if (current === parent) { + parent.abort = true + parent.abandoned = true + } + current = undefined + if (phase.startsWith("evict")) cancel() + }), + }) + parent.flushPendingToolResultsToHistory.mockImplementation(async () => { + if (phase.startsWith("flush")) cancel() + return true + }) + if (phase.startsWith("entry")) cancel() + await expect( + ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Child", + initialTodos: [], + mode: "code", + }), + ).rejects.toThrow(/cancelled|disposed/) + expect(create).toHaveBeenCalledTimes(created ? 1 : 0) + expect(schedule).not.toHaveBeenCalled() + expect(child.run).not.toHaveBeenCalled() + expect(restore).not.toHaveBeenCalled() + if (created) expect(child.dispose).toHaveBeenCalledOnce() + if (committed) { + expect(records.get("parent-1")).toMatchObject({ status: "delegated", awaitingChildId: "child-1" }) + expect(records.get("child-1")).toMatchObject({ status: "interrupted", parentTaskId: "parent-1" }) + } else expect(records.has("child-1")).toBe(false) + }) + it("stops before delegation when refreshing the interrupted task's owner fails", async () => { const parentTask = makeParentTask() const historyItem: HistoryItem = { @@ -587,10 +687,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { }), }) + const parentTask = makeParentTask() const provider = { taskScheduler: new TaskScheduler(), emit: vi.fn(), - getCurrentTask: vi.fn(() => makeParentTask()), + getCurrentTask: vi.fn(() => parentTask), removeClineFromStack: vi.fn().mockResolvedValue(undefined), createTask: vi.fn().mockResolvedValue({ taskId: "child-2", start: vi.fn(), run: () => Promise.resolve() }), handleModeSwitch: vi.fn().mockResolvedValue(undefined), diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index fce6bcb427..bc58f687a6 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3170,6 +3170,9 @@ export class Task extends EventEmitter implements TaskLike { } let { response, text, images, queuedMessageId } = await this.ask("tool", action.approvalText, false) + if (this.abort || this.abandoned) { + return + } if (response === "yesButtonClicked") { if (action.kind === "create_subtask") { diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index c743d651ae..3cf3375919 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -1363,6 +1363,22 @@ describe("Task persistence", () => { todos: [], } + it.each(["abort", "dispose"] as const)( + "does not delegate when %s wins pending approval", + async (cancellation) => { + const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, startTask: false }) + const approval = createDeferred<{ response: "yesButtonClicked" }>() + vi.spyOn(task, "ask").mockReturnValue(approval.promise) + mockProvider.delegateParentAndOpenChild = vi.fn() + const resume = task["resumePendingTaskAction"](createSubtaskAction) + if (cancellation === "dispose") await task.dispose() + else task.abort = true + approval.resolve({ response: "yesButtonClicked" }) + await resume + expect(mockProvider.delegateParentAndOpenChild).not.toHaveBeenCalled() + }, + ) + it("replays an unresolved pending action instead of a generic resume ask", async () => { const messages: ClineMessage[] = [ { ts: 1, type: "say", say: "text", text: "Child" }, diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index b30ee04b11..330425d30a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3803,6 +3803,12 @@ export class ClineProvider `[delegateParentAndOpenChild] Parent mismatch: expected ${parentTaskId}, current ${parent.taskId}`, ) } + const assertParentAvailable = () => { + if (this._disposed || parent.abort || parent.abandoned || this.getCurrentTask() !== parent) { + throw new Error("[delegateParentAndOpenChild] Delegation cancelled before handoff") + } + } + assertParentAvailable() // A different provider may have delegated this parent while this call // waited on the shared lock. Refresh before mutating either task stack. @@ -3900,6 +3906,7 @@ export class ClineProvider // 3) Enforce single-open invariant by closing/disposing the parent first // This ensures we never have >1 tasks open at any time during delegation. // Await abort completion to ensure clean disposal and prevent unhandled rejections. + assertParentAvailable() try { await this.removeClineFromStack() } catch (error) { @@ -3910,6 +3917,10 @@ export class ClineProvider ) // Non-fatal: proceed with child creation even if parent cleanup had issues } + // Parent abort/abandon now belongs to our intentional eviction, not cancellation. + if (this._disposed) { + throw new Error("[delegateParentAndOpenChild] Provider disposed during handoff") + } // 4) Bind the child directly to the delegating task's local provider // context. Delegation never mutates shared profile/global state. @@ -3944,7 +3955,11 @@ export class ClineProvider // 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. + let delegationCommitted = false try { + if (this._disposed || child.abort || child.abandoned) { + throw new Error("[delegateParentAndOpenChild] Delegation cancelled during child creation") + } await this.taskHistoryStore.atomicReadAndUpdate(parentTaskId, (historyItem) => { if (pendingActionId && historyItem.pendingAction?.actionId !== pendingActionId) { throw new Error( @@ -3964,6 +3979,7 @@ export class ClineProvider delegated.pendingAction?.actionId === pendingActionId ? undefined : delegated.pendingAction, } }) + delegationCommitted = true this.recentTasksCache = undefined if (this.isViewLaunched) { const updatedItem = this.taskHistoryStore.get(parentTaskId) @@ -3971,7 +3987,27 @@ export class ClineProvider await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedItem }) } } + if (this._disposed || child.abort || child.abandoned) { + throw new Error("[delegateParentAndOpenChild] Delegation cancelled before scheduling") + } } catch (err) { + const cancelled = this._disposed || child.abort || child.abandoned + if (cancelled && delegationCommitted) { + // Keep a committed handoff recoverable, but never start it after teardown. + // We already own the parent transition lock, so do not call eviction's + // lock-taking interruption helper here. + if (this.getCurrentTask()?.taskId === child.taskId) await this.removeClineFromStack() + await ClineProvider.prototype.drainTaskDisposal.call(this, child) + const parentHistory = this.taskHistoryStore.get(parentTaskId) + if (parentHistory?.awaitingChildId === child.taskId) { + await this.taskHistoryStore.atomicReadAndUpdate(child.taskId, (historyItem) => + historyItem.status === "active" + ? interruptDelegatedChild(parentHistory, historyItem) + : historyItem, + ) + } + throw err + } this.log( `[delegateParentAndOpenChild] Failed to persist parent metadata for ${parentTaskId} -> ${child.taskId}: ${ (err as Error)?.message ?? String(err) @@ -4008,6 +4044,7 @@ export class ClineProvider if (this.getCurrentTask()?.taskId === child.taskId) { await this.removeClineFromStack() } + if (cancelled) await child.dispose() } catch (cleanupError) { this.log( `[delegateParentAndOpenChild] Failed to close paused child ${child.taskId} during rollback: ${ @@ -4026,7 +4063,7 @@ export class ClineProvider } try { // Never restore a pending action whose authoritative settlement failed. - if (!settlementFailed) { + if (!settlementFailed && !cancelled && !this._disposed) { const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) if (pendingActionId && parentHistory.pendingAction?.actionId === pendingActionId) { // Resolve the failed action durably BEFORE restoring the parent. Otherwise