From 9947052656559bcc6e39ac9d58415b6fe1db8c71 Mon Sep 17 00:00:00 2001 From: keeb Date: Mon, 30 Mar 2026 18:38:11 -0700 Subject: [PATCH] fix: append index/key to forEach step names when expression evaluation fails When CEL expression evaluation fails for forEach step names (e.g., referencing a missing property like `self.item.missingField`), all items produced the same unexpanded template string, causing duplicate names that triggered CyclicDependencyError in TopologicalSortService. The fix tracks evaluation failures during the replace callback and appends the loop index (array iteration) or object key (object iteration) to the expanded name as a uniqueness suffix. A warning is logged so the misconfiguration is visible to users. Fixes #975 Co-Authored-By: Claude Opus 4.6 (1M context) --- src/domain/workflows/execution_service.ts | 22 +++ .../workflows/execution_service_test.ts | 173 ++++++++++++++++++ 2 files changed, 195 insertions(+) diff --git a/src/domain/workflows/execution_service.ts b/src/domain/workflows/execution_service.ts index 93f886db..0b41475a 100644 --- a/src/domain/workflows/execution_service.ts +++ b/src/domain/workflows/execution_service.ts @@ -1421,6 +1421,7 @@ export class WorkflowExecutionService { // like ${{ self.ep.attributes.show }}-${{ self.ep.attributes.rawTitle }}) let expandedName = step.name; if (nameHasExpression) { + let hadEvalFailure = false; expandedName = step.name.replace( /\$\{\{\s*(.+?)\s*\}\}/g, (_match, expr) => { @@ -1429,10 +1430,20 @@ export class WorkflowExecutionService { celEvaluator.evaluate(expr as string, stepContext), ); } catch { + hadEvalFailure = true; return _match as string; } }, ); + if (hadEvalFailure) { + expandedName = `${expandedName}-${index}`; + getLogger(["swamp", "workflows"]).warn( + "forEach step '{stepName}' has expression(s) that failed to evaluate for item at index {index}. " + + "Appending index to prevent duplicate names. " + + "Check that the expression references valid properties on self.{itemName}.", + { stepName: step.name, index, itemName }, + ); + } } else { // Step name has no expression template — append item value for uniqueness if ( @@ -1473,6 +1484,7 @@ export class WorkflowExecutionService { // Evaluate ALL expressions in step name let expandedName = step.name; if (nameHasExpression) { + let hadEvalFailure = false; expandedName = step.name.replace( /\$\{\{\s*(.+?)\s*\}\}/g, (_match, expr) => { @@ -1481,10 +1493,20 @@ export class WorkflowExecutionService { celEvaluator.evaluate(expr as string, stepContext), ); } catch { + hadEvalFailure = true; return _match as string; } }, ); + if (hadEvalFailure) { + expandedName = `${expandedName}-${key}`; + getLogger(["swamp", "workflows"]).warn( + "forEach step '{stepName}' has expression(s) that failed to evaluate for key '{key}'. " + + "Appending key to prevent duplicate names. " + + "Check that the expression references valid properties on self.{itemName}.", + { stepName: step.name, key, itemName }, + ); + } } else { // Step name has no expression template — append key for uniqueness expandedName = `${step.name}-${key}`; diff --git a/src/domain/workflows/execution_service_test.ts b/src/domain/workflows/execution_service_test.ts index 5a6009af..57129220 100644 --- a/src/domain/workflows/execution_service_test.ts +++ b/src/domain/workflows/execution_service_test.ts @@ -2151,3 +2151,176 @@ Deno.test("expandForEachSteps: object iteration with multi-expression step name" assertEquals(new Set(stepNames).size, stepNames.length); }); }); + +// Regression test for issue #975: forEach expression evaluation failure should +// append index to prevent duplicate step names. +Deno.test("expandForEachSteps: appends index when array item expression evaluation fails", async () => { + await withTempDir(async (tempDir) => { + const workflowRepo = new InMemoryWorkflowRepository(); + const runRepo = new InMemoryWorkflowRunRepository(); + const executor = new MockStepExecutor(); + + // Create workflow where forEach iterates an array, and the step name + // references a property that doesn't exist on the items. + // Both items will fail expression evaluation, but the index suffix + // should prevent duplicate names. + const workflow = Workflow.create({ + name: "foreach-eval-fail-array", + jobs: [ + Job.create({ + name: "job1", + steps: [ + Step.create({ + name: "process-${{ self.item.missingField }}", + task: StepTask.model("test-model", "run"), + forEach: { + item: "item", + in: "${{ inputs.items }}", + }, + }), + ], + }), + ], + }); + await workflowRepo.save(workflow); + + const service = new WorkflowExecutionService( + workflowRepo, + runRepo, + tempDir, + executor, + ); + + const run = await service.execute(workflow.name, { + inputs: { items: [{ a: 1 }, { a: 2 }] }, + }); + + assertEquals(run.status, "succeeded"); + // Each expanded step should have a unique name with index suffix + assertEquals(executor.executedSteps.length, 2); + assertEquals( + executor.executedSteps.includes( + "job1/process-${{ self.item.missingField }}-0", + ), + true, + ); + assertEquals( + executor.executedSteps.includes( + "job1/process-${{ self.item.missingField }}-1", + ), + true, + ); + }); +}); + +// Regression test for issue #975: forEach expression evaluation failure over +// object iteration should append key to prevent duplicate step names. +Deno.test("expandForEachSteps: appends key when object item expression evaluation fails", async () => { + await withTempDir(async (tempDir) => { + const workflowRepo = new InMemoryWorkflowRepository(); + const runRepo = new InMemoryWorkflowRunRepository(); + const executor = new MockStepExecutor(); + + // Create workflow where forEach iterates an object, and the step name + // references a property that doesn't exist on the items. + const workflow = Workflow.create({ + name: "foreach-eval-fail-object", + jobs: [ + Job.create({ + name: "job1", + steps: [ + Step.create({ + name: "process-${{ self.entry.missingField }}", + task: StepTask.model("test-model", "run"), + forEach: { + item: "entry", + in: "${{ inputs.items }}", + }, + }), + ], + }), + ], + }); + await workflowRepo.save(workflow); + + const service = new WorkflowExecutionService( + workflowRepo, + runRepo, + tempDir, + executor, + ); + + const run = await service.execute(workflow.name, { + inputs: { items: { alpha: "one", beta: "two" } }, + }); + + assertEquals(run.status, "succeeded"); + // Each expanded step should have a unique name with key suffix + assertEquals(executor.executedSteps.length, 2); + assertEquals( + executor.executedSteps.includes( + "job1/process-${{ self.entry.missingField }}-alpha", + ), + true, + ); + assertEquals( + executor.executedSteps.includes( + "job1/process-${{ self.entry.missingField }}-beta", + ), + true, + ); + }); +}); + +// Verify that forEach with successful expression evaluation still works without +// appending index/key suffix. +Deno.test("expandForEachSteps: does not append index when expression evaluates successfully", async () => { + await withTempDir(async (tempDir) => { + const workflowRepo = new InMemoryWorkflowRepository(); + const runRepo = new InMemoryWorkflowRunRepository(); + const executor = new MockStepExecutor(); + + const workflow = Workflow.create({ + name: "foreach-eval-success", + jobs: [ + Job.create({ + name: "job1", + steps: [ + Step.create({ + name: "process-${{ self.item.name }}", + task: StepTask.model("test-model", "run"), + forEach: { + item: "item", + in: "${{ inputs.items }}", + }, + }), + ], + }), + ], + }); + await workflowRepo.save(workflow); + + const service = new WorkflowExecutionService( + workflowRepo, + runRepo, + tempDir, + executor, + ); + + const run = await service.execute(workflow.name, { + inputs: { items: [{ name: "foo" }, { name: "bar" }] }, + }); + + assertEquals(run.status, "succeeded"); + assertEquals(executor.executedSteps.length, 2); + // Names should be resolved without index suffix + assertEquals( + executor.executedSteps.includes("job1/process-foo"), + true, + ); + assertEquals( + executor.executedSteps.includes("job1/process-bar"), + true, + ); + }); +});