fix(v2): serve HEAD, advertise PATCH, and document the reachable 403 - #6623
Conversation
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`.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Route builders now use Default v2 API convention docs (agents, Claude, Cursor) are extended with guidance on caller-reachable 500s, when to document 403 vs 404, shared error sets, and HEAD handling. Reviewed by Cursor Bugbot for commit 50c2cb0. Configure here. |
Greptile SummaryThe PR corrects HTTP semantics across the v2 API and consolidates error-response declarations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/api/server/routes/definition.ts | Adds a narrow method matcher that accepts only exact matches and the framework-supported HEAD-to-GET pairing. |
| apps/sim/lib/api/server/routes/v2-json-route.ts | Uses the shared method matcher so v2 JSON GET contracts can serve automatic HEAD requests. |
| apps/sim/lib/api/server/routes/v2-binary-route.ts | Applies the same HEAD-to-GET method semantics to binary routes. |
| apps/sim/lib/api/server/routes/v2-body-lifecycle-route.ts | Applies the shared method matcher while preserving all other method mismatches as errors. |
| apps/sim/lib/api/server/routes/internal-json-route.ts | Aligns internal JSON route method validation with Next.js HEAD behavior. |
| apps/sim/lib/api/server/routes/internal-binary-route.ts | Aligns internal binary route method validation with Next.js HEAD behavior. |
| apps/sim/proxy.ts | Expands default CORS methods and exposes operational response headers to browser clients. |
| apps/sim/lib/api/contracts/v2/openapi/shared.ts | Removes narrower error-set building blocks in favor of shared workspace-resource response sets. |
| apps/sim/lib/api/contracts/v2/openapi/files-audit.ts | Normalizes file and audit operation errors onto shared resource sets and documents reachable forbidden responses. |
| apps/sim/lib/api/contracts/v2/openapi/knowledge.ts | Normalizes knowledge operation errors and adds reachable forbidden responses to the generated contract. |
| scripts/check-route-verbs.ts | Updates route-verification documentation to account for the sole permitted HEAD-to-GET method pairing. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
HEAD["HEAD request"] --> Next["Next.js aliases HEAD to GET"]
Next --> Match["methodMatchesContract(HEAD, GET)"]
Match --> Handler["GET contract handler"]
Handler --> Strip["Next.js sends headers without body"]
Browser["Browser preflight"] --> Proxy["API proxy"]
Proxy --> Methods["Allow GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS"]
Proxy --> Expose["Expose retry, rate-limit, request, and run headers"]
Auth["Authorization failures"] --> Shared["Shared resource error sets"]
Shared --> Spec["Generated OpenAPI documents reachable 403"]
Reviews (3): Last reviewed commit: "fix(cors): expose the API response heade..." | Re-trigger Greptile
`proxy.test.ts` pinned the previous hand-written method string, so widening `resolveApiCorsPolicy` to advertise PATCH and HEAD left it asserting a list the middleware no longer returns. The literal is kept rather than imported from `proxy.ts` so the test still pins the exact wire value independently of the implementation.
The three knowledge reads and three upload operations lost their `403` by assembling `[...VALIDATED_ERRORS, ...]` by hand, and `VALIDATED_ERRORS` / `STANDARD_ERRORS` were the only exported sets that omit `Forbidden`. Migrating the last consumers to the shared `RESOURCE_*` sets left both unreferenced, so deleting them turns the fix from a one-time cleanup into an invariant: there is no longer a building block from which a workspace-scoped operation can assemble an error list without `Forbidden`. Regenerating the specs produces no diff, so the migration is output-neutral. Also folds `method-match.test.ts` into `definition.test.ts` to match the repo's `feature.ts` -> `feature.test.ts` convention, types `contractMethod` as `HttpMethod` so a contract declaring `HEAD` is unrepresentable, and drops the duplicated Next-aliasing rationale so `methodMatchesContract`'s TSDoc is its single home.
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 19e8b01. Configure here.
Without `Access-Control-Expose-Headers` a browser can read only the six CORS-safelisted response headers, so the rate-limit budget, the `Retry-After` a 429 or 503 asks the caller to observe, and the request/run correlation ids were all on the wire but invisible to `fetch()`. Server-to-server callers were unaffected, which is why it went unnoticed. Exposed on the default `/api` policy only. The per-route `CORS_RULES` entries are wildcard-origin public endpoints and opt in individually if they ever need it, so this does not widen what an anonymous cross-origin caller can read from them.
5e7f08a to
50c2cb0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 50c2cb0. Configure here.
What
Three HTTP-semantics defects on the v2 surface, found by probing the published
contract rather than the happy path. Follow-up to #6620, on top of the merged
pagination/envelope work.
HEAD /api/v2/workflows(and every v2 endpoint)PATCHopsPATCHnot advertisedGET /knowledge/{id}+ 5 siblings403undocumented but reachableHEAD returned 500 on every v2 endpoint
Next implements a missing
HEADexport by aliasing it straight ontoGET(
auto-implement-methods.ts:methods.HEAD = handlers.GET) and drops the bodyat send time (
send-response.tsskips the stream whenreq.method === 'HEAD').So a route's
GETlegitimately runs withrequest.method === 'HEAD'.Every route builder guards that the request method equals the contract's
declared method — the check that makes a handler exported under the wrong verb
fail loudly. It rejected the framework's own HEAD aliasing, threw, and
withRouteHandlerturned that into a 500. That is why the response carried ourx-request-idand a JSON content-type: it reached our handler and threw.RFC 9110 §9.3.2 makes HEAD identical to GET but for the body, which is exactly
what running the GET path and letting Next strip the body produces — so mirroring
GET is right, and a 405 would be wrong for a read endpoint. Fixed once in
methodMatchesContractand shared by all five builders (both v2 and internal —the internal builders carried the identical guard). Every other mismatch stays a
hard error.
Practical impact: health checkers, uptime monitors, link checkers, and some
proxies/CDNs issue HEAD, and every one of them read the v2 API as hard-down.
CORS omitted PATCH
The default
/apipolicy advertisedGET,POST,OPTIONS,PUT,DELETEwhile the v2spec has 17
PATCHoperations, so a browser preflight for any of them wasrejected. It also advertised
PUT, which only two operations use — the shape ofa hand-maintained list that its surface outgrew.
Blast radius today is limited because
access-control-allow-originechoes onlythe site's own origin, so cross-origin browser use is not enabled; this becomes
live the moment CORS is widened or a first-party client PATCHes cross-origin.
The list stays hand-written on purpose —
proxy.tsis edge middleware andimporting
lib/api/contractswould pull Zod and the whole contract tree into themiddleware bundle. Instead it is pinned by a test that sweeps the real
contracts and fails on any declared method the header omits, which is the check
that would have caught this.
HEADis added too, now that it is genuinely served.Six operations omitted a reachable 403
GET /knowledge/{id},GET /knowledge/{id}/documents,GET /knowledge/{id}/documents/{documentId}, and the three/files/uploads/{uploadId}operations omitted403while their siblings on thesame paths documented it.
Settled from the code, not by pattern-matching the spec.
requirePermissionthrows two different failures:
NoWorkspaceAccessError, whichcreateV2ResourceConcealmentPolicyconceals as 404minimumRole→InsufficientWorkspacePermissionsError,which is not in the concealment set and stays a 403
So the "reads conceal with 404 and can never 403" theory does not hold. The three
upload operations are
minimumRole: 'write', so a member withreadhits a 403directly. The three knowledge reads are
minimumRole: 'read'— the floor of theread < write < adminordering, so that path cannot 403 — but they are stillreachable by
PersonalApiKeysDisabledError(a personal API key against aworkspace whose organization disabled them), which is also not concealed. 403 is
reachable on all six.
They now use the shared
RESOURCE_ERRORS/RESOURCE_CONFLICT_ERRORSsetsrather than hand-assembled lists, which is how the codes went missing. Two
operations that spelled those same sets by hand (
listAuditLogs,getAuditLog)were normalized onto them — byte-identical output, one fewer way to drift.
All 128 documented operations now declare 403.
Verification
bun run type-check,bun run lint:check,bun run check:api-validation:strict,bun run check:openapi— passapp/api,lib/api,lib/knowledge,lib/workspace-filesbun run generate:openapi; never hand-editedexpected 500 to be 200, reproducing the staging symptom exactly, and theCORS sweep fails with
expected [ 'PATCH' ] to deeply equal []Not verified locally: HEAD behaviour is asserted against the builder and one
real route handler, but the framework's own body-stripping is not exercised by a
unit test. A human should confirm on staging after deploy that
HEAD /api/v2/workflowsreturns 200 with no body and the same headers as GET.Type of Change
Testing
See Verification above.
Checklist