fix: address M4 PR #8703 review feedback

assistant/src/daemon/handlers/recording.ts

@@ -1,11 +1,11 @@
 import * as net from 'node:net';
-import { existsSync, statSync, readFileSync } from 'node:fs';
+import { existsSync, statSync } from 'node:fs';
 import * as path from 'node:path';
 import { v4 as uuid } from 'uuid';
 import type { RecordingStatus, RecordingOptions } from '../ipc-protocol.js';
 import { log, findSocketForSession, defineHandlers, type HandlerContext } from './shared.js';
 import * as conversationStore from '../../memory/conversation-store.js';
-import { uploadAttachment, linkAttachmentToMessage } from '../../memory/attachments-store.js';
+import { uploadFileBackedAttachment, linkAttachmentToMessage } from '../../memory/attachments-store.js';
 
 // ─── Deterministic maps ──────────────────────────────────────────────────────
 // These ensure stop resolves the exact active recording for a conversation,
@@ -130,11 +130,8 @@ function handleRecordingStatus(
             const ext = filename.split('.').pop()?.toLowerCase();
             const mimeType = ext === 'mov' ? 'video/quicktime' : ext === 'mp4' ? 'video/mp4' : 'video/mp4';
 
-            // Read file and encode as base64 for the attachment store
-            const fileData = readFileSync(msg.filePath);
-            const dataBase64 = fileData.toString('base64');
-
-            const attachment = uploadAttachment(filename, mimeType, dataBase64);
+            // Store as file-backed attachment (avoids reading large files into memory)
+            const attachment = uploadFileBackedAttachment(filename, mimeType, msg.filePath, sizeBytes);
             log.info({ recordingId, attachmentId: attachment.id, sizeBytes, filePath: msg.filePath }, 'Created attachment for standalone recording');
 
             // Find or create an assistant message to attach the recording to
@@ -168,11 +165,30 @@ function handleRecordingStatus(
               ctx.send(socket, {
                 type: 'message_complete',
                 sessionId: conversationId,
+                attachments: [{
+                  id: attachment.id,
+                  filename: attachment.originalFilename,
+                  mimeType: attachment.mimeType,
+                  data: '',  // empty for file-backed; client uses content endpoint
+                  sizeBytes: attachment.sizeBytes,
+                }],
               });
             }
           }
         } catch (err) {
           log.error({ err, recordingId, filePath: msg.filePath }, 'Failed to create attachment for standalone recording');
+          // Notify the client about the finalization failure
+          if (conversationId) {
+            const errSocket = findSocketForSession(conversationId, ctx);
+            if (errSocket) {
+              ctx.send(errSocket, {
+                type: 'assistant_text_delta',
+                text: 'Recording saved but failed to attach to conversation.',
+                sessionId: conversationId,
+              });
+              ctx.send(errSocket, { type: 'message_complete', sessionId: conversationId });
+            }
+          }
         }
       }
 

assistant/src/daemon/handlers/sessions.ts

@@ -512,10 +512,12 @@ export function handleHistoryRequest(
         // the client, so non-video attachments always keep their inline data.
         const MAX_INLINE_B64_SIZE = 512 * 1024;
         attachments = linked.map((a) => {
-          const omit = a.mimeType.startsWith('video/') && a.dataBase64.length > MAX_INLINE_B64_SIZE;
+          const isFileBacked = !a.dataBase64; // empty string = file-backed attachment
+          const omit = isFileBacked || (a.mimeType.startsWith('video/') && a.dataBase64.length > MAX_INLINE_B64_SIZE);
 
           // Lazily generate thumbnails for existing video attachments on first history load.
-          if (a.mimeType.startsWith('video/') && !a.thumbnailBase64) {
+          // Skip for file-backed attachments — there is no in-memory base64 to generate from.
+          if (a.mimeType.startsWith('video/') && !a.thumbnailBase64 && a.dataBase64) {
             const attachmentId = a.id;
             const base64 = a.dataBase64;
             silentlyWithLog(

assistant/src/memory/attachments-store.ts

@@ -7,7 +7,7 @@
 
 import { eq } from 'drizzle-orm';
 import { v4 as uuid } from 'uuid';
-import { getDb, rawRun } from './db.js';
+import { getDb, rawRun, rawGet } from './db.js';
 import { attachments, messageAttachments } from './schema.js';
 
 export interface StoredAttachment {
@@ -151,6 +151,80 @@ function computeContentHash(dataBase64: string): string {
   return Bun.hash(dataBase64).toString(36);
 }
 
+// ---------------------------------------------------------------------------
+// File-backed attachment storage (avoids reading large files into memory)
+// ---------------------------------------------------------------------------
+
+function ensureFilePathColumn(): void {
+  try {
+    // SQLite allows ALTER TABLE ADD COLUMN for nullable columns
+    rawRun('ALTER TABLE attachments ADD COLUMN file_path TEXT');
+  } catch {
+    // Column already exists — ignore the error
+  }
+}
+
+let filePathColumnEnsured = false;
+
+/**
+ * Store a file-backed attachment by path reference, without reading the file
+ * into memory. This avoids OOM risk for large recordings that exceed the
+ * normal 20 MB upload limit.
+ *
+ * The file stays on disk; the attachment row stores an empty dataBase64 and
+ * records the on-disk path in a `file_path` column (added via runtime
+ * migration since the Drizzle schema doesn't know about it).
+ */
+export function uploadFileBackedAttachment(
+  filename: string,
+  mimeType: string,
+  filePath: string,
+  sizeBytes: number,
+): StoredAttachment & { filePath: string } {
+  if (!filePathColumnEnsured) {
+    ensureFilePathColumn();
+    filePathColumnEnsured = true;
+  }
+
+  const now = Date.now();
+  const kind = classifyKind(mimeType);
+  const id = uuid();
+
+  // Use raw SQL since the Drizzle schema doesn't know about the file_path column
+  rawRun(
+    `INSERT INTO attachments (id, original_filename, mime_type, size_bytes, kind, data_base64, file_path, created_at)
+     VALUES (?, ?, ?, ?, ?, '', ?, ?)`,
+    id, filename, mimeType, sizeBytes, kind, filePath, now,
+  );
+
+  return {
+    id,
+    originalFilename: filename,
+    mimeType,
+    sizeBytes,
+    kind,
+    thumbnailBase64: null,
+    createdAt: now,
+    filePath,
+  };
+}
+
+/**
+ * Returns the file_path for a file-backed attachment, or null if not file-backed.
+ * Uses raw SQL since file_path is added via runtime migration and is not in the Drizzle schema.
+ */
+export function getFilePathForAttachment(attachmentId: string): string | null {
+  if (!filePathColumnEnsured) {
+    ensureFilePathColumn();
+    filePathColumnEnsured = true;
+  }
+  const row = rawGet<{ file_path: string | null }>(
+    'SELECT file_path FROM attachments WHERE id = ?',
+    attachmentId,
+  );
+  return row?.file_path ?? null;
+}
+
 export function uploadAttachment(
   filename: string,
   mimeType: string,

assistant/src/runtime/http-server.ts

@@ -27,6 +27,7 @@ import {
   handleUploadAttachment,
   handleDeleteAttachment,
   handleGetAttachment,
+  handleGetAttachmentContent,
 } from './routes/attachment-routes.js';
 import {
   handleCreateRun,
@@ -581,6 +582,9 @@ export class RuntimeHttpServer {
       if (endpoint === 'attachments' && req.method === 'POST') return await handleUploadAttachment(req);
       if (endpoint === 'attachments' && req.method === 'DELETE') return await handleDeleteAttachment(req);
 
+      const attachmentContentMatch = endpoint.match(/^attachments\/([^/]+)\/content$/);
+      if (attachmentContentMatch && req.method === 'GET') return handleGetAttachmentContent(attachmentContentMatch[1], req);
+
       const attachmentMatch = endpoint.match(/^attachments\/([^/]+)$/);
       if (attachmentMatch && req.method === 'GET') return handleGetAttachment(attachmentMatch[1]);
 

assistant/src/runtime/routes/attachment-routes.ts

@@ -1,8 +1,9 @@
 /**
  * Route handlers for attachment upload, download, and deletion.
  */
+import { existsSync } from 'node:fs';
 import * as attachmentsStore from '../../memory/attachments-store.js';
-import { validateAttachmentUpload, AttachmentUploadError } from '../../memory/attachments-store.js';
+import { validateAttachmentUpload, AttachmentUploadError, getFilePathForAttachment } from '../../memory/attachments-store.js';
 
 /** 30 MB — base64-encoded 20 MB attachment ≈ 27 MB plus JSON wrapper overhead. */
 const MAX_UPLOAD_BODY_BYTES = 30 * 1024 * 1024;
@@ -122,12 +123,112 @@ export function handleGetAttachment(attachmentId: string): Response {
     return Response.json({ error: 'Attachment not found' }, { status: 404 });
   }
 
+  const isFileBacked = !attachment.dataBase64;
+
   return Response.json({
     id: attachment.id,
     filename: attachment.originalFilename,
     mimeType: attachment.mimeType,
     sizeBytes: attachment.sizeBytes,
     kind: attachment.kind,
     data: attachment.dataBase64,
+    // Signal to clients that they should fetch content via the /content endpoint
+    ...(isFileBacked ? { fileBacked: true } : {}),
+  });
+}
+
+/**
+ * Serve raw file bytes for an attachment. For file-backed attachments this
+ * streams from disk; for inline attachments it decodes the base64 data.
+ * Supports Range headers for video seeking.
+ */
+export function handleGetAttachmentContent(attachmentId: string, req: Request): Response {
+  const attachment = attachmentsStore.getAttachmentById(attachmentId);
+  if (!attachment) {
+    return Response.json({ error: 'Attachment not found' }, { status: 404 });
+  }
+
+  // Check for file-backed attachment
+  const filePath = getFilePathForAttachment(attachmentId);
+  if (filePath) {
+    if (!existsSync(filePath)) {
+      return Response.json({ error: 'Recording file not found on disk' }, { status: 404 });
+    }
+
+    const file = Bun.file(filePath);
+    const rangeHeader = req.headers.get('Range');
+
+    if (rangeHeader) {
+      const fileSize = attachment.sizeBytes;
+      let start: number;
+      let end: number;
+
+      // Parse suffix range: bytes=-N (last N bytes)
+      const suffixMatch = rangeHeader.match(/bytes=-(\d+)/);
+      if (suffixMatch) {
+        const suffixLen = parseInt(suffixMatch[1]);
+        start = Math.max(0, fileSize - suffixLen);
+        end = fileSize - 1;
+      } else {
+        // Parse standard range: bytes=start-end
+        const match = rangeHeader.match(/bytes=(\d+)-(\d*)/);
+        if (!match) {
+          // Unparseable range — return full file
+          return new Response(file, {
+            headers: {
+              'Content-Type': attachment.mimeType,
+              'Content-Length': String(fileSize),
+              'Accept-Ranges': 'bytes',
+            },
+          });
+        }
+        start = parseInt(match[1]);
+        end = match[2] ? parseInt(match[2]) : fileSize - 1;
+      }
+
+      // Clamp end to file size
+      end = Math.min(end, fileSize - 1);
+
+      // Reject invalid ranges
+      if (start > end || start >= fileSize) {
+        return new Response(null, {
+          status: 416,
+          headers: { 'Content-Range': `bytes */${fileSize}` },
+        });
+      }
+
+      const slice = file.slice(start, end + 1);
+      return new Response(slice, {
+        status: 206,
+        headers: {
+          'Content-Type': attachment.mimeType,
+          'Content-Range': `bytes ${start}-${end}/${fileSize}`,
+          'Accept-Ranges': 'bytes',
+          'Content-Length': String(end - start + 1),
+        },
+      });
+    }
+
+    return new Response(file, {
+      headers: {
+        'Content-Type': attachment.mimeType,
+        'Content-Length': String(attachment.sizeBytes),
+        'Accept-Ranges': 'bytes',
+      },
+    });
+  }
+
+  // Fall back to base64-decoded content for inline attachments
+  if (!attachment.dataBase64) {
+    return Response.json({ error: 'No content available' }, { status: 404 });
+  }
+
+  const buffer = Buffer.from(attachment.dataBase64, 'base64');
+  return new Response(buffer, {
+    headers: {
+      'Content-Type': attachment.mimeType,
+      'Content-Length': String(buffer.length),
+      'Accept-Ranges': 'bytes',
+    },
   });
 }