From e080bca464347eaa19624df705005fbdf00db2a7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 21:42:24 -0700 Subject: [PATCH] fix: persist awaitingApprovalReason through updateTask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The field was defined in persistence and serialization, the executor's Plan Review replan-cap park wrote it, the triage manual gate null-cleared it, and the dashboard special-cases it (isReviewBudgetExhaustedApproval badge + detail explanation) — but updateTask's field-by-field merge never applied the key, so every writer silently dropped it. FN-8647's 15-cycle non-converging Plan Review loop therefore parked with a generic 'needs approval' and no hint it was a cap escalation. Merge contract, pinned by tests with a measured revert proof (3/4 fail pre-fix): set persists, explicit null clears, a status write that leaves awaiting-approval without addressing the reason auto-clears it so an approved or replanned card cannot carry a stale escalation reason into its next park, and unrelated updates leave it untouched. Co-Authored-By: Claude Fable 5 --- .../awaiting-approval-reason-persisted.md | 7 ++ ...sk-update-awaiting-approval-reason.test.ts | 107 ++++++++++++++++++ packages/core/src/task-store/task-update.ts | 19 ++++ 3 files changed, 133 insertions(+) create mode 100644 .changeset/awaiting-approval-reason-persisted.md create mode 100644 packages/core/src/__tests__/task-update-awaiting-approval-reason.test.ts diff --git a/.changeset/awaiting-approval-reason-persisted.md b/.changeset/awaiting-approval-reason-persisted.md new file mode 100644 index 0000000000..88775571aa --- /dev/null +++ b/.changeset/awaiting-approval-reason-persisted.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Show why a task needs approval — the Plan Review replan-cap reason now survives to the board. +category: fix +dev: `updateTask` never merged `awaitingApprovalReason`, so every writer dropped it and `isReviewBudgetExhaustedApproval` UI was dead. Set persists, null clears, and leaving `awaiting-approval` auto-clears a stale reason. diff --git a/packages/core/src/__tests__/task-update-awaiting-approval-reason.test.ts b/packages/core/src/__tests__/task-update-awaiting-approval-reason.test.ts new file mode 100644 index 0000000000..d1b9305ce2 --- /dev/null +++ b/packages/core/src/__tests__/task-update-awaiting-approval-reason.test.ts @@ -0,0 +1,107 @@ +/* +FNXC:PlanApproval 2026-08-01-04:39: +`awaitingApprovalReason` was defined in persistence (defineTaskColumn) and serialization, and two +writers depend on it — the executor's Plan Review replan-cap park writes `"plan-review-replan-cap"` +so the board can say "non-converging Plan Review loop, human decision needed", and the triage manual +plan gate writes an explicit null so a stale reason never survives into a routine manual hold. But +`updateTaskUnlockedImpl`'s field-by-field merge never applied the key, so EVERY write silently +dropped it: FN-8647 looped plan → REVISE → replan 15 times, hit the hard cap, and parked with a +generic "needs approval" that gave the operator no hint it was a cap escalation. + +These tests pin the merge contract: set persists, null clears, a status write that moves the task +OFF `awaiting-approval` without addressing the reason auto-clears it (an approved or replanned card +must not carry a stale escalation reason into its next park), and unrelated updates leave it alone. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { Task } from "../types.js"; +import type { TaskStore } from "../store.js"; +import { updateTaskUnlockedImpl } from "../task-store/task-update.js"; + +function harness(task: Partial) { + const row = { + id: "FN-1", column: "todo", dependencies: [], steps: [], log: [], status: null, + title: "t", description: "d", createdAt: new Date(0).toISOString(), updatedAt: new Date(0).toISOString(), + ...task, + } as unknown as Task; + const store = { + taskDir: () => "/tmp/does-not-matter", + readTaskJson: async () => row, + writeTaskJson: vi.fn(async () => undefined), + atomicWriteTaskJson: vi.fn(async () => undefined), + syncAgentTaskLinkOnReassignment: vi.fn(async () => undefined), + logEntry: vi.fn(async () => undefined), + getSettings: vi.fn(async () => ({})), + assertNoDependencyCycle: vi.fn(async () => undefined), + getTaskWorkflowSelection: () => undefined, + getTaskWorkflowSelectionAsync: async () => undefined, + getWorkflowDefinition: async () => undefined, + emit: vi.fn(), + isWatching: false, + taskCache: new Map(), + } as Record; + // Same maintenance-proof pattern as task-update-lanes-resolved.test.ts: unlisted store methods + // answer with async no-ops; only return values the assertions depend on are stubbed explicitly. + const proxied = new Proxy(store, { + get(target, prop: string) { + if (prop in target) return target[prop]; + return async () => undefined; + }, + }) as unknown as TaskStore; + return { store: proxied, row }; +} + +const run = (store: TaskStore, updates: Record) => + updateTaskUnlockedImpl(store, "FN-1", updates as never).catch((err: unknown) => { + if (err instanceof Error && /ENOENT|EACCES|no such file/i.test(err.message)) return null; + throw err; + }); + +describe("awaitingApprovalReason survives updateTask", () => { + it("persists the replan-cap escalation reason alongside the awaiting-approval park", async () => { + // Pre-fix this was silently dropped and the board showed a generic "needs approval". + const { store, row } = harness({ status: "needs-replan" }); + + await run(store, { status: "awaiting-approval", awaitingApprovalReason: "plan-review-replan-cap" }); + + expect(row.status).toBe("awaiting-approval"); + expect(row.awaitingApprovalReason).toBe("plan-review-replan-cap"); + }); + + it("clears the reason on an explicit null (the manual plan gate's stale-reason guard)", async () => { + const { store, row } = harness({ + status: "needs-replan", + awaitingApprovalReason: "plan-review-replan-cap", + } as Partial); + + await run(store, { status: "awaiting-approval", awaitingApprovalReason: null }); + + expect(row.status).toBe("awaiting-approval"); + expect(row.awaitingApprovalReason).toBeUndefined(); + }); + + it("auto-clears a stored reason when a status write leaves awaiting-approval", async () => { + // approve-plan / replan writers clear status without knowing about the reason field; the merge + // must not let the escalation reason leak into the card's next lifecycle stage. + const { store, row } = harness({ + status: "awaiting-approval", + awaitingApprovalReason: "plan-review-replan-cap", + } as Partial); + + await run(store, { status: "queued" }); + + expect(row.status).toBe("queued"); + expect(row.awaitingApprovalReason).toBeUndefined(); + }); + + it("leaves a stored reason untouched on unrelated updates", async () => { + const { store, row } = harness({ + status: "awaiting-approval", + awaitingApprovalReason: "plan-review-replan-cap", + } as Partial); + + await run(store, { priority: "high" }); + + expect(row.status).toBe("awaiting-approval"); + expect(row.awaitingApprovalReason).toBe("plan-review-replan-cap"); + }); +}); diff --git a/packages/core/src/task-store/task-update.ts b/packages/core/src/task-store/task-update.ts index 66f2b48a1d..3e08418f7f 100644 --- a/packages/core/src/task-store/task-update.ts +++ b/packages/core/src/task-store/task-update.ts @@ -224,6 +224,25 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat } else if (updates.status !== undefined) { task.status = updates.status; } + /* + FNXC:PlanApproval 2026-08-01-04:39: + `awaitingApprovalReason` was persisted (persistence.ts) and serialized (serialization.ts) + but never applied by this field-by-field merge, so EVERY writer silently lost it — the + executor's Plan Review replan-cap park (`plan-review-replan-cap`) and the triage manual + gate's explicit null-clear both no-oped, and FN-8647's non-converging Plan Review loop + surfaced on the board as a generic "needs approval" with no explanation. Merge it like the + other nullable fields (null clears), and auto-clear the stored reason whenever a status + write moves the task OFF `awaiting-approval` without the caller addressing the reason, so + an approved/replanned card can never carry a stale escalation reason into its next park. + */ + const reasonUpdate = (updates as Record).awaitingApprovalReason; + if (reasonUpdate === null) { + task.awaitingApprovalReason = undefined; + } else if (reasonUpdate !== undefined) { + task.awaitingApprovalReason = reasonUpdate as Task["awaitingApprovalReason"]; + } else if (updates.status !== undefined && updates.status !== "awaiting-approval") { + task.awaitingApprovalReason = undefined; + } if (updates.blockedBy === null) { task.blockedBy = undefined; } else if (updates.blockedBy !== undefined) {