Skip to content

fix: resolve ternary CEL expressions with partial inputs (#814) #815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stack72 merged 1 commit into from
Mar 22, 2026
Merged

fix: resolve ternary CEL expressions with partial inputs (#814) #815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions integration/workflow_partial_inputs_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,88 @@ Deno.test("Workflow: succeeds with partial inputs when globalArguments reference
assertEquals(output.jobs[0].steps[0].status, "succeeded");
});
});

// Regression test for issue #814.
//
// A ternary expression in methods.execute.arguments.run must resolve correctly
// when the condition input is provided but one branch input is absent.
//
// The ternary is placed in methods.execute.arguments.run (not globalArguments)
// because the shell model reads args.run directly — this gives a clear pass/fail
// signal without being dependent on any model-specific globalArgs access.
//
// Before the fix: expression is skipped (regex pre-check treats all referenced
// inputs as required), shell executes the raw "${{ ... }}" string → exit code 1.
// After the fix: CEL short-circuits the ternary and resolves to "echo 100.64.0.1".
Deno.test("Workflow: ternary expression resolves when condition input provided and one branch input absent", async () => {
await withTempDir(async (repoDir) => {
await initializeTestRepo(repoDir);

const definitionRepo = new YamlDefinitionRepository(repoDir);
const workflowRepo = new YamlWorkflowRepository(repoDir);

const model = Definition.create({
name: "transport-host-model",
inputs: {
properties: {
transport: { type: "string" },
lan_host: { type: "string" },
tailnet_host: { type: "string" },
},
required: ["transport", "tailnet_host"],
},
globalArguments: {},
methods: {
execute: {
arguments: {
run:
'${{ "echo " + (inputs.transport == "lan" ? inputs.lan_host : inputs.tailnet_host) }}',
},
},
},
});
await definitionRepo.save(SHELL_MODEL_TYPE, model);

const workflow = Workflow.create({
name: "ternary-transport-workflow",
jobs: [
Job.create({
name: "run-job",
steps: [
Step.create({
name: "run-step",
task: StepTask.model("transport-host-model", "execute", {
transport: "wan",
tailnet_host: "100.64.0.1",
// lan_host deliberately absent — only the wan branch is needed
}),
}),
],
}),
],
});
await workflowRepo.save(workflow);

const result = await runCliCommand(
[
"workflow",
"run",
"ternary-transport-workflow",
"--repo-dir",
repoDir,
"--json",
],
Deno.cwd(),
);

assertEquals(
result.code,
0,
`Workflow should succeed with ternary expression. stderr: ${result.stderr}`,
);

const output = JSON.parse(result.stdout);
assertEquals(output.status, "succeeded");
assertEquals(output.jobs[0].steps[0].status, "succeeded");
});
});
60 changes: 24 additions & 36 deletions src/domain/expressions/expression_evaluation_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import { CelEvaluator } from "../../infrastructure/cel/cel_evaluator.ts";
import {
containsExpression,
extractExpressions,
extractInputReferencesFromCel,
replaceExpressions,
} from "./expression_parser.ts";
import type { ExpressionLocation } from "./expression.ts";
Expand Down Expand Up @@ -215,57 +214,46 @@ export class ExpressionEvaluationService {

// Evaluate CEL-only expressions; skip runtime expressions (vault, env)
// Runtime expressions are resolved at runtime only, never persisted
const providedInputKeys = inputValues
? new Set(Object.keys(inputValues))
: new Set<string>();
const evaluatedValues = new Map<string, unknown>();
for (const expr of expressions) {
if (containsRuntimeExpression(expr.celExpression)) {
// Leave runtime expressions (vault, env, and mixed) as raw
continue;
}

// Skip expressions that reference inputs not provided by the user.
// 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) {
for (const ref of inputRefs) {
if (!providedInputKeys.has(ref)) {
hasMissing = true;
break;
}
}
}

// 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;
}
// Unlike inputs, model data is never conditionally accessed in CEL —
// member access on a missing model ref is always an error.
let hasMissingModelDep = false;
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)
) {
hasMissingModelDep = true;
break;
}
}
}

if (hasMissing) {
if (hasMissingModelDep) {
continue;
}

const value = this.celEvaluator.evaluate(expr.celExpression, ctx);
evaluatedValues.set(expr.raw, value);
try {
const value = this.celEvaluator.evaluate(expr.celExpression, ctx);
evaluatedValues.set(expr.raw, value);
} catch {
// Leave unresolved — CEL threw because an input referenced directly
// (not inside a conditional branch) is absent from context.
// The Proxy on globalArgs will surface a clear error if the method
// actually needs the unresolved value.
}
}

// Replace only the CEL-only expressions with evaluated values
Expand Down
96 changes: 95 additions & 1 deletion src/domain/expressions/expression_evaluation_service_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,26 @@
// You should have received a copy of the GNU Affero General Public License
// along with Swamp. If not, see <https://www.gnu.org/licenses/>.

import { assertEquals } from "@std/assert";
import { assertEquals, assertStringIncludes } from "@std/assert";
import { Definition } from "../definitions/definition.ts";
import { ModelType } from "../models/model_type.ts";
import { YamlDefinitionRepository } from "../../infrastructure/persistence/yaml_definition_repository.ts";
import {
containsEnvExpression,
containsRuntimeExpression,
containsVaultExpression,
ExpressionEvaluationService,
} from "./expression_evaluation_service.ts";

async function withTempDir(fn: (dir: string) => Promise<void>): Promise<void> {
const dir = await Deno.makeTempDir({ prefix: "swamp-eval-service-" });
try {
await fn(dir);
} finally {
await Deno.remove(dir, { recursive: true });
}
}

// ============================================================================
// containsVaultExpression
// ============================================================================
Expand Down Expand Up @@ -126,3 +139,84 @@ Deno.test("containsRuntimeExpression returns false for model/self/inputs", () =>
assertEquals(containsRuntimeExpression("self.name"), false);
assertEquals(containsRuntimeExpression("inputs.param"), false);
});

// ============================================================================
// evaluateDefinition — ternary expression regression (#814)
// ============================================================================

// Regression test for issue #814: a ternary in globalArguments must resolve
// when the condition input is provided, even if one branch input is absent.
// Before the fix the regex pre-check treated all referenced inputs as required,
// so the whole expression was skipped and the globalArgument stayed as a raw
// ${{ }} string.
Deno.test("evaluateDefinition: ternary in globalArguments resolves when condition input provided and one branch input absent", async () => {
await withTempDir(async (repoDir) => {
const definitionRepo = new YamlDefinitionRepository(repoDir);
const service = new ExpressionEvaluationService(definitionRepo, repoDir);
const type = ModelType.create("command/shell");

const definition = Definition.create({
name: "transport-model",
inputs: {
properties: {
transport: { type: "string" },
lan_host: { type: "string" },
tailnet_host: { type: "string" },
},
required: ["transport", "tailnet_host"],
},
globalArguments: {
host:
'${{ inputs.transport == "lan" ? inputs.lan_host : inputs.tailnet_host }}',
},
methods: { exec: { arguments: { run: "echo hello" } } },
});

const result = await service.evaluateDefinition(
definition,
type,
{ transport: "wan", tailnet_host: "100.64.0.1" }, // lan_host deliberately absent
);

assertEquals(result.definition.globalArguments.host, "100.64.0.1");
});
});

// Confirms the original #653 fix still works: a directly-referenced missing
// input leaves the expression unresolved rather than throwing.
Deno.test("evaluateDefinition: directly-missing input in globalArguments stays unresolved", async () => {
await withTempDir(async (repoDir) => {
const definitionRepo = new YamlDefinitionRepository(repoDir);
const service = new ExpressionEvaluationService(definitionRepo, repoDir);
const type = ModelType.create("command/shell");

const definition = Definition.create({
name: "factory-model",
inputs: {
properties: {
instanceName: { type: "string" },
cidrBlock: { type: "string" },
},
required: ["instanceName", "cidrBlock"],
},
globalArguments: {
run: '${{ "echo " + inputs.instanceName }}',
unusedArg: "${{ inputs.cidrBlock }}",
},
methods: { execute: { arguments: { run: "echo fallback" } } },
});

const result = await service.evaluateDefinition(
definition,
type,
{ instanceName: "test-instance" }, // cidrBlock deliberately absent
);

// run is resolved, unusedArg stays as a raw expression
assertEquals(result.definition.globalArguments.run, "echo test-instance");
assertStringIncludes(
result.definition.globalArguments.unusedArg as string,
"${{",
);
});
});
14 changes: 14 additions & 0 deletions src/domain/expressions/expression_parser_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
extractCelExpression,
extractExpressions,
extractInputReferences,
extractInputReferencesFromCel,
isTaskInputsPath,
replaceExpressions,
} from "./expression_parser.ts";
Expand Down Expand Up @@ -373,3 +374,16 @@ Deno.test("extractInputReferences handles mixed dot and bracket notation", () =>
new Set(["region", "drop-name"]),
);
});

// Documents why extractInputReferencesFromCel must not be used as a skip gate
// for expression evaluation. It reports all inputs in ALL branches of a ternary,
// regardless of which branch CEL would actually evaluate at runtime.
// This was the root cause of the #814 regression introduced in #655.
Deno.test("extractInputReferencesFromCel: returns all branch inputs in ternary regardless of which branch runs", () => {
assertEquals(
extractInputReferencesFromCel(
`inputs.transport == "lan" ? inputs.lan_host : inputs.tailnet_host`,
),
new Set(["transport", "lan_host", "tailnet_host"]),
);
});
Loading
Loading