generated from john/python-template
174 lines
18 KiB
Markdown
174 lines
18 KiB
Markdown
# Architecture & Code Review Report
|
||
|
||
**Repository Target:** `project-root/`
|
||
**Target Stack:** Python 3.12+ | FastAPI | NiceGUI | SQLModel/SQLAlchemy | Pydantic V2 | asyncio | OpenRouter
|
||
|
||
---
|
||
|
||
## 1. Executive Summary
|
||
- The repository has strong architectural intent: explicit UI/service boundary tests, `lazy="raise"` ORM discipline, and append-only evidence tests are all present and passing.
|
||
- `pytest` is healthy (`324 passed` via `uv run python tools/run_destructive_tests.py --auto-restore -- pytest`), but static quality gates are not: `uv run ruff check` fails with 5 issues and `uv run ty check` fails with 12 diagnostics.
|
||
- The most important runtime risk is queue claiming on SQLite: `JobService.claim_next_queued_job` is only lock-safe on PostgreSQL, even though the project is SQLite-first.
|
||
- The `(job_id, source_id)` uniqueness invariant is documented and assumed by service code, but it is not enforced in the `JobSource` schema.
|
||
- UI media resolution does not fail closed; unmanaged paths fall back to `/uploads/{basename}`, which can display the wrong record’s media silently.
|
||
- Error taxonomy has drifted from canonical V4 authority: runtime/persisted categories use `*_error` values and map infrastructure failures to `timeout`/`internal`, conflicting with `docs/ver4/error_handling_v4.md`.
|
||
- Provenance capture is otherwise strong, but `application_commit` is taken directly from `os.environ` instead of `Settings`, so evidence context depends on undeclared ambient state.
|
||
- Highest-leverage refactors: make queue claims atomic on SQLite, add a `JobSource(job_id, source_id)` uniqueness constraint, make media resolution fail closed, and reconcile runtime error taxonomy with V4 docs/instructions/tests.
|
||
|
||
---
|
||
|
||
## 2. Executive Architecture Assessment
|
||
- Overall verdict: the codebase is structurally sound and notably governance-aware, but several “contract assumed in services/tests” rules are still not enforced at the persistence/runtime boundary.
|
||
- Domain cohesion is good: UI pages orchestrate services, orchestration lives in `store.py`/`workflows.py`, and evidence ownership remains centered in `SourceService`/`EvidenceService`.
|
||
- Boundary clarity is mostly strong, especially around print/export and append-only evidence, but controlled-path handling and queue exclusivity still rely too much on convention.
|
||
|
||
**Top systemic risks**
|
||
1. SQLite queue claims are not atomic, so duplicate provider work is possible under concurrent workers/processes.
|
||
2. `JobSource` uniqueness is assumed by business logic without a matching database constraint.
|
||
3. Error taxonomy and persisted `error_category` values have drifted from canonical V4 docs, weakening operator guidance and audit consistency.
|
||
4. General UI media resolution is less strict than print/export media resolution, creating a record-fidelity seam.
|
||
5. Tooling enforcement is asymmetric: behavior tests pass, but static-analysis debt is already present on mainline.
|
||
|
||
### Deterministic check outcomes
|
||
| Check | Result | Evidence |
|
||
| :--- | :--- | :--- |
|
||
| Service boundary rule | **PASS** | `tests/test_service_boundaries.py`; included in full passing `pytest` run |
|
||
| UI boundary rule | **PASS** | `tests/test_ui_boundaries.py`; included in full passing `pytest` run |
|
||
| Status vocabulary conformance | **PASS** | `src/transcription/db/models.py:60-77`; active runtime/UI usage matches `queued/processing/transcribed/partial_success/failed` and `pending/transcribed/failed/cancelled` |
|
||
| Evidence ownership conformance | **PASS** | `src/transcription/services/sources.py:466-590`, `src/transcription/services/evidence.py:97-126`; `tests/test_v42_evidence.py`, `tests/services/test_v45_candidates.py` passed |
|
||
| Canonical V4 authority | **FAIL** | `src/transcription/errors.py:12-24,62-76` diverges from `docs/ver4/error_handling_v4.md:5-20` |
|
||
| Schema contract fidelity | **PASS** | `docs/ver4/schema_v4.md` is field-aligned with `src/transcription/db/models.py`; however both omit enforcement of REQ-4-012 |
|
||
| Media boundary conformance | **FAIL** | Print path is validated in `src/transcription/api/v4_print.py:31-54`, but `src/transcription/ui/components/media_urls.py:41-74` fails open |
|
||
| Eager-loading conformance | **PASS** | Representative read paths (`documents.py`, `jobs.py`, `sources.py`, `people.py`) eagerly load needed relationships for `lazy="raise"` |
|
||
| Cross-cutting error conformance | **FAIL** | Runtime categories/mappings in `src/transcription/errors.py` and `src/transcription/services/store.py:121-128,198-206` do not match `.github/instructions/error-handling.instructions.md:16-38` |
|
||
|
||
---
|
||
|
||
## 3. Findings by Severity
|
||
|
||
### Critical Severity
|
||
#### [CRIT-01] SQLite queue claims are not atomic, so the same queued job can be claimed twice
|
||
- **Location:** `src/transcription/services/jobs.py:172-201`
|
||
- **Problem & Consequence:** The code does `SELECT ... WHERE status == queued LIMIT 1` and only uses `FOR UPDATE SKIP LOCKED` on PostgreSQL. On SQLite—the project’s default runtime from `pyproject.toml`/README—two workers or app processes can read the same queued row before either commit of `PROCESSING` becomes visible. That can trigger duplicate provider calls, duplicate `ExecutionAttempt` history, and conflicting terminal job writes for one logical job.
|
||
- **Recommendation:** Replace the select-then-set pattern with a dialect-safe atomic claim operation for SQLite as well (for example, compare-and-swap update inside a transaction, or `UPDATE ... WHERE id = (subquery) AND status='queued' RETURNING ...` where supported). Add a concurrency regression test that races two SQLite claimers against the same queue row.
|
||
- **Effort:** M
|
||
|
||
### High Severity
|
||
#### [HIGH-01] The documented one-row-per-`(job, source)` invariant is not enforced in `JobSource`
|
||
- **Location:** `docs/ver4/requirements_v4.md:16-21`, `src/transcription/db/models.py:359-384`, `src/transcription/services/sources.py:339-361`, `src/transcription/services/sources.py:519-529`
|
||
- **Problem & Consequence:** V4 requires exactly one `JobSource` row per `(job, source)` pair, and service code assumes uniqueness (`read_job_source_for_job` says “the unique JobSource association”; `update_job_source_transcription` fetches `.first()`). But `JobSource` has no `UniqueConstraint(job_id, source_id)`. Duplicate rows can therefore exist, after which write paths arbitrarily update the first row returned while reads and deletions become ambiguous.
|
||
- **Recommendation:** Add a DB constraint and migration for `(job_id, source_id)`, clean up any existing duplicates, and convert duplicate insert races into deterministic conflict handling. Mirror the invariant explicitly in `docs/ver4/schema_v4.md` once enforced.
|
||
- **Effort:** M
|
||
|
||
#### [HIGH-02] General UI media resolution falls back to basename instead of failing closed
|
||
- **Location:** `src/transcription/ui/components/media_urls.py:41-74`, `src/transcription/ui/pages/sources_page.py:261-265`
|
||
- **Problem & Consequence:** When a stored path is not provably under `upload_dir` and does not match an approved prefix, `resolve_media_url` still returns `/uploads/{path_obj.name}`. The source detail page passes that URL directly to the viewer. If an unmanaged or stale DB path shares a basename with another upload, the UI can render the wrong file instead of the recorded source, violating record fidelity and the “controlled resolver paths” rule.
|
||
- **Recommendation:** Fail closed: return `None` (or an explicit unavailable-state token) unless the path is validated as managed storage or an approved upload-relative form. Add tests for unmanaged absolute paths, stale relative paths, and basename-collision scenarios.
|
||
- **Effort:** S
|
||
|
||
#### [HIGH-03] Runtime and persisted error categories drift from canonical V4 policy
|
||
- **Location:** `src/transcription/errors.py:12-24`, `src/transcription/errors.py:62-76`, `src/transcription/services/store.py:121-128`, `src/transcription/services/store.py:198-206`, `src/transcription/services/workflows.py:602-623`
|
||
- **Problem & Consequence:** Canonical V4 policy allows six categories (`validation`, `not_found`, `conflict`, `external`, `timeout`, `internal`), but runtime code still carries a richer internal enum (`validation_error`, `external_provider_error`, `infrastructure_transient_error`, etc.) and persists those raw values to `ExecutionAttempt.error_category`. Worse, database/storage record-creation failures in `store.py` are classified `INFRA_TRANSIENT`, which API/UI mapping turns into `timeout`. Operators therefore receive category signals that do not match the docs or the actual failure mode.
|
||
- **Recommendation:** Either (a) collapse runtime and persisted categories to canonical V4 values end-to-end, or (b) explicitly version and document a two-layer taxonomy where internal categories are authoritative and canonical envelope mappings are deliberate. In either case, do not map storage/DB failures to `timeout`, and update tests/docs/instructions together.
|
||
- **Effort:** M
|
||
|
||
### Medium Severity
|
||
#### [MED-01] Provenance software context depends on undeclared ambient environment state
|
||
- **Location:** `src/transcription/providers/evidence.py:144-154`
|
||
- **Problem & Consequence:** `build_software_context` reads `TRANSCRIPTION_COMMIT` directly from `os.environ`, bypassing `Settings` and `.env.example` contract guards. That means evidence exports can vary by process environment in a way the repository’s documented configuration surface does not declare or test.
|
||
- **Recommendation:** Move commit identity into `Settings` (and `.env.example`/meta-contract tests) or derive it from a controlled startup-time source stored on app state. Provenance inputs should be explicit, versioned configuration, not ambient environment leakage.
|
||
- **Effort:** S
|
||
|
||
### Low Severity
|
||
#### [LOW-01] The advertised static-analysis baseline is currently red
|
||
- **Location:** `src/transcription/api/health.py:14-23`, `tests/services/test_service_base.py:31-138`, `src/transcription/config.py:204-208`, `tests/test_media_path_safety.py:3-5`, `tests/test_meta_contract_guards.py:3-8`, `tests/ui/test_media_urls.py:1-3`
|
||
- **Problem & Consequence:** `uv run ty check` reports 12 diagnostics (notably the health payload typing and untyped test doubles), and `uv run ruff check` reports 5 issues. The runtime test suite is green, but the repository’s stated quality toolchain is not actually clean, which reduces confidence that future structural drift will be caught early.
|
||
- **Recommendation:** Make `ruff check` and `ty check` pass on mainline, then keep them in CI as required gates alongside `pytest`.
|
||
- **Effort:** S
|
||
|
||
---
|
||
|
||
## 4. Architectural Drift & Gap Analysis
|
||
| Area / Component | Documented / Intended Rule | Actual Implementation State | Severity | Recommended Resolution |
|
||
| :--- | :--- | :--- | :--- | :--- |
|
||
| Queue claim lifecycle | `docs/ver4/architecture_v4.md` and service instructions require deterministic claim before provider work | `JobService.claim_next_queued_job` only has a hard concurrency guard on PostgreSQL; SQLite uses select-then-set | Critical | Implement atomic SQLite claim semantics and add concurrent claim tests |
|
||
| `JobSource` membership | `REQ-4-012` says one row per `(job, source)` pair | Model/service logic assumes uniqueness, but no DB constraint enforces it | High | Add schema constraint + migration + regression test |
|
||
| Error taxonomy | V4 docs/instructions define six canonical categories | Runtime/persisted categories use `*_error` values and mis-map infra transient to `timeout` | High | Reconcile docs, runtime enums, persisted fields, and tests in one change |
|
||
| General UI media resolver | `REQ-4-031` requires controlled application paths | Resolver falls back to basename `/uploads/{name}` even when storage provenance is not validated | High | Fail closed on unmanaged paths and add explicit resolver tests |
|
||
| Provenance config authority | Settings should be the single config authority | `TRANSCRIPTION_COMMIT` is read directly from `os.environ` | Medium | Add to `Settings` and meta-contract guards, or derive at startup and inject |
|
||
|
||
---
|
||
|
||
## 5. Invariant Inventory & Routing Recommendations
|
||
| Invariant / Constraint | Current Location | Recommended Target Layer | Rationale |
|
||
| :--- | :--- | :--- | :--- |
|
||
| One worker may claim one queued job exactly once | Service code + prose docs | Deterministic test + DB-aware implementation | This is a runtime correctness rule, not just documentation |
|
||
| One `JobSource` row per `(job, source)` pair | Requirements doc + service assumptions | Database constraint + migration + test | Uniqueness must be enforced where duplicates are created |
|
||
| Error category vocabulary stays canonical | Docs/instructions/tests partially; runtime diverges | Docs + instruction + deterministic tests + runtime enum cleanup | Taxonomy drift affects UX, API, and evidence exports |
|
||
| UI media URLs must resolve only validated managed paths | UI instructions + partial tests | Deterministic tests + resolver implementation | Security/fidelity rules should fail closed in code |
|
||
| Provenance software-context inputs are explicit configuration | Implementation only | `Settings` + `.env.example` + meta-contract test | Evidence metadata should not depend on undocumented ambient env |
|
||
|
||
---
|
||
|
||
## 6. Stack-Specific Analysis
|
||
- **Python 3.12+ Best Practices:** Modern typing is used widely (`StrEnum`, `Annotated`, builtin generics), but `ty` still fails on concrete payload typing (`api/health.py`) and permissive test doubles (`tests/services/test_service_base.py`).
|
||
- **FastAPI:** Lifespan wiring is modern and clean in `src/transcription/app.py`; dependency resolution for API services is explicit. The main concern is error-taxonomy drift, not route structure.
|
||
- **NiceGUI:** Page/service separation is well-enforced by tests. The main gap is not persistence leakage but UI media-resolution behavior in `components/media_urls.py`.
|
||
- **SQLModel & SQLAlchemy:** Relationship loading discipline is strong (`lazy="raise"` plus loader wrappers). The main persistence weaknesses are missing `JobSource` uniqueness enforcement and non-atomic SQLite queue claims.
|
||
- **Pydantic V2 & Settings:** V2 patterns are used correctly (`field_validator`, `model_validator`, `ConfigDict`, `SettingsConfigDict`). The notable exception is provenance code reading `os.environ` directly.
|
||
- **Asyncio Workers:** Worker containment and page-outcome durability are thoughtfully designed; reliability tests cover timeouts and stranded-job prevention. The open issue is claim exclusivity under SQLite concurrency.
|
||
- **OpenRouter / Adapter Boundary:** Request manifest and transport evidence capture are strong, secret-aware, and well-tested. Provenance context sourcing needs to be centralized under `Settings`.
|
||
- **Testing & Quality Tooling:** Behavioral coverage is excellent (`324` passing tests, including architecture guards and evidence tests). Static quality gates are currently failing and should be restored to green.
|
||
|
||
---
|
||
|
||
## 7. Duplication & Consolidation Report
|
||
| Pattern / Duplication | Locations | Proposed Canonical Home | Estimated Lines Removed |
|
||
| :--- | :--- | :--- | :--- |
|
||
| Repeated `try/except Exception -> show_error(...)` page action wrappers | `ui/pages/documents_page.py`, `ui/pages/jobs_page.py`, `ui/pages/people_page.py`, `ui/pages/settings_page.py`, `ui/pages/sources_page.py` | `ui/components/error_presenter.py` helper such as `run_ui_action(...)` | 50-80 |
|
||
| Repeated registry/read-model adapter shapes for document/person registries | `services/documents.py`, `services/people.py`, `api/v4_documents.py` | Shared registry/read-model adapter module near `services/registry.py` or `api/read_models.py` | 20-35 |
|
||
| Repeated storage-error wrapping around file persistence | `services/store.py`, `ui/homepage_store.py`, `services/people.py` | Higher-level wrappers in `services/media_storage.py` | 20-30 |
|
||
|
||
### Proposed Canonical Abstractions
|
||
- `async def run_ui_action(*, operation: str, title: str, action: Callable[[], Awaitable[T]]) -> T | None`
|
||
- `@dataclass(frozen=True) class RegistrySummary[ModelT]: ...`
|
||
- `async def persist_named_media(..., error: type[AppError], root: Path, namespace: str | None = None) -> Path`
|
||
|
||
---
|
||
|
||
## 8. Meta-Tooling & Instruction Update Recommendations
|
||
- Add a concurrency regression test for two simultaneous SQLite queue claimers against `JobService.claim_next_queued_job`.
|
||
- Add a deterministic resolver test suite for `resolve_media_url`, not just `public_media_path_label`.
|
||
- Update `.github/instructions/error-handling.instructions.md`, `docs/ver4/error_handling_v4.md`, and `tests/test_errors.py` together once the runtime taxonomy decision is made.
|
||
- Add an invariant test for `JobSource(job_id, source_id)` uniqueness once the schema is fixed.
|
||
- If `TRANSCRIPTION_COMMIT` remains supported, add it to `Settings`, `.env.example`, and `tests/test_meta_contract_guards.py`; otherwise remove ambient lookup from provenance code.
|
||
- Consider adding a meta-contract guard that `ruff check` and `ty check` must pass in CI, not just `pytest`.
|
||
|
||
---
|
||
|
||
## 9. Prioritized Dependency-Ordered Action Plan
|
||
1. **Phase 1: Blocking fixes**
|
||
- Make queue claims atomic on SQLite.
|
||
- Add `JobSource(job_id, source_id)` uniqueness enforcement and data cleanup.
|
||
2. **Phase 2: Enforcement hardening**
|
||
- Make `resolve_media_url` fail closed and add resolver-specific tests.
|
||
- Add deterministic tests for the queue-claim race and membership uniqueness.
|
||
3. **Phase 3: Reliability & concurrency**
|
||
- Reconcile runtime/persisted error categories with canonical V4 policy.
|
||
- Verify operator messaging and retry semantics after taxonomy cleanup.
|
||
4. **Phase 4: Consolidation & refactoring**
|
||
- Extract shared UI action/error helpers.
|
||
- Consolidate repeated registry/read-model and storage-wrapper patterns.
|
||
5. **Phase 5: Non-blocking governance/documentation depth**
|
||
- Move provenance commit identity into `Settings` or startup state.
|
||
- Restore `ruff`/`ty` to green and keep them gated alongside `pytest`.
|
||
|
||
---
|
||
|
||
## 10. Preserved Strengths
|
||
- Append-only evidence handling is thoughtfully designed and backed by strong tests (`tests/test_v42_evidence.py`, `tests/services/test_v45_candidates.py`).
|
||
- UI and service boundary rules are explicit, enforced, and currently passing.
|
||
- `lazy="raise"` plus eager-load wrappers create deterministic data-access behavior across services and pages.
|
||
- Print/export media handling is correctly record-validated in `src/transcription/api/v4_print.py`.
|
||
- Worker reliability has meaningful regression coverage for timeout handling, durability, and stuck-job recovery.
|