From 1a63ee9de6b25f08f9ad0c28ae735c383599ef14 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Mon, 8 Dec 2025 09:19:40 -0600 Subject: [PATCH] fix: always provide strategy and matrix contexts with defaults (#235) Fixes https://github.com/github/vscode-github-actions/issues/113 The strategy and matrix contexts are always available in job steps, even when no strategy block is defined. Changes: - Remove the hasStrategy filter from filterContextNames in default.ts - Return null for matrix when no strategy is defined - Provide default values for strategy properties: - fail-fast: true - job-index: 0 - job-total: 1 - max-parallel: 1 - Use defaults for missing strategy properties even when strategy IS defined - Add comprehensive unit tests for strategy context This eliminates false positive 'Context access might be invalid' warnings when using strategy.* or matrix in jobs without an explicit strategy block. --- .../src/complete.expressions.test.ts | 10 +- .../src/context-providers/default.ts | 20 +-- .../src/context-providers/matrix.test.ts | 6 +- .../src/context-providers/matrix.ts | 3 +- .../src/context-providers/strategy.test.ts | 126 ++++++++++++++++++ .../src/context-providers/strategy.ts | 14 +- .../src/validate.expressions.test.ts | 21 +-- 7 files changed, 156 insertions(+), 44 deletions(-) create mode 100644 languageservice/src/context-providers/strategy.test.ts diff --git a/languageservice/src/complete.expressions.test.ts b/languageservice/src/complete.expressions.test.ts index d6756d3..06153c1 100644 --- a/languageservice/src/complete.expressions.test.ts +++ b/languageservice/src/complete.expressions.test.ts @@ -861,7 +861,7 @@ jobs: }); describe("strategy context", () => { - it("strategy is not suggested when outside of a matrix job", async () => { + it("strategy is suggested even when no strategy defined", async () => { const input = ` on: push @@ -875,7 +875,7 @@ jobs: const result = await complete(...getPositionFromCursor(input), {contextProviderConfig}); - expect(result.map(x => x.label)).not.toContain("strategy"); + expect(result.map(x => x.label)).toContain("strategy"); }); it("strategy is suggested within a matrix job", async () => { @@ -922,7 +922,7 @@ jobs: }); describe("matrix context", () => { - it("matrix is not suggested when outside of a matrix job", async () => { + it("matrix is suggested even when no strategy defined", async () => { const input = ` on: push @@ -936,7 +936,7 @@ jobs: const result = await complete(...getPositionFromCursor(input), {contextProviderConfig}); - expect(result.map(x => x.label)).not.toContain("strategy"); + expect(result.map(x => x.label)).toContain("matrix"); }); it("matrix is suggested within a matrix job", async () => { @@ -1123,10 +1123,12 @@ jobs: "github", "inputs", "job", + "matrix", "needs", "runner", "secrets", "steps", + "strategy", "vars", "contains", "endsWith", diff --git a/languageservice/src/context-providers/default.ts b/languageservice/src/context-providers/default.ts index 0b59dec..95c4c9b 100644 --- a/languageservice/src/context-providers/default.ts +++ b/languageservice/src/context-providers/default.ts @@ -32,8 +32,9 @@ export async function getContext( ): Promise { const context = new DescriptionDictionary(); - const filteredNames = filterContextNames(names, workflowContext); - for (const contextName of filteredNames) { + // All context names are valid - strategy and matrix are always available + // (with default values when no strategy block is defined) + for (const contextName of names) { let value = getDefaultContext(contextName, workflowContext, mode) || new DescriptionDictionary(); if (value.kind === Kind.Null) { context.add(contextName, value); @@ -111,18 +112,3 @@ function objectToDictionary(object: {[key: string]: string}): DescriptionDiction return dictionary; } - -function filterContextNames(contextNames: string[], workflowContext: WorkflowContext): string[] { - return contextNames.filter(name => { - switch (name) { - case "matrix": - case "strategy": - return hasStrategy(workflowContext); - } - return true; - }); -} - -function hasStrategy(workflowContext: WorkflowContext): boolean { - return workflowContext.job?.strategy !== undefined || workflowContext.reusableWorkflowJob?.strategy !== undefined; -} diff --git a/languageservice/src/context-providers/matrix.test.ts b/languageservice/src/context-providers/matrix.test.ts index 7c5507d..03c0a56 100644 --- a/languageservice/src/context-providers/matrix.test.ts +++ b/languageservice/src/context-providers/matrix.test.ts @@ -64,7 +64,7 @@ describe("matrix context", () => { expect(workflowContext.job).toBeUndefined(); const context = getMatrixContext(workflowContext, Mode.Validation); - expect(context).toEqual(new DescriptionDictionary()); + expect(context).toEqual(new data.Null()); }); it("strategy not defined", () => { @@ -73,7 +73,7 @@ describe("matrix context", () => { expect(workflowContext.job!.strategy).toBeUndefined(); const context = getMatrixContext(workflowContext, Mode.Validation); - expect(context).toEqual(new DescriptionDictionary()); + expect(context).toEqual(new data.Null()); }); it("strategy is not a mapping token", () => { @@ -81,7 +81,7 @@ describe("matrix context", () => { expect(workflowContext.job!.strategy).toBeDefined(); const context = getMatrixContext(workflowContext, Mode.Validation); - expect(context).toEqual(new DescriptionDictionary()); + expect(context).toEqual(new data.Null()); }); it("matrix is not defined", () => { diff --git a/languageservice/src/context-providers/matrix.ts b/languageservice/src/context-providers/matrix.ts index 41c78cf..9906d4c 100644 --- a/languageservice/src/context-providers/matrix.ts +++ b/languageservice/src/context-providers/matrix.ts @@ -10,7 +10,8 @@ export function getMatrixContext(workflowContext: WorkflowContext, mode: Mode): // https://docs.github.com/en/actions/learn-github-actions/contexts#matrix-context const strategy = workflowContext.job?.strategy ?? workflowContext.reusableWorkflowJob?.strategy; if (!strategy || !isMapping(strategy)) { - return new DescriptionDictionary(); + // No strategy defined - matrix is null at runtime (not empty object) + return new data.Null(); } const matrix = strategy.find("matrix"); diff --git a/languageservice/src/context-providers/strategy.test.ts b/languageservice/src/context-providers/strategy.test.ts new file mode 100644 index 0000000..f129387 --- /dev/null +++ b/languageservice/src/context-providers/strategy.test.ts @@ -0,0 +1,126 @@ +import {data} from "@actions/expressions"; +import {Job} from "@actions/workflow-parser/model/workflow-template"; +import {BooleanToken} from "@actions/workflow-parser/templates/tokens/boolean-token"; +import {MappingToken} from "@actions/workflow-parser/templates/tokens/mapping-token"; +import {NumberToken} from "@actions/workflow-parser/templates/tokens/number-token"; +import {StringToken} from "@actions/workflow-parser/templates/tokens/string-token"; +import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token"; +import {WorkflowContext} from "../context/workflow-context"; +import {getStrategyContext} from "./strategy"; + +function stringToToken(value: string) { + return new StringToken(undefined, undefined, value, undefined); +} + +function boolToToken(value: boolean) { + return new BooleanToken(undefined, undefined, value, undefined); +} + +function numberToToken(value: number) { + return new NumberToken(undefined, undefined, value, undefined); +} + +function contextFromStrategy(strategy?: TemplateToken) { + return { + job: { + strategy: strategy + } + } as WorkflowContext; +} + +describe("strategy context", () => { + describe("no strategy defined", () => { + it("returns defaults when job is undefined", () => { + const workflowContext = {} as WorkflowContext; + + const context = getStrategyContext(workflowContext); + + expect(context.get("fail-fast")).toEqual(new data.BooleanData(true)); + expect(context.get("job-index")).toEqual(new data.NumberData(0)); + expect(context.get("job-total")).toEqual(new data.NumberData(1)); + expect(context.get("max-parallel")).toEqual(new data.NumberData(1)); + }); + + it("returns defaults when strategy is undefined", () => { + const job = {} as Job; + const workflowContext = {job} as WorkflowContext; + + const context = getStrategyContext(workflowContext); + + expect(context.get("fail-fast")).toEqual(new data.BooleanData(true)); + expect(context.get("job-index")).toEqual(new data.NumberData(0)); + expect(context.get("job-total")).toEqual(new data.NumberData(1)); + expect(context.get("max-parallel")).toEqual(new data.NumberData(1)); + }); + + it("returns defaults when strategy is not a mapping", () => { + const workflowContext = contextFromStrategy(stringToToken("hello")); + + const context = getStrategyContext(workflowContext); + + expect(context.get("fail-fast")).toEqual(new data.BooleanData(true)); + expect(context.get("job-index")).toEqual(new data.NumberData(0)); + expect(context.get("job-total")).toEqual(new data.NumberData(1)); + expect(context.get("max-parallel")).toEqual(new data.NumberData(1)); + }); + }); + + describe("strategy defined with partial properties", () => { + it("uses specified fail-fast, defaults for others", () => { + const strategy = new MappingToken(undefined, undefined, undefined); + strategy.add(stringToToken("fail-fast"), boolToToken(false)); + const workflowContext = contextFromStrategy(strategy); + + const context = getStrategyContext(workflowContext); + + expect(context.get("fail-fast")).toEqual(new data.BooleanData(false)); + expect(context.get("job-index")).toEqual(new data.NumberData(0)); + expect(context.get("job-total")).toEqual(new data.NumberData(1)); + expect(context.get("max-parallel")).toEqual(new data.NumberData(1)); + }); + + it("uses specified max-parallel, defaults for others", () => { + const strategy = new MappingToken(undefined, undefined, undefined); + strategy.add(stringToToken("max-parallel"), numberToToken(5)); + const workflowContext = contextFromStrategy(strategy); + + const context = getStrategyContext(workflowContext); + + expect(context.get("fail-fast")).toEqual(new data.BooleanData(true)); + expect(context.get("job-index")).toEqual(new data.NumberData(0)); + expect(context.get("job-total")).toEqual(new data.NumberData(1)); + expect(context.get("max-parallel")).toEqual(new data.NumberData(5)); + }); + + it("only has matrix defined, all strategy properties use defaults", () => { + const strategy = new MappingToken(undefined, undefined, undefined); + const matrix = new MappingToken(undefined, undefined, undefined); + strategy.add(stringToToken("matrix"), matrix); + const workflowContext = contextFromStrategy(strategy); + + const context = getStrategyContext(workflowContext); + + expect(context.get("fail-fast")).toEqual(new data.BooleanData(true)); + expect(context.get("job-index")).toEqual(new data.NumberData(0)); + expect(context.get("job-total")).toEqual(new data.NumberData(1)); + expect(context.get("max-parallel")).toEqual(new data.NumberData(1)); + }); + }); + + describe("strategy with all properties defined", () => { + it("uses all specified values", () => { + const strategy = new MappingToken(undefined, undefined, undefined); + strategy.add(stringToToken("fail-fast"), boolToToken(false)); + strategy.add(stringToToken("max-parallel"), numberToToken(3)); + const workflowContext = contextFromStrategy(strategy); + + const context = getStrategyContext(workflowContext); + + expect(context.get("fail-fast")).toEqual(new data.BooleanData(false)); + // job-index and job-total are runtime values, not specified in YAML + expect(context.get("job-index")).toEqual(new data.NumberData(0)); + expect(context.get("job-total")).toEqual(new data.NumberData(1)); + expect(context.get("max-parallel")).toEqual(new data.NumberData(3)); + }); + }); +}); diff --git a/languageservice/src/context-providers/strategy.ts b/languageservice/src/context-providers/strategy.ts index aa59936..e89ae24 100644 --- a/languageservice/src/context-providers/strategy.ts +++ b/languageservice/src/context-providers/strategy.ts @@ -3,15 +3,24 @@ import {isMapping, isScalar, isString} from "@actions/workflow-parser"; import {WorkflowContext} from "../context/workflow-context"; import {scalarToData} from "../utils/scalar-to-data"; +// Default strategy values when no strategy block is defined +const DEFAULT_STRATEGY = { + "fail-fast": new data.BooleanData(true), + "job-index": new data.NumberData(0), + "job-total": new data.NumberData(1), + "max-parallel": new data.NumberData(1) +}; + export function getStrategyContext(workflowContext: WorkflowContext): DescriptionDictionary { // https://docs.github.com/en/actions/learn-github-actions/contexts#strategy-context const keys = ["fail-fast", "job-index", "job-total", "max-parallel"]; const strategy = workflowContext.job?.strategy ?? workflowContext.reusableWorkflowJob?.strategy; if (!strategy || !isMapping(strategy)) { + // No strategy defined - return defaults that match runtime behavior return new DescriptionDictionary( ...keys.map(key => { - return {key, value: new data.Null()}; + return {key, value: DEFAULT_STRATEGY[key as keyof typeof DEFAULT_STRATEGY]}; }) ); } @@ -31,7 +40,8 @@ export function getStrategyContext(workflowContext: WorkflowContext): Descriptio for (const key of keys) { if (!strategyContext.get(key)) { - strategyContext.add(key, new data.Null()); + // Use default value for missing properties + strategyContext.add(key, DEFAULT_STRATEGY[key as keyof typeof DEFAULT_STRATEGY]); } } diff --git a/languageservice/src/validate.expressions.test.ts b/languageservice/src/validate.expressions.test.ts index dbf83ec..a29a09f 100644 --- a/languageservice/src/validate.expressions.test.ts +++ b/languageservice/src/validate.expressions.test.ts @@ -681,7 +681,8 @@ jobs: const result = await validate(createDocument("wf.yaml", input)); - expect(result).not.toEqual([]); + // Strategy context is always available with default values + expect(result).toEqual([]); }); it("invalid strategy property", async () => { @@ -996,22 +997,8 @@ jobs: const result = await validate(createDocument("wf.yaml", input)); - expect(result).toEqual([ - { - message: "Context access might be invalid: matrix", - range: { - end: { - character: 36, - line: 8 - }, - start: { - character: 18, - line: 8 - } - }, - severity: DiagnosticSeverity.Warning - } - ]); + // Matrix is null when no strategy is defined, accessing properties on null is valid + expect(result).toEqual([]); }); it("basic matrix", async () => {