mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-09 07:09:11 +02:00
Add agent skills for testing and code review (#2846)
This commit is contained in:
Jeremiah Lowin
GitHub
parent
4ae0cc205c
commit
16cd343ec5
4 changed files with 336 additions and 184 deletions
94
.claude/skills/code-review/SKILL.md
Normal file
94
.claude/skills/code-review/SKILL.md
Normal file
|
|
@ -0,0 +1,94 @@
|
|||
---
|
||||
name: reviewing-code
|
||||
description: Review code for quality, maintainability, and correctness. Use when reviewing pull requests, evaluating code changes, or providing feedback on implementations. Focuses on API design, patterns, and actionable feedback.
|
||||
---
|
||||
|
||||
# Code Review
|
||||
|
||||
## Philosophy
|
||||
|
||||
Code review maintains a healthy codebase while helping contributors succeed. The burden of proof is on the PR to demonstrate it adds value. Your job is to help it get there through actionable feedback.
|
||||
|
||||
**Critical**: A perfectly written PR that adds unwanted functionality must still be rejected. The code must advance the codebase in the intended direction. When rejecting, provide clear guidance on how to align with project goals.
|
||||
|
||||
Be friendly and welcoming while maintaining high standards. Call out what works well. When code needs improvement, be specific about why and how to fix it.
|
||||
|
||||
## What to Focus On
|
||||
|
||||
### Does this advance the codebase correctly?
|
||||
|
||||
Even perfect code for unwanted features should be rejected.
|
||||
|
||||
### API design and naming
|
||||
|
||||
Identify confusing patterns or non-idiomatic code:
|
||||
- Parameter values that contradict defaults
|
||||
- Mutable default arguments
|
||||
- Unclear naming that will confuse future readers
|
||||
- Inconsistent patterns with the rest of the codebase
|
||||
|
||||
### Specific improvements
|
||||
|
||||
Provide actionable feedback, not generic observations.
|
||||
|
||||
### User ergonomics
|
||||
|
||||
Think about the API from a user's perspective. Is it intuitive? What's the learning curve?
|
||||
|
||||
## For Agent Reviewers
|
||||
|
||||
1. **Read the full context**: Examine related files, tests, and documentation before reviewing
|
||||
2. **Check against established patterns**: Look for consistency with codebase conventions
|
||||
3. **Verify functionality claims**: Understand what the code actually does, not just what it claims
|
||||
4. **Consider edge cases**: Think through error conditions and boundary scenarios
|
||||
|
||||
## What to Avoid
|
||||
|
||||
- Generic feedback without specifics
|
||||
- Hypothetical problems unlikely to occur
|
||||
- Nitpicking organizational choices without strong reason
|
||||
- Summarizing what the PR already describes
|
||||
- Star ratings or excessive emojis
|
||||
- Bikeshedding style preferences when functionality is correct
|
||||
- Requesting changes without suggesting solutions
|
||||
- Focusing on personal coding style over project conventions
|
||||
|
||||
## Tone
|
||||
|
||||
- Acknowledge good decisions: "This API design is clean"
|
||||
- Be direct but respectful
|
||||
- Explain impact: "This will confuse users because..."
|
||||
- Remember: Someone else maintains this code forever
|
||||
|
||||
## Decision Framework
|
||||
|
||||
Before approving, ask:
|
||||
|
||||
1. Does this PR achieve its stated purpose?
|
||||
2. Is that purpose aligned with where the codebase should go?
|
||||
3. Would I be comfortable maintaining this code?
|
||||
4. Have I actually understood what it does, not just what it claims?
|
||||
5. Does this change introduce technical debt?
|
||||
|
||||
If something needs work, your review should help it get there through specific, actionable feedback. If it's solving the wrong problem, say so clearly.
|
||||
|
||||
## Comment Examples
|
||||
|
||||
**Good comments:**
|
||||
|
||||
| Instead of | Write |
|
||||
|------------|-------|
|
||||
| "Add more tests" | "The `handle_timeout` method needs tests for the edge case where timeout=0" |
|
||||
| "This API is confusing" | "The parameter name `data` is ambiguous - consider `message_content` to match the MCP specification" |
|
||||
| "This could be better" | "This approach works but creates a circular dependency. Consider moving the validation to `utils/validators.py`" |
|
||||
|
||||
## Checklist
|
||||
|
||||
Before approving, verify:
|
||||
|
||||
- [ ] All required development workflow steps completed (uv sync, prek, pytest)
|
||||
- [ ] Changes align with repository patterns and conventions
|
||||
- [ ] API changes are documented and backwards-compatible where possible
|
||||
- [ ] Error handling follows project patterns (specific exception types)
|
||||
- [ ] Tests cover new functionality and edge cases
|
||||
- [ ] The change advances the codebase in the intended direction
|
||||
220
.claude/skills/python-tests/SKILL.md
Normal file
220
.claude/skills/python-tests/SKILL.md
Normal file
|
|
@ -0,0 +1,220 @@
|
|||
---
|
||||
name: testing-python
|
||||
description: Write and evaluate effective Python tests using pytest. Use when writing tests, reviewing test code, debugging test failures, or improving test coverage. Covers test design, fixtures, parameterization, mocking, and async testing.
|
||||
---
|
||||
|
||||
# Writing Effective Python Tests
|
||||
|
||||
## Core Principles
|
||||
|
||||
Every test should be **atomic**, **self-contained**, and test **single functionality**. A test that tests multiple things is harder to debug and maintain.
|
||||
|
||||
## Test Structure
|
||||
|
||||
### Atomic unit tests
|
||||
|
||||
Each test should verify a single behavior. The test name should tell you what's broken when it fails. Multiple assertions are fine when they all verify the same behavior.
|
||||
|
||||
```python
|
||||
# Good: Name tells you what's broken
|
||||
def test_user_creation_sets_defaults():
|
||||
user = User(name="Alice")
|
||||
assert user.role == "member"
|
||||
assert user.id is not None
|
||||
assert user.created_at is not None
|
||||
|
||||
# Bad: If this fails, what behavior is broken?
|
||||
def test_user():
|
||||
user = User(name="Alice")
|
||||
assert user.role == "member"
|
||||
user.promote()
|
||||
assert user.role == "admin"
|
||||
assert user.can_delete_others()
|
||||
```
|
||||
|
||||
### Use parameterization for variations of the same concept
|
||||
|
||||
```python
|
||||
import pytest
|
||||
|
||||
@pytest.mark.parametrize("input,expected", [
|
||||
("hello", "HELLO"),
|
||||
("World", "WORLD"),
|
||||
("", ""),
|
||||
("123", "123"),
|
||||
])
|
||||
def test_uppercase_conversion(input, expected):
|
||||
assert input.upper() == expected
|
||||
```
|
||||
|
||||
### Use separate tests for different functionality
|
||||
|
||||
Don't parameterize unrelated behaviors. If the test logic differs, write separate tests.
|
||||
|
||||
## Project-Specific Rules
|
||||
|
||||
### No async markers needed
|
||||
|
||||
This project uses `asyncio_mode = "auto"` globally. Write async tests without decorators:
|
||||
|
||||
```python
|
||||
# Correct
|
||||
async def test_async_operation():
|
||||
result = await some_async_function()
|
||||
assert result == expected
|
||||
|
||||
# Wrong - don't add this
|
||||
@pytest.mark.asyncio
|
||||
async def test_async_operation():
|
||||
...
|
||||
```
|
||||
|
||||
### Imports at module level
|
||||
|
||||
Put ALL imports at the top of the file:
|
||||
|
||||
```python
|
||||
# Correct
|
||||
import pytest
|
||||
from fastmcp import FastMCP
|
||||
from fastmcp.client import Client
|
||||
|
||||
async def test_something():
|
||||
mcp = FastMCP("test")
|
||||
...
|
||||
|
||||
# Wrong - no local imports
|
||||
async def test_something():
|
||||
from fastmcp import FastMCP # Don't do this
|
||||
...
|
||||
```
|
||||
|
||||
### Use in-memory transport for testing
|
||||
|
||||
Pass FastMCP servers directly to clients:
|
||||
|
||||
```python
|
||||
from fastmcp import FastMCP
|
||||
from fastmcp.client import Client
|
||||
|
||||
mcp = FastMCP("TestServer")
|
||||
|
||||
@mcp.tool
|
||||
def greet(name: str) -> str:
|
||||
return f"Hello, {name}!"
|
||||
|
||||
async def test_greet_tool():
|
||||
async with Client(mcp) as client:
|
||||
result = await client.call_tool("greet", {"name": "World"})
|
||||
assert result[0].text == "Hello, World!"
|
||||
```
|
||||
|
||||
Only use HTTP transport when explicitly testing network features.
|
||||
|
||||
### Inline snapshots for complex data
|
||||
|
||||
Use `inline-snapshot` for testing JSON schemas and complex structures:
|
||||
|
||||
```python
|
||||
from inline_snapshot import snapshot
|
||||
|
||||
def test_schema_generation():
|
||||
schema = generate_schema(MyModel)
|
||||
assert schema == snapshot() # Will auto-populate on first run
|
||||
```
|
||||
|
||||
Commands:
|
||||
- `pytest --inline-snapshot=create` - populate empty snapshots
|
||||
- `pytest --inline-snapshot=fix` - update after intentional changes
|
||||
|
||||
## Fixtures
|
||||
|
||||
### Prefer function-scoped fixtures
|
||||
|
||||
```python
|
||||
@pytest.fixture
|
||||
def client():
|
||||
return Client()
|
||||
|
||||
async def test_with_client(client):
|
||||
result = await client.ping()
|
||||
assert result is not None
|
||||
```
|
||||
|
||||
### Use `tmp_path` for file operations
|
||||
|
||||
```python
|
||||
def test_file_writing(tmp_path):
|
||||
file = tmp_path / "test.txt"
|
||||
file.write_text("content")
|
||||
assert file.read_text() == "content"
|
||||
```
|
||||
|
||||
## Mocking
|
||||
|
||||
### Mock at the boundary
|
||||
|
||||
```python
|
||||
from unittest.mock import patch, AsyncMock
|
||||
|
||||
async def test_external_api_call():
|
||||
with patch("mymodule.external_client.fetch", new_callable=AsyncMock) as mock:
|
||||
mock.return_value = {"data": "test"}
|
||||
result = await my_function()
|
||||
assert result == {"data": "test"}
|
||||
```
|
||||
|
||||
### Don't mock what you own
|
||||
|
||||
Test your code with real implementations when possible. Mock external services, not internal classes.
|
||||
|
||||
## Test Naming
|
||||
|
||||
Use descriptive names that explain the scenario:
|
||||
|
||||
```python
|
||||
# Good
|
||||
def test_login_fails_with_invalid_password():
|
||||
def test_user_can_update_own_profile():
|
||||
def test_admin_can_delete_any_user():
|
||||
|
||||
# Bad
|
||||
def test_login():
|
||||
def test_update():
|
||||
def test_delete():
|
||||
```
|
||||
|
||||
## Error Testing
|
||||
|
||||
```python
|
||||
import pytest
|
||||
|
||||
def test_raises_on_invalid_input():
|
||||
with pytest.raises(ValueError, match="must be positive"):
|
||||
calculate(-1)
|
||||
|
||||
async def test_async_raises():
|
||||
with pytest.raises(ConnectionError):
|
||||
await connect_to_invalid_host()
|
||||
```
|
||||
|
||||
## Running Tests
|
||||
|
||||
```bash
|
||||
uv run pytest -n auto # Run all tests in parallel
|
||||
uv run pytest -n auto -x # Stop on first failure
|
||||
uv run pytest path/to/test.py # Run specific file
|
||||
uv run pytest -k "test_name" # Run tests matching pattern
|
||||
uv run pytest -m "not integration" # Exclude integration tests
|
||||
```
|
||||
|
||||
## Checklist
|
||||
|
||||
Before submitting tests:
|
||||
- [ ] Each test tests one thing
|
||||
- [ ] No `@pytest.mark.asyncio` decorators
|
||||
- [ ] Imports at module level
|
||||
- [ ] Descriptive test names
|
||||
- [ ] Using in-memory transport (not HTTP) unless testing networking
|
||||
- [ ] Parameterization for variations of same behavior
|
||||
- [ ] Separate tests for different behaviors
|
||||
|
|
@ -10,4 +10,4 @@ There are four major MCP object types:
|
|||
- Resource Templates (src/resources/)
|
||||
- Prompts (src/prompts)
|
||||
|
||||
While these have slightly different semantics and implementations, in general changes that affect interactions with any one (like adding tags, importing, etc.) will need to be adopted, applied, and tested on all others. Be sure to look at not only the object definition but also the related `Manager` (e.g. `ToolManager`, `ResourceManager`, and `PromptManager`). Also note that while resources and resource templates are different objects, they both are handled by the `ResourceManager`.
|
||||
While these have slightly different semantics and implementations, in general changes that affect interactions with any one (like adding tags, importing, etc.) will need to be adopted, applied, and tested on all others. Note that while resources and resource templates are different objects, they are both in `src/resources/`.
|
||||
204
AGENTS.md
204
AGENTS.md
|
|
@ -25,81 +25,30 @@ uv run pytest -n auto # Run full test suite
|
|||
|
||||
## Repository Structure
|
||||
|
||||
| Path | Purpose |
|
||||
| ------------------ | ----------------------------------------------------------------------------------- |
|
||||
| `src/fastmcp/` | Library source code (Python ≥ 3.10) |
|
||||
| `├─server/` | Server implementation, `FastMCP`, auth, networking |
|
||||
| `│ ├─auth/` | Authentication providers (Google, GitHub, Azure, AWS, WorkOS, Auth0, JWT, and more) |
|
||||
| `│ └─middleware/` | Error handling, logging, rate limiting |
|
||||
| `├─client/` | High-level client SDK + transports |
|
||||
| `│ └─auth/` | Client authentication (Bearer, OAuth) |
|
||||
| `├─tools/` | Tool implementations + `ToolManager` |
|
||||
| `├─resources/` | Resources, templates + `ResourceManager` |
|
||||
| `├─prompts/` | Prompt templates + `PromptManager` |
|
||||
| `├─cli/` | FastMCP CLI commands (`run`, `dev`, `install`) |
|
||||
| `├─contrib/` | Community contributions (bulk caller, mixins) |
|
||||
| `├─experimental/` | Experimental features (sampling handlers) |
|
||||
| `└─utilities/` | Shared utilities (logging, JSON schema, HTTP) |
|
||||
| `tests/` | Comprehensive pytest suite with markers |
|
||||
| `docs/` | Mintlify documentation (published to gofastmcp.com) |
|
||||
| `examples/` | Runnable demo servers (echo, smart_home, atproto) |
|
||||
| Path | Purpose |
|
||||
| ----------------- | -------------------------------------- |
|
||||
| `src/fastmcp/` | Library source code |
|
||||
| `├─server/` | Server implementation |
|
||||
| `│ ├─auth/` | Authentication providers |
|
||||
| `│ └─middleware/` | Error handling, logging, rate limiting |
|
||||
| `├─client/` | Client SDK |
|
||||
| `│ └─auth/` | Client authentication |
|
||||
| `├─tools/` | Tool definitions |
|
||||
| `├─resources/` | Resources and resource templates |
|
||||
| `├─prompts/` | Prompt templates |
|
||||
| `├─cli/` | CLI commands |
|
||||
| `└─utilities/` | Shared utilities |
|
||||
| `tests/` | Pytest suite |
|
||||
| `docs/` | Mintlify docs (gofastmcp.com) |
|
||||
|
||||
## Core MCP Objects
|
||||
|
||||
When modifying MCP functionality, changes typically need to be applied across all object types:
|
||||
|
||||
- **Tools** (`src/tools/` + `ToolManager`)
|
||||
- **Resources** (`src/resources/` + `ResourceManager`)
|
||||
- **Resource Templates** (`src/resources/` + `ResourceManager`)
|
||||
- **Prompts** (`src/prompts/` + `PromptManager`)
|
||||
|
||||
## Writing Style
|
||||
|
||||
- Be brief and to the point. Do not regurgitate information that can easily be gleaned from the code, except to guide the reader to where the code is located.
|
||||
- **NEVER** use "This isn't..." or "not just..." constructions. State what something IS directly. Avoid defensive writing patterns like:
|
||||
- "This isn't X, it's Y" or "Not just X, but Y" → Just say "This is Y"
|
||||
- "Not just about X" → State the actual purpose
|
||||
- "We're not doing X, we're doing Y" → Just explain what you're doing
|
||||
- Any variation of explaining what something isn't before what it is
|
||||
|
||||
## Testing Best Practices
|
||||
|
||||
### Testing Standards
|
||||
|
||||
- Every test: atomic, self-contained, single functionality
|
||||
- Use parameterization for multiple examples of same functionality
|
||||
- Use separate tests for different functionality pieces
|
||||
- **ALWAYS** Put imports at the top of the file, not in the test body
|
||||
- **NEVER** add `@pytest.mark.asyncio` to tests - `asyncio_mode = "auto"` is set globally
|
||||
- **ALWAYS** run pytest after significant changes
|
||||
|
||||
### Inline Snapshots
|
||||
|
||||
FastMCP uses `inline-snapshot` for testing complex data structures. On first run with empty `snapshot()`, pytest will auto-populate the expected value when running `pytest --inline-snapshot=create`. To update snapshots after intentional changes, run `pytest --inline-snapshot=fix`. This is particularly useful for testing JSON schemas and API responses.
|
||||
|
||||
### Always Use In-Memory Transport
|
||||
|
||||
Pass FastMCP servers directly to clients for testing:
|
||||
|
||||
```python
|
||||
mcp = FastMCP("TestServer")
|
||||
|
||||
@mcp.tool
|
||||
def greet(name: str) -> str:
|
||||
return f"Hello, {name}!"
|
||||
|
||||
# Direct connection - no network complexity
|
||||
async with Client(mcp) as client:
|
||||
result = await client.call_tool("greet", {"name": "World"})
|
||||
```
|
||||
|
||||
Only use HTTP transport when explicitly testing network features:
|
||||
|
||||
```python
|
||||
# Network testing only
|
||||
async with Client(transport=StreamableHttpTransport(server_url)) as client:
|
||||
result = await client.ping()
|
||||
```
|
||||
- **Tools** (`src/tools/`)
|
||||
- **Resources** (`src/resources/`)
|
||||
- **Resource Templates** (`src/resources/`)
|
||||
- **Prompts** (`src/prompts/`)
|
||||
|
||||
## Development Rules
|
||||
|
||||
|
|
@ -159,119 +108,8 @@ async with Client(transport=StreamableHttpTransport(server_url)) as client:
|
|||
- **Content:** User-focused sections, motivate features (why) before mechanics (how)
|
||||
- **Style:** Prose over code comments for important information
|
||||
|
||||
## Code Review Guidelines
|
||||
|
||||
### Philosophy
|
||||
|
||||
Code review is about maintaining a healthy codebase while helping contributors succeed. The burden of proof is on the PR to demonstrate it adds value in the intended way. Your job is to help it get there through actionable feedback.
|
||||
|
||||
**Critical**: A perfectly written PR that adds unwanted functionality must still be rejected. The code must advance the codebase in the intended direction, not just be well-written. When rejecting, provide clear guidance on how to align with project goals.
|
||||
|
||||
Be friendly and welcoming while maintaining high standards. Call out what works well - this reinforces good patterns. When code needs improvement, be specific about why and how to fix it. Remember that PRs serve as documentation for future developers.
|
||||
|
||||
### Focus On
|
||||
|
||||
- **Does this advance the codebase in the intended direction?** (Even perfect code for unwanted features should be rejected)
|
||||
- **API design and naming clarity** - Identify confusing patterns (e.g., parameter values that contradict defaults) or non-idiomatic code (mutable defaults, etc.). Contributed code will need to be maintained indefinitely, and by someone other than the author (unless the author is a maintainer).
|
||||
- **Suggest specific improvements**, not generic "add more tests" comments
|
||||
- **Think about API ergonomics and learning curve** from a user perspective
|
||||
|
||||
### For Agent Reviewers
|
||||
|
||||
- **Read the full context**: Always examine related files, tests, and documentation before reviewing
|
||||
- **Check against established patterns**: Look for consistency with existing codebase conventions
|
||||
- **Verify functionality claims**: Don't just read code - understand what it actually does
|
||||
- **Consider edge cases**: Think through error conditions and boundary scenarios
|
||||
|
||||
### Avoid
|
||||
|
||||
- Generic feedback without specifics
|
||||
- Hypothetical problems unlikely to occur
|
||||
- Nitpicking organizational choices without strong reason
|
||||
- Summarizing what the PR already describes
|
||||
- Star ratings or excessive emojis
|
||||
- Bikeshedding style preferences when functionality is correct
|
||||
- Requesting changes without suggesting solutions
|
||||
- Focusing on personal coding style over project conventions
|
||||
|
||||
### Tone
|
||||
|
||||
- Acknowledge good decisions ("This API design is clean")
|
||||
- Be direct but respectful
|
||||
- Explain impact ("This will confuse users because...")
|
||||
- Remember: Someone else maintains this code forever
|
||||
|
||||
### Decision Framework
|
||||
|
||||
Before approving, ask yourself:
|
||||
|
||||
1. Does this PR achieve its stated purpose?
|
||||
2. Is that purpose aligned with where the codebase should go?
|
||||
3. Would I be comfortable maintaining this code?
|
||||
4. Have I actually understood what it does, not just what it claims?
|
||||
5. Does this change introduce technical debt?
|
||||
|
||||
If something needs work, your review should help it get there through specific, actionable feedback. If it's solving the wrong problem, say so clearly.
|
||||
|
||||
### Review Comment Examples
|
||||
|
||||
**Good Review Comments:**
|
||||
|
||||
❌ "Add more tests"
|
||||
✅ "The `handle_timeout` method needs tests for the edge case where timeout=0"
|
||||
|
||||
❌ "This API is confusing"
|
||||
✅ "The parameter name `data` is ambiguous - consider `message_content` to match the MCP specification"
|
||||
|
||||
❌ "This could be better"
|
||||
✅ "This approach works but creates a circular dependency. Consider moving the validation to `utils/validators.py`"
|
||||
|
||||
### Review Checklist
|
||||
|
||||
Before approving, verify:
|
||||
|
||||
- [ ] All required development workflow steps completed (uv sync, prek, pytest)
|
||||
- [ ] Changes align with repository patterns and conventions
|
||||
- [ ] API changes are documented and backwards-compatible where possible
|
||||
- [ ] Error handling follows project patterns (specific exception types)
|
||||
- [ ] Tests cover new functionality and edge cases
|
||||
|
||||
## Key Tools & Commands
|
||||
|
||||
### Environment Setup
|
||||
|
||||
```bash
|
||||
git clone <repo>
|
||||
cd fastmcp
|
||||
uv sync # Installs all deps including dev tools
|
||||
```
|
||||
|
||||
### Validation Commands (Run Frequently)
|
||||
|
||||
- **Linting**: `uv run ruff check` (or with `--fix`)
|
||||
- **Type Checking**: `uv run ty check`
|
||||
- **All Checks**: `uv run prek run --all-files`
|
||||
|
||||
### Testing
|
||||
|
||||
- **Standard**: `uv run pytest -n auto`
|
||||
- **Integration**: `uv run pytest -n auto -m "integration"`
|
||||
- **Excluding markers**: `uv run pytest -n auto -m "not integration and not client_process"`
|
||||
|
||||
### CLI Usage
|
||||
|
||||
- **Run server**: `uv run fastmcp run server.py`
|
||||
- **Inspect server**: `uv run fastmcp inspect server.py`
|
||||
|
||||
## Critical Patterns
|
||||
|
||||
### Error Handling
|
||||
|
||||
- Never use bare `except` - be specific with exception types
|
||||
|
||||
### Build Issues (Common Solutions)
|
||||
|
||||
1. **Dependencies**: Always `uv sync` first
|
||||
2. **Prek fails**: Run `uv run prek run --all-files` to see failures
|
||||
3. **Type errors**: Use `uv run ty check` directly, check `pyproject.toml` config
|
||||
4. **Test timeouts**: Default 5s - optimize or mark as integration tests
|
||||
- Always `uv sync` first when debugging build issues
|
||||
- Default test timeout is 5s - optimize or mark as integration tests
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue