Skip to content

Make env/--env set environment variables for the Dev Proxy process - #1995

Merged
waldekmastykarz merged 11 commits into
dotnet:mainfrom
waldekmastykarz:fix-1953-env-option
Oct 26, 2026
Merged

waldekmastykarz merged 11 commits into
dotnet:mainfrom
waldekmastykarz:fix-1953-env-option

Conversation

@waldekmastykarz

Copy link
Copy Markdown
Collaborator

Closes #1953

--env and the config file's env were described as "Variables to set for the Dev Proxy process", but they were only used to fill {var} placeholders in API spec files for the MinimalPermissions plugins.

Changes

  • Both the config file's env object and --env name=value values are now set as process environment variables in BuildApplication, right after the config file loads and before logging, services, and plugins are created. Plugins that read env vars, like RewritePlugin, ApiCenter, and the DEV_PROXY_CERT_PATH cert loading, see them. --env values take precedence over config values.
  • {var} substitution in the MinimalPermissions plugins is unchanged.
  • Added env to schemas/v4.0.0/rc.schema.json as an object with string values.
  • --env help updated. The validator now rejects entries with an empty name, like =value.

Testing

  • New EnvOptionTests cover CLI parsing and config/CLI precedence.
  • Checked by hand: a detached proxy started with --env CI=1 no longer logs the API token, because ApiSecurity reads CI. Without the flag, it does log the token.

…loses dotnet#1953

Values from the config file's `env` object and `--env` are now set as
process environment variables right after the config file is loaded,
before plugins are created. `--env` values take precedence. They are
still used to replace {name} placeholders in API spec files for the
MinimalPermissions plugins. Adds `env` to rc.schema.json and rejects
`--env` entries with an empty name.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 846278cd-1ffb-4a4b-a0df-57d766209c4b
Copilot AI balanced review requested due to automatic review settings October 21, 2026 14:03
@waldekmastykarz
waldekmastykarz requested a review from a team as a code owner October 21, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Removed overrides persist after hot reload, and detached startup can print the API token despite the configured suppression policy.

2 open issues
What changed in this PR

Makes config env and CLI --env values available as process environment variables, addressing #1953 while preserving MinimalPermissions placeholder substitution.

Changes:

  • Applies config variables before service initialization, with CLI values taking precedence.
  • Updates CLI help, empty-name validation, and the configuration schema.
  • Adds parsing and precedence tests.
File Description
schemas/​v4.0.0/​rc.schema.json Defines the env configuration object.
DevProxy/​Proxy/​ProxyConfiguration.cs Applies process environment variables.
DevProxy/​Program.cs Initializes environment overrides during startup.
DevProxy/​Commands/​DevProxyConfigOptions.cs Parses early CLI environment options.
DevProxy/​Commands/​DevProxyCommand.cs Clarifies help and rejects empty names.
DevProxy.Tests/​EnvOptionTests.cs Tests parsing and override precedence.

Review effort: Balanced

Comment thread DevProxy/Program.cs Outdated
Comment thread DevProxy/Proxy/ProxyConfiguration.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Environment overrides are not restored when entries are removed during hot reload.

2 open issues

Review effort: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Environment overrides can relocate management state and credentials, breaking detached discovery and final cleanup.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Retain management directory before applying environment overrides

DevProxy/​Program.cs:367

On Linux, --env XDG_CONFIG_HOME=/tmp (or the equivalent config entry) moves the child's state and credentials, but the detached launcher still polls the inherited directory and times out. Ordinary status/stop calls without that override cannot discover the instance, and restoring the environment before final cleanup makes cleanup target the wrong directory. Resolve and retain the management directory from the inherited environment before applying overrides, and use that stable path for state and credentials throughout the process lifetime.

2 resolved since last review

Review effort: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Addressed the management-directory finding in the latest Copilot review in ee53732. Per the agreed policy, startup resolves the management directory from the inherited environment before applying config/CLI overrides, and StateManager retains it for the process lifetime. State, credentials, and logs therefore stay discoverable by the detached launcher and ordinary status/stop commands, including after hot reload and final cleanup; the requested environment overrides still reach consumers.

Added config/CLI regression coverage for management paths during and after overrides. Validation: 46 unit tests pass, locked restore and full solution build pass with zero warnings/errors, and thirteen real detached-process scenarios pass on macOS (including status authentication, stop, and state/token cleanup with path overrides and hot reload). Linux runtime validation is not available locally; the new environment-path regression runs cross-platform.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Status output still bypasses the persisted suppression policy and can print an instance’s bearer token.

1 open issue

Review effort: Balanced

Comment thread DevProxy/ApiSecurity.cs
waldekmastykarz and others added 2 commits October 26, 2026 11:54
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved blocking issues remain, and focused regression coverage addresses override lifetime, management-path stability, and token-suppression propagation.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Environment application order can invalidate tilde-based config paths and leave outbound HTTP clients using inherited proxy settings.

2 open issues

Review effort: Balanced

Comment thread DevProxy/Program.cs
Comment thread DevProxy/Program.cs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Hot reload updates environment variables but leaves cached default HTTP proxy settings unchanged, so clients can continue using removed or replaced proxy settings.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Refresh cached HTTP proxy settings during hot reload

DevProxy/​Program.cs:371

Changing or removing HTTPS_PROXY or NO_PROXY in env during hot reload updates the process variables, but not .NET's process-wide cached HttpClient.DefaultProxy. The first run's announcement initializes that cache, and the restart loop rebuilds only the host, so even new HTTP clients, such as ApiCenterClient's default handler, keep using the old proxy/bypass settings. Refresh the default HTTP proxy from the effective environment on each run, or require a full process restart when these settings change.

2 resolved since last review

Review effort: Balanced

waldekmastykarz and others added 2 commits October 26, 2026 13:17
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Addressed the cached-default-proxy finding in review 5651713707 with the explicitly agreed policy in 886d24e: changes/removals to HTTP_PROXY, HTTPS_PROXY, ALL_PROXY or NO_PROXY (and Unix lowercase variants) require a full stop/start. Startup overrides continue to apply before the first HTTP request. During hot reload, the process compares the applied values to the first run and logs a clear restart warning with variable names only, never values. CLI help and the env schema document the limitation.

The cached-proxy observation is correct: a real detached reload from local proxy A to B kept sending the rebuilt host request through A. We are deliberately not implementing custom proxy resolution, private-runtime cache manipulation, or automatic process restarts; the user chose the documented full-process-restart requirement. This is the supported limitation, not a claim that hot reload refreshes HttpClient.DefaultProxy.

Regression coverage checks changed/removed uppercase and lowercase settings, unchanged CLI precedence, no startup warning, and a real reload warning including the stop/start instruction without proxy values. Validation: full solution locked restore/build (zero warnings/errors), 104 unit tests, nineteen focused integration cases, and thirteen existing detached lifecycle scenarios pass. Latest main 25fcff3, including the logging/filter changes, is integrated via f4a7087.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Hot reload updates NO_COLOR and TERM overrides without refreshing the cached console-color policy.

1 open issue

Review effort: Balanced

Comment thread DevProxy/Program.cs
Recompute the shared NO_COLOR/TERM policy instead of retaining the first host's values. Cover config override additions, changes, removals, inherited defaults, and CLI precedence.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes address environment precedence, restoration, startup ordering, and token-output handling with regression coverage and no unresolved blocking findings.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@waldekmastykarz
waldekmastykarz merged commit cece509 into dotnet:main Oct 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

env / --env doesn't set environment variables

2 participants