Honor inprocess transport in C# E2E harness and fix in-process auth#1920
Merged
Conversation
The E2E harness's CreateClient always assigned options.Connection = ForStdio for the default (no-Connection, no-useStdio) case, which made CopilotClient skip ResolveDefaultConnection — the only place COPILOT_SDK_DEFAULT_CONNECTION is read. As a result the "inprocess" test matrix cell silently ran the whole suite over stdio; only the one test that hard-codes ForInProcess exercised the in-process transport. Leave Connection null when useStdio is null so the default transport is resolved from COPILOT_SDK_DEFAULT_CONNECTION (stdio by default, or in-process). useStdio still pins stdio/tcp explicitly. The CLI path flows via options.Environment["COPILOT_CLI_PATH"] (setup-copilot sets it in CI), so no path needs to be injected on the connection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Host-side native code in the loaded cdylib (fetch_copilot_user, and the gh-CLI auth fallback) reads the ambient process environment rather than the environment passed to copilot_runtime_host_start, so the E2E harness's per-test redirects (COPILOT_DEBUG_GITHUB_API_URL) and cleared tokens were invisible in-process, causing 401 Bad credentials. Mirror only the auth-relevant vars (and only when the session actually uses the in-process transport) onto the real process environment, using libc setenv on Unix since .NET's managed setter does not reach getenv. Gating avoids polluting stdio/tcp tests; serial test execution makes it safe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The COPILOT_OTEL_* telemetry variables derived from CopilotClientOptions.Telemetry were only applied to the stdio/tcp child process environment, so in-process FFI sessions never enabled OTLP export. Factor the derivation into a shared helper and apply it when building the in-process host environment, giving telemetry parity across transports. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI sets COPILOT_HMAC_KEY as an ambient credential, but in-process auth resolution runs host-side in the test process and ranks HMAC above the GitHub token, so provider.getEndpoint and login tests wrongly resolved to HMAC. Clear both HMAC trigger vars in the default E2E environment and mirror them (empty) onto the host process for in-process runs. Also simplify the in-process env-mirror gate to the default-connection env var. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The in-process auth env mirror mutated shared process-env vars (HMAC keys, GitHub API URL redirect, tokens) without restoring them, so a later test in a different fixture inherited a cleared HMAC key and a stale/disposed replay-proxy URL. That broke ClientE2ETests' direct-construction stdio/tcp cases, which rely on the ambient CI HMAC credential (order-dependent, so only some OS cells went red). Back up each var's pre-test value on first mutation and restore (or unset) it in CleanupAfterTestAsync so mirroring cannot leak across tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The HostSideAuthEnvVars allowlist existed to keep the shared-process-env mutation minimal so stale values couldn't leak into later tests. Now that RestoreMirroredEnvironment reverts every mutation after each test, that rationale is obsolete, and a curated allowlist is a maintenance trap: it silently breaks in-process auth whenever the runtime starts reading a new var host-side (as happened with COPILOT_HMAC_KEY). Since in-process mode mutates the shared host env regardless, filtering to a subset buys nothing. Mirror the whole options.Environment so every host-side read (auth, HMAC, COPILOT_HOME/XDG, gh-CLI config) observes the intended test environment, and rely on restore for isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The per-test env restore could be defeated on CI: the transient-client force-stop loop only caught transient exceptions, so a non-transient failure skipped RestoreMirroredEnvironment, leaving the shared process env cleared (empty HMAC + redirected GitHub API URL). Worse, the next test then captured that polluted value as its own 'pristine' baseline, so every later restore preserved the pollution permanently -- which is why direct-construction ClientE2ETests (Force_Stop / Allow_CreateSession / Not_Throw_When_Disposing) failed on CI with a malformed-URL user fetch while passing locally (cleanup never throws locally). Record each mirrored var's pristine value once, process-wide, and never overwrite it, so restore always reverts to the true ambient value regardless of ordering or a missed restore. Run RestoreMirroredEnvironment in a finally and also at fixture teardown as a backstop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
… inprocess mode Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…cases in inprocess mode" This reverts commit fe98e57.
…dentials per-test Move all in-process process-env hackery into InProcessEnvIsolation.cs and add an assembly-level BeforeAfterTest attribute that neutralizes ambient CI credentials (HMAC keys) around every test in the in-process job, independent of how the CopilotClient is constructed. This stops directly-constructed clients (e.g. ClientE2ETests) from authenticating for real and reaching the live API in the in-process cell. The per-test proxy/home mirror stays in CreateClient but now routes through the same single, deletable helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Replace the manual copy loop with a LINQ projection and drop the dead null-filter (neither options.Environment nor ApplyTelemetryEnvironment ever produces null values). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
| // 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) |
|
|
||
| [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); |
| Environment.SetEnvironmentVariable(name, value); | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| _ = NativeSetEnv(name, value, 1); |
Comment on lines
+167
to
+174
| if (value is null) | ||
| { | ||
| _ = NativeUnsetEnv(name); | ||
| } | ||
| else | ||
| { | ||
| _ = NativeSetEnv(name, value, 1); | ||
| } |
| { | ||
| if (value is null) | ||
| { | ||
| _ = NativeUnsetEnv(name); |
| } | ||
| else | ||
| { | ||
| _ = NativeSetEnv(name, value, 1); |
In in-process (FFI) mode the runtime is hosted inside the test process and holds the session-store SQLite handle open. ForceStopAsync only fires the abrupt native host_shutdown, which does not release that handle, so the temp-dir delete fails on Windows. Use graceful StopAsync for inproc so the runtime shutdown RPC closes the DB before the directory is removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅All changes in this PR are confined to the .NET SDK ( Analysis summary:
The other SDKs (Node.js, Python, Go, Rust, Java) each have
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the .NET/C# SDK E2E harness so the test suite truly exercises the in-process FFI transport when COPILOT_SDK_DEFAULT_CONNECTION=inprocess, and adds harness-side environment isolation to prevent in-process host-side code paths from escaping the replay proxy and authenticating against live endpoints.
Changes:
- Adjusts
E2ETestContext.CreateClientso the default (unset) connection leavesoptions.Connectionnull, allowingCopilotClientto honorCOPILOT_SDK_DEFAULT_CONNECTION. - Introduces a temporary, centralized in-process environment mirroring/restoration mechanism and an assembly-level xUnit hook to neutralize ambient credentials during in-process runs.
- Unifies telemetry environment variable application across child-process and in-process transports, and updates CI to exclude Windows in-process until shutdown locking is resolved.
Show a summary per file
| File | Description |
|---|---|
| dotnet/test/Harness/InProcessEnvIsolation.cs | Adds temporary process-env mirroring/restoration and an assembly-level xUnit hook for in-process hermeticity. |
| dotnet/test/Harness/E2ETestContext.cs | Ensures default connection resolution honors COPILOT_SDK_DEFAULT_CONNECTION; mirrors env for in-process; improves cleanup behavior. |
| dotnet/test/AssemblyInfo.cs | Registers the new assembly-level InProcessEnvIsolation hook for tests. |
| dotnet/src/Types.cs | Marks in-process connection APIs as experimental. |
| dotnet/src/Client.cs | Applies telemetry-derived environment variables consistently for the in-process FFI host path and refactors child-process telemetry env setup. |
| .github/workflows/dotnet-sdk-tests.yml | Temporarily excludes Windows in-process transport from the test matrix. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+151
to
+169
| 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); | ||
| } | ||
| } |
Comment on lines
+292
to
+298
| if (InProcessEnvIsolation.IsActive(options.Environment)) | ||
| { | ||
| foreach (var (name, value) in options.Environment) | ||
| { | ||
| InProcessEnvIsolation.Mirror(name, value); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Makes the C# E2E harness genuinely honor
COPILOT_SDK_DEFAULT_CONNECTION=inprocess, and fixes the in-process auth failures that surfaced once it does.Two commits:
Honor
COPILOT_SDK_DEFAULT_CONNECTIONin the harness.E2ETestContext.CreateClientpreviously always assigned an explicitForStdioconnection for the default (useStdio == null) case, which madeoptions.Connectionnon-null and short-circuitedCopilotClient'sResolveDefaultConnection— so the env var was never read. As a result theinprocessCI cell secretly ran the whole suite over stdio; only one hard-codedForInProcesssmoke test actually exercised the FFI transport. Now the default case leavesConnectionnull so the env var is honored.Fix in-process per-session auth (401 Bad credentials). Some runtime code paths run host-side in the SDK's own process (the loaded cdylib) and read the ambient process environment rather than the environment passed to
copilot_runtime_host_start:fetch_copilot_userreadsCOPILOT_DEBUG_GITHUB_API_URLviastd::env::vargh auth token, which inheritsGH_TOKEN/GITHUB_TOKEN/GH_CONFIG_DIRSo the harness's per-test redirects and cleared tokens (which live only in
options.Environment) were invisible in-process and auth escaped the replay proxy → 401. The harness now mirrors just those auth-relevant vars onto the real process environment (via libcsetenvon Unix, since .NET's managedSetEnvironmentVariabledoesn't reachgetenv). It is gated to run only when the session actually uses the in-process transport, and limited to vars thatGetEnvironment()re-sets every call, so it can't pollute stdio/tcp tests or leak stale state across the serially-run suite.Why this is harness-only
For real in-process consumers the parent process already holds the real token and real GitHub URL, so the host-side
getenvreads resolve correctly. The gap only bites the E2E harness because it redirects those values to a replay proxy. The proper long-term fix belongs in the runtime: thread thehost_startenvironment into these host-side reads instead of consulting the global process env — noted in the code comment.Verification
Local (Linux), CLI pointed at a from-source
dist-cli:PerSessionAuth+ModeHandlers+ account login/quota: 8/8 pass in-process (were failing with 401 before).Known remaining in-process gap (not addressed here)
TelemetryExportE2ETests.Should_Export_File_Telemetry_For_Sdk_Interactionsfails in-process (telemetry.jsonlnot written) — a separate transport gap in the OTLP file exporter, unrelated to auth. Left for CI to surface / a follow-up.