fix: skip unresolvable cross-model resource expressions in globalArguments

[stack72]

Summary

Fixes #641. When a model's globalArguments contain CEL expressions referencing another model's resource or file data (e.g., ${{ model.ssh-key.resource.state.key.attributes.id }}), method execution crashes with No such key: resource if the referenced model has no data on disk. This prevents methods like update from running even when they don't need the unresolvable field.

What Changed

1. Expression evaluation skips unresolvable resource/file references

In ExpressionEvaluationService.evaluateDefinition(), expressions referencing model.*.resource or model.*.file are now skipped when the context proves the data doesn't exist (the referenced model was never executed or its data was cleaned up). This extends the existing pattern that already skips unresolved inputs.* references.

The raw ${{ ... }} expression is preserved in the definition — it is not silently discarded.

2. Broadened unresolved-expression detection in globalArgs validation

In DefaultMethodExecutionService.executeWorkflow(), the check for unresolved globalArgs fields now catches any remaining ${{ ... }} expression, not just inputs.* ones. This is safe because vault/env expressions are always resolved before executeWorkflow() is called (both in the CLI path via model_method_run.ts:282 and the workflow path via execution_service.ts:402).

3. Fixed pre-existing silent data corruption bug in globalArgs Proxy

The Proxy on context.globalArgs previously only threw for unresolved inputs.* expressions. If a globalArg contained an unresolved model.*.resource expression, the Proxy would silently return the raw ${{ ... }} string to the method. A model method could unknowingly send that literal string to an API. The Proxy now throws for any unresolved expression:

Unresolved expression in globalArguments.ssh_keys: ${{ model["ssh-key"].resource.state... }}

Design Rationale

Follows an established pattern. The codebase already skips unresolved inputs.* expressions in evaluateDefinition() (lines 214-229). The model.*.resource case is structurally identical — an expression references context data that doesn't exist yet. Extending the same skip logic is a natural, consistent extension.

The safety net already exists. The Proxy on context.globalArgs catches any attempt to use an unresolved expression at runtime. The flow is:

1. Expression can't be resolved → skip it, leave the raw ${{ ... }} in the definition
2. If the method actually needs that field → Proxy throws a clear error
3. If the method doesn't need that field → everything works fine

buildContext() already loads data from disk. When dataRepo is provided (which it is in both CLI and workflow paths), ModelResolver.buildContext() eagerly populates model.*.resource from on-disk data. If the data exists, the expression resolves normally. The skip only triggers when data genuinely doesn't exist.

Alternative approaches are worse:

• "Only evaluate fields the method schema needs" — requires the evaluation service to understand method schemas, a layer violation
• "Make model.*.resource persistently available" — already happens; the problem is the referenced model has no data at all
• "Allow data.latest() in definitions" — pushes complexity to users

User Impact

• No breaking changes. Every expression that resolved before still resolves. The skip logic only triggers when the CEL evaluation would have crashed anyway.
• What was broken now works. Models with cross-model resource references in globalArguments can run methods that don't need those fields (e.g., update doesn't need ssh_keys).
• Better error messages. If a method accesses an unresolved field, the error is now Unresolved expression in globalArguments.ssh_keys: ${{ ... }} instead of the cryptic No such key: resource.
• Fixes silent corruption. Unresolved non-input expressions can no longer silently leak through the Proxy as raw strings.

Files Changed

File	Change
src/domain/expressions/expression_evaluation_service.ts	Skip expressions with unresolvable model.*.resource/model.*.file deps
src/domain/models/method_execution_service.ts	Broaden globalArgs unresolved detection + fix Proxy to catch all expressions
src/domain/models/method_execution_service_test.ts	3 new unit tests for Proxy and globalArgs validation behavior
integration/keeb_shell_model_test.ts	Integration test: model method run with unresolvable cross-model resource ref
.claude/skills/swamp-model/references/troubleshooting.md	Document new error message and updated behavior

Testing

• All 2769 existing tests pass
• 3 new unit tests covering Proxy throws, Proxy allows resolved fields, and executeWorkflow skips validation
• 1 new integration test verifying standalone method run succeeds with unresolvable cross-model resource expression
• Manually verified against reproduction case in /tmp/swamp-641-test

[github-actions]

Review Summary

This is a well-designed fix that follows established patterns in the codebase. No blocking issues found.

Code Quality ✅

• TypeScript strict mode compliance: No any types used
• Named exports: Used throughout
• Code follows existing patterns (extends the input-skip logic already present)

Domain-Driven Design ✅

• Changes are properly contained within domain services (ExpressionEvaluationService, DefaultMethodExecutionService)
• The Proxy pattern for lazy validation is a sound design choice - methods that don't need certain fields succeed, while accessing unresolved fields fails fast with clear errors
• The extension of skip logic for model.*.resource/model.*.file mirrors the existing inputs.* pattern

Test Coverage ✅

• 3 new unit tests in method_execution_service_test.ts:
  • Proxy throws for unresolved expressions
  • Proxy allows access to resolved fields
  • executeWorkflow skips validation for unresolved resource expressions
• 1 new integration test verifying end-to-end behavior with unresolvable cross-model resource expressions
• Integration test properly uses dot notation (model.source-shell.resource...) which matches the extractDependencies regex

Security ✅

• Fixes a silent data corruption bug where unresolved non-input expressions could leak through the Proxy as raw strings
• No new security vulnerabilities introduced

Documentation ✅

• Troubleshooting documentation updated with new error message and resolution steps

---

Suggestions (non-blocking)

1. Consider a unit test for evaluateDefinition(): While the integration test covers the code path, a direct unit test in expression_evaluation_service_test.ts for skipping unresolvable resource references would make the behavior more explicit and isolated.

2. Bracket notation limitation: The extractDependencies regex only matches dot notation (model.name.resource) not bracket notation (model["name"].resource). This is a pre-existing limitation and not introduced by this PR - the fix is still an improvement over the previous state.

---

LGTM! The fix is well-reasoned, well-tested, and follows the established patterns in the codebase.

🤖 Generated with Claude Code

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. extractDependencies doesn't handle bracket notation - src/domain/expressions/dependency_extractor.ts:54-55

   The MODEL_REF_PATTERN regex only matches dot notation:

   /model\.([a-zA-Z0-9_-]+)\.(input|resource|file|execution|definition)/g

   What breaks: Expressions written with bracket notation like model["ssh-key"].resource.state.x won't be detected by the new skip logic in evaluateDefinition. When extractDependencies(expr.celExpression) is called at expression_evaluation_service.ts:236, it returns an empty array for bracket notation, so the skip logic doesn't trigger.

   Concrete example: If a definition contains:

   globalArguments:
     vpc_id: "${{ model[\"my-vpc\"].resource.state.vpc.attributes.VpcId }}"

   And my-vpc was never executed, the expression WON'T be skipped because extractDependencies returns []. The CEL evaluator will then crash with "No such key: resource" - the exact error this PR aims to prevent.

   Evidence this is used: The codebase has existing tests using bracket notation:

   • integration/cel_data_access_test.ts:860: model['s3-infra'].resource...
   • integration/cel_data_access_test.ts:986: model["s3-infra"].resource...

   The CEL evaluator itself recognizes bracket notation (line 389): /model(?:\.\w[\w-]*|\["[^"]+"\])\.(?:resource|file)\b/

   Mitigating factors:

   • Most users write dot notation (model.ssh-key.resource) which IS handled (regex allows hyphens)
   • The Proxy fallback at runtime will catch unresolved expressions if the method accesses them
   • The failure is noisy (CEL crash), not silent corruption

   Suggested fix: Update MODEL_REF_PATTERN to also match bracket notation, similar to the pattern in cel_evaluator.ts:389.

2. Unit tests don't exercise the evaluateDefinition skip logic - src/domain/models/method_execution_service_test.ts:1536-1650

   All three new unit tests call service.execute() or service.executeWorkflow() directly with pre-built definitions. They test the Proxy behavior and validation skipping, but don't verify that evaluateDefinition correctly skips expressions with unresolvable resource dependencies.

   What this means: If someone breaks the skip logic in expression_evaluation_service.ts, these tests won't catch it. The integration test does cover this path (using dot notation), but a unit test for evaluateDefinition would provide faster feedback.

Low

1. Potential false positive on evaluated strings containing ${{ ... }} - method_execution_service.ts:141-146

   The new Proxy throws for ANY string containing ${{ ... }}:

   if (typeof value === "string" && containsExpression(value)) {
     throw new Error(`Unresolved expression in globalArguments.${prop}: ${value}`);
   }

   Edge case: If a CEL expression evaluates to a string that legitimately contains ${{ ... }} (e.g., ${{ 'Template: ${{ placeholder }}' }}), the Proxy would incorrectly throw.

   Why this is LOW: This is extremely unlikely in practice and arguably indicates a bug in the definition. The trade-off (preventing silent corruption) is worth it.

Verdict

PASS - The PR correctly extends the existing skip pattern for inputs.* expressions to also cover model.*.resource and model.*.file expressions. The Proxy fix is a genuine improvement that prevents silent data corruption. The bracket notation gap in extractDependencies is a pre-existing limitation that should be addressed in a follow-up, but doesn't block this fix since:

1. Dot notation (the common case) works correctly
2. The Proxy provides a runtime safety net
3. The failure mode for bracket notation is visible, not silent