Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
62 changes: 62 additions & 0 deletions docs/python-makefile-commands.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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**:
Expand Down
72 changes: 55 additions & 17 deletions docs/python-project-structure.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 (`<package_name>/__init__.py`) sits at the repo root instead of under `src/`.
**Enforcement**: judgment (file-layout check: `<repo-root>/<package_name>/__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:**

Expand Down
61 changes: 40 additions & 21 deletions docs/python-pydantic-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
54 changes: 54 additions & 0 deletions rules/index.json
Original file line number Diff line number Diff line change
Expand Up @@ -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 (`<package_name>/__init__.py`) sits at the repo root instead of under `src/`.",
"doc_path": "docs/python-project-structure.md",
"enforcement": "judgment (file-layout check: `<repo-root>/<package_name>/__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"
}
]
Loading