Add aspire --info to enumerate the running CLI's install + discovered installs + hives#17461
Open
radical wants to merge 18 commits into
Open
Add aspire --info to enumerate the running CLI's install + discovered installs + hives#17461radical wants to merge 18 commits into
aspire --info to enumerate the running CLI's install + discovered installs + hives#17461radical wants to merge 18 commits into
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17461Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17461" |
This comment was marked as outdated.
This comment was marked as outdated.
radical
force-pushed
the
radical/installs-discovery
branch
from
53d1e36 to
102af0d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR splits Aspire CLI installation enumeration out of aspire doctor into a dedicated aspire installs command, leaving doctor focused on environment checks only. It also completes a broad terminology rename from “install route” to “install source” across CLI code, scripts, tests, and docs.
Changes:
- Add new
aspire installscommand (including hidden--self --format jsonpeer-probe surface) and supporting hive enumeration. - Remove installation-table output and
--selfsupport fromaspire doctor, updating JSON contracts and resource strings accordingly. - Hoist WinGet first-run sidecar stamping into CLI startup (
CliExecutionContextfactory) and fix Windows PATH casing preservation inPathLookupHelper.
Reviewed changes
Copilot reviewed 74 out of 75 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/PathLookupHelperTests.cs | Adds Windows-only regression test for preserving filesystem casing when resolving PATHEXT hits. |
| tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs | Registers InstallsCommand and HiveEnumerator in the CLI test DI container. |
| tests/Aspire.Cli.Tests/Utils/CliPathHelperTests.cs | Renames route→source helpers/tests for Aspire home directory derivation. |
| tests/Aspire.Cli.Tests/Packaging/TemporaryNuGetConfigTests.cs | Updates cross-reference to renamed macOS firmlink test name. |
| tests/Aspire.Cli.Tests/Configuration/DotNetBasedAppHostServerChannelResolutionTests.cs | Updates terminology in test doc comment (route→source). |
| tests/Aspire.Cli.Tests/Commands/SetupCommandTests.cs | Renames route→source in test names/comments for setup default path behavior. |
| tests/Aspire.Cli.Tests/Commands/InstallsCommandTests.cs | New unit tests covering aspire installs (list output and hidden --self JSON). |
| tests/Aspire.Cli.Tests/Commands/DoctorCommandTests.cs | Updates doctor tests to assert installations are no longer included and --self is rejected. |
| tests/Aspire.Cli.Tests/BundleServiceComputeDefaultExtractDirTests.cs | Updates comments/terminology for bundle extraction layout selection (route→source). |
| tests/Aspire.Cli.Tests/Bundles/BundleServiceCrossSourceExtractionTests.cs | Renames cross-route extraction matrix test to cross-source and updates homebrew naming. |
| tests/Aspire.Cli.Tests/Acquisition/WingetStartupProbeTests.cs | New tests pinning startup-time WinGet sidecar probe behavior and failure swallowing. |
| tests/Aspire.Cli.Tests/Acquisition/WindowsRegistryReaderTests.cs | New Windows-only round-trip tests against HKCU Uninstall for WinGet detection. |
| tests/Aspire.Cli.Tests/Acquisition/PeerInstallProbeTests.cs | Updates peer probe to call installs --self --format json and accept bare-array JSON shape. |
| tests/Aspire.Cli.Tests/Acquisition/InstallSidecarReaderTests.cs | Updates spec reference and enum expectations (brew→Homebrew) plus “future-source” wording. |
| tests/Aspire.Cli.Tests/Acquisition/InstallationDiscoveryDiscoverAllTests.cs | Updates expectations/messages and property names (route→source) for discovery behavior. |
| tests/Aspire.Acquisition.Tests/Scripts/VerifyCliArchivePowerShellTests.cs | Updates user-facing verifier assertions to “install-source sidecar” wording. |
| tests/Aspire.Acquisition.Tests/Scripts/ReleaseScriptShellTests.cs | Updates dry-run sidecar messaging assertions (route→source). |
| tests/Aspire.Acquisition.Tests/Scripts/ReleaseScriptPowerShellTests.cs | Updates -WhatIf sidecar messaging assertions (route→source). |
| tests/Aspire.Acquisition.Tests/Scripts/PRScriptToolModeTests.cs | Updates tool-mode assertions to ensure no “source sidecar” messaging appears. |
| tests/Aspire.Acquisition.Tests/Scripts/PRScriptShellTests.cs | Updates PR-script dry-run tests and sidecar messaging (route→source). |
| tests/Aspire.Acquisition.Tests/Scripts/PRScriptPowerShellTests.cs | Updates PR-script WhatIf tests and sidecar messaging (route→source). |
| tests/Aspire.Acquisition.Tests/Scripts/PRScriptInstallerModeTests.cs | Updates installer-mode expectations and wording (“Homebrew” command naming). |
| tests/Aspire.Acquisition.Tests/Scripts/PRScriptInstallE2ETests.cs | Updates E2E assertion wording for the sidecar presence (route→source). |
| tests/Aspire.Acquisition.Tests/Scripts/LocalHiveScriptFunctionTests.cs | Adds guard test ensuring localhive scripts don’t reference removed aspire info. |
| tests/Aspire.Acquisition.Tests/Scripts/Common/FakeArchiveHelper.cs | Updates spec reference in archive helper docs (install-routes→install-sources). |
| src/Shared/PathLookupHelper.cs | Returns filesystem-cased executable paths on Windows instead of inheriting PATHEXT casing. |
| src/Aspire.Cli/Utils/EnvironmentChecker/EnvironmentCheckResult.cs | Removes installations from doctor JSON response model. |
| src/Aspire.Cli/Utils/CliPathHelper.cs | Renames route→source for Aspire home directory selection helper. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.cs.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.de.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.es.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.fr.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.it.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.ja.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.ko.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.pl.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.pt-BR.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.ru.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.tr.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.zh-Hans.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.zh-Hant.xlf | Removes installation-table-related localized resources from doctor XLF. |
| src/Aspire.Cli/Resources/DoctorCommandStrings.resx | Removes installation-table string resources now that doctor no longer renders installs. |
| src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs | Removes generated resource accessors for deleted installation-table strings. |
| src/Aspire.Cli/Program.cs | Runs WinGet first-run probe during CLI startup and registers InstallsCommand/HiveEnumerator. |
| src/Aspire.Cli/JsonSourceGenerationContext.cs | Adds source-gen support for InstallationInfo[] and List<InstallListItem>. |
| src/Aspire.Cli/Commands/SetupCommand.cs | Updates comments for source-aware vs source-independent extract/install path behavior. |
| src/Aspire.Cli/Commands/RootCommand.cs | Wires InstallsCommand into the CLI root command. |
| src/Aspire.Cli/Commands/InstallsCommand.cs | Implements new aspire installs command and JSON/list outputs including orphan hives. |
| src/Aspire.Cli/Commands/InstallationInfoOutput.cs | Removes doctor-oriented discovery output; keeps safe self-description helper for installs self-probe. |
| src/Aspire.Cli/Commands/DoctorCommand.cs | Removes install enumeration/self-probe and limits output to environment checks. |
| src/Aspire.Cli/CliExecutionContext.cs | Updates docs/comments to reflect source-specific Aspire home selection behavior. |
| src/Aspire.Cli/Bundles/BundleService.cs | Removes WinGet probe call from bundle extraction path and updates spec reference. |
| src/Aspire.Cli/Acquisition/WingetFirstRunProbe.cs | Updates documentation wording to “install-source sidecar”. |
| src/Aspire.Cli/Acquisition/PeerInstallProbe.cs | Switches peer self-describe call to installs --self --format json and supports new JSON shape. |
| src/Aspire.Cli/Acquisition/IPeerInstallProbe.cs | Updates contract docs for the new peer-probe invocation surface. |
| src/Aspire.Cli/Acquisition/InstallSource.cs | Renames enum member to Homebrew while keeping the wire string as brew. |
| src/Aspire.Cli/Acquisition/InstallSidecarReader.cs | Updates comments/spec reference to install-sources. |
| src/Aspire.Cli/Acquisition/InstallationInfo.cs | Renames JSON field route→source and updates contract docs to installs surfaces. |
| src/Aspire.Cli/Acquisition/InstallationDiscovery.cs | Renames/threads Source through discovery and updates messaging to install-source terminology. |
| src/Aspire.Cli/Acquisition/InstallationCandidateSources.cs | Updates comment wording for installs discovery. |
| src/Aspire.Cli/Acquisition/IInstallSidecarReader.cs | Updates sidecar reader docs to install-source terminology and removes obsolete command references. |
| src/Aspire.Cli/Acquisition/IInstallationDiscovery.cs | Updates discovery interface docs for aspire installs and new peer-probe surface. |
| src/Aspire.Cli/Acquisition/HiveEnumerator.cs | Adds enumeration of hives used by aspire installs list to show orphan hives. |
| localhive.sh | Updates sidecar stamping comments/spec reference to install-sources terminology. |
| localhive.ps1 | Updates sidecar stamping and archive validation messaging to install-sources terminology. |
| eng/scripts/verify-cli-tool-nupkg.ps1 | Updates comment wording around sidecar source expectations. |
| eng/scripts/verify-cli-archive.ps1 | Updates archive sidecar validation messaging to install-sources terminology. |
| eng/scripts/get-aspire-cli.sh | Updates installer sidecar messaging to “source sidecar” and spec reference. |
| eng/scripts/get-aspire-cli.ps1 | Updates installer sidecar messaging to “source sidecar” and spec reference. |
| eng/scripts/get-aspire-cli-pr.sh | Renames PR sidecar writer function and updates comments/messages to install-sources terminology. |
| eng/scripts/get-aspire-cli-pr.ps1 | Renames PR sidecar writer function and updates WhatIf messaging to “source sidecar”. |
| eng/clipack/Common.projitems | Updates build-time guard/error text to “install sources” terminology and new spec reference. |
| docs/specs/install-sources.md | Renames/updates the sidecar specification from install-routes to install-sources. |
| docs/specs/cli-output-formats.md | Documents aspire installs list --format json and clarifies hidden installs --self contract. |
| docs/dogfooding-pull-requests.md | Updates wording around Homebrew uninstall behavior and shared prefixes (route→source). |
Files not reviewed (1)
- src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs: Language not supported
mitchdenny
reviewed
May 26, 2026
Member
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
Reviewed the install-discovery split. Three related findings, all on InstallsCommand.cs — the status field in the new installs list --format json contract is currently overloaded for non-Ok rows, which both complicates programmatic consumption and forces a couple of magic-string duplications.
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Thanks for the thorough PR-test pass @mitchdenny. All three inline comments addressed in aa0d308:
On Scenario 6 (bare CI green, ready for re-review. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
radical
added
the
NO-MERGE
radical
changed the title
aspire installs command; drop install table from aspire doctoraspire --info to enumerate the running CLI's install + discovered installs + hives
JamesNK
reviewed
May 26, 2026
Member
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
One finding: BuildRowsAsync lacks the error handling described in the PR description for discovery-walk failures. Either the code needs a try-catch (matching what DiscoverAllSafelyAsync used to do), or the PR description should be updated to reflect that this invariant was intentionally dropped.
Rename the C# identifiers, prose, file names, and xmldoc references that
spoke of an "install route" to "install source" so the codebase matches
the on-disk sidecar field name (`{"source":"..."}`) which has always
been the authoritative spelling.
Mechanical rename only. No behavior change. Wire strings are unchanged:
`script`, `pr`, `winget`, `brew`, `dotnet-tool`, `localhive` all stay
literal; the sidecar JSON field name was already `source`.
Notable identifier renames:
- `InstallSource.Brew` enum value → `InstallSource.Homebrew` (the wire
string stays `"brew"` via the preserved `InstallSourceExtensions.BrewWire`
constant — only the C# enum name and the friendlier display label change).
- `InstallationInfo.Route` property → `InstallationInfo.Source` (and the
associated `[JsonPropertyName("route")]` → `"source"`; this surface
was added earlier in the branch and has not shipped).
- `CliPathHelper.TryGetAspireHomeDirectoryFromInstallRoute` →
`TryGetAspireHomeDirectoryFromInstallSource`.
- `docs/specs/install-routes.md` → `docs/specs/install-sources.md`.
- `tests/.../BundleServiceCrossRouteExtractionTests.cs` →
`BundleServiceCrossSourceExtractionTests.cs` and the contained
theory method's name.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…indows `PathLookupHelper.FindAllFullPathsFromPath` used to concatenate the candidate command name with each `PATHEXT` entry and return that synthesized path verbatim when the file existed. Because Windows `PATHEXT` is typically registered uppercase (`.EXE`, `.CMD`, ...), an on-disk `aspire.exe` would surface to user-facing output as `aspire.EXE`, which looks like a different binary and breaks string- compare dedup checks elsewhere in the install-discovery walk. After matching the file by case-insensitive existence check, look up the actual on-disk name via `Directory.EnumerateFileSystemEntries(directory, fileName)` and return that spelling instead of the `PATHEXT`-derived form. Linux and macOS are untouched. The probe is guarded against `Directory.EnumerateFileSystemEntries` throwing — if the lookup fails (permissions, path too long, etc.) the helper falls back to the original PATHEXT-derived spelling, preserving the prior behavior on the unhappy path. Regression test added at `tests/Aspire.Hosting.Tests/PathLookupHelperTests.cs` exercises the Windows-only casing-preservation path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Aspire CLI install enumeration was bolted onto `aspire doctor` alongside
the environment-prerequisite checks. That mixed two unrelated concerns
into one command, and there was no clean root-level surface for
"tell me what CLI I'm running and what other CLIs / hives this machine
has."
Add a root-level `--info` flag that returns the running CLI's identity
plus the install/orphan-hive table in one payload, and pull the install
table out of `aspire doctor`.
- `aspire --info` — text: `Version`, `Channel`, then the per-row
install block.
- `aspire --info --format json` —
`{ "version": ..., "channel": ..., "installs": [...] }`.
- `aspire --info --self` — text: just the running CLI's row.
- `aspire --info --self --format json` — single-element
`InstallationInfo` array; this is the cross-version peer-probe
contract consumed by `PeerInstallProbe.TryParseRichProbeResult`.
`--info` is wired as an `AsynchronousCommandLineAction` on a root-only
option, so `aspire --info` short-circuits routing the same way `--help`
and `--version` do — but unlike `--help`/`--version`, `--info` is
intentionally non-recursive: `aspire run --info` is a separate operation
(the `--info` token does not bind at subcommand scope, so `run` runs and
`--info` is ignored). This keeps install enumeration a deliberate
top-level concept and avoids the recursive-option / subcommand-local
`--format` shadowing trap, where `aspire run --info --format json` would
otherwise silently emit install text because `run`'s own `--format`
swallows the token.
`--self` and `--format` are hidden root-only options with validators
that fail parse when set without `--info`, so `aspire --self` and
`aspire --format json` produce a parse error instead of silently falling
through to grouped help (and `aspire doctor --format json` still routes
correctly to the doctor subcommand). `--info` is added to
`CommonOptionNames.InformationalOptionNames` so its text form does not
consume the first-run sentinel or start a tracked telemetry activity.
`aspire doctor` is now scoped to environment checks. The `installations`
field is removed from `aspire doctor --format json`; the `doctor --self`
hidden peer-probe variant is removed; the install-table rendering and
the install-related `DoctorCommandStrings` resource entries (and
matching xlf entries across 13 languages) are deleted.
`PeerInstallProbe` now spawns peers with
`["--info", "--self", "--format", "json"]`. An older peer that predates
`--info` exits non-zero on the unknown option; the probe falls through
to a second spawn of `["--version"]` and re-derives the peer's channel
from its InformationalVersion string for PR builds (the existing
`--version` fallback path in the probe). No explicit shim for the old
`doctor --self` argv is needed because no live consumer of that argv
remains.
Why this approach
-----------------
`CliExecutionContext`'s DI factory used to eagerly call
`IIdentityChannelReader.ReadChannel()`, which throws when a CLI binary
has missing or invalid `AspireCliChannel` assembly metadata. That made
the channel-failure path undeliverable for any diagnostic surface: the
factory would crash before any subcommand or root-option action ran.
The factory now wraps the read in `Lazy<string>` and
`CliExecutionContext.IdentityChannel` returns
`_lazy?.Value ?? identityChannel`, preserving the original throwing
semantics on first access for consumers that actually need the channel
(packaging, scaffolding, update / new / init), while letting
`InfoOptionAction` inject `IIdentityChannelReader` directly and catch
the exception itself — exit 0, channel field dropped from JSON (via
`DefaultIgnoreCondition.WhenWritingNull`), Channel row omitted from text.
`InfoOption`, `InfoSelfOption`, and `InfoFormatOption` are per-
`RootCommand` instance fields rather than `static`. Each `RootCommand`
construction sets `InfoOption.Action` and adds option validators; with
static fields, concurrent tests building their own `RootCommand` would
race on the shared `Action` setter (last write wins, the test that
built first sees another test's service provider) and `Validators.Add`
would accumulate across instances.
`WingetFirstRunProbe.Run` moves from `BundleService` + the old install
table call site into the `CliExecutionContext` DI factory in
`Program.cs`. Same behavior, single call site, fires before the
install-source sidecar is read by any downstream consumer;
`BundleService` is now install-source-agnostic.
Surprises and call-outs
-----------------------
- `--info` is intentionally root-only. `aspire --info` fires install
enumeration; `aspire run --info` is a separate operation that runs
the subcommand and ignores the unmatched `--info` token. Two
regression tests (`Info_OnSubcommand_DoesNotFireInfoAction`,
`Info_OnSubcommand_WithFormat_BindsFormatToSubcommand`) pin this so
a future Recursive flip can't silently resurrect the subcommand-local
`--format` shadowing trap.
- Discovery-walk failure exits 0, not non-zero. `BuildRowsSafelyAsync`
catches a throwing `IInstallationDiscovery.DiscoverAllAsync`
(filesystem ACL, IO on `~/.aspire/hives`, ...) and emits a single
failure row (`kind: "discovery-failed"`, `status: "failed"`,
`statusReason: <message>`) so `aspire --info` stays useful for
diagnosing a flaky environment. Same posture as the channel-read and
self-version tolerant paths. The empty-installs case still emits
`"installs": []` rather than dropping the field.
- Channel-read failure also deliberately does not exit non-zero. The
`--info` surface is exactly the one a user reaches for to find out
their binary is broken; it would be self-defeating to crash.
- `--format list` is accepted as the explicit spelling of the default
text rendering. The enum stays `{ List, Json }` so the
System.CommandLine value parser rejects unknown values like `table`.
- The `--info --self --format json` row uses the `InstallationInfo`
field set (`path`, `canonicalPath`, `version`, `channel`, `source`,
`pathStatus`, `status`, `statusReason`), which is *distinct from* the
aggregate `installs[*]` row shape (`id`, `kind`, `channel`, `path`,
`hive`, `status`, `statusReason`, `managedBy`). Both shapes are
documented in `docs/specs/cli-output-formats.md`.
Tests
-----
`InfoOptionTests.cs` covers: peer-probe contract roundtrip, channel-read
failure path (asserts `CliExecutionContext` still resolves and the
command exits 0), `channel: "local"` is not treated as failure,
empty-installs `installs: []`, discovery-walk failure exits 0 with a
`discovery-failed` row, `--format list` synonym + `--format table`
rejection, `aspire run --info` is non-recursive and does not fire the
info action, parse errors for `--self`/`--format` without `--info`, the
`aspire doctor --format json` regression guard, and an end-to-end
peer-probe roundtrip that feeds `--info --self --format json` output
through `InstallationInfoParser.Parse`.
`PeerInstallProbeTests` argv assertion, shell/batch fake scripts
(`EmitExit`, `StderrRepeatScript`, `DoctorOrVersionScript`), and inline
comments updated to dispatch on `--info` instead of `doctor --self`.
New tests: `HiveEnumeratorTests` (channel-shape allow-list + path
returns), `WindowsRegistryReaderTests` (Windows-only HKCU round-trip
with prefixed entries swept on each run), `WingetStartupProbeTests`
(pins the DI-factory invocation contract and the swallow-on-failure
behavior).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
radical
force-pushed
the
radical/installs-discovery
branch
from
8bf4093 to
402388b
Compare
The previous gate scanned raw args with
`args.Any(a => InformationalOptionNames.Contains(a))`. Because `--info`
is wired as a root-only non-recursive option on `RootCommand`,
`aspire run --info` is a real `run` invocation — the `--info` token
does not bind to `RootCommand.InfoOption`. But the position-blind scan
still matched the literal token, so `aspire run --info` silently
suppressed telemetry, suppressed the first-run banner, and skipped
creating the one-shot first-run sentinel for that real `run`
invocation.
Replace `InformationalOptionNames` with `IsInformationalInvocation`, a
position-aware helper:
- `--info` counts as informational only before the first non-option
token (matching the System.CommandLine binding).
- `--version`, `--help`, `-h`, `-?` stay position-blind because
System.CommandLine treats them as recursive — they really are
informational at any depth, e.g. `aspire run --help`.
`TelemetryManager` and `Program.DisplayFirstTimeUseNoticeIfNeededAsync`
both switch to the helper. Test coverage pins both sides of the new
contract:
- `Info_OnSubcommand_IsNotInformationalInvocation` /
`RecursiveInformationalFlags_OnSubcommand_RemainInformational` —
helper-level.
- `AzureMonitor_Disabled_WhenInfoFlagProvided` /
`AzureMonitor_Enabled_WhenInfoFlagOnSubcommand` /
`AzureMonitor_Disabled_WhenRecursiveInformationalFlagOnSubcommand`
— telemetry-gate behavior.
- `InfoFlagOnSubcommand_CreatesSentinelAndShowsBanner_OnFirstRun` /
`RecursiveInformationalFlag_OnSubcommand_DoesNotCreateSentinel`
— first-run sentinel / banner behavior.
- `--info` added to the existing
`InformationalFlag_SuppressesBannerAndDoesNotCreateSentinel`
theory for symmetry with `--version`/`--help`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
radical
removed
the
NO-MERGE
Member
Author
|
New version of the changes are in now. Instead of |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 95 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs: Language not supported
- src/Aspire.Cli/Resources/RootCommandStrings.Designer.cs: Language not supported
Before this change `aspire --info --format json` and
`aspire --info --self --format json` returned two structurally different
record types:
- aggregate `installs[*]` was an `InstallListItem` with
{id, kind, channel, path, hive, status, statusReason, managedBy}
- `--self` bare-array element was an `InstallationInfo` with
{path, canonicalPath, version, channel, source, pathStatus, status,
statusReason}
Only 4 of ~10 fields overlapped. Neither shape has shipped (the
introducing PR microsoft#17105 commit is not in any release tag or release branch),
so there is no backward-compat constraint here.
Unify both surfaces onto `InstallationInfo`:
- `Path` becomes nullable (orphan-hive rows describe a directory on disk
with no binary).
- Add `Id`, `Kind`, `Hive`, `ManagedBy` to `InstallationInfo`. Aggregate
rows populate them; the `--self` surface leaves them null (and
`WhenWritingNull` keeps them out of the JSON).
- Delete the `InstallListItem` record.
- `InfoOutput.Installs` is now `IReadOnlyList<InstallationInfo>`.
- Drop the `GetInstallStatus` projection that previously folded
`PathStatus` into `Status` for OK aggregate rows. `Status` is now
purely lifecycle (`ok`/`failed`/`notProbed`/`no install found`) and
`PathStatus` is the PATH-axis on both surfaces. The two axes are
orthogonal on the wire.
- Text rendering now emits `Status <lifecycle>` plus
`On PATH <pathStatus>` for the aggregate, mirroring what `--self`
already did.
Contract + behavior tests:
- `Info_UnifiedShape_AggregateRowAndSelfRowHaveSameKeys` pins that the
aggregate and self surfaces use the same record shape.
- `Info_Json_StatusAndPathStatusAreOrthogonalForOkInstalls` catches
accidental re-introduction of the status/pathStatus projection.
- `Info_Json_OrphanHiveRow_HasNullPathAndPopulatedHive` pins the
orphan-hive structural shape.
- `Info_Json_SortOrder_ActiveThenShadowedThenNotOnPathThenFailedThenOrphanHive`
pins the deterministic row order under the new orthogonal axes.
- `Info_TextRendering_DisplaysStatusAndOnPathSeparately` pins the
human-visible two-line rendering.
The existing `Info_Self_Json_IsParseableByInstallationInfoParser_PeerProbeContract`
roundtrip test continues to pin the peer-probe wire contract end-to-end.
Spec doc `docs/specs/cli-output-formats.md` is updated to document the
single unified shape with orthogonal status/pathStatus axes and the
orphan-hive special case.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`aspire run`, `aspire add`, every CLI command on Windows currently walks
both HKCU and HKLM `Uninstall` hives looking for the winget portable
manifest entry, on every single startup, for any install path that
didn't ship the `.aspire-install.json` sidecar (notably dev builds and
legacy pre-sidecar installs). The probe has a positive fast path —
sidecar exists, skip — but no negative-result cache. The old
`{"source":"winget"}` byte-array writer only fires when the registry
confirms a winget install, so non-winget Windows users re-walk the
registry forever. The factory comment in `Program.cs` claimed "the
worst case here is a no-op", which is true about side effects but
misleading about cost.
Rework the probe as a sidecar back-fill that always writes after one
registry read, either the positive `{"source":"winget"}` payload (when
the running binary IS a winget portable install) or a
`{"backfilled":true}` sentinel otherwise. The sentinel has no `source`
field, so `InstallSidecarReader.TryRead` parses it as Unknown and
downstream consumers see the row identically to a missing sidecar. The
existing `File.Exists` fast path at the top of `EnsureSidecar` then
short-circuits every subsequent startup — `HasWingetAspireUninstallEntry`
runs at most once per install location, ever.
Rename for clarity:
- `WingetFirstRunProbe` → `WingetSidecarBackfill`. "FirstRun" implied
run-once-and-never-again; the reality is "every run until a sidecar
exists". "Probe" implied read-only detection; the type writes.
- `WingetFirstRunProbe.Run(string)` → `WingetSidecarBackfill.EnsureSidecar(string)`.
The new name matches the post-call invariant: a sidecar exists at the
binary directory (real or backfilled sentinel) unless the write itself
failed.
- `Program.TryRunWingetFirstRunProbe` → `Program.TryEnsureWingetSidecar`.
- Test files `WingetRegistryProbeTests` / `WingetStartupProbeTests` →
`WingetSidecarBackfillTests` / `WingetSidecarBackfillStartupTests`.
Surprises worth calling out:
- One small behavior change for non-winget Windows installs: discovery
used to mark these rows as `notProbed` (no sidecar = "we won't execute
this binary"). Post-change the sidecar exists (sentinel form), so
discovery promotes the row to `ok` and spawns the peer probe. The
spawn target is the running binary itself — same binary that just
wrote the sentinel — so self-vouching is logically consistent; the
`notProbed` outcome was a side effect of the missing-sidecar gate,
not a deliberate categorization. End user effect: the running CLI's
row in `aspire --info` shows a real version instead of "not probed".
- A user who later installs via winget on top of a previously
probed-negative location pays one row of `source: null` until they
reinstall or delete the sentinel. `EnsureSidecar_DoesNotPromoteBackfilledSentinel_OnLaterWingetRun`
pins this trade-off explicitly. In practice winget portable installs
land in their own directory under `%LOCALAPPDATA%\Microsoft\WinGet\Packages\`,
so the collision is rare.
- The startup-contract tests (`WingetSidecarBackfillStartupTests`) now
clean up the sentinel written next to the test runner binary; without
cleanup, the second test would skip via the existing-sidecar fast path
and pollute the build output across runs.
Tests:
- `WingetSidecarBackfillTests`:
- `EnsureSidecar_WritesWingetSidecar_WhenRegistryClaimsWinget` — positive path.
- `EnsureSidecar_WritesBackfilledSentinel_WhenRegistryDoesNotClaimWinget` — sentinel content + asserts `source` field intentionally absent.
- `EnsureSidecar_SkipsRegistryWalk_WhenSidecarExists` — counting fake registry reader, asserts `CallCount == 0` (the cost-reduction guarantee).
- `EnsureSidecar_DoesNotOverwriteExistingSidecar` — idempotency.
- `EnsureSidecar_DoesNotPromoteBackfilledSentinel_OnLaterWingetRun` — trade-off pinned.
- `EnsureSidecar_ConcurrentInvocations_ProduceSingleValidSidecar` `[Theory]` — both positive and negative concurrent paths.
- `WingetSidecarBackfillStartupTests`: renamed + cleanup of negative-result sentinel.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ateCommand ctors
`aspire --info` is supposed to be the surface a user reaches for to
find out their binary is broken — including the case where the CLI
binary ships missing or invalid `[AssemblyMetadata("AspireCliChannel", ...)]`.
The `CliExecutionContext` DI factory in `Program.cs` already wraps
`IIdentityChannelReader.ReadChannel()` in a `Lazy<string>` precisely so
the context resolves cleanly when channel metadata is broken, and
`InfoOptionAction` injects the reader directly with its own try/catch.
Both pieces work in isolation. But the rest of the DI graph defeats
them: `NewCommand` and `UpdateCommand` are both pulled in as
constructor parameters of `RootCommand`, and both constructors read
`ExecutionContext.IdentityChannel` eagerly to pick a help-text variant
for `--channel`. On a broken-channel binary the Lazy throws on that
first access, `NewCommand`/`UpdateCommand` construction fails, and
`provider.GetRequiredService<RootCommand>()` throws — taking
`aspire --info` and `aspire --help` down with it.
Wrap the channel reads in tolerant helpers that mirror
`InfoOptionAction.TryReadChannel`: catch any non-cancellation
exception, fall back to the non-staging description, and let the
constructor complete. The fallback is benign — only the help text for
`--channel` loses its staging-channel mention; the option itself still
works, and the command's normal runtime reads of
`ExecutionContext.IdentityChannel` (which legitimately need to throw
on a broken binary) are unchanged.
Why the existing `Info_ChannelReadFailure_*` tests missed this:
`TestExecutionContextHelper.CreateExecutionContext` builds
`CliExecutionContext` with a literal `identityChannel: "local"` string
and leaves `IdentityChannelLazy` null, so
`CliExecutionContext.IdentityChannel` returns the literal string and
never invokes the throwing reader. Those tests exercise only the
direct-injection path inside `InfoOptionAction`, not the lazy property
path that production wiring goes through. Adding a sibling test that
sets `IdentityChannelLazy` to a throwing factory delegate exercises the
real production code path and pins both new tolerant helpers.
Tests:
- `InfoOptionTests.Info_ChannelLazyThrows_RootCommandStillResolves_AndInfoExitsZero`
builds the service provider with a `CliExecutionContext` whose
`IdentityChannelLazy.Value` throws on first access, resolves
`RootCommand` (forcing every subcommand ctor through the throwing
path), parses `--info --format json`, and asserts exit 0 with the
channel field omitted from the JSON payload.
- Verified the test correctly fails under the pre-fix code by
stashing the `NewCommand.cs` / `UpdateCommand.cs` changes and
re-running — the resolution throws with
`InvalidOperationException: AspireCliChannel assembly metadata is
missing.` exactly as production would.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… value-taking options and after `--` `CommonOptionNames.IsInformationalInvocation` is the pre-parse gate that decides whether an invocation is purely informational, so the CLI can opt out of telemetry, suppress the first-run banner, and skip consuming the one-shot first-run sentinel. It walks `args[]` once, treating the first non-option token as the subcommand boundary; before that boundary `--info` counts as informational, after it does not (mirroring `--info`'s root-only / non-recursive binding on RootCommand). Two correctness gaps in that scan: 1. **Value tokens for root value-taking options were being mistaken for subcommands.** Root options like `--log-level`, `-l`, `--format`, `--capture-profile-output`, `--capture-profile-delay` take a value in the next token, and value tokens don't start with `-`. The scan flipped `sawSubcommand=true` on the value, then refused to count the trailing `--info`. Real-world consequence: `aspire --log-level Debug --info`, `aspire --format json --info` (the exact example called out in the doc comment as "safe because the validator rejects it" — which is wrong, the validator only fires when --format is supplied WITHOUT --info), `aspire --locale fr-FR --info`, etc. all sent telemetry and showed the first-run banner — and on a fresh-install machine the banner could interleave with `--info --format json` output, breaking `PeerInstallProbe.TryParseRichProbeResult`'s JSON parse on the consumer side. 2. **The POSIX `--` end-of-options marker was ignored.** Tokens after `--` are positional / forwarded args (`aspire run -- --info` forwards `--info` to the AppHost), but the scan kept reading and would return true on the literal `--info` token. Result: `aspire -- --info` was misclassified as informational even though `--info` after `--` cannot bind to the root option in System.CommandLine. The fix is the same shape as the original gate, with two added rules: - Stop scanning at `arg == "--"`. The marker terminates option parsing by POSIX convention, so anything after — including `--info`, `--help`, `--version` — is a forwarded arg, not a CLI flag. - Maintain a `s_rootOptionsTakingAValue` set listing the non-bool root options. When the scan sees one of these and we're still before any subcommand, skip the next token so the option's value isn't treated as a subcommand boundary. `=`-form (`--log-level=Debug`) is a single token that starts with `-` and needs no special handling. The set is kept in sync by hand with the non-bool options declared on RootCommand. A bool option (`--debug`, `--non-interactive`, ...) doesn't take a separate value token and doesn't need to be listed. Also corrected the outdated XML doc comment that claimed `ValidateRequiresInfo` mitigates the `--format json --info` case — it doesn't, because `--info` IS present in that invocation so the validator passes. Tests: 34 new test cases in `InfoOptionTests`, all named `IsInformationalInvocation_*`: - `_ReturnsTrue_ForInfoAfterRootValueTakingOption` `[Theory]`, 9 inlines — pins the value-skip across `--log-level`, `-l`, `--format`, `--capture-profile-output`, `--capture-profile-delay`, mixed, and `=`-form. - `_ReturnsTrue_ForInfoAfterRootBoolOption` `[Theory]`, 6 inlines — sanity coverage for bool root options that don't need the skip, so a future accidental promotion into the value-taking set regresses one of these. - `_ReturnsTrue_ForInfoBeforeSubcommandToken` — `["--info", "run"]`. - `_ReturnsFalse_AfterEndOfOptionsMarker` `[Theory]`, 8 inlines — pins the `--` short-circuit across `aspire --`, `aspire run --`, `aspire run apphost.csproj --`, `aspire publish --` shapes, with every informational flag (`--info`, `--help`, `-h`, `--version`). - `_ReturnsTrue_WhenInfoIsBeforeEndOfOptionsMarker` — `--info` before `--` still binds at root. - `_ReturnsFalse_WhenInfoFollowsSubcommandEvenAfterRootValueTakingOption` `[Theory]`, 3 inlines — combined case from the bug-report shape. - `_ReturnsFalse_ForEmptyArgs`, `_ReturnsFalse_ForNullArgs`, `_ReturnsFalse_ForBareSubcommand`, `_ReturnsFalse_ForValueTakingOptionAtEndOfArgsWithNoInfo` (`aspire --log-level`, malformed parse handled cleanly). - `_ReturnsFalse_WhenValueLooksLikeInfo` — pathological `aspire --log-level --info` where `--info` is the VALUE of `--log-level` rather than a real flag. - `_ReturnsTrue_ForVersionAtAnyPosition` — recursive-flag sanity. Verified the new tests catch the bug: stashed `src/Aspire.Cli/CommonOptionNames.cs` and re-ran the suite — 13 of the 44 IsInformationalInvocation tests failed under the old code (7 for the value-taking-option case, 5 for the `--` case, 1 for the pathological-value case). With the fix in place all 1301 Commands tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
WingetSidecarBackfill.EnsureSidecar exists to amortize a HKCU + HKLM
Uninstall hive walk: on a winget portable install it stamps
{"source":"winget"}; on any other Windows install it writes a
{"backfilled":true} sentinel so the next startup hits the File.Exists
fast path and never re-walks the registry.
It was being invoked unconditionally from the CliExecutionContext DI
factory (Program.TryEnsureWingetSidecar). On Linux/macOS the
NullWindowsRegistryReader stub always returns false, so the helper
dropped the negative-result sentinel into the directory containing
Environment.ProcessPath — e.g. the dotnet global-tools store at
~/.dotnet/tools/.store/aspire.cli/<ver>/.../tools/<tfm>/<rid>/ — with
no amortization benefit, because there's no registry there to walk in
the first place. The write was harmless in isolation, but a stray
sidecar in the dotnet-tool layout silently re-classifies the install's
Source to a now-permanent sentinel-backed Unknown.
Gate EnsureSidecar with OperatingSystem.IsWindows() at the top, so the
method's contract (the docstring already says "on any other Windows
install ...") matches its observable behavior. The existing tests that
asserted the write happened unconditionally were also wrong on this
axis; gate them with Assert.SkipUnless(OperatingSystem.IsWindows())
mirroring WindowsRegistryReaderTests, and pin the new no-op behavior
with EnsureSidecar_IsNoOp_OnNonWindows.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
IsDisplayableInstall is the filter that decides whether a sidecar-less discovery row gets surfaced in `aspire --info`. It used a case-sensitive pattern match — `fileName is "aspire" or "aspire.exe"`. PathLookupHelper deliberately preserves whatever spelling NTFS recorded for the binary (that's the point of the recent on-disk-casing fix); Windows command resolution is case-insensitive, so a real install with on-disk filename `Aspire.exe` or `ASPIRE.EXE` and no sidecar would be silently filtered out as "not an Aspire CLI binary" and never appear in `aspire --info`. Switch to string.Equals with OrdinalIgnoreCase on Windows and Ordinal elsewhere, so POSIX paths stay case-sensitive (a deliberately-named `Aspire` script on Linux is still excluded). Cover the three plausible on-disk spellings with a Windows-only Theory in InfoOptionTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The unique-id minter in `InstallationInfoOutput.GetUniqueIdCore`
counted collisions on the base id only and never reserved the suffixed
name it returned. Two rows could therefore share an `id` when one row's
natural base id happened to match another row's disambiguation suffix:
1st "stable" → "stable"
2nd "stable" → "stable-2" (suffix NOT recorded)
orphan hive literally "stable-2" → "stable-2" (silent collision)
The orphan-hive base id is the directory name under `~/.aspire/hives/`,
which is not constrained to the `IdentityChannelReader` allowlist on
the enumeration path, so user-created (or installer-quirk) hive names
can naturally land on a `<channel>-N` shape. JSON consumers that key on
`id` (the documented purpose of the field) silently merge or overwrite
the colliding rows.
Loop the suffix counter until the candidate is unused, then record both
the bumped per-base counter and the candidate itself so future natural
ids can't recreate it.
Added a regression test exercising two `winget` installs sharing
channel `stable` plus an orphan-hive directory named `stable-2`; the
test fails on the pre-fix code (orphan ends up sharing `stable-2`) and
passes with the fix (orphan gets `stable-2-2`).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ages
`ValidateRequiresInfo` formats the offending option using `option.Name`.
System.CommandLine 2.0.8 returns the full token including the leading
`--` (e.g. `--self`), so the rendered message reads:
Option `--self` is only valid when `--info` is also specified.
A future SCL upgrade that changed `Name` semantics to strip the dashes
would silently degrade the message to the ungrammatical "Option `self`
is only valid when..." with no test failure today, because the existing
parse-error tests only check that `result.Errors` is non-empty.
Strengthen `Info_SelfWithoutInfo_FailsParse` and
`Info_FormatWithoutInfo_FailsParse` to assert the error text contains
``--self`` / ``--format`` (the token form, backticks included). This is
the same shape other consumers in the repo already rely on
(`BaseCommand.cs` checks `option.Name == "--format"`,
`GroupedHelpWriter.cs` calls `option.Name.StartsWith("--", ...)`), so
an SCL change in this area would break multiple call sites — the new
assertion just surfaces it on the user-visible path.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 101 changed files in this pull request and generated 3 comments.
Files not reviewed (2)
- src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs: Language not supported
- src/Aspire.Cli/Resources/RootCommandStrings.Designer.cs: Language not supported
… lifecycle
`AzureMonitor_Enabled_WhenInfoFlagOnSubcommand` was constructing
`TelemetryManager` directly:
var configuration = new ConfigurationBuilder().Build();
var manager = new TelemetryManager(configuration, args);
Assert.True(manager.HasAzureMonitor);
Because the args (`"run", "--info"` etc.) do not trip the informational
opt-out, this builds a real Azure Monitor `TracerProvider` and registers
a process-wide `ActivityListener` on `Aspire.Cli.Reported`. The manager
is never disposed, and even with `using` would not be — `TelemetryManager.Dispose`
only calls `Shutdown(0)` on the providers (see comment on
`TelemetryManager.cs:208`), which does not unregister the listener.
Across parallel test classes the leaked listener intercepts reported
activities from unrelated tests (`AddCommand`, `NewCommand`,
`RunCommand`, ...), and concurrent invocations race on
`ActivityCreationOptions.SamplingTags.Add("microsoft.sample_rate", ...)`
(Azure Monitor's `RateLimitedSampler` uses `Add`, not `TryAdd`),
throwing the same exception the test process tries to surface as the
failing test:
❌ An unexpected error occurred: "The collection already contains
item with same key 'microsoft.sample_rate'"
This is the same race tracked by microsoft#17450. The fix in
e126a1a (process-wide telemetry opt-out + opted-in tests routing
through `BuildHostAsync`) keeps existing Azure-Monitor-enabled tests on
`main` clean — they all go through DI disposal. The new tests on this
branch bypassed that lifecycle and resurrected the race.
Route the new test through `BuildHostAsync` like the surrounding
`AzureMonitor_Enabled_ByDefault` test. `BuildHostAsync` now accepts an
optional `args` parameter that is threaded into
`Program.BuildApplicationAsync`, so the position-aware
`IsInformationalInvocation` gate still gets exercised with the
non-informational subcommand-plus-`--info` shapes. `WithTelemetryOptIn`
undoes the process-wide opt-out (`TestTelemetryDefaults`) so Azure
Monitor is actually built — same lifecycle pattern as
`AzureMonitor_Enabled_ByDefault`.
The two new opted-OUT tests (`AzureMonitor_Disabled_WhenInfoFlagProvided`,
`AzureMonitor_Disabled_WhenRecursiveInformationalFlagOnSubcommand`)
stay on direct construction: they short-circuit before building a
TracerProvider, so there is no listener to leak.
The underlying bug in `TelemetryManager.Dispose` is pre-existing and
out of scope for this PR — filed separately.
Verified locally: three consecutive full Aspire.Cli.Tests runs all pass
3864/3864 with zero `sample_rate` failures. Before this change the same
runs surfaced 20-60 `sample_rate`-flavored failures each.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
Three findings from the latest Copilot review pass on PR microsoft#17461: 1. `WingetSidecarBackfillStartupTests.cs` — `CapturingWindowsRegistryReader` XML doc claimed returning `false` from the reader meant "the probe does not write a sidecar", which is the opposite of what the code does: returning `false` is exactly what drives `WingetSidecarBackfill` down the negative-result arm that writes the `{"backfilled":true}` sentinel. Rewrite the doc to describe the actual contract. 2. `WingetSidecarBackfillStartupTests.cs` — the test's `AssertBackfillReachableFromTestRunner` skip path silently dropped coverage on Windows. Root cause: `Program.TryEnsureWingetSidecar` derives `binaryDir` from `Environment.ProcessPath`, and the `CliExecutionContext` DI factory invokes it on every resolution. Tests that build a real host via `Program.BuildApplicationAsync` and resolve `CliExecutionContext` (e.g. `CliBootstrapTests`) trigger the back-fill on Windows, writing `.aspire-install.json` into the testhost binary directory. After that write, `WingetSidecarBackfillStartupTests`'s `File.Exists` precondition fires and the assertion path is skipped — silently, with no failure. The sentinel also persists across `dotnet test --no-build` invocations because build-output `bin/...` is not purged between runs. Fix: thread the running binary's process path through as an explicit parameter. Add an `internal static TryEnsureWingetSidecar(sp, processPath)` overload on `Program` and an `internal EnsureSidecar(binaryDir, processPath)` overload on `WingetSidecarBackfill`; the existing public/no-arg overloads delegate with `Environment.ProcessPath`. `EnsureSidecar` no longer reads `Environment.ProcessPath` mid-method, so `binaryDir` and the registry probe argument are guaranteed consistent for any caller. Production call site at `Program.cs:377` is unchanged. Rewrite `WingetSidecarBackfillStartupTests` to use a per-test temp directory via `TestTempDirectory` and a synthesized fake binary path, drop `AssertBackfillReachableFromTestRunner` and the silent-skip path entirely, and assert the registry probe sees the same fake path. The "DI factory binds binaryDir to `Environment.ProcessPath`" claim becomes a one-line delegating expression at the no-arg overload, below the threshold of needing dedicated coverage. Leave a breadcrumb in `CliBootstrapTests.BuildHostAsync` so the next person debugging a stray `.aspire-install.json` in the testhost binary directory on Windows finds the cause. 3. `eng/scripts/verify-cli-tool-nupkg.ps1` — wording fix: "so the CLI can source `aspire update --self`" implied the CLI is sourcing a script. Should be "route" — the sidecar's `source` field is what lets the CLI route the self-update command to the dotnet-tool delegate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
PR Testing Report — #17461PR Information
CLI Version Verification
Scope TestedThis PR's user-facing surface is the new top-level Test Scenarios ExecutedScenario 1:
|
| # | Scenario | Status |
|---|---|---|
| 1 | aspire --info (text) |
✅ Passed |
| 2 | aspire --info --format json (aggregate) |
✅ Passed |
| 3 | aspire --info --self (text) |
✅ Passed |
| 4 | aspire --info --self --format json (peer contract) |
✅ Passed |
| 5 | aspire --self without --info (parse error) |
✅ Passed |
| 6 | aspire --format json without --info (parse error) |
✅ Passed |
| 7 | aspire run -- --info (-- boundary) |
✅ Passed |
Overall Result
✅ PR VERIFIED on macOS arm64
All seven scenarios behaved as specified by the PR description and docs/specs/cli-output-formats.md. The peer-probe contract (scenario 4) returns the documented bare-array shape, the parse-time validation (5+6) renders option tokens with their leading -- intact, and the -- end-of-options boundary (7) correctly suppresses the informational scan so run dispatches as a real subcommand.
Not Covered Here
- WingetSidecarBackfill seam (Windows-only). The most recent commit (
dafaf4f5f8) addresses the silent-skip inWingetSidecarBackfillStartupTestsby adding aninternal void EnsureSidecar(string binaryDir, string? processPath)overload onWingetSidecarBackfilland a matchinginternal static TryEnsureWingetSidecar(IServiceProvider sp, string? processPath)overload onProgram. The non-Windows tests inAspire.Cli.Testspass locally; the assertion path is Windows-only and is validated by theTests / Cli / Cli (windows-latest)CI leg.
Recommendations
- No issues found. Windows CI on
dafaf4f5f8is the remaining signal needed before this PR is ready to ship.
Member
PR Testing ReportPR Information
CLI Version Verification
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: PR CLI install + version verificationObjective: Install the dogfood CLI into an isolated temp directory and verify the installed binary matches the PR head commit. Steps:
Evidence:
Observations:
Scenario 2: Human �spire --info outputObjective: Verify the human output reports version, channel, install/source status, path, and hive information. Steps:
Evidence:
Observations:
Scenario 3: JSON �spire --info --format json contractObjective: Verify the machine-readable aggregate output shape and renamed source/kind fields. Steps:
Evidence:
Observations:
Scenario 4: Peer-probe �spire --info --self --format json contractObjective: Verify the hidden self-probe emits a single InstallationInfo row for peer discovery. Steps:
Evidence:
Observations:
Scenario 5: Root option validation / scopingObjective: Verify hidden root modifiers require --info and subcommand --format remains command-specific. Steps:
Evidence:
Observations:
Expected Unhappy-Path Outcome: Root-only --self and --format modifiers fail clearly without --info; doctor keeps its own --format behavior. Scenario 6: Orphan hive discovery boundaryObjective: Verify an isolated hive with no matching install is reported as a safe diagnostic row. Steps:
Evidence:
Observations:
Expected Unhappy-Path Outcome: A hive directory without a matching binary is surfaced as an orphan-hive row and exits successfully. Scenario 7: Basic CLI regression smokeObjective: Verify the installed PR hive/templates remain usable for project creation. Steps:
Evidence:
Observations:
Summary
Overall ResultISSUES FOUND Recommendations
Artifacts
|
IEvangelist
reviewed
May 27, 2026
Member
IEvangelist
left a comment
IEvangelist
left a comment
There was a problem hiding this comment.
Code review: found 1 CLI output correctness issue.
`aspire --info` renders one heading line per discovered install via
`DisplayMarkdown($"**{row.Id}** {row.Kind}")`. For sidecar-less installs
`row.Id` falls back to `install.Path` (see
`InstallationInfoOutput.GetInstallId`), which on Windows looks like
`C:\Users\<user>\.aspire\bin\aspire.exe`. CommonMark treats any `\`
followed by ASCII punctuation as a backslash-escape and drops the
backslash, so the heading rendered as
`C:\Users\<user>.aspire\bin\aspire.exe` — silently corrupting the
diagnostic surface a user reaches for to figure out where their CLI
actually lives.
Escape ASCII punctuation in `row.Id` and `row.Kind` before they reach
the Markdown renderer. The escape covers the full CommonMark ASCII
punctuation range so other punctuation in unexpected paths, hive names,
or future install kinds doesn't reintroduce the same class of bug.
Round-trip test pins the contract: each test input is escaped, fed
through `MarkdownToSpectreConverter.ConvertToPlainText` (the same
renderer used for redirected/non-interactive `--info` output), and the
result must equal the original literal.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
What this does
aspire doctorisn't really the right home for the install / hive list. Move it to a new top-levelaspire --info.New:
aspire --infoaspire --info— text:Version,Channel, then the per-row install blockaspire --info --format json—{ version, channel, installs[] }aspire --info --self— text, running CLI onlyaspire --info --self --format json— bareInstallationInfoarray; this is the cross-version peer-probe contract consumed byPeerInstallProbeGone from
aspire doctorinstallationsfield indoctor --format jsondoctor --self/doctor --self --format jsonpeer-probe variantPeerInstallProbenow spawns peers with--info --self --format json, falling back to--versionfor binaries that predate--infoAlso in this PR (along for the ride)
route→sourceterminology rename across CLI code, scripts, and docs. On-disk wire strings are unchanged —script/pr/winget/brew/dotnet-tool/localhiveall stay literal; the sidecar JSON field name was alreadysource.InstallSource.Brew→InstallSource.Homebrew(C# only; wire string"brew"is preserved in serializer and parser).PathLookupHelper:aspire.exeno longer renders asaspire.EXEjust becausePATHEXTcontains.EXE. Linux / macOS paths are unaffected.Why this approach
--infois root-only / non-recursive.aspire run --inforunsrunand ignores--info; onlyaspire --infofires install enumeration. This avoids the recursive-option / subcommand-local--formatshadowing trap, whereaspire run --info --format jsonwould otherwise silently emit install text becauserun's own--formatswallows the token.Channel-read and discovery-walk failures exit 0, not non-zero.
--infois the surface a user reaches for to find out their binary is broken; crashing there would be self-defeating. Discovery failure emits akind: "discovery-failed"row carrying the IO error asstatusReason.Call-outs
Unified
--infoJSON shape. Both--info --format json(eachinstalls[]element) and--info --self --format json(bare array element) use the sameInstallationInforow:id,kind,path,canonicalPath,version,channel,source,hive,pathStatus,status,statusReason,managedBy. Nullable fields are omitted viaWhenWritingNull.status(lifecycle:ok/failed/notProbed/no install found) andpathStatus(PATH-axis:active/shadowed/notOnPath) are orthogonal — consumersswitchon each axis independently. Orphan-hive rows havepath: null+kind: "orphan-hive"and carry the hive directory inhive. Full spec indocs/specs/cli-output-formats.md.--self/--formatwithout--infoproduce a parse error rather than silently falling through to grouped help.aspire doctor --format jsonstill routes correctly.--format tableis rejected by the value parser; the enum stays{ List, Json }.listis the explicit spelling of the default text rendering.Checklist