fix(mothership): canonical folder ids (#5741)

* folder fix

* fix bug
This commit is contained in:
Siddharth Ganesan
2026-07-17 13:13:15 -07:00
committed by GitHub
parent 92feb24cb9
commit 772b710e39
5 changed files with 154 additions and 31 deletions
@@ -439,7 +439,11 @@ export const CreateWorkflow: ToolCatalogEntry = {
parameters: {
type: 'object',
properties: {
folderId: { type: 'string', description: 'Optional folder ID.' },
folderPath: {
type: 'string',
description:
'Optional canonical workflow-folder VFS path copied from glob("workflows/**"), for example "workflows/Dream" or "workflows/Client%20Work/Intake". Omit for the workspace root.',
},
name: { type: 'string', description: 'Workflow name.' },
workspaceId: { type: 'string', description: 'Optional workspace ID.' },
},
@@ -221,9 +221,10 @@ export const TOOL_RUNTIME_SCHEMAS: Record<string, ToolRuntimeSchemaEntry> = {
parameters: {
type: 'object',
properties: {
folderId: {
folderPath: {
type: 'string',
description: 'Optional folder ID.',
description:
'Optional canonical workflow-folder VFS path copied from glob("workflows/**"), for example "workflows/Dream" or "workflows/Client%20Work/Intake". Omit for the workspace root.',
},
name: {
type: 'string',
@@ -32,6 +32,9 @@ export interface GetBlockUpstreamReferencesParams {
export interface CreateWorkflowParams {
name?: string
workspaceId?: string
/** Canonical workflow-folder VFS path, for example `workflows/Dream`. */
folderPath?: string
/** Legacy executor input. New tool calls use folderPath and resolve the ID internally. */
folderId?: string
}
@@ -124,7 +124,7 @@ vi.mock('../access', () => ({
import { applyCreateWorkflowOutputToContext } from '@/lib/copilot/request/tools/workflow-context'
import { performUpdateWorkflow } from '@/lib/workflows/orchestration'
import { verifyFolderWorkspace } from '@/lib/workflows/utils'
import { listFolders, verifyFolderWorkspace } from '@/lib/workflows/utils'
import {
executeCreateWorkflow,
executeMoveWorkflow,
@@ -136,6 +136,7 @@ import {
} from './mutations'
const performUpdateWorkflowMock = vi.mocked(performUpdateWorkflow)
const listFoldersMock = vi.mocked(listFolders)
const verifyFolderWorkspaceMock = vi.mocked(verifyFolderWorkspace)
const billingAttribution: BillingAttributionSnapshot = {
actorUserId: 'user-1',
@@ -291,6 +292,7 @@ describe('executeCreateWorkflow billing attribution', () => {
payerUsage: { currentUsage: 1, limit: 10 },
})
reserveExecutionSlotMock.mockResolvedValue({ reserved: true, created: true })
listFoldersMock.mockResolvedValue([])
})
it('ignores legacy description input instead of persisting it', async () => {
@@ -320,6 +322,89 @@ describe('executeCreateWorkflow billing attribution', () => {
})
})
it('canonicalizes a workflow-folder VFS path and resolves its internal ID', async () => {
listFoldersMock.mockResolvedValue([
{ folderId: 'folder-dream', folderName: 'Dream', parentId: null },
{
folderId: 'folder-launch-plans',
folderName: 'Launch Plans',
parentId: 'folder-dream',
},
])
performCreateWorkflowMock.mockResolvedValue({
success: true,
workflow: {
id: 'created-workflow',
name: 'Created Workflow',
workspaceId: 'workspace-1',
folderId: 'folder-launch-plans',
},
})
const result = await executeCreateWorkflow(
{
name: 'Created Workflow',
workspaceId: 'workspace-1',
folderPath: 'workflows/Dream/Launch%20Plans',
},
executionContext
)
expect(result.success).toBe(true)
expect(performCreateWorkflowMock).toHaveBeenCalledWith({
userId: 'user-1',
workspaceId: 'workspace-1',
name: 'Created Workflow',
folderId: 'folder-launch-plans',
})
expect(workflowAuthzMockFns.mockAssertFolderMutable).toHaveBeenCalledWith('folder-launch-plans')
})
it('fails clearly when a workflow-folder VFS path does not exist', async () => {
listFoldersMock.mockResolvedValue([
{ folderId: 'folder-existing', folderName: 'Existing', parentId: null },
])
const result = await executeCreateWorkflow(
{
name: 'Created Workflow',
workspaceId: 'workspace-1',
folderPath: 'workflows/Dream',
},
executionContext
)
expect(result).toEqual({
success: false,
error: 'Folder not found at workflows/Dream',
})
expect(performCreateWorkflowMock).not.toHaveBeenCalled()
})
it('rejects canonically ambiguous workflow-folder VFS paths', async () => {
listFoldersMock.mockResolvedValue([
{ folderId: 'folder-cafe-nfc', folderName: 'Caf\u00e9', parentId: null },
{ folderId: 'folder-cafe-nfd', folderName: 'Cafe\u0301', parentId: null },
])
const result = await executeCreateWorkflow(
{
name: 'Created Workflow',
workspaceId: 'workspace-1',
folderPath: 'workflows/Caf%C3%A9',
},
executionContext
)
expect(result).toEqual({
success: false,
error:
'Folder path is ambiguous after canonicalization: workflows/Caf%C3%A9. Rename one of the conflicting folders and retry.',
})
expect(performCreateWorkflowMock).not.toHaveBeenCalled()
expect(workflowAuthzMockFns.mockAssertFolderMutable).not.toHaveBeenCalled()
})
it('keeps same-workspace creation and subsequent execution on the immutable payer', async () => {
const context: ExecutionContext = { ...executionContext, workflowId: '' }
performCreateWorkflowMock.mockResolvedValue({
@@ -393,9 +393,23 @@ export async function executeCreateWorkflow(
}
const workspaceId =
params?.workspaceId || context.workspaceId || (await getDefaultWorkspaceId(context.userId))
const folderId = params?.folderId || null
await ensureWorkspaceAccess(workspaceId, context.userId, 'write')
const folderPath = typeof params?.folderPath === 'string' ? params.folderPath.trim() : ''
let folderId =
typeof params?.folderId === 'string' && params.folderId.trim() ? params.folderId.trim() : null
if (folderPath) {
const relativePath = workflowFolderRelativePath(folderPath)
if (!relativePath) {
folderId = null
} else {
const target = resolveFolderIdByPath(folderPath, await loadFolderPathIndex(workspaceId))
if ('error' in target) return { success: false, error: target.error }
folderId = target.folderId
}
}
await assertFolderMutable(folderId)
assertWorkflowMutationNotAborted(context)
@@ -1230,40 +1244,57 @@ function workflowFolderRelativePath(rawPath: string): string {
return trimmed.startsWith('workflows/') ? trimmed.slice('workflows/'.length) : trimmed
}
type FolderPathIndex = Map<string, string | null>
/**
* Load a lookup from each folder's canonical encoded VFS path to its id by
* inverting the same {@link buildVfsFolderPathMap} the VFS uses to serve folder
* paths, so a path the agent sees via glob round-trips to the right id. Fetched
* once per manage_folder call and reused across target + parent resolution.
* Load an index from each canonical encoded VFS path to its folder id. A null
* value records that multiple folder ids collapse to the same canonical path,
* so callers can reject the ambiguous path instead of silently choosing one.
*/
async function loadFolderPathToIdMap(workspaceId: string): Promise<Map<string, string>> {
const byPath = new Map<string, string>()
async function loadFolderPathIndex(workspaceId: string): Promise<FolderPathIndex> {
const byPath: FolderPathIndex = new Map()
for (const [folderId, encodedPath] of buildVfsFolderPathMap(
await listFolders(workspaceId)
).entries()) {
byPath.set(encodedPath, folderId)
if (!byPath.has(encodedPath)) {
byPath.set(encodedPath, folderId)
} else if (byPath.get(encodedPath) !== folderId) {
byPath.set(encodedPath, null)
}
}
return byPath
}
function lookupFolderIdByPath(rawPath: string, byPath: Map<string, string>): string | null {
function resolveFolderIdByPath(
rawPath: string,
byPath: FolderPathIndex,
label = 'Folder'
): { folderId: string } | { error: string } {
const relative = workflowFolderRelativePath(rawPath)
if (!relative) return null
return byPath.get(encodeVfsPathSegments(decodeVfsPathSegments(relative))) ?? null
if (!relative) return { error: `${label} not found at ${rawPath}` }
const canonicalPath = encodeVfsPathSegments(decodeVfsPathSegments(relative))
if (!byPath.has(canonicalPath)) return { error: `${label} not found at ${rawPath}` }
const folderId = byPath.get(canonicalPath)
if (!folderId) {
return {
error: `${label} path is ambiguous after canonicalization: ${rawPath}. Rename one of the conflicting folders and retry.`,
}
}
return { folderId }
}
/** Resolve the folder a manage_folder op targets, preferring folderId over path. */
async function resolveManageFolderTarget(
params: ManageFolderParams,
getFolderPaths: () => Promise<Map<string, string>>
getFolderPaths: () => Promise<FolderPathIndex>
): Promise<{ folderId: string } | { error: string }> {
const directId = typeof params.folderId === 'string' ? params.folderId.trim() : ''
if (directId) return { folderId: directId }
const path = typeof params.path === 'string' ? params.path.trim() : ''
if (!path) return { error: 'Provide the folder path (e.g. "workflows/Marketing") or folderId.' }
const folderId = lookupFolderIdByPath(path, await getFolderPaths())
if (!folderId) return { error: `Folder not found at ${path}` }
return { folderId }
return resolveFolderIdByPath(path, await getFolderPaths())
}
/**
@@ -1273,16 +1304,16 @@ async function resolveManageFolderTarget(
*/
async function resolveManageFolderParent(
params: ManageFolderParams,
getFolderPaths: () => Promise<Map<string, string>>
getFolderPaths: () => Promise<FolderPathIndex>
): Promise<{ parentId: string | null } | { error: string }> {
const directId = typeof params.parentId === 'string' ? params.parentId.trim() : ''
if (directId) return { parentId: directId }
if (params.parentId === null) return { parentId: null }
const dest = typeof params.destinationPath === 'string' ? params.destinationPath.trim() : ''
if (!dest || !workflowFolderRelativePath(dest)) return { parentId: null }
const parentId = lookupFolderIdByPath(dest, await getFolderPaths())
if (!parentId) return { error: `Destination folder not found at ${dest}` }
return { parentId }
const parent = resolveFolderIdByPath(dest, await getFolderPaths(), 'Destination folder')
if ('error' in parent) return parent
return { parentId: parent.folderId }
}
/**
@@ -1302,8 +1333,8 @@ export async function executeManageFolder(
// Fetch the workspace folder list at most once, lazily — only when a path
// (vs an explicit id) actually needs resolving, and shared across the
// target + parent lookups a single move/create performs.
let folderPathsPromise: Promise<Map<string, string>> | undefined
const getFolderPaths = () => (folderPathsPromise ??= loadFolderPathToIdMap(workspaceId))
let folderPathsPromise: Promise<FolderPathIndex> | undefined
const getFolderPaths = () => (folderPathsPromise ??= loadFolderPathIndex(workspaceId))
switch (operation) {
case 'create': {
@@ -1318,14 +1349,13 @@ export async function executeManageFolder(
name = segments[segments.length - 1]
const parentSegments = segments.slice(0, -1)
if (parentSegments.length > 0) {
const resolved = lookupFolderIdByPath(
const parent = resolveFolderIdByPath(
encodeVfsPathSegments(parentSegments),
await getFolderPaths()
await getFolderPaths(),
'Parent folder'
)
if (!resolved) {
return { success: false, error: `Parent folder not found for ${path}` }
}
parentId = resolved
if ('error' in parent) return { success: false, error: parent.error }
parentId = parent.folderId
}
} else {
const parent = await resolveManageFolderParent(params, getFolderPaths)