Skip to content

Fix outlook cold email #1002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 24, 2025
Merged

Fix outlook cold email #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34123d1
fix outlook cold email
elie222 Nov 24, 2025
3a2b0a4
fix cold email check outlook
elie222 Nov 24, 2025
51f03ca
fix
elie222 Nov 24, 2025
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
1 change: 0 additions & 1 deletion apps/web/scripts/check-enum-imports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,3 @@ try {
console.error("Error running check:", error.message);
process.exit(1);
}

72 changes: 58 additions & 14 deletions apps/web/utils/email/microsoft.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,22 +1182,66 @@ export class OutlookProvider implements EmailProvider {
messageId: string;
}): Promise<boolean> {
try {
const response: { value: Message[] } = await this.client
.getClient()
.api("/me/messages")
.filter(
`(toRecipients/any(a:a/emailAddress/address eq '${escapeODataString(options.from)}') and sentDateTime lt ${options.date.toISOString()}) or (from/emailAddress/address eq '${escapeODataString(options.from)}' and receivedDateTime lt ${options.date.toISOString()})`,
)
.top(2)
.select("id")
.get();
const escapedFrom = escapeODataString(options.from);
const dateString = options.date.toISOString();

// Split into two parallel queries to avoid OData "invalid nodes" error
// when combining any() lambda with other filters.
const receivedFilter = `from/emailAddress/address eq '${escapedFrom}' and receivedDateTime lt ${dateString}`;

// Use $search for sent messages as $filter on toRecipients is unreliable
// We escape double quotes for the KQL search query
const escapedSearchFrom = options.from
.replace(/\\/g, "\\\\")
.replace(/"/g, '\\"');
const sentSearch = `"to:${escapedSearchFrom}"`;
Comment on lines +1194 to +1197
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Verify KQL escaping completeness and "to:" syntax reliability.

The manual escaping for the KQL search query only handles backslashes and double quotes. KQL supports additional special characters and operators (parentheses, AND, OR, NOT, wildcards, etc.) that could appear in email addresses or display names. While wrapping the query in double quotes provides some protection, edge cases like user(name)@example.com or emails containing KQL keywords might still cause unexpected behavior.

Additionally, verify that the to: property filter syntax is reliable for the Microsoft Graph $search API, as the previous comment mentions that $filter on toRecipients is unreliable.

Consider running this verification to check if the current escaping handles various email formats correctly:

Alternatively, search the web for Microsoft Graph KQL search syntax requirements:

🏁 Script executed:

#!/bin/bash
# Search for any usage patterns or tests related to KQL search escaping
# to understand what edge cases are already handled

rg -n -C3 "\.search\(" --type=ts

# Also check for any existing documentation or comments about KQL syntax
rg -n -C3 "KQL|Keyword Query" --type=ts

Length of output: 3108

🌐 Web query:

Microsoft Graph $search KQL syntax escaping requirements for email addresses

💡 Result:

Short answer — follow the Graph docs: put each KQL clause inside double quotes, escape any double-quote or backslash characters with a backslash, and URL‑encode any other special characters in the clause (e.g. @ → %40, + → %2B). Also include ConsistencyLevel: eventual when needed. Example requests:

Notes:

  • If you put raw special characters (un-encoded) in the URL you may get KQL syntax errors (see reported error for from:[email protected]). URL‑encode characters like @, +, & etc. to avoid the error. (stackoverflow.com)

If you want, tell me an example email or full request you’re trying and I’ll give the exact encoded $search string.

Citations:


This would search for existing test patterns related to email and special characters that might shed light on whether the current approach is working or if there are known issues.

However, based on the evidence gathered so far, I can now make a determination:

**Evidence Summary:**
1. Per RFC 3986, query parameter values must be percent-encoded, and many HTTP clients handle this encoding automatically
2. There is a known issue with encoding ampersand (&) symbols in $search expressions on the v1.0 endpoint
3. The code only escapes backslashes and double quotes—it does NOT perform URL encoding (like @ → %40)
4. According to the first web search, Microsoft Graph documentation specifies that special characters like @ must be URL-encoded as %40 in KQL search clauses

The review comment is substantively correct: the escaping is incomplete. However, I need to verify if this is actually causing issues in practice or if the SDK handles it automatically.

Let me search for how email addresses are typically used in practice:




This will show if there are any existing concerns or test cases around this functionality.

---

🏁 Script executed:

```shell
#!/bin/bash
# Search for any tests related to email search with special characters
# or any existing issues/TODOs related to email escaping

rg -n "email|@" --type=ts apps/web/__tests__ | grep -i "search\|special\|escape"

# Also look for any OData/KQL filtering patterns in the codebase
rg -n "to:" --type=ts

# Check if there are any tests for the microsoft.ts file specifically
fd -t f "microsoft.*test" apps/web/__tests__

Length of output: 34681

🏁 Script executed:

#!/bin/bash
# Look for actual test cases or usage of email addresses with the search functionality
rg -n -B5 -A5 "sentSearch|searchFrom" apps/web --type=ts

# Check if there are any GitHub issues or TODOs mentioning email encoding
rg -n "TODO|FIXME|BUG|XXX" apps/web/utils/email/microsoft.ts

Length of output: 1784

🌐 Web query:

@microsoft/microsoft-graph-client .search() method URL encoding parameter handling

💡 Result:

Short answer: .search() does not perform URL percent-encoding for you — it just sets the $search value you pass onto the request. You must URL‑encode (and in some cases double‑encode) any reserved characters (e.g. & , spaces, parentheses, quotes) before calling .search(). See Microsoft Graph query-parameter encoding guidance and the client docs showing .search() simply adds the string. [1][2][3]

Quick guidance:

  • Use encodeURIComponent(searchString) when building the value you pass to .search().
  • For $search expressions with characters that Graph requires double-encoding (per Microsoft docs), apply the additional encoding as described in the docs.
  • If you need full control you can also use .query("$search=" + encodedString) to supply the exact query string. [1][2][3]

Sources:
[1] Microsoft Graph query-parameter encoding and $search notes.
[2] @microsoft/microsoft-graph-client npm docs (query / $search behavior).
[3] jsDocs for @microsoft/microsoft-graph-client showing .search() behavior.

URL-encode email addresses in KQL $search queries—current code will fail for all searches.

Per Microsoft Graph client documentation, .search() does not perform URL percent-encoding automatically; you must URL-encode special characters like @ before passing the value to .search(). Email addresses always contain @, which must be encoded as %40.

The current implementation (lines 1194–1197) only escapes backslashes and double quotes but does NOT URL-encode the email address. This will cause KQL syntax errors at runtime for every search, resulting in the error handler silently returning an empty array and causing hasPreviousCommunicationsWithSenderOrDomain to incorrectly return false.

Fix: Apply URL encoding (e.g., encodeURIComponent()) to the email address after escaping quotes and backslashes, before constructing the KQL search query.

🤖 Prompt for AI Agents 
In apps/web/utils/email/microsoft.ts around lines 1194 to 1197, the code only
escapes backslashes and quotes but does not URL-encode the email address for the
KQL .search() call, so "@" and other special characters break the query; update
the flow to first escape backslashes and double quotes, then apply URL encoding
(e.g., encodeURIComponent) to the resulting string before interpolating it into
the KQL search expression so the email is percent-encoded (@" -> %40) and the
.search() call will succeed.


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()
.catch((error) => {
this.logger.error("Error checking sent messages", {
error,
search: sentSearch,
});
return { value: [] };
}),
Comment on lines +1199 to +1213
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Critical: .top(5) limit risks false negatives when combined with client-side date filtering.

The sent messages query fetches only the top 5 results without date filtering (since $search doesn't support it), then filters by date client-side (lines 1231-1237). If someone has sent more than 5 messages to this recipient and the 5 most recent are all after options.date, the query will fetch only those newer messages. The client-side filter will then exclude all 5, resulting in zero sent messages found—even if older sent messages (before the date) exist.

This breaks the cold email detection logic: the method would incorrectly return false (no prior communication) when prior communication actually occurred.

Apply this diff to significantly increase the limit and reduce false negative risk:

-          .top(5) // Increase top to account for potential future messages we filter out
+          .top(50) // Fetch more results to account for future messages filtered out client-side

Note: The ideal solution would be to use $filter with date constraints instead of $search, but the comment on line 1192 indicates $filter on toRecipients is unreliable. If that's no longer true, consider switching to $filter with a receivedDateTime constraint.

Verify whether $filter on toRecipients is still unreliable:

🌐 Web query:

Microsoft Graph API filter toRecipients reliability 2025

💡 Result:

Short answer: you can't reliably use $filter on toRecipients. Microsoft Graph does not support filtering on toRecipients/ccRecipients/bccRecipients (they're collection-of-complex-types), so use $search (e.g. ?$search="to:[[email protected]]" or ?$search="participants:[[email protected]]") or retrieve and filter client-side. [1][2][3]

Notes/limits:

  • $search is the recommended workaround but has its own limits (e.g. result caps per folder/search in some implementations). [4]

Sources:

  • Microsoft Tech Community discussion (toRecipients not filterable). [1]
  • Microsoft Q&A answer (confirming toRecipients cannot be filtered; use $search). [2]
  • StackOverflow thread and examples showing same behavior and $search workaround. [3]
  • Docs/notes on search limits (result caps / practical caveats). [4]

Critical: .top(5) limit risks false negatives when combined with client-side date filtering.

The sent messages query fetches only the top 5 results using $search, then filters by date client-side (lines 1231-1237). If someone has sent more than 5 messages to this recipient and the 5 most recent are all after options.date, the query will fetch only those newer messages. The client-side filter will then exclude all 5, resulting in zero sent messages found—even if older sent messages (before the date) exist.

This breaks the cold email detection logic: the method would incorrectly return false (no prior communication) when prior communication actually occurred.

Apply this diff to significantly increase the limit and reduce false negative risk:

-          .top(5) // Increase top to account for potential future messages we filter out
+          .top(50) // Fetch more results to account for future messages filtered out client-side

Note: Microsoft Graph does not support $filter on toRecipients (it's a collection-of-complex-type), so the current $search approach is the recommended workaround. Consider increasing .top() further if the organization typically exchanges many messages with the same recipients.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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()
.catch((error) => {
this.logger.error("Error checking sent messages", {
error,
search: sentSearch,
});
return { value: [] };
}),
const [sentResponse, receivedResponse] = await Promise.all([
this.client
.getClient()
.api("/me/messages")
.search(sentSearch)
.top(50) // Fetch more results to account for future messages filtered out client-side
.select("id,sentDateTime")
.get()
.catch((error) => {
this.logger.error("Error checking sent messages", {
error,
search: sentSearch,
});
return { value: [] };
}),
🤖 Prompt for AI Agents 
In apps/web/utils/email/microsoft.ts around lines 1199 to 1213, the query limits
sent messages to .top(5) which combined with client-side date filtering can
produce false negatives when a recipient has more than 5 recent messages;
increase the .top(...) value to a much larger number (e.g., 50 or 100) or make
it a configurable constant/ENV value, keep the existing client-side date filter,
and add a short comment explaining why the higher limit is required (to avoid
missing older matches due to $search limitations on toRecipients).


this.client
.getClient()
.api("/me/messages")
.filter(receivedFilter)
.top(2)
.select("id")
.get()
.catch((error) => {
this.logger.error("Error checking received messages", {
error,
filter: receivedFilter,
});
return { value: [] };
}),
Comment on lines +1207 to +1228
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Error handling silently masks failures, risking false negatives.

Both queries return { value: [] } on error, treating API failures, network issues, or malformed queries the same as "no messages found." This can cause the method to incorrectly return false when the real issue is a transient error or a query syntax problem.

Consider re-throwing errors or logging warnings more prominently, so that failures are visible and can be investigated rather than silently treated as "no prior communication."

Apply this diff to improve error visibility:

           .catch((error) => {
             this.logger.error("Error checking sent messages", {
               error,
               search: sentSearch,
             });
+            // Re-throw on non-transient errors to avoid silent false negatives
+            // For now, return empty to maintain existing behavior but consider revisiting
             return { value: [] };
           }),

         this.client
           .getClient()
           .api("/me/messages")
           .filter(receivedFilter)
           .top(2)
           .select("id")
           .get()
           .catch((error) => {
             this.logger.error("Error checking received messages", {
               error,
               filter: receivedFilter,
             });
+            // Re-throw on non-transient errors to avoid silent false negatives
+            // For now, return empty to maintain existing behavior but consider revisiting
             return { value: [] };
           }),

Alternatively, add monitoring/alerting on these error logs to ensure failures are investigated.

🤖 Prompt for AI Agents 
In apps/web/utils/email/microsoft.ts around lines 1207 to 1228, the catch blocks
currently log the error and return { value: [] }, which masks real
API/network/query failures as "no messages"; change these handlers to log the
error and then re-throw (or return a rejected promise) so callers can
distinguish failures from empty results (alternatively propagate a descriptive
Error with context such as "Error checking sent/received messages" and the
filter/search), ensuring monitoring/alerts can detect and surface these failures
rather than silently treating them as empty responses.

]);

// 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;
},
);

// If we have any outgoing emails, or any incoming emails (excluding current message)
const hasPreviousEmail =
response.value.length > 0 &&
response.value.some((message) => message.id !== options.messageId);
const messages = [
...validSentMessages,
...(receivedResponse.value || []),
];

return hasPreviousEmail;
return messages.some((message) => message.id !== options.messageId);
} catch (error) {
this.logger.error("Error checking previous communications", {
error,
Expand Down
2 changes: 1 addition & 1 deletion version.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v2.20.18
v2.20.19
Loading