mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
Patrick Buckley
e8352bd8e5
PR #498 round-robin review surfaced 5 findings. 4 applied; 1 rejected with rationale. Applied * **Copilot finding 5** (history_decoration.py:341): dispatcher inspected only ``provider_content[0]['type']``. OpenAI Responses captures EVERY ``output_item.done`` event into ``provider_blocks`` (not just reasoning) — in practice the order is ``[reasoning, message, ...]`` but the API doesn't guarantee that; a hypothetical ``[message, reasoning]`` ordering would silently drop the reasoning under an index-only check. Now walks the list for the first block whose type is in ``_BLOCK_TYPE_PROVIDER_FACTORY``, then dispatches the WHOLE list to that provider's extractor. Each provider's extractor already filters internally by its own block type, so passing the full list is correct. Regression test added (``test_dispatcher_scans_past_unrecognized_first_blocks``). * **Copilot finding 3** (migration 052 docstring): the previous review-fix wave used sed to rename ``persist_reasoning`` → ``surface_persisted_reasoning`` everywhere, which mangled a historical reference in the migration docstring ("The earlier name ``surface_persisted_reasoning`` was renamed..."). Restored to point at the actual pre-rename name (``persist_reasoning``). * **Copilot finding 4** (sdk/typescript/src/events.ts:26): ``HistoryEvent`` JSDoc still referenced ``persist_reasoning`` — the sed rename only walked ``turnstone/`` and ``tests/``, missing the TypeScript SDK. Updated to ``surface_persisted_reasoning``. Also widened the comment to cover all three reasoning-bearing block types (Anthropic ``thinking``, OpenAI Responses ``reasoning``, synthetic ``reasoning_text``) instead of mentioning only Anthropic. * **github-code-quality finding** (session.py:1120): ``_resolve_server_type`` had a bare ``except Exception: pass``. Replaced with a ``log.debug(..., exc_info=True)`` + explanatory comment. Behaviour unchanged (still returns ``""`` on any lookup failure); failures are now observable under DEBUG triage. Rejected (with rationale) * **github-code-quality finding** (_protocol.py:265): ``extract_reasoning_text``'s body is ``...`` per ``LLMProvider`` Protocol convention. Every method in the file uses ``...`` (PEP 544 idiomatic Protocol style). Changing only this one to ``raise NotImplementedError`` would be inconsistent with the rest of the file. CodeQL's "statement has no effect" warning is technically correct for ``...`` as a standalone expression but ignores the documented Python Protocol convention. No fix. Docs sync * docs/api-reference.md: ``history`` SSE event message-shape table gains the optional ``reasoning`` field. * docs/architecture.md: ``ModelCapabilities`` row in the type table gains ``supports_reasoning_replay``; ``StreamChunk`` and ``CompletionResult`` rows gain the existing ``provider_blocks`` field (was missing pre-PR). New "Per-model reasoning persistence" subsection under the Models config section, documenting the two flags + capability gate + three reasoning paths + cross-provider shape filter. * docs/settings.md: new "Reasoning persistence (per-model)" subsection with the two-flag table and capability-gate note. * docs/diagrams/03-core-engine-classes.puml: ``LLMProvider`` interface adds ``extract_reasoning_text`` + the new ``replay_reasoning_to_model`` kwarg; ``ModelCapabilities`` class adds ``supports_reasoning_replay``. PNG regenerated. Lint + test gate * ruff check + ruff format clean. * mypy clean (191 source files). * pytest -m 'not live' — 6116 passed (3 deselected), +1 net new test (``test_dispatcher_scans_past_unrecognized_first_blocks``).