From 78231482f5bca0461a2fddb491b48407cd073774 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Wed, 7 Jan 2026 08:42:59 -0600 Subject: [PATCH] Fix completion and validation issues in action.yml (#290) Follow-up to https://github.com/actions/languageservices/pull/289 ## What this fixes **Autocomplete was broken inside composite action steps.** When you typed inside a step and triggered autocomplete, nothing showed up. Now you correctly get suggestions like run, uses, shell, etc. **Duplicate error messages for missing required fields.** When a required field was missing (like main for Node.js actions), users saw two error messages - one generic schema validation error, and one custom error with a clear explanation. Now they only see the custom one. For example, with using: node24 but no main: - Before: Two errors shown - Schema: "There's not enough info to determine what you meant. Add one of these properties: args, entrypoint, image, main, ..." - Custom: "'main' is required for Node.js actions (using: node24)" - After: Only the custom error is shown --- languageservice/src/complete-action.test.ts | 23 +++++ languageservice/src/complete.ts | 20 ++--- languageservice/src/validate-action.test.ts | 99 +++++++++++++++++++++ languageservice/src/validate-action.ts | 51 +++++++++-- 4 files changed, 170 insertions(+), 23 deletions(-) diff --git a/languageservice/src/complete-action.test.ts b/languageservice/src/complete-action.test.ts index 76340f4..1ffe00b 100644 --- a/languageservice/src/complete-action.test.ts +++ b/languageservice/src/complete-action.test.ts @@ -262,6 +262,29 @@ runs: // It should have a sortText that makes it sort first expect(usingCompletion?.sortText).toBe("0_using"); }); + + it("completes step keys inside composite action steps", async () => { + const [doc, position] = createActionDocument(`name: Test +description: Test +runs: + using: composite + steps: + - run: echo hello + shell: bash + - |`); + const completions = await complete(doc, position); + const labels = completions.map(c => c.label); + + // Should show step keys, not filtered by runs-level logic + expect(labels).toContain("run"); + expect(labels).toContain("uses"); + expect(labels).toContain("shell"); + expect(labels).toContain("id"); + expect(labels).toContain("name"); + expect(labels).toContain("if"); + expect(labels).toContain("env"); + expect(labels).toContain("working-directory"); + }); }); describe("branding completions", () => { diff --git a/languageservice/src/complete.ts b/languageservice/src/complete.ts index 8b05356..b71da41 100644 --- a/languageservice/src/complete.ts +++ b/languageservice/src/complete.ts @@ -658,23 +658,15 @@ function filterActionRunsCompletions(values: Value[], path: TemplateToken[], roo return values; } - // Also verify we're completing at the runs level, not deeper (like inside steps) - // The runs mapping should be the last mapping in the path before the completion position - // or the path should only have root -> runs - let lastMappingIndex = -1; - for (let i = path.length - 1; i >= 0; i--) { - if (path[i] instanceof MappingToken) { - lastMappingIndex = i; - break; - } - } - if (lastMappingIndex === -1) { + // Find where runsMapping is in the path + const runsMappingIndex = path.indexOf(runsMapping); + if (runsMappingIndex === -1) { return values; } - // If the last mapping in path is not the runs mapping, we're nested deeper (e.g., inside steps) - const lastMapping = path[lastMappingIndex]; - if (lastMapping !== runsMapping) { + // Check if there's anything after runsMapping in the path + // If so, we're nested deeper (e.g., inside steps sequence or a step mapping) + if (runsMappingIndex < path.length - 1) { return values; } diff --git a/languageservice/src/validate-action.test.ts b/languageservice/src/validate-action.test.ts index 9f7e414..495fc0b 100644 --- a/languageservice/src/validate-action.test.ts +++ b/languageservice/src/validate-action.test.ts @@ -427,5 +427,104 @@ runs: expect(diagnostics.length).toBeGreaterThan(0); expect(diagnostics.some(d => d.message.includes("main"))).toBe(true); }); + + it("reports error for node24 action missing main", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid - node24 without main +runs: + using: node24 + pre: setup.js +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.message === "'main' is required for Node.js actions (using: node24)")).toBe(true); + // Should NOT have duplicate schema error + expect(diagnostics.filter(d => d.message.includes("main")).length).toBe(1); + }); + + it("reports error for node24 action with only using (no narrowing key)", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid - node24 without main +runs: + using: node24 +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.message === "'main' is required for Node.js actions (using: node24)")).toBe(true); + // Should NOT have the generic "not enough info" schema error + expect(diagnostics.some(d => d.message.includes("There's not enough info"))).toBe(false); + }); + + it("reports error for composite action missing steps", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid - composite without steps +runs: + using: composite +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.message === "'steps' is required for composite actions (using: composite)")).toBe( + true + ); + // Should NOT have duplicate schema error + expect(diagnostics.some(d => d.message.includes("There's not enough info"))).toBe(false); + }); + + it("reports error for docker action missing image", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid - docker without image +runs: + using: docker +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.message === "'image' is required for Docker actions (using: docker)")).toBe(true); + // Should NOT have duplicate schema error + expect(diagnostics.some(d => d.message.includes("There's not enough info"))).toBe(false); + }); + + it("reports error for docker action with entrypoint but missing image", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid - docker without image +runs: + using: docker + entrypoint: /entrypoint.sh +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.message === "'image' is required for Docker actions (using: docker)")).toBe(true); + // Should NOT have duplicate "Required property is missing: image" schema error + expect(diagnostics.filter(d => d.message.includes("image")).length).toBe(1); + }); + + it("lets schema handle missing using", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid - no using +runs: + main: index.js +`); + const diagnostics = await validate(doc); + // Should have schema error about not enough info or unexpected value + expect(diagnostics.length).toBeGreaterThan(0); + // Should NOT have custom validation error (can't determine action type) + expect(diagnostics.some(d => d.message.includes("is required for"))).toBe(false); + }); + + it("lets schema handle invalid using value", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid - bad using value +runs: + using: not-supported + main: index.js +`); + const diagnostics = await validate(doc); + // Should have schema error about unexpected value + expect(diagnostics.length).toBeGreaterThan(0); + // Should NOT have custom validation error (unknown action type) + expect(diagnostics.some(d => d.message.includes("is required for"))).toBe(false); + expect(diagnostics.some(d => d.message.includes("is not valid for"))).toBe(false); + }); }); }); diff --git a/languageservice/src/validate-action.ts b/languageservice/src/validate-action.ts index fce1ba2..4d4b2a1 100644 --- a/languageservice/src/validate-action.ts +++ b/languageservice/src/validate-action.ts @@ -8,6 +8,7 @@ import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; import {MappingToken} from "@actions/workflow-parser/templates/tokens/mapping-token"; import {SequenceToken} from "@actions/workflow-parser/templates/tokens/sequence-token"; import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token"; +import {TemplateValidationError} from "@actions/workflow-parser/templates/template-validation-error"; import {File} from "@actions/workflow-parser/workflows/file"; import {TextDocument} from "vscode-languageserver-textdocument"; import {Diagnostic, DiagnosticSeverity} from "vscode-languageserver-types"; @@ -64,8 +65,16 @@ export async function validateAction(textDocument: TextDocument, config?: Valida return []; } - // Map parser errors to diagnostics - for (const err of result.context.errors.getErrors()) { + // Get schema errors + const schemaErrors = result.context.errors.getErrors(); + + // Run custom runs key validation, which also filters redundant schema errors in place + if (result.value) { + diagnostics.push(...validateRunsKeysAndFilterErrors(result.value, schemaErrors)); + } + + // Map remaining schema errors to diagnostics + for (const err of schemaErrors) { const range = mapRange(err.range); // Determine severity based on error type @@ -83,12 +92,6 @@ export async function validateAction(textDocument: TextDocument, config?: Valida }); } - // Validate runs key combinations based on using type - if (result.value) { - const runsKeyDiagnostics = validateRunsKeys(result.value); - diagnostics.push(...runsKeyDiagnostics); - } - // Validate composite action steps if we have a parsed result if (result.value) { const template = getOrConvertActionTemplate(result.context, result.value, textDocument.uri, { @@ -137,8 +140,12 @@ function findStepsSequence(root: TemplateToken): SequenceToken | undefined { /** * 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. */ -function validateRunsKeys(root: TemplateToken): Diagnostic[] { +function validateRunsKeysAndFilterErrors( + root: TemplateToken, + schemaErrors: TemplateValidationError[] // mutated: redundant errors are removed +): Diagnostic[] { const diagnostics: Diagnostic[] = []; // Find the runs mapping from the root @@ -232,5 +239,31 @@ function validateRunsKeys(root: TemplateToken): Diagnostic[] { } } + // Remove schema errors that we're replacing with more specific messages (mutate in place) + for (let i = schemaErrors.length - 1; i >= 0; i--) { + const err = schemaErrors[i]; + + // Keep errors not at the runs section start + if ( + err.range?.start.line !== runsMapping.range?.start.line || + err.range?.start.column !== runsMapping.range?.start.column + ) { + continue; + } + + // Check if this is an error we're replacing + const isOneOfAmbiguity = err.rawMessage.startsWith("There's not enough info to determine"); + const isRequiredKey = /^Required property is missing: (main|steps|image)$/.test(err.rawMessage); + + if (!isOneOfAmbiguity && !isRequiredKey) { + continue; // Keep errors we're not replacing + } + + // Remove only if we have custom diagnostics for this + if (diagnostics.length > 0) { + schemaErrors.splice(i, 1); + } + } + return diagnostics; }