* feat(api): enforce API design rules via pytest + fix DELETE/status-code violations
Adds tests/test_api_design_rules.py with four forward-only design guardrails
that prevent new endpoints from accumulating REST debt:
Rule 1 — No new verbs in URL paths (existing 28 grandfathered via allowlist)
Rule 2 — DELETE must declare 204 No Content (zero allowlist entries)
Rule 3 — Creator POSTs (path has GET counterpart) must declare 201/202
Rule 4 — All protected /api/* routes must declare 401 and 403
Fixes found by running the rules:
- DELETE /api/admin/metrics/{metric_id}: return 204, drop redundant body
- DELETE /api/memory/{item_id}/dismiss (undismiss): return 204, drop body
- POST /api/memory/admin/contradictions: add status_code=201 (creates a resource)
- app/main.py: _add_auth_error_responses() injected into app.openapi() at startup;
declares 401/403 on all protected /api/* operations centrally, fixing the 120
routes that previously omitted these response codes from the spec.
Closes #337
* fix(api): resolve CI failures — extend 204 fixes + complete allowlists
- Fix remaining 6 DELETE endpoints to return 204: store entities,
store entity install, marketplace curated install, marketplace plugin
system flag, admin store submission, and observability view
- Update all affected tests to expect 204 (removed body assertions)
- Add 4 missing verb paths to _VERB_PATH_ALLOWLIST in test_api_design_rules.py
- Add 2 upsert endpoints to _CREATOR_POST_ALLOWLIST
- Update admin_marketplaces.html to not call r.json() on 204 DELETE
* fix(tests): align 2 DELETE-asserting tests with 204 contract (post-#339 rebase)
CI's test-shard (1) and (4) failures on this PR were caused by
Vojta's second commit (`fix(api): resolve CI failures — extend 204
fixes`) flipping more DELETE endpoints to status_code=204 than just
the two mentioned in the PR body. Two tests assert status_code==200
on the DELETE response and broke:
- tests/test_admin_store_submissions.py::TestQuarantineGates::test_admin_can_delete_quarantined
(DELETE /api/store/entities/{entity_id})
- tests/test_store_api.py::TestInstallCycle::test_admin_hard_delete_cascades_installs
(DELETE /api/store/entities/{entity_id}?hard=true)
Updated both to assert 204 with a comment pointing at
tests/test_api_design_rules.py rule 2 so future reviewers can
trace the contract. Verified via broader scan that no other test
asserts == 200 on a .delete() response directly (4 other sites do
.delete() then check 200 on a subsequent GET — those are fine).
* release: 0.54.26 — API design rules (test_api_design_rules.py) + 8 DELETE endpoints flip to 204
---------
Co-authored-by: ZdenekSrotyr <zdenek.srotyr@keboola.com>
268 lines
10 KiB
Python
268 lines
10 KiB
Python
"""Per-user opt-out (dismiss) for curated memory items — v46 feature.
|
|
|
|
Covers the four contract surfaces:
|
|
|
|
1. ``POST /api/memory/{item_id}/dismiss`` — idempotent dismiss; mandatory
|
|
items get a 400 with the governance message; missing items 404.
|
|
2. ``DELETE /api/memory/{item_id}/dismiss`` — idempotent un-dismiss.
|
|
3. ``GET /api/memory?hide_dismissed=true`` — excludes the user's dismissed
|
|
non-mandatory items but never hides mandatory ones (governance).
|
|
4. ``GET /api/memory/bundle`` — always excludes dismissed items for the
|
|
caller, except mandatory ones (the always-on opt-out for AI agents).
|
|
|
|
Plus the listing carries ``dismissed_by_me`` per item so the frontend can
|
|
render the gray-out state without a separate roundtrip.
|
|
"""
|
|
|
|
from src.repositories.knowledge import KnowledgeRepository
|
|
|
|
|
|
def _auth(token):
|
|
return {"Authorization": f"Bearer {token}"}
|
|
|
|
|
|
def _seed_item(conn, item_id: str, title: str, status: str, *, confidence: float | None = None):
|
|
"""Insert a knowledge item directly through the repo + force its status."""
|
|
repo = KnowledgeRepository(conn)
|
|
repo.create(
|
|
id=item_id,
|
|
title=title,
|
|
content=f"Content for {title}",
|
|
category="engineering",
|
|
status=status,
|
|
confidence=confidence,
|
|
)
|
|
# ``create`` honors the passed status, but for mandatory items we also
|
|
# need updated_at refreshed — keep parity with TestBundle's helper.
|
|
repo.update_status(item_id, status)
|
|
|
|
|
|
class TestDismissPost:
|
|
def test_dismiss_writes_row(self, seeded_app):
|
|
"""Non-admin dismissing an approved item lands a row in the table."""
|
|
from src.db import get_system_db
|
|
|
|
conn = get_system_db()
|
|
_seed_item(conn, "dm_a1", "Approved Fact", "approved")
|
|
conn.close()
|
|
|
|
c = seeded_app["client"]
|
|
r = c.post(
|
|
"/api/memory/dm_a1/dismiss",
|
|
headers=_auth(seeded_app["analyst_token"]),
|
|
)
|
|
assert r.status_code == 200, r.text
|
|
body = r.json()
|
|
assert body == {"id": "dm_a1", "dismissed": True}
|
|
|
|
conn = get_system_db()
|
|
cnt = conn.execute(
|
|
"SELECT COUNT(*) FROM knowledge_item_user_dismissed "
|
|
"WHERE user_id = 'analyst1' AND item_id = 'dm_a1'"
|
|
).fetchone()[0]
|
|
assert cnt == 1
|
|
conn.close()
|
|
|
|
def test_dismiss_is_idempotent(self, seeded_app):
|
|
"""Re-dismissing the same item returns 200 and doesn't duplicate the row."""
|
|
from src.db import get_system_db
|
|
|
|
conn = get_system_db()
|
|
_seed_item(conn, "dm_idem", "Approved Fact", "approved")
|
|
conn.close()
|
|
|
|
c = seeded_app["client"]
|
|
for _ in range(3):
|
|
r = c.post(
|
|
"/api/memory/dm_idem/dismiss",
|
|
headers=_auth(seeded_app["analyst_token"]),
|
|
)
|
|
assert r.status_code == 200
|
|
|
|
conn = get_system_db()
|
|
cnt = conn.execute(
|
|
"SELECT COUNT(*) FROM knowledge_item_user_dismissed "
|
|
"WHERE user_id = 'analyst1' AND item_id = 'dm_idem'"
|
|
).fetchone()[0]
|
|
assert cnt == 1
|
|
conn.close()
|
|
|
|
def test_dismiss_mandatory_item_rejected(self, seeded_app):
|
|
"""Mandatory items can never be dismissed — governance hard rule."""
|
|
from src.db import get_system_db
|
|
|
|
conn = get_system_db()
|
|
_seed_item(conn, "dm_m1", "Mandatory Fact", "mandatory")
|
|
conn.close()
|
|
|
|
c = seeded_app["client"]
|
|
r = c.post(
|
|
"/api/memory/dm_m1/dismiss",
|
|
headers=_auth(seeded_app["analyst_token"]),
|
|
)
|
|
assert r.status_code == 400
|
|
assert r.json()["detail"] == "Cannot dismiss a mandatory item"
|
|
|
|
# And nothing landed in the table.
|
|
conn = get_system_db()
|
|
cnt = conn.execute(
|
|
"SELECT COUNT(*) FROM knowledge_item_user_dismissed "
|
|
"WHERE item_id = 'dm_m1'"
|
|
).fetchone()[0]
|
|
assert cnt == 0
|
|
conn.close()
|
|
|
|
def test_dismiss_missing_item_returns_404(self, seeded_app):
|
|
c = seeded_app["client"]
|
|
r = c.post(
|
|
"/api/memory/nope-does-not-exist/dismiss",
|
|
headers=_auth(seeded_app["analyst_token"]),
|
|
)
|
|
assert r.status_code == 404
|
|
|
|
def test_dismiss_requires_auth(self, seeded_app):
|
|
r = seeded_app["client"].post("/api/memory/anything/dismiss")
|
|
assert r.status_code == 401
|
|
|
|
|
|
class TestUndismissDelete:
|
|
def test_delete_undismisses(self, seeded_app):
|
|
"""DELETE removes the dismissal row; subsequent DELETE is still 204."""
|
|
from src.db import get_system_db
|
|
|
|
conn = get_system_db()
|
|
_seed_item(conn, "dm_u1", "Approved Fact", "approved")
|
|
conn.close()
|
|
|
|
c = seeded_app["client"]
|
|
token = seeded_app["analyst_token"]
|
|
c.post("/api/memory/dm_u1/dismiss", headers=_auth(token))
|
|
|
|
r = c.delete("/api/memory/dm_u1/dismiss", headers=_auth(token))
|
|
assert r.status_code == 204
|
|
|
|
# Idempotent: a second DELETE still succeeds — absence of the row
|
|
# is the success state.
|
|
r2 = c.delete("/api/memory/dm_u1/dismiss", headers=_auth(token))
|
|
assert r2.status_code == 204
|
|
|
|
conn = get_system_db()
|
|
cnt = conn.execute(
|
|
"SELECT COUNT(*) FROM knowledge_item_user_dismissed "
|
|
"WHERE user_id = 'analyst1' AND item_id = 'dm_u1'"
|
|
).fetchone()[0]
|
|
assert cnt == 0
|
|
conn.close()
|
|
|
|
def test_delete_missing_item_returns_404(self, seeded_app):
|
|
r = seeded_app["client"].delete(
|
|
"/api/memory/nope-still-no/dismiss",
|
|
headers=_auth(seeded_app["analyst_token"]),
|
|
)
|
|
assert r.status_code == 404
|
|
|
|
|
|
class TestListingHidesDismissed:
|
|
def test_hide_dismissed_excludes_approved_but_keeps_mandatory(self, seeded_app):
|
|
"""``hide_dismissed=true`` filters dismissed approved items but
|
|
leaves mandatory items visible even if a stale dismissal row
|
|
exists for them — the governance hard rule reinforced at the SQL
|
|
layer.
|
|
"""
|
|
from src.db import get_system_db
|
|
|
|
conn = get_system_db()
|
|
_seed_item(conn, "dm_l_app", "Approved To Hide", "approved")
|
|
_seed_item(conn, "dm_l_keep", "Approved To Keep", "approved")
|
|
_seed_item(conn, "dm_l_mand", "Mandatory Survivor", "mandatory")
|
|
# Hand-insert a dismissal row for the mandatory item too — simulates
|
|
# the case where an item was approved + dismissed and later mandated.
|
|
conn.execute(
|
|
"INSERT INTO knowledge_item_user_dismissed (user_id, item_id) VALUES (?, ?)",
|
|
["analyst1", "dm_l_mand"],
|
|
)
|
|
conn.close()
|
|
|
|
c = seeded_app["client"]
|
|
token = seeded_app["analyst_token"]
|
|
# Dismiss the approved item via the API.
|
|
c.post("/api/memory/dm_l_app/dismiss", headers=_auth(token))
|
|
|
|
# Without hide_dismissed the dismissed item still appears.
|
|
r = c.get("/api/memory?per_page=100", headers=_auth(token))
|
|
assert r.status_code == 200
|
|
ids = {it["id"] for it in r.json()["items"]}
|
|
assert {"dm_l_app", "dm_l_keep", "dm_l_mand"} <= ids
|
|
|
|
# With hide_dismissed the approved item disappears, mandatory stays.
|
|
r2 = c.get(
|
|
"/api/memory?per_page=100&hide_dismissed=true",
|
|
headers=_auth(token),
|
|
)
|
|
assert r2.status_code == 200
|
|
ids2 = {it["id"] for it in r2.json()["items"]}
|
|
assert "dm_l_app" not in ids2, (
|
|
"dismissed approved item must be excluded with hide_dismissed=true"
|
|
)
|
|
assert "dm_l_keep" in ids2
|
|
assert "dm_l_mand" in ids2, (
|
|
"mandatory item must remain visible even when a dismissal row exists"
|
|
)
|
|
|
|
def test_listing_carries_dismissed_by_me_flag(self, seeded_app):
|
|
"""Each item in the listing carries ``dismissed_by_me``."""
|
|
from src.db import get_system_db
|
|
|
|
conn = get_system_db()
|
|
_seed_item(conn, "dm_f_yes", "Will be dismissed", "approved")
|
|
_seed_item(conn, "dm_f_no", "Will stay", "approved")
|
|
conn.close()
|
|
|
|
c = seeded_app["client"]
|
|
token = seeded_app["analyst_token"]
|
|
c.post("/api/memory/dm_f_yes/dismiss", headers=_auth(token))
|
|
|
|
r = c.get("/api/memory?per_page=100", headers=_auth(token))
|
|
assert r.status_code == 200
|
|
items_by_id = {it["id"]: it for it in r.json()["items"]}
|
|
assert items_by_id["dm_f_yes"]["dismissed_by_me"] is True
|
|
assert items_by_id["dm_f_no"]["dismissed_by_me"] is False
|
|
|
|
|
|
class TestBundleAlwaysHidesDismissed:
|
|
def test_bundle_excludes_dismissed_approved_but_keeps_mandatory(self, seeded_app):
|
|
"""The bundle endpoint is the always-on opt-out for AI agents —
|
|
no query param needed; dismissed approved items are gone, but
|
|
mandatory items stay regardless of any stale dismissal row.
|
|
"""
|
|
from src.db import get_system_db
|
|
|
|
conn = get_system_db()
|
|
_seed_item(conn, "dm_b_app", "Approved For Bundle", "approved", confidence=0.8)
|
|
_seed_item(conn, "dm_b_keep", "Approved Survivor", "approved", confidence=0.7)
|
|
_seed_item(conn, "dm_b_mand", "Mandatory Bundle Item", "mandatory")
|
|
# Stale dismissal for the mandatory item — must NOT hide it.
|
|
conn.execute(
|
|
"INSERT INTO knowledge_item_user_dismissed (user_id, item_id) VALUES (?, ?)",
|
|
["analyst1", "dm_b_mand"],
|
|
)
|
|
conn.close()
|
|
|
|
c = seeded_app["client"]
|
|
token = seeded_app["analyst_token"]
|
|
# Dismiss the approved item normally.
|
|
c.post("/api/memory/dm_b_app/dismiss", headers=_auth(token))
|
|
|
|
r = c.get("/api/memory/bundle", headers=_auth(token))
|
|
assert r.status_code == 200
|
|
body = r.json()
|
|
mandatory_ids = {i["id"] for i in body["mandatory"]}
|
|
approved_ids = {i["id"] for i in body["approved"]}
|
|
|
|
assert "dm_b_app" not in approved_ids, (
|
|
"dismissed approved item must be excluded from the bundle"
|
|
)
|
|
assert "dm_b_keep" in approved_ids
|
|
assert "dm_b_mand" in mandatory_ids, (
|
|
"mandatory item must remain in the bundle even with a stale dismissal row"
|
|
)
|