mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(model-turn): round-3 review — complete the scheme rollout to the main loop, coordinator role, admin save path, and CLI switch
- The main streaming loop now applies the in-code model-definition rung
(caps.default_reasoning_effort) exactly like model_turn does, so the
same alias samples identically between chat and every auxiliary lane
(resolve_lane's stated contract). This also unblocks operator
temperature on gpt-5.x aliases whose declared default is "none" — the
main loop previously sent neither knob while aux lanes sent both.
- coordinator.reasoning_effort default "medium" -> "" (the missed unset
sentinel): coordinators inherit like every other lane; the role rung
fires only when the operator stored a value.
- admin webux: _onSettingChange no longer hides the save button for a
blanked nullable number input, so the blank-means-inherit save path is
actually reachable from the field it decorates.
- /model switch on STORE-LESS sessions (the CLI) keeps the user's
explicit --temperature//reason knobs when the target alias declares no
override — the current knobs are the only authority there (mirrors
the max_tokens fallback). Store-backed sessions still re-resolve.
- ModelLane docstring no longer documents the removed caller-default
effort rung; CLI status line shows any resolved effort ("medium" is no
longer a hidden code default); dead `u = usage` alias dropped; three
test docstrings re-pointed from the deleted
ChatSession._maybe_synth_reasoning_block to
model_turn.synth_reasoning_block.
This commit is contained in:
@@ -509,8 +509,8 @@ class TestExtractReasoningForHistory:
|
||||
|
||||
def test_first_block_reasoning_text_dispatches_to_openai_chat(self) -> None:
|
||||
# Phase 3 path 3: synthetic ``reasoning_text`` blocks (stamped
|
||||
# by ChatSession._maybe_synth_reasoning_block for vLLM /
|
||||
# llama.cpp / Gemini-compat conversations) dispatch to
|
||||
# by model_turn.synth_reasoning_block for vLLM / llama.cpp /
|
||||
# Gemini-compat conversations) dispatch to
|
||||
# OpenAIChatCompletionsProvider.extract_reasoning_text.
|
||||
from turnstone.core.history_decoration import extract_reasoning_for_history
|
||||
|
||||
|
||||
@@ -1188,11 +1188,13 @@ class TestSessionModelCommand:
|
||||
assert session.max_tokens == 2048
|
||||
assert session.reasoning_effort == "high"
|
||||
|
||||
def test_model_switch_none_params_reverts_to_unset(self) -> None:
|
||||
"""Switching to a model with no overrides re-resolves the knobs for
|
||||
the NEW alias: nothing configured → unset (wire omission). The old
|
||||
model's override must not leak onto the new lane — pre-scheme, a
|
||||
store-less session kept the stale 1.5 forever."""
|
||||
def test_model_switch_storeless_keeps_explicit_knobs(self) -> None:
|
||||
"""On a STORE-LESS session (the CLI), the current knobs are the
|
||||
user's explicit flags — the only authority that exists — so a
|
||||
switch to an override-free alias keeps them (mirroring the
|
||||
max_tokens fallback). With a ConfigStore the shared resolvers
|
||||
re-resolve for the new alias instead (unset → None → wire
|
||||
omission), so per-model overrides don't leak between aliases."""
|
||||
reg = ModelRegistry(
|
||||
models={
|
||||
"hot": ModelConfig("hot", "x", "x", "hot-model", temperature=1.5),
|
||||
@@ -1201,9 +1203,15 @@ class TestSessionModelCommand:
|
||||
default="hot",
|
||||
)
|
||||
session = _make_session(registry=reg, model_alias="hot")
|
||||
session.temperature = 1.5 # as set by per-model override
|
||||
session.temperature = 0.9 # user's explicit --temperature flag
|
||||
session.reasoning_effort = "high" # user's explicit /reason choice
|
||||
session.handle_command("/model plain")
|
||||
assert session.temperature is None
|
||||
assert session.temperature == 0.9
|
||||
assert session.reasoning_effort == "high"
|
||||
# A per-model override on the TARGET alias still wins over the
|
||||
# carried knob.
|
||||
session.handle_command("/model hot")
|
||||
assert session.temperature == 1.5
|
||||
|
||||
def test_model_switch_unknown_alias(self) -> None:
|
||||
reg = ModelRegistry(
|
||||
|
||||
@@ -229,8 +229,9 @@ class TestReasoningAuditLogDiscipline:
|
||||
def test_synth_reasoning_block_via_stream_response_does_not_log_reasoning(
|
||||
self,
|
||||
) -> None:
|
||||
"""Drives ChatSession._stream_response (which calls
|
||||
_maybe_synth_reasoning_block at end-of-stream) with a fake
|
||||
"""Drives ChatSession._stream_response (which invokes
|
||||
model_turn.synth_reasoning_block at end-of-stream via
|
||||
_finalize_provider_blocks) with a fake
|
||||
``reasoning_delta=_MARKER`` chunk; asserts no log call carried
|
||||
the marker text."""
|
||||
session = make_session()
|
||||
@@ -258,7 +259,7 @@ class TestReasoningAuditLogDiscipline:
|
||||
if _payload_contains_marker(args, kwargs)
|
||||
]
|
||||
assert offending == [], (
|
||||
f"_stream_response + _maybe_synth_reasoning_block leaked reasoning "
|
||||
f"_stream_response + synth_reasoning_block leaked reasoning "
|
||||
f"text into INFO+ logs: {offending}"
|
||||
)
|
||||
|
||||
|
||||
@@ -214,10 +214,11 @@ class TestStreamResponseSynthBlockIntegration:
|
||||
"""Integration test: drives a fake reasoning-emitting stream
|
||||
through ``ChatSession._stream_response`` and asserts the
|
||||
synthesizer wires up correctly. Pins the call site at
|
||||
``session.py`` (where ``_maybe_synth_reasoning_block`` is invoked
|
||||
on the assembled provider_blocks before stamping ``_provider_content``)
|
||||
— without this, a future refactor that drops the synthesizer call
|
||||
would silently break path-3 capture (vLLM/llama.cpp/Gemini-compat
|
||||
``session.py`` (where ``model_turn.synth_reasoning_block`` is
|
||||
invoked — via ``_finalize_provider_blocks`` — on the assembled
|
||||
provider_blocks before stamping ``_provider_content``) — without
|
||||
this, a future refactor that drops the synthesizer call would
|
||||
silently break path-3 capture (vLLM/llama.cpp/Gemini-compat
|
||||
reasoning would be visible live but invisible on history reload).
|
||||
"""
|
||||
|
||||
|
||||
+3
-1
@@ -301,7 +301,9 @@ class TerminalUI(SessionUI):
|
||||
total_tok = usage["prompt_tokens"] + usage["completion_tokens"]
|
||||
pct = total_tok / context_window * 100 if context_window > 0 else 0
|
||||
parts = [f"{total_tok:,} / {context_window:,} tokens ({pct:.0f}%)"]
|
||||
if effort and effort != "medium":
|
||||
# Any resolved effort shows — "medium" is no longer a code default
|
||||
# to hide, it only appears when someone explicitly chose it.
|
||||
if effort:
|
||||
parts.append(f"reasoning: {effort}")
|
||||
sys.stdout.write(f"\n {DIM}[{' · '.join(parts)}]{RESET}\n")
|
||||
sys.stdout.flush()
|
||||
|
||||
@@ -4435,8 +4435,13 @@ function _onSettingChange(inp) {
|
||||
dirty = String(current) !== String(orig);
|
||||
}
|
||||
|
||||
// Disable save for empty number fields (server will reject)
|
||||
const emptyNumber = inp.type === "number" && current === "";
|
||||
// Disable save for empty number fields (server will reject) — EXCEPT
|
||||
// nullable-default settings, where blank is a saveable state meaning
|
||||
// "inherit" (the save handler maps it to reset-to-default).
|
||||
const emptyNumber =
|
||||
inp.type === "number" &&
|
||||
current === "" &&
|
||||
inp.getAttribute("data-nullable") !== "1";
|
||||
if (dirty && !emptyNumber) {
|
||||
saveBtn.classList.add("visible");
|
||||
} else {
|
||||
|
||||
@@ -340,12 +340,14 @@ class ModelLane:
|
||||
:func:`resolve_temperature_setting` / :func:`resolve_effort_setting`).
|
||||
``None`` means no operator spoke. For temperature that is terminal:
|
||||
the field is omitted from the wire and the inference engine's own
|
||||
default applies. For effort, :func:`model_turn` applies two more
|
||||
rungs below the lane — a caller-supplied request-shaped default,
|
||||
then the in-code model definition (``caps.default_reasoning_effort``)
|
||||
— before omitting. House rule: code never pins either knob; callers
|
||||
pass an explicit value only when relaying an operator/user-resolved
|
||||
knob (the session's own value on the session-model lanes).
|
||||
default applies. For effort, :func:`model_turn` applies exactly one
|
||||
more rung below the lane — the in-code model definition
|
||||
(``caps.default_reasoning_effort``) — before omitting; there is
|
||||
deliberately NO caller-supplied default rung (a code-chosen effort
|
||||
token is unvetted on local vocabularies). House rule: code never
|
||||
pins either knob; callers pass an explicit value only when relaying
|
||||
an operator/user-resolved knob (the session's own value on the
|
||||
session-model lanes).
|
||||
"""
|
||||
|
||||
provider: LLMProvider
|
||||
|
||||
+27
-10
@@ -4837,18 +4837,17 @@ class ChatSession:
|
||||
minimal UI stubs (some tests, replay shims) predate it and should
|
||||
skip recording rather than crash a title-gen or sub-agent turn.
|
||||
"""
|
||||
u = usage
|
||||
if u is None:
|
||||
if usage is None:
|
||||
return
|
||||
record = getattr(self.ui, "on_aux_usage", None)
|
||||
if record is None:
|
||||
return
|
||||
record(
|
||||
{
|
||||
"prompt_tokens": u.prompt_tokens,
|
||||
"completion_tokens": u.completion_tokens,
|
||||
"cache_creation_tokens": u.cache_creation_tokens,
|
||||
"cache_read_tokens": u.cache_read_tokens,
|
||||
"prompt_tokens": usage.prompt_tokens,
|
||||
"completion_tokens": usage.completion_tokens,
|
||||
"cache_creation_tokens": usage.cache_creation_tokens,
|
||||
"cache_read_tokens": usage.cache_read_tokens,
|
||||
"model": model or self.model,
|
||||
}
|
||||
)
|
||||
@@ -5158,7 +5157,15 @@ class ChatSession:
|
||||
tools=self._get_active_tools(),
|
||||
max_tokens=self.max_tokens,
|
||||
temperature=self.temperature,
|
||||
reasoning_effort=self.reasoning_effort,
|
||||
# The in-code model-definition rung applies here exactly as
|
||||
# it does in model_turn's effective computation — the main
|
||||
# loop must sample identically to every auxiliary lane on
|
||||
# the same alias (resolve_lane's stated contract). Session
|
||||
# effort (operator/user rungs) wins; unset falls to the
|
||||
# caps declaration; None omits the param.
|
||||
reasoning_effort=(
|
||||
self.reasoning_effort or resolved_caps.default_reasoning_effort or None
|
||||
),
|
||||
extra_params=self._provider_extra_params(
|
||||
provider=prov, model_alias=model_alias
|
||||
),
|
||||
@@ -17054,15 +17061,25 @@ class ChatSession:
|
||||
# away from a model with overrides doesn't leak them and
|
||||
# every surface samples identically on the same alias.
|
||||
# Unset resolves to None (wire omission), replacing any prior
|
||||
# model's value.
|
||||
# model's value. STORE-LESS sessions (the CLI) are the
|
||||
# exception: there the current knobs ARE the user's explicit
|
||||
# flags (--temperature / /reason) — the only authority that
|
||||
# exists — so the switch keeps them unless the new alias
|
||||
# declares its own (mirrors the max_tokens fallback below).
|
||||
cs = self._config_store
|
||||
self.temperature = resolve_temperature_setting(cfg, cs)
|
||||
if cs:
|
||||
self.temperature = resolve_temperature_setting(cfg, cs)
|
||||
self.reasoning_effort = resolve_effort_setting(cfg, cs)
|
||||
else:
|
||||
if cfg.temperature is not None:
|
||||
self.temperature = cfg.temperature
|
||||
if cfg.reasoning_effort:
|
||||
self.reasoning_effort = cfg.reasoning_effort
|
||||
self.max_tokens = (
|
||||
cfg.max_tokens
|
||||
if cfg.max_tokens is not None
|
||||
else (cs.get("model.max_tokens") if cs else self.max_tokens)
|
||||
)
|
||||
self.reasoning_effort = resolve_effort_setting(cfg, cs)
|
||||
self._init_system_messages()
|
||||
self._save_config()
|
||||
self.ui.on_info(f"Switched to {cyan(arg)}: {model_name}")
|
||||
|
||||
@@ -831,16 +831,16 @@ def _build_registry() -> dict[str, SettingDef]:
|
||||
SettingDef(
|
||||
"coordinator.reasoning_effort",
|
||||
"str",
|
||||
"medium",
|
||||
"",
|
||||
"Reasoning effort for coordinator sessions (empty = inherit from model.reasoning_effort)",
|
||||
"coordinator",
|
||||
choices=["", "none", "minimal", "low", "medium", "high", "xhigh", "max"],
|
||||
help="Reasoning effort for coordinator sessions. Coordinators benefit from "
|
||||
"medium-or-higher effort when juggling multiple child workstreams. Use "
|
||||
"'low' only when your coordinator handles simple, one-off dispatch "
|
||||
"workflows. (Empty here means “inherit” — the per-model "
|
||||
"override on the alias wins, otherwise model.reasoning_effort. Use "
|
||||
"‘none’ to actually disable reasoning.)",
|
||||
help="Reasoning effort for coordinator sessions. When empty (the default), "
|
||||
"coordinators inherit like every other lane: the per-model override on "
|
||||
"the alias wins, then model.reasoning_effort, then the model's own "
|
||||
"declared or serving-side default. Coordinators juggling many child "
|
||||
"workstreams often benefit from an explicit medium-or-higher value here. "
|
||||
"Use ‘none’ to actually disable reasoning.",
|
||||
),
|
||||
SettingDef(
|
||||
"coordinator.max_active",
|
||||
|
||||
Reference in New Issue
Block a user