Skip to content
Draft
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
72 changes: 72 additions & 0 deletions src/browser/utils/messages/modelMessageTransform.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
27 changes: 26 additions & 1 deletion src/browser/utils/messages/modelMessageTransform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}

/**
Expand Down
48 changes: 48 additions & 0 deletions src/node/services/historyService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
45 changes: 45 additions & 0 deletions src/node/services/historyService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Result<void>> {
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
Expand Down
17 changes: 16 additions & 1 deletion src/node/services/partialService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
Expand Down