diff --git a/languageservice/src/validate-action.test.ts b/languageservice/src/validate-action.test.ts index 887fb12..aad72f8 100644 --- a/languageservice/src/validate-action.test.ts +++ b/languageservice/src/validate-action.test.ts @@ -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); + }); + }); }); diff --git a/languageservice/src/validate-action.ts b/languageservice/src/validate-action.ts index d06939c..bbe803e 100644 --- a/languageservice/src/validate-action.ts +++ b/languageservice/src/validate-action.ts @@ -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. diff --git a/languageservice/src/validate.expressions-literal-text.test.ts b/languageservice/src/validate.expressions-literal-text.test.ts index ae3e97e..8741b6d 100644 --- a/languageservice/src/validate.expressions-literal-text.test.ts +++ b/languageservice/src/validate.expressions-literal-text.test.ts @@ -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", () => { diff --git a/workflow-parser/src/actions/action-parser.test.ts b/workflow-parser/src/actions/action-parser.test.ts index 0e76e06..35f13de 100644 --- a/workflow-parser/src/actions/action-parser.test.ts +++ b/workflow-parser/src/actions/action-parser.test.ts @@ -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'"); + } + }); }); diff --git a/workflow-parser/src/actions/action-template.ts b/workflow-parser/src/actions/action-template.ts index f701d38..f2c64fe 100644 --- a/workflow-parser/src/actions/action-template.ts +++ b/workflow-parser/src/actions/action-template.ts @@ -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; diff --git a/workflow-parser/src/model/converter/if-condition.ts b/workflow-parser/src/model/converter/if-condition.ts index 6914d3d..c991167 100644 --- a/workflow-parser/src/model/converter/if-condition.ts +++ b/workflow-parser/src/model/converter/if-condition.ts @@ -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; +}