From 5b8ab944465370596374bb5002e5ccc8a7d9ef28 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sat, 21 Mar 2026 16:12:50 -0700 Subject: [PATCH] fix: align ConfigStore implementation with spec (#153) * fix: align ConfigStore implementation with spec - Add cluster + skills sections to admin UI settings order and labels - Return default value in DELETE /v1/api/admin/settings response per spec - Document 4 missing settings in docs/settings.md (trusted_proxies, output_guard, redact_secrets, discovery_url) and correct count to 48 - Wire ConfigStore into console server replacing 4 raw get_system_setting() calls with validated/cached config_store.get() - Reload console ConfigStore on settings mutations via _publish_config_change() - Update registry URL tests for ConfigStore-based resolution * fix: address Copilot review feedback on ConfigStore PR - Move config_store.reload() before collector guard in _publish_config_change() so cache refreshes even without collector - Add DeleteSettingResponse schema and update OpenAPI spec to match the actual delete response (status + key + default) - Add test asserting default field in delete response - Fix stale docstring in test helper --- docs/settings.md | 7 +-- tests/test_mcp_registry_api.py | 74 +++++++------------------- tests/test_settings_api.py | 5 +- turnstone/api/console_spec.py | 4 +- turnstone/api/schemas.py | 9 ++++ turnstone/console/server.py | 87 ++++++++++++++----------------- turnstone/console/static/admin.js | 4 ++ 7 files changed, 82 insertions(+), 108 deletions(-) diff --git a/docs/settings.md b/docs/settings.md index faf94744..aa4e7294 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -51,7 +51,7 @@ connection, Redis, auth secrets, server bind address). These stay in | Bridge identity | `[bridge]` | config.toml / env | | Console bind | `[console]` | config.toml / env | -**ConfigStore settings** (~40 settings) are loaded from the database after +**ConfigStore settings** (48 settings) are loaded from the database after storage initialization: | Section | Settings | @@ -62,9 +62,10 @@ storage initialization: | `server` | workstream_idle_timeout, max_workstreams | | `cluster` | node_fan_out_limit, mcp_max_servers | | `mcp` | config_path, refresh_interval, registry_url | -| `ratelimit` | enabled, requests_per_second, burst | +| `ratelimit` | enabled, requests_per_second, burst, trusted_proxies | | `health` | backend_probe_interval, backend_probe_timeout, circuit_breaker_threshold, circuit_breaker_cooldown | -| `judge` | enabled, model, provider, base_url, api_key, confidence_threshold, max_context_ratio, timeout, read_only_tools | +| `judge` | enabled, model, provider, base_url, api_key, confidence_threshold, max_context_ratio, timeout, read_only_tools, output_guard, redact_secrets | +| `skills` | discovery_url | | `memory` | relevance_k, fetch_limit, max_content, nudge_cooldown, nudges | Settings are addressed by dotted key (e.g. `memory.relevance_k`). Each has a diff --git a/tests/test_mcp_registry_api.py b/tests/test_mcp_registry_api.py index c9044960..a64d36c4 100644 --- a/tests/test_mcp_registry_api.py +++ b/tests/test_mcp_registry_api.py @@ -558,10 +558,11 @@ class TestRegistryInstall: # --------------------------------------------------------------------------- -def _mock_request(storage: Any = None) -> MagicMock: - """Build a mock Request with app.state.auth_storage.""" +def _mock_request(storage: Any = None, config_store: Any = None) -> MagicMock: + """Build a mock Request with app.state.auth_storage and app.state.config_store.""" request = MagicMock() request.app.state.auth_storage = storage + request.app.state.config_store = config_store return request @@ -569,22 +570,20 @@ class TestGetRegistryUrl: """Verify three-tier URL resolution: DB setting -> config.toml -> default.""" def test_returns_db_setting_when_available(self) -> None: - storage = MagicMock() - storage.get_system_setting.return_value = { - "value": json.dumps("https://custom.registry.example.com"), - } - request = _mock_request(storage) + config_store = MagicMock() + config_store.get.return_value = "https://custom.registry.example.com" + request = _mock_request(config_store=config_store) with patch("turnstone.core.config.load_config", return_value={}): url = _get_registry_url(request) assert url == "https://custom.registry.example.com" - storage.get_system_setting.assert_called_once_with("mcp.registry_url") + config_store.get.assert_called_once_with("mcp.registry_url") - def test_falls_back_to_config_when_db_has_no_setting(self) -> None: - storage = MagicMock() - storage.get_system_setting.return_value = None - request = _mock_request(storage) + def test_falls_back_to_config_when_config_store_returns_empty(self) -> None: + config_store = MagicMock() + config_store.get.return_value = "" + request = _mock_request(config_store=config_store) with patch( "turnstone.core.config.load_config", @@ -594,10 +593,8 @@ class TestGetRegistryUrl: assert url == "https://config.registry.example.com" - def test_falls_back_to_config_when_storage_raises_caught_error(self) -> None: - storage = MagicMock() - storage.get_system_setting.side_effect = AttributeError("broken storage") - request = _mock_request(storage) + def test_falls_back_to_config_when_no_config_store(self) -> None: + request = _mock_request() with patch( "turnstone.core.config.load_config", @@ -607,53 +604,18 @@ class TestGetRegistryUrl: assert url == "https://config.registry.example.com" - def test_uncaught_storage_error_propagates(self) -> None: - """Storage errors outside the except tuple are not swallowed.""" - storage = MagicMock() - storage.get_system_setting.side_effect = RuntimeError("DB connection lost") - request = _mock_request(storage) - - with ( - patch("turnstone.core.config.load_config", return_value={}), - pytest.raises(RuntimeError, match="DB connection lost"), - ): - _get_registry_url(request) - def test_falls_back_to_default_when_both_unavailable(self) -> None: - storage = MagicMock() - storage.get_system_setting.side_effect = KeyError("missing") - request = _mock_request(storage) + config_store = MagicMock() + config_store.get.return_value = "" + request = _mock_request(config_store=config_store) with patch("turnstone.core.config.load_config", return_value={}): url = _get_registry_url(request) assert url == DEFAULT_REGISTRY_URL - def test_falls_back_to_default_when_no_storage(self) -> None: - request = _mock_request(storage=None) - - with patch("turnstone.core.config.load_config", return_value={}): - url = _get_registry_url(request) - - assert url == DEFAULT_REGISTRY_URL - - def test_skips_empty_db_value(self) -> None: - storage = MagicMock() - storage.get_system_setting.return_value = {"value": json.dumps("")} - request = _mock_request(storage) - - with patch( - "turnstone.core.config.load_config", - return_value={"registry_url": "https://config.example.com"}, - ): - url = _get_registry_url(request) - - assert url == "https://config.example.com" - - def test_skips_malformed_json_in_db(self) -> None: - storage = MagicMock() - storage.get_system_setting.return_value = {"value": "not-valid-json{"} - request = _mock_request(storage) + def test_falls_back_to_default_when_no_config_store_or_config(self) -> None: + request = _mock_request() with patch("turnstone.core.config.load_config", return_value={}): url = _get_registry_url(request) diff --git a/tests/test_settings_api.py b/tests/test_settings_api.py index ac7e1680..f28f456c 100644 --- a/tests/test_settings_api.py +++ b/tests/test_settings_api.py @@ -179,7 +179,10 @@ class TestDeleteSetting: # Delete it r = client.delete("/v1/api/admin/settings/tools.timeout") assert r.status_code == 200 - assert r.json()["status"] == "ok" + body = r.json() + assert body["status"] == "ok" + assert body["key"] == "tools.timeout" + assert body["default"] == 120 # registry default for tools.timeout def test_delete_then_list_shows_default(self, client): client.put( diff --git a/turnstone/api/console_spec.py b/turnstone/api/console_spec.py index 052c8376..f13a2d60 100644 --- a/turnstone/api/console_spec.py +++ b/turnstone/api/console_spec.py @@ -82,6 +82,7 @@ from turnstone.api.schemas import ( CreateTokenRequest, CreateTokenResponse, CreateUserRequest, + DeleteSettingResponse, ErrorResponse, ListScheduleRunsResponse, ListSchedulesResponse, @@ -751,7 +752,7 @@ CONSOLE_ENDPOINTS: list[EndpointSpec] = [ "/v1/api/admin/settings/{key}", "DELETE", "Reset a setting to its default value", - response_model=StatusResponse, + response_model=DeleteSettingResponse, query_params=[ QueryParam("node_id", "Node ID for node-scoped settings"), ], @@ -855,6 +856,7 @@ CONSOLE_ENDPOINTS: list[EndpointSpec] = [ _ALL_MODELS: list[type[BaseModel]] = [ ErrorResponse, StatusResponse, + DeleteSettingResponse, AuthLoginRequest, AuthLoginResponse, AuthSetupRequest, diff --git a/turnstone/api/schemas.py b/turnstone/api/schemas.py index 57371d01..0b92bdbc 100644 --- a/turnstone/api/schemas.py +++ b/turnstone/api/schemas.py @@ -8,6 +8,7 @@ as the single source of truth for the generated OpenAPI spec. from __future__ import annotations from enum import StrEnum +from typing import Any from pydantic import BaseModel, Field @@ -34,6 +35,14 @@ class StatusResponse(BaseModel): status: str = Field(default="ok", examples=["ok"]) +class DeleteSettingResponse(BaseModel): + """DELETE /v1/api/admin/settings/{key} response.""" + + status: str = Field(default="ok", examples=["ok"]) + key: str = Field(description="Dotted setting key that was reset") + default: Any = Field(description="Registry default value the setting reverted to") + + class AuthLoginRequest(BaseModel): """POST /v1/api/auth/login request body. diff --git a/turnstone/console/server.py b/turnstone/console/server.py index e50f92bc..abaedad6 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -684,19 +684,22 @@ async def _lifespan(app: Starlette) -> AsyncGenerator[None, None]: # auto-rotate via ServiceTokenManager instead of expiring after 1 hour. # Size the pool above the fan-out limit to leave headroom for non-fan-out # proxy traffic (UI proxying, SSE streams, etc.). - fan_out = _NODE_FAN_OUT_LIMIT + # + # Build a ConfigStore so console settings reads get type validation and + # caching instead of raw storage.get_system_setting() calls. storage = getattr(app.state, "auth_storage", None) + config_store = None if storage: try: - row = storage.get_system_setting("cluster.node_fan_out_limit") - if row: - fan_out = int(row["value"]) + from turnstone.core.config_store import ConfigStore + + config_store = ConfigStore(storage) except Exception: - log.warning( - "Failed to read cluster.node_fan_out_limit, using default %d", - fan_out, - exc_info=True, - ) + log.warning("Failed to initialise ConfigStore", exc_info=True) + app.state.config_store = config_store + fan_out = ( + config_store.get("cluster.node_fan_out_limit") if config_store else _NODE_FAN_OUT_LIMIT + ) app.state.fan_out_limit = fan_out app.state.proxy_client = httpx.AsyncClient( timeout=30, @@ -3105,20 +3108,18 @@ async def admin_delete_skill_resource(request: Request) -> JSONResponse: def _get_discovery_url(request: Request) -> str: - """Get skills discovery URL from DB settings, config.toml, or default.""" + """Get skills discovery URL via ConfigStore, config.toml, or default.""" from turnstone.core.config import load_config from turnstone.core.skill_sources import DEFAULT_DISCOVERY_URL - storage = getattr(request.app.state, "auth_storage", None) - if storage: - try: - row = storage.get_system_setting("skills.discovery_url") - if row: - val = json.loads(row["value"]) - if val: - return str(val) - except (KeyError, json.JSONDecodeError, TypeError, AttributeError): - pass + # ConfigStore: validated + cached + config_store = getattr(request.app.state, "config_store", None) + if config_store: + val = config_store.get("skills.discovery_url") + if val: + return str(val) + + # Fall back to config.toml [skills] section skills_cfg = load_config("skills") url = skills_cfg.get("discovery_url", "") if url: @@ -3483,6 +3484,12 @@ async def _publish_config_change(request: Request) -> None: Uses the collector's node registry, the shared async proxy client, and bounded concurrency via the fan-out semaphore. """ + # Reload the console's own ConfigStore so cached values stay fresh + # (must happen even when collector is absent — e.g. standalone console) + config_store = getattr(request.app.state, "config_store", None) + if config_store: + config_store.reload() + collector = getattr(request.app.state, "collector", None) if not collector: return @@ -3695,7 +3702,7 @@ async def admin_delete_setting(request: Request) -> JSONResponse: key = request.path_params["key"] try: - validate_key(key) + defn = validate_key(key) except ValueError: return JSONResponse({"error": f"Unknown setting: {key}"}, status_code=400) @@ -3717,7 +3724,7 @@ async def admin_delete_setting(request: Request) -> JSONResponse: await _publish_config_change(request) - return JSONResponse({"status": "ok", "key": key}) + return JSONResponse({"status": "ok", "key": key, "default": defn.default}) # --------------------------------------------------------------------------- @@ -3726,21 +3733,16 @@ async def admin_delete_setting(request: Request) -> JSONResponse: def _get_registry_url(request: Request) -> str: - """Get the MCP Registry URL from DB settings, config.toml, or default.""" + """Get the MCP Registry URL via ConfigStore, config.toml, or default.""" from turnstone.core.config import load_config from turnstone.core.mcp_registry import DEFAULT_REGISTRY_URL - # Check database settings first - storage = getattr(request.app.state, "auth_storage", None) - if storage: - try: - row = storage.get_system_setting("mcp.registry_url") - if row: - val = json.loads(row["value"]) - if val: - return str(val) - except (KeyError, json.JSONDecodeError, TypeError, AttributeError): - pass + # ConfigStore: validated + cached + config_store = getattr(request.app.state, "config_store", None) + if config_store: + val = config_store.get("mcp.registry_url") + if val: + return str(val) # Fall back to config.toml [mcp] section mcp_cfg = load_config("mcp") @@ -3993,19 +3995,10 @@ _MCP_MAX_SERVERS = 200 # fallback; prefer cluster.mcp_max_servers from storage def _get_mcp_max_servers(request: Request) -> int: - """Read cluster.mcp_max_servers from storage, falling back to the constant.""" - storage = getattr(request.app.state, "auth_storage", None) - if storage: - try: - row = storage.get_system_setting("cluster.mcp_max_servers") - if row: - return int(row["value"]) - except Exception: - log.warning( - "Failed to read cluster.mcp_max_servers, using default %d", - _MCP_MAX_SERVERS, - exc_info=True, - ) + """Read cluster.mcp_max_servers via ConfigStore (validated + cached).""" + config_store = getattr(request.app.state, "config_store", None) + if config_store: + return int(config_store.get("cluster.mcp_max_servers")) return _MCP_MAX_SERVERS diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index 1364c499..e8d3526e 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -2057,10 +2057,12 @@ var _settingsSectionOrder = [ "session", "tools", "server", + "cluster", "mcp", "ratelimit", "health", "judge", + "skills", "memory", ]; @@ -2070,10 +2072,12 @@ function _settingsSectionLabel(section) { session: "Session", tools: "Tools", server: "Server", + cluster: "Cluster", mcp: "MCP", ratelimit: "Rate Limiting", health: "Health", judge: "Judge", + skills: "Skills", memory: "Memory", }; return labels[section] || section;