feat(self-update): add self-update command for in-place pvm upgrades#19
feat(self-update): add self-update command for in-place pvm upgrades#19Fahl-Design merged 4 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesSelf-update feature (single cohesive DAG)
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Handler
participant Version as Version Parser
participant GitHub as GitHub API
participant Network as HTTP Client
participant FS as File System / Extractor
participant Exec as Executable
User->>CLI: Run `pvm self-update [--apply]`
CLI->>Version: Parse current build `PVM_VERSION`
CLI->>GitHub: GET /releases/latest
GitHub-->>CLI: Return `tag_name` and assets
CLI->>Version: Parse & compare semver
alt remote newer
CLI->>User: Inform new version available
alt --apply
CLI->>User: Prompt for confirmation
User->>CLI: Confirm
CLI->>Network: Download `pvm-{target}.tar.gz` and `.sha256`
Network->>Network: Stream with progress bar
Network-->>FS: Temp file with archive
FS->>FS: Compute digest & extract `pvm` binary, stage next to executable
FS->>FS: Set mode `0755`
FS->>Exec: Atomically replace current binary
Exec-->>User: Print success message
else no --apply
CLI-->>User: Instruct to run with `--apply`
end
else up-to-date
CLI-->>User: Print already up to date
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/self_update.rs`:
- Around line 23-35: The current_version() function assumes env!("PVM_VERSION")
is valid semver and fails parsing for git-describe or "unknown"; update
current_version() to attempt parsing PVM_VERSION as now, but if parsing fails
fall back to env!("CARGO_PKG_VERSION") (parse that and return it) so
non-tagged/CI builds still produce a usable Version; reference
current_version(), the use of env!("PVM_VERSION") and
semver::Version::parse(core) when implementing the try-then-fallback behavior
and ensure the original context error is preserved or replaced with a clear
message if both parses fail.
- Around line 172-178: The prompt currently swallows dialoguer errors by using
unwrap_or to map both I/O errors and explicit user aborts to false; update the
dialog handling so errors propagate instead of being treated as a cancellation:
replace the chained unwrap_or calls on
dialoguer::Confirm::with_theme(...).interact_opt() with an error-propagating
call (use the Result-returning interact_opt()? and the crate's Context or ?
operator) to return Err on prompt failures and only map the Option<bool> to a
concrete bool with unwrap_or(false) after the ?; e.g., call
interact_opt().context("prompt failed")? and then .unwrap_or(false) to set
confirmed in pub async fn call.
In `@tests/cli.rs`:
- Around line 30-46: Both new tests (test_self_update_help and
test_help_lists_self_update) must create a tempfile::tempdir() and set the
PVM_DIR environment variable to that tempdir path before asserting;
specifically, in each test create let tmp = tempfile::tempdir().unwrap() (or
similar) and call cmd.env("PVM_DIR", tmp.path()) (or
tmp.path().to_str().unwrap()) before cmd.assert(), ensuring isolation consistent
with the other CLI 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: eb0b5153-1c79-4849-b2f3-7b9608e68e9e
📒 Files selected for processing (5)
src/cli.rssrc/commands/mod.rssrc/commands/self_update.rssrc/network.rstests/cli.rs
- self_update: fall back to CARGO_PKG_VERSION when PVM_VERSION is not valid semver so non-tagged/CI builds still parse a usable version. - self_update: propagate dialoguer prompt errors via .context()? instead of silently mapping them to a user cancellation. - tests: isolate the two new self-update CLI tests with tempdir + PVM_DIR per the project's integration-test convention.
- self_update: fall back to CARGO_PKG_VERSION when PVM_VERSION is not valid semver so non-tagged/CI builds still parse a usable version. - self_update: propagate dialoguer prompt errors via .context()? instead of silently mapping them to a user cancellation. - tests: isolate the two new self-update CLI tests with tempdir + PVM_DIR per the project's integration-test convention.
af0e922 to
eddf267
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/network.rs (1)
67-85: ⚖️ Poor tradeoffConsider yielding control during chunk writes on the async runtime.
stream_to_tempfilewrites each chunk to a synchronousstd::fs::Fileon the async task. For typical archive sizes this is fine, but a slow disk could stall the executor thread. If you want extra resilience, wrapping the per-chunkwrite_allintokio::task::spawn_blocking(or switching totokio::fs::File) would keep the runtime responsive. Not blocking — current behavior is the same as the prior in-line implementation.🤖 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 `@src/network.rs` around lines 67 - 85, The stream_to_tempfile function writes chunks to a blocking std::fs::File on the async runtime; to avoid stalling the executor, change the per-chunk sync writes to run off the async thread by either (a) switch to an async file API (use tokio::fs::File and its write_all in an async context, updating seek/flush calls accordingly) or (b) keep the tempfile but wrap the tmp.write_all(&chunk) (and any tmp.flush()/seek if they might block) inside tokio::task::spawn_blocking and await it; update references inside stream_to_tempfile to use the chosen approach so each chunk write yields the runtime instead of blocking.tests/cli.rs (1)
30-50: ⚡ Quick winConsider adding unit tests for version parsing.
The two new integration tests cover only help output. The non-trivial parsing logic in
current_version(leading-vstrip, git-describe suffix drop, fallback toCARGO_PKG_VERSION) andparse_remote_versionwould benefit from cheap, hermetic unit tests insidesrc/commands/self_update.rs(#[cfg(test)] mod tests { ... }) — e.g., assertingv1.2.3,1.2.3-2-gabc, andunknownall yield sensiblesemver::Versionvalues. Network-dependent paths can stay manual.🤖 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 `@tests/cli.rs` around lines 30 - 50, Add hermetic unit tests for the version-parsing logic by creating a #[cfg(test)] mod tests in src/commands/self_update.rs that exercises current_version and parse_remote_version directly (no network). Write tests asserting that inputs like "v1.2.3" and "1.2.3-2-gabc" are parsed to the expected semver::Version (e.g., 1.2.3) and that fallback/unknown values produce the CARGO_PKG_VERSION or an appropriate parse error as intended; include separate test cases for leading-'v' stripping, git-describe suffix removal, and the unknown/fallback path. Use assert_eq! or predicates on semver::Version to keep tests cheap and deterministic. Ensure tests call the exact functions current_version and parse_remote_version so they run fast and hermetically.src/commands/self_update.rs (2)
64-97: 💤 Low valueSurface a clearer error when staging directory is not writable.
When
pvmis installed system-wide (e.g./usr/local/bin/pvm) without write access,tempfile_in(exe_dir)will fail with a low-levelPermission deniedmessage. Consider hinting at the likely cause (run with elevated privileges or reinstall via the original installer) so end-users aren't left to guess. Not blocking — the error is currently propagated with context, so the failure is at least visible.Suggested tweak
let mut staged = tempfile::Builder::new() .prefix(".pvm-update-") .tempfile_in(exe_dir) - .context("Failed to create staging file next to current executable")?; + .with_context(|| { + format!( + "Failed to create staging file in {:?} (need write access; \ + if pvm was installed system-wide, re-run with appropriate privileges)", + exe_dir + ) + })?;🤖 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 `@src/commands/self_update.rs` around lines 64 - 97, The tempfile staging call in download_and_replace (tempfile::Builder::new().tempfile_in(exe_dir)) can fail with a bare "Permission denied"; catch that specific error and return a clearer message suggesting likely causes and remedies (e.g., pvm is installed system-wide and requires elevated privileges or reinstall via the original installer). Replace the current .context("Failed to create staging file next to current executable") with logic that maps the error: detect std::io::ErrorKind::PermissionDenied (or downcast the anyhow error to std::io::Error) and return an anyhow::Context/Err with a human-friendly message referencing exe_dir and suggesting running with elevated privileges or reinstalling, otherwise preserve the original error for other kinds. Ensure this change is applied around the tempfile_in(...) call that creates staged.
92-124: 💤 Low valueThe release pipeline confirms single binary per archive—consider adding defensive file type validation.
The release workflow (
tar -C target/${{ matrix.target }}/release -czf ... pvm) creates a flat archive containing exactly onepvmbinary per release. This is tightly controlled across all three build targets (linux-x86_64, macos-aarch64, macos-x86_64), so the file-basename matching on line 113–114 is safe in practice.However, the extraction code does not validate that the matched entry is a regular file. Consider adding a defensive check via
tar::Entry's file type methods (e.g., checkingheader().entry_type()) before copying to caught non-file entries early, though the controlled release pipeline makes this unlikely to occur.🤖 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 `@src/commands/self_update.rs` around lines 92 - 124, The extraction loop accepts any archive entry named "pvm" without verifying it's a regular file; update the loop in the self_update logic (the Archive/entry handling around staged, archive, entry) to check the entry type before copying (e.g., use entry.header().entry_type() or tar::Entry::header() to ensure it's a regular file) and skip or bail on non-file types so only a proper file is written to staged; keep the existing name-match and error messages but add this defensive validation and appropriate context-aware error handling when the entry is not a regular file.
🤖 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.
Nitpick comments:
In `@src/commands/self_update.rs`:
- Around line 64-97: The tempfile staging call in download_and_replace
(tempfile::Builder::new().tempfile_in(exe_dir)) can fail with a bare "Permission
denied"; catch that specific error and return a clearer message suggesting
likely causes and remedies (e.g., pvm is installed system-wide and requires
elevated privileges or reinstall via the original installer). Replace the
current .context("Failed to create staging file next to current executable")
with logic that maps the error: detect std::io::ErrorKind::PermissionDenied (or
downcast the anyhow error to std::io::Error) and return an anyhow::Context/Err
with a human-friendly message referencing exe_dir and suggesting running with
elevated privileges or reinstalling, otherwise preserve the original error for
other kinds. Ensure this change is applied around the tempfile_in(...) call that
creates staged.
- Around line 92-124: The extraction loop accepts any archive entry named "pvm"
without verifying it's a regular file; update the loop in the self_update logic
(the Archive/entry handling around staged, archive, entry) to check the entry
type before copying (e.g., use entry.header().entry_type() or
tar::Entry::header() to ensure it's a regular file) and skip or bail on non-file
types so only a proper file is written to staged; keep the existing name-match
and error messages but add this defensive validation and appropriate
context-aware error handling when the entry is not a regular file.
In `@src/network.rs`:
- Around line 67-85: The stream_to_tempfile function writes chunks to a blocking
std::fs::File on the async runtime; to avoid stalling the executor, change the
per-chunk sync writes to run off the async thread by either (a) switch to an
async file API (use tokio::fs::File and its write_all in an async context,
updating seek/flush calls accordingly) or (b) keep the tempfile but wrap the
tmp.write_all(&chunk) (and any tmp.flush()/seek if they might block) inside
tokio::task::spawn_blocking and await it; update references inside
stream_to_tempfile to use the chosen approach so each chunk write yields the
runtime instead of blocking.
In `@tests/cli.rs`:
- Around line 30-50: Add hermetic unit tests for the version-parsing logic by
creating a #[cfg(test)] mod tests in src/commands/self_update.rs that exercises
current_version and parse_remote_version directly (no network). Write tests
asserting that inputs like "v1.2.3" and "1.2.3-2-gabc" are parsed to the
expected semver::Version (e.g., 1.2.3) and that fallback/unknown values produce
the CARGO_PKG_VERSION or an appropriate parse error as intended; include
separate test cases for leading-'v' stripping, git-describe suffix removal, and
the unknown/fallback path. Use assert_eq! or predicates on semver::Version to
keep tests cheap and deterministic. Ensure tests call the exact functions
current_version and parse_remote_version so they run fast and hermetically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8cbdfe3-3d28-440c-8199-f5cb301fc486
📒 Files selected for processing (5)
src/cli.rssrc/commands/mod.rssrc/commands/self_update.rssrc/network.rstests/cli.rs
Extract parse_pvm_version(&str) helper from current_version() so the leading-v strip, git-describe suffix drop, whitespace-token split, and CARGO_PKG_VERSION fallback paths can be exercised hermetically. Adds tests for parse_remote_version too. Addresses CodeRabbit nitpick on PR #19. The other three nitpicks (async blocking writes in stream_to_tempfile, permission-denied hint on tempfile_in, tar entry-type validation) were rated low-value / not-blocking by CodeRabbit itself and are intentionally skipped.
- self_update: fall back to CARGO_PKG_VERSION when PVM_VERSION is not valid semver so non-tagged/CI builds still parse a usable version. - self_update: propagate dialoguer prompt errors via .context()? instead of silently mapping them to a user cancellation. - tests: isolate the two new self-update CLI tests with tempdir + PVM_DIR per the project's integration-test convention.
Extract parse_pvm_version(&str) helper from current_version() so the leading-v strip, git-describe suffix drop, whitespace-token split, and CARGO_PKG_VERSION fallback paths can be exercised hermetically. Adds tests for parse_remote_version too. Addresses CodeRabbit nitpick on PR #19. The other three nitpicks (async blocking writes in stream_to_tempfile, permission-denied hint on tempfile_in, tar entry-type validation) were rated low-value / not-blocking by CodeRabbit itself and are intentionally skipped.
f9d58e9 to
a0a6ab3
Compare
- self_update: fall back to CARGO_PKG_VERSION when PVM_VERSION is not valid semver so non-tagged/CI builds still parse a usable version. - self_update: propagate dialoguer prompt errors via .context()? instead of silently mapping them to a user cancellation. - tests: isolate the two new self-update CLI tests with tempdir + PVM_DIR per the project's integration-test convention.
Extract parse_pvm_version(&str) helper from current_version() so the leading-v strip, git-describe suffix drop, whitespace-token split, and CARGO_PKG_VERSION fallback paths can be exercised hermetically. Adds tests for parse_remote_version too. Addresses CodeRabbit nitpick on PR #19. The other three nitpicks (async blocking writes in stream_to_tempfile, permission-denied hint on tempfile_in, tar entry-type validation) were rated low-value / not-blocking by CodeRabbit itself and are intentionally skipped.
a0a6ab3 to
bf85ff8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/commands/self_update.rs (2)
48-65: ⚡ Quick winConsider pinning the GitHub API version header.
"You should use the
X-GitHub-Api-Versionheader to specify an API version." Without it, requests default to the2022-11-28version — which is the same as explicitly pinning it. The benefit of adding it now is protection against future default version bumps: when a new REST API version is released, the previous version is supported for at least 24 more months, giving time to react, but without the header you'll be silently moved to whatever the new default becomes.✨ Suggested addition
let release = network::http_client()? .get(&url) .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") .send()🤖 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 `@src/commands/self_update.rs` around lines 48 - 65, The fetch_latest_release function should pin the GitHub REST API version by adding the X-GitHub-Api-Version header to the request; update the request built in fetch_latest_release to include .header("X-GitHub-Api-Version", "2022-11-28") alongside the existing Accept header so the call to network::http_client() in fetch_latest_release explicitly targets the 2022-11-28 API version.
48-65: ⚡ Quick winConsider explicitly pinning the GitHub API version for clarity.
Adding the
X-GitHub-Api-Versionheader makes the API contract explicit and improves code maintainability. While the header is optional and requests default to version 2022-11-28 (which is currently the documented stable default), explicitly pinning the version prevents ambiguity and is a recommended best practice per GitHub's API documentation.✨ Suggested addition
let release = network::http_client()? .get(&url) .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") .send()🤖 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 `@src/commands/self_update.rs` around lines 48 - 65, In fetch_latest_release add the explicit GitHub API version header: when building the request returned by network::http_client().get(&url) include .header("X-GitHub-Api-Version", "2022-11-28") alongside the existing Accept header so the request pins the API contract; update the request in function fetch_latest_release to send this extra header before .send().await to make the API versioning explicit.
🤖 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 `@src/commands/self_update.rs`:
- Around line 83-93: The download path accepts the archived release without
integrity checks: after obtaining response and writing to the tempfile via
network::stream_to_tempfile (where tmp is returned), fetch the companion
checksum (e.g., by adding a helper like fetch_text) for the release asset URL
(url + ".sha256"), compute the SHA-256 of the tempfile (seek tmp to start, hash
contents), and compare it to the fetched expected value (implement verify_sha256
or inline the comparison); if they differ return an error/context ("Integrity
check failed") before rewinding tmp (seek to start) and proceeding to
GzDecoder/installation so the binary is never staged without a verified
checksum.
- Around line 83-93: Add a SHA-256 integrity check after
network::stream_to_tempfile(...) and before replacing the executable: use
network::http_client() to GET a checksum asset (e.g. format!("{}.sha256", url)),
read the expected hex checksum, compute the SHA-256 of the tempfile returned by
stream_to_tempfile (tmp) and compare the values, and return an error (with
context like "Integrity check failed for downloaded release") if they differ;
only call the existing persist/staging logic when the checksum matches. Ensure
any I/O on tmp uses its path/bytes (not re-downloading) and surface clear,
contextual errors.
---
Nitpick comments:
In `@src/commands/self_update.rs`:
- Around line 48-65: The fetch_latest_release function should pin the GitHub
REST API version by adding the X-GitHub-Api-Version header to the request;
update the request built in fetch_latest_release to include
.header("X-GitHub-Api-Version", "2022-11-28") alongside the existing Accept
header so the call to network::http_client() in fetch_latest_release explicitly
targets the 2022-11-28 API version.
- Around line 48-65: In fetch_latest_release add the explicit GitHub API version
header: when building the request returned by network::http_client().get(&url)
include .header("X-GitHub-Api-Version", "2022-11-28") alongside the existing
Accept header so the request pins the API contract; update the request in
function fetch_latest_release to send this extra header before .send().await to
make the API versioning explicit.
🪄 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: 1d763181-e27a-4fdb-af59-52aafed67ea0
📒 Files selected for processing (5)
src/cli.rssrc/commands/mod.rssrc/commands/self_update.rssrc/network.rstests/cli.rs
✅ Files skipped from review due to trivial changes (1)
- src/commands/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli.rs
- tests/cli.rs
Fetches a companion .sha256 file alongside the release tarball and
hashes the downloaded bytes before atomically replacing the running
binary. Mismatched or missing checksums abort the update so a
compromised CDN response or wrong-asset upload cannot silently install
an unverified binary.
Adds sha256sum/shasum step to the release workflow that uploads
pvm-{target}.tar.gz.sha256 next to each archive.
Addresses CodeRabbit review on PR #19.
Adds 'pvm self-update' (check only) and 'pvm self-update --apply' (check + interactive Confirm + atomic binary replace) using the GitHub releases API and the same release artifacts that install.sh publishes. Extracts shared HTTP client, target-triple detection, progress bar, and stream-to-tempfile helpers from network.rs so the new command reuses them instead of duplicating ~50 lines.
- self_update: fall back to CARGO_PKG_VERSION when PVM_VERSION is not valid semver so non-tagged/CI builds still parse a usable version. - self_update: propagate dialoguer prompt errors via .context()? instead of silently mapping them to a user cancellation. - tests: isolate the two new self-update CLI tests with tempdir + PVM_DIR per the project's integration-test convention.
Extract parse_pvm_version(&str) helper from current_version() so the leading-v strip, git-describe suffix drop, whitespace-token split, and CARGO_PKG_VERSION fallback paths can be exercised hermetically. Adds tests for parse_remote_version too. Addresses CodeRabbit nitpick on PR #19. The other three nitpicks (async blocking writes in stream_to_tempfile, permission-denied hint on tempfile_in, tar entry-type validation) were rated low-value / not-blocking by CodeRabbit itself and are intentionally skipped.
Fetches a companion .sha256 file alongside the release tarball and
hashes the downloaded bytes before atomically replacing the running
binary. Mismatched or missing checksums abort the update so a
compromised CDN response or wrong-asset upload cannot silently install
an unverified binary.
Adds sha256sum/shasum step to the release workflow that uploads
pvm-{target}.tar.gz.sha256 next to each archive.
Addresses CodeRabbit review on PR #19.
0255ee5 to
a054812
Compare
## [1.2.0](v1.1.2...v1.2.0) (2026-05-07) ### Features * **self-update:** add self-update command for in-place pvm upgrades ([#19](#19)) ([d77f0ef](d77f0ef))
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary
Asset naming and repo match 'install.sh' so the same release artifacts cover both first-time install and self-update.
Implementation notes
Test plan
Summary by CodeRabbit
New Features
--applycan download, verify (SHA‑256), and atomically replace the running executable with streamed progress and confirmation prompts; without--applyit prints instructions to rerun with--apply.Tests
--applyflag in automated tests.Chores