From 75867c4002d8b00e5ff975ceffd8749c6bc7f543 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 8 Jun 2026 00:19:50 -0700 Subject: [PATCH] fix(FN-0000): address workflow loop review feedback --- docs/workflow-steps.md | 2 +- .../src/__tests__/workflow-ir-loop.test.ts | 41 +++++++++++++++++++ packages/core/src/workflow-ir.ts | 33 +++++++++++---- .../app/components/WorkflowNodeEditor.tsx | 10 ++++- .../__tests__/workflow-flow-mapping.test.ts | 6 ++- .../app/components/nodes/node-summary.ts | 16 ++++++-- .../app/components/workflow-flow-mapping.ts | 7 +++- packages/engine/src/workflow-graph-loop.ts | 11 +++-- 8 files changed, 106 insertions(+), 20 deletions(-) diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 0c3f71c88f..a0e368094c 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -123,7 +123,7 @@ The **step-inversion** track makes task *steps* themselves workflow-modelable. T `loop` repeats an inline template subgraph until a configured output condition matches or a budget is exhausted. Config: -``` +```ts { template: { nodes, edges }, exitWhen: { type: "output-contains", value: string, nodeId?: string diff --git a/packages/core/src/__tests__/workflow-ir-loop.test.ts b/packages/core/src/__tests__/workflow-ir-loop.test.ts index 99b3f7cf37..07ad840d9a 100644 --- a/packages/core/src/__tests__/workflow-ir-loop.test.ts +++ b/packages/core/src/__tests__/workflow-ir-loop.test.ts @@ -85,6 +85,44 @@ describe("loop validation", () => { expect(() => parseWorkflowIr(loopIr({ template }))).toThrow(/nested loop\/foreach/); }); + it("rejects loop nodes inside foreach templates", () => { + const ir: WorkflowIrV2 = { + version: "v2", + name: "foreach-loop-test", + columns, + nodes: [ + { id: "start", kind: "start" }, + { + id: "steps", + kind: "foreach", + config: { + source: "task-steps", + template: { + nodes: [ + { + id: "nested-loop", + kind: "loop", + config: { + exitWhen: { type: "output-contains", value: "DONE" }, + template: loopTemplate(), + }, + }, + ], + edges: [], + }, + }, + }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "steps" }, + { from: "steps", to: "end" }, + ], + }; + + expect(() => parseWorkflowIr(ir)).toThrow(/nested loop\/foreach/); + }); + it("rejects foreach-only seams and normal cycles inside loop templates", () => { const seamTemplate = loopTemplate(); seamTemplate.nodes[0] = { id: "ask", kind: "prompt", config: { seam: "step-execute" } }; @@ -106,6 +144,9 @@ describe("loop validation", () => { expect(() => parseWorkflowIr(loopIr({ exitWhen: { type: "output-matches", pattern: "[" } }))).toThrow( /exitWhen.pattern is invalid/, ); + expect(() => + parseWorkflowIr(loopIr({ exitWhen: { type: "output-matches", pattern: "(a+)+" } })), + ).toThrow(/potentially unsafe/); }); it("clamps high maxIterations and rejects invalid budgets", () => { diff --git a/packages/core/src/workflow-ir.ts b/packages/core/src/workflow-ir.ts index 06172011d4..2dfac8714d 100644 --- a/packages/core/src/workflow-ir.ts +++ b/packages/core/src/workflow-ir.ts @@ -99,6 +99,9 @@ const MAX_FOREACH_CONCURRENCY = 8; const MAX_LOOP_ITERATIONS_CAP = 50; const MAX_LOOP_TIMEOUT_MS = 3_600_000; const WORKFLOW_EXTENSION_KEY_PATTERN = /^plugin:[a-z0-9]([a-z0-9-]*[a-z0-9])?:[a-z0-9]([a-z0-9-]*[a-z0-9])?$/; +const MAX_LOOP_REGEX_PATTERN_LENGTH = 256; +const LOOP_REGEX_NESTED_QUANTIFIER = /\((?:[^()\\]|\\.)*[*+](?:[^()\\]|\\.)*\)\s*(?:[*+]|\{\d+,?\d*\})/; +const LOOP_REGEX_BACKREFERENCE = /\\[1-9]/; /** The implicit step-source artifact allowed when no artifacts are declared. */ const IMPLICIT_DEFAULT_ARTIFACT = "PROMPT.md"; @@ -108,6 +111,19 @@ function isStepExecuteNode(node: WorkflowIrNode): boolean { return node.kind === "prompt" && node.config?.seam === "step-execute"; } +function assertSafeLoopRegexPattern(nodeId: string, pattern: string): void { + if (pattern.length > MAX_LOOP_REGEX_PATTERN_LENGTH) { + throw new WorkflowIrError( + `loop node '${nodeId}' exitWhen.pattern must be ${MAX_LOOP_REGEX_PATTERN_LENGTH} characters or fewer`, + ); + } + if (LOOP_REGEX_BACKREFERENCE.test(pattern) || LOOP_REGEX_NESTED_QUANTIFIER.test(pattern)) { + throw new WorkflowIrError( + `loop node '${nodeId}' exitWhen.pattern uses a potentially unsafe regex construct`, + ); + } +} + /** Default-workflow column ids in legacy enum order (KTD-1). */ export const DEFAULT_WORKFLOW_COLUMN_IDS = [ "triage", @@ -374,13 +390,13 @@ function validateForeach( ); } - // No nested foreach. Also: a template node's declared `column` must resolve to a + // No nested template groups. Also: a template node's declared `column` must resolve to a // top-level column id (column-agent plan KTD-1) — otherwise a dangling reference // is a silent no-binding no-op at runtime instead of a typed authoring error. for (const inner of templateNodes) { - if (inner.kind === "foreach") { + if (inner.kind === "foreach" || inner.kind === "loop") { throw new WorkflowIrError( - `foreach node '${node.id}' template may not contain a nested foreach ('${inner.id}')`, + `foreach node '${node.id}' template may not contain nested loop/foreach ('${inner.id}')`, ); } if (inner.column !== undefined && !columnIds.has(inner.column)) { @@ -505,6 +521,7 @@ function validateLoop( `loop node '${node.id}' exitWhen.pattern is invalid: ${err instanceof Error ? err.message : String(err)}`, ); } + assertSafeLoopRegexPattern(node.id, exitWhen.pattern); } else { throw new WorkflowIrError(`loop node '${node.id}' exitWhen.type must be output-contains or output-matches`); } @@ -1218,8 +1235,8 @@ function validateV2(ir: WorkflowIrV2): void { validateForeachDominance(ir.nodes, ir.edges, outgoing); } -/** Clamp foreach `maxReworkCycles` > cap down to the cap, in place, mirroring the - * maxRetries clamp posture (KTD-5). Reject-of-<1 happens in validation. */ +/** Clamp bounded workflow-node configs down to their caps, in place, mirroring + * the maxRetries clamp posture. Reject-of-<1 happens in validation. */ function clampForeachConfigs(ir: WorkflowIrV2): void { for (const node of ir.nodes) { if (node.kind === "loop") { @@ -1351,9 +1368,9 @@ export function serializeWorkflowIr(ir: WorkflowIr): string { /** * Strip the trust-escalating `cliSkipApproval`/`autoApprove` flags from every - * node config in an IR, recursing into foreach `config.template.nodes` at any - * nesting depth (foreach-in-foreach). Mutates the passed IR in place and returns - * it alongside a `stripped` flag indicating whether anything was removed. + * node config in an IR, recursing into template-group `config.template.nodes`. + * Mutates the passed IR in place and returns it alongside a `stripped` flag + * indicating whether anything was removed. * * These flags bypass the CLI first-run approval gate (see executor.ts). They are * legitimate only for workflows authored through the trusted dashboard editor / diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.tsx b/packages/dashboard/app/components/WorkflowNodeEditor.tsx index af06376071..443f2eac6c 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.tsx +++ b/packages/dashboard/app/components/WorkflowNodeEditor.tsx @@ -3207,12 +3207,18 @@ function InnerEditor({ nextType === "output-matches" ? String(current.pattern ?? current.value ?? "") : String(current.value ?? current.pattern ?? ""); + const nextExitWhen: Record = { + ...current, + type: nextType, + }; + delete nextExitWhen.pattern; + delete nextExitWhen.value; return { ...prev, exitWhen: nextType === "output-matches" - ? { type: nextType, pattern: text } - : { type: nextType, value: text }, + ? { ...nextExitWhen, pattern: text } + : { ...nextExitWhen, value: text }, }; }, }); 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 2525c9eae3..0f63f04adb 100644 --- a/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts +++ b/packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts @@ -493,11 +493,15 @@ describe("workflow-flow-mapping foreach + rework round-trip", () => { { from: "retry", to: "end" }, ], }; - const inserted = insertFragment([], [], parseWorkflowIr(fragment), { x: 10, y: 20 }); + const inserted = insertFragment([], [], parseWorkflowIr(fragment), { x: 10, y: 20 }, { + [foreachChildFlowId("retry", "try")]: { x: 86, y: 132 }, + }); const group = inserted.nodes.find((n) => n.data.kind === "loop"); + const child = inserted.nodes.find((n) => n.parentId === group?.id); expect(group).toBeTruthy(); expect(group?.type).toBe("loop"); + expect(child?.position).toEqual({ x: 86, y: 132 }); expect(inserted.nodes.filter((n) => n.parentId === group?.id)).toHaveLength(1); expect(inserted.edges).toHaveLength(0); }); diff --git a/packages/dashboard/app/components/nodes/node-summary.ts b/packages/dashboard/app/components/nodes/node-summary.ts index 2885a0c870..9b0f52ab0c 100644 --- a/packages/dashboard/app/components/nodes/node-summary.ts +++ b/packages/dashboard/app/components/nodes/node-summary.ts @@ -165,15 +165,23 @@ export function nodeConfigSummary( ? (() => { const condition = exitWhen as Record; const type = str(condition.type); - if (type === "output-matches") return `until matches /${str(condition.pattern)}/`; - if (type === "output-contains") return `until contains "${str(condition.value)}"`; + if (type === "output-matches") { + return t("workflowNodes.summaryLoopUntilMatches", "until matches /{{pattern}}/", { + pattern: str(condition.pattern), + }); + } + if (type === "output-contains") { + return t('workflowNodes.summaryLoopUntilContains', 'until contains "{{value}}"', { + value: str(condition.value), + }); + } return ""; })() : ""; const maxIterations = typeof config.maxIterations === "number" && Number.isFinite(config.maxIterations) - ? `${config.maxIterations}x` - : "3x"; + ? t("workflowNodes.summaryLoopIterations", "{{count}}x", { count: config.maxIterations }) + : t("workflowNodes.summaryLoopIterations", "{{count}}x", { count: 3 }); return exit ? `${exit} · ${maxIterations}` : maxIterations; } case "step-review": { diff --git a/packages/dashboard/app/components/workflow-flow-mapping.ts b/packages/dashboard/app/components/workflow-flow-mapping.ts index a8ac000631..63f1b18222 100644 --- a/packages/dashboard/app/components/workflow-flow-mapping.ts +++ b/packages/dashboard/app/components/workflow-flow-mapping.ts @@ -1005,10 +1005,15 @@ export function insertFragment( const groupKind = editorKind(node); template.nodes.forEach((inner, innerIdx) => { const innerKind = editorKind(inner); + const childPos = + layout?.[foreachChildFlowId(node.id, inner.id)] ?? { + x: FOREACH_CHILD_X + innerIdx * FOREACH_CHILD_STEP_X, + y: FOREACH_CHILD_Y, + }; childNodes.push({ id: foreachChildFlowId(id, inner.id), type: innerKind, - position: { x: FOREACH_CHILD_X + innerIdx * FOREACH_CHILD_STEP_X, y: FOREACH_CHILD_Y }, + position: childPos, parentId: id, extent: "parent", data: { kind: innerKind, label: nodeLabel(inner), config: { ...(inner.config ?? {}) } }, diff --git a/packages/engine/src/workflow-graph-loop.ts b/packages/engine/src/workflow-graph-loop.ts index 3ac8002ec5..e31bc1c1f7 100644 --- a/packages/engine/src/workflow-graph-loop.ts +++ b/packages/engine/src/workflow-graph-loop.ts @@ -11,6 +11,7 @@ const MAX_TIMEOUT_MS = 3_600_000; interface LoopConfig { template: { nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[] }; exitWhen: WorkflowLoopConfig["exitWhen"]; + exitRegex?: RegExp; maxIterations: number; timeoutMs: number; } @@ -49,9 +50,12 @@ function resolveLoopConfig(node: WorkflowIrNode): LoopConfig { typeof cfg.timeoutMs === "number" && Number.isFinite(cfg.timeoutMs) ? Math.max(1, Math.min(MAX_TIMEOUT_MS, Math.floor(cfg.timeoutMs))) : DEFAULT_TIMEOUT_MS; + const exitRegex = + cfg.exitWhen.type === "output-matches" ? new RegExp(cfg.exitWhen.pattern, cfg.exitWhen.flags) : undefined; return { template: cfg.template, exitWhen: cfg.exitWhen, + exitRegex, maxIterations, timeoutMs, }; @@ -87,12 +91,13 @@ function exitNodeId(nodes: WorkflowIrNode[], edges: WorkflowIrEdge[], loopId: st return exits[0].id; } -function matchesExit(condition: WorkflowLoopConfig["exitWhen"], value: unknown): boolean { +function matchesExit(config: LoopConfig, value: unknown): boolean { const text = typeof value === "string" ? value : value == null ? "" : String(value); + const condition = config.exitWhen; if (condition.type === "output-contains") { return text.includes(condition.value); } - return new RegExp(condition.pattern, condition.flags).test(text); + return (config.exitRegex ?? new RegExp(condition.pattern, condition.flags)).test(text); } function publishIterationContext( @@ -185,7 +190,7 @@ export async function runLoop( ...(finalValue !== undefined ? { value: String(finalValue) } : {}), }); publishIterationContext(env.context, iterationContext); - if (matchesExit(config.exitWhen, finalValue)) { + if (matchesExit(config, finalValue)) { env.context[`node:${loopNode.id}:loop`] = { iterations: iteration, exitReason: "matched",