[licjun]

Summary

The native installer path cached the extracted install directory and added it to PATH on a cache hit without checking it against anything. GitHub's own caching guidance notes that cache contents are not signed or verified, and that restored content should not be trusted implicitly. This change makes the action verify what it restores.

What changed

Cache the release archive instead of the extracted directory. An extracted directory cannot be checked against a published digest; the archive can. The archive is extracted to a temporary directory after it passes verification.

Verify against the digest published on the GitHub release. installUsingNativeInstaller now fetches the release metadata (for a pinned version via releases/tags/v{version}, which it previously did not need) and reads the digest field of the relevant asset. On a cache hit the restored archive is hashed and compared; on mismatch the entry is discarded and the official archive is downloaded instead. Freshly downloaded archives are verified too, and a failure there fails the run rather than proceeding.

Bypass the cache when there is nothing to verify against. GitHub only populates the asset digest field for releases published after roughly mid-2025. For older versions, and when the release metadata request fails, the action downloads the archive on every run and does not write to the cache. Caching only what can be verified keeps the behaviour fail-closed rather than falling back to unverified content.

Cache key prefix is now sam-cli-verified. Entries written under the previous layout hold an extracted directory rather than an archive, so they must not be restored by this version.

New cache-hit output. The cache path previously had no observable signal, so no test could assert whether it was taken. The output makes that assertable and gives consumers visibility into whether the install came from the cache.

Behaviour change for consumers

Pinning a version published before the digest field existed (for example 1.104.0) now downloads on every run instead of using the cache. Functionally identical, slower on repeat runs. Versions from roughly 1.140.0 onward are unaffected and still benefit from caching. No input changes; nothing consumers need to update.

Testing

Unit tests cover digest match, digest mismatch, absent digest, release metadata lookup failure, and post-download verification failure. They use real temporary files and real SHA256 computation rather than stubbing the hash.

Two integration cases were added to test.yml. One pins a version with a published digest and runs the action twice, asserting the second run reports cache-hit == true — this is the only coverage of the verified cache-hit path against the real cache service, since the unit tests mock @actions/cache. The other asserts that a version with no published digest does not use the cache. Existing pinned versions in test.yml were left unchanged so the no-digest path keeps its coverage; the step previously named "should use cache" was renamed, since with this change that version no longer does.

npm run all is green.

Note for maintainers

release.yml rolls only the v3 tag, so v2 will not pick this up. Worth deciding separately whether v2 should receive the same change or be formally wound down.

[bnusunny]

Reviewed at 9157adc. The approach is sound: the expected digest always comes from the release API, never from the cache, and nothing unverified is ever written to the cache. npm test is green (29 tests), and a fresh ncc build reproduces the committed dist/index.js exactly.

I also mutation-checked the guards. Skipping verification on cache hit or after download, forcing the hash comparison true, caching no-digest versions, reverting the key prefix, and downgrading the download-mismatch setFailed to a warning all turn tests red. Two suggested fixes and a few nits:

1. A tampered entry is not actually discarded (lib/setup.js:465)

Cache entries are immutable. On mismatch the code re-downloads and calls saveCache with the same key, which @actions/cache rejects with ReserveCacheError and logs only at info level. So the bad entry stays. Every run restores it, fails verification, downloads, and fails to save. Since it is read on every run, it never ages out either. This stays fail-safe, but the "Discarding the cache entry" warning is untrue and the slow path is permanent. Suggest rewording it to say the entry is being ignored, and pointing at gh cache delete <key> or the Actions cache UI to clear it.

2. Pinned versions now make an unauthenticated API call on every run

Before this change, a pinned version that hit the cache made no API call. Now every run fetches releases/tags/v{version}, and token has no default. Unauthenticated requests are limited to 60/hour per IP. When that runs out, getReleaseByTag returns null and the run falls back to an uncached download. That slowdown is separate from the pre-digest-version one described in the PR. Suggest defaulting the input the way the first-party setup actions do:

token:
  default: ${{ github.server_url == 'https://github.com' && github.token || '' }}

3. Nit: tests don't pin which asset's digest is selected

releaseBody() builds a release with a single asset. So changing the match to a.name.endsWith(".zip") still passes, and so does accepting a digest that isn't sha256:-shaped. Real releases carry both the x86_64 and arm64 zips. Putting both in the fixture, with different digests, would catch the wrong one being chosen. The arm64 CI job shows the code is correct today; this only protects it going forward.

4. Nit: ImageOS in the cache key (lib/setup.js:446)

This made sense when an extracted install was cached. The archive is the same file on every image, so the key now splits the cache for no benefit. Fine to leave.

5. Pre-existing, not introduced here

• Any exception in downloadVerifyAndCache / downloadWithoutCache returns "" with only a warning, so the step succeeds with no sam on PATH.
• core.warning("...", error) at lib/setup.js:192 passes the error as the annotation-properties argument, so its message is dropped.

Agree that v2 should be decided separately.

[licjun]

Thanks for the thorough review — particularly the mutation checks, which caught two guards that had no real coverage.

1. Tampered entry is not discarded — fixed

You're right, and the wording was the smaller half of it. Entries are immutable, so re-saving under the same key is rejected; and because the entry is restored on every run it never ages out either, so the mismatch recurs indefinitely with no self-healing path. The warning now says the cached copy is being ignored, states that the entry cannot be overwritten, and prints gh cache delete '<key>'. The save-failure message explains the same constraint instead of reporting a generic failure.

I did not make the action delete the entry itself: that needs actions: write, which a setup-* action cannot assume its consumers grant, and deleting repository resources is beyond what this action should be doing. Reporting it accurately and leaving the decision to the operator seems like the right boundary — happy to revisit if you disagree.

A test now asserts the warning contains the gh cache delete command and does not claim to discard anything.

2. Unauthenticated API call on every run — fixed

Agreed, and this one is a regression I introduced: before this change a pinned version that hit the cache made no API call at all. Adopted your suggested default verbatim, which matches setup-node / setup-python / setup-go / setup-java. I confirmed the expression evaluates in a JS action's default — all four of those are using: node24 and use the identical expression.

Worth noting the failure mode this closes is distinct from the one in the PR description: that one is a version with no published digest, which is deterministic. This one is a version that has a digest we simply couldn't fetch, which is intermittent and depends on unrelated jobs sharing the runner IP — harder to diagnose. README updated, and I dropped the explicit token from the example and the new test steps so they exercise the default.

3. Fixture doesn't pin asset selection — fixed

releaseBody() now lists both architecture archives plus signatures and the macOS/Windows installers, gives every asset other than the one under test a decoy digest, and deliberately orders a decoy first so selecting by position fails too. Added cases for digests that are unprefixed, a different algorithm, truncated, non-hex, and empty.

Confirmed both gaps now close: matching assets by suffix fails 11 tests, and dropping the sha256: format check fails 4. Both passed before.

4. ImageOS in the cache key — leaving it, deliberately

Your reasoning is correct: the archive is byte-identical across images, so the key fragments the cache for no benefit, and two Ubuntu jobs on the same version will each store their own ~95 MB copy against the repo's cache quota.

Leaving it anyway, because it doesn't affect correctness and changing the key again would mean re-validating the cache paths for what is purely an optimisation. Flagging it as worth a follow-up rather than folding it in here.

5. Pre-existing issues — agreed, separately

Both are real and worth fixing, and I'd rather not fold them into this change so it stays reviewable as one thing:

• The silent-success path is worse than it looks on GitHub-hosted runners: the Ubuntu images preinstall AWS SAM CLI (1.166.2 on both 22.04 and 24.04), so when the download throws, the step succeeds and later sam calls silently resolve to the image's copy — at a version that may not match the pin, with nothing in the log drawing attention to it.
• core.warning("...", error) at lib/setup.js:192 passes the Error as the annotation-properties argument, so its message is dropped.

Happy to take both in a follow-up PR.

npm run all is green at 35 tests.

[bnusunny]

Re-reviewed at 2102106. Thanks for the quick turnaround. npm test passes (35 tests), and a fresh ncc build matches the committed dist/index.js.

I re-ran the mutation checks. Both earlier survivors now die: matching assets by suffix fails 12 tests, and dropping the sha256: format check fails 4. The original guards still die too, and so does removing the gh cache delete hint from the warning.

Items 2–5 look good to me. That covers the token default (including not sending it off-instance on GHES), the fixture, leaving ImageOS for a follow-up, and splitting out the pre-existing issues.

One thing left on item 1: the save path still reports success when the save is rejected (lib/setup.js:326-336)

This one is partly on me. I wrote that @actions/cache "rejects" the re-save with ReserveCacheError, which reads as if it throws. It doesn't. In 4.1.0 (the installed version), saveCache catches ReserveCacheError itself, logs Failed to save: … at info level, and returns -1 (node_modules/@actions/cache/lib/cache.js:489-490,516). The same applies to every error except ValidationError.

So in exactly the tampered-entry case:

• the new comment and warning in the catch block never run;
• the return value is ignored, and the next line logs Cached AWS SAM CLI <version> to GitHub Actions cache.

The test for this case mocks saveCache with mockRejectedValue(...) (test/setup.test.js:400-406), which the real library never does. That is why the test passes.

Suggested fix:

const cacheId = await cache.saveCache([archivePath], cacheKey);
if (cacheId === -1) {
  core.warning(`AWS SAM CLI ${version} was not saved to the GitHub Actions cache ...`);
} else {
  core.info(`Cached AWS SAM CLI ${version} to GitHub Actions cache`);
}

Then change the mock to mockResolvedValue(-1). Keep the catch for ValidationError.

Nit: the test title at test/setup.test.js:241 still says "discarded".

[licjun]

Fixed in 5c9eaa3.

Save path now checks the return value. saveCache resolving -1 is treated as "not saved", and the success message only appears when it isn't -1. The catch is kept for ValidationError.

I logged -1 at info rather than as a warning, and without a gh cache delete hint. -1 covers several unrelated causes — another job writing the same key, a read-only cache, a service error — so blaming an existing entry would often be wrong, and advising a delete would be wrong for a concurrent legitimate write. saveCache has also already logged the reason at a level suited to the error (info for a reservation conflict, warning for finalize, error for 5xx), so repeating it as a warning would just add noise. Happy to raise it to a warning if you'd prefer.

Went one step further on the rejected-entry path: when a restored entry fails verification, the key is already known to be taken, so the save is now skipped entirely. Since saveCache archives before it reserves, attempting it would cost ~95 MB of compression on every run only to be declined. The delete hint now appears once, in the verification warning.

Tests now mock saveCache as it actually behaves: resolving -1 for a declined save, and rejecting only with ValidationError. The unrealistic mockRejectedValue case is gone, and the test title no longer says "discarded". Mutation-checked: saving after a rejected entry, ignoring -1, putting the delete hint back on the -1 path, and swallowing ValidationError each fail a test.

npm run all is green at 37 tests.

[bnusunny]

Re-reviewed at 5c9eaa3. The save path now keys off saveCache's return value, ValidationError is still caught, and the mocks match the library's real behaviour. npm test is green (37), dist matches a fresh ncc build, and mutation checks on the new logic (ignoring -1, saving after a rejected entry, never saving, rethrowing ValidationError) all fail a test. LGTM, thanks.