fix(expressions): wire DataQueryService into model evaluate and detect nested unresolved expressions (swamp-club#358)

[stack72]

Summary

• Primary fix: createModelEvaluateDeps was the only evaluation path missing a DataQueryService, causing data.latest() and all data.* functions to silently return null during swamp model evaluate. The workflow evaluate, model validate, and model method run paths all had it wired up — the model evaluate path was missed. Added the same 2-line pattern.

• Defense-in-depth: Fixed method_execution_service.ts to detect unresolved ${{ }} expressions inside nested globalArguments objects/arrays. The hasUnresolved check and globalArgsProxy both only examined top-level string values — switched to the recursive valueContainsExpression that was already imported and used elsewhere in the same file.

Test Plan

• Added regression test for nested evaluateDefinition with inputs.* expressions (expression_evaluation_service_test.ts)
• Added 4 tests for nested unresolved expression detection in method_execution_service_test.ts:
  • Proxy throws for nested unresolved object
  • Proxy throws for unresolved array element
  • Proxy allows fully resolved nested objects
  • executeWorkflow detects nested unresolved in validation path
• Existing 5992 tests pass with 0 failures
• Reproduced bug in scratch repo before fix (data.latest() unresolved at all depths)
• Verified fix in scratch repo after compile (data.latest() resolved at all depths)

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — This PR is a pure internal bug fix with no new flags, commands, or output-format changes. The only user-visible delta is the globalArgsProxy error message, which now uses JSON.stringify(value) instead of bare string interpolation. That's a clear improvement: previously, when a nested object or array had an unresolved expression, users would see the unhelpful [object Object]; now they get the actual JSON content. The silent null return from data.latest() during swamp model evaluate is fixed transparently — no UX surface changed, the command just works correctly now.

[github-actions]

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. method_execution_service.ts:297-308 — Proxy only guards top-level property access, not deeply nested access paths. The Proxy on resolvedGlobalArgs intercepts get traps on the top-level object. When a method accesses context.globalArgs.config, the Proxy fires on the config key and valueContainsExpression correctly walks the nested object to detect expressions. However, if a method destructures or spreads the object (e.g., { ...context.globalArgs } or Object.entries(context.globalArgs)), the spread calls get on each key individually — if a key like config contains a nested object with an unresolved expression, the Proxy will correctly catch it. So the defense works for all standard access patterns. The only gap is if someone passes context.globalArgs to a function that uses Reflect.ownKeys + direct property descriptor access (bypassing the get trap), but that's an exotic pattern unlikely in practice. LOW risk, noting for completeness.

2. method_execution_service.ts:425-431 — executeWorkflow drops entire top-level keys when any nested value contains an expression, but doesn't propagate which specific nested fields were unresolved. Consider: globalArguments: { config: { region: "${{ inputs.region }}", name: "static" } }. The entire config key is dropped from resolvedGlobalArgs because valueContainsExpression returns true for the nested region. The name: "static" subfield is also lost from Zod validation. This is the correct conservative behavior (fail-safe), but extension authors may find it confusing that a partially-resolved object is entirely excluded rather than partially validated. This is a pre-existing design choice, not a bug introduced by this PR.

Low

1. expression_parser.ts:59-69 — valueContainsExpression doesn't guard against circular references. If a globalArguments value somehow contained a circular object reference, this recursive function would stack overflow. In practice, globalArguments come from YAML deserialization which cannot produce circular references, so this is theoretical only.

2. method_execution_service.ts:301-304 — JSON.stringify(value) in the error message could be verbose for deeply nested objects. If value is a large nested structure, the error message could be very long. Not a bug, just a readability concern for debugging output.

3. expression_evaluation_service_test.ts:580 — The new test creates ExpressionEvaluationService without dataQueryService. This is fine because the test is specifically testing inputs.* expression resolution which doesn't need dataQueryService, but it means the test doesn't exercise the actual bug fix (missing DataQueryService in createModelEvaluateDeps). The createModelEvaluateDeps wiring fix in evaluate.ts is tested by the existing integration/e2e tests mentioned in the PR description (reproduced in scratch repo), not by this unit test. The unit test covers the other fix (nested expression detection).

Verdict

PASS — Both fixes are correct and well-targeted. The primary fix (evaluate.ts) adds DataQueryService wiring that matches the established pattern in all other callers (model_method_run.ts, execution_service.ts). The defense-in-depth fix (method_execution_service.ts) correctly replaces shallow containsExpression with recursive valueContainsExpression in both the Proxy guard and the executeWorkflow validation loop. The containsExpression import removal is clean — it's still used internally by expression_parser.ts and expression_evaluation_service.ts. Tests cover the nested detection paths well.

[github-actions]

Code Review

Clean, well-targeted bug fix. Both changes are correct and follow established patterns already used in validate.ts and workflows/evaluate.ts.

Primary fix (src/libswamp/models/evaluate.ts): Wiring DataQueryService into createModelEvaluateDeps follows the exact same pattern as validate.ts:126-132 and workflows/evaluate.ts:124. Extracting catalogStore into a shared variable for both dataRepo and DataQueryService is the right approach.

Defense-in-depth fix (src/domain/models/method_execution_service.ts): Switching from containsExpression (string-only) to valueContainsExpression (recursive) in both the Proxy guard and the hasUnresolved check is correct — the recursive variant was already imported and used elsewhere. The unused containsExpression import is cleanly removed.

Tests: 5 new tests cover the fix thoroughly — nested object expressions, array expressions, resolved object pass-through, workflow validation path, and the expression evaluation service path.

Blocking Issues

None.

Suggestions

1. The comment at method_execution_service.ts:420-422 references the issue number (swamp-club#358). Per CLAUDE.md: "Don't reference the current task, fix, or callers ... since those belong in the PR description and rot as the codebase evolves." Consider removing the (swamp-club#358) suffix from the comment — the what and why are already clear without it. Same applies to the section comment headers in the test files.