diff --git a/src/browser/utils/messages/modelMessageTransform.test.ts b/src/browser/utils/messages/modelMessageTransform.test.ts index e2a57c9e82..b23458fdb9 100644 --- a/src/browser/utils/messages/modelMessageTransform.test.ts +++ b/src/browser/utils/messages/modelMessageTransform.test.ts @@ -1123,3 +1123,75 @@ describe("injectFileChangeNotifications", () => { expect(text).toContain("diff2"); }); }); + +describe("transformModelMessages - empty content filtering", () => { + it("should filter out messages with empty content arrays", () => { + // This tests the defense-in-depth filtering of empty content + // Note: consecutive user messages get merged, so we verify the empty assistant is removed + const messages: ModelMessage[] = [ + { role: "user", content: [{ type: "text", text: "Hello" }] }, + { role: "assistant", content: [] }, // Empty content - should be filtered + { role: "user", content: [{ type: "text", text: "World" }] }, + ]; + + const result = transformModelMessages(messages, "anthropic"); + + // The empty assistant message should be filtered out + // The two user messages get merged into one + expect(result.length).toBe(1); + expect(result[0].role).toBe("user"); + // Verify both user texts were merged + const content = result[0].content; + expect(Array.isArray(content)).toBe(true); + if (Array.isArray(content)) { + const text = content.find((c) => c.type === "text")?.text; + expect(text).toContain("Hello"); + expect(text).toContain("World"); + } + }); + + it("should filter out messages with empty string content", () => { + const messages: ModelMessage[] = [ + { role: "user", content: [{ type: "text", text: "Hello" }] }, + { role: "assistant", content: "" }, // Empty string content - should be filtered + { role: "user", content: [{ type: "text", text: "World" }] }, + ]; + + const result = transformModelMessages(messages, "anthropic"); + + // Same as above - empty assistant filtered, users merged + expect(result.length).toBe(1); + expect(result[0].role).toBe("user"); + }); + + it("should keep messages with non-empty content", () => { + const messages: ModelMessage[] = [ + { role: "user", content: [{ type: "text", text: "Hello" }] }, + { role: "assistant", content: [{ type: "text", text: "Hi there!" }] }, + { role: "user", content: [{ type: "text", text: "World" }] }, + ]; + + const result = transformModelMessages(messages, "anthropic"); + + expect(result.length).toBe(3); + expect(result[0].role).toBe("user"); + expect(result[1].role).toBe("assistant"); + expect(result[2].role).toBe("user"); + }); + + it("should filter out empty assistant message between non-user messages", () => { + const messages: ModelMessage[] = [ + { role: "user", content: [{ type: "text", text: "Hello" }] }, + { role: "assistant", content: [{ type: "text", text: "Hi there!" }] }, + { role: "assistant", content: [] }, // Empty - should be filtered + { role: "user", content: [{ type: "text", text: "Thanks" }] }, + ]; + + const result = transformModelMessages(messages, "anthropic"); + + expect(result.length).toBe(3); + expect(result[0].role).toBe("user"); + expect(result[1].role).toBe("assistant"); + expect(result[2].role).toBe("user"); + }); +}); diff --git a/src/browser/utils/messages/modelMessageTransform.ts b/src/browser/utils/messages/modelMessageTransform.ts index ce549be3aa..95c4d5977a 100644 --- a/src/browser/utils/messages/modelMessageTransform.ts +++ b/src/browser/utils/messages/modelMessageTransform.ts @@ -391,6 +391,26 @@ function filterReasoningOnlyMessages(messages: ModelMessage[]): ModelMessage[] { }); } +/** + * Filter out any ModelMessage with empty content. + * This is defense-in-depth to catch edge cases where upstream transforms + * (e.g., ignoreIncompleteToolCalls, reasoning stripping) leave messages + * with empty content arrays that would cause API errors. + * + * Anthropic API rejects messages like: { role: "assistant", content: [] } + */ +function filterEmptyContentMessages(messages: ModelMessage[]): ModelMessage[] { + return messages.filter((msg) => { + // String content with actual text is valid + if (typeof msg.content === "string") { + return msg.content.length > 0; + } + + // Array content must have at least one item + return msg.content.length > 0; + }); +} + /** * Coalesce consecutive parts of the same type within each message. * Streaming creates many individual text/reasoning parts; merge them for easier debugging. @@ -534,7 +554,12 @@ export function transformModelMessages(messages: ModelMessage[], provider: strin // Pass 3: Merge consecutive user messages (applies to all providers) const merged = mergeConsecutiveUserMessages(reasoningHandled); - return merged; + // Pass 4: Filter any messages that ended up with empty content + // This is defense-in-depth for edge cases where transforms produce empty content + // (e.g., tool calls dropped, reasoning stripped, but message structure remained) + const nonEmpty = filterEmptyContentMessages(merged); + + return nonEmpty; } /** diff --git a/src/node/services/historyService.test.ts b/src/node/services/historyService.test.ts index 6d3229676e..ddd8ccf3fa 100644 --- a/src/node/services/historyService.test.ts +++ b/src/node/services/historyService.test.ts @@ -523,4 +523,52 @@ describe("HistoryService", () => { } }); }); + + describe("deleteMessageBySequence", () => { + it("should delete message by historySequence", async () => { + const workspaceId = "workspace-delete"; + const msg1 = createMuxMessage("msg1", "user", "Hello"); + const msg2 = createMuxMessage("msg2", "assistant", "Hi"); + const msg3 = createMuxMessage("msg3", "user", "Bye"); + + await service.appendToHistory(workspaceId, msg1); + await service.appendToHistory(workspaceId, msg2); + await service.appendToHistory(workspaceId, msg3); + + // Delete the middle message (sequence 1) + const result = await service.deleteMessageBySequence(workspaceId, 1); + expect(result.success).toBe(true); + + const history = await service.getHistory(workspaceId); + if (history.success) { + expect(history.data).toHaveLength(2); + expect(history.data[0].metadata?.historySequence).toBe(0); + expect(history.data[1].metadata?.historySequence).toBe(2); + } + }); + + it("should return success if message not found (idempotent)", async () => { + const workspaceId = "workspace-delete-idempotent"; + const msg1 = createMuxMessage("msg1", "user", "Hello"); + + await service.appendToHistory(workspaceId, msg1); + + // Try to delete non-existent sequence + const result = await service.deleteMessageBySequence(workspaceId, 999); + expect(result.success).toBe(true); + + // History should be unchanged + const history = await service.getHistory(workspaceId); + if (history.success) { + expect(history.data).toHaveLength(1); + } + }); + + it("should handle deleting from empty workspace", async () => { + const workspaceId = "workspace-delete-empty"; + + const result = await service.deleteMessageBySequence(workspaceId, 0); + expect(result.success).toBe(true); + }); + }); }); diff --git a/src/node/services/historyService.ts b/src/node/services/historyService.ts index fbb86a6b80..db226bbd2c 100644 --- a/src/node/services/historyService.ts +++ b/src/node/services/historyService.ts @@ -246,6 +246,51 @@ export class HistoryService { }); } + /** + * Delete a specific message from history by its historySequence number. + * Used to clean up empty placeholder messages when a stream is interrupted + * before producing any content. + */ + async deleteMessageBySequence( + workspaceId: string, + historySequence: number + ): Promise> { + return this.fileLocks.withLock(workspaceId, async () => { + try { + const historyPath = this.getChatHistoryPath(workspaceId); + + // Read all messages + const historyResult = await this.getHistory(workspaceId); + if (!historyResult.success) { + return historyResult; + } + + const messages = historyResult.data; + + // Filter out the message with the target sequence + const filteredMessages = messages.filter( + (msg) => msg.metadata?.historySequence !== historySequence + ); + + // If nothing was removed, return success (idempotent) + if (filteredMessages.length === messages.length) { + return Ok(undefined); + } + + // Rewrite entire file without the deleted message + const historyEntries = filteredMessages + .map((msg) => JSON.stringify({ ...msg, workspaceId }) + "\n") + .join(""); + + await writeFileAtomic(historyPath, historyEntries); + return Ok(undefined); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return Err(`Failed to delete message: ${message}`); + } + }); + } + /** * Truncate history after a specific message ID * Removes the message with the given ID and all subsequent messages diff --git a/src/node/services/partialService.ts b/src/node/services/partialService.ts index 0f3677ed9e..7c594c35d4 100644 --- a/src/node/services/partialService.ts +++ b/src/node/services/partialService.ts @@ -156,10 +156,13 @@ export class PartialService { (msg) => msg.metadata?.historySequence === partialSeq ); + const partialHasContent = (partial.parts?.length ?? 0) > 0; + const existingIsEmpty = (existingMessage?.parts?.length ?? 0) === 0; + const shouldCommit = (!existingMessage || // No message with this sequence yet (partial.parts?.length ?? 0) > (existingMessage.parts?.length ?? 0)) && // Partial has more parts - (partial.parts?.length ?? 0) > 0; // Don't commit empty messages + partialHasContent; // Don't commit empty messages if (shouldCommit) { if (existingMessage) { @@ -175,6 +178,18 @@ export class PartialService { return appendResult; } } + } else if (!partialHasContent && existingMessage && existingIsEmpty) { + // Partial has no content AND there's an empty placeholder in history + // This happens when stream is interrupted before producing any content + // Delete the empty placeholder to prevent "content field is empty" API errors + const deleteResult = await this.historyService.deleteMessageBySequence( + workspaceId, + partialSeq + ); + if (!deleteResult.success) { + // Log but don't fail - the message filter will catch this as fallback + log.warn(`Failed to delete empty placeholder: ${deleteResult.error}`); + } } // Delete partial.json after successful commit (or if already finalized)