generated from john/python-template
296 lines
23 KiB
Markdown
296 lines
23 KiB
Markdown
# Implementation Plan (Version 4.6)
|
||
|
||
## Goal
|
||
|
||
Pay down the defects, duplication, and structural drift identified in the [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md) without changing any observable behavior. Re-level the database schema from current SQLModel metadata, correct read amplification and missing indexes, consolidate duplicated service and UI code, restore the project's own documented boundaries, and make `ty` a real quality gate.
|
||
|
||
## Planning Status
|
||
|
||
- V4.5 is the completed implementation baseline.
|
||
- The V4.6 scope is frozen and sufficiently detailed to begin implementation.
|
||
- Every task traces to a review finding ID. A change without a finding ID is a scope addition and requires an explicit amendment.
|
||
- The `SourceService` split ([MED-14]) is deferred to V4.7 by decision, not by omission.
|
||
|
||
## Planning Constraints
|
||
|
||
- **Behavior is preserved exactly.** All 264 pre-existing tests must still pass. A test that must change is evidence the change is not remediation.
|
||
- The application targets **SQLite only** in V4.6. PostgreSQL is unblocked but not enabled.
|
||
- The deployment is **single user, single process, single worker**. Forward-compatible code is written where cheap and dialect-guarded.
|
||
- The schema is re-leveled from metadata. **No Alembic, no revision directory, no history table, no down path.**
|
||
- All schema-affecting changes land in **one pass**; partial application is not a valid state.
|
||
- The data migration script is authored **last**, against the final schema and final loading strategy.
|
||
- Uploaded Source files, portraits, and artifact files on disk are never modified.
|
||
- Original Source files remain immutable; every V4.2–V4.5 evidence and provenance contract is preserved.
|
||
- Provider network work continues to occur outside database transactions.
|
||
- Database, integration, and UI tests use confirmed isolated data and never modify `data/transcription.db`.
|
||
- Potentially destructive tests run only through `tools/run_destructive_tests.py`.
|
||
|
||
## Expected Project Impact
|
||
|
||
| Area | Expected impact |
|
||
| --- | --- |
|
||
| Dead code | Remove `app_state.py`, `services/transcription.py`, legacy aliases, `ServiceBase.queue`, and a duplicate queued-job query. |
|
||
| Persistence | Delete the hand-rolled DDL chain; generate schema from metadata with correct indexes, FK ordering, and loading strategy. |
|
||
| Query behavior | Bounded queued-job poll, SQL-side filtering, bounded navigation queries, explicit eager loads. |
|
||
| Worker | Provider client and service bundle live for the worker's lifetime rather than per job. |
|
||
| Configuration | Remove the timeout cap and the silently-ignored `DATABASE_URL`; resolve dead settings; replace frozen-model mutation. |
|
||
| Service layer | Generic registry service, shared not-found guard, single media-storage implementation (~400 lines removed). |
|
||
| UI layer | Fix three boundary violations; extract duplicated components (~500 lines removed); externalize the SVG asset. |
|
||
| Async I/O | Move filesystem, hashing, and image work off the event loop. |
|
||
| Tooling | `ruff` and `ty` both reach zero and gate on pre-commit. |
|
||
| Data | One-time migration of backed-up V4.5 data into the re-leveled schema. |
|
||
| Tests and documentation | Add index, FK-cycle, claim-boundedness, client-reuse, and registry-parity coverage; correct the stale instruction path. |
|
||
|
||
## Implementation Phases
|
||
|
||
### 1. Deletions and Quick Wins
|
||
|
||
Independent of every other phase. Land first to shrink the surface everything else must consider.
|
||
|
||
- Delete `src/transcription/app_state.py` and confirm zero importers remain in `src`, `tests`, and `tools` ([HIGH-01]).
|
||
- Delete `src/transcription/services/transcription.py` and standardize every `build_prompt_execution` import on `services/sources.py` ([MED-05]).
|
||
- Delete the legacy compatibility aliases in `services/store.py:35,382,383` ([MED-05]).
|
||
- Delete `ServiceBase.queue` and its unparameterized `asyncio.Queue` ([MED-07]).
|
||
- Delete `db/operations.py:get_next_queued_job` as a divergent duplicate of the live implementation ([CRIT-01]).
|
||
- Resolve `sqlite_check_same_thread` and `worker_retry_backoff_seconds`: wire each to real behavior or delete it together with its test ([MED-02]).
|
||
- Remove `DATABASE_URL` from `docker-compose.yml` and document the real `DATABASE__DRIVER` / `DATABASE__PATH` nested names in `.env.example` ([MED-10]).
|
||
- Correct the stale path in `.github/instructions/services.instructions.md:10` to `src/transcription/db/models.py` ([LOW-02]).
|
||
- Remove the discarded `load_docs` parameter from `list_jobs` ([LOW-03]).
|
||
- Validate the `getattr` result in `resolve_worker_notifier` ([LOW-04]).
|
||
- Move `VIBESCRIBE_LOGO_SVG` to `ui/static/vibescribe_logo.svg` and load it through a `read_svg` sibling of `ui/resources.py:read_css` ([MED-09]).
|
||
- Run `ruff check --fix` and resolve the remainder by hand ([LOW-01]).
|
||
- Route `people_page.py:504` through `error_presenter.show_error` ([LOW-07]).
|
||
- Cancel the auto-refresh timer rather than only deactivating it, and name its interval constant ([LOW-06]).
|
||
|
||
**Verification:** full suite green, `ruff check` reports zero, no import of a deleted symbol remains.
|
||
|
||
### 2. Schema Re-Level — Single Pass
|
||
|
||
This phase is atomic. Every task below regenerates the same schema and must be verified together.
|
||
|
||
- Delete `upgrade_schema` and `_upgrade_*` (`db/operations.py:25-109`) and their tests (`tests/test_db.py:109-172`) ([HIGH-05]).
|
||
- Confirm `create_all()` remains gated by `Settings.should_bootstrap_schema` (`config.py:140-145`) ([HIGH-05]).
|
||
- Declare the composite index in the model: `Index("ix_job_status_date_created", "status", "date_created")`, plus `index=True` on the foreign keys the worker and detail pages filter on ([HIGH-04]).
|
||
- Declare `Source.preferred_execution_attempt_id`'s foreign key with `use_alter=True` and an explicit constraint name, breaking the `source` / `job_source` / `execution_attempt` cycle ([HIGH-08]).
|
||
- Flip relationship loading from bidirectional `lazy="selectin"` to `lazy="raise"`, model by model ([CRIT-02]):
|
||
- Work one model at a time with the suite as the safety net.
|
||
- Where a test fails with a lazy-load error, add an explicit `selectinload()` to the *service query* that feeds it — never restore the model-level default.
|
||
- Where an existing explicit `selectinload()` proves redundant, delete it; this is the primary source of the ~160 `ty` diagnostics addressed in Phase 6.
|
||
- Follow the two correct precedents already in the codebase: `Source.processing_artifacts:279` and `JobSource.execution_attempts:336`.
|
||
- Rebuild the development database from empty. Do **not** attempt to upgrade the existing file.
|
||
|
||
**Verification:**
|
||
- A test asserts the composite `Job` index and the hot foreign-key indexes exist in a freshly created schema.
|
||
- A test compiles the metadata against the PostgreSQL dialect and asserts **no** unresolvable-cycle warning is emitted.
|
||
- A test asserts `preferred_execution_attempt_id`'s column type matches the model declaration.
|
||
- Full suite green under `lazy="raise"`.
|
||
- No raw `ALTER TABLE` or `CREATE INDEX` string remains anywhere in `src`.
|
||
|
||
**Rollback:** this phase reverts as a unit. A partially applied schema pass is not a valid state.
|
||
|
||
### 3. Worker and Provider Reliability
|
||
|
||
Depends on Phase 2, because the claim query's cost profile is only correct once eager-loading defaults are fixed.
|
||
|
||
- Add `.limit(1)` to the queued-job selection and remove its eager-load options from the hot poll ([CRIT-01]).
|
||
- Convert the read-then-write claim into an atomic `QUEUED` → `PROCESSING` transition in one transaction ([CRIT-01]):
|
||
- Write the dialect-guarded `with_for_update(skip_locked=True)` branch for the multi-user direction.
|
||
- On SQLite, the claim executes as a bounded single-writer transaction.
|
||
- Load the eager relationships in a **second** query after the claim succeeds.
|
||
- Update the stale comment at `workflows.py:193-194` to describe the actual guarantee rather than the known hazard.
|
||
- Hoist `ServiceBundle` and the provider client out of the per-job body in `worker.py:157-174` to worker-loop scope; `aclose()` the client once at loop shutdown, not once per job ([HIGH-02]).
|
||
- Add `ServiceBundle.from_session_factory(...)`, replacing the three duplicated instantiation blocks at `app.py:45-50`, `worker.py:160-165`, and `services/__init__.py:19-22`. Have `_recover_stale_processing_jobs` (`app.py:73-84`) use the bundle built five lines earlier ([MED-06]).
|
||
- Remove `le=20.0` from `worker_provider_timeout_seconds` (`config.py:110`), raise the default to a realistic vision-transcription duration, and pass an explicit `httpx.Timeout` to the OpenRouter `AsyncClient` (`openrouter.py:198`) ([HIGH-03]).
|
||
- Extend the `TranscriptionProvider` Protocol to declare `aclose` and the evidence attributes; delete the per-call `inspect.signature(adapter.transcribe).parameters` reflection at `sources.py:1237` and the associated untyped kwargs dict ([MED-03]).
|
||
|
||
**Verification:**
|
||
- A test asserts the emitted claim SQL contains `LIMIT` and no `selectinload` join.
|
||
- A test asserts the worker processes two consecutive jobs against the same provider client instance.
|
||
- A test asserts a timeout value above 20 seconds is accepted by `Settings`.
|
||
- A test asserts the transcription call path resolves `requested_model` through the Protocol without reflection.
|
||
|
||
### 4. Service Layer Consolidation
|
||
|
||
Depends on Phase 2 only for the loading strategy; otherwise independent of Phase 3.
|
||
|
||
- Introduce `services/registry.py` with a generic `RegistryService[ModelT]` owning list, summaries with counts, create with `IntegrityError` → conflict mapping, read with not-found, update, delete with built-in and referenced guards, and `is_referenced` ([MED-11]):
|
||
- Define label normalization, the casefold key, and the summary shape once.
|
||
- Reduce `DocumentService`'s document-type methods (`documents.py:350-500`) and `PeopleService`'s person-role methods (`people.py:214-378`) to subclasses declaring model, error class, reference query, and noun.
|
||
- Preserve every existing user-facing message, error category, and suggestion string verbatim; template the noun only.
|
||
- Add `ServiceBase._get_or_raise(...)` and adopt it at all 38 not-found sites, including `documents.py:174,210,291`, which currently bypass the local `_get_document_or_raise` helper. Delete the now-redundant local helper ([MED-12]).
|
||
- Introduce `services/media_storage.py` as the single validate → hash → `mkdir` → write → wrap-`OSError` implementation, replacing `store.py:319-379`, `people.py:596-631`, and `ui/homepage_store.py:31-44`. Wrap the write in `asyncio.to_thread` ([MED-13], [MED-01]).
|
||
- Move `source_mime_type` out of `services/sources.py` into a shared module so `documents.py:24` no longer imports a sibling service, restoring the independence rule at `services.instructions.md:13` ([MED-14], partial).
|
||
- Correct the four query inefficiencies in `sources.py` ([LOW-08]):
|
||
- `list_sources_detail:338-343` — move the `job_id` filter from Python into a SQL join on `JobSource`.
|
||
- `read_source_navigation:233-244` — replace the full ordered-id scan with two `LIMIT 1` queries.
|
||
- `list_processing_artifacts:961` — add a `limit` parameter matching its summary sibling.
|
||
- `build_evidence_export:1012-1013` — move artifact integrity hashing into `asyncio.to_thread`.
|
||
|
||
**Verification:**
|
||
- Existing `DocumentType` and `PersonRole` tests pass **unchanged** against the shared implementation. This is the primary proof that behavior is preserved.
|
||
- A test asserts `list_sources_detail` filtered by `job_id` emits a join rather than loading the full table.
|
||
- No module in `services/` imports another concrete service module.
|
||
|
||
### 5. UI Boundaries and Duplication
|
||
|
||
Independent of Phases 2–4 except where a service signature changes.
|
||
|
||
- Fix the three `ui.instructions.md` violations ([HIGH-07]):
|
||
- Add a `JobService` or workflow method that owns `session_scope` internally; remove the import and transaction management from `jobs_page.py:17,185-192`.
|
||
- Have `SourceService` return a plain `transport_body_deferred: bool` on a read model; remove `sqlalchemy.inspect` from `sources_page.py:13,439`.
|
||
- Pass a ready media URL into `document_panzoom`, or delete the component — it is exported from `components/__init__.py` but used by no page ([HIGH-07]).
|
||
- Extract the duplication catalogued in review §4, highest value first:
|
||
- `ui/components/confirm_delete.py` — the blocked-deps card plus confirm/cancel row, from four pages (~120 lines).
|
||
- `ui/components/media_urls.py` — pure upload-URL resolution taking `upload_dir` and `base_url`, from three call sites (~110 lines).
|
||
- `ui/components/guards.py` — parse → error label → return, from nine call sites (~90 lines).
|
||
- `build_table` adoption for the remaining hand-rolled `ui.table` instances, adding selection and no-search options as needed (~70 lines).
|
||
- `ui/components/upload_panel.py` — file-picker wiring, from three pages (~50 lines).
|
||
- `ui/components/formatters.py` — `_parse_uuid` (five copies) and `_parse_iso_date` (two copies) (~49 lines).
|
||
- A shared page-helper for `_resolve_runtime_settings(request)` (three copies, ~18 lines).
|
||
- Annotate untyped handler parameters and replace loosely-typed dict returns with read models ([LOW-05]).
|
||
|
||
**Verification:** UI page tests pass unchanged; no page module imports `session_scope`, `sqlalchemy.inspect`, or `get_settings`.
|
||
|
||
### 6. Async I/O and Configuration Hygiene
|
||
|
||
- Wrap the remaining blocking work in `asyncio.to_thread`: Pillow orientation normalization, artifact writes, and evidence hashing not already covered by Phase 4 ([MED-01]).
|
||
- Replace `functools.cache` on the engine and session factories with an explicit URL-keyed registry supporting targeted eviction, removing the cross-test and cross-tenant coupling and restoring a visible call signature ([MED-04]).
|
||
- Replace `object.__setattr__` in `normalize_provider_models` (`config.py:130,137`) with `model_copy(update=...)` or a computed property.
|
||
- Add `onupdate` to the `updated_at` / `date_updated` columns that are expected to track modification, so they stop being stale on the update paths that do not set them by hand. Remove the now-redundant manual assignment at `jobs.py:166` and its siblings.
|
||
- Surface the exception currently swallowed to `None` in the ORM model property at `models.py:227` ([MED-08]).
|
||
|
||
**Note:** the `onupdate` change is schema-affecting in principle but not in emitted DDL, since `onupdate` is a Python-side default. If implementation reveals it alters generated DDL, it moves into Phase 2 and Phase 2 is re-verified.
|
||
|
||
**Verification:** a test asserts an update through a service advances `updated_at`; a test asserts two different database URLs produce two distinct engines and that evicting one leaves the other intact.
|
||
|
||
### 7. Type Checking and Tooling Gate
|
||
|
||
Depends on Phase 2, which is expected to remove most diagnostics by deleting redundant eager loads.
|
||
|
||
- Re-baseline `ty check` after Phase 2 and measure the remaining diagnostic count ([HIGH-06]).
|
||
- Convert every surviving `# pyright: ignore[...]` to `# ty: ignore[...]`, since `ty` does not honor pyright directives ([HIGH-06]).
|
||
- Fix the two real bugs currently hidden in the noise ([HIGH-06]):
|
||
- `tests/ui/test_sources_page.py:25` constructs `Source(...)` without the required `document_id`.
|
||
- `tools/run_destructive_tests.py:76,80` uses `fcntl`, which does not exist on Windows; use a cross-platform lock or guard by platform.
|
||
- Drive `ty check` to zero diagnostics and wire it into the existing pre-commit setup as a blocking gate.
|
||
- Configure `asyncio_default_fixture_loop_scope` explicitly so pytest-asyncio behavior does not change on upgrade.
|
||
|
||
**Verification:** `ty check` and `ruff check` both report zero; pre-commit fails when either regresses; `tools/run_destructive_tests.py` runs on Windows.
|
||
|
||
### 8. Data Migration
|
||
|
||
The final phase. Authored against the completed schema and the completed loading strategy.
|
||
|
||
- Write a one-time script under `tools/` that reads the backed-up V4.5 database and writes into the re-leveled schema (review §1a, "Items Added During Scoping").
|
||
- Because `lazy="raise"` is in force, every relationship traversal in the script carries an explicit eager load. This is the reason the script is written last.
|
||
- Preserve identity: UUIDs, digests, timestamps, attempt numbers, and `preferred_execution_attempt_id` selections carry across unchanged.
|
||
- Do not reinterpret, normalize, or regenerate any `ExecutionAttempt` or `ProcessingArtifact` evidence.
|
||
- Do not modify any on-disk Source file, portrait, or artifact file.
|
||
- The script is idempotent, is never invoked from application startup, and never runs in the test suite.
|
||
|
||
**Verification:** post-migration row counts match the backup for every table (`document` 8, `document_person` 11, `document_type` 7, `execution_attempt` 80, `job` 11, `job_source` 79, `person` 5, `person_role` 3, `processing_artifact` 2, `source` 76); artifact integrity verification passes for every migrated artifact; on-disk file hashes are unchanged.
|
||
|
||
## Sequencing Constraint
|
||
|
||
```mermaid
|
||
graph TD
|
||
P1[1. Deletions & Quick Wins]
|
||
P2[2. Schema Re-Level<br/>SINGLE ATOMIC PASS]
|
||
P3[3. Worker & Provider]
|
||
P4[4. Service Consolidation]
|
||
P5[5. UI Boundaries & Duplication]
|
||
P6[6. Async I/O & Config]
|
||
P7[7. Type-Check Gate]
|
||
P8[8. Data Migration]
|
||
|
||
P1 --> P2
|
||
P2 --> P3
|
||
P2 --> P4
|
||
P2 --> P7
|
||
P1 --> P5
|
||
P4 --> P5
|
||
P4 --> P6
|
||
P3 --> P8
|
||
P5 --> P8
|
||
P6 --> P8
|
||
P7 --> P8
|
||
```
|
||
|
||
The binding constraints are:
|
||
|
||
1. **Phase 2 is indivisible.** `create_all` from metadata, the indexes, `use_alter`, and the `lazy` flip all regenerate the same schema. They land together or not at all.
|
||
2. **Phase 7 follows Phase 2.** Measuring the `ty` baseline before the redundant eager loads are deleted would chase diagnostics that Phase 2 removes for free.
|
||
3. **Phase 8 is last.** The migration script must be written against the final schema and the final loading strategy.
|
||
|
||
## Test Strategy
|
||
|
||
- **The existing suite is the contract.** 264 tests pass today and must pass at every phase boundary. A test that requires modification is treated as a defect in that test, justified individually in the commit, and never as license to change behavior.
|
||
- **Registry parity is the key proof.** The `DocumentType` and `PersonRole` tests must pass *unchanged* against the shared `RegistryService`. If they need edits, the abstraction is wrong.
|
||
- **New tests are structural, not behavioral.** They assert schema shape, emitted SQL, dialect compatibility, and object lifetime — properties the current suite does not cover and that the review found were the reason these defects survived.
|
||
- New coverage to add:
|
||
|
||
| Assertion | Finding |
|
||
| :--- | :--- |
|
||
| Composite `Job` index and hot FK indexes exist in a fresh schema | [HIGH-04] |
|
||
| PostgreSQL-dialect metadata compilation emits no cycle warning | [HIGH-08] |
|
||
| `preferred_execution_attempt_id` column type matches the model | [HIGH-05] |
|
||
| Full suite passes under `lazy="raise"` | [CRIT-02] |
|
||
| Claim SQL contains `LIMIT` and no eager-load join | [CRIT-01] |
|
||
| Provider client instance is reused across two consecutive jobs | [HIGH-02] |
|
||
| `Settings` accepts a provider timeout above 20 seconds | [HIGH-03] |
|
||
| `list_sources_detail` emits a join rather than a full-table load | [LOW-08] |
|
||
| An update through a service advances `updated_at` | [SQLModel §3] |
|
||
| Distinct database URLs yield distinct, individually evictable engines | [MED-04] |
|
||
| Post-migration row counts match the backup | [Phase 8] |
|
||
|
||
- All database, integration, and UI tests continue to use isolated data and never touch `data/transcription.db`.
|
||
- Destructive tests continue to run only through `tools/run_destructive_tests.py`, which must first be made to run on Windows.
|
||
|
||
## Risks
|
||
|
||
| Risk | Likelihood | Impact | Mitigation |
|
||
| :--- | :--- | :--- | :--- |
|
||
| The `lazy="raise"` flip surfaces load paths the tests do not cover, breaking a UI page at runtime | High | Medium | Flip one model at a time; exercise every page manually at the phase boundary; `lazy="raise"` fails loudly rather than silently, which is the point |
|
||
| Phase 2 is partially applied and leaves an inconsistent schema | Medium | High | Treat Phase 2 as one commit; rebuild from empty rather than upgrading; verify all four schema assertions before proceeding |
|
||
| `RegistryService` generalization subtly changes a user-facing message or error category | Medium | Medium | Preserve message strings verbatim, templating only the noun; require the existing registry tests to pass unchanged |
|
||
| The atomic claim behaves differently on SQLite than the `FOR UPDATE SKIP LOCKED` path it is written to support | Medium | Low | Single worker in V4.6 means the SQLite path is the only one exercised; the Postgres branch is dialect-guarded and explicitly unverified until the cutover |
|
||
| Removing the timeout cap allows a pathological hang | Low | Medium | Pair the removal with an explicit `httpx.Timeout` so the client, not the config bound, enforces the ceiling |
|
||
| The migration script loses or reinterprets evidence | Low | High | Verify row counts per table, verify artifact integrity hashes post-migration, and never touch on-disk files |
|
||
| Remediation quietly becomes feature work | Medium | Medium | Every commit cites a finding ID; anything without one is recorded for a later revision |
|
||
| `ty` cannot reach zero without unsound suppressions | Medium | Low | Suppressions are acceptable where SQLModel typing is genuinely unrepresentable, but each must be `# ty: ignore[<rule>]` with a specific rule, never blanket |
|
||
|
||
## Delivery Order
|
||
|
||
1. Phase 1 — Deletions and Quick Wins
|
||
2. Phase 2 — Schema Re-Level (single atomic pass)
|
||
3. Phase 3 — Worker and Provider Reliability
|
||
4. Phase 4 — Service Layer Consolidation
|
||
5. Phase 5 — UI Boundaries and Duplication
|
||
6. Phase 6 — Async I/O and Configuration Hygiene
|
||
7. Phase 7 — Type Checking and Tooling Gate
|
||
8. Phase 8 — Data Migration
|
||
|
||
## Done Criteria
|
||
|
||
V4.6 is complete when every acceptance criterion in the [V4.6 Scope Boundary](scope_boundary_v4_6.md) is satisfied, specifically:
|
||
|
||
- All 264 pre-existing tests pass, with every modified test individually justified.
|
||
- `ruff check` and `ty check` both report zero and gate on pre-commit.
|
||
- No hand-rolled DDL, dead module, dead setting, or duplicate implementation identified in the review remains.
|
||
- The schema is generated from metadata, correctly indexed, cycle-free under the PostgreSQL dialect, and free of bidirectional `lazy="selectin"`.
|
||
- Roughly 900 lines of duplication are removed across the service and UI layers.
|
||
- The backed-up V4.5 data is restored into the re-leveled schema with matching row counts and unmodified on-disk files.
|
||
- No new user-facing feature exists that did not exist in V4.5.
|
||
|
||
## Related Local References
|
||
|
||
- [V4.6 Scope Boundary](scope_boundary_v4_6.md)
|
||
- [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md)
|
||
- [V4.5 Scope Boundary](../ver4.5/scope_boundary_v4_5.md)
|
||
- [V4.5 Implementation Plan](../ver4.5/implementation_plan_v4_5.md)
|
||
- [V4.2 Evidence and Provenance Scope](../ver4.2/scope_boundary_v4_2.md)
|
||
- [V4 Architecture](../ver4/architecture_v4.md)
|
||
- [V4 Schema](../ver4/schema_v4.md)
|
||
- [Transcription Methodology](../invariant/transcription_methodology.md)
|
||
- [AI Evidence and Provenance Invariant](../invariant/ai_evidence_and_provenance.md)
|