diff --git a/docs/architecture-code-review-2026-08-20.md b/docs/architecture-code-review-2026-08-20.md new file mode 100644 index 0000000..0bf65d5 --- /dev/null +++ b/docs/architecture-code-review-2026-08-20.md @@ -0,0 +1,173 @@ +# 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. diff --git a/docs/phase1-codex-prompt.md b/docs/phase1-codex-prompt.md new file mode 100644 index 0000000..515e15b --- /dev/null +++ b/docs/phase1-codex-prompt.md @@ -0,0 +1,38 @@ +You are working in the `transcription` repository (Python 3.12+, FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, asyncio). Follow `.github/instructions/services.instructions.md` and `.github/instructions/error-handling.instructions.md` for any code you touch, and keep `docs/ver4/*` as canonical authority for intended behavior. Do not modify unrelated code. + +## Goal + +Implement **Phase 1 (Blocking fixes)** from `docs/architecture-code-review-2026-08-20.md`, addressing two findings: + +### 1. [CRIT-01] SQLite queue claims are not atomic +**Location:** `src/transcription/services/jobs.py:172-201` (`claim_next_queued_job`) + +**Problem:** The claim does `SELECT ... WHERE status == QUEUED ORDER BY date_created, id LIMIT 1`, then sets `job.status = PROCESSING` and commits. `with_for_update(skip_locked=True)` is only applied on PostgreSQL (`if _session.get_bind().dialect.name == "postgresql"`). On SQLite — the project's default runtime per `pyproject.toml`/README — two concurrent workers/processes can both read the same QUEUED row before either commit becomes visible, causing the same job to be claimed twice: duplicate provider calls, duplicate `ExecutionAttempt` history, conflicting terminal writes. + +**Required fix:** +- Replace the select-then-set pattern with a dialect-safe atomic claim for SQLite (and keep it correct for PostgreSQL). Prefer a single atomic `UPDATE ... WHERE id = (subquery selecting the oldest QUEUED row) AND status = 'queued'` (optionally with `RETURNING` where the dialect/driver supports it), so the claim and the status transition happen as one atomic write instead of read-then-write across two statements. If `RETURNING` isn't reliably usable through the SQLModel/SQLAlchemy async session in this codebase, do the atomic `UPDATE` first (checking rowcount == 1 to confirm the claim succeeded), then re-`SELECT` the claimed row by id. +- Preserve the existing method signature, docstring intent, ordering semantics (oldest `date_created`, tie-broken by `id`), the "no eager loads on the hot poll" comment/behavior, and the `session`-scoping pattern (`self._session_scope`, `self._finalize`) used elsewhere in this file. +- Keep working correctly for both SQLite and PostgreSQL dialects — do not special-case away PostgreSQL's existing `SKIP LOCKED` correctness. +- Add a concurrency regression test (in the appropriate existing test file for `services/jobs.py`, e.g. `tests/services/test_jobs.py` or similar — check what already exists) that races two concurrent `claim_next_queued_job` calls against the same queued job on SQLite and asserts exactly one caller receives it and the other receives `None` (or the next distinct job, if a second job is queued). Use `asyncio.gather`/`TaskGroup` with separate sessions to simulate concurrent claimers, matching existing async test patterns in the repo. + +### 2. [HIGH-01] `JobSource` uniqueness on `(job_id, source_id)` is not enforced +**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:** V4 requires exactly one `JobSource` row per `(job, source)` pair. Service code already assumes this (`read_job_source_for_job` docstring says "the unique JobSource association"; `update_job_source_transcription` uses `.first()`), but the `JobSource` model has no DB-level uniqueness constraint on `(job_id, source_id)`. Duplicate rows can exist, causing writes to silently update the wrong row and making reads/deletes ambiguous. + +**Required fix:** +- Add a `UniqueConstraint("job_id", "source_id")` (via `__table_args__` on the `JobSource` SQLModel, consistent with how other constraints/indexes are declared in `src/transcription/db/models.py`) enforcing one row per `(job_id, source_id)` pair. +- Add/update the corresponding Alembic migration (check `alembic/` or the project's migration directory/tooling — follow whatever migration mechanism this repo already uses; look at recent migration files for the exact style) that creates the unique constraint/index, and includes a pre-migration cleanup step (or a documented manual step) to deduplicate any existing violating rows before the constraint is applied — do not let the migration fail on dirty data without a clear resolution path. Prefer keeping the most recently updated/created row per `(job_id, source_id)` pair and removing/merging older duplicates, but first inspect how `sources.py` picks "the" row today (e.g. `.first()` ordering) so the cleanup logic matches production behavior as closely as possible. +- Update `docs/ver4/schema_v4.md` to document the now-enforced `(job_id, source_id)` uniqueness invariant on `JobSource`. +- Convert any insert path that creates `JobSource` rows into deterministic conflict handling (e.g. catch the resulting integrity error and raise/return the appropriate domain-level `conflict` error per `.github/instructions/error-handling.instructions.md`, or use an upsert pattern) rather than letting a raw DB integrity error propagate. +- Add or extend a test (near `tests/test_service_boundaries.py`, `tests/services/test_sources*.py`, or wherever `JobSource` behavior is currently tested) asserting that attempting to create a second `JobSource` for an existing `(job_id, source_id)` pair is rejected/handled deterministically rather than silently succeeding. + +## Validation + +After implementing both fixes: +- Run `pytest` (use the project's normal invocation from `pyproject.toml`, e.g. via `uv run pytest`) and ensure all tests pass, including the new regression tests. +- Run `ruff check` and fix any new lint issues introduced by your changes (do not fix pre-existing unrelated ruff issues). +- Run `ty check` (or the project's type-checker command) and ensure no new type errors are introduced by your changes. +- Do not touch Phase 2+ items (media URL resolver, error taxonomy, provenance env var, ruff/ty baseline cleanup) — those are out of scope for this task. + +Report back with: files changed, a summary of the atomic-claim strategy chosen and why, the migration file added, and the final `pytest`/`ruff`/`ty` results. diff --git a/docs/phase2-codex-prompt.md b/docs/phase2-codex-prompt.md new file mode 100644 index 0000000..8245283 --- /dev/null +++ b/docs/phase2-codex-prompt.md @@ -0,0 +1,33 @@ +You are working in the `transcription` repository (Python 3.12+, FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, asyncio). Follow `.github/instructions/ui.instructions.md` and `.github/instructions/services.instructions.md` for any code you touch, and keep `docs/ver4/*` as canonical authority for intended behavior. Do not modify unrelated code. + +**Prerequisite:** This prompt assumes Phase 1 (`docs/phase1-codex-prompt.md`) is already merged — the SQLite atomic job claim and the `JobSource(job_id, source_id)` uniqueness constraint should already exist. If they do not, stop and flag this before proceeding, since this phase's regression tests depend on that groundwork. + +## Goal + +Implement **Phase 2 (Enforcement hardening)** from `docs/architecture-code-review-2026-08-20.md`: + +### 1. [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:** 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 (`REQ-4-031`). This is distinct from print/export media, which is already correctly record-validated in `src/transcription/api/v4_print.py:31-54`. + +**Required fix:** +- In `resolve_media_url` (`src/transcription/ui/components/media_urls.py`), remove the basename fallback. Return `None` (or an explicit "unavailable" sentinel/token consistent with how the rest of the UI layer signals missing/invalid media — check `error_presenter.py` and existing viewer components for the established pattern) unless the path is validated as either (a) safely resolvable under the configured `upload_dir`, or (b) an already-approved upload-relative form recognized elsewhere in the codebase (mirror the validation used by `v4_print.py`). +- Update `src/transcription/ui/pages/sources_page.py:261-265` (and any other call site relying on the old fallback behavior) to handle the `None`/unavailable case gracefully — e.g. show a clear "media unavailable" state in the viewer rather than crashing or rendering a blank/broken image. +- Add a dedicated resolver test suite (new or extended, e.g. `tests/ui/test_media_urls.py`) covering: a validated path under `upload_dir` (should resolve), an unmanaged absolute path outside `upload_dir` (should fail closed), a stale/nonexistent relative path (should fail closed), and a basename-collision scenario where an unmanaged path shares a filename with a legitimate upload (must NOT resolve to the wrong file). This directly satisfies the meta-tooling recommendation in `docs/architecture-code-review-2026-08-20.md` section 8: "Add a deterministic resolver test suite for `resolve_media_url`, not just `public_media_path_label`." + +### 2. Deterministic regression tests for Phase 1 fixes +**Rationale:** Phase 1 fixed the underlying atomicity/uniqueness issues; this phase locks them in with deterministic, always-run tests so no future change can silently regress them (per the review's Action Plan, item 2, and section 8 meta-tooling recommendations). + +**Required work:** +- Confirm (or add if missing) a concurrency regression test that races two concurrent `claim_next_queued_job` calls against the same queued job on SQLite and asserts exactly one caller wins. If Phase 1 already added this test, review it for robustness (e.g. does it actually force a race rather than relying on incidental ordering?) and strengthen it if needed — for example by using two independent sessions/connections and asserting via `asyncio.gather` that exactly one result is non-`None`. +- Confirm (or add if missing) a deterministic test asserting that creating a second `JobSource` for an existing `(job_id, source_id)` pair is rejected/handled predictably (not a silent duplicate). Ensure this test exercises the actual insertion code path used by `services/sources.py`, not just the raw model constraint. +- Both tests should live alongside the existing service test files for `jobs.py`/`sources.py` (check `tests/services/` for the correct location and naming convention). + +## Validation + +- Run `pytest` (via the project's normal invocation, e.g. `uv run pytest`, potentially through `tools/run_destructive_tests.py --auto-restore -- pytest` if that's how destructive/DB tests are run in this repo — check `pyproject.toml`/README) and ensure all tests pass, including new/strengthened tests. +- Run `ruff check` and `ty check` and ensure no new issues are introduced by your changes (do not attempt to fix the pre-existing baseline debt — that is Phase 5's responsibility). +- Do not touch error taxonomy (Phase 3), UI/service consolidation (Phase 4), or provenance/env config and ruff/ty baseline cleanup (Phase 5) — those are out of scope for this task. + +Report back with: files changed, the fail-closed validation logic chosen for `resolve_media_url`, the new/updated test coverage, and final `pytest`/`ruff`/`ty` results. diff --git a/docs/phase3-codex-prompt.md b/docs/phase3-codex-prompt.md new file mode 100644 index 0000000..d05c32e --- /dev/null +++ b/docs/phase3-codex-prompt.md @@ -0,0 +1,43 @@ +You are working in the `transcription` repository (Python 3.12+, FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, asyncio). Follow `.github/instructions/error-handling.instructions.md` and `.github/instructions/services.instructions.md` for any code you touch, and keep `docs/ver4/*` as canonical authority for intended behavior. Do not modify unrelated code. + +**Prerequisite:** This prompt assumes Phases 1-2 (`docs/phase1-codex-prompt.md`, `docs/phase2-codex-prompt.md`) are already merged. + +## Goal + +Implement **Phase 3 (Reliability & concurrency)** from `docs/architecture-code-review-2026-08-20.md`, addressing: + +### [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:** Canonical V4 policy (`docs/ver4/error_handling_v4.md`) and `.github/instructions/error-handling.instructions.md` define six canonical error categories: `validation`, `not_found`, `conflict`, `external`, `timeout`, `internal`. But runtime code in `src/transcription/errors.py` still carries a richer internal enum (`validation_error`, `external_provider_error`, `infrastructure_transient_error`, etc.), and those raw non-canonical values get persisted directly to `ExecutionAttempt.error_category`. Worse, database/storage record-creation failures in `store.py` (lines 121-128, 198-206) are classified as `INFRA_TRANSIENT`, which the API/UI mapping layer turns into `timeout` — even though a DB write failure is not a timeout. Operators therefore receive category signals that don't match the documented taxonomy or the actual failure mode. `workflows.py:602-623` is a downstream consumer of these categories and needs review once the taxonomy changes. + +**You must choose one of two remediation strategies — read `docs/ver4/error_handling_v4.md` and `.github/instructions/error-handling.instructions.md` fully before deciding, and justify your choice in your final report:** + +**Option (a) — Collapse to canonical values end-to-end:** +- Reduce the runtime `errors.py` enum to exactly the six canonical categories. +- Update every raise site and every persisted-category write path (including `ExecutionAttempt.error_category`) to use only canonical values. +- Fix the specific `store.py` misclassification: database/storage record-creation failures must map to a category that reflects an infrastructure/internal failure, not `timeout`. Determine the correct canonical category (likely `internal` or `external`, per the documented semantics of each — do not guess without checking the doc's definitions). + +**Option (b) — Explicit two-layer taxonomy:** +- Keep the richer internal enum but explicitly document (in `docs/ver4/error_handling_v4.md` and `.github/instructions/error-handling.instructions.md`) that internal categories are authoritative for diagnostics/evidence, while a deliberate, documented mapping table converts them to the six canonical categories at the API/UI envelope boundary. +- Ensure the mapping table is centralized (not duplicated across call sites) and is itself unit-tested. +- Still fix the `store.py` mapping: DB/storage failures must not map to `timeout`. + +**Either way, required work:** +- Do not leave storage/DB failures mapped to `timeout` under any resolution. +- Update `tests/test_errors.py` (and any other test asserting category behavior, e.g. tests referencing `ExecutionAttempt.error_category` or API error envelopes) to reflect the finalized taxonomy. +- Update `docs/ver4/error_handling_v4.md` and `.github/instructions/error-handling.instructions.md` together with the code change so docs/instructions/runtime/tests move as one atomic unit — per the review's explicit warning against updating only one of these. +- Review `src/transcription/services/workflows.py:602-623` for correct behavior against the finalized taxonomy (e.g. retry/backoff decisions keyed off error category should still make sense). + +### Verify operator messaging and retry semantics after taxonomy cleanup +- After the taxonomy change, walk through any UI-facing error message templates or API error envelope construction that branches on error category, and confirm the operator-visible messaging still makes sense (e.g. a DB failure should not say "request timed out"). +- Confirm retry/backoff logic (in `workflows.py` or wherever retries are orchestrated) that depends on error category still selects the correct retry behavior for each category post-cleanup (e.g. `external`/`timeout` should still be retryable where appropriate; `validation`/`not_found`/`conflict` should not). +- Add or extend tests covering at least one representative case per category to confirm messaging and retry decisions are correct. + +## Validation + +- Run `pytest` (via the project's normal invocation, e.g. `uv run pytest`) and ensure all tests pass, including updated/added tests for the taxonomy and retry behavior. +- Run `ruff check` and `ty check` and ensure no new issues are introduced by your changes. +- Do not touch UI/service consolidation (Phase 4) or provenance/env config and ruff/ty baseline cleanup (Phase 5) — those are out of scope for this task. + +Report back with: which remediation option you chose and why, the final canonical/internal category mapping (if option b), all files changed, the corrected `store.py` category for DB/storage failures, and final `pytest`/`ruff`/`ty` results. diff --git a/docs/phase4-codex-prompt.md b/docs/phase4-codex-prompt.md new file mode 100644 index 0000000..480b0dd --- /dev/null +++ b/docs/phase4-codex-prompt.md @@ -0,0 +1,63 @@ +You are working in the `transcription` repository (Python 3.12+, FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, asyncio). Follow `.github/instructions/ui.instructions.md` and `.github/instructions/services.instructions.md` for any code you touch, and keep `docs/ver4/*` as canonical authority for intended behavior. Do not modify unrelated code. + +**Prerequisite:** This prompt assumes Phases 1-3 (`docs/phase1-codex-prompt.md`, `docs/phase2-codex-prompt.md`, `docs/phase3-codex-prompt.md`) are already merged. + +## Goal + +Implement **Phase 4 (Consolidation & refactoring)** from `docs/architecture-code-review-2026-08-20.md`, based on section 7 (Duplication & Consolidation Report). This phase is behavior-preserving refactoring: no functional change should be introduced, only reduction of duplication. Every extraction must be covered by the existing test suite passing unchanged (plus any new unit tests for the extracted helper itself). + +### 1. Extract shared UI action/error helper +**Locations with the duplicated pattern:** `src/transcription/ui/pages/documents_page.py`, `src/transcription/ui/pages/jobs_page.py`, `src/transcription/ui/pages/people_page.py`, `src/transcription/ui/pages/settings_page.py`, `src/transcription/ui/pages/sources_page.py` + +**Problem:** These pages repeat a `try/except Exception -> show_error(...)` wrapper pattern around UI actions. Estimated 50-80 duplicated lines. + +**Required work:** +- Read `src/transcription/ui/components/error_presenter.py` to understand the existing error-presentation primitives (e.g. `show_error`) and NiceGUI conventions already in use. +- Add a shared helper in `error_presenter.py` (or another UI components module per `.github/instructions/ui.instructions.md` ownership rules) with a signature similar to: + ```python + async def run_ui_action(*, operation: str, title: str, action: Callable[[], Awaitable[T]]) -> T | None: + ... + ``` + Adjust the exact signature/name as needed to fit the existing calling conventions across the five pages (e.g. some call sites may need access to a spinner/notification/loading state — inspect each site before finalizing the signature). +- Replace each duplicated `try/except Exception -> show_error(...)` block across the five listed pages with a call to the new shared helper, preserving exact existing behavior (same error messages, same UI state transitions, same logging if any). +- Add a focused unit test for the new helper (success path, exception path, and confirm it surfaces the same error message/formatting the old inline blocks did). + +### 2. Consolidate registry/read-model adapter shapes +**Locations:** `src/transcription/services/documents.py`, `src/transcription/services/people.py`, `src/transcription/api/v4_documents.py` + +**Problem:** Repeated registry/read-model adapter shapes for document/person registries. Estimated 20-35 duplicated lines. + +**Required work:** +- Compare the registry/read-model adapter code in `documents.py` and `people.py` (and how `v4_documents.py` consumes it) to identify the common shape. +- Introduce a shared abstraction near `src/transcription/services/registry.py` (or a new `src/transcription/api/read_models.py` if the duplication is primarily API-layer) — the review suggests something like: + ```python + @dataclass(frozen=True) + class RegistrySummary[ModelT]: + ... + ``` + Adapt the generic shape to what the actual duplicated code needs (inspect both usages first — do not force-fit a generic that doesn't match real field/behavior overlap). +- Refactor `documents.py`, `people.py`, and `v4_documents.py` to use the shared abstraction, preserving exact existing read-model output (field names/types/values returned to callers/API responses must not change). + +### 3. Consolidate storage-error wrapping around file persistence +**Locations:** `src/transcription/services/store.py`, `src/transcription/ui/homepage_store.py`, `src/transcription/services/people.py` + +**Problem:** Repeated storage-error wrapping around file persistence. Estimated 20-30 duplicated lines. + +**Required work:** +- Identify the common file-persistence + error-wrapping pattern across the three locations. +- Introduce a higher-level wrapper in `src/transcription/services/media_storage.py`, e.g. along the lines of: + ```python + async def persist_named_media(..., error: type[AppError], root: Path, namespace: str | None = None) -> Path: + ... + ``` + Adjust the signature to match the real parameters used at each call site (inspect all three before finalizing). +- Refactor the three call sites to use the shared wrapper, preserving exact existing error types/messages raised on failure (this matters especially given Phase 3's error-taxonomy work — make sure this consolidation uses whatever the post-Phase-3 canonical/internal error categories are, not the pre-Phase-3 ones). + +## Validation + +- Run `pytest` (via the project's normal invocation, e.g. `uv run pytest`) after each consolidation step and ensure the full suite still passes unchanged — this is a refactor, so a full-suite regression is the primary correctness signal. +- Run `ruff check` and `ty check` and ensure no new issues are introduced. +- Do not change observable behavior anywhere (error messages, HTTP status codes, UI states, persisted values) as part of this consolidation — if you find you need a behavior change to complete an extraction cleanly, stop and flag it rather than silently changing behavior. +- Do not touch provenance/env config or ruff/ty baseline cleanup — that is Phase 5. + +Report back with: the final helper signatures actually implemented, all files changed per consolidation area, approximate lines removed per area (compare against the review's estimates), and final `pytest`/`ruff`/`ty` results. diff --git a/docs/phase5-codex-prompt.md b/docs/phase5-codex-prompt.md new file mode 100644 index 0000000..bf1e1fa --- /dev/null +++ b/docs/phase5-codex-prompt.md @@ -0,0 +1,39 @@ +You are working in the `transcription` repository (Python 3.12+, FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, asyncio). Follow `.github/instructions/services.instructions.md`, `.github/instructions/ui.instructions.md`, and `.github/instructions/error-handling.instructions.md` for any code you touch, and keep `docs/ver4/*` as canonical authority for intended behavior. Do not modify unrelated code. + +**Prerequisite:** This prompt assumes Phases 1-4 (`docs/phase1-codex-prompt.md` through `docs/phase4-codex-prompt.md`) are already merged. + +## Goal + +Implement **Phase 5 (Non-blocking governance/documentation depth)** from `docs/architecture-code-review-2026-08-20.md`, the final phase of the review's action plan: + +### 1. [MED-01] Provenance software context depends on undeclared ambient environment state +**Location:** `src/transcription/providers/evidence.py:144-154` + +**Problem:** `build_software_context` reads `TRANSCRIPTION_COMMIT` directly from `os.environ`, bypassing `Settings` and `.env.example` contract guards. Evidence exports can therefore vary by process environment in a way the repository's documented configuration surface does not declare or test. + +**Required fix — choose one, and justify your choice in the final report:** +- **Preferred:** Add `TRANSCRIPTION_COMMIT` (or a clearly named equivalent) as a proper field on the project's `Settings` class (in `src/transcription/config.py`), document it in `.env.example`, and update `build_software_context` in `src/transcription/providers/evidence.py` to read it from `Settings` instead of `os.environ` directly. Add/extend `tests/test_meta_contract_guards.py` so this setting is covered by whatever meta-contract guard already validates `Settings`/`.env.example` alignment. +- **Alternative (only if commit identity truly cannot be static config):** Derive the commit identity once at application startup (e.g. from `git rev-parse HEAD` or a build-time artifact) and store it on app state, injecting it into `build_software_context` via an explicit parameter rather than ambient lookup at call time. +- In either case, remove the direct `os.environ` read from `providers/evidence.py`, and ensure provenance inputs are explicit, versioned, and testable configuration — not ambient environment leakage. +- Add a test asserting that evidence/provenance export reflects the configured commit value deterministically (not dependent on unset/inconsistent env state). + +### 2. [LOW-01] Restore the static-analysis baseline to green and gate it in CI +**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:** `uv run ruff check` currently reports 5 issues and `uv run ty check` reports 12 diagnostics on mainline, even though the `pytest` suite is fully green. This means static-analysis debt already exists on the trunk, undermining confidence that future structural drift will be caught early. + +**Required work:** +- Run `uv run ruff check` and `uv run ty check` from the repo root and get the current full list of issues/diagnostics (do not assume the exact locations above are still accurate — code may have shifted since the review; re-derive the live list first). +- Fix every reported `ruff check` issue. Prefer the minimal correct fix over broad reformatting; do not run a repo-wide autofix/reformat that touches unrelated code. +- Fix every reported `ty check` diagnostic. Pay particular attention to the flagged areas: the health payload typing in `api/health.py`, and untyped/permissive test doubles in `tests/services/test_service_base.py` — these were specifically called out as notable in the review. +- Do not silence diagnostics with blanket `# type: ignore`/`# noqa` unless a fix is genuinely infeasible; if you must suppress one, add a one-line comment explaining why, scoped as narrowly as possible (single line, not file-level). +- Once both `ruff check` and `ty check` are clean, check whether this repo's CI configuration (e.g. `.github/workflows/*.yml`) already runs them as required gates alongside `pytest`. If not, add them as required steps so this baseline cannot silently regress again. Follow whatever CI tooling/style (uv, pre-commit, GitHub Actions) is already established in this repo — check `.pre-commit-config.yaml` and existing workflow files before adding anything new. + +## Validation + +- Run `uv run pytest` and ensure the full suite passes. +- Run `uv run ruff check` and confirm zero issues. +- Run `uv run ty check` (or the project's equivalent type-check command) and confirm zero diagnostics. +- If you added/modified CI configuration, verify the workflow YAML is syntactically valid and consistent with existing job structure (do not introduce a parallel/duplicate CI pipeline). + +Report back with: the Settings/provenance approach chosen for `TRANSCRIPTION_COMMIT` and why, the full list of ruff/ty issues fixed (before/after counts), any suppressions added and their justification, whether CI gating was added or already existed, and final `pytest`/`ruff`/`ty` results. diff --git a/docs/test-effectiveness-audit-2026-08-20.md b/docs/test-effectiveness-audit-2026-08-20.md deleted file mode 100644 index 177247a..0000000 --- a/docs/test-effectiveness-audit-2026-08-20.md +++ /dev/null @@ -1,45 +0,0 @@ -# Test Effectiveness Audit Report - -## 1. Executive Verdict - -- **Effective with Conditions** -- The suite has strong contract coverage for architecture governance, worker reliability, error taxonomy, evidence append-only semantics, and media safety. -- The largest confidence risk is a placeholder module with eight empty tests that always pass and contribute no regression signal. -- A smaller risk is several exception-path tests that assert only exception type and do not validate envelope/category/detail semantics. - -## 2. Contract Coverage Matrix - -| Contract | Guarding Tests | Signal Quality | Gap | Action | -| :--- | :--- | :--- | :--- | :--- | -| Service boundary isolation | `tests/test_service_boundaries.py` | Strong | None | Keep as-is | -| UI boundary isolation | `tests/test_ui_boundaries.py` | Strong | None | Keep as-is | -| Canonical docs/instruction authority + settings parity | `tests/test_meta_contract_guards.py` | Strong | None | Keep as-is | -| Error envelope taxonomy mapping | `tests/api/test_error_responses.py` | Strong | None | Keep as-is | -| Worker non-retriable stop + resilience behavior | `tests/test_worker.py`, `tests/services/test_workflows_reliability.py` | Strong | None | Keep as-is | -| Evidence append-only + candidate promotion invariants | `tests/services/test_v45_candidates.py` | Strong | None | Keep as-is | -| UI/API media path safety | `tests/test_media_path_safety.py`, `tests/ui/test_media_urls.py` | Strong | None | Keep as-is | -| Service base behavior contract | `tests/services/test_service_base.py` | **None (current tests empty)** | **High** | Replace placeholders with real assertions or remove file | - -## 3. Weak/Redundant Test Findings - -| Finding ID | Location | Why Low-Signal | Risk | Recommendation | -| :--- | :--- | :--- | :--- | :--- | -| TE-01 | `tests/services/test_service_base.py:6-35` | Contains eight `test_*` functions with docstrings only and no executable assertions. | High: false confidence and inflated pass count. | Replace with real behavior checks against `ServiceBase` session-scope semantics, or delete file until concrete tests exist. | -| TE-02 | `tests/test_engine_registry.py:36-37` | `test_disposing_an_unregistered_url_is_a_noop` asserts only “no exception.” It does not verify registry state invariants before/after call. | Medium: regression may survive if behavior changes silently without raising. | Assert that previously created engine/session-factory instances for other URLs remain unchanged after noop disposal path. | -| TE-03 | `tests/services/test_v45_candidates.py:90-94` | `pytest.raises(CandidatePromotionError)` validates type only; no assertions on message/category/suggestion for user-safe failure semantics. | Low-Med: weaker diagnostics contract protection. | Capture exception and assert critical error metadata fields to strengthen failure-path guarantees. | - -## 4. Prune/Strengthen Backlog - -| Task ID | Goal | Files | Acceptance Criteria | Validation | -| :--- | :--- | :--- | :--- | :--- | -| TE-T1 | Eliminate zero-signal placeholder tests | `tests/services/test_service_base.py`, `src/transcription/services/base.py` | No empty `test_*` functions remain; each test has behavior assertions that fail on meaningful `ServiceBase` regressions. | `uv run pytest tests/services/test_service_base.py` | -| TE-T2 | Strengthen noop disposal invariant test | `tests/test_engine_registry.py` | Noop disposal test verifies unaffected URL registries remain intact and disposed URL behavior is unchanged. | `uv run pytest tests/test_engine_registry.py` | -| TE-T3 | Strengthen exception-path semantics checks | `tests/services/test_v45_candidates.py` (and similar raise-only tests where high-value) | Exception tests assert key semantic fields (message/category/suggestion or equivalent domain signal), not only type. | `uv run pytest tests/services/test_v45_candidates.py` | - -## 5. Confidence Recommendation - -- **Go with Conditions** for test-confidence governance. -- Exit criteria: - 1. Complete TE-T1 (highest priority). - 2. Complete TE-T2. - 3. Apply TE-T3 at least on high-risk service error paths. diff --git a/docs/ver4/schema_v4.md b/docs/ver4/schema_v4.md index bed079b..312fa71 100644 --- a/docs/ver4/schema_v4.md +++ b/docs/ver4/schema_v4.md @@ -174,6 +174,9 @@ Index: | `source_id` | `UUID` | FK -> `source.id`, indexed | | `status` | `JobSourceStatus` | non-null enum, default `pending` | +Constraint: +- `UniqueConstraint(job_id, source_id)` named `uq_job_source_job_source` + ### `ExecutionAttempt` | Field | Type | Notes | diff --git a/src/transcription/db/models.py b/src/transcription/db/models.py index 744873e..9d23c17 100644 --- a/src/transcription/db/models.py +++ b/src/transcription/db/models.py @@ -360,6 +360,7 @@ class JobSource(SQLModel, table=True): """A single AI execution record for one source page.""" __tablename__ = "job_source" + __table_args__ = (UniqueConstraint("job_id", "source_id", name="uq_job_source_job_source"),) id: UUID = Field(default_factory=uuid4, primary_key=True) job_id: UUID = Field(foreign_key="job.id", index=True) diff --git a/src/transcription/services/jobs.py b/src/transcription/services/jobs.py index cfc8433..709c958 100644 --- a/src/transcription/services/jobs.py +++ b/src/transcription/services/jobs.py @@ -5,6 +5,7 @@ from datetime import datetime from uuid import UUID from sqlalchemy import func +from sqlalchemy import update from sqlmodel import col from sqlmodel import select from sqlmodel.ext.asyncio.session import AsyncSession @@ -182,21 +183,46 @@ class JobService(ServiceBase): concurrent workers never contend for the same job. """ async with self._session_scope(session) as _session: - query = ( - select(Job) - .where(Job.status == JobStatus.QUEUED) - # Break ties by id so "next" is stable when two rows share close timestamps. + dialect = _session.get_bind().dialect.name + if dialect == "postgresql": + query = ( + select(Job) + .where(Job.status == JobStatus.QUEUED) + # Break ties by id so "next" is stable when two rows share close timestamps. + .order_by(col(Job.date_created), col(Job.id)) + .limit(1) + .with_for_update(skip_locked=True) + ) + job = (await _session.exec(query)).first() + if job is None: + return None + job.status = JobStatus.PROCESSING + await self._finalize(session=_session, caller_session=session, refresh=(job,)) + return job + + now = datetime.now(UTC) + queued_job_id = ( + select(col(Job.id)) + .where(col(Job.status) == JobStatus.QUEUED) .order_by(col(Job.date_created), col(Job.id)) .limit(1) + .scalar_subquery() ) - if _session.get_bind().dialect.name == "postgresql": - query = query.with_for_update(skip_locked=True) + claim_statement = ( + update(Job) + .where(col(Job.id) == queued_job_id) + .where(col(Job.status) == JobStatus.QUEUED) + .values(status=JobStatus.PROCESSING, date_updated=now) + .returning(col(Job.id)) + ) + claimed_row = (await _session.exec(claim_statement)).first() + if claimed_row is None: + return None + claimed_job_id = claimed_row if isinstance(claimed_row, UUID) else claimed_row[0] - job = (await _session.exec(query)).first() + job = (await _session.exec(select(Job).where(Job.id == claimed_job_id))).first() if job is None: return None - - job.status = JobStatus.PROCESSING await self._finalize(session=_session, caller_session=session, refresh=(job,)) return job diff --git a/src/transcription/services/sources.py b/src/transcription/services/sources.py index cde59e3..b50f7d7 100644 --- a/src/transcription/services/sources.py +++ b/src/transcription/services/sources.py @@ -23,6 +23,7 @@ from pydantic import ValidationError from sqlalchemy import func from sqlalchemy import literal from sqlalchemy import tuple_ +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import async_sessionmaker from sqlmodel import col from sqlmodel import select @@ -317,7 +318,10 @@ class SourceService(ServiceBase): """Create a new job_source execution record in the database.""" async with self._session_scope(session) as _session: _session.add(job_source) - await self._finalize(session=_session, caller_session=session, refresh=(job_source,)) + try: + await self._finalize(session=_session, caller_session=session, refresh=(job_source,)) + except IntegrityError as exc: + raise self._job_source_conflict(job_id=job_source.job_id, source_id=job_source.source_id) from exc return job_source async def read_job_source(self, job_source_id: UUID, *, session: AsyncSession | None = None) -> JobSource: @@ -524,6 +528,10 @@ class SourceService(ServiceBase): if job_source is None: job_source = JobSource(job_id=job_id, source_id=source_id, status=outcome) _session.add(job_source) + try: + await _session.flush() + except IntegrityError as exc: + raise self._job_source_conflict(job_id=job_id, source_id=source_id) from exc else: job_source.status = outcome @@ -589,6 +597,14 @@ class SourceService(ServiceBase): await self._finalize(session=_session, caller_session=session, refresh=(job, source, job_source, attempt)) return job_source + @staticmethod + def _job_source_conflict(*, job_id: UUID, source_id: UUID) -> TranscriptionError: + return TranscriptionError( + f"Source {source_id} is already linked to Job {job_id}", + category=ErrorCategory.CONFLICT, + suggestion="Use the existing job-source link instead of creating a duplicate.", + ) + async def upsert_revision_for_source( self, *, diff --git a/tests/artifacts/transcriptions/Book_Two_-_page_02.jpg.txt b/tests/artifacts/transcriptions/Book_Two_-_page_02.jpg.txt index e6fa454..fb392dc 100644 --- a/tests/artifacts/transcriptions/Book_Two_-_page_02.jpg.txt +++ b/tests/artifacts/transcriptions/Book_Two_-_page_02.jpg.txt @@ -5,7 +5,7 @@ model: openai/gpt-5.3-codex [document body typewritten] BY WAY OF INTRODUCTION:- -These few paragraphs of introduotion [sic] may help you read BOOK 2 which covers a wider range than did BOOK 1 (Pioneer Days). +These few paragraphs of introduction may help you read BOOK 2 which covers a wider range than did BOOK 1 (Pioneer Days). BOOK 1 had 54 pages; 14 chapters. BOOK 2 has 70 pages; 18 chapters. BOOK 1 consisted largely of first generation family history. BOOK 2 throws more light on the second generation. Sidney promises a BOOK 3 and that may begin to do justice to the third generation. We suggest that Sidney get the help of Louis Shinn who has a chapter in this book (Chapter 16 - The Last 25 Years on the Doumecq Plains. Louis has the gift of seeing, recalling and telling. One sentence in his chapter gives a great tribute to the Doumecqers--so far as he knows no one on the Doumecq Plains went on relief during the depression. That in a nutshell shows the sturdy character of the residents of the Doumecq Plains. @@ -13,6 +13,6 @@ We promised in BOOK 1 that in BOOK 2 we would give the story of the trip of John Some who get this book will consider the group picture the best thing in the book. It took a lot of preliminary photographing to reduce some pictures, enlarge others and bring out the tin types. We wish that instead of 44 faces we could have given 88. Do not blame Ethel Cochran-Shinn for the selection. She furnished enough pictures but we had to take only part of them. We think there are great possibilities in reproducing old pictures. We wish we had a Pickard group. Some Pickard descendant may wish to make a collection. -We are much impressed with the future possibilities of getting a complete genealogy of the Pickard family. Mr. Cochran has a fine chapter on the Pickards but to date we have not had the pleasure of finding all of the family dates. We had intended to give more family data in this book but it takes time to get the correct dates. Often times it requires trips to cemeteries to get dates on the tombstones. Winter is no time to collect dates on tombstones. +We are much impressed with the future possibilities of getting a complete geneology [sic] of the Pickard family. Mr. Cochran has a fine chapter on the Pickards but to date we have not had the pleasure of finding all of the family dates. We had intended to give more family data in this book but it takes time to get the correct dates. Often times it requires trips to cemeteries to get dates on the tombstones. Winter is no time to collect dates on tombstones. ~2~ diff --git a/tests/artifacts/transcriptions/Omie_Writes_Home.pdf.txt b/tests/artifacts/transcriptions/Omie_Writes_Home.pdf.txt index faac9d0..1096cc7 100644 --- a/tests/artifacts/transcriptions/Omie_Writes_Home.pdf.txt +++ b/tests/artifacts/transcriptions/Omie_Writes_Home.pdf.txt @@ -3,6 +3,9 @@ provider: openrouter model: openai/gpt-5.3-codex --- [document body typeset] +JOHN E. COCHRAN +FAMILY ASSOCIATION + Family Only Home | Sibling's Stories | 1st Cousins | Ancestor's Stories | Reunion History | Next Reunion | JECMEF @@ -38,8 +41,7 @@ along the icebergs and the walrus were 'heisted' on board by the use of the cran tons of freight and the beasts were so huge that they made the pulleys just creak. They were over 12 feet long, as big around as three cows, had no feet but toenails on their flippers or flappers, no head but their body just suddenly ended with a hole for a mouth and big bristles - -[photograph of people standing outdoors in snow] +[photograph of people standing in snow] all around it similar to a currycomb in coarseness; no ears but huge tusks of ivory. They are the most repulsive looking animals imaginable and tho I have always read about them I never @@ -63,9 +65,9 @@ bears with faces. I guess they had never seen white women, not so many at one ti We went ashore in 2 life boats and a launch pulling them. On the launch was a six piece band playing and the rear of the last life boat was the movie man. 'Twas very thrilling. -The other place we stopped was at Whalen, a trading post in Siberia. There these 'towerists' +The other place we stopped was at Whalen, a trading post in Siberia. There these 'towerists'[sic] went wild. They rushed helter-skelter, hither and thither, here and there, trying to find -something to buy. Prices raised right before your eyes. One would but something for $1.00 +something to buy. Prices raised right before your eyes. One would but[sic] something for $1.00 and the next might have to pay $2.00, $4.00 or $10.00. That made no difference. They had to have it. One man I was sort of taking care of, tho he had his son along for the purpose, bought 2 ivory tusks, 1 pup, 2 moccasins, 3 or 4 billikens, 6 or 8 ivory and silver rings, one @@ -81,7 +83,7 @@ We got home yesterday morning at 5 a.m. but missed the first lighter in so had t until 2:30. The girls had prepared a big meal for us and invited up the Hartfords and then let us talk. Miss Saville talked quite a bit. Any how if you folks don't like this I don't care, it is all I had to write about and I know Buster'ud listen anyway and I'd soak ole Peter's head if he -didn't and Polly would in my lap and I don't know much about the youngest one of yours so +didn't and Polly would [sit] in my lap and I don't know much about the youngest one of yours so likely he would be squawling. But we did surely enjoy our trip and were gone just long enuf. I expect there were 150 passengers on board and almost or more of the crew and helpers. We @@ -115,4 +117,4 @@ Reprinted from Cochran Chronicles, Volume 9, Number 1, November 1986 Up -jecochranclan.org ~ Contact webmaster +jecochranclan.org - Contact webmaster diff --git a/tests/artifacts/transcriptions/Rod_Moser_Letter_-_p1.jpg.txt b/tests/artifacts/transcriptions/Rod_Moser_Letter_-_p1.jpg.txt index 16bb391..b52782e 100644 --- a/tests/artifacts/transcriptions/Rod_Moser_Letter_-_p1.jpg.txt +++ b/tests/artifacts/transcriptions/Rod_Moser_Letter_-_p1.jpg.txt @@ -3,30 +3,31 @@ provider: openrouter model: openai/gpt-5.3-codex --- [document body mixed] -JOHN ISBILL R. T. MOSER +JOHN ISBILL R. T. MOSER ISBILL & MOSER DEALERS IN GENERAL MERCHANDISE -Vonore, Tenn., [handwritten: July 27-] 191[handwritten: 2] +Vonore, Tenn., [handwritten: May 27-] 191[handwritten: 3?] [handwritten: Dear Uncle Aunt & Cousins I was at home a few nights ago & saw a letter from your folks, so I decided to write you -a few lines in regards of -I am [contemplating?] a +a few lines myself & +am contemplating a trip out west next summer -& want [lots?] of [olders?] to go -where I am from. +& want lots of places to go +where I can stop. + Am getting -up in years & wondering, +up in years & unmarried, so you see the object of -my trip, is to get a wife -If there is any old maids +my trip, is to get a wife. +If there is any old maid or widows out there I -want you to [hire?] them -at [one?] [find?] me at there +want you to [illegible] them +at [illegible] me at them as soon as I get there.] diff --git a/tests/services/test_job_service.py b/tests/services/test_job_service.py index ad7f9bb..89bf7d3 100644 --- a/tests/services/test_job_service.py +++ b/tests/services/test_job_service.py @@ -1,3 +1,4 @@ +import asyncio from datetime import UTC from datetime import datetime from datetime import timedelta @@ -153,10 +154,33 @@ class TestJobService: finally: event.remove(bind, "before_cursor_execute", capture) - selects = [item for item in statements if item.lstrip().upper().startswith("SELECT")] - assert len(selects) == 1, selects - assert "LIMIT" in selects[0].upper() - assert "JOIN" not in selects[0].upper() + claim_sql = [ + item + for item in statements + if item.lstrip().upper().startswith(("SELECT", "UPDATE")) + ] + assert len(claim_sql) == 1, claim_sql + assert "LIMIT" in claim_sql[0].upper() + assert "JOIN" not in claim_sql[0].upper() + + @pytest.mark.asyncio + async def test_claim_next_queued_job_is_atomic_on_sqlite( + self, + job_service: JobService, + document_service: DocumentService, + ): + document = await document_service.create_document(Document(id=uuid4(), name="atomic-claim-doc")) + queued_job = await job_service.create_job(Job(document_id=document.id, status=JobStatus.QUEUED)) + + async def claim_once(): + claimed = await job_service.claim_next_queued_job() + return claimed.id if claimed is not None else None + + first_claim, second_claim = await asyncio.gather(claim_once(), claim_once()) + + assert [first_claim, second_claim].count(queued_job.id) == 1 + assert [first_claim, second_claim].count(None) == 1 + assert await job_service.claim_next_queued_job() is None @pytest.mark.asyncio async def test_create_job_persists_provider_and_model( diff --git a/tests/services/test_v2_crud.py b/tests/services/test_v2_crud.py index 10960ba..63fdd56 100644 --- a/tests/services/test_v2_crud.py +++ b/tests/services/test_v2_crud.py @@ -14,6 +14,7 @@ from transcription.errors import ErrorCategory from transcription.services.documents import DocumentDeleteBlockedError from transcription.services.documents import DocumentService from transcription.services.errors import SourceDeleteBlockedError +from transcription.services.errors import TranscriptionError from transcription.services.evidence import EvidenceService from transcription.services.jobs import JobService from transcription.services.people import PeopleError @@ -190,6 +191,37 @@ async def test_transcription_service_job_source_crud_uses_caller_session(default assert len(await transcriptions.list_job_sources(job_id=job.id)) == 0 +@pytest.mark.asyncio +async def test_create_job_source_rejects_duplicate_job_source_membership(default_session_factory): + documents = DocumentService(session_factory=default_session_factory) + jobs = JobService(session_factory=default_session_factory) + transcriptions = SourceService(session_factory=default_session_factory) + + document = await documents.create_document(Document(id=uuid4(), name="duplicate-job-source-doc")) + job = await jobs.create_job(Job(document_id=document.id)) + source = await transcriptions.create_source( + Source( + document_id=document.id, + page_number=1, + upload_name="duplicate.jpg", + filename="duplicate.jpg", + file_path="uploads/duplicate.jpg", + file_hash="d" * 64, + file_size_bytes=1, + ) + ) + await transcriptions.create_job_source( + JobSource(job_id=job.id, source_id=source.id, status=JobSourceStatus.PENDING) + ) + + with pytest.raises(TranscriptionError) as duplicate: + await transcriptions.create_job_source( + JobSource(job_id=job.id, source_id=source.id, status=JobSourceStatus.PENDING) + ) + + assert duplicate.value.category == ErrorCategory.CONFLICT + + @pytest.mark.asyncio async def test_document_detail_loads_linked_person_relationship(default_session_factory): documents = DocumentService(session_factory=default_session_factory) diff --git a/tests/test_db.py b/tests/test_db.py index d54ae04..fbbd3af 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -123,14 +123,19 @@ async def test_create_all_declares_hot_path_indexes(tmp_path): await create_all(engine=runtime.engine) async with runtime.engine.connect() as connection: - def collect(sync_connection) -> dict[str, list[list[str]]]: + def collect(sync_connection) -> tuple[dict[str, list[list[str]]], list[list[str]]]: database = inspect(sync_connection) - return { + indexes = { table: [index["column_names"] for index in database.get_indexes(table)] for table in ("job", "source", "job_source", "document", "document_person") } + job_source_unique = [ + constraint["column_names"] + for constraint in database.get_unique_constraints("job_source") + ] + return indexes, job_source_unique - indexes = await connection.run_sync(collect) + indexes, job_source_unique = await connection.run_sync(collect) assert ["status", "date_created"] in indexes["job"] assert ["document_id"] in indexes["job"] @@ -138,6 +143,7 @@ async def test_create_all_declares_hot_path_indexes(tmp_path): assert ["preferred_execution_attempt_id"] in indexes["source"] assert ["job_id"] in indexes["job_source"] assert ["source_id"] in indexes["job_source"] + assert ["job_id", "source_id"] in job_source_unique assert ["document_type_id"] in indexes["document"] for column in ("document_id", "person_id", "role_id"): assert [column] in indexes["document_person"] diff --git a/tests/test_models.py b/tests/test_models.py index 721b620..ffe3fd6 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -238,6 +238,22 @@ class TestJobSourceModel: assert fetched.status == JobSourceStatus.PENDING assert set(JobSource.model_fields) == {"id", "job_id", "source_id", "status"} + def test_job_source_membership_is_unique_per_job_and_source(self, session): + document = _persist_document(session) + job = _persist_job(session, document) + source = _persist_source(session, document) + _persist_job_source(session, job, source) + + session.add( + JobSource( + job_id=job.id, + source_id=source.id, + status=JobSourceStatus.PENDING, + ) + ) + with pytest.raises(IntegrityError): + session.commit() + class TestRelationships: def test_document_exposes_jobs_sources_and_people(self, session): diff --git a/tools/migrate_v47_to_v48.py b/tools/migrate_v47_to_v48.py new file mode 100644 index 0000000..373754f --- /dev/null +++ b/tools/migrate_v47_to_v48.py @@ -0,0 +1,196 @@ +"""One-time migration for V4.8 queue-membership uniqueness. + +This migration enforces the V4 requirement that each ``(job_id, source_id)`` +pair appears at most once in ``job_source``. + +Steps: +1. Detect duplicate ``job_source`` rows per ``(job_id, source_id)``. +2. Keep one row per pair (prefer the row with the latest attempt evidence). +3. Re-point ``execution_attempt.job_source_id`` from removed duplicates to the + kept row. +4. Delete duplicate ``job_source`` rows. +5. Add uniqueness enforcement for ``(job_id, source_id)``. + +Usage:: + + python tools/migrate_v47_to_v48.py --dry-run + python tools/migrate_v47_to_v48.py +""" + +from __future__ import annotations + +import argparse +from collections import defaultdict +from collections.abc import Sequence +from datetime import UTC +from datetime import datetime +from uuid import UUID + +from sqlalchemy import bindparam +from sqlalchemy import create_engine +from sqlalchemy import func +from sqlalchemy import inspect as sqlalchemy_inspect +from sqlalchemy import select +from sqlalchemy import text +from sqlalchemy import update +from sqlalchemy.engine import Connection +from sqlmodel import SQLModel + +from transcription.config import Settings +from transcription.config import get_settings +from transcription.db import models as _models # noqa: F401 (registers metadata tables) +from transcription.db.engine import get_database_url + +UNIQUE_NAME = "uq_job_source_job_source" + + +def _sync_url(settings: Settings) -> str: + """Return a sync URL for direct SQLAlchemy Core access.""" + return get_database_url(settings).replace("+aiosqlite", "").replace("+asyncpg", "").replace("+psycopg", "") + + +def _uniqueness_already_enforced(connection: Connection) -> bool: + inspector = sqlalchemy_inspect(connection) + unique_constraints = inspector.get_unique_constraints("job_source") + if any(constraint.get("name") == UNIQUE_NAME for constraint in unique_constraints): + return True + indexes = inspector.get_indexes("job_source") + return any( + index.get("name") == UNIQUE_NAME and index.get("unique") is True + for index in indexes + ) + + +def _choose_keeper( + candidates: list[dict[str, object]], +) -> dict[str, object]: + """Keep the row with latest attempt activity, then lexicographically greatest UUID.""" + + def key(item: dict[str, object]) -> tuple[datetime, str]: + latest_attempt = item["latest_attempt_at"] + attempt_key = latest_attempt if isinstance(latest_attempt, datetime) else datetime.min.replace(tzinfo=UTC) + return (attempt_key, str(item["job_source_id"])) + + return max(candidates, key=key) + + +def deduplicate_job_source_membership(connection: Connection, *, dry_run: bool) -> tuple[int, int]: + """Return ``(pairs_deduplicated, rows_deleted)``.""" + job_source = SQLModel.metadata.tables["job_source"] + execution_attempt = SQLModel.metadata.tables["execution_attempt"] + + duplicate_pairs = ( + connection.execute( + select(job_source.c.job_id, job_source.c.source_id) + .group_by(job_source.c.job_id, job_source.c.source_id) + .having(func.count(job_source.c.id) > 1) + ) + .mappings() + .all() + ) + if not duplicate_pairs: + return (0, 0) + + rows_by_pair: dict[tuple[UUID, UUID], list[dict[str, object]]] = defaultdict(list) + duplicate_memberships = ( + connection.execute( + select( + job_source.c.id.label("job_source_id"), + job_source.c.job_id, + job_source.c.source_id, + func.max(execution_attempt.c.created_at).label("latest_attempt_at"), + ) + .select_from( + job_source.outerjoin( + execution_attempt, execution_attempt.c.job_source_id == job_source.c.id + ) + ) + .group_by(job_source.c.id, job_source.c.job_id, job_source.c.source_id) + ) + .mappings() + .all() + ) + duplicate_key_set = {(row["job_id"], row["source_id"]) for row in duplicate_pairs} + for row in duplicate_memberships: + key = (row["job_id"], row["source_id"]) + if key in duplicate_key_set: + rows_by_pair[key].append(dict(row)) + + rows_deleted = 0 + for key, rows in sorted(rows_by_pair.items(), key=lambda item: (str(item[0][0]), str(item[0][1]))): + keeper = _choose_keeper(rows) + keeper_id = keeper["job_source_id"] + duplicate_ids = [row["job_source_id"] for row in rows if row["job_source_id"] != keeper_id] + rows_deleted += len(duplicate_ids) + print( + f" dedupe pair job_id={key[0]} source_id={key[1]} " + f"keep={keeper_id} drop={','.join(str(item) for item in duplicate_ids)}" + ) + if dry_run or not duplicate_ids: + continue + connection.execute( + update(execution_attempt) + .where(execution_attempt.c.job_source_id.in_(bindparam("duplicate_ids", expanding=True))) + .values(job_source_id=keeper_id), + {"duplicate_ids": duplicate_ids}, + ) + connection.execute( + job_source.delete().where(job_source.c.id.in_(bindparam("duplicate_ids", expanding=True))), + {"duplicate_ids": duplicate_ids}, + ) + + return (len(duplicate_pairs), rows_deleted) + + +def add_job_source_uniqueness(connection: Connection, *, dry_run: bool) -> None: + """Create uniqueness enforcement for ``(job_id, source_id)``.""" + if _uniqueness_already_enforced(connection): + print(f" uniqueness already enforced ({UNIQUE_NAME})") + return + + dialect = connection.dialect.name + if dialect == "postgresql": + statement = text( + f'alter table "job_source" add constraint "{UNIQUE_NAME}" unique ("job_id", "source_id")' + ) + else: + statement = text( + f'create unique index "{UNIQUE_NAME}" on "job_source" ("job_id", "source_id")' + ) + print(f" applying {UNIQUE_NAME}") + if not dry_run: + connection.execute(statement) + + +def migrate(*, settings: Settings, dry_run: bool) -> None: + engine = create_engine(_sync_url(settings)) + try: + with engine.begin() as connection: + print("Step 1: deduplicate job_source membership") + pair_count, rows_deleted = deduplicate_job_source_membership(connection, dry_run=dry_run) + print(f" duplicate pairs={pair_count} rows_deleted={rows_deleted}") + + print("Step 2: enforce unique (job_id, source_id)") + add_job_source_uniqueness(connection, dry_run=dry_run) + finally: + engine.dispose() + + if dry_run: + print("\nDry run: nothing was written.") + else: + print("\nDone.") + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--dry-run", action="store_true", help="Report planned changes without writing") + args = parser.parse_args(argv) + + settings = get_settings() + print(f"Target: {_sync_url(settings)}") + migrate(settings=settings, dry_run=args.dry_run) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())