Log handler dispatch failures in the SDKs - #2425
Conversation
CopilotSession swallowed several handler failures silently. Most notably, ExecuteToolAndRespondAsync caught every exception, reported the message back over RPC, and logged nothing -- so a host whose tool never ran saw only an external_tool.requested followed by an external_tool.completed milliseconds later, with no way to tell whether the failure came from argument binding, result conversion, or the handler itself. Add structured LoggerMessage diagnostics for the tool, permission, command, elicitation, and MCP OAuth dispatch paths, covering both the handler failure and the follow-up failure to deliver the error back to the runtime. Also warn when a tool or command request arrives for a name this client has no handler for, including the registered names, guarded by an IsEnabled check. Behavior is unchanged: errors are still reported back via the same RPCs. Tool arguments and results are never logged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds structured .NET SDK diagnostics for handler dispatch and error-delivery failures.
Changes:
- Logs tool, permission, command, elicitation, and MCP OAuth failures.
- Forwards and captures SDK logs in the E2E harness.
- Verifies tool exceptions remain host-diagnosable without leaking to the model.
Show a summary per file
| File | Description |
|---|---|
dotnet/src/Session.cs |
Adds structured failure diagnostics. |
dotnet/test/Harness/E2ETestContext.cs |
Dynamically forwards logs to the active test. |
dotnet/test/Harness/E2ETestBase.cs |
Captures warning-and-above log entries. |
dotnet/test/E2E/ToolsE2ETests.cs |
Verifies tool failure logging. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
dotnet/src/Session.cs:2038
LogCommandFailedis also called when the command handler completed successfully but the normalHandlePendingCommandAsyncresponse failed, because both operations are inside the same try at lines 1283–1307. Labeling that as a handler failure sends operators to the wrong component; split the catches or make the message describe both possible stages.
Message = "Command handler failed. SessionId={SessionId}, RequestId={RequestId}, Command={CommandName}")]
dotnet/src/Session.cs:2053
- The enclosing catch also handles serialization and normal response-delivery failures after a successful elicitation handler, so this log can incorrectly blame the host handler. Since diagnostics are the purpose of this change, distinguish those stages or use wording that accurately covers both before attempting cancellation.
Message = "Elicitation handler failed; cancelling the pending elicitation. SessionId={SessionId}, RequestId={RequestId}")]
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
The outer catch in ExecuteToolAndRespondAsync also covers the success-path HandlePendingToolCallAsync, so 'failed before a result could be produced' was wrong whenever the handler succeeded and delivery failed -- reporting a transport failure as if the handler never produced anything. Track the stage the call reached and log it, which is both accurate for every path and more useful than a message that merely covers both: it separates argument binding (handler never ran) from a handler that threw, from a result that could not be converted or delivered. Apply the same correction to the command and elicitation messages, matching the 'handler or response delivery failed' wording the permission path already used. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Thanks - the consistency agent ran twice here (once per push), so this covers both passes. I checked the sibling SDKs rather than taking the summaries at face value, and the reported gaps are real:
I am deliberately keeping this PR .NET-only. These gaps all pre-date it, none of them are regressions introduced here, and porting the change to four more SDKs would mean re-running cross-language E2E snapshots and would invalidate the review and check runs already completed on this diff. Parity is worth doing as separate per-SDK work where each can be validated on its own. Java is the cheapest follow-up since it already logs the invocation failure and only needs the no-handler warning at 854-857. Python is the closest match to the partner scenario that motivated this PR, since it mirrors the pre-change .NET behavior exactly: the handler exception goes back over RPC and is never surfaced locally. Separately, the two suppressed review comments on |
stephentoub
changed the title
Replace the private ToolCallStage enum in the .NET SDK with plain string literals. The rendered log output is identical, since the enum was only ever formatted via ToString(), and strings let the same stage values be shared verbatim across every SDK. Port the handler dispatch diagnostics to the Python, Go, Node.js, Java, and Rust SDKs so a handler that fails to bind or throws is diagnosable from the host in every language rather than only in C#. Each SDK follows its own existing logging convention: module logger with lazy %s formatting in Python, level-gated log.Printf in Go, console.warn/error in Node.js, java.util.logging with lazy suppliers in Java, and tracing with structured fields in Rust. Behavior is unchanged. Errors are still reported back over the same RPC calls, and only tool names and request/session identifiers are logged, never argument payloads or results. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅This PR adds structured diagnostics for handler dispatch failures across all six SDKs (.NET, Python, Go, Node.js, Java, Rust). After reviewing the changes in each implementation, the feature is consistently implemented across all languages. Coverage matrix
Language-convention differences (intentional, not gaps)
No cross-SDK consistency issues found. The implementation is thorough and well-aligned across all six languages.
|
When a tool, permission, command, elicitation, or MCP OAuth handler failed to bind or threw, the SDKs reported the error back over JSON-RPC but logged nothing locally, so a host saw an
external_tool.requestedevent followed milliseconds later byexternal_tool.completedwith no way to see its own exception. Similarly, a tool request arriving for a name with no registered handler was dropped silently.This adds structured diagnostics on those paths in all six SDKs (.NET, Python, Go, Node.js, Java, and Rust), including the stage a tool call reached (
PreparingArguments,InvokingHandler,ConvertingResult,SendingResult) so a binding failure is distinguishable from a failure inside the handler body. Each SDK follows its own existing logging convention and level-gating, so default output is unchanged.Behavior is otherwise unchanged: errors still go back over the same RPC calls. Only tool names and request/session identifiers are logged, never argument payloads or results.