From f15c9834e423376054b35ba6fef5759641098ab8 Mon Sep 17 00:00:00 2001 From: Jim Lancaster <40281233+zoltan57@users.noreply.github.com> Date: Sun, 23 Aug 2026 19:25:39 -0500 Subject: [PATCH] Remove completed remediation handoff and mark the review closed The handoff brief was a work order for phases 2-5. That work is done, so the document now describes a future that already happened and would misdirect anyone who found it. The review report itself had the same problem in weaker form: its findings read as open. Adds a status banner marking it closed and retained for reasoning only. The banner also records that two of its recommendations were wrong on contact. The HIGH-03 fix as written would have stripped root-cause data from ExecutionAttempt provenance, and the HIGH-01 fix had to preserve per-page durability the report never mentioned. Leaving that unstated invites someone to 'restore' the report's version later. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reviews/2026-08-23-code-review.md | 10 + .../reviews/2026-08-23-remediation-handoff.md | 258 ------------------ docs/reviews/README.md | 9 +- 3 files changed, 14 insertions(+), 263 deletions(-) delete mode 100644 docs/reviews/2026-08-23-remediation-handoff.md diff --git a/docs/reviews/2026-08-23-code-review.md b/docs/reviews/2026-08-23-code-review.md index 1da10e5..2e64750 100644 --- a/docs/reviews/2026-08-23-code-review.md +++ b/docs/reviews/2026-08-23-code-review.md @@ -8,6 +8,16 @@ **Escalations applied:** `.github/skills/evidence-provenance-auditor/skill.md`, `.github/skills/test-effectiveness-auditor/skill.md` **Scope:** 77 Python modules / ~13k LOC under `src/transcription`, 57 test files (377 collected non-external tests), 23 documents under `docs/`, 9 active rule files. +> **Status: closed.** Every finding below was remediated in the phases following this +> review. This document is retained as a record of the reasoning, **not** as a list of +> open work, and it is not canonical authority. +> +> Two recommendations were wrong on contact and were corrected during implementation: +> the HIGH-03 fix as written would have stripped root-cause data from `ExecutionAttempt` +> provenance, and the HIGH-01 fix needed to preserve per-page durability that the report +> did not mention. Where this text and the current code or guard tests disagree, the code +> and tests are correct. + ### Verification commands and outcomes | Command | Outcome | diff --git a/docs/reviews/2026-08-23-remediation-handoff.md b/docs/reviews/2026-08-23-remediation-handoff.md deleted file mode 100644 index 79edeb8..0000000 --- a/docs/reviews/2026-08-23-remediation-handoff.md +++ /dev/null @@ -1,258 +0,0 @@ -# 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 - -```powershell -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[]` 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 - -```powershell -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. diff --git a/docs/reviews/README.md b/docs/reviews/README.md index 8618ce3..809df91 100644 --- a/docs/reviews/README.md +++ b/docs/reviews/README.md @@ -15,8 +15,7 @@ Naming: `-code-review.md` for review reports, and ## Current - [`2026-08-23-code-review.md`](./2026-08-23-code-review.md) — full review. 0 critical, - 4 high, 5 medium, 11 low. -- [`2026-08-23-remediation-handoff.md`](./2026-08-23-remediation-handoff.md) — **start here - to continue the remediation work.** Phase 1 (all 4 high findings) is complete as of commit - `de18c2e`; the handoff covers Phases 2-5 with per-task acceptance criteria, the verification - baseline, and the environment gotchas needed to avoid re-deriving them. + 4 high, 5 medium, 11 low. **All findings remediated.** Retained as a record of the + reasoning, not as a list of open work. Note that a few of its recommendations were + wrong on contact and were corrected during implementation; the code and the guard + tests are authoritative over the report text.