diff --git a/packages/core/src/__tests__/db-migrate.test.ts b/packages/core/src/__tests__/db-migrate.test.ts index 756152ac05..d38e55c9eb 100644 --- a/packages/core/src/__tests__/db-migrate.test.ts +++ b/packages/core/src/__tests__/db-migrate.test.ts @@ -1073,6 +1073,8 @@ describe("schema migration", () => { expect(reopened.getSchemaVersion()).toBe(109); const workflowColumns = reopened.prepare("PRAGMA table_info(workflows)").all() as Array<{ name: string }>; expect(workflowColumns.filter((c) => c.name === "kind")).toHaveLength(1); + const stepColumns = reopened.prepare("PRAGMA table_info(workflow_steps)").all() as Array<{ name: string }>; + expect(stepColumns.filter((c) => c.name === "migrated_fragment_id")).toHaveLength(1); reopened.close(); }); }); diff --git a/packages/core/src/__tests__/goals-schema.test.ts b/packages/core/src/__tests__/goals-schema.test.ts index 03398b9515..1bbcf5c447 100644 --- a/packages/core/src/__tests__/goals-schema.test.ts +++ b/packages/core/src/__tests__/goals-schema.test.ts @@ -90,7 +90,7 @@ describe("goals schema", () => { expect(table?.name).toBe("goals"); }); - it("reports schema version 101", () => { + it("reports schema version 109", () => { expect(db.getSchemaVersion()).toBe(109); }); }); diff --git a/packages/core/src/__tests__/mission-store.test.ts b/packages/core/src/__tests__/mission-store.test.ts index 2ded53b090..10de8917a2 100644 --- a/packages/core/src/__tests__/mission-store.test.ts +++ b/packages/core/src/__tests__/mission-store.test.ts @@ -3745,7 +3745,7 @@ describe("MissionStore", () => { // ── Loop State & Validator Run Schema Tests ─────────────────────────── describe("Loop State & Validator Run Schema (v31)", () => { - it("schema version is 101 after migration", () => { + it("schema version is 109 after migration", () => { expect(db.getSchemaVersion()).toBe(109); }); diff --git a/packages/core/src/__tests__/run-audit.test.ts b/packages/core/src/__tests__/run-audit.test.ts index ca428b6914..7c818fa605 100644 --- a/packages/core/src/__tests__/run-audit.test.ts +++ b/packages/core/src/__tests__/run-audit.test.ts @@ -583,7 +583,7 @@ describe("Run Audit", () => { expect(indexNames).toContain("idxRunAuditEventsTimestamp"); }); - it("schema version is bumped to 40", () => { + it("schema version is bumped to 109", () => { expect(db.getSchemaVersion()).toBe(109); }); }); diff --git a/packages/core/src/__tests__/strip-approval-bypass-flags.test.ts b/packages/core/src/__tests__/strip-approval-bypass-flags.test.ts index b17e3b26fd..60de85e497 100644 --- a/packages/core/src/__tests__/strip-approval-bypass-flags.test.ts +++ b/packages/core/src/__tests__/strip-approval-bypass-flags.test.ts @@ -76,4 +76,36 @@ describe("stripApprovalBypassFlags", () => { const ir = { version: "v1", name: "wf" } as unknown as WorkflowIr; expect(stripApprovalBypassFlags(ir).stripped).toBe(false); }); + + it("tolerates non-object entries in nodes (untrusted input)", () => { + const ir = { + version: "v1", + name: "wf", + nodes: [null, "bogus", 42, { id: "n1", kind: "prompt", config: { cliSkipApproval: true } }], + edges: [], + } as unknown as WorkflowIr; + const { ir: out, stripped } = stripApprovalBypassFlags(ir); + expect(stripped).toBe(true); + expect((out as any).nodes[3].config.cliSkipApproval).toBeUndefined(); + }); + + it("tolerates non-object entries in nested template.nodes", () => { + const ir = { + version: "v1", + name: "wf", + nodes: [ + { + id: "fe", + kind: "foreach", + config: { + template: { nodes: [null, 0, "x", { id: "inner", kind: "prompt", config: { autoApprove: true } }] }, + }, + }, + ], + edges: [], + } as unknown as WorkflowIr; + const { ir: out, stripped } = stripApprovalBypassFlags(ir); + expect(stripped).toBe(true); + expect((out as any).nodes[0].config.template.nodes[3].config.autoApprove).toBeUndefined(); + }); }); diff --git a/packages/core/src/__tests__/workflow-definition-store.test.ts b/packages/core/src/__tests__/workflow-definition-store.test.ts index b7501997de..64860b46c2 100644 --- a/packages/core/src/__tests__/workflow-definition-store.test.ts +++ b/packages/core/src/__tests__/workflow-definition-store.test.ts @@ -273,4 +273,39 @@ describe("TaskStore workflow definitions (U1)", () => { const task = await store.createTask({ description: "t" }); await expect(store.selectTaskWorkflow(task.id, frag.id)).rejects.toThrow(/fragment/i); }); + + it("setDefaultWorkflowId rejects a fragment id at the write boundary", async () => { + const frag = await store.createWorkflowDefinition({ name: "Frag", ir: fragmentIr(), kind: "fragment" }); + await expect(store.setDefaultWorkflowId(frag.id)).rejects.toThrow(/fragment/i); + expect(await store.getDefaultWorkflowId()).toBeUndefined(); + }); + + it("setDefaultWorkflowId accepts a real workflow and clears with null", async () => { + const wf = await store.createWorkflowDefinition({ name: "W", ir: makeIr() }); + await store.setDefaultWorkflowId(wf.id); + expect(await store.getDefaultWorkflowId()).toBe(wf.id); + await store.setDefaultWorkflowId(null); + expect(await store.getDefaultWorkflowId()).toBeUndefined(); + }); + + it("createTaskWithReservedId honors an explicit workflowId (precedence over default)", async () => { + const def = await store.createWorkflowDefinition({ name: "Explicit", ir: makeIr() }); + const task = await store.createTaskWithReservedId( + { description: "t", workflowId: def.id }, + { taskId: "task-explicit-wf" }, + ); + const sel = store.getTaskWorkflowSelection(task.id); + expect(sel?.workflowId).toBe(def.id); + }); + + it("createTaskWithReservedId treats workflowId:null as explicit opt-out", async () => { + const def = await store.createWorkflowDefinition({ name: "Def", ir: makeIr() }); + await store.setDefaultWorkflowId(def.id); + const task = await store.createTaskWithReservedId( + { description: "t", workflowId: null }, + { taskId: "task-optout-wf" }, + ); + const sel = store.getTaskWorkflowSelection(task.id); + expect(sel?.workflowId ?? undefined).toBeUndefined(); + }); }); diff --git a/packages/core/src/__tests__/workflow-step-migration.test.ts b/packages/core/src/__tests__/workflow-step-migration.test.ts index 1bf15a89fa..62d47bf2d3 100644 --- a/packages/core/src/__tests__/workflow-step-migration.test.ts +++ b/packages/core/src/__tests__/workflow-step-migration.test.ts @@ -155,6 +155,36 @@ describe("TaskStore.migrateLegacyWorkflowSteps (U2/R5)", () => { expect(await store.getDefaultWorkflowId()).toBe(existing.id); }); + it("compare-and-set: re-reads the default after the transaction and skips when a concurrent writer set one", async () => { + await store.createWorkflowStep({ name: "A", description: "a", prompt: "a", defaultOn: true }); + + const concurrent = await store.createWorkflowDefinition({ + name: "Concurrent", + ir: { + version: "v1", + name: "Concurrent", + nodes: [ + { id: "start", kind: "start" }, + { id: "end", kind: "end" }, + ], + edges: [{ from: "start", to: "end", condition: "success" }], + }, + kind: "workflow", + }); + + // A project default exists when migration's post-transaction compare-and-set + // re-reads it. Because the set is gated on the re-read (not a pre-transaction + // snapshot), an existing default is observed and never clobbered. + await store.setDefaultWorkflowId(concurrent.id); + + const result = await store.migrateLegacyWorkflowSteps(); + + expect(result.combinedWorkflowId).toBeTruthy(); + expect(result.combinedWorkflowId).not.toBe(concurrent.id); + // The compare-and-set re-read observed the existing default and did NOT clobber it. + expect(await store.getDefaultWorkflowId()).toBe(concurrent.id); + }); + it("is a no-op with zero user steps", async () => { const result = await store.migrateLegacyWorkflowSteps(); expect(result).toEqual({ migrated: 0, skipped: 0, combinedWorkflowId: undefined }); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index bc04eee0e7..4dafc0fc5f 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -4161,7 +4161,24 @@ export class TaskStore extends EventEmitter { : undefined; let pendingWorkflowSelection: { workflowId: string; stepIds: string[] } | undefined; - if (input.enabledWorkflowSteps === undefined && options.applyDefaultWorkflowSteps !== false) { + // U6/R3/KTD-4: an explicit create-time workflowId beats the project default, + // mirroring createTask(). `null` is an explicit opt-out, `string` materializes + // that workflow, `undefined` falls through to the default-workflow behavior. + // Explicit enabledWorkflowSteps still wins over workflowId for trusted callers. + const explicitWorkflowId = + input.enabledWorkflowSteps === undefined ? input.workflowId : undefined; + if (explicitWorkflowId !== undefined) { + if (explicitWorkflowId === null) { + // Explicit "No workflow": skip default materialization entirely. + resolvedWorkflowSteps = undefined; + } else { + // Compile + materialize up front so unknown/fragment ids throw BEFORE + // the task row is created (no orphaned steps, no half-created task). + const selected = await this.materializeExplicitWorkflowSteps(explicitWorkflowId); + resolvedWorkflowSteps = selected.stepIds; + pendingWorkflowSelection = selected; + } + } else if (input.enabledWorkflowSteps === undefined && options.applyDefaultWorkflowSteps !== false) { // Mirror createTask: a configured project default workflow takes // precedence over legacy default-on steps on this creation path too. try { @@ -13010,6 +13027,13 @@ ${stepsSection}`; if (workflowId) { const exists = await this.getWorkflowDefinition(workflowId); if (!exists) throw new Error(`Workflow '${workflowId}' not found`); + // KTD-1/R6: a fragment is a reusable palette piece, not a selectable + // workflow. Reject it at the write boundary so a fragment can never be + // persisted as the project default (the read-side skip in + // materializeDefaultWorkflowSteps remains as defense in depth). + if (exists.kind === "fragment") { + throw new Error(`Workflow '${workflowId}' is a fragment and cannot be set as the project default`); + } } // null is updateSettings' explicit-delete sentinel for project keys. await this.updateSettings({ defaultWorkflowId: workflowId } as unknown as Partial); @@ -13090,10 +13114,10 @@ ${stepsSection}`; combinedWorkflowId?: string; }> { // Resolve async prerequisites BEFORE the synchronous transaction: the - // workflow-columns flag (for flag-aware persistence) and the current project - // default (for the no-clobber guard). + // workflow-columns flag (for flag-aware persistence). The project default is + // re-read AFTER the transaction (compare-and-set) so a concurrently-set + // default is never clobbered. const flagOn = await this.workflowColumnsFlagOn(); - const existingDefaultId = await this.getDefaultWorkflowId(); const result = this.db.transactionImmediate(() => { // Write lock is now held. Read the raw step rows directly (the cached, @@ -13118,13 +13142,16 @@ ${stepsSection}`; // Every unmigrated user step → a single-node fragment; stamp the source row. for (const step of unmigrated) { + // parseWorkflowIr runs inside both insertWorkflowDefinitionSync and + // layoutForIr, so compute the fragment IR once and reuse it. + const fragmentIr = stepToFragmentIr(step); const fragment = this.insertWorkflowDefinitionSync( { name: step.name, description: step.description, kind: "fragment", - ir: stepToFragmentIr(step), - layout: layoutForIr(stepToFragmentIr(step)), + ir: fragmentIr, + layout: layoutForIr(fragmentIr), }, flagOn, ); @@ -13159,10 +13186,25 @@ ${stepsSection}`; // Set the combined workflow as the project default — only when one was // created AND no explicit default is already set (don't clobber a user // choice). Done outside the transaction via the async setter so the project - // default-workflow hooks run. Racing re-runs are harmless: the second run - // creates no combined workflow, so this branch is skipped. - if (result.combinedWorkflowId && !existingDefaultId) { - await this.setDefaultWorkflowId(result.combinedWorkflowId); + // default-workflow hooks run. Compare-and-set against the CURRENT default + // (re-read immediately before writing, not the pre-transaction snapshot) so + // a default set concurrently by another writer is never overwritten. If the + // set fails, swallow the error: a missing migrated default is recoverable + // (the user can set one), but throwing here would surface the whole + // migration as failed even though the definitions were written. + if (result.combinedWorkflowId) { + const currentDefaultId = await this.getDefaultWorkflowId(); + if (!currentDefaultId) { + try { + await this.setDefaultWorkflowId(result.combinedWorkflowId); + } catch (err) { + storeLog.warn("Failed to set migrated combined workflow as project default", { + phase: "migrateLegacyWorkflowSteps:set-default", + combinedWorkflowId: result.combinedWorkflowId, + error: err instanceof Error ? err.message : String(err), + }); + } + } } return result; diff --git a/packages/core/src/workflow-ir.ts b/packages/core/src/workflow-ir.ts index 577169eb14..9489431fca 100644 --- a/packages/core/src/workflow-ir.ts +++ b/packages/core/src/workflow-ir.ts @@ -923,6 +923,9 @@ export function stripApprovalBypassFlags(ir: WorkflowIr): { ir: WorkflowIr; stri if (!Array.isArray(nodes)) return { ir, stripped: false }; let stripped = false; const stripNode = (node: WorkflowIrNode): void => { + // Untrusted input may contain non-object entries (null, strings, numbers) + // in `nodes` / `template.nodes`; skip them rather than dereferencing. + if (!node || typeof node !== "object") return; const cfg = node.config as Record | undefined; if (cfg && typeof cfg === "object") { if ("cliSkipApproval" in cfg) { diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.tsx b/packages/dashboard/app/components/WorkflowNodeEditor.tsx index 545a040fc0..c03bd387af 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.tsx +++ b/packages/dashboard/app/components/WorkflowNodeEditor.tsx @@ -743,6 +743,11 @@ function InnerEditor({ const activeWorkflow = useMemo(() => workflows.find((w) => w.id === activeId), [workflows, activeId]); const isBuiltin = !!activeWorkflow && isBuiltinWorkflowId(activeWorkflow.id); + // Live mirror of the active workflow id, readable inside async callbacks that + // captured an earlier value before an await (e.g. the AI-design round-trip). + const activeIdRef = useRef(activeId); + activeIdRef.current = activeId; + // Trivial-graph palette hint (R9): a user-owned workflow whose graph carries no // user-authored node yet (everything is start/end/column-band — column bands map // to data.kind "start"). Disappears as soon as any user node exists; never shows @@ -1216,14 +1221,26 @@ function InnerEditor({ } const controller = new AbortController(); aiEditAbortRef.current = controller; + // Capture the target workflow up-front: if the user switches the active + // workflow during the (long) design round-trip, we must NOT apply the result + // to whatever workflow happens to be active when it resolves. + const targetWorkflow = activeWorkflow; setAiEditBusy(true); setAiEditError(null); try { const result = await designWorkflow( - { prompt: trimmed, workflowId: activeWorkflow.id }, + { prompt: trimmed, workflowId: targetWorkflow.id }, projectId, controller.signal, ); + // The active workflow changed mid-flight → discard the stale result. + if (activeIdRef.current !== targetWorkflow.id) { + addToast( + t("workflows.aiStaleDiscarded", "Discarded AI design — you switched workflows"), + "warning", + ); + return; + } // Always confirm before the destructive replace. const ok = await confirm({ title: t("workflows.aiReplaceTitle", "Replace graph?"), @@ -1238,14 +1255,14 @@ function InnerEditor({ // Map the returned IR through irToFlow on a definition-shaped object, // mirroring the active-workflow load effect's mapping. const flow = irToFlow({ - ...activeWorkflow, + ...targetWorkflow, ir: result.ir, layout: result.layout, }); setNodes(flow.nodes); setEdges(flow.edges); - setColumns(columnsOf({ ...activeWorkflow, ir: result.ir })); - setFields(fieldsOf({ ...activeWorkflow, ir: result.ir })); + setColumns(columnsOf({ ...targetWorkflow, ir: result.ir })); + setFields(fieldsOf({ ...targetWorkflow, ir: result.ir })); setSelectedNodeId(null); setSelectedEdgeId(null); setValidationError(null); diff --git a/packages/dashboard/app/components/__tests__/TaskForm.test.tsx b/packages/dashboard/app/components/__tests__/TaskForm.test.tsx index ae209f3dee..22bc7553cd 100644 --- a/packages/dashboard/app/components/__tests__/TaskForm.test.tsx +++ b/packages/dashboard/app/components/__tests__/TaskForm.test.tsx @@ -1108,23 +1108,39 @@ describe("TaskForm workflow picker (U6/R3)", () => { renderTaskForm({ onWorkflowIdChange: vi.fn() }); expect(screen.getByTestId("task-workflow-loading")).toBeTruthy(); - resolveFn([]); - }); - - it("regression: no per-step checkboxes and no fetchWorkflowSteps usage", async () => { - await mockWorkflows([{ id: "WF-1", name: "QA" }]); - renderTaskForm({ onWorkflowIdChange: vi.fn() }); + resolveFn([{ id: "WF-1", name: "QA" }]); + // After the promise resolves, the loading placeholder is replaced by the + // populated select containing the fetched workflow option. await waitFor(() => { - expect(screen.getByTestId("task-workflow-select")).toBeTruthy(); + expect(screen.queryByTestId("task-workflow-loading")).toBeNull(); }); - // The old per-step checkbox UI and execution-order controls are gone. - expect(screen.queryByTestId("workflow-step-order")).toBeNull(); - expect(document.querySelector('[data-testid^="workflow-step-checkbox-"]')).toBeNull(); - // The api mock no longer needs a fetchWorkflowSteps export — TaskForm never - // calls it. (If it still did, rendering above would have thrown on the - // missing mock export, so reaching this point is itself the regression proof.) + const select = screen.getByTestId("task-workflow-select") as HTMLSelectElement; + const optionValues = Array.from(select.options).map((o) => o.value); + expect(optionValues).toContain("WF-1"); }); + + it.each([ + ["create", { mode: "create" as const }], + ["edit", { mode: "edit" as const, title: "Existing task", onTitleChange: vi.fn() }], + ])( + "regression: no per-step checkboxes and no fetchWorkflowSteps usage (%s mode)", + async (_label, modeProps) => { + await mockWorkflows([{ id: "WF-1", name: "QA" }]); + renderTaskForm({ onWorkflowIdChange: vi.fn(), ...modeProps }); + + await waitFor(() => { + expect(screen.getByTestId("task-workflow-select")).toBeTruthy(); + }); + // The old per-step checkbox UI and execution-order controls are gone on + // every TaskForm surface (create and edit). + expect(screen.queryByTestId("workflow-step-order")).toBeNull(); + expect(document.querySelector('[data-testid^="workflow-step-checkbox-"]')).toBeNull(); + // The api mock no longer needs a fetchWorkflowSteps export — TaskForm never + // calls it. (If it still did, rendering above would have thrown on the + // missing mock export, so reaching this point is itself the regression proof.) + }, + ); }); describe("TaskForm focus behavior (FN-1459)", () => { diff --git a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx index 14a8111865..d4e7748912 100644 --- a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx +++ b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx @@ -1016,7 +1016,9 @@ describe("WorkflowNodeEditor — U4 create dialog / delete / inline rename / dir render( {}} addToast={() => {}} />); fireEvent.click((await screen.findByText("Delete")).closest("button")!); // The no-op fallback resolves false → deleteWorkflow is never called. - await new Promise((r) => setTimeout(r, 20)); + // Let the async fallback settle deterministically (no wall-clock delay). + await Promise.resolve(); + await Promise.resolve(); expect(deleteWorkflow).not.toHaveBeenCalled(); }); @@ -1167,6 +1169,7 @@ describe("WorkflowNodeEditor — U4 create dialog / delete / inline rename / dir function trivialUserDef(): WorkflowDefinition { return { id: "WF-TRIVIAL", + kind: "workflow", name: "Trivial", description: "", ir: { @@ -1536,11 +1539,14 @@ describe("WorkflowNodeEditor — U9 palette Templates section", () => { render( {}} addToast={() => {}} />); await screen.findByTestId("wf-palette-templates"); + // Canvas can lag the palette under cold-transform shard load. + await screen.findByTestId("wf-node-gate", undefined, { timeout: 3000 }); const before = screen.queryAllByTestId("wf-node-prompt").length; fireEvent.click(screen.getByTestId("wf-tpl-step-qa-check")); - await waitFor(() => - expect(screen.queryAllByTestId("wf-node-prompt").length).toBe(before + 1), + await waitFor( + () => expect(screen.queryAllByTestId("wf-node-prompt").length).toBe(before + 1), + { timeout: 3000 }, ); }); diff --git a/packages/dashboard/app/components/__tests__/workflow-auto-layout.test.ts b/packages/dashboard/app/components/__tests__/workflow-auto-layout.test.ts index 919ae38bfd..4a19724167 100644 --- a/packages/dashboard/app/components/__tests__/workflow-auto-layout.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-auto-layout.test.ts @@ -201,6 +201,25 @@ describe("autoLayout — foreach / unreachable / cycles", () => { expect(strictColumnForY(o.y, COLUMNS_3)).toBe("done"); }); + it("leaves an unplaced node (no column, parked outside all bands) unplaced", () => { + // y far below the last band, with no explicit column → strictColumnForY undefined. + const outsideY = bandTop(COLUMNS_3.length) + 5000; + const nodes: N[] = [ + node("start", "start", 0, midBand(0), { column: "triage" }), + node("a", "prompt", 0, midBand(1), { column: "in-progress" }), + node("loose", "prompt", 0, outsideY), + ]; + const edges = [edge("start", "a"), edge("a", "loose")]; + const pos = autoLayout(nodes, edges, COLUMNS_3); + + const loose = pos.get("loose")!; + // y is preserved (still outside every band) — NOT clamped into a band. + expect(loose.y).toBe(outsideY); + expect(strictColumnForY(loose.y, COLUMNS_3)).toBeUndefined(); + // x still flows left-to-right with the layering tidy. + expect(loose.x).toBeGreaterThan(pos.get("a")!.x); + }); + it("terminates and stays sane with a rework cycle edge present", () => { const nodes: N[] = [ node("start", "start", 0, midBand(0), { column: "triage" }), diff --git a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts index d6858ad1df..f04ef9da76 100644 --- a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts @@ -584,11 +584,36 @@ describe("edge-condition authoring (U2)", () => { error: "missing-endpoint", }); - // duplicate of the same condition. - expect(buildConnectionEdge({ source: "a", target: "b" }, edges, nodes)).toEqual({ + // second connect of an existing success pair (prompt source supports + // conditions) → births the parallel failure edge rather than rejecting. + const failureBirth = buildConnectionEdge({ source: "a", target: "b" }, edges, nodes); + expect("edge" in failureBirth).toBe(true); + if ("edge" in failureBirth) { + expect(failureBirth.edge.data?.condition).toBe("failure"); + } + + // once BOTH success and failure exist, a third connect is a duplicate. + const bothConditions = [ + ...edges, + { id: "3", source: "a", target: "b", data: { condition: "failure" } }, + ]; + expect(buildConnectionEdge({ source: "a", target: "b" }, bothConditions, nodes)).toEqual({ error: "duplicate", }); + // a source kind that does NOT support conditions stays a hard duplicate. + const readonlyNodes: FlowNode[] = [ + { id: "a", type: "start", position: { x: 0, y: 0 }, data: { kind: "start", label: "a" } }, + { id: "b", type: "prompt", position: { x: 100, y: 0 }, data: { kind: "prompt", label: "b" } }, + ]; + expect( + buildConnectionEdge( + { source: "a", target: "b" }, + [{ id: "1", source: "a", target: "b", data: { condition: "success" } }], + readonlyNodes, + ), + ).toEqual({ error: "duplicate" }); + // cycle: c→a closes a→b→c→a. expect(buildConnectionEdge({ source: "c", target: "a" }, edges, nodes)).toEqual({ error: "cycle", @@ -867,6 +892,59 @@ describe("insertFragment", () => { const allIds = second.nodes.map((n) => n.id); expect(new Set(allIds).size).toBe(allIds.length); }); + + it("expands a foreach fragment's template so flowToIr round-trips the full template", () => { + // Fragment: start → loop(foreach with a 2-node template) → end. + const foreachFragment: WorkflowDefinition["ir"] = { + version: "v1", + name: "frag", + nodes: [ + { id: "start", kind: "start" }, + { + id: "loop", + kind: "foreach", + config: { + source: "task-steps", + template: { + nodes: [ + { id: "t1", kind: "prompt", config: { prompt: "inner1" } }, + { id: "t2", kind: "prompt", config: { prompt: "inner2" } }, + ], + edges: [{ from: "t1", to: "t2", condition: "success" }], + }, + }, + }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "loop", condition: "success" }, + { from: "loop", to: "end", condition: "success" }, + ], + }; + + const existing = irToFlow(u8ChainDef()); + const { nodes, edges, insertedNodeIds } = insertFragment( + existing.nodes, + existing.edges, + foreachFragment, + { x: 400, y: 200 }, + ); + + // The foreach group's template children were expanded as parented nodes. + const groupId = insertedNodeIds[0]; + const children = nodes.filter((n) => n.parentId === groupId); + expect(children).toHaveLength(2); + + // Round-trip the live canvas back to IR — the inserted foreach must carry its + // full template (not an empty one) with both inner nodes and the inner edge. + const { ir: out } = flowToIr("wf", nodes, edges); + const loop = out.nodes.find((n) => n.kind === "foreach")!; + expect(loop).toBeTruthy(); + const template = (loop.config as { template?: { nodes: unknown[]; edges: unknown[] } }) + .template; + expect(template?.nodes).toHaveLength(2); + expect(template?.edges).toHaveLength(1); + }); }); describe("fragmentSeamConflicts", () => { @@ -999,4 +1077,37 @@ describe("copyIrWithFreshIds", () => { expect(result.ir.nodes.every((n) => n.column === "in-progress")).toBe(true); } }); + + it("remaps namespaced foreach child layout keys consistently with the template ids", () => { + const ir = v2WithForeach(); + const layout = { + start: { x: 0, y: 0 }, + loop: { x: 100, y: 0 }, + "loop::t1": { x: 10, y: 20 }, + "loop::t2": { x: 270, y: 20 }, + end: { x: 400, y: 0 }, + }; + const result = copyIrWithFreshIds(ir, layout); + + const loop = result.ir.nodes.find((n) => n.kind === "foreach")!; + const template = (loop.config as { template: { nodes: { id: string }[] } }).template; + const newGroupId = loop.id; + const childKeys = Object.keys(result.layout).filter((k) => k.includes("::")); + + // Both namespaced child keys survive (count preserved). + expect(childKeys).toHaveLength(2); + // Each child key is `${newGroupId}::${newTemplateId}` for a real template id. + const newInnerIds = new Set(template.nodes.map((n) => n.id)); + for (const k of childKeys) { + const [g, inner] = k.split("::"); + expect(g).toBe(newGroupId); + expect(newInnerIds.has(inner)).toBe(true); + // No stale original ids leak through. + expect(g).not.toBe("loop"); + expect(["t1", "t2"]).not.toContain(inner); + } + // Values preserved by position (t1's offset stays with the remapped t1 key). + const t1NewId = result.layout[`${newGroupId}::${template.nodes[0].id}`]; + expect(t1NewId).toEqual({ x: 10, y: 20 }); + }); }); diff --git a/packages/dashboard/app/components/workflow-auto-layout.ts b/packages/dashboard/app/components/workflow-auto-layout.ts index e2c042be2b..5de7389036 100644 --- a/packages/dashboard/app/components/workflow-auto-layout.ts +++ b/packages/dashboard/app/components/workflow-auto-layout.ts @@ -6,7 +6,7 @@ import { WF_CARD_HEIGHT, COLUMN_BAND_HEIGHT, bandTop, - columnForY, + strictColumnForY, isColumnBandNode, } from "./workflow-flow-mapping"; @@ -174,8 +174,20 @@ export function autoLayout( const rowsPerColumn = new Map(); for (const id of sorted) { const node = byId.get(id)!; - const colId = node.data.column ?? columnForY(node.position.y, columns); - const colIndex = colId ? columns.findIndex((c) => c.id === colId) : -1; + // Resolve the node's column WITHOUT clamping: an explicit column id, or a + // strict band hit-test. A node with neither (parked outside every band and + // carrying no column) is "unplaced" and must stay that way — auto-layout + // never silently re-columns it into the nearest band. We give it the new + // layer x (so the tidy still flows it left-to-right) but preserve its y so + // strictColumnForY(newY) remains undefined. + const colId = + node.data.column ?? + (strictColumnForY(node.position.y, columns) ? strictColumnForY(node.position.y, columns) : undefined); + if (!colId) { + positions.set(id, { x: layerX, y: node.position.y }); + continue; + } + const colIndex = columns.findIndex((c) => c.id === colId); const safeColIndex = colIndex >= 0 ? colIndex : 0; const top = bandTop(safeColIndex); diff --git a/packages/dashboard/app/components/workflow-flow-mapping.ts b/packages/dashboard/app/components/workflow-flow-mapping.ts index a1a162c542..ab97859a23 100644 --- a/packages/dashboard/app/components/workflow-flow-mapping.ts +++ b/packages/dashboard/app/components/workflow-flow-mapping.ts @@ -574,23 +574,38 @@ export function buildConnectionEdge( const target = connection.target ?? undefined; if (!source || !target) return { error: "missing-endpoint" }; - const condition = "success"; - // Skip exact duplicates of the SAME condition (a second identical edge is - // pointless); different conditions between the same pair are allowed. - const isDuplicate = edges.some( - (e) => - e.source === source && - e.target === target && - ((e.data?.condition as string | undefined) ?? "success") === condition, + const srcNode = nodes.find((n) => n.id === source); + const tgtNode = nodes.find((n) => n.id === target); + + // Existing conditions already authored between this exact pair. + const existingConditions = new Set( + edges + .filter((e) => e.source === source && e.target === target) + .map((e) => (e.data?.condition as string | undefined) ?? "success"), ); - if (isDuplicate) return { error: "duplicate" }; + + // New edges are normally born "success". But a second connect gesture between a + // pair that already has a success edge should author the *parallel* "failure" + // edge (rather than being rejected as a duplicate), so users can build a + // success/failure split with two connect gestures — but only when the source + // kind actually exposes a condition select. Block only when both conditions + // already exist (or the only available condition is already taken). + const supportsConditions = edgeConditionEditability(srcNode?.data.kind) === "conditions"; + let condition = "success"; + if (existingConditions.has("success")) { + if (supportsConditions && !existingConditions.has("failure")) { + condition = "failure"; + } else { + return { error: "duplicate" }; + } + } else if (existingConditions.has(condition)) { + return { error: "duplicate" }; + } // Cycle guard (KTD-9). Exempt connections where both endpoints are children of // the same foreach template — those may legitimately be rework cycles authored // separately; the simplest correct rule applies the guard only to non-template // connections. - const srcNode = nodes.find((n) => n.id === source); - const tgtNode = nodes.find((n) => n.id === target); const bothTemplateChildren = !!srcNode?.parentId && srcNode.parentId === tgtNode?.parentId; if (!bothTemplateChildren && wouldCreateCycle(edges, source, target)) { @@ -899,6 +914,12 @@ export function insertFragment( const minY = placed.length ? Math.min(...placed.map((p) => p.y)) : 0; const insertedNodeIds: string[] = []; + // foreach template children are expanded into parented child flow nodes (the + // same way irToFlow does), so an inserted foreach round-trips its full template + // through flowToIr instead of dropping config.template (which flowToIr would + // otherwise rebuild as an empty template from the absent children). + const childNodes: FlowNode[] = []; + const childEdges: FlowEdge[] = []; const newNodes = bodyNodes.map((node, index): FlowNode => { const id = idMap.get(node.id)!; insertedNodeIds.push(id); @@ -906,6 +927,40 @@ export function insertFragment( const pos = fromLayout ? { x: position.x + (fromLayout.x - minX), y: position.y + (fromLayout.y - minY) } : { x: position.x + index * 180, y: position.y }; + const foreachCfg = foreachConfigOf(node); + if (foreachCfg) { + const template = foreachCfg.template; + template.nodes.forEach((inner, innerIdx) => { + const innerKind = editorKind(inner); + childNodes.push({ + id: foreachChildFlowId(id, inner.id), + type: innerKind, + position: { x: FOREACH_CHILD_X + innerIdx * FOREACH_CHILD_STEP_X, y: FOREACH_CHILD_Y }, + parentId: id, + extent: "parent", + data: { kind: innerKind, label: nodeLabel(inner), config: { ...(inner.config ?? {}) } }, + deletable: true, + }); + }); + template.edges.forEach((edge, eIdx) => { + childEdges.push(irEdgeToFlow(edge, eIdx, `${id}${FOREACH_CHILD_SEP}`)); + }); + // The group node keeps everything except the template (children carry it). + const { template: _t, ...restCfg } = (node.config ?? {}) as Record; + return { + id, + type: "foreach", + position: pos, + data: { + kind: "foreach", + label: nodeLabel(node), + config: { ...restCfg }, + templateEmpty: template.nodes.length === 0, + }, + style: { width: FOREACH_GROUP_WIDTH, height: FOREACH_GROUP_HEIGHT }, + deletable: true, + }; + } return irNodeToFlowNode(node, id, pos); }); @@ -925,8 +980,9 @@ export function insertFragment( }); return { - nodes: [...nodes, ...newNodes], - edges: [...edges, ...newEdges], + // Group nodes (in newNodes) must precede their children (childNodes). + nodes: [...nodes, ...newNodes, ...childNodes], + edges: [...edges, ...newEdges, ...childEdges], insertedNodeIds, }; } @@ -935,11 +991,10 @@ export function insertFragment( * new template object; the original is untouched. Template-local ids are scoped * to the template, so a fresh local id space suffices (and keeps config compact * rather than reusing global ids). */ -function copyForeachTemplate(template: { - nodes: WorkflowIrNode[]; - edges: WorkflowIrEdge[]; -}): { nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[] } { - const innerMap = new Map(); +function copyForeachTemplate( + template: { nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[] }, + innerMap: Map, +): { nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[] } { for (const n of template.nodes) innerMap.set(n.id, newNodeId()); const nodes = template.nodes.map((n) => copyIrNode(n, innerMap.get(n.id)!)); const edges = template.edges.map((e) => ({ @@ -951,12 +1006,21 @@ function copyForeachTemplate(template: { } /** Deep-ish copy of an IR node under a new id, recursing into a foreach - * template's internal node references so they remain self-consistent. */ -function copyIrNode(node: WorkflowIrNode, newId: string): WorkflowIrNode { + * template's internal node references so they remain self-consistent. When the + * node is a foreach, its template-local id remap is recorded in `templateMaps` + * keyed by the node's ORIGINAL id, so the caller can remap namespaced + * `${groupId}::${templateNodeId}` layout keys consistently. */ +function copyIrNode( + node: WorkflowIrNode, + newId: string, + templateMaps?: Map>, +): WorkflowIrNode { const config = node.config ? { ...node.config } : undefined; const foreach = foreachConfigOf(node); if (foreach && config) { - config.template = copyForeachTemplate(foreach.template); + const innerMap = new Map(); + config.template = copyForeachTemplate(foreach.template, innerMap); + templateMaps?.set(node.id, innerMap); } const copy: WorkflowIrNode = { id: newId, kind: node.kind }; if (node.column !== undefined) copy.column = node.column; @@ -978,16 +1042,34 @@ export function copyIrWithFreshIds( const idMap = new Map(); for (const n of ir.nodes) idMap.set(n.id, newNodeId()); - const nodes = ir.nodes.map((n) => copyIrNode(n, idMap.get(n.id)!)); + // Per foreach group (by ORIGINAL group id): its template-local id remap, so + // namespaced layout keys `${groupId}::${templateNodeId}` can be remapped to + // `${newGroupId}::${newTemplateNodeId}` consistently. + const templateMaps = new Map>(); + const nodes = ir.nodes.map((n) => copyIrNode(n, idMap.get(n.id)!, templateMaps)); const edges = ir.edges.map((e) => ({ ...e, from: idMap.get(e.from) ?? e.from, to: idMap.get(e.to) ?? e.to, })); - // Remap layout keys for top-level nodes; leave any unrelated keys as-is. + // Remap layout keys. Top-level node keys remap via idMap; namespaced foreach + // child keys remap via the owning group's idMap entry + its inner map; any + // unrelated keys pass through unchanged. const newLayout: Record = {}; for (const [key, pos] of Object.entries(layout)) { + const sepIdx = key.indexOf(FOREACH_CHILD_SEP); + if (sepIdx >= 0) { + const groupId = key.slice(0, sepIdx); + const innerId = key.slice(sepIdx + FOREACH_CHILD_SEP.length); + const newGroupId = idMap.get(groupId); + const innerMap = templateMaps.get(groupId); + const newInnerId = innerMap?.get(innerId); + if (newGroupId && newInnerId) { + newLayout[foreachChildFlowId(newGroupId, newInnerId)] = { ...pos }; + continue; + } + } const mapped = idMap.get(key); newLayout[mapped ?? key] = { ...pos }; } diff --git a/packages/dashboard/src/routes/__tests__/workflow-design-route.test.ts b/packages/dashboard/src/routes/__tests__/workflow-design-route.test.ts index db371b277e..e763820042 100644 --- a/packages/dashboard/src/routes/__tests__/workflow-design-route.test.ts +++ b/packages/dashboard/src/routes/__tests__/workflow-design-route.test.ts @@ -45,6 +45,26 @@ function makeFakeAgent(text: string) { return { factory, captured }; } +/** A fake agent whose prompt rejects; tracks whether dispose() was called so we + * can assert the route releases the session even when the model turn throws. */ +function makeRejectingAgent() { + const state = { disposed: false }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const factory: any = async () => { + const session = { + on(_event: "text", _listener: (delta: string) => void) {}, + async prompt() { + throw new Error("model turn failed"); + }, + dispose() { + state.disposed = true; + }, + }; + return { session }; + }; + return { factory, state }; +} + /** A minimal valid v1 linear IR (start → prompt → end). */ function linearIr(overrides?: { nodeConfig?: Record }): WorkflowIr { return { @@ -174,6 +194,17 @@ describe("POST /api/workflows/design (U7/R11/KTD-6)", () => { expect(await userDefCount()).toBe(before); }); + it("prompt rejects → 5xx but session is still disposed (no leak)", async () => { + const { factory, state } = makeRejectingAgent(); + __setCreateFnAgentForDesign(factory); + + const before = await userDefCount(); + const res = await postJson("/api/workflows/design", { prompt: "x" }); + expect(res.status).toBeGreaterThanOrEqual(400); + expect(state.disposed).toBe(true); + expect(await userDefCount()).toBe(before); + }); + it("JSON failing parseWorkflowIr (missing start) → 422 with parser message, nothing persisted", async () => { const noStart = { version: "v1", diff --git a/packages/dashboard/src/routes/register-workflow-routes.ts b/packages/dashboard/src/routes/register-workflow-routes.ts index 4fc4e0b205..2843359962 100644 --- a/packages/dashboard/src/routes/register-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-workflow-routes.ts @@ -52,6 +52,13 @@ const designRateLimits = new Map(); * when the 10/hour window is exhausted. Same shape as ai-refine.checkRateLimit. */ function checkDesignRateLimit(ip: string): boolean { const now = Date.now(); + // Prune expired-window entries so the map can't grow unbounded across many + // distinct IPs (each request triggers a cheap sweep of stale entries). + for (const [key, value] of designRateLimits) { + if (now - value.firstRequestAt > DESIGN_RATE_LIMIT_WINDOW_MS) { + designRateLimits.delete(key); + } + } const entry = designRateLimits.get(ip); if (!entry || now - entry.firstRequestAt > DESIGN_RATE_LIMIT_WINDOW_MS) { designRateLimits.set(ip, { count: 1, firstRequestAt: now }); @@ -688,8 +695,11 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void { const userPrompt = baseIrJson ? `Modify the following base workflow per the request below. Output the full updated WorkflowIr.\n\nBASE WORKFLOW IR:\n${baseIrJson}\n\nREQUEST:\n${prompt}` : `Design a workflow for the following request:\n\n${prompt}`; - await designSession.prompt(userPrompt); - designSession.dispose(); + try { + await designSession.prompt(userPrompt); + } finally { + designSession.dispose(); + } // Extract JSON (handles fences/prose) → JSON.parse → parseWorkflowIr. const candidate = extractJsonFromText(output); diff --git a/packages/i18n/src/config.ts b/packages/i18n/src/config.ts index a3d6a105c6..a67c597b51 100644 --- a/packages/i18n/src/config.ts +++ b/packages/i18n/src/config.ts @@ -86,5 +86,12 @@ export function baseInitOptions(): InitOptions { // React (and Ink) escape on render; double-escaping mangles output. interpolation: { escapeValue: false }, returnNull: false, + // Untranslated keys are backfilled with "" placeholders across the non-en + // catalogs (hundreds of them). With i18next's default returnEmptyString:true + // those render blank — even when a component passes an inline English default + // to t() — because an empty string is still a "found" value. Setting this + // false makes empty values fall through the fallback chain to en, which is + // the intended behavior for the placeholder convention. + returnEmptyString: false, }; }