Launch managed SDK servers through the Rust runtime wrapper - #2395
Conversation
|
The hostless wrapper direction looks right, but this is not complete yet because I manually tested all 48 combinations: 6 SDKs × stdio/TCP/in-process/existing URL × source/published consumption, using The missing piece is preserving the hostless runtime assets relative to Node-only and delivery-only content can be excluded: I validated this approach on .NET in both development and after Before merging, I think this PR needs to:
|
Document PR #2395 and the Rust-only out-of-process transition on Node extension-authored factory coverage, the real-host extension environment test, and .NET extension lifecycle tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
805e7cb to
455f17b
Compare
There was a problem hiding this comment.
Pull request overview
Routes managed subprocess SDK connections through the coherent copilot-runtime/runtime.node distribution while preserving explicit and in-process launch modes.
Changes:
- Updates all six SDK launch paths and packaging.
- Retains auxiliary runtime assets with cache and integrity handling.
- Expands tests and documentation for runtime resolution.
Show a summary per file
| File | Description |
|---|---|
rust/tests/e2e/support.rs |
Preserves explicit CLI environment paths. |
rust/tests/cli_resolution_test.rs |
Tests wrapper resolution and extraction. |
rust/src/startup_timings.rs |
Updates resolution timing documentation. |
rust/src/resolve.rs |
Resolves and validates runtime pairs. |
rust/src/lib.rs |
Exposes bundled runtime installation. |
rust/src/ffi.rs |
Supports adjacent runtime libraries and musl. |
rust/src/embeddedcli.rs |
Extracts coherent runtime bundles. |
rust/README.md |
Documents bundled runtime behavior. |
rust/build/in_process.rs |
Builds and caches runtime distributions. |
rust/build.rs |
Unifies build implementation. |
python/test_client.py |
Tests explicit launch overrides. |
python/test_cli_download.py |
Tests runtime bundle provisioning. |
python/README.md |
Documents runtime downloads and paths. |
python/copilot/client.py |
Selects the managed wrapper. |
python/copilot/_cli_download.py |
Downloads and stages runtime assets. |
nodejs/test/runtimeArtifacts.test.ts |
Tests runtime materialization. |
nodejs/test/e2e/factory.e2e.test.ts |
Disables factory E2E coverage. |
nodejs/test/e2e/extension_env_access.e2e.test.ts |
Disables extension-host coverage. |
nodejs/test/e2e/builtin_tools.e2e.test.ts |
Adds grep runtime validation. |
nodejs/test/client.test.ts |
Tests explicit path precedence. |
nodejs/src/runtimeArtifacts.ts |
Materializes platform runtime assets. |
nodejs/src/client.ts |
Launches the bundled runtime wrapper. |
nodejs/README.md |
Documents managed wrapper launches. |
java/sdk/src/test/java/com/github/copilot/ffi/NativeRuntimeLoaderTest.java |
Tests classifier runtime extraction. |
java/sdk/src/test/java/com/github/copilot/CliServerManagerTest.java |
Tests explicit CLI paths. |
java/sdk/src/main/java/com/github/copilot/ffi/NativeRuntimeLoader.java |
Extracts wrapper and retained assets. |
java/sdk/src/main/java/com/github/copilot/CliServerManager.java |
Launches the resolved wrapper. |
java/README.md |
Documents Java runtime provisioning. |
java/copilot-native/scripts/fetch-native.test.mjs |
Tests native asset staging. |
java/copilot-native/scripts/fetch-native.mjs |
Builds classifier runtime trees. |
java/copilot-native/pom.xml |
Verifies packaged wrappers. |
go/README.md |
Documents embedded runtime resolution. |
go/internal/ffihost/resolve.go |
Supports adjacent runtime.node. |
go/internal/embeddedcli/embeddedcli.go |
Installs wrapper pairs and assets. |
go/internal/embeddedcli/embeddedcli_test.go |
Tests runtime installation. |
go/cmd/bundler/main.go |
Bundles coherent runtime artifacts. |
go/cmd/bundler/main_test.go |
Tests filtering and generated embeds. |
go/client.go |
Selects the managed runtime executable. |
go/client_test.go |
Tests runtime selection failures. |
dotnet/test/Unit/RuntimeWrapperTests.cs |
Tests wrapper resolution behavior. |
dotnet/test/Unit/MSBuildTargetsTests.cs |
Tests runtime asset copying. |
dotnet/test/E2E/RpcExtensionsLoadedE2ETests.cs |
Disables extension lifecycle tests. |
dotnet/test/E2E/BuiltinToolsE2ETests.cs |
Re-enables ripgrep tool tests. |
dotnet/src/Client.cs |
Resolves and validates runtime pairs. |
dotnet/src/build/GitHub.Copilot.SDK.targets |
Packages filtered runtime assets. |
dotnet/README.md |
Documents managed runtime launches. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
rust/build/in_process.rs:136
- Removing the shared install directory is not coordinated across build processes. Two concurrent Cargo builds can both observe an incomplete cache, then one can delete the directory while the other is publishing its file-level staging entries, causing a panic or a mixed/incomplete cache. Serialize refreshes with a cross-process lock, or stage a complete unique directory and publish it atomically without deleting another writer's live directory.
- Files reviewed: 46/46 changed files
- Comments generated: 9
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
|
Here's some output from a review agent. Sorry the formatting is a bit weird after the paste. Hopefully your agent can make sense of it even though the tables are misaligned. Manual validation. I rebuilt minimal consumers and reran all 48 combinations: six languages × stdio/TCP/in-process/existing URL × source/published. I required successful grep tool events and physically removed the SEA where possible. 23/48 satisfy both gates.
Minimal reproduction of the common defect: File.Delete("runtimes/linux-x64/native/copilot");
await StartAsync(RuntimeConnection.ForStdio()); // passes
await StartAsync(RuntimeConnection.ForInProcess()); // currently failsAdditional blockers. The PR disables real extension and factory E2Es because the new default loses the Node extension lifecycle (Node, .NET). That requires an explicit product decision or implementation, not skipped coverage. Also rather than disabling the e2e tests for extensions/factories, could we leave them there but have the test supply an external Node binary since we now support configuring that? |
This comment has been minimized.
This comment has been minimized.
|
Addressed in e3f7726. The extension/factory E2Es remain enabled and explicitly opt into an external legacy |
Document PR #2395 and the Rust-only out-of-process transition on Node extension-authored factory coverage, the real-host extension environment test, and .NET extension lifecycle tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
e3f7726 to
05b331b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for #2395 · sonnet46 136.3 AIC · ⌖ 5.83 AIC · ⊞ 6.6K
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for #2395 · sonnet46 132.2 AIC · ⌖ 5.63 AIC · ⊞ 6.6K
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Document PR #2395 and the Rust-only out-of-process transition on Node extension-authored factory coverage, the real-host extension environment test, and .NET extension lifecycle tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Match the successful grep completion to the grep tool invocation and document Java's bundled runtime-wrapper default. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
441db45 to
f5aeef6
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efc29e35-611b-46d8-9b8b-e1912d2eccf4
This comment has been minimized.
This comment has been minimized.
Add a real-wrapper regression that exercises typed provider dispatch, extension registration, tool invocation, disable/re-enable, environment ownership, and process teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for #2395 · sonnet46 133.5 AIC · ⌖ 8.56 AIC · ⊞ 6.6K
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for #2395 · sonnet46 93 AIC · ⌖ 5.76 AIC · ⊞ 6.6K
Comments that could not be inline-anchored
python/copilot/_ffi_runtime_host.py:136
This PR adds a new "adjacent runtime.node" lookup step to Go's ResolveLibraryPath (step 2 in go/internal/ffihost/resolve.go) to support the out-of-process wrapper layout, but Python's resolve_library_path is missing this equivalent step.
Go's updated lookup order is:
- Natural platform library name (flat/bundled layout)
runtime.nodeadjacent to entrypoint ← new step for the wrapper layoutprebuilds/<platform>/runtime.node(dev/package layout)
Python currently only has ste…
java/sdk/src/main/java/com/github/copilot/ffi/NativeRuntimeLoader.java:337
Similarly, Java's resolveFromCliPath is missing the new "adjacent runtime.node" lookup step that Go added in this PR for the out-of-process wrapper layout.
Go checks (in order): flat library → adjacent runtime.node → prebuilds/<platform>/runtime.node
Java currently only has the flat and prebuilds checks. Suggestion — add the intermediate step:
// Out-of-process wrapper layout: runtime.node adjacent to the wrapper.
Path adjacent = parent.resolve(RUNTIME_FILENAME); // alre…
</details>Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5c3c30-0cf7-4b9a-9a3b-2b01ca073015
Cross-SDK Consistency ReviewThis PR updates all six SDKs (Node.js, Python, Go, .NET, Java, Rust) to launch managed subprocess connections through the new ✅ Consistent across all SDKs
|
| Entry | Node.js (runtimeArtifacts.ts) |
Python (_cli_download.py) |
.NET (.targets) |
|---|---|---|---|
copilot.tgz |
❌ Not excluded | ❌ Not excluded | ✅ Excluded |
copilot.tgz is excluded in the .NET targets but not in the Node.js EXCLUDED_TOP_LEVEL set or the Python _HOSTLESS_EXCLUDED_TOP_LEVEL set. If the platform package ever ships a copilot.tgz at the root, Node.js and Python would stage it unnecessarily into the cache. This is low-severity (the file would be unused dead weight), but worth aligning for correctness.
Suggestion: Add "copilot.tgz" to EXCLUDED_TOP_LEVEL in nodejs/src/runtimeArtifacts.ts and to _HOSTLESS_EXCLUDED_TOP_LEVEL in python/copilot/_cli_download.py.
i️ Intentional .NET-only behavior: .copilot-explicit-cli marker
The .copilot-explicit-cli marker file (written by MSBuild targets when CopilotCliBinaryPath is set explicitly) lets .NET's GetBundledRuntimeLaunch() fall back to the old copilot SEA when a bundle contains only the legacy binary without the runtime pair. The other SDKs error out in this situation instead of falling back. This asymmetry appears intentional — it is a .NET MSBuild distribution detail with no parallel in the other SDK distribution models.
i️ Go: no fallback to legacy copilot SEA in managed launch
Go's resolveRuntimeExecutable returns an error when embeddedcli.RuntimePath() is empty (the bundle predates the runtime pair). It does not fall back to embeddedcli.Path() (the old copilot SEA). This matches the intent stated in the PR — break loudly on outdated bundles rather than silently falling back — and is consistent with Python and Node.js behavior.
Generated by SDK Consistency Review Agent for #2395 · sonnet46 101.9 AIC · ⌖ 8.45 AIC · ⊞ 6.6K · ◷
Managed out-of-process SDK connections currently launch the root Copilot SEA directly. The runtime now provides a dedicated
copilot-runtimeexecutable that loads the adjacentruntime.node, so SDK-managed subprocess connections should use that coherent wrapper/addon pair while preserving explicit external server and in-process modes.What changed
copilot-runtime(copilot-runtime.exeon Windows).runtime.node, retaining the rootcopilotexecutable only for residual and in-process compatibility.cwdserialization issue discovered during end-to-end validation.Validation
Validated all six SDKs against the immutable same-tip runtime release candidate
adce886fd1across supported root SEA, in-process, and hostless wrapper transports. The final focused cache/resume gates and supported full-suite cells passed with exact artifact hash checks and no leaked runtime processes.The auxiliary-asset follow-up adds focused staging, cache-migration, filtering, permission, and path-safety tests in all six SDKs. A real Node hostless-wrapper integration test also confirms built-in grep completes successfully through the staged ripgrep binary.
Known non-runtime exclusions remain documented from validation: hostless Node extension/factory lifecycle behavior (including the corresponding .NET extension tests), the Go invalid-model fixture, the Java literal-YAML fixture and order-dependent socket-helper contamination, and Rust forced-environment unit-test contamination.
Review notes
This is a draft while the runtime platform packages and Java classifier publication complete. The production platform package is treated as the runtime distribution source: the root
copilot[.exe], selectedprebuilds/<platform>/runtime.nodeandcopilot-runtime[.exe], and retained package-relative runtime assets are staged as one coherent version. Mixed or incomplete artifact sets are rejected.