fix: detect async CEL functions in forEach.in and throw actionable error

[stack72]

Summary

• forEach.in uses sync CelEvaluator.evaluate, but async CEL functions (data.latest, data.findByTag, data.findBySpec, data.query) return Promises that coerceBigInts silently converts to {}, causing forEach to expand zero steps with no error — a silent no-op
• Adds Promise detection in CelEvaluator.evaluate (before coerceBigInts destroys the Promise) that throws InvalidExpressionError with a clear message
• Wraps that error in expandForEachSteps and libswamp/evaluate.ts with a UserError pointing to the nested workflow workaround documented in docs(skills/swamp-workflow): document when to use nested workflows #1165
• Updates design/expressions.md to reflect the async limitation of data.findBySpec() in forEach.in

Before: workflow silently "succeeds" with 0 steps executed
After:

forEach.in expression '${{ data.findBySpec(...) }}' returned an unresolved Promise.

forEach.in is evaluated synchronously and cannot await async CEL functions
(data.latest, data.findByTag, data.findBySpec, data.query).

Fix: move the async call into a parent workflow's task.inputs (which IS awaited)
and have the child iterate over inputs.<name>. See:
.claude/skills/swamp-workflow/references/nested-workflows.md#when-to-use-nested-workflows

Closes swamp-club #88

Test Plan

• Unit tests in cel_evaluator_test.ts: Promise detection for data.latest, data.findBySpec, data.findByTag; sync data functions still work
• Unit test in execution_service_test.ts: expandForEachSteps throws UserError with actionable message when forEach.in uses async data function
• End-to-end verified: compiled binary, created scratch repo, ran workflow with data.findBySpec in forEach.in — confirmed clear error output
• All existing tests pass (4 pre-existing failures in extension_push_test.ts unrelated)

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

1. src/libswamp/workflows/evaluate.ts:248 — plain Error instead of UserError causes stack trace on swamp workflow evaluate

   The forEach async-detection error in evaluate.ts throws new Error(...) while the identical error in execution_service.ts correctly throws new UserError(...). This matters because the top-level renderError in main.ts distinguishes between the two:

   • UserError → logger.fatal("Error: {message}", ...) — clean, message only
   • plain Error → logger.fatal("{error}", { error: err }) — includes full stack trace

   When a user runs swamp workflow evaluate with an async CEL function in forEach.in, they'll receive the actionable message plus a noisy stack trace. The workflow run path avoids this because its catch block at workflow_run.ts:290-294 re-wraps the error in UserError, but workflow_evaluate.ts has no such re-wrap.

   Fix: Change throw new Error( at line 248 of src/libswamp/workflows/evaluate.ts to throw new UserError(, and add the missing import: import { UserError } from "../../domain/errors.ts". The evaluate.ts error message is clear and actionable — it just needs to be surfaced cleanly without a stack trace.

Suggestions

1. workflow_run.ts:294 — "Workflow execution failed: " prefix is redundant with the new error message. The error message already says forEach.in expression '...' returned an unresolved Promise — prefixing it with "Workflow execution failed:" doesn't add context. No change needed now (it's pre-existing catch-all behavior), but worth noting for future cleanup.

Verdict

NEEDS CHANGES — evaluate.ts throws new Error instead of new UserError, causing a stack trace to appear on swamp workflow evaluate for what is a clean, user-actionable error condition.

[github-actions]

Code Review

Blocking Issues

1. Overbroad InvalidExpressionError catch swallows non-Promise errors (src/domain/workflows/execution_service.ts:1557-1572, src/libswamp/workflows/evaluate.ts:246-261)

   Both catch blocks catch all InvalidExpressionError instances and unconditionally replace the message with "returned an unresolved Promise". However, CelEvaluator.evaluate() wraps every error (syntax errors, undefined variables, type mismatches, etc.) as InvalidExpressionError in its generic catch block (cel_evaluator.ts:378-384). This means any CEL evaluation error in a forEach.in expression would be misleadingly reported as an "unresolved Promise" error.

   For example, ${{ data.findBySpce("model", "spec") }} (a typo) would produce:

   forEach.in expression '...' returned an unresolved Promise.
   

   instead of the actual "function not found" / evaluation error.

   Fix: Discriminate based on the error message content:

   if (error instanceof InvalidExpressionError && error.message.includes("unresolved Promise")) {

   This works because CelEvaluator.evaluate() re-wraps the InvalidExpressionError from the Promise check, preserving the "unresolved Promise" substring in the message.

Suggestions

1. Double-wrapping of InvalidExpressionError in cel_evaluator.ts: The Promise detection throws InvalidExpressionError at line 364 inside the try block, which is then caught by the generic catch at line 378 and re-wrapped as another InvalidExpressionError. The resulting message is "Invalid expression: Invalid expression: Expression returned...". While the callers replace the message entirely, the cel_evaluator_test.ts tests assert on this double-prefixed message. Consider either: (a) checking for InvalidExpressionError in the catch and re-throwing it directly, or (b) moving the Promise check after the try/catch.

2. Inconsistent error type in evaluate.ts: The libswamp catch throws new Error(...) while execution_service.ts throws new UserError(...). Since evaluate.ts already imports from the domain layer (InvalidExpressionError), consider using UserError for consistency — it suppresses the stack trace for expected user-facing errors.

3. No unit test for the forEach Promise path in evaluate_test.ts: The convention is "unit tests live next to source files". The evaluate.ts module has its own parallel implementation of the Promise catch-and-wrap logic, but the test for this path only exists in execution_service_test.ts. A unit test in evaluate_test.ts would catch regressions if these two implementations drift apart.

[github-actions]

Adversarial Review

Critical / High

1. Over-broad catch replaces ALL forEach.in errors with misleading "unresolved Promise" message — execution_service.ts:1557-1572 and evaluate.ts:246-262

   Both catch blocks catch InvalidExpressionError and unconditionally throw a "returned an unresolved Promise" error. However, CelEvaluator.evaluate() wraps all errors as InvalidExpressionError via the catch-all at cel_evaluator.ts:378-385. This means any CEL evaluation failure in a forEach.in expression — syntax errors, missing variables, wrong argument counts — will produce the misleading Promise error instead of the real one.

   Breaking example: A user writes forEach.in: '${{ undefinedVar.field }}'. The CEL evaluator throws because undefinedVar is not in context → wrapped as InvalidExpressionError → caught by the new catch block → user sees "forEach.in expression returned an unresolved Promise" and guidance about async functions, when the actual problem is a missing variable.

   Fix: Check whether the caught error is specifically about Promises before replacing the message. For example:

   } catch (error) {
     if (
       error instanceof InvalidExpressionError &&
       error.message.includes("unresolved Promise")
     ) {
       throw new UserError(/* async-specific message */);
     }
     throw error;
   }

   Alternatively, throw the Promise-specific InvalidExpressionError outside the try/catch in cel_evaluator.ts (see finding #2) so it doesn't get double-wrapped, then use a distinct subclass or a flag to discriminate it.

Medium

2. Double-wrapping of InvalidExpressionError in cel_evaluator.ts:364→378 — The Promise detection at line 364 throws InvalidExpressionError inside the try block. The catch at line 378 catches it and wraps it in another InvalidExpressionError. Since InvalidExpressionError's constructor prepends "Invalid expression: ", the resulting message is "Invalid expression: Invalid expression: Expression returned an unresolved Promise...". The callers construct their own message so this doesn't directly hit users, but it would confuse anyone debugging.

   Fix: Move the Promise check and throw after the try/catch, or add if (error instanceof InvalidExpressionError) throw error; at the top of the catch block.

3. evaluate.ts:248 throws plain Error instead of UserError — The PR description says both sites throw UserError, but evaluate.ts throws new Error(...) while execution_service.ts correctly throws new UserError(...). Per domain/errors.ts, UserError suppresses stack traces for expected user-facing errors. Users hitting this code path through swamp workflow evaluate will see an unnecessary stack trace.

Low

4. No test for data.query Promise detection — The unit tests in cel_evaluator_test.ts cover data.latest, data.findBySpec, and data.findByTag, but not data.query. The detection logic at cel_evaluator.ts:357 handles all four correctly via instanceof Promise, so this is just a coverage gap, not a code bug.

Verdict

FAIL — Finding #1 is a logic error that turns all forEach.in CEL failures into misleading "unresolved Promise" messages, which is worse than the pre-PR behavior (where the real error propagated). The fix is straightforward: narrow the catch to only match Promise-specific errors.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. Swallowed async errors in forEach task input resolution (src/libswamp/workflows/evaluate.ts:386-393): Inside resolveForEachTaskExpressions, the sync deps.evaluateCel() call is wrapped in a bare catch {} that silently discards all errors. If a user places data.findBySpec() in a task.inputs expression within a forEach step (rather than in forEach.in), the new Promise detection will throw InvalidExpressionError, which gets silently caught. The input value is left as the raw ${{ ... }} string, which will then fail later with a confusing error (or worse, be passed through as a literal string to the model method). Same pattern at line 408 for shell args. This isn't introduced by this PR — it's pre-existing — but the PR makes it more relevant since users hitting this error will be guided to move async calls into task.inputs, yet the evaluate (dry-run) path would silently eat the same function there. The execution path handles it correctly since it uses evaluateAsync.

2. String-based error dispatch (src/domain/workflows/execution_service.ts:1558-1560, src/libswamp/workflows/evaluate.ts:248-250): The catch blocks identify the Promise case via error.message.includes("unresolved Promise"). This is fragile — if the InvalidExpressionError constructor or the message text in cel_evaluator.ts changes, these catches silently stop matching and the error falls through to the generic throw error path, surfacing as an unhelpful InvalidExpressionError instead of a UserError. A dedicated error subclass (e.g., AsyncInSyncContextError) or a discriminant property would be more robust. Not a correctness issue today, but a maintenance hazard.

3. Exact duplication of the UserError message across execution_service.ts:1562-1573 and evaluate.ts:252-263: These are identical 12-line blocks. If the guidance or the workaround path changes, one will be updated and the other forgotten. Consider extracting to a shared helper.

Low

1. result.catch(() => {}) swallows all rejections (src/infrastructure/cel/cel_evaluator.ts:362): The empty catch handler is intentional — the comment explains it. However, if the underlying async operation fails with a meaningful error (e.g., database connection failure), that error is silently discarded. In the current context this is fine (we're about to throw anyway and the caller doesn't care about the async result), but it means diagnostic information is lost. A result.catch((e) => { getLogger(["swamp", "cel"]).debug("suppressed dangling promise", { error: e }); }) would preserve observability without changing behavior.

2. Test sanitizers disabled (src/domain/workflows/execution_service_test.ts:2699-2700): sanitizeResources: false and sanitizeOps: false disable Deno's leak detection for this test. The comment explains why (CatalogStore WAL cleanup is interrupted by the error path), but this could mask real resource leaks introduced by future changes to this code path. Not blocking, but worth noting.

Verdict

PASS — The core fix is sound: detecting Promises before coerceBigInts destroys them and throwing a clear, actionable error is the right approach. The InvalidExpressionError re-throw guard in the catch block correctly prevents double-wrapping. Tests cover the key scenarios. The medium findings are maintenance concerns, not correctness bugs.

[github-actions]

Code Review

Well-structured fix for a subtle and nasty silent-failure bug. The three-layer approach (detect in infrastructure, translate in domain/libswamp services, document in design) is clean and follows DDD principles correctly.

Blocking Issues

None.

Suggestions

1. String-matching for error dispatch is fragile: Both execution_service.ts and evaluate.ts check error.message.includes("unresolved Promise") to identify the specific InvalidExpressionError. If the message text in cel_evaluator.ts is ever reworded, these catch blocks silently stop matching. Consider adding a discriminant property (e.g., errorCode: "ASYNC_IN_SYNC_CONTEXT") to InvalidExpressionError for a more robust check. Not urgent since both throw and catch sites are co-located in this codebase.

2. Missing data.query Promise test: The CEL evaluator tests cover data.latest, data.findBySpec, and data.findByTag returning Promises, but data.query (also listed in the error message as an async function) has no corresponding test. Adding one would complete the coverage.

3. No libswamp evaluate.ts forEach Promise test: The new catch-and-wrap in src/libswamp/workflows/evaluate.ts (the workflowEvaluate code path) isn't directly tested — only the execution_service.ts path has an integration test. The core detection in CelEvaluator is well-tested, so this is low risk, but a parallel test in evaluate_test.ts would fully close the gap.

DDD Assessment

• Error placement is correct: InvalidExpressionError lives in the domain expressions layer, UserError in the domain errors module.
• Promise detection at the infrastructure layer (CEL evaluator) with translation to user-facing errors at the service layer follows the right responsibility boundaries.
• The libswamp evaluate.ts imports from internal domain paths, which is permitted for libswamp-internal code per CLAUDE.md.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Error message reference path — Both execution_service.ts and libswamp/workflows/evaluate.ts end the error with:

   See:
   .claude/skills/swamp-workflow/references/nested-workflows.md#when-to-use-nested-workflows
   

   This path points to an internal Claude Code skill file. Users who installed swamp as a compiled binary won't have this directory at all, and even source-checkout users would find it unusual to navigate into .claude/skills/. The actionable fix is already in the error body itself (move the async call into a parent workflow's task.inputs), so this is supplementary — but a GitHub URL to the relevant docs or a swamp docs link would serve binary users better.

2. Duplicated error string — The UserError message is written verbatim in two places (execution_service.ts:1560 and libswamp/workflows/evaluate.ts:250). If the wording or the reference path ever changes, both must be updated in sync. Not a UX issue today, but worth noting.

Verdict

PASS — This is a clear UX improvement. The previous behavior silently produced 0 steps with no error; now users get an explicit, actionable message that names the bad expression, explains why it fails, and describes the fix. The UserError pattern propagates correctly through the workflow_run handler.