Make SdkResolver-provided environment variables take precedence over ambient environment by Copilot · Pull Request #12655 · dotnet/msbuild

Copilot · 2025-10-16T19:31:56Z

Summary

This PR implements the feature requested in the issue where SdkResolver-provided environment variables now take precedence over ambient environment variables from the host process.

Changes Made:

ProjectInstance.cs: Modified AddSdkResolvedEnvironmentVariable to:
- Add null parameter validation for name and value using ErrorUtilities
- Remove the check that prevented SDK env vars from overriding ambient env vars
- Remove ambient env vars from the _environmentVariableProperties dictionary when SDK sets them
- Log when an SDK env var overrides an ambient one
Project.cs: Modified AddSdkResolvedEnvironmentVariable to:
- Add null parameter validation for name and value using ErrorUtilities
- Remove the check that prevented SDK env vars from overriding ambient env vars
- Always call SetProperty to override ambient values
SdkResult.cs: Fixed serialization bug by:
- Adding _environmentVariablesToAdd to the Translate method
- Updating Equals method to compare _environmentVariablesToAdd
- Updating GetHashCode to include _environmentVariablesToAdd
Strings.resx: Added new log message "SdkEnvironmentVariableOverridingAmbient"
Tests: Added two new tests:
- SdkResolverEnvironmentVariablesOverrideAmbientEnvironmentVariables: Verifies SDK values override ambient env vars
- FirstSdkEnvironmentVariableWinsOverSubsequentSdks: Verifies first SDK wins among multiple SDKs

Behavior:

SDK env vars > Ambient env vars: SDK-resolved environment variables now override any ambient environment variables
First SDK wins: Among multiple SDKs, the first one to set an environment variable wins
XML properties still prioritized: Regular properties defined in XML are still not overridden by SDK env vars
Null parameters rejected: Both name and value parameters are validated to prevent null values

All existing SdkResultEvaluation tests pass, confirming backward compatibility is maintained.

Original prompt

This section details on the original issue you should resolve

<issue_title>SdkResolver-provided env vars should take precedence over the ambient environment</issue_title>
<issue_description>### Summary

Today the SdkResolver system can provide environment variables to the engine when an SDK is resolved. The primary usage of this feature is

to keep invocations of the SDK unified across MSBuild-driven tooling via the DOTNET_HOST_PATH environment variable

to drive usage of the .NET TaskHost feature for spawning .NET-targeted MSBuild tasks in a .NET TaskHost from a .NET Framework central node

However, the variables provided by this mechanism are currently overridden by ambient environment variables taken from the snapshot of the host process at the time the build is started. This means that external processes can override the SdkResolver's decisions.

Background and Motivation

Internal users are having some churn in mixed VS installs and need to set DOTNET_HOST_PATH in older VS environments to mitigate that churn, but want to defer to this feature in dev18 onwards by default.

Proposed Feature

We want the resolver to be the complete source of truth, so we should change the way the SdkResolver env vars are applied to the overall environmental state. The SdkResolver env vars should override any ambient env vars.

We need to make this change, update the existing tests, and add new tests pinning this behavior.

Alternative Designs

No response</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #12654

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

dotnet-policy-service · 2025-10-16T19:32:15Z

Hello @@copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo.

baronfel · 2025-10-16T20:20:57Z

The package vuln thing that's plaguing other builds is hitting this one too, so it's hard to tell if this change actually passes tests.

Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>

Copilot

Pull Request Overview

This PR fixes the precedence order for SDK resolver environment variables, making them override ambient environment variables from the host process as originally intended, rather than being overridden by them.

Key Changes:

Modified ProjectInstance.cs and Project.cs to allow SDK-resolved environment variables to override ambient environment variables
Fixed a serialization bug in SdkResult.cs where _environmentVariablesToAdd wasn't being translated for out-of-proc scenarios
Added logging when SDK variables override ambient ones through a new resource string SdkEnvironmentVariableOverridingAmbient

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
ProjectInstance.cs	Removed check preventing SDK env vars from overriding ambient vars; added logic to remove conflicting ambient vars and log overrides
Project.cs	Removed check preventing SDK env vars from overriding ambient vars; SDK values now always override ambient values
SdkResult.cs	Fixed serialization bug by adding translation for `_environmentVariablesToAdd`; updated `Equals` and `GetHashCode` to include the field
Strings.resx	Added new log message resource string for SDK variable overrides
Strings.*.xlf	Added translations (marked as "new") for the new log message across all localization files
SdkResultEvaluation_Tests.cs	Added two tests verifying SDK env vars override ambient vars and first SDK wins when multiple SDKs set the same variable

Copilot · 2025-10-20T16:11:28Z

src/Build/Instance/ProjectInstance.cs

+            // Set the property, overriding ambient environment variables but not regular properties defined in XML.
+            // If we're overriding an ambient variable, or if the property doesn't exist, set it.
+            // Otherwise, prioritize regular properties defined in XML.
+            if (overridingAmbient || GetProperty(name) is null)


The logic in the conditional at line 1414 needs clarification. When overridingAmbient is true, GetProperty(name) will never return null because the ambient property already exists (it was just removed from _environmentVariableProperties). This means the GetProperty(name) is null check is only meaningful when overridingAmbient is false. Consider simplifying this to make the logic clearer, such as checking GetProperty(name) is null first, then handling the override case separately.

Copilot · 2025-10-20T16:11:28Z

src/Build/BackEnd/Components/SdkResolution/SdkResult.cs

@@ -112,6 +113,7 @@ public override bool Equals(object obj)
                  _additionalPaths?.Count == result._additionalPaths?.Count &&
                  _propertiesToAdd?.Count == result._propertiesToAdd?.Count &&
                  _itemsToAdd?.Count == result._propertiesToAdd?.Count &&


The equality check is comparing _itemsToAdd?.Count with result._propertiesToAdd?.Count instead of result._itemsToAdd?.Count. This is a copy-paste error that makes the equality comparison incorrect.

Suggested change

_itemsToAdd?.Count == result._propertiesToAdd?.Count &&

_itemsToAdd?.Count == result._itemsToAdd?.Count &&

YuliiaKovalova · 2025-10-20T16:30:01Z

src/Build/Instance/ProjectInstance.cs

-            // Only set the local property if it does not already exist, prioritizing regular properties defined in XML.
-            if (GetProperty(name) is null)
+            // SDK-resolved environment variables override ambient environment variables.
+            bool overridingAmbient = _environmentVariableProperties.Contains(name);


is null-check needed?

@copilot ensure that null environment variable names are handled in the Project and ProjectInstance handling

Added null checks for both name and value parameters in AddSdkResolvedEnvironmentVariable methods in ProjectInstance.cs and Project.cs. The validation uses ErrorUtilities.VerifyThrowArgumentLength(name) and ErrorUtilities.VerifyThrowArgumentNull(value) to match the pattern used in other MSBuild property-setting methods. (commit c24c943 updated)

Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>

Copilot AI assigned Copilot and baronfel Oct 16, 2025

Copilot started work on behalf of baronfel October 16, 2025 19:32 View session

Copilot AI changed the title ~~[WIP] Fix SdkResolver environment vars precedence over ambient variables~~ Make SdkResolver-provided environment variables take precedence over ambient environment Oct 16, 2025

Copilot AI requested a review from baronfel October 16, 2025 20:01

Copilot stopped work on behalf of baronfel due to an error October 16, 2025 20:01
An unexpected error occurred. For more details, see the detailed logs in GitHub Actions.

baronfel mentioned this pull request Oct 17, 2025

Known Issue: .NET 10.0.100-rc.2 can fail to launch the C# compiler when used in Visual Studio 2022 #12669

Closed

Fix SDK environment variable precedence to override ambient env vars

5b852f6

Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>

baronfel force-pushed the copilot/fix-env-vars-precedence-issue branch from c24c943 to 5b852f6 Compare October 20, 2025 15:14

baronfel approved these changes Oct 20, 2025

View reviewed changes

baronfel marked this pull request as ready for review October 20, 2025 16:10

Copilot AI review requested due to automatic review settings October 20, 2025 16:10

Copilot AI reviewed Oct 20, 2025

View reviewed changes

baronfel requested review from YuliiaKovalova and rainersigwald October 20, 2025 16:17

YuliiaKovalova reviewed Oct 20, 2025

View reviewed changes

Copilot started work on behalf of baronfel October 21, 2025 19:51 View session

Add null parameter validation for SDK environment variables

6493c49

Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>

Copilot AI requested a review from baronfel October 21, 2025 20:04

Copilot finished work on behalf of baronfel October 21, 2025 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SdkResolver-provided environment variables take precedence over ambient environment#12655

Make SdkResolver-provided environment variables take precedence over ambient environment#12655
Copilot wants to merge 2 commits intovs18.0from
copilot/fix-env-vars-precedence-issue

Copilot AI commented Oct 16, 2025 •

edited

Loading

Uh oh!

dotnet-policy-service bot commented Oct 16, 2025

Uh oh!

baronfel commented Oct 16, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 20, 2025

Uh oh!

Copilot AI Oct 20, 2025

Uh oh!

YuliiaKovalova Oct 20, 2025

Uh oh!

baronfel Oct 21, 2025

Uh oh!

Copilot AI Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	_itemsToAdd?.Count == result._propertiesToAdd?.Count &&
	_itemsToAdd?.Count == result._itemsToAdd?.Count &&

Conversation

Copilot AI commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes Made:

Behavior:

Background and Motivation

Proposed Feature

Alternative Designs

Comments on the Issue (you are @copilot in this section)

Uh oh!

dotnet-policy-service bot commented Oct 16, 2025

Uh oh!

baronfel commented Oct 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

YuliiaKovalova Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

baronfel Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Oct 16, 2025 •

edited

Loading