feat(api): add centralized exception-to-http mapping mechanism

[abdulrafey1]

Closes: Routes need a centralized, reusable way to map domain exceptions to HTTP responses

What

Add a centralized, reusable mechanism where core and plugins register how each domain exception type renders as an HTTP response, wired onto the app once at startup — so routes can raise HTTP-agnostic domain exceptions instead of repeating try/except → HTTPException at every call site.

Changes

• feat(api): add the exception→HTTP registry in sparkth/core/exceptions/handlers.py (EXCEPTION_HANDLERS + register_exception_handler(exc_type, status_code), rendering {"detail": str(exc)}) with a public facade at sparkth/lib/exceptions/handlers.py
• refactor(core): the registry reuses the existing KeyedItemHook primitive rather than adding a new type-keyed hook (per review); it stores (exc_type, handler) entries keyed on exc_type (duplicate → ValueError), and adds KeyedItemHook.iter_values() so assemble_app can iterate them
• feat(api): wire the registry into assemble_app() via _register_exception_handlers in sparkth/main.py, inheriting Starlette's MRO-correct dispatch through add_exception_handler
• refactor: git mv sparkth/lib/exceptions.py → sparkth/lib/exceptions/__init__.py, preserving the existing HttpError/AuthenticationError/LMSRequestError surface, and flag that legacy status_code-carrying family as pre-standard so it isn't copied as the current pattern
• test(api): cover registration, duplicate rejection, MRO subclass dispatch, the rendered body, the ships-empty invariant, and that assemble_app() performs the wiring
• docs: document the public module in CLAUDE.md and the plugin authoring surface in sparkth/core/plugins/PLUGIN_GUIDE.md

How to Test

1. make mypy — passes (mypy --strict).
2. make lint.backend && make lint.format.backend check=1 — both clean.
3. make test.backend.pytest — full suite passes. New coverage lives in tests/core/exceptions/test_handlers.py and tests/lib/test_hooks.py.
4. Behaviour check: register a mapping (e.g. register_exception_handler(SomeError, 409)), raise SomeError from a route, and confirm the response is 409 with body {"detail": "<message>"}; a handler registered on a base exception also catches its subclasses.

Notes

No migration, no breaking change, no new env var, no dependency bump. The registry ships empty — this PR delivers only the capability. Migrating existing endpoints (e.g. whitelist, role-management routes) to the mechanism is intentionally out of scope and tracked in separate per-module issues. Fully unifying the remaining keyed-registry hook primitives (KeyedClassHook + KeyedItemHook) is tracked in #522.

This description was written with the assistance of an LLM (Claude).

🤖 Generated with Claude Code

[abdulrafey1]

@claude-review

[claude]

Claude finished @abdulrafey1's task in 3m 17s —— View job

---

PR Review: centralized exception-to-http mapping

Reviewed the full diff against origin/main. This is a clean, well-scoped, well-tested addition. Nice work — docs, tests, and the empty-ships invariant are all covered.

• Read the PR diff vs origin/main
• Review TypeKeyedHook primitive
• Review exception handler registry + facade
• Review assemble_app() wiring
• Review tests & docs
• Post review feedback

---

Overview

The mechanism is sound: TypeKeyedHook (flat, type-keyed, duplicate→ValueError) mirrors the existing SingleNamedItemHook; register_exception_handler populates a process-global EXCEPTION_HANDLERS; _register_exception_handlers wires it onto the app in assemble_app(), delegating MRO-correct dispatch to Starlette's add_exception_handler. Correct approach — no reinvention of dispatch. The sparkth/lib/exceptions.py → sparkth/lib/exceptions/__init__.py rename preserves the existing HttpError/AuthenticationError/LMSRequestError surface, and all existing from sparkth.lib.exceptions import ... callers (canvas, openedx, lib/http.py) keep working. ✅

No blocking issues. No security, correctness, or performance concerns. A few minor suggestions below.

Correctness ✅

• MRO subclass dispatch is inherited from Starlette and is explicitly covered by test_handler_on_base_class_catches_subclass_via_mro.
• assemble_app() registers handlers before the middleware stack is built, so wiring takes effect — covered by test_assemble_app_wires_handlers_from_global_registry.
• Registry ships empty (verified: no non-test caller invokes register_exception_handler), guarded by test_global_registry_ships_empty.

Suggestions (non-blocking)

1. Low — Fixed response shape / undocumented escape hatch. The public API only produces {"detail": str(exc)} + status. The PR description mentions status_handler / register_status, but those aren't in the shipped code (see inline comment). A custom body requires EXCEPTION_HANDLERS.add_item(...) directly, which works but is undocumented. Consider documenting it or adding a handler-accepting overload later.

2. Low — Leaking str(exc) to clients. The message is rendered verbatim in the response body. Fine for narrow, user-safe domain exceptions (which the CLAUDE.md guidance correctly steers toward), but worth keeping in mind: don't register broad exception types whose messages could expose internals. No change needed here since the registry ships empty.

3. Nit — main.py imports the registry from sparkth.core.exceptions.handlers. There's now a sparkth.lib.exceptions.handlers facade that re-exports EXCEPTION_HANDLERS. Importing from the facade would match the "import from lib" convention — though the composition root already pierces core.* elsewhere (config, plugins.service, routes.hooks), so this is consistent as-is. Take it or leave it.

4. Doc — PR description drift. The description references status_handler and register_status shorthand that the final diff doesn't include. Worth trimming the description before merge so it matches the shipped API.

Tests & Docs ✅

Coverage is thorough: registration, duplicate rejection, MRO dispatch, rendered body, assemble_app wiring, and the ships-empty invariant, plus dedicated TypeKeyedHook unit tests. CLAUDE.md and PLUGIN_GUIDE.md are both updated with clear guidance. The _restore_registry fixture touches EXCEPTION_HANDLERS._items (private) for snapshot/restore — acceptable for a process-global, just slightly coupled to internals.

Overall: LGTM. Ready to merge once the description drift (#4) is trimmed; the rest are optional follow-ups.
· rafey/exception-to-http-code-mapping

[claude]

Suggestion (Low): The public API only supports the fixed {"detail": str(exc)} shape + a status code. The PR description advertises a status_handler / custom-handler path, but that isn't in the shipped code — so a plugin needing a custom body/headers has to reach into EXCEPTION_HANDLERS.add_item(exc_type, my_handler) directly. That escape hatch works (EXCEPTION_HANDLERS is in __all__) but is undocumented.

Consider either (a) documenting the add_item escape hatch in PLUGIN_GUIDE.md, or (b) adding a thin register_exception_handler overload that accepts a caller-supplied ExceptionHandler. Not blocking — the fixed shape covers the intended domain-exception use case.

[regisb]

reviewed with experimental sparkth-code-reviewer

Review: exception-to-http mapping mechanism

0 must-fix, 3 suggestions. No checklist (M1–M21) violations found. Core mechanism, tests, and docs are consistent with each other and with existing conventions (sparkth/lib façade, hook pattern, TDD coverage, LLM-assistance notices, git mv for the exceptions.py → exceptions/__init__.py rename).

Suggestions

1. sparkth/lib/hooks.py:117-129 (new TypeKeyedHook) — duplicates a shape already generalized once in this file. SingleNamedItemHook (dict + "duplicate key raises ValueError" + iter_items) was itself generalized into KeyedItemHook. TypeKeyedHook re-implements that pattern a third time rather than extending KeyedItemHook to accept an explicit (non-derived) key. Guideline: "Extract duplicated logic into a shared helper" (abstraction.md). Judgment call — KeyedItemHook's key is derived from the stored item via a callback, and the handler closure here doesn't carry its own exc_type, so reuse isn't a drop-in. Worth a comment either justifying the new class or attempting the KeyedItemHook route.

2. sparkth/core/exceptions/handlers.py:26-32 — register_exception_handler's docstring says assemble_app wires the registry onto the app at startup, describing behavior owned by a different module (sparkth/main.py). Guideline: "Scope a docstring to only the class/module it documents" (docs.md). If _register_exception_handlers in main.py is refactored later, this comment can go stale. Consider trimming the assemble_app implementation detail.

3. sparkth/lib/exceptions/__init__.py (renamed, unchanged content) now sits in the same package as the new handlers.py, but encodes the opposite design principle the PR documents: HttpError/AuthenticationError/LMSRequestError carry status_code on the exception, whereas the new CLAUDE.md section states "a domain exception never carries an HTTP status." Two competing exception→HTTP philosophies now live side-by-side with no note reconciling them. Migrating existing call sites is reasonably out of scope, but a one-line comment noting the legacy pattern predates this standard would stop a future contributor copying HttpError-with-status_code as the "current" pattern. Guideline: "Keep a single source of truth" (abstraction.md).

Outside the corpus — PR description drift

The "Changes" section describes sparkth/core/exception_handlers.py with EXCEPTION_HANDLERS, register_exception_handler, status_handler, register_status, and a facade at sparkth/lib/exception_handlers.py. The shipped diff (after d5b095b and a564011) actually uses sparkth/core/exceptions/handlers.py / sparkth/lib/exceptions/handlers.py and only register_exception_handler — status_handler/register_status were dropped. Please update the description to match what's being merged.

[abdulrafey1]

Thanks @regisb — addressed all three suggestions and the description drift.

1. TypeKeyedHook duplicated KeyedItemHook's shape (9bf338f) — took the KeyedItemHook route: dropped TypeKeyedHook entirely and folded the exception registry onto the existing KeyedItemHook. As you noted, the handler closure doesn't carry its own exc_type, so the registry now stores (exc_type, handler) entries keyed on exc_type (key=lambda entry: entry[0]), plus a new KeyedItemHook.iter_values() for assemble_app to iterate. Fully unifying the two remaining keyed-registry hooks (KeyedClassHook + KeyedItemHook) into one caller-passes-key primitive is a larger cross-consumer change (audit + analytics + exceptions), so I tracked it separately in #522 rather than expand this PR.

2. Docstring described another module's behavior (9bf338f) — trimmed the assemble_app wiring detail from register_exception_handler's docstring (and from the module comment), scoping both to what handlers.py owns.

3. Competing exception→HTTP philosophies side-by-side (504514b) — added a module docstring to sparkth/lib/exceptions/__init__.py marking the legacy status_code-carrying HttpError/AuthenticationError/LMSRequestError family as pre-standard, and pointing new code to register_exception_handler instead.

PR description drift — rewrote the description to match the shipped diff: sparkth/core/exceptions/handlers.py / sparkth/lib/exceptions/handlers.py, only register_exception_handler (the status_handler/register_status shorthand was never shipped).

All green: mypy --strict, ruff lint + format, full suite (1420 passed).

This comment was written with the assistance of an LLM (Claude).

[hamza-56]

These review comments were written with the assistance of an LLM (Claude).

[hamza-56]

Nothing validates status_code, so a typo like 20 or 200 registers fine and only shows up later as a confusing runtime response. A quick if not 400 <= status_code <= 599: raise ValueError(...) at registration time would catch that early, and it doubles as documentation that this registry is meant for error statuses only.

[hamza-56]

One thing that worries me here: the handled exception is never logged. Starlette's ExceptionMiddleware doesn't log exceptions that a handler renders, so once a type is mapped, its occurrences leave no trace in the logs. A spike of 409s in production would be hard to debug.

[hamza-56]

Since str(exc) goes to the client verbatim, whatever the service layer puts in the message becomes API-visible the moment the type is registered. Could we call out in the docstring that registered exception types must carry client-safe messages (no internal IDs, paths, or SQL)?

[hamza-56]

The duplicate check guards the registry against itself, but not against FastAPI's built-ins: registering HTTPException or RequestValidationError here would silently override FastAPI's default handlers when assemble_app wires the registry, losing HTTPException.headers support and the standard 422 body shape. Probably worth rejecting starlette.exceptions.HTTPException subclasses in this function.

[zaira-bibi]

Leaving this for @abdulrafey1 to review and consider.

[hamza-56]

The registry stores opaque handler closures, so the status code is not introspectable after registration. That means tests can only assert that a type is registered, never the actual type to status contract (e.g. that WhitelistEntryAlreadyExists maps to 409), the duplicate-registration ValueError cannot report which status it conflicts with, and the {"detail": str(exc)} body shape gets baked in at registration time instead of at the wiring layer.

Have you considered storing data instead? Keep the registry as exception type to status code (plain int) and build the handler once inside _register_exception_handlers in sparkth/main.py. Registration stays one line for callers, the rendering policy lives in exactly one place, and tests can assert registered[SomeError] == 409.

[zaira-bibi]

Leaving this for @abdulrafey1 to review and consider.