diff --git a/.changeset/retire-optional-steps-declaration.md b/.changeset/retire-optional-steps-declaration.md index 72baaa6311..3ce5035347 100644 --- a/.changeset/retire-optional-steps-declaration.md +++ b/.changeset/retire-optional-steps-declaration.md @@ -1,5 +1,7 @@ --- -"@runfusion/fusion": patch +"@runfusion/fusion": major --- +**Breaking:** the `WorkflowOptionalStep` type, previously exported from `@runfusion/fusion`, is removed — any consumer importing it must migrate to `optional-group` nodes / `ResolvedWorkflowOptionalStep`. + Retire the legacy optional-step DECLARATION model now that optional steps are graph-native `optional-group` nodes. Remove the `WorkflowOptionalStep` type and the `WorkflowIrV2.optionalSteps` IR field, drop the workflow node editor's optional-step declaration authoring panel (sidebar section, mobile tab, and collapse state), and stop threading an `optionalSteps` array through `flowToIr`/`serializeGraph`. A legacy persisted `optionalSteps` key on an old v2 workflow row is now tolerated (ignored, not validated) at parse so old rows still load as v2, and the rollback-downgrade heuristic still treats such a row as v2. The per-task optional-step toggle surfaces are unchanged — they continue to list and toggle optional steps sourced from `optional-group` nodes via `resolveWorkflowOptionalSteps` (`ResolvedWorkflowOptionalStep`). diff --git a/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md b/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md index e5e89f55bd..03d9c3650d 100644 --- a/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md +++ b/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md @@ -539,19 +539,21 @@ surfaces are enumerated: `resolveWorkflowOptionalSteps`'s output shape (source re-pointed in U3). - Step→node projection (`workflow-steps-to-ir.ts`) reused to project add-ons (U5). -### Deferred to Follow-Up Work -- **Full legacy-path retirement (U7) — deferred after execution-time scope discovery.** U1–U6 shipped and - the new model is the live path (built-ins migrated, resolver + executor on optional-group nodes). The - legacy declaration surface is now inert but **not removed**, because U7 turned out far larger than scoped: - (a) `workflow-step` is a shared `WorkflowSeam` union member woven through ~9 engine runtime files +### Delivered cohort (this PR) vs. Deferred +This PR delivers **U1–U6 plus U7a** (10 commits). U7a retired the legacy declaration *model*: the core +`WorkflowOptionalStep` type + `WorkflowIrV2.optionalSteps` field + `validateOptionalSteps`, and the editor's +declaration **authoring** surface (`WorkflowOptionalStepsPanel`, `optionalStepsOf`, the `flowToIr` +`optionalSteps` threading). A code-review pass also fixed a P1 (the optional-group toggle-id collision in +enable resolution) — captured in the commit history and in +`docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md`. The per-task toggle +surfaces (`WorkflowOptionalStepsDropdown`, inline card, modal, Workflow tab) stayed — they consume the +distinct `ResolvedWorkflowOptionalStep`. + +- **Deferred: the `workflow-step` seam infrastructure removal.** What remains of "full U7" is excising the + `workflow-step` seam itself — a shared `WorkflowSeam` union member woven through ~9 engine runtime files (`runtime-primitives`, `step-session-executor`, `workflow-node-handlers`, `active-session-registry`, - `workflow-graph-task-runner`, `executor.runWorkflowSteps`, the compiler seam-anchor), not an - optional-steps-only node — excising it is its own refactor; and (b) the dashboard still carries the prior - declaration **authoring** surface (`WorkflowOptionalStepsPanel`/`WorkflowOptionalStepsDropdown`, the - `flowToIr` `optionalSteps` threading, `optionalStepsOf`) across ~10 files. Removing the core - `WorkflowOptionalStep` type without that dashboard cleanup breaks the build. Retire both surfaces in a - focused follow-up; until then the `WorkflowOptionalStepsPanel` authors declarations the resolver no longer - reads (a known dead-authoring UI to remove with it). + `workflow-graph-task-runner`, `executor.runWorkflowSteps`, the compiler seam-anchor). It is now orphaned + (no built-in graph reaches it) but inert; excising it is its own focused refactor with its own blast radius. - **Nested/conditional groups** (an optional-group inside a split/foreach, or gated by a workflow field rather than the per-task toggle) — single-level, per-task-toggle only for now. - **Plugin-contributed add-ons as optional-group presets** beyond inserting them as flat nodes. diff --git a/packages/core/src/__tests__/workflow-ir-optional-group.test.ts b/packages/core/src/__tests__/workflow-ir-optional-group.test.ts index 36590df04e..88806e3383 100644 --- a/packages/core/src/__tests__/workflow-ir-optional-group.test.ts +++ b/packages/core/src/__tests__/workflow-ir-optional-group.test.ts @@ -94,6 +94,13 @@ describe("optional-group validation", () => { expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/may not contain rework edges/); }); + it("rejects failure-condition edges inside the template (single-pass bails before routing them)", () => { + const template = groupTemplate(); + // A parallel failure edge that the single-pass walk would silently never take. + template.edges.push({ from: "verify", to: "report", condition: "failure" }); + expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/may not contain failure-condition edges/); + }); + it("rejects nested loop/foreach/optional-group regions", () => { const template = groupTemplate(); template.nodes.push({ diff --git a/packages/core/src/workflow-ir.ts b/packages/core/src/workflow-ir.ts index 93b56928a3..75d96e8968 100644 --- a/packages/core/src/workflow-ir.ts +++ b/packages/core/src/workflow-ir.ts @@ -668,6 +668,18 @@ function validateOptionalGroup( if (isReworkEdge(edge)) { throw new WorkflowIrError(`optional-group node '${node.id}' template may not contain rework edges`); } + // FNXC:WorkflowOptionalGroup 2026-06-22-09:00: the single-pass walk + // (runOptionalGroup) surfaces a template-node failure as the GROUP's outcome + // and bails before evaluating that node's edges — so a `failure`-condition + // edge inside the template would silently never execute. Reject it as a typed + // authoring error; failure routing belongs on the group's OUTER edges. + // (Code review: Greptile P2.) + if (edge.condition === "failure") { + throw new WorkflowIrError( + `optional-group node '${node.id}' template may not contain failure-condition edges — ` + + `a template-node failure surfaces as the group's outcome and routes the group's outer failure edge`, + ); + } } const incoming = new Map(); @@ -1466,16 +1478,19 @@ export function downgradeIrToV1IfPure(ir: WorkflowIr): WorkflowIr { // Step-inversion declarations (artifacts/fields), workflow settings (U1), and // any legacy persisted optional-step declarations are v2-only features. - // FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + // FNXC:WorkflowOptionalGroup 2026-06-21-18:00 (updated 2026-06-22-09:00): // `optionalSteps` is no longer a typed IR field (retired declaration model), but // a legacy v2 row may still carry the key. Read it via an untyped cast so such a - // row is still treated as v2 (kept on v2, never silently downgraded). - const legacyOptionalSteps = (ir as { optionalSteps?: unknown[] }).optionalSteps; + // row is still treated as v2 (kept on v2, never silently downgraded). The mere + // PRESENCE of the key — including an empty `[]` — is the v2 signal: an author + // who wrote the key intended v2, and downgrading an `optionalSteps: []` row to + // v1 would still mutate its persisted shape. (Code review: CodeRabbit.) + const legacyOptionalSteps = (ir as { optionalSteps?: unknown }).optionalSteps; if ( (ir.artifacts && ir.artifacts.length > 0) || (ir.fields && ir.fields.length > 0) || (ir.settings && ir.settings.length > 0) || - (Array.isArray(legacyOptionalSteps) && legacyOptionalSteps.length > 0) + legacyOptionalSteps !== undefined ) { return ir; } diff --git a/packages/core/src/workflow-optional-steps.ts b/packages/core/src/workflow-optional-steps.ts index 4cfef149e8..f3fea182cd 100644 --- a/packages/core/src/workflow-optional-steps.ts +++ b/packages/core/src/workflow-optional-steps.ts @@ -35,8 +35,10 @@ function isOptionalGroupNode( * * Source: v2 `ir.nodes` where `kind === "optional-group"` (NOT the legacy * `ir.optionalSteps` declaration). Non-v2 graphs and graphs without any - * optional-group node resolve to `[]`. Malformed group configs are skipped so a - * stale/partial node never renders a blank UI row or breaks workflow loading. + * optional-group node resolve to `[]`. A group with a missing or partial config + * still resolves to a usable entry — `name` falls back to the node id and + * `defaultOn` to false — rather than being dropped, so a stale/partial node never + * silently disappears from the toggle UI or breaks workflow loading. * * `pluginTemplates` is accepted for signature compatibility with the prior * template-backed resolver; group nodes are self-describing, so it is currently diff --git a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts index d214a4da50..ab16bb6cab 100644 --- a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts @@ -1460,13 +1460,21 @@ describe("insertFragment", () => { const allIds = second.nodes.map((n) => n.id); expect(new Set(allIds).size).toBe(allIds.length); - // Round-trip: the group carries defaultOn + a single-node template. + // Round-trip: BOTH inserted groups carry defaultOn + a single-node template, + // so a regression that breaks the second insert can't pass on the first. const { ir: out } = flowToIr("wf", second.nodes, second.edges); - const og = out.nodes.find((n) => n.kind === "optional-group")!; - expect(og.config?.defaultOn).toBe(true); - const template = (og.config as { template?: { nodes: { config?: Record }[] } }).template; - expect(template?.nodes).toHaveLength(1); - expect(template?.nodes[0].config?.name).toBe("Security Audit"); + // An optional-group is a v2-only kind: its presence forces v2 serialization + // even with no columns/fields/settings, or it would serialize as v1 and fail + // parse. (Code review: CodeRabbit.) + expect(out.version).toBe("v2"); + const ogs = out.nodes.filter((n) => n.kind === "optional-group"); + expect(ogs).toHaveLength(2); + for (const og of ogs) { + expect(og.config?.defaultOn).toBe(true); + const template = (og.config as { template?: { nodes: { config?: Record }[] } }).template; + expect(template?.nodes).toHaveLength(1); + expect(template?.nodes[0].config?.name).toBe("Security Audit"); + } }); }); diff --git a/packages/dashboard/app/components/workflow-flow-mapping.ts b/packages/dashboard/app/components/workflow-flow-mapping.ts index 11c718114e..66b5b9e67d 100644 --- a/packages/dashboard/app/components/workflow-flow-mapping.ts +++ b/packages/dashboard/app/components/workflow-flow-mapping.ts @@ -479,8 +479,13 @@ export function flowToIr( // node/edge mapping. Fields and settings remain v2-only declarations: a workflow // with either but no custom columns still serializes as v2 (with the synthesized // default columns). Empty/absent → not a v2 signal (R6 byte-identity for legacy). + // FNXC:WorkflowOptionalGroup 2026-06-22-09:00: a container/group node + // (foreach/loop/optional-group) is a v2-ONLY kind — its presence must force v2, + // or an inserted optional-group on an otherwise-plain workflow would serialize + // as v1 and fail parse (validateOptionalGroup runs only on v2). (Code review: + // CodeRabbit — corroborated by the pre-merge correctness review's residual risk.) const v2 = - (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings; + (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings || groupIds.size > 0; const layout: Record = {}; /** Project one flow node (top-level or template child) into an IR node. */ diff --git a/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts b/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts index 2420815b5c..b3500f4128 100644 --- a/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts +++ b/packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts @@ -88,6 +88,11 @@ describe("builtin coding browser-verification optional-group (U6)", () => { expect(result.context[`node:${GROUP_ID}:outcome`]).toBe("failure"); expect(result.visitedNodeIds).toContain(INNER_STEP_VISITED_ID); + // The group's only two outgoing edges are `success → review` and + // `failure → end`; the inner-step failure routes the failure edge, so review + // is skipped. (`end` is a terminal node the executor does not record in + // visitedNodeIds, so the routing is asserted via the group's failure outcome + // above + review being unreachable here.) expect(result.visitedNodeIds).not.toContain("review"); }); }); diff --git a/packages/engine/src/workflow-graph-executor.ts b/packages/engine/src/workflow-graph-executor.ts index 9dc13c158f..61bf21f3c7 100644 --- a/packages/engine/src/workflow-graph-executor.ts +++ b/packages/engine/src/workflow-graph-executor.ts @@ -506,10 +506,12 @@ export class WorkflowGraphExecutor { // sees "success" rather than undefined — disabled is fully inert, not // just edge-routing-inert. context[`node:${node.id}:outcome`] = "success"; - return await traverseChildren(node, { - outcome: "success", - value: "optional-group-bypassed", - }); + // FNXC:WorkflowOptionalGroup 2026-06-22-09:00: route a disabled group + // as a plain success with NO distinguishing value — a non-empty value + // could let an `outcome:*` edge preempt the success edge in + // traverseChildren, breaking the "disabled == node absent" inertness + // invariant. (Code review: CodeRabbit.) + return await traverseChildren(node, { outcome: "success" }); } const groupResult = await runOptionalGroup(node, { context,