fix(zip-uploads): address review follow-ups on the extract path

- Roll back already-uploaded files when an upload fails mid-extraction
  (storage/DB error, quota crossed), so callers and retries never observe
  a partial tree
- Fold reserved system folder names (.changelogs, .plans) into the
  'archive' fallback so extraction can't write into alias-backing
  namespaces or bypass the already-extracted lookup that hides them
- Align the archive byte-sniff budget with the read path's inline text
  cap (5MB), so any mislabeled '.zip' small enough to be read inline is
  sniffed and read instead of dead-ending

Author: Sg312

apps/sim/lib/copilot/tools/handlers/materialize-file.test.ts

@@ -429,6 +429,32 @@ describe('executeMaterializeFile - extract operation', () => {
     expect(mockDecompress).toHaveBeenCalledTimes(1)
   })
 
+  it('folds reserved system folder names into the "archive" fallback folder', async () => {
+    // '.changelogs' / '.plans' back workflow changelog/plan aliases; extraction
+    // must never write into them (and the already-extracted lookup hides them,
+    // so a second extract would silently duplicate).
+    mockFindUpload.mockResolvedValue(
+      zipRow({ displayName: '.changelogs.zip', originalName: '.changelogs.zip' })
+    )
+    mockFetchBuffer.mockResolvedValue(Buffer.from('zip-bytes'))
+    mockDecompress.mockResolvedValue({
+      extracted: [{ id: 'f1', name: 'a.txt', url: '/x', size: 1, type: 'text/plain', key: 'k1' }],
+      skipped: 0,
+      skippedUnsafePaths: [],
+    })
+
+    const result = await executeMaterializeFile(
+      { fileNames: ['.changelogs.zip'], operation: 'extract' },
+      context
+    )
+
+    expect(result.success).toBe(true)
+    expect(mockDecompress).toHaveBeenCalledWith(
+      expect.any(Buffer),
+      expect.objectContaining({ rootFolderSegments: ['archive'] })
+    )
+  })
+
   it('folds degenerate archive names into the "archive" fallback folder', async () => {
     mockFindUpload.mockResolvedValue(zipRow({ displayName: '..zip', originalName: '..zip' }))
     mockFetchBuffer.mockResolvedValue(Buffer.from('zip-bytes'))

apps/sim/lib/copilot/tools/handlers/materialize-file.ts

@@ -14,6 +14,7 @@ import {
 import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types'
 import { findMothershipUploadRowByChatAndName } from '@/lib/copilot/tools/handlers/upload-file-reader'
 import { canonicalWorkspaceFilePath, encodeVfsPathSegments } from '@/lib/copilot/vfs/path-utils'
+import { isReservedWorkflowAliasBackingDisplayPath } from '@/lib/copilot/vfs/workflow-aliases'
 import { getServePathPrefix } from '@/lib/uploads'
 import {
   ArchiveError,
@@ -303,6 +304,9 @@ async function executeImport(
  * empty), so a hostile upload name like `..zip` or `\x01.zip` lands in the
  * `archive` fallback instead of surfacing a raw internal error — and so the
  * VFS-encoded destination path can be computed before anything is extracted.
+ * Reserved system backing folders (`.changelogs`, `.plans`) also fall back:
+ * extraction must never write into — or hide behind — those namespaces (the
+ * already-extracted lookup skips them, so they'd also duplicate silently).
  */
 function archiveFolderBaseName(displayName: string): string {
   const stripped = displayName
@@ -312,7 +316,15 @@ function archiveFolderBaseName(displayName: string): string {
     .replace(/[\x00-\x1f\x7f]/g, '')
     .replace(/[/\\]/g, '-')
     .trim()
-  return !stripped || stripped === '.' || stripped === '..' ? 'archive' : stripped
+  if (
+    !stripped ||
+    stripped === '.' ||
+    stripped === '..' ||
+    isReservedWorkflowAliasBackingDisplayPath(stripped)
+  ) {
+    return 'archive'
+  }
+  return stripped
 }
 
 /**

apps/sim/lib/copilot/tools/handlers/upload-file-reader.test.ts

@@ -14,6 +14,7 @@ const { mockReadFileRecord, mockFetchBuffer } = vi.hoisted(() => ({
 
 vi.mock('@/lib/copilot/vfs/file-reader', () => ({
   readFileRecord: mockReadFileRecord,
+  MAX_TEXT_READ_BYTES: 5 * 1024 * 1024,
 }))
 
 vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({

apps/sim/lib/copilot/tools/handlers/upload-file-reader.ts

@@ -3,7 +3,11 @@ import { workspaceFiles } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { toError } from '@sim/utils/errors'
 import { and, asc, desc, eq, isNull, or } from 'drizzle-orm'
-import { type FileReadResult, readFileRecord } from '@/lib/copilot/vfs/file-reader'
+import {
+  type FileReadResult,
+  MAX_TEXT_READ_BYTES,
+  readFileRecord,
+} from '@/lib/copilot/vfs/file-reader'
 import {
   type GrepCountEntry,
   type GrepMatch,
@@ -27,9 +31,11 @@ const logger = createLogger('UploadFileReader')
  * bytes decide (a mislabeled text file named `data.zip` stays readable instead
  * of being trapped between read-says-extract and extract-says-invalid); above
  * it the extension is trusted so a real 100MB zip is never downloaded just to
- * refuse it.
+ * refuse it. Aligned with the read path's inline text cap — any mislabeled
+ * file too big to sniff would be rejected by read() as too large anyway, so
+ * nothing readable is ever dead-ended.
  */
-const ARCHIVE_SNIFF_MAX_BYTES = 1024 * 1024
+const ARCHIVE_SNIFF_MAX_BYTES = MAX_TEXT_READ_BYTES
 
 /**
  * True when the upload should get extract-first guidance: named like an archive

apps/sim/lib/copilot/vfs/file-reader.ts

@@ -26,7 +26,8 @@ function recordSpanError(span: Span, err: unknown) {
 
 const logger = createLogger('FileReader')
 
-const MAX_TEXT_READ_BYTES = 5 * 1024 * 1024 // 5 MB
+/** Inline text-read cap — exported so callers can align their own byte-sniff budgets with what read() can actually display. */
+export const MAX_TEXT_READ_BYTES = 5 * 1024 * 1024 // 5 MB
 const MAX_IMAGE_READ_BYTES = 5 * 1024 * 1024 // 5 MB
 // Parseable-document byte cap. Large office/PDF files can still
 // produce huge extracted text; reject up front to avoid wasting a

apps/sim/lib/uploads/archive.test.ts

@@ -5,15 +5,17 @@ import { Buffer } from 'buffer'
 import JSZip from 'jszip'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockEnsureFolder, mockUpload } = vi.hoisted(() => ({
+const { mockEnsureFolder, mockUpload, mockDelete } = vi.hoisted(() => ({
   mockEnsureFolder: vi.fn(),
   mockUpload: vi.fn(),
+  mockDelete: vi.fn(),
 }))
 vi.mock('@/lib/uploads/contexts/workspace/workspace-file-folder-manager', () => ({
   ensureWorkspaceFileFolderPath: mockEnsureFolder,
 }))
 vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
   uploadWorkspaceFile: mockUpload,
+  deleteWorkspaceFile: mockDelete,
 }))
 
 import {
@@ -70,6 +72,7 @@ function craftCentralDirectory(records: number, extraPerRecord: number): Buffer
 beforeEach(() => {
   vi.clearAllMocks()
   mockEnsureFolder.mockResolvedValue('folder_1')
+  mockDelete.mockResolvedValue(undefined)
   mockUpload.mockImplementation(async (_ws: string, _uid: string, buf: Buffer, name: string) => ({
     id: `f_${name}`,
     name,
@@ -184,6 +187,25 @@ describe('decompressArchiveBufferToWorkspaceFiles', () => {
     expect(mockUpload).not.toHaveBeenCalled()
   })
 
+  it('rolls back already-uploaded files when an upload fails mid-extraction', async () => {
+    // Pass 1 validates caps, but an upload itself can still fail mid-loop
+    // (storage/DB error, quota crossed). Every file written before the failure
+    // must be deleted so callers and retries never observe a partial tree.
+    const buffer = await buildZip({ 'a.txt': 'first', 'b.txt': 'second', 'c.txt': 'third' })
+    mockUpload
+      .mockResolvedValueOnce({ id: 'f_a', name: 'a.txt', url: '/a', key: 'k/a', size: 5 })
+      .mockResolvedValueOnce({ id: 'f_b', name: 'b.txt', url: '/b', key: 'k/b', size: 6 })
+      .mockRejectedValueOnce(new Error('storage quota exceeded'))
+
+    await expect(
+      decompressArchiveBufferToWorkspaceFiles(buffer, { workspaceId: 'ws', userId: 'u' })
+    ).rejects.toThrow('storage quota exceeded')
+
+    expect(mockDelete).toHaveBeenCalledTimes(2)
+    expect(mockDelete).toHaveBeenCalledWith('ws', 'f_a')
+    expect(mockDelete).toHaveBeenCalledWith('ws', 'f_b')
+  })
+
   it('does not count noise entries toward the extraction cap when they are being skipped', async () => {
     // macOS Finder zips carry a __MACOSX/._* shadow per file, doubling the raw
     // entry count. 501 files + 501 shadows = 1002 raw entries — over the

apps/sim/lib/uploads/archive.ts

@@ -3,7 +3,10 @@ import type { Readable } from 'stream'
 import JSZip from 'jszip'
 import { readZipCentralDirectoryStats } from '@/lib/file-parsers/zip-guard'
 import { ensureWorkspaceFileFolderPath } from '@/lib/uploads/contexts/workspace/workspace-file-folder-manager'
-import { uploadWorkspaceFile } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
+import {
+  deleteWorkspaceFile,
+  uploadWorkspaceFile,
+} from '@/lib/uploads/contexts/workspace/workspace-file-manager'
 import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils'
 import type { UserFile } from '@/executor/types'
 
@@ -328,40 +331,55 @@ export async function decompressArchiveBufferToWorkspaceFiles(
   }
 
   // Pass 2 — extract: the archive is proven within caps; inflate again and upload.
+  // Uploads themselves can still fail mid-loop (storage/DB errors, quota crossed
+  // by another writer), so a failure rolls back every file written so far —
+  // callers and their retries must never observe a partial tree.
   const folderIdCache = new Map<string, string | null>()
   const extracted: UserFile[] = []
   let totalBytes = 0
-  for (const { entry, segments } of safeEntries) {
-    const result = await inflateEntryWithinCaps(entry, MAX_ARCHIVE_TOTAL_BYTES - totalBytes, true)
-    if (!result.ok) throwInflateCapError(result.reason, entry.name)
-    totalBytes += result.size
-    const entryBuffer = result.buffer as Buffer
-
-    const leafName = segments[segments.length - 1]
-    const folderSegments = [...rootFolderSegments, ...segments.slice(0, -1)]
-    const folderKey = folderSegments.join('/')
-    let folderId = folderIdCache.get(folderKey)
-    if (folderId === undefined) {
-      folderId = await ensureWorkspaceFileFolderPath({
+  try {
+    for (const { entry, segments } of safeEntries) {
+      const result = await inflateEntryWithinCaps(entry, MAX_ARCHIVE_TOTAL_BYTES - totalBytes, true)
+      if (!result.ok) throwInflateCapError(result.reason, entry.name)
+      totalBytes += result.size
+      const entryBuffer = result.buffer as Buffer
+
+      const leafName = segments[segments.length - 1]
+      const folderSegments = [...rootFolderSegments, ...segments.slice(0, -1)]
+      const folderKey = folderSegments.join('/')
+      let folderId = folderIdCache.get(folderKey)
+      if (folderId === undefined) {
+        folderId = await ensureWorkspaceFileFolderPath({
+          workspaceId,
+          userId,
+          pathSegments: folderSegments,
+        })
+        folderIdCache.set(folderKey, folderId)
+      }
+
+      const mimeType = getMimeTypeFromExtension(getFileExtension(leafName))
+      const uploaded = await uploadWorkspaceFile(
         workspaceId,
         userId,
-        pathSegments: folderSegments,
-      })
-      folderIdCache.set(folderKey, folderId)
+        entryBuffer,
+        leafName,
+        mimeType,
+        {
+          folderId,
+        }
+      )
+      extracted.push(uploaded)
     }
-
-    const mimeType = getMimeTypeFromExtension(getFileExtension(leafName))
-    const uploaded = await uploadWorkspaceFile(
-      workspaceId,
-      userId,
-      entryBuffer,
-      leafName,
-      mimeType,
-      {
-        folderId,
+  } catch (error) {
+    for (const file of extracted) {
+      try {
+        await deleteWorkspaceFile(workspaceId, file.id)
+      } catch {
+        // Best-effort: a file whose cleanup fails is still soft-deletable by hand;
+        // the original error is what the caller needs to see.
       }
-    )
-    extracted.push(uploaded)
+    }
+    throw error
   }
 
   return { extracted, skipped, skippedUnsafePaths }