Skip to content

Commit b80a521

Browse files
fix(workflows): preserve VFS validation errors
1 parent 889d5d3 commit b80a521

4 files changed

Lines changed: 73 additions & 44 deletions

File tree

apps/sim/lib/copilot/application/execute-workflow-use-case.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ describe('executeCopilotWorkflowUseCase', () => {
8080
},
8181
{ workflowId: 'workflow-1' }
8282
)
83-
).toThrow('Unregistered Copilot workspace operation')
83+
).toThrow('Unregistered Copilot workflow operation')
8484
expect(execute).not.toHaveBeenCalled()
8585
})
8686

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,36 @@
1-
import { createLogger } from '@sim/logger'
2-
import { createCopilotWorkspaceUseCaseExecutor } from '@/lib/copilot/application/execute-workspace-use-case'
3-
import { asOrchestrationError } from '@/lib/core/orchestration/types'
1+
import { createCopilotApplicationAdapter } from '@/lib/copilot/application/application-adapter'
2+
import { messageForCopilotApplicationError } from '@/lib/copilot/application/error'
3+
import {
4+
COPILOT_APPLICATION_DELEGATION_TTL_MS,
5+
type CopilotExecutionContext,
6+
} from '@/lib/copilot/auth/application-delegation'
7+
import type { OperationUseCase } from '@/lib/core/application'
48
import { WORKFLOW_DELEGATION_AUDIENCE } from '@/lib/workflows/application/authorization'
5-
import { workflowOperations } from '@/lib/workflows/application/operations'
9+
import { type WorkflowOperation, workflowOperations } from '@/lib/workflows/application/operations'
610

7-
const logger = createLogger('CopilotWorkflowApplication')
8-
9-
/** Enters a registered workflow use case with identity derived only from trusted tool context. */
10-
export const executeCopilotWorkflowUseCase = createCopilotWorkspaceUseCaseExecutor({
11-
audience: WORKFLOW_DELEGATION_AUDIENCE,
11+
const executeWorkflowUseCase = createCopilotApplicationAdapter({
12+
domain: 'workflow',
13+
delegation: {
14+
audience: WORKFLOW_DELEGATION_AUDIENCE,
15+
ttlMs: COPILOT_APPLICATION_DELEGATION_TTL_MS,
16+
createDelegationId: (context) => `copilot-tool:${context.toolCallId}`,
17+
},
1218
operations: workflowOperations,
1319
})
1420

21+
/** Enters a registered workflow use case with identity derived only from trusted tool context. */
22+
export function executeCopilotWorkflowUseCase<O extends WorkflowOperation, I, R>(
23+
context: CopilotExecutionContext | undefined,
24+
useCase: OperationUseCase<O, I, R>,
25+
input: I
26+
): Promise<R> {
27+
return executeWorkflowUseCase(context, useCase, input)
28+
}
29+
1530
/** Projects actionable application errors without exposing infrastructure details to the model. */
1631
export function messageForCopilotWorkflowError(
1732
error: unknown,
1833
fallback = 'Workflow operation failed'
1934
): string {
20-
const classified = asOrchestrationError(error)
21-
if (classified && classified.code !== 'internal') return classified.message
22-
logger.error('Workflow application operation failed', { error })
23-
return fallback
35+
return messageForCopilotApplicationError(error, fallback)
2436
}

apps/sim/lib/copilot/tools/handlers/vfs-mutate.test.ts

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
*/
44
import { dbChainMock, resetDbChainMock, schemaMock, workflowAuthzMockFns } from '@sim/testing'
55
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { knowledgeOperations } from '@/lib/knowledge/application/operations'
7+
import { workflowOperations } from '@/lib/workflows/application/operations'
8+
import { fileOperations } from '@/lib/workspace-files/application/operations'
69

710
const mocks = vi.hoisted(() => ({
811
ensureWorkspaceAccess: vi.fn(),
@@ -74,41 +77,28 @@ vi.mock('@/lib/copilot/tools/server/files/file-folder-application', () => ({
7477

7578
vi.mock('@/lib/workspace-files/application/move-workspace-file-items', () => ({
7679
moveWorkspaceFileItemsOperation: {
77-
operation: { id: 'files.move', minimumRole: 'write', workspaceApiKey: 'allow' },
80+
operation: fileOperations.move,
7881
execute: mocks.moveWorkspaceFileItems,
7982
},
8083
}))
8184

82-
vi.mock('@/lib/workspace-files/application/operations', () => ({
83-
fileOperations: {
84-
move: { id: 'files.move', minimumRole: 'write', workspaceApiKey: 'allow' },
85-
rename: { id: 'files.rename', minimumRole: 'write', workspaceApiKey: 'allow' },
86-
delete: { id: 'files.delete', minimumRole: 'write', workspaceApiKey: 'allow' },
87-
updateFolder: {
88-
id: 'files.folders.update',
89-
minimumRole: 'write',
90-
workspaceApiKey: 'allow',
91-
},
92-
},
93-
}))
94-
9585
vi.mock('@/lib/workspace-files/application/workspace-file-folders', () => ({
9686
updateWorkspaceFileFolderOperation: {
97-
operation: { id: 'files.folders.update', minimumRole: 'write', workspaceApiKey: 'allow' },
87+
operation: fileOperations.updateFolder,
9888
execute: mocks.updateWorkspaceFileFolder,
9989
},
10090
}))
10191

10292
vi.mock('@/lib/workspace-files/application/delete-workspace-file', () => ({
10393
deleteWorkspaceFileOperation: {
104-
operation: { id: 'files.delete', minimumRole: 'write', workspaceApiKey: 'allow' },
94+
operation: fileOperations.delete,
10595
execute: mocks.deleteWorkspaceFile,
10696
},
10797
}))
10898

10999
vi.mock('@/lib/workspace-files/application/archive-workspace-file-items', () => ({
110100
archiveWorkspaceFileItemsOperation: {
111-
operation: { id: 'files.delete', minimumRole: 'write', workspaceApiKey: 'allow' },
101+
operation: fileOperations.delete,
112102
execute: mocks.deleteWorkspaceFile,
113103
},
114104
}))
@@ -117,26 +107,26 @@ vi.mock('@/lib/workspace-files/orchestration', () => ({}))
117107

118108
vi.mock('@/lib/workspace-files/application/rename-workspace-file', () => ({
119109
renameWorkspaceFile: {
120-
operation: { id: 'files.rename', minimumRole: 'write', workspaceApiKey: 'allow' },
110+
operation: fileOperations.rename,
121111
execute: mocks.renameWorkspaceFile,
122112
},
123113
}))
124114

125115
vi.mock('@/lib/workflows/application/workflow-vfs', () => ({
126116
moveWorkflowVfsItems: {
127-
operation: { id: 'workflows.vfs.move' },
117+
operation: workflowOperations.moveVfsItems,
128118
execute: mocks.moveWorkflowVfs,
129119
},
130120
copyWorkflowVfsItems: {
131-
operation: { id: 'workflows.vfs.copy' },
121+
operation: workflowOperations.copyVfsItems,
132122
execute: mocks.copyWorkflowVfs,
133123
},
134124
createWorkflowVfsFolders: {
135-
operation: { id: 'workflows.vfs.folders.create' },
125+
operation: workflowOperations.createVfsFolders,
136126
execute: mocks.createWorkflowVfsFolders,
137127
},
138128
deleteWorkflowVfsItems: {
139-
operation: { id: 'workflows.vfs.delete' },
129+
operation: workflowOperations.deleteVfsItems,
140130
execute: mocks.deleteWorkflowVfs,
141131
},
142132
}))
@@ -148,15 +138,15 @@ vi.mock('@/lib/table/service', () => ({
148138

149139
vi.mock('@/lib/knowledge/application/knowledge-bases', () => ({
150140
listKnowledgeBases: {
151-
operation: { id: 'knowledge.list' },
141+
operation: knowledgeOperations.list,
152142
execute: mocks.listKnowledgeBases,
153143
},
154144
updateKnowledgeBaseOperation: {
155-
operation: { id: 'knowledge.update' },
145+
operation: knowledgeOperations.update,
156146
execute: mocks.updateKnowledgeBase,
157147
},
158148
deleteKnowledgeBaseOperation: {
159-
operation: { id: 'knowledge.delete' },
149+
operation: knowledgeOperations.delete,
160150
execute: mocks.deleteKnowledgeBase,
161151
},
162152
}))
@@ -473,6 +463,25 @@ describe('vfs mv/cp', () => {
473463
expect(result.success).toBe(true)
474464
})
475465

466+
it('preserves safe workflow application validation errors', async () => {
467+
mocks.moveWorkflowVfs.mockRejectedValueOnce(
468+
new OrchestrationError(
469+
'validation',
470+
'With multiple sources the destination must be a folder'
471+
)
472+
)
473+
474+
const result = await executeVfsMv(
475+
{ sources: ['workflows/One', 'workflows/Two'], destination: 'workflows/Renamed' },
476+
context
477+
)
478+
479+
expect(result).toEqual({
480+
success: false,
481+
error: 'With multiple sources the destination must be a folder',
482+
})
483+
})
484+
476485
it('surfaces locked-workflow rejections per item', async () => {
477486
mocks.moveWorkflowVfs.mockResolvedValue({
478487
outcomes: [

apps/sim/lib/copilot/tools/handlers/vfs-mutate.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -601,11 +601,19 @@ async function mutateWorkflows(
601601
trailingSlash: hasTrailingSlash(destination),
602602
},
603603
}
604-
const result =
605-
verb === 'cp'
606-
? await executeCopilotWorkflowUseCase(context, copyWorkflowVfsItems, input)
607-
: await executeCopilotWorkflowUseCase(context, moveWorkflowVfsItems, input)
608-
return buildResult(verb, result.outcomes.map(presentWorkflowVfsOutcome))
604+
try {
605+
const result =
606+
verb === 'cp'
607+
? await executeCopilotWorkflowUseCase(context, copyWorkflowVfsItems, input)
608+
: await executeCopilotWorkflowUseCase(context, moveWorkflowVfsItems, input)
609+
return buildResult(verb, result.outcomes.map(presentWorkflowVfsOutcome))
610+
} catch (error) {
611+
if (context.abortSignal?.aborted) throw error
612+
return {
613+
success: false,
614+
error: messageForCopilotWorkflowError(error, 'Workflow mutation failed'),
615+
}
616+
}
609617
}
610618

611619
async function renameFlatResource(

0 commit comments

Comments
 (0)