Code Review

Blocking Issues

None.

Suggestions

1. Test assertions: The new tests use assertEquals(arr.includes(x), true) — consider using assert(arr.includes(x)) from @std/assert for slightly cleaner assertion messages on failure (the current form reports false !== true rather than showing the missing value). Non-blocking.

2. Code duplication: The array and object branches share nearly identical eval-failure logic (flag + suffix + warning). This is pre-existing duplication that the PR inherits — not something to address here, but worth noting for a future refactor if the forEach expansion logic grows further.

Overall: Clean, focused fix with good test coverage. The defensive fallback of appending index/key on eval failure is a sensible approach that prevents the CyclicDependencyError without masking the underlying misconfiguration (thanks to the warning log).

Adversarial Review

Critical / High

None.

Medium

1. Partial eval collision — theoretical but harmless (execution_service.ts:1439, 1502): If a step name has multiple expressions and only some fail, the expanded name is a hybrid like resolvedValue-${{ self.item.missing }}-0. The index/key suffix guarantees uniqueness within the loop, so this is correct. However, the unexpanded ${{ }} tokens remain in the name, which could look odd in logs/UI. This is a pre-existing behavior (the catch block already returned _match before this PR), so not introduced here — just noting it.

2. No test for partial expression failure (execution_service_test.ts): The tests cover the case where the only expression fails. There's no test for a step name like ${{ self.item.validField }}-${{ self.item.missingField }} where one expression succeeds and another fails. The code handles this correctly (the hadEvalFailure flag catches it), but a test would provide extra confidence.

Low

1. Redundant suffix when partial eval already produces unique names (execution_service.ts:1438-1439): If items are [{a: "x"}, {a: "y"}] and the step name is ${{ self.item.a }}-${{ self.item.missing }}, the a part already makes names unique (x-...-0 vs y-...-1). The index suffix is redundant but harmless — correctness over cleverness is the right call here.

Verdict

PASS — The fix is minimal, correct, and well-tested. It addresses the exact root cause (duplicate expanded names → cyclic dependency error) with a straightforward uniqueness suffix. Both the array and object iteration paths are covered symmetrically. No new failure modes introduced.

Code Review

Blocking Issues

None.

Suggestions

1. Minor test style: The tests use assertEquals(executor.executedSteps.includes("..."), true) — using a direct assert() or a helper like assertArrayIncludes would give clearer failure messages if a test ever breaks (e.g., showing what executedSteps actually contained). Non-blocking.

2. Pre-existing duplication: The expression-evaluation-and-expand logic is nearly identical between the array and object iteration branches (~30 lines each). This PR correctly applies the fix to both, but extracting a shared helper (e.g., expandStepNameExpressions(stepName, context, celEvaluator, suffix)) would reduce future maintenance surface. This is out of scope for this bug fix PR.

Overall: clean, focused fix with good regression tests. LGTM.

Adversarial Review

Critical / High

None found.

Medium

1. Partial expression failure with colliding successful evaluations (execution_service.ts:1438-1439, 1501-1502): If a step name has multiple expressions like ${{ self.item.a }}-${{ self.item.b }} and only b fails while a evaluates to the same value for different items, the unexpanded portion ${{ self.item.b }} is identical across items, and the expanded portion from a is also identical — producing duplicate names before the index suffix is applied. The fix does handle this correctly because hadEvalFailure is set to true and the index/key suffix is appended. However, if a succeeds with different values per item but b fails, the names are already unique without the suffix — the suffix is harmlessly redundant. This is a cosmetic over-correction, not a bug.

2. Successful evaluation producing duplicate names is not addressed (pre-existing): If expressions evaluate successfully but produce identical values (e.g., items [{name: "foo"}, {name: "foo"}] with step process-${{ self.item.name }}), both get process-foo with no index suffix. This would still trigger CyclicDependencyError. This is out of scope for this PR (no regression), but worth noting as a remaining gap.

Low

1. Code duplication between array and object branches (execution_service.ts:1424-1446 vs 1487-1509): The eval-failure logic is copy-pasted between the two branches. If the logic needs to change (e.g., to address Medium #2 above), both branches must be updated in lockstep. Not a correctness issue today.

Verdict

PASS — The fix is minimal, correctly targeted, and well-tested. The hadEvalFailure flag reliably detects when CEL evaluation throws, and appending the loop index (array) or object key (object) guarantees unique expanded names in the failure case. The three regression tests cover the key scenarios. No critical or high severity issues found.