From b07105dbfbddabb2d5fae8f3792e8f9942aaf96a Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 30 Jun 2026 20:50:34 -0700 Subject: [PATCH] fix(workflows): address workflow node PR feedback --- ...actor-workflow-node-runner-classes-plan.md | 2 +- .../executor-fast-mode-workflows.test.ts | 92 +++++++++++++++++++ .../__tests__/workflow-node-runner.test.ts | 20 ++++ .../engine/src/workflow-graph-task-runner.ts | 12 ++- 4 files changed, 122 insertions(+), 4 deletions(-) diff --git a/docs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.md b/docs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.md index e66c52a228..57ad15a010 100644 --- a/docs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.md +++ b/docs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.md @@ -16,7 +16,7 @@ execution: code | Field | Value | |---|---| | Objective | Refactor workflow lifecycle logic out of monolithic triage, executor, and reviewer files into well-defined workflow node runner modules/classes that call runtime primitives through explicit dependencies. | -| Authority | User request in this session: assess and plan whether workflow logic can be moved from monolithic triage/executor/reviewer files into well-defined node classes. Existing product direction says workflow policy belongs in workflow nodes and runtime primitives while engine substrate keeps hard safety invariants. | +| Authority | Architecture decision: decouple workflow node behavior from monolithic triage/executor/reviewer files into workflow-owned node runners and runtime primitive/service boundaries while engine substrate keeps hard safety invariants. | | Execution profile | Deep architecture refactor across `@fusion/engine` workflow dispatch, runtime primitive adapters, custom node execution, reviewer invocation, triage planning review, and focused parity tests. | | Stop conditions | Do not change workflow behavior while extracting boundaries. Do not weaken worktree, file-scope, merge-proof, pause/abort, semaphore, active-session, or self-healing invariants. Do not add arbitrary workflow-definition code execution. | | Tail ownership | Ship in small slices with characterization tests first. Add a changeset only when behavior of published `@runfusion/fusion` changes; pure internal refactor slices do not need one. | diff --git a/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts b/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts index 73c058bef8..50079509e1 100644 --- a/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts +++ b/packages/engine/src/__tests__/executor-fast-mode-workflows.test.ts @@ -202,6 +202,98 @@ describe("fast mode workflow/runtime invariants", () => { expect(seams.merge).toHaveBeenCalledTimes(1); }); + it("raw fast mode skips skill executor nodes when primitives are unavailable", async () => { + const runCustomNode = vi.fn(async () => ({ outcome: "success", value: "ran-skill" })); + const runner = new WorkflowGraphTaskRunner({ + store: { + getTaskWorkflowSelection: () => ({ workflowId: "WF-fast-skill", stepIds: [] }), + getWorkflowDefinition: vi.fn(async () => ({ + id: "WF-fast-skill", + name: "Fast skill", + description: "custom skill workflow", + kind: "workflow", + layout: {}, + createdAt: now, + updatedAt: now, + ir: { + version: "v1", + name: "Fast skill", + nodes: [ + { id: "start", kind: "start" }, + { id: "skill-review", kind: "prompt", config: { executor: "skill", skillName: "compound-engineering:ce-code-review" } }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "skill-review" }, + { from: "skill-review", to: "end", condition: "success" }, + ], + }, + })), + }, + seams: { + planning: vi.fn(async () => ({ outcome: "success", value: "planned" })), + execute: vi.fn(async () => ({ outcome: "success", value: "implemented" })), + review: vi.fn(async () => ({ outcome: "success", value: "approved" })), + merge: vi.fn(async () => ({ outcome: "success", value: "merged" })), + schedule: vi.fn(async () => ({ outcome: "success", value: "scheduled" })), + }, + runCustomNode, + }); + + const result = await runner.run(task({ executionMode: "fast" }), { experimentalFeatures: { workflowGraphExecutor: true } }); + + expect(result.disposition).toBe("completed"); + expect(result.visitedNodeIds).toEqual(["start", "skill-review"]); + expect(runCustomNode).not.toHaveBeenCalled(); + }); + + it("raw fast mode still invokes non-executable review seam nodes", async () => { + const review = vi.fn(async () => ({ outcome: "success" as const, value: "approved" })); + const runCustomNode = vi.fn(async () => ({ outcome: "failure" as const, value: "unexpected-custom-node" })); + const runner = new WorkflowGraphTaskRunner({ + store: { + getTaskWorkflowSelection: () => ({ workflowId: "WF-fast-review-seam", stepIds: [] }), + getWorkflowDefinition: vi.fn(async () => ({ + id: "WF-fast-review-seam", + name: "Fast review seam", + description: "custom review seam workflow", + kind: "workflow", + layout: {}, + createdAt: now, + updatedAt: now, + ir: { + version: "v1", + name: "Fast review seam", + nodes: [ + { id: "start", kind: "start" }, + { id: "review", kind: "prompt", config: { seam: "review" } }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "review" }, + { from: "review", to: "end", condition: "success" }, + ], + }, + })), + }, + seams: { + planning: vi.fn(async () => ({ outcome: "success", value: "planned" })), + execute: vi.fn(async () => ({ outcome: "success", value: "implemented" })), + review, + merge: vi.fn(async () => ({ outcome: "success", value: "merged" })), + schedule: vi.fn(async () => ({ outcome: "success", value: "scheduled" })), + }, + runCustomNode, + }); + + const result = await runner.run(task({ executionMode: "fast" }), { experimentalFeatures: { workflowGraphExecutor: true } }); + + expect(result.disposition).toBe("completed"); + expect(result.visitedNodeIds).toEqual(["start", "review"]); + expect(review).toHaveBeenCalledTimes(1); + expect(runCustomNode).not.toHaveBeenCalled(); + }); + it("fast builtin:coding executes explicitly selected optional-group template nodes", async () => { const calls: string[] = []; const prompt = "# Task\n\n## Steps\n\n### Step 1: Do the work\n- [ ] edit files"; diff --git a/packages/engine/src/__tests__/workflow-node-runner.test.ts b/packages/engine/src/__tests__/workflow-node-runner.test.ts index b0948f8b2f..15b8864ff0 100644 --- a/packages/engine/src/__tests__/workflow-node-runner.test.ts +++ b/packages/engine/src/__tests__/workflow-node-runner.test.ts @@ -7,6 +7,7 @@ import { handlerBackedRunner, type WorkflowNodeRunner, } from "../workflow-node-runner.js"; +import { createMergeAttemptHandler } from "../workflow-node-runners/merge-runner.js"; const task = { id: "FN-7300" } as TaskDetail; @@ -102,4 +103,23 @@ describe("WorkflowNodeRunnerRegistry", () => { expect(result.context["node:script:value"]).toBe("handler-backed"); expect(handler).toHaveBeenCalledWith(scriptNode, expect.objectContaining({ task })); }); + + it("delegates merge-attempt to the legacy merge seam when primitives are unwired", async () => { + const merge = vi.fn(async () => ({ outcome: "success" as const, value: "legacy-merged" })); + const handler = createMergeAttemptHandler({ + seams: { merge }, + buildPrimitiveContext: vi.fn(), + }); + const node: WorkflowIrNode = { id: "merge-attempt", kind: "merge-attempt" }; + const context = { branch: "main" }; + + const result = await handler(node, { + task, + settings: settingsOn(), + context, + }); + + expect(result).toEqual({ outcome: "success", value: "legacy-merged" }); + expect(merge).toHaveBeenCalledWith(task, context); + }); }); diff --git a/packages/engine/src/workflow-graph-task-runner.ts b/packages/engine/src/workflow-graph-task-runner.ts index 07789d8998..1de8858bd9 100644 --- a/packages/engine/src/workflow-graph-task-runner.ts +++ b/packages/engine/src/workflow-graph-task-runner.ts @@ -257,11 +257,14 @@ export class WorkflowGraphTaskRunner { FNXC:WorkflowFastMode 2026-07-01-00:00: Explicitly selected optional-group bodies carry workflow:optionalGroupActive and must still execute in fast mode because selecting the optional workflow step is operator intent, not default built-in review behavior. + + FNXC:WorkflowFastMode 2026-07-01-00:00: + Skill executor nodes use `config.skillName`, not `config.skill`; include that field in executable-node detection so raw fast-mode compatibility skips skill prompts the same way it skips prompt/script nodes. */ const hasExecutableConfig = typeof node.config?.prompt === "string" || typeof node.config?.scriptName === "string" || - typeof node.config?.skill === "string"; + typeof node.config?.skillName === "string"; const isExplicitOptionalGroupNode = typeof c[WORKFLOW_OPTIONAL_GROUP_CONTEXT_KEY] === "string"; if (hasExecutableConfig && !isExplicitOptionalGroupNode) { return Promise.resolve({ outcome: "success", value: "fast-mode-skipped" }); @@ -295,10 +298,13 @@ export class WorkflowGraphTaskRunner { /* FNXC:WorkflowFastMode 2026-07-01-00:00: Raw legacy-seam graph runs do not have TaskExecutor's parse-steps dependencies. For fast-mode compatibility, parse the task prompt from memory and project the parsed steps back onto the runner task so the stepwise built-in can reach the seam-backed lifecycle suffix without a store-backed projection. + + FNXC:WorkflowFastMode 2026-07-01-00:00: + Parse-step projection must write through the task object supplied by the graph node context, not a closed-over outer run argument. Raw fallback callers usually pass the same object today, but the dependency contract belongs to ParseStepsNodeRunner's write target. */ readArtifact: async (_task, key) => (key === "PROMPT.md" ? task.prompt : undefined), - writeSteps: async (_task, steps: TaskStep[]) => { - task.steps = steps; + writeSteps: async (target, steps: TaskStep[]) => { + target.steps = steps; }, } : undefined;