From 33865ca9d2b28d7bfad4ccd5a632c93f3ec3f413 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Fri, 8 May 2026 23:03:02 -0700 Subject: [PATCH] fix(reasoning): apply full-stack review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/_session_helpers.py | 45 ++++++ tests/test_build_history_reminders.py | 18 +-- tests/test_history_decoration.py | 51 +++++-- tests/test_model_definition_storage.py | 26 ++-- tests/test_model_registry.py | 14 +- tests/test_provider_anthropic_reasoning.py | 8 +- ...est_provider_openai_responses_reasoning.py | 6 +- tests/test_reasoning_audit_log_discipline.py | 143 +++++++++++++++++- tests/test_session_replay_reasoning.py | 26 +--- tests/test_session_synth_reasoning_block.py | 23 +-- tests/test_workstream_endpoints.py | 10 +- turnstone/api/console_schemas.py | 6 +- turnstone/console/server.py | 8 +- turnstone/console/static/admin.js | 12 +- .../console/static/coordinator/coordinator.js | 2 +- turnstone/console/static/index.html | 6 +- turnstone/core/history_decoration.py | 49 ++++-- turnstone/core/model_registry.py | 8 +- turnstone/core/providers/_anthropic.py | 9 +- turnstone/core/providers/_openai_chat.py | 9 +- turnstone/core/providers/_openai_responses.py | 9 +- turnstone/core/providers/_protocol.py | 45 +++++- turnstone/core/session_routes.py | 23 ++- turnstone/core/storage/_postgresql.py | 22 ++- turnstone/core/storage/_protocol.py | 2 +- turnstone/core/storage/_schema.py | 2 +- turnstone/core/storage/_sqlite.py | 22 ++- turnstone/core/storage/_utils.py | 2 +- .../052_model_reasoning_persistence.py | 19 ++- turnstone/sdk/events.py | 2 +- turnstone/server.py | 12 +- turnstone/ui/static/app.js | 2 +- 32 files changed, 442 insertions(+), 199 deletions(-) create mode 100644 tests/_session_helpers.py diff --git a/tests/_session_helpers.py b/tests/_session_helpers.py new file mode 100644 index 00000000..d1b1cca2 --- /dev/null +++ b/tests/_session_helpers.py @@ -0,0 +1,45 @@ +"""Shared session-test helpers. + +Two reasoning-test modules (``test_session_replay_reasoning.py`` and +``test_session_synth_reasoning_block.py``) need the same minimal +``ChatSession`` factory + a ``SessionUIBase`` no-op subclass. Hoisting +keeps a future third caller from drifting on the defaults — the third +existing ``_make_session`` (``test_model_registry.py``) deliberately +takes a different signature (registry / model_alias / reasoning_effort ++ ``_FakeUI``) and is NOT a candidate for sharing this helper. + +Module is named with a leading underscore so pytest doesn't try to +collect it as a test file — it's an importable utility, not a test. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock + +from turnstone.core.session import ChatSession +from turnstone.core.session_ui_base import SessionUIBase + + +class NullUI(SessionUIBase): + """Bare-bones UI satisfying the SessionUIBase contract for tests + that don't care about UI side effects.""" + + def __init__(self) -> None: + super().__init__() + + +def make_session(**kwargs: Any) -> ChatSession: + """Build a ChatSession with minimal defaults; tests override + individual fields via kwargs.""" + defaults: dict[str, Any] = { + "client": MagicMock(), + "model": "test-model", + "ui": NullUI(), + "instructions": None, + "temperature": 0.5, + "max_tokens": 4096, + "tool_timeout": 30, + } + defaults.update(kwargs) + return ChatSession(**defaults) diff --git a/tests/test_build_history_reminders.py b/tests/test_build_history_reminders.py index 832633c0..511eaa30 100644 --- a/tests/test_build_history_reminders.py +++ b/tests/test_build_history_reminders.py @@ -147,8 +147,8 @@ class _StubRegistry: """Minimal model registry — only ``get_config`` is read by ``_build_history``.""" - def __init__(self, persist_reasoning: bool = True) -> None: - self._cfg = SimpleNamespace(persist_reasoning=persist_reasoning) + def __init__(self, surface_persisted_reasoning: bool = True) -> None: + self._cfg = SimpleNamespace(surface_persisted_reasoning=surface_persisted_reasoning) def get_config(self, alias: str) -> Any: return self._cfg @@ -156,12 +156,12 @@ class _StubRegistry: def _build_with_registry( messages: list[dict[str, Any]], - persist_reasoning: bool = True, + surface_persisted_reasoning: bool = True, ) -> list[dict[str, Any]]: session = SimpleNamespace( messages=messages, _ws_id="ws-test", - _registry=_StubRegistry(persist_reasoning=persist_reasoning), + _registry=_StubRegistry(surface_persisted_reasoning=surface_persisted_reasoning), _model_alias="claude-opus-4-7", ) with patch( @@ -187,7 +187,7 @@ class TestReasoningSurfacing: {"type": "text", "text": "Final answer."}, ], } - history = _build_with_registry([msg], persist_reasoning=True) + history = _build_with_registry([msg], surface_persisted_reasoning=True) assert len(history) == 1 assert history[0]["reasoning"] == "let me think" @@ -199,7 +199,7 @@ class TestReasoningSurfacing: {"type": "thinking", "thinking": "hidden", "signature": "s"}, ], } - history = _build_with_registry([msg], persist_reasoning=False) + history = _build_with_registry([msg], surface_persisted_reasoning=False) assert "reasoning" not in history[0] def test_provider_content_never_in_wire_entry(self) -> None: @@ -212,12 +212,12 @@ class TestReasoningSurfacing: {"type": "thinking", "thinking": "x", "signature": "s"}, ], } - history = _build_with_registry([msg], persist_reasoning=True) + history = _build_with_registry([msg], surface_persisted_reasoning=True) assert "_provider_content" not in history[0] def test_no_reasoning_field_when_provider_content_missing(self) -> None: msg = {"role": "assistant", "content": "plain answer"} - history = _build_with_registry([msg], persist_reasoning=True) + history = _build_with_registry([msg], surface_persisted_reasoning=True) assert "reasoning" not in history[0] def test_no_reasoning_field_for_non_assistant_messages(self) -> None: @@ -232,7 +232,7 @@ class TestReasoningSurfacing: "_provider_content": [{"type": "thinking", "thinking": "leak", "signature": "s"}], }, ] - history = _build_with_registry(msgs, persist_reasoning=True) + history = _build_with_registry(msgs, surface_persisted_reasoning=True) assert "reasoning" not in history[0] assert "reasoning" not in history[1] diff --git a/tests/test_history_decoration.py b/tests/test_history_decoration.py index f65039e6..116ab83b 100644 --- a/tests/test_history_decoration.py +++ b/tests/test_history_decoration.py @@ -480,21 +480,21 @@ class TestExtractReasoningForHistory: from turnstone.core.history_decoration import extract_reasoning_for_history messages = [self._anthropic_thinking_msg("let me think")] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert messages[0]["reasoning"] == "let me think" def test_strips_provider_content_after_extraction(self) -> None: from turnstone.core.history_decoration import extract_reasoning_for_history messages = [self._anthropic_thinking_msg("anything")] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert "_provider_content" not in messages[0] def test_strips_provider_content_when_flag_false(self) -> None: from turnstone.core.history_decoration import extract_reasoning_for_history messages = [self._anthropic_thinking_msg("anything")] - extract_reasoning_for_history(messages, persist_reasoning_flag=False) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=False) # Strip is unconditional; reasoning is the conditional bit. assert "_provider_content" not in messages[0] assert "reasoning" not in messages[0] @@ -515,7 +515,7 @@ class TestExtractReasoningForHistory: ], } ] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert messages[0]["reasoning"] == "first" def test_first_block_reasoning_dispatches_to_openai_responses(self) -> None: @@ -535,7 +535,7 @@ class TestExtractReasoningForHistory: ], } ] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert messages[0]["reasoning"] == "s" assert "_provider_content" not in messages[0] @@ -549,7 +549,7 @@ class TestExtractReasoningForHistory: "_provider_content": [{"type": "text", "text": "no reasoning here"}], } ] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert "reasoning" not in messages[0] assert "_provider_content" not in messages[0] @@ -557,7 +557,7 @@ class TestExtractReasoningForHistory: from turnstone.core.history_decoration import extract_reasoning_for_history messages = [{"role": "assistant", "content": "plain"}] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert "reasoning" not in messages[0] assert messages[0]["content"] == "plain" @@ -569,7 +569,7 @@ class TestExtractReasoningForHistory: {"role": "tool", "tool_call_id": "c1", "content": "out"}, self._anthropic_thinking_msg("only this one"), ] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert "reasoning" not in messages[0] assert "reasoning" not in messages[1] assert messages[2]["reasoning"] == "only this one" @@ -578,7 +578,7 @@ class TestExtractReasoningForHistory: from turnstone.core.history_decoration import extract_reasoning_for_history messages = [{"role": "assistant", "content": "x", "_provider_content": []}] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert "reasoning" not in messages[0] # Empty-list provider_content is still stripped from the wire. assert "_provider_content" not in messages[0] @@ -593,7 +593,7 @@ class TestExtractReasoningForHistory: "_provider_content": ["bogus"], } ] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert "reasoning" not in messages[0] assert "_provider_content" not in messages[0] @@ -613,6 +613,35 @@ class TestExtractReasoningForHistory: ], } ] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert messages[0]["reasoning"] == "synth thought" assert "_provider_content" not in messages[0] + + def test_first_block_redacted_thinking_dispatches_to_anthropic(self) -> None: + # Anthropic's extended-thinking API documents that + # ``redacted_thinking`` blocks (sealed by the safety system) + # can appear before, after, or interleaved with regular + # ``thinking`` blocks. When the redacted block lands first, + # the dispatcher must still route to AnthropicProvider so the + # surrounding real thinking text surfaces — without this the + # reasoning bubble silently disappears on history rehydration. + # Pinned by registering "redacted_thinking" as a second key + # in _BLOCK_TYPE_PROVIDER_FACTORY pointing at the Anthropic + # factory; Anthropic's extractor's type=="thinking" filter + # already correctly skips the redacted block. + from turnstone.core.history_decoration import extract_reasoning_for_history + + messages = [ + { + "role": "assistant", + "content": "answer", + "_provider_content": [ + {"type": "redacted_thinking", "data": "sealed-blob"}, + {"type": "thinking", "thinking": "real thought", "signature": "s"}, + {"type": "text", "text": "answer"}, + ], + } + ] + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) + assert messages[0]["reasoning"] == "real thought" + assert "_provider_content" not in messages[0] diff --git a/tests/test_model_definition_storage.py b/tests/test_model_definition_storage.py index ea3b2dd5..bc44776b 100644 --- a/tests/test_model_definition_storage.py +++ b/tests/test_model_definition_storage.py @@ -214,12 +214,12 @@ class TestModelDefinitionStorage: assert m["temperature"] is None def test_reasoning_flags_default(self, db: SQLiteBackend) -> None: - """persist_reasoning defaults True; replay_reasoning_to_model defaults False.""" + """surface_persisted_reasoning defaults True; replay_reasoning_to_model defaults False.""" did = _make_id() db.create_model_definition(definition_id=did, alias="reason-default", model="gpt-5") m = db.get_model_definition(did) assert m is not None - assert m["persist_reasoning"] is True + assert m["surface_persisted_reasoning"] is True assert m["replay_reasoning_to_model"] is False def test_create_with_explicit_reasoning_flags(self, db: SQLiteBackend) -> None: @@ -228,27 +228,27 @@ class TestModelDefinitionStorage: definition_id=did, alias="reason-explicit", model="claude-opus-4-7", - persist_reasoning=False, + surface_persisted_reasoning=False, replay_reasoning_to_model=True, ) m = db.get_model_definition(did) assert m is not None - assert m["persist_reasoning"] is False + assert m["surface_persisted_reasoning"] is False assert m["replay_reasoning_to_model"] is True # Same values must round-trip via the alias lookup too. m_alias = db.get_model_definition_by_alias("reason-explicit") assert m_alias is not None - assert m_alias["persist_reasoning"] is False + assert m_alias["surface_persisted_reasoning"] is False assert m_alias["replay_reasoning_to_model"] is True - def test_update_persist_reasoning(self, db: SQLiteBackend) -> None: + def test_update_surface_persisted_reasoning(self, db: SQLiteBackend) -> None: did = _make_id() db.create_model_definition(definition_id=did, alias="upd-persist", model="gpt-5") - ok = db.update_model_definition(did, persist_reasoning=False) + ok = db.update_model_definition(did, surface_persisted_reasoning=False) assert ok is True m = db.get_model_definition(did) assert m is not None - assert m["persist_reasoning"] is False + assert m["surface_persisted_reasoning"] is False assert m["replay_reasoning_to_model"] is False # untouched def test_update_replay_reasoning_to_model(self, db: SQLiteBackend) -> None: @@ -258,7 +258,7 @@ class TestModelDefinitionStorage: assert ok is True m = db.get_model_definition(did) assert m is not None - assert m["persist_reasoning"] is True # untouched + assert m["surface_persisted_reasoning"] is True # untouched assert m["replay_reasoning_to_model"] is True def test_list_returns_reasoning_flags(self, db: SQLiteBackend) -> None: @@ -266,19 +266,19 @@ class TestModelDefinitionStorage: definition_id=_make_id(), alias="list-a", model="gpt-5", - persist_reasoning=True, + surface_persisted_reasoning=True, replay_reasoning_to_model=False, ) db.create_model_definition( definition_id=_make_id(), alias="list-b", model="claude-opus-4-7", - persist_reasoning=False, + surface_persisted_reasoning=False, replay_reasoning_to_model=True, ) models = db.list_model_definitions() by_alias = {m["alias"]: m for m in models} - assert by_alias["list-a"]["persist_reasoning"] is True + assert by_alias["list-a"]["surface_persisted_reasoning"] is True assert by_alias["list-a"]["replay_reasoning_to_model"] is False - assert by_alias["list-b"]["persist_reasoning"] is False + assert by_alias["list-b"]["surface_persisted_reasoning"] is False assert by_alias["list-b"]["replay_reasoning_to_model"] is True diff --git a/tests/test_model_registry.py b/tests/test_model_registry.py index 85d53e10..fba575ad 100644 --- a/tests/test_model_registry.py +++ b/tests/test_model_registry.py @@ -78,7 +78,7 @@ class TestModelConfig: def test_reasoning_flags_default(self) -> None: cfg = ModelConfig(alias="x", base_url="x", api_key="x", model="x") - assert cfg.persist_reasoning is True + assert cfg.surface_persisted_reasoning is True assert cfg.replay_reasoning_to_model is False def test_reasoning_flags_set(self) -> None: @@ -87,10 +87,10 @@ class TestModelConfig: base_url="x", api_key="x", model="x", - persist_reasoning=False, + surface_persisted_reasoning=False, replay_reasoning_to_model=True, ) - assert cfg.persist_reasoning is False + assert cfg.surface_persisted_reasoning is False assert cfg.replay_reasoning_to_model is True @@ -716,7 +716,7 @@ class TestLoadModelRegistryWithDB: "context_window": 200000, "capabilities": "{}", "enabled": True, - "persist_reasoning": False, + "surface_persisted_reasoning": False, "replay_reasoning_to_model": True, } ] @@ -724,7 +724,7 @@ class TestLoadModelRegistryWithDB: with patch("turnstone.core.model_registry.load_config", return_value={}): reg = load_model_registry("http://x/v1", "x", "x", storage=storage) cfg = reg.get_config("anth-thinking") - assert cfg.persist_reasoning is False + assert cfg.surface_persisted_reasoning is False assert cfg.replay_reasoning_to_model is True def test_db_reasoning_flags_default_when_absent(self) -> None: @@ -740,14 +740,14 @@ class TestLoadModelRegistryWithDB: "context_window": 32768, "capabilities": "{}", "enabled": True, - # persist_reasoning + replay_reasoning_to_model intentionally absent + # surface_persisted_reasoning + replay_reasoning_to_model intentionally absent } ] ) with patch("turnstone.core.model_registry.load_config", return_value={}): reg = load_model_registry("http://x/v1", "x", "x", storage=storage) cfg = reg.get_config("legacy-row") - assert cfg.persist_reasoning is True + assert cfg.surface_persisted_reasoning is True assert cfg.replay_reasoning_to_model is False def test_db_default_alias_not_clobbered(self) -> None: diff --git a/tests/test_provider_anthropic_reasoning.py b/tests/test_provider_anthropic_reasoning.py index 21bec44a..389e3f41 100644 --- a/tests/test_provider_anthropic_reasoning.py +++ b/tests/test_provider_anthropic_reasoning.py @@ -21,7 +21,7 @@ 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_BYTES as _MAX_REASONING_DISPLAY_BYTES, + MAX_REASONING_DISPLAY_CHARS as _MAX_REASONING_DISPLAY_CHARS, ) @@ -77,13 +77,13 @@ class TestExtractReasoningText: assert anthropic.extract_reasoning_text(blocks) == "" def test_truncation_at_64kib_cap(self, anthropic: AnthropicProvider) -> None: - long_text = "x" * (_MAX_REASONING_DISPLAY_BYTES + 1024) + 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_BYTES + assert len(result) == _MAX_REASONING_DISPLAY_CHARS def test_just_under_cap_not_truncated(self, anthropic: AnthropicProvider) -> None: - text = "y" * (_MAX_REASONING_DISPLAY_BYTES - 1) + text = "y" * (_MAX_REASONING_DISPLAY_CHARS - 1) blocks = [{"type": "thinking", "thinking": text, "signature": "s"}] assert anthropic.extract_reasoning_text(blocks) == text diff --git a/tests/test_provider_openai_responses_reasoning.py b/tests/test_provider_openai_responses_reasoning.py index 1a712a9a..84f841be 100644 --- a/tests/test_provider_openai_responses_reasoning.py +++ b/tests/test_provider_openai_responses_reasoning.py @@ -22,7 +22,7 @@ from turnstone.core.providers._openai_responses import ( _reasoning_item_for_input, ) from turnstone.core.providers._protocol import ( - MAX_REASONING_DISPLAY_BYTES as _MAX_REASONING_DISPLAY_BYTES, + MAX_REASONING_DISPLAY_CHARS as _MAX_REASONING_DISPLAY_CHARS, ) from turnstone.core.providers._protocol import ModelCapabilities @@ -98,7 +98,7 @@ class TestExtractReasoningText: assert "raw reasoning" in result def test_truncation_at_64kib_cap(self, provider: OpenAIResponsesProvider) -> None: - long_text = "x" * (_MAX_REASONING_DISPLAY_BYTES + 1024) + long_text = "x" * (_MAX_REASONING_DISPLAY_CHARS + 1024) blocks = [ { "type": "reasoning", @@ -107,7 +107,7 @@ class TestExtractReasoningText: } ] result = provider.extract_reasoning_text(blocks) - assert len(result) == _MAX_REASONING_DISPLAY_BYTES + assert len(result) == _MAX_REASONING_DISPLAY_CHARS def test_malformed_summary_entry_skipped(self, provider: OpenAIResponsesProvider) -> None: blocks = [ diff --git a/tests/test_reasoning_audit_log_discipline.py b/tests/test_reasoning_audit_log_discipline.py index 630682ee..f1e90017 100644 --- a/tests/test_reasoning_audit_log_discipline.py +++ b/tests/test_reasoning_audit_log_discipline.py @@ -28,11 +28,15 @@ from types import SimpleNamespace from typing import Any from unittest.mock import patch +from tests._session_helpers import make_session from turnstone.core.history_decoration import ( extract_reasoning_for_history, extract_reasoning_text_from_provider_content, ) 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 StreamChunk, UsageInfo from turnstone.server import _build_history _MARKER = "SECRET_REASONING_MARKER_xyz123_unlikely_collision" @@ -151,7 +155,7 @@ class TestReasoningAuditLogDiscipline: p.start() try: messages = [self._thinking_msg(_MARKER)] - extract_reasoning_for_history(messages, persist_reasoning_flag=True) + extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True) assert messages[0]["reasoning"] == _MARKER # UI-bound is allowed finally: for p in patchers: @@ -166,7 +170,9 @@ class TestReasoningAuditLogDiscipline: ) def test_build_history_does_not_log_reasoning(self) -> None: - registry = SimpleNamespace(get_config=lambda alias: SimpleNamespace(persist_reasoning=True)) + registry = SimpleNamespace( + get_config=lambda alias: SimpleNamespace(surface_persisted_reasoning=True) + ) session = SimpleNamespace( messages=[self._thinking_msg(_MARKER)], _ws_id="ws-audit", @@ -192,3 +198,136 @@ class TestReasoningAuditLogDiscipline: if _payload_contains_marker(args, kwargs) ] assert offending == [], f"_build_history leaked reasoning text into INFO+ logs: {offending}" + + # ------------------------------------------------------------------ + # Phase 2 + Phase 3 surfaces — added in response to a code-review + # finding that the original 4-test coverage missed every code path + # introduced after Phase 1. Each new test mirrors the structure + # above: capture every Logger.info / warning / error call across + # the operation, assert the marker doesn't appear in any captured + # payload (UI-bound returns IS allowed; logging at INFO+ is NOT). + # ------------------------------------------------------------------ + + def test_openai_responses_extractor_does_not_log_reasoning(self) -> None: + captured, patchers = _capture_log_calls() + for p in patchers: + p.start() + try: + provider = OpenAIResponsesProvider() + blocks = [ + { + "type": "reasoning", + "id": "r_1", + "summary": [{"type": "summary_text", "text": _MARKER}], + } + ] + text = provider.extract_reasoning_text(blocks) + assert _MARKER in text # UI-bound return is allowed + finally: + for p in patchers: + p.stop() + offending = [ + (lvl, args, kwargs) + for lvl, args, kwargs in captured + if _payload_contains_marker(args, kwargs) + ] + assert offending == [], ( + f"OpenAIResponsesProvider.extract_reasoning_text leaked reasoning " + f"text into INFO+ logs: {offending}" + ) + + def test_openai_chat_extractor_does_not_log_reasoning(self) -> None: + captured, patchers = _capture_log_calls() + for p in patchers: + p.start() + try: + provider = OpenAIChatCompletionsProvider() + blocks = [{"type": "reasoning_text", "text": _MARKER, "source": "vllm"}] + text = provider.extract_reasoning_text(blocks) + assert text == _MARKER + finally: + for p in patchers: + p.stop() + offending = [ + (lvl, args, kwargs) + for lvl, args, kwargs in captured + if _payload_contains_marker(args, kwargs) + ] + assert offending == [], ( + f"OpenAIChatCompletionsProvider.extract_reasoning_text leaked " + f"reasoning text into INFO+ logs: {offending}" + ) + + def test_synth_reasoning_block_via_stream_response_does_not_log_reasoning( + self, + ) -> None: + """Drives ChatSession._stream_response (which calls + _maybe_synth_reasoning_block at end-of-stream) with a fake + ``reasoning_delta=_MARKER`` chunk; asserts no log call carried + the marker text.""" + session = make_session() + chunks = [ + StreamChunk(reasoning_delta=_MARKER, is_first=True), + StreamChunk(content_delta="answer"), + StreamChunk( + finish_reason="stop", + usage=UsageInfo(prompt_tokens=10, completion_tokens=20, total_tokens=30), + ), + ] + captured, patchers = _capture_log_calls() + for p in patchers: + p.start() + try: + msg = session._stream_response(iter(chunks)) + # Synth block stamped onto _provider_content with the marker. + assert msg["_provider_content"][0]["text"] == _MARKER + finally: + for p in patchers: + p.stop() + offending = [ + (lvl, args, kwargs) + for lvl, args, kwargs in captured + if _payload_contains_marker(args, kwargs) + ] + assert offending == [], ( + f"_stream_response + _maybe_synth_reasoning_block leaked reasoning " + f"text into INFO+ logs: {offending}" + ) + + def test_anthropic_convert_messages_strip_does_not_log_reasoning(self) -> None: + """Drives the Phase 2 strip predicate + (``replay_reasoning_to_model=False``) which walks thinking + blocks to filter them out before the wire payload is built; + asserts no log call carried the marker text.""" + captured, patchers = _capture_log_calls() + for p in patchers: + p.start() + try: + provider = AnthropicProvider() + messages = [ + { + "role": "assistant", + "content": "Final answer.", + "_provider_content": [ + {"type": "thinking", "thinking": _MARKER, "signature": "s"}, + {"type": "text", "text": "Final answer."}, + ], + }, + ] + _, converted = provider._convert_messages(messages, replay_reasoning_to_model=False) + # Strip fired — thinking block dropped from wire. + assistant = next(m for m in converted if m["role"] == "assistant") + block_types = [b.get("type") for b in assistant["content"]] + assert "thinking" not in block_types + finally: + for p in patchers: + p.stop() + offending = [ + (lvl, args, kwargs) + for lvl, args, kwargs in captured + if _payload_contains_marker(args, kwargs) + ] + assert offending == [], ( + f"AnthropicProvider._convert_messages strip predicate leaked " + f"reasoning text into INFO+ logs: {offending}" + ) diff --git a/tests/test_session_replay_reasoning.py b/tests/test_session_replay_reasoning.py index e018ed20..3844c5fd 100644 --- a/tests/test_session_replay_reasoning.py +++ b/tests/test_session_replay_reasoning.py @@ -26,29 +26,7 @@ from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock, patch -from turnstone.core.session import ChatSession -from turnstone.core.session_ui_base import SessionUIBase - - -class _NullUI(SessionUIBase): - """Bare-bones UI satisfying the SessionUIBase contract for these tests.""" - - def __init__(self) -> None: - super().__init__() - - -def _make_session(**kwargs: Any) -> ChatSession: - defaults: dict[str, Any] = { - "client": MagicMock(), - "model": "test-model", - "ui": _NullUI(), - "instructions": None, - "temperature": 0.5, - "max_tokens": 4096, - "tool_timeout": 30, - } - defaults.update(kwargs) - return ChatSession(**defaults) +from tests._session_helpers import make_session as _make_session def _registry_with_flag(persist: bool = True, replay: bool = False) -> Any: @@ -56,7 +34,7 @@ def _registry_with_flag(persist: bool = True, replay: bool = False) -> Any: flags under test.""" return SimpleNamespace( get_config=lambda alias: SimpleNamespace( - persist_reasoning=persist, + surface_persisted_reasoning=persist, replay_reasoning_to_model=replay, ) ) diff --git a/tests/test_session_synth_reasoning_block.py b/tests/test_session_synth_reasoning_block.py index d8d539ff..7345dbac 100644 --- a/tests/test_session_synth_reasoning_block.py +++ b/tests/test_session_synth_reasoning_block.py @@ -26,34 +26,13 @@ from __future__ import annotations from types import SimpleNamespace from typing import Any -from unittest.mock import MagicMock +from tests._session_helpers import make_session as _make_session from turnstone.core.providers._anthropic import ( ANTHROPIC_VALID_BLOCK_TYPES, AnthropicProvider, ) from turnstone.core.providers._openai_chat import OpenAIChatCompletionsProvider -from turnstone.core.session import ChatSession -from turnstone.core.session_ui_base import SessionUIBase - - -class _NullUI(SessionUIBase): - def __init__(self) -> None: - super().__init__() - - -def _make_session(**kwargs: Any) -> ChatSession: - defaults: dict[str, Any] = { - "client": MagicMock(), - "model": "test-model", - "ui": _NullUI(), - "instructions": None, - "temperature": 0.5, - "max_tokens": 4096, - "tool_timeout": 30, - } - defaults.update(kwargs) - return ChatSession(**defaults) class TestMaybeSynthReasoningBlock: diff --git a/tests/test_workstream_endpoints.py b/tests/test_workstream_endpoints.py index d033c374..881fe1d1 100644 --- a/tests/test_workstream_endpoints.py +++ b/tests/test_workstream_endpoints.py @@ -1788,7 +1788,7 @@ class TestHistoryReasoningRehydration: ws_id, "assistant", "Final answer.", provider_data=provider_data ) # No live session — exercises the storage-only path which - # falls back to default persist_reasoning=True. + # falls back to default surface_persisted_reasoning=True. mock_mgr = MagicMock() mock_mgr.get.return_value = None client = _build_history_app(mock_mgr, _inject_storage) @@ -1814,7 +1814,7 @@ class TestHistoryReasoningRehydration: assert "_provider_content" not in m def test_history_handler_with_persist_flag_false_via_live_session(self, _inject_storage): - """Operator-flipped ``persist_reasoning=False`` on the active + """Operator-flipped ``surface_persisted_reasoning=False`` on the active model suppresses the reasoning field even when the data is stored. ``_provider_content`` is still stripped from the wire. """ @@ -1825,7 +1825,7 @@ class TestHistoryReasoningRehydration: live_session = SimpleNamespace( id=ws_id, _registry=SimpleNamespace( - get_config=lambda alias: SimpleNamespace(persist_reasoning=False) + get_config=lambda alias: SimpleNamespace(surface_persisted_reasoning=False) ), _model_alias="claude-opus-4-7", ) @@ -1844,7 +1844,7 @@ class TestHistoryReasoningRehydration: """Cold workstream (no live session) — the handler walks ``workstream_config.model_alias`` (persisted at first send by the SessionManager rehydrate path) and looks up the active - model's ``persist_reasoning`` flag through the global registry + model's ``surface_persisted_reasoning`` flag through the global registry on ``app.state``. Operator flag-flip is honored uniformly across live and cold workstreams. """ @@ -1884,7 +1884,7 @@ class TestHistoryReasoningRehydration: app.state.auth_storage = _inject_storage app.state.registry = SimpleNamespace( get_config=lambda alias: SimpleNamespace( - persist_reasoning=(alias != "claude-opus-4-7"), + surface_persisted_reasoning=(alias != "claude-opus-4-7"), ) ) client = TestClient(app) diff --git a/turnstone/api/console_schemas.py b/turnstone/api/console_schemas.py index 96266d33..9eb583f3 100644 --- a/turnstone/api/console_schemas.py +++ b/turnstone/api/console_schemas.py @@ -908,7 +908,7 @@ class ModelDefinitionInfo(BaseModel): temperature: float | None = None max_tokens: int | None = None reasoning_effort: str | None = None - persist_reasoning: bool = True + surface_persisted_reasoning: bool = True replay_reasoning_to_model: bool = False source: str = "" created_by: str = "" @@ -928,7 +928,7 @@ class CreateModelDefinitionRequest(BaseModel): temperature: float | None = None max_tokens: int | None = None reasoning_effort: str | None = None - persist_reasoning: bool = True + surface_persisted_reasoning: bool = True replay_reasoning_to_model: bool = False @@ -944,7 +944,7 @@ class UpdateModelDefinitionRequest(BaseModel): temperature: float | None = None max_tokens: int | None = None reasoning_effort: str | None = None - persist_reasoning: bool | None = None + surface_persisted_reasoning: bool | None = None replay_reasoning_to_model: bool | None = None diff --git a/turnstone/console/server.py b/turnstone/console/server.py index f2c61d6d..c50ffeed 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -9904,7 +9904,7 @@ async def admin_create_model_definition(request: Request) -> JSONResponse: if not reasoning_effort: reasoning_effort = None - persist_reasoning = bool(body.get("persist_reasoning", True)) + surface_persisted_reasoning = bool(body.get("surface_persisted_reasoning", True)) replay_reasoning_to_model = bool(body.get("replay_reasoning_to_model", False)) storage.create_model_definition( @@ -9921,7 +9921,7 @@ async def admin_create_model_definition(request: Request) -> JSONResponse: temperature=temperature, max_tokens=max_tokens, reasoning_effort=reasoning_effort, - persist_reasoning=persist_reasoning, + surface_persisted_reasoning=surface_persisted_reasoning, replay_reasoning_to_model=replay_reasoning_to_model, ) @@ -10084,8 +10084,8 @@ async def admin_update_model_definition(request: Request) -> JSONResponse: ) else: updates["reasoning_effort"] = re_val - if "persist_reasoning" in body: - updates["persist_reasoning"] = bool(body["persist_reasoning"]) + if "surface_persisted_reasoning" in body: + updates["surface_persisted_reasoning"] = bool(body["surface_persisted_reasoning"]) if "replay_reasoning_to_model" in body: updates["replay_reasoning_to_model"] = bool(body["replay_reasoning_to_model"]) diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index b88e69f1..708c422a 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -5011,7 +5011,7 @@ function _renderModels(items) { // Reasoning persistence flags surface only when non-default // (persist=False is the operator opt-out; replay=True is the // operator opt-in). Default values are silent. - if (m.persist_reasoning === false) overrides.push("persist=off"); + if (m.surface_persisted_reasoning === false) overrides.push("surface=off"); if (m.replay_reasoning_to_model === true) overrides.push("replay=on"); if (overrides.length) { var ovrSpan = document.createElement("span"); @@ -5201,7 +5201,7 @@ function showCreateModelModal() { el.style.borderColor = ""; }); document.getElementById("model-enabled").checked = true; - document.getElementById("model-persist-reasoning").checked = true; + document.getElementById("model-surface-persisted-reasoning").checked = true; document.getElementById("model-replay-reasoning").checked = false; document.getElementById("model-detect-result").style.display = "none"; document.getElementById("model-detect-btn").disabled = false; @@ -5288,8 +5288,8 @@ function showEditModelModal(definitionId) { // Reasoning persistence flags — defaults match the dataclass // defaults (persist=true, replay=false) when the API returns // them as undefined (legacy / pre-052 row). - document.getElementById("model-persist-reasoning").checked = - m.persist_reasoning !== false; + document.getElementById("model-surface-persisted-reasoning").checked = + m.surface_persisted_reasoning !== false; document.getElementById("model-replay-reasoning").checked = m.replay_reasoning_to_model === true; _applyProviderDefaults(); @@ -5436,8 +5436,8 @@ function submitCreateModel() { // Reasoning persistence flags — always serialize so a flip from // default takes effect on PUT (the server's update path keys off // "field present in body"). - form.persist_reasoning = document.getElementById( - "model-persist-reasoning", + form.surface_persisted_reasoning = document.getElementById( + "model-surface-persisted-reasoning", ).checked; form.replay_reasoning_to_model = document.getElementById( "model-replay-reasoning", diff --git a/turnstone/console/static/coordinator/coordinator.js b/turnstone/console/static/coordinator/coordinator.js index ca2d7941..641ce509 100644 --- a/turnstone/console/static/coordinator/coordinator.js +++ b/turnstone/console/static/coordinator/coordinator.js @@ -4202,7 +4202,7 @@ // snapshot ":2021" call sites — same appendMsg("reasoning") // helper, just driven from history-render rather than the // SSE handler. Only present when the active model's - // persist_reasoning flag is true and the message round-tripped + // surface_persisted_reasoning flag is true and the message round-tripped // a thinking lane. if (typeof m.reasoning === "string" && m.reasoning.length) { const rEl = appendMsg("reasoning", "", { label: "reasoning" }); diff --git a/turnstone/console/static/index.html b/turnstone/console/static/index.html index ea5791c2..6f6a256f 100644 --- a/turnstone/console/static/index.html +++ b/turnstone/console/static/index.html @@ -4127,13 +4127,13 @@ > Surface persisted reasoning