Skip to content

[fix] InProcessVsTestConsoleWrapper drops Windows drive-relative env vars (#15740)#15773

Draft
nohwnd wants to merge 4 commits into
mainfrom
fix/issue-15740-2bb8561774de7f51
Draft

[fix] InProcessVsTestConsoleWrapper drops Windows drive-relative env vars (#15740)#15773
nohwnd wants to merge 4 commits into
mainfrom
fix/issue-15740-2bb8561774de7f51

Conversation

@nohwnd
Copy link
Copy Markdown
Member

@nohwnd nohwnd commented May 14, 2026

Summary

🤖 This is an automated fix generated by Issue Repro Triage & Auto-Fix.

Fixes #15740

Root Cause

On Windows, Environment.GetEnvironmentVariables() (managed API) silently omits environment entries whose keys start with = (e.g. =C:=C:\path). These are drive-relative current directory entries maintained by the native Windows environment block (GetEnvironmentStringsW).

In the in-process vstest path, InProcessVsTestConsoleWrapper snapshots the current process environment via the managed API and passes it to the testhost process via ProcessHelper.ExternalEnvironmentVariables. Because the managed API omits =-prefixed entries, the testhost never receives them, causing drive-relative path resolution to behave differently in the testhost than in the host process.

The out-of-process path is unaffected: it uses ProcessStartInfo with OS-level inheritance, so the native environment block (including =-prefixed entries) is inherited automatically.

Fix

In InProcessVsTestConsoleWrapper.cs, after populating environmentVariableBaseline from the managed snapshot, supplement it on Windows with entries from the raw native environment block obtained via GetEnvironmentStringsW / FreeEnvironmentStringsW P/Invoke. Only =-prefixed entries are extracted from the native block; all other entries are already present in the managed snapshot.

Changes

  • src/vstest.console/InProcessVsTestConsoleWrapper.cs: Added GetWindowsNativeEqualsEnvironmentVariables() private static helper and called it when InheritEnvironmentVariables = true on Windows.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

On Windows, Environment.GetEnvironmentVariables() (managed API) silently
omits environment entries whose keys start with '=' (e.g. '=C:=C:\path').
These are drive-relative current directory entries maintained by the native
Windows environment block.

In the in-process vstest path, InProcessVsTestConsoleWrapper snapshots the
environment via the managed API and passes it to testhost via
ProcessHelper.ExternalEnvironmentVariables. As a result, these '='-prefixed
entries were dropped, causing drive-relative paths in testhost to resolve
incorrectly.

Fix: on Windows, supplement the managed snapshot with the native entries by
P/Invoking GetEnvironmentStringsW/FreeEnvironmentStringsW. Only '='-prefixed
entries are extracted from the native block; all other entries are already
captured by GetEnvironmentVariables().

Fixes #15740

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a Windows-specific environment inheritance gap in the in-process vstest execution path by ensuring =-prefixed drive-relative environment entries (present in the native environment block but omitted by Environment.GetEnvironmentVariables()) are forwarded to the testhost process.

Changes:

  • Supplement the managed environment snapshot with =-prefixed entries read from the native Windows environment block.
  • Add a private helper that P/Invokes GetEnvironmentStringsW / FreeEnvironmentStringsW and parses only =-prefixed variables.

Comment on lines +121 to +131
// On Windows, Environment.GetEnvironmentVariables() omits entries whose keys start
// with '=' (e.g. "=C:=C:\path" — drive-relative current directory entries kept by
// the native environment block). Supplement the managed snapshot with those entries
// so that testhost receives a complete environment.
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
foreach (var (key, value) in GetWindowsNativeEqualsEnvironmentVariables())
{
environmentVariableBaseline[key] = value;
}
}
Comment on lines +1325 to +1331
private static IEnumerable<(string Key, string? Value)> GetWindowsNativeEqualsEnvironmentVariables()
{
IntPtr block = GetEnvironmentStringsW();
if (block == IntPtr.Zero)
{
yield break;
}
}
else
{
// Key with no value (just "=key" or "=key=").
Copy link
Copy Markdown
Member Author

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

The fix is correct and well-scoped.

What was reviewed:

  • P/Invoke declarations for GetEnvironmentStringsW / FreeEnvironmentStringsW — valid; CA1416 is globally suppressed in Directory.Build.props, consistent with the rest of the codebase
  • yield return inside try/finally — valid C#; FreeEnvironmentStringsW is guaranteed to be called when the foreach disposes the enumerator
  • Entry parsing (IndexOf('=', 1) to skip the leading = and find the key/value separator) — correct for the canonical =C:=C:\path format
  • Block termination (zero-length string → break) — correctly handles the double-null Windows native environment block end
  • Offset arithmetic (offset * 2 for char→byte conversion) — correct; environment blocks are negligibly small compared to int.MaxValue / 2
  • Guard placement — supplement only runs inside InheritEnvironmentVariables = true branch and only on Windows

One minor nit (not blocking): the else branch comment says it handles "=key=" but that case is actually handled by the if branch (the else only covers "=key" with no second =). The code logic itself is correct.

🧠 Reviewed by Expert Code Reviewer 🧠

…ariables

The else branch only handles '=key' entries (no second '=').
Entries like '=key=value' are handled by the if branch above.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nohwnd
Copy link
Copy Markdown
Member Author

nohwnd commented May 14, 2026

Commit pushed: d746fcd

🔧 Iterated by PR Iteration Agent 🔧

Copy link
Copy Markdown
Member Author

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

The fix is correct and well-scoped. Re-confirmed the same findings as the prior review run:

  • P/Invoke declarations for GetEnvironmentStringsW / FreeEnvironmentStringsW are valid; CA1416 is globally suppressed in Directory.Build.props
  • yield return inside try/finally is valid — FreeEnvironmentStringsW is guaranteed to run when the enumerator is disposed, even on early iteration exit
  • IndexOf('=', 1) combined with separatorIndex > 0 correctly identifies the key/value separator (minimum found index is 1, not-found returns -1)
  • offset * 2 char→byte arithmetic is correct; environment blocks are negligibly small vs. int.MaxValue / 2
  • Guard placement is correct: supplement only runs inside InheritEnvironmentVariables = true and only on Windows
  • No public API surface changes, no IPC protocol changes, no binding redirect implications

🧠 Reviewed by Expert Code Reviewer 🧠

@nohwnd

This comment has been minimized.

On Windows, GetWindowsNativeEqualsEnvironmentVariables() adds '='-prefixed
drive-relative current-directory entries from the native environment block.
These are valid additions but cause the exact-count assertion to fail on CI.

Change the count check to exclude '='-prefixed keys, which are the native
entries added by the new code. The test still validates that all three
expected managed-API keys are present.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nohwnd
Copy link
Copy Markdown
Member Author

nohwnd commented May 16, 2026

Commit pushed: 359124f

🔧 Iterated by PR Iteration Agent 🔧

Copilot AI review requested due to automatic review settings May 16, 2026 05:47
@nohwnd

This comment has been minimized.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

int offset = 0;
while (true)
{
string entry = Marshal.PtrToStringUni(IntPtr.Add(block, offset * 2))!;
Copy link
Copy Markdown
Member Author

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

Reviewed the new test-fix commit (2026-05-16) added after the prior two review passes.

Activated dimensions (routed via src/vstest.console/): Environment Variable & Feature Flag Contracts · Process Architecture & Host Resolution · Null Safety & Boundary Validation

New commit analysis (InProcessVsTestConsoleWrapperTests.cs):

  • ?.Keys.Cast<string>().Count(...) — null-safe: ?. propagates through the chain; ?? 0 correctly handles a null dictionary. Cast<string>() is required because IDictionary.Keys returns a non-generic ICollection.
  • .Count(k => !k.StartsWith("=", StringComparison.Ordinal)) — correctly excludes the =-prefixed drive-relative entries that the new GetWindowsNativeEqualsEnvironmentVariables() adds on Windows. The subsequent ContainsKey assertions still validate all three expected managed-API variables.
  • No logic errors or boundary issues found.

Summary: The test fix is correct and the PR is sound end-to-end. No new concerns.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

@nohwnd

This comment has been minimized.

@nohwnd
Copy link
Copy Markdown
Member Author

nohwnd commented May 29, 2026

doubt that this is properly validated without acceptance test.

…rocessVsTestConsoleWrapper

Add InProcessWrapperConstructorWithRealEnvironmentHelper_OnWindows_IncludesNativeEqualsEnvironmentVariablesInProcessHelper
to verify that when InheritEnvironmentVariables = true on Windows, the P/Invoke path in
InProcessVsTestConsoleWrapper correctly supplements the managed env snapshot with the
'='-prefixed drive-relative current-directory entries from the native env block.

Unlike the existing unit tests which mock IEnvironmentVariableHelper, this test uses a real
implementation so the actual GetEnvironmentVariables() -> P/Invoke supplementation path is
exercised end-to-end.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nohwnd
Copy link
Copy Markdown
Member Author

nohwnd commented May 29, 2026

Commit pushed: 46c23e8

🔧 Iterated by PR Iteration Agent 🔧

@nohwnd

This comment has been minimized.

Copy link
Copy Markdown
Member Author

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

Reviewed the new commit (46c23e8) added since the last review pass (2026-05-29).

New commit: test: add Windows-specific test for native =-prefixed env vars

What was checked:

  • RealEnvironmentVariableHelper private sealed class — correctly delegates to Environment.GetEnvironmentVariables() / GetEnvironmentVariable(), exercising the actual managed API path
  • [TestCategory("Windows")] + [OSCondition(OperatingSystems.Windows)] — correct gating; test will only run on Windows
  • Assertion logic — Any(k => k.StartsWith("=", ...)) is a sound lower-bound check; Windows always maintains at least one =<Drive>:=<path> entry in the native environment block for any normally-started process
  • No new public API surface, no IPC changes, no binding redirect implications

No issues found. The test correctly exercises the end-to-end P/Invoke supplementation path introduced by the fix.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

@nohwnd
Copy link
Copy Markdown
Member Author

nohwnd commented May 29, 2026

The acceptance test infrastructure runs vstest.console as a separate process — the inProcess: true parameter in data sources means the testhost runs in-process within vstest.console, but it does not exercise InProcessVsTestConsoleWrapper. That class is a client-side wrapper used by Visual Studio / other in-proc callers, not by the acceptance test harness.

Because of this, writing an acceptance test that validates the =-prefixed env var forwarding would require a custom test fixture that checks its own environment from inside the testhost and reports back — which is disproportionately complex for this single Windows-specific P/Invoke edge case.

The unit test added in commit 46c23e8 uses a real IEnvironmentVariableHelper (no mocked GetEnvironmentVariables()) and verifies that after constructing InProcessVsTestConsoleWrapper with InheritEnvironmentVariables = true, ProcessHelper.ExternalEnvironmentVariables contains at least one =-prefixed key. This calls the actual GetEnvironmentStringsW P/Invoke — the same native Windows API that the fix uses — and validates the full code path end-to-end at the level where it can actually be observed.

If you feel an acceptance-level test is still needed, I am happy to add one — but I would need guidance on what testhost-observable behavior to assert, since ProcessHelper.ExternalEnvironmentVariables is not visible from outside the process boundary.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

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.

Wrong environment variables when vstest runs in-process on Windows with InheritEnvironmentVariables = true

2 participants