generated from john/python-template
Compare commits
4
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
f15c9834e4 | ||
|
|
2c59cbd2c7 | ||
|
|
5ff66a8c40 | ||
|
|
626b5d4b10 |
@@ -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.
|
||||||
@@ -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
|
||||||
|
|||||||
@@ -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.
|
||||||
@@ -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.
|
|
||||||
@@ -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.
|
|
||||||
|
|||||||
Reference in New Issue
Block a user