Validate implicit if conditions in action.yml files (#317)

## Problem

In workflow YAML files, writing `if: foo == bar` shows an error because `foo` and `bar` are not valid contexts. However, the same invalid expression in an action.yml file showed no error.

## Solution

Add expression validation for implicit `if` conditions in action.yml files, matching the behavior of workflow YAML validation.

## What's new

1. **Pre-if/post-if validation** (node and docker actions)
   - `pre-if: foo == bar` now shows error for unknown context
   - `post-if: unknownFunc()` now shows error for unknown function

2. **Composite step `if` validation** (fix)
   - Errors from `convertToIfCondition` were being lost due to call ordering
   - Now captured correctly by calling conversion before retrieving errors

## Why the refactor?

The diff includes consolidating multiple validation loops into a single `validateAllTokens()` traversal. This matches the pattern used in workflow YAML validation (`additionalValidations`), making the code consistent between the two validation paths.
This commit is contained in:
eric sciple
2026-01-27 08:37:42 -06:00
committed by GitHub
parent 46b216a6dc
commit d2f52a9043
6 changed files with 460 additions and 140 deletions
+251
View File
@@ -1011,4 +1011,255 @@ runs:
expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(true);
});
});
describe("if condition context validation", () => {
it("warns on unknown context in composite step if", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown context in if
runs:
using: composite
steps:
- if: foo == bar
run: echo hi
shell: bash
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(true);
});
it("warns on unknown context in pre-if for node action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown context in pre-if
runs:
using: node20
main: index.js
pre: setup.js
pre-if: foo == bar
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(true);
});
it("warns on unknown context in post-if for node action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown context in post-if
runs:
using: node20
main: index.js
post: cleanup.js
post-if: foo == bar
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(true);
});
it("warns on unknown context in pre-if for docker action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown context in pre-if
runs:
using: docker
image: Dockerfile
pre-entrypoint: /setup.sh
pre-if: foo == bar
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(true);
});
it("warns on unknown context in post-if for docker action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown context in post-if
runs:
using: docker
image: Dockerfile
post-entrypoint: /cleanup.sh
post-if: foo == bar
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(true);
});
it("allows valid contexts in composite step if", async () => {
const doc = createActionDocument(`
name: My Action
description: Valid context in if
runs:
using: composite
steps:
- if: github.event_name == 'push'
run: echo hi
shell: bash
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(false);
});
it("allows valid contexts in pre-if", async () => {
const doc = createActionDocument(`
name: My Action
description: Valid context in pre-if
runs:
using: node20
main: index.js
pre: setup.js
pre-if: runner.os == 'Linux'
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(false);
});
it("allows valid contexts in post-if", async () => {
const doc = createActionDocument(`
name: My Action
description: Valid context in post-if
runs:
using: node20
main: index.js
post: cleanup.js
post-if: runner.os == 'Linux'
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized named-value"))).toBe(false);
});
it("allows hashFiles function in composite step if", async () => {
const doc = createActionDocument(`
name: My Action
description: hashFiles in if
runs:
using: composite
steps:
- if: hashFiles('**/package-lock.json') != ''
run: echo hi
shell: bash
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized"))).toBe(false);
});
it("allows success, failure, always, cancelled functions in composite step if", async () => {
const doc = createActionDocument(`
name: My Action
description: Status functions in if
runs:
using: composite
steps:
- if: success() && !cancelled()
run: echo success
shell: bash
- if: failure()
run: echo failure
shell: bash
- if: always()
run: echo always
shell: bash
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized"))).toBe(false);
});
it("allows hashFiles function in pre-if", async () => {
const doc = createActionDocument(`
name: My Action
description: hashFiles in pre-if
runs:
using: node20
main: index.js
pre: setup.js
pre-if: hashFiles('**/package-lock.json') != ''
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized"))).toBe(false);
});
it("allows status functions in post-if", async () => {
const doc = createActionDocument(`
name: My Action
description: Status functions in post-if
runs:
using: node20
main: index.js
post: cleanup.js
post-if: always() || failure()
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized"))).toBe(false);
});
it("errors on unknown function in composite step if", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown function in if
runs:
using: composite
steps:
- if: unknownFunc()
run: echo hi
shell: bash
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized function"))).toBe(true);
});
it("errors on unknown function in pre-if for node action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown function in pre-if
runs:
using: node20
main: index.js
pre: setup.js
pre-if: unknownFunc()
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized function"))).toBe(true);
});
it("errors on unknown function in post-if for node action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown function in post-if
runs:
using: node20
main: index.js
post: cleanup.js
post-if: unknownFunc()
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized function"))).toBe(true);
});
it("errors on unknown function in pre-if for docker action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown function in pre-if
runs:
using: docker
image: Dockerfile
pre-entrypoint: /setup.sh
pre-if: unknownFunc()
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized function"))).toBe(true);
});
it("errors on unknown function in post-if for docker action", async () => {
const doc = createActionDocument(`
name: My Action
description: Unknown function in post-if
runs:
using: docker
image: Dockerfile
post-entrypoint: /cleanup.sh
post-if: unknownFunc()
`);
const diagnostics = await validate(doc);
expect(diagnostics.some(d => d.message.includes("Unrecognized function"))).toBe(true);
});
});
});
+113 -137
View File
@@ -4,9 +4,10 @@
import {Lexer, Parser} from "@actions/expressions";
import {Expr} from "@actions/expressions/ast";
import {isBasicExpression, isMapping, isString} from "@actions/workflow-parser";
import {isMapping, isString} from "@actions/workflow-parser";
import {isActionStep} from "@actions/workflow-parser/model/type-guards";
import {ErrorPolicy} from "@actions/workflow-parser/model/convert";
import {ActionTemplate} from "@actions/workflow-parser/actions/action-template";
import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition";
import {splitAllowedContext} from "@actions/workflow-parser/templates/allowed-context";
import {BasicExpressionToken} from "@actions/workflow-parser/templates/tokens/basic-expression-token";
@@ -75,7 +76,15 @@ export async function validateAction(textDocument: TextDocument, config?: Valida
return [];
}
// Get schema errors
// Convert the action template (this may add validation errors for pre-if/post-if)
let template: ActionTemplate | undefined;
if (result.value) {
template = getOrConvertActionTemplate(result.context, result.value, textDocument.uri, {
errorPolicy: ErrorPolicy.TryConversion
});
}
// Get schema and conversion errors (must be after conversion to include conversion errors)
const schemaErrors = result.context.errors.getErrors();
// Run custom runs key validation, which also filters redundant schema errors in place
@@ -103,13 +112,9 @@ export async function validateAction(textDocument: TextDocument, config?: Valida
}
// Validate composite action steps if we have a parsed result
if (result.value) {
const template = getOrConvertActionTemplate(result.context, result.value, textDocument.uri, {
errorPolicy: ErrorPolicy.TryConversion
});
if (result.value && template) {
// Only composite actions have steps to validate
if (template?.runs?.using === "composite") {
if (template.runs?.using === "composite") {
const steps = template.runs.steps ?? [];
// Find the steps sequence token from the raw parsed result
@@ -125,22 +130,16 @@ export async function validateAction(textDocument: TextDocument, config?: Valida
await validateActionReference(diagnostics, stepToken, step, config);
}
// Validate step tokens (uses format, if conditions)
// Validate step uses format
if (isMapping(stepToken)) {
validateCompositeStepTokens(diagnostics, stepToken);
validateStepUsesField(diagnostics, stepToken);
}
}
}
}
// Validate pre-if and post-if for node and docker actions
const runsMapping = findRunsMapping(result.value);
if (runsMapping) {
validateRunsIfConditions(diagnostics, runsMapping);
}
// Validate format() calls in all expressions throughout the action
validateAllExpressions(diagnostics, result.value);
// Single traversal for all expression validation (like workflow's additionalValidations)
validateAllTokens(diagnostics, result.value);
}
} catch (e) {
error(`Unhandled error while validating action file: ${(e as Error).message}`);
@@ -150,93 +149,124 @@ export async function validateAction(textDocument: TextDocument, config?: Valida
}
/**
* Validates tokens within a composite action step.
* Checks `uses` format and `if` literal text detection.
* Validates the `uses` field format in a composite action step.
*/
function validateCompositeStepTokens(diagnostics: Diagnostic[], stepToken: MappingToken): void {
function validateStepUsesField(diagnostics: Diagnostic[], stepToken: MappingToken): void {
for (let i = 0; i < stepToken.count; i++) {
const {key, value} = stepToken.get(i);
const keyStr = isString(key) ? key.value.toLowerCase() : "";
// Validate `uses` field format
if (keyStr === "uses" && isString(value)) {
validateStepUsesFormat(diagnostics, value);
}
// Validate `if` field for literal text outside expressions
if (keyStr === "if" && value.range) {
if (isString(value)) {
// Plain string if condition (no ${{ }} markers)
validateIfCondition(diagnostics, value);
} else if (isBasicExpression(value)) {
// Expression token - check for format() with literal text
// This happens when the parser converts "push == ${{ expr }}" to format('push == {0}', expr)
validateIfConditionExpression(diagnostics, value);
}
}
}
}
/**
* Validates an `if` condition (StringToken).
* Checks for literal text outside expressions and validates format() calls.
* Single traversal validation for all tokens in the action template.
* This follows the same pattern as workflow validation's additionalValidations:
* - For BasicExpressionToken: validate format() calls
* - For StringToken on if conditions: validate literal text detection and format() calls
* - For pre-if/post-if with explicit ${{ }}: report error (not supported by runner)
*
* Context validation (unknown named values) is handled by workflow-parser during conversion.
*/
function validateIfCondition(diagnostics: Diagnostic[], token: StringToken): void {
function validateAllTokens(diagnostics: Diagnostic[], root: TemplateToken): void {
for (const [parent, token] of TemplateToken.traverse(root)) {
const definitionKey = token.definition?.key;
// Validate all BasicExpressionToken instances for format() calls
if (token instanceof BasicExpressionToken && token.range) {
// Check for literal text in if conditions (format with literal text)
if (definitionKey === "step-if") {
validateIfLiteralText(diagnostics, token);
}
// Validate format() calls for all expressions
for (const expression of token.originalExpressions || [token]) {
validateExpressionFormatCalls(diagnostics, expression);
}
// Check for explicit ${{ }} in pre-if/post-if (not supported by runner)
if (definitionKey === "runs-if" && parent instanceof MappingToken) {
// Resolve the key name (pre-if or post-if) from parent mapping
let keyName: string | undefined;
for (let i = 0; i < parent.count; i++) {
const {key, value} = parent.get(i);
if (value === token) {
keyName = key.toString().toLowerCase();
break;
}
}
if (keyName) {
diagnostics.push({
message: `Explicit expression syntax \${{ }} is not supported for '${keyName}'. Remove the \${{ }} markers and use the expression directly.`,
range: mapRange(token.range),
severity: DiagnosticSeverity.Error,
code: "explicit-expression-not-allowed"
});
}
}
}
// Handle implicit if conditions (StringToken without ${{ }})
// These allow expression syntax without the markers
if (isString(token) && token.range) {
if (definitionKey === "step-if" || definitionKey === "runs-if") {
validateImplicitIfCondition(diagnostics, token);
}
}
}
}
const LITERAL_TEXT_IN_CONDITION_MESSAGE =
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?";
const LITERAL_TEXT_IN_CONDITION_CODE = "expression-literal-text-in-condition";
/**
* Validates an implicit if condition (StringToken without ${{ }}).
* Checks for literal text detection and validates format() calls.
*/
function validateImplicitIfCondition(diagnostics: Diagnostic[], token: StringToken): void {
const condition = token.value.trim();
if (!condition) {
return;
}
// Get allowed context for step-if from the token's definition
const allowedContext = token.definitionInfo?.allowedContext || [];
const {namedContexts, functions} = splitAllowedContext(allowedContext);
// Ensure the condition has a status function, wrapping if needed
const finalCondition = ensureStatusFunction(condition, token.definitionInfo);
// Create a BasicExpressionToken for validation
const expressionToken = new BasicExpressionToken(
token.file,
token.range,
finalCondition,
token.definitionInfo,
undefined,
token.source,
undefined,
token.blockScalarHeader
);
// Check for literal text in the expression (format with literal text)
try {
const l = new Lexer(expressionToken.expression);
const l = new Lexer(finalCondition);
const lr = l.lex();
const p = new Parser(lr.tokens, namedContexts, functions);
const expr = p.parse();
// Check for literal text in the expression (format with literal text)
if (hasFormatWithLiteralText(expr)) {
diagnostics.push({
message:
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
message: LITERAL_TEXT_IN_CONDITION_MESSAGE,
range: mapRange(token.range),
severity: DiagnosticSeverity.Error,
code: "expression-literal-text-in-condition"
code: LITERAL_TEXT_IN_CONDITION_CODE
});
}
// Validate format() function calls
validateFormatCallsAndAddDiagnostics(diagnostics, expr, token.range);
} catch {
// Ignore parse errors here - they'll be caught by schema validation
// Ignore parse errors - they'll be caught by schema validation or workflow-parser
}
}
/**
* Validates an `if` condition (BasicExpressionToken).
* Checks for literal text outside expressions.
* Called when the parser has converted "push == ${{ expr }}" to format('push == {0}', expr).
* Note: format() validation is handled by validateAllExpressions for BasicExpressionTokens.
* Validates a BasicExpressionToken for literal text in if conditions.
*/
function validateIfConditionExpression(diagnostics: Diagnostic[], token: BasicExpressionToken): void {
function validateIfLiteralText(diagnostics: Diagnostic[], token: BasicExpressionToken): void {
const allowedContext = token.definitionInfo?.allowedContext || [];
const {namedContexts, functions} = splitAllowedContext(allowedContext);
@@ -248,16 +278,33 @@ function validateIfConditionExpression(diagnostics: Diagnostic[], token: BasicEx
if (hasFormatWithLiteralText(expr)) {
diagnostics.push({
message:
"Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?",
message: LITERAL_TEXT_IN_CONDITION_MESSAGE,
range: mapRange(token.range),
severity: DiagnosticSeverity.Error,
code: "expression-literal-text-in-condition"
code: LITERAL_TEXT_IN_CONDITION_CODE
});
}
// Note: format() validation is done by validateAllExpressions() for all BasicExpressionTokens
} catch {
// Ignore parse errors here - they'll be caught by schema validation
// Ignore parse errors - they'll be caught by schema validation or workflow-parser
}
}
/**
* Validates format() function calls in an expression token.
*/
function validateExpressionFormatCalls(diagnostics: Diagnostic[], token: BasicExpressionToken): void {
const allowedContext = token.definitionInfo?.allowedContext || [];
const {namedContexts, functions} = splitAllowedContext(allowedContext);
try {
const l = new Lexer(token.expression);
const lr = l.lex();
const p = new Parser(lr.tokens, namedContexts, functions);
const expr = p.parse();
validateFormatCallsAndAddDiagnostics(diagnostics, expr, token.range);
} catch {
// Ignore parse errors - they'll be caught by schema validation
}
}
@@ -304,77 +351,6 @@ function findStepsSequence(root: TemplateToken): SequenceToken | undefined {
return undefined;
}
/**
* Find the runs mapping token from the raw action template.
*/
function findRunsMapping(root: TemplateToken): MappingToken | undefined {
if (root instanceof MappingToken) {
for (let i = 0; i < root.count; i++) {
const {key, value} = root.get(i);
if (key.toString().toLowerCase() === "runs" && value instanceof MappingToken) {
return value;
}
}
}
return undefined;
}
/**
* Validates pre-if and post-if conditions at the runs level (for node and docker actions).
* Checks for literal text outside expressions that would always be truthy.
*/
function validateRunsIfConditions(diagnostics: Diagnostic[], runsMapping: MappingToken): void {
for (let i = 0; i < runsMapping.count; i++) {
const {key, value} = runsMapping.get(i);
const keyStr = key.toString().toLowerCase();
// Validate pre-if and post-if fields for literal text
if ((keyStr === "pre-if" || keyStr === "post-if") && value.range) {
if (isString(value)) {
// Plain string condition (no ${{ }} markers)
validateIfCondition(diagnostics, value);
} else if (isBasicExpression(value)) {
// The runner doesn't support explicit ${{ }} syntax for pre-if/post-if
// Only implicit expressions are allowed
diagnostics.push({
message: `Explicit expression syntax \${{ }} is not supported for '${keyStr}'. Remove the \${{ }} markers and use the expression directly.`,
range: mapRange(value.range),
severity: DiagnosticSeverity.Error,
code: "explicit-expression-not-allowed"
});
}
}
}
}
/**
* Validates format() function calls in all expressions throughout the action template.
* This catches format string errors in any expression, not just if conditions.
*/
function validateAllExpressions(diagnostics: Diagnostic[], root: TemplateToken): void {
for (const [, token] of TemplateToken.traverse(root)) {
if (token instanceof BasicExpressionToken) {
// Process original expressions if available (for combined expressions like "${{ a }} text ${{ b }}")
// This ensures error ranges point to the correct original expression location
for (const expression of token.originalExpressions || [token]) {
const allowedContext = expression.definitionInfo?.allowedContext || [];
const {namedContexts, functions} = splitAllowedContext(allowedContext);
try {
const l = new Lexer(expression.expression);
const lr = l.lex();
const p = new Parser(lr.tokens, namedContexts, functions);
const expr = p.parse();
validateFormatCallsAndAddDiagnostics(diagnostics, expr, expression.range);
} catch {
// Ignore parse errors - they'll be caught by schema validation
}
}
}
}
}
/**
* Validates that the keys under `runs:` are valid for the specified `using:` type.
* Also filters out schema errors (in place) that this validation replaces with more specific messages.
@@ -160,6 +160,21 @@ jobs:
})
);
});
it("errors on unknown context in plain string if condition", async () => {
const input = `
on: push
jobs:
build:
runs-on: ubuntu-latest
steps:
- if: foo == bar
run: echo hi
`;
const result = await validate(createDocument("wf.yaml", input));
expect(result.some(d => d.message.includes("Unrecognized named-value"))).toBe(true);
});
});
describe("snapshot-if", () => {
@@ -317,4 +317,53 @@ runs:
}
}
});
it("reports error for invalid context in pre-if", () => {
const content = `
name: Node Action
description: A node action
runs:
using: node20
main: dist/index.js
pre: dist/setup.js
pre-if: foo == bar`;
const result = parseAction({name: "action.yml", content}, nullTrace);
expect(result.value).toBeDefined();
if (!result.value) return;
// Should have no errors before conversion
expect(result.context.errors.count).toBe(0);
// Convert the template - this should add the validation error
convertActionTemplate(result.context, result.value);
// Should have an error now about invalid context
expect(result.context.errors.count).toBeGreaterThan(0);
const errors = result.context.errors.getErrors();
expect(errors.some(e => e.rawMessage.includes("foo"))).toBe(true);
});
it("accepts valid context in pre-if", () => {
const content = `
name: Node Action
description: A node action
runs:
using: node20
main: dist/index.js
pre: dist/setup.js
pre-if: runner.os == 'Linux'`;
const result = parseAction({name: "action.yml", content}, nullTrace);
expect(result.value).toBeDefined();
if (!result.value) return;
const template = convertActionTemplate(result.context, result.value);
// Should have no errors
expect(result.context.errors.count).toBe(0);
if (template.runs.using === "node20") {
expect(template.runs.preIf).toBe("runner.os == 'Linux'");
}
});
});
@@ -9,7 +9,7 @@ import {TemplateContext} from "../templates/template-context.js";
import {isBoolean, isMapping, isScalar, isSequence, isString} from "../templates/tokens/type-guards.js";
import {ErrorPolicy} from "../model/convert.js";
import {Step} from "../model/workflow-template.js";
import {convertToIfCondition} from "../model/converter/if-condition.js";
import {convertToIfCondition, validateRunsIfCondition} from "../model/converter/if-condition.js";
/**
* Represents a parsed and converted action.yml file
@@ -310,7 +310,7 @@ function convertRuns(context: TemplateContext, token: TemplateToken): ActionRuns
case "pre-if":
if (isString(item.value)) {
preIf = item.value.value;
preIf = validateRunsIfCondition(context, item.value, item.value.value);
}
break;
@@ -322,7 +322,7 @@ function convertRuns(context: TemplateContext, token: TemplateToken): ActionRuns
case "post-if":
if (isString(item.value)) {
postIf = item.value.value;
postIf = validateRunsIfCondition(context, item.value, item.value.value);
}
break;
@@ -136,3 +136,32 @@ function walkTreeToFindStatusFunctionCalls(tree: Expr | undefined): boolean {
return false;
}
/**
* Validates a pre-if or post-if condition string.
* Unlike step if conditions, pre-if and post-if are evaluated as-is by the runner
* (they default to always() only when the field is missing entirely).
* This function validates the expression and reports errors through the context.
*
* @param context The template context for error reporting
* @param token The token containing the condition
* @param condition The condition string to validate
* @returns The validated condition string, or undefined on error
*/
export function validateRunsIfCondition(
context: TemplateContext,
token: TemplateToken,
condition: string
): string | undefined {
const allowedContext = token.definitionInfo?.allowedContext || [];
// Validate the expression directly - no wrapping needed for pre-if/post-if
try {
ExpressionToken.validateExpression(condition, allowedContext);
} catch (err) {
context.error(token, err as Error);
return undefined;
}
return condition;
}