ewilliams28/turnstone
1
0
Fork 0
mirror of https://github.com/turnstonelabs/turnstone.git synced 2026-08-12 23:12:23 -06:00
Code Issues Packages Projects Releases Wiki Activity

fix: align ConfigStore implementation with spec (#153)

Browse Source 
* 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
This commit is contained in:
Patrick Buckley
2026-03-21 16:12:50 -07:00
committed by GitHub
parent 30828e9f9c
commit 5b8ab94446
7 changed files with 82 additions and 108 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/settings.md
+4 -3
View File
@@ -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
tests/test_mcp_registry_api.py
+18 -56
View File
@@ -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)
tests/test_settings_api.py
+4 -1
View File
@@ -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(
turnstone/api/console_spec.py
+3 -1
View File
@@ -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,
turnstone/api/schemas.py
+9
View File
@@ -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.
turnstone/console/server.py
+40 -47
View File
@@ -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
turnstone/console/static/admin.js
+4
View File
@@ -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;