From cf6a14a46643f2f9a26c915769acc8edd6a7bd64 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 14:10:48 -0700 Subject: [PATCH] fix(v2): stop telling callers something the server did not do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A works-as-advertised sweep of the v2 surface found one defect class in five places: input is validated for shape, then its meaning is re-derived independently by each consumer — so a filter compiles differently than it validated, or a write commits and the response then reports failure. Knowledge tag filters were validated once and re-parsed three times. The document list read `Number()`, search read `parseFloat()`; the list matched booleans case-insensitively, search compared against the literal `'true'`; the list escaped LIKE metacharacters, search did not; and the date pattern was tested against the untrimmed string the validator had already trimmed. Two of those paths dropped the predicate entirely and answered 200 with the whole knowledge base — on a billed endpoint. Values are now coerced once, where the resolved field type is known, and both builders consume the result. A builder that cannot compile an already-validated filter now raises instead of silently widening the result set. `PUT /api/v2/secrets/{name}` with `scope: personal` committed the secret and then answered 500, because a user-global write was reported through a workspace-scoped mirror lookup, and an org admin's inherited access has no `permissions` row for the fan-out to find. The personal path no longer decides success from a per-workspace mirror. The `workspaceId` descriptions said a personal secret lives in one workspace; it does not, and they now say so. A custom tool could be stored with a schema the read path cannot serialize — `POST /workflows/import` and Copilot both wrote through name-only checks — so one row made the whole workspace list 500, and a title-only PATCH committed, audited, then reported failure. Every write now passes the same guard the response schema is derived from. `POST /api/v2/tables` accepted `workflowGroupId` on an initial column. Nothing can populate it legitimately, and it made every later column-add and group-add fail with no way to clear it. The key is refused at the boundary, and `createTable` now runs the invariant every later mutation already runs, closing the internal and v1 ingresses too. Those invariants moved to a leaf module: reaching them through `workflow-columns` pulled the executable tool registry into the tables page graph, taking it from 1,767 modules to 6,999. An out-of-range upload part number answered 500 rather than the 400 its published contract promises, because the throw happened above the route's try/catch and was not an `HttpError`. --- apps/docs/openapi-v2-resources.json | 16 +- apps/docs/openapi-v2-tables.json | 4 - .../parts/[partNumber]/route.test.ts | 56 ++++- .../[uploadId]/parts/[partNumber]/route.ts | 20 +- .../api/contracts/v2/__tests__/tables.test.ts | 21 ++ apps/sim/lib/api/contracts/v2/secrets.ts | 12 +- apps/sim/lib/api/contracts/v2/tables.ts | 23 +- apps/sim/lib/credentials/environment.ts | 50 ++++- .../application/use-cases.test.ts | 69 +++++- .../lib/custom-tools/application/use-cases.ts | 19 +- apps/sim/lib/custom-tools/schema.ts | 32 +++ apps/sim/lib/knowledge/documents/service.ts | 6 +- .../knowledge/documents/tag-filter.test.ts | 60 +++++ .../sim/lib/knowledge/documents/tag-filter.ts | 41 ++-- apps/sim/lib/knowledge/search/queries.test.ts | 171 ++++++++++++++ apps/sim/lib/knowledge/search/queries.ts | 67 +++--- apps/sim/lib/knowledge/tags/utils.test.ts | 70 ++++++ apps/sim/lib/knowledge/tags/utils.ts | 126 ++++++++--- .../lib/secrets/application/use-cases.test.ts | 147 +++++++++++- apps/sim/lib/secrets/application/use-cases.ts | 113 ++++++++-- apps/sim/lib/table/columns/service.ts | 2 +- apps/sim/lib/table/schema-invariants.ts | 210 ++++++++++++++++++ apps/sim/lib/table/service.test.ts | 98 ++++++++ apps/sim/lib/table/service.ts | 11 + apps/sim/lib/table/workflow-columns.ts | 197 ---------------- .../lib/table/workflow-groups/service.test.ts | 9 +- apps/sim/lib/table/workflow-groups/service.ts | 3 +- .../uploads/upload-session/service.test.ts | 28 +++ .../sim/lib/uploads/upload-session/service.ts | 5 +- .../workflows/custom-tools/operations.test.ts | 62 ++++++ .../lib/workflows/custom-tools/operations.ts | 9 + .../custom-tools-persistence.test.ts | 93 ++++++++ .../persistence/custom-tools-persistence.ts | 12 + 33 files changed, 1518 insertions(+), 344 deletions(-) create mode 100644 apps/sim/lib/custom-tools/schema.ts create mode 100644 apps/sim/lib/knowledge/search/queries.test.ts create mode 100644 apps/sim/lib/knowledge/tags/utils.test.ts create mode 100644 apps/sim/lib/table/schema-invariants.ts create mode 100644 apps/sim/lib/table/service.test.ts create mode 100644 apps/sim/lib/workflows/custom-tools/operations.test.ts create mode 100644 apps/sim/lib/workflows/persistence/custom-tools-persistence.test.ts diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 6b7fe8bd62b..28792358443 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -1965,22 +1965,22 @@ "name": "workspaceId", "in": "query", "required": true, - "description": "Workspace in which the secret is available.", + "description": "Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces.", "schema": { "type": "string", "minLength": 1, - "description": "Workspace in which the secret is available." + "description": "Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces." } }, { "name": "scope", "in": "query", "required": true, - "description": "Whether the secret belongs to the workspace or the caller.", + "description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace.", "schema": { "type": "string", "enum": ["workspace", "personal"], - "description": "Whether the secret belongs to the workspace or the caller." + "description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace." } } ], @@ -4088,7 +4088,7 @@ "scope": { "type": "string", "enum": ["workspace", "personal"], - "description": "Whether the secret belongs to the workspace or the caller." + "description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace." }, "role": { "type": "string", @@ -4184,12 +4184,12 @@ "workspaceId": { "type": "string", "minLength": 1, - "description": "Workspace in which the secret is available." + "description": "Workspace the request is authorized against. A workspace secret is written to it; a personal secret is written to the caller and is available in all of their workspaces." }, "scope": { "type": "string", "enum": ["workspace", "personal"], - "description": "Whether the secret belongs to the workspace or the caller." + "description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace." }, "value": { "type": "string", @@ -4224,7 +4224,7 @@ "scope": { "type": "string", "enum": ["workspace", "personal"], - "description": "Whether the secret belongs to the workspace or the caller." + "description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace." }, "deleted": { "type": "boolean", diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index a4aff379376..173c373bc16 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -4370,10 +4370,6 @@ "description": "ISO 4217 code for currency columns.", "type": "string", "pattern": "^[A-Za-z]{3}$" - }, - "workflowGroupId": { - "description": "Workflow group initially associated with the column.", - "type": "string" } }, "required": ["name", "type"], diff --git a/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.test.ts b/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.test.ts index c63a810e141..4a9e4ce0b9c 100644 --- a/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.test.ts +++ b/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.test.ts @@ -29,6 +29,7 @@ vi.mock('@/lib/uploads/upload-session/service', () => ({ verifyUploadSessionToken: mockVerifyUploadSessionToken, })) +import { OrchestrationError } from '@/lib/core/orchestration/types' import { PUT } from '@/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route' const SESSION = { @@ -37,6 +38,8 @@ const SESSION = { method: 'multipart', status: 'uploading', expiresAt: new Date('2999-01-01T00:00:00.000Z'), + partSize: 3, + partCount: 2, } as const describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => { @@ -84,6 +87,41 @@ describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => { expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled() }) + /** + * The part number is a path segment of a session-scoped signed URL, so any + * holder of a legitimate part URL can address a part the session does not + * have. `expectedUploadPartSize` classifies that as a validation failure; + * `service.test.ts` pins that classification on the real implementation, + * which this suite mocks away. + */ + it('maps an out-of-range part number to the documented 400', async () => { + mockExpectedUploadPartSize.mockImplementation(() => { + throw new OrchestrationError('validation', 'partNumber must be between 1 and 2') + }) + + const response = await request({ partNumber: '99' }) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: { code: 'BAD_REQUEST', message: 'partNumber must be between 1 and 2' }, + }) + expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled() + }) + + it('still renders an unclassified part-size failure as a generic 500', async () => { + mockExpectedUploadPartSize.mockImplementation(() => { + throw new Error('unexpected') + }) + + const response = await request() + + expect(response.status).toBe(500) + await expect(response.json()).resolves.toEqual({ + error: { code: 'INTERNAL_ERROR', message: 'Internal server error' }, + }) + expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled() + }) + it('rejects expired upload sessions before writing the part', async () => { mockVerifyUploadSessionToken.mockReturnValue({ ...SESSION, @@ -101,17 +139,21 @@ describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => { }) }) -function request(options?: { contentLength?: string | null }) { +function request(options?: { contentLength?: string | null; partNumber?: string }) { const headers = new Headers({ 'Content-Type': 'application/octet-stream' }) if (options?.contentLength !== null) { headers.set('Content-Length', options?.contentLength ?? '3') } + const partNumber = options?.partNumber ?? '1' return PUT( - new NextRequest('http://localhost:3000/api/v2/uploads/upload-1/parts/1?token=signed-token', { - method: 'PUT', - headers, - body: new Uint8Array([1, 2, 3]), - }), - { params: Promise.resolve({ uploadId: 'upload-1', partNumber: '1' }) } + new NextRequest( + `http://localhost:3000/api/v2/uploads/upload-1/parts/${partNumber}?token=signed-token`, + { + method: 'PUT', + headers, + body: new Uint8Array([1, 2, 3]), + } + ), + { params: Promise.resolve({ uploadId: 'upload-1', partNumber }) } ) } diff --git a/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts b/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts index dca212587dc..7a3aa46a8a3 100644 --- a/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts +++ b/apps/sim/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route.ts @@ -12,7 +12,12 @@ import { type UploadSessionRecord, verifyUploadSessionToken, } from '@/lib/uploads/upload-session/service' -import { v2Error, v2HttpError, v2UploadDataPlaneError } from '@/app/api/v2/lib/response' +import { + v2CaughtOrchestrationError, + v2Error, + v2HttpError, + v2UploadDataPlaneError, +} from '@/app/api/v2/lib/response' interface LocalPartRouteParams { params: Promise<{ uploadId: string; partNumber: string }> @@ -60,7 +65,18 @@ export const PUT = withRouteHandler( } const { partNumber } = parsed.data.params - const expectedSize = expectedUploadPartSize(session, partNumber) + let expectedSize: number + try { + expectedSize = expectedUploadPartSize(session, partNumber) + } catch (error) { + // The part number is a path segment of a session-scoped signed URL, so a + // caller can address a part this session does not have. That refusal is a + // classified domain failure, and the data plane's generic 500 tail would + // otherwise render it as an internal error. + const classified = v2CaughtOrchestrationError(error) + if (classified) return classified + throw error + } const contentLength = request.headers.get('content-length') if (contentLength !== null && Number(contentLength) !== expectedSize) { return v2Error('BAD_REQUEST', `Part ${partNumber} must contain exactly ${expectedSize} bytes`) 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 f21685a4f1e..0022839b939 100644 --- a/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts +++ b/apps/sim/lib/api/contracts/v2/__tests__/tables.test.ts @@ -59,6 +59,27 @@ describe('v2 table column contracts', () => { ).toMatchObject({ success: true, data: { updates: { required: true } } }) }) + /** + * v2 mints workflow group ids server-side and has no way to declare a group + * on the create body, so any id a caller supplied would name a group that + * does not exist. `createTable` does not check that, but every later schema + * mutation does — accepting the field made the created table's columns and + * groups permanently unaddable, with nothing on the update body able to clear + * it. + */ + it('refuses a workflow group id on an initial column', () => { + const result = v2CreateTableBodySchema.safeParse({ + workspaceId: WORKSPACE_ID, + name: 'contacts', + schema: { + columns: [{ name: 'email', type: 'string', workflowGroupId: 'wfg_does_not_exist' }], + }, + }) + + expect(result.success).toBe(false) + expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys') + }) + it('keeps required in table responses for existing stored schemas', () => { expect( v2ApiTableSchema.safeParse({ diff --git a/apps/sim/lib/api/contracts/v2/secrets.ts b/apps/sim/lib/api/contracts/v2/secrets.ts index a4177e8b4df..067bdc81ff5 100644 --- a/apps/sim/lib/api/contracts/v2/secrets.ts +++ b/apps/sim/lib/api/contracts/v2/secrets.ts @@ -15,7 +15,9 @@ const SECRET_NAME_REGEX = /^[A-Za-z0-9_]+$/ export const v2SecretScopeSchema = z .enum(['workspace', 'personal']) - .describe('Whether the secret belongs to the workspace or the caller.') + .describe( + 'Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace.' + ) export type V2SecretScope = z.output export const v2SecretNameSchema = z @@ -76,7 +78,9 @@ export type V2SecretParams = z.output export const v2SetSecretBodySchema = z .object({ - workspaceId: workspaceIdSchema.describe('Workspace in which the secret is available.'), + workspaceId: workspaceIdSchema.describe( + 'Workspace the request is authorized against. A workspace secret is written to it; a personal secret is written to the caller and is available in all of their workspaces.' + ), scope: v2SecretScopeSchema, value: z .string() @@ -90,7 +94,9 @@ export type V2SetSecretBody = z.input export const v2DeleteSecretQuerySchema = z .object({ - workspaceId: workspaceIdSchema.describe('Workspace in which the secret is available.'), + workspaceId: workspaceIdSchema.describe( + 'Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces.' + ), scope: v2SecretScopeSchema, }) .strict() diff --git a/apps/sim/lib/api/contracts/v2/tables.ts b/apps/sim/lib/api/contracts/v2/tables.ts index a0ff1b6984a..38ff366b4b4 100644 --- a/apps/sim/lib/api/contracts/v2/tables.ts +++ b/apps/sim/lib/api/contracts/v2/tables.ts @@ -390,24 +390,23 @@ export const v2TableColumnInputSchema = z .strict() .superRefine(refineColumnOptions) -const v2InitialTableColumnInputSchema = z - .object({ - ...v2TableColumnInputShape, - workflowGroupId: z - .string() - .optional() - .describe('Workflow group initially associated with the column.'), - }) - .strict() - .superRefine(refineColumnOptions) - +/** + * Initial columns take the same shape as every other v2 column input. + * + * They deliberately cannot name a workflow group: v2 has no way to declare one + * on this body and mints group ids server-side, so any id a caller supplied + * would necessarily dangle. A dangling `workflowGroupId` is a schema invariant + * violation, and `createTable` does not check it while every later schema + * mutation does — so accepting the field made the table's own columns and + * groups permanently unaddable, with no update body field able to clear it. + */ export const v2CreateTableBodySchema = v1CreateTableBodySchema .omit({ folderId: true, schema: true }) .extend({ schema: z .object({ columns: z - .array(v2InitialTableColumnInputSchema) + .array(v2TableColumnInputSchema) .min(1, 'Table must have at least one column') .max( TABLE_LIMITS.MAX_COLUMNS_PER_TABLE, diff --git a/apps/sim/lib/credentials/environment.ts b/apps/sim/lib/credentials/environment.ts index bce8c01158e..6735703d52f 100644 --- a/apps/sim/lib/credentials/environment.ts +++ b/apps/sim/lib/credentials/environment.ts @@ -3,7 +3,7 @@ import { credential, credentialMember, permissions, workspace } from '@sim/db/sc import { permissionSatisfies } from '@sim/platform-authz/workspace' import { chunkArray } from '@sim/utils/helpers' import { generateId } from '@sim/utils/id' -import { and, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm' +import { and, asc, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm' import { acquireUserBillingIdentityLock } from '@/lib/billing/organizations/billing-identity-lock' import type { DbOrTx } from '@/lib/db/types' import { @@ -552,7 +552,53 @@ export async function upsertPersonalEnvCredentialForUser(params: { await db.transaction(upsert) } -/** Deletes one caller-owned personal secret's credential metadata in every workspace. */ +export interface PersonalEnvCredentialMetadata { + id: string + createdAt: Date + updatedAt: Date +} + +/** + * Reads one caller-owned personal secret's credential metadata without scoping to + * a workspace. + * + * A personal secret is stored once per user; the `env_personal` credential rows + * are per-workspace mirrors, so a reader that needs the secret's own timestamps + * must not require a mirror in one particular workspace. The earliest mirror is + * the authoritative creation time — later ones are written when the caller joins + * another workspace, long after the secret itself was created. + */ +export async function getPersonalEnvCredentialMetadata(params: { + userId: string + envKey: string +}): Promise { + const [row] = await db + .select({ + id: credential.id, + createdAt: credential.createdAt, + updatedAt: credential.updatedAt, + }) + .from(credential) + .where( + and( + eq(credential.type, 'env_personal'), + eq(credential.envOwnerUserId, params.userId), + eq(credential.envKey, params.envKey) + ) + ) + .orderBy(asc(credential.createdAt)) + .limit(1) + + return row ?? null +} + +/** + * Deletes one caller-owned personal secret's credential metadata in every workspace. + * + * Deliberately unscoped by workspace: the value being removed alongside it lives + * in the user-global `environment` row, so leaving mirrors behind in other + * workspaces would advertise a secret that no longer exists. + */ export async function deletePersonalEnvCredentialForUser(params: { userId: string envKey: string diff --git a/apps/sim/lib/custom-tools/application/use-cases.test.ts b/apps/sim/lib/custom-tools/application/use-cases.test.ts index fb14babae18..b7f7136c421 100644 --- a/apps/sim/lib/custom-tools/application/use-cases.test.ts +++ b/apps/sim/lib/custom-tools/application/use-cases.test.ts @@ -8,6 +8,8 @@ const { mocks } = vi.hoisted(() => ({ loadContext: vi.fn(), resolvePermission: vi.fn(), getByTitle: vi.fn(), + getWorkspaceTool: vi.fn(), + updateWorkspaceTool: vi.fn(), upsert: vi.fn(), audit: vi.fn(), }, @@ -34,12 +36,12 @@ vi.mock('@/lib/workflows/custom-tools/operations', () => ({ deleteCustomTool: vi.fn(), deleteWorkspaceCustomTool: vi.fn(), getCustomToolById: vi.fn(), - getWorkspaceCustomTool: vi.fn(), + getWorkspaceCustomTool: mocks.getWorkspaceTool, getWorkspaceCustomToolByTitle: mocks.getByTitle, listCustomTools: vi.fn(), listWorkspaceCustomTools: vi.fn(), updateCustomTool: vi.fn(), - updateWorkspaceCustomTool: vi.fn(), + updateWorkspaceCustomTool: mocks.updateWorkspaceTool, upsertCustomTools: mocks.upsert, })) @@ -47,6 +49,7 @@ import { CUSTOM_TOOL_DELEGATION_AUDIENCE } from '@/lib/custom-tools/application/ import { createWorkspaceCustomToolUseCase, saveWorkspaceCustomToolUseCase, + updateWorkspaceCustomToolUseCase, } from '@/lib/custom-tools/application/use-cases' const workspace = { @@ -184,4 +187,66 @@ describe('custom tool application use cases', () => { expect(mocks.audit).not.toHaveBeenCalled() }) + + describe('public update against the shape the response publishes', () => { + const storableSchema = { + type: 'function', + function: { + name: 'lookup_order', + parameters: { type: 'object', properties: { id: { type: 'string' } } }, + }, + } + const session = { kind: 'session' as const, userId: 'user-1', sessionId: 'session-1' } + + it('renames a tool whose stored schema can be published', async () => { + const stored = { ...tool, schema: storableSchema } + mocks.getWorkspaceTool.mockResolvedValueOnce(stored) + mocks.updateWorkspaceTool.mockResolvedValueOnce({ ...stored, title: 'renamed' }) + + const result = await updateWorkspaceCustomToolUseCase.execute({ + principal: session, + input: { workspaceId: workspace.workspaceId, toolId: tool.id, title: 'renamed' }, + }) + + expect(result.tool.title).toBe('renamed') + expect(mocks.updateWorkspaceTool).toHaveBeenCalledWith( + expect.objectContaining({ schema: storableSchema, code: tool.code }) + ) + }) + + it('refuses before committing when the stored schema cannot be published', async () => { + mocks.getWorkspaceTool.mockResolvedValueOnce({ + ...tool, + schema: { function: storableSchema.function }, + }) + + await expect( + updateWorkspaceCustomToolUseCase.execute({ + principal: session, + input: { workspaceId: workspace.workspaceId, toolId: tool.id, title: 'renamed' }, + }) + ).rejects.toMatchObject({ code: 'validation' }) + + expect(mocks.updateWorkspaceTool).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + + it('refuses a supplied schema that cannot be published', async () => { + mocks.getWorkspaceTool.mockResolvedValueOnce({ ...tool, schema: storableSchema }) + + await expect( + updateWorkspaceCustomToolUseCase.execute({ + principal: session, + input: { + workspaceId: workspace.workspaceId, + toolId: tool.id, + schema: { ...storableSchema, type: 'object' }, + }, + }) + ).rejects.toMatchObject({ code: 'validation' }) + + expect(mocks.updateWorkspaceTool).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + }) }) diff --git a/apps/sim/lib/custom-tools/application/use-cases.ts b/apps/sim/lib/custom-tools/application/use-cases.ts index dd0fb4e44de..2027fa84cc0 100644 --- a/apps/sim/lib/custom-tools/application/use-cases.ts +++ b/apps/sim/lib/custom-tools/application/use-cases.ts @@ -11,6 +11,7 @@ import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { customToolDelegationPolicy } from '@/lib/custom-tools/application/authorization' import { customToolOperations } from '@/lib/custom-tools/application/operations' +import { assertStorableCustomToolSchema } from '@/lib/custom-tools/schema' import { loadActiveWorkspaceContext } from '@/lib/uploads/contexts/workspace' import { type CustomToolSortBy, @@ -261,12 +262,21 @@ export const updateWorkspaceCustomToolUseCase = defineAuthorizedWorkspaceUseCase async execute({ input, context }) { const title = input.title ?? context.tool.title await ensureTitleAvailable(context, title) + const schema = input.schema ?? context.tool.schema + /** + * The merged schema, not only a supplied one: this surface parses the tool + * it returns, so an update that falls back to a stored schema the response + * cannot serialize used to commit and audit and only then fail — telling + * the caller the write failed after it had succeeded. Refusing before the + * write makes that error true. + */ + assertStorableCustomToolSchema(schema) try { const tool = await updateWorkspaceCustomTool({ workspaceId: context.workspaceId, toolId: context.tool.id, title, - schema: input.schema ?? context.tool.schema, + schema, code: input.code ?? context.tool.code, }) if (!tool) throw new OrchestrationError('not_found', 'Custom tool not found') @@ -303,6 +313,13 @@ export const updateAvailableCustomToolUseCase = defineAuthorizedWorkspaceUseCase async execute({ principal, input, context }) { const title = input.title ?? context.tool.title await ensureTitleAvailable(context, title) + /** + * Only a supplied schema, unlike the public update above: this surface does + * not parse what it returns, so an edit that merely keeps a legacy stored + * schema still succeeds, while a caller-supplied one is held to the shape + * the public API has to publish. + */ + if (input.schema !== undefined) assertStorableCustomToolSchema(input.schema) try { const tool = await updateCustomTool({ workspaceId: context.workspaceId, diff --git a/apps/sim/lib/custom-tools/schema.ts b/apps/sim/lib/custom-tools/schema.ts new file mode 100644 index 00000000000..f921d52805e --- /dev/null +++ b/apps/sim/lib/custom-tools/schema.ts @@ -0,0 +1,32 @@ +import { customToolSchemaSchema } from '@/lib/api/contracts/tools/custom' +import { OrchestrationError } from '@/lib/core/orchestration/types' + +/** + * Storage invariant for the `custom_tools.schema` column. + * + * The column is published verbatim by the v2 custom tool surface, whose + * response declaration extends {@link customToolSchemaSchema} and is parsed on + * the way out. A stored schema that does not satisfy it cannot be serialized + * back at all: one such row fails the whole `GET /api/v2/custom-tools` page, + * and an update commits and audits before its own response parse throws, so the + * caller is told a write failed after it succeeded. + * + * Every writer is therefore held to the read shape here, against the same + * schema the response is built from, rather than each write path restating the + * check — which is how the two drifted apart in the first place. + */ +export function isStorableCustomToolSchema(schema: unknown): boolean { + return customToolSchemaSchema.safeParse(schema).success +} + +/** {@link isStorableCustomToolSchema} as a caller-fixable validation failure. */ +export function assertStorableCustomToolSchema(schema: unknown): void { + const parsed = customToolSchemaSchema.safeParse(schema) + if (parsed.success) return + const issue = parsed.error.issues[0] + const path = issue?.path.join('.') + throw new OrchestrationError( + 'validation', + `Invalid custom tool schema${path ? ` at ${path}` : ''}: ${issue?.message ?? 'does not match the published function declaration'}` + ) +} diff --git a/apps/sim/lib/knowledge/documents/service.ts b/apps/sim/lib/knowledge/documents/service.ts index 105935e945c..5a209ddc72a 100644 --- a/apps/sim/lib/knowledge/documents/service.ts +++ b/apps/sim/lib/knowledge/documents/service.ts @@ -99,6 +99,7 @@ import { parseBooleanValue, parseDateValue, parseNumberValue, + uncompilableTagFilterError, validateTagValue, } from '@/lib/knowledge/tags/utils' import type { ProcessedDocumentTags } from '@/lib/knowledge/types' @@ -1690,9 +1691,8 @@ export async function getDocuments( if (tagFilters && tagFilters.length > 0) { for (const filter of tagFilters) { const condition = buildTagFilterCondition(filter) - if (condition) { - whereConditions.push(condition) - } + if (!condition) throw uncompilableTagFilterError(filter) + whereConditions.push(condition) } } diff --git a/apps/sim/lib/knowledge/documents/tag-filter.test.ts b/apps/sim/lib/knowledge/documents/tag-filter.test.ts index 17d4d00bac8..83ff405d271 100644 --- a/apps/sim/lib/knowledge/documents/tag-filter.test.ts +++ b/apps/sim/lib/knowledge/documents/tag-filter.test.ts @@ -2,7 +2,9 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { getDocuments } from '@/lib/knowledge/documents/service' import { buildTagFilterCondition } from '@/lib/knowledge/documents/tag-filter' +import { validateTagValue } from '@/lib/knowledge/tags/utils' /** * The global `drizzle-orm` mock renders `sql` fragments to a `?`-placeholder @@ -174,4 +176,62 @@ describe('buildTagFilterCondition', () => { ).toBeUndefined() }) }) + + describe('agreement with the value the tag-value gate validated', () => { + it('compiles a date the gate trimmed rather than dropping the filter', () => { + expect(validateTagValue('due', ' 2026-04-21', 'date')).toBeNull() + const { sql, params } = rendered( + buildTagFilterCondition({ + tagSlot: 'date1', + fieldType: 'date', + operator: 'eq', + value: ' 2026-04-21', + }) + ) + expect(sql).toBe('?::date = ?::date') + expect(params).toEqual(['date1', '2026-04-21']) + }) + + it('compiles a trimmed between bound too', () => { + const condition = buildTagFilterCondition({ + tagSlot: 'date1', + fieldType: 'date', + operator: 'between', + value: '2026-04-01', + valueTo: ' 2026-04-30 ', + }) as unknown as { type: string; conditions: unknown[] } + expect(condition.type).toBe('and') + expect(rendered(condition.conditions[1] as never).params).toEqual(['date1', '2026-04-30']) + }) + + it('reads a boolean case-insensitively', () => { + expect(validateTagValue('flag', 'TRUE', 'boolean')).toBeNull() + expect( + buildTagFilterCondition({ + tagSlot: 'boolean1', + fieldType: 'boolean', + operator: 'eq', + value: 'TRUE', + }) + ).toEqual({ type: 'eq', left: 'boolean1', right: true }) + }) + }) +}) + +describe('getDocuments tag filters', () => { + it('raises rather than dropping a filter it cannot compile', async () => { + await expect( + getDocuments( + 'kb-1', + { + limit: 10, + offset: 0, + tagFilters: [ + { tagSlot: 'not_a_real_slot', fieldType: 'text', operator: 'eq', value: 'x' }, + ], + }, + 'req-1' + ) + ).rejects.toThrow(/Tag filter on slot "not_a_real_slot" could not be applied/) + }) }) diff --git a/apps/sim/lib/knowledge/documents/tag-filter.ts b/apps/sim/lib/knowledge/documents/tag-filter.ts index 740e97bf65d..96facf2c06f 100644 --- a/apps/sim/lib/knowledge/documents/tag-filter.ts +++ b/apps/sim/lib/knowledge/documents/tag-filter.ts @@ -1,6 +1,6 @@ import { document } from '@sim/db/schema' import { and, eq, gt, gte, lt, lte, ne, type SQL, sql } from 'drizzle-orm' -import { parseBooleanValue } from '@/lib/knowledge/tags/utils' +import { coerceTagFilterValue, escapeLikePattern } from '@/lib/knowledge/tags/utils' /** * A single tag filter applied to a document list query. @@ -33,27 +33,27 @@ const ALLOWED_TAG_SLOTS = new Set([ 'boolean3', ]) -const DATE_ONLY_PATTERN = /^\d{4}-\d{2}-\d{2}$/ - -function escapeLikePattern(s: string): string { - return s.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_') -} - /** * Builds a SQL predicate for a single tag filter against the document table. * * Text comparisons are case-insensitive and date comparisons are evaluated on * the calendar day, matching the semantics of the knowledge base search filter - * (`lib/knowledge/search/queries.ts`). Returns `undefined` when the slot, - * operator, or value is not usable so the caller can skip the condition. + * (`lib/knowledge/search/queries.ts`). The value is coerced by the same + * function the tag-value gate validates with, so a value that passed validation + * always compiles. Returns `undefined` only when the slot, operator, or value is + * genuinely unusable — which, after validation, is a bug the caller reports + * rather than a predicate it may skip. */ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undefined { if (!ALLOWED_TAG_SLOTS.has(filter.tagSlot)) return undefined const col = document[filter.tagSlot as keyof typeof document] + const coerced = coerceTagFilterValue(filter.value, filter.fieldType) + if (!coerced.ok) return undefined + if (filter.fieldType === 'text') { - const v = String(filter.value ?? '') + const v = coerced.value as string switch (filter.operator) { case 'eq': return sql`LOWER(${col}) = LOWER(${v})` @@ -81,8 +81,7 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef } if (filter.fieldType === 'number') { - const num = Number(filter.value) - if (Number.isNaN(num)) return undefined + const num = coerced.value as number switch (filter.operator) { case 'eq': return eq(col as typeof document.number1, num) @@ -97,8 +96,10 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef case 'lte': return lte(col as typeof document.number1, num) case 'between': { - const numTo = Number(filter.valueTo) - if (Number.isNaN(numTo)) return undefined + if (filter.valueTo === undefined || filter.valueTo === null) return undefined + const coercedTo = coerceTagFilterValue(filter.valueTo, 'number') + if (!coercedTo.ok) return undefined + const numTo = coercedTo.value as number return and( gte(col as typeof document.number1, num), lte(col as typeof document.number1, numTo) @@ -110,8 +111,7 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef } if (filter.fieldType === 'date') { - const v = String(filter.value ?? '') - if (!DATE_ONLY_PATTERN.test(v)) return undefined + const v = coerced.value as string switch (filter.operator) { case 'eq': return sql`${col}::date = ${v}::date` @@ -126,8 +126,9 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef case 'lte': return sql`${col}::date <= ${v}::date` case 'between': { - const valueTo = String(filter.valueTo ?? '') - if (!DATE_ONLY_PATTERN.test(valueTo)) return undefined + const coercedTo = coerceTagFilterValue(filter.valueTo, 'date') + if (!coercedTo.ok) return undefined + const valueTo = coercedTo.value as string return and(sql`${col}::date >= ${v}::date`, sql`${col}::date <= ${valueTo}::date`) } default: @@ -136,9 +137,7 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef } if (filter.fieldType === 'boolean') { - const boolVal = - typeof filter.value === 'boolean' ? filter.value : parseBooleanValue(String(filter.value)) - if (boolVal === null) return undefined + const boolVal = coerced.value as boolean switch (filter.operator) { case 'eq': return eq(col as typeof document.boolean1, boolVal) diff --git a/apps/sim/lib/knowledge/search/queries.test.ts b/apps/sim/lib/knowledge/search/queries.test.ts new file mode 100644 index 00000000000..6186d4f5a0c --- /dev/null +++ b/apps/sim/lib/knowledge/search/queries.test.ts @@ -0,0 +1,171 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { getStructuredTagFilters } from '@/lib/knowledge/search/queries' +import type { StructuredFilter } from '@/lib/knowledge/types' + +/** + * The builder only reads `embeddingTable[tagSlot]`, so a slot-to-name map stands + * in for the real table and makes each rendered parameter readable. + */ +const embeddingTable = { + tag1: 'tag1', + number1: 'number1', + date1: 'date1', + boolean1: 'boolean1', +} + +/** + * 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 renderOne(filters: StructuredFilter[]) { + const conditions = getStructuredTagFilters(filters, embeddingTable) + expect(conditions).toHaveLength(1) + return (conditions[0] as unknown as { toSQL: () => { sql: string; params: unknown[] } }).toSQL() +} + +describe('getStructuredTagFilters', () => { + describe('agreement with the value the gate validated', () => { + it('compiles a number the gate read as 0 rather than dropping the filter', () => { + const { sql, params } = renderOne([ + { tagSlot: 'number1', fieldType: 'number', operator: 'eq', value: '' }, + ]) + expect(sql).toBe('? = ?') + expect(params).toEqual(['number1', 0]) + }) + + it('reads a number in the same base the gate validated', () => { + const { params } = renderOne([ + { tagSlot: 'number1', fieldType: 'number', operator: 'eq', value: '0x10' }, + ]) + expect(params).toEqual(['number1', 16]) + }) + + it('reads a boolean case-insensitively instead of inverting it', () => { + const { params } = renderOne([ + { tagSlot: 'boolean1', fieldType: 'boolean', operator: 'eq', value: 'TRUE' }, + ]) + expect(params).toEqual(['boolean1', true]) + }) + + it('trims a date the gate trimmed rather than dropping the filter', () => { + const { sql, params } = renderOne([ + { tagSlot: 'date1', fieldType: 'date', operator: 'eq', value: ' 2026-08-13' }, + ]) + expect(sql).toBe('?::date = ?::date') + expect(params).toEqual(['date1', '2026-08-13']) + }) + + it('escapes LIKE metacharacters so a typed % is not a wildcard', () => { + const { sql, params } = renderOne([ + { tagSlot: 'tag1', fieldType: 'text', operator: 'contains', value: '50%off' }, + ]) + expect(sql).toBe("LOWER(?) LIKE LOWER(?) ESCAPE '\\'") + expect(params).toEqual(['tag1', '%50\\%off%']) + }) + + it('escapes LIKE metacharacters for every text operator that uses LIKE', () => { + for (const operator of ['not_contains', 'starts_with', 'ends_with']) { + const { sql, params } = renderOne([ + { tagSlot: 'tag1', fieldType: 'text', operator, value: 'a_b' }, + ]) + expect(sql).toContain("ESCAPE '\\'") + expect(params[1]).toContain('a\\_b') + } + }) + }) + + describe('a filter that cannot compile is reported, never skipped', () => { + it('raises instead of returning no predicate at all', () => { + expect(() => + getStructuredTagFilters( + [{ tagSlot: 'not_a_slot', fieldType: 'text', operator: 'eq', value: 'x' }], + embeddingTable + ) + ).toThrow(/Tag filter on slot "not_a_slot" could not be applied/) + }) + + it('raises rather than silently widening a multi-filter search', () => { + expect(() => + getStructuredTagFilters( + [ + { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'ok' }, + { tagSlot: 'not_a_slot', fieldType: 'text', operator: 'eq', value: 'x' }, + ], + embeddingTable + ) + ).toThrow(/could not be applied/) + }) + }) + + describe('a correct filter still compiles to the predicate it always did', () => { + it('text eq', () => { + const { sql, params } = renderOne([ + { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'Billing' }, + ]) + expect(sql).toBe('LOWER(?) = LOWER(?)') + expect(params).toEqual(['tag1', 'Billing']) + }) + + it('number gte', () => { + const { sql, params } = renderOne([ + { tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '42' }, + ]) + expect(sql).toBe('? >= ?') + expect(params).toEqual(['number1', 42]) + }) + + it('number between', () => { + const { sql, params } = renderOne([ + { + tagSlot: 'number1', + fieldType: 'number', + operator: 'between', + value: '1', + valueTo: '9', + }, + ]) + expect(sql).toBe('? >= ? AND ? <= ?') + expect(params).toEqual(['number1', 1, 'number1', 9]) + }) + + it('date between', () => { + const { sql, params } = renderOne([ + { + tagSlot: 'date1', + fieldType: 'date', + operator: 'between', + value: '2026-01-01', + valueTo: '2026-12-31', + }, + ]) + expect(sql).toBe('?::date >= ?::date AND ?::date <= ?::date') + expect(params).toEqual(['date1', '2026-01-01', 'date1', '2026-12-31']) + }) + + it('boolean neq', () => { + const { sql, params } = renderOne([ + { tagSlot: 'boolean1', fieldType: 'boolean', operator: 'neq', value: 'false' }, + ]) + expect(sql).toBe('? != ?') + expect(params).toEqual(['boolean1', false]) + }) + + it('ORs two filters on the same slot and keeps them one condition', () => { + const conditions = getStructuredTagFilters( + [ + { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'a' }, + { tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'b' }, + ], + 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']) + }) + }) +}) diff --git a/apps/sim/lib/knowledge/search/queries.ts b/apps/sim/lib/knowledge/search/queries.ts index 4a6c29a5a2d..1b380f1ac4e 100644 --- a/apps/sim/lib/knowledge/search/queries.ts +++ b/apps/sim/lib/knowledge/search/queries.ts @@ -3,6 +3,11 @@ import { document, embedding } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { and, eq, inArray, isNull, type SQL, sql } from 'drizzle-orm' +import { + coerceTagFilterValue, + escapeLikePattern, + uncompilableTagFilterError, +} from '@/lib/knowledge/tags/utils' import type { StructuredFilter } from '@/lib/knowledge/types' const logger = createLogger('KnowledgeSearchQueries') @@ -170,20 +175,23 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { // Handle text operators if (fieldType === 'text') { - const stringValue = String(value) + const coerced = coerceTagFilterValue(value, 'text') + if (!coerced.ok) return null + const stringValue = coerced.value as string + const escaped = escapeLikePattern(stringValue) switch (operator) { case 'eq': return sql`LOWER(${column}) = LOWER(${stringValue})` case 'neq': return sql`LOWER(${column}) != LOWER(${stringValue})` case 'contains': - return sql`LOWER(${column}) LIKE LOWER(${`%${stringValue}%`})` + return sql`LOWER(${column}) LIKE LOWER(${`%${escaped}%`}) ESCAPE '\\'` case 'not_contains': - return sql`LOWER(${column}) NOT LIKE LOWER(${`%${stringValue}%`})` + return sql`LOWER(${column}) NOT LIKE LOWER(${`%${escaped}%`}) ESCAPE '\\'` case 'starts_with': - return sql`LOWER(${column}) LIKE LOWER(${`${stringValue}%`})` + return sql`LOWER(${column}) LIKE LOWER(${`${escaped}%`}) ESCAPE '\\'` case 'ends_with': - return sql`LOWER(${column}) LIKE LOWER(${`%${stringValue}`})` + return sql`LOWER(${column}) LIKE LOWER(${`%${escaped}`}) ESCAPE '\\'` default: return sql`LOWER(${column}) = LOWER(${stringValue})` } @@ -191,8 +199,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { // Handle number operators if (fieldType === 'number') { - const numValue = typeof value === 'number' ? value : Number.parseFloat(String(value)) - if (Number.isNaN(numValue)) return null + const coerced = coerceTagFilterValue(value, 'number') + if (!coerced.ok) return null + const numValue = coerced.value as number switch (operator) { case 'eq': @@ -209,10 +218,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { return sql`${column} <= ${numValue}` case 'between': if (valueTo !== undefined) { - const numValueTo = - typeof valueTo === 'number' ? valueTo : Number.parseFloat(String(valueTo)) - if (Number.isNaN(numValueTo)) return sql`${column} = ${numValue}` - return sql`${column} >= ${numValue} AND ${column} <= ${numValueTo}` + const coercedTo = coerceTagFilterValue(valueTo, 'number') + if (!coercedTo.ok) return sql`${column} = ${numValue}` + return sql`${column} >= ${numValue} AND ${column} <= ${coercedTo.value as number}` } return sql`${column} = ${numValue}` default: @@ -222,11 +230,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { // Handle date operators - expects YYYY-MM-DD format from frontend if (fieldType === 'date') { - const dateStr = String(value) - // Validate YYYY-MM-DD format - if (!/^\d{4}-\d{2}-\d{2}$/.test(dateStr)) { - return null - } + const coerced = coerceTagFilterValue(value, 'date') + if (!coerced.ok) return null + const dateStr = coerced.value as string switch (operator) { case 'eq': @@ -243,10 +249,11 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { return sql`${column}::date <= ${dateStr}::date` case 'between': if (valueTo !== undefined) { - const dateStrTo = String(valueTo) - if (!/^\d{4}-\d{2}-\d{2}$/.test(dateStrTo)) { + const coercedTo = coerceTagFilterValue(valueTo, 'date') + if (!coercedTo.ok) { return sql`${column}::date = ${dateStr}::date` } + const dateStrTo = coercedTo.value as string return sql`${column}::date >= ${dateStr}::date AND ${column}::date <= ${dateStrTo}::date` } return sql`${column}::date = ${dateStr}::date` @@ -257,7 +264,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) { // Handle boolean operators if (fieldType === 'boolean') { - const boolValue = value === true || value === 'true' + const coerced = coerceTagFilterValue(value, 'boolean') + if (!coerced.ok) return null + const boolValue = coerced.value as boolean switch (operator) { case 'eq': return sql`${column} = ${boolValue}` @@ -276,8 +285,14 @@ 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 + * + * 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 + * tag term from the WHERE clause entirely and answered a filtered search with + * the whole knowledge base under a 200 — and search is billed, so the caller + * paid for the widened scan. It is reported as a validation failure instead. */ -function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: any) { +export function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: any) { // Group filters by tagSlot const filtersBySlot = new Map() for (const filter of filters) { @@ -291,12 +306,12 @@ function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: an // Build conditions: OR within same slot, AND across different slots const conditions: ReturnType[] = [] - for (const [slot, slotFilters] of filtersBySlot) { - const slotConditions = slotFilters - .map((f) => buildFilterCondition(f, embeddingTable)) - .filter((c): c is ReturnType => c !== null) - - if (slotConditions.length === 0) continue + 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 diff --git a/apps/sim/lib/knowledge/tags/utils.test.ts b/apps/sim/lib/knowledge/tags/utils.test.ts new file mode 100644 index 00000000000..d99b080eecc --- /dev/null +++ b/apps/sim/lib/knowledge/tags/utils.test.ts @@ -0,0 +1,70 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { coerceTagFilterValue, validateTagValue } from '@/lib/knowledge/tags/utils' + +describe('coerceTagFilterValue', () => { + it('accepts exactly what validateTagValue accepts', () => { + const cases: Array<[string, 'number' | 'date' | 'boolean']> = [ + ['', 'number'], + ['0x10', 'number'], + ['12.5', 'number'], + ['abc', 'number'], + ['TRUE', 'boolean'], + ['False', 'boolean'], + ['yes', 'boolean'], + [' 2026-08-13', 'date'], + ['2026-08-13', 'date'], + ['2026-02-31', 'date'], + ['13-08-2026', 'date'], + ] + for (const [value, fieldType] of cases) { + expect(coerceTagFilterValue(value, fieldType).ok, `${fieldType} "${value}"`).toBe( + validateTagValue('tag', value, fieldType) === null + ) + } + }) + + it('trims a date the same way the gate does', () => { + expect(coerceTagFilterValue(' 2026-08-13 ', 'date')).toEqual({ ok: true, value: '2026-08-13' }) + }) + + it('reads a boolean case-insensitively', () => { + expect(coerceTagFilterValue('TRUE', 'boolean')).toEqual({ ok: true, value: true }) + expect(coerceTagFilterValue('False', 'boolean')).toEqual({ ok: true, value: false }) + expect(coerceTagFilterValue(true, 'boolean')).toEqual({ ok: true, value: true }) + }) + + it('reads a number with the same base the gate validates with', () => { + expect(coerceTagFilterValue('0x10', 'number')).toEqual({ ok: true, value: 16 }) + expect(coerceTagFilterValue('', 'number')).toEqual({ ok: true, value: 0 }) + expect(coerceTagFilterValue(12.5, 'number')).toEqual({ ok: true, value: 12.5 }) + }) + + it('leaves a text value untouched so a search for padded text still matches', () => { + expect(coerceTagFilterValue(' padded ', 'text')).toEqual({ ok: true, value: ' padded ' }) + }) +}) + +describe('validateTagValue', () => { + it('keeps its distinct messages per failure', () => { + expect(validateTagValue('flag', 'yes', 'boolean')).toBe( + 'Tag "flag" expects a boolean value (true/false), but received "yes"' + ) + expect(validateTagValue('score', 'abc', 'number')).toBe( + 'Tag "score" expects a number value, but received "abc"' + ) + expect(validateTagValue('due', '13-08-2026', 'date')).toBe( + 'Tag "due" expects a date in YYYY-MM-DD format, but received "13-08-2026"' + ) + expect(validateTagValue('due', '2026-02-31', 'date')).toBe( + 'Tag "due" has an invalid date: "2026-02-31"' + ) + }) + + it('does not constrain text or unknown field types', () => { + expect(validateTagValue('name', 'anything', 'text')).toBeNull() + expect(validateTagValue('name', 'anything', 'json')).toBeNull() + }) +}) diff --git a/apps/sim/lib/knowledge/tags/utils.ts b/apps/sim/lib/knowledge/tags/utils.ts index 713a04cd411..58d000a3664 100644 --- a/apps/sim/lib/knowledge/tags/utils.ts +++ b/apps/sim/lib/knowledge/tags/utils.ts @@ -1,40 +1,114 @@ +import { OrchestrationError } from '@/lib/core/orchestration/types' + +const DATE_ONLY_PATTERN = /^\d{4}-\d{2}-\d{2}$/ + +/** Field types a tag filter value can be compiled for. */ +export type TagFilterFieldType = 'text' | 'number' | 'date' | 'boolean' + +export type TagFilterValueCoercion = + | { ok: true; value: string | number | boolean } + | { ok: false; reason: 'number' | 'date-format' | 'date-calendar' | 'boolean' } + /** - * Validate a tag value against its expected field type - * Returns an error message if invalid, or null if valid + * Coerces a raw tag filter value into the typed value its SQL predicate + * compares against. + * + * This is the only place a filter value is parsed. `validateTagValue` reports + * the failures it returns, and both filter builders — the document list + * (`lib/knowledge/documents/tag-filter.ts`) and search + * (`lib/knowledge/search/queries.ts`) — consume the value it produces. Sharing + * one parse is what makes "the gate accepted this value" and "the predicate + * compiled that same value" a single fact. Three independent parses used to + * disagree: a boolean `"TRUE"` passed the gate, matched `= true` on the + * document list and `= false` on search, and a date carrying a leading space + * passed the gate (which trimmed) but compiled to no predicate at all on the + * document list (which did not), returning the whole knowledge base under a + * 200. */ -export function validateTagValue(tagName: string, value: string, fieldType: string): string | null { - const stringValue = String(value).trim() - +export function coerceTagFilterValue( + value: unknown, + fieldType: TagFilterFieldType +): TagFilterValueCoercion { switch (fieldType) { - case 'boolean': { - const lowerValue = stringValue.toLowerCase() - if (lowerValue !== 'true' && lowerValue !== 'false') { - return `Tag "${tagName}" expects a boolean value (true/false), but received "${value}"` - } - return null - } + case 'text': + return { ok: true, value: String(value ?? '') } case 'number': { - const numValue = Number(stringValue) - if (Number.isNaN(numValue)) { - return `Tag "${tagName}" expects a number value, but received "${value}"` - } - return null + const numValue = typeof value === 'number' ? value : Number(String(value ?? '').trim()) + if (Number.isNaN(numValue)) return { ok: false, reason: 'number' } + return { ok: true, value: numValue } } case 'date': { - // Check format first - if (!/^\d{4}-\d{2}-\d{2}$/.test(stringValue)) { - return `Tag "${tagName}" expects a date in YYYY-MM-DD format, but received "${value}"` - } - // Validate the date is actually valid (e.g., reject 2024-02-31) + const stringValue = String(value ?? '').trim() + if (!DATE_ONLY_PATTERN.test(stringValue)) return { ok: false, reason: 'date-format' } const [year, month, day] = stringValue.split('-').map(Number) const date = new Date(year, month - 1, day) if (date.getFullYear() !== year || date.getMonth() !== month - 1 || date.getDate() !== day) { - return `Tag "${tagName}" has an invalid date: "${value}"` + return { ok: false, reason: 'date-calendar' } } - return null + return { ok: true, value: stringValue } + } + case 'boolean': { + if (typeof value === 'boolean') return { ok: true, value } + const lowerValue = String(value ?? '') + .trim() + .toLowerCase() + if (lowerValue === 'true') return { ok: true, value: true } + if (lowerValue === 'false') return { ok: true, value: false } + return { ok: false, reason: 'boolean' } } - default: - return null + } +} + +/** + * Escapes the LIKE metacharacters in a tag filter value so a `%` or `_` a + * caller typed matches itself instead of acting as a wildcard. Both filter + * builders pair this with `ESCAPE '\'`. + */ +export function escapeLikePattern(value: string): string { + return value.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_') +} + +/** + * The failure raised when a validated tag filter still fails to compile to a + * SQL predicate. + * + * Both filter builders return "no predicate" for a filter they cannot compile, + * and both consumers used to skip it. A skipped predicate does not narrow + * anything, so a filtered read answered 200 with every row the caller could + * see. Once validation has passed, that state is a defect in this code — not an + * input a surface may quietly ignore — so it is surfaced as a validation + * failure the caller can act on. + */ +export function uncompilableTagFilterError(filter: { + tagSlot: string + fieldType: string + operator: string +}): OrchestrationError { + return new OrchestrationError( + 'validation', + `Tag filter on slot "${filter.tagSlot}" could not be applied (field type "${filter.fieldType}", operator "${filter.operator}")` + ) +} + +/** + * Validate a tag value against its expected field type + * Returns an error message if invalid, or null if valid + */ +export function validateTagValue(tagName: string, value: string, fieldType: string): string | null { + if (fieldType !== 'boolean' && fieldType !== 'number' && fieldType !== 'date') return null + + const coerced = coerceTagFilterValue(value, fieldType) + if (coerced.ok) return null + + switch (coerced.reason) { + case 'boolean': + return `Tag "${tagName}" expects a boolean value (true/false), but received "${value}"` + case 'number': + return `Tag "${tagName}" expects a number value, but received "${value}"` + case 'date-format': + return `Tag "${tagName}" expects a date in YYYY-MM-DD format, but received "${value}"` + case 'date-calendar': + return `Tag "${tagName}" has an invalid date: "${value}"` } } diff --git a/apps/sim/lib/secrets/application/use-cases.test.ts b/apps/sim/lib/secrets/application/use-cases.test.ts index dd2abd92a3f..c7b708c06aa 100644 --- a/apps/sim/lib/secrets/application/use-cases.test.ts +++ b/apps/sim/lib/secrets/application/use-cases.test.ts @@ -3,7 +3,7 @@ */ import type { Principal } from '@sim/auth/principal' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { SetSecretInput } from '@/lib/secrets/application/use-cases' +import type { DeleteSecretInput, SetSecretInput } from '@/lib/secrets/application/use-cases' const { mocks } = vi.hoisted(() => ({ mocks: { @@ -11,7 +11,10 @@ const { mocks } = vi.hoisted(() => ({ resolvePermission: vi.fn(), workspaceAccess: vi.fn(), keyAccess: vi.fn(), + personalMetadata: vi.fn(), setWorkspace: vi.fn(), + setPersonal: vi.fn(), + deletePersonal: vi.fn(), listCredentials: vi.fn(), audit: vi.fn(), }, @@ -38,18 +41,19 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({ })) vi.mock('@/lib/credentials/environment', () => ({ getWorkspaceEnvKeyAdminAccess: mocks.keyAccess, + getPersonalEnvCredentialMetadata: mocks.personalMetadata, })) vi.mock('@/lib/credentials/queries', () => ({ listVisibleWorkspaceCredentials: mocks.listCredentials, })) vi.mock('@/lib/credentials/secret-values', () => ({ - deletePersonalSecret: vi.fn(), + deletePersonalSecret: mocks.deletePersonal, deleteWorkspaceSecret: vi.fn(), - setPersonalSecret: vi.fn(), + setPersonalSecret: mocks.setPersonal, setWorkspaceSecret: mocks.setWorkspace, })) -import { setSecretUseCase } from '@/lib/secrets/application/use-cases' +import { deleteSecretUseCase, setSecretUseCase } from '@/lib/secrets/application/use-cases' const workspace = { workspaceId: 'workspace-1', @@ -74,6 +78,18 @@ const secret = { role: 'admin' as const, } +const personalUpdatedAt = new Date('2026-02-01T00:00:00Z') +const personalSecret = { + ...secret, + id: 'secret-2', + type: 'env_personal' as const, + displayName: 'OPENAI_API_KEY', + envKey: 'OPENAI_API_KEY', + envOwnerUserId: 'user-1', +} + +const session = { kind: 'session', userId: 'user-1', sessionId: 'session-1' } as const + describe('secret application use cases', () => { beforeEach(() => { vi.clearAllMocks() @@ -81,7 +97,10 @@ describe('secret application use cases', () => { mocks.resolvePermission.mockResolvedValue('write') mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false }) mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() }) - mocks.setWorkspace.mockResolvedValue({ created: true }) + mocks.setWorkspace.mockResolvedValue({ created: true, updatedAt: personalUpdatedAt }) + mocks.setPersonal.mockResolvedValue({ created: true, updatedAt: personalUpdatedAt }) + mocks.personalMetadata.mockResolvedValue(null) + mocks.deletePersonal.mockResolvedValue(true) mocks.listCredentials.mockResolvedValue({ data: [secret], nextCursorKeys: null }) }) @@ -143,4 +162,122 @@ describe('secret application use cases', () => { ) expect(JSON.stringify(mocks.audit.mock.calls)).not.toContain('secret-value') }) + + it('still fails a workspace write whose metadata never materialized', async () => { + mocks.listCredentials.mockResolvedValue({ data: [], nextCursorKeys: null }) + + await expect( + setSecretUseCase.execute({ + principal: session, + input: { + workspaceId: workspace.workspaceId, + name: secret.envKey, + scope: 'workspace', + value: 'secret-value', + }, + }) + ).rejects.toThrow(/workspace:STRIPE_API_KEY/) + }) + + it('reports a committed personal write when this workspace holds no mirror', async () => { + mocks.resolvePermission.mockResolvedValue('admin') + mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: true }) + mocks.listCredentials.mockResolvedValue({ data: [], nextCursorKeys: null }) + + const result = await setSecretUseCase.execute({ + principal: session, + input: { + workspaceId: workspace.workspaceId, + name: personalSecret.envKey, + scope: 'personal', + value: 'secret-value', + }, + }) + + expect(mocks.setPersonal).toHaveBeenCalledWith({ + userId: 'user-1', + name: personalSecret.envKey, + value: 'secret-value', + }) + expect(result.created).toBe(true) + expect(result.secret).toMatchObject({ + type: 'env_personal', + envKey: personalSecret.envKey, + envOwnerUserId: 'user-1', + role: 'admin', + createdAt: personalUpdatedAt, + updatedAt: personalUpdatedAt, + }) + }) + + it('dates a mirrorless personal write from the secret the caller already owns', async () => { + const createdAt = new Date('2025-06-01T00:00:00Z') + mocks.listCredentials.mockResolvedValue({ data: [], nextCursorKeys: null }) + mocks.personalMetadata.mockResolvedValue({ + id: 'secret-2', + createdAt, + updatedAt: createdAt, + }) + mocks.setPersonal.mockResolvedValue({ created: false, updatedAt: personalUpdatedAt }) + + const result = await setSecretUseCase.execute({ + principal: session, + input: { + workspaceId: workspace.workspaceId, + name: personalSecret.envKey, + scope: 'personal', + value: 'secret-value', + }, + }) + + expect(mocks.personalMetadata).toHaveBeenCalledWith({ + userId: 'user-1', + envKey: personalSecret.envKey, + }) + expect(result.created).toBe(false) + expect(result.secret).toMatchObject({ + id: 'secret-2', + createdAt, + updatedAt: personalUpdatedAt, + }) + }) + + it('prefers this workspace mirror for a personal write when one exists', async () => { + mocks.listCredentials.mockResolvedValue({ data: [personalSecret], nextCursorKeys: null }) + + const result = await setSecretUseCase.execute({ + principal: session, + input: { + workspaceId: workspace.workspaceId, + name: personalSecret.envKey, + scope: 'personal', + value: 'secret-value', + }, + }) + + expect(result.secret).toBe(personalSecret) + expect(mocks.personalMetadata).not.toHaveBeenCalled() + }) + + it('deletes a personal secret for the caller rather than for one workspace', async () => { + const execute = deleteSecretUseCase.execute as (args: { + principal: Principal + input: DeleteSecretInput + }) => Promise<{ name: string; scope: string }> + + const result = await execute({ + principal: session, + input: { + workspaceId: workspace.workspaceId, + name: personalSecret.envKey, + scope: 'personal', + }, + }) + + expect(mocks.deletePersonal).toHaveBeenCalledWith({ + userId: 'user-1', + name: personalSecret.envKey, + }) + expect(result).toEqual({ name: personalSecret.envKey, scope: 'personal' }) + }) }) diff --git a/apps/sim/lib/secrets/application/use-cases.ts b/apps/sim/lib/secrets/application/use-cases.ts index 5d8856629e7..1cd0daadd80 100644 --- a/apps/sim/lib/secrets/application/use-cases.ts +++ b/apps/sim/lib/secrets/application/use-cases.ts @@ -4,7 +4,10 @@ import type { CursorKey, ListSortOrder } from '@/lib/api/list-query' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { ForbiddenOperationError } from '@/lib/core/application/forbidden' import { OrchestrationError } from '@/lib/core/orchestration/types' -import { getWorkspaceEnvKeyAdminAccess } from '@/lib/credentials/environment' +import { + getPersonalEnvCredentialMetadata, + getWorkspaceEnvKeyAdminAccess, +} from '@/lib/credentials/environment' import { listVisibleWorkspaceCredentials, type VisibleWorkspaceCredential, @@ -115,29 +118,90 @@ async function requireWorkspaceSecretMutationAccess(params: { } } -async function getSecretMetadata(params: { +async function findSecretMetadata(params: { workspaceId: string userId: string scope: SecretScope name: string -}): Promise { +}): Promise { const { data } = await listSecretMetadata({ ...params, search: params.name, sortBy: 'name', sortOrder: 'asc', }) - const row = data.find( - (candidate) => - candidate.envKey === params.name && - (params.scope === 'workspace' - ? candidate.type === 'env_workspace' - : candidate.type === 'env_personal' && candidate.envOwnerUserId === params.userId) + return ( + data.find( + (candidate) => + candidate.envKey === params.name && + (params.scope === 'workspace' + ? candidate.type === 'env_workspace' + : candidate.type === 'env_personal' && candidate.envOwnerUserId === params.userId) + ) ?? null ) - if (!row) throw new Error(`Secret metadata was not created for ${params.scope}:${params.name}`) +} + +async function getWorkspaceSecretMetadata(params: { + workspaceId: string + userId: string + name: string +}): Promise { + const row = await findSecretMetadata({ ...params, scope: 'workspace' }) + if (!row) throw new Error(`Secret metadata was not created for workspace:${params.name}`) return row } +/** + * Resolves the metadata a personal-scope write reports back. + * + * A personal secret is stored once per user, and its `env_personal` credential + * rows are per-workspace mirrors written only for the workspaces the caller holds + * an explicit grant on. A caller whose access to this workspace is inherited — an + * organization admin with no `permissions` row — authorizes fine and commits the + * value, but has no mirror here; deciding the response from that mirror would + * report a committed write as a failure. The workspace's own mirror still answers + * when it exists because it carries the real per-workspace metadata; otherwise the + * secret's earliest mirror does, and a secret with no mirror anywhere was created + * by this very write. + */ +async function getPersonalSecretMetadata(params: { + workspaceId: string + userId: string + name: string + updatedAt: Date +}): Promise { + const mirror = await findSecretMetadata({ + workspaceId: params.workspaceId, + userId: params.userId, + scope: 'personal', + name: params.name, + }) + if (mirror) return mirror + + const stored = await getPersonalEnvCredentialMetadata({ + userId: params.userId, + envKey: params.name, + }) + + return { + /** No credential row backs this projection; the id is never presented. */ + id: stored?.id ?? `env_personal:${params.userId}:${params.name}`, + workspaceId: params.workspaceId, + type: 'env_personal', + displayName: params.name, + description: null, + providerId: null, + accountId: null, + envKey: params.name, + envOwnerUserId: params.userId, + createdBy: params.userId, + createdAt: stored?.createdAt ?? params.updatedAt, + updatedAt: params.updatedAt, + hasServiceAccountKey: false, + role: 'admin', + } +} + const authorizationOptions = {} export interface ListSecretsInput { @@ -203,20 +267,27 @@ export const setSecretUseCase = defineAuthorizedWorkspaceUseCase({ }) } - const mutation = - input.scope === 'workspace' - ? await setWorkspaceSecret({ - workspaceId: context.workspaceId, - name: input.name, - value: input.value, - userId, - }) - : await setPersonalSecret({ userId, name: input.name, value: input.value }) - const secret = await getSecretMetadata({ + if (input.scope === 'workspace') { + const mutation = await setWorkspaceSecret({ + workspaceId: context.workspaceId, + name: input.name, + value: input.value, + userId, + }) + const secret = await getWorkspaceSecretMetadata({ + workspaceId: context.workspaceId, + userId, + name: input.name, + }) + return { secret, userId, created: mutation.created } + } + + const mutation = await setPersonalSecret({ userId, name: input.name, value: input.value }) + const secret = await getPersonalSecretMetadata({ workspaceId: context.workspaceId, userId, - scope: input.scope, name: input.name, + updatedAt: mutation.updatedAt, }) return { secret, userId, created: mutation.created } }, diff --git a/apps/sim/lib/table/columns/service.ts b/apps/sim/lib/table/columns/service.ts index ea010ad7b4c..b68ce8393ac 100644 --- a/apps/sim/lib/table/columns/service.ts +++ b/apps/sim/lib/table/columns/service.ts @@ -39,6 +39,7 @@ import { assertColumnDestructive, assertSchemaMutable } from '@/lib/table/mutati import type { DbTransaction } from '@/lib/table/planner' import { stripGroupExecutions } from '@/lib/table/rows/executions' import { updateTableRowsWithDerivedSecretProvenance } from '@/lib/table/rows/secret-provenance' +import { assertValidSchema } from '@/lib/table/schema-invariants' import { selectValueToNames } from '@/lib/table/select-values' import { withLockedTable } from '@/lib/table/service' import { scaledStatementTimeoutMs, setTableTxTimeouts } from '@/lib/table/tx' @@ -57,7 +58,6 @@ import type { UpdateColumnTypeData, } from '@/lib/table/types' import { validateColumnDefinition } from '@/lib/table/validation' -import { assertValidSchema } from '@/lib/table/workflow-columns' import { stripGroupDeps } from '@/lib/table/workflow-group-deps' const logger = createLogger('TableColumnService') diff --git a/apps/sim/lib/table/schema-invariants.ts b/apps/sim/lib/table/schema-invariants.ts new file mode 100644 index 00000000000..132b343dddc --- /dev/null +++ b/apps/sim/lib/table/schema-invariants.ts @@ -0,0 +1,210 @@ +/** + * Schema invariants for workflow-group columns, kept in a leaf module. + * + * Pure functions over a `TableSchema`. They lived in `workflow-columns.ts`, + * which transitively reaches the executable tool registry, so importing them + * from `service.ts` to validate a create pulled ~5,200 modules into every page + * graph that renders a table. Splitting them out keeps one source of truth for + * what a valid schema is — reachable from the create path and from every later + * mutation — without the weight. + */ + +import { OrchestrationError } from '@/lib/core/orchestration/types' +import { getColumnId } from '@/lib/table/column-keys' +import type { TableSchema, WorkflowGroup } from '@/lib/table/types' + +/** + * Validates schema-level invariants. Run on every `addTableColumn`, + * `addWorkflowGroup`, `updateWorkflowGroup`, `renameColumn`, `reorderColumns`, + * etc. Returns a list of human-readable errors (empty if valid). + */ +export function validateSchema(schema: TableSchema, columnOrder: string[] | undefined): string[] { + const errors: string[] = [] + // Group refs and columnOrder hold stable column ids (not display names). + const columnsById = new Map(schema.columns.map((c) => [getColumnId(c), c])) + const groups = schema.workflowGroups ?? [] + const groupsById = new Map(groups.map((g) => [g.id, g])) + + // Reference integrity for group outputs. + const claimedColumns = new Map() // columnId → groupId + for (const group of groups) { + if (group.outputs.length === 0) { + errors.push(`Workflow group "${group.name ?? group.id}" has no outputs.`) + } + for (const out of group.outputs) { + const col = columnsById.get(out.columnName) + if (!col) { + errors.push( + `Workflow group "${group.name ?? group.id}" references missing column "${out.columnName}".` + ) + continue + } + if (col.workflowGroupId !== group.id) { + errors.push( + `Column "${col.name}" is referenced by group "${group.id}" but its workflowGroupId is "${col.workflowGroupId ?? '(unset)'}".` + ) + } + const claimer = claimedColumns.get(out.columnName) + if (claimer && claimer !== group.id) { + errors.push( + `Column "${out.columnName}" is claimed by both groups "${claimer}" and "${group.id}".` + ) + } else { + claimedColumns.set(out.columnName, group.id) + } + } + } + + // Every column flagged with a workflowGroupId must appear in exactly one group's outputs. + for (const col of schema.columns) { + if (!col.workflowGroupId) continue + if (!groupsById.has(col.workflowGroupId)) { + errors.push( + `Column "${col.name}" references missing workflow group "${col.workflowGroupId}".` + ) + continue + } + if (claimedColumns.get(getColumnId(col)) !== col.workflowGroupId) { + errors.push( + `Column "${col.name}" has workflowGroupId "${col.workflowGroupId}" but isn't in that group's outputs.` + ) + } + if (col.required) { + errors.push(`Workflow-output column "${col.name}" cannot be required.`) + } + if (col.unique) { + errors.push(`Workflow-output column "${col.name}" cannot be unique.`) + } + } + + // Dependency integrity. Deps are columns only — workflow output columns are + // valid deps too (the upstream group fills them, downstream becomes eligible + // when filled). A group can't depend on its own outputs. + for (const group of groups) { + const ownOutputs = new Set(group.outputs.map((o) => o.columnName)) + for (const depCol of group.dependencies?.columns ?? []) { + const col = columnsById.get(depCol) + if (!col) { + errors.push(`Group "${group.name ?? group.id}" depends on missing column "${depCol}".`) + continue + } + if (ownOutputs.has(depCol)) { + errors.push( + `Group "${group.name ?? group.id}" depends on its own output column "${depCol}".` + ) + } + } + } + + // Cycle detection on the column-induced group graph. An edge A → B exists + // when B depends on a column that A produces. + const cycle = findGroupCycle(groups) + if (cycle) { + errors.push( + `Workflow groups form a dependency cycle: ${cycle.map((id) => groupsById.get(id)?.name ?? id).join(' → ')}.` + ) + } + + // Layout: every group's outputs must be contiguous in columnOrder (when set). + if (columnOrder && columnOrder.length > 0) { + for (const split of findSplitGroups(columnOrder, groups)) { + errors.push( + `Workflow group "${split.groupName}" output columns must be contiguous; got order [${split.actual.join(', ')}].` + ) + } + } + + return errors +} + +/** + * Returns the cycle as an ordered list of group ids, or null if acyclic. Edges + * are induced by columns: an edge A → B exists iff B depends on a column that + * A produces. + */ +function findGroupCycle(groups: WorkflowGroup[]): string[] | null { + // Map each output column → the group that produces it. + const producerByColumn = new Map() + for (const g of groups) { + for (const o of g.outputs) producerByColumn.set(o.columnName, g.id) + } + const adjacency = new Map() + for (const g of groups) { + const upstream = new Set() + for (const depCol of g.dependencies?.columns ?? []) { + const producer = producerByColumn.get(depCol) + if (producer && producer !== g.id) upstream.add(producer) + } + adjacency.set(g.id, [...upstream]) + } + const VISITING = 1 + const VISITED = 2 + const state = new Map() + const stack: string[] = [] + + const dfs = (id: string): string[] | null => { + if (state.get(id) === VISITED) return null + if (state.get(id) === VISITING) { + const cycleStart = stack.indexOf(id) + return cycleStart >= 0 ? [...stack.slice(cycleStart), id] : [id] + } + state.set(id, VISITING) + stack.push(id) + for (const next of adjacency.get(id) ?? []) { + const found = dfs(next) + if (found) return found + } + stack.pop() + state.set(id, VISITED) + return null + } + + for (const g of groups) { + const cycle = dfs(g.id) + if (cycle) return cycle + } + return null +} + +interface SplitGroupReport { + groupId: string + groupName: string + actual: number[] +} + +/** + * Returns groups whose output columns occupy non-contiguous positions in the + * given columnOrder. Empty array means all groups are cohesive. + */ +export function findSplitGroups( + columnOrder: string[], + groups: WorkflowGroup[] +): SplitGroupReport[] { + const positions = new Map() + columnOrder.forEach((name, idx) => positions.set(name, idx)) + const reports: SplitGroupReport[] = [] + for (const group of groups) { + const indices = group.outputs + .map((o) => positions.get(o.columnName)) + .filter((i): i is number => i !== undefined) + .sort((a, b) => a - b) + if (indices.length < 2) continue + const min = indices[0] + const max = indices[indices.length - 1] + if (max - min + 1 !== indices.length) { + reports.push({ + groupId: group.id, + groupName: group.name ?? group.id, + actual: indices, + }) + } + } + return reports +} + +export function assertValidSchema(schema: TableSchema, columnOrder: string[] | undefined): void { + const errs = validateSchema(schema, columnOrder) + if (errs.length > 0) { + throw new OrchestrationError('validation', `Schema validation failed: ${errs.join('; ')}`) + } +} diff --git a/apps/sim/lib/table/service.test.ts b/apps/sim/lib/table/service.test.ts new file mode 100644 index 00000000000..5774caea7ab --- /dev/null +++ b/apps/sim/lib/table/service.test.ts @@ -0,0 +1,98 @@ +/** + * @vitest-environment node + */ +import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TableSchema } from '@/lib/table/types' + +vi.mock('@/lib/realtime/notify', () => ({ + notifyWorkspaceTablesChanged: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock('@/lib/table/billing', () => ({ + assertRowCapacity: vi.fn().mockResolvedValue(undefined), + notifyTableRowUsage: vi.fn(), +})) + +import { createTable } from '@/lib/table/service' + +const WORKSPACE_ID = '6fc7631d-88cd-46f8-9f0a-d4764daef7f8' + +/** A column produced by a workflow group, and the group that declares it. */ +function groupedSchema(overrides: { columnGroupId: string; groupId: string }): TableSchema { + return { + columns: [ + { id: 'col_email', name: 'email', type: 'string' }, + { + id: 'col_summary', + name: 'summary', + type: 'string', + workflowGroupId: overrides.columnGroupId, + }, + ], + workflowGroups: [ + { + id: overrides.groupId, + workflowId: 'workflow-1', + outputs: [{ blockId: 'block-1', path: 'out', columnName: 'col_summary' }], + }, + ], + } as TableSchema +} + +function create(schema: TableSchema) { + return createTable( + { name: 'contacts', schema, workspaceId: WORKSPACE_ID, userId: 'user-1' }, + 'request-1' + ) +} + +describe('createTable schema invariants', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + /** + * `POST /api/table` and `POST /api/v1/tables` both forward caller-supplied + * columns into this function, and their bodies carry no `workflowGroups`, so + * any group id they carry names a group that cannot exist. Stored, it fails + * every later add-column and add-group with a 400 that nothing can clear. + */ + it('rejects a column naming a workflow group the schema does not declare', async () => { + await expect( + create({ + columns: [ + { id: 'col_email', name: 'email', type: 'string', workflowGroupId: 'wfg_missing' }, + ], + } as TableSchema) + ).rejects.toMatchObject({ + code: 'validation', + message: expect.stringContaining('references missing workflow group "wfg_missing"'), + }) + + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + }) + + it('still creates a table whose columns name a group the same schema declares', async () => { + queueTableRows(schemaMock.userTableDefinitions, [{ count: 0 }]) + + const table = await create(groupedSchema({ columnGroupId: 'group-1', groupId: 'group-1' })) + + expect(table.schema.columns.map((column) => column.workflowGroupId)).toEqual([ + undefined, + 'group-1', + ]) + expect(dbChainMockFns.insert).toHaveBeenCalled() + }) + + it('creates an ordinary group-free table unchanged', async () => { + queueTableRows(schemaMock.userTableDefinitions, [{ count: 0 }]) + + const table = await create({ columns: [{ name: 'email', type: 'string' }] } as TableSchema) + + expect(table.name).toBe('contacts') + expect(table.schema.columns[0].id).toEqual(expect.any(String)) + expect(dbChainMockFns.insert).toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/table/service.ts b/apps/sim/lib/table/service.ts index 49fb82c4ef5..842a0a84d75 100644 --- a/apps/sim/lib/table/service.ts +++ b/apps/sim/lib/table/service.ts @@ -46,6 +46,7 @@ import { createExactEmptyTableRowSecretProvenance, mutateTableRowsWithSecretProvenance, } from '@/lib/table/rows/secret-provenance' +import { assertValidSchema } from '@/lib/table/schema-invariants' import { setTableTxTimeouts } from '@/lib/table/tx' import { type CreateTableData, @@ -548,6 +549,16 @@ export async function createTable( // Stamp stable ids so the table is id-keyed from its first row write. const schema = withGeneratedColumnIds(data.schema) + // The same invariants every later schema mutation enforces, run over what is + // about to be persisted. `validateTableSchema` above only checks columns in + // isolation, so a create could store a column naming a workflow group the + // schema does not declare — which no update path can clear, and which then + // fails every subsequent add-column and add-group with a 400. Imported lazily + // because `workflow-columns` transitively reaches the executable tool + // registry, which a static edge would pull into every page graph that renders + // a table. + assertValidSchema(schema, undefined) + // Row limits are enforced per-write against the current plan (see assertRowCapacity); the stored // column is vestigial, so it just takes the caller's value (if any) or the default. const maxRows = data.maxRows ?? TABLE_LIMITS.MAX_ROWS_PER_TABLE diff --git a/apps/sim/lib/table/workflow-columns.ts b/apps/sim/lib/table/workflow-columns.ts index 34211a12266..76d9bbafa50 100644 --- a/apps/sim/lib/table/workflow-columns.ts +++ b/apps/sim/lib/table/workflow-columns.ts @@ -38,7 +38,6 @@ import type { RowExecutions, TableDefinition, TableRow, - TableSchema, WorkflowGroup, } from '@/lib/table/types' @@ -51,7 +50,6 @@ const TABLE_TRIGGER_CANCELLATION_MAX_RUNS = 5_000 const TABLE_TRIGGER_CANCELLATION_RETENTION_MS = 14 * 24 * 60 * 60_000 const TABLE_ROW_EXECUTIONS_ROW_FK = 'table_row_executions_row_id_user_table_rows_id_fk' -import { getColumnId } from '@/lib/table/column-keys' import { USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants' import { areGroupDepsSatisfied, areOutputsFilled, isExecInFlight } from '@/lib/table/deps' import { resolveTableDispatchConcurrency } from '@/lib/table/dispatch-concurrency' @@ -1072,165 +1070,6 @@ export async function runWorkflowColumn(opts: { return { dispatchId, shouldSignalRowsChanged: true } } -/** - * Validates schema-level invariants. Run on every `addTableColumn`, - * `addWorkflowGroup`, `updateWorkflowGroup`, `renameColumn`, `reorderColumns`, - * etc. Returns a list of human-readable errors (empty if valid). - */ -export function validateSchema(schema: TableSchema, columnOrder: string[] | undefined): string[] { - const errors: string[] = [] - // Group refs and columnOrder hold stable column ids (not display names). - const columnsById = new Map(schema.columns.map((c) => [getColumnId(c), c])) - const groups = schema.workflowGroups ?? [] - const groupsById = new Map(groups.map((g) => [g.id, g])) - - // Reference integrity for group outputs. - const claimedColumns = new Map() // columnId → groupId - for (const group of groups) { - if (group.outputs.length === 0) { - errors.push(`Workflow group "${group.name ?? group.id}" has no outputs.`) - } - for (const out of group.outputs) { - const col = columnsById.get(out.columnName) - if (!col) { - errors.push( - `Workflow group "${group.name ?? group.id}" references missing column "${out.columnName}".` - ) - continue - } - if (col.workflowGroupId !== group.id) { - errors.push( - `Column "${col.name}" is referenced by group "${group.id}" but its workflowGroupId is "${col.workflowGroupId ?? '(unset)'}".` - ) - } - const claimer = claimedColumns.get(out.columnName) - if (claimer && claimer !== group.id) { - errors.push( - `Column "${out.columnName}" is claimed by both groups "${claimer}" and "${group.id}".` - ) - } else { - claimedColumns.set(out.columnName, group.id) - } - } - } - - // Every column flagged with a workflowGroupId must appear in exactly one group's outputs. - for (const col of schema.columns) { - if (!col.workflowGroupId) continue - if (!groupsById.has(col.workflowGroupId)) { - errors.push( - `Column "${col.name}" references missing workflow group "${col.workflowGroupId}".` - ) - continue - } - if (claimedColumns.get(getColumnId(col)) !== col.workflowGroupId) { - errors.push( - `Column "${col.name}" has workflowGroupId "${col.workflowGroupId}" but isn't in that group's outputs.` - ) - } - if (col.required) { - errors.push(`Workflow-output column "${col.name}" cannot be required.`) - } - if (col.unique) { - errors.push(`Workflow-output column "${col.name}" cannot be unique.`) - } - } - - // Dependency integrity. Deps are columns only — workflow output columns are - // valid deps too (the upstream group fills them, downstream becomes eligible - // when filled). A group can't depend on its own outputs. - for (const group of groups) { - const ownOutputs = new Set(group.outputs.map((o) => o.columnName)) - for (const depCol of group.dependencies?.columns ?? []) { - const col = columnsById.get(depCol) - if (!col) { - errors.push(`Group "${group.name ?? group.id}" depends on missing column "${depCol}".`) - continue - } - if (ownOutputs.has(depCol)) { - errors.push( - `Group "${group.name ?? group.id}" depends on its own output column "${depCol}".` - ) - } - } - } - - // Cycle detection on the column-induced group graph. An edge A → B exists - // when B depends on a column that A produces. - const cycle = findGroupCycle(groups) - if (cycle) { - errors.push( - `Workflow groups form a dependency cycle: ${cycle.map((id) => groupsById.get(id)?.name ?? id).join(' → ')}.` - ) - } - - // Layout: every group's outputs must be contiguous in columnOrder (when set). - if (columnOrder && columnOrder.length > 0) { - for (const split of findSplitGroups(columnOrder, groups)) { - errors.push( - `Workflow group "${split.groupName}" output columns must be contiguous; got order [${split.actual.join(', ')}].` - ) - } - } - - return errors -} - -/** - * Returns the cycle as an ordered list of group ids, or null if acyclic. Edges - * are induced by columns: an edge A → B exists iff B depends on a column that - * A produces. - */ -function findGroupCycle(groups: WorkflowGroup[]): string[] | null { - // Map each output column → the group that produces it. - const producerByColumn = new Map() - for (const g of groups) { - for (const o of g.outputs) producerByColumn.set(o.columnName, g.id) - } - const adjacency = new Map() - for (const g of groups) { - const upstream = new Set() - for (const depCol of g.dependencies?.columns ?? []) { - const producer = producerByColumn.get(depCol) - if (producer && producer !== g.id) upstream.add(producer) - } - adjacency.set(g.id, [...upstream]) - } - const VISITING = 1 - const VISITED = 2 - const state = new Map() - const stack: string[] = [] - - const dfs = (id: string): string[] | null => { - if (state.get(id) === VISITED) return null - if (state.get(id) === VISITING) { - const cycleStart = stack.indexOf(id) - return cycleStart >= 0 ? [...stack.slice(cycleStart), id] : [id] - } - state.set(id, VISITING) - stack.push(id) - for (const next of adjacency.get(id) ?? []) { - const found = dfs(next) - if (found) return found - } - stack.pop() - state.set(id, VISITED) - return null - } - - for (const g of groups) { - const cycle = dfs(g.id) - if (cycle) return cycle - } - return null -} - -interface SplitGroupReport { - groupId: string - groupName: string - actual: number[] -} - /** * Cell context stored on `paused_executions.metadata` so the resume worker * can route post-resume block outputs back to the same `(tableId, rowId, @@ -1300,40 +1139,4 @@ export async function findCellContextByExecutionId( } } -/** - * Returns groups whose output columns occupy non-contiguous positions in the - * given columnOrder. Empty array means all groups are cohesive. - */ -export function findSplitGroups( - columnOrder: string[], - groups: WorkflowGroup[] -): SplitGroupReport[] { - const positions = new Map() - columnOrder.forEach((name, idx) => positions.set(name, idx)) - const reports: SplitGroupReport[] = [] - for (const group of groups) { - const indices = group.outputs - .map((o) => positions.get(o.columnName)) - .filter((i): i is number => i !== undefined) - .sort((a, b) => a - b) - if (indices.length < 2) continue - const min = indices[0] - const max = indices[indices.length - 1] - if (max - min + 1 !== indices.length) { - reports.push({ - groupId: group.id, - groupName: group.name ?? group.id, - actual: indices, - }) - } - } - return reports -} - /** Throws if the schema has any invariant violations. Convenience for callers. */ -export function assertValidSchema(schema: TableSchema, columnOrder: string[] | undefined): void { - const errs = validateSchema(schema, columnOrder) - if (errs.length > 0) { - throw new OrchestrationError('validation', `Schema validation failed: ${errs.join('; ')}`) - } -} diff --git a/apps/sim/lib/table/workflow-groups/service.test.ts b/apps/sim/lib/table/workflow-groups/service.test.ts index dd1f73cc2bf..5d8f164409f 100644 --- a/apps/sim/lib/table/workflow-groups/service.test.ts +++ b/apps/sim/lib/table/workflow-groups/service.test.ts @@ -21,10 +21,17 @@ vi.mock('@/lib/table/rows/secret-provenance', () => ({ updateTableRowsWithDerivedSecretProvenance: vi.fn(), })) vi.mock('@/lib/table/workflow-columns', () => ({ - assertValidSchema: vi.fn(), runWorkflowColumn: vi.fn().mockResolvedValue(undefined), stripGroupDeps: (schema: unknown) => schema, })) +/** + * These ceiling fixtures declare groups whose output columns are not in the + * schema, so the invariant check has to stay stubbed for them to exercise the + * count limit. It moved to its own leaf module, so the stub follows it. + */ +vi.mock('@/lib/table/schema-invariants', () => ({ + assertValidSchema: vi.fn(), +})) import { TABLE_LIMITS } from '@/lib/table/constants' import { addWorkflowGroup } from '@/lib/table/workflow-groups/service' diff --git a/apps/sim/lib/table/workflow-groups/service.ts b/apps/sim/lib/table/workflow-groups/service.ts index 638a3719163..7f526593b5a 100644 --- a/apps/sim/lib/table/workflow-groups/service.ts +++ b/apps/sim/lib/table/workflow-groups/service.ts @@ -24,6 +24,7 @@ import { NAME_PATTERN, TABLE_LIMITS } from '@/lib/table/constants' import { assertColumnDestructive, assertSchemaMutable } from '@/lib/table/mutation-locks' import { stripGroupExecutions } from '@/lib/table/rows/executions' import { updateTableRowsWithDerivedSecretProvenance } from '@/lib/table/rows/secret-provenance' +import { assertValidSchema } from '@/lib/table/schema-invariants' import { getTableById, withLockedTable } from '@/lib/table/service' import { setTableTxTimeouts } from '@/lib/table/tx' import type { @@ -37,7 +38,7 @@ import type { WorkflowGroup, WorkflowGroupOutput, } from '@/lib/table/types' -import { assertValidSchema, runWorkflowColumn } from '@/lib/table/workflow-columns' +import { runWorkflowColumn } from '@/lib/table/workflow-columns' import { stripGroupDeps } from '@/lib/table/workflow-group-deps' const logger = createLogger('TableWorkflowGroupsService') diff --git a/apps/sim/lib/uploads/upload-session/service.test.ts b/apps/sim/lib/uploads/upload-session/service.test.ts index 96b9413780d..fdbc1a72eb3 100644 --- a/apps/sim/lib/uploads/upload-session/service.test.ts +++ b/apps/sim/lib/uploads/upload-session/service.test.ts @@ -66,6 +66,7 @@ vi.mock('@/lib/uploads/upload-session/provider', () => ({ uploadStorageProvider: vi.fn(() => 's3'), })) +import { OrchestrationError } from '@/lib/core/orchestration/types' import { LOCAL_UPLOAD_METADATA_SUFFIX } from '@/lib/uploads/core/storage-key' import { abortUploadSession, @@ -75,6 +76,7 @@ import { createUploadPartUrls, createUploadSession, createUploadSessionAuthBinding, + expectedUploadPartSize, getOwnedUploadSession, getPrincipalKnowledgeDocumentUploadSession, UPLOAD_SESSION_PART_SIZE, @@ -654,6 +656,32 @@ describe('upload sessions', () => { ).rejects.toMatchObject({ code: 'validation' }) }) + /** + * The part-number path segment of a signed part URL is caller-editable, so + * the size lookup is a request boundary. It has to classify an out-of-range + * part as a validation failure for the data-plane route to answer 400 rather + * than falling through to its generic 500. + */ + it('classifies an out-of-range part number as a validation failure', () => { + const multipart = sessionRecord({ + method: 'multipart', + partSize: UPLOAD_SESSION_PART_SIZE, + partCount: 2, + fileSize: UPLOAD_SESSION_PART_SIZE + 1, + }) + + expect(expectedUploadPartSize(multipart, 1)).toBe(UPLOAD_SESSION_PART_SIZE) + expect(expectedUploadPartSize(multipart, 2)).toBe(1) + expect(() => expectedUploadPartSize(multipart, 3)).toThrow(OrchestrationError) + expect(() => expectedUploadPartSize(multipart, 3)).toThrow('partNumber must be between 1 and 2') + try { + expectedUploadPartSize(multipart, 3) + expect.unreachable('out-of-range part number must throw') + } catch (error) { + expect(error).toMatchObject({ code: 'validation' }) + } + }) + it('loads ownership from PostgreSQL and rejects a mismatched token', async () => { const token = 'upload-secret' const row = uploadRow({ tokenHash: sha256Hex(token) }) diff --git a/apps/sim/lib/uploads/upload-session/service.ts b/apps/sim/lib/uploads/upload-session/service.ts index 3f80269150a..73fed9321c1 100644 --- a/apps/sim/lib/uploads/upload-session/service.ts +++ b/apps/sim/lib/uploads/upload-session/service.ts @@ -928,7 +928,10 @@ export function expectedUploadPartSize(session: UploadSessionRecord, partNumber: throw new UploadSessionError('conflict', 'PUT upload sessions do not have multipart parts') } if (!Number.isInteger(partNumber) || partNumber < 1 || partNumber > session.partCount) { - throw new UploadSessionError('validation', 'Invalid upload part number') + throw new UploadSessionError( + 'validation', + `partNumber must be between 1 and ${session.partCount}` + ) } if (partNumber < session.partCount) return session.partSize return session.fileSize - session.partSize * (session.partCount - 1) diff --git a/apps/sim/lib/workflows/custom-tools/operations.test.ts b/apps/sim/lib/workflows/custom-tools/operations.test.ts new file mode 100644 index 00000000000..4316e1a8537 --- /dev/null +++ b/apps/sim/lib/workflows/custom-tools/operations.test.ts @@ -0,0 +1,62 @@ +/** + * @vitest-environment node + */ +import { db } from '@sim/db' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { upsertCustomTools } from '@/lib/workflows/custom-tools/operations' + +const WORKSPACE_ID = 'workspace-1' +const USER_ID = 'user-1' + +const storableSchema = { + type: 'function', + function: { + name: 'lookup_order', + parameters: { type: 'object', properties: { id: { type: 'string' } } }, + }, +} + +describe('upsertCustomTools schema invariant', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('refuses a declaration missing the function discriminator before opening a transaction', async () => { + await expect( + upsertCustomTools({ + tools: [ + { title: 'No discriminator', schema: { function: storableSchema.function }, code: '' }, + ], + workspaceId: WORKSPACE_ID, + userId: USER_ID, + }) + ).rejects.toMatchObject({ code: 'validation' }) + + expect(db.transaction).not.toHaveBeenCalled() + }) + + it('refuses the whole batch when any declaration is unstorable', async () => { + await expect( + upsertCustomTools({ + tools: [ + { title: 'Good', schema: storableSchema, code: '' }, + { title: 'Bad', schema: { ...storableSchema, type: 'object' }, code: '' }, + ], + workspaceId: WORKSPACE_ID, + userId: USER_ID, + }) + ).rejects.toMatchObject({ code: 'validation' }) + + expect(db.transaction).not.toHaveBeenCalled() + }) + + it('opens the transaction for a storable declaration', async () => { + await upsertCustomTools({ + tools: [{ title: 'Good', schema: storableSchema, code: '' }], + workspaceId: WORKSPACE_ID, + userId: USER_ID, + }) + + expect(db.transaction).toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/workflows/custom-tools/operations.ts b/apps/sim/lib/workflows/custom-tools/operations.ts index 79e84b18511..255ac4b8559 100644 --- a/apps/sim/lib/workflows/custom-tools/operations.ts +++ b/apps/sim/lib/workflows/custom-tools/operations.ts @@ -16,6 +16,7 @@ import { timestampKey, } from '@/lib/api/list-query' import { generateRequestId } from '@/lib/core/utils/request' +import { assertStorableCustomToolSchema } from '@/lib/custom-tools/schema' const logger = createLogger('CustomToolsOperations') @@ -38,6 +39,14 @@ export async function upsertCustomTools(params: { }) { const { tools, workspaceId, userId, requestId = generateRequestId() } = params + /** + * Ahead of the transaction so a batch is rejected whole rather than landing + * the tools that preceded the unstorable one. + */ + for (const tool of tools) { + assertStorableCustomToolSchema(tool.schema) + } + return await db.transaction(async (tx) => { for (const tool of tools) { const nowTime = new Date() diff --git a/apps/sim/lib/workflows/persistence/custom-tools-persistence.test.ts b/apps/sim/lib/workflows/persistence/custom-tools-persistence.test.ts new file mode 100644 index 00000000000..65ebb3ed0c8 --- /dev/null +++ b/apps/sim/lib/workflows/persistence/custom-tools-persistence.test.ts @@ -0,0 +1,93 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockUpsertCustomTools } = vi.hoisted(() => ({ + mockUpsertCustomTools: vi.fn(), +})) + +vi.mock('@/lib/workflows/custom-tools/operations', () => ({ + upsertCustomTools: mockUpsertCustomTools, +})) + +import { persistCustomToolsToDatabase } from '@/lib/workflows/persistence/custom-tools-persistence' + +const WORKSPACE_ID = 'workspace-1' +const USER_ID = 'user-1' + +const storableSchema = { + type: 'function' as const, + function: { + name: 'lookup_order', + description: 'Looks up an order', + parameters: { type: 'object', properties: { id: { type: 'string' } }, required: ['id'] }, + }, +} + +function customTool(overrides: Record = {}) { + return { + type: 'custom-tool' as const, + title: 'Lookup order', + schema: storableSchema, + code: 'return 1', + ...overrides, + } as Parameters[0][number] +} + +describe('persistCustomToolsToDatabase', () => { + beforeEach(() => { + vi.clearAllMocks() + mockUpsertCustomTools.mockResolvedValue([]) + }) + + it('persists a storable declaration unchanged', async () => { + const result = await persistCustomToolsToDatabase([customTool()], WORKSPACE_ID, USER_ID) + + expect(result).toEqual({ saved: 1, errors: [] }) + expect(mockUpsertCustomTools).toHaveBeenCalledWith({ + tools: [{ id: undefined, title: 'Lookup order', schema: storableSchema, code: 'return 1' }], + workspaceId: WORKSPACE_ID, + userId: USER_ID, + }) + }) + + it('skips a declaration missing the function discriminator the public API republishes', async () => { + const withoutType = customTool({ + title: 'No discriminator', + schema: { function: storableSchema.function }, + }) + + const result = await persistCustomToolsToDatabase([withoutType], WORKSPACE_ID, USER_ID) + + expect(result).toEqual({ saved: 0, errors: [] }) + expect(mockUpsertCustomTools).not.toHaveBeenCalled() + }) + + it('skips a declaration whose discriminator is not `function`', async () => { + const wrongType = customTool({ + title: 'Wrong discriminator', + schema: { ...storableSchema, type: 'object' }, + }) + + const result = await persistCustomToolsToDatabase([wrongType], WORKSPACE_ID, USER_ID) + + expect(result).toEqual({ saved: 0, errors: [] }) + expect(mockUpsertCustomTools).not.toHaveBeenCalled() + }) + + it('keeps the rest of an import when one declaration is unstorable', async () => { + const result = await persistCustomToolsToDatabase( + [customTool({ title: 'Bad', schema: { function: storableSchema.function } }), customTool()], + WORKSPACE_ID, + USER_ID + ) + + expect(result).toEqual({ saved: 1, errors: [] }) + expect(mockUpsertCustomTools).toHaveBeenCalledWith( + expect.objectContaining({ + tools: [{ id: undefined, title: 'Lookup order', schema: storableSchema, code: 'return 1' }], + }) + ) + }) +}) diff --git a/apps/sim/lib/workflows/persistence/custom-tools-persistence.ts b/apps/sim/lib/workflows/persistence/custom-tools-persistence.ts index 05f215fd8c1..5a3b87db65f 100644 --- a/apps/sim/lib/workflows/persistence/custom-tools-persistence.ts +++ b/apps/sim/lib/workflows/persistence/custom-tools-persistence.ts @@ -1,5 +1,6 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' +import { isStorableCustomToolSchema } from '@/lib/custom-tools/schema' import { upsertCustomTools } from '@/lib/workflows/custom-tools/operations' const logger = createLogger('CustomToolsPersistence') @@ -138,6 +139,17 @@ export async function persistCustomToolsToDatabase( logger.warn(`Skipping custom tool without function name: ${tool.title}`) return false } + /** + * An imported graph can carry any inline declaration, and a stored tool + * whose schema the public API cannot serialize back breaks every read of + * the workspace's tool list. Skipped per tool, like the check above, so one + * unstorable declaration does not cost the rest of the import its tools — + * the tool itself still runs from the inline definition on the block. + */ + if (!isStorableCustomToolSchema(tool.schema)) { + logger.warn(`Skipping custom tool with an unstorable schema: ${tool.title}`) + return false + } return true })