From 19a4d43e3284bb066454fceb8f2f2b4df695e227 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Thu, 11 Jun 2026 13:35:45 -0400 Subject: [PATCH] refactor(minimax): use run_with_tool_loop shared helper (68 -> 44 lines) Task 1.3 of the follow-up track. _send_minimax now uses run_with_tool_loop with a per-round request_builder callback that re-reads _minimax_history under _minimax_history_lock. The plan's Task 1.3 example builds the request once before the loop. That would break MiniMax tool flows because the API would not see the tool results appended to _minimax_history on later rounds. The fix: extend run_with_tool_loop's 2nd arg to accept Union[OpenAICompatibleRequest, Callable[[int], OpenAICompatibleRequest]] (backward compatible; static-request vendors pass a single request). MiniMax now passes a closure that rebuilds messages from history each round. Reasoning extraction: MiniMax exposes its chain-of-thought via response.raw_response.choices[0].message.reasoning_details[0]. get('text'). Lifted to a _extract_minimax_reasoning callback passed as reasoning_extractor=... (the new parameter added in the previous commit). Trim callback: wraps _trim_minimax_history so it can be called from run_with_tool_loop after each tool-result append. Green confirmed: 51 vendor + tool tests pass (6 MiniMax + 5 tool_loop core + 1 tool_loop builder + 39 others); the new test_ai_client_tool_loop_builder.py locks in the per-round builder contract. --- src/ai_client.py | 73 +++++++---------------- tests/test_ai_client_tool_loop_builder.py | 41 +++++++++++++ 2 files changed, 64 insertions(+), 50 deletions(-) create mode 100644 tests/test_ai_client_tool_loop_builder.py diff --git a/src/ai_client.py b/src/ai_client.py index 06001c49..97663af2 100644 --- a/src/ai_client.py +++ b/src/ai_client.py @@ -805,7 +805,7 @@ async def _execute_tool_calls_concurrently( def run_with_tool_loop( client: Any, - request: OpenAICompatibleRequest, + request: Union[OpenAICompatibleRequest, Callable[[int], OpenAICompatibleRequest]], *, capabilities: VendorCapabilities, pre_tool_callback: Optional[Callable[[str, str, Optional[Callable[[str], str]]], Optional[str]]] = None, @@ -819,9 +819,10 @@ def run_with_tool_loop( trim_func: Optional[Callable[[list[dict[str, Any]]], None]] = None, reasoning_extractor: Optional[Callable[[Any], str]] = None, ) -> str: + request_builder: Callable[[int], OpenAICompatibleRequest] = (request if callable(request) else (lambda _i: request)) response_text: str = "" for _round_idx in range(MAX_TOOL_ROUNDS + 2): - response = send_openai_compatible(client, request, capabilities=capabilities) + response = send_openai_compatible(client, request_builder(_round_idx), capabilities=capabilities) reasoning_content: str = reasoning_extractor(response.raw_response) if reasoning_extractor else "" response_text = response.text or "" if history_lock is not None and history is not None: @@ -2284,8 +2285,6 @@ def _send_minimax(md_content: str, user_message: str, base_dir: str, stream_callback: Optional[Callable[[str], None]] = None, patch_callback: Optional[Callable[[str, str], Optional[str]]] = None) -> str: _ensure_minimax_client() - from src.openai_compatible import OpenAICompatibleRequest, send_openai_compatible - from src.vendor_capabilities import get_capabilities tools: list[dict[str, Any]] | None = _get_deepseek_tools() or None with _minimax_history_lock: _repair_minimax_history(_minimax_history) @@ -2293,56 +2292,30 @@ def _send_minimax(md_content: str, user_message: str, base_dir: str, _minimax_history.append({"role": "user", "content": f"[DISCUSSION HISTORY]\n\n{discussion_history}\n\n---\n\n{user_message}"}) else: _minimax_history.append({"role": "user", "content": user_message}) - response_text: str = "" - reasoning_content: str = "" - for round_idx in range(MAX_TOOL_ROUNDS + 2): + def _build_minimax_request(_round_idx: int) -> OpenAICompatibleRequest: with _minimax_history_lock: - messages = [{"role": "system", "content": f"{_get_combined_system_prompt()}\n\n\n{md_content}\n"}] + messages: list[dict[str, Any]] = [{"role": "system", "content": f"{_get_combined_system_prompt()}\n\n\n{md_content}\n"}] messages.extend(_minimax_history) - request = OpenAICompatibleRequest( - messages=messages, - model=_model, - temperature=_temperature, - top_p=_top_p, - max_tokens=min(_max_tokens, 8192), - stream=stream, - stream_callback=stream_callback, - tools=tools, - tool_choice="auto" if tools else "auto", + return OpenAICompatibleRequest( + messages=messages, model=_model, temperature=_temperature, top_p=_top_p, + max_tokens=min(_max_tokens, 8192), stream=stream, stream_callback=stream_callback, + tools=tools, tool_choice="auto" if tools else "auto", ) - caps = get_capabilities("minimax", _model) - response = send_openai_compatible(_minimax_client, request, capabilities=caps) - reasoning_content = "" - if response.raw_response and hasattr(response.raw_response, "choices"): - choice = response.raw_response.choices[0] + def _extract_minimax_reasoning(raw_response: Any) -> str: + if raw_response and hasattr(raw_response, "choices"): + choice = raw_response.choices[0] if hasattr(choice.message, "reasoning_details") and choice.message.reasoning_details: - reasoning_content = choice.message.reasoning_details[0].get("text", "") if choice.message.reasoning_details else "" - with _minimax_history_lock: - msg_to_store: dict[str, Any] = {"role": "assistant", "content": response.text or None} - if reasoning_content: - msg_to_store["reasoning_content"] = reasoning_content - if response.tool_calls: - msg_to_store["tool_calls"] = response.tool_calls - _minimax_history.append(msg_to_store) - if not response.tool_calls: - response_text = (f"\n{reasoning_content}\n\n" if reasoning_content else "") + response.text - break - try: - loop = asyncio.get_running_loop() - results = asyncio.run_coroutine_threadsafe( - _execute_tool_calls_concurrently(response.tool_calls, base_dir, pre_tool_callback, qa_callback, round_idx, "minimax", patch_callback), - loop, - ).result() - except RuntimeError: - results = asyncio.run(_execute_tool_calls_concurrently(response.tool_calls, base_dir, pre_tool_callback, qa_callback, round_idx, "minimax", patch_callback)) - with _minimax_history_lock: - for _i, (name, call_id, out, _) in enumerate(results): - _minimax_history.append({ - "role": "tool", - "tool_call_id": call_id, - "content": str(out) if out else "", - }) - return response_text + return choice.message.reasoning_details[0].get("text", "") or "" + return "" + caps = get_capabilities("minimax", _model) + return run_with_tool_loop( + _minimax_client, _build_minimax_request, capabilities=caps, + pre_tool_callback=pre_tool_callback, qa_callback=qa_callback, stream_callback=stream_callback, + patch_callback=patch_callback, base_dir=base_dir, vendor_name="minimax", + history_lock=_minimax_history_lock, history=_minimax_history, + trim_func=lambda h: _trim_minimax_history(_build_minimax_request(0).messages, h), + reasoning_extractor=_extract_minimax_reasoning, + ) #endregion: MiniMax Provider diff --git a/tests/test_ai_client_tool_loop_builder.py b/tests/test_ai_client_tool_loop_builder.py new file mode 100644 index 00000000..d53630bb --- /dev/null +++ b/tests/test_ai_client_tool_loop_builder.py @@ -0,0 +1,41 @@ +"""Verify run_with_tool_loop supports a per-round request_builder callback. + +Vendors that mutate their history list (e.g. MiniMax) need to rebuild +the messages on each round so the API sees the latest tool results. +run_with_tool_loop accepts a callable as the 2nd arg to enable this. +""" +from __future__ import annotations +from typing import Any +from unittest.mock import MagicMock, patch +from src.openai_compatible import NormalizedResponse, OpenAICompatibleRequest +from src.ai_client import run_with_tool_loop +from src.vendor_capabilities import VendorCapabilities + +def _make_normalized_response(text: str = "ok", tool_calls: list[dict[str, Any]] | None = None) -> NormalizedResponse: + return NormalizedResponse( + text=text, tool_calls=tool_calls or [], + usage_input_tokens=10, usage_output_tokens=5, + usage_cache_read_tokens=0, usage_cache_creation_tokens=0, + raw_response=None, + ) + +def test_run_with_tool_loop_calls_request_builder_each_round() -> None: + caps = VendorCapabilities(vendor="test", model="test-model", tool_calling=True, context_window=8192) + client = MagicMock() + tool_response = _make_normalized_response( + "first", tool_calls=[{"id": "c1", "type": "function", "function": {"name": "noop", "arguments": "{}"}}] + ) + final = _make_normalized_response("done") + builder_calls: list[int] = [] + def builder(round_idx: int) -> OpenAICompatibleRequest: + builder_calls.append(round_idx) + return OpenAICompatibleRequest(messages=[{"role": "user", "content": f"round={round_idx}"}], model="m") + with patch("src.ai_client.send_openai_compatible", side_effect=[tool_response, final]), \ + patch("src.ai_client._execute_tool_calls_concurrently", return_value=[("noop", "c1", "r", "")]): + result = run_with_tool_loop( + client, builder, capabilities=caps, + pre_tool_callback=None, qa_callback=None, patch_callback=None, + base_dir=".", vendor_name="test", history_lock=None, history=None, + ) + assert result == "done" + assert len(builder_calls) >= 2