docs(claude): add .claude/rules for Python code/review conventions#50
Conversation
Adds four path-scoped rule files that auto-load when Claude Code touches matching Python files, and updates CLAUDE.md to index them. - python-style.md: tooling (uv/ruff/ty), structlog, naming, docstrings, error handling, FastAPI + MCP patterns, security. - python-types.md: strict typing (no Any, no untyped dicts), decision tree for Pydantic/TypedDict/dataclass, boundary parsing, str Enum for discriminators. - python-testing.md: pytest layout, fixtures, parametrize, mock at I/O boundaries only, no-network in unit tests, async patterns, FastAPI and MCP test patterns. - test-quality.md: 11-rule rubric used by reviewers (human or agent) to evaluate test design and assertion strength. Adapted from community-converged 2026 conventions and the claude-plugins reference rule set, tailored to this repo (Python 3.13, src/mcp_template_py/, FastAPI + MCP + Pydantic v2 + structlog).
lorr1
force-pushed
the
claude-rules-python
branch
from
db39eb0 to
79c021e
Compare
glageju
left a comment
glageju
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: docs-accuracy, consistency, python-correctness, docs-quality, codex (gpt-5.5)
Consensus Summary
| # | Finding | Score | Severity | Action |
|---|---|---|---|---|
| F1 | httpx.AsyncClient(app=app) example uses removed API | 9/10 | HIGH | Fix |
| F4 | # type: ignore[code] is mypy syntax; project uses ty |
9/10 | MEDIUM | Fix |
| F5 | Sync FastAPI handler claim is technically inaccurate | 9/10 | MEDIUM | Fix |
| F10 | Centralized test layout contradicts surviving CLAUDE.md text | 9/10 | MEDIUM | Fix |
| F11 | Template-repo: McpTemplateError / src/mcp_template_py/ baked in |
9/10 | MEDIUM | Fix |
| F9 | Stale task compose_agent_local in CLAUDE.md not fixed |
8/10 | MEDIUM | Fix |
| F14 | CLAUDE.md 'Technical considerations' duplicates new rule files | 8/10 | MEDIUM | Fix |
| F15 | **/*_test.py Go-style glob in two rule files |
8/10 | LOW | Fix |
| F2 | from __future__ import annotations mandate vs Pydantic on 3.13 |
7/10 | MEDIUM | Fix |
| F3 | # any: <reason> is invented; phrased as if tool-enforced |
7/10 | MEDIUM | Fix |
| F7 | Test-layout block presents non-existent dirs as fact | 7/10 | MEDIUM | Fix |
| F12 | blocker: framing is self-contradictory in test-quality.md |
7/10 | MEDIUM | Fix |
| F13 | Rule 11 severity disagrees with python-types.md Hard Rule 1 | 7/10 | MEDIUM | Fix |
| F8 | tests/helpers/ (Rule 4) doesn't exist |
7/10 | LOW | Fix |
| F17 | Frontmatter description asymmetry | 7/10 | LOW | Fix |
| F19/F43 | Cross-references use fragile rule numbers | 7/10 | LOW | Fix |
| F22 | structlog exc_info=True vs idiomatic log.exception(...) |
7/10 | LOW | Fix |
| F23 | Bandit suppression example uses uncommon form | 7/10 | LOW | Fix |
| F24 | MCP_AUTH_MODE fixture env var unverified |
7/10 | LOW | Fix |
| F26 | json.loads → dict[str, object] parenthetical incorrect |
7/10 | LOW | Fix |
| F28 | Async fixture scope ↔ asyncio_default_fixture_loop_scope not covered |
7/10 | LOW | Fix |
Overall
The approach — splitting strict typing, style, test-writing, and test-evaluation into four path-scoped files keyed on paths: globs — is sound and a strong fit for the CLAUDE.md context-budget problem. The findings below are about specific words, not the architecture.
Two findings rise above polish. F1 prescribes httpx.AsyncClient(app=app), which httpx 0.28 removed; agents and humans copying this pattern will hit a runtime error. F4 mandates # type: ignore[code] (mypy syntax) for suppressions, but the project uses ty, whose syntax is # ty: ignore[rule-name] — following the rule produces suppressions that don't actually suppress. Both are mechanical fixes worth landing before merge.
F11 is the longer-tail concern: this is a template repo, so hard-coded references to McpTemplateError and src/mcp_template_py/ will be wrong on day one in every downstream fork. Either abstract them or add a one-line note explaining they're template-specific placeholders.
Three findings (F9, F10, F14) are about CLAUDE.md leftovers the PR is already in a position to fix: a stale task compose_agent_local reference, a contradicted test-layout sentence, and duplicated uv/Taskfile bullets. The 'rule file wins' tiebreaker added in this PR papers over them rather than resolving them, and the project's own meta-CLAUDE explicitly warns against that pattern. See the inline comment on the rules-index area for specifics.
Recommendation: COMMENT — non-blocking, but several substantive fixes worth landing in this PR.
Dropped findings (auditability)
- codex flagged: "
pytest --covclaim conflicts with Taskfile (uv run pytest -vno--cov)". Dismissed by docs-accuracy reviewer with overrideNOTE(review): pyproject.toml hasaddopts = "--cov=mcp_template_py --cov-report html", so pytest's addopts auto-applies--covto every invocation including the Taskfile'spytest -v. Rule claim is accurate. mcp.shared.memorybrittleness concern (raised by docs-accuracy + python-correctness): codex independently confirmed the path is current; downgraded.- ~22 additional cosmetic / defensible-house-style findings (full records preserved locally).
Generated with Claude Code
|
|
||
| ## FastAPI tests | ||
|
|
||
| - `fastapi.testclient.TestClient` for sync style, `httpx.AsyncClient(app=app)` for async. |
There was a problem hiding this comment.
[HIGH · 9/10] httpx.AsyncClient(app=app) example uses removed API
app= was deprecated in httpx 0.27 and removed in httpx 0.28. Since this rule auto-loads on every test file, copy-pasters will hit a runtime error on a current httpx.
Raised by: python-correctness, docs-quality, codex
| - `fastapi.testclient.TestClient` for sync style, `httpx.AsyncClient(app=app)` for async. | |
| - `fastapi.testclient.TestClient` for sync style, `httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://test")` for async. |
There was a problem hiding this comment.
Fixed in 4febeb3 — switched to httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://test").
|
|
||
| - stdlib / third-party / first-party, blank-line separated. ruff's `I` rule enforces it. | ||
| - Absolute imports inside the package; relative imports max one level deep. No wildcards. | ||
| - `from __future__ import annotations` at the top of every module. |
There was a problem hiding this comment.
[MEDIUM · 7/10] from __future__ import annotations mandate on 3.13
Most of this rule's motivation (PEP 604 unions, PEP 695 generics) is now in-language on 3.13, while deferred annotations make Pydantic v2 get_type_hints and FastAPI Depends resolution more fragile around forward refs in pydantic model and DI modules.
Consider softening to default-with-carveouts ('except in modules defining Pydantic models or FastAPI dependencies — those need eagerly-evaluated annotations'), or dropping the rule now that the floor is 3.13.
Raised by: docs-accuracy, python-correctness, docs-quality (codex argued the rule is safe-if-not-strictly-necessary; the Pydantic interaction risk remains real)
There was a problem hiding this comment.
Dropped the rule entirely in 4febeb3.
Reasoning: with requires-python = ">=3.13", the original motivation (PEP 604 unions, PEP 695 generics) is in-language without from __future__ import annotations. The remaining benefit is faster startup from deferred evaluation, but in this stack the cost is real — Pydantic v2 get_type_hints and FastAPI Depends resolution both walk annotations eagerly and get flakier around forward refs when the module is from __future__-d. Net: more breakage than benefit on 3.13, so removing the mandate rather than carving out exceptions.
| - Pydantic models for every request/response body. Never `dict` or `Any` — the OpenAPI schema is part of the contract. | ||
| - Dependencies via `Depends(...)`. `app.dependency_overrides[real] = fake` is the test seam. | ||
| - Explicit status codes (`status_code=201`); errors raise `HTTPException` with a specific code. | ||
| - Async handlers unless the work is genuinely CPU-bound — a sync handler in an async app blocks the worker. |
There was a problem hiding this comment.
[MEDIUM · 9/10] Sync FastAPI handler claim is technically inaccurate
FastAPI/Starlette runs def path operations in an anyio threadpool — they do not block the event loop or the worker. The real failure mode is threadpool saturation (default ~40 threads) and the thread-hop overhead. The current wording leads readers to 'fix' sync handlers for the wrong reason and miss the actual capacity ceiling.
Raised by: python-correctness, codex
| - Async handlers unless the work is genuinely CPU-bound — a sync handler in an async app blocks the worker. | |
| - Async handlers for I/O-bound work. FastAPI runs sync `def` handlers in a threadpool, which is fine for occasional CPU work but caps concurrency at the threadpool size and adds a thread hop — async handlers scale better for I/O. Use sync only when the work is genuinely CPU-bound or calls a sync-only library you can't wrap. |
There was a problem hiding this comment.
Fixed in 4febeb3 — reworded to name the actual failure mode (anyio threadpool ~40-worker cap on concurrent I/O-bound sync handlers), not "blocks the worker".
|
|
||
| - No bare `except:` or `except BaseException:`. Catch the narrowest type that means something. | ||
| - Never silently swallow. Either log at WARNING with `exc_info=True` and comment why continuing is safe, or use `contextlib.suppress(SpecificError)` for genuinely-anticipated no-action exceptions. | ||
| - Define a small per-package exception hierarchy (`McpTemplateError` → narrower subclasses). No bare `Exception` / `RuntimeError`. |
There was a problem hiding this comment.
[MEDIUM · 9/10] Template-repo: McpTemplateError will be wrong in every downstream fork
McpTemplateError does not exist in the current source tree and is package-name-specific. Because this is a TEMPLATE repo (users fork via 'Use this template' and rename), every fork will inherit a rule referencing a class that doesn't exist under its renamed package. Same concern for src/mcp_template_py/auth/ later in this file.
Either abstract the references (<PackageError>, src/<package_name>/) or add a top-level note marking template-specific names that fork users must rename.
Raised by: docs-accuracy, docs-quality, codex
There was a problem hiding this comment.
Fixed in 4febeb3 — abstracted to <PackageError> and src/<package_name>/ throughout python-style.md, plus a top-of-file template note explaining the placeholder convention. Forks now have one obvious thing to substitute rather than inheriting broken refs.
| ## Error handling | ||
|
|
||
| - No bare `except:` or `except BaseException:`. Catch the narrowest type that means something. | ||
| - Never silently swallow. Either log at WARNING with `exc_info=True` and comment why continuing is safe, or use `contextlib.suppress(SpecificError)` for genuinely-anticipated no-action exceptions. |
There was a problem hiding this comment.
[LOW · 7/10] structlog exc_info=True works but log.exception(...) is more idiomatic inside except
Inside an except block, structlog's log.exception("event", ...) auto-attaches the active exception — cleaner than the stdlib-style exc_info=True. Either is fine; worth showing the preferred form since the rule mandates structlog two lines up.
Raised by: python-correctness
There was a problem hiding this comment.
Fixed in 4febeb3 — switched the error-handling guidance to prefer log.exception("event", ...) inside except, with contextlib.suppress as the alternative for anticipated no-action cases.
| ### Rule 11 — Type-checked tests | ||
| Tests run through `ty`. Fixtures have return types. Mocks use `Mock(spec=Foo)` / `AsyncMock(spec=Foo)` so attribute typos fail at construction. No `Any`, no untyped fixture parameters. | ||
| - **❌ when:** the test file fails type checking, a fixture/test has untyped parameters or return type, or `Mock()` is constructed without `spec=`. | ||
| - **Severity:** `suggestion:` for missing return types on individual test functions; `blocker:` when the file fails type checking or constructs unspecced Mocks for the unit under test. |
There was a problem hiding this comment.
[MEDIUM · 7/10] Rule 11 severity disagrees with python-types.md Hard Rule 1
python-types.md Hard Rule 1 says 'Every parameter and return value is typed. No exceptions.' and Hard Rule 5 calls task typecheck a merge gate. Rule 11 here grades missing return types on test functions as only suggestion:. If task typecheck actually fails on untyped tests, the suggestion: severity is wrong — that's a merge-blocking failure, not a non-blocker.
Either tighten Rule 11 to blocker: for any missing type in a test file, or relax python-types.md Hard Rule 1 to scope test-function return types as advisory. The two files need to agree.
Raised by: consistency
There was a problem hiding this comment.
Fixed in 4febeb3 by tightening both sides:
python-types.mdHard Rule 1 is now scoped to non-test code; test-function return types are explicitly allowed to be omitted (test bodies do not return values, and pytest discovery does not require them).- Rule 11 here now blocks on untyped fixture parameters and unspecced Mocks for the unit under test;
suggestion:is reserved for missing return-type annotations on individual test functions.
The two files now agree: missing test return types are a suggestion, everything else in test typing is a blocker.
| paths: | ||
| - "**/tests/**/*.py" | ||
| - "**/test_*.py" | ||
| - "**/*_test.py" |
There was a problem hiding this comment.
[LOW · 8/10] **/*_test.py Go-style glob also dead here
Same issue as in python-testing.md frontmatter — _test.py is not pytest's discovery pattern, and nothing in this repo uses it. If removed in python-testing.md, remove it here too.
Raised by: docs-quality, codex
| - "**/*_test.py" | |
| - "**/tests/**/*.py" | |
| - "**/test_*.py" |
There was a problem hiding this comment.
Fixed in 4febeb3 — dropped **/*_test.py from this file too.
| - "**/tests/**/*.py" | ||
| - "**/test_*.py" | ||
| - "**/*_test.py" | ||
| description: Test quality rubric — 11 rules reviewers apply to judge test design and assertion strength. |
There was a problem hiding this comment.
[LOW · 7/10] Frontmatter description doesn't disclaim out-of-scope concerns
python-style.md's description says 'Type-strictness rules live in python-types.md.' python-testing.md's description says 'Quality rubric in test-quality.md.' These help readers orient when they land on one file. Symmetrically, this file could note 'Patterns for writing tests in python-testing.md', and python-types.md could note 'Style/tooling in python-style.md'.
Raised by: consistency
There was a problem hiding this comment.
Fixed in 4febeb3 — added symmetric "see also" pointers to all four rule files (python-style.md, python-types.md, python-testing.md, test-quality.md) so any landing point links to the other three.
|
|
||
| ## Tests are typed too | ||
|
|
||
| Test files run through `ty`. Fixtures have return types. Mocks declare what they're mocking — `Mock(spec=Foo)` / `AsyncMock(spec=Foo)`. See `test-quality.md` Rule 11. |
There was a problem hiding this comment.
[LOW · 7/10] Cross-reference to 'Rule 11' is a fragile foreign key
If anyone adds, removes, or reorders rules in test-quality.md, this cross-reference rots silently. Use the rule's name or the heading anchor instead, e.g. see test-quality.md#rule-11--type-checked-tests. (Also worth back-referencing from test-quality.md Rule 11 to python-types.md — currently one-way.)
Raised by: consistency, docs-quality
There was a problem hiding this comment.
Fixed in 4febeb3 — replaced the bare "Rule 11" reference with a heading anchor (test-quality.md#rule-11--type-checked-tests). Also tightened Rule 11 itself to back-reference python-types.md Hard Rule 2 by name.
| reviewers (human and agent) to evaluate test design. Loads for | ||
| test files. | ||
|
|
||
| When the rules below conflict with anything in `.claude/rules/`, the |
There was a problem hiding this comment.
[MEDIUM · 8/10] 'Rule file wins' clause leaves real contradictions in CLAUDE.md unresolved
The clause is sound policy, but the document still contains three concrete conflicts the PR is in a position to just fix:
- F9 (8/10): existing CLAUDE.md references
task compose_agent_local, which doesn't exist — the Taskfile definestask compose. No new rule covers deployment, so 'rule file wins' doesn't help. Fix totask composedirectly. - F10 (9/10): existing CLAUDE.md says 'Unit tests should be in a
tests/folder co-located with the source code being tested', directly contradictingpython-testing.md's centralized layout. Delete the stale sentence rather than relying on the override clause. - F14 (8/10): existing 'Technical considerations' bullets duplicate uv/Taskfile guidance now in
python-style.md. Two sources of truth invite drift; the project's own meta-CLAUDE says 'don't leave a stale top-level doc for the next person'.
Raised by: docs-accuracy, consistency, docs-quality, codex
There was a problem hiding this comment.
Fixed all three in 4febeb3 by editing CLAUDE.md directly rather than relying on the override clause:
- F9:
task compose_agent_local→task composein the new Deployment section. - F10: Deleted the "Unit tests should be in a
tests/folder co-located with the source code" sentence. - F14: Removed the "Technical considerations" block. The uv / Taskfile /
listvsList/uv run python -cguidance now lives only inpython-style.md.
CLAUDE.md keeps the project blurb, rules index, deployment two-liner, code-structure note, and a tight implementation-guidelines block.
Fixes from multi-agent consensus review on PR #50: - python-testing.md: replace removed httpx.AsyncClient(app=app) with ASGITransport form; drop Go-style **/*_test.py glob; mark scaffold dirs (top-level conftest, fixtures) as target layout; swap unverified MCP_AUTH_MODE fixture example for REQUIRE_BEARER_TOKEN; document asyncio_default_fixture_loop_scope footgun - python-types.md: replace mypy # type: ignore[code] syntax with ty # ty: ignore[rule-name]; clarify # any: <reason> is a reviewer-enforced convention (not tool-enforced); fix json.loads typed as Any (not dict[str, object]); scope Hard Rule 1 to non-test code; anchor cross-ref to test-quality.md Rule 11; symmetric "see also" pointer to sibling rule files - python-style.md: abstract template-specific names (<PackageError>, src/<package_name>/) so forks do not inherit broken refs; drop from __future__ import annotations mandate (PEP 604/695 unions are in-language on 3.13 and deferred annotations make Pydantic v2 model resolution flaky); reword sync FastAPI handler claim (threadpool saturation, not event-loop blocking); prefer log.exception over exc_info=True inside except; concrete bandit suppression example - test-quality.md: drop Go-style _test.py glob; remove tests/helpers/ reference (directory does not exist); resolve blocker: framing contradiction (blocker: now consistently merge-blocking); align Rule 11 severity with python-types.md (untyped fixture params are a blocker, missing test-function return types stay suggestion:); symmetric "see also" pointer - CLAUDE.md: drop stale task compose_agent_local (Taskfile defines task compose); delete contradicted "co-located tests" sentence; remove uv/Taskfile bullets duplicated in python-style.md
|
Addressed all 21 consensus findings in 4febeb3 — in-thread replies on each. Summary of judgment calls:
Conflict-resolution batch (F9 / F10 / F14): edited CLAUDE.md directly instead of relying on the override clause — stale |
2 participants
Adds four path-scoped rule files under
.claude/rules/that Claude Code auto-loads when touching matching Python files, plus a rules index inCLAUDE.md.Summary
.claude/rules/python-style.md(**/*.py) — tooling (uv,ruff,ty), structlog logging, naming, docstrings, error handling, FastAPI + MCP patterns, security..claude/rules/python-types.md(**/*.py) — strict typing: noAnywithout a comment, no untypeddict/list, decision tree forpydantic.BaseModel/TypedDict/@dataclass, boundary parsing rule,str Enumfor discriminators..claude/rules/python-testing.md(test files) — pytest layout, fixtures, parametrize-over-loops, mock at I/O boundaries only, no-network in unit tests, async patterns, FastAPI + MCP test patterns..claude/rules/test-quality.md(test files) — 11-rule rubric used by reviewers (human or agent) to evaluate test design and assertion strength.CLAUDE.md— added a rules index pointing at the four files. Existing guidance kept.How rules load
Each file has
paths:frontmatter, so it only loads into Claude Code's context when files matching the glob are touched. This keeps the always-onCLAUDE.mdshort while letting detailed conventions live close to the code they govern.Sources
Adapted from community-converged Python conventions for AI coding (May 2026) plus the curated rule set in
claude-plugins:claude-plugins/.claude/rules/(Oz's curated set)Tailored to this repo's reality: Python 3.13,
src/mcp_template_py/, FastAPI + MCP + Pydantic v2 + structlog, Taskfile commands (task lint/typecheck/test/security).Test plan
python-style.md+python-types.mdare visible in the rules context.python-testing.md+test-quality.mdare visible.task checkshould pass unchanged.