Skip to content

Commit d67ada7

Browse files
icecrasher321icecrasher321
committed
fix(provenance): keep model turns running after attachment refusal
1 parent b4e4328 commit d67ada7

7 files changed

Lines changed: 279 additions & 91 deletions

File tree

‎apps/sim/executor/handlers/agent/agent-handler.test.ts‎

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,6 @@ vi.mock('@/lib/internal/mcp/discover-tools', () => ({
5353
}))
5454

5555
vi.mock('@/lib/uploads/contexts/workspace/workspace-file-secret-provenance', () => ({
56-
MODEL_UNSAFE_WORKSPACE_FILE_ERROR_MESSAGE:
57-
'File cannot be sent to a model because its secret provenance is unavailable',
5856
importWorkspaceFileSecretProvenanceForModelView:
5957
mockImportWorkspaceFileSecretProvenanceForModelView,
6058
}))
@@ -1042,9 +1040,13 @@ describe('AgentBlockHandler', () => {
10421040
}
10431041
})
10441042

1045-
it.each([false, true])(
1046-
'honors generated document contributor admission (safe=%s)',
1047-
async (safe) => {
1043+
it.each([
1044+
{ safe: false, includeSafeFile: false },
1045+
{ safe: false, includeSafeFile: true },
1046+
{ safe: true, includeSafeFile: true },
1047+
])(
1048+
'continues after document contributor admission (safe=$safe, mixed=$includeSafeFile)',
1049+
async ({ safe, includeSafeFile }) => {
10481050
const key = 'workspace/ws-1/report.pdf'
10491051
mockContext.workspaceId = 'ws-1'
10501052
const hydrationSpy = vi
@@ -1065,7 +1067,7 @@ describe('AgentBlockHandler', () => {
10651067
try {
10661068
mockGetProviderFromModel.mockReturnValue('openai')
10671069

1068-
const execution = handler.execute(mockContext, mockBlock, {
1070+
await handler.execute(mockContext, mockBlock, {
10691071
model: 'gpt-4o',
10701072
userPrompt: 'Analyze this document',
10711073
files: [
@@ -1077,20 +1079,35 @@ describe('AgentBlockHandler', () => {
10771079
size: 128,
10781080
type: 'text/x-python-pdf',
10791081
},
1082+
...(includeSafeFile
1083+
? [
1084+
{
1085+
id: 'file-2',
1086+
name: 'safe.pdf',
1087+
path: '/safe.pdf',
1088+
key: 'workspace/ws-1/safe.pdf',
1089+
size: 128,
1090+
type: 'application/pdf',
1091+
},
1092+
]
1093+
: []),
10801094
],
10811095
apiKey: 'test-api-key',
10821096
})
10831097

1098+
expect(mockExecuteProviderRequest).toHaveBeenCalledOnce()
1099+
const sent = mockExecuteProviderRequest.mock.calls[0][1].messages.at(-1)
1100+
expect(sent.files.map((file: { id: string }) => file.id)).toEqual([
1101+
...(safe ? ['file-1'] : []),
1102+
...(includeSafeFile ? ['file-2'] : []),
1103+
])
10841104
if (safe) {
1085-
await execution
1086-
expect(mockExecuteProviderRequest.mock.calls[0][1].messages.at(-1)?.files).toEqual([
1087-
expect.objectContaining({ key, base64: 'JVBERi0=' }),
1088-
])
1105+
expect(sent.content).toBe('Analyze this document')
10891106
} else {
1090-
await expect(execution).rejects.toThrow(
1091-
'File cannot be sent to a model because its secret provenance is unavailable'
1107+
expect(sent.content).toMatch(
1108+
/^Analyze this document\n\nAttachment error: 1 requested file attachment was not provided/
10921109
)
1093-
expect(mockExecuteProviderRequest).not.toHaveBeenCalled()
1110+
expect(JSON.stringify(sent)).not.toContain(key)
10941111
}
10951112
expect(mockImportWorkspaceFileSecretProvenanceForModelView).toHaveBeenCalledWith(
10961113
expect.objectContaining({

‎apps/sim/executor/handlers/agent/agent-handler.ts‎

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,18 +26,18 @@ import {
2626
resolveAutoModel,
2727
SIM_AUTO_SYSTEM_PREAMBLE,
2828
} from '@/lib/model-router/resolve'
29-
import {
30-
importWorkspaceFileSecretProvenanceForModelView,
31-
MODEL_UNSAFE_WORKSPACE_FILE_ERROR_MESSAGE,
32-
} from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance'
29+
import { importWorkspaceFileSecretProvenanceForModelView } from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance'
3330
import {
3431
getFileExtension,
3532
MODEL_SUPPORTED_IMAGE_MIME_TYPES,
3633
processFilesToUserFiles,
3734
type RawFileInput,
3835
tryInferContextFromKey,
3936
} from '@/lib/uploads/utils/file-utils'
40-
import { selectModelBoundFileInputPaths } from '@/lib/uploads/utils/model-input'
37+
import {
38+
appendUnavailableAttachmentNotice,
39+
selectModelBoundFileInputPaths,
40+
} from '@/lib/uploads/utils/model-input'
4141
import { hydrateUserFilesWithBase64 } from '@/lib/uploads/utils/user-file-base64.server'
4242
import { resolveCustomBlockToolBinding } from '@/lib/workflows/custom-blocks/operations'
4343
import {
@@ -1496,6 +1496,7 @@ export class AgentBlockHandler implements BlockHandler {
14961496
continue
14971497
}
14981498

1499+
const unsafeGeneratedDocumentFiles = new Set<string>()
14991500
const groups = new Map<boolean, Array<{ file: UserFile; index: number }>>()
15001501
message.files.forEach((file, index) => {
15011502
const workspaceFile =
@@ -1513,7 +1514,7 @@ export class AgentBlockHandler implements BlockHandler {
15131514
...(await resolveExecutorFileMaterializationContext(ctx, group[0].file)),
15141515
logger,
15151516
maxBytes: inlineMaxBytes,
1516-
onServableFileContributors: async (_file, contributors) => {
1517+
onServableFileContributors: async (file, contributors) => {
15171518
if (!ctx.workspaceId) return
15181519
for (const identity of contributors) {
15191520
const safe = await importWorkspaceFileSecretProvenanceForModelView({
@@ -1524,7 +1525,8 @@ export class AgentBlockHandler implements BlockHandler {
15241525
...(ctx.userId ? { actorUserId: ctx.userId } : {}),
15251526
})
15261527
if (!safe) {
1527-
throw new Error(MODEL_UNSAFE_WORKSPACE_FILE_ERROR_MESSAGE)
1528+
unsafeGeneratedDocumentFiles.add(`${file.key}:${file.id}`)
1529+
return
15281530
}
15291531
}
15301532
},
@@ -1536,7 +1538,9 @@ export class AgentBlockHandler implements BlockHandler {
15361538
})
15371539
)
15381540

1539-
const modelSafeHydratedFiles = hydratedFiles.map((file, fileIndex) => {
1541+
const modelSafeHydratedFiles = hydratedFiles.flatMap((file, fileIndex) => {
1542+
if (unsafeGeneratedDocumentFiles.has(`${file.key}:${file.id}`)) return []
1543+
15401544
const sourceFile = message.files?.[fileIndex]
15411545
const nameProjection = sourceFile ? projectedNameByFile.get(sourceFile) : undefined
15421546
if (
@@ -1545,20 +1549,30 @@ export class AgentBlockHandler implements BlockHandler {
15451549
largeFilePathAvailable: canUseProviderLargeFilePath(providerId),
15461550
})
15471551
) {
1548-
return file
1552+
return [file]
15491553
}
15501554

15511555
if (nameProjection.inputPath) modelBoundInputPaths.push(nameProjection.inputPath)
15521556
const extension = getFileExtension(file.name)
15531557
const suffix = extension ? `.${extension}` : ''
15541558
const keepsSuffix =
15551559
suffix !== '' && nameProjection.name.toLowerCase().endsWith(suffix.toLowerCase())
1556-
return {
1557-
...file,
1558-
name:
1559-
suffix !== '' && !keepsSuffix ? `${nameProjection.name}${suffix}` : nameProjection.name,
1560-
}
1560+
return [
1561+
{
1562+
...file,
1563+
name:
1564+
suffix !== '' && !keepsSuffix
1565+
? `${nameProjection.name}${suffix}`
1566+
: nameProjection.name,
1567+
},
1568+
]
15611569
})
1570+
if (modelSafeHydratedFiles.length !== hydratedFiles.length) {
1571+
logger.warn('Omitting generated document attachments with unsafe contributor provenance', {
1572+
omittedCount: hydratedFiles.length - modelSafeHydratedFiles.length,
1573+
attachmentCount: hydratedFiles.length,
1574+
})
1575+
}
15621576

15631577
const missingFile = modelSafeHydratedFiles.find(
15641578
(file) =>
@@ -1591,8 +1605,13 @@ export class AgentBlockHandler implements BlockHandler {
15911605
)
15921606
}
15931607

1608+
const omittedCount = hydratedFiles.length - modelSafeHydratedFiles.length
15941609
nextMessages[messageIndex] = {
15951610
...message,
1611+
content:
1612+
omittedCount > 0
1613+
? appendUnavailableAttachmentNotice(message.content, omittedCount)
1614+
: message.content,
15961615
files: modelSafeHydratedFiles,
15971616
}
15981617
}

‎apps/sim/lib/copilot/request/lifecycle/run.test.ts‎

Lines changed: 71 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,6 @@ vi.mock('@/lib/copilot/application/load-search-integrations', () => ({
5050
}))
5151

5252
vi.mock('@/lib/uploads/contexts/workspace/workspace-file-secret-provenance', () => ({
53-
MODEL_UNSAFE_WORKSPACE_FILE_ERROR_MESSAGE:
54-
'File cannot be sent to a model because its secret provenance is unavailable',
5553
filterModelSafeWorkspaceFileAttachments: (...args: unknown[]) =>
5654
mockFilterModelSafeWorkspaceFileAttachments(...args),
5755
}))
@@ -673,13 +671,22 @@ describe('runCopilotLifecycle', () => {
673671
{ key: 'fileAttachments', includeSafeFile: false },
674672
{ key: 'fileAttachments', includeSafeFile: true },
675673
])(
676-
'rejects refused initial $key before the Go request (mixed=$includeSafeFile)',
674+
'continues with an error notice for refused $key (mixed=$includeSafeFile)',
677675
async ({ key, includeSafeFile }) => {
678-
const unsafe = { id: 'wf-unsafe', name: 'unsafe.txt', key: 'workspace/ws-1/unsafe.txt' }
676+
const unsafe = {
677+
id: 'wf-private',
678+
name: 'private-filename.txt',
679+
key: 'private-storage-key',
680+
base64: 'private-bytes',
681+
}
679682
const safe = { id: 'wf-safe', name: 'safe.txt', key: 'workspace/ws-1/safe.txt' }
680683
const safeFiles = includeSafeFile ? [safe] : []
681684
mockFilterModelSafeWorkspaceFileAttachments.mockResolvedValueOnce(safeFiles)
682685
const onError = vi.fn()
686+
mockRunStreamLoop.mockImplementationOnce(async (_url, _request, context) => {
687+
context.accumulatedContent = 'I can continue with the available inputs.'
688+
context.completionStatus = MothershipStreamV1CompletionStatus.complete
689+
})
683690
const payload = {
684691
message: 'Review files',
685692
[key]: [...safeFiles, unsafe],
@@ -695,15 +702,71 @@ describe('runCopilotLifecycle', () => {
695702
onError,
696703
})
697704

698-
const message = 'File cannot be sent to a model because its secret provenance is unavailable'
699-
expect(result).toMatchObject({ success: false, error: message })
700-
expect(onError).toHaveBeenCalledWith(expect.objectContaining({ message }), result)
705+
expect(result).toMatchObject({
706+
success: true,
707+
content: 'I can continue with the available inputs.',
708+
})
709+
expect(onError).not.toHaveBeenCalled()
710+
expect(mockRunStreamLoop).toHaveBeenCalledOnce()
711+
const sent = JSON.parse(String(mockRunStreamLoop.mock.calls[0][1].body))
712+
expect(sent.message).toMatch(
713+
/^Review files\n\nAttachment error: 1 requested file attachment was not provided/
714+
)
715+
expect(sent[key] ?? []).toEqual(safeFiles)
716+
expect(JSON.stringify(sent)).not.toContain('private-')
701717
expect(mockFilterModelSafeWorkspaceFileAttachments).toHaveBeenCalledWith(
702718
[...safeFiles, unsafe],
703719
{ workspaceId: 'ws-1' }
704720
)
705721
expect(payload).toEqual(originalPayload)
706-
expect(mockRunStreamLoop).not.toHaveBeenCalled()
722+
}
723+
)
724+
725+
it.each(['messages', 'both', 'attachment-only', 'system-only'])(
726+
'reports combined attachment refusals in %s payloads without changing history',
727+
async (shape) => {
728+
const history = {
729+
role: 'assistant',
730+
content: 'Previous response',
731+
tool_calls: [{ id: 'existing-call' }],
732+
}
733+
const messages =
734+
shape === 'system-only'
735+
? [{ role: 'system', content: 'System context' }]
736+
: [history, { role: 'user', content: 'Review files' }]
737+
const payload = {
738+
...(shape === 'both' ? { message: 'Review files' } : {}),
739+
...(shape === 'attachment-only' ? {} : { messages }),
740+
attachments: [{ key: 'private-first-file' }],
741+
fileAttachments: [{ key: 'private-second-file' }],
742+
}
743+
const original = structuredClone(payload)
744+
mockFilterModelSafeWorkspaceFileAttachments
745+
.mockResolvedValueOnce([])
746+
.mockResolvedValueOnce([])
747+
mockRunStreamLoop.mockResolvedValueOnce(undefined)
748+
749+
const result = await runCopilotLifecycle(payload, {
750+
userId: 'user-1',
751+
workspaceId: 'ws-1',
752+
executionContext: { userId: 'user-1', workflowId: '', workspaceId: 'ws-1' },
753+
})
754+
755+
expect(result.success).toBe(true)
756+
const sent = JSON.parse(String(mockRunStreamLoop.mock.calls[0][1].body))
757+
const notice = 'Attachment error: 2 requested file attachments were not provided'
758+
if (shape === 'both' || shape === 'attachment-only') expect(sent.message).toContain(notice)
759+
if (shape !== 'attachment-only') {
760+
expect(sent.messages[0]).toEqual(messages[0])
761+
expect(sent.messages.at(-1)).toMatchObject({
762+
role: 'user',
763+
content: expect.stringContaining(notice),
764+
})
765+
}
766+
expect(sent).not.toHaveProperty('attachments')
767+
expect(sent).not.toHaveProperty('fileAttachments')
768+
expect(JSON.stringify(sent)).not.toContain('private-')
769+
expect(payload).toEqual(original)
707770
}
708771
)
709772

0 commit comments

Comments
 (0)