Skip to content

Commit 7476c67

Browse files
feat(task-lifecycle): task status transition guard and startup delegation reconciliation (#692)
* feat(task-lifecycle): transition guard and startup delegation reconciliation * test: updating TaskPersistence vitest mocks * fix(task-persistence): enforce status transition guard at upsertCore write boundary * fix(delegation): guard concurrent rollback pop and add transition tests * feat(ClineProvider): guarding parent, child transitions * feat(ClineProvider): adding delegatedToId * test(subtask): adding e2e test and apis for history tracking --------- Co-authored-by: Naved Merchant <naved.merchant@gmail.com>
1 parent f845f2a commit 7476c67

20 files changed

Lines changed: 953 additions & 79 deletions

apps/vscode-e2e/src/suite/subtasks.test.ts

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,103 @@ suite("Roo Code Subtasks", function () {
150150
}
151151
})
152152

153+
test("delegated child completion persists parent and child history state", async () => {
154+
const api = globalThis.api
155+
const asks: Record<string, ClineMessage[]> = {}
156+
const says: Record<string, ClineMessage[]> = {}
157+
158+
let delegationCompletedParentId: string | undefined
159+
let delegationCompletedChildId: string | undefined
160+
let delegationCompletedSummary: string | undefined
161+
162+
const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => {
163+
if (message.type === "ask") {
164+
asks[taskId] = asks[taskId] || []
165+
asks[taskId].push(message)
166+
}
167+
if (message.type === "say" && message.partial === false) {
168+
says[taskId] = says[taskId] || []
169+
says[taskId].push(message)
170+
}
171+
}
172+
173+
const delegationCompletedHandler = (parentId: string, childId: string, summary: string) => {
174+
delegationCompletedParentId = parentId
175+
delegationCompletedChildId = childId
176+
delegationCompletedSummary = summary
177+
}
178+
179+
api.on(RooCodeEventName.Message, messageHandler)
180+
api.on(RooCodeEventName.TaskDelegationCompleted, delegationCompletedHandler)
181+
182+
try {
183+
const parentTaskId = await api.startNewTask({
184+
configuration: {
185+
mode: "ask",
186+
alwaysAllowModeSwitch: true,
187+
alwaysAllowSubtasks: true,
188+
autoApprovalEnabled: true,
189+
enableCheckpoints: false,
190+
},
191+
text: SUBTASK_PARENT_PROMPT,
192+
})
193+
194+
let childTaskId: string | undefined
195+
await waitFor(() => {
196+
const stack = api.getCurrentTaskStack()
197+
const current = stack[stack.length - 1]
198+
if (current && current !== parentTaskId) {
199+
childTaskId = current
200+
return true
201+
}
202+
return false
203+
})
204+
205+
await waitFor(() => asks[childTaskId!]?.some(({ ask }) => ask === "followup") ?? false)
206+
207+
// Send the answer, then wait for TaskDelegationCompleted. That event fires after
208+
// atomicUpdatePair writes the persisted history but before the parent is re-created,
209+
// so it is the right gate for history assertions. waitUntilCompleted alone is not
210+
// sufficient because the resumed parent runs into a mock 404 and never emits
211+
// TaskCompleted in the test environment.
212+
await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER)
213+
await waitFor(() => delegationCompletedParentId !== undefined)
214+
215+
assert.strictEqual(
216+
delegationCompletedParentId,
217+
parentTaskId,
218+
"TaskDelegationCompleted should fire for parent",
219+
)
220+
assert.strictEqual(delegationCompletedChildId, childTaskId, "TaskDelegationCompleted should fire for child")
221+
assert.strictEqual(delegationCompletedSummary, "9", "TaskDelegationCompleted summary should be '9'")
222+
223+
const parent = await api.getTaskHistoryItem(parentTaskId)
224+
assert.ok(parent, "Parent history item should exist")
225+
assert.strictEqual(parent.status, "active", "Parent status should be 'active' after child completes")
226+
assert.strictEqual(parent.awaitingChildId, undefined, "Parent awaitingChildId should be cleared")
227+
assert.strictEqual(parent.delegatedToId, undefined, "Parent delegatedToId should be cleared")
228+
assert.strictEqual(parent.completedByChildId, childTaskId, "Parent completedByChildId should be the child")
229+
assert.strictEqual(parent.completionResultSummary, "9", "Parent completionResultSummary should be '9'")
230+
assert.ok(parent.childIds?.includes(childTaskId!), "Parent childIds should include the child")
231+
232+
const child = await api.getTaskHistoryItem(childTaskId!)
233+
assert.ok(child, "Child history item should exist")
234+
assert.strictEqual(child.status, "completed", "Child status should be 'completed'")
235+
assert.strictEqual(child.parentTaskId, parentTaskId, "Child parentTaskId should point to parent")
236+
assert.strictEqual(child.completionResultSummary, "9", "Child completionResultSummary should be '9'")
237+
} finally {
238+
api.off(RooCodeEventName.Message, messageHandler)
239+
api.off(RooCodeEventName.TaskDelegationCompleted, delegationCompletedHandler)
240+
if (api.getCurrentTaskStack().length > 0) {
241+
await api.clearCurrentTask()
242+
}
243+
if (api.getCurrentTaskStack().length > 0) {
244+
await api.clearCurrentTask()
245+
}
246+
await waitFor(() => api.getCurrentTaskStack().length === 0).catch(() => {})
247+
}
248+
})
249+
153250
// Race mitigation: skipDelegationRepair prevents removeClineFromStack from
154251
// auto-resuming the parent when the child is cancelled (Race 2).
155252
test("parent stays paused after subtask cancellation", async () => {

packages/types/src/api.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { Socket } from "net"
33

44
import type { RooCodeEvents } from "./events.js"
55
import type { RooCodeSettings } from "./global-settings.js"
6+
import type { HistoryItem } from "./history.js"
67
import type { ProviderSettingsEntry, ProviderSettings } from "./provider-settings.js"
78
import type { IpcMessage, IpcServerEvents } from "./ipc.js"
89

@@ -38,6 +39,12 @@ export interface RooCodeAPI extends EventEmitter<RooCodeAPIEvents> {
3839
* @returns True if the task is in the task history, false otherwise.
3940
*/
4041
isTaskInHistory(taskId: string): Promise<boolean>
42+
/**
43+
* Returns the HistoryItem for a task by ID. Intended for use in tests only.
44+
* @param taskId The ID of the task.
45+
* @returns The HistoryItem, or undefined if not found.
46+
*/
47+
getTaskHistoryItem(taskId: string): Promise<HistoryItem | undefined>
4148
/**
4249
* Returns the current task stack.
4350
* @returns An array of task IDs.

src/__tests__/helpers/provider-stub.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ export function makeProviderStub<T extends object>(stub: T): T {
1212
const proto = ClineProvider.prototype as any
1313
s.delegationTransitionLocks ??= new Map()
1414
s.cancelledDelegationChildIds ??= new Set()
15+
s.log ??= vi.fn()
1516
s.runDelegationTransition = proto.runDelegationTransition.bind(s)
1617
return s
1718
}

src/__tests__/history-resume-delegation.spec.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,15 @@ vi.mock("vscode", () => {
3030
vi.mock("../core/task-persistence/taskMessages", () => ({
3131
readTaskMessages: vi.fn().mockResolvedValue([]),
3232
}))
33-
vi.mock("../core/task-persistence", () => ({
34-
readApiMessages: vi.fn().mockResolvedValue([]),
35-
saveApiMessages: vi.fn().mockResolvedValue(undefined),
36-
saveTaskMessages: vi.fn().mockResolvedValue(undefined),
37-
}))
33+
vi.mock("../core/task-persistence", async (importOriginal) => {
34+
const real = await importOriginal<typeof import("../core/task-persistence")>()
35+
return {
36+
...real,
37+
readApiMessages: vi.fn().mockResolvedValue([]),
38+
saveApiMessages: vi.fn().mockResolvedValue(undefined),
39+
saveTaskMessages: vi.fn().mockResolvedValue(undefined),
40+
}
41+
})
3842

3943
import { ClineProvider } from "../core/webview/ClineProvider"
4044
import { readTaskMessages } from "../core/task-persistence/taskMessages"
@@ -130,9 +134,11 @@ describe("History resume delegation - parent metadata transitions", () => {
130134
expect(firstId).toBe("child-1")
131135
expect(secondId).toBe("parent-1")
132136

133-
// Verify child updater produces completed status
137+
// Verify child updater produces completed status and persists completionResultSummary
138+
// so startup reconciliation has the real result if the parent write fails.
134139
const updatedChild = firstUpdater({ id: "child-1", status: "active" } as HistoryItem)
135140
expect(updatedChild.status).toBe("completed")
141+
expect(updatedChild.completionResultSummary).toBe("Child done")
136142

137143
// Verify parent updater produces active status with correct fields
138144
const updatedParent = secondUpdater(parentHistoryItem as HistoryItem)
@@ -142,6 +148,7 @@ describe("History resume delegation - parent metadata transitions", () => {
142148
completedByChildId: "child-1",
143149
completionResultSummary: "Child done",
144150
awaitingChildId: undefined,
151+
delegatedToId: undefined,
145152
childIds: ["child-1"],
146153
})
147154

src/__tests__/nested-delegation-resume.spec.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ vi.mock("vscode", () => {
4444
vi.mock("../core/task-persistence/taskMessages", () => ({
4545
readTaskMessages: vi.fn().mockResolvedValue([]),
4646
}))
47-
vi.mock("../core/task-persistence", () => ({
47+
vi.mock("../core/task-persistence", async (importOriginal) => ({
48+
...(await importOriginal<typeof import("../core/task-persistence")>()),
4849
readApiMessages: vi.fn().mockResolvedValue([]),
4950
saveApiMessages: vi.fn().mockResolvedValue(undefined),
5051
saveTaskMessages: vi.fn().mockResolvedValue(undefined),

src/__tests__/provider-delegation.spec.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,11 +226,20 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
226226
atomicReadAndUpdate: vi.fn().mockRejectedValue(persistError),
227227
})
228228

229+
const child = { taskId: "child-1", start: childStart }
230+
// Before createTask: getCurrentTask returns parent (used by step 3 close).
231+
// After createTask: returns child so the rollback guard passes and the child is popped.
232+
const getCurrentTask = vi.fn().mockReturnValue(parentTask)
233+
const createTask = vi.fn().mockImplementation(async () => {
234+
getCurrentTask.mockReturnValue(child)
235+
return child
236+
})
237+
229238
const provider = {
230239
emit: vi.fn(),
231-
getCurrentTask: vi.fn(() => parentTask),
240+
getCurrentTask,
232241
removeClineFromStack,
233-
createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }),
242+
createTask,
234243
getTaskWithId,
235244
handleModeSwitch: vi.fn().mockResolvedValue(undefined),
236245
deleteTaskWithId,

src/__tests__/removeClineFromStack-delegation.spec.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
7979
id: "parent-1",
8080
status: "active",
8181
awaitingChildId: undefined,
82+
delegatedToId: undefined,
8283
}),
8384
)
8485

0 commit comments

Comments
 (0)