diff --git a/devplacepy/routers/CLAUDE.md b/devplacepy/routers/CLAUDE.md index 88fda68a..ef252f1e 100644 --- a/devplacepy/routers/CLAUDE.md +++ b/devplacepy/routers/CLAUDE.md @@ -60,6 +60,7 @@ Every page/redirect endpoint also returns JSON when the client asks. Core in `de - **Page GETs:** `return respond(request, "x.html", context, model=XOut)` - HTML renders the template; JSON does `XOut.model_validate(context).model_dump()`. One context, two renderings. - **Action POSTs:** `return action_result(request, url, data=)` - HTML 302 redirects; JSON returns `{ok, redirect, data}`. (Set cookies on the returned response after calling it, as `auth.py` login/signup do.) +- **Refusals:** `return json_error(status_code, message)` - **status first, message second**. Swapping them is not a lint-level mistake, it is a guaranteed 500: `JSONResponse(..., status_code="some message")` raises `TypeError` inside `Response.init_headers` before any byte is written, so the branch that was supposed to explain a limit to the user crashes instead. It was a real production bug - every refusal in the two workspace routers was written `json_error(message, status)`, so a member already at the default 2-workspace quota got a 500 from the workspace page's **Open workspace** button rather than the quota message. `tests/unit/responses.py` now AST-scans the whole package for both argument orders, so the swap cannot come back anywhere. Response models live in `devplacepy/schemas.py` (Pydantic v2, `extra="ignore"`, all-Optional so they validate the existing context dicts directly). **Always project users through `UserOut`** (and `AdminUserOut`) - the raw user rows contain `email`/`api_key`/`password_hash`, and the models drop them; never serialize a raw user row. List item shapes vary (feed/gists/news/admin-news are wrapped `{post|gist|article: ...}`; projects are flat rows with `author_name`/`my_vote`) - match the context exactly. Errors negotiate centrally: `main.py` 404/500/validation handlers and the rate-limit/maintenance middleware, plus `utils.require_user`/`require_admin` (401/403 for JSON, 303 redirect for browsers). The four legacy AJAX endpoints (votes/reactions/bookmarks/polls) keep their original flat JSON shapes and are left untouched. Documented in `docs_api.py`'s Conventions group. diff --git a/devplacepy/routers/admin/workspaces.py b/devplacepy/routers/admin/workspaces.py index a6ea3cc5..40acc290 100644 --- a/devplacepy/routers/admin/workspaces.py +++ b/devplacepy/routers/admin/workspaces.py @@ -117,7 +117,7 @@ async def admin_workspace_suspend( instance = _instance_or_404(uid) reason = (data.reason or "").strip() if not reason: - return json_error("a reason is required and is shown to the owner", 400) + return json_error(400, "a reason is required and is shown to the owner") provision.suspend(instance, admin["uid"], reason) _audit(request, admin, "container.workspace.suspend", instance, metadata={"reason": reason}) owner = instance.get("workspace_owner_uid", "") @@ -246,7 +246,7 @@ async def admin_workspace_quota( if not isinstance(admin, dict): return admin if not data.owner_id: - return json_error("owner_id is required", 400) + return json_error(400, "owner_id is required") table = get_table(quota.RULES_TABLE) existing = table.find_one( owner_kind="user", owner_id=data.owner_id, deleted_at=None diff --git a/devplacepy/routers/projects/containers/workspace.py b/devplacepy/routers/projects/containers/workspace.py index 78502bd6..fcbd61a7 100644 --- a/devplacepy/routers/projects/containers/workspace.py +++ b/devplacepy/routers/projects/containers/workspace.py @@ -15,11 +15,12 @@ from devplacepy.responses import action_result, json_error, respond from devplacepy.schemas import WorkspaceOut from devplacepy.services.audit import record as audit from devplacepy.services.containers import activity, api, forward, store +from devplacepy.services.containers.api import ContainerError from devplacepy.services.containers.workspace import provision, quota, tunnels from devplacepy.services.containers.workspace.provision import WorkspaceError from devplacepy.utils import not_found, require_user -from ._shared import audit_instance +from ._shared import audit_instance, fail router = APIRouter() @@ -103,7 +104,9 @@ async def workspace_open(request: Request, slug: str): summary=str(error), result="denied", ) - return json_error(str(error), 400) + return json_error(400, str(error)) + except ContainerError as error: + return fail(error) audit_instance( request, user, @@ -126,7 +129,7 @@ async def workspace_stop(request: Request, slug: str): project = _project_or_404(slug) instance = _workspace_or_404(project, user) if not can_manage_workspace(instance, project, user): - return json_error("Not allowed to manage this workspace", 403) + return json_error(403, "Not allowed to manage this workspace") provision.stop(instance) audit_instance(request, user, "container.workspace.stop", instance, project) return action_result(request, f"/projects/{slug}/workspace") @@ -140,7 +143,7 @@ async def workspace_delete(request: Request, slug: str): project = _project_or_404(slug) instance = _workspace_or_404(project, user) if not can_manage_workspace(instance, project, user): - return json_error("Not allowed to manage this workspace", 403) + return json_error(403, "Not allowed to manage this workspace") for row in tunnels.list_for_instance(instance["uid"]): tunnels.soft_delete(row["uid"], user["uid"]) store.delete_instance(instance["uid"], user["uid"]) @@ -156,7 +159,7 @@ async def tunnel_list(request: Request, slug: str): project = _project_or_404(slug) instance = _workspace_or_404(project, user) if not can_manage_workspace(instance, project, user): - return json_error("Not allowed to manage this workspace", 403) + return json_error(403, "Not allowed to manage this workspace") return {"tunnels": tunnels.list_for_instance(instance["uid"])} @@ -170,17 +173,17 @@ async def tunnel_create( project = _project_or_404(slug) instance = _workspace_or_404(project, user) if not can_manage_workspace(instance, project, user): - return json_error("Not allowed to manage this workspace", 403) + return json_error(403, "Not allowed to manage this workspace") if data.container_port <= 0: - return json_error("container_port must be between 1 and 65535", 400) + return json_error(400, "container_port must be between 1 and 65535") limits = quota.resolve(instance.get("workspace_owner_uid", ""), instance) if limits.max_tunnels and tunnels.count_for_instance( instance["uid"] ) >= limits.max_tunnels: - return json_error(f"tunnel limit reached ({limits.max_tunnels})", 400) + return json_error(400, f"tunnel limit reached ({limits.max_tunnels})") row = tunnels.create(instance, data.label, data.container_port, user["uid"]) if not row: - return json_error("could not create tunnel", 400) + return json_error(400, "could not create tunnel") audit_instance( request, user, @@ -202,7 +205,7 @@ async def tunnel_delete(request: Request, slug: str, uid: str): project = _project_or_404(slug) instance = _workspace_or_404(project, user) if not can_manage_workspace(instance, project, user): - return json_error("Not allowed to manage this workspace", 403) + return json_error(403, "Not allowed to manage this workspace") row = tunnels.get(uid) if not row or row.get("instance_uid") != instance["uid"]: raise not_found("Tunnel not found") @@ -228,7 +231,7 @@ def _editor_guard(request: Request, slug: str, uid: str): if not instance or not instance.get("is_workspace"): raise not_found("Workspace not found") if not can_manage_workspace(instance, project, user): - return None, None, json_error("Not allowed to open this workspace", 403) + return None, None, json_error(403, "Not allowed to open this workspace") return project, instance, None diff --git a/tests/unit/responses.py b/tests/unit/responses.py new file mode 100644 index 00000000..2c0a2641 --- /dev/null +++ b/tests/unit/responses.py @@ -0,0 +1,50 @@ +# retoor + +import ast +from pathlib import Path + +PACKAGE = Path(__file__).resolve().parents[2] / "devplacepy" + + +def _json_error_calls(): + for path in sorted(PACKAGE.rglob("*.py")): + tree = ast.parse(path.read_text(), filename=str(path)) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + name = node.func.attr if isinstance(node.func, ast.Attribute) else getattr(node.func, "id", "") + if name == "json_error" and node.args: + yield path.relative_to(PACKAGE.parent), node + + +def test_json_error_never_receives_a_message_as_its_status(): + offenders = [ + f"{path}:{node.lineno}" + for path, node in _json_error_calls() + if isinstance(node.args[0], ast.Constant) and isinstance(node.args[0].value, str) + ] + assert not offenders, ( + "json_error(status_code, message) - a string in the status position builds a " + f"JSONResponse with a non-int status and raises TypeError at request time: {offenders}" + ) + + +def test_json_error_never_receives_a_status_as_its_message(): + offenders = [ + f"{path}:{node.lineno}" + for path, node in _json_error_calls() + if len(node.args) > 1 + and isinstance(node.args[1], ast.Constant) + and isinstance(node.args[1].value, int) + ] + assert not offenders, ( + "json_error(status_code, message) - an int in the message position means the " + f"arguments were swapped: {offenders}" + ) + + +def test_every_json_error_status_is_a_real_http_error_code(): + for path, node in _json_error_calls(): + first = node.args[0] + if isinstance(first, ast.Constant) and isinstance(first.value, int): + assert 400 <= first.value <= 599, f"{path}:{node.lineno} status {first.value}"