Prep for GPT-5.3-codex architecture & code review.

2026-08-19 14:25:42 -05:00
parent edcfba9cb2
commit 2c26177d0c
5 changed files with 6 additions and 553 deletions
@@ -1,547 +0,0 @@
 # Architecture & Code Review Report
 > ## Status: Historical Snapshot - Superseded
 >
 > **This document describes the codebase as it stood on 2026-08-17, before the V4.6 remediation release.** It is retained because it is the canonical registry of the finding IDs (`CRIT-01`, `HIGH-06`, `MED-14`, and so on) cited throughout the V4.6, V4.7, and V4.8 planning documents. Six documents reference it; deleting it would orphan all 32 finding IDs.
 >
 > **Do not read it as a description of current state.** Every file path, line number, and metric below is pre-V4.6 and most are now wrong. The baseline figures in particular are stale: `ruff check` is clean, `ty check` reports 0 diagnostics, and the suite is at 292 passed / 4 skipped.
 >
 > **Disposition of all 32 findings:**
 >
 > | Status | Findings |
 > | :--- | :--- |
 > | Addressed in V4.6 | All 32 were dispositioned - fixed, consciously accepted, or explicitly deferred. See [V4.6 scope boundary](ver4.6/scope_boundary_v4_6.md) and [V4.6 implementation plan](ver4.6/implementation_plan_v4_6.md). |
 > | Carried into V4.7 | `MED-14` (SourceService decomposition) and `HIGH-06` (CI enforcement of the quality gate). See [V4.7 scope boundary](ver4.7/scope_boundary_v4_7.md). |
 >
 > **Where later thinking supersedes this report:** the [V4.6 review log](ver4.6/review_log_v4_6.md) records the decisions, deviations, and revisions made during implementation. Where this report and a committed planning document disagree, **the planning document wins**.
 >
 > Two recommendations here were later revised on evidence. `MED-14` proposed extracting a `services/artifacts.py`; V4.7 cancels that in favour of deleting the `ProcessingArtifact` subsystem outright. The report also treats `job_source` and `execution_attempt` as complementary; measurement showed their evidence columns are fully duplicated.
 **Repository Target:** `C:\GitHub\transcription\`
 **Target Stack:** Python 3.12+ | FastAPI | NiceGUI | SQLModel/SQLAlchemy | Pydantic V2 | asyncio | OpenRouter
 **Review Date:** 2026-08-17
 **Baseline verified:** `pytest` → 264 passed, 4 skipped. `ruff check` → 6 errors. `ty check` → 197 diagnostics.
 ---
 ## 1. Executive Summary
 - **The codebase is disciplined and unusually well-structured for its size (~9k src LOC).** Error taxonomy (`errors.py`), evidence capture (`providers/evidence.py`), transaction-ownership helpers (`ServiceBase._finalize`), and CSS/asset discipline in the UI are genuinely strong and should be preserved.
 - **Top risk is the job-claim path.** `JobService.read_next_queued_job` (`services/jobs.py:170-187`) selects *every* `QUEUED` job with three levels of eager loading, has no `LIMIT`, no `FOR UPDATE SKIP LOCKED`, and no compare-and-swap on the status transition. The code even carries a comment acknowledging the race (`services/workflows.py:193-194`) without fixing it.
 - **Second risk is model-level eager loading.** Nearly every `Relationship` in `db/models.py` sets `lazy="selectin"` on *both* sides of bidirectional links (`Document.jobs` ↔ `Job.document`, `Job.job_sources` ↔ `JobSource.job`, `JobSource.source` ↔ `Source.job_sources`). Reading one `Job` cascades into loading effectively the whole related graph, and it makes every explicit `selectinload()` in the services redundant.
 - **No index exists on `Job.status` or `Job.date_created`**, yet the worker polls `WHERE status='queued' ORDER BY date_created` once per second. Every poll is a full table scan.
 - **`worker_provider_timeout_seconds` is hard-capped at `le=20.0`** (`config.py:110`). Vision transcription of a full document page routinely exceeds 20s; this cap makes systematic timeouts unconfigurable-away.
 - **The provider HTTP client is destroyed and rebuilt for every single job** (`worker.py:157-174`), defeating connection pooling and TLS session reuse on the hottest path.
 - **`app_state.py` is dead code containing a guaranteed `TypeError`** (verified at runtime): `resolve_session_factory` calls `get_session_factory()` with no arguments. `@functools.cache` erases the signature, so `ty` cannot see it.
 - **`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.
 ---
 ## 2. Findings by Severity
 ### 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.
  Worse, the claim is a read-then-write with no atomicity: `process_queued_job` reads status `QUEUED`, then separately calls `mark_job_status(job.id, PROCESSING)`. Two workers (or an app replica plus the in-process worker) can both read the same row as `QUEUED` and both transcribe it — double provider spend and duplicate `ExecutionAttempt` evidence rows. The comment at `workflows.py:193-194` explicitly names this hazard ("otherwise other workers may see the job as still QUEUED") but the committed fix only narrows the window rather than closing it.
  Note also that `db/operations.py:get_next_queued_job` is a second, *different* implementation of the same concept that *does* have `.limit(1)`. The worse implementation is the live one.
 - **Recommendation:** Replace the read-then-write with a single atomic claim, and delete the duplicate.
  ```python
  # Before (services/jobs.py) — no limit, no lock
  query = select(Job).options(...).where(Job.status == JobStatus.QUEUED).order_by(Job.date_created, Job.id)
  return (await _session.exec(query)).first()
  # After — atomic claim, one row, dialect-aware
  async def claim_next_queued_job(self, *, session=None) -> Job | None:
      async with self._session_scope(session) as s:
          stmt = (
              select(Job)
              .where(Job.status == JobStatus.QUEUED)
              .order_by(Job.date_created, Job.id)
              .limit(1)
          )
          if s.bind.dialect.name == "postgresql":
              stmt = stmt.with_for_update(skip_locked=True)
          job = (await s.exec(stmt)).first()
          if job is None:
              return None
          job.status = JobStatus.PROCESSING
          job.date_updated = datetime.now(UTC)
          await self._finalize(session=s, caller_session=session, refresh=(job,))
          return job
  ```
  Load the eager relationships in a *second* query after the claim succeeds, so the hot poll stays a single narrow row. On SQLite, wrap the claim in `BEGIN IMMEDIATE` or accept single-worker-only and document it.
 - **Effort:** M
 #### [CRIT-02] Bidirectional `lazy="selectin"` on every relationship causes cascading read amplification
 - **Location:** `src/transcription/db/models.py:71-73, 89-91, 108-115, 160-168, 211-212, 269-276, 332-333`
 - **Problem & Consequence:** Every `Relationship` in the domain model sets `sa_relationship_kwargs={"lazy": "selectin"}`, including both sides of each pair. Fetching a single `Job` triggers: `Job` → `Job.document` → `Document.jobs` (all jobs for that document) → `Document.sources` → `Document.document_people` → `DocumentPerson.person` / `.role_ref` → each `Job.job_sources` → `JobSource.source` → `Source.job_sources` → … SQLAlchemy's identity map prevents infinite recursion but does **not** prevent the extra SELECT round trips per level.
  Two concrete consequences: (a) the per-second worker poll is far more expensive than it appears from reading `jobs.py`; (b) the dozens of explicit `selectinload(...)` options in `documents.py`, `jobs.py`, `sources.py`, and `people.py` are dead weight — the relationship default already does it — and they are the source of ~160 of the 197 `ty` diagnostics.
 - **Recommendation:** Flip the model default to `lazy="raise"` (or `"noload"`, as already correctly done for `Source.processing_artifacts` at `models.py:279` and `JobSource.execution_attempts` at `models.py:336`) and rely on the per-query `selectinload()` that services already declare. `lazy="raise"` converts silent N+1 into a loud test failure and would prove which eager loads are actually needed.
  ```python
  # models.py
  jobs: list["Job"] = Relationship(back_populates="document", sa_relationship_kwargs={"lazy": "raise"})
  ```
  Roll out per-model with the existing test suite as the safety net; the suite already covers the read paths.
 - **Effort:** M
 ### High Severity
 #### [HIGH-01] `app_state.py` is unreferenced dead code containing a guaranteed `TypeError`
 - **Location:** `src/transcription/app_state.py:29-34`; the called function at `src/transcription/db/session.py:20-26`
 - **Problem & Consequence:** `resolve_session_factory` falls back to `get_session_factory()` with no arguments, but the signature is `get_session_factory(database_url: str)`. Verified at runtime:
  ```
  TypeError: get_session_factory() missing 1 required positional argument: 'database_url'
  ```
  `@functools.cache` wraps the function in a `_lru_cache_wrapper`, which erases the signature — so `ty check src\transcription\app_state.py` reports "All checks passed". The whole module has **zero importers** anywhere in `src`, `tests`, or `tools`, so the bug is currently latent; anyone wiring this helper up hits an immediate crash on the fallback path.
 - **Recommendation:** Delete `app_state.py`. Its three live behaviors already exist elsewhere (`db/session.py:resolve_session_factory`, `db/runtime.py:get_database_runtime`, `worker.py:resolve_worker_notifier`). If retained instead, fix the fallback to `resolve_session_factory()` from `db.session`, and add a typed non-cached wrapper around cached functions so type checkers keep the signature.
 - **Effort:** S
 #### [HIGH-02] Provider HTTP client is rebuilt and torn down once per job
 - **Location:** `src/transcription/worker.py:148-174` (`finally: await services.sources.aclose()`), driven by the tight inner loop at `src/transcription/worker.py:134-142`; client construction at `src/transcription/providers/openrouter.py:197-201`
 - **Problem & Consequence:** `process_next_queued_job` constructs a fresh `ServiceBundle` per call and unconditionally closes the provider in `finally`. Since `workflows.py:243` accesses `services.sources.provider`, a new `httpx.AsyncClient` + `OpenRouter` SDK client is created and destroyed for **every job**. This throws away the connection pool and forces a full TLS handshake per job — added latency on the single most latency-sensitive path, plus churn of file descriptors during backlog drain.
 - **Recommendation:** Hoist the `ServiceBundle` to worker-loop scope (or reuse `app.state.services`, which the lifespan already builds at `app.py:45-50`) and close the provider once at loop shutdown.
  ```python
  # worker.py — before
  async def process_next_queued_job(...):
      services = ServiceBundle(...)
      try: ...
      finally: await services.sources.aclose()
  # after: build once in run_worker_loop / lifespan, pass in, close in the lifespan finally
  async def run_worker_loop(*, services: ServiceBundle, ...):
      try:
          while True: ... await process_next_queued_job(services=services, ...)
      finally:
          await services.sources.aclose()
  ```
 - **Effort:** M
 #### [HIGH-03] Provider timeout is capped at 20 seconds by configuration
 - **Location:** `src/transcription/config.py:110`
 - **Problem & Consequence:** `worker_provider_timeout_seconds: float = Field(default=20.0, gt=0.0, le=20.0)`. The `le=20.0` bound makes 20s both the default *and* the maximum. `workflows.py:238-248` wraps the provider call in `asyncio.wait_for(..., timeout=that_value)`. Multi-modal transcription of a full-page historical document commonly exceeds 20s; operators cannot raise the ceiling without editing source. Every such job fails with `failure_phase="local_timeout"`, and with `worker_max_retries` defaulting to `0` (`config.py:108`) it fails permanently on the first attempt.
  Compounding this, `httpx.AsyncClient(follow_redirects=True)` at `openrouter.py:198` sets no explicit `timeout`, so it inherits httpx's 5-second default for connect/read/write/pool unless the OpenRouter SDK overrides it.
 - **Recommendation:** Remove the `le=20.0` cap (keep `gt=0.0`), raise the default to something realistic (120s), and set an explicit `httpx.Timeout` derived from the same setting so the transport and the `wait_for` agree.
  ```python
  worker_provider_timeout_seconds: float = Field(default=120.0, gt=0.0)
  # openrouter.py
  httpx.AsyncClient(follow_redirects=True, timeout=httpx.Timeout(settings.worker_provider_timeout_seconds))
  ```
 - **Effort:** S
 #### [HIGH-04] No index on the columns the worker polls every second
 - **Location:** `src/transcription/db/models.py:171-212` (`Job.status`, `Job.date_created`, `Job.document_id` all lack `index=True`); also `Source.document_id:252`, `JobSource.job_id:313`, `JobSource.source_id:314`
 - **Problem & Consequence:** The worker executes `WHERE status = 'queued' ORDER BY date_created` once per second (`worker.py:130`, `jobs.py:183-185`). Without a composite index this is a full scan plus sort on every tick, and it grows linearly with total job history — not with queue depth. The `JobSource` foreign keys are joined on every job read; PostgreSQL does not auto-index FKs.
 - **Recommendation:** Add a composite index for the poll and plain indexes on the hot FKs.
  ```python
  class Job(SQLModel, table=True):
      __table_args__ = (Index("ix_job_status_date_created", "status", "date_created"),)
      document_id: UUID = Field(foreign_key="document.id", index=True)
  ```
  Note these must also be added to the hand-rolled upgrade path in `db/operations.py` (see [HIGH-05]).
 - **Effort:** S
 #### [HIGH-05] Hand-rolled schema migrations with SQLite-shaped DDL block the claimed Postgres support
 - **Location:** `src/transcription/db/operations.py:25-109`
 - **Problem & Consequence:** Schema evolution is a chain of `_upgrade_*` functions issuing raw `ALTER TABLE` / `CREATE INDEX IF NOT EXISTS` against whatever database is present, executed inside `create_all()`. Specific defects:
  - `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:** **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`
 - **Problem & Consequence:** The project pins `ty` as its type checker, but the codebase suppresses SQLModel relationship typing with `# pyright: ignore[reportArgumentType]` — a *pyright* directive that `ty` does not honor. Result: `ty check` emits 197 diagnostics (160 `invalid-argument-type`, 18 `unresolved-attribute`, 13 `not-subscriptable`), so nobody can run it as a gate, and genuine errors hide in the noise. Two real bugs are buried in there:
  - `tests/ui/test_sources_page.py:25` — `Source(...)` constructed without the required `document_id`.
  - `tools/run_destructive_tests.py:76,80` — `fcntl` is imported and used, but `fcntl` does not exist on Windows, which is this project's development platform.
 - **Recommendation:** Pick one checker and commit to it. If `ty`: replace `# pyright: ignore[...]` with `# ty: ignore[...]`, or better, eliminate the root cause by adopting [CRIT-02]'s `lazy="raise"` change plus typed column accessors, which removes most `selectinload` diagnostics outright. Then wire `ty check` into pre-commit (`pre-commit` is already a dev dependency at `pyproject.toml:35`).
 - **Effort:** M
 #### [HIGH-07] UI pages own persistence and ORM-loader concerns (violates `ui.instructions.md`)
 - **Location:** `src/transcription/ui/pages/jobs_page.py:17,185-192`; `src/transcription/ui/pages/sources_page.py:13,439`; `src/transcription/ui/components/document_panzoom.py:12,61,65`
 - **Problem & Consequence:** `ui.instructions.md` states pages must not import sessions or manage transactions, and components must not resolve app state. Three violations:
  - `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.
 - **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
 - **Location:** `src/transcription/services/store.py:363`; `src/transcription/services/people.py:623`; `src/transcription/services/sources.py:908-923,941,1297,1354`; `src/transcription/services/normalization.py:52-101`; `src/transcription/services/prompts.py:138,162,173-176`; `src/transcription/ui/homepage_store.py:22,28,41`; `src/transcription/ui/pages/home_page.py:87,92`; `src/transcription/ui/pages/people_page.py:495`
 - **Problem & Consequence:** All media persistence and artifact I/O is synchronous, called from `async def` paths. `_write_external_artifact` (`sources.py:908`) additionally calls `os.fsync()`, which can block for tens of milliseconds. `normalize_orientation` (`normalization.py:52`) runs full Pillow decode/transpose/re-encode at `quality=95, subsampling=0` inline — that is CPU-bound work measured in hundreds of milliseconds for a scanned page. Every one of these stalls the single event loop shared by the FastAPI API, all NiceGUI clients, and the worker.
 - **Recommendation:** Route blocking work through `asyncio.to_thread` at the service boundary (one wrapper per operation, not per call site). For NiceGUI handlers, `nicegui.run.io_bound` / `run.cpu_bound` are the idiomatic equivalents. `normalize_orientation` is the highest-value single conversion.
 - **Effort:** M
 #### [MED-02] Dead configuration surface: three settings are defined and tested but never read
 - **Location:** `src/transcription/config.py:99` (`sqlite_check_same_thread`), `:109` (`worker_retry_backoff_seconds`)
 - **Problem & Consequence:** `sqlite_check_same_thread` is never read — `engine.py:43` hardcodes `{"check_same_thread": False}`. `worker_retry_backoff_seconds` is never read either; `tests/test_config.py:152` asserts its default, which gives false confidence that backoff exists. `services.instructions.md:59` mandates a retry path with backoff, and `workflows.py:159-169` implements the `FAILED → QUEUED` transition, but nothing ever sleeps between attempts. Additionally, `advance_job` is invoked exactly once per `process_next_queued_job` call, so a job that transitions `FAILED → QUEUED` is only retried on a later poll — a documented behavior that reads as accidental.
 - **Recommendation:** Either wire `worker_retry_backoff_seconds` into the retry scheduler (a `next_attempt_at` column filtered in the claim query is the correct shape — sleeping in the worker loop would stall all other jobs) or delete both settings and their tests. Honor `sqlite_check_same_thread` in `engine.py:43` or remove it.
 - **Effort:** S
 #### [MED-03] Runtime `inspect.signature` and `getattr` duck-typing at the provider boundary
 - **Location:** `src/transcription/services/sources.py:1237-1238,1242-1244`; `src/transcription/services/sources.py:152-154`; `src/transcription/services/workflows.py:297-302`
 - **Problem & Consequence:** The `TranscriptionProvider` Protocol (`providers/base.py:102-117`) already declares `requested_model` as a parameter, yet `sources.py:1237` re-checks for it at runtime via `inspect.signature(adapter.transcribe).parameters` on **every transcription call**, then builds an untyped `dict` of kwargs. Similarly, `aclose` and `current_request_manifest` / `current_transport_evidence` are accessed via `getattr(..., None)` even though they are part of the de-facto contract. This defeats static checking on the most important interface in the system, adds per-call reflection overhead, and means a provider that silently drops `requested_model` fails only at runtime.
 - **Recommendation:** Extend the Protocol to declare `aclose()`, `current_request_manifest`, and `current_transport_evidence`; then call `adapter.transcribe(...)` with real keyword arguments and drop the `inspect` import.
  ```python
  class TranscriptionProvider(Protocol):
      current_request_manifest: RequestManifest | None
      current_transport_evidence: TransportEvidence | None
      async def transcribe(self, *, prompt_text: str, ..., requested_model: str | None = None) -> TranscriptionResult: ...
      async def aclose(self) -> None: ...
  ```
 - **Effort:** S
 #### [MED-04] `@cache` on `get_settings(**kwargs)` and on engine/session factories creates cross-test and cross-tenant coupling
 - **Location:** `src/transcription/config.py:148-151`; `src/transcription/db/engine.py:39-55`; `src/transcription/db/session.py:20-26`
 - **Problem & Consequence:** `get_settings(**kwargs: Any)` is `@cache`-decorated with arbitrary keyword arguments — any unhashable value raises `TypeError`, and the cache key is the kwargs tuple, so `get_settings()` and `get_settings(environment="test")` return different singletons. More seriously, `dispose_engine(database_url)` (`engine.py:50-55`) calls `get_engine.cache_clear()`, which evicts **all** cached engines, not just the one being disposed; a multi-database process would silently lose its other engines' pools. The same pattern applies to `dispose_session_factory` (`session.py:48-50`).
 - **Recommendation:** Replace the caches with an explicit registry keyed by URL that supports targeted eviction. `db/runtime.py` already models lifespan-owned resources correctly — extend that pattern rather than layering `functools.cache` beneath it. Separately, drop `**kwargs` from `get_settings` and keep it a true zero-argument singleton.
 - **Effort:** M
 #### [MED-05] Dead compatibility aliases and a three-way import path for one function
 - **Location:** `src/transcription/services/store.py:35,382-383`; `src/transcription/services/transcription.py:12,36`; imports at `store.py:26`, `workflows.py:42`, `sources.py:1263`
 - **Problem & Consequence:** `build_prompt_execution` is defined in `sources.py:1263` and imported through three different paths: `store.py` uses `from .transcription import build_prompt_execution`, `workflows.py` uses `from .sources import ...`, and `tests/test_prompts.py:12` uses a third. `transcription.py` (41 lines) exists solely as a re-export shim. Alongside it, `UploadError = SourceStorageError` (`store.py:35`), `create_upload_job = create_document_job` (`store.py:382`), and `store_file = store_source_file` (`store.py:383`) are aliases with zero remaining callers.
 - **Recommendation:** Delete the three aliases and the `transcription.py` shim; standardize all imports on `services.sources`.
 - **Effort:** S
 #### [MED-06] `ServiceBundle` default factories construct four services against global settings
 - **Location:** `src/transcription/services/__init__.py:15-22`; consumed at `src/transcription/worker.py:157-158`
 - **Problem & Consequence:** `ServiceBundle` declares `field(default_factory=DocumentService)` for all four services. Instantiating `ServiceBundle()` therefore calls `get_settings()` and `resolve_session_factory()` four times, binding to process-global state. `worker.py:157` takes exactly this path whenever `session_factory is None`. This is the "global singleton instead of injected dependency" pattern the FastAPI DI system exists to avoid, and it makes the worker's database target implicit.
 - **Recommendation:** Remove the default factories and require explicit construction, plus a single `ServiceBundle.from_session_factory(factory, settings)` classmethod — which also removes the four-way duplication of the same construction block at `app.py:45-50` and `worker.py:160-165`.
 - **Effort:** S
 #### [MED-07] Unused `asyncio.Queue` allocated in every service instance
 - **Location:** `src/transcription/services/base.py:20,26,30`
 - **Problem & Consequence:** `ServiceBase.__init__` does `self.queue = queue or asyncio.Queue()`. No code anywhere reads `self.queue`. The annotation is the unparameterized `asyncio.Queue`. Constructing an `asyncio.Queue` also binds to the running event loop policy, so building a `ServiceBundle` outside a loop is a latent hazard, and per [MED-06] this happens four times per bundle.
 - **Recommendation:** Delete the `queue` attribute and constructor parameter.
 - **Effort:** S
 #### [MED-08] Exception swallowed to `None` in an ORM model property
 - **Location:** `src/transcription/db/models.py:220-233`
 - **Problem & Consequence:** `Job.filename` reaches into `job_source.__dict__` to dodge lazy loading, then catches `DetachedInstanceError` *and* bare `Exception` (`models.py:227`), returning the string `"unknown"`. Any genuine error — a corrupted row, a mapper misconfiguration — is silently rendered as "unknown" in the UI with no log line. The workaround exists only because of the eager-loading design in [CRIT-02].
 - **Recommendation:** Remove the property from the model and compute the display value in the feature table read model (`ui/components/table/jobs.py`), which is where `ui.instructions.md` says presentation formatting belongs. If it stays, drop the bare `except Exception` and log the `DetachedInstanceError` case.
 - **Effort:** S
 #### [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.
 - **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
 - **Location:** `src/transcription/ui/theme.py:38,40`; `tests/test_app.py:23`; `tests/ui/test_upload_page.py:44`; plus 2 others
 - **Recommendation:** Run `ruff check --fix`; the only non-trivial one is the SVG line, addressed by [MED-09].
 - **Effort:** S
 #### [LOW-02] Stale path reference in project instructions
 - **Location:** `.github/instructions/services.instructions.md:10`
 - **Problem:** Points to `src/transcription/models.py`; the actual location is `src/transcription/db/models.py`.
 - **Effort:** S
 #### [LOW-03] `list_jobs` accepts and discards a parameter
 - **Location:** `src/transcription/services/jobs.py:113-120` (`_ = load_docs`)
 - **Problem:** A dead parameter kept alive only to satisfy `ARG` linting. Callers may believe it changes behavior.
 - **Recommendation:** Remove the parameter and update callers.
 - **Effort:** S
 #### [LOW-04] `resolve_worker_notifier` returns unvalidated `getattr` results
 - **Location:** `src/transcription/worker.py:54-61`
 - **Problem:** Any non-`None` attribute is returned as a `WorkerNotifier` without checking it has `notify`. Compare `app_state.py:15-18`, which correctly uses `isinstance`.
 - **Effort:** S
 #### [LOW-05] Untyped handler parameters and loosely-typed dict returns in UI
 - **Location:** `ui/pages/jobs_page.py:491`; `ui/pages/people_page.py:492`; `ui/pages/home_page.py:85`; `_render_document_form_fields` / `_render_person_form_fields` returning `dict[str, Any]`
 - **Recommendation:** Annotate with `nicegui.events.UploadEventArguments`; replace the form-field dicts with frozen dataclasses.
 - **Effort:** S
 #### [LOW-06] Auto-refresh timer deactivated but never cancelled; magic interval
 - **Location:** `ui/pages/jobs_page.py:245,251,253`
 - **Problem:** `ui.timer(4.0, refresh_job)` is toggled via `.active = False` rather than `.cancel()`; `4.0` is an unnamed literal. Client-scoped, so impact is bounded.
 - **Effort:** S
 #### [LOW-07] `people_page.py:504` catches `Exception` and discards it entirely
 - **Location:** `ui/pages/people_page.py:504`
 - **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
 ---
 ## 3. Stack-Specific Analysis
 ### Python 3.12+ Best Practices
 Modern syntax is used consistently and correctly: `type` statements (`db/session.py:17,45,73,102`), `X | None` unions, `StrEnum`, `match` statements (`db/engine.py:16-31`, `db/session.py:84-92`, `workflows.py:153-171`), frozen `dataclass(slots=True)`, and `pathlib` throughout — no `os.path` anywhere. Gaps: unparameterized `asyncio.Queue` ([MED-07]), untyped `prompt_execution` parameters (`store.py:192,247`), the untyped kwargs dict at `sources.py:1229-1238` ([MED-03]), and the swallowed exception at `models.py:227` ([MED-08]). Broad `except Exception` appears frequently but is almost always accompanied by `# noqa: BLE001` and immediate normalization through `classify_unexpected_error` — that is a defensible boundary pattern, not a defect.
 ### FastAPI
 `create_app` (`app.py:87-111`) is a clean factory using the modern `lifespan` context manager, not the deprecated `@app.on_event`. Routers are domain-organized with prefixes and tags. `response_model` is declared on every route. Dependency injection is used correctly in `api/v4_documents.py:111-128`, with the useful touch that `get_document_service` prefers lifespan-owned state and falls back gracefully. Two gaps: service methods called from `async def` endpoints perform synchronous file I/O ([MED-01]), and `_recover_stale_processing_jobs` (`app.py:73-84`) constructs a throwaway `JobService` rather than using the bundle built five lines earlier.
 ### NiceGUI
 The strongest layer of the codebase in terms of convention adherence. CSS discipline is exemplary: a single `ui.add_css(read_css("theme.css"), shared=True)` at the composition root (`ui/__init__.py:28`), read through `importlib.resources` with a `@cache`-backed loader and path validation (`ui/resources.py:10-19`), and zero inline `.style()` calls or `<style>` blocks in components. **No cross-client state leakage was found** — per-request state lives in page-function closures, and the only module-level globals are idempotent registration flags (`theme.py:15`, `_register_panzoom_assets`'s `lru_cache`). `error_presenter.show_error/summarize_error` is applied uniformly and preserves `AppError` id/category/suggestion. The table architecture (generic `build_table` + per-feature row read models) matches the documented split. Defects are the boundary violations in [HIGH-07], the blocking I/O in [MED-01], and the duplication catalogued in §4.
 ### SQLModel & SQLAlchemy
 Session lifecycle is the clear high point: `ServiceBase._finalize` (`services/base.py:41-60`) implements a genuinely well-reasoned commit-vs-flush ownership protocol that lets orchestration functions commit exactly once at the workflow boundary, and `session_scope` / `transaction_scope` (`db/session.py:53-105`) express the two modes cleanly, with `transaction_scope` correctly rejecting a supplied session that has no active transaction. `JSONBCompat` (`models.py:27-35`) is the right cross-dialect abstraction, and `BigInteger` for `file_size_bytes` and `StaticPool` for in-memory SQLite show real attention to portability. Against that, the eager-loading defaults ([CRIT-02]), missing indexes ([HIGH-04]), unlocked job claim ([CRIT-01]), and hand-rolled DDL ([HIGH-05]) are the four issues that most need attention. Note also that `models.py` sets `updated_at` / `date_updated` via `default_factory` only — there is no `onupdate`, so these columns are stale unless a service sets them by hand (`jobs.py:166` does; most other update paths do not).
 ### Pydantic V2 & Settings
 Fully V2-native. No `@validator`, no `class Config`, no `.dict()`, no `parse_obj` anywhere. `model_config = ConfigDict(...)` is used consistently, usually with `extra="forbid", frozen=True` — a good default that catches provider payload drift. `Settings` is a single `BaseSettings` source of truth with `env_nested_delimiter`, a discriminated `DatabaseSettings` union, `SecretStr` for credentials, and constrained `Annotated` types (`NonEmptyStr`, `Probability`, `Temperature`). There are **no scattered `os.getenv` calls** in `src`. The one wart is `object.__setattr__` in `normalize_provider_models` (`config.py:130,137`) to mutate a frozen model — functional but fragile; `model_copy(update=...)` or a computed property would express it more safely. Issues: [MED-02] dead settings, [HIGH-03] the timeout cap, [MED-04] the `@cache` signature.
 ### Asyncio Workers
 `worker_consumer_lifespan` (`worker.py:64-94`) is well-built: it holds a strong reference to the task, sets the stop event, wakes the loop, waits with a bounded timeout, and escalates to `cancel()` + `suppress(CancelledError)` on timeout — a correct graceful-shutdown sequence. The `WorkerNotifier` Protocol with `Event`/`Noop` implementations is a clean seam. `_persist_page_outcome_durably` (`workflows.py:486-499`) uses `asyncio.shield` with correct cancellation re-raise so a shutdown mid-job cannot lose provider evidence — a genuinely subtle piece of code done right. Remaining concerns: the single-worker assumption is unenforced ([CRIT-01]), there is no backpressure or concurrency limit (jobs are processed strictly serially, so a large backlog drains slowly while the provider sits idle), and blocking I/O inside the loop ([MED-01]) stalls both the worker and all HTTP/UI clients on the same loop.
 ### OpenRouter / Adapter Boundary
 Encapsulation is good — no OpenRouter-specific header, model name, or payload shape appears in `services`, `api`, or `ui`. `providers/__init__.py`'s factory keeps the concrete adapter behind `get_transcription_provider`. Responses are validated through real Pydantic schemas (`OpenRouterResponse`, `ResponseChoice`, `ResponseUsage`), and `_CapturingAsyncClient` / `_CapturingAsyncByteStream` (`openrouter.py:45-91`) is a thoughtful mechanism for retaining raw transport bytes for evidence without disturbing SDK parsing. The failures are lifecycle and typing: per-job client churn ([HIGH-02]), no explicit httpx timeout ([HIGH-03]), and runtime `inspect`/`getattr` duck-typing instead of an honest Protocol ([MED-03]).
 ### Testing & Quality Tooling
 264 tests pass with 4 skipped; markers (`unit`/`integration`/`external`) are declared and `--strict-markers` is on; `filterwarnings` escalates never-awaited coroutines to errors — a good async-specific guard. Coverage is broad across services, providers, API, and UI pages. The material gaps: (a) `asyncio_mode = "strict"` is set but no `asyncio_default_fixture_loop_scope` is configured, which pytest-asyncio warns about and which will change behavior on upgrade; (b) **every test runs on SQLite**, so the Postgres support that `JSONBCompat`, `asyncpg`, and `psycopg2-binary` all exist to provide is entirely unverified — [HIGH-05]'s `CHAR(32)` bug is exactly the class of defect this would catch; (c) no test asserts concurrent job-claim safety, which is why [CRIT-01] survives; (d) `ty` cannot gate ([HIGH-06]); (e) `tools/run_destructive_tests.py:76,80` uses `fcntl`, unavailable on this project's Windows development platform.
 ---
 ## 4. Duplication & Consolidation Report
 | Pattern / Duplication | Locations | Proposed Canonical Home | Est. Lines Removed |
 | :--- | :--- | :--- | :--- |
 | Delete-confirmation page scaffold (blocked-deps card + confirm/cancel row) | `ui/pages/documents_page.py:354-433`; `jobs_page.py:358-428`; `sources_page.py:204-277`; `people_page.py:~270-310` | `ui/components/confirm_delete.py` | ~120 |
 | Upload/media URL resolution (`_resolve_*_src`, `_to_absolute_upload_url`) | `ui/pages/sources_page.py:711-770`; `people_page.py:525-579`; `ui/components/document_panzoom.py:59-75` | `ui/components/media_urls.py` (pure, takes `upload_dir` + `base_url`) | ~110 |
 | Invalid-id / not-found guard (parse → red label → return) | `documents_page.py:172-184,219-231,275-287,359-371`; `jobs_page.py:212-221,310-319,363-372`; `sources_page.py:111-137,207-222` | `ui/components/guards.py:load_or_render_error(...)` | ~90 |
 | Hand-rolled `ui.table` instead of `build_table` | `ui/pages/settings_page.py:65-113` + person-roles table; `ui/components/linked_people.py:66-75`; `print_preview_page.py:125-161` | `ui/components/table/common.py:build_table` (add selection / no-search options) | ~70 |
 | File-picker upload wiring | `people_page.py:491-522`; `jobs_page.py:449-503`; `home_page.py:38-40,85-89` | `ui/components/upload_panel.py` | ~50 |
 | `_parse_uuid` | `documents_page.py:591`; `jobs_page.py:558`; `sources_page.py:780`; `people_page.py:589`; `linked_people.py:175` | `ui/components/formatters.py` | ~35 |
 | `_resolve_runtime_settings(request)` | `jobs_page.py:567`; `sources_page.py:773`; `people_page.py:582` | Shared page-helper module | ~18 |
 | `_parse_iso_date` | `documents_page.py:600`; `people_page.py:598` | `ui/components/formatters.py` | ~14 |
 | `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: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
 ```python
 # src/transcription/services/media_storage.py
 async def store_media(
    *, filename: str, content: bytes, root: Path, relative_directory: Path | None = None,
    filename_stem: str | None = None, validate: Callable[[str, bytes], None] | None = None,
 ) -> StoredMedia: ...          # StoredMedia = frozen dataclass(path, sha256, byte_size, media_type)
                               # wraps the blocking write in asyncio.to_thread — resolves [MED-01]
 # src/transcription/services/__init__.py
@classmethod
 def from_session_factory(cls, factory: SessionFactory, settings: Settings | None = None) -> ServiceBundle: ...
 # src/transcription/services/jobs.py
 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: ...
 # src/transcription/ui/components/confirm_delete.py
 def render_confirm_delete(
    *, title: str, blockers: Sequence[str], on_confirm: Callable[[], Awaitable[None]],
    on_cancel: Callable[[], None],
 ) -> None: ...
 # src/transcription/ui/components/guards.py
 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]).
 3. Add `Index("ix_job_status_date_created", "status", "date_created")` and `index=True` on the hot foreign keys ([HIGH-04]).
 4. Add `.limit(1)` to `read_next_queued_job` — a one-line change that removes the full-queue load ahead of the full [CRIT-01] fix.
 5. Delete `services/transcription.py`, the three `store.py` aliases, and `ServiceBase.queue`; standardize `build_prompt_execution` imports ([MED-05], [MED-07]).
 6. Move the 23KB SVG to `ui/static/` and run `ruff check --fix` ([MED-09], [LOW-01]).
 7. Resolve or delete `sqlite_check_same_thread` and `worker_retry_backoff_seconds` ([MED-02]).
 ### Phase 2: Reliability & Concurrency (PR 3-4)
 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~~ — **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)
 13. Flip relationship defaults to `lazy="raise"` model by model, letting the existing suite prove which explicit `selectinload()` calls are load-bearing ([CRIT-02]). This also removes most of the `# pyright: ignore` comments.
 14. Standardize on `ty`, convert remaining suppressions to `# ty: ignore[...]`, and wire `ty check` into the existing pre-commit setup ([HIGH-06]).
 15. Fix the three UI boundary violations: session ownership in `jobs_page`, `sqlalchemy.inspect` in `sources_page`, `get_settings()` in `document_panzoom` ([HIGH-07]).
 16. Extract the UI duplication per §4, highest value first: `confirm_delete` → `media_urls` → `guards` → `formatters` (~500 lines removed).
 17. Replace `functools.cache` on engine/session factories with an explicit URL-keyed registry supporting targeted eviction ([MED-04]).
 ---
 ## 6. Preserved Strengths
 - **`ServiceBase._finalize` (`services/base.py:41-60`)** — the commit-vs-flush ownership protocol is the single best idea in the codebase. It lets orchestration functions compose multiple services into one atomic transaction without any service knowing about the others, and it is documented in `services.instructions.md`. Keep it and keep enforcing it.
 - **Error taxonomy (`errors.py`)** — `AppError` carrying `category`, `suggestion`, `retriable`, and a short shareable `error_id`, with `classify_unexpected_error` normalizing at every boundary and `format_error_detail` producing a stable persisted string. It is applied consistently from services through API handlers to `ui/components/error_presenter.py`.
 - **Evidence capture pipeline** — `_CapturingAsyncClient` / `_CapturingAsyncByteStream` (`openrouter.py:45-91`) plus `ExecutionAttempt` / `ProcessingArtifact` with content-addressed digests and integrity verification (`sources.py:925-959`) is a serious, well-executed provenance design that is rare to see done properly.
 - **`asyncio.shield` around page-outcome persistence (`workflows.py:486-499`)** — correctly written, including the `await task` before re-raising `CancelledError`, so provider results survive shutdown mid-job.
 - **Worker lifespan shutdown (`worker.py:64-94`)** — strong task reference, stop event, wake, bounded wait, then cancel-and-suppress. Textbook correct.
 - **Pydantic V2 discipline** — zero V1 residue, `extra="forbid"` + `frozen=True` as the house default, constrained `Annotated` types, `SecretStr` for credentials, discriminated union for database config, and no `os.getenv` anywhere in `src`.
 - **UI CSS and asset discipline** — one `add_css` at the composition root, `importlib.resources` with a `@cache`d reader and path validation, semantic `ui-*` classes, no inline styles. This is exactly what `ui.instructions.md` prescribes, followed without exception.
 - **No cross-client state leakage in NiceGUI** — per-request state lives in page-function closures; the only module globals are idempotent registration flags. This is the most common NiceGUI defect and this codebase avoids it entirely.
 - **Cross-dialect care** — `JSONBCompat`, `BigInteger` for byte sizes, `native_enum=False` with `values_callable` for stable enum storage, `StaticPool` for in-memory SQLite. The intent is right; it just needs Postgres CI to make it real.
 - **The instruction files themselves** — `.github/instructions/services.instructions.md` and `ui.instructions.md` are specific, enforceable, and largely followed. Most findings in this report are deviations from rules the project already wrote down, which is a much healthier position than having no rules at all.
@@ -1,8 +1,8 @@
-# Implementation Plan (Version 4)
+# Implementation Plan (version 4.0)
 ## Goal
-Implement the Version 4 project definition from the current repository state while preserving existing data by default.
+Implement the version 4.0 project definition from the current repository state while preserving existing data by default.
 ## Migration Policy
@@ -1,6 +1,6 @@
-# V4 Scope Boundary
+# V4.0 Scope Boundary
-This document defines the scope for the transition from the current repository state to the Version 4 project definition.
+This document defines the scope for the transition from the current repository state to the Version 4.0 project definition.
 ## Purpose
@@ -141,6 +141,6 @@ Deliver the V4.1 usability revision as a small, behavior-safe increment over the
 ## Related Local References
 - [V4.1 Scope Boundary](scope_boundary_v4_1.md)
- [V4 Implementation Plan](../ver4/implementation_plan_v4.md)
+- [V4 Implementation Plan](../ver4.0/implementation_plan_v4.md)
 - [V4 Requirements](../ver4/requirements_v4.md)
 - [V4 Error Handling Policy](../ver4/error_handling_v4.md)
@@ -137,6 +137,6 @@ This document defines the scope of the first incremental revision to Version 4.
 ## Related Local References
 - [V4.1 Implementation Plan](implementation_plan_v4_1.md)
- [V4 Scope Boundary](../ver4/scope_boundary_v4.md)
+- [V4 Scope Boundary](../ver4.0/scope_boundary_v4.md)
 - [V4 Architecture](../ver4/architecture_v4.md)
 - [V4 Schema](../ver4/schema_v4.md)