Skip to content

Commit c09e0a0

Browse files
fix(copilot): disambiguate VFS upload paths to prevent stale-row reads (#4454)
* fix(copilot): disambiguate VFS upload paths to prevent stale-row reads
1 parent a9e9ecf commit c09e0a0

11 files changed

Lines changed: 15766 additions & 45 deletions

File tree

apps/sim/lib/copilot/chat/payload.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ export async function buildCopilotRequestPayload(
238238
const filename = (f.filename ?? f.name ?? 'file') as string
239239
const mediaType = (f.media_type ?? f.mimeType ?? 'application/octet-stream') as string
240240
try {
241-
await trackChatUpload(
241+
const { displayName } = await trackChatUpload(
242242
params.workspaceId,
243243
userId,
244244
chatId,
@@ -248,13 +248,13 @@ export async function buildCopilotRequestPayload(
248248
f.size
249249
)
250250
const lines = [
251-
`File "${filename}" (${mediaType}, ${f.size} bytes) uploaded.`,
252-
`Read with: read("uploads/${filename}")`,
253-
`To save permanently: materialize_file(fileName: "${filename}")`,
251+
`File "${displayName}" (${mediaType}, ${f.size} bytes) uploaded.`,
252+
`Read with: read("uploads/${displayName}")`,
253+
`To save permanently: materialize_file(fileName: "${displayName}")`,
254254
]
255-
if (filename.endsWith('.json')) {
255+
if (displayName.endsWith('.json')) {
256256
lines.push(
257-
`To import as a workflow: materialize_file(fileName: "${filename}", operation: "import")`
257+
`To import as a workflow: materialize_file(fileName: "${displayName}", operation: "import")`
258258
)
259259
}
260260
uploadContexts.push({

apps/sim/lib/copilot/tools/handlers/materialize-file.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ function toFileRecord(row: typeof workspaceFiles.$inferSelect) {
2121
return {
2222
id: row.id,
2323
workspaceId: row.workspaceId || '',
24-
name: row.originalName,
24+
name: row.displayName ?? row.originalName,
2525
key: row.key,
2626
path: `${pathPrefix}${encodeURIComponent(row.key)}?context=mothership`,
2727
size: row.size,
@@ -45,7 +45,7 @@ async function executeSave(fileName: string, chatId: string): Promise<ToolCallRe
4545

4646
const [updated] = await db
4747
.update(workspaceFiles)
48-
.set({ context: 'workspace', chatId: null })
48+
.set({ context: 'workspace', chatId: null, originalName: row.displayName ?? row.originalName })
4949
.where(and(eq(workspaceFiles.id, row.id), isNull(workspaceFiles.deletedAt)))
5050
.returning({ id: workspaceFiles.id, originalName: workspaceFiles.originalName })
5151

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
5+
import { dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
vi.mock('@sim/db', () => dbChainMock)
9+
10+
const { mockReadFileRecord } = vi.hoisted(() => ({
11+
mockReadFileRecord: vi.fn(),
12+
}))
13+
14+
vi.mock('@/lib/copilot/vfs/file-reader', () => ({
15+
readFileRecord: mockReadFileRecord,
16+
}))
17+
18+
import {
19+
findMothershipUploadRowByChatAndName,
20+
listChatUploads,
21+
readChatUpload,
22+
} from './upload-file-reader'
23+
24+
const CHAT_ID = '11111111-1111-1111-1111-111111111111'
25+
const NOW = new Date('2026-05-05T00:00:00.000Z')
26+
27+
function makeRow(overrides: Partial<Record<string, unknown>> = {}) {
28+
return {
29+
id: 'wf_1',
30+
key: 'mothership/abc/123-image.png',
31+
userId: 'user_1',
32+
workspaceId: 'ws_1',
33+
context: 'mothership',
34+
chatId: CHAT_ID,
35+
originalName: 'image.png',
36+
displayName: 'image.png',
37+
contentType: 'image/png',
38+
size: 1024,
39+
deletedAt: null,
40+
uploadedAt: NOW,
41+
updatedAt: NOW,
42+
...overrides,
43+
}
44+
}
45+
46+
/**
47+
* Resolver chain is `.where().orderBy(...).limit(1)`. The default chain mock makes
48+
* `orderBy` a terminal, so we wire a chainable `{limit}` for each call manually.
49+
*/
50+
function mockOrderByThenLimit(rows: unknown) {
51+
dbChainMockFns.orderBy.mockReturnValueOnce({ limit: dbChainMockFns.limit } as never)
52+
dbChainMockFns.limit.mockResolvedValueOnce(rows as never)
53+
}
54+
55+
describe('findMothershipUploadRowByChatAndName', () => {
56+
beforeEach(() => {
57+
vi.clearAllMocks()
58+
resetDbChainMock()
59+
})
60+
61+
it('matches by displayName for the first occurrence', async () => {
62+
const row = makeRow({ id: 'wf_1', displayName: 'image.png' })
63+
mockOrderByThenLimit([row])
64+
65+
const result = await findMothershipUploadRowByChatAndName(CHAT_ID, 'image.png')
66+
67+
expect(result).toEqual(row)
68+
})
69+
70+
it('matches by suffixed displayName for collision-disambiguated rows', async () => {
71+
const row = makeRow({ id: 'wf_2', displayName: 'image (2).png' })
72+
mockOrderByThenLimit([row])
73+
74+
const result = await findMothershipUploadRowByChatAndName(CHAT_ID, 'image (2).png')
75+
76+
expect(result?.id).toBe('wf_2')
77+
expect(result?.displayName).toBe('image (2).png')
78+
})
79+
80+
it('prefers the most recent row when legacy rows share the same originalName', async () => {
81+
// Pre-displayName legacy rows have displayName=null. Resolver's ORDER BY uploaded_at
82+
// DESC ensures the newest upload wins, fixing read("uploads/<name>") for legacy data.
83+
const newer = makeRow({
84+
id: 'wf_new',
85+
displayName: null,
86+
originalName: 'image.png',
87+
uploadedAt: new Date('2026-05-05T12:00:00.000Z'),
88+
})
89+
mockOrderByThenLimit([newer])
90+
91+
const result = await findMothershipUploadRowByChatAndName(CHAT_ID, 'image.png')
92+
93+
expect(result?.id).toBe('wf_new')
94+
})
95+
96+
it('returns null when no row matches and the fallback scan is empty', async () => {
97+
// First query: .where().orderBy().limit() returns [].
98+
mockOrderByThenLimit([])
99+
// Second query: .where().orderBy(...) (no .limit) — orderBy is the terminal.
100+
dbChainMockFns.orderBy.mockResolvedValueOnce([] as never)
101+
102+
const result = await findMothershipUploadRowByChatAndName(CHAT_ID, 'missing.png')
103+
104+
expect(result).toBeNull()
105+
})
106+
107+
it('falls back to normalized segment match when exact lookup misses (macOS U+202F)', async () => {
108+
// Model passes ASCII space; DB row was saved with U+202F (narrow no-break space).
109+
const macosName = 'Screenshot 2026-05-05 at 9.41.00 AM.png'
110+
const asciiName = 'Screenshot 2026-05-05 at 9.41.00 AM.png'
111+
const row = makeRow({ id: 'wf_3', displayName: macosName })
112+
113+
mockOrderByThenLimit([])
114+
dbChainMockFns.orderBy.mockResolvedValueOnce([row] as never)
115+
116+
const result = await findMothershipUploadRowByChatAndName(CHAT_ID, asciiName)
117+
118+
expect(result?.id).toBe('wf_3')
119+
})
120+
})
121+
122+
describe('listChatUploads', () => {
123+
beforeEach(() => {
124+
vi.clearAllMocks()
125+
resetDbChainMock()
126+
})
127+
128+
it('returns rows in upload order with name set to displayName', async () => {
129+
const rows = [
130+
makeRow({ id: 'a', displayName: 'image.png' }),
131+
makeRow({ id: 'b', displayName: 'image (2).png' }),
132+
makeRow({ id: 'c', displayName: 'image (3).png' }),
133+
]
134+
dbChainMockFns.orderBy.mockResolvedValueOnce(rows)
135+
136+
const result = await listChatUploads(CHAT_ID)
137+
138+
expect(result.map((r) => r.id)).toEqual(['a', 'b', 'c'])
139+
expect(result.map((r) => r.name)).toEqual(['image.png', 'image (2).png', 'image (3).png'])
140+
expect(result.every((r) => r.storageContext === 'mothership')).toBe(true)
141+
})
142+
143+
it('returns [] and does not throw when the DB query fails', async () => {
144+
dbChainMockFns.orderBy.mockRejectedValueOnce(new Error('boom'))
145+
const result = await listChatUploads(CHAT_ID)
146+
expect(result).toEqual([])
147+
})
148+
})
149+
150+
describe('readChatUpload', () => {
151+
beforeEach(() => {
152+
vi.clearAllMocks()
153+
resetDbChainMock()
154+
mockReadFileRecord.mockReset()
155+
})
156+
157+
it('reads the row resolved by the suffixed displayName', async () => {
158+
const row = makeRow({ id: 'wf_2', displayName: 'image (2).png' })
159+
mockOrderByThenLimit([row])
160+
mockReadFileRecord.mockResolvedValueOnce({ content: 'PNGDATA', totalLines: 1 })
161+
162+
const result = await readChatUpload('image (2).png', CHAT_ID)
163+
164+
expect(result).toEqual({ content: 'PNGDATA', totalLines: 1 })
165+
expect(mockReadFileRecord).toHaveBeenCalledWith(
166+
expect.objectContaining({ id: 'wf_2', name: 'image (2).png', storageContext: 'mothership' })
167+
)
168+
})
169+
170+
it('returns null when no row matches', async () => {
171+
mockOrderByThenLimit([])
172+
dbChainMockFns.orderBy.mockResolvedValueOnce([] as never)
173+
174+
const result = await readChatUpload('nope.png', CHAT_ID)
175+
176+
expect(result).toBeNull()
177+
expect(mockReadFileRecord).not.toHaveBeenCalled()
178+
})
179+
})

apps/sim/lib/copilot/tools/handlers/upload-file-reader.ts

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,25 @@ import { db } from '@sim/db'
22
import { workspaceFiles } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
44
import { toError } from '@sim/utils/errors'
5-
import { and, eq, isNull } from 'drizzle-orm'
5+
import { and, asc, desc, eq, isNull, or } from 'drizzle-orm'
66
import { type FileReadResult, readFileRecord } from '@/lib/copilot/vfs/file-reader'
77
import { normalizeVfsSegment } from '@/lib/copilot/vfs/normalize-segment'
88
import { getServePathPrefix } from '@/lib/uploads'
99
import type { WorkspaceFileRecord } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
1010

1111
const logger = createLogger('UploadFileReader')
1212

13+
/** VFS-visible name. Coalesces to originalName for legacy rows that predate displayName. */
14+
function vfsName(row: typeof workspaceFiles.$inferSelect): string {
15+
return row.displayName ?? row.originalName
16+
}
17+
1318
function toWorkspaceFileRecord(row: typeof workspaceFiles.$inferSelect): WorkspaceFileRecord {
1419
const pathPrefix = getServePathPrefix()
1520
return {
1621
id: row.id,
1722
workspaceId: row.workspaceId || '',
18-
name: row.originalName,
23+
name: vfsName(row),
1924
key: row.key,
2025
path: `${pathPrefix}${encodeURIComponent(row.key)}?context=mothership`,
2126
size: row.size,
@@ -29,8 +34,14 @@ function toWorkspaceFileRecord(row: typeof workspaceFiles.$inferSelect): Workspa
2934
}
3035

3136
/**
32-
* Resolve a mothership upload row by `originalName`, preferring an exact DB match (limit 1) and
33-
* only scanning all chat uploads when that misses (e.g. macOS U+202F vs ASCII space in the name).
37+
* Resolve a mothership upload row by VFS name (the collision-disambiguated `displayName`
38+
* for new rows, or `originalName` for legacy rows that predate the column). Prefers an
39+
* exact DB match; falls back to a normalized scan when the model passes a visually
40+
* equivalent name (e.g. macOS U+202F vs ASCII space in screenshot filenames).
41+
*
42+
* On ambiguity (multiple legacy rows sharing the same originalName in one chat — the
43+
* pre-displayName collision case), returns the most recent upload. New rows are unique
44+
* by index so this only affects pre-fix data.
3445
*/
3546
export async function findMothershipUploadRowByChatAndName(
3647
chatId: string,
@@ -43,10 +54,14 @@ export async function findMothershipUploadRowByChatAndName(
4354
and(
4455
eq(workspaceFiles.chatId, chatId),
4556
eq(workspaceFiles.context, 'mothership'),
46-
eq(workspaceFiles.originalName, fileName),
57+
or(
58+
eq(workspaceFiles.displayName, fileName),
59+
and(isNull(workspaceFiles.displayName), eq(workspaceFiles.originalName, fileName))
60+
),
4761
isNull(workspaceFiles.deletedAt)
4862
)
4963
)
64+
.orderBy(desc(workspaceFiles.uploadedAt), desc(workspaceFiles.id))
5065
.limit(1)
5166

5267
if (exactRows[0]) {
@@ -63,13 +78,14 @@ export async function findMothershipUploadRowByChatAndName(
6378
isNull(workspaceFiles.deletedAt)
6479
)
6580
)
81+
.orderBy(desc(workspaceFiles.uploadedAt), desc(workspaceFiles.id))
6682

6783
const segmentKey = normalizeVfsSegment(fileName)
68-
return allRows.find((r) => normalizeVfsSegment(r.originalName) === segmentKey) ?? null
84+
return allRows.find((r) => normalizeVfsSegment(vfsName(r)) === segmentKey) ?? null
6985
}
7086

7187
/**
72-
* List all chat-scoped uploads for a given chat.
88+
* List all chat-scoped uploads for a given chat in upload order.
7389
*/
7490
export async function listChatUploads(chatId: string): Promise<WorkspaceFileRecord[]> {
7591
try {
@@ -83,6 +99,7 @@ export async function listChatUploads(chatId: string): Promise<WorkspaceFileReco
8399
isNull(workspaceFiles.deletedAt)
84100
)
85101
)
102+
.orderBy(asc(workspaceFiles.uploadedAt), asc(workspaceFiles.id))
86103

87104
return rows.map(toWorkspaceFileRecord)
88105
} catch (err) {

0 commit comments

Comments
 (0)