From eb8a91c5d9d7ffb2d7bfd62b9177180942861f48 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 16:53:23 +0200 Subject: [PATCH] feat(rules): bootstrap 3 Python doc families (pydantic, project-structure, makefile) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second Python-side bootstrap. Same multi-doc-per-PR shape as PRs #17, #18, #19. 3 docs (~467-576 lines), 6 rules. This time the RULE blocks REPLACE the existing constraint sections (not insert above), avoiding the redundancy issue that hit PR #19. Rules added (rules/index.json: 74 -> 80): python-pydantic/* (owner: python-quality-assistant) - boundary-validation-only (MUST) — Pydantic at system boundaries only (API, queue ingestion, file parser); internal domain types use dataclass / plain types to avoid validation overhead on trusted data. - optional-needs-default (MUST) — Optional[T] alone is NOT omittable; it's a type-system 'T or None'. Pair with '= None' (or Field(default=...)) when the intent is 'may be omitted'. python-project-structure/* (owner: python-architecture-assistant) - src-layout-required (MUST) — packages live in src/, not at repo root. Root layout silently picks up the dev directory via sys.path; src/ forces 'install then import' which surfaces packaging bugs. - pyproject-toml-with-hatchling (MUST) — pyproject.toml + hatchling build backend, never setup.py. PEP 517/518 declarative manifest; hatchling is the recommended PyPA backend. python-makefile/* (owner: python-quality-assistant) - precommit-target-required (MUST) — every Python project's Makefile has a precommit target running format + test + check. Project- agnostic uniform entry point for CI scripts and pre-commit hooks. - uv-not-pip-or-poetry (SHOULD) — uv for dependency management (10-100x faster than pip/poetry/pipenv, deterministic uv.lock, reads standard PEP 621 metadata). SHOULD level acknowledges legacy projects mid-migration. CLAUDE.md doc-agent table updated with all 3 new mappings. Generic examples (User, Product, iphone_backup package, etc). No personal vault paths, no trading-domain terms. make build-index regenerated; check-index passes. --- CLAUDE.md | 3 ++ docs/python-makefile-commands.md | 62 +++++++++++++++++++++++++++ docs/python-project-structure.md | 72 ++++++++++++++++++++++++-------- docs/python-pydantic-guide.md | 61 +++++++++++++++++---------- rules/index.json | 54 ++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 38 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 206ca28..4a0529e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,6 +42,9 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | `python-architecture-patterns.md` | `python-architecture-assistant` | | `python-ioc-guide.md` | `python-architecture-assistant` | | `python-logging-guide.md` | `python-quality-assistant` | +| `python-pydantic-guide.md` | `python-quality-assistant` | +| `python-project-structure.md` | `python-architecture-assistant` | +| `python-makefile-commands.md` | `python-quality-assistant` | Reference-only docs (patterns, setup guides) don't need agents. diff --git a/docs/python-makefile-commands.md b/docs/python-makefile-commands.md index 0eff702..4264e2f 100644 --- a/docs/python-makefile-commands.md +++ b/docs/python-makefile-commands.md @@ -22,6 +22,38 @@ make make precommit ``` +### RULE python-makefile/precommit-target-required (MUST) + +**Owner**: python-quality-assistant +**Applies when**: a Python project's `Makefile` is missing a `precommit` target that runs `format` + `test` + `check` (or equivalent: lint, type-check, test). The default `make` target (no argument) MUST resolve to `precommit` so CI and pre-commit hooks can invoke `make` uniformly across projects. +**Enforcement**: judgment (Makefile presence + target-list inspection: `grep -E '^precommit:' Makefile` plus default-target check via `make -np | grep '^precommit'`) +**Why**: Different Python projects use different tools (ruff vs pyright vs mypy; pytest vs unittest; uv vs poetry). `make precommit` is the project-agnostic uniform entry point — CI scripts, pre-commit hooks, and editor integrations call `make precommit` and don't have to know the tool choices behind it. Without the target, CI scripts hard-code tool names that drift per-project, and the "run all quality checks" command is unmemorable per-developer. The target is one line; the value is uniform tooling across the org. + +#### Bad + +```makefile +# No precommit target — CI scripts must know each tool by name per project +test: + uv run pytest +lint: + uv run ruff check +typecheck: + uv run mypy src +``` + +#### Good + +```makefile +.PHONY: precommit format test check sync + +precommit: format test check + @echo "All precommit checks passed" + +# Alternative with dependency sync (recommended for CI/CD): +sync: + uv sync --all-extras +``` + **Standard implementation**: ```makefile precommit: format test check @@ -67,6 +99,36 @@ test: - Comprehensive assertion framework - Coverage reporting and threshold enforcement +### RULE python-makefile/uv-not-pip-or-poetry (SHOULD) + +**Owner**: python-quality-assistant +**Applies when**: a Python project's `Makefile` (or CI script) uses `pip install -r requirements.txt` / `poetry install` / `pipenv install` instead of `uv sync` for dependency management. +**Enforcement**: judgment (Makefile + CI-config inspection for non-uv install commands; cross-check against `pyproject.toml` having `[tool.uv]` or `uv.lock` present) +**Why**: `uv` is 10-100× faster than pip/poetry/pipenv on dependency resolution and install; it reads the standard `pyproject.toml` PEP 621 metadata directly (no per-tool config block); and it produces a deterministic `uv.lock` that's portable across machines. pip is fine for installing a single package; for project dependency management it lacks lock files and resolves slowly. Poetry has lock files but its own non-standard `[tool.poetry]` section in `pyproject.toml` and is much slower. The org has consolidated on uv; new Python projects MUST start there. (SHOULD level because legacy projects mid-migration are acceptable.) + +#### Bad + +```makefile +# pip — no lock file, slow resolution +install: + pip install -r requirements.txt + +# poetry — non-standard pyproject.toml, slower than uv +install: + poetry install --with dev +``` + +#### Good + +```makefile +install: + uv sync --all-extras + +# CI: +test: install + uv run pytest +``` + ### `make install` **Purpose**: Install project dependencies using uv. **What it does**: diff --git a/docs/python-project-structure.md b/docs/python-project-structure.md index 4bd832a..187c520 100644 --- a/docs/python-project-structure.md +++ b/docs/python-project-structure.md @@ -46,38 +46,76 @@ project-name/ ## Rules -### Use src/ Package Layout +### RULE python-project-structure/src-layout-required (MUST) -**Constraint:** Packages MUST be placed in `src/package_name/`, NOT at repository root. +**Owner**: python-architecture-assistant +**Applies when**: a Python project's importable package directory (`/__init__.py`) sits at the repo root instead of under `src/`. +**Enforcement**: judgment (file-layout check: `//__init__.py` exists at root level when `pyproject.toml` declares the package) +**Why**: Root-layout projects inherit a subtle bug — running tests from the repo root adds `.` to `sys.path`, so `import mypackage` succeeds against the *development* directory regardless of whether the package was actually built and installed. Tests pass locally; the wheel ships broken. `src/` layout forces the package to be installed (or `pip install -e .`-ed) before it's importable — tests then run against the same code consumers will get. Standard build tools (hatchling, setuptools, flit) recognise the layout automatically. -**Rationale:** src/ layout prevents accidental imports of development code, ensures package installation testing, and separates source from metadata files. +#### Bad -**Examples:** +```bash +# Root layout — tests pass against dev directory, miss packaging bugs +iphone_backup/ + __init__.py + backup.py +tests/ + test_backup.py +pyproject.toml +``` + +#### Good ```bash -# [GOOD] - src/ layout +# src/ layout — package must be installed to import; tests catch packaging bugs src/ iphone_backup/ __init__.py backup.py +tests/ + test_backup.py +pyproject.toml +``` -# [BAD] - Root layout -iphone_backup/ - __init__.py - backup.py +### RULE python-project-structure/pyproject-toml-with-hatchling (MUST) + +**Owner**: python-architecture-assistant +**Applies when**: a Python project ships a `setup.py` (legacy distutils/setuptools) or omits `pyproject.toml`, instead of using `pyproject.toml` + hatchling build backend. +**Enforcement**: judgment (file-layout check: `setup.py` present at repo root, OR `pyproject.toml` missing `[build-system]` with `build-backend = "hatchling.build"`) +**Why**: `setup.py` is the legacy build entry point — it executes arbitrary code at install time, has no declarative manifest, and ties the project to setuptools forever. PEP 517/518 made `pyproject.toml` the standard *declarative* build manifest; any modern build backend can read it. Hatchling is the recommended PyPA backend: zero plugin configuration for the 95% case, fast, maintained, and doesn't require setuptools as a transitive dependency. Sticking with `setup.py` blocks every modern Python tooling chain (uv, hatch, build, pip-tools-modern). + +#### Bad + +```python +# setup.py — legacy, executable, no declarative manifest +from setuptools import setup, find_packages + +setup( + name="iphone-backup", + version="0.5.0", + packages=find_packages(), + install_requires=["pymobiledevice3>=3.0.0"], +) ``` -**Benefits:** -- Prevents `sys.path` pollution during development -- Forces testing against installed package, not development directory -- Clean separation between package code and project files -- Standard recognized by all Python build tools +#### Good -### Use pyproject.toml with hatchling +```toml +# pyproject.toml — declarative, PEP 517/518, hatchling backend +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" -**Constraint:** Projects MUST use `pyproject.toml` with hatchling build backend, NOT setup.py. +[project] +name = "iphone-backup" +version = "0.5.0" +requires-python = ">=3.11" +dependencies = ["pymobiledevice3>=3.0.0"] -**Rationale:** PEP 517/518 standard build system; hatchling is simple, fast, and maintained by PyPA. +[tool.hatch.build.targets.wheel] +packages = ["src/iphone_backup"] +``` **Examples:** diff --git a/docs/python-pydantic-guide.md b/docs/python-pydantic-guide.md index f659ae7..753e3da 100644 --- a/docs/python-pydantic-guide.md +++ b/docs/python-pydantic-guide.md @@ -34,24 +34,38 @@ pip install "pydantic<2" ## Boundary Validation Rules -### Pydantic Usage Scope +### RULE python-pydantic/boundary-validation-only (MUST) -**Constraint:** Pydantic MUST ONLY be used at system boundaries for external data validation. +**Owner**: python-quality-assistant +**Applies when**: a Python service class or internal domain model inherits from `pydantic.BaseModel` instead of using `dataclass` / plain types, in code reached only from already-validated callers (not at an API boundary, queue consumer, file parser, or other untrusted-input ingestion point). +**Enforcement**: judgment (ast-grep first-pass filter: `class X(BaseModel):` declarations in files outside designated boundary directories like `pkg/api/`, `pkg/handler/`, `pkg/ingestion/`; the agent rules out the "actually at a boundary" case) +**Why**: Pydantic validation runs on every instantiation. At the system boundary, that cost is justified — incoming JSON could be malformed. Inside the service, the data has already been validated at ingestion; re-validating wastes CPU on every method call and obscures the trust boundary (a reader can't tell which `User` instances are "raw input" vs "already trusted"). Boundary-only use keeps the validation cost where the trust boundary actually is and makes internal code free of `**user.dict()` re-validation rituals. -**Rationale:** Validation overhead is justified only when parsing untrusted input; internal data has already been validated. +#### Bad -**Examples:** +```python +class UserService: + def get_user(self, user_id: int) -> User: # User is a Pydantic BaseModel + user = self._repo.find_by_id(user_id) # validates again on every read + return user +``` + +#### Good ```python -# [GOOD] - Validate at API boundary +# Pydantic at the boundary only @app.post("/users") -def create_user(request: CreateUserRequest): # Pydantic validates here - user_service.create(request.to_entity()) # Internal uses plain types +def create_user(request: CreateUserRequest): # validates incoming JSON + user_service.create(request.to_entity()) # internal uses plain User type -# [BAD] - Pydantic deep in internal code -class UserService: - def get_user(self, user_id: int) -> User: # Pydantic model internally - return self._repo.find_by_id(user_id) +# Internal types use dataclass — no validation overhead on trusted data +from dataclasses import dataclass + +@dataclass +class User: + id: int + name: str + email: str ``` ### Single Validation Point @@ -112,24 +126,29 @@ class UserEntity(BaseModel): # Unnecessary validation overhead ## Field Definition Rules -### Optional Field Declaration +### RULE python-pydantic/optional-needs-default (MUST) -**Constraint:** Omittable fields MUST have a default value; `Optional[T]` alone is NOT sufficient. +**Owner**: python-quality-assistant +**Applies when**: a Pydantic `BaseModel` field is typed `Optional[T]` / `T | None` (intended to be omittable) but has no default value assigned. +**Enforcement**: judgment (ast-grep first-pass: `class X(BaseModel):` body containing `name: Optional[T]` or `name: T | None` declarations without `= None` or `= Field(default=...)`; agent confirms intent if needed) +**Why**: `Optional[T]` is a type-system declaration that values can be `None` — it does NOT make the field omittable. Pydantic still requires the field at instantiation; callers must explicitly pass `name=None`. The "field omitted ⇒ default applied" semantic only kicks in when a default is declared. The bug is silent at definition time and explodes as a `ValidationError: field required` at instantiation — usually in a code path the author thought was optional. Always pair `Optional[T]` with `= None` (or `= Field(default=...)`) when the intent is "may be omitted." -**Rationale:** `Optional[T]` means "T or None", not "field can be omitted". Without a default, the field is still required. - -**Examples:** +#### Bad ```python -# [GOOD] - Truly optional with default class User(BaseModel): - name: Optional[str] = None # Can be omitted + name: Optional[str] # type says "T or None"; field is still REQUIRED -# [BAD] - Required despite Optional annotation +User() # ValidationError: name field required +``` + +#### Good + +```python class User(BaseModel): - name: Optional[str] # Still REQUIRED - only allows None as value + name: Optional[str] = None # truly omittable: default kicks in when omitted -User() # ValidationError: field required +User() # OK — name = None ``` ### Field Constraints diff --git a/rules/index.json b/rules/index.json index 52816f0..923d51d 100644 --- a/rules/index.json +++ b/rules/index.json @@ -664,5 +664,59 @@ "id": "python-logging/lazy-evaluation-for-debug", "level": "MUST", "owner": "python-quality-assistant" + }, + { + "anchor": "python-makefile/precommit-target-required", + "applies_when": "a Python project's `Makefile` is missing a `precommit` target that runs `format` + `test` + `check` (or equivalent: lint, type-check, test). The default `make` target (no argument) MUST resolve to `precommit` so CI and pre-commit hooks can invoke `make` uniformly across projects.", + "doc_path": "docs/python-makefile-commands.md", + "enforcement": "judgment (Makefile presence + target-list inspection: `grep -E '^precommit:' Makefile` plus default-target check via `make -np | grep '^precommit'`)", + "id": "python-makefile/precommit-target-required", + "level": "MUST", + "owner": "python-quality-assistant" + }, + { + "anchor": "python-makefile/uv-not-pip-or-poetry", + "applies_when": "a Python project's `Makefile` (or CI script) uses `pip install -r requirements.txt` / `poetry install` / `pipenv install` instead of `uv sync` for dependency management.", + "doc_path": "docs/python-makefile-commands.md", + "enforcement": "judgment (Makefile + CI-config inspection for non-uv install commands; cross-check against `pyproject.toml` having `[tool.uv]` or `uv.lock` present)", + "id": "python-makefile/uv-not-pip-or-poetry", + "level": "SHOULD", + "owner": "python-quality-assistant" + }, + { + "anchor": "python-project-structure/pyproject-toml-with-hatchling", + "applies_when": "a Python project ships a `setup.py` (legacy distutils/setuptools) or omits `pyproject.toml`, instead of using `pyproject.toml` + hatchling build backend.", + "doc_path": "docs/python-project-structure.md", + "enforcement": "judgment (file-layout check: `setup.py` present at repo root, OR `pyproject.toml` missing `[build-system]` with `build-backend = \"hatchling.build\"`)", + "id": "python-project-structure/pyproject-toml-with-hatchling", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-project-structure/src-layout-required", + "applies_when": "a Python project's importable package directory (`/__init__.py`) sits at the repo root instead of under `src/`.", + "doc_path": "docs/python-project-structure.md", + "enforcement": "judgment (file-layout check: `//__init__.py` exists at root level when `pyproject.toml` declares the package)", + "id": "python-project-structure/src-layout-required", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-pydantic/boundary-validation-only", + "applies_when": "a Python service class or internal domain model inherits from `pydantic.BaseModel` instead of using `dataclass` / plain types, in code reached only from already-validated callers (not at an API boundary, queue consumer, file parser, or other untrusted-input ingestion point).", + "doc_path": "docs/python-pydantic-guide.md", + "enforcement": "judgment (ast-grep first-pass filter: `class X(BaseModel):` declarations in files outside designated boundary directories like `pkg/api/`, `pkg/handler/`, `pkg/ingestion/`; the agent rules out the \"actually at a boundary\" case)", + "id": "python-pydantic/boundary-validation-only", + "level": "MUST", + "owner": "python-quality-assistant" + }, + { + "anchor": "python-pydantic/optional-needs-default", + "applies_when": "a Pydantic `BaseModel` field is typed `Optional[T]` / `T | None` (intended to be omittable) but has no default value assigned.", + "doc_path": "docs/python-pydantic-guide.md", + "enforcement": "judgment (ast-grep first-pass: `class X(BaseModel):` body containing `name: Optional[T]` or `name: T | None` declarations without `= None` or `= Field(default=...)`; agent confirms intent if needed)", + "id": "python-pydantic/optional-needs-default", + "level": "MUST", + "owner": "python-quality-assistant" } ]