-
Notifications
You must be signed in to change notification settings - Fork 47
fix(runner): server tool results, mixed-tool execution, thought_signature passthrough #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
288b70e
ad7379b
77e5958
59350e3
a8a9685
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
|
|
||
| import asyncio | ||
| import inspect | ||
| import json | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| Any, | ||
|
|
@@ -685,6 +686,7 @@ async def _execute_streaming_async( | |
| content_chunks = 0 | ||
| tool_call_chunks = 0 | ||
| finish_reason = None | ||
| mcp_tool_results_from_server: list = [] | ||
| async for chunk in stream: | ||
| chunk_count += 1 | ||
| if exec_config.verbose: | ||
|
|
@@ -694,6 +696,12 @@ async def _execute_streaming_async( | |
| meta = extra.get("x_dedalus_event") or extra.get("dedalus_event") | ||
| if isinstance(meta, dict) and meta.get("type") == "agent_updated": | ||
| print(f" [EVENT] agent_updated: agent={meta.get('agent')} model={meta.get('model')}") | ||
|
|
||
| # Collect MCP tool results emitted by the server | ||
| chunk_extra = getattr(chunk, "__pydantic_extra__", None) or {} | ||
| if isinstance(chunk_extra, dict) and "mcp_tool_results" in chunk_extra: | ||
| mcp_tool_results_from_server = chunk_extra["mcp_tool_results"] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MCP tool results overwritten instead of accumulatedLow Severity When collecting MCP tool results from server chunks, the code uses assignment ( 🔬 Verification TestWhy verification test was not possible: This potential edge case depends on the server's behavior when streaming MCP tool results. Without access to the actual server implementation or mock infrastructure that simulates multi-chunk MCP result delivery, it's not possible to verify whether results would be lost in practice. The bug is flagged based on the code pattern showing assignment rather than accumulation in a streaming context. Additional Locations (1)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Server tool results collected but never injected into messagesHigh Severity The variable 🔬 Verification TestWhy verification test was not possible: The VM infrastructure was unreachable during this review (repeated "Pod exists but exec-daemon is unreachable" errors across 30+ attempts). However, this bug is clearly identifiable through static analysis of the diff alone - the variable Additional Locations (1) |
||
|
|
||
| if hasattr(chunk, "choices") and chunk.choices: | ||
| choice = chunk.choices[0] | ||
| delta = choice.delta | ||
|
|
@@ -765,7 +773,6 @@ async def _execute_streaming_async( | |
| local_only = [ | ||
| tc for tc in tool_calls if tc["function"]["name"] in getattr(tool_handler, "_funcs", {}) | ||
| ] | ||
| messages.append({"role": "assistant", "tool_calls": local_only}) | ||
|
|
||
| from ._scheduler import execute_local_tools_async | ||
|
|
||
|
|
@@ -965,9 +972,16 @@ def _execute_streaming_sync( | |
| tool_call_chunks = 0 | ||
| finish_reason = None | ||
| accumulated_content = "" | ||
| mcp_tool_results_from_server: list = [] | ||
|
|
||
| for chunk in stream: | ||
| chunk_count += 1 | ||
|
|
||
| # Collect MCP tool results emitted by the server | ||
| chunk_extra = getattr(chunk, "__pydantic_extra__", None) or {} | ||
| if isinstance(chunk_extra, dict) and "mcp_tool_results" in chunk_extra: | ||
| mcp_tool_results_from_server = chunk_extra["mcp_tool_results"] | ||
|
|
||
| if hasattr(chunk, "choices") and chunk.choices: | ||
| choice = chunk.choices[0] | ||
| delta = choice.delta | ||
|
|
@@ -1048,7 +1062,6 @@ def _execute_streaming_sync( | |
| local_only = [ | ||
| tc for tc in tool_calls if tc["function"]["name"] in getattr(tool_handler, "_funcs", {}) | ||
| ] | ||
| messages.append({"role": "assistant", "tool_calls": local_only}) | ||
|
|
||
| from ._scheduler import execute_local_tools_sync | ||
|
|
||
|
|
@@ -1153,16 +1166,18 @@ def _extract_tool_calls(self, choice) -> list[ToolCall]: | |
| fn = tc_dict.get("function", {}) | ||
| fn_dict = vars(fn) if hasattr(fn, "__dict__") else fn | ||
|
|
||
| calls.append( | ||
| { | ||
| "id": tc_dict.get("id", ""), | ||
| "type": tc_dict.get("type", "function"), | ||
| "function": { | ||
| "name": fn_dict.get("name", ""), | ||
| "arguments": fn_dict.get("arguments", "{}"), | ||
| }, | ||
| } | ||
| ) | ||
| tc_out: ToolCall = { | ||
| "id": tc_dict.get("id", ""), | ||
| "type": tc_dict.get("type", "function"), | ||
| "function": { | ||
| "name": fn_dict.get("name", ""), | ||
| "arguments": fn_dict.get("arguments", "{}"), | ||
| }, | ||
| } | ||
| thought_sig = tc_dict.get("thought_signature") | ||
| if thought_sig: | ||
| tc_out["thought_signature"] = thought_sig | ||
| calls.append(tc_out) | ||
| return calls | ||
|
|
||
| async def _execute_tool_calls( | ||
|
|
@@ -1245,6 +1260,9 @@ def _accumulate_tool_calls(self, deltas, acc: list[ToolCall]) -> None: | |
| acc[index]["function"]["name"] = fn.name | ||
| if hasattr(fn, "arguments") and fn.arguments: | ||
| acc[index]["function"]["arguments"] += fn.arguments | ||
| thought_sig = getattr(delta, "thought_signature", None) | ||
| if thought_sig: | ||
| acc[index]["thought_signature"] = thought_sig | ||
|
|
||
| @staticmethod | ||
| def _mk_kwargs(mc: _ModelConfig) -> Dict[str, Any]: | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused json import added to module
Low Severity
The
import jsonstatement was added but thejsonmodule is not used anywhere in the changed code. This appears to be a leftover from planned code that was not implemented, likely related to the incompletemcp_tool_results_from_serverfeature which may have intended to serialize results using JSON.🔬 Verification Test
Why verification test was not possible: The VM infrastructure was unreachable during this review. However, this issue is verifiable through static analysis - the string "json." does not appear anywhere in the diff's changed code, confirming the import is unused in the new code being added.