fix(FN-7225): route failed plan review to triage

2026-06-29 00:48:49 -07:00
parent c7cbd1dbf1
commit b86ddf4d10
11 changed files with 301 additions and 26 deletions
--- a/.changeset/fn-7225-plan-review-replan.md
+++ 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.
--- 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.
--- 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,
--- 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<string, number> {
+  const ranks = new Map<string, number>();
+  if (ir.version !== "v2" || !Array.isArray(ir.nodes) || !Array.isArray(ir.edges)) {
+    return ranks;
+  }
+
+  const outgoing = new Map<string, string[]>();
+  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<ResolvedWorkflowOptionalStep & { rank: number; nodeIndex: number }> = [];
+  for (const [nodeIndex, node] of ir.nodes.entries()) {
    if (!isOptionalGroupNode(node)) continue;
    const config = (node.config ?? {}) as Partial<WorkflowOptionalGroupConfig>;
    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);
 }

 /**
--- 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({
--- 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;

--- 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<string, string>; defaults: Record<string, string>; effective: Record<string, string> };
      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<string, string>; effective: Record<string, string> }).stored.execute).toBe(
-        "Execute route override",
+      expect((set.body as { stored: Record<string, string>; effective: Record<string, string> }).stored.plan).toBe(
+        "Plan route override",
      );
-      expect((set.body as { effective: Record<string, string> }).effective.execute).toBe("Execute route override");
+      expect((set.body as { effective: Record<string, string> }).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<string, string>; defaults: Record<string, string>; effective: Record<string, string> };
-      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<string, string> }).stored.execute).toBeUndefined();
+      expect((res.body as { stored: Record<string, string> }).stored.plan).toBeUndefined();
    });

    it("PATCH rejects node ids that are not prompt-bearing", async () => {
--- 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);
--- 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[] = [];

--- a/packages/engine/src/executor.ts
+++ b/packages/engine/src/executor.ts
@@ -3875,10 +3875,47 @@ export class TaskExecutor {
    },
  ): Promise<boolean> {
    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;
--- 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,
            });