diff --git a/apps/docs/content/docs/en/cli/reference.mdx b/apps/docs/content/docs/en/cli/reference.mdx index f90021ebee2..9bb1d4cf8af 100644 --- a/apps/docs/content/docs/en/cli/reference.mdx +++ b/apps/docs/content/docs/en/cli/reference.mdx @@ -4615,7 +4615,7 @@ Also spelled `sim workflow`. Activate Workflow Version (personal API key required) ```bash -sim workflows activate create +sim workflows activate create [options] ``` **Arguments** @@ -4629,6 +4629,16 @@ sim workflows activate create +**Options** + + + +| Option | Required | Description | +| --- | --- | --- | +| `-y, --yes` | Yes | Confirm this operation. | + + + ### sim workflows operations apply Apply Workflow Operations (personal API key required) diff --git a/apps/docs/content/docs/en/cli/workflows.mdx b/apps/docs/content/docs/en/cli/workflows.mdx index 248b9d18776..18c783a1c31 100644 --- a/apps/docs/content/docs/en/cli/workflows.mdx +++ b/apps/docs/content/docs/en/cli/workflows.mdx @@ -12,7 +12,7 @@ Every command below also accepts the [global options](/cli/commands#global-optio ## Activate workflow version ```bash -sim workflows activate create +sim workflows activate create [options] ``` Activate Workflow Version (personal API key required) @@ -28,6 +28,16 @@ Activate Workflow Version (personal API key required) +**Options** + + + +| Option | Required | Description | +| --- | --- | --- | +| `-y, --yes` | Yes | Confirm this operation. | + + + ## Apply workflow operations ```bash diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index 88dd604ab00..696ff4e3623 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -2098,9 +2098,9 @@ "name": "parentPath", "in": "query", "required": false, - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "schema": { - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "$ref": "#/components/schemas/FolderPathInput" } }, diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index d433c0ea73b..38eecd23994 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -2718,9 +2718,9 @@ "name": "parentPath", "in": "query", "required": false, - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "schema": { - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "$ref": "#/components/schemas/FolderPathInput" } }, diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index 25250fc7714..359c8e1d4b9 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -3726,9 +3726,9 @@ "name": "parentPath", "in": "query", "required": false, - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "schema": { - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "$ref": "#/components/schemas/FolderPathInput" } }, diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index b80dda73fab..ca61a2adaca 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -3206,9 +3206,9 @@ "name": "parentPath", "in": "query", "required": false, - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "schema": { - "description": "Restrict results to direct children of this parent path.", + "description": "Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.", "$ref": "#/components/schemas/FolderPathInput" } }, diff --git a/apps/sim/lib/api/contracts/v2/shared.ts b/apps/sim/lib/api/contracts/v2/shared.ts index eb2ad6edb19..fd594f14f90 100644 --- a/apps/sim/lib/api/contracts/v2/shared.ts +++ b/apps/sim/lib/api/contracts/v2/shared.ts @@ -471,10 +471,10 @@ export const v2SearchSchema = z * to nothing, exactly as `workflowIds` naming no workflow does. These lists used * to answer `404 Folder not found` instead, which reported a missing collection * for a collection that exists, broke a pagination walk when a folder was - * deleted mid-walk, and made a list a folder-existence oracle. The sibling - * folder lists already answered a non-matching `parentPath` with an empty page. - * Mutations keep their 404 — creating into or moving to a folder that does not - * exist has no empty-set reading. + * deleted mid-walk, and made a list a folder-existence oracle. The folder lists + * answer a non-matching `parentPath` the same way, so one rule covers every + * folder filter in the family. Mutations keep their 404 — creating into or + * moving to a folder that does not exist has no empty-set reading. */ export const V2_FOLDER_FILTER_MISS = 'A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.' @@ -593,7 +593,9 @@ export const v2ListFoldersQuerySchema = z workspaceId: workspaceIdSchema.describe('Workspace whose folders should be listed.'), parentPath: v2FolderPathInputSchema .optional() - .describe('Restrict results to direct children of this parent path.'), + .describe( + `Restrict results to direct children of this parent path. ${V2_FOLDER_FILTER_MISS}` + ), search: v2SearchSchema.describe('Case-insensitive substring match against the folder name.'), ...v2SortFields(v2FolderSortFields, { sortBy: 'name', sortOrder: 'asc' }), }) diff --git a/apps/sim/lib/catalog/application/catalog-reads.test.ts b/apps/sim/lib/catalog/application/catalog-reads.test.ts index 9f8aa2d86fa..74a4e73e0a1 100644 --- a/apps/sim/lib/catalog/application/catalog-reads.test.ts +++ b/apps/sim/lib/catalog/application/catalog-reads.test.ts @@ -107,6 +107,22 @@ const TOOL_METADATA: Record> = { params: {}, hostedApiKey: 'always', }, + confluence_read: { + id: 'confluence_read', + name: 'Confluence Read', + description: 'Read a Confluence page.', + version: '1.0.0', + params: {}, + hostedApiKey: 'none', + }, + confluence_read_v2: { + id: 'confluence_read_v2', + name: 'Confluence Read', + description: 'Read a Confluence page.', + version: '2.0.0', + params: {}, + hostedApiKey: 'none', + }, } const WORKSPACE_ID = 'workspace-1' @@ -186,6 +202,7 @@ const confluenceV2 = block({ type: 'confluence_v2', name: 'Confluence', description: 'Read Confluence pages.', + tools: { access: ['confluence_read_v2'] }, }) interface Visibility { @@ -566,6 +583,34 @@ describe('catalog block and tool reads', () => { expect(listed.entries.map((entry) => entry.hostedApiKey)).toEqual(['none', 'none']) }) + /** + * A superseded v1 tool stays registered so execution of a stored id keeps + * working, so `resolveToolId('confluence_read')` answers with the v1 id no + * visible block exposes — and `GET /v2/tools/confluence_read` 404'd while the + * list published `confluence_read_v2`. + */ + it('resolves an unversioned tool name to its newest visible version and echoes the resolved id', async () => { + mocks.getAllBlocks.mockReturnValue([confluenceV2]) + + const { tool } = await getCatalogTool.execute({ + principal: session, + input: { workspaceId: WORKSPACE_ID, toolId: 'confluence_read' }, + }) + + expect(tool.id).toBe('confluence_read_v2') + }) + + it('echoes an exact versioned tool id unchanged', async () => { + mocks.getAllBlocks.mockReturnValue([confluenceV2]) + + const { tool } = await getCatalogTool.execute({ + principal: session, + input: { workspaceId: WORKSPACE_ID, toolId: 'confluence_read_v2' }, + }) + + expect(tool.id).toBe('confluence_read_v2') + }) + it('reads one tool with its params and outputs', async () => { const { tool } = await getCatalogTool.execute({ principal: session, diff --git a/apps/sim/lib/catalog/application/get-tool.ts b/apps/sim/lib/catalog/application/get-tool.ts index d08b181f838..62b1323a2c4 100644 --- a/apps/sim/lib/catalog/application/get-tool.ts +++ b/apps/sim/lib/catalog/application/get-tool.ts @@ -3,12 +3,11 @@ import { resolveCatalogGate, } from '@/lib/catalog/application/catalog-context' import { catalogOperations } from '@/lib/catalog/application/operations' -import { resolveVisibleToolIds } from '@/lib/catalog/application/tool-scope' +import { resolveVisibleToolId, resolveVisibleToolIds } from '@/lib/catalog/application/tool-scope' import { type CatalogToolDetail, projectToolDetail } from '@/lib/catalog/projection/tool' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { isHosted } from '@/lib/core/config/env-flags' import { OrchestrationError } from '@/lib/core/orchestration/types' -import { resolveToolId } from '@/tools/tool-ids' export interface GetCatalogToolInput { workspaceId: string @@ -22,10 +21,14 @@ export interface GetCatalogToolResult { /** * One built-in tool's parameters and outputs. * - * An unversioned name resolves to the newest version exactly as execution does, - * and the returned `id` is the resolved one so a caller can see which version - * answered. A tool the workspace's blocks do not expose answers 404 rather than - * 403, for the same enumeration reason as the block detail read. + * An unversioned name resolves to the newest version this caller can see, the + * way the block detail read does, and the returned `id` is the resolved one so + * a caller can see which version answered. Resolving through + * `@/tools/tool-ids` instead would answer with the superseded v1 that stays + * registered for execution's sake, which no visible block exposes — so every + * versioned family 404'd on the base name. A tool the workspace's blocks do not + * expose answers 404 rather than 403, for the same enumeration reason as the + * block detail read. */ export const getCatalogTool = defineAuthorizedWorkspaceUseCase({ operation: catalogOperations.readTool, @@ -33,16 +36,16 @@ export const getCatalogTool = defineAuthorizedWorkspaceUseCase({ loadCatalogWorkspaceContext(input.workspaceId), authorizationOptions: {}, execute: async ({ principal, input, context }): Promise => { - const resolvedToolId = resolveToolId(input.toolId) - const tool = projectToolDetail(resolvedToolId, { hostedKeys: isHosted }) - if (!tool) throw new OrchestrationError('not_found', 'Tool not found') - const gate = await resolveCatalogGate(principal, context) const visibleToolIds = await resolveVisibleToolIds(gate) + const resolvedToolId = resolveVisibleToolId(input.toolId, visibleToolIds) if (!visibleToolIds.has(resolvedToolId)) { throw new OrchestrationError('not_found', 'Tool not found') } + const tool = projectToolDetail(resolvedToolId, { hostedKeys: isHosted }) + if (!tool) throw new OrchestrationError('not_found', 'Tool not found') + return { tool } }, }) diff --git a/apps/sim/lib/catalog/application/tool-scope.ts b/apps/sim/lib/catalog/application/tool-scope.ts index c7c5a0c344a..77868f61a2f 100644 --- a/apps/sim/lib/catalog/application/tool-scope.ts +++ b/apps/sim/lib/catalog/application/tool-scope.ts @@ -6,6 +6,41 @@ import { import { getAllBlocks } from '@/blocks/registry' import { resolveToolId } from '@/tools/tool-ids' +const VERSION_SUFFIX = /^\d+$/ + +/** + * The newest visible version of a tool name, or the name unchanged when none is. + * + * The detail-read counterpart of {@link resolveVisibleToolIds}, and the tool + * analogue of `getLatestBlockForViewer`: "newest registered" and "newest visible + * to this caller" are different questions. `@/tools/tool-ids` answers the first + * — and superseded v1 tools stay registered so execution of a stored id keeps + * working, so `resolveToolId('github_comment')` returns `github_comment`, which + * no visible block exposes. Resolving against the visible set instead walks down + * to `github_comment_v2`, the id the tool list publishes. + * + * An id that is itself visible is returned untouched, so an exact versioned + * request never silently answers with a different version. + */ +export function resolveVisibleToolId(toolId: string, visibleToolIds: ReadonlySet): string { + if (visibleToolIds.has(toolId)) return toolId + + const prefix = `${toolId}_v` + let bestId: string | undefined + let bestVersion = 0 + for (const candidate of visibleToolIds) { + if (!candidate.startsWith(prefix)) continue + const suffix = candidate.slice(prefix.length) + if (!VERSION_SUFFIX.test(suffix)) continue + const version = Number.parseInt(suffix, 10) + if (version > bestVersion) { + bestVersion = version + bestId = candidate + } + } + return bestId ?? toolId +} + /** * The built-in tools this caller may run in this workspace. * diff --git a/apps/sim/lib/folders/queries.ts b/apps/sim/lib/folders/queries.ts index 91b000867a0..7c381e42c4b 100644 --- a/apps/sim/lib/folders/queries.ts +++ b/apps/sim/lib/folders/queries.ts @@ -266,8 +266,8 @@ export type FolderPathFilter = * event from all the others, told a caller its *collection* was missing when it * was not, turned a folder deleted mid-walk into a failed pagination loop, and * answered whether a path exists on an endpoint that was not asked. The sibling - * folder lists already answer a non-matching `parentPath` with an empty page, so - * this is the family's existing behavior applied to the resource lists too. + * folder lists answer a non-matching `parentPath` the same way, so one rule + * covers every folder filter the family accepts. * * A path that could not name a folder at all is still rejected by the contract, * as a 400, before any of this runs. Mutations keep their 404: creating into or diff --git a/apps/sim/lib/knowledge/application/folders.test.ts b/apps/sim/lib/knowledge/application/folders.test.ts index 33442e38f6d..a2aafbee769 100644 --- a/apps/sim/lib/knowledge/application/folders.test.ts +++ b/apps/sim/lib/knowledge/application/folders.test.ts @@ -42,6 +42,12 @@ vi.mock('@/lib/knowledge/application/contexts', () => ({ vi.mock('@/lib/folders/queries', () => ({ loadActiveFolderPathIndex: mocks.loadIndex, listActiveFolderRows: mocks.listRows, + resolveFolderPathFilter: (index: { idByPath: Map }, path: string | undefined) => { + if (path === undefined) return { kind: 'unfiltered' } + if (path === '/') return { kind: 'folder', folderId: null } + const folderId = index.idByPath.get(path) + return folderId === undefined ? { kind: 'noMatch' } : { kind: 'folder', folderId } + }, resolveFolderPathFromIndex: (index: { idByPath: Map }, path: string) => path === '/' ? null : index.idByPath.get(path), })) @@ -112,14 +118,19 @@ describe('knowledge folder application use cases', () => { expect(result.folders[0]).toMatchObject({ id: 'folder-1', path: '/Docs' }) }) - it('rejects a missing parent without querying folder rows', async () => { - await expect( - listKnowledgeFolders.execute({ - principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, - input: { workspaceId: 'workspace-1', parentPath: '/Missing' }, - }) - ).rejects.toMatchObject({ code: 'not_found' }) + /** + * `parentPath` is a filter, so a path naming no active folder narrows the + * result to nothing rather than reporting the collection missing. Falling + * through to `listActiveFolderRows` with an undefined `parentId` would list + * every folder in the workspace, so the miss has to short-circuit. + */ + it('answers a parent path naming no folder with an empty page', async () => { + const result = await listKnowledgeFolders.execute({ + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + input: { workspaceId: 'workspace-1', parentPath: '/Missing' }, + }) + expect(result.folders).toEqual([]) expect(mocks.listRows).not.toHaveBeenCalled() }) diff --git a/apps/sim/lib/knowledge/application/folders.ts b/apps/sim/lib/knowledge/application/folders.ts index b12a59c7eec..9bed04a3bbb 100644 --- a/apps/sim/lib/knowledge/application/folders.ts +++ b/apps/sim/lib/knowledge/application/folders.ts @@ -14,6 +14,7 @@ import { ROOT_FOLDER_PATH } from '@/lib/folders/paths' import { listActiveFolderRows, loadActiveFolderPathIndex, + resolveFolderPathFilter, resolveFolderPathFromIndex, } from '@/lib/folders/queries' import { defineAuthorizedKnowledgeUseCase } from '@/lib/knowledge/application/authorized-knowledge-use-case' @@ -67,15 +68,10 @@ export const listKnowledgeFolders = defineAuthorizedKnowledgeUseCase({ undefined, { maxRows: MAX_KNOWLEDGE_FOLDERS_PER_WORKSPACE } ) - const parentId = - input.parentPath === undefined - ? undefined - : resolveFolderPathFromIndex(index, input.parentPath) - if (input.parentPath !== undefined && parentId === undefined) { - throw new OrchestrationError('not_found', 'Folder not found') - } + const parentFilter = resolveFolderPathFilter(index, input.parentPath) + if (parentFilter.kind === 'noMatch') return { folders: [] } const folders = await listActiveFolderRows(context.workspaceId, 'knowledge_base', { - parentId, + parentId: parentFilter.kind === 'folder' ? parentFilter.folderId : undefined, search: input.search, sortBy: input.sortBy, sortOrder: input.sortOrder, diff --git a/apps/sim/lib/table/application/folders.test.ts b/apps/sim/lib/table/application/folders.test.ts new file mode 100644 index 00000000000..30f0b60af42 --- /dev/null +++ b/apps/sim/lib/table/application/folders.test.ts @@ -0,0 +1,106 @@ +/** + * @vitest-environment node + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + listRows: vi.fn(), + loadFolderIndex: vi.fn(), + resolvePermission: vi.fn(), + resolveWorkspaceContext: vi.fn(), +})) + +vi.mock('@sim/audit', () => ({ + AuditAction: { + FOLDER_CREATED: 'folder.created', + FOLDER_DELETED: 'folder.deleted', + FOLDER_MOVED: 'folder.moved', + FOLDER_RESTORED: 'folder.restored', + }, + AuditResourceType: { FOLDER: 'folder' }, + recordAudit: vi.fn(), +})) + +vi.mock('@sim/platform-authz/workspace', () => ({ + permissionSatisfies: (actual: string | null, required: string) => { + const rank = { read: 1, write: 2, admin: 3 } as const + return ( + actual !== null && rank[actual as keyof typeof rank] >= rank[required as keyof typeof rank] + ) + }, + resolveEffectiveWorkspacePermission: mocks.resolvePermission, +})) + +vi.mock('@/lib/folders/orchestration', () => ({ + createFolderAtPathTransition: vi.fn(), + deleteFolderByPathTransition: vi.fn(), + relocateFolderByPathTransition: vi.fn(), + restoreFolder: vi.fn(), +})) + +vi.mock('@/lib/folders/queries', () => ({ + findArchivedFolderIdByPath: vi.fn(), + listActiveFolderRows: mocks.listRows, + loadActiveFolderPathIndex: mocks.loadFolderIndex, + resolveFolderPathFilter: (index: { idByPath: Map }, path: string | undefined) => { + if (path === undefined) return { kind: 'unfiltered' } + if (path === '/') return { kind: 'folder', folderId: null } + const folderId = index.idByPath.get(path) + return folderId === undefined ? { kind: 'noMatch' } : { kind: 'folder', folderId } + }, +})) + +vi.mock('@/lib/table/application/context', () => ({ + resolveTableWorkspaceContext: mocks.resolveWorkspaceContext, +})) + +import { listTableFoldersUseCase } from '@/lib/table/application/folders' + +const principal = { kind: 'session', userId: 'user-1', sessionId: 'session-1' } as const + +describe('listTableFoldersUseCase', () => { + beforeEach(() => { + vi.clearAllMocks() + mocks.resolveWorkspaceContext.mockResolvedValue({ + workspaceId: 'ws-1', + billedAccountUserId: 'owner-1', + }) + mocks.resolvePermission.mockResolvedValue('admin') + mocks.loadFolderIndex.mockResolvedValue({ + idByPath: new Map([['/Reports', 'folder-1']]), + pathById: new Map([['folder-1', '/Reports']]), + rowById: new Map(), + }) + mocks.listRows.mockResolvedValue([]) + }) + + it('resolves a canonical parent path before listing', async () => { + await listTableFoldersUseCase.execute({ + principal, + input: { workspaceId: 'ws-1', parentPath: '/Reports' }, + }) + + expect(mocks.listRows).toHaveBeenCalledWith( + 'ws-1', + 'table', + expect.objectContaining({ parentId: 'folder-1' }) + ) + }) + + /** + * `parentPath` is a filter, so a path naming no active folder narrows the + * result to nothing rather than reporting the collection missing. Falling + * through to `listActiveFolderRows` with an undefined `parentId` would list + * every folder in the workspace, so the miss has to short-circuit. + */ + it('answers a parent path naming no folder with an empty page', async () => { + const result = await listTableFoldersUseCase.execute({ + principal, + input: { workspaceId: 'ws-1', parentPath: '/Missing' }, + }) + + expect(result.folders).toEqual([]) + expect(mocks.listRows).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/table/application/folders.ts b/apps/sim/lib/table/application/folders.ts index 5bff78498fd..0f7568171e8 100644 --- a/apps/sim/lib/table/application/folders.ts +++ b/apps/sim/lib/table/application/folders.ts @@ -14,7 +14,7 @@ import { findArchivedFolderIdByPath, listActiveFolderRows, loadActiveFolderPathIndex, - resolveFolderPathFromIndex, + resolveFolderPathFilter, } from '@/lib/folders/queries' import { defineAuthorizedTableUseCase } from '@/lib/table/application/authorized-table-use-case' import { resolveTableWorkspaceContext } from '@/lib/table/application/context' @@ -37,15 +37,10 @@ export const listTableFoldersUseCase = defineAuthorizedTableUseCase({ const index = await loadActiveFolderPathIndex(context.workspaceId, 'table', undefined, { maxRows: MAX_FOLDERS_PER_WORKSPACE, }) - const parentId = - input.parentPath === undefined - ? undefined - : resolveFolderPathFromIndex(index, input.parentPath) - if (input.parentPath !== undefined && parentId === undefined) { - throw new OrchestrationError('not_found', 'Folder not found') - } + const parentFilter = resolveFolderPathFilter(index, input.parentPath) + if (parentFilter.kind === 'noMatch') return { folders: [], index } const folders = await listActiveFolderRows(context.workspaceId, 'table', { - parentId, + parentId: parentFilter.kind === 'folder' ? parentFilter.folderId : undefined, search: input.search, sortBy: input.sortBy, sortOrder: input.sortOrder, diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.test.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.test.ts index 8b6963f7382..797b5469b97 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.test.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ -import { dbChainMockFns, resetDbChainMock } from '@sim/testing' +import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockAcquireFolderMutationLock, mockDeduplicateFolderName } = vi.hoisted(() => ({ @@ -25,6 +25,7 @@ import { buildWorkspaceFileFolderPathMap, createWorkspaceFileFolder, ensureWorkspaceFileFolderPath, + listWorkspaceFileFolders, normalizeWorkspaceFileItemName, WorkspaceFileFolderConflictError, WorkspaceFileItemsNotFoundError, @@ -168,6 +169,52 @@ describe('workspace file folder failure classification', () => { }) }) +describe('listWorkspaceFileFolders', () => { + const now = new Date('2026-08-17T12:00:00.000Z') + const activeParent = { + id: 'parent-1', + resourceType: 'file', + workspaceId: 'workspace-1', + userId: 'user-1', + name: 'Engineering', + parentId: null, + sortOrder: 0, + deletedAt: null, + createdAt: now, + updatedAt: now, + } + const archivedChild = { + ...activeParent, + id: 'child-1', + name: 'Archive', + parentId: 'parent-1', + deletedAt: now, + } + + beforeEach(() => { + resetDbChainMock() + }) + + it('resolves an archived folder path through its still-active ancestors', async () => { + queueTableRows(schemaMock.folder, [archivedChild]) + queueTableRows(schemaMock.folder, [activeParent, archivedChild]) + + const folders = await listWorkspaceFileFolders('workspace-1', { scope: 'archived' }) + + expect(folders).toHaveLength(1) + expect(folders[0].path).toBe('Engineering/Archive') + }) + + it('does not take an extra query for the active scope', async () => { + queueTableRows(schemaMock.folder, [activeParent]) + + const folders = await listWorkspaceFileFolders('workspace-1') + + expect(folders.map((folder) => folder.path)).toEqual(['Engineering']) + expect(dbChainMockFns.from).toHaveBeenCalledOnce() + }) +}) + describe('archiveWorkspaceFileFolderIfEmpty', () => { beforeEach(() => { resetDbChainMock() diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts index 4ec715d2474..c63aa73469b 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts @@ -385,6 +385,21 @@ export async function findWorkspaceFileFolderIdByPath( return parentId } +/** + * Selects the minimal columns needed to resolve every file folder's canonical path. + * + * Includes archived rows: an archived folder can still have an active ancestor, so a + * path map built only from archived rows truncates its path to the bare folder name. + */ +async function selectFileFolderPathRows( + workspaceId: string +): Promise>> { + return db + .select({ id: folderTable.id, name: folderTable.name, parentId: folderTable.parentId }) + .from(folderTable) + .where(and(eq(folderTable.workspaceId, workspaceId), isFileFolder)) +} + /** * Lists a workspace's file folders, ordered in the database like every other folder * list so a name sort uses the same collation and the same `createdAt` tiebreak. @@ -420,7 +435,9 @@ export async function listWorkspaceFileFolders( ) .orderBy(...listOrderBy(FOLDER_SORTS[sortBy], sortOrder)) - const paths = buildWorkspaceFileFolderPathMap(rows) + const paths = buildWorkspaceFileFolderPathMap( + scope === 'archived' ? await selectFileFolderPathRows(workspaceId) : rows + ) return rows.map((row) => mapFolder(row, paths)) } diff --git a/apps/sim/lib/workflows/application/workflow-folders.test.ts b/apps/sim/lib/workflows/application/workflow-folders.test.ts index 9b7e173b7c7..d7a0d67d7d3 100644 --- a/apps/sim/lib/workflows/application/workflow-folders.test.ts +++ b/apps/sim/lib/workflows/application/workflow-folders.test.ts @@ -41,6 +41,12 @@ vi.mock('@/lib/folders/orchestration', () => ({ vi.mock('@/lib/folders/queries', () => ({ listActiveFolderRows: mocks.listRows, loadActiveFolderPathIndex: mocks.loadIndex, + resolveFolderPathFilter: (index: { idByPath: Map }, path: string | undefined) => { + if (path === undefined) return { kind: 'unfiltered' } + if (path === '/') return { kind: 'folder', folderId: null } + const folderId = index.idByPath.get(path) + return folderId === undefined ? { kind: 'noMatch' } : { kind: 'folder', folderId } + }, resolveFolderPathFromIndex: (index: { idByPath: Map }, path: string) => path === '/' ? null : index.idByPath.get(path), })) @@ -146,6 +152,37 @@ describe('workflow folder application operations', () => { ) }) + /** + * `parentPath` is a filter, so a path naming no active folder narrows the + * result to nothing rather than reporting the collection missing. Falling + * through to `listActiveFolderRows` with an undefined `parentId` would list + * every folder in the workspace, so the miss has to short-circuit. + */ + it('answers a parent path naming no folder with an empty page', async () => { + const result = await listWorkflowFolders.execute({ + principal: principals[0], + input: { workspaceId: 'ws-1', parentPath: '/Missing', sortBy: 'name', sortOrder: 'asc' }, + }) + + expect(result.folders).toEqual([]) + expect(mocks.listRows).not.toHaveBeenCalled() + }) + + it('resolves a canonical parent path before listing', async () => { + mocks.listRows.mockResolvedValueOnce([folder]) + + await listWorkflowFolders.execute({ + principal: principals[0], + input: { workspaceId: 'ws-1', parentPath: '/Reports', sortBy: 'name', sortOrder: 'asc' }, + }) + + expect(mocks.listRows).toHaveBeenCalledWith( + 'ws-1', + 'workflow', + expect.objectContaining({ parentId: folder.id }) + ) + }) + it('rejects a workspace key outside the canonical workspace before mutation', async () => { await expect( createWorkflowFolder.execute({ diff --git a/apps/sim/lib/workflows/application/workflow-folders.ts b/apps/sim/lib/workflows/application/workflow-folders.ts index 64eb5141be5..5c8403d56fa 100644 --- a/apps/sim/lib/workflows/application/workflow-folders.ts +++ b/apps/sim/lib/workflows/application/workflow-folders.ts @@ -15,6 +15,7 @@ import { ROOT_FOLDER_PATH } from '@/lib/folders/paths' import { listActiveFolderRows, loadActiveFolderPathIndex, + resolveFolderPathFilter, resolveFolderPathFromIndex, } from '@/lib/folders/queries' import { defineAuthorizedWorkflowUseCase } from '@/lib/workflows/application/authorized-workflow-use-case' @@ -133,15 +134,10 @@ export const listWorkflowFolders = defineAuthorizedWorkflowUseCase({ const index = await loadActiveFolderPathIndex(context.workspaceId, 'workflow', undefined, { maxRows: MAX_FOLDERS_PER_WORKSPACE, }) - const parentId = - input.parentPath === undefined - ? undefined - : resolveFolderPathFromIndex(index, input.parentPath) - if (input.parentPath !== undefined && parentId === undefined) { - throw new OrchestrationError('not_found', 'Folder not found') - } + const parentFilter = resolveFolderPathFilter(index, input.parentPath) + if (parentFilter.kind === 'noMatch') return { folders: [], index } const folders = await listActiveFolderRows(context.workspaceId, 'workflow', { - parentId, + parentId: parentFilter.kind === 'folder' ? parentFilter.folderId : undefined, search: input.search, sortBy: input.sortBy, sortOrder: input.sortOrder, diff --git a/apps/sim/lib/workspace-files/application/workspace-file-folders.test.ts b/apps/sim/lib/workspace-files/application/workspace-file-folders.test.ts index cc0d072f031..b673baaec77 100644 --- a/apps/sim/lib/workspace-files/application/workspace-file-folders.test.ts +++ b/apps/sim/lib/workspace-files/application/workspace-file-folders.test.ts @@ -275,6 +275,22 @@ describe('workspace file folder operations', () => { expect(mockRestore).not.toHaveBeenCalled() }) + it('refuses to guess when two archived folders share the same path', async () => { + mockList.mockResolvedValueOnce([ + { id: 'folder-first', name: 'Archive', path: 'Engineering/Archive' }, + { id: 'folder-second', name: 'Archive', path: 'Engineering/Archive' }, + ]) + + await expect( + restoreWorkspaceFileFolderOperation.execute({ + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + input: { workspaceId: 'ws-1', path: '/Engineering/Archive' }, + }) + ).rejects.toMatchObject({ code: 'conflict' }) + + expect(mockRestore).not.toHaveBeenCalled() + }) + it('rejects restoring the workspace root', async () => { await expect( restoreWorkspaceFileFolderOperation.execute({ diff --git a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts index 82b8b7f18e3..50f138b94fb 100644 --- a/apps/sim/lib/workspace-files/application/workspace-file-folders.ts +++ b/apps/sim/lib/workspace-files/application/workspace-file-folders.ts @@ -264,15 +264,21 @@ async function findArchivedFolderIdByPath(workspaceId: string, path: string): Pr throw new OrchestrationError('validation', 'The workspace root cannot be restored') } const archived = await listWorkspaceFileFolders(workspaceId, { scope: 'archived' }) - const match = archived.find((folder) => { + const matches = archived.filter((folder) => { const segments = parseWorkspaceFileFolderDisplayPath(folder.path) return ( segments.length === target.length && segments.every((segment, index) => segment === target[index]) ) }) - if (!match) throw new OrchestrationError('not_found', 'Folder not found') - return match.id + if (matches.length === 0) throw new OrchestrationError('not_found', 'Folder not found') + if (matches.length > 1) { + throw new OrchestrationError( + 'conflict', + 'Multiple archived folders share this path. Restore by folder id instead.' + ) + } + return matches[0].id } async function executeRestoreWorkspaceFileFolder(args: { diff --git a/packages/sim-cli/src/commands/configure.test.ts b/packages/sim-cli/src/commands/configure.test.ts index 529d7baa2b5..05bb5187098 100644 --- a/packages/sim-cli/src/commands/configure.test.ts +++ b/packages/sim-cli/src/commands/configure.test.ts @@ -42,6 +42,10 @@ function run(...args: string[]): Promise { beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'sim-cli-')) process.env.SIM_CONFIG_DIR = dir + // The refusal reads SIM_PROFILE the way `resolveProfile` does, so an ambient + // one would otherwise decide what these assertions see. Empty rather than + // `undefined`: assigning to process.env stringifies, and "undefined" is truthy. + process.env.SIM_PROFILE = '' mocks.profileName = 'default' mocks.profileFrom.mockClear() mocks.profileFrom.mockImplementation(() => ({ name: mocks.profileName })) @@ -52,6 +56,7 @@ afterEach(() => { vi.restoreAllMocks() rmSync(dir, { recursive: true, force: true }) process.env.SIM_CONFIG_DIR = undefined + process.env.SIM_PROFILE = '' }) describe('configure --set-endpoint', () => { @@ -190,6 +195,78 @@ describe('configure and the root globals', () => { expect(readConfigProfile('default')).toEqual({}) }) + /** + * The suggested command is meant to be pasted verbatim, so omitting the + * selected profile made it write `default` and leave the profile the caller + * was targeting untouched — silently, and reported as a success. + */ + it('carries the selected profile into the command it suggests', async () => { + await expect(run('-P', 'dev', '--output', 'json')).rejects.toThrow( + 'sim configure --profile dev --set-output json' + ) + await expect(run('-P', 'dev', '--endpoint', 'https://other.example')).rejects.toThrow( + 'sim configure --profile dev --set-endpoint https://other.example' + ) + await expect(run('-P', 'dev', '-w', 'ws_9')).rejects.toThrow( + 'sim configure --profile dev --set-workspace ws_9' + ) + }) + + it('carries a SIM_PROFILE-selected profile into the command it suggests', async () => { + process.env.SIM_PROFILE = 'dev' + + await expect(run('--output', 'json')).rejects.toThrow( + 'sim configure --profile dev --set-output json' + ) + }) + + /** + * `resolveProfile` reads `overrides.profile || process.env.SIM_PROFILE`, so + * the suggestion has to name the profile the run would actually resolve to. + */ + it('lets an explicit --profile win over SIM_PROFILE, as resolveProfile does', async () => { + process.env.SIM_PROFILE = 'staging' + + await expect(run('-P', 'dev', '--output', 'json')).rejects.toThrow( + 'sim configure --profile dev --set-output json' + ) + }) + + /** + * The profile name is caller-supplied, so it is redacted like the value — + * and redaction turns the separator into a space, which the suggestion then + * has to quote to stay one argument. + */ + it('redacts a control character out of the profile it suggests', async () => { + await expect(run('-P', 'dev\u2028sim login', '--output', 'json')).rejects.toThrow( + "sim configure --profile 'dev sim login' --set-output json" + ) + }) + + /** + * Profile-name validation is creation-only by design, so a hand-written + * `[profile my stack]` keeps resolving and reaches this suggestion. Unquoted, + * a name carrying a `;` would end the pasted command and start another. + */ + it('quotes a profile name a pasted command would otherwise split', async () => { + await expect(run('-P', 'my stack', '--output', 'json')).rejects.toThrow( + "sim configure --profile 'my stack' --set-output json" + ) + await expect(run('-P', 'a;rm -rf x', '--output', 'json')).rejects.toThrow( + "sim configure --profile 'a;rm -rf x' --set-output json" + ) + await expect(run('-P', "it's mine", '--output', 'json')).rejects.toThrow( + "sim configure --profile 'it'\\''s mine' --set-output json" + ) + }) + + /** A name that already satisfies the creation rule needs no quoting noise. */ + it('leaves an ordinary profile name bare', async () => { + await expect(run('-P', 'dev.2_a-b', '--output', 'json')).rejects.toThrow( + 'sim configure --profile dev.2_a-b --set-output json' + ) + }) + it('does not print a stale stored value as if it had been set', async () => { writeConfigProfile('default', { endpoint: 'https://staging.example' }) diff --git a/packages/sim-cli/src/commands/configure.ts b/packages/sim-cli/src/commands/configure.ts index de3c881a75b..5e1f0c0a999 100644 --- a/packages/sim-cli/src/commands/configure.ts +++ b/packages/sim-cli/src/commands/configure.ts @@ -7,7 +7,12 @@ import { resolveAuthenticationProfileName, writeConfigProfile, } from '../config/index' -import { normalizeEndpoint, normalizeWorkspaceId, redact } from '../config/profile' +import { + normalizeEndpoint, + normalizeWorkspaceId, + PROFILE_NAME_PATTERN, + redact, +} from '../config/profile' import { globalsOf, profileFrom } from '../context' import { SimApiError } from '../http/client' @@ -20,6 +25,10 @@ import { SimApiError } from '../http/client' * it did not change, which reads exactly like a confirmation. Refusing and * naming the twin is the honest answer; making the global write instead would * give one command a persistent side effect the same flag has on no other. + * + * The suggested command has to carry `--profile` whenever one is selected: + * without it, a caller who follows the advice verbatim writes the `default` + * profile and leaves the one they were targeting untouched. */ const GLOBAL_FLAG_TWINS = [ { option: 'endpoint', flag: '--endpoint', setFlag: '--set-endpoint' }, @@ -48,6 +57,21 @@ function requireValue(value: string | undefined, flag: string, key: string): voi * they arrive through `sim login`, which is the only path that mints a key with * a recorded consent behind it. */ +/** + * Quotes a profile name that a pasted command would otherwise split. + * + * Profile-name validation is creation-only by design, so a hand-written + * `[profile my stack]` keeps resolving — and reaches this suggestion carrying + * whitespace, or a `;` that would end the pasted command and start another. + * Names that already match the creation rule are left bare, since quoting every + * one of them would only add noise to the common case. + */ +function quoteProfileArgument(name: string): string { + const redacted = redact(name) + if (PROFILE_NAME_PATTERN.test(redacted)) return redacted + return `'${redacted.replaceAll("'", "'\\''")}'` +} + export function configureCommand(): Command { return new Command('configure') .description("Set a profile's endpoint, default workspace, or output format") @@ -66,11 +90,15 @@ export function configureCommand(): Command { command: Command ) => { const globals = globalsOf(command) + const selectedProfile = globals.profile || process.env.SIM_PROFILE + const profileArg = selectedProfile + ? ` --profile ${quoteProfileArgument(selectedProfile)}` + : '' for (const { option, flag, setFlag } of GLOBAL_FLAG_TWINS) { const value = globals[option] if (value === undefined) continue throw new SimApiError( - `${flag} applies to a single command and is not stored. To save it, run: sim configure ${setFlag} ${redact(value)}`, + `${flag} applies to a single command and is not stored. To save it, run: sim configure${profileArg} ${setFlag} ${redact(value)}`, 0 ) } diff --git a/packages/sim-cli/src/contract/commands.test.ts b/packages/sim-cli/src/contract/commands.test.ts index b2a61123eff..cc3c41b692d 100644 --- a/packages/sim-cli/src/contract/commands.test.ts +++ b/packages/sim-cli/src/contract/commands.test.ts @@ -322,10 +322,11 @@ describe('folder-path fields', () => { }) describe('confirm gates say what is actually at stake', () => { - it('gates the two workflow writes that change what production serves', () => { + it('gates the three workflow writes that change what production serves', () => { // `undeploy` takes the workflow offline for every consumer, its published - // MCP tools included. `rollback` changes which version is live, while the - // gated `revert` only overwrites the draft. + // MCP tools included. `rollback` and `activate` are the same application + // operation under two transitions and both change which version is live, + // while the gated `revert` only overwrites the draft. const undeploy = CLI_CONTRACT.undeployWorkflow?.confirm ?? '' expect(undeploy).toContain('offline') expect(undeploy).toMatch(/MCP/) @@ -336,6 +337,7 @@ describe('confirm gates say what is actually at stake', () => { expect(undeploy).toContain('until it is deployed again') expect(undeploy).not.toMatch(/does not restore|not recoverable|cannot be undone/) expect(CLI_CONTRACT.rollbackWorkflow?.confirm).toBeTruthy() + expect(CLI_CONTRACT.activateWorkflowVersion?.confirm).toBeTruthy() }) it('does not promise irreversible loss for a recoverable delete', () => { diff --git a/packages/sim-cli/src/contract/commands.ts b/packages/sim-cli/src/contract/commands.ts index 46868afb00f..c12b9ed0b87 100644 --- a/packages/sim-cli/src/contract/commands.ts +++ b/packages/sim-cli/src/contract/commands.ts @@ -531,6 +531,13 @@ export const CLI_CONTRACT: CliContract = { confirm: 'This changes which deployed version runs in production for every API and chat consumer.', }, + // The same application operation as `rollback`, under a different transition: + // both switch production away from the version the caller last chose. Gating + // one and not the other was an accident of naming, not a policy. + activateWorkflowVersion: { + confirm: + 'This changes which deployed version runs in production for every API and chat consumer.', + }, // POST derives to `... create`, which creates nothing here. Named for the // operation instead, matching the shipped `files move`. moveWorkflows: { diff --git a/packages/sim-cli/src/generated/v2-api.ts b/packages/sim-cli/src/generated/v2-api.ts index d623c667e6f..e4a2f179863 100644 --- a/packages/sim-cli/src/generated/v2-api.ts +++ b/packages/sim-cli/src/generated/v2-api.ts @@ -11702,7 +11702,8 @@ export const V2_OPERATIONS = { }, parentPath: { kind: 'string', - describe: 'Restrict results to direct children of this parent path.', + describe: + 'Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.', }, search: { kind: 'string', @@ -12058,7 +12059,8 @@ export const V2_OPERATIONS = { }, parentPath: { kind: 'string', - describe: 'Restrict results to direct children of this parent path.', + describe: + 'Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.', }, search: { kind: 'string', @@ -12444,7 +12446,8 @@ export const V2_OPERATIONS = { }, parentPath: { kind: 'string', - describe: 'Restrict results to direct children of this parent path.', + describe: + 'Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.', }, search: { kind: 'string', @@ -12623,7 +12626,8 @@ export const V2_OPERATIONS = { }, parentPath: { kind: 'string', - describe: 'Restrict results to direct children of this parent path.', + describe: + 'Restrict results to direct children of this parent path. A path that names no folder narrows the result to nothing, so the response is an empty page rather than an error.', }, search: { kind: 'string', diff --git a/packages/sim-cli/src/http/client.test.ts b/packages/sim-cli/src/http/client.test.ts index 2c55e49866d..5a6ebd79aac 100644 --- a/packages/sim-cli/src/http/client.test.ts +++ b/packages/sim-cli/src/http/client.test.ts @@ -949,6 +949,10 @@ describe('destructive operations are gated', () => { * default by being named something the old regex did not match. */ const DESTRUCTIVE_NON_DELETE = new Set([ + // The same application operation as `rollbackWorkflow`, under a different + // transition: both switch which version production serves away from the one + // the caller last chose. + 'activateWorkflowVersion', 'applyWorkflowOperations', 'applyWorkflowVariables', 'bulkDeleteFiles', @@ -966,7 +970,6 @@ describe('destructive operations are gated', () => { * decision on anything new. */ const NON_DESTRUCTIVE = new Set([ - 'activateWorkflowVersion', 'addTableColumn', 'addWorkflowGroup', 'addWorkspaceFilesToKnowledgeBase', diff --git a/packages/sim-cli/src/program.ts b/packages/sim-cli/src/program.ts index 999350d71cd..e2e784672d4 100644 --- a/packages/sim-cli/src/program.ts +++ b/packages/sim-cli/src/program.ts @@ -21,7 +21,9 @@ Profiles work like the AWS CLI: settings live in ~/.sim/config, keys in with -P, --profile, or SIM_PROFILE. Workflow, knowledge-base and workspace IDs are bare UUIDs. Table IDs carry a -tbl_ prefix and file IDs a wf_ one, so wf_ never names a workflow. +tbl_ prefix and file IDs a wf_ one, so wf_ never names a workflow. An audit-log +or custom-tool ID can open with a dash, which reads as a flag; put -- in front +of it, as in sim audit-logs get -- -HlDcD1z76nK6R4crsUp0. Examples: $ sim login Authorize the default profile diff --git a/packages/sim-cli/src/runtime/build.test.ts b/packages/sim-cli/src/runtime/build.test.ts index d0f0bbaf72d..59468426151 100644 --- a/packages/sim-cli/src/runtime/build.test.ts +++ b/packages/sim-cli/src/runtime/build.test.ts @@ -326,6 +326,52 @@ describe('commands parsed through commander', () => { expect(errorOutput).not.toContain('--skillId') }) + it('shows the -- escape when an id argument opens with a dash', async () => { + const root = program() + const auditLogs = root.commands.find((command) => command.name() === 'audit-logs') + const get = auditLogs?.commands.find((command) => command.name() === 'get') + if (!get) throw new Error('Missing command audit-logs get') + + let errorOutput = '' + get.configureOutput({ + writeErr: (message) => { + errorOutput += message + }, + }) + + await expect( + root.parseAsync(['node', 'sim', 'audit-logs', 'get', '-HlDcD1z76nK6R4crsUp0']) + ).rejects.toMatchObject({ code: 'commander.unknownOption' }) + expect(errorOutput).toContain("error: unknown option '-HlDcD1z76nK6R4crsUp0'") + expect(errorOutput).toContain('Example: sim audit-logs get -- -HlDcD1z76nK6R4crsUp0') + }) + + it("leaves a misspelt flag with commander's own suggestion", async () => { + const root = program() + const auditLogs = root.commands.find((command) => command.name() === 'audit-logs') + const get = auditLogs?.commands.find((command) => command.name() === 'get') + if (!get) throw new Error('Missing command audit-logs get') + + let errorOutput = '' + get.configureOutput({ + writeErr: (message) => { + errorOutput += message + }, + }) + + await expect( + root.parseAsync(['node', 'sim', 'audit-logs', 'get', 'log_1', '--organisation']) + ).rejects.toMatchObject({ code: 'commander.unknownOption' }) + expect(errorOutput).toContain("error: unknown option '--organisation'") + expect(errorOutput).not.toContain('Example:') + + await expect( + root.parseAsync(['node', 'sim', 'audit-logs', 'get', 'log_1', '-organisation']) + ).rejects.toMatchObject({ code: 'commander.unknownOption' }) + expect(errorOutput).toContain("error: unknown option '-organisation'") + expect(errorOutput).not.toContain('Example:') + }) + it('dispatches generated commands through their singular resource alias', async () => { const [tablePath] = await run(['table', 'list']) expect(tablePath).toBe('/api/v2/tables') diff --git a/packages/sim-cli/src/runtime/build.ts b/packages/sim-cli/src/runtime/build.ts index b387f6420a6..3d80174fbe6 100644 --- a/packages/sim-cli/src/runtime/build.ts +++ b/packages/sim-cli/src/runtime/build.ts @@ -64,18 +64,55 @@ function commandPath(command: Command): string { return names.join(' ') } -function addMissingArgumentExample(command: Command): Command { +const UNKNOWN_OPTION_TOKEN = /^error: unknown option '(.+?)'/ + +/** + * Whether an unknown-option token reads as a resource id rather than a flag. + * + * `generateShortId` draws from a 64-character alphabet holding exactly one + * `-`, so one id in 64 opens with a dash and commander parses it as an option + * instead of the positional it was typed as — `audit-logs get` and + * `custom-tools get/update/delete` all take such an id. A flag on this surface + * is either a single-character short (`-w`) or a lowercase kebab-case long + * (`--dry-run`), so a lone dash followed by two or more characters of which at + * least one is an uppercase letter or a digit is not a flag any caller meant + * to type. `--organisation` and every other misspelt flag keeps the plain + * error and commander's own suggestion. + */ +function looksLikeAnId(token: string): boolean { + return ( + token.length > 2 && + !token.startsWith('--') && + /^-[A-Za-z0-9_-]*[A-Z0-9][A-Za-z0-9_-]*$/.test(token) + ) +} + +/** + * Appends a worked example to the parse errors a positional argument causes. + * + * Covers the argument being absent and the argument being swallowed as an + * option because its id opens with a dash; the second needs the `--` escape, + * which commander never mentions. + */ +function addArgumentExamples(command: Command): Command { const outputError = command.configureOutput().outputError if (!outputError) throw new Error('Commander output formatter is not configured') command.configureOutput({ outputError: (message, write) => { outputError(message, write) - if (!message.startsWith('error: missing required argument ')) return - const syntax = argumentSyntax(command) - const example = syntax ? `${commandPath(command)} ${syntax}` : commandPath(command) - write(`Example: ${example}\n`) + if (message.startsWith('error: missing required argument ')) { + const syntax = argumentSyntax(command) + const example = syntax ? `${commandPath(command)} ${syntax}` : commandPath(command) + write(`Example: ${example}\n`) + return + } + + if (command.registeredArguments.length === 0) return + const token = UNKNOWN_OPTION_TOKEN.exec(message)?.[1] + if (!token || !looksLikeAnId(token)) return + write(`Example: ${commandPath(command)} -- ${token}\n`) }, }) return command @@ -298,7 +335,7 @@ function configureOperation( } function buildLeaf(operation: V2OperationName, spec: CommandSpec, leafName: string): Command { - return addMissingArgumentExample(configureOperation(new Command(leafName), operation, spec)) + return addArgumentExamples(configureOperation(new Command(leafName), operation, spec)) } /** diff --git a/packages/sim-cli/src/runtime/execute.test.ts b/packages/sim-cli/src/runtime/execute.test.ts index 8750c52ded4..ad0e715ec3c 100644 --- a/packages/sim-cli/src/runtime/execute.test.ts +++ b/packages/sim-cli/src/runtime/execute.test.ts @@ -73,6 +73,19 @@ const MOVE_WORKFLOWS: OperationSpec = { const MOVE_FLAGS = { workflow: ['wf_1'], to: '/a' } +const DELETE_TABLE_ROWS: OperationSpec = { + method: 'DELETE', + path: '/api/v2/tables/[tableId]/rows', + pathParams: ['tableId'], + body: { rowIds: { kind: 'array' }, filter: { kind: 'unknown' } }, +} + +/** Invokes a generated command that takes both a path positional and flags. */ +function invokeRowDelete(flags: Record) { + const host = new Command('leaf') + return executeOperation('deleteTableRows', {}, DELETE_TABLE_ROWS, ['tbl_1', flags, host]) +} + /** Invokes a generated command that takes its input from flags rather than positionals. */ function invokeWithFlags( operation: 'bulkDeleteTables' | 'bulkDeleteFiles' | 'moveTables' | 'moveWorkflows', @@ -339,6 +352,48 @@ describe('a bulk call that changed nothing', () => { await expect(invokeWithFlags('moveTables', MOVE_TABLES, {})).resolves.toBeUndefined() }) + + it('fails the process when no requested row was deleted', async () => { + request.mockResolvedValue({ + data: { + deletedCount: 0, + deletedRowIds: [], + requestedCount: 1, + missingRowIds: ['00000000-0000-0000-0000-000000000000'], + }, + }) + + await expect( + invokeRowDelete({ row: ['00000000-0000-0000-0000-000000000000'] }) + ).rejects.toThrow(/Deleted nothing: none of the 1 requested row was deleted\./) + }) + + it('succeeds on a partial row delete', async () => { + request.mockResolvedValue({ + data: { + deletedCount: 1, + deletedRowIds: ['row_1'], + requestedCount: 2, + missingRowIds: ['row_gone'], + }, + }) + + await expect(invokeRowDelete({ row: ['row_1', 'row_gone'] })).resolves.toBeUndefined() + }) + + /** + * The selection mode the guard must not touch. A filter answers with a deleted + * count and no `requestedCount`, and a filter that matches nothing deleted + * nothing because there was nothing left to delete — the second run of an + * idempotent sweep, not a failure. + */ + it('succeeds when a filter matched no rows', async () => { + request.mockResolvedValue({ data: { deletedCount: 0, deletedRowIds: [] } }) + + await expect( + invokeRowDelete({ filter: { all: [{ field: 'status', op: 'eq', value: 'archived' }] } }) + ).resolves.toBeUndefined() + }) }) const BULK_UPDATE_CHUNKS: OperationSpec = { diff --git a/packages/sim-cli/src/runtime/execute.ts b/packages/sim-cli/src/runtime/execute.ts index 79be86a713d..031293d8288 100644 --- a/packages/sim-cli/src/runtime/execute.ts +++ b/packages/sim-cli/src/runtime/execute.ts @@ -129,6 +129,19 @@ export const BULK_OUTCOME_CHECKS: Readonly { + if (countOf(payload.deletedCount) > 0) return null + const requested = countOf(payload.requestedCount) + if (requested === 0) return null + return `Deleted nothing: none of the ${requested} requested ${requested === 1 ? 'row was' : 'rows were'} deleted.` + }, moveTables: (payload) => { if (lengthOf(payload.moved) > 0) return null const missed = lengthOf(payload.notFound) + lengthOf(payload.failed)