transcription/docs/reviews/2026-08-23-code-review.md

Architecture & Code Review Report

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

Review date: 2026-08-23 Governing procedure: .github/skills/python-code-reviewer/skill.md Escalations applied: .github/skills/evidence-provenance-auditor/skill.md, .github/skills/test-effectiveness-auditor/skill.md Scope: 77 Python modules / ~13k LOC under src/transcription, 57 test files (377 collected non-external tests), 23 documents under docs/, 9 active rule files.

Status: closed. Every finding below was remediated in the phases following this review. This document is retained as a record of the reasoning, not as a list of open work, and it is not canonical authority.

Two recommendations were wrong on contact and were corrected during implementation: the HIGH-03 fix as written would have stripped root-cause data from ExecutionAttempt provenance, and the HIGH-01 fix needed to preserve per-page durability that the report did not mention. Where this text and the current code or guard tests disagree, the code and tests are correct.

Verification commands and outcomes

Command	Outcome
uv run ruff check .	Pass — All checks passed!
uv run pytest -q -m "not external"	Pass — 377 passed
uv run ty check	10 diagnostics — all SQLModel/SQLAlchemy column-descriptor false positives (services/photos.py ×8, tests/test_storage_reconciliation.py ×2). Advisory only; no suppression strategy exists.

---

1. Executive Summary

• Overall health is good. The codebase has genuine architectural discipline: layered ui → services → db, a single Pydantic-V2 settings source, an atomic compare-and-swap job claim, append-only evidence history, and eleven deterministic guard tests that enforce structural rules rather than describing them.
• No Critical findings. The highest-risk category for this domain — secret leakage into stored provenance — was explicitly audited and passes: request headers are never persisted, response headers use an allowlist, and the API key is SecretStr end-to-end.
• The top risk is a transaction-atomicity violation on the worker hot path. Page evidence and terminal job status commit in two separate transactions (workflows.py:549-598), directly contradicting services.instructions.md. A crash between them leaves a transcript persisted against a job stuck in PROCESSING.
• That violation is invisible to the test suite. The test-effectiveness audit confirms no test can fail on a split commit — the pipeline tests assert the happy-path end state, which passes either way. The invariant is documented and steered but not enforced.
• Stale-job recovery is startup-only (app.py:79), with a 30-second staleness threshold. A job orphaned shortly before a fast restart is not recovered and remains PROCESSING indefinitely, because the worker only claims QUEUED rows.
• The mandated error-presentation boundary is bypassed at 8 sites. home_page.py and people_page.py hand-roll ui.notify(str(exc), ...), discarding the error_id, category, and suggestion that error_presenter.show_error provides. people_page.py imports the correct helpers and still bypasses them.
• User-facing output can leak filesystem paths. classify_unexpected_error (errors.py:94) interpolates the raw exception into a message rendered in the UI; a SQLAlchemy OperationalError embeds the database file path. This contradicts an explicit rule in error-handling.instructions.md.
• The retry gate ignores error category (workflows.py:185), so non-retriable faults would be requeued. Currently latent because worker_max_retries defaults to 0.
• Highest-leverage work is enforcement, not refactoring. Two atomicity tests, a ty suppression strategy that lets the pre-commit hook become blocking, and ruff format --check in the gate would convert three documented-but-unenforced invariants into deterministic ones.

---

2. Executive Architecture Assessment

Verdict: architecturally sound with a concentrated reliability gap in the worker's commit boundary.

Domain cohesion is strong. The services/ layer owns transactions and business rules, ui/ owns presentation, db/ owns schema, and providers/ isolates the OpenRouter adapter behind a TranscriptionProvider protocol. Dependency direction is correct and — unusually — mechanically enforced: test_service_boundaries.py AST-scans for service-to-service imports and test_ui_boundaries.py scans pages/components for persistence access. Provider details do not leak upward; workflows.py imports only the abstract providers types, never openrouter.

The evidence/provenance model is the strongest part of the system. ExecutionAttempt is genuinely append-only, retries append rather than rewrite, projection writes onto JobSource are clearly distinguished from history mutation, and all 14 provenance-auditor invariant checks pass.

Top systemic risks:

1. Split commit boundary on the worker path (High). Evidence durability and job terminal status are two transactions. This is the one place where the architecture's own written contract is contradicted by the implementation, on the hottest path in the system.
2. Recovery is a startup-only, time-thresholded sweep (Medium). There is no runtime reconciliation, so the self-healing property depends on restart cadence rather than on a bounded interval.
3. Enforcement coverage has known holes (Medium). Atomicity, error-presenter usage, and formatting are all documented rules with no deterministic test. The repo's own strength — routing invariants into tests — has not been applied to these three.
4. Leaky transaction ownership (Medium). workflows.py reaches into services.jobs._session_scope() and services.sources._session_scope() — private members of two different services — to open transactions. Session ownership is ambiguous exactly where it most needs to be explicit.
5. A 10-diagnostic type-checker baseline with no suppression policy (Low). The signal is currently ignorable, which means a real regression would blend into the noise.

---

3. Findings by Severity

Critical Severity

None identified.

The secret-leakage check — the only plausible Critical for this system — passes explicitly. OpenRouterProvider stores an allowlisted subset of response headers only (providers/evidence.py:130-134, SAFE_RESPONSE_HEADERS); request headers containing Authorization are never captured into TransportEvidence; and the key is held as SecretStr from config.py through to the client. Append-only evidence history is likewise intact and test-enforced.

---

High Severity

[HIGH-01] Page evidence and terminal job status commit in separate transactions

• Location: src/transcription/services/workflows.py:549-565 (_finalize_batch_outcome), src/transcription/services/workflows.py:584-598 (_persist_page_outcome)

• Problem & Consequence: .github/instructions/services.instructions.md states: "Never commit transcript updates separately from the paired terminal/retry job status change." The implementation does exactly that. _persist_page_outcome opens its own scope and commits page evidence (line 592-594); _finalize_batch_outcome later opens a second scope and commits the terminal JobStatus (line 558-560). For a single-page job these are two transactions with a window between them. A process crash, container eviction, or unhandled error in that window persists the transcript while the job remains PROCESSING. Because the worker only claims QUEUED rows, that job is not reprocessed; it is recoverable only by the startup sweep, and only if it has aged past the staleness threshold (see MED-01). The user sees a job that never completes despite the transcription having succeeded and been billed.

  This is a deliberate design tension, not an oversight: _persist_page_outcome_durably (line 568-581) wraps the page write in asyncio.shield precisely so per-page evidence survives cancellation mid-batch. That goal is correct for multi-page jobs. The defect is that the single-page and final-page cases inherit the split unnecessarily.

• Recommendation: Keep per-page durability for intermediate pages, but commit the final page outcome and the terminal status in one transaction.

  # Before — two scopes, two commits
  await _persist_page_outcome_durably(job=job, services=services, page=page, session=None)
  ...
  await _finalize_batch_outcome(job=job, services=services, status=status, session=None)
  
  # After — final page and terminal status share one transaction
  async with services.jobs.session_scope() as tx:
      for page in intermediate_pages:
          await _persist_page_outcome_durably(job=job, services=services, page=page, session=None)
      await _write_page_outcome(job=job, services=services, page=final_page, session=tx)
      await services.jobs.mark_job_status(job.id, status, session=tx)
      await tx.commit()
  

  Pair this with the atomicity test in HIGH-04 so the boundary cannot silently regress.

• Effort: M

---

[HIGH-02] Mandated error-presentation boundary bypassed at 8 sites

• Location: src/transcription/ui/pages/home_page.py:212,220,228,255; src/transcription/ui/pages/people_page.py:265,321,330,339

• Problem & Consequence: .github/instructions/ui.instructions.md:42 requires all user-facing error display to route through components/error_presenter.py. Seven of nine pages comply. These two hand-roll ui.notify(str(exc), type="negative"). The consequence is not cosmetic: show_error (error_presenter.py:52-67) surfaces the correlation error_id, the canonical error category, and the actionable suggestion field. Bypassing it means a user hitting a failure on the home or people page gets a bare exception string with no error reference to report, making these two pages unsupportable in production — precisely the pages most likely to be a user's entry point.

  people_page.py already imports run_ui_action and show_error at lines 28-29 and uses them elsewhere in the same module, so the bypass is inconsistency rather than missing infrastructure.

• Recommendation: Replace each site with the canonical helper. The unused summarize_error helper in error_presenter.py (currently a retained orphan — see LOW-07) is the natural fit where a compact string is genuinely needed.

  # Before
  except AppError as exc:
      ui.notify(str(exc), type="negative")
  
  # After
  except AppError as exc:
      show_error(exc)
  

  Then close the hole permanently by extending tests/test_ui_boundaries.py with an AST check that no module under PAGES_DIR calls ui.notify(...) with type="negative".

• Effort: S

---

[HIGH-03] Unexpected-error path leaks filesystem paths into user-facing output

• Location: src/transcription/errors.py:91-98 (line 94), rendered via src/transcription/ui/components/error_presenter.py:52-67

• Problem & Consequence: classify_unexpected_error builds f"Unexpected error during {operation}: {exc}" and stores it as AppError.message. show_error renders error.message directly to the user. Any exception whose str() contains infrastructure detail is therefore displayed verbatim — a SQLAlchemy OperationalError embeds the absolute SQLite database path, and an OSError from the media layer embeds the storage root. .github/instructions/error-handling.instructions.md:74 states: "Never leak … local filesystem paths in user-facing output." This is the generic catch-all path, so it applies to every unanticipated failure across the application.

• Recommendation: Split the diagnostic detail from the user-facing message. Log the full exception with the error_id as the correlation key; show the user a stable message plus that id.

  # Before
  return AppError(
      f"Unexpected error during {operation}: {exc}",
      category=ErrorCategory.INTERNAL_UNEXPECTED,
      ...
  )
  
  # After
  error = AppError(
      f"Unexpected error during {operation}.",
      category=ErrorCategory.INTERNAL_UNEXPECTED,
      suggestion="Retry once. If it persists, report the error reference id.",
      retriable=False,
  )
  logger.exception("error_id=%s operation=%s", error.error_id, operation)
  return error
  

  Add a case to tests/ui/test_error_presenter.py asserting that a raised OperationalError carrying a path does not surface that path in the rendered message.

• Effort: S

---

[HIGH-04] Transaction-atomicity invariants have no enforcing test

• Location: Contract at .github/instructions/services.instructions.md §"Workflow Transaction Boundaries"; gap confirmed across tests/integration/test_pipeline_flow.py:66-160 and tests/services/test_job_service.py:41-59

• Problem & Consequence: The test-effectiveness audit establishes that neither Transaction B (transcript + TRANSCRIBED) nor Transaction C (retry: error_detail + retry_count + QUEUED) is enforced. The existing pipeline test asserts the final state after a successful run — which passes identically whether the writes shared one commit or used two. To fail on a split-commit regression a test must inject a fault between the writes; no such test exists.

  The consequence is that HIGH-01 shipped undetected and any future refactor of advance_job can reintroduce it just as silently. This is a governance failure rather than a code defect: the repo's stated model is that hard rules belong in deterministic tests, and this rule is the most consequential one that never made the transition.

• Recommendation: Add tests/integration/test_pipeline_atomicity.py with two tests that patch the session to raise after flush() but before commit(), then assert that neither side of the pair is visible in a fresh session. These tests should fail against the current implementation and pass once HIGH-01 is fixed — write them first.

• Effort: M

---

Medium Severity

[MED-01] Stale-job recovery runs only at startup, behind a 30-second threshold

• Location: src/transcription/app.py:71-81 (_recover_stale_processing_jobs), sole caller at app.py:79 inside _lifespan
• Problem & Consequence: requeue_stale_processing_jobs has exactly one call site, in the lifespan startup handler. There is no runtime re-check. The staleness predicate is updated_at < now - worker_provider_timeout_seconds (default 30.0s, config.py:116). A job orphaned less than 30 seconds before a fast container restart therefore fails the predicate at the only moment recovery is attempted, and stays PROCESSING forever — the worker claims only QUEUED rows. It self-heals only on some later, unrelated restart. In a frequently-redeployed environment, restarts are exactly when orphans are created, so the recovery window is systematically misaligned with the failure it exists to handle.
• Recommendation: Move the sweep onto a periodic task in the worker loop (e.g. every max(30, provider_timeout * 2) seconds) in addition to the startup call, and derive the threshold from a dedicated worker_stale_job_seconds setting rather than reusing the provider timeout, so the two can be tuned independently.
• Effort: M

---

[MED-02] Retry gate ignores error_category, so non-retriable failures would be requeued

• Location: src/transcription/services/workflows.py:184-194

• Problem & Consequence: The JobStatus.FAILED branch gates solely on job.retry_count < settings.worker_max_retries. It does not consult error_category or the AppError.retriable flag. .github/instructions/error-handling.instructions.md classifies validation, not_found, and conflict as non-retriable; under this gate a malformed source or a missing record would be retried to exhaustion, consuming provider quota on calls that cannot succeed and delaying the terminal failure the user needs to see. There is also no backoff — retries requeue immediately.

  Currently latent: worker_max_retries defaults to 0 (config.py:113) and is commented out in .env, so the branch always falls through to the max-retries log. It becomes live the moment anyone enables retries.

• Recommendation: Gate on retriability and count, and add exponential backoff before requeue.

  case JobStatus.FAILED:
      if job.error_category in NON_RETRIABLE_CATEGORIES:
          logger.error("Job %s failed non-retriably (%s).", job.id, job.error_category)
          return
      if job.retry_count < settings.worker_max_retries:
          ...
  

  Cover with a test that a validation-category failure is not requeued even when worker_max_retries > 0.

• Effort: S

---

[MED-03] IntegrityError on the attempt-number flush is uncaught, risking evidence loss

• Location: src/transcription/services/sources.py:540-546 (attempt-number computation), sources.py:587 (unguarded flush())
• Problem & Consequence: attempt_number is derived read-then-write as MAX(attempt_number) + 1, and uq_execution_attempt_number enforces uniqueness (db/models.py:507, documented at docs/schema.md:273). The sibling JobSource insert does catch IntegrityError (sources.py:531-534), but the ExecutionAttempt flush at line 587 does not. Two concurrent attempt writes for the same job source would raise an unhandled IntegrityError and lose an evidence row — the one class of data this system exists to preserve. Not currently reachable: the worker is single-instance and processes sources sequentially. It becomes reachable the moment a second worker replica is deployed.
• Recommendation: Mirror the JobSource handling — catch IntegrityError, recompute MAX(attempt_number) + 1, and retry the insert a bounded number of times, raising a domain error on exhaustion. Note this constraint as a horizontal-scaling precondition in docs/production-runbook.md.
• Effort: M

---

[MED-04] Shutdown timeout is shorter than the provider timeout

• Location: src/transcription/worker.py:146 (asyncio.wait_for(worker_task, timeout=2.0)); provider timeout at config.py:116 (default 30.0s)
• Problem & Consequence: Graceful shutdown waits 2 seconds for the worker task, but the stop event is only checked between jobs and an in-flight provider call may run for up to 30 seconds. Any shutdown during a provider call therefore cancels mid-flight. Combined with HIGH-01's split commit, a cancellation that lands between the evidence commit and the status commit produces exactly the stuck-PROCESSING state described there — so this finding materially raises HIGH-01's probability rather than being independent of it.
• Recommendation: Derive the shutdown budget from the provider timeout (worker_provider_timeout_seconds + small_grace) instead of hardcoding 2.0, and ensure the container's termination grace period exceeds it. Document both in docs/production-runbook.md.
• Effort: S

---

[MED-05] workflows.py reaches into two services' private _session_scope

• Location: src/transcription/services/workflows.py:558 (services.jobs._session_scope()), workflows.py:592 (services.sources._session_scope())
• Problem & Consequence: The orchestration module opens transactions by calling a private member on two different service objects. This is the concrete mechanism behind HIGH-01: because transaction ownership is expressed through a private back-door rather than a declared boundary, nothing in the design makes it obvious that two scopes are being opened for one logical unit of work. It also couples workflows.py to a service implementation detail that test_service_boundaries.py cannot see (it checks imports, not attribute access).
• Recommendation: Promote a single explicit transaction entry point — a session_scope() on ServiceBundle, or a module-level unit_of_work(services) helper — and make workflows.py use only that. Extend test_service_boundaries.py with an AST check forbidding _session_scope attribute access outside the owning service module.
• Effort: M

---

Low Severity

[LOW-01] hashlib.sha256 over full file bytes runs on the event loop

• Location: src/transcription/services/store.py:401
• Problem & Consequence: Digest computation is CPU-bound and synchronous inside an async def. For large uploads this blocks the loop, stalling both the NiceGUI UI and the worker. Every sibling I/O path in the codebase correctly uses asyncio.to_thread (media_storage.py:43, normalization.py:117, photos.py:176, sources.py:740,753), so this is an isolated deviation.
• Recommendation: digest = await asyncio.to_thread(lambda: hashlib.sha256(file_bytes).hexdigest()).
• Effort: S

[LOW-02] homepage_store.py performs synchronous file I/O from async callers

• Location: src/transcription/ui/homepage_store.py:25,32; called from src/transcription/ui/pages/home_page.py:170
• Problem & Consequence: Same class as LOW-01 — reads/writes the homepage JSON directly rather than via asyncio.to_thread. Impact is small (a tiny file), but it is a second deviation from an otherwise universal convention.
• Recommendation: Wrap both calls in asyncio.to_thread.
• Effort: S

[LOW-03] Worker poll interval is hardcoded outside Settings

• Location: src/transcription/app.py:62 (poll_interval_seconds=1.0)
• Problem & Consequence: The single operational knob controlling worker latency-vs-load cannot be tuned without a code change, contradicting the otherwise-clean rule that all configuration lives in config.py (zero os.getenv calls exist outside it).
• Recommendation: Add worker_poll_interval_seconds: float = 1.0 to Settings and read it at the call site.
• Effort: S

[LOW-04] _build_request_manifest returns None silently, producing incomplete evidence

• Location: src/transcription/providers/openrouter.py:347
• Problem & Consequence: When source_reference is None the manifest is skipped with no log line. The attempt is still recorded but its provenance is quietly incomplete, and there is no signal that it happened — the failure mode is undetectable after the fact.
• Recommendation: Log at warning with the job/source identifiers before returning None, so incomplete provenance is at least attributable.
• Effort: S

[LOW-05] Ten ty diagnostics with no suppression strategy

• Location: src/transcription/services/photos.py (8), tests/test_storage_reconciliation.py (2)
• Problem & Consequence: All ten are SQLModel/SQLAlchemy false positives — column descriptors are typed as their Python value type (UUID, datetime, bool), so .is_(), .asc(), func.count(), and group_by() appear invalid. Because there is no suppression policy, the pre-commit hook must run ty in advisory mode, which means a genuine new type error would print alongside the known ten and block nothing.
• Recommendation: Add targeted # ty: ignore[...] comments with a one-line rationale at each of the ten sites, then flip the pre-commit hook to blocking. This converts a permanently-ignored signal into a real gate.
• Effort: M

[LOW-06] ruff format is not enforced; 35 files have drifted

• Location: .pre-commit-config.yaml, ruff.toml
• Problem & Consequence: ruff check is blocking but ruff format --check is absent from the gate, so formatting drift accumulates silently and inflates unrelated diffs whenever anyone does run the formatter.
• Recommendation: Run uv run ruff format . once as a single isolated commit, then add ruff format --check to the pre-commit gate.
• Effort: S

[LOW-07] Four retained orphans, all recorded as "uncertain — follow-up"

• Location: tests/test_orphan_sweep.py:33-52 (KNOWN_ORPHANS): BenchmarkManifest, dispose_all_engines, refresh_engine, summarize_error
• Problem & Consequence: Every entry carries the weakest possible justification. summarize_error is the notable one: it is an unused helper in error_presenter.py while two pages hand-roll error display (HIGH-02) — the orphan and the boundary violation are the same problem viewed from two directions. dispose_all_engines / refresh_engine are plausibly test-support utilities and should be classified as such rather than left uncertain.
• Recommendation: Resolve each to a definite outcome — summarize_error becomes used by the HIGH-02 fix; classify the engine helpers as test-support or delete them; decide on BenchmarkManifest.
• Effort: S

[LOW-08] Orphan sweep only scans module-level public definitions

• Location: tests/test_orphan_sweep.py
• Problem & Consequence: Methods and private functions are out of scope, so dead code inside classes — the most common kind in a service-oriented codebase — is structurally invisible to the sweep.
• Recommendation: Extend the AST walk to public methods on service classes, seeding KNOWN_ORPHANS with the current result set to keep the change non-breaking.
• Effort: M

[LOW-09] f-string interpolation in logging calls

• Location: src/transcription/services/workflows.py:193 and similar sites
• Problem & Consequence: logger.error(f"Job {job.id} has failed...") formats eagerly regardless of level and prevents structured-logging backends from grouping by template. Ruff's flake8-logging-format (G) rules are not enabled, so this is unenforced.
• Recommendation: Use logger.error("Job %s has failed and reached max retries.", job.id) and enable ruff rule set G.
• Effort: S

[LOW-10] Low-signal and always-true assertions in the test suite

• Location: tests/test_traceability.py:54-57; tests/integration/test_pipeline_flow.py:135-140,446-452; tests/test_orphan_sweep.py:119; tests/services/test_workflows_reliability.py:105,178,241,317,375
• Problem & Consequence: Per the test-effectiveness audit: test_traceability.py:54-57 asserts properties of dict literals defined in the same file (can only fail if the test itself is edited); assert processed is True in the pipeline tests is unfalsifiable because read_job raises rather than returning None; the >= 200 orphan threshold is a historical snapshot that tolerates ±40 drift; and the assert result is not None guards are shadowed by the attribute assertions that follow. Together these overstate effective coverage.
• Recommendation: Apply the prune/strengthen backlog in §6 (Testing).
• Effort: S

[LOW-11] Wall-clock timing dependencies risk CI flakiness

• Location: tests/services/test_workflows_reliability.py:157-196 (real time.sleep(0.40), upper bound < 540ms with only 10% slack); test_workflows_reliability.py:341 (asyncio.wait_for(..., timeout=2))
• Problem & Consequence: On a loaded CI runner, a 200ms asyncio task plus 400ms blocking setup can exceed the 540ms bound, producing false failures that erode trust in the suite.
• Recommendation: Widen the slack factor to 0.8 or replace the blocking sleep with a controlled clock mock.
• Effort: S

---

4. Architectural Drift & Gap Analysis

Direction is doc->code (implementation must change to match documented intent) or code->doc (an undocumented but repeatable convention that should be formalized).

Area / Component	Direction	Documented / Intended Rule	Actual Implementation State	Severity	Recommended Resolution
Worker commit boundary	doc->code	services.instructions.md: never commit transcript updates separately from the paired terminal status change	workflows.py:549-598 commits page evidence and terminal status in two separate sessions	High	Fix per HIGH-01; enforce per HIGH-04
UI error presentation	doc->code	ui.instructions.md:42: all user-facing error display routes through error_presenter.py	8 hand-rolled ui.notify sites in home_page.py and people_page.py	High	Fix per HIGH-02; add AST guard to test_ui_boundaries.py
Unexpected-error messaging	doc->code	error-handling.instructions.md:74: never leak local filesystem paths in user-facing output	errors.py:94 interpolates raw exc into the rendered message	High	Fix per HIGH-03
Retry policy	doc->code	error-handling.instructions.md: validation / not_found / conflict are non-retriable	workflows.py:185 gates on retry count only	Medium	Fix per MED-02
Stale-job recovery	code->doc	Not documented as startup-only or time-thresholded	Single startup call site; 30s threshold reuses the provider timeout	Medium	Fix per MED-01, then document the recovery contract in docs/production-runbook.md
Transaction ownership	code->doc	services.instructions.md assigns transaction ownership to services	workflows.py opens scopes via two services' private _session_scope	Medium	Fix per MED-05; document the single unit-of-work entry point
Blocking-I/O convention	code->doc	Not stated as a rule; followed at 5 of 7 sites	store.py:401 and homepage_store.py:25,32 deviate	Low	Fix per LOW-01/LOW-02, then state the asyncio.to_thread rule in services.instructions.md
Configuration centralization	code->doc	Zero os.getenv outside config.py — a real, held convention	Held everywhere except the hardcoded poll_interval_seconds at app.py:62	Low	Fix per LOW-03, then formalize the rule and add a deterministic guard
Type-check baseline	code->doc	No documented policy for ty diagnostics	10 tolerated false positives; hook is advisory-only	Low	Adopt the suppression strategy in LOW-05 and document it
Formatting	code->doc	ruff.toml configures the formatter	ruff format --check absent from the gate; 35 files drifted	Low	Fix per LOW-06
Dependency pin	—	docs/production-runbook.md "Dependency upgrade policy" records the exact nicegui==3.13.0 pin as a deliberate stability decision	Matches	—	No action — correctly documented, not a defect

---

5. Invariant Inventory & Routing Recommendations

Invariant / Constraint	Current Location	Recommended Target Layer	Rationale
Transcript + terminal status commit atomically	Instructions only	Deterministic test (tests/integration/test_pipeline_atomicity.py)	Highest-consequence rule in the system with zero enforcement; steering alone already failed to prevent HIGH-01
Retry writes commit atomically	Instructions only	Deterministic test (same file)	Same class; a partial retry commit corrupts retry_count accounting
All UI errors route through error_presenter	Instructions (ui.instructions.md:42)	Deterministic test (extend test_ui_boundaries.py)	Mechanically checkable via AST; 8 live violations prove instructions are insufficient here
No filesystem paths in user-facing output	Instructions (error-handling.instructions.md:74)	Deterministic test (extend tests/ui/test_error_presenter.py)	Checkable by asserting a path-bearing exception does not surface its path
Non-retriable categories are never requeued	Instructions	Deterministic test (tests/services/test_workflows_reliability.py)	Latent today; a test freezes the correct behavior before retries are enabled
Blocking I/O runs via asyncio.to_thread	Convention only (5/7 sites)	Instructions (services.instructions.md)	Judgment-dependent (thresholds vary by payload size); steering fits better than a hard test
Transaction opened through one owned entry point	Convention, violated	Instructions + test	Document the entry point; AST-guard against _session_scope access outside its owning module
Append-only ExecutionAttempt history	Docs + 3 tests	Keep as-is	Correctly routed and genuinely mutation-sensitive; the model to imitate
Service/UI boundary rules	Instructions + 2 AST tests	Keep as-is	Working exactly as intended
Status vocabulary conformance	docs/schema.md + contract guards	Keep as-is	Enum drift would fail the suite
No secrets in stored evidence	Docs + provenance skill + allowlist in code	Keep as-is	Allowlist is the right mechanism — fails closed by construction
ty diagnostic suppression policy	Nonexistent	Docs + blocking hook	Needs a written rationale per suppression before the gate can be trusted
NiceGUI exact pin	docs/production-runbook.md	Keep as-is	Deliberate, documented, correctly excluded from review findings

---

6. Stack-Specific Analysis

Python 3.12+ Best Practices

Modern syntax is used consistently: X | None unions throughout, builtin generics, no typing.List/Optional legacy forms, pathlib over os.path. Type-annotation coverage is high, with no bare Any on public service signatures. Broad except Exception appears where it belongs — the per-page handler at workflows.py:352 deliberately isolates one page's failure from the batch, which is correct. # noqa: PLR0915 / PLR1702 are used sparingly and consistently. Minor gaps: f-strings in logging (LOW-09), and two blocking-I/O deviations (LOW-01/LOW-02).

FastAPI

Lifespan is handled correctly via an asynccontextmanager _lifespan (app.py:36-68) rather than deprecated @app.on_event. Routers are domain-organized with typed path/query parameters and response_model declarations. Error handling is centralized through register_error_handlers, and the full internal→canonical category mapping is round-trip tested at the HTTP layer (tests/api/test_error_responses.py:59-95). print_api.py:42-49 performs correct relative_to-based path containment for media serving. No blocking calls found in async def route handlers.

NiceGUI

Separation of concerns is good — pages delegate to services and test_ui_boundaries.py mechanically prevents persistence access from pages and components. Client state is client-scoped; no cross-session global-state leaks found. API usage is correct for the pinned 3.13.0 release. The two defects are the error-presenter bypass (HIGH-02) and synchronous file I/O in homepage_store.py (LOW-02).

SQLModel & SQLAlchemy

The strongest layer. lazy="raise" is declared on relationships and correctly paired with expire_on_commit=False, which together make N+1 access a loud failure rather than a silent performance cost — no N+1 patterns found. The job claim is a genuine atomic compare-and-swap (jobs.py:212-222: conditional UPDATE ... WHERE status = QUEUED ... RETURNING), which is the correct primitive and correctly implemented. Hot-path indexes are declared and test-verified (test_db.py:131). Cross-dialect portability is handled for SQLite and PostgreSQL. Weaknesses are transaction ownership (MED-05, HIGH-01) rather than query construction, plus the uncaught IntegrityError at MED-03.

Pydantic V2 & Settings

Fully migrated — no @validator, no Config class, no .dict() or parse_obj anywhere. model_config = ConfigDict(...) and @field_validator are used correctly. config.py is a clean single source of truth: zero os.getenv calls exist outside it, .env is untracked and gitignored, and the API key is SecretStr end-to-end. The only deviation is the hardcoded poll interval (LOW-03).

Asyncio Workers

Task lifecycle is handled properly: task references are retained (no GC risk), CancelledError is re-raised rather than swallowed, the provider call happens outside any DB transaction, timeouts resolve to terminal states, and there is no tight polling spin. _persist_page_outcome_durably's use of asyncio.shield (workflows.py:568-581) is a thoughtful durability mechanism. The defects are the split commit boundary (HIGH-01), the shutdown-vs-provider timeout mismatch (MED-04), and startup-only recovery (MED-01).

OpenRouter / Adapter Boundary

Encapsulation is clean — workflows.py imports only abstract types from providers, never openrouter directly, so provider specifics do not leak into business logic. The AsyncClient is shared with configured timeouts and is properly closed: worker.py:248,271 → services.aclose() → sources.aclose() (sources.py:129-133) → provider aclose() (openrouter.py:86-87,233-235). Responses are Pydantic-validated. All 14 evidence-provenance-auditor invariant checks pass, including the critical one: the API key is never persisted, request headers are never stored, and TransportEvidence captures response headers through an explicit allowlist (evidence.py:130-134). Only LOW-04 applies here.

Testing & Quality Tooling

377 tests pass with -m "not external". The project test contract is honored: --strict-markers with all three markers (unit, integration, external) declared, asyncio_mode = "strict" with every async def test_ correctly decorated across all 17 async test files, external properly excluded from default runs, and no unawaited-coroutine warnings — the filterwarnings error promotion is clean.

Contract coverage is genuinely strong for structural rules. Confirmed mutation-sensitive enforcement exists for: append-only evidence history (3 independent tests, including full before/after field-tuple snapshots), stuck-in-PROCESSING prevention, the complete 10-category error mapping, and both boundary rules.

The critical gap is transaction atomicity (HIGH-04) — the audit verdict is "Effective with Conditions / Go with Conditions", blocking on the two missing atomicity tests. Secondary items are the low-signal assertions (LOW-10) and wall-clock flakiness (LOW-11).

Prune/strengthen backlog:

Priority	Task	Location
High	Add Transaction B atomicity test (fault injected between transcript and status writes)	new tests/integration/test_pipeline_atomicity.py
High	Add Transaction C atomicity test (retry: error_detail + retry_count + QUEUED)	same file
Medium	Delete tautological assertions on same-file dict literals	tests/test_traceability.py:54-57
Medium	Remove unfalsifiable assert processed is True	tests/integration/test_pipeline_flow.py:135-140,446-452
Medium	Replace >= 200 snapshot threshold with set-membership assertion	tests/test_orphan_sweep.py:119
Medium	Assert mapped test files contain ≥1 test, not merely that they exist	tests/test_traceability.py:59-60
Low	Widen timing slack or mock the clock	tests/services/test_workflows_reliability.py:157-196
Low	Drop assert result is not None guards shadowed by following assertions	tests/services/test_workflows_reliability.py:105,178,241,317,375

---

7. Duplication & Consolidation Report

Pattern / Duplication	Locations	Proposed Canonical Home	Estimated Lines Removed
Hand-rolled ui.notify(str(exc), type="negative")	home_page.py:212,220,228,255; people_page.py:265,321,330,339	ui/components/error_presenter.py::show_error (already exists)	~16
Optional-session if session is None: async with _session_scope() preamble	workflows.py:557-561, workflows.py:591-595, and sibling service write paths	services/base.py::unit_of_work(services, session) context manager	~30
Synchronous I/O not wrapped in asyncio.to_thread	store.py:401, homepage_store.py:25,32	services/base.py::run_blocking helper	~6
Read-then-increment MAX(n) + 1 with uniqueness retry	sources.py:540-546 (uncaught) vs sources.py:531-534 (caught)	services/base.py::insert_with_sequence_retry	~20

Proposed Canonical Abstractions

# src/transcription/services/base.py


@asynccontextmanager
async def unit_of_work(
    services: ServiceBundle,
    session: AsyncSession | None = None,
) -> AsyncIterator[AsyncSession]:
    """Single transaction entry point. Yields a session and commits once on clean exit.

    Replaces the `if session is None: async with X._session_scope()` preamble and the
    private-member access at workflows.py:558,592. Makes the two-commit split of
    HIGH-01 structurally hard to reintroduce.
    """


async def run_blocking[T](fn: Callable[[], T]) -> T:
    """Run a CPU- or disk-bound callable off the event loop."""
    return await asyncio.to_thread(fn)


async def insert_with_sequence_retry(
    session: AsyncSession,
    *,
    build: Callable[[int], SQLModel],
    next_value: Callable[[], Awaitable[int]],
    attempts: int = 3,
) -> SQLModel:
    """Insert a row carrying a derived sequence number, retrying on IntegrityError."""

---

8. Meta-Tooling & Instruction Update Recommendations

1. Add tests/integration/test_pipeline_atomicity.py (HIGH-04). The single highest-value enforcement change. Write it before fixing HIGH-01 so it demonstrably fails first.
2. Extend tests/test_ui_boundaries.py with an AST check forbidding ui.notify(..., type="negative") in PAGES_DIR, routing all error display through error_presenter. Converts ui.instructions.md:42 from steering into enforcement.
3. Extend tests/ui/test_error_presenter.py with a case asserting that a path-bearing exception does not surface its path, enforcing error-handling.instructions.md:74.
4. Adopt a ty suppression policy — targeted # ty: ignore[...] with rationale at the 10 known sites, documented in docs/ — then flip the pre-commit ty hook from advisory to blocking. Until this happens the type checker provides no gate.
5. Add ruff format --check to the pre-commit gate, preceded by one isolated formatting commit across the 35 drifted files.
6. Enable ruff rule set G (flake8-logging-format) to catch f-string logging (LOW-09).
7. Extend tests/test_orphan_sweep.py to public methods on service classes, seeding KNOWN_ORPHANS with current results (LOW-08). Then resolve all four existing "uncertain" entries to definite outcomes.
8. Extend tests/test_service_boundaries.py with an AST check forbidding _session_scope attribute access outside its owning service module (MED-05). Also address the noted classification gap: the test excludes orchestration modules by hardcoded stem name (store, workflows, __init__), so a new orchestration module under a different name would be misclassified as a service.
9. Update .github/instructions/services.instructions.md to state the asyncio.to_thread rule for blocking I/O and to name the single unit_of_work transaction entry point.
10. Update docs/production-runbook.md with the stale-job recovery contract (interval, threshold, and its relationship to the container termination grace period), and note single-worker as a current precondition until MED-03 is fixed.
11. Note for test_ui_boundaries.py: the forbidden-import lists are fixed string sets, so a future persistence helper under a new name would escape the check. Consider inverting to an allowlist of permitted imports for pages.

---

9. Prioritized Dependency-Ordered Action Plan

Phase 1: Blocking fixes

1. Write the two atomicity tests (HIGH-04) and confirm they fail against current main.
2. Fix the split commit boundary (HIGH-01) and confirm the tests now pass.
3. Fix the filesystem-path leak in classify_unexpected_error (HIGH-03).
4. Replace the 8 hand-rolled error notifications with show_error (HIGH-02).

Phase 2: Enforcement hardening 5. Add the ui.notify AST guard and the path-leak presenter test, locking in items 3-4. 6. Adopt the ty suppression policy and make the pre-commit hook blocking (LOW-05). 7. Run ruff format . as an isolated commit, then add ruff format --check to the gate (LOW-06). 8. Enable ruff rule set G and fix the resulting logging call sites (LOW-09).

Phase 3: Reliability & concurrency 9. Move stale-job recovery to a periodic worker task with a dedicated setting (MED-01). 10. Gate retries on error_category and add backoff (MED-02) — do this before ever raising worker_max_retries above 0. 11. Derive the shutdown budget from the provider timeout (MED-04). 12. Handle IntegrityError on the attempt-number flush (MED-03) — a hard precondition for running more than one worker replica. 13. Move sha256 and homepage-store I/O off the event loop (LOW-01, LOW-02); move the poll interval into Settings (LOW-03).

Phase 4: Consolidation & refactoring 14. Introduce unit_of_work and migrate workflows.py off private _session_scope access (MED-05); add the corresponding boundary guard. 15. Extract run_blocking and insert_with_sequence_retry (§7). 16. Prune the low-signal assertions and reduce timing flakiness (LOW-10, LOW-11).

Phase 5: Non-blocking governance/documentation depth 17. Extend the orphan sweep to methods and resolve the four uncertain orphans (LOW-07, LOW-08). 18. Update services.instructions.md and docs/production-runbook.md per §8 items 9-10. 19. Log incomplete request manifests (LOW-04). 20. Consider inverting the UI boundary check to an allowlist.

---

10. Preserved Strengths

• Evidence and provenance integrity is exemplary. All 14 provenance-auditor invariants pass. ExecutionAttempt history is genuinely append-only, retries append rather than rewrite, and projection writes are cleanly distinguished from history mutation. Three independent tests — including full before/after field-tuple snapshots — make any mutation regression fail loudly.
• Secret hygiene is correct by construction. The response-header allowlist (evidence.py:130-134) fails closed: a newly-introduced sensitive header is excluded by default rather than requiring someone to remember to block it. Request headers are never captured, and SecretStr is used end-to-end.
• Atomic job claiming. jobs.py:212-222 uses a conditional UPDATE ... WHERE status = QUEUED ... RETURNING — a true compare-and-swap that makes double-claiming impossible under concurrency, rather than the common read-then-write race.
• lazy="raise" paired with expire_on_commit=False. This combination turns accidental lazy loads into immediate errors instead of silent N+1 queries, and it is the reason no N+1 patterns exist in the codebase. Keep it.
• Architectural rules are mechanically enforced, not merely documented. AST-based boundary tests for service-to-service imports and UI persistence access are the right pattern; this review's main recommendation is simply to apply that same pattern to three more rules.
• Configuration discipline. Zero os.getenv calls outside config.py, .env untracked and gitignored, clean Pydantic V2 throughout with no V1 residue.
• Path containment on media serving. print_api.py:42-49 uses proper relative_to validation rather than string prefix matching.
• Async worker fundamentals. Task references retained, CancelledError re-raised, provider calls outside DB transactions, timeouts resolving to terminal states, no tight polling loop. asyncio.shield in _persist_page_outcome_durably is a genuinely thoughtful durability mechanism — the fix in HIGH-01 should preserve it for intermediate pages.
• Test contract rigor. --strict-markers, asyncio_mode = "strict" honored across all 17 async test files with no missing decorators, and coroutine-never-awaited promoted to a hard error with a clean run.