Replace StreamJsonRpc with a custom JSON-RPC implementation in the .NET SDK#1170
Open
stephentoub wants to merge 8 commits intomainfrom
Open
Replace StreamJsonRpc with a custom JSON-RPC implementation in the .NET SDK#1170stephentoub wants to merge 8 commits intomainfrom
stephentoub wants to merge 8 commits intomainfrom
Conversation
…ET SDK StreamJsonRpc was the only JSON-RPC client the .NET SDK used, but it dragged in 10 transitive runtime dependencies including Newtonsoft.Json, MessagePack, Nerdbank.Streams, and Microsoft.VisualStudio.Threading - none of which the SDK actually exercised. It also made every wire interaction go through a serialization stack we did not control, complicating AOT/trim work. This PR drops StreamJsonRpc and replaces it with a focused, internal JsonRpc class (~720 lines) that implements only what the SDK uses to talk to the Copilot CLI: LSP-style header-delimited framing (Content-Length: N\r\n\r\n + body), JSON-RPC 2.0 requests/responses/notifications, and reflection-based dispatch for the small set of methods the SDK registers. The wire format is the same one the Go and Python SDKs implement against the same CLI. Notable points: - Public API surface is unchanged for SDK consumers. - No StreamJsonRpc types leak through the public surface (they were already PrivateAssets=compile). - Codegen for the generated RPC handlers (scripts/codegen/csharp.ts) was updated to emit calls against the new JsonRpc class. - Cuts deployment footprint by ~3.24 MB / 10 assemblies in a published net8.0 app (10.14 MB -> 6.90 MB; Newtonsoft.Json, MessagePack, Nerdbank.MessagePack, Nerdbank.Streams, MessagePack.Annotations, Microsoft.VisualStudio.Threading, Microsoft.VisualStudio.Validation, PolyType, Microsoft.NET.StringTools all gone). - All existing unit tests pass; the read loop and framing parser were reviewed for correctness across header/body edge cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
SerializeArgs was calling GetTypeInfo(typeof(object?[])), which is not provided by any of the source-generated JsonSerializerContexts and therefore threw NotSupportedException at runtime on macOS/Ubuntu (Windows happened to skip these code paths in earlier failures). Build the params JSON array manually, looking up TypeInfo by each argument's runtime type, which is what the merged source-gen resolver actually contains. Also insert the missing space before ':' in the PendingRequest primary-constructor base list to satisfy 'dotnet format --verify-no-changes'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the .NET SDK’s StreamJsonRpc dependency with a new internal, purpose-built JSON-RPC implementation that speaks the Copilot CLI’s LSP-style Content-Length framing, and updates the C# codegen + generated RPC layer accordingly.
Changes:
- Introduces a new internal
JsonRpcimplementation and wiresCopilotClientto use it instead ofStreamJsonRpc. - Updates C# code generation and the generated RPC file to register handlers via
SetLocalRpcMethod(...)and useValueTaskfor incoming handlers. - Removes
StreamJsonRpcpackage references and related tests/ignores; adjusts serialization tests and.gitignoreplacement for.vs/.
Show a summary per file
| File | Description |
|---|---|
| scripts/codegen/csharp.ts | Updates codegen to emit SetLocalRpcMethod(...), ValueTask handler delegates, and makes registration type internal. |
| dotnet/src/JsonRpc.cs | Adds the new custom JSON-RPC transport (framing, dispatch, invoke, cancellation, errors). |
| dotnet/src/Client.cs | Switches connection setup and handler registration from StreamJsonRpc to the new JsonRpc. |
| dotnet/src/Generated/Rpc.cs | Updates generated registration to SetLocalRpcMethod(...), removes StreamJsonRpc import, and changes registration visibility. |
| dotnet/src/GitHub.Copilot.SDK.csproj | Removes StreamJsonRpc package reference from the SDK. |
| dotnet/src/Session.cs | Removes unused StreamJsonRpc import after transport swap. |
| dotnet/test/SerializationTests.cs | Drops StreamJsonRpc-specific RequestId round-trip tests and reframes scope to SDK serializer options. |
| dotnet/test/GitHub.Copilot.SDK.Test.csproj | Removes StreamJsonRpc test dependency. |
| dotnet/Directory.Packages.props | Removes StreamJsonRpc package version pin. |
| dotnet/.gitignore | Stops ignoring .vs/ locally (moved to repo root). |
| .gitignore | Adds .vs/ ignore at repo root. |
Copilot's findings
- Files reviewed: 9/11 changed files
- Comments generated: 6
The Copilot CLI uses vscode-jsonrpc-style request handlers which expect `params` to be the request object directly. The other SDKs (Node/Python/Go) all send single-object params, but the .NET InvokeAsync was wrapping the single arg in a positional JSON array, so the CLI couldn't deserialize it and never responded — every round-trip RPC test timed out. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
vscode-jsonrpc (used by the Copilot CLI) rejects responses that lack
both `result` and `error` with 'The received response has neither a
result nor an error property'. Our `JsonRpcResponse` inherited the
context-level `DefaultIgnoreCondition = WhenWritingNull`, so void/
nullable-returning handlers (e.g. session.plan.update, every sessionFs
handler that returns SessionFsError?) emitted `{jsonrpc, id}` with no
`result` field, hanging the CLI and timing out tests.
Override the policy on the Result property with [JsonInclude] +
[JsonIgnore(Condition = Never)] so we always serialize `result: null`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
If AssistantMessageEvent and SessionIdleEvent both arrived between SendAsync returning and the subscription being installed, AND GetMessagesAsync returned only the assistant message before SessionIdleEvent had arrived, the helper would hang. The subscription would later receive SessionIdleEvent but skip it because the local finalAssistantMessage variable was still null (the AssistantMessageEvent had been delivered before subscription). Now CheckExistingMessages backfills finalAssistantMessage from already-delivered events under a lock, so the SessionIdleEvent handler can complete the wait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅This PR makes only internal .NET SDK changes — all 12 modified files are 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.
Why
StreamJsonRpcwas the only JSON-RPC client the .NET SDK used to talk to the Copilot CLI, but it dragged in 10 transitive runtime dependencies (Newtonsoft.Json, MessagePack, Nerdbank.MessagePack, Nerdbank.Streams, Microsoft.VisualStudio.Threading, etc.) that the SDK never exercised. It also routed every wire interaction through a serialization stack we did not control.What
This PR drops
StreamJsonRpcand replaces it with a focused internalJsonRpcclass (~720 lines) that implements only what the SDK actually uses against the CLI:Content-Length: N\r\n\r\n+ body) — the same wire format the Go and Python SDKs already implement against the same CLI.singleObjectParam: true(wholeparamsobject deserialized as the first arg) and positional (JSON array). Anything else throws-32603.JsonTypeInfoeverywhere; noUnconditionalSuppressMessageshims required.The codegen script (
scripts/codegen/csharp.ts) was updated to emit registration calls against the newJsonRpcclass.Notable points
StreamJsonRpcwas alreadyPrivateAssets="compile", so nothing leaks out either way.net8.0app (10.14 MB -> 6.90 MB). Newtonsoft.Json, MessagePack(+Annotations), Nerdbank.MessagePack, Nerdbank.Streams, Microsoft.VisualStudio.Threading, Microsoft.VisualStudio.Validation, PolyType, and Microsoft.NET.StringTools all disappear from the dependency closure.