fix: Add output item processing for tool results#10348
Conversation
Introduced process_output_item to handle tool output items, attempting to parse text-type items as JSON. This improves downstream handling of tool outputs by converting JSON strings to dictionaries when possible.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds JSON parsing capability to MCPToolsComponent.build_output method. Text-type tool outputs are parsed as JSON when possible through a new process_output_item method. Includes early return path when all output items are dictionaries. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (3 passed)
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 |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (39.40%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10348 +/- ##
==========================================
- Coverage 30.16% 30.15% -0.01%
==========================================
Files 1318 1318
Lines 59680 59680
Branches 8925 8925
==========================================
- Hits 18003 17999 -4
- Misses 40846 40849 +3
- Partials 831 832 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lfx/src/lfx/components/agents/mcp_component.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lfx/src/lfx/components/agents/mcp_component.py (1)
src/lfx/src/lfx/schema/message.py (1)
json(312-314)
🪛 GitHub Actions: Ruff Style Check
src/lfx/src/lfx/components/agents/mcp_component.py
[error] 544-544: Command: uv run --only-dev ruff check --output-format=github . | ruff: TRY300: Consider moving this statement to an 'else' block.
🪛 GitHub Check: Ruff Style Check (3.13)
src/lfx/src/lfx/components/agents/mcp_component.py
[failure] 545-545: Ruff (BLE001)
src/lfx/src/lfx/components/agents/mcp_component.py:545:20: BLE001 Do not catch blind exception: Exception
[failure] 544-544: Ruff (RET504)
src/lfx/src/lfx/components/agents/mcp_component.py:544:24: RET504 Unnecessary assignment to json_dict before return statement
[failure] 544-544: Ruff (TRY300)
src/lfx/src/lfx/components/agents/mcp_component.py:544:17: TRY300 Consider moving this statement to an else block
⏰ 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). (1)
- GitHub Check: Update Starter Projects
🔇 Additional comments (1)
src/lfx/src/lfx/components/agents/mcp_component.py (1)
4-4: LGTM!The
jsonimport is necessary for the new JSON parsing functionality inprocess_output_item.
| item_dict = self.process_output_item(item_dict) | ||
| tool_content.append(item_dict) | ||
|
|
||
| if isinstance(tool_content, list) and all(isinstance(x, dict) for x in tool_content): | ||
| return DataFrame(tool_content) | ||
| return DataFrame(data=tool_content) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the DataFrame constructor behavior and simplify the list check.
The early return logic attempts to handle cases where parsed JSON might not be a dict. However, there are a couple of concerns:
-
Redundant list check:
tool_contentis always a list (built viaappendin the loop), soisinstance(tool_content, list)is alwaysTrue. This check can be simplified. -
Unclear difference: What's the semantic difference between
DataFrame(tool_content)(line 529) andDataFrame(data=tool_content)(line 530)? Without seeing the DataFrame implementation, it's unclear if these behave differently or are equivalent.
Run the following script to check the DataFrame constructor signature:
Consider simplifying the check:
- if isinstance(tool_content, list) and all(isinstance(x, dict) for x in tool_content):
+ if all(isinstance(x, dict) for x in tool_content):
return DataFrame(tool_content)
return DataFrame(data=tool_content)🏁 Script executed:
#!/bin/bash
# Description: Examine DataFrame constructor to understand the difference between positional and keyword argument
# Search for DataFrame class definition and __init__ method
ast-grep --pattern $'class DataFrame:
$$$
def __init__($$$):
$$$'
# Also search for DataFrame __init__ with different patterns
rg -n "class DataFrame" -A 30 src/lfx/src/lfx/schema/dataframe.pyLength of output: 1292
Remove the redundant list check; both DataFrame calls are equivalent.
The DataFrame constructor accepts data as its first parameter, so DataFrame(tool_content) and DataFrame(data=tool_content) behave identically. Since tool_content is always a list (built via append() in the loop), the isinstance(tool_content, list) check is redundant.
Simplify to:
- if isinstance(tool_content, list) and all(isinstance(x, dict) for x in tool_content):
+ if all(isinstance(x, dict) for x in tool_content):
return DataFrame(tool_content)
return DataFrame(data=tool_content)Or further simplify both lines to use consistent syntax:
- if isinstance(tool_content, list) and all(isinstance(x, dict) for x in tool_content):
- return DataFrame(tool_content)
- return DataFrame(data=tool_content)
+ if all(isinstance(x, dict) for x in tool_content):
+ return DataFrame(data=tool_content)
+ return DataFrame(data=tool_content)which reduces to:
- if isinstance(tool_content, list) and all(isinstance(x, dict) for x in tool_content):
- return DataFrame(tool_content)
- return DataFrame(data=tool_content)
+ return DataFrame(data=tool_content)📝 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.
| item_dict = self.process_output_item(item_dict) | |
| tool_content.append(item_dict) | |
| if isinstance(tool_content, list) and all(isinstance(x, dict) for x in tool_content): | |
| return DataFrame(tool_content) | |
| return DataFrame(data=tool_content) | |
| item_dict = self.process_output_item(item_dict) | |
| tool_content.append(item_dict) | |
| if all(isinstance(x, dict) for x in tool_content): | |
| return DataFrame(tool_content) | |
| return DataFrame(data=tool_content) |
🤖 Prompt for AI Agents
In src/lfx/src/lfx/components/agents/mcp_component.py around lines 525 to 530,
the code redundantly checks isinstance(tool_content, list) before constructing a
DataFrame even though tool_content is always a list built via append(); replace
the conditional block with a single consistent DataFrame construction (e.g.,
return DataFrame(tool_content) or return DataFrame(data=tool_content)) and
remove the unnecessary isinstance/all checks so the function returns the
DataFrame directly.
| def process_output_item(self, item_dict): | ||
| """Process the output of a tool.""" | ||
| if item_dict.get("type") == "text": | ||
| text = item_dict.get("text") | ||
| try: | ||
| json_dict = json.loads(text) | ||
| # convert it to dict | ||
| return json_dict | ||
| except Exception: | ||
| return item_dict | ||
| return item_dict |
There was a problem hiding this comment.
Fix static analysis issues and improve exception handling.
The method has several issues flagged by static analysis:
- Bare exception catch (BLE001): Catching
Exceptionis too broad and can hide unexpected errors. - Unnecessary assignment (RET504): The
json_dictvariable is assigned and immediately returned. - Should use else block (TRY300): The successful parse should be in an
elseblock.
Additionally, note that when JSON parsing succeeds, all metadata from item_dict (like the "type" field) is discarded. If this is intentional, consider documenting it.
Apply this diff to fix the static analysis issues:
def process_output_item(self, item_dict):
- """Process the output of a tool."""
+ """Process tool output item, parsing text-type items as JSON when possible.
+
+ Returns the parsed JSON object for valid JSON text, otherwise returns the original item_dict.
+ Note: Successful JSON parsing discards the original item_dict metadata.
+ """
if item_dict.get("type") == "text":
text = item_dict.get("text")
try:
- json_dict = json.loads(text)
- # convert it to dict
- return json_dict
- except Exception:
+ return json.loads(text)
+ except json.JSONDecodeError:
return item_dict
+ else:
+ # This else is unreachable due to the return in try, but satisfies TRY300
+ pass
return item_dictWait, the else block approach doesn't work with the return in try. Better approach:
def process_output_item(self, item_dict):
- """Process the output of a tool."""
+ """Process tool output item, parsing text-type items as JSON when possible.
+
+ Returns the parsed JSON object for valid JSON text, otherwise returns the original item_dict.
+ Note: Successful JSON parsing discards the original item_dict metadata.
+ """
if item_dict.get("type") == "text":
text = item_dict.get("text")
try:
- json_dict = json.loads(text)
- # convert it to dict
- return json_dict
- except Exception:
- return item_dict
+ return json.loads(text)
+ except json.JSONDecodeError:
+ pass
- return item_dict
+ return item_dict📝 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.
| def process_output_item(self, item_dict): | |
| """Process the output of a tool.""" | |
| if item_dict.get("type") == "text": | |
| text = item_dict.get("text") | |
| try: | |
| json_dict = json.loads(text) | |
| # convert it to dict | |
| return json_dict | |
| except Exception: | |
| return item_dict | |
| return item_dict | |
| def process_output_item(self, item_dict): | |
| """Process tool output item, parsing text-type items as JSON when possible. | |
| Returns the parsed JSON object for valid JSON text, otherwise returns the original item_dict. | |
| Note: Successful JSON parsing discards the original item_dict metadata. | |
| """ | |
| if item_dict.get("type") == "text": | |
| text = item_dict.get("text") | |
| try: | |
| return json.loads(text) | |
| except json.JSONDecodeError: | |
| pass | |
| return item_dict |
🧰 Tools
🪛 GitHub Actions: Ruff Style Check
[error] 544-544: Command: uv run --only-dev ruff check --output-format=github . | ruff: TRY300: Consider moving this statement to an 'else' block.
🪛 GitHub Check: Ruff Style Check (3.13)
[failure] 545-545: Ruff (BLE001)
src/lfx/src/lfx/components/agents/mcp_component.py:545:20: BLE001 Do not catch blind exception: Exception
[failure] 544-544: Ruff (RET504)
src/lfx/src/lfx/components/agents/mcp_component.py:544:24: RET504 Unnecessary assignment to json_dict before return statement
[failure] 544-544: Ruff (TRY300)
src/lfx/src/lfx/components/agents/mcp_component.py:544:17: TRY300 Consider moving this statement to an else block
github-actions
Bot
added
bug
Adam-Aghili
left a comment
Adam-Aghili
left a comment
There was a problem hiding this comment.
Didn't go through the JSON changes but other then the build failures LGTM!
thank you @edwinjosechittilappilly
Simplifies JSON parsing by returning the result directly and catching only JSONDecodeError. This improves error handling and code clarity.
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
* Add output item processing for tool results Introduced process_output_item to handle tool output items, attempting to parse text-type items as JSON. This improves downstream handling of tool outputs by converting JSON strings to dictionaries when possible. * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Refactor JSON parsing in MCPToolsComponent Simplifies JSON parsing by returning the result directly and catching only JSONDecodeError. This improves error handling and code clarity. * Update component_index.json * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Introduced process_output_item to handle tool output items, attempting to parse text-type items as JSON. This improves downstream handling of tool outputs by converting JSON strings to dictionaries when possible.
Summary by CodeRabbit