From 717b4bf3db7e9d7b7104217a4a7f5c923eab6491 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 16:53:44 -0700 Subject: [PATCH] fix(v2): hold create to the rules update enforces, and bind the last two cursors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects, one shape: a rule applied to one path and not its sibling. Two were found by probing the live surface after the previous fixes deployed, and the third by reading for the pattern. Creating a workflow group through the public surface validated almost nothing the update path validates. An enrichment group could name an enrichment the registry does not define, or an output the enrichment does not have, or carry no output id at all — each a 201 storing a column no run can ever write, discovered only when the caller later tried to edit the group and got the 400 create should have given. The workflow half was the same: a fabricated block-and-path coordinate was stored on create and refused on update. Create now runs the same two registry helpers and the same workflow-output check the update path uses. The discriminator there is the backing workflow id, not the declared type. The workflow sidebar creates enrichment-template groups labelled `enrichment` while backed by a real workflow and carrying no enrichment id, so keying on the label would have refused the first-party create path outright. A group's producer type could also be relabelled after the fact into a state creation refuses. Nothing rejected it and nothing could repair it, since the update body carries no enrichment id to supply. Relabelling an enrichment group as workflow-backed is the harmful direction: it keeps the enrichment id while moving the group onto the workflow branch with an empty workflow id, so every cell run fails. An update may now only restate the type the group already has. The workflow-version and workspace-member lists were the last two paged reads minting cursors with no route identity, so a token from one parent resumed another at a position that silently skips rows — the defect the previous change closed everywhere else. Both now wrap their domain token with the same scope binding, and the pagination guardrail gained a declaration of every nested list's parent path param, because the old one recorded only query filters and so could not tell an unfiltered list from a forgotten parent. --- apps/docs/openapi-v2-tables.json | 2 +- .../v2/workflows/[id]/versions/route.test.ts | 95 +++++-- .../api/v2/workflows/[id]/versions/route.ts | 35 ++- .../workspaces/[workspaceId]/members/route.ts | 37 ++- apps/sim/app/api/v2/workspaces/route.test.ts | 100 ++++++- apps/sim/lib/api/contracts/tables.ts | 12 +- .../v2/__tests__/list-pagination.test.ts | 47 ++++ apps/sim/lib/table/application/groups.test.ts | 248 ++++++++++++++++++ apps/sim/lib/table/application/groups.ts | 59 ++++- 9 files changed, 592 insertions(+), 43 deletions(-) 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