Redesign process transport options for v2 - #2536
Conversation
e16c5cd to
2d358e0
Compare
This comment has been minimized.
This comment has been minimized.
Move process-scoped launch settings from shared client options onto the out-of-process runtime connections across SDKs. Update docs, changelog migration notes, tests, and snapshots for the new v2 API boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2d358e0 to
a7bdc48
Compare
This comment has been minimized.
This comment has been minimized.
- Go: restore unconditional default of processEnv to os.Environ() when unset, matching pre-v2 behavior. The default previously only applied to out-of-process connections, which broke TestClient_EnvOptions under the inprocess CI matrix (COPILOT_SDK_DEFAULT_CONNECTION=inprocess) since the env default is harmless (and unused) for in-process transport but still part of the client's observable state. Removed the now-unused outOfProcessConnection interface and connEnv/ connWorkingDirectory methods that only existed to support the removed gate. - Python: fix two e2e tests in test_session_e2e.py that still passed working_directory/env as CopilotClient-level kwargs instead of on the RuntimeConnection, left over from an earlier pass of the v2 process/transport migration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore all test/snapshots/**/*.yaml files to match origin/main. These had been unintentionally altered/regenerated by earlier local e2e test runs against a live model (recorded new conversation content that doesn't match the checked-in mocked-proxy fixtures) and were carried through the branch's history and this session's rebase without being noticed, since git auto-merges YAML content without conflict. None of this content is related to the process/transport option redesign; the only fixtures this PR should touch are the small 'Tool ... does not exist' message truncation that varies by available tool count, which is itself unrelated pre-existing test-environment noise, not something introduced by option relocation. Also removed two orphan snapshot files with no corresponding test in the current tree (should_use_outofprocess_connection_workingdirectory.yaml and should_report_failure_or_implemented_error_for_missing_mcp_sampling.yaml). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
should_use_outofprocess_connection_workingdirectory.yaml backs the .NET ClientOptionsE2ETests test that was renamed from Should_Use_Client_Cwd_For_Default_WorkingDirectory to Should_Use_OutOfProcess_Connection_WorkingDirectory as part of this PR (the .NET E2E harness resolves snapshot fixtures by test method name). This file was incorrectly removed as an apparent orphan in the prior snapshot-cleanup commit; it is in fact required and its content was already correct. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SDK Consistency Review — PR #2523/#2536I reviewed the authoritative PR diff ( SummaryThis PR performs a coordinated breaking change across all six SDKs (Node.js, Python, Go, .NET, Java, Rust): client-wide process-scoped launch settings ( Cross-SDK consistency findings✅ Consistent across all languages — I verified the following pattern is applied uniformly:
Naming follows each language's idioms correctly (camelCase for TS/Java, snake_case for Python/Rust, PascalCase for Go/.NET public API). Bonus fix called out in the CHANGELOG and confirmed in the diff: Rust's Documentation ( ConclusionNo cross-SDK inconsistencies found. This PR maintains full feature parity and consistent API design across all six SDK implementations, with idiomatic naming per language and synchronized documentation/tests. No inline review comments needed.
|
1 participant
Summary
Closes #2523.
This PR implements the v2 process/transport configuration boundary across SDKs by moving SDK-managed process launch settings off shared client options and onto the out-of-process runtime connection/transport APIs that actually spawn a runtime process.
Current-main findings
Before editing, I refreshed and inspected the latest
origin/mainand the issue context from #2523, parent #2522, superseded tracker #1934, and linked historical issues/PRs. Currentmainalready contains the compatible in-process groundwork from #1930/#1976 for many first-class options, but the client-wide process launch APIs remained present and could still be configured for transports where they are inapplicable. This PR carries forward the remaining maintainer-intended v2 cleanup from #1934/#2523 rather than treating it as optional design exploration.Breaking changes and migration
CopilotClientOptions.WorkingDirectoryandCopilotClientOptions.Environment; moved them to stdio/TCP out-of-process runtime connections. RenamedChildProcessRuntimeConnectiontoOutOfProcessRuntimeConnection.ClientOptions.WorkingDirectoryandClientOptions.Env; moved them toStdioConnectionandTCPConnection. The unexported child-process helper is renamed to out-of-process terminology.CopilotClientOptions.cwd/setCwdandCopilotClientOptions.environment/setEnvironment; moved them toStdioRuntimeConnectionandTcpRuntimeConnectionasworkingDirectoryandenvironment. Java has no shared child-process base class, so this intentionally uses the two concrete out-of-process connection types.workingDirectoryandenv; moved them to the renamedOutOfProcessRuntimeConnectionbase used by stdio/TCP connections. RenamedChildProcessRuntimeConnectionaccordingly.working_directoryandenv; moved them to the renamedOutOfProcessRuntimeConnectionbase used by stdio/TCP connections. RenamedChildProcessRuntimeConnectionaccordingly.program, prefix/raw args,extra_args,working_directory,env, andenv_removefromClientOptionstoOutOfProcessOptionscarried by the process-spawning transport variants.Transport::Externaldoes not expose process launch options because the SDK does not own that process.The changelog and SDK READMEs include before/after examples for the migration path.
Runtime follow-up
The SDK-side API cleanup is implemented here. Runtime-side per-client host environment consumption remains tracked separately in #2533, the consolidated follow-up for runtime work required by #2523. This PR does not embed or acquire runtime artifacts and does not take lifecycle/SQLite work from #2524/#2525.
Validation
origin/mainbefore final rebase.getCwd,setCwd,.cwd,CopilotClientOptions.environment,getEnvironment,setEnvironment) after the migration../mvnw test-compile jar:jar,./mvnw -pl sdk verify -Dskip.test.harness=true, and./mvnw -pl sdk spotless:checkfor the environment-only phase. The follow-up cwd/full cross-SDK validation is intentionally left to CI because local E2E/Maven runs were prohibitively expensive in this sandbox.