From 2f00eca6d81b2267b7bbf9bf6d86da4b363b9d1b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 2 Nov 2026 16:21:13 -0800 Subject: [PATCH 1/6] improvement(cli): accept the file references, list shapes and multi-file values models send files batch-delete answers to --ids and --paths, reads a JSON array or a comma-joined run of ids as its entries, and the v2 bulk delete resolves files/ VFS paths through the canonical resolver after authorization, refusing the whole batch when one path names no single live file. tables rows batch-delete answers to --row-ids and the log reads to --workflow-ids, the request fields behind their shortened flags. Workflow edits store a multi-file upload field as the list of file objects the editor writes, and an empty list clears it. A document page selection that lists separate pages says to read one range per call. --- apps/docs/content/docs/cli/files.mdx | 4 +- apps/docs/content/docs/cli/reference.mdx | 4 +- apps/docs/openapi-v2-files-audit.json | 2 +- .../sim/app/api/v2/files/bulk-delete/route.ts | 6 +- apps/sim/lib/api/contracts/v2/files.ts | 9 +- .../lib/workflows/editing/validation.test.ts | 37 ++++++ apps/sim/lib/workflows/editing/validation.ts | 23 ++-- .../file-path-references.integration.ts | 96 ++++++++++++++- .../archive-workspace-file-items.test.ts | 62 ++++++++++ .../archive-workspace-file-items.ts | 67 +++++++++-- .../application/file-context.ts | 33 +++--- apps/sim/lib/workspace-files/limits.ts | 2 + .../render-document-for-vision.test.ts | 5 + .../render-document-for-vision.ts | 6 + packages/sim-cli/src/contract/commands.ts | 11 +- packages/sim-cli/src/contract/types.ts | 19 +++ packages/sim-cli/src/generated/v2-api.ts | 7 +- packages/sim-cli/src/runtime/execute.ts | 25 ++-- .../src/runtime/file-references.test.ts | 109 ++++++++++++++++++ packages/sim-cli/src/runtime/options.ts | 29 +++-- packages/sim-cli/src/runtime/request.ts | 32 ++++- .../src/mocks/workspace-file-manager.mock.ts | 2 + 22 files changed, 521 insertions(+), 69 deletions(-) create mode 100644 packages/sim-cli/src/runtime/file-references.test.ts diff --git a/apps/docs/content/docs/cli/files.mdx b/apps/docs/content/docs/cli/files.mdx index 402b92fe5ab..959f5809299 100644 --- a/apps/docs/content/docs/cli/files.mdx +++ b/apps/docs/content/docs/cli/files.mdx @@ -643,7 +643,7 @@ sim files batch-delete [file-ids...] [options] | Argument | Required | Description | | --- | --- | --- | -| `file-ids...` | No | File identifiers to delete. Same as --file-ids. | +| `file-ids...` | No | Files to delete, each by identifier or by the VFS path of a live file: `files//`. Pass the id when two files in a folder have names that display alike. At most 100 entries may be paths. Same as --file-ids. | @@ -653,7 +653,7 @@ sim files batch-delete [file-ids...] [options] | Option | Required | Description | | --- | --- | --- | -| `--file-ids ` | No | File identifiers to delete. (space-separated, or @path / @- with one value per line; @@value for a literal leading @) (required, or pass the values as arguments). | +| `--file-ids ` | No | Files to delete, each by identifier or by the VFS path of a live file: `files//`. Pass the id when two files in a folder have names that display alike. At most 100 entries may be paths. (space-separated, or @path / @- with one value per line; @@value for a literal leading @) (required, or pass the values as arguments). | | `-y, --yes` | Yes | Confirm this operation. | diff --git a/apps/docs/content/docs/cli/reference.mdx b/apps/docs/content/docs/cli/reference.mdx index 171810846f2..b6f24256769 100644 --- a/apps/docs/content/docs/cli/reference.mdx +++ b/apps/docs/content/docs/cli/reference.mdx @@ -981,7 +981,7 @@ sim files batch-delete [file-ids...] [options] | Argument | Required | Description | | --- | --- | --- | -| `file-ids...` | No | File identifiers to delete. Same as --file-ids. | +| `file-ids...` | No | Files to delete, each by identifier or by the VFS path of a live file: `files//`. Pass the id when two files in a folder have names that display alike. At most 100 entries may be paths. Same as --file-ids. | @@ -991,7 +991,7 @@ sim files batch-delete [file-ids...] [options] | Option | Required | Description | | --- | --- | --- | -| `--file-ids ` | No | File identifiers to delete. (space-separated, or @path / @- with one value per line; @@value for a literal leading @) (required, or pass the values as arguments). | +| `--file-ids ` | No | Files to delete, each by identifier or by the VFS path of a live file: `files//`. Pass the id when two files in a folder have names that display alike. At most 100 entries may be paths. (space-separated, or @path / @- with one value per line; @@value for a literal leading @) (required, or pass the values as arguments). | | `-y, --yes` | Yes | Confirm this operation. | diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index dab0fa456b1..0b741d9516c 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -7369,7 +7369,7 @@ "type": "string", "minLength": 1 }, - "description": "File identifiers to delete." + "description": "Files to delete, each by identifier or by the VFS path of a live file: `files//`. Pass the id when two files in a folder have names that display alike. At most 100 entries may be paths." } }, "required": ["workspaceId", "fileIds"], diff --git a/apps/sim/app/api/v2/files/bulk-delete/route.ts b/apps/sim/app/api/v2/files/bulk-delete/route.ts index 35ae13cfe31..7c2aae1700a 100644 --- a/apps/sim/app/api/v2/files/bulk-delete/route.ts +++ b/apps/sim/app/api/v2/files/bulk-delete/route.ts @@ -13,7 +13,11 @@ export const POST = defineV2JsonRoute({ operation: fileOperations.delete, rateLimit: v2RateLimits.publicApi, errorPolicy: v2FileErrorPolicies.default, - mapInput: ({ body }) => ({ workspaceId: body.workspaceId, fileIds: body.fileIds }), + mapInput: ({ body }) => ({ + workspaceId: body.workspaceId, + fileIds: body.fileIds, + acceptVfsPath: true, + }), useCase: archiveWorkspaceFileItemsOperation, present: ({ deletedItems }) => ({ data: { deletedItems: { files: deletedItems.files } } }), }) diff --git a/apps/sim/lib/api/contracts/v2/files.ts b/apps/sim/lib/api/contracts/v2/files.ts index 364b6ba9633..4bf57c0f621 100644 --- a/apps/sim/lib/api/contracts/v2/files.ts +++ b/apps/sim/lib/api/contracts/v2/files.ts @@ -46,7 +46,10 @@ import { MAX_FOLDER_PATH_BYTES, MAX_FOLDER_PATH_SEGMENTS } from '@/lib/folders/p import { MAX_WORKSPACE_FILE_SIZE } from '@/lib/uploads/shared/types' import { MAX_TEXT_EXTRACTION_BYTES } from '@/lib/uploads/utils/file' import { isWorkspaceFileVfsPath } from '@/lib/vfs/path' -import { MAX_ZIP_DOWNLOAD_FILES } from '@/lib/workspace-files/limits' +import { + MAX_WORKSPACE_FILE_BULK_REQUEST_PATHS, + MAX_ZIP_DOWNLOAD_FILES, +} from '@/lib/workspace-files/limits' import { FILE_SEARCH_DEFAULT_MAX_RESULTS, FILE_SEARCH_MAX_QUERY_LENGTH, @@ -527,7 +530,9 @@ export type V2MoveFileItemsResult = z.output export const v2BulkDeleteFilesBodySchema = z .object({ workspaceId: workspaceIdSchema.describe('Workspace containing the files.'), - fileIds: fileIdsSchema.describe('File identifiers to delete.'), + fileIds: fileIdsSchema.describe( + `Files to delete, each by identifier or by the VFS path of a live file: \`files//\`. Pass the id when two files in a folder have names that display alike. At most ${MAX_WORKSPACE_FILE_BULK_REQUEST_PATHS} entries may be paths.` + ), }) .strict() diff --git a/apps/sim/lib/workflows/editing/validation.test.ts b/apps/sim/lib/workflows/editing/validation.test.ts index 916a85f3190..5b58e61fc9a 100644 --- a/apps/sim/lib/workflows/editing/validation.test.ts +++ b/apps/sim/lib/workflows/editing/validation.test.ts @@ -345,6 +345,43 @@ describe('validateInputsForBlock', () => { ).toBe(false) }) + describe('file-upload values', () => { + const multiple = { id: 'attachmentFiles', type: 'file-upload' as const, multiple: true } + const single = { id: 'file', type: 'file-upload' as const } + const stored = { + id: 'wf_a', + name: 'brief.txt', + path: '/api/files/serve/workspace%2Fws%2Fbrief.txt?context=workspace', + key: 'workspace/ws/brief.txt', + size: 12, + type: 'text/plain', + } + const validate = (config: typeof multiple | typeof single, value: unknown) => + validateValueForSubBlockType(config, value, config.id, 'agent', 'agent-1') + + it('stores a multi-file field as the list of file objects the editor writes', () => { + const files = [stored, { ...stored, id: 'wf_b', name: 'notes.txt' }] + expect(validate(multiple, files)).toEqual({ valid: true, value: files }) + }) + + it('clears a multi-file field given an empty list, as removing its last file does', () => { + expect(validate(multiple, [])).toEqual({ valid: true, value: null }) + }) + + it('names the entry of a multi-file list that is not a stored file', () => { + const result = validate(multiple, [stored, { name: 'notes.txt' }]) + expect(result.valid).toBe(false) + expect(result.error?.error).toContain('attachmentFiles[1]') + expect(validate(multiple, [stored, 'brief.txt']).valid).toBe(false) + }) + + it('keeps a single-file field to one file object', () => { + expect(validate(single, stored)).toEqual({ valid: true, value: stored }) + expect(validate(single, [stored]).valid).toBe(false) + expect(validate(single, []).valid).toBe(false) + }) + }) + it('stores input-format rows with legacy or aliased types unchanged', () => { const rows = [ { name: 'payload', type: 'json' }, diff --git a/apps/sim/lib/workflows/editing/validation.ts b/apps/sim/lib/workflows/editing/validation.ts index a816ec9f8d0..fbba172062c 100644 --- a/apps/sim/lib/workflows/editing/validation.ts +++ b/apps/sim/lib/workflows/editing/validation.ts @@ -516,6 +516,11 @@ function validateFallbackModelEntry(item: any, index: number): string | null { return null } +/** A file object as the file-upload editor stores it: at least its name and serve path. */ +function isStoredFile(value: unknown): boolean { + return isPlainRecord(value) && Boolean(value.name) && Boolean(value.path) +} + /** * Validates a value against its expected subBlock type * Returns validation result with the value or an error @@ -609,9 +614,12 @@ export function validateValueForSubBlockType( } case 'file-upload': { - // File upload should be an object with specific properties or null - if (value === null) return { valid: true, value: null } - if (typeof value !== 'object') { + // A multi-file field stores the editor's list of file objects; removing its last file + // stores null, so an empty list clears it the same way. + if (subBlockConfig.multiple && Array.isArray(value)) { + if (value.length === 0) return { valid: true, value: null } + const index = value.findIndex((file) => !isStoredFile(file)) + if (index === -1) return { valid: true, value } return { valid: false, error: { @@ -619,12 +627,11 @@ export function validateValueForSubBlockType( blockType, field: fieldName, value, - error: `Invalid file-upload value for field "${fieldName}" - expected object with name and path properties, or null`, + error: `Invalid file-upload entry ${fieldName}[${index}] - each file must be an object with "name" and "path" properties`, }, } } - // Validate file object has required properties - if (value && (!value.name || !value.path)) { + if (!isStoredFile(value)) { return { valid: false, error: { @@ -632,7 +639,9 @@ export function validateValueForSubBlockType( blockType, field: fieldName, value, - error: `Invalid file-upload object for field "${fieldName}" - must have "name" and "path" properties`, + error: subBlockConfig.multiple + ? `Invalid file-upload value for field "${fieldName}" - expected a list of objects with "name" and "path" properties, or null` + : `Invalid file-upload value for field "${fieldName}" - expected one object with "name" and "path" properties, or null`, }, } } diff --git a/apps/sim/lib/workspace-files/__integration__/file-path-references.integration.ts b/apps/sim/lib/workspace-files/__integration__/file-path-references.integration.ts index cdbcac85ec9..37d428c27be 100644 --- a/apps/sim/lib/workspace-files/__integration__/file-path-references.integration.ts +++ b/apps/sim/lib/workspace-files/__integration__/file-path-references.integration.ts @@ -1,8 +1,9 @@ /** - * Real PostgreSQL behavior of single-file operations addressed by a `files/…` VFS path instead - * of an id: the path names exactly one live file in the asserted workspace, never a same-named - * file in another folder or workspace, a chat upload or an archived file, and authorization is - * unchanged. A caller who cannot reach the workspace learns nothing about the paths in it. + * Real PostgreSQL behavior of file operations addressed by a `files/…` VFS path instead of an id: + * the path names exactly one live file in the asserted workspace, never a same-named file in + * another folder or workspace, a chat upload or an archived file, and authorization is + * unchanged. A caller who cannot reach the workspace learns nothing about the paths in it, and a + * bulk delete with one unresolved path deletes nothing. */ import { db } from '@sim/db' import { permissions, user, workspace, workspaceFiles } from '@sim/db/schema' @@ -13,6 +14,7 @@ import { eq, inArray, sql } from 'drizzle-orm' import { afterAll, beforeAll, describe, expect, it } from 'vitest' import { MAX_FOLDERS_PER_WORKSPACE } from '@/lib/folders/constants' import { workspaceFileVfsPath } from '@/lib/uploads/contexts/workspace/file-manager' +import { archiveWorkspaceFileItemsOperation } from '@/lib/workspace-files/application/archive-workspace-file-items' import { deleteWorkspaceFileOperation } from '@/lib/workspace-files/application/delete-workspace-file' import { readWorkspaceFileMetadataWithVersion } from '@/lib/workspace-files/application/read-workspace-file-metadata' import { renameWorkspaceFile } from '@/lib/workspace-files/application/rename-workspace-file' @@ -64,6 +66,12 @@ const deleteFile = (fileId: string, userId = alice) => input: { fileId, acceptVfsPath: true, assertedWorkspaceId: workspaceId }, }) +const bulkDelete = (fileIds: string[], userId = alice, acceptVfsPath = true) => + archiveWorkspaceFileItemsOperation.execute({ + principal: session(userId), + input: { workspaceId, fileIds, acceptVfsPath }, + }) + /** The `{ code, message }` each path is refused with, or null where it resolved. */ const refusalsFor = ( principal: Parameters[0]['principal'], @@ -373,4 +381,84 @@ describe('single-file operations addressed by VFS path', () => { }) expect((await row(ids.rootReport))?.deletedAt).toBeNull() }) + + describe('bulk delete', () => { + const bulk = { folder: generateId(), one: '', two: '', three: '', rootOne: '' } + + beforeAll(async () => { + await db.execute(sql` + INSERT INTO folder (id, name, user_id, workspace_id, resource_type, parent_id) + VALUES (${bulk.folder}, 'Bulk', ${alice}, ${workspaceId}, 'file', NULL)`) + bulk.one = await insertFile({ name: 'one.csv', folderId: bulk.folder }) + bulk.two = await insertFile({ name: 'two.csv', folderId: bulk.folder }) + bulk.three = await insertFile({ name: 'three.csv', folderId: bulk.folder }) + bulk.rootOne = await insertFile({ name: 'one.csv' }) + }) + + const live = async (ids: string[]) => + Promise.all(ids.map(async (id) => (await row(id))?.deletedAt === null)) + + it('refuses the whole request when one path names no single live file', async () => { + await expect( + bulkDelete(['files/Bulk/three.csv', 'files/Bulk/missing.csv']) + ).rejects.toMatchObject({ + code: 'not_found', + message: 'No live file at "files/Bulk/missing.csv"', + }) + await expect( + bulkDelete([bulk.three, 'files/Bulk/one.csv,files/Bulk/two.csv']) + ).rejects.toMatchObject({ + message: expect.stringContaining('pass each path as its own entry'), + }) + await expect(bulkDelete([bulk.three, await ambiguousPath()])).rejects.toMatchObject({ + code: 'validation', + }) + await expect(bulkDelete([bulk.three, 'files/notes.txt'])).rejects.toMatchObject({ + code: 'not_found', + }) + expect(await live([bulk.one, bulk.two, bulk.three, ids.chatUpload])).toEqual([ + true, + true, + true, + true, + ]) + }) + + it('keeps authorization and answers a non-member the same for every path', async () => { + await expect(bulkDelete(['files/Bulk/one.csv'], bob)).rejects.toMatchObject({ + code: 'forbidden', + }) + const refusal = (fileIds: string[]) => + bulkDelete(fileIds, carol).then( + () => null, + (error: { code?: string; message?: string }) => ({ + code: error.code, + message: error.message, + }) + ) + const existing = await refusal(['files/Bulk/one.csv', 'files/Bulk/two.csv']) + expect(existing).toMatchObject({ code: 'forbidden' }) + expect(await refusal(['files/Bulk/one.csv', 'files/Bulk/missing.csv'])).toEqual(existing) + expect(await live([bulk.one, bulk.two])).toEqual([true, true]) + }) + + it('resolves no path for a caller that did not opt in', async () => { + await expect(bulkDelete(['files/Bulk/one.csv'], alice, false)).rejects.toMatchObject({ + code: 'not_found', + }) + expect(await live([bulk.one])).toEqual([true]) + }) + + it('deletes the files a mix of paths and ids names, and no same-named file elsewhere', async () => { + expect( + await bulkDelete(['files/Bulk/one.csv', bulk.two, 'files/Bulk/two.csv']) + ).toMatchObject({ deletedItems: { files: 2 } }) + expect(await live([bulk.one, bulk.two, bulk.three, bulk.rootOne])).toEqual([ + false, + false, + true, + true, + ]) + }) + }) }) diff --git a/apps/sim/lib/workspace-files/application/archive-workspace-file-items.test.ts b/apps/sim/lib/workspace-files/application/archive-workspace-file-items.test.ts index c3ab15a4d15..fa1cef47912 100644 --- a/apps/sim/lib/workspace-files/application/archive-workspace-file-items.test.ts +++ b/apps/sim/lib/workspace-files/application/archive-workspace-file-items.test.ts @@ -5,6 +5,10 @@ import { import { auditMock, auditMockFns } from '@sim/testing/mocks/audit.mock' import { realtimeNotifyMock, realtimeNotifyMockFns } from '@sim/testing/mocks/realtime-notify.mock' import { workspaceAuthzMock, workspaceAuthzMockFns } from '@sim/testing/mocks/workspace-authz.mock' +import { + workspaceFileManagerMock, + workspaceFileManagerMockFns, +} from '@sim/testing/mocks/workspace-file-manager.mock' import { workspaceUploadsMock, workspaceUploadsMockFns, @@ -16,6 +20,7 @@ const { events } = vi.hoisted(() => ({ })) vi.mock('@/lib/uploads/contexts/workspace', () => workspaceUploadsMock) +vi.mock('@/lib/uploads/contexts/workspace/file-manager', () => workspaceFileManagerMock) vi.mock('@sim/platform-authz/workspace', () => workspaceAuthzMock) vi.mock('@sim/audit', () => auditMock) vi.mock('@/lib/realtime/notify', () => realtimeNotifyMock) @@ -31,6 +36,7 @@ const mockResolvePermission = workspaceAuthzMockFns.mockResolveEffectiveWorkspac const mockAssertItems = workspaceUploadsMockFns.mockAssertWorkspaceFileItemsBelongToWorkspace const mockArchive = workspaceUploadsMockFns.mockBulkArchiveWorkspaceFileItems const mockLoadContext = workspaceUploadsMockFns.mockLoadWorkspaceFileOperationContext +const mockResolveReference = workspaceFileManagerMockFns.mockResolveFileReference describe('archiveWorkspaceFileItemsOperation', () => { beforeEach(() => { @@ -57,6 +63,10 @@ describe('archiveWorkspaceFileItemsOperation', () => { fileIds: ['file-1'], folderIds: [], })) + mockResolveReference.mockImplementation(async (_owner: unknown, reference: string) => { + events.push(`lookup ${reference}`) + return reference === 'files/Docs/a.csv' ? { id: 'file-a' } : null + }) }) it('authorizes a delegated bulk selection without borrowing the first file scope', async () => { @@ -151,4 +161,56 @@ describe('archiveWorkspaceFileItemsOperation', () => { expect(events).toEqual(['resolve', 'authorize']) expect(mockArchive).not.toHaveBeenCalled() }) + + describe('with files/ paths', () => { + const deleteByReference = (fileIds: string[], acceptVfsPath = true) => + archiveWorkspaceFileItemsOperation.execute({ + principal: createSessionPrincipal(), + input: { workspaceId: 'ws-1', fileIds, acceptVfsPath }, + }) + + it('archives the file each path names, once even when its id is also listed', async () => { + await deleteByReference(['files/Docs/a.csv', 'file-a', 'file-b']) + expect(mockArchive).toHaveBeenCalledWith( + expect.objectContaining({ fileIds: ['file-a', 'file-b'] }) + ) + }) + + it('archives nothing when one path names no live file, and says which', async () => { + await expect(deleteByReference(['files/Docs/a.csv', 'files/Docs/gone.csv'])).rejects.toThrow( + 'No live file at "files/Docs/gone.csv"' + ) + await expect(deleteByReference(['files/Docs/a.csv,files/Docs/b.csv'])).rejects.toThrow( + 'pass each path as its own entry' + ) + expect(mockAssertItems).not.toHaveBeenCalled() + expect(mockArchive).not.toHaveBeenCalled() + }) + + it('looks up no path for a caller it refuses', async () => { + mockResolvePermission.mockImplementation(async () => { + events.push('authorize') + return 'read' + }) + await expect(deleteByReference(['files/Docs/a.csv'])).rejects.toMatchObject({ + code: 'forbidden', + }) + expect(events).toEqual(['resolve', 'authorize']) + }) + + it('leaves a path an unknown id for a caller that did not opt in', async () => { + await deleteByReference(['files/Docs/a.csv'], false) + expect(mockResolveReference).not.toHaveBeenCalled() + expect(mockAssertItems).toHaveBeenCalledWith( + expect.objectContaining({ fileIds: ['files/Docs/a.csv'] }) + ) + }) + + it('refuses more paths than one request resolves before looking any up', async () => { + await expect( + deleteByReference(Array.from({ length: 101 }, (_, index) => `files/f-${index}.csv`)) + ).rejects.toThrow('at most 100 file paths') + expect(mockResolveReference).not.toHaveBeenCalled() + }) + }) }) diff --git a/apps/sim/lib/workspace-files/application/archive-workspace-file-items.ts b/apps/sim/lib/workspace-files/application/archive-workspace-file-items.ts index 30e7011c8a9..1f8721b192c 100644 --- a/apps/sim/lib/workspace-files/application/archive-workspace-file-items.ts +++ b/apps/sim/lib/workspace-files/application/archive-workspace-file-items.ts @@ -7,9 +7,14 @@ import { bulkArchiveWorkspaceFileItems, loadWorkspaceFileOperationContext, } from '@/lib/uploads/contexts/workspace' +import { isWorkspaceFileVfsPath } from '@/lib/vfs/path' import { defineAuthorizedWorkspaceFileUseCase } from '@/lib/workspace-files/application/authorized-workspace-file-use-case' +import { resolveWorkspaceFileVfsPath } from '@/lib/workspace-files/application/file-context' import { fileOperations } from '@/lib/workspace-files/application/operations' -import { MAX_WORKSPACE_FILE_BULK_REQUEST_IDS } from '@/lib/workspace-files/limits' +import { + MAX_WORKSPACE_FILE_BULK_REQUEST_IDS, + MAX_WORKSPACE_FILE_BULK_REQUEST_PATHS, +} from '@/lib/workspace-files/limits' const logger = createLogger('ArchiveWorkspaceFileItems') @@ -17,6 +22,11 @@ export interface ArchiveWorkspaceFileItemsInput { workspaceId: string fileIds?: string[] folderIds?: string[] + /** + * Also accept a live file's exact `files/…` VFS path among `fileIds`. Only the public v2 + * surface sets it; every other caller passes ids, and a path there stays an unknown id. + */ + acceptVfsPath?: boolean } export interface ArchiveWorkspaceFileItemsResult { @@ -31,6 +41,47 @@ function normalizeSelection(input: ArchiveWorkspaceFileItemsInput) { } } +function namesVfsPath(input: ArchiveWorkspaceFileItemsInput, reference: string): boolean { + return input.acceptVfsPath === true && isWorkspaceFileVfsPath(reference) +} + +/** + * Turns each path among the selected files into its file's id, all before anything is archived, + * so one path that names no single live file refuses the whole request. Runs only after the + * caller is admitted to the workspace, which is what lets a refusal name what the workspace + * holds. + */ +async function resolveFileReferences( + workspaceId: string, + input: ArchiveWorkspaceFileItemsInput, + references: string[] +): Promise { + const paths = references.filter((reference) => namesVfsPath(input, reference)) + if (paths.length > MAX_WORKSPACE_FILE_BULK_REQUEST_PATHS) { + throw new OrchestrationError( + 'validation', + `Bulk file operations resolve at most ${MAX_WORKSPACE_FILE_BULK_REQUEST_PATHS} file paths per request; pass file ids for the rest` + ) + } + const resolved: string[] = [] + for (const reference of references) { + if (!namesVfsPath(input, reference)) { + resolved.push(reference) + continue + } + resolved.push( + await resolveWorkspaceFileVfsPath(workspaceId, reference).catch((error: unknown) => { + if (!(error instanceof OrchestrationError) || error.code !== 'not_found') throw error + throw new OrchestrationError( + 'not_found', + `No live file at "${reference}"${reference.includes(',') ? '; pass each path as its own entry' : ''}` + ) + }) + ) + } + return [...new Set(resolved)] +} + async function executeArchiveWorkspaceFileItems({ input, context, @@ -38,12 +89,12 @@ async function executeArchiveWorkspaceFileItems({ input: ArchiveWorkspaceFileItemsInput context: Awaited> }): Promise { - const { fileIds, folderIds } = normalizeSelection(input) - if (fileIds.length === 0 && folderIds.length === 0) { + const { fileIds: references, folderIds } = normalizeSelection(input) + if (references.length === 0 && folderIds.length === 0) { throw new OrchestrationError('validation', 'At least one file or folder must be selected') } if ( - fileIds.length > MAX_WORKSPACE_FILE_BULK_REQUEST_IDS || + references.length > MAX_WORKSPACE_FILE_BULK_REQUEST_IDS || folderIds.length > MAX_WORKSPACE_FILE_BULK_REQUEST_IDS ) { throw new OrchestrationError( @@ -51,6 +102,7 @@ async function executeArchiveWorkspaceFileItems({ `Bulk file operations accept at most ${MAX_WORKSPACE_FILE_BULK_REQUEST_IDS} file and folder IDs` ) } + const fileIds = await resolveFileReferences(context.workspaceId, input, references) await assertWorkspaceFileItemsBelongToWorkspace({ workspaceId: context.workspaceId, @@ -82,10 +134,9 @@ async function resolveArchiveContext({ input }: { input: ArchiveWorkspaceFileIte const context = await loadWorkspaceFileOperationContext(input.workspaceId) if (!context) throw new OrchestrationError('not_found', 'Workspace not found') const { fileIds, folderIds } = normalizeSelection(input) - return { - ...context, - fileId: fileIds.length === 1 && folderIds.length === 0 ? fileIds[0] : undefined, - } + const sole = fileIds.length === 1 && folderIds.length === 0 ? fileIds[0] : undefined + // A path is not yet an id here, so it can never match a delegation scoped to one file. + return { ...context, fileId: sole === undefined || namesVfsPath(input, sole) ? undefined : sole } } export const archiveWorkspaceFileItemsOperation = defineAuthorizedWorkspaceFileUseCase({ diff --git a/apps/sim/lib/workspace-files/application/file-context.ts b/apps/sim/lib/workspace-files/application/file-context.ts index 0f2474d87ec..69bc50fcf2a 100644 --- a/apps/sim/lib/workspace-files/application/file-context.ts +++ b/apps/sim/lib/workspace-files/application/file-context.ts @@ -92,28 +92,26 @@ function namesVfsPath(input: WorkspaceFileTargetInput): boolean { } /** - * A path becomes an id first, so the canonical load and every authorization check run unchanged. - * The resolver's `validation` is decided from the reference alone, before any read, and says - * nothing about the workspace. Every other refusal it raises was read from the workspace, as is a - * path that names no single live file, so each leaves as an - * {@link UnresolvedWorkspaceFileVfsPathError} that only a caller the operation admits sees. + * The id of the one live file at `vfsPath` in `workspaceId`. The resolver's `validation` is + * decided from the path alone, before any read, and says nothing about the workspace. Every other + * refusal it raises was read from the workspace, as is a path that names no single live file, so + * each leaves as an {@link UnresolvedWorkspaceFileVfsPathError}, which a caller must admit to the + * workspace before its message can be shown. */ -async function resolveFileId(input: WorkspaceFileContextInput): Promise { - if (!namesVfsPath(input)) return input.fileId - const workspaceId = input.assertedWorkspaceId - if (workspaceId === undefined) { - throw new OrchestrationError('not_found', 'File not found') - } +export async function resolveWorkspaceFileVfsPath( + workspaceId: string, + vfsPath: string +): Promise { const file = await resolveFileReference( { entityType: 'workspace', entityId: workspaceId }, - input.fileId + vfsPath ).catch((error: unknown) => { if (!(error instanceof OrchestrationError) || error.code === 'validation') throw error if (error.code === 'conflict') { throw new UnresolvedWorkspaceFileVfsPathError( workspaceId, 'validation', - `"${input.fileId}" names more than one file whose names display alike; pass the file id instead` + `"${vfsPath}" names more than one file whose names display alike; pass the file id instead` ) } throw new UnresolvedWorkspaceFileVfsPathError(workspaceId, error.code, error.message) @@ -124,6 +122,15 @@ async function resolveFileId(input: WorkspaceFileContextInput): Promise return file.id } +/** A path becomes an id first, so the canonical load and every authorization check run unchanged. */ +async function resolveFileId(input: WorkspaceFileContextInput): Promise { + if (!namesVfsPath(input)) return input.fileId + if (input.assertedWorkspaceId === undefined) { + throw new OrchestrationError('not_found', 'File not found') + } + return resolveWorkspaceFileVfsPath(input.assertedWorkspaceId, input.fileId) +} + export async function resolveActiveWorkspaceFileContext( input: WorkspaceFileContextInput ): Promise { diff --git a/apps/sim/lib/workspace-files/limits.ts b/apps/sim/lib/workspace-files/limits.ts index d5b89b45483..c7b0339f1be 100644 --- a/apps/sim/lib/workspace-files/limits.ts +++ b/apps/sim/lib/workspace-files/limits.ts @@ -1,4 +1,6 @@ export const MAX_WORKSPACE_FILE_BULK_REQUEST_IDS = 1_000 +/** Each `files/…` path costs its own lookup, so a bulk request resolves far fewer than it takes ids. */ +export const MAX_WORKSPACE_FILE_BULK_REQUEST_PATHS = 100 export const MAX_WORKSPACE_FILE_BULK_AFFECTED_ITEMS = 5_000 /** diff --git a/apps/sim/lib/workspace-files/render-document-for-vision.test.ts b/apps/sim/lib/workspace-files/render-document-for-vision.test.ts index 359efdc8290..39289162272 100644 --- a/apps/sim/lib/workspace-files/render-document-for-vision.test.ts +++ b/apps/sim/lib/workspace-files/render-document-for-vision.test.ts @@ -34,6 +34,11 @@ describe('document page selection', () => { expect(() => resolveDocumentPages(37, pages)).toThrow() } ) + it('tells a caller who lists separate pages to read each range on its own', () => { + expect(() => resolveDocumentPages(37, '5,9, 17')).toThrow( + '"5,9, 17" lists several pages; pass one page or one contiguous range per read' + ) + }) }) describe('renderDocumentForVision', () => { diff --git a/apps/sim/lib/workspace-files/render-document-for-vision.ts b/apps/sim/lib/workspace-files/render-document-for-vision.ts index 716b74deb76..14d34f8e8d6 100644 --- a/apps/sim/lib/workspace-files/render-document-for-vision.ts +++ b/apps/sim/lib/workspace-files/render-document-for-vision.ts @@ -16,6 +16,12 @@ const pageResultSchema = z.object({ function requestedPages(pages?: string): { first: number; last: number } | undefined { if (pages === undefined) return undefined const match = /^(\d+)(?:-(\d+))?$/.exec(pages.trim()) + if (!match && /^\d+(?:-\d+)?(?:\s*,\s*\d+(?:-\d+)?)+$/.test(pages.trim())) { + throw new OrchestrationError( + 'validation', + `"${pages.trim()}" lists several pages; pass one page or one contiguous range per read, such as 5 or 2-5, and read the others separately` + ) + } if (!match) throw new OrchestrationError('validation', 'Pages must be a page number or range, such as 2-5') const first = Number(match[1]) diff --git a/packages/sim-cli/src/contract/commands.ts b/packages/sim-cli/src/contract/commands.ts index 476b62bd386..f62fae24641 100644 --- a/packages/sim-cli/src/contract/commands.ts +++ b/packages/sim-cli/src/contract/commands.ts @@ -114,7 +114,7 @@ const TARGET_FOLDER_PATH_FLAG = { * catch, and nothing about that failure would point back here. */ const LOG_LIST_FILTER_FLAGS = { - workflowIds: { name: 'workflow', list: true }, + workflowIds: { name: 'workflow', list: true, hiddenAliases: ['workflow-ids'] }, folderPaths: FOLDER_PATHS_FLAG, triggers: { name: 'trigger', list: true }, } as const @@ -251,7 +251,7 @@ export const CLI_CONTRACT: CliContract = { command: 'tables rows batch-delete', describe: 'Delete rows matching a filter, or an explicit list of ids', flags: { - rowIds: { name: 'row', list: true }, + rowIds: { name: 'row', list: true, hiddenAliases: ['row-ids'] }, filter: { json: true, describe: TABLE_FILTER_HELP }, }, confirm: 'This deletes every matching row and cannot be undone.', @@ -1558,7 +1558,12 @@ export const CLI_CONTRACT: CliContract = { command: 'files batch-delete', describe: 'Delete several files at once', flags: { - fileIds: { list: true, trailing: true }, + fileIds: { + list: true, + trailing: true, + fileReferences: true, + hiddenAliases: ['ids', 'paths'], + }, }, confirm: 'This archives the listed files; restore them with `files restore`.', }, diff --git a/packages/sim-cli/src/contract/types.ts b/packages/sim-cli/src/contract/types.ts index e22af992008..0823017e60f 100644 --- a/packages/sim-cli/src/contract/types.ts +++ b/packages/sim-cli/src/contract/types.ts @@ -43,6 +43,25 @@ export interface FlagSpec { * current spelling rather than silently picking one. */ renamedFrom?: readonly string[] + /** + * Other names this flag answers to because callers reach for them: the request field's own + * name behind a shortened flag (`--row-ids` for `--row`), or the noun a sibling command uses + * (`--ids`, `--paths` for `--file-ids`). Only for a spelling that can mean nothing but this + * field on this command. + * + * Hidden from help and the generated docs like a {@link renamedFrom} spelling, but silent, + * since nothing was renamed. Combining one with the documented spelling is refused. + */ + hiddenAliases?: readonly string[] + /** + * The list holds workspace file ids or `files/…` paths, so an inline value written as a JSON + * array (`["wf_a","wf_b"]`) or joined with commas (`wf_a,wf_b`) is read as its entries. + * + * Unambiguous only because of what a file reference can be: an id is `[A-Za-z0-9_-]` and a + * path starts with `files/`, so neither starts with `[`. A comma splits only a value with no + * `/`, which no id contains and no path lacks; a path keeps a comma in a file name. + */ + fileReferences?: true /** * Accept one or more space-separated values, or `@path` / `@-` with one * value per line. diff --git a/packages/sim-cli/src/generated/v2-api.ts b/packages/sim-cli/src/generated/v2-api.ts index 1b87843ded4..64b146e3987 100644 --- a/packages/sim-cli/src/generated/v2-api.ts +++ b/packages/sim-cli/src/generated/v2-api.ts @@ -16196,7 +16196,12 @@ export const V2_OPERATIONS = { summary: 'Delete files', body: { workspaceId: { kind: 'string', required: true, describe: 'Workspace containing the files.' }, - fileIds: { kind: 'array', required: true, describe: 'File identifiers to delete.' }, + fileIds: { + kind: 'array', + required: true, + describe: + 'Files to delete, each by identifier or by the VFS path of a live file: `files//`. Pass the id when two files in a folder have names that display alike. At most 100 entries may be paths.', + }, }, }, bulkDeleteTables: { diff --git a/packages/sim-cli/src/runtime/execute.ts b/packages/sim-cli/src/runtime/execute.ts index 2ffb1d7009d..c521ae209ad 100644 --- a/packages/sim-cli/src/runtime/execute.ts +++ b/packages/sim-cli/src/runtime/execute.ts @@ -359,35 +359,38 @@ function assertSelectorIsUsable(operation: V2OperationName, flags: Record