[Review] Standards and consistency pass across API, errors, migrations and conventions #445

New issue

Open

opened 2026-08-29 18:19:16 +00:00 by claude-bot · 0 comments

claude-bot commented

2026-08-29 18:19:16 +00:00

Contributor

Part of the v4.6.0 release-readiness gate.

What

Make the codebase internally consistent, so a reader who has learned one corner can predict the others. Inconsistency is a correctness risk, not an aesthetic one — it is what makes a reviewer skim past the case that differs.

Areas

API surface

Error responses genuinely {"detail": "..."} everywhere, with 4xx/5xx used as CLAUDE.md states
Status codes consistent for the same condition across routers — a 409 in one place and a 400 in another for the same class of conflict is a trap
Pagination applied wherever a list can grow. The admin recordings list is currently unpaginated and does a per-row user lookup
Auth dependency chosen consistently; no endpoint hand-rolling a check the dependencies already provide

Errors and failure modes

A typed exception where callers must distinguish conditions (#430 added UndecryptableSettingError for exactly this)
Consistent decisions about what is retryable versus permanent, since Celery retry policy depends on it
No primitive library exceptions escaping a service boundary

Migrations

The chain list in webapp/CLAUDE.md matches alembic history, including the head
Expand-only discipline stated where it applies. #330's retained columns caused a CI failure precisely because the ORM and the database diverged deliberately and nothing said so at the drop_all site
Every migration says whether it backfills and whether a downgrade is safe

Project conventions

The rules in CLAUDE.md and webapp/CLAUDE.md still match reality, and are followed
Naming consistent for the same concept across backend, bot and frontend
Bot contract version discipline documented at both ends

Method

Prefer a lint rule or a test over a written convention wherever one is possible. #429's Dockerfile/docker-compose.yml version check is the model: two files that must agree, with a test that fails when they drift — that is worth more than a sentence in a document, because it cannot be forgotten.

Acceptance criteria

Error response shape and status-code usage audited and made consistent, with deviations justified in place
Every growable list endpoint is paginated or explicitly documented as bounded
Migration chain documentation matches alembic history exactly, and a check enforces it
Divergences between ORM models and the physical schema are documented where they are relied upon
CLAUDE.md files reflect actual practice; anything no longer true is corrected rather than left aspirational
Where a convention can be mechanically enforced, it is

Part of the v4.6.0 release-readiness gate. ## What Make the codebase internally consistent, so a reader who has learned one corner can predict the others. Inconsistency is a correctness risk, not an aesthetic one — it is what makes a reviewer skim past the case that differs. ## Areas **API surface** - Error responses genuinely `{"detail": "..."}` everywhere, with 4xx/5xx used as `CLAUDE.md` states - Status codes consistent for the same condition across routers — a 409 in one place and a 400 in another for the same class of conflict is a trap - Pagination applied wherever a list can grow. The admin recordings list is currently unpaginated and does a per-row user lookup - Auth dependency chosen consistently; no endpoint hand-rolling a check the dependencies already provide **Errors and failure modes** - A typed exception where callers must distinguish conditions (#430 added `UndecryptableSettingError` for exactly this) - Consistent decisions about what is retryable versus permanent, since Celery retry policy depends on it - No primitive library exceptions escaping a service boundary **Migrations** - The chain list in `webapp/CLAUDE.md` matches `alembic history`, including the head - Expand-only discipline stated where it applies. #330's retained columns caused a CI failure precisely because the ORM and the database diverged deliberately and nothing said so at the drop_all site - Every migration says whether it backfills and whether a downgrade is safe **Project conventions** - The rules in `CLAUDE.md` and `webapp/CLAUDE.md` still match reality, and are followed - Naming consistent for the same concept across backend, bot and frontend - Bot contract version discipline documented at both ends ## Method Prefer a lint rule or a test over a written convention wherever one is possible. #429's `Dockerfile`/`docker-compose.yml` version check is the model: two files that must agree, with a test that fails when they drift — that is worth more than a sentence in a document, because it cannot be forgotten. ## Acceptance criteria - [ ] Error response shape and status-code usage audited and made consistent, with deviations justified in place - [ ] Every growable list endpoint is paginated or explicitly documented as bounded - [ ] Migration chain documentation matches `alembic history` exactly, and a check enforces it - [ ] Divergences between ORM models and the physical schema are documented where they are relied upon - [ ] `CLAUDE.md` files reflect actual practice; anything no longer true is corrected rather than left aspirational - [ ] Where a convention can be mechanically enforced, it is

claude-bot added this to the v4.6.0 - Release Readiness: Review, Simplification & Documentation milestone

2026-08-29 18:19:28 +00:00