Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an auto-update feature to the tiles CLI: a new public installer module implements fetching the latest release from GitHub, comparing it to the current Cargo package version (using semver), and running an installation script when an update is available. The CLI gains a new Update command and invokes an automatic update check on startup via try_app_update. Unit tests for the GitHub fetch behavior and new dev dependencies (wiremock, async-std) and semver were added to Cargo.toml. Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as tiles CLI
participant Commands as commands::try_app_update
participant Installer as utils::installer
participant GitHub as GitHub API
participant Local as Local Version
participant Shell as Install Script (curl | sh)
User->>CLI: run (no subcommand) or "tiles update"
CLI->>Commands: try_app_update() / invoke Update handler
Commands->>Installer: get_update_info()
Installer->>Local: read current version
Installer->>GitHub: GET /repos/tilesprivacy/tiles/releases/latest
GitHub-->>Installer: latest release JSON
Installer->>Installer: parse tag_name, compare via semver
alt update available
Installer->>CLI: return UpdateInfo (can_update=true)
CLI->>User: prompt to confirm
User-->>CLI: confirm
CLI->>Installer: try_update(UpdateInfo)
Installer->>Shell: run curl | sh to install
Shell-->>Installer: install result
Installer-->>CLI: success message
else already latest
Installer-->>CLI: "Already latest version"
end
CLI-->>User: final output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tiles/src/utils/installer.rs (1)
75-81: Avoidprintln!in utility code paths; return structured errors/results instead.Printing transient debug output from library utility methods makes CLI output noisy and harder to script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/installer.rs` around lines 75 - 81, The utility currently prints debug output (println!("err {:?}", err) and println!("{}", release_data.tag_name)) instead of returning structured results; remove both println! calls, propagate the original error with context (e.g. return Err(err).context("failed to fetch release") or Err(anyhow!(err).context(...)) in the error branch rather than Err(anyhow!("request failed")), and simply return Ok(release_data.tag_name) without printing; refer to the Release type, the release_data variable and its tag_name field when making these changes.scripts/install.sh (1)
4-4: MakeENVoverrideable instead of hard-coding production.Line 4 forces prod mode and removes runtime control for CI/dev install paths. Prefer defaulting to prod while allowing override.
Proposed change
-ENV="prod" # prod is another env, try taking it from github env +ENV="${TILES_ENV:-prod}" # allow override, default to prod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` at line 4, The script currently hard-codes ENV to "prod"; change the assignment so it defaults to "prod" but can be overridden by an environment variable (e.g., via GitHub Actions or CI) by using shell parameter expansion to set ENV only when not already provided and ensure it is exported for subprocesses (reference the ENV variable assignment in the script).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tiles/Cargo.toml`:
- Line 3: The Cargo.toml version field was regressed to "0.4.0" which risks
breaking update/upgrade semantics; revert this change by restoring the crate
version in Cargo.toml to the next expected/released version (the previous
forward version used before this PR) instead of "0.4.0", and ensure the package
version in Cargo.toml, workspace manifests, and any release
notes/changelog/Cargo.lock remain consistent with that restored version.
In `@tiles/src/utils/installer.rs`:
- Around line 39-51: The updater currently ignores the shell exit status and
always returns "Update completed"; modify the code after spawning the installer
to check the exit Status returned by run_sh_cmd (the variable holding
Command::new("sh").status()), and if status.success() is false return an Err
with a descriptive message (or propagate a failure via the existing error type)
instead of Ok; also propagate spawn errors from child and avoid unwrap on
child.stdout (use ok_or_else to return a clear error) so both spawn and non-zero
exit codes cause the function to return an error rather than reporting success.
- Around line 73-80: The current match on res only handles transport errors and
then reuses res to call .json(), which ignores HTTP status codes; replace the
match and the res handling in the installer function with a direct request
pipeline using .send().await?.error_for_status()? to propagate 4xx/5xx errors,
then deserialize with .json::<Release>().await? (i.e., let release_data: Release
= client.request(...).send().await?.error_for_status()?.json::<Release>().await?
); ensure any error context uses anyhow! where previously used.
- Around line 31-37: The code currently parses latest_vsn with VersionReq and
uses matches(), which treats latest_vsn as a range; instead parse latest_vsn
into a semver::Version (strip a leading 'v' if present) and compare the two
Version values directly (e.g., current_vsn >= latest_vsn) to decide "Already
latest version"; replace VersionReq::parse(&latest_vsn) with
Version::parse(cleaned_latest_vsn) and update the conditional to use direct
Version comparison, returning an error if parsing latest_vsn fails.
---
Nitpick comments:
In `@scripts/install.sh`:
- Line 4: The script currently hard-codes ENV to "prod"; change the assignment
so it defaults to "prod" but can be overridden by an environment variable (e.g.,
via GitHub Actions or CI) by using shell parameter expansion to set ENV only
when not already provided and ensure it is exported for subprocesses (reference
the ENV variable assignment in the script).
In `@tiles/src/utils/installer.rs`:
- Around line 75-81: The utility currently prints debug output (println!("err
{:?}", err) and println!("{}", release_data.tag_name)) instead of returning
structured results; remove both println! calls, propagate the original error
with context (e.g. return Err(err).context("failed to fetch release") or
Err(anyhow!(err).context(...)) in the error branch rather than
Err(anyhow!("request failed")), and simply return Ok(release_data.tag_name)
without printing; refer to the Release type, the release_data variable and its
tag_name field when making these changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
scripts/install.shtiles/Cargo.tomltiles/src/main.rstiles/src/utils/installer.rstiles/src/utils/mod.rs
- refactor: Tests + refactoring
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tiles/src/utils/installer.rs (2)
41-57:⚠️ Potential issue | 🔴 CriticalCheck both installer process exit codes before returning success.
try_updatestill returns success even whenshexits non-zero, and it never checkscurl’s final exit status. That can report successful upgrades on failed installs.Proposed fix
- let _run_sh_cmd = Command::new("sh") + let run_sh_status = Command::new("sh") .stdin( curl_process .stdout .take() .ok_or_else(|| anyhow!("Failed to pipe from the curled input"))?, ) .stdout(Stdio::inherit()) .stderr(Stdio::inherit()) .status()?; + if !run_sh_status.success() { + return Err(anyhow!( + "Install script failed with status: {}", + run_sh_status + )); + } + + let curl_status = curl_process.wait()?; + if !curl_status.success() { + return Err(anyhow!("Failed to download installer script: {}", curl_status)); + }#!/bin/bash # Verify that command statuses are checked before success is returned. cat -n tiles/src/utils/installer.rs | sed -n '38,70p' rg -n '\.status\(\)\?' tiles/src/utils/installer.rs rg -n '\.success\(' tiles/src/utils/installer.rs rg -n '\.wait\(' tiles/src/utils/installer.rsAs per coding guidelines,
**/*.{rs,toml}requires highlighting Rust systems/safety deviations and security concerns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/installer.rs` around lines 41 - 57, try_update currently spawns curl (curl_process) and then runs sh with the piped stdout but does not check both processes' exit codes, so failures can be masked; update the logic in try_update to call curl_process.wait() (or wait_with_output) to capture curl's exit status and capture the result of the sh command (currently stored in _run_sh_cmd), then check both Status.success(); if either is false return an Err(anyhow!(...)) with a clear message including which process failed and its exit code, otherwise return Ok(()) so success is only returned when both curl_process and the sh command exit successfully.
68-83:⚠️ Potential issue | 🟠 MajorUse
semver::Versionfor latest-version comparison, notVersionReq.
VersionReq::parse("0.4.1")is a compatible range, not an exact latest-version value. This can produce wrong update decisions (including downgrade prompts).Proposed fix
-use semver::{Version, VersionReq}; +use semver::Version; @@ - let req_vsn = VersionReq::parse(&latest_vsn)?; + let latest_vsn = latest_vsn.trim_start_matches('v'); + let latest_vsn = Version::parse(latest_vsn) + .map_err(|e| anyhow!("Failed to parse latest version due to {}", e))?; let current_vsn = Version::parse(env!("CARGO_PKG_VERSION")) .map_err(|e| anyhow!("Failed to parse pkg version due to {}", e))?; - if req_vsn.matches(¤t_vsn) { + if current_vsn >= latest_vsn { Ok(UpdateInfo { can_update: false, - latest_version: req_vsn.to_string(), + latest_version: latest_vsn.to_string(), current_version: current_vsn.to_string(), }) } else { Ok(UpdateInfo { can_update: true, - latest_version: req_vsn.to_string(), + latest_version: latest_vsn.to_string(), current_version: current_vsn.to_string(), }) }In Rust semver crate, what is the semantic difference between VersionReq::parse("0.4.1") and Version::parse("0.4.1"), and which should be used when comparing a current version against a concrete latest release version?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/utils/installer.rs` around lines 68 - 83, The code currently parses latest_vsn with VersionReq::parse and then uses matches against current_vsn, which treats the latest as a version requirement/range; instead parse latest_vsn into a semver::Version (use Version::parse on latest_vsn), compare the two Version values (equality or greater-than as appropriate) to decide can_update, and populate UpdateInfo.latest_version/current_version from the Version instances; update variable names (req_vsn -> latest_version or latest_vsn_parsed) and adjust the logic in the function containing req_vsn/current_vsn to use Version::parse for latest_vsn and compare Versions rather than VersionReq::matches.
🧹 Nitpick comments (1)
tiles/src/commands/mod.rs (1)
247-247: Surface the updater result to the user.Line 247 discards the returned message from
try_update, so users may not see a clear completion status.Proposed fix
- try_update(Some(update_info)).await?; + let msg = try_update(Some(update_info)).await?; + println!("{}", msg.green());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/commands/mod.rs` at line 247, The call to try_update(Some(update_info)).await? discards its returned message; capture its result and surface it to the user by assigning it to a variable (e.g., let result = try_update(Some(update_info)).await?;) and then print or log that result (e.g., println!("{}", result) or process_logger.info!("{}", result)) so the user sees the completion status; update the code around try_update to use the captured variable instead of discarding the return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tiles/src/commands/mod.rs`:
- Around line 242-245: The blocking call stdin.read_line(&mut input) inside
try_app_update (the code that creates stdin, input, and calls read_line) will
block the single-threaded Tokio runtime; move the blocking work into
tokio::task::spawn_blocking: capture stdin and the mutable buffer inside a
closure that performs the read_line and returns the read String (or
io::Result<String>), await the JoinHandle, propagate any errors, then continue
to call trim() on the returned String (clean_input). Replace the direct
read_line call with spawn_blocking(...).await? and handle both the join error
and the inner io::Result so the function's existing error propagation remains
intact.
In `@tiles/src/utils/installer.rs`:
- Around line 41-44: The current installer spawns Command::new("curl") into
curl_process to pipe https://tiles.run/install.sh directly to a shell; change
this to a secure download-and-verify flow: use Command::new("curl")/reqwest to
download the script to a temporary file, fetch or embed a pinned checksum or
signature for the release, verify the downloaded file against that
checksum/signature, and only then exec Command::new("sh") or run the file;
update the logic around the curl_process variable and the execution path that
currently pipes to sh so the code performs integrity verification (or switches
to installing pinned release artifacts) before any execution.
- Around line 58-60: There's a typo in the success message string in
installer.rs: change the format string used in the Ok(format!(...)) that
currently reads "Tiles updagraded to {}" to the correct "Tiles upgraded to {}",
keeping the existing interpolation of app_update_info.latest_version (i.e.,
update the string in the code that constructs the Ok(format!(...,
app_update_info.latest_version))).
---
Duplicate comments:
In `@tiles/src/utils/installer.rs`:
- Around line 41-57: try_update currently spawns curl (curl_process) and then
runs sh with the piped stdout but does not check both processes' exit codes, so
failures can be masked; update the logic in try_update to call
curl_process.wait() (or wait_with_output) to capture curl's exit status and
capture the result of the sh command (currently stored in _run_sh_cmd), then
check both Status.success(); if either is false return an Err(anyhow!(...)) with
a clear message including which process failed and its exit code, otherwise
return Ok(()) so success is only returned when both curl_process and the sh
command exit successfully.
- Around line 68-83: The code currently parses latest_vsn with VersionReq::parse
and then uses matches against current_vsn, which treats the latest as a version
requirement/range; instead parse latest_vsn into a semver::Version (use
Version::parse on latest_vsn), compare the two Version values (equality or
greater-than as appropriate) to decide can_update, and populate
UpdateInfo.latest_version/current_version from the Version instances; update
variable names (req_vsn -> latest_version or latest_vsn_parsed) and adjust the
logic in the function containing req_vsn/current_vsn to use Version::parse for
latest_vsn and compare Versions rather than VersionReq::matches.
---
Nitpick comments:
In `@tiles/src/commands/mod.rs`:
- Line 247: The call to try_update(Some(update_info)).await? discards its
returned message; capture its result and surface it to the user by assigning it
to a variable (e.g., let result = try_update(Some(update_info)).await?;) and
then print or log that result (e.g., println!("{}", result) or
process_logger.info!("{}", result)) so the user sees the completion status;
update the code around try_update to use the captured variable instead of
discarding the return value.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
server/backend/mlx_runner.pytiles/Cargo.tomltiles/src/commands/mod.rstiles/src/main.rstiles/src/utils/installer.rs
✅ Files skipped from review due to trivial changes (1)
- server/backend/mlx_runner.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tiles/src/main.rs
- tiles/Cargo.toml
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… feat/ota-updates
madclaws
force-pushed
the
feat/ota-updates
branch
from
8374b5f to
adeae64
Compare
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.
Auto update system for Tiles CLI