Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 103 additions & 3 deletions assistant/src/daemon/handlers/recording.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import * as net from 'node:net';
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 { uploadFileBackedAttachment, linkAttachmentToMessage } from '../../memory/attachments-store.js';

// ─── Deterministic maps ──────────────────────────────────────────────────────
// These ensure stop resolves the exact active recording for a conversation,
Expand Down Expand Up @@ -93,7 +97,7 @@ export function handleRecordingStop(
function handleRecordingStatus(
msg: RecordingStatus,
_socket: net.Socket,
_ctx: HandlerContext,
ctx: HandlerContext,
): void {
const recordingId = msg.sessionId;
const conversationId = standaloneRecordingConversationId.get(recordingId)
Expand All @@ -109,8 +113,86 @@ function handleRecordingStatus(
{ recordingId, conversationId, filePath: msg.filePath, durationMs: msg.durationMs },
'Standalone recording stopped — file ready',
);
// Clean up deterministic maps. Full finalization (attaching the file
// to the conversation as a message) is M4 scope.

// Finalize: attach the recording file to the conversation
if (msg.filePath) {
try {
if (!existsSync(msg.filePath)) {
log.error({ recordingId, filePath: msg.filePath }, 'Recording file does not exist');
} else if (!conversationId) {
log.warn({ recordingId }, 'No conversationId found for recording — cannot link attachment');
} else {
const stat = statSync(msg.filePath);
const sizeBytes = stat.size;
const filename = path.basename(msg.filePath);

// Infer MIME type from extension
const ext = filename.split('.').pop()?.toLowerCase();
const mimeType = ext === 'mov' ? 'video/quicktime' : ext === 'mp4' ? 'video/mp4' : 'video/mp4';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MIME type fallback always defaults to video/mp4 for any unrecognized extension

The ternary chain for inferring the MIME type from the file extension has a default branch that unconditionally returns 'video/mp4', making the entire else-branch a dead path that never produces a different type.

Root Cause

At assistant/src/daemon/handlers/recording.ts:131, the MIME type inference is:

const mimeType = ext === 'mov' ? 'video/quicktime' : ext === 'mp4' ? 'video/mp4' : 'video/mp4';

The final fallback 'video/mp4' means any extension that isn't mov or mp4 (e.g., .webm, .mkv, .avi, or even a completely non-video extension) will still be labeled as video/mp4. This incorrect MIME type is then stored in the database and served via the /content endpoint's Content-Type header (attachment-routes.ts:203), which can cause playback failures when the browser/player receives a non-MP4 file advertised as video/mp4.

Impact: If the recording client ever produces a file format other than .mp4 or .mov (e.g. .webm on Linux), the attachment would be served with a wrong Content-Type, breaking video playback for the end user.

Suggested change
const mimeType = ext === 'mov' ? 'video/quicktime' : ext === 'mp4' ? 'video/mp4' : 'video/mp4';
const mimeType = ext === 'mov' ? 'video/quicktime' : ext === 'webm' ? 'video/webm' : 'video/mp4';
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


// 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
const existingMessages = conversationStore.getMessages(conversationId);
const lastAssistantMsg = [...existingMessages].reverse().find((m) => m.role === 'assistant');

let messageId: string;
if (lastAssistantMsg) {
messageId = lastAssistantMsg.id;
} else {
const newMsg = conversationStore.addMessage(
conversationId,
'assistant',
JSON.stringify([{ type: 'text', text: 'Screen recording attached.' }]),
);
messageId = newMsg.id;
log.info({ recordingId, conversationId, messageId }, 'Created assistant message for recording attachment');
}

linkAttachmentToMessage(messageId, attachment.id, 0);
log.info({ recordingId, messageId, attachmentId: attachment.id }, 'Linked recording attachment to assistant message');

// Notify the client
const socket = findSocketForSession(conversationId, ctx);
if (socket) {
ctx.send(socket, {
type: 'assistant_text_delta',
text: 'Screen recording complete. Your recording has been saved.',
sessionId: conversationId,
});
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,
}],
});
Comment thread
Jasonnnz marked this conversation as resolved.
}
}
} catch (err) {
log.error({ err, recordingId, filePath: msg.filePath }, 'Failed to create attachment for standalone recording');
Comment thread
Jasonnnz marked this conversation as resolved.
// 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 });
}
}
}
}

// Clean up deterministic maps
standaloneRecordingConversationId.delete(recordingId);
if (conversationId) {
const current = recordingOwnerByConversation.get(conversationId);
Expand All @@ -126,6 +208,24 @@ function handleRecordingStatus(
{ recordingId, conversationId, error: msg.error },
'Standalone recording failed',
);

// Notify the client about the failure
if (conversationId) {
const socket = findSocketForSession(conversationId, ctx);
if (socket) {
ctx.send(socket, {
type: 'assistant_text_delta',
text: `Recording failed: ${msg.error ?? 'unknown error'}`,
sessionId: conversationId,
});
ctx.send(socket, {
type: 'message_complete',
sessionId: conversationId,
});
}
}

// Clean up deterministic maps
standaloneRecordingConversationId.delete(recordingId);
if (conversationId) {
const current = recordingOwnerByConversation.get(conversationId);
Expand Down
6 changes: 4 additions & 2 deletions assistant/src/daemon/handlers/sessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
76 changes: 75 additions & 1 deletion assistant/src/memory/attachments-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions assistant/src/runtime/http-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
handleUploadAttachment,
handleDeleteAttachment,
handleGetAttachment,
handleGetAttachmentContent,
} from './routes/attachment-routes.js';
import {
handleCreateRun,
Expand Down Expand Up @@ -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]);

Expand Down
103 changes: 102 additions & 1 deletion assistant/src/runtime/routes/attachment-routes.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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',
},
});
}