Skip to content

Commit f07194a

Browse files
committed
fix(folders): reach nested stragglers on retry and confine locking to workflows
Two follow-ons from the cascade reordering. The delete subtree walk still collected active folders only. Since the cascade now stamps folders before children, a failure during the child pass leaves intermediate folders archived, and an active-only walk drops them — so resources still live underneath were never reached on the retry and stayed outside every later timestamp-matched restore. The walk now admits folders that are active OR already carry this cascade's own timestamp, which reaches the stragglers while still excluding folders archived independently under a different stamp. Locking was only half-confined to workflows. `assertFolderMutable` was gated but the admin check and the `locked` write were not, so an admin could persist `locked` on a knowledge-base or table folder — a column nothing reads for those types — and a non-admin update that merely mentioned `locked` was rejected on a type where locking does not exist. Locking is now a declared capability on the resource config; the routes reject the field outright for types that lack it, and the engine refuses to write it as a backstop for the copilot tools that call it directly.
1 parent 7fe4e03 commit f07194a

8 files changed

Lines changed: 127 additions & 32 deletions

File tree

apps/sim/app/api/folders/[id]/route.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,26 @@ describe('Individual Folder API Route', () => {
244244
expect(data).toHaveProperty('folder')
245245
})
246246

247+
it('rejects a locked write on a resource type that has no lock semantics', async () => {
248+
mockAuthenticatedUser()
249+
queueFolderLookup()
250+
251+
const req = createMockRequest(
252+
'PUT',
253+
{ locked: true },
254+
{},
255+
'http://localhost:3000/api/folders/folder-1?resourceType=knowledge_base'
256+
)
257+
const params = Promise.resolve({ id: 'folder-1' })
258+
259+
const response = await PUT(req, { params })
260+
261+
expect(response.status).toBe(400)
262+
const data = await response.json()
263+
expect(data.error).toBe('Folder locking is only supported for workflow folders')
264+
expect(mockUpdateFolder).not.toHaveBeenCalled()
265+
})
266+
247267
it('should return 400 when trying to set folder as its own parent', async () => {
248268
mockAuthenticatedUser()
249269

apps/sim/app/api/folders/[id]/route.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { parseRequest } from '@/lib/api/server'
99
import { getSession } from '@/lib/auth'
1010
import { HttpError } from '@/lib/core/utils/http-error'
1111
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
12+
import { folderResourceConfig } from '@/lib/folders/config'
1213
import { deleteFolder, updateFolder } from '@/lib/folders/lifecycle'
1314
import { toFolderApi } from '@/lib/folders/queries'
1415
import { folderMutationStatus } from '@/lib/folders/status'
@@ -68,15 +69,27 @@ export const PUT = withRouteHandler(
6869
)
6970
}
7071

71-
if (locked !== undefined && workspacePermission !== 'admin') {
72+
// Locking is workflow-only. Reject the field outright for the other types rather than
73+
// dropping it silently, and keep the admin gate and the lock checks behind the same
74+
// capability so a non-workflow folder can neither be 403'd by a field that has no
75+
// meaning for it nor persist a `locked` value nothing will ever read.
76+
const supportsLocking = Boolean(folderResourceConfig(resourceType).supportsLocking)
77+
78+
if (locked !== undefined && !supportsLocking) {
7279
return NextResponse.json(
73-
{ error: 'Admin access required to lock folders' },
74-
{ status: 403 }
80+
{ error: 'Folder locking is only supported for workflow folders' },
81+
{ status: 400 }
7582
)
7683
}
7784

78-
// Folder locking is a workflow-only feature; other resource types leave `locked` false.
79-
if (resourceType === 'workflow') {
85+
if (supportsLocking) {
86+
if (locked !== undefined && workspacePermission !== 'admin') {
87+
return NextResponse.json(
88+
{ error: 'Admin access required to lock folders' },
89+
{ status: 403 }
90+
)
91+
}
92+
8093
const hasNonLockUpdate = Object.keys(parsed.data.body).some((key) => key !== 'locked')
8194
if (hasNonLockUpdate) {
8295
await assertFolderMutable(id)

apps/sim/app/api/folders/reorder/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { parseRequest } from '@/lib/api/server'
1010
import { getSession } from '@/lib/auth'
1111
import { generateRequestId } from '@/lib/core/utils/request'
1212
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
13+
import { folderResourceConfig } from '@/lib/folders/config'
1314
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1415

1516
const logger = createLogger('FolderReorderAPI')
@@ -116,7 +117,7 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
116117
}
117118

118119
// Folder locking is a workflow-only feature; other resource types leave `locked` false.
119-
if (resourceType === 'workflow') {
120+
if (folderResourceConfig(resourceType).supportsLocking) {
120121
for (const update of validUpdates) {
121122
await assertFolderMutable(update.id)
122123
if (update.parentId !== undefined) {

apps/sim/app/api/folders/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { createFolderContract, listFoldersContract } from '@/lib/api/contracts'
55
import { parseRequest } from '@/lib/api/server'
66
import { getSession } from '@/lib/auth'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
8+
import { folderResourceConfig } from '@/lib/folders/config'
89
import { createFolder } from '@/lib/folders/lifecycle'
910
import { listFoldersForWorkspace, toFolderApi } from '@/lib/folders/queries'
1011
import { folderMutationStatus } from '@/lib/folders/status'
@@ -78,7 +79,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
7879
}
7980

8081
// Folder locking is a workflow-only feature; other resource types leave `locked` false.
81-
if (resourceType === 'workflow') {
82+
if (folderResourceConfig(resourceType).supportsLocking) {
8283
await assertFolderMutable(parentId ?? null)
8384
}
8485

apps/sim/lib/folders/cascade.test.ts

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
import { beforeEach, describe, expect, it, vi } from 'vitest'
55
import {
66
archiveFolderCascade,
7-
collectActiveSubtreeIds,
87
collectArchivedSubtreeIds,
8+
collectCascadeSubtreeIds,
99
type DbOrTx,
1010
restoreFolderCascade,
1111
restoreFolderRows,
@@ -101,8 +101,8 @@ function hasCondition(
101101
const TIMESTAMP = new Date('2026-01-01T00:00:00.000Z')
102102
const NOW = new Date('2026-02-02T00:00:00.000Z')
103103

104-
describe('collectActiveSubtreeIds', () => {
105-
it('returns the folder plus every active descendant', async () => {
104+
describe('collectCascadeSubtreeIds', () => {
105+
it('returns the folder plus every descendant in the cascade', async () => {
106106
const { tx, selectCalls } = makeTx({
107107
selects: [
108108
[
@@ -114,17 +114,41 @@ describe('collectActiveSubtreeIds', () => {
114114
],
115115
})
116116

117-
const ids = await collectActiveSubtreeIds(tx, 'ws-1', 'table', 'root')
117+
const ids = await collectCascadeSubtreeIds(tx, 'ws-1', 'table', 'root', TIMESTAMP)
118118

119119
expect(ids).toEqual(['root', 'child', 'grandchild'])
120120
expect(selectCalls).toHaveLength(1)
121-
expect(hasCondition(selectCalls[0].where, (node) => node.type === 'isNull')).toBe(true)
121+
})
122+
123+
it('admits folders already stamped by this cascade so a retry reaches nested stragglers', async () => {
124+
// The cascade stamps folders before children, so a failure during the child pass leaves
125+
// intermediate folders archived. An active-only walk would drop `child` here and never
126+
// reach the resources still live under it.
127+
const { tx, selectCalls } = makeTx({
128+
selects: [
129+
[
130+
{ id: 'root', parentId: null },
131+
{ id: 'child', parentId: 'root' },
132+
{ id: 'grandchild', parentId: 'child' },
133+
],
134+
],
135+
})
136+
137+
const ids = await collectCascadeSubtreeIds(tx, 'ws-1', 'table', 'root', TIMESTAMP)
138+
139+
expect(ids).toEqual(['root', 'child', 'grandchild'])
140+
// Either still active, or carrying this cascade's own stamp — never another snapshot's.
141+
const clause = flattenConditions(selectCalls[0].where).find((node) => node.type === 'or')
142+
expect(clause).toBeDefined()
143+
const branches = (clause?.conditions ?? []) as Array<Record<string, unknown>>
144+
expect(branches.some((node) => node.type === 'isNull')).toBe(true)
145+
expect(branches.some((node) => node.right === TIMESTAMP)).toBe(true)
122146
})
123147

124148
it('scopes the walk to the workspace and resourceType', async () => {
125149
const { tx, selectCalls } = makeTx({ selects: [[]] })
126150

127-
await collectActiveSubtreeIds(tx, 'ws-1', 'knowledge_base', 'root')
151+
await collectCascadeSubtreeIds(tx, 'ws-1', 'knowledge_base', 'root', TIMESTAMP)
128152

129153
expect(hasCondition(selectCalls[0].where, (node) => node.right === 'knowledge_base')).toBe(true)
130154
expect(hasCondition(selectCalls[0].where, (node) => node.right === 'ws-1')).toBe(true)
@@ -395,6 +419,15 @@ describe('FOLDER_RESOURCES', () => {
395419
}
396420
})
397421

422+
it('grants lock semantics to workflows only', () => {
423+
// `folder.locked` predates the generic table and is deliberately not extended. Anything
424+
// that flips a second resource to lockable has to add the UI and authz to match.
425+
const lockable = Object.values(FOLDER_RESOURCES)
426+
.filter((config) => config.supportsLocking)
427+
.map((config) => config.resourceType)
428+
expect(lockable).toEqual(['workflow'])
429+
})
430+
398431
it('guards the delete of resources that gate their own deletion', () => {
399432
// Tables refuse deletion while delete-locked; deleting the folder around one must not
400433
// become a way around that control.

apps/sim/lib/folders/cascade.ts

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { db } from '@sim/db'
22
import { folder as folderTable } from '@sim/db/schema'
3-
import { and, eq, inArray, isNull, type SQL } from 'drizzle-orm'
3+
import { and, eq, inArray, isNull, or, type SQL } from 'drizzle-orm'
44
import type { FolderCascadeCountsApi, FolderResourceType } from '@/lib/api/contracts/folders'
55
import type { FolderResourceConfig } from '@/lib/folders/config'
66
import { collectDescendantFolderIds } from '@/lib/folders/subtree'
@@ -16,28 +16,36 @@ export interface FolderCascadeCounts {
1616
}
1717

1818
/**
19-
* Resolves the folder plus every *active* descendant, in one query. Used by delete: an
20-
* archived descendant was archived independently and keeps its own timestamp, so it must
21-
* not be swept into this cascade.
19+
* Resolves the folder plus every descendant that belongs to this delete cascade, in one
20+
* query: folders still active, OR already stamped with this cascade's own `timestamp`.
21+
*
22+
* Both halves are load-bearing. Excluding other archived folders is what keeps a descendant
23+
* archived independently — with its own timestamp — from being swept into this snapshot.
24+
* Including folders stamped with *this* timestamp is what makes a retry work: the cascade
25+
* stamps folders before children, so a failure partway through the child pass leaves nested
26+
* subfolders already archived. An active-only walk would drop those intermediate folders and
27+
* never reach the still-active resources beneath them, leaving them outside every future
28+
* timestamp-matched restore.
2229
*/
23-
export async function collectActiveSubtreeIds(
30+
export async function collectCascadeSubtreeIds(
2431
tx: DbOrTx,
2532
workspaceId: string,
2633
resourceType: FolderResourceType,
27-
folderId: string
34+
folderId: string,
35+
timestamp: Date
2836
): Promise<string[]> {
29-
const activeFolders = await tx
37+
const cascadeFolders = await tx
3038
.select({ id: folderTable.id, parentId: folderTable.parentId })
3139
.from(folderTable)
3240
.where(
3341
and(
3442
eq(folderTable.workspaceId, workspaceId),
3543
eq(folderTable.resourceType, resourceType),
36-
isNull(folderTable.deletedAt)
44+
or(isNull(folderTable.deletedAt), eq(folderTable.deletedAt, timestamp))
3745
)
3846
)
3947

40-
return [folderId, ...collectDescendantFolderIds(activeFolders, folderId)]
48+
return [folderId, ...collectDescendantFolderIds(cascadeFolders, folderId)]
4149
}
4250

4351
/**

apps/sim/lib/folders/config.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,16 @@ export interface FolderResourceConfig {
8989
* hand the cascade a `Record<string, unknown>` literal.
9090
*/
9191
buildSoftDeleteSet: (timestamp: Date | null, now: Date) => Record<string, unknown>
92+
/**
93+
* Whether folders of this type participate in the folder-locking feature.
94+
*
95+
* Only workflow folders do. `folder.locked` exists because workflow-folder locking shipped
96+
* before the generic table; it is deliberately not extended to the other resource types.
97+
* Declared here rather than checked as `resourceType === 'workflow'` at each call site, so
98+
* every surface that touches locking asks the same question and a future lockable resource
99+
* is one flag rather than a hunt through routes.
100+
*/
101+
supportsLocking?: boolean
92102
/** Narrows which rows of `table` participate in folder membership at all. */
93103
scope?: SQL
94104
/**
@@ -404,6 +414,7 @@ export const FOLDER_RESOURCES: Record<FolderResourceType, FolderResourceConfig>
404414
>,
405415
},
406416
],
417+
supportsLocking: true,
407418
archiveChildren: archiveWorkflowChildren,
408419
guardDelete: guardLastWorkflows,
409420
},

apps/sim/lib/folders/lifecycle.ts

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ import { and, eq, isNull, min } from 'drizzle-orm'
88
import type { FolderCascadeCountsApi, FolderResourceType } from '@/lib/api/contracts/folders'
99
import {
1010
archiveFolderCascade,
11-
collectActiveSubtreeIds,
1211
collectArchivedSubtreeIds,
12+
collectCascadeSubtreeIds,
1313
restoreFolderChildren,
1414
restoreFolderRows,
1515
toCascadeCounts,
@@ -235,6 +235,8 @@ export async function createFolder(params: CreateFolderParams): Promise<FolderMu
235235
}
236236

237237
export async function updateFolder(params: UpdateFolderParams): Promise<FolderMutationResult> {
238+
const config = folderResourceConfig(params.resourceType)
239+
238240
try {
239241
if (params.parentId && params.parentId === params.folderId) {
240242
return { success: false, error: 'Folder cannot be its own parent', errorCode: 'validation' }
@@ -266,7 +268,9 @@ export async function updateFolder(params: UpdateFolderParams): Promise<FolderMu
266268
// let `color`/`isExpanded` survive an earlier cutover after the create path dropped them.
267269
const updates: Partial<typeof folderTable.$inferInsert> = { updatedAt: new Date() }
268270
if (params.name !== undefined) updates.name = params.name.trim()
269-
if (params.locked !== undefined) updates.locked = params.locked
271+
// Backstop for the route's rejection: the engine is also reachable from the copilot
272+
// tools, and a `locked` value on a type that has no lock semantics must never persist.
273+
if (params.locked !== undefined && config.supportsLocking) updates.locked = params.locked
270274
if (params.parentId !== undefined) updates.parentId = params.parentId || null
271275
if (params.sortOrder !== undefined) updates.sortOrder = params.sortOrder
272276

@@ -332,20 +336,24 @@ export async function deleteFolder(params: DeleteFolderParams): Promise<DeleteFo
332336
return { success: false, error: 'Folder not found', errorCode: 'not_found' }
333337
}
334338

335-
const folderIds = await collectActiveSubtreeIds(db, workspaceId, resourceType, folderId)
339+
// Resolve the timestamp before the subtree, because the subtree walk needs it: on a retry
340+
// it is what distinguishes folders this cascade already stamped from folders archived
341+
// independently.
342+
const timestamp = existing.deletedAt ?? new Date()
343+
const folderIds = await collectCascadeSubtreeIds(
344+
db,
345+
workspaceId,
346+
resourceType,
347+
folderId,
348+
timestamp
349+
)
336350

337351
const rejection = await config.guardDelete?.({ workspaceId, folderIds })
338352
if (rejection) {
339353
return { success: false, error: rejection.error, errorCode: rejection.errorCode }
340354
}
341355

342-
const counts = await archiveFolderCascade(
343-
db,
344-
config,
345-
workspaceId,
346-
folderIds,
347-
existing.deletedAt ?? new Date()
348-
)
356+
const counts = await archiveFolderCascade(db, config, workspaceId, folderIds, timestamp)
349357

350358
logger.info('Deleted folder and all contents', { folderId, resourceType, counts })
351359

0 commit comments

Comments
 (0)