feat(ClineProvider): guarding parent, child transitions

Author: edelauna

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

@@ -30,12 +30,15 @@ vi.mock("vscode", () => {
 vi.mock("../core/task-persistence/taskMessages", () => ({
 	readTaskMessages: vi.fn().mockResolvedValue([]),
 }))
-vi.mock("../core/task-persistence", () => ({
-	readApiMessages: vi.fn().mockResolvedValue([]),
-	saveApiMessages: vi.fn().mockResolvedValue(undefined),
-	saveTaskMessages: vi.fn().mockResolvedValue(undefined),
-	assertValidTransition: vi.fn(),
-}))
+vi.mock("../core/task-persistence", async (importOriginal) => {
+	const real = await importOriginal<typeof import("../core/task-persistence")>()
+	return {
+		...real,
+		readApiMessages: vi.fn().mockResolvedValue([]),
+		saveApiMessages: vi.fn().mockResolvedValue(undefined),
+		saveTaskMessages: vi.fn().mockResolvedValue(undefined),
+	}
+})
 
 import { ClineProvider } from "../core/webview/ClineProvider"
 import { readTaskMessages } from "../core/task-persistence/taskMessages"
@@ -131,9 +134,11 @@ describe("History resume delegation - parent metadata transitions", () => {
 		expect(firstId).toBe("child-1")
 		expect(secondId).toBe("parent-1")
 
-		// Verify child updater produces completed status
+		// Verify child updater produces completed status and persists completionResultSummary
+		// so startup reconciliation has the real result if the parent write fails.
 		const updatedChild = firstUpdater({ id: "child-1", status: "active" } as HistoryItem)
 		expect(updatedChild.status).toBe("completed")
+		expect(updatedChild.completionResultSummary).toBe("Child done")
 
 		// Verify parent updater produces active status with correct fields
 		const updatedParent = secondUpdater(parentHistoryItem as HistoryItem)

src/__tests__/provider-delegation.spec.ts

@@ -226,11 +226,20 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
 			atomicReadAndUpdate: vi.fn().mockRejectedValue(persistError),
 		})
 
+		const child = { taskId: "child-1", start: childStart }
+		// Before createTask: getCurrentTask returns parent (used by step 3 close).
+		// After createTask: returns child so the rollback guard passes and the child is popped.
+		const getCurrentTask = vi.fn().mockReturnValue(parentTask)
+		const createTask = vi.fn().mockImplementation(async () => {
+			getCurrentTask.mockReturnValue(child)
+			return child
+		})
+
 		const provider = {
 			emit: vi.fn(),
-			getCurrentTask: vi.fn(() => parentTask),
+			getCurrentTask,
 			removeClineFromStack,
-			createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }),
+			createTask,
 			getTaskWithId,
 			handleModeSwitch: vi.fn().mockResolvedValue(undefined),
 			deleteTaskWithId,

src/core/task-persistence/TaskHistoryStore.ts

@@ -202,8 +202,13 @@ export class TaskHistoryStore {
 		// Enforce transition validity at the write boundary so that any caller
 		// (including fire-and-forget saves) cannot silently stomp a terminal status.
 		// Skip when there is no existing record — first insert has no prior state to transition from.
-		if (!options.skipTransitionCheck && existing && item.status !== undefined && item.status !== existing.status) {
-			assertValidTransition(existing.status, item.status)
+		// Normalize existing.status (undefined = legacy "active") before comparing so that writing
+		// status: "active" onto a legacy item without a status field is not treated as a transition.
+		if (!options.skipTransitionCheck && existing && item.status !== undefined) {
+			const normalizedExisting: HistoryItemStatus = existing.status ?? "active"
+			if (item.status !== normalizedExisting) {
+				assertValidTransition(existing.status, item.status)
+			}
 		}
 
 		// Merge: preserve existing metadata unless explicitly overwritten
@@ -684,7 +689,7 @@ export class TaskHistoryStore {
 					`[TaskHistoryStore] atomicReadAndUpdate: updater changed task id from ${taskId} to ${updated.id}`,
 				)
 			}
-			return this.upsertCore(updated, { skipTransitionCheck: true })
+			return this.upsertCore(updated)
 		})
 	}
 
@@ -722,6 +727,19 @@ export class TaskHistoryStore {
 				)
 			}
 
+			// Validate status transitions before any disk write — mirrors upsertCore guard.
+			for (const [existing, updated] of [
+				[first, updatedFirst],
+				[second, updatedSecond],
+			] as const) {
+				if (updated.status !== undefined) {
+					const normalizedExisting: HistoryItemStatus = existing.status ?? "active"
+					if (updated.status !== normalizedExisting) {
+						assertValidTransition(existing.status, updated.status)
+					}
+				}
+			}
+
 			// Merge with existing cache entries before writing, mirroring upsertCore.
 			const mergedFirst = { ...first, ...updatedFirst }
 			const mergedSecond = { ...second, ...updatedSecond }

src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts

@@ -89,6 +89,10 @@ describe("assertValidTransition", () => {
 				"Invalid task status transition: active → active",
 			)
 		})
+
+		it("undefined (implicit active) → delegated is valid", () => {
+			expect(() => assertValidTransition(undefined, "delegated")).not.toThrow()
+		})
 	})
 })
 
@@ -376,6 +380,20 @@ describe("TaskHistoryStore upsert transition guard", () => {
 		expect(store.get("task-guard-new")?.status).toBe("active")
 	})
 
+	it("allows writing status: active over a legacy item with status: undefined (implicit active → active no-op)", async () => {
+		// Legacy items pre-dating the status field have status: undefined, which normalizes
+		// to "active". Writing status: "active" must not throw as an invalid self-loop.
+		const item = makeItem({ id: "task-guard-legacy" })
+		delete (item as any).status
+		await seedItems([item])
+		store.dispose()
+		store = new TaskHistoryStore(tmpDir)
+		await store.initialize()
+
+		await expect(store.upsert({ ...item, status: "active" })).resolves.toBeDefined()
+		expect(store.get("task-guard-legacy")?.status).toBe("active")
+	})
+
 	it("allows upsert without a status field (no-op on status)", async () => {
 		const item = makeItem({ id: "task-guard-4", status: "completed" })
 		await seedItems([item])
@@ -389,4 +407,29 @@ describe("TaskHistoryStore upsert transition guard", () => {
 		// Status is preserved from the existing cache entry
 		expect(store.get("task-guard-4")?.status).toBe("completed")
 	})
+
+	it("atomicReadAndUpdate enforces the upsertCore transition guard on status changes", async () => {
+		// atomicReadAndUpdate now flows through upsertCore without skipTransitionCheck,
+		// so invalid transitions are rejected at the store boundary.
+		const item = makeItem({ id: "task-atomic-guard", status: "active" })
+		await store.upsert(item)
+
+		// active → delegated via atomicReadAndUpdate — valid, must succeed
+		await expect(
+			store.atomicReadAndUpdate("task-atomic-guard", (current) => ({
+				...current,
+				status: "delegated" as const,
+				awaitingChildId: "some-child",
+			})),
+		).resolves.toBeDefined()
+		expect(store.get("task-atomic-guard")?.status).toBe("delegated")
+
+		// delegated → completed via atomicReadAndUpdate — invalid, must throw
+		await expect(
+			store.atomicReadAndUpdate("task-atomic-guard", (current) => ({
+				...current,
+				status: "completed" as const,
+			})),
+		).rejects.toThrow("Invalid task status transition: delegated → completed")
+	})
 })

src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts

@@ -6,7 +6,7 @@ import * as os from "os"
 
 import type { HistoryItem } from "@roo-code/types"
 
-import { TaskHistoryStore } from "../TaskHistoryStore"
+import { TaskHistoryStore, assertValidTransition } from "../TaskHistoryStore"
 import { GlobalFileNames } from "../../../shared/globalFileNames"
 
 vi.mock("../../../utils/storage", () => ({
@@ -641,5 +641,78 @@ describe("TaskHistoryStore", () => {
 
 			storeWithCallback.dispose()
 		})
+
+		it("propagates throw from first updater — neither record is written", async () => {
+			await store.initialize()
+
+			const child = makeHistoryItem({ id: "child-updater-throw", status: "active" })
+			const parent = makeHistoryItem({ id: "parent-updater-throw", status: "delegated" })
+			await store.upsert(child)
+			await store.upsert(parent)
+
+			await expect(
+				store.atomicUpdatePair(
+					"child-updater-throw",
+					"parent-updater-throw",
+					(_c) => {
+						throw new Error("updater exploded")
+					},
+					(p) => p,
+				),
+			).rejects.toThrow("updater exploded")
+
+			// Neither record should have been modified
+			expect(store.get("child-updater-throw")?.status).toBe("active")
+			expect(store.get("parent-updater-throw")?.status).toBe("delegated")
+		})
+
+		it("transition guard in first updater: delegated → completed throws assertValidTransition", async () => {
+			await store.initialize()
+
+			const child = makeHistoryItem({ id: "child-guard-pair", status: "delegated" })
+			const parent = makeHistoryItem({ id: "parent-guard-pair", status: "active" })
+			await store.upsert(child)
+			await store.upsert(parent)
+
+			await expect(
+				store.atomicUpdatePair(
+					"child-guard-pair",
+					"parent-guard-pair",
+					(c) => {
+						assertValidTransition(c.status, "completed")
+						return { ...c, status: "completed" as const }
+					},
+					(p) => p,
+				),
+			).rejects.toThrow("Invalid task status transition: delegated → completed")
+
+			// Original statuses preserved
+			expect(store.get("child-guard-pair")?.status).toBe("delegated")
+			expect(store.get("parent-guard-pair")?.status).toBe("active")
+		})
+
+		it("store-level guard in atomicUpdatePair rejects invalid transition even without explicit updater assertion", async () => {
+			await store.initialize()
+
+			const child = makeHistoryItem({ id: "child-store-guard", status: "delegated" })
+			const parent = makeHistoryItem({ id: "parent-store-guard", status: "active" })
+			await store.upsert(child)
+			await store.upsert(parent)
+
+			// Updater returns delegated → completed without calling assertValidTransition.
+			// The store's internal guard must still catch this.
+			await expect(
+				store.atomicUpdatePair(
+					"child-store-guard",
+					"parent-store-guard",
+					(c) => ({ ...c, status: "completed" as const }),
+					(p) => p,
+				),
+			).rejects.toThrow("Invalid task status transition: delegated → completed")
+
+			// Neither record modified
+			expect(store.get("child-store-guard")?.status).toBe("delegated")
+			expect(store.get("parent-store-guard")?.status).toBe("active")
+		})
 	})
 })

src/core/webview/ClineProvider.ts

@@ -2348,9 +2348,8 @@ export class ClineProvider
 		}
 
 		try {
-			const { isZooCodeAuthenticated, getCachedZooCodeUserInfo, getZooCodeBaseUrl } = await import(
-				"../../services/zoo-code-auth"
-			)
+			const { isZooCodeAuthenticated, getCachedZooCodeUserInfo, getZooCodeBaseUrl } =
+				await import("../../services/zoo-code-auth")
 			const userInfo = getCachedZooCodeUserInfo()
 			zooCodeState = {
 				zooCodeIsAuthenticated: await isZooCodeAuthenticated(),
@@ -3778,8 +3777,14 @@ export class ClineProvider
 			await this.taskHistoryStore.atomicUpdatePair(
 				childTaskId,
 				parentTaskId,
-				(child) => ({ ...child, status: "completed" as const }),
+				(child) => {
+					assertValidTransition(child.status, "completed")
+					return { ...child, status: "completed" as const, completionResultSummary }
+				},
 				(parent) => {
+					if (parent.status !== "active") {
+						assertValidTransition(parent.status, "active")
+					}
 					const childIds = Array.from(new Set([...(parent.childIds ?? []), childTaskId]))
 					updatedHistory = {
 						...parent,
@@ -3803,7 +3808,8 @@ export class ClineProvider
 				}
 				if (updatedParent) {
 					await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedParent })
-				}			}
+				}
+			}
 
 			// 6) Emit TaskDelegationCompleted (provider-level)
 			try {