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.
46 lines
1.5 KiB
Python
46 lines
1.5 KiB
Python
"""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)
|