fix: auto-approve plans whose approval predates the prompt-hygiene injection
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) <noreply@anthropic.com>
This commit is contained in:
7
.changeset/legacy-plan-approval-fingerprint.md
Normal file
7
.changeset/legacy-plan-approval-fingerprint.md
Normal file
@@ -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.
|
||||||
@@ -3122,6 +3122,143 @@ describe("requirePlanApproval setting", () => {
|
|||||||
expect(store.moveTask).not.toHaveBeenCalled();
|
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<Task>);
|
||||||
|
const store = createMockStore({
|
||||||
|
getTask: vi.fn().mockResolvedValue(task),
|
||||||
|
} as Partial<TaskStore>);
|
||||||
|
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<void>;
|
||||||
|
}).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<Task>);
|
||||||
|
const store = createMockStore({
|
||||||
|
getTask: vi.fn().mockResolvedValue(task),
|
||||||
|
} as Partial<TaskStore>);
|
||||||
|
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<void>;
|
||||||
|
}).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<Task>);
|
||||||
|
const store = createMockStore({
|
||||||
|
getTask: vi.fn().mockResolvedValue(task),
|
||||||
|
} as Partial<TaskStore>);
|
||||||
|
const processor = new TriageProcessor(store, rootDir);
|
||||||
|
|
||||||
|
await (processor as unknown as {
|
||||||
|
finalizeApprovedTask(task: Task, writtenInput: string, settings: Settings): Promise<void>;
|
||||||
|
}).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 () => {
|
it("never-approved task (no fingerprint) still parks at awaiting-approval on first specify", async () => {
|
||||||
const task = createTriageTask({
|
const task = createTriageTask({
|
||||||
id: "FN-NEVER-APPROVED",
|
id: "FN-NEVER-APPROVED",
|
||||||
|
|||||||
@@ -2780,14 +2780,49 @@ export class TriageProcessor {
|
|||||||
* already made their independent decisions, so it never weakens either of those gates
|
* already made their independent decisions, so it never weakens either of those gates
|
||||||
* or auto-approve-all (which never reaches this branch at all).
|
* 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 priorFingerprint = latestTransitionTask?.approvedPlanFingerprint ?? task.approvedPlanFingerprint;
|
||||||
const currentFingerprint = computePlanApprovalFingerprint(written);
|
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(
|
await this.store.logEntry(
|
||||||
task.id,
|
task.id,
|
||||||
"Plan unchanged since prior approval — proceeding without re-approval",
|
"Plan unchanged since prior approval — proceeding without re-approval",
|
||||||
);
|
);
|
||||||
planLog.log(`${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 {
|
} else {
|
||||||
/*
|
/*
|
||||||
* FNXC:PlanApproval 2026-07-04-21:35:
|
* FNXC:PlanApproval 2026-07-04-21:35:
|
||||||
|
|||||||
Reference in New Issue
Block a user