Skip to content

Commit 1fa40b8

Browse files
authored
feat(v2): complete and align the v2 API surface (#6643)
* fix(v2): close four validation holes in the logs and billing surfaces 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. * feat(v2): expose the MCP tool plane and page the MCP server list 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. * feat(v2/tables): strict row bodies, a filtered row count, and round-trippable 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. * test(skills): pin the workspace-API-key split as structural, not accidental 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. * feat(v2): read deployment state, and undo a file delete 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. * fix(v2): keep the unresolvable-cursor rejection a 400 on every surface 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. * fix(v2/tables): sweep union bodies per member and name the shapes on 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. * fix(v2): classify MCP discovery failures by type, not by substring 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. * merge: bring in the MCP tool plane workstream * feat(v2): make knowledge tags usable and let documents be updated 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. * fix(v2): name every 403 cause, unfork boolean params, close nested strictness 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. * style: format the files the workspace-scoped lint gate does not reach `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. * fix(v2): collapse the four knowledge document projections onto one null-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. * fix(v2): stop HEAD driving MCP discovery, and unbreak the updatedAt keyset 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. * fix(v2): bound the bulk update echo, close the search leak, and make 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. * fix(mcp): bound the connect handshake, and stop the 403 description over-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. * chore: reconcile the route ratchet with staging * style: sort imports and format the three files biome flagged * fix(openapi): import the forbidden-code constants from their module, 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.
1 parent 128054e commit 1fa40b8

142 files changed

Lines changed: 8481 additions & 1842 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.agents/skills/v2-api-conventions/SKILL.md

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ A route built with `defineV2JsonRoute` gets this for free: its `present` returns
5252
| 200 / 201 || Success. 201 only for a created resource. |
5353
| 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. |
5454
| 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** |
55-
| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). |
55+
| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carries a machine-readable `details.code` from the closed set in `lib/core/application/forbidden.ts` (e.g. `INSUFFICIENT_WORKSPACE_ROLE`, `PERSONAL_API_KEYS_DISABLED`). |
5656
| 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. |
5757
| 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. |
5858
| 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. |
@@ -74,6 +74,12 @@ And this class survives a green test suite — `keysetAfter` returned well-forme
7474
- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403.
7575
- An operation whose `minimumRole` is `read` cannot 403 *that* way, because `read` is the floor of the `read < write < admin` ordering and anyone without access is concealed as 404 instead. It can still 403 through `PersonalApiKeysDisabledError` (a personal API key against a workspace whose organization disabled them) or `WorkspaceApiKeyAuthorizationError` (`workspaceApiKey: 'deny'`), and every v2 operation is reachable by a personal API key. **So in practice every workspace-scoped v2 operation documents 403**, and the reads that omitted it were wrong, not principled.
7676

77+
**Every 403 names its cause in `error.details.code`.** One status covers four different remedies — raise a member's role, issue a personal key instead of a workspace-scoped one, re-point a workspace key, buy an enterprise plan — and prose is not branchable, so a client that must tell them apart was string-matching messages, which turns every reword into a silent break.
78+
79+
The vocabulary is a closed set, `FORBIDDEN_DETAIL_CODES` in `lib/core/application/forbidden.ts`, with a `Record` of descriptions beside it that the generated OpenAPI 403 description is built from. Adding a member fails to compile until it is documented, so a code cannot reach the wire unpublished. Do not invent a code at a route: throw `ForbiddenOperationError(code, message)` from the domain and let `v2CaughtOrchestrationError` — the function every v2 error policy falls through to — attach it. `InsufficientWorkspacePermissionsError`, `PersonalApiKeysDisabledError`, `WorkspaceApiKeyAuthorizationError`, and `PrincipalKindAuthorizationError` already carry theirs.
80+
81+
The cross-tenant refusals (`NoWorkspaceAccessError`, `WorkspaceApiKeyScopeAuthorizationError`, `DelegatedWorkspaceAuthorizationError`) deliberately carry **no** code. They are concealed as 404, and naming their cause would hand back the resource-existence signal the concealment exists to withhold.
82+
7783
Use the shared sets in `contracts/v2/openapi/shared.ts``RESOURCE_ERRORS`, `RESOURCE_CONFLICT_ERRORS`, `RESOURCE_MUTATION_ERRORS` — rather than assembling a per-operation list; all three already include `Forbidden`, and hand-assembled lists are how three knowledge reads and three upload operations quietly lost it.
7884

7985
**HEAD is answered by the `GET` handler, not rejected.** Next aliases a missing `HEAD` export onto `GET` and drops the body when sending, so a route's `GET` legitimately runs with `request.method === 'HEAD'`. The builders' method guard accepts that pairing via `methodMatchesContract`; any other mismatch stays a hard error. Never hand-write a `HEAD` export to "fix" this.
@@ -99,9 +105,15 @@ Two cursor schemes exist, both opaque base64-JSON from `response.ts`. Which one
99105

100106
Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side.
101107

108+
**Ordering is `sortBy` + `sortOrder`, except where there is nothing to sort by.** Fourteen lists take the pair. Two — `GET /logs` and `GET /workflows/{id}/runs` — have exactly one sortable column (start time), so there is no `sortBy` to pair with and the direction rides on a single `order` param; `sortBy`/`sortOrder` are not accepted there. That split is documented in both contracts and is the *only* sanctioned deviation. A new list picks the pair. Do not "fix" the two by accepting `sortOrder` as an alias: an alias is a second spelling of one thing with undefined precedence when both arrive, which is its own inconsistency, and renaming `order` would break every shipped caller.
109+
110+
**A boolean query param is a real boolean**, declared with `booleanQueryFlagSchema` from `contracts/primitives.ts`. It coerces `'true'`/`'1'` and `'false'`/`'0'`/`''`, so it is a strict widening of a `z.enum(['true','false'])` — which is what two v2 params used to be, purely by inheritance from the internal shapes they reused. Reusing an internal `.shape.x` inherits the internal spelling; re-declare instead when the internal one is not the v2 convention.
111+
102112
## Rule 4 — reject what you do not implement
103113

104-
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.
114+
Query and body schemas are **`.strict()`** — and `.strict()` binds the **top level only**. A strict body containing a non-strict nested object still drops unknown keys one level down, which is the headline `filter` bug at a smaller scale: `sort: [{ field, direction, nulls: 'last' }]` answered 200 and ordered by the default. Strictness belongs on the shared nested schema (`sortSpecSchema`'s element, `tableViewConfigSchema`), not restated per body.
115+
116+
Before tightening a schema that is **also** a response or a stored blob, make the read canonical first. `table_views.config` is schemaless JSONB, so a legacy row carrying a retired key would fail a newly strict response parse and become a 500; `normalizeStoredViewConfig` projects the stored blob onto the declared keys so the tightening is safe in both directions. 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.
105117

106118
Error messages name the field and, where there is one, the escape hatch:
107119

@@ -194,12 +206,14 @@ Run this against any new or changed v2 endpoint.
194206
- [ ] The list is classified in `list-pagination.test.ts`.
195207
- [ ] Cross-tenant access answers 404, never 403 — and carries `Cache-Control: private, no-store`, because RFC 9110 §15.5.5 makes 404 heuristically cacheable and an authorization-dependent 404 must never be stored. `v2Error` sets this unconditionally; do not build a v2 response any other way.
196208
- [ ] A retryable failure says when: 429 and 503 carry `Retry-After`. No other status invents one.
197-
- [ ] 403s carry a machine-readable `details.code`.
209+
- [ ] 403s carry a machine-readable `details.code` from `FORBIDDEN_DETAIL_CODES`, thrown as `ForbiddenOperationError` in the domain rather than attached at the route.
210+
- [ ] Nested objects inside a `.strict()` body are strict too — `.strict()` does not recurse.
211+
- [ ] Ordering uses `sortBy` + `sortOrder`; boolean query params use `booleanQueryFlagSchema`.
198212
- [ ] Validation messages name the field and echo the valid set.
199213
- [ ] Response schema matches every field the route actually emits.
200214
- [ ] OpenAPI description regenerated and truthful about pagination.
201215
- [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass.
202216

203217
## Known gap
204218

205-
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.
219+
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 every v2 route file 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.

.claude/commands/v2-api-conventions.md

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ A route built with `defineV2JsonRoute` gets this for free: its `present` returns
5151
| 200 / 201 || Success. 201 only for a created resource. |
5252
| 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. |
5353
| 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** |
54-
| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). |
54+
| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carries a machine-readable `details.code` from the closed set in `lib/core/application/forbidden.ts` (e.g. `INSUFFICIENT_WORKSPACE_ROLE`, `PERSONAL_API_KEYS_DISABLED`). |
5555
| 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. |
5656
| 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. |
5757
| 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. |
@@ -73,6 +73,12 @@ And this class survives a green test suite — `keysetAfter` returned well-forme
7373
- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403.
7474
- An operation whose `minimumRole` is `read` cannot 403 *that* way, because `read` is the floor of the `read < write < admin` ordering and anyone without access is concealed as 404 instead. It can still 403 through `PersonalApiKeysDisabledError` (a personal API key against a workspace whose organization disabled them) or `WorkspaceApiKeyAuthorizationError` (`workspaceApiKey: 'deny'`), and every v2 operation is reachable by a personal API key. **So in practice every workspace-scoped v2 operation documents 403**, and the reads that omitted it were wrong, not principled.
7575

76+
**Every 403 names its cause in `error.details.code`.** One status covers four different remedies — raise a member's role, issue a personal key instead of a workspace-scoped one, re-point a workspace key, buy an enterprise plan — and prose is not branchable, so a client that must tell them apart was string-matching messages, which turns every reword into a silent break.
77+
78+
The vocabulary is a closed set, `FORBIDDEN_DETAIL_CODES` in `lib/core/application/forbidden.ts`, with a `Record` of descriptions beside it that the generated OpenAPI 403 description is built from. Adding a member fails to compile until it is documented, so a code cannot reach the wire unpublished. Do not invent a code at a route: throw `ForbiddenOperationError(code, message)` from the domain and let `v2CaughtOrchestrationError` — the function every v2 error policy falls through to — attach it. `InsufficientWorkspacePermissionsError`, `PersonalApiKeysDisabledError`, `WorkspaceApiKeyAuthorizationError`, and `PrincipalKindAuthorizationError` already carry theirs.
79+
80+
The cross-tenant refusals (`NoWorkspaceAccessError`, `WorkspaceApiKeyScopeAuthorizationError`, `DelegatedWorkspaceAuthorizationError`) deliberately carry **no** code. They are concealed as 404, and naming their cause would hand back the resource-existence signal the concealment exists to withhold.
81+
7682
Use the shared sets in `contracts/v2/openapi/shared.ts``RESOURCE_ERRORS`, `RESOURCE_CONFLICT_ERRORS`, `RESOURCE_MUTATION_ERRORS` — rather than assembling a per-operation list; all three already include `Forbidden`, and hand-assembled lists are how three knowledge reads and three upload operations quietly lost it.
7783

7884
**HEAD is answered by the `GET` handler, not rejected.** Next aliases a missing `HEAD` export onto `GET` and drops the body when sending, so a route's `GET` legitimately runs with `request.method === 'HEAD'`. The builders' method guard accepts that pairing via `methodMatchesContract`; any other mismatch stays a hard error. Never hand-write a `HEAD` export to "fix" this.
@@ -98,9 +104,15 @@ Two cursor schemes exist, both opaque base64-JSON from `response.ts`. Which one
98104

99105
Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side.
100106

107+
**Ordering is `sortBy` + `sortOrder`, except where there is nothing to sort by.** Fourteen lists take the pair. Two — `GET /logs` and `GET /workflows/{id}/runs` — have exactly one sortable column (start time), so there is no `sortBy` to pair with and the direction rides on a single `order` param; `sortBy`/`sortOrder` are not accepted there. That split is documented in both contracts and is the *only* sanctioned deviation. A new list picks the pair. Do not "fix" the two by accepting `sortOrder` as an alias: an alias is a second spelling of one thing with undefined precedence when both arrive, which is its own inconsistency, and renaming `order` would break every shipped caller.
108+
109+
**A boolean query param is a real boolean**, declared with `booleanQueryFlagSchema` from `contracts/primitives.ts`. It coerces `'true'`/`'1'` and `'false'`/`'0'`/`''`, so it is a strict widening of a `z.enum(['true','false'])` — which is what two v2 params used to be, purely by inheritance from the internal shapes they reused. Reusing an internal `.shape.x` inherits the internal spelling; re-declare instead when the internal one is not the v2 convention.
110+
101111
## Rule 4 — reject what you do not implement
102112

103-
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.
113+
Query and body schemas are **`.strict()`** — and `.strict()` binds the **top level only**. A strict body containing a non-strict nested object still drops unknown keys one level down, which is the headline `filter` bug at a smaller scale: `sort: [{ field, direction, nulls: 'last' }]` answered 200 and ordered by the default. Strictness belongs on the shared nested schema (`sortSpecSchema`'s element, `tableViewConfigSchema`), not restated per body.
114+
115+
Before tightening a schema that is **also** a response or a stored blob, make the read canonical first. `table_views.config` is schemaless JSONB, so a legacy row carrying a retired key would fail a newly strict response parse and become a 500; `normalizeStoredViewConfig` projects the stored blob onto the declared keys so the tightening is safe in both directions. 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.
104116

105117
Error messages name the field and, where there is one, the escape hatch:
106118

@@ -193,12 +205,14 @@ Run this against any new or changed v2 endpoint.
193205
- [ ] The list is classified in `list-pagination.test.ts`.
194206
- [ ] Cross-tenant access answers 404, never 403 — and carries `Cache-Control: private, no-store`, because RFC 9110 §15.5.5 makes 404 heuristically cacheable and an authorization-dependent 404 must never be stored. `v2Error` sets this unconditionally; do not build a v2 response any other way.
195207
- [ ] A retryable failure says when: 429 and 503 carry `Retry-After`. No other status invents one.
196-
- [ ] 403s carry a machine-readable `details.code`.
208+
- [ ] 403s carry a machine-readable `details.code` from `FORBIDDEN_DETAIL_CODES`, thrown as `ForbiddenOperationError` in the domain rather than attached at the route.
209+
- [ ] Nested objects inside a `.strict()` body are strict too — `.strict()` does not recurse.
210+
- [ ] Ordering uses `sortBy` + `sortOrder`; boolean query params use `booleanQueryFlagSchema`.
197211
- [ ] Validation messages name the field and echo the valid set.
198212
- [ ] Response schema matches every field the route actually emits.
199213
- [ ] OpenAPI description regenerated and truthful about pagination.
200214
- [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass.
201215

202216
## Known gap
203217

204-
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.
218+
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 every v2 route file or intercepting in `apps/sim/proxy.ts` with a static path→methods table. Neither is done. Unknown *paths* are handled — the catch-all covers those.

0 commit comments

Comments
 (0)