-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Tool reply meta #6074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tool reply meta #6074
Conversation
|
No, for now we're not changing what we are actually sending to the LLM; just what gets send to the client. we should look into that separately since it does feel like that currently we are missing some stuff. |
Oh, I see! OK, cool. Is there an easy way to inspect the client data in goose UI? |
|
depends on what you want to see. creating a diagnostics report is probably the best thing you can do. it gives you an export of the current conversation and some jsonl files of recent stuff we sent to the providers |
alexhancock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is good, thanks!
I want to get rid of ToolResult as it's not doing much helpful any more, but I can do that in a followup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
| CancellationToken::default(), | ||
| ) | ||
| .await | ||
| .map_err(|_e| StatusCode::INTERNAL_SERVER_ERROR)?; |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is being discarded. Consider logging it or including error details in the response to help with debugging when resource reads fail.
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
|
|
||
| let result = tool_result | ||
| .result | ||
| .await | ||
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error information is being discarded at two levels. When a tool call fails, the error details should be logged or included in the response to help with debugging.
| Ok(rmcp::model::CallToolResult { | ||
| content: vec![Content::text( | ||
| " | ||
| 50°F°C | ||
| Precipitation: 0% | ||
| Humidity: 84% | ||
| Wind: 2 mph | ||
| Weather | ||
| Saturday 9:00 PM | ||
| Clear", | ||
| )]), | ||
| )], | ||
| structured_content: None, | ||
| is_error: Some(false), | ||
| meta: None, | ||
| }), |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting is_error: Some(false) on success paths is redundant and creates unnecessary boilerplate. Consider having CallToolResult default is_error to None on success, or creating a helper method like CallToolResult::success(content) that handles this automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.
| CancellationToken::default(), | ||
| ) | ||
| .await | ||
| .map_err(|_e| StatusCode::INTERNAL_SERVER_ERROR)?; |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error information is being discarded when mapping errors to StatusCode. The specific error details could be helpful for debugging. Consider logging the error before converting to a generic HTTP status code.
bbd590e to
79b3d98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 3 comments.
| path = "/agent/call_tool", | ||
| request_body = CallToolRequest, | ||
| responses( | ||
| (status = 200, description = "Resource read successfully", body = CallToolResponse), |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error description says "Resource read successfully" but this endpoint is for calling tools, not reading resources. The description should say something like "Tool executed successfully" to match the endpoint's purpose.
| (status = 200, description = "Resource read successfully", body = CallToolResponse), | |
| (status = 200, description = "Tool executed successfully", body = CallToolResponse), |
| .extension_manager | ||
| .dispatch_tool_call(tool_call, CancellationToken::default()) | ||
| .await | ||
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error information is being discarded when converting errors to StatusCode. This makes debugging harder as the client won't know what went wrong. Consider logging the error or returning it in the response body.
| }, | ||
| "responses": { | ||
| "200": { | ||
| "description": "Resource read successfully", |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response description says "Resource read successfully" but this endpoint is for calling tools, not reading resources. The description should say something like "Tool executed successfully" to match the endpoint's purpose.
…nses-streaming * 'main' of github.com:block/goose: Fix community page mobile responsiveness and horizontal overflow (#6082) Tool reply meta (#6074) chore: avoid accidentally using native tls again (#6086) Update vars to be capitalised to be in line with other variables in config file (#6085) docs: restructure recipe reference (#5972) docs: configure custom providers (#6044) docs: Community All-Stars Spotlight November 2025, CodeTV Hackathon edition (#6070) fix: include file attachments in queued messages (#5961) fix(ui): prevent incorrect provider type suffix in update dialog #5908 (#5909) docs: mcp elicitation (#6060)
* 'main' of github.com:block/goose: (22 commits) Disallow subagents with no extensions (#5825) chore(deps): bump js-yaml in /documentation (#6093) feat: external goosed server (#5978) fix: Make datetime info message more explicit to prevent LLM confusion about current year (#6101) refactor: unify subagent and subrecipe tools into single tool (#5893) goose repo is too big for the issue solver workflow worker (#6099) fix: use system not developer role in db (#6098) Add /goose issue solver github workflow (#6068) OpenAI responses streaming (#5837) Canonical models for Providers (#5694) feat: add Inception provider for Mercury models (#6029) fix old sessions with tool results not loading (#6094) Fix community page mobile responsiveness and horizontal overflow (#6082) Tool reply meta (#6074) chore: avoid accidentally using native tls again (#6086) Update vars to be capitalised to be in line with other variables in config file (#6085) docs: restructure recipe reference (#5972) docs: configure custom providers (#6044) docs: Community All-Stars Spotlight November 2025, CodeTV Hackathon edition (#6070) fix: include file attachments in queued messages (#5961) ... # Conflicts: # crates/goose-server/src/routes/agent.rs # crates/goose/src/agents/extension_manager.rs # ui/desktop/src/api/sdk.gen.ts
…sions * 'main' of github.com:block/goose: (22 commits) Disallow subagents with no extensions (#5825) chore(deps): bump js-yaml in /documentation (#6093) feat: external goosed server (#5978) fix: Make datetime info message more explicit to prevent LLM confusion about current year (#6101) refactor: unify subagent and subrecipe tools into single tool (#5893) goose repo is too big for the issue solver workflow worker (#6099) fix: use system not developer role in db (#6098) Add /goose issue solver github workflow (#6068) OpenAI responses streaming (#5837) Canonical models for Providers (#5694) feat: add Inception provider for Mercury models (#6029) fix old sessions with tool results not loading (#6094) Fix community page mobile responsiveness and horizontal overflow (#6082) Tool reply meta (#6074) chore: avoid accidentally using native tls again (#6086) Update vars to be capitalised to be in line with other variables in config file (#6085) docs: restructure recipe reference (#5972) docs: configure custom providers (#6044) docs: Community All-Stars Spotlight November 2025, CodeTV Hackathon edition (#6070) fix: include file attachments in queued messages (#5961) ... # Conflicts: # crates/goose-server/src/routes/agent.rs # crates/goose/src/agents/extension_manager.rs # ui/desktop/src/api/sdk.gen.ts





Summary
Make goose keep the toolresults around