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; } /// diff --git a/dotnet/test/EnvironmentTests.cs b/dotnet/test/EnvironmentTests.cs new file mode 100644 index 000000000..0d462442b --- /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_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 + // 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/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..baa035539 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). 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 23aac99a3..d523d7b92 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -313,7 +313,10 @@ 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. + 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 0b98ebcb8..02b4d3cb9 100644 --- a/nodejs/test/client.test.ts +++ b/nodejs/test/client.test.ts @@ -842,6 +842,120 @@ describe("CopilotClient", () => { }); }); + // ───────────────────────────────────────────────────────────────────────── + // env option -- merge semantics (consistent with .NET and Python SDKs) + // + // SDK behavior (client.ts constructor): + // const effectiveEnv = options.env ? { ...process.env, ...options.env } : process.env; + // + // - 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 matches .NET (Environment merge-override) and Python (dict(os.environ) + update). + // + // Context for Issue #441: + // 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 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()); + + // Internal options.env should be the same process.env reference + expect((client as any).options.env).toBe(process.env); + }); + + 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 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 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 () => { + // 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 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: { SDK_ENV_TEST_CUSTOM: "test_value" }, + logLevel: "error", + }); + await client.start(); + onTestFinished(() => client.forceStop()); + + 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 () => { + // 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, + 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 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: { NODE_DEBUG: "http,net" } as Record, + logLevel: "error", + }); + await client.start(); + onTestFinished(() => client.forceStop()); + + // 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 removes NODE_DEBUG before 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(); 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..60f2c3c60 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 = SubprocessConfig(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: