Report only what doctor's checks actually establish

[Dione-b]

Motivation

Copilot's review of the upstream PR (stellar#2670) flagged three places where the new diagnostics claim more than the data behind them supports. All three are real, and each one can send a user chasing a cause that was never observed:

1. upgrade_check.rs records last_checked_by even when the fetch to crates.io failed — the attempt still paces the next check — but doctor reported the file as "last refreshed by" that install. That credits an install with version data it never fetched.
2. The single-install branch of check_installs printed a count without calling list_installs, so the one case where a listing would settle "which executable is this?" was the one case that omitted path and version. The separate Running executable line is not necessarily the entry PATH resolves by name, and carries no version.
3. common_version == None meant either "known versions disagree" or "at least one executable could not be asked". Both landed on a message blaming differing versions, so two unrunnable binaries — both listed as unknown version — were reported as a version conflict.

A follow-up review then found the fix for (3) committing a smaller version of the same error twice, plus a platform bug in the new tests. Both are addressed here; see Follow-up review below.

Groundwork for stellar#2464.

Behavior

doctor says "checked", not "refreshed". The cache writer is the last writer/pacer, not the source of the versions stored beside it. Kept that semantics — it is the one that answers "which install is holding the next check back?" — and made the wording match the field name (last_checked_by), in the four doctor messages and in the field/struct docs.

Alternative discarded: recording the writer only after a successful refresh. That would make "refreshed by" literally true, but it loses the diagnostic that matters. When a stale install fails its fetch and stamps the file, it still suppresses everyone else's check for 24h; dropping its identity would leave the file crediting whichever install last succeeded, hiding exactly the install a user needs to find.

Every discovered install is listed. list_installs now runs for any non-zero count, including one.

Undetermined agreement is no longer reported as disagreement, and neither report speaks for an executable that was never heard from. InstalledVersions is now:

enum InstalledVersions {
    Agreed(String),
    Disagree { unanswered: usize },
    Unanswered { agreed: Option<String>, unanswered: usize },
}

Situation	Report
All answered, all the same	✅ N Stellar CLI executables on PATH, all reporting X:
Known versions differ, all answered	⚠️ Found N Stellar CLI executables on PATH reporting different versions; an outdated one can report a version that disagrees with 'stellar --version':
Known versions differ, some unanswered	⚠️ Found N Stellar CLI executables on PATH; the K that reported a version do not agree (M could not be asked); an outdated one can report a version that disagrees with 'stellar --version':
Those that answered agree, some unanswered	⚠️ Found N Stellar CLI executables on PATH; every one that answered reports X, but M could not be asked, so a differing version cannot be ruled out:
None answered	⚠️ Found N Stellar CLI executables on PATH; none of them reported a version, so whether they agree could not be determined:

An observed disagreement still wins when it sits alongside a failed probe — a contradiction between two known versions is a fact, and a third unrunnable binary does not soften it. It does not join it either: only the executables that answered are counted as disagreeing.

Every count now sits next to what it counts, so each sentence can be checked against the listing printed directly beneath it.

Follow-up review

Three items from review of this branch, all fixed:

1. Three integration tests failed on macOS (blocking). find_installs canonicalizes every executable it discovers, but the assertions built their expected string from the raw sandbox directory. On macOS the temp dir sits under /var/folders, a symlink to /private/var, so the two spellings never compare equal. warns_when_installs_report_different_versions already failed before this branch; the two path assertions added since inherited the same flaw and turned one failure into three. empty_dir now canonicalizes, with a doc comment recording why.

2. Disagree over-claimed in exactly the way this PR exists to fix. Two executables reporting different versions beside a third that could not be run printed "Found 3 ... reporting different versions", contradicted by the listing right below it, where the third reads (unknown version). Now carries unanswered and names only the ones that were heard from.

3. Unanswered discarded the agreement among those that did answer. Two installs at 27.1.0 beside one that cannot run is a machine whose reachable installs agree — the most useful fact available, and the message dropped it. Now carries that version and leads with it, while still declining to call the whole set agreed.

Wording note on (3): the review suggested "the N that reported a version agree on X". That breaks grammatically when only one answered ("the 1 that reported a version agree on 27.1.0"), and that case is reachable, so the message reads "every one that answered reports X" instead — correct for any count without a second branch.

Also from that review: the test helper now takes impl AsRef<OsStr> rather than &Path, because a multi-entry PATH is several paths joined by : and is not a path; and the redundant return in the zero-install arm is gone, since list_installs prints nothing for an empty slice.

Tests

Unit (cmd/soroban-cli/src/commands/doctor.rs) — five cases for summarize_versions: agreement, distinct known versions, probes that returned nothing (both all-unknown and one-unknown), an agreement surviving a failed probe beside it, and a disagreement coexisting with a failed probe (asserting the unanswered one is counted apart).

Integration (cmd/crates/soroban-test/tests/it/doctor.rs) — three end-to-end cases driving real subprocesses through a fake PATH:

• does_not_blame_differing_versions_when_a_version_could_not_be_read — two unrunnable CLIs; asserts the undetermined message and that different versions is absent.
• reports_a_disagreement_even_when_another_executable_is_unreadable — 27.1.0 + 22.8.0 + one unrunnable across two PATH entries; asserts the disagreement surfaces and that only the two that answered are counted.
• reports_the_agreement_among_the_executables_that_answered — two at 27.1.0 + one unrunnable; asserts the agreement survives.

Also extended reports_the_running_executable_and_a_lone_install to assert the lone install is listed with its version, and updated the cache-writer assertions to the new wording. Helper write_unrunnable_cli writes a script that fails both version queries.

Runs — Linux only

x86_64 Linux, rustc 1.97.1 stable. macOS and Windows were not run; see the caveat below.

Command	Result
cargo fmt --all -- --check	clean
cargo clippy --all-targets (and --features additional-libs)	no warnings
cargo test -p soroban-cli --lib (and --features additional-libs)	333 passed
cargo test -p soroban-test --test it -- doctor:: version:: plugin:: help:: message::	32 passed
cargo test -p soroban-test -- --skip integration::	135 passed, 1 ignored
cargo test -p soroban-test --test it doctor:: with TMPDIR symlinked	11 passed
cargo test --workspace --exclude soroban-test (and --features additional-libs)	486 passed, 2 ignored

Counts are totals across every test target the command builds, so they are higher than the single test result line you may notice scrolling past.

Not run: make rpc-test (--features it -- integration), which needs a local RPC — nothing here touches RPC paths.

The macOS fix is not verified on macOS. I have no Mac. What I did instead was reproduce the mechanism on Linux by forcing TMPDIR through a symlink, which is what /var → /private/var amounts to:

mkdir -p /tmp/repro/private/real && ln -sfn /tmp/repro/private/real /tmp/repro/link
TMPDIR=/tmp/repro/link cargo test -p soroban-test --test it doctor::

Without the canonicalize that fails exactly the three tests reported on macOS — same names, no more, no fewer — and those three are precisely the ones that build an expected string from the sandbox path. With it, all 11 pass. That establishes the mechanism, not the platform: if something else macOS-specific were also contributing, this would not catch it.

Worth noting that CI will not close the gap either. build-and-test-macos in .github/workflows/rust.yml is gated on:

if: github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/release/')
    || startsWith(github.head_ref, 'release/')

so a PR from a fix/* branch never triggers it — which is why the pre-existing macOS failure went unnoticed in the first place, and why this fix will only be exercised once it lands on main. A confirmation run on a Mac before merge would be valuable.

Note for reviewers running these locally: the integration tests execute target/debug/stellar and do not rebuild it, so cargo build --bin stellar is needed first or the assertions run against a stale binary.

Release Impact

No breaking changes. No data migration: this change does not alter the upgrade_check.json shape — last_checked_by and its optional fields are unchanged, and files written before it existed still load.

User-visible surface is stellar doctor's stderr diagnostics only: four cache-writer lines reworded from "refreshed" to "checked", new messages for the undetermined and partially-unanswered cases, and the single-install case now followed by a listing. No command, flag, or help text changed, so FULL_HELP_DOCS.md is untouched. Anything matching on doctor's exact stderr text would need updating — no such consumer is known in this repo beyond the tests updated here.

Checklist

• Public and internal documentation updated to reflect behavior changes. — doc comments on CheckWriter, UpgradeCheck::last_checked_by, show_version_cache_writer, check_installs, InstalledVersions and summarize_versions now state that the writer paces the next check rather than vouching for the recorded versions, and that a failed probe is counted apart from an observed disagreement. No user-facing docs changed: no CLI surface change, so FULL_HELP_DOCS.md is unaffected.
• Error messages and failure codes reviewed or updated to cover new scenarios. — this change is that review; the unreadable-version case gained its own messages instead of borrowing the disagreement one. No exit codes changed; doctor remains diagnostic-only.
• Command examples and tutorials use isolated data or local test environments, adhering to security best practices. — tests inject PATH from a per-test temp dir and the cache via STELLAR_DATA_HOME; nothing touches the real config/data dirs, the network, or any key material.
• Dependencies, lockfiles, and CI/CD workflows are synchronized with the new changes. — no dependency, Cargo.lock or workflow change; the new tests live in the existing soroban-test it target already run by CI.
• All automated tests and linting checks pass successfully. — see the table above, with its stated platform limits.

[pedro-pelicioni]

I cloned the fork, read the code, ran the tests and reproduced doctor by hand.

What checks out

The diagnosis behind all three items is correct, and I verified the first one at the source: in cmd/soroban-cli/src/upgrade_check.rs:132-141 the failed-fetch branch sets latest_check_time and last_checked_by without touching max_stable_version / max_version. So "refreshed" really was false and "checked" is the right word.

It also lines up with what Copilot raised: the 3 suppressed comments on the 2026-08-05 review of stellar/stellar-cli#2670 are exactly these three items.

What I ran locally:

Command	Result
cargo test -p soroban-cli --lib doctor::	8 passed (324 filtered, so the 332 in the description adds up)
cargo fmt --all -- --check	clean
cargo clippy -p soroban-cli --lib --all-features	clean

The summarize_versions logic is correct. Unanswered(0) is only reachable for an empty list, which the (0, _) arm already handles, so no unreachable-looking state escapes.

Issues

1. Three integration tests fail on macOS

Not a theory, this is the actual run on this branch:

test result: FAILED. 7 passed; 3 failed
    doctor::does_not_blame_differing_versions_when_a_version_could_not_be_read
    doctor::reports_the_running_executable_and_a_lone_install
    doctor::warns_when_installs_report_different_versions

find_installs canonicalizes the path (cmd/soroban-cli/src/commands/doctor.rs:345), but the assertions build the expected string from the raw bin_dir. On macOS the temp dir lands under /var/folders/..., which is a symlink to /private/var/..., so the string comparison never matches.

warns_when_installs_report_different_versions already failed before this PR, but the two new path assertions inherited the same flaw and turned 1 failure into 3. One-line fix in empty_dir, which I tested and it takes all 10 to green:

fn empty_dir(sandbox: &TestEnv, name: &str) -> PathBuf {
    let dir = sandbox.dir().join(name);
    fs::create_dir_all(&dir).unwrap();
    dir.canonicalize().unwrap_or(dir)
}

Worth fixing the "Full run, all green" line in the description too, or at least saying which platform it was run on.

2. The Disagree message overreaches in exactly the way this PR is fixing

Running the scenario from the new test by hand:

⚠️  Found 3 Stellar CLI executables on PATH reporting different versions; ...
    - .../a/stellar (27.1.0)
    - .../b/stellar (unknown version)
    - .../a/soroban (22.8.0)

Three executables did not report different versions. Two reported and disagree, the third reported nothing, and the listing right below says so. That is the same class of over-claim the PR exists to kill, just smaller, and reports_a_disagreement_even_when_another_executable_is_unreadable pins that sentence as expected behavior.

The underlying call (an observed disagreement outranks a failed probe) is right. What is missing is keeping the unanswered ones out of the count:

Disagree { unanswered: usize },

with the sentence becoming something like "Found 3 ...; the 2 that reported a version do not agree (1 could not be asked)".

3. Unanswered throws away the agreement among those that did answer

Two binaries at 27.1.0 plus one that cannot run:

⚠️  Found 3 Stellar CLI executables on PATH, 1 of which did not report a version, so whether they agree could not be determined:

The two that answered agree, and that is the most useful fact on the line. It is gone. Same shape of fix:

Unanswered { agreed: Option<String>, unanswered: usize },

Nits

• doctor(&sandbox, Path::new(&path), &data_home) in cmd/crates/soroban-test/tests/it/doctor.rs:222: "dir1:dir2" is not a Path. It works because it becomes an OsStr in env(), but now that there is a test with a multi-entry PATH, the helper should take impl AsRef<OsStr>.
• The return in the (0, _) arm of check_installs is redundant, list_installs on an empty slice prints nothing. It breaks the symmetry with the other arms for no gain.

Left out of scope

Copilot's subprocess-timeout comment (doctor.rs:326 on the upstream PR) was never actually answered. Going by the API threading, the reply hanging off it (in_reply_to_id=3715688791) talks about integration tests, not about the timeout. run_version still uses Command::output() with no bound, so a hung binary on PATH hangs doctor forever.

It is a bit ironic on this PR specifically: it builds a whole taxonomy around "did not answer", and the case that genuinely never answers is the one that wedges the command. Separate scope, but worth opening an issue or replying on that thread rather than leaving it looking resolved.

---

Overall a good, well-argued PR. Item 1 is blocking (red tests); I would fix 2 and 3 before sending this upstream, since they are the PR's own subject matter.

[Dione-b]

All three are addressed, in two commits kept separate so each is green on its own:

• 3f3d65b8 — the test-only platform fix (item 1 and the AsRef<OsStr> nit)
• ce5f847f — the two over-claims (items 2 and 3, plus the redundant return)

1. The macOS path assertions

Taken as proposed. empty_dir now canonicalizes, and I added a doc comment recording why, since the absence of one is what let the same trap through twice:

/// A directory under the sandbox, in the same canonicalized form `doctor`
/// reports paths in.
///
/// `find_installs` canonicalizes every executable it discovers, so an expected
/// path built from an uncanonicalized sandbox never matches: on macOS the
/// temporary directory sits under `/var/folders`, a symlink to `/private/var`,
/// and the two spellings compare unequal as strings.

I did not want to take the fix on trust, so I reproduced the mechanism on Linux by forcing TMPDIR through a symlink — TestEnv uses tempfile, which honors it:

mkdir -p /tmp/repro/private/real && ln -sfn /tmp/repro/private/real /tmp/repro/link
TMPDIR=/tmp/repro/link cargo test -p soroban-test --test it doctor::

Without the canonicalize, that fails exactly the three you listed — same names, nothing more, nothing less. With it, all 11 pass. It also lines up with the cause: those three are precisely the tests that build an expected string from the sandbox path (bin_dir.join(...)), and no other test in the file does.

To be clear about what that is worth: it reproduces the mechanism, not the platform. If something else macOS-specific were also contributing, this would not catch it. Which brings me to the ask below.

2. Disagree overreaching

Fixed as suggested, with your wording:

before: Found 3 Stellar CLI executables on PATH reporting different versions; ...
after:  Found 3 Stellar CLI executables on PATH; the 2 that reported a version do not
        agree (1 could not be asked); an outdated one can report a version that
        disagrees with `stellar --version`:

Disagree { unanswered: usize }. With no failed probes the original sentence is kept verbatim, so the common case reads exactly as before.

3. Unanswered discarding the agreement

Unanswered { agreed: Option<String>, unanswered: usize }, as you proposed. Two shapes now:

Found 3 Stellar CLI executables on PATH; every one that answered reports 27.1.0,
but 1 could not be asked, so a differing version cannot be ruled out:

Found 2 Stellar CLI executables on PATH; none of them reported a version, so
whether they agree could not be determined:

One deviation worth flagging: I went with "every one that answered reports X" rather than "the N that reported a version agree on X". The latter breaks grammatically when only one answered — "the 1 that reported a version agree on 27.1.0" — and that case is reachable (two installs, one unrunnable). The chosen phrasing holds for any count without a second branch.

Every count now sits next to what it counts, so each sentence can be checked against the listing printed directly below it.

Nits

Both taken. The helper is path: impl AsRef<OsStr> with a note that it is a whole PATH value rather than a path, and the return in the (0, _) arm is gone.

The ask

Could you re-run cargo test -p soroban-test --test it doctor:: on your Mac? I have no macOS here, and CI will not close the gap either — build-and-test-macos in .github/workflows/rust.yml is gated on:

if: github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/release/')
    || startsWith(github.head_ref, 'release/')

so a PR from a fix/* branch never triggers it. That is why the pre-existing failure in warns_when_installs_report_different_versions survived unnoticed, and it means this fix will not be verified by CI on this PR — only once it lands on main. Your machine is the only place it can be confirmed before merge. (Whether that gating is right is a separate question, but it does mean the macOS suite is effectively untested on every PR.)

One trap if you do: cargo test -p soroban-test does not rebuild target/debug/stellar. The integration tests exec whatever binary is already there, so without a cargo build --bin stellar first you are testing the old one. That cost me three phantom failures before I noticed.

Left out of scope: the subprocess timeout

You are right that the thread reads as resolved when it was not — the reply hanging off it answers a different question, and run_version still calls Command::output() with no bound, so a hung binary on PATH wedges doctor indefinitely. I have not touched it here; it is a real behavioral change and deserves its own PR rather than riding along on a wording fix. I will open an issue and link it from that thread so it stops looking closed.

The irony is noted, and fair.

Local runs

Command	Result
cargo test -p soroban-cli --lib doctor::	9 passed (was 8; one added)
cargo test -p soroban-test --test it doctor::	11 passed (was 10; one added)
same, with TMPDIR symlinked	11 passed
3f3d65b8 alone, symlinked TMPDIR	10 passed
cargo fmt --all -- --check	clean
cargo clippy -p soroban-cli --lib --all-features	clean

Scoped to doctor:: above; the full suite is in the description, re-measured from a clean build and now including cargo test --workspace --exclude soroban-test --features additional-libs, which had never been run on this branch. All green.

Linux only — that is the correction to the "Full run, all green" line, which the description now carries, along with the platform it was run on.