diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index a8215eec631..b1bff5d7222 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -6806,7 +6806,7 @@ "enum": ["live", "deployed"] }, "type": { - "description": "Replacement workflow-group producer type.", + "description": "Workflow-group producer type. Must match the group's stored type — a group's producer cannot be changed after creation.", "type": "string", "enum": ["manual", "enrichment"] }, diff --git a/apps/sim/app/api/v2/workflows/[id]/versions/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/versions/route.test.ts index c6cc6e65974..d9762d98578 100644 --- a/apps/sim/app/api/v2/workflows/[id]/versions/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/versions/route.test.ts @@ -12,7 +12,7 @@ import { } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import { UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' +import { REFILTERED_CURSOR_MESSAGE, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' const mocks = vi.hoisted(() => ({ listVersions: vi.fn(), @@ -43,6 +43,50 @@ const auth = { } const context = { params: Promise.resolve({ id: 'workflow-1' }) } +function contextFor(workflowId: string) { + return { params: Promise.resolve({ id: workflowId }) } +} + +function listVersions(workflowId: string, query = '') { + return GET( + new NextRequest(`http://localhost/api/v2/workflows/${workflowId}/versions${query}`), + contextFor(workflowId) + ) +} + +/** + * A cursor as the route itself mints it, rather than a payload hand-built by + * the test. The binding a cursor carries is the route's to compute, so a test + * that reconstructs it would pass against a route that stopped applying one. + */ +async function mintCursor(workflowId: string): Promise { + mocks.listVersions.mockResolvedValueOnce({ + versions: [ + { + id: 'version-5', + version: 5, + name: 'Production', + description: null, + isActive: true, + createdAt: new Date('2026-08-01T00:00:00.000Z'), + deployedByName: 'Ada', + latestOperationStatus: 'active', + }, + ], + hasMore: true, + }) + const { nextCursor } = await (await listVersions(workflowId)).json() + expect(typeof nextCursor).toBe('string') + return nextCursor +} + +/** A minted cursor's binding, carrying a forged payload inside it. */ +async function forgeInsideBinding(workflowId: string, payload: unknown): Promise { + const { scope } = JSON.parse(Buffer.from(await mintCursor(workflowId), 'base64').toString()) + const inner = Buffer.from(JSON.stringify(payload)).toString('base64') + return Buffer.from(JSON.stringify({ scope, inner })).toString('base64') +} + describe('GET /api/v2/workflows/[id]/versions', () => { beforeEach(() => { vi.clearAllMocks() @@ -125,26 +169,25 @@ describe('GET /api/v2/workflows/[id]/versions', () => { ['carrying an unknown key', { version: 2, sort: 'name' }], ['missing its key', {}], ])('rejects a forged cursor %s', async (_case, payload) => { - const cursor = Buffer.from(JSON.stringify(payload)).toString('base64') - const response = await GET( - new NextRequest( - `http://localhost/api/v2/workflows/workflow-1/versions?cursor=${encodeURIComponent(cursor)}` - ), - context - ) + const cursor = await forgeInsideBinding('workflow-1', payload) + mocks.listVersions.mockClear() + + const response = await listVersions('workflow-1', `?cursor=${encodeURIComponent(cursor)}`) expect(response.status).toBe(400) expect(mocks.listVersions).not.toHaveBeenCalled() }) - it('resumes from a well-formed cursor', async () => { - const cursor = Buffer.from(JSON.stringify({ version: 5 })).toString('base64') - const response = await GET( - new NextRequest( - `http://localhost/api/v2/workflows/workflow-1/versions?cursor=${encodeURIComponent(cursor)}` - ), - context - ) + /** + * The regression guard for the binding: a list still pages through its OWN + * cursor. A token that no route accepts binds nothing — it just breaks + * pagination. + */ + it('resumes from the cursor it minted', async () => { + const cursor = await mintCursor('workflow-1') + mocks.listVersions.mockClear() + + const response = await listVersions('workflow-1', `?cursor=${encodeURIComponent(cursor)}`) expect(response.status).toBe(200) expect(mocks.listVersions).toHaveBeenCalledWith( @@ -152,6 +195,26 @@ describe('GET /api/v2/workflows/[id]/versions', () => { ) }) + /** + * A `version` is an ordinal every workflow's history numbers from 1, so an + * unbound token from a sibling workflow decoded cleanly and resumed from a + * position in a history the caller never walked — silently skipping versions. + */ + it('refuses a cursor minted on another workflow', async () => { + const cursor = await mintCursor('workflow-1') + mocks.listVersions.mockClear() + + const response = await listVersions('workflow-2', `?cursor=${encodeURIComponent(cursor)}`) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toBe(REFILTERED_CURSOR_MESSAGE) + expect(mocks.listVersions).not.toHaveBeenCalled() + }) + + it('mints a cursor bound to the workflow that answered', async () => { + expect(await mintCursor('workflow-1')).not.toBe(await mintCursor('workflow-2')) + }) + it('rejects an unauthenticated request', async () => { v2RouteMocks.authenticate.mockRejectedValueOnce(new MockV2ApiKeyUnauthenticatedError()) diff --git a/apps/sim/app/api/v2/workflows/[id]/versions/route.ts b/apps/sim/app/api/v2/workflows/[id]/versions/route.ts index f0c8088af85..30cff126cec 100644 --- a/apps/sim/app/api/v2/workflows/[id]/versions/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/versions/route.ts @@ -3,17 +3,36 @@ import { v2ListWorkflowVersionsContract, v2WorkflowVersionCursorSchema, } from '@/lib/api/contracts/v2/workflows' -import { UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes' import { OrchestrationError } from '@/lib/core/orchestration/types' import { v2WorkflowErrorPolicies } from '@/lib/workflows/api' import { listWorkflowVersions } from '@/lib/workflows/application/list-workflow-versions' import { workflowOperations } from '@/lib/workflows/application/operations' -import { decodeCursor, encodeCursor } from '@/app/api/v2/lib/response' +import { + decodeCursor, + encodeCursor, + encodeScopedCursor, + readScopedCursor, +} from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 +/** + * The sequence a version cursor names a position in: this list, on THIS + * workflow. + * + * The payload is a bare `version` ordinal and every workflow numbers its + * history from 1, so an unscoped token minted on one workflow decoded cleanly + * against another and answered 200 from a position the caller never reached. + * The workflow id lives in the path, so the route is the only place that knows + * which history the ordinal counts within. + */ +function versionCursorScope(workflowId: string): string { + return cursorScopeKey(cursorRoute(v2ListWorkflowVersionsContract, { id: workflowId })) +} + export const GET = defineV2JsonRoute({ contract: v2ListWorkflowVersionsContract, auth: v2ApiKeyAuth, @@ -21,9 +40,8 @@ export const GET = defineV2JsonRoute({ rateLimit: v2RateLimits.publicApi, errorPolicy: v2WorkflowErrorPolicies.concealWorkflowAuthorization, mapInput: ({ params, query }) => { - const decoded = query.cursor - ? v2WorkflowVersionCursorSchema.safeParse(decodeCursor(query.cursor)) - : undefined + const inner = readScopedCursor(query.cursor, versionCursorScope(params.id)) + const decoded = inner ? v2WorkflowVersionCursorSchema.safeParse(decodeCursor(inner)) : undefined if (decoded && !decoded.success) { throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) } @@ -34,7 +52,7 @@ export const GET = defineV2JsonRoute({ } }, useCase: listWorkflowVersions, - present: ({ versions, hasMore }) => { + present: ({ versions, hasMore }, { params }) => { const data: V2WorkflowVersion[] = versions.map((version) => ({ id: version.id, version: version.version, @@ -50,7 +68,10 @@ export const GET = defineV2JsonRoute({ data, nextCursor: hasMore && data.length > 0 - ? encodeCursor({ version: data[data.length - 1].version }) + ? encodeScopedCursor( + versionCursorScope(params.id), + encodeCursor({ version: data[data.length - 1].version }) + ) : null, } }, diff --git a/apps/sim/app/api/v2/workspaces/[workspaceId]/members/route.ts b/apps/sim/app/api/v2/workspaces/[workspaceId]/members/route.ts index 332a08c6456..190d930ca5f 100644 --- a/apps/sim/app/api/v2/workspaces/[workspaceId]/members/route.ts +++ b/apps/sim/app/api/v2/workspaces/[workspaceId]/members/route.ts @@ -2,7 +2,7 @@ import { v2ListWorkspaceMembersContract, v2WorkspaceMemberCursorSchema, } from '@/lib/api/contracts/v2/workspaces' -import { UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' +import { cursorRoute, cursorScopeKey, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' import { defineV2JsonRoute, v2ApiKeyAuth, @@ -12,7 +12,26 @@ import { import { OrchestrationError } from '@/lib/core/orchestration/types' import { listPublicWorkspaceMembers } from '@/lib/workspaces/application/list-public-workspace-members' import { workspaceOperations } from '@/lib/workspaces/application/operations' -import { decodeCursor, encodeCursor } from '@/app/api/v2/lib/response' +import { + decodeCursor, + encodeCursor, + encodeScopedCursor, + readScopedCursor, +} from '@/app/api/v2/lib/response' + +/** + * The sequence a member cursor names a position in: this roster, on THIS + * workspace. + * + * The payload is a bare email, and the same person is a member of every + * workspace they belong to, so an unscoped token minted on one roster decoded + * cleanly against another and resumed from an email that names a different + * position in it. The workspace id lives in the path, so the route is the only + * place that knows which roster the email indexes. + */ +function memberCursorScope(workspaceId: string): string { + return cursorScopeKey(cursorRoute(v2ListWorkspaceMembersContract, { workspaceId })) +} /** GET /api/v2/workspaces/[workspaceId]/members — Effective member roster. */ export const GET = defineV2JsonRoute({ @@ -22,9 +41,8 @@ export const GET = defineV2JsonRoute({ rateLimit: v2RateLimits.publicApi, errorPolicy: v2OrchestrationErrorPolicy, mapInput: ({ params, query }) => { - const decoded = query.cursor - ? v2WorkspaceMemberCursorSchema.safeParse(decodeCursor(query.cursor)) - : undefined + const inner = readScopedCursor(query.cursor, memberCursorScope(params.workspaceId)) + const decoded = inner ? v2WorkspaceMemberCursorSchema.safeParse(decodeCursor(inner)) : undefined if (decoded && !decoded.success) { throw new OrchestrationError('validation', UNREADABLE_CURSOR_MESSAGE) } @@ -35,7 +53,7 @@ export const GET = defineV2JsonRoute({ } }, useCase: listPublicWorkspaceMembers, - present: ({ page }) => ({ + present: ({ page }, { params }) => ({ data: page.members.map((member) => ({ email: member.email, name: member.name, @@ -44,6 +62,11 @@ export const GET = defineV2JsonRoute({ isExternal: member.isExternal, joinedAt: member.joinedAt.toISOString(), })), - nextCursor: page.nextEmail ? encodeCursor({ email: page.nextEmail }) : null, + nextCursor: page.nextEmail + ? encodeScopedCursor( + memberCursorScope(params.workspaceId), + encodeCursor({ email: page.nextEmail }) + ) + : null, }), }) diff --git a/apps/sim/app/api/v2/workspaces/route.test.ts b/apps/sim/app/api/v2/workspaces/route.test.ts index b4ad92a9872..e57d7679e56 100644 --- a/apps/sim/app/api/v2/workspaces/route.test.ts +++ b/apps/sim/app/api/v2/workspaces/route.test.ts @@ -35,6 +35,7 @@ vi.mock('@/lib/workspaces/application/list-public-workspace-members', () => ({ }, })) +import { REFILTERED_CURSOR_MESSAGE, UNREADABLE_CURSOR_MESSAGE } from '@/lib/api/cursor-binding' import { OrchestrationError } from '@/lib/core/orchestration/types' import { GET as listMembers } from '@/app/api/v2/workspaces/[workspaceId]/members/route' import { GET as getWorkspace } from '@/app/api/v2/workspaces/[workspaceId]/route' @@ -51,7 +52,34 @@ const auth = { rateLimitSubscription: null, keyType: 'workspace' as const, } -const context = () => ({ params: Promise.resolve({ workspaceId: WORKSPACE_ID }) }) +const OTHER_WORKSPACE_ID = 'b1c1a2f5-1f4b-4a2e-9a2f-1d0a5f1c9e77' +const context = (workspaceId: string = WORKSPACE_ID) => ({ + params: Promise.resolve({ workspaceId }), +}) + +function requestMembers(workspaceId: string, query = '') { + return listMembers( + new NextRequest(`http://localhost:3000/api/v2/workspaces/${workspaceId}/members${query}`), + context(workspaceId) + ) +} + +/** + * A cursor as the route itself mints it, rather than a payload hand-built by + * the test. The binding a cursor carries is the route's to compute, so a test + * that reconstructed it would pass against a route that stopped applying one. + */ +async function mintMemberCursor(workspaceId: string): Promise { + const { nextCursor } = await (await requestMembers(workspaceId, '?limit=1')).json() + expect(typeof nextCursor).toBe('string') + return nextCursor +} + +/** The payload inside a minted cursor's binding envelope. */ +function innerPayload(cursor: string): unknown { + const { inner } = JSON.parse(Buffer.from(cursor, 'base64').toString()) + return JSON.parse(Buffer.from(inner, 'base64').toString()) +} describe('v2 workspace routes', () => { beforeEach(() => { @@ -122,18 +150,72 @@ describe('v2 workspace routes', () => { isExternal: false, joinedAt: '2026-01-01T00:00:00.000Z', }) - expect(JSON.parse(Buffer.from(body.nextCursor, 'base64').toString())).toEqual({ - email: 'ada@example.com', - }) + expect(innerPayload(body.nextCursor)).toEqual({ email: 'ada@example.com' }) }) it('rejects malformed cursors before the application read', async () => { - const response = await listMembers( - new NextRequest( - `http://localhost:3000/api/v2/workspaces/${WORKSPACE_ID}/members?cursor=not-a-cursor` - ), - context() + const response = await requestMembers(WORKSPACE_ID, '?cursor=not-a-cursor') + + expect(response.status).toBe(400) + expect(mocks.listMembers).not.toHaveBeenCalled() + expect((await response.json()).error.message).toBe(UNREADABLE_CURSOR_MESSAGE) + }) + + /** + * The regression guard for the cursor's binding: the roster still pages + * through its OWN cursor. A token no route accepts binds nothing — it just + * breaks pagination. + */ + it('resumes from the cursor it minted', async () => { + const cursor = await mintMemberCursor(WORKSPACE_ID) + mocks.listMembers.mockClear() + + const response = await requestMembers(WORKSPACE_ID, `?cursor=${encodeURIComponent(cursor)}`) + + expect(response.status).toBe(200) + expect(mocks.listMembers).toHaveBeenCalledWith( + expect.objectContaining({ input: expect.objectContaining({ afterEmail: 'ada@example.com' }) }) + ) + }) + + /** + * The same person is a member of every workspace they belong to, so an + * unbound email token from one roster decoded cleanly against another and + * resumed from a position that silently skipped members. + */ + it('refuses a members cursor minted on another workspace', async () => { + const cursor = await mintMemberCursor(WORKSPACE_ID) + mocks.listMembers.mockClear() + + const response = await requestMembers( + OTHER_WORKSPACE_ID, + `?cursor=${encodeURIComponent(cursor)}` + ) + + expect(response.status).toBe(400) + expect((await response.json()).error.message).toBe(REFILTERED_CURSOR_MESSAGE) + expect(mocks.listMembers).not.toHaveBeenCalled() + }) + + it('mints a members cursor bound to the workspace that answered', async () => { + expect(await mintMemberCursor(WORKSPACE_ID)).not.toBe( + await mintMemberCursor(OTHER_WORKSPACE_ID) ) + }) + + it.each([ + ['non-email', { email: 'not-an-email' }], + ['carrying an unknown key', { email: 'ada@example.com', role: 'admin' }], + ['missing its key', {}], + ])('rejects a members cursor forged inside a valid binding %s', async (_case, payload) => { + const { scope } = JSON.parse( + Buffer.from(await mintMemberCursor(WORKSPACE_ID), 'base64').toString() + ) + const inner = Buffer.from(JSON.stringify(payload)).toString('base64') + const cursor = Buffer.from(JSON.stringify({ scope, inner })).toString('base64') + mocks.listMembers.mockClear() + + const response = await requestMembers(WORKSPACE_ID, `?cursor=${encodeURIComponent(cursor)}`) expect(response.status).toBe(400) expect(mocks.listMembers).not.toHaveBeenCalled() diff --git a/apps/sim/lib/api/contracts/tables.ts b/apps/sim/lib/api/contracts/tables.ts index 0186c186e69..a75f2ab495e 100644 --- a/apps/sim/lib/api/contracts/tables.ts +++ b/apps/sim/lib/api/contracts/tables.ts @@ -1684,8 +1684,16 @@ export const updateWorkflowGroupBodySchema = z.object({ deploymentMode: workflowGroupDeploymentModeSchema .optional() .describe('Replacement workflow execution mode.'), - /** Update the group's provenance. Omit to leave unchanged. */ - type: workflowGroupTypeSchema.optional().describe('Replacement workflow-group producer type.'), + /** + * Echo of the group's provenance. A producer is fixed at creation: this body + * has no `enrichmentId` field, so it can never supply the coordinate a new + * type would need. A `type` that differs from the stored one is a 400. + */ + type: workflowGroupTypeSchema + .optional() + .describe( + "Workflow-group producer type. Must match the group's stored type — a group's producer cannot be changed after creation." + ), /** Toggle the group's persisted auto-run flag. Omit to leave unchanged. */ autoRun: z.boolean().optional().describe('Replacement automatic-run setting.'), }) diff --git a/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts index b6034e3af3b..5004b64c806 100644 --- a/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts +++ b/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts @@ -174,6 +174,35 @@ const CURSOR_BINDINGS: Record = { 'GET /api/v2/workspaces/[workspaceId]/members': [], } +/** + * The path params each nested paged list binds its cursor to — the ones that + * name WHICH parent resource the sequence belongs to. + * + * {@link CURSOR_BINDINGS} covers only what a contract accepts as query or body, + * so a nested list's parent id is invisible to it: an empty binding there reads + * the same whether the list genuinely has no filters or whether its parent was + * forgotten. Both readings were true at once — `GET /workflows/[id]/versions` + * and `GET /workspaces/[workspaceId]/members` declared `[]`, accepted a sibling + * parent's token, and answered 200 from a position in a sequence the caller + * never walked. + * + * Every placeholder in a paged list's path is bound, with no exemptions. A path + * param is never merely an asserted scope the way a `workspaceId` *query* param + * is on the table lists — that one is refused by authorization before paging, + * which is why it is exempted in {@link UNBOUND_PARAMS} instead. A placeholder + * is what picks the sequence out, so leaving one unbound is exactly the defect + * above. Routes apply this through `cursorRoute(contract, params)`, which + * resolves the path before fingerprinting it. + */ +const CURSOR_BOUND_PATH_PARAMS: Record = { + 'GET /api/v2/knowledge/[id]/documents': ['id'], + 'GET /api/v2/tables/[tableId]/rows': ['tableId'], + 'POST /api/v2/tables/[tableId]/query': ['tableId'], + 'GET /api/v2/workflows/[id]/runs': ['id'], + 'GET /api/v2/workflows/[id]/versions': ['id'], + 'GET /api/v2/workspaces/[workspaceId]/members': ['workspaceId'], +} + /** * Params a paged list accepts that its cursor is deliberately NOT bound to, * with the reason. Anything not listed here and not in {@link CURSOR_BINDINGS} @@ -556,6 +585,24 @@ describe('v2 list pagination split', () => { expect([...declared].sort()).toEqual([...PAGED_LISTS].sort()) }) + it('binds every paged list to the parent its path names', () => { + for (const key of PAGED_LISTS) { + const placeholders = [...key.matchAll(/\[([^\]]+)\]/g)].map(([, name]) => name) + + expect( + [...(CURSOR_BOUND_PATH_PARAMS[key] ?? [])].sort(), + `${key} does not bind its cursor to the path param naming its parent resource. Pass it through cursorRoute(contract, params) in the route and declare it in CURSOR_BOUND_PATH_PARAMS; otherwise a sibling parent's token decodes cleanly here and answers from a sequence the caller never walked.` + ).toEqual(placeholders.sort()) + } + }) + + it('never declares a path binding for a list that has no such parent', () => { + for (const [key, bound] of Object.entries(CURSOR_BOUND_PATH_PARAMS)) { + expect(PAGED_LISTS.includes(key as never), `${key} is not a paged list`).toBe(true) + expect(bound.length, `${key} declares an empty path binding`).toBeGreaterThan(0) + } + }) + it('binds every sequence-affecting param a paged list accepts', async () => { const contracts = await loadV2ListContracts() const byKey = new Map(contracts.map((c) => [c.key, c])) diff --git a/apps/sim/lib/table/application/groups.test.ts b/apps/sim/lib/table/application/groups.test.ts index 0375eb0c290..6eb36a21c5c 100644 --- a/apps/sim/lib/table/application/groups.test.ts +++ b/apps/sim/lib/table/application/groups.test.ts @@ -414,6 +414,146 @@ describe('workflow and enrichment Table application commands', () => { expect(mocks.audit).not.toHaveBeenCalled() }) + it('refuses a created enrichment group whose enrichment id the registry does not define', async () => { + mocks.getEnrichment.mockReturnValue(undefined) + + await expect( + createTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + group: { + type: 'enrichment', + enrichmentId: 'no-such-enrichment', + outputs: [{ blockId: '', path: '', columnName: 'domain' }], + }, + outputColumns: [{ name: 'domain', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: expect.stringContaining('Unknown enrichment "no-such-enrichment"'), + }) + + expect(mocks.addGroup).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + + it('refuses a created enrichment output the registry does not define', async () => { + await expect( + createTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + group: { + type: 'enrichment', + enrichmentId: 'company-domain', + outputs: [{ blockId: '', path: '', outputId: 'nosuch', columnName: 'bogus' }], + }, + outputColumns: [{ name: 'bogus', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Enrichment "Company Domain" has no output "nosuch"', + }) + + expect(mocks.addGroup).not.toHaveBeenCalled() + }) + + it('refuses a created enrichment output that carries no output id', async () => { + await expect( + createTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + group: { + type: 'enrichment', + enrichmentId: 'company-domain', + outputs: [{ blockId: '', path: '', columnName: 'domain' }], + }, + outputColumns: [{ name: 'domain', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: 'Enrichment "Company Domain" has no output ""', + }) + + expect(mocks.addGroup).not.toHaveBeenCalled() + }) + + it('refuses a created workflow group output coordinate the workflow cannot produce', async () => { + await expect( + createTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + group: { + id: 'group-new', + workflowId: 'workflow-1', + outputs: [{ blockId: 'block-missing', path: 'nope', columnName: 'nope' }], + }, + outputColumns: [{ name: 'nope', type: 'string' }], + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: expect.stringContaining('Invalid output(s) for workflow workflow-1'), + }) + + expect(mocks.addGroup).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + + it('still creates a workflow group whose output coordinates the workflow produces', async () => { + const result = await createTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + group: { + id: 'group-new', + workflowId: 'workflow-1', + outputs: [{ blockId: 'block-2', path: 'score', columnName: 'score' }], + }, + outputColumns: [{ name: 'score', type: 'number' }], + }, + }) + + expect(mocks.addGroup).toHaveBeenCalledWith( + expect.objectContaining({ + group: expect.objectContaining({ id: 'group-new', workflowId: 'workflow-1' }), + }), + 'request-1' + ) + expect(result.group.workflowId).toBe('workflow-1') + }) + + it('still creates an enrichment-template group that carries a backing workflow', async () => { + const result = await createTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + group: { + id: 'group-new', + type: 'enrichment', + workflowId: 'workflow-1', + outputs: [{ blockId: 'block-2', path: 'score', columnName: 'score' }], + }, + outputColumns: [{ name: 'score', type: 'number' }], + }, + }) + + expect(mocks.getEnrichment).not.toHaveBeenCalled() + expect(result.group.workflowId).toBe('workflow-1') + }) + it('preserves the internal update contract for an invalid related workflow', async () => { mocks.resolveWorkflowContext.mockRejectedValueOnce( new OrchestrationError('not_found', 'Workflow not found') @@ -556,6 +696,114 @@ describe('workflow and enrichment Table application commands', () => { ) }) + it('refuses relabelling a workflow group as an enrichment', async () => { + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + type: 'enrichment', + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: + 'Workflow group "group-1" cannot change type from "manual" to "enrichment"; create a new group for a different producer', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + + it('refuses relabelling an enrichment group as a workflow group', async () => { + useEnrichmentTable() + + await expect( + updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: enrichmentGroup.id, + type: 'manual', + }, + }) + ).rejects.toMatchObject({ + code: 'validation', + message: + 'Workflow group "group-enrichment" cannot change type from "enrichment" to "manual"; create a new group for a different producer', + }) + + expect(mocks.updateGroup).not.toHaveBeenCalled() + expect(mocks.audit).not.toHaveBeenCalled() + }) + + it('accepts a type that echoes the group it is updating', async () => { + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + type: 'manual', + name: 'Renamed group', + }, + }) + + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ type: 'manual', name: 'Renamed group' }), + 'request-1' + ) + }) + + it('still applies an update that leaves the group type alone', async () => { + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + name: 'Renamed group', + autoRun: false, + }, + }) + + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ name: 'Renamed group', autoRun: false }), + 'request-1' + ) + }) + + it('lets an enrichment-template group backed by a workflow keep its enrichment label', async () => { + const templateGroup: WorkflowGroup = { ...group, type: 'enrichment' } + mocks.resolveContext.mockResolvedValue({ + tableId: table.id, + table: tableWithGroup(templateGroup), + workspaceId: table.workspaceId, + workspaceOrganizationId: null, + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + }) + + await updateTableGroupUseCase.execute({ + principal, + input: { + tableId: table.id, + workspaceId: table.workspaceId, + groupId: group.id, + type: 'enrichment', + name: 'Renamed template', + }, + }) + + expect(mocks.updateGroup).toHaveBeenCalledWith( + expect.objectContaining({ type: 'enrichment', name: 'Renamed template' }), + 'request-1' + ) + }) + it('names the enrichment instead of a missing workflow for a mapping update', async () => { useEnrichmentTable() diff --git a/apps/sim/lib/table/application/groups.ts b/apps/sim/lib/table/application/groups.ts index a9e692d6c10..f8eef13eaca 100644 --- a/apps/sim/lib/table/application/groups.ts +++ b/apps/sim/lib/table/application/groups.ts @@ -233,8 +233,36 @@ export const createTableGroupUseCase = defineAuthorizedTableUseCase({ requireBoundedGroupItems(input.group.outputs, 'Workflow group outputs') requireBoundedGroupItems(input.outputColumns, 'Workflow group output columns') requireBoundedGroupItems(input.group.inputMappings, 'Workflow group input mappings') + /** + * Creation must refuse the coordinate an update refuses. A group stores the + * mapping a run reads to fill a cell, so an output naming a workflow output + * that does not exist — or an enrichment output the registry does not + * define — creates a column nothing can ever populate, and the caller only + * discovers it when they later try to edit the group. + * + * `workflowId` is the producer discriminator, not `type`: an enrichment + * template spawned from the workflow sidebar carries `type: 'enrichment'` + * with a backing workflow and workflow output coordinates, and only a group + * with no workflow is filled from the enrichment registry. + */ if (input.group.workflowId) { - await resolveRelatedWorkflowForTableRoute(input.group.workflowId, context.workspaceId) + const resolvedWorkflow = await resolveRelatedWorkflowForTableRoute( + input.group.workflowId, + context.workspaceId + ) + validateRequestedOutputs( + input.group.outputs.map((output) => ({ + blockId: output.blockId ?? '', + path: output.path ?? '', + })), + resolvedWorkflow, + input.group.workflowId + ) + } else if (input.group.enrichmentId) { + requireKnownEnrichmentOutputIds( + requireEnrichment(input.group.enrichmentId), + input.group.outputs.map((output) => output.outputId) + ) } const outputNames = new Set(input.group.outputs.map((output) => output.columnName)) const orphan = input.outputColumns.find((column) => !outputNames.has(column.name)) @@ -586,6 +614,35 @@ export const updateTableGroupUseCase = defineAuthorizedTableUseCase({ const previousGroup = (context.table.schema.workflowGroups ?? []).find( (group) => group.id === input.groupId ) + /** + * `type` is provenance, not a producer switch. What a run actually reads is + * the pair the group was created with — `workflowId` for a workflow-backed + * group, `enrichmentId` for a registry one — and neither is settable here: + * the update body has no `enrichmentId` field at all. So a `type` flip only + * ever relabels a group into a coordinate creation refuses, and one it + * cannot be talked back out of. + * + * `manual` → `enrichment` leaves `enrichmentId` undefined, which is the + * exact shape `refineGroupSource` rejects on create, and it bricks the + * group for output editing: `addWorkflowTableGroupOutput` and + * `updateWorkflowTableGroup` both refuse a group whose `type` reads + * `enrichment`. `enrichment` → `manual` is worse — it keeps `enrichmentId` + * but steers the runner off the enrichment branch and onto the workflow + * one, where the group's `workflowId` is `''` and every cell run fails. + * + * Re-sending the type the group already has stays a no-op, so a caller that + * echoes back a whole group is unaffected. + */ + if ( + previousGroup && + input.type !== undefined && + input.type !== (previousGroup.type ?? 'manual') + ) { + throw new OrchestrationError( + 'validation', + `Workflow group "${input.groupId}" cannot change type from "${previousGroup.type ?? 'manual'}" to "${input.type}"; create a new group for a different producer` + ) + } /** * An enrichment group's outputs come from the registry, not from a workflow, * and it stores `workflowId: ''` — so there is nothing to resolve a new