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
85 changes: 85 additions & 0 deletions assistant/src/__tests__/cu-session-finalized.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,91 @@ describe('handleCuSessionFinalized', () => {
});
});

test('creates fallback attachment when reportToSessionId exists but conversation is missing and recording is present', () => {
// This tests the edge case where reportToSessionId is set but the conversation
// doesn't exist (e.g. it was deleted or never created). The recording should
// still get a file-backed attachment entry so cleanup can track it.
const ctx = makeCtx();
const cuSessionId = 'cu-session-orphan-with-report';
ctx.cuSessions.set(cuSessionId, {} as ComputerUseSession);
ctx.cuSessionMetadata.set(cuSessionId, {
reportToSessionId: 'nonexistent-session',
qaMode: true,
});

// Ensure the conversation does NOT exist.
mockConversations.delete('nonexistent-session');
mockAddedMessages.length = 0;

const msg: CuSessionFinalized = {
type: 'cu_session_finalized',
sessionId: cuSessionId,
status: 'completed',
summary: 'QA recording completed.',
stepCount: 3,
recording: {
localPath: '/tmp/orphan-recording.mp4',
mimeType: 'video/mp4',
sizeBytes: 2048000,
durationMs: 45000,
width: 1920,
height: 1080,
captureScope: 'window',
includeAudio: false,
},
};

// Should not throw.
handleCuSessionFinalized(msg, new net.Socket(), ctx);

// No message persisted (conversation missing), but the recording should
// still be tracked. We can't easily verify createFileBackedAttachment
// was called without mocking it, but at minimum the function shouldn't throw
// and state should be cleaned up.
expect(mockAddedMessages.length).toBe(0);
expect(ctx.cuSessions.has(cuSessionId)).toBe(false);
expect(ctx.cuSessionMetadata.has(cuSessionId)).toBe(false);
});

test('creates fallback attachment when summary is empty but recording is present', () => {
const ctx = makeCtx();
const cuSessionId = 'cu-session-empty-summary';
ctx.cuSessions.set(cuSessionId, {} as ComputerUseSession);
ctx.cuSessionMetadata.set(cuSessionId, {
reportToSessionId: 'some-session',
qaMode: true,
});

mockConversations.set('some-session', { id: 'some-session' });
mockAddedMessages.length = 0;

const msg: CuSessionFinalized = {
type: 'cu_session_finalized',
sessionId: cuSessionId,
status: 'completed',
summary: '', // Empty summary — injection path skipped
stepCount: 2,
recording: {
localPath: '/tmp/empty-summary-recording.mp4',
mimeType: 'video/mp4',
sizeBytes: 512000,
durationMs: 10000,
width: 1280,
height: 720,
captureScope: 'display',
includeAudio: true,
},
};

handleCuSessionFinalized(msg, new net.Socket(), ctx);

// No message persisted (empty summary skips injection path).
expect(mockAddedMessages.length).toBe(0);
// State cleaned up.
expect(ctx.cuSessions.has(cuSessionId)).toBe(false);
expect(ctx.cuSessionMetadata.has(cuSessionId)).toBe(false);
});

test('stores and retrieves CU session metadata', () => {
const ctx = makeCtx();

Expand Down
11 changes: 7 additions & 4 deletions assistant/src/daemon/handlers/computer-use.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ export function handleCuSessionFinalized(
ctx: HandlerContext,
): void {
const meta = ctx.cuSessionMetadata.get(msg.sessionId);
let recordingTracked = false;

log.info(
{
Expand Down Expand Up @@ -264,6 +265,7 @@ export function handleCuSessionFinalized(
expiresAt: msg.recording.expiresAt,
});
linkAttachmentToMessage(persistedMessage.id, attachment.id, 0);
recordingTracked = true;
Comment on lines 267 to +268
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Mark recording tracked before linking attachment

recordingTracked is only flipped after linkAttachmentToMessage(...), but the file is already tracked once createFileBackedAttachment(...) succeeds. If linking throws (for example from a transient DB write/constraint issue), this leaves recordingTracked false and the fallback path creates a second attachment row for the same recording, which can produce duplicate attachment records and duplicate cleanup processing for one file.

Useful? React with 👍 / 👎.

recordingAttachment = {
id: attachment.id,
filename: attachment.originalFilename,
Expand Down Expand Up @@ -332,9 +334,10 @@ export function handleCuSessionFinalized(
}
}

// Create a file-backed attachment for recordings without a reporting session
// so cleanup can track orphan files.
if (msg.recording && !(meta?.reportToSessionId)) {
// Create a fallback file-backed attachment for any recording that wasn't
// already tracked (no reportToSessionId, missing conversation, empty summary,
// or attachment creation failure) so cleanup can track the file on disk.
if (msg.recording && !recordingTracked) {
try {
createFileBackedAttachment({
filename: `qa-recording-${msg.sessionId}.mp4`,
Expand All @@ -344,7 +347,7 @@ export function handleCuSessionFinalized(
sha256: undefined,
expiresAt: msg.recording.expiresAt,
});
log.info({ sessionId: msg.sessionId }, 'Created orphan file-backed attachment for cleanup tracking (no reportToSessionId)');
log.info({ sessionId: msg.sessionId }, 'Created fallback file-backed attachment for cleanup tracking');
} catch (err) {
log.error({ err, sessionId: msg.sessionId }, 'Failed to create file-backed attachment for orphan recording');
}
Expand Down
Loading