From 7db7f99dd8d2df92c38334df1ad24c920a76b8f3 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sat, 2 May 2026 17:45:17 -0700 Subject: [PATCH] =?UTF-8?q?fix(console):=20address=20Copilot=20feedback=20?= =?UTF-8?q?on=20Models=20=E2=86=92=20Roles=20sub-tab?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes from PR review: - Permission gating: hide the Roles sub-tab button when the user lacks ``admin.settings``. The sub-tab loads/saves through ``/v1/api/admin/settings``, so an admin with ``admin.models`` but no ``admin.settings`` would otherwise see a perpetual 403 loader. When Roles is the active sub-tab and the permission check fails, snap the panel back to Definitions so the user lands somewhere usable. - Drop the redundant ``/v1/api/admin/model-definitions`` fetch from ``loadAdminModelRoles``. Both entry points (initial Models-tab open + ``models_changed`` SSE refresh) flow through ``loadAdminModels`` first, which already populates ``_modelDefs`` + ``_modelDefaultAlias``; ``_saveModelRole`` doesn't touch model definitions, so the cached snapshot stays accurate when the save chains back here. Halves the per-render request count and removes a wasted round-trip on every cluster-wide model edit. - Add ``test_models_changed_event.py`` covering the SSE fanout the prior commit introduced: each model-definition CRUD endpoint emits exactly one ``models_changed``, settings PUT/DELETE only emit for keys in ``_MODEL_AFFECTING_SETTING_KEYS`` (parametrised over all eight), and unrelated settings (e.g. ``session.retention_days``) don't trigger spurious refreshes. The expected key set is pinned in the test so a stray addition to the allowlist doesn't silently bypass coverage. --- tests/test_models_changed_event.py | 274 +++++++++++++++++++++++++++++ turnstone/console/static/admin.js | 51 ++++-- 2 files changed, 312 insertions(+), 13 deletions(-) create mode 100644 tests/test_models_changed_event.py diff --git a/tests/test_models_changed_event.py b/tests/test_models_changed_event.py new file mode 100644 index 00000000..4e34987d --- /dev/null +++ b/tests/test_models_changed_event.py @@ -0,0 +1,274 @@ +"""``models_changed`` SSE fanout coverage. + +The console pushes a ``models_changed`` cluster event whenever a model +definition is created / updated / deleted / reloaded, or whenever a +setting in :data:`turnstone.console.server._MODEL_AFFECTING_SETTING_KEYS` +is updated or reset. Connected browsers refetch ``/v1/api/models`` on +receipt so the home composer dropdown + admin Models → Roles sub-tab +reflect alias edits without a manual reload. + +These tests pin two contracts: + +- every model-definition CRUD path emits exactly one ``models_changed`` + fanout (so the browser stays in sync with the DB); +- settings PUT / DELETE only emit the fanout when the key is + model-affecting — unrelated keys (e.g. ``session.retention_days``) + must not trigger spurious dropdown re-renders across the cluster. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock + +import pytest +from starlette.applications import Starlette +from starlette.middleware import Middleware +from starlette.routing import Route +from starlette.testclient import TestClient + +from tests._coord_test_helpers import _AuthMiddleware +from turnstone.console.server import ( + _MODEL_AFFECTING_SETTING_KEYS, + admin_create_model_definition, + admin_delete_model_definition, + admin_delete_setting, + admin_model_reload, + admin_update_model_definition, + admin_update_setting, +) +from turnstone.core.storage._sqlite import SQLiteBackend + + +@pytest.fixture +def storage(tmp_path: Any) -> SQLiteBackend: + return SQLiteBackend(str(tmp_path / "models_changed.db")) + + +def _seed(storage: SQLiteBackend, *, definition_id: str, alias: str) -> None: + storage.create_model_definition( + definition_id=definition_id, + alias=alias, + model="model-x", + provider="openai-compatible", + base_url="http://localhost:8000/v1", + api_key="sk-test", + context_window=8192, + capabilities="{}", + enabled=True, + created_by="admin", + ) + + +def _make_client(storage: SQLiteBackend) -> tuple[TestClient, MagicMock]: + """Build a TestClient + return the stub collector for assertion. + + Wires the four model-definition CRUD/reload routes plus the two + settings mutation routes. Collector is a MagicMock so each + ``emit_models_changed`` call lands as a recorded call without + spinning up the full SSE listener queue. + """ + app = Starlette( + routes=[ + Route( + "/v1/api/admin/model-definitions", + admin_create_model_definition, + methods=["POST"], + ), + Route( + "/v1/api/admin/model-definitions/reload", + admin_model_reload, + methods=["POST"], + ), + Route( + "/v1/api/admin/model-definitions/{definition_id}", + admin_update_model_definition, + methods=["PUT"], + ), + Route( + "/v1/api/admin/model-definitions/{definition_id}", + admin_delete_model_definition, + methods=["DELETE"], + ), + Route( + "/v1/api/admin/settings/{key:path}", + admin_update_setting, + methods=["PUT"], + ), + Route( + "/v1/api/admin/settings/{key:path}", + admin_delete_setting, + methods=["DELETE"], + ), + ], + middleware=[Middleware(_AuthMiddleware)], + ) + app.state.auth_storage = storage + app.state.coord_registry = None # CRUD endpoints handle this gracefully + collector = MagicMock() + collector.get_all_nodes.return_value = [] + app.state.collector = collector + app.state.proxy_client = MagicMock() + app.state.config_store = MagicMock() + client = TestClient(app) + client.headers.update( + { + "X-Test-User": "admin", + "X-Test-Perms": "admin.models,admin.settings", + } + ) + return client, collector + + +# --------------------------------------------------------------------------- +# Model-definition CRUD endpoints fan out ``models_changed`` +# --------------------------------------------------------------------------- + + +def test_create_emits_models_changed(storage: SQLiteBackend) -> None: + client, collector = _make_client(storage) + resp = client.post( + "/v1/api/admin/model-definitions", + json={ + "alias": "fast", + "model": "fast-model", + "provider": "openai-compatible", + "base_url": "http://localhost:9000/v1", + "api_key": "sk-x", + "context_window": 4096, + }, + ) + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 1 + + +def test_update_emits_models_changed(storage: SQLiteBackend) -> None: + _seed(storage, definition_id="m1", alias="local") + client, collector = _make_client(storage) + resp = client.put( + "/v1/api/admin/model-definitions/m1", + json={"model": "swapped-model"}, + ) + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 1 + + +def test_update_with_empty_body_does_not_emit(storage: SQLiteBackend) -> None: + """Empty-body PUT writes no rows + skips the registry refresh — no + SSE fanout either, since nothing actually changed.""" + _seed(storage, definition_id="m1", alias="local") + client, collector = _make_client(storage) + resp = client.put("/v1/api/admin/model-definitions/m1", json={}) + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 0 + + +def test_delete_emits_models_changed(storage: SQLiteBackend) -> None: + _seed(storage, definition_id="m1", alias="local") + client, collector = _make_client(storage) + resp = client.delete("/v1/api/admin/model-definitions/m1") + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 1 + + +def test_reload_emits_models_changed(storage: SQLiteBackend) -> None: + _seed(storage, definition_id="m1", alias="local") + client, collector = _make_client(storage) + resp = client.post("/v1/api/admin/model-definitions/reload") + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 1 + + +# --------------------------------------------------------------------------- +# Settings PUT / DELETE only emit for model-affecting keys +# --------------------------------------------------------------------------- + + +# Pinned snapshot of the role-related keys we expect the allowlist to +# cover today. The frozenset itself is asserted further down so a +# stray addition doesn't silently bypass coverage. +_EXPECTED_AFFECTING_KEYS = frozenset( + { + "model.default_alias", + "model.plan_alias", + "model.plan_effort", + "model.task_alias", + "model.task_effort", + "coordinator.model_alias", + "coordinator.reasoning_effort", + "judge.model", + } +) + + +def _value_for_key(key: str) -> str: + """Return a registry-valid value for ``key``. + + ``reasoning_effort`` keys have a fixed choice list; alias-shaped + keys accept arbitrary strings. Avoids per-key custom payloads. + """ + if ( + key.endswith("reasoning_effort") + or key.endswith("plan_effort") + or key.endswith("task_effort") + ): + return "low" + return "anything" + + +def test_affecting_keys_set_matches_expected() -> None: + """Lock in the allowlist so an unintentional removal is caught.""" + assert _MODEL_AFFECTING_SETTING_KEYS == _EXPECTED_AFFECTING_KEYS + + +@pytest.mark.parametrize("key", sorted(_EXPECTED_AFFECTING_KEYS)) +def test_settings_put_emits_for_model_affecting_key(storage: SQLiteBackend, key: str) -> None: + client, collector = _make_client(storage) + resp = client.put( + f"/v1/api/admin/settings/{key}", + json={"value": _value_for_key(key)}, + ) + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 1 + + +@pytest.mark.parametrize("key", sorted(_EXPECTED_AFFECTING_KEYS)) +def test_settings_delete_emits_for_model_affecting_key(storage: SQLiteBackend, key: str) -> None: + client, collector = _make_client(storage) + # Seed a row so DELETE has something to remove (otherwise 404). + client.put( + f"/v1/api/admin/settings/{key}", + json={"value": _value_for_key(key)}, + ) + collector.emit_models_changed.reset_mock() + resp = client.delete(f"/v1/api/admin/settings/{key}") + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 1 + + +def test_settings_put_does_not_emit_for_unrelated_key( + storage: SQLiteBackend, +) -> None: + """Updating a non-model setting (here: a session retention knob) + must not trigger a cluster-wide dropdown refresh.""" + client, collector = _make_client(storage) + resp = client.put( + "/v1/api/admin/settings/session.retention_days", + json={"value": 30}, + ) + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 0 + + +def test_settings_delete_does_not_emit_for_unrelated_key( + storage: SQLiteBackend, +) -> None: + client, collector = _make_client(storage) + client.put( + "/v1/api/admin/settings/session.retention_days", + json={"value": 30}, + ) + collector.emit_models_changed.reset_mock() + resp = client.delete("/v1/api/admin/settings/session.retention_days") + assert resp.status_code == 200, resp.text + assert collector.emit_models_changed.call_count == 0 diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index ba45f5d2..64bb8ead 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -4487,7 +4487,33 @@ var MODEL_ROLES = [ }, ]; +// Roles sub-tab reads/writes via ``/v1/api/admin/settings`` which +// requires ``admin.settings`` — different from the ``admin.models`` +// permission gating the Models tab itself. When the user has Models +// access but not Settings, hide the sub-tab button + force the +// Definitions panel visible so they don't see a perpetual 403 loader. +function _modelRolesAccessible() { + var perms = sessionStorage.getItem("turnstone_permissions") || ""; + return perms.split(",").indexOf("admin.settings") !== -1; +} + +function _applyModelRolesPermission() { + var btn = document.getElementById("models-tab-roles"); + if (!btn) return; + if (_modelRolesAccessible()) { + btn.style.display = ""; + return; + } + btn.style.display = "none"; + // If Roles was the active sub-tab, snap back to Definitions so the + // user isn't staring at a hidden panel. + if (btn.classList.contains("active")) { + switchModelsSection("models-list"); + } +} + function loadAdminModels() { + _applyModelRolesPermission(); authFetch("/v1/api/admin/model-definitions") .then(function (r) { if (!r.ok) throw new Error("Failed"); @@ -4497,9 +4523,12 @@ function loadAdminModels() { _modelDefs = data.models || []; _modelDefaultAlias = data.default_alias || ""; _renderModels(_modelDefs); - // Render the Roles sub-tab off the same model list so the - // dropdowns stay in sync with the current set of aliases. - loadAdminModelRoles(); + // Roles sub-tab piggybacks on the model list; skip it when the + // user has no settings permission since the underlying API will + // 403 anyway. + if (_modelRolesAccessible()) { + loadAdminModelRoles(); + } }) .catch(function () { var el = document.getElementById("admin-models-table"); @@ -4557,10 +4586,12 @@ function _modelRolesError(container, msg) { function loadAdminModelRoles() { var c = document.getElementById("admin-models-roles-container"); if (!c) return; - // Re-fetch model-definitions alongside settings so the alias dropdown - // reflects the live enabled set even if the user toggled a model in - // the Definitions sub-tab between renders. Saving a role also calls - // back here, keeping the dropdown in sync without a tab switch. + // Reads ``_modelDefs`` / ``_modelDefaultAlias`` populated by the most + // recent ``loadAdminModels`` — both entry points into the Models tab + // (initial open + ``models_changed`` SSE refresh) go through + // ``loadAdminModels`` first, so the cached snapshot is fresh. Role + // saves don't change model definitions, so the snapshot stays + // accurate after ``_saveModelRole`` chains back here. Promise.all([ authFetch("/v1/api/admin/settings").then(function (r) { if (!r.ok) throw new Error("settings " + r.status); @@ -4570,10 +4601,6 @@ function loadAdminModelRoles() { if (!r.ok) throw new Error("schema " + r.status); return r.json(); }), - authFetch("/v1/api/admin/model-definitions").then(function (r) { - if (!r.ok) throw new Error("models " + r.status); - return r.json(); - }), ]) .then(function (results) { var values = {}; @@ -4582,8 +4609,6 @@ function loadAdminModelRoles() { var schema = {}; var sa = results[1].schema || []; for (var j = 0; j < sa.length; j++) schema[sa[j].key] = sa[j]; - _modelDefs = results[2].models || _modelDefs; - _modelDefaultAlias = results[2].default_alias || _modelDefaultAlias; _renderModelRoles(c, values, schema); }) .catch(function () {