Skip to content

Commit 8def5a4

Browse files
edelaunaedelauna
committed
chore: address delegation review cleanup
1 parent e82b18c commit 8def5a4

5 files changed

Lines changed: 175 additions & 49 deletions

File tree

‎apps/vscode-e2e/src/fixtures/subtasks.ts‎

Lines changed: 22 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,32 @@
11
import { LLMock } from "@copilotkit/aimock"
2+
import type { ChatCompletionRequest } from "@copilotkit/aimock"
23

34
import { toolResultContains } from "./tool-result"
45

56
const SUBTASK_PARENT_MARKER = "SUBTASK_PARENT_CANCELLATION_SMOKE"
67
const SUBTASK_CHILD_MARKER = "SUBTASK_CHILD_CALCULATOR_SMOKE"
78

8-
export 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.`
9+
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.`
910
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.`
1011
export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9"
1112
const INTERRUPTED_TOOL_RESULT = "Task was interrupted before this tool call could be completed."
1213

14+
const completionAfterAnswer = (followupId: string, completionId: string) => ({
15+
match: {
16+
toolCallId: followupId,
17+
predicate: (req: ChatCompletionRequest) => toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]),
18+
},
19+
response: {
20+
toolCalls: [
21+
{
22+
name: "attempt_completion",
23+
arguments: JSON.stringify({ result: "9" }),
24+
id: completionId,
25+
},
26+
],
27+
},
28+
})
29+
1330
export function addSubtaskFixtures(mock: InstanceType<typeof LLMock>) {
1431
mock.addFixture({
1532
match: {
@@ -66,39 +83,11 @@ export function addSubtaskFixtures(mock: InstanceType<typeof LLMock>) {
6683
},
6784
})
6885

69-
mock.addFixture({
70-
match: {
71-
toolCallId: "call_subtasks_child_followup_001",
72-
predicate: (req) =>
73-
toolResultContains(req, "call_subtasks_child_followup_001", [SUBTASK_CHILD_FOLLOWUP_ANSWER]),
74-
},
75-
response: {
76-
toolCalls: [
77-
{
78-
name: "attempt_completion",
79-
arguments: JSON.stringify({ result: "9" }),
80-
id: "call_subtasks_child_completion_002",
81-
},
82-
],
83-
},
84-
})
86+
mock.addFixture(completionAfterAnswer("call_subtasks_child_followup_001", "call_subtasks_child_completion_002"))
8587

86-
mock.addFixture({
87-
match: {
88-
toolCallId: "call_subtasks_child_followup_resume_002",
89-
predicate: (req) =>
90-
toolResultContains(req, "call_subtasks_child_followup_resume_002", [SUBTASK_CHILD_FOLLOWUP_ANSWER]),
91-
},
92-
response: {
93-
toolCalls: [
94-
{
95-
name: "attempt_completion",
96-
arguments: JSON.stringify({ result: "9" }),
97-
id: "call_subtasks_child_completion_resume_003",
98-
},
99-
],
100-
},
101-
})
88+
mock.addFixture(
89+
completionAfterAnswer("call_subtasks_child_followup_resume_002", "call_subtasks_child_completion_resume_003"),
90+
)
10291

10392
mock.addFixture({
10493
match: {

‎src/__tests__/provider-delegation.spec.ts‎

Lines changed: 90 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
9898
it("calls child.start() only after parent metadata is persisted (no race condition)", async () => {
9999
const callOrder: string[] = []
100100

101-
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
101+
const parentTask = {
102+
taskId: "parent-1",
103+
emit: vi.fn(),
104+
getTaskMode: vi.fn().mockResolvedValue("architect"),
105+
} as any
102106
const childStart = vi.fn(() => callOrder.push("child.start"))
103107

104108
const updateTaskHistory = vi.fn(async () => {
@@ -143,25 +147,32 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
143147
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"])
144148
})
145149

146-
it("does not start the child when parent delegation metadata cannot be persisted", async () => {
147-
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
150+
it("removes the paused child when parent delegation metadata cannot be persisted", async () => {
151+
const parentTask = {
152+
taskId: "parent-1",
153+
emit: vi.fn(),
154+
getTaskMode: vi.fn().mockResolvedValue("architect"),
155+
} as any
148156
const childStart = vi.fn()
149157
const persistError = new Error("history write failed")
150158

151159
const providerEmit = vi.fn()
152160
const updateTaskHistory = vi.fn().mockRejectedValue(persistError)
153161
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
162+
const deleteTaskFromState = vi.fn().mockResolvedValue(undefined)
154163
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart })
164+
const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined)
155165
const handleModeSwitch = vi.fn().mockResolvedValue(undefined)
166+
const parentHistory = {
167+
id: "parent-1",
168+
task: "Parent",
169+
tokensIn: 0,
170+
tokensOut: 0,
171+
totalCost: 0,
172+
childIds: [],
173+
}
156174
const getTaskWithId = vi.fn().mockResolvedValue({
157-
historyItem: {
158-
id: "parent-1",
159-
task: "Parent",
160-
tokensIn: 0,
161-
tokensOut: 0,
162-
totalCost: 0,
163-
childIds: [],
164-
},
175+
historyItem: parentHistory,
165176
})
166177

167178
const provider = {
@@ -171,6 +182,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
171182
createTask,
172183
getTaskWithId,
173184
updateTaskHistory,
185+
deleteTaskFromState,
186+
createTaskWithHistoryItem,
174187
handleModeSwitch,
175188
log: vi.fn(),
176189
} as unknown as ClineProvider
@@ -190,6 +203,72 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
190203
startTask: false,
191204
})
192205
expect(childStart).not.toHaveBeenCalled()
206+
expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipDelegationRepair: true })
207+
expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipDelegationRepair: true })
208+
expect(deleteTaskFromState).toHaveBeenCalledWith("child-1")
209+
expect(createTaskWithHistoryItem).toHaveBeenCalledWith(
210+
{ ...parentHistory, mode: "architect" },
211+
{ startTask: false },
212+
)
193213
expect(providerEmit).not.toHaveBeenCalledWith(RooCodeEventName.TaskDelegated, "parent-1", "child-1")
194214
})
215+
216+
it("logs rollback cleanup failures while preserving the parent metadata error", async () => {
217+
const parentTask = {
218+
taskId: "parent-1",
219+
emit: vi.fn(),
220+
getTaskMode: vi.fn().mockRejectedValue(new Error("mode unavailable")),
221+
} as any
222+
const childStart = vi.fn()
223+
const persistError = new Error("history write failed")
224+
const log = vi.fn()
225+
226+
const removeClineFromStack = vi
227+
.fn()
228+
.mockResolvedValueOnce(undefined)
229+
.mockRejectedValueOnce(new Error("child stack cleanup failed"))
230+
const deleteTaskFromState = vi.fn().mockRejectedValue(new Error("child history cleanup failed"))
231+
const createTaskWithHistoryItem = vi.fn().mockRejectedValue(new Error("parent rehydrate failed"))
232+
const parentHistory = {
233+
id: "parent-1",
234+
task: "Parent",
235+
tokensIn: 0,
236+
tokensOut: 0,
237+
totalCost: 0,
238+
childIds: [],
239+
mode: "ask",
240+
}
241+
242+
const provider = {
243+
emit: vi.fn(),
244+
getCurrentTask: vi.fn(() => parentTask),
245+
removeClineFromStack,
246+
createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }),
247+
getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistory }),
248+
updateTaskHistory: vi.fn().mockRejectedValue(persistError),
249+
deleteTaskFromState,
250+
createTaskWithHistoryItem,
251+
handleModeSwitch: vi.fn().mockResolvedValue(undefined),
252+
log,
253+
} as unknown as ClineProvider
254+
255+
await expect(
256+
(ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, {
257+
parentTaskId: "parent-1",
258+
message: "Do something",
259+
initialTodos: [],
260+
mode: "code",
261+
}),
262+
).rejects.toThrow("history write failed")
263+
264+
expect(childStart).not.toHaveBeenCalled()
265+
expect(deleteTaskFromState).toHaveBeenCalledWith("child-1")
266+
expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistory, { startTask: false })
267+
expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to capture parent mode"))
268+
expect(log).toHaveBeenCalledWith(
269+
expect.stringContaining("Failed to remove child child-1 after parent metadata"),
270+
)
271+
expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to remove child child-1 history"))
272+
expect(log).toHaveBeenCalledWith(expect.stringContaining("Failed to rehydrate parent parent-1"))
273+
})
195274
})

‎src/core/tools/AttemptCompletionTool.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
8686
// to prevent duplicate tool_results when user revisits from history
8787
const provider = task.providerRef.deref() as DelegationProvider | undefined
8888
if (provider) {
89+
let historyLookupTaskId = task.taskId
8990
try {
9091
const { historyItem } = await provider.getTaskWithId(task.taskId)
9192
const status = historyItem?.status
@@ -96,9 +97,13 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
9697
// This shows the user the completion result and waits for acceptance
9798
// without injecting another tool_result to the parent
9899
} else if (status === "active") {
100+
historyLookupTaskId = task.parentTaskId
99101
const { historyItem: parentHistory } = await provider.getTaskWithId(task.parentTaskId)
100102

101-
if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === task.taskId) {
103+
if (
104+
parentHistory?.status === "delegated" &&
105+
parentHistory?.awaitingChildId === task.taskId
106+
) {
102107
const delegation = await this.delegateToParent(
103108
task,
104109
result,
@@ -110,6 +115,9 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
110115
this.emitTaskCompleted(task)
111116
}
112117
if (delegation !== "continue") return
118+
} else {
119+
// Parent already detached, such as when the user cancelled this child.
120+
// Fall through to the normal completion ask flow.
113121
}
114122
} else {
115123
// Unexpected status (undefined or "delegated") - log error and skip delegation
@@ -124,7 +132,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
124132
} catch (err) {
125133
// If we can't get the history, log error and skip delegation
126134
console.error(
127-
`[AttemptCompletionTool] Failed to get history for task ${task.taskId}: ${(err as Error)?.message ?? String(err)}. ` +
135+
`[AttemptCompletionTool] Failed to get history for task ${historyLookupTaskId}: ${(err as Error)?.message ?? String(err)}. ` +
128136
`Skipping delegation.`,
129137
)
130138
// Fall through to normal completion ask flow

‎src/core/tools/__tests__/attemptCompletionTool.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,7 @@ describe("attemptCompletionTool", () => {
534534
expect(mockPushToolResult).toHaveBeenCalledWith("")
535535
})
536536

537-
it("does not delegate a lineage-preserving subtask when the parent is no longer awaiting it", async () => {
537+
it("does not resume the parent when the parent is no longer awaiting this child", async () => {
538538
const block: AttemptCompletionToolUse = {
539539
type: "tool_use",
540540
name: "attempt_completion",

‎src/core/webview/ClineProvider.ts‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ export class ClineProvider
484484
try {
485485
const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId)
486486

487-
if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === childTaskId) {
487+
if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) {
488488
await this.updateTaskHistory({
489489
...parentHistory,
490490
status: "active",
@@ -2965,13 +2965,16 @@ export class ClineProvider
29652965
try {
29662966
const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId)
29672967

2968-
if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === task.taskId) {
2968+
if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) {
29692969
await this.updateTaskHistory({
29702970
...parentHistory,
29712971
status: "active",
29722972
awaitingChildId: undefined,
29732973
})
29742974

2975+
this.log(
2976+
`[cancelTask] Detached delegated parent ${task.parentTaskId}: delegated → active (child ${task.taskId} cancelled)`,
2977+
)
29752978
parentTask = undefined
29762979
rootTask = undefined
29772980
}
@@ -3169,6 +3172,16 @@ export class ClineProvider
31693172
`[delegateParentAndOpenChild] Parent mismatch: expected ${parentTaskId}, current ${parent.taskId}`,
31703173
)
31713174
}
3175+
let parentModeBeforeDelegation: string | undefined
3176+
try {
3177+
parentModeBeforeDelegation = await parent.getTaskMode()
3178+
} catch (error) {
3179+
this.log(
3180+
`[delegateParentAndOpenChild] Failed to capture parent mode for ${parentTaskId} before delegation (non-fatal): ${
3181+
error instanceof Error ? error.message : String(error)
3182+
}`,
3183+
)
3184+
}
31723185
// 2) Flush pending tool results to API history BEFORE disposing the parent.
31733186
// This is critical: when tools are called before new_task,
31743187
// their tool_result blocks are in userMessageContent but not yet saved to API history.
@@ -3249,8 +3262,10 @@ export class ClineProvider
32493262
})
32503263

32513264
// 5) Persist parent delegation metadata BEFORE the child starts writing.
3265+
let parentHistoryBeforeDelegation: HistoryItem | undefined
32523266
try {
32533267
const { historyItem } = await this.getTaskWithId(parentTaskId)
3268+
parentHistoryBeforeDelegation = historyItem
32543269
const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId]))
32553270
const updatedHistory: typeof historyItem = {
32563271
...historyItem,
@@ -3266,6 +3281,41 @@ export class ClineProvider
32663281
(err as Error)?.message ?? String(err)
32673282
}`,
32683283
)
3284+
try {
3285+
await this.removeClineFromStack({ skipDelegationRepair: true })
3286+
} catch (cleanupErr) {
3287+
this.log(
3288+
`[delegateParentAndOpenChild] Failed to remove child ${child.taskId} after parent metadata persistence failure (non-fatal): ${
3289+
(cleanupErr as Error)?.message ?? String(cleanupErr)
3290+
}`,
3291+
)
3292+
}
3293+
try {
3294+
await this.deleteTaskFromState(child.taskId)
3295+
} catch (cleanupErr) {
3296+
this.log(
3297+
`[delegateParentAndOpenChild] Failed to remove child ${child.taskId} history after parent metadata persistence failure (non-fatal): ${
3298+
(cleanupErr as Error)?.message ?? String(cleanupErr)
3299+
}`,
3300+
)
3301+
}
3302+
if (parentHistoryBeforeDelegation) {
3303+
try {
3304+
await this.createTaskWithHistoryItem(
3305+
{
3306+
...parentHistoryBeforeDelegation,
3307+
mode: parentModeBeforeDelegation ?? parentHistoryBeforeDelegation.mode,
3308+
},
3309+
{ startTask: false },
3310+
)
3311+
} catch (cleanupErr) {
3312+
this.log(
3313+
`[delegateParentAndOpenChild] Failed to rehydrate parent ${parentTaskId} after parent metadata persistence failure (non-fatal): ${
3314+
(cleanupErr as Error)?.message ?? String(cleanupErr)
3315+
}`,
3316+
)
3317+
}
3318+
}
32693319
throw err
32703320
}
32713321

0 commit comments

Comments
 (0)