From c855662eeb516af5ac78f75a906c5051d0f32640 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Wed, 20 May 2026 12:24:44 -1000 Subject: [PATCH] Address copilot review feedback --- .github/workflows/path-validation-e2e.yml | 13 ++++++++---- __tests__/actionUtils.test.ts | 25 ++++++++++------------- __tests__/restoreImpl.test.ts | 14 ++++++------- docs/path-validation-test-plan.md | 2 +- src/restoreImpl.ts | 7 ++++++- src/utils/actionUtils.ts | 12 +++++++---- 6 files changed, 42 insertions(+), 31 deletions(-) diff --git a/.github/workflows/path-validation-e2e.yml b/.github/workflows/path-validation-e2e.yml index ab53712..1fb580c 100644 --- a/.github/workflows/path-validation-e2e.yml +++ b/.github/workflows/path-validation-e2e.yml @@ -24,10 +24,15 @@ permissions: # - warn: the malicious entry is extracted but a workflow warning is logged. # - error: the malicious entry is rejected (no extraction). # -# NOTE: The poisoned-cache phase requires a small Node.js helper script -# (__tests__/e2e/generate-poisoned-archive.mjs) that the test workflow invokes. -# We build the archive locally and upload it via the action under a strict-paths -# label so the cache key namespacing remains consistent. +# NOTE: The poisoned-cache phase relies on a small Node.js helper script +# (__tests__/e2e/save-poisoned-cache.mjs) that the workflow invokes. Rather +# than fabricating a tar archive by hand, the helper calls the toolkit's +# `@actions/cache.saveCache()` with the declared `path` AND one or more extra +# paths that escape it; the toolkit packs everything into a normal cache +# archive. The action's later restore step declares only the legitimate +# `path`, so the extra entries become "escape" entries that the client-side +# validation should reject (or warn about) per the configured strict-paths +# mode. jobs: good-cache: diff --git a/__tests__/actionUtils.test.ts b/__tests__/actionUtils.test.ts index c9a9e2b..f14cd98 100644 --- a/__tests__/actionUtils.test.ts +++ b/__tests__/actionUtils.test.ts @@ -1,7 +1,7 @@ import * as cache from "@actions/cache"; import * as core from "@actions/core"; -import { Events, RefKey } from "../src/constants"; +import { Events, Inputs, RefKey } from "../src/constants"; import * as actionUtils from "../src/utils/actionUtils"; import * as testUtils from "../src/utils/testUtils"; @@ -267,11 +267,11 @@ test("isGhes returns true when the GITHUB_SERVER_URL environment variable is set }); describe("getPathValidationInput", () => { - const inputEnv = "INPUT_STRICT-PATHS"; + const inputEnv = `INPUT_${Inputs.StrictPaths.toUpperCase()}`; beforeEach(() => { delete process.env[inputEnv]; - // Re-mock getInput so the each-test environment reads INPUT_STRICT-PATHS + // Re-mock getInput so the each-test environment reads the input env var jest.spyOn(core, "getInput").mockImplementation((name, options) => { return jest.requireActual("@actions/core").getInput(name, options); }); @@ -297,24 +297,21 @@ describe("getPathValidationInput", () => { expect(actionUtils.getPathValidationInput()).toBe(expected); }); - test("falls back to 'warn' for unrecognized values and logs a warning", () => { + test("falls back to 'warn' for unrecognized values and emits a workflow warning", () => { process.env[inputEnv] = "strict"; - // getPathValidationInput() calls the same module's logWarning() via a - // local function reference (TypeScript compiles intra-module calls as - // direct references, not exports.X lookups), so a jest.spyOn on - // actionUtils.logWarning cannot intercept it. Spy on core.info — the - // only externally observable side effect — and suppress the real - // implementation so the warning does not pollute the Jest log. - const infoSpy = jest - .spyOn(core, "info") + // Suppress the real implementation so the warning does not pollute + // the Jest log, and assert it was emitted via core.warning so it + // surfaces as a real `::warning::` workflow annotation. + const warningSpy = jest + .spyOn(core, "warning") .mockImplementation(() => undefined); try { expect(actionUtils.getPathValidationInput()).toBe("warn"); - expect(infoSpy).toHaveBeenCalledWith( + expect(warningSpy).toHaveBeenCalledWith( expect.stringContaining("Unrecognized value for strict-paths") ); } finally { - infoSpy.mockRestore(); + warningSpy.mockRestore(); } }); diff --git a/__tests__/restoreImpl.test.ts b/__tests__/restoreImpl.test.ts index e1a4c25..1f168c6 100644 --- a/__tests__/restoreImpl.test.ts +++ b/__tests__/restoreImpl.test.ts @@ -546,12 +546,12 @@ test("restore falls back to 'warn' when strict-paths input is unrecognized", asy const restoreCacheMock = jest .spyOn(cache, "restoreCache") .mockResolvedValueOnce(key); - // getPathValidationInput()'s call to logWarning() is intra-module so a - // spy on actionUtils.logWarning would not intercept it. Spy on core.info - // (the underlying transport for logWarning) and suppress the real - // implementation so the warning does not print into the Jest log. - const infoMock = jest - .spyOn(core, "info") + // getPathValidationInput() emits the misconfiguration notice via + // core.warning() so it surfaces as a real `::warning::` workflow + // annotation. Suppress the real implementation to keep the Jest log + // clean while asserting it was called. + const warningMock = jest + .spyOn(core, "warning") .mockImplementation(() => undefined); await restoreImpl(new StateProvider()); @@ -563,7 +563,7 @@ test("restore falls back to 'warn' when strict-paths input is unrecognized", asy { lookupOnly: false, pathValidation: "warn" }, false ); - expect(infoMock).toHaveBeenCalledWith( + expect(warningMock).toHaveBeenCalledWith( expect.stringContaining("Unrecognized value for strict-paths") ); }); diff --git a/docs/path-validation-test-plan.md b/docs/path-validation-test-plan.md index b1cc642..a3826a1 100644 --- a/docs/path-validation-test-plan.md +++ b/docs/path-validation-test-plan.md @@ -49,7 +49,7 @@ errors per the `fail-on-cache-invalid` input. | defaults strict-paths to `'warn'` and forwards it to `restoreCache` | Default option object contains `pathValidation: 'warn'` | | `test.each(['off', 'warn', 'error'])` forwards each value to `restoreCache` | All three valid values reach the toolkit unchanged | | falls back to `'warn'` when strict-paths input is unrecognized | Unknown values are coerced to `'warn'` and a warning is logged | -| treats `CacheIntegrityError` as a cache miss by default | When the toolkit throws `CacheIntegrityError` and `fail-on-cache-invalid: false`, action sets `cache-hit=false` and continues | +| treats `CacheIntegrityError` as a cache miss by default | When the toolkit throws `CacheIntegrityError` and `fail-on-cache-invalid: false`, action logs the rejection and returns without setting the `cache-hit` output (intentionally unset to match regular cache-miss semantics — see issue #1466) | | fails when `CacheIntegrityError` is raised and `fail-on-cache-invalid: true` | When `fail-on-cache-invalid: true`, `core.setFailed()` is called with a message containing `integrity validation` and the code | | propagates non-integrity errors normally | Network/auth errors still surface via `core.setFailed()` rather than being mis-classified as integrity failures | | `PARSE_ERROR` integrity failure also treated as miss by default | Validation handles both `PATH_VIOLATION` and `PARSE_ERROR` codes identically | diff --git a/src/restoreImpl.ts b/src/restoreImpl.ts index 83c3ec9..ce8ba65 100644 --- a/src/restoreImpl.ts +++ b/src/restoreImpl.ts @@ -64,11 +64,16 @@ export async function restoreImpl( if (err instanceof Error && err.name === "CacheIntegrityError") { const code = (err as Error & { code?: string }).code; if (failOnCacheInvalid) { - throw new Error( + // Preserve the toolkit's original error via `Error.cause`. + // (Assigned after construction because this project's + // tsconfig targets ES6.) + const failure = new Error( `Restored cache failed integrity validation (${ code ?? "unknown" }): ${err.message}` ); + (failure as Error & { cause?: unknown }).cause = err; + throw failure; } // Treat as a cache miss. Intentionally do NOT set the // `cache-hit` output here, to preserve the same downstream diff --git a/src/utils/actionUtils.ts b/src/utils/actionUtils.ts index eff847d..e8fa835 100644 --- a/src/utils/actionUtils.ts +++ b/src/utils/actionUtils.ts @@ -1,7 +1,7 @@ import * as cache from "@actions/cache"; import * as core from "@actions/core"; -import { RefKey } from "../constants"; +import { Inputs, RefKey } from "../constants"; export function isGhes(): boolean { const ghUrl = new URL( @@ -70,15 +70,19 @@ export function getInputAsBool( * Read the `strict-paths` input and coerce it to a value the `@actions/cache` * toolkit understands. Unknown values default to `'warn'` (a best-effort * recovery — we don't want a typo to silently disable client-side validation) - * and a warning is emitted so the user notices. + * and a workflow warning annotation is emitted so the user notices. + * + * Uses `core.warning()` directly (rather than this module's `logWarning()` + * helper, which routes through `core.info()`) so an input misconfiguration + * surfaces as a real `::warning::` annotation in the run summary. */ export function getPathValidationInput(): "off" | "warn" | "error" { - const raw = core.getInput("strict-paths") || "warn"; + const raw = core.getInput(Inputs.StrictPaths) || "warn"; const normalized = raw.toLowerCase(); if (normalized === "off" || normalized === "warn" || normalized === "error") { return normalized; } - logWarning( + core.warning( `Unrecognized value for strict-paths: "${raw}". Falling back to "warn". Valid values are: off, warn, error.` ); return "warn";