Set FunctionResultContent.Error on failed Copilot tool executions#698
Set FunctionResultContent.Error on failed Copilot tool executions#698PratikDhanave wants to merge 2 commits into
Conversation
f41c6ea to
e2065cc
Compare
There was a problem hiding this comment.
Pull request overview
Updates the Copilot provider’s tool.execution_complete handling so failed tool executions populate FunctionResultContent.Error (the framework’s canonical failure channel) while still keeping Result populated for continuity.
Changes:
- Set
FunctionResultContent.Erroron failedtool.execution_completeevents using SDK error details (message + optional code). - Added tests to assert
Error == nilon success andError != nilwith message/code on failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| provider/copilotprovider/copilot.go | Populate FunctionResultContent.Error for failed tool executions via toolExecutionError. |
| provider/copilotprovider/copilot_test.go | Extend tool execution completion tests to validate Error behavior on success/failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if data.Error != nil && data.Error.Message != "" { | ||
| if data.Error.Code != nil { | ||
| return fmt.Errorf("%s (%s)", data.Error.Message, *data.Error.Code) | ||
| } | ||
| return errors.New(data.Error.Message) | ||
| } | ||
| return errors.New("tool execution failed") |
There was a problem hiding this comment.
Good catch — fixed in 8da0f1b: toolExecutionError now reads Error.Code independently of Message, so a code-only failure yields "tool execution failed (CODE)" instead of dropping the code, and an empty code no longer produces a stray "message ()" suffix.
| if result.Error == nil { | ||
| t.Fatalf("Error = nil, want non-nil failure error") | ||
| } | ||
| if msg := result.Error.Error(); !strings.Contains(msg, "Access denied to resource") || !strings.Contains(msg, "PERMISSION_DENIED") { | ||
| t.Fatalf("Error = %q, want message and code", msg) | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in 8da0f1b: the failure-without-error-details test now asserts result.Error is non-nil and carries the default "tool execution failed" message, locking in the new FunctionResultContent.Error behavior.
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
parity-approved
This comment has been minimized.
This comment has been minimized.
toolExecutionCompleteUpdate only populated Result, leaving Error nil even when data.Success was false. Callers that detect tool failures via FunctionResultContent.Error (e.g. autocall error collection) therefore treated failed Copilot tool calls as successful. Populate Error from data.Error, combining Message with Code when present and falling back to a default when the SDK omits error details, while keeping Result populated for display continuity. This aligns the Copilot provider with the framework's cross-provider failure-channel convention.
8da0f1b to
0fbd5b1
Compare
Parity ReviewScope: Exported API change: None. Behavioral change: Failed Copilot tool executions now populate Parity verdict: This change increases cross-SDK consistency. No upstream divergence found. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
When the Copilot provider converts a
tool.execution_completeevent withsuccess == false,toolExecutionCompleteUpdatenow setsFunctionResultContent.Errorin addition toResult. The error is built from the SDKToolExecutionCompleteError: the human-readableMessage, combined with the machine-readableCodewhen present ("<message> (<code>)"), falling back to a default when the SDK omits error details.Resultis still populated for display continuity.Why
The framework treats
FunctionResultContent.Erroras the canonical failure channel: autocall collects tool errors viafrc.Error != nil, and the mcptool tests setErroron failure. Previously the Copilot provider only ever setResultand leftErrornil, so a failed Copilot tool call looked identical to a successful one to any consumer that inspectsError. This matches the .NET/Python semantics where a failed function result carries its exception/error rather than folding it into the result payload, keeping cross-SDK behavior aligned.How tested
Extended the existing tests in
copilot_test.go:Error != niland that its message includes both the SDKMessageandCode.Error == nil.go build ./...,go vet ./provider/copilotprovider/..., andgo test ./provider/copilotprovider/...all pass.