Skip to content

Commit 18d0ad5

Browse files
edelaunaedelauna
committed
fix: delegated subtask lifecycle races
1 parent 169423d commit 18d0ad5

5 files changed

Lines changed: 419 additions & 194 deletions

File tree

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

Lines changed: 113 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ describe("History resume delegation - parent metadata transitions", () => {
110110

111111
// Verify child closed and parent reopened with updated metadata
112112
expect(removeClineFromStack).toHaveBeenCalledTimes(1)
113+
expect(removeClineFromStack).toHaveBeenCalledWith({ skipDelegationRepair: true })
113114
expect(createTaskWithHistoryItem).toHaveBeenCalledWith(
114115
expect.objectContaining({
115116
status: "active",
@@ -502,7 +503,7 @@ describe("History resume delegation - parent metadata transitions", () => {
502503
childTaskId: "child-rpd06",
503504
completionResultSummary: "Subtask finished despite overwrite failures",
504505
}),
505-
).resolves.toBeUndefined()
506+
).resolves.toBe(true)
506507

507508
expect(parentInstance.overwriteClineMessages).toHaveBeenCalledTimes(1)
508509
expect(parentInstance.overwriteApiConversationHistory).toHaveBeenCalledTimes(1)
@@ -705,7 +706,7 @@ describe("History resume delegation - parent metadata transitions", () => {
705706
childTaskId: "child-rpd04",
706707
completionResultSummary: "Child completion with persistence failure",
707708
}),
708-
).resolves.toBeUndefined()
709+
).resolves.toBe(true)
709710

710711
expect(logSpy).toHaveBeenCalledWith(
711712
expect.stringContaining(
@@ -760,7 +761,7 @@ describe("History resume delegation - parent metadata transitions", () => {
760761
childTaskId: "c5",
761762
completionResultSummary: "Result",
762763
}),
763-
).resolves.toBeUndefined()
764+
).resolves.toBe(true)
764765

765766
// Verify saves still occurred with just the injected message
766767
expect(saveTaskMessages).toHaveBeenCalledWith(
@@ -784,4 +785,113 @@ describe("History resume delegation - parent metadata transitions", () => {
784785
}),
785786
)
786787
})
788+
789+
it("reopenParentFromDelegation aborts when parent is already active (stale-delegation guard)", async () => {
790+
const logSpy = vi.fn()
791+
const updateTaskHistory = vi.fn()
792+
const saveTaskMessagesMock = vi.mocked(saveTaskMessages)
793+
const saveApiMessagesMock = vi.mocked(saveApiMessages)
794+
795+
const makeProvider = (historyItem: object) =>
796+
({
797+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
798+
getTaskWithId: vi.fn().mockResolvedValue({ historyItem }),
799+
emit: vi.fn(),
800+
log: logSpy,
801+
getCurrentTask: vi.fn(() => null),
802+
removeClineFromStack: vi.fn(),
803+
createTaskWithHistoryItem: vi.fn(),
804+
updateTaskHistory,
805+
}) as unknown as ClineProvider
806+
807+
const providerActive = makeProvider({
808+
id: "parent-guard",
809+
status: "active",
810+
awaitingChildId: undefined,
811+
})
812+
await expect(
813+
(ClineProvider.prototype as any).reopenParentFromDelegation.call(providerActive, {
814+
parentTaskId: "parent-guard",
815+
childTaskId: "child-guard",
816+
completionResultSummary: "should be ignored",
817+
}),
818+
).resolves.toBe(false)
819+
expect(saveTaskMessagesMock).not.toHaveBeenCalled()
820+
expect(saveApiMessagesMock).not.toHaveBeenCalled()
821+
expect(updateTaskHistory).not.toHaveBeenCalled()
822+
expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting"))
823+
})
824+
825+
it("reopenParentFromDelegation aborts when in-process cancellation failed closed", async () => {
826+
const logSpy = vi.fn()
827+
const updateTaskHistory = vi.fn()
828+
const saveTaskMessagesMock = vi.mocked(saveTaskMessages)
829+
const saveApiMessagesMock = vi.mocked(saveApiMessages)
830+
const provider = {
831+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
832+
getTaskWithId: vi.fn().mockResolvedValue({
833+
historyItem: {
834+
id: "parent-guard",
835+
status: "delegated",
836+
awaitingChildId: "child-guard",
837+
},
838+
}),
839+
emit: vi.fn(),
840+
log: logSpy,
841+
getCurrentTask: vi.fn(() => null),
842+
removeClineFromStack: vi.fn(),
843+
createTaskWithHistoryItem: vi.fn(),
844+
updateTaskHistory,
845+
cancelledDelegationChildIds: new Set(["child-guard"]),
846+
} as unknown as ClineProvider
847+
848+
await expect(
849+
(ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, {
850+
parentTaskId: "parent-guard",
851+
childTaskId: "child-guard",
852+
completionResultSummary: "should be ignored",
853+
}),
854+
).resolves.toBe(false)
855+
856+
expect(saveTaskMessagesMock).not.toHaveBeenCalled()
857+
expect(saveApiMessagesMock).not.toHaveBeenCalled()
858+
expect(updateTaskHistory).not.toHaveBeenCalled()
859+
expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting"))
860+
})
861+
862+
it("reopenParentFromDelegation aborts when parent awaits a different child (stale-delegation guard)", async () => {
863+
const logSpy = vi.fn()
864+
const updateTaskHistory = vi.fn()
865+
const saveTaskMessagesMock = vi.mocked(saveTaskMessages)
866+
const saveApiMessagesMock = vi.mocked(saveApiMessages)
867+
868+
const makeProvider = (historyItem: object) =>
869+
({
870+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
871+
getTaskWithId: vi.fn().mockResolvedValue({ historyItem }),
872+
emit: vi.fn(),
873+
log: logSpy,
874+
getCurrentTask: vi.fn(() => null),
875+
removeClineFromStack: vi.fn(),
876+
createTaskWithHistoryItem: vi.fn(),
877+
updateTaskHistory,
878+
}) as unknown as ClineProvider
879+
880+
const providerWrongChild = makeProvider({
881+
id: "parent-guard",
882+
status: "delegated",
883+
awaitingChildId: "other-child",
884+
})
885+
await expect(
886+
(ClineProvider.prototype as any).reopenParentFromDelegation.call(providerWrongChild, {
887+
parentTaskId: "parent-guard",
888+
childTaskId: "child-guard",
889+
completionResultSummary: "should be ignored",
890+
}),
891+
).resolves.toBe(false)
892+
expect(saveTaskMessagesMock).not.toHaveBeenCalled()
893+
expect(saveApiMessagesMock).not.toHaveBeenCalled()
894+
expect(updateTaskHistory).not.toHaveBeenCalled()
895+
expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting"))
896+
})
787897
})

‎src/core/tools/AttemptCompletionTool.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ interface DelegationProvider {
3030
parentTaskId: string
3131
childTaskId: string
3232
completionResultSummary: string
33-
}): Promise<void>
33+
}): Promise<boolean>
3434
}
3535

3636
export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
@@ -178,14 +178,17 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
178178
return "denied"
179179
}
180180

181-
pushToolResult("")
182-
183-
await provider.reopenParentFromDelegation({
181+
const didReopen = await provider.reopenParentFromDelegation({
184182
parentTaskId: task.parentTaskId!,
185183
childTaskId: task.taskId,
186184
completionResultSummary: result,
187185
})
188186

187+
if (didReopen === false) {
188+
return "continue"
189+
}
190+
191+
pushToolResult("")
189192
return "delegated"
190193
}
191194

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

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,7 @@ describe("attemptCompletionTool", () => {
504504
}
505505
throw new Error(`unexpected task id ${id}`)
506506
}),
507-
reopenParentFromDelegation: vi.fn().mockResolvedValue(undefined),
507+
reopenParentFromDelegation: vi.fn().mockResolvedValue(true),
508508
}
509509

510510
Object.assign(mockTask, {
@@ -534,6 +534,57 @@ describe("attemptCompletionTool", () => {
534534
expect(mockPushToolResult).toHaveBeenCalledWith("")
535535
})
536536

537+
it("falls through to standalone completion when parent delegation becomes stale after approval", async () => {
538+
const block: AttemptCompletionToolUse = {
539+
type: "tool_use",
540+
name: "attempt_completion",
541+
params: { result: "9" },
542+
nativeArgs: { result: "9" },
543+
partial: false,
544+
}
545+
const mockProvider = {
546+
getTaskWithId: vi.fn().mockImplementation((id: string) => {
547+
if (id === "child-1") {
548+
return Promise.resolve({ historyItem: { id, status: "active" } })
549+
}
550+
if (id === "parent-1") {
551+
return Promise.resolve({
552+
historyItem: { id, status: "delegated", awaitingChildId: "child-1" },
553+
})
554+
}
555+
throw new Error(`unexpected task id ${id}`)
556+
}),
557+
reopenParentFromDelegation: vi.fn().mockResolvedValue(false),
558+
}
559+
560+
Object.assign(mockTask, {
561+
taskId: "child-1",
562+
parentTaskId: "parent-1",
563+
providerRef: { deref: () => mockProvider },
564+
})
565+
mockTask.ask = vi.fn().mockResolvedValue({ response: "messageResponse", text: "revise", images: [] })
566+
mockAskFinishSubTaskApproval.mockResolvedValue(true)
567+
568+
const callbacks: AttemptCompletionCallbacks = {
569+
askApproval: mockAskApproval,
570+
handleError: mockHandleError,
571+
pushToolResult: mockPushToolResult,
572+
askFinishSubTaskApproval: mockAskFinishSubTaskApproval,
573+
toolDescription: mockToolDescription,
574+
}
575+
576+
await attemptCompletionTool.handle(mockTask as Task, block, callbacks)
577+
578+
expect(mockProvider.reopenParentFromDelegation).toHaveBeenCalledWith({
579+
parentTaskId: "parent-1",
580+
childTaskId: "child-1",
581+
completionResultSummary: "9",
582+
})
583+
expect(mockTask.ask).toHaveBeenCalledWith("completion_result", "", false)
584+
expect(mockPushToolResult).not.toHaveBeenCalledWith("")
585+
expect(mockCaptureTaskCompleted).not.toHaveBeenCalled()
586+
})
587+
537588
it("does not resume the parent when the parent is no longer awaiting this child", async () => {
538589
const block: AttemptCompletionToolUse = {
539590
type: "tool_use",

0 commit comments

Comments
 (0)