Address PR review feedback (#2006)

- fix: fragments are hidden in the add-step dialog when the target edge is
  inside a foreach/loop/optional-group (they expand to top-level subgraphs
  and cannot splice into a template-child edge), and
  spliceInsertedSubgraphOnEdge now refuses container-internal edges as a
  second line of defense (Greptile P1 x2).
- test: add-step modal container-target hiding, multi-entry/exit splice
  fan-out, internal-cycle entries fallback, ambiguous merge-inbound
  lifecycle fallback, and edge-targeted "as optional group" wiring
  (CodeRabbit nitpicks).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

packages/dashboard/app/components/WorkflowAddStepModal.tsx

@@ -17,7 +17,11 @@ The simplified workflow view's add-step surface. Requirements:
    registry, because the flat 16-button advanced palette is the main thing
    users found hard to use.
  - When inserting INSIDE a container (foreach/loop/optional-group child
-   edge), container kinds are hidden — containers cannot nest.
+   edge), container kinds are hidden — containers cannot nest — and
+   FRAGMENTS are hidden too (PR #2006 review): fragments expand to top-level
+   subgraphs, so splicing one into a template-child edge would create
+   cross-boundary edges into the container. Step templates stay available
+   (they materialize a single prompt/script node, valid as a sibling child).
 */
 
 export interface AddStepPaletteEntry {
@@ -108,8 +112,8 @@ export function WorkflowAddStepModal({
   }, [palette, disallowContainers, q, t]);
 
   const filteredFragments = useMemo(
-    () => fragments.filter((f) => !q || f.name.toLowerCase().includes(q)),
-    [fragments, q],
+    () => (disallowContainers ? [] : fragments.filter((f) => !q || f.name.toLowerCase().includes(q))),
+    [fragments, q, disallowContainers],
   );
   const filteredStepTemplates = useMemo(
     () => stepTemplates.filter((s) => !q || s.name.toLowerCase().includes(q)),

packages/dashboard/app/components/__tests__/WorkflowAddStepModal.test.tsx

@@ -0,0 +1,72 @@
+import { render, screen, cleanup } from "@testing-library/react";
+import { describe, expect, it, vi, afterEach } from "vitest";
+import { MessageSquare, Repeat } from "lucide-react";
+import type { WorkflowDefinition, WorkflowStepTemplate } from "@fusion/core";
+import { WorkflowAddStepModal, type AddStepPaletteEntry } from "../WorkflowAddStepModal";
+
+/*
+FNXC:WorkflowSimpleView 2026-07-12-14:30:
+PR #2006 review coverage: when the add-step dialog targets an edge INSIDE a
+container (disallowContainers), it must hide container palette kinds,
+fragments (top-level subgraphs cannot splice into a template-child edge),
+and the "as optional group" template variant — while keeping plain step
+templates, which materialize a single sibling-safe node.
+*/
+
+const palette: AddStepPaletteEntry[] = [
+  { kind: "prompt", label: "Prompt", icon: MessageSquare },
+  { kind: "loop", label: "Loop", icon: Repeat },
+];
+
+const fragment = {
+  id: "WF-FRAG",
+  kind: "fragment",
+  name: "Lint fragment",
+  description: "",
+  ir: { version: "v1", name: "Lint fragment", nodes: [], edges: [] },
+  layout: {},
+  createdAt: "2026-06-03T00:00:00.000Z",
+  updatedAt: "2026-06-03T00:00:00.000Z",
+} as WorkflowDefinition;
+
+const stepTemplate = { id: "tpl-1", name: "Security review" } as WorkflowStepTemplate;
+
+function renderModal(disallowContainers: boolean) {
+  return render(
+    <WorkflowAddStepModal
+      open
+      onClose={() => {}}
+      palette={palette}
+      disallowContainers={disallowContainers}
+      fragments={[fragment]}
+      stepTemplates={[stepTemplate]}
+      pluginTemplates={[]}
+      onPickPalette={vi.fn()}
+      onPickFragment={vi.fn()}
+      onPickStepTemplate={vi.fn()}
+      onPickStepTemplateAsOptionalGroup={vi.fn()}
+    />,
+  );
+}
+
+describe("WorkflowAddStepModal", () => {
+  afterEach(() => cleanup());
+
+  it("offers containers, fragments, and optional-group inserts for top-level targets", () => {
+    renderModal(false);
+    expect(screen.getByTestId("wf-add-step-loop-loop")).toBeInTheDocument();
+    expect(screen.getByTestId("wf-add-step-fragment-WF-FRAG")).toBeInTheDocument();
+    expect(screen.getByTestId("wf-add-step-tpl-tpl-1")).toBeInTheDocument();
+    expect(screen.getByTestId("wf-add-step-tpl-tpl-1-optional-group")).toBeInTheDocument();
+  });
+
+  it("hides containers, fragments, and optional-group inserts for container-internal targets", () => {
+    renderModal(true);
+    expect(screen.getByTestId("wf-add-step-prompt-prompt")).toBeInTheDocument();
+    expect(screen.queryByTestId("wf-add-step-loop-loop")).not.toBeInTheDocument();
+    expect(screen.queryByTestId("wf-add-step-fragment-WF-FRAG")).not.toBeInTheDocument();
+    // Plain step templates remain — they insert a single sibling-safe node.
+    expect(screen.getByTestId("wf-add-step-tpl-tpl-1")).toBeInTheDocument();
+    expect(screen.queryByTestId("wf-add-step-tpl-tpl-1-optional-group")).not.toBeInTheDocument();
+  });
+});

packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx

@@ -4187,6 +4187,34 @@ describe("WorkflowNodeEditor simplified view modes", () => {
      (workflow-simple-layout.test.ts) and the toolbar-pick test above, which
      exercises the same insertFromAddStep path end-to-end. */
 
+  it("splices an edge-targeted 'as optional group' pick into the targeted edge", async () => {
+    // FNXC:WorkflowSimpleView 2026-07-12-14:30: PR #2006 review coverage —
+    // the optional-group template variant must wire into the targeted edge,
+    // not land free-floating.
+    vi.mocked(fetchWorkflowStepTemplates).mockResolvedValue({
+      templates: [{ id: "tpl-sec", name: "Security review", prompt: "Review security", defaultOn: true }],
+    });
+    vi.mocked(updateWorkflow).mockImplementation(async (_id, updates) => ({ ...def(), ...(updates as object) }));
+    render(<WorkflowNodeEditor isOpen onClose={() => {}} addToast={() => {}} />);
+    await screen.findByTestId("wf-simple-canvas");
+
+    fireEvent.click(screen.getByTestId("wf-simple-toolbar-add-step"));
+    const dialog = await screen.findByTestId("wf-add-step-modal");
+    fireEvent.click(within(dialog).getByTestId("wf-add-step-tpl-tpl-sec-optional-group"));
+    await waitFor(() => expect(screen.queryByTestId("wf-add-step-modal")).not.toBeInTheDocument());
+
+    fireEvent.click(screen.getByText("Save").closest("button")!);
+    await waitFor(() => expect(updateWorkflow).toHaveBeenCalledTimes(1));
+    const [, updates] = vi.mocked(updateWorkflow).mock.calls[0];
+    const ir = (updates as { ir: { nodes: Array<{ id: string; kind: string }>; edges: Array<{ from: string; to: string }> } }).ir;
+    const group = ir.nodes.find((n) => n.kind === "optional-group");
+    expect(group).toBeDefined();
+    // def()'s single edge into end was the target: merge → group → end.
+    expect(ir.edges.some((e) => e.from === "merge" && e.to === "end")).toBe(false);
+    expect(ir.edges.some((e) => e.from === "merge" && e.to === group!.id)).toBe(true);
+    expect(ir.edges.some((e) => e.from === group!.id && e.to === "end")).toBe(true);
+  });
+
   it("keeps built-in workflows read-only in the simplified view", async () => {
     vi.mocked(fetchWorkflows).mockResolvedValue([builtinDef()]);
     render(<WorkflowNodeEditor isOpen onClose={() => {}} addToast={() => {}} />);

packages/dashboard/app/components/__tests__/workflow-lifecycle-autofix.test.ts

@@ -50,6 +50,24 @@ describe("lifecycleFixTargetEdgeId", () => {
     expect(lifecycleFixTargetEdgeId(nodes, edges, "missing-merge-region")).toBe("e3");
   });
 
+  it("falls back to the append edge when the merge node has multiple inbound edges", () => {
+    const nodes = [
+      node("start", "start"),
+      node("a", "prompt", 300, 0),
+      node("b", "prompt", 300, 200),
+      node("m", "merge", 600, 0),
+      node("end", "end", 900, 0),
+    ];
+    const edges = [
+      edge("e1", "start", "a"),
+      edge("e2", "start", "b"),
+      edge("e3", "a", "m"),
+      edge("e4", "b", "m"),
+      edge("e5", "m", "end"),
+    ];
+    expect(lifecycleFixTargetEdgeId(nodes, edges, "missing-completion-summary")).toBe("e5");
+  });
+
   it("falls back to the edge into end when no merge node exists", () => {
     const nodes = [node("start", "start"), node("end", "end", 360, 0)];
     const edges = [edge("e1", "start", "end")];

packages/dashboard/app/components/__tests__/workflow-simple-layout.test.ts

@@ -260,6 +260,48 @@ describe("spliceInsertedSubgraphOnEdge", () => {
     expect(child.position).toEqual({ x: 30, y: 56 });
   });
 
+  it("refuses to splice into a container-internal (template child) edge", () => {
+    // FNXC:WorkflowSimpleView 2026-07-12-14:30: PR #2006 review — subgraphs
+    // are top-level; splicing into a template-child edge would create
+    // cross-boundary edges into the container.
+    const nodes = [
+      ...baseNodes(),
+      node("grp", "foreach", 240, 700, { style: { width: 560, height: 220 } }),
+      node("c1", "prompt", 30, 56, { parentId: "grp" }),
+      node("c2", "prompt", 300, 56, { parentId: "grp" }),
+      node("f1", "gate", 240, 900),
+    ];
+    const edges = [...baseEdges(), edge("t1", "c1", "c2")];
+    expect(spliceInsertedSubgraphOnEdge(nodes, edges, "t1", ["f1"])).toBeNull();
+  });
+
+  it("fans out to multiple entries and exits, preserving the inbound condition on each entry", () => {
+    const nodes = [
+      ...baseNodes(),
+      node("in1", "gate", 200, 700),
+      node("in2", "script", 500, 700),
+      node("out", "prompt", 350, 900),
+    ];
+    // Diamond: in1/in2 are entries (no internal inbound), out is the exit.
+    const edges = [...baseEdges(), edge("i1", "in1", "out"), edge("i2", "in2", "out")];
+    const result = spliceInsertedSubgraphOnEdge(nodes, edges, "e2", ["in1", "in2", "out"]);
+    expect(result).not.toBeNull();
+    const inbound = result!.edges.filter((e) => e.source === "a" && ["in1", "in2"].includes(e.target));
+    expect(inbound).toHaveLength(2);
+    expect(inbound.every((e) => e.data?.condition === "failure")).toBe(true);
+    expect(result!.edges.some((e) => e.source === "out" && e.target === "end")).toBe(true);
+  });
+
+  it("falls back to all inserted nodes when the subgraph is an internal cycle (no entries/exits)", () => {
+    const nodes = [...baseNodes(), node("x", "prompt", 200, 700), node("y", "prompt", 500, 700)];
+    const edges = [...baseEdges(), edge("c1", "x", "y"), edge("c2", "y", "x")];
+    const result = spliceInsertedSubgraphOnEdge(nodes, edges, "e2", ["x", "y"]);
+    expect(result).not.toBeNull();
+    // Every inserted node is treated as both entry and exit.
+    expect(result!.edges.filter((e) => e.source === "a" && ["x", "y"].includes(e.target))).toHaveLength(2);
+    expect(result!.edges.filter((e) => e.target === "end" && ["x", "y"].includes(e.source))).toHaveLength(2);
+  });
+
   it("returns null when the target edge is gone or ineligible", () => {
     const nodes = [...baseNodes(), node("f1", "gate", 240, 700)];
     expect(spliceInsertedSubgraphOnEdge(nodes, baseEdges(), "missing", ["f1"])).toBeNull();

packages/dashboard/app/components/workflow-simple-layout.ts

@@ -299,6 +299,12 @@ export function spliceInsertedSubgraphOnEdge(
   const source = nodes.find((n) => n.id === edge.source);
   const target = nodes.find((n) => n.id === edge.target);
   if (!source || !target) return null;
+  // FNXC:WorkflowSimpleView 2026-07-12-14:30: PR #2006 review — subgraphs
+  // insert as TOP-LEVEL nodes, so splicing one into a container-internal
+  // (template child) edge would wire cross-boundary edges into the container.
+  // Refuse; the caller falls back to the fixed-position insert, and the
+  // add-step dialog hides fragments for container-edge targets anyway.
+  if (source.parentId || target.parentId) return null;
 
   const insertedSet = new Set(insertedNodeIds);
   const insertedTop = nodes.filter(