[Backend] GET /api/sessions/{id} silently drops five declared response fields #417

New issue

Closed

opened 2026-08-25 21:46:48 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-08-25 21:46:48 +00:00

Collaborator

Severity: HIGH. Found while implementing #328, which needed a new field on the same response builder.

The defect

GET /api/sessions/{id} declares five fields on SessionResponse that its only builder never populates. They always come back as their schema default (None), regardless of what is in the database.

Missing: lore_generation_status, lore_generation_error, content_approved_at, content_approved_by_id, erasure_notes.

Evidence

webapp/backend/app/services/session_service.py — build_session_response constructs SessionResponse(...) field by field and simply does not pass those five.
It is the only builder: webapp/backend/app/routers/sessions.py:143 (GET) and :383 (PATCH) are its sole callers.
SessionListItem does not carry them either, so there is no alternate path.
Verified empirically against the test suite: with lore_generation_status = "ready" written to the row and flushed, the endpoint returns None for it, and None for content_approved_at and erasure_notes. A field added to the builder in the same run returned correctly, so the fixture and flush are sound.

What this actually breaks

The lore-generation status UI cannot render. webapp/frontend/src/pages/SessionDetail.jsx:222 gates the whole block on session.lore_generation_status, so the four-stage progress labelling, the failure message with its re-run button, and the Review proposals → link are all dead from this endpoint. The August 2026 UX audit called this "the best async status in the product" — that assessment was inferred from JSX and is wrong.
Content-approval state is invisible on the session page.
Per-member erasure notices (#118) never render, so a GM has no in-app trace that a player's lines were scrubbed.

Why it matters

This is a sibling of #303 and it sharpens that issue's systemic point: the response builder is kept in sync by hand, and nothing anywhere catches a field that is declared on the schema but never populated. #303 was found because a write silently did nothing; this one was found by accident. Neither was found by a test.

Proposed fix

Populate the five fields. Then add a structural guard so the class cannot recur silently: a test that reflects over SessionResponse.model_fields and asserts every field is either populated by build_session_response or explicitly listed as intentionally computed elsewhere. That converts "remember to update the builder" from a convention into a check.

While in there, consider whether the builder should be model_validate(session) with explicit overrides for the gated fields (transcript is GM-only) rather than a full manual construction — the manual list is the thing that keeps drifting.

Acceptance criteria

All five fields are returned by GET /api/sessions/{id} and PATCH /api/sessions/{id}
The lore-generation status block renders end to end on SessionDetail
Erasure notices render for a session with erasure_notes
A reflection-based test fails if a new SessionResponse field is added without being populated
Frontend behaviour verified against a running stack, not inferred

**Severity: HIGH.** Found while implementing #328, which needed a new field on the same response builder. ## The defect `GET /api/sessions/{id}` declares five fields on `SessionResponse` that its only builder never populates. They always come back as their schema default (`None`), regardless of what is in the database. Missing: `lore_generation_status`, `lore_generation_error`, `content_approved_at`, `content_approved_by_id`, `erasure_notes`. ## Evidence - `webapp/backend/app/services/session_service.py` — `build_session_response` constructs `SessionResponse(...)` field by field and simply does not pass those five. - It is the only builder: `webapp/backend/app/routers/sessions.py:143` (GET) and `:383` (PATCH) are its sole callers. - `SessionListItem` does not carry them either, so there is no alternate path. - Verified empirically against the test suite: with `lore_generation_status = "ready"` written to the row and flushed, the endpoint returns `None` for it, and `None` for `content_approved_at` and `erasure_notes`. A field added to the builder in the same run returned correctly, so the fixture and flush are sound. ## What this actually breaks - **The lore-generation status UI cannot render.** `webapp/frontend/src/pages/SessionDetail.jsx:222` gates the whole block on `session.lore_generation_status`, so the four-stage progress labelling, the failure message with its re-run button, and the `Review proposals →` link are all dead from this endpoint. The August 2026 UX audit called this "the best async status in the product" — that assessment was inferred from JSX and is wrong. - **Content-approval state is invisible** on the session page. - **Per-member erasure notices (#118) never render**, so a GM has no in-app trace that a player's lines were scrubbed. ## Why it matters This is a sibling of #303 and it sharpens that issue's systemic point: the response builder is kept in sync by hand, and nothing anywhere catches a field that is declared on the schema but never populated. #303 was found because a write silently did nothing; this one was found by accident. Neither was found by a test. ## Proposed fix Populate the five fields. Then add a structural guard so the class cannot recur silently: a test that reflects over `SessionResponse.model_fields` and asserts every field is either populated by `build_session_response` or explicitly listed as intentionally computed elsewhere. That converts "remember to update the builder" from a convention into a check. While in there, consider whether the builder should be `model_validate(session)` with explicit overrides for the gated fields (`transcript` is GM-only) rather than a full manual construction — the manual list is the thing that keeps drifting. ## Acceptance criteria - [ ] All five fields are returned by `GET /api/sessions/{id}` and `PATCH /api/sessions/{id}` - [ ] The lore-generation status block renders end to end on SessionDetail - [ ] Erasure notices render for a session with `erasure_notes` - [ ] A reflection-based test fails if a new `SessionResponse` field is added without being populated - [ ] Frontend behaviour verified against a running stack, not inferred

claude-bot added this to the v4.1.0 - Data Durability & Recovery milestone

2026-08-25 21:46:48 +00:00

claude-bot added the

bug

backend

labels

2026-08-25 21:46:48 +00:00

claude-bot modified the milestone from v4.1.0 - Data Durability & Recovery to v3.11.5 - Recording Clock Hotfix

2026-08-25 22:13:56 +00:00

claude-bot commented

2026-08-25 22:13:56 +00:00

Author

Collaborator

Fixed in the v3.11.5 hotfix

Pulled into the hotfix rather than deferred. The deciding argument: #328 added a sixth field to this same builder, and the only reason that field works is that the drift was noticed by chance. Shipping a fix that adds one field while knowingly leaving the others broken — without the guard that would have caught its own near-miss — is not a fix.

Second reason: after the first correctly-recorded session, lore_generation_status gates the entire lore block, including the only link the product offers to the proposals queue. Invisible status means the payoff step of the pipeline has no visible entry point, so "the next session records correctly" would still not be true end to end.

It was seven fields, not five

The source-level guard found two more that reading the code by hand had missed:

lore_generation_status, lore_generation_error, content_approved_at, content_approved_by_id, erasure_notes, series_id, series_occurrence_date

The guard

tests/test_session_response_completeness.py asserts every SessionResponse field that maps to a Session attribute is actually passed by the builder. It is deliberately a source-level check rather than a value comparison, because a value test only catches fields a test author remembered to populate — precisely the failure mode that allowed this. It failed on first run and named the two extra fields.

Two things handled while in there

erasure_notes no longer carry member_user_id. The note text is deliberately anonymous ("Contributions from a former member were removed on …") and #118 shows it to every member, but the stored dict also holds the erased member's id — which would undo that anonymity for anyone reading the API. Nothing renders it. Covered by a test asserting it from a player's view, since that is who the anonymity protects.

The "Review proposals →" link destination. It pointed at the wiki home rather than /wiki/proposals. Populating the status makes that link visible for the first time, and shipping a newly-visible link to the wrong page would just be a new defect. Taken from #375; the rest of that issue's scope (the triple-surfaced proposal UI and the mobile-unreachable entry point) is untouched.

Backend suite: 914 passed. Frontend: 431 passed, eslint clean.

## Fixed in the v3.11.5 hotfix Pulled into the hotfix rather than deferred. The deciding argument: #328 added a *sixth* field to this same builder, and the only reason that field works is that the drift was noticed by chance. Shipping a fix that adds one field while knowingly leaving the others broken — without the guard that would have caught its own near-miss — is not a fix. Second reason: after the first correctly-recorded session, `lore_generation_status` gates the entire lore block, including the only link the product offers to the proposals queue. Invisible status means the payoff step of the pipeline has no visible entry point, so "the next session records correctly" would still not be true end to end. ### It was seven fields, not five The source-level guard found two more that reading the code by hand had missed: `lore_generation_status`, `lore_generation_error`, `content_approved_at`, `content_approved_by_id`, `erasure_notes`, **`series_id`**, **`series_occurrence_date`** ### The guard `tests/test_session_response_completeness.py` asserts every `SessionResponse` field that maps to a `Session` attribute is actually passed by the builder. It is deliberately a source-level check rather than a value comparison, because a value test only catches fields a test author remembered to populate — precisely the failure mode that allowed this. It failed on first run and named the two extra fields. ### Two things handled while in there **`erasure_notes` no longer carry `member_user_id`.** The note text is deliberately anonymous ("Contributions from a former member were removed on …") and #118 shows it to every member, but the stored dict also holds the erased member's id — which would undo that anonymity for anyone reading the API. Nothing renders it. Covered by a test asserting it from a *player's* view, since that is who the anonymity protects. **The "Review proposals →" link destination.** It pointed at the wiki home rather than `/wiki/proposals`. Populating the status makes that link visible for the first time, and shipping a newly-visible link to the wrong page would just be a new defect. Taken from #375; the rest of that issue's scope (the triple-surfaced proposal UI and the mobile-unreachable entry point) is untouched. Backend suite: 914 passed. Frontend: 431 passed, eslint clean.

claude-bot closed this issue

2026-08-25 22:13:57 +00:00

claude-bot referenced this issue

2026-08-25 22:13:58 +00:00

[Frontend] Fix lore-proposal discovery: wrong link destination, triple-surfaced UI, and a proposals page reachable by one hidden link #375