fix: enable sequential tool execution in OpenAI client #849
Conversation
Removes premature loop termination after tool execution to allow sequential tool calls. The model can now see tool results and make additional tool calls as needed. Fixes #845 Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
WalkthroughThe update modifies the control flow within the OpenAI client’s tool-calling loop, removing the special-case logic for the "sequentialthinking" tool and the final completion retrieval after tool execution. Now, the loop always continues to allow further tool calls until none remain or a maximum iteration count is reached, streamlining sequential tool invocation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Agent
participant OpenAIClient
participant Tool
User->>Agent: Submit prompt with possible tool calls
Agent->>OpenAIClient: Request chat completion with tools
loop Tool execution loop
OpenAIClient->>Tool: Execute tool call(s)
Tool-->>OpenAIClient: Return tool result(s)
OpenAIClient->>OpenAIClient: Continue loop if tool calls remain
end
OpenAIClient-->>Agent: Return final response
Agent-->>User: Respond with combined result
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@claude review this pull request and do a detailed analysis and fix this if the existing code doesn't have the solution implemented. Making sure it has backward compatibility, no existing features removed. After making those changes, again use multi agents to review the applied changes. Use @web to search if you dont know any information or to find the latest documentation or to find the latest version. Run the code if you think you need to run it to test it. Minimal code change to start with if required any changes. |
|
Claude encountered an error —— View job PR Review: Sequential Tool Execution FixTodo List:
Code Review Summary✅ What the PR does correctly:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @MervinPraison, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request delivers a crucial fix for the OpenAI client, enabling it to properly handle sequential tool execution. This enhancement allows the model to perform complex, multi-step operations by calling multiple tools in a single interaction, bringing its tool-use behavior in line with other LLM clients and significantly improving its capabilities for advanced agentic workflows.
Highlights
- Sequential Tool Execution Fix: I've addressed a critical bug in the OpenAI client's tool execution logic that previously prevented sequential tool calls. Before this fix, the system would prematurely terminate after the first tool invocation, limiting the agent's ability to handle multi-step tasks.
- Code Logic Adjustment: I've removed the
breakstatements that caused premature loop termination in both the synchronous (chat_completion_with_tools) and asynchronous (achat_completion_with_tools) functions within the OpenAI client. This change ensures that the model can continue processing and make multiple, chained tool calls as needed. - New Test Case: I've added a new dedicated test file,
test_sequential_fix.py, to specifically validate and demonstrate the correct sequential execution of multiple tools (e.g.,get_stock_pricefollowed bymultiply) when using the OpenAI client.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
test_sequential_fix.py
Outdated
| result = agent.start("what is the stock price of Google? multiply the Google stock price with 2") | ||
| print("\n" + "=" * 60) | ||
| print("FINAL RESULT:") | ||
| print(result) | ||
| print("=" * 60) |
There was a problem hiding this comment.
Consider adding assert statements to verify the final result, instead of relying on manual inspection of the printed output. This would make it a proper test case.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test_sequential_fix.py (2)
22-27: Enhance the docstring for consistency.The
multiplyfunction should have a complete docstring likeget_stock_price.def multiply(a: int, b: int) -> int: """ Multiply two numbers + + Args: + a (int): First number + b (int): Second number + + Returns: + int: The product of a and b """
39-44: Consider adding assertions to verify expected behavior.While print statements are useful for debugging, adding assertions would make the test more robust and suitable for automated testing.
result = agent.start("what is the stock price of Google? multiply the Google stock price with 2") print("\n" + "=" * 60) print("FINAL RESULT:") print(result) print("=" * 60) + +# Verify the result contains expected information +assert "100" in result, "Result should contain the stock price" +assert "200" in result, "Result should contain the multiplication result"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/llm/openai_client.py(2 hunks)test_sequential_fix.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
test_sequential_fix.py (3)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/tools/test.ts : The 'src/tools/test.ts' file should provide a script for running each tool's internal test or example.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-06-30T10:06:44.129Z
Learning: Applies to src/praisonai-ts/src/tools/test.ts : The 'src/tools/test.ts' file should serve as a script for running internal tests or examples for each tool.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-06-30T10:06:17.673Z
Learning: Run individual test files as scripts (e.g., `python tests/basic-agents.py`) rather than using a formal test runner.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Cursor BugBot
- GitHub Check: test-core (3.11)
- GitHub Check: quick-test
- GitHub Check: Run tests and collect coverage
- GitHub Check: quick-test
🔇 Additional comments (2)
src/praisonai-agents/praisonaiagents/llm/openai_client.py (2)
909-909: Fix enables proper sequential tool execution.The removal of premature loop termination allows the model to see tool results and invoke additional tools as needed, which correctly addresses issue #845.
1064-1064: Async implementation correctly mirrors the sync fix.The async method maintains consistency with the synchronous version, ensuring sequential tool execution works properly in both contexts.
|
@claude fix above issue |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @MervinPraison's task —— View job PR Review: Sequential Tool Execution FixCompleted Tasks:
SummaryI've completed a thorough review of this PR and made improvements to clean up dead code. Here's what was done: 1. Original Fix Analysis ✅The PR correctly fixes the sequential tool execution issue by removing code that was breaking out of the loop prematurely. This allows the model to see tool results and make additional tool calls as needed. 2. Code Improvements Made 🔧
3. Verification ✅
4. Multi-Agent Review Results 🔍
Final AssessmentThe PR successfully fixes issue #845 with minimal, targeted changes. After removing the dead code, the implementation is cleaner and more maintainable. The fix enables sequential tool execution as intended, matching the behavior of other LLM clients in the codebase. Status: ✅ Ready to merge |
|
Claude finished @MervinPraison's task —— View job Todo List:
SummaryI've successfully addressed the code review feedback by:
The test file now properly validates that sequential tool execution works correctly, addressing the feedback from @gemini-code-assist about having assertions instead of relying on manual inspection of printed output. |
- Add proper assertions to verify sequential tool execution results - Improve multiply function docstring with complete Args and Returns sections - Add success message when all assertions pass Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude review if the changes made are as per the requirements |
|
Claude finished @MervinPraison's task —— View job ✅ PR Review: Changes Meet RequirementsI've reviewed the changes and can confirm they properly address all the requirements: Core Fix ✓The sequential tool execution issue has been correctly fixed by removing the premature loop termination in both Backward Compatibility ✓No existing features were removed. The changes only fix the bug that was preventing sequential tool execution. All existing functionality remains intact. Test Implementation ✓The test file
Working Implementation ✓The test demonstrates the expected behavior:
Minor ObservationThe ConclusionThe PR successfully fixes issue #845 with minimal, focused changes that enable sequential tool execution while maintaining full backward compatibility. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test_sequential_fix.py (1)
9-21: Consider making the hardcoded value more explicit.The function correctly simulates a stock price lookup with proper documentation and debug output. However, the hardcoded return value of "100" could be made more explicit.
Consider this small improvement for clarity:
def get_stock_price(company_name: str) -> str: """ Get the stock price of a company Args: company_name (str): The name of the company Returns: str: The stock price of the company """ print(f"[Tool Called] get_stock_price with company_name='{company_name}'") - return f"The stock price of {company_name} is 100" + # Hardcoded for testing - simulates stock price lookup + return f"The stock price of {company_name} is 100"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test_sequential_fix.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
test_sequential_fix.py (3)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/tools/test.ts : The 'src/tools/test.ts' file should provide a script for running each tool's internal test or example.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-06-30T10:06:44.129Z
Learning: Applies to src/praisonai-ts/src/tools/test.ts : The 'src/tools/test.ts' file should serve as a script for running internal tests or examples for each tool.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-06-30T10:06:17.673Z
Learning: Run individual test files as scripts (e.g., `python tests/basic-agents.py`) rather than using a formal test runner.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: quick-test
- GitHub Check: test-core (3.11)
- GitHub Check: Run tests and collect coverage
- GitHub Check: quick-test
🔇 Additional comments (4)
test_sequential_fix.py (4)
1-8: LGTM! Well-documented test purpose.The file header clearly identifies the test purpose and references the specific issue being addressed. The import statement is appropriate for the testing framework.
22-35: LGTM! Clean implementation of multiplication tool.The multiply function is well-documented with proper type hints and includes helpful debug output for tracking tool execution.
46-46: Well-crafted test prompt for sequential execution.The prompt effectively requires both tools to be called in sequence - first to get the stock price, then to multiply it. This directly tests the fix for issue #845.
58-61: Excellent documentation of expected behavior.The comments clearly explain the sequential execution flow that should occur, making it easy to understand what the test is validating.
test_sequential_fix.py
Outdated
| # Verify the result contains expected information | ||
| assert "100" in result, "Result should contain the stock price (100)" | ||
| assert "200" in result, "Result should contain the multiplication result (200)" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance assertion robustness for better test reliability.
While the string-based assertions address the previous review feedback, they may be brittle if the LLM formats responses differently. The test should be more robust in verifying actual sequential execution.
Consider these improvements for more reliable testing:
# Verify the result contains expected information
-assert "100" in result, "Result should contain the stock price (100)"
-assert "200" in result, "Result should contain the multiplication result (200)"
+import re
+# More robust assertions that account for different formatting
+stock_price_pattern = r'100'
+multiplication_pattern = r'200'
+
+assert re.search(stock_price_pattern, result), f"Result should contain the stock price (100). Got: {result}"
+assert re.search(multiplication_pattern, result), f"Result should contain the multiplication result (200). Got: {result}"
+
+# Additional verification: check that both tools were actually called
+# This could be enhanced by capturing tool execution logs
+print(f"Result content for verification: {repr(result)}")Additionally, consider capturing and verifying the actual tool call sequence rather than just checking if the results appear in the final output.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Verify the result contains expected information | |
| assert "100" in result, "Result should contain the stock price (100)" | |
| assert "200" in result, "Result should contain the multiplication result (200)" | |
| # Verify the result contains expected information | |
| import re | |
| # More robust assertions that account for different formatting | |
| stock_price_pattern = r'100' | |
| multiplication_pattern = r'200' | |
| assert re.search(stock_price_pattern, result), f"Result should contain the stock price (100). Got: {result}" | |
| assert re.search(multiplication_pattern, result), f"Result should contain the multiplication result (200). Got: {result}" | |
| # Additional verification: check that both tools were actually called | |
| # This could be enhanced by capturing tool execution logs | |
| print(f"Result content for verification: {repr(result)}") |
🤖 Prompt for AI Agents
In test_sequential_fix.py around lines 52 to 54, the current assertions check
for specific strings in the result, which can be brittle if the output format
changes. To improve robustness, modify the test to capture the actual sequence
of tool calls or operations performed during execution and assert that these
calls occur in the expected order. This can be done by mocking or instrumenting
the tool invocation methods to record their call sequence, then verifying this
sequence explicitly rather than relying solely on string presence in the final
output.
test_sequential_fix.py
Outdated
| agent = Agent( | ||
| instructions="You are a helpful assistant. You can use the tools provided to you to help the user.", | ||
| llm="gpt-4o", | ||
| tools=[get_stock_price, multiply] | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify model availability and version.
The Agent configuration looks correct, but ensure the "gpt-4o" model identifier is valid and available.
🌐 Web query:
What is the correct model identifier for GPT-4o in OpenAI's API? Is "gpt-4o" the standard identifier?
💡 Result:
In OpenAI's API, the standard model identifier for GPT-4o is gpt-4o. This alias points to the latest version of GPT-4o, which is gpt-4o-2024-08-06. While gpt-4o is convenient for general use, it's advisable to use the dated version identifier, such as gpt-4o-2024-08-06, in production applications. This practice ensures consistency, as the alias may point to different snapshots over time. (blog.liveleads.co)
Citations:
Specify the GPT-4o model version
While the gpt-4o alias is valid, it’s best practice to pin to a dated snapshot in production to avoid unintentional model updates.
• File: test_sequential_fix.py (lines 40–44)
agent = Agent(
instructions="You are a helpful assistant. You can use the tools provided to you to help the user.",
- llm="gpt-4o",
+ llm="gpt-4o-2024-08-06",
tools=[get_stock_price, multiply]
)Use gpt-4o-2024-08-06 (or the latest dated identifier) instead of the alias.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agent = Agent( | |
| instructions="You are a helpful assistant. You can use the tools provided to you to help the user.", | |
| llm="gpt-4o", | |
| tools=[get_stock_price, multiply] | |
| ) | |
| agent = Agent( | |
| instructions="You are a helpful assistant. You can use the tools provided to you to help the user.", | |
| llm="gpt-4o-2024-08-06", | |
| tools=[get_stock_price, multiply] | |
| ) |
🤖 Prompt for AI Agents
In test_sequential_fix.py around lines 40 to 44, replace the llm model alias
"gpt-4o" with the specific dated version "gpt-4o-2024-08-06" to ensure
consistent and stable model behavior in production environments.
- Remove unused should_continue variable and related logic - Simplify code by removing special handling for sequentialthinking tool - Add clear comments explaining the sequential execution behavior Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Fixes #845
Description
This PR fixes the sequential tool execution issue in the OpenAI client where only the first tool would be executed and subsequent tools would be skipped.
Changes
chat_completion_with_toolsachat_completion_with_toolsTesting
The fix allows the example code to work correctly:
get_stock_price("Google")returns "100"multiply(100, 2)returns "200"Generated with Claude Code
Here is the working code for reference this is for litellm client,
Also make sure no feature is removed
Summary by CodeRabbit