Agent Skills: Python Code Review Patterns

Python Code Review Patterns

Stack-specific rules loaded by dh:code-reviewer when pyproject.toml or *.py files are detected.

Type Annotations

• All public function boundaries must have type annotations — parameters and return type
• Any is only acceptable with an inline comment explaining why a specific type cannot be used
• TypedDict is required for dicts that cross module boundaries; plain dict[str, ...] is only acceptable within a single function
• Optional[X] is legacy — use X | None (Python 3.10+ union syntax)
• Union[X, Y] is legacy — use X | Y
• Return type None must be explicit on functions that have no return value but have side effects
• Protocol classes are preferred over ABC for structural typing

Ruff Compliance

• Bare except: is a blocking finding — must be except ExceptionType:
• print() in library code (non-CLI, non-script) is a blocking finding (T201) — use logging instead
• Magic numbers in comparisons are a blocking finding (PLR2004) — extract to named constants
• Unused imports must be removed, not commented out
• f-strings are required for string interpolation — % and .format() are legacy
• __all__ must be defined in modules that have public API surface

ty Type Safety

• Unresolved imports indicate a missing uv add — fix the import, not the type checker
• # ty: ignore suppressions are prohibited — fix the code to satisfy ty
• ty extra-paths in pyproject.toml must stay in sync with pytest pythonpath
• TypedDict nominal incompatibility across modules means the TypedDict should be defined in a shared location, not duplicated

pytest Patterns

• Test names follow behavioral naming: test_<what>_when_<condition>_returns_<result> or test_<what>_when_<condition>_raises_<exception>
• Tests assert on observable behavior, not on implementation details (internal state, private methods)
• Integration tests hit real dependencies — mocking the thing under test is not a test
• Fixtures are preferred over test-class instance state
• assert result is not None is not a meaningful assertion — assert the actual expected value
• Tests are isolated — no shared mutable state between test functions
• Parametrize repetitive test cases with @pytest.mark.parametrize
• Each test file mirrors the module it tests: src/foo/bar.py → tests/foo/test_bar.py

uv Usage

• All Python execution uses uv run — bare python invocations are only acceptable in CI where the venv is pre-activated
• New dependencies are added with uv add — manual edits to pyproject.toml dependencies without running uv add leave the lockfile stale
• Scripts with external dependencies declare PEP 723 inline metadata (# /// script) and also add those deps via uv add --dev for IDE tooling
• uv run --script is used for standalone scripts with PEP 723 metadata

Error Handling

• except Exception: is a blocking finding unless immediately followed by a re-raise or very specific logging
• Empty except blocks are a blocking finding — they swallow all errors silently
• Exception messages must include enough context to diagnose without reading the source: raise ValueError(f"Expected positive int, got {value!r}") not raise ValueError("invalid input")
• Sentinel return values (returning None or -1 on error without raising) require a documented contract — silence must be intentional and documented

Modern Python 3.11+ Idioms

• match statements are preferred for multi-branch dispatch on type or value (over long if/elif chains)
• pathlib.Path is required for all file path operations — os.path is legacy
• tomllib (stdlib in 3.11+) is used for reading TOML — do not add tomli as a dependency
• datetime.UTC is used instead of datetime.timezone.utc (Python 3.11+)
• ExceptionGroup and except* are used for concurrent exception handling where appropriate
• str.removeprefix() and str.removesuffix() replace manual slicing for prefix/suffix removal

Anti-Patterns

# WRONG: bare except
try:
    do_thing()
except:
    pass

# RIGHT: narrow except with action
try:
    do_thing()
except ConnectionError as e:
    logger.warning("Connection failed: %s", e)
    raise

# WRONG: magic number
if status == 429:
    ...

# RIGHT: named constant
HTTP_TOO_MANY_REQUESTS = 429
if status == HTTP_TOO_MANY_REQUESTS:
    ...

# WRONG: os.path
import os
path = os.path.join(base, "config.toml")

# RIGHT: pathlib
from pathlib import Path
path = Path(base) / "config.toml"