-
Notifications
You must be signed in to change notification settings - Fork 857
fix: normalize restored tool results #2320
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -370,6 +370,59 @@ def test_fix_broken_tool_use_extends_partial_tool_results(existing_session_manag | |
| assert missing_result["toolResult"]["content"][0]["text"] == "Tool was interrupted." | ||
|
|
||
|
|
||
| def test_fix_broken_tool_use_drops_extra_tool_results(session_manager): | ||
| """Keep valid toolResults, drop stale duplicates, and fill missing ones.""" | ||
| messages = [ | ||
| { | ||
| "role": "assistant", | ||
| "content": [ | ||
| {"toolUse": {"toolUseId": "complete-123", "name": "test_tool", "input": {"input": "test1"}}}, | ||
| {"toolUse": {"toolUseId": "missing-456", "name": "test_tool", "input": {"input": "test2"}}}, | ||
| ], | ||
| }, | ||
| { | ||
| "role": "user", | ||
| "content": [ | ||
| {"toolResult": {"toolUseId": "complete-123", "status": "success", "content": [{"text": "ok"}]}}, | ||
| {"toolResult": {"toolUseId": "stale-789", "status": "error", "content": [{"text": "old"}]}}, | ||
| {"toolResult": {"toolUseId": "complete-123", "status": "success", "content": [{"text": "dup"}]}}, | ||
| ], | ||
| }, | ||
| ] | ||
|
|
||
| fixed_messages = session_manager._fix_broken_tool_use(messages) | ||
|
|
||
| tool_results = [content["toolResult"] for content in fixed_messages[1]["content"]] | ||
| assert [tool_result["toolUseId"] for tool_result in tool_results] == ["complete-123", "missing-456"] | ||
| assert tool_results[0]["content"] == [{"text": "ok"}] | ||
| assert tool_results[1]["status"] == "error" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: Consider adding a test for the edge case where Suggestion: def test_fix_broken_tool_use_drops_stale_results_with_no_missing_ids(session_manager):
"""Test that stale toolResults are removed even when all toolUse IDs are satisfied."""
messages = [
{
"role": "assistant",
"content": [
{"toolUse": {"toolUseId": "id-1", "name": "t", "input": {}}},
],
},
{
"role": "user",
"content": [
{"toolResult": {"toolUseId": "id-1", "status": "success", "content": [{"text": "ok"}]}},
{"toolResult": {"toolUseId": "stale-999", "status": "error", "content": [{"text": "old"}]}},
],
},
]
fixed = session_manager._fix_broken_tool_use(messages)
results = [c["toolResult"] for c in fixed[1]["content"]]
assert len(results) == 1
assert results[0]["toolUseId"] == "id-1"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the bot again! I think we need this test for sure |
||
|
|
||
|
|
||
| def test_fix_broken_tool_use_drops_stale_results_with_no_missing_ids(session_manager): | ||
| """Drop stale toolResults even when every toolUse already has a valid result.""" | ||
| messages = [ | ||
| { | ||
| "role": "assistant", | ||
| "content": [ | ||
| {"toolUse": {"toolUseId": "complete-123", "name": "test_tool", "input": {"input": "test1"}}}, | ||
| ], | ||
| }, | ||
| { | ||
| "role": "user", | ||
| "content": [ | ||
| {"toolResult": {"toolUseId": "complete-123", "status": "success", "content": [{"text": "ok"}]}}, | ||
| {"toolResult": {"toolUseId": "stale-789", "status": "error", "content": [{"text": "old"}]}}, | ||
| ], | ||
| }, | ||
| ] | ||
|
|
||
| fixed_messages = session_manager._fix_broken_tool_use(messages) | ||
|
|
||
| tool_results = [content["toolResult"] for content in fixed_messages[1]["content"]] | ||
| assert [tool_result["toolUseId"] for tool_result in tool_results] == ["complete-123"] | ||
| assert tool_results[0]["content"] == [{"text": "ok"}] | ||
|
|
||
|
|
||
| def test_fix_broken_tool_use_handles_multiple_orphaned_tools(existing_session_manager): | ||
| """Test fixing multiple orphaned toolUse messages.""" | ||
|
|
||
|
|
||
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.
Issue: Missing docstring. The neighboring tests (
test_fix_broken_tool_use_extends_partial_tool_results,test_fix_broken_tool_use_handles_multiple_orphaned_tools) all have docstrings describing what they test. This one should too for consistency.Suggestion: Add a brief docstring:
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, docstring would be great