From 0cdb6798924c3d408d3269ef302fd102b2fd3390 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Thu, 6 Aug 2026 15:14:52 -0700 Subject: [PATCH] Follow-ups on the #832 fold: supersession predicate, wire-prep error hygiene, reasoning-parser tile (#986) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(session): ask the shared supersession predicate at the older sites ``_check_cancelled`` and ``_compaction_event`` predate ``_generation_superseded`` and each carried its own inline copy of the formula, so the drift the helper exists to prevent had two live places to start from. Both are behaviour-identical today. What the pin protects is the generation-0 convention: a bare ``!=`` reads a direct seam caller as an orphan, which would raise a cancel on a live turn and stamp a live compaction superseded — suppressing the end notice, so an operator watching a real compaction fail would be told nothing at all. * fix(session): render a wire-prep fault's cause class, never its message Every other branch of the fatal formatter tails the backend's own diagnostic text, which is what the operator needs. This branch is different in kind: ``prepare_wire`` is our lowering over the session's stored history, so its exception message can quote that history — and the formatted string is both shown to the operator and persisted to ``last_error``, which a coordinating agent reads. ``redact_credentials`` is a best-effort regex by its own docstring, so it is no floor for arbitrary conversation text. The cause's class still identifies the fault, the guidance is unchanged, and the debug traceback logged in the same function localizes the raise site. * feat(console): surface the server-side reasoning parser capability The inline think-tag scan is a fallback for inference servers with no reasoning parser, and for misconfigured ones. An operator running vLLM or llama.cpp with a parser configured had no way to say so from the model shelf — ``server_parses_reasoning`` was reachable only by hand editing the raw capabilities JSON, and it defaults to off, so the scan stays on and both channels run at once. The tile test is a general invariant rather than a single-key pin: every tile key must render a checkbox, carry a default, and — where the key is a ``ModelCapabilities`` field — agree with the dataclass. The matrix is a hand-maintained mirror, so it drifts silently otherwise. * fix(model_turn): a wire-prep wrapper carries the cause's class, not its text Withholding the message in the fatal formatter was not enough. The wrapper was built as ``WirePreparationError(str(prep_err))``, so ``str(exc)`` IS the cause's message — and the interactive retry arm renders exactly that into the dashboard SSE, one line after the formatter emitted the redacted version. ``sanitize_error_text`` is no floor there: it returns arbitrary stored-history text unchanged. Fixing the exception rather than the one consumer closes every caller that stringifies it, now and later. The message still rides ``__cause__`` for tracebacks and debug logs. * fix(console): coerce lifted capability values the way the backend does The tile lift used bare ``!!``, but the capabilities dict is hand-edited JSON: a stored string "false" is truthy to JS while ``apply_capability_overrides`` reads it as False. Opening such a row rendered the tile CHECKED and saving persisted boolean true — inverting the capability without the operator touching it. For ``server_parses_reasoning`` that silently disables the inline tag scan, the exact typo model_turn's comment already warns about, and this key had just been lifted into the matrix. ``_capBool`` mirrors the backend's spelling table; a value the backend would not coerce stays in the raw JSON rather than being rewritten, which is the policy the modal already applies to thinking_mode. Cases are generated from the Python table and executed under node, so a spelling added on one side fails here. Also tightens two pins the tile test left open: the checkbox must render inside the container the JS actually queries, and a tile key that is not a capability field is exempted by NAME rather than by a blanket hasattr, which was swallowing the consistent-rename case. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- tests/test_app_js.py | 125 ++++++++++++++++++++++++++++ tests/test_cancel.py | 43 ++++++++++ tests/test_midstream_retry.py | 48 ++++++++++- turnstone/console/static/admin.js | 37 +++++++- turnstone/console/static/index.html | 7 ++ turnstone/core/model_turn.py | 10 ++- turnstone/core/session.py | 22 +++-- 7 files changed, 282 insertions(+), 10 deletions(-) diff --git a/tests/test_app_js.py b/tests/test_app_js.py index cbaec8f0..0bfa9a55 100644 --- a/tests/test_app_js.py +++ b/tests/test_app_js.py @@ -708,6 +708,131 @@ def test_audio_roles_gated_to_openai_sdk_providers() -> None: assert '_providerCarriesAudio((md && md.provider) || "openai")' in body +# Tile keys that are deliberately NOT ``ModelCapabilities`` fields. +# ``supports_rerank`` is a registry-level flag read off the model row. +_NON_DATACLASS_TILES = {"supports_rerank"} + + +def test_capability_bool_lift_agrees_with_the_backend_coercion() -> None: + """The tile lift coerces a stored capability the way the backend does. + + The capabilities dict is hand-edited JSON, so a stored string + ``"false"`` is TRUTHY to JS while ``apply_capability_overrides`` reads + it as ``False``. Lifting it into a tile with bare ``!!`` renders the + row checked and then persists boolean ``true`` on the next save — + inverting the capability without the operator touching it. For + ``server_parses_reasoning`` that silently disables the inline tag scan, + which is the leak model_turn's own comment names. + + Cases are generated FROM the Python table, so a spelling added on one + side and not the other fails here rather than in the field. + """ + import tempfile + + from turnstone.core.model_turn import _CAPABILITY_BOOL_STRINGS + + admin = _CONSOLE_ADMIN_JS.read_text(encoding="utf-8") + table = re.search(r"const _CAP_BOOL_STRINGS = \{.*?\n\};", admin, re.S) + fn = re.search(r"function _capBool\(value\) \{.*?\n\}", admin, re.S) + assert table and fn, "capability bool coercion not found in admin.js" + + checks: list[str] = [] + for spelling, expected in _CAPABILITY_BOOL_STRINGS.items(): + # Same spelling, uppercased, and padded — the Python arm strips and + # lowercases before lookup, so the JS must too. + for variant in (spelling, spelling.upper(), f" {spelling} "): + lit = json.dumps(variant) + checks.append( + f"if (_capBool({lit}) !== {json.dumps(expected)}) " + f"throw new Error('spelling ' + {lit} + ' -> ' + _capBool({lit}));" + ) + checks += [ + "if (_capBool(true) !== true) throw new Error('boolean true');", + "if (_capBool(false) !== false) throw new Error('boolean false');", + "if (_capBool(1) !== true) throw new Error('number 1');", + "if (_capBool(0) !== false) throw new Error('number 0');", + # Unrecognized values must NOT coerce — the caller leaves them in the + # raw JSON instead of rewriting them (the thinking_mode policy). + "if (_capBool('maybe') !== undefined) throw new Error('garbage string');", + "if (_capBool(null) !== undefined) throw new Error('null');", + "if (_capBool({}) !== undefined) throw new Error('object');", + ] + harness = table.group(0) + chr(10) + fn.group(0) + chr(10) + chr(10).join(checks) + + with tempfile.NamedTemporaryFile(mode="w", suffix=".mjs", delete=False) as f: + f.write(harness) + tmp = f.name + try: + proc = subprocess.run(["node", tmp], capture_output=True, text=True, timeout=15) + except FileNotFoundError: + pytest.skip("node binary not available on PATH") + finally: + os.unlink(tmp) + assert proc.returncode == 0, ( + f"_capBool disagrees with the backend coercion. stderr={proc.stderr!r}" + ) + + # The lift must actually USE it — a reverted call site would leave the + # helper in place and every case above still passing. + assert "_modelCapsExplicit[k] = !!capsObj[k]" not in admin + assert "const asBool = _capBool(capsObj[k]);" in admin + + +def test_capability_tiles_agree_with_the_capabilities_dataclass() -> None: + """Every tile is a real capability, rendered, and defaulted like Python. + + The tile matrix is a hand-maintained mirror of + :class:`ModelCapabilities`, so it drifts silently: a renamed field + leaves a tile that writes a key nothing reads, and a JS default that + disagrees with the dataclass shows the operator a state the backend + would not apply. A tile whose key is not a capability field is + allowed only by NAME (``_NON_DATACLASS_TILES``) — a blanket + "skip what the dataclass lacks" would exempt exactly the rename this + test exists to catch. + """ + from turnstone.core.providers._protocol import ModelCapabilities + + admin = _CONSOLE_ADMIN_JS.read_text(encoding="utf-8") + html = _CONSOLE_INDEX.read_text(encoding="utf-8") + + keys_block = re.search(r"const _MODEL_CAP_KEYS = \[(.*?)\];", admin, re.S) + defaults_block = re.search(r"const _MODEL_CAP_DEFAULTS = \{(.*?)\};", admin, re.S) + assert keys_block and defaults_block, "capability tile matrix not found in admin.js" + keys = re.findall(r'"(\w+)"', keys_block.group(1)) + defaults = { + k: v == "true" for k, v in re.findall(r"(\w+):\s*(true|false)", defaults_block.group(1)) + } + assert keys, "no capability tile keys parsed" + + # The tile is only reachable if it renders INSIDE the container + # ``_modelTileEl`` queries; outside it, ``_modelGetTile`` silently falls + # back to the default and a saved ``true`` is rewritten as ``false``. + grid_start = html.index('id="model-capgrid"') + grid = html[grid_start : html.index("", grid_start)] + + caps = ModelCapabilities() + for key in keys: + assert f'data-cap="{key}"' in grid, f"tile {key} renders outside #model-capgrid" + assert key in defaults, f"tile {key} has no entry in _MODEL_CAP_DEFAULTS" + # No hasattr escape hatch: a tile whose key is neither a capability + # field nor a known registry flag writes a key nothing reads, which + # is the first drift this test exists to catch. + assert hasattr(caps, key) or key in _NON_DATACLASS_TILES, ( + f"tile {key} is neither a ModelCapabilities field nor a known registry flag" + ) + if hasattr(caps, key): + assert defaults[key] == getattr(caps, key), ( + f"tile default for {key} disagrees with ModelCapabilities" + ) + assert set(defaults) == set(keys), "_MODEL_CAP_DEFAULTS and _MODEL_CAP_KEYS disagree" + + # The inline-tag scan is a FALLBACK for servers with no reasoning parser + # (and for misconfigured ones). An operator running vLLM/llama.cpp with a + # parser configured needs a discoverable way to say so — without it the + # only route is hand-editing the raw capabilities JSON. + assert "server_parses_reasoning" in keys + + def test_model_response_controls_are_capability_driven_and_sparse() -> None: """The model shelf surfaces Responses-only scalar controls without hard-coding GPT-5.6 IDs or pinning inherited capability-table values.""" diff --git a/tests/test_cancel.py b/tests/test_cancel.py index 7a3041df..1d589c3b 100644 --- a/tests/test_cancel.py +++ b/tests/test_cancel.py @@ -1193,6 +1193,49 @@ class TestOrphanArmDutiesGate: tracker.record_success.assert_called_once() +class TestOlderSitesAskTheSharedPredicate: + """The two supersession sites that PREDATE the shared predicate ask it + too, so generation 0 stays unscoped at both. + + Each carried its own inline copy of the formula, so the drift the + helper exists to prevent had two live places to start from. A bare + ``!=`` in either reads a direct seam caller (generation 0) as an + orphan: ``_check_cancelled`` would raise a cancel on a live turn, and + ``_compaction_event`` would stamp a live compaction ``superseded`` — + which suppresses its end notice, so the operator watching a real + compaction fail would be told nothing at all. + """ + + def test_check_cancelled_leaves_an_unscoped_generation_alone(self, tmp_db): + session = _make_session() + session._generation = 7 + session._check_cancelled(0) # unscoped — a direct seam caller + session._check_cancelled(7) # the live generation + with pytest.raises(GenerationCancelled): + session._check_cancelled(2) # a real orphan + + def test_compaction_event_calls_an_unscoped_generation_live(self, tmp_db): + class _Recorder(NullUI): + def __init__(self): + super().__init__() + self.events = [] + + def on_compaction(self, event): + self.events.append(event) + + ui = _Recorder() + session = _make_session(ui=ui) + session._generation = 7 + failed_end = {"phase": "end", "ok": False, "reason": "cancelled", "trigger": "manual"} + session._compaction_event(0, dict(failed_end)) + session._compaction_event(2, dict(failed_end)) + + assert ui.events[0]["superseded"] is False + assert ui.events[0]["notice"] is True # the operator is told + assert ui.events[1]["superseded"] is True + assert ui.events[1]["notice"] is False + + class TestSupersessionVerdictAgreement: """Every arm of one streaming turn must reach the SAME supersession verdict for the same generation shape. Generation 0 is unscoped, so diff --git a/tests/test_midstream_retry.py b/tests/test_midstream_retry.py index 22d86dfa..1089fa27 100644 --- a/tests/test_midstream_retry.py +++ b/tests/test_midstream_retry.py @@ -828,16 +828,29 @@ class TestRecreateWindowClassification: patch.object( session, "_prepare_wire_messages", side_effect=ValueError("malformed turn 7") ), - pytest.raises(WirePreparationError), + pytest.raises(WirePreparationError) as excinfo, ): session.send("test") + # The wrapper carries the cause's CLASS, not its text. Callers that + # render ``str(exc)`` directly — the interactive retry arm's dashboard + # line — would otherwise re-emit the stored-history text the fatal + # formatter just withheld, on a surface that reaches the operator and + # the persisted error row. + assert str(excinfo.value) == "ValueError" + assert "malformed turn 7" not in str(excinfo.value) + tracker.record_failure.assert_not_called() fb_spy.assert_called_once() provider.create_streaming.assert_not_called() errors = ui.of("error") assert errors and "stored history" in errors[-1] - assert "malformed turn 7" in errors[-1] + # Reached the dedicated branch — identified by the cause's CLASS. + # Its message is withheld all the way through the live send path: + # it is our lowering's text over stored history, and this string + # is persisted (see TestWirePrepFaultRedaction). + assert "ValueError" in errors[-1] + assert "malformed turn 7" not in errors[-1] def test_fallback_prep_fault_continues_walk(self, tmp_db): """A prep fault on one lane must not abort the walk: the next @@ -951,6 +964,37 @@ class TestFallbackFailureRedaction: assert not any("SECRETKEY" in i for i in infos) +class TestWirePrepFaultRedaction: + def test_operator_text_carries_the_cause_class_only(self, tmp_db): + """A wire-prep fault renders its cause's CLASS, never its message. + + Every other branch of the formatter tails the backend's own + diagnostic text, which is what the operator needs. This one is + different in kind: ``prepare_wire`` is our lowering over the + session's STORED HISTORY, so its exception message can quote that + history — and this string is both shown to the operator and + persisted to ``last_error``, which a coordinating agent reads. + ``redact_credentials`` is a best-effort regex by its own + docstring, so it is no floor for arbitrary conversation text. + """ + from turnstone.core.model_turn import WirePreparationError + + session = _make_session(RecordingUI()) + cause = ValueError("malformed block in turn 4: {'text': 'the user's private notes'}") + exc = WirePreparationError(str(cause)) + exc.__cause__ = cause + + rendered = session._format_backend_error(exc) + + assert rendered is not None + assert "ValueError" in rendered + assert "private notes" not in rendered + assert "malformed block" not in rendered + # Still actionable: the operator learns what failed and what to try. + assert "stored history" in rendered + assert "/compact" in rendered + + class TestPrepareWireLaneCaps: """The per-attempt wire prep folds with the SERVING lane's capabilities — a fallback whose template rejects mid-conversation diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index 3193c550..1cd8afab 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -6434,6 +6434,7 @@ const _MODEL_CAP_KEYS = [ "supports_web_search", "supports_temperature", "supports_effort", + "server_parses_reasoning", "supports_verbosity", "supports_pro_mode", "supports_transcription", @@ -6448,6 +6449,7 @@ const _MODEL_CAP_DEFAULTS = { supports_web_search: false, supports_temperature: true, supports_effort: false, + server_parses_reasoning: false, supports_verbosity: false, supports_pro_mode: false, supports_transcription: false, @@ -6455,6 +6457,37 @@ const _MODEL_CAP_DEFAULTS = { supports_audio_input: false, supports_rerank: false, }; +// Mirrors ``_CAPABILITY_BOOL_STRINGS`` / ``apply_capability_overrides`` in +// core/model_turn.py. The capabilities dict is hand-edited JSON, so a stored +// string "false" is TRUTHY to JS while the backend reads it as False — lifting +// it into a tile with bare ``!!`` would render the row checked and then persist +// boolean true, inverting the capability on a routine save. Returns undefined +// for anything the backend would not coerce; such a value stays in the raw JSON +// rather than being silently rewritten (the thinking_mode policy below). +const _CAP_BOOL_STRINGS = { + "true": true, + yes: true, + on: true, + 1: true, + "false": false, + no: false, + off: false, + 0: false, + "": false, +}; +function _capBool(value) { + if (typeof value === "boolean") return value; + // bool-before-int, like the Python arm: JS has no bool/int overlap, but + // 0/1 rows must coerce the same way the backend coerces them. + if (typeof value === "number") return !!value; + if (typeof value === "string") { + const spelling = value.trim().toLowerCase(); + return Object.prototype.hasOwnProperty.call(_CAP_BOOL_STRINGS, spelling) + ? _CAP_BOOL_STRINGS[spelling] + : undefined; + } + return undefined; +} let _modelCapsBaseline = {}; // known-model table values (display + delta base) let _modelCapsExplicit = {}; // keys that persist: saved-in-JSON + user-toggled @@ -7912,7 +7945,9 @@ function showEditModelModal(definitionId) { _modelCapsExplicit = {}; _MODEL_CAP_KEYS.forEach(function (k) { if (k in capsObj) { - _modelCapsExplicit[k] = !!capsObj[k]; + const asBool = _capBool(capsObj[k]); + if (asBool === undefined) return; // unrepresentable — leave it raw + _modelCapsExplicit[k] = asBool; delete capsObj[k]; } }); diff --git a/turnstone/console/static/index.html b/turnstone/console/static/index.html index 0430e1ed..2138be4f 100644 --- a/turnstone/console/static/index.html +++ b/turnstone/console/static/index.html @@ -1978,6 +1978,13 @@ >Reasoning-effort control +