Review Summary

Verdict: Approved ✅

This PR implements a well-designed three-layer fix for #626, making input validation method-aware for extension models.

Code Quality

Domain-Driven Design

• Domain services (ExpressionEvaluationService, MethodExecutionService) are stateless and appropriately placed
• Pure functions (extractInputReferences, extractInputReferencesFromCel) are well-isolated in the expression parser module
• Changes follow existing architectural patterns

Suggestions (non-blocking)

1. The third integration test validates success when globalArgs have unresolved inputs, but doesn't explicitly test the error case when a method accesses an unresolved globalArg. An automated test for this edge case could be added for regression protection (manual testing confirmed it works per the PR description).

2. Consider adding a unit test for the regex handling of model.*.inputs.* patterns to ensure negative lookbehind handles both input and inputs in cross-model paths.

Highlights

• The three-layer approach is thorough: CLI validation, CEL evaluation, and runtime Proxy guard
• Well-documented comments explaining the "why" behind each layer
• Clear error messages via the Proxy pattern for missing inputs
• Comprehensive test coverage including edge cases (dot/bracket notation, deduplication, cross-model exclusion)

Great work! 🎉

Adversarial Review

Medium

1. Nested globalArguments bypass unresolved expression detection (method_execution_service.ts:221-232)

   The loop that identifies unresolved input expressions only checks top-level string values:

   for (const [key, value] of Object.entries(rawGlobalArgs)) {
     if (typeof value === "string" && containsExpression(value) && ...) {

   Breaking example: A model with nested globalArguments:

   globalArguments:
     config:
       apiToken: "${{ inputs.apiToken }}"

   When running a method without providing apiToken:

   • config is an object, so typeof value === "string" fails
   • The nested expression isn't detected, hasUnresolved stays false
   • Full Zod validation runs and passes (the expression is a valid string)
   • The method receives context.globalArgs.config.apiToken as the raw string "${{ inputs.apiToken }}"

   The Proxy guard (lines 138-155) also only intercepts top-level property access, so context.globalArgs.config.apiToken bypasses the check entirely.

   Impact: Methods receive raw expression strings instead of values. Most likely causes runtime failures or incorrect behavior. Could leak expression syntax to external systems (API calls, logs).

   Suggested fix: Either recursively check nested structures, or use a recursive Proxy that guards all levels.

2. Expressions referencing both provided and unprovided inputs skip entirely (expression_evaluation_service.ts:214-229)

   If an expression references multiple inputs and ANY is missing, the entire expression is skipped:

   if (hasMissing) {
     continue;  // Skip the entire expression
   }

   Breaking example:

   globalArguments:
     combined: "${{ inputs.provided + ' ' + inputs.unprovided }}"

   With only provided given:

   • The expression is skipped (not evaluated)
   • combined retains the raw expression string
   • Method may receive/use raw expression string

   Impact: Same as issue #1 - raw expressions may reach runtime code. This is intentional per the PR design (defer to runtime Proxy), but the Proxy doesn't catch all cases (nested objects, bracket access patterns like context.globalArgs["combined"]).

Low

3. Single-quoted bracket notation not detected (expression_parser.ts:233)

   INPUTS_BRACKET_PATTERN only matches double quotes: inputs["field"]. Single quotes inputs['field'] won't be detected. CEL typically uses double quotes, so low risk, but inconsistent with CEL's actual syntax support.

4. Escaped quotes in input names parsed incorrectly (expression_parser.ts:233)

   Pattern [^"]+ stops at first quote. inputs["field\"name"] extracts field\ not field"name. Edge case.

5. Unicode input names not supported (expression_parser.ts:227)

   Pattern [a-zA-Z_][a-zA-Z0-9_]* is ASCII-only. If Unicode input names are valid elsewhere in the system, they won't be detected here.

Verdict

PASS — No critical or high severity issues.

The medium issues represent edge cases (nested globalArguments) that are unlikely in typical usage. The core fix for #626 is sound: method arguments are scanned correctly, and top-level globalArguments are guarded by the Proxy. The integration tests cover the intended scenarios.

The gaps in nested object handling could cause confusing failures in complex extension models, but won't cause data loss, security vulnerabilities, or crashes in production. The failure mode (method receives raw expression string) will typically manifest as an obvious runtime error when the method tries to use the invalid value.

Consider adding a follow-up task to recursively guard nested globalArguments structures if extension models with nested configs become common.