mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: make removeClineFromStack() delegation-aware to prevent orphaned parent tasks (#11302)
* fix: make removeClineFromStack() delegation-aware to prevent orphaned parent tasks When a delegated child task is removed via removeClineFromStack() (e.g., Clear Task, navigate to history, start new task), the parent task was left orphaned in "delegated" status with a stale awaitingChildId. This made the parent unresumable without manual history repair. This fix captures parentTaskId and childTaskId before abort/dispose, then repairs the parent metadata (status -> active, clear awaitingChildId) when the popped task is a delegated child and awaitingChildId matches. Parent lookup + updateTaskHistory are wrapped in try/catch so failures are non-fatal (logged but do not block the pop). Closes #11301 * fix: add skipDelegationRepair opt-out to removeClineFromStack() for nested delegation --------- Co-authored-by: Roo Code <roomote@roocode.com>
This commit is contained in:
parent
bb86eb8651
commit
70775f0ec1
2 changed files with 319 additions and 2 deletions
281
src/__tests__/removeClineFromStack-delegation.spec.ts
Normal file
281
src/__tests__/removeClineFromStack-delegation.spec.ts
Normal file
|
|
@ -0,0 +1,281 @@
|
|||
// npx vitest run __tests__/removeClineFromStack-delegation.spec.ts
|
||||
|
||||
import { describe, it, expect, vi } from "vitest"
|
||||
import { ClineProvider } from "../core/webview/ClineProvider"
|
||||
|
||||
describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
|
||||
/**
|
||||
* Helper to build a minimal mock provider with a single task on the stack.
|
||||
* The task's parentTaskId and taskId are configurable.
|
||||
*/
|
||||
function buildMockProvider(opts: {
|
||||
childTaskId: string
|
||||
parentTaskId?: string
|
||||
parentHistoryItem?: Record<string, any>
|
||||
getTaskWithIdError?: Error
|
||||
}) {
|
||||
const childTask = {
|
||||
taskId: opts.childTaskId,
|
||||
instanceId: "inst-1",
|
||||
parentTaskId: opts.parentTaskId,
|
||||
emit: vi.fn(),
|
||||
abortTask: vi.fn().mockResolvedValue(undefined),
|
||||
}
|
||||
|
||||
const updateTaskHistory = vi.fn().mockResolvedValue([])
|
||||
const getTaskWithId = opts.getTaskWithIdError
|
||||
? vi.fn().mockRejectedValue(opts.getTaskWithIdError)
|
||||
: vi.fn().mockImplementation(async (id: string) => {
|
||||
if (id === opts.parentTaskId && opts.parentHistoryItem) {
|
||||
return { historyItem: { ...opts.parentHistoryItem } }
|
||||
}
|
||||
throw new Error("Task not found")
|
||||
})
|
||||
|
||||
const provider = {
|
||||
clineStack: [childTask] as any[],
|
||||
taskEventListeners: new Map(),
|
||||
log: vi.fn(),
|
||||
getTaskWithId,
|
||||
updateTaskHistory,
|
||||
}
|
||||
|
||||
return { provider, childTask, updateTaskHistory, getTaskWithId }
|
||||
}
|
||||
|
||||
it("repairs parent metadata (delegated → active) when a delegated child is removed", async () => {
|
||||
const { provider, updateTaskHistory, getTaskWithId } = buildMockProvider({
|
||||
childTaskId: "child-1",
|
||||
parentTaskId: "parent-1",
|
||||
parentHistoryItem: {
|
||||
id: "parent-1",
|
||||
task: "Parent task",
|
||||
ts: 1000,
|
||||
number: 1,
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
status: "delegated",
|
||||
awaitingChildId: "child-1",
|
||||
delegatedToId: "child-1",
|
||||
childIds: ["child-1"],
|
||||
},
|
||||
})
|
||||
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider)
|
||||
|
||||
// Stack should be empty after pop
|
||||
expect(provider.clineStack).toHaveLength(0)
|
||||
|
||||
// Parent lookup should have been called
|
||||
expect(getTaskWithId).toHaveBeenCalledWith("parent-1")
|
||||
|
||||
// Parent metadata should be repaired
|
||||
expect(updateTaskHistory).toHaveBeenCalledTimes(1)
|
||||
const updatedParent = updateTaskHistory.mock.calls[0][0]
|
||||
expect(updatedParent).toEqual(
|
||||
expect.objectContaining({
|
||||
id: "parent-1",
|
||||
status: "active",
|
||||
awaitingChildId: undefined,
|
||||
}),
|
||||
)
|
||||
|
||||
// Log the repair
|
||||
expect(provider.log).toHaveBeenCalledWith(expect.stringContaining("Repaired parent parent-1 metadata"))
|
||||
})
|
||||
|
||||
it("does NOT modify parent metadata when the task has no parentTaskId (non-delegated)", async () => {
|
||||
const { provider, updateTaskHistory, getTaskWithId } = buildMockProvider({
|
||||
childTaskId: "standalone-1",
|
||||
// No parentTaskId — this is a top-level task
|
||||
})
|
||||
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider)
|
||||
|
||||
// Stack should be empty
|
||||
expect(provider.clineStack).toHaveLength(0)
|
||||
|
||||
// No parent lookup or update should happen
|
||||
expect(getTaskWithId).not.toHaveBeenCalled()
|
||||
expect(updateTaskHistory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("does NOT modify parent metadata when awaitingChildId does not match the popped child", async () => {
|
||||
const { provider, updateTaskHistory, getTaskWithId } = buildMockProvider({
|
||||
childTaskId: "child-1",
|
||||
parentTaskId: "parent-1",
|
||||
parentHistoryItem: {
|
||||
id: "parent-1",
|
||||
task: "Parent task",
|
||||
ts: 1000,
|
||||
number: 1,
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
status: "delegated",
|
||||
awaitingChildId: "child-OTHER", // different child
|
||||
delegatedToId: "child-OTHER",
|
||||
childIds: ["child-OTHER"],
|
||||
},
|
||||
})
|
||||
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider)
|
||||
|
||||
// Parent was looked up but should NOT be updated
|
||||
expect(getTaskWithId).toHaveBeenCalledWith("parent-1")
|
||||
expect(updateTaskHistory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("does NOT modify parent metadata when parent status is not 'delegated'", async () => {
|
||||
const { provider, updateTaskHistory, getTaskWithId } = buildMockProvider({
|
||||
childTaskId: "child-1",
|
||||
parentTaskId: "parent-1",
|
||||
parentHistoryItem: {
|
||||
id: "parent-1",
|
||||
task: "Parent task",
|
||||
ts: 1000,
|
||||
number: 1,
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
status: "completed", // already completed
|
||||
awaitingChildId: "child-1",
|
||||
childIds: ["child-1"],
|
||||
},
|
||||
})
|
||||
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider)
|
||||
|
||||
expect(getTaskWithId).toHaveBeenCalledWith("parent-1")
|
||||
expect(updateTaskHistory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("catches and logs errors during parent metadata repair without blocking the pop", async () => {
|
||||
const { provider, childTask, updateTaskHistory, getTaskWithId } = buildMockProvider({
|
||||
childTaskId: "child-1",
|
||||
parentTaskId: "parent-1",
|
||||
getTaskWithIdError: new Error("Storage unavailable"),
|
||||
})
|
||||
|
||||
// Should NOT throw
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider)
|
||||
|
||||
// Stack should still be empty (pop was not blocked)
|
||||
expect(provider.clineStack).toHaveLength(0)
|
||||
|
||||
// The abort should still have been called
|
||||
expect(childTask.abortTask).toHaveBeenCalledWith(true)
|
||||
|
||||
// Error should be logged as non-fatal
|
||||
expect(provider.log).toHaveBeenCalledWith(
|
||||
expect.stringContaining("Failed to repair parent metadata for parent-1 (non-fatal)"),
|
||||
)
|
||||
|
||||
// No update should have been attempted
|
||||
expect(updateTaskHistory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("handles empty stack gracefully", async () => {
|
||||
const provider = {
|
||||
clineStack: [] as any[],
|
||||
taskEventListeners: new Map(),
|
||||
log: vi.fn(),
|
||||
getTaskWithId: vi.fn(),
|
||||
updateTaskHistory: vi.fn(),
|
||||
}
|
||||
|
||||
// Should not throw
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider)
|
||||
|
||||
expect(provider.clineStack).toHaveLength(0)
|
||||
expect(provider.getTaskWithId).not.toHaveBeenCalled()
|
||||
expect(provider.updateTaskHistory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("skips delegation repair when skipDelegationRepair option is true", async () => {
|
||||
const { provider, updateTaskHistory, getTaskWithId } = buildMockProvider({
|
||||
childTaskId: "child-1",
|
||||
parentTaskId: "parent-1",
|
||||
parentHistoryItem: {
|
||||
id: "parent-1",
|
||||
task: "Parent task",
|
||||
ts: 1000,
|
||||
number: 1,
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
status: "delegated",
|
||||
awaitingChildId: "child-1",
|
||||
delegatedToId: "child-1",
|
||||
childIds: ["child-1"],
|
||||
},
|
||||
})
|
||||
|
||||
// Call with skipDelegationRepair: true (as delegateParentAndOpenChild would)
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider, { skipDelegationRepair: true })
|
||||
|
||||
// Stack should be empty after pop
|
||||
expect(provider.clineStack).toHaveLength(0)
|
||||
|
||||
// Parent lookup should NOT have been called — repair was skipped entirely
|
||||
expect(getTaskWithId).not.toHaveBeenCalled()
|
||||
expect(updateTaskHistory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("does NOT reset grandparent during A→B→C nested delegation transition", async () => {
|
||||
// Scenario: A delegated to B, B is now delegating to C.
|
||||
// delegateParentAndOpenChild() pops B via removeClineFromStack({ skipDelegationRepair: true }).
|
||||
// Grandparent A should remain "delegated" — its metadata must not be repaired.
|
||||
const grandparentHistory = {
|
||||
id: "task-A",
|
||||
task: "Grandparent task",
|
||||
ts: 1000,
|
||||
number: 1,
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
status: "delegated",
|
||||
awaitingChildId: "task-B",
|
||||
delegatedToId: "task-B",
|
||||
childIds: ["task-B"],
|
||||
}
|
||||
|
||||
const taskB = {
|
||||
taskId: "task-B",
|
||||
instanceId: "inst-B",
|
||||
parentTaskId: "task-A",
|
||||
emit: vi.fn(),
|
||||
abortTask: vi.fn().mockResolvedValue(undefined),
|
||||
}
|
||||
|
||||
const getTaskWithId = vi.fn().mockImplementation(async (id: string) => {
|
||||
if (id === "task-A") {
|
||||
return { historyItem: { ...grandparentHistory } }
|
||||
}
|
||||
throw new Error("Task not found")
|
||||
})
|
||||
const updateTaskHistory = vi.fn().mockResolvedValue([])
|
||||
|
||||
const provider = {
|
||||
clineStack: [taskB] as any[],
|
||||
taskEventListeners: new Map(),
|
||||
log: vi.fn(),
|
||||
getTaskWithId,
|
||||
updateTaskHistory,
|
||||
}
|
||||
|
||||
// Simulate what delegateParentAndOpenChild does: pop B with skipDelegationRepair
|
||||
await (ClineProvider.prototype as any).removeClineFromStack.call(provider, { skipDelegationRepair: true })
|
||||
|
||||
// B was popped
|
||||
expect(provider.clineStack).toHaveLength(0)
|
||||
|
||||
// Grandparent A should NOT have been looked up or modified
|
||||
expect(getTaskWithId).not.toHaveBeenCalled()
|
||||
expect(updateTaskHistory).not.toHaveBeenCalled()
|
||||
|
||||
// Grandparent A's metadata remains intact (delegated, awaitingChildId: task-B)
|
||||
// The caller (delegateParentAndOpenChild) will update A to point to C separately.
|
||||
})
|
||||
})
|
||||
|
|
@ -456,7 +456,7 @@ export class ClineProvider
|
|||
|
||||
// Removes and destroys the top Cline instance (the current finished task),
|
||||
// activating the previous one (resuming the parent task).
|
||||
async removeClineFromStack() {
|
||||
async removeClineFromStack(options?: { skipDelegationRepair?: boolean }) {
|
||||
if (this.clineStack.length === 0) {
|
||||
return
|
||||
}
|
||||
|
|
@ -465,6 +465,11 @@ export class ClineProvider
|
|||
let task = this.clineStack.pop()
|
||||
|
||||
if (task) {
|
||||
// Capture delegation metadata before abort/dispose, since abortTask(true)
|
||||
// is async and the task reference is cleared afterwards.
|
||||
const childTaskId = task.taskId
|
||||
const parentTaskId = task.parentTaskId
|
||||
|
||||
task.emit(RooCodeEventName.TaskUnfocused)
|
||||
|
||||
try {
|
||||
|
|
@ -488,6 +493,37 @@ export class ClineProvider
|
|||
// Make sure no reference kept, once promises end it will be
|
||||
// garbage collected.
|
||||
task = undefined
|
||||
|
||||
// Delegation-aware parent metadata repair:
|
||||
// If the popped task was a delegated child, repair the parent's metadata
|
||||
// so it transitions from "delegated" back to "active" and becomes resumable
|
||||
// from the task history list.
|
||||
// Skip when called from delegateParentAndOpenChild() during nested delegation
|
||||
// transitions (A→B→C), where the caller intentionally replaces the active
|
||||
// child and will update the parent to point at the new child.
|
||||
if (parentTaskId && childTaskId && !options?.skipDelegationRepair) {
|
||||
try {
|
||||
const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId)
|
||||
|
||||
if (parentHistory.status === "delegated" && parentHistory.awaitingChildId === childTaskId) {
|
||||
await this.updateTaskHistory({
|
||||
...parentHistory,
|
||||
status: "active",
|
||||
awaitingChildId: undefined,
|
||||
})
|
||||
this.log(
|
||||
`[ClineProvider#removeClineFromStack] Repaired parent ${parentTaskId} metadata: delegated → active (child ${childTaskId} removed)`,
|
||||
)
|
||||
}
|
||||
} catch (err) {
|
||||
// Non-fatal: log but do not block the pop operation.
|
||||
this.log(
|
||||
`[ClineProvider#removeClineFromStack] Failed to repair parent metadata for ${parentTaskId} (non-fatal): ${
|
||||
err instanceof Error ? err.message : String(err)
|
||||
}`,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -3268,7 +3304,7 @@ export class ClineProvider
|
|||
// This ensures we never have >1 tasks open at any time during delegation.
|
||||
// Await abort completion to ensure clean disposal and prevent unhandled rejections.
|
||||
try {
|
||||
await this.removeClineFromStack()
|
||||
await this.removeClineFromStack({ skipDelegationRepair: true })
|
||||
} catch (error) {
|
||||
this.log(
|
||||
`[delegateParentAndOpenChild] Error during parent disposal (non-fatal): ${
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue