From e8a17921bfb563535fbc9696e8a51d98b9fbe085 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Mon, 13 Jul 2026 07:46:41 -0700 Subject: [PATCH] =?UTF-8?q?fix(model-turn):=20round-3=20review=20=E2=80=94?= =?UTF-8?q?=20complete=20the=20scheme=20rollout=20to=20the=20main=20loop,?= =?UTF-8?q?=20coordinator=20role,=20admin=20save=20path,=20and=20CLI=20swi?= =?UTF-8?q?tch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- tests/test_history_decoration.py | 4 +-- tests/test_model_registry.py | 22 ++++++++---- tests/test_reasoning_audit_log_discipline.py | 7 ++-- tests/test_session_synth_reasoning_block.py | 9 ++--- turnstone/cli.py | 4 ++- turnstone/console/static/admin.js | 9 +++-- turnstone/core/model_turn.py | 14 ++++---- turnstone/core/session.py | 37 ++++++++++++++------ turnstone/core/settings_registry.py | 14 ++++---- 9 files changed, 78 insertions(+), 42 deletions(-) diff --git a/tests/test_history_decoration.py b/tests/test_history_decoration.py index 5c794166..8581b219 100644 --- a/tests/test_history_decoration.py +++ b/tests/test_history_decoration.py @@ -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 diff --git a/tests/test_model_registry.py b/tests/test_model_registry.py index 996d0f3b..04db8cd9 100644 --- a/tests/test_model_registry.py +++ b/tests/test_model_registry.py @@ -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( diff --git a/tests/test_reasoning_audit_log_discipline.py b/tests/test_reasoning_audit_log_discipline.py index 58acc331..99928be9 100644 --- a/tests/test_reasoning_audit_log_discipline.py +++ b/tests/test_reasoning_audit_log_discipline.py @@ -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}" ) diff --git a/tests/test_session_synth_reasoning_block.py b/tests/test_session_synth_reasoning_block.py index a5f47a90..7cc0a8ba 100644 --- a/tests/test_session_synth_reasoning_block.py +++ b/tests/test_session_synth_reasoning_block.py @@ -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). """ diff --git a/turnstone/cli.py b/turnstone/cli.py index 9af52cea..3ac10d39 100644 --- a/turnstone/cli.py +++ b/turnstone/cli.py @@ -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() diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index c45f7ea7..c17e7630 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -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 { diff --git a/turnstone/core/model_turn.py b/turnstone/core/model_turn.py index 5d8944fe..a9645306 100644 --- a/turnstone/core/model_turn.py +++ b/turnstone/core/model_turn.py @@ -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 diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 3d9bb7e0..0bb66d40 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -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}") diff --git a/turnstone/core/settings_registry.py b/turnstone/core/settings_registry.py index 3c083e3a..5019ad8d 100644 --- a/turnstone/core/settings_registry.py +++ b/turnstone/core/settings_registry.py @@ -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",