Skip to content

Commit f7e6cbd

Browse files
committed
fix(v2): serve HEAD, advertise PATCH, and document the reachable 403
Three HTTP-semantics defects on the v2 surface, all found by probing the published contract rather than the happy path. **HEAD answered 500 on every v2 endpoint.** Next implements a missing `HEAD` export by aliasing it onto `GET` and dropping the body when it sends, so a route's `GET` legitimately runs with `request.method === 'HEAD'`. The builders' method guard compared that against the contract's declared method and threw, so `HEAD /api/v2/workflows` and every sibling replied 500 — which is what health checkers, uptime monitors, link checkers, and some CDNs send, all of them reading the API as hard-down. RFC 9110 §9.3.2 makes HEAD identical to GET but for the body, which is exactly what running the GET path produces. Fixed once in `methodMatchesContract`, shared by all five route builders; every other mismatch stays a hard error so a handler exported under the wrong verb still fails loudly. **CORS advertised `GET,POST,OPTIONS,PUT,DELETE`** while the v2 spec has 17 `PATCH` operations, so a browser preflight for any of them was rejected. It also advertised `PUT`, which two operations use — the shape of a hand-maintained list outgrown by its surface. The list stays hand-written because middleware cannot import the contract tree without pulling Zod into the edge bundle, but it is now pinned by a test that sweeps the real contracts and fails on any method it omits. **Six operations omitted a 403 their siblings documented** — three knowledge reads and three file-upload operations. Traced from the code rather than the spec: `requirePermission` throws `NoWorkspaceAccessError` for no access at all (concealed as 404) but `InsufficientWorkspacePermissionsError` for access below `minimumRole` (a real 403), and `PersonalApiKeysDisabledError` reaches every operation a personal API key can call. So 403 was reachable on all six and the omission was an accident of hand-assembled error lists, not a policy. They now use the shared `RESOURCE_ERRORS` / `RESOURCE_CONFLICT_ERRORS` sets, and two operations spelling those same sets by hand were normalized onto them. All 128 documented operations now declare 403. The rules for HEAD, for the 403/404 split, and for using the shared error sets are recorded in `.agents/skills/v2-api-conventions/SKILL.md`.
1 parent 5cf60f6 commit f7e6cbd

17 files changed

Lines changed: 262 additions & 26 deletions

File tree

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,16 @@ Two of these carry real design weight:
6262

6363
**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.
6464

65-
**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.
65+
**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. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.
66+
67+
**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So:
68+
69+
- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403.
70+
- 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.
71+
72+
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.
73+
74+
**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.
6675

6776
## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them
6877

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,16 @@ Two of these carry real design weight:
6161

6262
**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.
6363

64-
**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.
64+
**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. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.
65+
66+
**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So:
67+
68+
- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403.
69+
- 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.
70+
71+
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.
72+
73+
**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.
6574

6675
## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them
6776

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,16 @@ Two of these carry real design weight:
5656

5757
**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.
5858

59-
**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.
59+
**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. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.
60+
61+
**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So:
62+
63+
- An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403.
64+
- 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.
65+
66+
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.
67+
68+
**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.
6069

6170
## Rule 3 — a collection that returns `nextCursor` must accept `limit` + `cursor`, and must apply them
6271

apps/docs/openapi-v2-files-audit.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,9 @@
370370
"401": {
371371
"$ref": "#/components/responses/Unauthorized"
372372
},
373+
"403": {
374+
"$ref": "#/components/responses/Forbidden"
375+
},
373376
"404": {
374377
"$ref": "#/components/responses/NotFound"
375378
},
@@ -468,6 +471,9 @@
468471
"401": {
469472
"$ref": "#/components/responses/Unauthorized"
470473
},
474+
"403": {
475+
"$ref": "#/components/responses/Forbidden"
476+
},
471477
"404": {
472478
"$ref": "#/components/responses/NotFound"
473479
},
@@ -555,6 +561,9 @@
555561
"401": {
556562
"$ref": "#/components/responses/Unauthorized"
557563
},
564+
"403": {
565+
"$ref": "#/components/responses/Forbidden"
566+
},
558567
"404": {
559568
"$ref": "#/components/responses/NotFound"
560569
},

apps/docs/openapi-v2-knowledge.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,9 @@
295295
"401": {
296296
"$ref": "#/components/responses/Unauthorized"
297297
},
298+
"403": {
299+
"$ref": "#/components/responses/Forbidden"
300+
},
298301
"404": {
299302
"$ref": "#/components/responses/NotFound"
300303
},
@@ -672,6 +675,9 @@
672675
"401": {
673676
"$ref": "#/components/responses/Unauthorized"
674677
},
678+
"403": {
679+
"$ref": "#/components/responses/Forbidden"
680+
},
675681
"404": {
676682
"$ref": "#/components/responses/NotFound"
677683
},
@@ -1252,6 +1258,9 @@
12521258
"401": {
12531259
"$ref": "#/components/responses/Unauthorized"
12541260
},
1261+
"403": {
1262+
"$ref": "#/components/responses/Forbidden"
1263+
},
12551264
"404": {
12561265
"$ref": "#/components/responses/NotFound"
12571266
},

apps/sim/app/api/v2/skills/route.test.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ const skill = {
9090
updatedAt: new Date('2026-01-02T00:00:00Z'),
9191
}
9292

93-
function request(method: 'GET' | 'POST', url: string, body?: unknown) {
93+
function request(method: 'GET' | 'POST' | 'HEAD', url: string, body?: unknown) {
9494
return new NextRequest(`http://localhost:3000${url}`, {
9595
method,
9696
headers: {
@@ -179,6 +179,19 @@ describe('/api/v2/skills', () => {
179179
expect(mocks.list).not.toHaveBeenCalled()
180180
})
181181

182+
/**
183+
* Next answers a HEAD by invoking this route's own GET export and dropping the
184+
* body when it sends. The builder's method guard used to reject that, so every
185+
* v2 read replied 500 to a plain HEAD — what health checkers and uptime
186+
* monitors send.
187+
*/
188+
it('serves HEAD through the GET handler instead of throwing', async () => {
189+
const response = await GET(request('HEAD', `/api/v2/skills?workspaceId=${WORKSPACE_ID}`))
190+
191+
expect(response.status).toBe(200)
192+
expect(mocks.list).toHaveBeenCalled()
193+
})
194+
182195
it('rejects a malformed cursor rather than silently restarting at page one', async () => {
183196
const response = await GET(
184197
request('GET', `/api/v2/skills?workspaceId=${WORKSPACE_ID}&cursor=not-a-cursor`)

apps/sim/lib/api/contracts/v2/openapi/files-audit.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,12 @@ import {
2626
ERROR_RESPONSES,
2727
type ErrorResponseId,
2828
RATE_LIMIT_HEADERS,
29+
RESOURCE_CONFLICT_ERRORS,
2930
RESOURCE_ERRORS,
30-
STANDARD_ERRORS,
3131
V2_API_KEY_SECURITY,
3232
V2_API_KEY_SECURITY_SCHEMES,
3333
V2_COMMON_HEADERS,
3434
V2_ERROR_SCHEMA,
35-
VALIDATED_ERRORS,
3635
WORKSPACE_API_KEY_DENIED,
3736
WORKSPACE_API_KEY_DENIED_AS_NOT_FOUND,
3837
WORKSPACE_ERRORS,
@@ -213,7 +212,7 @@ const routes = [
213212
operationId: 'abortFileUpload',
214213
summary: 'Abort File Upload',
215214
description: 'Abort an active upload session and release provider-side multipart state.',
216-
errors: [...VALIDATED_ERRORS, 'NotFound', 'Conflict'],
215+
errors: [...RESOURCE_CONFLICT_ERRORS],
217216
success: { description: 'The aborted upload session.' },
218217
}),
219218
{
@@ -249,7 +248,7 @@ const routes = [
249248
operationId: 'createFileUploadPartUrls',
250249
summary: 'Create File Upload Part URLs',
251250
description: 'Create signed URLs for a bounded set of multipart upload part numbers.',
252-
errors: [...STANDARD_ERRORS, 'BadRequest', 'NotFound', 'Conflict'],
251+
errors: [...RESOURCE_CONFLICT_ERRORS],
253252
success: { description: 'Signed URLs for the requested upload parts.' },
254253
}),
255254
{
@@ -293,7 +292,7 @@ const routes = [
293292
summary: 'Complete File Upload',
294293
description:
295294
'Finalize uploaded bytes, verify provider state, and begin atomic workspace-file registration.',
296-
errors: [...STANDARD_ERRORS, 'BadRequest', 'NotFound', 'Conflict'],
295+
errors: [...RESOURCE_CONFLICT_ERRORS],
297296
success: { description: 'The completed or finalizing upload session.' },
298297
}),
299298
{
@@ -460,7 +459,7 @@ const routes = [
460459
operationId: 'listAuditLogs',
461460
summary: 'List Audit Logs',
462461
description: `List an organization audit trail with filters and opaque cursor pagination. Requires an Enterprise subscription and organization admin or owner access. ${WORKSPACE_API_KEY_DENIED}`,
463-
errors: [...VALIDATED_ERRORS, 'Forbidden', 'NotFound'],
462+
errors: [...RESOURCE_ERRORS],
464463
success: { description: 'A page of audit-log entries.' },
465464
}),
466465
{
@@ -485,7 +484,7 @@ const routes = [
485484
operationId: 'getAuditLog',
486485
summary: 'Get Audit Log',
487486
description: `Return one organization audit-log entry. Requires an Enterprise subscription and organization admin or owner access. ${WORKSPACE_API_KEY_DENIED}`,
488-
errors: [...VALIDATED_ERRORS, 'Forbidden', 'NotFound'],
487+
errors: [...RESOURCE_ERRORS],
489488
success: { description: 'The requested audit-log entry.' },
490489
}),
491490
{

apps/sim/lib/api/contracts/v2/openapi/knowledge.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import {
3131
V2_API_KEY_SECURITY_SCHEMES,
3232
V2_COMMON_HEADERS,
3333
V2_ERROR_SCHEMA,
34-
VALIDATED_ERRORS,
3534
WORKSPACE_ERRORS,
3635
} from '@/lib/api/contracts/v2/openapi/shared'
3736
import {
@@ -116,7 +115,7 @@ const routes = [
116115
operationId: 'getKnowledgeBase',
117116
summary: 'Get Knowledge Base',
118117
description: `Retrieve a knowledge base by identifier. Inaccessible knowledge bases are reported as not found. ${FOLDER_TREE_TOO_LARGE}`,
119-
errors: [...VALIDATED_ERRORS, 'NotFound', 'PayloadTooLarge'],
118+
errors: [...RESOURCE_ERRORS, 'PayloadTooLarge'],
120119
success: { description: 'The requested knowledge base.' },
121120
}),
122121
{
@@ -241,7 +240,7 @@ const routes = [
241240
summary: 'List Documents',
242241
description:
243242
'List documents in a knowledge base with filename search, state filtering, sorting, and opaque cursor pagination.',
244-
errors: [...VALIDATED_ERRORS, 'NotFound'],
243+
errors: [...RESOURCE_ERRORS],
245244
success: { description: 'A page of knowledge documents.' },
246245
}),
247246
{
@@ -478,7 +477,7 @@ const routes = [
478477
operationId: 'getKnowledgeDocument',
479478
summary: 'Get Document',
480479
description: 'Retrieve document detail, processing state, and connector provenance.',
481-
errors: [...VALIDATED_ERRORS, 'NotFound'],
480+
errors: [...RESOURCE_ERRORS],
482481
success: { description: 'The requested knowledge document.' },
483482
}),
484483
{

apps/sim/lib/api/server/routes/definition.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,25 @@ export function requireBinaryRouteDefinition(
6464
}
6565
return { successStatus, successStatuses: [successStatus] }
6666
}
67+
68+
/**
69+
* Whether an incoming request's method is the one its contract declares.
70+
*
71+
* `HEAD` satisfies a `GET` contract because Next aliases a missing `HEAD`
72+
* export straight to the `GET` handler
73+
* (`auto-implement-methods.ts`: `methods.HEAD = handlers.GET`) and then drops
74+
* the body when sending (`send-response.ts` skips the stream when
75+
* `req.method === 'HEAD'`). So the handler legitimately runs with
76+
* `request.method === 'HEAD'` against a `GET` contract, and rejecting that made
77+
* every v2 read answer 500 to a plain `HEAD` — the request health checkers,
78+
* uptime monitors, and link checkers send. RFC 9110 §9.3.2 makes HEAD identical
79+
* to GET but for the body, which is exactly what running the GET path and
80+
* letting the framework strip the body produces.
81+
*
82+
* Everything else stays a hard error: a handler exported under the wrong verb is
83+
* a wiring mistake that should fail loudly rather than serve the wrong contract.
84+
*/
85+
export function methodMatchesContract(requestMethod: string, contractMethod: string): boolean {
86+
if (requestMethod === contractMethod) return true
87+
return requestMethod === 'HEAD' && contractMethod === 'GET'
88+
}

apps/sim/lib/api/server/routes/internal-binary-route.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
import type { Principal, SessionPrincipal } from '@sim/auth/principal'
22
import type { NextRequest } from 'next/server'
33
import { NextResponse } from 'next/server'
4-
import { requireBinaryRouteDefinition } from '@/lib/api/server/routes/definition'
4+
import {
5+
methodMatchesContract,
6+
requireBinaryRouteDefinition,
7+
} from '@/lib/api/server/routes/definition'
58
import {
69
type InternalErrorPolicy,
710
InternalUnauthenticatedError,
@@ -72,7 +75,7 @@ export function defineInternalBinaryRoute<
7275

7376
const wrapped = withRouteHandler<JsonRouteContext | undefined>(
7477
async (request, context) => {
75-
if (request.method !== options.contract.method) {
78+
if (!methodMatchesContract(request.method, options.contract.method)) {
7679
throw new Error(
7780
`Route received ${request.method} for ${options.contract.method} contract ${options.contract.path}`
7881
)

0 commit comments

Comments
 (0)