From 9c6b4dd3cd5ad7b273b4d734e018f321aaa9fb6d Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 17:25:12 -0700 Subject: [PATCH 01/13] feat(FN-6879): document workflow nodes in editor detail pane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a per-node Help section to the workflow editor's node detail pane: description, configuration, inputs, outputs, and edges for every node kind. Keys off the effective kind (preserved IR kind) so graph-only policy nodes — auto-merge gate, branch-group member integration / promotion, PR and recovery nodes — get specific help instead of reading as a generic merge/gate/hold, and are flagged "Engine-managed". Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/workflow-node-help.md | 5 + .../app/components/WorkflowNodeEditor.css | 74 +++++ .../app/components/WorkflowNodeEditor.tsx | 37 ++- .../__tests__/WorkflowNodeEditor.test.tsx | 19 ++ .../nodes/__tests__/node-help.test.ts | 107 +++++++ .../app/components/nodes/node-help.ts | 294 ++++++++++++++++++ 6 files changed, 535 insertions(+), 1 deletion(-) create mode 100644 .changeset/workflow-node-help.md create mode 100644 packages/dashboard/app/components/nodes/__tests__/node-help.test.ts create mode 100644 packages/dashboard/app/components/nodes/node-help.ts diff --git a/.changeset/workflow-node-help.md b/.changeset/workflow-node-help.md new file mode 100644 index 0000000000..29a99275c9 --- /dev/null +++ b/.changeset/workflow-node-help.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": minor +--- + +Workflow editor: add a Help section to the node detail pane. Every node now documents what it does, how to configure it, and its inputs/outputs/edges — including the engine-managed merge-lifecycle nodes (auto-merge gate, branch-group member integration, branch-group promotion, PR and recovery nodes), which are surfaced read-only with an "Engine-managed" badge. diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.css b/packages/dashboard/app/components/WorkflowNodeEditor.css index e94ada1125..5c9c173cf2 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.css +++ b/packages/dashboard/app/components/WorkflowNodeEditor.css @@ -957,6 +957,80 @@ React Flow ships white default controls and mini-map chrome, but the workflow ed color: var(--ws-warning); } +/* ── Per-node Help (FNXC:WorkflowEditor 2026-06-21-10:00) ─────────── + * Collapsible
teaching what the selected node does, how to + * configure it, and its inputs/outputs/edges. Sits under the heading, + * collapsed by default so it never pushes config fields below the fold. */ +.wf-inspector-help { + border: 1px solid var(--border); + border-radius: var(--radius-sm); + background: var(--bg-secondary); +} + +.wf-inspector-help-summary { + display: flex; + align-items: center; + gap: var(--space-xs); + padding: var(--space-xs) var(--space-sm); + font-size: 0.78rem; + color: var(--text); + cursor: pointer; + list-style: none; + user-select: none; +} + +.wf-inspector-help-summary::-webkit-details-marker { + display: none; +} + +.wf-inspector-help-summary:hover { + background: var(--bg-tertiary); + border-radius: var(--radius-sm); +} + +/* Engine-managed badge for graph-only policy nodes (read-only lifecycle). */ +.wf-inspector-help-badge { + margin-left: auto; + padding: 1px var(--space-xs); + font-size: 0.66rem; + text-transform: uppercase; + letter-spacing: 0.03em; + color: var(--text-dim); + border: 1px solid var(--border); + border-radius: var(--radius-sm); +} + +.wf-inspector-help-body { + display: flex; + flex-direction: column; + gap: var(--space-xs); + padding: 0 var(--space-sm) var(--space-sm); + font-size: 0.76rem; + color: var(--text-muted); +} + +.wf-inspector-help-summary-text { + margin: 0; + color: var(--text); +} + +.wf-inspector-help-dl { + display: grid; + grid-template-columns: max-content 1fr; + gap: 2px var(--space-sm); + margin: 0; +} + +.wf-inspector-help-dl dt { + font-weight: 600; + color: var(--text-dim); +} + +.wf-inspector-help-dl dd { + margin: 0; + color: var(--text-muted); +} + .wf-field--checkbox { flex-direction: row; align-items: center; diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.tsx b/packages/dashboard/app/components/WorkflowNodeEditor.tsx index 69c5017161..dc681b0401 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.tsx +++ b/packages/dashboard/app/components/WorkflowNodeEditor.tsx @@ -53,6 +53,7 @@ import { isMobileViewport, useViewportMode } from "../hooks/useViewportMode"; import { workflowNodeTypes, type WorkflowFlowNodeData, type WorkflowEditorNodeKind } from "./nodes/WorkflowNodeTypes"; import { WorkflowEditorCatalogContext } from "./nodes/WorkflowEditorCatalogContext"; import { bareSkillName, type NodeSummaryCatalogs } from "./nodes/node-summary"; +import { nodeHelpForData } from "./nodes/node-help"; import { irToFlow, flowToIr, @@ -1968,6 +1969,8 @@ function InnerEditor({ * The structural start node needs an inspector because its entry column is editable and persisted in the workflow IR. Keep end structural-only until it has a meaningful editable property. */ const selectedNodeHasInspector = selectedNode !== null && selectedNode.data.kind !== "end"; + // FNXC:WorkflowEditor 2026-06-21-10:00: Help content for the inspector, keyed by the node's effective kind (preserved IR kind when a graph-only policy node collapsed onto a generic merge/gate/hold shape). + const selectedNodeHelp = selectedNode !== null ? nodeHelpForData(selectedNode.data) : null; const selectedEdge = edges.find((e) => e.id === selectedEdgeId) ?? null; const mobileNodeDetailStage = isMobileMode && selectedNodeHasInspector && !inspectorCollapsed; const mobileEdgeDetailStage = isMobileMode && selectedEdge !== null; @@ -3309,7 +3312,8 @@ function InnerEditor({ !(compactLayoutEnabled && !isMobileMode) && ( @@ -2669,7 +2633,6 @@ function InnerEditor({ ["add", t("workflowNodes.mobileAdd", "Add")], ["settings", t("workflowSettings.title", "Settings")], ["fields", t("workflowFields.title", "Fields")], - ["optional-steps", t("workflowOptionalSteps.title", "Optional steps")], ["columns", t("workflowColumns.title", "Columns")], ["actions", t("workflowNodes.mobileActions", "Actions")], ] as Array<[MobileWorkflowPanel, string]>).map(([panel, label]) => ( @@ -2866,16 +2829,6 @@ function InnerEditor({ )} - {mobilePanel === "optional-steps" && ( -
- p.template)} - /> -
- )} {mobilePanel === "columns" && (
diff --git a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.css b/packages/dashboard/app/components/WorkflowOptionalStepsPanel.css deleted file mode 100644 index d458de87a4..0000000000 --- a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.css +++ /dev/null @@ -1,107 +0,0 @@ -/* WorkflowOptionalStepsPanel — sibling of WorkflowFieldsPanel; mirrors its layout - * so the optional-steps panel reads consistently alongside Fields/Settings. */ - -.wf-optional-steps-panel { - display: flex; - flex-direction: column; - gap: var(--space-sm); - padding: var(--space-md); -} - -.wf-optional-steps-header h3 { - margin: 0; -} - -.wf-optional-steps-hint, -.wf-optional-steps-empty { - font-size: 0.75rem; - color: var(--text-muted); - margin: 0; -} - -.wf-optional-steps-list { - list-style: none; - margin: 0; - padding: 0; - display: flex; - flex-direction: column; - gap: var(--space-sm); -} - -.wf-optional-step-item { - display: flex; - flex-direction: column; - gap: 4px; - padding: var(--space-sm); - border: 1px solid var(--border); - border-radius: var(--radius-sm, 6px); -} - -.wf-optional-step-item.is-unknown { - opacity: 0.6; -} - -.wf-optional-step-head { - display: flex; - align-items: center; - justify-content: space-between; - gap: 8px; -} - -.wf-optional-step-title { - display: inline-flex; - align-items: center; - gap: 6px; - min-width: 0; -} - -.wf-optional-step-name { - font-weight: 600; - font-size: 0.8rem; -} - -.wf-optional-step-name--unknown { - font-style: italic; - font-weight: 400; -} - -.wf-optional-step-description { - font-size: 0.72rem; - color: var(--text-muted); - margin: 0; -} - -.wf-optional-step-default { - display: inline-flex; - align-items: center; - gap: 6px; - font-size: 0.75rem; -} - -.wf-optional-step-remove { - display: inline-flex; - align-items: center; - justify-content: center; - background: transparent; - border: none; - color: var(--text-muted); - cursor: pointer; -} - -.wf-optional-step-remove:hover:not(:disabled) { - color: var(--color-error); -} - -.wf-optional-steps-add { - display: flex; - flex-direction: column; - gap: 4px; -} - -.wf-optional-steps-add-label { - display: inline-flex; - align-items: center; - gap: 4px; - font-size: 0.75rem; - color: var(--text-muted); -} diff --git a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.tsx b/packages/dashboard/app/components/WorkflowOptionalStepsPanel.tsx deleted file mode 100644 index 40bdc94a0f..0000000000 --- a/packages/dashboard/app/components/WorkflowOptionalStepsPanel.tsx +++ /dev/null @@ -1,177 +0,0 @@ -/** - * FNXC:WorkflowOptionalSteps 2026-06-21-00:00: - * Workflow authors need to declare which step templates are optional and set each - * one's defaultOn from the visual editor (persisted on the IR's `optionalSteps` - * array) so optional steps are authorable without hand-editing IR. - * - * WorkflowOptionalStepsPanel — the workflow editor's optional-step authoring - * surface. Sibling to {@link WorkflowFieldsPanel} / WorkflowSettingsPanel: lives - * alongside the canvas in {@link WorkflowNodeEditor} and mutates the IR's - * `optionalSteps` array through the same state/save flow (preserved across the - * round-trip by `flowToIr`). - * - * A declaration is just `{ templateId, defaultOn? }`. Display metadata - * (name/description/phase) is resolved from the built-in step-template catalog at - * render time — never duplicated into the IR — so the resolver stays the single - * source of truth. Unknown/stale template ids render a muted, still-removable row - * rather than being silently dropped. - */ -import { useCallback, useMemo } from "react"; -import { useTranslation } from "react-i18next"; -import { Plus, Trash2 } from "lucide-react"; -import { WORKFLOW_STEP_TEMPLATES, type WorkflowOptionalStep, type WorkflowStepTemplate } from "@fusion/core"; -import { phaseBadge } from "./workflow-phase-badge"; -import "./WorkflowOptionalStepsPanel.css"; - -interface WorkflowOptionalStepsPanelProps { - optionalSteps: WorkflowOptionalStep[]; - onChange: (next: WorkflowOptionalStep[]) => void; - readOnly: boolean; - /** Plugin-contributed templates, merged into the catalog when available. */ - pluginTemplates?: WorkflowStepTemplate[]; -} - -export function WorkflowOptionalStepsPanel({ - optionalSteps, - onChange, - readOnly, - pluginTemplates = [], -}: WorkflowOptionalStepsPanelProps) { - const { t } = useTranslation("app"); - - const templatesById = useMemo(() => { - const map = new Map(); - for (const tpl of [...WORKFLOW_STEP_TEMPLATES, ...pluginTemplates]) map.set(tpl.id, tpl); - return map; - }, [pluginTemplates]); - - const declaredIds = useMemo(() => new Set(optionalSteps.map((s) => s.templateId)), [optionalSteps]); - - // Catalog entries not already declared — the "Add optional step" picker source. - const available = useMemo( - () => [...templatesById.values()].filter((tpl) => !declaredIds.has(tpl.id)), - [templatesById, declaredIds], - ); - - const addStep = useCallback( - (templateId: string) => { - if (!templateId || declaredIds.has(templateId)) return; - onChange([...optionalSteps, { templateId, defaultOn: false }]); - }, - [optionalSteps, onChange, declaredIds], - ); - - const removeStep = useCallback( - (templateId: string) => onChange(optionalSteps.filter((s) => s.templateId !== templateId)), - [optionalSteps, onChange], - ); - - const toggleDefaultOn = useCallback( - (templateId: string, defaultOn: boolean) => - onChange(optionalSteps.map((s) => (s.templateId === templateId ? { ...s, defaultOn } : s))), - [optionalSteps, onChange], - ); - - return ( - - ); -} - -export default WorkflowOptionalStepsPanel; diff --git a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx index b93babdb02..f10fd6db68 100644 --- a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx +++ b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx @@ -164,13 +164,9 @@ function v2Def(): WorkflowDefinition { }; } -function v2DefWithOptional(): WorkflowDefinition { - const base = v2Def(); - return { - ...base, - ir: { ...(base.ir as object), optionalSteps: [{ templateId: "browser-verification" }] } as WorkflowDefinition["ir"], - }; -} +// FNXC:WorkflowOptionalGroup 2026-06-21-18:00: `v2DefWithOptional` and its +// optional-step DECLARATION hydration/save test are removed — the declaration +// authoring panel is retired (optional-group nodes now). function builtinDef(): WorkflowDefinition { return { @@ -751,34 +747,6 @@ describe("WorkflowNodeEditor", () => { expect(start?.column).toBe("done"); }); - it("hydrates declared optional steps and preserves them through a dirty save (round-trip)", async () => { - vi.mocked(fetchWorkflows).mockResolvedValue([v2DefWithOptional()]); - vi.mocked(updateWorkflow).mockImplementation(async (_id, updates) => ({ - ...v2DefWithOptional(), - ...(updates as object), - })); - vi.mocked(compileWorkflow).mockResolvedValue({ steps: [] }); - - render( {}} addToast={() => {}} />); - - await screen.findByText("Save"); - // The declared optional step is hydrated into the panel (optionalStepsOf). - const row = await screen.findByTestId("wf-optional-step-browser-verification"); - expect(within(row).getByText("Browser Verification")).toBeTruthy(); - - // Toggling defaultOn must mark the editor dirty (serializeGraph threading) so - // the Save button enables and persists the change. - fireEvent.click(within(row).getByRole("checkbox")); - fireEvent.click(screen.getByText("Save").closest("button")!); - - await waitFor(() => expect(updateWorkflow).toHaveBeenCalled()); - const [, updates] = vi.mocked(updateWorkflow).mock.calls[0]; - const ir = (updates as { ir: WorkflowDefinition["ir"] }).ir as { - optionalSteps?: { templateId: string; defaultOn?: boolean }[]; - }; - expect(ir.optionalSteps).toEqual([{ templateId: "browser-verification", defaultOn: true }]); - }); - it("renders the start inspector without the entry-column select for v1 workflows", async () => { vi.mocked(fetchWorkflows).mockResolvedValue([def()]); diff --git a/packages/dashboard/app/components/__tests__/WorkflowOptionalStepsPanel.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowOptionalStepsPanel.test.tsx deleted file mode 100644 index 93398b01f9..0000000000 --- a/packages/dashboard/app/components/__tests__/WorkflowOptionalStepsPanel.test.tsx +++ /dev/null @@ -1,92 +0,0 @@ -import { describe, it, expect, vi, afterEach } from "vitest"; -import { render, screen, fireEvent, cleanup, within } from "@testing-library/react"; -import { useState } from "react"; -import type { WorkflowOptionalStep } from "@fusion/core"; -import { WorkflowOptionalStepsPanel } from "../WorkflowOptionalStepsPanel"; - -// Controlled host mirroring how WorkflowNodeEditor drives the panel. -function Host({ - initial, - readOnly = false, - onState, -}: { - initial: WorkflowOptionalStep[]; - readOnly?: boolean; - onState?: (s: WorkflowOptionalStep[]) => void; -}) { - const [optionalSteps, setOptionalSteps] = useState(initial); - return ( - { - setOptionalSteps(next); - onState?.(next); - }} - /> - ); -} - -afterEach(() => { - cleanup(); - vi.clearAllMocks(); -}); - -describe("WorkflowOptionalStepsPanel", () => { - it("renders the empty state and an add picker when no steps are declared", () => { - render(); - expect(screen.getByText(/No optional steps/i)).toBeTruthy(); - const select = screen.getByTestId("wf-optional-steps-add-select") as HTMLSelectElement; - // browser-verification is in the catalog and not yet declared → available. - expect(within(select).getByRole("option", { name: "Browser Verification" })).toBeTruthy(); - }); - - it("adds a step from the picker (defaultOn false) and removes it from the picker", () => { - const onState = vi.fn(); - render(); - fireEvent.change(screen.getByTestId("wf-optional-steps-add-select"), { - target: { value: "browser-verification" }, - }); - expect(onState).toHaveBeenCalledWith([{ templateId: "browser-verification", defaultOn: false }]); - // The declared row is shown with the resolved template name… - const row = screen.getByTestId("wf-optional-step-browser-verification"); - expect(within(row).getByText("Browser Verification")).toBeTruthy(); - // …and the picker no longer offers it. - const select = screen.getByTestId("wf-optional-steps-add-select") as HTMLSelectElement; - expect(within(select).queryByRole("option", { name: "Browser Verification" })).toBeNull(); - }); - - it("toggles defaultOn for a declared step", () => { - const onState = vi.fn(); - render(); - const row = screen.getByTestId("wf-optional-step-browser-verification"); - fireEvent.click(within(row).getByRole("checkbox")); - expect(onState).toHaveBeenCalledWith([{ templateId: "browser-verification", defaultOn: true }]); - }); - - it("removes a declared step and returns it to the picker", () => { - render(); - const row = screen.getByTestId("wf-optional-step-browser-verification"); - fireEvent.click(within(row).getByRole("button", { name: /Remove optional step/i })); - expect(screen.queryByTestId("wf-optional-step-browser-verification")).toBeNull(); - const select = screen.getByTestId("wf-optional-steps-add-select") as HTMLSelectElement; - expect(within(select).getByRole("option", { name: "Browser Verification" })).toBeTruthy(); - }); - - it("renders an unknown/stale templateId as a muted, still-removable row", () => { - const onState = vi.fn(); - render(); - const row = screen.getByTestId("wf-optional-step-does-not-exist"); - expect(row.className).toContain("is-unknown"); - expect(within(row).getByText(/Unknown step/i)).toBeTruthy(); - fireEvent.click(within(row).getByRole("button", { name: /Remove optional step/i })); - expect(onState).toHaveBeenCalledWith([]); - }); - - it("disables editing when readOnly", () => { - render(); - const row = screen.getByTestId("wf-optional-step-browser-verification"); - expect((within(row).getByRole("checkbox") as HTMLInputElement).disabled).toBe(true); - expect((within(row).getByRole("button", { name: /Remove optional step/i }) as HTMLButtonElement).disabled).toBe(true); - }); -}); 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 864e9ddf79..d214a4da50 100644 --- a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts @@ -10,7 +10,6 @@ import { fragmentSeamConflicts, copyIrWithFreshIds, columnsOf, - optionalStepsOf, columnForY, bandTop, columnsToBandNodes, @@ -1636,58 +1635,12 @@ describe("copyIrWithFreshIds", () => { }); }); -describe("optionalSteps round-trip (U2)", () => { - const v2WithOptional = (optionalSteps?: { templateId: string; defaultOn?: boolean }[]) => - makeDef( - parseWorkflowIr({ - version: "v2", - name: "wf-opt", - columns: [ - { id: "triage", name: "Triage", traits: [] }, - { id: "done", name: "Done", traits: [{ trait: "complete" }] }, - ], - nodes: [ - { id: "start", kind: "start", column: "triage" }, - { id: "end", kind: "end", column: "done" }, - ], - edges: [{ from: "start", to: "end" }], - ...(optionalSteps ? { optionalSteps } : {}), - }), - ); - - it("optionalStepsOf reads declarations from a v2 IR and returns a copy", () => { - const def = v2WithOptional([{ templateId: "browser-verification", defaultOn: true }]); - const read = optionalStepsOf(def); - expect(read).toEqual([{ templateId: "browser-verification", defaultOn: true }]); - // mutating the result does not mutate the source IR - read[0].defaultOn = false; - expect(optionalStepsOf(def)).toEqual([{ templateId: "browser-verification", defaultOn: true }]); - }); - - it("optionalStepsOf returns [] for v1 and for v2 without optionalSteps", () => { - const v1 = makeDef({ - version: "v1", - name: "legacy", - nodes: [ - { id: "start", kind: "start" }, - { id: "end", kind: "end" }, - ], - edges: [{ from: "start", to: "end" }], - }); - expect(optionalStepsOf(v1)).toEqual([]); - expect(optionalStepsOf(v2WithOptional())).toEqual([]); - }); - - it("flowToIr preserves optionalSteps across a full irToFlow round-trip", () => { - const def = v2WithOptional([{ templateId: "browser-verification", defaultOn: true }]); - const { nodes, edges } = irToFlow(def); - const { ir: out } = flowToIr("wf-opt", nodes, edges, columnsOf(def), [], [], optionalStepsOf(def)); - expect((out as { optionalSteps?: unknown }).optionalSteps).toEqual([ - { templateId: "browser-verification", defaultOn: true }, - ]); - }); - - it("serializes as v2 when optionalSteps present but no custom columns/fields/settings", () => { +// FNXC:WorkflowOptionalGroup 2026-06-21-18:00: +// The legacy optional-step DECLARATION authoring surface is retired: `optionalStepsOf` +// is removed and `flowToIr` no longer accepts/emits an `optionalSteps` array. Optional +// steps are graph-native `optional-group` nodes carried by the normal node/edge mapping. +describe("optionalSteps declaration authoring removed (U7)", () => { + it("flowToIr never emits a legacy optionalSteps key", () => { const { ir: out } = flowToIr( "opt-only", [ @@ -1698,21 +1651,7 @@ describe("optionalSteps round-trip (U2)", () => { [], [], [], - [{ templateId: "browser-verification" }], ); - expect(out.version).toBe("v2"); - expect((out as { optionalSteps?: unknown }).optionalSteps).toEqual([ - { templateId: "browser-verification" }, - ]); - }); - - it("omits the optionalSteps key entirely when empty (R6 byte-identity)", () => { - const def = v2WithOptional(); - const { nodes, edges } = irToFlow(def); - const { ir: out } = flowToIr("wf-opt", nodes, edges, columnsOf(def), [], [], []); expect("optionalSteps" in out).toBe(false); - // and with the arg omitted entirely - const { ir: out2 } = flowToIr("wf-opt", nodes, edges, columnsOf(def)); - expect("optionalSteps" in out2).toBe(false); }); }); diff --git a/packages/dashboard/app/components/workflow-flow-mapping.ts b/packages/dashboard/app/components/workflow-flow-mapping.ts index f889a188f2..11c718114e 100644 --- a/packages/dashboard/app/components/workflow-flow-mapping.ts +++ b/packages/dashboard/app/components/workflow-flow-mapping.ts @@ -9,7 +9,6 @@ import type { WorkflowDefinition, WorkflowFieldDefinition, WorkflowSettingDefinition, - WorkflowOptionalStep, } from "@fusion/core"; import type { WorkflowFlowNodeData, WorkflowEditorNodeKind } from "./nodes/WorkflowNodeTypes"; @@ -453,7 +452,6 @@ export function flowToIr( columns?: WorkflowIrColumn[], fields?: WorkflowFieldDefinition[], settings?: WorkflowSettingDefinition[], - optionalSteps?: WorkflowOptionalStep[], ): { ir: WorkflowIr; layout: Record } { const realNodes = nodes.filter((n) => !isColumnBandNode(n.id)); // Partition by parentId: foreach group children reassemble into that group's @@ -475,15 +473,14 @@ export function flowToIr( ); const hasFields = Array.isArray(fields) && fields.length > 0; const hasSettings = Array.isArray(settings) && settings.length > 0; - const hasOptionalSteps = Array.isArray(optionalSteps) && optionalSteps.length > 0; - // FNXC:WorkflowOptionalSteps 2026-06-21-00:00: - // Optional steps must round-trip through the node editor without data loss, yet - // must never upgrade a legacy v1 graph. Fields, settings, and optional steps are - // v2-only declarations: a workflow with any of them but no custom columns still - // serializes as v2 (with the synthesized default columns). Empty/absent → not a - // v2 signal, and the key is omitted entirely (R6 byte-identity for legacy graphs). + // FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + // The editor no longer AUTHORS legacy `optionalSteps` declarations — optional + // steps are graph-native `optional-group` nodes carried through the normal + // 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). const v2 = - (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings || hasOptionalSteps; + (Array.isArray(columns) && columns.length > 0) || hasFields || hasSettings; const layout: Record = {}; /** Project one flow node (top-level or template child) into an IR node. */ @@ -595,12 +592,6 @@ export function flowToIr( render: s.render ? { ...s.render } : undefined, })); } - if (hasOptionalSteps) { - // Optional-step DECLARATIONS round-trip through the editor opaquely (they are - // not graph nodes; the resolver + server validator are the source of truth). - // Omitted entirely when empty so legacy graphs stay byte-identical (R6). - (ir as { optionalSteps?: unknown }).optionalSteps = optionalSteps!.map((o) => ({ ...o })); - } return { ir, layout }; } @@ -1013,15 +1004,11 @@ export function settingsOf(def: WorkflowDefinition): WorkflowSettingDefinition[] })); } -/** Extract the editor's working optional-step declaration list from a definition. - * v2 with `optionalSteps` → a shallow copy; v1 or none → empty. Display metadata - * (name/icon/phase) is NOT carried here — it is resolved from the step-template - * catalog at render time so the resolver stays the single source of truth. */ -export function optionalStepsOf(def: WorkflowDefinition): WorkflowOptionalStep[] { - const ir = def.ir as { optionalSteps?: WorkflowOptionalStep[] }; - if (!isV2(def.ir) || !Array.isArray(ir.optionalSteps)) return []; - return ir.optionalSteps.map((o) => ({ ...o })); -} +/* FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + `optionalStepsOf` (the editor's legacy `optionalSteps` declaration extractor) + is removed. Optional steps are graph-native `optional-group` nodes now; the + editor reads/writes them through the normal node/edge mapping, and the per-task + toggle surfaces resolve them via `resolveWorkflowOptionalSteps`. */ /** Seed graph for a brand-new workflow: start → end with room to insert steps. */ export function emptyWorkflowIr(name: string): WorkflowIr { From 68d3c5820e938d1e99acd6cd21735c52bf37943c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 20:33:32 -0700 Subject: [PATCH 11/13] docs(FN-6880): capture optional-group toggle-id collision learning Document the code-review P1 as a logic-errors learning: a per-task graph toggle (enabledWorkflowSteps, keyed by optional-group node id) collided with the legacy step-template namespace and was silently remapped by the store resolver, bypassing an enabled group. Cross-references the per-task-override blast-radius cousins as the id-namespace-collision variant of that class. Seeds an "Optional step group" entry in CONCEPTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- CONCEPTS.md | 5 + ...toggle-id-remapped-by-step-materializer.md | 102 ++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md diff --git a/CONCEPTS.md b/CONCEPTS.md index c6990d47c7..5549f4ffa3 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -250,6 +250,11 @@ A workflow graph node that reads a declared Artifact and runs a registry parser ### Custom task field A workflow-declared, typed task field (`string | text | number | boolean | enum | multi-enum | date | url`, with enum options and render hints) whose values live in `tasks.customFields`, keyed by field id. The task model is thereby recast as core fields (title, description) + standard metadata + these workflow-defined fields. Writes pass through a single store authority (`updateTaskCustomFields`) that validates each value against the resolving workflow's schema and returns typed rejections (offending `fieldId` + `code`); agents write them via `fn_task_update`'s `custom_fields` patch. Editing a workflow's fields or switching a task's workflow orphans (never destroys) values for removed or type-incompatible ids — orphans are retained and surfaced under a detail disclosure, excluded from cards. Same id means the same field within a project; there is no cross-workflow shared field namespace. +### Optional step group +A workflow graph container node (alongside `foreach`/`loop`) whose template subgraph runs once when a task has enabled it and is bypassed otherwise — the graph-native way to make a step optional per task. Enablement is a per-task toggle set seeded from the group's workflow-level default; the group's own node id is the toggle key. It replaces the earlier execution-inert *declaration* model (a separate optional-step list run through a hidden seam), so optional steps are now real, placeable nodes rather than an out-of-graph facet. + +Single pass — no iteration or rework inside the template (this is what distinguishes it from `foreach`/`loop`). Because the toggle key is the node id, renaming or recreating a group resets its per-task enablement; and because that id may deliberately equal a built-in step-template id, the per-task enable set must keep group ids identity-stable rather than round-tripping them through legacy step-template materialization (which would remap the key and silently bypass the group). + ## Persistence & migrations ### Schema-Version Sweep diff --git a/docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md b/docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md new file mode 100644 index 0000000000..4221daeea7 --- /dev/null +++ b/docs/solutions/logic-errors/optional-group-toggle-id-remapped-by-step-materializer.md @@ -0,0 +1,102 @@ +--- +title: "Optional-group enable toggle silently bypassed — node id collided with a legacy step-template namespace and was remapped" +date: 2026-06-21 +category: docs/solutions/logic-errors +module: engine (workflow store + graph executor) +problem_type: logic_error +component: service_object +symptoms: + - "Enabling a built-in optional-group (browser-verification) on a coding/stepwise task did nothing — the group's steps never ran." + - "The default-on seed path and direct graph-executor unit tests passed, masking the bug; only user-driven enable (create-with-enable or update/toggle) failed." + - "No error surfaced — the enabled group was silently bypassed." +root_cause: logic_error +resolution_type: code_fix +severity: high +related_components: + - workflow-store + - graph-executor + - optional-group +tags: + - optional-group + - enabledworkflowsteps + - per-task-override + - id-collision + - workflow-store + - silent-bypass +--- + +# Optional-group enable toggle silently bypassed — node id collided with a legacy step-template namespace and was remapped + +## Problem + +A graph-native `optional-group` workflow node is enabled per task via the `enabledWorkflowSteps` array, keyed by the group's **node id**. The graph executor runs the group only when `task.enabledWorkflowSteps.includes(node.id)`. But the store's `resolveEnabledWorkflowSteps` ran every id through the **legacy step-template materializer** (`getBuiltInWorkflowTemplate` → `ensureWorkflowStepForTemplate`). The built-in `browser-verification` group deliberately reused the template id `"browser-verification"` as its node id (for back-compat), so that id matched a `WORKFLOW_STEP_TEMPLATES` entry and was **remapped to a materialized `WorkflowStep` row id** (≠ the node id). The executor's membership check then never matched, and the enabled group was silently bypassed — the headline use case (turn the optional step on) did nothing, with no error. + +## Symptoms + +- Enabling `browser-verification` on a coding/stepwise task ran nothing pre-merge. +- Direct graph-executor tests (which pass a raw `enabledWorkflowSteps: ["browser-verification"]`) and the default-on **seed** path passed — masking the defect. +- Only the **user-driven** enable paths failed: create-with-explicit-enable and `updateTask({ enabledWorkflowSteps })` (the per-task toggle in the UI). + +## What Didn't Work + +- **Trusting the existing tests.** The unit tests used group ids like `og-on`/`og-off` that do **not** collide with any `WORKFLOW_STEP_TEMPLATES` id, so `getBuiltInWorkflowTemplate` returned undefined and the id passed through untouched — the tests were green precisely because they avoided the colliding id. The bug only fires when the group id equals a built-in template id. +- **Assuming the executor test covered it.** The two-task divergence test enabled the group by writing `enabledWorkflowSteps` straight onto the task, bypassing the store's resolver — so it never exercised the remap. The defect lived entirely in the create/update **resolution** path, one layer above the executor. + +## Solution + +Pass a workflow's optional-group node ids through `resolveEnabledWorkflowSteps` **untouched** — they are executor toggle keys, not legacy step-template ids to be materialized. + +```ts +// NEW: enumerate every optional-group node id (regardless of defaultOn). +export function resolveAllOptionalGroupIds(ir: WorkflowIr): string[] { + return resolveWorkflowOptionalSteps(ir).map((step) => step.templateId); // templateId === group node id +} + +// store.ts — the resolver gains an optional pass-through set: +private async resolveEnabledWorkflowSteps( + stepIds?: string[], + optionalGroupIds?: Set, +): Promise { + // ... + // Optional-group toggle ids pass through raw — never materialized as legacy step rows. + const template = optionalGroupIds?.has(stepId) + ? undefined + : this.getBuiltInWorkflowTemplate(stepId); + const resolvedId = template ? (await this.ensureWorkflowStepForTemplate(stepId)).id : stepId; + // ... +} + +// helper resolving the task's workflow IR → its optional-group id set: +private async optionalGroupIdSet(workflowId?: string | null): Promise> { + const wfId = workflowId ?? (await this.getDefaultWorkflowId()); + if (!wfId) return new Set(); + const def = await this.getWorkflowDefinition(wfId); + if (!def || def.kind === "fragment") return new Set(); + return new Set(resolveAllOptionalGroupIds(def.ir)); +} +``` + +Both user-enable call sites supply the set: create (`optionalGroupIdSet(input.workflowId)`) and update (`optionalGroupIdSet(getTaskWorkflowSelection(task.id)?.workflowId)`). + +**Regression test** — must use a **colliding** id (`browser-verification`), since non-colliding ids never reproduce it: create-with-enable and update/toggle both assert the raw group node id survives in `enabledWorkflowSteps`. + +## Why This Works + +The bug is a **per-task override that is read correctly at the action site but rewritten en route**. The override (`enabledWorkflowSteps`) was consulted exactly where the action runs (the graph executor), but the value was mutated in the **resolution path** before it got there, because two id namespaces overlap: graph-native optional-group **node ids** and legacy **`WorkflowStep` template ids**. The materializer is meaningful only for the retired declaration/`workflow-step`-seam execution model; for a graph-native group it is pure harm. Marking group ids as pass-through keeps the key **identity-stable** from definition through every consumer, so the executor's `includes(node.id)` check matches. + +(Verified the related slim-projection trap does **not** apply: the executor reads `enabledWorkflowSteps` off the `TaskDetail` snapshot it is handed, not a column-narrowed SELECT, so the array is fully hydrated.) + +## Prevention + +- **When introducing a new identity/key that shares a namespace with an existing one, grep every reader AND every *transformer* of that key.** A silent remap in a resolver is as fatal as a missing read — the override "survives" but as the wrong value. Demand each consumer is either re-keyed or argued identity-stable. +- **Regression tests for namespace collisions must use a *colliding* value.** A test with a deliberately distinct id proves nothing about the collision; pick the id that actually overlaps the legacy namespace (here, a built-in template id reused as a node id). +- **Test the path the user actually takes, not just the layer under test.** The executor-level test bypassed the store resolver where the bug lived; a create/update round-trip through the store would have caught it. Prefer at least one end-to-end seam test per per-task facet. +- **A facet that "works on seed/default but not on toggle" is the tell.** Asymmetry between the seed path (writes raw ids) and the user-enable path (runs the resolver) localizes the defect to the resolver. + +## Related Issues + +This is the **id-namespace-collision variant** of the per-task/per-entity override blast-radius class. Same disease (override invisible to the user, no error), different organ (key rewritten in resolution vs. not consulted at a trigger gate): + +- [Per-task auto-merge override ignored by trigger-layer gates](../logic-errors/per-task-auto-merge-override-ignored-by-trigger-gates.md) — sibling: override dead from the user's perspective; theirs is a missed trigger gate, ours is a resolution-path key remap. Its "consult the override everywhere between definition and action" rule covers this case too. +- [Per-entity execution-principal override: the full blast-radius checklist](../architecture-patterns/per-entity-execution-principal-override-blast-radius.md) — the generalizing checklist; closest prior art is its "validate composite node ids against the graph, never round-trip them" example. This bug is a new bullet for that checklist. +- [Workflow-native execution through runtime primitives](../architecture-patterns/workflow-native-runtime-primitives.md) — context: the legacy-`WorkflowStep`-row vs. graph-node two-control-planes tension this collision exploits. From e4a810e9b43cabef117543a06e7dd2dc7f0ad440 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 20:47:31 -0700 Subject: [PATCH 12/13] fix(FN-6880): address PR review feedback (#1712) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- .../retire-optional-steps-declaration.md | 4 ++- ...-workflow-optional-group-subgraphs-plan.md | 26 ++++++++++--------- .../workflow-ir-optional-group.test.ts | 7 +++++ packages/core/src/workflow-ir.ts | 23 +++++++++++++--- packages/core/src/workflow-optional-steps.ts | 6 +++-- .../__tests__/workflow-flow-mapping.test.ts | 20 +++++++++----- .../app/components/workflow-flow-mapping.ts | 7 ++++- ...-coding-browser-verification-group.test.ts | 5 ++++ .../engine/src/workflow-graph-executor.ts | 10 ++++--- 9 files changed, 78 insertions(+), 30 deletions(-) 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, From 95ec4bcdb6bc6d58e192a95daa8b026d3e55c656 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 22 Jun 2026 02:45:46 -0700 Subject: [PATCH 13/13] docs(FN-6880): reconcile changeset + note P1 fix (review #1712) Address CodeRabbit out-of-diff review comments: the feature changeset no longer claims the legacy declaration surface "remains" (U7a retired it; the sibling changeset documents the removal), and now calls out the optional-group enable id-collision fix for release-note visibility. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/workflow-optional-group-subgraphs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/workflow-optional-group-subgraphs.md b/.changeset/workflow-optional-group-subgraphs.md index 46a98cc066..c60dad2060 100644 --- a/.changeset/workflow-optional-group-subgraphs.md +++ b/.changeset/workflow-optional-group-subgraphs.md @@ -2,4 +2,4 @@ "@runfusion/fusion": minor --- -Workflow editor: optional steps are now graph-native. A new `optional-group` container node (foreach/loop-style) holds a subgraph the executor runs once when the group is enabled for a task (per-task `enabledWorkflowSteps` + workflow `defaultOn`) and bypasses when disabled. All seven built-in add-ons (documentation-review, qa-check, security-audit, performance-review, accessibility-check, browser-verification, frontend-ux-design) are insertable from the node-editor palette as a node or wrapped in an optional-group. The built-in coding and stepwise-coding workflows now express `browser-verification` as an optional-group. The legacy declaration-based optional-steps surface remains for back-compat; its full removal is a follow-up. +Workflow editor: optional steps are now graph-native. A new `optional-group` container node (foreach/loop-style) holds a subgraph the executor runs once when the group is enabled for a task (per-task `enabledWorkflowSteps` + workflow `defaultOn`) and bypasses when disabled. All seven built-in add-ons (documentation-review, qa-check, security-audit, performance-review, accessibility-check, browser-verification, frontend-ux-design) are insertable from the node-editor palette as a node or wrapped in an optional-group. The built-in coding and stepwise-coding workflows now express `browser-verification` as an optional-group. Optional-group enable resolution correctly handles id collisions with add-on template ids, so a group's enable state is not silently bypassed during task creation/update. (The legacy declaration-based optional-steps model is retired in a sibling changeset; only the `workflow-step` seam infrastructure removal remains a follow-up.)