From eac35254546164c7c8bd9a63fb52414423adc029 Mon Sep 17 00:00:00 2001 From: Shane Date: Mon, 30 Mar 2026 17:26:36 -0500 Subject: [PATCH 1/4] fix(dotnet): merge env vars instead of full replacement in CLI process The Environment property in CopilotClientOptions was calling startInfo.Environment.Clear() before populating user-provided vars, wiping ALL inherited environment variables (PATH, SystemRoot, TEMP, COMSPEC, etc.) when even a single override was specified. This caused the Node.js-based CLI subprocess to crash on Windows because it requires system env vars that are only available in the inherited environment. Fix: remove the Clear() call so that user-provided entries are merged into the inherited environment (keys are overridden, all others remain). Also update documentation in Types.cs and README.md to clearly describe the merge-override semantics (consistent with how Go and Python document their Env / env options). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/README.md | 2 +- dotnet/src/Client.cs | 1 - dotnet/src/Types.cs | 5 ++++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/dotnet/README.md b/dotnet/README.md index 0f67fb11a..6ecda6fea 100644 --- a/dotnet/README.md +++ b/dotnet/README.md @@ -75,7 +75,7 @@ new CopilotClient(CopilotClientOptions? options = null) - `LogLevel` - Log level (default: "info") - `AutoStart` - Auto-start server (default: true) - `Cwd` - Working directory for the CLI process -- `Environment` - Environment variables to pass to the CLI process +- `Environment` - Environment variables to set for the CLI process. Specified keys override their inherited values; all other variables are inherited from the current process. When null, the CLI process inherits the current environment unchanged. - `Logger` - `ILogger` instance for SDK logging - `GitHubToken` - GitHub token for authentication. When provided, takes priority over other auth methods. - `UseLoggedInUser` - Whether to use logged-in user for authentication (default: true, but false when `GitHubToken` is provided). Cannot be used with `CliUrl`. diff --git a/dotnet/src/Client.cs b/dotnet/src/Client.cs index d1cea218e..77a413839 100644 --- a/dotnet/src/Client.cs +++ b/dotnet/src/Client.cs @@ -1148,7 +1148,6 @@ private async Task VerifyProtocolVersionAsync(Connection connection, Cancellatio if (options.Environment != null) { - startInfo.Environment.Clear(); foreach (var (key, value) in options.Environment) { startInfo.Environment[key] = value; diff --git a/dotnet/src/Types.cs b/dotnet/src/Types.cs index d6530f9c7..8f7b972bc 100644 --- a/dotnet/src/Types.cs +++ b/dotnet/src/Types.cs @@ -107,7 +107,10 @@ protected CopilotClientOptions(CopilotClientOptions? other) [Obsolete("AutoRestart has no effect and will be removed in a future release.")] public bool AutoRestart { get; set; } /// - /// Environment variables to pass to the CLI process. + /// Environment variables to set for the CLI process. + /// When provided, these keys override (or add to) the inherited environment variables; + /// the CLI process still inherits all other variables from the current process. + /// When null, the CLI process inherits the current process environment unchanged. /// public IReadOnlyDictionary? Environment { get; set; } /// From 85b31ddc79a5899b31f19e4a4a8e992c7581f4fe Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 31 Mar 2026 13:48:55 -0500 Subject: [PATCH 2/4] test(dotnet,nodejs): prove env merge behavior and contrast with Node.js full-replacement Add regression tests for Issue #441 (Environment.Clear() bug in .NET SDK). ## dotnet/test/EnvironmentTests.cs (8 new tests) Proves the merge-vs-replace fix works through public APIs only: 1. Environment_DefaultsToNull - documents the API contract 2. Should_Start_When_Environment_Is_Null - baseline: null env works 3. Should_Start_When_Environment_Is_An_Empty_Dictionary - empty dict must NOT wipe inherited vars (before fix: Clear() was still called on empty dict) 4. Should_Start_When_Environment_Has_One_Custom_Key - CANONICAL regression: before the fix this test threw IOException because PATH/SystemRoot were wiped; after the fix the CLI starts with all inherited vars intact 5. Should_Start_When_Environment_Has_Multiple_Custom_Keys - N keys, all merged 6. Should_Start_When_Environment_Overrides_An_Inherited_Key - override PATH 7. TestHarness_GetEnvironment_Pattern_Works_After_Fix - documents why E2E tests never caught the bug (harness always passed full env) 8. Should_Strip_NODE_DEBUG_Even_When_Environment_Is_Null - NODE_DEBUG removal ## nodejs/test/client.test.ts (6 new tests in 'env option' describe block) Documents Node.js SDK's intentionally different semantics: - Node.js uses full-replacement (env: dict replaces process.env entirely) - .NET uses merge-override (Environment: dict merges into inherited env) - Both are correct for their respective runtimes - Node.js never had the bug because spawn env: is always full-replacement Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/test/EnvironmentTests.cs | 288 ++++++++++++++++++++++++++++++++ nodejs/test/client.test.ts | 128 ++++++++++++++ 2 files changed, 416 insertions(+) create mode 100644 dotnet/test/EnvironmentTests.cs diff --git a/dotnet/test/EnvironmentTests.cs b/dotnet/test/EnvironmentTests.cs new file mode 100644 index 000000000..37adb7913 --- /dev/null +++ b/dotnet/test/EnvironmentTests.cs @@ -0,0 +1,288 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +using Xunit; + +namespace GitHub.Copilot.SDK.Test; + +/// +/// Regression tests for the Environment merge-vs-replace bug (Issue #441). +/// +/// Background: +/// Before the fix, was handled with: +/// +/// startInfo.Environment.Clear(); // ← BUG: wiped PATH, SystemRoot, COMSPEC, TEMP, etc. +/// foreach (var (key, value) in options.Environment) +/// startInfo.Environment[key] = value; +/// +/// ProcessStartInfo.Environment is pre-populated with the current process's inherited +/// environment. The Clear() call threw it all away, so supplying even ONE custom key +/// caused the Node.js-based CLI subprocess to crash on Windows because essential system +/// variables (PATH, SystemRoot, COMSPEC) were gone. +/// +/// After the fix, user-supplied keys are merged (override or add) into the inherited +/// environment -- the CLI subprocess receives all inherited vars plus any overrides. +/// +/// How the tests prove the fix: +/// Every test below that provides a non-null Environment dict would have thrown an +/// IOException ("CLI process exited unexpectedly") BEFORE the fix. After the fix they +/// all pass because PATH/SystemRoot/COMSPEC remain available to the subprocess. +/// +public class EnvironmentTests +{ + // ── Null / empty cases ──────────────────────────────────────────────────── + + [Fact] + public void Environment_DefaultsToNull() + { + // Verify the documented default: null means "fully inherit from parent process". + var options = new CopilotClientOptions(); + Assert.Null(options.Environment); + } + + [Fact] + public async Task Should_Start_When_Environment_Is_Null() + { + // Baseline: null Environment → all inherited vars are present → CLI starts. + using var client = new CopilotClient(new CopilotClientOptions + { + UseStdio = true, + Environment = null, + }); + + try + { + await client.StartAsync(); + Assert.Equal(ConnectionState.Connected, client.State); + + var pong = await client.PingAsync("null-env"); + Assert.Equal("pong: null-env", pong.Message); + + await client.StopAsync(); + } + finally + { + await client.ForceStopAsync(); + } + } + + [Fact] + public async Task Should_Start_When_Environment_Is_An_Empty_Dictionary() + { + // An empty dictionary supplies no keys, so the loop in Client.cs runs zero + // iterations -- the inherited environment is completely unchanged. + // Before the fix: Clear() was still called → crash. + // After the fix: no Clear(); inherited env untouched → CLI starts normally. + using var client = new CopilotClient(new CopilotClientOptions + { + UseStdio = true, + Environment = new Dictionary(), + }); + + try + { + await client.StartAsync(); + Assert.Equal(ConnectionState.Connected, client.State); + + var pong = await client.PingAsync("empty-env"); + Assert.Equal("pong: empty-env", pong.Message); + + await client.StopAsync(); + } + finally + { + await client.ForceStopAsync(); + } + } + + // ── Partial-dict merge cases ────────────────────────────────────────────── + + [Fact] + public async Task Should_Start_When_Environment_Has_One_Custom_Key() + { + // This is the canonical regression test for Issue #441. + // + // The user provides a single custom environment variable -- a perfectly + // reasonable thing to do (e.g. to set COPILOT_API_URL, a proxy, etc.). + // + // Before the fix: + // startInfo.Environment.Clear() ← removes PATH, SystemRoot, COMSPEC … + // startInfo.Environment["MY_KEY"] = "value" + // → CLI subprocess starts with only MY_KEY → crashes immediately + // → StartAsync() throws IOException + // + // After the fix: + // startInfo.Environment["MY_KEY"] = "value" (merged) + // → CLI subprocess retains all inherited vars + MY_KEY → starts normally + using var client = new CopilotClient(new CopilotClientOptions + { + UseStdio = true, + Environment = new Dictionary + { + ["MY_CUSTOM_SDK_VAR"] = "hello_world", + }, + }); + + try + { + // This line would throw before the fix: + // System.IO.IOException: CLI process exited unexpectedly … + await client.StartAsync(); + Assert.Equal(ConnectionState.Connected, client.State); + + var pong = await client.PingAsync("one-key-env"); + Assert.Equal("pong: one-key-env", pong.Message); + + await client.StopAsync(); + } + finally + { + await client.ForceStopAsync(); + } + } + + [Fact] + public async Task Should_Start_When_Environment_Has_Multiple_Custom_Keys() + { + // Multiple custom keys, none of them system variables. + // Proves that the merge works for an arbitrary number of custom entries. + using var client = new CopilotClient(new CopilotClientOptions + { + UseStdio = true, + Environment = new Dictionary + { + ["SDK_TEST_VAR_A"] = "alpha", + ["SDK_TEST_VAR_B"] = "beta", + ["SDK_TEST_VAR_C"] = "gamma", + }, + }); + + try + { + await client.StartAsync(); + Assert.Equal(ConnectionState.Connected, client.State); + + var pong = await client.PingAsync("multi-key-env"); + Assert.Equal("pong: multi-key-env", pong.Message); + + await client.StopAsync(); + } + finally + { + await client.ForceStopAsync(); + } + } + + [Fact] + public async Task Should_Start_When_Environment_Overrides_An_Inherited_Key() + { + // Overriding an EXISTING env var (e.g. COPILOT_LOG_LEVEL) should work: + // the override takes effect, and all other inherited vars remain. + using var client = new CopilotClient(new CopilotClientOptions + { + UseStdio = true, + Environment = new Dictionary + { + // Override a var that is almost certainly already present in the + // parent process environment so we exercise the "override" code path. + ["PATH"] = System.Environment.GetEnvironmentVariable("PATH") ?? "/usr/bin", + }, + }); + + try + { + await client.StartAsync(); + Assert.Equal(ConnectionState.Connected, client.State); + + var pong = await client.PingAsync("override-inherited-key"); + Assert.Equal("pong: override-inherited-key", pong.Message); + + await client.StopAsync(); + } + finally + { + await client.ForceStopAsync(); + } + } + + // ── Verifying the merge semantics via the harness pattern ────────────── + + [Fact] + public async Task TestHarness_GetEnvironment_Pattern_Works_After_Fix() + { + // The E2E test harness (E2ETestContext.GetEnvironment) follows this pattern: + // + // var env = Environment.GetEnvironmentVariables() + // .Cast() + // .ToDictionary(...); + // env["COPILOT_API_URL"] = proxyUrl; // ← override + // env["XDG_CONFIG_HOME"] = homeDir; // ← override + // env["XDG_STATE_HOME"] = homeDir; // ← override + // return env; + // + // This pattern always supplied the FULL environment, so it happened to work + // even before the fix. Here we verify the same pattern continues to work. + var fullEnvWithOverrides = System.Environment.GetEnvironmentVariables() + .Cast() + .ToDictionary(e => (string)e.Key, e => e.Value?.ToString() ?? ""); + + fullEnvWithOverrides["SDK_HARNESS_STYLE_OVERRIDE"] = "harness_value"; + + using var client = new CopilotClient(new CopilotClientOptions + { + UseStdio = true, + Environment = fullEnvWithOverrides, + }); + + try + { + await client.StartAsync(); + Assert.Equal(ConnectionState.Connected, client.State); + + var pong = await client.PingAsync("harness-pattern"); + Assert.Equal("pong: harness-pattern", pong.Message); + + await client.StopAsync(); + } + finally + { + await client.ForceStopAsync(); + } + } + + // ── NODE_DEBUG is always stripped ───────────────────────────────────────── + + [Fact] + public async Task Should_Strip_NODE_DEBUG_Even_When_Environment_Is_Null() + { + // Client.cs always calls startInfo.Environment.Remove("NODE_DEBUG") after + // the merge step, so the CLI subprocess never sees NODE_DEBUG regardless of + // whether the parent process has it set. The CLI must start normally. + var envWithNodeDebug = System.Environment.GetEnvironmentVariables() + .Cast() + .ToDictionary(e => (string)e.Key, e => e.Value?.ToString() ?? ""); + envWithNodeDebug["NODE_DEBUG"] = "http,net"; // would pollute CLI stdout if kept + + using var client = new CopilotClient(new CopilotClientOptions + { + UseStdio = true, + Environment = envWithNodeDebug, + }); + + try + { + await client.StartAsync(); + Assert.Equal(ConnectionState.Connected, client.State); + + var pong = await client.PingAsync("node-debug-stripped"); + Assert.Equal("pong: node-debug-stripped", pong.Message); + + await client.StopAsync(); + } + finally + { + await client.ForceStopAsync(); + } + } +} diff --git a/nodejs/test/client.test.ts b/nodejs/test/client.test.ts index 0b98ebcb8..484a36c1b 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -842,6 +842,134 @@ describe("CopilotClient", () => { }); }); + // ───────────────────────────────────────────────────────────────────────── + // env option -- full-replacement semantics (contrast with .NET merge behavior) + // + // Node.js / child_process.spawn semantics: + // When `env` is provided, it becomes the ENTIRE environment of the child + // process. There is no automatic merging with process.env. + // + // SDK behavior (client.ts constructor): + // const effectiveEnv = options.env ?? process.env; + // + // - options.env is undefined → child process inherits process.env in full + // - options.env is a dict → child process gets ONLY that dict + // + // This is DIFFERENT from the fixed .NET SDK behavior, where + // CopilotClientOptions.Environment is MERGED into the inherited environment. + // + // Context for Issue #441: + // The .NET SDK had a bug where it called startInfo.Environment.Clear() before + // applying user overrides. Node.js never had this bug because the spawn `env` + // option has always been full-replacement -- there is no pre-populated dict to + // accidentally wipe. The bug existed only in the .NET SDK. + // ───────────────────────────────────────────────────────────────────────── + describe("env option", () => { + it("uses process.env when env is not specified", async () => { + // The SDK sets: effectiveEnv = options.env ?? process.env + // When env is omitted the CLI subprocess inherits everything from the + // parent process, including PATH, so it starts normally. + const client = new CopilotClient({ logLevel: "error" }); + await client.start(); + onTestFinished(() => client.forceStop()); + + // The internal options.env should reference the same process.env object + expect((client as any).options.env).toBe(process.env); + }); + + it("stores provided env as-is (full replacement by Node.js spawn)", async () => { + // When an explicit env dict is provided, the SDK stores it verbatim and + // passes it directly to child_process.spawn. Node.js spawn does NOT + // merge it with process.env -- it replaces the environment entirely. + // + // This is intentional in Node.js (unlike the .NET bug where Clear() was + // unintentional). Callers who want merge semantics must spread process.env + // themselves: env: { ...process.env, MY_VAR: "value" } + // + // The test harness (sdkTestContext.ts) always does exactly this: + // const env = { ...process.env, COPILOT_API_URL: proxyUrl, ... } + const customEnv = { ...process.env, MY_CUSTOM_SDK_VAR: "hello" } as Record< + string, + string | undefined + >; + const client = new CopilotClient({ env: customEnv, logLevel: "error" }); + + // The stored env is the same object we passed in + expect((client as any).options.env).toBe(customEnv); + }); + + it("starts and pings successfully when env is undefined (inherits process.env)", async () => { + // Regression guard: omitting env must not break the client. + const client = new CopilotClient({ logLevel: "error" }); + await client.start(); + onTestFinished(() => client.forceStop()); + + const pong = await client.ping("env-undefined"); + expect(pong.message).toBe("pong: env-undefined"); + }); + + it("starts and pings successfully when env is a full copy of process.env", async () => { + // Providing env: { ...process.env } is equivalent to not providing env + // at all. Both cases give the child process the same environment. + const client = new CopilotClient({ + env: { ...process.env } as Record, + logLevel: "error", + }); + await client.start(); + onTestFinished(() => client.forceStop()); + + const pong = await client.ping("env-full-copy"); + expect(pong.message).toBe("pong: env-full-copy"); + }); + + it("starts and pings successfully when env is a full copy of process.env plus a custom key", async () => { + // This mirrors exactly the pattern that test harnesses use in every + // language SDK -- spread the full environment then add overrides. + // It also mirrors the *correct* way to do per-test env overrides in Node.js + // (as opposed to the partial-dict-with-merge approach that .NET now supports). + const client = new CopilotClient({ + env: { + ...process.env, + SDK_ENV_TEST_CUSTOM: "test_value", + } as Record, + logLevel: "error", + }); + await client.start(); + onTestFinished(() => client.forceStop()); + + const pong = await client.ping("env-spread-plus-custom"); + expect(pong.message).toBe("pong: env-spread-plus-custom"); + }); + + it("NODE_DEBUG is stripped from env before spawning the CLI subprocess", async () => { + // Client.ts removes NODE_DEBUG so it cannot pollute the CLI's stdout + // (the SDK reads CLI stdout as a JSON-RPC message stream). + // This removal happens regardless of whether env is provided or not. + const client = new CopilotClient({ + env: { + ...process.env, + NODE_DEBUG: "http,net", // would corrupt JSON-RPC if not removed + } as Record, + logLevel: "error", + }); + await client.start(); + onTestFinished(() => client.forceStop()); + + // Verify the env passed to spawn does NOT contain NODE_DEBUG + const spawnedEnv = (client as any).options.env as Record< + string, + string | undefined + >; + // The original options.env still has it (it's not mutated) + expect(spawnedEnv["NODE_DEBUG"]).toBe("http,net"); + + // But the CLI starts fine because startCLIServer spreads the env and + // deletes NODE_DEBUG before passing it to spawn + const pong = await client.ping("node-debug-stripped"); + expect(pong.message).toBe("pong: node-debug-stripped"); + }); + }); + describe("ui elicitation", () => { it("reads capabilities from session.create response", async () => { const client = new CopilotClient(); From 21cd7d8e9bec48be646cbfd0c9b9106e69fe7087 Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 31 Mar 2026 14:19:05 -0500 Subject: [PATCH 3/4] fix(nodejs,python): align env semantics to merge-override, update Go docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four SDKs now document and implement the same ergonomic contract: providing a partial env dict ADDS/OVERRIDES those keys while keeping PATH, HOME, and all other inherited variables intact. ## Node.js (nodejs/src/client.ts + types.ts + README.md) Changed: const effectiveEnv = options.env ?? process.env; To: const effectiveEnv = options.env ? { ...process.env, ...options.env } : process.env; A partial dict like { COPILOT_API_URL: '...' } now merges into process.env instead of replacing it entirely. The test-harness pattern of { ...process.env, KEY: val } continues to work unchanged. Updated JSDoc for CopilotClientOptions.env with merge semantics and example. Added env option to the README options table (it was missing). ## Python (python/copilot/client.py + README.md) Changed _start_process to: env = dict(os.environ) if cfg.env is not None: env.update(cfg.env) Applied the same merge at the CLI-path-lookup site (effective_env). Updated the ClientConfig.env docstring with merge semantics and example. ## Go (go/types.go + README.md) - docs only Go's Env []string uses full-replacement semantics (matching exec.Cmd.Env). The comment and README now explicitly document this, note the difference from the other three SDKs, and show the correct idiom: env := append(os.Environ(), 'KEY=value') Go behavior is unchanged; callers already need to pass os.Environ() when they want partial overrides, and the type makes that clear. ## Tests (nodejs/test/client.test.ts) Updated the 'env option' describe block to reflect the new merge semantics: - Renamed 'stores provided env as-is' → 'merges provided env keys into process.env' with assertions proving the merged object differs from the input dict and preserves the PATH key. - Added a new test: 'starts and pings when env is a partial dict with one custom key' -- this is the canonical merge regression test for Node.js. - Updated block comment from 'full-replacement' to 'merge semantics'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- go/README.md | 2 +- go/types.go | 23 ++++++-- nodejs/README.md | 1 + nodejs/src/client.ts | 7 ++- nodejs/src/types.ts | 12 +++- nodejs/test/client.test.ts | 110 ++++++++++++++++--------------------- python/README.md | 2 +- python/copilot/client.py | 30 +++++++--- 8 files changed, 108 insertions(+), 79 deletions(-) diff --git a/go/README.md b/go/README.md index f29ef9fb7..dbe1e76a1 100644 --- a/go/README.md +++ b/go/README.md @@ -139,7 +139,7 @@ Event types: `SessionLifecycleCreated`, `SessionLifecycleDeleted`, `SessionLifec - `UseStdio` (bool): Use stdio transport instead of TCP (default: true) - `LogLevel` (string): Log level (default: "info") - `AutoStart` (\*bool): Auto-start server on first use (default: true). Use `Bool(false)` to disable. -- `Env` ([]string): Environment variables for CLI process (default: inherits from current process) +- `Env` ([]string): Full environment for the CLI process as `"KEY=VALUE"` strings (default: inherits from current process when nil). **Unlike the Node.js, Python, and .NET SDKs, the Go SDK uses full-replacement semantics**: when non-nil, this slice becomes the CLI process's complete environment. To add or override specific keys while preserving system variables, start from `os.Environ()`: `env := append(os.Environ(), "MY_KEY=value")`. - `GitHubToken` (string): GitHub token for authentication. When provided, takes priority over other auth methods. - `UseLoggedInUser` (\*bool): Whether to use logged-in user for authentication (default: true, but false when `GitHubToken` is provided). Cannot be used with `CLIUrl`. - `Telemetry` (\*TelemetryConfig): OpenTelemetry configuration for the CLI process. Providing this enables telemetry — no separate flag needed. See [Telemetry](#telemetry) below. diff --git a/go/types.go b/go/types.go index f888c9b6e..34f5f325b 100644 --- a/go/types.go +++ b/go/types.go @@ -40,11 +40,24 @@ type ClientOptions struct { AutoStart *bool // Deprecated: AutoRestart has no effect and will be removed in a future release. AutoRestart *bool - // Env is the environment variables for the CLI process (default: inherits from current process). - // Each entry is of the form "key=value". - // If Env is nil, the new process uses the current process's environment. - // If Env contains duplicate environment keys, only the last value in the - // slice for each duplicate key is used. + // Env specifies the full environment for the CLI process. Each entry must be + // of the form "key=value". + // + // If Env is nil (the default), the CLI process inherits the current process's + // environment unchanged. + // + // If Env is non-nil, it becomes the COMPLETE environment of the CLI process — + // variables not listed here are NOT inherited. To add or override a few keys + // while preserving PATH and other system variables, start from os.Environ(): + // + // env := append(os.Environ(), "COPILOT_API_URL=http://proxy:8080") + // + // Note: the Go SDK behaves differently from the Node.js, Python, and .NET SDKs, + // which automatically merge user-provided keys into the inherited environment. + // In Go, the []string format makes full replacement the natural choice; callers + // must explicitly include inherited variables when partial overrides are needed. + // + // If Env contains duplicate keys, only the last value for each key is used. Env []string // GitHubToken is the GitHub token to use for authentication. // When provided, the token is passed to the CLI server via environment variable. diff --git a/nodejs/README.md b/nodejs/README.md index eee4c2b65..6b5227b79 100644 --- a/nodejs/README.md +++ b/nodejs/README.md @@ -86,6 +86,7 @@ new CopilotClient(options?: CopilotClientOptions) - `useStdio?: boolean` - Use stdio transport instead of TCP (default: true) - `logLevel?: string` - Log level (default: "info") - `autoStart?: boolean` - Auto-start server (default: true) +- `env?: Record` - Extra environment variables for the CLI process. Specified keys are **merged into** `process.env` (they override or add to inherited variables; all other variables remain intact). When not set, the CLI process inherits `process.env` unchanged. - `githubToken?: string` - GitHub token for authentication. When provided, takes priority over other auth methods. - `useLoggedInUser?: boolean` - Whether to use logged-in user for authentication (default: true, but false when `githubToken` is provided). Cannot be used with `cliUrl`. - `telemetry?: TelemetryConfig` - OpenTelemetry configuration for the CLI process. Providing this object enables telemetry — no separate flag needed. See [Telemetry](#telemetry) below. diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 23aac99a3..9554eb856 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -313,7 +313,12 @@ export class CopilotClient { this.onGetTraceContext = options.onGetTraceContext; this.sessionFsConfig = options.sessionFs ?? null; - const effectiveEnv = options.env ?? process.env; + // Merge user-provided env overrides into the current process environment. + // Providing a partial dict (e.g. { COPILOT_API_URL: "..." }) adds/overrides + // those keys while keeping PATH, HOME, and all other inherited variables intact. + // Users who need a fully-isolated environment can pass { ...process.env } as a + // base themselves, but that case is rare and explicit. + const effectiveEnv = options.env ? { ...process.env, ...options.env } : process.env; this.options = { cliPath: options.cliUrl ? undefined diff --git a/nodejs/src/types.ts b/nodejs/src/types.ts index ceca07d64..ea0dfa418 100644 --- a/nodejs/src/types.ts +++ b/nodejs/src/types.ts @@ -115,7 +115,17 @@ export interface CopilotClientOptions { autoRestart?: boolean; /** - * Environment variables to pass to the CLI process. If not set, inherits process.env. + * Extra environment variables to set for the CLI process. + * + * When provided, the specified keys are **merged into** (override or add to) the + * current `process.env`. All other inherited variables (PATH, HOME, etc.) remain + * intact. When not set, the CLI process inherits `process.env` unchanged. + * + * @example + * ```typescript + * // Add one override — PATH and everything else are still inherited + * const client = new CopilotClient({ env: { COPILOT_API_URL: "http://proxy:8080" } }); + * ``` */ env?: Record; diff --git a/nodejs/test/client.test.ts b/nodejs/test/client.test.ts index 484a36c1b..02b4d3cb9 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -843,59 +843,54 @@ describe("CopilotClient", () => { }); // ───────────────────────────────────────────────────────────────────────── - // env option -- full-replacement semantics (contrast with .NET merge behavior) - // - // Node.js / child_process.spawn semantics: - // When `env` is provided, it becomes the ENTIRE environment of the child - // process. There is no automatic merging with process.env. + // env option -- merge semantics (consistent with .NET and Python SDKs) // // SDK behavior (client.ts constructor): - // const effectiveEnv = options.env ?? process.env; + // const effectiveEnv = options.env ? { ...process.env, ...options.env } : process.env; // - // - options.env is undefined → child process inherits process.env in full - // - options.env is a dict → child process gets ONLY that dict + // - options.env is undefined → effectiveEnv is process.env (by reference) + // - options.env is a dict → effectiveEnv is process.env MERGED WITH options.env + // (user keys override, all inherited vars stay) // - // This is DIFFERENT from the fixed .NET SDK behavior, where - // CopilotClientOptions.Environment is MERGED into the inherited environment. + // This matches .NET (Environment merge-override) and Python (dict(os.environ) + update). // // Context for Issue #441: - // The .NET SDK had a bug where it called startInfo.Environment.Clear() before - // applying user overrides. Node.js never had this bug because the spawn `env` - // option has always been full-replacement -- there is no pre-populated dict to - // accidentally wipe. The bug existed only in the .NET SDK. + // The .NET SDK had a bug where startInfo.Environment.Clear() wiped PATH and all other + // inherited vars when the user provided even ONE custom env key. After fixing that, + // we aligned Node.js and Python to the same merge semantics so that providing a + // partial dict (e.g. { COPILOT_API_URL: "..." }) works correctly in all SDKs. // ───────────────────────────────────────────────────────────────────────── describe("env option", () => { - it("uses process.env when env is not specified", async () => { - // The SDK sets: effectiveEnv = options.env ?? process.env - // When env is omitted the CLI subprocess inherits everything from the - // parent process, including PATH, so it starts normally. + it("uses process.env by reference when env is not specified", async () => { + // When env is omitted, effectiveEnv = process.env (falsy branch of ternary). + // The CLI subprocess inherits everything from the parent process. const client = new CopilotClient({ logLevel: "error" }); await client.start(); onTestFinished(() => client.forceStop()); - // The internal options.env should reference the same process.env object + // Internal options.env should be the same process.env reference expect((client as any).options.env).toBe(process.env); }); - it("stores provided env as-is (full replacement by Node.js spawn)", async () => { - // When an explicit env dict is provided, the SDK stores it verbatim and - // passes it directly to child_process.spawn. Node.js spawn does NOT - // merge it with process.env -- it replaces the environment entirely. - // - // This is intentional in Node.js (unlike the .NET bug where Clear() was - // unintentional). Callers who want merge semantics must spread process.env - // themselves: env: { ...process.env, MY_VAR: "value" } + it("merges provided env keys into process.env (does not replace entirely)", async () => { + // When an explicit env dict is provided, the SDK creates a merged object: + // { ...process.env, ...options.env } + // This means PATH, HOME, and all other inherited variables are preserved + // while the user's keys override or add to them. // - // The test harness (sdkTestContext.ts) always does exactly this: - // const env = { ...process.env, COPILOT_API_URL: proxyUrl, ... } - const customEnv = { ...process.env, MY_CUSTOM_SDK_VAR: "hello" } as Record< - string, - string | undefined - >; + // The test harness pattern (sdkTestContext.ts) spreads process.env anyway: + // { ...process.env, COPILOT_API_URL: proxyUrl, ... } + // Under merge semantics this becomes: + // { ...process.env, ...{ ...process.env, COPILOT_API_URL: proxyUrl } } + // = { ...process.env, COPILOT_API_URL: proxyUrl } ← same result, backward-compatible. + const customEnv = { MY_CUSTOM_SDK_VAR: "hello" } as Record; const client = new CopilotClient({ env: customEnv, logLevel: "error" }); - // The stored env is the same object we passed in - expect((client as any).options.env).toBe(customEnv); + // The stored env is the MERGED result, not the original customEnv object + const storedEnv = (client as any).options.env as Record; + expect(storedEnv).not.toBe(customEnv); // it's a new merged object + expect(storedEnv["MY_CUSTOM_SDK_VAR"]).toBe("hello"); // user key is present + expect(storedEnv["PATH"]).toBe(process.env["PATH"]); // inherited key is preserved }); it("starts and pings successfully when env is undefined (inherits process.env)", async () => { @@ -908,25 +903,25 @@ describe("CopilotClient", () => { expect(pong.message).toBe("pong: env-undefined"); }); - it("starts and pings successfully when env is a full copy of process.env", async () => { - // Providing env: { ...process.env } is equivalent to not providing env - // at all. Both cases give the child process the same environment. + it("starts and pings successfully when env is a partial dict with one custom key", async () => { + // Core merge-semantics test: providing just { MY_VAR: "value" } keeps PATH + // and all other inherited vars, so the CLI subprocess starts normally. + // Before the Issue #441 fix, this pattern would have crashed the CLI in .NET. + // After aligning all SDKs, it works correctly everywhere. const client = new CopilotClient({ - env: { ...process.env } as Record, + env: { SDK_ENV_TEST_CUSTOM: "test_value" }, logLevel: "error", }); await client.start(); onTestFinished(() => client.forceStop()); - const pong = await client.ping("env-full-copy"); - expect(pong.message).toBe("pong: env-full-copy"); + const pong = await client.ping("env-partial-dict"); + expect(pong.message).toBe("pong: env-partial-dict"); }); it("starts and pings successfully when env is a full copy of process.env plus a custom key", async () => { - // This mirrors exactly the pattern that test harnesses use in every - // language SDK -- spread the full environment then add overrides. - // It also mirrors the *correct* way to do per-test env overrides in Node.js - // (as opposed to the partial-dict-with-merge approach that .NET now supports). + // The test-harness spread pattern still works under merge semantics. + // { ...process.env, MY_KEY: "val" } merged with process.env = same dict. const client = new CopilotClient({ env: { ...process.env, @@ -941,30 +936,21 @@ describe("CopilotClient", () => { expect(pong.message).toBe("pong: env-spread-plus-custom"); }); - it("NODE_DEBUG is stripped from env before spawning the CLI subprocess", async () => { - // Client.ts removes NODE_DEBUG so it cannot pollute the CLI's stdout - // (the SDK reads CLI stdout as a JSON-RPC message stream). - // This removal happens regardless of whether env is provided or not. + it("NODE_DEBUG is stripped from merged env before spawning the CLI subprocess", async () => { + // The SDK always removes NODE_DEBUG from the effective env before spawn so it + // cannot corrupt the JSON-RPC message stream (CLI stdout). const client = new CopilotClient({ - env: { - ...process.env, - NODE_DEBUG: "http,net", // would corrupt JSON-RPC if not removed - } as Record, + env: { NODE_DEBUG: "http,net" } as Record, logLevel: "error", }); await client.start(); onTestFinished(() => client.forceStop()); - // Verify the env passed to spawn does NOT contain NODE_DEBUG - const spawnedEnv = (client as any).options.env as Record< - string, - string | undefined - >; - // The original options.env still has it (it's not mutated) - expect(spawnedEnv["NODE_DEBUG"]).toBe("http,net"); + // The merged env stored in options.env contains NODE_DEBUG (from our override) + const storedEnv = (client as any).options.env as Record; + expect(storedEnv["NODE_DEBUG"]).toBe("http,net"); - // But the CLI starts fine because startCLIServer spreads the env and - // deletes NODE_DEBUG before passing it to spawn + // But the CLI starts fine because startCLIServer removes NODE_DEBUG before spawn const pong = await client.ping("node-debug-stripped"); expect(pong.message).toBe("pong: node-debug-stripped"); }); diff --git a/python/README.md b/python/README.md index 33f62c2d4..fab24dbb3 100644 --- a/python/README.md +++ b/python/README.md @@ -146,7 +146,7 @@ CopilotClient( - `use_stdio` (bool): Use stdio transport instead of TCP (default: True) - `port` (int): Server port for TCP mode (default: 0 for random) - `log_level` (str): Log level (default: "info") -- `env` (dict | None): Environment variables for the CLI process +- `env` (dict | None): Extra environment variables for the CLI process. Specified keys are **merged into** `os.environ` (they override or add to inherited variables; all other variables remain intact). When `None`, the CLI process inherits `os.environ` unchanged. - `github_token` (str | None): GitHub token for authentication. When provided, takes priority over other auth methods. - `use_logged_in_user` (bool | None): Whether to use logged-in user for authentication (default: True, but False when `github_token` is provided). - `telemetry` (dict | None): OpenTelemetry configuration for the CLI process. Providing this enables telemetry — no separate flag needed. See [Telemetry](#telemetry) below. diff --git a/python/copilot/client.py b/python/copilot/client.py index ab8074756..da8dc5208 100644 --- a/python/copilot/client.py +++ b/python/copilot/client.py @@ -110,7 +110,17 @@ class SubprocessConfig: """Log level for the CLI process.""" env: dict[str, str] | None = None - """Environment variables for the CLI process. ``None`` inherits the current env.""" + """Extra environment variables for the CLI process. + + When provided, the specified keys are **merged into** (override or add to) the + current ``os.environ``. All other inherited variables (PATH, HOME, etc.) remain + intact. When ``None``, the CLI process inherits ``os.environ`` unchanged. + + Example:: + + config = ClientConfig(env={"COPILOT_API_URL": "http://proxy:8080"}) + # PATH and everything else are still inherited + """ github_token: str | None = None """GitHub token for authentication. Takes priority over other auth methods.""" @@ -793,8 +803,11 @@ def __init__( else: self._actual_port = None - # Resolve CLI path: explicit > COPILOT_CLI_PATH env var > bundled binary - effective_env = config.env if config.env is not None else os.environ + # Resolve CLI path: explicit > COPILOT_CLI_PATH env var (merged) > bundled binary + # Build the effective (merged) env early so COPILOT_CLI_PATH lookup sees it. + effective_env: dict[str, str] = dict(os.environ) + if config.env is not None: + effective_env.update(config.env) if config.cli_path is None: env_cli_path = effective_env.get("COPILOT_CLI_PATH") if env_cli_path: @@ -2032,11 +2045,12 @@ async def _start_cli_server(self) -> None: else: args = [cli_path] + args - # Get environment variables - if cfg.env is None: - env = dict(os.environ) - else: - env = dict(cfg.env) + # Build subprocess environment: start from os.environ, then merge in any + # user-provided overrides. This preserves PATH, HOME, and all other inherited + # variables when the caller only specifies a few custom keys. + env = dict(os.environ) + if cfg.env is not None: + env.update(cfg.env) # Set auth token in environment if provided if cfg.github_token: From 56d810a0c36b27b96380e5782cc3e16ffbed0b8a Mon Sep 17 00:00:00 2001 From: Shane Date: Thu, 2 Apr 2026 16:11:13 -0500 Subject: [PATCH 4/4] fix: address Copilot review comments on PR #993 - python/copilot/client.py: fix docstring example to use SubprocessConfig instead of ClientConfig (which doesn't exist) - nodejs/src/client.ts: remove misleading comment suggesting a fully-isolated env is possible via merge semantics - nodejs/README.md: align env type to Record to match CopilotClientOptions and document that undefined unsets an inherited variable - dotnet/test/EnvironmentTests.cs: rename test from Should_Strip_NODE_DEBUG_Even_When_Environment_Is_Null to Should_Strip_NODE_DEBUG_When_Environment_Dict_Is_Provided to accurately describe what the test does (it passes a non-null Environment dict) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/test/EnvironmentTests.cs | 2 +- nodejs/README.md | 2 +- nodejs/src/client.ts | 2 -- python/copilot/client.py | 2 +- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/dotnet/test/EnvironmentTests.cs b/dotnet/test/EnvironmentTests.cs index 37adb7913..0d462442b 100644 --- a/dotnet/test/EnvironmentTests.cs +++ b/dotnet/test/EnvironmentTests.cs @@ -254,7 +254,7 @@ public async Task TestHarness_GetEnvironment_Pattern_Works_After_Fix() // ── NODE_DEBUG is always stripped ───────────────────────────────────────── [Fact] - public async Task Should_Strip_NODE_DEBUG_Even_When_Environment_Is_Null() + public async Task Should_Strip_NODE_DEBUG_When_Environment_Dict_Is_Provided() { // Client.cs always calls startInfo.Environment.Remove("NODE_DEBUG") after // the merge step, so the CLI subprocess never sees NODE_DEBUG regardless of diff --git a/nodejs/README.md b/nodejs/README.md index 6b5227b79..baa035539 100644 --- a/nodejs/README.md +++ b/nodejs/README.md @@ -86,7 +86,7 @@ new CopilotClient(options?: CopilotClientOptions) - `useStdio?: boolean` - Use stdio transport instead of TCP (default: true) - `logLevel?: string` - Log level (default: "info") - `autoStart?: boolean` - Auto-start server (default: true) -- `env?: Record` - Extra environment variables for the CLI process. Specified keys are **merged into** `process.env` (they override or add to inherited variables; all other variables remain intact). When not set, the CLI process inherits `process.env` unchanged. +- `env?: Record` - Extra environment variables for the CLI process. Specified keys are **merged into** `process.env` (they override or add to inherited variables; all other variables remain intact). Pass `undefined` as a value to unset an inherited variable (e.g., `{ NODE_DEBUG: undefined }`). When not set, the CLI process inherits `process.env` unchanged. - `githubToken?: string` - GitHub token for authentication. When provided, takes priority over other auth methods. - `useLoggedInUser?: boolean` - Whether to use logged-in user for authentication (default: true, but false when `githubToken` is provided). Cannot be used with `cliUrl`. - `telemetry?: TelemetryConfig` - OpenTelemetry configuration for the CLI process. Providing this object enables telemetry — no separate flag needed. See [Telemetry](#telemetry) below. diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 9554eb856..d523d7b92 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -316,8 +316,6 @@ export class CopilotClient { // Merge user-provided env overrides into the current process environment. // Providing a partial dict (e.g. { COPILOT_API_URL: "..." }) adds/overrides // those keys while keeping PATH, HOME, and all other inherited variables intact. - // Users who need a fully-isolated environment can pass { ...process.env } as a - // base themselves, but that case is rare and explicit. const effectiveEnv = options.env ? { ...process.env, ...options.env } : process.env; this.options = { cliPath: options.cliUrl diff --git a/python/copilot/client.py b/python/copilot/client.py index da8dc5208..60f2c3c60 100644 --- a/python/copilot/client.py +++ b/python/copilot/client.py @@ -118,7 +118,7 @@ class SubprocessConfig: Example:: - config = ClientConfig(env={"COPILOT_API_URL": "http://proxy:8080"}) + config = SubprocessConfig(env={"COPILOT_API_URL": "http://proxy:8080"}) # PATH and everything else are still inherited """