From 53b52092f959578bd3547cf1d0c5d2583f38ad84 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sat, 9 May 2026 02:13:51 -0700 Subject: [PATCH] fix(reasoning): per-block ANTHROPIC_VALID_BLOCK_TYPES filter + review fixes The earlier all-or-nothing shape check on ``_provider_content`` discarded every valid Anthropic block in a message the moment a single foreign block (OpenAI ``reasoning``, Gemini thought parts, the synthetic ``reasoning_text`` from path-3 capture) appeared. In the cross-model resumption edge case that meant ``server_tool_use`` / ``web_search_tool_result`` blocks lost their ``encrypted_content`` silently, breaking web-search round-trip continuity on subsequent turns. Replaced with a per-block walk: foreign blocks are dropped individually, valid blocks ride the verbatim path, and an identity-preserving fast path reuses the source list reference when nothing was filtered or stripped (pinned by the ``is`` assertions in test_providers.py). Also addresses validation-pass review findings: - Document the single-tier vs three-tier ``surface_persisted_reasoning`` resolution divergence between server.py:_build_history and session_routes.make_history_handler. - Document why OpenAIResponsesProvider._convert_messages defaults ``replay_reasoning_to_model=False`` while Anthropic's defaults True. - Document the ``source`` metadata field on synthetic ``reasoning_text`` blocks as reserved-for-future-use, not dead code. - Add edge tests for non-dict / missing-type-key blocks in _provider_content (defensive branches in the per-block walk). --- tests/test_provider_anthropic_replay.py | 105 +++++++++- turnstone/core/providers/_anthropic.py | 195 +++++++++--------- turnstone/core/providers/_openai_responses.py | 11 + turnstone/core/session.py | 8 + turnstone/server.py | 12 +- 5 files changed, 224 insertions(+), 107 deletions(-) diff --git a/tests/test_provider_anthropic_replay.py b/tests/test_provider_anthropic_replay.py index 3ebedfc3..7cb143af 100644 --- a/tests/test_provider_anthropic_replay.py +++ b/tests/test_provider_anthropic_replay.py @@ -4,11 +4,12 @@ Phase 2 of optional reasoning persistence wraps the verbatim ``_provider_content`` replay path at ``_anthropic.py:_convert_messages`` with two gates: -1. ``ANTHROPIC_VALID_BLOCK_TYPES`` shape filter — foreign-shaped - blocks (OpenAI Responses ``type="reasoning"`` after Phase 3 lands, - Gemini thought parts, anything else) fall through to the - text+tool_calls rebuild path rather than 400-ing the API. Closes - a pre-existing latent bug. +1. ``ANTHROPIC_VALID_BLOCK_TYPES`` per-block shape filter — foreign- + shaped blocks (OpenAI Responses ``type="reasoning"``, Gemini thought + parts, the synthetic ``reasoning_text`` from path-3 capture) are + dropped individually; valid Anthropic blocks in the same message + still ride the verbatim path. When NO valid blocks survive, the + converter falls through to the text+tool_calls rebuild path. 2. ``replay_reasoning_to_model`` operator flag — when False (the ``model_definitions`` server_default), thinking blocks are stripped before the wire payload is built. Tool_use / @@ -277,10 +278,10 @@ class TestShapeFilterFallthrough: types_present = [b["type"] for b in assistant["content"]] assert "reasoning" not in types_present - def test_mixed_shape_one_foreign_block_falls_through(self, provider: AnthropicProvider) -> None: - # Even a single foreign block in a mostly-Anthropic payload - # forces fall-through (the shape predicate is "all blocks - # match", not "majority match"). + def test_mixed_shape_drops_foreign_keeps_valid(self, provider: AnthropicProvider) -> None: + # Per-block filter: a single foreign block in a mostly-Anthropic + # payload no longer forces fall-through. Valid Anthropic blocks + # ride the verbatim path; the foreign block is dropped. msg = { "role": "assistant", "content": "Mixed.", @@ -292,9 +293,72 @@ class TestShapeFilterFallthrough: } _, converted = provider._convert_messages([msg], replay_reasoning_to_model=True) assistant = next(m for m in converted if m["role"] == "assistant") + types_present = [b["type"] for b in assistant["content"]] + assert "reasoning" not in types_present # foreign dropped + assert "thinking" in types_present # valid + replay=True kept + assert "text" in types_present for b in assistant["content"]: assert b.get("type") in ANTHROPIC_VALID_BLOCK_TYPES + def test_mixed_shape_preserves_web_search_encrypted_content( + self, provider: AnthropicProvider + ) -> None: + # The motivating case for per-block (vs all-or-nothing) filter: + # cross-model resumption stamps a foreign ``reasoning`` block + # alongside Anthropic web-search blocks carrying encrypted + # citations. An all-or-nothing filter would discard the whole + # message and rebuild from text+tool_calls — silently losing + # the encrypted_content the API needs for round-trip continuity. + msg = { + "role": "assistant", + "content": "From search: ...", + "_provider_content": [ + {"type": "reasoning", "summary": []}, # foreign (e.g. OpenAI) + { + "type": "server_tool_use", + "id": "stu_1", + "name": "web_search", + "input": {"query": "x"}, + }, + { + "type": "web_search_tool_result", + "tool_use_id": "stu_1", + "content": [{"type": "web_search_result", "url": "https://e.com"}], + "encrypted_content": "encrypted-blob-must-survive", + "encrypted_index": "encrypted-idx-must-survive", + }, + {"type": "text", "text": "From search: ..."}, + ], + } + _, converted = provider._convert_messages([msg], replay_reasoning_to_model=False) + assistant = next(m for m in converted if m["role"] == "assistant") + types_present = [b["type"] for b in assistant["content"]] + assert "reasoning" not in types_present # foreign dropped + assert "server_tool_use" in types_present + assert "web_search_tool_result" in types_present + assert "text" in types_present + wsr = next(b for b in assistant["content"] if b["type"] == "web_search_tool_result") + assert wsr["encrypted_content"] == "encrypted-blob-must-survive" + assert wsr["encrypted_index"] == "encrypted-idx-must-survive" + + def test_all_foreign_blocks_fall_through_to_rebuild(self, provider: AnthropicProvider) -> None: + # When every block is foreign-shaped (no Anthropic-valid block + # survives the per-block filter), the converter still falls + # through to text+tool_calls rebuild rather than emitting an + # empty assistant turn. + msg = { + "role": "assistant", + "content": "Final answer.", + "_provider_content": [ + {"type": "reasoning", "summary": []}, + {"type": "reasoning_text", "text": "synthetic"}, # path-3 shape + ], + } + _, converted = provider._convert_messages([msg], replay_reasoning_to_model=True) + assistant = next(m for m in converted if m["role"] == "assistant") + # Rebuild path: msg.content lifted into a single text block. + assert assistant["content"] == [{"type": "text", "text": "Final answer."}] + def test_empty_provider_content_falls_through(self, provider: AnthropicProvider) -> None: msg = { "role": "assistant", @@ -327,6 +391,29 @@ class TestShapeFilterFallthrough: assistant = next(m for m in converted if m["role"] == "assistant") assert assistant["content"] == [{"type": "text", "text": "Plain."}] + def test_non_dict_and_missing_type_blocks_are_dropped( + self, provider: AnthropicProvider + ) -> None: + # Defensive branches in the per-block walk: a stray non-dict + # element (corrupted JSON) or a dict with no/None ``type`` key + # (provider drift) must be silently dropped without raising. + # Valid blocks in the same list still ride the verbatim path. + msg = { + "role": "assistant", + "content": "ok", + "_provider_content": [ + {"type": "text", "text": "ok"}, + "stray-string", # non-dict + {"type": None, "text": "huh"}, # None type + {"no_type_key": 1}, # missing type + {"type": "thinking", "thinking": "t", "signature": "s"}, + ], + } + _, converted = provider._convert_messages([msg], replay_reasoning_to_model=True) + assistant = next(m for m in converted if m["role"] == "assistant") + types_present = [b.get("type") for b in assistant["content"]] + assert types_present == ["text", "thinking"] + class TestLegacyAnthropicRowsNoRegression: """Critical property: rows persisted before Phase 2 carry valid diff --git a/turnstone/core/providers/_anthropic.py b/turnstone/core/providers/_anthropic.py index 21cd8d86..52cfef77 100644 --- a/turnstone/core/providers/_anthropic.py +++ b/turnstone/core/providers/_anthropic.py @@ -168,14 +168,18 @@ def _map_reasoning_to_effort( # Anthropic accepts only a closed set of content-block types on the -# input boundary. ``_convert_messages`` gates the verbatim -# ``_provider_content`` replay path on this set so foreign-shaped -# blocks (OpenAI Responses ``type="reasoning"`` after Phase 3, Gemini -# thought parts, anything else) fall through to the text+tool_calls -# rebuild path rather than 400-ing the API. Web-search related blocks -# (``server_tool_use`` / ``web_search_tool_result``) intentionally -# stay in the set — they carry ``encrypted_content`` that the API -# requires for round-trip continuity. +# input boundary. ``_convert_messages`` runs each block in +# ``_provider_content`` through this set per-block: foreign-shaped +# blocks (OpenAI Responses ``type="reasoning"``, Gemini thought parts, +# the synthetic ``reasoning_text`` from path-3 capture) are dropped +# individually; valid Anthropic blocks in the same message still ride +# the verbatim path. When no valid blocks survive, the converter falls +# through to the text+tool_calls rebuild path. Web-search blocks +# (``server_tool_use`` / ``web_search_tool_result``) stay in the set +# because they carry ``encrypted_content`` the API requires for +# round-trip continuity — the per-block filter preserves them even when +# they share a message with a foreign block (the prior all-or-nothing +# filter would have silently dropped them in that case). ANTHROPIC_VALID_BLOCK_TYPES = frozenset( { "text", @@ -342,12 +346,15 @@ class AnthropicProvider: (``ChatSession._try_stream`` / ``_utility_completion``) always pass the resolved flag explicitly. - Foreign-shaped ``_provider_content`` (e.g. OpenAI Responses - ``type="reasoning"`` blocks reaching Anthropic mid-workstream - once Phase 3 lands) fails the ``ANTHROPIC_VALID_BLOCK_TYPES`` - shape check and falls through to the text+tool_calls rebuild - path — closes a pre-existing latent bug where the verbatim - replay would have 400'd the API. + Foreign-shaped blocks (OpenAI ``reasoning``, Gemini thought + parts, the synthetic ``reasoning_text`` from path-3 capture) are + dropped per-block; valid Anthropic blocks in the same message + still ride the verbatim path. Critical for cross-model + resumption: an earlier all-or-nothing filter silently lost + web-search ``encrypted_content`` whenever a foreign block + shared a message with ``server_tool_use`` / + ``web_search_tool_result``. If the filter leaves nothing, the + converter falls through to the text+tool_calls rebuild path. """ system_parts: list[str] = [] converted: list[dict[str, Any]] = [] @@ -370,86 +377,88 @@ class AnthropicProvider: if pending_orphan_results: converted.append({"role": "user", "content": pending_orphan_results}) pending_orphan_results = [] - # If raw provider content was preserved, pass it through verbatim - # so encrypted_content/encrypted_index from web search are retained. - # Phase 2: gated by ``ANTHROPIC_VALID_BLOCK_TYPES`` shape check - # (foreign or partially-foreign payloads fall through to the - # text+tool_calls rebuild path) and the ``replay_reasoning_to_model`` - # operator flag (when False, ``thinking`` blocks are stripped). + # Per-block shape filter: drop foreign blocks individually, + # apply the replay strip to valid ``thinking`` / + # ``redacted_thinking`` blocks. See ``_convert_messages`` + # docstring for why this is per-block, not all-or-nothing. provider_content = msg.get("_provider_content") - # Inline isinstance + non-empty + all-blocks-valid check - # so mypy can narrow ``provider_content`` to ``list`` for - # the subsequent walk; pulling these into a helper boolean - # would require an explicit ``cast`` to recover the type. - if ( - isinstance(provider_content, list) - and provider_content - and all( - isinstance(b, dict) and b.get("type") in ANTHROPIC_VALID_BLOCK_TYPES - for b in provider_content - ) - ): - # replay=True keeps the verbatim reference (existing - # behaviour — pinned by an identity assertion in - # test_convert_messages_uses_provider_content). replay=False - # builds a new list with thinking blocks filtered out; - # tool_use / web-search blocks survive intact so their - # encrypted_content round-trips correctly. - if replay_reasoning_to_model: - wire_blocks: list[dict[str, Any]] = provider_content - else: - wire_blocks = [ - b - for b in provider_content - if b.get("type") not in ANTHROPIC_REASONING_BLOCK_TYPES - ] - if wire_blocks: - converted.append({"role": "assistant", "content": wire_blocks}) - # Orphan-tool detection runs on the ORIGINAL provider_content - # (not the strip-filtered ``wire_blocks``) — the strip - # only drops thinking blocks, so tool_use IDs are - # identical between the two lists, but we keep the - # source-of-truth read explicit. - pc_tool_ids = [ - b["id"] - for b in provider_content - if isinstance(b, dict) and b.get("type") == "tool_use" and b.get("id") - ] - if pc_tool_ids: - j = i + 1 - result_ids_pc: set[str] = set() - while j < len(messages) and messages[j]["role"] == "tool": - tc_id = messages[j].get("tool_call_id", "") - if tc_id: - result_ids_pc.add(tc_id) - j += 1 - orphaned_pc = [uid for uid in pc_tool_ids if uid not in result_ids_pc] - if orphaned_pc: - log.debug( - "Synthesizing %d tool_result(s) for orphaned provider_content tool_use IDs", - len(orphaned_pc), - ) - synthetic_pc = [ - { - "type": "tool_result", - "tool_use_id": uid, - "content": "Tool execution was cancelled.", - "is_error": True, - } - for uid in orphaned_pc - ] - if j == i + 1: - converted.append({"role": "user", "content": synthetic_pc}) - else: - pending_orphan_results = synthetic_pc - i += 1 - continue - # All blocks were stripped (message had only thinking - # content + no text + no tool_calls). Fall through to the - # text+tool_calls rebuild path; if that also produces an - # empty message it will be silently skipped, which is - # correct: a turn with only stripped reasoning has nothing - # to replay. + wire_blocks: list[dict[str, Any]] = [] + valid_blocks: list[dict[str, Any]] = [] + all_input_valid = True + if isinstance(provider_content, list) and provider_content: + dropped_foreign: list[str] = [] + for b in provider_content: + if not isinstance(b, dict): + all_input_valid = False + continue + btype = b.get("type") + if btype not in ANTHROPIC_VALID_BLOCK_TYPES: + dropped_foreign.append(str(btype)) + all_input_valid = False + continue + valid_blocks.append(b) + if ( + not replay_reasoning_to_model + and btype in ANTHROPIC_REASONING_BLOCK_TYPES + ): + continue + wire_blocks.append(b) + if dropped_foreign: + log.debug( + "Dropped %d foreign block(s) from _provider_content " + "during Anthropic conversion: %s", + len(dropped_foreign), + dropped_foreign, + ) + # Identity-preserving fast path: when nothing was filtered + # or stripped, reuse the source list reference rather than + # the per-block-built copy. Pinned by the ``is`` assertions + # in test_providers.py (test_convert_messages_uses_provider_ + # content + test_thinking_block_multiturn_roundtrip). + if all_input_valid and replay_reasoning_to_model: + wire_blocks = provider_content + if valid_blocks and wire_blocks: + converted.append({"role": "assistant", "content": wire_blocks}) + # Orphan-tool detection reads ``valid_blocks`` (unstripped + # valid blocks) so a future widening of the strip predicate + # can't accidentally drop tool_use IDs. + pc_tool_ids = [ + b["id"] for b in valid_blocks if b.get("type") == "tool_use" and b.get("id") + ] + if pc_tool_ids: + j = i + 1 + result_ids_pc: set[str] = set() + while j < len(messages) and messages[j]["role"] == "tool": + tc_id = messages[j].get("tool_call_id", "") + if tc_id: + result_ids_pc.add(tc_id) + j += 1 + orphaned_pc = [uid for uid in pc_tool_ids if uid not in result_ids_pc] + if orphaned_pc: + log.debug( + "Synthesizing %d tool_result(s) for orphaned provider_content tool_use IDs", + len(orphaned_pc), + ) + synthetic_pc = [ + { + "type": "tool_result", + "tool_use_id": uid, + "content": "Tool execution was cancelled.", + "is_error": True, + } + for uid in orphaned_pc + ] + if j == i + 1: + converted.append({"role": "user", "content": synthetic_pc}) + else: + pending_orphan_results = synthetic_pc + i += 1 + continue + # Fall through to text+tool_calls rebuild when nothing + # survived the filter (missing/empty pc, all-foreign, or + # strip removed every remaining thinking block). An empty + # rebuild is silently skipped — a turn with only stripped + # reasoning has nothing to replay. content_blocks: list[dict[str, Any]] = [] text = msg.get("content") diff --git a/turnstone/core/providers/_openai_responses.py b/turnstone/core/providers/_openai_responses.py index ba621ca8..52db8aca 100644 --- a/turnstone/core/providers/_openai_responses.py +++ b/turnstone/core/providers/_openai_responses.py @@ -117,6 +117,17 @@ class OpenAIResponsesProvider: When *replay_reasoning_to_model* is False, reasoning items are silently dropped (they were stripped from the wire by ``sanitize_messages`` anyway, but we also skip the input-item emission step). + + Default ``False`` differs intentionally from + ``AnthropicProvider._convert_messages`` (which defaults + ``True``). Anthropic's default exists for back-compat with + pre-Phase-2 callers who never threaded the kwarg; OpenAI + Responses replay is brand-new in Phase 3 and has no such + legacy. Production callers (``_build_kwargs``) always pass + the resolved flag explicitly, so the default only matters in + tests. Conservative-default-False keeps the persist-only + capture path live without forcing a downstream cost on every + unaware caller. """ # Capture ``_provider_content`` reasoning items per ASSISTANT # ORDINAL (not raw message index) BEFORE sanitization strips diff --git a/turnstone/core/session.py b/turnstone/core/session.py index c78fd573..74434aff 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -1156,6 +1156,14 @@ class ChatSession: resumption — protecting against operator-switches from a local-model session to Anthropic, which would otherwise hit Anthropic's input boundary with an unsigned ``thinking`` block. + + The optional ``source`` field tags the block with the + originating server (``vllm``, ``llamacpp``, ``sglang``, etc.) + when ``ModelConfig.capabilities["server_compat"]["server_type"]`` + is populated. Reserved for future per-server replay paths + (e.g. an operator-flagged path that re-injects synthetic + reasoning back into a vllm round-trip) — not consumed today; + the field is informational metadata, not dead code. """ if provider_blocks: return provider_blocks diff --git a/turnstone/server.py b/turnstone/server.py index 70e761a4..31dd6a71 100644 --- a/turnstone/server.py +++ b/turnstone/server.py @@ -474,11 +474,13 @@ def _build_history( else: ws_id = getattr(session, "_ws_id", "") or "" verdicts_by_call_id, assessments_by_call_id = _load_verdict_indexes(ws_id) - # Active-model reasoning-persistence flag — defaults True so that a - # registry/alias lookup miss still surfaces reasoning bubbles. The - # default-True semantic mirrors the migration's server_default for - # ``model_definitions.surface_persisted_reasoning`` and matches the conservative - # rehydration default (Phase 1 spec). + # Active-model ``surface_persisted_reasoning`` flag — single-tier + # resolution (live session's registry only). This path always has + # a live ``ChatSession`` in hand, so the cold-workstream and + # app.state-registry tiers used by ``make_history_handler`` + # (``session_routes.py:2396-2429``) are unreachable here. Default + # True mirrors the migration's server_default and matches the + # conservative rehydration default in the Phase 1 spec. surface_persisted_reasoning = True registry = getattr(session, "_registry", None) model_alias = getattr(session, "_model_alias", "") or ""