Adopt FastAPI native error detail shape end to end

claude-bot commented

2026-07-28 06:00:23 +00:00

Severity: HIGH

The bug

The documented {error: {code, message, details}} envelope never reaches the client.

FastAPI serializes HTTPException as {"detail": <detail>}. The 409 handler
(backend/app/api/routes/photos.py:75-86) raises detail={"error": {...}}, so the wire format
is {"detail": {"error": {...}}}. Every other raise uses a bare string, giving
{"detail": "Photo not found"}. Meanwhile frontend/src/api/client.ts:32-46 parses
{"error": ...} exclusively.

Result: every error code and message is lost. Errors surface as code "unknown", message
"HTTP <status>". And the 409 conflict banner — the flagship concurrency UX — renders
"Current version is -1", because details.current_version never arrives.

Decision

Adopt FastAPI's native detail shape rather than keeping the custom envelope. Structured
detail is still available where needed — detail may be an object:

raise HTTPException(status_code=409, detail={
    "code": "review_version_conflict",
    "message": "This photo was modified by someone else.",
    "current_version": photo.review_version,
})

Scope

Standardize on {"detail": ...} — a string for simple errors, an object with code and any
extra fields for errors the client must act on.
Update client.ts to parse detail, handling both the string and object forms.
Add a RequestValidationError handler so 422s follow the same shape.
Update docs/circa-api-spec.md, which currently documents the {error} envelope.
Fix the conflict banner to read current_version from the new shape.

Note the success envelope {data, meta} is unaffected and stays as it is.

Why this matters beyond the banner

#8's API contract tests are meaningless until the wire format is settled, and this shape is what
the generated TypeScript types (companion issue) will encode.

Done when

All errors use FastAPI's native detail, with structured content where the client needs it
The frontend parses it, and error messages are visible to the user
The 409 banner shows the real current version
422 validation errors follow the same shape
docs/circa-api-spec.md matches reality

References

backend/app/api/routes/photos.py:75-86
frontend/src/api/client.ts:32-46
frontend/src/pages/ReviewWorkspacePage.tsx:478-492
docs/circa-api-spec.md

## Severity: HIGH ## The bug The documented `{error: {code, message, details}}` envelope never reaches the client. FastAPI serializes `HTTPException` as `{"detail": <detail>}`. The 409 handler (`backend/app/api/routes/photos.py:75-86`) raises `detail={"error": {...}}`, so the wire format is `{"detail": {"error": {...}}}`. Every other raise uses a bare string, giving `{"detail": "Photo not found"}`. Meanwhile `frontend/src/api/client.ts:32-46` parses `{"error": ...}` exclusively. Result: every error code and message is lost. Errors surface as code `"unknown"`, message `"HTTP <status>"`. And the 409 conflict banner — the flagship concurrency UX — renders **"Current version is -1"**, because `details.current_version` never arrives. ## Decision **Adopt FastAPI's native `detail` shape** rather than keeping the custom envelope. Structured detail is still available where needed — `detail` may be an object: ```python raise HTTPException(status_code=409, detail={ "code": "review_version_conflict", "message": "This photo was modified by someone else.", "current_version": photo.review_version, }) ``` ## Scope - Standardize on `{"detail": ...}` — a string for simple errors, an object with `code` and any extra fields for errors the client must act on. - Update `client.ts` to parse `detail`, handling both the string and object forms. - Add a `RequestValidationError` handler so 422s follow the same shape. - Update `docs/circa-api-spec.md`, which currently documents the `{error}` envelope. - Fix the conflict banner to read `current_version` from the new shape. Note the **success** envelope `{data, meta}` is unaffected and stays as it is. ## Why this matters beyond the banner #8's API contract tests are meaningless until the wire format is settled, and this shape is what the generated TypeScript types (companion issue) will encode. ## Done when - [ ] All errors use FastAPI's native `detail`, with structured content where the client needs it - [ ] The frontend parses it, and error messages are visible to the user - [ ] The 409 banner shows the real current version - [ ] 422 validation errors follow the same shape - [ ] `docs/circa-api-spec.md` matches reality ## References - `backend/app/api/routes/photos.py:75-86` - `frontend/src/api/client.ts:32-46` - `frontend/src/pages/ReviewWorkspacePage.tsx:478-492` - `docs/circa-api-spec.md`

claude-bot added this to the v0.2.0 milestone

2026-07-28 06:00:23 +00:00

claude-bot added the

bug

backend

frontend

labels

2026-07-28 06:00:23 +00:00

claude-bot referenced this issue

2026-07-28 06:07:54 +00:00

Add API contract and authorization tests for the Phase 1 surface #8

rbrooks referenced this issue from a commit

2026-08-01 05:31:06 +00:00

Make the review_version guard actually prevent overwrites

claude-bot referenced this issue

2026-08-01 05:52:37 +00:00

review_version guard is check-then-write and races #77

rbrooks referenced this issue from a commit

2026-08-01 07:06:45 +00:00

Settle on one error shape and make it reach the client

rbrooks closed this issue

2026-08-01 07:06:45 +00:00

claude-bot commented

2026-08-01 07:07:03 +00:00

Author

Done in 2961bc5. All five "done when" items covered — 16 backend tests in test_error_shape.py, 6 frontend tests in client.test.ts.

Took the decision as written: FastAPI's native detail, string form for display-only errors, object form with code for anything the client branches on. Extra fields sit beside code rather than under a nested details — one less level to walk, and the nesting is exactly what got lost.

Two things worth recording beyond the issue:

current_version can be null, and that forced a real fix. When the database-level guard from #77 rejects a write (rather than the pre-check), the server no longer holds a trustworthy read of the row — inventing a number would be worse than admitting we lack one. But the banner used conflictVersion !== null as its visibility condition, so passing null would have hidden the banner entirely. Conflict state is now {version: number | null} | null: the outer null means "no conflict", the inner means "we can't name it" and renders "Someone else saved first." Collapsing those two is how "-1" came to be rendered as a version in the first place, so it seemed worth not repeating the shape of the original mistake.

422s no longer echo the caller's payload. FastAPI's default includes input in each error, which for a rejected login or an oversized note means putting the submitted value back on the wire. Only loc and msg cross now, with a test asserting a submitted secret doesn't come back.

I dropped the latest_projection snapshot the spec described in the conflict body. The client reloads the photo anyway, and embedding a second representation of a photo in an error is two things that can disagree. Documented that reasoning in the spec rather than silently omitting it.

docs/circa-api-spec.md was wrong in two places and now matches reality, with a note saying why it changed.

This unblocks #8 (contract tests have a settled format to assert against) and #76 (the generated types encode this shape).

Done in 2961bc5. All five "done when" items covered — 16 backend tests in `test_error_shape.py`, 6 frontend tests in `client.test.ts`. Took the decision as written: FastAPI's native `detail`, string form for display-only errors, object form with `code` for anything the client branches on. **Extra fields sit beside `code` rather than under a nested `details`** — one less level to walk, and the nesting is exactly what got lost. Two things worth recording beyond the issue: **`current_version` can be `null`, and that forced a real fix.** When the database-level guard from #77 rejects a write (rather than the pre-check), the server no longer holds a trustworthy read of the row — inventing a number would be worse than admitting we lack one. But the banner used `conflictVersion !== null` as its *visibility* condition, so passing null would have hidden the banner entirely. Conflict state is now `{version: number | null} | null`: the outer null means "no conflict", the inner means "we can't name it" and renders "Someone else saved first." **Collapsing those two is how "-1" came to be rendered as a version in the first place**, so it seemed worth not repeating the shape of the original mistake. **422s no longer echo the caller's payload.** FastAPI's default includes `input` in each error, which for a rejected login or an oversized note means putting the submitted value back on the wire. Only `loc` and `msg` cross now, with a test asserting a submitted secret doesn't come back. I dropped the `latest_projection` snapshot the spec described in the conflict body. The client reloads the photo anyway, and embedding a second representation of a photo in an error is two things that can disagree. Documented that reasoning in the spec rather than silently omitting it. `docs/circa-api-spec.md` was wrong in two places and now matches reality, with a note saying why it changed. This unblocks #8 (contract tests have a settled format to assert against) and #76 (the generated types encode this shape).

claude-bot referenced this issue

2026-08-02 06:52:33 +00:00

Add API contract and authorization tests for the Phase 1 surface #8

claude-bot referenced this issue

2026-08-02 07:17:21 +00:00

Add optimistic-concurrency (409 review_version_conflict) tests #9

claude-bot referenced this issue

2026-08-02 07:34:01 +00:00

Set up frontend test tooling and cover the review workspace #11

claude-bot referenced this issue

2026-08-04 05:38:25 +00:00

Conflict state shows -1, sits in the wrong place, and does not block #98