From d8af1abd3ce7c54c2d2485daf24b54855c093057 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 19:15:55 -0700 Subject: [PATCH 1/2] fix(api): collapse the internal error envelope and restore requestId The builders shipped two internal error envelopes: internalOrchestrationErrorPolicy emitted { success: false, error } while internalPlainOrchestrationErrorPolicy emitted { error }. That split approximated pre-builder behavior, where the shape depended on which branch failed - guard clauses returned { error } and a route's terminal try/catch returned { success: false, error }. A per-route policy cannot express a per-branch rule, so the two disagreed on the same status across families. Collapse to the bare { error } shape. It is what messageFromErrorBody reads on the client and what most migrated routes already emitted. requestJson throws ApiClientError for any non-2xx, so no typed client ever observes the discriminator. success: true on success bodies is a separate contract and is untouched. Also restore requestId to internal error bodies. withRouteHandler stamps it on the bodies it generates, but the builder overrides dropped it, leaving it only on the x-request-id header - invisible when a user pastes an error out of the UI. It is now applied at the createJsonErrorResponse chokepoint and in both wrapper overrides, and is omitted when there is no active request scope. --- apps/sim/app/api/audit-logs/route.ts | 4 +- .../app/api/table/[tableId]/exports/route.ts | 4 +- .../api/table/[tableId]/groups/route.test.ts | 2 +- .../app/api/table/[tableId]/groups/route.ts | 4 +- .../exports/[exportId]/download/route.ts | 4 +- .../app/api/table/exports/[exportId]/route.ts | 6 +-- .../imports/[importId]/complete/route.ts | 4 +- .../table/imports/[importId]/parts/route.ts | 4 +- .../app/api/table/imports/[importId]/route.ts | 6 +-- apps/sim/app/api/table/imports/route.ts | 4 +- .../api/table/table-transfer-routes.test.ts | 2 +- .../app/api/workflows/[id]/deployed/route.ts | 4 +- apps/sim/app/api/workflows/[id]/route.test.ts | 2 +- apps/sim/app/api/workflows/[id]/route.ts | 6 +-- .../[id]/files/[fileId]/content/route.test.ts | 1 - .../[id]/files/[fileId]/csv-preview/route.ts | 2 +- .../[id]/files/[fileId]/route.test.ts | 2 - .../[id]/files/[fileId]/share/route.test.ts | 4 +- .../files/folders/[folderId]/route.test.ts | 2 - .../[id]/files/folders/route.test.ts | 1 - .../workspaces/[id]/files/move/route.test.ts | 1 - .../api/workspaces/[id]/files/route.test.ts | 3 +- apps/sim/lib/api/server/routes/index.ts | 1 - .../server/routes/internal-binary-route.ts | 11 ++-- .../server/routes/internal-json-route.test.ts | 53 +++++++++++++++---- .../api/server/routes/internal-json-route.ts | 40 +++++++------- .../lib/api/server/routes/request-id.test.ts | 45 ++++++++++++++++ apps/sim/lib/api/server/routes/request-id.ts | 24 +++++++++ apps/sim/lib/knowledge/api/route-policies.ts | 8 +-- apps/sim/lib/table/api/route-policies.test.ts | 4 +- .../api/internal-error-policies.test.ts | 2 +- .../api/internal-error-policies.ts | 13 ++--- 32 files changed, 183 insertions(+), 90 deletions(-) create mode 100644 apps/sim/lib/api/server/routes/request-id.test.ts create mode 100644 apps/sim/lib/api/server/routes/request-id.ts diff --git a/apps/sim/app/api/audit-logs/route.ts b/apps/sim/app/api/audit-logs/route.ts index b944bfb9d52..f2cc96a2b3f 100644 --- a/apps/sim/app/api/audit-logs/route.ts +++ b/apps/sim/app/api/audit-logs/route.ts @@ -1,7 +1,7 @@ import { listAuditLogsContract } from '@/lib/api/contracts/audit-logs' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, internalSessionAuth, } from '@/lib/api/server/routes' @@ -18,7 +18,7 @@ export const GET = defineInternalJsonRoute({ rateLimit: internalRateLimits.none({ reason: 'Existing authenticated audit-log settings read has no request-rate policy', }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ query }) => ({ organizationId: query.organizationId, includeDeparted: query.includeDeparted, diff --git a/apps/sim/app/api/table/[tableId]/exports/route.ts b/apps/sim/app/api/table/[tableId]/exports/route.ts index 228fc3be0db..0fcb3b2538f 100644 --- a/apps/sim/app/api/table/[tableId]/exports/route.ts +++ b/apps/sim/app/api/table/[tableId]/exports/route.ts @@ -1,7 +1,7 @@ import { createTableExportResourceContract } from '@/lib/api/contracts/table-transfers' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -16,7 +16,7 @@ export const POST = defineInternalJsonRoute({ rateLimit: internalRateLimits.none({ reason: 'Existing authenticated table export creation has no request-rate policy', }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, body }) => ({ tableId: params.tableId, workspaceId: body.workspaceId, diff --git a/apps/sim/app/api/table/[tableId]/groups/route.test.ts b/apps/sim/app/api/table/[tableId]/groups/route.test.ts index 5f717198604..7a774f8434d 100644 --- a/apps/sim/app/api/table/[tableId]/groups/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/groups/route.test.ts @@ -36,7 +36,7 @@ vi.mock('@/lib/api/server/routes', () => ({ }, extendInternalErrorPolicy: vi.fn(() => ({ kind: 'table' })), internalErrorResponse: vi.fn(), - internalPlainOrchestrationErrorPolicy: { kind: 'plain' }, + internalOrchestrationErrorPolicy: { kind: 'plain' }, internalRateLimits: { none: ({ reason }: { reason: string }) => ({ kind: 'none', reason }), }, diff --git a/apps/sim/app/api/table/[tableId]/groups/route.ts b/apps/sim/app/api/table/[tableId]/groups/route.ts index 4e56d8b509c..66c2d00adde 100644 --- a/apps/sim/app/api/table/[tableId]/groups/route.ts +++ b/apps/sim/app/api/table/[tableId]/groups/route.ts @@ -7,7 +7,7 @@ import { defineInternalJsonRoute, extendInternalErrorPolicy, internalErrorResponse, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -21,7 +21,7 @@ import { TableLockedError } from '@/lib/table/mutation-locks' import type { TableDefinition } from '@/lib/table/types' import { normalizeColumn } from '@/app/api/table/utils' -const errorPolicy = extendInternalErrorPolicy(internalPlainOrchestrationErrorPolicy, (error) => +const errorPolicy = extendInternalErrorPolicy(internalOrchestrationErrorPolicy, (error) => error instanceof TableLockedError ? internalErrorResponse(423, { error: error.message, lock: error.lock }) : null diff --git a/apps/sim/app/api/table/exports/[exportId]/download/route.ts b/apps/sim/app/api/table/exports/[exportId]/download/route.ts index 71c9ca300c3..9731e8a4a0d 100644 --- a/apps/sim/app/api/table/exports/[exportId]/download/route.ts +++ b/apps/sim/app/api/table/exports/[exportId]/download/route.ts @@ -1,7 +1,7 @@ import { downloadTableExportResourceContract } from '@/lib/api/contracts/table-transfers' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -15,7 +15,7 @@ export const GET = defineInternalJsonRoute({ rateLimit: internalRateLimits.none({ reason: 'Existing authenticated table export download signing has no request-rate policy', }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, query }) => ({ exportId: params.exportId, workspaceId: query.workspaceId, diff --git a/apps/sim/app/api/table/exports/[exportId]/route.ts b/apps/sim/app/api/table/exports/[exportId]/route.ts index 5e4d3a8c14c..bc0be516ddd 100644 --- a/apps/sim/app/api/table/exports/[exportId]/route.ts +++ b/apps/sim/app/api/table/exports/[exportId]/route.ts @@ -4,7 +4,7 @@ import { } from '@/lib/api/contracts/table-transfers' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -21,7 +21,7 @@ export const GET = defineInternalJsonRoute({ auth: internalTableSessionOrExecutorAuth, operation: tableOperations.readExport, rateLimit, - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, query }) => ({ exportId: params.exportId, workspaceId: query.workspaceId, @@ -35,7 +35,7 @@ export const DELETE = defineInternalJsonRoute({ auth: internalTableSessionOrExecutorAuth, operation: tableOperations.cancelExport, rateLimit, - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, query }) => ({ exportId: params.exportId, workspaceId: query.workspaceId, diff --git a/apps/sim/app/api/table/imports/[importId]/complete/route.ts b/apps/sim/app/api/table/imports/[importId]/complete/route.ts index 6952511835d..06336349d4f 100644 --- a/apps/sim/app/api/table/imports/[importId]/complete/route.ts +++ b/apps/sim/app/api/table/imports/[importId]/complete/route.ts @@ -1,7 +1,7 @@ import { completeTableImportResourceContract } from '@/lib/api/contracts/table-transfers' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -16,7 +16,7 @@ export const POST = defineInternalJsonRoute({ rateLimit: internalRateLimits.none({ reason: 'Existing authenticated table import completion has no request-rate policy', }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, query, headers }) => ({ importId: params.importId, workspaceId: query.workspaceId, diff --git a/apps/sim/app/api/table/imports/[importId]/parts/route.ts b/apps/sim/app/api/table/imports/[importId]/parts/route.ts index 4a3b4261c60..3317e2c602f 100644 --- a/apps/sim/app/api/table/imports/[importId]/parts/route.ts +++ b/apps/sim/app/api/table/imports/[importId]/parts/route.ts @@ -1,7 +1,7 @@ import { createTableImportPartUrlsContract } from '@/lib/api/contracts/table-transfers' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -15,7 +15,7 @@ export const POST = defineInternalJsonRoute({ rateLimit: internalRateLimits.none({ reason: 'Existing authenticated table import part signing has no request-rate policy', }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, query, headers, body }) => ({ importId: params.importId, workspaceId: query.workspaceId, diff --git a/apps/sim/app/api/table/imports/[importId]/route.ts b/apps/sim/app/api/table/imports/[importId]/route.ts index 60dd58dc125..7a7ffd6584f 100644 --- a/apps/sim/app/api/table/imports/[importId]/route.ts +++ b/apps/sim/app/api/table/imports/[importId]/route.ts @@ -4,7 +4,7 @@ import { } from '@/lib/api/contracts/table-transfers' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -21,7 +21,7 @@ export const GET = defineInternalJsonRoute({ auth: internalTableSessionOrExecutorAuth, operation: tableOperations.readImport, rateLimit, - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, query }) => ({ importId: params.importId, workspaceId: query.workspaceId, @@ -35,7 +35,7 @@ export const DELETE = defineInternalJsonRoute({ auth: internalTableSessionOrExecutorAuth, operation: tableOperations.cancelImport, rateLimit, - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params, query, headers }) => ({ importId: params.importId, workspaceId: query.workspaceId, diff --git a/apps/sim/app/api/table/imports/route.ts b/apps/sim/app/api/table/imports/route.ts index 143207c2e5e..9bb37c360f6 100644 --- a/apps/sim/app/api/table/imports/route.ts +++ b/apps/sim/app/api/table/imports/route.ts @@ -1,7 +1,7 @@ import { createTableImportResourceContract } from '@/lib/api/contracts/table-transfers' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' @@ -16,7 +16,7 @@ export const POST = defineInternalJsonRoute({ rateLimit: internalRateLimits.none({ reason: 'Existing authenticated table import creation has no request-rate policy', }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ body }) => ({ body }), useCase: createTableImportUseCase, present: ({ import: created }) => ({ data: toV2CreateTableImport(created) }), diff --git a/apps/sim/app/api/table/table-transfer-routes.test.ts b/apps/sim/app/api/table/table-transfer-routes.test.ts index 7511900d81f..ca8c8ca6b6c 100644 --- a/apps/sim/app/api/table/table-transfer-routes.test.ts +++ b/apps/sim/app/api/table/table-transfer-routes.test.ts @@ -35,7 +35,7 @@ vi.mock('@/lib/api/server/routes', () => ({ mocks.definitions.push(definition) return vi.fn() }, - internalPlainOrchestrationErrorPolicy: { kind: 'plain' }, + internalOrchestrationErrorPolicy: { kind: 'plain' }, internalRateLimits: { none: ({ reason }: { reason: string }) => ({ kind: 'none', reason }), }, diff --git a/apps/sim/app/api/workflows/[id]/deployed/route.ts b/apps/sim/app/api/workflows/[id]/deployed/route.ts index 6a9ad7d1bc4..1df1cefeb2c 100644 --- a/apps/sim/app/api/workflows/[id]/deployed/route.ts +++ b/apps/sim/app/api/workflows/[id]/deployed/route.ts @@ -5,7 +5,7 @@ import { } from '@/lib/api/contracts/deployments' import { defineInternalJsonRoute, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { internalWorkflowSessionOrExecutorAuth } from '@/lib/workflows/api' @@ -23,7 +23,7 @@ export const GET = defineInternalJsonRoute({ rateLimit: internalRateLimits.none({ reason: 'Preserve existing internal workflow read behavior', }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params }) => ({ workflowId: params.id, state: 'deployed' as const }), useCase: readWorkflowDefinition, present: ({ state }) => ({ diff --git a/apps/sim/app/api/workflows/[id]/route.test.ts b/apps/sim/app/api/workflows/[id]/route.test.ts index e86f3046733..c219c102d66 100644 --- a/apps/sim/app/api/workflows/[id]/route.test.ts +++ b/apps/sim/app/api/workflows/[id]/route.test.ts @@ -20,7 +20,7 @@ vi.mock('@/lib/api/server', () => ({ parseRequest: mocks.parseRequest })) vi.mock('@/lib/api/server/routes', () => ({ defineInternalJsonRoute: mocks.defineRoute, InternalUnauthenticatedError: class InternalUnauthenticatedError extends Error {}, - internalPlainOrchestrationErrorPolicy: { kind: 'plain-orchestration' }, + internalOrchestrationErrorPolicy: { kind: 'plain-orchestration' }, internalRateLimits: { none: vi.fn(() => ({ kind: 'none' })) }, })) diff --git a/apps/sim/app/api/workflows/[id]/route.ts b/apps/sim/app/api/workflows/[id]/route.ts index 7fd22761d93..ff1bc691125 100644 --- a/apps/sim/app/api/workflows/[id]/route.ts +++ b/apps/sim/app/api/workflows/[id]/route.ts @@ -12,7 +12,7 @@ import { parseRequest } from '@/lib/api/server' import { defineInternalJsonRoute, InternalUnauthenticatedError, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes' import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' @@ -37,7 +37,7 @@ export const GET = defineInternalJsonRoute({ auth: internalWorkflowReadAuth, operation: readWorkflowDefinition.operation, rateLimit: workflowInternalRateLimit, - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params }) => ({ workflowId: params.id, state: 'draft' as const }), useCase: readWorkflowDefinition, present: ({ workflow: workflowData, state }) => { @@ -82,7 +82,7 @@ export const DELETE = defineInternalJsonRoute({ auth: internalWorkflowSessionOrExecutorAuth, operation: deleteWorkflow.operation, rateLimit: workflowInternalRateLimit, - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: ({ params }) => ({ workflowId: params.id }), useCase: deleteWorkflow, present: () => ({ success: true as const }), diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts index 3cfcc4664c0..c1fbd5c74f5 100644 --- a/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/content/route.test.ts @@ -143,7 +143,6 @@ describe('PUT /api/workspaces/[id]/files/[fileId]/content', () => { expect(response.status).toBe(402) await expect(response.json()).resolves.toEqual({ - success: false, error: 'Storage limit exceeded', }) }) diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts index de4b8c6104f..802191d380f 100644 --- a/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts @@ -14,7 +14,7 @@ export const GET = defineInternalJsonRoute({ auth: internalSessionOrExecutorAuth, operation: csvPreviewWorkspaceFile.operation, rateLimit: internalRateLimits.none({ reason: 'Preserve existing internal CSV preview behavior' }), - errorPolicy: internalFileErrorPolicies.plain, + errorPolicy: internalFileErrorPolicies.default, mapInput: ({ params, query }) => ({ fileId: params.fileId, assertedWorkspaceId: params.id, diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.test.ts index da7a2a60614..fd31638e313 100644 --- a/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.test.ts @@ -126,7 +126,6 @@ describe('PATCH /api/workspaces/[id]/files/[fileId]', () => { expect(response.status).toBe(403) expect(await response.json()).toEqual({ - success: false, error: 'Insufficient workspace permissions', }) expect(mocks.captureServerEvent).not.toHaveBeenCalled() @@ -139,7 +138,6 @@ describe('PATCH /api/workspaces/[id]/files/[fileId]', () => { expect(response.status).toBe(500) expect(await response.json()).toEqual({ - success: false, error: 'Internal server error', }) }) diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/share/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/share/route.test.ts index 2fe55b10776..3cb9bc235cf 100644 --- a/apps/sim/app/api/workspaces/[id]/files/[fileId]/share/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/share/route.test.ts @@ -96,7 +96,7 @@ describe('/api/workspaces/[id]/files/[fileId]/share', () => { const response = await GET(getRequest(), context) expect(response.status).toBe(403) - expect(await response.json()).toEqual({ success: false, error: 'Access denied' }) + expect(await response.json()).toEqual({ error: 'Access denied' }) }) it('renders resource absence as 404', async () => { @@ -142,7 +142,7 @@ describe('/api/workspaces/[id]/files/[fileId]/share', () => { const response = await PUT(putRequest({ isActive: true }), context) expect(response.status).toBe(400) - expect(await response.json()).toEqual({ success: false, error: 'Password is required' }) + expect(await response.json()).toEqual({ error: 'Password is required' }) }) it('preserves the internal caller-supplied token field for compatibility', async () => { diff --git a/apps/sim/app/api/workspaces/[id]/files/folders/[folderId]/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/folders/[folderId]/route.test.ts index 019a6aacc55..8140e1f09e0 100644 --- a/apps/sim/app/api/workspaces/[id]/files/folders/[folderId]/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/folders/[folderId]/route.test.ts @@ -112,7 +112,6 @@ describe('/api/workspaces/[id]/files/folders/[folderId]', () => { expect(response.status).toBe(409) await expect(response.json()).resolves.toEqual({ - success: false, error: 'A folder named "Reports" already exists in this location', }) expect(mocks.captureServerEvent).not.toHaveBeenCalled() @@ -140,7 +139,6 @@ describe('/api/workspaces/[id]/files/folders/[folderId]', () => { expect(response.status).toBe(404) expect(await response.json()).toEqual({ - success: false, error: `Workspace file items not found (folders: ${FOLDER_ID})`, }) expect(mocks.captureServerEvent).not.toHaveBeenCalled() diff --git a/apps/sim/app/api/workspaces/[id]/files/folders/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/folders/route.test.ts index a0d229bc1c0..603549affd6 100644 --- a/apps/sim/app/api/workspaces/[id]/files/folders/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/folders/route.test.ts @@ -113,7 +113,6 @@ describe('/api/workspaces/[id]/files/folders', () => { expect(response.status).toBe(409) await expect(response.json()).resolves.toEqual({ - success: false, error: 'A folder named "Reports" already exists in this location', }) expect(mocks.captureServerEvent).not.toHaveBeenCalled() diff --git a/apps/sim/app/api/workspaces/[id]/files/move/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/move/route.test.ts index f3b96fe2f26..c1700fc83a5 100644 --- a/apps/sim/app/api/workspaces/[id]/files/move/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/move/route.test.ts @@ -83,7 +83,6 @@ describe('/api/workspaces/[id]/files/move', () => { expect(response.status).toBe(409) await expect(response.json()).resolves.toEqual({ - success: false, error: 'A file named "report.csv" already exists in the destination folder', }) expect(mocks.captureServerEvent).not.toHaveBeenCalled() diff --git a/apps/sim/app/api/workspaces/[id]/files/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/route.test.ts index 5add87b4660..4220a1cd9d8 100644 --- a/apps/sim/app/api/workspaces/[id]/files/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/route.test.ts @@ -146,13 +146,12 @@ describe('/api/workspaces/[id]/files', () => { mocks.createFile.mockRejectedValueOnce(new OrchestrationError('conflict', 'Name exists')) const conflict = await POST(createRequest({ name: 'notes.md' }), context) expect(conflict.status).toBe(409) - expect(await conflict.json()).toEqual({ success: false, error: 'Name exists' }) + expect(await conflict.json()).toEqual({ error: 'Name exists' }) mocks.createFile.mockRejectedValueOnce(new Error('database details')) const unexpected = await POST(createRequest({ name: 'notes.md' }), context) expect(unexpected.status).toBe(500) expect(await unexpected.json()).toEqual({ - success: false, error: 'Internal server error', }) }) diff --git a/apps/sim/lib/api/server/routes/index.ts b/apps/sim/lib/api/server/routes/index.ts index 03951db9c67..e93cf272631 100644 --- a/apps/sim/lib/api/server/routes/index.ts +++ b/apps/sim/lib/api/server/routes/index.ts @@ -9,7 +9,6 @@ export { internalErrorResponse, internalJsonPresenters, internalOrchestrationErrorPolicy, - internalPlainOrchestrationErrorPolicy, internalRateLimits, internalSessionAuth, } from '@/lib/api/server/routes/internal-json-route' diff --git a/apps/sim/lib/api/server/routes/internal-binary-route.ts b/apps/sim/lib/api/server/routes/internal-binary-route.ts index 33ca0a13574..34845b53b3b 100644 --- a/apps/sim/lib/api/server/routes/internal-binary-route.ts +++ b/apps/sim/lib/api/server/routes/internal-binary-route.ts @@ -7,6 +7,7 @@ import { InternalUnauthenticatedError, type internalSessionAuth, } from '@/lib/api/server/routes/internal-json-route' +import { withRequestId } from '@/lib/api/server/routes/request-id' import type { BinaryApiRouteContract, BinaryResponseDescriptor, @@ -111,10 +112,10 @@ export function defineInternalBinaryRoute< } }, { - typedErrorResponse: ({ error, status }) => - NextResponse.json({ error: error.message }, { status }), - unhandledErrorResponse: () => - NextResponse.json({ error: 'Internal server error' }, { status: 500 }), + typedErrorResponse: ({ error, status, requestId }) => + NextResponse.json({ error: error.message, requestId }, { status }), + unhandledErrorResponse: ({ requestId }) => + NextResponse.json({ error: 'Internal server error', requestId }, { status: 500 }), } ) @@ -122,7 +123,7 @@ export function defineInternalBinaryRoute< } function createJsonErrorResponse(descriptor: JsonErrorResponseDescriptor): NextResponse { - return NextResponse.json(descriptor.body, { + return NextResponse.json(withRequestId(descriptor.body), { status: descriptor.status, headers: descriptor.headers, }) diff --git a/apps/sim/lib/api/server/routes/internal-json-route.test.ts b/apps/sim/lib/api/server/routes/internal-json-route.test.ts index c57b07879ed..40ee26c5629 100644 --- a/apps/sim/lib/api/server/routes/internal-json-route.test.ts +++ b/apps/sim/lib/api/server/routes/internal-json-route.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { getRequestContext } from '@sim/logger' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' import { z } from 'zod' @@ -8,12 +9,14 @@ import { defineRouteContract } from '@/lib/api/contracts' import { defineInternalJsonRoute, internalErrorResponse, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, internalRateLimits, } from '@/lib/api/server/routes/internal-json-route' import { OrchestrationError } from '@/lib/core/orchestration/types' import { HttpError } from '@/lib/core/utils/http-error' +const mockGetRequestContext = vi.mocked(getRequestContext) + class TestLockedError extends HttpError { readonly statusCode = 423 } @@ -39,6 +42,7 @@ const contract = defineRouteContract({ describe('defineInternalJsonRoute', () => { beforeEach(() => { vi.clearAllMocks() + mockGetRequestContext.mockReturnValue(undefined) }) it('uses the use-case result directly when it already matches the contract', async () => { @@ -47,7 +51,7 @@ describe('defineInternalJsonRoute', () => { auth, operation, rateLimit: internalRateLimits.none({ reason: 'Unit test' }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: () => undefined, useCase: { operation, @@ -70,7 +74,7 @@ describe('defineInternalJsonRoute', () => { auth, operation, rateLimit: internalRateLimits.none({ reason: 'Unit test' }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: () => undefined, useCase: { operation, @@ -92,7 +96,7 @@ describe('defineInternalJsonRoute', () => { auth, operation, rateLimit: internalRateLimits.none({ reason: 'Unit test' }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: () => undefined, useCase: { operation, @@ -105,7 +109,10 @@ describe('defineInternalJsonRoute', () => { const response = await handler(new NextRequest('http://localhost/api/test/internal-json-route')) expect(response.status).toBe(423) - await expect(response.json()).resolves.toEqual({ error: 'Table imports are locked' }) + await expect(response.json()).resolves.toEqual({ + error: 'Table imports are locked', + requestId: expect.any(String), + }) expect(response.headers.get('x-request-id')).toBeTruthy() }) @@ -115,6 +122,32 @@ describe('defineInternalJsonRoute', () => { ) }) + it('projects a classified orchestration error as a bare error envelope', async () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-orchestration' }) + + const handler = defineInternalJsonRoute({ + contract, + auth, + operation, + rateLimit: internalRateLimits.none({ reason: 'Unit test' }), + errorPolicy: internalOrchestrationErrorPolicy, + mapInput: () => undefined, + useCase: { + operation, + async execute() { + throw new OrchestrationError('not_found', 'Widget not found') + }, + }, + }) + + const response = await handler(new NextRequest('http://localhost/api/test/internal-json-route')) + const body = await response.json() + + expect(response.status).toBe(404) + expect(body).toEqual({ error: 'Widget not found', requestId: 'req-orchestration' }) + expect(body).not.toHaveProperty('success') + }) + it('orders auth, rate limiting, parsing, async mapping, and application execution', async () => { const events: string[] = [] const orderedContract = defineRouteContract({ @@ -142,7 +175,7 @@ describe('defineInternalJsonRoute', () => { events.push('rate') }, }, - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, async mapInput({ body }) { events.push('map:start') await Promise.resolve() @@ -176,7 +209,7 @@ describe('defineInternalJsonRoute', () => { auth, operation, rateLimit: internalRateLimits.none({ reason: 'Unit test' }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, async mapInput() { await Promise.resolve() throw new OrchestrationError('validation', 'Invalid mapped input') @@ -198,7 +231,7 @@ describe('defineInternalJsonRoute', () => { auth, operation, rateLimit: internalRateLimits.none({ reason: 'Unit test' }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: () => undefined, useCase: { operation, @@ -222,7 +255,7 @@ describe('defineInternalJsonRoute', () => { auth, operation, rateLimit: internalRateLimits.none({ reason: 'Unit test' }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: () => undefined, useCase: { operation, @@ -256,7 +289,7 @@ describe('defineInternalJsonRoute', () => { auth, operation, rateLimit: internalRateLimits.none({ reason: 'Unit test' }), - errorPolicy: internalPlainOrchestrationErrorPolicy, + errorPolicy: internalOrchestrationErrorPolicy, mapInput: () => undefined, useCase: { operation, diff --git a/apps/sim/lib/api/server/routes/internal-json-route.ts b/apps/sim/lib/api/server/routes/internal-json-route.ts index aff46bae3dc..fae641945e9 100644 --- a/apps/sim/lib/api/server/routes/internal-json-route.ts +++ b/apps/sim/lib/api/server/routes/internal-json-route.ts @@ -8,6 +8,7 @@ import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' import type { ContractJsonResponse } from '@/lib/api/contracts' import { requireJsonRouteDefinition } from '@/lib/api/server/routes/definition' +import { withRequestId } from '@/lib/api/server/routes/request-id' import type { JsonApiRouteContract, JsonErrorResponseDescriptor, @@ -116,18 +117,24 @@ export interface InternalErrorPolicy { unhandled?(): JsonErrorResponseDescriptor } +/** + * The single internal error envelope: `{ error, requestId? }`. + * + * Routes previously chose between a bare `{ error }` and a `{ success: false, + * error }` variant. That split approximated pre-builder behavior, where the + * shape depended on which branch failed — guard clauses returned `{ error }` + * while a route's terminal `try/catch` returned `{ success: false, error }`. + * A per-route policy cannot express a per-branch rule, so the two variants + * disagreed on the same status across families. The bare shape wins because it + * is what {@link messageFromErrorBody} on the client reads and what the + * majority of migrated routes already emitted. + * + * `success: false` is not carried on error bodies: `requestJson` throws an + * `ApiClientError` for any non-2xx response, so no typed client ever observes + * the discriminator. `success: true` on *success* bodies is a separate + * contract and is unaffected. + */ export const internalOrchestrationErrorPolicy: InternalErrorPolicy = { - project(error) { - const classified = asOrchestrationError(error) - if (!classified) return null - return internalErrorResponse(statusForOrchestrationError(classified.code), { - success: false, - error: classified.message, - }) - }, -} - -export const internalPlainOrchestrationErrorPolicy: InternalErrorPolicy = { project(error) { const classified = asOrchestrationError(error) if (!classified) return null @@ -231,7 +238,7 @@ type InternalJsonRouteOptions< } & InternalJsonPresenter function createJsonErrorResponse(descriptor: JsonErrorResponseDescriptor): NextResponse { - return NextResponse.json(descriptor.body, { + return NextResponse.json(withRequestId(descriptor.body), { status: descriptor.status, headers: descriptor.headers, }) @@ -358,15 +365,12 @@ export function defineInternalJsonRoute< } }, { - typedErrorResponse: ({ error, status }) => - NextResponse.json({ error: error.message }, { status }), + typedErrorResponse: ({ error, status, requestId }) => + NextResponse.json({ error: error.message, requestId }, { status }), unhandledErrorResponse: () => createJsonErrorResponse( options.errorPolicy.unhandled?.() ?? - internalErrorResponse(500, { - success: false, - error: 'Internal server error', - }) + internalErrorResponse(500, { error: 'Internal server error' }) ), } ) diff --git a/apps/sim/lib/api/server/routes/request-id.test.ts b/apps/sim/lib/api/server/routes/request-id.test.ts new file mode 100644 index 00000000000..73d75cebe9a --- /dev/null +++ b/apps/sim/lib/api/server/routes/request-id.test.ts @@ -0,0 +1,45 @@ +/** + * @vitest-environment node + */ +import { getRequestContext } from '@sim/logger' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { withRequestId } from '@/lib/api/server/routes/request-id' + +const mockGetRequestContext = vi.mocked(getRequestContext) + +describe('withRequestId', () => { + beforeEach(() => { + vi.clearAllMocks() + mockGetRequestContext.mockReturnValue(undefined) + }) + + it('stamps the ambient request id onto an error body', () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-123' }) + + expect(withRequestId({ error: 'Table not found' })).toEqual({ + error: 'Table not found', + requestId: 'req-123', + }) + }) + + it('leaves the body untouched when there is no active request scope', () => { + expect(withRequestId({ error: 'Table not found' })).toEqual({ error: 'Table not found' }) + }) + + it('does not overwrite a requestId the policy already set', () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-123' }) + + expect(withRequestId({ error: 'boom', requestId: 'explicit' })).toEqual({ + error: 'boom', + requestId: 'explicit', + }) + }) + + it('passes through non-object bodies', () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-123' }) + + expect(withRequestId('plain text')).toBe('plain text') + expect(withRequestId(null)).toBeNull() + expect(withRequestId([{ error: 'a' }])).toEqual([{ error: 'a' }]) + }) +}) diff --git a/apps/sim/lib/api/server/routes/request-id.ts b/apps/sim/lib/api/server/routes/request-id.ts new file mode 100644 index 00000000000..96ca523c1af --- /dev/null +++ b/apps/sim/lib/api/server/routes/request-id.ts @@ -0,0 +1,24 @@ +import { getRequestContext } from '@sim/logger' + +/** + * Stamps the ambient request id onto an internal error body. + * + * `withRouteHandler` runs every handler inside a `runWithRequestContext` scope + * and already emits the same id as the `x-request-id` header. Carrying it in + * the body as well is what lets a user paste an error straight from the UI and + * have it correlate to a log line — a header is not visible at that point. + * + * Returns the body untouched when it is not a plain object (so array and + * scalar error bodies are preserved), when a `requestId` is already present, + * or when there is no active request scope — the last case keeps the field out + * of unit tests, where `getRequestContext` is mocked to `undefined`. + */ +export function withRequestId(body: unknown): unknown { + if (!body || typeof body !== 'object' || Array.isArray(body)) return body + if ('requestId' in body) return body + + const requestId = getRequestContext()?.requestId + if (!requestId) return body + + return { ...body, requestId } +} diff --git a/apps/sim/lib/knowledge/api/route-policies.ts b/apps/sim/lib/knowledge/api/route-policies.ts index 6b20b06a984..0d2d715fc0e 100644 --- a/apps/sim/lib/knowledge/api/route-policies.ts +++ b/apps/sim/lib/knowledge/api/route-policies.ts @@ -3,7 +3,7 @@ import { createV2ResourceConcealmentPolicy, type InternalErrorPolicy, internalErrorResponse, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, type V2ErrorPolicy, v2OrchestrationErrorPolicy, } from '@/lib/api/server/routes' @@ -16,7 +16,7 @@ import { v2Error } from '@/app/api/v2/lib/response' function internalKnowledgeErrorPolicy(unhandledMessage: string): InternalErrorPolicy { return { - project: internalPlainOrchestrationErrorPolicy.project, + project: internalOrchestrationErrorPolicy.project, unhandled: () => internalErrorResponse(500, { error: unhandledMessage }), } } @@ -29,7 +29,7 @@ const internalKnowledgeUploadErrorPolicy: InternalErrorPolicy = { if (error instanceof KnowledgeUsageLimitExceededError) { return internalErrorResponse(402, { error: error.message }) } - return internalPlainOrchestrationErrorPolicy.project(error) + return internalOrchestrationErrorPolicy.project(error) }, unhandled: () => internalErrorResponse(500, { error: 'Failed to process knowledge upload request' }), @@ -43,7 +43,7 @@ const internalKnowledgeSearchErrorPolicy: InternalErrorPolicy = { if (error instanceof KnowledgeSearchProvenanceUnavailableError) { return internalErrorResponse(422, { error: error.message }) } - return internalPlainOrchestrationErrorPolicy.project(error) + return internalOrchestrationErrorPolicy.project(error) }, unhandled: () => internalErrorResponse(500, { error: 'Failed to perform vector search' }), } diff --git a/apps/sim/lib/table/api/route-policies.test.ts b/apps/sim/lib/table/api/route-policies.test.ts index 2f5a0ba19e2..f691f55db8e 100644 --- a/apps/sim/lib/table/api/route-policies.test.ts +++ b/apps/sim/lib/table/api/route-policies.test.ts @@ -24,7 +24,7 @@ vi.unmock('@/lib/auth/internal') import { InternalUnauthenticatedError, - internalPlainOrchestrationErrorPolicy, + internalOrchestrationErrorPolicy, } from '@/lib/api/server/routes' import { generateInternalDelegationToken, generateInternalToken } from '@/lib/auth/internal' import { OrchestrationError } from '@/lib/core/orchestration/types' @@ -176,7 +176,7 @@ describe('internal Table route authentication', () => { it('renders an invalid related workflow as 400 on internal and v2 surfaces', async () => { const error = new OrchestrationError('validation', 'Invalid workflow ID') - expect(internalPlainOrchestrationErrorPolicy.project(error)).toEqual({ + expect(internalOrchestrationErrorPolicy.project(error)).toEqual({ status: 400, body: { error: 'Invalid workflow ID' }, headers: undefined, diff --git a/apps/sim/lib/workspace-files/api/internal-error-policies.test.ts b/apps/sim/lib/workspace-files/api/internal-error-policies.test.ts index ed962991516..e8794b28bb5 100644 --- a/apps/sim/lib/workspace-files/api/internal-error-policies.test.ts +++ b/apps/sim/lib/workspace-files/api/internal-error-policies.test.ts @@ -41,7 +41,7 @@ describe('internal file error policies', () => { ) ).toEqual({ status: 402, - body: { success: false, error: 'Storage limit exceeded' }, + body: { error: 'Storage limit exceeded' }, headers: undefined, }) }) diff --git a/apps/sim/lib/workspace-files/api/internal-error-policies.ts b/apps/sim/lib/workspace-files/api/internal-error-policies.ts index 2dad228d3c8..a9fd4bd52a0 100644 --- a/apps/sim/lib/workspace-files/api/internal-error-policies.ts +++ b/apps/sim/lib/workspace-files/api/internal-error-policies.ts @@ -4,7 +4,6 @@ import { type InternalErrorPolicy, internalErrorResponse, internalOrchestrationErrorPolicy, - internalPlainOrchestrationErrorPolicy, } from '@/lib/api/server/routes' import { StorageLimitExceededError } from '@/lib/billing/storage' import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' @@ -16,12 +15,12 @@ import { StyleExtractionUnsupportedError } from '@/lib/workspace-files/applicati const logger = createLogger('InternalWorkspaceFileErrors') -const style = extendInternalErrorPolicy(internalPlainOrchestrationErrorPolicy, (error) => { +const style = extendInternalErrorPolicy(internalOrchestrationErrorPolicy, (error) => { if (!(error instanceof StyleExtractionUnsupportedError)) return null return internalErrorResponse(422, { error: error.message }) }) -const compiledCheck = extendInternalErrorPolicy(internalPlainOrchestrationErrorPolicy, (error) => { +const compiledCheck = extendInternalErrorPolicy(internalOrchestrationErrorPolicy, (error) => { if (error instanceof CompiledCheckUnsupportedError) { return internalErrorResponse(422, { error: error.message }) } @@ -33,7 +32,7 @@ const compiledCheck = extendInternalErrorPolicy(internalPlainOrchestrationErrorP const content = extendInternalErrorPolicy(internalOrchestrationErrorPolicy, (error) => { if (!(error instanceof StorageLimitExceededError)) return null - return internalErrorResponse(402, { success: false, error: error.message }) + return internalErrorResponse(402, { error: error.message }) }) const downloadUrl: InternalErrorPolicy = { @@ -41,10 +40,7 @@ const downloadUrl: InternalErrorPolicy = { const typed = internalOrchestrationErrorPolicy.project(error) if (typed) return typed logger.error('Failed to generate workspace file download URL', { error }) - return internalErrorResponse(500, { - success: false, - error: 'Failed to generate download URL', - }) + return internalErrorResponse(500, { error: 'Failed to generate download URL' }) }, } @@ -84,7 +80,6 @@ const inline: InternalErrorPolicy = { export const internalFileErrorPolicies = { default: internalOrchestrationErrorPolicy, - plain: internalPlainOrchestrationErrorPolicy, content, style, compiledCheck, From 8423bdc3cf2086ac437256e87944f940fd2a2978 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 19:22:36 -0700 Subject: [PATCH 2/2] fix(api): stamp requestId on internal auth and parse failures --- .../server/routes/internal-binary-route.ts | 7 +- .../server/routes/internal-json-route.test.ts | 67 +++++++++++++++++++ .../api/server/routes/internal-json-route.ts | 6 +- .../lib/api/server/routes/request-id.test.ts | 63 ++++++++++++++++- apps/sim/lib/api/server/routes/request-id.ts | 35 ++++++++++ 5 files changed, 171 insertions(+), 7 deletions(-) diff --git a/apps/sim/lib/api/server/routes/internal-binary-route.ts b/apps/sim/lib/api/server/routes/internal-binary-route.ts index 34845b53b3b..58ec51ec4f1 100644 --- a/apps/sim/lib/api/server/routes/internal-binary-route.ts +++ b/apps/sim/lib/api/server/routes/internal-binary-route.ts @@ -5,9 +5,10 @@ import { requireBinaryRouteDefinition } from '@/lib/api/server/routes/definition import { type InternalErrorPolicy, InternalUnauthenticatedError, + internalErrorResponse, type internalSessionAuth, } from '@/lib/api/server/routes/internal-json-route' -import { withRequestId } from '@/lib/api/server/routes/request-id' +import { responseWithRequestId, withRequestId } from '@/lib/api/server/routes/request-id' import type { BinaryApiRouteContract, BinaryResponseDescriptor, @@ -82,14 +83,14 @@ export function defineInternalBinaryRoute< principal = await options.auth.authenticate() } catch (error) { if (error instanceof InternalUnauthenticatedError) { - return NextResponse.json({ error: error.message }, { status: 401 }) + return createJsonErrorResponse(internalErrorResponse(401, { error: error.message })) } throw error } await options.rateLimit.enforce(request, principal) const parsed = await parseRequest(options.contract, request, context ?? {}) - if (!parsed.success) return parsed.response + if (!parsed.success) return responseWithRequestId(parsed.response) try { const input = options.mapInput(parsed.data) diff --git a/apps/sim/lib/api/server/routes/internal-json-route.test.ts b/apps/sim/lib/api/server/routes/internal-json-route.test.ts index 40ee26c5629..53c0e054622 100644 --- a/apps/sim/lib/api/server/routes/internal-json-route.test.ts +++ b/apps/sim/lib/api/server/routes/internal-json-route.test.ts @@ -8,6 +8,7 @@ import { z } from 'zod' import { defineRouteContract } from '@/lib/api/contracts' import { defineInternalJsonRoute, + InternalUnauthenticatedError, internalErrorResponse, internalOrchestrationErrorPolicy, internalRateLimits, @@ -148,6 +149,72 @@ describe('defineInternalJsonRoute', () => { expect(body).not.toHaveProperty('success') }) + it('stamps the request id onto an authentication failure', async () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-auth' }) + + const handler = defineInternalJsonRoute({ + contract, + auth: { + authenticate: vi.fn(async () => { + throw new InternalUnauthenticatedError('Unauthorized') + }), + }, + operation, + rateLimit: internalRateLimits.none({ reason: 'Unit test' }), + errorPolicy: internalOrchestrationErrorPolicy, + mapInput: () => undefined, + useCase: { + operation, + async execute() { + return { value: 'unreachable' } + }, + }, + }) + + const response = await handler(new NextRequest('http://localhost/api/test/internal-json-route')) + + expect(response.status).toBe(401) + await expect(response.json()).resolves.toEqual({ + error: 'Unauthorized', + requestId: 'req-auth', + }) + }) + + it('stamps the request id onto a request parsing failure', async () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-parse' }) + + const queryContract = defineRouteContract({ + method: 'GET', + path: '/api/test/internal-json-route', + query: z.object({ widgetId: z.string().min(1, 'widgetId is required') }), + response: { + mode: 'json', + schema: z.object({ value: z.string() }), + }, + }) + + const handler = defineInternalJsonRoute({ + contract: queryContract, + auth, + operation, + rateLimit: internalRateLimits.none({ reason: 'Unit test' }), + errorPolicy: internalOrchestrationErrorPolicy, + mapInput: () => undefined, + useCase: { + operation, + async execute() { + return { value: 'unreachable' } + }, + }, + }) + + const response = await handler(new NextRequest('http://localhost/api/test/internal-json-route')) + const body = await response.json() + + expect(response.status).toBe(400) + expect(body.requestId).toBe('req-parse') + }) + it('orders auth, rate limiting, parsing, async mapping, and application execution', async () => { const events: string[] = [] const orderedContract = defineRouteContract({ diff --git a/apps/sim/lib/api/server/routes/internal-json-route.ts b/apps/sim/lib/api/server/routes/internal-json-route.ts index fae641945e9..4d85af2eb51 100644 --- a/apps/sim/lib/api/server/routes/internal-json-route.ts +++ b/apps/sim/lib/api/server/routes/internal-json-route.ts @@ -8,7 +8,7 @@ import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' import type { ContractJsonResponse } from '@/lib/api/contracts' import { requireJsonRouteDefinition } from '@/lib/api/server/routes/definition' -import { withRequestId } from '@/lib/api/server/routes/request-id' +import { responseWithRequestId, withRequestId } from '@/lib/api/server/routes/request-id' import type { JsonApiRouteContract, JsonErrorResponseDescriptor, @@ -304,7 +304,7 @@ export function defineInternalJsonRoute< principal = await options.auth.authenticate(request, rawParams) } catch (error) { if (error instanceof InternalUnauthenticatedError) { - return NextResponse.json({ error: error.message }, { status: 401 }) + return createJsonErrorResponse(internalErrorResponse(401, { error: error.message })) } throw error } @@ -325,7 +325,7 @@ export function defineInternalJsonRoute< context ?? {}, options.parseOptions ) - if (!parsed.success) return parsed.response + if (!parsed.success) return responseWithRequestId(parsed.response) try { const input = await options.mapInput(parsed.data, { principal, request }) diff --git a/apps/sim/lib/api/server/routes/request-id.test.ts b/apps/sim/lib/api/server/routes/request-id.test.ts index 73d75cebe9a..ca0ea1cae72 100644 --- a/apps/sim/lib/api/server/routes/request-id.test.ts +++ b/apps/sim/lib/api/server/routes/request-id.test.ts @@ -2,8 +2,9 @@ * @vitest-environment node */ import { getRequestContext } from '@sim/logger' +import { NextResponse } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import { withRequestId } from '@/lib/api/server/routes/request-id' +import { responseWithRequestId, withRequestId } from '@/lib/api/server/routes/request-id' const mockGetRequestContext = vi.mocked(getRequestContext) @@ -43,3 +44,63 @@ describe('withRequestId', () => { expect(withRequestId([{ error: 'a' }])).toEqual([{ error: 'a' }]) }) }) + +describe('responseWithRequestId', () => { + beforeEach(() => { + vi.clearAllMocks() + mockGetRequestContext.mockReturnValue(undefined) + }) + + it('stamps the request id into an already-built JSON error response', async () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-123' }) + + const stamped = await responseWithRequestId( + NextResponse.json({ error: 'Validation error', details: [] }, { status: 400 }) + ) + + expect(stamped.status).toBe(400) + await expect(stamped.json()).resolves.toEqual({ + error: 'Validation error', + details: [], + requestId: 'req-123', + }) + }) + + it('preserves headers other than a stale content-length', async () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-123' }) + + const original = NextResponse.json( + { error: 'Validation error' }, + { status: 400, headers: { 'x-custom': 'kept', 'content-length': '29' } } + ) + const stamped = await responseWithRequestId(original) + + expect(stamped.headers.get('x-custom')).toBe('kept') + expect(stamped.headers.get('content-length')).toBeNull() + }) + + it('returns the original response when there is no active request scope', async () => { + const original = NextResponse.json({ error: 'Validation error' }, { status: 400 }) + + expect(await responseWithRequestId(original)).toBe(original) + }) + + it('returns the original response when the body is not JSON', async () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-123' }) + + const original = new NextResponse('plain text', { + status: 400, + headers: { 'content-type': 'text/plain' }, + }) + + expect(await responseWithRequestId(original)).toBe(original) + }) + + it('leaves a response that already carries a requestId untouched', async () => { + mockGetRequestContext.mockReturnValue({ requestId: 'req-123' }) + + const original = NextResponse.json({ error: 'boom', requestId: 'explicit' }, { status: 400 }) + + expect(await responseWithRequestId(original)).toBe(original) + }) +}) diff --git a/apps/sim/lib/api/server/routes/request-id.ts b/apps/sim/lib/api/server/routes/request-id.ts index 96ca523c1af..91feb86ce6c 100644 --- a/apps/sim/lib/api/server/routes/request-id.ts +++ b/apps/sim/lib/api/server/routes/request-id.ts @@ -1,4 +1,5 @@ import { getRequestContext } from '@sim/logger' +import { NextResponse } from 'next/server' /** * Stamps the ambient request id onto an internal error body. @@ -22,3 +23,37 @@ export function withRequestId(body: unknown): unknown { return { ...body, requestId } } + +/** + * Rebuilds an already-constructed JSON error response with the ambient request + * id stamped into its body. + * + * Request parsing failures arrive as a finished `NextResponse` from the shared + * validation helpers, which v1 and v2 routes also use and whose envelopes must + * not change. Stamping here — at the internal builders' call site rather than + * inside those helpers — keeps the added field scoped to internal routes. + * + * Returns the original response when there is no active request scope, when the + * body is not JSON, or when it cannot be re-read. The body is read from a clone + * so the original stays usable on any of those paths. + */ +export async function responseWithRequestId( + response: NextResponse +): Promise> { + if (!getRequestContext()?.requestId) return response + if (!response.headers.get('content-type')?.includes('application/json')) return response + + let body: unknown + try { + body = await response.clone().json() + } catch { + return response + } + + const stamped = withRequestId(body) + if (stamped === body) return response + + const headers = new Headers(response.headers) + headers.delete('content-length') + return NextResponse.json(stamped, { status: response.status, headers }) +}