root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(FN-6880): address PR review feedback (#1712)

Browse Source 
- Reject failure-condition edges inside optional-group templates (the single-pass
  walk surfaces template failures as the group's outcome, so an internal failure
  edge was silently dead) — Greptile P2.
- flowToIr: a container/group node (foreach/loop/optional-group) is v2-only — its
  presence now forces v2 serialization (an inserted optional-group on a plain
  workflow no longer serializes as invalid v1) — CodeRabbit.
- Disabled optional-group bypass routes a plain success with no distinguishing
  value, so an outcome:* edge can't preempt success routing (inertness) — CodeRabbit.
- Downgrade heuristic: presence of a legacy optionalSteps key (incl. []) keeps v2.
- Resolver docblock corrected (config-less groups resolve to a fallback entry).
- Strengthen tests: assert both inserted groups + v2 round-trip; failure-edge
  rejection case.
- Changeset: bump to major (removed exported WorkflowOptionalStep type).
- Plan: record U7a as delivered in this cohort; only the workflow-step seam
  infra removal remains deferred.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-06-21 20:47:31 -07:00
parent 68d3c5820e
commit e4a810e9b4
9 changed files with 78 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
.changeset/retire-optional-steps-declaration.md
View File

@@ -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`).

26
docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md
View File

@@ -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.

7
packages/core/src/__tests__/workflow-ir-optional-group.test.ts
View File

@@ -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({

23
packages/core/src/workflow-ir.ts
View File

@@ -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<string, number>();
@@ -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;
}

6
packages/core/src/workflow-optional-steps.ts
View File

@@ -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

20
packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts
View File

@@ -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<string, unknown> }[] } }).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<string, unknown> }[] } }).template;
expect(template?.nodes).toHaveLength(1);
expect(template?.nodes[0].config?.name).toBe("Security Audit");
}
});
});

7
packages/dashboard/app/components/workflow-flow-mapping.ts
View File

@@ -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<string, { x: number; y: number }> = {};
/** Project one flow node (top-level or template child) into an IR node. */

5
packages/engine/src/__tests__/builtin-coding-browser-verification-group.test.ts
View File

@@ -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");
});
});

10
packages/engine/src/workflow-graph-executor.ts
View File

@@ -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,