Skip to content

Commit e3de9a0

Browse files
committed
fix(files): resolve a named append inside the chosen folder
cubic raised this twice; I deferred it once and it deserved the fix. Append resolves a picked file by canonical id, where the folder beside it is redundant. The advanced entry supplies a name instead, and a name is only unique within a folder — a workspace-wide lookup takes the oldest match anywhere, so the folder sat next to the field looking like it scoped the operation while doing nothing. That is the same objection that had me remove this field earlier in the branch, reappearing in advanced mode only. The rule is now stated properly: the folder travels when it is what identifies the file, and stays behind when the id already does. A named append expands the chosen folder through the authorized use case, matches the name inside it, and resolves by the id it finds — so no path-shaped reference is ever built and the slash-in-a-folder-name hazard cannot arise on this path. A name that is not in the folder refuses rather than reaching for a same-named file elsewhere. Tests cover the case that motivated it: two files named notes.md in different folders, appending to the one in the folder that was picked.
1 parent d7388c6 commit e3de9a0

5 files changed

Lines changed: 160 additions & 8 deletions

File tree

apps/sim/blocks/blocks/file-folders.test.ts

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,12 @@ describe('file_v5 folder operations produce contract-valid tool input', () => {
190190
* target. The earlier version of this field was removed precisely because
191191
* it looked like it scoped the picker without doing so; now it does.
192192
*/
193-
it('sends no folder on append even when one narrows the picker', () => {
193+
/*
194+
* A picked file is a canonical id and already exact, so the folder beside
195+
* it would be a second constraint on one target. A typed name is not
196+
* exact — that case is the next test.
197+
*/
198+
it('sends no folder when append resolved the file by id', () => {
194199
const params = paramsFor('file_append', {
195200
appendFileInput: { id: 'wf_abc', name: 'notes.md' },
196201
appendContent: 'more',
@@ -201,6 +206,42 @@ describe('file_v5 folder operations produce contract-valid tool input', () => {
201206
expect(params.folderPaths).toBeUndefined()
202207
})
203208

209+
/*
210+
* The advanced entry supplies a name, and a name is only unique inside a
211+
* folder — without the scope a duplicate resolves to the oldest match
212+
* anywhere in the workspace.
213+
*/
214+
it('sends the folder when append resolved the file by name', () => {
215+
const params = paramsFor('file_append', {
216+
appendFileInput: 'notes.md',
217+
appendContent: 'more',
218+
folderScopeRef: '/Reports',
219+
})
220+
221+
expect(params.folderPath).toBe('/Reports')
222+
})
223+
224+
it('carries the subfolder scope with a name-based append', () => {
225+
expect(
226+
paramsFor('file_append', {
227+
appendFileInput: 'notes.md',
228+
appendContent: 'more',
229+
folderScopeRef: '/Reports',
230+
folderIncludeSubfolders: 'false',
231+
}).includeSubfolders
232+
).toBe(false)
233+
})
234+
235+
it('still sends no folder when the pick carried an id', () => {
236+
expect(
237+
paramsFor('file_append', {
238+
appendFileInput: { id: 'wf_abc', name: 'notes.md' },
239+
appendContent: 'more',
240+
folderScopeRef: '/Reports',
241+
}).folderPath
242+
).toBeUndefined()
243+
})
244+
204245
it('appends by the picked file id, not its name', () => {
205246
expect(
206247
paramsFor('file_append', {

apps/sim/blocks/blocks/file.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1742,6 +1742,7 @@ export const FileV5Block: BlockConfig<FileParserV3Output> = {
17421742
}
17431743

17441744
let fileName: string
1745+
let resolvedById = false
17451746
if (typeof appendInput === 'string') {
17461747
fileName = appendInput.trim()
17471748
} else {
@@ -1755,15 +1756,30 @@ export const FileV5Block: BlockConfig<FileParserV3Output> = {
17551756
* file.
17561757
*/
17571758
const pickedId = typeof file?.id === 'string' ? file.id : ''
1759+
resolvedById = Boolean(pickedId)
17581760
fileName = pickedId || ((file?.name as string) ?? '')
17591761
}
17601762

17611763
if (!fileName) {
17621764
throw new Error('Could not determine file name')
17631765
}
17641766

1767+
/*
1768+
* The folder travels only when the name is what identifies the file.
1769+
* A picked file is a canonical id and already exact, so sending the
1770+
* folder beside it would imply a second constraint on one target.
1771+
*/
1772+
const appendFolder = resolvedById ? undefined : folderScopePath(params.folderScopeRef)
17651773
return {
17661774
fileName,
1775+
...(appendFolder
1776+
? {
1777+
folderPath: appendFolder,
1778+
...(switchValue(params.folderIncludeSubfolders, true)
1779+
? {}
1780+
: { includeSubfolders: false }),
1781+
}
1782+
: {}),
17671783
content: params.appendContent,
17681784
workspaceId: params._context?.workspaceId,
17691785
}

apps/sim/lib/api/contracts/tools/file.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,13 @@ export const fileManageAppendBodySchema = z.object({
5757
operation: z.literal('append'),
5858
workspaceId: z.string().min(1).optional(),
5959
fileName: z.string({ error: 'fileName is required for append operation' }).min(1),
60+
/**
61+
* Folder the name is resolved inside. A name is only unique within a folder,
62+
* so without this a duplicate name resolves to the oldest match anywhere.
63+
* Ignored when `fileName` is already a canonical id.
64+
*/
6065
folderPath: v2FolderPathInputSchema.optional(),
66+
includeSubfolders: z.boolean().optional(),
6167
content: z.string({ error: 'content is required for append operation' }),
6268
[PRIVATE_SECRET_PROVENANCE_FIELD]: privateSecretProvenanceBundleSchema.optional(),
6369
})

apps/sim/lib/internal/file/operations.test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,11 @@ describe('file manage folder wiring', () => {
337337
key: 'workspace/workspace-1/new.txt',
338338
url: '/api/files/serve/new-file',
339339
})
340+
mockResolveWorkspaceFileReference.mockImplementation(
341+
async ({ reference }: { reference: string }) => workspaceFile(reference)
342+
)
343+
mockFetchWorkspaceFileBuffer.mockResolvedValue(Buffer.from('before'))
344+
mockUpdateWorkspaceFileContent.mockResolvedValue({ file: workspaceFile('file-1') })
340345
})
341346

342347
it('expands a folder-only read instead of rejecting it', async () => {
@@ -399,6 +404,60 @@ describe('file manage folder wiring', () => {
399404
)
400405
})
401406

407+
/*
408+
* The case that motivated this: two files share a name in different folders,
409+
* and a typed name alone resolves to the oldest match anywhere. The folder is
410+
* the only thing telling them apart.
411+
*/
412+
it('appends to the file in the chosen folder, not a same-named file elsewhere', async () => {
413+
mockListAllWorkspaceFiles.mockResolvedValue({
414+
files: [
415+
{ ...workspaceFile('notes-elsewhere'), name: 'notes.md', folderId: null },
416+
{ ...workspaceFile('notes-in-reports'), name: 'notes.md', folderId: 'folder-reports' },
417+
],
418+
})
419+
mockGetWorkspaceFile.mockImplementation(async (_ws: string, fileId: string) => ({
420+
...workspaceFile(fileId),
421+
name: 'notes.md',
422+
}))
423+
424+
await POST(
425+
createMockRequest('POST', {
426+
operation: 'append',
427+
workspaceId: 'workspace-1',
428+
fileName: 'notes.md',
429+
folderPath: '/Reports',
430+
content: 'more',
431+
})
432+
)
433+
434+
// Resolved by the id found inside the folder, never by the bare name.
435+
expect(mockResolveWorkspaceFileReference).toHaveBeenCalledWith(
436+
'workspace-1',
437+
'notes-in-reports'
438+
)
439+
})
440+
441+
it('refuses rather than appending to a same-named file outside the folder', async () => {
442+
mockListAllWorkspaceFiles.mockResolvedValue({
443+
files: [{ ...workspaceFile('notes-elsewhere'), name: 'notes.md', folderId: null }],
444+
})
445+
446+
const response = await POST(
447+
createMockRequest('POST', {
448+
operation: 'append',
449+
workspaceId: 'workspace-1',
450+
fileName: 'notes.md',
451+
folderPath: '/Reports',
452+
content: 'more',
453+
})
454+
)
455+
const body = await response.json()
456+
457+
expect(body.success).toBe(false)
458+
expect(String(body.error)).toContain('No file named notes.md in /Reports')
459+
})
460+
402461
it('lists what a folder holds, folders and files together', async () => {
403462
const response = await POST(
404463
createMockRequest('POST', { operation: 'list', workspaceId: 'workspace-1' })

apps/sim/lib/internal/file/operations.ts

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -578,12 +578,12 @@ export function fileContentJsonResponse(
578578
* Path resolution itself lives in {@link resolveFolderIdsForPaths}; this is the
579579
* IO around it.
580580
*/
581-
async function expandFolderPathsToFileIds(args: {
581+
async function expandFolderPathsToFiles(args: {
582582
principal: Principal
583583
workspaceId: string
584584
folderPaths: string[] | undefined
585585
includeSubfolders: boolean | undefined
586-
}): Promise<string[]> {
586+
}): Promise<Array<{ id: string; name: string; folderId?: string | null }>> {
587587
if (!args.folderPaths?.length) return []
588588

589589
const [{ folders }, { files }] = await Promise.all([
@@ -609,9 +609,16 @@ async function expandFolderPathsToFileIds(args: {
609609
throw new OrchestrationError('not_found', `Folder not found: ${selection.missingPath}`)
610610
}
611611

612-
return files
613-
.filter((file) => file.folderId && selection.folderIds.has(file.folderId))
614-
.map((file) => file.id)
612+
return files.filter((file) => file.folderId && selection.folderIds.has(file.folderId))
613+
}
614+
615+
async function expandFolderPathsToFileIds(args: {
616+
principal: Principal
617+
workspaceId: string
618+
folderPaths: string[] | undefined
619+
includeSubfolders: boolean | undefined
620+
}): Promise<string[]> {
621+
return (await expandFolderPathsToFiles(args)).map((file) => file.id)
615622
}
616623

617624
export async function executeFileManageOperation(
@@ -1187,14 +1194,37 @@ export async function executeFileManageOperation(
11871194
}
11881195

11891196
case 'append': {
1190-
const { fileName, content } = body
1197+
const { fileName, content, folderPath, includeSubfolders } = body
11911198
signal?.throwIfAborted()
11921199

1200+
/*
1201+
* A picked file arrives as a canonical id, which is already exact. A
1202+
* typed name is not: the same name can exist in several folders, and a
1203+
* workspace-wide lookup takes the oldest match anywhere. When a folder
1204+
* was chosen it is the only thing disambiguating the target, so the
1205+
* name is resolved inside it — by id, so the slash-in-a-folder-name
1206+
* hazard of a path-shaped reference never arises.
1207+
*/
1208+
let scopedReference = fileName
1209+
if (folderPath && !fileName.startsWith('wf_')) {
1210+
const scoped = await expandFolderPathsToFiles({
1211+
principal,
1212+
workspaceId,
1213+
folderPaths: [folderPath],
1214+
includeSubfolders,
1215+
})
1216+
const match = scoped.find((file) => file.name === fileName)
1217+
if (!match) {
1218+
throw new OrchestrationError('not_found', `No file named ${fileName} in ${folderPath}`)
1219+
}
1220+
scopedReference = match.id
1221+
}
1222+
11931223
const existing = await resolveWorkspaceFileReference({
11941224
principal,
11951225
operation: fileOperations.updateContent,
11961226
workspaceId,
1197-
reference: fileName,
1227+
reference: scopedReference,
11981228
})
11991229

12001230
const lockKey = `file-append:${workspaceId}:${existing.id}`

0 commit comments

Comments
 (0)