mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
33865ca9d2
Multi-stage /review on the full Phase 1+2+3+4 stack surfaced 9 findings (0 critical, 3 major, 5 minor, 1 nit, 1 uncertain). All applied. Major * perf-1 (session_routes.py:2402): make_history_handler ran sync storage.load_workstream_config inside async def history on the cold- workstream path, blocking the event loop on every dashboard /history request for non-resident workstreams. Every other storage call in the same handler correctly used asyncio.to_thread. Wrap the sync call in asyncio.to_thread (preserving the existing try/except so a DB failure still degrades to the conservative-default branch instead of bubbling out). * q-2 (test_reasoning_audit_log_discipline.py): the security-sensitive test (reasoning text never lands at INFO+ severity) only covered the 4 Phase 1 surfaces. Phase 2 added the strip predicate in AnthropicProvider._convert_messages and Phase 3 added 3 more code paths that touch reasoning text — none guarded. Added 4 parallel tests using the existing capture-and-walk infrastructure: OpenAIResponsesProvider.extract_reasoning_text, OpenAIChatCompletionsProvider.extract_reasoning_text, ChatSession._stream_response (drives the synth-block stamp via a fake reasoning-emitting stream), AnthropicProvider._convert_messages with replay_reasoning_to_model=False (drives the Phase 2 strip predicate). * q-1 (model_registry.py:42): the persist_reasoning flag name implied storage-control but actually gates UI rehydration only — operators flipping it could reasonably expect "stop persisting reasoning" but storage of reasoning bytes happens in provider_data regardless. Renamed everywhere to surface_persisted_reasoning: ModelConfig field, migration 052 column (renaming in-place since 052 is not yet on main), schema, MODEL_DEFINITION_MUTABLE allowlist, _postgresql.py + _sqlite.py CRUD impls, _protocol.py create_model_definition signature, 3 console_schemas Pydantic models, console/server.py admin POST + PUT, model_registry row mapper, history_decoration.py helper parameter, server.py _build_history local var, session_routes.py make_history_handler local var, sdk/events.py HistoryEvent docstring, admin.js form id + override pill label, index.html form input id + UI label + tooltip, coordinator.js (none needed), and every test that referenced the old field name. The admin tooltip now reads "Storage of reasoning bytes is unaffected by this flag — they ride in provider_data regardless" so the decoupling stays explicit at the operator surface. Minor * bug-1 (history_decoration.py:336): dispatcher discriminated on provider_content[0]["type"] only. Anthropic's redacted_thinking blocks (sealed by the safety system) can appear before, after, or interleaved with regular thinking blocks per the API docs. When a redacted block lands first, the dispatcher returned "" and the UI silently lost the surrounding thinking text. Registered "redacted_thinking" as a second key in _BLOCK_TYPE_PROVIDER_FACTORY pointing at the same AnthropicProvider factory — the existing extractor's type=="thinking" filter already correctly skips redacted blocks while walking the full list. Regression test added. * q-3 (_protocol.py:155): replay_reasoning_to_model defaults split across 9 sites — operator-side defaults to False (matches DB server_default), provider-API defaults to True (back-compat with direct callers). Original "pick False everywhere" fix would have silently flipped behaviour for any direct provider caller. Instead documented the intentional bifurcation in the Protocol's create_streaming docstring. * q-4+q-5 (_protocol.py:107 + 3 providers): MAX_REASONING_DISPLAY_BYTES was enforced via Python str slicing which counts code points, not UTF-8 bytes — 4-byte CJK/emoji glyphs would blow past the byte ceiling. Renamed to MAX_REASONING_DISPLAY_CHARS to match actual behaviour. Hoisted the 4-line truncation pattern into a shared _join_reasoning_with_cap helper in _protocol.py; each provider's extractor becomes a single line at the tail. * q-6 (tests/_session_helpers.py): _NullUI + _make_session were duplicated verbatim between test_session_replay_reasoning.py and test_session_synth_reasoning_block.py. Hoisted to a shared tests/_session_helpers.py module (importable, leading underscore so pytest doesn't try to collect it). test_model_registry.py's _make_session has a different signature (registry/model_alias args + _FakeUI) and is not a candidate for sharing. Nit * q-7 (history_decoration.py:286): _make_provider_factory used a dict-as-cell workaround for closure read-only scope. Replaced with the more idiomatic nonlocal pattern. Lint + test gate * ruff check + ruff format -- clean. * mypy -- no issues across all 191 source files. * pytest -m 'not live' -- 6115 passed (3 deselected). Net +5 tests (4 audit-log discipline + 1 redacted_thinking dispatcher). Refinements vs the dedupe output (caught during sanity rendering the report) * perf-1 fix preserved the try/except wrapper. The original "wrap in to_thread" one-liner would have let an OperationalError bubble out instead of degrading to the fallback branch. * q-3 fix explicitly documented the bifurcation rather than collapsing both sides to False. "Pick False everywhere" would silently flip back-compat behaviour for direct provider callers. * q-1 fix included the admin.js:5292 fallback site (m.persist_reasoning !== false) that the original threaded-change list missed. * q-6 fix verified the third _make_session in test_model_registry.py is structurally different (different signature + different UI helper) and intentionally NOT a dedupe target.
126 lines
5.9 KiB
Python
126 lines
5.9 KiB
Python
"""Tests for ``AnthropicProvider.extract_reasoning_text``.
|
|
|
|
Phase 1 of the optional-reasoning-persistence feature: provider-side
|
|
extractor that walks stored ``provider_blocks`` and returns the
|
|
concatenated thinking text, capped at the operator-friendly UI display
|
|
size.
|
|
|
|
These tests drive through the real ``AnthropicProvider`` instance — no
|
|
mocks of the extractor itself — using fixture-shaped blocks that match
|
|
what ``_iter_anthropic_stream`` actually accumulates at
|
|
``_anthropic.py:713-724`` (``thinking_delta`` + ``signature_delta``
|
|
combined into ``{"type": "thinking", "thinking": <text>, "signature":
|
|
<sig>}``).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
|
|
from turnstone.core.providers._anthropic import AnthropicProvider
|
|
from turnstone.core.providers._openai_chat import OpenAIChatCompletionsProvider
|
|
from turnstone.core.providers._openai_responses import OpenAIResponsesProvider
|
|
from turnstone.core.providers._protocol import (
|
|
MAX_REASONING_DISPLAY_CHARS as _MAX_REASONING_DISPLAY_CHARS,
|
|
)
|
|
|
|
|
|
@pytest.fixture
|
|
def anthropic() -> AnthropicProvider:
|
|
return AnthropicProvider()
|
|
|
|
|
|
class TestExtractReasoningText:
|
|
def test_none_input_returns_empty_string(self, anthropic: AnthropicProvider) -> None:
|
|
assert anthropic.extract_reasoning_text(None) == ""
|
|
|
|
def test_empty_list_returns_empty_string(self, anthropic: AnthropicProvider) -> None:
|
|
assert anthropic.extract_reasoning_text([]) == ""
|
|
|
|
def test_no_thinking_blocks_returns_empty(self, anthropic: AnthropicProvider) -> None:
|
|
blocks: list[dict[str, object]] = [
|
|
{"type": "text", "text": "hello"},
|
|
{"type": "tool_use", "id": "t1", "name": "x", "input": {}},
|
|
]
|
|
assert anthropic.extract_reasoning_text(blocks) == ""
|
|
|
|
def test_single_thinking_block_returns_text(self, anthropic: AnthropicProvider) -> None:
|
|
blocks = [{"type": "thinking", "thinking": "Let me think about this.", "signature": "abc"}]
|
|
assert anthropic.extract_reasoning_text(blocks) == "Let me think about this."
|
|
|
|
def test_multiple_thinking_blocks_joined_with_newline(
|
|
self, anthropic: AnthropicProvider
|
|
) -> None:
|
|
blocks = [
|
|
{"type": "thinking", "thinking": "first thought", "signature": "s1"},
|
|
{"type": "thinking", "thinking": "second thought", "signature": "s2"},
|
|
]
|
|
assert anthropic.extract_reasoning_text(blocks) == "first thought\nsecond thought"
|
|
|
|
def test_mixed_blocks_extracts_only_thinking(self, anthropic: AnthropicProvider) -> None:
|
|
blocks = [
|
|
{"type": "thinking", "thinking": "reason A", "signature": "s"},
|
|
{"type": "text", "text": "visible answer"},
|
|
{"type": "tool_use", "id": "t1", "name": "x", "input": {}},
|
|
{"type": "thinking", "thinking": "reason B", "signature": "s"},
|
|
]
|
|
assert anthropic.extract_reasoning_text(blocks) == "reason A\nreason B"
|
|
|
|
def test_thinking_block_without_thinking_field_skipped(
|
|
self, anthropic: AnthropicProvider
|
|
) -> None:
|
|
blocks = [{"type": "thinking", "signature": "s"}]
|
|
assert anthropic.extract_reasoning_text(blocks) == ""
|
|
|
|
def test_thinking_block_with_empty_text_skipped(self, anthropic: AnthropicProvider) -> None:
|
|
blocks = [{"type": "thinking", "thinking": "", "signature": "s"}]
|
|
assert anthropic.extract_reasoning_text(blocks) == ""
|
|
|
|
def test_truncation_at_64kib_cap(self, anthropic: AnthropicProvider) -> None:
|
|
long_text = "x" * (_MAX_REASONING_DISPLAY_CHARS + 1024)
|
|
blocks = [{"type": "thinking", "thinking": long_text, "signature": "s"}]
|
|
result = anthropic.extract_reasoning_text(blocks)
|
|
assert len(result) == _MAX_REASONING_DISPLAY_CHARS
|
|
|
|
def test_just_under_cap_not_truncated(self, anthropic: AnthropicProvider) -> None:
|
|
text = "y" * (_MAX_REASONING_DISPLAY_CHARS - 1)
|
|
blocks = [{"type": "thinking", "thinking": text, "signature": "s"}]
|
|
assert anthropic.extract_reasoning_text(blocks) == text
|
|
|
|
def test_malformed_block_entry_skipped(self, anthropic: AnthropicProvider) -> None:
|
|
# A defensive sanity check — we should not crash if some
|
|
# entry isn't a dict (e.g. a corrupted JSON payload).
|
|
blocks = [
|
|
"not a dict", # type: ignore[list-item]
|
|
{"type": "thinking", "thinking": "good one", "signature": "s"},
|
|
]
|
|
assert anthropic.extract_reasoning_text(blocks) == "good one" # type: ignore[arg-type]
|
|
|
|
def test_non_list_input_returns_empty(self, anthropic: AnthropicProvider) -> None:
|
|
# Defensive against a corrupted provider_data payload.
|
|
assert anthropic.extract_reasoning_text("not a list") == "" # type: ignore[arg-type]
|
|
assert anthropic.extract_reasoning_text({"type": "thinking"}) == "" # type: ignore[arg-type]
|
|
|
|
|
|
class TestOtherProvidersDefault:
|
|
"""Non-Anthropic providers return "" for the same fixture shapes."""
|
|
|
|
def test_openai_chat_returns_empty(self) -> None:
|
|
provider = OpenAIChatCompletionsProvider()
|
|
blocks = [{"type": "thinking", "thinking": "would-be-text", "signature": "s"}]
|
|
assert provider.extract_reasoning_text(blocks) == ""
|
|
|
|
def test_openai_responses_returns_empty(self) -> None:
|
|
provider = OpenAIResponsesProvider()
|
|
blocks = [{"type": "thinking", "thinking": "would-be-text", "signature": "s"}]
|
|
assert provider.extract_reasoning_text(blocks) == ""
|
|
|
|
def test_openai_responses_extracts_reasoning_summary(self) -> None:
|
|
# Phase 3: extractor now walks reasoning items captured via
|
|
# include=["reasoning.encrypted_content"] and returns the
|
|
# summary[*].text concatenation. Pre-Phase-3 this returned
|
|
# "" — the stub was replaced once the wire path landed.
|
|
provider = OpenAIResponsesProvider()
|
|
blocks = [{"type": "reasoning", "summary": [{"type": "summary_text", "text": "x"}]}]
|
|
assert provider.extract_reasoning_text(blocks) == "x"
|