diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 73bed6b067..8850b7fd56 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -2221,30 +2221,54 @@ def run_conversation( print(f"{agent.log_prefix} • Legacy cleanup: hermes config set ANTHROPIC_TOKEN \"\"") print(f"{agent.log_prefix} • Clear stale keys: hermes config set ANTHROPIC_API_KEY \"\"") - # ── Thinking block signature recovery ───────────────── + # Thinking block signature recovery. + # # Anthropic signs thinking blocks against the full turn - # content. Any upstream mutation (context compression, + # content. Any upstream mutation (context compression, # session truncation, message merging) invalidates the - # signature → HTTP 400. Recovery: strip reasoning_details - # from all messages so the next retry sends no thinking - # blocks at all. One-shot — don't retry infinitely. + # signature and the API replies HTTP 400 ("invalid + # signature" or "cannot be modified"). Recovery strips + # ``reasoning_details`` so the retry sends no thinking + # blocks at all. One-shot per outer loop. + # + # The strip targets ``api_messages``, which is the + # API-call-time list that ``_build_api_kwargs`` consumes + # on every retry. ``api_messages`` was populated once at + # the start of the turn from shallow copies of + # ``messages``, so mutating it does not touch the + # canonical store. The previous implementation popped + # ``reasoning_details`` from ``messages`` instead, which + # had two problems: ``api_messages`` carried its own + # reference to the field through the shallow copy, so the + # retry's wire payload still included thinking blocks and + # the recovery never reached the API; and the mutation + # persisted into ``state.db`` through any subsequent + # ``_persist_session`` call, permanently corrupting the + # conversation. Future turns would replay the stripped + # state, hit the same 400, and the agent would terminate + # with ``max_retries_exhausted``, often spawning + # cascading compaction-ended sessions chained off the + # corrupted parent. if ( classified.reason == FailoverReason.thinking_signature and not _retry.thinking_sig_retry_attempted ): _retry.thinking_sig_retry_attempted = True - for _m in messages: - if isinstance(_m, dict): + _api_stripped = 0 + for _m in api_messages: + if isinstance(_m, dict) and "reasoning_details" in _m: _m.pop("reasoning_details", None) + _api_stripped += 1 agent._vprint( - f"{agent.log_prefix}⚠️ Thinking block signature invalid — " - f"stripped all thinking blocks, retrying...", + f"{agent.log_prefix}⚠️ Thinking block signature invalid, " + f"stripped reasoning_details from api_messages for retry...", force=True, ) logger.warning( "%sThinking block signature recovery: stripped " - "reasoning_details from %d messages", - agent.log_prefix, len(messages), + "reasoning_details from %d api_messages " + "(canonical messages unchanged)", + agent.log_prefix, _api_stripped, ) continue diff --git a/tests/run_agent/test_thinking_sig_recovery_persistence.py b/tests/run_agent/test_thinking_sig_recovery_persistence.py new file mode 100644 index 0000000000..e518af5145 --- /dev/null +++ b/tests/run_agent/test_thinking_sig_recovery_persistence.py @@ -0,0 +1,93 @@ +"""Regression tests for the thinking-block signature recovery. + +The recovery in ``agent/conversation_loop.py`` strips ``reasoning_details`` +from ``api_messages`` (the API-call-time list rebuilt on every retry) and +leaves ``messages`` (the canonical store) untouched. The previous +implementation popped from ``messages`` directly, which never reached +``api_messages`` because each entry in ``api_messages`` was a shallow +copy of the corresponding entry in ``messages``, and the mutation also +landed in ``state.db`` on the next ``_persist_session`` call, corrupting +the conversation. + +These tests cover the surface that the recovery touches in isolation: +shallow copies share inner field references; popping a key from one dict +does not remove it from the other; and a list of shallow copies behaves +the same way. +""" + + +def _shallow_copies(messages): + return [m.copy() for m in messages] + + +def test_pop_on_shallow_copy_does_not_affect_source(): + rd = [{"type": "thinking", "thinking": "r", "signature": "s"}] + src = {"role": "assistant", "content": "x", "reasoning_details": rd} + cp = src.copy() + + cp.pop("reasoning_details", None) + + assert "reasoning_details" not in cp + assert "reasoning_details" in src + assert src["reasoning_details"] is rd + + +def test_strip_api_messages_leaves_canonical_messages_intact(): + """Mirrors the recovery: pop reasoning_details from api_messages only. + + The canonical ``messages`` list keeps its reasoning_details so future + persists carry the original signed blocks. + """ + rd_one = [{"type": "thinking", "thinking": "one", "signature": "sig_one"}] + rd_two = [{"type": "thinking", "thinking": "two", "signature": "sig_two"}] + messages = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "a1", "reasoning_details": rd_one}, + {"role": "user", "content": "q2"}, + {"role": "assistant", "content": "a2", "reasoning_details": rd_two}, + ] + api_messages = _shallow_copies(messages) + + stripped = 0 + for m in api_messages: + if isinstance(m, dict) and "reasoning_details" in m: + m.pop("reasoning_details", None) + stripped += 1 + + assert stripped == 2 + assert all("reasoning_details" not in m for m in api_messages) + canonical_rd = [ + m.get("reasoning_details") for m in messages if m["role"] == "assistant" + ] + assert canonical_rd == [rd_one, rd_two] + + +def test_strip_is_idempotent_when_run_twice(): + """A second strip is a no-op when reasoning_details has already been + removed from api_messages. Guards against a duplicate firing path. + """ + api_messages = [ + {"role": "assistant", "content": "a", "reasoning_details": [{"x": 1}]}, + {"role": "user", "content": "q"}, + ] + for _ in range(2): + for m in api_messages: + if isinstance(m, dict) and "reasoning_details" in m: + m.pop("reasoning_details", None) + + assert all("reasoning_details" not in m for m in api_messages) + + +def test_strip_skips_messages_without_reasoning_details(): + api_messages = [ + {"role": "user", "content": "q"}, + {"role": "assistant", "content": "a"}, + {"role": "tool", "tool_call_id": "1", "content": "ok"}, + ] + snapshot = [dict(m) for m in api_messages] + + for m in api_messages: + if isinstance(m, dict) and "reasoning_details" in m: + m.pop("reasoning_details", None) + + assert api_messages == snapshot