fix(task-persistence): enforce status transition guard at upsertCore write boundary

Author: edelauna

src/core/task-persistence/TaskHistoryStore.ts

@@ -9,7 +9,7 @@ import { safeWriteJson } from "../../utils/safeWriteJson"
 import { getStorageBasePath } from "../../utils/storage"
 
 /** Valid status values for a task's HistoryItem. */
-export type HistoryItemStatus = "active" | "delegated" | "completed"
+export type HistoryItemStatus = NonNullable<HistoryItem["status"]>
 
 const VALID_TRANSITIONS: Record<HistoryItemStatus, HistoryItemStatus[]> = {
 	active: ["delegated", "completed"],
@@ -188,10 +188,24 @@ export class TaskHistoryStore {
 
 	/**
 	 * Core upsert logic — must only be called from within `withLock`.
+	 *
+	 * Enforces state-machine transition rules when `item.status` changes.
+	 * Pass `skipTransitionCheck: true` only for administrative repairs (reconciliation,
+	 * migration) that need to write corrected state outside the normal task lifecycle.
 	 */
-	private async upsertCore(item: HistoryItem): Promise<HistoryItem[]> {
+	private async upsertCore(
+		item: HistoryItem,
+		options: { skipTransitionCheck?: boolean } = {},
+	): Promise<HistoryItem[]> {
 		const existing = this.cache.get(item.id)
 
+		// 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)
+		}
+
 		// Merge: preserve existing metadata unless explicitly overwritten
 		const merged = existing ? { ...existing, ...item } : item
 
@@ -324,10 +338,13 @@ export class TaskHistoryStore {
 	 *
 	 * Called once from `initialize()` after `reconcile()`. Runs inside `withLock` to
 	 * prevent interleaving with watcher-triggered reconcile() calls. Iterates until
-	 * convergence so that chained delegations (A→B→C) are fully resolved in one startup.
+	 * convergence so that one-level chained delegations visible at startup are resolved.
 	 *
-	 * Must NOT be called from within `withLock` — each upsertCore call holds the lock
-	 * for the duration of a single write, and the outer lock wraps the whole pass.
+	 * Must NOT be called from within `withLock` — `withLock` is non-reentrant (promise
+	 * chain); calling `upsert` (which acquires the lock) from inside would deadlock.
+	 * `upsertCore` is called directly here instead, bypassing transition validation via
+	 * `skipTransitionCheck: true` because these writes are administrative repairs, not
+	 * runtime state-machine transitions.
 	 *
 	 * Cases repaired per pass:
 	 * - Parent `delegated` with no `awaitingChildId` → parent → `active` (invalid state)
@@ -343,15 +360,18 @@ export class TaskHistoryStore {
 				repairsInThisPass = 0
 				// Rebuild the lookup map each pass so repairs from the previous pass
 				// are visible when evaluating chained delegations.
-				const byId = new Map(this.getAll().map((i) => [i.id, i]))
+				const byId = new Map(Array.from(this.cache.values()).map((i) => [i.id, i]))
 
 				for (const [, item] of byId) {
 					if (item.status !== "delegated") {
 						continue
 					}
 
 					if (!item.awaitingChildId) {
-						await this.upsertCore({ ...item, status: "active", delegatedToId: undefined })
+						await this.upsertCore(
+							{ ...item, status: "active", awaitingChildId: undefined, delegatedToId: undefined },
+							{ skipTransitionCheck: true },
+						)
 						console.warn(
 							`[TaskHistoryStore] Reconciled invalid delegation: task ${item.id} → active (no awaitingChildId)`,
 						)
@@ -362,26 +382,32 @@ export class TaskHistoryStore {
 					const child = byId.get(item.awaitingChildId)
 
 					if (!child) {
-						await this.upsertCore({
-							...item,
-							status: "active",
-							awaitingChildId: undefined,
-							delegatedToId: undefined,
-						})
+						await this.upsertCore(
+							{
+								...item,
+								status: "active",
+								awaitingChildId: undefined,
+								delegatedToId: undefined,
+							},
+							{ skipTransitionCheck: true },
+						)
 						console.warn(
 							`[TaskHistoryStore] Reconciled orphaned delegation: task ${item.id} → active (child ${item.awaitingChildId} not found)`,
 						)
 						repairsInThisPass++
 					} else if (child.status === "completed") {
-						await this.upsertCore({
-							...item,
-							status: "active",
-							awaitingChildId: undefined,
-							delegatedToId: undefined,
-							completedByChildId: child.id,
-							completionResultSummary:
-								child.completionResultSummary ?? "Task completed (recovered after interruption)",
-						})
+						await this.upsertCore(
+							{
+								...item,
+								status: "active",
+								awaitingChildId: undefined,
+								delegatedToId: undefined,
+								completedByChildId: child.id,
+								completionResultSummary:
+									child.completionResultSummary ?? "Task completed (recovered after interruption)",
+							},
+							{ skipTransitionCheck: true },
+						)
 						console.warn(
 							`[TaskHistoryStore] Reconciled interrupted handoff: task ${item.id} → active (child ${item.awaitingChildId} already completed)`,
 						)

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

@@ -179,15 +179,23 @@ describe("TaskHistoryStore reconcileDelegationState", () => {
 		expect(unchanged?.awaitingChildId).toBe("child-4")
 	})
 
-	it("repairs invalid delegation: delegated parent with no awaitingChildId → active (clears delegatedToId)", async () => {
-		const parent = makeItem({ id: "parent-5", status: "delegated", delegatedToId: "stale-child" })
+	it("repairs invalid delegation: delegated parent with no awaitingChildId → active (clears delegatedToId and awaitingChildId)", async () => {
+		// awaitingChildId is falsy but explicitly set (empty string), delegatedToId is stale
+		const parent = makeItem({
+			id: "parent-5",
+			status: "delegated",
+			delegatedToId: "stale-child",
+			awaitingChildId: "",
+		} as any)
 		await seedItems([parent])
 
 		await store.initialize()
 
 		const repaired = store.get("parent-5")
 		expect(repaired?.status).toBe("active")
 		expect(repaired?.delegatedToId).toBeUndefined()
+		// Fix #4: falsy awaitingChildId must also be cleared
+		expect(repaired?.awaitingChildId).toBeUndefined()
 	})
 
 	it("does not touch active or completed tasks", async () => {
@@ -266,4 +274,119 @@ describe("TaskHistoryStore reconcileDelegationState", () => {
 
 		warnSpy.mockRestore()
 	})
+
+	it("invokes onWrite callback after startup repairs", async () => {
+		const onWrite = vi.fn().mockResolvedValue(undefined)
+		store.dispose()
+		store = new TaskHistoryStore(tmpDir, { onWrite })
+
+		const parent = makeItem({ id: "parent-onwrite", status: "delegated", awaitingChildId: "nonexistent-child" })
+		await seedItems([parent])
+
+		await store.initialize()
+
+		// The startup repair writes the repaired item, which must trigger onWrite
+		expect(onWrite).toHaveBeenCalled()
+		// The final state passed to onWrite must contain the repaired item
+		const lastCall = onWrite.mock.calls[onWrite.mock.calls.length - 1][0] as HistoryItem[]
+		const repaired = lastCall.find((i) => i.id === "parent-onwrite")
+		expect(repaired?.status).toBe("active")
+	})
+})
+
+// ─────────────────────────────────────────────────────────────────────────────
+// upsert — transition guard enforcement at the write boundary
+// ─────────────────────────────────────────────────────────────────────────────
+
+describe("TaskHistoryStore upsert transition guard", () => {
+	let tmpDir: string
+	let store: TaskHistoryStore
+
+	async function seedItems(items: HistoryItem[]): Promise<void> {
+		const tasksDir = path.join(tmpDir, "tasks")
+		await fs.mkdir(tasksDir, { recursive: true })
+		for (const item of items) {
+			const taskDir = path.join(tasksDir, item.id)
+			await fs.mkdir(taskDir, { recursive: true })
+			await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item))
+		}
+	}
+
+	beforeEach(async () => {
+		tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "upsert-guard-test-"))
+		store = new TaskHistoryStore(tmpDir)
+		await store.initialize()
+	})
+
+	afterEach(async () => {
+		store.dispose()
+		await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {})
+	})
+
+	it("rejects completed → active transition, preserving the completed status", async () => {
+		const item = makeItem({ id: "task-guard-1", status: "completed" })
+		await seedItems([item])
+		store.dispose()
+		store = new TaskHistoryStore(tmpDir)
+		await store.initialize()
+
+		// Fire-and-forget late save: tries to write status: "active" over "completed"
+		await expect(store.upsert({ ...item, status: "active" })).rejects.toThrow(
+			"Invalid task status transition: completed → active",
+		)
+
+		// The completed status must be preserved in the cache
+		expect(store.get("task-guard-1")?.status).toBe("completed")
+	})
+
+	it("rejects delegated → completed transition", async () => {
+		// Must include a live active child so reconciliation doesn't repair the parent to active
+		const child = makeItem({ id: "child-guard-2", status: "active" })
+		const item = makeItem({ id: "task-guard-2", status: "delegated", awaitingChildId: "child-guard-2" })
+		await seedItems([child, item])
+		store.dispose()
+		store = new TaskHistoryStore(tmpDir)
+		await store.initialize()
+
+		// Confirm reconciliation left the delegated status alone
+		expect(store.get("task-guard-2")?.status).toBe("delegated")
+
+		await expect(store.upsert({ ...item, status: "completed" })).rejects.toThrow(
+			"Invalid task status transition: delegated → completed",
+		)
+
+		expect(store.get("task-guard-2")?.status).toBe("delegated")
+	})
+
+	it("allows valid active → completed transition", async () => {
+		const item = makeItem({ id: "task-guard-3", status: "active" })
+		await seedItems([item])
+		store.dispose()
+		store = new TaskHistoryStore(tmpDir)
+		await store.initialize()
+
+		await expect(store.upsert({ ...item, status: "completed" })).resolves.toBeDefined()
+		expect(store.get("task-guard-3")?.status).toBe("completed")
+	})
+
+	it("allows first insert with status: active (no prior record to transition from)", async () => {
+		const item = makeItem({ id: "task-guard-new", status: "active" })
+		// Do NOT seed — this is the very first write for this task
+		await expect(store.upsert(item)).resolves.toBeDefined()
+		expect(store.get("task-guard-new")?.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])
+		store.dispose()
+		store = new TaskHistoryStore(tmpDir)
+		await store.initialize()
+
+		// Omitting status entirely — no transition should be validated
+		const { status: _omit, ...noStatus } = item
+		await expect(store.upsert(noStatus as HistoryItem)).resolves.toBeDefined()
+		// Status is preserved from the existing cache entry
+		expect(store.get("task-guard-4")?.status).toBe("completed")
+	})
 })