Loading…
Reference in a new issue
No description provided.
Delete branch "fix/content-approval-and-json-repair"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Patch release for three bugs found while investigating why a recurring session was offered no title suggestions.
Closes #278. Closes #279.
Commits
fix(backend)fix(frontend)fix(backend)chore(release)The headline
sessions.audio_processing_statuswas carrying two unrelated concerns — where the raw audio file is, and whether the GM has approved the content. The enum comments admitted it:trashedmeant "GM approved",approvedmeant "audio permanently deleted".Under the default retention mode
delete_after_processing,process_audiosets the status straight toapproved, skippingready.approve_audio409s unless the status isready. So on a default install the endpoint was unreachable and four features were silently dead: highlight publication, the Discord summary re-post, journal entries, and title suggestions.Verified against the live deployment before writing any code: no
audio_retention_policyrow, no per-campaign override, six transcribed sessions all atapprovedwithaudio_trashed_atNULL, five with a draft Discord embed posted and never replaced, andnext_session_title_suggestions_generated_atNULL on every campaign since #23 shipped.Content approval now has its own state and its own endpoint. The audio-trash transition survives as a conditional side effect when the audio is still
ready, so retain-mode installs keep today's behaviour exactly.Migration
One migration,
e0f1a2b3c4d5. Additive — two nullable columns. The backfill fromaudio_trashed_at/_by_idis best-effort by design: the retention sweep nulls those columns when a session ages past the grace period, so a retain-mode install with older sessions will be offered approval once more. Approving again is idempotent. The alternative — treatingapprovedas consent — would wrongly mark everydelete_after_processingsession as GM-approved, which is the bug being fixed.Bot contract
BOT_CONTRACT_VERSIONstays 1. Every change is on/api/sessions/*; nothing touches/api/bot/*. Images can be upgraded independently.Two decisions worth reviewing
The
first_timeflag on the new fan-out. Three sites independently enqueued overlapping subsets of the post-summary tasks without knowing about each other, which is why a hand-edited summary rebuilt the storyline and nothing else. They are now one function — but the recap email and lore extraction must not re-fire when a GM fixes a typo, while the storyline, journal, and title tasks are self-guarding or idempotent. That distinction is encoded rather than left to the next caller to rediscover.JSON repair preserves the longest parseable prefix, not the nearest element boundary. Cutting at the nearest comma sounds safer, but on a real captured sample it discarded a whole complete quote. The cost is that repair can leave an object missing the key it was about to receive — structurally indistinguishable from an object legitimately omitting an optional one. So the contract is that callers validate required fields, not shape;
extract_highlightsdoes, and a test proves it drops a repaired half-object.strict=Trueexists for callers where that is not practical.An early version of the repair also mined the first
{out of a well-formed bare[...], silently discarding every later element.test_workbench_rumorcaught it. Valid-but-wrong-shape JSON is now reported as a parse failure so the caller's own coercion fallback runs, as before.Verification
ruff check+ruff format --check: clean, 189 fileseslint src: clean (1 pre-existing warning inCampaignDetail.jsx, untouched)scripts/check_version_sync.py: OK — app 3.11.1, bot contract v1What this does NOT fix
The LLM endpoint still is not honouring its JSON grammar — it returns malformed JSON on roughly two thirds of large prompts. This release recovers from that and makes the rate visible in the logs for the first time. Constrained decoding via
json_schemaor an explicit grammar is the real fix and deserves its own spike, ideally against the telemetry this release starts producing.🤖 Generated with Claude Code
`sessions.audio_processing_status` was carrying two orthogonal concerns: where the raw audio file physically is, and whether the GM has signed off on the transcript/summary. The enum comments admitted it — `trashed` meant "GM approved" and `approved` meant "audio permanently deleted". That conflation disabled four features on any default install. Under the default retention mode (`delete_after_processing`, settings_service.py:82, labelled "recommended" in the Admin UI), `apply_post_processing_audio_retention` sets the session straight to `approved`, skipping `ready` entirely. But `approve_audio` 409s unless the status is `ready` — so it was unreachable, and everything gated behind it was dead: - highlight publication (`approved=True`), leaving the player-facing quote board permanently empty - the `session_summary_approved` bot event, so the draft Discord embed was never replaced with the GM's corrected summary - generate_journal_entry - generate_session_title_suggestions Confirmed against a live deployment: no `audio_retention_policy` row and no per-campaign override, six transcribed sessions all sitting at `approved` with `audio_trashed_at` NULL, five with a draft embed posted and never replaced, and `next_session_title_suggestions_generated_at` NULL on every campaign since the feature shipped. Content approval now has its own state, `content_approved_at` / `content_approved_by_id`, and its own endpoint, POST /sessions/{id}/approve, gated on having a summary rather than on audio state. The audio-trash transition survives as a *conditional* side effect — only when the audio is still `ready` — so installs on a retain-mode keep today's behaviour exactly. `POST /sessions/{id}/audio/approve` stays as a deprecated shim so existing clients keep working. No `/api/bot/*` change, so BOT_CONTRACT_VERSION is untouched. The migration backfills from `audio_trashed_at`/`_by_id`. That is deliberately best-effort: the retention sweep nulls those columns when a session ages past the grace period, so a retain-mode install with older sessions will be offered approval once more. Approving again is idempotent, and the alternative — treating `approved` as consent — would wrongly mark every delete_after_processing session as GM-approved, which is the bug. Also adds the shared fan-out this exposed. Three sites independently enqueued overlapping subsets of the post-summary tasks without knowing about each other (process_audio, session_service.update_session, the canonical-name refresh), which is why a hand-edited summary rebuilt the storyline and nothing else. `summary_events.on_session_summary_available()` replaces all three. The `first_time` flag is load-bearing: the recap email and lore proposals must not re-fire because a GM fixed a typo, while the storyline, journal, and title tasks are self-guarding or idempotent and are safe to re-run. Each enqueue is individually try/excepted so one broker failure can't swallow the rest. Two further defects in the title-suggestion task, both of which would have kept it useless even with the trigger fixed: - it selected context from `status == completed`, but fires while the session is still `confirmed`. update_campaign_storyline hit this exact bug and was fixed with `.in_([completed, confirmed])`; this task never got that fix, so it almost always generated from campaign name and description alone. - on any LLM exception it wrote `[]` plus a fresh `generated_at`, so one transient timeout destroyed a good suggestion set and stamped it as current. It now returns early without writing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Follows the backend split of content approval from audio retention. Every control the GM needs after a recording was gated on `audio_processing_status === "ready"`, a state a default install never passes through. - Approve button: now shown when there is a summary and `content_approved_at` is null, and calls the new POST /sessions/{id}/approve. Relabelled "Approve summary" — it approves content, not audio. Its confirm() copy is now conditional, since the audio-trash sentence is only true when the audio is still on disk. - Transcript Edit: same re-gate. This is why a GM could correct a summary but never a transcript — summary editing was ungated, transcript editing was not. - Feedback tally: same re-gate. - Adds a `✓ Approved <date>` indicator once content is approved. Also renders TitleSuggestions in the GM inline edit form. Suggestions previously appeared only in the new-session form, and were explicitly suppressed there when "make recurring" was checked — so a session materialized from a series by the Beat task (with `title=series.title_template`, often empty) had no titling path at all. The edit form is the only surface those sessions ever reach. The Admin retention warning for delete_after_processing said only that the recording could not be re-processed. It now also tells the admin to correct the transcript before approving, since there is no audio left to re-transcribe from. It deliberately does *not* claim the transcript becomes uneditable — that was true before this change and is not true after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The quote board had produced zero rows since it shipped. The extraction call was succeeding — HTTP 200, no exception — and the parse was silently failing. `generate_structured_text` sends `response_format: {"type":"json_object"}`, but the endpoint does not enforce it: reproduced against the live llama.cpp router with the real transcript, 6 of 9 prod-shaped calls returned `finish_reason=stop` with structurally invalid JSON. On the large (~33k token) highlights prompt the model emits compact single-line JSON corrupt at the tail — a missing `}`, a missing `]}`, or one extra `}`. Compact-style output correlated perfectly with malformation; pretty-printed output always parsed. `_parse_highlights_json` could not recover any of it. Its salvage path was gated behind `if not text.startswith("{")`, so a response that *did* start with `{` — every truncated one — skipped repair entirely and fell straight through to `return {}`. `repair_json_object` in llm_service is now the shared tolerant parser: clean parse, then raw_decode (which alone fixes the extra-brace shape), then closing the open bracket stack, then truncating to the last parseable point and closing. Every candidate is validated with json.loads before being returned, so it can only ever hand back real JSON. Two properties worth stating, because both were arrived at by getting them wrong first: - It preserves the *longest* parseable prefix rather than preferring element boundaries. Cutting at the nearest comma sounds safer but discards a complete trailing element for no benefit — on a real captured sample it threw away a whole quote. The cost is that repair can leave an object missing the key it was about to receive, which is structurally indistinguishable from an object legitimately omitting an optional one. So the contract is that **callers validate required fields, not shape** — extract_highlights does, and there is a test proving it drops a repaired half-object. `strict=True` is available where that is not practical. - Well-formed JSON of the wrong shape is NOT repaired. An early version mined the first `{` out of a bare `[...]`, silently discarding every later element and presenting the fragment as the whole response; test_workbench_rumor caught it. A valid non-object now reports failure so the caller's own shape-coercion fallback runs, as before. Both outcomes are logged at WARNING, with response size only — never content, which is table talk. This is the actual reason the bug survived a release: `INFO … extracted 0 highlights` was indistinguishable from "the model found nothing memorable". Also fixes a data-loss bug next to it. The delete-then-reinsert cleared existing highlights before checking whether extraction returned anything, and extract_highlights returns [] for both failure and genuine emptiness — so every retry of a failing session destroyed a good set from an earlier successful run. Note what this does not do: the endpoint still is not honouring its JSON grammar. This recovers from that and makes the rate visible in the logs. Constrained decoding via json_schema or an explicit grammar remains the real fix, and is worth a spike once there is real telemetry to measure it against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>