Skip to content

Commit 039e61f

Browse files
committed
fix(v2): give every collection one pagination contract and close four envelope holes
A fractional `limit` reached Postgres as `LIMIT 2.5` and answered 500 on both `GET /workflows` and `GET /audit-logs`: each list re-declared the param inline, and these two copies lost their `.int()`. The same divergence left `limit` validated five different ways and five collections emitting `nextCursor` while accepting no `limit` at all, or accepting one and silently discarding it. Adds `v2PaginationFields()` in `contracts/v2/shared.ts` — a bounded integer `limit` and an opaque `cursor` — and adopts it across all 17 paged lists, so the family cannot drift again. `/files`, `/logs` and `/tables` keep the truncate-and- clamp leniency they published, now as an explicit named mode rather than three hand-rolled copies. Gives `/skills`, `/custom-tools`, `/secrets`, `/credentials` and `/knowledge` real pagination using the existing cursor codecs: a keyset for the four whose page comes from one ordered SQL read, and the offset cursor for `/skills`, whose merge of the static builtin registry into DB rows cannot be expressed as a SQL keyset. Each keyset sort now ends in a unique `id`; knowledge tie-broke on `createdAt`, which cannot separate rows sharing a millisecond. Two correctness fixes pagination forced: the secrets visibility filter moved from a post-query JS pass into SQL, because trimming rows after the page is cut returns fewer than `limit` while `nextCursor` claims more; and the skills list stopped selecting the 50k-char `content` column only to discard it. Also restores the canonical error envelope where it had holes: a malformed JSON body returned a bare `{"error":string}` because the envelope was a per-route opt-in only 8 of 77 routes remembered, and an unknown `/api/v2` path returned an HTML 404. Both are now defaults — `V2_PARSE_DEFAULTS` on the builders and the two raw routes, and a catch-all whose body is byte-identical to the rollout gate's so an unknown path stays indistinguishable from an ungated one. Consolidates the keyset paging block (`resumeKeyset`/`keysetPage` in `list-query.ts`) that had been open-coded in six modules, and folds the bespoke `InvalidWorkflowListCursorError` into the `OrchestrationError` every other list already used. Prevention: the contract sweep in `list-pagination.test.ts` now also asserts that every paged list rejects a fractional `limit`, that every list query is `.strict()`, and that the three clamping lists still truncate. The fractional- limit assertion is what caught `/audit-logs`. Documented in `.agents/skills/v2-api-conventions/SKILL.md`. 405 responses still carry no `Allow` header — Next.js generates those before any handler runs. Recorded as a known gap.
1 parent 7b3e6a6 commit 039e61f

84 files changed

Lines changed: 2220 additions & 544 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
---
2+
name: v2-api-conventions
3+
description: The response, error, pagination, and validation contract every `/api/v2` endpoint must satisfy. Use when adding or changing a route under `apps/sim/app/api/v2/`, or when auditing one for conformance.
4+
argument-hint: <route-path>
5+
---
6+
7+
# v2 API Conventions
8+
9+
The v2 surface makes one promise: **every response is the same two shapes, and a caller-supplied value can never produce a 500.**
10+
11+
```
12+
success (single) { "data": {...} }
13+
success (collection) { "data": [...], "nextCursor": "..." | null }
14+
failure (always) { "error": { "code": "...", "message": "...", "details"?: ... } }
15+
```
16+
17+
Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML.
18+
19+
That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local:
20+
21+
- `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`.
22+
- A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered.
23+
- `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page.
24+
- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`.
25+
26+
Each was one line. The rules below are the generalisations.
27+
28+
## Where the machinery lives
29+
30+
| Concern | File |
31+
|---|---|
32+
| Envelope + error codes + cursor codecs | `apps/sim/app/api/v2/lib/response.ts` |
33+
| Rollout gate (`v2-api` flag) | `apps/sim/app/api/v2/lib/gate.ts` |
34+
| Cross-tenant concealment | `apps/sim/lib/api/server/routes/resource-concealment.ts` |
35+
| Route builder | `apps/sim/lib/api/server/routes/v2-json-route.ts` |
36+
| Contracts | `apps/sim/lib/api/contracts/v2/**` |
37+
| Shared list/keyset helpers | `apps/sim/lib/api/list-query.ts` |
38+
39+
## Rule 1 — the envelope is produced by helpers, never by hand
40+
41+
`v2Data`, `v2CursorList`, and `v2Error` in `response.ts` are the only things that build a v2 body. They also set `Cache-Control: private, no-store`, which every v2 response needs because every v2 response is authed per-caller data.
42+
43+
A route built with `defineV2JsonRoute` gets this for free: its `present` returns the *body shape* and the builder renders it. Never call `NextResponse.json` from a v2 route.
44+
45+
**The envelope must hold for every failure mode, including the ones that happen before your handler runs.** That is what the four bugs above have in common. Defaults for the transport-level failures live on the builder — `v2PayloadTooLargeResponse` (413) and `v2InvalidJsonResponse` (400) — precisely so a route cannot forget them.
46+
47+
## Rule 2 — status codes mean specific things
48+
49+
| Status | `code` | Meaning |
50+
|---|---|---|
51+
| 200 / 201 || Success. 201 only for a created resource. |
52+
| 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. |
53+
| 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** |
54+
| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). |
55+
| 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. |
56+
| 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. |
57+
| 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. |
58+
| 429 | `RATE_LIMITED` | With `Retry-After` and `X-RateLimit-*`. |
59+
| 500 | `INTERNAL_ERROR` | Genuine server fault only. Message is always generic. |
60+
61+
Two of these carry real design weight:
62+
63+
**404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose.
64+
65+
**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks.
66+
67+
## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them
68+
69+
Every list returns `{ data, nextCursor }`. Whether it *pages* is a separate, pinned decision — see `lib/api/contracts/v2/__tests__/list-pagination.test.ts`, which enumerates both sets and fails when a new list is in neither.
70+
71+
Build the query slice from the shared helper, never by hand:
72+
73+
```ts
74+
...v2PaginationFields({ description: 'Maximum widgets to return per page.' })
75+
```
76+
77+
That gives `limit` (integer, 1..`V2_MAX_PAGE_SIZE`, defaulting to `V2_DEFAULT_PAGE_SIZE` = 50) and an opaque `cursor`. Re-declaring `limit: z.coerce.number()...` inline is how the 500 happened; there is one schema so the family cannot drift again.
78+
79+
Two cursor schemes exist, both opaque base64-JSON from `response.ts`. Which one you use is decided by what the read can express, not by taste:
80+
81+
- **Keyset** (`readSortedCursor` in, `encodeSortedCursor` out) — the default. Requires the page to come from one ordered SQL read. The sort is stamped into the cursor and re-checked on replay, so changing `sortBy` mid-pagination is a 400, not a silently skipped page.
82+
- **Offset** (`decodeOffsetCursor` / `encodeCursor({ offset })`) — only when a keyset is impossible. Two lists qualify: `GET /skills` merges a static in-code registry with DB rows and re-sorts in JS, and `GET /knowledge/{id}/documents` sits on a limit/offset query.
83+
84+
**A keyset's key list must end in a unique column (`id`).** A non-unique trailing key cannot separate tied rows, so the page boundary either repeats or drops them. `lib/api/list-keyset-paging.test.ts` demonstrates the failure.
85+
86+
Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side.
87+
88+
## Rule 4 — reject what you do not implement
89+
90+
Query and body schemas are **`.strict()`**. Zod strips unknown keys by default, so a non-strict schema answers `?limit=1` with 200 and the whole set — the caller believes it bounded the response and it did not. That is a contract lie, and on an uncapped list it is also an unbounded-response risk.
91+
92+
Error messages name the field and, where there is one, the escape hatch:
93+
94+
```
95+
limit must be a whole number
96+
limit cannot exceed 100
97+
search cannot be empty
98+
sortBy: expected one of "name" | "createdAt" | "updatedAt"
99+
Limit cannot exceed 1000; use limit=0 to stream all rows, or create an export
100+
```
101+
102+
That last one is the standard to aim for. A message that only says `Invalid input` fails this rule — the caller cannot act on it.
103+
104+
## Rule 5 — contract first, then use case, then route
105+
106+
Order matters because each layer is checked against the one before it.
107+
108+
1. **Contract** in `lib/api/contracts/v2/<domain>.ts` via `defineRouteContract`. Response schemas are `.parse`d on the way out, so a field the producer does not actually emit becomes a 500 on a successful read — assert only what you can prove.
109+
2. **Application use case** owns canonical loading, authorization, business behavior, and audit. The route's `present` receives **only the use-case result**, so anything the presenter needs (e.g. the active `sortBy`/`sortOrder` to stamp a cursor) must be returned by the use case.
110+
3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing.
111+
4. **OpenAPI description** in `lib/api/contracts/v2/openapi/<domain>.ts`, then `bun run generate:openapi`. A description that claims behaviour the route does not have is the same class of bug as a wrong schema.
112+
113+
## Checklist
114+
115+
Run this against any new or changed v2 endpoint.
116+
117+
- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`.
118+
- [ ] Route uses a shared builder; no hand-built `NextResponse.json`.
119+
- [ ] Query and body schemas are `.strict()`.
120+
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer.
121+
- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`.
122+
- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them.
123+
- [ ] Keyset sorts end in a unique `id` key.
124+
- [ ] The list is classified in `list-pagination.test.ts`.
125+
- [ ] Cross-tenant access answers 404, never 403.
126+
- [ ] 403s carry a machine-readable `details.code`.
127+
- [ ] Validation messages name the field and echo the valid set.
128+
- [ ] Response schema matches every field the route actually emits.
129+
- [ ] OpenAPI description regenerated and truthful about pagination.
130+
- [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass.
131+
132+
## Known gap
133+
134+
A 405 on a path that *does* have a route file but does not export that verb is generated by Next.js before any Sim code runs: zero-byte body, no `content-type`, and no `Allow` header, which RFC 9110 §15.5.6 requires. Fixing it means either exporting explicit rejecting handlers from all 77 v2 route files or intercepting in `apps/sim/proxy.ts` with a static path→methods table. Neither is done. Unknown *paths* are handled — the catch-all covers those.
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
---
2+
description: The response, error, pagination, and validation contract every `/api/v2` endpoint must satisfy. Use when adding or changing a route under `apps/sim/app/api/v2/`, or when auditing one for conformance.
3+
argument-hint: <route-path>
4+
---
5+
6+
# v2 API Conventions
7+
8+
The v2 surface makes one promise: **every response is the same two shapes, and a caller-supplied value can never produce a 500.**
9+
10+
```
11+
success (single) { "data": {...} }
12+
success (collection) { "data": [...], "nextCursor": "..." | null }
13+
failure (always) { "error": { "code": "...", "message": "...", "details"?: ... } }
14+
```
15+
16+
Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML.
17+
18+
That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local:
19+
20+
- `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`.
21+
- A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered.
22+
- `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page.
23+
- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`.
24+
25+
Each was one line. The rules below are the generalisations.
26+
27+
## Where the machinery lives
28+
29+
| Concern | File |
30+
|---|---|
31+
| Envelope + error codes + cursor codecs | `apps/sim/app/api/v2/lib/response.ts` |
32+
| Rollout gate (`v2-api` flag) | `apps/sim/app/api/v2/lib/gate.ts` |
33+
| Cross-tenant concealment | `apps/sim/lib/api/server/routes/resource-concealment.ts` |
34+
| Route builder | `apps/sim/lib/api/server/routes/v2-json-route.ts` |
35+
| Contracts | `apps/sim/lib/api/contracts/v2/**` |
36+
| Shared list/keyset helpers | `apps/sim/lib/api/list-query.ts` |
37+
38+
## Rule 1 — the envelope is produced by helpers, never by hand
39+
40+
`v2Data`, `v2CursorList`, and `v2Error` in `response.ts` are the only things that build a v2 body. They also set `Cache-Control: private, no-store`, which every v2 response needs because every v2 response is authed per-caller data.
41+
42+
A route built with `defineV2JsonRoute` gets this for free: its `present` returns the *body shape* and the builder renders it. Never call `NextResponse.json` from a v2 route.
43+
44+
**The envelope must hold for every failure mode, including the ones that happen before your handler runs.** That is what the four bugs above have in common. Defaults for the transport-level failures live on the builder — `v2PayloadTooLargeResponse` (413) and `v2InvalidJsonResponse` (400) — precisely so a route cannot forget them.
45+
46+
## Rule 2 — status codes mean specific things
47+
48+
| Status | `code` | Meaning |
49+
|---|---|---|
50+
| 200 / 201 || Success. 201 only for a created resource. |
51+
| 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. |
52+
| 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** |
53+
| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). |
54+
| 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. |
55+
| 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. |
56+
| 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. |
57+
| 429 | `RATE_LIMITED` | With `Retry-After` and `X-RateLimit-*`. |
58+
| 500 | `INTERNAL_ERROR` | Genuine server fault only. Message is always generic. |
59+
60+
Two of these carry real design weight:
61+
62+
**404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose.
63+
64+
**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks.
65+
66+
## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them
67+
68+
Every list returns `{ data, nextCursor }`. Whether it *pages* is a separate, pinned decision — see `lib/api/contracts/v2/__tests__/list-pagination.test.ts`, which enumerates both sets and fails when a new list is in neither.
69+
70+
Build the query slice from the shared helper, never by hand:
71+
72+
```ts
73+
...v2PaginationFields({ description: 'Maximum widgets to return per page.' })
74+
```
75+
76+
That gives `limit` (integer, 1..`V2_MAX_PAGE_SIZE`, defaulting to `V2_DEFAULT_PAGE_SIZE` = 50) and an opaque `cursor`. Re-declaring `limit: z.coerce.number()...` inline is how the 500 happened; there is one schema so the family cannot drift again.
77+
78+
Two cursor schemes exist, both opaque base64-JSON from `response.ts`. Which one you use is decided by what the read can express, not by taste:
79+
80+
- **Keyset** (`readSortedCursor` in, `encodeSortedCursor` out) — the default. Requires the page to come from one ordered SQL read. The sort is stamped into the cursor and re-checked on replay, so changing `sortBy` mid-pagination is a 400, not a silently skipped page.
81+
- **Offset** (`decodeOffsetCursor` / `encodeCursor({ offset })`) — only when a keyset is impossible. Two lists qualify: `GET /skills` merges a static in-code registry with DB rows and re-sorts in JS, and `GET /knowledge/{id}/documents` sits on a limit/offset query.
82+
83+
**A keyset's key list must end in a unique column (`id`).** A non-unique trailing key cannot separate tied rows, so the page boundary either repeats or drops them. `lib/api/list-keyset-paging.test.ts` demonstrates the failure.
84+
85+
Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side.
86+
87+
## Rule 4 — reject what you do not implement
88+
89+
Query and body schemas are **`.strict()`**. Zod strips unknown keys by default, so a non-strict schema answers `?limit=1` with 200 and the whole set — the caller believes it bounded the response and it did not. That is a contract lie, and on an uncapped list it is also an unbounded-response risk.
90+
91+
Error messages name the field and, where there is one, the escape hatch:
92+
93+
```
94+
limit must be a whole number
95+
limit cannot exceed 100
96+
search cannot be empty
97+
sortBy: expected one of "name" | "createdAt" | "updatedAt"
98+
Limit cannot exceed 1000; use limit=0 to stream all rows, or create an export
99+
```
100+
101+
That last one is the standard to aim for. A message that only says `Invalid input` fails this rule — the caller cannot act on it.
102+
103+
## Rule 5 — contract first, then use case, then route
104+
105+
Order matters because each layer is checked against the one before it.
106+
107+
1. **Contract** in `lib/api/contracts/v2/<domain>.ts` via `defineRouteContract`. Response schemas are `.parse`d on the way out, so a field the producer does not actually emit becomes a 500 on a successful read — assert only what you can prove.
108+
2. **Application use case** owns canonical loading, authorization, business behavior, and audit. The route's `present` receives **only the use-case result**, so anything the presenter needs (e.g. the active `sortBy`/`sortOrder` to stamp a cursor) must be returned by the use case.
109+
3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing.
110+
4. **OpenAPI description** in `lib/api/contracts/v2/openapi/<domain>.ts`, then `bun run generate:openapi`. A description that claims behaviour the route does not have is the same class of bug as a wrong schema.
111+
112+
## Checklist
113+
114+
Run this against any new or changed v2 endpoint.
115+
116+
- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`.
117+
- [ ] Route uses a shared builder; no hand-built `NextResponse.json`.
118+
- [ ] Query and body schemas are `.strict()`.
119+
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer.
120+
- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`.
121+
- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them.
122+
- [ ] Keyset sorts end in a unique `id` key.
123+
- [ ] The list is classified in `list-pagination.test.ts`.
124+
- [ ] Cross-tenant access answers 404, never 403.
125+
- [ ] 403s carry a machine-readable `details.code`.
126+
- [ ] Validation messages name the field and echo the valid set.
127+
- [ ] Response schema matches every field the route actually emits.
128+
- [ ] OpenAPI description regenerated and truthful about pagination.
129+
- [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass.
130+
131+
## Known gap
132+
133+
A 405 on a path that *does* have a route file but does not export that verb is generated by Next.js before any Sim code runs: zero-byte body, no `content-type`, and no `Allow` header, which RFC 9110 §15.5.6 requires. Fixing it means either exporting explicit rejecting handlers from all 77 v2 route files or intercepting in `apps/sim/proxy.ts` with a static path→methods table. Neither is done. Unknown *paths* are handled — the catch-all covers those.

0 commit comments

Comments
 (0)