Follow-ups on the #832 fold: supersession predicate, wire-prep error hygiene, reasoning-parser tile (#986)

* 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>
This commit is contained in:
Patrick Buckley
2026-08-06 15:14:52 -07:00
committed by GitHub
parent f15e53dd36
commit 0cdb679892
7 changed files with 282 additions and 10 deletions
+125
View File
@@ -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("</div>", 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."""
+43
View File
@@ -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
+46 -2
View File
@@ -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
+36 -1
View File
@@ -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];
}
});
+7
View File
@@ -1978,6 +1978,13 @@
></span
><span class="cap-name">Reasoning-effort control</span></label
>
<label class="cap"
><input
type="checkbox"
data-cap="server_parses_reasoning"
/><span class="cap-led"></span
><span class="cap-name">Server-side reasoning parser</span></label
>
<label class="cap"
><input type="checkbox" data-cap="supports_verbosity" /><span
class="cap-led"
+8 -2
View File
@@ -1046,8 +1046,14 @@ def model_turn(
wire = prepare_wire(wire, lane)
except Exception as prep_err:
# A caller-data fault, never a backend signal — typed so the
# retry and fallback ladders cannot treat it as one.
raise WirePreparationError(str(prep_err)) from prep_err
# retry and fallback ladders cannot treat it as one. The
# wrapper carries the cause's CLASS, not its message: this is
# our lowering over the caller's stored history, so the
# message can quote that history, and callers render
# ``str(exc)`` on surfaces that reach the operator and the
# persisted error row. The message rides ``__cause__``, which
# tracebacks and debug logs still have.
raise WirePreparationError(type(prep_err).__name__) from prep_err
wire = maybe_attach_vllm_chat_reasoning(wire, lane.provider, lane.registry, lane.alias, cfg=cfg)
# The effort assignment scheme's lower rungs: explicit relay → lane
# (operator) → in-code model definition → None. None/unset knobs are
+17 -5
View File
@@ -5795,11 +5795,17 @@ class ChatSession:
# the backend's. Checked FIRST: the overflow text-match below must
# not claim a lowering error whose message mentions token limits.
if isinstance(exc, WirePreparationError):
# Exception TYPE only, never its message. Every other branch
# tails the BACKEND's own diagnostic; this one would carry our
# lowering's message over the session's STORED HISTORY, and
# this string is both shown to the operator and persisted to
# ``last_error`` for a coordinating agent to read. The debug
# traceback logged alongside localizes the raise site.
prep_cause = exc.__cause__
detail = f"{type(prep_cause).__name__}: {prep_cause}" if prep_cause else raw_msg
detail = type(prep_cause).__name__ if prep_cause else type(exc).__name__
return (
f"Preparing the request from this conversation's history failed: "
f"{detail}. This is a fault in the session's stored history, not "
f"Preparing the request from this conversation's history failed "
f"({detail}). This is a fault in the session's stored history, not "
f"in the {model_label} backend (backend health is unaffected). "
f"/compact may clear a malformed turn; please report this."
)
@@ -6555,7 +6561,7 @@ class ChatSession:
"""
if self._cancel_event.is_set():
raise GenerationCancelled()
if my_generation and my_generation != self._generation:
if _generation_superseded(self, my_generation):
raise GenerationCancelled()
def _claim_generation(self) -> int:
@@ -7568,6 +7574,12 @@ class ChatSession:
# through (largely) untruncated instead of snipping them only to
# summarise them moments later. Generation-guarded so an
# orphaned thread can't replace history under the active one.
# Positive equality, not ``_generation_superseded``: inside
# ``send`` my_generation is always a CLAIMED one (>= 1), and
# the two spellings agree there. They part at generation 0,
# which the helper reads as unscoped-and-live — so if this
# guard is ever converted, keep the "am I still the active
# generation" reading rather than "was I superseded".
pre_attempted_compact = False
if self._generation == my_generation and self._compaction_owed():
self._do_auto_compact("mid-turn", preserve_tail=1, my_generation=my_generation)
@@ -9559,7 +9571,7 @@ class ChatSession:
because nobody is waiting on a force-abandoned compaction and its
notice mid-turn reads as the LIVE work being cancelled.
"""
stale = bool(my_generation and my_generation != self._generation)
stale = _generation_superseded(self, my_generation)
event: dict[str, Any] = {"compaction_id": my_generation, "superseded": stale, **payload}
if payload.get("phase") == "end" and not payload.get("ok"):
event["notice"] = (