Version-independent SDK test and codegen fixes split from the 1.0.76-0 bump - #2110
Merged
Conversation
pytest-timeout has been a declared dev dependency for a while but was never configured, and the Python job sets no timeout-minutes either. When CLI 1.0.76-0 introduced a session.destroy deadlock, five of the six Python jobs sat in_progress for hours instead of failing, producing no diagnostic signal at all and holding runners until GitHub's 6-hour job limit. Setting timeout = 300 makes a deadlocked test fail in five minutes with a full stack dump pointing at the blocked call, which is how the other SDKs already behave (Node uses 30s/60s/180s per test, Go bounds the package at 20m). The value is deliberately generous: the whole Python suite completes in about 10 minutes, so no individual test comes close to 300s. Verified locally that pytest honours the setting from pyproject.toml and that it interrupts a hanging test with a traceback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e615d062-bcb7-431e-aa9c-d3e47405723a (cherry picked from commit c33d8e2)
The "should kill shell process" e2e test exists in all five SDKs, but only Node, Python and Rust destroy the session at the end of it. Go tears the whole client down with client.ForceStop() and .NET simply ended at the last assert, so neither ever exercised session.destroy after a session.shell.exec. That gap is why Go and .NET stayed green on CLI 1.0.76-0 while Node, Python and Rust hang: the hang is in session.destroy after a shell exec, and the two green SDKs never made the call. Adding the teardown makes all five cover the same sequence. Verified locally against the same CLI binary (1.0.76-0, sha256 b8bfad2c...): with the added Disconnect(), the Go test hangs past a 3-minute timeout where it previously passed in seconds. The same test passes on CLI 1.0.73. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e615d062-bcb7-431e-aa9c-d3e47405723a (cherry picked from commit 2e54b47)
The runtime team confirmed this method is marked `visibility: internal` in the shared API contract: its `client` and `transport` fields are live in-process MCP SDK instances that cannot be serialized over JSON-RPC. No SDK exposes it as a typed method, and the Rust test drove it through a raw `call_session_rpc` with placeholder JSON objects that could never satisfy that contract. It passed on CLI 1.0.73 only because the old TypeScript dispatch happened to route internal methods generically. 1.0.76-0 routes the method through the Rust native registry, where the host-effect switch has no case for it, so it now returns -32603 "Unsupported native session host effect". That is correct behaviour, not a regression, so the test is removed along with its now-orphaned snapshot and a note recording why the gap is deliberate. Both `call_session_rpc` and `is_mcp_server_running` remain in use by other tests in the file, so no helper became dead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e615d062-bcb7-431e-aa9c-d3e47405723a (cherry picked from commit f0c846c)
The Rust codegen only emitted a named type for result schemas that were an
enum, array, map, or object. A method whose result is an inline primitive
(e.g. {"type":"integer"}) produced a reference to a type that was never
defined, so the generated crate failed to compile with E0425.
No RPC method had a primitive result until session.cancelAllBackgroundAgents
was added, so this latent gap only surfaces on a schema update. Add
rustScalarType()/emitRustScalarAlias() so scalar results emit a plain type
alias alongside the existing shapes.
This is a no-op against the current pinned schema and prevents the next
dependency bump from hitting the same build break.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e615d062-bcb7-431e-aa9c-d3e47405723a
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
Separates version-independent codegen and test reliability fixes from the dependency bump.
Changes:
- Adds Rust scalar RPC result aliases.
- Updates E2E teardown and removes unsupported Rust MCP coverage.
- Adds a five-minute Python test timeout.
Show a summary per file
| File | Description |
|---|---|
scripts/codegen/rust.ts |
Generates Rust aliases for scalar schemas. |
rust/tests/e2e/rpc_mcp_lifecycle.rs |
Removes unsupported raw-RPC coverage. |
test/snapshots/rpc_mcp_lifecycle/should_register_and_unregister_external_mcp_client.yaml |
Deletes the obsolete snapshot. |
python/pyproject.toml |
Configures the per-test timeout. |
go/internal/e2e/rpc_shell_and_fleet_e2e_test.go |
Adds shell-session teardown. |
dotnet/test/E2E/RpcShellAndFleetE2ETests.cs |
Adds shell-session disposal. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Medium
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b565403c-57ed-4368-84f1-f04bfe39e46e
Contributor
There was a problem hiding this comment.
Review details
Resolved since last review (2)
- ✅ dotnet/test/E2E/RpcShellAndFleetE2ETests.cs#L45 — Resolved by @stephentoub - This disposal is skipped if
ExecAsync,KillAsync, or either assertion throws, leaving the… - ✅ go/internal/e2e/rpc_shell_and_fleet_e2e_test.go#L87 — Resolved by @stephentoub - This disconnect is skipped by every preceding
t.Fatal, so a failing exec/kill assertion still…
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Medium
Contributor
Cross-SDK Consistency Review ✅This PR makes targeted, consistent fixes across multiple SDKs. Here's a summary of the cross-SDK analysis: Session teardown in
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
session.mcp.registerExternalCliente2e coverageThese version-independent fixes were split from #2100 so they can land separately from the automated
@github/copilot1.0.76-0 dependency bump.Validation
cargo check --tests --features test-supportGenerated by Copilot