fix(api): give every v1 endpoint quota headers and errors that name the field - #6012
Conversation
…he field Three consistency gaps found by probing the live v1 surface end to end. Rate-limit headers were only published by routes built on createApiResponse — workflows, logs and audit-logs. Tables, files and knowledge are rate limited by the same bucket and will return 429, but published no quota on success, so a client discovered the ceiling only by hitting it. Adds a shared rateLimitHeaders() builder, reused by createRateLimitResponse, and attaches it to all 31 success responses on those three families. Missing required fields did not name themselves. .min(1, '...') only fires for a present-but-empty string, so an omitted field fell through to Zod's default "Invalid input: expected string, received undefined". A previous pass fixed the shared id schemas, but 22 of 23 v1 workspaceId declarations bypassed them, so the fix reached almost nothing. Adds requiredFieldSchema(message) and routes every v1 request input through it, preserving each site's existing, more specific wording (for example "workspaceId query parameter is required") instead of flattening them to the generic one. Response schemas are left alone — "required" wording would be wrong there. Validation failures on tables, files and knowledge reported the literal "Validation error" and discarded the schema's message. Adds v1ValidationErrorResponse, which surfaces the first issue while keeping details, and wires it into the 19 parseRequest calls that had no handler. Routes with deliberately specific wording keep theirs. The global default is untouched, since routes outside v1 assert the current string.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Rate limits: Authenticated v1 handlers now record a per-request snapshot when Validation errors: Adds Reviewed by Cursor Bugbot for commit 3a22a75. Configure here. |
Greptile SummaryFollow-up pass on v1 API consistency for quota headers and field-named validation errors.
Confidence Score: 5/5Safe to merge; prior incomplete validation-header and download rate-limit gaps are addressed and no blocking failures remain. Table row mutations now surface schema field messages via v1ValidationErrorResponse, and file download responses receive X-RateLimit headers through checkRateLimit snapshots applied in withRouteHandler. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/utils/with-route-handler.ts | Applies request-id and recorded rate-limit headers on success and unhandled-error responses. |
| apps/sim/lib/api/server/rate-limit-context.ts | Request-scoped WeakMap snapshot plus shared X-RateLimit header builder. |
| apps/sim/app/api/v1/middleware.ts | Records rate-limit snapshots on check; adds v1 validation error helpers. |
| apps/sim/app/api/v1/tables/[tableId]/rows/route.ts | Row list/create/update/delete parseRequest paths use v1ValidationErrorResponse. |
| apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts | PATCH (and other handlers) pass v1ValidationErrorResponse for contract failures. |
| apps/sim/app/api/v1/files/[fileId]/route.ts | Download/delete validation uses v1 helper; download binary responses get quota headers from withRouteHandler. |
Reviews (6): Last reviewed commit: "fix(api): stop the last v1 validation pa..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Review found three places the first pass missed, all from filters that worked
on whole files instead of individual call sites.
- GET /api/v1/files/{fileId} returns the file bytes via `new Response`, not a
`success: true` JSON body, so the header pass skipped it. The download now
carries the same quota headers as the DELETE beside it.
- Four parseRequest calls in the table-row routes still reported the generic
"Validation error". The first pass skipped any file that already had a
handler anywhere in it, which excluded these two files wholesale. The check
is now per call site, and no bare call remains.
- POST /api/v1/tables takes its body from the shared tables contract, which
still used the bare `.min(1)` form, so an omitted workspaceId did not name
itself. Converted there and in the other v1-reachable contracts.
Scope note: roughly a thousand `.min(1, '... is required')` declarations remain
under contracts/tools/**. Those are block and tool definitions rather than the
public REST surface, and converting them belongs in its own change.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 51f0104. Configure here.
… sites
A quality pass found the previous commit only made the happy path consistent.
Those three route families have 117 response sites; 32 got headers. The other
85 are the error paths — 400/403/404/500 — which are exactly the responses a
client is deciding whether to retry, and they published no quota at all.
`checkRateLimit` now records the bucket snapshot against the request, and
`withRouteHandler` attaches the headers next to the `x-request-id` it already
sets, on both the success and the unhandled-error branch. Every v1 response
carries the quota now, and a new v1 route gets it without remembering to. The
carrier is a WeakMap keyed by the request, so it needs no cleanup and routes
that never record a snapshot — everything outside v1 — are untouched.
This deletes more than it adds: the 32 decorations are gone, and so is the
`rateLimit` parameter that had been threaded into `handleBatchInsert` purely so
a business-logic helper could decorate its own response.
Also from the same pass:
- One definition of the header trio. `createApiResponse` had its own copy, so
after the last commit there were two; both now build from
`buildRateLimitHeaders`.
- `v1ValidationErrorResponse` delegates to the shared `validationErrorResponse`
instead of hand-rolling the same body, and takes a fallback message, which
collapses four route closures that differed only in that string.
- 36 sites wrote `requiredFieldSchema('Workspace ID is required')` — the
verbatim definition of the exported `workspaceIdSchema`. Using the primitive
is the whole point of having it; they now import it.
- Dropped TSDoc that had gone stale or contradicted the call sites it advised.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 607ea4a. Configure here.
The comment pass caught a real casualty of the previous commit: inserting `applyResponseHeaders` put it between `withRouteHandler`'s docblock and the function itself, so the file's most-used export lost its documentation to the new private helper. Reattached, and its header bullet now mentions the rate-limit trio it also emits. Remaining edits are wording only. The WeakMap rationale moved off `RateLimitSnapshot` — three self-evident fields — onto the `snapshots` declaration it actually describes. The record site no longer restates that rationale; it keeps only the part unique to it. And three id-schema docs claimed "same constraint as nonEmptyIdSchema", which stopped being true when that schema was documented as deliberately message-less.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 b912fd9. Configure here.
The spec already asserted, in the RateLimited description, that the X-RateLimit-* trio accompanies every authenticated response. Before this branch that was false for tables, files and knowledge; it is true now, but no operation documented it — only 1 of 40 v1 success responses carried the headers. All 40 now reference the shared header components. The shared BadRequest, Forbidden and NotFound components are deliberately left alone: they are also $ref-ed by non-v1 operations that publish no quota, so annotating them there would over-claim. The RateLimited description carries the general rule instead, now stating explicitly that the only responses without the headers are the ones that failed authentication.
|
@cursor review |
Bugbot found GET /api/v1/tables/{tableId}/rows still answering with the bare
"Validation error". Its handler special-cases malformed filter/sort JSON and
then falls back to the shared helper — so the site looked handled to a check
that only asked whether a handler existed, which is why the earlier call-level
sweep passed over it.
Auditing the whole class turned up more of the same shape:
- The optional-body parses on deploy and rollback, where a bad `version` lost
"version must be a positive integer".
- Eleven catch-block `validationErrorResponseFromError` handlers across the
table routes, which discard the message of any ZodError thrown deeper.
Adds `v1ValidationErrorResponseFromError` as the v1 counterpart for unknown
caught values, and routes every remaining v1 validation path through the v1
helpers. No call to the generic helpers survives under app/api/v1 outside
admin, which keeps its own error envelope.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 3a22a75. Configure here.
1 participant
Summary
Three consistency gaps found by probing the live v1 surface end to end after #6011.
1. Rate-limit headers were missing on tables / files / knowledge. Only routes built on
createApiResponse(workflows, logs, audit-logs) published them:Those endpoints share the same bucket and will return 429 — clients just couldn't see the ceiling until they hit it. Adds a shared
rateLimitHeaders()builder (reused bycreateRateLimitResponse) and attaches it to all 31 success responses across the three families.2. #6011's error-message fix reached almost nothing.
.min(1, '...')only fires for a present-but-empty string, so an omitted field fell through to Zod's default. I fixed the shared id schemas last time — but 22 of 23 v1workspaceIddeclarations bypassed them, so/workflowswithoutworkspaceIdstill returnedInvalid input: expected string, received undefined. AddsrequiredFieldSchema(message)and routes every v1 request input through it.Deliberately preserves each site's more specific wording (
'workspaceId query parameter is required') rather than flattening to the generic message — and leaves response schemas alone, where "required" wording would be wrong.3. tables/files/knowledge reported a bare
"Validation error"and threw away the schema's message. Addsv1ValidationErrorResponse, wired into the 19parseRequestcalls that had no handler. Routes with intentionally specific messages keep theirs.I did not change the global
validationErrorResponsedefault — routes outside v1 (copilot, providers) assert that literal in their tests, so a blanket change would have been the wrong blast radius.Not included
Two things I found and deliberately left alone: the two response envelopes (
{data, limits}vs{data, success}) are inconsistent but correctly documented in OpenAPI, and unifying them is breaking; and/tables/{id}returning 400 rather than 404 for an unknown id follows from itsworkspaceId-scoped addressing.Type of Change
Testing
472 tests pass across
app/api/v1/,lib/api/,app/api/workflows/andlib/core/rate-limiter/. New coverage forrateLimitHeaders(consistent pair, empty when no bucket was consulted) andv1ValidationErrorResponse(surfaces the message, keepsdetails).Three route test files mocked
@/app/api/v1/middlewareand needed the new exports added to their mocks — caught by the suite, fixed.Typecheck, lint,
check:api-validation:strictand the monorepo boundary check all pass.Checklist