Files
6krrt/tests/test_admin_profile_is_default.py
adlee-was-taken 52173c4703 fix(admin): the profiles page claimed a restart the API says is not needed
Setting a new routing default showed "A restart is required before dispatch
sees this change." It has not been true since the no-restart work landed:
`build_router(cfg, ...)` is handed the SAME RouterConfig object the
dispatcher holds and `_resolve_profile` reads `cfg.routing.default_profile`
per request, so POST /api/config/routing.default_profile mutating it lands
on the very next route. The endpoint has been returning "Set as default and
active now -- no restart required" the whole time. The page threw that
response away and asserted the opposite, so operators were told to bounce a
service that had already picked the change up.

Confirmed end to end against a live instance before changing anything: POST
the key, then /route reports `profile: onlycheaps` with no restart.

Three separate strings made the claim, none of them consulting the API -- a
sticky banner after every save, the page subtitle, and the delete dialog.
The same is true of profile definitions: all three CRUD paths go through
`_persist_profile`, which mirrors the write into the running `cfg.profiles`
and answers "Saved and live -- no restart required".

The fix is not to flip the wording, which would drift again the first time a
write genuinely does need a bounce. `_showRestartHint` is gone and the
banner renders the endpoint's own `message`, warning-styled only when the
message actually asks for a restart. The two prose lines say the write
applies to the next request. The page now reports the server instead of
holding a second rule about it.

Four tests, because the interesting half is the contract, not the string:
the write reaches the live config, the endpoint promises no restart, an
ordinary allowlisted key still DOES say restart (the exemption is narrow by
design and must stay that way), and the page hardcodes no claim of its own.
The last one strips `//` lines first -- the comment explaining this fix
quotes the old string, and a test that fails on its own rationale is a test
nobody keeps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
2026-09-12 11:54:25 -04:00

188 lines
7.3 KiB
Python

"""The profiles page can see, and set, which profile bare `auto` resolves to.
`routing.default_profile` decides what every client that does not name a
profile actually gets -- including all 13 opencode agents, which send bare
`llm-router/auto`. It was only reachable from a dropdown on the Controls
page, and the profiles page could not even show which profile was live.
"""
from __future__ import annotations
import sqlite3
from pathlib import Path
import pytest
import yaml
from fastapi import FastAPI
from starlette.testclient import TestClient
import admin
from config import load_config
ROOT = Path(__file__).resolve().parent.parent
SCHEMA_SQL = (ROOT / "config" / "schema.sql").read_text()
@pytest.fixture
def client(tmp_path):
cfg_dir = tmp_path / "config"
cfg_dir.mkdir()
doc = yaml.safe_load((ROOT / "config" / "config.yaml").read_text())
(cfg_dir / "config.yaml").write_text(yaml.safe_dump(doc, sort_keys=False))
db_path = tmp_path / "t.db"
conn = sqlite3.connect(db_path)
conn.executescript(SCHEMA_SQL)
admin.ensure_admin_tables(conn)
conn.commit()
conn.close()
def _db():
c = sqlite3.connect(db_path)
c.row_factory = sqlite3.Row
return c
cfg = load_config(cfg_dir / "config.yaml")
app = FastAPI()
app.include_router(admin.build_router(cfg, _db, str(tmp_path)), prefix="/admin")
return TestClient(app), cfg_dir
def _profiles(c):
return {p["name"]: p for p in c.get("/admin/api/profiles").json()}
def test_exactly_one_profile_is_marked_default(client):
c, _ = client
ps = _profiles(c)
flagged = [n for n, p in ps.items() if p["is_default"]]
assert flagged == ["default"], flagged
def test_setting_the_default_moves_the_flag(client):
c, _ = client
resp = c.post("/admin/api/config/routing.default_profile",
json={"value": "onlycheaps"})
assert resp.status_code == 200, resp.text
ps = _profiles(c)
assert ps["onlycheaps"]["is_default"] is True
assert ps["default"]["is_default"] is False
assert [n for n, p in ps.items() if p["is_default"]] == ["onlycheaps"]
def test_the_flag_reflects_the_overlay_without_a_restart(client):
"""Read from the config store, not from cfg.
cfg binds at import, so sourcing this from cfg would leave the page
showing the old profile as default immediately after setting it -- the
one moment the operator is looking straight at it.
"""
c, cfg_dir = client
c.post("/admin/api/config/routing.default_profile", json={"value": "batch"})
assert "batch" in (cfg_dir / "config.local.yaml").read_text()
assert _profiles(c)["batch"]["is_default"] is True
def test_an_unknown_profile_is_still_refused(client):
"""The button reuses the allowlisted config endpoint, so it inherits the
validation that already exists rather than a second rule that can drift."""
c, _ = client
resp = c.post("/admin/api/config/routing.default_profile",
json={"value": "ghostprofile"})
assert resp.status_code == 422
assert "not a known profile" in resp.text
assert _profiles(c)["default"]["is_default"] is True
def test_a_configured_profile_can_be_made_default(client):
"""Not just built-ins -- an overlay profile is a valid target too."""
c, _ = client
assert c.post("/admin/api/profiles/",
json={"name": "cheapo", "max_cost_per_1m_completion": 0.4}
).status_code == 200
assert c.post("/admin/api/config/routing.default_profile",
json={"value": "cheapo"}).status_code == 200
assert _profiles(c)["cheapo"]["is_default"] is True
def test_deleting_the_default_is_still_refused(client):
"""The UI disables the button; the endpoint must still hold the line."""
c, _ = client
c.post("/admin/api/profiles/", json={"name": "cheapo", "min_tier": 1})
c.post("/admin/api/config/routing.default_profile", json={"value": "cheapo"})
resp = c.delete("/admin/api/profiles/cheapo")
assert resp.status_code == 422
assert "default_profile" in resp.text
def test_setting_the_default_applies_to_the_live_config(client):
"""The write is not deferred to a restart, and the endpoint says so.
`build_router(cfg, ...)` is handed the SAME RouterConfig object the
dispatcher holds, and `_resolve_profile` reads
`cfg.routing.default_profile` per request -- so mutating it lands on the
very next route. Verified end to end against a live instance: POST the
key, then `/route` reports the new profile with no bounce.
"""
c, _ = client
before = c.get("/admin/api/active-profile").json()
assert before["active"] == "default"
assert c.post("/admin/api/config/routing.default_profile",
json={"value": "batch"}).status_code == 200
# `active` is read off the running cfg; `boots_to` is read off disk. Both
# move, and `active` moving is the half a restart would otherwise be for.
after = c.get("/admin/api/active-profile").json()
assert after["active"] == "batch"
assert after["boots_to"] == "batch"
def test_the_endpoint_promises_no_restart_for_the_default_profile(client):
"""The message is the page's only source for the restart claim.
The profiles page used to assert "A restart is required before dispatch
sees this change" after every save, unconditionally, while this endpoint
was already returning the opposite. Operators were told to bounce a
service that had picked the change up on the previous request. The page
now renders whatever comes back here, so this string is load-bearing.
"""
c, _ = client
body = c.post("/admin/api/config/routing.default_profile",
json={"value": "batch"}).json()
assert "no restart" in body["message"].lower()
def test_a_key_that_does_need_a_restart_still_says_so(client):
"""The narrow exemption must stay narrow.
Only routing.default_profile has a live counterpart. Every other
allowlisted key genuinely does need the bounce, so the restart message
has to remain the default answer rather than disappearing wholesale.
"""
c, _ = client
body = c.post("/admin/api/config/logging.level",
json={"value": "DEBUG"}).json()
assert "restart is required" in body["message"].lower()
def test_the_profiles_page_never_hardcodes_a_restart_claim():
"""Guarding the actual defect: a claim the page made on its own.
Three separate strings said a reload was needed -- a sticky banner, the
page subtitle, and the delete dialog -- none of them consulting the API.
The banner is now driven by the response `message`; the two prose lines
are gone. Asserted as an absence, since the failure mode is the claim
coming back, not the fix being undone.
"""
html = (ROOT / "admin" / "frontend" / "profiles.html").read_text()
# Comments are stripped first: the code comment explaining this fix quotes
# the old string, and a test that fails on its own rationale is a test
# nobody keeps.
live = "\n".join(
line for line in html.splitlines() if not line.lstrip().startswith("//")
)
assert "_showRestartHint" not in live
assert "A restart is required before dispatch sees this change" not in live
assert "service reload is required" not in live.lower()
# The banner must read the server rather than decide for itself.
assert "_saveNotice = (result && result.message)" in html