cold email: fail toward not blocking, and stop clobbering learned patterns - #3075
Open
elie222 wants to merge 5 commits into
Open
cold email: fail toward not blocking, and stop clobbering learned patterns#3075elie222 wants to merge 5 commits into
elie222 wants to merge 5 commits into
Conversation
…terns When a check that feeds the cold email decision cannot answer, the code now defaults to leaving the sender alone. Blocking is compounding: a wrongly blocked sender is labelled, archived out of the inbox, and on many accounts auto-emailed to say their message was unsolicited. Missing a cold email costs one email in the inbox. - The Outlook prior-contact lookup returned false when the Graph query failed. False reads as "no prior contact", which pushes toward blocking, so a transient API error could start blocking real contacts. It now fails toward reporting contact. Partial failures no longer resolve to an empty result set for the same reason. - A message without a date or id cannot be checked for prior contact, and is no longer eligible to be classified as a cold email. Separately: - saveLearnedPattern overwrote exclude and source on every upsert, including for callers that never passed them. A caller that omitted exclude would silently re-block a sender the user had corrected. Callers now only overwrite what they supply, and source keeps recording how the pattern was first learned. - An action that declines to run on purpose is recorded as skipped rather than failed, so deliberate policy skips stop appearing as execution errors. - Junking a thread fires one webhook event per message. Spam learning is thread-scoped, so only the first event now pays for the thread read. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
The fail-safe was written three times: inside the Outlook provider's catch, and as two hand-written ternaries at the call sites. Gmail had none, so a lookup failure there aborted the whole rule run instead of degrading to "not cold". Moves it to a single hasPriorContactOrAssumeYes helper that both callers use. Providers no longer encode what a failed lookup means to a classifier: Outlook's catch is gone and errors propagate like Gmail's always did. One place to read the policy, one set of tests, identical behaviour on both providers. Also applies the same rule where it was missing: - An explicit user correction may now claim a learned pattern's source again. Blocking inferred writers from restating it was right, but it also stopped "not a cold email" from promoting the row, so a later un-junk deleted the user's correction and the sender became blockable again. - isColdEmail now returns early for a sender inside the user's own organisation. The guard existed when learning from junk and when notifying the sender, but not in the step that actually decides, so a colleague could still be labelled and archived. Cleanups from review: - Junking a thread no longer fetches the sender for every message in it. The dedupe check now runs before that call, not after. - The batch-scoped thread set is required rather than optional, so a caller cannot silently lose the dedupe, and is built at its single construction site. - Reuse asRecord for the skipped-result check, name the ActionSkipped type, and drop test assertions that restated ones already made in the same file. Co-Authored-By: Claude <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Guiding rule
When a check that feeds the cold email decision cannot answer, default to leaving the sender alone.
The two errors are not symmetric. A wrongly blocked sender is labelled, archived out of the inbox, and on accounts with
NOTIFY_SENDERenabled, auto-emailed to say their message was unsolicited outreach. A missed cold email costs the user one email in the inbox.Fail toward not blocking
Outlook prior-contact lookup returned
falseon error.hasPreviousCommunicationsWithSenderOrDomainwrapped its Graph queries in acatchthat returnedfalse. Butfalsemeans "no prior contact", which is exactly the input that pushes the blocker toward blocking. A transient Graph outage could therefore start blocking established contacts. It now fails toward reporting contact.The four inner per-query
.catch()es had the same shape, resolving a failed query to{ value: [] }so a partial failure looked like a confirmed absence of history. They now propagate to the outer handler.Gmail's implementation has no
catch, so errors already propagate and no blocking occurs. Left as is.A message without a date or id can no longer be classified as cold. That combination makes the prior-contact check unrunnable, and the old fallback treated "couldn't check" as "no prior contact".
Pattern writes no longer clobber
saveLearnedPatternwroteexcludeandsourceon every upsert, including for callers that never passed them.excludedefaulted tofalse, so any caller omitting it would silently un-exclude a sender the user had explicitly corrected, and re-blocking begins.Callers now overwrite only what they supply, and
sourcekeeps recording how the pattern was first learned rather than what touched it last. Two consumers depend on that: undoing a junk action, and the audit trail.Reachable today via
analyze-sender-pattern, though it has written zero cold-email patterns in production so far, so this is a latent footgun rather than a live regression. Prisma already skipsundefinedon update, soreason/threadId/messageIdwere never actually being nulled.Skips are not failures
NOTIFY_SENDERis inACTION_RESULT_FAILURE_TYPES, so the internal-sender guard returning{ success: false }was persisted asExecutedActionStatus.FAILEDwith anexecutionError, and pushed intoactionFailures, which flips the whole rule toERROR. A deliberate policy decision was being reported as an execution failure.Actions can now return
{ skipped: true }, handled in both executors, following theExecutedActionStatus.SKIPPEDprecedent already used for the automated-archive exception.One thread read per junk
Gmail labels every message in a thread as spam, so junking an N-message thread fires N webhook events. Spam learning is thread-scoped, so all N produced the same answer, and on the conversation path nothing was written, meaning the duplicate guard never engaged and every event paid a full
threads.getthat downloads all N bodies.A batch-scoped set of thread ids means only the first event does the work. Junking a 25-message thread goes from 25 thread reads to 1.
Tests
Each change was mutation-tested: reverting the fix makes its test fail.
sourceback in the update blocknever overwrites sourcekeeps the rule APPLIED when an action skips itself on purposeshould only read the thread once when a whole thread is junkedshould not classify as cold when prior contact cannot be checkedFull suite: 495 files passed, 97 skipped, 0 failed. Lint clean, no new type errors.
🤖 Generated with Claude Code