diff --git a/actions/setup/js/check_workflow_timestamp_api.cjs b/actions/setup/js/check_workflow_timestamp_api.cjs index beabdbf49f5..45ab8befddb 100644 --- a/actions/setup/js/check_workflow_timestamp_api.cjs +++ b/actions/setup/js/check_workflow_timestamp_api.cjs @@ -20,7 +20,7 @@ const path = require("path"); const { getErrorMessage } = require("./error_helpers.cjs"); const { extractHashFromLockFile, extractBodyHashFromLockFile, computeFrontmatterHash, computeBodyHash, createGitHubFileReader } = require("./frontmatter_hash_pure.cjs"); const { getFileContent } = require("./github_api_helpers.cjs"); -const { ERR_CONFIG } = require("./error_codes.cjs"); +const { ERR_CONFIG, SAFE_OUTPUT_E009, CONFIG_HASH_MISMATCH } = require("./error_codes.cjs"); // Matches GitHub workflow ref paths of the form "owner/repo/...[@ref]" // and captures: [1] owner, [2] repo, [3] optional ref @@ -423,7 +423,7 @@ async function main() { await summary.write(); core.setOutput("stale_lock_file_failed", "true"); - core.setFailed(`${ERR_CONFIG}: ${warningMessage}`); + core.setFailed(`${ERR_CONFIG}: ${SAFE_OUTPUT_E009} ${CONFIG_HASH_MISMATCH}: ${warningMessage}`); } else if (hashComparison.match) { // Hashes match - lock file is up to date core.info("✅ Lock file is up to date (hashes match)"); @@ -450,7 +450,7 @@ async function main() { core.setOutput("stale_lock_file_failed", "true"); // Fail the step to prevent workflow from running with outdated configuration - core.setFailed(`${ERR_CONFIG}: ${warningMessage}`); + core.setFailed(`${ERR_CONFIG}: ${SAFE_OUTPUT_E009} ${CONFIG_HASH_MISMATCH}: ${warningMessage}`); } } diff --git a/actions/setup/js/check_workflow_timestamp_api.test.cjs b/actions/setup/js/check_workflow_timestamp_api.test.cjs index f93d1dcff13..de7c5834063 100644 --- a/actions/setup/js/check_workflow_timestamp_api.test.cjs +++ b/actions/setup/js/check_workflow_timestamp_api.test.cjs @@ -173,6 +173,8 @@ model: claude-sonnet-4 await main(); expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Lock file")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("E009")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("CONFIG_HASH_MISMATCH")); expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("frontmatter has changed")); expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("gh aw compile")); @@ -241,6 +243,8 @@ jobs: await main(); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not compare frontmatter hashes")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("E009")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("CONFIG_HASH_MISMATCH")); expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); expect(mockCore.summary.addRaw).toHaveBeenCalled(); expect(mockCore.summary.write).toHaveBeenCalled(); diff --git a/actions/setup/js/error_codes.cjs b/actions/setup/js/error_codes.cjs index a7260c71f4d..3fd758187eb 100644 --- a/actions/setup/js/error_codes.cjs +++ b/actions/setup/js/error_codes.cjs @@ -45,9 +45,21 @@ const ERR_SYSTEM = "ERR_SYSTEM"; /** @type {string} Safe output validation/input errors (legacy numeric taxonomy) */ const SAFE_OUTPUT_E001 = "E001"; +/** @type {string} Safe output lock file frontmatter hash mismatch (legacy numeric taxonomy) */ +const SAFE_OUTPUT_E009 = "E009"; + +/** @type {string} Safe output retry exhaustion due to rate limiting (legacy numeric taxonomy) */ +const SAFE_OUTPUT_E010 = "E010"; + /** @type {string} Safe output operation/runtime failures (legacy numeric taxonomy) */ const SAFE_OUTPUT_E099 = "E099"; +/** @type {string} Named code for lock file frontmatter hash mismatch */ +const CONFIG_HASH_MISMATCH = "CONFIG_HASH_MISMATCH"; + +/** @type {string} Named code for rate limit retry exhaustion */ +const RATE_LIMIT_EXCEEDED = "RATE_LIMIT_EXCEEDED"; + module.exports = { ERR_VALIDATION, ERR_PERMISSION, @@ -57,5 +69,9 @@ module.exports = { ERR_PARSE, ERR_SYSTEM, SAFE_OUTPUT_E001, + SAFE_OUTPUT_E009, + SAFE_OUTPUT_E010, SAFE_OUTPUT_E099, + CONFIG_HASH_MISMATCH, + RATE_LIMIT_EXCEEDED, }; diff --git a/actions/setup/js/error_codes.test.cjs b/actions/setup/js/error_codes.test.cjs index fbd589095e2..edaec1b0474 100644 --- a/actions/setup/js/error_codes.test.cjs +++ b/actions/setup/js/error_codes.test.cjs @@ -1,6 +1,20 @@ // @ts-check import { describe, it, expect } from "vitest"; -const { ERR_VALIDATION, ERR_PERMISSION, ERR_API, ERR_CONFIG, ERR_NOT_FOUND, ERR_PARSE, ERR_SYSTEM, SAFE_OUTPUT_E001, SAFE_OUTPUT_E099 } = require("./error_codes.cjs"); +const { + ERR_VALIDATION, + ERR_PERMISSION, + ERR_API, + ERR_CONFIG, + ERR_NOT_FOUND, + ERR_PARSE, + ERR_SYSTEM, + SAFE_OUTPUT_E001, + SAFE_OUTPUT_E009, + SAFE_OUTPUT_E010, + SAFE_OUTPUT_E099, + CONFIG_HASH_MISMATCH, + RATE_LIMIT_EXCEEDED, +} = require("./error_codes.cjs"); describe("error_codes", () => { describe("module exports", () => { @@ -13,11 +27,15 @@ describe("error_codes", () => { expect(ERR_PARSE).toBeDefined(); expect(ERR_SYSTEM).toBeDefined(); expect(SAFE_OUTPUT_E001).toBeDefined(); + expect(SAFE_OUTPUT_E009).toBeDefined(); + expect(SAFE_OUTPUT_E010).toBeDefined(); expect(SAFE_OUTPUT_E099).toBeDefined(); + expect(CONFIG_HASH_MISMATCH).toBeDefined(); + expect(RATE_LIMIT_EXCEEDED).toBeDefined(); }); it("exports string values only", () => { - const codes = [ERR_VALIDATION, ERR_PERMISSION, ERR_API, ERR_CONFIG, ERR_NOT_FOUND, ERR_PARSE, ERR_SYSTEM, SAFE_OUTPUT_E001, SAFE_OUTPUT_E099]; + const codes = [ERR_VALIDATION, ERR_PERMISSION, ERR_API, ERR_CONFIG, ERR_NOT_FOUND, ERR_PARSE, ERR_SYSTEM, SAFE_OUTPUT_E001, SAFE_OUTPUT_E009, SAFE_OUTPUT_E010, SAFE_OUTPUT_E099, CONFIG_HASH_MISMATCH, RATE_LIMIT_EXCEEDED]; for (const code of codes) { expect(typeof code).toBe("string"); } @@ -59,9 +77,25 @@ describe("error_codes", () => { expect(SAFE_OUTPUT_E001).toBe("E001"); }); + it("SAFE_OUTPUT_E009 is 'E009'", () => { + expect(SAFE_OUTPUT_E009).toBe("E009"); + }); + + it("SAFE_OUTPUT_E010 is 'E010'", () => { + expect(SAFE_OUTPUT_E010).toBe("E010"); + }); + it("SAFE_OUTPUT_E099 is 'E099'", () => { expect(SAFE_OUTPUT_E099).toBe("E099"); }); + + it("CONFIG_HASH_MISMATCH is 'CONFIG_HASH_MISMATCH'", () => { + expect(CONFIG_HASH_MISMATCH).toBe("CONFIG_HASH_MISMATCH"); + }); + + it("RATE_LIMIT_EXCEEDED is 'RATE_LIMIT_EXCEEDED'", () => { + expect(RATE_LIMIT_EXCEEDED).toBe("RATE_LIMIT_EXCEEDED"); + }); }); describe("usage as error message prefixes", () => { @@ -82,7 +116,7 @@ describe("error_codes", () => { }); it("legacy codes are distinct from primary codes", () => { - const all = [ERR_VALIDATION, ERR_PERMISSION, ERR_API, ERR_CONFIG, ERR_NOT_FOUND, ERR_PARSE, ERR_SYSTEM, SAFE_OUTPUT_E001, SAFE_OUTPUT_E099]; + const all = [ERR_VALIDATION, ERR_PERMISSION, ERR_API, ERR_CONFIG, ERR_NOT_FOUND, ERR_PARSE, ERR_SYSTEM, SAFE_OUTPUT_E001, SAFE_OUTPUT_E009, SAFE_OUTPUT_E010, SAFE_OUTPUT_E099, CONFIG_HASH_MISMATCH, RATE_LIMIT_EXCEEDED]; const unique = new Set(all); expect(unique.size).toBe(all.length); }); diff --git a/actions/setup/js/error_recovery.cjs b/actions/setup/js/error_recovery.cjs index 22f8d4cdb40..8814641a932 100644 --- a/actions/setup/js/error_recovery.cjs +++ b/actions/setup/js/error_recovery.cjs @@ -7,7 +7,7 @@ */ const { getErrorMessage } = require("./error_helpers.cjs"); -const { ERR_API } = require("./error_codes.cjs"); +const { ERR_API, SAFE_OUTPUT_E010, RATE_LIMIT_EXCEEDED } = require("./error_codes.cjs"); const { logRetryEvent } = require("./github_rate_limit_logger.cjs"); /** @@ -53,6 +53,22 @@ const RATE_LIMIT_RETRY_CONFIG = { shouldRetry: isTransientError, }; +/** + * Message indicators used to detect GitHub API rate-limit errors. + * Matched case-insensitively against lower-cased error messages. + * Entries are kept lower-case because we lower-case input messages before substring matching. + * @type {string[]} + */ +const RATE_LIMIT_INDICATORS = ["rate limit", "secondary rate limit", "abuse detection", "too many requests"]; + +/** + * @param {string} messageLower - Lower-cased error message + * @returns {boolean} True when message indicates GitHub rate limiting + */ +function hasRateLimitIndicator(messageLower) { + return RATE_LIMIT_INDICATORS.some(pattern => messageLower.includes(pattern)); +} + /** * Determine if an error is transient and worth retrying * @param {any} error - The error to check @@ -69,6 +85,13 @@ function isTransientError(error) { return true; } + // Status-based rate-limit detection handles cases where the message text is + // not descriptive enough (e.g. Octokit errors with status 429 and a generic + // "Request failed" message). Must be checked before the text patterns. + if (isRateLimitError(error)) { + return true; + } + // Network-related errors that are likely transient const transientPatterns = [ "network", @@ -81,9 +104,7 @@ function isTransientError(error) { "502 bad gateway", "503 service unavailable", "504 gateway timeout", - "rate limit", // GitHub API rate limiting - "secondary rate limit", // GitHub secondary rate limits - "abuse detection", // GitHub abuse detection + ...RATE_LIMIT_INDICATORS, // GitHub rate limiting signals (including HTTP 429 text) "temporarily unavailable", "no server is currently available", // GitHub API server unavailability ]; @@ -155,6 +176,25 @@ function getRetryAfterMs(error) { return null; } +/** + * Determine whether an error is specifically a GitHub API rate-limit error. + * @param {any} error - The error to classify + * @returns {boolean} True when the error indicates primary or secondary rate limiting + */ +function isRateLimitError(error) { + const status = error?.response?.status ?? error?.status ?? null; + const headers = error?.response?.headers ?? error?.headers ?? null; + const remainingHeader = headers?.["x-ratelimit-remaining"]; + const retryAfterHeader = headers?.["retry-after"]; + const hasRateLimitHeaders = status === 403 && (retryAfterHeader != null || (remainingHeader != null && parseInt(remainingHeader, 10) === 0)); + if (status === 429 || hasRateLimitHeaders) { + return true; + } + + const errorMsg = getErrorMessage(error).toLowerCase(); + return hasRateLimitIndicator(errorMsg); +} + /** * Execute an operation with retry logic and exponential backoff * @template T @@ -204,6 +244,18 @@ async function withRetry(operation, config = {}, operationName = "operation") { // If this was the last attempt, throw the enhanced error if (attempt === fullConfig.maxRetries) { core.warning(`${operationName} failed after ${fullConfig.maxRetries} retry attempts: ${errorMsg}`); + // E010 applies only when retry budget is at least 3 retries. + const hasEnoughRetriesForE010 = fullConfig.maxRetries >= 3; + if (hasEnoughRetriesForE010 && isRateLimitError(error)) { + throw enhanceError(error, { + operation: operationName, + attempt: attempt + 1, + maxRetries: fullConfig.maxRetries, + retryable: true, + code: `${SAFE_OUTPUT_E010} ${RATE_LIMIT_EXCEEDED}`, + suggestion: "RATE_LIMIT_EXCEEDED: GitHub API rate limit persisted after 3 retries.", + }); + } throw enhanceError(error, { operation: operationName, attempt: attempt + 1, diff --git a/actions/setup/js/error_recovery.test.cjs b/actions/setup/js/error_recovery.test.cjs index 761bd5435d6..3ccf74f9b0c 100644 --- a/actions/setup/js/error_recovery.test.cjs +++ b/actions/setup/js/error_recovery.test.cjs @@ -35,6 +35,7 @@ describe("error_recovery", () => { it("should identify rate limit errors as transient", () => { expect(isTransientError(new Error("Rate limit exceeded"))).toBe(true); expect(isTransientError(new Error("Secondary rate limit hit"))).toBe(true); + expect(isTransientError(new Error("Too Many Requests"))).toBe(true); expect(isTransientError(new Error("Abuse detection triggered"))).toBe(true); }); @@ -50,6 +51,12 @@ describe("error_recovery", () => { expect(isTransientError(new Error(" ..."))).toBe(true); }); + it("should identify status-only 429 errors as transient even with a non-descriptive message", () => { + // Octokit can produce errors where status=429 but the message contains no rate-limit text. + expect(isTransientError({ status: 429, message: "Request failed" })).toBe(true); + expect(isTransientError({ response: { status: 429 }, message: "Request failed" })).toBe(true); + }); + it("should not identify validation errors as transient", () => { expect(isTransientError(new Error("Invalid input"))).toBe(false); expect(isTransientError(new Error("Field is required"))).toBe(false); @@ -103,6 +110,45 @@ describe("error_recovery", () => { expect(core.warning).toHaveBeenCalledWith(expect.stringContaining("failed after 2 retry attempts")); }); + it("should emit E010 RATE_LIMIT_EXCEEDED when rate limit persists after 3 retries", async () => { + const rateLimitError = { + message: "Too Many Requests", + response: { status: 429, headers: { "retry-after": "1" } }, + }; + const operation = vi.fn().mockRejectedValue(rateLimitError); + + await expect(withRetry(operation, { maxRetries: 3, initialDelayMs: 1 }, "test-operation")).rejects.toThrow("E010 RATE_LIMIT_EXCEEDED"); + + expect(operation).toHaveBeenCalledTimes(4); // Initial + 3 retries + }); + + it("should emit E010 for exhausted retries on 403 with retry-after header", async () => { + const rateLimitError = { + message: "secondary rate limit", + response: { status: 403, headers: { "retry-after": "1" } }, + }; + const operation = vi.fn().mockRejectedValue(rateLimitError); + + await expect(withRetry(operation, { maxRetries: 3, initialDelayMs: 1 }, "test-operation")).rejects.toThrow("E010 RATE_LIMIT_EXCEEDED"); + }); + + it("should emit E010 for exhausted retries on abuse-detection message without status", async () => { + const rateLimitError = new Error("Abuse detection mechanism triggered"); + const operation = vi.fn().mockRejectedValue(rateLimitError); + + await expect(withRetry(operation, { maxRetries: 3, initialDelayMs: 1 }, "test-operation")).rejects.toThrow("E010 RATE_LIMIT_EXCEEDED"); + }); + + it("should emit E010 for status-only 429 with non-descriptive message persisting after 3 retries", async () => { + // Verifies that an Octokit-style error with status=429 and a generic message + // is retried (isTransientError returns true via isRateLimitError) and emits E010 on exhaustion. + const rateLimitError = { status: 429, message: "Request failed" }; + const operation = vi.fn().mockRejectedValue(rateLimitError); + + await expect(withRetry(operation, { maxRetries: 3, initialDelayMs: 1 }, "test-operation")).rejects.toThrow("E010 RATE_LIMIT_EXCEEDED"); + expect(operation).toHaveBeenCalledTimes(4); // Initial + 3 retries + }); + it("should use exponential backoff", async () => { const operation = vi.fn().mockRejectedValueOnce(new Error("Network timeout")).mockRejectedValueOnce(new Error("Network timeout")).mockResolvedValue("success");