api-surface-review

# API Surface Review (Polar) The handle is the **diff**. The evidence is **what this change does to the API contract**, and whether any of it is breaking. This is where Polar's most expensive review mistakes live. A schema change does not stay in the repo: it flows into `docs/openapi.json`, the generated TypeScript client, the Speakeasy SDKs under `sdk/`, and out to merchants who already wrote code against it. ## Scope Diffs touching `**/schemas.py`, `**/endpoints.py`, `polar/openapi.py`, `docs/openapi.json`, `sdk/`, `clients/packages/client/`. **CI already does part of this job.** The `OpenAPI Diff` workflow posts the schema delta as a PR comment, and `OpenAPI Client Regeneration Check` fails if the client is stale. Do not recompute either by hand. Read the diff comment if it exists and judge whether the delta is acceptable; that judgement is the thing CI cannot make. Owned elsewhere: `ADR-0002` (status-coded `PolarError`), `ADR-0007` (no default in output schemas) → `adr-check`. Endpoint conventions written in `server/AGENTS.md` — `response_model` and ORM returns, POST/PATCH/DELETE status codes, `ListResource`, `responses=`, the `PolarRequestValidationError` boundary → `conventions-check`. Existing shared types → `reuse-check`. ## Checks ### 1. Public or private `APITag` has two values, `public` and `private` (`polar/openapi.py`). Public means documented and in the SDKs. - Set it deliberately on every new router and route. Never hide something with `include_in_schema=False`; use `tags=[APITag.private]`. - Tag the route, not the router, when only one route should change. The pattern: keep `APITag.public` on what stays, add `APITag.private` to the deprecated one. - Private endpoints (payouts are the standing example) are not in the SDK. Do not reason about them as public API. - **On every new field, ask: does a merchant need to see this?** Internal `Organization` settings keep leaking into the public API this way. ### 2. Schema shape - Output fields are required, nullable where needed. A `default` makes them optional in OpenAPI, which generates a wrongly-optional TS field. (ADR-0007 — name it, do not restate it.) - Type fields with their enum, not `str`. - Reuse a related resource's base schema rather than redefining its fields: `class DisputeCustomer(CustomerBase): ...`. - A value that already exists as a model property is a plain field, not a reimplemented `computed_field`. - Descriptions are user-facing: no implementation detail, and write `None` not `null`. - Any field writing to a SQL `INT` column is `Int32`, so out-of-range input is rejected at validation instead of overflowing. - Constrain enums by listing allowed values (`reason: Literal[RefundReason.foo, ...]`) rather than a hand-rolled validator. Pydantic validates, OpenAPI documents. - Discriminated unions get a plain `Discriminator` plus `SetSchemaReference`, so the union lands in OpenAPI properly. - Do not over-constrain fields fed by a payment processor. *"We'll never know what Stripe or other payment processor will send us."* ### 3. Naming The union takes the clean name, the variants get qualified: `CustomerCreate` is the union, `CustomerIndividualCreate` and `CustomerTeamCreate` are members. Same for `CustomerState`. Fields are `snake_case`. Prefer explicit over clever (`payment_method_type`, not `payment_method`). Do not overload an existing concept with a new meaning. ### 4. Breaking changes Breaking = removing a field, flipping required/optional, changing a type, renaming anything, or changing a status code a client branches on. Before removing a field or behaviour: 1. **Check Logfire for real usage.** This is what François actually does. Merchants create things programmatically that nobody expects. 2. If it is used, keep a compatibility layer and use `SkipJsonSchema` so runtime still accepts the value while it disappears from future SDKs. 3. If it is genuinely breaking, say so plainly. That is a human decision, not something to wave through. ### 5. Route shape Only the parts `AGENTS.md` does not already cover: - Trailing slash on root endpoints. - Errors on the same route need distinct status codes, or their schemas collide in the OpenAPI output. - A query parameter must act as a pure filter. If it changes the response shape, the resource is wrong and it wants its own route. - Removing an endpoint and updating its frontend caller in one PR is a deploy hazard → `ship-safety` owns the split. ## Output ``` ## API Surface ### 🔴 Blocking - `file:line` — <what breaks, and for whom>. Fix: <fix> ### 🟠 Should fix - `file:line` — <claim>. Fix: <fix> ### 🟡 Question - `file:line` — <question> ### Notes - public surface delta: <one line, or "see the OpenAPI Diff PR comment"> ### Verdict ✅ Clean | ❌ n blocking, n should-fix ``` Anything blocking needs a human decision before merge.