v4.0.1 — retention safety and secrets at rest #435

Merged

claude-bot merged 4 commits from fix/v4.0.1-retention-safety into main 2026-08-29 06:58:08 +00:00

Conversation 0 Commits 4 Files changed 27 +1467 -103

claude-bot commented 2026-08-29 05:58:03 +00:00

Contributor

Copy link

Closes the v4.0.1 milestone's code: #427, #428, #429, #430, #432.

No migration. The only model change is a one-line comment, so there is no schema change, no backfill, and no rehearsal needed — a far lower-risk deploy than v4.0.0.

⚠️ Images must be rebuilt, not pulled. #429 changes the backend Dockerfile; a pull-and-restart would not apply it. Deploy with docker compose up -d --build.

---

#427 — raw audio is never deleted inline

The recommended retention mode destroyed the per-speaker WAVs during process_audio, 27 seconds after the transcript was written in the case that prompted this. Its reasoning — "there is nothing left to approve or reprocess" — is wrong in the case that matters, because the pipeline changes: v4.0.0 rewrote transcription, extraction, validation and composition, and no recording survived to validate any of it against.

The mode is now trash_after_processing. The audio is marked trashed, the bytes stay, and the sweeper removes them once the grace elapses. audio_file_path is deliberately kept — the sweeper needs it to find the directory and admin restore needs it to bring the session back.

Two further changes were required, or the fix would have been cosmetic. Neither was in the issue; both came from reading the sweeper rather than running it:

• Pass 2 would have deleted the audio anyway. It removes anything whose retention window has elapsed, guarded only by "has an audio path". A trashed session keeps its path, and trash-on-success stamps the trash at the exact moment the transcript is written — so the next Beat tick would have deleted it with the grace never observed. That race was already reachable for a GM trashing an older session by hand.
• An expiring window still deleted directly, so retain_days never routed through the trash at all.

⚠️ This changes effective retention: audio under retain_days now lives for the configured window plus the trash grace. That is the intent — an expiring window should give the GM a last chance rather than delete the only copy on a scheduled task — but it is a disk-usage change.

The old mode name is accepted on read and normalised. Every install predating this has delete_after_processing in app_settings and in campaigns.audio_retention_mode; renaming without an alias would have silently dropped them to the default.

#432 — a silent speaker reported as having spoken

merge_uncaptured_members added a backend-dropped track's owner to the uncaptured list correctly, and it made no difference: the same owner was still in speakers, and spoke is assigned before in_channel_silent. #425 tested that function in isolation, where it behaves — the defect lived only in the pairing.

Extracted as narrow_speakers_to_captured beside its counterpart, deliberately rather than inlining three lines: the missing step would otherwise live only inside process_audio, which no test drives end to end.

#430 — a restore onto a different SECRET_KEY

Reading an undecryptable row raised InvalidTag straight out of get_setting, past the four "not configured" policies the module's own docstring commits to. Now it degrades to None, logs once per key, and is reported at startup and via GET /api/admin/settings/secrets-health — which is what lets Admin tell "never configured" from "configured but unreadable". Those render identically in every settings form, which is how a restore produced a system that looked complete and was not.

The startup audit is non-fatal by design: an operator who has just restored onto a fresh host needs the app up to re-enter the settings the report is telling them about.

#428 — plaintext at rest

Encryption only happened on write, and legacy plaintext rows were passed through on read forever, so a credential never rotated since encryption was introduced was still in clear text — in the database, in every dump, in every off-host archive — with nothing reporting it. The startup audit rewrites such rows and names each one, because the operator needs to know which credential to rotate.

#429 — backups restorable by the server that made them

pg_dump ran from the backend image while pg_restore runs against the db image, on different major versions — so every dump was rejected with unsupported version (1.16) in file header while the job recorded success. Production has eight such files.

The image now pins postgresql-client-16 to match postgres:16-alpine, and a test asserts the Dockerfile and docker-compose.yml stay in step. Both backup paths now compare client and server majors before writing, so a mismatch is a failed backup rather than an unrestorable one.

Shipping that check without the alignment would have turned "unrestorable backups" into "no backups at all" — strictly worse — which is why they are one commit.

Verified end to end rather than by inspection: built the image, confirmed pg_dump 16.15, dumped in custom format with it, restored with postgres:16-alpine's pg_restore, row counts matched.

---

Verification

1,407 backend tests (from 1,362), 440 frontend, ruff clean at CI's pinned 0.4.4.

Every new guard was reverted individually and its test confirmed to fail — the sweeper one reports "grace period was not honoured", and both decryption tests fail with the exception escaping exactly as production did. Two existing retention tests were rewritten to assert the two-stage trash-then-delete explicitly, which is stronger than the single assertion they replaced.

Still needs a human before this milestone is done

• Rotate the production bot API key (#428). Deliberately not done here: it must change on both sides at once and breaks bot↔backend auth in between. The startup audit re-encrypts the existing key, which protects it going forward but does not un-expose it — the current value is already in every archive taken to date.
• Prod's eight existing dumps are confirmed unrestorable and will be replaced by the first backup after this deploys. Not deleted; until a fresh one exists they are the only dumps there.

Knock-on findings, recorded not fixed

• #404 is defused by #427 — there is no longer a delete before the commit. Suggested for closure.
• #402's countdown half now exists for free; the warning half remains.
• #407 compounds with erasure's inline wav.unlink(), which #427 deliberately did not touch — a mis-targeted erasure destroys audio with no grace while every other route now has one. Options laid out on that issue.

🤖 Generated with Claude Code

Closes the v4.0.1 milestone's code: #427, #428, #429, #430, #432. **No migration.** The only model change is a one-line comment, so there is no schema change, no backfill, and no rehearsal needed — a far lower-risk deploy than v4.0.0. ⚠️ **Images must be rebuilt, not pulled.** #429 changes the backend `Dockerfile`; a pull-and-restart would not apply it. Deploy with `docker compose up -d --build`. --- ## #427 — raw audio is never deleted inline The recommended retention mode destroyed the per-speaker WAVs during `process_audio`, 27 seconds after the transcript was written in the case that prompted this. Its reasoning — *"there is nothing left to approve or reprocess"* — is wrong in the case that matters, because the pipeline changes: v4.0.0 rewrote transcription, extraction, validation and composition, and no recording survived to validate any of it against. The mode is now `trash_after_processing`. The audio is marked trashed, the bytes stay, and the sweeper removes them once the grace elapses. `audio_file_path` is deliberately kept — the sweeper needs it to find the directory and admin restore needs it to bring the session back. **Two further changes were required, or the fix would have been cosmetic.** Neither was in the issue; both came from reading the sweeper rather than running it: - **Pass 2 would have deleted the audio anyway.** It removes anything whose retention window has elapsed, guarded only by "has an audio path". A trashed session keeps its path, and trash-on-success stamps the trash at the exact moment the transcript is written — so the next Beat tick would have deleted it with the grace never observed. That race was *already* reachable for a GM trashing an older session by hand. - **An expiring window still deleted directly**, so `retain_days` never routed through the trash at all. ⚠️ **This changes effective retention**: audio under `retain_days` now lives for the configured window **plus** the trash grace. That is the intent — an expiring window should give the GM a last chance rather than delete the only copy on a scheduled task — but it is a disk-usage change. The old mode name is accepted on read and normalised. Every install predating this has `delete_after_processing` in `app_settings` *and* in `campaigns.audio_retention_mode`; renaming without an alias would have silently dropped them to the default. ## #432 — a silent speaker reported as having spoken `merge_uncaptured_members` added a backend-dropped track's owner to the uncaptured list correctly, and it made no difference: the same owner was still in `speakers`, and `spoke` is assigned before `in_channel_silent`. #425 tested that function in isolation, where it behaves — the defect lived only in the pairing. Extracted as `narrow_speakers_to_captured` beside its counterpart, deliberately rather than inlining three lines: the missing step would otherwise live only inside `process_audio`, which no test drives end to end. ## #430 — a restore onto a different `SECRET_KEY` Reading an undecryptable row raised `InvalidTag` straight out of `get_setting`, past the four "not configured" policies the module's own docstring commits to. Now it degrades to `None`, logs once per key, and is reported at startup and via `GET /api/admin/settings/secrets-health` — which is what lets Admin tell "never configured" from "configured but unreadable". Those render identically in every settings form, which is how a restore produced a system that looked complete and was not. The startup audit is **non-fatal by design**: an operator who has just restored onto a fresh host needs the app up to re-enter the settings the report is telling them about. ## #428 — plaintext at rest Encryption only happened on write, and legacy plaintext rows were passed through on read forever, so a credential never rotated since encryption was introduced was still in clear text — in the database, in every dump, in every off-host archive — with nothing reporting it. The startup audit rewrites such rows and **names each one**, because the operator needs to know which credential to rotate. ## #429 — backups restorable by the server that made them `pg_dump` ran from the backend image while `pg_restore` runs against the db image, on different major versions — so every dump was rejected with `unsupported version (1.16) in file header` while the job recorded success. Production has eight such files. The image now pins `postgresql-client-16` to match `postgres:16-alpine`, and a test asserts the `Dockerfile` and `docker-compose.yml` stay in step. Both backup paths now compare client and server majors *before* writing, so a mismatch is a failed backup rather than an unrestorable one. Shipping that check without the alignment would have turned "unrestorable backups" into "no backups at all" — strictly worse — which is why they are one commit. Verified end to end rather than by inspection: built the image, confirmed `pg_dump 16.15`, dumped in custom format with it, restored with `postgres:16-alpine`'s `pg_restore`, row counts matched. --- ## Verification 1,407 backend tests (from 1,362), 440 frontend, ruff clean at CI's pinned 0.4.4. Every new guard was reverted individually and its test confirmed to fail — the sweeper one reports *"grace period was not honoured"*, and both decryption tests fail with the exception escaping exactly as production did. Two existing retention tests were rewritten to assert the two-stage trash-then-delete explicitly, which is stronger than the single assertion they replaced. ## Still needs a human before this milestone is done - **Rotate the production bot API key** (#428). Deliberately not done here: it must change on both sides at once and breaks bot↔backend auth in between. The startup audit re-encrypts the existing key, which protects it going forward but does **not** un-expose it — the current value is already in every archive taken to date. - **Prod's eight existing dumps are confirmed unrestorable** and will be replaced by the first backup after this deploys. Not deleted; until a fresh one exists they are the only dumps there. ## Knock-on findings, recorded not fixed - **#404** is defused by #427 — there is no longer a delete before the commit. Suggested for closure. - **#402**'s countdown half now exists for free; the warning half remains. - **#407** compounds with erasure's inline `wav.unlink()`, which #427 deliberately did not touch — a mis-targeted erasure destroys audio with no grace while every other route now has one. Options laid out on that issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 3 commits 2026-08-29 05:58:03 +00:00

fix(backend): never delete raw audio inline (#427, #432) ... d4881b9900

## #427 — trash on success, with a grace floor

The recommended retention mode destroyed the per-speaker WAVs during
process_audio, 27 seconds after the transcript was written in the case that
prompted this. The reasoning in its docstring — "there is nothing left to
approve or reprocess" — is wrong in the case that matters, because the
pipeline changes. v4.0.0 rewrote transcription, extraction, validation and
composition, and the most useful way to validate that work was impossible:
every recording had already been destroyed at the moment its first transcript
was written. The nightly off-host archive was running during the deletion and
walked that directory afterwards, so it captured it empty.

So the mode is now trash_after_processing and nothing is deleted inline.
The audio is marked trashed, the bytes stay on disk, and the sweeper removes
them once the grace has elapsed. audio_file_path is deliberately kept: the
sweeper needs it to find the directory and admin restore needs it to bring the
session back.

Three things had to change together, or the fix would have been cosmetic:

- The trash grace is now a floor rather than a default. A configured value
  below seven days is clamped up, so a misconfiguration cannot reintroduce
  same-day loss.
- Sweeper pass 2 skips trashed sessions. It deletes anything whose retention
  window has elapsed and its only guard was "has an audio path", so a session
  trashed today with an expired window was deleted on the next Beat tick —
  its grace never observed. Since trash-on-success stamps the trash at the
  exact moment the transcript is written, that race would have undone the
  whole change. It was already reachable for a GM trashing an older session
  by hand.
- An expiring window now trashes rather than deletes, so every mode reaches
  deletion through the same recoverable window. Audio under retain_days
  therefore lives for the window plus the grace; that is the intent, not an
  oversight.

The mode keeps accepting its old name on read. Every install predating this
has delete_after_processing written into app_settings and into
campaigns.audio_retention_mode, and a mode literally named "delete" that does
not delete is the drift that cost a session in the first place — so the value
is normalised rather than left to mean two things.

## #432 — the silent speaker reported as having spoken

merge_uncaptured_members added a backend-dropped track's owner to the
uncaptured list correctly, and it made no difference: the same owner was still
in `speakers`, generate_attendance_proposals assigns `spoke` to every id in
that dict before assigning in_channel_silent to uncaptured ids not already
proposed, so `spoke` always won. #425 tested that function in isolation, where
it behaves; the defect lived only in the pairing.

Extracted the narrowing as narrow_speakers_to_captured next to
merge_uncaptured_members, because a step that exists only inline in a task no
integration test drives is a step nothing can catch. The two are now visibly a
pair and documented as one.

Verified by temporarily reverting each guard and confirming the new tests fail:
the sweeper test reports "grace period was not honoured" without the pass-2
skip. 1,378 backend and 440 frontend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(backend): make secrets at rest readable, or say so (#428, #430) ... 9517977068

Two halves of one question — is what we stored actually protected, and can we
still read it — so they share a single pass over the _ENCRYPTED_KEYS rows.

## #430 — a restore onto a different SECRET_KEY

Sensitive settings are encrypted with a key derived from SECRET_KEY, which
lives in .env and is therefore in no database backup. Restoring a dump onto a
host whose key differs leaves every one of those rows unreadable — and reading
one raised InvalidTag straight out of get_setting, past the four "not
configured" policies this module's own docstring commits to. Every LLM-touching
endpoint and task failed with an opaque cryptography error, nowhere near the
restore that caused it.

decrypt_setting now raises UndecryptableSettingError, and get_setting catches
it and returns None so the existing policies apply. The catch in crypto is
deliberately broad: a wrong key raises InvalidTag, but a truncated or
hand-edited row raises from base64, the slicing, or json, and all of them mean
the same thing to a caller.

This is a demotion, not a repair. An undecryptable row is worse than an absent
one, because the operator believes it is configured — hence the one-time log
line (settings are read on nearly every request), the startup report, and
GET /api/admin/settings/secrets-health, which is what lets Admin tell "never
configured" from "configured but unreadable". Those look identical in every
settings form, which is how a restore produced a system that looked complete
and was not.

## #428 — plaintext at rest

Encryption happens on write, and decrypt_setting passes legacy plaintext rows
through unchanged. Correct as a migration path, wrong as a permanent resting
state: the bot API key had never been rotated since encryption was introduced,
so it was still in clear text in the database, in every dump, and in every
off-host archive, and nothing ever said so. The startup audit rewrites such
rows through encrypt_setting and names each one, because a credential that was
readable in every backup taken until now needs rotating and the operator has to
know which.

The audit is non-fatal on purpose. An operator who has just restored onto a
fresh host needs the app up to re-enter the settings the report is telling them
about; a stricter check would lock them out of the only UI that can fix it.

## Coverage

There were no tests for encryption at all, which is most of the explanation for
both defects: nothing asserted that an _ENCRYPTED_KEYS row is actually stored
encrypted, and nothing ever read a row back under a different key. The new file
does both, driving the mismatch by swapping SECRET_KEY in the environment
between write and read — faithful, since _derive_key resolves it per call.

Verified by reverting the catch: both degradation tests fail there with the
exception escaping, exactly as production did. 1,391 backend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(backend): make backups restorable by the server that made them (#429) ... 9698b4c32a

The backup ran pg_dump from the backend image; a restore runs pg_restore
against the db image. Debian's postgresql-client metapackage tracks the distro
(trixie ships 17) while the db service is pinned to postgres:16-alpine, so
every dump was written in an archive format the server's own pg_restore
refuses outright — "unsupported version (1.16) in file header", reading nothing
at all. The job exited zero, backup_logs recorded success, and the files were
the right size. Production has eight such dumps.

Two changes, and the second is only safe because of the first.

Align the versions. The image now installs postgresql-client-${PG_MAJOR} with
PG_MAJOR=16, matching docker-compose's postgres:16-alpine. Debian 13 carries
only 17, so this needs the PGDG repo; gnupg is purged afterwards so the build
dependency does not persist into the runtime image. A test parses ARG PG_MAJOR
out of the Dockerfile and the image tag out of docker-compose.yml and asserts
they agree — they live in different files and nothing else connected them,
which is how they drifted apart.

Refuse an unrestorable backup. Both the scheduled task and the manual CLI path
compare pg_dump's major against the server's server_version_num before writing
anything, so a mismatch is a failed backup with an explanation rather than a
success recording a file nobody can read. Shipping that check without the
alignment above would have turned "unrestorable backups" into "no backups",
which is worse; they go together.

A client newer than the server is the dangerous direction and raises. An older
client produces an archive the newer server reads fine, so it is allowed and
merely noted. Unknown versions proceed with a warning: refusing to back up
because a version string could not be parsed would be a worse failure than the
one being guarded against.

Verified end to end rather than by inspection. Built the image, confirmed
pg_dump 16.15, dumped a database in custom format with it, and restored that
dump with postgres:16-alpine's pg_restore — the exact operation that used to
fail. Row counts matched. ffmpeg and libmagic still work in the built image.

docs/OPERATIONS.md now states the version requirement, how to check it, and
that a failing backup with a mismatch message means the two have drifted.

1,407 backend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rbrooks added 1 commit 2026-08-29 06:44:51 +00:00

chore(release): v4.0.1 ... 287d171669

APP_VERSION and BOT_EXPECTED_APP_VERSION to 4.0.1. BOT_CONTRACT_VERSION stays
1 — nothing under /api/bot/* changed, so an operator can still upgrade one
image at a time.

Changelog entry verified extractable by the release workflow's own regex.

Two warnings are in the entry rather than left to be discovered:

- The backend image now pins its PostgreSQL client to the database's major
  version, so this must be deployed with `--build`. A pull-and-restart looks
  like it worked and silently keeps the old, mismatched client.
- Audio now occupies disk for the retention window PLUS the trash grace,
  where the previous default deleted it during processing and occupied
  nothing. That is the point of the release, but it is a capacity change and
  a self-hoster should not meet it as a full volume.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude-bot merged commit db77a4e657 into main 2026-08-29 06:58:08 +00:00

claude-bot referenced this pull request from a commit 2026-08-29 06:58:09 +00:00

Merge PR #435: v4.0.1 — retention safety and secrets at rest

Sign in to join this conversation.

Reviewers

No reviewers

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

2 participants

Notifications

Subscribe

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!435

No description provided.