diff --git a/.changeset/fn-7225-plan-review-approval.md b/.changeset/fn-7225-plan-review-approval.md new file mode 100644 index 0000000000..896ba63526 --- /dev/null +++ b/.changeset/fn-7225-plan-review-approval.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Prevent Plan Review approvals from looping back into triage as failures. +category: fix +dev: Parses explicit reviewer prose verdicts and applies default-on optional workflow steps when no explicit selection exists. diff --git a/packages/core/src/__tests__/workflow-ir-resolver.test.ts b/packages/core/src/__tests__/workflow-ir-resolver.test.ts index bbcc28ded0..4fb4c5bd8a 100644 --- a/packages/core/src/__tests__/workflow-ir-resolver.test.ts +++ b/packages/core/src/__tests__/workflow-ir-resolver.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi } from "vitest"; import { getBuiltinWorkflow } from "../builtin-workflows.js"; -import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR } from "../builtin-stepwise-final-review-coding-workflow-ir.js"; import type { WorkflowIr } from "../workflow-ir-types.js"; import { resolveWorkflowIrForTask, @@ -73,20 +73,20 @@ describe("resolveWorkflowIrForTask", () => { defs: { "wf-gone": undefined }, }); const ir = await resolveWorkflowIrForTask(store, "t1"); - expect(ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); }); it("falls back to the default when there is no selection", async () => { const store = makeStore({ selection: undefined }); const ir = await resolveWorkflowIrForTask(store, "t1"); - expect(ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); expect(store.getWorkflowDefinition).not.toHaveBeenCalled(); }); it("degrades to the default when the selection lookup throws", async () => { const store = makeStore({ selectionThrows: true }); const ir = await resolveWorkflowIrForTask(store, "t1"); - expect(ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); }); it("caches by workflowId so the definition is fetched once across calls", async () => { @@ -104,11 +104,11 @@ describe("resolveWorkflowIrForTask", () => { }); describe("resolveWorkflowIrById", () => { - it("resolves builtin:coding to the canonical authored v2 IR with review column traits", async () => { + it("resolves builtin:coding to the catalog default stepwise final-review IR", async () => { const store = makeStore({}); const ir = await resolveWorkflowIrById(store, "builtin:coding"); - expect(ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); expect(ir).toBe(getBuiltinWorkflow("builtin:coding")!.ir); expect(ir.version).toBe("v2"); if (ir.version !== "v2") throw new Error("expected v2"); @@ -123,14 +123,14 @@ describe("resolveWorkflowIrById", () => { it("resolves an explicit builtin:coding task selection through the canonical IR path", async () => { const store = makeStore({ selection: { workflowId: "builtin:coding", stepIds: [] } }); const ir = await resolveWorkflowIrForTask(store, "t1"); - expect(ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); expect(store.getWorkflowDefinition).not.toHaveBeenCalled(); }); it("falls back to the canonical IR for an unknown built-in id", async () => { const store = makeStore({}); const ir = await resolveWorkflowIrById(store, "builtin:missing"); - expect(ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); expect(store.getWorkflowDefinition).not.toHaveBeenCalled(); }); @@ -142,7 +142,7 @@ describe("resolveWorkflowIrById", () => { const ir = await resolveWorkflowIrById(store, "builtin:coding"); - expect(ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); expect(store.getWorkflowSettingsProjectId).toHaveBeenCalledTimes(1); expect(store.getWorkflowPromptOverrides).not.toHaveBeenCalled(); expect(store.getWorkflowDefinition).not.toHaveBeenCalled(); @@ -162,7 +162,7 @@ describe("resolveWorkflowIrById", () => { it("keeps project-scoped prompt overrides and cache keys when project identity resolves", async () => { const store = makeStore({ projectId: "proj-override", - promptOverrides: { planning: "Project-specific plan" }, + promptOverrides: { plan: "Project-specific plan" }, }); const cache = new Map(); @@ -170,8 +170,8 @@ describe("resolveWorkflowIrById", () => { const second = await resolveWorkflowIrById(store, "builtin:coding", cache); expect(first).toBe(second); - expect(first).not.toBe(BUILTIN_CODING_WORKFLOW_IR); - expect(first.nodes.find((node) => node.id === "planning")?.config?.prompt).toBe("Project-specific plan"); + expect(first).not.toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); + expect(first.nodes.find((node) => node.id === "plan")?.config?.prompt).toBe("Project-specific plan"); expect(cache.get("builtin:coding\u0000proj-override")).toBe(first); expect(store.getWorkflowPromptOverrides).toHaveBeenCalledTimes(1); }); diff --git a/packages/core/src/__tests__/workflow-restart-durability.test.ts b/packages/core/src/__tests__/workflow-restart-durability.test.ts index a60629dd69..1904fc58aa 100644 --- a/packages/core/src/__tests__/workflow-restart-durability.test.ts +++ b/packages/core/src/__tests__/workflow-restart-durability.test.ts @@ -3,7 +3,6 @@ import { join } from "node:path"; import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; import { BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR } from "../builtin-stepwise-final-review-coding-workflow-ir.js"; import type { TaskStore } from "../store.js"; import type { WorkflowRunStepInstance } from "../types.js"; @@ -287,9 +286,9 @@ describe("workflow restart durability for explicit selections", () => { expect(store().getTaskWorkflowSelection(selectedTask.id)).toEqual(selectionBefore); expect(await taskJsonEnabledWorkflowSteps(selectedTask.id)).toEqual(enabledBefore); - // Current hot-path resolution degrades a dangling custom definition to the built-in IR instead of throwing. + // Current hot-path resolution degrades a dangling custom definition to the default built-in Coding IR instead of throwing. // The explicit materialization APIs below must still fail closed when asked to write that missing id again. - expect(privateStore().resolveTaskWorkflowIrSync(selectedTask.id)).toEqual(BUILTIN_CODING_WORKFLOW_IR); + expect(privateStore().resolveTaskWorkflowIrSync(selectedTask.id)).toEqual(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); await expect(store().selectTaskWorkflow(untouchedTask.id, workflow.id)).rejects.toThrow( `Workflow '${workflow.id}' not found`, diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 54efad9b52..fa473b78fa 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -15697,19 +15697,24 @@ ${stepsSection}`; private resolveTaskWorkflowIrSync(taskId: string): WorkflowIr { const selection = this.getTaskWorkflowSelection(taskId); const workflowId = selection?.workflowId; - if (!workflowId) return this.applyBuiltInPromptOverridesSync("builtin:coding", BUILTIN_CODING_WORKFLOW_IR); + /* + * FNXC:WorkflowBuiltins 2026-06-29-02:18: + * The built-in id `builtin:coding` now points at the stepwise final-review workflow. No-selection tasks must resolve through the built-in catalog, otherwise dashboard/operator defaults say "Coding" while the engine silently executes legacy coding. + */ + const defaultCodingIr = getBuiltinWorkflow("builtin:coding")?.ir ?? BUILTIN_CODING_WORKFLOW_IR; + if (!workflowId) return this.applyBuiltInPromptOverridesSync("builtin:coding", defaultCodingIr); if (isBuiltinWorkflowId(workflowId)) { const builtin = getBuiltinWorkflow(workflowId); - return this.applyBuiltInPromptOverridesSync(workflowId, builtin?.ir ?? BUILTIN_CODING_WORKFLOW_IR); + return this.applyBuiltInPromptOverridesSync(workflowId, builtin?.ir ?? defaultCodingIr); } try { const row = this.db .prepare("SELECT ir FROM workflows WHERE id = ?") .get(workflowId) as { ir: string } | undefined; - if (!row) return BUILTIN_CODING_WORKFLOW_IR; + if (!row) return defaultCodingIr; return parseWorkflowIr(row.ir); } catch { - return BUILTIN_CODING_WORKFLOW_IR; + return defaultCodingIr; } } diff --git a/packages/core/src/workflow-ir-resolver.ts b/packages/core/src/workflow-ir-resolver.ts index fab311b988..620a2b20c0 100644 --- a/packages/core/src/workflow-ir-resolver.ts +++ b/packages/core/src/workflow-ir-resolver.ts @@ -20,6 +20,16 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { applyPromptOverridesToIr } from "./workflow-prompt-overrides.js"; import type { WorkflowIr } from "./workflow-ir-types.js"; +function defaultCodingWorkflowIr(): WorkflowIr { + /* + * FNXC:WorkflowBuiltins 2026-06-29-02:18: + * `builtin:coding` is the operator-facing default workflow id, not the legacy monolithic IR export. Resolve the catalog entry first so no-selection tasks follow the new stepwise default; keep the old IR only as a missing-catalog safety fallback. + */ + const builtin = getBuiltinWorkflow("builtin:coding"); + const ir = builtin?.ir ?? BUILTIN_CODING_WORKFLOW_IR; + return typeof ir === "string" ? parseWorkflowIr(ir) : ir; +} + /** Minimal store surface the resolver needs (public APIs only). */ export interface WorkflowIrResolverStore { getTaskWorkflowSelection(taskId: string): { workflowId: string; stepIds: string[] } | undefined; @@ -104,7 +114,7 @@ export async function resolveWorkflowIrById( if (isBuiltinWorkflowId(workflowId)) { const builtin = getBuiltinWorkflow(workflowId); - const ir = builtin?.ir ?? BUILTIN_CODING_WORKFLOW_IR; + const ir = builtin?.ir ?? defaultCodingWorkflowIr(); const resolved = typeof ir === "string" ? parseWorkflowIr(ir) : ir; const overrides = projectId ? store.getWorkflowPromptOverrides?.(workflowId, projectId) : undefined; // FNXC:CustomWorkflows 2026-06-21-19:12: @@ -116,12 +126,12 @@ export async function resolveWorkflowIrById( try { const def = await store.getWorkflowDefinition(workflowId); - if (!def) return BUILTIN_CODING_WORKFLOW_IR; + if (!def) return defaultCodingWorkflowIr(); const ir = typeof def.ir === "string" ? parseWorkflowIr(def.ir) : def.ir; irCache?.set(cacheKey, ir); return ir; } catch { - return BUILTIN_CODING_WORKFLOW_IR; + return defaultCodingWorkflowIr(); } } @@ -138,7 +148,7 @@ export async function resolveWorkflowIrForTask( try { workflowId = store.getTaskWorkflowSelection(taskId)?.workflowId; } catch { - return BUILTIN_CODING_WORKFLOW_IR; + return defaultCodingWorkflowIr(); } if (!workflowId) return resolveWorkflowIrById(store, "builtin:coding", irCache); return resolveWorkflowIrById(store, workflowId, irCache); diff --git a/packages/engine/src/__tests__/triage.test.ts b/packages/engine/src/__tests__/triage.test.ts index f37149ec21..286e9b3027 100644 --- a/packages/engine/src/__tests__/triage.test.ts +++ b/packages/engine/src/__tests__/triage.test.ts @@ -1672,6 +1672,33 @@ describe("requirePlanApproval setting", () => { expect(store.moveTask).not.toHaveBeenCalled(); }); + it("clears stale workflow step instances when a fresh accepted plan replaces existing steps", async () => { + const task = createTriageTask({ + id: "FN-7224", + title: "Rebuilt plan task", + status: "planning", + steps: [{ name: "Old step", status: "pending" }], + } as Partial); + const clearWorkflowRunStepInstances = vi.fn(); + const store = createMockStore({ + getTask: vi.fn().mockResolvedValue(task), + parseStepsFromPrompt: vi.fn().mockResolvedValue([{ name: "Fresh step", status: "pending" }]), + clearWorkflowRunStepInstances, + } as Partial); + const processor = new TriageProcessor(store, rootDir); + + await (processor as unknown as { + finalizeApprovedTask(task: Task, writtenInput: string, settings: Settings): Promise; + }).finalizeApprovedTask( + task, + "# Task: FN-7224 - Rebuilt plan task\n\n## Steps\n\n### Step 1: Fresh step\n- Execute the fresh plan.\n", + { requirePlanApproval: false } as Settings, + ); + + expect(clearWorkflowRunStepInstances).toHaveBeenCalledWith("FN-7224"); + expect(store.moveTask).toHaveBeenCalledWith("FN-7224", "todo"); + }); + it.each([ { mode: "workflow" as const, requirePlanApproval: true, expectedApproval: true }, { mode: undefined, requirePlanApproval: false, expectedApproval: false }, 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 477c2362b1..03d5c039fb 100644 --- a/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts @@ -165,6 +165,28 @@ describe("WorkflowGraphExecutor optional-group", () => { expect(disabledResult.outcome).toBe("success"); }); + it("runs default-on optional groups when the task has no explicit optional-step selection", async () => { + const ir = optionalGroupIr(); + const group = ir.nodes.find((node) => node.id === "group"); + if (group?.config) group.config.defaultOn = true; + const calls: string[] = []; + const executor = new WorkflowGraphExecutor({ + handlers: { + prompt: async (node) => { + calls.push(node.id); + return { outcome: "success" }; + }, + }, + }); + + const unsetResult = await executor.run(taskWith(undefined), settingsOn(), ir); + const explicitEmptyResult = await executor.run(taskWith([]), settingsOn(), ir); + + expect(calls.filter((id) => id === "optstep")).toHaveLength(1); + expect(unsetResult.visitedNodeIds).toContain("group::optstep"); + expect(explicitEmptyResult.visitedNodeIds).not.toContain("group::optstep"); + }); + it("runs an enabled group's template exactly once (single pass, not per-step/looped)", async () => { const runTemplate = vi.fn(async () => ({ outcome: "success" })); const executor = new WorkflowGraphExecutor({ @@ -444,6 +466,64 @@ describe("WorkflowGraphExecutor optional-group", () => { ])); }); + it("does not synthesize a Plan Review replan from advisory malformed output", async () => { + const requestFix = vi.fn(async () => true); + const calls: string[] = []; + const records: unknown[] = []; + const ir: WorkflowIr = { + version: "v2", + name: "plan-review-advisory-malformed", + 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: "execute", to: "end" }, + ], + }; + const executor = new WorkflowGraphExecutor({ + handlers: { + prompt: async (node) => { + calls.push(node.id); + return node.id === "plan-review-step" + ? { outcome: "success", value: "advisory_failure", contextPatch: { output: "malformed output — no verdict extracted" } } + : { outcome: "success" }; + }, + }, + recordWorkflowStepResult: async (_taskId, result) => { records.push(result); }, + requestPreMergeOptionalStepFix: requestFix, + }); + + const result = await executor.run(taskWith(["plan-review"]), settingsOn(), ir); + + expect(requestFix).not.toHaveBeenCalled(); + expect(calls).toContain("execute"); + expect(result.outcome).toBe("success"); + expect(records).toEqual(expect.arrayContaining([ + expect.objectContaining({ + workflowStepId: "plan-review", + status: "advisory_failure", + output: "malformed output — no verdict extracted", + }), + ])); + }); + 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-step-verdict-parsing.test.ts b/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts index cd1f5d0fd5..a0e06f1bca 100644 --- a/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts +++ b/packages/engine/src/__tests__/workflow-step-verdict-parsing.test.ts @@ -28,6 +28,21 @@ describe("inferWorkflowStepVerdictFromProse", () => { expect(inferWorkflowStepVerdictFromProse("looks good")).toEqual({ verdict: "APPROVE", notes: "" }); }); + it("infers explicit markdown verdicts from reviewer-style output", () => { + expect(inferWorkflowStepVerdictFromProse("## Spec Review\n\n### Verdict: APPROVE\n\nThe plan is ready.")).toEqual({ + verdict: "APPROVE", + notes: "", + }); + expect(inferWorkflowStepVerdictFromProse("Status: APPROVE_WITH_NOTES\n\nProceed with notes.")).toEqual({ + verdict: "APPROVE_WITH_NOTES", + notes: "", + }); + expect(inferWorkflowStepVerdictFromProse("Verdict: REVISE\n\nFix the plan.")).toEqual({ + verdict: "REVISE", + notes: "", + }); + }); + it("returns null for unrelated prose", () => { expect(inferWorkflowStepVerdictFromProse("lorem ipsum")).toBeNull(); }); diff --git a/packages/engine/src/__tests__/workflow-task-runtime.test.ts b/packages/engine/src/__tests__/workflow-task-runtime.test.ts index 0f94cf17ba..03dfa53175 100644 --- a/packages/engine/src/__tests__/workflow-task-runtime.test.ts +++ b/packages/engine/src/__tests__/workflow-task-runtime.test.ts @@ -7,6 +7,14 @@ import type { PreparedWorktree, WorkflowRuntimePrimitives } from "../runtime-pri const task = { id: "FN-9002" } as TaskDetail; const flagOff = { experimentalFeatures: {} } as unknown as Pick; +const promptWithOneStep = "# Task: FN-9002 - Runtime default\n\n## Steps\n\n### Step 1: Implement runtime default\n- Exercise the default workflow.\n"; + +const parseStepsDeps = { + readArtifact: async (_task: TaskDetail, key: string) => key === "PROMPT.md" ? promptWithOneStep : undefined, + writeSteps: async (target: TaskDetail, steps: TaskDetail["steps"]) => { + target.steps = steps; + }, +}; function selectedIr(): WorkflowIr { return { @@ -35,6 +43,7 @@ function recordingPrimitives( observed: { prepared?: PreparedWorktree; executedTasks?: TaskDetail[]; + stepTasks?: TaskDetail[]; mergeAttempt?: number; mergeRunId?: string; mergeWorkflowId?: string; @@ -55,7 +64,7 @@ function recordingPrimitives( : overrides.prepareData ?? prepared, }; }, - readArtifact: async () => undefined, + readArtifact: async (_ctx, _task, key) => key === "PROMPT.md" ? promptWithOneStep : undefined, writeArtifact: async (_ctx, _task, key) => ({ outcome: "success", data: { key } }), runPlanningSession: async () => { calls.push("planning"); @@ -73,7 +82,12 @@ function recordingPrimitives( data: { taskDone: override?.outcome !== "failure", modifiedFiles: [] }, }; }, - runTaskStep: async () => ({ outcome: "success" }), + runTaskStep: async (_ctx, _task, stepIndex) => { + calls.push(`step:${stepIndex}`); + observed.stepTasks?.push(_task); + observed.executedTasks?.push(_task); + return { outcome: "success" }; + }, resetTaskStep: async () => ({ ok: true }), runReview: async (_ctx, _task, input) => { calls.push(input.stepIndex === undefined ? "review" : "step-review"); @@ -94,7 +108,10 @@ function recordingPrimitives( data: { allPassed: override?.value !== "remediation-scheduled" }, }; }, - updateSteps: async (_ctx, _task, steps) => ({ outcome: "success", data: { count: steps.length } }), + updateSteps: async (_ctx, _task, steps) => { + _task.steps = steps; + return { outcome: "success", data: { count: steps.length } }; + }, transitionTask: async () => { calls.push("schedule"); return { outcome: "success" }; @@ -118,6 +135,7 @@ describe("WorkflowTaskRuntime", () => { store: { getTaskWorkflowSelection: () => undefined, getWorkflowDefinition: async () => undefined, + getTaskDocument: async (_taskId, key) => key === "PROMPT.md" ? { key } : null, }, runCustomNode: async () => ({ outcome: "success" }), }; @@ -148,6 +166,7 @@ describe("WorkflowTaskRuntime", () => { calls.push(`custom:${node.id}`); return { outcome: "success" }; }, + parseStepsDeps, }); const result = await runtime.run(task, flagOff); @@ -191,6 +210,7 @@ describe("WorkflowTaskRuntime", () => { calls.push(`custom:${node.id}`); return { outcome: "success" }; }, + parseStepsDeps, }); const result = await runtime.run(attachmentTask, flagOff); @@ -218,20 +238,22 @@ describe("WorkflowTaskRuntime", () => { store: { getTaskWorkflowSelection: () => undefined, getWorkflowDefinition: async () => undefined, + getTaskDocument: async (_taskId, key) => key === "PROMPT.md" ? { key } : null, }, primitives: recordingPrimitives(calls, undefined, observed), runCustomNode: async (node) => { calls.push(`custom:${node.id}`); return { outcome: "success" }; }, + parseStepsDeps, }); const result = await runtime.run(attachmentTask, flagOff); expect(result.disposition).toBe("completed"); - // U6: the coding built-in no longer runs a `workflow-step` seam; the pre-merge - // browser-verification optional-group is default-OFF and bypassed (no call). - expect(calls).toEqual(["planning", "prepare-worktree", "execute", "review", "merge"]); + // Default Coding is stepwise: planning writes PROMPT.md, parse projects steps, + // then foreach runs `runTaskStep` before merge. No legacy execute/review seam. + expect(calls).toEqual(["planning", "custom:plan-review-step", "step:0", "custom:code-review-step", "merge"]); expect(observed.executedTasks).toHaveLength(1); expect(observed.executedTasks[0]?.attachments).toEqual(attachments); }); @@ -242,9 +264,11 @@ describe("WorkflowTaskRuntime", () => { store: { getTaskWorkflowSelection: () => undefined, getWorkflowDefinition: async () => undefined, + getTaskDocument: async (_taskId, key) => key === "PROMPT.md" ? { key } : null, }, primitives: recordingPrimitives([], undefined, observed), runCustomNode: async () => ({ outcome: "success" }), + parseStepsDeps, }); const result = await runtime.run(task, flagOff); @@ -284,29 +308,28 @@ describe("WorkflowTaskRuntime", () => { store: { getTaskWorkflowSelection: () => undefined, getWorkflowDefinition: async () => undefined, + getTaskDocument: async (_taskId, key) => key === "PROMPT.md" ? { key } : null, }, primitives: recordingPrimitives(calls), runCustomNode: async (node) => { calls.push(`custom:${node.id}`); return { outcome: "success" }; }, + parseStepsDeps, }); - const result = await runtime.run(task, flagOff); + const defaultTask = { ...task, enabledWorkflowSteps: ["plan-review", "code-review"] } as TaskDetail; + const result = await runtime.run(defaultTask, flagOff); expect(result.disposition).toBe("completed"); - // U6: no `workflow-step` seam; the bypassed browser-verification group node - // sits between execute and review in the visited sequence. - expect(calls).toEqual(["planning", "prepare-worktree", "execute", "review", "merge"]); - expect(result.visitedNodeIds).toEqual([ - "start", - "planning", - "execute", - "browser-verification", - "code-review", - "review", - "merge", - ]); + expect(calls).toEqual(["planning", "custom:plan-review-step", "step:0", "custom:code-review-step", "merge"]); + expect(result.visitedNodeIds).toContain("plan"); + expect(result.visitedNodeIds).toContain("plan-review"); + expect(result.visitedNodeIds).toContain("parse"); + expect(result.visitedNodeIds).toContain("steps"); + expect(result.visitedNodeIds).toContain("code-review"); + expect(result.visitedNodeIds).not.toContain("execute"); + expect(result.visitedNodeIds).not.toContain("review"); }); it("runs the pre-merge browser-verification optional-group once when enabled, before review", async () => { @@ -319,37 +342,33 @@ describe("WorkflowTaskRuntime", () => { store: { getTaskWorkflowSelection: () => undefined, getWorkflowDefinition: async () => undefined, + getTaskDocument: async (_taskId, key) => key === "PROMPT.md" ? { key } : null, }, primitives: recordingPrimitives(calls), runCustomNode: async (node) => { calls.push(`custom:${node.id}`); return { outcome: "success" }; }, + parseStepsDeps, }); - const enabledTask = { ...task, enabledWorkflowSteps: ["browser-verification"] } as TaskDetail; + const enabledTask = { ...task, enabledWorkflowSteps: ["plan-review", "browser-verification", "code-review"] } as TaskDetail; const result = await runtime.run(enabledTask, flagOff); expect(result.disposition).toBe("completed"); expect(calls).toEqual([ "planning", - "prepare-worktree", - "execute", + "custom:plan-review-step", + "step:0", "custom:browser-verification-step", - "review", - "merge", - ]); - expect(result.visitedNodeIds).toEqual([ - "start", - "planning", - "execute", - // The group container node, then its inner template step (run once). - "browser-verification", - "browser-verification::browser-verification-step", - "code-review", - "review", + "custom:code-review-step", "merge", ]); + expect(result.visitedNodeIds).toContain("plan-review::plan-review-step"); + expect(result.visitedNodeIds).toContain("browser-verification::browser-verification-step"); + expect(result.visitedNodeIds).toContain("code-review::code-review-step"); + expect(result.visitedNodeIds).not.toContain("execute"); + expect(result.visitedNodeIds).not.toContain("review"); }); it("fails selected workflow lookup misses instead of running the built-in workflow", async () => { @@ -426,6 +445,7 @@ describe("WorkflowTaskRuntime", () => { }, primitives: recordingPrimitives([]), runCustomNode: async () => ({ outcome: "success" }), + parseStepsDeps, branchPersistence: { loadBranchStates: (_taskId, runId) => { observedRunIds.push(runId); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index dffccc39ec..997d6d0405 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -1128,12 +1128,26 @@ export function parseWorkflowStepVerdict(rawOutput: string): { verdict: "APPROVE return null; } -export function inferWorkflowStepVerdictFromProse(rawOutput: string): { verdict: "APPROVE" | "REVISE"; notes: string } | null { +export function inferWorkflowStepVerdictFromProse(rawOutput: string): { verdict: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; notes: string } | null { const trimmed = rawOutput.trim(); const revisionMatch = trimmed.match(/^REQUEST REVISION\s*\n*/i); if (revisionMatch) { return { verdict: "REVISE", notes: trimmed.slice(revisionMatch[0].length).trim() || "Revision requested" }; } + /* + * FNXC:PlanReview 2026-06-29-02:05: + * Plan Review runs through reviewer-style agents that often emit a markdown + * section such as `### Verdict: APPROVE` even when the prompt asks for trailing + * JSON. Treat that explicit verdict as authoritative so a real approval does + * not collapse into a synthetic pre-execution plan failure loop. + */ + const explicitVerdictMatch = trimmed.match(/(?:^|\n)\s*(?:#{1,6}\s*)?(?:verdict|status)\s*:\s*(APPROVE_WITH_NOTES|APPROVE|REVISE)\b/i); + if (explicitVerdictMatch) { + return { + verdict: explicitVerdictMatch[1].toUpperCase() as "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE", + notes: "", + }; + } if (/\b(approve|approved|looks good|no issues|out of scope)\b/i.test(trimmed)) { return { verdict: "APPROVE", notes: "" }; } @@ -6714,9 +6728,17 @@ export class TaskExecutor { const contextPatch: Record = {}; if (typeof stepOutput === "string") contextPatch.output = stepOutput; if (typeof stepNotes === "string" && stepNotes) contextPatch.notes = stepNotes; + /* + * FNXC:PlanReview 2026-06-29-02:05: + * Advisory graph steps still need a distinct non-pass value when their + * review output is malformed. Returning plain `failed` made optional-group + * recovery synthesize a Plan Review REVISE even when no reviewer requested + * one; `advisory_failure` preserves visibility without inventing feedback. + */ + const advisoryFailureValue = (outcome as { malformed?: boolean }).malformed ? "advisory_failure" : "failed"; return { outcome: outcome.success || !blocking ? "success" : "failure", - value: verdict ?? (outcome.success ? "passed" : "failed"), + value: verdict ?? (outcome.success ? "passed" : advisoryFailureValue), ...(Object.keys(contextPatch).length > 0 ? { contextPatch } : {}), }; } diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index e79d796b9c..f75ffb6622 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -1867,6 +1867,9 @@ export class TriageProcessor { if (parsedSteps.length > 0) { taskUpdates.steps = parsedSteps; } + const shouldClearWorkflowRunStepInstances = + parsedSteps.length > 0 + && (options.isReplan === true || (task.steps?.length ?? 0) > 0); const duplicateLineage = getTaskDuplicateLineage({ id: task.id, @@ -2200,10 +2203,13 @@ export class TriageProcessor { return; } - if (options.isReplan) { + if (shouldClearWorkflowRunStepInstances) { /* FNXC:WorkflowReplan 2026-06-29-00:33: AI spec revision replaces the task's step-source PROMPT.md, so graph foreach instance pins from the previous plan must be discarded before execution reparses steps. Otherwise rebuilt tasks can fail at parse with a stale pin-mismatch even though the new plan is valid. + + FNXC:WorkflowReplan 2026-06-29-02:24: + User-triggered spec rebuilds can race an old paused graph run that writes step-instance rows after the route cleared them. Clear again when triage accepts a fresh parsed plan over an existing step projection, even if the task snapshot no longer has status `needs-replan`. */ const maybeStore = this.store as unknown as { clearWorkflowRunStepInstances?: (taskId: string) => void; diff --git a/packages/engine/src/workflow-graph-executor.ts b/packages/engine/src/workflow-graph-executor.ts index 945adff80e..f7b8ff6ea7 100644 --- a/packages/engine/src/workflow-graph-executor.ts +++ b/packages/engine/src/workflow-graph-executor.ts @@ -557,7 +557,16 @@ export class WorkflowGraphExecutor { * the enabled one runs the body, the disabled one runs none and * still reaches the same downstream node. */ - const enabled = task.enabledWorkflowSteps?.includes(node.id) ?? false; + /* + * FNXC:WorkflowOptionalSteps 2026-06-29-02:05: + * `enabledWorkflowSteps === undefined` means the task has no explicit + * optional-step selection, so default-on workflow nodes must run. An + * explicit empty array still means the operator disabled every optional + * step from Quick Add or task details. + */ + const enabled = Array.isArray(task.enabledWorkflowSteps) + ? task.enabledWorkflowSteps.includes(node.id) + : node.config?.defaultOn === true; if (!enabled) { // FNXC:WorkflowOptionalGroup 2026-06-21-16:30: record the group's own // outcome on bypass too (mirrors the enabled path + every other node @@ -632,6 +641,7 @@ export class WorkflowGraphExecutor { : undefined; let stepStatus: WorkflowStepResult["status"]; if (groupResult.outcome === "failure") stepStatus = "failed"; + else if (groupResult.value === "advisory_failure") stepStatus = "advisory_failure"; else if (verdict === "REVISE") stepStatus = "advisory_failure"; else stepStatus = "passed"; const exitContextPatch = exitResult?.contextPatch; @@ -676,10 +686,17 @@ export class WorkflowGraphExecutor { * seam with a synthesized REVISE verdict so the executor can route it back * to triage and then let approved replans continue through todo/execution. */ + /* + * FNXC:PlanReview 2026-06-29-02:05: + * Plan Review should send a task back to triage only for an actual + * REVISE verdict or a hard step failure. A malformed advisory result is + * visible as `advisory_failure`, but it must not fabricate a plan-rewrite + * request after the reviewer already approved or failed to emit JSON. + */ const shouldRequestPreMergeFix = stepPhase === "pre-merge" && (stepStatus === "advisory_failure" || stepStatus === "failed") - && (verdict === "REVISE" || node.id === PLAN_REVIEW_GROUP_ID); + && (verdict === "REVISE" || (node.id === PLAN_REVIEW_GROUP_ID && stepStatus === "failed")); if (shouldRequestPreMergeFix) { const feedback = stepOutput?.trim() || stepNotes?.trim() diff --git a/packages/engine/src/workflow-task-runtime.ts b/packages/engine/src/workflow-task-runtime.ts index 2ff6c93ea7..6f81994521 100644 --- a/packages/engine/src/workflow-task-runtime.ts +++ b/packages/engine/src/workflow-task-runtime.ts @@ -1,6 +1,5 @@ import type { Settings, TaskDetail, WorkflowIr, WorkflowIrArtifact, WorkflowIrNode, WorkflowWorkItem, WorkflowWorkItemState } from "@fusion/core"; import { - BUILTIN_CODING_WORKFLOW_IR, getBuiltinWorkflow, isBuiltinWorkflowId, parseWorkflowIr, @@ -319,7 +318,14 @@ interface WorkflowRuntimeTarget { } function builtinCodingTarget(): WorkflowRuntimeTarget { - return { workflowId: "builtin:coding", ir: BUILTIN_CODING_WORKFLOW_IR }; + /* + * FNXC:WorkflowBuiltins 2026-06-29-02:18: + * Runtime defaulting must follow the built-in catalog entry for `builtin:coding`; importing the legacy coding IR here would bypass the renamed default workflow and strand unselected tasks on the old monolithic graph. + */ + const builtin = getBuiltinWorkflow("builtin:coding"); + if (!builtin) throw new Error("workflow-missing: builtin:coding"); + const ir = typeof builtin.ir === "string" ? parseWorkflowIr(builtin.ir) : builtin.ir; + return { workflowId: "builtin:coding", ir }; } function buildWorkflowRuntimeSettings(