diff --git a/apps/web/utils/ai/choose-rule/execute.test.ts b/apps/web/utils/ai/choose-rule/execute.test.ts index 4bad34b6b9..5cc0b549ea 100644 --- a/apps/web/utils/ai/choose-rule/execute.test.ts +++ b/apps/web/utils/ai/choose-rule/execute.test.ts @@ -220,6 +220,33 @@ describe("executeAct", () => { }); }); + it("keeps the rule APPLIED when an action skips itself on purpose", async () => { + mockRunActionFunction.mockResolvedValueOnce({ skipped: true }); + + const executedRule = { + ...baseExecutedRule, + actionItems: [{ id: "action-1", type: ActionType.NOTIFY_SENDER }], + } as any; + + const result = await executeAct({ + client: mockClient, + executedRule, + message, + emailAccount, + logger, + }); + + expect(result).toBe(ExecutedRuleStatus.APPLIED); + expect(mockExecutedActionUpdate).toHaveBeenCalledWith({ + where: { id: "action-1" }, + data: { + executionStatus: "SKIPPED", + executedAt: expect.any(Date), + executionError: Prisma.DbNull, + }, + }); + }); + it("continues later messaging notifications after one delivery failure", async () => { mockRunActionFunction .mockResolvedValueOnce({ diff --git a/apps/web/utils/cold-email/has-prior-contact.test.ts b/apps/web/utils/cold-email/has-prior-contact.test.ts new file mode 100644 index 0000000000..363780ee39 --- /dev/null +++ b/apps/web/utils/cold-email/has-prior-contact.test.ts @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { hasPriorContactOrAssumeYes } from "./has-prior-contact"; +import { createTestLogger } from "@/__tests__/helpers"; + +const logger = createTestLogger(); + +describe("hasPriorContactOrAssumeYes", () => { + const provider = { + hasPreviousCommunicationsWithSenderOrDomain: vi.fn(), + }; + + const check = (overrides: Record = {}) => + hasPriorContactOrAssumeYes({ + provider: provider as never, + from: "sender@example.com", + date: new Date(), + messageId: "msg-1", + logger, + ...overrides, + }); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("reports what the provider found", async () => { + provider.hasPreviousCommunicationsWithSenderOrDomain.mockResolvedValue( + false, + ); + + await expect(check()).resolves.toBe(false); + }); + + // Each of these would otherwise read as "no prior contact", which is the input that + // pushes the cold email blocker toward blocking a sender we could not verify. + it.each([ + [ + "the provider errors", + {}, + () => + provider.hasPreviousCommunicationsWithSenderOrDomain.mockRejectedValue( + new Error("api down"), + ), + ], + ["the sender is blank", { from: " " }, () => {}], + ["the date is missing", { date: undefined }, () => {}], + ["the date is invalid", { date: new Date(Number.NaN) }, () => {}], + ["the message id is missing", { messageId: undefined }, () => {}], + ])("assumes contact when %s", async (_name, overrides, arrange) => { + provider.hasPreviousCommunicationsWithSenderOrDomain.mockResolvedValue( + false, + ); + arrange(); + + await expect(check(overrides)).resolves.toBe(true); + }); + + it.each([ + { from: " " }, + { date: undefined }, + { messageId: undefined }, + ])("does not call the provider when contact cannot be identified", async (overrides) => { + await check(overrides); + + expect( + provider.hasPreviousCommunicationsWithSenderOrDomain, + ).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/utils/cold-email/has-prior-contact.ts b/apps/web/utils/cold-email/has-prior-contact.ts new file mode 100644 index 0000000000..47836f2c89 --- /dev/null +++ b/apps/web/utils/cold-email/has-prior-contact.ts @@ -0,0 +1,53 @@ +import type { EmailProvider } from "@/utils/email/types"; +import type { Logger } from "@/utils/logger"; + +/** + * Whether the user has corresponded with this sender before, assuming yes whenever we + * cannot tell. + * + * Blocking a sender is compounding: it labels, archives, and on many accounts emails + * them to say their message was unsolicited. Letting a cold email through costs one + * email in the inbox. So a missing date, an unreadable message, or a provider outage + * must all resolve toward leaving the sender alone rather than toward blocking them. + */ +export async function hasPriorContactOrAssumeYes({ + provider, + from, + date, + messageId, + logger, +}: { + provider: EmailProvider; + from: string; + date: Date | undefined; + messageId: string | undefined; + logger: Logger; +}): Promise { + if ( + !from.trim() || + !date || + Number.isNaN(date.getTime()) || + !messageId?.trim() + ) { + logger.warn( + "Assuming prior contact - message is missing a sender, date, or id", + ); + return true; + } + + try { + return await provider.hasPreviousCommunicationsWithSenderOrDomain({ + from, + date, + messageId, + }); + } catch (error) { + logger.warn( + "Assuming prior contact - could not check for previous emails", + { + error, + }, + ); + return true; + } +} diff --git a/apps/web/utils/cold-email/is-cold-email.test.ts b/apps/web/utils/cold-email/is-cold-email.test.ts index 3f1108be42..91286dd678 100644 --- a/apps/web/utils/cold-email/is-cold-email.test.ts +++ b/apps/web/utils/cold-email/is-cold-email.test.ts @@ -141,6 +141,85 @@ describe("isColdEmail", () => { }); }); + // Guarded here rather than only at the actions, so a colleague is never labelled + // or archived either. + it("should not classify a colleague as cold", async () => { + vi.mocked(prisma.groupItem.findFirst).mockResolvedValue(null); + + const result = await isColdEmail({ + email: { + id: "msg-internal", + from: "ceo@company.com", + to: "user@company.com", + subject: "Quick favour", + content: "Can you take a look at this?", + date: new Date(), + }, + emailAccount: getEmailAccount({ + id: "test-account-id", + email: "user@company.com", + }), + provider: mockProvider as never, + coldEmailRule: { instructions: "test instructions", groupId: "group-id" }, + }); + + expect(result.isColdEmail).toBe(false); + expect( + mockProvider.hasPreviousCommunicationsWithSenderOrDomain, + ).not.toHaveBeenCalled(); + }); + + it("should not classify a colleague as cold when a learned pattern exists", async () => { + vi.mocked(prisma.groupItem.findFirst).mockResolvedValue({ + id: "group-item-id", + type: GroupItemType.FROM, + value: "ceo@company.com", + exclude: false, + group: { id: "group-id", name: "Cold Email" }, + } as any); + + const result = await isColdEmail({ + email: { + id: "msg-internal", + from: "ceo@company.com", + to: "user@company.com", + subject: "Quick favour", + content: "Can you take a look at this?", + date: new Date(), + }, + emailAccount: getEmailAccount({ + id: "test-account-id", + email: "user@company.com", + }), + provider: mockProvider as never, + coldEmailRule: { instructions: "test instructions", groupId: "group-id" }, + }); + + expect(result.isColdEmail).toBe(false); + }); + + // Blocking a sender we could not verify is worse than missing a cold email. + it("should not classify as cold when prior contact cannot be checked", async () => { + vi.mocked(prisma.groupItem.findFirst).mockResolvedValue(null); + + const result = await isColdEmail({ + email: { + id: "msg-no-date", + from: "unknown@example.com", + to: "user@test.com", + subject: "Hello", + content: "Hello", + date: undefined as never, + }, + emailAccount: getEmailAccount({ id: "test-account-id" }), + provider: mockProvider as never, + coldEmailRule: { instructions: "test instructions", groupId: "group-id" }, + }); + + expect(result.isColdEmail).toBe(false); + expect(result.reason).toBe("hasPreviousEmail"); + }); + it("should handle various email formats consistently", async () => { const emailAccount = getEmailAccount({ id: "test-account-id" }); const normalizedEmail = "sender@example.com"; diff --git a/apps/web/utils/cold-email/is-cold-email.ts b/apps/web/utils/cold-email/is-cold-email.ts index a472094276..33af090b62 100644 --- a/apps/web/utils/cold-email/is-cold-email.ts +++ b/apps/web/utils/cold-email/is-cold-email.ts @@ -10,7 +10,8 @@ import type { EmailForLLM } from "@/utils/types"; import type { EmailProvider } from "@/utils/email/types"; import { getModel, type ModelType } from "@/utils/llms/model"; import { createGenerateObject } from "@/utils/llms"; -import { extractEmailAddress } from "@/utils/email"; +import { extractEmailAddress, isSameOrganization } from "@/utils/email"; +import { hasPriorContactOrAssumeYes } from "@/utils/cold-email/has-prior-contact"; export const COLD_EMAIL_FOLDER_NAME = "Cold Emails"; @@ -52,6 +53,13 @@ export async function isColdEmail({ logger.info("Checking is cold email"); + // Nobody at your own company is a cold emailer. Checked here rather than only at the + // actions, so a colleague is never labelled or archived either. + if (isSameOrganization(email.from, emailAccount.email)) { + logger.info("Sender is internal"); + return { isColdEmail: false, reason: "hasPreviousEmail" }; + } + // Check if we marked it as a cold email already const groupId = coldEmailRule?.groupId; let patternMatch: @@ -95,14 +103,13 @@ export async function isColdEmail({ return { isColdEmail: false, reason: "excluded" }; } - const hasPreviousEmail = - email.date && email.id - ? await provider.hasPreviousCommunicationsWithSenderOrDomain({ - from: extractEmailAddress(email.from) || email.from, - date: email.date, - messageId: email.id, - }) - : false; + const hasPreviousEmail = await hasPriorContactOrAssumeYes({ + provider, + from: extractEmailAddress(email.from) || email.from, + date: email.date, + messageId: email.id, + logger, + }); if (hasPreviousEmail) { logger.info("Has previous email"); diff --git a/apps/web/utils/email/microsoft.ts b/apps/web/utils/email/microsoft.ts index 938bf9ead2..1717984af1 100644 --- a/apps/web/utils/email/microsoft.ts +++ b/apps/web/utils/email/microsoft.ts @@ -1745,113 +1745,39 @@ export class OutlookProvider implements EmailProvider { date: Date; messageId: string; }): Promise { - try { - // Use shared logic: for public domains search by full email, for company domains search by domain - const searchTerm = getSearchTermForSender(options.from); - const isFullEmail = searchTerm.includes("@"); - - const dateString = options.date.toISOString(); - - // For domain matching, use $search instead of $filter since endsWith has limitations - // For exact email matching, use $filter with eq (case-insensitive for email addresses) - if (!isFullEmail) { - // Domain-based search - use $search for both sent and received - const escapedKqlDomain = searchTerm - .replace(/\\/g, "\\\\") - .replace(/"/g, '\\"'); - - const [sentResponse, receivedResponse] = await Promise.all([ - this.client - .getClient() - .api("/me/messages") - .search(`"to:@${escapedKqlDomain}"`) - .top(5) - .select("id,sentDateTime") - .get() - .catch((error) => { - this.logger.warn("Error checking sent messages (domain)", { - error, - }); - return { value: [] }; - }), + // Use shared logic: for public domains search by full email, for company domains search by domain + const searchTerm = getSearchTermForSender(options.from); + const isFullEmail = searchTerm.includes("@"); - this.client - .getClient() - .api("/me/messages") - .search(`"from:@${escapedKqlDomain}"`) - .top(5) - .select("id,receivedDateTime") - .get() - .catch((error) => { - this.logger.warn("Error checking received messages (domain)", { - error, - }); - return { value: [] }; - }), - ]); + const dateString = options.date.toISOString(); - // Filter by date since $search doesn't support date filtering well - const validSentMessages = (sentResponse.value || []).filter( - (msg: Message) => { - if (!msg.sentDateTime) return false; - return new Date(msg.sentDateTime) < options.date; - }, - ); - - const validReceivedMessages = (receivedResponse.value || []).filter( - (msg: Message) => { - if (!msg.receivedDateTime) return false; - return new Date(msg.receivedDateTime) < options.date; - }, - ); - - const messages = [...validSentMessages, ...validReceivedMessages]; - return messages.some((message) => message.id !== options.messageId); - } - - // Full email search - use $filter for received, $search for sent - const escapedSearchTerm = escapeODataString(searchTerm); - const receivedFilter = `from/emailAddress/address eq '${escapedSearchTerm}' and receivedDateTime lt ${dateString}`; - - // Use $search for sent messages as $filter on toRecipients is unreliable - const escapedKqlSearchTerm = searchTerm + // For domain matching, use $search instead of $filter since endsWith has limitations + // For exact email matching, use $filter with eq (case-insensitive for email addresses) + if (!isFullEmail) { + // Domain-based search - use $search for both sent and received + const escapedKqlDomain = searchTerm .replace(/\\/g, "\\\\") .replace(/"/g, '\\"'); - const sentSearch = `"to:${escapedKqlSearchTerm}"`; const [sentResponse, receivedResponse] = await Promise.all([ this.client .getClient() .api("/me/messages") - .search(sentSearch) - .top(5) // Increase top to account for potential future messages we filter out + .search(`"to:@${escapedKqlDomain}"`) + .top(5) .select("id,sentDateTime") - .get() - .catch((error) => { - this.logger.warn("Error checking sent messages", { - error, - search: sentSearch, - }); - return { value: [] }; - }), + .get(), this.client .getClient() .api("/me/messages") - .filter(receivedFilter) - .top(2) - .select("id") - .get() - .catch((error) => { - this.logger.warn("Error checking received messages", { - error, - filter: receivedFilter, - }); - return { value: [] }; - }), + .search(`"from:@${escapedKqlDomain}"`) + .top(5) + .select("id,receivedDateTime") + .get(), ]); - // Filter sent messages by date since $search doesn't support date filtering well + // Filter by date since $search doesn't support date filtering well const validSentMessages = (sentResponse.value || []).filter( (msg: Message) => { if (!msg.sentDateTime) return false; @@ -1859,18 +1785,56 @@ export class OutlookProvider implements EmailProvider { }, ); - const messages = [ - ...validSentMessages, - ...(receivedResponse.value || []), - ]; + const validReceivedMessages = (receivedResponse.value || []).filter( + (msg: Message) => { + if (!msg.receivedDateTime) return false; + return new Date(msg.receivedDateTime) < options.date; + }, + ); + const messages = [...validSentMessages, ...validReceivedMessages]; return messages.some((message) => message.id !== options.messageId); - } catch (error) { - this.logger.warn("Error checking previous communications", { - error, - }); - return false; } + + // Full email search - use $filter for received, $search for sent + const escapedSearchTerm = escapeODataString(searchTerm); + const receivedFilter = `from/emailAddress/address eq '${escapedSearchTerm}' and receivedDateTime lt ${dateString}`; + + // Use $search for sent messages as $filter on toRecipients is unreliable + const escapedKqlSearchTerm = searchTerm + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"'); + const sentSearch = `"to:${escapedKqlSearchTerm}"`; + + const [sentResponse, receivedResponse] = await Promise.all([ + this.client + .getClient() + .api("/me/messages") + .search(sentSearch) + .top(5) // Increase top to account for potential future messages we filter out + .select("id,sentDateTime") + .get(), + + this.client + .getClient() + .api("/me/messages") + .filter(receivedFilter) + .top(2) + .select("id") + .get(), + ]); + + // Filter sent messages by date since $search doesn't support date filtering well + const validSentMessages = (sentResponse.value || []).filter( + (msg: Message) => { + if (!msg.sentDateTime) return false; + return new Date(msg.sentDateTime) < options.date; + }, + ); + + const messages = [...validSentMessages, ...(receivedResponse.value || [])]; + + return messages.some((message) => message.id !== options.messageId); } async getThreadsFromSenderWithSubject( diff --git a/apps/web/utils/rule/learned-patterns.test.ts b/apps/web/utils/rule/learned-patterns.test.ts index b7cfbb4461..2526c01861 100644 --- a/apps/web/utils/rule/learned-patterns.test.ts +++ b/apps/web/utils/rule/learned-patterns.test.ts @@ -54,11 +54,17 @@ describe("saveLearnedPattern", () => { value: "test@example.com", }, }, - update: expect.objectContaining({ exclude: false }), + // Omitted fields must not reset stored state: overwriting exclude would + // silently re-block a sender the user had corrected. + update: expect.objectContaining({ + exclude: undefined, + source: undefined, + }), create: expect.objectContaining({ groupId: existingGroupId, type: GroupItemType.FROM, value: "test@example.com", + exclude: false, }), }); }); diff --git a/apps/web/utils/rule/learned-patterns.ts b/apps/web/utils/rule/learned-patterns.ts index 2eb7dbeb6b..617269993b 100644 --- a/apps/web/utils/rule/learned-patterns.ts +++ b/apps/web/utils/rule/learned-patterns.ts @@ -1,6 +1,6 @@ import prisma from "@/utils/prisma"; import type { Logger } from "@/utils/logger"; -import { GroupItemType, type GroupItemSource } from "@/generated/prisma/enums"; +import { GroupItemSource, GroupItemType } from "@/generated/prisma/enums"; import { isDuplicateError } from "@/utils/prisma-helpers"; /** @@ -12,7 +12,7 @@ export async function saveLearnedPattern({ emailAccountId, from, ruleId, - exclude = false, + exclude, logger, reason, threadId, @@ -55,18 +55,23 @@ export async function saveLearnedPattern({ value: from, }, }, + // Undefined fields are left untouched by Prisma, so a caller only overwrites what + // it actually knows. Inferred writers must not restate `source`, which records how + // the pattern was first learned and is what undoing a junk action keys off. An + // explicit user correction is not an inference and does claim the row, so undoing a + // junk action can never delete it. update: { exclude, reason, threadId, messageId, - source, + source: source === GroupItemSource.USER ? source : undefined, }, create: { groupId, type: GroupItemType.FROM, value: from, - exclude, + exclude: exclude ?? false, reason, threadId, messageId, diff --git a/apps/web/utils/scheduled-actions/executor.test.ts b/apps/web/utils/scheduled-actions/executor.test.ts index 3896feb6c8..3a2eca0436 100644 --- a/apps/web/utils/scheduled-actions/executor.test.ts +++ b/apps/web/utils/scheduled-actions/executor.test.ts @@ -287,6 +287,50 @@ describe("executor", () => { ); }); + it("links a deliberately skipped action to the completed scheduled action", async () => { + const scheduledNotificationAction = { + ...mockScheduledAction, + actionType: ActionType.NOTIFY_SENDER, + }; + mockScheduledActionUpdate( + ScheduledActionStatus.COMPLETED, + scheduledNotificationAction, + ); + mockExecutedActionCreate({ type: ActionType.NOTIFY_SENDER }); + mockExecutedRuleFind(); + mockCompletionCounts({ pendingActions: 0, failedActions: 0 }); + mockExecutedRuleUpdate(ExecutedRuleStatus.APPLIED); + vi.mocked(runActionFunction).mockResolvedValue({ skipped: true }); + + const result = await executeScheduledAction( + scheduledNotificationAction, + await getMockEmailProvider(), + logger, + ); + + expect(result).toEqual({ + success: true, + executedActionId: "executed-action-123", + }); + expect(prisma.scheduledAction.update).toHaveBeenCalledWith({ + where: { id: "scheduled-action-123" }, + data: { + status: ScheduledActionStatus.COMPLETED, + executedAt: expect.any(Date), + executedActionId: "executed-action-123", + }, + }); + expect(prisma.executedAction.update).toHaveBeenCalledWith({ + where: { id: "executed-action-123" }, + data: { + executionStatus: ExecutedActionStatus.SKIPPED, + executedAt: expect.any(Date), + executionError: Prisma.DbNull, + }, + }); + expectExecutedRuleStatus(ExecutedRuleStatus.APPLIED); + }); + it("should handle account not found errors", async () => { mockScheduledActionUpdate(ScheduledActionStatus.FAILED); mockCompletionCounts({ pendingActions: 0, failedActions: 1 }); diff --git a/apps/web/utils/webhook/google/process-history-item.test.ts b/apps/web/utils/webhook/google/process-history-item.test.ts index 905281b1a1..fc851f86ef 100644 --- a/apps/web/utils/webhook/google/process-history-item.test.ts +++ b/apps/web/utils/webhook/google/process-history-item.test.ts @@ -11,6 +11,7 @@ import { GmailLabel } from "@/utils/gmail/label"; import { getEmailAccount, createTestLogger } from "@/__tests__/helpers"; import { createEmailProvider } from "@/utils/email/provider"; import { handleOutboundMessage } from "@/utils/reply-tracker/handle-outbound"; +import { handleLabelRemovedEvent } from "@/utils/webhook/google/process-label-removed-event"; const logger = createTestLogger(); @@ -101,6 +102,10 @@ vi.mock("@/utils/rule/learned-patterns", () => ({ saveLearnedPatterns: vi.fn().mockResolvedValue(undefined), })); +vi.mock("@/utils/webhook/google/process-label-removed-event", () => ({ + handleLabelRemovedEvent: vi.fn().mockResolvedValue(undefined), +})); + describe("processHistoryItem", () => { beforeEach(() => { vi.clearAllMocks(); @@ -262,6 +267,26 @@ describe("processHistoryItem", () => { }); }); + it("allows spam learning again after spam is removed from a thread", async () => { + const spamLearnedThreadIds = new Set(["thread-123"]); + const options = { + ...defaultOptions, + emailAccount: getDefaultEmailAccount(), + spamLearnedThreadIds, + }; + + await processHistoryItem( + createHistoryItem("123", "thread-123", HistoryEventType.LABEL_REMOVED, [ + GmailLabel.SPAM, + ]), + options, + logger, + ); + + expect(handleLabelRemovedEvent).toHaveBeenCalled(); + expect(spamLearnedThreadIds).not.toContain("thread-123"); + }); + it("should skip if email is unsubscribed", async () => { const mockPrisma = await import("@/utils/prisma"); vi.mocked(mockPrisma.default.newsletter.findFirst).mockResolvedValueOnce({ diff --git a/apps/web/utils/webhook/google/process-history-item.ts b/apps/web/utils/webhook/google/process-history-item.ts index c9c296086b..3fd47d52cd 100644 --- a/apps/web/utils/webhook/google/process-history-item.ts +++ b/apps/web/utils/webhook/google/process-history-item.ts @@ -20,7 +20,13 @@ export async function processHistoryItem( options: ProcessHistoryOptions, logger: Logger, ) { - const { emailAccount, hasAutomationRules, hasAiAccess, rules } = options; + const { + emailAccount, + hasAutomationRules, + hasAiAccess, + rules, + spamLearnedThreadIds, + } = options; const { type, item } = historyItem; const messageId = item.message?.id; const threadId = item.message?.threadId; @@ -66,9 +72,14 @@ export async function processHistoryItem( // Handle Google-specific label events if (type === HistoryEventType.LABEL_REMOVED) { + const labelRemovedItem = item as gmail_v1.Schema$HistoryLabelRemoved; + if (labelRemovedItem.labelIds?.includes(GmailLabel.SPAM)) { + spamLearnedThreadIds.delete(threadId); + } + logger.info("Processing label removed event for learning"); return handleLabelRemovedEvent( - item as gmail_v1.Schema$HistoryLabelRemoved, + labelRemovedItem, { emailAccount, provider, @@ -88,6 +99,7 @@ export async function processHistoryItem( { emailAccount, provider, + spamLearnedThreadIds, }, logger, ); diff --git a/apps/web/utils/webhook/google/process-history.ts b/apps/web/utils/webhook/google/process-history.ts index efe854759d..b2a0796838 100644 --- a/apps/web/utils/webhook/google/process-history.ts +++ b/apps/web/utils/webhook/google/process-history.ts @@ -158,6 +158,7 @@ export async function processHistoryForUser( await processHistory( { history: historyEntries, + spamLearnedThreadIds: new Set(), gmail, accessToken: accountAccessToken, hasAutomationRules, diff --git a/apps/web/utils/webhook/google/process-label-added-event.test.ts b/apps/web/utils/webhook/google/process-label-added-event.test.ts index 4425ace3a2..aa90b29df2 100644 --- a/apps/web/utils/webhook/google/process-label-added-event.test.ts +++ b/apps/web/utils/webhook/google/process-label-added-event.test.ts @@ -102,10 +102,7 @@ describe("process-label-added-event", () => { hasPreviousCommunicationsWithSenderOrDomain: vi.fn(), } as any; - const defaultOptions = { - emailAccount: mockEmailAccount, - provider: mockProvider, - }; + let defaultOptions: Parameters[1]; // The junked message is always "123" so it is found in the thread. const mockThreadSenders = (...senders: string[]) => { @@ -123,6 +120,11 @@ describe("process-label-added-event", () => { describe("handleLabelAddedEvent", () => { beforeEach(() => { + defaultOptions = { + emailAccount: mockEmailAccount, + provider: mockProvider, + spamLearnedThreadIds: new Set(), + }; mockThreadSenders("sender@example.com"); vi.mocked(fetchSenderFromMessage).mockResolvedValue("sender@example.com"); vi.mocked( @@ -307,6 +309,24 @@ describe("process-label-added-event", () => { expect(saveLearnedPattern).not.toHaveBeenCalled(); }); + it("should not learn when the junked message date is missing", async () => { + vi.mocked(mockProvider.getThreadMessages).mockResolvedValue([ + { + id: "123", + internalDate: undefined, + headers: { from: "cold@vendor.com" }, + }, + ]); + vi.mocked(fetchSenderFromMessage).mockResolvedValue("cold@vendor.com"); + + await junkMessage(); + + expect( + mockProvider.hasPreviousCommunicationsWithSenderOrDomain, + ).not.toHaveBeenCalled(); + expect(saveLearnedPattern).not.toHaveBeenCalled(); + }); + it("should learn the sole sender of a one-way thread", async () => { mockThreadSenders("cold@vendor.com", "cold@vendor.com"); vi.mocked(fetchSenderFromMessage).mockResolvedValue("cold@vendor.com"); @@ -318,6 +338,40 @@ describe("process-label-added-event", () => { ); }); + it("should only read the thread once when a whole thread is junked", async () => { + mockThreadSenders("cold@vendor.com", "cold@vendor.com"); + vi.mocked(fetchSenderFromMessage).mockResolvedValue("cold@vendor.com"); + // Gmail fires one event per message in the junked thread. + await junkMessage(); + await handleLabelAddedEvent( + createLabelAddedItem("456", "thread-123"), + defaultOptions, + logger, + ); + + expect(mockProvider.getThreadMessages).toHaveBeenCalledTimes(1); + expect(saveLearnedPattern).toHaveBeenCalledTimes(1); + }); + + it("retries spam learning after the first thread read fails", async () => { + vi.mocked(mockProvider.getThreadMessages) + .mockRejectedValueOnce(new Error("Temporary provider error")) + .mockResolvedValueOnce([ + { + id: "123", + internalDate: "1700000000000", + headers: { from: "cold@vendor.com" }, + }, + ]); + vi.mocked(fetchSenderFromMessage).mockResolvedValue("cold@vendor.com"); + + await junkMessage(); + await junkMessage(); + + expect(mockProvider.getThreadMessages).toHaveBeenCalledTimes(2); + expect(saveLearnedPattern).toHaveBeenCalledTimes(1); + }); + it.each([ ["the account has no cold email rule", null], ["the sender is already known", { id: "rule-123", groupId: "group-1" }], diff --git a/apps/web/utils/webhook/google/process-label-added-event.ts b/apps/web/utils/webhook/google/process-label-added-event.ts index f9c47b3f78..0eb16c042c 100644 --- a/apps/web/utils/webhook/google/process-label-added-event.ts +++ b/apps/web/utils/webhook/google/process-label-added-event.ts @@ -20,6 +20,7 @@ import { import { fetchSenderFromMessage } from "@/utils/webhook/google/fetch-sender-from-message"; import { isSameEmailAddress, isSameOrganization } from "@/utils/email"; import { internalDateToDate } from "@/utils/date"; +import { hasPriorContactOrAssumeYes } from "@/utils/cold-email/has-prior-contact"; /** * When labels are added to an email: @@ -31,9 +32,11 @@ export async function handleLabelAddedEvent( { emailAccount, provider, + spamLearnedThreadIds, }: { emailAccount: EmailAccountWithAI; provider: EmailProvider; + spamLearnedThreadIds: Set; }, logger: Logger, ) { @@ -52,7 +55,11 @@ export async function handleLabelAddedEvent( (labelId) => !GMAIL_SYSTEM_LABELS.includes(labelId), ); - if (!hasSpam && classifiableLabelIds.length === 0) { + // Junking a thread fires one event per message, but spam learning is thread-scoped + // and the answer is the same for all of them, so only the first event does the work. + const shouldLearnSpam = hasSpam && !spamLearnedThreadIds.has(threadId); + + if (!shouldLearnSpam && classifiableLabelIds.length === 0) { logger.trace("No actionable labels added, skipping", { messageId, addedLabelIds, @@ -63,8 +70,8 @@ export async function handleLabelAddedEvent( const sender = await fetchSenderFromMessage(messageId, provider, logger); if (!sender) return; - if (hasSpam) { - await learnColdEmailFromSpam({ + if (shouldLearnSpam) { + const spamLearningHandled = await learnColdEmailFromSpam({ sender, messageId, threadId, @@ -72,6 +79,9 @@ export async function handleLabelAddedEvent( provider, logger, }); + if (spamLearningHandled) { + spamLearnedThreadIds.add(threadId); + } } await Promise.all( @@ -112,7 +122,7 @@ async function learnColdEmailFromSpam({ if (isSameOrganization(sender, emailAccount.email)) { logger.info("Skipping cold email learning for an internal sender"); - return; + return true; } const coldEmailRule = await prisma.rule.findFirst({ @@ -126,7 +136,7 @@ async function learnColdEmailFromSpam({ if (!coldEmailRule) { logger.info("No Cold Email rule found for account, skipping"); - return; + return true; } // Don't overwrite existing patterns (e.g., AI classification) @@ -146,7 +156,7 @@ async function learnColdEmailFromSpam({ logger.trace("Sender already in cold email group, skipping", { sender, }); - return; + return true; } } @@ -157,7 +167,7 @@ async function learnColdEmailFromSpam({ provider, logger, }); - if (!threadMessages?.length) return; + if (!threadMessages?.length) return false; // Junking a conversation is not a claim about everyone who replied in it. if ( @@ -166,23 +176,27 @@ async function learnColdEmailFromSpam({ logger.info( "Skipping cold email learning - junked thread is a conversation", ); - return; + return true; } // The check every other pattern writer runs. Junking one message from someone you // already correspond with does not make them a cold emailer. const junkedMessage = threadMessages.find((m) => m.id === messageId); - const hasPreviousEmail = junkedMessage - ? await provider.hasPreviousCommunicationsWithSenderOrDomain({ - from: sender, - date: internalDateToDate(junkedMessage.internalDate), - messageId, - }) - : true; + const hasPreviousEmail = await hasPriorContactOrAssumeYes({ + provider, + from: sender, + date: junkedMessage + ? internalDateToDate(junkedMessage.internalDate, { + fallbackToNow: false, + }) + : undefined, + messageId, + logger, + }); if (hasPreviousEmail) { logger.info("Skipping cold email learning - sender is a known contact"); - return; + return true; } logger.trace("Saving cold email learned pattern from SPAM action", { @@ -200,6 +214,7 @@ async function learnColdEmailFromSpam({ reason: "Marked as spam by user", source: GroupItemSource.LABEL_ADDED, }); + return true; } async function recordClassificationFromLabelAdd({ diff --git a/apps/web/utils/webhook/google/types.ts b/apps/web/utils/webhook/google/types.ts index f298ef66d7..c9ec7fdb83 100644 --- a/apps/web/utils/webhook/google/types.ts +++ b/apps/web/utils/webhook/google/types.ts @@ -14,6 +14,9 @@ export type HistoryEventType = export type ProcessHistoryOptions = { history: gmail_v1.Schema$History[]; + // Gmail labels every message in a thread as spam, so junking one thread fires one + // event per message. Spam learning is thread-scoped, so it only runs for the first. + spamLearnedThreadIds: Set; gmail: gmail_v1.Gmail; accessToken: string; rules: RuleWithActions[];