fix: feed tool results to open ai compat providers as role tool with id - #1505
fix: feed tool results to open ai compat providers as role tool with id#1505AngeloDanducci wants to merge 2 commits into
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
| tool_call = getattr(msg, "_tool", None) | ||
| if tool_call is not None and getattr(tool_call, "tool_call_id", None) is not None: | ||
| result["tool_call_id"] = tool_call.tool_call_id | ||
| name = getattr(msg, "name", None) |
There was a problem hiding this comment.
If name isn't part of the openai spec, will it cause any issues when passed? Or does it get silently ignored?
There was a problem hiding this comment.
It doesn't collide and extraneous fields should be dropped, but it's legacy and not required.
…-result name Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
| # `role: "tool"` message to reference the assistant's originating tool call | ||
| # via `tool_call_id` (issue #1389). Emit it when the ToolMessage carries a | ||
| # provider-supplied id. Duck-type on `_tool` to avoid importing ToolMessage | ||
| # (circular import) and to leave plain `role="tool"` Messages, and ids-less |
There was a problem hiding this comment.
The circular-import claim appears incorrect — I tested a module-level from mellea.stdlib.components import ToolMessage and confirmed all entry points (mellea, cli/, every backend) import cleanly. The cycle is already broken by chat.py:180's deferred import of build_tool_calls. A deferred import + isinstance inside this function would be a clean alternative (this file already does that pattern at lines 64–65).
| @@ -415,8 +431,12 @@ def build_tool_calls(output: ModelOutputThunk) -> list[ToolCallDict] | None: | |||
| assert output.tool_calls is not None | |||
| tool_calls: list[ToolCallDict] = [] | |||
| for model_tool_call in output.tool_calls: | |||
There was a problem hiding this comment.
The comment says the reuse exists so "the replayed assistant turn and its tool-result turn reference the same call via _tool.tool_call_id". On the openai/litellm paths, the assistant turn's tool calls come from the provider payload (chat.py:203), not from build_tool_calls. The live case is cli/serve/{app,streaming} forwarding upstream ids downstream. Worth adjusting the comment to match the actual call path.
| # required to satisfy the result-turn contract. | ||
| tool_call = getattr(msg, "_tool", None) | ||
| if tool_call is not None and getattr(tool_call, "tool_call_id", None) is not None: | ||
| result["tool_call_id"] = tool_call.tool_call_id |
There was a problem hiding this comment.
Minor: this gates on is not None while build_tool_calls (line 439) uses truthiness (or). Currently no backend returns "" for tool_call_id, but aligning the checks adds a tiny bit more defense in depth.
3 participants
Pull Request
Issue
Fixes #1389
Description
Feed tool results back to OpenAI-compatible providers as role: "tool" messages with a matching tool_call_id, instead of as user text.
Spec-strict providers may still mishandle the result prior to this change (as a string).
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.