Skip to content

Commit 45c94f3

Browse files
edelaunaedelauna
committed
chore: address delegation review cleanup
1 parent e82b18c commit 45c94f3

5 files changed

Lines changed: 61 additions & 38 deletions

File tree

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

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

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

@@ -10,6 +11,22 @@ export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task
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: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,14 +143,15 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
143143
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"])
144144
})
145145

146-
it("does not start the child when parent delegation metadata cannot be persisted", async () => {
146+
it("removes the paused child when parent delegation metadata cannot be persisted", async () => {
147147
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
148148
const childStart = vi.fn()
149149
const persistError = new Error("history write failed")
150150

151151
const providerEmit = vi.fn()
152152
const updateTaskHistory = vi.fn().mockRejectedValue(persistError)
153153
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
154+
const deleteTaskFromState = vi.fn().mockResolvedValue(undefined)
154155
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart })
155156
const handleModeSwitch = vi.fn().mockResolvedValue(undefined)
156157
const getTaskWithId = vi.fn().mockResolvedValue({
@@ -171,6 +172,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
171172
createTask,
172173
getTaskWithId,
173174
updateTaskHistory,
175+
deleteTaskFromState,
174176
handleModeSwitch,
175177
log: vi.fn(),
176178
} as unknown as ClineProvider
@@ -190,6 +192,9 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
190192
startTask: false,
191193
})
192194
expect(childStart).not.toHaveBeenCalled()
195+
expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipDelegationRepair: true })
196+
expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipDelegationRepair: true })
197+
expect(deleteTaskFromState).toHaveBeenCalledWith("child-1")
193198
expect(providerEmit).not.toHaveBeenCalledWith(RooCodeEventName.TaskDelegated, "parent-1", "child-1")
194199
})
195200
})

‎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: 23 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
}
@@ -3266,6 +3269,24 @@ export class ClineProvider
32663269
(err as Error)?.message ?? String(err)
32673270
}`,
32683271
)
3272+
try {
3273+
await this.removeClineFromStack({ skipDelegationRepair: true })
3274+
} catch (cleanupErr) {
3275+
this.log(
3276+
`[delegateParentAndOpenChild] Failed to remove child ${child.taskId} after parent metadata persistence failure (non-fatal): ${
3277+
(cleanupErr as Error)?.message ?? String(cleanupErr)
3278+
}`,
3279+
)
3280+
}
3281+
try {
3282+
await this.deleteTaskFromState(child.taskId)
3283+
} catch (cleanupErr) {
3284+
this.log(
3285+
`[delegateParentAndOpenChild] Failed to remove child ${child.taskId} history after parent metadata persistence failure (non-fatal): ${
3286+
(cleanupErr as Error)?.message ?? String(cleanupErr)
3287+
}`,
3288+
)
3289+
}
32693290
throw err
32703291
}
32713292

0 commit comments

Comments
 (0)