diff --git a/.github/workflows/dotnet-sdk-tests.yml b/.github/workflows/dotnet-sdk-tests.yml index 909742cdf..dcf559228 100644 --- a/.github/workflows/dotnet-sdk-tests.yml +++ b/.github/workflows/dotnet-sdk-tests.yml @@ -37,6 +37,10 @@ jobs: matrix: os: [ubuntu-latest, macos-latest, windows-latest] transport: ["default", "inprocess"] + # TODO: Re-enable after fixing in-process sqlite file locking on shutdown on Windows + exclude: + - os: windows-latest + transport: "inprocess" runs-on: ${{ matrix.os }} defaults: run: diff --git a/dotnet/src/Client.cs b/dotnet/src/Client.cs index 18ed2eeed..0e1ec145e 100644 --- a/dotnet/src/Client.cs +++ b/dotnet/src/Client.cs @@ -291,7 +291,14 @@ async Task StartCoreAsync(CancellationToken ct) { // In-process FFI hosting: load the Rust cdylib and let it spawn // the CLI worker, instead of the SDK launching a CLI child process. - var ffiHost = FfiRuntimeHost.Create(ResolveCliPathForFfi(), GetNapiPrebuildsFolderOrThrow(), _options.Environment, _logger); + // The worker reads its configuration (telemetry export, etc.) from + // the environment passed here, so apply the same telemetry-derived + // vars the child-process path sets on its startInfo.Environment. + var ffiEnvironment = _options.Environment?.ToDictionary(kvp => kvp.Key, kvp => (string?)kvp.Value) + ?? new Dictionary(); + ApplyTelemetryEnvironment(ffiEnvironment, _options.Telemetry); + var resolvedFfiEnvironment = ffiEnvironment.ToDictionary(kvp => kvp.Key, kvp => kvp.Value!); + var ffiHost = FfiRuntimeHost.Create(ResolveCliPathForFfi(), GetNapiPrebuildsFolderOrThrow(), resolvedFfiEnvironment, _logger); _ffiHost = ffiHost; await ffiHost.StartAsync(ct); connection = await ConnectToServerAsync(null, null, null, null, ct, ffiHost); @@ -1909,6 +1916,25 @@ private static bool IsUnsupportedConnectMethod(RemoteRpcException ex) || string.Equals(ex.Message, "Unhandled method connect", StringComparison.Ordinal); } + // Applies the telemetry-derived environment variables the runtime reads to + // enable OTLP export. Shared by the stdio/tcp child-process path and the + // in-process FFI path so telemetry behaves identically across transports. + private static void ApplyTelemetryEnvironment(IDictionary environment, TelemetryConfig? telemetry) + { + if (telemetry is null) + { + return; + } + + environment["COPILOT_OTEL_ENABLED"] = "true"; + if (telemetry.OtlpEndpoint is not null) environment["OTEL_EXPORTER_OTLP_ENDPOINT"] = telemetry.OtlpEndpoint; + if (telemetry.OtlpProtocol is not null) environment["OTEL_EXPORTER_OTLP_PROTOCOL"] = telemetry.OtlpProtocol; + if (telemetry.FilePath is not null) environment["COPILOT_OTEL_FILE_EXPORTER_PATH"] = telemetry.FilePath; + if (telemetry.ExporterType is not null) environment["COPILOT_OTEL_EXPORTER_TYPE"] = telemetry.ExporterType; + if (telemetry.SourceName is not null) environment["COPILOT_OTEL_SOURCE_NAME"] = telemetry.SourceName; + if (telemetry.CaptureContent is { } capture) environment["OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT"] = capture ? "true" : "false"; + } + private async Task<(Process Process, int? DetectedLocalhostTcpPort, ProcessStderrPump StderrPump)> StartCliServerAsync(CancellationToken cancellationToken) { var options = _options; @@ -2023,16 +2049,7 @@ private static bool IsUnsupportedConnectMethod(RemoteRpcException ex) } // Set telemetry environment variables if configured - if (options.Telemetry is { } telemetry) - { - startInfo.Environment["COPILOT_OTEL_ENABLED"] = "true"; - if (telemetry.OtlpEndpoint is not null) startInfo.Environment["OTEL_EXPORTER_OTLP_ENDPOINT"] = telemetry.OtlpEndpoint; - if (telemetry.OtlpProtocol is not null) startInfo.Environment["OTEL_EXPORTER_OTLP_PROTOCOL"] = telemetry.OtlpProtocol; - if (telemetry.FilePath is not null) startInfo.Environment["COPILOT_OTEL_FILE_EXPORTER_PATH"] = telemetry.FilePath; - if (telemetry.ExporterType is not null) startInfo.Environment["COPILOT_OTEL_EXPORTER_TYPE"] = telemetry.ExporterType; - if (telemetry.SourceName is not null) startInfo.Environment["COPILOT_OTEL_SOURCE_NAME"] = telemetry.SourceName; - if (telemetry.CaptureContent is { } capture) startInfo.Environment["OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT"] = capture ? "true" : "false"; - } + ApplyTelemetryEnvironment(startInfo.Environment, options.Telemetry); var cliProcess = new Process { StartInfo = startInfo }; try diff --git a/dotnet/src/Types.cs b/dotnet/src/Types.cs index c49650ca6..bcd8903c8 100644 --- a/dotnet/src/Types.cs +++ b/dotnet/src/Types.cs @@ -156,6 +156,7 @@ public static UriRuntimeConnection ForUri(string url, string? connectionToken = /// Works across the SDK's target frameworks: modern .NET uses NativeLibrary, /// while netstandard2.0 consumers use a built-in fallback native loader. /// + [Experimental(Diagnostics.Experimental)] public static InProcessRuntimeConnection ForInProcess() => new(); } @@ -190,6 +191,7 @@ internal StdioRuntimeConnection() { } /// To point at a non-default runtime entrypoint, set the COPILOT_CLI_PATH /// environment variable. /// +[Experimental(Diagnostics.Experimental)] public sealed class InProcessRuntimeConnection : RuntimeConnection { internal InProcessRuntimeConnection() { } diff --git a/dotnet/test/AssemblyInfo.cs b/dotnet/test/AssemblyInfo.cs index e34f0e255..df814ff89 100644 --- a/dotnet/test/AssemblyInfo.cs +++ b/dotnet/test/AssemblyInfo.cs @@ -3,6 +3,7 @@ *--------------------------------------------------------------------------------------------*/ using Xunit; +using GitHub.Copilot.Test.Harness; // Each E2E test class fixture spins up its own Copilot CLI subprocess plus a CapiProxy // (replaying HTTP proxy) Node.js subprocess. With ~25 test classes, running them in parallel @@ -13,3 +14,10 @@ // (a) sharing a single CLI subprocess across classes, or (b) gating concurrency with a // 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 index 6e26299a4..1f09988a7 100644 --- a/dotnet/test/Harness/E2ETestContext.cs +++ b/dotnet/test/Harness/E2ETestContext.cs @@ -216,6 +216,16 @@ public Dictionary GetEnvironment() env["GITHUB_TOKEN"] = env["GH_TOKEN"] = DefaultGitHubToken; + // Disable HMAC auth for E2E runs. CI sets COPILOT_HMAC_KEY at the job + // level as an ambient credential, but the replay snapshots are captured + // against Bearer/OAuth (SDK-token) requests. In stdio the SDK token + // outranks HMAC so this is a no-op, but in-process auth resolution runs + // host-side in this process and would otherwise pick HMAC (which ranks + // above the GitHub token) and fail provider.getEndpoint. An empty value + // disables the method (runtime filters out empty HMAC keys). + env["COPILOT_HMAC_KEY"] = ""; + env["CAPI_HMAC_KEY"] = ""; + return env!; } @@ -238,9 +248,10 @@ public CopilotClient CreateClient( options.Environment ??= GetEnvironment(); options.Logger ??= Logger; - // Build the connection. If the caller supplied one, just ensure the runtime path is set; - // otherwise default to Stdio with the bundled runtime (matches CopilotClient's own default). - // useStdio is a convenience shortcut for the no-Connection case; passing both is ambiguous. + // Build the connection. If the caller supplied one, just ensure the runtime path is set. + // When neither a Connection nor useStdio is specified, leave Connection null so + // CopilotClient honors COPILOT_SDK_DEFAULT_CONNECTION (defaulting to stdio); useStdio + // is a convenience shortcut to pin stdio/tcp. Passing both a Connection and useStdio is ambiguous. if (useStdio is not null && options.Connection is not null) { throw new ArgumentException( @@ -252,16 +263,40 @@ public CopilotClient CreateClient( var cliPath = GetCliPath(_repoRoot); switch (options.Connection) { + case null when useStdio == true: + options.Connection = RuntimeConnection.ForStdio(path: cliPath); + break; + case null when useStdio == false: + options.Connection = RuntimeConnection.ForTcp(path: cliPath); + break; case null: - options.Connection = useStdio == false - ? RuntimeConnection.ForTcp(path: cliPath) - : RuntimeConnection.ForStdio(path: cliPath); + // useStdio is null: leave Connection unset so CopilotClient's + // ResolveDefaultConnection honors COPILOT_SDK_DEFAULT_CONNECTION + // (stdio by default, or in-process). The CLI path flows through + // options.Environment["COPILOT_CLI_PATH"] (GetEnvironment copies + // the process env, where CI's setup-copilot sets it). break; case ChildProcessRuntimeConnection child when child.Path is null: child.Path = cliPath; break; } + // In-process hosting workaround: 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, 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)) + { + foreach (var (name, value) in options.Environment) + { + InProcessEnvIsolation.Mirror(name, value); + } + } + // Auto-inject auth token unless connecting to an existing runtime via URI. var isExistingRuntime = options.Connection is UriRuntimeConnection; if (autoInjectGitHubToken @@ -308,17 +343,28 @@ public async Task CleanupAfterTestAsync() _transientClients.Clear(); } - foreach (var client in transientClients) + try { - try + foreach (var client in transientClients) { - await client.ForceStopAsync(); - } - catch (Exception ex) when (IsTransientCleanupException(ex)) - { - errors.Add(ex); + try + { + 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) { @@ -346,7 +392,7 @@ public async ValueTask DisposeAsync() { try { - await client.ForceStopAsync(); + await StopClientForCleanupAsync(client); } catch (Exception ex) when (IsTransientCleanupException(ex)) { @@ -354,6 +400,11 @@ 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); } @@ -408,6 +459,19 @@ 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()) + { + await client.StopAsync(); + } + else + { + await client.ForceStopAsync(); + } + } + private static bool IsTransientCleanupException(Exception exception) => exception is IOException or UnauthorizedAccessException; } diff --git a/dotnet/test/Harness/InProcessEnvIsolation.cs b/dotnet/test/Harness/InProcessEnvIsolation.cs new file mode 100644 index 000000000..c3e43af4b --- /dev/null +++ b/dotnet/test/Harness/InProcessEnvIsolation.cs @@ -0,0 +1,196 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +using System.Reflection; +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. +// ============================================================================= + +/// +/// 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. +/// +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"]; + + private static readonly object s_lock = new(); + + // 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 s_pristineByName = new(); + + [DllImport("libc", EntryPoint = "setenv", CharSet = CharSet.Ansi, + BestFitMapping = false, ThrowOnUnmappableChar = true)] + private static extern int NativeSetEnv(string name, string value, int overwrite); + + [DllImport("libc", EntryPoint = "unsetenv", CharSet = CharSet.Ansi, + BestFitMapping = false, ThrowOnUnmappableChar = true)] + private static extern int NativeUnsetEnv(string name); + + /// + /// 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. + /// + public static bool IsActive(IReadOnlyDictionary? 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); + } + + /// + /// Mirrors a variable onto the shared process environment for the in-process + /// host-side runtime to observe, recording the pristine value once so + /// can always revert to the true original. + /// + public static void Mirror(string name, string value) + { + lock (s_lock) + { + if (!s_pristineByName.ContainsKey(name)) + { + s_pristineByName[name] = Environment.GetEnvironmentVariable(name); + } + } + + SetProcessEnvironmentVariable(name, value); + } + + /// + /// Reverts every variable ever touched by 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. + /// + public static void Restore() + { + KeyValuePair[] pristine; + lock (s_lock) + { + if (s_pristineByName.Count == 0) + { + return; + } + + pristine = [.. s_pristineByName]; + } + + foreach (var (name, value) in pristine) + { + RestoreProcessEnvironmentVariable(name, value); + } + } + + /// + /// Neutralizes ambient credentials that would otherwise let a directly + /// constructed client authenticate for real in the in-process job. Recorded + /// via so reverts them. + /// + public static void NeutralizeAmbientCredentials() + { + foreach (var name in LeakyCredentialVars) + { + Mirror(name, ""); + } + } + + // 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) + { + Environment.SetEnvironmentVariable(name, value); + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + _ = NativeSetEnv(name, value, 1); + } + } + + // 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)) + { + _ = value is null ? NativeUnsetEnv(name) : NativeSetEnv(name, value, 1); + } + } +} + +/// +/// 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 +/// . +/// +[AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false)] +public sealed class InProcessEnvIsolationAttribute : BeforeAfterTestAttribute +{ + public override void Before(MethodInfo methodUnderTest) + { + if (InProcessEnvIsolation.IsActive()) + { + InProcessEnvIsolation.NeutralizeAmbientCredentials(); + } + } + + public override void After(MethodInfo methodUnderTest) + { + if (InProcessEnvIsolation.IsActive()) + { + InProcessEnvIsolation.Restore(); + } + } +}