feat(server): uniform error handling — domain errors to HTTP with stable codes (#31) - #96
Merged
Merged
Conversation
…ble codes (#31) Every failure the API can produce — a kernel domain error, a framework HTTPException, a request-validation failure, or an unhandled bug — is now rendered by one function into one ErrorBody {code, message, detail?}, declared in openapi.json's components and applied to every route. Clients branch on `code`, never on the status. DestructiveSchemaChange and SchemaChangeWouldOrphan are both 409, and only the first is retryable with a flag — a client branching on the status alone would loop against the second, which is the loop that error's own docstring warns about. Also introduces create_app(), keeping visionset.server.main:app importable for scripts/export_openapi.py, and gives #80's WorkspaceBusy its first caller.
2 tasks
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 21, 2026
…ble codes (#31) (#96) Every failure the API can produce — a kernel domain error, a framework HTTPException, a request-validation failure, or an unhandled bug — is now rendered by one function into one ErrorBody {code, message, detail?}, declared in openapi.json's components and applied to every route. Clients branch on `code`, never on the status. DestructiveSchemaChange and SchemaChangeWouldOrphan are both 409, and only the first is retryable with a flag — a client branching on the status alone would loop against the second, which is the loop that error's own docstring warns about. Also introduces create_app(), keeping visionset.server.main:app importable for scripts/export_openapi.py, and gives #80's WorkspaceBusy its first caller.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #31. M3 task 2 of 14.
Every failure the API can produce — a kernel domain error, a framework
HTTPException, a request-validation failure, or an unhandled bug — is rendered by one function into oneErrorBody{code, message, detail?}, declared inopenapi.json'scomponentsand applied to every route.Branch on
code, not on the statusDESTRUCTIVE_SCHEMA_CHANGEandSCHEMA_CHANGE_WOULD_ORPHANare both 409. The first is retryable withallow_destructive=true; the second has no override at all. A client that saw 409 and retried with the flag would loop forever against the second — which is exactly the loopSchemaChangeWouldOrphan's docstring warns about. Over HTTP thecodeis the only thing carrying that distinction.Three rules place a domain error, not fifty
Plus 503 for the one transient error and 500 for what a caller cannot fix.
The calls worth reviewing
Each is commented in
ERROR_RULESwith the docstring sentence that decides it.ConstraintViolated→ 500, opaque. Its message isstr(exc.orig)— raw SQLite text naming our own tables. A 409 would publish the schema to every client. Services pre-check rather than relying on the write to fail, and the two that translate a constraint each own exactly one index, so anything reaching the boundary is a guard nobody wrote.NoSplitRecipe→ 404, not 409. A release is immutable, so its state will never change and "resolve the conflict and resubmit" is a promise that cannot be kept.NO_SPLIT_RECIPEvsRELEASE_NOT_FOUNDis the case codes exist for.UnserializableManifest→ 409, not 422. The request body is valid; the defect is in state stored long before, and the remedy is to fix the annotation and publish again.MediaToolUnavailable→ 500, not 503. 503 promises transience, and no amount of retrying installs ffmpeg. Its message is exposed, because the install hint is the whole reason the message exists.UnsupportedMedia→ 422, not 415. Every raise site reads a file on disk during ingest; 415 is about the request's ownContent-Type.WorkspaceBusy→ 503 withRetry-After: 5, matched to the store's busy timeout. This is kernel: SQLite concurrency posture — WAL, busy_timeout, OperationalError → domain error (closes the "database is locked" gap before background ingest) #80's first caller. A shorter hint would aim a retry storm at the contention being reported.Opacity: 5xx by default, three named opt-outs
A 5xx body carries an
incident_idand a generic sentence; the real message and traceback go to the log under that id.WORKSPACE_BUSY,WORKSPACE_FORMAT_TOO_NEWandMEDIA_TOOL_UNAVAILABLEopt out because their message is the remedy. A mapped 5xx keeps its own code; an unmapped exception getsINTERNAL_ERROR, so the two are told apart in a log without reading the message.A test caught a real hole here mid-implementation:
MediaError.__str__isf"{name}: {reason}", so keeping the file's path out ofdetailwhilemessagecarried it hid nothing. The API's message for a media error is nowexc.reasonalone — the kernel already separated the two ("reason never repeats the name"), and this takes the half that is safe to publish.Two traps
starlette.exceptions.HTTPException, notfastapi's. The router raises the Starlette class for an unknown path and for a 405, and FastAPI's is a subclass — keying on the subclass would leave those two answering{"detail": "Not Found"}while everything else answeredErrorBody. There is a test for it.(Request, Exception)and narrow inside. Starlette'sExceptionHandlertype takesException; a narrower annotation is contravariance-incompatible and failsmypy src/visionset, which CI runs. No# type: ignore.The app factory
create_app()now exists, andapp = create_app()stays module-level becausescripts/export_openapi.pyimports it by name./healthmoves onto anAPIRouter— the pattern #27 extends.responses=is applied at app level rather than per route. Declaring 422 there displaces FastAPI's generatedHTTPValidationError, keeping that model — and the second error shape it implies — out ofopenapi.jsonentirely. That is what makes "one error body, used everywhere" true by construction instead of by every future route remembering; a test asserts it never comes back.Tests
tests/server/test_errors.py, 28 tests. Two structural ones carry the acceptance criterion and cannot drift:vars(kernel.errors)(not__subclasses__(), which only sees imported classes and would depend on collection order) and asserts exact equality withERROR_RULES. A new kernel error fails the suite until somebody maps it deliberately.{ClassName: (status, code)}of all 49, so a status change is a readable diff.Codes are literals rather than derived from class names: a code is a public contract keyed to a Python identifier, and deriving it means a pure refactor rename silently breaks every client while passing every test. A test asserts each literal still matches its class name today, with a
RENAMEDmap for when one genuinely changes.What did NOT change
FORMAT_VERSIONstays 10 — no migration (M3's only one belongs to #25; a second appearing here would be the signal that logic is leaking upward).VERSIONstays0.0.1.dev0. No new dependency, no new kernel error, no new service, port or event._auth_provider,require_tokenandEnvTokenAuthProviderare untouched — #25 owns them; the existing 401 simply starts speakingErrorBody, challenge header intact.919 tests, up from 896.
Checks
Verified by hand against a running server:
/healthunchanged, an unknown path returns{"code":"NOT_FOUND",...}, andcomponents.schemasis exactly["ErrorBody"].Next: #25 — the real
AuthProvider: a persisted adapter inkernel/adapters/, issuance as aTokenService, and M3's only migration.