From 70c6a094e109d51bc47b15ed04eaed8660e39ed5 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 12:24:25 -0500 Subject: [PATCH 01/15] feat(cache): surface cache read-denied as a distinct restore warning Mirror the existing cache write-denied handling on the restore path. When the receiver refuses a download URL because the run's token has no readable cache scopes, it returns a twirp PermissionDenied (HTTP 403). The twirp client wraps that 403 in a generic Error, so the stable 'cache read denied:' prefix is embedded in the message rather than at the start. - Add CACHE_READ_DENIED_PREFIX and CacheReadDeniedError - Dispatch on the prefix in the restoreCacheV2 catch block (V2 only), log a policy-specific warning, and report a cache miss so the run continues - Add a test mirroring the write-denied coverage --- .../cache/__tests__/restoreCacheV2.test.ts | 35 +++++++++++++++ packages/cache/src/cache.ts | 44 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/packages/cache/__tests__/restoreCacheV2.test.ts b/packages/cache/__tests__/restoreCacheV2.test.ts index 421069db9d..7046412a72 100644 --- a/packages/cache/__tests__/restoreCacheV2.test.ts +++ b/packages/cache/__tests__/restoreCacheV2.test.ts @@ -112,6 +112,41 @@ test('restore with server error should fail', async () => { ) }) +test('restore denied by read-only token logs warning and reports cache miss', async () => { + // When the receiver refuses the download URL because the run's token has no + // readable cache scopes, it returns a twirp PermissionDenied (HTTP 403). + // The twirp client wraps that 403 into a generic Error, so the stable + // `cache read denied:` prefix arrives embedded in the message (not at the + // start). The toolkit must dispatch on that prefix, log a single warning + // (not an error, even though the underlying status is 403), and report a + // cache miss so the workflow continues. + const paths = ['node_modules'] + const key = 'node-test' + const logErrorMock = jest.spyOn(core, 'error') + const logWarningMock = jest.spyOn(core, 'warning') + // Realistic wrapped shape produced by cacheTwirpClient for a 403 response. + const wrappedDeniedMessage = + 'Failed to GetCacheEntryDownloadURL: Received non-retryable error: ' + + 'Failed request: (403) Forbidden: cache read denied: token has no readable scopes' + + jest + .spyOn(CacheServiceClientJSON.prototype, 'GetCacheEntryDownloadURL') + .mockImplementation(() => { + throw new Error(wrappedDeniedMessage) + }) + + const cacheKey = await restoreCache(paths, key) + expect(cacheKey).toBe(undefined) + // 403 must not be logged as an error. + expect(logErrorMock).not.toHaveBeenCalled() + // A single warning that carries the stable `cache read denied:` prefix so + // the runner UI and consumers can dispatch on it. + expect(logWarningMock).toHaveBeenCalledWith( + `Failed to restore: ${wrappedDeniedMessage}` + ) + expect(logWarningMock).toHaveBeenCalledTimes(1) +}) + test('restore with restore keys and no cache found', async () => { const paths = ['node_modules'] const key = 'node-test' diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index c53404049b..e467345bb0 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -62,6 +62,41 @@ export class CacheWriteDeniedError extends ReserveCacheError { } } +/** + * Stable prefix the receiver embeds in the twirp error it returns from + * GetCacheEntryDownloadURL when the issuer scoped the run's token so that it + * has no readable cache scopes (a policy read denial). restoreCacheV2 + * dispatches on this prefix to re-classify the failure as a + * CacheReadDeniedError so it can surface a policy-specific warning instead of + * a generic restore failure. + * + * Note: unlike the write path, GetCacheEntryDownloadURLResponse has no + * `message` field, so the receiver signals the denial as a twirp + * PermissionDenied (HTTP 403). The twirp client wraps that 403 in a generic + * Error, so this prefix appears embedded in the thrown error's message rather + * than at the very start -- match with `includes`, not `startsWith`. + */ +export const CACHE_READ_DENIED_PREFIX = 'cache read denied:' + +/** + * Raised when the cache backend refuses to return a cache download URL because + * the JWT issued for this run has no readable cache scopes (for example, the + * run was triggered by an event the repository administrator classified as + * untrusted). The receiver-supplied detail message contains + * `cache read denied:` (the full error message includes additional context). + * + * Caching is best-effort, so restoreCacheV2 does not rethrow this to + * consumers; it logs a policy-specific warning and reports a cache miss so the + * workflow continues. + */ +export class CacheReadDeniedError extends Error { + constructor(message: string) { + super(message) + this.name = 'CacheReadDeniedError' + Object.setPrototypeOf(this, CacheReadDeniedError.prototype) + } +} + export class FinalizeCacheError extends Error { constructor(message: string) { super(message) @@ -369,6 +404,15 @@ async function restoreCacheV2( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error + } else if (typedError.message?.includes(CACHE_READ_DENIED_PREFIX)) { + // The receiver returns a twirp PermissionDenied (HTTP 403) when the + // run's token has no readable cache scopes. The twirp client wraps that + // 403 in a generic Error, so the stable `cache read denied:` prefix is + // embedded in the message rather than at the start. Surface a + // policy-specific warning and treat it as a cache miss so the workflow + // continues. + const readDeniedError = new CacheReadDeniedError(typedError.message) + core.warning(`Failed to restore: ${readDeniedError.message}`) } else { // Supress all non-validation cache related errors because caching should be optional // Log server errors (5xx) as errors, all other errors as warnings From 4262e2521d350bb314f6dedc3b053145420b5300 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 12:32:30 -0500 Subject: [PATCH 02/15] chore(cache): trim comments, bump to 6.2.0, add RELEASES entry --- packages/cache/RELEASES.md | 4 ++ .../cache/__tests__/restoreCacheV2.test.ts | 14 ++----- packages/cache/package-lock.json | 4 +- packages/cache/package.json | 2 +- packages/cache/src/cache.ts | 40 +++++-------------- 5 files changed, 19 insertions(+), 45 deletions(-) diff --git a/packages/cache/RELEASES.md b/packages/cache/RELEASES.md index cc9e33a202..7671212b8f 100644 --- a/packages/cache/RELEASES.md +++ b/packages/cache/RELEASES.md @@ -1,5 +1,9 @@ # @actions/cache Releases +## 6.2.0 + +- Handle cache read error due to read-only token: detect the `cache read denied:` prefix on cache download failures and surface it as a `core.warning` (without failing the run). + ## 6.1.0 - Handle cache write error due to read-only token: detect the `cache write denied:` prefix on cache reservation failures and surface it as a `core.warning` (without failing the run). diff --git a/packages/cache/__tests__/restoreCacheV2.test.ts b/packages/cache/__tests__/restoreCacheV2.test.ts index 7046412a72..4379b1f958 100644 --- a/packages/cache/__tests__/restoreCacheV2.test.ts +++ b/packages/cache/__tests__/restoreCacheV2.test.ts @@ -113,18 +113,13 @@ test('restore with server error should fail', async () => { }) test('restore denied by read-only token logs warning and reports cache miss', async () => { - // When the receiver refuses the download URL because the run's token has no - // readable cache scopes, it returns a twirp PermissionDenied (HTTP 403). - // The twirp client wraps that 403 into a generic Error, so the stable - // `cache read denied:` prefix arrives embedded in the message (not at the - // start). The toolkit must dispatch on that prefix, log a single warning - // (not an error, even though the underlying status is 403), and report a - // cache miss so the workflow continues. + // The receiver returns twirp PermissionDenied (403) when the run's token has + // no readable cache scopes; the client wraps it so the `cache read denied:` + // prefix arrives embedded. Expect a single warning (not error) and a miss. const paths = ['node_modules'] const key = 'node-test' const logErrorMock = jest.spyOn(core, 'error') const logWarningMock = jest.spyOn(core, 'warning') - // Realistic wrapped shape produced by cacheTwirpClient for a 403 response. const wrappedDeniedMessage = 'Failed to GetCacheEntryDownloadURL: Received non-retryable error: ' + 'Failed request: (403) Forbidden: cache read denied: token has no readable scopes' @@ -137,10 +132,7 @@ test('restore denied by read-only token logs warning and reports cache miss', as const cacheKey = await restoreCache(paths, key) expect(cacheKey).toBe(undefined) - // 403 must not be logged as an error. expect(logErrorMock).not.toHaveBeenCalled() - // A single warning that carries the stable `cache read denied:` prefix so - // the runner UI and consumers can dispatch on it. expect(logWarningMock).toHaveBeenCalledWith( `Failed to restore: ${wrappedDeniedMessage}` ) diff --git a/packages/cache/package-lock.json b/packages/cache/package-lock.json index 92530faf35..1226ca4e6b 100644 --- a/packages/cache/package-lock.json +++ b/packages/cache/package-lock.json @@ -1,12 +1,12 @@ { "name": "@actions/cache", - "version": "6.1.0", + "version": "6.2.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@actions/cache", - "version": "6.1.0", + "version": "6.2.0", "license": "MIT", "dependencies": { "@actions/core": "^3.0.1", diff --git a/packages/cache/package.json b/packages/cache/package.json index 209212de77..bc7115f50c 100644 --- a/packages/cache/package.json +++ b/packages/cache/package.json @@ -1,6 +1,6 @@ { "name": "@actions/cache", - "version": "6.1.0", + "version": "6.2.0", "description": "Actions cache lib", "keywords": [ "github", diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index e467345bb0..fa5ca7e50a 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -62,33 +62,15 @@ export class CacheWriteDeniedError extends ReserveCacheError { } } -/** - * Stable prefix the receiver embeds in the twirp error it returns from - * GetCacheEntryDownloadURL when the issuer scoped the run's token so that it - * has no readable cache scopes (a policy read denial). restoreCacheV2 - * dispatches on this prefix to re-classify the failure as a - * CacheReadDeniedError so it can surface a policy-specific warning instead of - * a generic restore failure. - * - * Note: unlike the write path, GetCacheEntryDownloadURLResponse has no - * `message` field, so the receiver signals the denial as a twirp - * PermissionDenied (HTTP 403). The twirp client wraps that 403 in a generic - * Error, so this prefix appears embedded in the thrown error's message rather - * than at the very start -- match with `includes`, not `startsWith`. - */ +// Prefix the receiver embeds in the twirp error from GetCacheEntryDownloadURL +// when the run's token has no readable cache scopes. Match with `includes`, +// not `startsWith`: the 403 is wrapped by the twirp client, so the prefix ends +// up embedded in the message. export const CACHE_READ_DENIED_PREFIX = 'cache read denied:' -/** - * Raised when the cache backend refuses to return a cache download URL because - * the JWT issued for this run has no readable cache scopes (for example, the - * run was triggered by an event the repository administrator classified as - * untrusted). The receiver-supplied detail message contains - * `cache read denied:` (the full error message includes additional context). - * - * Caching is best-effort, so restoreCacheV2 does not rethrow this to - * consumers; it logs a policy-specific warning and reports a cache miss so the - * workflow continues. - */ +// Raised when the cache backend denies a download URL because the run's token +// has no readable cache scopes. Caching is best-effort, so restoreCacheV2 logs +// a warning and reports a cache miss rather than rethrowing this. export class CacheReadDeniedError extends Error { constructor(message: string) { super(message) @@ -405,12 +387,8 @@ async function restoreCacheV2( if (typedError.name === ValidationError.name) { throw error } else if (typedError.message?.includes(CACHE_READ_DENIED_PREFIX)) { - // The receiver returns a twirp PermissionDenied (HTTP 403) when the - // run's token has no readable cache scopes. The twirp client wraps that - // 403 in a generic Error, so the stable `cache read denied:` prefix is - // embedded in the message rather than at the start. Surface a - // policy-specific warning and treat it as a cache miss so the workflow - // continues. + // Read denied by policy (token has no readable cache scopes). Warn and + // treat as a cache miss so the workflow continues. const readDeniedError = new CacheReadDeniedError(typedError.message) core.warning(`Failed to restore: ${readDeniedError.message}`) } else { From 4fe17c45c2b999afaec8d017f25e3091e6b4e1fa Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 12:36:21 -0500 Subject: [PATCH 03/15] refactor(cache): dispatch read-denied by error name to mirror write path Re-throw CacheReadDeniedError from an inner try/catch around GetCacheEntryDownloadURL and dispatch on typedError.name in the outer catch, matching how saveCacheV2 handles CacheWriteDeniedError. --- packages/cache/src/cache.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index fa5ca7e50a..cde2e2a38a 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -331,7 +331,19 @@ async function restoreCacheV2( ) } - const response = await twirpClient.GetCacheEntryDownloadURL(request) + let response + try { + response = await twirpClient.GetCacheEntryDownloadURL(request) + } catch (error) { + // The receiver returns twirp PermissionDenied (403) when the run's token + // has no readable cache scopes. The client wraps that 403, so the stable + // prefix is embedded in the message rather than leading it. + const errorMessage = (error as Error)?.message ?? '' + if (errorMessage.includes(CACHE_READ_DENIED_PREFIX)) { + throw new CacheReadDeniedError(errorMessage) + } + throw error + } if (!response.ok) { core.debug( @@ -386,11 +398,10 @@ async function restoreCacheV2( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error - } else if (typedError.message?.includes(CACHE_READ_DENIED_PREFIX)) { + } else if (typedError.name === CacheReadDeniedError.name) { // Read denied by policy (token has no readable cache scopes). Warn and // treat as a cache miss so the workflow continues. - const readDeniedError = new CacheReadDeniedError(typedError.message) - core.warning(`Failed to restore: ${readDeniedError.message}`) + core.warning(`Failed to restore: ${typedError.message}`) } else { // Supress all non-validation cache related errors because caching should be optional // Log server errors (5xx) as errors, all other errors as warnings From a924773f3a0b9b808e06f4759306f846901665ca Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 13:09:08 -0500 Subject: [PATCH 04/15] feat(cache): handle read-denied on the v1 restore path Extend the read-denied handling to Cache Service v1 so GHES (which forces v1 via _apis/artifactcache) is covered when read-scope enforcement ships there. - Surface the receiver's error body message from getCacheEntry instead of a generic status-code error, so the cache read denied: prefix reaches callers - Re-throw CacheReadDeniedError from restoreCacheV1 and dispatch on it in the outer catch, mirroring restoreCacheV2 and the write-denied v1 handling - Add a v1 read-denied test --- packages/cache/RELEASES.md | 2 +- packages/cache/__tests__/restoreCache.test.ts | 24 ++++++++++++ packages/cache/src/cache.ts | 37 ++++++++++++++----- .../cache/src/internal/cacheHttpClient.ts | 7 +++- 4 files changed, 58 insertions(+), 12 deletions(-) diff --git a/packages/cache/RELEASES.md b/packages/cache/RELEASES.md index 7671212b8f..c61e60d37d 100644 --- a/packages/cache/RELEASES.md +++ b/packages/cache/RELEASES.md @@ -2,7 +2,7 @@ ## 6.2.0 -- Handle cache read error due to read-only token: detect the `cache read denied:` prefix on cache download failures and surface it as a `core.warning` (without failing the run). +- Handle cache read error due to read-only token: detect the `cache read denied:` prefix on cache download failures (both the v2 twirp path and the v1 `_apis/artifactcache` path) and surface it as a `core.warning` (without failing the run). ## 6.1.0 diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 4b5c6f865f..2c51b7ea45 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -99,6 +99,30 @@ test('restore with server error should fail', async () => { delete process.env['ACTIONS_RESULTS_URL'] }) +test('restore denied by read-only token logs warning and reports cache miss', async () => { + // The GHES v1 artifact cache service returns HTTP 403 with a + // `cache read denied:` body when the token has no readable scopes. + // getCacheEntry surfaces that message; expect a single warning (not error) + // and a cache miss. + const paths = ['node_modules'] + const key = 'node-test' + const logErrorMock = jest.spyOn(core, 'error') + const logWarningMock = jest.spyOn(core, 'warning') + const deniedMessage = 'cache read denied: token has no readable scopes' + + jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(async () => { + throw new Error(deniedMessage) + }) + + const cacheKey = await restoreCache(paths, key) + expect(cacheKey).toBe(undefined) + expect(logErrorMock).not.toHaveBeenCalled() + expect(logWarningMock).toHaveBeenCalledWith( + `Failed to restore: ${deniedMessage}` + ) + expect(logWarningMock).toHaveBeenCalledTimes(1) +}) + test('restore with restore keys and no cache found', async () => { const paths = ['node_modules'] const key = 'node-test' diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index cde2e2a38a..ed7bf54f48 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -62,15 +62,15 @@ export class CacheWriteDeniedError extends ReserveCacheError { } } -// Prefix the receiver embeds in the twirp error from GetCacheEntryDownloadURL -// when the run's token has no readable cache scopes. Match with `includes`, -// not `startsWith`: the 403 is wrapped by the twirp client, so the prefix ends -// up embedded in the message. +// Prefix the receiver embeds in a cache read denial: the v2 twirp +// GetCacheEntryDownloadURL error, or the GHES v1 `_apis/artifactcache` 403 +// body. Match with `includes`, not `startsWith`: the message is wrapped by +// the transport, so the prefix ends up embedded rather than leading. export const CACHE_READ_DENIED_PREFIX = 'cache read denied:' // Raised when the cache backend denies a download URL because the run's token -// has no readable cache scopes. Caching is best-effort, so restoreCacheV2 logs -// a warning and reports a cache miss rather than rethrowing this. +// has no readable cache scopes. Caching is best-effort, so restoreCache logs a +// warning and reports a cache miss rather than rethrowing this. export class CacheReadDeniedError extends Error { constructor(message: string) { super(message) @@ -208,10 +208,23 @@ async function restoreCacheV1( let archivePath = '' try { // path are needed to compute version - const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { - compressionMethod, - enableCrossOsArchive - }) + let cacheEntry + try { + cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { + compressionMethod, + enableCrossOsArchive + }) + } catch (error) { + // The GHES v1 artifact cache service returns HTTP 403 with a + // `cache read denied:` body when the run's token has no readable cache + // scopes. getCacheEntry surfaces that body message, so re-classify it + // here to mirror the read-denied handling on the v2 path. + const errorMessage = (error as Error)?.message ?? '' + if (errorMessage.includes(CACHE_READ_DENIED_PREFIX)) { + throw new CacheReadDeniedError(errorMessage) + } + throw error + } if (!cacheEntry?.archiveLocation) { // Cache not found return undefined @@ -254,6 +267,10 @@ async function restoreCacheV1( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error + } else if (typedError.name === CacheReadDeniedError.name) { + // Read denied by policy (token has no readable cache scopes). Warn and + // treat as a cache miss so the workflow continues. + core.warning(`Failed to restore: ${typedError.message}`) } else { // warn on cache restore failure and continue build // Log server errors (5xx) as errors, all other errors as warnings diff --git a/packages/cache/src/internal/cacheHttpClient.ts b/packages/cache/src/internal/cacheHttpClient.ts index 879d73f079..b277d4957b 100644 --- a/packages/cache/src/internal/cacheHttpClient.ts +++ b/packages/cache/src/internal/cacheHttpClient.ts @@ -101,7 +101,12 @@ export async function getCacheEntry( return null } if (!isSuccessStatusCode(response.statusCode)) { - throw new Error(`Cache service responded with ${response.statusCode}`) + // Surface the receiver's error message (e.g. a `cache read denied:` policy + // denial) so callers can dispatch on it, falling back to the status code. + throw new Error( + response.error?.message ?? + `Cache service responded with ${response.statusCode}` + ) } const cacheResult = response.result From 0555e65a40f6347ef4a4ebe1587fc673392bc8c5 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 13:36:10 -0500 Subject: [PATCH 05/15] refactor(cache): only surface receiver body for read-denied on v1 --- packages/cache/src/internal/cacheHttpClient.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/cache/src/internal/cacheHttpClient.ts b/packages/cache/src/internal/cacheHttpClient.ts index b277d4957b..296f29fe60 100644 --- a/packages/cache/src/internal/cacheHttpClient.ts +++ b/packages/cache/src/internal/cacheHttpClient.ts @@ -101,12 +101,13 @@ export async function getCacheEntry( return null } if (!isSuccessStatusCode(response.statusCode)) { - // Surface the receiver's error message (e.g. a `cache read denied:` policy - // denial) so callers can dispatch on it, falling back to the status code. - throw new Error( - response.error?.message ?? - `Cache service responded with ${response.statusCode}` - ) + // Only surface the receiver's body for a `cache read denied:` policy denial + // so callers can dispatch on it; keep the generic message otherwise. + const errorMessage = response.error?.message + if (errorMessage?.includes('cache read denied:')) { + throw new Error(errorMessage) + } + throw new Error(`Cache service responded with ${response.statusCode}`) } const cacheResult = response.result From 47f41470a2512d4fd12373b85cf803fbdf8129fa Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 13:38:49 -0500 Subject: [PATCH 06/15] test(cache): assert getCacheEntry only surfaces body for read-denied --- .../cache/__tests__/cacheHttpClient.test.ts | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/packages/cache/__tests__/cacheHttpClient.test.ts b/packages/cache/__tests__/cacheHttpClient.test.ts index e2201cd1cb..5cc129f78b 100644 --- a/packages/cache/__tests__/cacheHttpClient.test.ts +++ b/packages/cache/__tests__/cacheHttpClient.test.ts @@ -1,8 +1,10 @@ -import {downloadCache} from '../src/internal/cacheHttpClient' +import {downloadCache, getCacheEntry} from '../src/internal/cacheHttpClient' import {getCacheVersion} from '../src/internal/cacheUtils' import {CompressionMethod} from '../src/internal/constants' import * as downloadUtils from '../src/internal/downloadUtils' +import * as requestUtils from '../src/internal/requestUtils' import {DownloadOptions, getDownloadOptions} from '../src/options' +import {HttpClientError} from '@actions/http-client' jest.mock('../src/internal/downloadUtils') @@ -57,6 +59,37 @@ test('getCacheVersion with enableCrossOsArchive as false returns version on wind } }) +test('getCacheEntry throws a generic status-code error for non-read-denied failures', async () => { + // Regression: a non read-denied failure must NOT leak the receiver's body + // message; it should surface the generic status-code error. + jest.spyOn(requestUtils, 'retryTypedResponse').mockResolvedValue({ + statusCode: 403, + result: null, + headers: {}, + error: new HttpClientError('some other server detail', 403) + }) + + await expect(getCacheEntry(['key'], ['node_modules'])).rejects.toThrow( + 'Cache service responded with 403' + ) +}) + +test('getCacheEntry surfaces the body message for a cache read denial', async () => { + jest.spyOn(requestUtils, 'retryTypedResponse').mockResolvedValue({ + statusCode: 403, + result: null, + headers: {}, + error: new HttpClientError( + 'cache read denied: token has no readable scopes', + 403 + ) + }) + + await expect(getCacheEntry(['key'], ['node_modules'])).rejects.toThrow( + 'cache read denied: token has no readable scopes' + ) +}) + test('downloadCache uses http-client for non-Azure URLs', async () => { const downloadCacheHttpClientMock = jest.spyOn( downloadUtils, From 345211808d95a7e024b10d12541b412d1b943304 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 13:40:35 -0500 Subject: [PATCH 07/15] test(cache): cover non-read-denied getCacheEntry passthrough on v1 --- packages/cache/__tests__/restoreCache.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 2c51b7ea45..8c592aebe0 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -123,6 +123,29 @@ test('restore denied by read-only token logs warning and reports cache miss', as expect(logWarningMock).toHaveBeenCalledTimes(1) }) +test('restore surfaces a non-read-denied getCacheEntry error as a normal warning', async () => { + // Guards the inner catch in restoreCacheV1: only `cache read denied:` errors + // are re-classified; every other getCacheEntry failure must pass through and + // be logged normally (not swallowed or mislabeled as a read denial). + const paths = ['node_modules'] + const key = 'node-test' + const logErrorMock = jest.spyOn(core, 'error') + const logWarningMock = jest.spyOn(core, 'warning') + const genericMessage = 'Cache service responded with 400' + + jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(async () => { + throw new Error(genericMessage) + }) + + const cacheKey = await restoreCache(paths, key) + expect(cacheKey).toBe(undefined) + expect(logErrorMock).not.toHaveBeenCalled() + expect(logWarningMock).toHaveBeenCalledWith( + `Failed to restore: ${genericMessage}` + ) + expect(logWarningMock).toHaveBeenCalledTimes(1) +}) + test('restore with restore keys and no cache found', async () => { const paths = ['node_modules'] const key = 'node-test' From b504f4ff4a23a06538b3a8eac0400e56615e8ed9 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Wed, 1 Jul 2026 15:23:05 -0500 Subject: [PATCH 08/15] refactor(cache): share read-denied prefix via constants to avoid drift --- packages/cache/src/cache.ts | 9 ++++----- packages/cache/src/internal/cacheHttpClient.ts | 3 ++- packages/cache/src/internal/constants.ts | 5 +++++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index ed7bf54f48..ed4167c602 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -13,6 +13,7 @@ import { GetCacheEntryDownloadURLRequest } from './generated/results/api/v1/cache.js' import {HttpClientError} from '@actions/http-client' +import {CacheReadDeniedMessagePrefix} from './internal/constants.js' export type {DownloadOptions, UploadOptions} export class ValidationError extends Error { @@ -62,11 +63,9 @@ export class CacheWriteDeniedError extends ReserveCacheError { } } -// Prefix the receiver embeds in a cache read denial: the v2 twirp -// GetCacheEntryDownloadURL error, or the GHES v1 `_apis/artifactcache` 403 -// body. Match with `includes`, not `startsWith`: the message is wrapped by -// the transport, so the prefix ends up embedded rather than leading. -export const CACHE_READ_DENIED_PREFIX = 'cache read denied:' +// Re-exported from constants so consumers keep referencing it here; the shared +// value also drives detection in cacheHttpClient without duplicating the string. +export const CACHE_READ_DENIED_PREFIX = CacheReadDeniedMessagePrefix // Raised when the cache backend denies a download URL because the run's token // has no readable cache scopes. Caching is best-effort, so restoreCache logs a diff --git a/packages/cache/src/internal/cacheHttpClient.ts b/packages/cache/src/internal/cacheHttpClient.ts index 296f29fe60..3b364814d5 100644 --- a/packages/cache/src/internal/cacheHttpClient.ts +++ b/packages/cache/src/internal/cacheHttpClient.ts @@ -35,6 +35,7 @@ import { retryTypedResponse } from './requestUtils.js' import {getCacheServiceURL} from './config.js' +import {CacheReadDeniedMessagePrefix} from './constants.js' import {getUserAgentString} from './shared/user-agent.js' function getCacheApiUrl(resource: string): string { @@ -104,7 +105,7 @@ export async function getCacheEntry( // Only surface the receiver's body for a `cache read denied:` policy denial // so callers can dispatch on it; keep the generic message otherwise. const errorMessage = response.error?.message - if (errorMessage?.includes('cache read denied:')) { + if (errorMessage?.includes(CacheReadDeniedMessagePrefix)) { throw new Error(errorMessage) } throw new Error(`Cache service responded with ${response.statusCode}`) diff --git a/packages/cache/src/internal/constants.ts b/packages/cache/src/internal/constants.ts index 8c5d1ee440..cec23808dc 100644 --- a/packages/cache/src/internal/constants.ts +++ b/packages/cache/src/internal/constants.ts @@ -38,3 +38,8 @@ export const TarFilename = 'cache.tar' export const ManifestFilename = 'manifest.txt' export const CacheFileSizeLimit = 10 * Math.pow(1024, 3) // 10GiB per repository + +// Prefix the cache backend embeds in a read-denial message (v2 twirp +// GetCacheEntryDownloadURL error or the GHES v1 `_apis/artifactcache` 403 body). +// Shared so cache.ts and cacheHttpClient.ts match the same contract value. +export const CacheReadDeniedMessagePrefix = 'cache read denied:' From 22b0367c603b3ecd50eb04d4c6a1a940361dddfb Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Thu, 2 Jul 2026 10:29:32 -0500 Subject: [PATCH 09/15] feat(cache): skip restore/save per ACTIONS_CACHE_MODE --- packages/cache/RELEASES.md | 1 + packages/cache/__tests__/config.test.ts | 34 ++++++++++ packages/cache/__tests__/restoreCache.test.ts | 51 ++++++++++++++ .../cache/__tests__/restoreCacheV2.test.ts | 13 ++++ packages/cache/__tests__/saveCache.test.ts | 66 +++++++++++++++++++ packages/cache/src/cache.ts | 31 ++++++++- packages/cache/src/internal/config.ts | 20 ++++++ 7 files changed, 215 insertions(+), 1 deletion(-) diff --git a/packages/cache/RELEASES.md b/packages/cache/RELEASES.md index c61e60d37d..fb668e6974 100644 --- a/packages/cache/RELEASES.md +++ b/packages/cache/RELEASES.md @@ -3,6 +3,7 @@ ## 6.2.0 - Handle cache read error due to read-only token: detect the `cache read denied:` prefix on cache download failures (both the v2 twirp path and the v1 `_apis/artifactcache` path) and surface it as a `core.warning` (without failing the run). +- Honor the `ACTIONS_CACHE_MODE` environment variable: skip restore when the effective cache-mode does not permit reads (`none`, `write-only`) and skip save when it does not permit writes (`none`, `read`), logging a single non-fatal `core.info` line. When `ACTIONS_CACHE_MODE` is unset or unrecognized, behavior is unchanged. ## 6.1.0 diff --git a/packages/cache/__tests__/config.test.ts b/packages/cache/__tests__/config.test.ts index 52d86d3620..7582bb3251 100644 --- a/packages/cache/__tests__/config.test.ts +++ b/packages/cache/__tests__/config.test.ts @@ -23,3 +23,37 @@ test('isGhes returns false for ghe.localhost', () => { process.env.GITHUB_SERVER_URL = 'https://my.domain.ghe.localhost' expect(config.isGhes()).toBe(false) }) + +describe('cache-mode helpers', () => { + const original = process.env.ACTIONS_CACHE_MODE + + afterEach(() => { + if (original === undefined) { + delete process.env.ACTIONS_CACHE_MODE + } else { + process.env.ACTIONS_CACHE_MODE = original + } + }) + + test('getCacheMode normalizes whitespace and case', () => { + process.env.ACTIONS_CACHE_MODE = ' Write-Only ' + expect(config.getCacheMode()).toBe('write-only') + }) + + test('getCacheMode returns empty string when unset', () => { + delete process.env.ACTIONS_CACHE_MODE + expect(config.getCacheMode()).toBe('') + }) + + test.each([ + ['', true, true], + ['read', true, false], + ['write', true, true], + ['write-only', false, true], + ['none', false, false], + ['garbage', true, true] + ])("mode '%s' -> readable=%s writable=%s", (mode, readable, writable) => { + expect(config.isCacheReadable(mode)).toBe(readable) + expect(config.isCacheWritable(mode)).toBe(writable) + }) +}) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 8c592aebe0..38d237e724 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -146,6 +146,57 @@ test('restore surfaces a non-read-denied getCacheEntry error as a normal warning expect(logWarningMock).toHaveBeenCalledTimes(1) }) +describe('restore cache-mode gating', () => { + const original = process.env.ACTIONS_CACHE_MODE + + afterEach(() => { + if (original === undefined) { + delete process.env.ACTIONS_CACHE_MODE + } else { + process.env.ACTIONS_CACHE_MODE = original + } + }) + + test.each(['none', 'write-only'])( + "mode '%s' skips restore without touching the cache service", + async mode => { + process.env.ACTIONS_CACHE_MODE = mode + const logInfoMock = jest.spyOn(core, 'info') + const getCacheEntryMock = jest.spyOn(cacheHttpClient, 'getCacheEntry') + + const cacheKey = await restoreCache(['node_modules'], 'node-test') + + expect(cacheKey).toBe(undefined) + expect(getCacheEntryMock).not.toHaveBeenCalled() + expect(logInfoMock).toHaveBeenCalledWith( + `Cache restore skipped: the effective cache-mode '${mode}' does not permit reads.` + ) + } + ) + + test.each(['read', 'write', ''])( + "mode '%s' does not skip restore", + async mode => { + if (mode === '') { + delete process.env.ACTIONS_CACHE_MODE + } else { + process.env.ACTIONS_CACHE_MODE = mode + } + const logInfoMock = jest.spyOn(core, 'info') + const getCacheEntryMock = jest + .spyOn(cacheHttpClient, 'getCacheEntry') + .mockResolvedValue(null as never) + + await restoreCache(['node_modules'], 'node-test') + + expect(getCacheEntryMock).toHaveBeenCalledTimes(1) + expect(logInfoMock).not.toHaveBeenCalledWith( + expect.stringContaining('Cache restore skipped') + ) + } + ) +}) + test('restore with restore keys and no cache found', async () => { const paths = ['node_modules'] const key = 'node-test' diff --git a/packages/cache/__tests__/restoreCacheV2.test.ts b/packages/cache/__tests__/restoreCacheV2.test.ts index 4379b1f958..0b62baa463 100644 --- a/packages/cache/__tests__/restoreCacheV2.test.ts +++ b/packages/cache/__tests__/restoreCacheV2.test.ts @@ -33,6 +33,19 @@ beforeAll(() => { // Ensure that we're using v2 for these tests jest.spyOn(config, 'getCacheServiceVersion').mockReturnValue('v2') + // config is auto-mocked; use the real cache-mode helpers so gating reflects + // ACTIONS_CACHE_MODE and unset stays permissive. + const actualConfig = jest.requireActual('../src/internal/config') + jest + .spyOn(config, 'getCacheMode') + .mockImplementation(actualConfig.getCacheMode) + jest + .spyOn(config, 'isCacheReadable') + .mockImplementation(actualConfig.isCacheReadable) + jest + .spyOn(config, 'isCacheWritable') + .mockImplementation(actualConfig.isCacheWritable) + logDebugMock = jest.spyOn(core, 'debug') logInfoMock = jest.spyOn(core, 'info') }) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index 917846425b..f8aacb2266 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -35,6 +35,18 @@ beforeAll(() => { jest.spyOn(cacheUtils, 'createTempDirectory').mockImplementation(async () => { return Promise.resolve('/foo/bar') }) + // config is auto-mocked; use the real cache-mode helpers so gating reflects + // ACTIONS_CACHE_MODE and unset stays permissive. + const actualConfig = jest.requireActual('../src/internal/config') + jest + .spyOn(config, 'getCacheMode') + .mockImplementation(actualConfig.getCacheMode) + jest + .spyOn(config, 'isCacheReadable') + .mockImplementation(actualConfig.isCacheReadable) + jest + .spyOn(config, 'isCacheWritable') + .mockImplementation(actualConfig.isCacheWritable) }) test('save with missing input should fail', async () => { @@ -45,6 +57,60 @@ test('save with missing input should fail', async () => { ) }) +describe('save cache-mode gating', () => { + const original = process.env.ACTIONS_CACHE_MODE + + afterEach(() => { + if (original === undefined) { + delete process.env.ACTIONS_CACHE_MODE + } else { + process.env.ACTIONS_CACHE_MODE = original + } + }) + + test.each(['read', 'none'])( + "mode '%s' skips save without touching the cache service", + async mode => { + process.env.ACTIONS_CACHE_MODE = mode + const logInfoMock = jest.spyOn(core, 'info') + const resolvePathsMock = jest.spyOn(cacheUtils, 'resolvePaths') + + const cacheId = await saveCache(['node_modules'], 'node-test') + + expect(cacheId).toBe(-1) + expect(resolvePathsMock).not.toHaveBeenCalled() + expect(logInfoMock).toHaveBeenCalledWith( + `Cache save skipped: the effective cache-mode '${mode}' does not permit writes.` + ) + } + ) + + test.each(['write', 'write-only', ''])( + "mode '%s' does not skip save", + async mode => { + if (mode === '') { + delete process.env.ACTIONS_CACHE_MODE + } else { + process.env.ACTIONS_CACHE_MODE = mode + } + const logInfoMock = jest.spyOn(core, 'info') + const resolvePathsMock = jest.spyOn(cacheUtils, 'resolvePaths') + + try { + await saveCache(['node_modules'], 'node-test') + } catch { + // Downstream client is not fully mocked here; we only assert the guard + // let execution proceed past it. + } + + expect(resolvePathsMock).toHaveBeenCalled() + expect(logInfoMock).not.toHaveBeenCalledWith( + expect.stringContaining('Cache save skipped') + ) + } + ) +}) + test('save with large cache outputs should fail', async () => { const filePath = 'node_modules' const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index ed4167c602..0183a9a613 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -3,7 +3,13 @@ import * as path from 'path' import * as utils from './internal/cacheUtils.js' import * as cacheHttpClient from './internal/cacheHttpClient.js' import * as cacheTwirpClient from './internal/shared/cacheTwirpClient.js' -import {getCacheServiceVersion, isGhes} from './internal/config.js' +import { + getCacheServiceVersion, + isGhes, + getCacheMode, + isCacheReadable, + isCacheWritable +} from './internal/config.js' import {DownloadOptions, UploadOptions} from './options.js' import {createTar, extractTar, listTar} from './internal/tar.js' import { @@ -150,6 +156,17 @@ export async function restoreCache( checkPaths(paths) + const cacheMode = getCacheMode() + if (!isCacheReadable(cacheMode)) { + core.info( + `Cache restore skipped: the effective cache-mode '${cacheMode}' does not permit reads.` + ) + core.debug( + `Skipped restore for paths [${paths.join(', ')}] with primary key '${primaryKey}'.` + ) + return undefined + } + switch (cacheServiceVersion) { case 'v2': return await restoreCacheV2( @@ -463,6 +480,18 @@ export async function saveCache( core.debug(`Cache service version: ${cacheServiceVersion}`) checkPaths(paths) checkKey(key) + + const cacheMode = getCacheMode() + if (!isCacheWritable(cacheMode)) { + core.info( + `Cache save skipped: the effective cache-mode '${cacheMode}' does not permit writes.` + ) + core.debug( + `Skipped save for paths [${paths.join(', ')}] with key '${key}'.` + ) + return -1 + } + switch (cacheServiceVersion) { case 'v2': return await saveCacheV2(paths, key, options, enableCrossOsArchive) diff --git a/packages/cache/src/internal/config.ts b/packages/cache/src/internal/config.ts index 24b9fa1af8..68d433d474 100644 --- a/packages/cache/src/internal/config.ts +++ b/packages/cache/src/internal/config.ts @@ -19,6 +19,26 @@ export function getCacheServiceVersion(): string { return process.env['ACTIONS_CACHE_SERVICE_V2'] ? 'v2' : 'v1' } +// The cache-mode lattice (ADR c2c-actions#10264): readable = {read, write}, +// writable = {write, write-only}, none = neither. +const KNOWN_CACHE_MODES = ['none', 'read', 'write', 'write-only'] + +// The effective cache-mode exported by the runner, or '' when not set. +export function getCacheMode(): string { + return (process.env['ACTIONS_CACHE_MODE'] || '').trim().toLowerCase() +} + +// Unset or unrecognized modes are permissive so behavior matches today. +export function isCacheReadable(mode: string): boolean { + if (!KNOWN_CACHE_MODES.includes(mode)) return true + return mode === 'read' || mode === 'write' +} + +export function isCacheWritable(mode: string): boolean { + if (!KNOWN_CACHE_MODES.includes(mode)) return true + return mode === 'write' || mode === 'write-only' +} + export function getCacheServiceURL(): string { const version = getCacheServiceVersion() From 15fe834764cc5b927bf5e63a753fb046a5ed4270 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Thu, 2 Jul 2026 11:00:02 -0500 Subject: [PATCH 10/15] test(cache): expand ACTIONS_CACHE_MODE skip coverage across v1/v2 and unknown modes --- packages/cache/__tests__/restoreCache.test.ts | 40 ++++++++++++++++--- packages/cache/__tests__/saveCache.test.ts | 40 ++++++++++++++++--- 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 38d237e724..64a0084464 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -147,14 +147,20 @@ test('restore surfaces a non-read-denied getCacheEntry error as a normal warning }) describe('restore cache-mode gating', () => { - const original = process.env.ACTIONS_CACHE_MODE + const originalMode = process.env.ACTIONS_CACHE_MODE + const originalV2 = process.env.ACTIONS_CACHE_SERVICE_V2 - afterEach(() => { - if (original === undefined) { - delete process.env.ACTIONS_CACHE_MODE + const restoreEnv = (key: string, value: string | undefined): void => { + if (value === undefined) { + delete process.env[key] } else { - process.env.ACTIONS_CACHE_MODE = original + process.env[key] = value } + } + + afterEach(() => { + restoreEnv('ACTIONS_CACHE_MODE', originalMode) + restoreEnv('ACTIONS_CACHE_SERVICE_V2', originalV2) }) test.each(['none', 'write-only'])( @@ -168,13 +174,35 @@ describe('restore cache-mode gating', () => { expect(cacheKey).toBe(undefined) expect(getCacheEntryMock).not.toHaveBeenCalled() + expect(logInfoMock).toHaveBeenCalledTimes(1) expect(logInfoMock).toHaveBeenCalledWith( `Cache restore skipped: the effective cache-mode '${mode}' does not permit reads.` ) } ) - test.each(['read', 'write', ''])( + // The skip short-circuits before v1/v2 dispatch, so it applies regardless of + // the ACTIONS_CACHE_SERVICE_V2 feature flag. + test.each([ + ['none', undefined], + ['none', 'true'], + ['write-only', undefined], + ['write-only', 'true'] + ])( + "mode '%s' skips restore with ACTIONS_CACHE_SERVICE_V2=%s", + async (mode, v2) => { + process.env.ACTIONS_CACHE_MODE = mode + restoreEnv('ACTIONS_CACHE_SERVICE_V2', v2) + const getCacheEntryMock = jest.spyOn(cacheHttpClient, 'getCacheEntry') + + const cacheKey = await restoreCache(['node_modules'], 'node-test') + + expect(cacheKey).toBe(undefined) + expect(getCacheEntryMock).not.toHaveBeenCalled() + } + ) + + test.each(['read', 'write', '', 'garbage'])( "mode '%s' does not skip restore", async mode => { if (mode === '') { diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index f8aacb2266..b1a32f4577 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -58,14 +58,20 @@ test('save with missing input should fail', async () => { }) describe('save cache-mode gating', () => { - const original = process.env.ACTIONS_CACHE_MODE + const originalMode = process.env.ACTIONS_CACHE_MODE + const originalV2 = process.env.ACTIONS_CACHE_SERVICE_V2 - afterEach(() => { - if (original === undefined) { - delete process.env.ACTIONS_CACHE_MODE + const restoreEnv = (key: string, value: string | undefined): void => { + if (value === undefined) { + delete process.env[key] } else { - process.env.ACTIONS_CACHE_MODE = original + process.env[key] = value } + } + + afterEach(() => { + restoreEnv('ACTIONS_CACHE_MODE', originalMode) + restoreEnv('ACTIONS_CACHE_SERVICE_V2', originalV2) }) test.each(['read', 'none'])( @@ -79,13 +85,35 @@ describe('save cache-mode gating', () => { expect(cacheId).toBe(-1) expect(resolvePathsMock).not.toHaveBeenCalled() + expect(logInfoMock).toHaveBeenCalledTimes(1) expect(logInfoMock).toHaveBeenCalledWith( `Cache save skipped: the effective cache-mode '${mode}' does not permit writes.` ) } ) - test.each(['write', 'write-only', ''])( + // The skip short-circuits before v1/v2 dispatch, so it applies regardless of + // the ACTIONS_CACHE_SERVICE_V2 feature flag. + test.each([ + ['read', undefined], + ['read', 'true'], + ['none', undefined], + ['none', 'true'] + ])( + "mode '%s' skips save with ACTIONS_CACHE_SERVICE_V2=%s", + async (mode, v2) => { + process.env.ACTIONS_CACHE_MODE = mode + restoreEnv('ACTIONS_CACHE_SERVICE_V2', v2) + const resolvePathsMock = jest.spyOn(cacheUtils, 'resolvePaths') + + const cacheId = await saveCache(['node_modules'], 'node-test') + + expect(cacheId).toBe(-1) + expect(resolvePathsMock).not.toHaveBeenCalled() + } + ) + + test.each(['write', 'write-only', '', 'garbage'])( "mode '%s' does not skip save", async mode => { if (mode === '') { From a4b189127fd3d8b4e79cc97a5b743f168cb30f7b Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Mon, 6 Jul 2026 09:33:41 -0500 Subject: [PATCH 11/15] fix copilot pr feedback Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/cache/src/cache.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 0183a9a613..56795056dd 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -436,7 +436,7 @@ async function restoreCacheV2( // treat as a cache miss so the workflow continues. core.warning(`Failed to restore: ${typedError.message}`) } else { - // Supress all non-validation cache related errors because caching should be optional + // Suppress all non-validation cache related errors because caching should be optional // Log server errors (5xx) as errors, all other errors as warnings if ( typedError instanceof HttpClientError && From 77bc4baf825172c521319d8cd3e706f80317267e Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Fri, 10 Jul 2026 09:09:56 -0500 Subject: [PATCH 12/15] docs(cache): remove internal reference from cache-mode comment --- packages/cache/src/internal/config.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cache/src/internal/config.ts b/packages/cache/src/internal/config.ts index 68d433d474..70526475a4 100644 --- a/packages/cache/src/internal/config.ts +++ b/packages/cache/src/internal/config.ts @@ -19,8 +19,8 @@ export function getCacheServiceVersion(): string { return process.env['ACTIONS_CACHE_SERVICE_V2'] ? 'v2' : 'v1' } -// The cache-mode lattice (ADR c2c-actions#10264): readable = {read, write}, -// writable = {write, write-only}, none = neither. +// The cache-mode lattice: readable = {read, write}, writable = {write, +// write-only}, none = neither. const KNOWN_CACHE_MODES = ['none', 'read', 'write', 'write-only'] // The effective cache-mode exported by the runner, or '' when not set. From 93c70c4657e451311fb753918ce7ca61b492cf58 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Fri, 10 Jul 2026 15:34:22 -0500 Subject: [PATCH 13/15] test(cache): merge redundant cache-mode skip tests and simplify read-denied handling Address PR review feedback: - Merge the duplicate restore/save skip test.each blocks into single blocks parametrized over ACTIONS_CACHE_SERVICE_V2. - Drop the redundant CacheReadDeniedError catch arms; the typed error is not an HttpClientError so it already falls through to a non-fatal warning. - Clarify why read-denied classification happens both in getCacheEntry and cache.ts (dependency-free internal module cannot import the typed error). --- packages/cache/__tests__/restoreCache.test.ts | 23 ++++-------------- packages/cache/__tests__/saveCache.test.ts | 23 ++++-------------- packages/cache/src/cache.ts | 24 +++++++++---------- 3 files changed, 21 insertions(+), 49 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 64a0084464..290644a42a 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -163,24 +163,6 @@ describe('restore cache-mode gating', () => { restoreEnv('ACTIONS_CACHE_SERVICE_V2', originalV2) }) - test.each(['none', 'write-only'])( - "mode '%s' skips restore without touching the cache service", - async mode => { - process.env.ACTIONS_CACHE_MODE = mode - const logInfoMock = jest.spyOn(core, 'info') - const getCacheEntryMock = jest.spyOn(cacheHttpClient, 'getCacheEntry') - - const cacheKey = await restoreCache(['node_modules'], 'node-test') - - expect(cacheKey).toBe(undefined) - expect(getCacheEntryMock).not.toHaveBeenCalled() - expect(logInfoMock).toHaveBeenCalledTimes(1) - expect(logInfoMock).toHaveBeenCalledWith( - `Cache restore skipped: the effective cache-mode '${mode}' does not permit reads.` - ) - } - ) - // The skip short-circuits before v1/v2 dispatch, so it applies regardless of // the ACTIONS_CACHE_SERVICE_V2 feature flag. test.each([ @@ -193,12 +175,17 @@ describe('restore cache-mode gating', () => { async (mode, v2) => { process.env.ACTIONS_CACHE_MODE = mode restoreEnv('ACTIONS_CACHE_SERVICE_V2', v2) + const logInfoMock = jest.spyOn(core, 'info') const getCacheEntryMock = jest.spyOn(cacheHttpClient, 'getCacheEntry') const cacheKey = await restoreCache(['node_modules'], 'node-test') expect(cacheKey).toBe(undefined) expect(getCacheEntryMock).not.toHaveBeenCalled() + expect(logInfoMock).toHaveBeenCalledTimes(1) + expect(logInfoMock).toHaveBeenCalledWith( + `Cache restore skipped: the effective cache-mode '${mode}' does not permit reads.` + ) } ) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index b1a32f4577..89eaf085f2 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -74,24 +74,6 @@ describe('save cache-mode gating', () => { restoreEnv('ACTIONS_CACHE_SERVICE_V2', originalV2) }) - test.each(['read', 'none'])( - "mode '%s' skips save without touching the cache service", - async mode => { - process.env.ACTIONS_CACHE_MODE = mode - const logInfoMock = jest.spyOn(core, 'info') - const resolvePathsMock = jest.spyOn(cacheUtils, 'resolvePaths') - - const cacheId = await saveCache(['node_modules'], 'node-test') - - expect(cacheId).toBe(-1) - expect(resolvePathsMock).not.toHaveBeenCalled() - expect(logInfoMock).toHaveBeenCalledTimes(1) - expect(logInfoMock).toHaveBeenCalledWith( - `Cache save skipped: the effective cache-mode '${mode}' does not permit writes.` - ) - } - ) - // The skip short-circuits before v1/v2 dispatch, so it applies regardless of // the ACTIONS_CACHE_SERVICE_V2 feature flag. test.each([ @@ -104,12 +86,17 @@ describe('save cache-mode gating', () => { async (mode, v2) => { process.env.ACTIONS_CACHE_MODE = mode restoreEnv('ACTIONS_CACHE_SERVICE_V2', v2) + const logInfoMock = jest.spyOn(core, 'info') const resolvePathsMock = jest.spyOn(cacheUtils, 'resolvePaths') const cacheId = await saveCache(['node_modules'], 'node-test') expect(cacheId).toBe(-1) expect(resolvePathsMock).not.toHaveBeenCalled() + expect(logInfoMock).toHaveBeenCalledTimes(1) + expect(logInfoMock).toHaveBeenCalledWith( + `Cache save skipped: the effective cache-mode '${mode}' does not permit writes.` + ) } ) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 56795056dd..07999f51a4 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -231,10 +231,12 @@ async function restoreCacheV1( enableCrossOsArchive }) } catch (error) { - // The GHES v1 artifact cache service returns HTTP 403 with a + // The v1 artifact cache service returns HTTP 403 with a // `cache read denied:` body when the run's token has no readable cache - // scopes. getCacheEntry surfaces that body message, so re-classify it - // here to mirror the read-denied handling on the v2 path. + // scopes. getCacheEntry lives in a dependency-free internal module and + // cannot import CacheReadDeniedError without a circular dependency, so it + // only surfaces the raw denial message; we classify it into the typed + // error here so the outer catch and consumers can dispatch on it. const errorMessage = (error as Error)?.message ?? '' if (errorMessage.includes(CACHE_READ_DENIED_PREFIX)) { throw new CacheReadDeniedError(errorMessage) @@ -283,13 +285,11 @@ async function restoreCacheV1( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error - } else if (typedError.name === CacheReadDeniedError.name) { - // Read denied by policy (token has no readable cache scopes). Warn and - // treat as a cache miss so the workflow continues. - core.warning(`Failed to restore: ${typedError.message}`) } else { // warn on cache restore failure and continue build - // Log server errors (5xx) as errors, all other errors as warnings + // Log server errors (5xx) as errors, all other errors as warnings. + // A read denied by policy (CacheReadDeniedError) is not an HttpClientError + // so it falls here and is warned, treated as a cache miss. if ( typedError instanceof HttpClientError && typeof typedError.statusCode === 'number' && @@ -431,13 +431,11 @@ async function restoreCacheV2( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error - } else if (typedError.name === CacheReadDeniedError.name) { - // Read denied by policy (token has no readable cache scopes). Warn and - // treat as a cache miss so the workflow continues. - core.warning(`Failed to restore: ${typedError.message}`) } else { // Suppress all non-validation cache related errors because caching should be optional - // Log server errors (5xx) as errors, all other errors as warnings + // Log server errors (5xx) as errors, all other errors as warnings. + // A read denied by policy (CacheReadDeniedError) is not an HttpClientError + // so it falls here and is warned, treated as a cache miss. if ( typedError instanceof HttpClientError && typeof typedError.statusCode === 'number' && From 3e6533e02070f9901762a2fce3aa0001f2d8f02e Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Fri, 10 Jul 2026 15:34:32 -0500 Subject: [PATCH 14/15] refactor(cache): drop redundant CacheWriteDeniedError catch arms Mirror the read-denied simplification on the save path. CacheWriteDeniedError is not an HttpClientError and its name does not match the ReserveCacheError arm, so it falls through to the same non-fatal warning. Logging behavior is unchanged (warns, never fails the run) and the exported type is still thrown internally for consumers and tests. Also refresh stale doc wording. --- packages/cache/src/cache.ts | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 07999f51a4..af8eabb6b5 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -39,12 +39,13 @@ export class ReserveCacheError extends Error { } /** - * Stable prefix the receiver writes into the cache reservation response when - * the issuer downgraded the cache token to read-only (for example, because + * Stable prefix the cache service writes into the cache reservation response + * when the issuer downgraded the cache token to read-only (for example, because * the run was triggered by an untrusted event). saveCacheV1 / saveCacheV2 - * dispatch on this prefix to re-classify the failure as a - * CacheWriteDeniedError so consumers (and the outer catch arm) can - * distinguish a policy denial from other reservation failures. + * dispatch on this prefix to re-classify the failure as a CacheWriteDeniedError + * so consumers and tests can distinguish a policy denial from other reservation + * failures. Internally it is logged as a non-fatal warning like other + * best-effort save failures. */ export const CACHE_WRITE_DENIED_PREFIX = 'cache write denied:' @@ -52,7 +53,7 @@ export const CACHE_WRITE_DENIED_PREFIX = 'cache write denied:' * Raised when the cache backend refuses to reserve a writable cache entry * because the JWT issued for this run was scoped read-only (for example, the * run was triggered by an event the repository administrator classified as - * untrusted). The receiver-supplied detail message always begins with + * untrusted). The service-supplied detail message always begins with * `cache write denied:` (the full error message includes additional context * like the cache key). * @@ -597,15 +598,13 @@ async function saveCacheV1( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error - } else if (typedError.name === CacheWriteDeniedError.name) { - // Cache write was denied by policy (read-only token). Surface to the - // customer at warning level so it is visible in the workflow log - // without failing the run. - core.warning(`Failed to save: ${typedError.message}`) } else if (typedError.name === ReserveCacheError.name) { core.info(`Failed to save: ${typedError.message}`) } else { - // Log server errors (5xx) as errors, all other errors as warnings + // Log server errors (5xx) as errors, all other errors as warnings. + // A write denied by policy (CacheWriteDeniedError) is not an + // HttpClientError and its name does not match the ReserveCacheError arm, + // so it falls here and is warned without failing the run. if ( typedError instanceof HttpClientError && typeof typedError.statusCode === 'number' && @@ -759,17 +758,15 @@ async function saveCacheV2( const typedError = error as Error if (typedError.name === ValidationError.name) { throw error - } else if (typedError.name === CacheWriteDeniedError.name) { - // Cache write was denied by policy (read-only token). Surface to the - // customer at warning level so it is visible in the workflow log - // without failing the run. - core.warning(`Failed to save: ${typedError.message}`) } else if (typedError.name === ReserveCacheError.name) { core.info(`Failed to save: ${typedError.message}`) } else if (typedError.name === FinalizeCacheError.name) { core.warning(typedError.message) } else { - // Log server errors (5xx) as errors, all other errors as warnings + // Log server errors (5xx) as errors, all other errors as warnings. + // A write denied by policy (CacheWriteDeniedError) is not an + // HttpClientError and its name does not match the ReserveCacheError arm, + // so it falls here and is warned without failing the run. if ( typedError instanceof HttpClientError && typeof typedError.statusCode === 'number' && From 21f71d417e60316bc11003c1bdace3f3ad516a12 Mon Sep 17 00:00:00 2001 From: Philip Gai Date: Fri, 10 Jul 2026 15:46:00 -0500 Subject: [PATCH 15/15] test(cache): collapse redundant restore getCacheEntry-failure tests The two restoreCache tests exercised the identical warning + cache-miss path now that read-denied is no longer reclassified in the catch, so merge them into one. The read-denied prefix detection that actually branches on the message is covered by getCacheEntry tests in cacheHttpClient.test.ts. --- .../cache/__tests__/cacheHttpClient.test.ts | 2 +- packages/cache/__tests__/restoreCache.test.ts | 40 ++++--------------- 2 files changed, 8 insertions(+), 34 deletions(-) diff --git a/packages/cache/__tests__/cacheHttpClient.test.ts b/packages/cache/__tests__/cacheHttpClient.test.ts index 5cc129f78b..397461f33d 100644 --- a/packages/cache/__tests__/cacheHttpClient.test.ts +++ b/packages/cache/__tests__/cacheHttpClient.test.ts @@ -60,7 +60,7 @@ test('getCacheVersion with enableCrossOsArchive as false returns version on wind }) test('getCacheEntry throws a generic status-code error for non-read-denied failures', async () => { - // Regression: a non read-denied failure must NOT leak the receiver's body + // Regression: a non read-denied failure must NOT leak the server's body // message; it should surface the generic status-code error. jest.spyOn(requestUtils, 'retryTypedResponse').mockResolvedValue({ statusCode: 403, diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 290644a42a..cfff4738f6 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -99,50 +99,24 @@ test('restore with server error should fail', async () => { delete process.env['ACTIONS_RESULTS_URL'] }) -test('restore denied by read-only token logs warning and reports cache miss', async () => { - // The GHES v1 artifact cache service returns HTTP 403 with a - // `cache read denied:` body when the token has no readable scopes. - // getCacheEntry surfaces that message; expect a single warning (not error) - // and a cache miss. +test('restore surfaces a getCacheEntry failure as a warning and reports a cache miss', async () => { + // restoreCache treats any getCacheEntry failure (read-denied or otherwise) + // as a non-fatal warning and a cache miss so the workflow continues. The + // read-denied prefix detection itself is covered in cacheHttpClient.test.ts. const paths = ['node_modules'] const key = 'node-test' const logErrorMock = jest.spyOn(core, 'error') const logWarningMock = jest.spyOn(core, 'warning') - const deniedMessage = 'cache read denied: token has no readable scopes' + const message = 'cache read denied: token has no readable scopes' jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(async () => { - throw new Error(deniedMessage) + throw new Error(message) }) const cacheKey = await restoreCache(paths, key) expect(cacheKey).toBe(undefined) expect(logErrorMock).not.toHaveBeenCalled() - expect(logWarningMock).toHaveBeenCalledWith( - `Failed to restore: ${deniedMessage}` - ) - expect(logWarningMock).toHaveBeenCalledTimes(1) -}) - -test('restore surfaces a non-read-denied getCacheEntry error as a normal warning', async () => { - // Guards the inner catch in restoreCacheV1: only `cache read denied:` errors - // are re-classified; every other getCacheEntry failure must pass through and - // be logged normally (not swallowed or mislabeled as a read denial). - const paths = ['node_modules'] - const key = 'node-test' - const logErrorMock = jest.spyOn(core, 'error') - const logWarningMock = jest.spyOn(core, 'warning') - const genericMessage = 'Cache service responded with 400' - - jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(async () => { - throw new Error(genericMessage) - }) - - const cacheKey = await restoreCache(paths, key) - expect(cacheKey).toBe(undefined) - expect(logErrorMock).not.toHaveBeenCalled() - expect(logWarningMock).toHaveBeenCalledWith( - `Failed to restore: ${genericMessage}` - ) + expect(logWarningMock).toHaveBeenCalledWith(`Failed to restore: ${message}`) expect(logWarningMock).toHaveBeenCalledTimes(1) })