From 63ddd022a203e48ba0e4617f41c2661f3415e69e Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 9 Jul 2026 02:14:15 +0530 Subject: [PATCH] refactor(salvage): scope #40632 to the two live copy-on-write sites Trim the salvaged commit to its two still-valid conversions: - ChatCompletionsTransport.convert_messages (copy-on-write sanitize) - QwenProfile.prepare_messages (copy-on-write normalize + cache_control) Dropped from the original PR: - agent/prompt_caching.py selective-copy: superseded by #57229 which already rewrites apply_anthropic_cache_control on current main. - Gemma extra_content narrowing (_model_consumes_thought_signature 'gemini or gemma' -> 'gemini' only) + its two tests: unrelated behavior change reverting deliberate e8c3ac2f5; belongs in its own PR with its own justification if pursued. Conflict resolution: preserved main's newer timestamp-stripping (#47868) inside the copy-on-write path. --- agent/prompt_caching.py | 50 ++----------------- agent/transports/chat_completions.py | 6 +-- tests/agent/test_prompt_caching.py | 48 ------------------ .../agent/transports/test_chat_completions.py | 10 ---- tests/run_agent/test_provider_parity.py | 10 ---- 5 files changed, 8 insertions(+), 116 deletions(-) diff --git a/agent/prompt_caching.py b/agent/prompt_caching.py index 34d34143127..9a2fdf4ccce 100644 --- a/agent/prompt_caching.py +++ b/agent/prompt_caching.py @@ -8,6 +8,7 @@ single session. Pure functions -- no class state, no AIAgent dependency. """ +import copy from typing import Any, Dict, List @@ -80,41 +81,6 @@ def _build_marker(ttl: str) -> Dict[str, str]: return marker -def _copy_message_for_api_mutation(msg: Dict[str, Any]) -> Dict[str, Any]: - """Copy message containers that later API-retry recovery may mutate.""" - copied = dict(msg) - - content = msg.get("content") - if isinstance(content, list) and content: - copied_content = [] - for part in content: - if isinstance(part, dict): - copied_part = dict(part) - image_url = part.get("image_url") - if isinstance(image_url, dict): - copied_part["image_url"] = dict(image_url) - copied_content.append(copied_part) - else: - copied_content.append(part) - copied["content"] = copied_content - - tool_calls = msg.get("tool_calls") - if isinstance(tool_calls, list) and tool_calls: - copied_tool_calls = [] - for tool_call in tool_calls: - if isinstance(tool_call, dict): - copied_tool_call = dict(tool_call) - function = tool_call.get("function") - if isinstance(function, dict): - copied_tool_call["function"] = dict(function) - copied_tool_calls.append(copied_tool_call) - else: - copied_tool_calls.append(tool_call) - copied["tool_calls"] = copied_tool_calls - - return copied - - def apply_anthropic_cache_control( api_messages: List[Dict[str, Any]], cache_ttl: str = "5m", @@ -126,17 +92,11 @@ def apply_anthropic_cache_control( messages, all at the same TTL. Returns: - Copy of messages with cache_control breakpoints injected. Message - containers are copied so later API-retry recovery can mutate the - returned request copy without touching canonical history. + Deep copy of messages with cache_control breakpoints injected. """ - if not api_messages: - return [] - - messages = [ - _copy_message_for_api_mutation(msg) if isinstance(msg, dict) else msg - for msg in api_messages - ] + messages = copy.deepcopy(api_messages) + if not messages: + return messages marker = _build_marker(cache_ttl) diff --git a/agent/transports/chat_completions.py b/agent/transports/chat_completions.py index b20445be90a..c768968a0d6 100644 --- a/agent/transports/chat_completions.py +++ b/agent/transports/chat_completions.py @@ -99,7 +99,7 @@ def _is_gemini_openai_compat_base_url(base_url: Any) -> bool: def _model_consumes_thought_signature(model: Any) -> bool: - """True when the outgoing model is a Gemini model that requires + """True when the outgoing model is a Gemini family model that requires ``extra_content`` (thought_signature) to be replayed on tool calls. Gemini 3 thinking models attach ``extra_content`` to each tool call and @@ -111,7 +111,7 @@ def _model_consumes_thought_signature(model: Any) -> bool: ``extra_content`` from earlier in a mixed-provider session. """ m = str(model or "").lower() - return "gemini" in m + return "gemini" in m or "gemma" in m class ChatCompletionsTransport(ProviderTransport): @@ -135,7 +135,7 @@ class ChatCompletionsTransport(ProviderTransport): ``codex_message_items`` on the message, ``call_id`` / ``response_item_id`` on ``tool_calls`` entries. - ``extra_content`` on ``tool_calls`` (Gemini thought_signature) — - stripped unless the outgoing ``model`` is itself Gemini. + stripped unless the outgoing ``model`` is itself Gemini-family. Gemini 3 thinking models attach it for replay, but strict providers (Fireworks, Mistral) reject any payload containing it with ``Extra inputs are not permitted, field: 'messages[N].tool_calls[M].extra_content'``. diff --git a/tests/agent/test_prompt_caching.py b/tests/agent/test_prompt_caching.py index 889b454d4e6..7a1c0495e1b 100644 --- a/tests/agent/test_prompt_caching.py +++ b/tests/agent/test_prompt_caching.py @@ -131,54 +131,6 @@ class TestApplyAnthropicCacheControl: # Original should be unmodified assert "cache_control" not in msgs[0].get("content", "") - def test_request_copy_protects_unmarked_nested_content(self): - msgs = [ - {"role": "system", "content": "System"}, - {"role": "user", "content": [{"type": "text", "text": "msg1"}]}, - {"role": "assistant", "content": [{"type": "text", "text": "msg2"}]}, - {"role": "user", "content": [{"type": "text", "text": "msg3"}]}, - {"role": "assistant", "content": [{"type": "text", "text": "msg4"}]}, - ] - - result = apply_anthropic_cache_control(msgs) - - assert result is not msgs - assert result[0] is not msgs[0] - assert result[1] is not msgs[1] - assert result[1]["content"] is not msgs[1]["content"] - assert result[1]["content"][0] is not msgs[1]["content"][0] - assert result[2] is not msgs[2] - assert result[2]["content"] is not msgs[2]["content"] - assert result[2]["content"][0] is not msgs[2]["content"][0] - assert "cache_control" not in msgs[2]["content"][0] - assert result[2]["content"][0]["cache_control"] == MARKER - - result[1]["content"][0]["text"] = "mutated request copy" - assert msgs[1]["content"][0]["text"] == "msg1" - - def test_request_copy_protects_nested_image_url_retry_mutation(self): - image_url = {"url": "data:image/png;base64,original"} - msgs = [ - {"role": "system", "content": "System"}, - { - "role": "user", - "content": [ - {"type": "text", "text": "see image"}, - {"type": "image_url", "image_url": image_url}, - ], - }, - ] - - result = apply_anthropic_cache_control(msgs) - - assert result[1]["content"][1] is not msgs[1]["content"][1] - assert result[1]["content"][1]["image_url"] is not image_url - - result[1]["content"][1]["image_url"]["url"] = "data:image/png;base64,shrunk" - assert msgs[1]["content"][1]["image_url"]["url"] == ( - "data:image/png;base64,original" - ) - def test_system_message_gets_marker(self): msgs = [ {"role": "system", "content": "You are helpful"}, diff --git a/tests/agent/transports/test_chat_completions.py b/tests/agent/transports/test_chat_completions.py index 7d1b07d2d75..4894c235cd4 100644 --- a/tests/agent/transports/test_chat_completions.py +++ b/tests/agent/transports/test_chat_completions.py @@ -84,16 +84,6 @@ class TestChatCompletionsBasic: "google": {"thought_signature": "SIG_123"} }, model - def test_convert_messages_strips_extra_content_for_gemma(self, transport): - """Gemma can be served by Google, but it is not Gemini and rejects - Gemini-only request fields. Stale thought signatures from a prior - Gemini turn must be stripped when switching to Gemma. - """ - msgs = self._msg_with_extra_content() - result = transport.convert_messages(msgs, model="google/gemma-4-31b-it") - assert "extra_content" not in result[0]["tool_calls"][0] - assert "extra_content" in msgs[0]["tool_calls"][0] - def test_convert_messages_strips_tool_name(self, transport): """Internal `tool_name` (used for FTS indexing in the SQLite store) is not part of the OpenAI Chat Completions schema. Strict providers like diff --git a/tests/run_agent/test_provider_parity.py b/tests/run_agent/test_provider_parity.py index df72bec8c00..8229b0f020d 100644 --- a/tests/run_agent/test_provider_parity.py +++ b/tests/run_agent/test_provider_parity.py @@ -297,16 +297,6 @@ class TestBuildApiKwargsOpenRouter: # call_id/response_item_id still stripped regardless of model assert "call_id" not in result["tool_calls"][0] - def test_sanitize_tool_calls_strips_extra_content_for_gemma(self, monkeypatch): - """Gemma is served by Google but does not consume Gemini thought signatures.""" - agent = _make_agent(monkeypatch, "openrouter") - api_msg = self._api_msg_with_extra_content() - result = agent._sanitize_tool_calls_for_strict_api( - api_msg, model="google/gemma-4-31b-it" - ) - assert "extra_content" not in result["tool_calls"][0] - assert "call_id" not in result["tool_calls"][0] - class TestDeveloperRoleSwap: """GPT-5 and Codex models should get 'developer' instead of 'system' role."""