diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 9b598af8ae0..24157c46f4e 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -728,9 +728,9 @@ "name": "tagFilters", "in": "query", "required": false, - "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. A name that is not defined in this knowledge base is rejected, never ignored.", + "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. Every filter must hold, including two that name the same tag. A name that is not defined in this knowledge base is rejected, never ignored.", "schema": { - "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. A name that is not defined in this knowledge base is rejected, never ignored.", + "description": "A JSON-encoded array of at most 10 tag filters, using the same display-name shape as knowledge search: `[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]`. Every filter must hold, including two that name the same tag. A name that is not defined in this knowledge base is rejected, never ignored.", "examples": [ "[{\"tagName\":\"category\",\"operator\":\"eq\",\"value\":\"billing\"}]" ], @@ -2820,9 +2820,10 @@ "examples": [["7c9e6679-7425-40de-944b-e07fc1f90ae7"]] }, "query": { - "description": "Natural-language query; required when tag filters are omitted.", + "description": "Natural-language query; required when tag filters are omitted. At most 32768 characters — longer text exceeds the embedding model's per-input token ceiling and would be truncated before the billed search ran.", "examples": ["How do I reset my password?"], - "type": "string" + "type": "string", + "maxLength": 32768 }, "topK": { "default": 10, @@ -2832,7 +2833,8 @@ "maximum": 100 }, "tagFilters": { - "description": "Structured tag filters. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with `GET /api/v2/knowledge/{id}/tags`.", + "description": "Structured tag filters, at most 10 of them. Every filter must hold, including two that name the same tag: repeating one tag narrows the result rather than widening it, matching `GET /api/v2/knowledge/{id}/documents`. To match either of two values for one tag, issue a search per value. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with `GET /api/v2/knowledge/{id}/tags`.", + "maxItems": 10, "type": "array", "items": { "$ref": "#/components/schemas/V2KnowledgeSearchTagFilter" diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index 404a537a28b..0a64c9f581f 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -64,10 +64,10 @@ "name": "triggers", "in": "query", "required": false, - "description": "Comma-separated trigger types to include. An empty entry is rejected. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.", + "description": "Comma-separated trigger types to include. An empty entry is rejected. Values are matched exactly and are case-sensitive — every recorded trigger is lowercase, so `API` matches nothing while `api` matches. The vocabulary is open: it covers the core trigger types (`manual`, `api`, `schedule`, `chat`, `webhook`, `mcp`, `copilot`, `workflow`, `custom_block`) and the provider id of any webhook trigger (`slack`, `gmail`, `github`, …), so an unrecognized member is not rejected — it selects no runs. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.", "schema": { "type": "string", - "description": "Comma-separated trigger types to include. An empty entry is rejected. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`." + "description": "Comma-separated trigger types to include. An empty entry is rejected. Values are matched exactly and are case-sensitive — every recorded trigger is lowercase, so `API` matches nothing while `api` matches. The vocabulary is open: it covers the core trigger types (`manual`, `api`, `schedule`, `chat`, `webhook`, `mcp`, `copilot`, `workflow`, `custom_block`) and the provider id of any webhook trigger (`slack`, `gmail`, `github`, …), so an unrecognized member is not rejected — it selects no runs. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`." } }, { diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 28792358443..51e85c8125b 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -2492,7 +2492,7 @@ "description": "Whether the server tools are available to workflows." }, "connectionStatus": { - "description": "Result of the most recent connection attempt. Registration and re-registration store a configuration without contacting the endpoint, so a server begins — and returns to — `disconnected` until a tool discovery runs.", + "description": "Result of the most recent connection attempt. Registration and re-registration establish no connection — the auth-type probe they may send does not count as one — so a server begins, and returns to, `disconnected` until a tool discovery runs.", "type": "string", "enum": ["connected", "disconnected", "error"] }, @@ -2694,8 +2694,7 @@ "description": "Absolute HTTP or HTTPS endpoint URL without `{{ENV_VAR}}` references. It determines server identity and is immutable: delete and recreate the server to change endpoints." }, "authType": { - "description": "Authentication method. Applied server-side as `headers` when omitted; registration never contacts the server, so an omitted value is never detected from it.", - "default": "headers", + "description": "Authentication method. When omitted, and no `headers` are sent, registration probes the endpoint once to classify it, falling back to `headers` when the probe fails or the server does not advertise OAuth. A server publishing RFC 9728 metadata is therefore stored as `oauth`, and headers configured afterwards will not authenticate — send this field explicitly to pin the method.", "type": "string", "enum": ["none", "headers", "oauth"] }, @@ -2881,8 +2880,7 @@ "maxLength": 2048 }, "authType": { - "description": "Authentication method. Applied server-side as `headers` when omitted; registration never contacts the server, so an omitted value is never detected from it.", - "default": "headers", + "description": "Authentication method. When omitted, and no `headers` are sent, registration probes the endpoint once to classify it, falling back to `headers` when the probe fails or the server does not advertise OAuth. A server publishing RFC 9728 metadata is therefore stored as `oauth`, and headers configured afterwards will not authenticate — send this field explicitly to pin the method.", "type": "string", "enum": ["none", "headers", "oauth"] }, diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index 173c373bc16..a8215eec631 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -6611,7 +6611,8 @@ "type": "boolean" } }, - "required": ["name", "type"] + "required": ["name", "type"], + "additionalProperties": false }, "description": "Columns created for producer outputs." }, @@ -6750,7 +6751,8 @@ "type": "boolean" } }, - "required": ["name", "type"] + "required": ["name", "type"], + "additionalProperties": false } }, "mappingUpdates": { diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index c93a2b3352e..6d9fdd9da01 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -1423,9 +1423,9 @@ "name": "includeOutput", "in": "query", "required": false, - "description": "Include final and block outputs when true.", + "description": "Include the final workflow output when true. It does not gate `blockOutputs`, which `selectedOutputs` selects on its own.", "schema": { - "description": "Include final and block outputs when true.", + "description": "Include the final workflow output when true. It does not gate `blockOutputs`, which `selectedOutputs` selects on its own.", "type": "boolean" } }, @@ -1433,9 +1433,9 @@ "name": "selectedOutputs", "in": "query", "required": false, - "description": "Comma-separated block output references to include.", + "description": "Comma-separated block output references to include, as `blockId` or `blockId.path`. Block *names* are not resolved here — unlike the execute request, this resource reads a recorded run and matches ids only, so a name selects nothing and yields an empty `blockOutputs`.", "schema": { - "description": "Comma-separated block output references to include.", + "description": "Comma-separated block output references to include, as `blockId` or `blockId.path`. Block *names* are not resolved here — unlike the execute request, this resource reads a recorded run and matches ids only, so a name selects nothing and yields an empty `blockOutputs`.", "type": "string" } } @@ -3882,21 +3882,20 @@ "INVALID_INPUT", "BLOCK_EXECUTION_FAILED", "CHILD_WORKFLOW_FAILED", - "OUTPUT_TOO_LARGE", "EXECUTION_FAILED" ], - "description": "Stable machine-readable execution failure code." + "description": "Stable machine-readable execution failure code. `BLOCK_EXECUTION_FAILED` and `CHILD_WORKFLOW_FAILED` are reported only where block attribution is available; elsewhere a block-level failure is reported as `EXECUTION_FAILED`." }, "blockId": { - "description": "Identifier of the failing block, when attributable.", + "description": "Identifier of the failing block. Present on the synchronous execute response only; the polled run resource and the resume response cannot attribute a block.", "type": "string" }, "blockName": { - "description": "Display name of the failing block.", + "description": "Display name of the failing block. Present on the synchronous execute response only.", "type": "string" }, "blockType": { - "description": "Integration or block type that failed.", + "description": "Integration or block type that failed. Present on the synchronous execute response only.", "type": "string" } }, @@ -4292,7 +4291,7 @@ "type": "null" } ], - "description": "Trigger type, or null before the run is recorded." + "description": "Trigger type that started the run. Backfilled as `api` for a run that is still queued, so it is populated from the first poll." }, "startedAt": { "anyOf": [ @@ -4303,7 +4302,7 @@ "type": "null" } ], - "description": "ISO 8601 start timestamp, or null while queued.", + "description": "ISO 8601 start timestamp. A queued run reports the time it was enqueued, so it is populated from the first poll.", "format": "date-time" }, "endedAt": { @@ -4453,7 +4452,7 @@ "type": "null" } ], - "description": "Structured execution failure, or null when none occurred." + "description": "Structured execution failure, or null when none occurred. Reclassified from the persisted error message, so `blockId`/`blockName`/`blockType` are absent and a block-level failure reports `EXECUTION_FAILED` here even when the same run reported `BLOCK_EXECUTION_FAILED` on its synchronous execute response." }, "output": { "anyOf": [ @@ -4481,7 +4480,7 @@ "type": "null" } ], - "description": "Selected block outputs when requested, otherwise null." + "description": "Outputs of the blocks named by `selectedOutputs`, or null when none were requested. Gated by `selectedOutputs` alone — `includeOutput` governs `output` only." } }, "required": [ diff --git a/apps/sim/app/api/v2/audit-logs/route.ts b/apps/sim/app/api/v2/audit-logs/route.ts index d69532e01ae..207251b3145 100644 --- a/apps/sim/app/api/v2/audit-logs/route.ts +++ b/apps/sim/app/api/v2/audit-logs/route.ts @@ -1,5 +1,10 @@ import { v2ListAuditLogsContract } from '@/lib/api/contracts/v2/audit-logs' -import { cursorScopeKey, instantScopePart, unorderedScopePart } from '@/lib/api/cursor-binding' +import { + cursorRoute, + cursorScopeKey, + instantScopePart, + unorderedScopePart, +} from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -23,7 +28,7 @@ function auditLogCursorFilters(query: { startDate?: string endDate?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListAuditLogsContract), { organizationId: query.organizationId, includeDeparted: query.includeDeparted, action: query.action, diff --git a/apps/sim/app/api/v2/billing/logs/route.test.ts b/apps/sim/app/api/v2/billing/logs/route.test.ts index f1534458b47..c929a42215a 100644 --- a/apps/sim/app/api/v2/billing/logs/route.test.ts +++ b/apps/sim/app/api/v2/billing/logs/route.test.ts @@ -24,7 +24,8 @@ vi.mock('@/lib/billing/application/list-billing-logs', () => ({ listBillingLogs: { operation: { id: 'billing.logs.list' }, execute: mocks.execute }, })) -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { v2ListBillingLogsContract } from '@/lib/api/contracts/v2/billing' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { UNKNOWN_CURSOR_MESSAGE } from '@/lib/billing/core/usage-log' import { OrchestrationError } from '@/lib/core/orchestration/types' import { GET } from '@/app/api/v2/billing/logs/route' @@ -35,7 +36,7 @@ function ledgerCursor( inner: string, filters: { source?: string; workspaceId?: string; period?: string } ): string { - return encodeScopedCursor(cursorScopeKey(filters), inner) + return encodeScopedCursor(cursorScopeKey(cursorRoute(v2ListBillingLogsContract), filters), inner) } const auth = { diff --git a/apps/sim/app/api/v2/billing/logs/route.ts b/apps/sim/app/api/v2/billing/logs/route.ts index f306ff40682..61f1e4acd80 100644 --- a/apps/sim/app/api/v2/billing/logs/route.ts +++ b/apps/sim/app/api/v2/billing/logs/route.ts @@ -1,5 +1,5 @@ import { v2ListBillingLogsContract } from '@/lib/api/contracts/v2/billing' -import { cursorScopeKey, instantScopePart } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey, instantScopePart } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2BillingErrorPolicies } from '@/lib/billing/api/route-policies' import { listBillingLogs } from '@/lib/billing/application/list-billing-logs' @@ -32,7 +32,7 @@ function billingLogCursorFilters(query: { startDate?: string endDate?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListBillingLogsContract), { source: query.source, workspaceId: query.workspaceId, period: query.period, diff --git a/apps/sim/app/api/v2/credentials/route.ts b/apps/sim/app/api/v2/credentials/route.ts index bccfffccbe2..b0dde7a39ab 100644 --- a/apps/sim/app/api/v2/credentials/route.ts +++ b/apps/sim/app/api/v2/credentials/route.ts @@ -1,6 +1,6 @@ import type { V2Credential } from '@/lib/api/contracts/v2/credentials' import { v2ListCredentialsContract } from '@/lib/api/contracts/v2/credentials' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -42,7 +42,7 @@ function credentialCursorFilters(query: { providerId?: string search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListCredentialsContract), { workspaceId: query.workspaceId, type: query.type, providerId: query.providerId, diff --git a/apps/sim/app/api/v2/custom-tools/route.ts b/apps/sim/app/api/v2/custom-tools/route.ts index ca027ae013a..4cbfafb5492 100644 --- a/apps/sim/app/api/v2/custom-tools/route.ts +++ b/apps/sim/app/api/v2/custom-tools/route.ts @@ -2,7 +2,7 @@ import { v2CreateCustomToolContract, v2ListCustomToolsContract, } from '@/lib/api/contracts/v2/custom-tools' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -22,7 +22,7 @@ export const revalidate = 0 /** Every param that changes which custom tools, in which order, this list returns. */ function customToolCursorFilters(query: { workspaceId: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListCustomToolsContract), { workspaceId: query.workspaceId, search: query.search, }) diff --git a/apps/sim/app/api/v2/files/route.ts b/apps/sim/app/api/v2/files/route.ts index 31930cf1e60..e544f4bf231 100644 --- a/apps/sim/app/api/v2/files/route.ts +++ b/apps/sim/app/api/v2/files/route.ts @@ -3,7 +3,7 @@ import { v2CreateFileContract, v2ListFilesContract, } from '@/lib/api/contracts/v2/files' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils' import { v2FileErrorPolicies } from '@/lib/workspace-files/api' @@ -24,7 +24,7 @@ function fileCursorFilters(query: { folderPath?: string search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListFilesContract), { workspaceId: query.workspaceId, scope: query.scope, folderPath: query.folderPath, diff --git a/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts b/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts index c6440f890de..436567fe845 100644 --- a/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts +++ b/apps/sim/app/api/v2/knowledge/[id]/documents/route.ts @@ -5,7 +5,7 @@ import { v2ListKnowledgeDocumentsContract, v2UploadKnowledgeDocumentContract, } from '@/lib/api/contracts/v2/knowledge' -import { cursorScopeKey, unorderedScopeOf } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey, unorderedScopeOf } from '@/lib/api/cursor-binding' import { defineV2BodyLifecycleRoute, defineV2JsonRoute, @@ -59,8 +59,7 @@ function documentCursorFilters( query: { workspaceId: string; enabledFilter?: string; search?: string; tagFilters?: string } ) { const parsed = parseV2KnowledgeTagFiltersParam(query.tagFilters) - return cursorScopeKey({ - knowledgeBaseId, + return cursorScopeKey(cursorRoute(v2ListKnowledgeDocumentsContract, { id: knowledgeBaseId }), { workspaceId: query.workspaceId, enabledFilter: query.enabledFilter, search: query.search, diff --git a/apps/sim/app/api/v2/knowledge/route.ts b/apps/sim/app/api/v2/knowledge/route.ts index 07d9be95f16..8bfaabf2fcd 100644 --- a/apps/sim/app/api/v2/knowledge/route.ts +++ b/apps/sim/app/api/v2/knowledge/route.ts @@ -2,7 +2,7 @@ import { v2CreateKnowledgeBaseContract, v2ListKnowledgeBasesContract, } from '@/lib/api/contracts/v2/knowledge' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -28,7 +28,7 @@ function knowledgeCursorFilters(query: { folderPath?: string search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListKnowledgeBasesContract), { workspaceId: query.workspaceId, folderPath: query.folderPath, search: query.search, diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index 7ffd9fc9f88..8f5f61215b5 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -251,22 +251,14 @@ export function decodeCursor>(cursor: string): T | n interface OffsetCursorPayload { /** The ordering the offset counts positions within. */ sort: string - /** Fingerprint of the filters the offset counts positions within. */ - filter?: string + /** Fingerprint of the list and filters the offset counts positions within. */ + filter: string offset: number } -/** An offset cursor stamped with the sort and filters that produced it. */ -export function encodeOffsetCursor( - sort: string, - filter: string | undefined, - offset: number -): string { - return encodeCursor({ - sort, - ...(filter ? { filter } : {}), - offset, - } satisfies OffsetCursorPayload) +/** An offset cursor stamped with the sort and scope that produced it. */ +export function encodeOffsetCursor(sort: string, filter: string, offset: number): string { + return encodeCursor({ sort, filter, offset } satisfies OffsetCursorPayload) } /** @@ -288,14 +280,15 @@ export function encodeOffsetCursor( export function decodeOffsetCursor( cursor: string | undefined, sort: string, - filter?: string | undefined + filter: string ): number { if (!cursor) return 0 const decoded = decodeCursor>(cursor) - if (!decoded || decoded.sort !== sort) { + if (!decoded) throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) + if (decoded.sort !== sort) { throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) } - if ((decoded.filter ?? undefined) !== (filter || undefined)) { + if ((decoded.filter ?? undefined) !== filter) { throw new OrchestrationError('validation', REFILTERED_CURSOR_MESSAGE) } const { offset } = decoded @@ -317,29 +310,27 @@ export function cursorSortKey(sortBy: string, sortOrder: string): string { interface SortedCursorPayload { sort: string keys: CursorKey[] - /** Fingerprint of the filters the page was read under; absent = unfiltered. */ - filter?: string + /** Fingerprint of the list and filters the page was read under. */ + filter: string } /** - * A keyset cursor stamped with the sort AND the filters that produced it. The + * A keyset cursor stamped with the sort AND the scope that produced it. The * keys are only meaningful under that exact ordering, and only name a useful * position within that exact row set, so both stamps travel with them. */ -export function encodeSortedCursor( - sort: string, - keys: CursorKey[], - filter?: string | undefined -): string { - return encodeCursor({ sort, keys, ...(filter ? { filter } : {}) } satisfies SortedCursorPayload) +export function encodeSortedCursor(sort: string, keys: CursorKey[], filter: string): string { + return encodeCursor({ sort, keys, filter } satisfies SortedCursorPayload) } type DecodedSortedCursor = | { status: 'absent' } | { status: 'ok'; keys: CursorKey[] } - /** Malformed, or minted under a different sort — the page cannot be resumed. */ + /** Not a pagination cursor at all — it does not decode into one. */ + | { status: 'unreadable' } + /** Minted under a different sort — the keys compare the wrong column. */ | { status: 'invalid' } - /** Minted under different filters — the position names another sequence. */ + /** Minted under a different list or different filters — another sequence. */ | { status: 'refiltered' } /** @@ -365,14 +356,15 @@ type DecodedSortedCursor = export function decodeSortedCursor( cursor: string | undefined, sort: string, - filter?: string | undefined + filter: string ): DecodedSortedCursor { if (!cursor) return { status: 'absent' } const decoded = decodeCursor>(cursor) - if (!decoded || decoded.sort !== sort || !Array.isArray(decoded.keys)) { - return { status: 'invalid' } + if (!decoded || typeof decoded.sort !== 'string' || !Array.isArray(decoded.keys)) { + return { status: 'unreadable' } } - if ((decoded.filter ?? undefined) !== (filter || undefined)) return { status: 'refiltered' } + if (decoded.sort !== sort) return { status: 'invalid' } + if ((decoded.filter ?? undefined) !== filter) return { status: 'refiltered' } return { status: 'ok', keys: decoded.keys } } @@ -392,9 +384,12 @@ export function readSortedCursor( cursor: string | undefined, sortBy: string, sortOrder: string, - filter?: string | undefined + filter: string ): CursorKey[] | undefined { const decoded = decodeSortedCursor(cursor, cursorSortKey(sortBy, sortOrder), filter) + if (decoded.status === 'unreadable') { + throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) + } if (decoded.status === 'invalid') { throw new OrchestrationError('validation', INVALID_CURSOR_MESSAGE) } @@ -416,14 +411,14 @@ export function writeSortedCursor( keys: CursorKey[] | null | undefined, sortBy: string, sortOrder: string, - filter?: string | undefined + filter: string ): string | null { return keys ? encodeSortedCursor(cursorSortKey(sortBy, sortOrder), keys, filter) : null } interface ScopedCursorPayload { - /** Fingerprint of the filters and sort the inner token was minted under. */ - scope?: string + /** Fingerprint of the list, filters, and sort the inner token was minted under. */ + scope: string /** The domain codec's own opaque token, passed through untouched. */ inner: string } @@ -438,8 +433,8 @@ interface ScopedCursorPayload { * lists the same binding as the rest of the surface — one rule for v2 callers * rather than "some lists notice, some don't". */ -export function encodeScopedCursor(scope: string | undefined, inner: string): string { - return encodeCursor({ ...(scope ? { scope } : {}), inner } satisfies ScopedCursorPayload) +export function encodeScopedCursor(scope: string, inner: string): string { + return encodeCursor({ scope, inner } satisfies ScopedCursorPayload) } /** @@ -456,16 +451,13 @@ export function encodeScopedCursor(scope: string | undefined, inner: string): st * reached through the wrapper instead of through the token, and it slipped past * the unresolvable-cursor 400 that exists to stop it. */ -export function readScopedCursor( - cursor: string | undefined, - scope: string | undefined -): string | undefined { +export function readScopedCursor(cursor: string | undefined, scope: string): string | undefined { if (!cursor) return undefined const decoded = decodeCursor>(cursor) if (!decoded || typeof decoded.inner !== 'string' || decoded.inner.length === 0) { throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) } - if ((decoded.scope ?? undefined) !== (scope || undefined)) { + if ((decoded.scope ?? undefined) !== scope) { throw new OrchestrationError('validation', REFILTERED_CURSOR_MESSAGE) } return decoded.inner diff --git a/apps/sim/app/api/v2/logs/route.test.ts b/apps/sim/app/api/v2/logs/route.test.ts index 277f05cd1f1..36917bedbab 100644 --- a/apps/sim/app/api/v2/logs/route.test.ts +++ b/apps/sim/app/api/v2/logs/route.test.ts @@ -24,7 +24,8 @@ vi.mock('@/lib/logs/application/list-public-logs', () => ({ listPublicLogs: { operation: { id: 'logs.list' }, execute: mocks.execute }, })) -import { cursorScopeKey, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' +import { v2ListLogsContract } from '@/lib/api/contracts/v2/logs' +import { cursorRoute, cursorScopeKey, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' import { OrchestrationError } from '@/lib/core/orchestration/types' import { encodeScopedCursor } from '@/app/api/v2/lib/response' import { GET } from '@/app/api/v2/logs/route' @@ -202,7 +203,7 @@ describe('GET /api/v2/logs', () => { */ it('rejects a cursor whose inner token is empty instead of restarting at page one', async () => { const cursor = encodeScopedCursor( - cursorScopeKey({ workspaceId: WORKSPACE_ID, order: 'desc' }), + cursorScopeKey(cursorRoute(v2ListLogsContract), { workspaceId: WORKSPACE_ID, order: 'desc' }), '' ) diff --git a/apps/sim/app/api/v2/logs/route.ts b/apps/sim/app/api/v2/logs/route.ts index 5ecf1a474f3..74123a88266 100644 --- a/apps/sim/app/api/v2/logs/route.ts +++ b/apps/sim/app/api/v2/logs/route.ts @@ -5,6 +5,7 @@ import { v2LogStatusSchema, } from '@/lib/api/contracts/v2/logs' import { + cursorRoute, cursorScopeKey, instantScopePart, parseUnorderedList, @@ -45,7 +46,7 @@ function logCursorFilters(query: { folderPaths?: string order?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListLogsContract), { workspaceId: query.workspaceId, workflowIds: unorderedScopePart(query.workflowIds), triggers: unorderedScopePart(query.triggers), diff --git a/apps/sim/app/api/v2/mcp-servers/route.ts b/apps/sim/app/api/v2/mcp-servers/route.ts index 37757f7824b..91ea160afc2 100644 --- a/apps/sim/app/api/v2/mcp-servers/route.ts +++ b/apps/sim/app/api/v2/mcp-servers/route.ts @@ -2,7 +2,7 @@ import { v2CreateMcpServerContract, v2ListMcpServersContract, } from '@/lib/api/contracts/v2/mcp-servers' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -20,7 +20,7 @@ export const revalidate = 0 /** Every param that changes which MCP servers, in which order, this list returns. */ function mcpServerCursorFilters(query: { workspaceId: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListMcpServersContract), { workspaceId: query.workspaceId, search: query.search, }) diff --git a/apps/sim/app/api/v2/secrets/route.ts b/apps/sim/app/api/v2/secrets/route.ts index 591508f8be1..bd9a44c1d39 100644 --- a/apps/sim/app/api/v2/secrets/route.ts +++ b/apps/sim/app/api/v2/secrets/route.ts @@ -1,5 +1,5 @@ import { v2ListSecretsContract } from '@/lib/api/contracts/v2/secrets' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -16,7 +16,7 @@ export const revalidate = 0 /** Every param that changes which secrets, in which order, this list returns. */ function secretCursorFilters(query: { workspaceId: string; scope?: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListSecretsContract), { workspaceId: query.workspaceId, scope: query.scope, search: query.search, diff --git a/apps/sim/app/api/v2/skills/route.test.ts b/apps/sim/app/api/v2/skills/route.test.ts index e84de0dfcdb..40b8668b169 100644 --- a/apps/sim/app/api/v2/skills/route.test.ts +++ b/apps/sim/app/api/v2/skills/route.test.ts @@ -50,7 +50,8 @@ vi.mock('@/lib/skills/application/use-cases', () => ({ createSkillUseCase: { operation: { id: 'skills.create' }, execute: mocks.create }, })) -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { v2ListSkillsContract } from '@/lib/api/contracts/v2/skills' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { PrincipalKindAuthorizationError } from '@/lib/core/application' import { cursorSortKey, encodeOffsetCursor } from '@/app/api/v2/lib/response' import { GET, POST } from '@/app/api/v2/skills/route' @@ -75,7 +76,7 @@ function skillCursor({ }): string { return encodeOffsetCursor( cursorSortKey(sortBy, sortOrder), - cursorScopeKey({ workspaceId: WORKSPACE_ID, search }), + cursorScopeKey(cursorRoute(v2ListSkillsContract), { workspaceId: WORKSPACE_ID, search }), offset ) } diff --git a/apps/sim/app/api/v2/skills/route.ts b/apps/sim/app/api/v2/skills/route.ts index a0bf8acdeb2..42de2765968 100644 --- a/apps/sim/app/api/v2/skills/route.ts +++ b/apps/sim/app/api/v2/skills/route.ts @@ -1,5 +1,5 @@ import { v2CreateSkillContract, v2ListSkillsContract } from '@/lib/api/contracts/v2/skills' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -14,7 +14,10 @@ import { toV2Skill, toV2SkillSummary } from '@/app/api/v2/skills/utils' /** Every param that changes which skills, in which order, this list returns. */ function skillCursorFilters(query: { workspaceId: string; search?: string }) { - return cursorScopeKey({ workspaceId: query.workspaceId, search: query.search }) + return cursorScopeKey(cursorRoute(v2ListSkillsContract), { + workspaceId: query.workspaceId, + search: query.search, + }) } export const dynamic = 'force-dynamic' diff --git a/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts index d32782c9bb2..e271d8eee6c 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/query/route.test.ts @@ -39,8 +39,16 @@ vi.mock('@/lib/table/application/rows', () => ({ queryTableRows: { operation: { id: 'tables.rows.query' }, execute: mocks.queryRows }, })) +import { v2QueryRowsContract } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' +import { encodeScopedCursor } from '@/app/api/v2/lib/response' import { POST } from '@/app/api/v2/tables/[tableId]/query/route' +/** A query cursor exactly as the route mints one, for the table given. */ +function queryCursor(tableId: string, inner: string): string { + return encodeScopedCursor(cursorScopeKey(cursorRoute(v2QueryRowsContract, { tableId })), inner) +} + const WORKSPACE_ID = 'workspace-1' const PRINCIPAL = { kind: 'workspace_api_key' as const, @@ -152,12 +160,31 @@ describe('POST /api/v2/tables/[tableId]/query', () => { new MockTableRowsValidationError('Invalid cursor', { code: 'INVALID_CURSOR' }) ) - const response = await call({ workspaceId: WORKSPACE_ID, cursor: 'malformed' }).response + const response = await call({ + workspaceId: WORKSPACE_ID, + cursor: queryCursor('table-1', 'malformed'), + }).response expect(response.status).toBe(400) expect((await response.json()).error.details).toEqual({ code: 'INVALID_CURSOR' }) }) + /** + * The row codec binds the predicate and sort a page was produced under but + * carries no table identity, so an unfiltered token from one table decoded + * cleanly against another and answered 200 with that other table's rows. + */ + it('refuses a query cursor minted on a different table', async () => { + const response = await call({ + workspaceId: WORKSPACE_ID, + cursor: queryCursor('table-2', 'native-row-cursor'), + }).response + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toMatch(/requested filters/) + expect(mocks.queryRows).not.toHaveBeenCalled() + }) + it('enforces the one MiB body cap before delegation', async () => { const response = await call({ workspaceId: WORKSPACE_ID, cursor: 'x'.repeat(1024 * 1024) }) .response diff --git a/apps/sim/app/api/v2/tables/[tableId]/query/route.ts b/apps/sim/app/api/v2/tables/[tableId]/query/route.ts index 76805afa9b5..5b9d0516056 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/query/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/query/route.ts @@ -1,15 +1,29 @@ import { TABLE_QUERY_MAX_BODY_BYTES } from '@/lib/api/contracts/tables' import { V2_DEFAULT_ROW_LIMIT, v2QueryRowsContract } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2TableRowsErrorPolicy } from '@/lib/table/api/row-route-policies' import { tableOperations } from '@/lib/table/application/operations' import { queryTableRows } from '@/lib/table/application/rows' import { namedRowMapper } from '@/lib/table/cell-format' +import { encodeScopedCursor, readScopedCursor } from '@/app/api/v2/lib/response' import { toApiRow } from '@/app/api/v2/tables/utils' export const dynamic = 'force-dynamic' export const revalidate = 0 +/** + * The sequence a query cursor names a position in: this list, on THIS table. + * + * The row codec binds the predicate and sort a page was produced under, but not + * the table — so an unfiltered token from one table decoded cleanly against + * another and answered 200 with that other table's rows. The table id lives in + * the path, so the route is the only place that knows it. + */ +function queryRowCursorScope(tableId: string): string { + return cursorScopeKey(cursorRoute(v2QueryRowsContract, { tableId })) +} + export const POST = defineV2JsonRoute({ contract: v2QueryRowsContract, operation: tableOperations.queryRows, @@ -22,17 +36,19 @@ export const POST = defineV2JsonRoute({ assertedWorkspaceId: body.workspaceId, predicate: body.predicate, sort: body.sort, - cursor: body.cursor, + cursor: readScopedCursor(body.cursor, queryRowCursorScope(params.tableId)), limit: body.limit === undefined ? V2_DEFAULT_ROW_LIMIT : body.limit === 0 ? undefined : body.limit, includeTotal: false, }), useCase: queryTableRows, - present: ({ table, rows, nextCursor }) => { + present: ({ table, rows, nextCursor }, { params }) => { const toNamedRow = namedRowMapper(table.schema.columns) return { data: rows.map((row) => toApiRow(row, toNamedRow)), - nextCursor, + nextCursor: nextCursor + ? encodeScopedCursor(queryRowCursorScope(params.tableId), nextCursor) + : null, } }, }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts index d36a43f1516..1f0b8a3b5e7 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts @@ -38,8 +38,19 @@ vi.mock('@/lib/table/application/rows', () => ({ deleteTableRows: { operation: { id: 'tables.rows.delete_many' }, execute: mocks.deleteRows }, })) +import { v2ListTableRowsContract } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' +import { encodeScopedCursor } from '@/app/api/v2/lib/response' import { DELETE, GET, PATCH, POST } from '@/app/api/v2/tables/[tableId]/rows/route' +/** A row cursor exactly as the route mints one, for the table given. */ +function rowCursor(tableId: string, inner: string): string { + return encodeScopedCursor( + cursorScopeKey(cursorRoute(v2ListTableRowsContract, { tableId })), + inner + ) +} + const WORKSPACE_ID = 'workspace-1' const PRINCIPAL = { kind: 'workspace_api_key' as const, @@ -102,7 +113,7 @@ describe('/api/v2/tables/[tableId]/rows', () => { }) it('passes the opaque native row cursor through the route unchanged', async () => { - const cursor = 'native-row-cursor' + const cursor = rowCursor('table-1', 'native-row-cursor') mocks.listRows.mockResolvedValue({ table: TABLE, rows: [ROW], @@ -122,11 +133,32 @@ describe('/api/v2/tables/[tableId]/rows', () => { tableId: 'table-1', assertedWorkspaceId: WORKSPACE_ID, limit: 25, - cursor, + cursor: 'native-row-cursor', }, request: req, }) - expect((await response.json()).nextCursor).toBe('next-native-cursor') + expect((await response.json()).nextCursor).toBe(rowCursor('table-1', 'next-native-cursor')) + }) + + /** + * The row codec binds the sort and predicate a page was produced under but + * carries no table identity, so an unfiltered token from one table decoded + * cleanly against another and answered 200 with that other table's rows. + */ + it('refuses a row cursor minted on a different table', async () => { + const foreign = rowCursor('table-2', 'native-row-cursor') + const response = await GET( + request( + 'GET', + undefined, + `?workspaceId=${WORKSPACE_ID}&limit=25&cursor=${encodeURIComponent(foreign)}` + ), + CONTEXT + ) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toMatch(/requested filters/) + expect(mocks.listRows).not.toHaveBeenCalled() }) it('rejects an unauthenticated request', async () => { diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts index e309607496f..4727a7bba6a 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts @@ -4,6 +4,7 @@ import { v2ListTableRowsContract, v2UpdateRowsByFilterContract, } from '@/lib/api/contracts/v2/tables' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2TableRowsErrorPolicy } from '@/lib/table/api/row-route-policies' import { tableOperations } from '@/lib/table/application/operations' @@ -14,11 +15,24 @@ import { updateTableRows, } from '@/lib/table/application/rows' import { namedRowMapper } from '@/lib/table/cell-format' +import { encodeScopedCursor, readScopedCursor } from '@/app/api/v2/lib/response' import { toApiRow } from '@/app/api/v2/tables/utils' export const dynamic = 'force-dynamic' export const revalidate = 0 +/** + * The sequence a row cursor names a position in: this list, on THIS table. + * + * The row codec binds a token to the sort and predicate it was minted under but + * carries no table identity, so an unfiltered token from one table decoded + * cleanly against another and answered 200 with that other table's rows. The + * table id lives in the path, so the route is the only place that knows it. + */ +function tableRowCursorScope(tableId: string): string { + return cursorScopeKey(cursorRoute(v2ListTableRowsContract, { tableId })) +} + export const GET = defineV2JsonRoute({ contract: v2ListTableRowsContract, operation: tableOperations.listRows, @@ -29,14 +43,16 @@ export const GET = defineV2JsonRoute({ tableId: params.tableId, assertedWorkspaceId: query.workspaceId, limit: query.limit, - cursor: query.cursor, + cursor: readScopedCursor(query.cursor, tableRowCursorScope(params.tableId)), }), useCase: listTableRows, - present: ({ table, rows, nextCursor }) => { + present: ({ table, rows, nextCursor }, { params }) => { const toNamedRow = namedRowMapper(table.schema.columns) return { data: rows.map((row) => toApiRow(row, toNamedRow)), - nextCursor, + nextCursor: nextCursor + ? encodeScopedCursor(tableRowCursorScope(params.tableId), nextCursor) + : null, } }, }) diff --git a/apps/sim/app/api/v2/tables/route.ts b/apps/sim/app/api/v2/tables/route.ts index d4d715fd7fd..a91b9c4ef61 100644 --- a/apps/sim/app/api/v2/tables/route.ts +++ b/apps/sim/app/api/v2/tables/route.ts @@ -1,5 +1,5 @@ import { v2CreateTableContract, v2ListTablesContract } from '@/lib/api/contracts/v2/tables' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { v2TableErrorPolicies } from '@/lib/table/api' import { tableOperations } from '@/lib/table/application/operations' @@ -12,7 +12,7 @@ export const revalidate = 0 /** Every param that changes which tables, in which order, this list returns. */ function tableCursorFilters(query: { workspaceId: string; folderPath?: string; search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListTablesContract), { workspaceId: query.workspaceId, folderPath: query.folderPath, search: query.search, diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/route.ts b/apps/sim/app/api/v2/workflows/[id]/runs/route.ts index b76eb2e88f5..e504143b335 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/route.ts @@ -4,6 +4,7 @@ import { v2WorkflowRunListStatusValueSchema, } from '@/lib/api/contracts/v2/workflows' import { + cursorRoute, cursorScopeKey, instantScopePart, UNREADABLE_CURSOR_MESSAGE, @@ -23,8 +24,7 @@ function runCursorFilters( workflowId: string, query: { status?: string; trigger?: string; startDate?: string; endDate?: string } ) { - return cursorScopeKey({ - workflowId, + return cursorScopeKey(cursorRoute(v2ListWorkflowRunsContract, { id: workflowId }), { status: query.status, trigger: query.trigger, startDate: instantScopePart(query.startDate), diff --git a/apps/sim/app/api/v2/workflows/route.ts b/apps/sim/app/api/v2/workflows/route.ts index c8095d27ce2..d291c231a6c 100644 --- a/apps/sim/app/api/v2/workflows/route.ts +++ b/apps/sim/app/api/v2/workflows/route.ts @@ -1,6 +1,6 @@ import type { V2WorkflowListItem } from '@/lib/api/contracts/v2/workflows' import { v2CreateWorkflowContract, v2ListWorkflowsContract } from '@/lib/api/contracts/v2/workflows' -import { cursorScopeKey } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -22,7 +22,7 @@ function workflowCursorFilters(query: { deployedOnly: boolean search?: string }) { - return cursorScopeKey({ + return cursorScopeKey(cursorRoute(v2ListWorkflowsContract), { workspaceId: query.workspaceId, folderPath: query.folderPath, deployedOnly: query.deployedOnly, 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 698f524a4ce..5be59775eea 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 @@ -135,9 +135,9 @@ describe('PUT /api/workspaces/[id]/files/[fileId]/content', () => { }) }) - it('allows JSON bodies above the default 50 MiB cap for base64 expansion', async () => { + it('allows a base64 JSON body up to what the proxy forwards intact', async () => { const response = await PUT( - createRequest({ content: 'TQ==', encoding: 'base64' }, 60 * 1024 * 1024), + createRequest({ content: 'TQ==', encoding: 'base64' }, 10 * 1024 * 1024), routeContext ) @@ -145,8 +145,13 @@ describe('PUT /api/workspaces/[id]/files/[fileId]/content', () => { expect(mocks.updateContent).toHaveBeenCalled() }) - it('rejects a JSON body above the inline-content cap after admission', async () => { - const response = await PUT(createRequest({ content: '' }, 70 * 1024 * 1024 + 1), routeContext) + /** + * The route declares a 70 MB inline cap, but Next's proxy truncates a client + * body past 10 MiB, so the parser clamps to that ceiling and answers 413 + * rather than letting a truncated prefix surface as malformed JSON. + */ + it('rejects a JSON body above the proxy ceiling after admission', async () => { + const response = await PUT(createRequest({ content: '' }, 10 * 1024 * 1024 + 1), routeContext) expect(response.status).toBe(413) expect(mocks.admit).toHaveBeenCalled() diff --git a/apps/sim/executor/utils/errors.classify.test.ts b/apps/sim/executor/utils/errors.classify.test.ts index 3051cf6a52c..0e91f223679 100644 --- a/apps/sim/executor/utils/errors.classify.test.ts +++ b/apps/sim/executor/utils/errors.classify.test.ts @@ -2,11 +2,13 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { v2ExecutionErrorSchema } from '@/lib/api/contracts/v2/workflows' import type { ExecutionResult } from '@/executor/types' import { attachExecutionResult, buildBlockExecutionError, classifyExecutionError, + type WorkflowExecutionErrorCode, } from '@/executor/utils/errors' function failedResult(partial?: Partial): ExecutionResult { @@ -146,4 +148,37 @@ describe('classifyExecutionError', () => { blockType: undefined, }) }) + + /** + * The published enum promises callers a code to route on, so a member this + * function cannot produce is a branch no response ever takes — the state + * `OUTPUT_TOO_LARGE` was in until it was retired, since an oversize run + * response is an HTTP 413 and never reaches classification. Pinning the two + * together makes the next unreachable member fail here instead of shipping + * into SDK types. + */ + it('produces every code the public execution-error contract publishes', () => { + const producedBy: Record = { + TIMEOUT: new Error('Execution timed out after 5 minutes'), + CANCELLED: new Error('Run was cancelled'), + USAGE_LIMIT_EXCEEDED: new Error('Usage limit exceeded for this billing period'), + INVALID_INPUT: new Error('Invalid input format for the workflow'), + BLOCK_EXECUTION_FAILED: buildBlockExecutionError({ + block: { id: 'block-1', metadata: { name: 'Send Email', id: 'gmail' } } as never, + error: new Error('Invalid credentials'), + }), + CHILD_WORKFLOW_FAILED: buildBlockExecutionError({ + block: { id: 'block-2', metadata: { name: 'Child', id: 'workflow' } } as never, + error: new Error('Child run failed'), + }), + EXECUTION_FAILED: new Error('something odd'), + } + + for (const [code, error] of Object.entries(producedBy)) { + expect(classifyExecutionError(error).code).toBe(code) + } + expect(new Set(v2ExecutionErrorSchema.shape.code.options)).toEqual( + new Set(Object.keys(producedBy)) + ) + }) }) diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index 90bd6a11b1b..7bbef22da0e 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -138,6 +138,16 @@ export function normalizeError(error: unknown): string { * substring-matching messages; this module is the single place raw errors are * interpreted, so the executor can later attach codes natively at throw sites * without a wire change. + * + * Append-only binds what a caller can observe, and an oversize run response has + * never been observable here: it is not an in-band failure at all. The execution + * service short-circuits it into an HTTP 413 carrying `workflow_response_too_large` + * in the error envelope, so no run ever reaches classification with an oversize + * output. `OUTPUT_TOO_LARGE` was therefore a code no caller could ever have + * routed on — publishing it only invited a branch that never runs, and an + * exhaustive `switch` in a generated SDK to claim coverage it does not have. + * Retiring an unreachable member narrows the published type without changing a + * single response; adding a reachable member later is still the append-only path. */ export type WorkflowExecutionErrorCode = | 'TIMEOUT' @@ -146,7 +156,6 @@ export type WorkflowExecutionErrorCode = | 'INVALID_INPUT' | 'BLOCK_EXECUTION_FAILED' | 'CHILD_WORKFLOW_FAILED' - | 'OUTPUT_TOO_LARGE' | 'EXECUTION_FAILED' export interface StructuredExecutionError { diff --git a/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts index 0022839b939..481cf479b6e 100644 --- a/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts +++ b/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts @@ -11,6 +11,7 @@ import { V2_SEARCH_MAX_LENGTH } from '@/lib/api/contracts/v2/shared' import * as tableContracts from '@/lib/api/contracts/v2/tables' import { V2_TABLE_IMPORT_OPTIONS_MAX_BYTES, + v2AddWorkflowGroupBodySchema, v2ApiTableSchema, v2CreateTableBodySchema, v2CreateTableColumnBodySchema, @@ -25,6 +26,7 @@ import { v2TableImportStatusSchema, v2TableUploadImportSourceSchema, v2UpdateTableColumnBodySchema, + v2UpdateWorkflowGroupBodySchema, } from '@/lib/api/contracts/v2/tables' import { getValidationErrorMessage } from '@/lib/api/server/validation' import { MAX_RUN_TARGET_ROW_IDS, TABLE_LIMITS } from '@/lib/table/constants' @@ -80,6 +82,51 @@ describe('v2 table column contracts', () => { expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys') }) + /** + * Same field, same reason, one level down: the group body's `.strict()` binds + * its own level, so an `outputColumns` entry that kept `workflowGroupId` was + * stripped, overwritten with the server-minted id, and answered 201. + */ + it('refuses a workflow group id on a group output column', () => { + const result = v2AddWorkflowGroupBodySchema.safeParse({ + workspaceId: WORKSPACE_ID, + group: { + type: 'enrichment', + enrichmentId: 'company-domain', + outputs: [{ blockId: '', path: 'domain', columnName: 'zz_y' }], + }, + outputColumns: [{ name: 'zz_y', type: 'string', workflowGroupId: 'wfg_does_not_exist' }], + }) + + expect(result.success).toBe(false) + expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys') + }) + + it('refuses a workflow group id on an added output column', () => { + const result = v2UpdateWorkflowGroupBodySchema.safeParse({ + workspaceId: WORKSPACE_ID, + groupId: 'group-1', + newOutputColumns: [{ name: 'zz_z', type: 'string', workflowGroupId: 'wfg_does_not_exist' }], + }) + + expect(result.success).toBe(false) + expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys') + }) + + it('accepts a group output column that leaves the group id to the server', () => { + expect( + v2AddWorkflowGroupBodySchema.safeParse({ + workspaceId: WORKSPACE_ID, + group: { + type: 'enrichment', + enrichmentId: 'company-domain', + outputs: [{ blockId: '', path: 'domain', columnName: 'zz_y' }], + }, + outputColumns: [{ name: 'zz_y', type: 'string' }], + }).success + ).toBe(true) + }) + it('keeps required in table responses for existing stored schemas', () => { expect( v2ApiTableSchema.safeParse({ diff --git a/apps/sim/lib/api/contracts/v2/knowledge-search-bounds.test.ts b/apps/sim/lib/api/contracts/v2/knowledge-search-bounds.test.ts new file mode 100644 index 00000000000..ea79ec8c3d3 --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/knowledge-search-bounds.test.ts @@ -0,0 +1,106 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS, + MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH, + parseV2KnowledgeTagFiltersParam, + v2KnowledgeSearchBodySchema, +} from '@/lib/api/contracts/v2/knowledge' + +const workspaceId = '7a6cce2b-78b8-40bc-b8d3-0a2a6dfd9023' +const knowledgeBaseIds = ['ae814592-a730-4ad6-8741-b51e48843300'] + +function tagFilters(count: number) { + return Array.from({ length: count }, (_, index) => ({ + tagName: 'Messages in Thread', + operator: 'gte' as const, + value: String(index), + })) +} + +function issueMessages(result: ReturnType) { + return result.success ? [] : result.error.issues.map((issue) => issue.message) +} + +/** + * Search is billed per call, so an input it cannot honour must be rejected at the + * boundary rather than accepted and quietly reshaped. + */ +describe('v2 knowledge search body bounds', () => { + it('accepts a query at the maximum length', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'a'.repeat(MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH), + }) + expect(result.success).toBe(true) + }) + + it('rejects a query one character past the maximum, naming the limit', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'a'.repeat(MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH + 1), + }) + expect(result.success).toBe(false) + expect(issueMessages(result)).toContain( + `query cannot exceed ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters` + ) + }) + + it('bounds the query by the embedding model per-input token ceiling', () => { + expect(MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH).toBe(8192 * 4) + }) + + it('accepts tag filters at the cap', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + tagFilters: tagFilters(MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS), + }) + expect(result.success).toBe(true) + }) + + /** + * The document list has always rejected an eleventh filter. Search shares the + * tag vocabulary, so it must share the policy — the same request cannot be a + * 400 on one surface and an applied 200 on the other. + */ + it('rejects one tag filter past the cap with the same message the document list uses', () => { + const overLimit = tagFilters(MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS + 1) + const expected = `tagFilters cannot contain more than ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} filters` + + const searchResult = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + tagFilters: overLimit, + }) + expect(searchResult.success).toBe(false) + expect(issueMessages(searchResult)).toContain(expected) + + const listResult = parseV2KnowledgeTagFiltersParam(JSON.stringify(overLimit)) + expect(listResult.success).toBe(false) + expect(listResult.success === false && listResult.message).toContain(expected) + }) + + it('still accepts an ordinary single filter of each field type', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'How do I reset my password?', + tagFilters: [ + { tagName: 'From', operator: 'contains', value: 'brex' }, + { tagName: 'Messages in Thread', operator: 'gte', value: 9 }, + { + tagName: 'Last Message', + operator: 'between', + value: '2026-01-01', + valueTo: '2026-12-31', + }, + ], + }) + expect(result.success).toBe(true) + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index 44a7c723f1a..52cdf284756 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -821,6 +821,21 @@ export const v2KnowledgeSearchTagFilterSchema = v1SearchTagFilterSchema description: 'A structured tag filter applied to knowledge search.', }) +/** Maximum tag filters accepted on one document-list or search request. */ +export const MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS = 10 + +/** + * Maximum `query` length accepted by knowledge search. + * + * Every knowledge-base-eligible embedding model caps a single input at 8192 + * tokens, and the embedding client silently truncates anything longer, so a + * caller paid for a billed search whose query was mostly discarded. The bound is + * that ceiling expressed in characters using the four-characters-per-token + * conversion the tokenizer's own fallback uses, which is generous enough that + * nothing that could have been embedded whole is rejected. + */ +export const MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH = 8192 * 4 + export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema .safeExtend({ workspaceId: v1KnowledgeSearchBodySchema.shape.workspaceId.describe( @@ -829,17 +844,29 @@ export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema knowledgeBaseIds: v1KnowledgeSearchBodySchema.shape.knowledgeBaseIds .describe('One knowledge base identifier or an array of up to 20 identifiers.') .meta({ examples: [['7c9e6679-7425-40de-944b-e07fc1f90ae7']] }), - query: v1KnowledgeSearchBodySchema.shape.query - .describe('Natural-language query; required when tag filters are omitted.') + query: z + .string() + .max( + MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH, + `query cannot exceed ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters` + ) + .optional() + .describe( + `Natural-language query; required when tag filters are omitted. At most ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters — longer text exceeds the embedding model's per-input token ceiling and would be truncated before the billed search ran.` + ) .meta({ examples: ['How do I reset my password?'] }), topK: v1KnowledgeSearchBodySchema.shape.topK.describe( 'Maximum number of search results to return. Must be a whole number between 1 and 100; the boundary schema only bounds the range, so a fractional value is admitted here and then rejected with 400 during search.' ), tagFilters: z .array(v2KnowledgeSearchTagFilterSchema) + .max( + MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS, + `tagFilters cannot contain more than ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} filters` + ) .optional() .describe( - 'Structured tag filters. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with `GET /api/v2/knowledge/{id}/tags`.' + `Structured tag filters, at most ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} of them. Every filter must hold, including two that name the same tag: repeating one tag narrows the result rather than widening it, matching \`GET /api/v2/knowledge/{id}/documents\`. To match either of two values for one tag, issue a search per value. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with \`GET /api/v2/knowledge/{id}/tags\`.` ), searchMode: v1KnowledgeSearchBodySchema.shape.searchMode.describe( 'Retrieval strategy: vector is semantic-only, while hybrid also runs full-text search.' @@ -895,9 +922,6 @@ export const v2SearchKnowledgeContract = defineRouteContract({ }, }) -/** Maximum tag filters accepted on one document-list request. */ -export const MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS = 10 - const v2KnowledgeDocumentTagFiltersSchema = z .array(v2KnowledgeSearchTagFilterSchema) .max( @@ -974,7 +998,7 @@ export const v2ListKnowledgeDocumentsQuerySchema = v1ListKnowledgeDocumentsQuery .string() .optional() .describe( - `A JSON-encoded array of at most ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} tag filters, using the same display-name shape as knowledge search: \`[{"tagName":"category","operator":"eq","value":"billing"}]\`. A name that is not defined in this knowledge base is rejected, never ignored.` + `A JSON-encoded array of at most ${MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS} tag filters, using the same display-name shape as knowledge search: \`[{"tagName":"category","operator":"eq","value":"billing"}]\`. Every filter must hold, including two that name the same tag. A name that is not defined in this knowledge base is rejected, never ignored.` ) .meta({ examples: ['[{"tagName":"category","operator":"eq","value":"billing"}]'] }), }) diff --git a/apps/sim/lib/api/contracts/v2/logs.ts b/apps/sim/lib/api/contracts/v2/logs.ts index 19ca8d358ea..669969b01d4 100644 --- a/apps/sim/lib/api/contracts/v2/logs.ts +++ b/apps/sim/lib/api/contracts/v2/logs.ts @@ -294,9 +294,27 @@ export const v2ListLogsQuerySchema = v1ListLogsQuerySchema 'workflowIds', 'Comma-separated workflow identifiers to include. An empty entry is rejected.' ).optional(), + /** + * Not a closed enum, which is why an unrecognized member is not a 400. + * `workflow_execution_logs.trigger` holds the core trigger types *and* the + * webhook provider id a run arrived on — `executeWebhookJobInternal` passes + * `payload.provider` straight through as the trigger — so the live + * vocabulary is the union of the core set and every webhook provider that + * has ever fired, including spellings retired since (`microsoft-teams` + * alongside `microsoftteams`). Pinning an enum here would reject the + * historical values a diagnostic search exists to find, so the filter states + * that an unmatched member simply selects nothing rather than pretending to + * validate one. + * + * Matching is exact and case-sensitive because the column is: every value + * ever written is lowercase, so `API` and `ALL` name nothing. They are + * caller mistakes, but the boundary cannot tell them apart from an unknown + * provider id, and normalizing case here would silently repair one class of + * typo while leaving the rest — so the case rule is documented instead. + */ triggers: v2CommaListSchema( 'triggers', - 'Comma-separated trigger types to include. An empty entry is rejected. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.' + 'Comma-separated trigger types to include. An empty entry is rejected. Values are matched exactly and are case-sensitive — every recorded trigger is lowercase, so `API` matches nothing while `api` matches. The vocabulary is open: it covers the core trigger types (`manual`, `api`, `schedule`, `chat`, `webhook`, `mcp`, `copilot`, `workflow`, `custom_block`) and the provider id of any webhook trigger (`slack`, `gmail`, `github`, …), so an unrecognized member is not rejected — it selects no runs. The literal value `all` is a sentinel that disables this filter entirely, so a list containing it returns runs of every trigger type; no real trigger type is named `all`.' ).optional(), level: z.enum(['info', 'error']).describe('Severity level to include.').optional(), startDate: v2RunWindowBoundSchema('startDate').optional(), diff --git a/apps/sim/lib/api/contracts/v2/mcp-servers.ts b/apps/sim/lib/api/contracts/v2/mcp-servers.ts index 2ce5c3369d0..ce89b5e1bf1 100644 --- a/apps/sim/lib/api/contracts/v2/mcp-servers.ts +++ b/apps/sim/lib/api/contracts/v2/mcp-servers.ts @@ -117,12 +117,13 @@ export const v2McpServerSchema = z 'Whether the server tools are available to workflows.' ), /** - * These three are written only by a real discovery. Registration stores a - * configuration without contacting the endpoint, so it leaves all three at - * their defaults rather than asserting a connection nothing has verified. + * These three are written only by a real discovery. Registration's one + * outbound touch is the auth-type probe, which classifies the endpoint and + * never records a connection, so registration leaves all three at their + * defaults rather than asserting a connection nothing has verified. */ connectionStatus: mcpServerSchema.shape.connectionStatus.describe( - 'Result of the most recent connection attempt. Registration and re-registration store a configuration without contacting the endpoint, so a server begins — and returns to — `disconnected` until a tool discovery runs.' + 'Result of the most recent connection attempt. Registration and re-registration establish no connection — the auth-type probe they may send does not count as one — so a server begins, and returns to, `disconnected` until a tool discovery runs.' ), lastError: mcpServerSchema.shape.lastError.describe( 'Message from the most recent failed connection, or null when absent. A re-registration clears it, since the configuration it described no longer applies.' @@ -221,12 +222,18 @@ export const v2CreateMcpServerBodySchema = z ) .meta({ default: 'streamable-http' }), url: v2McpServerUrlSchema, + /** + * No published `default`: when the field is omitted the stored value is + * detected, not defaulted, so a `default` here would be wrong on create and + * — inherited by the `.partial()` update body — would make an SDK that + * materializes JSON-Schema defaults revoke a stored OAuth grant on every + * unrelated PATCH. + */ authType: mcpAuthTypeSchema .optional() .describe( - 'Authentication method. Applied server-side as `headers` when omitted; registration never contacts the server, so an omitted value is never detected from it.' - ) - .meta({ default: 'headers' }), + 'Authentication method. When omitted, and no `headers` are sent, registration probes the endpoint once to classify it, falling back to `headers` when the probe fails or the server does not advertise OAuth. A server publishing RFC 9728 metadata is therefore stored as `oauth`, and headers configured afterwards will not authenticate — send this field explicitly to pin the method.' + ), /** Write-only. Reads expose `hasHeaders` and `headerNames` instead. */ headers: v2McpServerHeadersSchema .optional() diff --git a/apps/sim/lib/api/contracts/v2/tables.ts b/apps/sim/lib/api/contracts/v2/tables.ts index 38ff366b4b4..3708be6662f 100644 --- a/apps/sim/lib/api/contracts/v2/tables.ts +++ b/apps/sim/lib/api/contracts/v2/tables.ts @@ -1274,10 +1274,19 @@ export const v2ListWorkflowGroupsContract = defineRouteContract({ * shape carries `workflowGroupId` because the client mints the group id before * posting; v2 server-generates it, so the field is stamped from the group being * written rather than being a caller's to supply (and get wrong). + * + * `.strict()` is what makes that omission observable. `omit()` yields an + * ordinary object schema, and the enclosing body's `.strict()` binds its own + * level only — so a caller who kept the first-party `workflowGroupId` had it + * stripped, silently overwritten with the server-minted id, and answered 201. + * The same key is now a 400 naming the field, matching how `POST /v2/tables` + * refuses it on initial columns. */ -const v2WorkflowGroupOutputColumnSchema = workflowGroupOutputColumnSchema.omit({ - workflowGroupId: true, -}) +const v2WorkflowGroupOutputColumnSchema = workflowGroupOutputColumnSchema + .omit({ + workflowGroupId: true, + }) + .strict() /** * A group names its producer two mutually exclusive ways, and the underlying diff --git a/apps/sim/lib/api/contracts/v2/workflows.ts b/apps/sim/lib/api/contracts/v2/workflows.ts index eb075e9ddc7..8d0aa30a245 100644 --- a/apps/sim/lib/api/contracts/v2/workflows.ts +++ b/apps/sim/lib/api/contracts/v2/workflows.ts @@ -787,6 +787,15 @@ export const v2RollbackWorkflowContract = defineRouteContract({ * `@/executor/utils/errors` (duplicated literally: contracts are * client-importable and must not pull executor modules). APPEND-ONLY: callers * route on these instead of substring-matching messages. + * + * `OUTPUT_TOO_LARGE` was retired rather than kept for the append-only promise: + * an oversize run response is not an in-band failure and never reached + * classification, so the code was never emitted on any response. See + * `WorkflowExecutionErrorCode` for the full reasoning; the oversize case is an + * HTTP 413 carrying `workflow_response_too_large` in the error envelope. + * + * Block attribution is NOT uniform across the operations that carry this + * object — see `blockId` below. */ export const v2ExecutionErrorSchema = z .object({ @@ -799,14 +808,41 @@ export const v2ExecutionErrorSchema = z 'INVALID_INPUT', 'BLOCK_EXECUTION_FAILED', 'CHILD_WORKFLOW_FAILED', - 'OUTPUT_TOO_LARGE', 'EXECUTION_FAILED', ]) - .describe('Stable machine-readable execution failure code.'), - /** Failing block, when attributable. Deliberately crosses the workspace boundary for shared/child workflows — the runId + block context is the reproducible handle a caller hands the workflow provider. */ - blockId: z.string().optional().describe('Identifier of the failing block, when attributable.'), - blockName: z.string().optional().describe('Display name of the failing block.'), - blockType: z.string().optional().describe('Integration or block type that failed.'), + .describe( + 'Stable machine-readable execution failure code. `BLOCK_EXECUTION_FAILED` and `CHILD_WORKFLOW_FAILED` are reported only where block attribution is available; elsewhere a block-level failure is reported as `EXECUTION_FAILED`.' + ), + /** + * Failing block, when attributable. Deliberately crosses the workspace boundary for shared/child workflows — the runId + block context is the reproducible handle a caller hands the workflow provider. + * + * Attribution is produced at execution time from the failing block's throw + * site, so it reaches the caller only on the synchronous execute response. + * The polled run resource and the resume response reclassify a persisted + * error *string* — the log row keeps no structured error — so these three + * fields are absent there and the code collapses to `EXECUTION_FAILED`. + * That is a known asymmetry, documented rather than silently promised: + * a caller that needs the failing block reads the run's trace spans, which + * do carry `blockId`, `name`, and `type`. + */ + blockId: z + .string() + .optional() + .describe( + 'Identifier of the failing block. Present on the synchronous execute response only; the polled run resource and the resume response cannot attribute a block.' + ), + blockName: z + .string() + .optional() + .describe( + 'Display name of the failing block. Present on the synchronous execute response only.' + ), + blockType: z + .string() + .optional() + .describe( + 'Integration or block type that failed. Present on the synchronous execute response only.' + ), }) .meta({ id: 'ExecutionError', @@ -1191,19 +1227,36 @@ export const v2ListWorkflowRunsContract = defineRouteContract({ /** * The polled run resource. `queued` is backfilled from the async job * queue before the worker writes the durable log row — v1's jobs endpoint 404 - * window doesn't exist here. `error` is the same structured object the execute - * response carries. + * window doesn't exist here. `error` is the same *shape* the execute response + * carries, but not the same content: this resource reclassifies the persisted + * error string, so it never attributes a block. See `v2ExecutionErrorSchema`. */ export const v2WorkflowRunStatusSchema = z .object({ runId: v2WorkflowRunIdSchema, workflowId: z.string().describe('Workflow that produced the run.'), status: v2WorkflowRunStatusValueSchema, - trigger: z.string().nullable().describe('Trigger type, or null before the run is recorded.'), + /** + * Kept nullable on the wire while never being null in practice: every + * projection this resource has — the queued job, the queued resume, and the + * durable log row — backfills both fields (`api` and the job's creation time + * when the run is not yet recorded), so no caller has observed a null here. + * The nullability is the schema's tolerance for a future projection, not a + * state a caller needs to branch on, which is why neither description + * promises a null that never arrives. + */ + trigger: z + .string() + .nullable() + .describe( + 'Trigger type that started the run. Backfilled as `api` for a run that is still queued, so it is populated from the first poll.' + ), startedAt: z .string() .nullable() - .describe('ISO 8601 start timestamp, or null while queued.') + .describe( + 'ISO 8601 start timestamp. A queued run reports the time it was enqueued, so it is populated from the first poll.' + ) .meta({ format: 'date-time' }), endedAt: z .string() @@ -1224,7 +1277,9 @@ export const v2WorkflowRunStatusSchema = z .describe('Credit cost, or null when unavailable.'), error: v2ExecutionErrorSchema .nullable() - .describe('Structured execution failure, or null when none occurred.'), + .describe( + 'Structured execution failure, or null when none occurred. Reclassified from the persisted error message, so `blockId`/`blockName`/`blockType` are absent and a block-level failure reports `EXECUTION_FAILED` here even when the same run reported `BLOCK_EXECUTION_FAILED` on its synchronous execute response.' + ), /** Populated only with `includeOutput=true` on completed runs. */ output: z .unknown() @@ -1234,7 +1289,9 @@ export const v2WorkflowRunStatusSchema = z blockOutputs: z .record(z.string(), z.unknown().describe('Output value produced by one workflow block.')) .nullable() - .describe('Selected block outputs when requested, otherwise null.'), + .describe( + 'Outputs of the blocks named by `selectedOutputs`, or null when none were requested. Gated by `selectedOutputs` alone — `includeOutput` governs `output` only.' + ), }) .meta({ id: 'WorkflowRunStatus', @@ -1257,11 +1314,19 @@ export const v2GetWorkflowRunContract = defineRouteContract({ * working identically; it only widens what parses. */ includeOutput: booleanQueryFlagSchema - .describe('Include final and block outputs when true.') + .describe( + 'Include the final workflow output when true. It does not gate `blockOutputs`, which `selectedOutputs` selects on its own.' + ) .optional() .default(false), + /** + * Block *ids*, unlike the execute request's `selectedOutputs`, which also + * accepts `BlockName.path` and resolves it against the live workflow. This + * resource reads a recorded run and never loads the workflow's blocks, so + * a name has no id to resolve to and selects nothing. + */ selectedOutputs: workflowExecutionStatusQuerySchema.shape.selectedOutputs.describe( - 'Comma-separated block output references to include.' + 'Comma-separated block output references to include, as `blockId` or `blockId.path`. Block *names* are not resolved here — unlike the execute request, this resource reads a recorded run and matches ids only, so a name selects nothing and yields an empty `blockOutputs`.' ), }) .strict() diff --git a/apps/sim/lib/api/cursor-binding.test.ts b/apps/sim/lib/api/cursor-binding.test.ts index 84caf533a45..2afdb65ccf7 100644 --- a/apps/sim/lib/api/cursor-binding.test.ts +++ b/apps/sim/lib/api/cursor-binding.test.ts @@ -1,9 +1,12 @@ /** * @vitest-environment node */ +import { globSync, readFileSync } from 'node:fs' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' import { parseV2KnowledgeTagFiltersParam } from '@/lib/api/contracts/v2/knowledge' import { + cursorRoute, cursorScopeKey, instantScopePart, parseUnorderedList, @@ -14,6 +17,7 @@ import { cursorSortKey, decodeOffsetCursor, decodeSortedCursor, + encodeCursor, encodeOffsetCursor, encodeScopedCursor, encodeSortedCursor, @@ -33,11 +37,18 @@ import { * `GET /skills` and `GET /knowledge/{id}/documents`, the keyset used by the nine * SQL-ordered lists, and the wrapper that binds the domain-minted tokens on * `GET /logs`, `GET /audit-logs`, and `GET /billing/logs` — to that one rule. + * + * The scope also carries the list's own identity, because two lists that filter + * identically are still two sequences — see the `list identity` block. */ + +/** Stand-in for one v2 list route, as its contract declares it. */ +const LIST = cursorRoute({ method: 'GET', path: '/api/v2/files' }) + describe('v2 cursor binding', () => { const sort = cursorSortKey('name', 'asc') const filters = { workspaceId: 'ws-1', search: undefined as string | undefined } - const scope = cursorScopeKey(filters) + const scope = cursorScopeKey(LIST, filters) describe('offset cursor', () => { it('resumes a cursor replayed under the same query state', () => { @@ -59,10 +70,10 @@ describe('v2 cursor binding', () => { const cursor = encodeOffsetCursor(sort, scope, 40) expect(() => - decodeOffsetCursor(cursor, sort, cursorScopeKey({ ...filters, search: 'deploy' })) + decodeOffsetCursor(cursor, sort, cursorScopeKey(LIST, { ...filters, search: 'deploy' })) ).toThrow(/requested filters/) expect(() => - decodeOffsetCursor(cursor, sort, cursorScopeKey({ ...filters, workspaceId: 'ws-2' })) + decodeOffsetCursor(cursor, sort, cursorScopeKey(LIST, { ...filters, workspaceId: 'ws-2' })) ).toThrow(/requested filters/) }) @@ -100,7 +111,7 @@ describe('v2 cursor binding', () => { */ it('rejects a cursor replayed under a different filter', () => { const cursor = encodeSortedCursor(sort, keys, scope) - const narrowed = cursorScopeKey({ ...filters, search: 'deploy' }) + const narrowed = cursorScopeKey(LIST, { ...filters, search: 'deploy' }) expect(decodeSortedCursor(cursor, sort, narrowed)).toEqual({ status: 'refiltered' }) expect(() => readSortedCursor(cursor, 'name', 'asc', narrowed)).toThrow(/requested filters/) @@ -117,13 +128,10 @@ describe('v2 cursor binding', () => { ).toThrow(/sortBy\/sortOrder/) }) - it('refuses an unfiltered cursor replayed under a filter, and the reverse', () => { - const unfiltered = encodeSortedCursor(sort, keys, undefined) + it('refuses a cursor minted with no scope stamp at all', () => { + const unstamped = encodeCursor({ sort, keys }) - expect(() => readSortedCursor(unfiltered, 'name', 'asc', scope)).toThrow(/requested filters/) - expect(() => - readSortedCursor(encodeSortedCursor(sort, keys, scope), 'name', 'asc', undefined) - ).toThrow(/requested filters/) + expect(() => readSortedCursor(unstamped, 'name', 'asc', scope)).toThrow(/requested filters/) }) it('treats an absent cursor as page one', () => { @@ -142,7 +150,7 @@ describe('v2 cursor binding', () => { const cursor = encodeScopedCursor(scope, 'domain-token') expect(() => - readScopedCursor(cursor, cursorScopeKey({ ...filters, search: 'deploy' })) + readScopedCursor(cursor, cursorScopeKey(LIST, { ...filters, search: 'deploy' })) ).toThrow(/requested filters/) }) @@ -173,15 +181,20 @@ describe('v2 cursor binding', () => { }) it('does not depend on the order the parts are written', () => { - expect(cursorScopeKey({ b: '2', a: '1' })).toBe(cursorScopeKey({ a: '1', b: '2' })) + expect(cursorScopeKey(LIST, { b: '2', a: '1' })).toBe( + cursorScopeKey(LIST, { a: '1', b: '2' }) + ) }) it('treats an omitted part and an undefined part as the same scope', () => { - expect(cursorScopeKey({ a: '1', b: undefined })).toBe(cursorScopeKey({ a: '1' })) + expect(cursorScopeKey(LIST, { a: '1', b: undefined })).toBe(cursorScopeKey(LIST, { a: '1' })) }) - it('has no fingerprint at all when nothing is filtered', () => { - expect(cursorScopeKey({ a: undefined })).toBeUndefined() + it('still fingerprints the list when nothing is filtered', () => { + const bare = cursorScopeKey(LIST, { a: undefined }) + + expect(bare).toHaveLength(22) + expect(bare).toBe(cursorScopeKey(LIST)) }) /** @@ -189,27 +202,119 @@ describe('v2 cursor binding', () => { * and `{a:'1|2'}` are different reads and must fingerprint differently. */ it('separates parts rather than concatenating their values', () => { - expect(cursorScopeKey({ a: '1', b: '2' })).not.toBe(cursorScopeKey({ a: '1|2' })) - expect(cursorScopeKey({ a: '1' })).not.toBe(cursorScopeKey({ b: '1' })) + expect(cursorScopeKey(LIST, { a: '1', b: '2' })).not.toBe(cursorScopeKey(LIST, { a: '1|2' })) + expect(cursorScopeKey(LIST, { a: '1' })).not.toBe(cursorScopeKey(LIST, { b: '1' })) }) it('stays short enough to sit inside an opaque token', () => { - expect(cursorScopeKey({ search: 'x'.repeat(200) })).toHaveLength(22) + expect(cursorScopeKey(LIST, { search: 'x'.repeat(200) })).toHaveLength(22) + }) + }) + + /** + * The whole point of the scope: two lists that filter identically are still + * two sequences. Every v2 workspace list shares a `workspaceId`-only filter + * set and the same `{sort,keys,filter}` payload shape, so a fingerprint over + * filters alone made each of them accept the others' tokens and answer 200 + * with a page that silently skipped rows. + */ + describe('list identity', () => { + const workspace = { workspaceId: 'ws-1' } + + it('separates two lists that filter identically', () => { + const tables = cursorRoute({ method: 'GET', path: '/api/v2/tables' }) + const knowledge = cursorRoute({ method: 'GET', path: '/api/v2/knowledge' }) + const credentials = cursorRoute({ method: 'GET', path: '/api/v2/credentials' }) + + const keys = new Set( + [tables, knowledge, credentials].map((route) => cursorScopeKey(route, workspace)) + ) + + expect(keys.size).toBe(3) + }) + + it('refuses a cursor minted by another list', () => { + const tablesScope = cursorScopeKey(cursorRoute({ method: 'GET', path: '/api/v2/tables' }), { + workspaceId: 'ws-1', + }) + const knowledgeScope = cursorScopeKey( + cursorRoute({ method: 'GET', path: '/api/v2/knowledge' }), + { workspaceId: 'ws-1' } + ) + const fromTables = encodeSortedCursor( + sort, + ['2026-03-17T10:09:31.755Z', 'tbl_1'], + tablesScope + ) + + expect(() => readSortedCursor(fromTables, 'name', 'asc', knowledgeScope)).toThrow( + /requested filters/ + ) + expect(readSortedCursor(fromTables, 'name', 'asc', tablesScope)).toEqual([ + '2026-03-17T10:09:31.755Z', + 'tbl_1', + ]) + }) + + it('separates two parents of the same nested list', () => { + const rowsOf = (tableId: string) => + cursorScopeKey( + cursorRoute({ method: 'GET', path: '/api/v2/tables/[tableId]/rows' }, { tableId }) + ) + + const fromX = encodeScopedCursor(rowsOf('tbl_x'), 'domain-token') + + expect(() => readScopedCursor(fromX, rowsOf('tbl_y'))).toThrow(/requested filters/) + expect(readScopedCursor(fromX, rowsOf('tbl_x'))).toBe('domain-token') + }) + + /** + * A template binds every parent to one scope, which is the defect this + * exists to prevent — so an unresolved placeholder is a hard failure rather + * than a fingerprint of the literal `[tableId]`. + */ + it('refuses to fingerprint an unresolved path param', () => { + expect(() => + cursorScopeKey(cursorRoute({ method: 'GET', path: '/api/v2/tables/[tableId]/rows' })) + ).toThrow(/tableId/) + }) + }) + + /** + * A token that does not decode says nothing about the sort, and the sort + * message tells a caller who changed nothing to go re-read the sort docs. + */ + describe('undecodable cursor', () => { + it('reports that the token is unreadable, not that the sort changed', () => { + expect(() => readSortedCursor('GARBAGE', 'name', 'asc', scope)).toThrow( + /not a valid pagination cursor/ + ) + expect(() => readSortedCursor('GARBAGE', 'name', 'asc', scope)).not.toThrow(/sortBy/) + expect(() => decodeOffsetCursor('GARBAGE', sort, scope)).toThrow( + /not a valid pagination cursor/ + ) + expect(() => decodeOffsetCursor('GARBAGE', sort, scope)).not.toThrow(/sortBy/) + }) + + it('still names the sort when the token decodes and the sort is what changed', () => { + expect(() => + readSortedCursor(encodeSortedCursor(sort, ['a', 'b'], scope), 'createdAt', 'asc', scope) + ).toThrow(/sortBy\/sortOrder/) }) }) }) describe('unordered filter scope parts', () => { it('fingerprints a reordered set identically', () => { - const a = cursorScopeKey({ workflowIds: unorderedScopePart('A,B') }) - const b = cursorScopeKey({ workflowIds: unorderedScopePart('B,A') }) + const a = cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,B') }) + const b = cursorScopeKey(LIST, { workflowIds: unorderedScopePart('B,A') }) expect(a).toBe(b) }) it('fingerprints a duplicate-bearing set identically', () => { // The filters compile to `inArray`, which is set membership, so A,A,B // selects exactly what A,B does and must resume the same page. - expect(cursorScopeKey({ workflowIds: unorderedScopePart('A,A,B') })).toBe( - cursorScopeKey({ workflowIds: unorderedScopePart('A,B') }) + expect(cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,A,B') })).toBe( + cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,B') }) ) expect(unorderedScopePart('B,A,B')).toBe('A,B') }) @@ -292,8 +397,8 @@ describe('unordered filter scope parts', () => { }) it('still separates genuinely different sets', () => { - expect(cursorScopeKey({ workflowIds: unorderedScopePart('A,B') })).not.toBe( - cursorScopeKey({ workflowIds: unorderedScopePart('A,C') }) + expect(cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,B') })).not.toBe( + cursorScopeKey(LIST, { workflowIds: unorderedScopePart('A,C') }) ) }) it('treats an all-empty list as absent, matching the parsers', () => { @@ -301,3 +406,43 @@ describe('unordered filter scope parts', () => { expect(unorderedScopePart('A,,B')).toBe('A,B') }) }) + +/** + * The declaration sweep in `contracts/v2/__tests__/list-pagination.test.ts` + * reconciles each list's cursor binding against its CONTRACT, and never looks at + * the `cursorScopeKey` call the route actually makes — which is precisely where + * the missing list and parent identity sat. This closes that half: the scope a + * route builds must start from its own route identity, and a nested list must + * resolve the path params that name its parent. + */ +describe('every v2 route binds its cursor to its route identity', () => { + const routeFiles = globSync('app/api/v2/**/route.ts', { + cwd: join(import.meta.dirname, '..', '..'), + absolute: true, + }) + + it('finds the v2 route tree', () => { + expect(routeFiles.length).toBeGreaterThan(50) + }) + + it.each(routeFiles.filter((file) => readFileSync(file, 'utf8').includes('cursorScopeKey(')))( + '%s', + (file) => { + const source = readFileSync(file, 'utf8') + const calls = [...source.matchAll(/cursorScopeKey\(\s*([A-Za-z_][\w.]*)/g)].map( + (match) => match[1] + ) + + expect(calls.length).toBeGreaterThan(0) + for (const first of calls) { + expect(first).toBe('cursorRoute') + } + + if (file.includes('[')) { + for (const call of source.matchAll(/cursorRoute\(([^)]*)\)/g)) { + expect(call[1]).toContain(',') + } + } + } + ) +}) diff --git a/apps/sim/lib/api/cursor-binding.ts b/apps/sim/lib/api/cursor-binding.ts index 95fc95b98eb..920cc72886e 100644 --- a/apps/sim/lib/api/cursor-binding.ts +++ b/apps/sim/lib/api/cursor-binding.ts @@ -158,24 +158,87 @@ export function fingerprint(canonical: string): string { } /** - * The fingerprint of a list's sequence-affecting params, or `undefined` when - * the caller supplied none of them. - * - * A cursor names a position in *one* sequence, so everything that reorders or - * re-filters that sequence has to travel with it — otherwise replaying the token - * against a re-filtered read silently answers from a sequence the caller never - * asked for. Pass every param that changes *which rows, in which order*. Keep - * `limit` out: it selects how much of the sequence to return, not what the - * sequence is, so a caller may change page size mid-walk. Response-shaping - * params (whether to inline trace spans, say) stay out for the same reason. - * - * `undefined` is a real state rather than an empty hash: an unstamped cursor is - * an unfiltered one, so it stays short, and replaying it under a filter still - * mismatches (`undefined !== `). Params whose value is `undefined` are - * dropped, so omitting a filter and never having sent it are the same scope. + * The list a cursor names a position in: the route that mints it, with the path + * params that pick out *which* parent resource resolved into the path. + * + * Identity is taken from the contract rather than a per-route literal on + * purpose. A hand-written name is a step an author can forget, and forgetting it + * is invisible: two lists whose only filter is `workspaceId` fingerprint + * identically and silently accept each other's tokens. The contract already + * carries the one string that is unique per list and impossible to omit. + */ +export interface CursorScopeRoute { + method: string + path: string + /** Resolved values for the path's `[placeholders]`. */ + params?: Record +} + +/** + * The route identity half of a cursor scope, built from the route's own + * contract so it cannot drift from the endpoint it binds. + * + * Pass `params` for every `[placeholder]` in the contract path — those name the + * parent resource, and a nested list bound only to its filters accepts a sibling + * parent's cursor and answers with the wrong rows. + */ +export function cursorRoute( + contract: { method: string; path: string }, + params?: Record +): CursorScopeRoute { + return { method: contract.method, path: contract.path, params } +} + +const PATH_PARAM_PATTERN = /\[([^\]]+)\]/g + +/** + * `METHOD /concrete/path`, with every `[placeholder]` replaced by its value. + * + * An unresolved placeholder throws rather than fingerprinting the template: a + * template binds every parent resource to one scope, which is the defect this + * exists to prevent, and a route whose params never reach here is misconfigured + * for every request rather than for an unlucky one. + */ +function resolveRouteIdentity(route: CursorScopeRoute): string { + const path = route.path.replace(PATH_PARAM_PATTERN, (_match, name: string) => { + const value = route.params?.[name] + if (value === undefined || value === '') { + throw new Error( + `cursorScopeKey: ${route.method} ${route.path} has no value for path param "${name}"` + ) + } + return encodeURIComponent(value) + }) + return `${route.method} ${path}` +} + +/** + * The fingerprint of the exact sequence a cursor names a position in: the list + * itself, plus the params that reorder or re-filter it. + * + * A cursor names a position in *one* sequence, so everything that decides which + * sequence that is has to travel with it — otherwise replaying the token + * silently answers from a sequence the caller never asked for. That includes the + * list's own identity, not only its filters: `GET /v2/tables` and + * `GET /v2/knowledge` share both a `{sort,keys,filter}` payload shape and a + * `workspaceId`-only filter set, so a fingerprint over filters alone made every + * such pair accept each other's tokens and answer 200 with a page that silently + * skipped rows. + * + * Pass every param that changes *which rows, in which order*. Keep `limit` out: + * it selects how much of the sequence to return, not what the sequence is, so a + * caller may change page size mid-walk. Response-shaping params (whether to + * inline trace spans, say) stay out for the same reason. + * + * Always a string, never `undefined`: the route identity is always present, so + * there is no "unstamped" cursor to represent. Params whose value is `undefined` + * are dropped, so omitting a filter and never having sent it are the same scope. */ -export function cursorScopeKey(parts: Record): string | undefined { - const present = filterUndefined(parts) - if (Object.keys(present).length === 0) return undefined - return fingerprint(canonicalJson(present)) +export function cursorScopeKey( + route: CursorScopeRoute, + parts: Record = {} +): string { + return fingerprint( + canonicalJson({ route: resolveRouteIdentity(route), filters: filterUndefined(parts) }) + ) } diff --git a/apps/sim/lib/api/server/validation.test.ts b/apps/sim/lib/api/server/validation.test.ts new file mode 100644 index 00000000000..68b050e9e8e --- /dev/null +++ b/apps/sim/lib/api/server/validation.test.ts @@ -0,0 +1,143 @@ +/** + * @vitest-environment node + */ +import { NextRequest } from 'next/server' +import { describe, expect, it } from 'vitest' +import { + DEFAULT_MAX_JSON_BODY_BYTES, + parseJsonBody, + parseOptionalJsonBody, +} from '@/lib/api/server/validation' + +/** + * Next.js truncates a proxied client body past `experimental.proxyClientMaxBodySize` + * without signalling it, so this is the largest body a handler can actually receive. + */ +const PROXY_CLIENT_MAX_BODY_BYTES = 10 * 1024 * 1024 + +/** Mirrors `MAX_WORKSPACE_FILE_INLINE_BODY_BYTES` — an explicit override above the ceiling. */ +const INLINE_FILE_BODY_BYTES = 70 * 1024 * 1024 + +/** Mirrors the knowledge-search override — below the ceiling, so the clamp must not touch it. */ +const BELOW_CEILING_BODY_BYTES = 2 * 1024 * 1024 + +/** + * Declares `content-length` independently of the bytes actually attached, which is + * how the guard sees an oversized request without buffering one in the test. + */ +function requestDeclaring(contentLength: number, body: string): NextRequest { + return new NextRequest('http://localhost/api/v2/widgets', { + method: 'POST', + headers: { + 'content-type': 'application/json', + 'content-length': String(contentLength), + }, + body, + }) +} + +describe('DEFAULT_MAX_JSON_BODY_BYTES', () => { + it('never exceeds the body size the proxy will forward intact', () => { + expect(DEFAULT_MAX_JSON_BODY_BYTES).toBeLessThanOrEqual(PROXY_CLIENT_MAX_BODY_BYTES) + }) +}) + +describe('parseJsonBody default size boundary', () => { + it('accepts a body at the proxy cap', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES, JSON.stringify({ value: 'ok' })) + ) + + expect(result.success).toBe(true) + }) + + it('accepts a body just under the proxy cap', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES - 1, JSON.stringify({ value: 'ok' })) + ) + + expect(result.success).toBe(true) + }) + + it('rejects a body just over the proxy cap as too large, not as malformed', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })) + ) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('too_large') + expect(result.response.status).toBe(413) + }) + + it('rejects an explicit over-ceiling override the same way, quoting the enforced limit', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })), + 'response', + INLINE_FILE_BODY_BYTES + ) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('too_large') + expect(result.response.status).toBe(413) + await expect(result.response.json()).resolves.toEqual({ + error: `Request body exceeds the maximum allowed size of ${PROXY_CLIENT_MAX_BODY_BYTES} bytes`, + }) + }) + + it('still accepts a body at the ceiling under an over-ceiling override', async () => { + const result = await parseJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES, JSON.stringify({ value: 'ok' })), + 'response', + INLINE_FILE_BODY_BYTES + ) + + expect(result.success).toBe(true) + }) + + it('leaves an override below the ceiling exactly as declared', async () => { + const atLimit = await parseJsonBody( + requestDeclaring(BELOW_CEILING_BODY_BYTES, JSON.stringify({ value: 'ok' })), + 'response', + BELOW_CEILING_BODY_BYTES + ) + expect(atLimit.success).toBe(true) + + const overLimit = await parseJsonBody( + requestDeclaring(BELOW_CEILING_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })), + 'response', + BELOW_CEILING_BODY_BYTES + ) + expect(overLimit.success).toBe(false) + if (overLimit.success) return + expect(overLimit.reason).toBe('too_large') + await expect(overLimit.response.json()).resolves.toEqual({ + error: `Request body exceeds the maximum allowed size of ${BELOW_CEILING_BODY_BYTES} bytes`, + }) + }) + + it('applies the same clamp to an optional body', async () => { + const result = await parseOptionalJsonBody( + requestDeclaring(PROXY_CLIENT_MAX_BODY_BYTES + 1, JSON.stringify({ value: 'ok' })), + INLINE_FILE_BODY_BYTES + ) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('too_large') + expect(result.response.status).toBe(413) + }) + + it('still reports a genuinely malformed body as malformed', async () => { + const result = await parseJsonBody(requestDeclaring(7, '{"a": ')) + + expect(result.success).toBe(false) + if (result.success) return + expect(result.reason).toBe('invalid_json') + expect(result.response.status).toBe(400) + await expect(result.response.json()).resolves.toEqual({ + error: 'Request body must be valid JSON', + }) + }) +}) diff --git a/apps/sim/lib/api/server/validation.ts b/apps/sim/lib/api/server/validation.ts index 9e9fdb50286..096b1a63656 100644 --- a/apps/sim/lib/api/server/validation.ts +++ b/apps/sim/lib/api/server/validation.ts @@ -20,16 +20,52 @@ import { readStreamToBufferWithLimit, } from '@/lib/core/utils/stream-limits' +/** + * Next.js buffers the client body for the proxy and *silently truncates* anything + * past `experimental.proxyClientMaxBodySize` (default 10 MB), and `apps/sim/proxy.ts` + * matches `/api/:path*`. A larger body therefore reaches the handler as a truncated + * prefix, which fails JSON parsing — so an oversized request has to be rejected on + * its declared size before it is read, or the truncation gets misreported as a + * malformed body. + */ +const PROXY_CLIENT_MAX_BODY_BYTES = 10 * 1024 * 1024 + /** * Default upper bound on the JSON request body that contract routes will read - * and parse into memory. Next.js App Router imposes no body cap, so without - * this an unauthenticated caller could buffer an arbitrarily large body before - * schema validation runs. Override per-route via `ParseRequestOptions.maxBodyBytes`. + * and parse into memory. Without a cap an unauthenticated caller could buffer a + * large body before schema validation runs. Override per-route via + * `ParseRequestOptions.maxBodyBytes`. + * * Falls back to 50 MB if the env value is missing or non-numeric so a misconfig - * can never silently disable the cap (a NaN limit would never reject). + * can never silently disable the cap (a NaN limit would never reject), then + * clamps to {@link PROXY_CLIENT_MAX_BODY_BYTES} because the app can never + * actually receive more than the proxy forwards. */ -export const DEFAULT_MAX_JSON_BODY_BYTES = - Number.parseInt(env.API_MAX_JSON_BODY_BYTES, 10) || 50 * 1024 * 1024 +export const DEFAULT_MAX_JSON_BODY_BYTES = Math.min( + Number.parseInt(env.API_MAX_JSON_BODY_BYTES, 10) || 50 * 1024 * 1024, + PROXY_CLIENT_MAX_BODY_BYTES +) + +/** + * Clamps a per-route body cap to {@link PROXY_CLIENT_MAX_BODY_BYTES}. + * + * A route that raises `maxBodyBytes` above the proxy ceiling cannot actually + * receive a body that large: the proxy truncates the stream, the handler parses + * a prefix, and the caller gets `400 "Request body must be valid JSON"` for a + * request whose only fault was its size. Clamping at the point of use turns that + * into an accurate `413`; nothing that succeeds today changes, because a body + * over the ceiling already fails — just less honestly. + * + * Consequence worth keeping in view: `MAX_WORKSPACE_FILE_INLINE_BODY_BYTES` + * (70 MB) exists so a 50 MiB file can be sent inline as base64, and that ceiling + * stays unreachable until `experimental.proxyClientMaxBodySize` is raised in + * `apps/sim/next.config.ts`. Raising it changes the memory profile of every + * `/api` route, so it is a separate decision — this clamp only makes the limit + * that is actually in force report itself correctly. + */ +function clampToProxyLimit(maxBytes: number): number { + return Math.min(maxBytes, PROXY_CLIENT_MAX_BODY_BYTES) +} export interface ValidationErrorBody { error: string @@ -61,7 +97,8 @@ export interface ParseRequestOptions { /** * Maximum number of bytes to read for the JSON body before rejecting with a * 413. Defaults to {@link DEFAULT_MAX_JSON_BODY_BYTES}. Raise this only for - * routes that legitimately accept large JSON payloads (e.g. inline file uploads). + * routes that legitimately accept large JSON payloads (e.g. inline file uploads); + * a value above what the proxy forwards is clamped — see {@link clampToProxyLimit}. */ maxBodyBytes?: number /** Treat an absent or whitespace-only body as `undefined` before contract validation. */ @@ -148,8 +185,9 @@ export async function parseJsonBody( response: NextResponse<{ error: string }> } > { + const limit = clampToProxyLimit(maxBytes) try { - return { success: true, data: await readJsonBodyWithLimit(request, maxBytes) } + return { success: true, data: await readJsonBodyWithLimit(request, limit) } } catch (error) { if (invalidJson === 'throw') throw error if (isPayloadSizeLimitError(error)) { @@ -157,7 +195,7 @@ export async function parseJsonBody( success: false, reason: 'too_large', response: NextResponse.json( - { error: `Request body exceeds the maximum allowed size of ${maxBytes} bytes` }, + { error: `Request body exceeds the maximum allowed size of ${limit} bytes` }, { status: 413 } ), } @@ -188,13 +226,14 @@ export async function parseOptionalJsonBody( response: NextResponse<{ error: string }> } > { + const limit = clampToProxyLimit(maxBytes) try { - assertContentLengthWithinLimit(request.headers, maxBytes, REQUEST_BODY_LABEL) + assertContentLengthWithinLimit(request.headers, limit, REQUEST_BODY_LABEL) const stream = request.body const text = stream ? new TextDecoder().decode( - await readStreamToBufferWithLimit(stream, { maxBytes, label: REQUEST_BODY_LABEL }) + await readStreamToBufferWithLimit(stream, { maxBytes: limit, label: REQUEST_BODY_LABEL }) ) : await request.text() @@ -208,7 +247,7 @@ export async function parseOptionalJsonBody( success: false, reason: 'too_large', response: NextResponse.json( - { error: `Request body exceeds the maximum allowed size of ${maxBytes} bytes` }, + { error: `Request body exceeds the maximum allowed size of ${limit} bytes` }, { status: 413 } ), } diff --git a/apps/sim/lib/knowledge/search/queries.test.ts b/apps/sim/lib/knowledge/search/queries.test.ts index 6186d4f5a0c..dda82f335c0 100644 --- a/apps/sim/lib/knowledge/search/queries.test.ts +++ b/apps/sim/lib/knowledge/search/queries.test.ts @@ -2,6 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { buildTagFilterCondition } from '@/lib/knowledge/documents/tag-filter' import { getStructuredTagFilters } from '@/lib/knowledge/search/queries' import type { StructuredFilter } from '@/lib/knowledge/types' @@ -20,10 +21,14 @@ const embeddingTable = { * The global `drizzle-orm` mock renders `sql` fragments to a `?`-placeholder * string via `toSQL()`, so we can assert the exact predicate each filter builds. */ +function render(condition: unknown) { + return (condition as { toSQL: () => { sql: string; params: unknown[] } }).toSQL() +} + function renderOne(filters: StructuredFilter[]) { const conditions = getStructuredTagFilters(filters, embeddingTable) expect(conditions).toHaveLength(1) - return (conditions[0] as unknown as { toSQL: () => { sql: string; params: unknown[] } }).toSQL() + return render(conditions[0]) } describe('getStructuredTagFilters', () => { @@ -152,8 +157,17 @@ describe('getStructuredTagFilters', () => { expect(sql).toBe('? != ?') expect(params).toEqual(['boolean1', false]) }) + }) - it('ORs two filters on the same slot and keeps them one condition', () => { + /** + * The callers spread the returned conditions into `and(...)`, so one condition + * per filter is what makes the whole array conjunctive. Grouping same-slot + * filters into a single OR'd condition made search answer an impossible + * predicate with a full page while the document list, which ANDs the same + * filters, answered with nothing. + */ + describe('every filter is a conjunct, including two naming the same tag', () => { + it('emits one condition per filter for two filters on the same slot', () => { const conditions = getStructuredTagFilters( [ { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'a' }, @@ -161,11 +175,62 @@ describe('getStructuredTagFilters', () => { ], embeddingTable ) - expect(conditions).toHaveLength(1) - const joined = (conditions[0] as unknown as { values: unknown[] }).values[0] as { - toSQL: () => { sql: string; params: unknown[] } - } - expect(joined.toSQL().params).toEqual(['tag1', 'a', 'tag1', 'b']) + expect(conditions).toHaveLength(2) + expect(render(conditions[0]).params).toEqual(['tag1', 'a']) + expect(render(conditions[1]).params).toEqual(['tag1', 'b']) + }) + + it('keeps an impossible same-tag range as two conditions rather than a union', () => { + const conditions = getStructuredTagFilters( + [ + { tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '9' }, + { tagSlot: 'number1', fieldType: 'number', operator: 'lte', value: '2' }, + ], + embeddingTable + ) + expect(conditions).toHaveLength(2) + expect(render(conditions[0]).sql).toBe('? >= ?') + expect(render(conditions[1]).sql).toBe('? <= ?') + expect(conditions.every((condition) => !render(condition).sql.includes('OR'))).toBe(true) + }) + + it('still emits one condition per filter across different slots', () => { + const conditions = getStructuredTagFilters( + [ + { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'a' }, + { tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '9' }, + { tagSlot: 'boolean1', fieldType: 'boolean', operator: 'eq', value: 'true' }, + ], + embeddingTable + ) + expect(conditions).toHaveLength(3) + }) + }) + + /** + * The document list builds one predicate per filter and pushes each into a + * single `and(...)`. Search must yield the same number of conjuncts for the + * same filters, or the two surfaces answer different questions over one tag + * vocabulary. + */ + describe('agreement with the document-list surface', () => { + it('produces the same number of conjuncts as the document-list builder', () => { + const filters: StructuredFilter[] = [ + { tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '9' }, + { tagSlot: 'number1', fieldType: 'number', operator: 'lte', value: '2' }, + ] + + const listConditions = filters.map((filter) => + buildTagFilterCondition({ + tagSlot: filter.tagSlot, + fieldType: 'number', + operator: filter.operator, + value: filter.value, + }) + ) + + expect(listConditions.every((condition) => condition !== undefined)).toBe(true) + expect(getStructuredTagFilters(filters, embeddingTable)).toHaveLength(listConditions.length) }) }) }) diff --git a/apps/sim/lib/knowledge/search/queries.ts b/apps/sim/lib/knowledge/search/queries.ts index 1b380f1ac4e..a28a2e78536 100644 --- a/apps/sim/lib/knowledge/search/queries.ts +++ b/apps/sim/lib/knowledge/search/queries.ts @@ -282,9 +282,18 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { } /** - * Build SQL conditions from structured filters with operator support - * - Same tag multiple times: OR logic - * - Different tags: AND logic + * Build SQL conditions from structured filters with operator support. Every + * filter is a conjunct, including two that name the same tag. + * + * Search used to group filters by slot and OR same-slot conditions together, + * which made the two surfaces over the same tag vocabulary answer different + * questions: the document list ANDs every filter, so `gte 9` plus `lte 2` on one + * number tag returned nothing there and a full page of results from search — + * a widening on the billed endpoint, the same failure mode as dropping a filter. + * OR also made a range on a single text tag (`contains A` and `contains B`) + * inexpressible, while the union it produced stays reachable as separate + * searches. Neither contract ever documented the OR, so no caller could have + * been relying on it deliberately. * * Every filter reaching here has already been validated, so one that fails to * compile is a defect rather than a predicate to skip. Skipping it dropped the @@ -293,36 +302,11 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { * paid for the widened scan. It is reported as a validation failure instead. */ export function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: any) { - // Group filters by tagSlot - const filtersBySlot = new Map() - for (const filter of filters) { - const slot = filter.tagSlot - if (!filtersBySlot.has(slot)) { - filtersBySlot.set(slot, []) - } - filtersBySlot.get(slot)!.push(filter) - } - - // Build conditions: OR within same slot, AND across different slots - const conditions: ReturnType[] = [] - - for (const [, slotFilters] of filtersBySlot) { - const slotConditions = slotFilters.map((f) => { - const condition = buildFilterCondition(f, embeddingTable) - if (condition === null) throw uncompilableTagFilterError(f) - return condition - }) - - if (slotConditions.length === 1) { - // Single condition for this slot - conditions.push(slotConditions[0]) - } else { - // Multiple conditions for same slot - OR them together - conditions.push(sql`(${sql.join(slotConditions, sql` OR `)})`) - } - } - - return conditions + return filters.map((filter) => { + const condition = buildFilterCondition(filter, embeddingTable) + if (condition === null) throw uncompilableTagFilterError(filter) + return condition + }) } /** diff --git a/apps/sim/lib/logs/application/get-public-log.ts b/apps/sim/lib/logs/application/get-public-log.ts index c16fa10dc3b..cf40838c5c2 100644 --- a/apps/sim/lib/logs/application/get-public-log.ts +++ b/apps/sim/lib/logs/application/get-public-log.ts @@ -48,11 +48,20 @@ export interface GetPublicLogResult { * folder means the caller's own tree is inconsistent; it is wrong for a * diagnostic log read, where the run may long outlive the folder it ran in and a * 500 would withhold the whole run over one unresolvable field. + * + * `workflowExists` is the join, not the folder: the log's `workflow_id` is set + * null when the workflow is deleted, so the left join yields a null `folderId` + * that is indistinguishable from a workflow sitting at the workspace root. Left + * unseparated, a run whose workflow is gone reports the root path next to + * `deleted: true` — a path the caller can hand back to `folderPaths` as a filter + * for a workflow that is no longer in any folder at all. */ function publicLogFolderPath( pathById: ReadonlyMap, - folderId: string | null + folderId: string | null, + workflowExists: boolean ): string | null { + if (!workflowExists) return null if (!folderId) return ROOT_FOLDER_PATH return pathById.get(folderId) ?? null } @@ -90,7 +99,11 @@ export const getPublicLog = defineAuthorizedWorkspaceUseCase({ } return { log: { ...log, workflowState: sanitizeExecutionSnapshotState(log.workflowState) }, - workflowFolderPath: publicLogFolderPath(folderIndex.pathById, log.workflowFolderId), + workflowFolderPath: publicLogFolderPath( + folderIndex.pathById, + log.workflowFolderId, + log.workflowName !== null + ), executionData, } }, diff --git a/apps/sim/lib/logs/application/public-log-use-cases.test.ts b/apps/sim/lib/logs/application/public-log-use-cases.test.ts index f0584ff6169..de575537efc 100644 --- a/apps/sim/lib/logs/application/public-log-use-cases.test.ts +++ b/apps/sim/lib/logs/application/public-log-use-cases.test.ts @@ -101,6 +101,8 @@ const log = { executionId: 'run-1', workspaceId: 'workspace-1', workflowId: 'workflow-1', + /** Null exactly when the left join found no workflow row — the delete signal. */ + workflowName: 'Support triage', workflowFolderId: 'folder-1', workflowUserId: 'owner-1', workflowOwnerEmail: 'owner@example.com', @@ -196,6 +198,35 @@ describe('public log application use cases', () => { expect(result.workflowFolderPath).toBeNull() }) + /** + * A deleted workflow nulls the log's `workflow_id`, so the left join returns a + * null folder that is shape-identical to a workflow sitting at the root. Read + * as the root, the run reports `/` beside `deleted: true` and hands the caller + * a `folderPaths` value for a workflow that is in no folder at all. + */ + it('reports no folder path for a run whose workflow has been deleted', async () => { + mocks.getLogScope.mockResolvedValueOnce({ + executionId: 'run-1', + workspaceId: 'workspace-1', + workflowId: null, + }) + mocks.getLog.mockResolvedValueOnce({ + ...log, + workflowId: null, + workflowName: null, + workflowFolderId: null, + workflowUserId: null, + workflowOwnerEmail: null, + }) + + const result = await getPublicLog.execute({ + principal: workspacePrincipal, + input: { runId: 'run-1' }, + }) + + expect(result.workflowFolderPath).toBeNull() + }) + it('redacts credential values from the run snapshot', async () => { mocks.getLog.mockResolvedValueOnce({ ...log, diff --git a/apps/sim/lib/mcp/application/use-cases.test.ts b/apps/sim/lib/mcp/application/use-cases.test.ts index 73eb5404a19..9f59727b7a2 100644 --- a/apps/sim/lib/mcp/application/use-cases.test.ts +++ b/apps/sim/lib/mcp/application/use-cases.test.ts @@ -257,6 +257,24 @@ describe('MCP server application use cases', () => { expect(mocks.discoverServerTools).not.toHaveBeenCalled() }) + it('answers a disabled server with a conflict instead of an untyped fault', async () => { + mocks.getServer.mockResolvedValueOnce({ ...server, enabled: false }) + + await expect( + discoverMcpServerToolsUseCase.execute({ + principal: { kind: 'personal_api_key', userId: 'user-1', keyId: 'key-1' }, + input: { workspaceId: workspace.workspaceId, serverId: server.id }, + }) + ).rejects.toMatchObject({ code: 'conflict' }) + + /** + * Discovery loads its configuration through a query that filters on + * `enabled`, so reaching it raised a plain `Error` — rendered as a 500 for + * a documented registration value — and stamped a bogus failure on the row. + */ + expect(mocks.discoverServerTools).not.toHaveBeenCalled() + }) + it('discovers one server tools for the acting subject, honouring refresh', async () => { const tools = [ { diff --git a/apps/sim/lib/mcp/application/use-cases.ts b/apps/sim/lib/mcp/application/use-cases.ts index 3189a7ef481..dc7d3433516 100644 --- a/apps/sim/lib/mcp/application/use-cases.ts +++ b/apps/sim/lib/mcp/application/use-cases.ts @@ -159,6 +159,20 @@ export const discoverMcpServerToolsUseCase = defineAuthorizedWorkspaceUseCase({ resolveServerContext(input.workspaceId, input.serverId), authorizationOptions, async execute({ principal, input, context }) { + /** + * `enabled: false` is a documented registration value, but discovery loads + * its configuration through a query that filters on `enabled`, so a + * disabled server surfaced as an untyped "not accessible" fault — rendered + * as a Sim-side 500 — and stamped a bogus failure on the row on the way + * out. It is a state conflict the caller resolves by enabling the server. + */ + if (!context.server.enabled) { + throw new OrchestrationError( + 'conflict', + 'The MCP server is disabled; enable it before listing its tools' + ) + } + const tools = await mcpService.discoverServerTools( requirePrincipalSubjectUserId(principal), context.server.id, diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts index f89751d23db..b6df53b6b12 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts @@ -167,6 +167,81 @@ describe('MCP server lifecycle orchestration', () => { ) }) + it('resets the connection when a headers server rotates the headers that authenticate it', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { + url: 'https://example.com/mcp', + authType: 'headers', + headers: { authorization: 'Bearer original' }, + oauthClientId: null, + oauthClientSecret: null, + }, + ]) + dbChainMockFns.returning.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/mcp', + authType: 'headers', + }, + ]) + + const result = await performUpdateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + serverId: 'server-1', + headers: { authorization: 'Bearer rotated' }, + }) + + expect(result.success).toBe(true) + expect(dbChainMockFns.set).toHaveBeenCalledWith( + expect.objectContaining({ + headers: { authorization: 'Bearer rotated' }, + connectionStatus: 'disconnected', + lastConnected: null, + lastError: null, + }) + ) + // Rotating headers invalidates nothing OAuth holds, so the grant survives. + expect(mockRevokeOauthTokens).not.toHaveBeenCalled() + }) + + it('leaves the connection alone when a headers rewrite changes nothing', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { + url: 'https://example.com/mcp', + authType: 'headers', + headers: { authorization: 'Bearer original' }, + oauthClientId: null, + oauthClientSecret: null, + }, + ]) + dbChainMockFns.returning.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/mcp', + authType: 'headers', + }, + ]) + + const result = await performUpdateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + serverId: 'server-1', + headers: { authorization: 'Bearer original' }, + }) + + expect(result.success).toBe(true) + expect(dbChainMockFns.set).not.toHaveBeenCalledWith( + expect.objectContaining({ connectionStatus: 'disconnected' }) + ) + }) + it('audits only the columns an edit wrote, not the params it was handed', async () => { dbChainMockFns.limit.mockResolvedValueOnce([ { @@ -278,6 +353,64 @@ describe('MCP server lifecycle orchestration', () => { ) }) + it('stores an explicit retries of 0 rather than folding it into the default', async () => { + mockGenerateMcpServerId.mockReturnValue('server-1') + dbChainMockFns.limit.mockResolvedValueOnce([]) + dbChainMockFns.limit.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/anything', + authType: 'headers', + }, + ]) + + const result = await performCreateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + name: 'Example', + url: 'https://example.com/anything', + authType: 'headers', + retries: 0, + }) + + expect(result.success).toBe(true) + expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ retries: 0 })) + }) + + it('keeps an explicit auth type when an OAuth client ID is also supplied', async () => { + mockGenerateMcpServerId.mockReturnValue('server-1') + dbChainMockFns.limit.mockResolvedValueOnce([]) + dbChainMockFns.limit.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Example', + transport: 'streamable-http', + url: 'https://example.com/anything', + authType: 'headers', + }, + ]) + + const result = await performCreateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + name: 'Example', + url: 'https://example.com/anything', + authType: 'headers', + oauthClientId: 'client-1', + oauthClientIdProvided: true, + }) + + expect(result.success).toBe(true) + expect(result.authType).toBe('headers') + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ authType: 'headers', oauthClientId: 'client-1' }) + ) + }) + it('leaves a re-registered server disconnected until discovery re-runs', async () => { mockGenerateMcpServerId.mockReturnValue('server-1') dbChainMockFns.limit.mockResolvedValueOnce([ diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index ca9ecde3730..f0a7cc64f34 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -142,7 +142,13 @@ export async function createMcpServer( const transport = params.transport || 'streamable-http' const timeout = params.timeout || 30000 - const retries = params.retries || 3 + /** + * `0` is a published, in-range value meaning "no retry", so the default may + * only apply when the field is absent. `||` folded it into `3`, which is the + * opposite of what the caller asked for; the update path already guards on + * `!== undefined`. + */ + const retries = params.retries ?? 3 const enabled = params.enabled !== false const serverId = params.url ? generateMcpServerId(params.workspaceId, params.url) : generateId() @@ -195,7 +201,14 @@ export async function createMcpServer( } } } - if (params.oauthClientId) resolvedAuthType = 'oauth' + /** + * An OAuth client id only *implies* an auth type; it may not overrule one + * the caller stated. Unconditional promotion turned an explicit + * `authType: 'headers'` into `oauth`, so the caller's own header + * configuration was never used to authenticate. The update path already + * promotes only when `authType` is absent. + */ + if (!params.authType && params.oauthClientId) resolvedAuthType = 'oauth' if (existingServer) { const credsChanged = await oauthCredsChanged({ @@ -378,6 +391,7 @@ export async function updateMcpServer( .select({ url: mcpServers.url, authType: mcpServers.authType, + headers: mcpServers.headers, oauthClientId: mcpServers.oauthClientId, oauthClientSecret: mcpServers.oauthClientSecret, }) @@ -415,8 +429,26 @@ export async function updateMcpServer( // Turning OAuth off must revoke and delete its now-orphaned tokens, not just reset the connection. const oauthDisabled = currentServer.authType === 'oauth' && resolvedAuthType !== 'oauth' const shouldClearOauth = urlChanged || credsChanged || oauthDisabled + /** + * On a `headers` server the headers *are* the credential, so rotating them + * invalidates the connection exactly as an OAuth credential change does — + * and the registration path already counts headers as a connection input. + * The reset is scoped to that auth type: under `oauth` (or `none`) the + * headers authenticate nothing, and clearing an OAuth server's status + * strands it, since discovery only reruns for an OAuth row that is + * `connected`. Header revocation never revokes the OAuth grant, so this + * stays out of `shouldClearOauth`. + */ + const headersInvalidateAuth = + resolvedAuthType === 'headers' && + params.headers !== undefined && + !isEqual(currentServer.headers ?? {}, params.headers) // An auth-type flip (either direction) or OAuth creds/URL change invalidates the connection: reset and clear stale state. - if (authTypeChanged || (shouldClearOauth && resolvedAuthType === 'oauth')) { + if ( + authTypeChanged || + headersInvalidateAuth || + (shouldClearOauth && resolvedAuthType === 'oauth') + ) { updateData.connectionStatus = 'disconnected' updateData.lastConnected = null updateData.lastError = null diff --git a/apps/sim/lib/table/application/groups.test.ts b/apps/sim/lib/table/application/groups.test.ts index 718790a6450..0375eb0c290 100644 --- a/apps/sim/lib/table/application/groups.test.ts +++ b/apps/sim/lib/table/application/groups.test.ts @@ -111,6 +111,29 @@ const table: TableDefinition = { createdAt: new Date('2026-08-01T00:00:00.000Z'), updatedAt: new Date('2026-08-01T00:00:00.000Z'), } +/** An enrichment group stores `workflowId: ''` — there is no workflow to resolve. */ +const enrichmentGroup: WorkflowGroup = { + id: 'group-enrichment', + workflowId: '', + enrichmentId: 'company-domain', + type: 'enrichment', + outputs: [{ blockId: '', path: '', outputId: 'domain', columnName: 'column-domain' }], +} +const enrichmentTable: TableDefinition = { + ...table, + schema: { + columns: [ + { id: 'column-name', name: 'name', type: 'string' }, + { + id: 'column-domain', + name: 'domain', + type: 'string', + workflowGroupId: 'group-enrichment', + }, + ], + workflowGroups: [enrichmentGroup], + }, +} const principal = { kind: 'delegated' as const, serviceId: 'copilot' as const, @@ -151,6 +174,37 @@ function tableWithGroup(nextGroup: WorkflowGroup, columns = table.schema.columns } } +/** + * Points the command at the enrichment table and a registry entry that defines a + * second output, so an extension has something valid to ask for. + */ +function useEnrichmentTable(): void { + mocks.resolveContext.mockResolvedValue({ + tableId: table.id, + table: enrichmentTable, + workspaceId: table.workspaceId, + workspaceOrganizationId: null, + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + }) + mocks.getEnrichment.mockReturnValue({ + id: 'company-domain', + name: 'Company Domain', + inputs: [{ id: 'company', name: 'Company', type: 'string', required: true }], + outputs: [ + { id: 'domain', name: 'domain', type: 'string' }, + { id: 'company_name', name: 'company name', type: 'string' }, + ], + }) + mocks.updateGroup.mockImplementation(async (input) => ({ + ...enrichmentTable, + schema: { + ...enrichmentTable.schema, + workflowGroups: [{ ...enrichmentGroup, outputs: input.outputs ?? enrichmentGroup.outputs }], + }, + })) +} + describe('workflow and enrichment Table application commands', () => { beforeEach(() => { vi.clearAllMocks() @@ -406,6 +460,168 @@ describe('workflow and enrichment Table application commands', () => { ) }) + it('extends an enrichment group with a registry output without resolving a workflow', async () => { + useEnrichmentTable() + + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + outputs: [ + ...enrichmentGroup.outputs, + { blockId: '', path: '', outputId: 'company_name', columnName: 'zz_z' }, + ], + newOutputColumns: [{ name: 'zz_z', type: 'string' }], + }, + }) + + expect(mocks.resolveWorkflowContext).not.toHaveBeenCalled() + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ + groupId: enrichmentGroup.id, + newOutputColumns: [{ name: 'zz_z', type: 'string', workflowGroupId: enrichmentGroup.id }], + }), + 'request-1' + ) + }) + + it('refuses an enrichment output the registry does not define', async () => { + useEnrichmentTable() + + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + outputs: [ + ...enrichmentGroup.outputs, + { blockId: '', path: '', outputId: 'invented', columnName: 'zz_z' }, + ], + newOutputColumns: [{ name: 'zz_z', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Enrichment "Company Domain" has no output "invented"', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + + it('refuses an enrichment output coordinate that carries no registry output id', async () => { + useEnrichmentTable() + + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + outputs: [...enrichmentGroup.outputs, { blockId: '', path: 'name', columnName: 'zz_z' }], + newOutputColumns: [{ name: 'zz_z', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Enrichment "Company Domain" has no output ""', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + }) + + it('leaves an untouched enrichment binding alone while renaming the group', async () => { + useEnrichmentTable() + + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + name: 'Renamed enrichment', + outputs: enrichmentGroup.outputs, + }, + }) + + expect(mocks.getEnrichment).not.toHaveBeenCalled() + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ name: 'Renamed enrichment' }), + 'request-1' + ) + }) + + it('names the enrichment instead of a missing workflow for a mapping update', async () => { + useEnrichmentTable() + + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + mappingUpdates: [{ columnName: 'column-domain', blockId: 'block-1', path: 'content' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Mapping updates are not supported for an enrichment group; send outputs[] instead', + }) + + expect(mocks.resolveWorkflowContext).not.toHaveBeenCalled() + expect(mocks.updateGroup).not.toHaveBeenCalled() + }) + + it('still resolves the workflow for a new output coordinate on a manual group', async () => { + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + outputs: [...group.outputs, { blockId: 'block-2', path: 'score', columnName: 'score' }], + newOutputColumns: [{ name: 'score', type: 'number' }], + }, + }) + + expect(mocks.resolveWorkflowContext).toHaveBeenCalledWith({ + workflowId: 'workflow-1', + assertedWorkspaceId: 'workspace-1', + }) + expect(mocks.getEnrichment).not.toHaveBeenCalled() + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ + newOutputColumns: [{ name: 'score', type: 'number', workflowGroupId: group.id }], + }), + 'request-1' + ) + }) + + it('refuses an output column no output names instead of dropping it', async () => { + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + newOutputColumns: [{ name: 'zz_w', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'newOutputColumns entry "zz_w" has no matching outputs[].columnName', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + }) + it('does not start auto-run when the generic update saves a legacy enabled group', async () => { await updateTableGroupUseCase.execute({ principal, diff --git a/apps/sim/lib/table/application/groups.ts b/apps/sim/lib/table/application/groups.ts index b842b0a60c0..a9e692d6c10 100644 --- a/apps/sim/lib/table/application/groups.ts +++ b/apps/sim/lib/table/application/groups.ts @@ -37,6 +37,7 @@ import { resolveActiveWorkflowApplicationContext } from '@/lib/workflows/applica import type { ResolveWorkflowOutputsResult } from '@/lib/workflows/application/resolve-workflow-outputs' import { loadResolvedWorkflowOutputs } from '@/lib/workflows/application/resolve-workflow-outputs' import { getEnrichment } from '@/enrichments/registry' +import type { EnrichmentConfig } from '@/enrichments/types' const logger = createLogger('TableGroupApplication') @@ -120,6 +121,38 @@ function validateRequestedOutputs( ) } +/** Resolves the registry enrichment a group is bound to, refusing an unknown id. */ +function requireEnrichment(enrichmentId: string | undefined): EnrichmentConfig { + const enrichment = getEnrichment(enrichmentId) + if (!enrichment) { + throw new OrchestrationError( + 'validation', + `Unknown enrichment "${enrichmentId ?? ''}". Call list_enrichments to see available ids.` + ) + } + return enrichment +} + +/** + * Refuses an output id the enrichment registry does not define. A run fills a + * cell by reading `result[outputId]`, so a coordinate carrying an unknown — or + * absent — output id names a column no run can ever write. + */ +function requireKnownEnrichmentOutputIds( + enrichment: EnrichmentConfig, + outputIds: Array +): void { + const known = new Set(enrichment.outputs.map((output) => output.id)) + for (const outputId of outputIds) { + if (!outputId || !known.has(outputId)) { + throw new OrchestrationError( + 'validation', + `Enrichment "${enrichment.name}" has no output "${outputId ?? ''}"` + ) + } + } +} + function workflowOutputColumnType( requestedType: string | undefined, resolvedLeafType: string | undefined @@ -413,13 +446,7 @@ export const createTableEnrichmentGroup = defineAuthorizedTableUseCase({ `Enrichment output names cannot exceed ${TABLE_LIMITS.MAX_COLUMNS_PER_TABLE} entries` ) } - const enrichment = getEnrichment(input.enrichmentId) - if (!enrichment) { - throw new OrchestrationError( - 'validation', - `Unknown enrichment "${input.enrichmentId}". Call list_enrichments to see available ids.` - ) - } + const enrichment = requireEnrichment(input.enrichmentId) const enrichmentInputIds = new Set( enrichment.inputs.map((enrichmentInput) => enrichmentInput.id) @@ -440,15 +467,7 @@ export const createTableEnrichmentGroup = defineAuthorizedTableUseCase({ } mappingByInput.set(mapping.inputName, mapping.columnName) } - const enrichmentOutputIds = new Set(enrichment.outputs.map((output) => output.id)) - for (const outputId of Object.keys(input.outputColumnNames ?? {})) { - if (!enrichmentOutputIds.has(outputId)) { - throw new OrchestrationError( - 'validation', - `Enrichment "${enrichment.name}" has no output "${outputId}"` - ) - } - } + requireKnownEnrichmentOutputIds(enrichment, Object.keys(input.outputColumnNames ?? {})) const existingColumns = new Set(context.table.schema.columns.map((column) => column.name)) for (const enrichmentInput of enrichment.inputs) { const mapped = mappingByInput.get(enrichmentInput.id) @@ -567,15 +586,79 @@ export const updateTableGroupUseCase = defineAuthorizedTableUseCase({ const previousGroup = (context.table.schema.workflowGroups ?? []).find( (group) => group.id === input.groupId ) + /** + * An enrichment group's outputs come from the registry, not from a workflow, + * and it stores `workflowId: ''` — so there is nothing to resolve a new + * output coordinate against. Validating one anyway resolved the empty id and + * answered `404 Workflow not found`, which made an enrichment group's output + * set permanently unextendable. Only a body that supplies a `workflowId` + * converts the group to workflow-backed and needs workflow metadata. + */ + const producerIsEnrichment = + input.workflowId === undefined && + previousGroup !== undefined && + (previousGroup.type === 'enrichment' || !previousGroup.workflowId) + /** + * Skipping workflow resolution must not mean skipping validation. An + * enrichment run fills a column by registry `outputId`, so a coordinate the + * registry does not define is a column nothing can ever populate. Hold an + * added — or repointed — output to the same check enrichment creation + * applies, and leave an untouched existing binding alone so a group whose + * enrichment has since changed stays editable. + */ + if (producerIsEnrichment && input.outputs?.length) { + const boundOutputKeys = new Set( + previousGroup?.outputs.map((output) => `${output.columnName}::${output.outputId ?? ''}`) ?? + [] + ) + const addedOutputs = input.outputs.filter( + (output) => !boundOutputKeys.has(`${output.columnName}::${output.outputId ?? ''}`) + ) + if (addedOutputs.length > 0) { + requireKnownEnrichmentOutputIds( + requireEnrichment(previousGroup?.enrichmentId), + addedOutputs.map((output) => output.outputId) + ) + } + } + /** + * `mappingUpdates` repoints a column at a new `(blockId, path)` — coordinates + * an enrichment output does not have and the writer cannot translate into an + * `outputId`. Say that, rather than resolving the group's empty workflow id + * and answering `404 Workflow not found`. + */ + if (producerIsEnrichment && input.mappingUpdates?.length) { + throw new OrchestrationError( + 'validation', + 'Mapping updates are not supported for an enrichment group; send outputs[] instead' + ) + } + /** + * A `newOutputColumns` entry that no resulting output names is dropped by the + * writer, so a caller asking for a column got a 200 and no column. Refuse it + * the way group creation refuses an orphan `outputColumns` entry. + */ + if (input.newOutputColumns?.length) { + const requestedOutputNames = new Set((input.outputs ?? []).map((output) => output.columnName)) + const orphan = input.newOutputColumns.find((column) => !requestedOutputNames.has(column.name)) + if (orphan) { + throw new OrchestrationError( + 'validation', + `newOutputColumns entry "${orphan.name}" has no matching outputs[].columnName` + ) + } + } const previousOutputKeys = new Set( previousGroup?.outputs.map((output) => `${output.blockId}::${output.path}`) ?? [] ) const workflowChanged = input.workflowId !== undefined && input.workflowId !== previousGroup?.workflowId - const outputCoordinatesToValidate = - input.outputs?.filter( - (output) => workflowChanged || !previousOutputKeys.has(`${output.blockId}::${output.path}`) - ) ?? [] + const outputCoordinatesToValidate = producerIsEnrichment + ? [] + : (input.outputs?.filter( + (output) => + workflowChanged || !previousOutputKeys.has(`${output.blockId}::${output.path}`) + ) ?? []) const workflowMetadataRequired = input.workflowId !== undefined || outputCoordinatesToValidate.length > 0 || diff --git a/apps/sim/lib/table/views/service.test.ts b/apps/sim/lib/table/views/service.test.ts index 1cc5c7ec5c0..a76c568d310 100644 --- a/apps/sim/lib/table/views/service.test.ts +++ b/apps/sim/lib/table/views/service.test.ts @@ -408,6 +408,42 @@ describe('view config column-reference normalization', () => { expect(dbChainMockFns.insert).not.toHaveBeenCalled() }) + /** + * The layout half used to answer 200 and store a name no read ever shows, + * while the same unknown name in `filter` answered 400 — one request, two + * policies. + */ + it('refuses a hidden column that does not exist for a strict caller', async () => { + queueTableRows(tableViews, [{ total: 0 }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect(create({ hiddenColumns: ['ghost'] })).rejects.toMatchObject({ + name: 'TableViewValidationError', + }) + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + + it('refuses an unknown column in every other layout key too', async () => { + for (const config of [ + { columnOrder: ['ghost'] }, + { pinnedColumns: ['ghost'] }, + { columnWidths: { ghost: 120 } }, + ]) { + queueTableRows(tableViews, [{ total: 0 }]) + await expect(create(config)).rejects.toMatchObject({ name: 'TableViewValidationError' }) + } + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + + it('keeps storing a first-party layout reference that no longer resolves', async () => { + queueTableRows(tableViews, [{ total: 0 }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await create({ hiddenColumns: ['col_gone'] }, false) + + expect(insertedConfig().hiddenColumns).toEqual(['col_gone']) + }) + it('refuses a sort on a column that does not exist for a strict caller', async () => { queueTableRows(tableViews, [{ total: 0 }]) dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) @@ -511,6 +547,61 @@ describe('view config column-reference normalization', () => { ).rejects.toMatchObject({ name: 'TableViewValidationError' }) }) + /** + * The carried-forward exemption exists so a dangling FILTER ref stays + * writable, not so it becomes a valid target for a NEW layout ref. Without + * scoping, a strict caller could store `hiddenColumns: ['col_gone']` purely + * because `col_gone` survives in the stored filter — a layout entry the very + * next read drops, which is the asymmetry the strict check closes. + */ + it('refuses a NEW layout reference that resolves only via a carried-forward filter ref', async () => { + const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } + queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect( + updateTableView({ + viewId: 'view-1', + tableId: 'table-1', + config: { filter: stale, hiddenColumns: ['col_gone'] }, + columns, + strictRefs: true, + }) + ).rejects.toMatchObject({ name: 'TableViewValidationError' }) + expect(dbChainMockFns.update).not.toHaveBeenCalled() + }) + + it('still lets a strict save carry the stale filter reference forward with a valid layout', async () => { + const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } + queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect( + updateTableView({ + viewId: 'view-1', + tableId: 'table-1', + config: { filter: stale, hiddenColumns: ['col_a'] }, + columns, + strictRefs: true, + }) + ).resolves.not.toBeNull() + }) + + it('leaves the non-strict grid path free to save that same layout reference', async () => { + const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } + queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) + dbChainMockFns.returning.mockResolvedValueOnce([storedRow]) + + await expect( + updateTableView({ + viewId: 'view-1', + tableId: 'table-1', + config: { filter: stale, hiddenColumns: ['col_gone'] }, + columns, + }) + ).resolves.not.toBeNull() + }) + it('accepts that same new reference from a first-party caller', async () => { const stale = { all: [{ field: 'col_gone', op: 'eq' as const, value: 'x' }] } queueTableRows(tableViews, [{ ...storedRow, config: { filter: stale } }]) diff --git a/apps/sim/lib/table/views/service.ts b/apps/sim/lib/table/views/service.ts index 1cd7462091e..68a0ddce1e2 100644 --- a/apps/sim/lib/table/views/service.ts +++ b/apps/sim/lib/table/views/service.ts @@ -177,10 +177,19 @@ function tolerantColumns( * * `filter` and `sort` are then validated: a predicate or sort naming no column * is one the query routes answer 400 for, so storing it would save a view that - * can never load. Column LAYOUT is deliberately not validated — it auto-saves as + * can never load. Column LAYOUT is not validated by default — it auto-saves as * the user drags, and racing a concurrent column delete must self-heal through * {@link pruneViewConfig}, not fail the drag. * + * `strictLayoutRefs` extends the same refusal to the LAYOUT keys, for the same + * caller that gets it on `filter` and `sort`: one request naming an unknown + * column twice answered 400 for the filter leaf and 200 for the hidden column, + * whose name was then stored in a blob no read ever shows (`pruneViewConfig` + * drops it) and would silently take effect if a column of that name were later + * created. It stays off for the first-party grid, whose layout auto-saves as the + * user drags and must self-heal past a concurrent column delete. A v2 caller + * cannot carry a dangling layout ref forward: the read it echoes is pruned. + * * `carriedForward` names the references that are exempt from that refusal. * Deleting a column leaves every view that filtered on it dangling — * `pruneViewConfig` deliberately does not prune a filter — so without the @@ -191,11 +200,20 @@ function tolerantColumns( * already held, so a reference the caller INTRODUCES is refused; a first-party * caller exempts its own refs too, which is the behavior the grid has always * had — see {@link CreateTableViewData.strictRefs}. + * + * The exemption is scoped to `filter` and `sort`, the only halves it exists for: + * the layout check runs against the LIVE columns, not the tolerant set. A + * carried-forward placeholder standing in for a deleted column named in the + * stored filter must not also make that name a valid target for a new layout + * ref, which `pruneViewConfig` would drop on the very next read — the same + * write-accepts / read-discards asymmetry `strictLayoutRefs` closes. Validating + * against `columns` makes the strict path accept exactly what a read keeps. */ export function normalizeViewConfigForStorage( config: TableViewConfig, columns: ColumnDefinition[], - carriedForward: readonly string[] = [] + carriedForward: readonly string[] = [], + strictLayoutRefs = false ): TableViewConfig { const stored = remapViewConfigColumnRefs(config, viewConfigRefMap(columns)) const known = tolerantColumns(columns, carriedForward) @@ -208,9 +226,38 @@ export function normalizeViewConfigForStorage( } throw error } + if (strictLayoutRefs) assertLayoutRefsResolve(stored, columns) return stored } +/** The layout keys, paired with the references each one holds. */ +function layoutRefEntries(config: TableViewConfig): Array<[string, readonly string[]]> { + return [ + ['columnOrder', config.columnOrder ?? []], + ['pinnedColumns', config.pinnedColumns ?? []], + ['hiddenColumns', config.hiddenColumns ?? []], + ['columnWidths', Object.keys(config.columnWidths ?? {})], + ] +} + +/** + * Refuses a layout reference naming no column. Mirrors what the query + * validators do for `filter` and `sort`, including the message shape, so one + * config gets one answer whichever half the unknown name landed in. + */ +function assertLayoutRefsResolve(config: TableViewConfig, known: ColumnDefinition[]): void { + const live = new Set(known.map(getColumnId)) + for (const [key, refs] of layoutRefEntries(config)) { + for (const ref of refs) { + if (!live.has(ref)) { + throw new TableViewValidationError( + `Unknown ${key} column "${ref}". It is not a column on this table.` + ) + } + } + } +} + /** * Migrates a config stored before the grammar switch. The feature never * released, so legacy-shaped rows exist only from pre-refactor testing: a @@ -371,9 +418,9 @@ export interface CreateTableViewData { userId: string columns: ColumnDefinition[] /** - * Whether to refuse a filter or sort reference naming no live column. Set by - * the `/api/v2` surface only, whose caller authored the config in this request - * and can be told which reference was wrong. + * Whether to refuse a filter, sort, or column-layout reference naming no live + * column. Set by the `/api/v2` surface only, whose caller authored the config + * in this request and can be told which reference was wrong. * * Absent — the first-party grid, which does not author these refs so much as * carry them: a view filtered on a since-deleted column keeps the dangling @@ -405,7 +452,8 @@ export async function createTableView(data: CreateTableViewData): Promise { @@ -500,11 +548,21 @@ export async function updateTableView(data: UpdateTableViewData): Promise = { updatedAt: new Date() } if (data.name !== undefined) patch.name = normalizeName(data.name)