diff --git a/docs/architecture_code_review_2026-08-17.md b/docs/architecture_code_review_2026-08-17.md index ad16ca7..8284abe 100644 --- a/docs/architecture_code_review_2026-08-17.md +++ b/docs/architecture_code_review_2026-08-17.md @@ -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 (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) diff --git a/docs/ver4.6/implementation_plan_v4_6.md b/docs/ver4.6/implementation_plan_v4_6.md new file mode 100644 index 0000000..f4c48f9 --- /dev/null +++ b/docs/ver4.6/implementation_plan_v4_6.md @@ -0,0 +1,295 @@ +# Implementation Plan (Version 4.6) + +## Goal + +Pay down the defects, duplication, and structural drift identified in the [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md) without changing any observable behavior. Re-level the database schema from current SQLModel metadata, correct read amplification and missing indexes, consolidate duplicated service and UI code, restore the project's own documented boundaries, and make `ty` a real quality gate. + +## Planning Status + +- V4.5 is the completed implementation baseline. +- The V4.6 scope is frozen and sufficiently detailed to begin implementation. +- Every task traces to a review finding ID. A change without a finding ID is a scope addition and requires an explicit amendment. +- The `SourceService` split ([MED-14]) is deferred to V4.7 by decision, not by omission. + +## Planning Constraints + +- **Behavior is preserved exactly.** All 264 pre-existing tests must still pass. A test that must change is evidence the change is not remediation. +- The application targets **SQLite only** in V4.6. PostgreSQL is unblocked but not enabled. +- The deployment is **single user, single process, single worker**. Forward-compatible code is written where cheap and dialect-guarded. +- The schema is re-leveled from metadata. **No Alembic, no revision directory, no history table, no down path.** +- All schema-affecting changes land in **one pass**; partial application is not a valid state. +- The data migration script is authored **last**, against the final schema and final loading strategy. +- Uploaded Source files, portraits, and artifact files on disk are never modified. +- Original Source files remain immutable; every V4.2–V4.5 evidence and provenance contract is preserved. +- Provider network work continues to occur outside database transactions. +- Database, integration, and UI tests use confirmed isolated data and never modify `data/transcription.db`. +- Potentially destructive tests run only through `tools/run_destructive_tests.py`. + +## Expected Project Impact + +| Area | Expected impact | +| --- | --- | +| Dead code | Remove `app_state.py`, `services/transcription.py`, legacy aliases, `ServiceBase.queue`, and a duplicate queued-job query. | +| Persistence | Delete the hand-rolled DDL chain; generate schema from metadata with correct indexes, FK ordering, and loading strategy. | +| Query behavior | Bounded queued-job poll, SQL-side filtering, bounded navigation queries, explicit eager loads. | +| Worker | Provider client and service bundle live for the worker's lifetime rather than per job. | +| Configuration | Remove the timeout cap and the silently-ignored `DATABASE_URL`; resolve dead settings; replace frozen-model mutation. | +| Service layer | Generic registry service, shared not-found guard, single media-storage implementation (~400 lines removed). | +| UI layer | Fix three boundary violations; extract duplicated components (~500 lines removed); externalize the SVG asset. | +| Async I/O | Move filesystem, hashing, and image work off the event loop. | +| Tooling | `ruff` and `ty` both reach zero and gate on pre-commit. | +| Data | One-time migration of backed-up V4.5 data into the re-leveled schema. | +| Tests and documentation | Add index, FK-cycle, claim-boundedness, client-reuse, and registry-parity coverage; correct the stale instruction path. | + +## Implementation Phases + +### 1. Deletions and Quick Wins + +Independent of every other phase. Land first to shrink the surface everything else must consider. + +- Delete `src/transcription/app_state.py` and confirm zero importers remain in `src`, `tests`, and `tools` ([HIGH-01]). +- Delete `src/transcription/services/transcription.py` and standardize every `build_prompt_execution` import on `services/sources.py` ([MED-05]). +- Delete the legacy compatibility aliases in `services/store.py:35,382,383` ([MED-05]). +- Delete `ServiceBase.queue` and its unparameterized `asyncio.Queue` ([MED-07]). +- Delete `db/operations.py:get_next_queued_job` as a divergent duplicate of the live implementation ([CRIT-01]). +- Resolve `sqlite_check_same_thread` and `worker_retry_backoff_seconds`: wire each to real behavior or delete it together with its test ([MED-02]). +- Remove `DATABASE_URL` from `docker-compose.yml` and document the real `DATABASE__DRIVER` / `DATABASE__PATH` nested names in `.env.example` ([MED-10]). +- Correct the stale path in `.github/instructions/services.instructions.md:10` to `src/transcription/db/models.py` ([LOW-02]). +- Remove the discarded `load_docs` parameter from `list_jobs` ([LOW-03]). +- Validate the `getattr` result in `resolve_worker_notifier` ([LOW-04]). +- Move `VIBESCRIBE_LOGO_SVG` to `ui/static/vibescribe_logo.svg` and load it through a `read_svg` sibling of `ui/resources.py:read_css` ([MED-09]). +- Run `ruff check --fix` and resolve the remainder by hand ([LOW-01]). +- Route `people_page.py:504` through `error_presenter.show_error` ([LOW-07]). +- Cancel the auto-refresh timer rather than only deactivating it, and name its interval constant ([LOW-06]). + +**Verification:** full suite green, `ruff check` reports zero, no import of a deleted symbol remains. + +### 2. Schema Re-Level — Single Pass + +This phase is atomic. Every task below regenerates the same schema and must be verified together. + +- Delete `upgrade_schema` and `_upgrade_*` (`db/operations.py:25-109`) and their tests (`tests/test_db.py:109-172`) ([HIGH-05]). +- Confirm `create_all()` remains gated by `Settings.should_bootstrap_schema` (`config.py:140-145`) ([HIGH-05]). +- Declare the composite index in the model: `Index("ix_job_status_date_created", "status", "date_created")`, plus `index=True` on the foreign keys the worker and detail pages filter on ([HIGH-04]). +- Declare `Source.preferred_execution_attempt_id`'s foreign key with `use_alter=True` and an explicit constraint name, breaking the `source` / `job_source` / `execution_attempt` cycle ([HIGH-08]). +- Flip relationship loading from bidirectional `lazy="selectin"` to `lazy="raise"`, model by model ([CRIT-02]): + - Work one model at a time with the suite as the safety net. + - Where a test fails with a lazy-load error, add an explicit `selectinload()` to the *service query* that feeds it — never restore the model-level default. + - Where an existing explicit `selectinload()` proves redundant, delete it; this is the primary source of the ~160 `ty` diagnostics addressed in Phase 6. + - Follow the two correct precedents already in the codebase: `Source.processing_artifacts:279` and `JobSource.execution_attempts:336`. +- Rebuild the development database from empty. Do **not** attempt to upgrade the existing file. + +**Verification:** +- A test asserts the composite `Job` index and the hot foreign-key indexes exist in a freshly created schema. +- A test compiles the metadata against the PostgreSQL dialect and asserts **no** unresolvable-cycle warning is emitted. +- A test asserts `preferred_execution_attempt_id`'s column type matches the model declaration. +- Full suite green under `lazy="raise"`. +- No raw `ALTER TABLE` or `CREATE INDEX` string remains anywhere in `src`. + +**Rollback:** this phase reverts as a unit. A partially applied schema pass is not a valid state. + +### 3. Worker and Provider Reliability + +Depends on Phase 2, because the claim query's cost profile is only correct once eager-loading defaults are fixed. + +- Add `.limit(1)` to the queued-job selection and remove its eager-load options from the hot poll ([CRIT-01]). +- Convert the read-then-write claim into an atomic `QUEUED` → `PROCESSING` transition in one transaction ([CRIT-01]): + - Write the dialect-guarded `with_for_update(skip_locked=True)` branch for the multi-user direction. + - On SQLite, the claim executes as a bounded single-writer transaction. + - Load the eager relationships in a **second** query after the claim succeeds. + - Update the stale comment at `workflows.py:193-194` to describe the actual guarantee rather than the known hazard. +- Hoist `ServiceBundle` and the provider client out of the per-job body in `worker.py:157-174` to worker-loop scope; `aclose()` the client once at loop shutdown, not once per job ([HIGH-02]). +- Add `ServiceBundle.from_session_factory(...)`, replacing the three duplicated instantiation blocks at `app.py:45-50`, `worker.py:160-165`, and `services/__init__.py:19-22`. Have `_recover_stale_processing_jobs` (`app.py:73-84`) use the bundle built five lines earlier ([MED-06]). +- Remove `le=20.0` from `worker_provider_timeout_seconds` (`config.py:110`), raise the default to a realistic vision-transcription duration, and pass an explicit `httpx.Timeout` to the OpenRouter `AsyncClient` (`openrouter.py:198`) ([HIGH-03]). +- Extend the `TranscriptionProvider` Protocol to declare `aclose` and the evidence attributes; delete the per-call `inspect.signature(adapter.transcribe).parameters` reflection at `sources.py:1237` and the associated untyped kwargs dict ([MED-03]). + +**Verification:** +- A test asserts the emitted claim SQL contains `LIMIT` and no `selectinload` join. +- A test asserts the worker processes two consecutive jobs against the same provider client instance. +- A test asserts a timeout value above 20 seconds is accepted by `Settings`. +- A test asserts the transcription call path resolves `requested_model` through the Protocol without reflection. + +### 4. Service Layer Consolidation + +Depends on Phase 2 only for the loading strategy; otherwise independent of Phase 3. + +- Introduce `services/registry.py` with a generic `RegistryService[ModelT]` owning list, summaries with counts, create with `IntegrityError` → conflict mapping, read with not-found, update, delete with built-in and referenced guards, and `is_referenced` ([MED-11]): + - Define label normalization, the casefold key, and the summary shape once. + - Reduce `DocumentService`'s document-type methods (`documents.py:350-500`) and `PeopleService`'s person-role methods (`people.py:214-378`) to subclasses declaring model, error class, reference query, and noun. + - Preserve every existing user-facing message, error category, and suggestion string verbatim; template the noun only. +- Add `ServiceBase._get_or_raise(...)` and adopt it at all 38 not-found sites, including `documents.py:174,210,291`, which currently bypass the local `_get_document_or_raise` helper. Delete the now-redundant local helper ([MED-12]). +- Introduce `services/media_storage.py` as the single validate → hash → `mkdir` → write → wrap-`OSError` implementation, replacing `store.py:319-379`, `people.py:596-631`, and `ui/homepage_store.py:31-44`. Wrap the write in `asyncio.to_thread` ([MED-13], [MED-01]). +- Move `source_mime_type` out of `services/sources.py` into a shared module so `documents.py:24` no longer imports a sibling service, restoring the independence rule at `services.instructions.md:13` ([MED-14], partial). +- Correct the four query inefficiencies in `sources.py` ([LOW-08]): + - `list_sources_detail:338-343` — move the `job_id` filter from Python into a SQL join on `JobSource`. + - `read_source_navigation:233-244` — replace the full ordered-id scan with two `LIMIT 1` queries. + - `list_processing_artifacts:961` — add a `limit` parameter matching its summary sibling. + - `build_evidence_export:1012-1013` — move artifact integrity hashing into `asyncio.to_thread`. + +**Verification:** +- Existing `DocumentType` and `PersonRole` tests pass **unchanged** against the shared implementation. This is the primary proof that behavior is preserved. +- A test asserts `list_sources_detail` filtered by `job_id` emits a join rather than loading the full table. +- No module in `services/` imports another concrete service module. + +### 5. UI Boundaries and Duplication + +Independent of Phases 2–4 except where a service signature changes. + +- Fix the three `ui.instructions.md` violations ([HIGH-07]): + - Add a `JobService` or workflow method that owns `session_scope` internally; remove the import and transaction management from `jobs_page.py:17,185-192`. + - Have `SourceService` return a plain `transport_body_deferred: bool` on a read model; remove `sqlalchemy.inspect` from `sources_page.py:13,439`. + - Pass a ready media URL into `document_panzoom`, or delete the component — it is exported from `components/__init__.py` but used by no page ([HIGH-07]). +- Extract the duplication catalogued in review §4, highest value first: + - `ui/components/confirm_delete.py` — the blocked-deps card plus confirm/cancel row, from four pages (~120 lines). + - `ui/components/media_urls.py` — pure upload-URL resolution taking `upload_dir` and `base_url`, from three call sites (~110 lines). + - `ui/components/guards.py` — parse → error label → return, from nine call sites (~90 lines). + - `build_table` adoption for the remaining hand-rolled `ui.table` instances, adding selection and no-search options as needed (~70 lines). + - `ui/components/upload_panel.py` — file-picker wiring, from three pages (~50 lines). + - `ui/components/formatters.py` — `_parse_uuid` (five copies) and `_parse_iso_date` (two copies) (~49 lines). + - A shared page-helper for `_resolve_runtime_settings(request)` (three copies, ~18 lines). +- Annotate untyped handler parameters and replace loosely-typed dict returns with read models ([LOW-05]). + +**Verification:** UI page tests pass unchanged; no page module imports `session_scope`, `sqlalchemy.inspect`, or `get_settings`. + +### 6. Async I/O and Configuration Hygiene + +- Wrap the remaining blocking work in `asyncio.to_thread`: Pillow orientation normalization, artifact writes, and evidence hashing not already covered by Phase 4 ([MED-01]). +- Replace `functools.cache` on the engine and session factories with an explicit URL-keyed registry supporting targeted eviction, removing the cross-test and cross-tenant coupling and restoring a visible call signature ([MED-04]). +- Replace `object.__setattr__` in `normalize_provider_models` (`config.py:130,137`) with `model_copy(update=...)` or a computed property. +- Add `onupdate` to the `updated_at` / `date_updated` columns that are expected to track modification, so they stop being stale on the update paths that do not set them by hand. Remove the now-redundant manual assignment at `jobs.py:166` and its siblings. +- Surface the exception currently swallowed to `None` in the ORM model property at `models.py:227` ([MED-08]). + +**Note:** the `onupdate` change is schema-affecting in principle but not in emitted DDL, since `onupdate` is a Python-side default. If implementation reveals it alters generated DDL, it moves into Phase 2 and Phase 2 is re-verified. + +**Verification:** a test asserts an update through a service advances `updated_at`; a test asserts two different database URLs produce two distinct engines and that evicting one leaves the other intact. + +### 7. Type Checking and Tooling Gate + +Depends on Phase 2, which is expected to remove most diagnostics by deleting redundant eager loads. + +- Re-baseline `ty check` after Phase 2 and measure the remaining diagnostic count ([HIGH-06]). +- Convert every surviving `# pyright: ignore[...]` to `# ty: ignore[...]`, since `ty` does not honor pyright directives ([HIGH-06]). +- Fix the two real bugs currently hidden in the noise ([HIGH-06]): + - `tests/ui/test_sources_page.py:25` constructs `Source(...)` without the required `document_id`. + - `tools/run_destructive_tests.py:76,80` uses `fcntl`, which does not exist on Windows; use a cross-platform lock or guard by platform. +- Drive `ty check` to zero diagnostics and wire it into the existing pre-commit setup as a blocking gate. +- Configure `asyncio_default_fixture_loop_scope` explicitly so pytest-asyncio behavior does not change on upgrade. + +**Verification:** `ty check` and `ruff check` both report zero; pre-commit fails when either regresses; `tools/run_destructive_tests.py` runs on Windows. + +### 8. Data Migration + +The final phase. Authored against the completed schema and the completed loading strategy. + +- Write a one-time script under `tools/` that reads the backed-up V4.5 database and writes into the re-leveled schema (review §1a, "Items Added During Scoping"). +- Because `lazy="raise"` is in force, every relationship traversal in the script carries an explicit eager load. This is the reason the script is written last. +- Preserve identity: UUIDs, digests, timestamps, attempt numbers, and `preferred_execution_attempt_id` selections carry across unchanged. +- Do not reinterpret, normalize, or regenerate any `ExecutionAttempt` or `ProcessingArtifact` evidence. +- Do not modify any on-disk Source file, portrait, or artifact file. +- The script is idempotent, is never invoked from application startup, and never runs in the test suite. + +**Verification:** post-migration row counts match the backup for every table (`document` 8, `document_person` 11, `document_type` 7, `execution_attempt` 80, `job` 11, `job_source` 79, `person` 5, `person_role` 3, `processing_artifact` 2, `source` 76); artifact integrity verification passes for every migrated artifact; on-disk file hashes are unchanged. + +## Sequencing Constraint + +```mermaid +graph TD + P1[1. Deletions & Quick Wins] + P2[2. Schema Re-Level
SINGLE ATOMIC PASS] + P3[3. Worker & Provider] + P4[4. Service Consolidation] + P5[5. UI Boundaries & Duplication] + P6[6. Async I/O & Config] + P7[7. Type-Check Gate] + P8[8. Data Migration] + + P1 --> P2 + P2 --> P3 + P2 --> P4 + P2 --> P7 + P1 --> P5 + P4 --> P5 + P4 --> P6 + P3 --> P8 + P5 --> P8 + P6 --> P8 + P7 --> P8 +``` + +The binding constraints are: + +1. **Phase 2 is indivisible.** `create_all` from metadata, the indexes, `use_alter`, and the `lazy` flip all regenerate the same schema. They land together or not at all. +2. **Phase 7 follows Phase 2.** Measuring the `ty` baseline before the redundant eager loads are deleted would chase diagnostics that Phase 2 removes for free. +3. **Phase 8 is last.** The migration script must be written against the final schema and the final loading strategy. + +## Test Strategy + +- **The existing suite is the contract.** 264 tests pass today and must pass at every phase boundary. A test that requires modification is treated as a defect in that test, justified individually in the commit, and never as license to change behavior. +- **Registry parity is the key proof.** The `DocumentType` and `PersonRole` tests must pass *unchanged* against the shared `RegistryService`. If they need edits, the abstraction is wrong. +- **New tests are structural, not behavioral.** They assert schema shape, emitted SQL, dialect compatibility, and object lifetime — properties the current suite does not cover and that the review found were the reason these defects survived. +- New coverage to add: + + | Assertion | Finding | + | :--- | :--- | + | Composite `Job` index and hot FK indexes exist in a fresh schema | [HIGH-04] | + | PostgreSQL-dialect metadata compilation emits no cycle warning | [HIGH-08] | + | `preferred_execution_attempt_id` column type matches the model | [HIGH-05] | + | Full suite passes under `lazy="raise"` | [CRIT-02] | + | Claim SQL contains `LIMIT` and no eager-load join | [CRIT-01] | + | Provider client instance is reused across two consecutive jobs | [HIGH-02] | + | `Settings` accepts a provider timeout above 20 seconds | [HIGH-03] | + | `list_sources_detail` emits a join rather than a full-table load | [LOW-08] | + | An update through a service advances `updated_at` | [SQLModel §3] | + | Distinct database URLs yield distinct, individually evictable engines | [MED-04] | + | Post-migration row counts match the backup | [Phase 8] | + +- All database, integration, and UI tests continue to use isolated data and never touch `data/transcription.db`. +- Destructive tests continue to run only through `tools/run_destructive_tests.py`, which must first be made to run on Windows. + +## Risks + +| Risk | Likelihood | Impact | Mitigation | +| :--- | :--- | :--- | :--- | +| The `lazy="raise"` flip surfaces load paths the tests do not cover, breaking a UI page at runtime | High | Medium | Flip one model at a time; exercise every page manually at the phase boundary; `lazy="raise"` fails loudly rather than silently, which is the point | +| Phase 2 is partially applied and leaves an inconsistent schema | Medium | High | Treat Phase 2 as one commit; rebuild from empty rather than upgrading; verify all four schema assertions before proceeding | +| `RegistryService` generalization subtly changes a user-facing message or error category | Medium | Medium | Preserve message strings verbatim, templating only the noun; require the existing registry tests to pass unchanged | +| The atomic claim behaves differently on SQLite than the `FOR UPDATE SKIP LOCKED` path it is written to support | Medium | Low | Single worker in V4.6 means the SQLite path is the only one exercised; the Postgres branch is dialect-guarded and explicitly unverified until the cutover | +| Removing the timeout cap allows a pathological hang | Low | Medium | Pair the removal with an explicit `httpx.Timeout` so the client, not the config bound, enforces the ceiling | +| The migration script loses or reinterprets evidence | Low | High | Verify row counts per table, verify artifact integrity hashes post-migration, and never touch on-disk files | +| Remediation quietly becomes feature work | Medium | Medium | Every commit cites a finding ID; anything without one is recorded for a later revision | +| `ty` cannot reach zero without unsound suppressions | Medium | Low | Suppressions are acceptable where SQLModel typing is genuinely unrepresentable, but each must be `# ty: ignore[]` with a specific rule, never blanket | + +## Delivery Order + +1. Phase 1 — Deletions and Quick Wins +2. Phase 2 — Schema Re-Level (single atomic pass) +3. Phase 3 — Worker and Provider Reliability +4. Phase 4 — Service Layer Consolidation +5. Phase 5 — UI Boundaries and Duplication +6. Phase 6 — Async I/O and Configuration Hygiene +7. Phase 7 — Type Checking and Tooling Gate +8. Phase 8 — Data Migration + +## Done Criteria + +V4.6 is complete when every acceptance criterion in the [V4.6 Scope Boundary](scope_boundary_v4_6.md) is satisfied, specifically: + +- All 264 pre-existing tests pass, with every modified test individually justified. +- `ruff check` and `ty check` both report zero and gate on pre-commit. +- No hand-rolled DDL, dead module, dead setting, or duplicate implementation identified in the review remains. +- The schema is generated from metadata, correctly indexed, cycle-free under the PostgreSQL dialect, and free of bidirectional `lazy="selectin"`. +- Roughly 900 lines of duplication are removed across the service and UI layers. +- The backed-up V4.5 data is restored into the re-leveled schema with matching row counts and unmodified on-disk files. +- No new user-facing feature exists that did not exist in V4.5. + +## Related Local References + +- [V4.6 Scope Boundary](scope_boundary_v4_6.md) +- [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md) +- [V4.5 Scope Boundary](../ver4.5/scope_boundary_v4_5.md) +- [V4.5 Implementation Plan](../ver4.5/implementation_plan_v4_5.md) +- [V4.2 Evidence and Provenance Scope](../ver4.2/scope_boundary_v4_2.md) +- [V4 Architecture](../ver4/architecture_v4.md) +- [V4 Schema](../ver4/schema_v4.md) +- [Transcription Methodology](../invariant/transcription_methodology.md) +- [AI Evidence and Provenance Invariant](../invariant/ai_evidence_and_provenance.md) diff --git a/docs/ver4.6/scope_boundary_v4_6.md b/docs/ver4.6/scope_boundary_v4_6.md new file mode 100644 index 0000000..3a8e6b7 --- /dev/null +++ b/docs/ver4.6/scope_boundary_v4_6.md @@ -0,0 +1,205 @@ +# V4.6 Scope Boundary + +This document defines the frozen boundary for V4.6, a **pure remediation release**. V4 through V4.5 remain the architecture and behavioral baseline. V4.6 introduces **no new user-facing features**; it pays down the defects, duplication, and structural drift catalogued in [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md). + +Every item in scope is traceable to a review finding ID. Any change that cannot be traced to a finding ID is out of scope. + +## Purpose + +- Remove dead code, dead configuration, and duplicate implementations that create maintenance drift. +- Re-level the database schema from current SQLModel metadata, ending hand-rolled DDL while the schema is still pre-production. +- Correct read amplification, missing indexes, and query patterns that scale with table size rather than result size. +- Restore the boundaries the project already wrote down in `.github/instructions/services.instructions.md` and `ui.instructions.md`. +- Make `ty` usable as a real quality gate. +- Preserve every existing behavior, evidence guarantee, and provenance contract established in V4 through V4.5. + +## Confirmed Operating Context + +These answers are frozen for V4.6 and govern every decision below. + +| Question | Answer | +| :--- | :--- | +| Database | **SQLite only.** PostgreSQL remains the intended destination but is deferred beyond V4.6. `JSONBCompat` and the Postgres drivers are retained. | +| Topology | **Single user, single process, single worker.** A multi-user server is the stated direction, so forward-compatibility work is retained where it is cheap. | +| Schema evolution | **Re-level from current metadata.** No Alembic, no migration framework, no `_upgrade_*` chain. | +| Existing data | The development database is rebuilt from scratch during implementation and migrated from backup as the final step. | +| Release character | **Pure remediation.** No new features. | +| Scope band | Critical through Low, inclusive. | + +## In Scope + +### 1. Dead Code and Dead Configuration Removal + +- `src/transcription/app_state.py` is deleted. It has zero importers and contains a guaranteed `TypeError` ([HIGH-01]). +- `src/transcription/services/transcription.py` is deleted; `build_prompt_execution` has exactly one import path ([MED-05]). +- The legacy compatibility aliases in `services/store.py` are deleted ([MED-05]). +- `ServiceBase.queue` is deleted; no service allocates an unused `asyncio.Queue` ([MED-07]). +- `sqlite_check_same_thread` and `worker_retry_backoff_seconds` are either wired to real behavior or deleted, along with their tests ([MED-02]). +- `db/operations.py:get_next_queued_job` is deleted as a divergent duplicate ([CRIT-01]). +- `DATABASE_URL` is removed from `docker-compose.yml`, and the real nested `DATABASE__*` names are documented. The application never silently ignores a database configuration variable ([MED-10]). +- `document_panzoom` is either fixed or deleted; it is exported but referenced by no page ([HIGH-07]). + +### 2. Schema Re-Level + +The following changes are schema-affecting and land as **one single pass** against a database rebuilt from empty. + +- `upgrade_schema` and the three `_upgrade_*` functions (`db/operations.py:25-109`) are deleted, along with their tests (`tests/test_db.py:109-172`) ([HIGH-05]). +- The schema is generated exclusively from SQLModel metadata via `create_all()`, gated by the existing `Settings.should_bootstrap_schema` ([HIGH-05]). +- The hand-written `CHAR(32)` column for `preferred_execution_attempt_id` ceases to exist; the column type is whatever the model declares ([HIGH-05]). +- A composite index on `Job.status, Job.date_created` is declared in the model, plus `index=True` on the foreign keys the worker and detail pages filter on ([HIGH-04]). +- `Source.preferred_execution_attempt_id` declares its foreign key with `use_alter=True`, resolving the `source` / `job_source` / `execution_attempt` cycle so `create_all` will succeed on PostgreSQL when that cutover is taken ([HIGH-08]). +- Relationship loading defaults change from bidirectional `lazy="selectin"` to `lazy="raise"`, with per-query `selectinload()` retained or added where a load path genuinely requires it ([CRIT-02]). + +No migration script runs against a populated database. No history table, revision directory, or down path is introduced. + +### 3. Data Migration + +- A one-time script under `tools/` migrates the user's backed-up V4.5 data into the re-leveled schema. +- The script is authored **after** the `lazy="raise"` flip is complete, so that every relationship it traverses carries an explicit eager load. +- The script is idempotent, is never invoked automatically at startup, and never runs as part of the test suite. +- Uploaded Source files, portraits, and artifact files on disk are preserved unchanged; only database rows are rewritten. +- This is the **final** step of V4.6. + +### 4. Worker and Provider Reliability + +- `read_next_queued_job` gains `LIMIT 1` and stops materializing the entire queue plus its eager graph on every poll ([CRIT-01]). +- The claim becomes an atomic `QUEUED` → `PROCESSING` transition. On SQLite this is a bounded single-writer transaction; the `FOR UPDATE SKIP LOCKED` path is written and dialect-guarded for the multi-user direction but is not exercised in V4.6 ([CRIT-01]). +- Eager relationships are loaded in a second query after the claim succeeds, keeping the hot poll a single narrow row ([CRIT-01]). +- `ServiceBundle` and the provider client are hoisted to worker-loop scope so the HTTP connection pool and TLS session survive across jobs ([HIGH-02], [MED-06]). +- `ServiceBundle` gains a `from_session_factory` constructor, replacing three duplicated instantiation blocks ([MED-06]). +- The `le=20.0` cap on `worker_provider_timeout_seconds` is removed, the default is raised, and an explicit `httpx.Timeout` is passed to the OpenRouter client ([HIGH-03]). +- The `TranscriptionProvider` Protocol is extended to cover `aclose` and the evidence attributes; the per-call `inspect.signature` reflection at `sources.py:1237` is deleted ([MED-03]). + +### 5. Service Layer Consolidation + +- A generic `RegistryService[ModelT]` owns list, summaries, create, read, update, delete, and reference-check for semantic-key registries. `DocumentType` and `PersonRole` become thin subclasses declaring their model, error class, reference query, and noun ([MED-11]). +- Label normalization, the casefold key, and the registry summary shape are defined once ([MED-11]). +- `ServiceBase` gains `_get_or_raise`, and all 38 hand-written not-found guards adopt it, including the three in `documents.py` that already bypass the local helper ([MED-12]). +- `services/media_storage.py` becomes the single implementation of validate → hash → write → wrap-error, replacing `store_source_file`, `store_person_portrait`, and the homepage image writer ([MED-13]). +- `source_mime_type` moves out of `services/sources.py` to a shared module so `documents.py` no longer imports a sibling service ([MED-14], partial). +- The four query inefficiencies in `sources.py` are corrected: the `job_id` filter moves into SQL, navigation uses two bounded queries, `list_processing_artifacts` gains a `limit`, and artifact re-hashing moves off the event loop ([LOW-08]). + +### 6. Async I/O and Configuration Hygiene + +- Blocking filesystem and CPU work — media writes, artifact writes, integrity hashing, and Pillow orientation normalization — is wrapped in `asyncio.to_thread` ([MED-01]). +- `functools.cache` on the engine and session factories is replaced with an explicit URL-keyed registry supporting targeted eviction ([MED-04]). +- The `object.__setattr__` mutation of a frozen `Settings` model in `normalize_provider_models` is replaced with `model_copy(update=...)` or a computed property ([Pydantic V2 §3]). +- `models.py` timestamp columns that are expected to track modification gain `onupdate`, so `updated_at` and `date_updated` stop being stale on paths that do not set them by hand ([SQLModel §3]). +- The exception swallowed to `None` in an ORM model property is surfaced ([MED-08]). + +### 7. UI Boundary and Duplication + +- The three `ui.instructions.md` violations are corrected ([HIGH-07]): + - `jobs_page.py` no longer imports `session_scope` or manages transactions; a service or workflow method owns the session. + - `sources_page.py` no longer imports `sqlalchemy.inspect`; the service returns a plain `transport_body_deferred` flag on a read model. + - `document_panzoom` no longer calls `get_settings()`; a ready media URL is passed in. +- The duplication catalogued in the review's §4 is extracted, highest value first: `confirm_delete`, `media_urls`, `guards`, `formatters`, `upload_panel`, and the hand-rolled tables that should use `build_table` (~500 lines). +- The 23KB inline SVG moves to `ui/static/` and is loaded through an `importlib.resources` reader alongside the existing `read_css` ([MED-09]). +- `people_page.py:504` routes its error through `error_presenter.show_error` like every sibling handler ([LOW-07]). +- Untyped handler parameters and loosely-typed dict returns are annotated ([LOW-05]). +- The auto-refresh timer is cancelled rather than only deactivated, and its interval becomes a named constant ([LOW-06]). + +### 8. Type Checking and Tooling Gate + +- The codebase standardizes on `ty`. Remaining suppressions are converted from `# pyright: ignore[...]` to `# ty: ignore[...]` ([HIGH-06]). +- The `lazy="raise"` flip in §2 is expected to eliminate most of the ~160 `selectinload` diagnostics by removing redundant eager loads. +- `ty check` reaches zero diagnostics and is wired into the existing pre-commit setup as a gate ([HIGH-06]). +- The two real bugs currently hidden in the diagnostic noise are fixed: `tests/ui/test_sources_page.py:25` constructs `Source(...)` without the required `document_id`, and `tools/run_destructive_tests.py:76,80` uses `fcntl`, which does not exist on the Windows development platform ([HIGH-06]). +- `ruff check` reaches zero errors ([LOW-01]). +- `asyncio_default_fixture_loop_scope` is configured explicitly so pytest-asyncio behavior does not change on upgrade ([Testing §3]). +- The stale path in `.github/instructions/services.instructions.md:10` is corrected to `src/transcription/db/models.py` ([LOW-02]). +- `list_jobs` stops accepting and discarding `load_docs` ([LOW-03]). +- `resolve_worker_notifier` validates its `getattr` result ([LOW-04]). + +## Out of Scope + +- Any new user-facing feature, page, action, or field. +- PostgreSQL enablement, Postgres-backed CI, or a Postgres cutover. The `use_alter` fix unblocks it; it does not perform it. +- Alembic or any migration framework, revision directory, history table, or down path. +- Multi-worker or multi-process execution. Forward-compatible code paths are written but not enabled or exercised. +- Concurrency limits, backpressure, or parallel job processing. Jobs remain strictly serial. +- **Splitting `SourceService` into per-model services and relocating `update_job_source_transcription` to `workflows.py` ([MED-14]). Deferred to V4.7.** It touches the transcription write path and cannot safely share a release with the schema re-level. +- Any change to transcription prompt content, medium markers, quality-warning rules, or the retranscription workflow established in V4.5. +- Any change to the evidence, provenance, or immutability contracts established in V4.2 through V4.5. +- Deleting, rewriting, or reinterpreting existing `ExecutionAttempt` or `ProcessingArtifact` evidence during data migration. +- Rewriting the UI table architecture, theme system, or CSS conventions beyond removing duplication. +- Performance work not traceable to a review finding. + +## Locked Design Decisions + +### A. Remediation Only + +Every change traces to a review finding ID. A desirable improvement discovered during implementation that has no finding ID is recorded for a later revision rather than absorbed. + +### B. Re-Level, Do Not Migrate + +The schema is pre-production and the data is disposable and backed up. Deleting the hand-rolled upgrade chain and regenerating from metadata is correct precisely because this window will not exist again. A migration framework is the right answer once the schema stabilizes, and V4.6 deliberately does not pretend that moment has arrived. + +### C. One Schema Pass + +The re-level, the indexes, the `use_alter` fix, and the `lazy="raise"` flip all regenerate the same schema. They land together, are verified together, and are reverted together if verification fails. Partial application is not a valid state. + +### D. Data Migration Is Last + +The migration script is written against the final schema and the final loading strategy. Writing it earlier guarantees rework and risks it carrying implicit lazy loads that `lazy="raise"` will later reject. + +### E. Forward Compatibility Where It Is Cheap + +Single-process operation makes the atomic job claim non-urgent, not wrong. Where the correct multi-user implementation costs little more than the single-user one, V4.6 writes the correct one and guards it by dialect. Where it costs substantially more, V4.6 defers it and documents the assumption. + +### F. Behavior Is Preserved Exactly + +A pure-remediation release that changes observable behavior has failed. The existing test suite is the contract: 264 passing tests must still pass, and any test that must change is treated as evidence that the change is not remediation. + +### G. The Instruction Files Are the Standard + +Most findings are deviations from rules the project already wrote down. V4.6 restores conformance to `services.instructions.md` and `ui.instructions.md` rather than inventing new conventions — except where a rule is itself wrong, in which case the rule is corrected explicitly. + +## Acceptance Criteria + +1. `app_state.py`, `services/transcription.py`, the `store.py` aliases, `ServiceBase.queue`, and `db/operations.py:get_next_queued_job` no longer exist, and the full suite passes without them. +2. `upgrade_schema` and the three `_upgrade_*` functions no longer exist; no raw `ALTER TABLE` or `CREATE INDEX` string appears in `src`. +3. A database created from empty by `create_all()` contains the composite `Job` index, indexed hot foreign keys, and a `preferred_execution_attempt_id` column whose type matches the model declaration. +4. Compiling the metadata against the PostgreSQL dialect emits **no** unresolvable-cycle warning. +5. No `Relationship` in `db/models.py` uses `lazy="selectin"` as a bidirectional default; every load path that requires eager loading declares it per query, and the suite passes under `lazy="raise"`. +6. `read_next_queued_job` returns at most one row and issues no eager-load queries; a test asserts the emitted SQL contains `LIMIT`. +7. The worker processes two consecutive jobs against a single provider client instance; a test asserts the client is not reconstructed between jobs. +8. `worker_provider_timeout_seconds` accepts a value above 20 seconds, and the OpenRouter client receives an explicit `httpx.Timeout`. +9. `inspect.signature` no longer appears in the transcription call path. +10. `DocumentType` and `PersonRole` CRUD is served by one shared implementation; the existing registry tests for both pass unchanged. +11. `ServiceBase._get_or_raise` is the only place a `NOT_FOUND` guard is written for an entity fetched by id. +12. One media-storage implementation serves Source files, portraits, and homepage images, and its write is off the event loop. +13. `list_sources_detail` filters by `job_id` in SQL; `read_source_navigation` issues bounded queries; `list_processing_artifacts` accepts a `limit`. +14. No page imports `session_scope`, `sqlalchemy.inspect`, or `get_settings`. +15. The 23KB SVG literal no longer appears in any `.py` file. +16. `ruff check` reports zero errors. +17. `ty check` reports zero diagnostics and runs as a pre-commit gate. +18. `tools/run_destructive_tests.py` runs on Windows. +19. All 264 pre-existing tests still pass. Any test modified during V4.6 is individually justified as a test defect rather than a behavior change. +20. The migration script restores the backed-up V4.5 data into the re-leveled schema with row counts matching the backup, and no on-disk Source file, portrait, or artifact is modified. +21. No new user-facing feature, page, action, or field exists in V4.6 that did not exist in V4.5. +22. Database, integration, and UI verification uses isolated test data and never modifies `data/transcription.db`. + +## Scope Freeze Gate + +V4.6 is sufficiently frozen to begin implementation: + +- The operating context — SQLite, single process, disposable data — is confirmed and its consequences for severity are resolved. +- The schema strategy is resolved: re-level, no Alembic, one pass, migration last. +- The severity band is resolved: Critical through Low, inclusive. +- The service-layer consolidation set is resolved, and the `SourceService` split is explicitly deferred to V4.7. +- The release character is resolved: pure remediation, no new features. + +Any expansion into PostgreSQL enablement, multi-worker execution, a migration framework, the `SourceService` split, or any new feature requires an explicit V4.6 scope amendment or a later revision. + +## Related Local References + +- [V4.6 Implementation Plan](implementation_plan_v4_6.md) +- [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md) +- [V4.5 Scope Boundary](../ver4.5/scope_boundary_v4_5.md) +- [V4.5 Implementation Plan](../ver4.5/implementation_plan_v4_5.md) +- [V4.2 Evidence and Provenance Scope](../ver4.2/scope_boundary_v4_2.md) +- [V4 Architecture](../ver4/architecture_v4.md) +- [V4 Schema](../ver4/schema_v4.md) +- [Transcription Methodology](../invariant/transcription_methodology.md) +- [AI Evidence and Provenance Invariant](../invariant/ai_evidence_and_provenance.md)