diff --git a/CHANGELOG.md b/CHANGELOG.md index f498321..7bd05ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,33 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C ## [Unreleased] +## [0.54.26] — 2026-05-18 + +### Changed +- **BREAKING:** eight `DELETE` endpoints that previously returned `200` with + a JSON body now correctly return `204 No Content` (HTTP semantics for + idempotent removal). External clients that parsed the response body + (e.g. `r.json()["status"]`) will hit JSON-decode errors against the now- + empty payload and must drop the body read: + `DELETE /api/admin/metrics/{id}`, `DELETE /api/memory/{id}/dismiss`, + `DELETE /api/store/entities/{id}`, + `DELETE /api/store/entities/{id}/install`, + `DELETE /api/marketplace/curated/{marketplace}/{plugin}/install`, + `DELETE /api/marketplaces/{marketplace}/plugins/{plugin}/system`, + `DELETE /api/admin/store/submissions/{id}`, and + `DELETE /api/admin/observability/views/{id}`. +- **BREAKING:** `POST /api/memory/admin/contradictions` now returns `201 + Created` instead of `200 OK` on success (creator-POST contract). + +### Internal +- Added `tests/test_api_design_rules.py` — four forward-only design guardrails that + prevent new endpoints from adding to existing REST debt: no new verbs in URL paths, + `DELETE` must declare 204, creator `POST`s must declare 201, and all protected + `/api/*` routes must declare 401 and 403. +- `_add_auth_error_responses()` injected into `app.openapi()` at startup to + declare 401/403 on all protected `/api/*` operations centrally — 220 ops + now carry the auth-error responses in the spec. + ## [0.54.25] — 2026-05-18 ### Fixed diff --git a/app/api/admin.py b/app/api/admin.py index 4589422..697853b 100644 --- a/app/api/admin.py +++ b/app/api/admin.py @@ -4042,7 +4042,7 @@ async def admin_retry_store_submission( return {"ok": True, "submission_id": submission_id, "status": "pending_llm"} -@router.delete("/store/submissions/{submission_id}") +@router.delete("/store/submissions/{submission_id}", status_code=204) async def admin_delete_store_submission( submission_id: str, user: dict = Depends(require_admin), @@ -4082,7 +4082,6 @@ async def admin_delete_store_submission( "status": sub.get("status"), }, ) - return {"ok": True} # --------------------------------------------------------------------------- diff --git a/app/api/marketplace.py b/app/api/marketplace.py index e23746b..88af51f 100644 --- a/app/api/marketplace.py +++ b/app/api/marketplace.py @@ -1870,7 +1870,7 @@ async def curated_install( @router.delete( "/curated/{marketplace_id}/{plugin_name}/install", - response_model=InstallActionResponse, + status_code=204, ) async def curated_uninstall( marketplace_id: str, @@ -1902,7 +1902,6 @@ async def curated_uninstall( f"plugin:{marketplace_id}/{plugin_name}", ) _invalidate_etag() - return InstallActionResponse(installed=False) # --------------------------------------------------------------------------- diff --git a/app/api/marketplaces.py b/app/api/marketplaces.py index ab2a4bf..8e660d2 100644 --- a/app/api/marketplaces.py +++ b/app/api/marketplaces.py @@ -643,7 +643,7 @@ def mark_plugin_system( @router.delete( "/{marketplace_id}/plugins/{plugin_name}/system", - response_model=SystemFlagResponse, + status_code=204, ) def unmark_plugin_system( marketplace_id: str, @@ -676,9 +676,3 @@ def unmark_plugin_system( None, ) _invalidate_marketplace_etag() - - return SystemFlagResponse( - marketplace_id=marketplace_id, - plugin_name=plugin_name, - is_system=False, - ) diff --git a/app/api/memory.py b/app/api/memory.py index d1ea562..1c12afb 100644 --- a/app/api/memory.py +++ b/app/api/memory.py @@ -578,13 +578,13 @@ async def dismiss_item( return {"id": item_id, "dismissed": True} -@router.delete("/{item_id}/dismiss") +@router.delete("/{item_id}/dismiss", status_code=204) async def undismiss_item( item_id: str, user: dict = Depends(get_current_user), conn: duckdb.DuckDBPyConnection = Depends(_get_db), ): - """Idempotent un-dismiss — a second DELETE still returns 200. + """Idempotent un-dismiss — a second DELETE still returns 204. Returns 404 if the item itself doesn't exist (consistent with the rest of the per-item endpoints); the dismissal row's existence is not @@ -595,7 +595,6 @@ async def undismiss_item( if not item or not _can_view_item(user, item, _is_privileged_viewer(user, conn)): raise HTTPException(status_code=404, detail="Knowledge item not found") repo.undismiss(user["id"], item_id) - return {"id": item_id, "dismissed": False} @router.get("/{item_id}/provenance") @@ -861,7 +860,7 @@ async def admin_contradictions( return {"contradictions": contradictions, "count": len(contradictions)} -@router.post("/admin/contradictions") +@router.post("/admin/contradictions", status_code=201) async def admin_create_contradiction( request: CreateContradictionRequest, user: dict = Depends(require_admin), diff --git a/app/api/metrics.py b/app/api/metrics.py index 8883b00..6f1a335 100644 --- a/app/api/metrics.py +++ b/app/api/metrics.py @@ -96,7 +96,7 @@ async def create_or_update_metric( return metric -@router.delete("/api/admin/metrics/{metric_id:path}") +@router.delete("/api/admin/metrics/{metric_id:path}", status_code=204) async def delete_metric( metric_id: str, user: dict = Depends(require_admin), @@ -107,7 +107,6 @@ async def delete_metric( deleted = repo.delete(metric_id) if not deleted: raise HTTPException(status_code=404, detail=f"Metric '{metric_id}' not found") - return {"status": "deleted", "id": metric_id} @router.post("/api/admin/metrics/import", status_code=200) diff --git a/app/api/observability.py b/app/api/observability.py index 57bb756..afffe82 100644 --- a/app/api/observability.py +++ b/app/api/observability.py @@ -236,7 +236,7 @@ def save_view( return ObservabilityViewsRepository(conn).create(user_id, name, query) -@router.delete("/views/{view_id}") +@router.delete("/views/{view_id}", status_code=204) def delete_view( view_id: str, user: dict = Depends(require_admin), @@ -246,4 +246,3 @@ def delete_view( ok = ObservabilityViewsRepository(conn).delete(user_id, view_id) if not ok: raise HTTPException(status_code=404, detail="view not found") - return {"deleted": view_id} diff --git a/app/api/store.py b/app/api/store.py index 2cfbb6e..99a0671 100644 --- a/app/api/store.py +++ b/app/api/store.py @@ -2333,7 +2333,7 @@ async def _restore_version_locked( # --------------------------------------------------------------------------- -@router.delete("/entities/{entity_id}", response_model=OkResponse) +@router.delete("/entities/{entity_id}", status_code=204) async def delete_entity( entity_id: str, hard: bool = False, @@ -2412,7 +2412,7 @@ async def delete_entity( "owner_user_id": entity.get("owner_user_id")}, ) _invalidate_etag() - return OkResponse() + return # Soft archive — preserves disk + installs + audit chain. # v36+: archive renames the entity row's `name` (appends @@ -2473,7 +2473,6 @@ async def delete_entity( "by_admin": is_admin_caller and entity["owner_user_id"] != user["id"]}, ) _invalidate_etag() - return OkResponse() # --------------------------------------------------------------------------- @@ -2508,7 +2507,7 @@ async def install_entity( return InstallResponse(entity_id=entity_id, installed=True) -@router.delete("/entities/{entity_id}/install", response_model=InstallResponse) +@router.delete("/entities/{entity_id}/install", status_code=204) async def uninstall_entity( entity_id: str, user: dict = Depends(get_current_user), @@ -2520,7 +2519,6 @@ async def uninstall_entity( StoreEntitiesRepository(conn).bump_install_count(entity_id, -1) _audit(conn, user["id"], "store.entity.uninstall", entity_id) _invalidate_etag() - return InstallResponse(entity_id=entity_id, installed=False) # --------------------------------------------------------------------------- diff --git a/app/main.py b/app/main.py index bc9976f..2835547 100644 --- a/app/main.py +++ b/app/main.py @@ -894,7 +894,53 @@ def create_app() -> FastAPI: body["error"] = str(exc) return JSONResponse(body, status_code=500) + _patch_openapi_auth_errors(app) + return app +# --------------------------------------------------------------------------- +# OpenAPI schema post-processing +# --------------------------------------------------------------------------- + +#: Paths that are intentionally unauthenticated. Every other /api/* route +#: gets 401 and 403 injected into its declared responses so the spec truthfully +#: reflects that auth errors are possible. FastAPI cannot derive these from +#: Depends() chains automatically. +_PUBLIC_API_PATHS = frozenset({ + "/api/health", + "/api/health/detailed", + "/api/version", +}) + +_HTTP_METHODS = frozenset({"get", "post", "put", "delete", "patch"}) + + +def _add_auth_error_responses(schema: dict) -> dict: + """Inject 401/403 into every protected /api/* operation.""" + _401 = {"description": "Not authenticated"} + _403 = {"description": "Insufficient permissions"} + for path, methods in schema.get("paths", {}).items(): + if not path.startswith("/api/") or path in _PUBLIC_API_PATHS: + continue + for method, op in methods.items(): + if method not in _HTTP_METHODS: + continue + responses = op.setdefault("responses", {}) + responses.setdefault("401", _401) + responses.setdefault("403", _403) + return schema + + +def _patch_openapi_auth_errors(app: "FastAPI") -> None: + """Wrap app.openapi() to call _add_auth_error_responses on every generation.""" + original = app.openapi + + def patched() -> dict: + schema = original() + return _add_auth_error_responses(schema) + + app.openapi = patched # type: ignore[method-assign] + + app = create_app() diff --git a/app/web/templates/admin_marketplaces.html b/app/web/templates/admin_marketplaces.html index 9a9a00c..64032c1 100644 --- a/app/web/templates/admin_marketplaces.html +++ b/app/web/templates/admin_marketplaces.html @@ -699,8 +699,8 @@ async function performToggleSystem(btn, marketplaceId, marketplaceName, isSystem btn.innerHTML = prevHtml; return; } - const result = await r.json(); if (!isSystem) { + const result = await r.json(); toast(`Marked ${pluginName} as system (${result.affected_groups} groups, ${result.affected_users} users)`, "success"); } else { toast(`Unmarked ${pluginName} from system`, "success"); diff --git a/pyproject.toml b/pyproject.toml index c175b4f..a1a6ccc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "agnes-the-ai-analyst" -version = "0.54.25" +version = "0.54.26" description = "Agnes — AI Data Analyst platform for AI analytical systems" requires-python = ">=3.11,<3.14" license = "MIT" diff --git a/tests/test_admin_store_submissions.py b/tests/test_admin_store_submissions.py index 6a80aaf..b072f01 100644 --- a/tests/test_admin_store_submissions.py +++ b/tests/test_admin_store_submissions.py @@ -634,7 +634,7 @@ class TestAdminDelete: f"/api/admin/store/submissions/{sid}", cookies=admin_cookies, ) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text conn = get_system_db() assert StoreEntitiesRepository(conn).get("e2") is None @@ -1737,7 +1737,9 @@ class TestQuarantineGates: r = web_client.delete( f"/api/store/entities/{entity_id}", cookies=admin_cookies, ) - assert r.status_code == 200, r.text + # DELETE returns 204 No Content per the API design rule landed in + # this PR (tests/test_api_design_rules.py rule 2). + assert r.status_code == 204, r.text def test_non_owner_non_admin_cannot_view_quarantined(self, web_client): """Random user navigating to ANY per-entity asset endpoint gets @@ -1980,7 +1982,7 @@ class TestArchiveSoftDelete: r = web_client.delete( f"/api/store/entities/{eid_v1}", cookies=owner_cookies, ) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text # Re-upload under the original name — must succeed. eid_v2 = self._upload_clean(web_client, owner_cookies, name="myskill") @@ -2120,7 +2122,7 @@ class TestArchiveSoftDelete: conn.close() r = web_client.delete(f"/api/store/entities/{eid}", cookies=owner_cookies) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text conn = get_system_db() ent = StoreEntitiesRepository(conn).get(eid) @@ -2151,7 +2153,7 @@ class TestArchiveSoftDelete: _, admin_cookies = _create_admin(web_client) r = web_client.delete(f"/api/store/entities/{eid}?hard=true", cookies=admin_cookies) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text conn = get_system_db() assert StoreEntitiesRepository(conn).get(eid) is None @@ -2226,7 +2228,7 @@ class TestArchiveSoftDelete: _, admin_cookies = _create_admin(web_client) r = web_client.delete(f"/api/store/entities/{eid}", cookies=admin_cookies) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text conn = get_system_db() ent = StoreEntitiesRepository(conn).get(eid) @@ -2363,7 +2365,7 @@ class TestSubmissionLifecycleMarking: _, admin_cookies = _create_admin(web_client) r = web_client.delete(f"/api/store/entities/{eid}?hard=true", cookies=admin_cookies) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text conn = get_system_db() sub = StoreSubmissionsRepository(conn).get(sub_id) @@ -2425,7 +2427,7 @@ class TestSubmissionLifecycleMarking: _, admin_cookies = _create_admin(web_client) r = web_client.delete(f"/api/store/entities/{eid}?hard=true", cookies=admin_cookies) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text # Detail page must render and include at least one audit row # (creation events scoped to store_entity:{eid} would otherwise diff --git a/tests/test_api.py b/tests/test_api.py index 2396c1d..4255869 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -310,8 +310,7 @@ class TestMetricsAPI: client.post("/api/admin/metrics", json=SAMPLE_METRIC, headers=headers) resp = client.delete("/api/admin/metrics/finance/mrr", headers=headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "deleted" + assert resp.status_code == 204 resp = client.get("/api/metrics/finance/mrr", headers=headers) assert resp.status_code == 404 diff --git a/tests/test_api_design_rules.py b/tests/test_api_design_rules.py new file mode 100644 index 0000000..426e559 --- /dev/null +++ b/tests/test_api_design_rules.py @@ -0,0 +1,248 @@ +"""API design rule enforcement — prevents new violations from accumulating. + +Existing violations are captured in allowlists: visible, deliberate, +and documented so they can be shrunk over time. + +See: https://github.com/keboola/agnes-the-ai-analyst/issues/337 +""" + +import os +from pathlib import Path + +import pytest + +SNAPSHOT_PATH = Path(__file__).parent / "snapshots" / "openapi.json" +_HTTP_METHODS = {"get", "post", "put", "delete", "patch", "head", "options"} + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(scope="module") +def spec(): + """Boot the app in test mode — same fixture strategy as test_openapi_snapshot.""" + os.environ.setdefault("TESTING", "1") + from app.main import create_app + + return create_app().openapi() + + +def _ops(spec): + for path, methods in spec.get("paths", {}).items(): + for method, op in methods.items(): + if method in _HTTP_METHODS: + yield path, method, op + + +# --------------------------------------------------------------------------- +# Rule 1 — No new verbs in URL path segments +# +# Rationale: verb-in-URL encodes intent in the path rather than the HTTP method, +# which breaks REST client assumptions, prevents generic caching/retry logic, +# and makes the API surface harder to discover. +# +# Exceptions: RPC-style command-bus operations where the HTTP method genuinely +# cannot express the intent (e.g. fire-and-forget triggers, state machines). +# These are explicitly listed below so the allowlist is self-documenting. +# --------------------------------------------------------------------------- + +_VERBS = frozenset({ + "trigger", "run", "activate", "deactivate", "approve", "reject", "revoke", + "register", "discover", "refresh", "reset", "send", "import", "export", + "push", "pull", "enable", "disable", "rebuild", "reload", "bulk", "precheck", + "rescan", +}) + +# Existing violations — grandfathered. Do not extend this list. +# Each entry should include a brief note on why it is intentional RPC. +_VERB_PATH_ALLOWLIST = frozenset({ + # Command-bus triggers — fire-and-forget, no idiomatic REST resource + "/api/sync/trigger", + "/api/scripts/run", + "/api/scripts/run-due", + "/api/scripts/{script_id}/run", + "/api/marketplaces/{marketplace_id}/sync", + "/api/marketplaces/sync-all", + # State transitions on governance resources + "/api/memory/admin/approve", + "/api/memory/admin/reject", + "/api/memory/admin/revoke", + "/api/memory/admin/bulk-update", + # User lifecycle — activate/deactivate map to a boolean field (acceptable PATCH candidate) + "/api/users/{user_id}/activate", + "/api/users/{user_id}/deactivate", + "/api/users/{user_id}/reset-password", + # Admin operations — discovery + registration (complex multi-step, no single resource) + "/api/admin/discover-and-register", + "/api/admin/discover-tables", + "/api/admin/register-table", + "/api/admin/register-table/precheck", + "/api/admin/metadata/{table_id}/push", + "/api/admin/metrics/import", + # Profile refresh — triggers async re-profiling of table metadata + "/api/catalog/profile/{table_name}/refresh", + # BQ metadata cache refresh — on-demand operator trigger for a single registry row + "/api/v2/metadata-cache/refresh", + # Cache warmup — manual trigger (idempotent fire-and-forget) + "/api/admin/cache-warmup/run", + # Store submission rescan — re-runs guardrail scan on an existing submission + "/api/admin/store/submissions/{submission_id}/rescan", + # Telemetry export — GET because it streams a report, not a resource collection + "/api/admin/telemetry/export", + # Auth flows — /auth/* uses verb-style paths by convention across the industry + "/auth/email/send-link", + "/auth/password/reset", + "/auth/password/reset/confirm", + "/auth/password/setup", + "/auth/password/setup/confirm", + "/auth/password/setup/request", + # Sync sub-resources — "sync" is the resource namespace here, not the verb + "/api/sync/manifest", + "/api/sync/settings", + "/api/sync/table-subscriptions", +}) + + +def test_no_new_verbs_in_path(spec): + """New path segments must not contain action verbs.""" + violations = [] + for path, method, _ in _ops(spec): + if path in _VERB_PATH_ALLOWLIST: + continue + segs = [s for s in path.split("/") if s and not s.startswith("{")] + hits = [s for s in segs if s.lower() in _VERBS] + if hits: + violations.append(f" {method.upper():6} {path} (verbs: {hits})") + + assert not violations, ( + f"{len(violations)} new verb-in-URL violation(s):\n" + "\n".join(violations) + "\n\n" + "Fix: model the action as a resource state change (noun + HTTP method).\n" + "If the operation is genuinely RPC (fire-and-forget, state machine), add to " + "_VERB_PATH_ALLOWLIST with a comment explaining why." + ) + + +# --------------------------------------------------------------------------- +# Rule 2 — DELETE must return 204 No Content +# +# Rationale: DELETE is idempotent; 204 signals successful removal without a +# response body. Returning 200 with a body on DELETE conflates "removed" with +# "here is the removed representation" — which is a read concern, not a write one. +# +# No allowlist: the two pre-existing violations were fixed in this PR. +# --------------------------------------------------------------------------- + + +def test_delete_returns_204(spec): + """DELETE operations must declare 204 No Content.""" + violations = [] + for path, method, op in _ops(spec): + if method != "delete": + continue + codes = set(op.get("responses", {}).keys()) + if "204" not in codes: + violations.append(f" DELETE {path} (declares: {sorted(codes)})") + + assert not violations, ( + f"{len(violations)} DELETE endpoint(s) not declaring 204:\n" + "\n".join(violations) + "\n\n" + "Fix: return Response(status_code=204) and remove any response body.\n" + "If the endpoint intentionally returns content after deletion, return 200 and " + "add a response_model — then add it to an allowlist here with a comment." + ) + + +# --------------------------------------------------------------------------- +# Rule 3 — True creator POSTs must declare 201 Created +# +# Heuristic: a POST is a "creator" if the same path also has a GET method +# (i.e. it is a collection endpoint with read+write). Pure RPC commands +# (/api/query, /api/sync/trigger) have no GET counterpart and are excluded. +# +# Allowlist: false positives from the heuristic (upserts, config saves, +# auth flows that respond with 200 by design). +# --------------------------------------------------------------------------- + +_CREATOR_POST_ALLOWLIST = frozenset({ + # Config upserts — update existing config, not create a new resource + "/api/admin/server-config", + "/api/sync/settings", + # Subscription upsert — sets per-table enabled flags, not a pure create + "/api/sync/table-subscriptions", + # Auth flows — 200 is conventional for token/session responses + "/auth/email/verify", + "/auth/password/reset", + "/auth/password/setup", + # Register/update upsert — saves config, not a pure create + "/api/admin/initial-workspace", + # Saved-view upsert — ON CONFLICT updates existing name rather than creating + "/api/admin/observability/views", +}) + + +def test_creator_post_declares_201(spec): + """POST on a collection endpoint (path also has GET) must declare 201 or 202.""" + violations = [] + paths = spec.get("paths", {}) + for path, methods in paths.items(): + if "post" not in methods or "get" not in methods: + continue + if path in _CREATOR_POST_ALLOWLIST: + continue + last = path.rstrip("/").split("/")[-1] + if last.startswith("{"): + continue # item endpoint, not collection + op = methods["post"] + codes = set(op.get("responses", {}).keys()) + if "201" not in codes and "202" not in codes: + violations.append(f" POST {path} (declares: {sorted(codes)})") + + assert not violations, ( + f"{len(violations)} creator POST(s) missing 201/202:\n" + "\n".join(violations) + "\n\n" + "Fix: add responses={{201: {{...}}}} (sync create) or 202 (async create) to the decorator.\n" + "If the POST is an upsert or config save rather than a create, add to " + "_CREATOR_POST_ALLOWLIST with a comment." + ) + + +# --------------------------------------------------------------------------- +# Rule 4 — Protected /api/* endpoints must declare 401 and 403 +# +# Rationale: auth errors are real contract elements. Clients (including LLMs) +# that read the spec to understand retry / fallback behaviour need to know +# these codes exist. The declarations are injected centrally via +# _add_auth_error_responses() in app/main.py, so per-route boilerplate is +# not required. +# +# Public paths: intentionally unauthenticated (health probes, auth entry points). +# --------------------------------------------------------------------------- + +_PUBLIC_API_PATHS = frozenset({ + "/api/health", + "/api/health/detailed", + "/api/version", +}) + + +def test_protected_endpoints_declare_auth_errors(spec): + """Every /api/* endpoint not in PUBLIC must declare 401 and 403.""" + violations = [] + for path, method, op in _ops(spec): + if not path.startswith("/api/"): + continue + if path in _PUBLIC_API_PATHS: + continue + codes = set(op.get("responses", {}).keys()) + missing = [c for c in ("401", "403") if c not in codes] + if missing: + violations.append( + f" {method.upper():6} {path} (missing: {', '.join(missing)})" + ) + + assert not violations, ( + f"{len(violations)} protected endpoint(s) missing auth error declarations:\n" + + "\n".join(violations[:40]) + + ("\n … (truncated)" if len(violations) > 40 else "") + + "\n\nFix: ensure the path is covered by _add_auth_error_responses() in app/main.py, " + "or add to _PUBLIC_API_PATHS above if it is genuinely unauthenticated." + ) diff --git a/tests/test_marketplace_api.py b/tests/test_marketplace_api.py index 13f0e02..c7563de 100644 --- a/tests/test_marketplace_api.py +++ b/tests/test_marketplace_api.py @@ -457,7 +457,7 @@ class TestCuratedDetail: r = web_client.delete( "/api/marketplace/curated/mkt-x/alpha/install", cookies=cookies, ) - assert r.status_code == 200 + assert r.status_code == 204 conn = get_system_db() try: assert not UserCuratedSubscriptionsRepository(conn).is_subscribed( diff --git a/tests/test_marketplace_plugin_system.py b/tests/test_marketplace_plugin_system.py index d1c766b..08b2688 100644 --- a/tests/test_marketplace_plugin_system.py +++ b/tests/test_marketplace_plugin_system.py @@ -220,8 +220,7 @@ class TestMarkUnmark: r = web_client.delete( "/api/marketplaces/mkt-x/plugins/alpha/system", cookies=cookies, ) - assert r.status_code == 200 - assert r.json()["is_system"] is False + assert r.status_code == 204 from src.db import get_system_db conn = get_system_db() diff --git a/tests/test_memory_dismiss.py b/tests/test_memory_dismiss.py index 0d2b858..94502d8 100644 --- a/tests/test_memory_dismiss.py +++ b/tests/test_memory_dismiss.py @@ -127,7 +127,7 @@ class TestDismissPost: class TestUndismissDelete: def test_delete_undismisses(self, seeded_app): - """DELETE removes the dismissal row; subsequent DELETE is still 200.""" + """DELETE removes the dismissal row; subsequent DELETE is still 204.""" from src.db import get_system_db conn = get_system_db() @@ -139,14 +139,12 @@ class TestUndismissDelete: 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 == 200 - assert r.json() == {"id": "dm_u1", "dismissed": False} + assert r.status_code == 204 - # Idempotent: a second DELETE still succeeds with the same body — - # absence of the row is the success state. + # 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 == 200 - assert r2.json() == {"id": "dm_u1", "dismissed": False} + assert r2.status_code == 204 conn = get_system_db() cnt = conn.execute( diff --git a/tests/test_store_api.py b/tests/test_store_api.py index b276cc2..85922f6 100644 --- a/tests/test_store_api.py +++ b/tests/test_store_api.py @@ -1047,7 +1047,7 @@ class TestInstallCycle: # Owner soft-archives (default DELETE semantics in v35). d = web_client.delete(f"/api/store/entities/{eid}", cookies=owner_cookies) - assert d.status_code == 200 + assert d.status_code == 204 # Detail still reachable for owner — visibility flipped, not deleted. det = web_client.get(f"/api/store/entities/{eid}", cookies=owner_cookies).json() @@ -1094,7 +1094,9 @@ class TestInstallCycle: f"/api/store/entities/{eid}?hard=true", cookies={"access_token": admin_token}, ) - assert d.status_code == 200, d.text + # DELETE returns 204 No Content per the API design rule landed in + # this PR (tests/test_api_design_rules.py rule 2). + assert d.status_code == 204, d.text # GET 404 + install row gone. assert web_client.get( diff --git a/tests/test_store_entity_versions.py b/tests/test_store_entity_versions.py index 3c88e22..ab62a85 100644 --- a/tests/test_store_entity_versions.py +++ b/tests/test_store_entity_versions.py @@ -2710,7 +2710,7 @@ class TestFullLifecycleFromInstaller: r = web_client.delete( f"/api/store/entities/{eid}", cookies=owner_cookies, ) - assert r.status_code == 200, r.text + assert r.status_code == 204, r.text conn = get_system_db() ent_after = StoreEntitiesRepository(conn).get(eid)