From 039e61f19655c70177a36d19871b0144d254b7ba Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 12 Aug 2026 04:11:02 -0700 Subject: [PATCH 1/2] fix(v2): give every collection one pagination contract and close four envelope holes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .agents/skills/v2-api-conventions/SKILL.md | 134 ++++++++++++++ .claude/commands/v2-api-conventions.md | 133 ++++++++++++++ .cursor/commands/v2-api-conventions.md | 128 +++++++++++++ apps/docs/openapi-v2-billing.json | 4 +- apps/docs/openapi-v2-files-audit.json | 19 +- apps/docs/openapi-v2-knowledge.json | 26 ++- apps/docs/openapi-v2-logs.json | 15 +- apps/docs/openapi-v2-resources.json | 112 +++++++++++- apps/docs/openapi-v2-tables.json | 8 +- apps/docs/openapi-v2-workflows.json | 32 ++-- apps/sim/app/api/credentials/route.ts | 2 +- .../app/api/v2/[[...segments]]/route.test.ts | 50 +++++ apps/sim/app/api/v2/[[...segments]]/route.ts | 40 ++++ apps/sim/app/api/v2/credentials/route.test.ts | 11 +- apps/sim/app/api/v2/credentials/route.ts | 12 +- .../sim/app/api/v2/custom-tools/route.test.ts | 4 +- apps/sim/app/api/v2/custom-tools/route.ts | 13 +- .../api/v2/files/[fileId]/content/route.ts | 2 - apps/sim/app/api/v2/files/route.ts | 37 ++-- apps/sim/app/api/v2/knowledge/[id]/route.ts | 4 - .../sim/app/api/v2/knowledge/folders/route.ts | 7 - apps/sim/app/api/v2/knowledge/route.test.ts | 6 + apps/sim/app/api/v2/knowledge/route.ts | 13 +- apps/sim/app/api/v2/knowledge/search/route.ts | 2 - apps/sim/app/api/v2/lib/response.ts | 22 ++- apps/sim/app/api/v2/secrets/route.test.ts | 12 +- apps/sim/app/api/v2/secrets/route.ts | 12 +- apps/sim/app/api/v2/skills/route.test.ts | 38 +++- apps/sim/app/api/v2/skills/route.ts | 8 +- apps/sim/app/api/v2/skills/utils.ts | 8 +- apps/sim/app/api/v2/tables/route.ts | 29 +-- .../v2/workflows/[id]/deploy/route.test.ts | 16 +- .../app/api/v2/workflows/[id]/deploy/route.ts | 2 - .../api/v2/workflows/[id]/execute/route.ts | 2 + .../v2/workflows/[id]/rollback/route.test.ts | 16 +- .../api/v2/workflows/[id]/rollback/route.ts | 2 - .../[id]/runs/[runId]/resume/route.test.ts | 1 + .../[id]/runs/[runId]/resume/route.ts | 2 + apps/sim/app/api/v2/workflows/route.ts | 31 ++-- .../v2/__tests__/list-pagination.test.ts | 131 ++++++++++++- .../v2/__tests__/pagination-limit.test.ts | 112 ++++++++++++ apps/sim/lib/api/contracts/v2/audit-logs.ts | 13 +- apps/sim/lib/api/contracts/v2/billing.ts | 21 +-- apps/sim/lib/api/contracts/v2/credentials.ts | 36 ++-- apps/sim/lib/api/contracts/v2/custom-tools.ts | 18 +- apps/sim/lib/api/contracts/v2/files.ts | 14 +- apps/sim/lib/api/contracts/v2/knowledge.ts | 14 +- apps/sim/lib/api/contracts/v2/logs.ts | 14 +- apps/sim/lib/api/contracts/v2/mcp-servers.ts | 10 +- .../lib/api/contracts/v2/openapi/knowledge.ts | 2 +- .../lib/api/contracts/v2/openapi/resources.ts | 8 +- apps/sim/lib/api/contracts/v2/secrets.ts | 21 ++- apps/sim/lib/api/contracts/v2/shared.ts | 135 +++++++++++++- apps/sim/lib/api/contracts/v2/skills.ts | 20 +- apps/sim/lib/api/contracts/v2/tables.ts | 35 ++-- apps/sim/lib/api/contracts/v2/workflows.ts | 41 +---- apps/sim/lib/api/contracts/v2/workspaces.ts | 11 +- apps/sim/lib/api/list-convention.test.ts | 2 +- apps/sim/lib/api/list-keyset-paging.test.ts | 173 ++++++++++++++++++ apps/sim/lib/api/list-query.ts | 51 ++++++ apps/sim/lib/api/server/routes/index.ts | 1 + .../lib/api/server/routes/v2-binary-route.ts | 4 +- .../server/routes/v2-body-lifecycle-route.ts | 4 +- .../server/routes/v2-error-envelope.test.ts | 38 ++++ .../lib/api/server/routes/v2-json-route.ts | 29 ++- .../application/list-workspace-credentials.ts | 58 +++--- apps/sim/lib/credentials/queries.test.ts | 11 +- apps/sim/lib/credentials/queries.ts | 139 +++++++++++--- .../lib/custom-tools/application/use-cases.ts | 13 +- .../folders/application-folder-caps.test.ts | 1 - .../application/knowledge-bases.test.ts | 4 +- .../knowledge/application/knowledge-bases.ts | 42 +++-- .../knowledge/application/knowledge-vfs.ts | 2 +- apps/sim/lib/knowledge/service.ts | 117 ++++++++---- .../lib/secrets/application/use-cases.test.ts | 2 +- apps/sim/lib/secrets/application/use-cases.ts | 49 ++++- apps/sim/lib/skills/application/use-cases.ts | 24 ++- .../workflows/application/list-workflows.ts | 29 +-- .../application/workflow-crud.test.ts | 1 - .../lib/workflows/custom-tools/operations.ts | 66 +++++-- apps/sim/lib/workflows/queries.ts | 24 +-- .../lib/workflows/skills/operations.test.ts | 78 +++++++- apps/sim/lib/workflows/skills/operations.ts | 122 ++++++++++-- scripts/check-api-validation-contracts.ts | 9 +- 84 files changed, 2220 insertions(+), 544 deletions(-) create mode 100644 .agents/skills/v2-api-conventions/SKILL.md create mode 100644 .claude/commands/v2-api-conventions.md create mode 100644 .cursor/commands/v2-api-conventions.md create mode 100644 apps/sim/app/api/v2/[[...segments]]/route.test.ts create mode 100644 apps/sim/app/api/v2/[[...segments]]/route.ts create mode 100644 apps/sim/lib/api/contracts/v2/__tests__/pagination-limit.test.ts create mode 100644 apps/sim/lib/api/list-keyset-paging.test.ts create mode 100644 apps/sim/lib/api/server/routes/v2-error-envelope.test.ts diff --git a/.agents/skills/v2-api-conventions/SKILL.md b/.agents/skills/v2-api-conventions/SKILL.md new file mode 100644 index 00000000000..a8e5be1d6bb --- /dev/null +++ b/.agents/skills/v2-api-conventions/SKILL.md @@ -0,0 +1,134 @@ +--- +name: v2-api-conventions +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. +argument-hint: +--- + +# v2 API Conventions + +The v2 surface makes one promise: **every response is the same two shapes, and a caller-supplied value can never produce a 500.** + +``` +success (single) { "data": {...} } +success (collection) { "data": [...], "nextCursor": "..." | null } +failure (always) { "error": { "code": "...", "message": "...", "details"?: ... } } +``` + +Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML. + +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: + +- `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`. +- 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. +- `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. +- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`. + +Each was one line. The rules below are the generalisations. + +## Where the machinery lives + +| Concern | File | +|---|---| +| Envelope + error codes + cursor codecs | `apps/sim/app/api/v2/lib/response.ts` | +| Rollout gate (`v2-api` flag) | `apps/sim/app/api/v2/lib/gate.ts` | +| Cross-tenant concealment | `apps/sim/lib/api/server/routes/resource-concealment.ts` | +| Route builder | `apps/sim/lib/api/server/routes/v2-json-route.ts` | +| Contracts | `apps/sim/lib/api/contracts/v2/**` | +| Shared list/keyset helpers | `apps/sim/lib/api/list-query.ts` | + +## Rule 1 — the envelope is produced by helpers, never by hand + +`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. + +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. + +**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. + +## Rule 2 — status codes mean specific things + +| Status | `code` | Meaning | +|---|---|---| +| 200 / 201 | — | Success. 201 only for a created resource. | +| 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. | +| 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** | +| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). | +| 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. | +| 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. | +| 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. | +| 429 | `RATE_LIMITED` | With `Retry-After` and `X-RateLimit-*`. | +| 500 | `INTERNAL_ERROR` | Genuine server fault only. Message is always generic. | + +Two of these carry real design weight: + +**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. + +**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. + +## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them + +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. + +Build the query slice from the shared helper, never by hand: + +```ts +...v2PaginationFields({ description: 'Maximum widgets to return per page.' }) +``` + +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. + +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: + +- **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. +- **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. + +**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. + +Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side. + +## Rule 4 — reject what you do not implement + +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. + +Error messages name the field and, where there is one, the escape hatch: + +``` +limit must be a whole number +limit cannot exceed 100 +search cannot be empty +sortBy: expected one of "name" | "createdAt" | "updatedAt" +Limit cannot exceed 1000; use limit=0 to stream all rows, or create an export +``` + +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. + +## Rule 5 — contract first, then use case, then route + +Order matters because each layer is checked against the one before it. + +1. **Contract** in `lib/api/contracts/v2/.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. +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. +3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing. +4. **OpenAPI description** in `lib/api/contracts/v2/openapi/.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. + +## Checklist + +Run this against any new or changed v2 endpoint. + +- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`. +- [ ] Route uses a shared builder; no hand-built `NextResponse.json`. +- [ ] Query and body schemas are `.strict()`. +- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer. +- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`. +- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. +- [ ] Keyset sorts end in a unique `id` key. +- [ ] The list is classified in `list-pagination.test.ts`. +- [ ] Cross-tenant access answers 404, never 403. +- [ ] 403s carry a machine-readable `details.code`. +- [ ] Validation messages name the field and echo the valid set. +- [ ] Response schema matches every field the route actually emits. +- [ ] OpenAPI description regenerated and truthful about pagination. +- [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass. + +## Known gap + +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. diff --git a/.claude/commands/v2-api-conventions.md b/.claude/commands/v2-api-conventions.md new file mode 100644 index 00000000000..f902d2c0424 --- /dev/null +++ b/.claude/commands/v2-api-conventions.md @@ -0,0 +1,133 @@ +--- +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. +argument-hint: +--- + +# v2 API Conventions + +The v2 surface makes one promise: **every response is the same two shapes, and a caller-supplied value can never produce a 500.** + +``` +success (single) { "data": {...} } +success (collection) { "data": [...], "nextCursor": "..." | null } +failure (always) { "error": { "code": "...", "message": "...", "details"?: ... } } +``` + +Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML. + +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: + +- `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`. +- 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. +- `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. +- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`. + +Each was one line. The rules below are the generalisations. + +## Where the machinery lives + +| Concern | File | +|---|---| +| Envelope + error codes + cursor codecs | `apps/sim/app/api/v2/lib/response.ts` | +| Rollout gate (`v2-api` flag) | `apps/sim/app/api/v2/lib/gate.ts` | +| Cross-tenant concealment | `apps/sim/lib/api/server/routes/resource-concealment.ts` | +| Route builder | `apps/sim/lib/api/server/routes/v2-json-route.ts` | +| Contracts | `apps/sim/lib/api/contracts/v2/**` | +| Shared list/keyset helpers | `apps/sim/lib/api/list-query.ts` | + +## Rule 1 — the envelope is produced by helpers, never by hand + +`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. + +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. + +**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. + +## Rule 2 — status codes mean specific things + +| Status | `code` | Meaning | +|---|---|---| +| 200 / 201 | — | Success. 201 only for a created resource. | +| 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. | +| 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** | +| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). | +| 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. | +| 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. | +| 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. | +| 429 | `RATE_LIMITED` | With `Retry-After` and `X-RateLimit-*`. | +| 500 | `INTERNAL_ERROR` | Genuine server fault only. Message is always generic. | + +Two of these carry real design weight: + +**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. + +**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. + +## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them + +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. + +Build the query slice from the shared helper, never by hand: + +```ts +...v2PaginationFields({ description: 'Maximum widgets to return per page.' }) +``` + +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. + +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: + +- **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. +- **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. + +**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. + +Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side. + +## Rule 4 — reject what you do not implement + +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. + +Error messages name the field and, where there is one, the escape hatch: + +``` +limit must be a whole number +limit cannot exceed 100 +search cannot be empty +sortBy: expected one of "name" | "createdAt" | "updatedAt" +Limit cannot exceed 1000; use limit=0 to stream all rows, or create an export +``` + +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. + +## Rule 5 — contract first, then use case, then route + +Order matters because each layer is checked against the one before it. + +1. **Contract** in `lib/api/contracts/v2/.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. +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. +3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing. +4. **OpenAPI description** in `lib/api/contracts/v2/openapi/.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. + +## Checklist + +Run this against any new or changed v2 endpoint. + +- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`. +- [ ] Route uses a shared builder; no hand-built `NextResponse.json`. +- [ ] Query and body schemas are `.strict()`. +- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer. +- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`. +- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. +- [ ] Keyset sorts end in a unique `id` key. +- [ ] The list is classified in `list-pagination.test.ts`. +- [ ] Cross-tenant access answers 404, never 403. +- [ ] 403s carry a machine-readable `details.code`. +- [ ] Validation messages name the field and echo the valid set. +- [ ] Response schema matches every field the route actually emits. +- [ ] OpenAPI description regenerated and truthful about pagination. +- [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass. + +## Known gap + +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. diff --git a/.cursor/commands/v2-api-conventions.md b/.cursor/commands/v2-api-conventions.md new file mode 100644 index 00000000000..a61cd94a7a9 --- /dev/null +++ b/.cursor/commands/v2-api-conventions.md @@ -0,0 +1,128 @@ +# v2 API Conventions + +The v2 surface makes one promise: **every response is the same two shapes, and a caller-supplied value can never produce a 500.** + +``` +success (single) { "data": {...} } +success (collection) { "data": [...], "nextCursor": "..." | null } +failure (always) { "error": { "code": "...", "message": "...", "details"?: ... } } +``` + +Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML. + +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: + +- `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`. +- 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. +- `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. +- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`. + +Each was one line. The rules below are the generalisations. + +## Where the machinery lives + +| Concern | File | +|---|---| +| Envelope + error codes + cursor codecs | `apps/sim/app/api/v2/lib/response.ts` | +| Rollout gate (`v2-api` flag) | `apps/sim/app/api/v2/lib/gate.ts` | +| Cross-tenant concealment | `apps/sim/lib/api/server/routes/resource-concealment.ts` | +| Route builder | `apps/sim/lib/api/server/routes/v2-json-route.ts` | +| Contracts | `apps/sim/lib/api/contracts/v2/**` | +| Shared list/keyset helpers | `apps/sim/lib/api/list-query.ts` | + +## Rule 1 — the envelope is produced by helpers, never by hand + +`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. + +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. + +**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. + +## Rule 2 — status codes mean specific things + +| Status | `code` | Meaning | +|---|---|---| +| 200 / 201 | — | Success. 201 only for a created resource. | +| 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. | +| 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** | +| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). | +| 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. | +| 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. | +| 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. | +| 429 | `RATE_LIMITED` | With `Retry-After` and `X-RateLimit-*`. | +| 500 | `INTERNAL_ERROR` | Genuine server fault only. Message is always generic. | + +Two of these carry real design weight: + +**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. + +**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. + +## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them + +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. + +Build the query slice from the shared helper, never by hand: + +```ts +...v2PaginationFields({ description: 'Maximum widgets to return per page.' }) +``` + +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. + +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: + +- **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. +- **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. + +**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. + +Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side. + +## Rule 4 — reject what you do not implement + +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. + +Error messages name the field and, where there is one, the escape hatch: + +``` +limit must be a whole number +limit cannot exceed 100 +search cannot be empty +sortBy: expected one of "name" | "createdAt" | "updatedAt" +Limit cannot exceed 1000; use limit=0 to stream all rows, or create an export +``` + +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. + +## Rule 5 — contract first, then use case, then route + +Order matters because each layer is checked against the one before it. + +1. **Contract** in `lib/api/contracts/v2/.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. +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. +3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing. +4. **OpenAPI description** in `lib/api/contracts/v2/openapi/.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. + +## Checklist + +Run this against any new or changed v2 endpoint. + +- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`. +- [ ] Route uses a shared builder; no hand-built `NextResponse.json`. +- [ ] Query and body schemas are `.strict()`. +- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer. +- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`. +- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. +- [ ] Keyset sorts end in a unique `id` key. +- [ ] The list is classified in `list-pagination.test.ts`. +- [ ] Cross-tenant access answers 404, never 403. +- [ ] 403s carry a machine-readable `details.code`. +- [ ] Validation messages name the field and echo the valid set. +- [ ] Response schema matches every field the route actually emits. +- [ ] OpenAPI description regenerated and truthful about pagination. +- [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass. + +## Known gap + +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. diff --git a/apps/docs/openapi-v2-billing.json b/apps/docs/openapi-v2-billing.json index 05e85b78dd7..a03fc6a80eb 100644 --- a/apps/docs/openapi-v2-billing.json +++ b/apps/docs/openapi-v2-billing.json @@ -174,10 +174,10 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum usage events per page, from 1 to 100.", + "description": "Maximum usage events per page. Must be a whole number from 1 to 100. Defaults to 50.", "schema": { "default": 50, - "description": "Maximum usage events per page, from 1 to 100.", + "description": "Maximum usage events per page. Must be a whole number from 1 to 100. Defaults to 50.", "type": "integer", "minimum": 1, "maximum": 100 diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index 53404f27332..702da1a9924 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -104,11 +104,13 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum files per page, clamped to 1–1000.", + "description": "Maximum files per page. Values outside 1–1000 are truncated and clamped into that range rather than rejected. Defaults to 100.", "schema": { - "description": "Maximum files per page, clamped to 1–1000.", - "default": 100, - "type": "number" + "description": "Maximum files per page. Values outside 1–1000 are truncated and clamped into that range rather than rejected. Defaults to 100.", + "type": "integer", + "minimum": 1, + "maximum": 1000, + "default": 100 } }, { @@ -984,11 +986,11 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum entries per page, from 1 to 100.", + "description": "Maximum audit entries to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "schema": { "default": 50, - "description": "Maximum entries per page, from 1 to 100.", - "type": "number", + "description": "Maximum audit entries to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "type": "integer", "minimum": 1, "maximum": 100 } @@ -1000,7 +1002,8 @@ "description": "Opaque cursor returned by the previous page.", "schema": { "description": "Opaque cursor returned by the previous page.", - "type": "string" + "type": "string", + "minLength": 1 } }, { diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 37a2238c614..2ec9e730198 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -36,7 +36,7 @@ "get": { "operationId": "listKnowledgeBases", "summary": "List Knowledge Bases", - "description": "List knowledge bases in a workspace with folder filtering, search, sorting, and the canonical cursor envelope. The bounded workspace set is returned in one page with `nextCursor` always null; there is no second page to fetch. An unknown `folderPath` is a 404. A workspace whose folder tree exceeds 10,000 folders is a 413, because the response needs the whole tree to render folder paths.", + "description": "List knowledge bases in a workspace with folder filtering, search, and sorting. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null. An unknown `folderPath` is a 404. A workspace whose folder tree exceeds 10,000 folders is a 413, because the response needs the whole tree to render folder paths.", "tags": ["Knowledge Bases"], "parameters": [ { @@ -95,6 +95,30 @@ "type": "string", "enum": ["asc", "desc"] } + }, + { + "name": "limit", + "in": "query", + "required": false, + "description": "Maximum knowledge bases to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "schema": { + "default": 50, + "description": "Maximum knowledge bases to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "type": "integer", + "minimum": 1, + "maximum": 100 + } + }, + { + "name": "cursor", + "in": "query", + "required": false, + "description": "Opaque cursor returned by the previous page.", + "schema": { + "description": "Opaque cursor returned by the previous page.", + "type": "string", + "minLength": 1 + } } ], "responses": { diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index d4df4c2b0df..e38fe2e6026 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -187,21 +187,24 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum log entries per page, clamped to 1–1000.", + "description": "Maximum log entries per page. Values outside 1–1000 are truncated and clamped into that range rather than rejected. Defaults to 100.", "schema": { - "description": "Maximum log entries per page, clamped to 1–1000.", - "default": 100, - "type": "number" + "description": "Maximum log entries per page. Values outside 1–1000 are truncated and clamped into that range rather than rejected. Defaults to 100.", + "type": "integer", + "minimum": 1, + "maximum": 1000, + "default": 100 } }, { "name": "cursor", "in": "query", "required": false, - "description": "Opaque cursor returned by a previous page.", + "description": "Opaque cursor returned by the previous page.", "schema": { + "description": "Opaque cursor returned by the previous page.", "type": "string", - "description": "Opaque cursor returned by a previous page." + "minLength": 1 } }, { diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 920147e91ce..741b4b84251 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -139,10 +139,10 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum members to return. Defaults to 50 and cannot exceed 100.", + "description": "Maximum members to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "schema": { "default": 50, - "description": "Maximum members to return. Defaults to 50 and cannot exceed 100.", + "description": "Maximum members to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "type": "integer", "minimum": 1, "maximum": 100 @@ -152,9 +152,9 @@ "name": "cursor", "in": "query", "required": false, - "description": "Opaque cursor returned by the preceding page.", + "description": "Opaque cursor returned by the previous page.", "schema": { - "description": "Opaque cursor returned by the preceding page.", + "description": "Opaque cursor returned by the previous page.", "type": "string", "minLength": 1 } @@ -599,7 +599,7 @@ "get": { "operationId": "listSkills", "summary": "List Skills", - "description": "List workspace and built-in skills. Built-ins are marked read-only. The list omits skill bodies and uses the standard cursor envelope with `nextCursor` always null, so there is no second page to fetch; fetch one skill to read its content.", + "description": "List workspace and built-in skills. Built-ins are marked read-only. The list omits skill bodies; fetch one skill to read its content. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null.", "tags": ["Skills"], "parameters": [ { @@ -648,6 +648,30 @@ "type": "string", "enum": ["asc", "desc"] } + }, + { + "name": "limit", + "in": "query", + "required": false, + "description": "Maximum skills to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "schema": { + "default": 50, + "description": "Maximum skills to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "type": "integer", + "minimum": 1, + "maximum": 100 + } + }, + { + "name": "cursor", + "in": "query", + "required": false, + "description": "Opaque cursor returned by the previous page.", + "schema": { + "description": "Opaque cursor returned by the previous page.", + "type": "string", + "minLength": 1 + } } ], "responses": { @@ -991,7 +1015,7 @@ "get": { "operationId": "listCustomTools", "summary": "List Custom Tools", - "description": "List code-backed custom tools defined in a workspace. Legacy personal tools are excluded. The bounded workspace set uses the standard cursor envelope with `nextCursor` always null; there is no second page to fetch.", + "description": "List code-backed custom tools defined in a workspace. Legacy personal tools are excluded. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null.", "tags": ["Custom Tools"], "parameters": [ { @@ -1040,6 +1064,30 @@ "type": "string", "enum": ["asc", "desc"] } + }, + { + "name": "limit", + "in": "query", + "required": false, + "description": "Maximum custom tools to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "schema": { + "default": 50, + "description": "Maximum custom tools to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "type": "integer", + "minimum": 1, + "maximum": 100 + } + }, + { + "name": "cursor", + "in": "query", + "required": false, + "description": "Opaque cursor returned by the previous page.", + "schema": { + "description": "Opaque cursor returned by the previous page.", + "type": "string", + "minLength": 1 + } } ], "responses": { @@ -1383,7 +1431,7 @@ "get": { "operationId": "listCredentials", "summary": "List Credentials", - "description": "List OAuth and service-account connections visible to the caller. Secret material is never returned. Credential mutations and single-resource reads are intentionally not exposed. The bounded set uses the standard cursor envelope with `nextCursor` always null; there is no second page to fetch.", + "description": "List OAuth and service-account connections visible to the caller. Secret material is never returned. Credential mutations and single-resource reads are intentionally not exposed. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null.", "tags": ["Credentials"], "parameters": [ { @@ -1454,6 +1502,30 @@ "type": "string", "enum": ["asc", "desc"] } + }, + { + "name": "limit", + "in": "query", + "required": false, + "description": "Maximum credentials to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "schema": { + "default": 50, + "description": "Maximum credentials to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "type": "integer", + "minimum": 1, + "maximum": 100 + } + }, + { + "name": "cursor", + "in": "query", + "required": false, + "description": "Opaque cursor returned by the previous page.", + "schema": { + "description": "Opaque cursor returned by the previous page.", + "type": "string", + "minLength": 1 + } } ], "responses": { @@ -1506,7 +1578,7 @@ "get": { "operationId": "listSecrets", "summary": "List Secrets", - "description": "List workspace and caller-owned personal secret metadata. Only names, scope, role, and timestamps are returned; secret values are never read or returned. The bounded set uses the standard cursor envelope with `nextCursor` always null; there is no second page to fetch. A workspace API key cannot call this operation and is rejected with `403`; use a personal API key.", + "description": "List workspace and caller-owned personal secret metadata. Only names, scope, role, and timestamps are returned; secret values are never read or returned. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null. A workspace API key cannot call this operation and is rejected with `403`; use a personal API key.", "tags": ["Secrets"], "parameters": [ { @@ -1566,6 +1638,30 @@ "type": "string", "enum": ["asc", "desc"] } + }, + { + "name": "limit", + "in": "query", + "required": false, + "description": "Maximum secrets to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "schema": { + "default": 50, + "description": "Maximum secrets to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "type": "integer", + "minimum": 1, + "maximum": 100 + } + }, + { + "name": "cursor", + "in": "query", + "required": false, + "description": "Opaque cursor returned by the previous page.", + "schema": { + "description": "Opaque cursor returned by the previous page.", + "type": "string", + "minLength": 1 + } } ], "responses": { diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index 20c7ec7bb62..64601b2a7ef 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -100,9 +100,9 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum tables to return (1-1000). Fractional or out-of-range values are truncated and clamped into that range rather than rejected.", + "description": "Maximum tables to return per page. Values outside 1–1000 are truncated and clamped into that range rather than rejected. Defaults to 100.", "schema": { - "description": "Maximum tables to return (1-1000). Fractional or out-of-range values are truncated and clamped into that range rather than rejected.", + "description": "Maximum tables to return per page. Values outside 1–1000 are truncated and clamped into that range rather than rejected. Defaults to 100.", "type": "integer", "minimum": 1, "maximum": 1000, @@ -113,9 +113,9 @@ "name": "cursor", "in": "query", "required": false, - "description": "Opaque cursor from the previous page.", + "description": "Opaque cursor returned by the previous page.", "schema": { - "description": "Opaque cursor from the previous page.", + "description": "Opaque cursor returned by the previous page.", "type": "string", "minLength": 1 } diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index 722d9688c06..77cb3151340 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -78,11 +78,11 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum workflows to return per page.", + "description": "Maximum workflows to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "schema": { "default": 50, - "description": "Maximum workflows to return per page.", - "type": "number", + "description": "Maximum workflows to return per page. Must be a whole number from 1 to 100. Defaults to 50.", + "type": "integer", "minimum": 1, "maximum": 100 } @@ -91,10 +91,11 @@ "name": "cursor", "in": "query", "required": false, - "description": "Opaque pagination cursor returned by a previous request.", + "description": "Opaque cursor returned by the previous page.", "schema": { - "description": "Opaque pagination cursor returned by a previous request.", - "type": "string" + "description": "Opaque cursor returned by the previous page.", + "type": "string", + "minLength": 1 } }, { @@ -496,10 +497,10 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum deployment versions to return per page.", + "description": "Maximum deployment versions to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "schema": { "default": 50, - "description": "Maximum deployment versions to return per page.", + "description": "Maximum deployment versions to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "type": "integer", "minimum": 1, "maximum": 100 @@ -509,10 +510,11 @@ "name": "cursor", "in": "query", "required": false, - "description": "Opaque pagination cursor returned by a previous request.", + "description": "Opaque cursor returned by the previous page.", "schema": { - "description": "Opaque pagination cursor returned by a previous request.", - "type": "string" + "description": "Opaque cursor returned by the previous page.", + "type": "string", + "minLength": 1 } } ], @@ -1237,10 +1239,10 @@ "name": "limit", "in": "query", "required": false, - "description": "Maximum workflow runs to return per page.", + "description": "Maximum workflow runs to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "schema": { "default": 50, - "description": "Maximum workflow runs to return per page.", + "description": "Maximum workflow runs to return per page. Must be a whole number from 1 to 100. Defaults to 50.", "type": "integer", "minimum": 1, "maximum": 100 @@ -1250,9 +1252,9 @@ "name": "cursor", "in": "query", "required": false, - "description": "Opaque pagination cursor returned by a previous request.", + "description": "Opaque cursor returned by the previous page.", "schema": { - "description": "Opaque pagination cursor returned by a previous request.", + "description": "Opaque cursor returned by the previous page.", "type": "string", "minLength": 1 } diff --git a/apps/sim/app/api/credentials/route.ts b/apps/sim/app/api/credentials/route.ts index 20b4a4bcac0..69ec1fb54e2 100644 --- a/apps/sim/app/api/credentials/route.ts +++ b/apps/sim/app/api/credentials/route.ts @@ -222,7 +222,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => { types: type ? [type] : undefined, providerId, }) - const credentials = visible.map(({ hasServiceAccountKey: _hasKey, ...rest }) => rest) + const credentials = visible.data.map(({ hasServiceAccountKey: _hasKey, ...rest }) => rest) return NextResponse.json({ credentials }) } catch (error) { diff --git a/apps/sim/app/api/v2/[[...segments]]/route.test.ts b/apps/sim/app/api/v2/[[...segments]]/route.test.ts new file mode 100644 index 00000000000..4d4e14da713 --- /dev/null +++ b/apps/sim/app/api/v2/[[...segments]]/route.test.ts @@ -0,0 +1,50 @@ +/** + * @vitest-environment node + */ +import { NextRequest } from 'next/server' +import { describe, expect, it } from 'vitest' +import { DELETE, GET, PATCH, POST, PUT } from '@/app/api/v2/[[...segments]]/route' + +/** + * An unknown path under `/api/v2` used to fall through to the app's global + * `not-found` page, so a mistyped URL handed an API client a full HTML document + * — the one v2 response a JSON-parsing caller cannot read. + * + * The body must stay byte-identical to the rollout gate's 404 + * (`v2ApiGateError`), which answers 404 so an ungated caller cannot tell "not in + * the cohort" from "no such endpoint". A different body here would give that + * distinction straight back. + */ +describe('unknown /api/v2 path', () => { + const EXPECTED = { error: { code: 'NOT_FOUND', message: 'Not found' } } + + function probe(method: string) { + return new NextRequest('http://localhost/api/v2/nonexistent', { method }) + } + + it('answers JSON, not an HTML document', async () => { + const response = await GET(probe('GET'), undefined) + + expect(response.status).toBe(404) + expect(response.headers.get('content-type')).toContain('application/json') + expect(await response.json()).toEqual(EXPECTED) + }) + + it.each([ + ['POST', POST], + ['PUT', PUT], + ['PATCH', PATCH], + ['DELETE', DELETE], + ])('answers the same envelope for %s', async (method, handler) => { + const response = await handler(probe(method), undefined) + + expect(response.status).toBe(404) + expect(await response.json()).toEqual(EXPECTED) + }) + + it('does not require an API key, so probing a typo cannot become a 401', async () => { + const response = await GET(probe('GET'), undefined) + + expect(response.status).toBe(404) + }) +}) diff --git a/apps/sim/app/api/v2/[[...segments]]/route.ts b/apps/sim/app/api/v2/[[...segments]]/route.ts new file mode 100644 index 00000000000..e02fa71a017 --- /dev/null +++ b/apps/sim/app/api/v2/[[...segments]]/route.ts @@ -0,0 +1,40 @@ +import { withRouteHandler } from '@/lib/core/utils/with-route-handler' +import { v2Error } from '@/app/api/v2/lib/response' + +export const dynamic = 'force-dynamic' +export const revalidate = 0 + +/** + * JSON 404 for any `/api/v2` path that matches no route file. + * + * Without it a mistyped path falls through to the app's global `not-found` + * page and hands an API client a full HTML document, which is the one v2 + * response a JSON-parsing caller cannot read. Every other v2 failure — including + * the rollout gate's own 404 — is the `{ error: { code, message } }` envelope. + * + * The body is deliberately byte-identical to `v2ApiGateError`'s. The gate + * answers 404 so an ungated caller cannot distinguish "not in the rollout + * cohort" from "no such endpoint"; a different body here would reintroduce + * exactly that distinction. + * + * This is a documented raw-`withRouteHandler` route rather than a contract + * builder: it has no contract, no operation, and no authentication, because a + * caller probing an unknown path must get the same answer whether or not it + * holds a key — requiring auth first would turn the 404 into a 401 and confirm + * that the path is special. + * + * Next.js only routes a request here when no literal segment matches, so the 77 + * real v2 routes are unaffected. The optional form (`[[...segments]]`) also + * covers bare `/api/v2`. It cannot fix a 405 on a path that *does* have a route + * file but does not export that verb — Next generates that response itself, + * before any handler runs. + */ +const notFound = () => v2Error('NOT_FOUND', 'Not found') + +export const GET = withRouteHandler(notFound) +export const POST = withRouteHandler(notFound) +export const PUT = withRouteHandler(notFound) +export const PATCH = withRouteHandler(notFound) +export const DELETE = withRouteHandler(notFound) +export const HEAD = withRouteHandler(notFound) +export const OPTIONS = withRouteHandler(notFound) diff --git a/apps/sim/app/api/v2/credentials/route.test.ts b/apps/sim/app/api/v2/credentials/route.test.ts index a1987f1b5af..a84ba1f6d68 100644 --- a/apps/sim/app/api/v2/credentials/route.test.ts +++ b/apps/sim/app/api/v2/credentials/route.test.ts @@ -27,6 +27,7 @@ vi.mock('@/lib/credentials/application/list-workspace-credentials', () => ({ }, })) +import { V2_DEFAULT_PAGE_SIZE } from '@/lib/api/contracts/v2/shared' import { GET } from '@/app/api/v2/credentials/route' const WORKSPACE_ID = '11111111-2222-4333-8444-555555555555' @@ -65,7 +66,12 @@ describe('GET /api/v2/credentials', () => { v2RouteMocks.gate.mockResolvedValue(null) v2RouteMocks.preauthRate.mockResolvedValue(V2_PREAUTH_RATE_LIMIT_ALLOWED) v2RouteMocks.operationRate.mockResolvedValue(V2_OPERATION_RATE_LIMIT_ALLOWED) - mocks.execute.mockResolvedValue({ credentials: [credential] }) + mocks.execute.mockResolvedValue({ + credentials: [credential], + nextCursorKeys: null, + sortBy: 'createdAt', + sortOrder: 'desc', + }) }) it('authenticates and charges before validating workspace input', async () => { @@ -93,6 +99,9 @@ describe('GET /api/v2/credentials', () => { search: undefined, sortBy: 'createdAt', sortOrder: 'desc', + limit: V2_DEFAULT_PAGE_SIZE, + cursor: undefined, + cursorKeys: undefined, }, request, }) diff --git a/apps/sim/app/api/v2/credentials/route.ts b/apps/sim/app/api/v2/credentials/route.ts index 0312ea3a957..057ab1012be 100644 --- a/apps/sim/app/api/v2/credentials/route.ts +++ b/apps/sim/app/api/v2/credentials/route.ts @@ -8,6 +8,7 @@ import { import { listWorkspaceCredentials } from '@/lib/credentials/application/list-workspace-credentials' import { credentialOperations } from '@/lib/credentials/application/operations' import { toV2Credential } from '@/app/api/v2/credentials/utils' +import { cursorSortKey, encodeSortedCursor, readSortedCursor } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -19,10 +20,15 @@ export const GET = defineV2JsonRoute({ operation: credentialOperations.listConnections, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - mapInput: ({ query }) => query, + mapInput: ({ query }) => ({ + ...query, + cursorKeys: readSortedCursor(query.cursor, query.sortBy, query.sortOrder), + }), useCase: listWorkspaceCredentials, - present: ({ credentials }) => ({ + present: ({ credentials, nextCursorKeys, sortBy, sortOrder }) => ({ data: credentials.map(toV2Credential), - nextCursor: null, + nextCursor: nextCursorKeys + ? encodeSortedCursor(cursorSortKey(sortBy, sortOrder), nextCursorKeys) + : null, }), }) diff --git a/apps/sim/app/api/v2/custom-tools/route.test.ts b/apps/sim/app/api/v2/custom-tools/route.test.ts index f0d113e8961..b4609531cff 100644 --- a/apps/sim/app/api/v2/custom-tools/route.test.ts +++ b/apps/sim/app/api/v2/custom-tools/route.test.ts @@ -54,6 +54,7 @@ vi.mock('@/lib/custom-tools/application/use-cases', () => ({ }, })) +import { V2_DEFAULT_PAGE_SIZE } from '@/lib/api/contracts/v2/shared' import { GET, POST } from '@/app/api/v2/custom-tools/route' const WORKSPACE_ID = 'workspace-1' @@ -121,9 +122,10 @@ describe('/api/v2/custom-tools', () => { principal: PRINCIPAL, input: { workspaceId: WORKSPACE_ID, - search: undefined, sortBy: 'createdAt', sortOrder: 'desc', + limit: V2_DEFAULT_PAGE_SIZE, + cursorKeys: undefined, }, request: expect.anything(), }) diff --git a/apps/sim/app/api/v2/custom-tools/route.ts b/apps/sim/app/api/v2/custom-tools/route.ts index 0da00d8ce8f..88d691efd3a 100644 --- a/apps/sim/app/api/v2/custom-tools/route.ts +++ b/apps/sim/app/api/v2/custom-tools/route.ts @@ -14,6 +14,7 @@ import { listWorkspaceCustomToolsUseCase, } from '@/lib/custom-tools/application/use-cases' import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils' +import { cursorSortKey, encodeSortedCursor, readSortedCursor } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -25,9 +26,17 @@ export const GET = defineV2JsonRoute({ auth: v2ApiKeyAuth, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - mapInput: ({ query }) => query, + mapInput: ({ query }) => ({ + ...query, + cursorKeys: readSortedCursor(query.cursor, query.sortBy, query.sortOrder), + }), useCase: listWorkspaceCustomToolsUseCase, - present: ({ tools }) => ({ data: tools.map(toV2CustomTool), nextCursor: null }), + present: ({ tools, nextCursorKeys, sortBy, sortOrder }) => ({ + data: tools.map(toV2CustomTool), + nextCursor: nextCursorKeys + ? encodeSortedCursor(cursorSortKey(sortBy, sortOrder), nextCursorKeys) + : null, + }), }) /** POST /api/v2/custom-tools — Create a custom tool. */ diff --git a/apps/sim/app/api/v2/files/[fileId]/content/route.ts b/apps/sim/app/api/v2/files/[fileId]/content/route.ts index 48d35dd94b7..c2a64201d23 100644 --- a/apps/sim/app/api/v2/files/[fileId]/content/route.ts +++ b/apps/sim/app/api/v2/files/[fileId]/content/route.ts @@ -8,7 +8,6 @@ import { } from '@/lib/workspace-files/application/update-workspace-file-content' import { MAX_WORKSPACE_FILE_INLINE_BODY_BYTES } from '@/lib/workspace-files/orchestration' import { toV2File } from '@/app/api/v2/files/utils' -import { v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -21,7 +20,6 @@ export const PUT = defineV2JsonRoute({ rateLimit: v2RateLimits.publicApi, errorPolicy: v2FileErrorPolicies.concealResourceAuthorization, parseOptions: { - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), maxBodyBytes: MAX_WORKSPACE_FILE_INLINE_BODY_BYTES, }, beforeParse: async ({ principal, params }) => { diff --git a/apps/sim/app/api/v2/files/route.ts b/apps/sim/app/api/v2/files/route.ts index 9b905ca9342..bf3c3b4a291 100644 --- a/apps/sim/app/api/v2/files/route.ts +++ b/apps/sim/app/api/v2/files/route.ts @@ -3,9 +3,7 @@ import { v2CreateFileContract, v2ListFilesContract, } from '@/lib/api/contracts/v2/files' -import { INVALID_CURSOR_MESSAGE } from '@/lib/api/list-query' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' -import { OrchestrationError } from '@/lib/core/orchestration/types' import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils' import { v2FileErrorPolicies } from '@/lib/workspace-files/api' import { createWorkspaceFile } from '@/lib/workspace-files/application/create-workspace-file' @@ -13,12 +11,7 @@ import { queryWorkspaceFilePage } from '@/lib/workspace-files/application/list-w import { fileOperations } from '@/lib/workspace-files/application/operations' import { MAX_WORKSPACE_FILE_INLINE_BODY_BYTES } from '@/lib/workspace-files/orchestration' import { toV2File, toV2Files } from '@/app/api/v2/files/utils' -import { - cursorSortKey, - decodeSortedCursor, - encodeSortedCursor, - v2Error, -} from '@/app/api/v2/lib/response' +import { cursorSortKey, encodeSortedCursor, readSortedCursor } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -30,23 +23,16 @@ export const GET = defineV2JsonRoute({ operation: fileOperations.list, rateLimit: v2RateLimits.publicApi, errorPolicy: v2FileErrorPolicies.default, - mapInput: ({ query }) => { - const cursorSort = cursorSortKey(query.sortBy, query.sortOrder) - const decoded = decodeSortedCursor(query.cursor, cursorSort) - if (decoded.status === 'invalid') { - throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) - } - return { - workspaceId: query.workspaceId, - folderPath: query.folderPath, - search: query.search, - sortBy: query.sortBy, - sortOrder: query.sortOrder, - limit: query.limit, - after: decoded.status === 'ok' ? decoded.keys : undefined, - cursorSort, - } - }, + mapInput: ({ query }) => ({ + workspaceId: query.workspaceId, + folderPath: query.folderPath, + search: query.search, + sortBy: query.sortBy, + sortOrder: query.sortOrder, + limit: query.limit, + after: readSortedCursor(query.cursor, query.sortBy, query.sortOrder), + cursorSort: cursorSortKey(query.sortBy, query.sortOrder), + }), useCase: queryWorkspaceFilePage, present: async ({ files, nextKeys, cursorSort }) => { const items: V2File[] = await toV2Files(files) @@ -62,7 +48,6 @@ export const POST = defineV2JsonRoute({ rateLimit: v2RateLimits.publicApi, errorPolicy: v2FileErrorPolicies.default, parseOptions: { - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), maxBodyBytes: MAX_WORKSPACE_FILE_INLINE_BODY_BYTES, }, mapInput: ({ body }) => ({ diff --git a/apps/sim/app/api/v2/knowledge/[id]/route.ts b/apps/sim/app/api/v2/knowledge/[id]/route.ts index b9bcee78298..6e2fdf65ba3 100644 --- a/apps/sim/app/api/v2/knowledge/[id]/route.ts +++ b/apps/sim/app/api/v2/knowledge/[id]/route.ts @@ -13,7 +13,6 @@ import { } from '@/lib/knowledge/application/knowledge-bases' import { knowledgeOperations } from '@/lib/knowledge/application/operations' import { toV2KnowledgeBase } from '@/app/api/v2/knowledge/utils' -import { v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -42,9 +41,6 @@ export const PATCH = defineV2JsonRoute({ operation: knowledgeOperations.update, rateLimit: v2RateLimits.publicApi, errorPolicy: v2KnowledgeErrorPolicies.concealKnowledgeBaseAuthorization, - parseOptions: { - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), - }, mapInput: ({ params, body }) => ({ knowledgeBaseId: params.id, assertedWorkspaceId: body.workspaceId, diff --git a/apps/sim/app/api/v2/knowledge/folders/route.ts b/apps/sim/app/api/v2/knowledge/folders/route.ts index 45887f4c4b9..8392f343e68 100644 --- a/apps/sim/app/api/v2/knowledge/folders/route.ts +++ b/apps/sim/app/api/v2/knowledge/folders/route.ts @@ -18,7 +18,6 @@ import { relocateKnowledgeFolder, } from '@/lib/knowledge/application/folders' import { knowledgeOperations } from '@/lib/knowledge/application/operations' -import { v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -49,9 +48,6 @@ export const POST = defineV2JsonRoute({ operation: knowledgeOperations.createFolder, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - parseOptions: { - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), - }, mapInput: ({ body }) => ({ workspaceId: body.workspaceId, path: body.path, source: 'api' }), useCase: createKnowledgeFolder, present: ({ folder }) => ({ data: toFolderPathView(folder, folder.path) }), @@ -63,9 +59,6 @@ export const PATCH = defineV2JsonRoute({ operation: knowledgeOperations.relocateFolder, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - parseOptions: { - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), - }, mapInput: ({ body }) => ({ workspaceId: body.workspaceId, path: body.path, diff --git a/apps/sim/app/api/v2/knowledge/route.test.ts b/apps/sim/app/api/v2/knowledge/route.test.ts index 22084ec7aca..d2a9bf3e81a 100644 --- a/apps/sim/app/api/v2/knowledge/route.test.ts +++ b/apps/sim/app/api/v2/knowledge/route.test.ts @@ -59,6 +59,7 @@ vi.mock('@/lib/users/queries', () => ({ requireResolvedUserEmail: (emails: Map, userId: string) => emails.get(userId)!, })) +import { V2_DEFAULT_PAGE_SIZE } from '@/lib/api/contracts/v2/shared' import { GET, POST } from '@/app/api/v2/knowledge/route' const WORKSPACE_ID = 'workspace-1' @@ -104,6 +105,9 @@ describe('/api/v2/knowledge route composition', () => { mockGetUserEmailsByIds.mockResolvedValue(new Map([['user-1', 'owner@example.com']])) mockList.mockResolvedValue({ knowledgeBases: [{ knowledgeBase: buildKnowledgeBase(), folderPath: '/' }], + nextCursorKeys: null, + sortBy: 'name', + sortOrder: 'desc', }) mockCreate.mockResolvedValue({ knowledgeBase: buildKnowledgeBase(), folderPath: '/' }) }) @@ -125,6 +129,8 @@ describe('/api/v2/knowledge route composition', () => { search: 'support', sortBy: 'name', sortOrder: 'desc', + limit: V2_DEFAULT_PAGE_SIZE, + cursorKeys: undefined, }, request, }) diff --git a/apps/sim/app/api/v2/knowledge/route.ts b/apps/sim/app/api/v2/knowledge/route.ts index f8f0ee2e4a5..789db35d934 100644 --- a/apps/sim/app/api/v2/knowledge/route.ts +++ b/apps/sim/app/api/v2/knowledge/route.ts @@ -16,7 +16,7 @@ import { import { knowledgeOperations } from '@/lib/knowledge/application/operations' import { captureServerEvent } from '@/lib/posthog/server' import { toV2KnowledgeBase, toV2KnowledgeBases } from '@/app/api/v2/knowledge/utils' -import { v2Error } from '@/app/api/v2/lib/response' +import { cursorSortKey, encodeSortedCursor, readSortedCursor } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -34,11 +34,15 @@ export const GET = defineV2JsonRoute({ search: query.search, sortBy: query.sortBy, sortOrder: query.sortOrder, + limit: query.limit, + cursorKeys: readSortedCursor(query.cursor, query.sortBy, query.sortOrder), }), useCase: listKnowledgeBases, - present: async ({ knowledgeBases }) => ({ + present: async ({ knowledgeBases, nextCursorKeys, sortBy, sortOrder }) => ({ data: await toV2KnowledgeBases(knowledgeBases), - nextCursor: null, + nextCursor: nextCursorKeys + ? encodeSortedCursor(cursorSortKey(sortBy, sortOrder), nextCursorKeys) + : null, }), }) @@ -49,9 +53,6 @@ export const POST = defineV2JsonRoute({ operation: knowledgeOperations.create, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - parseOptions: { - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), - }, mapInput: ({ body }) => ({ workspaceId: body.workspaceId, name: body.name, diff --git a/apps/sim/app/api/v2/knowledge/search/route.ts b/apps/sim/app/api/v2/knowledge/search/route.ts index 070e1342015..95bef90bd1b 100644 --- a/apps/sim/app/api/v2/knowledge/search/route.ts +++ b/apps/sim/app/api/v2/knowledge/search/route.ts @@ -3,7 +3,6 @@ import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/ import { v2KnowledgeErrorPolicies } from '@/lib/knowledge/api/route-policies' import { knowledgeOperations } from '@/lib/knowledge/application/operations' import { searchKnowledge } from '@/lib/knowledge/application/search' -import { v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -24,7 +23,6 @@ export const POST = defineV2JsonRoute({ errorPolicy: v2KnowledgeErrorPolicies.concealKnowledgeBaseUsageAuthorization, parseOptions: { maxBodyBytes: V2_KNOWLEDGE_SEARCH_MAX_BODY_BYTES, - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), }, mapInput: ({ body }) => ({ workspaceId: body.workspaceId, diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index c0567024d65..a625ecc3e81 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -241,9 +241,25 @@ export function decodeSortedCursor(cursor: string | undefined, sort: string): De return { status: 'ok', keys: decoded.keys } } -/** The 400 for a cursor that cannot be resumed under the request's sort. */ -export function v2CursorSortError(): NextResponse { - return v2Error('BAD_REQUEST', INVALID_CURSOR_MESSAGE) +/** + * The keyset a paged list should resume from, or `undefined` for page one. + * + * This is the `mapInput` half of every keyset list: it stamps the request's + * sort, reads the cursor back under it, and turns a cursor that was minted + * under a different sort into the canonical 400 rather than letting mismatched + * keys reach `keysetAfter`. Sharing it is what keeps "a bad cursor is a 400" + * from being re-decided per route. + */ +export function readSortedCursor( + cursor: string | undefined, + sortBy: string, + sortOrder: string +): CursorKey[] | undefined { + const decoded = decodeSortedCursor(cursor, cursorSortKey(sortBy, sortOrder)) + if (decoded.status === 'invalid') { + throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) + } + return decoded.status === 'ok' ? decoded.keys : undefined } const V2_CODE_BY_ORCHESTRATION_ERROR: Record = { diff --git a/apps/sim/app/api/v2/secrets/route.test.ts b/apps/sim/app/api/v2/secrets/route.test.ts index 5c173b4d883..47147b25f91 100644 --- a/apps/sim/app/api/v2/secrets/route.test.ts +++ b/apps/sim/app/api/v2/secrets/route.test.ts @@ -46,6 +46,7 @@ vi.mock('@/lib/secrets/application/use-cases', () => ({ listSecretsUseCase: { operation: { id: 'secrets.list' }, execute: mocks.list }, })) +import { V2_DEFAULT_PAGE_SIZE } from '@/lib/api/contracts/v2/shared' import { GET } from '@/app/api/v2/secrets/route' const WORKSPACE_ID = 'workspace-1' @@ -88,7 +89,13 @@ describe('GET /api/v2/secrets', () => { mocks.preauthRate.mockResolvedValue(RATE_LIMIT_OK) mocks.operationRate.mockResolvedValue(RATE_LIMIT_OK) mocks.gate.mockResolvedValue(null) - mocks.list.mockResolvedValue({ secrets: [secret], userId: 'user-1' }) + mocks.list.mockResolvedValue({ + secrets: [secret], + userId: 'user-1', + nextCursorKeys: null, + sortBy: 'name', + sortOrder: 'asc', + }) }) it('lists secret metadata without exposing values', async () => { @@ -121,6 +128,9 @@ describe('GET /api/v2/secrets', () => { search: undefined, sortBy: 'name', sortOrder: 'asc', + limit: V2_DEFAULT_PAGE_SIZE, + cursor: undefined, + cursorKeys: undefined, }, request: expect.anything(), }) diff --git a/apps/sim/app/api/v2/secrets/route.ts b/apps/sim/app/api/v2/secrets/route.ts index 62a1685dc88..c0b64d7f338 100644 --- a/apps/sim/app/api/v2/secrets/route.ts +++ b/apps/sim/app/api/v2/secrets/route.ts @@ -7,6 +7,7 @@ import { } from '@/lib/api/server/routes' import { secretOperations } from '@/lib/secrets/application/operations' import { listSecretsUseCase } from '@/lib/secrets/application/use-cases' +import { cursorSortKey, encodeSortedCursor, readSortedCursor } from '@/app/api/v2/lib/response' import { toV2Secret } from '@/app/api/v2/secrets/utils' export const dynamic = 'force-dynamic' @@ -19,10 +20,15 @@ export const GET = defineV2JsonRoute({ auth: v2ApiKeyAuth, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - mapInput: ({ query }) => query, + mapInput: ({ query }) => ({ + ...query, + cursorKeys: readSortedCursor(query.cursor, query.sortBy, query.sortOrder), + }), useCase: listSecretsUseCase, - present: ({ secrets, userId }) => ({ + present: ({ secrets, userId, nextCursorKeys, sortBy, sortOrder }) => ({ data: secrets.map((secret) => toV2Secret(secret, userId)), - nextCursor: null, + nextCursor: nextCursorKeys + ? encodeSortedCursor(cursorSortKey(sortBy, sortOrder), nextCursorKeys) + : null, }), }) diff --git a/apps/sim/app/api/v2/skills/route.test.ts b/apps/sim/app/api/v2/skills/route.test.ts index 6d7770e05a0..4141c05b494 100644 --- a/apps/sim/app/api/v2/skills/route.test.ts +++ b/apps/sim/app/api/v2/skills/route.test.ts @@ -97,7 +97,7 @@ describe('/api/v2/skills', () => { mocks.preauthRate.mockResolvedValue(RATE_LIMIT_OK) mocks.operationRate.mockResolvedValue(RATE_LIMIT_OK) mocks.gate.mockResolvedValue(null) - mocks.list.mockResolvedValue({ skills: [skill] }) + mocks.list.mockResolvedValue({ skills: [skill], hasMore: false, offset: 0, limit: 50 }) mocks.create.mockResolvedValue({ skill }) }) @@ -105,7 +105,9 @@ describe('/api/v2/skills', () => { const response = await GET(request('GET', `/api/v2/skills?workspaceId=${WORKSPACE_ID}`)) expect(response.status).toBe(200) - expect((await response.json()).data[0]).not.toHaveProperty('content') + const body = await response.json() + expect(body.data[0]).not.toHaveProperty('content') + expect(body.nextCursor).toBeNull() expect(mocks.list).toHaveBeenCalledWith({ principal: PRINCIPAL, input: { @@ -113,11 +115,43 @@ describe('/api/v2/skills', () => { search: undefined, sortBy: 'createdAt', sortOrder: 'desc', + limit: 50, + cursor: undefined, + offset: 0, }, request: expect.anything(), }) }) + it('resumes from the offset cursor and mints the next one while pages remain', async () => { + mocks.list.mockResolvedValueOnce({ skills: [skill], hasMore: true, offset: 2, limit: 2 }) + const cursor = Buffer.from(JSON.stringify({ offset: 2 })).toString('base64') + + const response = await GET( + request( + 'GET', + `/api/v2/skills?workspaceId=${WORKSPACE_ID}&limit=2&cursor=${encodeURIComponent(cursor)}` + ) + ) + + expect(response.status).toBe(200) + expect((await response.json()).nextCursor).toBe( + Buffer.from(JSON.stringify({ offset: 4 })).toString('base64') + ) + expect(mocks.list).toHaveBeenCalledWith( + expect.objectContaining({ input: expect.objectContaining({ limit: 2, offset: 2 }) }) + ) + }) + + it('rejects a malformed cursor rather than silently restarting at page one', async () => { + const response = await GET( + request('GET', `/api/v2/skills?workspaceId=${WORKSPACE_ID}&cursor=not-a-cursor`) + ) + + expect(response.status).toBe(400) + expect(mocks.list).not.toHaveBeenCalled() + }) + it('creates a skill with the v2 source and status', async () => { const response = await POST( request('POST', '/api/v2/skills', { diff --git a/apps/sim/app/api/v2/skills/route.ts b/apps/sim/app/api/v2/skills/route.ts index b49c685fe90..27ab9f4b0c5 100644 --- a/apps/sim/app/api/v2/skills/route.ts +++ b/apps/sim/app/api/v2/skills/route.ts @@ -8,6 +8,7 @@ import { import { captureServerEvent } from '@/lib/posthog/server' import { skillOperations } from '@/lib/skills/application/operations' import { createSkillUseCase, listSkillsUseCase } from '@/lib/skills/application/use-cases' +import { decodeOffsetCursor, encodeCursor } from '@/app/api/v2/lib/response' import { toV2Skill, toV2SkillSummary } from '@/app/api/v2/skills/utils' export const dynamic = 'force-dynamic' @@ -20,9 +21,12 @@ export const GET = defineV2JsonRoute({ auth: v2ApiKeyAuth, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - mapInput: ({ query }) => query, + mapInput: ({ query }) => ({ ...query, offset: decodeOffsetCursor(query.cursor) }), useCase: listSkillsUseCase, - present: ({ skills }) => ({ data: skills.map(toV2SkillSummary), nextCursor: null }), + present: ({ skills, hasMore, offset, limit }) => ({ + data: skills.map(toV2SkillSummary), + nextCursor: hasMore ? encodeCursor({ offset: offset + limit }) : null, + }), }) /** POST /api/v2/skills — Create a skill. */ diff --git a/apps/sim/app/api/v2/skills/utils.ts b/apps/sim/app/api/v2/skills/utils.ts index a1cc30ceb02..34ffbc18002 100644 --- a/apps/sim/app/api/v2/skills/utils.ts +++ b/apps/sim/app/api/v2/skills/utils.ts @@ -3,6 +3,7 @@ import type { NextResponse } from 'next/server' import type { V2Skill, V2SkillSummary } from '@/lib/api/contracts/v2/skills' import type { SkillOrchestrationErrorCode } from '@/lib/skills/orchestration' import { isBuiltinSkillId } from '@/lib/workflows/skills/builtin-skills' +import type { SkillSummaryRow } from '@/lib/workflows/skills/operations' import { v2Error } from '@/app/api/v2/lib/response' /** @@ -11,8 +12,11 @@ import { v2Error } from '@/app/api/v2/lib/response' type SkillRow = typeof skill.$inferSelect -/** List projection — no `content`; skill bodies are fetched per skill. */ -export function toV2SkillSummary(row: SkillRow): V2SkillSummary { +/** + * List projection — no `content`; skill bodies are fetched per skill. It takes + * the body-less row so the list query never has to load one. + */ +export function toV2SkillSummary(row: SkillSummaryRow): V2SkillSummary { return { id: row.id, name: row.name, diff --git a/apps/sim/app/api/v2/tables/route.ts b/apps/sim/app/api/v2/tables/route.ts index b9515fdb91a..9099a1f51b9 100644 --- a/apps/sim/app/api/v2/tables/route.ts +++ b/apps/sim/app/api/v2/tables/route.ts @@ -1,11 +1,9 @@ import { v2CreateTableContract, v2ListTablesContract } from '@/lib/api/contracts/v2/tables' -import { INVALID_CURSOR_MESSAGE } from '@/lib/api/list-query' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' -import { OrchestrationError } from '@/lib/core/orchestration/types' import { v2TableErrorPolicies } from '@/lib/table/api' import { tableOperations } from '@/lib/table/application/operations' import { createTableUseCase, listTablesUseCase } from '@/lib/table/application/tables' -import { cursorSortKey, decodeSortedCursor, encodeSortedCursor } from '@/app/api/v2/lib/response' +import { cursorSortKey, encodeSortedCursor, readSortedCursor } from '@/app/api/v2/lib/response' import { toApiTable, toApiTables } from '@/app/api/v2/tables/utils' export const dynamic = 'force-dynamic' @@ -18,22 +16,15 @@ export const GET = defineV2JsonRoute({ auth: v2ApiKeyAuth, rateLimit: v2RateLimits.publicApi, errorPolicy: v2TableErrorPolicies.default, - mapInput: ({ query }) => { - const sort = cursorSortKey(query.sortBy, query.sortOrder) - const decoded = decodeSortedCursor(query.cursor, sort) - if (decoded.status === 'invalid') { - throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) - } - return { - workspaceId: query.workspaceId, - folderPath: query.folderPath, - search: query.search, - sortBy: query.sortBy, - sortOrder: query.sortOrder, - limit: query.limit, - after: decoded.status === 'ok' ? decoded.keys : undefined, - } - }, + mapInput: ({ query }) => ({ + workspaceId: query.workspaceId, + folderPath: query.folderPath, + search: query.search, + sortBy: query.sortBy, + sortOrder: query.sortOrder, + limit: query.limit, + after: readSortedCursor(query.cursor, query.sortBy, query.sortOrder), + }), present: async ({ tables, nextKeys, sortBy, sortOrder }) => ({ data: await toApiTables(tables), nextCursor: nextKeys ? encodeSortedCursor(cursorSortKey(sortBy, sortOrder), nextKeys) : null, diff --git a/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts index 8e77d94b1dd..21e6ad41708 100644 --- a/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/deploy/route.test.ts @@ -30,6 +30,12 @@ import { workflowOperations } from '@/lib/workflows/application/operations' import { DELETE, POST } from '@/app/api/v2/workflows/[id]/deploy/route' describe('/api/v2/workflows/[id]/deploy route definitions', () => { + /** + * Both the malformed-body 400 and the oversized-body 413 are v2 builder + * defaults, so neither belongs on the route. The envelope they produce is + * asserted once against the builder in + * `lib/api/server/routes/v2-error-envelope.test.ts`. + */ it('keeps an omitted deploy body valid and binds the authorized deployment use case', async () => { expect(v2DeployWorkflowContract.body?.parse(undefined)).toEqual({}) expect(POST).toMatchObject({ @@ -46,15 +52,7 @@ describe('/api/v2/workflows/[id]/deploy route definitions', () => { }) ) - const invalidJsonResponse = Reflect.get( - Reflect.get(POST, 'parseOptions'), - 'invalidJsonResponse' - )() - expect(invalidJsonResponse.status).toBe(400) - expect(await invalidJsonResponse.json()).toEqual({ - error: { code: 'BAD_REQUEST', message: 'Request body must be valid JSON' }, - }) - + expect(Reflect.get(Reflect.get(POST, 'parseOptions'), 'invalidJsonResponse')).toBeUndefined() expect( Reflect.get(Reflect.get(POST, 'parseOptions'), 'payloadTooLargeResponse') ).toBeUndefined() diff --git a/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts b/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts index a72e3838f7f..93f0e6a8cdb 100644 --- a/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/deploy/route.ts @@ -8,7 +8,6 @@ import { captureServerEvent } from '@/lib/posthog/server' import { v2WorkflowErrorPolicies } from '@/lib/workflows/api' import { deployWorkflow, undeployWorkflow } from '@/lib/workflows/application/deployments' import { workflowOperations } from '@/lib/workflows/application/operations' -import { v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const runtime = 'nodejs' @@ -22,7 +21,6 @@ export const POST = defineV2JsonRoute({ errorPolicy: v2WorkflowErrorPolicies.concealWorkflowAuthorization, parseOptions: { optionalJsonBody: true, - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), }, mapInput: ({ params, body }) => ({ workflowId: params.id, diff --git a/apps/sim/app/api/v2/workflows/[id]/execute/route.ts b/apps/sim/app/api/v2/workflows/[id]/execute/route.ts index b9fe5eab1e9..95015eb4055 100644 --- a/apps/sim/app/api/v2/workflows/[id]/execute/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/execute/route.ts @@ -12,6 +12,7 @@ import { import { parseRequest } from '@/lib/api/server' import { admitOptionalV2Request, + V2_PARSE_DEFAULTS, V2RouteInfrastructureError, v2ApiKeyAuth, v2RateLimits, @@ -185,6 +186,7 @@ export const POST = withRouteHandler( try { const parsed = await parseRequest(v2ExecuteWorkflowContract, req, context, { + ...V2_PARSE_DEFAULTS, maxBodyBytes: 10 * 1024 * 1024, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts index 2bd018a3df9..e06621ad0cc 100644 --- a/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/rollback/route.test.ts @@ -25,6 +25,12 @@ import { workflowOperations } from '@/lib/workflows/application/operations' import { POST } from '@/app/api/v2/workflows/[id]/rollback/route' describe('/api/v2/workflows/[id]/rollback route definition', () => { + /** + * Both the malformed-body 400 and the oversized-body 413 are v2 builder + * defaults, so neither belongs on the route. The envelope they produce is + * asserted once against the builder in + * `lib/api/server/routes/v2-error-envelope.test.ts`. + */ it('keeps an omitted rollback body valid and delegates version selection to the use case', async () => { expect(v2RollbackWorkflowContract.body?.parse(undefined)).toEqual({}) expect(POST).toMatchObject({ @@ -41,15 +47,7 @@ describe('/api/v2/workflows/[id]/rollback route definition', () => { }) ) - const invalidJsonResponse = Reflect.get( - Reflect.get(POST, 'parseOptions'), - 'invalidJsonResponse' - )() - expect(invalidJsonResponse.status).toBe(400) - expect(await invalidJsonResponse.json()).toEqual({ - error: { code: 'BAD_REQUEST', message: 'Request body must be valid JSON' }, - }) - + expect(Reflect.get(Reflect.get(POST, 'parseOptions'), 'invalidJsonResponse')).toBeUndefined() expect( Reflect.get(Reflect.get(POST, 'parseOptions'), 'payloadTooLargeResponse') ).toBeUndefined() diff --git a/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts b/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts index c0b9b6c20e0..3c40d92b4ba 100644 --- a/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/rollback/route.ts @@ -4,7 +4,6 @@ import { generateRequestId } from '@/lib/core/utils/request' import { v2WorkflowErrorPolicies } from '@/lib/workflows/api' import { activateWorkflowVersion } from '@/lib/workflows/application/deployments' import { workflowOperations } from '@/lib/workflows/application/operations' -import { v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const runtime = 'nodejs' @@ -18,7 +17,6 @@ export const POST = defineV2JsonRoute({ errorPolicy: v2WorkflowErrorPolicies.concealWorkflowAuthorization, parseOptions: { optionalJsonBody: true, - invalidJsonResponse: () => v2Error('BAD_REQUEST', 'Request body must be valid JSON'), }, mapInput: ({ params, body }) => ({ workflowId: params.id, diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.test.ts index 2d726aa208d..87d14c90168 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.test.ts @@ -45,6 +45,7 @@ vi.mock('@/lib/api/server/routes', () => { V2RouteInfrastructureError, v2ApiKeyAuth: { kind: 'v2-api-key' }, v2RateLimits: { publicApi: { kind: 'public-api' } }, + V2_PARSE_DEFAULTS: {}, v2OrchestrationErrorPolicy: { render: renderOrchestrationError }, } }) diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.ts b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.ts index cc0938d8232..5e459182179 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/resume/route.ts @@ -8,6 +8,7 @@ import { import { parseRequest } from '@/lib/api/server' import { admitV2Request, + V2_PARSE_DEFAULTS, V2RouteInfrastructureError, v2ApiKeyAuth, v2RateLimits, @@ -52,6 +53,7 @@ export const POST = withRouteHandler( if (!admission.success) return admission.response const parsed = await parseRequest(v2ResumeWorkflowContract, request, context, { + ...V2_PARSE_DEFAULTS, maxBodyBytes: 10 * 1024 * 1024, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/app/api/v2/workflows/route.ts b/apps/sim/app/api/v2/workflows/route.ts index f4dc34c3ef7..8fd65da66ab 100644 --- a/apps/sim/app/api/v2/workflows/route.ts +++ b/apps/sim/app/api/v2/workflows/route.ts @@ -1,17 +1,15 @@ import type { V2WorkflowListItem } from '@/lib/api/contracts/v2/workflows' import { v2CreateWorkflowContract, v2ListWorkflowsContract } from '@/lib/api/contracts/v2/workflows' -import { INVALID_CURSOR_MESSAGE } from '@/lib/api/list-query' import { defineV2JsonRoute, v2ApiKeyAuth, v2OrchestrationErrorPolicy, v2RateLimits, } from '@/lib/api/server/routes' -import { OrchestrationError } from '@/lib/core/orchestration/types' import { createWorkflow } from '@/lib/workflows/application/create-workflow' import { listWorkflows } from '@/lib/workflows/application/list-workflows' import { workflowOperations } from '@/lib/workflows/application/operations' -import { cursorSortKey, decodeSortedCursor, encodeSortedCursor } from '@/app/api/v2/lib/response' +import { cursorSortKey, encodeSortedCursor, readSortedCursor } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -22,23 +20,16 @@ export const GET = defineV2JsonRoute({ operation: workflowOperations.list, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - mapInput: ({ query }) => { - const sort = cursorSortKey(query.sortBy, query.sortOrder) - const decoded = decodeSortedCursor(query.cursor, sort) - if (decoded.status === 'invalid') { - throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) - } - return { - workspaceId: query.workspaceId, - folderPath: query.folderPath, - deployedOnly: query.deployedOnly, - search: query.search, - sortBy: query.sortBy, - sortOrder: query.sortOrder, - cursorKeys: decoded.status === 'ok' ? decoded.keys : undefined, - limit: query.limit, - } - }, + mapInput: ({ query }) => ({ + workspaceId: query.workspaceId, + folderPath: query.folderPath, + deployedOnly: query.deployedOnly, + search: query.search, + sortBy: query.sortBy, + sortOrder: query.sortOrder, + cursorKeys: readSortedCursor(query.cursor, query.sortBy, query.sortOrder), + limit: query.limit, + }), useCase: listWorkflows, present: ({ workflows, nextCursorKeys, sortBy, sortOrder }) => ({ data: workflows.map( diff --git a/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts index de20ecbc46f..cbaf49cc562 100644 --- a/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts +++ b/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts @@ -46,9 +46,14 @@ const CONTRACTS_DIR = path.resolve(import.meta.dirname, '..', '..') const PAGED_LISTS = [ 'GET /api/v2/audit-logs', 'GET /api/v2/billing/logs', + 'GET /api/v2/credentials', + 'GET /api/v2/custom-tools', 'GET /api/v2/files', + 'GET /api/v2/knowledge', 'GET /api/v2/knowledge/[id]/documents', 'GET /api/v2/logs', + 'GET /api/v2/secrets', + 'GET /api/v2/skills', 'GET /api/v2/tables', 'GET /api/v2/tables/[tableId]/rows', 'POST /api/v2/tables/[tableId]/query', @@ -61,22 +66,35 @@ const PAGED_LISTS = [ /** * Lists that accept neither param and always return `nextCursor: null`, because * the set is small and bounded per workspace or per table. + * + * Every remaining entry but the MCP server list is a *folder* list, and a folder + * tree is already capped where it is loaded + * (`MAX_*_FOLDERS_PER_WORKSPACE`) — bounded by construction rather than by a + * caller's `limit`. */ const FULL_SET_LISTS = [ - 'GET /api/v2/credentials', - 'GET /api/v2/custom-tools', 'GET /api/v2/files/folders', - 'GET /api/v2/knowledge', 'GET /api/v2/knowledge/folders', 'GET /api/v2/mcp-servers', - 'GET /api/v2/secrets', - 'GET /api/v2/skills', 'GET /api/v2/tables/[tableId]/groups', 'GET /api/v2/tables/[tableId]/views', 'GET /api/v2/tables/folders', 'GET /api/v2/workflows/folders', ] as const +/** + * Lists that deliberately truncate a fractional `limit` instead of rejecting it. + * + * These three shipped that leniency and published it in their OpenAPI + * description, so tightening them would turn a currently-successful request into + * an error. Everything else must reject — see the fractional-limit test below. + */ +const CLAMPED_LIMIT_LISTS = new Set([ + 'GET /api/v2/files', + 'GET /api/v2/logs', + 'GET /api/v2/tables', +]) + interface ContractLike { method: string path: string @@ -201,6 +219,47 @@ function paginationParams(variants: string[][]): { any: string[]; all: string[] } } +/** + * Whether a schema rejects keys it does not declare, i.e. is `.strict()`. + * + * This is what separates "this list does not page" from "this list quietly + * throws your `limit` away". Zod strips unknown keys by default, so a full-set + * list that is not strict answers `?limit=1` with 200 and the entire set — the + * caller believes it bounded the response and it did not. Only the outermost + * object is inspected; that is where a query's unknown key is caught. + */ +function rejectsUnknownKeys(schema: unknown, depth: number = MAX_SCHEMA_DEPTH): boolean | null { + if (!schema || depth <= 0) return null + const def = (schema as { def?: Record }).def + if (!def) return null + if (def.type === 'object') { + return (def.catchall as { def?: { type?: string } } | undefined)?.def?.type === 'never' + } + const inner = def.innerType ?? def.in ?? def.schema + return inner ? rejectsUnknownKeys(inner, depth - 1) : null +} + +/** + * Whether a fractional `limit` is rejected by whichever slot carries it. + * + * The other required params are left out on purpose: the schema reports every + * failure at once, so it is enough to ask whether one of them is about `limit`. + * That keeps this free of per-resource fixtures and lets it run over every list + * the sweep finds — which matters, because the one list that still carried the + * original `LIMIT 2.5` defect was the one nobody remembered to add to a + * hand-written list. + */ +function rejectsFractionalLimit(contract: ContractLike): boolean { + for (const schema of [contract.query, contract.body]) { + if (!schema) continue + const result = schema.safeParse({ limit: '1.5' }) + if (!result.success && result.error.issues.some((issue) => issue.path[0] === 'limit')) { + return true + } + } + return false +} + function listContractFiles(dir: string): string[] { const files: string[] = [] for (const entry of readdirSync(dir, { withFileTypes: true }).sort((a, b) => @@ -221,6 +280,10 @@ interface V2ListContract { key: string name: string params: { any: string[]; all: string[] } + /** `undefined` when the contract has no `query`; `null` when it could not be introspected. */ + strictQuery: boolean | null | undefined + /** Whether a fractional `limit` draws a validation issue on `limit` itself. */ + rejectsFractionalLimit: boolean } /** @@ -245,7 +308,13 @@ async function sweepV2ListContracts(): Promise { const label = `${name} (${key})` if (!isListResponse(label, value.response?.schema)) continue if (found.has(key)) continue - found.set(key, { key, name, params: paginationParams(inputVariants(label, value)) }) + found.set(key, { + key, + name, + params: paginationParams(inputVariants(label, value)), + strictQuery: value.query ? rejectsUnknownKeys(value.query) : undefined, + rejectsFractionalLimit: rejectsFractionalLimit(value), + }) } } return [...found.values()].sort((a, b) => a.key.localeCompare(b.key)) @@ -268,7 +337,7 @@ describe('v2 list pagination split', () => { expect( unclassified, - 'A new v2 list must be classified in PAGED_LISTS or FULL_SET_LISTS.' + 'A new v2 list must be classified in PAGED_LISTS or FULL_SET_LISTS. See .agents/skills/v2-api-conventions/SKILL.md.' ).toEqual([]) expect(contracts.map((c) => c.key).sort()).toEqual([...classified].sort()) }, @@ -282,7 +351,7 @@ describe('v2 list pagination split', () => { for (const key of PAGED_LISTS) { expect( byKey.get(key)?.params.all, - `${key} is declared paged, so every input shape it accepts must offer limit and cursor` + `${key} is declared paged, so every input shape it accepts must offer limit and cursor. See .agents/skills/v2-api-conventions/SKILL.md.` ).toEqual(['limit', 'cursor']) } }) @@ -294,11 +363,55 @@ describe('v2 list pagination split', () => { for (const key of FULL_SET_LISTS) { expect( byKey.get(key)?.params.any, - `${key} returns the full set; adding a defaulted limit to any accepted input shape would truncate existing callers` + `${key} returns the full set; adding a defaulted limit to any accepted input shape would truncate existing callers. See .agents/skills/v2-api-conventions/SKILL.md.` ).toEqual([]) } }) + it('makes every v2 list query reject a param it does not implement', async () => { + const contracts = await loadV2ListContracts() + const byKey = new Map(contracts.map((c) => [c.key, c])) + + for (const key of [...PAGED_LISTS, ...FULL_SET_LISTS]) { + const strictQuery = byKey.get(key)?.strictQuery + /** + * `undefined` is a list that takes no query at all — `POST .../query` + * carries its input in the body. `null` means the walk could not tell, + * which must fail rather than pass silently. + */ + if (strictQuery === undefined) continue + expect( + strictQuery, + `${key} must declare its query \`.strict()\`. Zod strips unknown keys by default, so a non-strict list answers ?limit=1 with 200 and the whole set — the caller believes it bounded the response and it did not. See .agents/skills/v2-api-conventions/SKILL.md.` + ).toBe(true) + } + }) + + it('never lets a fractional limit reach the query as a fractional LIMIT', async () => { + const contracts = await loadV2ListContracts() + const byKey = new Map(contracts.map((c) => [c.key, c])) + + for (const key of PAGED_LISTS) { + if (CLAMPED_LIMIT_LISTS.has(key)) continue + expect( + byKey.get(key)?.rejectsFractionalLimit, + `${key} accepts a fractional limit, which reaches Postgres as \`LIMIT 2.5\` and answers 500. Build the param from v2PaginationFields. See .agents/skills/v2-api-conventions/SKILL.md.` + ).toBe(true) + } + }) + + it('holds the clamping lists to truncation rather than rejection', async () => { + const contracts = await loadV2ListContracts() + const byKey = new Map(contracts.map((c) => [c.key, c])) + + for (const key of CLAMPED_LIMIT_LISTS) { + expect( + byKey.get(key)?.rejectsFractionalLimit, + `${key} published that it truncates a fractional limit; rejecting it now would break callers relying on that.` + ).toBe(false) + } + }) + it('sees a pagination param hidden in a single union member', () => { const unionQuery = z.union([ z.object({ workspaceId: z.string(), limit: z.coerce.number().default(50) }), diff --git a/apps/sim/lib/api/contracts/v2/__tests__/pagination-limit.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/pagination-limit.test.ts new file mode 100644 index 00000000000..497c4a71c55 --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/__tests__/pagination-limit.test.ts @@ -0,0 +1,112 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { v2ListBillingLogsContract } from '@/lib/api/contracts/v2/billing' +import { v2ListCredentialsContract } from '@/lib/api/contracts/v2/credentials' +import { v2ListCustomToolsContract } from '@/lib/api/contracts/v2/custom-tools' +import { v2ListKnowledgeBasesContract } from '@/lib/api/contracts/v2/knowledge' +import { v2ListSecretsContract } from '@/lib/api/contracts/v2/secrets' +import { + V2_DEFAULT_PAGE_SIZE, + V2_MAX_PAGE_SIZE, + v2LimitSchema, +} from '@/lib/api/contracts/v2/shared' +import { v2ListSkillsContract } from '@/lib/api/contracts/v2/skills' +import { v2ListWorkflowsContract } from '@/lib/api/contracts/v2/workflows' +import { v2ListWorkspaceMembersContract } from '@/lib/api/contracts/v2/workspaces' + +/** + * `limit` is the one v2 query param whose value reaches SQL as a number rather + * than a bound comparison, so a value that survives validation but is not a + * whole number lands in `LIMIT`. `GET /api/v2/workflows` shipped without + * `.int()` — copied from a sibling that had it — and `?limit=1.5` therefore + * became `LIMIT 2.5`, a Postgres type error surfacing as 500. A caller-supplied + * query value must never be able to produce a 500. + */ +describe('v2 limit validation', () => { + const WORKSPACE = '00000000-0000-4000-8000-000000000000' + + function parseLimit(limit: string) { + return v2ListWorkflowsContract.query?.safeParse({ workspaceId: WORKSPACE, limit }) + } + + it('rejects a fractional limit instead of passing it through to SQL', () => { + for (const limit of ['1.5', '2.5', '3.7', '99.9']) { + const parsed = parseLimit(limit) + expect(parsed?.success, `limit=${limit} must not parse`).toBe(false) + expect(parsed?.error?.issues[0]?.message).toBe('limit must be a whole number') + } + }) + + it('still accepts an integral limit written with a decimal point', () => { + const parsed = parseLimit('2.0') + expect(parsed?.success).toBe(true) + expect(parsed?.success && parsed.data.limit).toBe(2) + }) + + it('names the parameter for a non-finite limit rather than "expected number, received number"', () => { + for (const limit of ['Infinity', '-Infinity', 'abc']) { + const parsed = parseLimit(limit) + expect(parsed?.success).toBe(false) + expect(parsed?.error?.issues[0]?.message).toBe('limit must be a number') + } + }) + + it('reports the bound it enforces', () => { + expect(parseLimit('0')?.error?.issues[0]?.message).toBe('limit must be at least 1') + expect(parseLimit('101')?.error?.issues[0]?.message).toBe( + `limit cannot exceed ${V2_MAX_PAGE_SIZE}` + ) + }) + + /** + * The five collections that gained pagination in this change, plus the three + * that already had it. Every one must answer a fractional `limit` the same + * way — the divergence is what let `/workflows` drift into a 500 while its + * two same-file siblings stayed correct. + */ + const PAGED_CONTRACTS = [ + ['workflows', v2ListWorkflowsContract, { workspaceId: WORKSPACE }], + ['workspace members', v2ListWorkspaceMembersContract, {}], + ['billing logs', v2ListBillingLogsContract, { workspaceId: WORKSPACE }], + ['credentials', v2ListCredentialsContract, { workspaceId: WORKSPACE }], + ['custom tools', v2ListCustomToolsContract, { workspaceId: WORKSPACE }], + ['knowledge bases', v2ListKnowledgeBasesContract, { workspaceId: WORKSPACE }], + ['secrets', v2ListSecretsContract, { workspaceId: WORKSPACE }], + ['skills', v2ListSkillsContract, { workspaceId: WORKSPACE }], + ] as const + + it.each(PAGED_CONTRACTS)('rejects a fractional limit on %s', (_name, contract, base) => { + const parsed = contract.query?.safeParse({ ...base, limit: '1.5' }) + expect(parsed?.success).toBe(false) + expect(parsed?.error?.issues[0]?.message).toBe('limit must be a whole number') + }) + + it.each(PAGED_CONTRACTS)('defaults %s to the shared page size', (_name, contract, base) => { + const parsed = contract.query?.safeParse(base) + expect(parsed?.success).toBe(true) + expect(parsed?.success && parsed.data.limit).toBe(V2_DEFAULT_PAGE_SIZE) + }) + + /** + * `/files`, `/logs`, and `/tables` shipped truncating and clamping instead, + * and published that leniency in their OpenAPI description, so they keep it. + * Pinning it here is what makes the difference deliberate rather than another + * copy that lost its `.int()`. + */ + describe('the clamping variant kept for already-shipped lenient lists', () => { + const clamped = v2LimitSchema({ max: 1000, fallback: 100, outOfRange: 'clamp' }) + + it('truncates a fractional limit rather than rejecting it', () => { + expect(clamped.parse('1.5')).toBe(1) + expect(clamped.parse('7.9')).toBe(7) + }) + + it('clamps out-of-range values into the accepted band', () => { + expect(clamped.parse('0')).toBe(1) + expect(clamped.parse('-5')).toBe(1) + expect(clamped.parse('99999')).toBe(1000) + }) + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/audit-logs.ts b/apps/sim/lib/api/contracts/v2/audit-logs.ts index b2485d29cfc..8df2a91061e 100644 --- a/apps/sim/lib/api/contracts/v2/audit-logs.ts +++ b/apps/sim/lib/api/contracts/v2/audit-logs.ts @@ -5,7 +5,11 @@ import { v1AuditLogParamsSchema, v1ListAuditLogsQuerySchema, } from '@/lib/api/contracts/v1/audit-logs' -import { v2CursorListResponse, v2DataResponse } from '@/lib/api/contracts/v2/shared' +import { + v2CursorListResponse, + v2DataResponse, + v2PaginationFields, +} from '@/lib/api/contracts/v2/shared' /** * v2 audit-logs contracts. These are org-scoped enterprise endpoints. The @@ -84,12 +88,7 @@ export const v2ListAuditLogsQuerySchema = v1ListAuditLogsQuerySchema includeDeparted: v1ListAuditLogsQuerySchema.shape.includeDeparted.describe( 'Include actions by users who have left the organization.' ), - limit: v1ListAuditLogsQuerySchema.shape.limit.describe( - 'Maximum entries per page, from 1 to 100.' - ), - cursor: v1ListAuditLogsQuerySchema.shape.cursor.describe( - 'Opaque cursor returned by the previous page.' - ), + ...v2PaginationFields({ description: 'Maximum audit entries to return per page.' }), organizationId: organizationIdSchema.describe( 'Organization whose audit trail should be queried.' ), diff --git a/apps/sim/lib/api/contracts/v2/billing.ts b/apps/sim/lib/api/contracts/v2/billing.ts index 3dcfd28b9d9..78eb5759bdb 100644 --- a/apps/sim/lib/api/contracts/v2/billing.ts +++ b/apps/sim/lib/api/contracts/v2/billing.ts @@ -2,7 +2,11 @@ import { z } from 'zod' import { workspaceIdSchema } from '@/lib/api/contracts/primitives' import { defineRouteContract } from '@/lib/api/contracts/types' import { usageLogPeriodSchema, usageLogSourceSchema } from '@/lib/api/contracts/user' -import { v2CursorListResponse, v2DataResponse } from '@/lib/api/contracts/v2/shared' +import { + v2CursorListResponse, + v2DataResponse, + v2PaginationFields, +} from '@/lib/api/contracts/v2/shared' /** * v2 billing contracts — separate read-only status and ledger resources. @@ -141,20 +145,9 @@ export const v2BillingLogsQuerySchema = z endDate: parseableDateSchema .optional() .describe('End of a custom window as a Date-parseable string; defaults to now.'), - limit: z.coerce - .number() - .int() - .min(1) - .max(100) - .optional() - .default(50) - .describe('Maximum usage events per page, from 1 to 100.'), - cursor: z - .string() - .min(1, 'cursor must be a non-empty token') - .optional() - .describe('Opaque cursor returned by the previous page.'), + ...v2PaginationFields({ description: 'Maximum usage events per page.' }), }) + .strict() .refine((query) => query.period !== 'custom' || query.startDate !== undefined, { error: 'startDate is required when period is "custom"', path: ['startDate'], diff --git a/apps/sim/lib/api/contracts/v2/credentials.ts b/apps/sim/lib/api/contracts/v2/credentials.ts index 5e164406730..6694a626a3c 100644 --- a/apps/sim/lib/api/contracts/v2/credentials.ts +++ b/apps/sim/lib/api/contracts/v2/credentials.ts @@ -4,6 +4,7 @@ import { workspaceIdSchema } from '@/lib/api/contracts/primitives' import { defineRouteContract } from '@/lib/api/contracts/types' import { v2CursorListResponse, + v2PaginationFields, v2SearchSchema, v2SortFields, v2TimestampSchema, @@ -47,22 +48,29 @@ export type V2Credential = z.output export const v2CredentialSortFields = ['displayName', 'createdAt', 'updatedAt'] as const export type V2CredentialSortBy = (typeof v2CredentialSortFields)[number] -export const v2ListCredentialsQuerySchema = z.object({ - workspaceId: workspaceIdSchema.describe('Workspace whose credentials should be listed.'), - type: v2CredentialTypeSchema.optional().describe('Restrict results to this credential type.'), - providerId: z - .string() - .min(1, 'providerId cannot be empty') - .optional() - .describe('Restrict results to credentials for this integration provider.'), - search: v2SearchSchema.describe( - 'Case-insensitive substring match against the credential display name.' - ), - ...v2SortFields(v2CredentialSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), -}) +export const v2ListCredentialsQuerySchema = z + .object({ + workspaceId: workspaceIdSchema.describe('Workspace whose credentials should be listed.'), + type: v2CredentialTypeSchema.optional().describe('Restrict results to this credential type.'), + providerId: z + .string() + .min(1, 'providerId cannot be empty') + .optional() + .describe('Restrict results to credentials for this integration provider.'), + search: v2SearchSchema.describe( + 'Case-insensitive substring match against the credential display name.' + ), + ...v2SortFields(v2CredentialSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), + ...v2PaginationFields({ description: 'Maximum credentials to return per page.' }), + }) + .strict() export type V2ListCredentialsQuery = z.output -/** Lists OAuth and service-account connections. Credential mutations are intentionally absent. */ +/** + * Lists OAuth and service-account connections, keyset-paginated over the active + * sort. Credential mutations are intentionally absent. Nothing capped the + * per-workspace set before pagination, so the response grew without bound. + */ export const v2ListCredentialsContract = defineRouteContract({ method: 'GET', path: '/api/v2/credentials', diff --git a/apps/sim/lib/api/contracts/v2/custom-tools.ts b/apps/sim/lib/api/contracts/v2/custom-tools.ts index 9b259ca1686..ae764afba70 100644 --- a/apps/sim/lib/api/contracts/v2/custom-tools.ts +++ b/apps/sim/lib/api/contracts/v2/custom-tools.ts @@ -8,6 +8,7 @@ import { defineRouteContract } from '@/lib/api/contracts/types' import { v2CursorListResponse, v2DataResponse, + v2PaginationFields, v2SearchSchema, v2SortFields, v2TimestampSchema, @@ -119,10 +120,13 @@ export const v2CustomToolSortFields = ['title', 'createdAt', 'updatedAt'] as con export type V2CustomToolSortBy = (typeof v2CustomToolSortFields)[number] -export const v2ListCustomToolsQuerySchema = v2CustomToolWorkspaceQuerySchema.extend({ - search: v2SearchSchema.describe('Case-insensitive substring match against the tool title.'), - ...v2SortFields(v2CustomToolSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), -}) +export const v2ListCustomToolsQuerySchema = v2CustomToolWorkspaceQuerySchema + .extend({ + search: v2SearchSchema.describe('Case-insensitive substring match against the tool title.'), + ...v2SortFields(v2CustomToolSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), + ...v2PaginationFields({ description: 'Maximum custom tools to return per page.' }), + }) + .strict() export type V2ListCustomToolsQuery = z.output @@ -159,9 +163,9 @@ export const v2UpdateCustomToolBodySchema = z export type V2UpdateCustomToolBody = z.input /** - * Custom tool list. The per-workspace set is small and bounded, so the full set - * is returned as a single page (`nextCursor` is always `null`); the canonical - * cursor envelope keeps the v2 list surface uniform. + * Custom tool list, keyset-paginated over the active sort. Nothing capped the + * per-workspace set, and each row carries its full `code` and `schema`, so the + * response grew without bound before pagination. */ export const v2ListCustomToolsContract = defineRouteContract({ method: 'GET', diff --git a/apps/sim/lib/api/contracts/v2/files.ts b/apps/sim/lib/api/contracts/v2/files.ts index 01a50144b8b..3e6d2b6e2a7 100644 --- a/apps/sim/lib/api/contracts/v2/files.ts +++ b/apps/sim/lib/api/contracts/v2/files.ts @@ -16,6 +16,7 @@ import { v2FolderPathSchema, v2FolderSchema, v2ListFoldersQuerySchema, + v2PaginationFields, v2RelocateFolderBodySchema, v2SearchSchema, v2SortFields, @@ -287,13 +288,12 @@ export const v2ListFilesQuerySchema = z .describe('Restrict results to files directly inside this folder.'), search: v2SearchSchema.describe('Case-insensitive substring match against the file name.'), ...v2SortFields(v2FileSortFields, { sortBy: 'uploadedAt', sortOrder: 'asc' }), - limit: z.coerce - .number() - .optional() - .default(100) - .transform((v) => Math.min(Math.max(1, Math.trunc(v)), 1000)) - .describe('Maximum files per page, clamped to 1–1000.'), - cursor: z.string().min(1).optional().describe('Opaque cursor returned by the previous page.'), + ...v2PaginationFields({ + max: 1000, + fallback: 100, + outOfRange: 'clamp', + description: 'Maximum files per page.', + }), }) .strict() diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index acc4ecf3ed1..1f8e4d4a6c2 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -25,6 +25,7 @@ import { v2FolderPathSchema, v2FolderSchema, v2ListFoldersQuerySchema, + v2PaginationFields, v2RelocateFolderBodySchema, v2SearchSchema, v2SortFields, @@ -504,6 +505,7 @@ export const v2ListKnowledgeBasesQuerySchema = z .describe('Restrict results to knowledge bases in this folder.'), search: v2SearchSchema, ...v2SortFields(v2KnowledgeBaseSortFields, { sortBy: 'createdAt', sortOrder: 'asc' }), + ...v2PaginationFields({ description: 'Maximum knowledge bases to return per page.' }), }) .strict() @@ -578,11 +580,12 @@ export const v2UpdateKnowledgeBaseBodySchema = z }) /** - * KB list. `getKnowledgeBases` returns the full workspace set (a small, bounded - * per-workspace list), so today the cursor list is a single full page - * (`nextCursor` always `null`). The canonical cursor envelope keeps the v2 list - * surface uniform; real pagination can be added later behind the opaque cursor. - * Search, folder filter, and sort all run in that query, not over its result. + * KB list, keyset-paginated over the active sort. Search, folder filter, and + * sort all run in the query, not over its result. + * + * Before pagination this list returned `nextCursor` while rejecting `limit` and + * `cursor` outright, so it advertised a pagination scheme no caller could + * drive. */ export const v2ListKnowledgeBasesContract = defineRouteContract({ method: 'GET', @@ -782,6 +785,7 @@ export const v2ListKnowledgeDocumentsQuerySchema = v1ListKnowledgeDocumentsQuery sortOrder: v1ListKnowledgeDocumentsQuerySchema.shape.sortOrder.describe('Sort direction.'), cursor: z.string().min(1).optional().describe('Opaque cursor returned by the previous page.'), }) + .strict() export type V2ListKnowledgeDocumentsQuery = z.output export const v2ListKnowledgeDocumentsContract = defineRouteContract({ diff --git a/apps/sim/lib/api/contracts/v2/logs.ts b/apps/sim/lib/api/contracts/v2/logs.ts index 18895a327cf..a8f7c1af30e 100644 --- a/apps/sim/lib/api/contracts/v2/logs.ts +++ b/apps/sim/lib/api/contracts/v2/logs.ts @@ -8,6 +8,7 @@ import { v2DataResponse, v2FolderPathInputSchema, v2FolderPathSchema, + v2PaginationFields, v2TimestampSchema, } from '@/lib/api/contracts/v2/shared' import { PERSISTED_WORKFLOW_EXECUTION_STATUSES } from '@/lib/logs/types' @@ -225,13 +226,12 @@ export const v2ListLogsQuerySchema = v1ListLogsQuerySchema .describe('Whether to include the final workflow output.') .optional() .default(false), - limit: z.coerce - .number() - .optional() - .default(100) - .transform((value) => Math.min(Math.max(1, Math.trunc(value)), 1000)) - .describe('Maximum log entries per page, clamped to 1–1000.'), - cursor: z.string().describe('Opaque cursor returned by a previous page.').optional(), + ...v2PaginationFields({ + max: 1000, + fallback: 100, + outOfRange: 'clamp', + description: 'Maximum log entries per page.', + }), order: z .enum(['desc', 'asc']) .describe('Sort order by execution start time.') diff --git a/apps/sim/lib/api/contracts/v2/mcp-servers.ts b/apps/sim/lib/api/contracts/v2/mcp-servers.ts index 86d2db5d08f..4ceae20d290 100644 --- a/apps/sim/lib/api/contracts/v2/mcp-servers.ts +++ b/apps/sim/lib/api/contracts/v2/mcp-servers.ts @@ -176,10 +176,12 @@ export const v2McpServerSortFields = ['name', 'createdAt', 'updatedAt'] as const export type V2McpServerSortBy = (typeof v2McpServerSortFields)[number] -export const v2ListMcpServersQuerySchema = v2McpServerWorkspaceQuerySchema.extend({ - search: v2SearchSchema.describe('Case-insensitive substring match against the server name.'), - ...v2SortFields(v2McpServerSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), -}) +export const v2ListMcpServersQuerySchema = v2McpServerWorkspaceQuerySchema + .extend({ + search: v2SearchSchema.describe('Case-insensitive substring match against the server name.'), + ...v2SortFields(v2McpServerSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), + }) + .strict() export type V2ListMcpServersQuery = z.output diff --git a/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts b/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts index 3e86d717eb9..4e4b7a1f68b 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/knowledge.ts @@ -66,7 +66,7 @@ const routes = [ knowledgeOperation({ operationId: 'listKnowledgeBases', summary: 'List Knowledge Bases', - description: `List knowledge bases in a workspace with folder filtering, search, sorting, and the canonical cursor envelope. The bounded workspace set is returned in one page with \`nextCursor\` always null; there is no second page to fetch. An unknown \`folderPath\` is a 404. ${FOLDER_TREE_TOO_LARGE}`, + description: `List knowledge bases in a workspace with folder filtering, search, and sorting. Paginate with \`limit\` and \`cursor\`, stopping when \`nextCursor\` is null. An unknown \`folderPath\` is a 404. ${FOLDER_TREE_TOO_LARGE}`, errors: [...RESOURCE_ERRORS, 'PayloadTooLarge'], success: { description: 'A page of knowledge bases.' }, }), diff --git a/apps/sim/lib/api/contracts/v2/openapi/resources.ts b/apps/sim/lib/api/contracts/v2/openapi/resources.ts index 2ab84ab4128..66ebd341d63 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/resources.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/resources.ts @@ -409,7 +409,7 @@ const routes = [ operationId: 'listSkills', summary: 'List Skills', description: - 'List workspace and built-in skills. Built-ins are marked read-only. The list omits skill bodies and uses the standard cursor envelope with `nextCursor` always null, so there is no second page to fetch; fetch one skill to read its content.', + 'List workspace and built-in skills. Built-ins are marked read-only. The list omits skill bodies; fetch one skill to read its content. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null.', errors: [...WORKSPACE_ERRORS, 'NotFound'], success: { description: 'Skills available in the workspace.' }, }), @@ -564,7 +564,7 @@ const routes = [ operationId: 'listCustomTools', summary: 'List Custom Tools', description: - 'List code-backed custom tools defined in a workspace. Legacy personal tools are excluded. The bounded workspace set uses the standard cursor envelope with `nextCursor` always null; there is no second page to fetch.', + 'List code-backed custom tools defined in a workspace. Legacy personal tools are excluded. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null.', errors: [...WORKSPACE_ERRORS, 'NotFound'], success: { description: 'Custom tools defined in the workspace.' }, }), @@ -720,7 +720,7 @@ const routes = [ operationId: 'listCredentials', summary: 'List Credentials', description: - 'List OAuth and service-account connections visible to the caller. Secret material is never returned. Credential mutations and single-resource reads are intentionally not exposed. The bounded set uses the standard cursor envelope with `nextCursor` always null; there is no second page to fetch.', + 'List OAuth and service-account connections visible to the caller. Secret material is never returned. Credential mutations and single-resource reads are intentionally not exposed. Paginate with `limit` and `cursor`, stopping when `nextCursor` is null.', errors: [...WORKSPACE_ERRORS, 'NotFound'], success: { description: 'Credentials visible to the caller.' }, }), @@ -745,7 +745,7 @@ const routes = [ resourceOperation('Secrets', { operationId: 'listSecrets', summary: 'List Secrets', - description: `List workspace and caller-owned personal secret metadata. Only names, scope, role, and timestamps are returned; secret values are never read or returned. The bounded set uses the standard cursor envelope with \`nextCursor\` always null; there is no second page to fetch. ${WORKSPACE_API_KEY_DENIED}`, + description: `List workspace and caller-owned personal secret metadata. Only names, scope, role, and timestamps are returned; secret values are never read or returned. Paginate with \`limit\` and \`cursor\`, stopping when \`nextCursor\` is null. ${WORKSPACE_API_KEY_DENIED}`, errors: [...WORKSPACE_ERRORS, 'NotFound'], success: { description: 'Secret metadata visible to the caller.' }, }), diff --git a/apps/sim/lib/api/contracts/v2/secrets.ts b/apps/sim/lib/api/contracts/v2/secrets.ts index ee566ec649a..13744bf83cb 100644 --- a/apps/sim/lib/api/contracts/v2/secrets.ts +++ b/apps/sim/lib/api/contracts/v2/secrets.ts @@ -5,6 +5,7 @@ import { defineRouteContract } from '@/lib/api/contracts/types' import { v2CursorListResponse, v2DataResponse, + v2PaginationFields, v2SearchSchema, v2SortFields, v2TimestampSchema, @@ -57,12 +58,15 @@ export type V2SecretDeleteData = z.output export const v2SecretSortFields = ['name', 'createdAt', 'updatedAt'] as const export type V2SecretSortBy = (typeof v2SecretSortFields)[number] -export const v2ListSecretsQuerySchema = z.object({ - workspaceId: workspaceIdSchema.describe('Workspace whose secret metadata should be listed.'), - scope: v2SecretScopeSchema.optional().describe('Restrict results to one ownership scope.'), - search: v2SearchSchema.describe('Case-insensitive substring match against the secret name.'), - ...v2SortFields(v2SecretSortFields, { sortBy: 'name', sortOrder: 'asc' }), -}) +export const v2ListSecretsQuerySchema = z + .object({ + workspaceId: workspaceIdSchema.describe('Workspace whose secret metadata should be listed.'), + scope: v2SecretScopeSchema.optional().describe('Restrict results to one ownership scope.'), + search: v2SearchSchema.describe('Case-insensitive substring match against the secret name.'), + ...v2SortFields(v2SecretSortFields, { sortBy: 'name', sortOrder: 'asc' }), + ...v2PaginationFields({ description: 'Maximum secrets to return per page.' }), + }) + .strict() export type V2ListSecretsQuery = z.output export const v2SecretParamsSchema = z.object({ @@ -90,7 +94,10 @@ export const v2DeleteSecretQuerySchema = z.object({ }) export type V2DeleteSecretQuery = z.output -/** Lists names and metadata only. There is deliberately no single-secret GET. */ +/** + * Lists names and metadata only, keyset-paginated over the active sort. There is + * deliberately no single-secret GET, and no response ever carries a value. + */ export const v2ListSecretsContract = defineRouteContract({ method: 'GET', path: '/api/v2/secrets', diff --git a/apps/sim/lib/api/contracts/v2/shared.ts b/apps/sim/lib/api/contracts/v2/shared.ts index d6e05660d9e..62647fa6a74 100644 --- a/apps/sim/lib/api/contracts/v2/shared.ts +++ b/apps/sim/lib/api/contracts/v2/shared.ts @@ -70,9 +70,9 @@ import { FolderPathError, parseFolderPath, requireNonRootFolderPath } from '@/li * on the surface (`scope`, `folderPath`, `deployedOnly`, `type`, `providerId`, * `resourceType`). No generic filter expression. * - * Every one of these is pushed into SQL, except on `GET /skills` (which merges - * the static builtin registry into the DB rows, then re-filters and re-sorts the - * merged array) and `GET /files/folders` (which applies `parentPath` and `search` + * Every one of these is pushed into SQL, except on `GET /skills` (which narrows the + * static builtin registry with the same search term, merges it into the DB rows, + * then re-sorts the merged array) and `GET /files/folders` (which applies `parentPath` and `search` * in JS; its sort is pushed into SQL like every other folder list). Both read a * full result set to produce a page; neither is a pattern to copy. * @@ -82,10 +82,25 @@ import { FolderPathError, parseFolderPath, requireNonRootFolderPath } from '@/li * not restated here. A full-set list returns `nextCursor: null` on every * response — its OpenAPI description says so explicitly, so a caller never * writes a pagination loop that can only ever run once. - * Adding `limit`/`cursor` to a full-set list is additive, - * but making a `limit` *default* would silently truncate callers that rely on - * the full set today, so a default page size cannot be introduced without a - * version bump. + * + * Every list whose result set grows with workspace content is now paged. What + * remains full-set is the folder lists, whose trees are already capped where + * they load, plus `GET /mcp-servers`. + * + * Adding `limit`/`cursor` to a full-set list is additive, but giving it a + * *default* `limit` truncates callers reading the whole set today, so it is a + * breaking change. Five lists took exactly that change while `v2-api` was off + * in production and enabled only for a staging cohort — the window in which it + * costs nothing. Once v2 is generally available, moving a shipped full-set list + * to a defaulted page size needs a version bump. + * + * Both cursor schemes are opaque base64-JSON from `app/api/v2/lib/response.ts`, + * and which one a list uses is decided by what its read can express rather than + * by preference: a keyset (`encodeSortedCursor`) wherever the page comes from + * one ordered SQL read, and an offset (`encodeCursor({ offset })`) only where it + * cannot — `GET /skills`, which merges the static builtin registry into the DB + * rows and re-sorts in JS, and `GET /knowledge/{id}/documents`, whose underlying + * query is limit/offset. Prefer the keyset; an offset needs that kind of reason. * * ## Sort and the opaque cursor * @@ -151,6 +166,112 @@ export const v2CursorListResponse = (itemSchema: T) => ), }) +/** + * Default and maximum page size for a v2 paged list. + * + * These are the values the majority of already-paged v2 lists shipped with + * (`/workflows`, `/workflows/{id}/versions`, `/workflows/{id}/runs`, + * `/workspaces/{id}/members`, `/billing/logs`), so they are what a list adopting + * pagination now inherits. + */ +export const V2_DEFAULT_PAGE_SIZE = 50 +export const V2_MAX_PAGE_SIZE = 100 + +/** + * How a `limit` outside `1..max` is handled. + * + * `reject` is the rule for every list: an out-of-range or fractional page size + * is a 400 naming the bound. `clamp` exists only for the three lists that + * shipped truncating and clamping instead (`/files`, `/logs`, `/tables`) and + * published that leniency in their OpenAPI description — flipping them to + * `reject` would turn a currently-successful request into an error for callers + * already relying on it. New lists must use `reject`; `clamp` is not a pattern + * to copy and should be collapsed into `reject` at the next major version. + */ +export type V2LimitOutOfRange = 'reject' | 'clamp' + +interface V2LimitOptions { + max?: number + /** Page size applied when the caller omits `limit`. */ + fallback?: number + /** Defaults to `reject`. */ + outOfRange?: V2LimitOutOfRange + /** Overrides the generated `describe()` text. */ + description?: string +} + +/** + * The v2 `limit` param, as one schema so the family cannot drift per route. + * + * It exists because it already drifted: `GET /api/v2/workflows` was copied from + * a sibling and lost its `.int()`, so a fractional `limit` passed validation and + * reached Postgres as `LIMIT 2.5`, which is a 500. A caller-supplied query value + * must never be able to produce a 500, and the only durable fix is for every + * list to derive its bound from one place rather than restating it. + * + * `z.coerce.number()` is what accepts the query string at all, so JS numeric + * parsing applies (`1e2` is 100, `0x10` is 16, surrounding whitespace is + * ignored). That leniency is inherited from the coercion, not chosen here; the + * bounds below are what actually constrain the value. + */ +export function v2LimitSchema(options: V2LimitOptions = {}) { + const { + max = V2_MAX_PAGE_SIZE, + fallback = V2_DEFAULT_PAGE_SIZE, + outOfRange = 'reject', + description, + } = options + + /** + * The bounds are appended rather than left to the caller's sentence, so a + * per-list `description` cannot drop them from the published parameter — the + * numbers a caller needs are the ones this schema actually enforces. + */ + const bounds = + outOfRange === 'clamp' + ? `Values outside 1–${max} are truncated and clamped into that range rather than rejected. Defaults to ${fallback}.` + : `Must be a whole number from 1 to ${max}. Defaults to ${fallback}.` + const described = `${description ?? 'Maximum items to return per page.'} ${bounds}` + + const base = z.coerce.number({ error: 'limit must be a number' }) + + if (outOfRange === 'clamp') { + return base + .optional() + .default(fallback) + .transform((value) => Math.min(Math.max(1, Math.trunc(value)), max)) + .describe(described) + .meta({ type: 'integer', minimum: 1, maximum: max }) + } + + return base + .int('limit must be a whole number') + .min(1, 'limit must be at least 1') + .max(max, `limit cannot exceed ${max}`) + .optional() + .default(fallback) + .describe(described) +} + +/** + * The v2 `cursor` param: the opaque token a previous page returned as + * `nextCursor`. Empty is rejected rather than treated as "start over", so a + * caller that accidentally forwards an empty string learns about it instead of + * looping on page one. + */ +export function v2CursorSchema(description = 'Opaque cursor returned by the previous page.') { + return z.string().min(1, 'cursor must be a non-empty token').optional().describe(description) +} + +/** + * The `limit` + `cursor` pair for a paged v2 list. Spread into a query object; + * a list that returns `nextCursor` must accept both, and must actually apply + * them. + */ +export function v2PaginationFields(options: V2LimitOptions = {}) { + return { limit: v2LimitSchema(options), cursor: v2CursorSchema() } +} + /** * The v2 `search` term: a case-insensitive substring match on the resource's * natural name field. Bounded at 200 characters — a longer term cannot match diff --git a/apps/sim/lib/api/contracts/v2/skills.ts b/apps/sim/lib/api/contracts/v2/skills.ts index c307677993a..31a182d3b67 100644 --- a/apps/sim/lib/api/contracts/v2/skills.ts +++ b/apps/sim/lib/api/contracts/v2/skills.ts @@ -9,6 +9,7 @@ import { defineRouteContract } from '@/lib/api/contracts/types' import { v2CursorListResponse, v2DataResponse, + v2PaginationFields, v2SearchSchema, v2SortFields, v2TimestampSchema, @@ -96,10 +97,13 @@ export const v2SkillSortFields = ['name', 'createdAt', 'updatedAt'] as const export type V2SkillSortBy = (typeof v2SkillSortFields)[number] -export const v2ListSkillsQuerySchema = v2SkillWorkspaceQuerySchema.extend({ - search: v2SearchSchema.describe('Case-insensitive substring match against the skill name.'), - ...v2SortFields(v2SkillSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), -}) +export const v2ListSkillsQuerySchema = v2SkillWorkspaceQuerySchema + .extend({ + search: v2SearchSchema.describe('Case-insensitive substring match against the skill name.'), + ...v2SortFields(v2SkillSortFields, { sortBy: 'createdAt', sortOrder: 'desc' }), + ...v2PaginationFields({ description: 'Maximum skills to return per page.' }), + }) + .strict() export type V2ListSkillsQuery = z.output @@ -143,9 +147,11 @@ export const v2UpdateSkillBodySchema = z export type V2UpdateSkillBody = z.input /** - * Skill list. The per-workspace set is small and bounded, so the full set is - * returned as a single page (`nextCursor` is always `null`); the canonical - * cursor envelope keeps the v2 list surface uniform. + * Skill list, paginated by an opaque offset cursor rather than the keyset the + * other v2 lists use: the list merges the code-only built-in skills — which + * have no DB row for a SQL cursor predicate to act on — with the workspace's + * rows and re-sorts the result in memory, so no keyset over `skill` columns can + * name a position inside that merged sequence. */ export const v2ListSkillsContract = defineRouteContract({ method: 'GET', diff --git a/apps/sim/lib/api/contracts/v2/tables.ts b/apps/sim/lib/api/contracts/v2/tables.ts index a84d0306321..37480e4275c 100644 --- a/apps/sim/lib/api/contracts/v2/tables.ts +++ b/apps/sim/lib/api/contracts/v2/tables.ts @@ -51,6 +51,7 @@ import { v2FolderPathSchema, v2FolderSchema, v2ListFoldersQuerySchema, + v2PaginationFields, v2RelocateFolderBodySchema, v2SearchSchema, v2SortFields, @@ -339,31 +340,24 @@ export const v2ListTablesQuerySchema = z .describe('Restrict results to tables in this folder.'), search: v2SearchSchema, ...v2SortFields(v2TableSortFields, { sortBy: 'createdAt', sortOrder: 'asc' }), - /** - * The `.transform()` clamp erases every bound from `z.toJSONSchema`, which - * published a bare `type: number` while the description promised a 1–1000 - * range. `.meta()` restores the JSON Schema keywords without touching the - * runtime pipeline: the published schema states the supported domain, and - * the server stays deliberately lenient about inputs outside it. - */ - limit: z.coerce - .number() - .optional() - .default(100) - .transform((v) => Math.min(Math.max(1, Math.trunc(v)), 1000)) - .describe( - 'Maximum tables to return (1-1000). Fractional or out-of-range values are truncated and clamped into that range rather than rejected.' - ) - .meta({ type: 'integer', minimum: 1, maximum: 1000 }), - cursor: z.string().min(1).optional().describe('Opaque cursor from the previous page.'), + ...v2PaginationFields({ + max: 1000, + fallback: 100, + outOfRange: 'clamp', + description: 'Maximum tables to return per page.', + }), }) .strict() export type V2ListTablesQuery = z.output -export const v2TableWorkspaceQuerySchema = v1ListTablesQuerySchema.extend({ - workspaceId: v1ListTablesQuerySchema.shape.workspaceId.describe('Workspace that owns the table.'), -}) +export const v2TableWorkspaceQuerySchema = v1ListTablesQuerySchema + .extend({ + workspaceId: v1ListTablesQuerySchema.shape.workspaceId.describe( + 'Workspace that owns the table.' + ), + }) + .strict() export type V2TableWorkspaceQuery = z.output const v2TableColumnInputShape = { @@ -671,6 +665,7 @@ export const v2TableRowsQuerySchema = tableRowsQueryBaseSchema .optional() .describe('Opaque cursor returned by the previous page.'), }) + .strict() export type V2TableRowsQuery = z.output /** Cursor-paginated row list. */ diff --git a/apps/sim/lib/api/contracts/v2/workflows.ts b/apps/sim/lib/api/contracts/v2/workflows.ts index 2eb1131dd71..f8dc403eb4d 100644 --- a/apps/sim/lib/api/contracts/v2/workflows.ts +++ b/apps/sim/lib/api/contracts/v2/workflows.ts @@ -25,6 +25,7 @@ import { v2FolderPathSchema, v2FolderSchema, v2ListFoldersQuerySchema, + v2PaginationFields, v2RelocateFolderBodySchema, v2SearchSchema, v2SortFields, @@ -135,17 +136,7 @@ export const v2ListWorkflowsQuerySchema = z .optional() .default(false) .describe('Return only workflows with an active deployment when true.'), - limit: z.coerce - .number() - .min(1) - .max(100) - .optional() - .default(50) - .describe('Maximum workflows to return per page.'), - cursor: z - .string() - .optional() - .describe('Opaque pagination cursor returned by a previous request.'), + ...v2PaginationFields({ description: 'Maximum workflows to return per page.' }), search: v2SearchSchema, ...v2SortFields(v2WorkflowSortFields, { sortBy: 'position', sortOrder: 'asc' }), }) @@ -574,19 +565,9 @@ export type V2WorkflowVersion = z.output */ export const v2ListWorkflowVersionsQuerySchema = z .object({ - limit: z.coerce - .number() - .int() - .min(1) - .max(100) - .optional() - .default(50) - .describe('Maximum deployment versions to return per page.'), - cursor: z - .string() - .optional() - .describe('Opaque pagination cursor returned by a previous request.'), + ...v2PaginationFields({ description: 'Maximum deployment versions to return per page.' }), }) + .strict() .meta({ id: 'ListWorkflowVersionsQuery', title: 'List workflow versions query', @@ -1038,19 +1019,7 @@ export const v2ListWorkflowRunsQuerySchema = z .datetime() .optional() .describe('Include runs started at or before this ISO 8601 timestamp.'), - limit: z.coerce - .number() - .int() - .min(1) - .max(100) - .optional() - .default(50) - .describe('Maximum workflow runs to return per page.'), - cursor: z - .string() - .min(1, 'cursor cannot be empty') - .optional() - .describe('Opaque pagination cursor returned by a previous request.'), + ...v2PaginationFields({ description: 'Maximum workflow runs to return per page.' }), /** * Deliberate deviation from the v2 `sortBy` + `sortOrder` convention. Runs * have exactly one sortable column (start time), so there is no `sortBy` to diff --git a/apps/sim/lib/api/contracts/v2/workspaces.ts b/apps/sim/lib/api/contracts/v2/workspaces.ts index 761c54ddb78..6624ff5b075 100644 --- a/apps/sim/lib/api/contracts/v2/workspaces.ts +++ b/apps/sim/lib/api/contracts/v2/workspaces.ts @@ -4,6 +4,7 @@ import { defineRouteContract } from '@/lib/api/contracts/types' import { v2CursorListResponse, v2DataResponse, + v2PaginationFields, v2TimestampSchema, } from '@/lib/api/contracts/v2/shared' @@ -58,15 +59,7 @@ export type V2WorkspaceMember = z.output export const v2ListWorkspaceMembersQuerySchema = z .object({ - limit: z.coerce - .number() - .int() - .min(1) - .max(100) - .optional() - .default(50) - .describe('Maximum members to return. Defaults to 50 and cannot exceed 100.'), - cursor: z.string().min(1).optional().describe('Opaque cursor returned by the preceding page.'), + ...v2PaginationFields({ description: 'Maximum members to return per page.' }), }) .strict() export type V2ListWorkspaceMembersQuery = z.output diff --git a/apps/sim/lib/api/list-convention.test.ts b/apps/sim/lib/api/list-convention.test.ts index fcb2594dc26..83640d97c87 100644 --- a/apps/sim/lib/api/list-convention.test.ts +++ b/apps/sim/lib/api/list-convention.test.ts @@ -126,7 +126,7 @@ const CASES: ListCase[] = [ sort: { sortBy: 'name', sortOrder: 'asc', - columns: [schemaMock.knowledgeBase.name, schemaMock.knowledgeBase.createdAt], + columns: [schemaMock.knowledgeBase.name, schemaMock.knowledgeBase.id], }, }, { diff --git a/apps/sim/lib/api/list-keyset-paging.test.ts b/apps/sim/lib/api/list-keyset-paging.test.ts new file mode 100644 index 00000000000..b22011f8769 --- /dev/null +++ b/apps/sim/lib/api/list-keyset-paging.test.ts @@ -0,0 +1,173 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + type CursorKey, + encodeKeyset, + type KeysetKey, + type ListSortOrder, +} from '@/lib/api/list-query' +import { cursorSortKey, decodeSortedCursor, encodeSortedCursor } from '@/app/api/v2/lib/response' + +/** + * Page-boundary correctness for the keyset every v2 cursor list shares. + * + * `keysetAfter` renders SQL, so it cannot be evaluated here. What *can* be + * evaluated — and is the part that actually decides whether a page boundary + * loses or repeats a row — is the algebra the SQL implements: rows are ordered + * by the key tuple, a page is the first `limit` of them, and the next page is + * every row whose tuple is strictly greater than the last emitted row's. This + * models exactly that, over the same `KeysetKey.encode` functions the real + * cursors are minted from. + * + * The dataset is deliberately degenerate — repeated names and a timestamp + * shared by four rows — because that is the only place the property can fail. A + * keyset whose trailing key is not unique cannot separate tied rows, so the + * "strictly after" step either skips them or serves them twice. + */ + +interface Row { + id: string + name: string + createdAt: Date +} + +/** A `KeysetKey` reduced to what this simulation needs: how to read its value. */ +type EncodeOnly = Pick, 'encode'> + +const SHARED_INSTANT = new Date('2024-03-01T12:00:00.000Z') + +const ROWS: Row[] = [ + { id: 'a', name: 'alpha', createdAt: SHARED_INSTANT }, + { id: 'b', name: 'alpha', createdAt: SHARED_INSTANT }, + { id: 'c', name: 'alpha', createdAt: SHARED_INSTANT }, + { id: 'd', name: 'beta', createdAt: SHARED_INSTANT }, + { id: 'e', name: 'beta', createdAt: new Date('2024-03-02T09:30:00.000Z') }, + { id: 'f', name: 'gamma', createdAt: new Date('2024-03-03T00:00:00.000Z') }, + { id: 'g', name: 'delta', createdAt: new Date('2024-03-04T18:45:00.000Z') }, +] + +const idKey: EncodeOnly = { encode: (row) => row.id } +const nameKey: EncodeOnly = { encode: (row) => row.name } +const createdAtKey: EncodeOnly = { encode: (row) => row.createdAt.toISOString() } + +/** The trailing `id` is what the production sorts add; the third omits it. */ +const SORTS = { + name: [nameKey, idKey], + createdAt: [createdAtKey, idKey], + /** Deliberately non-total — used only to prove this test can fail. */ + nameWithoutTiebreaker: [nameKey], +} satisfies Record + +function compareKeys(a: CursorKey[], b: CursorKey[]): number { + for (const [i, left] of a.entries()) { + const right = b[i] + if (left < right) return -1 + if (left > right) return 1 + } + return 0 +} + +function ordered(rows: Row[], keys: EncodeOnly[], order: ListSortOrder): Row[] { + const direction = order === 'asc' ? 1 : -1 + return [...rows].sort( + (a, b) => + direction * + compareKeys( + encodeKeyset(keys as KeysetKey[], a), + encodeKeyset(keys as KeysetKey[], b) + ) + ) +} + +/** One page: the first `limit` rows strictly after `cursorKeys` in sort order. */ +function page( + keys: EncodeOnly[], + order: ListSortOrder, + limit: number, + cursorKeys?: CursorKey[] +): { data: Row[]; nextCursorKeys: CursorKey[] | null } { + const direction = order === 'asc' ? 1 : -1 + const remaining = ordered(ROWS, keys, order).filter( + (row) => + !cursorKeys || + direction * compareKeys(encodeKeyset(keys as KeysetKey[], row), cursorKeys) > 0 + ) + const data = remaining.slice(0, limit) + const last = data.at(-1) + return { + data, + nextCursorKeys: + remaining.length > limit && last ? encodeKeyset(keys as KeysetKey[], last) : null, + } +} + +/** Walks every page, capped so a cursor that fails to advance cannot hang the suite. */ +function walk(keys: EncodeOnly[], order: ListSortOrder, limit: number): Row[] { + const seen: Row[] = [] + let cursorKeys: CursorKey[] | undefined + for (let guard = 0; guard <= ROWS.length + 1; guard++) { + const result = page(keys, order, limit, cursorKeys) + seen.push(...result.data) + if (result.nextCursorKeys === null) return seen + cursorKeys = result.nextCursorKeys + } + throw new Error('pagination did not terminate') +} + +describe('v2 keyset page boundaries', () => { + const SORT_CASES = [ + ['name', SORTS.name], + ['createdAt', SORTS.createdAt], + ] as const + const ORDERS: ListSortOrder[] = ['asc', 'desc'] + + for (const [sortName, keys] of SORT_CASES) { + for (const order of ORDERS) { + for (const limit of [1, 2, 3, ROWS.length]) { + it(`emits every row exactly once paging ${sortName} ${order} at limit ${limit}`, () => { + const seen = walk(keys, order, limit) + + expect(seen).toHaveLength(ROWS.length) + expect(new Set(seen.map((row) => row.id)).size).toBe(ROWS.length) + expect(seen.map((row) => row.id)).toEqual(ordered(ROWS, keys, order).map((row) => row.id)) + }) + } + } + } + + it('reports a null nextCursor on the final page and not before', () => { + expect(page(SORTS.name, 'asc', 3).nextCursorKeys).not.toBeNull() + + const lastPage = page(SORTS.name, 'asc', 3, page(SORTS.name, 'asc', 3).nextCursorKeys ?? []) + expect(lastPage.nextCursorKeys).not.toBeNull() + + expect(page(SORTS.name, 'asc', ROWS.length).nextCursorKeys).toBeNull() + }) + + /** + * Proves the harness above is capable of failing. Without a unique trailing + * key the three rows sharing `name: 'alpha'` cannot be separated, so resuming + * "strictly after alpha" drops the other two. + */ + it('loses rows when the keyset has no unique trailing key', () => { + const seen = walk(SORTS.nameWithoutTiebreaker, 'asc', 1) + + expect(seen.length).toBeLessThan(ROWS.length) + }) + + it('round-trips a cursor only under the sort that minted it', () => { + const keys = encodeKeyset(SORTS.name as KeysetKey[], ROWS[0]) + const cursor = encodeSortedCursor(cursorSortKey('name', 'asc'), keys) + + expect(decodeSortedCursor(cursor, cursorSortKey('name', 'asc'))).toEqual({ + status: 'ok', + keys, + }) + expect(decodeSortedCursor(cursor, cursorSortKey('name', 'desc'))).toEqual({ status: 'invalid' }) + expect(decodeSortedCursor(cursor, cursorSortKey('createdAt', 'asc'))).toEqual({ + status: 'invalid', + }) + }) +}) diff --git a/apps/sim/lib/api/list-query.ts b/apps/sim/lib/api/list-query.ts index 6607ecde36a..d45d28dacf0 100644 --- a/apps/sim/lib/api/list-query.ts +++ b/apps/sim/lib/api/list-query.ts @@ -12,6 +12,7 @@ import { type SQLWrapper, sql, } from 'drizzle-orm' +import { OrchestrationError } from '@/lib/core/orchestration/types' export const LIST_SORT_ORDERS = ['asc', 'desc'] as const export type ListSortOrder = (typeof LIST_SORT_ORDERS)[number] @@ -147,6 +148,56 @@ export function encodeKeyset(keys: readonly KeysetKey[], row: Row): Cu return keys.map((key) => key.encode(row)) } +/** + * The `WHERE` fragment that resumes a page, or `undefined` for page one. + * + * Wraps {@link keysetAfter}'s `null` — a cursor that does not fit this sort — in + * the canonical validation failure, so every keyset list renders a bad cursor as + * the same 400 instead of each deciding for itself. A cursor is caller-supplied, + * so the one outcome that must never happen is it reaching the query and + * surfacing as a 500. + */ +export function resumeKeyset( + keys: readonly KeysetKey[], + cursorKeys: CursorKey[] | undefined, + order: ListSortOrder +): SQL | undefined { + if (!cursorKeys) return undefined + const after = keysetAfter(keys, cursorKeys, order) + if (after === null) throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) + return after +} + +/** One page of a keyset list, plus the keys that resume it. */ +export interface KeysetPage { + data: Row[] + nextCursorKeys: CursorKey[] | null +} + +/** + * Cuts an over-fetched row set down to the requested page. + * + * Pair with `.limit(limit + 1)`: the extra row is how "is there a next page" + * is answered without a second count query. Writing the `rows.length > limit` + * comparison out per resource is how an off-by-one becomes a page that repeats + * its last row, so it lives here once. + * + * `limit` is optional because several of these readers are also called by + * unpaged internal surfaces that want the whole set; an absent `limit` means no + * `LIMIT` clause was applied, and such a read can never carry a cursor. + */ +export function keysetPage( + keys: readonly KeysetKey[], + rows: Row[], + limit: number | undefined +): KeysetPage { + if (limit === undefined) return { data: rows, nextCursorKeys: null } + const hasMore = rows.length > limit + const data = rows.slice(0, limit) + const last = data.at(-1) + return { data, nextCursorKeys: hasMore && last ? encodeKeyset(keys, last) : null } +} + /** * The `WHERE` half of the keyset: strictly after `values` in the requested * direction, expanded lexicographically so ties on a leading key fall through diff --git a/apps/sim/lib/api/server/routes/index.ts b/apps/sim/lib/api/server/routes/index.ts index 9c72405f0b9..904f8cd1731 100644 --- a/apps/sim/lib/api/server/routes/index.ts +++ b/apps/sim/lib/api/server/routes/index.ts @@ -23,6 +23,7 @@ export { admitOptionalV2Request, admitV2Request, defineV2JsonRoute, + V2_PARSE_DEFAULTS, type V2ErrorPolicy, V2RouteInfrastructureError, v2ApiKeyAuth, diff --git a/apps/sim/lib/api/server/routes/v2-binary-route.ts b/apps/sim/lib/api/server/routes/v2-binary-route.ts index 67cc35af1f6..1802816a19a 100644 --- a/apps/sim/lib/api/server/routes/v2-binary-route.ts +++ b/apps/sim/lib/api/server/routes/v2-binary-route.ts @@ -8,11 +8,11 @@ import type { } from '@/lib/api/server/routes/types' import { admitV2Request, + V2_PARSE_DEFAULTS, type V2ErrorPolicy, type V2RateLimitPolicy, V2RouteInfrastructureError, type v2ApiKeyAuth, - v2PayloadTooLargeResponse, } from '@/lib/api/server/routes/v2-json-route' import { parseRequest } from '@/lib/api/server/validation' import type { ApplicationOperation } from '@/lib/core/application' @@ -59,7 +59,7 @@ export function defineV2BinaryRoute< if (!admission.success) return admission.response const parsed = await parseRequest(options.contract, request, context ?? {}, { - payloadTooLargeResponse: v2PayloadTooLargeResponse, + ...V2_PARSE_DEFAULTS, validationErrorResponse: v2ValidationError, }) if (!parsed.success) return parsed.response diff --git a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts index a6c415ec493..79767ef4641 100644 --- a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts +++ b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts @@ -9,11 +9,11 @@ import type { } from '@/lib/api/server/routes/types' import { admitV2Request, + V2_PARSE_DEFAULTS, type V2ErrorPolicy, type V2RateLimitPolicy, V2RouteInfrastructureError, type v2ApiKeyAuth, - v2PayloadTooLargeResponse, } from '@/lib/api/server/routes/v2-json-route' import { type ParsedRequest, @@ -129,7 +129,7 @@ export function defineV2BodyLifecycleRoute< if (!routeAdmission.success) return routeAdmission.response const parsed = await parseRequest(options.contract, request, context ?? {}, { - payloadTooLargeResponse: v2PayloadTooLargeResponse, + ...V2_PARSE_DEFAULTS, ...options.parseOptions, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/lib/api/server/routes/v2-error-envelope.test.ts b/apps/sim/lib/api/server/routes/v2-error-envelope.test.ts new file mode 100644 index 00000000000..ceab810f8b5 --- /dev/null +++ b/apps/sim/lib/api/server/routes/v2-error-envelope.test.ts @@ -0,0 +1,38 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { v2InvalidJsonResponse } from '@/lib/api/server/routes/v2-json-route' + +/** + * v2 promises exactly one failure shape — `{ error: { code, message } }` — and a + * client written against it reads `error.code`. That promise held for validation + * errors and broke for transport-level ones: a malformed or absent JSON body + * fell through to `parseRequest`'s framework default, a bare + * `{ "error": "Request body must be valid JSON" }` string, so `error.code` was + * `undefined` exactly when the request was malformed. + * + * The fix made the envelope a builder default rather than a per-route opt-in + * that only 8 of 77 routes remembered, so this asserts the default itself. + */ +describe('v2 malformed-body envelope', () => { + it('renders the canonical error envelope, not a bare string', async () => { + const response = v2InvalidJsonResponse() + + expect(response.status).toBe(400) + expect(response.headers.get('content-type')).toContain('application/json') + expect(await response.json()).toEqual({ + error: { code: 'BAD_REQUEST', message: 'Request body must be valid JSON' }, + }) + }) + + it('keeps the body out of shared caches like every other v2 response', () => { + expect(v2InvalidJsonResponse().headers.get('Cache-Control')).toBe('private, no-store') + }) + + it('exposes a machine-readable code, which the bare-string form could not', async () => { + const body = (await v2InvalidJsonResponse().json()) as { error: { code?: string } } + expect(typeof body.error).toBe('object') + expect(body.error.code).toBe('BAD_REQUEST') + }) +}) diff --git a/apps/sim/lib/api/server/routes/v2-json-route.ts b/apps/sim/lib/api/server/routes/v2-json-route.ts index 20261375568..960aa46415c 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.ts @@ -111,6 +111,33 @@ export const v2RateLimits = { export const v2PayloadTooLargeResponse = () => v2Error('PAYLOAD_TOO_LARGE', 'Request body is too large') +/** + * Default `400` for a body that is absent or not valid JSON, for the same + * reason as {@link v2PayloadTooLargeResponse}: `parseRequest`'s fallback is a + * bare `{ "error": "Request body must be valid JSON" }` carrying no + * `error.code`, so a client reading `error.code` off every other v2 failure + * gets `undefined` exactly when its request was malformed. + * + * It is a default rather than a per-route opt-in because the opt-in *was* the + * bug: only 8 of the 77 v2 routes remembered to pass it, so the envelope held + * for validation errors and broke for transport-level ones. A route supplying + * its own `invalidJsonResponse` still wins. + */ +export const v2InvalidJsonResponse = () => v2Error('BAD_REQUEST', 'Request body must be valid JSON') + +/** + * The parse failures every v2 route renders the same way. + * + * The builders spread this, and so must the handful of raw `withRouteHandler` + * v2 routes that call `parseRequest` directly — they are exactly the routes a + * builder default cannot reach, and leaving them out is what kept the bare + * `{ "error": string }` body alive on two of the busiest v2 POSTs. + */ +export const V2_PARSE_DEFAULTS = { + payloadTooLargeResponse: v2PayloadTooLargeResponse, + invalidJsonResponse: v2InvalidJsonResponse, +} as const + export interface V2ErrorPolicy { render(error: unknown): NextResponse | null } @@ -250,7 +277,7 @@ export function defineV2JsonRoute< } const parsed = await parseRequest(options.contract, request, context ?? {}, { - payloadTooLargeResponse: v2PayloadTooLargeResponse, + ...V2_PARSE_DEFAULTS, ...options.parseOptions, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/lib/credentials/application/list-workspace-credentials.ts b/apps/sim/lib/credentials/application/list-workspace-credentials.ts index 2f92c8f1a86..45f2eee5c67 100644 --- a/apps/sim/lib/credentials/application/list-workspace-credentials.ts +++ b/apps/sim/lib/credentials/application/list-workspace-credentials.ts @@ -1,3 +1,4 @@ +import type { CursorKey, ListSortOrder } from '@/lib/api/list-query' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { credentialOperations } from '@/lib/credentials/application/operations' @@ -15,11 +16,20 @@ export interface ListWorkspaceCredentialsInput { providerId?: string search?: string sortBy: 'displayName' | 'createdAt' | 'updatedAt' - sortOrder: 'asc' | 'desc' + sortOrder: ListSortOrder + limit: number + cursorKeys?: CursorKey[] } export interface ListWorkspaceCredentialsResult { credentials: VisibleWorkspaceCredential[] + nextCursorKeys: CursorKey[] | null + /** + * Echoed back because the route's presenter receives only this result, and the + * cursor it hands out has to be stamped with the sort that produced it. + */ + sortBy: ListWorkspaceCredentialsInput['sortBy'] + sortOrder: ListSortOrder } export const listWorkspaceCredentials = defineAuthorizedWorkspaceUseCase({ @@ -34,34 +44,36 @@ export const listWorkspaceCredentials = defineAuthorizedWorkspaceUseCase({ const types: Array<'oauth' | 'service_account'> = input.type ? [input.type] : ['oauth', 'service_account'] + const sort = { sortBy: input.sortBy, sortOrder: input.sortOrder } + if (principal.kind === 'workspace_api_key') { - return { - credentials: await listWorkspacePrincipalCredentials({ - workspaceId: context.workspaceId, - types, - providerId: input.providerId, - search: input.search, - sortBy: input.sortBy, - sortOrder: input.sortOrder, - }), - } + const page = await listWorkspacePrincipalCredentials({ + workspaceId: context.workspaceId, + types, + providerId: input.providerId, + search: input.search, + ...sort, + limit: input.limit, + cursorKeys: input.cursorKeys, + }) + return { credentials: page.data, nextCursorKeys: page.nextCursorKeys, ...sort } } const workspaceAccess = await checkWorkspaceAccess(context.workspaceId, principal.userId) if (!workspaceAccess.hasAccess) { throw new OrchestrationError('forbidden', 'Access denied') } - return { - credentials: await listVisibleWorkspaceCredentials({ - workspaceId: context.workspaceId, - userId: principal.userId, - workspaceAccess, - types, - providerId: input.providerId, - search: input.search, - sortBy: input.sortBy, - sortOrder: input.sortOrder, - }), - } + const page = await listVisibleWorkspaceCredentials({ + workspaceId: context.workspaceId, + userId: principal.userId, + workspaceAccess, + types, + providerId: input.providerId, + search: input.search, + ...sort, + limit: input.limit, + cursorKeys: input.cursorKeys, + }) + return { credentials: page.data, nextCursorKeys: page.nextCursorKeys, ...sort } }, }) diff --git a/apps/sim/lib/credentials/queries.test.ts b/apps/sim/lib/credentials/queries.test.ts index d4391d06206..cc5dfb3a368 100644 --- a/apps/sim/lib/credentials/queries.test.ts +++ b/apps/sim/lib/credentials/queries.test.ts @@ -11,7 +11,7 @@ describe('listWorkspacePrincipalCredentials', () => { }) it('selects and returns only connection metadata', async () => { - dbChainMockFns.orderBy.mockResolvedValueOnce([ + dbChainMockFns.limit.mockResolvedValueOnce([ { id: 'credential-1', workspaceId: 'workspace-1', @@ -30,9 +30,11 @@ describe('listWorkspacePrincipalCredentials', () => { const result = await listWorkspacePrincipalCredentials({ workspaceId: 'workspace-1', types: ['oauth', 'service_account'], + limit: 50, }) - expect(result).toEqual([ + expect(result.nextCursorKeys).toBeNull() + expect(result.data).toEqual([ { id: 'credential-1', workspaceId: 'workspace-1', @@ -57,19 +59,20 @@ describe('listWorkspacePrincipalCredentials', () => { it('fails fast on an empty connection-type policy', async () => { await expect( - listWorkspacePrincipalCredentials({ workspaceId: 'workspace-1', types: [] }) + listWorkspacePrincipalCredentials({ workspaceId: 'workspace-1', types: [], limit: 50 }) ).rejects.toThrow('Workspace credential types cannot be empty') expect(dbChainMockFns.select).not.toHaveBeenCalled() }) it('propagates database failures', async () => { const failure = new Error('database unavailable') - dbChainMockFns.orderBy.mockRejectedValueOnce(failure) + dbChainMockFns.limit.mockRejectedValueOnce(failure) await expect( listWorkspacePrincipalCredentials({ workspaceId: 'workspace-1', types: ['oauth', 'service_account'], + limit: 50, }) ).rejects.toBe(failure) }) diff --git a/apps/sim/lib/credentials/queries.ts b/apps/sim/lib/credentials/queries.ts index 9724fadd410..bacb9d2cda7 100644 --- a/apps/sim/lib/credentials/queries.ts +++ b/apps/sim/lib/credentials/queries.ts @@ -1,9 +1,20 @@ import { db } from '@sim/db' import { credential, credentialMember } from '@sim/db/schema' -import { and, type Column, eq, inArray, isNotNull, or, sql } from 'drizzle-orm' +import { and, eq, inArray, isNotNull, or, sql } from 'drizzle-orm' import type { V2CredentialSortBy } from '@/lib/api/contracts/v2/credentials' -import type { ListSortOrder } from '@/lib/api/list-query' -import { listOrderBy, searchFilter } from '@/lib/api/list-query' +import { + type CursorKey, + type KeysetKey, + type KeysetPage, + keysetColumns, + keysetPage, + type ListSortOrder, + listOrderBy, + resumeKeyset, + searchFilter, + textKey, + timestampKey, +} from '@/lib/api/list-query' import { isSharedCredentialType, SHARED_CREDENTIAL_TYPES } from '@/lib/credentials/access' import type { WorkspaceAccess } from '@/lib/workspaces/permissions/utils' @@ -31,6 +42,38 @@ export interface VisibleWorkspaceCredential { role: 'admin' | 'member' } +const credentialIdKey = textKey(credential.id, (row) => row.id) + +/** + * Keyset orderings for the public list's sortable fields, made total over the + * contract enum by `satisfies`. Each ends in `id` so credentials sharing a + * display name or a timestamp still come back in a stable order — which is also + * what makes the cursor resumable, since a non-unique final key can repeat or + * skip a row at a page boundary. + * + * The keys encode from {@link VisibleWorkspaceCredential} — the mapped row, not + * the raw select — because both readers below derive their projection before + * the page is cut, so the cursor is always stamped from the row the caller + * actually received. + */ +const CREDENTIAL_SORTS = { + displayName: [ + textKey(credential.displayName, (row) => row.displayName), + credentialIdKey, + ], + createdAt: [ + timestampKey(credential.createdAt, (row) => row.createdAt), + credentialIdKey, + ], + updatedAt: [ + timestampKey(credential.updatedAt, (row) => row.updatedAt), + credentialIdKey, + ], +} satisfies Record[]> + +/** One page of workspace credentials plus the keys that resume it. */ +export type WorkspaceCredentialPage = KeysetPage + /** * The credentials a user may see in a workspace. * @@ -38,17 +81,6 @@ export interface VisibleWorkspaceCredential { * admins — every shared-type credential, plus the caller's own personal env * credentials. Encrypted secret material is never selected. */ -/** - * Orderings for the public list's sortable fields, made total over the contract - * enum by `satisfies`. Each ends in `id` so credentials sharing a display name - * or a timestamp still come back in a stable order. - */ -const CREDENTIAL_SORTS = { - displayName: [credential.displayName, credential.id], - createdAt: [credential.createdAt, credential.id], - updatedAt: [credential.updatedAt, credential.id], -} satisfies Record - export async function listVisibleWorkspaceCredentials(params: { workspaceId: string userId: string @@ -59,7 +91,22 @@ export async function listVisibleWorkspaceCredentials(params: { search?: string sortBy?: V2CredentialSortBy sortOrder?: ListSortOrder -}): Promise { + /** Page size. Omitted by the unpaged session surface — see {@link keysetPage}. */ + limit?: number + cursorKeys?: CursorKey[] + /** + * Restricts personal env credentials to the caller's own, in SQL. + * + * The secrets surface used to apply this as a JS filter over the query's + * result. That is invisible on an unpaged read but a correctness bug once the + * query is paged: a page of `limit` rows trimmed afterwards hands the caller + * fewer rows than it asked for while `nextCursor` still claims more. It is a + * pure page-size fix — the predicate drops exactly the rows the JS filter + * dropped (another user's `env_personal` row, reachable only via an explicit + * `credential_member` grant), and never widens what a caller can see. + */ + ownedEnvSecretsOnly?: boolean +}): Promise { const { workspaceId, userId, @@ -69,11 +116,21 @@ export async function listVisibleWorkspaceCredentials(params: { search, sortBy = 'createdAt', sortOrder = 'desc', + limit, } = params const whereClauses = [eq(credential.workspaceId, workspaceId)] if (types?.length) whereClauses.push(inArray(credential.type, types)) if (providerId) whereClauses.push(eq(credential.providerId, providerId)) + const ownedEnvSecretsClause = params.ownedEnvSecretsOnly + ? or( + eq(credential.type, 'env_workspace'), + and(eq(credential.type, 'env_personal'), eq(credential.envOwnerUserId, userId)) + ) + : undefined + + const keys = CREDENTIAL_SORTS[sortBy] + const after = resumeKeyset(keys, params.cursorKeys, sortOrder) const isWorkspaceAdmin = workspaceAccess.canAdmin const accessClause = isWorkspaceAdmin @@ -84,7 +141,7 @@ export async function listVisibleWorkspaceCredentials(params: { ) : or(isNotNull(credentialMember.id), eq(credential.envOwnerUserId, userId)) - const rows = await db + const query = db .select({ id: credential.id, workspaceId: credential.workspaceId, @@ -110,20 +167,34 @@ export async function listVisibleWorkspaceCredentials(params: { eq(credentialMember.status, 'active') ) ) - .where(and(...whereClauses, accessClause, searchFilter(credential.displayName, search))) - .orderBy(...listOrderBy(CREDENTIAL_SORTS[sortBy], sortOrder)) + .where( + and( + ...whereClauses, + accessClause, + ownedEnvSecretsClause, + searchFilter(credential.displayName, search), + after + ) + ) + .orderBy(...listOrderBy(keysetColumns(keys), sortOrder)) - return rows.map(({ memberRole, encryptedServiceAccountKey, ...rest }) => ({ + const rows = await (limit === undefined ? query : query.limit(limit + 1)) + + const mapped = rows.map(({ memberRole, encryptedServiceAccountKey, ...rest }) => ({ ...rest, hasServiceAccountKey: Boolean(encryptedServiceAccountKey), + /** + * An `env_personal` credential's own env owner administers it regardless of + * workspace role — otherwise the owner of a personal secret can't manage it. + */ role: - // An `env_personal` credential's own env owner administers it regardless of - // workspace role — otherwise the owner of a personal secret can't manage it. (rest.type === 'env_personal' && rest.envOwnerUserId === userId) || (isWorkspaceAdmin && isSharedCredentialType(rest.type)) - ? 'admin' - : (memberRole ?? 'member'), + ? ('admin' as const) + : (memberRole ?? ('member' as const)), })) + + return keysetPage(keys, mapped, limit) } /** @@ -141,7 +212,9 @@ export async function listWorkspacePrincipalCredentials(params: { search?: string sortBy?: V2CredentialSortBy sortOrder?: ListSortOrder -}): Promise { + limit: number + cursorKeys?: CursorKey[] +}): Promise { const { workspaceId, types, @@ -149,13 +222,17 @@ export async function listWorkspacePrincipalCredentials(params: { search, sortBy = 'createdAt', sortOrder = 'desc', + limit, } = params if (types.length === 0) throw new Error('Workspace credential types cannot be empty') const whereClauses = [eq(credential.workspaceId, workspaceId), inArray(credential.type, types)] if (providerId) whereClauses.push(eq(credential.providerId, providerId)) - const rows = await db + const keys = CREDENTIAL_SORTS[sortBy] + const after = resumeKeyset(keys, params.cursorKeys, sortOrder) + + const query = db .select({ id: credential.id, workspaceId: credential.workspaceId, @@ -170,15 +247,19 @@ export async function listWorkspacePrincipalCredentials(params: { hasServiceAccountKey: sql`${credential.encryptedServiceAccountKey} IS NOT NULL`, }) .from(credential) - .where(and(...whereClauses, searchFilter(credential.displayName, search))) - .orderBy(...listOrderBy(CREDENTIAL_SORTS[sortBy], sortOrder)) + .where(and(...whereClauses, searchFilter(credential.displayName, search), after)) + .orderBy(...listOrderBy(keysetColumns(keys), sortOrder)) - return rows.map((row) => ({ + const rows = await query.limit(limit + 1) + + const mapped = rows.map((row) => ({ ...row, envKey: null, envOwnerUserId: null, - role: 'member', + role: 'member' as const, })) + + return keysetPage(keys, mapped, limit) } /** diff --git a/apps/sim/lib/custom-tools/application/use-cases.ts b/apps/sim/lib/custom-tools/application/use-cases.ts index 853e7662b4b..dd0fb4e44de 100644 --- a/apps/sim/lib/custom-tools/application/use-cases.ts +++ b/apps/sim/lib/custom-tools/application/use-cases.ts @@ -6,7 +6,7 @@ import { } from '@sim/auth/principal' import type { customTools } from '@sim/db/schema' import { getErrorMessage, getPostgresErrorCode } from '@sim/utils/errors' -import type { ListSortOrder } from '@/lib/api/list-query' +import type { CursorKey, ListSortOrder } from '@/lib/api/list-query' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { customToolDelegationPolicy } from '@/lib/custom-tools/application/authorization' @@ -92,6 +92,8 @@ export interface ListWorkspaceCustomToolsInput { search?: string sortBy?: CustomToolSortBy sortOrder?: ListSortOrder + limit: number + cursorKeys?: CursorKey[] } export const listWorkspaceCustomToolsUseCase = defineAuthorizedWorkspaceUseCase({ @@ -100,8 +102,13 @@ export const listWorkspaceCustomToolsUseCase = defineAuthorizedWorkspaceUseCase( resolveWorkspaceContext(input.workspaceId), authorizationOptions, async execute({ input, context }) { - const tools = await listWorkspaceCustomTools({ ...input, workspaceId: context.workspaceId }) - return { tools } + const page = await listWorkspaceCustomTools({ ...input, workspaceId: context.workspaceId }) + return { + tools: page.data, + nextCursorKeys: page.nextCursorKeys, + sortBy: input.sortBy ?? 'createdAt', + sortOrder: input.sortOrder ?? 'desc', + } }, }) diff --git a/apps/sim/lib/folders/application-folder-caps.test.ts b/apps/sim/lib/folders/application-folder-caps.test.ts index 7a6d2524f25..08c63d8798a 100644 --- a/apps/sim/lib/folders/application-folder-caps.test.ts +++ b/apps/sim/lib/folders/application-folder-caps.test.ts @@ -27,7 +27,6 @@ vi.mock('@/lib/workflows/application/context', () => ({ resolveActiveWorkspaceApplicationContext: mocks.resolveWorkflowWorkspace, })) vi.mock('@/lib/workflows/queries', () => ({ - InvalidWorkflowListCursorError: class InvalidWorkflowListCursorError extends Error {}, listWorkspaceWorkflows: mocks.listWorkflows, })) vi.mock('@/lib/table/application/context', () => ({ diff --git a/apps/sim/lib/knowledge/application/knowledge-bases.test.ts b/apps/sim/lib/knowledge/application/knowledge-bases.test.ts index c3318e7daa1..f3328157bae 100644 --- a/apps/sim/lib/knowledge/application/knowledge-bases.test.ts +++ b/apps/sim/lib/knowledge/application/knowledge-bases.test.ts @@ -143,7 +143,7 @@ describe('knowledge base application use cases', () => { }) mocks.loadFolderIndex.mockResolvedValue({ pathById: new Map() }) mocks.createRecord.mockResolvedValue(knowledgeBase) - mocks.listRecords.mockResolvedValue([]) + mocks.listRecords.mockResolvedValue({ data: [], nextCursorKeys: null }) mocks.listInternalRecords.mockResolvedValue([knowledgeBase]) mocks.getRecord.mockResolvedValue(knowledgeBase) mocks.getRestorableRecord.mockResolvedValue(knowledgeBase) @@ -188,7 +188,7 @@ describe('knowledge base application use cases', () => { }) it('loads the active knowledge catalog and tag metadata only after workspace authorization', async () => { - mocks.listRecords.mockResolvedValueOnce([knowledgeBase]) + mocks.listRecords.mockResolvedValueOnce({ data: [knowledgeBase], nextCursorKeys: null }) dbChainMockFns.orderBy.mockResolvedValueOnce([ { knowledgeBaseId: 'knowledge-1', diff --git a/apps/sim/lib/knowledge/application/knowledge-bases.ts b/apps/sim/lib/knowledge/application/knowledge-bases.ts index 44180f1a1b7..9fd5ba27b57 100644 --- a/apps/sim/lib/knowledge/application/knowledge-bases.ts +++ b/apps/sim/lib/knowledge/application/knowledge-bases.ts @@ -4,6 +4,7 @@ import { db } from '@sim/db' import { knowledgeBaseTagDefinitions } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { inArray } from 'drizzle-orm' +import type { CursorKey } from '@/lib/api/list-query' import { authorizeWorkspaceOperation, type OperationUseCase, @@ -69,6 +70,12 @@ export interface ListKnowledgeBasesInput { search?: string sortBy?: 'name' | 'createdAt' | 'updatedAt' sortOrder?: 'asc' | 'desc' + /** + * Page size for the public list. Omitted reads the whole workspace set, which + * is what the internal catalog caller still wants. + */ + limit?: number + cursorKeys?: CursorKey[] } export interface KnowledgeBaseResult { @@ -78,6 +85,9 @@ export interface KnowledgeBaseResult { export interface ListKnowledgeBasesResult { knowledgeBases: KnowledgeBaseResult[] + nextCursorKeys: CursorKey[] | null + sortBy: 'name' | 'createdAt' | 'updatedAt' + sortOrder: 'asc' | 'desc' } export interface ListArchivedKnowledgeBasesResult { @@ -218,29 +228,37 @@ async function executeListKnowledgeBases(args: { input: ListKnowledgeBasesInput context: KnowledgeWorkspaceContext }): Promise { - const index = await loadActiveFolderPathIndex( - args.context.workspaceId, - 'knowledge_base', - undefined, - { maxRows: MAX_KNOWLEDGE_FOLDERS_PER_WORKSPACE } - ) - const folderId = + /** + * The folder index renders each row's `folderPath` and the folder filter + * resolves the caller's `folderPath` to an id. Neither reads the other, so + * they run together rather than adding a serial round-trip to a list route. + */ + const [index, folderId] = await Promise.all([ + loadActiveFolderPathIndex(args.context.workspaceId, 'knowledge_base', undefined, { + maxRows: MAX_KNOWLEDGE_FOLDERS_PER_WORKSPACE, + }), args.input.folderPath === undefined ? undefined - : await resolveKnowledgeFolderPath(args.context.workspaceId, args.input.folderPath).then( + : resolveKnowledgeFolderPath(args.context.workspaceId, args.input.folderPath).then( (resolved) => resolved.folderId - ) - const rows = await getWorkspaceKnowledgeBases(args.context.workspaceId, 'active', { + ), + ]) + const page = await getWorkspaceKnowledgeBases(args.context.workspaceId, 'active', { folderId, search: args.input.search, sortBy: args.input.sortBy, sortOrder: args.input.sortOrder, + limit: args.input.limit, + cursorKeys: args.input.cursorKeys, }) return { - knowledgeBases: rows.map((knowledgeBase) => ({ + knowledgeBases: page.data.map((knowledgeBase) => ({ knowledgeBase, folderPath: knowledgeFolderPathForId(index, knowledgeBase.folderId), })), + nextCursorKeys: page.nextCursorKeys, + sortBy: args.input.sortBy ?? 'createdAt', + sortOrder: args.input.sortOrder ?? 'asc', } } @@ -397,7 +415,7 @@ export const listArchivedKnowledgeBases = defineAuthorizedKnowledgeUseCase({ resolveKnowledgeWorkspaceContext(input), async execute({ context }): Promise { return { - knowledgeBases: await getWorkspaceKnowledgeBases(context.workspaceId, 'archived'), + knowledgeBases: (await getWorkspaceKnowledgeBases(context.workspaceId, 'archived')).data, } }, }) diff --git a/apps/sim/lib/knowledge/application/knowledge-vfs.ts b/apps/sim/lib/knowledge/application/knowledge-vfs.ts index da538dae88e..f7fbff3e8d7 100644 --- a/apps/sim/lib/knowledge/application/knowledge-vfs.ts +++ b/apps/sim/lib/knowledge/application/knowledge-vfs.ts @@ -29,7 +29,7 @@ async function resolveKnowledgeBaseByVfsName( context: KnowledgeWorkspaceContext, sourceName: string ): Promise { - const rows = await getWorkspaceKnowledgeBases(context.workspaceId, 'active', { + const { data: rows } = await getWorkspaceKnowledgeBases(context.workspaceId, 'active', { search: sourceName, }) const matches = rows.filter((row) => row.name === sourceName) diff --git a/apps/sim/lib/knowledge/service.ts b/apps/sim/lib/knowledge/service.ts index 1ab816f87ae..d3b9ffa5e37 100644 --- a/apps/sim/lib/knowledge/service.ts +++ b/apps/sim/lib/knowledge/service.ts @@ -10,22 +10,18 @@ import { import { createLogger } from '@sim/logger' import { getPostgresErrorCode } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' -import { - and, - type Column, - count, - eq, - exists, - inArray, - isNotNull, - isNull, - ne, - or, - sql, -} from 'drizzle-orm' +import { and, count, eq, exists, inArray, isNotNull, isNull, ne, or, sql } from 'drizzle-orm' import type { V2KnowledgeBaseSortBy } from '@/lib/api/contracts/v2/knowledge' -import type { ListSortOrder } from '@/lib/api/list-query' -import { listOrderBy, searchFilter } from '@/lib/api/list-query' +import type { CursorKey, KeysetKey, ListSortOrder } from '@/lib/api/list-query' +import { + keysetColumns, + keysetPage, + listOrderBy, + resumeKeyset, + searchFilter, + textKey, + timestampKey, +} from '@/lib/api/list-query' import type { HighestPrioritySubscription } from '@/lib/billing/core/plan' import { getHighestPrioritySubscription } from '@/lib/billing/core/subscription' import { ensureUserStatsExists } from '@/lib/billing/core/usage' @@ -126,16 +122,36 @@ type KnowledgeBaseStorageMove = ownerUserId: string } +/** The columns a knowledge-base keyset orders and resumes on. */ +interface KnowledgeBaseSortRow { + id: string + name: string + createdAt: Date + updatedAt: Date +} + +const knowledgeBaseIdKey = textKey(knowledgeBase.id, (row) => row.id) + /** - * Orderings for the public list's sortable fields, made total over the contract - * enum by `satisfies`. Each ends in `createdAt` so knowledge bases sharing a - * name still come back in a stable order. + * Keyset orderings for the public list's sortable fields, made total over the + * contract enum by `satisfies`. + * + * Each ends in `id` rather than `createdAt`. `createdAt` is not unique, so it + * could not separate two knowledge bases created in the same millisecond — which + * left ties in an order the planner chose, and would let a cursor repeat or skip + * a row at a page boundary now that the list pages. */ const KNOWLEDGE_BASE_SORTS = { - name: [knowledgeBase.name, knowledgeBase.createdAt], - createdAt: [knowledgeBase.createdAt], - updatedAt: [knowledgeBase.updatedAt, knowledgeBase.createdAt], -} satisfies Record + name: [textKey(knowledgeBase.name, (row) => row.name), knowledgeBaseIdKey], + createdAt: [ + timestampKey(knowledgeBase.createdAt, (row) => row.createdAt), + knowledgeBaseIdKey, + ], + updatedAt: [ + timestampKey(knowledgeBase.updatedAt, (row) => row.updatedAt), + knowledgeBaseIdKey, + ], +} satisfies Record[]> export interface GetKnowledgeBasesOptions { /** Restrict to one knowledge-base folder; `undefined` lists all and `null` lists the root. */ @@ -144,6 +160,14 @@ export interface GetKnowledgeBasesOptions { search?: string sortBy?: V2KnowledgeBaseSortBy sortOrder?: ListSortOrder + /** + * Page size. Omitted reads the whole workspace set as one page, capped by + * {@link MAX_KNOWLEDGE_BASES_PER_WORKSPACE} — what the internal callers that + * need every row still do. + */ + limit?: number + /** Keyset to resume after, from the previous page's `nextCursorKeys`. */ + cursorKeys?: CursorKey[] } async function attachConnectorTypes( @@ -195,8 +219,23 @@ export async function getWorkspaceKnowledgeBases( workspaceId: string, scope: KnowledgeBaseScope = 'active', options?: GetKnowledgeBasesOptions -): Promise { - const { folderId, search, sortBy = 'createdAt', sortOrder = 'asc' } = options ?? {} +): Promise<{ data: KnowledgeBaseWithCounts[]; nextCursorKeys: CursorKey[] | null }> { + const { + folderId, + search, + sortBy = 'createdAt', + sortOrder = 'asc', + limit, + cursorKeys, + } = options ?? {} + const keys = KNOWLEDGE_BASE_SORTS[sortBy] + const resumeAfter = resumeKeyset(keys, cursorKeys, sortOrder) + + /** + * An unpaged read still reads one row past the cap so an oversized workspace + * is a hard failure rather than a silently truncated list. + */ + const readLimit = (limit ?? MAX_KNOWLEDGE_BASES_PER_WORKSPACE) + 1 const scopeCondition = scope === 'all' ? undefined @@ -240,26 +279,32 @@ export async function getWorkspaceKnowledgeBases( : folderId === null ? isNull(knowledgeBase.folderId) : eq(knowledgeBase.folderId, folderId), - searchFilter(knowledgeBase.name, search) + searchFilter(knowledgeBase.name, search), + resumeAfter ) ) .groupBy(knowledgeBase.id) - .orderBy(...listOrderBy(KNOWLEDGE_BASE_SORTS[sortBy], sortOrder)) - .limit(MAX_KNOWLEDGE_BASES_PER_WORKSPACE + 1) + .orderBy(...listOrderBy(keysetColumns(keys), sortOrder)) + .limit(readLimit) - if (rows.length > MAX_KNOWLEDGE_BASES_PER_WORKSPACE) { + if (limit === undefined && rows.length > MAX_KNOWLEDGE_BASES_PER_WORKSPACE) { throw new Error( `Knowledge base list exceeds the ${MAX_KNOWLEDGE_BASES_PER_WORKSPACE} row limit` ) } - return attachConnectorTypes( - rows.map((kb) => ({ - ...kb, - chunkingConfig: kb.chunkingConfig as ChunkingConfig, - docCount: Number(kb.docCount), - })) - ) + const page = keysetPage(keys, rows, limit) + + return { + data: await attachConnectorTypes( + page.data.map((kb) => ({ + ...kb, + chunkingConfig: kb.chunkingConfig as ChunkingConfig, + docCount: Number(kb.docCount), + })) + ), + nextCursorKeys: page.nextCursorKeys, + } } /** @@ -352,7 +397,7 @@ export async function getKnowledgeBases( ) ) .groupBy(knowledgeBase.id) - .orderBy(...listOrderBy(KNOWLEDGE_BASE_SORTS[sortBy], sortOrder)) + .orderBy(...listOrderBy(keysetColumns(KNOWLEDGE_BASE_SORTS[sortBy]), sortOrder)) const kbIds = knowledgeBasesWithCounts.map((kb) => kb.id) diff --git a/apps/sim/lib/secrets/application/use-cases.test.ts b/apps/sim/lib/secrets/application/use-cases.test.ts index 1bb47279bd4..dd2abd92a3f 100644 --- a/apps/sim/lib/secrets/application/use-cases.test.ts +++ b/apps/sim/lib/secrets/application/use-cases.test.ts @@ -82,7 +82,7 @@ describe('secret application use cases', () => { mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false }) mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() }) mocks.setWorkspace.mockResolvedValue({ created: true }) - mocks.listCredentials.mockResolvedValue([secret]) + mocks.listCredentials.mockResolvedValue({ data: [secret], nextCursorKeys: null }) }) it('rejects workspace keys before resolving or reading secret state', async () => { diff --git a/apps/sim/lib/secrets/application/use-cases.ts b/apps/sim/lib/secrets/application/use-cases.ts index abd1b5c27ce..2fa1f1a5a07 100644 --- a/apps/sim/lib/secrets/application/use-cases.ts +++ b/apps/sim/lib/secrets/application/use-cases.ts @@ -1,6 +1,6 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import type { Principal } from '@sim/auth/principal' -import type { ListSortOrder } from '@/lib/api/list-query' +import type { CursorKey, ListSortOrder } from '@/lib/api/list-query' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { getWorkspaceEnvKeyAdminAccess } from '@/lib/credentials/environment' @@ -46,6 +46,18 @@ function credentialTypes(scope?: SecretScope) { return ['env_workspace', 'env_personal'] as const } +/** + * Reads secret metadata for the caller. + * + * `ownedEnvSecretsOnly` is what keeps another user's personal secret out of the + * result. It used to be a JS filter applied after the query; it lives in SQL now + * because trimming rows after the page is cut would hand the caller fewer than + * `limit` rows while `nextCursor` still promised more. + * + * The secret's public `name` is stored as the credential `displayName`, so the + * caller-facing `name` sort aliases to that column here. The alias must not + * escape into the cursor's sort stamp — see {@link listSecretsUseCase}. + */ async function listSecretMetadata(params: { workspaceId: string userId: string @@ -53,9 +65,11 @@ async function listSecretMetadata(params: { search?: string sortBy: SecretSortBy sortOrder: ListSortOrder -}): Promise { + limit?: number + cursorKeys?: CursorKey[] +}): Promise<{ data: VisibleWorkspaceCredential[]; nextCursorKeys: CursorKey[] | null }> { const workspaceAccess = await checkWorkspaceAccess(params.workspaceId, params.userId) - const rows = await listVisibleWorkspaceCredentials({ + return listVisibleWorkspaceCredentials({ workspaceId: params.workspaceId, userId: params.userId, workspaceAccess, @@ -63,8 +77,10 @@ async function listSecretMetadata(params: { search: params.search, sortBy: params.sortBy === 'name' ? 'displayName' : params.sortBy, sortOrder: params.sortOrder, + limit: params.limit, + cursorKeys: params.cursorKeys, + ownedEnvSecretsOnly: true, }) - return rows.filter((row) => row.type === 'env_workspace' || row.envOwnerUserId === params.userId) } async function requireWorkspaceSecretMutationAccess(params: { @@ -101,13 +117,13 @@ async function getSecretMetadata(params: { scope: SecretScope name: string }): Promise { - const rows = await listSecretMetadata({ + const { data } = await listSecretMetadata({ ...params, search: params.name, sortBy: 'name', sortOrder: 'asc', }) - const row = rows.find( + const row = data.find( (candidate) => candidate.envKey === params.name && (params.scope === 'workspace' @@ -126,8 +142,19 @@ export interface ListSecretsInput { search?: string sortBy: SecretSortBy sortOrder: ListSortOrder + limit: number + cursorKeys?: CursorKey[] } +/** + * Lists secret metadata as one keyset page. + * + * `sortBy`/`sortOrder` are echoed back in the caller's own vocabulary — `name`, + * not the `displayName` column it aliases to — because the presenter stamps the + * cursor with them and the route re-checks that stamp on replay. Stamping the + * aliased column name would make every cursor minted under `name` fail to + * validate on the next request. + */ export const listSecretsUseCase = defineAuthorizedWorkspaceUseCase({ operation: secretOperations.list, resolveContext: ({ input }: { input: ListSecretsInput }) => @@ -135,12 +162,18 @@ export const listSecretsUseCase = defineAuthorizedWorkspaceUseCase({ authorizationOptions, async execute({ principal, input, context }) { const userId = principalUserId(principal) - const secrets = await listSecretMetadata({ + const page = await listSecretMetadata({ ...input, workspaceId: context.workspaceId, userId, }) - return { secrets, userId } + return { + secrets: page.data, + nextCursorKeys: page.nextCursorKeys, + userId, + sortBy: input.sortBy, + sortOrder: input.sortOrder, + } }, }) diff --git a/apps/sim/lib/skills/application/use-cases.ts b/apps/sim/lib/skills/application/use-cases.ts index 258280e26b8..251a2dc95b3 100644 --- a/apps/sim/lib/skills/application/use-cases.ts +++ b/apps/sim/lib/skills/application/use-cases.ts @@ -19,7 +19,7 @@ import { import { loadActiveWorkspaceContext } from '@/lib/uploads/contexts/workspace' import { getSkillById, - listSkills, + listSkillSummariesPage, listSkillsForUser, type SkillSortBy, } from '@/lib/workflows/skills/operations' @@ -58,20 +58,36 @@ export interface ListSkillsInput { search?: string sortBy: SkillSortBy sortOrder: ListSortOrder + limit: number + /** Position in the merged built-in + workspace list, read from the cursor. */ + offset: number } +/** + * The public skill list. Returns one page plus the window it was taken from, + * because the surface presenter sees only this result and needs both to mint + * the next cursor. + */ export const listSkillsUseCase = defineAuthorizedWorkspaceUseCase({ operation: skillOperations.list, resolveContext: ({ input }: { input: ListSkillsInput }) => resolveWorkspaceContext(input.workspaceId), authorizationOptions, async execute({ input, context }) { - const skills = await listSkills({ + const page = await listSkillSummariesPage({ workspaceId: context.workspaceId, search: input.search, - sort: { sortBy: input.sortBy, sortOrder: input.sortOrder }, + sortBy: input.sortBy, + sortOrder: input.sortOrder, + limit: input.limit, + offset: input.offset, }) - return { skills } + return { + skills: page.skills, + hasMore: page.hasMore, + offset: page.offset, + limit: page.limit, + } }, }) diff --git a/apps/sim/lib/workflows/application/list-workflows.ts b/apps/sim/lib/workflows/application/list-workflows.ts index f00ade5e38b..cf92b3863f2 100644 --- a/apps/sim/lib/workflows/application/list-workflows.ts +++ b/apps/sim/lib/workflows/application/list-workflows.ts @@ -8,7 +8,6 @@ import { resolveActiveWorkspaceApplicationContext } from '@/lib/workflows/applic import { workflowOperations } from '@/lib/workflows/application/operations' import { workflowFolderPathForId } from '@/lib/workflows/application/workflow-folders' import { - InvalidWorkflowListCursorError, listWorkspaceWorkflows, type WorkflowSortBy, type WorkflowSortOrder, @@ -48,24 +47,16 @@ export const listWorkflows = defineAuthorizedWorkflowUseCase({ throw new OrchestrationError('not_found', 'Folder not found') } - let page - try { - page = await listWorkspaceWorkflows({ - workspaceId: context.workspaceId, - folderId, - deployedOnly: input.deployedOnly, - search: input.search, - sortBy: input.sortBy, - sortOrder: input.sortOrder, - cursorKeys: input.cursorKeys, - limit: input.limit, - }) - } catch (error) { - if (error instanceof InvalidWorkflowListCursorError) { - throw new OrchestrationError('validation', error.message) - } - throw error - } + const page = await listWorkspaceWorkflows({ + workspaceId: context.workspaceId, + folderId, + deployedOnly: input.deployedOnly, + search: input.search, + sortBy: input.sortBy, + sortOrder: input.sortOrder, + cursorKeys: input.cursorKeys, + limit: input.limit, + }) logger.info('Listed workflows', { workspaceId: context.workspaceId, diff --git a/apps/sim/lib/workflows/application/workflow-crud.test.ts b/apps/sim/lib/workflows/application/workflow-crud.test.ts index f71e3546d9b..e6df3d563b8 100644 --- a/apps/sim/lib/workflows/application/workflow-crud.test.ts +++ b/apps/sim/lib/workflows/application/workflow-crud.test.ts @@ -72,7 +72,6 @@ vi.mock('@/lib/folders/queries', () => ({ })) vi.mock('@/lib/workflows/queries', () => ({ - InvalidWorkflowListCursorError: class InvalidWorkflowListCursorError extends Error {}, listWorkspaceWorkflows: mocks.listRows, loadWorkflowReadSnapshot: mocks.loadSnapshot, })) diff --git a/apps/sim/lib/workflows/custom-tools/operations.ts b/apps/sim/lib/workflows/custom-tools/operations.ts index aedc1f6cc2b..79e84b18511 100644 --- a/apps/sim/lib/workflows/custom-tools/operations.ts +++ b/apps/sim/lib/workflows/custom-tools/operations.ts @@ -2,8 +2,19 @@ import { db } from '@sim/db' import { customTools } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateShortId } from '@sim/utils/id' -import { and, type Column, desc, eq, isNull, or } from 'drizzle-orm' -import { type ListSortOrder, listOrderBy, searchFilter } from '@/lib/api/list-query' +import { and, desc, eq, isNull, or } from 'drizzle-orm' +import { + type CursorKey, + type KeysetKey, + keysetColumns, + keysetPage, + type ListSortOrder, + listOrderBy, + resumeKeyset, + searchFilter, + textKey, + timestampKey, +} from '@/lib/api/list-query' import { generateRequestId } from '@/lib/core/utils/request' const logger = createLogger('CustomToolsOperations') @@ -131,6 +142,29 @@ export async function listCustomTools(params: { userId: string; workspaceId?: st .orderBy(desc(customTools.createdAt)) } +type CustomToolRow = typeof customTools.$inferSelect + +const customToolId = textKey(customTools.id, (row) => row.id) + +/** + * Keyset orderings for the public list's sortable fields, made total over the + * contract enum by `satisfies`. Each ends in `id` so tools sharing a title or a + * timestamp still come back in a stable order — which is also what makes the + * cursor resumable, since a non-unique final key can repeat or skip a row at a + * page boundary. + */ +const CUSTOM_TOOL_SORTS = { + title: [textKey(customTools.title, (row) => row.title), customToolId], + createdAt: [ + timestampKey(customTools.createdAt, (row) => row.createdAt), + customToolId, + ], + updatedAt: [ + timestampKey(customTools.updatedAt, (row) => row.updatedAt), + customToolId, + ], +} satisfies Record[]> + /** * Workspace-scoped reads and deletes. * @@ -139,35 +173,33 @@ export async function listCustomTools(params: { userId: string; workspaceId?: st * scoped in every direction, so it uses these instead — a caller holding a * workspace key must never reach another user's personal tool. */ -/** - * Orderings for the public list's sortable fields, made total over the contract - * enum by `satisfies`. Each ends in `id` so tools sharing a timestamp still come - * back in a stable order. - */ -const CUSTOM_TOOL_SORTS = { - title: [customTools.title, customTools.id], - createdAt: [customTools.createdAt, customTools.id], - updatedAt: [customTools.updatedAt, customTools.id], -} satisfies Record - export async function listWorkspaceCustomTools(params: { workspaceId: string /** Case-insensitive substring match on the tool title. */ search?: string sortBy?: CustomToolSortBy sortOrder?: ListSortOrder + limit: number + cursorKeys?: CursorKey[] }) { - const { sortBy = 'createdAt', sortOrder = 'desc' } = params - return db + const { sortBy = 'createdAt', sortOrder = 'desc', limit } = params + const keys = CUSTOM_TOOL_SORTS[sortBy] + const resumeAfter = resumeKeyset(keys, params.cursorKeys, sortOrder) + + const rows = await db .select() .from(customTools) .where( and( eq(customTools.workspaceId, params.workspaceId), - searchFilter(customTools.title, params.search) + searchFilter(customTools.title, params.search), + resumeAfter ) ) - .orderBy(...listOrderBy(CUSTOM_TOOL_SORTS[sortBy], sortOrder)) + .orderBy(...listOrderBy(keysetColumns(keys), sortOrder)) + .limit(limit + 1) + + return keysetPage(keys, rows, limit) } export async function getWorkspaceCustomTool(params: { workspaceId: string; toolId: string }) { diff --git a/apps/sim/lib/workflows/queries.ts b/apps/sim/lib/workflows/queries.ts index f4a4a11c198..8e7fa6f0663 100644 --- a/apps/sim/lib/workflows/queries.ts +++ b/apps/sim/lib/workflows/queries.ts @@ -4,13 +4,13 @@ import { and, asc, eq, inArray, isNull, type SQL, sql } from 'drizzle-orm' import type { WorkflowListItem } from '@/lib/api/contracts/workflows' import { type CursorKey, - encodeKeyset, type KeysetKey, - keysetAfter, keysetColumns, + keysetPage, type ListSortOrder, listOrderBy, numberKey, + resumeKeyset, searchFilter, textKey, timestampKey, @@ -62,13 +62,6 @@ const WORKFLOW_SORTS = { ], } satisfies Record[]> -export class InvalidWorkflowListCursorError extends Error { - constructor() { - super('Cursor does not match the requested workflow sort') - this.name = 'InvalidWorkflowListCursorError' - } -} - export interface ListWorkspaceWorkflowsInput { workspaceId: string folderId?: string | null @@ -83,10 +76,7 @@ export interface ListWorkspaceWorkflowsInput { /** Cursor-paged active workflow query used by the public workflow adapter. */ export async function listWorkspaceWorkflows(input: ListWorkspaceWorkflowsInput) { const keys = WORKFLOW_SORTS[input.sortBy] - const resumeAfter = input.cursorKeys - ? keysetAfter(keys, input.cursorKeys, input.sortOrder) - : undefined - if (resumeAfter === null) throw new InvalidWorkflowListCursorError() + const resumeAfter = resumeKeyset(keys, input.cursorKeys, input.sortOrder) const folderCondition: SQL | undefined = input.folderId === undefined @@ -124,13 +114,7 @@ export async function listWorkspaceWorkflows(input: ListWorkspaceWorkflowsInput) .orderBy(...listOrderBy(keysetColumns(keys), input.sortOrder)) .limit(input.limit + 1) - const hasMore = rows.length > input.limit - const data = rows.slice(0, input.limit) - const last = data.at(-1) - return { - data, - nextCursorKeys: hasMore && last ? encodeKeyset(keys, last) : null, - } + return keysetPage(keys, rows, input.limit) } /** diff --git a/apps/sim/lib/workflows/skills/operations.test.ts b/apps/sim/lib/workflows/skills/operations.test.ts index 62f49c01b57..e4e3efdc19e 100644 --- a/apps/sim/lib/workflows/skills/operations.test.ts +++ b/apps/sim/lib/workflows/skills/operations.test.ts @@ -1,7 +1,13 @@ /** * @vitest-environment node */ -import { dbChainMock, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { + dbChainMock, + dbChainMockFns, + queueTableRows, + resetDbChainMock, + schemaMock, +} from '@sim/testing' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' const { getEditableSkillIdsMock } = vi.hoisted(() => ({ @@ -14,7 +20,12 @@ vi.mock('@/lib/skills/access', () => ({ getEditableSkillIds: getEditableSkillIdsMock, })) -import { listSkills, listSkillsForUser } from '@/lib/workflows/skills/operations' +import { BUILTIN_SKILLS } from '@/lib/workflows/skills/builtin-skills' +import { + listSkillSummariesPage, + listSkills, + listSkillsForUser, +} from '@/lib/workflows/skills/operations' describe('listSkills includeBuiltins', () => { beforeEach(() => { @@ -47,6 +58,69 @@ describe('listSkills includeBuiltins', () => { }) }) +/** + * The page is sliced after the built-ins are merged in and the merged array is + * re-sorted, so these assert the window over that merged sequence — not over + * the DB rows alone. + */ +describe('listSkillSummariesPage', () => { + const page = (limit: number, offset: number) => + listSkillSummariesPage({ + workspaceId: 'ws-1', + sortBy: 'name', + sortOrder: 'asc', + limit, + offset, + }) + + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + afterAll(() => { + resetDbChainMock() + }) + + it('returns at most `limit` skills and reports that more remain', async () => { + const result = await page(2, 0) + + expect(result.skills).toHaveLength(2) + expect(result.hasMore).toBe(true) + expect(result).toMatchObject({ offset: 0, limit: 2 }) + }) + + it('resumes at the offset and reports the end of the list', async () => { + const first = await page(BUILTIN_SKILLS.length, 0) + const second = await page(BUILTIN_SKILLS.length, BUILTIN_SKILLS.length) + + expect(first.skills).toHaveLength(BUILTIN_SKILLS.length) + expect(first.hasMore).toBe(false) + expect(second.skills).toHaveLength(0) + expect(second.hasMore).toBe(false) + }) + + it('pages over the merged built-in and workspace sequence, not the DB rows alone', async () => { + queueTableRows(schemaMock.skill, [ + { id: 'sk-1', name: 'aaa-mine', description: 'd', workspaceId: 'ws-1' }, + ]) + + const result = await page(1, 0) + + expect(result.skills.map((s) => s.id)).toEqual(['sk-1']) + expect(result.hasMore).toBe(true) + }) + + it('never selects the skill body for a list page', async () => { + await page(50, 0) + + const projection = dbChainMockFns.select.mock.calls.at(-1)?.[0] + expect(projection).toBeDefined() + expect(projection).not.toHaveProperty('content') + expect(projection).toHaveProperty('name') + }) +}) + describe('listSkillsForUser', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/apps/sim/lib/workflows/skills/operations.ts b/apps/sim/lib/workflows/skills/operations.ts index bdc1347fe5f..9ad431d3516 100644 --- a/apps/sim/lib/workflows/skills/operations.ts +++ b/apps/sim/lib/workflows/skills/operations.ts @@ -19,21 +19,57 @@ const logger = createLogger('SkillsOperations') /** Stable epoch timestamp for built-in (template) skills, which have no DB row. */ const BUILTIN_SKILL_TIMESTAMP = new Date(0) -/** Shape a built-in skill as a `skill` table row so it can ride alongside DB skills. */ -function builtinSkillRow(workspaceId: string, builtin: BuiltinSkill): typeof skill.$inferSelect { +type SkillRow = typeof skill.$inferSelect + +/** + * A skill row without its body. Skill content runs to 50 000 characters, so + * list surfaces that only render metadata select this projection instead of + * loading every body just to drop it in the presenter. + */ +export type SkillSummaryRow = Omit + +/** The `skill` columns that make up {@link SkillSummaryRow}. */ +const SKILL_SUMMARY_COLUMNS = { + id: skill.id, + workspaceId: skill.workspaceId, + userId: skill.userId, + name: skill.name, + description: skill.description, + createdAt: skill.createdAt, + updatedAt: skill.updatedAt, +} + +/** Shape a built-in skill as a body-less `skill` row so it can ride alongside DB skills. */ +function builtinSkillSummaryRow(workspaceId: string, builtin: BuiltinSkill): SkillSummaryRow { return { id: builtin.id, workspaceId, userId: null, name: builtin.name, description: builtin.description, - content: builtin.content, createdAt: BUILTIN_SKILL_TIMESTAMP, updatedAt: BUILTIN_SKILL_TIMESTAMP, } } -type SkillRow = typeof skill.$inferSelect +function builtinSkillRow(workspaceId: string, builtin: BuiltinSkill): SkillRow { + return { ...builtinSkillSummaryRow(workspaceId, builtin), content: builtin.content } +} + +/** + * The built-ins a list should show: those no DB row shadows by name, narrowed + * by the same search term the DB half ran. + * + * Restricting `dbNames` to the searched rows is safe: a DB skill only shadows a + * built-in by sharing its name, and a name that matches the search on the + * built-in matches it on the DB row too. + */ +function visibleBuiltins(dbNames: Set, search?: string): BuiltinSkill[] { + const term = search?.toLowerCase() + return BUILTIN_SKILLS.filter( + (b) => !dbNames.has(b.name.toLowerCase()) && (!term || b.name.toLowerCase().includes(term)) + ) +} export type SkillSortBy = 'name' | 'createdAt' | 'updatedAt' /** @@ -48,12 +84,12 @@ const SKILL_SORTS = { } satisfies Record /** The sort key {@link SKILL_SORTS} orders on, for one row. */ -function skillSortKey(row: SkillRow, sortBy: SkillSortBy): [string | number, string] { +function skillSortKey(row: SkillSummaryRow, sortBy: SkillSortBy): [string | number, string] { if (sortBy === 'name') return [row.name, row.id] return [(sortBy === 'createdAt' ? row.createdAt : row.updatedAt).getTime(), row.id] } -function compareSkills(a: SkillRow, b: SkillRow, sortBy: SkillSortBy): number { +function compareSkills(a: SkillSummaryRow, b: SkillSummaryRow, sortBy: SkillSortBy): number { const [aKey, aId] = skillSortKey(a, sortBy) const [bKey, bId] = skillSortKey(b, sortBy) if (aKey !== bKey) return aKey < bKey ? -1 : 1 @@ -79,6 +115,9 @@ function compareSkills(a: SkillRow, b: SkillRow, sortBy: SkillSortBy): number { * The merged ordering compares names with JS string order rather than the * database collation. Only the handful of ASCII built-in names are placed by * it, so the two agree in practice. + * + * This returns the whole set. The public v2 list pages through + * {@link listSkillSummariesPage} instead. */ export async function listSkills(params: { workspaceId: string @@ -100,16 +139,10 @@ export async function listSkills(params: { return dbRows } - /** - * Restricting `dbNames` to the searched rows is safe: a DB skill only shadows - * a built-in by sharing its name, and a name that matches the search on the - * built-in matches it on the DB row too. - */ const dbNames = new Set(dbRows.map((r) => r.name.toLowerCase())) - const term = params.search?.toLowerCase() - const builtins = BUILTIN_SKILLS.filter( - (b) => !dbNames.has(b.name.toLowerCase()) && (!term || b.name.toLowerCase().includes(term)) - ).map((b) => builtinSkillRow(params.workspaceId, b)) + const builtins = visibleBuiltins(dbNames, params.search).map((b) => + builtinSkillRow(params.workspaceId, b) + ) if (!params.sort) return [...builtins, ...dbRows] @@ -117,6 +150,65 @@ export async function listSkills(params: { return [...builtins, ...dbRows].sort((a, b) => direction * compareSkills(a, b, sortBy)) } +/** One page of skill summaries, plus the window it was taken from. */ +export interface SkillSummaryPage { + skills: SkillSummaryRow[] + hasMore: boolean + /** The window actually applied, so a caller can mint the next cursor. */ + offset: number + limit: number +} + +/** + * A page of the public skill list: built-ins merged with the workspace's DB + * rows, sorted as requested, then sliced. + * + * **Why the window is an offset and not a keyset.** Every other v2 list resumes + * from a keyset cursor pushed into the SQL `WHERE`. This list cannot: half its + * items are {@link BUILTIN_SKILLS}, a code constant with no DB row for a + * predicate to act on, and the merged array is re-sorted in JS afterwards. A + * keyset over `skill` columns therefore cannot express a position inside the + * merged sequence — under `createdAt asc` the epoch-stamped built-ins all sort + * ahead of every DB row and a SQL cursor would skip or repeat them. So this + * follows the offset-cursor pattern that `GET /api/v2/knowledge/[id]/documents` + * already uses. + * + * The consequence of that merge is that the DB half is read whole on every + * page: the slice happens after the merge, so `LIMIT` cannot be pushed down. + * The projection drops `content` to keep that read cheap. + */ +export async function listSkillSummariesPage(params: { + workspaceId: string + search?: string + sortBy: SkillSortBy + sortOrder: ListSortOrder + limit: number + offset: number +}): Promise { + const { sortBy, sortOrder, limit, offset } = params + + const dbRows = await db + .select(SKILL_SUMMARY_COLUMNS) + .from(skill) + .where(and(eq(skill.workspaceId, params.workspaceId), searchFilter(skill.name, params.search))) + .orderBy(...listOrderBy(SKILL_SORTS[sortBy], sortOrder)) + + const dbNames = new Set(dbRows.map((r) => r.name.toLowerCase())) + const builtins = visibleBuiltins(dbNames, params.search).map((b) => + builtinSkillSummaryRow(params.workspaceId, b) + ) + + const direction = sortOrder === 'asc' ? 1 : -1 + const merged = [...builtins, ...dbRows].sort((a, b) => direction * compareSkills(a, b, sortBy)) + + return { + skills: merged.slice(offset, offset + limit), + hasMore: offset + limit < merged.length, + offset, + limit, + } +} + /** A skill row tagged with whether the caller can edit it (always false on builtins). */ export type SkillWithAccess = typeof skill.$inferSelect & { canEdit: boolean } diff --git a/scripts/check-api-validation-contracts.ts b/scripts/check-api-validation-contracts.ts index 63030a06ee2..c62f448314d 100644 --- a/scripts/check-api-validation-contracts.ts +++ b/scripts/check-api-validation-contracts.ts @@ -9,8 +9,8 @@ const QUERY_HOOKS_DIR = path.join(ROOT, 'apps/sim/hooks/queries') const SELECTOR_HOOKS_DIR = path.join(ROOT, 'apps/sim/hooks/selectors') const BASELINE = { - totalRoutes: 1098, - zodRoutes: 1098, + totalRoutes: 1099, + zodRoutes: 1099, nonZodRoutes: 0, } as const @@ -31,6 +31,11 @@ const BOUNDARY_POLICY_BASELINE = { } as const const INDIRECT_ZOD_ROUTES = new Set([ + // Catch-all JSON 404 for unknown /api/v2 paths. It has no contract by + // construction: it exists precisely for requests that match no operation, so + // there is no input to validate and its only response is the fixed v2 error + // envelope. + 'apps/sim/app/api/v2/[[...segments]]/route.ts', 'apps/sim/app/api/demo-requests/route.ts', // Input-less session-bound GET: nothing to validate; response is // contract-typed via `satisfies InvitationDetails` in the route. From f77fddbbf6fa1ebde62ca95fc76d0f9e16900650 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 12 Aug 2026 04:24:23 -0700 Subject: [PATCH 2/2] fix(v2): bind the offset cursor to the query state it counts positions in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An offset names a position in one exact sequence. `GET /skills` accepted a bare `{offset}` cursor and applied it to whatever sequence the next request asked for, so following `nextCursor` with a different `search`, `sortBy` or `sortOrder` silently skipped rows, repeated them, or landed past the end and returned an empty page while the cursor implied more. Fixed in the codec rather than the route so the sibling could not keep the gap: `decodeOffsetCursor` now takes a scope stamp and rejects a cursor minted under a different one, which is what `decodeSortedCursor` has always done for keysets. `offsetCursorScope()` builds the stamp from every param that filters or orders the sequence; `limit` is excluded because it selects how much of the sequence to return, not what the sequence is, so paging with a different page size still works. `GET /knowledge/{id}/documents` had the identical latent gap and gets the same treatment — the compiler surfaced it as soon as the signature changed. --- .agents/skills/v2-api-conventions/SKILL.md | 2 +- .claude/commands/v2-api-conventions.md | 2 +- .cursor/commands/v2-api-conventions.md | 2 +- .../api/v2/knowledge/[id]/documents/route.ts | 46 ++++++++--- apps/sim/app/api/v2/lib/response.ts | 48 +++++++++-- apps/sim/app/api/v2/skills/route.test.ts | 42 +++++++++- apps/sim/app/api/v2/skills/route.ts | 40 +++++++++- apps/sim/lib/api/offset-cursor-scope.test.ts | 80 +++++++++++++++++++ .../lib/knowledge/application/documents.ts | 8 +- apps/sim/lib/skills/application/use-cases.ts | 6 ++ 10 files changed, 247 insertions(+), 29 deletions(-) create mode 100644 apps/sim/lib/api/offset-cursor-scope.test.ts diff --git a/.agents/skills/v2-api-conventions/SKILL.md b/.agents/skills/v2-api-conventions/SKILL.md index a8e5be1d6bb..52ab14450ad 100644 --- a/.agents/skills/v2-api-conventions/SKILL.md +++ b/.agents/skills/v2-api-conventions/SKILL.md @@ -79,7 +79,7 @@ That gives `limit` (integer, 1..`V2_MAX_PAGE_SIZE`, defaulting to `V2_DEFAULT_PA 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: - **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. -- **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. +- **Offset** (`decodeOffsetCursor` / `encodeOffsetCursor`) — 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. An offset cursor **must** be stamped with `offsetCursorScope(...)` covering every param that filters or orders the sequence (not `limit`, which only selects how much of it to return). A bare offset replayed against a re-sorted or re-filtered sequence names a different row, which skips or repeats results — the exact failure the keyset's sort stamp already prevents. **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. diff --git a/.claude/commands/v2-api-conventions.md b/.claude/commands/v2-api-conventions.md index f902d2c0424..c7740489850 100644 --- a/.claude/commands/v2-api-conventions.md +++ b/.claude/commands/v2-api-conventions.md @@ -78,7 +78,7 @@ That gives `limit` (integer, 1..`V2_MAX_PAGE_SIZE`, defaulting to `V2_DEFAULT_PA 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: - **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. -- **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. +- **Offset** (`decodeOffsetCursor` / `encodeOffsetCursor`) — 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. An offset cursor **must** be stamped with `offsetCursorScope(...)` covering every param that filters or orders the sequence (not `limit`, which only selects how much of it to return). A bare offset replayed against a re-sorted or re-filtered sequence names a different row, which skips or repeats results — the exact failure the keyset's sort stamp already prevents. **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. diff --git a/.cursor/commands/v2-api-conventions.md b/.cursor/commands/v2-api-conventions.md index a61cd94a7a9..a19ab57e6aa 100644 --- a/.cursor/commands/v2-api-conventions.md +++ b/.cursor/commands/v2-api-conventions.md @@ -73,7 +73,7 @@ That gives `limit` (integer, 1..`V2_MAX_PAGE_SIZE`, defaulting to `V2_DEFAULT_PA 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: - **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. -- **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. +- **Offset** (`decodeOffsetCursor` / `encodeOffsetCursor`) — 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. An offset cursor **must** be stamped with `offsetCursorScope(...)` covering every param that filters or orders the sequence (not `limit`, which only selects how much of it to return). A bare offset replayed against a re-sorted or re-filtered sequence names a different row, which skips or repeats results — the exact failure the keyset's sort stamp already prevents. **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. diff --git a/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts b/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts index 6ba62a1a941..dfd8dd1255c 100644 --- a/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts +++ b/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts @@ -29,7 +29,11 @@ import { captureServerEvent } from '@/lib/posthog/server' import { MAX_KNOWLEDGE_DOCUMENT_FILE_SIZE } from '@/lib/uploads/shared/types' import { validateFileType } from '@/lib/uploads/utils/validation' import { serializeDate } from '@/app/api/v1/knowledge/utils' -import { decodeOffsetCursor, encodeCursor } from '@/app/api/v2/lib/response' +import { + decodeOffsetCursor, + encodeOffsetCursor, + offsetCursorScope, +} from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -71,21 +75,37 @@ export const GET = defineV2JsonRoute({ operation: knowledgeOperations.listDocuments, rateLimit: v2RateLimits.publicApi, errorPolicy: v2KnowledgeErrorPolicies.concealKnowledgeBaseAuthorization, - mapInput: ({ params, query }) => ({ - knowledgeBaseId: params.id, - assertedWorkspaceId: query.workspaceId, - enabledFilter: query.enabledFilter, - search: query.search, - limit: query.limit, - offset: decodeOffsetCursor(query.cursor), - sortBy: query.sortBy, - sortOrder: query.sortOrder, - }), + mapInput: ({ params, query }) => { + /** + * The offset counts positions in the filtered, sorted document sequence, so + * every param that changes that sequence is stamped into the cursor and + * re-checked here. `limit` selects how much of the sequence to return, not + * what the sequence is, so it stays out. + */ + const cursorScope = offsetCursorScope({ + knowledgeBaseId: params.id, + enabledFilter: query.enabledFilter, + search: query.search, + sortBy: query.sortBy, + sortOrder: query.sortOrder, + }) + return { + knowledgeBaseId: params.id, + assertedWorkspaceId: query.workspaceId, + enabledFilter: query.enabledFilter, + search: query.search, + limit: query.limit, + offset: decodeOffsetCursor(query.cursor, cursorScope), + sortBy: query.sortBy, + sortOrder: query.sortOrder, + cursorScope, + } + }, useCase: listKnowledgeDocuments, - present: ({ documents, pagination }) => ({ + present: ({ documents, pagination, cursorScope }) => ({ data: documents.map(toV2DocumentSummary), nextCursor: pagination.hasMore - ? encodeCursor({ offset: pagination.offset + pagination.limit }) + ? encodeOffsetCursor(cursorScope ?? '', pagination.offset + pagination.limit) : null, }), }) diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index a625ecc3e81..93c983828cb 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -174,18 +174,56 @@ export function decodeCursor>(cursor: string): T | n } } +interface OffsetCursorPayload { + /** The query state the offset counts positions within. */ + scope: string + offset: number +} + +/** + * The filters and sort an offset cursor was minted under. + * + * An offset is only meaningful against one exact sequence, so everything that + * reorders or re-filters that sequence has to travel with it. Build the stamp + * from every such param; a value that does not affect ordering or membership + * (the page size itself) must stay out, or paging with a different `limit` + * would be rejected for no reason. + */ +export function offsetCursorScope(parts: Record): string { + return Object.keys(parts) + .sort() + .map((key) => `${key}=${parts[key] ?? ''}`) + .join('&') +} + +/** An offset cursor stamped with the query state that produced it. */ +export function encodeOffsetCursor(scope: string, offset: number): string { + return encodeCursor({ scope, offset } satisfies OffsetCursorPayload) +} + /** - * Reads back an offset cursor minted by `encodeCursor({ offset })`. + * Reads back an offset cursor, refusing one minted under different filters or a + * different sort. * * An absent cursor means page one. A cursor that is not valid base64-JSON, or * that does not carry a non-negative integer `offset`, is rejected rather than * coerced to 0: silently restarting at page one while the caller believes it is - * paging forward makes a paging client loop over the first page forever. The v2 - * error policies render the thrown validation error as the canonical 400. + * paging forward makes a paging client loop over the first page forever. + * + * The `scope` check is the offset counterpart of {@link decodeSortedCursor}'s + * sort stamp. A bare offset replayed against a newly filtered or re-sorted + * sequence names a different position in it, which silently skips rows, repeats + * them, or lands past the end and returns an empty page — the failure a keyset + * cursor is already protected from. The v2 error policies render the thrown + * validation error as the canonical 400. */ -export function decodeOffsetCursor(cursor: string | undefined): number { +export function decodeOffsetCursor(cursor: string | undefined, scope: string): number { if (!cursor) return 0 - const offset = decodeCursor<{ offset?: unknown }>(cursor)?.offset + const decoded = decodeCursor>(cursor) + if (!decoded || decoded.scope !== scope) { + throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) + } + const { offset } = decoded if (typeof offset !== 'number' || !Number.isInteger(offset) || offset < 0) { throw new OrchestrationError('validation', 'Invalid cursor') } diff --git a/apps/sim/app/api/v2/skills/route.test.ts b/apps/sim/app/api/v2/skills/route.test.ts index 4141c05b494..6e193ae8d11 100644 --- a/apps/sim/app/api/v2/skills/route.test.ts +++ b/apps/sim/app/api/v2/skills/route.test.ts @@ -53,6 +53,17 @@ vi.mock('@/lib/skills/application/use-cases', () => ({ import { GET, POST } from '@/app/api/v2/skills/route' const WORKSPACE_ID = 'workspace-1' + +/** + * The scope stamp the route mints for the default query. Written out rather + * than imported so the test pins the wire format a shipped cursor carries. + */ +const SCOPE = ({ + search = '', + sortBy = 'createdAt', + sortOrder = 'desc', +}: Record = {}) => + `search=${search}&sortBy=${sortBy}&sortOrder=${sortOrder}&workspaceId=${WORKSPACE_ID}` const PRINCIPAL = { kind: 'workspace_api_key' as const, workspaceId: WORKSPACE_ID, keyId: 'key-1' } const AUTH = { principal: PRINCIPAL, @@ -118,14 +129,21 @@ describe('/api/v2/skills', () => { limit: 50, cursor: undefined, offset: 0, + cursorScope: SCOPE(), }, request: expect.anything(), }) }) it('resumes from the offset cursor and mints the next one while pages remain', async () => { - mocks.list.mockResolvedValueOnce({ skills: [skill], hasMore: true, offset: 2, limit: 2 }) - const cursor = Buffer.from(JSON.stringify({ offset: 2 })).toString('base64') + mocks.list.mockResolvedValueOnce({ + skills: [skill], + hasMore: true, + offset: 2, + limit: 2, + cursorScope: SCOPE(), + }) + const cursor = Buffer.from(JSON.stringify({ scope: SCOPE(), offset: 2 })).toString('base64') const response = await GET( request( @@ -136,13 +154,31 @@ describe('/api/v2/skills', () => { expect(response.status).toBe(200) expect((await response.json()).nextCursor).toBe( - Buffer.from(JSON.stringify({ offset: 4 })).toString('base64') + Buffer.from(JSON.stringify({ scope: SCOPE(), offset: 4 })).toString('base64') ) expect(mocks.list).toHaveBeenCalledWith( expect.objectContaining({ input: expect.objectContaining({ limit: 2, offset: 2 }) }) ) }) + /** + * An offset means nothing against a sequence it was not counted in, so a + * cursor minted under one sort must not silently resume under another. + */ + it('rejects a cursor replayed under a different sort', async () => { + const cursor = Buffer.from(JSON.stringify({ scope: SCOPE(), offset: 2 })).toString('base64') + + const response = await GET( + request( + 'GET', + `/api/v2/skills?workspaceId=${WORKSPACE_ID}&sortBy=name&cursor=${encodeURIComponent(cursor)}` + ) + ) + + expect(response.status).toBe(400) + expect(mocks.list).not.toHaveBeenCalled() + }) + it('rejects a malformed cursor rather than silently restarting at page one', async () => { const response = await GET( request('GET', `/api/v2/skills?workspaceId=${WORKSPACE_ID}&cursor=not-a-cursor`) diff --git a/apps/sim/app/api/v2/skills/route.ts b/apps/sim/app/api/v2/skills/route.ts index 27ab9f4b0c5..e1356ef7ea0 100644 --- a/apps/sim/app/api/v2/skills/route.ts +++ b/apps/sim/app/api/v2/skills/route.ts @@ -8,9 +8,28 @@ import { import { captureServerEvent } from '@/lib/posthog/server' import { skillOperations } from '@/lib/skills/application/operations' import { createSkillUseCase, listSkillsUseCase } from '@/lib/skills/application/use-cases' -import { decodeOffsetCursor, encodeCursor } from '@/app/api/v2/lib/response' +import { + decodeOffsetCursor, + encodeOffsetCursor, + offsetCursorScope, +} from '@/app/api/v2/lib/response' import { toV2Skill, toV2SkillSummary } from '@/app/api/v2/skills/utils' +/** The query state a skills offset cursor is only valid within. */ +function skillCursorScope(query: { + workspaceId: string + search?: string + sortBy: string + sortOrder: string +}): string { + return offsetCursorScope({ + workspaceId: query.workspaceId, + search: query.search, + sortBy: query.sortBy, + sortOrder: query.sortOrder, + }) +} + export const dynamic = 'force-dynamic' export const revalidate = 0 @@ -21,11 +40,24 @@ export const GET = defineV2JsonRoute({ auth: v2ApiKeyAuth, rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, - mapInput: ({ query }) => ({ ...query, offset: decodeOffsetCursor(query.cursor) }), + mapInput: ({ query }) => { + /** + * The offset counts positions in the merged, filtered, sorted sequence, so + * every param that changes that sequence is stamped into the cursor and + * re-checked here. `limit` is deliberately absent — it selects how much of + * the sequence to return, not what the sequence is. + */ + const scope = skillCursorScope(query) + return { + ...query, + offset: decodeOffsetCursor(query.cursor, scope), + cursorScope: scope, + } + }, useCase: listSkillsUseCase, - present: ({ skills, hasMore, offset, limit }) => ({ + present: ({ skills, hasMore, offset, limit, cursorScope }) => ({ data: skills.map(toV2SkillSummary), - nextCursor: hasMore ? encodeCursor({ offset: offset + limit }) : null, + nextCursor: hasMore ? encodeOffsetCursor(cursorScope, offset + limit) : null, }), }) diff --git a/apps/sim/lib/api/offset-cursor-scope.test.ts b/apps/sim/lib/api/offset-cursor-scope.test.ts new file mode 100644 index 00000000000..b81cfb1dffe --- /dev/null +++ b/apps/sim/lib/api/offset-cursor-scope.test.ts @@ -0,0 +1,80 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + decodeOffsetCursor, + encodeOffsetCursor, + offsetCursorScope, +} from '@/app/api/v2/lib/response' + +/** + * An offset names a position in one exact sequence. Replay it against a + * differently filtered or sorted sequence and it names a different row — + * silently skipping rows, repeating them, or landing past the end and returning + * an empty page while `nextCursor` implied more. + * + * The keyset cursor has always been protected from this by its sort stamp + * (`decodeSortedCursor`). These assertions hold the offset cursor — used by + * `GET /skills` and `GET /knowledge/{id}/documents` — to the same rule. + */ +describe('offset cursor scope', () => { + const base = { workspaceId: 'ws-1', search: undefined, sortBy: 'name', sortOrder: 'asc' } + + it('resumes a cursor replayed under the same query state', () => { + const scope = offsetCursorScope(base) + expect(decodeOffsetCursor(encodeOffsetCursor(scope, 40), scope)).toBe(40) + }) + + it('rejects a cursor replayed under a different sort', () => { + const cursor = encodeOffsetCursor(offsetCursorScope(base), 40) + + expect(() => + decodeOffsetCursor(cursor, offsetCursorScope({ ...base, sortBy: 'createdAt' })) + ).toThrow(/does not match the requested/) + expect(() => + decodeOffsetCursor(cursor, offsetCursorScope({ ...base, sortOrder: 'desc' })) + ).toThrow(/does not match the requested/) + }) + + it('rejects a cursor replayed under a different filter', () => { + const cursor = encodeOffsetCursor(offsetCursorScope(base), 40) + + expect(() => + decodeOffsetCursor(cursor, offsetCursorScope({ ...base, search: 'deploy' })) + ).toThrow(/does not match the requested/) + expect(() => + decodeOffsetCursor(cursor, offsetCursorScope({ ...base, workspaceId: 'ws-2' })) + ).toThrow(/does not match the requested/) + }) + + it('treats an absent cursor as page one', () => { + expect(decodeOffsetCursor(undefined, offsetCursorScope(base))).toBe(0) + }) + + it('rejects a cursor that is not valid base64-JSON', () => { + expect(() => decodeOffsetCursor('not-a-cursor', offsetCursorScope(base))).toThrow() + }) + + it('rejects an offset that is not a non-negative integer', () => { + const scope = offsetCursorScope(base) + expect(() => decodeOffsetCursor(encodeOffsetCursor(scope, -1), scope)).toThrow('Invalid cursor') + expect(() => decodeOffsetCursor(encodeOffsetCursor(scope, 1.5), scope)).toThrow( + 'Invalid cursor' + ) + }) + + /** + * `limit` selects how much of the sequence to return, not what the sequence + * is, so paging with a different page size must keep working. + */ + it('is unaffected by the page size', () => { + expect(offsetCursorScope({ ...base, sortBy: 'name' })).toBe(offsetCursorScope(base)) + }) + + it('does not depend on the order the parts are written', () => { + expect(offsetCursorScope({ sortBy: 'name', workspaceId: 'ws-1', sortOrder: 'asc' })).toBe( + offsetCursorScope({ sortOrder: 'asc', workspaceId: 'ws-1', sortBy: 'name' }) + ) + }) +}) diff --git a/apps/sim/lib/knowledge/application/documents.ts b/apps/sim/lib/knowledge/application/documents.ts index 30b9445fbd3..11b1add79b2 100644 --- a/apps/sim/lib/knowledge/application/documents.ts +++ b/apps/sim/lib/knowledge/application/documents.ts @@ -74,6 +74,12 @@ export interface ListKnowledgeDocumentsInput { sortBy?: DocumentSortField sortOrder?: SortOrder tagFilters?: TagFilterCondition[] + /** + * The query state `offset` counts positions within, echoed back so a surface + * presenter can stamp the next cursor with it. Surface-only; the read itself + * does not use it. + */ + cursorScope?: string } export interface ReadKnowledgeDocumentInput { @@ -219,7 +225,7 @@ export const listKnowledgeDocuments = defineAuthorizedKnowledgeUseCase({ }, generateRequestId() ) - return { ...result, workspaceId: context.workspaceId } + return { ...result, workspaceId: context.workspaceId, cursorScope: input.cursorScope } }, }) diff --git a/apps/sim/lib/skills/application/use-cases.ts b/apps/sim/lib/skills/application/use-cases.ts index 251a2dc95b3..8f54b55dd69 100644 --- a/apps/sim/lib/skills/application/use-cases.ts +++ b/apps/sim/lib/skills/application/use-cases.ts @@ -61,6 +61,11 @@ export interface ListSkillsInput { limit: number /** Position in the merged built-in + workspace list, read from the cursor. */ offset: number + /** + * The query state that position is valid within, echoed back so the presenter + * can stamp the next cursor with it. + */ + cursorScope: string } /** @@ -87,6 +92,7 @@ export const listSkillsUseCase = defineAuthorizedWorkspaceUseCase({ hasMore: page.hasMore, offset: page.offset, limit: page.limit, + cursorScope: input.cursorScope, } }, })