transcription/docs/architecture_code_review_2026-08-17.md

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 and V4.6 implementation plan.
Carried into V4.7	MED-14 (SourceService decomposition) and HIGH-06 (CI enforcement of the quality gate). See V4.7 scope boundary.

Where later thinking supersedes this report: the V4.6 review log 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. 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.

  # 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.

  # 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.

  # 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.

  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.

  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.

  # 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.

  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.

  # 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

# 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; see 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 @cached 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.