mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(title): reliable titles on thinking models; defer utility temperature
Auto-title generation and manual refresh stopped producing titles on reasoning models (the cluster serves qwen3.6). The title call capped max_tokens at 200, so the model's think pass consumed the whole budget and content came back empty (finish_reason=length) -> the title was skipped. Both paths share _generate_title, so both broke. Title path: - Raise the title completion to 2048 tokens so reasoning finishes and the title text actually lands. - Recover the title from content (never reasoning): reuse the canonical _strip_reasoning (handles <think>/<reasoning>, paired or unclosed) plus a backstop for the opener-absent </think> shape some templates emit, take the first non-empty line, and peel a "Title:" label and wrapping markdown/quote decoration. Internal punctuation is preserved. Cap at 80 to match the manual-alias bound. Temperature: - _utility_completion no longer hard-codes a temperature; it defaults to the session/registry value the main turn uses. Title (was 0.7/0.3), web-fetch extraction (was 0.2), and compaction all defer. Hard-coding a constant fought thinking/no-temp models and silently overrode an explicit [models.*] temperature; the provider still gates temperature per model. Tests: title sanitization across think/reasoning variants, truncation, and a trailing-prose case; utility-completion temperature deferral + explicit override.
This commit is contained in:
@@ -997,6 +997,120 @@ class TestTitleRetry:
|
||||
# Flag stays True after successful generation
|
||||
assert session._title_generated is True
|
||||
|
||||
def test_title_sanitizes_thinking_model_output(self, tmp_db):
|
||||
"""A reasoning model's answer can arrive wrapped in an unparsed
|
||||
``<think>`` span (lanes that don't split it into reasoning_content)
|
||||
plus markdown / quotes. There is no portable switch to disable thinking,
|
||||
so the title pass gives reasoning room (raised max_tokens), reuses
|
||||
``_strip_reasoning``, and peels wrapping decoration — keeping INTERNAL
|
||||
punctuation (the hyphen survives)."""
|
||||
from turnstone.core.providers._protocol import ModelCapabilities
|
||||
from turnstone.core.session import _TITLE_MAX_TOKENS
|
||||
|
||||
session = _make_session()
|
||||
session._title_generated = True
|
||||
session.messages = turns_from_dicts([{"role": "user", "content": "hi"}])
|
||||
result = MagicMock()
|
||||
result.content = (
|
||||
"<think>The user greets me; a fitting title would be...</think>\n\n"
|
||||
'**"Cluster Routing Deep-Dive"**'
|
||||
)
|
||||
session._provider = MagicMock()
|
||||
session._provider.get_capabilities.return_value = ModelCapabilities()
|
||||
session._provider.create_completion.return_value = result
|
||||
|
||||
captured: dict[str, str] = {}
|
||||
with patch(
|
||||
"turnstone.core.session.update_workstream_title",
|
||||
side_effect=lambda ws_id, title: captured.update(title=title),
|
||||
):
|
||||
session._generate_title()
|
||||
|
||||
assert captured["title"] == "Cluster Routing Deep-Dive"
|
||||
# Reasoning gets room to finish rather than a 200-token squeeze that
|
||||
# the think pass swallows whole (the empty-content regression); and the
|
||||
# title call forces no temperature — it defers to the session value.
|
||||
_, kw = session._provider.create_completion.call_args
|
||||
assert kw["max_tokens"] == _TITLE_MAX_TOKENS
|
||||
assert kw["temperature"] == session.temperature
|
||||
|
||||
def test_title_skipped_when_reasoning_consumes_whole_budget(self, tmp_db):
|
||||
"""If the budget is spent inside an unclosed ``<think>`` (the empty/
|
||||
cut-off content that broke titling), the cleaner yields no words — so
|
||||
nothing is persisted rather than a fragment of reasoning becoming the
|
||||
title."""
|
||||
from turnstone.core.providers._protocol import ModelCapabilities
|
||||
|
||||
session = _make_session()
|
||||
session._title_generated = True
|
||||
session.messages = turns_from_dicts([{"role": "user", "content": "hi"}])
|
||||
result = MagicMock()
|
||||
result.content = "<think>still reasoning, never closed before the cap"
|
||||
session._provider = MagicMock()
|
||||
session._provider.get_capabilities.return_value = ModelCapabilities()
|
||||
session._provider.create_completion.return_value = result
|
||||
|
||||
with patch("turnstone.core.session.update_workstream_title") as upd:
|
||||
session._generate_title()
|
||||
|
||||
upd.assert_not_called()
|
||||
|
||||
def test_title_strips_reasoning_variants(self, tmp_db):
|
||||
"""Reasoning reaches ``content`` in several shapes the title pass must
|
||||
survive: an opener-absent ``…</think>`` (templates that pre-inject the
|
||||
opening tag), a paired ``<reasoning>`` block, and a trailing
|
||||
explanation after the title (only the first non-empty line is kept)."""
|
||||
from turnstone.core.providers._protocol import ModelCapabilities
|
||||
|
||||
cases = [
|
||||
("I should weigh the options here</think>\n\nRendezvous Routing", "Rendezvous Routing"),
|
||||
(
|
||||
"<reasoning>pondering the ask</reasoning>\nCluster Health Digest",
|
||||
"Cluster Health Digest",
|
||||
),
|
||||
("Auth Layer Refactor\n\nThis title captures the request well.", "Auth Layer Refactor"),
|
||||
]
|
||||
for content, expected in cases:
|
||||
session = _make_session()
|
||||
session._title_generated = True
|
||||
session.messages = turns_from_dicts([{"role": "user", "content": "hi"}])
|
||||
result = MagicMock()
|
||||
result.content = content
|
||||
session._provider = MagicMock()
|
||||
session._provider.get_capabilities.return_value = ModelCapabilities()
|
||||
session._provider.create_completion.return_value = result
|
||||
|
||||
captured: dict[str, str] = {}
|
||||
with patch(
|
||||
"turnstone.core.session.update_workstream_title",
|
||||
side_effect=lambda ws_id, title, _c=captured: _c.update(title=title),
|
||||
):
|
||||
session._generate_title()
|
||||
assert captured.get("title") == expected, (content, captured)
|
||||
|
||||
def test_title_truncates_to_max_chars(self, tmp_db):
|
||||
"""The ``[:_TITLE_MAX_CHARS]`` slice is the only length guard now that
|
||||
the persist-time ``title[:80]`` is gone — a long title is bounded."""
|
||||
from turnstone.core.providers._protocol import ModelCapabilities
|
||||
from turnstone.core.session import _TITLE_MAX_CHARS
|
||||
|
||||
session = _make_session()
|
||||
session._title_generated = True
|
||||
session.messages = turns_from_dicts([{"role": "user", "content": "hi"}])
|
||||
result = MagicMock()
|
||||
result.content = "Story " * 40 # 240 chars on one line
|
||||
session._provider = MagicMock()
|
||||
session._provider.get_capabilities.return_value = ModelCapabilities()
|
||||
session._provider.create_completion.return_value = result
|
||||
|
||||
captured: dict[str, str] = {}
|
||||
with patch(
|
||||
"turnstone.core.session.update_workstream_title",
|
||||
side_effect=lambda ws_id, title: captured.update(title=title),
|
||||
):
|
||||
session._generate_title()
|
||||
assert len(captured["title"]) == _TITLE_MAX_CHARS
|
||||
|
||||
def test_title_skipped_after_resume_changes_ws_id(self, tmp_db):
|
||||
"""If ws_id changes (via resume) during title generation, discard the result."""
|
||||
from turnstone.core.providers._protocol import ModelCapabilities
|
||||
@@ -5433,6 +5547,29 @@ def test_utility_completion_records_aux_usage():
|
||||
assert rec["model"] == "test-model"
|
||||
|
||||
|
||||
def test_utility_completion_defers_temperature_to_session():
|
||||
"""Utility calls (title, compaction, web-fetch extraction) must NOT force a
|
||||
temperature: an unset temperature resolves to the session/registry value, so
|
||||
one operator-set ``[models.*]`` temperature governs every lane and code never
|
||||
fights a thinking/no-temp model by hard-coding a constant. An explicit
|
||||
override still wins for any caller that genuinely needs one."""
|
||||
from turnstone.core.providers._protocol import CompletionResult, ModelCapabilities
|
||||
|
||||
session = _make_session()
|
||||
session.temperature = 0.42
|
||||
session._provider = MagicMock()
|
||||
session._provider.get_capabilities.return_value = ModelCapabilities()
|
||||
session._provider.create_completion.return_value = CompletionResult(content="x")
|
||||
|
||||
session._utility_completion([{"role": "user", "content": "hi"}])
|
||||
_, kw = session._provider.create_completion.call_args
|
||||
assert kw["temperature"] == 0.42 # deferred to the session/registry value
|
||||
|
||||
session._utility_completion([{"role": "user", "content": "hi"}], temperature=0.9)
|
||||
_, kw2 = session._provider.create_completion.call_args
|
||||
assert kw2["temperature"] == 0.9 # explicit override still honored
|
||||
|
||||
|
||||
def test_record_aux_usage_skips_when_usage_missing():
|
||||
"""A provider that reports no usage object must not emit a phantom
|
||||
zero-token row."""
|
||||
|
||||
+55
-13
@@ -616,6 +616,27 @@ _SKILL_ARG_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
|
||||
# excludes the ``$ARGUMENTS[N]`` form which is a different placeholder.
|
||||
_SPEC_ARGUMENTS_LITERAL_RE = re.compile(r"\$ARGUMENTS\b(?!\[)")
|
||||
|
||||
# Auto-title generation runs on the session's own (often thinking-capable)
|
||||
# utility model, and there is no portable way to turn reasoning off:
|
||||
# ``enable_thinking`` is Qwen-only and commercial providers each differ. A
|
||||
# small ``max_tokens`` lets the reasoning pass swallow the entire budget so the
|
||||
# title text never lands (``finish_reason=length``, empty ``content`` → skip).
|
||||
# This hits auto-title and refresh alike (shared path) on any thinking model.
|
||||
# So give the think pass room (``_TITLE_MAX_TOKENS``), then recover the title
|
||||
# from ``content``: reuse :meth:`ChatSession._strip_reasoning` (the canonical
|
||||
# ``<think>``/``<reasoning>`` remover, for lanes that leave reasoning inline
|
||||
# rather than in ``reasoning_content``), take the first non-empty line (a model
|
||||
# that appends an explanation shouldn't fold prose into the title), then peel a
|
||||
# ``Title:`` label and wrapping markdown/quote decoration. Internal punctuation
|
||||
# is preserved so ``.NET``, ``CI/CD``, ``v1.6.0`` survive.
|
||||
_TITLE_MAX_TOKENS = 2048
|
||||
# Match the manual-rename (alias) cap so generated and hand-set titles share
|
||||
# one length bound.
|
||||
_TITLE_MAX_CHARS = 80
|
||||
_TITLE_LABEL_RE = re.compile(r"(?i)^\s*title\s*[:\-—]\s*")
|
||||
# Wrapping decoration peeled off both ends of a generated title.
|
||||
_TITLE_WRAP_CHARS = "*`\"' "
|
||||
|
||||
|
||||
# Soft cap on ``"watch_triggered"`` entries in the per-session NudgeQueue.
|
||||
# The pull-model path batches N watch fires into ONE envelope splice on
|
||||
@@ -2533,8 +2554,11 @@ class ChatSession:
|
||||
snippet += "\n\nTitle:"
|
||||
log.info("ws.title.llm_call_start", ws_id=ws_id[:8])
|
||||
|
||||
# Use slightly higher temperature for refreshes to encourage variety
|
||||
temp = 0.7 if current_title else 0.3
|
||||
# No temperature override here: defer to the session/registry
|
||||
# temperature (``_utility_completion`` default). A manual refresh
|
||||
# leans on the prompt's "generate a DIFFERENT title" instruction and
|
||||
# the changing ``current_title`` it feeds in for variety, rather than
|
||||
# forcing a hotter sample on top of the operator's chosen model.
|
||||
|
||||
result = self._utility_completion(
|
||||
[
|
||||
@@ -2551,17 +2575,28 @@ class ChatSession:
|
||||
},
|
||||
{"role": "user", "content": snippet},
|
||||
],
|
||||
max_tokens=200,
|
||||
temperature=temp,
|
||||
max_tokens=_TITLE_MAX_TOKENS,
|
||||
)
|
||||
raw = (result.content or "").strip()
|
||||
raw = result.content or ""
|
||||
log.info("ws.title.llm_response", ws_id=ws_id[:8], raw=raw[:200])
|
||||
# Take first line, strip quotes
|
||||
title = raw.split("\n")[0].strip().strip('"').strip("'")
|
||||
# Take the assistant's answer (``content``), never its reasoning.
|
||||
# Reuse the canonical reasoning stripper, then drop a leftover close
|
||||
# tag from lanes that pre-inject the opening ``<think>`` into the
|
||||
# prompt (only ``</think>`` reaches ``content``). See ``_TITLE_*``.
|
||||
stripped = self._strip_reasoning(raw)
|
||||
for _close in ("</think>", "</reasoning>"):
|
||||
_pos = stripped.rfind(_close)
|
||||
if _pos != -1:
|
||||
stripped = stripped[_pos + len(_close) :]
|
||||
# First non-empty line, with a ``Title:`` label and wrapping
|
||||
# markdown/quote decoration peeled (internal punctuation kept).
|
||||
line = next((ln for ln in stripped.splitlines() if ln.strip()), "")
|
||||
line = _TITLE_LABEL_RE.sub("", line.strip(_TITLE_WRAP_CHARS))
|
||||
title = line.strip(_TITLE_WRAP_CHARS)[:_TITLE_MAX_CHARS]
|
||||
if title and self._ws_id == ws_id:
|
||||
log.info("ws.title.updating", ws_id=ws_id[:8], title=title)
|
||||
update_workstream_title(ws_id, title[:80])
|
||||
self.ui.on_rename(title[:80])
|
||||
update_workstream_title(ws_id, title)
|
||||
self.ui.on_rename(title)
|
||||
log.info("ws.title.success", ws_id=ws_id[:8], title=title)
|
||||
else:
|
||||
log.info(
|
||||
@@ -3548,7 +3583,7 @@ class ChatSession:
|
||||
messages: list[dict[str, Any]],
|
||||
*,
|
||||
max_tokens: int = 4096,
|
||||
temperature: float = 0.3,
|
||||
temperature: float | None = None,
|
||||
reasoning_effort: str = "low",
|
||||
) -> CompletionResult:
|
||||
"""Run a lightweight internal completion (title gen, compaction, extraction).
|
||||
@@ -3557,6 +3592,13 @@ class ChatSession:
|
||||
commercial providers) and ``extra_params`` (for local model servers)
|
||||
so callers don't need to duplicate it. ``max_tokens`` is clamped to
|
||||
the model's advertised output limit so small models don't error.
|
||||
|
||||
``temperature`` defaults to the session temperature (``self.temperature``)
|
||||
— the same operator/registry-resolved value the main turn uses — rather
|
||||
than a hard-coded constant: utility calls should not silently override an
|
||||
explicit ``[models.*]`` temperature. The provider still drops it for
|
||||
models that forbid temperature (GPT-5 base, O-series) or pins it (Claude
|
||||
with thinking), so this only governs models that genuinely accept one.
|
||||
"""
|
||||
caps = self._get_capabilities()
|
||||
clamped = min(max_tokens, caps.max_output_tokens) if caps.max_output_tokens else max_tokens
|
||||
@@ -3566,7 +3608,7 @@ class ChatSession:
|
||||
model=self.model,
|
||||
messages=messages,
|
||||
max_tokens=clamped,
|
||||
temperature=temperature,
|
||||
temperature=self.temperature if temperature is None else temperature,
|
||||
reasoning_effort=reasoning_effort,
|
||||
extra_params=self._provider_extra_params(),
|
||||
capabilities=caps,
|
||||
@@ -13033,7 +13075,8 @@ class ChatSession:
|
||||
# Phase 3: summarization API call.
|
||||
# Use a generous max_tokens so thinking models don't starve the
|
||||
# visible answer, and pass reasoning_effort="low" to avoid wasting
|
||||
# budget on deep reasoning for a simple extraction task.
|
||||
# budget on deep reasoning for a simple extraction task. Temperature
|
||||
# is left to the session/registry default rather than overridden here.
|
||||
try:
|
||||
result = self._utility_completion(
|
||||
[
|
||||
@@ -13057,7 +13100,6 @@ class ChatSession:
|
||||
},
|
||||
],
|
||||
max_tokens=8192,
|
||||
temperature=0.2,
|
||||
)
|
||||
answer = result.content or ""
|
||||
if not answer:
|
||||
|
||||
Reference in New Issue
Block a user