GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — fix(v2): give every collection one pagination contract and close four envelope holes by waleedlatif1 · Pull Request #6620 · simstudioai/sim Conversation
|
| In | Was | Now |
|---|---|---|
GET /workflows?limit=1.5 |
500 INTERNAL_ERROR |
400 limit must be a whole number |
GET /audit-logs?limit=1.5 |
500 INTERNAL_ERROR |
400 (same) |
limit=Infinity |
400 expected number, received number |
400 limit must be a number |
POST with malformed/absent JSON |
{"error":"Request body must be valid JSON"} — a bare string |
{"error":{"code":"BAD_REQUEST","message":...}} |
GET /api/v2/nonexistent |
Next.js HTML 404 document | {"error":{"code":"NOT_FOUND","message":"Not found"}} |
GET /skills?limit=1 |
200, 11 items, nextCursor present but unusable |
200, 1 item, working nextCursor |
GET /knowledge?limit=1 |
400 Unrecognized key: limit, yet nextCursor in the body |
200, 1 item, working nextCursor |
limit was re-declared inline by every list. GET /workflows was copied from a
sibling and lost its .int(), so 1.5 passed validation and reached Postgres as
LIMIT 2.5. Same defect, independently, on GET /audit-logs. Meanwhile the same
param was validated five different ways across the API, and five collections
emitted nextCursor while accepting no limit at all — or accepting one and
silently discarding it, because Zod strips unknown keys unless the schema is
.strict().
So this is one problem — no shared pagination contract — wearing five hats, and
it is fixed once: v2PaginationFields() in lib/api/contracts/v2/shared.ts,
adopted by all 17 paged lists.
The v2-api rollout flag is off in production and enabled only for a
staging cohort. The four changes below are breaking the moment v2 goes GA, and
cost nothing today. This is the last cheap window.
Intentional, user-visible:
/skills,/custom-tools, /secrets, /credentials, /knowledge previously returnedlimit up to 100 and a working cursor. A caller that read thenextCursor..strict(),limit was ignored" into an error you can see.cursor= is now a 400 on /workflows, /workflows/{id}/versions/logs. This is a deliberate behaviour change to three endpoints thatcursor= silently restarts at page one on every request and loops forever,/skills and /knowledge/{id}/documents. An offset names a position in onelimit is excluded from the stamp, soDeliberately not changed: /files, /logs, /tables still truncate and
clamp an out-of-range limit rather than rejecting it. That leniency is
published in their OpenAPI description, so tightening it would break working
callers. It is now an explicit, named mode on the shared schema (outOfRange: 'clamp') and pinned by a test, rather than three hand-rolled copies.
Both cursor schemes are the existing opaque base64-JSON codecs; no new format.
encodeSortedCursor, the /workflows model) — /custom-tools,/credentials, /secrets, /knowledge. Each sort now ends in a unique idknowledge previously tie-broke on createdAt, which cannot separate twoencodeCursor({offset}), the /knowledge/{id}/documents model) —/skills only, because it merges the 4-entry static builtin registry into theWHERE tocreatedAt asc they all sort ahead of every DB row. This isTwo correctness fixes that pagination forced:
row.type === 'env_workspace' || row.envOwnerUserId === userId)limit while nextCursor still claims more, so it moved into SQL.content column (up to 50k chars/skill)/skills pagination bounds the response, not the DB read. Because the four
built-in skills merge in JS and the merged array is re-sorted before slicing,
LIMIT cannot be pushed down, so every page still reads the workspace's skill
rows. The projection now drops the content column (up to 50k chars/skill),
which is the large win; bounding the row count needs a second query to resolve
built-in shadowing across the truncation boundary, and that has a correctness
surface worth testing against a real database rather than landing blind here.
405 responses still have no Allow header and an empty body. Next.js
generates that itself for a verb a route file does not export, before any Sim
code runs. Fixing it means either exporting rejecting handlers from all 77 v2
route files, or building a static path→methods table in proxy.ts that
duplicates the router. Both are worse than the defect; it is recorded as a known
gap in the new skill. Unknown paths are fixed by the catch-all.
limit=0x10 → 16, 1e2 → 100. Inherited from z.coerce.number()'s JS
numeric parsing. Both land inside the accepted range, so the bound is still
enforced. Left alone deliberately.
list-pagination.test.ts already swept every v2 contract and pinned which lists
are paged. It now also asserts, over that same sweep:
limit (this is what caught/audit-logs, which my first pass missed because the assertion was originally.strict()Plus .agents/skills/v2-api-conventions/SKILL.md (projected to
.claude/commands and .cursor/commands by scripts/sync-skills.ts) documenting
the envelope, the status-code table, the pagination contract, and a checklist.
The test's failure messages point at it.
bun run type-check, bun run check:api-validation:strict, bun run check:openapi — passapp/api/v2, lib/api, lib/knowledge, lib/credentials,lib/secrets, lib/workflows, lib/skills, lib/table, lib/uploads — passNot verified locally: no DB was exercised, so the generated keyset SQL is
covered by unit tests and the cursor algebra, not by a real paged read. A human
should confirm on staging: walk nextCursor to the end on each of the five newly
paged collections and check for duplicate or missing rows across page boundaries,
and confirm /api/v2/nonexistent returns the JSON 404.
bun run type-check, bun run lint:check, bun run check:api-validation:strict, bun run check:openapi — all passapp/api/v2, lib/api, lib/knowledge, lib/credentials, lib/secrets, lib/workflows, lib/custom-tools, lib/skills, lib/table, lib/uploads, lib/folders
Sorry, something went wrong.
There was an error while loading. Please reload this page.
… envelope holes
A fractional `limit` reached Postgres as `LIMIT 2.5` and answered 500 on both
`GET /workflows` and `GET /audit-logs`: each list re-declared the param inline,
and these two copies lost their `.int()`. The same divergence left `limit`
validated five different ways and five collections emitting `nextCursor` while
accepting no `limit` at all, or accepting one and silently discarding it.
Adds `v2PaginationFields()` in `contracts/v2/shared.ts` — a bounded integer
`limit` and an opaque `cursor` — and adopts it across all 17 paged lists, so the
family cannot drift again. `/files`, `/logs` and `/tables` keep the truncate-and-
clamp leniency they published, now as an explicit named mode rather than three
hand-rolled copies.
Gives `/skills`, `/custom-tools`, `/secrets`, `/credentials` and `/knowledge`
real pagination using the existing cursor codecs: a keyset for the four whose
page comes from one ordered SQL read, and the offset cursor for `/skills`, whose
merge of the static builtin registry into DB rows cannot be expressed as a SQL
keyset. Each keyset sort now ends in a unique `id`; knowledge tie-broke on
`createdAt`, which cannot separate rows sharing a millisecond.
Two correctness fixes pagination forced: the secrets visibility filter moved from
a post-query JS pass into SQL, because trimming rows after the page is cut
returns fewer than `limit` while `nextCursor` claims more; and the skills list
stopped selecting the 50k-char `content` column only to discard it.
Also restores the canonical error envelope where it had holes: a malformed JSON
body returned a bare `{"error":string}` because the envelope was a per-route
opt-in only 8 of 77 routes remembered, and an unknown `/api/v2` path returned an
HTML 404. Both are now defaults — `V2_PARSE_DEFAULTS` on the builders and the two
raw routes, and a catch-all whose body is byte-identical to the rollout gate's so
an unknown path stays indistinguishable from an ungated one.
Consolidates the keyset paging block (`resumeKeyset`/`keysetPage` in
`list-query.ts`) that had been open-coded in six modules, and folds the bespoke
`InvalidWorkflowListCursorError` into the `OrchestrationError` every other list
already used.
Prevention: the contract sweep in `list-pagination.test.ts` now also asserts that
every paged list rejects a fractional `limit`, that every list query is
`.strict()`, and that the three clamping lists still truncate. The fractional-
limit assertion is what caught `/audit-logs`. Documented in
`.agents/skills/v2-api-conventions/SKILL.md`.
405 responses still carry no `Allow` header — Next.js generates those before any
handler runs. Recorded as a known gap.
There was an error while loading. Please reload this page.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
PR SummaryMedium Risk Overview Envelope fixes: fractional Pagination: Also adds conformance tests, OpenAPI updates, and the Reviewed by Cursor Bugbot for commit f77fddb. Configure here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Greptile SummaryThe PR standardizes pagination and v2 response envelopes across collection endpoints, including shared limit validation, strict query schemas, and JSON transport errors.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported offset-cursor query-state defect is fixed by stamping and validating all reachable sequence-defining parameters.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/v2/lib/response.ts | Adds scoped offset-cursor encoding and strict replay validation; the prior query-state replay defect is addressed. |
| apps/sim/app/api/v2/skills/route.ts | Stamps every reachable skills filter, sort, and workspace parameter into the offset cursor scope. |
| apps/sim/app/api/v2/knowledge/[id]/documents/route.ts | Applies equivalent cursor-scope validation to document pagination using the knowledge-base, filter, search, and sort state. |
| apps/sim/lib/api/contracts/v2/shared.ts | Centralizes standard and clamping pagination schemas with consistent validation and defaults. |
| apps/sim/lib/api/offset-cursor-scope.test.ts | Covers valid replay, changed filters and sorting, malformed offsets, page-size changes, and scope key ordering. |
sequenceDiagram
participant Client
participant Contract
participant Route
participant UseCase
participant Store
Client->>Contract: GET collection with limit, cursor, filters, sort
Contract->>Contract: Validate strict query and pagination fields
Contract->>Route: Parsed query
Route->>Route: Decode cursor and verify query scope
Route->>UseCase: Canonical pagination input
UseCase->>Store: Read limit + 1 ordered records
Store-->>UseCase: Page candidates
UseCase-->>Route: Data, hasMore, pagination state
Route->>Route: Encode next cursor under current scope
Route-->>Client: "{ data, nextCursor }"
Reviews (2): Last reviewed commit: "fix(v2): bind the offset cursor to the q..." | Re-trigger Greptile
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…s in
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.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f77fddb. Configure here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
f77fddbb
Deployed
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.