bbchops/transcription
1
0
Fork 0
generated from john/python-template
Code Issues 4 Pull Requests Actions Packages Projects Releases Wiki Activity

V4.6 Scope & Implementation Plan

Browse Source
This commit is contained in:
zoltan57
2026-08-17 15:25:52 -05:00
parent 1ee9ebbffc
commit b3d8eb6e97
3 changed files with 658 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/architecture_code_review_2026-08-17.md
+158 -7
View File
@@ -19,6 +19,37 @@
- **`ty` is configured as a dev dependency but is not usable as a gate.** 197 diagnostics, ~160 of which are SQLModel relationship false positives already suppressed with `# pyright: ignore` comments that `ty` does not honor.
- **Schema evolution is hand-rolled** in `db/operations.py` with raw `ALTER TABLE`/`CREATE INDEX IF NOT EXISTS` and a SQLite-shaped `CHAR(32)` UUID column. There is no Alembic. Postgres portability is claimed but not actually exercised.
- **Meaningful duplication exists in the UI layer** (~500 lines): media-URL resolution, `_parse_uuid`, settings resolution, delete-confirmation scaffolds, and hand-rolled tables are each reimplemented 3-5 times.
- **Meaningful duplication also exists in the service layer** (~400 lines): `DocumentType` and `PersonRole` registry CRUD are structurally identical, 38 "not-found" raises are hand-written, and three media-storage flows are reimplemented.
---
## 1a. Post-Review Addendum
The findings below were established during the V4.6 scoping discussion that followed the original review. They restate severity in light of the project's confirmed operating context and add findings discovered during that discussion. **The original finding IDs are stable and remain the canonical reference for the V4.6 documents.**
### Confirmed Operating Context
| Question | Answer |
| :--- | :--- |
| Database | **SQLite only.** PostgreSQL is the intended destination but is deferred beyond V4.6. `JSONBCompat` is retained. |
| Topology | **Single user, single process** today. Multi-user server is the stated direction. |
| Schema evolution | **Re-level from current metadata.** No Alembic. The app is pre-production and the schema is still moving. |
| Existing data | Rebuilt from scratch during implementation; migrated from backup as the final step. |
| Release character | **Pure remediation.** No new features. |
| Scope band | Critical through Low, inclusive. |
### Severity Re-Grades
| ID | Original | Re-graded | Rationale |
| :--- | :--- | :--- | :--- |
| CRIT-01 | Critical | **High** | With one process and one worker there is no live duplicate-processing race. The missing `.limit(1)` and the eager-load cost remain genuine defects; the atomic claim becomes forward-compatibility work for the multi-user direction rather than an active-incident fix. |
| CRIT-02 | Critical | **Critical** (unchanged) | Read amplification is independent of both topology and dialect. It costs on every read today. |
| HIGH-05 | High | **High** (reframed) | The remedy is **not** Alembic. Because the schema is pre-production and the data is disposable, the correct fix is to delete `upgrade_schema` and the three `_upgrade_*` functions outright and re-level the schema from current SQLModel metadata. This automatically resolves the `CHAR(32)` defect. |
| MED-01 | Medium | **Medium** (low urgency) | Single-user operation means event-loop stalls are self-inflicted only. Remains in scope. |
### Items Added During Scoping
These are recorded as [HIGH-08], [MED-10] through [MED-14], and [LOW-08] below.
---
@@ -27,6 +58,7 @@
### Critical Severity
#### [CRIT-01] Queued-job claim has no row lock, no CAS, and no LIMIT — duplicate processing and full-queue load
> **Re-graded to High.** See [§1a](#severity-re-grades). The single-process deployment removes the live duplicate-processing race; the missing `.limit(1)` and the eager-load cost are still real, and the atomic claim is retained as forward-compatibility work.
- **Location:** `src/transcription/services/jobs.py:170-187`; claim logic at `src/transcription/services/workflows.py:188-196`; divergent duplicate at `src/transcription/db/operations.py:143-151`
- **Problem & Consequence:** `read_next_queued_job` issues `SELECT ... WHERE status = 'queued' ORDER BY date_created, id` with `selectinload(Job.document)` and `selectinload(Job.job_sources).selectinload(JobSource.source)` — and **no `.limit(1)`**. It materializes the entire queue plus its document/job_source/source graph on every worker tick just to call `.first()`. With a backlog of N jobs this is O(N) rows and several extra SELECT round-trips per second.
@@ -137,8 +169,9 @@
- `operations.py:77` adds `preferred_execution_attempt_id CHAR(32)` — but the model declares it a `UUID` FK to `execution_attempt.id` (`models.py:260-264`). On PostgreSQL this creates a `char(32)` column that will not compare or join against a native `uuid` column, and the declared foreign key is never created at all.
- Every upgrade is unversioned and re-inspected on each startup; there is no down path, no history table, and no way to tell whether a production database is current.
- `asyncpg` and `psycopg2-binary` are both dependencies (`pyproject.toml:17,21`) and `JSONBCompat` (`models.py:27-35`) carefully supports JSONB, so Postgres is clearly an intended target — but no test exercises it. All 264 tests run on SQLite.
- **Recommendation:** Adopt Alembic. Generate an initial revision from current metadata, convert the three `_upgrade_*` functions into explicit revisions, and keep `create_all()` for the test/dev bootstrap path only (`Settings.should_bootstrap_schema` already gates this correctly at `config.py:140-145`). At minimum, immediately fix the `CHAR(32)` type to match the model.
- **Effort:** L
- **Recommendation:** **Re-level the schema from current metadata; do not adopt Alembic.** The application is pre-production, the schema is still evolving, and the existing data is disposable and backed up. Delete `upgrade_schema` and the three `_upgrade_*` functions (`operations.py:25-109`) together with their tests (`tests/test_db.py:109-172`), drop the database, and let `create_all()` generate the schema from SQLModel metadata. This removes the `CHAR(32)` defect at the root rather than patching it, because SQLModel emits the correct column type per dialect automatically (verified: it emits native `UUID` and `JSONB` under the PostgreSQL dialect). `Settings.should_bootstrap_schema` (`config.py:140-145`) already gates the bootstrap path correctly. Reintroduce a migration tool only when the schema stabilizes and real data must survive upgrades.
- **Sequencing:** This must land in the *same* pass as [HIGH-04] (missing indexes), [CRIT-02] (`lazy` flip), and [HIGH-08] (`use_alter`), because all four regenerate the same schema.
- **Effort:** M
#### [HIGH-06] `ty` is a configured dev tool but produces 197 diagnostics and cannot gate CI
- **Location:** `pyproject.toml:38`; suppression comments throughout, e.g. `src/transcription/services/jobs.py:67-68,103-104,123-124,156,180-181`
@@ -154,9 +187,30 @@
- `jobs_page.py` imports `transcription.db.session.session_scope` and manages the session lifecycle itself around `create_job_for_document`, while every sibling call site goes through a service.
- `sources_page.py:439` imports `sqlalchemy.inspect` and reads `inspect(attempt).unloaded` to decide rendering — the presentation layer is now coupled to the loader strategy, and will silently misbehave if a service changes its deferred columns.
- `document_panzoom.py` calls `get_settings()` inside a component and re-implements upload-path resolution.
- **Recommendation:** Add a `JobService`/workflow method that owns `session_scope` internally; have `SourceService` return a plain `transport_body_deferred: bool` flag on a read model; pass a ready media URL into `document_panzoom` (or delete it — it is exported from `components/__init__.py` but used by no page).
- **Effort:** M
#### [HIGH-08] Circular foreign-key cycle makes `create_all` fail on PostgreSQL
- **Location:** `src/transcription/db/models.py:260-264` (`Source.preferred_execution_attempt_id`), with the cycle running `source` → `job_source` → `execution_attempt` → `source`
- **Problem & Consequence:** Verified by compiling the SQLModel metadata against the PostgreSQL dialect, which emits:
> `SAWarning: Cannot correctly sort tables; there are unresolvable cycles between tables "execution_attempt, job_source, source", which is usually caused by mutually dependent foreign key constraints.`
The resulting sort order places `execution_attempt` **before** `source`, but `execution_attempt.source_id` is a foreign key to `source.id`. On PostgreSQL, where foreign keys are enforced inline at `CREATE TABLE` time, this is a hard `create_all()` failure. SQLite does not enforce the ordering, so the defect is completely invisible on the current test suite and will surface only at the moment of the Postgres cutover.
- **Recommendation:** Mark the nullable leg of the cycle with `use_alter=True` so SQLAlchemy emits it as a deferred `ALTER TABLE ... ADD CONSTRAINT` after all tables exist. Verified to silence the warning and produce a correct ordering.
```python
# models.py — Source
preferred_execution_attempt_id: UUID | None = Field(
default=None,
sa_column=Column(
GUID(),
ForeignKey("execution_attempt.id", use_alter=True, name="fk_source_preferred_attempt"),
nullable=True,
),
)
```
This is cheap, harmless on SQLite, and should land with the schema re-level ([HIGH-05]) so the Postgres path is unblocked whenever it is taken.
- **Effort:** S
### Medium Severity
#### [MED-01] Blocking filesystem and CPU work on the async event loop
@@ -217,9 +271,71 @@
#### [MED-09] Large inline SVG asset embedded in a Python module
- **Location:** `src/transcription/ui/theme.py:36-40` (single 23,317-character line)
- **Problem & Consequence:** `VIBESCRIBE_LOGO_SVG` is a 23KB string literal inside a Python source file. It trips `ruff`'s `line-too-long`, makes the module unreadable and undiffable, and contradicts `ui.instructions.md`'s rule that static assets live under `ui/static/` and be read via `importlib.resources`. The project already has exactly the right helper for this — `ui/resources.py:10-19`'s cached `importlib.resources` reader.
- **Recommendation:** Move to `ui/static/vibescribe_logo.svg` and load it through a `read_svg` sibling of the existing `read_css`.
- **Effort:** S
#### [MED-10] `DATABASE_URL` is silently ignored by `Settings`
- **Location:** `src/transcription/config.py` (`Settings`, nested `database` config); `docker-compose.yml:10`
- **Problem & Consequence:** `docker-compose.yml:10` sets `DATABASE_URL`, plainly intending to point the application at a different database. `Settings` reads its database configuration from a *nested* `database` model with `env_nested_delimiter="__"` and `extra="ignore"`, so `DATABASE_URL` matches nothing and is discarded without warning. Verified at runtime: with `DATABASE_URL=postgresql://...` exported, `get_settings().database` still resolves to `driver='sqlite' path='./data/transcription.db'`. An operator following the committed compose file gets SQLite while believing they configured PostgreSQL — silent, and the failure mode is data written to the wrong place.
- **Recommendation:** Pick one contract and make the other loud. Either add an explicit `DATABASE_URL` field that parses a full URL into the nested settings, or delete `DATABASE_URL` from `docker-compose.yml` and document `DATABASE__DRIVER` / `DATABASE__PATH`. Given [HIGH-05] defers PostgreSQL, the correct V4.6 action is to remove the misleading compose variable and document the real nested names. Escalates to Critical the moment PostgreSQL is enabled.
- **Effort:** S
#### [MED-11] `DocumentType` and `PersonRole` registry CRUD is duplicated wholesale
- **Location:** `src/transcription/services/documents.py:49-61,64-72,350-500`; `src/transcription/services/people.py:49-79,214-378`
- **Problem & Consequence:** The two models are structurally identical (`id, semantic_key, label, normalized_label, is_active, created_at, updated_at`) and carry identical operation sets, guards, and error mappings:
| Operation | `DocumentType` | `PersonRole` |
| :--- | :--- | :--- |
| label normalizer + casefold key | `documents.py:49-61` | `people.py:49-75` |
| summary dataclass | `documents.py:64-72` | `people.py:79` |
| list / list summaries with counts | `documents.py:350-388` | `people.py:214-249` |
| create, `IntegrityError` → conflict | `documents.py:390-413` | `people.py:251-274` |
| read, not-found raise | `documents.py:415-430` | `people.py:276-291` |
| update, `IntegrityError` → conflict | `documents.py:432-461` | `people.py:293-322` |
| delete, built-in guard + referenced guard | `documents.py:463-491` | `people.py:324-352` |
| `is_*_referenced` | `documents.py:493-500` | `people.py:354-378` |
The duplication extends to the wording of the user-facing suggestion strings ("Deactivate the type instead" / "Deactivate the role instead"). Any fix to one — a normalization bug, a missing guard, an error-category correction — has to be remembered twice.
- **Recommendation:** Introduce a generic `RegistryService[ModelT]` base that owns the eight operations, the label normalization, and the `IntegrityError` mapping. Each concrete registry declares its model, its error class, its reference query, and its noun for message templating. Collapses roughly 200 lines and makes a third registry nearly free.
- **Effort:** M
#### [MED-12] 38 hand-written "not found" raises; the helper that solves it exists and is used once
- **Location:** `src/transcription/services/people.py` (15 sites), `sources.py` (14), `documents.py` (9); helper at `documents.py:123-132`
- **Problem & Consequence:** The pattern `entity = await session.get(Model, id)` / `if entity is None: raise <Error>(f"... {id} not found", category=ErrorCategory.NOT_FOUND, suggestion=...)` is written out longhand 38 times across the service layer, roughly 150 lines. `DocumentService._get_document_or_raise` (`documents.py:123-132`) already implements exactly this — but it is called from only one site (`documents.py:533`), while the identical block is still hand-written at `documents.py:174`, `210`, and `291` in the same file. The abstraction was created and then not adopted, which is the worst of both outcomes: the maintenance burden of a helper plus the drift risk of copies.
- **Recommendation:** Promote the helper to `ServiceBase` and adopt it everywhere.
```python
# services/base.py
async def _get_or_raise[T](
self, session: AsyncSession, model: type[T], entity_id: UUID, *,
error: type[AppError], noun: str, suggestion: str,
) -> T: ...
```
- **Effort:** M
#### [MED-13] Three parallel media-storage implementations
- **Location:** `src/transcription/services/store.py:319-379`; `src/transcription/services/people.py:596-631`; `src/transcription/ui/homepage_store.py:31-44`
- **Problem & Consequence:** `store_source_file`, `store_person_portrait`, and the homepage image writer each independently perform: empty-content check → extension allowlist check → `mkdir(parents=True, exist_ok=True)` → `write_bytes` → wrap `OSError` in a domain error → log. They differ in which of those steps they actually do, so the guarantees are inconsistent — only one of the three hashes its content. All three also block the event loop ([MED-01]).
- **Recommendation:** Consolidate into `services/media_storage.py` per §4, wrapping the write in `asyncio.to_thread`. Resolves this finding and [MED-01] together.
- **Effort:** M
#### [MED-14] `SourceService` owns four domain models, violating the project's own service rule
- **Location:** `src/transcription/services/sources.py` (1254 lines)
- **Problem & Consequence:** `.github/instructions/services.instructions.md:12` states "1 service class per data model." `SourceService` owns `Source`, `JobSource`, `ExecutionAttempt`, and `ProcessingArtifact`:
| Responsibility | Lines |
| :--- | :--- |
| Source CRUD, navigation, listing | 157-345 |
| JobSource association CRUD | 347-510 |
| Evidence write (`update_job_source_transcription`) | 511-670 |
| Attempt promotion and listing | 672-725 |
| Artifact storage (JSON, binary, external, verify) | 727-992 |
| Evidence export | 994-1091 |
| Revisions | 1093-1141 |
The clearest symptom is `update_job_source_transcription` — 160 lines, 17 keyword parameters, mutating five models in one call. The same instruction file (line 13) says an operation spanning more than one service "needs to have a separate orchestration function"; this method *is* that orchestration function, living inside a service. The size also made `sources.py` an import hub: `documents.py:24` and `store.py:24-25` both import from it, and `documents.py:24` importing `source_mime_type` violates the "services are completely independent" rule at line 13.
- **Recommendation:** Extract `ExecutionAttempt` and `ProcessingArtifact` into their own services and relocate `update_job_source_transcription` to `workflows.py` as orchestration. Keep `Source` and `JobSource` together — they are written in the same transaction on every path, and separating them would add ceremony without benefit. Move `source_mime_type` to a shared module so `documents.py` no longer imports a sibling service.
- **Deferred to V4.7.** This touches the transcription write path and is too large to absorb alongside the V4.6 schema re-level.
- **Effort:** L
### Low Severity
#### [LOW-01] `ruff check` fails on 6 issues, 5 auto-fixable
@@ -255,7 +371,16 @@
#### [LOW-07] `people_page.py:504` catches `Exception` and discards it entirely
- **Location:** `ui/pages/people_page.py:504`
- **Problem:** Unlike every sibling handler, this one shows a generic message without routing through `error_presenter.show_error`, so the user gets no `error_id` to report.
- **Effort:** S
#### [LOW-08] Four avoidable query inefficiencies in `sources.py`
- **Location:** `src/transcription/services/sources.py:233-244, 338-343, 961, 1012-1013`
- **Problem & Consequence:**
- `list_sources_detail:338-343` filters by `job_id` **in Python**, after loading every `Source` row and its eager graph, instead of joining `JobSource` in SQL. Cost grows with the whole table rather than with the result set.
- `read_source_navigation:233-244` fetches the complete ordered id list for a document to identify two neighbours. Two `LIMIT 1` queries (`page_number < n ORDER BY page_number DESC`, and the mirror) return the same answer at constant cost.
- `list_processing_artifacts:961` has no `limit` parameter while its sibling `list_processing_artifact_summaries:980` does, and it loads `inline_payload` blobs that the caller frequently does not need.
- `build_evidence_export:1012-1013` re-reads and re-hashes every external artifact file synchronously on the event loop before serializing. Integrity verification is correct to perform, but it belongs in `asyncio.to_thread` ([MED-01]).
- **Recommendation:** Push the `job_id` filter into SQL, replace the navigation scan with two bounded queries, add a `limit` to `list_processing_artifacts`, and move artifact hashing off the loop.
- **Effort:** S
---
@@ -303,7 +428,10 @@ Encapsulation is good — no OpenRouter-specific header, model name, or payload
| `ServiceBundle` construction block (4 identical service instantiations) | `app.py:45-50`; `worker.py:160-165`; `services/__init__.py:19-22` | `ServiceBundle.from_session_factory(...)` classmethod | ~20 |
| "Next queued job" query, two divergent implementations | `services/jobs.py:170-187` (no `LIMIT`); `db/operations.py:143-151` (has `LIMIT`) | `JobService.claim_next_queued_job` (per [CRIT-01]); delete the `operations.py` copy | ~12 |
| `build_prompt_execution` re-export shim + legacy aliases | `services/transcription.py` (whole module); `services/store.py:35,382,383` | `services/sources.py` (single import path) | ~45 |
| `store_source_file` / `store_person_portrait` / `store_homepage_image` — three near-identical validate-hash-write-bytes flows | `services/store.py:319-379`; `services/people.py:~610-630`; `ui/homepage_store.py:31-44` | `services/media_storage.py` (one async, `to_thread`-wrapped writer) | ~60 |
| `store_source_file` / `store_person_portrait` / `store_homepage_image` — three near-identical validate-hash-write-bytes flows | `services/store.py:319-379`; `services/people.py:596-631`; `ui/homepage_store.py:31-44` | `services/media_storage.py` (one async, `to_thread`-wrapped writer) | ~60 |
| Registry CRUD (list / summaries / create / read / update / delete / referenced) for `DocumentType` and `PersonRole` | `services/documents.py:350-500`; `services/people.py:214-378` | `services/registry.py:RegistryService[ModelT]` ([MED-11]) | ~200 |
| Label normalization + casefold key + summary dataclass | `services/documents.py:49-72`; `services/people.py:49-79` | `services/registry.py` (base) | ~35 |
| `get(...)` → `if None: raise ...NOT_FOUND` guard, written longhand 38 times | `services/people.py` (15), `sources.py` (14), `documents.py` (9) | `ServiceBase._get_or_raise` ([MED-12]) | ~150 |
### Proposed Canonical Abstractions
@@ -323,6 +451,27 @@ def from_session_factory(cls, factory: SessionFactory, settings: Settings | None
async def claim_next_queued_job(self, *, session: AsyncSession | None = None) -> Job | None: ...
# atomic QUEUED -> PROCESSING with LIMIT 1 + FOR UPDATE SKIP LOCKED
# src/transcription/services/registry.py
class RegistryService[ModelT: RegistryModel](ServiceBase):
"""Shared CRUD for semantic-key registries (DocumentType, PersonRole)."""
model: type[ModelT]
error: type[AppError]
noun: str
async def list_all(self, *, active_only: bool = True, session=None) -> Sequence[ModelT]: ...
async def list_summaries(self, *, session=None) -> Sequence[RegistrySummary]: ...
async def create(self, *, label: str, is_active: bool = True, session=None) -> ModelT: ...
async def read(self, entity_id: UUID, *, session=None) -> ModelT: ...
async def update(self, entity_id: UUID, *, label: str, is_active: bool, session=None) -> ModelT: ...
async def delete(self, entity_id: UUID, *, session=None) -> None: ...
async def is_referenced(self, entity_id: UUID, *, session=None) -> bool: ...
def _reference_query(self, entity: ModelT) -> Select[tuple[UUID]]: ... # subclass hook
# src/transcription/services/base.py
async def _get_or_raise[T](
self, session: AsyncSession, model: type[T], entity_id: UUID, *,
error: type[AppError], noun: str, suggestion: str,
) -> T: ... # absorbs 38 hand-written not-found blocks — resolves [MED-12]
# src/transcription/ui/components/media_urls.py
def build_upload_url(*, file_path: Path, upload_dir: Path, base_url: str) -> str | None: ...
@@ -340,6 +489,8 @@ def parse_uuid_or_render_error(raw: str, *, entity: str) -> UUID | None: ...
## 5. Prioritized Action Plan
> **Superseded for V4.6.** The three phases below are the original review's sequencing. The V4.6 release restructures this into seven phases against the confirmed operating context in [§1a](#1a-post-review-addendum); see [`ver4.6/implementation_plan_v4_6.md`](ver4.6/implementation_plan_v4_6.md). The material differences are: Alembic is replaced by a schema re-level; the schema-affecting items are merged into a single pass; the service-layer consolidation ([MED-11], [MED-12], [MED-13]) is added; and the `SourceService` split ([MED-14]) is deferred to V4.7.
### Phase 1: Quick Wins (PR 1-2)
1. Delete `src/transcription/app_state.py` — dead module with a live `TypeError` ([HIGH-01]).
2. Remove `le=20.0` from `worker_provider_timeout_seconds`, raise the default, and pass an explicit `httpx.Timeout` to the OpenRouter client ([HIGH-03]).
@@ -353,7 +504,7 @@ def parse_uuid_or_render_error(raw: str, *, entity: str) -> UUID | None: ...
8. Implement `claim_next_queued_job` with `LIMIT 1` + `FOR UPDATE SKIP LOCKED`, delete the `db/operations.py` duplicate, and add a concurrency test that runs two claimers against one queued job ([CRIT-01]).
9. Hoist `ServiceBundle` and the provider client to worker-loop scope so the HTTP connection pool survives across jobs ([HIGH-02], [MED-06]).
10. Wrap blocking media/artifact I/O and Pillow normalization in `asyncio.to_thread` behind a single `services/media_storage.py` ([MED-01]).
11. Adopt Alembic; first revision fixes `preferred_execution_attempt_id` from `CHAR(32)` to a real UUID FK. Add one Postgres-backed integration test job ([HIGH-05]).
11. ~~Adopt Alembic~~ — **superseded**: re-level the schema from current metadata and delete the `_upgrade_*` chain ([HIGH-05]), landing together with [HIGH-04], [HIGH-08], and [CRIT-02] in one pass.
12. Extend `TranscriptionProvider` Protocol to cover `aclose` and the evidence attributes; delete the `inspect.signature` reflection ([MED-03]).
### Phase 3: Consolidation & Refactoring (PR 5-6)