diff --git a/.github/agents/python-reviewer.agent.md b/.github/agents/python-reviewer.agent.md index ccbcc9a..14d3816 100644 --- a/.github/agents/python-reviewer.agent.md +++ b/.github/agents/python-reviewer.agent.md @@ -1,12 +1,6 @@ --- name: Python Architect Reviewer description: Evidence-based senior architect reviewer for FastAPI, NiceGUI, and SQLModel codebases. -tools: - - read_file - - list_dir - - file_search - - grep_search - - run_in_terminal skills: - python-code-reviewer --- @@ -15,6 +9,10 @@ skills: You are a Senior Python Architect performing an evidence-based, read-only code review. +> No `tools:` allowlist is declared here on purpose. Tool identifiers differ between the runtimes +> this agent is invoked from, so a hard-coded list silently under-tools the agent in one of them. +> Read-only discipline is enforced by the **Read-Only Scope** rule below, not by the frontmatter. + ## Operating Principles - **Stack Context:** Python 3.12+, FastAPI, NiceGUI, SQLModel, SQLAlchemy (SQLite/PostgreSQL), Pydantic V2, asyncio workers, and OpenRouter adapters. diff --git a/.github/instructions/documentation-sync.instructions.md b/.github/instructions/documentation-sync.instructions.md index a964522..debedf3 100644 --- a/.github/instructions/documentation-sync.instructions.md +++ b/.github/instructions/documentation-sync.instructions.md @@ -7,6 +7,11 @@ applyTo: 'src/transcription/**/*.py' Keep docs in sync in the same change whenever implementation alters a documented contract, behavior, or roadmap decision. +Documentation targets below always refer to the **current** baseline. `docs/index.md` states which +baseline that is; resolve any version-specific document from there. Never cite a superseded version +tree by name in this file or in the docs you update — retired revision trees are not authority, and +`tests/test_meta_contract_guards.py` fails active contract files that route authority through them. + ## Update documentation when any of these change 1. **Schema/Data contract** @@ -27,7 +32,8 @@ Keep docs in sync in the same change whenever implementation alters a documented 5. **Roadmap/scope decisions** - Version targets, sequencing, deferrals, and accepted alternatives. - - **Required doc update:** `docs/roadmap_plan.md` and related backlog docs (for example `docs/ver4.8/feature_backlog_v4_8.md`). + - **Required doc update:** `docs/roadmap_plan.md`, plus any backlog or feature document for the + current baseline. Locate it through `docs/index.md` rather than assuming a version-named path. ## Working rule diff --git a/.github/instructions/error-handling.instructions.md b/.github/instructions/error-handling.instructions.md index 423a8fa..741c6c0 100644 --- a/.github/instructions/error-handling.instructions.md +++ b/.github/instructions/error-handling.instructions.md @@ -75,6 +75,28 @@ Required internal -> canonical mapping: - Include actionable remediation guidance aligned to category. - Keep envelope structure consistent across API endpoints. +### `AppError.message` vs `AppError.detail` + +`AppError` carries two texts with different audiences, and they must not be collapsed. Getting this +wrong has already caused a real defect in this repository, in both directions. + +| Attribute | Audience | Reaches | Rule | +| :--- | :--- | :--- | :--- | +| `message` | User and API clients | `ErrorEnvelope.message`, UI notifications | Stays generic. Never embed exception text, provider payloads, or filesystem paths. | +| `detail` | Internal only | Logs, and `format_error_detail` -> `ExecutionAttempt.error_detail` and `MaintenanceRun.error_detail` | Carries the root cause. Never rendered to users or serialized into an envelope. | + +- Putting root-cause data in `message` leaks infrastructure detail to users. +- Omitting it from `detail` silently degrades the provenance record this system exists to preserve — + a failed attempt whose `error_detail` says nothing is an attempt that cannot be diagnosed later. +- When you raise from a caught exception, populate **both**: a generic `message` and a `detail` + carrying `type(exc).__name__` and the exception text, with `raise ... from exc`. +- `detail` is optional (`None`). A read path that assumes it is populated must handle its absence. +- Before changing either attribute, or any helper that formats them, enumerate every consumer — + evidence writes, maintenance runs, logging, API envelopes, and UI presentation all read these + fields, and tests assert on the persisted text. + +Canonical definitions live in `src/transcription/errors.py`; see also `docs/error_handling.md`. + ## Logging and Diagnostics - Log operation identifiers and error IDs where available. diff --git a/.github/instructions/services.instructions.md b/.github/instructions/services.instructions.md index a7b894a..4e19c57 100644 --- a/.github/instructions/services.instructions.md +++ b/.github/instructions/services.instructions.md @@ -17,9 +17,17 @@ applyTo: 'src/transcription/services/*.py' - **A service module must not import another service module.** This is enforced by [test_service_boundaries](../../tests/test_service_boundaries.py). Shared types go in a neutral module that defines no service class (see [errors](../../src/transcription/services/errors.py)). -- Not every module in this package is a service. Helper modules that define no `*Service` - class (`base`, `errors`, `normalization`, `prompts`, `quality`, `media_storage`, - `source_media`) are free-function modules and are exempt from the service rules below. +- Not every module in this package is a service. Modules fall into three kinds: + - **Aggregate services** own models and define a `*Service` class: `documents.py`, `sources.py`, + `jobs.py`, `people.py`, `photos.py`, `maintenance.py`, and `evidence.py` (read/projection only, + owns nothing). + - **Orchestration modules** define no service class and compose writes across aggregates: + `store.py`, `workflows.py`. They are the sanctioned place to create or delete rows owned by more + than one service — see [Service Composition](#service-composition). + - **Shared infrastructure and free-function helpers** are exempt from the service rules below: + `base.py` (`ServiceBase`), `registry.py` (`RegistryService`, a generic base for lookup tables — + not an aggregate owner itself), `unit_of_work.py`, `errors.py`, `normalization.py`, `prompts.py`, + `quality.py`, `media_storage.py`, `source_media.py`. `__init__.py` exposes `ServiceBundle`. - Cross-cutting error behavior must follow [error-handling instructions](./error-handling.instructions.md). @@ -30,11 +38,37 @@ is the only service that may **create or delete** its rows. | Model | Owner | | --- | --- | -| `Document`, `DocumentType` | `DocumentService` | +| `Document`, `DocumentType`, `DocumentTag` | `DocumentService` | | `Source`, `JobSource` | `SourceService` | | `Job` | `JobService` | -| `Person`, `PersonRole`, `DocumentPerson` | `PeopleService` | +| `Person`, `PersonRole`, `DocumentPerson`, `PersonTag` | `PeopleService` | +| `Photo` | `PhotosService` | +| `MaintenanceRun` | `MaintenanceService` | | `ExecutionAttempt` | `SourceService` | +| `Tag` | shared — see below | + +Keep this table complete: every table in `src/transcription/db/models.py` appears exactly once, +except `Tag`. When you add a model, add its owner here in the same change. + +### `Tag` is deliberately shared + +`Tag` is one table reached through two `RegistryService[Tag]` facades that differ only in the +reference model they count usage through: `TagRegistry` (`documents.py`, via `DocumentTag`) and +`PersonTagRegistry` (`people.py`, via `PersonTag`). Both create and delete `Tag` rows through the +generic registry. This is the single sanctioned exception to one-owner-per-model — do not "fix" it by +assigning `Tag` to one service, because the other facade would then be creating rows it does not own. +Any change to `Tag` semantics, labels, or normalization must be validated against **both** facades +and the junction table each one counts. + +`DocumentTag` and `PersonTag` follow the junction rule below: each is created and deleted only by the +service on its own side. + +### Registries + +`DocumentTypeRegistry`, `TagRegistry`, `PersonRoleRegistry`, and `PersonTagRegistry` are +`RegistryService` subclasses, not independent services. A registry belongs to the aggregate service +whose module declares it and shares that service's ownership. Registry CRUD uses `_entry` +naming (see [CRUD Methods](#crud-methods)). ### Junction tables @@ -50,8 +84,14 @@ lifecycle owner. The service on the other side may read through the junction (vi Two consequences follow, and both are deliberate: - **Cascade deletion is not a violation.** A service deleting the aggregate root it owns - may delete junction rows referencing that root, because they cannot outlive it - (`JobService.delete_job_with_guardrails`). + may delete rows referencing that root which cannot outlive it + (`JobService.delete_job_with_guardrails` deletes the job's `job_source` rows). +- **Evidence deletion is an explicit workflow, not a runtime path.** `JobService.delete_job_and_evidence` + deletes `ExecutionAttempt` rows owned by `SourceService`. That is sanctioned because it is the + named retention workflow that `delete_job_with_guardrails` refuses to perform implicitly — that + method *blocks* deletion when attempts exist. Append-only means runtime code never rewrites or + removes history to represent a new outcome; it does not forbid a deliberate, operator-invoked + retention operation. Do not add a second path that deletes attempts. - **Ownership governs creation and deletion, not every state transition.** `job_source` is both a link and the transcription work queue. `JobService.cancel_job` and `resubmit_failed_sources` transition `job_source.status` across a whole job, because that diff --git a/.github/prompts/review-python-architecture.prompt.md b/.github/prompts/review-python-architecture.prompt.md index c3936ef..a7b5440 100644 --- a/.github/prompts/review-python-architecture.prompt.md +++ b/.github/prompts/review-python-architecture.prompt.md @@ -9,7 +9,7 @@ agent: Python Architect Reviewer Execute a comprehensive, evidence-based code review of the target codebase. ## Target Scope -- **Review Target:** ${{input:target_path:./}} +- **Review Target:** the repository root, unless the invoker names a narrower path; review that path instead. - **Source Root:** `src/` - **Docs Root:** `docs/` - **Focus Areas:** FastAPI endpoints, NiceGUI components, SQLModel persistence, asyncio workers, Pydantic V2 models, and OpenRouter provider adapters. @@ -17,7 +17,7 @@ Execute a comprehensive, evidence-based code review of the target codebase. ## Execution Rules 1. Map repository layout, dependency manifests, and configuration files from the project root before inspecting modules. 2. Read real code modules under `src/` (or the specified target path); cite exact file paths and line ranges for every finding. -3. Validate issues using terminal tools (`ruff check`, `pytest`, `ty`) where appropriate. +3. Validate issues by running `uv run ruff check .`, `uv run ty check`, and `uv run pytest -q -m "not external"`. Nothing is on `PATH` in this `uv` project, so bare `ruff`/`ty`/`pytest` will fail. 4. Check for duplication, divergent implementations, and extractable helpers. -5. Format the entire review following the standardized 6-section template defined in the `python-code-reviewer` skill. -6. Write the final report as a Markdown file to `./docs/code-review-${{current_date}}.md`. \ No newline at end of file +5. Format the entire review following the standardized 10-section template defined in the `python-code-reviewer` skill. +6. Write the final report to `./docs/reviews/-code-review.md`, using today's date. This path is defined by the skill; do not write the report anywhere else. \ No newline at end of file diff --git a/.github/skills/python-code-reviewer/skill.md b/.github/skills/python-code-reviewer/skill.md index d7c8e7e..0ca733f 100644 --- a/.github/skills/python-code-reviewer/skill.md +++ b/.github/skills/python-code-reviewer/skill.md @@ -31,7 +31,7 @@ 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. +1. **Map the Repository First:** Inspect entry points, package layout, configurations, dependency manifests, and any project-specific rule files (`AGENTS.md`, `.github/instructions/`, `.github/skills/`). Project-specific conventions override generic advice. 2. **Establish Canonical Authority First:** Read architecture/contracts (`docs/*`, `docs/invariant/*`, UI docs) and active instructions/skills before evaluating source behavior. 3. **Read Representative Modules:** Sample across all layers (routes/pages, UI components, services, workers, persistence, provider adapters, settings, tests) before drawing conclusions. 4. **Run Drift Analysis:** Compare documented intended behavior versus repository ground truth; identify both implementation drift and undocumented-but-repeatable conventions that should be formalized. diff --git a/.github/skills/test-effectiveness-auditor/skill.md b/.github/skills/test-effectiveness-auditor/skill.md index bd25ed4..623f4a4 100644 --- a/.github/skills/test-effectiveness-auditor/skill.md +++ b/.github/skills/test-effectiveness-auditor/skill.md @@ -64,7 +64,9 @@ Run a deterministic audit of test usefulness. Focus on whether tests catch real ## Output Format -Produce a Markdown report in `docs/`: +Produce a Markdown report at `docs/reviews/-test-effectiveness.md`, using today's date. +Like code review reports, it is a dated, non-canonical artifact: `docs/reviews/**` is not part of the +canonical authority set. ```markdown # Test Effectiveness Audit Report diff --git a/AGENTS.md b/AGENTS.md index 2bd344a..ff228c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,6 +89,12 @@ Non-obvious things that have already caused real bugs here: - **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. +- **`Tag` is owned by two services, on purpose.** Every other model has exactly one owning + service, so the ownership rule reads as absolute — it isn't. `Tag` is a single table reached + through two `RegistryService[Tag]` facades, `TagRegistry` (documents) and `PersonTagRegistry` + (people), which count usage through `DocumentTag` and `PersonTag` respectively. Changing tag + semantics through one facade silently changes the other. Consolidating them under one service + is not a cleanup; it makes the other side a cross-aggregate writer. - **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,