Simplify in-process env isolation to snapshot/restore

SteveSandersonMS · Copilot · SteveSandersonMS · commit 8d191fdd5b3d · 2026-07-07T09:27:06.000Z
Replace the targeted per-name pristine tracking (s_pristineByName,
Mirror/Restore) with a blanket load-time environment snapshot restored
after every test. The attribute now blanks leaky CI credentials before
each test and reverts the whole process environment afterwards,
regardless of transport, so per-test mirrors and fixture-teardown
backstops in E2ETestContext are no longer needed.

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/dotnet/test/AssemblyInfo.cs b/dotnet/test/AssemblyInfo.cs
@@ -15,9 +15,4 @@
 // semaphore that limits concurrent fixtures to a small number (e.g. 2-3).
 [assembly: CollectionBehavior(DisableTestParallelization = true)]
 
-// TEMPORARY: isolate the ambient process environment around every test in the
-// in-process job so directly-constructed clients cannot pick up ambient CI
-// credentials and reach the live API. No-op outside the in-process job. Delete
-// together with InProcessEnvIsolation.cs once the runtime stops reading the
-// ambient process environment host-side.
 [assembly: InProcessEnvIsolation]
diff --git a/dotnet/test/Harness/E2ETestContext.cs b/dotnet/test/Harness/E2ETestContext.cs
@@ -287,14 +287,11 @@ public CopilotClient CreateClient(
         // copilot_runtime_host_start, so our per-test redirects, cleared tokens,
         // cleared HMAC keys, and isolated home in options.Environment are
         // invisible to them unless mirrored onto this process's real environment.
-        // All of this hackery lives in InProcessEnvIsolation so it can be deleted
-        // in one place once the runtime stops reading the ambient process env.
-        if (InProcessEnvIsolation.IsActive(options.Environment))
+        // Restored after each test by InProcessEnvIsolationAttribute. Harmless for
+        // child-process transports, which configure their child's environment.
+        foreach (var (name, value) in options.Environment)
         {
-            foreach (var (name, value) in options.Environment)
-            {
-                InProcessEnvIsolation.Mirror(name, value);
-            }
+            InProcessEnvIsolation.Apply(name, value);
         }
 
         // Auto-inject auth token unless connecting to an existing runtime via URI.
@@ -343,27 +340,16 @@ public async Task CleanupAfterTestAsync()
             _transientClients.Clear();
         }
 
-        try
+        foreach (var client in transientClients)
         {
-            foreach (var client in transientClients)
+            try
             {
-                try
-                {
-                    await StopClientForCleanupAsync(client);
-                }
-                catch (Exception ex) when (IsTransientCleanupException(ex))
-                {
-                    errors.Add(ex);
-                }
+                await StopClientForCleanupAsync(client);
+            }
+            catch (Exception ex) when (IsTransientCleanupException(ex))
+            {
+                errors.Add(ex);
             }
-        }
-        finally
-        {
-            // Undo any in-process env mirroring so it cannot leak into the next
-            // test. In a finally so a non-transient force-stop failure above can
-            // never skip it (a skipped restore would otherwise strand the shared
-            // process env in its cleared/redirected state until the next mirror).
-            InProcessEnvIsolation.Restore();
         }
 
         if (errors.Count == 1)
@@ -400,11 +386,6 @@ public async ValueTask DisposeAsync()
             }
         }
 
-        // Backstop: revert any in-process env mirroring at fixture teardown too,
-        // so a class's mutations cannot survive into the next class even if a
-        // per-test cleanup was bypassed.
-        InProcessEnvIsolation.Restore();
-
         // Skip writing snapshots in CI to avoid corrupting them on test failures
         var isCI = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("GITHUB_ACTIONS"));
         try { await _proxy.StopAsync(skipWritingCache: isCI); } catch (Exception ex) when (IsTransientCleanupException(ex)) { errors.Add(ex); }
@@ -462,7 +443,11 @@ private static async Task DeleteDirectoryAsync(string path)
     // Inproc holds the session-store SQLite handle in-process; graceful StopAsync releases it so the temp-dir delete succeeds on Windows.
     private static async Task StopClientForCleanupAsync(CopilotClient client)
     {
-        if (InProcessEnvIsolation.IsActive())
+        var isInProcess = string.Equals(
+            Environment.GetEnvironmentVariable("COPILOT_SDK_DEFAULT_CONNECTION"),
+            "inprocess",
+            StringComparison.OrdinalIgnoreCase);
+        if (isInProcess)
         {
             await client.StopAsync();
         }
diff --git a/dotnet/test/Harness/InProcessEnvIsolation.cs b/dotnet/test/Harness/InProcessEnvIsolation.cs
@@ -2,68 +2,32 @@
  *  Copyright (c) Microsoft Corporation. All rights reserved.
  *--------------------------------------------------------------------------------------------*/
 
+using System.Collections;
 using System.Reflection;
+using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using Xunit.Sdk;
 
 namespace GitHub.Copilot.Test.Harness;
 
-// =============================================================================
-// TEMPORARY in-process env-var isolation. DELETE THIS ENTIRE FILE (and its two
-// references in E2ETestContext plus the [assembly: InProcessEnvIsolation] in
-// AssemblyInfo.cs) once the runtime stops reading the ambient process
-// environment host-side.
-//
-// Why this exists
-// ---------------
-// Over the in-process FFI transport several runtime code paths run host-side in
-// THIS process (the loaded cdylib) and read the ambient process environment
-// rather than the environment passed to copilot_runtime_host_start — e.g.
-// native fetch_copilot_user reads COPILOT_DEBUG_GITHUB_API_URL via
-// std::env::var, the gh-CLI fallback spawns `gh auth token` (inheriting this
-// process's GH_TOKEN / GITHUB_TOKEN / GH_CONFIG_DIR), auth-method selection
-// reads the HMAC keys, and session state/config reads COPILOT_HOME / XDG_*.
-// So the per-test environment we hand to CopilotClient is invisible to them.
-//
-// Two problems follow, both handled here:
-//  1. CreateClient-routed tests must mirror their per-test environment onto this
-//     process so host-side reads observe the replay proxy, isolated home, and
-//     cleared credentials. See Mirror/Restore below.
-//  2. Tests that construct CopilotClient directly (e.g. ClientE2ETests) never go
-//     through CreateClient, so nothing clears the ambient CI HMAC credential;
-//     in the in-process job they would authenticate for real and hit the live
-//     api.githubcopilot.com. The assembly-level BeforeAfterTest attribute below
-//     neutralizes those ambient credentials around EVERY test, independent of
-//     how the client is constructed.
-//
-// Everything here is gated to the in-process job (COPILOT_SDK_DEFAULT_CONNECTION
-// == "inprocess") and is a no-op otherwise.
-// =============================================================================
-
-/// <summary>
-/// Owns all process-wide environment mutation used to make the in-process FFI
-/// transport hermetic in tests. Consolidated in one file so it can be deleted
-/// wholesale once the runtime no longer reads the ambient process environment.
-/// </summary>
+// Because many of the tests mutate global environment variables, we have to snapshot the original
+// state and restore it after each test. Otherwise tests influence each other depending on run order.
+// This is especially important for the in-process transport because the runtime is inside the test
+// host process and will be reading/writing its environment variables directly.
 internal static class InProcessEnvIsolation
 {
-    // Ambient credentials that would otherwise let a directly-constructed client
-    // authenticate for real (and reach the live API) in the in-process job. CI
-    // sets COPILOT_HMAC_KEY at the job level; the replay snapshots are captured
-    // against Bearer/OAuth requests, so real HMAC auth must be disabled. An empty
-    // value disables the method (the runtime filters out empty HMAC keys).
-    private static readonly string[] LeakyCredentialVars = ["COPILOT_HMAC_KEY", "CAPI_HMAC_KEY"];
+    // Unset because CI sets them but the replay snapshots expect Bearer/OAuth.
+    private static readonly string[] SuppressEnvVars = ["COPILOT_HMAC_KEY", "CAPI_HMAC_KEY"];
 
-    private static readonly object s_lock = new();
+    // Captured at load, before any fixture/test mutates env.
+    private static readonly Dictionary<string, string?> s_ambient = CaptureEnvironment();
 
-    // Process-wide, permanent record of the PRISTINE (pre-any-mirror) value of
-    // every variable we have ever overwritten. A null entry means the variable
-    // was originally unset. Captured once per name and never overwritten, so it
-    // is immune to a cascade in which a skipped restore would otherwise let a
-    // later test back up an already-polluted value as its baseline. Static
-    // because the shared process env is itself process-global and E2E tests run
-    // serially (DisableTestParallelization).
-    private static readonly Dictionary<string, string?> s_pristineByName = new();
+    // Runs at assembly load so the ambient env is snapshotted before the shared
+    // fixture mirrors per-test env onto the process. Justifies suppressing CA2255.
+#pragma warning disable CA2255 // ModuleInitializer discouraged in libraries; intentional in this test harness.
+    [ModuleInitializer]
+    internal static void CaptureAtLoad() => _ = s_ambient;
+#pragma warning restore CA2255
 
     [DllImport("libc", EntryPoint = "setenv", CharSet = CharSet.Ansi,
         BestFitMapping = false, ThrowOnUnmappableChar = true)]
@@ -73,124 +37,57 @@ internal static class InProcessEnvIsolation
         BestFitMapping = false, ThrowOnUnmappableChar = true)]
     private static extern int NativeUnsetEnv(string name);
 
-    /// <summary>
-    /// Whether the in-process FFI transport is the default for this run, honoring
-    /// COPILOT_SDK_DEFAULT_CONNECTION from the supplied per-test environment (if
-    /// any) else the process environment. Mirrors CopilotClient's own resolution.
-    /// </summary>
-    public static bool IsActive(IReadOnlyDictionary<string, string>? environment = null)
-    {
-        var value = environment is not null
-            && environment.TryGetValue("COPILOT_SDK_DEFAULT_CONNECTION", out var fromOptions)
-                ? fromOptions
-                : Environment.GetEnvironmentVariable("COPILOT_SDK_DEFAULT_CONNECTION");
-        return string.Equals(value, "inprocess", StringComparison.OrdinalIgnoreCase);
-    }
-
-    /// <summary>
-    /// Mirrors a variable onto the shared process environment for the in-process
-    /// host-side runtime to observe, recording the pristine value once so
-    /// <see cref="Restore"/> can always revert to the true original.
-    /// </summary>
-    public static void Mirror(string name, string value)
+    // Sets/unsets on the managed cache and, on Unix, the libc block so native
+    // readers in the loaded cdylib observe it.
+    public static void Apply(string name, string? value)
     {
-        lock (s_lock)
-        {
-            if (!s_pristineByName.ContainsKey(name))
-            {
-                s_pristineByName[name] = Environment.GetEnvironmentVariable(name);
-            }
-        }
-
-        SetProcessEnvironmentVariable(name, value);
-    }
-
-    /// <summary>
-    /// Reverts every variable ever touched by <see cref="Mirror"/> back to its
-    /// permanently-recorded pristine value (or unsets it). Idempotent and
-    /// cascade-proof: because pristine values are never overwritten, calling this
-    /// always restores the true ambient environment even if a previous restore
-    /// was skipped.
-    /// </summary>
-    public static void Restore()
-    {
-        KeyValuePair<string, string?>[] pristine;
-        lock (s_lock)
-        {
-            if (s_pristineByName.Count == 0)
-            {
-                return;
-            }
-
-            pristine = [.. s_pristineByName];
-        }
-
-        foreach (var (name, value) in pristine)
+        Environment.SetEnvironmentVariable(name, value);
+        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
         {
-            RestoreProcessEnvironmentVariable(name, value);
+            _ = value is null ? NativeUnsetEnv(name) : NativeSetEnv(name, value, 1);
         }
     }
 
-    /// <summary>
-    /// Neutralizes ambient credentials that would otherwise let a directly
-    /// constructed client authenticate for real in the in-process job. Recorded
-    /// via <see cref="Mirror"/> so <see cref="Restore"/> reverts them.
-    /// </summary>
     public static void NeutralizeAmbientCredentials()
     {
-        foreach (var name in LeakyCredentialVars)
+        foreach (var name in SuppressEnvVars)
         {
-            Mirror(name, "");
+            Apply(name, null);
         }
     }
 
-    // Sets an environment variable on both the managed cache and (on Unix) the
-    // libc environment block, so native getenv/std::env::var readers in the
-    // loaded cdylib observe it. On Windows the managed setter already reaches
-    // native GetEnvironmentVariableW, so setenv is not needed.
-    private static void SetProcessEnvironmentVariable(string name, string value)
+    public static void RestoreAmbient()
     {
-        Environment.SetEnvironmentVariable(name, value);
-        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        foreach (DictionaryEntry entry in Environment.GetEnvironmentVariables())
         {
-            _ = NativeSetEnv(name, value, 1);
+            var name = (string)entry.Key;
+            if (!s_ambient.ContainsKey(name))
+            {
+                Apply(name, null);
+            }
         }
-    }
 
-    // Restores (or unsets) an environment variable on both the managed cache and
-    // (on Unix) the libc environment block.
-    private static void RestoreProcessEnvironmentVariable(string name, string? value)
-    {
-        Environment.SetEnvironmentVariable(name, value);
-        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        foreach (var (name, value) in s_ambient)
         {
-            _ = value is null ? NativeUnsetEnv(name) : NativeSetEnv(name, value, 1);
+            if (!string.Equals(Environment.GetEnvironmentVariable(name), value, StringComparison.Ordinal))
+            {
+                Apply(name, value);
+            }
         }
     }
+
+    private static Dictionary<string, string?> CaptureEnvironment() =>
+        Environment.GetEnvironmentVariables()
+            .Cast<DictionaryEntry>()
+            .ToDictionary(e => (string)e.Key, e => e.Value?.ToString(), StringComparer.Ordinal);
 }
 
-/// <summary>
-/// Assembly-level xUnit hook that isolates the ambient process environment around
-/// every test in the in-process job, independent of how the CopilotClient is
-/// constructed. No-op outside the in-process job. TEMPORARY — see
-/// <see cref="InProcessEnvIsolation"/>.
-/// </summary>
 [AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false)]
 public sealed class InProcessEnvIsolationAttribute : BeforeAfterTestAttribute
 {
-    public override void Before(MethodInfo methodUnderTest)
-    {
-        if (InProcessEnvIsolation.IsActive())
-        {
-            InProcessEnvIsolation.NeutralizeAmbientCredentials();
-        }
-    }
+    public override void Before(MethodInfo methodUnderTest) =>
+        InProcessEnvIsolation.NeutralizeAmbientCredentials();
 
-    public override void After(MethodInfo methodUnderTest)
-    {
-        if (InProcessEnvIsolation.IsActive())
-        {
-            InProcessEnvIsolation.Restore();
-        }
-    }
+    public override void After(MethodInfo methodUnderTest) =>
+        InProcessEnvIsolation.RestoreAmbient();
 }
diff --git a/dotnet/test/Harness/ModuleInitializerAttribute.cs b/dotnet/test/Harness/ModuleInitializerAttribute.cs
@@ -0,0 +1,13 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *--------------------------------------------------------------------------------------------*/
+
+#if !NET5_0_OR_GREATER
+namespace System.Runtime.CompilerServices;
+
+// Polyfill so [ModuleInitializer] compiles on net472; recognized by the compiler.
+[AttributeUsage(AttributeTargets.Method, Inherited = false)]
+internal sealed class ModuleInitializerAttribute : Attribute
+{
+}
+#endif
