Fix five aspire ls bugs from #17620 (L1–L5)#17631
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17631Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17631" |
There was a problem hiding this comment.
Pull request overview
This PR fixes several aspire ls settings/discovery regressions around legacy settings migration, missing-path warnings, streamed ordering, invalid configured paths, and symlink-aware duplicate detection.
Changes:
- Stops startup settings registration from eagerly migrating legacy
.aspire/settings.jsonintoaspire.config.json. - Updates AppHost settings lookup to validate configured paths and dedupe symlink-equivalent candidates.
- Documents
aspire ls --streamas arrival-ordered and adds regression tests for the fixed cases.
Reviewed changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Cli/Utils/ConfigurationHelper.cs |
Removes eager migration during settings registration. |
src/Aspire.Cli/Program.cs |
Updates call site for the new RegisterSettingsFiles signature. |
src/Aspire.Cli/Projects/ProjectLocator.cs |
Adds invalid-path validation, warning behavior updates, and symlink-aware dedupe. |
src/Shared/PathNormalizer.cs |
Adds symlink-resolution helper. |
src/Aspire.Cli/Commands/LsCommand.cs |
Removes dead stream-mode sort and documents arrival-order behavior. |
src/Aspire.Cli/Resources/ErrorStrings.resx |
Adds invalid AppHost path error string. |
src/Aspire.Cli/Resources/ErrorStrings.Designer.cs |
Adds generated accessor for the new error string. |
src/Aspire.Cli/Resources/SharedCommandStrings.resx |
Updates --stream option description. |
src/Aspire.Cli/Resources/xlf/ErrorStrings.*.xlf |
Adds localized placeholders for the new error string. |
src/Aspire.Cli/Resources/xlf/SharedCommandStrings.*.xlf |
Updates localized placeholders for the stream option description. |
docs/specs/cli-output-formats.md |
Documents streamed output ordering. |
tests/Aspire.Cli.Tests/Configuration/ConfigurationHelperTests.cs |
Updates tests for non-mutating startup registration. |
tests/Aspire.Cli.Tests/Commands/LsCommandTests.cs |
Adds stream arrival-order regression coverage. |
tests/Aspire.Cli.Tests/Projects/ProjectLocatorTests.cs |
Adds warning, invalid-path, and symlink-dedupe regression tests. |
tests/Aspire.Cli.Tests/Utils/PathNormalizerTests.cs |
Adds symlink-resolution unit tests. |
tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs |
Updates test helper for the settings registration signature change. |
Files not reviewed (1)
- src/Aspire.Cli/Resources/ErrorStrings.Designer.cs: Language not supported
8b34c07 to
3869490
Compare
Fixes microsoft#17615, microsoft#17620, microsoft#17621, microsoft#17624, microsoft#17626. - L1 (microsoft#17615): Remove the eager-migration block in ConfigurationHelper.RegisterSettingsFiles. Read commands like `aspire ls` no longer silently materialize an aspire.config.json next to a user's legacy .aspire/settings.json. Migration now happens lazily/explicitly via the existing write paths. - L2 (microsoft#17620): Drop the `silent` parameter from ProjectLocator.GetAppHostProjectFileFromSettingsAsync so the legacy branch unconditionally surfaces the migration warning, and surface the actual user-authored `.aspire/settings.json` path in the warning text rather than the auto-created `aspire.config.json` path. - L3 (microsoft#17621): Remove the dead post-emission `appHosts.Sort()` in LsCommand.FindAppHostsWithJsonStreamAsync (--stream emits candidates as they are discovered, so the sort had no effect on already-emitted output). Update the --stream option description and docs/specs/cli-output-formats.md to declare the arrival-ordered contract. - L4 (microsoft#17624): Add an IsValidConfiguredAppHostPath helper in ProjectLocator that rejects `\0` and Path.GetInvalidPathChars() before the path is passed to Path.IsPathRooted / Path.Combine. Wired into both the modern `aspire.config.json` (`appHost.path`) branch and the legacy `.aspire/settings.json` (`appHostPath`) branch. Validation is intentionally at the consumption point rather than in AspireConfigFile.Load, which has 12+ unrelated callers. Adds a new ConfiguredAppHostPathHasInvalidCharacters resource string and refreshes the xlf set via UpdateXlf. - L5 (microsoft#17626): Add PathNormalizer.ResolveSymlinks in src/Shared, a recursive segment-walker that canonicalizes intermediate symlinks (Directory.ResolveLinkTarget only reads exactly the path it is given, and returns the link target as stored on disk — so a single call on /tmp/x/y.cs does not unwrap /tmp -> /private/tmp, and following a link whose stored target is /var/.../app keeps the un-canonical /var prefix). The recursion has a hard depth limit of 40 and falls back to the un-resolved input on broken or circular links. Use it in AddSettingsAppHostCandidateAsync as a comparison key only — the surfaced AppHostProjectCandidate keeps its original FileInfo so the displayed path matches what the user authored in settings. Tests: 158 of 160 targeted tests pass (2 Windows-only skipped on macOS). New tests cover L1 (no migration on read), L2 (legacy warning references settings.json), L3 (arrival-order under --stream), L4 (NUL byte in modern and legacy branches), L5 (symlink dedupe via a node_modules-hosted link the discovery walk excludes), plus 5 unit tests on PathNormalizer.ResolveSymlinks itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3869490 to
ee4ff12
Compare
🧪 Local validation resultsBuilt and tested the CLI from this branch ( Targeted regression: L1–L5
Adversarial probe (15 attacks)After addressing review feedback, I ran a focused adversarial pass against the same code paths. One additional in-scope bug was found and fixed in this PR. Full attack table (click to expand)
Bug found and fixed mid-review (ATTACK C): Empty Test results
Filed follow-ups (out-of-scope for this PR)
Notes
|
JamesNK
left a comment
There was a problem hiding this comment.
Solid set of fixes. One minor nit about a missing diagnostic log in a silent catch path — otherwise LGTM.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| capabilities.Add(capability.CapabilityId, capability); | ||
| } | ||
|
|
||
| private static bool CapabilitiesAreEquivalent(AtsCapability left, AtsCapability right) => |
There was a problem hiding this comment.
I actually think we should fail in every case the same id is found, even if the types are the same. We want to fix it, not keep these collisions
There was a problem hiding this comment.
I understand it's not your PR that introduced it (a recent Foundry one). Other PRs are blocked by the same issue. We can just wait for #17671 to be merged
Match the duplicate ATS capability parser/test/baseline shape from PR microsoft#17631 to avoid merge conflicts between the branches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR microsoft#17671 owns the Foundry ATS baseline update; this branch should only contain the AppHost and CLI fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/backport to release/13.4 |
|
Started backporting to |
Description
This PR fixes the five
aspire lsbugs catalogued in #17620 (L1–L5). They are all consequences of how the new settings/discovery pipeline interacts with the legacy.aspire/settings.json, the modernaspire.config.json, parallel discovery, and macOS-style symlinks.Fixes #17615
Fixes #17620
Fixes #17621
Fixes #17624
Fixes #17626
What changes for users
aspire ls(and any other read command) no longer silently creates anaspire.config.jsonnext to an existing.aspire/settings.json. Read commands no longer mutate the workspace; migration is performed only by the explicit write paths..aspire/settings.jsonnow references the file the user actually authored, instead of the auto-createdaspire.config.jsonthey have never seen.aspire ls --format json --streamnow correctly documents and emits candidates in arrival order from the parallel discovery walk (the previous dead post-emissionSort()had no effect and only confused readers).Path.GetInvalidPathChars()) inappHost.path/appHostPathnow surfaces a clear, localized error instead of crashing with the generic “An unexpected error occurred: Null character in path.”/tmp -> /private/tmp),aspire lsno longer lists the same apphost twice — once from the discovery walk and once from the settings file pointing at it via the symlink.User-facing usage
The CLI surface is unchanged; only behavior under existing scenarios is corrected.
aspire ls --streamis now documented as arrival-ordered:A bad
appHost.pathnow produces a useful, localized error rather than a stack-trace–style message:Implementation notes
ConfigurationHelper.RegisterSettingsFilesno longer eagerly migrates.aspire/settings.json→aspire.config.json. Migration is still available through the normal write paths.ProjectLocator.GetAppHostProjectFileFromSettingsAsyncdrops thesilentparameter; the legacy branch unconditionally surfaces the warning, and the message now uses the user-authored settings.json path.appHosts.Sort()inLsCommand.FindAppHostsWithJsonStreamAsync; updated the resx (LsStreamOptionDescription) anddocs/specs/cli-output-formats.md. xlf set refreshed viaUpdateXlf.IsValidConfiguredAppHostPathhelper inProjectLocatorvalidates against\0andPath.GetInvalidPathChars()beforePath.Combine/Path.IsPathRooted. Called in both the modern (appHost.pathfromaspire.config.json) and legacy (appHostPathfrom.aspire/settings.json) branches. Validation is intentionally at the consumption point rather than inAspireConfigFile.Load, which has 12+ unrelated callers that should not be impacted. NewConfiguredAppHostPathHasInvalidCharactersresource string.PathNormalizer.ResolveSymlinks(string path)insrc/Shared. It walks each path segment withDirectory.ResolveLinkTarget/File.ResolveLinkTarget(returnFinalTarget: true). The critical subtlety is thatResolveLinkTargetreturns the link target as stored on disk — so a link whose target is/var/.../appretains the un-canonical/varprefix, even when the rest of the path has already been canonicalized through/var -> /private/var. To produce a canonical form regardless of which side of the comparison reached the file first, the helper recursively canonicalizes each resolved target, with a hard depth limit of 40 to defend against pathological chains, and falls back to the input on broken or circular links.AddSettingsAppHostCandidateAsyncuses the resolved paths as a comparison key only — the surfacedAppHostProjectCandidatekeeps its originalFileInfoso the displayed path matches what the user authored.Tests
PathNormalizer.ResolveSymlinks(idempotence, empty input, final-file symlink, intermediate-directory symlink, broken-link fallback).ConfigurationHelperTestsrewritten to verify noaspire.config.jsonis written when only legacy settings are present.ProjectLocatorTestsadds a regression that the legacy warning referencessettings.json, notaspire.config.json.LsCommandTestsadds an arrival-order test using non-alphabetical names (Z/A/M) so any incidental sort would fail.ProjectLocatorTestsadds two NUL-byte tests covering both the modern and the legacy branch.ProjectLocatorTestsadds an integration test that places the symlink innode_modules(excluded fromDefaultFiltereddiscovery) so the walk surfaces only the canonical path while the settings file references the symbolic path. The test verifies dedupe collapses them to a single entry.Tests on a clean checkout: 158/160 targeted tests pass; 2 Windows-only tests are skipped on macOS as expected.
Checklist
<remarks />and<code />elements on your triple slash comments?