diff --git a/.changeset/fn-6939-dev-server-narrow-preview-modal.md b/.changeset/fn-6939-dev-server-narrow-preview-modal.md new file mode 100644 index 0000000000..2133d343fb --- /dev/null +++ b/.changeset/fn-6939-dev-server-narrow-preview-modal.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix narrow right-sidebar Dev Server preview overlap by replacing the inline preview with an accessible modal launcher when the dock is very narrow, while keeping inline preview for full-page, mobile viewport, and expanded pop-out hosts. diff --git a/.changeset/fn-6953-ntfy-test-unsaved-config.md b/.changeset/fn-6953-ntfy-test-unsaved-config.md new file mode 100644 index 0000000000..6371518d61 --- /dev/null +++ b/.changeset/fn-6953-ntfy-test-unsaved-config.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Fix ntfy test notifications to honor unsaved Settings form config so users can enable ntfy, enter a valid topic/server/token, and send a test notification before saving. diff --git a/.changeset/retire-optional-steps-declaration.md b/.changeset/retire-optional-steps-declaration.md new file mode 100644 index 0000000000..3ce5035347 --- /dev/null +++ b/.changeset/retire-optional-steps-declaration.md @@ -0,0 +1,7 @@ +--- +"@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/.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/.changeset/workflow-optional-group-subgraphs.md b/.changeset/workflow-optional-group-subgraphs.md new file mode 100644 index 0000000000..c60dad2060 --- /dev/null +++ b/.changeset/workflow-optional-group-subgraphs.md @@ -0,0 +1,5 @@ +--- +"@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. 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.) diff --git a/CONCEPTS.md b/CONCEPTS.md index c68ffea0ef..2afdad7e97 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -264,6 +264,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/dashboard-guide.md b/docs/dashboard-guide.md index de1e9379d9..0ce4b15586 100644 --- a/docs/dashboard-guide.md +++ b/docs/dashboard-guide.md @@ -866,6 +866,9 @@ Features: - Start, stop, and restart the current server session - Manage preview URLs with embedded preview and **Open in new tab** fallback - Tail live logs, load older history, and refresh session status +- When Dev Server is hosted in a very narrow right sidebar, open the preview from the compact **Open preview** launcher; the modal keeps preview actions available while configuration and logs stay usable in the sidebar. + + For module-level behavior and API surfaces, see [Dev Server modules](./dev-server-modules.md). 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 new file mode 100644 index 0000000000..03d9c3650d --- /dev/null +++ b/docs/plans/2026-06-21-002-feat-workflow-optional-group-subgraphs-plan.md @@ -0,0 +1,626 @@ +--- +title: "feat: Optional-group container nodes + add-ons as insertable subgraphs" +status: active +date: 2026-06-21 +type: feat +plan_id: 2026-06-21-002-feat-workflow-optional-group-subgraphs +--- + +# feat: Optional-group container nodes + add-ons as insertable subgraphs + +## Summary + +Today "optional steps" are **execution-inert declarations**: a workflow lists `optionalSteps: +[{ templateId, defaultOn }]`, the create/edit UI seeds a per-task `enabledWorkflowSteps` set, and a +single hidden `workflow-step` seam node runs every enabled step *after* the graph finishes. Nothing about +optionality is visible in the graph, and the steps cannot be placed, ordered, or composed. + +This plan makes optionality **graph-native**. A new `optional-group` container node — modeled on the +existing `foreach`/`loop` container nodes — holds a `template:{ nodes, edges }` subgraph. The graph +executor runs that subgraph **once** when the group is enabled for the task and **passes through +(skips)** it when disabled. Enable state reuses the existing per-task `enabledWorkflowSteps` facet plus a +workflow-level `defaultOn`, keyed by the group. Separately, every prior pre-workflow **add-on** (the seven +`WORKFLOW_STEP_TEMPLATES`: documentation-review, qa-check, security-audit, performance-review, +accessibility-check, browser-verification, frontend-ux-design) becomes **insertable from the editor's +template palette as a node subgraph**, and can be inserted already wrapped in an `optional-group` so an +author can drop in "Security Audit (optional)" in one action. + +Finally, the plan **replaces** the declaration-based system per the confirmed scope decision: the built-in +**coding** and **stepwise-coding** workflows migrate `browser-verification` onto an `optional-group`, and +the now-dead `WorkflowOptionalStep` / `optionalSteps` declaration, `resolveWorkflowOptionalSteps` source, +the `workflow-step` seam node, and its compiler seam-anchor are retired — without breaking the create/edit +toggle surfaces, which re-point to the new source. + +**Plan depth:** Deep. Cross-cutting across core IR + validation, the engine graph executor, per-task +persistence/seeding, the visual node editor, the built-in workflows, and a behavior-affecting removal of +the legacy execution path. + +--- + +## Problem Frame + +The declaration model has three structural limits this plan removes: + +- **Invisible & unplaceable.** `optionalSteps` never appears in the graph + (`packages/core/src/workflow-ir-types.ts:314-339`, marked "Execution-inert; the graph executor ignores + this facet"). All enabled steps run in one lump at the `workflow-step` seam + (`packages/engine/src/executor.ts` `runWorkflowSteps`, gated on `enabledWorkflowSteps`), so an author + cannot put an optional step *between* two graph nodes, order multiple optional steps, or branch on one. +- **Add-ons are flat, not composable.** A `WorkflowStepTemplate` + (`packages/core/src/types.ts:868-899`) is a flat prompt/script config. The seven built-in add-ons appear + in the editor palette today only as **single "Built-in steps"** entries + (`WorkflowNodeEditor.tsx:1002` `stepEntries`), not as subgraphs you can compose or gate. +- **Two ways to express "run this sometimes."** The graph already has real conditional routing + (`shouldTraverseEdge` in `packages/engine/src/workflow-graph-executor.ts:657`, edge `condition` of + `success`/`failure`/`outcome:`) and container nodes (`foreach`/`loop`), yet optionality lives in a + parallel, execution-inert declaration channel. Converging optionality onto the graph removes the split. + +The graph already provides every seam this needs: container nodes compile/execute via a `template` +subgraph (`WorkflowForeachConfig`/`WorkflowLoopConfig`, `workflow-ir-types.ts:129-165`), the executor +dispatches them in `runNodeAndTraverse` (`workflow-graph-executor.ts:431-485`), the editor renders them as +React Flow group nodes with `parentId` children (`workflow-flow-mapping.ts:46-82,316-403,431-550`), and the +palette can already insert multi-node subgraphs via `insertFragment` (`WorkflowNodeEditor.tsx:1425-1430`). +The work is to add one new container kind that branches on a per-task toggle, project the add-on catalog +into the palette as subgraphs, and migrate the built-ins off the legacy path. + +--- + +## Requirements + +- **R1 — Optional-group container kind.** Add an `optional-group` node kind to the IR carrying a + `template:{ nodes, edges }` subgraph, mirroring `WorkflowForeachConfig`. Parse + validate it. +- **R2 — Run-or-bypass execution.** The graph executor runs the group's template **once** when the group + is enabled for the task and **passes through** (skips the subgraph, continues to the group's children) + when disabled. No rework budget; a single pass. +- **R3 — Enable state reuses the per-task facet.** Whether a group runs is driven by the existing per-task + `enabledWorkflowSteps` set plus a workflow-level `defaultOn` on the group, keyed by the group's stable + id. New tasks seed their enabled set from each group's `defaultOn` at creation. +- **R4 — Author optional groups in the node editor.** An author can add an `optional-group` container, + name it, set `defaultOn`, and place nodes inside it — reusing the foreach/loop group UX. The node type is + registered so it renders (not as `react-flow__node-default`). +- **R5 — Every add-on is an insertable subgraph.** All seven `WORKFLOW_STEP_TEMPLATES` add-ons are + insertable from the editor palette as a node subgraph, and offered with an "insert as optional group" + variant that drops the add-on wrapped in an `optional-group` (seeded `defaultOn`). +- **R6 — Built-ins migrated, behavior preserved.** The coding and stepwise-coding built-ins express + `browser-verification` as an `optional-group` (default OFF). A task with it enabled runs the step; a task + with it disabled does not — proven by an execution-level (not traversal-only) test. +- **R7 — Legacy path retired without surface breakage.** The `WorkflowOptionalStep`/`optionalSteps` + declaration, `resolveWorkflowOptionalSteps` as the toggle source, the `workflow-step` seam node, and its + compiler seam-anchor are removed. The create/edit toggle surfaces (inline card, New Task modal, Workflow + tab, steps dropdown) keep working by resolving their toggle list from `optional-group` nodes instead. +- **R8 — Validation invariants hold.** Optional-group templates are validated by walking the subgraph + (children are not in `ir.nodes`): all template nodes reachable, no illegal (non-rework) cycles, no seam + nodes inside a group. Graphs with no optional groups serialize byte-identically (R9 of prior art). +- **R9 — Additive serialization.** A workflow with no optional groups round-trips through the node editor + byte-identically; `optional-group` introduces no new top-level IR keys (it is just a node kind). + +--- + +## High-Level Technical Design + +### Execution: container branches on the per-task toggle + +The new dispatch slots into `runNodeAndTraverse` beside `foreach`/`loop`. Enabled → run the template +sub-walk once (reuse the loop/foreach template-walk machinery, no rework budget); disabled → return success +and traverse the group's children, skipping the body entirely. + +```mermaid +flowchart TD + Prev["upstream node"] --> OG{{"optional-group node\n(id, defaultOn, template)"}} + OG -->|"enabled = task.enabledWorkflowSteps.includes(group.id)"| CHK{enabled?} + CHK -->|yes| RUN["run template sub-walk ONCE\n(template.nodes/edges, single pass)"] + CHK -->|no| SKIP["pass through\noutcome=success, value=bypassed"] + RUN --> CHILD["traverseChildren(OG, result)"] + SKIP --> CHILD + CHILD --> Next["downstream node"] +``` + +Key boundary: the **decision** (run vs skip) is read from per-task state at the trigger seam, exactly like +the existing per-task auto-merge override — so it must be consulted wherever the run is gated, not only in +the executor branch (see Risks R-2). The **body** is an ordinary subgraph the executor already knows how to +walk. + +### Authoring + add-on projection: catalog → palette → graph + +```mermaid +flowchart LR + CAT["WORKFLOW_STEP_TEMPLATES\n(7 add-ons: flat prompt/script config)"] + CAT -->|"project to subgraph entry"| PAL["editor template palette\n(Built-in steps → subgraph entries)"] + PAL -->|"insert as node"| N["prompt/script node\n(add-on config)"] + PAL -->|"insert as optional group"| OGW["optional-group{ template:[ add-on node ], defaultOn }"] + N --> CANVAS["canvas IR"] + OGW --> CANVAS + CANVAS -->|"flowToIr / irToFlow\n(group children via parentId)"| IR["WorkflowIrV2"] +``` + +The add-on→subgraph projection reuses the existing `insertFragment` subgraph-insertion path +(`WorkflowNodeEditor.tsx:1425`), which already remaps ids and rewires internal edges — so "insert as +optional group" is a wrap-then-insert, not a new insertion engine. + +--- + +## Key Technical Decisions + +- **KTD-1 — `optional-group` is a container node mirroring `WorkflowForeachConfig`, not a bypass edge.** + Per the confirmed scope decision, optionality is encapsulated in a container (foreach/loop style) holding + a `template:{ nodes, edges }`, rather than inline nodes plus an explicit bypass edge. This reuses the + entire group-node toolchain (IR config, validation, React Flow `parentId` children, `insertFragment`), + and the "skip" is the container passing through rather than a visible routed edge. + +- **KTD-2 — Enable state reuses `enabledWorkflowSteps`, keyed by the group's stable node id.** No new + persistence. The per-task `tasks.enabledWorkflowSteps` column (`db.ts:324`, + `store.ts:424,2140`) holds the ids of enabled groups; `defaultOn` on the group seeds it at task creation + via the existing materialization path. Keying on the **node id** (stable for built-ins and preserved + across editor round-trips, like foreach/loop ids) means renaming/recreating a group resets its per-task + state — acceptable and identical to today's `templateId` keying. + +- **KTD-3 — Single pass, no rework budget.** Unlike `foreach` (per-step) and `loop` (bounded repeat), an + optional-group runs its template exactly once when enabled. Reuse the loop/foreach template-walk helper + but disable rework/iteration. Rework edges are **forbidden inside** an optional-group template (validation + rejects them) to keep the single-pass guarantee unambiguous. + +- **KTD-4 — Re-point the toggle resolver, don't keep two sources.** `resolveWorkflowOptionalSteps` + (`workflow-optional-steps.ts`) currently maps `ir.optionalSteps` → display metadata for the create/edit + UI. Replace its source with a scan of `optional-group` nodes (id, group name, `defaultOn`), preserving its + output shape (`ResolvedWorkflowOptionalStep[]`) so the inline card, New Task modal, Workflow tab, and + steps dropdown keep consuming it unchanged. This is what lets R7 retire the declaration without breaking + the four toggle surfaces. + +- **KTD-5 — Add-ons stay flat configs; the palette projects them to subgraphs at insert time.** Do **not** + rewrite `WorkflowStepTemplate` into a nodes+edges shape. A template projects to a single `prompt`/`script` + node (carrying its `prompt`/`scriptName`/`toolMode`/`gateMode`/`phase`/model), and the "optional" variant + wraps that node in an `optional-group`. Keeping the catalog flat avoids migrating plugin-contributed + templates and keeps the resolver/seeding logic simple. + +- **KTD-6 — `workflow-step` seam removal is a compiler change, not just an IR edit.** `workflow-step` is a + registered seam anchor in the compiler's canonical pipeline + (`workflow-compiler.ts:45,170` `planning → execute → workflow-step → review → merge`). Retiring it + requires removing it from `SEAM_NAMES`/`expectedSeamOrder` and updating the built-in IRs + their + byte-identity parity oracles together, or the compiler will reject (or mis-order) the migrated graphs. + +--- + +## Implementation Units + +Grouped into three phases: **A — core/engine** (the construct runs), **B — editor** (authoring + add-on +palette), **C — migration/cleanup** (built-ins on the new model, legacy path retired). + +### Phase A — Core construct + execution + +### U1. `optional-group` IR type, parse, and validation + +**Goal:** Introduce the `optional-group` node kind and its `template` config, and validate it by walking +the subgraph (R1, R8). + +**Requirements:** R1, R8, R9 + +**Dependencies:** none + +**Files:** +- `packages/core/src/workflow-ir-types.ts` (modify — add `"optional-group"` to `WorkflowIrNodeKind`; add + `WorkflowOptionalGroupConfig { defaultOn?: boolean; template: { nodes; edges } }` mirroring + `WorkflowForeachConfig` at `:129`) +- `packages/core/src/workflow-ir.ts` (modify — `validateV2` calls a new `validateOptionalGroup(node, ...)`; + extend cycle/reachability/seam walks to descend into the template subgraph) +- `packages/core/src/__tests__/workflow-ir.test.ts` (modify/create — validation cases) + +**Approach:** +- Add the kind + config type. The group's stable enable key is its node `id` (KTD-2); `defaultOn` lives on + the node `config`. +- In validation, treat the template like a foreach template: its nodes are **not** in `ir.nodes`, so every + reader/validator must walk the subgraph explicitly (learning: per-entity blast-radius `:37`). Validate: + all template nodes reachable from the template's entry; endpoints reference template-local nodes; **no + rework edges** inside (KTD-3); **no seam nodes** inside (mirror `validateParallelism`'s seam-in-branch + rule, `workflow-ir.ts:190+`). +- `parseWorkflowIr` descends into optional-group templates the same way it clamps foreach/loop configs. + +**Patterns to follow:** `WorkflowForeachConfig` type + `validateV2`/foreach template validation in +`workflow-ir.ts`; the seam-in-branch check in `validateParallelism`. + +**Test scenarios:** +- A v2 IR with one `optional-group` (valid template) parses and validates. +- A template referencing an undefined template-local node throws `WorkflowIrError`. +- A rework edge inside an optional-group template is rejected. +- A seam node (e.g. `merge-gate`) inside an optional-group template is rejected. +- An unreachable template node is rejected. +- A graph with **no** optional-group serializes/parses byte-identically (R9). + +**Verification:** `pnpm --filter @fusion/core test workflow-ir` green; no change to graphs without optional +groups. + +--- + +### U2. Executor: run-once-or-bypass dispatch for `optional-group` + +**Goal:** Make the graph executor run an enabled group's template once and pass through a disabled group +(R2), reading enable state from per-task `enabledWorkflowSteps` (R3). + +**Requirements:** R2, R3 + +**Dependencies:** U1 + +**Files:** +- `packages/engine/src/workflow-graph-executor.ts` (modify — add an `optional-group` branch in + `runNodeAndTraverse` at `:389-485`, beside `foreach`/`loop`) +- `packages/engine/src/workflow-graph-loop.ts` or a shared helper (modify/extract — reuse the + single-template-walk without iteration/rework for the enabled path) +- `packages/engine/src/__tests__/workflow-graph-optional-group.test.ts` (create — execution-level tests) + +**Approach:** +- Resolve `enabled = currentTask.enabledWorkflowSteps?.includes(node.id) ?? false`. **Read the freshest + task state** at the seam (the executor already re-reads the task in `runWorkflowSteps`); ensure + `enabledWorkflowSteps` is in whatever projection the gate reads (learning: per-task override slim-SELECT + trap `:105`). +- Enabled → walk `node.config.template` once via the extracted helper, threading the same + `runTemplateNode`/`shouldTraverseEdge` deps the foreach/loop handlers pass; collect `visitedNodeIds`; + set `context[node:id:outcome]`. Disabled → `return await traverseChildren(node, { outcome: "success", + value: "optional-group-bypassed" })`. +- Namespaced child ids: reuse the foreach instance-id scheme defensively — parse candidate-style and + validate the template node exists, not just the container (learning `:56`). + +**Execution note:** Start with the failing **two-task divergence** execution test (below) — it is the +contract that guards the dead-toggle failure mode. + +**Test scenarios:** +- **Two-task divergence (critical):** two tasks identical except `enabledWorkflowSteps` — the one including + the group id records the template's node execution; the sibling records none and reaches the same + downstream node. (Shape from per-task-auto-merge learning `:106`.) +- Enabled group runs its template exactly **once** (not per-step, not looped) even when the graph has a + foreach elsewhere. +- Disabled group is byte-inert: downstream context/outcome identical to a graph with the group removed + (kill-switch inertness, learning `:55`). +- A template node failure inside an enabled group surfaces as the group's outcome and routes the group's + `failure`/`outcome:` edges. + +**Verification:** new engine test green; existing graph-executor tests unaffected. + +--- + +### U3. Per-task enable resolution from optional-group nodes + `defaultOn` seeding + +**Goal:** Re-point the per-task toggle source from `ir.optionalSteps` to `optional-group` nodes and seed +new tasks' enabled set from each group's `defaultOn` (R3, R7-prep). + +**Requirements:** R3, R7 + +**Dependencies:** U1 + +**Files:** +- `packages/core/src/workflow-optional-steps.ts` (modify — `resolveWorkflowOptionalSteps` scans + `optional-group` nodes instead of `ir.optionalSteps`, preserving `ResolvedWorkflowOptionalStep[]` output) +- `packages/core/src/` task-creation/materialization path that seeds `enabledWorkflowSteps` from `defaultOn` + (modify — seed from optional-group `defaultOn`; today's `materializeDefaultWorkflowSteps`, see + `types.ts:2668-2675`) +- `packages/core/src/__tests__/workflow-optional-steps.test.ts` (modify — group-sourced resolution) + +**Approach:** +- Resolver: walk `ir` (v2) nodes, collect `optional-group` nodes → `{ id, name (from config), defaultOn }`. + Keep output shape identical so the four UI surfaces (inline card, modal, Workflow tab, dropdown) need no + change beyond what U7/U-editor already covers. Unknown/stale ids in `enabledWorkflowSteps` are ignored + (defensive, as today). +- Seeding: at task creation, the enabled set is the ids of optional-group nodes whose effective + `defaultOn` is true — mirroring the prior `optionalStep.defaultOn ?? false` precedence. +- Grep every reader of `enabledWorkflowSteps`/`optionalSteps` and confirm each now resolves from groups + (learning: consult the override at every trigger seam `:100,:102`). + +**Test scenarios:** +- `resolveWorkflowOptionalSteps` over a workflow with two optional-group nodes returns both, with names and + `defaultOn` from node config. +- A new task created against that workflow seeds `enabledWorkflowSteps` to exactly the `defaultOn: true` + group ids. +- A workflow with no optional-group nodes resolves to `[]` and seeds an empty set. +- Stale id in `enabledWorkflowSteps` (group since removed) does not crash resolution or execution. + +**Verification:** core optional-steps tests green; creation seeding covered. + +--- + +### Phase B — Editor authoring + add-on palette + +### U4. Render & author the `optional-group` container in the node editor + +**Goal:** Let an author add, name, configure (`defaultOn`), and fill an `optional-group` container, +reusing the foreach/loop group UX, with the node type registered so it renders (R4). + +**Requirements:** R4 + +**Dependencies:** U1 (kind exists) + +**Files:** +- `packages/dashboard/app/components/nodes/WorkflowNodeTypes.tsx` (modify — add `optional-group` to the + node-type registry + icon, render as a group container like `foreach`/`loop`) +- `packages/dashboard/app/components/workflow-flow-mapping.ts` (modify — treat `optional-group` as a group + kind everywhere foreach/loop are special-cased: `groupTemplateConfigOf` `:266`, group child + reassembly `:441-480`, intra-template edge handling `:517`, group delete `:611,:636`, + condition-editability `:663`) +- `packages/dashboard/app/components/WorkflowNodeEditor.tsx` (modify — inspector controls: group name + + `defaultOn` toggle; help entry) +- `packages/dashboard/app/components/nodes/node-help.ts` (modify — add an `optional-group` help entry) +- `packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts` (modify — round-trip group + children) +- `packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx` (modify — add/name/toggle/fill) + +**Approach:** +- Mirror the `foreach`/`loop` group node: a React Flow `type: "group"` with `parentId` children using the + existing `foreachChildFlowId` namespacing (`workflow-flow-mapping.ts:75`). `irToFlow` renders the + template as children; `flowToIr` reassembles it (the `:454,:480` kind checks gain `optional-group`). +- Inspector: a `defaultOn` checkbox (labeled, focus ring) and the group name; the body is authored by + dropping nodes inside, identical to foreach. +- **Register the node type** in `WorkflowNodeTypes.tsx` — an unregistered kind renders as + `react-flow__node-default` with missing children (learning: worktree bundle `:24`). Verify against a + fresh `FUSION_CLIENT_DIR` bundle, non-4040 port. + +**Test scenarios:** +- Adding an optional-group, dropping a prompt node inside, and saving yields IR with an `optional-group` + node whose `template.nodes` contains the inner node (round-trip). +- Toggling `defaultOn` marks the editor dirty and persists on save. +- Deleting the group removes its `parentId` children (no orphans) — mirrors the foreach delete test. +- The node renders with its registered type (not `react-flow__node-default`) — asserted via node-type + registry presence. + +**Verification:** mapping + editor tests green; real-browser check that the container renders, accepts +child nodes, and the `defaultOn` toggle works (fresh worktree bundle; verify on a mobile viewport per +Risks R-4). + +--- + +### U5. Add-ons as insertable subgraphs (plus "insert as optional group") + +**Goal:** Make all seven `WORKFLOW_STEP_TEMPLATES` add-ons insertable from the palette as node subgraphs, +each also offerable wrapped in an `optional-group` (R5). + +**Requirements:** R5, KTD-5 + +**Dependencies:** U4 (optional-group authoring exists) + +**Files:** +- `packages/dashboard/app/components/WorkflowNodeEditor.tsx` (modify — the "Built-in steps" palette + section `:1002,:2741`, the existing `stepTemplateToNode()` projector `:249-275`, and the + `handleInsertStepTemplate`/`handleInsertFragment` handlers `:1425-1453`: add an "insert as optional + group" variant per add-on) +- `packages/dashboard/app/components/workflow-flow-mapping.ts` (modify — `insertFragment` `:1110-1219` + already expands subgraphs incl. group `parentId` children; add the wrap-in-`optional-group` helper that + builds the fragment IR from a projected add-on node) +- `packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx` (modify — insert-as-node and + insert-as-optional-group for an add-on) + +**Approach:** +- Project each add-on to a node using the **existing `stepTemplateToNode()`** (`WorkflowNodeEditor.tsx: + 249-275`), which already maps a `WorkflowStepTemplate` → a `prompt`/`script` node carrying its + `prompt`/`scriptName`/`toolMode`/`gateMode`/model (KTD-5). "Insert as node" is today's behavior. +- "Insert as optional group" wraps that projected node in an `optional-group{ template:{ nodes:[node], + edges:[] }, defaultOn }` (seeded from the template's `defaultOn`) and inserts it through the existing + `insertFragment` path (`workflow-flow-mapping.ts:1110-1219`), which already remaps ids, rewires internal + edges, and expands group `parentId` children — so no new insertion engine is needed. +- Surface both variants in the palette's existing "Built-in steps" group; keep `data-testid` conventions. + +**Test scenarios:** +- Each of the seven add-ons appears in the palette and inserts a node carrying its template config. +- "Insert as optional group" for `security-audit` yields an `optional-group` whose `template` holds the + security-audit node and whose `defaultOn` matches the template default. +- Inserting an add-on subgraph remaps ids so two insertions of the same add-on do not collide. +- A plugin-contributed template (if present) still inserts as a node (catalog stays flat, KTD-5). + +**Verification:** editor tests green; real-browser insert of an add-on and an optional-group-wrapped add-on, +then save → reopen round-trip. + +--- + +### Phase C — Migrate built-ins, retire the legacy path + +### U6. Migrate coding + stepwise-coding built-ins to `optional-group` browser-verification + +**Goal:** Express `browser-verification` as an `optional-group` (default OFF) in both built-ins, preserving +runtime behavior, and update parity oracles (R6). + +**Requirements:** R6, R8 + +**Dependencies:** U2, U3 (execution + seeding), U1 (kind) + +**Files:** +- `packages/core/src/builtin-coding-workflow-ir.ts` (modify — replace `optionalSteps:[{templateId: + "browser-verification"}]` `:123` + the `workflow-step` seam node `:69` with a `browser-verification` + `optional-group` on the pre-merge path) +- `packages/core/src/builtin-stepwise-coding-workflow-ir.ts` (modify — add the `browser-verification` + optional-group on the pre-merge path; stepwise had no `workflow-step` seam at all) +- `packages/core/src/__tests__/` built-in IR snapshot/parity fixtures (modify — update byte-identity + oracles deliberately) +- `packages/engine/src/__tests__/` stepwise/coding execution parity test (modify — enabled-runs/disabled- + skips at the built-in level) + +**Approach:** +- Place the optional-group on the success path where `workflow-step` sat (pre-merge, after execute/steps, + before review), so an enabled task runs browser-verification pre-merge exactly as before. +- The stepwise IR is a documented byte-identity parity oracle — adding the construct shifts its snapshot; + update the fixture deliberately and confirm the group runs **once** post-foreach, not per step-instance + (learning `:55`, prior plan R-5). + +**Test scenarios:** +- `resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)` returns one `browser-verification` group, + `defaultOn: false`; same for stepwise. +- Execution: a coding task with the group enabled runs browser-verification pre-merge; disabled does not + (two-task divergence at the built-in level). +- Stepwise: same divergence; the group runs once after the foreach completes. +- Both built-ins parse and pass `validateV2`. + +**Verification:** core + engine built-in tests green; parity oracles updated and passing. + +--- + +### U7. Retire the declaration-based optional-steps path + +**Goal:** Remove the now-dead `WorkflowOptionalStep`/`optionalSteps` declaration, the `workflow-step` seam +node + handler, and its compiler seam-anchor, keeping the four toggle surfaces working via U3's resolver +(R7). + +**Requirements:** R7 + +**Dependencies:** U3 (resolver re-pointed), U6 (built-ins migrated — nothing still declares `optionalSteps`) + +**Files:** +- `packages/core/src/workflow-ir-types.ts` (modify — remove `WorkflowOptionalStep` + `WorkflowIrV2. + optionalSteps`) +- `packages/core/src/workflow-compiler.ts` (modify — remove `workflow-step` from `SEAM_NAMES` `:45` and + `expectedSeamOrder` `:170`) +- `packages/engine/src/executor.ts` (modify — remove `runWorkflowSteps` + the `workflow-step` seam + dispatch, now unreachable) +- `packages/dashboard/app/components/workflow-flow-mapping.ts` (modify — drop `optionalSteps` threading in + `flowToIr`/`optionalStepsOf` if present from the prior plan) +- `packages/core/src/__tests__/`, `packages/engine/src/__tests__/` (modify — delete/replace tests asserting + the legacy path; keep the resolver/seeding tests now backed by groups) + +**Approach:** +- This is a **behavior-affecting removal** — follow the codebase's Surface Enumeration discipline (see the + section below). Enumerate every reader of `optionalSteps`/`workflow-step`/`runWorkflowSteps` and confirm + each is migrated or removed; do not leave a mock-masked dead path (learning: branch-group dead-wiring). +- Removing the seam anchor changes the compiler's accepted pipeline — confirm no remaining built-in or + fragment references `workflow-step`, then drop it from both anchor lists together with the built-in edits + from U6. + +**Test scenarios:** +- Grep proves zero remaining references to `optionalSteps`, `WorkflowOptionalStep`, `workflow-step` seam, + and `runWorkflowSteps` in non-test source. +- The compiler accepts the migrated built-ins with `workflow-step` removed from the seam order. +- The four toggle surfaces (inline card, New Task modal, Workflow tab, steps dropdown) still render and + submit `enabledWorkflowSteps` — now sourced from optional-group nodes (regression). +- A pre-existing workflow JSON that still carries `optionalSteps` (legacy persisted) does not crash parse — + the key is ignored, not fatal (back-compat decision: tolerate-and-drop). *(Confirm this stance in review; + alternative is a one-time parse upgrade.)* + +**Verification:** full core + engine + dashboard suites green; `pnpm lint`, `pnpm typecheck`, `pnpm build`, +`pnpm test:gate` pass. + +--- + +## Surface Enumeration + +Behavior-affecting change (new execution construct + removal of the legacy path) and a UI-affordance change +(new node kind + retired toggle source), so per AGENTS.md ("Fix the Invariant, Not the Repro", FN-5893) the +surfaces are enumerated: + +- **Workflow providers/graphs:** built-in **coding** and **stepwise-coding** (both migrated, U6); any + fragment or user workflow that declared `optionalSteps` (tolerated-and-dropped, U7). +- **Execution states:** group **enabled** (runs once), **disabled** (passes through), **stale id** in + `enabledWorkflowSteps` (ignored), **template failure** (routes group failure edge). +- **Editor breakpoints:** desktop and **mobile** node editor — container render, child placement, + `defaultOn` toggle, palette insert (both variants). +- **Per-task toggle surfaces:** inline quick-create card, New Task modal, task-detail Workflow tab, steps + dropdown — all re-sourced from optional-group nodes (U3/U7). +- **Validation:** subgraph walked (not just `ir.nodes`); no rework/seam inside a group; graphs without + optional groups byte-identical. +- **Compiler:** seam-anchor list with `workflow-step` removed; canonical pipeline still valid for migrated + built-ins. + +## Symptom Verification + +- **Original symptom:** optionality is invisible and unplaceable — enabled steps run in one lump at a + hidden seam, and add-ons cannot be composed or gated in the graph. +- **Exact reproduction:** build/inspect the coding workflow; `browser-verification` appears only as an + `optionalSteps` declaration, runs at the `workflow-step` seam, and is absent from the graph; add-ons + appear in the palette only as flat single steps. +- **Assertion it is gone:** an enabled optional-group runs its placed template once at its graph position + and a disabled one is inert (two-task divergence execution test, U2/U6); every add-on inserts as a + node/optional-group subgraph (U5); no `workflow-step`/`optionalSteps` path remains (U7 greps). + +--- + +## Scope Boundaries + +**In scope:** +- `optional-group` IR kind + validation (U1), executor run/bypass (U2), per-task resolution + seeding (U3). +- Editor container authoring + node-type registration (U4); add-ons as insertable subgraphs incl. + optional-group wrapping (U5). +- Built-in coding + stepwise migration (U6); retiring the declaration/seam/compiler-anchor path (U7). + +**Already built (reuse, not rebuilt):** +- Container-node toolchain: `foreach`/`loop` config, group rendering, `parentId` children, + `insertFragment` subgraph insertion. +- Per-task `enabledWorkflowSteps` column, create-surface toggles, the four toggle UIs, and + `resolveWorkflowOptionalSteps`'s output shape (source re-pointed in U3). +- Step→node projection (`workflow-steps-to-ir.ts`) reused to project add-ons (U5). + +### 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). 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. +- The prior plan's deferred **unified per-task workflow-facet override** abstraction (optional steps + + auto-merge + column-agent overrides) — strong `/ce-compound` candidate once this lands. + +**Out of scope:** +- New persistence/migrations. Reuses `tasks.enabledWorkflowSteps`; `optional-group` is just a node kind. +- Rewriting `WorkflowStepTemplate` into a nodes+edges shape (KTD-5 keeps it flat). +- Changing unrelated execution (merge lifecycle, branch groups, PR nodes). + +--- + +## Risks & Dependencies + +- **R-1 — Subgraph-walking validation/readers miss template children.** Optional-group template nodes are + not in `ir.nodes`; any validator, reachability pass, or id parser that only sees top-level nodes will be + wrong. *Mitigation:* walk the subgraph explicitly and parse namespaced child ids candidate-style, + validating the template node exists, not just the container. + (`docs/solutions/architecture-patterns/per-entity-execution-principal-override-blast-radius.md:37,:56`) +- **R-2 — Per-task enable consulted only in the executor branch.** If skip-vs-run is read only where the + group executes and not at every gate/trigger that decides whether to run it, the toggle silently no-ops; + a slim SELECT omitting `enabledWorkflowSteps` reads `undefined`. *Mitigation:* grep every gate; ensure the + column is selected; ship the two-task divergence test. + (`docs/solutions/logic-errors/per-task-auto-merge-override-ignored-by-trigger-gates.md:100,:102,:105,:106`) +- **R-3 — Seam-anchor / parity-oracle drift on built-in migration.** `workflow-step` is a compiler seam + anchor and the stepwise IR is a byte-identity oracle; removing the seam and adding the construct shifts + snapshots and can break compile-order validation. *Mitigation:* edit built-ins, seam-anchor lists, and + parity fixtures together (KTD-6); confirm the group runs once post-foreach. + (`docs/solutions/architecture-patterns/workflow-native-runtime-primitives.md:117`; prior plan R-5) +- **R-4 — Editor node-type registration + mobile.** An unregistered `optional-group` renders as + `react-flow__node-default` with missing children, looking like a source bug; per-task toggle controls have + a real-browser-only mobile failure history. *Mitigation:* register the node type and verify against a + fresh `FUSION_CLIENT_DIR` worktree bundle on a non-4040 port; real-browser-check the `defaultOn` toggle on + a mobile viewport. + (`docs/solutions/developer-experience/browser-testing-dashboard-from-worktree-safely.md:24,:44`; + `docs/solutions/ui-bugs/mobile-auto-merge-toggle-document-scroll-blank.md:32,:53`) +- **R-5 — Mock-masked dead wiring on removal.** Retiring the legacy path risks a green suite over a feature + whose new path is never actually exercised (the branch-group failure class). *Mitigation:* execution-level + (not traversal-only) tests for enabled-runs/disabled-skips at both the construct and built-in levels; + grep-prove the old path is gone. + (`docs/solutions/integration-issues/branch-group-single-pr-synthetic-id-dead-wiring.md`) + +--- + +## Sources & Research + +- **Execution seam:** `packages/engine/src/workflow-graph-executor.ts:389-485` (`runNodeAndTraverse` + foreach/loop dispatch), `:657` (`shouldTraverseEdge`); `workflow-graph-foreach.ts`, + `workflow-graph-loop.ts` (template sub-walk to reuse). +- **IR + validation:** `packages/core/src/workflow-ir-types.ts:41-165` (node/edge/container types), + `:314-339` (legacy `optionalSteps`); `packages/core/src/workflow-ir.ts:190+` (seam-in-branch), + `:1218-1298` (`validateV2`, cycles/endpoints), `:1327` (`parseWorkflowIr`). +- **Per-task facet:** `packages/core/src/types.ts:2421,2663-2680` (`enabledWorkflowSteps`, `workflowId` + precedence), `db.ts:324`, `store.ts:424,2140`; `workflow-optional-steps.ts` (resolver to re-point); + `executor.ts` `runWorkflowSteps` (seam consumer to retire). +- **Add-on catalog:** `packages/core/src/types.ts:868-899` (`WorkflowStepTemplate`), `:902-1150` + (seven `WORKFLOW_STEP_TEMPLATES`: documentation-review, qa-check, security-audit, performance-review, + accessibility-check, browser-verification, frontend-ux-design); `WorkflowNodeEditor.tsx:249-275` + (`stepTemplateToNode` add-on→node projector to reuse). +- **Editor:** `packages/dashboard/app/components/workflow-flow-mapping.ts:46-82,266-269,316-403,431-550` + (group children, `groupTemplateConfigOf`), `:1110-1219` (`insertFragment` subgraph insertion); + `WorkflowNodeEditor.tsx:985-1010` (palette: fragments/steps/plugins, sourced from + `/api/workflow-step-templates`), `:1425-1453` (insert handlers); `nodes/WorkflowNodeTypes.tsx` (node-type + registry to extend). +- **Compiler:** `packages/core/src/workflow-compiler.ts:45,165-196` (seam anchors + canonical order). +- **Built-ins:** `packages/core/src/builtin-coding-workflow-ir.ts:69,123`, + `builtin-stepwise-coding-workflow-ir.ts:132`. +- **Prior art:** `docs/plans/2026-06-20-001-feat-workflow-optional-steps-node-editor-modal-plan.md` (the + declaration-based system this replaces); institutional learnings cited inline under Risks. diff --git a/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md b/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md index 385e7fc7fb..d16a26537d 100644 --- a/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md +++ b/docs/plans/2026-06-21-003-refactor-merger-unification-u0-plan.md @@ -23,7 +23,7 @@ It also installs the **R7 workspace merge-boundary guard** at every merge entry Merge is dispatched at `packages/engine/src/project-engine.ts:2275-2282`: -``` +```ts const mergerMode = normalizeMergerMode(settings.merger?.mode); // defaults to "ai" return mergerMode === "ai" ? runAiMerge(store, cwd, taskId, mergeOptionsWithSettings) @@ -61,7 +61,7 @@ Before claiming low blast radius, grep test fixtures, CI configs, and seeded/def ## Implementation Units > **Units `U1–U4` below are local to this plan** (they decompose master-plan U0); they are **not** the master plan's `U1–U10`. U4 (audit) may run in parallel with U1–U3. - +> > **Standing requirements:** `FNXC:Workspace ` dated comments at each non-obvious decision point (dispatch collapse, the R7 guard, the deprecation warning). A `.changeset/*.md` (`@runfusion/fusion: minor`). Respect the merge gate (`pnpm lint`, typecheck, `pnpm build`, `pnpm test:gate`) and FN-5048 (narrow seams, fake timers, no real polling / mock-the-world). **Base branch (decided):** branch off the **foundation** (`pr-1710` / `feat/workspace-multi-repo` head) — the R7 guard (U3) reads `task.workspaceWorktrees`, which the foundation adds and `main` lacks. Do **not** commit onto `pr-1710` directly; use a new branch and open a **stacked PR targeting `feat/workspace-multi-repo`** so the diff is only U0's changes. ### U1. Collapse the engine dispatch and route the two direct callers to `runAiMerge` 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. diff --git a/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts b/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts index 985260d595..7d7f31bfdf 100644 --- a/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts +++ b/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts @@ -38,11 +38,33 @@ describe("builtin coding workflow ir", () => { const seams = BUILTIN_CODING_WORKFLOW_IR.nodes .map((node) => String(node.config?.seam ?? "")) .filter((seam) => seam.length > 0); - expect(seams).toEqual(expect.arrayContaining(["execute", "workflow-step", "review"])); + expect(seams).toEqual(expect.arrayContaining(["execute", "review"])); + // U6: the `workflow-step` seam was replaced by the browser-verification + // optional-group; no node declares the legacy seam anymore. + expect(seams).not.toContain("workflow-step"); expect(seams).not.toContain("merge"); expect(seams).not.toContain("triage"); }); + it("expresses pre-merge browser-verification as a default-off optional-group (U6)", () => { + const byId = new Map(BUILTIN_CODING_WORKFLOW_IR.nodes.map((n) => [n.id, n])); + expect(byId.get("workflow-step")).toBeUndefined(); + const group = byId.get("browser-verification"); + expect(group?.kind).toBe("optional-group"); + expect(group?.config?.name).toBe("Browser Verification"); + expect(group?.config?.defaultOn).toBe(false); + // execute → browser-verification → review on the success path; failure → end. + expect(BUILTIN_CODING_WORKFLOW_IR.edges).toEqual( + expect.arrayContaining([ + expect.objectContaining({ from: "execute", to: "browser-verification", condition: "success" }), + expect.objectContaining({ from: "browser-verification", to: "review", condition: "success" }), + expect.objectContaining({ from: "browser-verification", to: "end", condition: "failure" }), + ]), + ); + // The legacy optionalSteps declaration is gone (the group replaces it). + expect("optionalSteps" in BUILTIN_CODING_WORKFLOW_IR).toBe(false); + }); + it("defines the six legacy columns in legacy order (KTD-1)", () => { expect(BUILTIN_CODING_WORKFLOW_IR.version).toBe("v2"); if (BUILTIN_CODING_WORKFLOW_IR.version !== "v2") throw new Error("expected v2"); @@ -73,16 +95,17 @@ describe("builtin coding workflow ir", () => { it("places seam nodes in their columns", () => { const byId = new Map(BUILTIN_CODING_WORKFLOW_IR.nodes.map((n) => [n.id, n])); expect(byId.get("execute")?.column).toBe("in-progress"); - expect(byId.get("workflow-step")?.column).toBe("in-progress"); + // U6: browser-verification optional-group replaces the workflow-step seam. + expect(byId.get("browser-verification")?.column).toBe("in-progress"); expect(byId.get("review")?.column).toBe("in-review"); expect(byId.get("merge-gate")?.column).toBe("in-review"); expect(byId.get("merge-attempt")?.column).toBe("in-review"); }); - it("assigns descriptive names to execute/workflow-step/review/merge seam nodes", () => { + it("assigns descriptive names to execute/review seam nodes and the browser-verification group", () => { const byId = new Map(BUILTIN_CODING_WORKFLOW_IR.nodes.map((n) => [n.id, n])); expect(byId.get("execute")?.config?.name).toBe("Execute"); - expect(byId.get("workflow-step")?.config?.name).toBe("Pre-merge workflow steps"); + expect(byId.get("browser-verification")?.config?.name).toBe("Browser Verification"); expect(byId.get("review")?.config?.name).toBe("Review"); }); @@ -94,9 +117,8 @@ describe("builtin coding workflow ir", () => { expect(config.maxRetries).toBeLessThanOrEqual(10); const byId = new Map(BUILTIN_CODING_WORKFLOW_IR.nodes.map((n) => [n.id, n])); - expect(byId.get("workflow-step")?.config?.name).toBe("Pre-merge workflow steps"); + expect(byId.get("browser-verification")?.config?.name).toBe("Browser Verification"); expect(byId.get("review")?.config?.name).toBe("Review"); - expect(byId.get("workflow-step")?.config?.maxRetries).toBeUndefined(); expect(byId.get("review")?.config?.maxRetries).toBeUndefined(); expect(byId.get("merge-attempt")?.config?.maxReworkCycles).toBe(3); }); diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index 7dbedd6888..491314877f 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -152,7 +152,11 @@ describe("built-in workflows", () => { const byId = new Map(ir.nodes.map((node) => [node.id, node])); expect(byId.get("execute")?.column).toBe("in-progress"); - expect(byId.get("workflow-step")?.column).toBe("in-progress"); + // U6: the legacy `workflow-step` seam is replaced by the pre-merge + // `browser-verification` optional-group, placed in the implementation column. + expect(byId.get("workflow-step")).toBeUndefined(); + expect(byId.get("browser-verification")?.kind).toBe("optional-group"); + expect(byId.get("browser-verification")?.column).toBe("in-progress"); expect(byId.get("review")?.column).toBe("in-review"); // Merge is the native primitive region (FN-6035), placed in in-review. expect(byId.get("merge")).toBeUndefined(); @@ -312,9 +316,12 @@ describe("built-in workflows", () => { expect(executeConfig?.maxRetries).toBeLessThanOrEqual(10); const byId = new Map(candidate.nodes.map((node) => [node.id, node])); - expect(byId.get("workflow-step")?.config?.name).toBe("Pre-merge workflow steps"); + // U6: pre-merge browser-verification is an optional-group (default OFF), + // not the legacy `workflow-step` seam. + expect(byId.get("workflow-step")).toBeUndefined(); + expect(byId.get("browser-verification")?.kind).toBe("optional-group"); + expect(byId.get("browser-verification")?.config?.name).toBe("Browser Verification"); expect(byId.get("review")?.config?.name).toBe("Review"); - expect(byId.get("workflow-step")?.config?.maxRetries).toBeUndefined(); expect(byId.get("review")?.config?.maxRetries).toBeUndefined(); // The merge lifecycle is no longer a single `merge` seam node (FN-6035): it // is expressed as the merge-gate/merge-attempt/branch-group primitive region. @@ -600,17 +607,22 @@ describe("built-in workflows", () => { description: "implicit builtin default", }); + // U6: builtin:coding now carries the `browser-verification` optional-group + // (an interpreter-deferred construct), so its DEFAULT-workflow materialization + // falls back to no legacy WorkflowStep rows and records no selection row — + // identical to the stepwise built-in below. The group is defaultOn:false, so + // enabledWorkflowSteps stays empty. await store.setDefaultWorkflowId("builtin:coding"); const codingTask = await store.createTask({ description: "default builtin coding" }); expect((await store.getTask(codingTask.id)).enabledWorkflowSteps ?? []).toEqual([]); - expect(store.getTaskWorkflowSelection(codingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: [] }); + expect(store.getTaskWorkflowSelection(codingTask.id)).toBeUndefined(); const reservedCodingTask = await store.createTaskWithReservedId( { description: "reserved default builtin coding" }, { taskId: "reserved-default-builtin-coding" }, ); expect((await store.getTask(reservedCodingTask.id)).enabledWorkflowSteps ?? []).toEqual([]); - expect(store.getTaskWorkflowSelection(reservedCodingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: [] }); + expect(store.getTaskWorkflowSelection(reservedCodingTask.id)).toBeUndefined(); await store.setDefaultWorkflowId("builtin:stepwise-coding"); const stepwiseTask = await store.createTask({ description: "default builtin stepwise" }); diff --git a/packages/core/src/__tests__/workflow-compiler.test.ts b/packages/core/src/__tests__/workflow-compiler.test.ts index 21086ebfcb..361db9cd16 100644 --- a/packages/core/src/__tests__/workflow-compiler.test.ts +++ b/packages/core/src/__tests__/workflow-compiler.test.ts @@ -4,7 +4,6 @@ import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; import { compileWorkflowToSteps, - MERGE_REGION_NODE_KINDS, validateLinearity, WorkflowCompileError, WORKFLOW_INTERPRETER_DEFERRED_SUFFIX, @@ -127,33 +126,25 @@ describe("compileWorkflowToSteps (U2)", () => { expect(() => compileWorkflowToSteps(ir)).toThrow(/interpreter \(deferred\)/i); }); - it("validates builtin workflow linearity while preserving stepwise interpreter deferral", () => { - expect(validateLinearity(BUILTIN_CODING_WORKFLOW_IR)).toBeNull(); + it("defers both builtin coding and stepwise to the interpreter (U6: coding now carries an optional-group)", () => { + // U6: builtin:coding gained the `browser-verification` optional-group on its + // pre-merge path — a branching, single-pass container the linear WorkflowStep + // runner cannot lower. Like stepwise, coding is now interpreter-deferred. + const codingErr = validateLinearity(BUILTIN_CODING_WORKFLOW_IR); + expect(codingErr).toBeInstanceOf(WorkflowCompileError); + expect(codingErr?.message).toContain(WORKFLOW_INTERPRETER_DEFERRED_SUFFIX); const stepwiseErr = validateLinearity(BUILTIN_STEPWISE_CODING_WORKFLOW_IR); expect(stepwiseErr).toBeInstanceOf(WorkflowCompileError); expect(stepwiseErr?.message).toContain(WORKFLOW_INTERPRETER_DEFERRED_SUFFIX); }); - it("compiles the builtin coding workflow without merge-region steps", () => { - const steps = compileWorkflowToSteps(BUILTIN_CODING_WORKFLOW_IR); - const mergeRegionNodeIds = BUILTIN_CODING_WORKFLOW_IR.nodes - .filter((node) => MERGE_REGION_NODE_KINDS.has(node.kind)) - .map((node) => node.id); - - expect(steps.map((step) => step.name)).toEqual([]); - expect(mergeRegionNodeIds).toEqual( - expect.arrayContaining([ - "merge-gate", - "merge-retry", - "merge-manual-hold", - "branch-group-member-integration", - "branch-group-promotion", - "merge-attempt", - "recovery-router", - ]), - ); - expect(steps.some((step) => mergeRegionNodeIds.includes(step.name))).toBe(false); + it("defers compiling the builtin coding workflow to the interpreter (U6)", () => { + // The browser-verification optional-group makes the graph non-linear, so + // compileWorkflowToSteps throws the interpreter-deferred error rather than + // producing a (previously empty) linear pre-merge step list. + expect(() => compileWorkflowToSteps(BUILTIN_CODING_WORKFLOW_IR)).toThrow(WorkflowCompileError); + expect(() => compileWorkflowToSteps(BUILTIN_CODING_WORKFLOW_IR)).toThrow(/interpreter \(deferred\)/i); }); it("compiles a workflow whose post-review merge region branches into primitives (FN-6035)", () => { diff --git a/packages/core/src/__tests__/workflow-ir-optional-group.test.ts b/packages/core/src/__tests__/workflow-ir-optional-group.test.ts new file mode 100644 index 0000000000..88806e3383 --- /dev/null +++ b/packages/core/src/__tests__/workflow-ir-optional-group.test.ts @@ -0,0 +1,154 @@ +import { describe, expect, it } from "vitest"; +import { parseWorkflowIr, serializeWorkflowIr } from "../workflow-ir.js"; +import type { WorkflowIrEdge, WorkflowIrNode, WorkflowIrV2 } from "../workflow-ir-types.js"; + +/* +FNXC:WorkflowOptionalGroup 2026-06-21-11:00: +U1 validation contract for the `optional-group` container node — the single-pass, +toggle-gated subgraph that replaces the declaration-based optional-steps model. +Mirrors the loop validation suite minus loop-specific exit config. +*/ + +const columns: WorkflowIrV2["columns"] = [{ id: "work", name: "Work", traits: [] }]; + +function groupTemplate(): { nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[] } { + return { + nodes: [ + { id: "verify", kind: "prompt", config: { prompt: "verify in browser" } }, + { id: "report", kind: "prompt", config: { prompt: "report" } }, + ], + edges: [{ from: "verify", to: "report" }], + }; +} + +function groupIr(config: Record = {}): WorkflowIrV2 { + return { + version: "v2", + name: "optional-group-test", + columns, + nodes: [ + { id: "start", kind: "start" }, + { + id: "browser-verification", + kind: "optional-group", + config: { + name: "Browser Verification", + defaultOn: false, + template: groupTemplate(), + ...config, + }, + }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "browser-verification" }, + { from: "browser-verification", to: "end" }, + ], + }; +} + +describe("optional-group validation", () => { + it("parses and round-trips a valid optional-group node", () => { + const parsed = parseWorkflowIr(groupIr()) as WorkflowIrV2; + const group = parsed.nodes.find((n) => n.id === "browser-verification"); + + expect(group?.kind).toBe("optional-group"); + expect(parseWorkflowIr(serializeWorkflowIr(parsed))).toEqual(parsed); + }); + + it("does not require defaultOn (defaults to off via the resolver)", () => { + expect(() => parseWorkflowIr(groupIr({ defaultOn: undefined }))).not.toThrow(); + }); + + it("rejects a non-boolean defaultOn", () => { + expect(() => parseWorkflowIr(groupIr({ defaultOn: "yes" as unknown as boolean }))).toThrow( + /defaultOn must be a boolean/, + ); + }); + + it("rejects an empty template", () => { + expect(() => parseWorkflowIr(groupIr({ template: { nodes: [], edges: [] } }))).toThrow(/non-empty/); + }); + + it("rejects a missing template", () => { + expect(() => parseWorkflowIr(groupIr({ template: undefined }))).toThrow( + /must declare a template/, + ); + }); + + it("rejects duplicate template node ids", () => { + const template = groupTemplate(); + template.nodes.push({ id: "verify", kind: "script" }); + expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/duplicate node ids/); + }); + + it("rejects template edges that leave the template", () => { + const template = groupTemplate(); + template.edges.push({ from: "report", to: "end" }); + expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/references a node outside/); + }); + + it("rejects rework edges inside the template (single-pass guarantee)", () => { + const template = groupTemplate(); + template.edges.push({ from: "report", to: "verify", kind: "rework" }); + 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({ + id: "nested", + kind: "optional-group", + config: { template: groupTemplate() }, + }); + template.edges.push({ from: "report", to: "nested" }); + expect(() => parseWorkflowIr(groupIr({ template }))).toThrow( + /nested loop\/foreach\/optional-group/, + ); + }); + + it("rejects more than one entry node", () => { + const template: ReturnType = { + nodes: [ + { id: "a", kind: "prompt", config: { prompt: "a" } }, + { id: "b", kind: "prompt", config: { prompt: "b" } }, + { id: "join", kind: "prompt", config: { prompt: "join" } }, + ], + // a and b both have no incoming edge → two entries. + edges: [ + { from: "a", to: "join" }, + { from: "b", to: "join" }, + ], + }; + expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/exactly one entry node/); + }); + + it("rejects a template node id colliding with a top-level node id", () => { + const template = groupTemplate(); + template.nodes[0] = { id: "start", kind: "prompt", config: { prompt: "collide" } }; + template.edges = [{ from: "start", to: "report" }]; + expect(() => parseWorkflowIr(groupIr({ template }))).toThrow(/collides with a top-level node id/); + }); + + it("leaves graphs without optional-group nodes byte-identical", () => { + const ir: WorkflowIrV2 = { + version: "v2", + name: "plain", + columns, + nodes: [ + { id: "start", kind: "start" }, + { id: "end", kind: "end" }, + ], + edges: [{ from: "start", to: "end" }], + }; + const parsed = parseWorkflowIr(ir); + expect(parseWorkflowIr(serializeWorkflowIr(parsed))).toEqual(parsed); + }); +}); diff --git a/packages/core/src/__tests__/workflow-ir.test.ts b/packages/core/src/__tests__/workflow-ir.test.ts index 291e681e31..6285828f7f 100644 --- a/packages/core/src/__tests__/workflow-ir.test.ts +++ b/packages/core/src/__tests__/workflow-ir.test.ts @@ -155,7 +155,12 @@ describe("parseWorkflowIr — v2 columns & placement", () => { }); }); -describe("parseWorkflowIr — optionalSteps", () => { +// FNXC:WorkflowOptionalGroup 2026-06-21-18:00: +// The legacy `optionalSteps` declaration field is retired. A legacy persisted +// `optionalSteps` key on an old v2 row is now TOLERATED — no longer validated or +// required — so old rows still parse as v2 (optional steps are graph-native +// `optional-group` nodes now). +describe("parseWorkflowIr — legacy optionalSteps tolerated", () => { const columns = DEFAULT_WORKFLOW_COLUMN_IDS.map((id) => ({ id, name: id, traits: [] })); const base = (): WorkflowIrV2 => v2( columns, @@ -166,27 +171,25 @@ describe("parseWorkflowIr — optionalSteps", () => { [{ from: "start", to: "end" }], ); - it("parses and serializes optionalSteps deterministically", () => { - const ir: WorkflowIrV2 = { + it("parses a legacy v2 row carrying an optionalSteps key without throwing", () => { + const ir = { ...base(), + // Legacy declaration shapes — including ones the old validator rejected — + // are now ignored, not validated. optionalSteps: [ { templateId: "browser-verification" }, - { templateId: "plugin:example:step", defaultOn: true }, + { defaultOn: "yes" }, + "nope", ], - }; + } as unknown as WorkflowIr; + expect(() => parseWorkflowIr(ir)).not.toThrow(); const parsed = parseWorkflowIr(ir); - expect(parsed).toEqual(ir); + expect(parsed.version).toBe("v2"); + // The key passes through untouched (round-trips through serialize/parse). expect(JSON.parse(serializeWorkflowIr(parsed))).toEqual(ir); }); - it("rejects malformed optionalSteps", () => { - expect(() => parseWorkflowIr({ ...base(), optionalSteps: "nope" } as unknown as WorkflowIr)).toThrow(WorkflowIrError); - expect(() => parseWorkflowIr({ ...base(), optionalSteps: [{}] } as unknown as WorkflowIr)).toThrow(/non-empty templateId/); - expect(() => parseWorkflowIr({ ...base(), optionalSteps: [{ templateId: "" }] } as unknown as WorkflowIr)).toThrow(/non-empty templateId/); - expect(() => parseWorkflowIr({ ...base(), optionalSteps: [{ templateId: "browser-verification", defaultOn: "yes" }] } as unknown as WorkflowIr)).toThrow(/defaultOn must be a boolean/); - }); - it("upgrades v1 graphs without optionalSteps", () => { const parsed = parseWorkflowIr({ version: "v1", @@ -196,7 +199,7 @@ describe("parseWorkflowIr — optionalSteps", () => { }); expect(parsed.version).toBe("v2"); if (parsed.version !== "v2") throw new Error("expected v2"); - expect(parsed.optionalSteps).toBeUndefined(); + expect((parsed as { optionalSteps?: unknown }).optionalSteps).toBeUndefined(); }); }); diff --git a/packages/core/src/__tests__/workflow-optional-steps.test.ts b/packages/core/src/__tests__/workflow-optional-steps.test.ts index d3ea6ba91e..5f101815d0 100644 --- a/packages/core/src/__tests__/workflow-optional-steps.test.ts +++ b/packages/core/src/__tests__/workflow-optional-steps.test.ts @@ -1,8 +1,16 @@ import { describe, expect, it } from "vitest"; import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; -import { resolveWorkflowOptionalSteps } from "../workflow-optional-steps.js"; -import type { WorkflowIr, WorkflowIrV2 } from "../workflow-ir-types.js"; +import { + resolveDefaultOnOptionalGroupIds, + resolveWorkflowOptionalSteps, +} from "../workflow-optional-steps.js"; +import type { + WorkflowIr, + WorkflowIrNode, + WorkflowIrV2, + WorkflowOptionalGroupConfig, +} from "../workflow-ir-types.js"; const v1: WorkflowIr = { version: "v1", @@ -14,105 +22,125 @@ const v1: WorkflowIr = { edges: [{ from: "start", to: "end" }], }; -function v2(optionalSteps?: WorkflowIrV2["optionalSteps"]): WorkflowIrV2 { +/** Build an optional-group node with a trivial single-prompt template. */ +function optionalGroupNode( + id: string, + config: Partial, +): WorkflowIrNode { + return { + id, + kind: "optional-group", + column: "todo", + config: { + ...config, + template: config.template ?? { + nodes: [{ id: `${id}-inner`, kind: "prompt" }], + edges: [], + }, + } satisfies WorkflowOptionalGroupConfig, + }; +} + +function v2(extraNodes: WorkflowIrNode[] = []): WorkflowIrV2 { return { version: "v2", name: "optional", columns: [{ id: "todo", name: "Todo", traits: [] }], nodes: [ { id: "start", kind: "start", column: "todo" }, + ...extraNodes, { id: "end", kind: "end", column: "todo" }, ], edges: [{ from: "start", to: "end" }], - optionalSteps, }; } -describe("resolveWorkflowOptionalSteps", () => { - it("resolves the builtin coding browser verification optional step", () => { - expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)).toEqual([ +describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { + it("resolves two optional-group nodes with names + defaultOn from node config", () => { + const ir = v2([ + optionalGroupNode("og-browser", { name: "Browser Verification", defaultOn: false }), + optionalGroupNode("og-security", { name: "Security Audit", defaultOn: true }), + ]); + + expect(resolveWorkflowOptionalSteps(ir)).toEqual([ { - templateId: "browser-verification", + templateId: "og-browser", name: "Browser Verification", - description: "Verify web application functionality using browser automation", - icon: "globe", + description: "", phase: "pre-merge", defaultOn: false, }, + { + templateId: "og-security", + name: "Security Audit", + description: "", + phase: "pre-merge", + defaultOn: true, + }, ]); }); - it("resolves the builtin stepwise-coding browser verification optional step", () => { - expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual([ - { - templateId: "browser-verification", - name: "Browser Verification", - description: "Verify web application functionality using browser automation", - icon: "globe", - phase: "pre-merge", - defaultOn: false, - }, - ]); + it("falls back to the node id when the group config omits a name", () => { + const ir = v2([optionalGroupNode("og-unnamed", { defaultOn: true })]); + const [resolved] = resolveWorkflowOptionalSteps(ir); + expect(resolved.templateId).toBe("og-unnamed"); + expect(resolved.name).toBe("og-unnamed"); + expect(resolved.defaultOn).toBe(true); }); - it("places a single workflow-step seam node between steps and review in stepwise", () => { - const ir = BUILTIN_STEPWISE_CODING_WORKFLOW_IR; - if (ir.version !== "v2") throw new Error("expected v2"); - const seamNodes = ir.nodes.filter( - (n) => n.kind === "prompt" && n.config?.seam === "workflow-step", - ); - expect(seamNodes).toHaveLength(1); - // success path: steps -> workflow-step -> review - expect(ir.edges).toEqual( - expect.arrayContaining([ - expect.objectContaining({ from: "steps", to: "workflow-step", condition: "success" }), - expect.objectContaining({ from: "workflow-step", to: "review", condition: "success" }), - ]), - ); - }); - - it("skips unknown template ids", () => { - expect( - resolveWorkflowOptionalSteps(v2([ - { templateId: "missing" }, - { templateId: "browser-verification" }, - ])), - ).toHaveLength(1); - }); - - it("returns an empty array for v1 and v2 workflows without optional steps", () => { + it("returns an empty array for v1 and v2 workflows without optional-group nodes", () => { expect(resolveWorkflowOptionalSteps(v1)).toEqual([]); expect(resolveWorkflowOptionalSteps(v2())).toEqual([]); }); - it("preserves declaration order and resolves plugin templates", () => { - const result = resolveWorkflowOptionalSteps( - v2([ - { templateId: "plugin:demo:first", defaultOn: true }, - { templateId: "browser-verification" }, - ]), - [ - { - id: "plugin:demo:first", - name: "Plugin First", - description: "Plugin optional verification", - prompt: "Run plugin verification", - category: "Quality", - icon: "plug", - phase: "post-merge", - }, - ], - ); - - expect(result.map((step) => step.templateId)).toEqual([ - "plugin:demo:first", - "browser-verification", + it("ignores a malformed (config-less) optional-group node without crashing", () => { + // A stale/partial optional-group node must not throw; it resolves to a + // defaultOn:false entry keyed by its id rather than breaking workflow loading. + const ir = v2([{ id: "og-bare", kind: "optional-group", column: "todo" }]); + expect(resolveWorkflowOptionalSteps(ir)).toEqual([ + { + templateId: "og-bare", + name: "og-bare", + description: "", + phase: "pre-merge", + defaultOn: false, + }, ]); - expect(result[0]).toMatchObject({ - name: "Plugin First", - icon: "plug", - phase: "post-merge", - defaultOn: true, - }); + }); + + it("resolves the built-in coding/stepwise browser-verification optional-group (U6)", () => { + // U6 migrated both built-ins: `browser-verification` is now an optional-group + // node (default OFF), so the resolver advertises exactly one toggle entry per + // built-in, keyed by the group node id `browser-verification`. + const expected = [ + { + templateId: "browser-verification", + name: "Browser Verification", + description: "", + phase: "pre-merge" as const, + defaultOn: false, + }, + ]; + expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)).toEqual(expected); + expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(expected); + }); +}); + +describe("resolveDefaultOnOptionalGroupIds (task-creation seeding)", () => { + it("returns exactly the defaultOn:true group ids", () => { + const ir = v2([ + optionalGroupNode("og-off", { defaultOn: false }), + optionalGroupNode("og-on-a", { defaultOn: true }), + optionalGroupNode("og-on-b", { defaultOn: true }), + ]); + expect(resolveDefaultOnOptionalGroupIds(ir)).toEqual(["og-on-a", "og-on-b"]); + }); + + it("seeds an empty set when no optional-group has defaultOn (or none exist)", () => { + expect(resolveDefaultOnOptionalGroupIds(v2())).toEqual([]); + expect( + resolveDefaultOnOptionalGroupIds(v2([optionalGroupNode("og-off", { defaultOn: false })])), + ).toEqual([]); + expect(resolveDefaultOnOptionalGroupIds(v1)).toEqual([]); }); }); diff --git a/packages/core/src/__tests__/workflow-selection-store.test.ts b/packages/core/src/__tests__/workflow-selection-store.test.ts index 53d19e029f..898875d8da 100644 --- a/packages/core/src/__tests__/workflow-selection-store.test.ts +++ b/packages/core/src/__tests__/workflow-selection-store.test.ts @@ -176,6 +176,130 @@ describe("TaskStore workflow selection (U3)", () => { expect(store.getTaskWorkflowSelection(task.id)?.workflowId).toBe(wf.id); }); + // FNXC:WorkflowOptionalGroup 2026-06-21-14:30: a new task seeds + // `enabledWorkflowSteps` with exactly the defaultOn:true optional-group ids of + // its selected workflow (U3, R3), alongside the compiled workflow step ids. + describe("optional-group defaultOn seeding (U3/R3)", () => { + /** v2 workflow whose success path threads through two optional-group nodes. */ + function optionalGroupIr(): WorkflowIr { + const groupTemplate = (id: string) => ({ + nodes: [{ id: `${id}-inner`, kind: "prompt" as const, config: { prompt: "x" } }], + edges: [], + }); + return { + version: "v2", + name: "og-wf", + columns: [{ id: "todo", name: "Todo", traits: [] }], + nodes: [ + { id: "start", kind: "start", column: "todo" }, + { + id: "og-on", + kind: "optional-group", + column: "todo", + config: { name: "On Group", defaultOn: true, template: groupTemplate("og-on") }, + }, + { + id: "og-off", + kind: "optional-group", + column: "todo", + config: { name: "Off Group", defaultOn: false, template: groupTemplate("og-off") }, + }, + { id: "end", kind: "end", column: "todo" }, + ], + edges: [ + { from: "start", to: "og-on", condition: "success" }, + { from: "og-on", to: "og-off", condition: "success" }, + { from: "og-off", to: "end", condition: "success" }, + ], + }; + } + + it("seeds the defaultOn:true group id at creation from the default workflow", async () => { + const wf = await store.createWorkflowDefinition({ name: "OG Default", ir: optionalGroupIr() }); + await store.setDefaultWorkflowId(wf.id); + + const task = await store.createTask({ description: "seeded" }); + const detail = await store.getTask(task.id); + expect(detail.enabledWorkflowSteps).toContain("og-on"); + expect(detail.enabledWorkflowSteps).not.toContain("og-off"); + }); + + it("seeds an empty set when the workflow has no optional groups", async () => { + const wf = await store.createWorkflowDefinition({ name: "No OG", ir: linearIr() }); + await store.setDefaultWorkflowId(wf.id); + + const task = await store.createTask({ description: "no groups" }); + const detail = await store.getTask(task.id); + expect(detail.enabledWorkflowSteps ?? []).not.toContain("og-on"); + }); + + it("a stale optional-group id in enabledWorkflowSteps does not crash resolution", async () => { + // Group since removed from the workflow: the toggle resolver ignores the + // stale id rather than throwing, keeping create/edit surfaces alive. + const task = await store.createTask({ + description: "stale", + enabledWorkflowSteps: ["og-removed"], + }); + const detail = await store.getTask(task.id); + expect(detail.enabledWorkflowSteps).toContain("og-removed"); + }); + + // FNXC:WorkflowOptionalGroup 2026-06-21-16:30: code-review P1 regression. A + // built-in optional-group id deliberately equals a WORKFLOW_STEP_TEMPLATES id + // (the browser-verification migration). Enabling it on a task must keep the + // RAW group node id in enabledWorkflowSteps — not a materialized WorkflowStep + // row id — or the executor's `enabledWorkflowSteps.includes(node.id)` check + // silently bypasses the group. (The og-on/og-off ids above don't collide, so + // only a colliding id exercises the remap bug.) + function collidingGroupIr(): WorkflowIr { + return { + version: "v2", + name: "bv-wf", + columns: [{ id: "todo", name: "Todo", traits: [] }], + nodes: [ + { id: "start", kind: "start", column: "todo" }, + { + id: "browser-verification", + kind: "optional-group", + column: "todo", + config: { + name: "Browser Verification", + defaultOn: false, + template: { nodes: [{ id: "bv-inner", kind: "prompt", config: { prompt: "verify" } }], edges: [] }, + }, + }, + { id: "end", kind: "end", column: "todo" }, + ], + edges: [ + { from: "start", to: "browser-verification", condition: "success" }, + { from: "browser-verification", to: "end", condition: "success" }, + ], + }; + } + + it("keeps a built-in-colliding optional-group id unremapped on create-with-enable", async () => { + const wf = await store.createWorkflowDefinition({ name: "BV", ir: collidingGroupIr() }); + await store.setDefaultWorkflowId(wf.id); + + const task = await store.createTask({ + description: "enable bv", + enabledWorkflowSteps: ["browser-verification"], + }); + const detail = await store.getTask(task.id); + expect(detail.enabledWorkflowSteps).toContain("browser-verification"); + }); + + it("keeps a built-in-colliding optional-group id unremapped on update/toggle", async () => { + const wf = await store.createWorkflowDefinition({ name: "BV", ir: collidingGroupIr() }); + await store.setDefaultWorkflowId(wf.id); + + const task = await store.createTask({ description: "toggle bv" }); + await store.updateTask(task.id, { enabledWorkflowSteps: ["browser-verification"] }); + const detail = await store.getTask(task.id); + expect(detail.enabledWorkflowSteps).toContain("browser-verification"); + }); + }); + it("explicit enabledWorkflowSteps overrides the project default", async () => { const wf = await store.createWorkflowDefinition({ name: "Default", ir: linearIr() }); await store.setDefaultWorkflowId(wf.id); diff --git a/packages/core/src/builtin-browser-verification-group.ts b/packages/core/src/builtin-browser-verification-group.ts new file mode 100644 index 0000000000..72cab875e2 --- /dev/null +++ b/packages/core/src/builtin-browser-verification-group.ts @@ -0,0 +1,79 @@ +import type { WorkflowIrNode } from "./workflow-ir-types.js"; +import { WORKFLOW_STEP_TEMPLATES } from "./types.js"; + +/* +FNXC:WorkflowOptionalGroup 2026-06-21-15:10: +Both the built-in coding and stepwise-coding workflows express the optional +`browser-verification` step as an `optional-group` container node on the pre-merge +path (default OFF), REPLACING the legacy `optionalSteps: [{ templateId: +"browser-verification" }]` declaration + the hidden `workflow-step` seam node (U6). +Enabled (task's `enabledWorkflowSteps` includes the group id) → the browser- +verification step runs ONCE pre-merge between implementation and review. Disabled → +the group passes through (byte-inert), exactly preserving the prior runtime behavior +where the step only ran when toggled on. + +The group node id `browser-verification` is the STABLE per-task enable key (KTD-2): +keeping it identical to the prior `optionalSteps` templateId preserves any persisted +`enabledWorkflowSteps` entry. The inner template node carries a DISTINCT id +(`browser-verification-step`) because a template node id may not collide with the +group/top-level node id (U1 validation). + +The inner node mirrors the dashboard's `stepTemplateToNode` projection of the +canonical `browser-verification` WORKFLOW_STEP_TEMPLATE: a `prompt` node carrying the +template's prompt, `toolMode` (coding), and `gateMode` (advisory default). Sourcing +prompt/toolMode from the catalog keeps the built-in byte-identical to the template a +human would insert from the palette (KTD-5). +*/ + +function resolveBrowserVerificationTemplate() { + const tpl = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "browser-verification"); + if (!tpl) { + throw new Error("browser-verification WORKFLOW_STEP_TEMPLATE is missing"); + } + return tpl; +} + +const BROWSER_VERIFICATION_TEMPLATE = resolveBrowserVerificationTemplate(); + +/** Stable per-task enable key + group node id (preserved from the prior templateId). */ +export const BROWSER_VERIFICATION_GROUP_ID = "browser-verification"; + +/** Inner template node id — distinct from the group id (template-node-id collision rule, U1). */ +export const BROWSER_VERIFICATION_STEP_NODE_ID = "browser-verification-step"; + +/** + * Build the `browser-verification` optional-group node placed on a workflow's + * pre-merge path. `column` matches where the legacy `workflow-step` seam sat + * (in-progress) so the editor renders the group in the implementation column. + * + * Mirrors `stepTemplateToNode(browser-verification)`: a single `prompt` node whose + * config carries the catalog prompt + `toolMode: "coding"` + `gateMode: "advisory"`. + */ +export function browserVerificationOptionalGroupNode(column: string): WorkflowIrNode { + const tpl = BROWSER_VERIFICATION_TEMPLATE; + return { + id: BROWSER_VERIFICATION_GROUP_ID, + kind: "optional-group", + column, + config: { + name: tpl.name, + defaultOn: false, + template: { + nodes: [ + { + id: BROWSER_VERIFICATION_STEP_NODE_ID, + kind: "prompt", + config: { + name: tpl.name, + description: tpl.description, + prompt: tpl.prompt ?? "", + toolMode: tpl.toolMode === "coding" ? "coding" : "readonly", + gateMode: tpl.gateMode ?? "advisory", + }, + }, + ], + edges: [], + }, + }, + }; +} diff --git a/packages/core/src/builtin-coding-workflow-ir.ts b/packages/core/src/builtin-coding-workflow-ir.ts index f663d6b7fd..b506ffec31 100644 --- a/packages/core/src/builtin-coding-workflow-ir.ts +++ b/packages/core/src/builtin-coding-workflow-ir.ts @@ -2,6 +2,7 @@ import type { WorkflowIr } from "./workflow-ir-types.js"; import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; +import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; /** * The built-in default workflow as a v2 IR. Its six columns have ids that are @@ -20,9 +21,16 @@ import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; * * The lifecycle seam nodes are placed in their columns. Planning is explicit so * the built-in workflow owns the specification phase rather than relying on - * triage code that runs outside the graph; workflow-step keeps the legacy - * pre-merge quality gate between implementation and review; execute/review/ - * merge keep the same observable pipeline and failure routing. + * triage code that runs outside the graph; execute/review/merge keep the same + * observable pipeline and failure routing. + * + * FNXC:WorkflowOptionalGroup 2026-06-21-15:10: + * The pre-merge optional `browser-verification` step is now an `optional-group` + * container node (default OFF) sitting on the success path between execute and + * review — REPLACING the legacy `workflow-step` seam node + the execution-inert + * `optionalSteps: [{ templateId: "browser-verification" }]` declaration (U6). A + * task whose `enabledWorkflowSteps` includes the group id runs browser + * verification pre-merge exactly as before; a task with it off bypasses it. */ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { version: "v2", @@ -65,12 +73,8 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { column: "in-progress", config: { ...builtinPromptConfig("execute", "Execute"), maxRetries: 2 }, }, - { - id: "workflow-step", - kind: "prompt", - column: "in-progress", - config: builtinPromptConfig("workflow-step", "Pre-merge workflow steps"), - }, + // Pre-merge optional browser-verification (optional-group, default OFF). + browserVerificationOptionalGroupNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, @@ -94,10 +98,10 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { edges: [ { from: "start", to: "planning" }, { from: "planning", to: "execute", condition: "success" }, - { from: "execute", to: "workflow-step", condition: "success" }, - { from: "workflow-step", to: "review", condition: "success" }, - { from: "workflow-step", to: "end", condition: "outcome:remediation-scheduled" }, - { from: "workflow-step", to: "end", condition: "outcome:deferred-paused" }, + // execute → browser-verification (optional-group) → review. When the group is + // disabled it passes through with outcome=success and routes straight to review. + { from: "execute", to: "browser-verification", condition: "success" }, + { from: "browser-verification", to: "review", condition: "success" }, { from: "review", to: "merge-gate", condition: "success" }, { from: "merge-gate", to: "branch-group-member-integration", condition: "outcome:auto-on" }, { from: "merge-gate", to: "merge-manual-hold", condition: "outcome:auto-off" }, @@ -113,14 +117,13 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { { from: "recovery-router", to: "merge-attempt", condition: "outcome:wake-merge", kind: "rework" }, { from: "planning", to: "end", condition: "failure" }, { from: "execute", to: "end", condition: "failure" }, - { from: "workflow-step", to: "end", condition: "failure" }, + { from: "browser-verification", to: "end", condition: "failure" }, { from: "review", to: "end", condition: "failure" }, { from: "merge-attempt", to: "end", condition: "failure" }, ], // Workflow-settings (U1, R4): declare the full moved-key catalog with defaults // byte-equal to today's DEFAULT_PROJECT_SETTINGS literals. Inert until U3. settings: BUILTIN_WORKFLOW_SETTINGS, - optionalSteps: [{ templateId: "browser-verification" }], }; export const BUILTIN_CODING_WORKFLOW_IR = parseWorkflowIr(RAW_BUILTIN_CODING_WORKFLOW_IR); diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index 348125fd5a..39f14f740e 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -2,6 +2,7 @@ import type { WorkflowIr } from "./workflow-ir-types.js"; import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; +import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; /** * The built-in **stepwise** coding workflow (KTD-9) — the demonstration of step @@ -123,16 +124,16 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { }, // KTD-5: rework exhaustion escalates to a manual hold (a human releases it). { id: "rework-hold", kind: "hold", column: "in-progress", config: { release: "manual" } }, - // FNXC:WorkflowOptionalSteps 2026-06-21-00:00: - // The stepwise workflow must actually run a task's enabled optional steps (e.g. - // browser verification), so it needs the same pre-merge workflow-step seam the - // coding workflow has — declaring the optional step without this node would be a - // dead toggle. Pre-merge workflow-step seam (parity with builtin-coding-workflow-ir): - // the ONLY node that makes the graph invoke `runWorkflowSteps`, so a per-task - // `enabledWorkflowSteps` (e.g. the optional browser-verification step declared - // below) actually executes. Runs ONCE after the foreach completes, between - // implementation and review — not per step-instance. - { id: "workflow-step", kind: "prompt", column: "in-progress", config: builtinPromptConfig("workflow-step", "Pre-merge workflow steps") }, + // FNXC:WorkflowOptionalGroup 2026-06-21-15:10: + // Pre-merge optional browser-verification as an `optional-group` container + // (default OFF), parity with builtin-coding-workflow-ir (U6). It REPLACES the + // prior `workflow-step` seam + `optionalSteps` declaration. R-3 run-once + // guarantee: the group sits on the post-foreach success path (steps → here → + // review), so when enabled the browser-verification step runs EXACTLY ONCE + // after every step-instance completes — never per step-instance — and when + // disabled the group passes through inert. Both the normal foreach-success path + // and the rework-exhausted manual-release path flow through this node. + browserVerificationOptionalGroupNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, @@ -163,17 +164,16 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { { from: "parse", to: "steps", condition: "outcome:no-steps" }, { from: "parse", to: "end", condition: "failure" }, { from: "parse", to: "end", condition: "outcome:parse-error" }, - // Implementation complete → pre-merge workflow-step seam → review. Both the - // normal foreach-success path and the rework-exhausted manual-release path flow - // through the seam so enabled workflow steps run regardless of route. - { from: "steps", to: "workflow-step", condition: "success" }, - // KTD-5: bounded rework exhaustion → manual hold; release re-enters the seam. + // Implementation complete → pre-merge browser-verification optional-group → + // review. Both the normal foreach-success path and the rework-exhausted + // manual-release path flow through the group so an enabled task runs the step + // ONCE after the foreach (R-3), and a disabled task passes through to review. + { from: "steps", to: "browser-verification", condition: "success" }, + // KTD-5: bounded rework exhaustion → manual hold; release re-enters the group. { from: "steps", to: "rework-hold", condition: "outcome:rework-exhausted" }, - { from: "rework-hold", to: "workflow-step", condition: "success" }, - { from: "workflow-step", to: "review", condition: "success" }, - { from: "workflow-step", to: "end", condition: "outcome:remediation-scheduled" }, - { from: "workflow-step", to: "end", condition: "outcome:deferred-paused" }, - { from: "workflow-step", to: "end", condition: "failure" }, + { from: "rework-hold", to: "browser-verification", condition: "success" }, + { from: "browser-verification", to: "review", condition: "success" }, + { from: "browser-verification", to: "end", condition: "failure" }, { from: "steps", to: "end", condition: "failure" }, { from: "review", to: "merge-gate", condition: "success" }, { from: "review", to: "end", condition: "failure" }, @@ -193,9 +193,6 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { ], // Workflow-settings (U1, R4): same moved-key catalog as the default builtin. settings: BUILTIN_WORKFLOW_SETTINGS, - // Optional browser-verification step, parity with builtin-coding-workflow-ir. - // Default OFF; runnable because the workflow-step seam node above is present. - optionalSteps: [{ templateId: "browser-verification" }], }; export const BUILTIN_STEPWISE_CODING_WORKFLOW_IR = parseWorkflowIr( diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 1b660ecd21..5f30fb1ad2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -86,13 +86,13 @@ export type { WorkflowForeachConfig, WorkflowLoopConfig, WorkflowLoopExitCondition, + WorkflowOptionalGroupConfig, WorkflowIrArtifact, WorkflowFieldDefinition, WorkflowFieldType, WorkflowFieldOption, WorkflowFieldRender, // Workflow-settings (U1): typed setting declaration IR types. - WorkflowOptionalStep, WorkflowSettingDefinition, WorkflowSettingType, WorkflowSettingOption, @@ -119,7 +119,10 @@ export type { } from "./column-agent-resolver.js"; export { BUILTIN_CODING_WORKFLOW_IR } from "./builtin-coding-workflow-ir.js"; export { BUILTIN_MARKETING_WORKFLOW_IR } from "./builtin-marketing-workflow-ir.js"; -export { resolveWorkflowOptionalSteps } from "./workflow-optional-steps.js"; +export { + resolveWorkflowOptionalSteps, + resolveDefaultOnOptionalGroupIds, +} from "./workflow-optional-steps.js"; export type { ResolvedWorkflowOptionalStep } from "./workflow-optional-steps.js"; export { applyPromptOverridesToIr, diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 0bcaf02b8d..350bd90b63 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -86,6 +86,7 @@ import type { WorkflowNodeLayout, } from "./workflow-definition-types.js"; import { compileWorkflowToSteps, isInterpreterDeferredWorkflowCompileError } from "./workflow-compiler.js"; +import { resolveDefaultOnOptionalGroupIds, resolveAllOptionalGroupIds } from "./workflow-optional-steps.js"; import { BUILTIN_WORKFLOWS, getBuiltinWorkflow, @@ -4286,7 +4287,26 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} }); } - private async resolveEnabledWorkflowSteps(stepIds?: string[]): Promise { + /* + FNXC:WorkflowOptionalGroup 2026-06-21-16:30: + `optionalGroupIds` are the optional-group node ids of the task's workflow. They are executor toggle keys (matched by node id in `enabledWorkflowSteps`), NOT legacy `WorkflowStep` template ids. A built-in group id can deliberately collide with a `WORKFLOW_STEP_TEMPLATES` id (e.g. "browser-verification"); without this pass-through the colliding id is materialized into a step row whose id differs from the group node id, so the executor's `enabledWorkflowSteps.includes(node.id)` check fails and an enabled group is silently bypassed (P1 from code review). Editor-authored group ids never collide (they come from `newNodeId()`), so they already passed through; this guards the built-in collision. + */ + /** Optional-group node ids for a workflow (its `enabledWorkflowSteps` toggle + * keys). Falls back to the project default workflow when `workflowId` is + * nullish; empty for missing/fragment workflows. Used to keep group ids out of + * the legacy step-template materialization in {@link resolveEnabledWorkflowSteps}. */ + 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)); + } + + private async resolveEnabledWorkflowSteps( + stepIds?: string[], + optionalGroupIds?: Set, + ): Promise { if (!stepIds?.length) return undefined; const resolved: string[] = []; @@ -4304,7 +4324,10 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} continue; } - const template = this.getBuiltInWorkflowTemplate(stepId); + // 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; @@ -4444,7 +4467,10 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} // Determine enabledWorkflowSteps: explicit input takes precedence, otherwise auto-apply default-on steps let resolvedWorkflowSteps: string[] | undefined = input.enabledWorkflowSteps?.length - ? await this.resolveEnabledWorkflowSteps(input.enabledWorkflowSteps) + ? await this.resolveEnabledWorkflowSteps( + input.enabledWorkflowSteps, + await this.optionalGroupIdSet(input.workflowId), + ) : undefined; // When a project default workflow is configured, new tasks inherit it @@ -4639,7 +4665,10 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} const title = input.title?.trim() || undefined; let resolvedWorkflowSteps: string[] | undefined = input.enabledWorkflowSteps?.length - ? await this.resolveEnabledWorkflowSteps(input.enabledWorkflowSteps) + ? await this.resolveEnabledWorkflowSteps( + input.enabledWorkflowSteps, + await this.optionalGroupIdSet(input.workflowId), + ) : undefined; let pendingWorkflowSelection: { workflowId: string; stepIds: string[] } | undefined; @@ -8712,7 +8741,14 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} task.nextRecoveryAt = updates.nextRecoveryAt; } if (updates.enabledWorkflowSteps !== undefined) { - task.enabledWorkflowSteps = await this.resolveEnabledWorkflowSteps(updates.enabledWorkflowSteps); + // Pass the task's own workflow optional-group ids through untouched so a + // toggled built-in group id (e.g. "browser-verification") is not remapped + // to a materialized step row the executor never matches (code-review P1). + const taskWorkflowId = this.getTaskWorkflowSelection(task.id)?.workflowId; + task.enabledWorkflowSteps = await this.resolveEnabledWorkflowSteps( + updates.enabledWorkflowSteps, + await this.optionalGroupIdSet(taskWorkflowId), + ); } if (updates.noCommitsExpected === null) { task.noCommitsExpected = undefined; @@ -15952,11 +15988,17 @@ ${stepsSection}`; if (isBuiltinWorkflowId(workflowId) && isInterpreterDeferredWorkflowCompileError(err)) return undefined; throw err; } + // FNXC:WorkflowOptionalGroup 2026-06-21-14:20: seed `enabledWorkflowSteps` + // with the ids of `optional-group` nodes whose `defaultOn` is true, mirroring + // the prior `optionalStep.defaultOn ?? false` precedence (U3, R3). These group + // ids are NOT WorkflowStep rows — they are toggle keys the executor reads at + // the optional-group seam — so they ride alongside the compiled step ids. + const defaultGroupIds = resolveDefaultOnOptionalGroupIds(def.ir); if (isBuiltinWorkflowId(workflowId) && inputs.length === 0) { - return { workflowId, stepIds: [] }; + return { workflowId, stepIds: defaultGroupIds }; } const stepIds = await this.materializeWorkflowSteps(workflowId, inputs); - return { workflowId, stepIds }; + return { workflowId, stepIds: [...stepIds, ...defaultGroupIds] }; } /** Resolve an EXPLICITLY requested workflow id (U6/R3/KTD-4) into materialized @@ -15977,11 +16019,15 @@ ${stepsSection}`; try { inputs = compileWorkflowToSteps(def.ir); } catch (err) { - if (isBuiltinWorkflowId(workflowId) && isInterpreterDeferredWorkflowCompileError(err)) return { workflowId, stepIds: [] }; + if (isBuiltinWorkflowId(workflowId) && isInterpreterDeferredWorkflowCompileError(err)) + return { workflowId, stepIds: resolveDefaultOnOptionalGroupIds(def.ir) }; throw err; } + // FNXC:WorkflowOptionalGroup 2026-06-21-14:20: same defaultOn-group seeding as + // the default-workflow path, for an explicitly requested create-time workflow. + const defaultGroupIds = resolveDefaultOnOptionalGroupIds(def.ir); const stepIds = await this.materializeWorkflowSteps(workflowId, inputs); - return { workflowId, stepIds }; + return { workflowId, stepIds: [...stepIds, ...defaultGroupIds] }; } /** diff --git a/packages/core/src/workflow-ir-types.ts b/packages/core/src/workflow-ir-types.ts index f2d5c90e1d..2bc146afaa 100644 --- a/packages/core/src/workflow-ir-types.ts +++ b/packages/core/src/workflow-ir-types.ts @@ -23,6 +23,7 @@ export type WorkflowIrNodeKind = | "join" | "foreach" | "loop" + | "optional-group" | "step-review" | "parse-steps" | "code" @@ -164,6 +165,26 @@ export interface WorkflowLoopConfig { }; } +/* +FNXC:WorkflowOptionalGroup 2026-06-21-11:00: +An `optional-group` node is a container (mirroring `foreach`/`loop`) whose `template` subgraph the executor runs ONCE when the group is enabled for the task and passes through (skips) when disabled. +Enable state reuses the per-task `enabledWorkflowSteps` facet keyed by the group node id, seeded from `defaultOn` at task creation — this replaces the execution-inert declaration-based optional-steps model (`WorkflowOptionalStep`/`optionalSteps`). +Single pass only: no iteration, no rework budget. Rework edges are forbidden inside the template so the single-pass guarantee is unambiguous (validated in `validateOptionalGroup`). +*/ +/** Config for an `optional-group` container node. `defaultOn` seeds the per-task + * enable set at creation; the `template` is the subgraph run once when enabled. + * Unlike `foreach`/`loop`, there is no iteration or rework — a single pass. */ +export interface WorkflowOptionalGroupConfig { + /** Workflow-author default for whether new tasks enable this group. */ + defaultOn?: boolean; + /** Display name for the group (editor + per-task toggle surfaces). */ + name?: string; + template: { + nodes: WorkflowIrNode[]; + edges: WorkflowIrEdge[]; + }; +} + /** Step-inversion (KTD-12): a workflow-declared task document. Artifacts ride the * existing task-documents machinery; `step-source` artifacts feed `parse-steps`. */ export interface WorkflowIrArtifact { @@ -311,13 +332,10 @@ export interface WorkflowIrV1 { edges: WorkflowIrEdge[]; } -/** Workflow-declared optional step backed by a workflow-step template. - * Execution-inert: consumed by create/edit UI to seed per-task - * `enabledWorkflowSteps`, never by the graph executor. Absent on legacy graphs. */ -export interface WorkflowOptionalStep { - templateId: string; - defaultOn?: boolean; -} +/* +FNXC:WorkflowOptionalGroup 2026-06-21-18:00: +Retired the legacy declaration-based optional-steps model. The `WorkflowOptionalStep` interface and the `WorkflowIrV2.optionalSteps` field are removed — optional steps are now graph-native `optional-group` NODES (see `WorkflowOptionalGroupConfig` above), resolved by `resolveWorkflowOptionalSteps`. A legacy persisted `optionalSteps` key on an old v2 row is TOLERATED at parse (ignored, not validated) so old rows still load as v2. +*/ /** A v2 workflow IR graph: v1 plus workflow-defined columns and node placement. * Step-inversion adds optional `artifacts` (KTD-12) and `fields` (KTD-13) @@ -333,9 +351,6 @@ export interface WorkflowIrV2 { /** Workflow-settings (U1, R1): typed setting declarations. Additive; absent on * legacy graphs. Values persist per-`(workflowId, projectId)` (U2), not here. */ settings?: WorkflowSettingDefinition[]; - /** Optional workflow-step templates tasks may independently enable/disable via - * `enabledWorkflowSteps`. Execution-inert; the graph executor ignores this facet. */ - optionalSteps?: WorkflowOptionalStep[]; } /** Either IR version. v1 graphs upgrade to v2 on parse (see parseWorkflowIr). */ diff --git a/packages/core/src/workflow-ir.ts b/packages/core/src/workflow-ir.ts index 0a052a87a9..75d96e8968 100644 --- a/packages/core/src/workflow-ir.ts +++ b/packages/core/src/workflow-ir.ts @@ -9,11 +9,11 @@ import type { WorkflowHoldRelease, WorkflowForeachConfig, WorkflowLoopConfig, + WorkflowOptionalGroupConfig, WorkflowFieldDefinition, WorkflowFieldType, WorkflowSettingDefinition, WorkflowSettingType, - WorkflowOptionalStep, } from "./workflow-ir-types.js"; import { getWorkflowExtensionRegistry } from "./workflow-extension-registry.js"; import type { WorkflowExtensionConfigField } from "./workflow-extension-types.js"; @@ -602,6 +602,120 @@ function validateLoop( } } +/* +FNXC:WorkflowOptionalGroup 2026-06-21-11:00: +Validate an `optional-group` container template, mirroring `validateLoop` minus the loop's exit/iteration config. +The template runs once when enabled, so rework edges (and any cycles) are forbidden inside, single entry/exit is required, and nested foreach/loop groups are rejected — keeping the single-pass guarantee unambiguous. +`defaultOn` must be boolean when present; `name` must be a string when present. +*/ +function validateOptionalGroup( + node: WorkflowIrNode, + topLevelNodeIds: Set, + columnIds: Set, +): void { + const cfg = node.config as Partial | undefined; + const template = cfg?.template; + if ( + !cfg || + !template || + !Array.isArray(template.nodes) || + !Array.isArray(template.edges) + ) { + throw new WorkflowIrError( + `optional-group node '${node.id}' must declare a template with nodes and edges arrays`, + ); + } + if (template.nodes.length === 0) { + throw new WorkflowIrError(`optional-group node '${node.id}' template must be non-empty`); + } + if (cfg.defaultOn !== undefined && typeof cfg.defaultOn !== "boolean") { + throw new WorkflowIrError(`optional-group node '${node.id}' defaultOn must be a boolean`); + } + if (cfg.name !== undefined && typeof cfg.name !== "string") { + throw new WorkflowIrError(`optional-group node '${node.id}' name must be a string`); + } + + const templateNodes = template.nodes; + const templateIds = new Set(templateNodes.map((n) => n.id)); + if (templateIds.size !== templateNodes.length) { + throw new WorkflowIrError(`optional-group node '${node.id}' template has duplicate node ids`); + } + for (const inner of templateNodes) { + if (inner.kind === "loop" || inner.kind === "foreach" || inner.kind === "optional-group") { + throw new WorkflowIrError( + `optional-group node '${node.id}' template may not contain nested loop/foreach/optional-group ('${inner.id}')`, + ); + } + if (isStepExecuteNode(inner)) { + throw new WorkflowIrError( + `step-execute seam node '${inner.id}' is only legal inside a foreach template`, + ); + } + if (inner.column !== undefined && !columnIds.has(inner.column)) { + throw new WorkflowIrError( + `Workflow node '${inner.id}' references undefined column '${inner.column}'`, + ); + } + } + for (const edge of template.edges) { + const fromInside = templateIds.has(edge.from); + const toInside = templateIds.has(edge.to); + if (!fromInside || !toInside) { + throw new WorkflowIrError( + `optional-group node '${node.id}' template edge '${edge.from}' -> '${edge.to}' references a node outside the template`, + ); + } + 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(); + const outgoingCount = new Map(); + for (const edge of template.edges) { + incoming.set(edge.to, (incoming.get(edge.to) ?? 0) + 1); + outgoingCount.set(edge.from, (outgoingCount.get(edge.from) ?? 0) + 1); + } + const entries = templateNodes.filter((n) => (incoming.get(n.id) ?? 0) === 0); + const exits = templateNodes.filter((n) => (outgoingCount.get(n.id) ?? 0) === 0); + if (entries.length !== 1) { + throw new WorkflowIrError( + `optional-group node '${node.id}' template must have exactly one entry node (found ${entries.length})`, + ); + } + if (exits.length !== 1) { + throw new WorkflowIrError( + `optional-group node '${node.id}' template must have exactly one exit node (found ${exits.length})`, + ); + } + + const templateById = new Map(templateNodes.map((n) => [n.id, n])); + const templateOutgoing = buildOutgoing(template.edges); + validateNoIllegalCycles(templateNodes, templateOutgoing); + validateParallelism(templateNodes, templateOutgoing, templateById); + validateStepReviewRouting(templateNodes, templateOutgoing, templateById, false); + + for (const id of templateIds) { + if (topLevelNodeIds.has(id)) { + throw new WorkflowIrError( + `optional-group node '${node.id}' template node id '${id}' collides with a top-level node id`, + ); + } + } +} + /** step-execute seam nodes are legal ONLY inside a foreach template (KTD-4): * reject any at the top level. (Inside-split-branch rejection is handled by * SEAM_FORBIDDEN_IN_BRANCH within validateParallelism.) */ @@ -1071,29 +1185,6 @@ function validateSettings(settings: WorkflowSettingDefinition[] | undefined): vo } } -function validateOptionalSteps(optionalSteps: WorkflowOptionalStep[] | undefined): void { - if (optionalSteps === undefined) return; - if (!Array.isArray(optionalSteps)) { - throw new WorkflowIrError("Workflow IR optionalSteps must be an array"); - } - for (const optionalStep of optionalSteps) { - if (!optionalStep || typeof optionalStep !== "object" || Array.isArray(optionalStep)) { - throw new WorkflowIrError("Workflow optional step must be an object"); - } - if (typeof optionalStep.templateId !== "string" || optionalStep.templateId === "") { - throw new WorkflowIrError("Workflow optional step must have a non-empty templateId"); - } - if ( - optionalStep.defaultOn !== undefined && - typeof optionalStep.defaultOn !== "boolean" - ) { - throw new WorkflowIrError( - `Workflow optional step '${optionalStep.templateId}' defaultOn must be a boolean`, - ); - } - } -} - function validateColumns(ir: WorkflowIrV2): void { if (!Array.isArray(ir.columns)) { throw new WorkflowIrError("Workflow IR v2 columns must be an array"); @@ -1265,6 +1356,7 @@ function validateV2(ir: WorkflowIrV2): void { for (const node of ir.nodes) { if (node.kind === "foreach") validateForeach(node, topLevelIds, columnIds); if (node.kind === "loop") validateLoop(node, topLevelIds, columnIds); + if (node.kind === "optional-group") validateOptionalGroup(node, topLevelIds, columnIds); } validateStepReviewRouting(ir.nodes, outgoing, nodesById, false); validateParseStepsNodes(ir); @@ -1272,7 +1364,11 @@ function validateV2(ir: WorkflowIrV2): void { validateNotifyNodes(ir.nodes); validateFields(ir.fields); validateSettings(ir.settings); - validateOptionalSteps(ir.optionalSteps); + // FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + // The legacy `optionalSteps` declaration field is retired (optional steps are + // now graph-native `optional-group` nodes). A legacy persisted `optionalSteps` + // key on an old v2 row is TOLERATED — no longer validated/required — so old + // rows still parse as v2. // Rework edges are legal intra-template (foreach, KTD-5) and — since U6 // generalized the bounded-rework mechanism to the top-level walk — for a @@ -1381,12 +1477,20 @@ export function downgradeIrToV1IfPure(ir: WorkflowIr): WorkflowIr { } // Step-inversion declarations (artifacts/fields), workflow settings (U1), and - // optional workflow-step declarations are v2-only features. + // any legacy persisted optional-step declarations are v2-only features. + // 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). 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) || - (ir.optionalSteps && ir.optionalSteps.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 6b1c714ee2..f3fea182cd 100644 --- a/packages/core/src/workflow-optional-steps.ts +++ b/packages/core/src/workflow-optional-steps.ts @@ -1,5 +1,9 @@ -import type { WorkflowIr } from "./workflow-ir-types.js"; -import { WORKFLOW_STEP_TEMPLATES, type WorkflowStepTemplate } from "./types.js"; +import type { + WorkflowIr, + WorkflowIrNode, + WorkflowOptionalGroupConfig, +} from "./workflow-ir-types.js"; +import type { WorkflowStepTemplate } from "./types.js"; export interface ResolvedWorkflowOptionalStep { templateId: string; @@ -10,37 +14,74 @@ export interface ResolvedWorkflowOptionalStep { defaultOn: boolean; } +/* +FNXC:WorkflowOptionalGroup 2026-06-21-14:05: +Re-pointed the per-task optional-step toggle SOURCE from the execution-inert `ir.optionalSteps` declaration to v2 `optional-group` NODES (one resolved entry per group). The legacy `WorkflowOptionalStep` type + `optionalSteps` IR field are now REMOVED (FNXC:WorkflowOptionalGroup 2026-06-21-18:00); a legacy persisted `optionalSteps` key on an old v2 row is tolerated/ignored at parse. +KEYING: the resolved entry is keyed by the group node `id`. The output field is still named `templateId` (not renamed) so the four consuming UI surfaces — inline quick-create card, New Task modal/TaskForm, task-detail Workflow tab, and the optional-steps dropdown — keep reading the same shape unchanged; they now toggle group ids into `enabledWorkflowSteps` instead of template ids. Renaming/recreating a group resets per-task state, identical to the prior `templateId` keying. +Display metadata: `name` comes from `config.name` (falling back to the node id), `defaultOn` from `config.defaultOn ?? false`. The group node carries no description/icon/phase, so `description` is "" and `phase` defaults to "pre-merge" — keeping every field the consumers read populated and non-blank. +*/ + +function isOptionalGroupNode( + node: WorkflowIrNode, +): node is WorkflowIrNode & { config: WorkflowOptionalGroupConfig } { + return node.kind === "optional-group"; +} + /** - * Resolve workflow-declared optional step template ids into display metadata. + * Resolve a workflow's `optional-group` nodes into per-task toggle display + * metadata. Each enabled group's node id is what a task stores in + * `enabledWorkflowSteps`; this resolver advertises which groups a task may + * toggle plus their seed default. * - * The declaration is intentionally execution-inert: it only advertises which - * template-backed workflow steps a task may toggle into `enabledWorkflowSteps`. - * Unknown template ids are skipped so stale/custom declarations never render - * blank UI rows or break workflow loading. + * 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 `[]`. 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 + * unused. */ export function resolveWorkflowOptionalSteps( ir: WorkflowIr, - pluginTemplates: WorkflowStepTemplate[] = [], + _pluginTemplates: WorkflowStepTemplate[] = [], ): ResolvedWorkflowOptionalStep[] { - if (ir.version !== "v2" || !ir.optionalSteps?.length) return []; - - const templates = new Map(); - for (const template of [...WORKFLOW_STEP_TEMPLATES, ...pluginTemplates]) { - templates.set(template.id, template); - } + if (ir.version !== "v2" || !Array.isArray(ir.nodes)) return []; const resolved: ResolvedWorkflowOptionalStep[] = []; - for (const optionalStep of ir.optionalSteps) { - const template = templates.get(optionalStep.templateId); - if (!template) continue; + for (const node of ir.nodes) { + if (!isOptionalGroupNode(node)) continue; + const config = (node.config ?? {}) as Partial; resolved.push({ - templateId: optionalStep.templateId, - name: template.name, - description: template.description, - icon: template.icon, - phase: template.phase ?? "pre-merge", - defaultOn: optionalStep.defaultOn ?? template.defaultOn ?? false, + // Keyed by the group node id (documented above); field name preserved. + templateId: node.id, + name: typeof config.name === "string" && config.name.trim() ? config.name : node.id, + description: "", + phase: "pre-merge", + defaultOn: config.defaultOn === true, }); } return resolved; } + +/** + * Ids of `optional-group` nodes whose effective `defaultOn` is true. Used to + * seed a new task's `enabledWorkflowSteps` at creation, mirroring the prior + * `optionalStep.defaultOn ?? false` precedence (U3, R3). Defensive: non-v2 + * graphs and graphs without optional groups yield `[]`. + */ +export function resolveDefaultOnOptionalGroupIds(ir: WorkflowIr): string[] { + return resolveWorkflowOptionalSteps(ir) + .filter((step) => step.defaultOn) + .map((step) => step.templateId); +} + +/* +FNXC:WorkflowOptionalGroup 2026-06-21-16:30: +Every optional-group node id in a workflow, regardless of `defaultOn`. These ids are executor toggle keys (the per-task `enabledWorkflowSteps` set), NOT legacy `WorkflowStep` template ids. A built-in group id can deliberately equal a `WORKFLOW_STEP_TEMPLATES` id (e.g. "browser-verification"), so the store must pass these through `resolveEnabledWorkflowSteps` untouched instead of materializing them into a step row whose id the executor would never match. +*/ +export function resolveAllOptionalGroupIds(ir: WorkflowIr): string[] { + return resolveWorkflowOptionalSteps(ir).map((step) => step.templateId); +} diff --git a/packages/dashboard/app/components/CustomModelDropdown.css b/packages/dashboard/app/components/CustomModelDropdown.css index e943c7f93d..933c7d29f6 100644 --- a/packages/dashboard/app/components/CustomModelDropdown.css +++ b/packages/dashboard/app/components/CustomModelDropdown.css @@ -67,8 +67,8 @@ border: 1px solid var(--border); border-radius: var(--radius); box-shadow: var(--shadow); - /* Must sit above floating dashboard panels. */ - z-index: 1200; + /* Must sit above floating dashboard panels and the shared floating-window stack (10100+). */ + z-index: 11000; max-height: 320px; display: flex; flex-direction: column; diff --git a/packages/dashboard/app/components/DevServerView.css b/packages/dashboard/app/components/DevServerView.css index 5bb722fc38..437f83087e 100644 --- a/packages/dashboard/app/components/DevServerView.css +++ b/packages/dashboard/app/components/DevServerView.css @@ -582,6 +582,65 @@ exactly when the surrounding chrome is gone. margin: 0; } +.devserver-preview-modal-launcher { + align-items: stretch; +} + +.devserver-preview-modal-launcher__copy { + display: flex; + align-items: center; + gap: var(--space-sm); + min-width: 0; +} + +.devserver-preview-modal-launcher__copy .devserver-preview-url-badge { + max-width: none; +} + +.devserver-preview-modal-launcher__description { + margin: 0; + color: var(--text-muted); + line-height: 1.5; +} + +.devserver-preview-modal-overlay { + align-items: center; + padding: var(--space-xl); +} + +.devserver-preview-modal { + width: min(calc(var(--space-2xl) * 28), calc(100vw - var(--space-xl) * 2)); + max-height: calc(100vh - var(--space-xl) * 2); +} + +.devserver-preview-modal__titlebar { + display: flex; + align-items: center; + justify-content: space-between; + gap: var(--space-md); + padding: var(--space-md); + border-bottom: 1px solid var(--border); +} + +.devserver-preview-modal__titlebar h2 { + margin: 0; + font-size: 1rem; +} + +.devserver-preview-modal__body { + display: flex; + flex: 1; + flex-direction: column; + min-height: 0; + overflow: hidden; +} + +.devserver-preview-modal__body .devserver-preview-container { + flex: 1; + min-height: min(60vh, calc(var(--space-2xl) * 14)); + max-height: none; +} + /* Legacy selector compatibility for static CSS tests */ .dev-server-preview-fallback { border: 1px solid color-mix(in srgb, var(--color-warning) 40%, transparent); @@ -655,7 +714,8 @@ exactly when the surrounding chrome is gone. max-width: none; } - .devserver-preview-header { + .devserver-preview-header, + .devserver-preview-modal-launcher__copy { flex-wrap: wrap; } @@ -703,6 +763,20 @@ exactly when the surrounding chrome is gone. max-height: calc(var(--space-2xl) * 3); } + .devserver-preview-modal-overlay { + align-items: stretch; + padding: var(--space-md); + } + + .devserver-preview-modal { + width: 100%; + max-height: calc(100vh - var(--space-md) * 2); + } + + .devserver-preview-modal__body .devserver-preview-container { + min-height: calc(var(--space-2xl) * 7); + } + .dev-server-config { max-height: min(48vh, calc(var(--space-2xl) * 13)); } @@ -852,7 +926,8 @@ vertically (.dev-server-view overflow-y:auto), so each panel just needs to be fu flex-direction: column; } - .devserver-preview-panel { + .devserver-preview-panel, + .devserver-preview-modal-launcher { grid-column: auto; grid-row: auto; } @@ -862,10 +937,25 @@ vertically (.dev-server-view overflow-y:auto), so each panel just needs to be fu max-width: none; } - .devserver-preview-header { + .devserver-preview-header, + .devserver-preview-modal-launcher__copy { flex-wrap: wrap; } + .devserver-preview-modal-overlay { + align-items: stretch; + padding: var(--space-md); + } + + .devserver-preview-modal { + width: min(calc(var(--space-2xl) * 20), calc(100vw - var(--space-md) * 2)); + max-height: calc(100vh - var(--space-md) * 2); + } + + .devserver-preview-modal__body .devserver-preview-container { + min-height: calc(var(--space-2xl) * 7); + } + .devserver-preview-url-badge { order: 2; flex: 1 1 100%; @@ -915,8 +1005,8 @@ vertically (.dev-server-view overflow-y:auto), so each panel just needs to be fu } .dev-server-logs, - .devserver-preview-container, - .devserver-preview-iframe { + .devserver-preview-panel .devserver-preview-container, + .devserver-preview-panel .devserver-preview-iframe { min-height: calc(var(--space-2xl) * 4 + var(--space-md)); max-height: none; } diff --git a/packages/dashboard/app/components/DevServerView.tsx b/packages/dashboard/app/components/DevServerView.tsx index e88bf63bf8..b3b88febad 100644 --- a/packages/dashboard/app/components/DevServerView.tsx +++ b/packages/dashboard/app/components/DevServerView.tsx @@ -1,13 +1,15 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import type { RefObject } from "react"; import type { TFunction } from "i18next"; import { useTranslation } from "react-i18next"; -import { AlertTriangle, ExternalLink, Eye, Loader2, Monitor, Play, RefreshCw, RotateCw, ShieldAlert, Square } from "lucide-react"; +import { AlertTriangle, ExternalLink, Eye, Loader2, Monitor, Play, RefreshCw, RotateCw, ShieldAlert, Square, X } from "lucide-react"; import type { Task, TaskDetail } from "@fusion/core"; import "./DevServerView.css"; import type { DetectedDevServerCommand } from "../api"; import { useDevServer } from "../hooks/useDevServer"; import { useDevServerLogs } from "../hooks/useDevServerLogs"; import { usePreviewEmbed } from "../hooks/usePreviewEmbed"; +import { useOverlayDismiss } from "../hooks/useOverlayDismiss"; import type { ToastType } from "../hooks/useToast"; import { DevServerLogViewer } from "./DevServerLogViewer"; import { PreviewIframe } from "./PreviewIframe"; @@ -37,6 +39,85 @@ function getStatusBadgeConfig(t: TFunction<"app">): Record<"stopped" | "starting }; } + +const NARROW_RIGHT_DOCK_PREVIEW_THRESHOLD = 480; + +function isTrueMobileViewport(): boolean { + if (typeof window === "undefined" || typeof window.matchMedia !== "function") { + return false; + } + + return window.matchMedia("(max-width: 768px)").matches; +} + +function getDirectRightDockBodyHost(element: HTMLElement): HTMLElement | null { + if (element.closest(".right-dock-expand-modal__body")) { + return null; + } + + const parent = element.parentElement; + if (!parent?.classList.contains("right-dock__body")) { + return null; + } + + return parent; +} + +function readHostInlineSize(host: HTMLElement): number { + if (host.clientWidth > 0) { + return host.clientWidth; + } + + const rect = host.getBoundingClientRect(); + return rect.width; +} + +function shouldUseNarrowRightDockPreviewMode(root: HTMLElement | null): boolean { + if (!root || isTrueMobileViewport()) { + return false; + } + + const host = getDirectRightDockBodyHost(root); + if (!host) { + return false; + } + + return readHostInlineSize(host) <= NARROW_RIGHT_DOCK_PREVIEW_THRESHOLD; +} + +function useNarrowRightDockPreviewMode(rootRef: RefObject): boolean { + const [isNarrowRightDockPreviewMode, setIsNarrowRightDockPreviewMode] = useState(false); + + useEffect(() => { + const root = rootRef.current; + if (!root) { + setIsNarrowRightDockPreviewMode(false); + return; + } + + const host = getDirectRightDockBodyHost(root); + const updateMode = () => setIsNarrowRightDockPreviewMode(shouldUseNarrowRightDockPreviewMode(root)); + + updateMode(); + + if (!host || typeof ResizeObserver === "undefined") { + window.addEventListener("resize", updateMode); + return () => window.removeEventListener("resize", updateMode); + } + + const observer = new ResizeObserver(updateMode); + observer.observe(host); + window.addEventListener("resize", updateMode); + + return () => { + observer.disconnect(); + window.removeEventListener("resize", updateMode); + }; + }, [rootRef]); + + return isNarrowRightDockPreviewMode; +} + let devServerViewWasPreviouslyInactive = false; function normalizeError(error: unknown): string { @@ -142,6 +223,14 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps const effectivePreviewUrl = previewUrl; const selectedSource = session?.config?.cwd ?? null; + const rootRef = useRef(null); + const isNarrowRightDockPreviewMode = useNarrowRightDockPreviewMode(rootRef); + + /* + FNXC:DevServer 2026-06-23-00:00: + The Dev Server preview must escape into a modal when the direct right-dock host is very narrow so preview chrome does not crowd logs and configuration in the same dock column. + The 480px threshold catches the dock's compact range before preview chrome becomes unusable while preserving full-page, true mobile viewport, and expanded pop-out inline previews. + */ const [showCandidates, setShowCandidates] = useState(true); const [commandInput, setCommandInput] = useState(""); const [previewInput, setPreviewInput] = useState(""); @@ -170,6 +259,9 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps }, [executingTasks, selectedTaskId]); const [previewMode, setPreviewMode] = useState("embedded"); + const [isPreviewModalOpen, setIsPreviewModalOpen] = useState(false); + const previewModalLauncherRef = useRef(null); + const previewModalRef = useRef(null); const previewEmbedUrl = previewMode === "embedded" ? effectivePreviewUrl : null; const { @@ -271,6 +363,60 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps setPreviewInput(effectivePreviewUrl ?? ""); }, [effectivePreviewUrl]); + const closePreviewModal = useCallback(() => { + setIsPreviewModalOpen(false); + window.requestAnimationFrame(() => previewModalLauncherRef.current?.focus()); + }, []); + const previewModalOverlayDismissProps = useOverlayDismiss(closePreviewModal); + + useEffect(() => { + if (!isPreviewModalOpen) { + return; + } + + previewModalRef.current?.focus(); + + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === "Escape") { + closePreviewModal(); + return; + } + + if (event.key !== "Tab") { + return; + } + + const focusableElements = Array.from( + previewModalRef.current?.querySelectorAll( + 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])', + ) ?? [], + ).filter((element) => !element.hasAttribute("disabled") && element.getAttribute("aria-hidden") !== "true"); + + const firstElement = focusableElements[0]; + const lastElement = focusableElements.at(-1); + if (!firstElement || !lastElement) { + return; + } + + if (event.shiftKey && document.activeElement === firstElement) { + event.preventDefault(); + lastElement.focus(); + } else if (!event.shiftKey && document.activeElement === lastElement) { + event.preventDefault(); + firstElement.focus(); + } + }; + + document.addEventListener("keydown", handleKeyDown); + return () => document.removeEventListener("keydown", handleKeyDown); + }, [closePreviewModal, isPreviewModalOpen]); + + useEffect(() => { + if (!isNarrowRightDockPreviewMode && isPreviewModalOpen) { + setIsPreviewModalOpen(false); + } + }, [isNarrowRightDockPreviewMode, isPreviewModalOpen]); + const handleOpenInNewTab = useCallback(() => { if (!effectivePreviewUrl) { return; @@ -399,8 +545,136 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps const stopDisabled = status === "stopped" || actionInFlight !== null; const restartDisabled = status === "stopped" || status === "starting" || actionInFlight !== null; + const renderPreviewContent = () => ( + <> +
+
+ + {t("devserver.preview", "Preview")} +
+ + {isManualPreviewOverride ? t("devserver.manual", "Manual") : t("devserver.auto", "Auto")} + {effectivePreviewUrl ? ` · ${effectivePreviewUrl}` : t("devserver.notAvailable", " · Not available")} + +
+ + + +
+
+ +
+ {!effectivePreviewUrl && !isRunning && ( +

{t("devserver.startDevServer", "Start a dev server to see a live preview here.")}

+ )} + + {!effectivePreviewUrl && isRunning && ( +

{t("devserver.noPreviewDetected", "No preview URL detected. Start the dev server or set a manual URL to preview your app.")}

+ )} + + {effectivePreviewUrl && previewMode === "external" && ( +
+

{t("devserver.embeddedPreviewDisabled", "Embedded preview is disabled. Open your app in a separate browser tab.")}

+ +
+ )} + + {effectivePreviewUrl && previewMode === "embedded" && showFallback && isBlocked && ( +
+ {embedStatus === "error" + ?
+ )} + + {effectivePreviewUrl && previewMode === "embedded" && !showFallback && ( + + )} +
+ + ); + return ( -
+
{/* FNXC:DevServer 2026-06-22-01:00: Migrated to the shared ViewHeader for cross-view consistency. The status badge sits next to the title inside the actions slot (wrapped in .dev-server-header-title so the existing mobile flex-wrap rule still applies), and the Start/Stop/Restart controls follow in .dev-server-header-actions. ViewHeader supplies the standard view padding; the view body must not repeat the top padding. @@ -641,126 +915,75 @@ export function DevServerView({ addToast, projectId, tasks }: DevServerViewProps
-
-
-
- - {t("devserver.preview", "Preview")} + {isNarrowRightDockPreviewMode ? ( +
+
+
+ + {t("devserver.preview", "Preview")} +
+ + {effectivePreviewUrl ? effectivePreviewUrl : t("devserver.notAvailable", "Not available")} +
- + {effectivePreviewUrl + ? t("devserver.previewModalLauncherDescription", "Open the live preview in a modal so logs and configuration stay usable in this narrow dock.") + : t("devserver.previewModalLauncherUnavailable", "Start the dev server or set a preview URL to open the preview modal.")} +

+ - - -
-
+ {t("devserver.openPreview", "Open preview")} + +
+ ) : ( +
+ {renderPreviewContent()} +
+ )} -
- {!effectivePreviewUrl && !isRunning && ( -

{t("devserver.startDevServer", "Start a dev server to see a live preview here.")}

- )} - - {!effectivePreviewUrl && isRunning && ( -

{t("devserver.noPreviewDetected", "No preview URL detected. Start the dev server or set a manual URL to preview your app.")}

- )} - - {effectivePreviewUrl && previewMode === "external" && ( -
-

{t("devserver.embeddedPreviewDisabled", "Embedded preview is disabled. Open your app in a separate browser tab.")}

+ {isNarrowRightDockPreviewMode && isPreviewModalOpen && ( +
+
+
+

{t("devserver.preview", "Preview")}

- )} - - {effectivePreviewUrl && previewMode === "embedded" && showFallback && isBlocked && ( -
- {embedStatus === "error" - ?
- + )}
); } diff --git a/packages/dashboard/app/components/SettingsModal.css b/packages/dashboard/app/components/SettingsModal.css index b5d7af8c75..8ab9bd73e1 100644 --- a/packages/dashboard/app/components/SettingsModal.css +++ b/packages/dashboard/app/components/SettingsModal.css @@ -696,13 +696,16 @@ The embedded title reads like other embedded-view titles (Planning modal-header- } } +/* +FNXC:SettingsMobile 2026-06-23-09:02: +Settings section headings should preserve hierarchy through spacing and type only. Avoid per-heading divider borders so mobile and desktop shared Settings sections keep the lighter scrollbar-focused chrome contract. +*/ .settings-section-heading { font-size: 14px; font-weight: 600; padding: var(--space-lg) 0 var(--space-md); margin: 0 0 var(--space-md); color: var(--text); - border-bottom: 1px solid var(--border); } /* First heading inside the section drops top padding to remove a redundant diff --git a/packages/dashboard/app/components/SettingsModal.tsx b/packages/dashboard/app/components/SettingsModal.tsx index ee7f96253a..28d6d01ed3 100644 --- a/packages/dashboard/app/components/SettingsModal.tsx +++ b/packages/dashboard/app/components/SettingsModal.tsx @@ -1851,17 +1851,22 @@ export function SettingsModal({ return next; }); try { + /* + FNXC:Notifications 2026-06-23-08:49: + Settings notification tests must send the current unsaved ntfy form values for every ntfy test affordance. Users validate the exact topic/server/token they just typed before saving, so message/room test requests carry the same request-scoped config as the general ntfy test. + */ + const currentNtfyConfig = { + ntfyEnabled: form.ntfyEnabled, + ntfyTopic: form.ntfyTopic, + ...(form.ntfyBaseUrl?.trim() ? { ntfyBaseUrl: form.ntfyBaseUrl.trim() } : {}), + ...(form.ntfyAccessToken?.trim() ? { ntfyAccessToken: form.ntfyAccessToken.trim() } : {}), + }; const config = providerId === "ntfy" - ? { - ntfyEnabled: form.ntfyEnabled, - ntfyTopic: form.ntfyTopic, - ...(form.ntfyBaseUrl?.trim() ? { ntfyBaseUrl: form.ntfyBaseUrl.trim() } : {}), - ...(form.ntfyAccessToken?.trim() ? { ntfyAccessToken: form.ntfyAccessToken.trim() } : {}), - } + ? currentNtfyConfig : providerId === "ntfy-message" - ? { messageEventType: "message:agent-to-user" } + ? { ...currentNtfyConfig, messageEventType: "message:agent-to-user" } : providerId === "ntfy-room" - ? { messageEventType: "message:room" } + ? { ...currentNtfyConfig, messageEventType: "message:room" } : { webhookUrl: form.webhookUrl, webhookFormat: form.webhookFormat || "generic", diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.css b/packages/dashboard/app/components/WorkflowNodeEditor.css index a25ebb0685..50ce4480c7 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.css +++ b/packages/dashboard/app/components/WorkflowNodeEditor.css @@ -1171,6 +1171,80 @@ Built-in workflow prompts need visible override state and a reset action without 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 4fd00531b4..81cf94c863 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.tsx +++ b/packages/dashboard/app/components/WorkflowNodeEditor.tsx @@ -16,8 +16,8 @@ import { } from "@xyflow/react"; import { createPortal } from "react-dom"; import { useTranslation } from "react-i18next"; -import { X, Plus, Trash2, Save, MessageSquare, Terminal, Shield, GitMerge, Loader2, HelpCircle, PauseCircle, Split, Merge, Repeat, ClipboardCheck, ListChecks, Code2, Bell, LayoutGrid, Workflow, Download, Upload, ChevronDown, ChevronRight, ChevronLeft, Library, Sparkles, Maximize2, Minimize2 } from "lucide-react"; -import type { WorkflowDefinition, WorkflowIrColumn, TraitViolation, WorkflowStepTemplate, WorkflowOptionalStep } from "@fusion/core"; +import { X, Plus, Trash2, Save, MessageSquare, Terminal, Shield, GitMerge, Loader2, HelpCircle, PauseCircle, Split, Merge, Repeat, ToggleRight, ClipboardCheck, ListChecks, Code2, Bell, LayoutGrid, Workflow, Download, Upload, ChevronDown, ChevronRight, ChevronLeft, Library, Sparkles, Maximize2, Minimize2 } from "lucide-react"; +import type { WorkflowDefinition, WorkflowIrColumn, TraitViolation, WorkflowStepTemplate, WorkflowIrNodeKind } from "@fusion/core"; import { getErrorMessage } from "@fusion/core"; import { fetchWorkflows, @@ -56,6 +56,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, @@ -63,11 +64,11 @@ import { emptyWorkflowLayout, copyIrWithFreshIds, insertFragment, + optionalGroupFragmentIr, fragmentSeamConflicts, columnsOf, fieldsOf, settingsOf, - optionalStepsOf, columnsToBandNodes, reconcileNodeColumns, strictColumnForY, @@ -91,7 +92,6 @@ import { fetchTraits, fetchStepParsers, type TraitCatalogEntry } from "../api"; import { WorkflowColumnPanel } from "./WorkflowColumnPanel"; import { WorkflowFieldsPanel } from "./WorkflowFieldsPanel"; import { WorkflowSettingsPanel } from "./WorkflowSettingsPanel"; -import { WorkflowOptionalStepsPanel } from "./WorkflowOptionalStepsPanel"; import type { WorkflowFieldDefinition, WorkflowSettingDefinition } from "../api"; import { CustomModelDropdown } from "./CustomModelDropdown"; import { MobileWorkflowGraphView } from "./MobileWorkflowGraphView"; @@ -103,7 +103,9 @@ import { } from "./workflow-mobile-graph"; type ExecutorKind = "model" | "agent" | "skill" | "cli" | "cli-agent"; -type MobileWorkflowPanel = "graph" | "add" | "settings" | "fields" | "optional-steps" | "columns" | "actions"; +// FNXC:WorkflowOptionalGroup 2026-06-21-18:00: dropped the "optional-steps" mobile +// panel — the declaration authoring surface is retired (optional-group nodes now). +type MobileWorkflowPanel = "graph" | "add" | "settings" | "fields" | "columns" | "actions"; function builtinSeamPrompt(config: Record | undefined): string { const seam = typeof config?.seam === "string" ? config.seam : ""; @@ -190,7 +192,6 @@ function serializeGraph( columns: WorkflowIrColumn[], fields: WorkflowFieldDefinition[], settings: WorkflowSettingDefinition[], - optionalSteps: WorkflowOptionalStep[], ): string { const { ir, layout } = flowToIr( name, @@ -199,7 +200,6 @@ function serializeGraph( columns.length ? columns : undefined, fields.length ? fields : undefined, settings.length ? settings : undefined, - optionalSteps.length ? optionalSteps : undefined, ); return JSON.stringify({ name, description, ir, layout }); } @@ -269,6 +269,8 @@ const PALETTE: Array<{ kind: WorkflowEditorNodeKind; label: string; icon: typeof // Step-inversion (KTD-3/4/12/15). { kind: "foreach", label: "For-each step", icon: Repeat, presetConfig: { source: "task-steps" } }, { kind: "loop", label: "Loop", icon: Repeat, presetConfig: { maxIterations: 3, exitWhen: { type: "output-contains", value: "DONE" } } }, + // FNXC:WorkflowOptionalGroup 2026-06-21-11:30: An optional-group container holds a template subgraph run once when the task enables it (per-task `enabledWorkflowSteps`, seeded from `defaultOn`) and skipped otherwise. + { kind: "optional-group", label: "Optional group", icon: ToggleRight, presetConfig: { defaultOn: false } }, { kind: "step-review", label: "Step review", icon: ClipboardCheck, presetConfig: { type: "code" } }, { kind: "parse-steps", label: "Parse steps", icon: ListChecks, presetConfig: { artifact: "PROMPT.md", parser: "step-headings" } }, { kind: "code", label: "Code", icon: Code2, presetConfig: { source: "" } }, @@ -320,6 +322,7 @@ const USER_NODE_KINDS: ReadonlySet = new Set([]); - const [optionalSteps, setOptionalSteps] = useState([]); + /* FNXC:WorkflowOptionalGroup 2026-06-21-18:00: + The legacy optional-step DECLARATION authoring state/panel is removed. Optional + steps are graph-native `optional-group` nodes authored through the canvas; the + editor no longer carries a separate `optionalSteps` declaration array. */ // FNXC:WorkflowEditor 2026-06-21-20:06: // Built-in workflow graph structure remains read-only, but prompt/gate node prompts need a separate per-project override state so editing prompts does not mark structural graph edits dirty or use the read-only workflow PATCH authority. const [promptOverrides, setPromptOverrides] = useState(null); @@ -834,7 +840,6 @@ function InnerEditor({ const columnsCollapsedStorageKey = "fusion:wf-sidebar-columns-collapsed"; const fieldsCollapsedStorageKey = "fusion:wf-sidebar-fields-collapsed"; const settingsCollapsedStorageKey = "fusion:wf-sidebar-settings-collapsed"; - const optionalStepsCollapsedStorageKey = "fusion:wf-sidebar-optional-steps-collapsed"; const [sidebarCollapsed, setSidebarCollapsed] = useState(() => { try { return localStorage.getItem(sidebarCollapsedStorageKey) === "1"; @@ -863,13 +868,6 @@ function InnerEditor({ return false; } }); - const [optionalStepsCollapsed, setOptionalStepsCollapsed] = useState(() => { - try { - return localStorage.getItem(optionalStepsCollapsedStorageKey) === "1"; - } catch { - return false; - } - }); useEffect(() => { try { localStorage.setItem(sidebarCollapsedStorageKey, sidebarCollapsed ? "1" : "0"); @@ -898,13 +896,6 @@ function InnerEditor({ // localStorage unavailable (private mode / SSR): non-fatal. } }, [settingsCollapsed]); - useEffect(() => { - try { - localStorage.setItem(optionalStepsCollapsedStorageKey, optionalStepsCollapsed ? "1" : "0"); - } catch { - // localStorage unavailable (private mode / SSR): non-fatal. - } - }, [optionalStepsCollapsed]); // React Flow instance for programmatic viewport control (auto-layout on load). const { setViewport } = useReactFlow(); // Wrapper around so keyboard deletion can return focus to the @@ -1092,10 +1083,10 @@ function InnerEditor({ if (isBuiltin) return false; if (!activeWorkflow || loadedSnapshotRef.current === null) return false; return ( - serializeGraph(name, description, nodes, edges, columns, fields, settings, optionalSteps) !== + serializeGraph(name, description, nodes, edges, columns, fields, settings) !== loadedSnapshotRef.current ); - }, [isBuiltin, activeWorkflow, name, description, nodes, edges, columns, fields, settings, optionalSteps]); + }, [isBuiltin, activeWorkflow, name, description, nodes, edges, columns, fields, settings]); const loadWorkflows = useCallback(async () => { setLoading(true); @@ -1243,7 +1234,6 @@ function InnerEditor({ setColumns([]); setFields([]); setSettings([]); - setOptionalSteps([]); setPromptOverrides(null); setPromptOverrideSavingNodeId(null); setName(""); @@ -1255,7 +1245,6 @@ function InnerEditor({ const loadedColumns = columnsOf(activeWorkflow); const loadedFields = fieldsOf(activeWorkflow); const loadedSettings = settingsOf(activeWorkflow); - const loadedOptionalSteps = optionalStepsOf(activeWorkflow); // Auto-layout on load: compute tidy positions and apply them before the // first render so nodes are visible in the top-left viewport. const layoutPositions = autoLayout(flow.nodes, flow.edges, loadedColumns); @@ -1265,7 +1254,6 @@ function InnerEditor({ setColumns(loadedColumns); setFields(loadedFields); setSettings(loadedSettings); - setOptionalSteps(loadedOptionalSteps); setName(activeWorkflow.name); setDescription(activeWorkflow.description ?? ""); setEditingName(false); @@ -1280,7 +1268,6 @@ function InnerEditor({ loadedColumns, loadedFields, loadedSettings, - loadedOptionalSteps, ); setSelectedNodeId(null); setSelectedEdgeId(null); @@ -1455,16 +1442,19 @@ function InnerEditor({ const baseConfig = kind === "gate" ? { gateMode: "gate" } : {}; const config = presetConfig ? { ...baseConfig, ...presetConfig } : baseConfig; - if (kind === "foreach" || kind === "loop") { + if (kind === "foreach" || kind === "loop" || kind === "optional-group") { // Template groups render as React Flow group nodes. Foreach seeds the - // required step-execute seam; loop seeds a regular prompt so authors can - // wire the repeated body immediately. The group node must precede its - // child for React Flow's parent extent to apply. + // required step-execute seam; loop + optional-group seed a regular prompt + // so authors can wire the body immediately. The group node must precede + // its child for React Flow's parent extent to apply. + // FNXC:WorkflowOptionalGroup 2026-06-21-11:30: An optional-group is authored exactly like a foreach/loop region — drop nodes inside; the subgraph runs once when the task enables the group. const childId = foreachChildFlowId(id, newNodeId()); const childLabel = kind === "foreach" ? t("workflowNodes.stepExecuteLabel", "Step execute") - : t("workflowNodes.loopStepLabel", "Loop step"); + : kind === "optional-group" + ? t("workflowNodes.optionalGroupStepLabel", "Optional step") + : t("workflowNodes.loopStepLabel", "Loop step"); const childConfig = kind === "foreach" ? { seam: "step-execute" } : { prompt: "" }; setNodes((ns) => [ ...ns, @@ -1521,6 +1511,34 @@ function InnerEditor({ [isBuiltin, addNode], ); + /* + FNXC:WorkflowOptionalGroup 2026-06-21-14:32: + "Insert as optional group" (U5/R5): drop an add-on already wrapped in an `optional-group` container in + one action, seeding the group's `defaultOn` from the template's `defaultOn`. Reuses `stepTemplateToNode` + (KTD-5 — the catalog stays flat) to project the add-on to a prompt/script node, then `optionalGroupFragmentIr` + to wrap it and the EXISTING `insertFragment` path to remap ids + expand the group's template child — so two + inserts of the same add-on never collide. The group name carries the template name so the per-task toggle + surfaces label it. + */ + const handleInsertStepTemplateAsOptionalGroup = useCallback( + (tpl: WorkflowStepTemplate) => { + if (isBuiltin) return; + const { kind, config } = stepTemplateToNode(tpl); + const fragmentIr = optionalGroupFragmentIr( + { kind: kind as WorkflowIrNodeKind, config }, + { name: tpl.name, defaultOn: tpl.defaultOn ?? false }, + ); + const result = insertFragment(nodes, edges, fragmentIr, { + x: 240, + y: 200 + (nodes.length % 4) * 40, + }); + setNodes(result.nodes); + setEdges(result.edges); + setSelectedNodeId(result.insertedNodeIds[0] ?? null); + }, + [isBuiltin, nodes, edges, setNodes, setEdges], + ); + // U9/R8: insert a fragment definition's body into the active graph. Pre-validates // seam duplication via fragmentSeamConflicts; on conflict, surfaces a persistent // inline error inside the Templates section and does NOT insert. Otherwise @@ -1619,11 +1637,12 @@ function InnerEditor({ setEdges(flow.edges); setColumns(columnsOf({ ...targetWorkflow, ir: result.ir })); setFields(fieldsOf({ ...targetWorkflow, ir: result.ir })); - // Hydrate settings + optionalSteps on the fragment/generate path too — it - // previously dropped both, which silently lost the declarations on the next - // save (the round-trip data loss U2 fixes for the primary load path). + // Hydrate settings on the fragment/generate path too — it previously dropped + // them, which silently lost the declarations on the next save (the round-trip + // data loss U2 fixes for the primary load path). Optional steps need no + // separate hydration: they are graph-native `optional-group` nodes carried by + // the node/edge mapping above (FNXC:WorkflowOptionalGroup 2026-06-21-18:00). setSettings(settingsOf({ ...targetWorkflow, ir: result.ir })); - setOptionalSteps(optionalStepsOf({ ...targetWorkflow, ir: result.ir })); setSelectedNodeId(null); setSelectedEdgeId(null); setValidationError(null); @@ -1996,7 +2015,6 @@ function InnerEditor({ columns.length ? columns : undefined, fields.length ? fields : undefined, settings.length ? settings : undefined, - optionalSteps.length ? optionalSteps : undefined, ); // Include name/description in the PATCH only when they changed from the // loaded workflow (KTD-10 inline rename/description persist here). @@ -2014,7 +2032,6 @@ function InnerEditor({ columns, fields, settings, - optionalSteps, ); setName(updated.name); setDescription(updated.description ?? ""); @@ -2084,7 +2101,7 @@ function InnerEditor({ } finally { setSaving(false); } - }, [activeWorkflow, name, description, nodes, edges, columns, fields, settings, optionalSteps, unplaced, blockingViolationCount, projectId, addToast, t]); + }, [activeWorkflow, name, description, nodes, edges, columns, fields, settings, unplaced, blockingViolationCount, projectId, addToast, t]); // Stamp the shared error-state badge onto offending nodes: unplaced step // nodes and any node the server flagged (seam-in-branch). One component @@ -2101,11 +2118,14 @@ function InnerEditor({ let errorBadge: string | undefined; if (unplacedSet.has(n.id)) errorBadge = t("workflowColumns.nodeUnplaced", "Not placed in a column"); if (serverNodeError?.nodeId === n.id) errorBadge = serverNodeError.message; - const isTemplateGroup = n.data.kind === "foreach" || n.data.kind === "loop"; + const isTemplateGroup = + n.data.kind === "foreach" || n.data.kind === "loop" || n.data.kind === "optional-group"; const emptyHint = n.data.kind === "loop" ? t("workflowNodes.loopEmptyHint", "Drag loop steps here") - : t("workflowNodes.foreachEmptyHint", "Drag a step-execute node here"); + : n.data.kind === "optional-group" + ? t("workflowNodes.optionalGroupEmptyHint", "Drag optional steps here") + : t("workflowNodes.foreachEmptyHint", "Drag a step-execute node here"); const templateEmpty = isTemplateGroup ? (childCount.get(n.id) ?? 0) === 0 : undefined; if ( errorBadge === n.data.errorBadge && @@ -2129,6 +2149,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; @@ -2726,26 +2748,9 @@ function InnerEditor({ )} -
- - {!optionalStepsCollapsed && ( - p.template)} - /> - )} -
+ {/* FNXC:WorkflowOptionalGroup 2026-06-21-18:00: The optional-step + DECLARATION authoring sidebar section is removed. Optional steps + are authored as graph-native `optional-group` nodes on the canvas. */}
)} @@ -2881,7 +2886,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]) => ( @@ -2992,19 +2996,37 @@ function InnerEditor({ {templateGroups.stepEntries.length > 0 && (

{t("workflowNodes.templatesBuiltinSteps", "Built-in steps")}

+ {/* FNXC:WorkflowOptionalGroup 2026-06-21-14:38: mobile mirrors the desktop two-variant insert (node / optional group). */} {templateGroups.stepEntries.map((s) => ( - +
+ + +
))}
)} @@ -3060,16 +3082,6 @@ function InnerEditor({
)} - {mobilePanel === "optional-steps" && ( -
- p.template)} - /> -
- )} {mobilePanel === "columns" && (
@@ -3396,22 +3408,48 @@ function InnerEditor({ {t("workflowNodes.templatesBuiltinSteps", "Built-in steps")}
+ {/* + FNXC:WorkflowOptionalGroup 2026-06-21-14:36: + Each built-in add-on surfaces TWO insert variants: the row inserts as a single node + (today's behavior), and a small secondary "as optional group" affordance wraps it in + an `optional-group` container (U5/R5). Both keep the established `wf-tpl-step-*` testid + convention (the wrap variant suffixes `-optional-group`). + */} {templateGroups.stepEntries.map((s) => ( - +
+ + +
))}
@@ -3587,7 +3625,8 @@ function InnerEditor({ !(compactLayoutEnabled && !isMobileMode) && (