fix(folders): fix migration-breaking prod data collision and KB unlock bug

- The backfill INSERT for workflow_folder assumed (workspace_id, parent_id,
  name) was unique, but that table has no such constraint -- verified
  against production via PlanetScale: 33 groups of genuine active
  duplicates exist today (up to 8x in one workspace), which would have
  aborted the entire migration with a unique-violation the moment it ran
  against prod data. Backfill now deduplicates active rows deterministically
  (ordered by created_at, then id) appending " (N)" per collision, matching
  the app's own "New folder (N)" dedup convention -- archived rows are
  exempt from the new constraint and are never renamed. Verified live: a
  reproduction of the exact prod collision shape backfills correctly, and
  the full 0000-0258 sequence still applies cleanly from scratch
- Knowledge base unlock was completely broken: PUT /api/knowledge/[id]
  always builds a full literal update object with every field present
  (unset ones as `undefined`), so Object.keys(updates) always included
  name/description/etc regardless of what was actually sent, making the
  "is this a lock-only update" check permanently true -- an admin could
  never unlock a KB once locked (directly or via an ancestor folder), since
  the mutability check ran against the still-locked current row before the
  unlock could apply. Fixed to check `!== undefined` per field, matching
  the isLockOnlyUpdate pattern already used correctly by renameTable() and
  performRenameWorkspaceFile(). Added a regression test using the exact
  object shape the route actually constructs -- the existing test never
  caught this since it called the function with a clean `{ locked: false }`
  object that doesn't reproduce the route's actual literal-object shape
- Trimmed 3 files' leftover WHAT-comments (pure restatement of the code
  below them) as part of a full comment-quality pass across the diff

Author: waleedlatif1

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

@@ -15,7 +15,6 @@ import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
 
 const logger = createLogger('FoldersIDAPI')
 
-// PUT - Update a folder
 export const PUT = withRouteHandler(
   async (request: NextRequest, context: { params: Promise<{ id: string }> }) => {
     try {
@@ -41,7 +40,6 @@ export const PUT = withRouteHandler(
       const { id } = parsed.data.params
       const { name, locked, parentId, sortOrder } = parsed.data.body
 
-      // Verify the folder exists
       const existingFolder = await db
         .select()
         .from(folder)
@@ -52,7 +50,6 @@ export const PUT = withRouteHandler(
         return NextResponse.json({ error: 'Folder not found' }, { status: 404 })
       }
 
-      // Check if user has write permissions for the workspace
       const workspacePermission = await getUserEntityPermissions(
         session.user.id,
         'workspace',
@@ -120,7 +117,6 @@ export const PUT = withRouteHandler(
   }
 )
 
-// DELETE - Delete a folder and all its contents
 export const DELETE = withRouteHandler(
   async (request: NextRequest, { params }: { params: Promise<{ id: string }> }) => {
     try {
@@ -131,7 +127,6 @@ export const DELETE = withRouteHandler(
 
       const { id } = await params
 
-      // Verify the folder exists
       const existingFolder = await db
         .select()
         .from(folder)

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

@@ -20,7 +20,6 @@ function folderMutationStatus(errorCode: string | undefined): number {
   return 500
 }
 
-// GET - Fetch folders for a workspace
 export const GET = withRouteHandler(async (request: NextRequest) => {
   try {
     const session = await getSession()
@@ -32,7 +31,6 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
     if (!parsed.success) return parsed.response
     const { workspaceId, resourceType, scope } = parsed.data.query
 
-    // Check if user has workspace permissions
     const workspacePermission = await getUserEntityPermissions(
       session.user.id,
       'workspace',
@@ -52,7 +50,6 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
   }
 })
 
-// POST - Create a new folder
 export const POST = withRouteHandler(async (request: NextRequest) => {
   try {
     const session = await getSession()

apps/sim/lib/folders/orchestration.ts

@@ -756,18 +756,11 @@ export async function performReorderFolders(
   const validUpdates = updates.filter((u) => validIds.has(u.id))
   if (validUpdates.length === 0) return { success: false, updated: 0 }
 
-  // A reparent (non-null parentId) must independently be validated against
-  // `assertFolderParentValid` — the `validIds` check above only confirms `id`
-  // itself belongs to this workspace/resourceType, not that the *new parent*
-  // does. This is an all-or-nothing check: ANY invalid parentId fails the
-  // whole batch before the transaction runs (rather than silently skipping
-  // that one entry and reporting a partial `updated` count as success) —
-  // matching this endpoint's pre-generalization behavior, where a bad
-  // reparent surfaced as `Parent folder not found` up front.
-  //
-  // A subtree drag-drop can reparent many folders in one call, so the
-  // distinct target parentIds are validated concurrently (one round-trip per
-  // distinct parent, not one per update) rather than sequentially.
+  // Reparents also need `assertFolderParentValid` on the new parent (the
+  // `validIds` check above only validates `id`). Any invalid parentId fails
+  // the whole batch up front rather than silently skipping that entry.
+  // Distinct target parentIds are validated concurrently since a subtree
+  // drag-drop can reparent many folders in one call.
   const targetParentIds = Array.from(
     new Set(validUpdates.map((u) => u.parentId).filter((id): id is string => Boolean(id)))
   )

apps/sim/lib/knowledge/service.test.ts

@@ -273,4 +273,30 @@ describe('updateKnowledgeBase — resource-lock enforcement', () => {
 
     expect(mockAssertResourceMutable).not.toHaveBeenCalled()
   })
+
+  it('skips the lock check when unlocking via the route-shaped object (all keys present, unset ones undefined)', async () => {
+    // Regression test: apps/sim/app/api/knowledge/[id]/route.ts always builds a full
+    // literal object (`{ name: validatedData.name, ..., locked: validatedData.locked }`)
+    // rather than spreading only the fields the client actually sent. `Object.keys()`
+    // includes keys whose value is `undefined`, so a naive `Object.keys(updates).some(...)`
+    // check always sees every field as "provided" and can never detect a lock-only
+    // update — permanently blocking unlock, since the mutability check reads the
+    // still-locked current row. This calls updateKnowledgeBase with that exact shape.
+    dbChainMockFns.limit.mockResolvedValueOnce([{ workspaceId: 'ws-current', userId: 'u-1' }])
+
+    await updateKnowledgeBase(
+      'kb-1',
+      {
+        name: undefined,
+        description: undefined,
+        workspaceId: undefined,
+        folderId: undefined,
+        chunkingConfig: undefined,
+        locked: false,
+      },
+      'req-1'
+    ).catch(() => undefined)
+
+    expect(mockAssertResourceMutable).not.toHaveBeenCalled()
+  })
 })

apps/sim/lib/knowledge/service.ts

@@ -298,7 +298,16 @@ export async function updateKnowledgeBase(
     updateData.chunkingConfig = updates.chunkingConfig
   }
 
-  const hasNonLockUpdate = Object.keys(updates).some((key) => key !== 'locked')
+  // `Object.keys(updates)` can't distinguish "field genuinely provided" from "field
+  // present as undefined" (the route always builds a full literal object) — reuse the
+  // same `!== undefined` checks that already gate `updateData` above, matching the
+  // isLockOnlyUpdate pattern used by renameTable()/performRenameWorkspaceFile().
+  const hasNonLockUpdate =
+    updates.name !== undefined ||
+    updates.description !== undefined ||
+    updates.workspaceId !== undefined ||
+    updates.folderId !== undefined ||
+    updates.chunkingConfig !== undefined
   if (hasNonLockUpdate) {
     await assertResourceMutable('knowledge_base', knowledgeBaseId)
   }

packages/db/migrations/0258_lyrical_flatman.sql

@@ -76,15 +76,46 @@ FOR EACH ROW EXECUTE FUNCTION "folder_parent_resource_type_match"();
 -- Source table also has color/is_expanded columns, but the generic `folder` table dropped
 -- them (no UI consumer for color; is_expanded's real state lives client-side in the
 -- folders Zustand store, never read from the DB), so they are intentionally not carried over.
+--
+-- `workflow_folder` has no uniqueness constraint on (workspace_id, parent_id, name) today,
+-- but the new `folder` table does (folder_workspace_resource_parent_name_active_unique,
+-- scoped to active rows). Production has genuine active duplicates (verified against
+-- prod: 33 groups, mostly default "Folder 1"/"Folder 2" names) that would otherwise abort
+-- this INSERT with a unique-violation. Deduplicate active rows at backfill time by
+-- appending " (N)" per collision, ordered by created_at then id for determinism --
+-- matching the "New folder (N)" convention this app's own create-folder dedup already
+-- uses, so a renamed row reads as expected in the UI. Archived rows are exempt from the
+-- constraint (WHERE deleted_at IS NULL) and are partitioned separately so they're never
+-- renamed.
 INSERT INTO "folder" (id, resource_type, name, user_id, workspace_id, parent_id, locked, sort_order, created_at, updated_at, deleted_at)
-SELECT id, 'workflow', name, user_id, workspace_id, parent_id, locked, sort_order, created_at, updated_at, archived_at
-FROM "workflow_folder";
+SELECT
+	id, 'workflow',
+	CASE WHEN archived_at IS NULL AND rn > 1 THEN name || ' (' || rn || ')' ELSE name END,
+	user_id, workspace_id, parent_id, locked, sort_order, created_at, updated_at, archived_at
+FROM (
+	SELECT *, ROW_NUMBER() OVER (
+		PARTITION BY workspace_id, coalesce(parent_id, ''), name, (archived_at IS NULL)
+		ORDER BY created_at, id
+	) AS rn
+	FROM "workflow_folder"
+) "ranked_workflow_folder";
 --> statement-breakpoint
 -- Backfill: copy existing file folders into the generic table, preserving id verbatim.
--- Source table has no locked column, so use the same default as new rows.
+-- Source table has no locked column, so use the same default as new rows. No active
+-- duplicates exist in production today (verified), but the same defensive dedup as the
+-- workflow backfill above is applied in case one is created before this migration runs.
 INSERT INTO "folder" (id, resource_type, name, user_id, workspace_id, parent_id, locked, sort_order, created_at, updated_at, deleted_at)
-SELECT id, 'file', name, user_id, workspace_id, parent_id, false, sort_order, created_at, updated_at, deleted_at
-FROM "workspace_file_folders";
+SELECT
+	id, 'file',
+	CASE WHEN deleted_at IS NULL AND rn > 1 THEN name || ' (' || rn || ')' ELSE name END,
+	user_id, workspace_id, parent_id, false, sort_order, created_at, updated_at, deleted_at
+FROM (
+	SELECT *, ROW_NUMBER() OVER (
+		PARTITION BY workspace_id, coalesce(parent_id, ''), name, (deleted_at IS NULL)
+		ORDER BY created_at, id
+	) AS rn
+	FROM "workspace_file_folders"
+) "ranked_workspace_file_folders";
 --> statement-breakpoint
 ALTER TABLE "knowledge_base" ADD CONSTRAINT "knowledge_base_folder_id_folder_id_fk" FOREIGN KEY ("folder_id") REFERENCES "public"."folder"("id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint
 ALTER TABLE "user_table_definitions" ADD CONSTRAINT "user_table_definitions_folder_id_folder_id_fk" FOREIGN KEY ("folder_id") REFERENCES "public"."folder"("id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint