|
| 1 | +# typescript-sdk Review Conventions |
| 2 | + |
| 3 | +Guidance for reviewing pull requests on this repository. The first three sections are |
| 4 | +stable principles; the **Recurring Catches** section is auto-maintained from past human |
| 5 | +review rounds and grows over time. |
| 6 | + |
| 7 | +## Guiding Principles |
| 8 | + |
| 9 | +1. **Minimalism** — The SDK should do less, not more. Protocol correctness, transport |
| 10 | + lifecycle, types, and clean handler context belong in the SDK. Middleware engines, |
| 11 | + registry managers, builder patterns, and content helpers belong in userland. |
| 12 | +2. **Burden of proof is on addition** — The default answer to "should we add this?" is |
| 13 | + no. Removing something from the public API is far harder than not adding it. |
| 14 | +3. **Justify with concrete evidence** — Every new abstraction needs a concrete consumer |
| 15 | + today. Ask for real issues, benchmarks, real-world examples; apply the same standard |
| 16 | + to your own review (link spec sections, link code, show the simpler alternative). |
| 17 | +4. **Spec is the anchor** — The SDK implements the protocol spec. The further a feature |
| 18 | + drifts from the spec, the stronger the justification needs to be. |
| 19 | +5. **Kill at the highest level** — If the design is wrong, don't review the |
| 20 | + implementation. Lead with the highest-level concern; specific bugs are supporting |
| 21 | + detail. |
| 22 | +6. **Decompose by default** — A PR doing multiple things should be multiple PRs unless |
| 23 | + there's a strong reason to bundle. |
| 24 | + |
| 25 | +## Review Ordering |
| 26 | + |
| 27 | +1. **Design justification** — Is the overall approach sound? Is the complexity warranted? |
| 28 | +2. **Structural concerns** — Is the architecture right? Are abstractions justified? |
| 29 | +3. **Correctness** — Bugs, regressions, missing functionality. |
| 30 | +4. **Style and naming** — Nits, conventions, documentation. |
| 31 | + |
| 32 | +## Checklist |
| 33 | + |
| 34 | +**Protocol & spec** |
| 35 | +- Types match [`schema.ts`](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/draft/schema.ts) exactly (optional vs required fields) |
| 36 | +- Correct `ProtocolError` codes (enum `ProtocolErrorCode`); HTTP status codes match spec (e.g., 404 vs 410) |
| 37 | +- Works for both stdio and Streamable HTTP transports — no transport-specific assumptions |
| 38 | +- Cross-SDK consistency: check what `python-sdk` does for the same feature |
| 39 | + |
| 40 | +**API surface** |
| 41 | +- Every new export is intentional (see CLAUDE.md § Public API Exports); helpers users can write themselves belong in a cookbook, not the SDK |
| 42 | +- New abstractions have at least one concrete callsite in the PR |
| 43 | +- One way to do things — improving an existing API beats adding a parallel one |
| 44 | + |
| 45 | +**Correctness** |
| 46 | +- Async: race conditions, cleanup on cancellation, unhandled rejections, missing `await` |
| 47 | +- Error propagation: caught/rethrown properly, resources cleaned up on error paths |
| 48 | +- Type safety: no unjustified `any`, no unsafe `as` assertions |
| 49 | +- Backwards compat: public-interface changes, default changes, removed exports — flagged and justified |
| 50 | + |
| 51 | +**Tests & docs** |
| 52 | +- New behavior has vitest coverage including error paths |
| 53 | +- Breaking changes documented in `docs/migration.md` and `docs/migration-SKILL.md` |
| 54 | +- Bugfix or behavior change: check whether `docs/**/*.md` describes the old behavior and needs updating; flag prose that now contradicts the implementation |
| 55 | +- New feature: verify prose documentation is added (not just JSDoc), and assess whether `examples/` needs a new or updated example |
| 56 | +- Behavior change: assess whether existing `examples/` still compile and demonstrate the current API |
| 57 | + |
| 58 | +## Reference |
| 59 | + |
| 60 | +When verifying spec compliance, consult the spec directly rather than relying on memory: |
| 61 | + |
| 62 | +- MCP documentation server: `https://modelcontextprotocol.io/mcp` |
| 63 | +- Full spec text (single file, LLM-friendly): `https://modelcontextprotocol.io/llms-full.txt` — fetch to a temp file and grep for the relevant section |
| 64 | +- Schema source of truth: [`schema.ts`](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/draft/schema.ts) |
| 65 | + |
| 66 | +## Recurring Catches |
| 67 | + |
| 68 | +### HTTP Transport |
| 69 | + |
| 70 | +- When validating `Mcp-Session-Id`, return **400** for a missing header and **404** for an unknown/expired session — never conflate `!sessionId || !transports[sessionId]` into one status, because the client needs to distinguish "fix your request" from "start a new session". Flag any diff that branches on session-id presence/lookup with a single 4xx. (#1707, #1770) |
| 71 | + |
| 72 | +### Error Handling |
| 73 | + |
| 74 | +- Broad `catch` blocks must not emit client-fault JSON-RPC codes (`-32700` ParseError, `-32602` InvalidParams) for server-internal failures like stream setup, task-store misses, or polling errors — map those to `-32603` InternalError so clients don't retry/reformat pointlessly. Flag any catch-all that hard-codes ParseError/InvalidParams without discriminating the thrown cause. (#1752, #1769) |
| 75 | + |
| 76 | +### Schema Compliance |
| 77 | + |
| 78 | +- When editing Zod protocol schemas in `schemas.ts`, verify unknown-key handling matches the spec `schema.ts`: if the spec type has no `additionalProperties: false`, the SDK schema must use `z.looseObject()` / `.catchall(z.unknown())` rather than implicit strict — over-strict Zod (incl. `z.literal('object')` on `type`) rejects spec-valid payloads from other SDKs. Also confirm `spec.types.test.ts` still passes bidirectionally. (#1768, #1849, #1169) |
| 79 | + |
| 80 | +### Async / Lifecycle |
| 81 | + |
| 82 | +- In `close()` / shutdown paths, wrap user-supplied or chained callbacks (`onclose?.()`, cancel fns) in `try/finally` so a throw can't skip the remaining teardown (`abort()`, `_onclose()`, map clears) — otherwise the transport is left half-open. (#1735, #1763) |
| 83 | +- Deferred callbacks (`setTimeout`, `.finally()`, reconnect closures) must check closed/aborted state before mutating `this._*` or starting I/O — a callback scheduled pre-close can fire after close/reconnect and corrupt the new connection's state (e.g., delete the new request's `AbortController`). (#1735, #1763) |
| 84 | + |
| 85 | +### Completeness |
| 86 | + |
| 87 | +- When a PR replaces a pattern (error class, auth-flow step, catch shape), grep the package for surviving instances of the old form — partial migrations leave sibling code paths with the very bug the PR claims to fix. Flag every leftover site. (#1657, #1761, #1595) |
| 88 | + |
| 89 | +### Documentation & Changesets |
| 90 | + |
| 91 | +- Read added `.changeset/*.md` text and new inline comments against the implementation in the same diff — prose that promises behavior the code no longer ships misleads consumers and contradicts stated intent. Flag any claim the diff doesn't back. (#1718, #1838) |
| 92 | + |
| 93 | +### CI & GitHub Actions |
| 94 | + |
| 95 | +- Do **not** assert that a third-party GitHub Action or publish toolchain will fail or needs extra permissions/tokens without verifying its docs or source — `pnpm publish` delegates to the system npm CLI (so npm OIDC works), and `changesets/action` in publish mode has no PR-comment step requiring `pull-requests: write`. For diffs under `.github/workflows/`, confirm claimed behavior in the action's README/source before flagging. (#1838, #1836) |
0 commit comments