Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion apps/sim/lib/copilot/chat/payload.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,12 @@ describe('buildCopilotRequestPayload', () => {
workspaceId: 'ws-1',
chatId: 'chat-1',
fileAttachments: [
{ id: 'a1', key: 'workspace/ws-1/1731000000000-ab12cd34-payroll.xlsx', size: 1 },
{
id: 'a1',
key: 'workspace/ws-1/1731000000000-ab12cd34-payroll.xlsx',
filename: 'payroll.xlsx',
size: 1,
},
],
}

Expand Down Expand Up @@ -350,6 +355,39 @@ describe('buildCopilotRequestPayload', () => {
'msg-1'
)
})

it('includes successfully prepared attachments in the model context', async () => {
const payload = await buildCopilotRequestPayload(
{ ...attachmentParams, userPermission: 'write' },
{ selectedModel: 'claude-opus-4-8' }
)

expect(payload.context).toEqual([
{
type: 'uploaded_file',
content: [
'File "payroll.xlsx" (application/octet-stream, 1 bytes) uploaded.',
'Read with: read("uploads/payroll.xlsx")',
'To save permanently: materialize_file(fileName: "payroll.xlsx")',
].join('\n'),
},
])
})

it('fails the request when an authorized attachment cannot be prepared', async () => {
const cause = new Error('provenance sidecar unavailable')
mockTrackChatUpload.mockRejectedValueOnce(cause)

await expect(
buildCopilotRequestPayload(
{ ...attachmentParams, userPermission: 'write' },
{ selectedModel: 'claude-opus-4-8' }
)
).rejects.toMatchObject({
message: 'Failed to prepare attached file "payroll.xlsx" for Copilot. Please try again.',
cause,
})
})
})

it('passes workspaceContext through to the Go request payload', async () => {
Expand Down
7 changes: 6 additions & 1 deletion apps/sim/lib/copilot/chat/payload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,11 +395,16 @@ export async function buildCopilotRequestPayload(
content: lines.join('\n'),
})
} catch (err) {
const cause = toError(err)
logger.warn('Failed to track chat upload', {
filename,
chatId,
error: toError(err).message,
error: cause.message,
})
throw new Error(
Comment on lines 401 to +404

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Attachment failure leaves persisted turn

When trackChatUpload rejects, this new exception aborts the request after persistence of the user message has already begun, causing the client to receive an HTTP 500 while the chat retains a user turn with no assistant response; retrying can then create duplicate user turns.

`Failed to prepare attached file "${filename}" for Copilot. Please try again.`,
{ cause }
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prepare failure leaves chat incomplete

Medium Severity

When trackChatUpload fails, the new rethrow aborts after persistUserMessage has already saved the user turn. For workspace chats that also publish started, the request returns 500 without terminal status or assistant finalization, so the chat can keep an attachment-bearing user message and no completed turn.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e698b10. Configure here.

}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ describe('workspace file secret provenance', () => {
)
})

it('treats only canonically bound untouched legacy files as model-safe without a sidecar', async () => {
it('classifies attachments by their canonical storage key without requiring a database file id', async () => {
queueTableRows(workspaceFiles, [
{
id: 'safe-id',
Expand Down Expand Up @@ -219,9 +219,11 @@ describe('workspace file secret provenance', () => {

const attachments = [
{ id: 'safe-id', key: 'safe-key' },
{ key: 'safe-key' },
{ id: 'file-1700000000000', key: 'safe-key' },
{ id: 'tracked-no-sidecar-id', key: 'tracked-no-sidecar-key' },
{ id: 'wrong-id', key: 'safe-key' },
{ id: 'tainted-id', key: 'tainted-key' },
{ id: 'safe-id', key: 'tainted-key' },
{ id: 'unknown-id', key: 'unknown-key' },
{ id: 'other-workspace-id', key: 'other-workspace-key' },
{ id: 'pre-marker-sidecar-id', key: 'pre-marker-sidecar-key' },
Expand All @@ -234,6 +236,9 @@ describe('workspace file secret provenance', () => {
filterModelSafeWorkspaceFileAttachments(attachments, { workspaceId: 'workspace-1' })
).resolves.toEqual([
{ id: 'safe-id', key: 'safe-key' },
{ key: 'safe-key' },
{ id: 'file-1700000000000', key: 'safe-key' },
{ id: 'wrong-id', key: 'safe-key' },
{ id: 'pre-marker-sidecar-id', key: 'pre-marker-sidecar-key' },
{ id: 'synthetic-execution-id', key: 'untracked-context-key' },
{ id: 'legacy-id', key: 'legacy-key' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ export interface WorkspaceFileSecretProvenanceEnvelope<T> {
}

interface ModelSafeWorkspaceFileRow {
id: string
key: string
workspaceId: string | null
context: string
Expand Down Expand Up @@ -543,9 +542,12 @@ export async function importWorkspaceFileSecretProvenanceForRuntime(args: {
}

/**
* Removes model attachments whose canonical workspace-file record is tainted, unknown, or does
* not match the supplied file id. Missing legacy records remain compatible; any persisted record
* is classified exclusively by its trusted key/id binding and private provenance row.
* Removes model attachments whose canonical workspace-file record is tainted or unknown.
* Missing legacy records remain compatible; persisted records are classified by their unique
* active storage-key binding and private provenance row. Attachment ids are deliberately ignored:
* older persisted workflows omit them and file normalization may synthesize a runtime-only id.
* This classification is not file authorization; callers still enforce storage access before
* reading bytes or issuing a provider URL.
*/
export async function filterModelSafeWorkspaceFileAttachments<
TAttachment extends WorkspaceFileAttachmentIdentity,
Expand All @@ -567,32 +569,14 @@ export async function filterModelSafeWorkspaceFileAttachments<
]
if (keys.length === 0) return [...attachments]

const rows = await db
.select({
id: workspaceFiles.id,
key: workspaceFiles.key,
workspaceId: workspaceFiles.workspaceId,
context: workspaceFiles.context,
fileContentUpdatedAt: workspaceFiles.contentUpdatedAt,
secretProvenanceVersion: workspaceFiles.secretProvenanceVersion,
provenanceContentUpdatedAt: workspaceFileSecretProvenance.contentUpdatedAt,
status: workspaceFileSecretProvenance.status,
entries: workspaceFileSecretProvenance.entries,
})
.from(workspaceFiles)
.leftJoin(
workspaceFileSecretProvenance,
eq(workspaceFileSecretProvenance.fileId, workspaceFiles.id)
)
.where(and(inArray(workspaceFiles.key, keys), isNull(workspaceFiles.deletedAt)))
const rows = await loadModelSafeWorkspaceFileRows(keys)

const rowByKey = new Map(rows.map((row) => [row.key, row]))
return attachments.filter((attachment) => {
if (typeof attachment.key !== 'string' || attachment.key.length === 0) return true
const row = rowByKey.get(attachment.key)
if (!row) return true
if (row.context !== 'workspace' && row.context !== 'mothership') return true
if (typeof attachment.id !== 'string' || attachment.id !== row.id) return false
return isModelSafeWorkspaceFileRow(row, options.workspaceId)
})
}
Expand All @@ -614,6 +598,28 @@ function isModelSafeWorkspaceFileRow(
return row.entries.length === 0
}

async function loadModelSafeWorkspaceFileRows(
keys: readonly string[]
): Promise<ModelSafeWorkspaceFileRow[]> {
return db
.select({
key: workspaceFiles.key,
workspaceId: workspaceFiles.workspaceId,
context: workspaceFiles.context,
fileContentUpdatedAt: workspaceFiles.contentUpdatedAt,
secretProvenanceVersion: workspaceFiles.secretProvenanceVersion,
provenanceContentUpdatedAt: workspaceFileSecretProvenance.contentUpdatedAt,
status: workspaceFileSecretProvenance.status,
entries: workspaceFileSecretProvenance.entries,
})
.from(workspaceFiles)
.leftJoin(
workspaceFileSecretProvenance,
eq(workspaceFileSecretProvenance.fileId, workspaceFiles.id)
)
.where(and(inArray(workspaceFiles.key, [...keys]), isNull(workspaceFiles.deletedAt)))
}

/**
* Verifies a server-authorized storage key before its bytes or signed URL cross a model boundary.
* Unlike attachment filtering, the key has already passed access control, so no caller-provided
Expand Down Expand Up @@ -642,24 +648,7 @@ export async function areModelSafeWorkspaceFileKeys(
throw new Error('Too many file keys to verify secret provenance')
}

const rows = await db
.select({
id: workspaceFiles.id,
key: workspaceFiles.key,
workspaceId: workspaceFiles.workspaceId,
context: workspaceFiles.context,
fileContentUpdatedAt: workspaceFiles.contentUpdatedAt,
secretProvenanceVersion: workspaceFiles.secretProvenanceVersion,
provenanceContentUpdatedAt: workspaceFileSecretProvenance.contentUpdatedAt,
status: workspaceFileSecretProvenance.status,
entries: workspaceFileSecretProvenance.entries,
})
.from(workspaceFiles)
.leftJoin(
workspaceFileSecretProvenance,
eq(workspaceFileSecretProvenance.fileId, workspaceFiles.id)
)
.where(and(inArray(workspaceFiles.key, uniqueKeys), isNull(workspaceFiles.deletedAt)))
const rows = await loadModelSafeWorkspaceFileRows(uniqueKeys)

return rows.every(
(row) =>
Expand Down
Loading