fix(agent): stop the send-path repair from rewriting persisted history
`_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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user