feat: support restore keys #13
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Action
participant S3 as S3
participant FS as Filesystem
participant State as Workflow State
Runner->>S3: HEAD exact cache key
alt Exact found
S3-->>Runner: Object metadata
Runner->>S3: GET object (stream)
S3-->>Runner: Stream bytes
Runner->>FS: Write file → Extract → Cleanup
Runner->>State: set exactMatch = true
else Exact not found
Runner->>S3: For each restore-key prefix → LIST objects
S3-->>Runner: Object lists
Runner->>Runner: Filter *.tar.zst, sort by lastModified, choose newest (foundKey)
alt Candidate found
Runner->>S3: GET foundKey (stream)
S3-->>Runner: Stream bytes
Runner->>FS: Write file → Extract → Cleanup
Runner->>State: set cache-upload-needed = true, exactMatch = false
else No candidate
Runner->>State: no-cache
end
end
Runner->>Runner: Continue (may skip install/build if exactMatch and skip enabled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds support for restore keys to the S3 cache action, allowing fallback cache keys when an exact cache key match is not found. This feature mirrors the behavior of GitHub's official cache action, enabling more flexible cache restoration strategies where partial or prefix-based cache matches can be used when the exact key doesn't exist.
Key Changes:
- Added
restore-keysinput parameter to allow specification of fallback cache key prefixes - Implemented fallback logic that searches for matching cache files by prefix when exact key is not found
- Refactored cache restoration flow to differentiate between exact matches and fuzzy matches
- Changed command execution logic and early return behavior for exact cache hits
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| action.yml | Adds new optional restore-keys input parameter for fallback cache key prefixes |
| src/index.js | Implements restore keys logic with S3 listObjectsV2 prefix search, refactors cache download flow, and updates command execution logic |
| dist/index.js | Compiled/bundled version of src/index.js changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/index.js (1)
44-46: Consider distinguishing "not found" from other S3 errors.The catch block swallows all
headObjecterrors. Access denied, network failures, or other S3 errors would be silently ignored, potentially leading to confusing behavior where existing caches are never found.} catch (headErr) { - // Not found exact match + // Only treat "not found" as expected; rethrow other errors + if (headErr.code !== 'NotFound' && headErr.statusCode !== 404) { + throw headErr; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
action.yml(1 hunks)src/index.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
Be concise and precise. Skip minor issues and focus on critical problems and potentially wrong code. Avoid raising multiple comments with similar or repetitive content and instead use one comment in the selected key place to clearly state common concerns.
Files:
action.ymlsrc/index.js
🧬 Code graph analysis (1)
src/index.js (1)
src/post.js (7)
core(1-1)s3(6-6)fileName(11-11)s3Bucket(10-10)exec(4-4)fileStream(26-26)fs(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
action.yml (1)
9-10: LGTM!The new
restore-keysinput is properly defined as optional and follows the existing pattern for other inputs.src/index.js (2)
104-108: LGTM!The logic correctly triggers cache upload for fuzzy matches, ensuring the exact key gets cached for future runs.
54-58: Handle pagination when listing objects withlistObjectsV2.The code calls
listObjectsV2without settingMaxKeys, which defaults to 1000 objects per request. If a restore-key prefix matches more than 1000.tar.zstfiles, the truly most recent cache will not be found since pagination is not handled. Check theIsTruncatedflag and useContinuationTokento page through all results when needed, or explicitly setMaxKeysif a smaller page size is intentional.
xahhy
force-pushed
the
feat/support-restore-keys
branch
from
6c618a4 to
3528659
Compare
xahhy
force-pushed
the
feat/support-restore-keys
branch
from
3528659 to
b674be9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/index.js (2)
14-14: S3listObjectsV2pagination not handled for restore-keys listing (may pick wrong “latest”).
If a prefix has >1000 objects, the most recent.tar.zstmight not be in the first page.Also applies to: 48-67
76-80: Fixcache-hit-skipflow: don’t return before restore; and don’t skip command on fuzzy restores (unless intended).
Currently you return on(exactMatch && cacheHitSkip)before download/extract (Lines 76-80), and laterif (cacheHitSkip) return;skips the command even for fuzzy matches (Lines 104-107), preventing the “refresh + upload” path from running.- // Skip command to save time - if (exactMatch && cacheHitSkip) { - console.log(`Cache found, skipping command: ${command}`); - return; - } - // Cache found. Download and extract console.log(`Found a cache for key: ${foundKey}`); @@ await exec.exec(`tar ${untarOption} ${fileName}`); await exec.exec(`rm -f ${fileName}`); console.log(exactMatch ? `Restored from cache-key: ${foundKey}` : `Restored from restore-key: ${foundKey}`); - if (cacheHitSkip) { - console.log(`Cache restored and cache-hit-skip is set. Skipping command: ${command}`); - return; - } + // Skip command only on exact hits + if (exactMatch && cacheHitSkip) { + console.log(`Cache restored (exact hit) and cache-hit-skip is set. Skipping command: ${command}`); + return; + } // When fuzzy matched cache, will need to upload latest files in post step. // Require workflow to run command to update the cache files otherwise uploaded cache will not match - if (!exactMatch) { - await exec.exec(command); - core.saveState('cache-upload', true); - } + await exec.exec(command); + if (!exactMatch) core.saveState('cache-upload', true);Also applies to: 104-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dist/index.jsis excluded by!**/dist/**dist/post/index.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
action.yml(1 hunks)package.json(1 hunks)src/index.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- action.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
Be concise and precise. Skip minor issues and focus on critical problems and potentially wrong code. Avoid raising multiple comments with similar or repetitive content and instead use one comment in the selected key place to clearly state common concerns.
Files:
src/index.jspackage.json
🧬 Code graph analysis (1)
src/index.js (2)
dist/post/index.js (9)
core(76434-76434)s3(76439-76439)exec(819-819)exec(4395-4395)exec(76437-76437)fs(570-570)fs(2023-2023)fs(4191-4191)fs(76436-76436)dist/index.js (10)
core(77726-77726)s3(77732-77732)exec(819-819)exec(4395-4395)exec(77727-77727)fs(570-570)fs(2023-2023)fs(4191-4191)fs(77729-77729)downloader(77733-77733)
| "dependencies": { | ||
| "@actions/core": "^1.11.1", | ||
| "@actions/core": "^2.0.1", | ||
| "@actions/exec": "^1.1.1", | ||
| "@zeit/ncc": "^0.22.1", | ||
| "aws-sdk": "^2.654.0", | ||
| "aws-sdk": "^2.1693.0", | ||
| "graceful-fs": "^4.2.3", | ||
| "s3-download": "^1.0.0" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@actions/core 2.0.1 breaking changes Node 20 compatibility
💡 Result:
Short answer: There is no published @actions/[email protected] on npm — the actions/toolkit @actions/core package latest published version is 1.11.1 — so there are no official “@actions/core 2.0.1” breaking-change notes to reference. [1][2]
Notes and pointers:
- npm package page for @actions/core (shows current version 1.11.1). [1]
- GitHub Actions toolkit (actions/toolkit) repo / packages info — use this repo for official changes to @actions/core. [2]
- If you’re worried about Node.js runtime changes: GitHub Actions now supports running JavaScript actions on node20 (and is moving runners toward node24); check GitHub’s docs/changelog and ensure your action metadata uses runs.using: node20 (or node24) and that your action bundle (and its @types/node / compiled target) is compatible. [3][4]
If you want, I can:
- search the specific repo that bumped to “@actions/core v2” (some third-party forks/packs exist) and show their breaking changes; or
- check a specific action/release that mentions depending on @actions/core v2.0.1 and list its breaking changes. Which would you prefer?
🌐 Web query:
aws-sdk v2.1693.0 release notes changelog
💡 Result:
Changelog entry for aws-sdk v2.1693.0:
- 2.1693.0 — Support: Update docs to indicate JS SDK v2 is end-of-support. (raw.githubusercontent.com)
Citations:
🏁 Script executed:
# Check for lockfile in the repository
ls -la | grep -E "package-lock.json|yarn.lock|pnpm-lock.yaml"Repository: getjerry/actions-s3-cache
Length of output: 136
🏁 Script executed:
# Check the package.json to see full context
cat -n package.jsonRepository: getjerry/actions-s3-cache
Length of output: 671
🏁 Script executed:
# Check what version of @actions/core is actually in the lockfile
grep -A 5 '"@actions/core"' package-lock.json | head -20Repository: getjerry/actions-s3-cache
Length of output: 267
🏁 Script executed:
# Also check @actions/exec version in lockfile
grep -A 5 '"@actions/exec"' package-lock.json | head -20Repository: getjerry/actions-s3-cache
Length of output: 424
@actions/[email protected] does not exist on npm — this will cause installation failures.
The declared version @actions/[email protected] has no published release on npm (current latest is 1.11.1). Change this to a valid version, likely @actions/core@^1.11.1 or another existing release, or confirm if a private/forked version is intended. This is blocking proper dependency resolution.
Additionally, [email protected] is end-of-support; consider migrating to AWS SDK v3 or document why v2 is pinned for this action.
🤖 Prompt for AI Agents
In package.json around lines 7 to 14, the dependency entry "@actions/core":
"^2.0.1" is invalid (no such release) and will break installs; change it to a
valid published version such as "@actions/core": "^1.11.1" (or the exact
intended release) and run npm install to verify resolution, and for
[email protected] note that v2 is end-of-support so either migrate to AWS SDK v3
(preferred) or add a comment/README note explaining why v2 is intentionally
pinned and ensure any security/maintenance implications are documented.
| await exec.exec(`tar ${untarOption} ${fileName}`); | ||
| await exec.exec(`rm -f ${fileName}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/index.js | sed -n '95,110p'Repository: getjerry/actions-s3-cache
Length of output: 841
🏁 Script executed:
cat -n src/index.js | head -120 | tail -50Repository: getjerry/actions-s3-cache
Length of output: 1958
🏁 Script executed:
cat -n src/index.js | head -70Repository: getjerry/actions-s3-cache
Length of output: 2966
🏁 Script executed:
npm list @actions/exec 2>/dev/null || echo "Checking package.json"Repository: getjerry/actions-s3-cache
Length of output: 140
🏁 Script executed:
cat package.jsonRepository: getjerry/actions-s3-cache
Length of output: 524
🌐 Web query:
@actions/exec API documentation version 1.1.1 exec function arguments array
💡 Result:
The exec function accepts an arguments array as its second parameter: a string[] of individual arguments (no shell splitting). Usage example:
await exec.exec('node', ['index.js', 'foo=bar'], options)
Sources: [1] npm @actions/exec (v1.1.1) README. [2] npm.io package page.
🏁 Script executed:
# Verify graceful-fs supports fs.promises
grep -r "fs.promises" . --include="*.js" | head -5 || echo "Checking Node.js compatibility"Repository: getjerry/actions-s3-cache
Length of output: 641
Avoid shell-string tar/rm execution (command injection vulnerability).
untarOption is user-controlled input from GitHub Actions and is interpolated directly into a shell command at line 100, allowing command injection. Use exec.exec('tar', args) with properly split arguments and fs.promises.unlink instead of rm.
- await exec.exec(`tar ${untarOption} ${fileName}`);
- await exec.exec(`rm -f ${fileName}`);
+ const untarArgs = (untarOption || '').split(/\s+/).filter(Boolean);
+ await exec.exec('tar', [...untarArgs, fileName]);
+ await fs.promises.unlink(fileName).catch(() => {});🤖 Prompt for AI Agents
In src/index.js around lines 100-101, the current code interpolates
user-controlled untarOption into a shell string for tar and uses rm via shell,
causing command-injection risk; change to call exec.exec('tar', argsArray) where
argsArray is a safe array (validate or whitelist allowed flags/options then push
fileName as a separate element) so no shell interpolation occurs, and replace
the rm -f call with await fs.promises.unlink(fileName) (wrapped in try/catch to
handle missing files) to delete the file safely.
Summary by CodeRabbit
New Features
Improvements
Defaults
Chores
✏️ Tip: You can customize this high-level summary in your review settings.