mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(reasoning): apply Copilot review feedback + docs sync
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``).
This commit is contained in:
@@ -281,6 +281,7 @@ Each message in the `messages` array has:
|
||||
| `role` | string | `"user"`, `"assistant"`, or `"tool"` |
|
||||
| `content` | string or null | Text content of the message |
|
||||
| `tool_calls` | array or null | Present only on assistant messages with calls |
|
||||
| `reasoning` | string (optional) | Concatenated reasoning / chain-of-thought text on assistant turns whose `provider_data` carried reasoning-bearing blocks (Anthropic `thinking`, OpenAI Responses `reasoning`, or synthetic `reasoning_text` from local-model servers). Present only when the active model's `surface_persisted_reasoning` flag is True. |
|
||||
|
||||
Each entry in `tool_calls`:
|
||||
|
||||
|
||||
+32
-3
@@ -634,9 +634,9 @@ LLMProvider (protocol)
|
||||
|
||||
| Type | Fields |
|
||||
|------|--------|
|
||||
| `StreamChunk` | `content_delta`, `reasoning_delta`, `tool_call_deltas`, `info_delta`, `usage`, `finish_reason` |
|
||||
| `CompletionResult` | `content`, `tool_calls`, `finish_reason`, `usage` |
|
||||
| `ModelCapabilities` | `context_window`, `max_output_tokens`, `supports_temperature`, `token_param`, `thinking_mode`, `supports_effort`, `supports_web_search`, `supports_tool_search`, `supports_vision` |
|
||||
| `StreamChunk` | `content_delta`, `reasoning_delta`, `tool_call_deltas`, `info_delta`, `usage`, `finish_reason`, `provider_blocks` |
|
||||
| `CompletionResult` | `content`, `tool_calls`, `finish_reason`, `usage`, `provider_blocks` |
|
||||
| `ModelCapabilities` | `context_window`, `max_output_tokens`, `supports_temperature`, `token_param`, `thinking_mode`, `supports_effort`, `supports_web_search`, `supports_tool_search`, `supports_vision`, `supports_reasoning_replay` |
|
||||
| `UsageInfo` | `prompt_tokens`, `completion_tokens`, `total_tokens`, `cache_creation_tokens`, `cache_read_tokens` |
|
||||
|
||||
**OpenAIProvider** (`_openai.py`): passes messages through unchanged (they are
|
||||
@@ -724,6 +724,35 @@ and `"openai-compatible"`.
|
||||
`max_tokens`, and `reasoning_effort` to override the global defaults from
|
||||
ConfigStore. When unset (`NULL`), the global default is used.
|
||||
|
||||
**Per-model reasoning persistence:** Two booleans on `model_definitions`
|
||||
(migration 052) control how reasoning text round-trips:
|
||||
|
||||
* `surface_persisted_reasoning` (default `True`) — gates whether stored
|
||||
reasoning text is surfaced on `/history` payloads for UI rehydration.
|
||||
**Storage of reasoning bytes happens regardless of this flag** — they
|
||||
ride in `provider_data` independently. Phase-1 admin UI label "Surface
|
||||
persisted reasoning."
|
||||
* `replay_reasoning_to_model` (default `False`) — gates whether stored
|
||||
reasoning blocks are sent back to the provider on subsequent turns.
|
||||
Capability-gated: `ModelCapabilities.supports_reasoning_replay` must
|
||||
also be `True` for the wire path to actually replay (canonical OpenAI
|
||||
gpt-5*/o-series and Anthropic Claude entries set it; unknown / local-
|
||||
server models default to `False`).
|
||||
|
||||
Three reasoning paths are recognised:
|
||||
|
||||
| Path | Provider | Capture | Persist | Replay |
|
||||
|------|----------|---------|---------|--------|
|
||||
| 1 | Anthropic Messages API | `thinking_delta` | `provider_blocks` (`type="thinking"`) | Verbatim via `_provider_content` |
|
||||
| 2 | OpenAI Responses (gpt-5*, o-series) | `response.reasoning_text.delta` events | `provider_blocks` (`type="reasoning"`) — only when `include=["reasoning.encrypted_content"]` | `ResponseReasoningItemParam` input items |
|
||||
| 3 | OpenAI Chat Completions (vLLM, llama.cpp, Gemini-compat) | `delta.reasoning_content` Pydantic extras | Synthetic `{type: "reasoning_text", text, source}` block stamped at end-of-stream | None — no API surface for replay on Chat Completions |
|
||||
|
||||
Cross-provider safety is enforced by `ANTHROPIC_VALID_BLOCK_TYPES` (a
|
||||
shape filter in `_anthropic.py:_convert_messages`): foreign blocks
|
||||
(OpenAI `reasoning`, synthetic `reasoning_text`) fall through to the
|
||||
text+tool_calls rebuild path rather than reaching Anthropic's input
|
||||
boundary as malformed content.
|
||||
|
||||
```toml
|
||||
[models.local]
|
||||
base_url = "http://localhost:8000/v1"
|
||||
|
||||
@@ -69,9 +69,10 @@ class "NullUI" as NullUI {
|
||||
interface "LLMProvider" as LLMProvider <<Protocol>> {
|
||||
+ provider_name: str {property}
|
||||
+ get_capabilities(model) → ModelCapabilities
|
||||
+ create_streaming(client, model, messages, ...) → Iterator[StreamChunk]
|
||||
+ create_completion(client, model, messages, ...) → CompletionResult
|
||||
+ create_streaming(client, model, messages, ..., replay_reasoning_to_model) → Iterator[StreamChunk]
|
||||
+ create_completion(client, model, messages, ..., replay_reasoning_to_model) → CompletionResult
|
||||
+ convert_tools(tools) → list[dict]
|
||||
+ extract_reasoning_text(provider_blocks) → str
|
||||
+ retryable_error_names: frozenset[str] {property}
|
||||
--
|
||||
core/providers/_protocol.py
|
||||
@@ -126,6 +127,7 @@ class "ModelCapabilities" as ModelCaps <<frozen>> {
|
||||
+ supports_web_search: bool
|
||||
+ supports_tool_search: bool
|
||||
+ supports_vision: bool
|
||||
+ supports_reasoning_replay: bool
|
||||
}
|
||||
|
||||
' ChatSession
|
||||
|
||||
@@ -1,3 +1,3 @@
|
||||
version https://git-lfs.github.com/spec/v1
|
||||
oid sha256:25b5448bbb7da8ddafe4f65c6c5e6cbcaa9cb9f31746ca46d3a2241bc47b1956
|
||||
size 259687
|
||||
oid sha256:9857db23fe3c4316d492073aac69c7e7558b1abe3b95ad7756d4a5933bd0ece7
|
||||
size 620214
|
||||
|
||||
@@ -59,6 +59,21 @@ from ConfigStore. Model names and context windows are now configured per-model
|
||||
in the Models tab. A startup warning is logged if these keys appear in
|
||||
`config.toml`.
|
||||
|
||||
### Reasoning persistence (per-model)
|
||||
|
||||
Two boolean flags on `model_definitions` (migration 052) control how
|
||||
reasoning text round-trips per model:
|
||||
|
||||
| Flag | Default | Effect |
|
||||
|------|---------|--------|
|
||||
| `surface_persisted_reasoning` | `True` | Surface stored reasoning text on `/history` payloads so a page reload re-renders the reasoning bubble. **Storage of reasoning bytes is independent of this flag** — they ride in `provider_data` regardless. |
|
||||
| `replay_reasoning_to_model` | `False` | Send stored reasoning blocks back to the provider on subsequent turns. Capability-gated: only takes effect when the model's `ModelCapabilities.supports_reasoning_replay` is also `True`. Set on canonical OpenAI gpt-5*/o-series and Anthropic Claude entries; unknown / local-server models default to `False` so an operator who flips the flag on a model whose API doesn't understand reasoning replay silently no-ops rather than 400-ing. |
|
||||
|
||||
Edit both via the admin Models tab. See the architecture doc for the
|
||||
provider-side mechanics (Anthropic `thinking`, OpenAI Responses
|
||||
`reasoning` + `include=["reasoning.encrypted_content"]`, synthetic
|
||||
`reasoning_text` for Chat Completions / vLLM / llama.cpp / Gemini-compat).
|
||||
|
||||
### Plan / task agent overrides
|
||||
|
||||
`plan_agent` and `task_agent` sub-sessions resolve independently from the
|
||||
|
||||
@@ -22,8 +22,10 @@ export interface HistoryEvent {
|
||||
* - `reminders`: metacognitive nudge bubbles (user/tool channels)
|
||||
* - `advisories`: extracted `UserInterjection` payloads on tool turns
|
||||
* - `reasoning`: concatenated reasoning text for assistant turns whose
|
||||
* `provider_data` carried thinking blocks (Anthropic today). Present
|
||||
* only when the active model's `persist_reasoning` flag is true.
|
||||
* `provider_data` carried reasoning-bearing blocks (Anthropic
|
||||
* `thinking`, OpenAI Responses `reasoning`, or synthetic
|
||||
* `reasoning_text` from path-3 servers). Present only when the
|
||||
* active model's `surface_persisted_reasoning` flag is true.
|
||||
*/
|
||||
messages: Array<Record<string, unknown>>;
|
||||
}
|
||||
|
||||
@@ -617,6 +617,36 @@ class TestExtractReasoningForHistory:
|
||||
assert messages[0]["reasoning"] == "synth thought"
|
||||
assert "_provider_content" not in messages[0]
|
||||
|
||||
def test_dispatcher_scans_past_unrecognized_first_blocks(self) -> None:
|
||||
# Regression for Copilot finding: dispatcher used to inspect
|
||||
# only provider_content[0]['type']. OpenAI Responses captures
|
||||
# EVERY output_item.done event into provider_blocks (not just
|
||||
# reasoning), so a hypothetical [message, reasoning, ...]
|
||||
# ordering would have silently dropped the reasoning. Now
|
||||
# walks the list for the first recognised reasoning-bearing
|
||||
# type and dispatches the whole list to that provider.
|
||||
from turnstone.core.history_decoration import extract_reasoning_for_history
|
||||
|
||||
messages = [
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "answer",
|
||||
"_provider_content": [
|
||||
# First block is a non-reasoning OpenAI Responses item.
|
||||
{"type": "message", "role": "assistant", "content": "answer"},
|
||||
# Reasoning sits later in the list.
|
||||
{
|
||||
"type": "reasoning",
|
||||
"id": "r_1",
|
||||
"summary": [{"type": "summary_text", "text": "deferred"}],
|
||||
},
|
||||
],
|
||||
}
|
||||
]
|
||||
extract_reasoning_for_history(messages, surface_persisted_reasoning_flag=True)
|
||||
assert messages[0]["reasoning"] == "deferred"
|
||||
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)
|
||||
|
||||
@@ -339,9 +339,28 @@ _BLOCK_TYPE_PROVIDER_FACTORY: dict[str, Callable[[], LLMProvider]] = {
|
||||
|
||||
|
||||
def extract_reasoning_text_from_provider_content(provider_content: Any) -> str:
|
||||
"""Dispatch reasoning extraction by first-block ``type`` field.
|
||||
"""Dispatch reasoning extraction by scanning for a recognised block type.
|
||||
|
||||
Returns ``""`` for empty / missing / non-list / unknown-type input.
|
||||
Walks ``provider_content`` looking for the first block whose
|
||||
``type`` is in :data:`_BLOCK_TYPE_PROVIDER_FACTORY`, then dispatches
|
||||
the WHOLE list to that provider's ``extract_reasoning_text``. Each
|
||||
provider's extractor already filters internally by its own block
|
||||
type (Anthropic walks ``thinking``, OpenAI Responses walks
|
||||
``reasoning``, OpenAI Chat walks ``reasoning_text``), so passing
|
||||
the full list is correct — interleaved foreign blocks are ignored.
|
||||
|
||||
Returns ``""`` for empty / missing / non-list input or when no
|
||||
recognised reasoning-bearing block type appears anywhere in the
|
||||
list.
|
||||
|
||||
Why scan instead of just inspecting ``provider_content[0]``: the
|
||||
OpenAI Responses streaming layer captures EVERY ``output_item.done``
|
||||
event into ``provider_blocks`` (``_openai_responses.py:415-420``),
|
||||
not just reasoning items. In practice the order is usually
|
||||
``[reasoning, message, ...]`` but the API doesn't guarantee that —
|
||||
a hypothetical ``[message, reasoning]`` ordering would silently
|
||||
drop the reasoning under an index-only check. Same robustness
|
||||
point for Anthropic's hypothetical mixed-order outputs.
|
||||
|
||||
Pure transform — safe from any thread. Both history surfaces
|
||||
(interactive ``_build_history`` and lifted ``make_history_handler``)
|
||||
@@ -350,16 +369,16 @@ def extract_reasoning_text_from_provider_content(provider_content: Any) -> str:
|
||||
"""
|
||||
if not isinstance(provider_content, list) or not provider_content:
|
||||
return ""
|
||||
first_block = provider_content[0]
|
||||
if not isinstance(first_block, dict):
|
||||
return ""
|
||||
block_type = first_block.get("type")
|
||||
if not isinstance(block_type, str):
|
||||
return ""
|
||||
factory = _BLOCK_TYPE_PROVIDER_FACTORY.get(block_type)
|
||||
if factory is None:
|
||||
return ""
|
||||
return factory().extract_reasoning_text(provider_content)
|
||||
for block in provider_content:
|
||||
if not isinstance(block, dict):
|
||||
continue
|
||||
block_type = block.get("type")
|
||||
if not isinstance(block_type, str):
|
||||
continue
|
||||
factory = _BLOCK_TYPE_PROVIDER_FACTORY.get(block_type)
|
||||
if factory is not None:
|
||||
return factory().extract_reasoning_text(provider_content)
|
||||
return ""
|
||||
|
||||
|
||||
def extract_reasoning_for_history(
|
||||
|
||||
@@ -1118,7 +1118,15 @@ class ChatSession:
|
||||
if isinstance(sc, dict):
|
||||
return str(sc.get("server_type") or "")
|
||||
except Exception:
|
||||
pass
|
||||
# Best-effort lookup — synth-block source tagging is
|
||||
# informational, never load-bearing. Log at debug so a
|
||||
# repeated registry-lookup failure during a session shows
|
||||
# up under DEBUG triage but doesn't spam normal logs.
|
||||
log.debug(
|
||||
"_resolve_server_type lookup failed for alias=%s; defaulting to empty",
|
||||
target_alias,
|
||||
exc_info=True,
|
||||
)
|
||||
return ""
|
||||
|
||||
def _maybe_synth_reasoning_block(
|
||||
|
||||
@@ -8,9 +8,10 @@ Adds two boolean (integer-coded) operator knobs:
|
||||
page refresh re-renders the reasoning bubble. **Storage of the
|
||||
reasoning bytes happens regardless of this flag** — it only controls
|
||||
the extract-and-include step in ``_build_history`` /
|
||||
``decorate_history_messages``. The earlier name ``surface_persisted_reasoning``
|
||||
was renamed because it implied a storage-control switch; this flag
|
||||
is purely about UI rehydration.
|
||||
``decorate_history_messages``. The pre-rename column was
|
||||
``persist_reasoning``; the rename to ``surface_persisted_reasoning``
|
||||
happened in the review-fix wave because the original name implied a
|
||||
storage-control switch when the flag is purely about UI rehydration.
|
||||
* ``replay_reasoning_to_model`` (default ``0``) — when true, the
|
||||
wire-build path keeps reasoning blocks in the outgoing
|
||||
``_provider_content`` lane on subsequent provider calls. False is the
|
||||
|
||||
Reference in New Issue
Block a user