|
| 1 | +# Development Guidelines |
| 2 | + |
| 3 | +## Branching Model |
| 4 | + |
| 5 | +<!-- TODO: drop this section once v2 ships and main becomes the stable line --> |
| 6 | + |
| 7 | +- `main` is currently the V2 rework. Breaking changes are expected here — when removing or |
| 8 | + replacing an API, delete it outright and document the change in |
| 9 | + `docs/migration.md`. Do not add `@deprecated` shims or backward-compat layers |
| 10 | + on `main`. |
| 11 | +- `v1.x` is the release branch for the current stable line. Backport PRs target |
| 12 | + this branch and use a `[v1.x]` title prefix. |
| 13 | +- `README.md` is frozen at v1 (a pre-commit hook rejects edits). Edit |
| 14 | + `README.v2.md` instead. |
| 15 | + |
| 16 | +## Package Management |
| 17 | + |
| 18 | +- ONLY use uv, NEVER pip |
| 19 | +- Installation: `uv add <package>` |
| 20 | +- Running tools: `uv run --frozen <tool>`. Always pass `--frozen` so uv doesn't |
| 21 | + rewrite `uv.lock` as a side effect. |
| 22 | +- Cross-version testing: `uv run --frozen --python 3.10 pytest ...` to run |
| 23 | + against a specific interpreter (CI covers 3.10–3.14). |
| 24 | +- Upgrading: `uv lock --upgrade-package <package>` |
| 25 | +- FORBIDDEN: `uv pip install`, `@latest` syntax |
| 26 | +- Don't raise dependency floors for CVEs alone. The `>=` constraint already |
| 27 | + lets users upgrade. Only raise a floor when the SDK needs functionality from |
| 28 | + the newer version, and don't add SDK code to work around a dependency's |
| 29 | + vulnerability. See Kludex/uvicorn#2643 and python-sdk #1552 for reasoning. |
| 30 | + |
| 31 | +## Code Quality |
| 32 | + |
| 33 | +- Type hints required for all code |
| 34 | +- Public APIs must have docstrings |
| 35 | +- `src/mcp/__init__.py` defines the public API surface via `__all__`. Adding a |
| 36 | + symbol there is a deliberate API decision, not a convenience re-export. |
| 37 | +- IMPORTANT: All imports go at the top of the file — inline imports hide |
| 38 | + dependencies and obscure circular-import bugs. Only exception: when a |
| 39 | + top-level import genuinely can't work (lazy-loading optional deps, or |
| 40 | + tests that re-import a module). |
| 41 | + |
| 42 | +## Testing |
| 43 | + |
| 44 | +- Framework: `uv run --frozen pytest` |
| 45 | +- Async testing: use anyio, not asyncio |
| 46 | +- Do not use `Test` prefixed classes, use functions |
| 47 | +- IMPORTANT: Tests should be fast and deterministic. Prefer in-memory async execution; |
| 48 | + reach for threads only when necessary, and subprocesses only as a last resort. |
| 49 | +- For end-to-end behavior, an in-memory `Client(server)` is usually the |
| 50 | + cleanest approach (see `tests/client/test_client.py` for the canonical |
| 51 | + pattern). For narrower changes, testing the function directly is fine. Use |
| 52 | + judgment. |
| 53 | +- Test files mirror the source tree: `src/mcp/client/streamable_http.py` → |
| 54 | + `tests/client/test_streamable_http.py`. Add tests to the existing file for that module. |
| 55 | +- Avoid `anyio.sleep()` with a fixed duration to wait for async operations. Instead: |
| 56 | + - Use `anyio.Event` — set it in the callback/handler, `await event.wait()` in the test |
| 57 | + - For stream messages, use `await stream.receive()` instead of `sleep()` + `receive_nowait()` |
| 58 | + - Exception: `sleep()` is appropriate when testing time-based features (e.g., timeouts) |
| 59 | +- Wrap indefinite waits (`event.wait()`, `stream.receive()`) in `anyio.fail_after(5)` to prevent hangs |
| 60 | +- Pytest is configured with `filterwarnings = ["error"]`, so warnings fail |
| 61 | + tests. Don't silence them with `filterwarnings` or `warnings.catch_warnings()`; |
| 62 | + fix the underlying cause instead. |
| 63 | + |
| 64 | +### Coverage |
| 65 | + |
| 66 | +CI requires 100% (`fail_under = 100`, `branch = true`). |
| 67 | + |
| 68 | +- Full check: `./scripts/test` (~23s). Runs coverage + `strict-no-cover` on the |
| 69 | + default Python. Not identical to CI: CI also runs 3.10–3.14 × {ubuntu, windows}, |
| 70 | + and some branch-coverage quirks only surface on specific matrix entries. |
| 71 | +- Targeted check while iterating (~4s, deterministic): |
| 72 | + |
| 73 | + ```bash |
| 74 | + uv run --frozen coverage erase |
| 75 | + uv run --frozen coverage run -m pytest tests/path/test_foo.py |
| 76 | + uv run --frozen coverage combine |
| 77 | + uv run --frozen coverage report --include='src/mcp/path/foo.py' --fail-under=0 |
| 78 | + UV_FROZEN=1 uv run --frozen strict-no-cover |
| 79 | + ``` |
| 80 | + |
| 81 | + Partial runs can't hit 100% (coverage tracks `tests/` too), so `--fail-under=0` |
| 82 | + and `--include` scope the report. `strict-no-cover` has no false positives on |
| 83 | + partial runs — if your new test executes a line marked `# pragma: no cover`, |
| 84 | + even a single-file run catches it. |
| 85 | + |
| 86 | +Avoid adding new `# pragma: no cover`, `# type: ignore`, or `# noqa` comments. |
| 87 | +In tests, use `assert isinstance(x, T)` to narrow types instead of |
| 88 | +`# type: ignore`. In library code (`src/`), a `# pragma: no cover` needs very |
| 89 | +good reasoning — it usually means a test is missing. Audit before pushing: |
| 90 | + |
| 91 | +```bash |
| 92 | +git diff origin/main... | grep -E '^\+.*(pragma|type: ignore|noqa)' |
| 93 | +``` |
| 94 | + |
| 95 | +What the existing pragmas mean: |
| 96 | + |
| 97 | +- `# pragma: no cover` — line is never executed. CI's `strict-no-cover` fails if |
| 98 | + it IS executed. When your test starts covering such a line, remove the pragma. |
| 99 | +- `# pragma: lax no cover` — excluded from coverage but not checked by |
| 100 | + `strict-no-cover`. Use for lines covered on some platforms/versions but not |
| 101 | + others. |
| 102 | +- `# pragma: no branch` — excludes branch arcs only. coverage.py misreports the |
| 103 | + `->exit` arc for nested `async with` on Python 3.11+ (worse on 3.14/Windows). |
| 104 | + |
| 105 | +## Breaking Changes |
| 106 | + |
| 107 | +When making breaking changes, document them in `docs/migration.md`. Include: |
| 108 | + |
| 109 | +- What changed |
| 110 | +- Why it changed |
| 111 | +- How to migrate existing code |
| 112 | + |
| 113 | +Search for related sections in the migration guide and group related changes together |
| 114 | +rather than adding new standalone sections. |
| 115 | + |
| 116 | +## Formatting & Type Checking |
| 117 | + |
| 118 | +- Format: `uv run --frozen ruff format .` |
| 119 | +- Lint: `uv run --frozen ruff check . --fix` |
| 120 | +- Type check: `uv run --frozen pyright` |
| 121 | +- Pre-commit runs all of the above plus markdownlint, a `uv.lock` consistency |
| 122 | + check, and README checks — see `.pre-commit-config.yaml` |
| 123 | + |
| 124 | +## Exception Handling |
| 125 | + |
| 126 | +- **Always use `logger.exception()` instead of `logger.error()` when catching exceptions** |
| 127 | + - Don't include the exception in the message: `logger.exception("Failed")` not `logger.exception(f"Failed: {e}")` |
| 128 | +- **Catch specific exceptions** where possible: |
| 129 | + - File ops: `except (OSError, PermissionError):` |
| 130 | + - JSON: `except json.JSONDecodeError:` |
| 131 | + - Network: `except (ConnectionError, TimeoutError):` |
| 132 | +- **FORBIDDEN** `except Exception:` - unless in top-level handlers |
0 commit comments