Keep launch-configuration AppHost targets out of the workspace default - #19126
Keep launch-configuration AppHost targets out of the workspace default#19126Adam Ratzman (adamint) wants to merge 16 commits into
Conversation
Keep explicit --apphost selection one-shot while preserving implicit and prompted selection persistence. Add focused CLI contract coverage and a real Extension Host launch-configuration regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ffeff87e-f284-434d-87d3-843e21a7aebb
Keep explicit launch-configuration targets session-scoped while preserving persistence for directory discovery, direct CLI selection, and extension user selection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ffeff87e-f284-434d-87d3-843e21a7aebb
…launch-config-persistence
Move the explicit-launch-configuration check out of RunCommand and into ProjectLocator.CreateSettingsFileAsync, the single point that writes aspire.config.json. A VS Code launch configuration can name run, publish, deploy, or do, and every one of those commands resolved its AppHost with createSettingsFile: true, so the workspace default was still clobbered for all commands other than run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da1d8af7-a583-4a0c-b434-3dd5d6feb7ff
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19126Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19126" |
There was a problem hiding this comment.
Pull request overview
Prevents explicit VS Code launch configurations from changing the workspace’s persisted default AppHost.
Changes:
- Adds AppHost-selection provenance across the extension-to-CLI boundary.
- Suppresses configuration persistence for explicit launch targets.
- Adds unit and extension E2E regression coverage.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs |
Supplies CLI configuration to ProjectLocator. |
tests/Aspire.Cli.Tests/Projects/ProjectLocatorTests.cs |
Tests origin-based persistence behavior. |
tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs |
Verifies commands request centralized persistence. |
src/Shared/KnownConfigNames.cs |
Defines the provenance environment variable. |
src/Aspire.Cli/Projects/ProjectLocator.cs |
Suppresses persistence for explicit launch configurations. |
extension/src/utils/environment.ts |
Adds the environment-variable identifier. |
extension/src/types/extensionApi.ts |
Adds E2E named-configuration launching. |
extension/src/testing/e2eStateFileBridge.ts |
Implements the new E2E control command. |
extension/src/test/aspireDebugSession.test.ts |
Tests provenance forwarding to the CLI. |
extension/src/test/aspireDebugConfigurationProvider.test.ts |
Tests provenance classification and preservation. |
extension/src/test/appHostLaunchService.test.ts |
Verifies user-selection classification. |
extension/src/test-e2e/debugDashboard.e2e.test.ts |
Covers alternating named launch configurations. |
extension/src/services/AppHostLaunchService.ts |
Marks UI-selected AppHosts. |
extension/src/server/interactionService.ts |
Classifies RPC-started debug sessions. |
extension/src/debugger/AspireDebugSession.ts |
Passes provenance through the CLI environment. |
extension/src/debugger/AspireDebugConfigurationProvider.ts |
Classifies launch targets by origin. |
extension/src/debugger/AspireDebugConfigurationMetadata.ts |
Defines internal provenance metadata. |
extension/src/dcp/types.ts |
Types the provenance field. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Address code review of the AppHost selection-origin work. The debug configuration provider was registered once for both the Dynamic and Initial trigger kinds. VS Code serializes an Initial provider's result verbatim into a newly created launch.json, so the internal selection-origin marker was written into a user-owned file alongside a concrete AppHost program. Because resolve-time classification short-circuits when the marker is already present, such an entry stayed pinned to 'default-discovery' and kept rewriting the workspace default AppHost -- reproducing issue microsoft#19080 for launch configurations the extension itself generated. Register separate provider instances per trigger kind and only stamp the marker on ephemeral Dynamic configurations. An entry that lands in launch.json is now classified when it is resolved, so a generated configuration naming a specific AppHost is correctly treated as explicit. Also narrow the directory downgrade. Any directory-valued program used to be demoted to 'default-discovery', so a hand-written per-AppHost directory configuration still clobbered the workspace default. Only a program pointing at the workspace folder root delegates back to discovery now. Test fixes: create the secondary AppHost candidate inside the try block and remove it in suite teardown so a throw cannot leak it into later tests, give the multi-configuration test its own timeout, and have the E2E control command wait for VS Code to see a newly written launch configuration before starting it by name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da1d8af7-a583-4a0c-b434-3dd5d6feb7ff
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/debugger/AspireDebugConfigurationProvider.ts:90
- The documented contract says any
programthat resolves to a directory is downgraded todefault-discoveryand therefore persists the selected AppHost. This condition only does that for the workspace root; AppHost subdirectories remainexplicit-launch-configuration(as the new test also codifies). Please align the implementation and tests with the stated directory behavior, or update the PR description if subdirectories are intentionally session-scoped.
if (aspireConfig[appHostSelectionOriginConfigKey] === 'explicit-launch-configuration' && this.isWorkspaceFolderRoot(program, folder)) {
// Only a program pointing at the workspace folder root delegates the choice back to
// normal discovery, which is what the extension's own default configuration does. A
// configuration naming a specific AppHost file *or* subdirectory is scoped to that
// target and must not become the workspace default.
aspireConfig[appHostSelectionOriginConfigKey] = 'default-discovery';
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Second round of code review fixes. Suppressing the write outright meant a repo whose only AppHost was launched from a generated launch.json never got an aspire.config.json at all. The startup prompt from checkForExistingAppHostPathInWorkspace is not gated on ambiguity, so those users would be asked to create the file on every window open instead of having the first launch settle it. Narrow the rule to what issue microsoft#19080 actually reports: an explicit launch configuration must not *replace* a workspace default the user already has. It may still establish one when there is nothing to preserve, and it still heals a default whose AppHost no longer exists, since there is no choice left to protect in either case. Alternating between per-AppHost launch configurations remains stable, which is the behavior being fixed. Also lower the new waitForLaunchConfiguration budget to 5s. At 15s it sat above the 10s test-side control-command timeout, so its diagnostic was unreachable and a timed-out test would leave the handler running long enough to start a debug session behind the test's own teardown. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da1d8af7-a583-4a0c-b434-3dd5d6feb7ff
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Aspire.Cli/Projects/ProjectLocator.cs:155
- This summary contradicts the persistence rule below: an explicit launch target can become the workspace default when no valid default exists; it only must not replace an existing valid default. Describing it as “never” becoming the default could lead future changes to break the intentional first-launch behavior.
/// <summary>
/// Identifies a CLI invocation whose AppHost target came from an editor launch configuration
/// (for example a VS Code <c>launch.json</c> entry with an explicit <c>program</c>). Such a target
/// is owned by the individual debug session, so it must never become the workspace default.
/// </summary>
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The explicit-launch-configuration check asked whether a recorded default existed, but it did not read that default the way the rest of the CLI does. Two gaps: - It loaded the config with a plain Load, which does not see a legacy .aspire/settings.json workspace. A workspace that had never been migrated therefore looked like it had no recorded default, so a launch configuration was free to write one -- the exact overwrite microsoft#19080 reports. LoadOrCreate cannot be called from a read-only predicate because it persists the migration as a side effect, so the config the upward search already loaded is threaded out and reused instead. - It resolved the recorded path with Path.GetFullPath alone, unlike the canonical readers, which normalize separators first. A config committed from Windows carries backslashes, so on a Unix checkout the recorded path was read as a literal filename. The predicate also no longer requires the recorded file to exist. A missing file is indistinguishable from a branch switch or a sparse checkout, and treating it as "nothing to preserve" would let the next launch permanently re-point a default the user still wants. Stale entries are still healed by every other selection origin, which is where a deliberate choice comes from. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da1d8af7-a583-4a0c-b434-3dd5d6feb7ff
ASPIRE_CLI_APPHOST_SELECTION_ORIGIN describes one CLI invocation: it tells ProjectLocator that this run's AppHost was named by a VS Code launch configuration and therefore must not rewrite the workspace default. The extension sets it on the CLI process, but ProcessExecutionFactory only stripped the ASPIRE_CLI_* identity overrides, so every child inherited the marker -- `dotnet build`, the AppHost itself, and the env block the CLI hands to the extension for an IDE-launched AppHost. Any nested `aspire` invocation inside that tree would then believe its own target came from the outer launch configuration and silently skip recording its workspace default. Strip the marker alongside the identity overrides in both factory overloads. The (fileName, args, env, ...) overload applies caller env after the strip, so AppHostLauncher re-adds the marker for its detached child CLI, which continues the same logical invocation and re-resolves the AppHost. The ProcessStartInfo overload serves only AppHost/guest spawn paths, so it always strips. Tests cover both directions: the marker is removed from a parent-inherited block and from an explicitly populated ProcessStartInfo, and it survives on the detached-child-CLI path spawned with AppHostLauncher's own env and options. Both strip tests fail against the previous behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Aspire.Cli/Projects/ProjectLocator.cs:1081
- This process-scoped handoff is being read through the merged
IConfiguration, but workspace/global settings are registered after environment variables (Program.cs:299-312,ConfigurationHelper.cs:73-83) and therefore take precedence. A same-named top-level setting can make an explicit launch rewrite the default, or make ordinary CLI invocations suppress persistence. Read this internal signal from the existingIEnvironment.GetEnvironmentVariable(...)abstraction instead, and remove the newIConfigurationconstructor plumbing.
var isExplicitLaunchConfiguration = string.Equals(
configuration[KnownConfigNames.CliAppHostSelectionOrigin],
ExplicitLaunchConfigurationSelectionOrigin,
StringComparison.OrdinalIgnoreCase);
src/Aspire.Cli/Projects/ProjectLocator.cs:156
- This documentation contradicts the implemented “establish, but never replace” rule:
CreateSettingsFileAsyncintentionally allows this origin to become the default when none exists. Describe that exception so future callers do not incorrectly suppress all persistence.
/// Identifies a CLI invocation whose AppHost target came from an editor launch configuration
/// (for example a VS Code <c>launch.json</c> entry with an explicit <c>program</c>). Such a target
/// is owned by the individual debug session, so it must never become the workspace default.
/// </summary>
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
tests/Aspire.Cli.Tests/DotNet/ProcessExecutionFactoryEnvironmentTests.cs:21
- These tests mutate process-wide environment variables, but this class is not serialized with the repository's existing
EnvVarMutatingTestCollection. While the overrides on lines 42–43 or 78 are active, an unrelated test can snapshotASPIRE_CLI_APPHOST_SELECTION_ORIGINintoIConfigurationor a child process and unexpectedly suppress its config write. Put this class in the same non-parallel collection used by the otherEnvVarOverridesuites.
public sealed class ProcessExecutionFactoryEnvironmentTests
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Balanced
TryGetRecordedAppHostDefault resolved appHost.path only to hand the result to a
LogDebug argument: once the File.Exists check was dropped, every caller consumed
it through an `is { }` null test. The resolution could not change an outcome, but
it could still throw.
GetAppHostProjectFileFromSettingsAsync guards its resolution with
IsValidConfiguredAppHostPath, which rejects NUL bytes and other invalid
characters that survive JSON parsing precisely because Path.GetFullPath throws
ArgumentException on them (microsoft#17624). This helper had no such guard. The upward
search resolves the config it finds and would throw first, but the
`recordedConfig ??= AspireConfigFile.Load(...)` fallback reaches the
working-directory config in the case where the AppHost sits outside that config's
tree -- a config nothing else in this method had ever resolved. An explicit
--apphost never runs the settings reader, so no earlier validation applies.
Return the recorded path as written instead. Presence is the whole decision, the
raw string is the more useful log value because it is what appears in the file,
and the throw path disappears with the dead code. The config directory is now
logged separately so the message still says which file was consulted.
The remarks no longer claim stale entries are healed by every other origin. The
extension's startup prompt only checks that appHost.path is present, never that
it resolves, so a default left stale by a rename is repaired only by a selection
the user actually makes. That is the real cost of preferring preservation, and it
belongs in the remarks rather than an overstated reassurance.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: da1d8af7-a583-4a0c-b434-3dd5d6feb7ff
The "establish, but never replace" rule added in this PR is a check-then-act across processes: CreateSettingsFileAsync reads whether the workspace already records an AppHost default, decides on that basis, and then rewrites the whole config file. A VS Code compound launch configuration starts every AppHost it lists at the same moment, so two CLI processes can both observe "no default recorded" and both establish one, and one whole-file write can land on top of the other's. Wrap the body of CreateSettingsFileAsync in the CLI's existing cross-process FileLock, keyed on an XxHash3 of the symlink-resolved config root so AppHosts that share a config file serialize while unrelated workspaces never block each other. Locking the whole body rather than just the decision also covers the legacy .aspire/settings.json migration, which is another unsynchronized whole-file write on the same path. The lock file lives under the CLI cache directory rather than the workspace so read-only workspaces still work and nothing transient shows up in git status. Exclusion is enforced by the OS on every platform we ship (share mode on Windows, advisory flock on Linux and macOS) and released when the holder exits, so there is no stale lock to recover. Failing to lock degrades to the previous unsynchronized behavior rather than failing the user's command, since recording the default is bookkeeping around the command they actually asked for. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Aspire.Cli/Projects/ProjectLocator.cs:156
- This summary contradicts the implemented “establish, but never replace” rule: an explicit launch target does become the workspace default when none exists. Describe that exception here so future changes do not incorrectly suppress establishment altogether.
/// is owned by the individual debug session, so it must never become the workspace default.
tests/Aspire.Cli.Tests/DotNet/ProcessExecutionFactoryEnvironmentTests.cs:21
- This class mutates process-wide environment variables with
EnvVarOverridebut is not in the repository’s non-parallel environment-variable collection. xUnit can run it alongside CLI tests that build configuration from the environment, causing them to transiently observeASPIRE_CLI_APPHOST_SELECTION_ORIGINand suppress persistence. JoinEnvVarMutatingTestCollection, as done byInstallationDiscoveryDiscoverAllTests.cs:28andNpmRunnerTests.cs:15.
public sealed class ProcessExecutionFactoryEnvironmentTests
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Balanced
CreateSettingsFileAsync carried the settings file, the loaded config, that config's directory, and the scoped-config AppHost directory as four independent locals through discovery, legacy migration, preservation and writing. Every correctness question in this method is about whether they still agree -- whether the config that decided "the workspace already has a default" is the one about to be overwritten, and whether the directory a relative AppHost path is resolved against is the one it will be stored in -- and nothing in the shape of the code enforced that. Move discovery into ResolveWorkspaceConfigTarget, which is now the only producer of a WorkspaceConfigTarget, and derive the config root from the settings file rather than tracking it separately. Disagreement between the loaded config and the file that gets written is now unrepresentable, including across the legacy migration that rebases the config root onto the parent of .aspire/. The preservation check and the write path consume that result instead of the loose locals, which also removes the lazy re-load the preservation path needed to cover the case where discovery had not loaded anything. No behavior change: the resolution method performs the same search in the same order and returns null where the old code returned early. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Aspire.Cli/Projects/ProjectLocator.cs:1331
- On case-insensitive macOS volumes, differently cased paths can identify the same config directory but produce different lock hashes here. That bypasses the cross-process serialization and allows compound launches to both establish/rewrite the same default. Canonicalize case on macOS as well; on a case-sensitive macOS volume, an occasional false collision only serializes unrelated writes and does not affect correctness.
if (OperatingSystem.IsWindows())
{
normalizedRoot = normalizedRoot.ToLowerInvariant();
}
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The lock key added for concurrent default establishment was case-folded only on Windows, but macOS ships a case-insensitive volume by default. Two launches that spell one config root with different casing resolve to the same aspire.config.json while hashing to different lock file names, so they never contend and both establish the default -- exactly the race the lock exists to prevent. Resolving symlinks canonicalizes links but not casing, so it does not close the gap. Fold on every platform instead. On a genuinely case-sensitive volume this can only make two distinct roots share one lock, which briefly over-serializes a critical section measured in milliseconds; not folding lets two processes that share a config file miss each other entirely. A lock has to fail toward blocking. Probing the volume for case sensitivity would be more precise but adds IO to every launch and could answer differently in two processes, which is the one thing a lock key must never do. Extract the key derivation so the invariant is directly testable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CreateSettingsFileAsync loaded aspire.config.json itself to decide whether the workspace already had an AppHost default to preserve, which meant a second loader with its own legacy .aspire/settings.json reasoning next to the IConfigurationService call the same method already makes for sdk.version. Read appHost.path (and the legacy appHostPath spelling) through GetConfigurationFromDirectoryAsync instead, mirroring that pair, and drop the config the resolved workspace target used to carry. Also add 'agent-selection' to the selection-origin taxonomy. Agent and language-model tools launch an AppHost the user never named, so they get the same establish-but-never-replace treatment as an explicit launch configuration. Membership lives in one set in ProjectLocator so the persistence policy stays in one place, and AppHostLaunchService.launch now takes the origin from its caller instead of always stamping 'user-selection'. ProjectLocatorTests' configuration double answered directory-scoped reads with null, which would have made every preservation assertion vacuous; it now delegates them to the real ConfigurationService, whose global settings file is already sandboxed inside the temporary workspace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
tests/Aspire.Cli.Tests/DotNet/ProcessExecutionFactoryEnvironmentTests.cs:21
- These tests mutate process-wide environment variables via
EnvVarOverride, but this class is not in the assembly's nonparallelEnvVarMutatingTestCollection. While lines 42/78 set the selection origin, any concurrently running CLI test that buildsIConfigurationcan observeexplicit-launch-configurationand unexpectedly suppress settings writes. Add this class to the existing collection defined atInstallationDiscoveryDiscoverAllTests.cs:1037.
public sealed class ProcessExecutionFactoryEnvironmentTests
src/Aspire.Cli/Projects/ProjectLocator.cs:157
- This summary contradicts the implemented “establish, but never replace” rule: lines 1114-1128 intentionally allow an explicit launch target to become the default when none exists. Describe the actual constraint as not replacing an already-recorded default so future maintenance does not suppress the required establish behavior.
/// <summary>
/// Identifies a CLI invocation whose AppHost target came from an editor launch configuration
/// (for example a VS Code <c>launch.json</c> entry with an explicit <c>program</c>). Such a target
/// is owned by the individual debug session, so it must never become the workspace default.
/// </summary>
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Aspire.Cli/Projects/ProjectLocator.cs:1159
- This second lookup still cannot see a sibling legacy
.aspire/settings.jsonwhenaspire.config.jsonexists but omitsappHost.path:GetConfigurationFromDirectoryAsyncstops at the modern file before checking legacy, whereas the canonical AppHost reader falls back to legacy in that case. In such split-layout workspaces, an explicit launch misses the recorded default and writes its target into the modern file. Use a local AppHost-specific presence lookup that checks the modern path and then the sibling legacy path.
?? await configurationService.GetConfigurationFromDirectoryAsync(LegacySettingsAppHostPathKey, configRootDirectory, cancellationToken: cancellationToken);
src/Aspire.Cli/Projects/ProjectLocator.cs:1158
- This lookup falls back to the global settings file, but global
appHost.pathvalues are explicitly ignored (Program.WarnIfGlobalSettingsContainAppHostPath, lines 199-210). A leftover global value therefore makesrecordedDefaultnonempty and prevents an explicit launch from establishing the missing workspace default, reintroducing the recurring-prompt behavior this PR is intended to preserve. Read only the local config target here, without the global fallback.
This issue also appears on line 1159 of the same file.
var recordedDefault = await configurationService.GetConfigurationFromDirectoryAsync(AspireConfigAppHostPathKey, configRootDirectory, cancellationToken: cancellationToken)
src/Aspire.Cli/Projects/ProjectLocator.cs:156
- This documentation contradicts the implemented “establish, but never replace” policy: an explicit launch target may become the workspace default when none exists. State that it must not replace an existing default instead.
/// is owned by the individual debug session, so it must never become the workspace default.
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
The typed WorkspaceConfigTarget makes the settings file and its base directory a single value, but nothing asserted that the value stays correct once the three resolution branches diverge. The recorded AppHost path is stored relative, so it is only meaningful if the branch that chose the config file and the base directory the path was made relative to agree. Add a theory covering all three branches: a modern config at the workspace root, a legacy .aspire/settings.json that must be rebased onto aspire.config.json in its parent, and a config beside the AppHost that wins over the working directory. Each case resolves the recorded value against the directory of the file it actually landed in and requires it to lead back to the selected AppHost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Aspire.Cli/Projects/ProjectLocator.cs:156
- This summary contradicts the implemented “establish, but never replace” rule: lines 1136–1141 intentionally allow an explicit launch configuration to become the workspace default when none exists. Describe that exception here so future changes do not turn this into blanket suppression.
/// is owned by the individual debug session, so it must never become the workspace default.
tests/Aspire.Cli.Tests/Projects/ProjectLocatorTests.cs:119
- This test only proves that session-scoped origins do not replace an already-recorded default; such origins do persist when no default exists, as the test at line 152 demonstrates. Rename it to reflect the narrower contract and avoid contradicting the establish-but-never-replace behavior.
public async Task UseOrFindAppHostProjectFileDoesNotPersistSelectionFromSessionScopedOrigins(string selectionOrigin)
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
…aunch-config-persistence
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/Aspire.Cli/Projects/ProjectLocator.cs:1159
GetConfigurationFromDirectoryAsyncfalls back to the global settings file (ConfigurationService.cs:403-419), but global AppHost paths are explicitly ignored by the CLI (Program.cs:199-210). A user with a stale globalappHost.pathand no workspace default will therefore hit this branch and skip the “establish” write, leaving the workspace without a default and restoring the recurring activation prompt this PR is intended to avoid. Read only the resolved workspace target here; the legacy layout has already been migrated byResolveWorkspaceConfigTarget.
var recordedDefault = await configurationService.GetConfigurationFromDirectoryAsync(AspireConfigAppHostPathKey, configRootDirectory, cancellationToken: cancellationToken)
?? await configurationService.GetConfigurationFromDirectoryAsync(LegacySettingsAppHostPathKey, configRootDirectory, cancellationToken: cancellationToken);
src/Aspire.Cli/Projects/ProjectLocator.cs:157
- This documentation contradicts the implemented “establish, but never replace” rule: an explicit launch target does become the workspace default when none exists. Describe it as unable to replace an existing default so future changes do not incorrectly suppress the establish path.
/// <summary>
/// Identifies a CLI invocation whose AppHost target came from an editor launch configuration
/// (for example a VS Code <c>launch.json</c> entry with an explicit <c>program</c>). Such a target
/// is owned by the individual debug session, so it must never become the workspace default.
/// </summary>
tests/Aspire.Cli.Tests/DotNet/ProcessExecutionFactoryEnvironmentTests.cs:21
- These tests mutate process-wide environment variables, so running them in parallel with other CLI tests can transiently mark unrelated
ProjectLocatoror child-process tests asexplicit-launch-configuration. The repository’s existing convention is to place everyEnvVarOverridetest inEnvVarMutatingTestCollection, whose collection definition disables parallelization (InstallationDiscoveryDiscoverAllTests.cs:20-25,1037-1039; seeNpmRunnerTests.cs:15). Add this class to that collection to prevent nondeterministic cross-test contamination.
public sealed class ProcessExecutionFactoryEnvironmentTests
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Fixes #19080
Problem
A repo with several AppHosts gives each one its own VS Code launch configuration, pointing
programat a different AppHost. Launching any of them rewritesaspire.config.jsonto name that AppHost as the workspace default. Alternating between two configurations means the file is rewritten on every launch, so it shows up as a modified file in every commit and every AppHost-agnostic command follows whichever one ran last.Picking a target for one debug session is not a statement about what the workspace defaults to, but the CLI could not tell the two apart. Every path into
UseOrFindAppHostProjectFileAsyncpassedcreateSettingsFile: true, so any resolved selection was persisted.Approach
The extension classifies where an AppHost selection came from and forwards it to the CLI in
ASPIRE_CLI_APPHOST_SELECTION_ORIGIN:explicit-launch-configurationlaunch.jsonnamed this AppHostdefault-discoveryuser-selectionThe CLI rule is establish, but never replace. An explicit launch configuration may write the default when the workspace has none, and is otherwise a no-op.
The "establish" half matters. Blanket suppression looked cleaner but regressed single-AppHost repos:
checkForExistingAppHostPathInWorkspaceprompts on every activation while no config exists, and previously the first F5 wrote the file and ended the prompting. I confirmed on the real binary that suppressing the write turns a self-resolving flow into a recurring prompt.The gate lives in
ProjectLocator.CreateSettingsFileAsync, the single writer ofappHost.pathfor selection persistence.launch.jsoncan namerun,deploy,publish, ordo, and all of them reach this method, so gatingRunCommandalone would have left the other three rewriting the file.Without the environment variable the behavior is unchanged.
Notable details
launch.jsonnever gets stamped. VS Code serializes anInitial-kind provider'sprovideDebugConfigurationsresult verbatim into a newlaunch.json. Stamping the origin there would bakedefault-discoveryinto a user-owned file and pin it forever, reproducing Launching aspire from different vscode launch configurations (using the extension) overrides the aspire.config.json file #19080 for extension-generated configurations. Only theDynamicprovider stamps, so the two are now registered as separate instances..aspire/settings.jsonworkspace is not mistaken for having no default, and it normalizes separators so a config committed from Windows is understood on a Unix checkout.aspire config set— every path that carries a deliberate choice.Testing
ProjectLocatorTestsandRunCommandTests: 168 tests, 0 failures. New coverage for each origin, for establishing a default, for preserving one across alternating launches (a direct Launching aspire from different vscode launch configurations (using the extension) overrides the aspire.config.json file #19080 repro), for a legacy.aspire/settings.jsonworkspace, for a Windows-authored path, and for a recorded default whose file is gone.runandpublish, with and without the variable, alternating launches, the single-AppHost establish path, and healing throughuser-selection.Pre-existing issue found while testing (not fixed here)
A hand-authored
appHost.pathcontaining a NUL byte (or anotherPath.GetInvalidPathChars()character) crashesaspire runwithAn unexpected error occurred: Null character in path.This is the same class of bug as #17624. The canonical readers guard their resolution withIsValidConfiguredAppHostPath, but the upward config search inCreateSettingsFileAsyncdoes not.It reproduces on
mainwith no environment variable set, and this PR adds no new unguarded resolution —git diffagainst the merge base shows zero addedPath.GetFullPathorPath.IsPathRootedcalls. Flagging it because anyone probing the new preservation logic with a hostile config will hit it, and it should not be read as a regression from this change. Tracked separately as #19137.