diff --git a/tests/_coord_test_helpers.py b/tests/_coord_test_helpers.py index 6d56849f..3d5b3725 100644 --- a/tests/_coord_test_helpers.py +++ b/tests/_coord_test_helpers.py @@ -79,18 +79,18 @@ def _fake_registry() -> MagicMock: return reg -def _build_mgr(storage: Any) -> SessionManager: - """Build a SessionManager(CoordinatorAdapter) with stub factories (test default).""" - - def _sf(ui, model_alias=None, ws_id=None, **kw): # type: ignore[no-untyped-def] - s = MagicMock() - s.send.return_value = None - return s +def _build_mgr_with_factory(storage: Any, session_factory: Any) -> SessionManager: + """Build a SessionManager(CoordinatorAdapter) with a caller-supplied factory. + Used by tests that need to capture or assert factory kwargs (e.g. + per-call ``model`` / ``judge_model`` overrides). Plain :func:`_build_mgr` + is the right entry point when the test doesn't care about the + factory. + """ adapter = CoordinatorAdapter( collector=MagicMock(), ui_factory=lambda ws: ConsoleCoordinatorUI(ws_id=ws.id, user_id=ws.user_id or ""), - session_factory=_sf, + session_factory=session_factory, ) mgr = SessionManager( adapter, @@ -103,6 +103,17 @@ def _build_mgr(storage: Any) -> SessionManager: return mgr +def _build_mgr(storage: Any) -> SessionManager: + """Build a SessionManager(CoordinatorAdapter) with stub factories (test default).""" + + def _sf(ui, model_alias=None, ws_id=None, **kw): # type: ignore[no-untyped-def] + s = MagicMock() + s.send.return_value = None + return s + + return _build_mgr_with_factory(storage, _sf) + + class MockStorage: """Minimal storage mock that implements ``list_services``. diff --git a/tests/test_coordinator_endpoints.py b/tests/test_coordinator_endpoints.py index b2e2271b..05ffd968 100644 --- a/tests/test_coordinator_endpoints.py +++ b/tests/test_coordinator_endpoints.py @@ -27,6 +27,7 @@ from starlette.testclient import TestClient from tests._coord_test_helpers import ( _AuthMiddleware, _build_mgr, + _build_mgr_with_factory, _fake_registry, _FakeConfigStore, ) @@ -414,6 +415,107 @@ def test_create_returns_ws_id_and_records_audit(storage): assert "coordinator.create" in actions +def _capture_factory_pair(): + """Return ``(factory, captured)`` — factory records model_alias + + judge_model into the captured dict on every call so tests can assert + the per-call override threading.""" + captured: dict = {} + + def _factory(ui, model_alias=None, ws_id=None, **kw): # type: ignore[no-untyped-def] + captured["model_alias"] = model_alias + captured["judge_model"] = kw.get("judge_model") + return MagicMock() + + return _factory, captured + + +def test_create_forwards_model_and_judge_model_overrides(storage): + """Per-call ``model`` + ``judge_model`` body fields land on the + coord session factory (mirrors interactive's create surface).""" + factory, captured = _capture_factory_pair() + mgr = _build_mgr_with_factory(storage, factory) + client = _make_client(storage, coord_mgr=mgr, registry=_fake_registry()) + resp = client.post( + "/v1/api/workstreams/new", + json={ + "name": "tuned-coord", + "model": "gpt-5", + "judge_model": "gpt-5-mini", + }, + headers=_COORD_HEADERS, + ) + assert resp.status_code == 200, resp.text + assert captured == {"model_alias": "gpt-5", "judge_model": "gpt-5-mini"} + + +def test_create_empty_model_fields_collapse_to_none(storage): + """Empty-string ``model`` / ``judge_model`` body fields don't override + the ConfigStore default — they collapse to ``None`` so the factory + falls back to ``coordinator.model_alias`` / ``judge.model``.""" + factory, captured = _capture_factory_pair() + mgr = _build_mgr_with_factory(storage, factory) + client = _make_client(storage, coord_mgr=mgr, registry=_fake_registry()) + resp = client.post( + "/v1/api/workstreams/new", + json={"name": "default-coord", "model": " ", "judge_model": ""}, + headers=_COORD_HEADERS, + ) + assert resp.status_code == 200, resp.text + assert captured == {"model_alias": None, "judge_model": None} + + +def test_create_503_factory_misconfig_message_is_sanitised(storage): + """503 response from a factory ``ValueError`` strips ASCII control + chars and caps the echoed alias text — defence-in-depth for the + user-controlled ``body["model"]`` reflection surface. Operators + keep the actionable message in the log; clients see a clean + bounded string.""" + + def _factory_raises(ui, model_alias=None, ws_id=None, **kw): # type: ignore[no-untyped-def] + # Simulate the registry's actual exception shape, plus a + # control char + a long-tail attacker payload. + raise ValueError("Unknown model alias: \x00\x07attack\x1b[31m" + ("A" * 1000)) + + mgr = _build_mgr_with_factory(storage, _factory_raises) + client = _make_client(storage, coord_mgr=mgr, registry=_fake_registry()) + resp = client.post( + "/v1/api/workstreams/new", + json={"name": "c"}, + headers=_COORD_HEADERS, + ) + assert resp.status_code == 503 + err = resp.json()["error"] + # Cap enforced (hard-capped at _FACTORY_MISCONFIG_MAX_LEN total — + # the truncation reserves one codepoint for the ellipsis). + assert len(err) <= 200 + assert "\x00" not in err + assert "\x1b" not in err + assert "Unknown model alias" in err + assert err.endswith("…") + + +def test_create_non_string_model_fields_collapse_to_none(storage): + """Non-string ``model`` / ``judge_model`` body fields (e.g. a hostile + dict / list / int) collapse to ``None`` rather than reaching + ``.strip()`` and crashing into the lifted handler's generic 500 + path. Defense-in-depth — the auth gate already requires + ``admin.coordinator``.""" + factory, captured = _capture_factory_pair() + mgr = _build_mgr_with_factory(storage, factory) + client = _make_client(storage, coord_mgr=mgr, registry=_fake_registry()) + resp = client.post( + "/v1/api/workstreams/new", + json={ + "name": "hostile-body", + "model": {"url": "http://evil"}, + "judge_model": [1, 2, 3], + }, + headers=_COORD_HEADERS, + ) + assert resp.status_code == 200, resp.text + assert captured == {"model_alias": None, "judge_model": None} + + _PNG_1X1 = ( b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01" b"\x08\x06\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\rIDATx\x9cc\xfc\xcf" diff --git a/turnstone/console/server.py b/turnstone/console/server.py index 375a07b3..bcdce7ae 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -2636,12 +2636,14 @@ def _coord_create_build_kwargs( ) -> dict[str, Any]: """Build kwargs for ``coord_mgr.create`` from a parsed coord create body. - Coord's create takes a smaller set than interactive's (no - ``model`` / ``judge_model`` / ``client_type`` / ``parent_ws_id`` / - ``ws_id``) — those concepts either don't apply to coordinators - (no parent on coord; coord ws_id is always server-generated) - or live on a separate ConfigStore knob (the dashboard-managed - plan/task model + reasoning_effort settings). + Coord's create still takes a smaller set than interactive's + (no ``client_type`` / ``parent_ws_id`` / ``ws_id`` — coord ws_id + is always server-generated and coord has no parent), but + per-call ``model`` and ``judge_model`` overrides flow through + here onto the coord session factory the same way they flow + through interactive's: ConfigStore (``coordinator.model_alias`` + / ``judge.model``) sets the default; this body field overrides + for one session. """ # Use the canonical skill name from the resolved row when one was # found; falls back to the stripped body value (which is what the @@ -2653,12 +2655,25 @@ def _coord_create_build_kwargs( else: canonical_skill = (body.get("skill") or "").strip() or None name = (body.get("name") or "").strip() + # Empty / non-string / whitespace-only body fields collapse to None + # so the factory falls back to ConfigStore defaults rather than + # treating "" (or a hostile dict / list) as a request to override + # with the empty alias. The isinstance guard also keeps a + # truthy-non-string body (e.g. ``{"model": {"url": "x"}}``) from + # reaching ``.strip()`` and crashing into the lifted handler's + # generic 500 path. + model_raw = body.get("model") + judge_raw = body.get("judge_model") + model = (model_raw.strip() if isinstance(model_raw, str) else "") or None + judge_model = (judge_raw.strip() if isinstance(judge_raw, str) else "") or None return { "user_id": uid, "name": name, "skill": canonical_skill, "skill_id": skill_id, "skill_version": applied_skill_version, + "model": model, + "judge_model": judge_model, } diff --git a/turnstone/console/session_factory.py b/turnstone/console/session_factory.py index 2451ceba..fb784f69 100644 --- a/turnstone/console/session_factory.py +++ b/turnstone/console/session_factory.py @@ -88,6 +88,7 @@ def build_console_session_factory( client_type: str = "web", kind: WorkstreamKind = WorkstreamKind.COORDINATOR, parent_ws_id: str | None = None, + judge_model: str | None = None, ) -> ChatSession: assert ui is not None, "console session_factory requires a non-None UI" if kind != WorkstreamKind.COORDINATOR: @@ -134,6 +135,28 @@ def build_console_session_factory( # name that provider may not even know about (e.g. coordinator on # Anthropic, judge alias pointing at OpenAI gpt-5-mini → silent # ``llm_fallback`` verdicts on every tool call). + if live_judge_config and judge_model: + import dataclasses + + # Per-call judge_model override mirrors the server-side + # interactive factory: pin the alias on the JudgeConfig but + # leave alias→client/provider resolution to IntentJudge for + # the same reason as above. ``registry.resolve`` is only + # called as a typo / unknown-alias guard so a misconfigured + # body field surfaces in the log instead of silently falling + # back to the session's provider. + try: + registry.resolve(judge_model) + live_judge_config = dataclasses.replace( + live_judge_config, + model=judge_model, + ) + except Exception as e: + log.warning( + "coord_factory.judge_model_resolve_failed alias=%r err=%s", + judge_model, + e, + ) eff_temperature = ( r_cfg.temperature diff --git a/turnstone/console/static/app.js b/turnstone/console/static/app.js index 84735eb1..eab5b5b5 100644 --- a/turnstone/console/static/app.js +++ b/turnstone/console/static/app.js @@ -1450,6 +1450,8 @@ function _hasCoordPermission() { function _createCoordinator(opts) { var name = (opts.name || "").trim(); var skill = opts.skill || ""; + var model = (opts.model || "").trim(); + var judgeModel = (opts.judge_model || "").trim(); var task = (opts.task || "").trim(); var errEl = opts.errEl; var setBusy = opts.setBusy || function () {}; @@ -1463,6 +1465,8 @@ function _createCoordinator(opts) { var body = {}; if (name) body.name = name; if (skill) body.skill = skill; + if (model) body.model = model; + if (judgeModel) body.judge_model = judgeModel; if (task) body.initial_message = task; authFetch("/v1/api/workstreams/new", { @@ -1527,6 +1531,7 @@ function _ensureHomeComposerInit() { _homeComposerInit = true; _mountHomeCoordComposer(); _populateHomeSkillDropdown(); + _populateHomeModelDropdowns(); _probeCoordSubsystem(); _refreshHomeComposerVisibility(); } @@ -1555,6 +1560,8 @@ function _mountHomeCoordComposer() { var bits = []; if (v.name) bits.push(v.name); if (v.skill) bits.push(v.skill); + if (v.model) bits.push(v.model); + if (v.judge_model) bits.push("judge: " + v.judge_model); return bits.join(" \u00b7 "); }, fields: [ @@ -1571,6 +1578,22 @@ function _mountHomeCoordComposer() { type: "select", choices: [{ value: "", text: "Use defaults" }], }, + { + id: "model", + label: "Model", + type: "select", + choices: [{ value: "", text: "Default model" }], + }, + { + id: "judge_model", + label: "Judge Model", + type: "select", + // Neutral label — the actual default is ConfigStore + // ``judge.model`` when set, IntentJudge's agent-model + // fallback when not. "Default judge model" doesn't + // mislead either way. + choices: [{ value: "", text: "Default judge model" }], + }, ], }, onSend: function (text) { @@ -1599,6 +1622,30 @@ function _populateHomeSkillDropdown() { }); } +// Populate Model + Judge Model dropdowns from /v1/api/models — same +// list the interactive new-ws modal uses. Empty/default option stays +// at the top so submitting without a choice falls back to the +// ConfigStore-configured coordinator.model_alias / judge.model. +function _populateHomeModelDropdowns() { + if (!_homeCoordComposer) return; + authFetch("/v1/api/models") + .then(function (r) { + return r.ok ? r.json() : { models: [] }; + }) + .then(function (data) { + var choices = (data.models || []).map(function (m) { + var label = + m.alias === m.model ? m.alias : m.alias + " (" + m.model + ")"; + return { value: m.alias, text: label }; + }); + _homeCoordComposer.setOptionChoices("model", choices); + _homeCoordComposer.setOptionChoices("judge_model", choices); + }) + .catch(function () { + /* defaults still work even without the dropdown populated */ + }); +} + // Probe GET /v1/api/workstreams — 200 = subsystem ready; 503 = no model // alias resolvable, show remediation banner. 4xx (auth / permission) is // treated as "unknown, don't flip the banner" because the probe cannot @@ -1654,6 +1701,8 @@ function submitHomeCoord(textFromComposer) { _createCoordinator({ name: opts.name || "", skill: opts.skill || "", + model: opts.model || "", + judge_model: opts.judge_model || "", task: task, errEl: document.getElementById("home-coord-error"), setBusy: function (b) { diff --git a/turnstone/core/session_routes.py b/turnstone/core/session_routes.py index 784de124..9340230a 100644 --- a/turnstone/core/session_routes.py +++ b/turnstone/core/session_routes.py @@ -52,6 +52,37 @@ if TYPE_CHECKING: log = get_logger(__name__) +# Cap echoed factory-misconfig messages. ``ValueError`` from the +# session factory carries an operator-actionable remediation hint +# (``"Unknown model alias: "`` etc.) that the lifted handlers +# surface as a 503 — but the alias portion is user-controlled on the +# create path (body ``model`` / ``judge_model`` fields) so a raw echo +# reflects arbitrary input back into anything that renders the JSON +# error verbatim. Length cap + control-char strip keep the message +# actionable for legit alias typos while neutralising hostile payloads. +_FACTORY_MISCONFIG_MAX_LEN = 200 + + +def _safe_factory_misconfig_message(exc: BaseException) -> str: + """Sanitise a factory-misconfig ``ValueError`` for echo in a 503 body. + + Strips ASCII control characters (``\\x00``-``\\x1f`` + ``\\x7f``) + and truncates to :data:`_FACTORY_MISCONFIG_MAX_LEN`. Empty after + sanitisation falls back to a fixed generic message so a control- + char-only payload doesn't surface as ``"error": ""``. + """ + text = str(exc) + cleaned = "".join(ch for ch in text if ch.isprintable()) + if not cleaned: + return "session factory misconfigured" + if len(cleaned) > _FACTORY_MISCONFIG_MAX_LEN: + # Reserve one codepoint for the ellipsis so the returned string + # is hard-capped at _FACTORY_MISCONFIG_MAX_LEN total, not + # MAX_LEN+1. + cleaned = cleaned[: _FACTORY_MISCONFIG_MAX_LEN - 1] + "…" + return cleaned + + Handler = Callable[["Request"], Awaitable["Response"]] PermissionGate = Callable[["Request"], "JSONResponse | None"] ManagerLookup = Callable[["Request"], tuple["SessionManager | None", "JSONResponse | None"]] @@ -1197,7 +1228,8 @@ def make_open_handler( # text as a 503 so the operator can fix it without # digging through stack traces. Same shape coord used # pre-lift; standardised across both kinds here. - return JSONResponse({"error": str(exc)}, status_code=503) + log.warning("ws.open.factory_misconfig ws_id=%s exc=%r", ws_id[:8], exc) + return JSONResponse({"error": _safe_factory_misconfig_message(exc)}, status_code=503) except Exception: # Bare ``Exception`` is intentional: ``mgr.open`` can # raise from ``adapter.build_session`` (no documented @@ -1768,8 +1800,11 @@ def make_create_handler( # (model alias points at a model that no longer exists, # etc.). Surface the factory's remediation text as 503 so # operators get the actionable message instead of a - # stack-traced 500. - return JSONResponse({"error": str(exc)}, status_code=503) + # stack-traced 500. Sanitiser caps + scrubs the echoed + # text since the alias is user-controlled on the create + # path (body ``model`` / ``judge_model``). + log.warning("ws.create.factory_misconfig exc=%r", exc) + return JSONResponse({"error": _safe_factory_misconfig_message(exc)}, status_code=503) except Exception: # Don't echo the exception text — it can leak internal # paths / frame names. Log with a correlation id and @@ -2235,7 +2270,10 @@ def make_detail_handler(cfg: SessionEndpointConfig) -> Handler: # Session factory misconfig (e.g. a model alias that no # longer resolves). Surface remediation text as 503 # mirroring :func:`make_open_handler`. - return JSONResponse({"error": str(exc)}, status_code=503) + log.warning("ws.detail.factory_misconfig ws_id=%s exc=%r", ws_id[:8], exc) + return JSONResponse( + {"error": _safe_factory_misconfig_message(exc)}, status_code=503 + ) except Exception: # Bare ``Exception`` is intentional — see # :func:`make_open_handler` for the rationale