fix(agents): support Bedrock toolUse format in _parse_native_tool_call (fixes #4972)#5902
fix(agents): support Bedrock toolUse format in _parse_native_tool_call (fixes #4972)#5902STiFLeR7 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR fixes Bedrock Converse API tool-call parsing by detecting and unwrapping the ChangesBedrock toolUse wrapper support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/agents/crew_agent_executor.py`:
- Around line 806-808: After unwrapping tool_call with tool_call =
tool_call["toolUse"], add a dict-type guard so subsequent .get() calls don't
raise AttributeError: check isinstance(tool_call, dict) and if it's not a dict,
log or handle the malformed response (e.g., processLogger.warn or raise a
controlled exception) and skip or fallback to the original tool_call behavior;
update the code around the unwrap in crew_agent_executor (where tool_call is
reassigned) to perform this type check before any .get() usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c02a532-1f09-4f79-a429-578923b11b4e
📒 Files selected for processing (1)
lib/crewai/src/crewai/agents/crew_agent_executor.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai/src/crewai/agents/crew_agent_executor.py (1)
619-641:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBedrock
toolUsewrapper can still be dropped before parsingLine 634+ only treats dict tool calls as native when they expose
name/inputdirectly. A wrapped payload like{"toolUse": {...}}returnsFalsein_is_tool_call_list, so_parse_native_tool_callis never reached in the native loop.💡 Minimal fix
@@ if ( isinstance(first_item, dict) + and "toolUse" in first_item + and isinstance(first_item["toolUse"], dict) + ): + return True + if ( + isinstance(first_item, dict) and "name" in first_item and "input" in first_item ): return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/agents/crew_agent_executor.py` around lines 619 - 641, The _is_tool_call_list logic fails to recognize wrapped Bedrock tool calls like {"toolUse": {...}} so _parse_native_tool_call is never invoked; update the predicate in _is_tool_call_list to also detect when the first_item is a dict containing a single "toolUse" key whose value is a dict (or contains name/input/function_call/tool fields) so wrapped payloads are treated as native tool calls; reference the _is_tool_call_list method and ensure it checks for "toolUse" wrappers on first_item before returning False so the existing _parse_native_tool_call path will be exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@repro_4972.py`:
- Around line 1-45: The test file repro_4972.py is not collected by CI because
it's at the repo root and not named/placed per pytest conventions; move and
rename it so pytest will run it (e.g., place the file under lib/crewai/tests/
and rename to test_repro_4972.py or similar) and keep the tests as-is (they
exercise CrewAgentExecutor.__new__ and the _parse_native_tool_call method via
the TestParseNativeToolCall class and its test_parse_bedrock_converse_format and
test_parse_direct_bedrock_format methods) so CI run from lib/crewai will
discover and execute them.
---
Outside diff comments:
In `@lib/crewai/src/crewai/agents/crew_agent_executor.py`:
- Around line 619-641: The _is_tool_call_list logic fails to recognize wrapped
Bedrock tool calls like {"toolUse": {...}} so _parse_native_tool_call is never
invoked; update the predicate in _is_tool_call_list to also detect when the
first_item is a dict containing a single "toolUse" key whose value is a dict (or
contains name/input/function_call/tool fields) so wrapped payloads are treated
as native tool calls; reference the _is_tool_call_list method and ensure it
checks for "toolUse" wrappers on first_item before returning False so the
existing _parse_native_tool_call path will be exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 351a155c-7682-4136-9043-e950a6aa7264
📒 Files selected for processing (2)
lib/crewai/src/crewai/agents/crew_agent_executor.pyrepro_4972.py
| import unittest | ||
| from typing import Any | ||
| from crewai.agents.crew_agent_executor import CrewAgentExecutor | ||
|
|
||
| class TestParseNativeToolCall(unittest.TestCase): | ||
| def setUp(self): | ||
| # We need a mock agent or just call the method on a dummy instance | ||
| self.executor = CrewAgentExecutor.__new__(CrewAgentExecutor) | ||
|
|
||
| def test_parse_bedrock_converse_format(self): | ||
| # Format reported in issue #4972 | ||
| tool_call = { | ||
| "toolUse": { | ||
| "toolUseId": "tooluse_123", | ||
| "name": "calculator", | ||
| "input": {"expression": "1 + 1"} | ||
| } | ||
| } | ||
|
|
||
| result = self.executor._parse_native_tool_call(tool_call) | ||
|
|
||
| self.assertIsNotNone(result, "Should not return None for Bedrock format") | ||
| call_id, func_name, func_args = result | ||
| self.assertEqual(call_id, "tooluse_123") | ||
| self.assertEqual(func_name, "calculator") | ||
| self.assertEqual(func_args, {"expression": "1 + 1"}) | ||
|
|
||
| def test_parse_direct_bedrock_format(self): | ||
| # Format if already unwrapped | ||
| tool_call = { | ||
| "toolUseId": "tooluse_123", | ||
| "name": "calculator", | ||
| "input": {"expression": "1 + 1"} | ||
| } | ||
|
|
||
| result = self.executor._parse_native_tool_call(tool_call) | ||
|
|
||
| self.assertIsNotNone(result) | ||
| call_id, func_name, func_args = result | ||
| self.assertEqual(call_id, "tooluse_123") | ||
| self.assertEqual(func_name, "calculator") | ||
| self.assertEqual(func_args, {"expression": "1 + 1"}) | ||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Test discovery config files =="
fd -HI '^(pytest\.ini|pyproject\.toml|setup\.cfg|tox\.ini)$' -x sh -c '
echo "--- $1 ---"
rg -n "python_files|testpaths|addopts|unittest" "$1" || true
' sh {}
echo
echo "== Files matching common test patterns =="
fd -HI '(^test_.*\.py$|.*_test\.py$)'
echo
echo "== PR-added file =="
fd -HI '^repro_4972\.py$'Repository: crewAIInc/crewAI
Length of output: 14485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate repro_4972.py =="
fd -HI '^repro_4972\.py$' -t f -a .
echo
echo "== Where pytest is invoked (CI/workflows or scripts) =="
fd -HI '(.github/workflows|scripts|Makefile|pyproject\.toml)$' -t f -a . || true
rg -n --hidden --glob '!.git' -S 'pytest' .github workflows scripts Makefile pyproject.toml 2>/dev/null || rg -n --hidden --glob '!.git' -S 'pytest' . || true
echo
echo "== Any pytest test file pattern overrides =="
rg -n -S 'python_files|testpaths|unittest|python_classes|norecursedirs' pyproject.toml **/pyproject.toml 2>/dev/null || true
echo
echo "== Any explicit unittest discovery config/commands =="
rg -n -S 'unittest\.TestLoader|unittest\.discover|python -m unittest|nose' . 2>/dev/null || trueRepository: crewAIInc/crewAI
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Root pyproject: pytest ini options =="
python - <<'PY'
import re, itertools, pathlib
p = pathlib.Path("pyproject.toml")
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if l.strip()=="[tool.pytest.ini_options]":
start=i
break
else:
start=None
if start:
for j in range(start, min(start+60, len(lines)+1)):
print(f"{j}:{lines[j-1]}")
PY
echo
echo "== lib/crewai pyproject: pytest ini options =="
python - <<'PY'
import pathlib
p = pathlib.Path("lib/crewai/pyproject.toml")
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if l.strip()=="[tool.pytest.ini_options]":
start=i
break
else:
start=None
if start:
for j in range(start, min(start+60, len(lines)+1)):
print(f"{j}:{lines[j-1]}")
PY
echo
echo "== lib/crewai-tools pyproject: pytest ini options =="
python - <<'PY'
import pathlib
p = pathlib.Path("lib/crewai-tools/pyproject.toml")
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if l.strip()=="[tool.pytest.ini_options]":
start=i
break
else:
start=None
if start:
for j in range(start, min(start+60, len(lines)+1)):
print(f"{j}:{lines[j-1]}")
PY
echo
echo "== Locate CI pytest invocations (relevant lines) =="
rg -n "cd lib/crewai && uv run pytest|cd lib/crewai-tools && uv run pytest" .github/workflows/tests.ymlRepository: crewAIInc/crewAI
Length of output: 3726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant parts of .github/workflows/tests.yml =="
sed -n '60,120p' .github/workflows/tests.yml
echo
echo "== Check whether lib/crewai*.pyproject.toml defines pytest ini options =="
rg -n "^\[tool\.pytest\.ini_options\]" lib/crewai/pyproject.toml lib/crewai-tools/pyproject.toml || true
echo
echo "== Show surrounding pytest config in root (just in case) =="
sed -n '120,180p' pyproject.tomlRepository: crewAIInc/crewAI
Length of output: 4897
Move/rename repro_4972.py so CI can collect it
repro_4972.py sits at the repo root, but pytest in this repo is configured to only collect from lib/*/tests and only files matching test_*.py, and CI runs pytest from lib/crewai and lib/crewai-tools. As a result, these tests won’t run in CI.
Move it under the relevant lib/crewai/tests/... (or other appropriate lib/*/tests directory) and rename it to test_*.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@repro_4972.py` around lines 1 - 45, The test file repro_4972.py is not
collected by CI because it's at the repo root and not named/placed per pytest
conventions; move and rename it so pytest will run it (e.g., place the file
under lib/crewai/tests/ and rename to test_repro_4972.py or similar) and keep
the tests as-is (they exercise CrewAgentExecutor.__new__ and the
_parse_native_tool_call method via the TestParseNativeToolCall class and its
test_parse_bedrock_converse_format and test_parse_direct_bedrock_format methods)
so CI run from lib/crewai will discover and execute them.
|
@coderabbitai I have added the dict-type guard as requested in the latest commit. Please review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1673 🏁 Script executed: Length of output: 61880
if "toolUse" in tool_call:
tool_call = tool_call["toolUse"]
if not isinstance(tool_call, dict):
return NoneThis safely handles the case where I do have two remaining observations to flag: 1.
# None of these match {"toolUse": {"toolUseId": ..., "name": ..., "input": ...}}
isinstance(first_item, dict) and "function" in first_item # False
isinstance(first_item, dict) and "name" in first_item and "input" in first_item # FalseConsider adding a check there: if isinstance(first_item, dict) and "toolUse" in first_item:
return TrueThis is only needed if LiteLLM does not normalise Bedrock converse responses before returning them. If LiteLLM always converts them to OpenAI format upstream, this gap is irrelevant — but it may be worth double-checking or adding a comment clarifying that assumption. 2. Test file placement
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/crewai/tests/agents/test_repro_4972.py (2)
6-8: ⚖️ Poor tradeoffConsider improving testability instead of bypassing initialization.
Using
__new__to bypass__init__is a testing anti-pattern. Consider one of these approaches:
- If
_parse_native_tool_calldoesn't depend on instance state, refactor it to a static or class method.- If
__init__has complex dependencies, introduce a factory method or use dependency injection to make the class easier to test.- Mock required dependencies in
setUprather than creating an uninitialized instance.This improves test maintainability and signals whether the method truly needs instance state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/agents/test_repro_4972.py` around lines 6 - 8, The test currently creates an uninitialized CrewAgentExecutor via CrewAgentExecutor.__new__ in setUp; instead, make _parse_native_tool_call testable without bypassing __init__ by either refactoring _parse_native_tool_call into a `@staticmethod` or `@classmethod` if it does not use instance state, or by adding a test-friendly factory or dependency-injection hook on CrewAgentExecutor so setUp can construct a fully initialized instance; alternatively, if instance state is required, update setUp to construct a real CrewAgentExecutor with mocked dependencies (mock whatever __init__ requires) rather than using __new__, and update tests to call the method on that properly initialized object.
10-42: ⚡ Quick winAdd test coverage for malformed inputs to verify type guards.
The PR description states that "type guards to prevent AttributeErrors on malformed responses" were added, but the current test suite only validates happy paths. Add test cases for:
- Malformed
toolUsevalue:{"toolUse": "not_a_dict"}Noneinput- Empty dict:
{}- Missing required keys (e.g.,
{"toolUse": {"name": "foo"}}withoutinputortoolUseId)These tests will verify that the defensive type guards work as intended and prevent regressions.
📋 Example test cases
def test_parse_malformed_tooluse_value(self): """Type guard should handle toolUse with non-dict value.""" tool_call = {"toolUse": "not_a_dict"} result = self.executor._parse_native_tool_call(tool_call) self.assertIsNone(result, "Should return None for malformed toolUse") def test_parse_none_input(self): """Should handle None gracefully.""" result = self.executor._parse_native_tool_call(None) self.assertIsNone(result) def test_parse_empty_dict(self): """Should handle empty dict gracefully.""" result = self.executor._parse_native_tool_call({}) self.assertIsNone(result) def test_parse_missing_required_keys(self): """Should handle missing keys gracefully.""" tool_call = {"toolUse": {"name": "calculator"}} # missing input and toolUseId result = self.executor._parse_native_tool_call(tool_call) # Verify behavior matches implementation🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/agents/test_repro_4972.py` around lines 10 - 42, Add unit tests to exercise defensive type guards in _parse_native_tool_call by verifying it returns None (and does not raise) for malformed inputs: (1) toolUse as non-dict (e.g., {"toolUse": "not_a_dict"}), (2) None input, (3) empty dict {}, and (4) missing required keys (e.g., {"toolUse": {"name": "calculator"}} lacking toolUseId/input). Place these new test methods alongside existing tests in test_repro_4972.py, assert result is None for each case, and name them clearly (e.g., test_parse_malformed_tooluse_value, test_parse_none_input, test_parse_empty_dict, test_parse_missing_required_keys) to document the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/tests/agents/test_repro_4972.py`:
- Line 2: The test file imports the unused typing symbol Any; remove the unused
import by deleting Any from the import statement (i.e., update the line
importing Any so it no longer includes the Any symbol) to eliminate the
unused-import warning in lib/crewai/tests/agents/test_repro_4972.py.
---
Nitpick comments:
In `@lib/crewai/tests/agents/test_repro_4972.py`:
- Around line 6-8: The test currently creates an uninitialized CrewAgentExecutor
via CrewAgentExecutor.__new__ in setUp; instead, make _parse_native_tool_call
testable without bypassing __init__ by either refactoring
_parse_native_tool_call into a `@staticmethod` or `@classmethod` if it does not use
instance state, or by adding a test-friendly factory or dependency-injection
hook on CrewAgentExecutor so setUp can construct a fully initialized instance;
alternatively, if instance state is required, update setUp to construct a real
CrewAgentExecutor with mocked dependencies (mock whatever __init__ requires)
rather than using __new__, and update tests to call the method on that properly
initialized object.
- Around line 10-42: Add unit tests to exercise defensive type guards in
_parse_native_tool_call by verifying it returns None (and does not raise) for
malformed inputs: (1) toolUse as non-dict (e.g., {"toolUse": "not_a_dict"}), (2)
None input, (3) empty dict {}, and (4) missing required keys (e.g., {"toolUse":
{"name": "calculator"}} lacking toolUseId/input). Place these new test methods
alongside existing tests in test_repro_4972.py, assert result is None for each
case, and name them clearly (e.g., test_parse_malformed_tooluse_value,
test_parse_none_input, test_parse_empty_dict, test_parse_missing_required_keys)
to document the intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dd22aae-2b4b-4ce3-84e9-fbc73bac2698
📒 Files selected for processing (1)
lib/crewai/tests/agents/test_repro_4972.py
1 participant
Closes #4972
Summary
Restored tool calling for AWS Bedrock Converse API by supporting the toolUse nesting level in native tool parsing.
Changes
Summary by CodeRabbit
Bug Fixes
Tests