transcription/docs/reviews/2026-08-23-remediation-handoff.md

Handoff Brief — Phases 2-5 of the 2026-08-23 Code Review

Repo: C:\Github\transcription · Branch: traumatized · Baseline commit: de18c2e Source of truth: docs/reviews/2026-08-23-code-review.md (§9 Prioritized Action Plan)

Phase 1 is done and committed. This brief covers everything after it.

Status note. This is a dated, non-canonical artifact, like everything under docs/reviews/**. It records a plan, not a contract. Where it disagrees with .github/instructions/** or the canonical docs, they win.

---

1. Environment — read this first

• uv project on Windows / PowerShell. ruff, ty, and pytest are not on PATH. Always prefix with uv run.
• PowerShell has no heredoc. Don't write python - <<'PY'. Use python -c "..." or pipe a single-quoted here-string (@' … '@ | python -).
• && only chains external commands in PowerShell. Use ; before PowerShell keywords.
• Ruff config (ruff.toml): line length 120, force-single-line = true — one import per line. Never combine imports.
• # noqa: PLR0915 / PLR1702 is established repo convention; don't strip existing ones.

2. Mandatory reading before editing src/transcription/**

The repo's instruction table requires these before source edits. They are contracts, not suggestions:

• .github/instructions/services.instructions.md — transaction boundaries, model ownership, no service-to-service imports
• .github/instructions/error-handling.instructions.md — error categories, retriability, user-safe messaging
• .github/instructions/ui.instructions.md — page/component boundaries
• .github/instructions/documentation-sync.instructions.md — docs must be updated in the same change when contracts or behavior change

3. Verification commands

uv run ruff check .                    # must be clean
uv run pytest -q -m "not external"     # must be 381+ passing
uv run ty check                        # baseline is exactly 10 diagnostics

The ty baseline is 10, and all 10 are false positives — SQLModel/SQLAlchemy column descriptors typed as UUID/datetime/bool, so .is_(), .asc(), func.count(), and group_by() appear invalid. They are in services/photos.py (8) and tests/test_storage_reconciliation.py (2). Do not "fix" these by changing code. Handling them is task P2-1 below, and the fix is suppression comments, not code edits.

Hard-won gotcha: typing.Mapping trips ruff's deprecated-import, and collections.abc.Mapping doesn't satisfy ty for SQLAlchemy row results. Sequence[RowMapping] + RowMapping is the only spelling that satisfies both. Don't rediscover this.

---

4. What Phase 1 changed (context you need)

Commit de18c2e. Four things, all with tests:

1. workflows.py commit boundary. process_queued_job now commits every page except the last individually, then defers the final page's write into _finalize_batch_outcome so it shares the terminal-status transaction.

   • _finalize_batch_outcome gained a final_page kwarg. If you touch this function, that parameter is load-bearing.
   • Two invariants are in tension here — preserve both. Intermediate pages must stay individually durable (guarded by test_workflows_reliability.py::...::test_transcribed_page_is_committed_before_next_provider_call_finishes), and the final page must be atomic with the terminal status (guarded by tests/integration/test_pipeline_atomicity.py). Do not collapse the whole batch into one transaction to simplify things — that breaks multi-page durability.

2. AppError gained an internal-only detail field. message is user/API-facing and must stay generic; detail carries the root cause and flows into evidence records via format_error_detail and into logs. Documented in docs/error_handling.md §"Message vs detail split". When adding error paths: never put exception text into message.

3. 8 ui.notify error sites replaced with show_error in home_page.py / people_page.py.

4. New AST guard test_ui_boundaries.py::test_no_page_hand_rolls_error_notifications.

---

5. Phase 2 — Enforcement hardening

P2-1 · ty suppression policy, then make the hook blocking

Report ref: LOW-05 · Effort: M

.pre-commit-config.yaml currently runs ty in advisory mode (a Python subprocess wrapper that forces sys.exit(0)) because of the 10 known false positives. Net effect: a genuine new type error prints alongside the known 10 and blocks nothing.

1. Add a targeted # ty: ignore[<rule>] at each of the 10 sites, each with a one-line comment explaining it's a SQLAlchemy descriptor false positive.
2. Confirm uv run ty check reports 0.
3. Flip the pre-commit hook to blocking (drop the sys.exit(0) wrapper).
4. Document the policy in docs/ — when a suppression is acceptable and what the comment must say.

Acceptance: uv run ty check → 0 diagnostics; introducing a deliberate type error fails git commit; revert the deliberate error afterward.

P2-2 · ruff format enforcement

Report ref: LOW-06 · Effort: S

~35 files have formatting drift. Two separate commits, in this order:

1. uv run ruff format . — formatting only, no other changes in this commit.
2. Add ruff format --check to .pre-commit-config.yaml.

Keeping these separate matters: a mixed commit makes the formatting noise unreviewable.

Acceptance: uv run ruff format --check . clean; full suite still 381+.

P2-3 · Enable ruff ruleset G (flake8-logging-format)

Report ref: LOW-09 · Effort: S

f-strings in logging calls format eagerly regardless of level and break template grouping in structured backends. Known instance: workflows.py:193. Enable G in ruff.toml, then convert offenders to %s lazy args: logger.error("Job %s failed.", job.id).

Acceptance: uv run ruff check . clean with G enabled.

---

6. Phase 3 — Reliability & concurrency

P3-1 · Periodic stale-job recovery

Report ref: MED-01 · Effort: M

requeue_stale_processing_jobs has exactly one caller — app.py:79, in the lifespan startup handler. There is no runtime re-check. The threshold reuses worker_provider_timeout_seconds (30.0s, config.py:116).

The failure mode: a job orphaned <30s before a fast restart fails the staleness predicate at the only moment recovery runs, so it stays PROCESSING forever (the worker only claims QUEUED). Restarts are exactly when orphans are created, so the recovery window is systematically misaligned with the failure it exists to handle.

1. Add worker_stale_job_seconds to Settings (don't keep overloading the provider timeout — they need independent tuning). Update .env.example in the same change (required by services.instructions.md).
2. Run the sweep periodically in the worker loop, in addition to the startup call.
3. Test: a job left PROCESSING past the threshold is requeued without a restart.

P3-2 · Gate retries on error category

Report ref: MED-02 · Effort: S

workflows.py:184-194 gates only on job.retry_count < settings.worker_max_retries. It never consults error_category or AppError.retriable, so validation / not_found / conflict failures would retry to exhaustion, burning provider quota on calls that cannot succeed.

Latent today because worker_max_retries defaults to 0 — which is exactly why this must be fixed before anyone raises that value. Add backoff too; retries currently requeue immediately.

Acceptance: a validation-category failure is not requeued even with worker_max_retries=1.

P3-3 · Shutdown budget derived from provider timeout

Report ref: MED-04 · Effort: S

worker.py:146 waits 2.0s for the worker task, but an in-flight provider call may run 30s and the stop event is only checked between jobs. Derive the budget from worker_provider_timeout_seconds plus a small grace. Document the relationship to the container termination grace period in docs/production-runbook.md.

P3-4 · Handle IntegrityError on the attempt-number flush

Report ref: MED-03 · Effort: M

sources.py:540-546 computes MAX(attempt_number) + 1; uq_execution_attempt_number enforces uniqueness. The sibling JobSource insert catches IntegrityError at sources.py:531-534, but the attempt flush() at sources.py:587 does not — a race loses an evidence row.

Not reachable today (single worker, sequential sources). It becomes reachable the moment a second worker replica is deployed — treat this as a hard precondition for horizontal scaling and note that in docs/production-runbook.md.

Mirror the JobSource handling: catch, recompute, retry bounded, raise a domain error on exhaustion.

P3-5 · Move blocking work off the event loop

Report ref: LOW-01, LOW-02, LOW-03 · Effort: S

• store.py:401 — hashlib.sha256(file_bytes) is CPU-bound on the loop. Wrap in asyncio.to_thread.
• homepage_store.py:25,32 — sync file I/O called from async page handlers. Same fix.
• app.py:62 — poll_interval_seconds=1.0 hardcoded. Move to Settings; update .env.example.

Every other I/O path already uses to_thread (media_storage.py:43, normalization.py:117, photos.py:176, sources.py:740,753) — follow those.

---

7. Phase 4 — Consolidation

P4-1 · Single transaction entry point

Report ref: MED-05 · Effort: M

workflows.py opens transactions via services.jobs._session_scope() and services.sources._session_scope() — private members of two different services. This is the mechanism that made HIGH-01 easy to introduce: nothing in the design signals that two scopes are being opened for one logical unit of work.

Introduce unit_of_work(services, session) (proposed signature in review §7), migrate workflows.py onto it, then add an AST guard to test_service_boundaries.py forbidding _session_scope access outside its owning module. Note the existing test checks imports, not attribute access, so it can't currently see this.

Do not attempt this before Phase 1's tests are green in your working tree — it touches the same functions.

P4-2 · Extract shared helpers

Report ref: §7 · Effort: M

run_blocking and insert_with_sequence_retry, per the review's proposed signatures. Do this after P3-4 and P3-5, so the call sites exist.

P4-3 · Prune low-signal tests

Report ref: LOW-10, LOW-11 · Effort: S

Full table in review §6 "Prune/strengthen backlog". Highlights:

• test_traceability.py:54-57 — asserts properties of dict literals in the same file.
• test_pipeline_flow.py:135-140,446-452 — assert processed is True is unfalsifiable (read_job raises rather than returning None).
• test_orphan_sweep.py:119 — >= 200 snapshot threshold tolerates ±40 drift.
• test_workflows_reliability.py:157-196 — real time.sleep(0.40) with only 10% slack; flaky under CI load.

---

8. Phase 5 — Governance

• P5-1 — Extend test_orphan_sweep.py beyond module-level definitions to public methods (LOW-08); seed KNOWN_ORPHANS with current results to keep it non-breaking.
• P5-2 — Resolve the 4 "uncertain" orphans (LOW-07). Note summarize_error should now be reachable — consider using it, or delete it.
• P5-3 — Log incomplete request manifests instead of silently returning None (openrouter.py:347, LOW-04).
• P5-4 — Consider inverting the UI boundary check from a forbidden-list to an allowlist; a future persistence helper under a new name currently escapes it.

---

9. Ground rules

1. Red first. For any behavioral fix, write the test, run it, observe it fail, then fix. That's how Phase 1 caught that its own first HIGH-03 attempt was wrong.
2. One phase per commit series. Don't mix P2-2's formatting sweep with logic changes.
3. Docs in the same change. Required by documentation-sync.instructions.md whenever contracts, behavior, or Settings change. Settings changes additionally require .env.example updates in the same commit.
4. Don't widen the NiceGUI pin. nicegui==3.13.0 is a deliberate release-stability decision recorded in docs/production-runbook.md. It is explicitly not a defect.
5. docs/reviews/** is not canonical. It's a dated artifact; don't treat it as a contract the way docs/schema.md or the instruction files are.
6. Ask before scope-expanding. If a fix seems to require restructuring beyond its task, stop and confirm — that's the signal a Phase boundary is being crossed.

10. Suggested first command

cd C:\Github\transcription
git log --oneline -3
uv run ruff check . ; uv run pytest -q -m "not external" ; uv run ty check

Confirm the baseline (clean ruff, 381+ passing, exactly 10 ty diagnostics) before changing anything. If that doesn't reproduce, stop and report rather than proceeding.