Hide StreamJsonRpc implementation detail behind IOException #202
Conversation
Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
dotnet/src/Session.cs
Outdated
| catch (StreamJsonRpc.RemoteInvocationException ex) | ||
| { | ||
| throw new CopilotRpcException($"Failed to get messages: {ex.Message}", ex); | ||
| } |
There was a problem hiding this comment.
@copilot Don't wrap more lines in this try/catch than you have to. It's enough only to wrap the InvokeWithCancellationAsync call.
There was a problem hiding this comment.
Fixed in 81d8dec. Now only the InvokeWithCancellationAsync calls are wrapped in try-catch blocks, with processing moved outside.
dotnet/src/Types.cs
Outdated
| /// <remarks> | ||
| /// This exception wraps underlying communication errors to provide a stable API surface | ||
| /// that doesn't expose implementation details of the JSON-RPC protocol. | ||
| /// </remarks> |
There was a problem hiding this comment.
Make these XML docs less specific about implementation details. No need to mention JSON-RPC specifically. Don't need to say why the exception is here (i.e. the stable API reason).
There was a problem hiding this comment.
Simplified in f46b3d5. Removed JSON-RPC mentions and the remarks section explaining the API design rationale.
|
@stephentoub This looks reasonable to me. Is it the sort of thing you had in mind? |
Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
dotnet/src/Session.cs
Outdated
| catch (StreamJsonRpc.RemoteInvocationException ex) | ||
| { | ||
| throw new CopilotRpcException($"Failed to destroy session: {ex.Message}", ex); | ||
| } |
There was a problem hiding this comment.
Would it be cleaner / more maintainable to have a custom InvokeWithCancellationAsync wrapper that does the catch/throw and then all the call sites would just use that wrapper?
There was a problem hiding this comment.
@copilot Please do this. Also we can just use IOException - no need for a custom CopilotRpcException type.
If the ability to differentiate these exceptions by type is important (or if you want to expose additional strongly-typed data on the exception), yes. Otherwise, it could just use the base IOException and not have a custom exception type. |
Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
|
@copilot Address this feedback: It would be cleaner / more maintainable to have a custom InvokeWithCancellationAsync wrapper that does the catch/throw and then all the call sites would just use that wrapper do this. Also we can just use IOException - no need for a custom CopilotRpcException type. |
Cross-SDK Consistency ReviewI've analyzed error handling patterns across all four SDK implementations and found partial consistency issues with this PR. Current State✅ Good: Custom Exception Type AlignmentAll SDKs except Node.js now use custom exception types for RPC errors:
This PR brings .NET into alignment with Python and Go.
|
Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
Implemented in 63b974a. Created |
SDK Consistency Review: Error Handling PatternsI've reviewed this PR for cross-SDK consistency. The changes hide the Consistency AnalysisCurrent Error Handling Across SDKs:
Observations✅ Good: This PR aligns the .NET SDK with Node.js by hiding JSON-RPC implementation details and using standard library exception types.
RecommendationThis is not a blocking issue - the divergence reflects different language idioms:
However, for future consideration: The team may want to document this intentional design divergence in the SDK design guidelines, or consider whether all SDKs should converge on one approach:
No action required for this PR - the changes maintain .NET SDK quality and follow reasonable C# conventions. ✅
|
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #201 by hiding the StreamJsonRpc implementation detail from the public API surface. Previously, StreamJsonRpc.RemoteInvocationException was leaking through to API consumers. The PR wraps all RPC communication errors with standard IOException to maintain abstraction boundaries.
Changes:
- Created wrapper methods
InvokeRpcAsyncin bothSession.csandClient.csto centralize exception handling - Updated all RPC invocation call sites to use the wrapper methods
- Updated test assertions and documentation examples to expect
IOExceptioninstead ofStreamJsonRpc.RemoteInvocationException
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/test/SessionTests.cs | Updated test assertions to expect IOException instead of RemoteInvocationException |
| dotnet/src/Types.cs | Added System.IO using statement (unnecessary - handled by implicit usings) |
| dotnet/src/Session.cs | Added InvokeRpcAsync wrapper method and updated all RPC calls to use it |
| dotnet/src/Client.cs | Added static InvokeRpcAsync wrapper method and updated all RPC calls to use it |
| dotnet/README.md | Updated error handling documentation example to show IOException |
Comments suppressed due to low confidence (1)
dotnet/src/Session.cs:101
- The XML documentation should include an exception tag for IOException to inform API consumers that this method can throw IOException when communication errors occur with the Copilot CLI. Add:
/// <exception cref="IOException">Thrown when communication with the Copilot CLI fails.</exception>
/// <summary>
/// Sends a message to the Copilot session and waits for the response.
/// </summary>
/// <param name="options">Options for the message to be sent, including the prompt and optional attachments.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> that can be used to cancel the operation.</param>
/// <returns>A task that resolves with the ID of the response message, which can be used to correlate events.</returns>
/// <exception cref="InvalidOperationException">Thrown if the session has been disposed.</exception>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dotnet/src/Session.cs
Outdated
| { | ||
| return await _rpc.InvokeWithCancellationAsync<T>(method, args, cancellationToken); | ||
| } | ||
| catch (StreamJsonRpc.RemoteInvocationException ex) |
There was a problem hiding this comment.
Consider catching a broader set of StreamJsonRpc exceptions beyond just RemoteInvocationException. Other exceptions like ConnectionLostException, RemoteMethodNotFoundException, or the base RemoteRpcException could also be thrown during RPC operations and would currently leak through the abstraction. Consider catching the base RemoteRpcException type or handling these additional exception types to fully hide the StreamJsonRpc implementation details.
| catch (StreamJsonRpc.RemoteInvocationException ex) | |
| catch (StreamJsonRpc.RemoteRpcException ex) |
dotnet/src/Client.cs
Outdated
| { | ||
| return await rpc.InvokeWithCancellationAsync<T>(method, args, cancellationToken); | ||
| } | ||
| catch (StreamJsonRpc.RemoteInvocationException ex) |
There was a problem hiding this comment.
Consider catching a broader set of StreamJsonRpc exceptions beyond just RemoteInvocationException. Other exceptions like ConnectionLostException, RemoteMethodNotFoundException, or the base RemoteRpcException could also be thrown during RPC operations and would currently leak through the abstraction. Consider catching the base RemoteRpcException type or handling these additional exception types to fully hide the StreamJsonRpc implementation details.
| catch (StreamJsonRpc.RemoteInvocationException ex) | |
| catch (StreamJsonRpc.RemoteRpcException ex) |
dotnet/src/Types.cs
Outdated
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| using System.IO; |
There was a problem hiding this comment.
The using System.IO; directive is unnecessary in this file since IOException is not used anywhere in Types.cs. This import should be removed. Note that IOException is used in Session.cs and Client.cs, but those files have implicit usings enabled via the project's ImplicitUsings setting, making System.IO automatically available.
| using System.IO; |
dotnet/src/Session.cs
Outdated
| WorkspacePath = workspacePath; | ||
| } | ||
|
|
||
| private async Task<T> InvokeRpcAsync<T>(string method, object?[]? args, CancellationToken cancellationToken) |
There was a problem hiding this comment.
nit: this method is identical to Client.InvokeRpcAsync, except that Client.InvokeRpcAsync accepts a JsonRpc parameter and this method bakes in the _rpc reference. If Client.InvokeRpcAsync were internal instead, this method could just be:
private Task<T> InvokeRpcAsync<T>(string method, object?[]? args, CancellationToken cancellationToken) =>
Client.InvokeRpcAsync(_rpc, method, args, cancellationToken);and avoid the duplication.
|
@copilot Address the following review comments only:
nit: this method is identical to Client.InvokeRpcAsync, except that Client.InvokeRpcAsync accepts a JsonRpc parameter and this method bakes in the _rpc reference. If Client.InvokeRpcAsync were internal instead, this method could just be: and avoid the duplication. |
…sed using Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
Addressed in bad23c1:
|
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, .NET). Here are my findings: SummaryThis PR introduces a deliberate divergence in error handling philosophy between the .NET SDK and the other three SDKs. While not a bug, teams should be aware of this difference when documenting and supporting the SDKs. Error Handling Comparison
Key Differences
Consistency Considerations✅ This PR is acceptable because:
|
* Initial plan * Wrap StreamJsonRpc exceptions with CopilotRpcException Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com> * Add explicit using System.IO for IOException Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com> * Only wrap RPC invocation calls in try-catch blocks Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com> * Remove implementation details from CopilotRpcException docs Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com> * Replace CopilotRpcException with IOException and add RPC wrapper methods Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com> * Catch RemoteRpcException, refactor Session.InvokeRpcAsync, remove unused using Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
This PR ensures that
StreamJsonRpc.RemoteInvocationExceptionis not exposed through the public API by wrapping all RPC communication errors with standardIOException.Changes Made
InvokeRpcAsynchelper methods in bothSession.csandClient.csto centralize exception handlingStreamJsonRpc.RemoteRpcExceptionand throwIOExceptioninsteadStreamJsonRpc.RemoteInvocationExceptiontoIOExceptionIOExceptionusing System.IO;from Types.csImplementation Details
The wrapper methods provide a single point of exception translation:
Client.cs: Internal static method that accepts aJsonRpcparameter and catchesRemoteRpcExceptionSession.cs: Instance method that delegates toClient.InvokeRpcAsyncwith the session's_rpcfieldThis approach eliminates ~115 lines of repetitive try-catch blocks while ensuring StreamJsonRpc remains an implementation detail that doesn't leak through the public API surface.
Testing
IOExceptionOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.