Skip to content

Commit 03d68e8

Browse files
ericscipleericsciple
authored
Refactor if-condition to use schema-driven validation and AST-based status function detection (#218)
- Read allowed context from schema definition instead of hardcoded constants - Parse expressions into AST to accurately detect status functions (avoids false positives from string literals) - Export ensureStatusFunction helper that combines checking and wrapping logic - Remove step-if.yml from skipped tests (now passes with accurate detection) - Add tests for if-condition wrapping in hover/completion position mapping
1 parent bad1fb9 commit 03d68e8

File tree

9 files changed

+521
-90
lines changed

9 files changed

+521
-90
lines changed

‎languageservice/src/expression-hover/expression-pos.test.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,59 @@ jobs:
6969
}
7070
});
7171
});
72+
73+
it("job-level if condition without status function (gets wrapped)", () => {
74+
expect(
75+
testMapToExpressionPos(`on: push
76+
jobs:
77+
build:
78+
if: git|hub.event_name == 'push'
79+
runs-on: ubuntu-latest`)
80+
).toEqual<ExpressionPos>({
81+
expression: "success() && (github.event_name == 'push')",
82+
position: {line: 0, column: 17}, // "success() && (".length + 3 = 17
83+
documentRange: {
84+
start: {line: 3, character: 8},
85+
end: {line: 3, character: 35} // End of the original condition in the document
86+
}
87+
});
88+
});
89+
90+
it("job-level if condition with status function (not wrapped)", () => {
91+
expect(
92+
testMapToExpressionPos(`on: push
93+
jobs:
94+
build:
95+
if: alw|ays()
96+
runs-on: ubuntu-latest`)
97+
).toEqual<ExpressionPos>({
98+
expression: "always()",
99+
position: {line: 0, column: 3},
100+
documentRange: {
101+
start: {line: 3, character: 8},
102+
end: {line: 3, character: 16}
103+
}
104+
});
105+
});
106+
107+
it("step-level if condition without status function (gets wrapped)", () => {
108+
expect(
109+
testMapToExpressionPos(`on: push
110+
jobs:
111+
build:
112+
runs-on: ubuntu-latest
113+
steps:
114+
- if: steps.test.outc|ome == 'success'
115+
run: echo hello`)
116+
).toEqual<ExpressionPos>({
117+
expression: "success() && (steps.test.outcome == 'success')",
118+
position: {line: 0, column: 29}, // Actual position in the wrapped expression
119+
documentRange: {
120+
start: {line: 5, character: 12},
121+
end: {line: 5, character: 43} // End of the original condition in the document
122+
}
123+
});
124+
});
72125
});
73126

74127
function testMapToExpressionPos(input: string) {

‎languageservice/src/expression-hover/expression-pos.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {Pos} from "@actions/expressions/lexer";
2+
import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition";
23
import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token";
34
import {isBasicExpression, isString} from "@actions/workflow-parser/templates/tokens/type-guards";
45
import {Position, Range as LSPRange} from "vscode-languageserver-textdocument";
@@ -42,14 +43,14 @@ export function mapToExpressionPos(token: TemplateToken, position: Position): Ex
4243
) {
4344
const condition = token.value.trim();
4445
if (condition) {
45-
// Check if the condition already contains a status function
46-
const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition);
47-
const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`;
46+
// Ensure the condition has a status function, wrapping if needed
47+
const finalCondition = ensureStatusFunction(condition, token.definitionInfo);
4848

4949
const exprRange = mapRange(token.range);
5050

51-
// Calculate offset for wrapping
52-
const offset = hasStatusFunction ? 0 : "success() && (".length;
51+
// Calculate offset: find where the original condition appears in the final expression
52+
// If wrapped, it will be after "success() && (", otherwise it's at position 0
53+
const offset = finalCondition.indexOf(condition);
5354

5455
return {
5556
expression: finalCondition,

‎languageservice/src/validate.expressions.test.ts‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1505,4 +1505,174 @@ jobs:
15051505
expect(result).toEqual([]);
15061506
});
15071507
});
1508+
1509+
describe("if condition context restrictions", () => {
1510+
describe("job-level if", () => {
1511+
it("allows github context", async () => {
1512+
const input = `
1513+
on: push
1514+
jobs:
1515+
build:
1516+
if: github.event_name == 'push'
1517+
runs-on: ubuntu-latest
1518+
steps:
1519+
- run: echo hello`;
1520+
1521+
const result = await validate(createDocument("wf.yaml", input));
1522+
expect(result).toEqual([]);
1523+
});
1524+
1525+
it("allows needs context", async () => {
1526+
const input = `
1527+
on: push
1528+
jobs:
1529+
a:
1530+
runs-on: ubuntu-latest
1531+
steps:
1532+
- run: echo hello
1533+
b:
1534+
needs: a
1535+
if: needs.a.result == 'success'
1536+
runs-on: ubuntu-latest
1537+
steps:
1538+
- run: echo hello`;
1539+
1540+
const result = await validate(createDocument("wf.yaml", input));
1541+
expect(result).toEqual([]);
1542+
});
1543+
1544+
it("allows inputs context", async () => {
1545+
const input = `
1546+
on:
1547+
workflow_dispatch:
1548+
inputs:
1549+
environment:
1550+
type: string
1551+
jobs:
1552+
build:
1553+
if: inputs.environment == 'prod'
1554+
runs-on: ubuntu-latest
1555+
steps:
1556+
- run: echo hello`;
1557+
1558+
const result = await validate(createDocument("wf.yaml", input));
1559+
expect(result).toEqual([]);
1560+
});
1561+
1562+
// Note: vars and matrix contexts are validated at runtime based on their existence
1563+
// vars context only exists if organization/repository variables are defined
1564+
// matrix context only exists if a strategy.matrix is defined
1565+
});
1566+
1567+
describe("step-level if", () => {
1568+
it("allows steps context", async () => {
1569+
const input = `
1570+
on: push
1571+
jobs:
1572+
build:
1573+
runs-on: ubuntu-latest
1574+
steps:
1575+
- id: setup
1576+
run: echo hello
1577+
- if: steps.setup.outcome == 'success'
1578+
run: echo world`;
1579+
1580+
const result = await validate(createDocument("wf.yaml", input));
1581+
expect(result).toEqual([]);
1582+
});
1583+
1584+
it("allows job context", async () => {
1585+
const input = `
1586+
on: push
1587+
jobs:
1588+
build:
1589+
runs-on: ubuntu-latest
1590+
steps:
1591+
- if: job.status == 'success'
1592+
run: echo hello`;
1593+
1594+
const result = await validate(createDocument("wf.yaml", input));
1595+
expect(result).toEqual([]);
1596+
});
1597+
1598+
it("allows runner context", async () => {
1599+
const input = `
1600+
on: push
1601+
jobs:
1602+
build:
1603+
runs-on: ubuntu-latest
1604+
steps:
1605+
- if: runner.os == 'Linux'
1606+
run: echo hello`;
1607+
1608+
const result = await validate(createDocument("wf.yaml", input));
1609+
expect(result).toEqual([]);
1610+
});
1611+
1612+
it("allows env context", async () => {
1613+
const input = `
1614+
on: push
1615+
jobs:
1616+
build:
1617+
runs-on: ubuntu-latest
1618+
env:
1619+
MY_VAR: value
1620+
steps:
1621+
- if: env.MY_VAR == 'value'
1622+
run: echo hello`;
1623+
1624+
const result = await validate(createDocument("wf.yaml", input));
1625+
expect(result).toEqual([]);
1626+
});
1627+
1628+
it("allows matrix context in matrix job", async () => {
1629+
const input = `
1630+
on: push
1631+
jobs:
1632+
build:
1633+
strategy:
1634+
matrix:
1635+
os: [ubuntu, windows]
1636+
runs-on: ubuntu-latest
1637+
steps:
1638+
- if: matrix.os == 'ubuntu'
1639+
run: echo hello`;
1640+
1641+
const result = await validate(createDocument("wf.yaml", input));
1642+
expect(result).toEqual([]);
1643+
});
1644+
1645+
it("allows hashFiles function", async () => {
1646+
const input = `
1647+
on: push
1648+
jobs:
1649+
build:
1650+
runs-on: ubuntu-latest
1651+
steps:
1652+
- if: hashFiles('**/*.txt') != ''
1653+
run: echo hello`;
1654+
1655+
const result = await validate(createDocument("wf.yaml", input));
1656+
expect(result).toEqual([]);
1657+
});
1658+
1659+
it("allows all contexts together", async () => {
1660+
const input = `
1661+
on: push
1662+
jobs:
1663+
build:
1664+
runs-on: ubuntu-latest
1665+
env:
1666+
JOB_VAR: job-value
1667+
steps:
1668+
- id: first
1669+
run: echo hello
1670+
- if: github.event_name == 'push' && steps.first.outcome == 'success' && job.status == 'success' && runner.os == 'Linux' && env.JOB_VAR == 'job-value'
1671+
run: echo world`;
1672+
1673+
const result = await validate(createDocument("wf.yaml", input));
1674+
expect(result).toEqual([]);
1675+
});
1676+
});
1677+
});
15081678
});

‎languageservice/src/validate.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import {Lexer, Parser} from "@actions/expressions";
22
import {Expr} from "@actions/expressions/ast";
33
import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isString} from "@actions/workflow-parser";
44
import {ErrorPolicy} from "@actions/workflow-parser/model/convert";
5+
import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition";
56
import {splitAllowedContext} from "@actions/workflow-parser/templates/allowed-context";
67
import {BasicExpressionToken} from "@actions/workflow-parser/templates/tokens/basic-expression-token";
78
import {StringToken} from "@actions/workflow-parser/templates/tokens/string-token";
@@ -118,9 +119,8 @@ async function additionalValidations(
118119
// Convert the string to an expression token for validation
119120
const condition = token.value.trim();
120121
if (condition) {
121-
// Check if the condition already contains a status function
122-
const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition);
123-
const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`;
122+
// Ensure the condition has a status function, wrapping if needed
123+
const finalCondition = ensureStatusFunction(condition, token.definitionInfo);
124124

125125
// Create a BasicExpressionToken for validation
126126
const expressionToken = new BasicExpressionToken(

0 commit comments

Comments
 (0)