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..854ebf6293 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,62 @@ suite("Roo Code Subtasks", function () { } }) + test("interrupted child settles pending new_task before resume despite 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) + // A generic resume ask is emitted only after authoritative pending-action settlement. + // Waiting for it verifies the recovery path finished without creating another child. + await waitFor(() => asks[childId]?.some(({ ask }) => ask === "resume_task") ?? false) + const resumed = await api.getTaskHistoryItem(childId) + assert.strictEqual(resumed?.status, "interrupted") + assert.strictEqual(resumed?.pendingAction, undefined) + assert.strictEqual(resumed?.parentTaskId, rootId) + assert.deepStrictEqual(resumed?.childIds ?? [], []) + assert.strictEqual(api.getCurrentTaskStack().at(-1), childId) + assert.strictEqual(delegations.length, 1) + } 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 7b2eabdd63..6ce140c466 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -58,6 +58,8 @@ Each task slot can also hold one of two pending `create_subtask` actions. A `sta 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 live 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 settlement or result persistence stops restoration. Reload remains conservative: persisted pending actions do not distinguish an unattempted approval from a rejected delegation whose settlement failed, so interrupted pending approvals are settled before generic resume rather than replayed. Provider and history-resume tests cover this persistence boundary; the subtask extension-host smoke test checks that enabling subtask auto-approval does not replay the interrupted action (#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: @@ -97,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/scripts/check-task-lifecycle.ts b/scripts/check-task-lifecycle.ts index bf05ad68e9..0465e6f1c0 100644 --- a/scripts/check-task-lifecycle.ts +++ b/scripts/check-task-lifecycle.ts @@ -36,8 +36,20 @@ interface WitnessContext { const MAX_DEPTH = 12 const MAX_STATES = 10_000 const actionIds = ["action-1", "action-2"] as const -const expectedActions = ["delegate", "interrupt", "complete", "abandon", "stage", "settle-rejected"] as const +const expectedActions = [ + "delegate", + "resume-delegate", + "interrupt", + "complete", + "abandon", + "stage", + "settle-rejected", +] 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" && @@ -146,14 +158,20 @@ function transitions(state: ModelState): Transition[] { const awaitedStatus = parent.awaitingChildId ? state[parent.awaitingChildId as TaskId]?.status : undefined const delegationValid = - parent.status === "active" || (parent.status === "delegated" && awaitedStatus === "interrupted") + parent.status === "active" || + parent.status === "interrupted" || + (parent.status === "delegated" && awaitedStatus === "interrupted") for (const childId of taskIds) { if (childId === parentId || state[childId]) continue if (!delegationValid) continue - const delegated = { ...delegateTaskToChild(parent, childId, awaitedStatus), pendingAction: undefined } + const owningParent = parent.parentTaskId ? state[parent.parentTaskId as TaskId] : undefined + const delegated = { + ...delegateTaskToChild(parent, childId, awaitedStatus, owningParent), + pendingAction: undefined, + } result.push({ - name: `delegate(${parentId}, ${childId})`, + name: `${parent.status === "interrupted" ? "resume-delegate" : "delegate"}(${parentId}, ${childId})`, next: replace(state, delegated, task(childId, parentId)), delegation: { parentId }, }) @@ -168,7 +186,13 @@ function transitions(state: ModelState): Transition[] { } const pending = parent.pendingAction - if (!delegationValid && parent.status !== "completed" && pending?.kind === "create_subtask") { + // Reload conservatively settles interrupted approvals: persisted actions do not + // distinguish an unattempted approval from a rejected attempt whose settlement failed. + if ( + (!delegationValid || parent.status === "interrupted") && + parent.status !== "completed" && + pending?.kind === "create_subtask" + ) { result.push({ name: `settle-rejected(${parentId}, ${actionId})`, next: replace(state, settleRejectedCreateSubtaskAction(parent, actionId)), @@ -416,8 +440,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/) @@ -437,7 +470,11 @@ function runRepresentativeScenarios(): void { status: "interrupted", pendingAction: createSubtaskAction("action-1"), } - assert.throws(() => delegateTaskToChild(rejectedParent, childA.id), /Invalid task status transition/) + assert.equal(delegateTaskToChild(rejectedParent, childA.id).status, "delegated") + assert.throws( + () => delegateTaskToChild({ ...rejectedParent, status: "completed" }, childA.id), + /Invalid task status transition/, + ) const settled = settleRejectedCreateSubtaskAction(rejectedParent, "action-1") assert.equal(settled.status, "interrupted") assert.equal(settled.pendingAction, undefined) diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index 68f39c6517..c53d317904 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -10,6 +10,14 @@ import { providerIdentifiers, RooCodeEventName } from "@roo-code/types" 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()), + readApiMessages: vi.fn(), + saveApiMessages: vi.fn(), +})) const parentHistoryItem: HistoryItem = { id: "parent-1", @@ -52,6 +60,140 @@ 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 = { + ...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() @@ -95,49 +237,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 + 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 - await ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { - parentTaskId: "parent-1", - message: "Do something", - initialTodos: [], - mode: "code", - pendingActionId: "create-action", - }) + 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" }) - }) + 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 = { @@ -525,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), @@ -705,59 +868,175 @@ 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), - }) + 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") + 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 === "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 === "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", + 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: expect.arrayContaining([ + { + type: "tool_result", + tool_use_id: pendingAction.actionId, + is_error: true, + content: expect.any(String), + }, + ]), + }) + } + }) + const getTaskWithId = vi.fn().mockResolvedValue({ historyItem }) - 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 taskHistoryStore = makeStoreStub({ + get: vi.fn().mockReturnValue(historyItem), + atomicReadAndUpdate: vi.fn().mockRejectedValue(persistError), + }) - 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 + 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" || scenario === "missing-tool-use") { + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() + } else { + expect(createTaskWithHistoryItem).toHaveBeenCalledExactlyOnceWith(historyItem) + } + expect(saveApiMessages).toHaveBeenCalledTimes( + 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({ + content: [{ content: "Subtask creation failed: parent metadata persist failed" }], + }) + } + }, + ) it("settles an interrupted parent's pending action when delegation is rejected", async () => { const pendingAction = { @@ -783,10 +1062,9 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { 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] - }), + // The disk-authoritative write can reject a stale cached parent even though + // interrupted-to-delegated is now a valid transition in the local reducer. + atomicReadAndUpdate: vi.fn().mockRejectedValue(new LifecycleTransitionError("Delegation rejected")), clearPendingActionIfMatching: vi.fn(async (_taskId: string, actionId: string) => { if (current.pendingAction?.kind === "create_subtask" && current.pendingAction.actionId === actionId) { current = { ...current, pendingAction: undefined } @@ -818,7 +1096,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { mode: pendingAction.mode, pendingActionId: pendingAction.actionId, }), - ).rejects.toThrow("Invalid task status transition: interrupted → delegated") + ).rejects.toThrow(LifecycleTransitionError) expect(current.pendingAction).toBeUndefined() expect(current.status).toBe("interrupted") @@ -844,16 +1122,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { getCurrentTask.mockReturnValue(child) return child }) - let transitionError: LifecycleTransitionError | undefined - const atomicReadAndUpdate = vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { - try { - updater(interruptedParent) - } catch (error) { - if (error instanceof LifecycleTransitionError) transitionError = error - throw error - } - return [] - }) + const transitionError = new LifecycleTransitionError("Delegation rejected") + const atomicReadAndUpdate = vi.fn().mockRejectedValue(transitionError) const clearPendingActionIfMatching = vi.fn() const deleteTaskWithId = vi.fn().mockResolvedValue(undefined) const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) @@ -921,7 +1191,9 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { const atomicReadAndUpdate = vi.fn().mockRejectedValue(persistenceError) const clearPendingActionIfMatching = vi.fn().mockResolvedValue(interruptedParent) const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) + vi.mocked(readApiMessages).mockResolvedValue([]) const provider = { + contextProxy: { globalStorageUri: { fsPath: "/storage" } }, taskScheduler: new TaskScheduler(), emit: vi.fn(), getCurrentTask, @@ -955,7 +1227,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(clearPendingActionIfMatching).not.toHaveBeenCalled() expect(interruptedParent.pendingAction).toBe(pendingAction) expect(provider.deleteTaskWithId).toHaveBeenCalledWith("child-1", false) - expect(createTaskWithHistoryItem).toHaveBeenCalledWith(interruptedParent) + // No owning tool call is available to persist a safe failure result. + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() }) it("does not restore a rejected pending action when its settlement write fails", async () => { @@ -980,10 +1253,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { getCurrentTask.mockReturnValue(child) return child }) - const atomicReadAndUpdate = vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { - updater(interruptedParent) - return [] - }) + const atomicReadAndUpdate = vi.fn().mockRejectedValue(new LifecycleTransitionError("Delegation rejected")) const clearPendingActionIfMatching = vi.fn().mockRejectedValue(settlementError) const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) const provider = { @@ -1014,7 +1284,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { mode: pendingAction.mode, pendingActionId: pendingAction.actionId, }), - ).rejects.toThrow("Invalid task status transition: interrupted → delegated") + ).rejects.toThrow(LifecycleTransitionError) expect(atomicReadAndUpdate).toHaveBeenCalledTimes(1) expect(clearPendingActionIfMatching).toHaveBeenCalledTimes(1) @@ -1065,10 +1335,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { taskHistoryStore: { invalidate: vi.fn().mockResolvedValue(undefined), get: vi.fn(() => interruptedParent), - atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { - updater(interruptedParent) - return [] - }), + atomicReadAndUpdate: vi.fn().mockRejectedValue(new LifecycleTransitionError("Delegation rejected")), clearPendingActionIfMatching, }, } as unknown as ClineProvider @@ -1081,7 +1348,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { mode: pendingAction.mode, pendingActionId: pendingAction.actionId, }), - ).rejects.toThrow("Invalid task status transition: interrupted → delegated") + ).rejects.toThrow(LifecycleTransitionError) expect(clearPendingActionIfMatching).toHaveBeenCalledWith("parent-1", pendingAction.actionId) expect(provider.deleteTaskWithId).toHaveBeenCalledWith("child-1", false) @@ -1132,10 +1399,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { taskHistoryStore: { invalidate: vi.fn().mockResolvedValue(undefined), get: vi.fn(() => cachedParent), - atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { - updater(cachedParent) - return [] - }), + atomicReadAndUpdate: vi.fn().mockRejectedValue(new LifecycleTransitionError("Delegation rejected")), clearPendingActionIfMatching, }, } as unknown as ClineProvider @@ -1148,7 +1412,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { mode: pendingAction.mode, pendingActionId: pendingAction.actionId, }), - ).rejects.toThrow("Invalid task status transition: interrupted → delegated") + ).rejects.toThrow(LifecycleTransitionError) expect(clearPendingActionIfMatching).toHaveBeenCalledTimes(1) expect(replacedParent.pendingAction).toEqual(replacementAction) @@ -1215,10 +1479,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { taskHistoryStore: { invalidate: vi.fn().mockResolvedValue(undefined), get: vi.fn(() => interruptedParent), - atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { - updater(interruptedParent) - return [] - }), + atomicReadAndUpdate: vi.fn().mockRejectedValue(new LifecycleTransitionError("Delegation rejected")), clearPendingActionIfMatching, deleteMany: vi.fn().mockResolvedValue(undefined), }, @@ -1233,7 +1494,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { mode: pendingAction.mode, pendingActionId: pendingAction.actionId, }), - ).rejects.toThrow("Invalid task status transition: interrupted → delegated") + ).rejects.toThrow(LifecycleTransitionError) // The child task directory was removed even though the child's // history lock failed and was swallowed, and the settled parent diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 346d697d9a..460a5f3548 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -769,17 +769,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) }) } @@ -889,10 +885,16 @@ 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 + 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 3e277ac867..5d8eee1cef 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts @@ -93,6 +93,52 @@ describe("TaskHistoryStore", () => { }) }) + describe("invalidate()", () => { + 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.toMatchObject( + scenario === "read-error" + ? { code: "EISDIR" } + : scenario === "malformed" + ? { name: "SyntaxError" } + : { message: expect.stringContaining("Invalid task history record") }, + ) + 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/task-persistence/__tests__/taskLifecycle.spec.ts b/src/core/task-persistence/__tests__/taskLifecycle.spec.ts index 5d3129e023..30ddb7ff13 100644 --- a/src/core/task-persistence/__tests__/taskLifecycle.spec.ts +++ b/src/core/task-persistence/__tests__/taskLifecycle.spec.ts @@ -35,6 +35,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", @@ -176,12 +208,12 @@ describe("settleRejectedCreateSubtaskAction", () => { expect(settleRejectedCreateSubtaskAction(parent, "create-action")).toBe(parent) }) - it("rejects an interrupted parent's delegation with a typed transition error", () => { - const parent = item("parent", { status: "interrupted", pendingAction: createSubtaskAction }) + it("rejects a completed parent's delegation with a typed transition error", () => { + const parent = item("parent", { status: "completed", pendingAction: createSubtaskAction }) expect(() => delegateTaskToChild(parent, "child")).toThrow(LifecycleTransitionError) expect(() => delegateTaskToChild(parent, "child")).toThrow( - "Invalid task status transition: interrupted → delegated", + "Invalid task status transition: completed → delegated", ) }) }) diff --git a/src/core/task-persistence/taskLifecycle.ts b/src/core/task-persistence/taskLifecycle.ts index b8a5e571f0..18deffd0ef 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: [], } @@ -45,8 +45,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/Task.ts b/src/core/task/Task.ts index 4de2b84590..bc58f687a6 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1071,8 +1071,9 @@ export class Task extends EventEmitter implements TaskLike { } /** - * An interrupted task cannot legally delegate, so a staged create-subtask - * action is a durable rejection marker rather than replayable work. The + * An interrupted task can delegate after live approval, but persisted actions + * cannot distinguish an unattempted approval from a rejected attempt whose + * settlement failed. Conservatively settle them instead of replaying. The * constructor-injected history item can be stale, so the persisted record * is refreshed first and the refreshed action is the one settled. A failed * refresh, lookup, or settlement stops replay instead of risking another @@ -3169,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 25e97f2d44..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" }, @@ -1405,6 +1421,58 @@ describe("Task persistence", () => { expect(mockSaveTaskMessages).not.toHaveBeenCalled() }) + it.each(["finish_subtask", "create_subtask"] as const)( + "reconciles an active task's 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: "active", + 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() + + expect(mockProvider.clearPendingTaskAction).toHaveBeenCalledWith("child-1", action.actionId) + expect(replay).not.toHaveBeenCalled() + expect(task.ask).toHaveBeenCalledWith("resume_task") + }, + ) + it("replays a create-subtask action for an active historical task without restart settlement", async () => { mockReadTaskMessages.mockResolvedValue([ { ts: 1, type: "ask", ask: "tool", text: createSubtaskAction.approvalText }, diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 874e2f8f08..330425d30a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3803,11 +3803,20 @@ 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. 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") @@ -3897,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) { @@ -3907,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. @@ -3941,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( @@ -3951,13 +3969,17 @@ 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: delegated.pendingAction?.actionId === pendingActionId ? undefined : delegated.pendingAction, } }) + delegationCommitted = true this.recentTasksCache = undefined if (this.isViewLaunched) { const updatedItem = this.taskHistoryStore.get(parentTaskId) @@ -3965,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) @@ -4002,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: ${ @@ -4019,12 +4062,69 @@ export class ClineProvider ) } try { - // A failed settlement write leaves the rejected pending action in - // durable storage. Restoring the stored parent would replay it in - // this process, so leave the parent unrestored. Restart recovery also - // settles interrupted create-subtask actions before allowing replay. - if (!settlementFailed) { + // Never restore a pending action whose authoritative settlement failed. + 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 + // 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) { + 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, + ), + ) + // 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" + ? [{ 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, 0, { + role: "user", + ts: Date.now(), + content: [result], + }) + } + await saveApiMessages({ + taskId: parentTaskId, + globalStoragePath, + merge: true, + messages, + }) + } + } await this.createTaskWithHistoryItem(parentHistory) } } catch (rollbackError) { @@ -4033,6 +4133,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 583485c628..4b46289b10 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": {