-
-
Notifications
You must be signed in to change notification settings - Fork 926
enh: cors middleware & html fallback #537
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.
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
Changes from all commits
92840ac
af8c1c4
b93cc1c
d659f90
9feeccc
d067645
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,19 @@ | |
| GMAIL_REQUEST_DELAY = 0.1 | ||
| HTML_BODY_TRUNCATE_LIMIT = 20000 | ||
| GMAIL_METADATA_HEADERS = ["Subject", "From", "To", "Cc", "Message-ID", "Date"] | ||
| LOW_VALUE_TEXT_PLACEHOLDERS = ( | ||
| "your client does not support html", | ||
| "view this email in your browser", | ||
| "open this email in your browser", | ||
| ) | ||
| LOW_VALUE_TEXT_FOOTER_MARKERS = ( | ||
| "mailing list", | ||
| "mailman/listinfo", | ||
| "unsubscribe", | ||
| "list-unsubscribe", | ||
| "manage preferences", | ||
| ) | ||
| LOW_VALUE_TEXT_HTML_DIFF_MIN = 80 | ||
|
|
||
|
|
||
| class _HTMLTextExtractor(HTMLParser): | ||
|
|
@@ -154,16 +167,29 @@ def _format_body_content(text_body: str, html_body: str) -> str: | |
| """ | ||
| text_stripped = text_body.strip() | ||
| html_stripped = html_body.strip() | ||
| html_text = _html_to_text(html_stripped).strip() if html_stripped else "" | ||
|
|
||
| plain_lower = " ".join(text_stripped.split()).lower() | ||
| html_lower = " ".join(html_text.split()).lower() | ||
| plain_is_low_value = plain_lower and ( | ||
| any(marker in plain_lower for marker in LOW_VALUE_TEXT_PLACEHOLDERS) | ||
| or ( | ||
| any(marker in plain_lower for marker in LOW_VALUE_TEXT_FOOTER_MARKERS) | ||
| and len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN | ||
| ) | ||
| or ( | ||
| len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN | ||
| and html_lower.endswith(plain_lower) | ||
| ) | ||
| ) | ||
|
|
||
| # Detect useless fallback: HTML comments in text, or HTML is 50x+ longer | ||
| use_html = html_stripped and ( | ||
| not text_stripped | ||
| or "<!--" in text_stripped | ||
| or len(html_stripped) > len(text_stripped) * 50 | ||
| # Prefer plain text, but fall back to HTML when plain text is empty or clearly low-value. | ||
| use_html = html_text and ( | ||
| not text_stripped or "<!--" in text_stripped or plain_is_low_value | ||
| ) | ||
|
Comment on lines
+174
to
189
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an explicit “minimum meaningful plain-text length” fallback condition. Current logic only flags low-value plain text via marker/footer/suffix heuristics. Very short non-empty text/plain (e.g., 1–20 chars) can still win over rich HTML content, which misses the “below minimal meaningful length” requirement. 💡 Proposed adjustment LOW_VALUE_TEXT_HTML_DIFF_MIN = 80
+LOW_VALUE_TEXT_MIN_MEANINGFUL_LEN = 40
@@
- plain_is_low_value = plain_lower and (
- any(marker in plain_lower for marker in LOW_VALUE_TEXT_PLACEHOLDERS)
+ plain_is_low_value = bool(plain_lower) and (
+ (
+ len(plain_lower) < LOW_VALUE_TEXT_MIN_MEANINGFUL_LEN
+ and len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN
+ )
+ or any(marker in plain_lower for marker in LOW_VALUE_TEXT_PLACEHOLDERS)
or (
any(marker in plain_lower for marker in LOW_VALUE_TEXT_FOOTER_MARKERS)
and len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN
)
or (
len(html_lower) >= len(plain_lower) + LOW_VALUE_TEXT_HTML_DIFF_MIN
and html_lower.endswith(plain_lower)
)
)🤖 Prompt for AI Agents |
||
|
|
||
| if use_html: | ||
| content = _html_to_text(html_stripped) | ||
| content = html_text | ||
| if len(content) > HTML_BODY_TRUNCATE_LIMIT: | ||
| content = content[:HTML_BODY_TRUNCATE_LIMIT] + "\n\n[Content truncated...]" | ||
| return content | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| #!/usr/bin/env python3 | ||
| """Quick test to verify CORS middleware is configured correctly.""" | ||
|
|
||
| import sys | ||
|
|
||
| sys.path.insert(0, ".") | ||
|
|
||
| # Set required env vars before importing | ||
| import os | ||
|
|
||
| os.environ["MCP_ENABLE_OAUTH21"] = "true" | ||
| os.environ["WORKSPACE_MCP_STATELESS_MODE"] = "true" | ||
| os.environ["GOOGLE_OAUTH_CLIENT_ID"] = "test" | ||
| os.environ["GOOGLE_OAUTH_CLIENT_SECRET"] = "test" | ||
|
|
||
| from core.server import server | ||
|
|
||
| # Get the HTTP app | ||
| app = server.http_app() | ||
|
|
||
| # Check middleware stack | ||
| print("Middleware stack:") | ||
| for i, m in enumerate(app.user_middleware): | ||
| if hasattr(m, "cls"): | ||
| print(f" {i}: {m.cls.__name__}") | ||
| else: | ||
| print(f" {i}: {type(m).__name__}") | ||
|
|
||
| print("\nChecking for CORSMiddleware...") | ||
| has_cors = any( | ||
| hasattr(m, "cls") and m.cls.__name__ == "CORSMiddleware" | ||
| for m in app.user_middleware | ||
| ) | ||
| print(f"CORS middleware found: {has_cors}") | ||
|
|
||
| if has_cors: | ||
| print("✅ CORS middleware is configured!") | ||
| else: | ||
| print("❌ CORS middleware is NOT configured!") | ||
| sys.exit(1) | ||
|
Comment on lines
+6
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid import-time execution and This file runs assertions at import time and exits the process on failure (Line 40), which can break/abort test collection and leak env mutations into unrelated tests. Please convert it to a real test function with scoped env setup and plain assertions. ✅ Suggested rewrite-#!/usr/bin/env python3
-"""Quick test to verify CORS middleware is configured correctly."""
-
-import sys
-
-sys.path.insert(0, ".")
-
-# Set required env vars before importing
-import os
-
-os.environ["MCP_ENABLE_OAUTH21"] = "true"
-os.environ["WORKSPACE_MCP_STATELESS_MODE"] = "true"
-os.environ["GOOGLE_OAUTH_CLIENT_ID"] = "test"
-os.environ["GOOGLE_OAUTH_CLIENT_SECRET"] = "test"
-
-from core.server import server
-
-# Get the HTTP app
-app = server.http_app()
-
-# Check middleware stack
-print("Middleware stack:")
-for i, m in enumerate(app.user_middleware):
- if hasattr(m, "cls"):
- print(f" {i}: {m.cls.__name__}")
- else:
- print(f" {i}: {type(m).__name__}")
-
-print("\nChecking for CORSMiddleware...")
-has_cors = any(
- hasattr(m, "cls") and m.cls.__name__ == "CORSMiddleware"
- for m in app.user_middleware
-)
-print(f"CORS middleware found: {has_cors}")
-
-if has_cors:
- print("✅ CORS middleware is configured!")
-else:
- print("❌ CORS middleware is NOT configured!")
- sys.exit(1)
+from starlette.middleware.cors import CORSMiddleware
+
+
+def test_http_app_has_cors_middleware(monkeypatch):
+ monkeypatch.setenv("MCP_ENABLE_OAUTH21", "true")
+ monkeypatch.setenv("WORKSPACE_MCP_STATELESS_MODE", "true")
+ monkeypatch.setenv("GOOGLE_OAUTH_CLIENT_ID", "test")
+ monkeypatch.setenv("GOOGLE_OAUTH_CLIENT_SECRET", "test")
+
+ from core.server import server
+
+ app = server.http_app()
+ assert any(
+ getattr(m, "cls", None) is CORSMiddleware for m in app.user_middleware
+ )🤖 Prompt for AI Agents |
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.