GitHub Action to cache a directory via rsync#7
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a new ChangesCache via rsync action
Sequence Diagram(s)sequenceDiagram
participant GitHubAction
participant restore.js
participant Rsync
participant rsync
participant commit.js
GitHubAction->>restore.js: start restore step
restore.js->>Rsync: pull()
Rsync->>rsync: sync remote cache to local path
rsync-->>Rsync: exit code
GitHubAction->>commit.js: run post step after success
commit.js->>Rsync: push()
Rsync->>rsync: sync local path to remote cache
rsync-->>Rsync: exit code
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
6cb0531 to
dd072f3
Compare
|
This is the caching action I wrote for vcpkg. |
ca62d9b to
66d0d97
Compare
Probably needs more error handling.
66d0d97 to
9e99f4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
actions/rsync-cache/src/rsync.js (2)
45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
endsWithfor clarity.Using
String.prototype.endsWith()is more direct, robust, and readable than a regular expression for checking a trailing slash.♻️ Proposed refactor
- if (!this.path.match(/\/$/)) { + if (!this.path.endsWith("/")) { this.path += "/"; }🤖 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 45 - 47, Update the trailing-slash check in the path normalization logic to use String.prototype.endsWith("/") instead of the regular expression, while preserving the existing behavior of appending a slash only when this.path does not already end with one.
29-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the empty-string check.
Using
.trim()simplifies this check by explicitly conveying the intent and avoiding a regular expression.♻️ Proposed refactor
- if ( - !this.ssh_key || - this.ssh_key.length == 0 || - this.ssh_key.match(/^\s*$/) - ) { + if (!this.ssh_key || this.ssh_key.trim().length === 0) {🤖 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 29 - 33, Update the ssh_key validation condition in the surrounding method to use trim-based whitespace checking instead of ssh_key.match(/^\s*$/), while preserving the existing handling for missing, empty, and whitespace-only keys..github/workflows/test.yml (2)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider explicitly setting the Node.js version.
The workflow runs
npm run testandnpm run buildusing the runner's default Node.js version. Since this action is designed for the Node 24 execution lifecycle, consider usingactions/setup-nodeto explicitly test against Node 24 and ensure consistency.♻️ Proposed refactor
set -eu + # Set up Node.js + # - uses: actions/setup-node@v4 + # with: + # node-version: '24' + # Run all of the tests. npm run test(Note: You will need to add the
usesstep before therunstep block if you decide to implement this).🤖 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 @.github/workflows/test.yml around lines 20 - 22, Add an actions/setup-node step before the test and build commands, explicitly configuring Node.js 24 so both npm run test and npm run build use the intended runtime.
17-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseto improve security posture.As highlighted by static analysis, when
actions/checkoutis used withoutpersist-credentials: false, the repositoryGITHUB_TOKENis persisted in the local git config, which can be inadvertently exposed to subsequent steps. Disabling this reduces the risk of credential leakage.🛡️ Proposed refactor
- - uses: actions/checkout@v6 + - uses: actions/checkout@v6 + with: + persist-credentials: false🤖 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 @.github/workflows/test.yml at line 17, Update the actions/checkout step in the workflow to set persist-credentials to false, ensuring the repository token is not stored in local Git configuration while preserving the existing checkout behavior.Source: Linters/SAST tools
actions/rsync-cache/test/rsync_test.js (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider throwing an
Errorobject instead of a string.Throwing an
Errorobject preserves the stack trace and aligns with best practices for error handling in JavaScript.♻️ Proposed refactor
if (options["required"] && !val) { - throw `missing required input: ${key}`; + throw new Error(`missing required input: ${key}`); }🤖 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/test/rsync_test.js` around lines 57 - 59, Update the required-input validation in the options handling block to throw an Error object with the existing “missing required input” message instead of throwing a string, preserving the current validation condition and message content.
🤖 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 25-26: Update the SSH key path initialization in the constructor
to generate a unique key filename for each execution instead of always using
private.key, while preserving the existing ~/.ssh directory location and
ensuring all subsequent key operations use the generated path.
- Around line 102-118: Wrap the rsync execution and key-file cleanup in the
method containing the shown flow with a try...finally block, ensuring
this.fs.unlink(this.ssh_key_file) runs even when this.fs.chmod or this.exec.exec
throws. Use return await for the exec result inside the try block so the finally
cleanup waits for command completion, while preserving the existing exit_code
return behavior.
In `@actions/rsync-cache/test/rsync_test.js`:
- Around line 192-205: Replace the async forEach callback in the “disabled on
missing ssh key” test with an awaited sequential loop, preserving the existing
key values and assertions so each rsync.run() completes before the test
finishes.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 20-22: Add an actions/setup-node step before the test and build
commands, explicitly configuring Node.js 24 so both npm run test and npm run
build use the intended runtime.
- Line 17: Update the actions/checkout step in the workflow to set
persist-credentials to false, ensuring the repository token is not stored in
local Git configuration while preserving the existing checkout behavior.
In `@actions/rsync-cache/src/rsync.js`:
- Around line 45-47: Update the trailing-slash check in the path normalization
logic to use String.prototype.endsWith("/") instead of the regular expression,
while preserving the existing behavior of appending a slash only when this.path
does not already end with one.
- Around line 29-33: Update the ssh_key validation condition in the surrounding
method to use trim-based whitespace checking instead of ssh_key.match(/^\s*$/),
while preserving the existing handling for missing, empty, and whitespace-only
keys.
In `@actions/rsync-cache/test/rsync_test.js`:
- Around line 57-59: Update the required-input validation in the options
handling block to throw an Error object with the existing “missing required
input” message instead of throwing a string, preserving the current validation
condition and message content.
🪄 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: 16fc13c5-69a4-4b2e-bd73-c766f4845f16
⛔ Files ignored due to path filters (4)
actions/rsync-cache/dist/commit.jsis excluded by!**/dist/**actions/rsync-cache/dist/commit.js.mapis excluded by!**/dist/**,!**/*.mapactions/rsync-cache/dist/restore.jsis excluded by!**/dist/**actions/rsync-cache/dist/restore.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (10)
.github/workflows/test.ymlREADME.mdactions/rsync-cache/action.ymlactions/rsync-cache/src/commit.jsactions/rsync-cache/src/restore.jsactions/rsync-cache/src/rsync.jsactions/rsync-cache/test/rsync_test.jsflake.nixpackage.jsonrollup.config.js
🚧 Files skipped from review as they are similar to previous changes (7)
- rollup.config.js
- actions/rsync-cache/src/restore.js
- actions/rsync-cache/src/commit.js
- actions/rsync-cache/action.yml
- flake.nix
- package.json
- README.md
0b014e4 to
22e5ba5
Compare
22e5ba5 to
469e7f7
Compare
|
@timosachsenberg I added tests and finished this up. If you approve we can start using it. |
|
give me a minute... looking at it now |
|
Code looks solid. Thanks! |
|
@castorNova2 this caching action is ready to use with vcpkg. |
2 participants
Simple directory sync via rsync.
Summary by CodeRabbit