diff --git a/CLAUDE.md b/CLAUDE.md index 6522bda..206ca28 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,6 +39,9 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | `go-licensing-guide.md` | `license-assistant` | | `agent-command-development-guide.md` | `agent-auditor` + `slash-command-auditor` | | `claude-code-skill-writing-guide.md` | `skill-auditor` | +| `python-architecture-patterns.md` | `python-architecture-assistant` | +| `python-ioc-guide.md` | `python-architecture-assistant` | +| `python-logging-guide.md` | `python-quality-assistant` | Reference-only docs (patterns, setup guides) don't need agents. diff --git a/docs/python-architecture-patterns.md b/docs/python-architecture-patterns.md index 4755386..989ab1e 100644 --- a/docs/python-architecture-patterns.md +++ b/docs/python-architecture-patterns.md @@ -12,17 +12,30 @@ The standard pattern for Python services follows this structure: ## 1. Core Pattern: Constructor Injection -### Protocol Definition +### RULE python-architecture/constructor-injection-only (MUST) -```python -from typing import Protocol +**Owner**: python-architecture-assistant +**Applies when**: a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a *method parameter* instead of through `__init__`. Related but distinct anti-patterns covered by other rules: dependency from a global module variable (see `python-architecture/main-py-composition-root`); post-construction `self.foo = bar.foo` mutation (see `python-ioc/dependencies-as-private-fields`, which mandates private-field storage that makes mutation socially difficult). +**Enforcement**: judgment (semantic — requires distinguishing dependency injection from runtime parameter passing. The other two related anti-patterns have their own rules and enforcement paths.) +**Why**: Mixing dependency injection with runtime parameter passing breaks the contract Python's type system relies on. Constructor injection makes the dep set visible at `__init__`, immutable after construction, and self-documenting at the type signature. Method-level injection means tests have to construct the full dep graph for every call site; global injection means tests are order-dependent. Methods should receive only the data needed to do their job — `create_user(user: User)`, not `create_user(repo, logger, validator, user)`. + +#### Bad -class UserRepository(Protocol): - def save(self, user: User) -> None: ... - def find_by_id(self, user_id: int) -> User | None: ... +```python +class UserService: + def create_user( + self, + repo: UserRepository, # ← dependency, not runtime data + logger: Logger, # ← dependency, not runtime data + validator: UserValidator, # ← dependency, not runtime data + user: User, # ← actual runtime data + ) -> None: + validator.validate(user) + repo.save(user) + logger.info(f"Created user {user.id}") ``` -### Service Implementation +#### Good ```python class UserService: @@ -32,24 +45,64 @@ class UserService: logger: Logger, validator: UserValidator, ): - self._repo = repo # Static dependency - self._logger = logger # Static dependency - self._validator = validator # Static dependency + self._repo = repo + self._logger = logger + self._validator = validator - def create_user(self, user: User) -> None: # Runtime param only + def create_user(self, user: User) -> None: # only runtime data self._validator.validate(user) self._repo.save(user) self._logger.info(f"Created user {user.id}") ``` -**Key points:** -- Constructor receives ALL dependencies (static, mockable) -- Methods receive ONLY runtime data (request params, user input) -- Dependencies stored as private fields (`self._dep`) -- Clean separation: deps bound once, methods reusable +**Key points** (the RULE above shows the canonical Bad/Good shape; below are the orthogonal supporting conventions): + +- Constructor receives ALL dependencies (static, mockable); methods receive ONLY runtime data. +- Dependencies stored as private fields (`self._dep`) — see [`python-ioc/dependencies-as-private-fields`](python-ioc-guide.md). +- Dependency interfaces use `Protocol` — see [`python-ioc/protocol-not-abc-for-dependencies`](python-ioc-guide.md). ## 2. main.py Pattern (Composition Root) +### RULE python-architecture/main-py-composition-root (SHOULD) + +**Owner**: python-architecture-assistant +**Applies when**: a Python application instantiates service objects at module level (top of `repository.py`, `handler.py`, etc.) instead of wiring them inside `main()` / `main.py`. +**Enforcement**: judgment (file-scope check: module-level `Service()` / `Repository()` calls outside `if __name__ == "__main__":` blocks) +**Why**: Module-level instantiation runs at *import time* — order-dependent, hard to test, impossible to mock for unit tests. The composition root pattern says: configuration parsing, infrastructure setup, and service wiring all happen in one place (`main.py` → `main(argv)`), in a deterministic order, with explicit dependency edges. Tests can then construct their own composition root with mock infrastructure; production constructs the real one. Module-level state collapses this clean separation into spaghetti. + +#### Bad + +```python +# user_service.py +from infrastructure import database, api_client # imports trigger connection! + +repo = SqlUserRepository(database) # module-level — runs at import +service = UserService(repo, api_client) # module-level — runs at import +``` + +#### Good + +```python +# main.py — composition root +def main(argv): + logging.basicConfig(...) + db_url = os.getenv("DATABASE_URL") + api_key = os.getenv("API_KEY") + + database = Database(db_url) # infrastructure first + api_client = ApiClient(api_key) + + repo = SqlUserRepository(database) # services composed explicitly + service = UserService(repo, api_client) + + handler = UserHandler(service) + HttpServer(8080, handler).run() + +# user_service.py — no module-level instantiation +class UserService: + def __init__(self, repo: UserRepository, api: ApiClient): ... +``` + The `main.py` is the **composition root** where all dependencies are wired together. ### Structure diff --git a/docs/python-ioc-guide.md b/docs/python-ioc-guide.md index b7355e8..ed5a3c7 100644 --- a/docs/python-ioc-guide.md +++ b/docs/python-ioc-guide.md @@ -53,22 +53,17 @@ class UserValidator(Protocol): ## Rules -### Use Protocol for Dependency Interfaces +### RULE python-ioc/protocol-not-abc-for-dependencies (MUST) -**Constraint:** MUST use `Protocol` for defining dependency interfaces unless shared implementation is required. +**Owner**: python-architecture-assistant +**Applies when**: a Python service interface (dependency surface, e.g. `UserRepository`, `Logger`) is declared with `abc.ABC` + `@abstractmethod` decorators instead of `typing.Protocol`, AND no shared implementation across concrete types motivates the ABC. +**Enforcement**: judgment (ast-grep partial: detect `class X(ABC):` declarations that only contain `@abstractmethod` methods — but the "no shared impl needed" trigger is semantic) +**Why**: Protocol gives you structural typing — any class with the right method shape satisfies it, without inheritance. That's the right primitive for dependency interfaces: mocks don't need to inherit from anything, tests don't fake an `ABC` hierarchy, and you avoid the "every protocol class has both an ABC and a Protocol declaration" duplication that ABC-first projects accrete. ABC is the right primitive when concrete implementations share code via `super()` — that's the actual reason ABCs exist. For dependency interfaces (which are pure contracts), Protocol is shorter, lighter, and friendlier to mocks. -**Rationale:** Protocol enables structural typing and works seamlessly with mocks without inheritance overhead. +#### Bad -**Examples:** ```python -# [GOOD] -from typing import Protocol - -class UserRepository(Protocol): - def save(self, user: User) -> None: ... - def find_by_id(self, user_id: int) -> User | None: ... - -# [BAD] - Using ABC when no shared implementation needed +# Using ABC when no shared implementation needed from abc import ABC, abstractmethod class UserRepository(ABC): @@ -77,6 +72,16 @@ class UserRepository(ABC): pass ``` +#### Good + +```python +from typing import Protocol + +class UserRepository(Protocol): + def save(self, user: User) -> None: ... + def find_by_id(self, user_id: int) -> User | None: ... +``` + **Protocol vs ABC Decision:** | Need | Use | @@ -120,23 +125,30 @@ class UserService: self._validator = UserValidator() ``` -### Store Dependencies as Private Fields +### RULE python-ioc/dependencies-as-private-fields (MUST) -**Constraint:** MUST store injected dependencies as private fields with underscore prefix (`self._repo`). +**Owner**: python-architecture-assistant +**Applies when**: a Python service class stores an injected dependency on `self.` (public attribute) instead of `self._` (single-underscore private convention). +**Enforcement**: judgment (ast-grep first-pass filter: any `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_`. Type-annotation resolution to distinguish service deps from public data fields cannot be done in ast-grep — the agent makes the final call. Overinclusive first-pass is acceptable: most public `self.X` assignments in service classes ARE the smell.) +**Why**: Public attribute storage invites external mutation — `user_service.repo = MockRepository()` — which breaks the immutability contract that makes constructor injection safe in the first place. Tests that mutate inject-time fields hide behavior that production never exercises. Single-underscore prefix is Python's universal "internal, don't touch" convention; following it forces test code to compose deps the same way production does (through the constructor) and keeps the dep set observable only at `__init__`. -**Rationale:** Prevents external mutation and clearly indicates internal implementation details. +#### Bad -**Examples:** ```python -# [GOOD] class UserService: def __init__(self, repo: UserRepository): - self._repo = repo # Private field + self.repo = repo # public — external code can rebind it -# [BAD] +# elsewhere: +user_service.repo = SomeOtherRepo() # silent contract violation +``` + +#### Good + +```python class UserService: def __init__(self, repo: UserRepository): - self.repo = repo # Public field allows external mutation + self._repo = repo # private — single-underscore convention ``` ### Never Return Values from Constructors diff --git a/docs/python-logging-guide.md b/docs/python-logging-guide.md index 5a348e2..5a0696c 100644 --- a/docs/python-logging-guide.md +++ b/docs/python-logging-guide.md @@ -38,44 +38,49 @@ logger = logging.getLogger(__name__) ## Configuration Rules -### Configure Logging Once at Application Entry Point +### RULE python-logging/configure-once-in-main (MUST) -**Constraint:** Application code MUST call `logging.basicConfig()` exactly once at startup in `main.py`. Library code MUST NOT call `basicConfig()` or configure handlers. +**Owner**: python-quality-assistant +**Applies when**: a Python file in *library code* (modules imported by application code, intended for reuse across applications) calls `logging.basicConfig()` or adds handlers to the root logger. **Exempt**: the application entry point (`main.py` / `__main__.py` / `cli.py`) AND application-private helper modules wired from the entry point exclusively for configuration (e.g. `logging_setup.py` — see the next section), since those are part of the entry-point configuration surface, not library code. +**Enforcement**: judgment (semantic — distinguishing library code from application-private configuration helpers requires reading the import graph; ast-grep can flag every non-entry-point `logging.basicConfig` call as a first-pass filter, but the agent must rule out the helper-module exemption case) +**Why**: `basicConfig` is a one-shot global root-logger setup. Calling it from library code produces three failure modes: (1) first import wins, so the library's config silently overrides the application's choice depending on import order; (2) repeated calls add duplicate handlers, doubling every log line; (3) applications can't change log level without code edits in libraries they don't control. Libraries call `logging.getLogger(__name__)` and emit; configuration is the application's responsibility, and the application does it exactly once. -**Rationale:** Multiple configurations cause conflicts, overwrites, and duplicate log entries across modules. +#### Bad -**Examples:** ```python -# [GOOD] Application entry point -# main.py +# mylib/service.py — library configures logging — wrong import logging -logging.basicConfig( - format='%(asctime)s %(levelname)-8s [%(name)s:%(lineno)d] %(message)s', - level=logging.INFO, - datefmt='%Y-%m-%d %H:%M:%S' -) - +logging.basicConfig(level=logging.INFO) # first import wins, overrides app logger = logging.getLogger(__name__) -logger.info("Application started") -# [GOOD] Library module -# mylib/service.py +class UserService: ... +``` + +#### Good + +```python +# main.py — application entry point, configure once import logging -logger = logging.getLogger(__name__) # Get logger only +def main(): + logging.basicConfig( + format='%(asctime)s %(levelname)-8s [%(name)s:%(lineno)d] %(message)s', + level=logging.INFO, + ) + # ... rest of app wiring ... -class UserService: - def process(self): - logger.info("Processing user") +if __name__ == "__main__": + main() -# [BAD] Library configuring logging -# mylib/service.py -logging.basicConfig(level=logging.INFO) # Never do this in libraries +# mylib/service.py — library only gets a logger, never configures +import logging + +logger = logging.getLogger(__name__) -# [BAD] Multiple configurations -# orders/handler.py -logging.basicConfig(level=logging.DEBUG) # Conflicts with main config +class UserService: + def process(self): + logger.info("processing user") ``` ### Extract Logging Configuration to Dedicated Module @@ -377,23 +382,28 @@ logger.warning("Failed to connect", connection_error) # Only logs first arg logger.warning("Failed to connect: %s", connection_error) ``` -### Use Lazy Evaluation for Expensive DEBUG Operations +### RULE python-logging/lazy-evaluation-for-debug (MUST) -**Constraint:** Code MUST use `%s` formatting (not f-strings) for DEBUG messages with expensive operations. +**Owner**: python-quality-assistant +**Applies when**: a Python `logger.debug(...)` / `logger.log(logging.DEBUG, ...)` call passes an f-string whose interpolation calls an expensive function (serializer, JSON dump, network fetch, large-collection traversal) instead of using `%s` placeholders with deferred arguments. +**Enforcement**: judgment (ast-grep is a first-pass filter only: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument matching `f"..."`. Trivial f-string interpolations like `logger.debug(f"count={count}")` are NOT violations — the rule targets *expensive* interpolations (serializers, JSON dumps, network fetches, large-collection traversals). The agent makes the expensive-vs-trivial judgment after the mechanical filter.) +**Why**: F-strings interpolate immediately at function-call time. With `logger.debug(f"...{expensive(x)}...")`, `expensive(x)` runs every call, even when DEBUG is filtered out and the message is discarded. `logger.debug("... %s ...", expensive(x))` defers `expensive(x)` to the logging library, which only evaluates arguments when the level is enabled. In hot paths with DEBUG-by-default-off production, the difference between "free" and "this expensive call runs millions of times for log lines no one sees" is measurable. The `isEnabledFor(logging.DEBUG)` guard is the explicit alternative; `%s` is the implicit-defer pattern that scales. -**Rationale:** `%s` formatting only evaluates arguments if DEBUG level is enabled, avoiding unnecessary computation. +#### Bad -**Examples:** ```python -# [BAD] F-string with expensive DEBUG operation +# F-string forces expensive_serialization to run on every call, +# even when DEBUG is filtered out and the message is discarded logger.debug(f"Details: {expensive_serialization(large_object)}") -# Always evaluates, even when DEBUG disabled +``` + +#### Good -# [GOOD] Lazy evaluation with %s +```python +# %s placeholder defers evaluation to the logging library — runs only if DEBUG enabled logger.debug("Details: %s", expensive_serialization(large_object)) -# Only evaluates if DEBUG enabled -# [GOOD] Explicit guard for expensive operations +# Explicit guard — same effect, more readable when you also need f-string features if logger.isEnabledFor(logging.DEBUG): logger.debug(f"Details: {expensive_serialization(large_object)}") ``` diff --git a/rules/index.json b/rules/index.json index 069de26..52816f0 100644 --- a/rules/index.json +++ b/rules/index.json @@ -610,5 +610,59 @@ "id": "go-time/no-time-time-in-fields", "level": "MUST", "owner": "go-time-assistant" + }, + { + "anchor": "python-architecture/constructor-injection-only", + "applies_when": "a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a *method parameter* instead of through `__init__`. Related but distinct anti-patterns covered by other rules: dependency from a global module variable (see `python-architecture/main-py-composition-root`); post-construction `self.foo = bar.foo` mutation (see `python-ioc/dependencies-as-private-fields`, which mandates private-field storage that makes mutation socially difficult).", + "doc_path": "docs/python-architecture-patterns.md", + "enforcement": "judgment (semantic — requires distinguishing dependency injection from runtime parameter passing. The other two related anti-patterns have their own rules and enforcement paths.)", + "id": "python-architecture/constructor-injection-only", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-architecture/main-py-composition-root", + "applies_when": "a Python application instantiates service objects at module level (top of `repository.py`, `handler.py`, etc.) instead of wiring them inside `main()` / `main.py`.", + "doc_path": "docs/python-architecture-patterns.md", + "enforcement": "judgment (file-scope check: module-level `Service()` / `Repository()` calls outside `if __name__ == \"__main__\":` blocks)", + "id": "python-architecture/main-py-composition-root", + "level": "SHOULD", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-ioc/dependencies-as-private-fields", + "applies_when": "a Python service class stores an injected dependency on `self.` (public attribute) instead of `self._` (single-underscore private convention).", + "doc_path": "docs/python-ioc-guide.md", + "enforcement": "judgment (ast-grep first-pass filter: any `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_`. Type-annotation resolution to distinguish service deps from public data fields cannot be done in ast-grep — the agent makes the final call. Overinclusive first-pass is acceptable: most public `self.X` assignments in service classes ARE the smell.)", + "id": "python-ioc/dependencies-as-private-fields", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-ioc/protocol-not-abc-for-dependencies", + "applies_when": "a Python service interface (dependency surface, e.g. `UserRepository`, `Logger`) is declared with `abc.ABC` + `@abstractmethod` decorators instead of `typing.Protocol`, AND no shared implementation across concrete types motivates the ABC.", + "doc_path": "docs/python-ioc-guide.md", + "enforcement": "judgment (ast-grep partial: detect `class X(ABC):` declarations that only contain `@abstractmethod` methods — but the \"no shared impl needed\" trigger is semantic)", + "id": "python-ioc/protocol-not-abc-for-dependencies", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-logging/configure-once-in-main", + "applies_when": "a Python file in *library code* (modules imported by application code, intended for reuse across applications) calls `logging.basicConfig()` or adds handlers to the root logger. **Exempt**: the application entry point (`main.py` / `__main__.py` / `cli.py`) AND application-private helper modules wired from the entry point exclusively for configuration (e.g. `logging_setup.py` — see the next section), since those are part of the entry-point configuration surface, not library code.", + "doc_path": "docs/python-logging-guide.md", + "enforcement": "judgment (semantic — distinguishing library code from application-private configuration helpers requires reading the import graph; ast-grep can flag every non-entry-point `logging.basicConfig` call as a first-pass filter, but the agent must rule out the helper-module exemption case)", + "id": "python-logging/configure-once-in-main", + "level": "MUST", + "owner": "python-quality-assistant" + }, + { + "anchor": "python-logging/lazy-evaluation-for-debug", + "applies_when": "a Python `logger.debug(...)` / `logger.log(logging.DEBUG, ...)` call passes an f-string whose interpolation calls an expensive function (serializer, JSON dump, network fetch, large-collection traversal) instead of using `%s` placeholders with deferred arguments.", + "doc_path": "docs/python-logging-guide.md", + "enforcement": "judgment (ast-grep is a first-pass filter only: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument matching `f\"...\"`. Trivial f-string interpolations like `logger.debug(f\"count={count}\")` are NOT violations — the rule targets *expensive* interpolations (serializers, JSON dumps, network fetches, large-collection traversals). The agent makes the expensive-vs-trivial judgment after the mechanical filter.)", + "id": "python-logging/lazy-evaluation-for-debug", + "level": "MUST", + "owner": "python-quality-assistant" } ]