mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(console): address Copilot feedback on Models → Roles sub-tab
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.
This commit is contained in:
Patrick Buckley
committed by
Patrick Buckley
Patrick Buckley
parent
35a1c50e60
commit
7db7f99dd8
@@ -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
|
||||
@@ -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 () {
|
||||
|
||||
Reference in New Issue
Block a user