4 Commits
Author SHA1 Message Date
Jim LancasterandCopilot App f15c9834e4 Remove completed remediation handoff and mark the review closed
Quality Gate / gate (push) Failing after 47s
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 <[email protected]>
2026-08-23 19:25:39 -05:00
Jim LancasterandCopilot App 2c59cbd2c7 Add AGENTS.md to make repo context auto-discoverable
Nothing in this repo was auto-read by an agent at the start of a session.
The .github/instructions files only attach once a matching file is edited,
which is too late to steer strategy, and the canonical docs set is not
discoverable without already knowing to look for it.

AGENTS.md routes rather than duplicates: it states the authority order
(docs/* first, docs/reviews/** explicitly non-canonical), the uv-only
command set, the test-enforced boundaries, and the change protocol.

The Traps section records failure modes this codebase has actually produced
rather than generic advice: the message/detail split that leaked paths in one
direction and degraded provenance in the other, the two competing atomicity
invariants in workflows.py where the obvious simplification breaks multi-page
durability, and the habit of changing a shared symbol without enumerating its
consumers.

Verified: cited test paths exist, full suite passes, ruff/ty clean.

Co-authored-by: Copilot App <[email protected]>
2026-08-23 19:23:26 -05:00
Jim LancasterandCopilot App 5ff66a8c40 Align python-reviewer agent with the python-code-reviewer skill
The agent file had drifted from the skill it delegates to, in two ways that
would corrupt a review run.

Report target: the agent said write reports to ./docs, but the skill targets
./docs/reviews/<date>-code-review.md and explicitly marks docs/reviews/** as
non-canonical. Following the agent would place a dated, opinionated review
inside the canonical authority set that findings are supposed to resolve
against.

Verification commands: the agent said run 'ruff check', 'pytest', and 'ty'.
None are on PATH in this uv project, so an agent following its own instruction
gets command-not-found and is pushed toward guessing instead of verifying.

The agent now defers to the skill for all specifics rather than restating them,
which is what let the two copies drift apart. Also carries forward the
consumer-tracing rule and states the read-only scope explicitly.

Co-authored-by: Copilot App <[email protected]>
2026-08-23 19:17:33 -05:00
Jim LancasterandCopilot App 626b5d4b10 Harden python-code-reviewer skill with lessons from executing its own review
Three gaps surfaced by implementing the 2026-08-23 review's recommendations.

1. Recommendations were never verified the way claims were. The report's fix for
   the error path leak would have stripped root-cause data from evidence records,
   because the review traced one consumer of AppError.message and missed that
   format_error_detail writes it to ExecutionAttempt.error_detail. Adds workflow
   step 9 (validate recommendations against consumers), a Blast Radius field on
   findings, and the worked example so the failure mode is concrete.

2. Fixes that sit between competing invariants were not flagged. The atomicity
   recommendation did not note that per-page durability and terminal-status
   atomicity pull in opposite directions, so the obvious simplification silently
   breaks multi-page durability. Recommendations must now name both invariants,
   the test guarding each, and the over-correction to avoid.

3. Severity could not express reachability. Two findings were latent behind a
   default setting and a single-instance deployment, which is a sequencing
   constraint: they must be fixed before the change that makes them live. Adds an
   explicit Reachability field with Live / Latent / Theoretical.

Verified: meta contract guards and traceability tests pass.

Co-authored-by: Copilot App <[email protected]>
2026-08-23 19:14:35 -05:00
6 changed files with 177 additions and 272 deletions
+4 -3
View File
@@ -19,6 +19,7 @@ You are a Senior Python Architect performing an evidence-based, read-only code r
- **Stack Context:** Python 3.12+, FastAPI, NiceGUI, SQLModel, SQLAlchemy (SQLite/PostgreSQL), Pydantic V2, asyncio workers, and OpenRouter adapters. - **Stack Context:** Python 3.12+, FastAPI, NiceGUI, SQLModel, SQLAlchemy (SQLite/PostgreSQL), Pydantic V2, asyncio workers, and OpenRouter adapters.
- **Evidence-Based:** Always inspect real files. Every finding must reference concrete file paths and line numbers (e.g., `app/services/worker.py:45-78`). Do not speculate. - **Evidence-Based:** Always inspect real files. Every finding must reference concrete file paths and line numbers (e.g., `app/services/worker.py:45-78`). Do not speculate.
- **Tool Verification:** Run linters and tests via the terminal (`ruff check`, `pytest`, `ty`) to verify issues before reporting. - **Tool Verification:** This is a `uv` project; the toolchain is not on `PATH`. Verify with `uv run ruff check .`, `uv run ty check`, and `uv run pytest -q -m "not external"`, and record the exact commands and outcomes. Never report a lint, type, or test claim you did not run.
- **Skill Execution:** Adhere strictly to the review dimensions, duplication analysis, and report scaffolding defined in the `python-code-reviewer` skill. - **Verify Recommendations, Not Just Findings:** Before recommending a change to a shared symbol, enumerate its consumers and confirm the fix is safe for each. See the skill's consumer-tracing step and `Blast Radius` field.
- **Report Target:** Output all complete review reports as Markdown files written to `./docs`. - **Skill Is Canonical:** The `python-code-reviewer` skill defines the review workflow, deterministic checks, severity and reachability rubrics, report location, and report template. Follow it exactly. Where this file and the skill disagree, the skill wins — do not restate its specifics here.
- **Read-Only Scope:** Do not modify source, tests, docs, instructions, or configuration. The review report is the only artifact you produce.
+52 -6
View File
@@ -39,13 +39,30 @@ upgrade policy") — do not report it as a defect or recommend widening it.
6. **Assess Boundary and Coupling Health:** Evaluate UI/service/persistence/provider dependency flow, identify circular dependencies, leaky abstractions, and transaction ownership ambiguity. 6. **Assess Boundary and Coupling Health:** Evaluate UI/service/persistence/provider dependency flow, identify circular dependencies, leaky abstractions, and transaction ownership ambiguity.
7. **Assess Invariant Placement:** For each hard rule, decide whether it belongs in docs (rationale), instructions (active steering), skills (periodic audit procedure), or deterministic tests (enforcement). 7. **Assess Invariant Placement:** For each hard rule, decide whether it belongs in docs (rationale), instructions (active steering), skills (periodic audit procedure), or deterministic tests (enforcement).
8. **Verify Claims:** This is a `uv` project (`uv.lock`, root `ruff.toml`). Run `uv run ruff check .`, `uv run ty check`, and `uv run pytest -q -m "not external"` rather than guessing, and record the exact commands and their outcomes in the report. 8. **Verify Claims:** This is a `uv` project (`uv.lock`, root `ruff.toml`). Run `uv run ruff check .`, `uv run ty check`, and `uv run pytest -q -m "not external"` rather than guessing, and record the exact commands and their outcomes in the report.
9. **Prioritize Hot Paths:** Focus deeply on request handling, database sessions, background workers, and external API calls. 9. **Validate Recommendations Against Consumers:** A recommendation is a claim about the future and must be verified like any other. Before recommending a change to a shared symbol — a model field, an exception attribute, a helper's return value, a function signature — enumerate **every** consumer of that symbol (`grep` the whole repo, including tests) and confirm the fix is safe for each one. Record the consumers in the finding's **Blast Radius**. A fix that is correct for the path that produced the finding can silently break a second consumer, and evidence/provenance and logging paths are the usual casualties because they read the same fields the UI does.
10. **Enforce Read-Only Safety:** Do not modify code unless explicitly instructed. 10. **Prioritize Hot Paths:** Focus deeply on request handling, database sessions, background workers, and external API calls.
11. **Escalate Provenance Audits:** For evidence/provenance-heavy changes, apply invariant checks from `.github/skills/evidence-provenance-auditor/skill.md` and include pass/fail outcomes in the report. 11. **Enforce Read-Only Safety:** Do not modify code unless explicitly instructed.
12. **Escalate Test-Suite Audits:** When findings touch test coverage, redundancy, or assertion strength, apply `.github/skills/test-effectiveness-auditor/skill.md` and include its outcomes alongside the provenance results. 12. **Escalate Provenance Audits:** For evidence/provenance-heavy changes, apply invariant checks from `.github/skills/evidence-provenance-auditor/skill.md` and include pass/fail outcomes in the report.
13. **Escalate Test-Suite Audits:** When findings touch test coverage, redundancy, or assertion strength, apply `.github/skills/test-effectiveness-auditor/skill.md` and include its outcomes alongside the provenance results.
### Worked example: why step 9 exists
The 2026-08-23 review recommended fixing a filesystem-path leak in
`classify_unexpected_error` by making `AppError.message` generic and logging the exception
detail instead. The analysis of the leak was correct, and the fix was implemented as written.
It was wrong. `AppError.message` had a second consumer the review never traced:
`format_error_detail`, which writes `ExecutionAttempt.error_detail` — a **provenance record**.
The recommended fix closed a privacy leak by silently stripping root-cause data from the
evidence history this system exists to preserve. It was caught only because an unrelated
integration test asserted on the persisted error text.
The correct fix separated the audiences — a user-safe `message` and an internal-only `detail`
that still reaches evidence and logs. One `grep` for consumers of `.message` during the review
would have found this. Treat any recommendation that changes a widely-read field as unverified
until its consumers are enumerated.
## Repo-Specific Deterministic Checks (Transcription) ## Repo-Specific Deterministic Checks (Transcription)
When reviewing this repository, always include explicit pass/fail checks for the following. When reviewing this repository, always include explicit pass/fail checks for the following.
Where **Enforced by** reads *unenforced*, recommending a deterministic test is itself a finding. Where **Enforced by** reads *unenforced*, recommending a deterministic test is itself a finding.
@@ -127,6 +144,30 @@ Severity reflects concrete consequence, never style preference or effort to fix.
- **Medium:** Correctness risk under load or edge conditions (N+1, missing eager load, leaked task, missing timeout); drift between docs and code with no immediate runtime impact. - **Medium:** Correctness risk under load or edge conditions (N+1, missing eager load, leaked task, missing timeout); drift between docs and code with no immediate runtime impact.
- **Low:** Maintainability, typing completeness, duplication, naming, or dead code with no behavioral risk. - **Low:** Maintainability, typing completeness, duplication, naming, or dead code with no behavioral risk.
### Reachability
Severity states how bad the consequence is; **Reachability** states whether it can happen today.
They are independent, and a finding is not complete without both. Record one of:
- **Live:** reachable in the current configuration and deployment.
- **Latent:** the defective code is present but unreachable because of a current setting, single-
instance deployment, or absent caller. **State the exact condition that unblocks it.**
- **Theoretical:** requires a combination the project has explicitly ruled out.
Latent findings carry a scheduling constraint that severity alone cannot express: a latent defect
must usually be fixed *before* the change that makes it live, not after. Say so explicitly in the
finding and reflect the ordering in the §9 action plan — for example, "fix the retry-category gate
before raising `worker_max_retries` above 0," or "handle this `IntegrityError` before deploying a
second worker replica." Do not downgrade severity merely because a finding is latent.
### Conflicting invariants
When a fix sits between two invariants that pull in opposite directions, say so in the
**Recommendation** and name both, along with the test that guards each. Flag explicitly what the
over-correction would be, because the simplest-looking fix usually satisfies one invariant by
silently destroying the other. A recommendation that resolves one side without naming the other is
incomplete and will be implemented incorrectly.
## Output Report Structure & Template ## Output Report Structure & Template
Generate Markdown reports at `./docs/reviews/<YYYY-MM-DD>-code-review.md` following this exact Generate Markdown reports at `./docs/reviews/<YYYY-MM-DD>-code-review.md` following this exact
@@ -157,8 +198,13 @@ template structure. Reports are dated, non-canonical artifacts: `docs/reviews/**
### Critical Severity ### Critical Severity
#### [CRIT-01] Title #### [CRIT-01] Title
- **Location:** `path/to/file.py:lines` - **Location:** `path/to/file.py:lines`
- **Reachability:** Live / Latent (state the exact condition that unblocks it) / Theoretical
- **Problem & Consequence:** Concrete consequence, not a style opinion. - **Problem & Consequence:** Concrete consequence, not a style opinion.
- **Recommendation:** Fix with before/after sketch. - **Blast Radius:** Every consumer of the symbols the recommendation changes, each confirmed
safe. Write `None — change is local` only after actually searching. If the fix touches a
shared field or helper, list the call sites (including tests and evidence/logging paths).
- **Recommendation:** Fix with before/after sketch. If two invariants conflict here, name both,
name the test guarding each, and state what the over-correction would be.
- **Effort:** S / M / L - **Effort:** S / M / L
### High Severity ### High Severity
+107
View File
@@ -0,0 +1,107 @@
# AGENTS.md
Orientation for AI agents working in this repository. This file is a **router**, not a spec:
it points at canonical authority and flags the traps that are expensive to discover by trial.
Where this file and `docs/*` disagree, `docs/*` wins.
## What This Is
A document transcription system that preserves durable archival records (Documents, Sources,
People) and executes page transcription asynchronously through vision/LLM providers. Its
defining constraint is **evidence**: every machine attempt is recorded append-only with
request/response provenance. Features that would lose, mutate, or obscure that history are
wrong regardless of how convenient they are.
Stack: Python 3.12+ · FastAPI + NiceGUI · SQLModel/SQLAlchemy (SQLite-first, PostgreSQL-
compatible) · Pydantic V2 · asyncio worker · OpenRouter adapter.
## Commands
This is a `uv` project. **Nothing is on `PATH`**`ruff`, `ty`, and `pytest` all require
`uv run`. Bare invocations fail with command-not-found.
```bash
uv run ruff check . # lint (blocking in pre-commit)
uv run ruff format --check . # format (blocking in pre-commit)
uv run ty check # types (blocking in pre-commit)
uv run pytest -q -m "not external" # default verification run
```
`external` marks tests that hit live services; always exclude it unless explicitly asked.
All four commands are expected to pass clean — there is no tolerated baseline of failures.
If `ty` reports something, fix it or suppress it inline *with a rationale comment*; a bare
`ignore` will not survive review.
## Authority Order
Resolve every question in this order, and stop at the first that answers it:
1. **`docs/*`** — canonical. Start at [`docs/index.md`](docs/index.md), which defines the
reading order. `docs/invariant/*` holds cross-version rules that outlive any release.
2. **`.github/instructions/*.md`** — active steering, auto-attached when you edit matching
paths. Covers services, UI, error handling, and documentation sync.
3. **`.github/skills/*`** — periodic audit procedures (code review, provenance, test
effectiveness).
4. **`tests/`** — deterministic enforcement. A guard test is the ground truth for whatever
rule it encodes.
`docs/reviews/**` is **not** canonical. Those are dated, opinionated snapshots that were
accurate when written and may since have been fixed, superseded, or found wrong.
## Layout
| Path | Role |
| :--- | :--- |
| `src/transcription/ui/**`, `api/**` | Interface. No direct persistence access. |
| `src/transcription/services/**` | Domain logic and transaction ownership. |
| `src/transcription/db/**` | Models and persistence. |
| `src/transcription/providers/**` | Provider adapters; provider details stop here. |
| `src/transcription/worker.py` | Asyncio worker loop. |
| `tests/` | Includes boundary/contract guards, not just behavior tests. |
## Enforced Boundaries
These are not conventions — a test fails if you break them:
- **No service-to-service imports** (`test_service_boundaries.py`). Compose in the caller.
- **No persistence access from pages/components** (`test_ui_boundaries.py`, allowlist-based).
- **No hand-rolled error notifications in UI** — use the shared error presenter.
- **No stringly-typed status literals** — use the enums (`test_model_contract_guards.py`).
- **Attempt history is append-only** (`test_v42_evidence.py`).
- **`docs/schema.md` stays field-accurate** with `db/models.py`.
- **Orphans are tracked, not tolerated** — `test_orphan_sweep.py` records each retained
orphan with rationale in `KNOWN_ORPHANS`.
## Traps
Non-obvious things that have already caused real bugs here:
- **`AppError.message` vs `AppError.detail`.** `message` is user/API-facing and must stay
generic — never put exception text or filesystem paths in it. `detail` is internal-only and
is what reaches logs and `ExecutionAttempt.error_detail`. Putting root-cause data in
`message` leaks; removing it from `detail` silently degrades provenance. See
`docs/error_handling.md`.
- **Two competing atomicity invariants in `services/workflows.py`.** Intermediate pages must
commit individually (durability across a long multi-page job); the *final* page must commit
atomically with the terminal job status. Collapsing the batch into one transaction satisfies
the second and destroys the first. Both are guarded — `test_workflows_reliability.py` and
`tests/integration/test_pipeline_atomicity.py`.
- **Shared symbols have more consumers than the obvious one.** Before changing a model field,
exception attribute, or helper return value, grep for every consumer including tests.
Evidence and logging paths frequently read the same fields the UI does.
- **Import style:** ruff `isort` runs with `force-single-line = true`. One import per line.
- **Latent defects have ordering constraints.** Some code is unreachable only because of a
current setting or single-instance deployment. Fix it *before* the change that unblocks it,
not after.
## Change Protocol
- **Write the failing test first** for behavioral fixes, and confirm it actually fails for the
reason you think. Several bugs here were subtle enough that a test written afterward would
have passed against the broken code.
- **Update docs in the same change** when you alter a contract, behavior, or scope — see
`.github/instructions/documentation-sync.instructions.md`.
- **Do not commit unless asked.** Making a requested change is not consent to commit it.
- **Do not push or open PRs on your own initiative.**
- **Scope discipline:** fix what was asked plus what your change genuinely breaks. Pre-existing
unrelated issues are a separate conversation.
+10
View File
@@ -8,6 +8,16 @@
**Escalations applied:** `.github/skills/evidence-provenance-auditor/skill.md`, `.github/skills/test-effectiveness-auditor/skill.md` **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. **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 ### Verification commands and outcomes
| Command | Outcome | | Command | Outcome |
@@ -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[<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
```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.
+4 -5
View File
@@ -15,8 +15,7 @@ Naming: `<YYYY-MM-DD>-code-review.md` for review reports, and
## Current ## Current
- [`2026-08-23-code-review.md`](./2026-08-23-code-review.md) — full review. 0 critical, - [`2026-08-23-code-review.md`](./2026-08-23-code-review.md) — full review. 0 critical,
4 high, 5 medium, 11 low. 4 high, 5 medium, 11 low. **All findings remediated.** Retained as a record of the
- [`2026-08-23-remediation-handoff.md`](./2026-08-23-remediation-handoff.md) — **start here reasoning, not as a list of open work. Note that a few of its recommendations were
to continue the remediation work.** Phase 1 (all 4 high findings) is complete as of commit wrong on contact and were corrected during implementation; the code and the guard
`de18c2e`; the handoff covers Phases 2-5 with per-task acceptance criteria, the verification tests are authoritative over the report text.
baseline, and the environment gotchas needed to avoid re-deriving them.