Fix explicit dashboard run options not overriding inherited environment values - #19968
Open
Muhammad Bilal (MuhammadBilal64) wants to merge 5 commits into
Open
Muhammad Bilal (MuhammadBilal64) wants to merge 5 commits into
Muhammad Bilal (MuhammadBilal64) wants to merge 5 commits into
Conversation
Fixes microsoft#19779 DashboardRunCommand.AddStringOptionArg and AddBoolOptionArg checked for an existing environment value before checking whether the user explicitly supplied a CLI option, so an inherited ASPIRE_DASHBOARD_* environment variable always won over an explicit --persistence or --application-name flag. Both methods now check for an explicit CLI option first and only fall back to the environment-check early return when no explicit option was supplied. Added test coverage for both the override case and the existing fallback-to-environment case.
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19968Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19968" |
Copilot started reviewing on behalf of
Muhammad Bilal (MuhammadBilal64)
September 6, 2026 11:08
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The anonymous-access precedence issue can break authentication, and Boolean override regression coverage is required.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes dashboard CLI option precedence so explicit values override inherited environment variables.
Changes:
- Prioritizes explicit string and Boolean options.
- Adds injectable test environment support.
- Adds string-option regression tests.
File summaries
| File | Review |
|---|---|
tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs |
Supports custom test environments. |
tests/Aspire.Cli.Tests/Commands/DashboardRunCommandTests.cs |
Adds string-option tests, but lacks Boolean-option regression coverage. |
src/Aspire.Cli/Commands/DashboardRunCommand.cs |
Corrects option precedence, but explicit --allow-anonymous false can suppress required credential generation when inherited environment values conflict. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+224
to
+226
| var result = parseResult.GetResult(option); | ||
| var isExplicit = result is not null && !result.Implicit; | ||
| if (!isExplicit && ConfigSettingHasValue(unmatchedTokens, environment, envVarName)) |
Comment on lines
+224
to
+226
| var result = parseResult.GetResult(option); | ||
| var isExplicit = result is not null && !result.Implicit; | ||
| if (!isExplicit && ConfigSettingHasValue(unmatchedTokens, environment, envVarName)) |
ExecuteAsync determined whether to generate the browser token and API key using a separate ConfigSettingHasValue check that only asked whether the environment variable existed, not what the effective value actually was. An explicit --allow-anonymous false overriding an inherited ASPIRE_DASHBOARD_UNSECURED_ALLOW_ANONYMOUS=true environment value would forward the correct false to the child process, but the CLI itself would still skip generating credentials, leaving the now-secured dashboard without auth material. ExecuteAsync now resolves the effective value via the same precedence used elsewhere in the file (forwarded arg, then unmatched token, then environment), so credential generation matches what's actually forwarded. Added regression tests for both the boolean override path and credential generation in that scenario.
Copilot started reviewing on behalf of
Muhammad Bilal (MuhammadBilal64)
September 6, 2026 13:40
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Effective Boolean resolution must include unmatched arguments and final forwarding order, with a conflicting-input regression test.
Review details
Suppressed comments (1)
src/Aspire.Cli/Commands/DashboardRunCommand.cs:128
ResolveSettingValuedoes not actually follow the child process's precedence here. It checks the typeddashboardArgsbefore space-separated unmatched tokens, and becausedashboardArgs.AddRange(unmatchedTokens)happens later, it misses unmatched--ASPIRE_DASHBOARD_UNSECURED_ALLOW_ANONYMOUS=valueand bare-boolean forms entirely. Since unmatched tokens are appended last and intentionally win, conflicting inputs can make credential generation disagree with the dashboard (for example,--allow-anonymousplus an unmatched--ASPIRE_DASHBOARD_UNSECURED_ALLOW_ANONYMOUS=falseomits credentials even though the child enables authentication). Resolve the effective boolean by scanning the final forwarded argument order, including=, space-separated, and bare forms, and add a conflicting typed/unmatched regression test.
var allowAnonymousValue = ResolveSettingValue(dashboardArgs, unmatchedTokens, _environment, KnownConfigNames.DashboardUnsecuredAllowAnonymous);
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ous values Adds a test verifying that when a typed --allow-anonymous option and a conflicting unmatched --ASPIRE_DASHBOARD_UNSECURED_ALLOW_ANONYMOUS token are both present, the unmatched token's value wins in the final forwarded arguments and credential generation matches it, confirming the resolution added in the previous commit correctly follows the codebase's stated precedence.
Copilot started reviewing on behalf of
Muhammad Bilal (MuhammadBilal64)
September 6, 2026 14:35
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Argument resolution does not preserve last-wins ordering across supported argument forms.
Review details
Suppressed comments (1)
src/Aspire.Cli/Commands/DashboardRunCommand.cs:129
ResolveSettingValuechecks all--KEY=valuearguments before checking space-separated unmatched tokens, so it does not actually preserve the child process's last-wins order. For example,--allow-anonymous --ASPIRE_DASHBOARD_UNSECURED_ALLOW_ANONYMOUS falseleaves the generated=trueargument visible to the first scan, returnstrue, and skips credential generation even though the later raw argument makes the dashboard run with anonymous access disabled. Resolve both argument forms in one ordered pass (including a bare boolean key), and cover the space-separated conflict alongside the=falsecase.
var allowAnonymousValue = ResolveSettingValue(dashboardArgs, unmatchedTokens, _environment, KnownConfigNames.DashboardUnsecuredAllowAnonymous);
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Fix dashboard arg resolution: collapse ResolveArgValue's two-pass short-circuit into ResolveSettingValue's single ordered scan so space-separated --KEY value overrides aren't silently ignored in favor of an earlier --KEY=value forward. Adds regression test for the space-separated conflicting allow-anonymous case. Validation: DashboardRunCommandTests — 51/51 passed.
Copilot started reviewing on behalf of
Muhammad Bilal (MuhammadBilal64)
September 6, 2026 16:26
View session
Author
|
Hi James Newton-King (@JamesNK) David Fowler (@davidfowl) Mitch Denny (@mitchdenny), gentle reminder on this PR when you get a chance. I’ve addressed the previous review feedback and added the requested regression coverage. Would appreciate a review. Thanks! |
Author
|
Hi James Newton-King (@JamesNK) David Fowler (@davidfowl) Mitch Denny (@mitchdenny), following up on #19968 since it’s still awaiting a maintainer review. The requested Copilot feedback has been addressed, and the latest review recommends approval. Would appreciate a review when you get a chance. Thanks! |
This branch has not been deployed
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.
Description
DashboardRunCommand.AddStringOptionArgandAddBoolOptionArgchecked whether an environment variable already had a value before checking if the user explicitly supplied a CLI option. If the environment variable existed, the method returned early and never forwarded the explicit--persistenceor--application-nameflag, so the inherited environment value always won — even when the user explicitly passed a different value on the command line.Both methods now check for an explicit CLI option first and only fall back to the environment-check early return when no explicit option was supplied, so explicit CLI options correctly take precedence.
Added test coverage for both the override case (explicit option beats environment value) and the existing correct fallback case (environment value is still used when no explicit option is given).
Fixes #19779
Checklist