GlobalArguments CEL expressions fail when factory inputs are omitted — selective evaluation not working

[stack72]

Summary

PR #652 documents that globalArgument expressions are selectively evaluated — inputs not provided should be skipped, with a runtime error only if the method code actually accesses an unresolved globalArgument. However, this doesn't work in practice. CEL expressions referencing unprovided inputs fail at expression evaluation time, before the method code runs.

Steps to Reproduce

1. Create a factory model with an inputs schema containing multiple fields:

name: prod-private-route
inputs:
  properties:
    instanceName:
      type: string
    routeTableId:
      type: string
    natGatewayId:
      type: string
  required:
    - instanceName
    - routeTableId
    - natGatewayId
globalArguments:
  name: ${{ inputs.instanceName }}
  RouteTableId: ${{ inputs.routeTableId }}
  DestinationCidrBlock: "0.0.0.0/0"
  NatGatewayId: ${{ inputs.natGatewayId }}

2. Call the delete method providing only instanceName and identifier (the only inputs the delete method implementation actually uses):

- name: delete-private-route-a
  task:
    type: model_method
    modelIdOrName: prod-private-route
    methodName: delete
    inputs:
      instanceName: private-route-a
      identifier: ${{ data.latest("prod-private-route", "private-route-a").attributes.RouteTableId + "|" + ... }}

3. Expected: routeTableId and natGatewayId globalArgument expressions are skipped since the delete method never accesses context.globalArgs.RouteTableId or context.globalArgs.NatGatewayId.

4. Actual: Fails immediately with:

Invalid expression: No such key: routeTableId

>    1 | inputs.routeTableId
                ^

Impact

This means factory model delete steps must provide all inputs defined in the model's inputs schema, even when the delete method only needs instanceName (for data keying) and identifier (for the AWS API call). This creates unnecessary coupling — delete workflows must duplicate all create-time parameters.

Expected Behavior

GlobalArgument expressions referencing unprovided inputs should be:

1. Skipped during evaluation (not resolved)
2. Only cause a runtime error if the method code accesses the unresolved value via context.globalArgs

This would allow delete workflow steps to provide only instanceName + identifier, making them simpler and less coupled to create-time configuration.

Workaround

Provide all inputs to every delete step, pulling values from stored data or hardcoding create-time values:

inputs:
  instanceName: private-route-a
  routeTableId: ${{ data.latest("prod-private-route", "private-route-a").attributes.RouteTableId }}
  natGatewayId: ${{ data.latest("prod-private-route", "private-route-a").attributes.NatGatewayId }}
  identifier: ${{ ... }}

Environment

Discovered while testing a production VPC workflow with 10 factory models and 21 AWS resources. The documentation in PR #652 describes the selective evaluation behavior, but the runtime doesn't implement it.

[stack72]

Root Cause Analysis

The bug is a missing code path — selective evaluation of globalArgument expressions works in the CLI path but not the workflow path.

Two evaluation paths exist:

1. CLI path (swamp model method run → ExpressionEvaluationService.evaluateDefinition() in src/domain/expressions/expression_evaluation_service.ts:174-269)
   — HAS selective skipping logic (lines 217-254) that checks extractInputReferencesFromCel() against providedInputKeys and skips expressions with missing inputs.

2. Workflow path (workflow step execution → DefaultStepExecutor.evaluateDefinitionExpressions() in src/domain/workflows/execution_service.ts:639-673)
   — LACKS selective skipping. Calls celEvaluator.evaluate() on ALL expressions unconditionally, causing "No such key" when an expression references an input that wasn't provided.

Why it fails:

When a workflow step calls a factory model's delete method with only instanceName + identifier:

1. Step inputs are evaluated and merged into context.inputs (line 371)
2. evaluateDefinitionExpressions() iterates ALL expressions in the model definition
3. ${{ inputs.routeTableId }} is passed to the CEL evaluator
4. CEL evaluator looks up routeTableId in context.inputs, doesn't find it, throws "No such key"
5. The method never runs

Why the CLI path works:

evaluateDefinition() builds providedInputKeys = new Set(Object.keys(inputValues)) and for each expression calls extractInputReferencesFromCel() to check if all referenced inputs are available. Missing-input expressions are skipped — they remain as raw ${{ ... }} strings. The existing Proxy in method_execution_service.ts (lines 154-167) catches any runtime access to unresolved globalArguments, so the method only fails if it actually tries to use a skipped value.

Proposed Fix

Port the selective skipping logic from evaluateDefinition() into evaluateDefinitionExpressions():

1. Add imports to execution_service.ts:

   • extractInputReferencesFromCel from expression_parser.ts
   • extractDependencies from dependency_extractor.ts

2. Add ~20 lines inside evaluateDefinitionExpressions() before celEvaluator.evaluate():

   • Build providedInputKeys from context.inputs
   • Skip expressions referencing missing inputs (via extractInputReferencesFromCel)
   • Skip expressions referencing unavailable cross-model resource/file data (via extractDependencies)

3. Add integration test for partial-input workflow execution

Safety net already exists:

The Proxy wrapper in method_execution_service.ts already guards against accessing unresolved globalArguments at runtime. Skipped expressions stay as raw ${{ ... }} strings — the Proxy throws a clear error only if the method code actually tries to read them.

User Impact

Before fix: Factory model delete workflows must provide ALL inputs defined in the model's inputs schema, even those the delete method never uses. This creates unnecessary coupling — delete steps must duplicate create-time parameters via data.latest() lookups.

After fix: Delete workflow steps only need to provide the inputs that the method actually accesses (typically instanceName for data keying + identifier for the API call). Simpler delete workflows with less boilerplate.

No breaking changes. Existing workflows that provide all inputs continue to work identically. The fix only enables workflows that previously failed with partial inputs.

[stack72]

Why This Is the Right Fix

Why two evaluation paths exist (not a design choice — an oversight)

evaluateDefinitionExpressions() exists as a separate private method in the workflow executor because the workflow path has different context setup needs:

• Adds forEach variables to self (for ${{ self.env }} in forEach steps)
• Pre-builds the expression context with step outputs from upstream jobs
• Merges step inputs before evaluation

It can't simply delegate to ExpressionEvaluationService.evaluateDefinition() because that method would overwrite self and lose the forEach variables.

The selective skipping logic was added to evaluateDefinition() over time:

• Input skipping (extractInputReferencesFromCel + providedInputKeys) — designed specifically for factory models. The comment literally says "e.g., delete doesn't need create-time inputs".
• Cross-model resource skipping (extractDependencies) — added in commit df22793 (March 7) for issue Update method fails when globalArguments contain cross-model CEL expressions #641.

Neither was propagated to evaluateDefinitionExpressions(). The two paths diverged as features were added to one but not the other.

Why "port the logic" vs alternatives

Approach	Pros	Cons
Port the skipping logic (chosen)	Minimal change, proven logic, no structural risk	Duplicates ~20 lines between paths
Reuse evaluateDefinition()	Eliminates duplication	Would overwrite self (losing forEach vars), requires restructuring workflow context setup
Extract shared helper	DRYest	More refactoring than a bug fix warrants

Porting is the right scope for a bug fix. A future refactoring could extract a shared evaluation loop if desired.

Implementation Plan

1. src/domain/workflows/execution_service.ts

Add imports (extend existing import blocks):

// Add to line 46-50 (expression_parser.ts import)
import {
  extractExpressions,
  extractInputReferencesFromCel,  // ← new
  isTaskInputsPath,
  replaceExpressions,
} from "../expressions/expression_parser.ts";

// Add to line 55-57 (dependency_extractor.ts import)
import {
  extractDependencies,  // ← new
  hasStepOutputDependency,
} from "../expressions/dependency_extractor.ts";

Add selective skipping in evaluateDefinitionExpressions() (lines 652-661):

Before calling celEvaluator.evaluate(), add the same two checks from evaluateDefinition():

// Before the loop
const providedInputKeys = context.inputs
  ? new Set(Object.keys(context.inputs))
  : new Set<string>();

// Inside the loop, after the runtime expression check:

// Skip expressions referencing inputs not provided by the caller
const inputRefs = extractInputReferencesFromCel(expr.celExpression);
let hasMissing = false;
if (inputRefs.size > 0) {
  for (const ref of inputRefs) {
    if (!providedInputKeys.has(ref)) {
      hasMissing = true;
      break;
    }
  }
}

// Skip expressions referencing unavailable cross-model resource/file data
if (!hasMissing) {
  const deps = extractDependencies(expr.celExpression);
  for (const dep of deps) {
    if (dep.type === "resource" || dep.type === "file") {
      const modelData = context.model[dep.modelRef];
      if (
        !modelData ||
        (dep.type === "resource" && !modelData.resource) ||
        (dep.type === "file" && !modelData.file)
      ) {
        hasMissing = true;
        break;
      }
    }
  }
}

if (hasMissing) {
  continue;
}

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.

2. Integration test

Add a test (modeled on keeb_shell_model_test.ts:416-479) that:

1. Creates a factory model with globalArguments referencing multiple inputs
2. Creates a workflow that calls the model with only partial inputs
3. Verifies the method executes successfully

User Impact

Before: Factory delete workflows must provide ALL inputs — instanceName, cidrBlock, availabilityZone, identifier — even though the delete method only needs instanceName + identifier.

After: Delete steps only need inputs the method actually accesses. Simpler workflows, less boilerplate.

No breaking changes. Existing workflows providing all inputs work identically.