Skip to content

C#: make per-client Environment coherent per transport#1930

Merged
SteveSandersonMS merged 2 commits into
mainfrom
stevesa/env-restructure
Jul 7, 2026
Merged

C#: make per-client Environment coherent per transport#1930
SteveSandersonMS merged 2 commits into
mainfrom
stevesa/env-restructure

Conversation

@SteveSandersonMS

Copy link
Copy Markdown
Contributor

Problem

CopilotClientOptions.Environment is a per-client dictionary. That model works for child-process transports (each client = its own OS process with its own environment) but is incoherent for the in-process (FFI) transport, where the native Rust runtime loads into the single shared .NET host process. A process has one environment block, so "per-client env vars" cannot be honored for host-resident native code — the option silently did nothing there.

What this PR does (C# / SDK slice)

SDK (dotnet/src)

  • Add Environment to ChildProcessRuntimeConnection — per-client env where it's actually coherent (stdio/TCP). Clear+replace semantics, matching options.Environment for easy migration.
  • Guards in CopilotClient (fail loud instead of fail silent):
    • throw if options.Environment is set with RuntimeConnection.ForInProcess()
    • throw if options.Telemetry is set with the in-process transport (telemetry lowers to env vars read host-side, so it can't be honored per-client in-process)
    • throw if both options.Environment and the connection's Environment are set

Test harness (dotnet/test)

  • E2ETestContext.CreateClient gains an environment parameter that routes the environment to the connection (child-process) or mirrors it onto the host process (in-process), and rejects options.Environment.
  • All E2E call sites converted off options.Environment onto the new parameter, dogfooding ChildProcessRuntimeConnection.Environment.
  • Removed the useStdio shortcut (misleading false == TCP overload); the few transport-pinned tests now set options.Connection directly.
  • Pinned the file-telemetry E2E test to stdio (telemetry is unsupported in-process under the new guard).

Scope / follow-ups

This is deliberately the SDK-side slice: it makes every previously-silent incoherence a loud error and gives child-process transports the correct API shape. It does not make in-process telemetry (or in-process env) actually work — that requires threading the exporter/env config through the FFI boundary as explicit per-connection config in the runtime, which is a separate change. Deprecating options.Environment outright is planned for v2.

Validation

  • SDK + tests build clean; dotnet format --verify-no-changes passes.
  • Representative converted tests pass under both stdio and in-process transports (incl. the CLI-path-dependent ConnectionTokenTests/SessionFsSqlite and the stdio-pinned telemetry test).

CopilotClientOptions.Environment is a per-client dictionary that only makes
sense for child-process transports (each client owns its own OS process). Under
the in-process (FFI) transport the native runtime loads into the shared host
process, whose single environment block cannot carry per-client values, so the
option silently did nothing for host-resident native code.

SDK changes:
- Add Environment to ChildProcessRuntimeConnection (clear+replace semantics,
  matching options.Environment for easy migration).
- Guard in CopilotClient: throw if options.Environment is set with the
  in-process transport; throw if options.Telemetry is set with the in-process
  transport (telemetry lowers to env vars read host-side); throw if both
  options.Environment and the connection's Environment are set.

Test harness changes:
- E2ETestContext.CreateClient gains an 'environment' parameter that routes the
  environment to the connection (child-process) or mirrors it onto the host
  process (in-process), and rejects options.Environment.
- Convert all E2E call sites off options.Environment onto the new parameter,
  dogfooding ChildProcessRuntimeConnection.Environment.
- Remove the useStdio shortcut (misleading false==TCP overload); tests that pin
  a transport now set options.Connection directly.
- Pin the file-telemetry E2E test to stdio (telemetry is unsupported in-process).

This is the SDK-side slice; making in-process telemetry actually work is a
follow-up runtime change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

… ctor

The Dictionary<TKey,TValue>(IEnumerable<KeyValuePair>) constructor does not
exist on .NET Framework 4.7.2; use LINQ ToDictionary, which works across all
target frameworks.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

All 18 changed files are in dotnet/ — no other SDK is modified. I reviewed whether the changes introduce patterns that should be replicated across Node.js, Python, Go, Java, and Rust.

Finding: No cross-SDK consistency action required.

Why this is .NET-specific

The core problem this PR fixes — CopilotClientOptions.Environment silently doing nothing for InProcessRuntimeConnection — is unique to .NET because it is the only SDK with an in-process (FFI) transport. The other five SDKs only have child-process and external-URI transport modes, so:

SDK In-process transport? Per-connection env? Notes
.NET (this PR) InProcessRuntimeConnection ✅ New ChildProcessRuntimeConnection.Environment Fix applies here
Node.js options.env only; no incoherence possible
Python CopilotClient(env=...) only; URI connections documented to ignore it
Go ClientOptions.Env only; no incoherence possible
Java CopilotClientOptions.environment only
Rust No equivalent architecture

New .NET API: ChildProcessRuntimeConnection.Environment

This is the right place for env vars in .NET's transport hierarchy, but is not a pattern other SDKs need to adopt. They each have a single transport level (client options) and no in-process variant, so there's no semantic ambiguity to resolve.

The three new validation guards (options.Environment with in-process, options.Telemetry with in-process, and both env fields set simultaneously) are all specific to .NET's architecture.

Generated by SDK Consistency Review Agent for issue #1930 · sonnet46 1.2M ·

@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review July 7, 2026 13:45
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner July 7, 2026 13:45
Copilot AI review requested due to automatic review settings July 7, 2026 13:45
@SteveSandersonMS SteveSandersonMS merged commit 7aaa13d into main Jul 7, 2026
41 of 42 checks passed
@SteveSandersonMS SteveSandersonMS deleted the stevesa/env-restructure branch July 7, 2026 13:46
Copilot stopped reviewing on behalf of SteveSandersonMS due to an error July 7, 2026 13:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors .NET E2E harness/client environment handling to support transport-aware environment routing, adds a per-connection environment option for child-process transports, and updates tests to use the new pattern.

Changes:

  • Update E2ETestContext.CreateClient to take an explicit environment parameter and route env vars via in-process host env or child-process connection env.
  • Add ChildProcessRuntimeConnection.Environment and validate incompatible environment/telemetry options for in-process transport in CopilotClient.
  • Update E2E tests to stop using CopilotClientOptions.Environment and to pin stdio/tcp via RuntimeConnection.
Show a summary per file
File Description
dotnet/test/Harness/E2ETestContext.cs Reworks client creation to route environment by transport and removes useStdio flag.
dotnet/test/E2E/TelemetryExportE2ETests.cs Pins stdio transport explicitly for telemetry export test.
dotnet/test/E2E/SubagentHooksE2ETests.cs Migrates env injection to CreateClient(environment: ...).
dotnet/test/E2E/SessionFsSqliteE2ETests.cs Replaces useStdio with RuntimeConnection.ForStdio().
dotnet/test/E2E/RpcSessionStateExtrasE2ETests.cs Migrates env injection to CreateClient(..., environment: env).
dotnet/test/E2E/RpcServerPluginsE2ETests.cs Migrates env injection to CreateClient(environment: ...).
dotnet/test/E2E/RpcServerMiscE2ETests.cs Migrates env handling to CreateClient(..., environment: env).
dotnet/test/E2E/RpcServerE2ETests.cs Migrates env injection and replaces useStdio with explicit stdio connection.
dotnet/test/E2E/RpcMcpAndSkillsE2ETests.cs Migrates env injection to CreateClient(environment: ...).
dotnet/test/E2E/RpcExtensionsLoadedE2ETests.cs Passes env via CreateClient(..., environment: ...) while keeping stdio args.
dotnet/test/E2E/ProviderEndpointE2ETests.cs Migrates env injection to CreateClient(environment: ...).
dotnet/test/E2E/PerSessionAuthE2ETests.cs Migrates env injection to CreateClient(..., environment: ...).
dotnet/test/E2E/ModeHandlersE2ETests.cs Migrates env injection to CreateClient(environment: ...).
dotnet/test/E2E/CopilotRequestWebSocketE2ETests.cs Migrates env injection to CreateClient(..., environment: ...).
dotnet/test/E2E/ClientOptionsE2ETests.cs Migrates env injection to CreateClient(..., environment: ...).
dotnet/test/ConnectionTokenTests.cs Replaces useStdio: false with explicit TCP connection.
dotnet/src/Types.cs Adds ChildProcessRuntimeConnection.Environment and clarifies CopilotClientOptions.Environment remarks.
dotnet/src/Client.cs Adds env/telemetry validation for in-process transport and supports connection-scoped env for child processes.

Review details

  • Files reviewed: 18/18 changed files
  • Comments generated: 4
  • Review effort level: Low

Comment thread dotnet/test/Harness/E2ETestContext.cs
Comment thread dotnet/test/Harness/E2ETestContext.cs
Comment thread dotnet/test/Harness/E2ETestContext.cs
Comment thread dotnet/src/Client.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants