diff --git a/.changeset/fn-7225-plan-review-replan.md b/.changeset/fn-7225-plan-review-replan.md new file mode 100644 index 0000000000..00b5ff57cf --- /dev/null +++ b/.changeset/fn-7225-plan-review-replan.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Route failed Plan Review workflow steps back through triage for automatic replanning. +category: fix +dev: Orders Plan Review before execution steps and sends failed Plan Review results to needs-replan instead of executor fixes. diff --git a/packages/core/src/__tests__/workflow-optional-steps.test.ts b/packages/core/src/__tests__/workflow-optional-steps.test.ts index 8fd535dbad..652e4dbc7d 100644 --- a/packages/core/src/__tests__/workflow-optional-steps.test.ts +++ b/packages/core/src/__tests__/workflow-optional-steps.test.ts @@ -109,7 +109,7 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { ]); }); - it("resolves the built-in coding/stepwise optional-groups in node order", () => { + it("resolves the built-in coding/stepwise optional-groups in execution order", () => { // Legacy coding carries two optional-group toggles on the pre-merge path: // `browser-verification` (default OFF) then `code-review` (default ON — runs by default // but is toggleable off per task). @@ -144,6 +144,28 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR)).toEqual(stepwiseExpected); }); + it("orders Plan Review before execution even when the node is appended after Code Review", () => { + const ir = v2([ + { id: "plan", kind: "prompt", column: "todo" }, + { id: "parse", kind: "parse-steps", column: "todo", config: { artifact: "PROMPT.md", parser: "step-headings" } }, + optionalGroupNode("code-review", { name: "Code Review", defaultOn: true }), + optionalGroupNode("plan-review", { name: "Plan Review", defaultOn: true }), + ]); + ir.edges = [ + { from: "start", to: "plan" }, + { from: "plan", to: "plan-review" }, + { from: "plan-review", to: "parse" }, + { from: "parse", to: "code-review" }, + { from: "code-review", to: "end" }, + ]; + + expect(resolveWorkflowOptionalSteps(ir).map((step) => step.templateId)).toEqual([ + "plan-review", + "code-review", + ]); + expect(resolveDefaultOnOptionalGroupIds(ir)).toEqual(["plan-review", "code-review"]); + }); + it("seeds default-on optional groups but not browser-verification for the built-ins", () => { // resolveDefaultOnOptionalGroupIds drives which groups a new task gets enabled by // default: review groups are on, browser-verification is off. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 947cd72df1..e1334ff519 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -151,6 +151,7 @@ export type { EffectiveAgentResult, } from "./column-agent-resolver.js"; export { BUILTIN_CODING_WORKFLOW_IR } from "./builtin-coding-workflow-ir.js"; +export { PLAN_REVIEW_GROUP_ID } from "./builtin-plan-review-group.js"; export { BUILTIN_MARKETING_WORKFLOW_IR } from "./builtin-marketing-workflow-ir.js"; export { resolveWorkflowOptionalSteps, diff --git a/packages/core/src/workflow-optional-steps.ts b/packages/core/src/workflow-optional-steps.ts index 85639c5fb4..87c47092b7 100644 --- a/packages/core/src/workflow-optional-steps.ts +++ b/packages/core/src/workflow-optional-steps.ts @@ -27,6 +27,39 @@ function isOptionalGroupNode( return node.kind === "optional-group"; } +function computeNodeExecutionRanks(ir: WorkflowIr): Map { + const ranks = new Map(); + if (ir.version !== "v2" || !Array.isArray(ir.nodes) || !Array.isArray(ir.edges)) { + return ranks; + } + + const outgoing = new Map(); + for (const edge of ir.edges) { + if (!outgoing.has(edge.from)) outgoing.set(edge.from, []); + outgoing.get(edge.from)?.push(edge.to); + } + + const queue: Array<{ id: string; rank: number }> = []; + for (const node of ir.nodes) { + if (node.kind === "start" || node.id === "start") { + queue.push({ id: node.id, rank: 0 }); + } + } + + while (queue.length > 0) { + const next = queue.shift(); + if (!next) continue; + const current = ranks.get(next.id); + if (current !== undefined && current <= next.rank) continue; + ranks.set(next.id, next.rank); + for (const target of outgoing.get(next.id) ?? []) { + queue.push({ id: target, rank: next.rank + 1 }); + } + } + + return ranks; +} + /** * Resolve a workflow's `optional-group` nodes into per-task toggle display * metadata. Each enabled group's node id is what a task stores in @@ -50,8 +83,9 @@ export function resolveWorkflowOptionalSteps( ): ResolvedWorkflowOptionalStep[] { if (ir.version !== "v2" || !Array.isArray(ir.nodes)) return []; - const resolved: ResolvedWorkflowOptionalStep[] = []; - for (const node of ir.nodes) { + const ranks = computeNodeExecutionRanks(ir); + const resolved: Array = []; + for (const [nodeIndex, node] of ir.nodes.entries()) { if (!isOptionalGroupNode(node)) continue; const config = (node.config ?? {}) as Partial; resolved.push({ @@ -61,9 +95,17 @@ export function resolveWorkflowOptionalSteps( description: "", phase: "pre-merge", defaultOn: config.defaultOn === true, + /* + FNXC:WorkflowDefinitionSteps 2026-06-29-00:41: + Definition/task creation surfaces must order optional groups by graph execution position, not raw node-array order. Derived built-ins can insert Plan Review between planning and parse while appending its node object, and operators still need the step list to show Plan Review before execution. + */ + rank: ranks.get(node.id) ?? Number.MAX_SAFE_INTEGER, + nodeIndex, }); } - return resolved; + return resolved + .sort((a, b) => a.rank - b.rank || a.nodeIndex - b.nodeIndex) + .map(({ rank: _rank, nodeIndex: _nodeIndex, ...step }) => step); } /** diff --git a/packages/dashboard/app/utils/__tests__/taskProgress.test.ts b/packages/dashboard/app/utils/__tests__/taskProgress.test.ts index 10a5a6b201..8cb69b8ce0 100644 --- a/packages/dashboard/app/utils/__tests__/taskProgress.test.ts +++ b/packages/dashboard/app/utils/__tests__/taskProgress.test.ts @@ -168,6 +168,41 @@ describe("getUnifiedTaskProgress", () => { expect(progress.items.filter((i) => i.source === "workflow")).toHaveLength(2); }); + it("orders Plan Review before implementation steps and Code Review after them", () => { + const progress = getUnifiedTaskProgress( + makeTask({ + steps: [ + { name: "Preflight", status: "pending" }, + { name: "Implement", status: "pending" }, + ], + enabledWorkflowSteps: ["plan-review", "code-review"], + workflowStepResults: [ + { + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + phase: "pre-merge", + status: "failed", + startedAt: "2026-06-29T07:38:49.871Z", + completedAt: "2026-06-29T07:38:57.744Z", + }, + ], + }), + ); + + expect(progress.items.map((item) => item.name)).toEqual([ + "Plan Review", + "Preflight", + "Implement", + "Code Review", + ]); + expect(progress.items.map((item) => item.id)).toEqual([ + "workflow-plan-review", + "step-0", + "step-1", + "workflow-code-review", + ]); + }); + it("maps impl step statuses straight through and skipped as completed", () => { const progress = getUnifiedTaskProgress( makeTask({ diff --git a/packages/dashboard/app/utils/taskProgress.ts b/packages/dashboard/app/utils/taskProgress.ts index 71f249c30c..aae4c89447 100644 --- a/packages/dashboard/app/utils/taskProgress.ts +++ b/packages/dashboard/app/utils/taskProgress.ts @@ -113,7 +113,13 @@ export function getUnifiedTaskProgress( }; }); - const items = [...stepItems, ...workflowItems]; + /* + FNXC:TaskCardWorkflowProgress 2026-06-29-00:41: + Plan Review is a pre-execution optional step in the default stepwise Coding workflow, so task cards must show it before parsed implementation steps. End-of-work optional steps such as Code Review stay after implementation steps so the card order matches workflow execution order. + */ + const preExecutionWorkflowItems = workflowItems.filter((item) => item.id === "workflow-plan-review"); + const remainingWorkflowItems = workflowItems.filter((item) => item.id !== "workflow-plan-review"); + const items = [...preExecutionWorkflowItems, ...stepItems, ...remainingWorkflowItems]; const total = items.length; const completed = items.filter((item) => isCompleted(item.status)).length; diff --git a/packages/dashboard/src/__tests__/workflow-routes.test.ts b/packages/dashboard/src/__tests__/workflow-routes.test.ts index 94578b705d..4731dfaa87 100644 --- a/packages/dashboard/src/__tests__/workflow-routes.test.ts +++ b/packages/dashboard/src/__tests__/workflow-routes.test.ts @@ -251,6 +251,13 @@ describe("workflow routes (U4)", () => { // supplied `icon: "globe"`; the group node instead yields `description: ""` // and `phase: "pre-merge"`. Assert the current group-sourced shape. expect(builtin.body).toEqual([ + expect.objectContaining({ + templateId: "plan-review", + name: "Plan Review", + description: "", + phase: "pre-merge", + defaultOn: true, + }), expect.objectContaining({ templateId: "browser-verification", name: "Browser Verification", @@ -258,6 +265,13 @@ describe("workflow routes (U4)", () => { phase: "pre-merge", defaultOn: false, }), + expect.objectContaining({ + templateId: "code-review", + name: "Code Review", + description: "", + phase: "pre-merge", + defaultOn: true, + }), ]); const custom = await post("/api/workflows", { name: "A", ir: linearIr() }); @@ -313,7 +327,7 @@ describe("workflow routes (U4)", () => { const sel = await put(`/api/tasks/${task.id}/workflow`, { workflowId: wfId }); expect(sel.status).toBe(200); const detail = await store.getTask(task.id); - expect(detail.enabledWorkflowSteps).toHaveLength(1); + expect(detail.enabledWorkflowSteps ?? []).toHaveLength(0); const read = await get(`/api/tasks/${task.id}/workflow`); expect((read.body as { workflowId: string }).workflowId).toBe(wfId); @@ -403,7 +417,7 @@ describe("workflow routes (U4)", () => { const task = await store.createTask({ description: "inherits" }); const detail = await store.getTask(task.id); - expect(detail.enabledWorkflowSteps).toHaveLength(1); + expect(detail.enabledWorkflowSteps ?? []).toHaveLength(0); }); it("selecting an unknown workflow returns 404 without mutating or emitting SSE", async () => { @@ -772,19 +786,19 @@ describe("workflow routes (U4)", () => { expect(res.status).toBe(200); const body = res.body as { stored: Record; defaults: Record; effective: Record }; expect(body.stored).toEqual({}); - expect(body.defaults.execute).toContain("You are a task execution agent"); - expect(body.effective.execute).toBe(body.defaults.execute); + expect(body.defaults.plan).toContain("You are a task specification agent"); + expect(body.effective.plan).toBe(body.defaults.plan); }); it("PATCH sets and resets a built-in prompt override", async () => { const set = await patch("/api/workflows/builtin:coding/prompt-overrides", { - overrides: { execute: "Execute route override" }, + overrides: { plan: "Plan route override" }, }); expect(set.status).toBe(200); - expect((set.body as { stored: Record; effective: Record }).stored.execute).toBe( - "Execute route override", + expect((set.body as { stored: Record; effective: Record }).stored.plan).toBe( + "Plan route override", ); - expect((set.body as { effective: Record }).effective.execute).toBe("Execute route override"); + expect((set.body as { effective: Record }).effective.plan).toBe("Plan route override"); expect(emitWorkflowSseEvent).toHaveBeenCalledWith( "workflow:updated", expect.objectContaining({ id: "builtin:coding" }), @@ -792,23 +806,23 @@ describe("workflow routes (U4)", () => { ); const reset = await patch("/api/workflows/builtin:coding/prompt-overrides", { - overrides: { execute: null }, + overrides: { plan: null }, }); expect(reset.status).toBe(200); const resetBody = reset.body as { stored: Record; defaults: Record; effective: Record }; - expect(resetBody.stored.execute).toBeUndefined(); - expect(resetBody.effective.execute).toBe(resetBody.defaults.execute); + expect(resetBody.stored.plan).toBeUndefined(); + expect(resetBody.effective.plan).toBe(resetBody.defaults.plan); }); it("PATCH treats empty and whitespace prompt overrides as reset", async () => { await patch("/api/workflows/builtin:coding/prompt-overrides", { - overrides: { execute: "Execute route override" }, + overrides: { plan: "Plan route override" }, }); const res = await patch("/api/workflows/builtin:coding/prompt-overrides", { - overrides: { execute: " " }, + overrides: { plan: " " }, }); expect(res.status).toBe(200); - expect((res.body as { stored: Record }).stored.execute).toBeUndefined(); + expect((res.body as { stored: Record }).stored.plan).toBeUndefined(); }); it("PATCH rejects node ids that are not prompt-bearing", async () => { diff --git a/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts b/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts index dd6e822b2c..477c2362b1 100644 --- a/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts @@ -381,6 +381,69 @@ describe("WorkflowGraphExecutor optional-group", () => { expect(requestFix).not.toHaveBeenCalled(); }); + it("routes hard Plan Review failures into the pre-merge replan seam before execution continues", async () => { + const requestFix = vi.fn(async () => true); + const calls: string[] = []; + const records: unknown[] = []; + const ir: WorkflowIr = { + version: "v2", + name: "plan-review-hard-failure", + columns: [{ id: "work", name: "Work", traits: [] }], + nodes: [ + { id: "start", kind: "start" }, + { + id: "plan-review", + kind: "optional-group", + config: { + name: "Plan Review", + defaultOn: true, + template: { + nodes: [{ id: "plan-review-step", kind: "prompt", config: { prompt: "review plan" } }], + edges: [], + }, + }, + }, + { id: "execute", kind: "prompt", config: { prompt: "execute" } }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "plan-review" }, + { from: "plan-review", to: "execute", condition: "success" }, + { from: "plan-review", to: "end", condition: "failure" }, + { from: "execute", to: "end" }, + ], + }; + const executor = new WorkflowGraphExecutor({ + handlers: { + prompt: async (node) => { + calls.push(node.id); + return node.id === "plan-review-step" + ? { outcome: "failure" } + : { outcome: "success" }; + }, + }, + recordWorkflowStepResult: async (_taskId, result) => { records.push(result); }, + requestPreMergeOptionalStepFix: requestFix, + }); + + const result = await executor.run(taskWith(["plan-review"]), settingsOn(), ir); + + expect(requestFix).toHaveBeenCalledWith("FN-OG", { + stepName: "Plan Review", + feedback: "Plan Review failed before execution. Re-run triage to revise PROMPT.md before implementation continues.", + phase: "pre-merge", + status: "failed", + verdict: "REVISE", + nodeId: "plan-review", + maxRevisions: undefined, + }); + expect(calls).not.toContain("execute"); + expect(result.context["node:plan-review:fixScheduled"]).toBe(true); + expect(records).toEqual(expect.arrayContaining([ + expect.objectContaining({ workflowStepId: "plan-review", status: "failed" }), + ])); + }); + it("cycles REVISE findings across graph runs until APPROVE, and falls through only after the budget seam declines", async () => { const verdicts = ["REVISE", "REVISE", "APPROVE"]; const requestFix = vi.fn(async () => true); diff --git a/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts b/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts index d9ee88d6a2..37b350a816 100644 --- a/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts @@ -126,6 +126,42 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => { expect(store.updateTask.mock.invocationCallOrder[0]).toBeLessThan(sendBack.mock.invocationCallOrder[0]); }); + it("routes Plan Review failures to triage replan instead of executor remediation", async () => { + const store = createMockStore(); + const liveTask = task({ postReviewFixCount: 0, column: "in-progress", status: null }); + store.getTask.mockResolvedValue(liveTask); + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 3 }); + const executor = new TaskExecutor(store, "/tmp/test"); + const sendBack = vi.spyOn(executor as any, "sendTaskBackForFix").mockResolvedValue(undefined); + + const scheduled = await (executor as any).requestPreMergeOptionalStepFix(liveTask.id, liveTask, { + stepName: "Plan Review", + feedback: "PROMPT.md is missing the new workflow-order requirement", + phase: "pre-merge" as const, + status: "failed" as const, + verdict: "REVISE", + nodeId: "plan-review", + }); + + expect(scheduled).toBe(true); + expect(sendBack).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-7066", + "AI spec revision requested", + expect.stringContaining("PROMPT.md is missing the new workflow-order requirement"), + undefined, + ); + expect(store.moveTask).toHaveBeenCalledWith("FN-7066", "triage"); + expect(store.updateTask).toHaveBeenCalledWith("FN-7066", { + status: "needs-replan", + error: null, + recoveryRetryCount: null, + nextRecoveryAt: null, + graphResumeRetryCount: 0, + }, undefined); + expect(store.updateTask).not.toHaveBeenCalledWith("FN-7066", { postReviewFixCount: 1 }, undefined); + }); + it("uses the default budget of 3 for repeated fix passes and then declines when exhausted", async () => { const sendBackCalls: number[] = []; diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 305c1b1934..67a34d2141 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -3875,10 +3875,47 @@ export class TaskExecutor { }, ): Promise { if (info.phase !== "pre-merge") return false; - if (info.verdict !== "REVISE") return false; if (info.status !== "advisory_failure" && info.status !== "failed") return false; const liveTask = await this.store.getTask(taskId).catch(() => fallbackTask); + const isPlanReview = info.nodeId === "plan-review" || info.stepName === "Plan Review"; + if (isPlanReview) { + if (info.verdict !== undefined && info.verdict !== "REVISE") return false; + /* + * FNXC:PlanReviewReplan 2026-06-29-00:41: + * Plan Review is pre-execution spec validation, so a failed/revision result + * must repair PROMPT.md through triage instead of reopening implementation + * steps. Triage already advances an approved `needs-replan` task to `todo`, + * which lets the scheduler continue execution after the planner fixes it. + */ + const feedback = info.feedback?.trim() + || "Plan Review failed before execution. Revise the task plan, then continue execution."; + await this.store.logEntry( + taskId, + "AI spec revision requested", + `Plan Review requested a planning revision before execution.\n\nStatus: ${info.status}\nFeedback:\n${feedback}`, + this.getRunContextFor(taskId), + ); + await this.store.logEntry( + taskId, + "Plan Review failed — moved to triage for automatic replan", + feedback, + this.getRunContextFor(taskId), + ); + if (liveTask.column !== "triage") { + await this.store.moveTask(taskId, "triage"); + } + await this.store.updateTask(taskId, { + status: "needs-replan", + error: null, + recoveryRetryCount: null, + nextRecoveryAt: null, + graphResumeRetryCount: 0, + }, this.getRunContextFor(taskId)); + return true; + } + + if (info.verdict !== "REVISE") return false; const settings = await mergeEffectiveSettings(this.store, liveTask, await this.store.getSettings()); const budget = resolveOptionalStepRevisionBudget(info.maxRevisions, settings.maxPostReviewFixes ?? 3); if (!budget.unbounded && (!Number.isFinite(budget.max) || budget.max <= 0)) return false; diff --git a/packages/engine/src/workflow-graph-executor.ts b/packages/engine/src/workflow-graph-executor.ts index 6662c4692e..945adff80e 100644 --- a/packages/engine/src/workflow-graph-executor.ts +++ b/packages/engine/src/workflow-graph-executor.ts @@ -9,7 +9,7 @@ import type { WorkflowNodeExtensionResult, WorkflowStepResult, } from "@fusion/core"; -import { BUILTIN_CODING_WORKFLOW_IR, WorkflowIrError, getWorkflowExtensionRegistry, resolveMaxReworkCycles, isExperimentalFeatureEnabled, GRAPH_NATIVE_POST_MERGE_FLAG } from "@fusion/core"; +import { BUILTIN_CODING_WORKFLOW_IR, PLAN_REVIEW_GROUP_ID, WorkflowIrError, getWorkflowExtensionRegistry, resolveMaxReworkCycles, isExperimentalFeatureEnabled, GRAPH_NATIVE_POST_MERGE_FLAG } from "@fusion/core"; import { createDefaultNodeHandlers, @@ -668,18 +668,30 @@ export class WorkflowGraphExecutor { }; context[`node:${node.id}:outcome`] = result.outcome; if (result.value !== undefined) context[`node:${node.id}:value`] = result.value; - if ( + /* + * FNXC:PlanReviewReplan 2026-06-29-00:41: + * Plan Review sits between specification and execution. A REVISE verdict + * or hard failure at this node means PROMPT.md needs another planning pass, + * not executor remediation. Forward the failure into the same pre-merge fix + * seam with a synthesized REVISE verdict so the executor can route it back + * to triage and then let approved replans continue through todo/execution. + */ + const shouldRequestPreMergeFix = stepPhase === "pre-merge" - && verdict === "REVISE" && (stepStatus === "advisory_failure" || stepStatus === "failed") - ) { - const feedback = stepOutput?.trim() || stepNotes?.trim() || "(no feedback captured)"; + && (verdict === "REVISE" || node.id === PLAN_REVIEW_GROUP_ID); + if (shouldRequestPreMergeFix) { + const feedback = stepOutput?.trim() + || stepNotes?.trim() + || (node.id === PLAN_REVIEW_GROUP_ID + ? "Plan Review failed before execution. Re-run triage to revise PROMPT.md before implementation continues." + : "(no feedback captured)"); const fixScheduled = await this.deps.requestPreMergeOptionalStepFix?.(task.id, { stepName: groupName, feedback, phase: stepPhase, status: stepStatus, - verdict, + verdict: verdict ?? (node.id === PLAN_REVIEW_GROUP_ID ? "REVISE" : undefined), nodeId: node.id, maxRevisions: node.config?.maxRevisions, });