-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix ResultsFormat error when loading old sessions #6385
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
Conversation
| } | ||
| } | ||
|
|
||
| #[test] |
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.
these tests are rather repetitive - if you wanted to do this, turn them into parameterized tests that only have the difference between the cases rather than repeating yourself.
but we should maybe think one deeper on how we avoid breaking this in the future - it happened before when we change the message format. adding tests like this is just very specifically going to test for this, doesn't seem that likely that we'll catch future issues.
wouldn't it be better to just check in a bunch of historic conversations and verify that we can load them?
|
|
||
| pub fn deserialize<'de, T, D>(deserializer: D) -> Result<ToolResult<T>, D::Error> | ||
| /// Legacy ToolCall format from older sessions (pre-rmcp migration). | ||
| /// The old format had `arguments: Value` instead of `arguments: Option<JsonObject>`. |
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.
I think it would be better to remove the comments here and also not call it just legacy; surely we'll have more changes to formats coming up, representing in the name what's going on here would be better. LegacyToolCallWithValue? longer names beats comments here for sure.
| Success { status: String, value: T }, | ||
| Error { status: String, error: String }, | ||
| enum ResultFormat { | ||
| NewSuccess { |
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.
same here; New is not going stay that way
* 'main' of github.com:block/goose: Fixed fonts (#6389) Update confidence levels prompt injection detection to reduce false positive rates (#6390) Add ML-based prompt injection detection (#5623) docs: update custom extensions tutorial (#6388) fix ResultsFormat error when loading old sessions (#6385) docs: add MCP Apps tutorial and documentation updates (#6384) changed z-index to make sure the search highlighter does not appear on modal overlay (#6386) Handling special claude model response in github copilot provider (#6369) fix: prevent duplicate rendering when tool returns both mcp-ui and mcp-apps resources (#6378) fix: update MCP Apps _meta.ui.resourceUri to use nested format (SEP-1865) (#6372) feat(providers): add streaming support for Google Gemini provider (#6191) Blog: edit links in mcp apps post (#6371) fix: prevent infinite loop of tool-input notifications in MCP Apps (#6374)
* main: (31 commits) added validation and debug for invalid call tool result (#6368) Update MCP apps tutorial: fix _meta structure and version prereq (#6404) Fixed fonts (#6389) Update confidence levels prompt injection detection to reduce false positive rates (#6390) Add ML-based prompt injection detection (#5623) docs: update custom extensions tutorial (#6388) fix ResultsFormat error when loading old sessions (#6385) docs: add MCP Apps tutorial and documentation updates (#6384) changed z-index to make sure the search highlighter does not appear on modal overlay (#6386) Handling special claude model response in github copilot provider (#6369) fix: prevent duplicate rendering when tool returns both mcp-ui and mcp-apps resources (#6378) fix: update MCP Apps _meta.ui.resourceUri to use nested format (SEP-1865) (#6372) feat(providers): add streaming support for Google Gemini provider (#6191) Blog: edit links in mcp apps post (#6371) fix: prevent infinite loop of tool-input notifications in MCP Apps (#6374) fix: Show platform-specific keyboard shortcuts in UI (#6323) fix: we load extensions when agent starts so don't do it up front (#6350) docs: credit HumanLayer in RPI tutorial (#6365) Blog: Goose Lands MCP Apps (#6172) Claude 3.7 is out. we had some harcoded stuff (#6197) ...
* main: (89 commits) fix(google): treat signed text as regular content in streaming (#6400) Add frameDomains and baseUriDomains CSP support for MCP Apps (#6399) fix(ci): add missing dependencies to openapi-schema-check job (#6367) feat: http proxy support Add support for changing working dir and extensions in same window/session (#6057) Sort keys in canonical models (#6403) added validation and debug for invalid call tool result (#6368) Update MCP apps tutorial: fix _meta structure and version prereq (#6404) Fixed fonts (#6389) Update confidence levels prompt injection detection to reduce false positive rates (#6390) Add ML-based prompt injection detection (#5623) docs: update custom extensions tutorial (#6388) fix ResultsFormat error when loading old sessions (#6385) docs: add MCP Apps tutorial and documentation updates (#6384) changed z-index to make sure the search highlighter does not appear on modal overlay (#6386) Handling special claude model response in github copilot provider (#6369) fix: prevent duplicate rendering when tool returns both mcp-ui and mcp-apps resources (#6378) fix: update MCP Apps _meta.ui.resourceUri to use nested format (SEP-1865) (#6372) feat(providers): add streaming support for Google Gemini provider (#6191) Blog: edit links in mcp apps post (#6371) ...
* main: (89 commits) fix(google): treat signed text as regular content in streaming (#6400) Add frameDomains and baseUriDomains CSP support for MCP Apps (#6399) fix(ci): add missing dependencies to openapi-schema-check job (#6367) feat: http proxy support Add support for changing working dir and extensions in same window/session (#6057) Sort keys in canonical models (#6403) added validation and debug for invalid call tool result (#6368) Update MCP apps tutorial: fix _meta structure and version prereq (#6404) Fixed fonts (#6389) Update confidence levels prompt injection detection to reduce false positive rates (#6390) Add ML-based prompt injection detection (#5623) docs: update custom extensions tutorial (#6388) fix ResultsFormat error when loading old sessions (#6385) docs: add MCP Apps tutorial and documentation updates (#6384) changed z-index to make sure the search highlighter does not appear on modal overlay (#6386) Handling special claude model response in github copilot provider (#6369) fix: prevent duplicate rendering when tool returns both mcp-ui and mcp-apps resources (#6378) fix: update MCP Apps _meta.ui.resourceUri to use nested format (SEP-1865) (#6372) feat(providers): add streaming support for Google Gemini provider (#6191) Blog: edit links in mcp apps post (#6371) ...
Follow up for #6094
Summary: Session Loading Backward Compatibility Fix
Problem
Users reported an error when loading/chatting after upgrading the desktop app: stream error: failed to read session: data did not match any variant of untagged enum ResultFormat at line 1 column 136
Root Cause
The issue was a schema mismatch between old and new session data formats:
ToolCall):arguments: serde_json::Value(could be any JSON type)CallToolRequestParam):arguments: Option<JsonObject>(must be an object or null)When deserializing old sessions where
argumentswas a string, array, or number, the new deserialization logic failed because it couldn't match any variant of theResultFormatenum.Solution
Modified
crates/goose/src/conversation/tool_result_serde.rsto handle legacyToolCallformat:LegacyToolCallstruct to acceptarguments: serde_json::Valuedeserializefunction to include aLegacySuccessvariant for backward compatibilityLegacyToolCall::into_call_tool_request_param():Some(map)null→ converted toNone{"value": <original>}Files Changed
crates/goose/src/conversation/tool_result_serde.rsLegacyToolCallstruct witharguments: serde_json::Valueinto_call_tool_request_param()conversion methoddeserializefunction to handle both new and legacy formatscrates/goose/src/conversation/message.rstest_legacy_tool_request_with_value_arguments(string)test_legacy_tool_request_with_array_argumentstest_legacy_tool_request_with_number_argumentstest_legacy_tool_request_with_null_argumentstest_legacy_tool_request_with_object_argumentsVerification
cargo fmt