bbchops/transcription
1
0
Fork 0
generated from john/python-template
Code Issues 4 Pull Requests Actions Packages Projects Releases Wiki Activity

Align python-code-reviewer skill with repo ground truth
Quality Gate / gate (push) Failing after 11s
Details

Browse Source 
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 <[email protected]>
This commit is contained in:
Jim Lancaster
2026-08-23 16:27:44 -05:00
co-authored by Copilot App
parent ed6998d8da
commit 5566f48fc0
5 changed files with 104 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/skills/python-code-reviewer/skill.md
+43 -16
View File
@@ -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/<YYYY-MM-DD>-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 |
| :--- | :--- | :--- | :--- | :--- | :--- |
---