-
Notifications
You must be signed in to change notification settings - Fork 20
Update to fix MCP issue #157
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
PR 150 caused a problem with MCP tool use because of the sync invoke inside the tool_use node Needed to revert to the ToolNode, but that required some better structure in the fix from PR 150. This appears to work fully, but need to run the tests and get others to take a shot at it.
awadell1
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.
It probably would be good to have more explicit testing of the checkpointer. I also haven't tested locally, but with the removal of tool_use this should now be hitting the tests on in test_base_interface again
|
This does not appear to fully resolve the mcp issues: ❯ uv run ursa --config ursa.yaml
<frozen importlib._bootstrap>:241: DeprecationWarning: builtin type SwigPyPacked has no __module__ attribute
<frozen importlib._bootstrap>:241: DeprecationWarning: builtin type SwigPyObject has no __module__ attribute
<frozen importlib._bootstrap>:241: DeprecationWarning: builtin type swigvarlink has no __module__ attribute
__ ________________ _
/ / / / ___/ ___/ __ `/
/ /_/ / / (__ ) /_/ /
\__,_/_/ /____/\__,_/
For help, type: ? or help. Exit with Ctrl+d.
ursa> execute what is the homo lumo level of caffine?
Error: Error code: 400 - {'error': {'message': "An assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'. The following tool_call_ids did not have response messages: call_B56UtJufVckNfjoqkNXWSPeS", 'type': 'invalid_request_error', 'param': 'messages.[3].role', 'code': None}} what is the homo lumo level of caffine?
Error: Error code: 400 - {'error': {'message': "An assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'. The following tool_call_ids did not have response messages: call_B56UtJufVckNfjoqkNXWSPeS", 'type': 'invalid_request_error', 'param': 'messages.[3].role', 'code': None}}
You are a summarizing agent. You have a user/assistant conversation as they work through a complex problem requiring multiple steps.
Your responsibilities is to write a condensed summary of the conversation.
- Keep all important points from the conversation.
- Ensure the summary responds to the goals of the original query.
- Summarize all the work that was carried out to meet those goals
- Highlight any places where those goals were not achieved and why.
⠹ Generating response
Recap of Work
│ Response error Error code: 400 - {'error': {'message': "An assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'. The following tool_call_ids did not have response messages: call_B56UtJufVckNfjoqkNXWSPeS", 'type': 'invalid_request_error', 'param': 'messages.[3].role', 'code': None}}
Response error Error code: 400 - {'error': {'message': "An assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'. The following tool_call_ids did not have response messages: call_B56UtJufVckNfjoqkNXWSPeS", 'type': 'invalid_request_error', 'param': 'messages.[3].role', 'code': None}}
|
That looks like a different error. Specifically, it looks like you are trying to start from a checkpoint that is corrupted because it has a dangling tool call. Can you try that same test in a clean workspace (or at least one without a checkpoint file)? |
|
Yep that was it, it works great with |
|
@ndebard was having some tool response issues. I dont think they are because of this PR, but because he sees them on this PR branch, I want to get to the bottom of it before merging this. |
…angGraph type expectations.
PR #150 caused a problem with MCP tool use because of the sync invoke inside the tool_use node needed to revert to the ToolNode, but that required some better structure in the fix from PR #150.
This appears to work fully, but need to run the tests and get others to take a shot at it. Especially @awadell1
NOTE: The branch name is not descriptive of the final changes. I expected to need to revert back more but was able to get it working differently.