Fix two crashes the first live run exposed - #17
Merged
Conversation
`requireEnv` validates a list and returns nothing; two reports called it as a getter and assigned its result, so the key stability check died before reading a single key and the cache value report would have died after collecting all of its samples. `@actions/cache` publishes an `exports` map with no "." entry, so requiring it by package name throws ERR_PACKAGE_PATH_NOT_EXPORTED and every cache save slot failed. Resolve its manifest and require what `main` points at instead, which goes around the map and keeps working across the versions different setup-java refs pin. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes two runtime failures encountered in the first live run of the rescoped benchmarks by aligning requireEnv usage with its actual contract and by making @actions/cache loadable despite its restrictive exports map.
Changes:
- Update
check-cache-keys.mjsandreport-cache-value.mjsto callrequireEnv(env, [...])for validation and then read values fromenv. - Update
cache-save.shto load@actions/cacheby resolving and requiring itsmainentry via the package manifest, avoidingERR_PACKAGE_PATH_NOT_EXPORTED.
Show a summary per file
| File | Description |
|---|---|
| scripts/report-cache-value.mjs | Switches from treating requireEnv as a getter to using it for validation and reading env vars directly. |
| scripts/check-cache-keys.mjs | Same requireEnv contract fix for cache key stability metadata inputs. |
| scripts/cache-save.sh | Adds a manifest-based loader for @actions/cache to avoid failures caused by the package exports map. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
scripts/cache-save.sh:286
loadCacheClient()redeclares themanifestparameter as aconst manifest, which is a syntax error and will cause the cache-save Node snippet to fail at parse time.
const manifest = JSON.parse(readFileSync(manifestPath, "utf8"));
return require(join(dirname(manifestPath), manifest.main ?? "lib/cache.js"));
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+239
to
+240
| const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); | ||
| return require(join(dirname(manifestPath), manifest.main ?? "lib/cache.js")); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first live run of the rescoped benchmarks (after #15 merged) surfaced two defects that unit tests could not catch.
requireEnvused as a getter. It validates a list of names and returns nothing.check-cache-keys.mjsandreport-cache-value.mjsboth called it asrequireEnv(env, "NAME")and assigned the result, so it received a string where it expected an array and threwnames.filter is not a function. The key stability check died before reading a single key — all four probe jobs had succeeded — and the cache value report would have died after collecting every sample.@actions/cachecannot be required by name. It publishes anexportsmap with no"."entry, sorequire("@actions/cache")throwsERR_PACKAGE_PATH_NOT_EXPORTED. Every cache save slot failed on it. Resolving the package manifest and requiring what itsmainpoints at goes around the map, and keeps working across the different toolkit versions each setup-java ref pins — which is the whole reason that workflow installs each ref's own dependencies. Verified locally against@actions/cache@4.Run evidence: key stability 30983228434, cache save 30983247712.
Action overhead, Transfer overlap and JDK cache all passed on that same run.