diff --git a/languageservice/src/expression-hover/expression-pos.test.ts b/languageservice/src/expression-hover/expression-pos.test.ts index 9bcc6b7..47e538c 100644 --- a/languageservice/src/expression-hover/expression-pos.test.ts +++ b/languageservice/src/expression-hover/expression-pos.test.ts @@ -69,6 +69,59 @@ jobs: } }); }); + + it("job-level if condition without status function (gets wrapped)", () => { + expect( + testMapToExpressionPos(`on: push +jobs: + build: + if: git|hub.event_name == 'push' + runs-on: ubuntu-latest`) + ).toEqual({ + expression: "success() && (github.event_name == 'push')", + position: {line: 0, column: 17}, // "success() && (".length + 3 = 17 + documentRange: { + start: {line: 3, character: 8}, + end: {line: 3, character: 35} // End of the original condition in the document + } + }); + }); + + it("job-level if condition with status function (not wrapped)", () => { + expect( + testMapToExpressionPos(`on: push +jobs: + build: + if: alw|ays() + runs-on: ubuntu-latest`) + ).toEqual({ + expression: "always()", + position: {line: 0, column: 3}, + documentRange: { + start: {line: 3, character: 8}, + end: {line: 3, character: 16} + } + }); + }); + + it("step-level if condition without status function (gets wrapped)", () => { + expect( + testMapToExpressionPos(`on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - if: steps.test.outc|ome == 'success' + run: echo hello`) + ).toEqual({ + expression: "success() && (steps.test.outcome == 'success')", + position: {line: 0, column: 29}, // Actual position in the wrapped expression + documentRange: { + start: {line: 5, character: 12}, + end: {line: 5, character: 43} // End of the original condition in the document + } + }); + }); }); function testMapToExpressionPos(input: string) { diff --git a/languageservice/src/expression-hover/expression-pos.ts b/languageservice/src/expression-hover/expression-pos.ts index c01e659..01c0156 100644 --- a/languageservice/src/expression-hover/expression-pos.ts +++ b/languageservice/src/expression-hover/expression-pos.ts @@ -1,4 +1,5 @@ import {Pos} from "@actions/expressions/lexer"; +import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition"; import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token"; import {isBasicExpression, isString} from "@actions/workflow-parser/templates/tokens/type-guards"; import {Position, Range as LSPRange} from "vscode-languageserver-textdocument"; @@ -42,14 +43,14 @@ export function mapToExpressionPos(token: TemplateToken, position: Position): Ex ) { const condition = token.value.trim(); if (condition) { - // Check if the condition already contains a status function - const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition); - const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`; + // Ensure the condition has a status function, wrapping if needed + const finalCondition = ensureStatusFunction(condition, token.definitionInfo); const exprRange = mapRange(token.range); - // Calculate offset for wrapping - const offset = hasStatusFunction ? 0 : "success() && (".length; + // Calculate offset: find where the original condition appears in the final expression + // If wrapped, it will be after "success() && (", otherwise it's at position 0 + const offset = finalCondition.indexOf(condition); return { expression: finalCondition, diff --git a/languageservice/src/validate.expressions.test.ts b/languageservice/src/validate.expressions.test.ts index 94f5041..a237787 100644 --- a/languageservice/src/validate.expressions.test.ts +++ b/languageservice/src/validate.expressions.test.ts @@ -1505,4 +1505,174 @@ jobs: expect(result).toEqual([]); }); }); + + describe("if condition context restrictions", () => { + describe("job-level if", () => { + it("allows github context", async () => { + const input = ` +on: push +jobs: + build: + if: github.event_name == 'push' + runs-on: ubuntu-latest + steps: + - run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows needs context", async () => { + const input = ` +on: push +jobs: + a: + runs-on: ubuntu-latest + steps: + - run: echo hello + b: + needs: a + if: needs.a.result == 'success' + runs-on: ubuntu-latest + steps: + - run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows inputs context", async () => { + const input = ` +on: + workflow_dispatch: + inputs: + environment: + type: string +jobs: + build: + if: inputs.environment == 'prod' + runs-on: ubuntu-latest + steps: + - run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + // Note: vars and matrix contexts are validated at runtime based on their existence + // vars context only exists if organization/repository variables are defined + // matrix context only exists if a strategy.matrix is defined + }); + + describe("step-level if", () => { + it("allows steps context", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - id: setup + run: echo hello + - if: steps.setup.outcome == 'success' + run: echo world`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows job context", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - if: job.status == 'success' + run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows runner context", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - if: runner.os == 'Linux' + run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows env context", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + env: + MY_VAR: value + steps: + - if: env.MY_VAR == 'value' + run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows matrix context in matrix job", async () => { + const input = ` +on: push +jobs: + build: + strategy: + matrix: + os: [ubuntu, windows] + runs-on: ubuntu-latest + steps: + - if: matrix.os == 'ubuntu' + run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows hashFiles function", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - if: hashFiles('**/*.txt') != '' + run: echo hello`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + + it("allows all contexts together", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + env: + JOB_VAR: job-value + steps: + - id: first + run: echo hello + - if: github.event_name == 'push' && steps.first.outcome == 'success' && job.status == 'success' && runner.os == 'Linux' && env.JOB_VAR == 'job-value' + run: echo world`; + + const result = await validate(createDocument("wf.yaml", input)); + expect(result).toEqual([]); + }); + }); + }); }); diff --git a/languageservice/src/validate.ts b/languageservice/src/validate.ts index d250258..235557b 100644 --- a/languageservice/src/validate.ts +++ b/languageservice/src/validate.ts @@ -2,6 +2,7 @@ import {Lexer, Parser} from "@actions/expressions"; import {Expr} from "@actions/expressions/ast"; import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isString} from "@actions/workflow-parser"; import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; +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"; import {StringToken} from "@actions/workflow-parser/templates/tokens/string-token"; @@ -118,9 +119,8 @@ async function additionalValidations( // Convert the string to an expression token for validation const condition = token.value.trim(); if (condition) { - // Check if the condition already contains a status function - const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition); - const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`; + // 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( diff --git a/workflow-parser/src/model/convert.test.ts b/workflow-parser/src/model/convert.test.ts index ed92dd1..d15584d 100644 --- a/workflow-parser/src/model/convert.test.ts +++ b/workflow-parser/src/model/convert.test.ts @@ -382,4 +382,200 @@ jobs: ] }); }); + + describe("if condition context validation", () => { + it("validates job-level if with allowed contexts", async () => { + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + if: github.event_name == 'push' && needs.test.result == 'success' + needs: test + runs-on: ubuntu-latest + test: + runs-on: ubuntu-latest` + }, + nullTrace + ); + + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + + // Should convert successfully - github and needs are allowed in job-level if + expect(result.context.errors.getErrors()).toHaveLength(0); + expect(template.jobs).toHaveLength(2); + }); + + it("validates job-level if rejects disallowed contexts", async () => { + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + if: steps.test.outcome == 'success' + runs-on: ubuntu-latest + steps: + - id: test + run: echo hello` + }, + nullTrace + ); + + await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + + // Should have error - steps context not allowed in job-level if + const errors = result.context.errors.getErrors(); + expect(errors.length).toBeGreaterThan(0); + const errorMessages = errors.map(e => e.message).join(" "); + expect(errorMessages.toLowerCase()).toMatch(/steps|context/); + }); + + it("validates step-level if allows all contexts", async () => { + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - id: first + run: echo hello + - if: steps.first.outcome == 'success' && job.status == 'success' + run: echo world` + }, + nullTrace + ); + + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + + // Should convert successfully - steps and job contexts allowed in step-level if + expect(result.context.errors.getErrors()).toHaveLength(0); + expect(template.jobs).toHaveLength(1); + }); + + it("handles case-insensitive status functions in if conditions", async () => { + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - if: Success() + run: echo "uppercase Success" + - if: FAILURE() + run: echo "uppercase FAILURE" + - if: Cancelled() || Always() + run: echo "mixed case"` + }, + nullTrace + ); + + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + + // Should convert successfully - status functions are case-insensitive + expect(result.context.errors.getErrors()).toHaveLength(0); + expect(template.jobs).toHaveLength(1); + + // Verify the conditions are preserved without wrapping in success() && + const job = template.jobs[0]; + expect(job.type).toBe("job"); + if (job.type === "job") { + expect(job.steps[0].if?.expression).toBe("Success()"); + expect(job.steps[1].if?.expression).toBe("FAILURE()"); + expect(job.steps[2].if?.expression).toBe("Cancelled() || Always()"); + } + }); + + it("handles empty if condition", async () => { + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + job1: + if: "" + runs-on: ubuntu-latest + steps: + - run: echo hello + job2: + if: '' + runs-on: ubuntu-latest + steps: + - if: "" + run: echo world + - if: '' + run: echo test` + }, + nullTrace + ); + + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + + // Empty conditions should default to success() + expect(result.context.errors.getErrors()).toHaveLength(0); + expect(template.jobs).toHaveLength(2); + + const job1 = template.jobs[0]; + expect(job1.if?.expression).toBe("success()"); + + const job2 = template.jobs[1]; + expect(job2.if?.expression).toBe("success()"); + + if (job2.type === "job") { + expect(job2.steps[0].if?.expression).toBe("success()"); + expect(job2.steps[1].if?.expression).toBe("success()"); + } + }); + + it("handles status functions with property access", async () => { + const result = parseWorkflow( + { + name: "wf.yaml", + content: `on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - if: success().outputs.result + run: echo "success with property" + - if: failure().outputs.value + run: echo "failure with property" + - if: always() && steps.test.outcome + run: echo "always with &&"` + }, + nullTrace + ); + + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + + // Should not wrap - status functions are present even with property access + expect(result.context.errors.getErrors()).toHaveLength(0); + expect(template.jobs).toHaveLength(1); + + const job = template.jobs[0]; + expect(job.type).toBe("job"); + if (job.type === "job") { + expect(job.steps[0].if?.expression).toBe("success().outputs.result"); + expect(job.steps[1].if?.expression).toBe("failure().outputs.value"); + expect(job.steps[2].if?.expression).toBe("always() && steps.test.outcome"); + } + }); + }); }); diff --git a/workflow-parser/src/model/converter/if-condition.ts b/workflow-parser/src/model/converter/if-condition.ts index d07c4a1..66dbb18 100644 --- a/workflow-parser/src/model/converter/if-condition.ts +++ b/workflow-parser/src/model/converter/if-condition.ts @@ -1,24 +1,66 @@ +import {Lexer, Parser} from "@actions/expressions"; +import {Binary, Expr, FunctionCall, Grouping, IndexAccess, Logical, Unary} from "@actions/expressions/ast"; +import {DefinitionInfo} from "../../templates/schema/definition-info"; +import {splitAllowedContext} from "../../templates/allowed-context"; import {TemplateContext} from "../../templates/template-context"; import {BasicExpressionToken, ExpressionToken, TemplateToken} from "../../templates/tokens"; +/** + * Ensures a condition expression contains a status function call. + * If the condition doesn't contain success(), failure(), cancelled(), or always(), + * wraps it in `success() && (condition)`. + * + * Parses the expression to accurately detect status functions, avoiding false positives + * from string literals or property access. If parsing fails (e.g., partially typed expression), + * returns the original condition unchanged to allow validation to report the actual error. + * + * @param condition The condition expression to check + * @param definitionInfo Schema definition containing allowed contexts for parsing + * @returns The condition with status function guaranteed, or original on parse error + */ +export function ensureStatusFunction(condition: string, definitionInfo: DefinitionInfo | undefined): string { + const allowedContext = definitionInfo?.allowedContext || []; + + try { + const {namedContexts, functions} = splitAllowedContext(allowedContext); + const lexer = new Lexer(condition); + const result = lexer.lex(); + const parser = new Parser(result.tokens, namedContexts, functions); + const tree = parser.parse(); + + // Check if tree contains status function + if (walkTreeToFindStatusFunctionCalls(tree)) { + return condition; // Already has status function + } + + // Wrap it + return `success() && (${condition})`; + } catch { + // Parse error - return original and let validation report the actual error + // This is important for hover/autocomplete on partially-typed expressions + return condition; + } +} + /** * Converts an if condition token to a BasicExpressionToken. - * Similar to Go's convertToIfCondition - treats the value as a string and parses it as an expression. + * Treats the value as a string and parses it as an expression. * Wraps the condition in success() && (...) if it doesn't already contain a status function. * This allows both 'if: success()' and 'if: ${{ success() }}' to work correctly. * + * Reads the allowed context directly from the schema definition attached to the token, + * ensuring consistency with the schema. + * * @param context The template context for error reporting * @param token The token containing the if condition - * @param allowedContext The allowed expression contexts (varies by job-if vs step-if vs snapshot-if) * @returns A BasicExpressionToken with the processed condition, or undefined on error */ -export function convertToIfCondition( - context: TemplateContext, - token: TemplateToken, - allowedContext: string[] -): BasicExpressionToken | undefined { +export function convertToIfCondition(context: TemplateContext, token: TemplateToken): BasicExpressionToken | undefined { const scalar = token.assertScalar("if condition"); + // Get allowed context from the schema definition attached to the token + const allowedContext = token.definitionInfo?.allowedContext || []; + // If it's already an expression, use its value let condition: string; let source: string | undefined; @@ -33,21 +75,13 @@ export function convertToIfCondition( source = stringToken.source; } + let finalCondition: string; if (!condition) { // Empty condition defaults to success() - return new BasicExpressionToken(token.file, token.range, "success()", token.definitionInfo, undefined, undefined); - } - - // Check if the condition already contains a status function (success, failure, cancelled, always) - // This is a simple check - just look for these function names - const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition); - - let finalCondition: string; - if (hasStatusFunction) { - finalCondition = condition; + finalCondition = "success()"; } else { - // Wrap in success() && (condition) - finalCondition = `success() && (${condition})`; + // Ensure the condition has a status function, wrapping if needed + finalCondition = ensureStatusFunction(condition, token.definitionInfo); } // Validate the expression before creating the token @@ -63,57 +97,42 @@ export function convertToIfCondition( } /** - * Allowed context for job-level if conditions + * Walks an expression AST to find status function calls (success, failure, cancelled, always). + * Recursively checks all nodes including function arguments and logical/binary operations. */ -export const JOB_IF_CONTEXT = [ - "github", - "inputs", - "vars", - "needs", - "always(0,0)", - "failure(0,MAX)", - "cancelled(0,0)", - "success(0,MAX)" -]; +function walkTreeToFindStatusFunctionCalls(tree: Expr | undefined): boolean { + if (!tree) { + return false; + } -/** - * Allowed context for step-level if conditions - */ -export const STEP_IF_CONTEXT = [ - "github", - "inputs", - "vars", - "needs", - "strategy", - "matrix", - "steps", - "job", - "runner", - "env", - "always(0,0)", - "failure(0,0)", - "cancelled(0,0)", - "success(0,0)", - "hashFiles(1,255)" -]; + if (tree instanceof FunctionCall) { + const funcName = tree.functionName.lexeme.toLowerCase(); + if (funcName === "success" || funcName === "failure" || funcName === "cancelled" || funcName === "always") { + return true; + } + // Check arguments recursively + return tree.args.some(arg => walkTreeToFindStatusFunctionCalls(arg)); + } -/** - * Allowed context for snapshot-level if conditions - */ -export const SNAPSHOT_IF_CONTEXT = [ - "github", - "inputs", - "vars", - "needs", - "strategy", - "matrix", - "steps", - "job", - "runner", - "env", - "always(0,0)", - "failure(0,0)", - "cancelled(0,0)", - "success(0,0)", - "hashFiles(1,255)" -]; + if (tree instanceof Binary) { + return walkTreeToFindStatusFunctionCalls(tree.left) || walkTreeToFindStatusFunctionCalls(tree.right); + } + + if (tree instanceof Unary) { + return walkTreeToFindStatusFunctionCalls(tree.expr); + } + + if (tree instanceof Logical) { + return tree.args.some(arg => walkTreeToFindStatusFunctionCalls(arg)); + } + + if (tree instanceof Grouping) { + return walkTreeToFindStatusFunctionCalls(tree.group); + } + + if (tree instanceof IndexAccess) { + return walkTreeToFindStatusFunctionCalls(tree.expr) || walkTreeToFindStatusFunctionCalls(tree.index); + } + + return false; +} diff --git a/workflow-parser/src/model/converter/job.ts b/workflow-parser/src/model/converter/job.ts index 5338b73..c3081f3 100644 --- a/workflow-parser/src/model/converter/job.ts +++ b/workflow-parser/src/model/converter/job.ts @@ -2,7 +2,7 @@ import {TemplateContext} from "../../templates/template-context"; import {BasicExpressionToken, MappingToken, ScalarToken, StringToken, TemplateToken} from "../../templates/tokens"; import {isSequence, isString} from "../../templates/tokens/type-guards"; import {Step, WorkflowJob} from "../workflow-template"; -import {convertToIfCondition, JOB_IF_CONTEXT} from "./if-condition"; +import {convertToIfCondition} from "./if-condition"; import {convertConcurrency} from "./concurrency"; import {convertToJobContainer, convertToJobServices} from "./container"; import {handleTemplateTokenErrors} from "./handle-errors"; @@ -62,7 +62,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: break; case "if": - ifCondition = convertToIfCondition(context, item.value, JOB_IF_CONTEXT); + ifCondition = convertToIfCondition(context, item.value); break; case "name": diff --git a/workflow-parser/src/model/converter/steps.ts b/workflow-parser/src/model/converter/steps.ts index ead5ea5..4745949 100644 --- a/workflow-parser/src/model/converter/steps.ts +++ b/workflow-parser/src/model/converter/steps.ts @@ -2,7 +2,7 @@ import {TemplateContext} from "../../templates/template-context"; import {BasicExpressionToken, MappingToken, ScalarToken, StringToken, TemplateToken} from "../../templates/tokens"; import {isSequence} from "../../templates/tokens/type-guards"; import {isActionStep} from "../type-guards"; -import {convertToIfCondition, STEP_IF_CONTEXT} from "./if-condition"; +import {convertToIfCondition} from "./if-condition"; import {ActionStep, Step} from "../workflow-template"; import {handleTemplateTokenErrors} from "./handle-errors"; import {IdBuilder} from "./id-builder"; @@ -79,7 +79,7 @@ function convertStep(context: TemplateContext, idBuilder: IdBuilder, step: Templ env = item.value.assertMapping("step env"); break; case "if": - ifCondition = convertToIfCondition(context, item.value, STEP_IF_CONTEXT); + ifCondition = convertToIfCondition(context, item.value); break; case "continue-on-error": if (!item.value.isExpression) { @@ -138,10 +138,3 @@ function createActionStepId(step: ActionStep): string { return ""; } - -/** - * Converts an if condition token to a BasicExpressionToken. - * Similar to Go's convertToIfCondition - treats the value as a string and parses it as an expression. - * Wraps the condition in success() && (...) if it doesn't already contain a status function. - * This allows both 'if: success()' and 'if: ${{ success() }}' to work correctly. - */ diff --git a/workflow-parser/testdata/skipped-tests.txt b/workflow-parser/testdata/skipped-tests.txt index 7afa98c..9f0ac87 100644 --- a/workflow-parser/testdata/skipped-tests.txt +++ b/workflow-parser/testdata/skipped-tests.txt @@ -83,7 +83,6 @@ reusable-workflow-job-permissions-overrides-default-write.yml reusable-workflow-job-permissions-overrides-workflow-level.yml root-env-defaults.yml round-to-infinity.yml -step-if.yml scientific-notation-number.yml skip-reusable-workflows.yml workflow-defaults.yml