feat: add self-update command for in-place pvm upgrades#19
Open
Fahl-Design wants to merge 1 commit intomainfrom
Open
feat: add self-update command for in-place pvm upgrades#19Fahl-Design wants to merge 1 commit intomainfrom
Fahl-Design wants to merge 1 commit intomainfrom
Conversation
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.
📝 WalkthroughWalkthroughThe PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Handler
participant Version as Version Parser
participant GitHub as GitHub API
participant Network as Network/Download
participant FileOps as File Operations
participant Exec as Executable
User->>CLI: Run pvm self-update [--apply]
CLI->>Version: Parse current PVM_VERSION
Version-->>CLI: Parsed semver
CLI->>GitHub: Fetch latest release
GitHub-->>CLI: Latest tag_name & assets
CLI->>Version: Compare remote vs current
alt Remote version newer
CLI->>User: Display new version available
alt --apply flag set
CLI->>User: Prompt for confirmation
User->>CLI: Approve
CLI->>Network: Download pvm-{target}.tar.gz
Network->>Network: Stream with progress bar
Network-->>FileOps: Temp file with binary
FileOps->>FileOps: Extract pvm from archive
FileOps->>FileOps: Set permissions to 0755
FileOps->>Exec: Atomic replace current binary
FileOps-->>User: Success message
else No --apply flag
CLI-->>User: Instruct to run with --apply
end
else Remote version not newer
CLI-->>User: 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)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
🤖 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
Comment on lines
+23
to
+35
| fn current_version() -> Result<semver::Version> { | ||
| let raw = env!("PVM_VERSION"); | ||
| let token = raw.split_whitespace().next().unwrap_or(raw); | ||
| let trimmed = token.trim_start_matches('v'); | ||
| // git-describe extras (e.g. "-2-gabc") are not valid semver; drop them. | ||
| let core = trimmed.split('-').next().unwrap_or(trimmed); | ||
| semver::Version::parse(core).with_context(|| { | ||
| format!( | ||
| "Failed to parse current pvm version '{}' from '{}'", | ||
| core, raw | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fall back when PVM_VERSION is not semver.
build.rs can embed git describe --always output or "unknown" for CI/non-tagged builds, so this parser can fail before pvm self-update even checks GitHub. Falling back to env!("CARGO_PKG_VERSION") would keep branch/dev artifacts usable.
Suggested change
fn current_version() -> Result<semver::Version> {
let raw = env!("PVM_VERSION");
let token = raw.split_whitespace().next().unwrap_or(raw);
let trimmed = token.trim_start_matches('v');
// git-describe extras (e.g. "-2-gabc") are not valid semver; drop them.
let core = trimmed.split('-').next().unwrap_or(trimmed);
- semver::Version::parse(core).with_context(|| {
- format!(
- "Failed to parse current pvm version '{}' from '{}'",
- core, raw
- )
- })
+ semver::Version::parse(core)
+ .or_else(|_| semver::Version::parse(env!("CARGO_PKG_VERSION")))
+ .with_context(|| {
+ format!(
+ "Failed to parse current pvm version '{}' from '{}'",
+ core, raw
+ )
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn current_version() -> Result<semver::Version> { | |
| let raw = env!("PVM_VERSION"); | |
| let token = raw.split_whitespace().next().unwrap_or(raw); | |
| let trimmed = token.trim_start_matches('v'); | |
| // git-describe extras (e.g. "-2-gabc") are not valid semver; drop them. | |
| let core = trimmed.split('-').next().unwrap_or(trimmed); | |
| semver::Version::parse(core).with_context(|| { | |
| format!( | |
| "Failed to parse current pvm version '{}' from '{}'", | |
| core, raw | |
| ) | |
| }) | |
| } | |
| fn current_version() -> Result<semver::Version> { | |
| let raw = env!("PVM_VERSION"); | |
| let token = raw.split_whitespace().next().unwrap_or(raw); | |
| let trimmed = token.trim_start_matches('v'); | |
| // git-describe extras (e.g. "-2-gabc") are not valid semver; drop them. | |
| let core = trimmed.split('-').next().unwrap_or(trimmed); | |
| semver::Version::parse(core) | |
| .or_else(|_| semver::Version::parse(env!("CARGO_PKG_VERSION"))) | |
| .with_context(|| { | |
| format!( | |
| "Failed to parse current pvm version '{}' from '{}'", | |
| core, raw | |
| ) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/self_update.rs` around lines 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.
Comment on lines
+172
to
+178
| let theme = dialoguer::theme::ColorfulTheme::default(); | ||
| let confirmed = dialoguer::Confirm::with_theme(&theme) | ||
| .with_prompt(format!("Update pvm to {}?", remote_str).bold().to_string()) | ||
| .default(true) | ||
| .interact_opt() | ||
| .unwrap_or(Some(false)) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
git ls-files | grep -E "src/commands|self_update" | head -20Repository: WebProject-xyz/php-version-manager
Length of output: 310
🏁 Script executed:
# Check if the file exists and get its size
wc -l src/commands/self_update.rs 2>/dev/null || echo "File not found"Repository: WebProject-xyz/php-version-manager
Length of output: 110
🏁 Script executed:
# Read the relevant section around lines 172-178
sed -n '160,185p' src/commands/self_update.rsRepository: WebProject-xyz/php-version-manager
Length of output: 857
🏁 Script executed:
# Check the function signature containing these lines
sed -n '1,180p' src/commands/self_update.rs | tail -50Repository: WebProject-xyz/php-version-manager
Length of output: 1778
🏁 Script executed:
# Check imports at the start of the file
head -30 src/commands/self_update.rsRepository: WebProject-xyz/php-version-manager
Length of output: 969
🏁 Script executed:
# Get the full context of the call function
sed -n '135,199p' src/commands/self_update.rsRepository: WebProject-xyz/php-version-manager
Length of output: 2034
Propagate prompt failures instead of reporting a fake cancel.
This currently maps both dialoguer errors and an explicit abort to false, so pvm self-update --apply can exit successfully with "Update cancelled." when stdin is unavailable or the prompt cannot be rendered. That hides a real failure in a command-level fallible path.
Suggested change
let theme = dialoguer::theme::ColorfulTheme::default();
let confirmed = dialoguer::Confirm::with_theme(&theme)
.with_prompt(format!("Update pvm to {}?", remote_str).bold().to_string())
.default(true)
.interact_opt()
- .unwrap_or(Some(false))
+ .context("Failed to read update confirmation from terminal")?
.unwrap_or(false);The function signature pub async fn call(self) -> Result<()> with use anyhow::{Context, Result}; confirms this is a command-level fallible function that should properly propagate I/O errors rather than converting them to user cancellations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let theme = dialoguer::theme::ColorfulTheme::default(); | |
| let confirmed = dialoguer::Confirm::with_theme(&theme) | |
| .with_prompt(format!("Update pvm to {}?", remote_str).bold().to_string()) | |
| .default(true) | |
| .interact_opt() | |
| .unwrap_or(Some(false)) | |
| .unwrap_or(false); | |
| let theme = dialoguer::theme::ColorfulTheme::default(); | |
| let confirmed = dialoguer::Confirm::with_theme(&theme) | |
| .with_prompt(format!("Update pvm to {}?", remote_str).bold().to_string()) | |
| .default(true) | |
| .interact_opt() | |
| .context("Failed to read update confirmation from terminal")? | |
| .unwrap_or(false); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/self_update.rs` around lines 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.
Comment on lines
+30
to
+46
| #[test] | ||
| fn test_self_update_help() { | ||
| let mut cmd = assert_cmd::cargo::cargo_bin_cmd!("pvm"); | ||
| cmd.arg("self-update").arg("--help"); | ||
| cmd.assert() | ||
| .success() | ||
| .stdout(predicate::str::contains("--apply")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_help_lists_self_update() { | ||
| let mut cmd = assert_cmd::cargo::cargo_bin_cmd!("pvm"); | ||
| cmd.arg("--help"); | ||
| cmd.assert() | ||
| .success() | ||
| .stdout(predicate::str::contains("self-update")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Set PVM_DIR in these new integration tests.
These two tests are the only changed integration cases that still run against the caller's real environment. Please create a tempfile::tempdir() and point PVM_DIR at it here too, just like the other CLI tests do.
Suggested change
#[test]
fn test_self_update_help() {
+ let temp_dir = tempfile::tempdir().unwrap();
let mut cmd = assert_cmd::cargo::cargo_bin_cmd!("pvm");
+ cmd.env("PVM_DIR", temp_dir.path());
cmd.arg("self-update").arg("--help");
cmd.assert()
.success()
.stdout(predicate::str::contains("--apply"));
}
#[test]
fn test_help_lists_self_update() {
+ let temp_dir = tempfile::tempdir().unwrap();
let mut cmd = assert_cmd::cargo::cargo_bin_cmd!("pvm");
+ cmd.env("PVM_DIR", temp_dir.path());
cmd.arg("--help");
cmd.assert()
.success()
.stdout(predicate::str::contains("self-update"));
}As per coding guidelines, "Every integration test MUST use tempfile::tempdir() and set PVM_DIR to that path for isolation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli.rs` around lines 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.
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
1 participant
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.
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
self-updatesubcommand to check for and install the latest version--applyflag to download and install updates with confirmation prompts