-
Notifications
You must be signed in to change notification settings - Fork 672
Fix EnvironmentPropagationFilter to filter current process environment variables correctly #10902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…nOptions with comprehensive tests Co-authored-by: mitchdenny <[email protected]>
@@ -441,7 +442,10 @@ public virtual async Task<int> ExecuteAsync(string[] args, IDictionary<string, s | |||
{ | |||
foreach (var envKvp in env) | |||
{ | |||
startInfo.EnvironmentVariables[envKvp.Key] = envKvp.Value; | |||
if (options.EnvironmentPropagationFilter is null || options.EnvironmentPropagationFilter(envKvp.Key, envKvp.Value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - the env
argument here is for extra arguments that the caller of ExecuteAsync
wants to add to the arguments launching the process. What we need to do here is iterate over all of the existing environment variables for the current process and then copy them into the startInfo.EnvironmentVariables collection but first filter them through options.EnvironmentPropogationFilter
.
Then we layer the additional environment variables pass in via the env
variable on top of those (no need to pass them through the filter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 8a79e1b. The EnvironmentPropagationFilter now correctly:
- First iterates over current process environment variables via
Environment.GetEnvironmentVariables()
- Filters those through the
EnvironmentPropagationFilter
function - Adds filtered current process env vars to
startInfo.EnvironmentVariables
- Then layers additional environment variables from the
env
parameter on top without filtering
This matches the expected behavior where the filter controls which current process environment variables get propagated, while additional environment variables passed explicitly by the caller are always included.
…t variables instead of additional env vars Co-authored-by: mitchdenny <[email protected]>
Fixes the
EnvironmentPropagationFilter
property inDotNetCliRunnerInvocationOptions
to correctly filter environment variables from the current process rather than only filtering additional environment variables passed by the caller.Issue
The initial implementation incorrectly applied the filter only to the
env
parameter (additional environment variables), but the intended behavior was to filter the current process's environment variables that get propagated to the child dotnet process.Solution
The implementation now correctly:
Environment.GetEnvironmentVariables()
EnvironmentPropagationFilter
functionProcessStartInfo.EnvironmentVariables
env
parameter on top (without filtering, since these are explicitly provided by the caller)Usage
Benefits
env
parameter are always includednull
, all current process variables are propagated as beforeThis change ensures that the filter serves its intended purpose of controlling which environment variables from the current execution context reach the spawned dotnet processes, while preserving the ability for callers to explicitly set additional variables.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.