GitHub Action to cache a directory via rsync#7
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new ChangesCache via rsync action
Sequence Diagram(s)sequenceDiagram
participant restore.js
participant commit.js
participant Rsync
participant exec.exec
participant core
restore.js->>Rsync: pull()
Rsync->>exec.exec: rsync remote cache -> local path
exec.exec-->>Rsync: exit code
Rsync-->>restore.js: exit code
restore.js->>core: setFailed(error.message) on error
commit.js->>Rsync: push()
Rsync->>exec.exec: rsync local path -> remote cache
exec.exec-->>Rsync: exit code
Rsync-->>commit.js: exit code
commit.js->>core: setFailed(error.message) on error
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
actions/rsync-cache/src/rsync.js (1)
71-79: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrivate key file is not cleaned up if rsync invocation throws.
exec.execruns withignoreReturnCode: true, so a non-zero exit won't throw, but any thrown error (e.g.,execitself failing) leaves the private key atthis.ssh_key_fileon disk. Wrap the run/unlink intry/finallyto guarantee removal.🛡️ Proposed fix
- // Run rsync. - const options = {ignoreReturnCode: true}; - const exit_code = await exec.exec(this.command, [...this.args, ...additional_arguments], options); - - // Remove the key file. - await fs.unlink(this.ssh_key_file); - - // Let callers decide what to do with the exit code. - return exit_code; + // Run rsync. + const options = {ignoreReturnCode: true}; + try { + return await exec.exec(this.command, [...this.args, ...additional_arguments], options); + } finally { + // Always remove the key file. + await fs.unlink(this.ssh_key_file); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@actions/rsync-cache/src/rsync.js` around lines 71 - 79, The rsync command path in rsync.js does not guarantee cleanup of the temporary SSH key when exec.exec throws. Update the run/unlink flow in the rsync method to use a try/finally around the exec.exec call so that fs.unlink(this.ssh_key_file) always runs, even if this.command or the exec invocation fails; keep the existing ignoreReturnCode behavior and preserve the current key file creation/chmod logic.flake.nix (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the action's runtime tools to the dev shell.
nix developcurrently gives you Node and shell linters, but this action's core path shells out throughrsyncand SSH. Including those binaries here would make local smoke-testing much closer to the real runtime.Suggested diff
devShells.default = pkgs.mkShell { nativeBuildInputs = with pkgs; [ nodejs_24 + rsync + openssh shellcheck shfmt ]; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flake.nix` around lines 19 - 24, The dev shell in the mkShell setup is missing the runtime binaries used by the action’s main path, so local testing does not match production behavior. Update the nativeBuildInputs for devShells.default to include the tools used by the action’s rsync and SSH execution path, alongside nodejs_24, shellcheck, and shfmt, so nix develop provides everything needed for realistic smoke tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@actions/rsync-cache/src/rsync.js`:
- Around line 20-24: The rsync target is missing the configured SSH username, so
`ssh_user` is read in the `RsyncCache` constructor but never applied. Update the
SSH/rsync command assembly in `RsyncCache` (including the `ssh_dir` target and
the command built later in the class) to prefix the remote host with
`this.ssh_user` so connections authenticate against the intended account instead
of the local runner user.
- Around line 76-82: `run()` in `RsyncCache` has two breakages: it is passing
merged arguments incorrectly and returning an undefined exit code. Update the
`exec.exec` call to combine `this.args` with `additional_arguments` as an array
so `this.command` receives valid args, and fix the exit-code variable naming so
the value assigned from `exec.exec` is the same symbol returned later
(`exit_code` instead of the misspelled temporary). Ensure the method still
removes `this.ssh_key_file` before returning.
In `@README.md`:
- Line 28: The ssh_port example has invalid YAML because it includes an extra
PORT: prefix in the value assignment. Update the README example so ssh_port is
defined as a normal key-value pair using the ARCHIVE_RRSYNC_PORT secret, and
remove the extraneous prefix to keep the YAML well-formed.
---
Nitpick comments:
In `@actions/rsync-cache/src/rsync.js`:
- Around line 71-79: The rsync command path in rsync.js does not guarantee
cleanup of the temporary SSH key when exec.exec throws. Update the run/unlink
flow in the rsync method to use a try/finally around the exec.exec call so that
fs.unlink(this.ssh_key_file) always runs, even if this.command or the exec
invocation fails; keep the existing ignoreReturnCode behavior and preserve the
current key file creation/chmod logic.
In `@flake.nix`:
- Around line 19-24: The dev shell in the mkShell setup is missing the runtime
binaries used by the action’s main path, so local testing does not match
production behavior. Update the nativeBuildInputs for devShells.default to
include the tools used by the action’s rsync and SSH execution path, alongside
nodejs_24, shellcheck, and shfmt, so nix develop provides everything needed for
realistic smoke tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50cc7193-63a9-4551-9afb-7949237892fe
⛔ Files ignored due to path filters (2)
flake.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.envrc.example.gitignoreREADME.mdactions/rsync-cache/action.ymlactions/rsync-cache/src/commit.jsactions/rsync-cache/src/restore.jsactions/rsync-cache/src/rsync.jsflake.nixpackage.jsonrollup.config.js
pjones
force-pushed
the
pjones/cache-action
branch
2 times, most recently
from
6cb0531 to
dd072f3
Compare
|
This is the caching action I wrote for vcpkg. |
pjones
force-pushed
the
pjones/cache-action
branch
2 times, most recently
from
ca62d9b to
66d0d97
Compare
Probably needs more error handling.
pjones
force-pushed
the
pjones/cache-action
branch
from
66d0d97 to
9e99f4b
Compare
2 participants
Simple directory sync via rsync.
Summary by CodeRabbit