transcription/docs/architecture-code-review-2026-08-20.md

Architecture & Code Review Report

Repository Target: project-root/ Target Stack: Python 3.12+ | FastAPI | NiceGUI | SQLModel/SQLAlchemy | Pydantic V2 | asyncio | OpenRouter

---

1. Executive Summary

• The repository has strong architectural intent: explicit UI/service boundary tests, lazy="raise" ORM discipline, and append-only evidence tests are all present and passing.
• pytest is healthy (324 passed via uv run python tools/run_destructive_tests.py --auto-restore -- pytest), but static quality gates are not: uv run ruff check fails with 5 issues and uv run ty check fails with 12 diagnostics.
• The most important runtime risk is queue claiming on SQLite: JobService.claim_next_queued_job is only lock-safe on PostgreSQL, even though the project is SQLite-first.
• The (job_id, source_id) uniqueness invariant is documented and assumed by service code, but it is not enforced in the JobSource schema.
• UI media resolution does not fail closed; unmanaged paths fall back to /uploads/{basename}, which can display the wrong record’s media silently.
• Error taxonomy has drifted from canonical V4 authority: runtime/persisted categories use *_error values and map infrastructure failures to timeout/internal, conflicting with docs/ver4/error_handling_v4.md.
• Provenance capture is otherwise strong, but application_commit is taken directly from os.environ instead of Settings, so evidence context depends on undeclared ambient state.
• Highest-leverage refactors: make queue claims atomic on SQLite, add a JobSource(job_id, source_id) uniqueness constraint, make media resolution fail closed, and reconcile runtime error taxonomy with V4 docs/instructions/tests.

---

2. Executive Architecture Assessment

• Overall verdict: the codebase is structurally sound and notably governance-aware, but several “contract assumed in services/tests” rules are still not enforced at the persistence/runtime boundary.
• Domain cohesion is good: UI pages orchestrate services, orchestration lives in store.py/workflows.py, and evidence ownership remains centered in SourceService/EvidenceService.
• Boundary clarity is mostly strong, especially around print/export and append-only evidence, but controlled-path handling and queue exclusivity still rely too much on convention.

Top systemic risks

1. SQLite queue claims are not atomic, so duplicate provider work is possible under concurrent workers/processes.
2. JobSource uniqueness is assumed by business logic without a matching database constraint.
3. Error taxonomy and persisted error_category values have drifted from canonical V4 docs, weakening operator guidance and audit consistency.
4. General UI media resolution is less strict than print/export media resolution, creating a record-fidelity seam.
5. Tooling enforcement is asymmetric: behavior tests pass, but static-analysis debt is already present on mainline.

Deterministic check outcomes

Check	Result	Evidence
Service boundary rule	PASS	tests/test_service_boundaries.py; included in full passing pytest run
UI boundary rule	PASS	tests/test_ui_boundaries.py; included in full passing pytest run
Status vocabulary conformance	PASS	src/transcription/db/models.py:60-77; active runtime/UI usage matches queued/processing/transcribed/partial_success/failed and pending/transcribed/failed/cancelled
Evidence ownership conformance	PASS	src/transcription/services/sources.py:466-590, src/transcription/services/evidence.py:97-126; tests/test_v42_evidence.py, tests/services/test_v45_candidates.py passed
Canonical V4 authority	FAIL	src/transcription/errors.py:12-24,62-76 diverges from docs/ver4/error_handling_v4.md:5-20
Schema contract fidelity	PASS	docs/ver4/schema_v4.md is field-aligned with src/transcription/db/models.py; however both omit enforcement of REQ-4-012
Media boundary conformance	FAIL	Print path is validated in src/transcription/api/v4_print.py:31-54, but src/transcription/ui/components/media_urls.py:41-74 fails open
Eager-loading conformance	PASS	Representative read paths (documents.py, jobs.py, sources.py, people.py) eagerly load needed relationships for lazy="raise"
Cross-cutting error conformance	FAIL	Runtime categories/mappings in src/transcription/errors.py and src/transcription/services/store.py:121-128,198-206 do not match .github/instructions/error-handling.instructions.md:16-38

---

3. Findings by Severity

Critical Severity

[CRIT-01] SQLite queue claims are not atomic, so the same queued job can be claimed twice

• Location: src/transcription/services/jobs.py:172-201
• Problem & Consequence: The code does SELECT ... WHERE status == queued LIMIT 1 and only uses FOR UPDATE SKIP LOCKED on PostgreSQL. On SQLite—the project’s default runtime from pyproject.toml/README—two workers or app processes can read the same queued row before either commit of PROCESSING becomes visible. That can trigger duplicate provider calls, duplicate ExecutionAttempt history, and conflicting terminal job writes for one logical job.
• Recommendation: Replace the select-then-set pattern with a dialect-safe atomic claim operation for SQLite as well (for example, compare-and-swap update inside a transaction, or UPDATE ... WHERE id = (subquery) AND status='queued' RETURNING ... where supported). Add a concurrency regression test that races two SQLite claimers against the same queue row.
• Effort: M

High Severity

[HIGH-01] The documented one-row-per-(job, source) invariant is not enforced in JobSource

• Location: docs/ver4/requirements_v4.md:16-21, src/transcription/db/models.py:359-384, src/transcription/services/sources.py:339-361, src/transcription/services/sources.py:519-529
• Problem & Consequence: V4 requires exactly one JobSource row per (job, source) pair, and service code assumes uniqueness (read_job_source_for_job says “the unique JobSource association”; update_job_source_transcription fetches .first()). But JobSource has no UniqueConstraint(job_id, source_id). Duplicate rows can therefore exist, after which write paths arbitrarily update the first row returned while reads and deletions become ambiguous.
• Recommendation: Add a DB constraint and migration for (job_id, source_id), clean up any existing duplicates, and convert duplicate insert races into deterministic conflict handling. Mirror the invariant explicitly in docs/ver4/schema_v4.md once enforced.
• Effort: M

[HIGH-02] General UI media resolution falls back to basename instead of failing closed

• Location: src/transcription/ui/components/media_urls.py:41-74, src/transcription/ui/pages/sources_page.py:261-265
• Problem & Consequence: When a stored path is not provably under upload_dir and does not match an approved prefix, resolve_media_url still returns /uploads/{path_obj.name}. The source detail page passes that URL directly to the viewer. If an unmanaged or stale DB path shares a basename with another upload, the UI can render the wrong file instead of the recorded source, violating record fidelity and the “controlled resolver paths” rule.
• Recommendation: Fail closed: return None (or an explicit unavailable-state token) unless the path is validated as managed storage or an approved upload-relative form. Add tests for unmanaged absolute paths, stale relative paths, and basename-collision scenarios.
• Effort: S

[HIGH-03] Runtime and persisted error categories drift from canonical V4 policy

• Location: src/transcription/errors.py:12-24, src/transcription/errors.py:62-76, src/transcription/services/store.py:121-128, src/transcription/services/store.py:198-206, src/transcription/services/workflows.py:602-623
• Problem & Consequence: Canonical V4 policy allows six categories (validation, not_found, conflict, external, timeout, internal), but runtime code still carries a richer internal enum (validation_error, external_provider_error, infrastructure_transient_error, etc.) and persists those raw values to ExecutionAttempt.error_category. Worse, database/storage record-creation failures in store.py are classified INFRA_TRANSIENT, which API/UI mapping turns into timeout. Operators therefore receive category signals that do not match the docs or the actual failure mode.
• Recommendation: Either (a) collapse runtime and persisted categories to canonical V4 values end-to-end, or (b) explicitly version and document a two-layer taxonomy where internal categories are authoritative and canonical envelope mappings are deliberate. In either case, do not map storage/DB failures to timeout, and update tests/docs/instructions together.
• Effort: M

Medium Severity

[MED-01] Provenance software context depends on undeclared ambient environment state

• Location: src/transcription/providers/evidence.py:144-154
• Problem & Consequence: build_software_context reads TRANSCRIPTION_COMMIT directly from os.environ, bypassing Settings and .env.example contract guards. That means evidence exports can vary by process environment in a way the repository’s documented configuration surface does not declare or test.
• Recommendation: Move commit identity into Settings (and .env.example/meta-contract tests) or derive it from a controlled startup-time source stored on app state. Provenance inputs should be explicit, versioned configuration, not ambient environment leakage.
• Effort: S

Low Severity

[LOW-01] The advertised static-analysis baseline is currently red

• Location: src/transcription/api/health.py:14-23, tests/services/test_service_base.py:31-138, src/transcription/config.py:204-208, tests/test_media_path_safety.py:3-5, tests/test_meta_contract_guards.py:3-8, tests/ui/test_media_urls.py:1-3
• Problem & Consequence: uv run ty check reports 12 diagnostics (notably the health payload typing and untyped test doubles), and uv run ruff check reports 5 issues. The runtime test suite is green, but the repository’s stated quality toolchain is not actually clean, which reduces confidence that future structural drift will be caught early.
• Recommendation: Make ruff check and ty check pass on mainline, then keep them in CI as required gates alongside pytest.
• Effort: S

---

4. Architectural Drift & Gap Analysis

Area / Component	Documented / Intended Rule	Actual Implementation State	Severity	Recommended Resolution
Queue claim lifecycle	docs/ver4/architecture_v4.md and service instructions require deterministic claim before provider work	JobService.claim_next_queued_job only has a hard concurrency guard on PostgreSQL; SQLite uses select-then-set	Critical	Implement atomic SQLite claim semantics and add concurrent claim tests
JobSource membership	REQ-4-012 says one row per (job, source) pair	Model/service logic assumes uniqueness, but no DB constraint enforces it	High	Add schema constraint + migration + regression test
Error taxonomy	V4 docs/instructions define six canonical categories	Runtime/persisted categories use *_error values and mis-map infra transient to timeout	High	Reconcile docs, runtime enums, persisted fields, and tests in one change
General UI media resolver	REQ-4-031 requires controlled application paths	Resolver falls back to basename /uploads/{name} even when storage provenance is not validated	High	Fail closed on unmanaged paths and add explicit resolver tests
Provenance config authority	Settings should be the single config authority	TRANSCRIPTION_COMMIT is read directly from os.environ	Medium	Add to Settings and meta-contract guards, or derive at startup and inject

---

5. Invariant Inventory & Routing Recommendations

Invariant / Constraint	Current Location	Recommended Target Layer	Rationale
One worker may claim one queued job exactly once	Service code + prose docs	Deterministic test + DB-aware implementation	This is a runtime correctness rule, not just documentation
One JobSource row per (job, source) pair	Requirements doc + service assumptions	Database constraint + migration + test	Uniqueness must be enforced where duplicates are created
Error category vocabulary stays canonical	Docs/instructions/tests partially; runtime diverges	Docs + instruction + deterministic tests + runtime enum cleanup	Taxonomy drift affects UX, API, and evidence exports
UI media URLs must resolve only validated managed paths	UI instructions + partial tests	Deterministic tests + resolver implementation	Security/fidelity rules should fail closed in code
Provenance software-context inputs are explicit configuration	Implementation only	Settings + .env.example + meta-contract test	Evidence metadata should not depend on undocumented ambient env

---

6. Stack-Specific Analysis

• Python 3.12+ Best Practices: Modern typing is used widely (StrEnum, Annotated, builtin generics), but ty still fails on concrete payload typing (api/health.py) and permissive test doubles (tests/services/test_service_base.py).
• FastAPI: Lifespan wiring is modern and clean in src/transcription/app.py; dependency resolution for API services is explicit. The main concern is error-taxonomy drift, not route structure.
• NiceGUI: Page/service separation is well-enforced by tests. The main gap is not persistence leakage but UI media-resolution behavior in components/media_urls.py.
• SQLModel & SQLAlchemy: Relationship loading discipline is strong (lazy="raise" plus loader wrappers). The main persistence weaknesses are missing JobSource uniqueness enforcement and non-atomic SQLite queue claims.
• Pydantic V2 & Settings: V2 patterns are used correctly (field_validator, model_validator, ConfigDict, SettingsConfigDict). The notable exception is provenance code reading os.environ directly.
• Asyncio Workers: Worker containment and page-outcome durability are thoughtfully designed; reliability tests cover timeouts and stranded-job prevention. The open issue is claim exclusivity under SQLite concurrency.
• OpenRouter / Adapter Boundary: Request manifest and transport evidence capture are strong, secret-aware, and well-tested. Provenance context sourcing needs to be centralized under Settings.
• Testing & Quality Tooling: Behavioral coverage is excellent (324 passing tests, including architecture guards and evidence tests). Static quality gates are currently failing and should be restored to green.

---

7. Duplication & Consolidation Report

Pattern / Duplication	Locations	Proposed Canonical Home	Estimated Lines Removed
Repeated try/except Exception -> show_error(...) page action wrappers	ui/pages/documents_page.py, ui/pages/jobs_page.py, ui/pages/people_page.py, ui/pages/settings_page.py, ui/pages/sources_page.py	ui/components/error_presenter.py helper such as run_ui_action(...)	50-80
Repeated registry/read-model adapter shapes for document/person registries	services/documents.py, services/people.py, api/v4_documents.py	Shared registry/read-model adapter module near services/registry.py or api/read_models.py	20-35
Repeated storage-error wrapping around file persistence	services/store.py, ui/homepage_store.py, services/people.py	Higher-level wrappers in services/media_storage.py	20-30

Proposed Canonical Abstractions

• async def run_ui_action(*, operation: str, title: str, action: Callable[[], Awaitable[T]]) -> T | None
• @dataclass(frozen=True) class RegistrySummary[ModelT]: ...
• async def persist_named_media(..., error: type[AppError], root: Path, namespace: str | None = None) -> Path

---

8. Meta-Tooling & Instruction Update Recommendations

• Add a concurrency regression test for two simultaneous SQLite queue claimers against JobService.claim_next_queued_job.
• Add a deterministic resolver test suite for resolve_media_url, not just public_media_path_label.
• Update .github/instructions/error-handling.instructions.md, docs/ver4/error_handling_v4.md, and tests/test_errors.py together once the runtime taxonomy decision is made.
• Add an invariant test for JobSource(job_id, source_id) uniqueness once the schema is fixed.
• If TRANSCRIPTION_COMMIT remains supported, add it to Settings, .env.example, and tests/test_meta_contract_guards.py; otherwise remove ambient lookup from provenance code.
• Consider adding a meta-contract guard that ruff check and ty check must pass in CI, not just pytest.

---

9. Prioritized Dependency-Ordered Action Plan

1. Phase 1: Blocking fixes
   • Make queue claims atomic on SQLite.
   • Add JobSource(job_id, source_id) uniqueness enforcement and data cleanup.
2. Phase 2: Enforcement hardening
   • Make resolve_media_url fail closed and add resolver-specific tests.
   • Add deterministic tests for the queue-claim race and membership uniqueness.
3. Phase 3: Reliability & concurrency
   • Reconcile runtime/persisted error categories with canonical V4 policy.
   • Verify operator messaging and retry semantics after taxonomy cleanup.
4. Phase 4: Consolidation & refactoring
   • Extract shared UI action/error helpers.
   • Consolidate repeated registry/read-model and storage-wrapper patterns.
5. Phase 5: Non-blocking governance/documentation depth
   • Move provenance commit identity into Settings or startup state.
   • Restore ruff/ty to green and keep them gated alongside pytest.

---

10. Preserved Strengths

• Append-only evidence handling is thoughtfully designed and backed by strong tests (tests/test_v42_evidence.py, tests/services/test_v45_candidates.py).
• UI and service boundary rules are explicit, enforced, and currently passing.
• lazy="raise" plus eager-load wrappers create deterministic data-access behavior across services and pages.
• Print/export media handling is correctly record-validated in src/transcription/api/v4_print.py.
• Worker reliability has meaningful regression coverage for timeout handling, durability, and stuck-job recovery.