-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix MCP tool timeouts halting agent flow #2249
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?
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
I will check later but overall it looks safe and good to me. |
|
@codex review again |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a64f752bb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| wrapped_error = AgentsException("Error invoking MCP tool crashing_tool: Crash!") | ||
| expected_error_msg = default_tool_error_function(tool_context, wrapped_error) | ||
| assert result == expected_error_msg |
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.
Fix expected error message mismatch in MCP test
The test builds expected_error_msg from AgentsException("Error invoking MCP tool crashing_tool: Crash!"), but invoke_mcp_tool actually wraps failures as "Error invoking MCP tool <name> on server '<server>'", so the result returned by function_tool.on_invoke_tool will include the server name (e.g., fake_mcp_server) and this exact equality assertion will fail. To keep the test reliable, construct the expected message using the same format as invoke_mcp_tool or assert on a stable substring.
Useful? React with 👍 / 👎.
Fixes an issue where MCP tool timeouts (and other errors) would halt the entire agent flow instead of being handled gracefully.
Problem:
When an MCP tool timed out or errored, it would raise an
AgentsExceptionthat propagated up and stopped the agent. This was inconsistent with regular function tools, which handle errors gracefully viafailure_error_function.Solution:
Wrapped the MCP tool invocation in
to_function_toolwith error handling that catches exceptions and converts them to error messages usingdefault_tool_error_function, matching the behavior of regular function tools.Changes:
MCPUtil.to_function_tool()Repro Script: