mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
Patrick Buckley
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.
285 lines
12 KiB
Python
285 lines
12 KiB
Python
"""Tests for model definition storage CRUD operations."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import uuid
|
|
from typing import TYPE_CHECKING
|
|
|
|
if TYPE_CHECKING:
|
|
from turnstone.core.storage._sqlite import SQLiteBackend
|
|
|
|
|
|
def _make_id() -> str:
|
|
return uuid.uuid4().hex
|
|
|
|
|
|
class TestModelDefinitionStorage:
|
|
def test_create_and_get(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(
|
|
definition_id=did,
|
|
alias="test-model",
|
|
model="gpt-5",
|
|
provider="openai",
|
|
base_url="https://api.openai.com/v1",
|
|
api_key="sk-test",
|
|
context_window=128000,
|
|
)
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["alias"] == "test-model"
|
|
assert m["model"] == "gpt-5"
|
|
assert m["provider"] == "openai"
|
|
assert m["base_url"] == "https://api.openai.com/v1"
|
|
assert m["api_key"] == "sk-test"
|
|
assert m["context_window"] == 128000
|
|
assert m["capabilities"] == "{}"
|
|
assert m["enabled"] is True
|
|
|
|
def test_get_by_alias(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(definition_id=did, alias="by-alias", model="gpt-5")
|
|
m = db.get_model_definition_by_alias("by-alias")
|
|
assert m is not None
|
|
assert m["definition_id"] == did
|
|
|
|
def test_get_by_alias_not_found(self, db: SQLiteBackend) -> None:
|
|
assert db.get_model_definition_by_alias("nope") is None
|
|
|
|
def test_get_not_found(self, db: SQLiteBackend) -> None:
|
|
assert db.get_model_definition("nonexistent") is None
|
|
|
|
def test_list_empty(self, db: SQLiteBackend) -> None:
|
|
assert db.list_model_definitions() == []
|
|
|
|
def test_list_all(self, db: SQLiteBackend) -> None:
|
|
db.create_model_definition(definition_id=_make_id(), alias="alpha", model="gpt-5")
|
|
db.create_model_definition(
|
|
definition_id=_make_id(), alias="beta", model="claude-opus-4-6", provider="anthropic"
|
|
)
|
|
models = db.list_model_definitions()
|
|
assert len(models) == 2
|
|
assert models[0]["alias"] == "alpha" # ordered by alias
|
|
assert models[1]["alias"] == "beta"
|
|
|
|
def test_list_enabled_only(self, db: SQLiteBackend) -> None:
|
|
db.create_model_definition(
|
|
definition_id=_make_id(), alias="enabled-model", model="gpt-5", enabled=True
|
|
)
|
|
db.create_model_definition(
|
|
definition_id=_make_id(), alias="disabled-model", model="gpt-5", enabled=False
|
|
)
|
|
enabled = db.list_model_definitions(enabled_only=True)
|
|
assert len(enabled) == 1
|
|
assert enabled[0]["alias"] == "enabled-model"
|
|
|
|
def test_update_basic_fields(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(
|
|
definition_id=did, alias="orig", model="gpt-5", base_url="http://old"
|
|
)
|
|
ok = db.update_model_definition(did, alias="renamed", base_url="http://new")
|
|
assert ok is True
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["alias"] == "renamed"
|
|
assert m["base_url"] == "http://new"
|
|
|
|
def test_update_boolean_conversion(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(definition_id=did, alias="booltest", model="gpt-5")
|
|
db.update_model_definition(did, enabled=False)
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["enabled"] is False
|
|
|
|
def test_update_not_found(self, db: SQLiteBackend) -> None:
|
|
ok = db.update_model_definition("nonexistent", alias="x")
|
|
assert ok is False
|
|
|
|
def test_update_ignores_disallowed_fields(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(
|
|
definition_id=did, alias="guard", model="gpt-5", created_by="admin"
|
|
)
|
|
original = db.get_model_definition(did)
|
|
assert original is not None
|
|
original_created = original["created"]
|
|
# created_by and created are not in the mutable allowlist
|
|
db.update_model_definition(did, created_by="evil", created="2000-01-01T00:00:00")
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["created_by"] == "admin" # unchanged
|
|
assert m["created"] == original_created # unchanged
|
|
|
|
def test_delete(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(definition_id=did, alias="delme", model="gpt-5")
|
|
ok = db.delete_model_definition(did)
|
|
assert ok is True
|
|
assert db.get_model_definition(did) is None
|
|
|
|
def test_delete_not_found(self, db: SQLiteBackend) -> None:
|
|
ok = db.delete_model_definition("nonexistent")
|
|
assert ok is False
|
|
|
|
def test_create_duplicate_alias(self, db: SQLiteBackend) -> None:
|
|
db.create_model_definition(definition_id=_make_id(), alias="unique", model="gpt-5")
|
|
# Second create with same alias but different ID should be no-op (OR IGNORE)
|
|
did2 = _make_id()
|
|
db.create_model_definition(definition_id=did2, alias="unique", model="gpt-5")
|
|
assert db.get_model_definition(did2) is None
|
|
|
|
def test_create_idempotent_same_id(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(definition_id=did, alias="idem", model="gpt-5")
|
|
db.create_model_definition(definition_id=did, alias="idem", model="gpt-5-mini")
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["model"] == "gpt-5" # original preserved
|
|
|
|
def test_capabilities_json(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
caps = '{"supports_vision": true, "supports_web_search": false}'
|
|
db.create_model_definition(
|
|
definition_id=did, alias="caps-test", model="gpt-5", capabilities=caps
|
|
)
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["capabilities"] == caps
|
|
|
|
def test_defaults(self, db: SQLiteBackend) -> None:
|
|
"""Verify default values for optional fields."""
|
|
did = _make_id()
|
|
db.create_model_definition(definition_id=did, alias="defaults", model="gpt-5")
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["provider"] == "openai"
|
|
assert m["base_url"] == ""
|
|
assert m["api_key"] == ""
|
|
assert m["context_window"] == 32768
|
|
assert m["capabilities"] == "{}"
|
|
assert m["enabled"] is True
|
|
assert m["created_by"] == ""
|
|
# Per-model sampling params default to None (use global default)
|
|
assert m["temperature"] is None
|
|
assert m["max_tokens"] is None
|
|
assert m["reasoning_effort"] is None
|
|
|
|
def test_create_with_sampling_params(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(
|
|
definition_id=did,
|
|
alias="sampling",
|
|
model="gpt-5",
|
|
temperature=0.7,
|
|
max_tokens=8192,
|
|
reasoning_effort="high",
|
|
)
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["temperature"] == 0.7
|
|
assert m["max_tokens"] == 8192
|
|
assert m["reasoning_effort"] == "high"
|
|
|
|
def test_create_with_zero_temperature(self, db: SQLiteBackend) -> None:
|
|
"""temperature=0.0 is a valid override, distinct from None."""
|
|
did = _make_id()
|
|
db.create_model_definition(
|
|
definition_id=did, alias="zero-temp", model="o3", temperature=0.0
|
|
)
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["temperature"] == 0.0
|
|
|
|
def test_update_sampling_params(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(definition_id=did, alias="upd-samp", model="gpt-5")
|
|
db.update_model_definition(did, temperature=1.2, max_tokens=4096, reasoning_effort="low")
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["temperature"] == 1.2
|
|
assert m["max_tokens"] == 4096
|
|
assert m["reasoning_effort"] == "low"
|
|
|
|
def test_clear_sampling_params(self, db: SQLiteBackend) -> None:
|
|
"""Setting sampling params to None clears them back to global default."""
|
|
did = _make_id()
|
|
db.create_model_definition(
|
|
definition_id=did, alias="clear-samp", model="gpt-5", temperature=0.9
|
|
)
|
|
db.update_model_definition(did, temperature=None)
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
assert m["temperature"] is None
|
|
|
|
def test_reasoning_flags_default(self, db: SQLiteBackend) -> None:
|
|
"""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["surface_persisted_reasoning"] is True
|
|
assert m["replay_reasoning_to_model"] is False
|
|
|
|
def test_create_with_explicit_reasoning_flags(self, db: SQLiteBackend) -> None:
|
|
did = _make_id()
|
|
db.create_model_definition(
|
|
definition_id=did,
|
|
alias="reason-explicit",
|
|
model="claude-opus-4-7",
|
|
surface_persisted_reasoning=False,
|
|
replay_reasoning_to_model=True,
|
|
)
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
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["surface_persisted_reasoning"] is False
|
|
assert m_alias["replay_reasoning_to_model"] is True
|
|
|
|
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, surface_persisted_reasoning=False)
|
|
assert ok is True
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
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:
|
|
did = _make_id()
|
|
db.create_model_definition(definition_id=did, alias="upd-replay", model="gpt-5")
|
|
ok = db.update_model_definition(did, replay_reasoning_to_model=True)
|
|
assert ok is True
|
|
m = db.get_model_definition(did)
|
|
assert m is not None
|
|
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:
|
|
db.create_model_definition(
|
|
definition_id=_make_id(),
|
|
alias="list-a",
|
|
model="gpt-5",
|
|
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",
|
|
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"]["surface_persisted_reasoning"] is True
|
|
assert by_alias["list-a"]["replay_reasoning_to_model"] is False
|
|
assert by_alias["list-b"]["surface_persisted_reasoning"] is False
|
|
assert by_alias["list-b"]["replay_reasoning_to_model"] is True
|