From 5e5fa9a2be7a11b2d01ad42646f1418d12ee57e1 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 15 Jul 2026 13:17:24 -0700 Subject: [PATCH] fix: auto-approve plans whose approval predates the prompt-hygiene injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An operator was re-asked to approve a plan they had already approved and that had not changed. POST /tasks/:id/approve-plan fingerprints the on-disk PROMPT.md, so a plan approved before the `## Original Description` hygiene injection (applyOriginalDescription) shipped carries a hash over PRE-injection content. On the task's next pass the injection rewrites PROMPT.md, the fingerprint moves, and FN-7569's idempotency short-circuit misses — so the manual gate re-parks an unchanged, already-approved plan. finalizeApprovedTask now also compares the recorded fingerprint against the as-read (pre-injection) content. This does not weaken the gate: `written` diverges from `writtenInput` only via that injection, so both arms hash bytes the operator actually approved — only the representation differs. A genuinely changed plan matches neither arm and still parks. On a legacy match the stored fingerprint is migrated forward, so the reconciliation is one-time per task rather than a comparison carried forever. The migration is a direct updateTask — the taskUpdates batch is flushed well before this gate runs. Covers both finalizeApprovedTask callers (direct + recoverApprovedTask), asserts the changed-plan safety edge still parks, and asserts no redundant fingerprint write when the approval is already post-hygiene. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../legacy-plan-approval-fingerprint.md | 7 + packages/engine/src/__tests__/triage.test.ts | 137 ++++++++++++++++++ packages/engine/src/triage.ts | 37 ++++- 3 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 .changeset/legacy-plan-approval-fingerprint.md diff --git a/.changeset/legacy-plan-approval-fingerprint.md b/.changeset/legacy-plan-approval-fingerprint.md new file mode 100644 index 0000000000..e929c332aa --- /dev/null +++ b/.changeset/legacy-plan-approval-fingerprint.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Stop re-asking approval for plans approved before the Original Description update. +category: fix +dev: `approve-plan` fingerprints the on-disk PROMPT.md, so plans approved before the `## Original Description` hygiene injection (`applyOriginalDescription`) shipped carry a hash over pre-injection content. The injection then rewrote the prompt and moved the hash, defeating FN-7569's idempotency short-circuit and re-parking unchanged plans at `awaiting-approval`. `finalizeApprovedTask` now also compares the recorded fingerprint against the as-read (pre-injection) content — safe because `written` diverges from `writtenInput` only via that injection, so both arms hash bytes the operator actually approved — and migrates the stored fingerprint forward on a legacy match so the reconciliation is one-time per task. A genuinely changed plan matches neither arm and still parks. diff --git a/packages/engine/src/__tests__/triage.test.ts b/packages/engine/src/__tests__/triage.test.ts index 651749f4ba..70ad8f954b 100644 --- a/packages/engine/src/__tests__/triage.test.ts +++ b/packages/engine/src/__tests__/triage.test.ts @@ -3122,6 +3122,143 @@ describe("requirePlanApproval setting", () => { expect(store.moveTask).not.toHaveBeenCalled(); }); + /* + FNXC:PlanApproval 2026-07-15-15:10: + Legacy approvals — recorded before the `## Original Description` hygiene injection existed — + must still auto-approve. approve-plan hashes the on-disk PROMPT.md, so those tasks carry a + fingerprint over PRE-injection content; once the injection ships it rewrites the prompt, the + fingerprint moves, and the operator gets re-asked to approve a plan they already approved and + that has not changed. + + ## Surface Enumeration + - Legacy fingerprint + unchanged plan -> auto-approve (the reported symptom). + - Legacy fingerprint + unchanged plan -> stored fingerprint migrated forward, so the + reconciliation happens once per task rather than on every pass. + - Legacy fingerprint + CHANGED plan -> must still park. This is the safety edge: the + tolerance must not become "any prior approval approves any later plan". + - Current (post-injection) fingerprint -> unchanged behavior, no spurious migration write. + - Both finalizeApprovedTask callers (direct + recoverApprovedTask) share this gate. + */ + it("auto-approves a plan whose fingerprint predates the prompt-hygiene injection", async () => { + // A pre-injection approval: the operator approved the raw plan, before the hygiene + // injection existed, so the recorded hash is over PRE-injection content. + const legacyFingerprint = computePlanApprovalFingerprint(planText); + const task = createTriageTask({ + id: "FN-LEGACY-FP", + status: "planning", + approvedPlanFingerprint: legacyFingerprint, + } as Partial); + const store = createMockStore({ + getTask: vi.fn().mockResolvedValue(task), + } as Partial); + await mkdir(join(rootDir, ".fusion", "tasks", "FN-LEGACY-FP"), { recursive: true }); + await writeFile(join(rootDir, ".fusion", "tasks", "FN-LEGACY-FP", "PROMPT.md"), planText); + const processor = new TriageProcessor(store, rootDir); + + await (processor as unknown as { + finalizeApprovedTask(task: Task, writtenInput: string, settings: Settings): Promise; + }).finalizeApprovedTask( + task, + planText, + { requirePlanApproval: true, planApprovalMode: "require-all" } as Settings, + ); + + expect(store.moveTask).toHaveBeenCalledWith("FN-LEGACY-FP", "todo"); + expect(store.updateTask).not.toHaveBeenCalledWith("FN-LEGACY-FP", expect.objectContaining({ status: "awaiting-approval" })); + // Migrated forward to the post-injection hash, so this is a one-time reconciliation. + const migratedFingerprint = computePlanApprovalFingerprint(approvedOnDisk(planText, "Triage task")); + expect(store.updateTask).toHaveBeenCalledWith("FN-LEGACY-FP", { approvedPlanFingerprint: migratedFingerprint }); + expect(migratedFingerprint).not.toBe(legacyFingerprint); + }); + + it("still re-asks approval for a CHANGED plan when the prior approval predates prompt hygiene", async () => { + // The safety edge: legacy tolerance must not approve a plan the operator never saw. + const legacyFingerprint = computePlanApprovalFingerprint(planText); + const task = createTriageTask({ + id: "FN-LEGACY-FP-CHANGED", + status: "planning", + approvedPlanFingerprint: legacyFingerprint, + } as Partial); + const store = createMockStore({ + getTask: vi.fn().mockResolvedValue(task), + } as Partial); + await mkdir(join(rootDir, ".fusion", "tasks", "FN-LEGACY-FP-CHANGED"), { recursive: true }); + await writeFile(join(rootDir, ".fusion", "tasks", "FN-LEGACY-FP-CHANGED", "PROMPT.md"), changedPlanText); + const processor = new TriageProcessor(store, rootDir); + + await (processor as unknown as { + finalizeApprovedTask(task: Task, writtenInput: string, settings: Settings): Promise; + }).finalizeApprovedTask( + task, + changedPlanText, + { requirePlanApproval: true, planApprovalMode: "require-all" } as Settings, + ); + + expect(store.updateTask).toHaveBeenCalledWith("FN-LEGACY-FP-CHANGED", expect.objectContaining({ status: "awaiting-approval" })); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.updateTask).not.toHaveBeenCalledWith("FN-LEGACY-FP-CHANGED", expect.objectContaining({ approvedPlanFingerprint: expect.anything() })); + }); + + it("recoverApprovedTask auto-approves a legacy pre-hygiene fingerprint too", async () => { + const legacyFingerprint = computePlanApprovalFingerprint(planText); + await mkdir(join(rootDir, ".fusion", "tasks", "FN-LEGACY-RECOVER"), { recursive: true }); + await writeFile(join(rootDir, ".fusion", "tasks", "FN-LEGACY-RECOVER", "PROMPT.md"), planText); + const store = createMockStore({ + getSettings: vi.fn().mockResolvedValue({ + maxConcurrent: 2, + maxWorktrees: 4, + pollIntervalMs: 10000, + groupOverlappingFiles: false, + autoMerge: true, + requirePlanApproval: true, + } as Settings), + }); + const processor = new TriageProcessor(store, rootDir); + + const recovered = await processor.recoverApprovedTask({ + id: "FN-LEGACY-RECOVER", + description: "Recovered triage task", + column: "triage", + status: "planning", + approvedPlanFingerprint: legacyFingerprint, + dependencies: [], + steps: [], + currentStep: 0, + log: [{ timestamp: "2026-01-01T00:00:00.000Z", action: "Spec review: APPROVE" }], + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:02:00.000Z", + }); + + expect(recovered).toBe(true); + expect(store.moveTask).toHaveBeenCalledWith("FN-LEGACY-RECOVER", "todo"); + expect(store.updateTask).not.toHaveBeenCalledWith("FN-LEGACY-RECOVER", expect.objectContaining({ status: "awaiting-approval" })); + }); + + it("does not write a fingerprint migration when the approval is already post-hygiene", async () => { + const approvedPlan = approvedOnDisk(planText, "Triage task"); + const task = createTriageTask({ + id: "FN-FP-NO-MIGRATE", + status: "planning", + approvedPlanFingerprint: computePlanApprovalFingerprint(approvedPlan), + } as Partial); + const store = createMockStore({ + getTask: vi.fn().mockResolvedValue(task), + } as Partial); + const processor = new TriageProcessor(store, rootDir); + + await (processor as unknown as { + finalizeApprovedTask(task: Task, writtenInput: string, settings: Settings): Promise; + }).finalizeApprovedTask( + task, + approvedPlan, + { requirePlanApproval: true, planApprovalMode: "require-all" } as Settings, + ); + + expect(store.moveTask).toHaveBeenCalledWith("FN-FP-NO-MIGRATE", "todo"); + // Already current — no redundant fingerprint write on every pass. + expect(store.updateTask).not.toHaveBeenCalledWith("FN-FP-NO-MIGRATE", expect.objectContaining({ approvedPlanFingerprint: expect.anything() })); + }); + it("never-approved task (no fingerprint) still parks at awaiting-approval on first specify", async () => { const task = createTriageTask({ id: "FN-NEVER-APPROVED", diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index d07f64a572..213aa1945c 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -2780,14 +2780,49 @@ export class TriageProcessor { * already made their independent decisions, so it never weakens either of those gates * or auto-approve-all (which never reaches this branch at all). */ + /* + * FNXC:PlanApproval 2026-07-15-15:10: + * Accept a fingerprint recorded BEFORE the `## Original Description` hygiene injection + * existed (applyOriginalDescription, added 2026-07-14), so the short-circuit above is not + * defeated for plans approved by an older build. + * + * Why this is needed: approve-plan fingerprints the on-disk PROMPT.md. For a plan approved + * before the injection shipped, that recorded hash is over PRE-injection content. On the + * task's next pass the injection rewrites PROMPT.md, `currentFingerprint` moves, and the + * operator is asked to re-approve a plan they already approved and that has not changed. + * + * `written` diverges from `writtenInput` ONLY via that injection (the sole rewrite in this + * method), so `writtenInput` IS the as-approved content for such a task, and hashing it + * recovers the legacy fingerprint exactly. This does not weaken the gate: both arms compare + * against bytes the operator actually approved — only the representation differs. A plan + * that genuinely changed matches neither arm and still parks. + * + * Migrate the stored fingerprint forward on a legacy match so this is a one-time + * reconciliation per task rather than a comparison carried forever. + */ const priorFingerprint = latestTransitionTask?.approvedPlanFingerprint ?? task.approvedPlanFingerprint; const currentFingerprint = computePlanApprovalFingerprint(written); - if (priorFingerprint && priorFingerprint === currentFingerprint) { + const preHygieneFingerprint = written === writtenInput + ? currentFingerprint + : computePlanApprovalFingerprint(writtenInput); + const matchesPriorApproval = Boolean(priorFingerprint) + && (priorFingerprint === currentFingerprint || priorFingerprint === preHygieneFingerprint); + if (matchesPriorApproval) { await this.store.logEntry( task.id, "Plan unchanged since prior approval — proceeding without re-approval", ); planLog.log(`${task.id} plan unchanged since prior approval — proceeding without re-approval`); + if (priorFingerprint !== currentFingerprint) { + // Direct write, not `taskUpdates` — that batch was already flushed above (line ~2579), + // long before this gate runs. + await this.store.updateTask(task.id, { approvedPlanFingerprint: currentFingerprint }); + await this.store.logEntry( + task.id, + "Approved plan fingerprint migrated to current prompt hygiene format", + ); + planLog.log(`${task.id} approved plan fingerprint migrated to post-hygiene content`); + } } else { /* * FNXC:PlanApproval 2026-07-04-21:35: