Review: fix: resolve ternary CEL expressions with partial inputs (#814)

No blocking issues found. The fix is clean, well-tested, and correctly addresses the regression.

What I verified

• Root cause & fix: The regex-based extractInputReferencesFromCel() pre-check was semantically incorrect for conditional expressions — it treated all inputs.* references as required regardless of branch. Replacing it with try-catch around celEvaluator.evaluate() is the right approach: CEL natively short-circuits ternaries, so only the taken branch needs its inputs.
• Both code paths fixed: The identical fix is applied in both ExpressionEvaluationService.evaluateDefinition() and DefaultStepExecutor.evaluateDefinitionExpressions(), keeping them in sync.
• Model dependency pre-check retained: The structural extractDependencies() check for model resource/file availability is correctly preserved — model data is never conditionally accessed, so the pre-check remains valid. The rename from hasMissing to hasMissingModelDep makes the intent clearer.
• #653 regression protection: The "directly-missing input stays unresolved" test explicitly verifies the original fix is preserved.
• Import boundary: No libswamp internal imports from CLI/presentation layers. Integration tests import from domain/infrastructure directly, which is correct for test code.
• License headers: All new/modified files have the AGPLv3 header.
• DDD: Changes are scoped to domain services (ExpressionEvaluationService, DefaultStepExecutor) — the right layer for expression evaluation logic. No domain model changes needed.
• Test coverage: Excellent — 6 files changed with tests at every layer: CEL evaluator unit tests, expression parser documentation test, expression evaluation service unit tests, and an end-to-end integration test.

Suggestions (non-blocking)

1. Bare catch swallows all CEL errors (expression_evaluation_service.ts:251, execution_service.ts:779): The catch blocks silently discard any InvalidExpressionError, not just missing-input errors. A CEL syntax error in a ${{ }} expression would be silently ignored and left as a raw string, only surfacing later when the Proxy throws. Consider catching InvalidExpressionError specifically and logging at debug level so expression authoring mistakes are easier to diagnose. This is a minor observability improvement, not a correctness issue — the Proxy still guards against using unresolved values.

2. extractInputReferencesFromCel is now unused outside tests and extractInputReferences: It's still exported and called internally by extractInputReferences() in expression_parser.ts:272, so it's not dead code, but it might be worth adding a doc comment noting it should not be used as a skip gate (the test documents this, but a comment on the function itself would help future contributors).

LGTM — approve.

Adversarial Review

Critical / High

None found.

Medium

1. Overly broad catch swallows all CEL errors, not just missing-input errors — src/domain/expressions/expression_evaluation_service.ts:251 and src/domain/workflows/execution_service.ts:779

   The empty catch {} catches every error from celEvaluator.evaluate(), including syntax errors, type mismatches, and other genuine bugs in CEL expressions — not just the missing-input case the comment describes.

   Breaking example: A user writes a typo in a CEL expression in methods.execute.arguments.run:

   run: '${{ "echo " + inputs.transprot }}'  # typo: transprot instead of transport

   Previously (before #655), this would throw InvalidExpressionError with a clear message like "No such key: transprot". Now it's silently swallowed, the raw ${{ ... }} string is passed to the shell, and the user sees bash: ${{: command not found or similar — a far less helpful error.

   For globalArguments, the Proxy safety net catches this with a reasonable message. But for methods.<name>.arguments values (the execution_service.ts path), there is no Proxy — the raw ${{ }} string flows directly to the execution driver. The comment on line 782 says "The Proxy on globalArgs will surface a clear error" but this is only true for globalArguments, not method arguments resolved in this same code path.

   Suggested fix: Catch only InvalidExpressionError where the cause message indicates a missing variable/key, and re-throw everything else. Alternatively, at minimum log a warning in the catch block so users have a breadcrumb when debugging expression issues. Something like:

   } catch (error) {
     if (error instanceof InvalidExpressionError && isMissingInputError(error)) {
       // Leave unresolved — CEL short-circuited past this input
       continue;
     }
     throw error;
   }

2. Model dependency pre-check has the same ternary problem this PR fixes for inputs — src/domain/expressions/expression_evaluation_service.ts:228-243 and src/domain/workflows/execution_service.ts:756-770

   The comment says "model data is never conditionally accessed in CEL — member access on a missing model ref is always an error." This is an assumption about user behavior, not enforced by the system. A user could write:

   ${{ inputs.use_cache ? model.cache-model.resource.value : "default" }}
   

   The extractDependencies regex would find model.cache-model.resource, see it missing, and skip the entire expression — the exact same class of bug this PR fixes for inputs.

   This is pre-existing behavior and not introduced by this PR, so it doesn't block merge. But worth noting since the PR specifically touches this code and adds a comment claiming it's always correct.

Low

1. Duplicated evaluation logic between two files — The expression evaluation loop in execution_service.ts:749-785 and expression_evaluation_service.ts:221-256 are near-identical. This was pre-existing duplication, but the PR required applying the same fix in both places. A future refactor to share this logic would prevent the two paths from diverging.

2. extractInputReferencesFromCel is now unused in production code but still exported — The import was removed from both expression_evaluation_service.ts and execution_service.ts. It's still used in tests (to document why it shouldn't be used as a skip gate), but if no production code calls it, it's dead code.

Verdict

PASS — The fix is logically sound and well-tested. CEL's native short-circuit evaluation is the correct approach to handle conditional expressions with partial inputs, and the regression tests are thorough. The broad catch (Medium #1) is a real concern for debuggability but not a correctness or safety issue — the Proxy provides a safety net for globalArguments, and for method arguments the expression would have been skipped by the old regex check in many of the same scenarios. The test coverage (unit + integration) gives confidence the fix works for both the new ternary case and the original #653 partial-inputs case.