From 4d5b51a67ce053fe89e8c48460c186e1bbde8d61 Mon Sep 17 00:00:00 2001 From: Lokesh Gopu Date: Wed, 1 Jul 2026 01:14:49 -0700 Subject: [PATCH 1/2] Reject `if` conditions on wait, wait-all, and cancel steps --- workflow-parser/src/model/convert.test.ts | 44 ++++++++++++++++++++ workflow-parser/src/model/converter/steps.ts | 15 +++++++ workflow-parser/src/workflow-v1.0.json | 3 ++ 3 files changed, 62 insertions(+) diff --git a/workflow-parser/src/model/convert.test.ts b/workflow-parser/src/model/convert.test.ts index 5ba1453c..f6c8bd99 100644 --- a/workflow-parser/src/model/convert.test.ts +++ b/workflow-parser/src/model/convert.test.ts @@ -868,4 +868,48 @@ jobs: expect(template.errors!.some(e => e.Message.includes("Nested 'parallel' blocks are not allowed"))).toBe(true); }); }); + + describe("conditions on control-flow steps", () => { + const conditionError = + "'if' is not supported on 'wait', 'wait-all', and 'cancel' steps. These steps always run and cannot be conditional."; + + it.each([ + [ + "wait", + ` - id: bg\n run: echo hi\n background: true\n - if: \${{ always() }}\n wait: bg` + ], + [ + "wait-all", + ` - id: bg\n run: echo hi\n background: true\n - if: \${{ always() }}\n wait-all:` + ], + [ + "cancel", + ` - id: bg\n run: echo hi\n background: true\n - if: \${{ always() }}\n cancel: bg` + ] + ])("rejects 'if' on a %s step", async (_name, stepsYaml) => { + const context = new TemplateContext(new TemplateValidationErrors(), getWorkflowSchema(), nullTrace); + context.state.featureFlags = new FeatureFlags({allowBackgroundSteps: true}); + + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + steps: +${stepsYaml}` + }, + context + ); + + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion, + featureFlags: new FeatureFlags({allowBackgroundSteps: true}) + }); + + expect(template.errors).toBeDefined(); + expect(template.errors!.some(e => e.Message.includes(conditionError))).toBe(true); + }); + }); }); diff --git a/workflow-parser/src/model/converter/steps.ts b/workflow-parser/src/model/converter/steps.ts index 02935172..9375d50f 100644 --- a/workflow-parser/src/model/converter/steps.ts +++ b/workflow-parser/src/model/converter/steps.ts @@ -15,6 +15,10 @@ import {handleTemplateTokenErrors} from "./handle-errors.js"; import {IdBuilder} from "./id-builder.js"; import {FeatureFlags} from "@actions/expressions/features"; +// wait, wait-all, and cancel steps always run and cannot be made conditional. +const CONTROL_FLOW_CONDITION_ERROR = + "'if' is not supported on 'wait', 'wait-all', and 'cancel' steps. These steps always run and cannot be conditional."; + export function convertSteps(context: TemplateContext, steps: TemplateToken): Step[] { if (!isSequence(steps)) { context.error(steps, "Invalid format for steps"); @@ -83,6 +87,7 @@ function convertStep( let continueOnError: boolean | ScalarToken | undefined; let env: MappingToken | undefined; let ifCondition: BasicExpressionToken | undefined; + let ifKeyToken: StringToken | undefined; for (const item of mapping) { const key = item.key.assertString("steps item key"); switch (key.value) { @@ -126,6 +131,7 @@ function convertStep( env = item.value.assertMapping("step env"); break; case "if": + ifKeyToken = key; ifCondition = convertToIfCondition(context, item.value); break; case "continue-on-error": @@ -162,6 +168,9 @@ function convertStep( } if (wait) { + if (ifCondition) { + context.error(ifKeyToken ?? mapping, CONTROL_FLOW_CONDITION_ERROR); + } return { id: id?.value || "", name: name || createSyntheticStepName("Wait"), @@ -171,6 +180,9 @@ function convertStep( } if (waitAll !== undefined) { + if (ifCondition) { + context.error(ifKeyToken ?? mapping, CONTROL_FLOW_CONDITION_ERROR); + } return { id: id?.value || "", name: name || createSyntheticStepName("Wait for all"), @@ -180,6 +192,9 @@ function convertStep( } if (cancel) { + if (ifCondition) { + context.error(ifKeyToken ?? mapping, CONTROL_FLOW_CONDITION_ERROR); + } return { id: id?.value || "", name: name || createSyntheticStepName("Cancel"), diff --git a/workflow-parser/src/workflow-v1.0.json b/workflow-parser/src/workflow-v1.0.json index 3d920fc1..0e05b888 100644 --- a/workflow-parser/src/workflow-v1.0.json +++ b/workflow-parser/src/workflow-v1.0.json @@ -2212,6 +2212,7 @@ "properties": { "name": "step-name", "id": "step-id", + "if": "step-if", "continue-on-error": "step-continue-on-error", "wait": { "type": "step-wait-target", @@ -2225,6 +2226,7 @@ "properties": { "name": "step-name", "id": "step-id", + "if": "step-if", "continue-on-error": "step-continue-on-error", "wait-all": { "type": "step-wait-all-value", @@ -2238,6 +2240,7 @@ "properties": { "name": "step-name", "id": "step-id", + "if": "step-if", "continue-on-error": "step-continue-on-error", "cancel": { "type": "step-cancel-target", From f6bf1fd1d8f6273b4a65efd34e40482eba719354 Mon Sep 17 00:00:00 2001 From: Lokesh Gopu Date: Wed, 1 Jul 2026 01:30:27 -0700 Subject: [PATCH 2/2] resolve pr comments --- workflow-parser/src/model/convert.test.ts | 30 ++++++++++++++++++++ workflow-parser/src/model/converter/steps.ts | 12 ++++---- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/workflow-parser/src/model/convert.test.ts b/workflow-parser/src/model/convert.test.ts index f6c8bd99..225e7ba5 100644 --- a/workflow-parser/src/model/convert.test.ts +++ b/workflow-parser/src/model/convert.test.ts @@ -911,5 +911,35 @@ ${stepsYaml}` expect(template.errors).toBeDefined(); expect(template.errors!.some(e => e.Message.includes(conditionError))).toBe(true); }); + + it("rejects 'if' on a cancel step even when the if expression is invalid", async () => { + const context = new TemplateContext(new TemplateValidationErrors(), getWorkflowSchema(), nullTrace); + context.state.featureFlags = new FeatureFlags({allowBackgroundSteps: true}); + + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - id: bg + run: echo hi + background: true + - if: \${{ not-a-real-function() }} + cancel: bg` + }, + context + ); + + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion, + featureFlags: new FeatureFlags({allowBackgroundSteps: true}) + }); + + expect(template.errors).toBeDefined(); + expect(template.errors!.some(e => e.Message.includes(conditionError))).toBe(true); + }); }); }); diff --git a/workflow-parser/src/model/converter/steps.ts b/workflow-parser/src/model/converter/steps.ts index 9375d50f..578133a8 100644 --- a/workflow-parser/src/model/converter/steps.ts +++ b/workflow-parser/src/model/converter/steps.ts @@ -168,8 +168,8 @@ function convertStep( } if (wait) { - if (ifCondition) { - context.error(ifKeyToken ?? mapping, CONTROL_FLOW_CONDITION_ERROR); + if (ifKeyToken) { + context.error(ifKeyToken, CONTROL_FLOW_CONDITION_ERROR); } return { id: id?.value || "", @@ -180,8 +180,8 @@ function convertStep( } if (waitAll !== undefined) { - if (ifCondition) { - context.error(ifKeyToken ?? mapping, CONTROL_FLOW_CONDITION_ERROR); + if (ifKeyToken) { + context.error(ifKeyToken, CONTROL_FLOW_CONDITION_ERROR); } return { id: id?.value || "", @@ -192,8 +192,8 @@ function convertStep( } if (cancel) { - if (ifCondition) { - context.error(ifKeyToken ?? mapping, CONTROL_FLOW_CONDITION_ERROR); + if (ifKeyToken) { + context.error(ifKeyToken, CONTROL_FLOW_CONDITION_ERROR); } return { id: id?.value || "",