feat(v2): complete and align the v2 API surface - #6643
Merged
Conversation
Each of these answered a caller-supplied value with a 500 or a silently wrong result instead of a 400. - `GET /api/v2/logs` accepted any string as `startDate`/`endDate`. The route constructs a `Date` from it, so `?startDate=abc` reached the driver's timestamp mapper as an `Invalid Date` and 500'd. Both bounds now carry `.datetime()`, matching the sibling run list so one timestamp works on both collections. This narrows the accepted set: a date without a time and an offset-bearing timestamp are now rejected, and the field descriptions say "UTC ISO 8601" rather than overpromising "ISO 8601". - `v2BillingStatusQuerySchema` was the only non-strict query schema in its family, so a mis-cased `workspaceID` was stripped and the caller got account-scope billing in place of the workspace scope it asked for — a wrong answer about money, served as a 200. - An unresolvable `cursor` on `/api/v2/billing/logs` applied no cursor condition and restarted the sequence at page 1 while still reporting `hasMore`, so a pager holding a cursor across a deploy loops over the first page and counts the same credits on every lap. It is now a 400. The message does not reuse `INVALID_CURSOR_MESSAGE`, which names `sortBy`/`sortOrder` params this collection does not accept. - The logs `status` field disagrees with the run resources for the same run: the run projection overlays `paused` from `paused_executions`, so an ordinary human-in-the-loop pause reads `paused` there and `pending` here. Reconciling would mean joining `paused_executions` in this read and silently moving live runs between two buckets of a shipped field, so the divergence is documented on the contract instead.
Registering an MCP server through v2 dead-ended: nothing on the public
surface ever ran tool discovery, so connectionStatus, toolCount, lastError,
and lastToolsRefresh stayed at their registration defaults and there was no
way to read a server's tools without opening the UI.
Adds GET /api/v2/mcp-servers/{id}/tools over a thin use case composed from
the existing mcp_servers.tools.discover operation, resolveServerContext, and
mcpService.discoverServerTools. It is personal-API-key-only — discovery
resolves the acting user's own OAuth credentials, which a workspace key
cannot supply — and the contract says so rather than letting callers meet an
unexplained 403. Discovery failures are classified instead of collapsing
into a 500: an unreachable or cooling-down server is a retryable 503, a
stale OAuth grant is a 401.
Also pages GET /api/v2/mcp-servers. It was the one unbounded list on the v2
surface, classified full-set on a bounded-by-construction rationale that
only holds for folder lists; nothing caps how many servers a workspace
registers.
…rippable required columns
Three tables gaps from the v2 capability evaluation.
Strictness. Every v2 tables request body is now `.strict()`. The row family
was the whole hole: `POST /query` sent v1's `filter` key answered 200 with a
fully unfiltered page, because Zod strips unknown keys unless told not to. The
same laxity covered the row create/update/delete/upsert/find bodies, the
run and cancel-runs bodies, the enrichment body, and — outside the row family
but the same class — the column delete, view create/update, and export bodies.
A contract sweep now walks every body-bearing tables contract and fails if one
of them stops rejecting an unrecognized key.
Filtered row count. `POST /api/v2/tables/{tableId}/query/count` answers the
question v1's `includeTotal`/`totalCount` answered and the `{data, nextCursor}`
envelope has nowhere to put: how many rows a predicate matches. It binds the
existing `queryTableRows` use case with `includeTotal: true, limit: 1` — no new
domain logic and the same `tables.rows.query` read policy. The use case types
`totalCount` as nullable because paged callers can decline it; this route always
asks for it, so a null is treated as a broken invariant rather than presented as
a fabricated zero.
Required columns. `required` is accepted on create-table, add-column, and
update-column, matching v1. v2 emitted the flag on every read while stripping it
from every write, so a column could not round-trip. Enforcement was already
complete: turning it on over rows with null, missing, or empty cells is rejected
by the domain.
…dental A workspace API key can create a skill it can then never update or delete, which no sibling resource does — so the asymmetry reads like an oversight worth widening. It is not. Skill edits are authorized by the per-skill editor row belonging to the acting user, which is why update/upsert/delete declare a 'read' floor rather than 'write': workspace role is not the authority. A workspace key carries no user subject, so allowing one replaces a 403 with an unclassified PrincipalSubjectUserRequiredError that the v2 surface renders as a caller-reachable 500. Records the reason on the registry and pins it, so the next reader finds the argument instead of flipping the flag.
Two v2 reads that existed only as a side effect of a mutation.
`GET /api/v2/workflows/{id}/deployment` publishes the state the deploy,
undeploy, and rollback responses carry, plus `needsRedeployment` — which
those responses structurally cannot carry, because they answer at the
moment the draft and the live version are equal. A caller that lost the
mutation response, or that polls from another process, had no way to ask.
Reuses `readWorkflowDeploymentStatus` behind `workflows.read`, the same
use case the internal status and deploy GETs already adapt.
`DELETE /api/v2/files/{fileId}` was a soft delete with no way to see what
it archived and no way to reverse it. `GET /api/v2/files?scope=archived`
pages the archived set and `deletedAt` on the file resource dates each
one; `POST /api/v2/files/{fileId}/restore` reverses the delete through
the existing `files.restore` operation. Restore is not a pure undo — it
returns the file to the root and renames it on a collision — so the use
case now reads the file back and both the response and the OpenAPI
description say what actually came back rather than what was deleted.
`scope=all` is rejected on the list for the reason the internal contract
already gives: it drops the `deleted_at` predicate and cannot use the
partial index. `scope=archived` combined with `folderPath` 404s when the
containing folder was archived too, which the contract documents.
The cursor rejection lived in shared billing core but was an OrchestrationError only, which the session-only GET /api/users/me/usage-logs cannot project: that route is raw withRouteHandler and readTypedError matches instanceof HttpError, so any signed-in caller typing ?cursor=x got a 500. UnknownUsageCursorError is an HttpError carrying the OrchestrationError as its cause, so the v2 route still renders BAD_REQUEST off the cause chain and the internal route answers 400. Also closes the other half of the run-list parity: an inverted window on GET /api/v2/logs is now a 400 instead of a silently empty page.
…a rows 400
Review follow-ups on the strictness work.
The sweep was vacuous on the one union body it covers. Parsing
`{ notAContractField: true }` against `v2CreateTableRowsBodySchema` and looking
for `unrecognized_keys` anywhere in the issue tree is satisfied by either member
alone, so dropping `.strict()` from the single-row branch shipped green —
reproduced, 36/36 passing with the regression in place. The sweep now flattens a
union body into its members and asserts each one separately; removing `.strict()`
from either branch now fails a case that names it.
`POST /rows` answered an unknown key with `Invalid input`, the exact message the
v2 conventions name as failing the actionable-error rule, because a union
surfaces `invalid_union` first. The union now carries a message naming both
accepted shapes; the per-member failures still ride along in `details`.
Two TSDoc corrections. The `required` docstring claimed the domain rejects
turning the flag on over rows with empty cells — true of the update path, false
of add-column, which applies the flag as given (the same shape `unique` already
had here). And `.strict()` binds the top level only, so the view `config` object
and the shared sort-spec elements still strip unknown keys; both docstrings now
say so instead of implying full coverage.
The tool-discovery error policy consumed categorizeError's status, whose fallback is a substring match on the upstream message. Three consequences, all caller-visible: - A ZodError from the builder's own response `.parse` contains `invalid_type`, so a Sim-side response-schema defect answered 400 "Invalid request parameters" and suppressed the builder's 500 and its unhandled-error log. - An upstream `Invalid params` or `not found` became the caller's 400/404 on a request the contract had already validated. - A stale OAuth grant to the third-party server answered 401, the status this surface reserves for a missing or invalid Sim API key, so a client would rotate a credential that was never the problem. The policy now dispatches on the MCP error families and returns null for anything else. Reauthorization is a 409 carrying `details.code: MCP_SERVER_REAUTHORIZATION_REQUIRED`; an unreachable, slow, or cooling-down server is a 503 with a constant message. Also: widen the shared server path-param description now that it covers tool listing, map the list query explicitly so no undeclared `cursor` reaches the use-case input, and document the endpoint's write side effects.
v2 accepted tag slots on upload and filtered search by tag display name,
but no response ever returned a tag value and nothing listed the
vocabulary, so a shipped feature dead-ended in the public API. A document
that failed processing could only be deleted and re-uploaded, and
retiring 500 documents cost 500 requests.
- GET /api/v2/knowledge/{id}/tags returns the vocabulary (display name,
slot, field type) as a full-set list.
- Document list and detail responses carry `tags`, keyed by display name
exactly as search keys its result metadata. Writes stay slot-keyed; the
tags endpoint is the mapping and the contract documents the split.
- PATCH /api/v2/knowledge/{id}/documents/{documentId} renames, enables,
disables, retags, or requeues processing. Derived indexing state is not
writable: asserting `processingStatus` on an unindexed document would
corrupt search. A retry may not ride along with field updates.
- PATCH /api/v2/knowledge/{id}/documents bulk-enables or bulk-disables.
Bulk delete is deliberately absent — that operation records no semantic
audit, and a public bulk delete would empty a knowledge base leaving no
DOCUMENT_DELETED entries.
- The document list accepts the same name-based `tagFilters` as search;
the name-to-slot resolver moves out of search into a shared helper, and
the filters are stamped into the offset cursor scope so a replayed
cursor cannot cross a filter change.
- Search accepts `rerankerEnabled`, `rerankerModel`, `rerankerInputCount`
and returns `rerankerScore`; `rerankerApiKey` and `skipUsageBilling`
stay unexposed. Every result now names its `knowledgeBaseId`.
knowledge.tags.list flips from workspaceApiKey 'deny' to 'allow' (and
gains the workspace_api_key principal kind) so it matches the sibling
reads knowledge.documents.list / read / search. The vocabulary is
required input for two operations a workspace key can already perform.
Every tag write stays human-delegated.
…rictness holes
Four cross-cutting consistency gaps on the v2 public surface.
**403s now carry a machine-readable cause.** The conventions skill mandated
`error.details.code` on 403 and nothing emitted one, so a client had to
string-match prose to tell "raise this member's role" from "this workspace
refuses personal keys" from "buy an enterprise plan" — four different
remedies behind one status, and every message reword a silent break. The
vocabulary is a closed set, `FORBIDDEN_DETAIL_CODES`, with a `Record` of
descriptions beside it that the generated OpenAPI 403 description is built
from, so a code cannot reach the wire unpublished. Refusals throw
`ForbiddenOperationError` in the domain and `v2CaughtOrchestrationError` —
the function every v2 error policy falls through to — attaches the code, so a
route cannot forget it. The audit-log resolver distinguished four causes and
collapsed them into one; it now names each.
Cross-tenant refusals deliberately get no code: they are concealed as 404 and
naming their cause would hand back the existence signal the concealment
withholds.
**Two boolean query params rejoin the majority.** `?includeDeparted` and
`?includeOutput` were `'true'`/`'false'` string enums inherited from the
internal shapes they reused, while four sibling params were real booleans.
Both move to `booleanQueryFlagSchema`, which still coerces both strings — a
strict widening, so an existing caller is unaffected, and the spec stops
telling callers to send a string.
**Two nested strictness holes close.** `.strict()` binds the top level only,
so `sort: [{ field, direction, nulls: 'last' }]` was answered 200 with the
null-ordering request dropped, and an unknown key inside a saved view's
`config` was accepted and discarded — the headline `filter` bug one level
down. `sortSpecSchema`'s element and both view-config schemas are now strict.
Safe on the read side because `normalizeStoredViewConfig` projects the
schemaless stored blob onto the declared keys first, so a legacy row cannot
turn into a 500.
The two sort dialects stay as they are. `/logs` and `/workflows/{id}/runs`
have one sortable column, so there is no `sortBy` to pair with; renaming
`order` breaks every caller and an alias is a second spelling of one thing
with undefined precedence. Both contracts and the skill now state the rule.
`turbo run lint:check` runs `biome check .` per workspace, so `scripts/` at the repo root is outside the graph and four changed files were unformatted — one of them a merge artifact from reconciling the route baseline across branches.
…ll-tolerant summary Extracts toV2DocumentSummary in app/api/v2/knowledge/utils.ts and composes the list, upload-acknowledgement and detail presenters from it. toV2TaggedDocument serialized uploadedAt with a bare .toISOString(), so a document with no upload timestamp threw where every sibling returned null and the contract declares the field nullable. Also consolidates the two Zod strictness walkers onto one shared introspection helper that unwraps wrappers and expands unions, closing the hole where a union-shaped schema answered null and was skipped by the pagination sweep.
…eyset page B1: Next aliases HEAD onto GET, which RFC 9110 permits only because GET is safe. The MCP tool-discovery GET is not: it opens a live connection to the registered endpoint and writes the outcome onto the server row. The v2 JSON builder gains a headSafe option, default true, and the discovery route declares itself unsafe — a HEAD is authenticated and rate-limited, then answered bodiless. B2: a discovery status write stamped updatedAt, which this branch added as a keyset sort, so any concurrent discovery duplicated and skipped servers across a caller's pages. Discovery liveness already has lastConnected, lastToolsRefresh, lastError and statusConfig. B4: a public refresh now skips the positive cache but keeps the failure cooldown, so it cannot be used to drive a connection attempt per request at a failing endpoint. An explicit user action on their own server keeps the full bypass. B6: the consecutive-failure counter is incremented SQL-side rather than read, incremented and written back, and the success branch carries the same workspace, liveness and staleness guard the failure branch already had.
…the docs true
B3: a selectAll bulk document update echoed every changed identifier, which the
request does not bound — a 100k-document knowledge base produced a multi-megabyte
array, materialized and then element-wise validated. The use case now reports
whether the selection was unbounded and the presenter omits the echo.
A1: the knowledge search presenter spread the whole use-case result, which also
carries userId, workspaceId, a cost breakdown and a live secret-trace registry.
Only Zod's default key-stripping kept them off the wire. Projected explicitly.
P1-a: GET /knowledge/{id}/tags advertised all 17 slots while the document PATCH
accepted only the seven text ones. The writer already coerces every slot type,
so the PATCH now takes all 17 in their declared types, with a 400 where a
malformed value used to silently clear the tag.
P1-b: both new PATCHes deny workspace API keys and now say so.
P1-c: the two table query reads declare maxBodyBytes and now document the 413.
P1-d: getWorkflowDeploymentV2 loses its legacy suffix.
C3: deletes two orchestration error mappers with no callers that mapped
'forbidden' with no details.
D2: a stored null in table_views.config survived the pick and failed the
response schema.
Also folds the six 'bounded set' paraphrases onto one FULL_SET_LIST constant,
shares the run-window date bound between the logs and runs lists so their
documented parity is enforced rather than asserted, adds the missing barrel
export for FORBIDDEN_DETAIL_CODE_DESCRIPTIONS, and strictens two response
schemas whose peers were already strict.
Migrates 40 v2 route tests onto the shared @sim/testing harness: 26 asserted a
rateLimitSubjectIds shape v2 auth never returns, 26 asserted the wrong
refillRate, 33 could not exercise their 401 path at all, and 6 hard-wired the
rollout gate to null.
…ver-claiming B5: the connect clamp was getMaxExecutionTimeout(), the workflow ceiling of seven days, so the real bound became the server row's own timeout — which the registration contract permits up to 300s — times the connect retries. A slow server could hold a Node request for roughly twenty minutes. Connecting is not a workflow run, so the handshake now shares the one-minute ceiling tools/list already applies to itself. C2: the generated 403 description asserted that error.details.code names the cause on every 403. Nine domain refusals still throw a bare forbidden OrchestrationError and reach the wire codeless, so the wording now says 'where the cause is one a caller can act on'. Reparenting those throws is left as a deliberate change: one of them is a cross-tenant refusal that belongs in the codeless class and would change its status.
# Conflicts: # scripts/check-api-validation-contracts.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview New and extended operations include Contract and behavior fixes standardize 403 with machine-readable Tests move many v2 route tests to shared Reviewed by Cursor Bugbot for commit d673626. Configure here. |
Collaborator
Author
Contributor
Greptile SummaryThis PR completes and aligns the v2 API surface, adding MCP tool discovery, table row counts, file restoration, and workflow deployment-state reads while standardizing pagination, validation, errors, and response projections.
Confidence Score: 5/5The PR appears safe to merge; no concrete blocking or independently actionable non-blocking issue remained after reviewing the changed API contracts and execution paths. The checked MCP, table, knowledge, pagination, restoration, and deployment changes preserve authorization and response contracts, guard side effects, normalize legacy data where strict schemas apply, and retain compatibility with existing callers.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/api/server/routes/v2-json-route.ts | Adds side-effect-free HEAD handling before parsing and use-case execution while preserving authentication and rate limiting. |
| apps/sim/lib/mcp/service.ts | Separates MCP liveness updates from configuration timestamps and improves concurrent discovery-state handling. |
| apps/sim/lib/mcp/queries.ts | Introduces stable keyset pagination for MCP server lists with unique tie-breakers. |
| apps/sim/lib/api/contracts/v2/tables.ts | Adds the filtered row-count contract, strict request schemas, and round-trippable required-column fields. |
| apps/sim/lib/table/views/service.ts | Normalizes legacy persisted view configuration before strict API response validation. |
| apps/sim/app/api/v2/tables/[tableId]/query/count/route.ts | Implements filtered row counts through the same authorized query and predicate path used for row retrieval. |
| apps/sim/lib/api/contracts/v2/knowledge.ts | Tightens typed tag-slot validation and bounds bulk-update response fields. |
| apps/sim/app/api/v2/knowledge/search/route.ts | Projects only public search-result fields instead of exposing internal use-case metadata. |
| apps/sim/lib/api/list-query.ts | Adds explicit timestamp parameter casts so replayed keyset cursors resolve correctly in PostgreSQL. |
| apps/sim/app/api/v2/files/[fileId]/restore/route.ts | Adds the v2 file-restore operation using the shared route, authorization, and response machinery. |
| apps/sim/app/api/v2/workflows/[id]/deployment/route.ts | Adds deployment-state retrieval alongside the existing deployment mutation behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[API client] --> Route[V2 route contract]
Route --> Auth[Authentication and rate limiting]
Auth --> Validation[Strict input validation]
Validation --> UseCase[Application use case]
UseCase --> Domain[(Domain and persistence services)]
Domain --> Presenter[Canonical response projection]
Presenter --> Envelope[V2 data or error envelope]
Envelope --> Client
Contract[Shared v2 contracts] -.-> Route
Contract -.-> Presenter
Contract -.-> OpenAPI[Generated OpenAPI specifications]
Reviews (1): Last reviewed commit: "style: sort imports and format the three..." | Re-trigger Greptile
…not the application barrel The barrel also re-exports the authorized use-case layer, which loads @sim/db at import time. That pulled a database connection into the OpenAPI spec check, so check:audits failed wherever DATABASE_URL is absent, including CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the v2 surface so it reads as one API rather than seven independently-grown resource families. Builds on the eleven PRs already merged into
staging; this is the consistency and capability pass on top of them.What changed
New capability — 128 → 135 operations
Correctness
HEADno longer drives MCP tool discovery. Next aliases a missingHEADontoGET, so aHEADwas silently running the discovery effect. The route now opts out viaheadSafe: falseand answers with no effect and no body —HEADis defined as safe in RFC 9110 §9.2.1, and this is the only side-effectingGETon the v2 surface.updatedAtkeyset page no longer corrupts. A background discovery was stampingupdatedAt, which both moved rows out from under an in-flight cursor and reported a change that never happened.updatedAtnow means "config last changed"; liveness stays onlastConnected/lastToolsRefresh/lastError.Consistency
limit.FORBIDDEN_DETAIL_CODESunion.nulland silently cleared the tag with a 200; it is now a 400.Test-fixture correction
26 test files mocked
rateLimitSubjectIdsas['workspace:<id>']. Auth always returns two subjects —['api-key:<keyId>', 'user:<userId>']or['api-key:<keyId>', 'workspace:<wsId>']. The mocks asserted a shape the code never produces, and that masked a real bug:tables/[tableId]/querylimits per subject, sooperationRateis called twice, and only the understated fixture let the one-call assertion pass.Two review findings were rejected with evidence
.refine()does not hide.strict(). In Zod 4.refine()is a check on the schema, not a wrapper — all four flagged schemas still reportdef.type === 'object',catchall: never, and removing a.strict()turns the existing sweep red. The real hole was next door: the strictness walker returnednullfor unions andnullwas skipped, so union-shaped queries opted out silently. One shared walker now handles wrappers and unions, with a union strict only when every member is.RestoreWorkspaceFileResult.restoredis not dead — the internal route consumes it viainternalJsonPresenters.successFrom('restored').Verification
type-check·biome check(direct, all changed files) ·check:api-validation:strict·check:openapi(7 specs, 135 operations, 137 contracts, 259 examples) ·check-route-verbs— all green. 6502 tests pass across the affected paths.Each behavior fix has a captured red proof: reverting it fails the new test.
Notes for review