From 5566f48fc01b75767edda59ef9a91e24b7730750 Mon Sep 17 00:00:00 2001 From: Jim Lancaster <40281233+zoltan57@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:27:44 -0500 Subject: [PATCH] Align python-code-reviewer skill with repo ground truth Update the reviewer skill so its procedure matches how this repo actually works: - Route review reports to docs/reviews/ and mark them non-canonical, resolving the conflict where reports landed in the same docs/ tree they resolve findings against. - Pin verification commands to uv (uv run ruff check / ty check / pytest -m 'not external'). - Record the pytest contract: strict markers, strict asyncio mode, and the never-awaited-coroutine warning promoted to an error. - Convert the deterministic checks to a table with an Enforced by column; three checks are unenforced and one only partial, which are now findings by construction. - Add a consequence-based severity rubric and a Direction column for bidirectional drift. - Escalate test-suite concerns to test-effectiveness-auditor. Also fix tests/test_db.py, which was missing 'from sqlalchemy import text' while using it in 14 places. Three tests were failing with NameError. Wrapped the pre-existing long lines in the same file so it lints clean. Document the deliberate nicegui==3.13.0 pin in pyproject.toml, a new runbook dependency upgrade policy, and the reviewer skill, so the pin is not flagged as a defect or widened as incidental cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/python-code-reviewer/skill.md | 59 ++++++++++++++------ docs/production-runbook.md | 31 ++++++++++ docs/reviews/README.md | 12 ++++ pyproject.toml | 4 ++ tests/test_db.py | 17 +++++- 5 files changed, 104 insertions(+), 19 deletions(-) create mode 100644 docs/reviews/README.md diff --git a/.github/skills/python-code-reviewer/skill.md b/.github/skills/python-code-reviewer/skill.md index 7055fca..ecfae3f 100644 --- a/.github/skills/python-code-reviewer/skill.md +++ b/.github/skills/python-code-reviewer/skill.md @@ -11,7 +11,7 @@ Perform thorough, evidence-based code reviews for Python projects. Every finding - Performing an architectural or code quality review of a Python codebase. - Auditing applications using FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, or asyncio workers. -- Generating structured Markdown review reports in `./docs`. +- Generating structured Markdown review reports in `./docs/reviews`. ## Technical Stack Scope @@ -21,8 +21,14 @@ Perform thorough, evidence-based code reviews for Python projects. Every finding - **Validation & Settings:** Pydantic V2 and pydantic-settings - **Concurrency:** Python asyncio workers - **Vision/LLM Integration:** OpenRouter / provider adapters +- **Image & Print Pipeline:** Pillow-backed media handling and print/export rendering - **Quality & Testing:** pytest, pytest-asyncio, Ruff, and ty +NiceGUI is pinned to an exact version (`nicegui==3.13.0` in `pyproject.toml`); API guidance +must be correct for that release rather than for the latest published version. The exact pin is +a deliberate release-stability decision recorded in `docs/production-runbook.md` ("Dependency +upgrade policy") — do not report it as a defect or recommend widening it. + ## Review Workflow 1. **Map the Repository First:** Inspect entry points, package layout, configurations, dependency manifests, and any project-specific rule files (`AGENTS.md`, `CLAUDE.md`, `.github/instructions/`). Project-specific conventions override generic advice. @@ -32,25 +38,29 @@ Perform thorough, evidence-based code reviews for Python projects. Every finding 5. **Run Dead-Code/Orphan Sweep:** Identify candidate orphan modules/functions/classes with zero inbound references, then verify expected exceptions (entrypoints, framework/plugin registration, dynamic imports/reflection, CLI hooks, test-only utilities) before marking as orphaned. 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). -8. **Verify Claims:** Run or reference project tooling (`ruff check`, `ty`, `pytest`) rather than guessing. +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. 10. **Enforce Read-Only Safety:** Do not modify code unless explicitly instructed. 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. +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. ## Repo-Specific Deterministic Checks (Transcription) -When reviewing this repository, always include explicit pass/fail checks for: +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. -1. **Service boundary rule:** no service-to-service imports (`tests/test_service_boundaries.py`). -2. **UI boundary rule:** pages/components do not perform persistence access (`tests/test_ui_boundaries.py`). -3. **Status vocabulary conformance:** `JobStatus`/`JobSourceStatus` usage matches current enums in `src/transcription/db/models.py`. -4. **Evidence ownership conformance:** append-only attempt history is preserved and projection writes are not mistaken for history mutation (`src/transcription/services/sources.py`, `src/transcription/services/evidence.py`). -5. **Canonical authority:** findings must resolve against `docs/*` first. -6. **Schema contract fidelity:** when model/persistence behavior changes, `docs/schema.md` remains field-accurate with `src/transcription/db/models.py`. -7. **Media boundary conformance:** print/export media is record-validated and UI media URL generation uses controlled resolver paths. -8. **Eager-loading conformance:** service/UI read paths satisfy `lazy="raise"` expectations. -9. **Cross-cutting error conformance:** service/API/UI translation and retry behavior align with `.github/instructions/error-handling.instructions.md`. -10. **Orphaned/dead-code conformance:** include a deterministic orphan sweep and report confirmed orphans removed/retained with rationale. +| # | Check | Enforced by | +| :-- | :--- | :--- | +| 1 | **Service boundary rule:** no service-to-service imports | `tests/test_service_boundaries.py` | +| 2 | **UI boundary rule:** pages/components do not perform persistence access | `tests/test_ui_boundaries.py` | +| 3 | **Status vocabulary conformance:** `JobStatus`/`JobSourceStatus` usage matches current enums in `src/transcription/db/models.py`; no stringly-typed status literals | *unenforced* — only incidental coverage via `tests/services/test_job_service.py` | +| 4 | **Evidence ownership conformance:** append-only attempt history is preserved and projection writes are not mistaken for history mutation (`src/transcription/services/sources.py`, `src/transcription/services/evidence.py`) | `tests/test_v42_evidence.py::test_attempts_are_append_only_and_exported_with_integrity` | +| 5 | **Canonical authority:** findings must resolve against `docs/*` first | `tests/test_meta_contract_guards.py::test_canonical_authority_references_are_present` | +| 6 | **Schema contract fidelity:** when model/persistence behavior changes, `docs/schema.md` remains field-accurate with `src/transcription/db/models.py` | *partial* — `tests/test_meta_contract_guards.py` verifies presence and references only, **not** field accuracy | +| 7 | **Media boundary conformance:** print/export media is record-validated and UI media URL generation uses controlled resolver paths | `tests/test_media_path_safety.py`, `tests/ui/test_media_urls.py` | +| 8 | **Eager-loading conformance:** service/UI read paths satisfy `lazy="raise"` expectations | *unenforced* — no guard test; only incidental use in `tests/ui/test_sources_page.py` | +| 9 | **Cross-cutting error conformance:** service/API/UI translation and retry behavior align with `.github/instructions/error-handling.instructions.md` | `tests/test_errors.py`, `tests/api/test_error_responses.py`, `tests/ui/test_error_presenter.py` | +| 10 | **Orphaned/dead-code conformance:** include a deterministic orphan sweep and report confirmed orphans removed/retained with rationale | *unenforced* | ## Core Review Areas @@ -91,6 +101,8 @@ When reviewing this repository, always include explicit pass/fail checks for: ### 8. Testing & Quality Tooling - **Test Isolation:** Verify tests do not rely on live external services, real clocks, or shared global state. - **Async Test Setup:** Check `pytest-asyncio` configuration and fixture lifecycle. +- **Project Test Contract (`pyproject.toml`):** `--strict-markers` is enabled, so every marker must be declared; `asyncio_mode = "strict"` requires explicit `@pytest.mark.asyncio`; declared markers are `unit`, `integration`, and `external`, and `external` must be excluded from default verification runs. `filterwarnings` promotes `coroutine ... was never awaited` to an **error** — treat any unawaited coroutine as a hard failure and a Critical/High finding, never a warning. +- **Suite Signal Quality:** For low-value, redundant, or tautological tests, escalate to `.github/skills/test-effectiveness-auditor/skill.md` and fold its outcomes into the report. ### 9. Duplication & Consolidation - Identify repeated code blocks, candidate helper abstractions, divergent patterns for identical operations, and duplicated domain constants. @@ -106,9 +118,20 @@ When reviewing this repository, always include explicit pass/fail checks for: - **Invariant Routing:** Recommend the correct enforcement layer per rule (docs vs instructions vs skills vs tests). - **Meta-Tooling Alignment:** Recommend updates for instruction files and skills when repository patterns or contracts evolve. +## Severity Rubric + +Severity reflects concrete consequence, never style preference or effort to fix. + +- **Critical:** Data or evidence loss/corruption; provenance or append-only history violated; secret leakage; silent wrong output presented as authoritative. +- **High:** Architectural boundary violated (service/UI/persistence/provider); runtime failure or unhandled exception on a hot path (request handling, DB sessions, worker loop, external API calls); documented invariant contradicted by implementation. +- **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. + ## Output Report Structure & Template -Generate Markdown reports in `./docs` following this exact template structure: +Generate Markdown reports at `./docs/reviews/-code-review.md` following this exact +template structure. Reports are dated, non-canonical artifacts: `docs/reviews/**` is explicitly +**not** part of the canonical authority set that the canonical-authority check resolves against. ```markdown # Architecture & Code Review Report @@ -153,8 +176,12 @@ Generate Markdown reports in `./docs` following this exact template structure: --- ## 4. Architectural Drift & Gap Analysis -| Area / Component | Documented / Intended Rule | Actual Implementation State | Severity | Recommended Resolution | -| :--- | :--- | :--- | :--- | :--- | + +`Direction` is `doc->code` (implementation must change to match documented intent) or +`code->doc` (an undocumented but repeatable convention that should be formalized). + +| Area / Component | Direction | Documented / Intended Rule | Actual Implementation State | Severity | Recommended Resolution | +| :--- | :--- | :--- | :--- | :--- | :--- | --- diff --git a/docs/production-runbook.md b/docs/production-runbook.md index c6ab402..4cc3f02 100644 --- a/docs/production-runbook.md +++ b/docs/production-runbook.md @@ -82,3 +82,34 @@ This runbook is the operational checklist for releasing and monitoring the trans 1. Inspect affected `job_source` and `execution_attempt` records. 2. Confirm failures are category-aligned (`external`/`timeout`/`internal`). 3. Triage whether issue is source quality, provider, or runtime regression. + +## 6. Dependency upgrade policy + +Dependencies are declared in `pyproject.toml` and resolved through the committed +`uv.lock`. The lockfile guarantees reproducible installs; the version specifiers +control what a deliberate `uv lock --upgrade` is allowed to move. + +### NiceGUI is pinned exactly (`nicegui==3.13.0`) + +1. **Rationale.** NiceGUI bundles Quasar and Vue. Minor releases change component + props, slots, and styling, which surfaces as visual and interaction regressions + rather than import or type errors. The UI suite under `tests/ui/` asserts + structure and behavior, not rendered appearance, so a NiceGUI bump can pass the + full test suite and still degrade the interface. +2. **Scope of risk.** All NiceGUI usage is confined to `src/transcription/ui/` and + uses only the public `nicegui.ui` and `nicegui.events` surfaces. The coupling is + shallow, so the pin is about release stability, not about unpicking deep + framework entanglement. +3. **Current stance.** Hold the exact pin through release stabilization. Do not + widen it as incidental cleanup, and do not let automated dependency updates move + it. This includes forgoing patch releases, which is the accepted cost. +4. **Revisiting.** Treat a NiceGUI upgrade as scheduled work with its own change + window: bump the pin deliberately, run `uv run pytest -m "not external"`, then + manually verify each page contract in `docs/ui/pages/` before accepting. + +### All other dependencies + +Declared with `>=` floors and moved by explicit `uv lock --upgrade`. Verify with +`uv run ruff check .`, `uv run ty check`, and `uv run pytest -q -m "not external"` +before committing a changed lockfile. + diff --git a/docs/reviews/README.md b/docs/reviews/README.md new file mode 100644 index 0000000..b8d9e6c --- /dev/null +++ b/docs/reviews/README.md @@ -0,0 +1,12 @@ +# Review Reports + +Dated architecture and code review reports generated by +`.github/skills/python-code-reviewer/skill.md`. + +**These files are not canonical authority.** Everything in `docs/reviews/**` is a +point-in-time observation, not a contract. Canonical intent lives in `docs/index.md`, +`docs/architecture.md`, `docs/requirements.md`, `docs/schema.md`, +`docs/error_handling.md`, and `docs/invariant/**`. When a report and a canonical +document disagree, the canonical document wins until it is deliberately updated. + +Naming: `-code-review.md`. diff --git a/pyproject.toml b/pyproject.toml index 9282c48..766fef6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,6 +15,10 @@ dependencies = [ "aiosqlite>=0.21.0", "asyncpg>=0.31.0", "fastapi>=0.138.0", + # Exact pin, deliberate. NiceGUI 3.x minor releases ship Quasar/Vue changes that + # break component props and styling, and tests/ui/ cannot detect visual regressions. + # Hold through the current release stabilization; revisit as a scheduled upgrade. + # See docs/production-runbook.md, "Dependency upgrade policy". "nicegui==3.13.0", "openrouter>=0.7.0", "pillow>=10.0.0", diff --git a/tests/test_db.py b/tests/test_db.py index aee87fd..a078206 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -6,6 +6,7 @@ import pytest import pytest_asyncio import sqlalchemy as sa from sqlalchemy import inspect +from sqlalchemy import text from sqlalchemy.dialects import postgresql from sqlalchemy.exc import SAWarning from sqlmodel import SQLModel @@ -144,7 +145,15 @@ async def test_create_all_declares_hot_path_indexes(tmp_path): database = inspect(sync_connection) indexes = { table: [index["column_names"] for index in database.get_indexes(table)] - for table in ("job", "source", "job_source", "document", "document_person", "document_tag", "person_tag") + for table in ( + "job", + "source", + "job_source", + "document", + "document_person", + "document_tag", + "person_tag", + ) } job_source_unique = [ constraint["column_names"] @@ -300,8 +309,10 @@ async def test_reconcile_canonical_media_paths_normalizes_source_and_photo_paths ) await connection.execute( text( - 'insert into "source" (id, document_id, page_number, upload_name, filename, file_path, file_hash, file_size_bytes, date_uploaded) ' - 'values (:id, :document_id, 1, :upload_name, :filename, :file_path, :file_hash, :file_size_bytes, CURRENT_TIMESTAMP)' + 'insert into "source" (id, document_id, page_number, upload_name, filename, ' + "file_path, file_hash, file_size_bytes, date_uploaded) " + "values (:id, :document_id, 1, :upload_name, :filename, :file_path, " + ":file_hash, :file_size_bytes, CURRENT_TIMESTAMP)" ), { "id": "33" * 16,