diff --git a/skills/python-standards/SKILL.md b/skills/python-standards/SKILL.md new file mode 100644 index 0000000..f04a9e1 --- /dev/null +++ b/skills/python-standards/SKILL.md @@ -0,0 +1,114 @@ +--- +name: python-standards +description: >- + Python coding standards, that covers repository-first conventions, type annotations, error handling, testing, logging, documentation, async code, and dependency hygiene. Activate this skill whenever the user is editing or creating .py files, working in a Python project, or asks for help writing functions, classes, tests, modules, or fixing code - even if they do not say "Python" explicitly. Also triggers on questions about coding conventions, type hints, pytest, imports, logging setup, or dependency management in a Python context. +--- + +# Python Coding Standards + +## Initial phase - Context probe + +**Repo conventions first.** Check for `pyproject.toml`, `setup.cfg`, `.pre-commit-config.yaml`, `CONTRIBUTING.md`. If found: read and follow them. Skip any rule below that conflicts. Don't introduce new tools unless the user explicitly asks. + +**Infer regime and extras from file path:** + +| Signal | Regime | Load extras | +|--------|--------|-------------| +| Path contains `tests/`, or filename matches `test_*.py` / `*_test.py` | strict | load `references/testing.md` | +| Path in `src/`, `app/`, or a named service package | strict | - | +| Path in `scripts/`, `tools/`, `notebooks/`, or a one-off utility | relaxed | - | +| Ambiguous - none of the above match | **ask the user**: "strict (production) or relaxed (script)?" | based on answer | + +**Relaxed regime:** annotate public API only; `print()` allowed; docstrings recommended but not required. + +--- + +## Type Annotations + +- Annotate all public function signatures (params + return). Relaxed: public API only. +- Annotate class attributes and instance variables. +- Use built-in generics: `list[str]`, `dict[str, int]`, `tuple[int, ...]` (Python 3.9+). +- Use `X | None` not `Optional[X]` (Python 3.10+). +- Create type aliases for complex types to improve readability. +- `# type: ignore` only when the type system genuinely can't express correct code. Always include the error code and a justification: `# type: ignore[assignment] # covariant return is safe here`. A bare `# type: ignore` is never acceptable. + +## Code Structure + +**Imports:** top of file; three blocks (stdlib / third-party / local) separated by blank lines; never `from x import *`. Local imports acceptable to break circular deps or defer startup cost. + +**Module:** one primary concern per module; `__init__.py` re-exports only, no logic; separate business logic from I/O so it's testable without mocking infrastructure. + +**Naming (PEP 8):** `snake_case` functions/variables, `PascalCase` classes, `UPPER_SNAKE_CASE` constants, `_prefix` for private. No abbreviations unless universally understood (`id`, `url`, `http`). + +## Error Handling + +- Catch specific exceptions. Broad `except Exception:` is acceptable only at top-level boundaries (e.g., request handlers, background workers) to log and re-raise or convert to a safe response - never silently swallow. +- Use `raise ... from err` to preserve the chain. +- Create domain-specific exception classes instead of reusing generic built-ins. +- Fail fast: validate inputs early, raise immediately on invalid state. +- Use `with` for all resources needing cleanup (files, connections, locks). + +## Runtime Validation + +- Validate external data (user input, API responses, env vars) at the ingestion boundary, not deep in logic. +- Strict by default: reject unknown fields and unexpected types; don't silently coerce. +- Keep validated data immutable after creation when practical. +- For vendor responses: lenient on unknown fields, strict on expected fields. + +## Logging + +- Use `logging.getLogger(__name__)` - never `print()` in production code. +- Lazy format: pass values as args - `logger.info("Processing %d items", count)` - not f-strings in log calls. +- f-strings are fine in exception messages (always evaluated): `raise ValueError(f"Invalid ID: {user_id}")`. +- Never log credentials, tokens, API keys, or PII. Sanitise or redact sensitive fields before logging. + +| Level | Use for | +|-------|---------| +| `DEBUG` | Dev detail: variable values, control flow | +| `INFO` | Operational events: request handled, job started | +| `WARNING` | Recoverable issues: retry succeeded, deprecated feature used | +| `ERROR` | Failures needing attention | +| `CRITICAL` | Non-recoverable: service can't start, data corruption | + +## Documentation + +- Docstring on every public module, class, and function. First line: single summary sentence. +- Follow the repo's existing docstring style (Google, NumPy, reST). No convention → pick one and be consistent. +- Comment the *why*, not the *what*. Remove commented-out code. No separator comments (`# ---`). + +## Idiomatic Python + +- f-strings for formatting (except log calls - see above). +- `pathlib.Path` over `os.path`. +- Dataclasses or typed models over plain dicts. `enum.Enum` for fixed sets. +- `@dataclass(frozen=True)` for immutable value objects. +- List/dict/set comprehensions over `map()`/`filter()` with lambdas. +- `if x is None` not `if not x` for None checks. +- `dict.get(key, default)` for optional lookups. Use assignment expressions (`:=`) to combine fetch and check: `if value := d.get("key"):` avoids a redundant double-lookup. +- No mutable default arguments - use `None` + conditional creation in body. +- `asyncio.gather()` for concurrent independent tasks. Never block the event loop with sync I/O. +- `asyncio.to_thread()` for blocking sync helpers; process pool for CPU-bound work. + +## Dependencies + +- Follow the repo's existing dependency-management approach. +- For new projects or repos already using modern packaging, prefer `pyproject.toml` with PEP 621. +- Match version constraints to project type: libraries → compatible ranges; applications → tighter lockfiles. +- Separate runtime from dev/test dependencies when tooling supports it. +- When adding or changing a dependency, update the related lockfile in the same change. + +## Database + +- Never inline SQL as string literals in Python. +- Define queries in `.sql` files. Consider `aiosql` to load them as typed callables. Not mandated; follow what the repo already uses. +- Always pass query parameters through the driver's binding - never string interpolation. + +## Security + +- Never commit credentials, API keys, or tokens. Load secrets from env vars or a secrets manager at runtime. +- Use the `secrets` module for tokens and random values - `random` is not cryptographically secure. +- Validate and sanitise all user-provided input before using it in queries, commands, or file paths. +- Set explicit timeouts on all network calls - hanging connections are a denial-of-service vector. +- Keep dependencies locked per the repo's process and audit them regularly. + +For code patterns and examples, see `references/examples.md`. diff --git a/skills/python-standards/evals/evals.json b/skills/python-standards/evals/evals.json new file mode 100644 index 0000000..5096780 --- /dev/null +++ b/skills/python-standards/evals/evals.json @@ -0,0 +1,70 @@ +{ + "skill_name": "python-standards", + "evals": [ + { + "id": 1, + "category": "happy-path", + "prompt": "Write a function that fetches user profiles from an external API, with proper error handling and type annotations.", + "context": "User is editing src/services/user_service.py", + "expected_output": "Function has full type annotations (params + return), uses httpx or similar with an explicit timeout, catches specific exceptions, uses raise...from for chaining, and follows the coding standards (no bare except, proper naming).", + "files": [], + "expectations": [ + "Function signature has type annotations for all parameters and return type", + "Network call has an explicit timeout set", + "Exception handling catches specific exceptions, not bare except or broad Exception", + "Uses raise...from to preserve exception chain", + "Naming follows snake_case for the function and variables", + "Uses modern Python syntax (union types or built-in generics where applicable)" + ] + }, + { + "id": 2, + "category": "happy-path", + "prompt": "Create a validation module that accepts user registration input (name, email, age) and validates it at the boundary. Use dataclasses or pydantic.", + "context": "User is creating src/validators/registration.py", + "expected_output": "Module defines a typed model or dataclass for the input, validates at the ingestion boundary, rejects invalid data strictly, and keeps validated data immutable. Docstrings are present on public classes/functions.", + "files": [], + "expectations": [ + "Uses a dataclass, pydantic model, or typed structure - not raw dicts", + "Validation happens at the boundary (constructor or parse method), not deep in logic", + "Invalid input is rejected with a clear error, not silently coerced", + "Public class/function has a docstring with a summary line", + "Type annotations are present on all public signatures", + "Imports are grouped correctly: stdlib, third-party, local" + ] + }, + { + "id": 3, + "category": "happy-path", + "prompt": "Write tests for the retry utility. It retries a function with exponential backoff and raises RetryExhaustedError after max attempts.", + "context": "User is editing tests/unit/test_retry.py", + "expected_output": "Tests are well-structured: one behaviour per test, external calls are mocked, fixtures are used for shared setup. Assertions use expected == actual order. No real network calls.", + "files": [], + "expectations": [ + "Each test function tests one behaviour (names do not contain 'and')", + "External dependencies are mocked, no real I/O or network calls", + "Uses assert expected == actual ordering", + "Tests verify observable behaviour: success after retry, failure after exhaustion, correct delay timing", + "Test function names follow test_ convention", + "Return type annotation -> None is present on test functions" + ] + }, + { + "id": 4, + "category": "happy-path", + "prompt": "Refactor this function to follow best practices:\n\ndef get_data(url, items=[]):\n import requests\n try:\n r = requests.get(url)\n data = r.json()\n except:\n print('error')\n return None\n for i in data:\n items.append(i)\n return items", + "context": "User is editing src/utils/fetcher.py", + "expected_output": "Refactored code fixes the mutable default argument, moves imports to top, catches specific exceptions, replaces print with logging, adds type annotations, and adds a timeout to the request.", + "files": [], + "expectations": [ + "Mutable default argument is replaced with None + conditional creation inside the body", + "Import is at the top of the file (or justified if local)", + "Bare except is replaced with specific exception types", + "print() is replaced with logging or the error is raised/re-raised", + "Type annotations are added to the function signature", + "Network call includes an explicit timeout", + "Uses if x is None pattern rather than if not x for the None check" + ] + } + ] +} diff --git a/skills/python-standards/evals/trigger-eval.json b/skills/python-standards/evals/trigger-eval.json new file mode 100644 index 0000000..bc4d4f1 --- /dev/null +++ b/skills/python-standards/evals/trigger-eval.json @@ -0,0 +1,135 @@ +[ + { + "id": "t01-write-function-in-py-file", + "query": "Write a function that processes CSV files and returns aggregated results.", + "context": "User is editing src/etl/processor.py", + "should_trigger": true, + "reason": "User is in a .py file and asks to write a function - context-based trigger." + }, + { + "id": "t02-review-module-quality", + "query": "Review this module for code quality and best practices.", + "context": "User is viewing src/handlers/auth.py", + "should_trigger": true, + "reason": "User is in a .py file and asks for quality review - context triggers the skill." + }, + { + "id": "t03-refactor-tests", + "query": "Refactor these tests to follow better patterns and reduce duplication.", + "context": "User is editing tests/unit/test_auth.py", + "should_trigger": true, + "reason": "Editing a .py test file and asking to refactor - context-based trigger." + }, + { + "id": "t04-conventions-question", + "query": "What are our coding conventions for type annotations and error handling?", + "context": "User is working in a Python project (pyproject.toml present)", + "should_trigger": true, + "reason": "Convention question inside a Python project - triggers on context + topic." + }, + { + "id": "t05-new-service-class", + "query": "Create a service class that connects to Redis and handles caching with proper error handling.", + "context": "User is creating src/services/cache_service.py", + "should_trigger": true, + "reason": "Creating a new .py file - context is sufficient to trigger." + }, + { + "id": "t06-fix-typing", + "query": "Add type annotations to this module and fix the mypy errors.", + "context": "User is editing src/utils/helpers.py", + "should_trigger": true, + "reason": "Working in .py file on type annotations - directly covered." + }, + { + "id": "t07-logging-question", + "query": "How should I set up logging in this service? Is print() okay?", + "context": "User is editing src/services/notifier.py", + "should_trigger": true, + "reason": "Logging question while in a .py file - triggers on context + topic." + }, + { + "id": "t08-async-function", + "query": "Write an async function that fetches data from multiple APIs concurrently.", + "context": "User is editing src/clients/multi_fetch.py", + "should_trigger": true, + "reason": "Writing async code in a .py file - skill covers async patterns." + }, + { + "id": "t09-fix-bare-excepts", + "query": "Can you fix this file? It has bare excepts and mutable defaults everywhere.", + "context": "User is editing src/legacy/importer.py", + "should_trigger": true, + "reason": "Fixing code smells in a .py file - directly covered by the skill." + }, + { + "id": "t10-add-dependency", + "query": "I'm adding httpx as a dependency. What's the right way to declare it?", + "context": "User is in a Python project with pyproject.toml", + "should_trigger": true, + "reason": "Dependency management in a Python project - covered by skill." + }, + { + "id": "t11-explicit-python-mention", + "query": "Write a Python script that parses JSON logs and groups errors by type.", + "context": "No specific file open", + "should_trigger": true, + "reason": "Explicit 'Python' keyword in prompt - still triggers even without file context." + }, + { + "id": "n01-javascript-function", + "query": "Write a TypeScript function that validates email addresses.", + "context": "User is editing src/validators/email.ts", + "should_trigger": false, + "reason": "TypeScript file context - not Python." + }, + { + "id": "n02-review-pr", + "query": "Review this PR diff for breaking changes before we merge.", + "context": "User is viewing a GitHub PR", + "should_trigger": false, + "reason": "PR review request - routes to pr-review skill." + }, + { + "id": "n03-create-issue", + "query": "Create a GitHub issue for the login timeout bug.", + "context": "No specific file open", + "should_trigger": false, + "reason": "Issue creation - routes to create-issue skill." + }, + { + "id": "n04-kudos", + "query": "Give kudos to Martin for fixing the deployment pipeline.", + "context": "No specific file open", + "should_trigger": false, + "reason": "Kudos workflow - routes to kudos skill." + }, + { + "id": "n05-sql-query", + "query": "Write a PostgreSQL query that joins users and orders tables.", + "context": "User is editing migrations/001_create_tables.sql", + "should_trigger": false, + "reason": "SQL file context - not Python." + }, + { + "id": "n06-docker-setup", + "query": "Create a Dockerfile for this application.", + "context": "User is editing Dockerfile", + "should_trigger": false, + "reason": "Docker config task, not Python code." + }, + { + "id": "n07-git-help", + "query": "How do I rebase my branch onto main and resolve conflicts?", + "context": "No specific file open", + "should_trigger": false, + "reason": "Git workflow question, not Python coding." + }, + { + "id": "n08-crossall-update", + "query": "Generate the crossall update report for the last two weeks.", + "context": "No specific file open", + "should_trigger": false, + "reason": "Progress reporting - routes to crossall-changes skill." + } +] diff --git a/skills/python-standards/references/examples.md b/skills/python-standards/references/examples.md new file mode 100644 index 0000000..879cbee --- /dev/null +++ b/skills/python-standards/references/examples.md @@ -0,0 +1,106 @@ +# Code Examples + +On-demand reference patterns. Load when you need a concrete illustration of a rule. + +## Type annotations + +```python +# Type alias for a complex structure +UserRecord = dict[str, str | int | None] + +def fetch_users(org_id: str, *, active_only: bool = True) -> list[UserRecord]: + ... +``` + +## Imports + +```python +import json +import logging +from pathlib import Path + +import httpx +from pydantic import BaseModel + +from core.config import Settings +from core.errors import AppError +``` + +## Error handling - custom exception + chaining + +```python +class VendorTimeoutError(AppError): + """Raised when a vendor API call exceeds the configured timeout.""" + + def __init__(self, vendor: str, timeout_seconds: float) -> None: + super().__init__(f"Vendor '{vendor}' did not respond within {timeout_seconds}s") + self.vendor = vendor + self.timeout_seconds = timeout_seconds + + +def call_vendor(vendor: str, payload: dict[str, str]) -> VendorResponse: + try: + response = client.post(url, json=payload, timeout=timeout) + except httpx.TimeoutException as err: + raise VendorTimeoutError(vendor, timeout) from err +``` + +## Resource management - context manager + +```python +from collections.abc import Generator +from contextlib import contextmanager + +@contextmanager +def db_transaction(conn: Connection) -> Generator[Cursor, None, None]: + cursor = conn.cursor() + try: + yield cursor + conn.commit() + except Exception: + conn.rollback() + raise +``` + +## Logging + +```python +logger.info("Processing %d items out of %d", processed, total) +logger.warning("Retrying request to %s (attempt %d of %d)", url, attempt, max_attempts) +``` + +## Mutable default argument + +```python +# Wrong +def process_items(items: list[str] = []) -> list[str]: ... + +# Right +def process_items(items: list[str] | None = None) -> list[str]: + if items is None: + items = [] + ... +``` + +## Docstring + +```python +def retry_with_backoff( + func: Callable[[], T], + max_attempts: int = 3, + base_delay: float = 1.0, +) -> T: + """Execute `func` with exponential backoff on failure. + + Args: + func: Zero-argument callable to retry. + max_attempts: Maximum number of attempts before giving up. + base_delay: Initial delay in seconds, doubled after each failure. + + Returns: + The return value of `func` on success. + + Raises: + RetryExhaustedError: If all attempts fail. + """ +``` diff --git a/skills/python-standards/references/testing.md b/skills/python-standards/references/testing.md new file mode 100644 index 0000000..17d064b --- /dev/null +++ b/skills/python-standards/references/testing.md @@ -0,0 +1,52 @@ +# Testing Reference +Loaded automatically when working in test files (`tests/`, `test_*.py`, `*_test.py`). + +## Structure +- Follow the repository's existing test layout first. +- Name test files `test_.py` and functions `test_` unless the repo already uses a different convention. +- Separate tests with no I/O from tests that use real services or containers. +- `unit/` subdirectory is one common pattern but not required - match what the repo already has. +- Group unit tests under a class decorated with `@pytest.mark.unit`. + +```python +@pytest.mark.unit +class TestJobState: + def test_terminal_states(self) -> None: + assert JobState.COMPLETED.value == "completed" + assert JobState.FAILED.value == "failed" + + def test_non_terminal_states(self) -> None: + assert JobState.ACCEPTED.value == "accepted" + assert JobState.PROCESSING.value == "processing" +``` + +## Writing tests +- One behaviour per test. If the name contains "and", it is probably two tests. +- `assert expected == actual` - known-good value on the left. +- Fixtures for shared setup. Avoid deep fixture chains that obscure what the test actually needs. +- Importing inside a test function is acceptable when testing a fresh import (e.g. singleton initialisation). +- Mock external dependencies (databases, APIs, file systems) in unit tests. No real network calls. +- Use `pytest-mock`'s `mocker` fixture - never import `unittest.mock` directly. + +```python +import pytest +from pytest_mock import MockerFixture + +@pytest.fixture +def sample_config() -> AppConfig: + return AppConfig(region="eu-west-1", timeout=30) + + +@pytest.mark.unit +class TestVendorClient: + def test_timeout_raises_after_configured_duration(self, sample_config: AppConfig, mocker: MockerFixture) -> None: + mocker.patch.object(HttpClient, "post", side_effect=httpx.TimeoutException("timeout")) + with pytest.raises(VendorTimeoutError) as exc_info: + call_vendor("test-vendor", {}, config=sample_config) + assert exc_info.value.timeout_seconds == 30 +``` + +## Test hygiene +- Tests must not depend on execution order. +- Tests must not share mutable state - each test starts from a clean slate. +- Test observable behaviour, not implementation details. A refactor that doesn't change behaviour shouldn't break tests.