Expose resolved environment variables to debug launch producers - #19077
Conversation
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19077Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19077" |
|
Self-review notes (posted as a comment since GitHub does not allow reviewing your own PR). I ran a deep review pass over the full
CI is green (337 passing). The remaining items below are open; the core design and the migration itself look sound. High
Medium
Low
Public API assessmentShape is right: replacing the two Two smaller points: Coverage assessmentWell covered: fresh context per replica and per restart; Flaky-pattern check is clean — Gaps: items 1–4 above, plus |
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Two findings from a review of the launch-configuration lifecycle move: one behavior regression for MAUI resources, and one leftover async signature.
- 1 correctness/behavior issue (
ProjectLaunchArgsOverrideAnnotationguard disables the MAUI producer) - 1 API-shape cleanup (
PrepareObjectsAsyncis no longer asynchronous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f29216a-bf33-40a3-9f37-3863b3d1a79e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3253974d-4f18-486f-863c-281a607656d4
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Aspire.Hosting/Dcp/ExecutableCreator.cs:204
- For a project with
ProjectLaunchArgsOverrideAnnotation, the debug args callback is now intentionally skipped, butRewritesArgumentsForDebuggingremainstruebecause anargsCallbackwas registered. A failing supported non-project producer therefore bypasses this catch and aborts startup even though the override left a valid process command. Allow the process fallback when a project launch-args override is present (and add the faulting-producer case to the new MAUI regression test).
catch (Exception exception) when (
!isProjectLaunchConfiguration
&& !supportsDebuggingAnnotation.RewritesArgumentsForDebugging)
src/Aspire.Hosting/ResourceBuilderExtensions.cs:4858
- This new public overload throws
ArgumentNullExceptionforbuilderandlaunchConfigurationProducer, but its XML documentation omits that exception. Public Aspire APIs must document thrown exceptions.
/// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Supplying an argsCallback sets RewritesArgumentsForDebugging, and the exception filter in ExecutableCreator deliberately excludes those resources from the process fallback that plain debuggable executables get. Falling back there could start a resource with half-applied debug arguments, so a producer fault has to fail the resource instead: the exception escapes ExecutableCreator, DcpExecutor reports the resource as failed to start, and no Executable is ever created. Every existing producer-fault test omits argsCallback, so that rule had no coverage. Adds a theory over both producer overloads asserting the producer runs once, the args callback never runs, and no Executable is created. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f1bd51d5-cb9f-4370-a07b-c94416baad61
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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. |
…-launch-config-context
David Pine (IEvangelist)
left a comment
There was a problem hiding this comment.
1 correctness issue found.
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedChange Categories
Summary of changes
Key testing considerationThe new behavior fires on the IDE/debug executable-creation path. A plain Test Scenarios ExecutedScenario 1: Feature validation — core hosting unit tests (PR source)Objective: Validate the new context, the new overload, env-var flow, restart/replica reuse, and MAUI process-mode-override behavior. Command: Notable tests covered:
Observations: The 1 skipped test is Scenario 2: Feature validation — Dotnet "project" producer tests (PR source)Objective: Validate the Command: Scenario 3: No-regression smoke —
|
| Case | Where validated | Result |
|---|---|---|
| Resource without debug support | DebugSupportExtensionsTests.CreateLaunchConfigurationThrowsWhenTheResourceHasNoDebugSupport |
✅ InvalidOperationException ("does not declare debug launch support") |
project launch type without project metadata |
...ThrowsWhenTheResourceHasNoProjectMetadata |
✅ InvalidOperationException ("has no project metadata") |
| Producer returns null | ...ThrowsWhenTheProducerReturnsNull |
✅ InvalidOperationException ("returned null", names resource) |
| Task/ValueTask-returning sync producer bound to sync overload | ExecutableResourceBuilderExtensionTests.WithDebugSupportRejects{ATask,AValueTask}ReturningSynchronousProducer |
✅ ArgumentException with updated guidance message |
Scope Notes
- Go / JavaScript (Bun+Node) / Python / MAUI producer test projects were not rebuilt separately: their diffs are mechanical signature adaptations (
LaunchConfigurationAnnotator(exe, mode, ct)→(exe, context)), and the one substantive per-language behavior change (MAUI under a process-mode override) is already covered inDcpExecutorTestsabove. Running them would require Go/Node/Bun/Python runtimes and MAUI workloads for little additional signal. - Container mode was unavailable (Docker Desktop not running); all scenarios ran locally. The starter used no container resources, so this did not limit the smoke test.
Summary
| Scenario | Status | Notes |
|---|---|---|
| Core hosting unit tests (292) | ✅ Passed | 0 failed, 1 Linux-only skip |
| Dotnet project producer tests (2) | ✅ Passed | 0 failed |
CLI start/restart/stop smoke |
✅ Passed | Both projects Running/Healthy; dashboard up |
Overall Result
✅ PR VERIFIED
- 294 unit tests passed (0 failed, 1 expected Linux-only skip) across the core DCP executor, debug-support, and Dotnet project-producer paths — directly exercising the new
LaunchConfigurationCallbackContext, the additive context-basedWithDebugSupportoverload, the resolved-environment flow (evaluated once), and the MAUI process-mode-override behavior. - Dogfood CLI (
13.6.0-pr.19077.g629c3ea1, matching head) shows no regression in normal AppHost orchestration, including restart.
Recommendations
- None blocking. Feature and no-regression behavior verified. IDE-attached debug behavior (VS/VS Code actually consuming the resolved environment variables) is inherently outside CLI scope and is covered here by the DCP executor unit tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b9961b8-65d7-4919-82a9-05b90a62a1b7
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Aspire.Hosting/ApplicationModel/DebugSupportExtensions.cs:117
- The method still promises to create “the launch configuration that this resource sends to the IDE,” but a context-based producer receives an empty environment here while executable creation supplies resolved values. A producer that depends on
EnvironmentVariablescan therefore throw or return different metadata through this public inspection API. Please either expose a way to inspect with caller-supplied resolved values, or explicitly change the method’s summary/return contract to say the result is only a synthetic configuration and may differ from what is sent.
/// This inspection API does not resolve the resource's environment variables. A producer that accepts a
/// <see cref="LaunchConfigurationCallbackContext"/> receives an empty
/// <see cref="LaunchConfigurationCallbackContext.EnvironmentVariables"/> collection. Aspire invokes that producer
/// separately with resolved values when it creates the executable.
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s):
Suggested actions:
You can re-run the failed jobs from the workflow run page. |
Description
Debug launch configuration producers can now read environment variables after Aspire has resolved their value providers. This fixes #18956 without exposing a second mutable argument model or moving every existing integration to a new callback shape.
The new
LaunchConfigurationCallbackContextcontains the launch mode, resource, resolved environment variables, and cancellation token. A new context-basedWithDebugSupportoverload is additive; the existing synchronous and asynchronous mode-based overloads remain unchanged.Aspire creates the context after resolving the executable environment and before producing IDE launch metadata. Restarts and replicas therefore receive the values for that executable creation, and environment providers are not evaluated again by the launch producer.
The executable creation path also continues running non-project launch producers, including MAUI, when a project launch-args override keeps execution in process mode.
Security considerations
Resolved environment variables can contain secrets. The callback runs as AppHost/integration code in the existing AppHost process, and the context is not serialized automatically. Producers should copy only values required by the IDE into the returned launch configuration.
Validation
Fixes #18956
Checklist
<remarks />and<code />elements on your triple slash comments?