claude-sonnet-5 review: Phase 1 implemented by gpt-5.3-codex

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.