From 0cd7deabb5ba3a093bd5fdf38ca1292cc517f49e Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Wed, 29 Jul 2026 03:46:07 +0000 Subject: [PATCH 1/6] =?UTF-8?q?feat(TaskScheduler):=20fan-out=20=E2=80=94?= =?UTF-8?q?=20parent=20stays=20active=20while=20child=20runs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- apps/vscode-e2e/src/fixtures/subtasks.ts | 63 ++++ apps/vscode-e2e/src/suite/subtasks.test.ts | 60 ++++ packages/types/src/api.ts | 5 + src/__tests__/provider-delegation.spec.ts | 8 +- src/core/task/TaskScheduler.ts | 36 ++- .../__tests__/delegation-concurrent.spec.ts | 297 ++++++++++++++++++ src/core/webview/ClineProvider.ts | 135 ++++++-- src/eslint-suppressions.json | 2 +- src/extension/api.ts | 4 + src/utils/TaskSemaphore.ts | 20 ++ 10 files changed, 601 insertions(+), 29 deletions(-) create mode 100644 src/core/task/__tests__/delegation-concurrent.spec.ts diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index f5dfc813df..57955aa537 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -14,6 +14,8 @@ const SUBTASK_FAST_CHILD_MARKER = "SUBTASK_CHILD_IMMEDIATE_COMPLETION" 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" +const SUBTASK_FANOUT_PARENT_MARKER = "SUBTASK_PARENT_FANOUT_CONCURRENT" +const SUBTASK_FANOUT_CHILD_MARKER = "SUBTASK_CHILD_FANOUT_CONCURRENT" const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.` @@ -21,6 +23,10 @@ export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" export const SUBTASK_FAST_CHILD_RESULT = "Fast child completed" const SUBTASK_FAST_CHILD_PROMPT = `${SUBTASK_FAST_CHILD_MARKER}: Complete immediately with the exact result "${SUBTASK_FAST_CHILD_RESULT}".` export const SUBTASK_FAST_PARENT_PROMPT = `${SUBTASK_FAST_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FAST_CHILD_PROMPT}" Do not answer directly.` +export const SUBTASK_FANOUT_PARENT_FOLLOWUP = "Parent fan-out is still active?" +export const SUBTASK_FANOUT_CHILD_RESULT = "Fan-out child completed" +const SUBTASK_FANOUT_CHILD_PROMPT = `${SUBTASK_FANOUT_CHILD_MARKER}: Complete with the exact result "${SUBTASK_FANOUT_CHILD_RESULT}".` +export const SUBTASK_FANOUT_PARENT_PROMPT = `${SUBTASK_FANOUT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FANOUT_CHILD_PROMPT}" After delegation, ask the user exactly this follow-up question: ${SUBTASK_FANOUT_PARENT_FOLLOWUP}` const SUBTASK_INTERRUPT_CHILD_PROMPT = `${SUBTASK_INTERRUPT_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_INTERRUPT_PARENT_PROMPT = `${SUBTASK_INTERRUPT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_INTERRUPT_CHILD_PROMPT}" Do not answer directly. When the subtask returns, complete with the exact result "Interrupted parent resumed".` @@ -135,6 +141,63 @@ export function addSubtaskFixtures(mock: InstanceType) { }, }) + mock.addFixture({ + match: { + userMessage: new RegExp(SUBTASK_FANOUT_PARENT_MARKER), + sequenceIndex: 0, + }, + response: { + toolCalls: [ + { + name: "new_task", + arguments: JSON.stringify({ + mode: "ask", + message: SUBTASK_FANOUT_CHILD_PROMPT, + }), + id: "call_subtasks_fanout_parent_new_task_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + lastUserMessageContains(req, SUBTASK_FANOUT_CHILD_MARKER) && + !requestContains(req, [SUBTASK_FANOUT_PARENT_MARKER]), + }, + latency: 15_000, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_FANOUT_CHILD_RESULT }), + id: "call_subtasks_fanout_child_completion_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + requestContains(req, [SUBTASK_FANOUT_PARENT_MARKER, "Delegated to child task"]) && + !requestContains(req, [SUBTASK_RESULT_INJECTION]), + }, + response: { + toolCalls: [ + { + name: "ask_followup_question", + arguments: JSON.stringify({ + question: SUBTASK_FANOUT_PARENT_FOLLOWUP, + follow_up: [{ text: "continue" }], + }), + id: "call_subtasks_fanout_parent_followup_003", + }, + ], + }, + }) + // The parent prompt embeds SUBTASK_FAST_CHILD_MARKER verbatim, so parent-resume turns // can also match a bare substring check (same collision class as #561). Exclude the // parent marker so those turns fall through to the parent-resume fixture below. diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 9e1d0e83cb..af5e4aed8e 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -19,6 +19,8 @@ import { SUBTASK_API_HANG_RESUME_MESSAGE, SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_FAST_CHILD_RESULT, + SUBTASK_FANOUT_PARENT_FOLLOWUP, + SUBTASK_FANOUT_PARENT_PROMPT, SUBTASK_FAST_PARENT_PROMPT, SUBTASK_INTERRUPT_CHILD_FOLLOWUP_ANSWER, SUBTASK_INTERRUPT_PARENT_PROMPT, @@ -129,6 +131,64 @@ suite("Roo Code Subtasks", function () { } }) + test("fan-out keeps parent executing while child request is in flight", async () => { + const api = globalThis.api + const asks: Record = {} + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } + } + + api.on(RooCodeEventName.Message, messageHandler) + + try { + api.setTaskSchedulerMaxConcurrency(2) + + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_FANOUT_PARENT_PROMPT, + }) + + let childTaskId: string | undefined + await waitFor(() => { + const stack = api.getCurrentTaskStack() + const current = stack.at(-1) + if (current && current !== parentTaskId) { + childTaskId = current + return stack.includes(parentTaskId) + } + return false + }) + + await waitFor(() => + (asks[parentTaskId] ?? []).some( + ({ ask, text }) => ask === "followup" && text?.includes(SUBTASK_FANOUT_PARENT_FOLLOWUP), + ), + ) + + const stack = api.getCurrentTaskStack() + assert.ok(stack.includes(parentTaskId), "Fan-out parent should remain in the live task stack") + assert.ok(stack.includes(childTaskId!), "Fan-out child should remain in the live task stack") + assert.strictEqual(stack.at(-1), childTaskId, "Child should remain the focused task while parent runs") + } finally { + api.off(RooCodeEventName.Message, messageHandler) + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } + api.setTaskSchedulerMaxConcurrency(1) + await waitFor(() => api.getCurrentTaskStack().length === 0).catch(() => {}) + } + }) + // 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/packages/types/src/api.ts b/packages/types/src/api.ts index 89e9c8bc2b..07a730e6a1 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -56,6 +56,11 @@ export interface RooCodeAPI extends EventEmitter { * @returns An array of task IDs. */ getCurrentTaskStack(): string[] + /** + * Sets the TaskScheduler concurrency for extension-host tests. + * Intended for test/integration harnesses that need to exercise fan-out. + */ + setTaskSchedulerMaxConcurrency(maxConcurrency: number): void /** * Clears the current task. */ diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 0154027753..6e79cf2f29 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -369,6 +369,9 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { isViewLaunched: false, recentTasksCache: undefined, taskHistoryStore, + // Rollback looks up the just-created child by id to decide whether it + // still needs evicting, independent of current focus. + taskRegistry: { getById: vi.fn((id: string) => (id === "child-1" ? child : undefined)) }, } as unknown as ClineProvider await expect( @@ -381,8 +384,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { ).rejects.toThrow(persistError) expect(childRun).not.toHaveBeenCalled() + // 1st call (step 3): closes the parent to enforce the single-open invariant. expect(removeClineFromStack).toHaveBeenNthCalledWith(1) - expect(removeClineFromStack).toHaveBeenNthCalledWith(2) + // 2nd call (rollback): evicts the just-created child by id, regardless of + // current focus — see Story 3.2b fan-out rollback fix. + expect(removeClineFromStack).toHaveBeenNthCalledWith(2, "child-1") expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistoryItem) }) diff --git a/src/core/task/TaskScheduler.ts b/src/core/task/TaskScheduler.ts index 7431fb76ad..b0cbdcf979 100644 --- a/src/core/task/TaskScheduler.ts +++ b/src/core/task/TaskScheduler.ts @@ -10,8 +10,10 @@ import { type Task } from "./Task" */ export class TaskScheduler { private readonly sem: TaskSemaphore + readonly maxConcurrency: number constructor(maxConcurrency = 1) { + this.maxConcurrency = maxConcurrency this.sem = new TaskSemaphore(maxConcurrency) } @@ -19,6 +21,26 @@ export class TaskScheduler { return this.sem.waiting } + /** Number of permits not currently held by a running task. */ + get available(): number { + return this.sem.available + } + + /** + * Reserve a permit only if one is immediately free, without queueing. + * Returns a release function on success, or `undefined` if none was free. + * + * Use this (not `available > 0` followed later by `schedule()`) when a + * caller needs to make an irreversible decision — e.g. keeping a parent + * task alive for fan-out — based on whether a child can actually run + * concurrently. Checking `available` and then `await`-ing other work + * before calling `schedule()` leaves a window where another caller can + * consume the last permit; reserving it immediately closes that window. + */ + async tryReserve(): Promise<(() => void) | undefined> { + return this.sem.tryAcquire() + } + /** * Acquire a permit for `task`, call `run()`, and release on completion. * @@ -30,7 +52,19 @@ export class TaskScheduler { * without calling `run()`. */ async schedule(task: Task, run: () => Promise): Promise { - const release = await this.sem.acquire() + return this.runWithRelease(await this.sem.acquire(), task, run) + } + + /** + * Run `task` using a permit already obtained via `tryReserve()`, instead of + * acquiring a new one. Same abort/abandon and release-on-completion + * semantics as `schedule()`. + */ + async runWithReservation(release: () => void, task: Task, run: () => Promise): Promise { + return this.runWithRelease(release, task, run) + } + + private async runWithRelease(release: () => void, task: Task, run: () => Promise): Promise { if (task.abort || task.abandoned) { release() return diff --git a/src/core/task/__tests__/delegation-concurrent.spec.ts b/src/core/task/__tests__/delegation-concurrent.spec.ts new file mode 100644 index 0000000000..f28589657b --- /dev/null +++ b/src/core/task/__tests__/delegation-concurrent.spec.ts @@ -0,0 +1,297 @@ +// npx vitest run src/core/task/__tests__/delegation-concurrent.spec.ts + +import { describe, it, expect, vi, beforeEach } from "vitest" +import type { HistoryItem } from "@roo-code/types" + +import { ClineProvider } from "../../webview/ClineProvider" +import { TaskScheduler } from "../TaskScheduler" +import { TaskRegistry } from "../TaskRegistry" +import { type Task } from "../Task" +import { makeProviderStub } from "../../../__tests__/helpers/provider-stub" + +function makeParent(overrides: Record = {}): Task { + return { + taskId: "parent-1", + clineMessages: [] as unknown[], + flushPendingToolResultsToHistory: vi.fn().mockResolvedValue(true), + retrySaveApiConversationHistory: vi.fn().mockResolvedValue(true), + abort: false, + abandoned: false, + ...overrides, + } as unknown as Task +} + +function makeChild(overrides: Record = {}): Task { + return { + taskId: "child-1", + clineMessages: [] as unknown[], + abort: false, + abandoned: false, + run: vi.fn().mockResolvedValue(undefined), + ...overrides, + } as unknown as Task +} + +/** + * Real createTask() pushes the child onto taskRegistry (which also focuses + * it) before returning. Mirror that here so the fan-out branch has a real + * registry entry to operate on, same as production. + */ +function makeCreateTaskMock(provider: ClineProvider, child: Task) { + return vi.fn().mockImplementation(async () => { + ;(provider as unknown as { taskRegistry: { push: (t: Task) => void } }).taskRegistry.push(child) + return child + }) +} + +function baseStubFields(parent: Task) { + const atomicReadAndUpdate = vi.fn(async (_id: string, updater: (h: HistoryItem) => HistoryItem) => + updater({ id: "parent-1", status: "active", childIds: [] } as unknown as HistoryItem), + ) + return { + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getCurrentTask: vi.fn(() => parent), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + taskHistoryStore: { + get: vi.fn(() => undefined), + atomicReadAndUpdate, + }, + isViewLaunched: false, + emit: vi.fn(), + deleteTaskWithId: vi.fn().mockResolvedValue(undefined), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { id: "parent-1" } }), + createTaskWithHistoryItem: vi.fn().mockResolvedValue(parent), + } +} + +function getRunningTasks(provider: ClineProvider): Task[] { + return ClineProvider.prototype.getRunningTasks.call(provider) +} + +function callDelegate(provider: ClineProvider) { + return ( + ClineProvider.prototype as unknown as { + delegateParentAndOpenChild: (params: { + parentTaskId: string + message: string + initialTodos: never[] + mode: string + }) => Promise + } + ).delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "do work", + initialTodos: [], + mode: "code", + }) +} + +describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("maxConcurrency=1: removeClineFromStack is still called — existing behavior fully preserved", async () => { + const parent = makeParent() + const child = makeChild() + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const createTask = vi.fn().mockResolvedValue(child) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack, + createTask, + }) + + await callDelegate(provider) + + expect(removeClineFromStack).toHaveBeenCalledTimes(1) + }) + + it("maxConcurrency=2 with a free permit: parent stays active, fan-out path is taken", async () => { + const parent = makeParent() + const child = makeChild() + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(2), + removeClineFromStack, + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + + // Parent must not be suspended. + expect(removeClineFromStack).not.toHaveBeenCalled() + // UI focus moves to the child, parent remains in the registry. + const registry = (provider as unknown as { taskRegistry: TaskRegistry }).taskRegistry + expect(registry.current?.taskId).toBe("child-1") + expect(registry.getById("parent-1")).toBeDefined() + }) + + it("maxConcurrency=2 but no free permit: falls back to the suspending (maxConcurrency=1) path", async () => { + const parent = makeParent() + const child = makeChild() + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const createTask = vi.fn().mockResolvedValue(child) + + const scheduler = new TaskScheduler(2) + // Occupy both permits so none are free for fan-out. Don't await — + // the occupying "run" functions never resolve on purpose. + void scheduler.schedule(makeParent({ taskId: "occupant-1" }), () => new Promise(() => {})) + void scheduler.schedule(makeParent({ taskId: "occupant-2" }), () => new Promise(() => {})) + // Let both schedule() calls' internal sem.acquire() microtasks settle so + // both permits are actually held before we check availability. + await Promise.resolve() + await Promise.resolve() + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack, + createTask, + }) + + await callDelegate(provider) + + expect(removeClineFromStack).toHaveBeenCalledTimes(1) + }) + + it("fan-out path: both parent and child are tracked in the registry with no shared clineMessages reference", async () => { + const parent = makeParent({ clineMessages: [{ text: "parent msg" }] }) + const child = makeChild({ clineMessages: [{ text: "child msg" }] }) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(2), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + + const running = getRunningTasks(provider).map((t) => t.taskId) + expect(running).toContain("parent-1") + expect(running).toContain("child-1") + expect(parent.clineMessages).not.toBe(child.clineMessages) + }) + + it("fan-out path: the child's run() actually starts concurrently with the parent, not just the registry bookkeeping", async () => { + const parent = makeParent() + let parentResolve!: () => void + // The parent's own request loop is "in flight" (never resolves during + // this test) — provided as run() the way ClineProvider's real scheduler + // invocation for the parent would use it, to prove the child does not + // wait for the parent to finish. + const parentRun = vi.fn(() => new Promise((res) => (parentResolve = res))) + + let childStarted = false + let childResolve!: () => void + const child = makeChild({ + run: vi.fn().mockImplementation(() => { + childStarted = true + return new Promise((res) => (childResolve = res)) + }), + }) + + const scheduler = new TaskScheduler(2) + // Occupy one permit with the "parent's own request loop" so only one + // permit remains — exactly the scenario fan-out is meant to handle. + void scheduler.schedule(parent, parentRun) + await Promise.resolve() + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + // Let the reserved-permit run() invocation's microtask fire. + await Promise.resolve() + await Promise.resolve() + + expect(childStarted).toBe(true) + expect(child.run).toHaveBeenCalledTimes(1) + + childResolve() + parentResolve() + }) + + it("fan-out path: persisted parent history status is still 'delegated' (awaiting child), independent of the in-memory registry", async () => { + // Fan-out only changes whether the parent Task instance stays alive in + // TaskRegistry — it does not change what delegateParentAndOpenChild + // persists to TaskHistoryStore. The parent's HistoryItem status becomes + // "delegated" (awaiting the child) in both the fan-out and suspending + // paths; "both recorded as active simultaneously" is not the intended + // semantics here, even though the parent Task keeps running in memory. + const parent = makeParent() + const child = makeChild() + let persistedParent: HistoryItem | undefined + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(2), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + taskHistoryStore: { + get: vi.fn(() => undefined), + atomicReadAndUpdate: vi.fn(async (_id: string, updater: (h: HistoryItem) => HistoryItem) => { + persistedParent = updater({ + id: "parent-1", + status: "active", + childIds: [], + } as unknown as HistoryItem) + return persistedParent + }), + }, + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + + expect(persistedParent?.status).toBe("delegated") + expect(persistedParent?.awaitingChildId).toBe("child-1") + // But the parent Task instance itself is still live and running in the registry. + expect(getRunningTasks(provider).map((t) => t.taskId)).toContain("parent-1") + }) + + it("permit is reserved atomically at the fan-out decision — a concurrent delegation cannot steal the last free permit", async () => { + // maxConcurrency=2, one permit already held by an unrelated running task, + // leaving exactly one free — the contested permit. + const scheduler = new TaskScheduler(2) + void scheduler.schedule(makeParent({ taskId: "occupant" }), () => new Promise(() => {})) + await Promise.resolve() + expect(scheduler.available).toBe(1) + + // Two reservation attempts race for the single remaining permit. Because + // tryReserve() has no await before the underlying semaphore decrements + // its count, only one can win even though both observe available === 1 + // beforehand. + const [first, second] = await Promise.all([scheduler.tryReserve(), scheduler.tryReserve()]) + + const wins = [first, second].filter((r) => r !== undefined) + expect(wins).toHaveLength(1) + expect(scheduler.available).toBe(0) + }) + + it("getRunningTasks() exposes all concurrently active tasks", () => { + const parent = makeParent() + const child = makeChild() + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent, child], + taskScheduler: new TaskScheduler(2), + }) + + expect(getRunningTasks(provider).map((t) => t.taskId)).toEqual(["parent-1", "child-1"]) + }) +}) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 2ee92edebc..3d23bd67eb 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -160,6 +160,13 @@ function scheduleTask(scheduler: TaskScheduler, task: Task, source: string): voi .catch((error) => console.error(`[${source}] taskScheduler.schedule failed:`, error)) } +/** Run `task` using a permit already reserved via `TaskScheduler.tryReserve()`. */ +function runReservedTask(scheduler: TaskScheduler, release: () => void, task: Task, source: string): void { + void scheduler + .runWithReservation(release, task, () => task.run()) + .catch((error) => console.error(`[${source}] taskScheduler.runWithReservation failed:`, error)) +} + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -505,13 +512,17 @@ export class ClineProvider // Removes and destroys the top Cline instance (the current finished task), // activating the previous one (resuming the parent task). - async removeClineFromStack() { + // + // Pass `taskId` to evict a specific task regardless of focus (e.g. rollback + // cleanup of a just-created child after focus has already moved elsewhere). + // Defaults to the current task, preserving prior behavior. + async removeClineFromStack(taskId?: string) { if (this.taskRegistry.length === 0) { return } - // Remove the focused Cline instance from the stack. - let task = this.taskRegistry.current + const targetId = taskId ?? this.taskRegistry.current?.taskId + let task = targetId ? this.taskRegistry.getById(targetId) : undefined if (task) { task = this.taskRegistry.remove(task.taskId) } @@ -639,6 +650,17 @@ export class ClineProvider return this.taskRegistry.taskIds } + public setTaskSchedulerMaxConcurrency(maxConcurrency: number): void { + if (!Number.isInteger(maxConcurrency) || maxConcurrency < 1) { + throw new Error(`maxConcurrency must be a positive integer, got ${maxConcurrency}`) + } + if (this.taskRegistry.length > 0) { + throw new Error("Cannot change task scheduler concurrency while tasks are active") + } + this.taskScheduler.cancelQueued() + this.taskScheduler = new TaskScheduler(maxConcurrency) + } + // Pending Edit Operations Management /** @@ -1506,9 +1528,14 @@ export class ClineProvider /** * Handle switching to a new mode, including updating the associated API configuration * @param newMode The mode to switch to + * @param targetTask The task whose in-memory `_taskMode` should be updated to `newMode`. + * Defaults to the current task. Pass `null` to skip the per-task mutation and only + * apply the global mode/API-config side effects — used by delegation fan-out, where + * the "current" task is still the parent (which must keep its own mode) even though + * `newMode` is the child's requested mode. */ - public async handleModeSwitch(newMode: Mode) { - const task = this.getCurrentTask() + public async handleModeSwitch(newMode: Mode, targetTask: Task | null | undefined = this.getCurrentTask()) { + const task = targetTask if (task) { TelemetryService.instance.captureModeSwitch(task.taskId, newMode) @@ -3049,6 +3076,11 @@ export class ClineProvider return this.taskRegistry.current } + /** All tasks concurrently active in the registry (parent(s) plus any fanned-out children). */ + public getRunningTasks(): Task[] { + return this.taskRegistry.getRunning() + } + private logWebviewHiddenDiagnostics(): void { const task = this.getCurrentTask() if (!task || task.abort || task.abandoned) { @@ -3517,10 +3549,11 @@ export class ClineProvider /** * Delegate parent task and open child task. * - * - Enforce single-open invariant + * - Enforce single-open invariant, unless fan-out (maxConcurrency > 1 with a + * free permit) keeps the parent running alongside the child * - Persist parent delegation metadata * - Emit TaskDelegated (task-level; API forwards to provider/bridge) - * - Create child as sole active and switch mode to child's mode + * - Create and focus child, preserving parent reference for lineage, and switch mode to child's mode */ public async delegateParentAndOpenChild(params: { parentTaskId: string @@ -3576,11 +3609,28 @@ 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. + // 3) Enforce single-open invariant by closing/disposing the parent first — + // unless fan-out is enabled (maxConcurrency > 1) AND a permit can be + // reserved for the child right now, in which case the parent stays + // active on the registry and runs concurrently with the child. This + // path is only reachable via explicit TaskScheduler configuration; at + // the default maxConcurrency=1 it can never be taken. + // + // The permit is reserved here — not just checked via `available > 0` — + // because several awaits follow before the child actually starts + // running (mode switch, task creation, history persistence). Checking + // availability without reserving would leave a window where another + // delegation could consume the last permit, leaving the parent live + // but the child queued rather than concurrent. childReservedRelease is + // handed to the scheduler in step 6 instead of it acquiring its own. // Await abort completion to ensure clean disposal and prevent unhandled rejections. + const childReservedRelease = + this.taskScheduler.maxConcurrency > 1 ? await this.taskScheduler.tryReserve() : undefined + const fanOut = childReservedRelease !== undefined try { - await this.removeClineFromStack() + if (!fanOut) { + await this.removeClineFromStack() + } } catch (error) { this.log( `[delegateParentAndOpenChild] Error during parent disposal (non-fatal): ${ @@ -3594,8 +3644,17 @@ export class ClineProvider // This ensures the child's system prompt and configuration are based on the correct mode. // The mode switch must happen before createTask() because the Task constructor // initializes its mode from provider.getState() during initializeTaskMode(). + // In fan-out, the parent is still `getCurrentTask()` at this point (it hasn't been + // removed and the child doesn't exist yet) — pass `null` so handleModeSwitch applies + // the global mode/API-config side effects for the child's benefit without stomping + // the still-running parent's own `_taskMode`. + const requestedMode = mode as Mode try { - await this.handleModeSwitch(mode as any) + if (fanOut) { + await this.handleModeSwitch(requestedMode, null) + } else { + await this.handleModeSwitch(requestedMode) + } } catch (e) { this.log( `[delegateParentAndOpenChild] handleModeSwitch failed for mode '${mode}': ${ @@ -3604,7 +3663,9 @@ export class ClineProvider ) } - // 4) Create child as sole active (parent reference preserved for lineage) + // 4) Create and focus child, preserving parent reference for lineage. + // In the non-fan-out path the parent was already removed above, so the + // child is the sole active task; in fan-out both remain active. // Pass initialStatus: "active" to ensure the child task's historyItem is created // with status from the start, avoiding race conditions where the task might // call attempt_completion before status is persisted separately. @@ -3621,6 +3682,11 @@ export class ClineProvider startTask: false, }) + // createTask() -> addClineToStack() -> taskRegistry.push() already focuses + // the child. In the fan-out case the parent remains in the registry + // alongside it instead of being evicted, so both are now tracked with the + // child focused. + // 5) Persist parent delegation metadata BEFORE the child starts writing. // atomicReadAndUpdate reads from the in-memory cache and writes back within a // single lock acquisition — no concurrent writer can slip between the read and @@ -3683,10 +3749,11 @@ export class ClineProvider }`, ) try { - // Only pop the stack if the child we just created is still on top. - // A concurrent delegation could have pushed another child since we created ours. - if (this.getCurrentTask()?.taskId === child.taskId) { - await this.removeClineFromStack() + // Evict the child by id regardless of current focus — in fan-out (or if a + // concurrent delegation shifted focus), the child we just created may no + // longer be `current`, but it must still be removed from the registry. + if (this.taskRegistry.getById(child.taskId)) { + await this.removeClineFromStack(child.taskId) } } catch (cleanupError) { this.log( @@ -3704,21 +3771,37 @@ export class ClineProvider }`, ) } - try { - const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - await this.createTaskWithHistoryItem(parentHistory) - } catch (rollbackError) { - this.log( - `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback: ${ - (rollbackError as Error)?.message ?? String(rollbackError) - }`, - ) + // In the fan-out case the parent was never removed from the registry + // (it kept running throughout), so it must not be re-created here — + // doing so would push a duplicate parent Task instance. If the child was + // still focused, removeClineFromStack() above already re-focused the + // registry onto the parent (TaskRegistry.remove only reassigns focus + // when the removed task was current). + if (!fanOut) { + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + } catch (rollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback: ${ + (rollbackError as Error)?.message ?? String(rollbackError) + }`, + ) + } + } else { + // The child never reached step 6, so the reserved permit must be + // released here or it leaks for the lifetime of the scheduler. + childReservedRelease?.() } throw err } // 6) Start the child task now that parent metadata is safely persisted. - scheduleTask(this.taskScheduler, child, "delegateParentAndOpenChild") + if (childReservedRelease) { + runReservedTask(this.taskScheduler, childReservedRelease, child, "delegateParentAndOpenChild") + } else { + scheduleTask(this.taskScheduler, child, "delegateParentAndOpenChild") + } // 7) Emit TaskDelegated (provider-level) try { diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 608e190d04..89cdd46f63 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1076,7 +1076,7 @@ }, "core/webview/ClineProvider.ts": { "@typescript-eslint/no-explicit-any": { - "count": 12 + "count": 11 } }, "core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts": { diff --git a/src/extension/api.ts b/src/extension/api.ts index 2d0d5a6975..c50e29b700 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -253,6 +253,10 @@ export class API extends EventEmitter implements RooCodeAPI { return this.sidebarProvider.getCurrentTaskStack() } + public setTaskSchedulerMaxConcurrency(maxConcurrency: number): void { + this.sidebarProvider.setTaskSchedulerMaxConcurrency(maxConcurrency) + } + public async clearCurrentTask(_lastMessage?: string) { // Legacy finishSubTask removed; clear current by closing active task instance. await this.sidebarProvider.evictCurrentTask() diff --git a/src/utils/TaskSemaphore.ts b/src/utils/TaskSemaphore.ts index 15db11c2e1..188d4a98ba 100644 --- a/src/utils/TaskSemaphore.ts +++ b/src/utils/TaskSemaphore.ts @@ -36,6 +36,26 @@ export class TaskSemaphore { return this._waiting } + /** + * Reserve a permit only if one is immediately free; never queues. + * + * `async-mutex`'s `Semaphore.acquire()` resolves its permit synchronously + * (inside the Promise executor, before `acquire()` returns) whenever a + * permit is free at call time — the queue is only used when none is + * free. So `isLocked()` (== false means a permit is free) followed + * immediately by `acquire()`, with no `await` between them, cannot race: + * nothing else runs on the event loop in that gap. If `isLocked()` was + * false, the following `acquire()` is guaranteed not to queue. + * + * Returns `undefined` without side effects if no permit was free. + */ + async tryAcquire(): Promise<(() => void) | undefined> { + if (this.sem.isLocked()) { + return undefined + } + return this.acquire() + } + async acquire(): Promise<() => void> { // Only count as waiting if the permit won't be granted immediately. const willQueue = this.sem.isLocked() From fa561b18625d317be29b5758b7619f07476824bf Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Thu, 30 Jul 2026 00:59:35 +0000 Subject: [PATCH 2/6] fix(delegation): isolate parent/child provider state during fan-out --- apps/vscode-e2e/src/fixtures/subtasks.ts | 65 ++++++ apps/vscode-e2e/src/suite/subtasks.test.ts | 177 ++++++++++++++-- src/__tests__/helpers/provider-stub.ts | 3 + src/__tests__/provider-delegation.spec.ts | 20 ++ src/core/task/Task.ts | 40 ++-- src/core/task/__tests__/Task.spec.ts | 153 +++++++++++++- .../__tests__/delegation-concurrent.spec.ts | 172 ++++++++++++++- src/core/webview/ClineProvider.ts | 196 +++++++++++++++--- .../ClineProvider.apiHandlerRebuild.spec.ts | 156 +++++++++++++- src/eslint-suppressions.json | 2 +- 10 files changed, 909 insertions(+), 75 deletions(-) diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index 57955aa537..73cc4a8ff8 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -16,6 +16,8 @@ const SUBTASK_XPROFILE_SAME_CHILD_MARKER = "SUBTASK_CHILD_SAME_PROFILE" const SUBTASK_XPROFILE_DIFFERENT_CHILD_MARKER = "SUBTASK_CHILD_DIFFERENT_PROFILE" const SUBTASK_FANOUT_PARENT_MARKER = "SUBTASK_PARENT_FANOUT_CONCURRENT" const SUBTASK_FANOUT_CHILD_MARKER = "SUBTASK_CHILD_FANOUT_CONCURRENT" +const SUBTASK_FANOUT_XPROFILE_PARENT_MARKER = "SUBTASK_PARENT_FANOUT_CROSS_PROFILE" +const SUBTASK_FANOUT_XPROFILE_CHILD_MARKER = "SUBTASK_CHILD_FANOUT_CROSS_PROFILE" const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.` @@ -27,6 +29,12 @@ export const SUBTASK_FANOUT_PARENT_FOLLOWUP = "Parent fan-out is still active?" export const SUBTASK_FANOUT_CHILD_RESULT = "Fan-out child completed" const SUBTASK_FANOUT_CHILD_PROMPT = `${SUBTASK_FANOUT_CHILD_MARKER}: Complete with the exact result "${SUBTASK_FANOUT_CHILD_RESULT}".` export const SUBTASK_FANOUT_PARENT_PROMPT = `${SUBTASK_FANOUT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FANOUT_CHILD_PROMPT}" After delegation, ask the user exactly this follow-up question: ${SUBTASK_FANOUT_PARENT_FOLLOWUP}` +export const SUBTASK_FANOUT_XPROFILE_PARENT_MODEL = "openai/gpt-4.1" +export const SUBTASK_FANOUT_XPROFILE_CHILD_MODEL = "openai/gpt-4.1-mini" +export const SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP = "Parent cross-profile fan-out is still isolated?" +export const SUBTASK_FANOUT_XPROFILE_CHILD_RESULT = "Fan-out cross-profile child completed" +const SUBTASK_FANOUT_XPROFILE_CHILD_PROMPT = `${SUBTASK_FANOUT_XPROFILE_CHILD_MARKER}: Complete with the exact result "${SUBTASK_FANOUT_XPROFILE_CHILD_RESULT}".` +export const SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT = `${SUBTASK_FANOUT_XPROFILE_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FANOUT_XPROFILE_CHILD_PROMPT}" After delegation, ask the user exactly this follow-up question: ${SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP}` const SUBTASK_INTERRUPT_CHILD_PROMPT = `${SUBTASK_INTERRUPT_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.` export const SUBTASK_INTERRUPT_PARENT_PROMPT = `${SUBTASK_INTERRUPT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_INTERRUPT_CHILD_PROMPT}" Do not answer directly. When the subtask returns, complete with the exact result "Interrupted parent resumed".` @@ -198,6 +206,63 @@ export function addSubtaskFixtures(mock: InstanceType) { }, }) + mock.addFixture({ + match: { + userMessage: new RegExp(SUBTASK_FANOUT_XPROFILE_PARENT_MARKER), + sequenceIndex: 0, + }, + response: { + toolCalls: [ + { + name: "new_task", + arguments: JSON.stringify({ + mode: "ask", + message: SUBTASK_FANOUT_XPROFILE_CHILD_PROMPT, + }), + id: "call_subtasks_fanout_xprofile_parent_new_task_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + lastUserMessageContains(req, SUBTASK_FANOUT_XPROFILE_CHILD_MARKER) && + !requestContains(req, [SUBTASK_FANOUT_XPROFILE_PARENT_MARKER]), + }, + latency: 15_000, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_FANOUT_XPROFILE_CHILD_RESULT }), + id: "call_subtasks_fanout_xprofile_child_completion_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + requestContains(req, [SUBTASK_FANOUT_XPROFILE_PARENT_MARKER, "Delegated to child task"]) && + !requestContains(req, [SUBTASK_RESULT_INJECTION]), + }, + response: { + toolCalls: [ + { + name: "ask_followup_question", + arguments: JSON.stringify({ + question: SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP, + follow_up: [{ text: "continue" }], + }), + id: "call_subtasks_fanout_xprofile_parent_followup_003", + }, + ], + }, + }) + // The parent prompt embeds SUBTASK_FAST_CHILD_MARKER verbatim, so parent-resume turns // can also match a bare substring check (same collision class as #561). Exclude the // parent marker so those turns fall through to the parent-resume fixture below. diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index af5e4aed8e..2ff97f458d 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -21,6 +21,11 @@ import { SUBTASK_FAST_CHILD_RESULT, SUBTASK_FANOUT_PARENT_FOLLOWUP, SUBTASK_FANOUT_PARENT_PROMPT, + SUBTASK_FANOUT_XPROFILE_CHILD_MODEL, + SUBTASK_FANOUT_XPROFILE_CHILD_RESULT, + SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP, + SUBTASK_FANOUT_XPROFILE_PARENT_MODEL, + SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT, SUBTASK_FAST_PARENT_PROMPT, SUBTASK_INTERRUPT_CHILD_FOLLOWUP_ANSWER, SUBTASK_INTERRUPT_PARENT_PROMPT, @@ -36,6 +41,7 @@ type AimockMessageContent = string | Array<{ type?: string; text?: string }> type AimockJournalEntry = { body?: { + model?: string messages?: Array<{ role?: string content?: AimockMessageContent @@ -51,24 +57,55 @@ const messageContentText = (content?: AimockMessageContent) => { return content?.map((part) => part.text ?? "").join("") ?? "" } +const requestUserText = (entry: AimockJournalEntry) => + (entry.body?.messages ?? []) + .filter((message) => message.role === "user") + .map((message) => messageContentText(message.content)) + .join("") + const waitForAimockRequestContaining = async (expectedText: string, excludeText?: string) => { + await waitForAimockRequest((entry) => { + const messages = entry.body?.messages + if (!messages) return false + const entryText = messages.map((m) => messageContentText(m.content)).join("") + if (excludeText && entryText.includes(excludeText)) return false + return messages.some( + (message) => message.role === "user" && messageContentText(message.content).includes(expectedText), + ) + }) +} + +const waitForAimockRequest = async (matches: (entry: AimockJournalEntry) => boolean) => { + let latestEntries: AimockJournalEntry[] = [] + + try { + await waitFor(async () => { + latestEntries = await readAimockJournal() + return latestEntries.some(matches) + }) + } catch { + assertAimockRequest(latestEntries, matches) + } +} + +const readAimockJournal = async () => { const aimockUrl = process.env.AIMOCK_URL assert.ok(aimockUrl, "AIMOCK_URL must be set for aimock journal assertions") - await waitFor(async () => { - const response = await fetch(`${aimockUrl}/__aimock/journal`) - const entries = (await response.json()) as AimockJournalEntry[] - - return entries.some((entry) => { - const messages = entry.body?.messages - if (!messages) return false - const entryText = messages.map((m) => messageContentText(m.content)).join("") - if (excludeText && entryText.includes(excludeText)) return false - return messages.some( - (message) => message.role === "user" && messageContentText(message.content).includes(expectedText), - ) - }) - }) + const response = await fetch(`${aimockUrl}/__aimock/journal`) + return (await response.json()) as AimockJournalEntry[] +} + +const assertAimockRequest = (entries: AimockJournalEntry[], matches: (entry: AimockJournalEntry) => boolean) => { + if (entries.some(matches)) return + + const summary = entries.map((entry) => ({ + model: entry.body?.model, + userText: entry.body?.messages + ?.filter((message) => message.role === "user") + .map((message) => messageContentText(message.content).slice(0, 180)), + })) + assert.fail(`Expected aimock request was not found. Requests: ${JSON.stringify(summary, null, 2)}`) } suite("Roo Code Subtasks", function () { @@ -189,6 +226,118 @@ suite("Roo Code Subtasks", function () { } }) + test("fan-out keeps parent API config isolated when child switches to a different saved profile", async () => { + const api = globalThis.api + const asks: Record = {} + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } + } + + api.on(RooCodeEventName.Message, messageHandler) + + const aimockUrl = process.env.AIMOCK_URL + const parentProfile = { + apiProvider: "openrouter" as const, + openRouterApiKey: "mock-key", + openRouterModelId: SUBTASK_FANOUT_XPROFILE_PARENT_MODEL, + rateLimitSeconds: 0, + ...(aimockUrl && { openRouterBaseUrl: `${aimockUrl}/v1` }), + } + const childProfile = { + ...parentProfile, + openRouterModelId: SUBTASK_FANOUT_XPROFILE_CHILD_MODEL, + } + const priorModeApiConfigs = api.getConfiguration().modeApiConfigs ?? {} + const parentProfileId = await api.upsertProfile("subtask-fanout-parent-profile", parentProfile, true) + const childProfileId = await api.upsertProfile("subtask-fanout-child-profile", childProfile, false) + await api.setConfiguration({ + modeApiConfigs: { + ...priorModeApiConfigs, + code: parentProfileId!, + ask: childProfileId!, + }, + }) + + try { + api.setTaskSchedulerMaxConcurrency(2) + + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "code", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT, + }) + + let childTaskId: string | undefined + await waitFor(() => { + const stack = api.getCurrentTaskStack() + const current = stack.at(-1) + if (current && current !== parentTaskId) { + childTaskId = current + return stack.includes(parentTaskId) + } + return false + }) + + await waitFor(() => + (asks[parentTaskId] ?? []).some( + ({ ask, text }) => ask === "followup" && text?.includes(SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP), + ), + ) + + assert.ok( + (asks[parentTaskId] ?? []).some( + ({ ask, text }) => ask === "followup" && text?.includes(SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP), + ), + "Parent should keep running and ask its follow-up using its own profile", + ) + + const stack = api.getCurrentTaskStack() + assert.ok(stack.includes(parentTaskId), "Fan-out parent should remain in the live task stack") + assert.ok(stack.includes(childTaskId!), "Fan-out child should remain in the live task stack") + assert.strictEqual(stack.at(-1), childTaskId, "Child should remain the focused task while parent runs") + + await waitForAimockRequest((entry) => { + const text = requestUserText(entry) + const userMessageCount = (entry.body?.messages ?? []).filter( + (message) => message.role === "user", + ).length + return ( + entry.body?.model === SUBTASK_FANOUT_XPROFILE_PARENT_MODEL && + text.includes(SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT) && + userMessageCount > 1 + ) + }) + await waitForAimockRequest( + (entry) => + entry.body?.model === SUBTASK_FANOUT_XPROFILE_CHILD_MODEL && + (entry.body.messages ?? []).some( + (message) => + message.role === "user" && + messageContentText(message.content).includes(SUBTASK_FANOUT_XPROFILE_CHILD_RESULT), + ), + ) + } finally { + api.off(RooCodeEventName.Message, messageHandler) + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } + api.setTaskSchedulerMaxConcurrency(1) + await waitFor(() => api.getCurrentTaskStack().length === 0).catch(() => {}) + await api.setConfiguration({ modeApiConfigs: priorModeApiConfigs }) + await api.deleteProfile("subtask-fanout-child-profile").catch(() => {}) + await api.deleteProfile("subtask-fanout-parent-profile").catch(() => {}) + } + }) + // 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/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts index 59dde33933..85bd1a0b19 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -13,12 +13,14 @@ type ProviderStubFields = { runDelegationTransition?: unknown removeClineFromStack?: unknown evictCurrentTask?: unknown + restoreParentOrReleasePermit?: unknown } type PrivateProviderMethods = { runDelegationTransition: (this: unknown, ...args: unknown[]) => unknown removeClineFromStack: (this: unknown, ...args: unknown[]) => unknown evictCurrentTask: (this: unknown, ...args: unknown[]) => unknown + restoreParentOrReleasePermit: (this: unknown, ...args: unknown[]) => unknown } /** @@ -51,5 +53,6 @@ export function makeProviderStub(stub: T): ClineProvider { s.runDelegationTransition ??= proto.runDelegationTransition.bind(s) s.removeClineFromStack ??= proto.removeClineFromStack.bind(s) s.evictCurrentTask ??= proto.evictCurrentTask.bind(s) + s.restoreParentOrReleasePermit ??= proto.restoreParentOrReleasePermit.bind(s) return s as unknown as ClineProvider } diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 6e79cf2f29..8ac6670479 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -6,6 +6,24 @@ import { RooCodeEventName } from "@roo-code/types" import { ClineProvider } from "../core/webview/ClineProvider" import { TaskScheduler } from "../core/task/TaskScheduler" +/** + * restoreParentOrReleasePermit is a private prototype method that plain + * object-literal provider stubs don't have unless bound explicitly (same + * reason removeClineFromStack/evictCurrentTask need binding in provider-stub.ts). + */ +function bindRestoreParentOrReleasePermit(provider: ClineProvider): void { + type WithRestore = { + restoreParentOrReleasePermit: ( + parentTaskId: string, + fanOut: boolean, + childReservedRelease: (() => void) | undefined, + ) => Promise + } + ;(provider as unknown as WithRestore).restoreParentOrReleasePermit = ( + ClineProvider.prototype as unknown as WithRestore + ).restoreParentOrReleasePermit.bind(provider) +} + const parentHistoryItem: HistoryItem = { id: "parent-1", task: "Parent", @@ -318,6 +336,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindRestoreParentOrReleasePermit(provider) await expect( (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { @@ -373,6 +392,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { // still needs evicting, independent of current focus. taskRegistry: { getById: vi.fn((id: string) => (id === "child-1" ? child : undefined)) }, } as unknown as ClineProvider + bindRestoreParentOrReleasePermit(provider) await expect( (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 8f69a3a0d4..7e5c288197 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1591,7 +1591,10 @@ export class Task extends EventEmitter implements TaskLike { // Get condensing configuration const state = await this.providerRef.deref()?.getState() const customCondensingPrompt = state?.customSupportPrompts?.CONDENSE - const { mode, apiConfiguration } = state ?? {} + // mode/apiConfiguration come from this task's own fields, not shared + // provider state — see getSystemPrompt() for why. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration const { contextTokens: prevContextTokens } = this.getTokenUsage() @@ -3780,16 +3783,16 @@ export class Task extends EventEmitter implements TaskLike { const state = await this.providerRef.deref()?.getState() - const { - mode, - customModes, - customModePrompts, - customInstructions, - experiments, - language, - apiConfiguration, - enableSubfolderRules, - } = state ?? {} + const { customModes, customModePrompts, customInstructions, experiments, language, enableSubfolderRules } = + state ?? {} + + // mode and apiConfiguration are read from this task's own fields, not the + // shared provider state — provider.getState().mode/apiConfiguration reflect + // whichever task is currently focused, which during delegation fan-out can + // be a different, concurrently-running task. Reading global state here would + // leak the focused task's mode/config into this task's own system prompt. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration return await (async () => { const provider = this.providerRef.deref() @@ -3839,7 +3842,11 @@ export class Task extends EventEmitter implements TaskLike { private async handleContextWindowExceededError(): Promise { const state = await this.providerRef.deref()?.getState() - const { profileThresholds = {}, mode, apiConfiguration } = state ?? {} + const { profileThresholds = {} } = state ?? {} + // mode/apiConfiguration come from this task's own fields, not shared + // provider state — see getSystemPrompt() for why. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration const { contextTokens } = this.getTokenUsage() const modelInfo = this.api.getModel().info @@ -3978,9 +3985,10 @@ export class Task extends EventEmitter implements TaskLike { * the `api_req_rate_limit_wait` say type (not an error). */ private async maybeWaitForProviderRateLimit(retryAttempt: number): Promise { - const state = await this.providerRef.deref()?.getState() - const rateLimitSeconds = - state?.apiConfiguration?.rateLimitSeconds ?? this.apiConfiguration?.rateLimitSeconds ?? 0 + // This task's own apiConfiguration takes precedence over shared provider + // state, which during delegation fan-out reflects whichever task is + // currently focused — see getSystemPrompt() for the general rationale. + const rateLimitSeconds = this.apiConfiguration?.rateLimitSeconds ?? 0 const lastRequestTime = this.rateLimitClock.getLastRequestTime() if (rateLimitSeconds <= 0 || !lastRequestTime) { @@ -4432,7 +4440,7 @@ export class Task extends EventEmitter implements TaskLike { // Respect provider rate limit window let rateLimitDelay = 0 - const rateLimit = (state?.apiConfiguration ?? this.apiConfiguration)?.rateLimitSeconds || 0 + const rateLimit = this.apiConfiguration?.rateLimitSeconds || 0 const lastRequestTime = this.rateLimitClock.getLastRequestTime() if (lastRequestTime && rateLimit > 0) { const elapsed = performance.now() - lastRequestTime diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index d9f7240c5c..3e3005b5dd 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -16,6 +16,7 @@ import { import { TelemetryService } from "@roo-code/telemetry" import { Task } from "../Task" +import { SYSTEM_PROMPT } from "../../prompts/system" import { createRateLimitClock } from "../RateLimitClock" import { summarizeConversation } from "../../condense" import { ClineProvider } from "../../webview/ClineProvider" @@ -213,6 +214,15 @@ vi.mock("../../condense", async (importOriginal) => { }), } }) +// Spy on SYSTEM_PROMPT's mode argument without changing its behavior for tests +// that mock getSystemPrompt() at a higher level and never reach this call. +vi.mock("../../prompts/system", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + SYSTEM_PROMPT: vi.fn(actual.SYSTEM_PROMPT), + } +}) // Mock storagePathManager to prevent dynamic import issues. vi.mock("../../../utils/storage", () => ({ getTaskDirectoryPath: vi @@ -441,6 +451,65 @@ describe("Cline", () => { }) }) + describe("getSystemPrompt mode/apiConfiguration isolation", () => { + // Regression test for delegation fan-out (Story 3.2b): a parent task must + // keep using its own mode and apiConfiguration even while shared provider + // state has moved on to reflect a concurrently-running child (e.g. during + // delegateParentAndOpenChild's fan-out window, before the child registers + // itself as current). Reading provider.getState().mode/apiConfiguration + // directly — instead of the task's own fields — would leak the child's + // mode/config into the parent's next system prompt. + it("uses the task's own mode, not provider.getState().mode, when they diverge", async () => { + mockProvider.getState = vi.fn().mockResolvedValue({ mode: "architect" }) + + const cline = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + await cline.getTaskMode() + + // Simulate a concurrently-running child having switched shared provider + // state to a different mode (what handleModeSwitch does today). + mockProvider.getState = vi.fn().mockResolvedValue({ mode: "code" }) + + await getTaskTestAccess(cline).getSystemPrompt() + + expect(cline.taskMode).toBe("architect") + const [, , , , , modeArg] = vi.mocked(SYSTEM_PROMPT).mock.calls.at(-1)! + expect(modeArg).toBe("architect") + }) + + it("uses the task's own apiConfiguration, not provider.getState().apiConfiguration, when they diverge", async () => { + const parentApiConfig: ProviderSettings = { + ...mockApiConfig, + todoListEnabled: true, + } + + const cline = new Task({ + provider: mockProvider, + apiConfiguration: parentApiConfig, + task: "test task", + startTask: false, + }) + await cline.getTaskMode() + + // Simulate shared provider state now reflecting a different task's + // (the child's) apiConfiguration — opposite todoListEnabled value so + // a leak is directly observable in SYSTEM_PROMPT's settings argument. + mockProvider.getState = vi.fn().mockResolvedValue({ + mode: "code", + apiConfiguration: { ...mockApiConfig, todoListEnabled: false }, + }) + + await getTaskTestAccess(cline).getSystemPrompt() + + const [, , , , , , , , , , , , settingsArg] = vi.mocked(SYSTEM_PROMPT).mock.calls.at(-1)! + expect((settingsArg as { todoListEnabled?: boolean }).todoListEnabled).toBe(true) + }) + }) + describe("sayAndCreateMissingParamError", () => { it("surfaces a localized error notice and returns the missing-parameter tool error for both relPath branches", async () => { const cline = new Task({ @@ -681,7 +750,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() { @@ -700,7 +769,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() { @@ -772,7 +841,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() {}, @@ -788,7 +857,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() {}, @@ -820,6 +889,82 @@ describe("Cline", () => { expect(clock.getLastRequestTime()).toBeDefined() }) + it("uses the task's own rate limit for retry backoff when provider state belongs to another task", async () => { + const clock = createRateLimitClock() + const cline = new Task({ + provider: mockProvider, + apiConfiguration: { + ...mockApiConfig, + rateLimitSeconds: 4, + }, + task: "test task", + startTask: false, + rateLimitClock: clock, + }) + vi.spyOn(getTaskTestAccess(cline), "getSystemPrompt").mockResolvedValue("mock system prompt") + + const mockDelay = vi.fn().mockResolvedValue(undefined) + vi.spyOn(await import("delay"), "default").mockImplementation(mockDelay) + + const mockError = new Error("API Error") + const mockFailedStream = { + // eslint-disable-next-line require-yield + async *[Symbol.asyncIterator]() { + throw mockError + }, + async next() { + throw mockError + }, + async return() { + return { done: true, value: undefined } + }, + async throw(e: any) { + throw e + }, + async [Symbol.asyncDispose]() {}, + } as AsyncGenerator + + const mockSuccessStream = { + async *[Symbol.asyncIterator]() { + yield { type: "text", text: "Success" } + }, + async next() { + return { done: true, value: { type: "text", text: "Success" } } + }, + async return() { + return { done: true, value: undefined } + }, + async throw(e: any) { + throw e + }, + async [Symbol.asyncDispose]() {}, + } as AsyncGenerator + + let firstAttempt = true + vi.spyOn(cline.api, "createMessage").mockImplementation(() => { + if (firstAttempt) { + firstAttempt = false + return mockFailedStream + } + return mockSuccessStream + }) + const providerState = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + ...providerState, + apiConfiguration: { + ...mockApiConfig, + rateLimitSeconds: 10, + }, + autoApprovalEnabled: true, + requestDelaySeconds: 1, + }) + + const iterator = cline.attemptApiRequest(0) + await iterator.next() + + expect(mockDelay).toHaveBeenCalledTimes(4) + }) + it("should not apply retry delay twice", async () => { const cline = new Task({ provider: mockProvider, diff --git a/src/core/task/__tests__/delegation-concurrent.spec.ts b/src/core/task/__tests__/delegation-concurrent.spec.ts index f28589657b..c711997754 100644 --- a/src/core/task/__tests__/delegation-concurrent.spec.ts +++ b/src/core/task/__tests__/delegation-concurrent.spec.ts @@ -1,5 +1,6 @@ // npx vitest run src/core/task/__tests__/delegation-concurrent.spec.ts +import * as vscode from "vscode" import { describe, it, expect, vi, beforeEach } from "vitest" import type { HistoryItem } from "@roo-code/types" @@ -144,10 +145,11 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { // the occupying "run" functions never resolve on purpose. void scheduler.schedule(makeParent({ taskId: "occupant-1" }), () => new Promise(() => {})) void scheduler.schedule(makeParent({ taskId: "occupant-2" }), () => new Promise(() => {})) - // Let both schedule() calls' internal sem.acquire() microtasks settle so - // both permits are actually held before we check availability. - await Promise.resolve() - await Promise.resolve() + // Poll the deterministic signal instead of assuming a fixed microtask + // depth for sem.acquire() to settle. + while (scheduler.available > 0) { + await Promise.resolve() + } const provider = makeProviderStub({ ...baseStubFields(parent), @@ -162,6 +164,113 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { expect(removeClineFromStack).toHaveBeenCalledTimes(1) }) + it("createTask() throws in fan-out: the reserved permit is released, not leaked", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const scheduler = new TaskScheduler(2) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + }) + + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + // Permit must have been released, not leaked — a subsequent reservation + // attempt must succeed immediately. + expect(scheduler.available).toBe(2) + const release = await scheduler.tryReserve() + expect(release).toBeDefined() + }) + + it("handleModeSwitch() throws in fan-out: delegation aborts and releases the reserved permit before child creation", async () => { + const parent = makeParent() + const modeSwitchError = new Error("Provider profile mutation timed out") + const scheduler = new TaskScheduler(2) + const createTask = vi.fn() + + const provider = makeProviderStub({ + ...baseStubFields(parent), + handleModeSwitch: vi.fn().mockRejectedValue(modeSwitchError), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask, + }) + + await expect(callDelegate(provider)).rejects.toThrow(modeSwitchError) + + expect(createTask).not.toHaveBeenCalled() + expect(scheduler.available).toBe(2) + }) + + it("createTask() throws in the non-fan-out path: the evicted parent is restored", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const createTaskWithHistoryItem = vi.fn().mockResolvedValue(parent) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + createTaskWithHistoryItem, + }) + + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + expect(createTaskWithHistoryItem).toHaveBeenCalledWith({ id: "parent-1" }) + }) + + it("createTask() throws in the non-fan-out path: parent restore retries once after a restore failure", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const restoreError = new Error("restore failed once") + const createTaskWithHistoryItem = vi.fn().mockRejectedValueOnce(restoreError).mockResolvedValueOnce(parent) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + createTaskWithHistoryItem, + }) + + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + expect(createTaskWithHistoryItem).toHaveBeenCalledTimes(2) + expect(createTaskWithHistoryItem).toHaveBeenNthCalledWith(1, { id: "parent-1" }) + expect(createTaskWithHistoryItem).toHaveBeenNthCalledWith(2, { id: "parent-1" }) + }) + + it("createTask() throws in the non-fan-out path: parent restore reports an error after retry exhaustion", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const createTaskWithHistoryItem = vi.fn().mockRejectedValue(new Error("restore keeps failing")) + const showErrorMessage = vi.spyOn(vscode.window, "showErrorMessage").mockResolvedValue(undefined) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + createTaskWithHistoryItem, + }) + + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + expect(createTaskWithHistoryItem).toHaveBeenCalledTimes(2) + expect(showErrorMessage).toHaveBeenCalledWith( + "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", + ) + }) + it("fan-out path: both parent and child are tracked in the registry with no shared clineMessages reference", async () => { const parent = makeParent({ clineMessages: [{ text: "parent msg" }] }) const child = makeChild({ clineMessages: [{ text: "child msg" }] }) @@ -204,7 +313,9 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { // Occupy one permit with the "parent's own request loop" so only one // permit remains — exactly the scenario fan-out is meant to handle. void scheduler.schedule(parent, parentRun) - await Promise.resolve() + while (scheduler.available > 1) { + await Promise.resolve() + } const provider = makeProviderStub({ ...baseStubFields(parent), @@ -215,9 +326,11 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) await callDelegate(provider) - // Let the reserved-permit run() invocation's microtask fire. - await Promise.resolve() - await Promise.resolve() + // Poll the deterministic signal (the child's run() having actually been + // invoked) instead of assuming a fixed microtask depth. + for (let i = 0; i < 10 && !childStarted; i++) { + await Promise.resolve() + } expect(childStarted).toBe(true) expect(child.run).toHaveBeenCalledTimes(1) @@ -295,3 +408,46 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { expect(getRunningTasks(provider).map((t) => t.taskId)).toEqual(["parent-1", "child-1"]) }) }) + +describe("ClineProvider.setTaskSchedulerMaxConcurrency()", () => { + function setMaxConcurrency(provider: ClineProvider, maxConcurrency: number): void { + ClineProvider.prototype.setTaskSchedulerMaxConcurrency.call(provider, maxConcurrency) + } + + it("replaces the scheduler with one at the new maxConcurrency when no tasks are active", () => { + const cancelQueued = vi.fn() + const provider = makeProviderStub({ + tasks: [], + taskScheduler: { cancelQueued, maxConcurrency: 1 } as unknown as TaskScheduler, + }) + + setMaxConcurrency(provider, 2) + + expect(cancelQueued).toHaveBeenCalledTimes(1) + expect((provider as unknown as { taskScheduler: TaskScheduler }).taskScheduler.maxConcurrency).toBe(2) + }) + + it("throws and leaves the scheduler untouched if a task is active", () => { + const cancelQueued = vi.fn() + const originalScheduler = { cancelQueued, maxConcurrency: 1 } as unknown as TaskScheduler + const provider = makeProviderStub({ + tasks: [makeParent()], + taskScheduler: originalScheduler, + }) + + expect(() => setMaxConcurrency(provider, 2)).toThrow("Cannot change task scheduler concurrency") + expect(cancelQueued).not.toHaveBeenCalled() + expect((provider as unknown as { taskScheduler: TaskScheduler }).taskScheduler).toBe(originalScheduler) + }) + + it("rejects non-positive-integer values", () => { + const provider = makeProviderStub({ + tasks: [], + taskScheduler: new TaskScheduler(1), + }) + + expect(() => setMaxConcurrency(provider, 0)).toThrow("must be a positive integer") + expect(() => setMaxConcurrency(provider, 1.5)).toThrow("must be a positive integer") + expect(() => setMaxConcurrency(provider, -1)).toThrow("must be a positive integer") + }) +}) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3d23bd67eb..65ea9803fa 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -203,11 +203,37 @@ export class ClineProvider private globalStateWriteThroughTimer: ReturnType | null = null private static readonly GLOBAL_STATE_WRITE_THROUGH_DEBOUNCE_MS = 5000 // 5 seconds private static readonly PENDING_OPERATION_TIMEOUT_MS = 30000 // 30 seconds + private providerProfileMutationQueue = Promise.resolve() private runDelegationTransition(parentTaskId: string, fn: () => Promise): Promise { this.delegationTransitionLocks ??= new Map() return runDelegationTransition(this.delegationTransitionLocks, parentTaskId, fn) } + + private enqueueProviderProfileMutation(fn: () => Promise): Promise { + const run = this.providerProfileMutationQueue.then(fn, fn) + const callerResult = this.withProviderProfileMutationTimeout(run) + this.providerProfileMutationQueue = run.then( + () => undefined, + () => undefined, + ) + return callerResult + } + + private withProviderProfileMutationTimeout(operation: Promise): Promise { + let timeoutId: ReturnType | undefined + const timeout = new Promise((_, reject) => { + timeoutId = setTimeout(() => { + reject(new Error("Provider profile mutation timed out")) + }, ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + }) + + return Promise.race([operation, timeout]).finally(() => { + if (timeoutId) { + clearTimeout(timeoutId) + } + }) + } private readonly pendingEditOperations: PendingEditOperationStore private cloudOrganizationsCache: CloudOrganizationMembership[] | null = null @@ -1535,6 +1561,10 @@ export class ClineProvider * `newMode` is the child's requested mode. */ public async handleModeSwitch(newMode: Mode, targetTask: Task | null | undefined = this.getCurrentTask()) { + return this.enqueueProviderProfileMutation(() => this.handleModeSwitchUnlocked(newMode, targetTask)) + } + + private async handleModeSwitchUnlocked(newMode: Mode, targetTask: Task | null | undefined): Promise { const task = targetTask if (task) { @@ -1572,7 +1602,14 @@ export class ClineProvider // If workspace lock is on, keep the current API config — don't load mode-specific config const lockApiConfigAcrossModes = this.context.workspaceState.get("lockApiConfigAcrossModes", false) if (lockApiConfigAcrossModes) { - await this.postStateToWebview() + // Skip in fan-out (targetTask === null): postStateToWebview() reads + // getCurrentTask(), which is still the live parent here (the child + // doesn't exist yet) — pushing now would flash the parent's + // clineMessages/taskId to the webview as "current". The child's own + // registration moments later triggers the correct push. + if (targetTask !== null) { + await this.postStateToWebview() + } return } @@ -1598,7 +1635,18 @@ export class ClineProvider const hasActualSettings = !!fullProfile.apiProvider if (hasActualSettings) { - await this.activateProviderProfile({ name: profile.name }) + // targetTask === null means this call came from delegation fan-out + // (see handleModeSwitch's targetTask doc) — the parent is still + // "current" here and must not have its live API handler rebuilt + // to the child's mode's saved profile. + if (targetTask === null) { + await this.activateProviderProfileUnlocked( + { name: profile.name }, + { skipCurrentTaskRebuild: true }, + ) + } else { + await this.activateProviderProfileUnlocked({ name: profile.name }) + } } else { // The task will continue with the current/default configuration. } @@ -1618,7 +1666,9 @@ export class ClineProvider } } - await this.postStateToWebview() + if (targetTask !== null) { + await this.postStateToWebview() + } } // Provider Profile Management @@ -1634,8 +1684,9 @@ export class ClineProvider */ private updateTaskApiHandlerIfNeeded( providerSettings: ProviderSettings, - options: { forceRebuild?: boolean } = {}, + options: { forceRebuild?: boolean; skipCurrentTaskRebuild?: boolean } = {}, ): void { + if (options.skipCurrentTaskRebuild) return const task = this.getCurrentTask() if (!task) return @@ -1751,7 +1802,13 @@ export class ClineProvider await this.postStateToWebview() } - private async persistStickyProviderProfileToCurrentTask(apiConfigName: string): Promise { + private async persistStickyProviderProfileToCurrentTask( + apiConfigName: string, + options: { skipCurrentTaskRebuild?: boolean } = {}, + ): Promise { + if (options.skipCurrentTaskRebuild) { + return + } const task = this.getCurrentTask() if (!task) { return @@ -1781,12 +1838,35 @@ export class ClineProvider async activateProviderProfile( args: { name: string } | { id: string }, - options?: { persistModeConfig?: boolean; persistTaskHistory?: boolean }, + options?: { + persistModeConfig?: boolean + persistTaskHistory?: boolean + /** + * Skip rebuilding/mutating the current task's API handler and sticky + * profile. Used by delegation fan-out mode switches, where the + * "current" task is still the live parent (the child doesn't exist + * yet) and must not have its API configuration swapped to the + * child's mode's saved profile. + */ + skipCurrentTaskRebuild?: boolean + }, ) { + return this.enqueueProviderProfileMutation(() => this.activateProviderProfileUnlocked(args, options)) + } + + private async activateProviderProfileUnlocked( + args: { name: string } | { id: string }, + options?: { + persistModeConfig?: boolean + persistTaskHistory?: boolean + skipCurrentTaskRebuild?: boolean + }, + ): Promise { const { name, id, ...providerSettings } = await this.providerSettingsManager.activateProfile(args) const persistModeConfig = options?.persistModeConfig ?? true const persistTaskHistory = options?.persistTaskHistory ?? true + const skipCurrentTaskRebuild = options?.skipCurrentTaskRebuild ?? false // See `upsertProviderProfile` for a description of what this is doing. await Promise.all([ @@ -1802,17 +1882,27 @@ export class ClineProvider } // Change the provider for the current task. - this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) + this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true, skipCurrentTaskRebuild }) // Update the current task's sticky provider profile, unless this activation is // being used purely as a non-persisting restoration (e.g., reopening a task from history). if (persistTaskHistory) { - await this.persistStickyProviderProfileToCurrentTask(name) + await this.persistStickyProviderProfileToCurrentTask(name, { skipCurrentTaskRebuild }) } - await this.postStateToWebview() + // Same fan-out hazard as handleModeSwitch: postStateToWebview() reads + // getCurrentTask(), which is still the live parent while skipCurrentTaskRebuild + // is set, so pushing here would flash the parent's clineMessages/taskId to + // the webview as "current" a moment before the child registers itself. + if (!skipCurrentTaskRebuild) { + await this.postStateToWebview() + } - if (providerSettings.apiProvider) { + // In fan-out, ProviderProfileChanged would be received by the still-running + // parent task and make it reload the child's provider settings through its + // own listener. The child is created from the updated provider state, so no + // live-task event is needed for that path. + if (providerSettings.apiProvider && !skipCurrentTaskRebuild) { this.emit(RooCodeEventName.ProviderProfileChanged, { name, provider: providerSettings.apiProvider }) } } @@ -3546,6 +3636,48 @@ export class ClineProvider return this.currentWorkspacePath || getWorkspacePath() } + /** + * Undo the "parent kept running" (fan-out) or "parent evicted" (non-fan-out) + * side effect from step 3 of `delegateParentAndOpenChild`, for failures that + * happen before a child exists to attach lineage to. Shared by the + * `createTask()` failure path and the metadata-persistence failure path. + */ + private async restoreParentOrReleasePermit( + parentTaskId: string, + fanOut: boolean, + childReservedRelease: (() => void) | undefined, + ): Promise { + if (!fanOut) { + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + } catch (firstRollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback, retrying once: ${ + (firstRollbackError as Error)?.message ?? String(firstRollbackError) + }`, + ) + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + } catch (rollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback retry: ${ + (rollbackError as Error)?.message ?? String(rollbackError) + }`, + ) + vscode.window.showErrorMessage( + "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", + ) + } + } + } else { + // The child never reached step 6, so the reserved permit must be + // released here or it leaks for the lifetime of the scheduler. + childReservedRelease?.() + } + } + /** * Delegate parent task and open child task. * @@ -3661,6 +3793,10 @@ export class ClineProvider (e as Error)?.message ?? String(e) }`, ) + if (fanOut) { + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) + throw e + } } // 4) Create and focus child, preserving parent reference for lineage. @@ -3676,11 +3812,24 @@ export class ClineProvider // Without this, the child's fire-and-forget startTask() races with step 5, // and the last writer to globalState overwrites the other's changes— // causing the parent's delegation fields to be lost. - const child = await this.createTask(message, undefined, parent as any, { - initialTodos, - initialStatus: "active", - startTask: false, - }) + let child: Task + try { + child = await this.createTask(message, undefined, parent as any, { + initialTodos, + initialStatus: "active", + startTask: false, + }) + } catch (err) { + this.log( + `[delegateParentAndOpenChild] createTask failed for parent ${parentTaskId}: ${ + (err as Error)?.message ?? String(err) + }`, + ) + // No child was created, so there is no lineage to unwind — just undo + // step 3's parent-eviction (or release the reserved permit in fan-out). + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) + throw err + } // createTask() -> addClineToStack() -> taskRegistry.push() already focuses // the child. In the fan-out case the parent remains in the registry @@ -3777,22 +3926,7 @@ export class ClineProvider // still focused, removeClineFromStack() above already re-focused the // registry onto the parent (TaskRegistry.remove only reassigns focus // when the removed task was current). - if (!fanOut) { - try { - const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - await this.createTaskWithHistoryItem(parentHistory) - } catch (rollbackError) { - this.log( - `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback: ${ - (rollbackError as Error)?.message ?? String(rollbackError) - }`, - ) - } - } else { - // The child never reached step 6, so the reserved permit must be - // released here or it leaks for the lifetime of the scheduler. - childReservedRelease?.() - } + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) throw err } diff --git a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts index 3c777d0fc0..fa02ff6b5d 100644 --- a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts @@ -3,9 +3,10 @@ import * as vscode from "vscode" import { TelemetryService } from "@roo-code/telemetry" -import { getModelId } from "@roo-code/types" +import { getModelId, RooCodeEventName } from "@roo-code/types" import { ContextProxy } from "../../config/ContextProxy" +import type { Mode } from "../../../shared/modes" import { Task, TaskOptions } from "../../task/Task" import { ClineProvider } from "../ClineProvider" @@ -235,6 +236,7 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { { name: "test-config", id: "test-id", apiProvider: "openrouter", modelId: "openai/gpt-4" }, ]), setModeConfig: vi.fn(), + getModeConfigId: vi.fn().mockResolvedValue(undefined), activateProfile: vi.fn().mockResolvedValue({ name: "test-config", id: "test-id", @@ -410,6 +412,158 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { }) describe("activateProviderProfile", () => { + test("serializes provider profile mutations without interleaving", async () => { + const events: string[] = [] + let resolveFirst!: () => void + + provider["providerSettingsManager"].activateProfile = vi + .fn() + .mockImplementationOnce(async () => { + events.push("first:start") + await new Promise((resolve) => { + resolveFirst = resolve + }) + events.push("first:end") + return { + name: "first-profile", + id: "first-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + } + }) + .mockImplementationOnce(async () => { + events.push("second:start") + return { + name: "second-profile", + id: "second-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + } + }) + + const first = provider.activateProviderProfile({ name: "first-profile" }) + const second = provider.activateProviderProfile({ name: "second-profile" }) + + await Promise.resolve() + expect(events).toEqual(["first:start"]) + + resolveFirst() + await first + await second + + expect(events).toEqual(["first:start", "first:end", "second:start"]) + }) + + test("provider profile mutation rejection does not poison later queued mutations", async () => { + const firstError = new Error("first profile failed") + + provider["providerSettingsManager"].activateProfile = vi + .fn() + .mockRejectedValueOnce(firstError) + .mockResolvedValueOnce({ + name: "second-profile", + id: "second-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + }) + + await expect(provider.activateProviderProfile({ name: "first-profile" })).rejects.toThrow(firstError) + await expect(provider.activateProviderProfile({ name: "second-profile" })).resolves.toBeUndefined() + }) + + test("provider profile mutation timeout does not advance the queue until the timed-out mutation settles", async () => { + vi.useFakeTimers() + try { + const events: string[] = [] + let resolveFirst!: () => void + provider["providerSettingsManager"].activateProfile = vi + .fn() + .mockImplementationOnce(async () => { + events.push("first:start") + await new Promise((resolve) => { + resolveFirst = resolve + }) + events.push("first:end") + return { + name: "stuck-profile", + id: "stuck-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + } + }) + .mockImplementationOnce(async () => { + events.push("second:start") + return { + name: "second-profile", + id: "second-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + } + }) + + const first = provider.activateProviderProfile({ name: "stuck-profile" }) + const firstResult = first.catch((error: unknown) => error) + await Promise.resolve() + expect(events).toEqual(["first:start"]) + + await vi.advanceTimersByTimeAsync(30_000) + + expect(await firstResult).toEqual(new Error("Provider profile mutation timed out")) + const second = provider.activateProviderProfile({ name: "second-profile" }) + await Promise.resolve() + expect(events).toEqual(["first:start"]) + + resolveFirst() + await second + expect(events).toEqual(["first:start", "first:end", "second:start"]) + } finally { + vi.useRealTimers() + } + }) + + test("skipCurrentTaskRebuild does not rebuild current task or emit profile-change event", async () => { + const mockTask = new Task({ + ...defaultTaskOptions, + apiConfiguration: { + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + }, + }) + await provider.addClineToStack(mockTask) + + provider["providerSettingsManager"].activateProfile = vi.fn().mockResolvedValue({ + name: "ask-profile", + id: "ask-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + }) + const emitSpy = vi.spyOn(provider, "emit") + + await provider.activateProviderProfile({ name: "ask-profile" }, { skipCurrentTaskRebuild: true }) + + expect(mockTask.updateApiConfiguration).not.toHaveBeenCalled() + expect(emitSpy).not.toHaveBeenCalledWith( + RooCodeEventName.ProviderProfileChanged, + expect.objectContaining({ name: "ask-profile" }), + ) + }) + + test("fan-out mode switch does not post the current parent state when no saved profile is activated", async () => { + const mockTask = new Task({ + ...defaultTaskOptions, + apiConfiguration: { + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + }, + }) + await provider.addClineToStack(mockTask) + const postStateSpy = vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + + await provider.handleModeSwitch("ask" as Mode, null) + + expect(postStateSpy).not.toHaveBeenCalled() + }) + test("calls updateApiConfiguration when provider/model unchanged but settings differ (explicit profile switch)", async () => { const mockTask = new Task({ ...defaultTaskOptions, diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 89cdd46f63..8e4d671079 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -856,7 +856,7 @@ }, "core/task/__tests__/Task.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 31 + "count": 29 } }, "core/task/__tests__/Task.sticky-profile-race.spec.ts": { From 6d5d0fb214aba697c2848d2f9ecc17d34059c388 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Thu, 30 Jul 2026 01:00:05 +0000 Subject: [PATCH 3/6] test(delegation): add TLA+ formal model + drift-monitor spec --- docs/formal/task-delegation/README.md | 84 ++++ .../formal/task-delegation/TaskDelegation.cfg | 15 + .../formal/task-delegation/TaskDelegation.tla | 208 ++++++++++ .../delegation-state-machine.spec.ts | 359 ++++++++++++++++++ 4 files changed, 666 insertions(+) create mode 100644 docs/formal/task-delegation/README.md create mode 100644 docs/formal/task-delegation/TaskDelegation.cfg create mode 100644 docs/formal/task-delegation/TaskDelegation.tla create mode 100644 src/core/task/__tests__/delegation-state-machine.spec.ts diff --git a/docs/formal/task-delegation/README.md b/docs/formal/task-delegation/README.md new file mode 100644 index 0000000000..6632c09622 --- /dev/null +++ b/docs/formal/task-delegation/README.md @@ -0,0 +1,84 @@ +# Task Delegation Formal Model + +This directory contains a small TLA+ model for the task-delegation state machine. + +The model is intentionally abstract. It does not model VS Code, webviews, prompt contents, provider implementations, or task message history. It only models the state that must remain consistent while a parent task delegates work to a child task: + +- parent task liveness while a child is created +- child focus after creation +- semaphore reservation and release +- rollback after child creation or history persistence failures +- parent and child API profile isolation +- parent history status after delegation + +## Local Checks + +The executable CI check lives in `src/core/task/__tests__/delegation-state-machine.spec.ts`. It exhaustively explores the same small state machine with Vitest and includes explicit negative controls for the highest-risk invariant violations: + +- broadcasting a child provider-profile change to the live parent +- leaking a reserved permit after failed child creation +- failing to restore a suspended parent after serial child creation fails + +Run it with: + +```bash +pnpm --dir src exec vitest run core/task/__tests__/delegation-state-machine.spec.ts +``` + +## TLA+ Checks + +The TLA+ spec can be checked with TLC from the TLA+ Toolbox, the VS Code TLA+ extension, or a CLI TLC installation: + +```bash +java -cp /path/to/tla2tools.jar tlc2.TLC TaskDelegation.tla -config TaskDelegation.cfg +``` + +The main invariants are: + +- `PermitBound`: held plus reserved permits never exceed the configured concurrency. +- `ParentProfileIsolation`: a live parent keeps its original API profile. +- `ChildProfileIsolation`: a live child uses the child API profile. +- `RollbackRestoresParent`: failed delegation releases reservations and returns focus to the parent. +- `RunningChildHasDelegatedParent`: a running child implies the parent history item is delegated. + +## Drift Control + +The Vitest model is the primary drift monitor because it runs with the normal test suite. If production delegation behavior changes, update the Vitest model and this TLA+ model in the same change. + +Reviewers should check this mapping when touching delegation code: + +- `TaskScheduler.tryReserve()` / `runWithReservation()` map to the `reservedPermit` and `permitsHeld` transitions. +- `delegateParentAndOpenChild()` maps to the parent liveness, child creation, focus, rollback, and history transitions. +- provider-profile switching maps to `globalProfile`, `parentLocalProfile`, and `childLocalProfile`. +- task-history updates map to `parentHistoryStatus` and `childHistoryStatus`. + +Run TLC locally for changes that alter delegation ordering, rollback, profile switching, or scheduler permits. For smaller implementation-only changes, the Vitest model plus targeted unit/e2e coverage is usually sufficient. + +## State-Machine Review Checklist + +Use this checklist for changes that touch delegation, task focus, scheduler permits, provider profile or mode switching, task-local API configuration, or rollback behavior. + +Before coding, state the invariant the change is preserving. Example: "provider/profile mutations are serialized; a timed-out caller must not allow a later mutation to overtake an earlier one." + +For every async or mutating fix, answer: + +- Does a timeout cancel the work, or only stop waiting for it? +- If timed-out work can still complete later, can it mutate state after a newer operation? +- Can a later mode/profile mutation overtake an earlier one? +- Does rejection poison the queue, skip queued work, or create an unhandled rejection? +- If a required setup step fails, does delegation abort or continue? +- If rollback fails, what live task, focused task, history status, and reserved permits remain? +- Does rollback route through the same queue or lock that may already be blocked? +- Does any code read shared provider state where task-local state is required? +- Does any webview push re-derive the focused task when the operation is about a different task? + +Required tests for these changes: + +- A positive test for the intended path. +- A negative test that would fail with the original bug. +- A fix-interaction test that would fail if the fix introduces a stale completion, queue overtake, leaked permit, or rollback-of-rollback failure. + +Any timeout around a mutating async operation must prove one of these two properties: + +- the underlying operation is actually cancelled before later conflicting mutations can run; or +- later conflicting mutations remain queued until the timed-out operation truly settles. diff --git a/docs/formal/task-delegation/TaskDelegation.cfg b/docs/formal/task-delegation/TaskDelegation.cfg new file mode 100644 index 0000000000..0e12afd0a8 --- /dev/null +++ b/docs/formal/task-delegation/TaskDelegation.cfg @@ -0,0 +1,15 @@ +SPECIFICATION Spec + +CONSTANTS + ParentProfile = parent_profile + ChildProfile = child_profile + +INVARIANT PermitBound +INVARIANT NonNegativePermits +INVARIANT ParentProfileIsolation +INVARIANT ChildProfileIsolation +INVARIANT RollbackRestoresParent +INVARIANT RunningChildHasDelegatedParent +INVARIANT FocusReferencesLiveTask + +CHECK_DEADLOCK FALSE diff --git a/docs/formal/task-delegation/TaskDelegation.tla b/docs/formal/task-delegation/TaskDelegation.tla new file mode 100644 index 0000000000..643b7ae279 --- /dev/null +++ b/docs/formal/task-delegation/TaskDelegation.tla @@ -0,0 +1,208 @@ +---- MODULE TaskDelegation ---- +EXTENDS Naturals, TLC + +CONSTANTS ParentProfile, ChildProfile + +Profiles == { ParentProfile, ChildProfile } +FocusValues == { "parent", "child", "none" } +Phases == { + "parent-running", + "parent-suspended", + "permit-reserved", + "child-profile-selected", + "child-created", + "delegation-persisted", + "child-running", + "child-completed", + "failed" +} +HistoryStatuses == { "active", "delegated", "completed" } + +VARIABLES + phase, + maxConcurrency, + permitsHeld, + reservedPermit, + focus, + globalProfile, + parentLive, + parentLocalProfile, + parentHistoryStatus, + childLive, + childLocalProfile, + childHistoryStatus + +vars == << + phase, + maxConcurrency, + permitsHeld, + reservedPermit, + focus, + globalProfile, + parentLive, + parentLocalProfile, + parentHistoryStatus, + childLive, + childLocalProfile, + childHistoryStatus +>> + +Init == + /\ phase = "parent-running" + /\ maxConcurrency \in { 1, 2 } + /\ permitsHeld = IF maxConcurrency = 2 THEN 1 ELSE 0 + /\ reservedPermit = FALSE + /\ focus = "parent" + /\ globalProfile = ParentProfile + /\ parentLive = TRUE + /\ parentLocalProfile = ParentProfile + /\ parentHistoryStatus = "active" + /\ childLive = FALSE + /\ childLocalProfile = ChildProfile + /\ childHistoryStatus = "active" + +ActivePermits == permitsHeld + IF reservedPermit THEN 1 ELSE 0 + +ReserveFanOutPermit == + /\ phase = "parent-running" + /\ maxConcurrency > 1 + /\ ActivePermits < maxConcurrency + /\ phase' = "permit-reserved" + /\ reservedPermit' = TRUE + /\ UNCHANGED << maxConcurrency, permitsHeld, focus, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus, + childLive, childLocalProfile, childHistoryStatus >> + +SuspendParentForSerialDelegation == + /\ phase = "parent-running" + /\ maxConcurrency = 1 + /\ phase' = "parent-suspended" + /\ parentLive' = FALSE + /\ focus' = "none" + /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, globalProfile, parentLocalProfile, parentHistoryStatus, + childLive, childLocalProfile, childHistoryStatus >> + +CreateChildAfterSuspendSucceeds == + /\ phase = "parent-suspended" + /\ phase' = "child-created" + /\ focus' = "child" + /\ globalProfile' = ChildProfile + /\ childLive' = TRUE + /\ childLocalProfile' = ChildProfile + /\ childHistoryStatus' = "active" + /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, parentLive, parentLocalProfile, parentHistoryStatus >> + +CreateChildAfterSuspendFails == + /\ phase = "parent-suspended" + /\ phase' = "failed" + /\ parentLive' = TRUE + /\ focus' = "parent" + /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, globalProfile, parentLocalProfile, parentHistoryStatus, + childLive, childLocalProfile, childHistoryStatus >> + +SelectChildProfile == + /\ phase = "permit-reserved" + /\ phase' = "child-profile-selected" + /\ globalProfile' = ChildProfile + /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, focus, parentLive, parentLocalProfile, parentHistoryStatus, + childLive, childLocalProfile, childHistoryStatus >> + +CreateChildSucceeds == + /\ phase = "child-profile-selected" + /\ phase' = "child-created" + /\ focus' = "child" + /\ childLive' = TRUE + /\ childLocalProfile' = globalProfile + /\ childHistoryStatus' = "active" + /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus >> + +CreateChildFails == + /\ phase = "child-profile-selected" + /\ phase' = "failed" + /\ reservedPermit' = FALSE + /\ focus' = "parent" + /\ globalProfile' = ParentProfile + /\ childLive' = FALSE + /\ childLocalProfile' = ChildProfile + /\ childHistoryStatus' = "active" + /\ UNCHANGED << maxConcurrency, permitsHeld, parentLive, parentLocalProfile, parentHistoryStatus >> + +PersistDelegationSucceeds == + /\ phase = "child-created" + /\ phase' = "delegation-persisted" + /\ parentHistoryStatus' = "delegated" + /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, focus, globalProfile, parentLive, parentLocalProfile, + childLive, childLocalProfile, childHistoryStatus >> + +PersistDelegationFails == + /\ phase = "child-created" + /\ phase' = "failed" + /\ reservedPermit' = FALSE + /\ focus' = "parent" + /\ globalProfile' = ParentProfile + /\ parentLive' = TRUE + /\ childLive' = FALSE + /\ childLocalProfile' = ChildProfile + /\ childHistoryStatus' = "active" + /\ UNCHANGED << maxConcurrency, permitsHeld, parentLocalProfile, parentHistoryStatus >> + +StartChildWithReservation == + /\ phase = "delegation-persisted" + /\ reservedPermit = TRUE + /\ phase' = "child-running" + /\ reservedPermit' = FALSE + /\ permitsHeld' = permitsHeld + 1 + /\ UNCHANGED << maxConcurrency, focus, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus, + childLive, childLocalProfile, childHistoryStatus >> + +ParentContinues == + /\ phase = "child-running" + /\ UNCHANGED vars + +ChildUsesScopedProfile == + /\ phase = "child-running" + /\ UNCHANGED vars + +ChildCompletes == + /\ phase = "child-running" + /\ phase' = "child-completed" + /\ permitsHeld' = permitsHeld - 1 + /\ focus' = "parent" + /\ childLive' = FALSE + /\ childHistoryStatus' = "completed" + /\ UNCHANGED << maxConcurrency, reservedPermit, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus, + childLocalProfile >> + +Next == + \/ ReserveFanOutPermit + \/ SuspendParentForSerialDelegation + \/ CreateChildAfterSuspendSucceeds + \/ CreateChildAfterSuspendFails + \/ SelectChildProfile + \/ CreateChildSucceeds + \/ CreateChildFails + \/ PersistDelegationSucceeds + \/ PersistDelegationFails + \/ StartChildWithReservation + \/ ParentContinues + \/ ChildUsesScopedProfile + \/ ChildCompletes + +Spec == Init /\ [][Next]_vars + +PermitBound == ActivePermits <= maxConcurrency +NonNegativePermits == permitsHeld >= 0 +ParentProfileIsolation == parentLive => parentLocalProfile = ParentProfile +ChildProfileIsolation == childLive => childLocalProfile = ChildProfile +RollbackRestoresParent == + phase = "failed" => /\ reservedPermit = FALSE + /\ parentLive = TRUE + /\ focus = "parent" +RunningChildHasDelegatedParent == + phase = "child-running" => /\ childLive = TRUE + /\ parentHistoryStatus = "delegated" +FocusReferencesLiveTask == + /\ focus \in FocusValues + /\ (focus = "parent" => parentLive) + /\ (focus = "child" => childLive) + +==== diff --git a/src/core/task/__tests__/delegation-state-machine.spec.ts b/src/core/task/__tests__/delegation-state-machine.spec.ts new file mode 100644 index 0000000000..3ff20f2b22 --- /dev/null +++ b/src/core/task/__tests__/delegation-state-machine.spec.ts @@ -0,0 +1,359 @@ +// npx vitest run src/core/task/__tests__/delegation-state-machine.spec.ts + +import { describe, expect, it } from "vitest" + +type Profile = "parent-profile" | "child-profile" +type Focus = "parent" | "child" | "none" +type Phase = + | "parent-running" + | "parent-suspended" + | "permit-reserved" + | "child-profile-selected" + | "child-created" + | "delegation-persisted" + | "child-running" + | "child-completed" + | "failed" + +interface TaskModel { + live: boolean + localProfile: Profile + historyStatus: "active" | "delegated" | "completed" +} + +interface ModelState { + phase: Phase + maxConcurrency: number + permitsHeld: number + reservedPermit: boolean + focus: Focus + globalProfile: Profile + parent: TaskModel + child?: TaskModel + trace: string[] +} + +interface Transition { + name: string + canApply: (state: ModelState) => boolean + apply: (state: ModelState) => ModelState +} + +function cloneState(state: ModelState): ModelState { + return { + ...state, + parent: { ...state.parent }, + child: state.child ? { ...state.child } : undefined, + trace: [...state.trace], + } +} + +function transition(name: string, state: ModelState, patch: (next: ModelState) => void): ModelState { + const next = cloneState(state) + next.trace.push(name) + patch(next) + return next +} + +function activePermitCount(state: ModelState): number { + return state.permitsHeld + (state.reservedPermit ? 1 : 0) +} + +function assertInvariants(state: ModelState) { + const trace = state.trace.join(" -> ") + + expect(activePermitCount(state), `permit bound violated after ${trace}`).toBeLessThanOrEqual(state.maxConcurrency) + expect(state.permitsHeld, `negative held permits after ${trace}`).toBeGreaterThanOrEqual(0) + + if (state.parent.live) { + expect(state.parent.localProfile, `parent profile leaked after ${trace}`).toBe("parent-profile") + } + + if (state.child?.live) { + expect(state.child.localProfile, `child profile was not scoped after ${trace}`).toBe("child-profile") + } + + if (state.phase === "failed") { + expect(state.reservedPermit, `failed delegation leaked a reserved permit after ${trace}`).toBe(false) + expect(state.parent.live, `failed delegation lost the parent after ${trace}`).toBe(true) + expect(state.focus, `failed delegation did not restore parent focus after ${trace}`).toBe("parent") + } + + if (state.phase === "child-running") { + expect(state.parent.historyStatus, `running child without delegated parent history after ${trace}`).toBe( + "delegated", + ) + expect(state.child?.live, `child-running phase without live child after ${trace}`).toBe(true) + } +} + +function initialFanOutState(): ModelState { + return { + phase: "parent-running", + maxConcurrency: 2, + permitsHeld: 1, + reservedPermit: false, + focus: "parent", + globalProfile: "parent-profile", + parent: { + live: true, + localProfile: "parent-profile", + historyStatus: "active", + }, + trace: [], + } +} + +const fanOutTransitions: Transition[] = [ + { + name: "reserve fan-out permit", + canApply: (state) => state.phase === "parent-running" && activePermitCount(state) < state.maxConcurrency, + apply: (state) => + transition("reserve fan-out permit", state, (next) => { + next.phase = "permit-reserved" + next.reservedPermit = true + }), + }, + { + name: "select child profile without broadcasting to live tasks", + canApply: (state) => state.phase === "permit-reserved", + apply: (state) => + transition("select child profile without broadcasting to live tasks", state, (next) => { + next.phase = "child-profile-selected" + next.globalProfile = "child-profile" + }), + }, + { + name: "create child succeeds", + canApply: (state) => state.phase === "child-profile-selected", + apply: (state) => + transition("create child succeeds", state, (next) => { + next.phase = "child-created" + next.focus = "child" + next.child = { + live: true, + localProfile: next.globalProfile, + historyStatus: "active", + } + }), + }, + { + name: "create child throws and rolls back", + canApply: (state) => state.phase === "child-profile-selected", + apply: (state) => + transition("create child throws and rolls back", state, (next) => { + next.phase = "failed" + next.reservedPermit = false + next.focus = "parent" + next.child = undefined + next.globalProfile = "parent-profile" + }), + }, + { + name: "persist parent delegation succeeds", + canApply: (state) => state.phase === "child-created", + apply: (state) => + transition("persist parent delegation succeeds", state, (next) => { + next.phase = "delegation-persisted" + next.parent.historyStatus = "delegated" + }), + }, + { + name: "persist parent delegation throws and rolls back", + canApply: (state) => state.phase === "child-created", + apply: (state) => + transition("persist parent delegation throws and rolls back", state, (next) => { + next.phase = "failed" + next.reservedPermit = false + next.focus = "parent" + next.child = undefined + next.globalProfile = "parent-profile" + }), + }, + { + name: "start child with reserved permit", + canApply: (state) => state.phase === "delegation-persisted" && state.reservedPermit, + apply: (state) => + transition("start child with reserved permit", state, (next) => { + next.phase = "child-running" + next.reservedPermit = false + next.permitsHeld += 1 + }), + }, + { + name: "parent continues while child runs", + canApply: (state) => state.phase === "child-running", + apply: (state) => transition("parent continues while child runs", state, () => {}), + }, + { + name: "child uses its scoped profile", + canApply: (state) => state.phase === "child-running", + apply: (state) => transition("child uses its scoped profile", state, () => {}), + }, + { + name: "child completes", + canApply: (state) => state.phase === "child-running", + apply: (state) => + transition("child completes", state, (next) => { + next.phase = "child-completed" + next.permitsHeld -= 1 + next.focus = "parent" + if (next.child) { + next.child.live = false + next.child.historyStatus = "completed" + } + }), + }, +] + +function initialSerialDelegationState(): ModelState { + return { + phase: "parent-running", + maxConcurrency: 1, + permitsHeld: 0, + reservedPermit: false, + focus: "parent", + globalProfile: "parent-profile", + parent: { + live: true, + localProfile: "parent-profile", + historyStatus: "active", + }, + trace: [], + } +} + +const serialDelegationTransitions: Transition[] = [ + { + name: "suspend parent before serial child creation", + canApply: (state) => state.phase === "parent-running", + apply: (state) => + transition("suspend parent before serial child creation", state, (next) => { + next.phase = "parent-suspended" + next.parent.live = false + next.focus = "none" + }), + }, + { + name: "create child throws and restores suspended parent", + canApply: (state) => state.phase === "parent-suspended", + apply: (state) => + transition("create child throws and restores suspended parent", state, (next) => { + next.phase = "failed" + next.parent.live = true + next.focus = "parent" + }), + }, + { + name: "create child succeeds after parent suspension", + canApply: (state) => state.phase === "parent-suspended", + apply: (state) => + transition("create child succeeds after parent suspension", state, (next) => { + next.phase = "child-created" + next.focus = "child" + next.globalProfile = "child-profile" + next.child = { + live: true, + localProfile: "child-profile", + historyStatus: "active", + } + }), + }, + { + name: "persist serial delegation throws and restores suspended parent", + canApply: (state) => state.phase === "child-created" && !state.parent.live, + apply: (state) => + transition("persist serial delegation throws and restores suspended parent", state, (next) => { + next.phase = "failed" + next.parent.live = true + next.focus = "parent" + next.child = undefined + next.globalProfile = "parent-profile" + }), + }, +] + +function explore(state: ModelState, transitions: Transition[], depth: number, terminalStates: ModelState[]) { + assertInvariants(state) + + if (depth === 0) { + terminalStates.push(state) + return + } + + const applicable = transitions.filter((candidate) => candidate.canApply(state)) + if (applicable.length === 0) { + terminalStates.push(state) + return + } + + for (const candidate of applicable) { + explore(candidate.apply(state), transitions, depth - 1, terminalStates) + } +} + +describe("delegation state machine model", () => { + it("exhaustively preserves fan-out permit, rollback, history, and profile-isolation invariants", () => { + const terminalStates: ModelState[] = [] + + explore(initialFanOutState(), fanOutTransitions, 8, terminalStates) + + expect(terminalStates.length).toBeGreaterThan(0) + expect(terminalStates.some((state) => state.phase === "child-running")).toBe(true) + expect(terminalStates.some((state) => state.phase === "failed")).toBe(true) + expect(terminalStates.some((state) => state.phase === "child-completed")).toBe(true) + }) + + it("exhaustively preserves serial-path rollback when the parent was suspended before child creation", () => { + const terminalStates: ModelState[] = [] + + explore(initialSerialDelegationState(), serialDelegationTransitions, 3, terminalStates) + + expect(terminalStates.some((state) => state.phase === "failed")).toBe(true) + expect( + terminalStates.find( + (state) => + state.phase === "failed" && + state.trace.includes("create child throws and restores suspended parent"), + )?.parent.live, + ).toBe(true) + expect( + terminalStates.find( + (state) => + state.phase === "failed" && + state.trace.includes("persist serial delegation throws and restores suspended parent"), + )?.parent.live, + ).toBe(true) + }) + + it("would catch a provider-profile event broadcast leaking the child's profile into the live parent", () => { + const buggyState = transition("buggy profile broadcast", initialFanOutState(), (next) => { + next.phase = "child-profile-selected" + next.reservedPermit = true + next.globalProfile = "child-profile" + next.parent.localProfile = "child-profile" + }) + + expect(() => assertInvariants(buggyState)).toThrow(/parent profile leaked/) + }) + + it("would catch a failed child creation that leaks its reserved permit", () => { + const buggyState = transition("buggy createTask rollback", initialFanOutState(), (next) => { + next.phase = "failed" + next.reservedPermit = true + next.focus = "parent" + }) + + expect(() => assertInvariants(buggyState)).toThrow(/reserved permit/) + }) + + it("would catch a serial-path child creation failure that does not restore the suspended parent", () => { + const buggyState = transition("buggy serial createTask rollback", initialSerialDelegationState(), (next) => { + next.phase = "failed" + next.parent.live = false + next.focus = "none" + }) + + expect(() => assertInvariants(buggyState)).toThrow(/lost the parent/) + }) +}) From 5e899f307b5332f90816764500ad8f6bf08f7857 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Thu, 30 Jul 2026 01:00:33 +0000 Subject: [PATCH 4/6] feat(api): persist modeApiConfigs in setConfiguration() --- src/extension/api.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/extension/api.ts b/src/extension/api.ts index c50e29b700..af68d24b5a 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -23,6 +23,7 @@ import { import { IpcServer } from "@roo-code/ipc" import { Package } from "../shared/package" +import type { Mode } from "../shared/modes" import { ClineProvider } from "../core/webview/ClineProvider" import { Terminal } from "../integrations/terminal/Terminal" import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry" @@ -509,6 +510,13 @@ export class API extends EventEmitter implements RooCodeAPI { public async setConfiguration(values: RooCodeSettings) { await this.sidebarProvider.contextProxy.setValues(values) await this.sidebarProvider.providerSettingsManager.saveConfig(values.currentApiConfigName || "default", values) + if (values.modeApiConfigs) { + await Promise.all( + Object.entries(values.modeApiConfigs).map(([mode, configId]) => + this.sidebarProvider.providerSettingsManager.setModeConfig(mode as Mode, configId), + ), + ) + } await this.sidebarProvider.postStateToWebview() } From 17109ab2d4ea7d52dde85b00a5a74435ac02ef8c Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Thu, 30 Jul 2026 02:29:02 +0000 Subject: [PATCH 5/6] fix: isolate fan-out task state during delegation --- apps/vscode-e2e/src/suite/subtasks.test.ts | 6 ++- src/__tests__/helpers/provider-stub.ts | 10 +++- src/__tests__/provider-delegation.spec.ts | 19 +------ src/core/task/Task.ts | 6 ++- src/core/task/__tests__/Task.spec.ts | 51 +++++++++++++++++++ .../__tests__/delegation-concurrent.spec.ts | 18 ++++--- .../ClineProvider.lockApiConfig.spec.ts | 9 ++-- 7 files changed, 85 insertions(+), 34 deletions(-) diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 2ff97f458d..b3f151796e 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -254,11 +254,13 @@ suite("Roo Code Subtasks", function () { const priorModeApiConfigs = api.getConfiguration().modeApiConfigs ?? {} const parentProfileId = await api.upsertProfile("subtask-fanout-parent-profile", parentProfile, true) const childProfileId = await api.upsertProfile("subtask-fanout-child-profile", childProfile, false) + assert.ok(parentProfileId, "Failed to create parent profile") + assert.ok(childProfileId, "Failed to create child profile") await api.setConfiguration({ modeApiConfigs: { ...priorModeApiConfigs, - code: parentProfileId!, - ask: childProfileId!, + code: parentProfileId, + ask: childProfileId, }, }) diff --git a/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts index 85bd1a0b19..346abf73ac 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -23,6 +23,12 @@ type PrivateProviderMethods = { restoreParentOrReleasePermit: (this: unknown, ...args: unknown[]) => unknown } +export function bindRestoreParentOrReleasePermit(provider: ClineProvider): void { + const s = provider as unknown as ProviderStubFields + const proto = ClineProvider.prototype as unknown as PrivateProviderMethods + s.restoreParentOrReleasePermit = proto.restoreParentOrReleasePermit.bind(s) +} + /** * Augments a plain stub object with the instance fields and bound methods that * ClineProvider methods read from `this` (runDelegationTransition, @@ -53,6 +59,8 @@ export function makeProviderStub(stub: T): ClineProvider { s.runDelegationTransition ??= proto.runDelegationTransition.bind(s) s.removeClineFromStack ??= proto.removeClineFromStack.bind(s) s.evictCurrentTask ??= proto.evictCurrentTask.bind(s) - s.restoreParentOrReleasePermit ??= proto.restoreParentOrReleasePermit.bind(s) + if (!s.restoreParentOrReleasePermit) { + bindRestoreParentOrReleasePermit(s as unknown as ClineProvider) + } return s as unknown as ClineProvider } diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 8ac6670479..d3bd639931 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -5,24 +5,7 @@ import type { HistoryItem } from "@roo-code/types" import { RooCodeEventName } from "@roo-code/types" import { ClineProvider } from "../core/webview/ClineProvider" import { TaskScheduler } from "../core/task/TaskScheduler" - -/** - * restoreParentOrReleasePermit is a private prototype method that plain - * object-literal provider stubs don't have unless bound explicitly (same - * reason removeClineFromStack/evictCurrentTask need binding in provider-stub.ts). - */ -function bindRestoreParentOrReleasePermit(provider: ClineProvider): void { - type WithRestore = { - restoreParentOrReleasePermit: ( - parentTaskId: string, - fanOut: boolean, - childReservedRelease: (() => void) | undefined, - ) => Promise - } - ;(provider as unknown as WithRestore).restoreParentOrReleasePermit = ( - ClineProvider.prototype as unknown as WithRestore - ).restoreParentOrReleasePermit.bind(provider) -} +import { bindRestoreParentOrReleasePermit } from "./helpers/provider-stub" const parentHistoryItem: HistoryItem = { id: "parent-1", diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 7e5c288197..f789ee7020 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -4021,14 +4021,16 @@ export class Task extends EventEmitter implements TaskLike { const state = await this.providerRef.deref()?.getState() const { - apiConfiguration, autoApprovalEnabled, requestDelaySeconds, - mode, autoCondenseContext = true, autoCondenseContextPercent = 100, profileThresholds = {}, } = state ?? {} + // mode/apiConfiguration come from this task's own fields, not shared + // provider state — see getSystemPrompt() for why. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration // Get condensing configuration for automatic triggers. const customCondensingPrompt = state?.customSupportPrompts?.CONDENSE diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 3e3005b5dd..adca708a0c 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -599,6 +599,57 @@ describe("Cline", () => { expect(Object.keys(cleanConversationHistory[0]!)).toEqual(["role", "content"]) }) + it("uses task-local mode and apiConfiguration in request metadata when provider state diverges", async () => { + const taskApiConfiguration = { + ...mockApiConfig, + apiProvider: providerIdentifiers.gemini, + } as ProviderSettings + + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + mode: "ask", + apiConfiguration: taskApiConfiguration, + autoApprovalEnabled: true, + requestDelaySeconds: 0, + }) + + const cline = new Task({ + provider: mockProvider, + apiConfiguration: taskApiConfiguration, + task: "test task", + startTask: false, + }) + await cline.getTaskMode() + vi.spyOn(getTaskTestAccess(cline), "getSystemPrompt").mockResolvedValue("mock system prompt") + vi.spyOn(cline.api, "getModel").mockReturnValue({ + id: requireDefined(mockApiConfig.apiModelId), + info: { contextWindow: 200000, maxTokens: 4096 } as ModelInfo, + }) + + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + mode: "code", + apiConfiguration: { + ...mockApiConfig, + apiProvider: providerIdentifiers.anthropic, + }, + autoApprovalEnabled: true, + requestDelaySeconds: 0, + }) + + const mockStream = (async function* () { + yield { type: "text", text: "response" } as ApiStreamChunk + })() + const createMessageSpy = vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStream) + cline.apiConversationHistory = [ + { role: "user", content: [{ type: "text", text: "test message" }], ts: Date.now() }, + ] + + await cline.attemptApiRequest(0).next() + + const [, , metadata] = requireDefined(createMessageSpy.mock.calls[0]) + expect(metadata?.mode).toBe("ask") + expect(metadata?.allowedFunctionNames).toBeDefined() + }) + it("should shape image blocks for API compatibility before request construction", async () => { const conversationHistory = [ { diff --git a/src/core/task/__tests__/delegation-concurrent.spec.ts b/src/core/task/__tests__/delegation-concurrent.spec.ts index c711997754..b0d8b7d851 100644 --- a/src/core/task/__tests__/delegation-concurrent.spec.ts +++ b/src/core/task/__tests__/delegation-concurrent.spec.ts @@ -147,7 +147,7 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { void scheduler.schedule(makeParent({ taskId: "occupant-2" }), () => new Promise(() => {})) // Poll the deterministic signal instead of assuming a fixed microtask // depth for sem.acquire() to settle. - while (scheduler.available > 0) { + for (let i = 0; i < 10 && scheduler.available > 0; i++) { await Promise.resolve() } @@ -263,12 +263,16 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { createTaskWithHistoryItem, }) - await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + try { + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) - expect(createTaskWithHistoryItem).toHaveBeenCalledTimes(2) - expect(showErrorMessage).toHaveBeenCalledWith( - "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", - ) + expect(createTaskWithHistoryItem).toHaveBeenCalledTimes(2) + expect(showErrorMessage).toHaveBeenCalledWith( + "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", + ) + } finally { + showErrorMessage.mockRestore() + } }) it("fan-out path: both parent and child are tracked in the registry with no shared clineMessages reference", async () => { @@ -313,7 +317,7 @@ describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { // Occupy one permit with the "parent's own request loop" so only one // permit remains — exactly the scenario fan-out is meant to handle. void scheduler.schedule(parent, parentRun) - while (scheduler.available > 1) { + for (let i = 0; i < 10 && scheduler.available > 1; i++) { await Promise.resolve() } diff --git a/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts b/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts index 49d181ed36..07e6b82a64 100644 --- a/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts @@ -374,14 +374,15 @@ describe("ClineProvider - Lock API Config Across Modes", () => { apiProvider: "anthropic", }) - const activateProviderProfileSpy = vi - .spyOn(provider, "activateProviderProfile") - .mockResolvedValue(undefined) + const activateProfileSpy = vi.spyOn(provider.providerSettingsManager, "activateProfile").mockResolvedValue({ + name: "architect-profile", + apiProvider: "anthropic", + }) await provider.handleModeSwitch("architect") expect(getModeConfigIdSpy).toHaveBeenCalledWith("architect") - expect(activateProviderProfileSpy).toHaveBeenCalledWith({ name: "architect-profile" }) + expect(activateProfileSpy).toHaveBeenCalledWith({ name: "architect-profile" }) }) }) }) From b2d2156f71da39170cd26b2c518974108365e92e Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Thu, 30 Jul 2026 13:20:57 +0000 Subject: [PATCH 6/6] test: harden fan-out regression coverage --- apps/vscode-e2e/src/fixtures/search-files.ts | 2 +- .../src/fixtures/terminal-reuse-shell-race.ts | 25 +- apps/vscode-e2e/src/suite/subtasks.test.ts | 13 + docs/formal/task-delegation/README.md | 84 ---- .../formal/task-delegation/TaskDelegation.cfg | 15 - .../formal/task-delegation/TaskDelegation.tla | 208 ---------- src/core/task/__tests__/Task.spec.ts | 7 +- .../delegation-state-machine.spec.ts | 359 ------------------ 8 files changed, 41 insertions(+), 672 deletions(-) delete mode 100644 docs/formal/task-delegation/README.md delete mode 100644 docs/formal/task-delegation/TaskDelegation.cfg delete mode 100644 docs/formal/task-delegation/TaskDelegation.tla delete mode 100644 src/core/task/__tests__/delegation-state-machine.spec.ts diff --git a/apps/vscode-e2e/src/fixtures/search-files.ts b/apps/vscode-e2e/src/fixtures/search-files.ts index 3c1318c299..b653e199c0 100644 --- a/apps/vscode-e2e/src/fixtures/search-files.ts +++ b/apps/vscode-e2e/src/fixtures/search-files.ts @@ -89,7 +89,7 @@ export function addSearchFilesResultFixtures(mock: InstanceType) toolName: "search_files", arguments: '{"path":"search-files-tool-fixture","regex":"nonExistentPattern12345"}', toolCallId: "call_search_files_no_match_001", - expected: ["No results found"], + expected: ["Found 0 results."], result: "No matches were found for `nonExistentPattern12345` in the search fixture directory.", id: "call_search_files_no_match_002", }, diff --git a/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts b/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts index 2fa9fc25b6..14d9293ff8 100644 --- a/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts +++ b/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts @@ -4,10 +4,19 @@ import { toolResultContains } from "./tool-result" export function addTerminalReuseShellRaceFixtures(mock: InstanceType) { // First command completes — model issues a second command on the same terminal. - // With the temp-script fix, both commands now deliver real output. mock.addFixture({ match: { - predicate: (req) => toolResultContains(req, "call_terminal_reuse_001", ["first", "Exit code: 0"]), + predicate: (req) => { + const messages = Array.isArray(req?.messages) ? req.messages : [] + const lastToolMsg = messages.filter((message) => message?.role === "tool").at(-1) + + return ( + lastToolMsg?.tool_call_id === "call_terminal_reuse_001" && + toolResultContains(req, "call_terminal_reuse_001", [ + "Command was submitted in the VS Code terminal", + ]) + ) + }, }, response: { toolCalls: [ @@ -25,7 +34,17 @@ export function addTerminalReuseShellRaceFixtures(mock: InstanceType toolResultContains(req, "call_terminal_reuse_002", ["second", "Exit code: 0"]), + predicate: (req) => { + const messages = Array.isArray(req?.messages) ? req.messages : [] + const lastToolMsg = messages.filter((message) => message?.role === "tool").at(-1) + + return ( + lastToolMsg?.tool_call_id === "call_terminal_reuse_002" && + toolResultContains(req, "call_terminal_reuse_002", [ + "Command was submitted in the VS Code terminal", + ]) + ) + }, }, response: { toolCalls: [ diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index b3f151796e..ad40c4a0ba 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -216,6 +216,19 @@ suite("Roo Code Subtasks", function () { assert.ok(stack.includes(parentTaskId), "Fan-out parent should remain in the live task stack") assert.ok(stack.includes(childTaskId!), "Fan-out child should remain in the live task stack") assert.strictEqual(stack.at(-1), childTaskId, "Child should remain the focused task while parent runs") + + const parentHistory = await api.getTaskHistoryItem(parentTaskId) + assert.strictEqual(parentHistory?.status, "delegated", "Fan-out parent history should stay delegated") + assert.strictEqual( + parentHistory?.awaitingChildId, + childTaskId, + "Fan-out parent history should point at the running child", + ) + assert.strictEqual( + parentHistory?.delegatedToId, + childTaskId, + "Fan-out parent history should record the delegated child", + ) } finally { api.off(RooCodeEventName.Message, messageHandler) while (api.getCurrentTaskStack().length > 0) { diff --git a/docs/formal/task-delegation/README.md b/docs/formal/task-delegation/README.md deleted file mode 100644 index 6632c09622..0000000000 --- a/docs/formal/task-delegation/README.md +++ /dev/null @@ -1,84 +0,0 @@ -# Task Delegation Formal Model - -This directory contains a small TLA+ model for the task-delegation state machine. - -The model is intentionally abstract. It does not model VS Code, webviews, prompt contents, provider implementations, or task message history. It only models the state that must remain consistent while a parent task delegates work to a child task: - -- parent task liveness while a child is created -- child focus after creation -- semaphore reservation and release -- rollback after child creation or history persistence failures -- parent and child API profile isolation -- parent history status after delegation - -## Local Checks - -The executable CI check lives in `src/core/task/__tests__/delegation-state-machine.spec.ts`. It exhaustively explores the same small state machine with Vitest and includes explicit negative controls for the highest-risk invariant violations: - -- broadcasting a child provider-profile change to the live parent -- leaking a reserved permit after failed child creation -- failing to restore a suspended parent after serial child creation fails - -Run it with: - -```bash -pnpm --dir src exec vitest run core/task/__tests__/delegation-state-machine.spec.ts -``` - -## TLA+ Checks - -The TLA+ spec can be checked with TLC from the TLA+ Toolbox, the VS Code TLA+ extension, or a CLI TLC installation: - -```bash -java -cp /path/to/tla2tools.jar tlc2.TLC TaskDelegation.tla -config TaskDelegation.cfg -``` - -The main invariants are: - -- `PermitBound`: held plus reserved permits never exceed the configured concurrency. -- `ParentProfileIsolation`: a live parent keeps its original API profile. -- `ChildProfileIsolation`: a live child uses the child API profile. -- `RollbackRestoresParent`: failed delegation releases reservations and returns focus to the parent. -- `RunningChildHasDelegatedParent`: a running child implies the parent history item is delegated. - -## Drift Control - -The Vitest model is the primary drift monitor because it runs with the normal test suite. If production delegation behavior changes, update the Vitest model and this TLA+ model in the same change. - -Reviewers should check this mapping when touching delegation code: - -- `TaskScheduler.tryReserve()` / `runWithReservation()` map to the `reservedPermit` and `permitsHeld` transitions. -- `delegateParentAndOpenChild()` maps to the parent liveness, child creation, focus, rollback, and history transitions. -- provider-profile switching maps to `globalProfile`, `parentLocalProfile`, and `childLocalProfile`. -- task-history updates map to `parentHistoryStatus` and `childHistoryStatus`. - -Run TLC locally for changes that alter delegation ordering, rollback, profile switching, or scheduler permits. For smaller implementation-only changes, the Vitest model plus targeted unit/e2e coverage is usually sufficient. - -## State-Machine Review Checklist - -Use this checklist for changes that touch delegation, task focus, scheduler permits, provider profile or mode switching, task-local API configuration, or rollback behavior. - -Before coding, state the invariant the change is preserving. Example: "provider/profile mutations are serialized; a timed-out caller must not allow a later mutation to overtake an earlier one." - -For every async or mutating fix, answer: - -- Does a timeout cancel the work, or only stop waiting for it? -- If timed-out work can still complete later, can it mutate state after a newer operation? -- Can a later mode/profile mutation overtake an earlier one? -- Does rejection poison the queue, skip queued work, or create an unhandled rejection? -- If a required setup step fails, does delegation abort or continue? -- If rollback fails, what live task, focused task, history status, and reserved permits remain? -- Does rollback route through the same queue or lock that may already be blocked? -- Does any code read shared provider state where task-local state is required? -- Does any webview push re-derive the focused task when the operation is about a different task? - -Required tests for these changes: - -- A positive test for the intended path. -- A negative test that would fail with the original bug. -- A fix-interaction test that would fail if the fix introduces a stale completion, queue overtake, leaked permit, or rollback-of-rollback failure. - -Any timeout around a mutating async operation must prove one of these two properties: - -- the underlying operation is actually cancelled before later conflicting mutations can run; or -- later conflicting mutations remain queued until the timed-out operation truly settles. diff --git a/docs/formal/task-delegation/TaskDelegation.cfg b/docs/formal/task-delegation/TaskDelegation.cfg deleted file mode 100644 index 0e12afd0a8..0000000000 --- a/docs/formal/task-delegation/TaskDelegation.cfg +++ /dev/null @@ -1,15 +0,0 @@ -SPECIFICATION Spec - -CONSTANTS - ParentProfile = parent_profile - ChildProfile = child_profile - -INVARIANT PermitBound -INVARIANT NonNegativePermits -INVARIANT ParentProfileIsolation -INVARIANT ChildProfileIsolation -INVARIANT RollbackRestoresParent -INVARIANT RunningChildHasDelegatedParent -INVARIANT FocusReferencesLiveTask - -CHECK_DEADLOCK FALSE diff --git a/docs/formal/task-delegation/TaskDelegation.tla b/docs/formal/task-delegation/TaskDelegation.tla deleted file mode 100644 index 643b7ae279..0000000000 --- a/docs/formal/task-delegation/TaskDelegation.tla +++ /dev/null @@ -1,208 +0,0 @@ ----- MODULE TaskDelegation ---- -EXTENDS Naturals, TLC - -CONSTANTS ParentProfile, ChildProfile - -Profiles == { ParentProfile, ChildProfile } -FocusValues == { "parent", "child", "none" } -Phases == { - "parent-running", - "parent-suspended", - "permit-reserved", - "child-profile-selected", - "child-created", - "delegation-persisted", - "child-running", - "child-completed", - "failed" -} -HistoryStatuses == { "active", "delegated", "completed" } - -VARIABLES - phase, - maxConcurrency, - permitsHeld, - reservedPermit, - focus, - globalProfile, - parentLive, - parentLocalProfile, - parentHistoryStatus, - childLive, - childLocalProfile, - childHistoryStatus - -vars == << - phase, - maxConcurrency, - permitsHeld, - reservedPermit, - focus, - globalProfile, - parentLive, - parentLocalProfile, - parentHistoryStatus, - childLive, - childLocalProfile, - childHistoryStatus ->> - -Init == - /\ phase = "parent-running" - /\ maxConcurrency \in { 1, 2 } - /\ permitsHeld = IF maxConcurrency = 2 THEN 1 ELSE 0 - /\ reservedPermit = FALSE - /\ focus = "parent" - /\ globalProfile = ParentProfile - /\ parentLive = TRUE - /\ parentLocalProfile = ParentProfile - /\ parentHistoryStatus = "active" - /\ childLive = FALSE - /\ childLocalProfile = ChildProfile - /\ childHistoryStatus = "active" - -ActivePermits == permitsHeld + IF reservedPermit THEN 1 ELSE 0 - -ReserveFanOutPermit == - /\ phase = "parent-running" - /\ maxConcurrency > 1 - /\ ActivePermits < maxConcurrency - /\ phase' = "permit-reserved" - /\ reservedPermit' = TRUE - /\ UNCHANGED << maxConcurrency, permitsHeld, focus, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus, - childLive, childLocalProfile, childHistoryStatus >> - -SuspendParentForSerialDelegation == - /\ phase = "parent-running" - /\ maxConcurrency = 1 - /\ phase' = "parent-suspended" - /\ parentLive' = FALSE - /\ focus' = "none" - /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, globalProfile, parentLocalProfile, parentHistoryStatus, - childLive, childLocalProfile, childHistoryStatus >> - -CreateChildAfterSuspendSucceeds == - /\ phase = "parent-suspended" - /\ phase' = "child-created" - /\ focus' = "child" - /\ globalProfile' = ChildProfile - /\ childLive' = TRUE - /\ childLocalProfile' = ChildProfile - /\ childHistoryStatus' = "active" - /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, parentLive, parentLocalProfile, parentHistoryStatus >> - -CreateChildAfterSuspendFails == - /\ phase = "parent-suspended" - /\ phase' = "failed" - /\ parentLive' = TRUE - /\ focus' = "parent" - /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, globalProfile, parentLocalProfile, parentHistoryStatus, - childLive, childLocalProfile, childHistoryStatus >> - -SelectChildProfile == - /\ phase = "permit-reserved" - /\ phase' = "child-profile-selected" - /\ globalProfile' = ChildProfile - /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, focus, parentLive, parentLocalProfile, parentHistoryStatus, - childLive, childLocalProfile, childHistoryStatus >> - -CreateChildSucceeds == - /\ phase = "child-profile-selected" - /\ phase' = "child-created" - /\ focus' = "child" - /\ childLive' = TRUE - /\ childLocalProfile' = globalProfile - /\ childHistoryStatus' = "active" - /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus >> - -CreateChildFails == - /\ phase = "child-profile-selected" - /\ phase' = "failed" - /\ reservedPermit' = FALSE - /\ focus' = "parent" - /\ globalProfile' = ParentProfile - /\ childLive' = FALSE - /\ childLocalProfile' = ChildProfile - /\ childHistoryStatus' = "active" - /\ UNCHANGED << maxConcurrency, permitsHeld, parentLive, parentLocalProfile, parentHistoryStatus >> - -PersistDelegationSucceeds == - /\ phase = "child-created" - /\ phase' = "delegation-persisted" - /\ parentHistoryStatus' = "delegated" - /\ UNCHANGED << maxConcurrency, permitsHeld, reservedPermit, focus, globalProfile, parentLive, parentLocalProfile, - childLive, childLocalProfile, childHistoryStatus >> - -PersistDelegationFails == - /\ phase = "child-created" - /\ phase' = "failed" - /\ reservedPermit' = FALSE - /\ focus' = "parent" - /\ globalProfile' = ParentProfile - /\ parentLive' = TRUE - /\ childLive' = FALSE - /\ childLocalProfile' = ChildProfile - /\ childHistoryStatus' = "active" - /\ UNCHANGED << maxConcurrency, permitsHeld, parentLocalProfile, parentHistoryStatus >> - -StartChildWithReservation == - /\ phase = "delegation-persisted" - /\ reservedPermit = TRUE - /\ phase' = "child-running" - /\ reservedPermit' = FALSE - /\ permitsHeld' = permitsHeld + 1 - /\ UNCHANGED << maxConcurrency, focus, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus, - childLive, childLocalProfile, childHistoryStatus >> - -ParentContinues == - /\ phase = "child-running" - /\ UNCHANGED vars - -ChildUsesScopedProfile == - /\ phase = "child-running" - /\ UNCHANGED vars - -ChildCompletes == - /\ phase = "child-running" - /\ phase' = "child-completed" - /\ permitsHeld' = permitsHeld - 1 - /\ focus' = "parent" - /\ childLive' = FALSE - /\ childHistoryStatus' = "completed" - /\ UNCHANGED << maxConcurrency, reservedPermit, globalProfile, parentLive, parentLocalProfile, parentHistoryStatus, - childLocalProfile >> - -Next == - \/ ReserveFanOutPermit - \/ SuspendParentForSerialDelegation - \/ CreateChildAfterSuspendSucceeds - \/ CreateChildAfterSuspendFails - \/ SelectChildProfile - \/ CreateChildSucceeds - \/ CreateChildFails - \/ PersistDelegationSucceeds - \/ PersistDelegationFails - \/ StartChildWithReservation - \/ ParentContinues - \/ ChildUsesScopedProfile - \/ ChildCompletes - -Spec == Init /\ [][Next]_vars - -PermitBound == ActivePermits <= maxConcurrency -NonNegativePermits == permitsHeld >= 0 -ParentProfileIsolation == parentLive => parentLocalProfile = ParentProfile -ChildProfileIsolation == childLive => childLocalProfile = ChildProfile -RollbackRestoresParent == - phase = "failed" => /\ reservedPermit = FALSE - /\ parentLive = TRUE - /\ focus = "parent" -RunningChildHasDelegatedParent == - phase = "child-running" => /\ childLive = TRUE - /\ parentHistoryStatus = "delegated" -FocusReferencesLiveTask == - /\ focus \in FocusValues - /\ (focus = "parent" => parentLive) - /\ (focus = "child" => childLive) - -==== diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index adca708a0c..56718ceb71 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -460,7 +460,7 @@ describe("Cline", () => { // directly — instead of the task's own fields — would leak the child's // mode/config into the parent's next system prompt. it("uses the task's own mode, not provider.getState().mode, when they diverge", async () => { - mockProvider.getState = vi.fn().mockResolvedValue({ mode: "architect" }) + mockProvider.getState = vi.fn().mockResolvedValue({ mode: "architect", mcpEnabled: false }) const cline = new Task({ provider: mockProvider, @@ -472,7 +472,8 @@ describe("Cline", () => { // Simulate a concurrently-running child having switched shared provider // state to a different mode (what handleModeSwitch does today). - mockProvider.getState = vi.fn().mockResolvedValue({ mode: "code" }) + mockProvider.getState = vi.fn().mockResolvedValue({ mode: "code", mcpEnabled: false }) + vi.mocked(SYSTEM_PROMPT).mockResolvedValueOnce("mock system prompt") await getTaskTestAccess(cline).getSystemPrompt() @@ -500,8 +501,10 @@ describe("Cline", () => { // a leak is directly observable in SYSTEM_PROMPT's settings argument. mockProvider.getState = vi.fn().mockResolvedValue({ mode: "code", + mcpEnabled: false, apiConfiguration: { ...mockApiConfig, todoListEnabled: false }, }) + vi.mocked(SYSTEM_PROMPT).mockResolvedValueOnce("mock system prompt") await getTaskTestAccess(cline).getSystemPrompt() diff --git a/src/core/task/__tests__/delegation-state-machine.spec.ts b/src/core/task/__tests__/delegation-state-machine.spec.ts deleted file mode 100644 index 3ff20f2b22..0000000000 --- a/src/core/task/__tests__/delegation-state-machine.spec.ts +++ /dev/null @@ -1,359 +0,0 @@ -// npx vitest run src/core/task/__tests__/delegation-state-machine.spec.ts - -import { describe, expect, it } from "vitest" - -type Profile = "parent-profile" | "child-profile" -type Focus = "parent" | "child" | "none" -type Phase = - | "parent-running" - | "parent-suspended" - | "permit-reserved" - | "child-profile-selected" - | "child-created" - | "delegation-persisted" - | "child-running" - | "child-completed" - | "failed" - -interface TaskModel { - live: boolean - localProfile: Profile - historyStatus: "active" | "delegated" | "completed" -} - -interface ModelState { - phase: Phase - maxConcurrency: number - permitsHeld: number - reservedPermit: boolean - focus: Focus - globalProfile: Profile - parent: TaskModel - child?: TaskModel - trace: string[] -} - -interface Transition { - name: string - canApply: (state: ModelState) => boolean - apply: (state: ModelState) => ModelState -} - -function cloneState(state: ModelState): ModelState { - return { - ...state, - parent: { ...state.parent }, - child: state.child ? { ...state.child } : undefined, - trace: [...state.trace], - } -} - -function transition(name: string, state: ModelState, patch: (next: ModelState) => void): ModelState { - const next = cloneState(state) - next.trace.push(name) - patch(next) - return next -} - -function activePermitCount(state: ModelState): number { - return state.permitsHeld + (state.reservedPermit ? 1 : 0) -} - -function assertInvariants(state: ModelState) { - const trace = state.trace.join(" -> ") - - expect(activePermitCount(state), `permit bound violated after ${trace}`).toBeLessThanOrEqual(state.maxConcurrency) - expect(state.permitsHeld, `negative held permits after ${trace}`).toBeGreaterThanOrEqual(0) - - if (state.parent.live) { - expect(state.parent.localProfile, `parent profile leaked after ${trace}`).toBe("parent-profile") - } - - if (state.child?.live) { - expect(state.child.localProfile, `child profile was not scoped after ${trace}`).toBe("child-profile") - } - - if (state.phase === "failed") { - expect(state.reservedPermit, `failed delegation leaked a reserved permit after ${trace}`).toBe(false) - expect(state.parent.live, `failed delegation lost the parent after ${trace}`).toBe(true) - expect(state.focus, `failed delegation did not restore parent focus after ${trace}`).toBe("parent") - } - - if (state.phase === "child-running") { - expect(state.parent.historyStatus, `running child without delegated parent history after ${trace}`).toBe( - "delegated", - ) - expect(state.child?.live, `child-running phase without live child after ${trace}`).toBe(true) - } -} - -function initialFanOutState(): ModelState { - return { - phase: "parent-running", - maxConcurrency: 2, - permitsHeld: 1, - reservedPermit: false, - focus: "parent", - globalProfile: "parent-profile", - parent: { - live: true, - localProfile: "parent-profile", - historyStatus: "active", - }, - trace: [], - } -} - -const fanOutTransitions: Transition[] = [ - { - name: "reserve fan-out permit", - canApply: (state) => state.phase === "parent-running" && activePermitCount(state) < state.maxConcurrency, - apply: (state) => - transition("reserve fan-out permit", state, (next) => { - next.phase = "permit-reserved" - next.reservedPermit = true - }), - }, - { - name: "select child profile without broadcasting to live tasks", - canApply: (state) => state.phase === "permit-reserved", - apply: (state) => - transition("select child profile without broadcasting to live tasks", state, (next) => { - next.phase = "child-profile-selected" - next.globalProfile = "child-profile" - }), - }, - { - name: "create child succeeds", - canApply: (state) => state.phase === "child-profile-selected", - apply: (state) => - transition("create child succeeds", state, (next) => { - next.phase = "child-created" - next.focus = "child" - next.child = { - live: true, - localProfile: next.globalProfile, - historyStatus: "active", - } - }), - }, - { - name: "create child throws and rolls back", - canApply: (state) => state.phase === "child-profile-selected", - apply: (state) => - transition("create child throws and rolls back", state, (next) => { - next.phase = "failed" - next.reservedPermit = false - next.focus = "parent" - next.child = undefined - next.globalProfile = "parent-profile" - }), - }, - { - name: "persist parent delegation succeeds", - canApply: (state) => state.phase === "child-created", - apply: (state) => - transition("persist parent delegation succeeds", state, (next) => { - next.phase = "delegation-persisted" - next.parent.historyStatus = "delegated" - }), - }, - { - name: "persist parent delegation throws and rolls back", - canApply: (state) => state.phase === "child-created", - apply: (state) => - transition("persist parent delegation throws and rolls back", state, (next) => { - next.phase = "failed" - next.reservedPermit = false - next.focus = "parent" - next.child = undefined - next.globalProfile = "parent-profile" - }), - }, - { - name: "start child with reserved permit", - canApply: (state) => state.phase === "delegation-persisted" && state.reservedPermit, - apply: (state) => - transition("start child with reserved permit", state, (next) => { - next.phase = "child-running" - next.reservedPermit = false - next.permitsHeld += 1 - }), - }, - { - name: "parent continues while child runs", - canApply: (state) => state.phase === "child-running", - apply: (state) => transition("parent continues while child runs", state, () => {}), - }, - { - name: "child uses its scoped profile", - canApply: (state) => state.phase === "child-running", - apply: (state) => transition("child uses its scoped profile", state, () => {}), - }, - { - name: "child completes", - canApply: (state) => state.phase === "child-running", - apply: (state) => - transition("child completes", state, (next) => { - next.phase = "child-completed" - next.permitsHeld -= 1 - next.focus = "parent" - if (next.child) { - next.child.live = false - next.child.historyStatus = "completed" - } - }), - }, -] - -function initialSerialDelegationState(): ModelState { - return { - phase: "parent-running", - maxConcurrency: 1, - permitsHeld: 0, - reservedPermit: false, - focus: "parent", - globalProfile: "parent-profile", - parent: { - live: true, - localProfile: "parent-profile", - historyStatus: "active", - }, - trace: [], - } -} - -const serialDelegationTransitions: Transition[] = [ - { - name: "suspend parent before serial child creation", - canApply: (state) => state.phase === "parent-running", - apply: (state) => - transition("suspend parent before serial child creation", state, (next) => { - next.phase = "parent-suspended" - next.parent.live = false - next.focus = "none" - }), - }, - { - name: "create child throws and restores suspended parent", - canApply: (state) => state.phase === "parent-suspended", - apply: (state) => - transition("create child throws and restores suspended parent", state, (next) => { - next.phase = "failed" - next.parent.live = true - next.focus = "parent" - }), - }, - { - name: "create child succeeds after parent suspension", - canApply: (state) => state.phase === "parent-suspended", - apply: (state) => - transition("create child succeeds after parent suspension", state, (next) => { - next.phase = "child-created" - next.focus = "child" - next.globalProfile = "child-profile" - next.child = { - live: true, - localProfile: "child-profile", - historyStatus: "active", - } - }), - }, - { - name: "persist serial delegation throws and restores suspended parent", - canApply: (state) => state.phase === "child-created" && !state.parent.live, - apply: (state) => - transition("persist serial delegation throws and restores suspended parent", state, (next) => { - next.phase = "failed" - next.parent.live = true - next.focus = "parent" - next.child = undefined - next.globalProfile = "parent-profile" - }), - }, -] - -function explore(state: ModelState, transitions: Transition[], depth: number, terminalStates: ModelState[]) { - assertInvariants(state) - - if (depth === 0) { - terminalStates.push(state) - return - } - - const applicable = transitions.filter((candidate) => candidate.canApply(state)) - if (applicable.length === 0) { - terminalStates.push(state) - return - } - - for (const candidate of applicable) { - explore(candidate.apply(state), transitions, depth - 1, terminalStates) - } -} - -describe("delegation state machine model", () => { - it("exhaustively preserves fan-out permit, rollback, history, and profile-isolation invariants", () => { - const terminalStates: ModelState[] = [] - - explore(initialFanOutState(), fanOutTransitions, 8, terminalStates) - - expect(terminalStates.length).toBeGreaterThan(0) - expect(terminalStates.some((state) => state.phase === "child-running")).toBe(true) - expect(terminalStates.some((state) => state.phase === "failed")).toBe(true) - expect(terminalStates.some((state) => state.phase === "child-completed")).toBe(true) - }) - - it("exhaustively preserves serial-path rollback when the parent was suspended before child creation", () => { - const terminalStates: ModelState[] = [] - - explore(initialSerialDelegationState(), serialDelegationTransitions, 3, terminalStates) - - expect(terminalStates.some((state) => state.phase === "failed")).toBe(true) - expect( - terminalStates.find( - (state) => - state.phase === "failed" && - state.trace.includes("create child throws and restores suspended parent"), - )?.parent.live, - ).toBe(true) - expect( - terminalStates.find( - (state) => - state.phase === "failed" && - state.trace.includes("persist serial delegation throws and restores suspended parent"), - )?.parent.live, - ).toBe(true) - }) - - it("would catch a provider-profile event broadcast leaking the child's profile into the live parent", () => { - const buggyState = transition("buggy profile broadcast", initialFanOutState(), (next) => { - next.phase = "child-profile-selected" - next.reservedPermit = true - next.globalProfile = "child-profile" - next.parent.localProfile = "child-profile" - }) - - expect(() => assertInvariants(buggyState)).toThrow(/parent profile leaked/) - }) - - it("would catch a failed child creation that leaks its reserved permit", () => { - const buggyState = transition("buggy createTask rollback", initialFanOutState(), (next) => { - next.phase = "failed" - next.reservedPermit = true - next.focus = "parent" - }) - - expect(() => assertInvariants(buggyState)).toThrow(/reserved permit/) - }) - - it("would catch a serial-path child creation failure that does not restore the suspended parent", () => { - const buggyState = transition("buggy serial createTask rollback", initialSerialDelegationState(), (next) => { - next.phase = "failed" - next.parent.live = false - next.focus = "none" - }) - - expect(() => assertInvariants(buggyState)).toThrow(/lost the parent/) - }) -})