From de61ac15cb3e1a5ef065ef9703f770508825d7e9 Mon Sep 17 00:00:00 2001 From: Paul Stack Date: Sat, 7 Mar 2026 22:39:21 +0000 Subject: [PATCH] fix: skip unresolvable cross-model resource expressions in globalArguments (#641) When a model's globalArguments contain CEL expressions referencing another model's resource or file data (e.g., model.ssh-key.resource.state...), method execution fails with 'No such key: resource' if the referenced model has no data on disk. This extends the existing 'skip unresolved inputs' pattern in ExpressionEvaluationService.evaluateDefinition() to also skip expressions with model.*.resource or model.*.file references when the context proves that data doesn't exist. The raw expression is preserved in the definition so the Proxy on globalArgs throws a clear error if the method actually accesses the field, while methods that don't need it succeed. Also fixes a pre-existing bug in the globalArgs Proxy: previously, it only checked for unresolved inputs.* references, so a globalArg containing an unresolved model.*.resource expression would silently return the raw ${{ ... }} string to the method, risking silent data corruption. The Proxy now catches any remaining unresolved expression. --- .../swamp-model/references/troubleshooting.md | 30 ++++- integration/keeb_shell_model_test.ts | 66 ++++++++++ .../expression_evaluation_service.ts | 33 ++++- src/domain/models/method_execution_service.ts | 27 ++-- .../models/method_execution_service_test.ts | 118 ++++++++++++++++++ 5 files changed, 251 insertions(+), 23 deletions(-) diff --git a/.claude/skills/swamp-model/references/troubleshooting.md b/.claude/skills/swamp-model/references/troubleshooting.md index 3c90895c..f6d41fa7 100644 --- a/.claude/skills/swamp-model/references/troubleshooting.md +++ b/.claude/skills/swamp-model/references/troubleshooting.md @@ -135,7 +135,15 @@ swamp model validate my-model --json vpcId: ${{ model.my-vpc.resource.vpc.main.attributes.VpcId }} ``` -2. **Model never executed**: +2. **Model never executed** — expressions referencing `model.*.resource` or + `model.*.file` are automatically skipped when the referenced model has no + data. If a method accesses a skipped field, it throws a clear error: + + ``` + Unresolved expression in globalArguments.ssh_keys: ${{ model.ssh-key.resource... }} + ``` + + To fix, run the referenced model first: ```bash swamp model method run my-vpc create --json @@ -158,6 +166,26 @@ swamp model validate my-model --json swamp data get my-vpc vpc --json ``` +### "Unresolved expression in globalArguments" + +**Symptom**: +`Error: Unresolved expression in globalArguments.: ${{ ... }}` + +**Cause**: A `globalArguments` field contains a CEL expression that couldn't be +resolved (e.g., the referenced model has no resource data), and the method tried +to use that field. + +**Solutions**: + +1. **Run the referenced model first** so its data is available: + + ```bash + swamp model method run create --json + ``` + +2. **Use a workflow** that runs models in the correct order — dependencies are + resolved automatically within a workflow run. + ### "Model type not found" **Symptom**: `Error: Model type '' not found` diff --git a/integration/keeb_shell_model_test.ts b/integration/keeb_shell_model_test.ts index 21930f5a..5733931c 100644 --- a/integration/keeb_shell_model_test.ts +++ b/integration/keeb_shell_model_test.ts @@ -412,3 +412,69 @@ Deno.test("CLI: command/shell model with self-reference expressions", async () = assertEquals(output.data.attributes.exitCode, 0); }); }); + +Deno.test("CLI: model method run succeeds when globalArguments has unresolvable cross-model resource expression", async () => { + await withTempDir(async (repoDir) => { + await initializeTestRepo(repoDir); + + const definitionRepo = new YamlDefinitionRepository(repoDir); + + // Create source model (no data — never executed) + const sourceModel = Definition.create({ + name: "source-shell", + globalArguments: { + run: "echo source", + }, + methods: { + execute: { + arguments: { + run: "echo source", + }, + }, + }, + }); + await definitionRepo.save(SHELL_MODEL_TYPE, sourceModel); + + // Create dependent model that references source model's resource state + // in globalArguments — this resource does NOT exist yet + const dependentModel = Definition.create({ + name: "dependent-shell", + globalArguments: { + run: "echo hello", + workingDir: + "${{ model.source-shell.resource.state.result.attributes.stdout }}", + }, + methods: { + execute: { + arguments: { + run: "echo hello", + }, + }, + }, + }); + await definitionRepo.save(SHELL_MODEL_TYPE, dependentModel); + + // Run the dependent model directly — should succeed because: + // 1. The unresolvable resource expression in globalArguments.workingDir is skipped + // 2. The method only uses the "run" argument, not "workingDir" + const result = await runCliCommand( + [ + "model", + "method", + "run", + "dependent-shell", + "execute", + "--repo-dir", + repoDir, + "--json", + ], + Deno.cwd(), + ); + + assertEquals( + result.code, + 0, + `Command should succeed. stderr: ${result.stderr}`, + ); + }); +}); diff --git a/src/domain/expressions/expression_evaluation_service.ts b/src/domain/expressions/expression_evaluation_service.ts index 7c44e4cb..fae2b4b3 100644 --- a/src/domain/expressions/expression_evaluation_service.ts +++ b/src/domain/expressions/expression_evaluation_service.ts @@ -29,7 +29,10 @@ import { replaceExpressions, } from "./expression_parser.ts"; import type { ExpressionLocation } from "./expression.ts"; -import { extractModelRefs } from "./dependency_extractor.ts"; +import { + extractDependencies, + extractModelRefs, +} from "./dependency_extractor.ts"; import { buildEnvContext, type ExpressionContext, @@ -215,19 +218,41 @@ export class ExpressionEvaluationService { // This allows methods that don't need certain inputs to run without // those inputs being provided (e.g., delete doesn't need create-time inputs). const inputRefs = extractInputReferencesFromCel(expr.celExpression); + let hasMissing = false; if (inputRefs.size > 0) { - let hasMissing = false; for (const ref of inputRefs) { if (!providedInputKeys.has(ref)) { hasMissing = true; break; } } - if (hasMissing) { - continue; + } + + // Skip expressions referencing model resource/file data that isn't + // available in context (e.g., referenced model was never executed). + // This mirrors the inputs skip above — the raw expression is preserved + // so the Proxy on globalArgs will throw if the method actually needs it. + if (!hasMissing) { + const deps = extractDependencies(expr.celExpression); + for (const dep of deps) { + if (dep.type === "resource" || dep.type === "file") { + const modelData = ctx.model[dep.modelRef]; + if ( + !modelData || + (dep.type === "resource" && !modelData.resource) || + (dep.type === "file" && !modelData.file) + ) { + hasMissing = true; + break; + } + } } } + if (hasMissing) { + continue; + } + const value = this.celEvaluator.evaluate(expr.celExpression, ctx); evaluatedValues.set(expr.raw, value); } diff --git a/src/domain/models/method_execution_service.ts b/src/domain/models/method_execution_service.ts index 595657e0..4e5455cd 100644 --- a/src/domain/models/method_execution_service.ts +++ b/src/domain/models/method_execution_service.ts @@ -38,10 +38,7 @@ import { createResourceWriter, } from "./data_writer.ts"; import { coerceMethodArgs } from "./zod_type_coercion.ts"; -import { - containsExpression, - extractInputReferencesFromCel, -} from "../expressions/expression_parser.ts"; +import { containsExpression } from "../expressions/expression_parser.ts"; /** * Maximum depth for recursive follow-up action processing. @@ -133,8 +130,8 @@ export class DefaultMethodExecutionService implements MethodExecutionService { // Populate context with global args and definition metadata. // Wrap globalArgs in a Proxy that throws a clear error when the method - // accesses a field with an unresolved ${{ inputs.* }} expression. - // This allows methods that don't need certain inputs to succeed while + // accesses a field with an unresolved ${{ ... }} expression. + // This allows methods that don't need certain fields to succeed while // failing fast with a helpful message if they do. const rawGlobalArgs = definition.globalArguments; const globalArgsProxy = new Proxy(rawGlobalArgs, { @@ -144,13 +141,9 @@ export class DefaultMethodExecutionService implements MethodExecutionService { typeof prop === "string" && typeof value === "string" && containsExpression(value) ) { - const refs = extractInputReferencesFromCel(value); - if (refs.size > 0) { - const missing = [...refs].join(", "); - throw new Error( - `Missing required input(s): ${missing} (needed by globalArguments.${prop})`, - ); - } + throw new Error( + `Unresolved expression in globalArguments.${prop}: ${value}`, + ); } return value; }, @@ -219,14 +212,12 @@ export class DefaultMethodExecutionService implements MethodExecutionService { if (modelDef.globalArguments) { const rawGlobalArgs = currentDefinition.globalArguments; - // Identify globalArg fields with unresolved input expressions + // Identify globalArg fields with unresolved expressions (inputs, + // model resource/file refs, or any other ${{ ... }} that wasn't evaluated) let hasUnresolved = false; const resolvedGlobalArgs: Record = {}; for (const [key, value] of Object.entries(rawGlobalArgs)) { - if ( - typeof value === "string" && containsExpression(value) && - extractInputReferencesFromCel(value).size > 0 - ) { + if (typeof value === "string" && containsExpression(value)) { hasUnresolved = true; } else { resolvedGlobalArgs[key] = value; diff --git a/src/domain/models/method_execution_service_test.ts b/src/domain/models/method_execution_service_test.ts index 25ebc6f9..de850ef1 100644 --- a/src/domain/models/method_execution_service_test.ts +++ b/src/domain/models/method_execution_service_test.ts @@ -1530,3 +1530,121 @@ Deno.test("executeWorkflow - explicit kind overrides name inference for deletion // kind: "action" should NOT trigger deletion markers assertEquals(mockRepo.savedData.length, 0); }); + +// ---------- Unresolved Expression Tests ---------- + +Deno.test("execute - Proxy throws for any unresolved expression in globalArgs", async () => { + const service = new DefaultMethodExecutionService(); + + const model: ModelDefinition = { + type: ModelType.create("test/unresolved-expr"), + version: "1", + methods: { + run: { + description: "Test method", + arguments: z.object({}), + execute: (_args: Record, context) => { + // Access the unresolved field — should throw + const _val = context.globalArgs.ssh_keys; + return Promise.resolve({}); + }, + }, + }, + }; + + const definition = Definition.create({ + name: "test-definition", + globalArguments: { + name: "my-server", + ssh_keys: + '${{ string(model["test-ssh-key"].resource.state.key.attributes.id) }}', + }, + methods: { run: { arguments: {} } }, + }); + + const { context } = createTestContext({ modelType: model.type }); + await assertRejects( + () => service.execute(definition, model.methods.run, context), + Error, + "Unresolved expression in globalArguments.ssh_keys", + ); +}); + +Deno.test("execute - Proxy allows access to resolved globalArgs fields", async () => { + const service = new DefaultMethodExecutionService(); + + let receivedName = ""; + const model: ModelDefinition = { + type: ModelType.create("test/resolved-expr"), + version: "1", + methods: { + run: { + description: "Test method", + arguments: z.object({}), + execute: (_args: Record, context) => { + receivedName = context.globalArgs.name as string; + return Promise.resolve({}); + }, + }, + }, + }; + + const definition = Definition.create({ + name: "test-definition", + globalArguments: { + name: "my-server", + ssh_keys: + '${{ string(model["test-ssh-key"].resource.state.key.attributes.id) }}', + }, + methods: { run: { arguments: {} } }, + }); + + const { context } = createTestContext({ modelType: model.type }); + await service.execute(definition, model.methods.run, context); + assertEquals(receivedName, "my-server"); +}); + +Deno.test("executeWorkflow - skips validation for globalArgs with unresolved model resource expressions", async () => { + const service = new DefaultMethodExecutionService(); + + const schema = z.object({ + name: z.string(), + ssh_keys: z.array(z.string()), + }); + + let receivedName = ""; + const model: ModelDefinition = { + type: ModelType.create("test/unresolved-resource-expr"), + version: "1", + globalArguments: schema, + methods: { + update: { + description: "Update method", + arguments: z.object({}), + execute: (_args: Record, context) => { + receivedName = context.globalArgs.name as string; + return Promise.resolve({}); + }, + }, + }, + }; + + const definition = Definition.create({ + name: "test-definition", + globalArguments: { + name: "my-server", + ssh_keys: + '${{ [string(model["test-ssh-key"].resource.state.key.attributes.id)] }}', + }, + }); + + const { context } = createTestContext({ modelType: model.type }); + const result = await service.executeWorkflow( + definition, + model, + "update", + context, + ); + assertEquals(result !== undefined, true); + assertEquals(receivedName, "my-server"); +});