fix: .NET Environment.Clear() wipes inherited env vars + cross-SDK merge alignment

[PureWeen]

Problem

Fixes PureWeen/PolyPilot#441.

In the .NET SDK, CopilotClientOptions.Environment silently clears and replaces the entire child process environment instead of merging user-specified keys with the inherited environment.

When a .NET embedder sets Environment to augment PATH or add a few variables, the SDK calls startInfo.Environment.Clear() (Client.cs), wiping all inherited platform variables (COMSPEC, SystemRoot, TEMP, LOCALAPPDATA, etc.). On Windows this breaks ConPTY shell spawning — the CLI's PowerShell tool fails with File not found: (empty path) because the shell executable can't be resolved without these variables.

Before (broken):

if (options.Environment != null)
{
    startInfo.Environment.Clear();       // wipes ALL inherited vars
    foreach (var entry in options.Environment)
        startInfo.Environment[entry.Key] = entry.Value;
}

Fix

Remove the Environment.Clear() call. In .NET, ProcessStartInfo.Environment is pre-populated with the current process's environment, so iterating through the user-provided keys naturally merges (overrides specified keys, inherits the rest).

After (fixed):

if (options.Environment != null)
{
    foreach (var entry in options.Environment)
        startInfo.Environment[entry.Key] = entry.Value;  // merge: override or add
}

This PR also aligns Node.js and Python to the same merge semantics, and documents Go's different behavior (full replacement by design).

Cross-SDK Comparison

SDK	Behavior when user provides env	Notes
.NET (this PR)	Merge — override specified keys, inherit rest	Natural for .NET: ProcessStartInfo.Environment is pre-seeded
Node.js (this PR)	Merge ({ ...process.env, ...options.env })	Fixed to match .NET
Python (this PR)	Merge (dict(os.environ); env.update(cfg.env))	Fixed to match .NET
Go	Full replacement (options.Env or os.Environ())	Documented: callers must pass append(os.Environ(), ...)

Testing

• 9 new EnvironmentTests in .NET prove merge semantics via public API only (StartAsync + PingAsync)
• 6 new Node.js env-option tests confirm merge semantics
• All existing tests continue to pass
• No breaking change: callers providing a full env dict get the same result; callers providing a partial dict now get correct merge behavior instead of a broken stripped environment

References

• Bug report: External: copilot-sdk Environment.Clear() replaces child process environment instead of merging PureWeen/PolyPilot#441 — filed in the PureWeen/PolyPilot issue tracker
• Related: External: copilot-cli shell tool 'File not found' error provides no diagnostic information PureWeen/PolyPilot#442 (downstream symptom: empty shell path in ConPTY)

[Copilot]

Pull request overview

Fixes the .NET SDK’s CLI subprocess environment handling so user-specified environment variables merge into the inherited environment (instead of clearing it), and aligns Node.js + Python to the same merge semantics while documenting Go’s intentional full-replacement behavior.

Changes:

• .NET: stop clearing ProcessStartInfo.Environment and add regression tests covering merge behavior.
• Node.js + Python: change subprocess env building to merge with inherited env; update SDK docs and tests accordingly.
• Go: clarify in docs/comments that Env is full-replacement and show how to start from os.Environ() for partial overrides.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
python/README.md	Documents Python env merge semantics for CLI subprocess.
python/copilot/client.py	Implements merged env for CLI path resolution and subprocess spawn; expands env docstring.
nodejs/test/client.test.ts	Adds tests asserting env merge semantics and NODE_DEBUG stripping behavior.
nodejs/src/types.ts	Updates public option docs for env to describe merge semantics.
nodejs/src/client.ts	Changes effective env construction to { ...process.env, ...options.env } merge semantics.
nodejs/README.md	Documents Node env merge semantics.
go/types.go	Documents Go’s full-replacement Env semantics and how to preserve inherited env.
go/README.md	Clarifies Go Env behavior vs other SDKs and provides os.Environ() example.
dotnet/test/EnvironmentTests.cs	Adds regression tests validating merge semantics and NODE_DEBUG stripping.
dotnet/src/Types.cs	Updates .NET options docs to describe merge semantics.
dotnet/src/Client.cs	Removes startInfo.Environment.Clear() to preserve inherited env vars.
dotnet/README.md	Documents .NET env merge behavior for the CLI process.

[Copilot]

SubprocessConfig.env docstring example refers to ClientConfig, but this module defines SubprocessConfig / ExternalServerConfig (and CopilotClient takes one of those). The example should use SubprocessConfig(env=...) (or show passing SubprocessConfig into CopilotClient) to avoid confusing users.

Suggested change

	config = ClientConfig(env={"COPILOT_API_URL": "http://proxy:8080"})
	config = SubprocessConfig(env={"COPILOT_API_URL": "http://proxy:8080"})

[Copilot]

The comment suggests users can get a “fully-isolated environment” by passing { ...process.env } as a base, but that still inherits the full parent environment (just as a copy). With the new merge semantics there’s also no way to request full-replacement behavior via env. Please adjust the comment to avoid implying isolation/replacement is supported (or document an explicit way to opt into replacement if that’s intended).

Suggested change

	// Users who need a fully-isolated environment can pass { ...process.env } as a
	// base themselves, but that case is rare and explicit.
	// This API does not currently support replacing the inherited environment
	// wholesale via `env`; values in `options.env` are always merged with process.env.

[Copilot]

README documents env?: Record<string, string>, but the public type is env?: Record<string, string | undefined> (and undefined is meaningful for child_process.spawn env values). Please align the README type signature with CopilotClientOptions.env to avoid confusing consumers.

Suggested change

	- `env?: Record<string, string>` - 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<string, string | undefined>` - 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.

[Copilot]

Test name says Environment_Is_Null, but the test sets Environment = envWithNodeDebug (non-null). Rename the test (or change it to actually pass Environment = null and set NODE_DEBUG on the parent process) so the intent matches the behavior under test.

[stephentoub]

I'm not convinced this is desirable. With the current design, you get to start from a blank slate and include only the environment variables you want, which could include starting from Environment.GetEnvironmentVariables() or from empty. With the proposed design, though, starting from a clean slate is much more cumbersome.