feat(update): Add update command#80
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return; | ||
| }; | ||
| let how = match detect_install_method() { | ||
| InstallMethod::Homebrew => "Run: brew upgrade hotdata", |
There was a problem hiding this comment.
The Homebrew formula is named cli in the hotdata-dev/tap tap (see dist-workspace.toml line 15: formula = "cli", and the README install command brew install hotdata-dev/tap/cli). brew upgrade hotdata will fail with Error: No formula or cask found for hotdata, sending the user down a confusing path.
Suggest brew upgrade hotdata-dev/tap/cli (matches the README install command and works whether or not the tap is currently pinned to that formula name locally).
| InstallMethod::Homebrew => "Run: brew upgrade hotdata", | |
| InstallMethod::Homebrew => "Run: brew upgrade hotdata-dev/tap/cli", |
|
|
||
| if detect_install_method() == InstallMethod::Homebrew { | ||
| println!("hotdata was installed via Homebrew. Update with:"); | ||
| println!(" {}", "brew upgrade hotdata".cyan()); |
There was a problem hiding this comment.
Same issue as the notice text above — brew upgrade hotdata is not a valid formula name. Suggest matching the README install command (brew install hotdata-dev/tap/cli):
| println!(" {}", "brew upgrade hotdata".cyan()); | |
| println!(" {}", "brew upgrade hotdata-dev/tap/cli".cyan()); |
| if tmp_dir.exists() { | ||
| let _ = fs::remove_dir_all(&tmp_dir); | ||
| } | ||
| fs::create_dir_all(&tmp_dir).map_err(|e| format!("creating temp dir: {e}"))?; |
There was a problem hiding this comment.
nit: (not blocking) Using temp_dir().join(format!("hotdata-update-{}", std::process::id())) makes the path predictable (PID is easy to observe or guess on shared/multi-user systems and on CI runners). The if exists { remove_dir_all } + create_dir_all sequence ignores errors with let _ = ..., so a pre-planted symlink survives the cleanup step (remove_dir_all on a symlink errors out and is dropped; create_dir_all succeeds on a symlink-to-existing-dir). An attacker who wins the race could redirect the tar extraction into a directory they control and have the subsequent Move::to_dest install their binary as hotdata.
tempfile is already in the transitive dep tree via self_update. Suggest tempfile::TempDir::new() (random suffix, mode 0o700) instead — it also handles cleanup on drop, which lets you drop the manual remove_dir_all calls below.
| } | ||
| let xz_bytes = resp | ||
| .bytes() | ||
| .map_err(|e| format!("reading download: {e}"))?; |
There was a problem hiding this comment.
super nit: (not blocking) The downloaded binary isn't verified against a checksum or signature. cargo-dist publishes *.tar.xz.sha256 alongside each asset — fetching and comparing that would catch a corrupted/tampered asset before it lands as hotdata on disk. Most self-updaters skip this and TLS gives transport integrity, so this is a defense-in-depth nit rather than a real concern today.
There was a problem hiding this comment.
Review
Blocking Issues
src/update.rs:137andsrc/update.rs:154— the Homebrew formula is namedcliin thehotdata-dev/taptap (perdist-workspace.tomlformula = "cli"and the README install command).brew upgrade hotdatawill fail withError: No formula or cask found for hotdata, so the user-facing guidance directs Homebrew users to a broken command.
Action Required
Replace brew upgrade hotdata with brew upgrade hotdata-dev/tap/cli in both the update-available notice (line 137) and the hotdata update Homebrew message (line 154) so the suggestion matches the install command in the README.
Two non-blocking items left inline (temp dir predictability, optional checksum verification).
1 participant
No description provided.