From dec597b0dbeabb7e8bc607c9b19a92b2467eb515 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Thu, 4 Dec 2025 11:25:15 -0600 Subject: [PATCH] Improve cron schedule validation and diagnostics (#224) --- languageservice/src/hover.test.ts | 7 +- languageservice/src/hover.ts | 26 +----- languageservice/src/validate.test.ts | 91 ++++++++++++++++++- languageservice/src/validate.ts | 53 +++++++++++ .../src/model/converter/cron.test.ts | 50 +++++++++- workflow-parser/src/model/converter/cron.ts | 78 +++++++++++++++- workflow-parser/src/model/converter/events.ts | 2 +- 7 files changed, 266 insertions(+), 41 deletions(-) diff --git a/languageservice/src/hover.test.ts b/languageservice/src/hover.test.ts index 0ac2047..b49ae0d 100644 --- a/languageservice/src/hover.test.ts +++ b/languageservice/src/hover.test.ts @@ -110,11 +110,8 @@ jobs: `; const result = await hover(...getPositionFromCursor(input)); expect(result).not.toBeUndefined(); - expect(result?.contents).toEqual( - "Runs at 0 and 30 minutes past the hour, at 00:00 and 12:00\n\n" + - "Actions schedules run at most every 5 minutes. " + - "[Learn more](https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule)" - ); + // Cron description is now shown via diagnostics, not hover + expect(result?.contents).toEqual(""); }); it("on a cron mapping key", async () => { diff --git a/languageservice/src/hover.ts b/languageservice/src/hover.ts index 4ab8ed6..2102ced 100644 --- a/languageservice/src/hover.ts +++ b/languageservice/src/hover.ts @@ -2,11 +2,9 @@ import {data, DescriptionDictionary, Parser} from "@actions/expressions"; import {FunctionDefinition, FunctionInfo} from "@actions/expressions/funcs/info"; import {Lexer} from "@actions/expressions/lexer"; import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; -import {getCronDescription} from "@actions/workflow-parser/model/converter/cron"; import {splitAllowedContext} from "@actions/workflow-parser/templates/allowed-context"; -import {StringToken} from "@actions/workflow-parser/templates/tokens/string-token"; import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token"; -import {isBasicExpression, isString} from "@actions/workflow-parser/templates/tokens/type-guards"; +import {isBasicExpression} from "@actions/workflow-parser/templates/tokens/type-guards"; import {File} from "@actions/workflow-parser/workflows/file"; import {FileProvider} from "@actions/workflow-parser/workflows/file-provider"; import {Position, TextDocument} from "vscode-languageserver-textdocument"; @@ -23,7 +21,7 @@ import {ExpressionPos, mapToExpressionPos} from "./expression-hover/expression-p import {HoverVisitor} from "./expression-hover/visitor"; import {info} from "./log"; import {isPotentiallyExpression} from "./utils/expression-detection"; -import {findToken, TokenResult} from "./utils/find-token"; +import {findToken} from "./utils/find-token"; import {mapRange} from "./utils/range"; import {fetchOrConvertWorkflowTemplate, fetchOrParseWorkflow} from "./utils/workflow-cache"; @@ -89,17 +87,6 @@ export async function hover(document: TextDocument, position: Position, config?: info(`Calculating hover for token with definition ${token.definition.key}`); - if (tokenResult.parent && isCronMappingValue(tokenResult)) { - const tokenValue = (token as StringToken).value; - const description = getCronDescription(tokenValue); - if (description) { - return { - contents: description, - range: mapRange(token.range) - } satisfies Hover; - } - } - if (tokenResult.parent && isReusableWorkflowJobInput(tokenResult)) { let description = getReusableWorkflowInputDescription(workflowContext, tokenResult); description = appendContext(description, token.definitionInfo?.allowedContext); @@ -156,15 +143,6 @@ async function getDescription( return description || defaultDescription; } -function isCronMappingValue(tokenResult: TokenResult): boolean { - return ( - tokenResult.parent?.definition?.key === "cron-mapping" && - !!tokenResult.token && - isString(tokenResult.token) && - tokenResult.token.value !== "cron" - ); -} - function expressionHover( exprPos: ExpressionPos, context: DescriptionDictionary, diff --git a/languageservice/src/validate.test.ts b/languageservice/src/validate.test.ts index cb87c76..06d79cf 100644 --- a/languageservice/src/validate.test.ts +++ b/languageservice/src/validate.test.ts @@ -181,7 +181,7 @@ jobs: expect(result.length).toBe(1); expect(result[0]).toEqual({ - message: "Invalid cron string", + message: "Invalid cron expression. Expected format: '* * * * *' (minute hour day month weekday)", range: { end: { character: 21, @@ -195,6 +195,95 @@ jobs: } as Diagnostic); }); + it("cron with interval less than 5 minutes shows warning", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: + schedule: + - cron: '*/1 * * * *' +jobs: + build: + runs-on: ubuntu-latest` + ), + {valueProviderConfig: defaultValueProviders} + ); + + expect(result.length).toBe(1); + expect(result[0]).toEqual({ + message: "Runs every minute. Note: Actions schedules run at most every 5 minutes.", + severity: DiagnosticSeverity.Warning, + code: "on-schedule", + codeDescription: { + href: "https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule" + }, + range: { + end: { + character: 25, + line: 2 + }, + start: { + character: 12, + line: 2 + } + } + } as Diagnostic); + }); + + it("cron with interval of 5 minutes or more shows info", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: + schedule: + - cron: '*/5 * * * *' +jobs: + build: + runs-on: ubuntu-latest` + ), + {valueProviderConfig: defaultValueProviders} + ); + + expect(result.length).toBe(1); + expect(result[0]).toEqual({ + message: "Runs every 5 minutes", + severity: DiagnosticSeverity.Information, + code: "on-schedule", + codeDescription: { + href: "https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule" + }, + range: { + end: { + character: 25, + line: 2 + }, + start: { + character: 12, + line: 2 + } + } + } as Diagnostic); + }); + + it("cron with comma-separated minutes less than 5 apart shows warning", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: + schedule: + - cron: '0,2 * * * *' +jobs: + build: + runs-on: ubuntu-latest` + ), + {valueProviderConfig: defaultValueProviders} + ); + + expect(result.length).toBe(1); + expect(result[0]?.severity).toBe(DiagnosticSeverity.Warning); + expect(result[0]?.message).toContain("Note: Actions schedules run at most every 5 minutes."); + }); + it("invalid YAML", async () => { // This YAML has some mismatched single-quotes, which causes the string to be terminated early // within the fromJSON() expression. diff --git a/languageservice/src/validate.ts b/languageservice/src/validate.ts index da44ad0..3f565d3 100644 --- a/languageservice/src/validate.ts +++ b/languageservice/src/validate.ts @@ -2,6 +2,7 @@ import {Lexer, Parser, data} from "@actions/expressions"; import {Expr, FunctionCall, Literal, Logical} from "@actions/expressions/ast"; import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isString} from "@actions/workflow-parser"; import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; +import {getCronDescription, hasCronIntervalLessThan5Minutes} from "@actions/workflow-parser/model/converter/cron"; 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"; @@ -27,6 +28,9 @@ import {validateAction} from "./validate-action"; import {ValueProviderConfig, ValueProviderKind} from "./value-providers/config"; import {defaultValueProviders} from "./value-providers/default"; +const CRON_SCHEDULE_DOCS_URL = + "https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule"; + export type ValidationConfig = { valueProviderConfig?: ValueProviderConfig; contextProviderConfig?: ContextProviderConfig; @@ -166,6 +170,11 @@ async function additionalValidations( validateWorkflowUsesFormat(diagnostics, token); } + // Validate cron expressions - warn if interval is less than 5 minutes + if (isString(token) && token.range && validationDefinition?.key === "cron-pattern") { + validateCronExpression(diagnostics, token); + } + // Allowed values coming from the schema have already been validated. Only check if // a value provider is defined for a token and if it is, validate the values match. if (token.range && validationDefinition) { @@ -216,6 +225,50 @@ function invalidValue(diagnostics: Diagnostic[], token: StringToken, kind: Value } } +/** + * Validates cron expressions and provides diagnostics for valid cron schedules. + * Shows a warning if the interval is less than 5 minutes (since GitHub Actions + * schedules run at most every 5 minutes), otherwise shows an info message. + */ +function validateCronExpression(diagnostics: Diagnostic[], token: StringToken): void { + const cronValue = token.value; + + // Ensure we have a range for diagnostics + if (!token.range) { + return; + } + + // Only check valid cron expressions - invalid ones are already caught by the parser + const description = getCronDescription(cronValue); + if (!description) { + return; + } + + // Check if the cron specifies an interval less than 5 minutes + if (hasCronIntervalLessThan5Minutes(cronValue)) { + diagnostics.push({ + message: `${description}. Note: Actions schedules run at most every 5 minutes.`, + range: mapRange(token.range), + severity: DiagnosticSeverity.Warning, + code: "on-schedule", + codeDescription: { + href: CRON_SCHEDULE_DOCS_URL + } + }); + } else { + // Show info message for valid cron expressions + diagnostics.push({ + message: description, + range: mapRange(token.range), + severity: DiagnosticSeverity.Information, + code: "on-schedule", + codeDescription: { + href: CRON_SCHEDULE_DOCS_URL + } + }); + } +} + /** * Validates the format of a step's `uses` field. * diff --git a/workflow-parser/src/model/converter/cron.test.ts b/workflow-parser/src/model/converter/cron.test.ts index 18075a9..f8ef4df 100644 --- a/workflow-parser/src/model/converter/cron.test.ts +++ b/workflow-parser/src/model/converter/cron.test.ts @@ -1,4 +1,4 @@ -import {isValidCron, getCronDescription} from "./cron"; +import {isValidCron, getCronDescription, hasCronIntervalLessThan5Minutes} from "./cron"; describe("cron", () => { describe("valid cron", () => { @@ -66,14 +66,54 @@ describe("cron", () => { describe("getCronDescription", () => { it(`Produces a sentence for valid cron`, () => { - expect(getCronDescription("0 * * * *")).toEqual( - "Runs every hour\n\n" + - "Actions schedules run at most every 5 minutes. [Learn more](https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule)" - ); + expect(getCronDescription("0 * * * *")).toEqual("Runs every hour"); }); it(`Returns nothing for invalid cron`, () => { expect(getCronDescription("* * * * * *")).toBeUndefined(); }); }); + + describe("hasCronIntervalLessThan5Minutes", () => { + it("returns true for step expressions with interval < 5 min", () => { + expect(hasCronIntervalLessThan5Minutes("*/1 * * * *")).toBe(true); + expect(hasCronIntervalLessThan5Minutes("*/4 * * * *")).toBe(true); + }); + + it("returns false for step expressions with interval >= 5 min", () => { + expect(hasCronIntervalLessThan5Minutes("*/5 * * * *")).toBe(false); + expect(hasCronIntervalLessThan5Minutes("*/15 * * * *")).toBe(false); + }); + + it("returns true for comma-separated values with gap < 5 min", () => { + expect(hasCronIntervalLessThan5Minutes("0,2,4 * * * *")).toBe(true); + expect(hasCronIntervalLessThan5Minutes("0,10,12 * * * *")).toBe(true); + }); + + it("returns false for comma-separated values with gap >= 5 min", () => { + expect(hasCronIntervalLessThan5Minutes("0,10,20 * * * *")).toBe(false); + expect(hasCronIntervalLessThan5Minutes("0,30 * * * *")).toBe(false); + }); + + it("returns true for comma-separated values with wrap-around gap < 5 min", () => { + expect(hasCronIntervalLessThan5Minutes("0,58 * * * *")).toBe(true); + expect(hasCronIntervalLessThan5Minutes("2,59 * * * *")).toBe(true); + }); + + it("returns true for * (every minute)", () => { + expect(hasCronIntervalLessThan5Minutes("* * * * *")).toBe(true); + }); + + it("returns true for range expressions (runs every minute in range)", () => { + expect(hasCronIntervalLessThan5Minutes("0-4 * * * *")).toBe(true); + }); + + it("returns false for single value (hourly)", () => { + expect(hasCronIntervalLessThan5Minutes("0 * * * *")).toBe(false); + }); + + it("returns false for invalid cron", () => { + expect(hasCronIntervalLessThan5Minutes("invalid")).toBe(false); + }); + }); }); diff --git a/workflow-parser/src/model/converter/cron.ts b/workflow-parser/src/model/converter/cron.ts index b104eed..41062f0 100644 --- a/workflow-parser/src/model/converter/cron.ts +++ b/workflow-parser/src/model/converter/cron.ts @@ -8,6 +8,78 @@ type Range = { names?: Record; }; +/** + * Checks if a cron expression specifies an interval shorter than 5 minutes. + * GitHub Actions schedules run at most every 5 minutes, so intervals < 5 min won't work as expected. + */ +export function hasCronIntervalLessThan5Minutes(cron: string): boolean { + if (!isValidCron(cron)) { + return false; + } + + const parts = cron.split(/ +/); + const minutePart = parts[0]; + + // Parse the minute field to determine the effective interval + return getMinuteInterval(minutePart) < 5; +} + +/** + * Gets the minimum interval in minutes between cron executions based on the minute field. + * Returns 60 if there's only one execution per hour, otherwise returns the minimum gap. + */ +function getMinuteInterval(minutePart: string): number { + // Handle step expressions like */1, */3, 0-59/2 + if (minutePart.includes("/")) { + const [, step] = minutePart.split("/"); + const stepNum = parseInt(step, 10); + if (!isNaN(stepNum) && stepNum > 0) { + return stepNum; + } + } + + // Handle comma-separated values like 0,2,4 or 0,1,5,10 + if (minutePart.includes(",")) { + const values = minutePart + .split(",") + .map(v => parseInt(v, 10)) + .filter(n => !isNaN(n)) + .sort((a, b) => a - b); + if (values.length >= 2) { + let minGap = 60; + for (let i = 1; i < values.length; i++) { + const gap = values[i] - values[i - 1]; + if (gap < minGap) { + minGap = gap; + } + } + // Check wrap-around gap from last minute to first minute of next hour + const wrapGap = values[0] + 60 - values[values.length - 1]; + if (wrapGap < minGap) { + minGap = wrapGap; + } + return minGap; + } + } + + // Handle range expressions like 0-4 (runs every minute from 0-4) + if (minutePart.includes("-") && !minutePart.includes("/")) { + const [start, end] = minutePart.split("-").map(v => parseInt(v, 10)); + if (!isNaN(start) && !isNaN(end) && end > start) { + // A range without step means every minute in that range + return 1; + } + } + + // * means every minute + if (minutePart === "*") { + return 1; + } + + // Single value or unrecognized pattern - assume hourly (60 min interval) + return 60; +} + export function isValidCron(cron: string): boolean { // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule @@ -46,11 +118,7 @@ export function getCronDescription(cronspec: string): string | undefined { } // Make first character lowercase - let result = "Runs " + desc.charAt(0).toLowerCase() + desc.slice(1); - result += - "\n\nActions schedules run at most every 5 minutes." + - " [Learn more](https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#onschedule)"; - return result; + return "Runs " + desc.charAt(0).toLowerCase() + desc.slice(1); } function validateCronPart(value: string, range: Range, allowSeparators = true): boolean { diff --git a/workflow-parser/src/model/converter/events.ts b/workflow-parser/src/model/converter/events.ts index 955de5b..f2b5841 100644 --- a/workflow-parser/src/model/converter/events.ts +++ b/workflow-parser/src/model/converter/events.ts @@ -158,7 +158,7 @@ function convertSchedule(context: TemplateContext, token: SequenceToken): Schedu const cron = schedule.value.assertString(`schedule cron`); // Validate the cron string if (!isValidCron(cron.value)) { - context.error(cron, "Invalid cron string"); + context.error(cron, "Invalid cron expression. Expected format: '* * * * *' (minute hour day month weekday)"); } result.push({cron: cron.value}); } else {