From adc106417669520487acee27415a6b54dddf1f92 Mon Sep 17 00:00:00 2001 From: Dione-b Date: Tue, 4 Aug 2026 16:42:29 -0300 Subject: [PATCH 1/3] Identify which CLI an upgrade warning is about What: name the running executable in the upgrade warning; have `doctor` list every `stellar`/`soroban` on PATH with its version, and report which CLI last refreshed the shared version cache. Bound the crates.io request and give the background check a grace period to persist its result. Why: the warning only prints when the latest version exceeds the running one, so a stale cache cannot produce the output in #2464 -- a 25.2.0 binary with a 25.1.0 cache prints nothing. `current_version` is `env!("CARGO_PKG_VERSION")`, so an older install reports its own version while looking like it speaks for the CLI the user thinks they run. The release dates agree: 22.1.0 predates the report by 15 months. Separately, returning from `main` dropped a still-running check, so a fast command never persisted the versions it had just fetched. Known limitations: the grace period is skipped on the error paths that call `process::exit`, and it can add up to 2s to the first command of the day, when the check actually goes to the network. Cache-writer reporting is diagnostic only -- no decision keys off it, so files written before the field existed behave exactly as before. Co-authored-by: Nearx-Labs --- cmd/soroban-cli/src/cli.rs | 29 +++- cmd/soroban-cli/src/commands/doctor.rs | 174 +++++++++++++++++++- cmd/soroban-cli/src/config/upgrade_check.rs | 36 ++++ cmd/soroban-cli/src/upgrade_check.rs | 95 ++++++++++- 4 files changed, 324 insertions(+), 10 deletions(-) diff --git a/cmd/soroban-cli/src/cli.rs b/cmd/soroban-cli/src/cli.rs index 3a1c378eba..3b806b0d87 100644 --- a/cmd/soroban-cli/src/cli.rs +++ b/cmd/soroban-cli/src/cli.rs @@ -77,8 +77,9 @@ pub async fn main() { // Spawn a thread to check if a new version exists. // It depends on logger, so we need to place it after // the code block that initializes the logger. - tokio::spawn(async move { - upgrade_check(root.global_args.quiet).await; + let quiet = root.global_args.quiet; + let upgrade_check_handle = tokio::spawn(async move { + upgrade_check(quiet).await; }); let printer = Print::new(root.global_args.quiet); @@ -106,6 +107,30 @@ pub async fn main() { printer.errorln(format!("error: {e}")); std::process::exit(1); } + + finish_upgrade_check(upgrade_check_handle).await; +} + +// Returning from `main` ends the runtime, so a still-running upgrade check is +// dropped where it stands. For a command that finishes faster than the request +// to crates.io, that meant the fetched versions were never written to the cache +// and the next run started over -- the check could keep re-fetching and keep +// reporting whatever stale versions the cache already held. +// +// Give it a brief chance to land instead. The fetch runs alongside the command, +// so by this point it has usually already finished and this returns +// immediately; the wait only bites when the check actually went to the network, +// which is at most once a day. Dropping it after the grace period is no worse +// than the unconditional drop it replaces. +const UPGRADE_CHECK_GRACE: std::time::Duration = std::time::Duration::from_secs(2); + +async fn finish_upgrade_check(handle: tokio::task::JoinHandle<()>) { + if tokio::time::timeout(UPGRADE_CHECK_GRACE, handle) + .await + .is_err() + { + tracing::debug!("upgrade check did not finish within its grace period"); + } } // Load config.toml defaults as env vars, honoring --config-dir if present in raw args. diff --git a/cmd/soroban-cli/src/commands/doctor.rs b/cmd/soroban-cli/src/commands/doctor.rs index b2d2ea989d..5e9fe7a8aa 100644 --- a/cmd/soroban-cli/src/commands/doctor.rs +++ b/cmd/soroban-cli/src/commands/doctor.rs @@ -2,6 +2,7 @@ use clap::Parser; use rustc_version::version; use semver::Version; use std::fmt::Debug; +use std::path::{Path, PathBuf}; use std::process::Command; use crate::{ @@ -10,10 +11,11 @@ use crate::{ self, data, locator::{self, KeyType}, network::{Network, DEFAULTS as DEFAULT_NETWORKS}, + upgrade_check::UpgradeCheck, }, print::Print, rpc, - upgrade_check::has_available_upgrade, + upgrade_check::{check_performed_by, has_available_upgrade, running_binary, upgrade_message}, utils::url::redact_url, }; @@ -46,7 +48,14 @@ impl Cmd { pub async fn run(&self, _global_args: &global::Args) -> Result<(), Error> { let print = Print::new(false); + // Read this before `check_version`, which refreshes the cache and would + // otherwise record this very run as the writer -- hiding the mismatch + // the report exists to reveal. + let previous_cache_writer = version_cache_writer(); + check_version(&print).await?; + check_installs(&print); + show_version_cache_writer(&print, previous_cache_writer.as_deref()); check_rust_version(&print); check_wasm_target(&print); check_optional_features(&print); @@ -149,9 +158,7 @@ async fn check_version(print: &Print) -> Result<(), Error> { has_available_upgrade(false).await { if upgrade_available { - print.warnln(format!( - "A new release of Stellar CLI is available: {current_version} -> {latest_version}" - )); + print.warnln(upgrade_message(¤t_version, &latest_version)); } else { print.checkln(format!( "You are using the latest version of Stellar CLI: {current_version}" @@ -162,6 +169,130 @@ async fn check_version(print: &Print) -> Result<(), Error> { Ok(()) } +/// Which CLI last refreshed the shared version cache, if it recorded itself. +fn version_cache_writer() -> Option { + UpgradeCheck::load().ok()?.last_checked_by +} + +/// Report which CLI last refreshed the shared version cache. +/// +/// Every install writes the same file, so the entry that decides when the next +/// check happens -- and the versions any warning is built from -- may have come +/// from a different install than the one being run. When it did, say so plainly: +/// that mismatch is the signal worth surfacing. +fn show_version_cache_writer(print: &Print, writer: Option<&str>) { + let Some(writer) = writer else { + // Either no cache yet or one written before the CLI recorded this, so + // there is nothing to report rather than something being wrong. + print.infoln("Version cache was last refreshed by an unknown Stellar CLI".to_string()); + return; + }; + + let this_cli = check_performed_by(); + + if writer == this_cli { + print.checkln(format!("Version cache last refreshed by: {writer}")); + } else { + print.warnln(format!( + "Version cache was last refreshed by a different Stellar CLI: {writer}" + )); + print.blankln(format!("this one is {this_cli}")); + } +} + +/// The binary names this CLI ships under. Both are built from the same crate, +/// so a stale `soroban` runs the same upgrade check as a current `stellar` and +/// reports its own, older version. +const CLI_BINARY_NAMES: [&str; 2] = ["stellar", "soroban"]; + +/// Report the running executable and every other Stellar CLI on `PATH`. +/// +/// Several installs at different versions is the case that makes the upgrade +/// warning look wrong: an old binary correctly reports its own old version, +/// but the user compares it against whichever binary `stellar --version` +/// resolves to and sees a contradiction. Listing them makes that visible. +fn check_installs(print: &Print) { + match running_binary() { + Some(binary) => print.infoln(format!("Running executable: {binary}")), + None => print.warnln("Could not determine the running executable".to_string()), + } + + let mut installs: Vec<(PathBuf, Option)> = Vec::new(); + + for name in CLI_BINARY_NAMES { + let Ok(paths) = which::which_all(name) else { + continue; + }; + + for path in paths { + // `which_all` yields one entry per matching `PATH` element, so the + // same binary shows up repeatedly when `PATH` has duplicates. + let key = path.canonicalize().unwrap_or_else(|_| path.clone()); + if installs.iter().any(|(seen, _)| *seen == key) { + continue; + } + + let version = installed_version(&path); + installs.push((key, version)); + } + } + + if installs.len() <= 1 { + print.checkln("Only one Stellar CLI found on PATH".to_string()); + return; + } + + print.warnln(format!( + "Found {} Stellar CLI executables on PATH; an outdated one can report a \ + version that disagrees with `stellar --version`:", + installs.len() + )); + + for (path, version) in &installs { + let version = version.as_deref().unwrap_or("unknown version"); + print.blankln(format!("- {} ({version})", path.to_string_lossy())); + } +} + +/// Ask a CLI executable for its version. Returns `None` if it cannot be run or +/// does not answer like a Stellar CLI. +fn installed_version(path: &Path) -> Option { + // `--only-version` predates neither every release nor every binary name: + // releases old enough to cause the version confusion this check exists to + // surface reject the flag, so fall back to parsing the full version banner. + run_version(path, &["version", "--only-version"]) + .and_then(|output| parse_only_version(&output)) + .or_else(|| run_version(path, &["--version"]).and_then(|o| parse_version_banner(&o))) +} + +fn run_version(path: &Path, args: &[&str]) -> Option { + let output = Command::new(path).args(args).output().ok()?; + + if output.status.success() { + Some(String::from_utf8_lossy(&output.stdout).into_owned()) + } else { + None + } +} + +fn parse_only_version(output: &str) -> Option { + let version = output.trim(); + + // An older CLI treats `--only-version` as an unknown argument and may still + // exit zero while printing usage, so require a bare version. + Version::parse(version).ok().map(|_| version.to_string()) +} + +/// Pull the version out of a banner like `stellar 22.8.0 (18f54cd...)`. +fn parse_version_banner(output: &str) -> Option { + output + .lines() + .next()? + .split_whitespace() + .find(|token| Version::parse(token).is_ok()) + .map(ToString::to_string) +} + fn check_rust_version(print: &Print) { match version() { Ok(rust_version) => { @@ -266,3 +397,38 @@ fn get_expected_wasm_target() -> String { "wasm32v1-none".into() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_bare_only_version_output() { + assert_eq!(parse_only_version("27.1.0\n").as_deref(), Some("27.1.0")); + } + + #[test] + fn rejects_usage_text_printed_for_an_unknown_flag() { + // A CLI old enough to reject `--only-version` must not be reported as + // if the usage text were its version. + let usage = "error: unexpected argument '--only-version' found\n\n\ + Usage: soroban version [OPTIONS]\n"; + + assert_eq!(parse_only_version(usage), None); + } + + #[test] + fn parses_version_from_an_older_cli_banner() { + // Real output from a 22.8.0 `soroban`, which predates `--only-version`. + let banner = "stellar 22.8.0 (18f54cdc0342726eb13ac14f84e899703d9f180a)\n\ + stellar-xdr 22.1.0 (e139229708)\n\ + xdr curr (529d5176f2)\n"; + + assert_eq!(parse_version_banner(banner).as_deref(), Some("22.8.0")); + } + + #[test] + fn returns_none_for_a_banner_without_a_version() { + assert_eq!(parse_version_banner("not a stellar cli\n"), None); + } +} diff --git a/cmd/soroban-cli/src/config/upgrade_check.rs b/cmd/soroban-cli/src/config/upgrade_check.rs index cfa9b40a05..a165ef31b1 100644 --- a/cmd/soroban-cli/src/config/upgrade_check.rs +++ b/cmd/soroban-cli/src/config/upgrade_check.rs @@ -21,6 +21,19 @@ pub struct UpgradeCheck { pub max_stable_version: Version, /// The latest version of the CLI available on crates.io, including pre-releases. pub max_version: Version, + /// Which CLI last refreshed this file, as `" ()"`. + /// + /// Every install shares this one file, and both the `stellar` and `soroban` + /// binaries are built from the same crate, so the entry that paces the next + /// check may well have been written by a different -- possibly much older -- + /// CLI than the one reading it. Recording the writer makes that visible in + /// `stellar doctor` instead of leaving it to be guessed at. + /// + /// `None` for files written before this field existed. Nothing keys a + /// decision off it: it is diagnostic only, so an absent value cannot change + /// whether an upgrade is reported. + #[serde(default)] + pub last_checked_by: Option, } impl Default for UpgradeCheck { @@ -29,6 +42,7 @@ impl Default for UpgradeCheck { latest_check_time: DateTime::::UNIX_EPOCH, max_stable_version: Version::new(0, 0, 0), max_version: Version::new(0, 0, 0), + last_checked_by: None, } } } @@ -72,6 +86,27 @@ mod tests { use serial_test::serial; use std::env; + /// A file written before `last_checked_by` existed must still load, and the + /// absent field must not disturb any of the values the check reasons about. + #[test] + fn test_loads_legacy_file_without_last_checked_by() { + let legacy = r#"{ + "latest_check_time": "2026-08-04T10:00:00Z", + "max_stable_version": "27.1.0", + "max_version": "27.1.0" + }"#; + + let check: UpgradeCheck = serde_json::from_str(legacy).unwrap(); + + assert_eq!( + check.latest_check_time, + "2026-08-04T10:00:00Z".parse::>().unwrap() + ); + assert_eq!(check.max_stable_version, Version::new(27, 1, 0)); + assert_eq!(check.max_version, Version::new(27, 1, 0)); + assert_eq!(check.last_checked_by, None); + } + #[test] #[serial] fn test_upgrade_check_load_save() { @@ -95,6 +130,7 @@ mod tests { latest_check_time: DateTime::::from_timestamp(1_234_567_890, 0).unwrap(), max_stable_version: Version::new(1, 2, 3), max_version: Version::parse("1.2.4-rc.1").unwrap(), + last_checked_by: Some("1.2.3 (/usr/local/bin/stellar)".to_string()), }; saved_check.save().unwrap(); let loaded_check = UpgradeCheck::load().unwrap(); diff --git a/cmd/soroban-cli/src/upgrade_check.rs b/cmd/soroban-cli/src/upgrade_check.rs index a27e57d327..dcf96a423b 100644 --- a/cmd/soroban-cli/src/upgrade_check.rs +++ b/cmd/soroban-cli/src/upgrade_check.rs @@ -7,7 +7,13 @@ use std::error::Error; use std::io::IsTerminal; use std::time::Duration; -const MINIMUM_CHECK_INTERVAL: Duration = Duration::from_hours(24); // 1 day +// One day. +const MINIMUM_CHECK_INTERVAL: Duration = Duration::from_hours(24); +// The shared HTTP client only bounds how long connecting may take, so a server +// that accepts the connection and then stalls would leave the request hanging +// indefinitely. Bound the whole request: this is a background nicety, and it +// must not be able to outlive the command it is running alongside. +const FETCH_TIMEOUT: Duration = Duration::from_secs(5); const CRATES_IO_API_URL: &str = "https://crates.io/api/v1/crates/"; const NO_UPDATE_CHECK_ENV_VAR: &str = "STELLAR_NO_UPDATE_CHECK"; @@ -25,12 +31,28 @@ struct Crate { max_version: Version, // This is the latest version, including pre-releases } +/// The path of the executable that is running, if it can be resolved. +/// +/// `current_version` comes from `env!("CARGO_PKG_VERSION")`, so it describes +/// the binary that is running and nothing else. When more than one Stellar CLI +/// is installed (a stale `soroban` alongside a current `stellar`, or a Homebrew +/// install shadowed by a `cargo install` one), an old binary reports its own +/// old version and the message reads as though it were about the CLI the user +/// thinks they are running. Naming the executable makes the warning say which +/// install it is actually about. +pub fn running_binary() -> Option { + std::env::current_exe() + .ok() + .map(|path| path.to_string_lossy().into_owned()) +} + /// Fetch the latest stable version of the crate from crates.io async fn fetch_latest_crate_info() -> Result> { let crate_name = env!("CARGO_PKG_NAME"); let url = format!("{CRATES_IO_API_URL}{crate_name}"); let resp = http::client() .get(url) + .timeout(FETCH_TIMEOUT) .send() .await? .json::() @@ -38,6 +60,28 @@ async fn fetch_latest_crate_info() -> Result> { Ok(resp.crate_) } +/// How this CLI identifies itself as the writer of the shared cache file. +pub fn check_performed_by() -> String { + let version = crate::commands::version::pkg(); + + match running_binary() { + Some(binary) => format!("{version} ({binary})"), + None => version.to_string(), + } +} + +/// The upgrade warning, naming the executable it refers to when that can be +/// resolved. +pub fn upgrade_message(current_version: &Version, latest_version: &Version) -> String { + let message = + format!("A new release of Stellar CLI is available: {current_version} -> {latest_version}"); + + match running_binary() { + Some(binary) => format!("{message} ({binary})"), + None => message, + } +} + /// Print a warning if a new version of the CLI is available pub async fn upgrade_check(quiet: bool) { // We should skip the upgrade check if we're not in a tty environment. @@ -55,9 +99,7 @@ pub async fn upgrade_check(quiet: bool) { if let Ok((true, current_version, latest_version)) = has_available_upgrade(true).await { let printer = Print::new(quiet); - printer.warnln(format!( - "A new release of Stellar CLI is available: {current_version} -> {latest_version}" - )); + printer.warnln(upgrade_message(¤t_version, &latest_version)); } tracing::debug!("finished upgrade check"); @@ -82,6 +124,7 @@ pub async fn has_available_upgrade( latest_check_time: now, max_stable_version: c.max_stable_version, max_version: c.max_version, + last_checked_by: Some(check_performed_by()), }; } Err(e) => { @@ -89,6 +132,10 @@ pub async fn has_available_upgrade( // Only update the latest check time if the fetch failed // This way we don't spam the user with errors stats.latest_check_time = now; + // A failed attempt still paces the next one, so record who + // paced it -- otherwise the file credits whichever install + // last succeeded, which may not be the one holding it back. + stats.last_checked_by = Some(check_performed_by()); } } @@ -137,6 +184,7 @@ mod tests { latest_check_time: chrono::Utc::now(), max_stable_version: Version::parse("1.0.0").unwrap(), max_version: Version::parse("1.1.0-rc.1").unwrap(), + last_checked_by: None, }; // When using a non-preview version @@ -155,6 +203,45 @@ mod tests { assert_eq!(*latest_version, Version::parse("1.1.0-rc.1").unwrap()); } + #[test] + fn test_upgrade_message_names_the_running_binary() { + let current = Version::parse("22.1.0").unwrap(); + let latest = Version::parse("23.3.0").unwrap(); + let binary = running_binary().expect("test binary path should resolve"); + + let message = upgrade_message(¤t, &latest); + + assert!( + message.starts_with("A new release of Stellar CLI is available: 22.1.0 -> 23.3.0"), + "unexpected message: {message}" + ); + // Without this, a stale install's warning is indistinguishable from the + // current install's -- the confusion reported in #2464. + assert!( + message.contains(&binary), + "message should name the running binary, got: {message}" + ); + } + + #[test] + fn test_check_performed_by_identifies_version_and_executable() { + let identifier = check_performed_by(); + let binary = running_binary().expect("test binary path should resolve"); + + // `doctor` compares this against the value stored in the shared cache to + // decide whether another install wrote it, so it has to carry both the + // version and the path -- version alone cannot distinguish two installs + // of the same version. + assert!( + identifier.starts_with(crate::commands::version::pkg()), + "should start with the running version: {identifier}" + ); + assert!( + identifier.contains(&binary), + "should name the executable: {identifier}" + ); + } + #[test] fn test_semver_compare() { assert!(Version::parse("0.1.0").unwrap() < Version::parse("0.2.0").unwrap()); From de2f34541ed5040b4e8c4ad5d78fd4ff564f929b Mon Sep 17 00:00:00 2001 From: Dione-b Date: Tue, 4 Aug 2026 17:23:57 -0300 Subject: [PATCH 2/3] Decide CLI identity by path, not by version and path What: record the version-cache writer as separate version and executable fields, canonicalize the executable on both write and read, and compare only the path. Warn about several executables on PATH when their versions disagree rather than when there is more than one, and report finding none. Why: the writer was a single `" ()"` string, so an in-place upgrade -- same path, new version -- read as a different install; the cache is only rewritten once a day, so that warning could repeat on every `doctor` run for up to 24h. The count was misleading in the same way: this crate ships both `stellar` and `soroban`, so one ordinary install puts two files on PATH and was warned about. Zero executables fell into the same branch and reported "Only one Stellar CLI found on PATH". Same path with an earlier version is now informational rather than a warning: it is the ordinary state after an upgrade, and it corrects itself at the next refresh. When either path is unknown there is no identity to compare, so the writer is reported without claiming a match either way. Co-authored-by: Nearx-Labs --- cmd/soroban-cli/src/commands/doctor.rs | 109 +++++++++++++++----- cmd/soroban-cli/src/config/upgrade_check.rs | 66 +++++++++++- cmd/soroban-cli/src/upgrade_check.rs | 44 ++++---- 3 files changed, 168 insertions(+), 51 deletions(-) diff --git a/cmd/soroban-cli/src/commands/doctor.rs b/cmd/soroban-cli/src/commands/doctor.rs index 5e9fe7a8aa..3f7d192c82 100644 --- a/cmd/soroban-cli/src/commands/doctor.rs +++ b/cmd/soroban-cli/src/commands/doctor.rs @@ -11,7 +11,7 @@ use crate::{ self, data, locator::{self, KeyType}, network::{Network, DEFAULTS as DEFAULT_NETWORKS}, - upgrade_check::UpgradeCheck, + upgrade_check::{CheckWriter, UpgradeCheck}, }, print::Print, rpc, @@ -55,7 +55,7 @@ impl Cmd { check_version(&print).await?; check_installs(&print); - show_version_cache_writer(&print, previous_cache_writer.as_deref()); + show_version_cache_writer(&print, previous_cache_writer.as_ref()); check_rust_version(&print); check_wasm_target(&print); check_optional_features(&print); @@ -170,7 +170,7 @@ async fn check_version(print: &Print) -> Result<(), Error> { } /// Which CLI last refreshed the shared version cache, if it recorded itself. -fn version_cache_writer() -> Option { +fn version_cache_writer() -> Option { UpgradeCheck::load().ok()?.last_checked_by } @@ -180,7 +180,12 @@ fn version_cache_writer() -> Option { /// check happens -- and the versions any warning is built from -- may have come /// from a different install than the one being run. When it did, say so plainly: /// that mismatch is the signal worth surfacing. -fn show_version_cache_writer(print: &Print, writer: Option<&str>) { +/// +/// Identity is the executable path, not the recorded version. An in-place +/// upgrade leaves an older version recorded against the very path now running, +/// and that is one install, not two -- treating it as a mismatch would warn +/// about every upgrade until the cache next refreshed. +fn show_version_cache_writer(print: &Print, writer: Option<&CheckWriter>) { let Some(writer) = writer else { // Either no cache yet or one written before the CLI recorded this, so // there is nothing to report rather than something being wrong. @@ -190,13 +195,31 @@ fn show_version_cache_writer(print: &Print, writer: Option<&str>) { let this_cli = check_performed_by(); - if writer == this_cli { - print.checkln(format!("Version cache last refreshed by: {writer}")); - } else { - print.warnln(format!( - "Version cache was last refreshed by a different Stellar CLI: {writer}" - )); - print.blankln(format!("this one is {this_cli}")); + match (&writer.executable, &this_cli.executable) { + (Some(written_by), Some(running)) if written_by == running => { + if writer.version == this_cli.version { + print.checkln(format!("Version cache last refreshed by: {writer}")); + } else { + // Same install, earlier version: the ordinary state after an + // upgrade, and it corrects itself at the next refresh. + let recorded = writer.version.as_deref().unwrap_or("an unknown version"); + let running = this_cli.version.as_deref().unwrap_or("unknown"); + + print.infoln(format!( + "Version cache was last refreshed by this install running {recorded}; \ + it is now {running}" + )); + } + } + (Some(_), Some(_)) => { + print.warnln(format!( + "Version cache was last refreshed by a different Stellar CLI: {writer}" + )); + print.blankln(format!("this one is {this_cli}")); + } + // Without both paths there is no identity to compare, so report the + // writer without claiming it was or was not this install. + _ => print.infoln(format!("Version cache was last refreshed by: {writer}")), } } @@ -207,16 +230,57 @@ const CLI_BINARY_NAMES: [&str; 2] = ["stellar", "soroban"]; /// Report the running executable and every other Stellar CLI on `PATH`. /// -/// Several installs at different versions is the case that makes the upgrade -/// warning look wrong: an old binary correctly reports its own old version, -/// but the user compares it against whichever binary `stellar --version` -/// resolves to and sees a contradiction. Listing them makes that visible. +/// Several executables at *different versions* is the case that makes the +/// upgrade warning look wrong: an old binary correctly reports its own old +/// version, but the user compares it against whichever binary +/// `stellar --version` resolves to and sees a contradiction. Listing them makes +/// that visible. +/// +/// Count alone is not the signal. This crate ships two binaries, `stellar` and +/// `soroban`, so a single healthy install puts two files on `PATH` -- warning +/// about that would flag nearly every install. Warn when the versions actually +/// disagree. fn check_installs(print: &Print) { match running_binary() { Some(binary) => print.infoln(format!("Running executable: {binary}")), None => print.warnln("Could not determine the running executable".to_string()), } + let installs = find_installs(); + + let mut versions = installs.iter().map(|(_, version)| version.as_deref()); + let common_version = match versions.next() { + // Every executable answered, and answered the same: one version is in + // play however many files carry it. + Some(Some(first)) if versions.all(|version| version == Some(first)) => Some(first), + _ => None, + }; + + match (installs.len(), common_version) { + (0, _) => print.warnln( + "No Stellar CLI found on PATH; the running executable is not reachable by name" + .to_string(), + ), + (1, _) => print.checkln("Only one Stellar CLI found on PATH".to_string()), + (count, Some(version)) => { + print.checkln(format!( + "{count} Stellar CLI executables on PATH, all reporting {version}:" + )); + list_installs(print, &installs); + } + (count, None) => { + print.warnln(format!( + "Found {count} Stellar CLI executables on PATH reporting different versions; \ + an outdated one can report a version that disagrees with `stellar --version`:" + )); + list_installs(print, &installs); + } + } +} + +/// Every distinct Stellar CLI executable reachable by name on `PATH`, with the +/// version it reports. +fn find_installs() -> Vec<(PathBuf, Option)> { let mut installs: Vec<(PathBuf, Option)> = Vec::new(); for name in CLI_BINARY_NAMES { @@ -237,18 +301,11 @@ fn check_installs(print: &Print) { } } - if installs.len() <= 1 { - print.checkln("Only one Stellar CLI found on PATH".to_string()); - return; - } - - print.warnln(format!( - "Found {} Stellar CLI executables on PATH; an outdated one can report a \ - version that disagrees with `stellar --version`:", - installs.len() - )); + installs +} - for (path, version) in &installs { +fn list_installs(print: &Print, installs: &[(PathBuf, Option)]) { + for (path, version) in installs { let version = version.as_deref().unwrap_or("unknown version"); print.blankln(format!("- {} ({version})", path.to_string_lossy())); } diff --git a/cmd/soroban-cli/src/config/upgrade_check.rs b/cmd/soroban-cli/src/config/upgrade_check.rs index a165ef31b1..9ef605e65b 100644 --- a/cmd/soroban-cli/src/config/upgrade_check.rs +++ b/cmd/soroban-cli/src/config/upgrade_check.rs @@ -10,6 +10,34 @@ use super::data::project_dir; const FILE_NAME: &str = "upgrade_check.json"; +/// The CLI that last refreshed the shared version cache. +/// +/// Version and executable are kept apart on purpose. The executable is the +/// installation identity: an in-place upgrade changes the version at one path +/// without becoming a second install, so only the path can answer "was this a +/// different CLI?". The version is diagnostic detail carried alongside it. +#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)] +pub struct CheckWriter { + /// The version of the CLI that wrote the file, if it recorded one. + #[serde(default)] + pub version: Option, + /// The canonicalized path of the executable that wrote the file, if it + /// could be resolved. + #[serde(default)] + pub executable: Option, +} + +impl std::fmt::Display for CheckWriter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match (&self.version, &self.executable) { + (Some(version), Some(executable)) => write!(f, "{version} ({executable})"), + (Some(version), None) => write!(f, "{version} (unknown executable)"), + (None, Some(executable)) => write!(f, "unknown version ({executable})"), + (None, None) => write!(f, "an unidentified Stellar CLI"), + } + } +} + /// The `UpgradeCheck` struct represents the state of the upgrade check. /// This state is global and stored in the `upgrade_check.json` file in /// the global configuration directory. @@ -21,7 +49,7 @@ pub struct UpgradeCheck { pub max_stable_version: Version, /// The latest version of the CLI available on crates.io, including pre-releases. pub max_version: Version, - /// Which CLI last refreshed this file, as `" ()"`. + /// Which CLI last refreshed this file. /// /// Every install shares this one file, and both the `stellar` and `soroban` /// binaries are built from the same crate, so the entry that paces the next @@ -33,7 +61,7 @@ pub struct UpgradeCheck { /// decision off it: it is diagnostic only, so an absent value cannot change /// whether an upgrade is reported. #[serde(default)] - pub last_checked_by: Option, + pub last_checked_by: Option, } impl Default for UpgradeCheck { @@ -107,6 +135,35 @@ mod tests { assert_eq!(check.last_checked_by, None); } + /// The writer is diagnostic, so a file that recorded only part of it has to + /// load rather than fail the whole check. + #[test] + fn test_loads_partially_recorded_writer() { + let partial = r#"{ + "latest_check_time": "2026-08-04T10:00:00Z", + "max_stable_version": "27.1.0", + "max_version": "27.1.0", + "last_checked_by": { "version": "27.1.0" } + }"#; + + let check: UpgradeCheck = serde_json::from_str(partial).unwrap(); + let writer = check.last_checked_by.unwrap(); + + assert_eq!(writer.version.as_deref(), Some("27.1.0")); + assert_eq!(writer.executable, None); + assert_eq!(writer.to_string(), "27.1.0 (unknown executable)"); + } + + #[test] + fn test_writer_displays_version_and_executable() { + let writer = CheckWriter { + version: Some("27.1.0".to_string()), + executable: Some("/usr/local/bin/stellar".to_string()), + }; + + assert_eq!(writer.to_string(), "27.1.0 (/usr/local/bin/stellar)"); + } + #[test] #[serial] fn test_upgrade_check_load_save() { @@ -130,7 +187,10 @@ mod tests { latest_check_time: DateTime::::from_timestamp(1_234_567_890, 0).unwrap(), max_stable_version: Version::new(1, 2, 3), max_version: Version::parse("1.2.4-rc.1").unwrap(), - last_checked_by: Some("1.2.3 (/usr/local/bin/stellar)".to_string()), + last_checked_by: Some(CheckWriter { + version: Some("1.2.3".to_string()), + executable: Some("/usr/local/bin/stellar".to_string()), + }), }; saved_check.save().unwrap(); let loaded_check = UpgradeCheck::load().unwrap(); diff --git a/cmd/soroban-cli/src/upgrade_check.rs b/cmd/soroban-cli/src/upgrade_check.rs index dcf96a423b..8557d98064 100644 --- a/cmd/soroban-cli/src/upgrade_check.rs +++ b/cmd/soroban-cli/src/upgrade_check.rs @@ -1,4 +1,4 @@ -use crate::config::upgrade_check::UpgradeCheck; +use crate::config::upgrade_check::{CheckWriter, UpgradeCheck}; use crate::print::Print; use crate::utils::http; use semver::Version; @@ -40,10 +40,14 @@ struct Crate { /// old version and the message reads as though it were about the CLI the user /// thinks they are running. Naming the executable makes the warning say which /// install it is actually about. +/// +/// Canonicalized, so that the value written to the shared cache and the value +/// compared against it later describe the same path in the same form. pub fn running_binary() -> Option { - std::env::current_exe() - .ok() - .map(|path| path.to_string_lossy().into_owned()) + let path = std::env::current_exe().ok()?; + let path = path.canonicalize().unwrap_or(path); + + Some(path.to_string_lossy().into_owned()) } /// Fetch the latest stable version of the crate from crates.io @@ -61,12 +65,10 @@ async fn fetch_latest_crate_info() -> Result> { } /// How this CLI identifies itself as the writer of the shared cache file. -pub fn check_performed_by() -> String { - let version = crate::commands::version::pkg(); - - match running_binary() { - Some(binary) => format!("{version} ({binary})"), - None => version.to_string(), +pub fn check_performed_by() -> CheckWriter { + CheckWriter { + version: Some(crate::commands::version::pkg().to_string()), + executable: running_binary(), } } @@ -225,21 +227,19 @@ mod tests { #[test] fn test_check_performed_by_identifies_version_and_executable() { - let identifier = check_performed_by(); + let writer = check_performed_by(); let binary = running_binary().expect("test binary path should resolve"); - // `doctor` compares this against the value stored in the shared cache to - // decide whether another install wrote it, so it has to carry both the - // version and the path -- version alone cannot distinguish two installs - // of the same version. - assert!( - identifier.starts_with(crate::commands::version::pkg()), - "should start with the running version: {identifier}" - ); - assert!( - identifier.contains(&binary), - "should name the executable: {identifier}" + // `doctor` compares the executable against the one stored in the shared + // cache to decide whether another *install* wrote it, and reports the + // version alongside it. They are recorded separately so that an in-place + // upgrade -- same path, new version -- is not mistaken for a second + // install. + assert_eq!( + writer.version.as_deref(), + Some(crate::commands::version::pkg()) ); + assert_eq!(writer.executable.as_deref(), Some(binary.as_str())); } #[test] From 2832125189c3b30863d04c8d75ca2e4cc587ef9c Mon Sep 17 00:00:00 2001 From: Dione-b Date: Tue, 4 Aug 2026 17:24:07 -0300 Subject: [PATCH 3/3] Cover doctor's install and cache-writer reporting What: add `soroban-test` integration coverage for zero, one, two agreeing and two disagreeing Stellar CLIs on PATH, and for cache writers that are this install, this install at an earlier version, a different install, and absent. Why: the externally visible behavior was covered only by parser unit tests, so nothing exercised the subprocess probing or the messages themselves -- both false positives fixed in the previous commit would have been caught here. PATH comes from fake CLIs in a temp dir, as in `plugin.rs`, and the version cache from `STELLAR_DATA_HOME`, so no production code had to change to make the inputs injectable. One fake rejects `version --only-version` so the `--version` banner fallback runs through a real subprocess. Unix only: the fake CLIs are shell scripts needing an execute bit. Assertions are on stderr, where `Print` writes. The seeded cache writer survives the run because `doctor` reads it before `has_available_upgrade` can overwrite it. Co-authored-by: Nearx-Labs --- cmd/crates/soroban-test/tests/it/doctor.rs | 236 +++++++++++++++++++++ cmd/crates/soroban-test/tests/it/main.rs | 2 + 2 files changed, 238 insertions(+) create mode 100644 cmd/crates/soroban-test/tests/it/doctor.rs diff --git a/cmd/crates/soroban-test/tests/it/doctor.rs b/cmd/crates/soroban-test/tests/it/doctor.rs new file mode 100644 index 0000000000..0d4cec02f0 --- /dev/null +++ b/cmd/crates/soroban-test/tests/it/doctor.rs @@ -0,0 +1,236 @@ +/* +`doctor` reports on things it discovers outside the process: which Stellar CLI +executables are on `PATH`, and which CLI last refreshed the shared version +cache. Both inputs are injectable -- `PATH` like in `plugin.rs`, and the cache +via `STELLAR_DATA_HOME` -- so the reporting can be exercised end to end. + +Unix only: the fake CLIs are shell scripts that need an execute bit. +*/ + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; + +use assert_cmd::Command; +use predicates::prelude::PredicateBooleanExt; +use predicates::str::contains; +use soroban_cli::commands::version::pkg; +use soroban_test::TestEnv; + +/// A stand-in CLI on `PATH` that answers version queries and nothing else. +/// +/// `supports_only_version` distinguishes a current CLI from one old enough to +/// reject `version --only-version` -- the releases that cause the version +/// confusion these checks exist to surface only answer `--version`. +fn write_fake_cli(dir: &Path, name: &str, version: &str, supports_only_version: bool) { + let only_version = if supports_only_version { + format!("echo \"{version}\"") + } else { + "echo \"error: unexpected argument '--only-version' found\" >&2; exit 2".to_string() + }; + + let script = format!( + "#!/bin/sh\n\ + case \"$*\" in\n\ + \"version --only-version\") {only_version} ;;\n\ + \"--version\") echo \"stellar {version} (0000000000000000000000000000000000000000)\" ;;\n\ + *) echo \"unexpected args: $*\" >&2; exit 1 ;;\n\ + esac\n" + ); + + let path = dir.join(name); + fs::write(&path, script).unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap(); +} + +fn empty_dir(sandbox: &TestEnv, name: &str) -> PathBuf { + let dir = sandbox.dir().join(name); + fs::create_dir_all(&dir).unwrap(); + dir +} + +/// The path `doctor` sees as its own executable, in the canonicalized form it +/// compares cache entries against. +fn running_binary() -> String { + let path = assert_cmd::cargo::cargo_bin("stellar"); + let path = path.canonicalize().unwrap_or(path); + path.to_string_lossy().into_owned() +} + +/// Seed the shared version cache with a given writer, and return the data home +/// that holds it. +/// +/// `latest_check_time` is irrelevant to what is asserted here: `doctor` calls +/// `has_available_upgrade` with caching off, so it always attempts a refresh. +/// The seeded writer is still what gets reported, because `doctor` reads it +/// before that refresh can overwrite it. +fn seed_cache_writer(sandbox: &TestEnv, writer: serde_json::Value) -> PathBuf { + let data_home = empty_dir(sandbox, "cache-data-home"); + + let cache = serde_json::json!({ + "latest_check_time": "2026-08-04T10:00:00Z", + "max_stable_version": "27.1.0", + "max_version": "27.1.0", + "last_checked_by": writer, + }); + + fs::write( + data_home.join("upgrade_check.json"), + serde_json::to_string(&cache).unwrap(), + ) + .unwrap(); + + data_home +} + +/// `doctor` with `PATH` and the version cache pointed at test-controlled state. +fn doctor(sandbox: &TestEnv, path: &Path, data_home: &Path) -> Command { + let mut cmd = sandbox.new_assert_cmd("doctor"); + cmd.env("PATH", path).env("STELLAR_DATA_HOME", data_home); + cmd +} + +#[test] +fn reports_the_running_executable_and_a_lone_install() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "one-install"); + let data_home = empty_dir(&sandbox, "data-home"); + write_fake_cli(&bin_dir, "stellar", "27.1.0", true); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains(format!("Running executable: {}", running_binary()))) + .stderr(contains("Only one Stellar CLI found on PATH")); +} + +#[test] +fn reports_when_no_cli_is_on_path() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "no-installs"); + let data_home = empty_dir(&sandbox, "data-home"); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains("No Stellar CLI found on PATH")) + // Nothing was found, so claiming a single install would be a lie. + .stderr(contains("Only one Stellar CLI").not()); +} + +#[test] +fn does_not_warn_when_both_binary_names_report_the_same_version() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "two-installs-agreeing"); + let data_home = empty_dir(&sandbox, "data-home"); + // What an ordinary install looks like: one crate, two binary names. + write_fake_cli(&bin_dir, "stellar", "27.1.0", true); + write_fake_cli(&bin_dir, "soroban", "27.1.0", true); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "2 Stellar CLI executables on PATH, all reporting 27.1.0", + )) + .stderr(contains("different versions").not()); +} + +#[test] +fn warns_when_installs_report_different_versions() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "two-installs-disagreeing"); + let data_home = empty_dir(&sandbox, "data-home"); + write_fake_cli(&bin_dir, "stellar", "27.1.0", true); + // Old enough to only answer `--version`, which is the fallback path. + write_fake_cli(&bin_dir, "soroban", "22.8.0", false); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "Found 2 Stellar CLI executables on PATH reporting different versions", + )) + .stderr(contains(format!( + "- {} (27.1.0)", + bin_dir.join("stellar").to_string_lossy() + ))) + .stderr(contains(format!( + "- {} (22.8.0)", + bin_dir.join("soroban").to_string_lossy() + ))); +} + +#[test] +fn confirms_the_cache_writer_when_it_is_this_cli() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + let data_home = seed_cache_writer( + &sandbox, + serde_json::json!({ "version": pkg(), "executable": running_binary() }), + ); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains(format!( + "Version cache last refreshed by: {} ({})", + pkg(), + running_binary() + ))); +} + +#[test] +fn does_not_warn_when_the_same_install_wrote_the_cache_at_an_older_version() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + // An in-place upgrade: same path, earlier version recorded against it. + let data_home = seed_cache_writer( + &sandbox, + serde_json::json!({ "version": "26.1.0", "executable": running_binary() }), + ); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains(format!( + "Version cache was last refreshed by this install running 26.1.0; it is now {}", + pkg() + ))) + .stderr(contains("a different Stellar CLI").not()); +} + +#[test] +fn warns_when_a_different_install_wrote_the_cache() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + let data_home = seed_cache_writer( + &sandbox, + serde_json::json!({ "version": "22.8.0", "executable": "/opt/elsewhere/bin/soroban" }), + ); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "Version cache was last refreshed by a different Stellar CLI: \ + 22.8.0 (/opt/elsewhere/bin/soroban)", + )) + .stderr(contains(format!("this one is {} (", pkg()))); +} + +#[test] +fn reports_an_unknown_cache_writer_without_claiming_a_mismatch() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + // A cache written before the writer was recorded. + let data_home = seed_cache_writer(&sandbox, serde_json::Value::Null); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "Version cache was last refreshed by an unknown Stellar CLI", + )) + .stderr(contains("a different Stellar CLI").not()); +} diff --git a/cmd/crates/soroban-test/tests/it/main.rs b/cmd/crates/soroban-test/tests/it/main.rs index 61b62b6b6b..cd666961a1 100644 --- a/cmd/crates/soroban-test/tests/it/main.rs +++ b/cmd/crates/soroban-test/tests/it/main.rs @@ -2,6 +2,8 @@ mod build; mod config; #[cfg(unix)] mod container; +#[cfg(unix)] +mod doctor; #[cfg(feature = "emulator-tests")] mod emulator; mod help;