FN-8008: normalize plan approval fingerprints
Keep approval recovery idempotent when deterministic prompt hygiene is injected. - Normalize plan approval fingerprints around Original Description and Frontend UX sections. - Preserve re-approval for operator-authored plan changes and cover recovery behavior. - Document the normalization contract and add a patch changeset. Files changed: .changeset/fn-8008-plan-approval-fingerprint.md | 7 +++ docs/workflow-steps.md | 2 +- packages/core/src/__tests__/plan-approval.test.ts | 53 +++++++++++++++- packages/core/src/plan-approval.ts | 73 ++++++++++++++++++++++- packages/engine/src/__tests__/triage.test.ts | 45 ++++++-------- packages/engine/src/triage.ts | 40 ++----------- 6 files changed, 153 insertions(+), 67 deletions(-) Fusion-Task-Id: FN-8008 Fusion-Task-Lineage: 9c0f415d-662a-455a-a4bd-b873307e53bc Co-authored-by: Fusion (runfusion.ai) <noreply@runfusion.ai>
This commit is contained in:
gsxdsm
7
.changeset/fn-8008-plan-approval-fingerprint.md
Normal file
7
.changeset/fn-8008-plan-approval-fingerprint.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Fix already-approved plans being re-asked for approval after recovery.
|
||||
category: fix
|
||||
dev: Plan-approval fingerprint now ignores auto-injected ## Original Description / Frontend UX hygiene sections so finalizeApprovedTask idempotency survives on-disk PROMPT.md injection (FN-8008). Keeps approve-plan producer and manual-gate consumer hashing identical normalized content.
|
||||
@@ -218,7 +218,7 @@ Workflow Plan Review is separate from manual plan approval. Project `planApprova
|
||||
|
||||
**FN-7559 (superseded by FN-7732) — telling the holds apart:** Plan Review parks a task with its own distinct statuses (`needs-replan` for a revision verdict, `plan-review-unavailable` for a reviewer-outage retry), so it never renders identically to a plan-approval hold. A separate triage release-authorization gate used to also use `status: "awaiting-approval"` with a distinct `awaitingApprovalReason: "release-authorization"` discriminator and its own dashboard label; that gate was removed (it over-fired on AI-authored specs that merely mentioned release tooling — see `b5b0458`, FN-7732). Releases are kept out of Fusion by agent instruction instead (AGENTS.md → "Releasing"), not by an engine/UI gate. The `Task.awaitingApprovalReason` field and its `"release-authorization"` value are kept only so legacy rows deserialize; any task that still carries the legacy value now renders as an ordinary manual plan-approval hold.
|
||||
|
||||
**FN-7569 — manual plan approval is idempotent against unchanged plans:** approving a plan under the manual gate records a fingerprint of the exact approved `PROMPT.md`. If the same task is later re-specified — a `needs-replan` replan, a Plan Review reviewer-outage retry, or a self-healing rebound back to `triage` — and produces byte-identical `PROMPT.md` content, the manual gate detects the match and proceeds straight to `todo` instead of re-parking at `status: "awaiting-approval"`. A genuinely revised plan still produces a different fingerprint and re-asks as before, and using Reject Plan clears the fingerprint so the regenerated plan is always treated as new. This idempotency check runs only inside the manual gate, strictly after Plan Review has already made its independent decision, and never applies under `planApprovalMode: "auto-approve-all"` (which bypasses the manual gate entirely).
|
||||
**FN-7569 / FN-8008 — manual plan approval is idempotent against unchanged plans:** approving a plan under the manual gate records a fingerprint of the approved `PROMPT.md`, normalized to ignore deterministic `## Original Description` and Frontend UX hygiene sections. If the same task is later re-specified — a `needs-replan` replan, a Plan Review reviewer-outage retry, or a self-healing rebound back to `triage` — the manual gate detects the unchanged operator-authored plan and proceeds straight to `todo` instead of re-parking at `status: "awaiting-approval"`, regardless of whether those generated sections were injected before either fingerprint was calculated. A genuinely revised Mission, Steps, or File Scope still produces a different fingerprint and re-asks as before, and using Reject Plan clears the fingerprint so the regenerated plan is always treated as new. This idempotency check runs only inside the manual gate, strictly after Plan Review has already made its independent decision, and never applies under `planApprovalMode: "auto-approve-all"` (which bypasses the manual gate entirely).
|
||||
|
||||
`builtin:legacy-coding` is backed by the original monolithic `BUILTIN_CODING_WORKFLOW_IR`: `planning` → `execute` → optional quality gates → `review` → merge region.
|
||||
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { computePlanApprovalFingerprint, resolvePlanApprovalRequired, type PlanApprovalMode } from "../plan-approval.js";
|
||||
import { applyFrontendUxCriteria } from "../frontend-ux-policy.js";
|
||||
import { applyOriginalDescription } from "../original-description-policy.js";
|
||||
|
||||
const workflowValues = [true, false, undefined] as const;
|
||||
|
||||
@@ -54,7 +56,56 @@ describe("computePlanApprovalFingerprint", () => {
|
||||
expect(computePlanApprovalFingerprint("line one \nline two")).toBe(computePlanApprovalFingerprint("line one\nline two"));
|
||||
});
|
||||
|
||||
it("differs when the plan content actually changes", () => {
|
||||
it("ignores deterministic Original Description and Frontend UX hygiene sections", () => {
|
||||
const plannerText = "# Task: FN-1\n\n## Mission\n\nBuild the interface.\n\n## File Scope\n\n- packages/dashboard/app/page.tsx\n";
|
||||
const withOriginalDescription = applyOriginalDescription(plannerText, "Operator request");
|
||||
const withAllHygiene = applyFrontendUxCriteria(withOriginalDescription, ["packages/dashboard/app/page.tsx"]);
|
||||
|
||||
expect(computePlanApprovalFingerprint(withOriginalDescription)).toBe(
|
||||
computePlanApprovalFingerprint(plannerText),
|
||||
);
|
||||
expect(computePlanApprovalFingerprint(withAllHygiene)).toBe(
|
||||
computePlanApprovalFingerprint(plannerText),
|
||||
);
|
||||
});
|
||||
|
||||
it("ignores Original Description when its verbatim body contains marker-like text", () => {
|
||||
const plannerText = "# Task: FN-1\n\n## Mission\n\nBuild the interface.\n";
|
||||
const descriptionWithMarker = [
|
||||
"Keep this literal marker in the task request:",
|
||||
"<!-- fusion-original-description:end -->",
|
||||
"It is description content, not the generated section boundary.",
|
||||
].join("\n");
|
||||
|
||||
expect(computePlanApprovalFingerprint(applyOriginalDescription(plannerText, descriptionWithMarker))).toBe(
|
||||
computePlanApprovalFingerprint(plannerText),
|
||||
);
|
||||
});
|
||||
|
||||
it("preserves plan changes after an end-marker literal outside the generated section", () => {
|
||||
const plannerText = [
|
||||
"# Task: FN-1",
|
||||
"",
|
||||
"## Mission",
|
||||
"",
|
||||
"Build the interface.",
|
||||
"",
|
||||
"<!-- fusion-original-description:end -->",
|
||||
"",
|
||||
"## Steps",
|
||||
"",
|
||||
"- [ ] Implement the original plan.",
|
||||
"",
|
||||
].join("\n");
|
||||
const changedPlan = plannerText.replace("Implement the original plan.", "Implement the revised plan.");
|
||||
const description = "Operator request";
|
||||
|
||||
expect(computePlanApprovalFingerprint(applyOriginalDescription(plannerText, description))).not.toBe(
|
||||
computePlanApprovalFingerprint(applyOriginalDescription(changedPlan, description)),
|
||||
);
|
||||
});
|
||||
|
||||
it("differs when the operator-authored plan content actually changes", () => {
|
||||
const original = "# Task: FN-1\n\n## File Scope\n\n- a.ts\n";
|
||||
const changed = "# Task: FN-1\n\n## File Scope\n\n- a.ts\n- b.ts\n";
|
||||
expect(computePlanApprovalFingerprint(original)).not.toBe(computePlanApprovalFingerprint(changed));
|
||||
|
||||
@@ -1,4 +1,10 @@
|
||||
import { createHash } from "node:crypto";
|
||||
import {
|
||||
ORIGINAL_DESCRIPTION_END_MARKER,
|
||||
ORIGINAL_DESCRIPTION_HEADING,
|
||||
ORIGINAL_DESCRIPTION_START_MARKER,
|
||||
} from "./original-description-policy.js";
|
||||
import { FRONTEND_UX_CRITERIA_SECTION } from "./frontend-ux-policy.js";
|
||||
import type { ProjectSettings } from "./types.js";
|
||||
|
||||
export type PlanApprovalMode = NonNullable<ProjectSettings["planApprovalMode"]>;
|
||||
@@ -15,9 +21,17 @@ export type PlanApprovalMode = NonNullable<ProjectSettings["planApprovalMode"]>;
|
||||
* written PROMPT.md is unchanged, while still re-asking when the plan genuinely changed or
|
||||
* was rejected. Normalizes only trailing whitespace/newlines so cosmetic write differences
|
||||
* (trailing newline, trailing spaces) never cause spurious re-approval.
|
||||
*
|
||||
* FNXC:PlanApproval 2026-07-15-20:45:
|
||||
* FN-8008 — `finalizeApprovedTask` deterministically injects Original Description and
|
||||
* Frontend UX hygiene after a planner produces a spec. Approval fingerprints must ignore
|
||||
* precisely those generated sections: approve-plan reads the on-disk prompt while recovery
|
||||
* may compare its pre-injection text. Keeping normalization here makes every producer and
|
||||
* consumer agree without treating an operator-authored Mission, Steps, or File Scope change
|
||||
* as unchanged.
|
||||
*/
|
||||
export function computePlanApprovalFingerprint(promptText: string): string {
|
||||
const normalized = promptText
|
||||
const normalized = normalizePlanApprovalPrompt(promptText)
|
||||
.split("\n")
|
||||
.map((line) => line.replace(/[ \t]+$/, ""))
|
||||
.join("\n")
|
||||
@@ -25,6 +39,63 @@ export function computePlanApprovalFingerprint(promptText: string): string {
|
||||
return createHash("sha256").update(normalized, "utf8").digest("hex");
|
||||
}
|
||||
|
||||
/** Remove only the exact deterministic sections injected during specification hygiene. */
|
||||
function normalizePlanApprovalPrompt(promptText: string): string {
|
||||
return stripInjectedFrontendUxCriteria(stripInjectedOriginalDescription(promptText));
|
||||
}
|
||||
|
||||
function stripInjectedOriginalDescription(promptText: string): string {
|
||||
const start = promptText.indexOf(ORIGINAL_DESCRIPTION_START_MARKER);
|
||||
const end = findGeneratedOriginalDescriptionEnd(promptText, start);
|
||||
if (start === -1 || end === -1) return promptText;
|
||||
|
||||
const heading = promptText.lastIndexOf(ORIGINAL_DESCRIPTION_HEADING, start);
|
||||
if (heading === -1) return promptText;
|
||||
|
||||
const sectionEnd = end + ORIGINAL_DESCRIPTION_END_MARKER.length;
|
||||
const before = promptText.slice(0, heading).trimEnd();
|
||||
const after = promptText.slice(sectionEnd).replace(/^\n+/, "");
|
||||
return after ? `${before}\n\n${after}` : `${before}\n`;
|
||||
}
|
||||
|
||||
/**
|
||||
* FNXC:PlanApproval 2026-07-15-21:30:
|
||||
* FN-8008 — Original Description bodies are verbatim, so marker-like text can occur both
|
||||
* inside the generated body and later in operator-authored prompt content. The generated
|
||||
* closing marker is bounded by the next known PROMPT section (or end of file), preventing a
|
||||
* later literal marker from swallowing a real Mission/Steps/File Scope revision.
|
||||
*/
|
||||
function findGeneratedOriginalDescriptionEnd(promptText: string, start: number): number {
|
||||
if (start === -1) return -1;
|
||||
|
||||
let searchFrom = start + ORIGINAL_DESCRIPTION_START_MARKER.length;
|
||||
while (searchFrom < promptText.length) {
|
||||
const end = promptText.indexOf(ORIGINAL_DESCRIPTION_END_MARKER, searchFrom);
|
||||
if (end === -1) return -1;
|
||||
|
||||
const after = promptText.slice(end + ORIGINAL_DESCRIPTION_END_MARKER.length);
|
||||
if (
|
||||
!after.trim()
|
||||
|| /^\n{1,2}##\s+(?:Before\s*→\s*After Transformation|Review Level(?:\s*:.*)?|Mission|Surface Enumeration|Symptom Verification|Dependencies|Context to Read First|File Scope|Steps|Documentation Requirements|Completion Criteria|Git Commit Convention|Do NOT|Changeset Requirements|Frontend UX Criteria|Acceptance Criteria|Notifications|External Integration Evidence)\s*(?:\n|$)/.test(after)
|
||||
) {
|
||||
return end;
|
||||
}
|
||||
searchFrom = end + ORIGINAL_DESCRIPTION_END_MARKER.length;
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
|
||||
function stripInjectedFrontendUxCriteria(promptText: string): string {
|
||||
const sectionStart = promptText.indexOf(FRONTEND_UX_CRITERIA_SECTION);
|
||||
if (sectionStart === -1) return promptText;
|
||||
|
||||
const before = promptText.slice(0, sectionStart).trimEnd();
|
||||
const after = promptText
|
||||
.slice(sectionStart + FRONTEND_UX_CRITERIA_SECTION.length)
|
||||
.replace(/^\n+/, "");
|
||||
return after ? `${before}\n\n${after}` : `${before}\n`;
|
||||
}
|
||||
|
||||
/**
|
||||
* FNXC:PlanApproval 2026-06-26-00:00:
|
||||
* Per-project planApprovalMode controls the planning approval gate for every task in the project: require-all always parks approved specs for manual approval, auto-approve-all always bypasses the gate, and workflow/undefined preserves the workflow-resolved requirePlanApproval value.
|
||||
|
||||
@@ -3169,25 +3169,14 @@ describe("requirePlanApproval setting", () => {
|
||||
});
|
||||
|
||||
/*
|
||||
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.
|
||||
FNXC:PlanApproval 2026-07-15-20:45:
|
||||
FN-8008 — stored approval fingerprints and finalize recovery must ignore deterministic
|
||||
Original Description / Frontend UX hygiene. Cover the successful on-disk write seam: the
|
||||
fingerprint is recorded for the raw operator-authored plan, then finalize injects a non-empty
|
||||
description before comparing it and must still move directly to todo.
|
||||
*/
|
||||
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.
|
||||
it("auto-approves an unchanged plan after successfully injecting Original Description", async () => {
|
||||
// The recorded fingerprint is over the pre-injection planner text.
|
||||
const legacyFingerprint = computePlanApprovalFingerprint(planText);
|
||||
const task = createTriageTask({
|
||||
id: "FN-LEGACY-FP",
|
||||
@@ -3211,10 +3200,10 @@ describe("requirePlanApproval setting", () => {
|
||||
|
||||
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);
|
||||
expect(store.updateTask).not.toHaveBeenCalledWith(
|
||||
"FN-LEGACY-FP",
|
||||
expect.objectContaining({ approvedPlanFingerprint: expect.anything() }),
|
||||
);
|
||||
});
|
||||
|
||||
it("still re-asks approval for a CHANGED plan when the prior approval predates prompt hygiene", async () => {
|
||||
@@ -3280,7 +3269,7 @@ describe("requirePlanApproval setting", () => {
|
||||
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 () => {
|
||||
it("does not rewrite a matching approved fingerprint", async () => {
|
||||
const approvedPlan = approvedOnDisk(planText, "Triage task");
|
||||
const task = createTriageTask({
|
||||
id: "FN-FP-NO-MIGRATE",
|
||||
@@ -3301,7 +3290,7 @@ describe("requirePlanApproval setting", () => {
|
||||
);
|
||||
|
||||
expect(store.moveTask).toHaveBeenCalledWith("FN-FP-NO-MIGRATE", "todo");
|
||||
// Already current — no redundant fingerprint write on every pass.
|
||||
// A matching normalized fingerprint needs no redundant write.
|
||||
expect(store.updateTask).not.toHaveBeenCalledWith("FN-FP-NO-MIGRATE", expect.objectContaining({ approvedPlanFingerprint: expect.anything() }));
|
||||
});
|
||||
|
||||
@@ -3379,11 +3368,11 @@ describe("requirePlanApproval setting", () => {
|
||||
* proven to reach every finalizeApprovedTask caller, not just a direct-call seam.
|
||||
*/
|
||||
it("recoverApprovedTask (self-healing planning recovery) skips re-park for an unchanged already-approved plan", async () => {
|
||||
// The plan the operator approved is the on-disk file, description already injected.
|
||||
const approvedPlan = approvedOnDisk(planText, "Recovered triage task");
|
||||
const fingerprint = computePlanApprovalFingerprint(approvedPlan);
|
||||
// Recovery reads raw planner text from disk and successfully injects the description.
|
||||
// Its pre-injection approval fingerprint must compare equal after that write.
|
||||
const fingerprint = computePlanApprovalFingerprint(planText);
|
||||
await mkdir(join(rootDir, ".fusion", "tasks", "FN-RECOVER-IDEMPOTENT"), { recursive: true });
|
||||
await writeFile(join(rootDir, ".fusion", "tasks", "FN-RECOVER-IDEMPOTENT", "PROMPT.md"), approvedPlan);
|
||||
await writeFile(join(rootDir, ".fusion", "tasks", "FN-RECOVER-IDEMPOTENT", "PROMPT.md"), planText);
|
||||
const store = createMockStore({
|
||||
getSettings: vi.fn().mockResolvedValue({
|
||||
maxConcurrent: 2,
|
||||
|
||||
@@ -2951,49 +2951,17 @@ 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;
|
||||
// FNXC:PlanApproval 2026-07-15-20:45: The shared hasher strips deterministic
|
||||
// Original Description / Frontend UX hygiene, so approve-plan's on-disk fingerprint and
|
||||
// this post-injection recovery fingerprint represent the same operator-authored plan.
|
||||
const currentFingerprint = computePlanApprovalFingerprint(written);
|
||||
const preHygieneFingerprint = written === writtenInput
|
||||
? currentFingerprint
|
||||
: computePlanApprovalFingerprint(writtenInput);
|
||||
const matchesPriorApproval = Boolean(priorFingerprint)
|
||||
&& (priorFingerprint === currentFingerprint || priorFingerprint === preHygieneFingerprint);
|
||||
if (matchesPriorApproval) {
|
||||
if (priorFingerprint && priorFingerprint === currentFingerprint) {
|
||||
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:
|
||||
|
||||
Reference in New Issue
Block a user