From e60ca1c6cae714b26ced24b9f5d018da4d1ec6ca Mon Sep 17 00:00:00 2001 From: Gr1mmJ4w Date: Fri, 7 Aug 2026 01:10:19 +0300 Subject: [PATCH] fix(agent): stop the send-path repair from rewriting persisted history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_canonicalize_api_tool_calls` promises copy-on-write in its own docstring — "the persisted history is untouched" — and the call site repeats it: "Operates on api_messages (the API copy) so the original conversation history in `messages` is untouched." The canonicalize branch keeps that promise (`tc = {**tc, "function": {...}}`). The repair branch does not: except Exception: tc["function"]["arguments"] = _repair_tool_call_arguments(...) `api_messages` is built with `msg.copy()` — a SHALLOW per-message copy — so every `tool_calls` entry is the same dict object the persisted history holds. Assigning into `tc["function"]` therefore writes through to the stored turn. The sibling loop two lines above only touches `am["content"]`, one level deep, which is why the aliasing never showed up there. On the unrepairable path `_repair_tool_call_arguments` returns "{}", so that write replaces the model's real arguments with an empty object in the transcript. A stream that dies mid `write_file` loses the file content it had already streamed — the reported symptom in #80498, where a chapter draft was silently reduced to `{}` and only a WARNING remained: Unrepairable tool_call arguments for write_file — replaced with empty object (was: {"content": "# 骨架-第25章\n> 承接...) Mirror the canonicalize branch: build a new tool-call dict instead of assigning into the shared one. The API copy still carries "{}" — the repair's whole purpose is to never ship broken JSON — but the history keeps what the model actually sent, so the transcript, session persistence and any later retry still have it. The in-place write was not an oversight in isolation: it predates the memo refactor, which preserved it deliberately for byte-parity. The existing `test_history_not_mutated` asserts exactly this invariant but restricts itself to valid arguments, and its docstring records the gap — "(Malformed args take the in-place repair path — pre-existing behavior)". That is why a test file whose header already claims "the persisted history is never mutated (copy-on-write preserved)" stayed green through the bug. Four tests close it: history keeps the original bytes, the send copy is still repaired, a broken call does not disturb its siblings, and repeated sends stay lossless. On unpatched main three of them fail; the parity and complexity tests are unaffected because the difference is only observable when the history list is separate from the send copy — which is the shape production uses. Refs #80498 Co-Authored-By: Claude Opus 5 --- agent/conversation_loop.py | 21 ++++- tests/agent/test_canon_args_memo_parity.py | 91 ++++++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index f2f48f53f4..4dfe8bd1f6 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -821,10 +821,23 @@ def _canonicalize_api_tool_calls(api_messages) -> None: ), }} except Exception: - tc["function"]["arguments"] = _repair_tool_call_arguments( - tc["function"]["arguments"], - tc["function"].get("name", "?"), - ) + # Copy-on-write here too. ``api_messages`` holds shallow + # per-message copies (``msg.copy()`` at the send-path + # build), so the tool_call dicts are the SAME objects as + # the persisted history's — assigning into + # ``tc["function"]`` rewrites the stored turn. On the + # unrepairable path the repair returns "{}", so that + # in-place write replaced the model's real arguments with + # an empty object in the transcript: a stream that died + # mid ``write_file`` lost the file content it had already + # streamed, with only a WARNING to show for it (#80498). + tc = {**tc, "function": { + **tc["function"], + "arguments": _repair_tool_call_arguments( + tc["function"]["arguments"], + tc["function"].get("name", "?"), + ), + }} new_tcs.append(tc) am["tool_calls"] = new_tcs diff --git a/tests/agent/test_canon_args_memo_parity.py b/tests/agent/test_canon_args_memo_parity.py index 827c58eeae..91519d0676 100644 --- a/tests/agent/test_canon_args_memo_parity.py +++ b/tests/agent/test_canon_args_memo_parity.py @@ -237,3 +237,94 @@ class TestComplexityProof: # quadratic -> linear, by exact call count assert old_counter[0] == (n + 1) / 2 * new_counter[0] + + +class TestUnrepairableArgsAreNotWrittenBackToHistory: + """The repair path must be copy-on-write too (#80498). + + ``api_messages`` is built with ``msg.copy()`` — a SHALLOW per-message + copy — so every ``tool_calls`` entry is the same dict object the + persisted history holds. The canonicalize branch has always honoured + that (``test_history_not_mutated``), but the repair branch assigned + straight into ``tc["function"]``, so an unrepairable argument string + (repair returns ``"{}"``) overwrote the model's real arguments in the + stored turn. + + Field report: a stream died mid ``write_file`` and the file content it + had already streamed was replaced by ``{}`` in the transcript, leaving + only a WARNING behind. + """ + + @staticmethod + def _history_with_truncated_write(): + # Exactly the incident shape: arguments cut off mid-string. + truncated = '{"content": "# chapter draft\nline one\nline two' + history = [{ + "role": "assistant", + "content": "", + "tool_calls": [{ + "id": "call_1", + "type": "function", + "function": {"name": "write_file", "arguments": truncated}, + }], + }] + return history, truncated + + def test_history_keeps_the_original_arguments(self): + history, truncated = self._history_with_truncated_write() + before = copy.deepcopy(history) + + api_messages = [dict(m) for m in history] # shallow, like the send path + cl._canonicalize_api_tool_calls(api_messages) + + assert history == before, ( + "the send-path canonicalizer rewrote the persisted history" + ) + assert ( + history[0]["tool_calls"][0]["function"]["arguments"] == truncated + ), "the model's streamed arguments were destroyed in the transcript" + + def test_send_copy_is_still_repaired(self): + """The API copy must still carry safe JSON — only the aliasing changes.""" + history, _ = self._history_with_truncated_write() + + api_messages = [dict(m) for m in history] + cl._canonicalize_api_tool_calls(api_messages) + + sent = api_messages[0]["tool_calls"][0]["function"]["arguments"] + assert sent == "{}" + json.loads(sent) # the whole point of the repair: never ship broken JSON + + def test_valid_calls_alongside_a_broken_one_are_untouched(self): + """A broken call must not disturb its siblings' history entries.""" + good = json.dumps({"path": "a.txt", "u": UNI}) + history = [{ + "role": "assistant", + "content": "", + "tool_calls": [ + {"id": "c1", "type": "function", + "function": {"name": "read_file", "arguments": good}}, + {"id": "c2", "type": "function", + "function": {"name": "write_file", "arguments": '{"content": "cut'}}, + ], + }] + before = copy.deepcopy(history) + + api_messages = [dict(m) for m in history] + cl._canonicalize_api_tool_calls(api_messages) + + assert history == before + sent = api_messages[0]["tool_calls"] + assert json.loads(sent[0]["function"]["arguments"]) == json.loads(good) + assert sent[1]["function"]["arguments"] == "{}" + + def test_repeated_sends_do_not_accumulate_damage(self): + """Re-canonicalizing the same history every iteration stays lossless.""" + history, truncated = self._history_with_truncated_write() + for _ in range(5): + api_messages = [dict(m) for m in history] + cl._canonicalize_api_tool_calls(api_messages) + assert api_messages[0]["tool_calls"][0]["function"]["arguments"] == "{}" + assert ( + history[0]["tool_calls"][0]["function"]["arguments"] == truncated + )