From 72e1216b696a0c43b93935d698c928da6e78569b Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 20 Sep 2026 14:00:20 +0000 Subject: [PATCH 1/3] 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/3] 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/3] 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", () => {