Version the AppHost build-ownership capability so no launch runs stale output - #19132
Closed
Adam Ratzman (adamint) wants to merge 11 commits into
Closed
Version the AppHost build-ownership capability so no launch runs stale output#19132Adam Ratzman (adamint) wants to merge 11 commits into
Adam Ratzman (adamint) wants to merge 11 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 -- 19132Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19132" |
Contributor
There was a problem hiding this comment.
Pull request overview
Versions the CLI–VS Code AppHost build-ownership handshake to prevent stale launches.
Changes:
- Introduces the exact
.v2capability contract on both sides. - Adds CLI, extension unit, and E2E regression coverage.
- Wires the new E2E test into CI; the brace-glob spec currently breaks the existing
Test-Pathvalidation.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Cli/Utils/ExtensionHelper.cs |
Advertises the v2 capability. |
src/Aspire.Cli/Projects/DotNetAppHostProject.cs |
Requires v2 before CLI build ownership. |
extension/src/capabilities.ts |
Advertises and centralizes the v2 token. |
extension/src/server/interactionService.ts |
Requires v2 before skipping extension builds. |
tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs |
Tests build ownership across launch modes and versions. |
tests/Aspire.Cli.Tests/Commands/ConfigCommandTests.cs |
Verifies only v2 is advertised. |
extension/src/test/capabilities.test.ts |
Tests extension capability advertisement. |
extension/src/test/rpc/interactionServiceTests.test.ts |
Tests CLI compatibility behavior. |
extension/src/test-e2e/buildOwnership.e2e.test.ts |
Adds stale-output regression coverage. |
extension/CONTRIBUTING.md |
Documents the expanded E2E shard command. |
.github/workflows/extension-e2e-tests.yml |
Adds the regression test to Linux and Windows shards. |
Review details
Suppressed comments (1)
.github/workflows/extension-e2e-tests.yml:176
- This spec never reaches the E2E runner: the existing compile guard at lines 421-422 calls PowerShell
Test-Pathwithmatrix.spec, and PowerShell does not expand{debugDashboard,buildOwnership}brace alternatives. It therefore checks for a literal brace-named file and fails the Windows shard. Please make that guard validate glob matches (using the runner's matcher) or split these into individually valid matrix entries.
spec: 'out/test-e2e/test-e2e/{debugDashboard,buildOwnership}.e2e.test.js'
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Balanced
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Adam Ratzman (adamint)
pushed a commit
to adamint/aspire
that referenced
this pull request
Aug 7, 2026
…dshake Review feedback on microsoft#19132. The buildOwnership E2E spec was folded into the debug-dashboard shard as '{debugDashboard,buildOwnership}.e2e.test.js'. scripts/run-e2e.js expands brace alternation, but the workflow's compile guard does not: it calls PowerShell Test-Path, which only understands literal paths and the *, ? and [] wildcards. Verified locally -- Test-Path on the brace form returns False while the literal path returns True -- so the guard would have thrown "Compiled E2E shard spec was not found" and failed both debug-dashboard shards before the spec ever ran. Split it into its own build-ownership shard (Linux and Windows) instead of teaching the guard to glob. Every other matrix entry is one literal spec per shard, so this keeps the matrix uniform, restores debug-dashboard to what it was, and gives the new test its own runner, timeout budget and diagnostics artifact rather than extending an already long UI shard. A comment on the guard records why matrix specs must stay literal. CONTRIBUTING lists the new shard; its glob examples stay, since those run through run-e2e.js and not the guard. Also rewrote the capability comments on both sides. They claimed "whichever side advertises this promises the AppHost is built exactly once", which reads as a symmetric promise and contradicts BuildAppHostIfNeededAsync. The contract is asymmetric: the CLI advertising v2 is a promise ("I pre-build before every launch"), which is why InteractionService passes forceBuild: false, while an extension advertising v2 is a request ("you own the pre-build"), which is why the CLI builds only when it sees the token. Each comment now states its own side's meaning first, and both explain why the unversioned token could not carry the CLI-side promise. No behavior change; comments, workflow matrix and docs only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (2)
extension/src/capabilities.ts:40
- The “built exactly once” guarantee does not hold for project-based AppHosts with an
Executablelaunch profile. Inextension/src/debugger/languages/dotnet.ts:471-482,shouldBuildProjectis always true for non-file-based projects, so the extension callsbuildDotNetProjecteven when v2 madeforceBuildfalse, after the CLI has already pre-built. Since duplicate builds are explicitly acceptable here, document the actual no-skipped-build guarantee rather than an invariant the implementation does not provide.
* Either way the AppHost is built exactly once, and neither side has to know the other's version -
* only whether it speaks this contract.
src/Aspire.Cli/Utils/ExtensionHelper.cs:48
- The “built exactly once” guarantee does not hold for project-based AppHosts with an
Executablelaunch profile. Inextension/src/debugger/languages/dotnet.ts:471-482, the extension still callsbuildDotNetProjectwhenforceBuildis false, after this CLI has already pre-built. Please state the actual no-skipped-build guarantee here instead of promising exactly one build.
// Either way the AppHost is built exactly once, and neither side has to know the other's
// version -- only whether it speaks this contract.
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/server/interactionService.ts:650
forceBuild: falsedoes not actually transfer build ownership for a non-file AppHost that selects anExecutablelaunch profile. Inextension/src/debugger/languages/dotnet.ts:418,shouldBuildProjectis always true when!isFileBasedProject, and lines 480-482 then build unconditionally. A new CLI/new extension therefore builds twice on this supported path, contradicting the v2 contract's “extension skips its own build” guarantee. Make theExecutablebranch honorforceBuild === falsefor AppHosts and add the corresponding debugger test.
const cliBuiltProject = cliCapabilities.includes(buildDotnetUsingCliCapability);
const forceBuild = !cliBuiltProject;
return debugSession.startAppHost(projectFile, args, environment, debug, { forceBuild });
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
…e output The `build-dotnet-using-cli` capability was a plain token, not proof that the CLI actually pre-built the AppHost. CLI 13.2.0-13.2.4 advertised it unconditionally, but derived watch mode from `isExtensionHost && !StartDebugSession`, so a no-debug "Run with Aspire" launch turned watch on and skipped the pre-build entirely. Meanwhile the CLI never spawns `dotnet watch` in extension host mode -- it hands the launch to the extension over RPC -- so nothing rebuilt. An extension that trusted the token skipped its own build too, and the user silently ran stale output. That is the original microsoft#15850 symptom. Rename the capability to `build-dotnet-using-cli.v2` on both sides. Build ownership now transfers only when the peer understands the newer contract, which guarantees a pre-build on every launch (debug and no-debug). Matching is exact, not prefix-based, so a future revision has to opt in deliberately. Compatibility (the AppHost is built exactly once in every combination): | CLI | Extension | CLI pre-builds | Extension builds | |-----------|-----------|----------------|------------------| | <= 13.2.4 | <= 13.2.4 | debug only | no | | <= 13.2.4 | new | no | yes | | new | <= 13.2.4 | no | yes | | new | new | yes | no | Only the already-shipped old/old pair can still run stale output on the no-debug path; nothing either new side ships can influence that combination. Every combination involving new bits builds exactly once. Tests: the extension unit test asserts that a CLI advertising the legacy unversioned token still forces an extension build, and the CLI theory covers legacy/v2/v3 tokens across both debug and no-debug launches while asserting watch mode stays off. A new E2E test edits a running AppHost, relaunches, and requires the compiler error to surface instead of stale output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dshake Review feedback on microsoft#19132. The buildOwnership E2E spec was folded into the debug-dashboard shard as '{debugDashboard,buildOwnership}.e2e.test.js'. scripts/run-e2e.js expands brace alternation, but the workflow's compile guard does not: it calls PowerShell Test-Path, which only understands literal paths and the *, ? and [] wildcards. Verified locally -- Test-Path on the brace form returns False while the literal path returns True -- so the guard would have thrown "Compiled E2E shard spec was not found" and failed both debug-dashboard shards before the spec ever ran. Split it into its own build-ownership shard (Linux and Windows) instead of teaching the guard to glob. Every other matrix entry is one literal spec per shard, so this keeps the matrix uniform, restores debug-dashboard to what it was, and gives the new test its own runner, timeout budget and diagnostics artifact rather than extending an already long UI shard. A comment on the guard records why matrix specs must stay literal. CONTRIBUTING lists the new shard; its glob examples stay, since those run through run-e2e.js and not the guard. Also rewrote the capability comments on both sides. They claimed "whichever side advertises this promises the AppHost is built exactly once", which reads as a symmetric promise and contradicts BuildAppHostIfNeededAsync. The contract is asymmetric: the CLI advertising v2 is a promise ("I pre-build before every launch"), which is why InteractionService passes forceBuild: false, while an extension advertising v2 is a request ("you own the pre-build"), which is why the CLI builds only when it sees the token. Each comment now states its own side's meaning first, and both explain why the unversioned token could not carry the CLI-side promise. No behavior change; comments, workflow matrix and docs only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…once The comments I added in 9621821 claimed "the AppHost is built exactly once". That is not true on every path. In extension/src/debugger/languages/dotnet.ts shouldBuildProject is `!isFileBasedProject || !isApphost || forceBuild !== false`, so for a project-based AppHost the first term is already true and the Executable launch-profile branch calls buildDotNetProject unconditionally -- even when the CLI advertised v2 and InteractionService passed forceBuild: false. A new CLI paired with a new extension therefore builds twice on that path. Leaving the wording alone would have documented an invariant the implementation does not provide, which is the same class of mistake the version bump exists to fix. State the guarantee that actually holds instead: neither side skips the build believing the other did it, so no launch runs stale output. Redundant builds are tolerated and the reason is recorded -- that build compiles the launch profile's dependencies rather than the AppHost output, so a wasted incremental build is acceptable where a skipped one is not. Comment-only; no behavior change. Making the Executable branch honor forceBuild === false is a real behavior change to class-library integration launches and does not belong in this capability-versioning PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
The build-ownership E2E shard timed out at 180s waiting for a running AppHost on both Linux and Windows even though the debug console showed the AppHost reaching "Distributed application started". waitForWorkspaceAppHost() opens the E2E workspace folder on the first spec of a shard, which reloads the VS Code window back to the Explorer view. AppHostDataRepository only polls `aspire ps` while the Aspire panel is visible or an AppHost tab is open (its `_dataActive` gate), and `aspire ps` is the only source of `state.appHosts` -- the list every running-AppHost assertion reads. The captured extension log confirms it: `aspire ps --follow` is spawned in the pre-reload activation and never again in the post-reload workspace activation, and the failure screenshot shows the Explorer sidebar with the Aspire view unselected. Every other spec that waits for a running AppHost already reopens the view after the folder open; this one did not. Add the missing openAspireView() and a unit-level guard that scans the spec sources so the next spec cannot reintroduce the same hang. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d1cf760-b1bc-46ba-a6d4-628354b00f2c
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/server/interactionService.ts:647
- Mandatory: This cross-process regression still lacks VS Code E2E coverage. The new unit tests stub
getCliCapabilities()andstartAppHost()separately, so they cannot catch a mismatch in the real capability transport or the no-debug “Run with Aspire” launch—the integration that originally allowed stale output. Add an extension E2E test that launches without debugging, changes or breaks the AppHost source, relaunches, and verifies that the new build runs or its compiler failure is surfaced; existing E2E coverage only verifies command routing (extension/src/test-e2e/packageSurface.e2e.test.ts:225-265).
const cliBuiltProject = cliCapabilities.includes(buildDotnetUsingCliCapability);
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Adam Ratzman (adamint)
marked this pull request as ready for review
August 11, 2026 03:17
Adam Ratzman (adamint)
marked this pull request as draft
August 11, 2026 06:44
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.
Summary
Versions the CLI/VS Code build-ownership capability so an AppHost launch never skips the build because both sides believed the other side owned it.
Fixes #15850.
Changes
build-dotnet-using-clitoken with the exactbuild-dotnet-using-cli.v2contract on both sides of the backchannel.The E2E shard-matrix extraction is intentionally not part of this PR.
Validation
Checklist