sangkyoonnam commented on code in PR #1185:
URL: https://github.com/apache/flink-agents/pull/1185#discussion_r4236824761
##########
python/flink_agents/plan/actions/tool_call_action.py:
##########
@@ -403,15 +398,15 @@ def _record_tool_response(
success: dict,
error: dict,
) -> None:
- response = value if isinstance(value, ToolResponse) else
ToolResponse.success(value)
+ response = to_tool_response(value)
Review Comment:
`to_tool_response` can now raise here, and in parallel mode that escapes
`_record_outcome` into the batch-wide `except` in `_execute_parallel`, which
marks every call in the batch as failed. With two parallel calls where one
returns `"ok"` and the other returns a `date`:
```
parallel success={'call-1': False, 'call-2': False}
serial success={'call-1': True, 'call-2': False}
```
Rejecting the `date` is expected. Both calls get the conversion diagnostic
in `ToolResponseEvent.error` and a generic tool-failure response, so the model
is told the `"ok"` call failed too. `_report_execution` still receives the
original outcomes and reports both as successful. Catching the conversion error
per call, and passing a failed outcome to `_report_execution` for that call,
would keep parallel and serial consistent. A mixed valid/invalid parallel test
could check both the responses and the execution reporting.
##########
python/flink_agents/plan/actions/tool_result_utils.py:
##########
@@ -34,6 +34,24 @@
from pydantic import TypeAdapter
+from flink_agents.api.tools import ToolResponse
+
+
+def to_tool_response(value: Any) -> ToolResponse:
+ """Convert ordinary tool returns to text, preserving explicit responses."""
+ if isinstance(value, ToolResponse):
+ return value
+ try:
+ text = (
+ value
+ if isinstance(value, str)
+ else json.dumps(value, ensure_ascii=False, allow_nan=False)
+ )
+ except (TypeError, ValueError) as error:
Review Comment:
This also rejects results from the built-in MCP tool.
`extract_mcp_content_item` returns the resource `uri` as a pydantic `AnyUrl`
for `EmbeddedResource` (and `ResourceLink`), so `json.dumps` fails:
```
Tool return value requires JSON data or an explicit ToolResponse
cause: TypeError('Object of type AnyUrl is not JSON serializable')
```
On main, the same value went through `ToolResponse.success(value)`. In
`mcp/utils.py`, stringifying the two embedded-resource `uri` fields and
switching the generic fallback to `model_dump(mode="json")` would cover
`ResourceLink` too. The Java `MCPTool` already JSON-serializes its result into
`ToolResponse.text(...)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
