Conversation
…execution (#653) The workflow execution path (evaluateDefinitionExpressions) was evaluating all globalArgument CEL expressions unconditionally, causing "No such key" errors when factory model steps provided only partial inputs. The CLI path (evaluateDefinition) already had selective skipping logic that checked whether referenced inputs were actually provided before evaluating. This adds the same two-phase check to the workflow path: 1. Skip expressions referencing inputs not in the caller's provided set 2. Skip expressions referencing model resource/file data not in context Skipped expressions stay as raw ${{ ... }} strings. The existing Proxy in method_execution_service.ts throws a clear error only if the method code actually tries to access an unresolved globalArgument at runtime. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Review Summary
This PR correctly fixes #653 by adding selective skipping of globalArgument expressions in the workflow execution path, aligning it with the existing CLI path behavior.
No Blocking Issues
The code is well-implemented and ready to merge.
Code Quality Assessment
Compliance with CLAUDE.md:
- TypeScript strict mode: No
anytypes - Named exports used consistently
- AGPLv3 license header present in the new test file
- Integration test correctly placed in
integration/directory
DDD Perspective:
The implementation correctly applies domain logic to skip expressions referencing unavailable context (missing inputs or model data). The two-phase check approach is sound.
Test Coverage:
The integration test (integration/workflow_partial_inputs_test.ts) thoroughly covers the primary use case - a workflow step providing only a subset of inputs when globalArguments reference multiple inputs. This tests the fix through the actual workflow execution path.
Security:
No security vulnerabilities introduced. The code safely handles missing context by skipping evaluation rather than exposing errors.
Suggestions (Non-blocking)
- Code Duplication: The selective skipping logic in
evaluateDefinitionExpressions()(lines 666-701) closely mirrors the logic inExpressionEvaluationService.evaluateDefinition()(lines 217-254). Consider extracting this into a shared utility function to prevent future drift between the CLI and workflow paths. However, since both paths now behave identically, this is a future improvement opportunity rather than a blocking concern.
Summary
The PR:
- Correctly fixes the bug where workflow steps failed with "No such key" errors when globalArguments referenced unprovided inputs
- Aligns workflow path behavior with the existing CLI path
- Includes a well-structured integration test
- Follows project code style and DDD principles
LGTM!
There was a problem hiding this comment.
Adversarial Review
I performed a systematic adversarial review attempting to break this code across logic, error handling, security, concurrency, and data integrity dimensions.
Critical / High
None found. The code correctly mirrors the CLI path implementation.
Medium
-
Bracket notation model references not detected by dependency extractor (execution_service.ts:683)
extractDependencies()uses regexmodel\.([a-zA-Z0-9_-]+)\.(input|resource|file|execution|definition)which matches dot notation only- If a user writes
${{ model["hyphenated-name"].resource }}directly (bracket notation), the dependency check won't detect it - Breaking example: Definition with
globalArguments.arg: '${{ model["vpc-main"].resource.net.id }}'where vpc-main hasn't run - expression is evaluated and CEL throws - Mitigation: This is pre-existing behavior (same regex used in CLI path). The more common pattern
model.vpc-main.resourceIS matched correctly (hyphens allowed in character class). - Impact: Low - unusual syntax, fails safely with CEL error
-
Only resource/file dependencies are checked (execution_service.ts:685)
extractDependencies()returns types including "data", "execution", "definition", but only "resource" and "file" trigger skip logic- If expression references
model.X.execution.*and that model hasn't run, the expression is evaluated anyway - Mitigation: Consistent with CLI path (lines 236-249 in expression_evaluation_service.ts have identical behavior). The execution context is typically set by prior workflow steps.
Low
-
Dynamic input key access not detected - Expression like
${{ inputs[someVariable] }}(non-literal key) wouldn't have its dependencies extracted due to regex-based parsing. Fails safely with CEL error if variable undefined. -
Test model schema declares both inputs required (workflow_partial_inputs_test.ts:102-103) - The test model has
required: ["instanceName", "cidrBlock"]but only provides instanceName. This works because:- Expression skipping prevents cidrBlock evaluation
- Method execution strips unresolved fields before Zod validation
- Proxy guards runtime access
This is intentional per the design rationale, just noting the interaction is non-obvious.
Verification
The implementation correctly:
- Extracts provided input keys from
context.inputs(lines 655-657) - Uses
extractInputReferencesFromCel()which handles both dot (inputs.field) and bracket (inputs["field"]) notation - Skips expressions with missing inputs before CEL evaluation
- Checks model resource/file availability in context
- Matches the CLI path logic exactly (compare lines 207-258 in expression_evaluation_service.ts)
The Proxy in method_execution_service.ts (lines 154-167) provides the safety net: if skipped expressions are accessed at runtime, a clear error is thrown.
Verdict
PASS - The implementation is correct and matches the existing CLI path behavior. No critical or high severity issues. The medium/low findings are pre-existing patterns, not regressions introduced by this PR.
PR #655 introduced a skip-guard in evaluateDefinitionExpressions() and evaluateDefinition() that used extractInputReferencesFromCel() — a regex scan — to detect all inputs.* references and skip evaluation if any were absent. This was intended to fix issue #653 (factory model delete steps failing with 'No such key: cidrBlock' when create-time inputs are omitted). The guard is too aggressive: for a ternary such as inputs.transport == "lan" ? inputs.lan_host : inputs.tailnet_host the regex reports all three inputs as required. When lan_host is absent (because transport is "wan"), the entire expression is skipped — leaving globalArguments.host as a raw ${{ }} string. The shell model then tries to execute that literal string, producing exit code 1 and a failed workflow step. CEL natively short-circuits ternaries, so missing branch inputs are never accessed at runtime. The correct fix is to attempt evaluation and catch any CEL error (which only fires for inputs referenced directly, not inside a non-taken branch). This preserves the #653 behaviour — a directly- missing input still throws at eval time and is caught, leaving the expression unresolved — while allowing ternary and other conditional expressions to evaluate correctly. Changes: - execution_service.ts: remove providedInputKeys / extractInputReferencesFromCel skip block; wrap celEvaluator.evaluate() in try-catch; rename hasMissing → hasMissingModelDep for the retained model-resource dependency check - expression_evaluation_service.ts: identical change for the CLI path - cel_evaluator_test.ts: two new tests proving CEL ternary short-circuit and direct-missing-input throw behaviour - expression_parser_test.ts: documents why extractInputReferencesFromCel must not be used as a skip gate (reports all ternary branch inputs) - expression_evaluation_service_test.ts: two model-agnostic unit tests for evaluateDefinition() with globalArguments ternary (primary regression proof) - workflow_partial_inputs_test.ts: end-to-end ternary test through the shell model execution path Fixes #814. Preserves #653 fix. Co-authored-by: Adam Stacoviak <[email protected]> Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Summary
Fixes #653.
The workflow execution path (
evaluateDefinitionExpressions()inexecution_service.ts) was evaluating all globalArgument CEL expressions unconditionally. When a factory model definesglobalArgumentsreferencing multipleinputsfields (e.g.,instanceName,routeTableId,natGatewayId) but a workflow step only provides a subset (e.g., justinstanceNamefor a delete operation), the CEL evaluator would fail with:The CLI path (
ExpressionEvaluationService.evaluateDefinition()) already had selective skipping logic that checks whether referenced inputs are actually provided before evaluating. This PR adds the same two-phase check to the workflow path:providedInputKeysset fromcontext.inputsand skips any expression whoseinputs.*references aren't in that setmodel.X.resourceormodel.X.filewhen that model data isn't in contextSkipped expressions stay as raw
${{ ... }}strings. The existing Proxy inmethod_execution_service.tsthrows a clear error only if the method code actually tries to access an unresolved globalArgument at runtime — this is the correct design because it means expressions are evaluated lazily and errors only surface when a value is genuinely needed.Design rationale
This selective skipping is the correct approach because:
deletetypically only needsinstanceName+identifier, not the full set of create-time parameters likecidrBlockorgatewayId.model method run) and the workflow path (workflow run) apply the same skipping logic, eliminating a confusing behavioral difference.Testing
Automated tests
integration/workflow_partial_inputs_test.ts— creates a factory model withinstanceName,cidrBlock, andgatewayIdinputs inglobalArguments, then runs a workflow providing onlyinstanceName. Verifies the workflow succeeds.Manual verification
Created a test repo in
/tmp/swamp-653-testwith a factory model (partial-input-test) that has three inputs (instanceName,cidrBlock,gatewayId) wired intoglobalArguments, and a workflow that only providesinstanceName.Old binary (on PATH) — fails:
{ "status": "failed", "error": "Invalid expression: No such key: cidrBlock\n\n> 1 | inputs.cidrBlock\n ^" }Fixed binary (compiled) — succeeds:
{ "status": "succeeded", "jobs": [{ "status": "succeeded", "steps": [{ "status": "succeeded" }] }] }Files changed
src/domain/workflows/execution_service.tsevaluateDefinitionExpressions()integration/workflow_partial_inputs_test.tsVerification
deno check— passesdeno lint— passesdeno fmt— passesdeno run test— 2773 passed, 0 faileddeno run compile— binary compiles successfully🤖 Generated with Claude Code