281f4fbe28
Sweep findings #8 and #10. #9 turned out not to be a defect -- see below. #8. CloudFallbackConfig types base_url as a plain str, so RouterConfig accepted "notaurl" and it landed in the overlay, failing only later at classify time -- when the local classifier is already down and this block is the thing meant to save the request. POST /api/providers/{name} has always rejected the same mistake with _validate_provider_url; both the cloud fallback card and the classifier card's cloud_primary now use it too. #10. Allowlist add was INSERT OR IGNORE, so re-adding an entry with a corrected note returned 200 carrying the ORIGINAL note. There was no way to fix a note through the API or the UI. Now an upsert on (provider, model_id) that updates the note and leaves added_at alone: correcting a note is not a re-admission. #9 was my error, not the code's. The sweep flagged DELETE returning 200 for an absent allowlist entry as a false confirmation, and I changed it to 404 -- which broke test_allowlist_delete_idempotent, a test that pins that exact behaviour with a docstring saying so. That is a deliberate decision, so it is reverted, with a comment at the endpoint pointing at the test. The inconsistency the finding noticed is still real: profile delete and model-override delete both 404 on a missing target while this one does not. Which way that should resolve is a decision rather than a bug fix, so it goes to the sweep plan as an open question instead of being settled here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
158 lines
6.0 KiB
Python
158 lines
6.0 KiB
Python
"""Endpoints that reported success without doing anything.
|
|
|
|
Sweep findings #8, #9 and #10. All three share a shape: a write path that
|
|
answers 200 while the thing the caller asked for did not happen.
|
|
"""
|
|
|
|
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):
|
|
"""Rooted at a throwaway config dir so no write touches the real overlay."""
|
|
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
|
|
|
|
|
|
ALLOWLISTED = "openrouter"
|
|
|
|
|
|
# --- #9: NOT a defect -- the idempotent delete is deliberate ---------------
|
|
#
|
|
# The sweep flagged DELETE returning 200 for an absent entry as a silent
|
|
# false confirmation. It is not an oversight: test_allowlist_delete_idempotent
|
|
# in test_admin_allowlist.py pins exactly that behaviour, docstring and all.
|
|
# The inconsistency with profile delete and model-override delete (both 404)
|
|
# is real and is recorded as an open question in the sweep plan; resolving it
|
|
# is a decision, not a bug fix, so nothing here changes it.
|
|
|
|
def test_deleting_an_absent_entry_is_a_deliberate_200(client):
|
|
c, _ = client
|
|
resp = c.delete(f"/admin/api/providers/{ALLOWLISTED}/allowlist/nope/nope")
|
|
assert resp.status_code == 200, resp.text
|
|
|
|
|
|
def test_deleting_a_real_entry_removes_it(client):
|
|
c, _ = client
|
|
c.post(f"/admin/api/providers/{ALLOWLISTED}/allowlist",
|
|
json={"model_id": "vendor/m", "note": "n"})
|
|
resp = c.delete(f"/admin/api/providers/{ALLOWLISTED}/allowlist/vendor/m")
|
|
assert resp.status_code == 200, resp.text
|
|
ids = [e["model_id"]
|
|
for e in c.get(f"/admin/api/providers/{ALLOWLISTED}/allowlist").json()]
|
|
assert "vendor/m" not in ids
|
|
|
|
|
|
# --- #10: re-adding could not correct a note -------------------------------
|
|
|
|
def test_re_adding_updates_the_note(client):
|
|
c, _ = client
|
|
c.post(f"/admin/api/providers/{ALLOWLISTED}/allowlist",
|
|
json={"model_id": "vendor/m", "note": "first"})
|
|
resp = c.post(f"/admin/api/providers/{ALLOWLISTED}/allowlist",
|
|
json={"model_id": "vendor/m", "note": "corrected"})
|
|
assert resp.status_code == 200, resp.text
|
|
assert resp.json()["note"] == "corrected"
|
|
entry = [e for e in c.get(f"/admin/api/providers/{ALLOWLISTED}/allowlist").json()
|
|
if e["model_id"] == "vendor/m"][0]
|
|
assert entry["note"] == "corrected"
|
|
|
|
|
|
def test_re_adding_does_not_duplicate_the_row(client):
|
|
c, _ = client
|
|
for note in ("a", "b", "c"):
|
|
c.post(f"/admin/api/providers/{ALLOWLISTED}/allowlist",
|
|
json={"model_id": "vendor/m", "note": note})
|
|
rows = [e for e in c.get(f"/admin/api/providers/{ALLOWLISTED}/allowlist").json()
|
|
if e["model_id"] == "vendor/m"]
|
|
assert len(rows) == 1
|
|
|
|
|
|
def test_re_adding_keeps_the_original_added_at(client):
|
|
"""Correcting a note is not a re-admission."""
|
|
c, _ = client
|
|
first = c.post(f"/admin/api/providers/{ALLOWLISTED}/allowlist",
|
|
json={"model_id": "vendor/m", "note": "first"}).json()
|
|
second = c.post(f"/admin/api/providers/{ALLOWLISTED}/allowlist",
|
|
json={"model_id": "vendor/m", "note": "second"}).json()
|
|
assert second["added_at"] == first["added_at"]
|
|
|
|
|
|
# --- #8: an unusable base_url persisted ------------------------------------
|
|
|
|
def test_cloud_fallback_rejects_a_non_url(client):
|
|
c, cfg_dir = client
|
|
resp = c.post("/admin/api/cloud-fallback-config",
|
|
json={"cloud_fallback": {"base_url": "notaurl",
|
|
"model": "m",
|
|
"api_key_env": "K"}})
|
|
assert resp.status_code == 422, resp.text
|
|
assert "http(s)" in resp.text
|
|
overlay = cfg_dir / "config.local.yaml"
|
|
if overlay.exists():
|
|
assert "notaurl" not in overlay.read_text()
|
|
|
|
|
|
def test_cloud_fallback_accepts_a_real_url(client):
|
|
c, cfg_dir = client
|
|
resp = c.post("/admin/api/cloud-fallback-config",
|
|
json={"cloud_fallback": {"base_url": "https://api.example.com/v1",
|
|
"model": "m",
|
|
"api_key_env": "K"}})
|
|
assert resp.status_code == 200, resp.text
|
|
assert "api.example.com" in (cfg_dir / "config.local.yaml").read_text()
|
|
|
|
|
|
def test_cloud_primary_rejects_a_non_url(client):
|
|
"""The classifier card had the same gap as the fallback card."""
|
|
c, _ = client
|
|
resp = c.post("/admin/api/classifier-config",
|
|
json={"mode": "cloud_llm", "cloud_primary_auto": False,
|
|
"cloud_primary": {"base_url": "ftp://nope", "model": "m"}})
|
|
assert resp.status_code == 422, resp.text
|
|
assert "http(s)" in resp.text
|
|
|
|
|
|
def test_clearing_the_fallback_still_works(client):
|
|
"""The None path is the documented remove signal and must stay reachable."""
|
|
c, cfg_dir = client
|
|
c.post("/admin/api/cloud-fallback-config",
|
|
json={"cloud_fallback": {"base_url": "https://api.example.com/v1",
|
|
"model": "m", "api_key_env": "K"}})
|
|
resp = c.post("/admin/api/cloud-fallback-config", json={"cloud_fallback": None})
|
|
assert resp.status_code == 200, resp.text
|
|
assert "api.example.com" not in (cfg_dir / "config.local.yaml").read_text()
|