generated from john/python-template
Full review per .github/skills/python-code-reviewer/skill.md, with escalations to the evidence-provenance-auditor and test-effectiveness-auditor skills. Verification: ruff clean, 377 tests passing, 10 advisory ty diagnostics. Outcome: 0 critical, 4 high, 5 medium, 11 low. Report only; no source changes. Co-authored-by: Copilot App <[email protected]>
This commit is contained in:
Jim Lancaster
co-authored by
Copilot App
Copilot App
parent
8a30231adf
commit
8d3c60fce1
@@ -0,0 +1,475 @@
|
||||
# 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.
|
||||
|
||||
### 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.
|
||||
|
||||
```python
|
||||
# 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.
|
||||
|
||||
```python
|
||||
# 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.
|
||||
|
||||
```python
|
||||
# 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.
|
||||
|
||||
```python
|
||||
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
|
||||
|
||||
```python
|
||||
# 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.
|
||||
Reference in New Issue
Block a user