[Recording] Guard against re-submitting audio for a session that already has GM-edited content #397
Closed
opened 2026-08-25 20:44:37 +00:00 by claude-bot
·
3 comments
No Branch/Tag specified
main
feat/1328-fill-it-in
feat/1328-fill-frontend
feat/1328-fill-backend
feat/9-foundry-push
fix/9-foundry-live
feat/9-foundry-push-frontend
renovate/celery-5.x
feat/5-foundry-connect
feat/9-foundry-push-backend
feat/9-foundry-push-module
feat/5-foundry-frontend
feat/5-foundry-backend
feat/5-foundry-module
release/v4.8.0-rc1
docs/d341-convert-draft
feat/958-update-to-version
feat/958-convert-sheet
feat/958-pathbuilder-import-frontend
fix/1360-heading-order
feat/958-pathbuilder-import-backend
feat/11-weekly-digest
feat/1341-auto-update
docs/d332-superseded
chore/1345-names-cleanup
feat/1342-mentioned-pages
feat/1328-recipes
fix/1346-pgdg-key
perf/1247-shared-recording-stream
chore/132-gitleaks-allowlist
release/v4.7.1
spike/foundry-module
release/v4.7.1-rc1
docs/decisions-d332-d337
fix/bot-certifi
chore/release-4.7.0
chore/release-4.7.0-rc4
chore/deps-4.7.0
chore/release-4.7.0-rc3
fix/1280-quote-must-say-it
fix/1257-names-row-squeeze
chore/release-4.7.0-rc2
fix/1245-pg-upgrade-count
chore/release-4.7.0-rc1
feat/1257-names-in-prep
feat/1257-be
feat/1257-fe
feat/1257-rating-fixes
design/1328-fill-it-in
feat/1257-name-splits
fix/1326-redis-503
feat/1280-fact-suggestions
feat/1280-be
feat/1280-fe
design/1280-fact-suggestions
feat/1279-infobox
feat/1279-be
feat/1279-fe
feat/1321-shelf-copies
design/1279-infobox
feat/1243-table-find
feat/1257-who-rated
feat/1243-prep-drawer
feat/1243-sheet-pickers
design/1243-browse-in-place
feat/1315-hotwords-default
feat/1242-system-filters
feat/1257-more-name-styles
feat/506-leg3-hotwords
design/1242-system-filters
fix/1302-packs-polish
feat/1241-rules-library
design/1241-rules-library
feat/1245-postgres-18
integ/4.6.2
fix/1263-map-ui-2
fix/1263-map-server
fix/1263-map-ui-1
fix/1261-editor-link-menu
fix/1229-axe-4.14-name-mismatch
renovate/axllent-mailpit-1.x
renovate/eslint-monorepo
renovate/authlib-1.x
chore/release-4.6.0
chore/release-4.6.0-rc2
fix/460-rc2-screens
fix/460-rc2-server
fix/release-prerelease-exit
chore/release-4.6.0-rc1
feat/125-campaign-templates
feat/126-calendar
feat/1181-pictures-and-files
feat/1190-event-people
integ/4.6.0
feat/1191-line-of-the-night
feat/1200-starter-packs
chore/bot-multidict-and-playwright-group
fix/1202-this-week-answered-count
fix/122-124-leftovers
fix/1178-get-routes-that-write
fix/1203-journeys-check-names
design/125-campaign-templates
feat/126-calendar-engine
design/126-in-world-calendar
fix/1189-cited-stamp-in-range
fix/1189-v4-looser-check
fix/1189-richer-beats
fix/1113-export-bind-lists
fix/1154-release-notes-by-file
chore/release-4.5.0
chore/release-4.5.0-rc12
fix/1135-wiki-markup-as-text
fix/demo-usage-cost-in-window
chore/release-4.5.0-rc11
fix/1121-sheet-consistency
fix/1121-sheet-5e-terms-and-card-hp
copy/1122-busiest-pages
fix/1121-relevant-choices
fix/1121-wiki-create-npc-save-sheet-links
fix/1029-menu-stays-on-screen
docs/prototype-transcript-bin-wording
fix/1023-editor-save-row-back-scroll
fix/1020-search-exact-first
fix/1020-table-prep
fix/1020-shell-wordmark-footer-bell
fix/1020-wiki-article-party
fix/872-admin-kept-until-phone-tables
feat/l13-milestone-level-waiting
fix/1000-1001-bell-older-setup-context
fix/999-pronouns-in-prompts
fix/l19-other-class-feats
fix/1010-transcript-window-ordinary-confirm
docs/d254-d269-decisions
fix/702-member-search
fix/680-transcript-bin
fix/993-unidentified-prompts
fix/841-redis-timeouts
fix/990-binned-not-exported
fix/830-export-prep-shelf
fix/deps-audit-2026-10-01
fix/877-title-reads
fix/798-audio-received-at
build/lane11-17-j7
build/lane11-16-levelup
build/lane11-14-seams
build/lane11-14-seed
build/lane11-15-start
build/lane11-13-old-block-out
fix/980-review-r3
build/lane11-12-block
build/lane11-8-packs
fix/demo-seed-canonical-names
build/lane11-11-hot-half
docs/l11-decisions
build/lane11-7-convert
build/lane11-10-editing
fix/npm-audit-brace-expansion
build/lane11-9-sheet-page
fix/bot-pyjwt
build/lane11-6-content
fix/lane11-rank-names
build/lane11-2b-language
build/lane11-4-js-evaluator
build/lane11-3-server
build/lane11-5a-pf2e
build/lane11-5b-5e
feat/design-admin
build/lane11-2-evaluator
build/lane11-1-sheet-pieces
build/lane11-0-drawing
fix/943-quest-objectives-fk
fix/prepsheet-shelf-flake
fix/945-leftovers
build/demo-gm-copy
fix/901-email-landings
build/lane10-8-journeys
build/demo-seed-lanes-8-10
fix/887-gm-own-confirmation
build/lane9-5-j1-admin
fix/918-audience-click
fix/useasync-overtaken
ci/939-critical-path
test/937-backend-suite-speed
chore/claude-orchestration
fix/924-xp-gm-only
fix/session-stale-poll
docs/decisions-d239-d242
fix/930-pypdf
build/lane8-journeys
build/925-quest-editor
fix/894-private-page-pictures
build/lane8-p4-drafts
build/lane8-p7-map
build/lane8-p6-handouts-rules
fix/wiki-fixture-names
build/lane10-d227-test-usage
fix/rsvp-horizon-test-tuesday
fix/media-volume-owner
build/lane8-p3-editor
fix/912-unidentified-rename
build/lane8-p5-party-quests
build/lane8-handout-takeback
build/lane10-pickers
build/lane8-p2-article
build/lane8-6-map
build/lane10-7b-email-pages
build/lane8-5-handouts
build/lane10-7a-test-the-bot-pages
build/lane8-p1-pages
build/lane8-4-quests
build/lane8-2b-feeds
build/lane8-3-parties
build/lane8-2a-links
build/lane10-5-commands
build/lane9-6-seed
build/lane10-4b-reactions
build/lane9-4d-pages
build/lane9-4c-pages
build/lane10-4a-posts
build/lane10-3-email
fix/891-web-fonts
build/lane9-4b-pages
build/lane10-6-test-the-bot
build/lane10-2-notify-out
build/lane8-1-wiki-text
build/lane9-4a-frame
design/lane10-drawings
fix/j6-budget
fix/j5-phone-flake
fix/869-test-leaks
fix/843-record-start-followups
build/lane7-all
build/lane6-all
build/lane9-b2-server
chore/changelog-fragments
docs/decisions-d218
build/lane9-a-owner-overview
fix/rule-card-level-label
fix/834-retention-semantics
fix/o3c-record-without-announcement
docs/decisions-d179
docs/decisions-d131-d170
fix/851-commit-before-response
build/lane6-j3-seed
build/lane9-b1-d67-identity
fix/841-blocking-loop
build/lane9-c-backups-audit
fix/vote-race-and-start-commit
fix/recording-announcement
fix/proposal-read-leak
fix/last-admin-guard
build/lane6-p4c-planning
build/lane6-p4b-margin
build/lane6-p4a-sheet
build/lane5-j4-seed
build/lane6-base
build/lane7-pages-talk
build/lane5-pages
build/lane7-pages-safety
build/lane7-p3-safety
build/lane7-p2-notify
build/lane6-p3-planning
build/lane6-p2-history
build/lane6-p1b-items
build/lane7-p1-threads
build/lane5-p6-faces
build/lane6-p1a-sheet
build/lane5-p5-find
build/lane5-p4a-board
build/lane5-p4b-scenes
build/lane5-p3-live
build/lane5-base
build/lane5-p2-cards
docs/lane5-decisions
build/review-r2-wide
fix/review-r1
build/lane5-p1-scenes
build/demo-seed
build/lane4-journeys-j5
build/lane4-page-review-b
build/lane4-page-review-a
build/lane4-page-read
build/lane4-journeys
build/lane4-page-sess2
build/lane4-page-sess1
build/lane4-notes
build/lane4-discord-on-approval
build/lane4-scheduling
build/lane4-versions
build/760-session-number
build/lane4-decisions
build/797-erase-no-regenerate
build/test-race-audit
build/lane3-completions
build/794-dark-only
build/796-titles-scroll
build/791-retention-anchor
build/a11y-sweep
build/shell-followups
build/retention-preview
build/753-backup-readback
docs/d91-d93
build/lane3-j1
build/ci-harness-paths-selftest
fix/useauth-test-race
build/ci-harness-paths
build/777-ai-settings-read
build/lane3-settings-c
build/lane3-settings-b
build/lane3-j6
build/lane3-sessionform
build/lane3-settings-a
build/lane3-members
build/lane3-thisweek
build/lane3-story
chore/87-align-python-3.12
v4.8.0-rc1
v4.7.1
v4.7.1-rc1
v4.7.0
v4.7.0-rc4
v4.7.0-rc3
v4.7.0-rc2
v4.7.0-rc1
v4.6.2
v4.6.2-rc4
v4.6.2-rc3
v4.6.2-rc2
v4.6.2-rc1
v4.6.1
v4.6.1-rc1
v4.6.0
v4.6.0-rc2
v4.6.0-rc1
v4.5.3
v4.5.3-rc1
v4.5.2
v4.5.2-rc1
v4.5.1
v4.5.1-rc2
v4.5.1-rc1
v4.5.0
v4.5.0-rc12
v4.5.0-rc11
v4.5.0-rc10
v4.5.0-rc9
v4.5.0-rc8
v4.5.0-rc7
v4.5.0-rc6
v4.5.0-rc5
v4.5.0-rc4
v4.5.0-rc3
v4.5.0-rc2
v4.5.0-rc1
v4.4.1
v4.4.0
v4.3.0
v4.2.3
v4.2.2
v4.2.1
v4.2.0
v4.1.2
v4.1.1
v4.1.0
v4.0.1
v4.0.0
v3.11.4
v3.11.3
v3.11.2
v3.11.1
v3.11.0
v3.10.0
v3.9.1
v3.9.0
v3.8.0
v3.7.0
v3.6.1
v3.6.0
v3.5.0
v3.4.1
v3.4.0
v3.3.0
v3.2.0
v3.1.0
v3.0.0
v2.1.0
v2.0.0
v1.1.0
v1.0.0
v0.12.0
v0.11.0
v0.10.0
v0.9.1
v0.3.3
v0.9.0
v0.8.0
v0.3.2
v0.3.1
v0.3.0
v0.7.0
v0.6.0
v0.5.0
v0.4.0
v0.2.1
v0.2.0
v0.1.1
v0.1.0
Labels
Clear labels
backend
Issue related to the backend
bot
Issue related to the bot
bug
Something is not working
duplicate
This issue or pull request already exists
enhancement
New feature
frontend
Issue related to the frontend
help wanted
Need some help
invalid
Something is wrong
question
More information is needed
recording
Issue related to the recording system
security
Security hardening / vulnerability work
wontfix
This won't be fixed
No labels
backend
bot
bug
duplicate
enhancement
frontend
help wanted
invalid
question
recording
security
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Dependencies
No dependencies set
Reference
rbrooks/Quest-Board#397
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
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?
Severity: CRITICAL
Found in the August 2026 session lifecycle review (#319).
Any second recording of a session silently destroys everything a GM built from the first one. When the bot POSTs a session's audio a second time — the natural "record part 2 after a bathroom break" or "co-GM records the epilogue" workflow — the backend has no idea a first recording was ever processed. It resets the session straight back to
processing, re-queues transcription, and the resulting run overwrites the GM's transcript and summary edits and deletes every highlight row for the session (including ones a GM manually added or approved) before reinserting a fresh set. Worse, because the bot writes each speaker's audio to a fixed per-session path, the second recording's WAV files land directly on top of the first recording's WAV files on disk — so there is no raw audio left to recover from even if someone wanted to reprocess the original.Evidence
webapp/backend/app/routers/bot.py:869-931(bot_upload_audio) — no check of the session's currentaudio_processing_status,content_approved_at, or whether a transcript already exists; it accepts a newsession_dirunconditionally.webapp/backend/app/routers/bot.py:903— setssession.audio_processing_status = AudioProcessingStatus.processingon every call, even when the session was alreadyapproved/readywith GM edits.webapp/backend/app/tasks/reminder_tasks.py:2184-2185—process_audiounconditionally overwritessession.transcriptandsession.summarywith the new run's output.webapp/backend/app/tasks/reminder_tasks.py:2235-2244— highlights are deleted bysession_idalone and reinserted withapproved=False; the delete has no manual/approved-row filter, so GM-approved and manually-added highlight rows are destroyed along with the machine-generated ones.bot/questboard_bot/cogs/recording.py:708(per-userout_wav = session_dir / f"{user_id_str}.wav") and:730(speakers.jsonrewrite) — both are keyed only by session ID and Discord user ID, so a second recording of the same session overwrites the first recording's files in place.webapp/backend/app/tasks/reminder_tasks.py:2826-2860, 3001(erase_member_recordings(regenerate_summaries=True)) — reaches the sameprocess_audiore-run path for every scrubbed session, clobbering GM-edited summaries/highlights as a side effect of an unrelated privacy operation.Failure scenario
A GM runs a 4-hour session and stops recording for a bathroom break, then starts a new
/recordfor part 2. Or: the GM spends an hour after the session fixing transcription errors and hand-curating highlights, then a co-GM later hits "record" against the same session ID for a recap epilogue. In both cases, the secondPOST /api/bot/sessions/{id}/audioclobbers the first recording's transcript, summary, and highlights, and the first recording's audio is gone from disk — unrecoverable, because no versioning of transcript/summary exists anywhere (unlike lore entries, which do haveLoreEntryVersion).Proposed fix
Add a state guard to
bot_upload_audio— refuse (409) a re-submission when the session already has a non-nulltranscriptorcontent_approved_atset, unless the caller passes an explicit force flag (surfaced in the bot as an admin-only override, not the default record-stop flow). Separately, giveprocess_audio's highlight reinsert an escape hatch: skip deleting rows where a GM-editable flag (e.g.edited_by_id IS NOT NULLorapproved = true) is set, only replacing machine-generated, unreviewed rows. Longer term, apply theLoreEntryVersionpattern toSession.transcript/summary(a version snapshot written before every overwrite, with a restore endpoint) so that even a forced re-run is recoverable. This state guard must land before any Celeryacks_late/retry work (tracked separately) — otherwise task retries after a worker crash will hit the exact same unguarded overwrite path.Acceptance criteria
POST /api/bot/sessions/{id}/audioreturns 409 when the target session already has a transcript orcontent_approved_atset, unless an explicit force flag is supplied.process_audio's highlight reinsert never deletes a highlight row that has been manually edited or GM-approved.erase_member_recordings(regenerate_summaries=True)no longer discards GM-edited summary/highlight content for sessions it touches (either it skips them or the same guard applies).Moved from v4.1.0 to v4.0.0.
Not because the defect changed, but because it sits directly in the path of the accuracy work. Developing the summarisation re-architecture means re-running
process_audioagainst real sessions repeatedly to compare output — and every one of those re-runs currently destroys the GM-edited transcript, summary and curated highlights, and can overwrite the source WAVs unrecoverably.So this needs to land before the v4.0.0 iteration loop starts in earnest — specifically before the chunking/beat work (#331–#334), where comparing successive summarisation runs against the same real session is the whole point.
Note the constraint this creates in the other direction: #398 (
acks_late+ stuck-state watchdog) still cannot land until this does, wherever #398 ends up. Enabling task redelivery before re-submission is guarded means a worker loss clobbers GM edits rather than recovering them.Verified against the acceptance criteria before closing. Not closing — one criterion is genuinely unmet, and it is the same class of data loss this issue was filed for, reached through a second entry point.
Met, with evidence
app/routers/bot.py:924-932, keyed ontranscript_updated_at. Tested:tests/test_bot.py:611-661covers both the 409 and theforce=trueoverride.bot/questboard_bot/cogs/recording.py:1004-1019catchesAudioAlreadySubmittedError, names the take directory, says nothing was lost.recording.py:857-880; first take keeps the bare session-id directory, later takes get a-take{N}suffix.app/tasks/reminder_tasks.py:2344-2349, delete filtered onapproved.is_(False), edited_by_id.is_(None). Code inspection only; no end-to-end test drivesprocess_audiowith a pre-existing approved highlight.Not met
Highlights are safe (the filtered delete above). The summary is not.
The protection was added at the router layer, but there are four call sites:
app/routers/bot.py:955app/routers/sessions.py:945app/routers/admin.py:224app/tasks/reminder_tasks.py:3262run_member_erasurecallsprocess_audio.delay(...)directly, andprocess_audiounconditionally writes:with no check on
content_approved_atortranscript_updated_at. So a GM who has edited a summary, then erases a different member's recordings withregenerate_summaries=True, silently loses their edit. Erasing one person's audio destroying another person's written work is exactly the shape of loss this issue exists to prevent.No test covers it:
tests/test_member_erasure.py:455only exercisesregenerate_summaries: False.The fix is a layer, not a patch
Guarding one router leaves the invariant enforced by discipline at four call sites, which is how this reached a second entry point in the first place. The check belongs inside
process_audio, with an explicit parameter —force=True/allow_overwrite=True— that each caller must pass deliberately. Retry and admin reprocess then say so out loud; erasure does not, and gets the protection by default.That also makes the rule testable in one place rather than four.
Remaining work
process_audio, with an explicit force parametersessions.py) and admin reprocess (admin.py) pass it; erasure regenerate does noterase_member_recordings(regenerate_summaries=True)— and fails against the current code before it passes against the fixFound by an acceptance-criteria pass rather than by a test or an incident. Related merge-blocker from the same sweep: #425.
The remaining criterion is fixed in
633f841.The guard now sits with the write it protects rather than in front of one of four doors.
process_audiorefuses whentranscript_updated_atis set unless the caller passesreplace_existing, defaulting to False — so the safe behaviour is what you get by omission, and replacing is the thing that has to be spelled out:routers/bot.pydata.forcerouters/sessions.pyTruerouters/admin.pyTruetasks/reminder_tasks.pyGuarding one router left the invariant enforced by discipline across four call sites, which is exactly how it reached a second entry point in the first place. Now the dangerous case is the one that requires an explicit argument.
The bot endpoint keeps its 409 rather than relying solely on the task: a caller deserves an error, not a task that vanishes silently.
Tests: erasure's call site queues five positional arguments and never asks to replace; and the signature keeps
replace_existingdefaulting to False, so a future caller cannot acquire overwrite behaviour by accident.One criterion I did not close out: the highlight filtered-delete is still inspection-only — no end-to-end test drives
process_audiowith a pre-existing approved highlight. The filter itself (approved.is_(False), edited_by_id.is_(None)) is correct, but it is the kind of thing that regresses silently. Worth a test when someone is next in that code; not blocking, since the summary path was the actual data loss.1,227 passing before, 1,233 after with #425. Closing.