fix: prevent Discord embed field 400s from empty/oversized values
C1: build_ping_embed set sender_id field value="" whenever sender_id was None, which manual_share.py always passes -- Discord rejects empty embed field values (min length 1), so every manual-share ping failed to send. Fixed by using a zero-width space sentinel that round-trips back to "" (raw_content) or None (sender_id) in parse_ping_embed. Added regression tests asserting no field value is ever empty after build_ping_embed runs. C2: raw_content over 1024 chars (Discord's embed field cap) caused inbox.send() to raise before the UID was marked seen, so oversized emails were retried forever and never ingested. Added a defensive hard truncation in build_ping_embed itself, plus an explicit truncation in email_ingest.py before the embed is built (lossy for very long emails, acceptable for MVP).
This commit is contained in:
parent
4d96b74453
commit
75ee9f8117
3 changed files with 121 additions and 14 deletions
|
|
@ -1,7 +1,6 @@
|
||||||
import json
|
import json
|
||||||
import os
|
import os
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
import discord
|
|
||||||
from discord.ext import commands, tasks
|
from discord.ext import commands, tasks
|
||||||
from imaplib import IMAP4_SSL
|
from imaplib import IMAP4_SSL
|
||||||
from email import message_from_bytes
|
from email import message_from_bytes
|
||||||
|
|
@ -10,6 +9,13 @@ from chorus_bot.ping import build_ping_embed
|
||||||
|
|
||||||
SEEN_UIDS_PATH = Path(os.environ.get("CHORUS_SEEN_UIDS_PATH", "seen_uids.json"))
|
SEEN_UIDS_PATH = Path(os.environ.get("CHORUS_SEEN_UIDS_PATH", "seen_uids.json"))
|
||||||
|
|
||||||
|
# Discord embed field values cap at 1024 chars. Truncate the raw_content we pass
|
||||||
|
# into the embed with headroom below that cap for build_ping_embed's own
|
||||||
|
# truncation marker (see ping.py). This is intentionally lossy for very long
|
||||||
|
# emails -- acceptable for MVP; the full body still lives in the source mailbox.
|
||||||
|
_RAW_CONTENT_MAX = 1000
|
||||||
|
_TRUNCATION_MARKER = "\n\n[...truncated for Discord embed limit...]"
|
||||||
|
|
||||||
# modality -> (imap host attr, imap user attr, imap password attr) on Config
|
# modality -> (imap host attr, imap user attr, imap password attr) on Config
|
||||||
MAILBOXES = {
|
MAILBOXES = {
|
||||||
"bh_email": ("bh_email_imap_host", "bh_email_imap_user", "bh_email_imap_password"),
|
"bh_email": ("bh_email_imap_host", "bh_email_imap_user", "bh_email_imap_password"),
|
||||||
|
|
@ -57,22 +63,28 @@ class EmailIngestCog(commands.Cog):
|
||||||
try:
|
try:
|
||||||
conn.login(user, password)
|
conn.login(user, password)
|
||||||
conn.select("INBOX")
|
conn.select("INBOX")
|
||||||
_, data = conn.search(None, "UNSEEN")
|
# Use IMAP UIDs (stable across mailbox changes/expunges), not sequence
|
||||||
|
# numbers, which shift and would make persisted seen_uids.json entries
|
||||||
|
# refer to the wrong message later on.
|
||||||
|
_, data = conn.uid("SEARCH", None, "UNSEEN")
|
||||||
for uid in data[0].split():
|
for uid in data[0].split():
|
||||||
uid_str = uid.decode()
|
uid_str = uid.decode()
|
||||||
if uid_str in seen:
|
if uid_str in seen:
|
||||||
continue
|
continue
|
||||||
_, msg_data = conn.fetch(uid, "(RFC822)")
|
# BODY.PEEK[] fetches without setting the \Seen flag server-side,
|
||||||
|
# so we don't silently mark the user's real emails as read.
|
||||||
|
_, msg_data = conn.uid("FETCH", uid, "(BODY.PEEK[])")
|
||||||
raw_email = msg_data[0][1]
|
raw_email = msg_data[0][1]
|
||||||
msg = message_from_bytes(raw_email)
|
msg = message_from_bytes(raw_email)
|
||||||
sender = msg.get("From")
|
sender = msg.get("From")
|
||||||
captured_at = parsedate_to_datetime(msg.get("Date"))
|
captured_at = parsedate_to_datetime(msg.get("Date"))
|
||||||
body = self._extract_body(msg)
|
body = self._extract_body(msg)
|
||||||
snippet = body if len(body) <= 120 else body[:117] + "..."
|
snippet = body if len(body) <= 120 else body[:117] + "..."
|
||||||
|
raw_content = self._truncate_for_embed(body)
|
||||||
|
|
||||||
embed = build_ping_embed(
|
embed = build_ping_embed(
|
||||||
modality=modality, sender_id=sender, snippet=snippet,
|
modality=modality, sender_id=sender, snippet=snippet,
|
||||||
raw_content=body, captured_at=captured_at,
|
raw_content=raw_content, captured_at=captured_at,
|
||||||
)
|
)
|
||||||
inbox = self.bot.get_channel(self.config.inbox_channel_id)
|
inbox = self.bot.get_channel(self.config.inbox_channel_id)
|
||||||
role_mention = f"<@&{self.config.triage_role_id}>"
|
role_mention = f"<@&{self.config.triage_role_id}>"
|
||||||
|
|
@ -83,6 +95,15 @@ class EmailIngestCog(commands.Cog):
|
||||||
finally:
|
finally:
|
||||||
conn.logout()
|
conn.logout()
|
||||||
|
|
||||||
|
@staticmethod
|
||||||
|
def _truncate_for_embed(body: str) -> str:
|
||||||
|
"""Cap the raw_content passed into the embed at a safe length below
|
||||||
|
Discord's 1024-char field limit. Lossy for very long emails but keeps
|
||||||
|
ingestion from failing (and retrying forever) on oversized bodies."""
|
||||||
|
if len(body) <= _RAW_CONTENT_MAX:
|
||||||
|
return body
|
||||||
|
return body[:_RAW_CONTENT_MAX] + _TRUNCATION_MARKER
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _extract_body(msg) -> str:
|
def _extract_body(msg) -> str:
|
||||||
if msg.is_multipart():
|
if msg.is_multipart():
|
||||||
|
|
@ -90,15 +111,22 @@ class EmailIngestCog(commands.Cog):
|
||||||
html_part = None
|
html_part = None
|
||||||
for part in msg.walk():
|
for part in msg.walk():
|
||||||
if part.get_content_type() == "text/plain" and plain_part is None:
|
if part.get_content_type() == "text/plain" and plain_part is None:
|
||||||
plain_part = part.get_payload(decode=True).decode(errors="replace")
|
payload = part.get_payload(decode=True)
|
||||||
|
if payload is not None:
|
||||||
|
plain_part = payload.decode(errors="replace")
|
||||||
elif part.get_content_type() == "text/html" and html_part is None:
|
elif part.get_content_type() == "text/html" and html_part is None:
|
||||||
html_part = part.get_payload(decode=True).decode(errors="replace")
|
payload = part.get_payload(decode=True)
|
||||||
|
if payload is not None:
|
||||||
|
html_part = payload.decode(errors="replace")
|
||||||
if plain_part is not None:
|
if plain_part is not None:
|
||||||
return plain_part
|
return plain_part
|
||||||
if html_part is not None:
|
if html_part is not None:
|
||||||
return html_part
|
return html_part
|
||||||
return "(no text content found)"
|
return "(no text content found)"
|
||||||
return msg.get_payload(decode=True).decode(errors="replace")
|
payload = msg.get_payload(decode=True)
|
||||||
|
if payload is None:
|
||||||
|
return "(no text content found)"
|
||||||
|
return payload.decode(errors="replace")
|
||||||
|
|
||||||
|
|
||||||
async def setup(bot: commands.Bot, config):
|
async def setup(bot: commands.Bot, config):
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,26 @@ MODALITY_LABELS = {
|
||||||
"nd_messenger": "ND Messenger",
|
"nd_messenger": "ND Messenger",
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# Discord's API rejects embed field values of length 0 (min 1, max 1024 chars).
|
||||||
|
# Use a zero-width space as a round-trip-safe sentinel for "empty" values so we
|
||||||
|
# never send a literal "" to Discord.
|
||||||
|
_EMPTY_FIELD_SENTINEL = ""
|
||||||
|
|
||||||
|
# Hard cap for any field value, with headroom for a truncation marker so the
|
||||||
|
# final value never exceeds Discord's 1024-char limit regardless of caller.
|
||||||
|
_FIELD_VALUE_MAX = 1024
|
||||||
|
_TRUNCATION_MARKER = "\n\n[...truncated for Discord embed limit...]"
|
||||||
|
|
||||||
|
|
||||||
|
def _safe_field_value(value: str) -> str:
|
||||||
|
"""Return a Discord-safe embed field value: never empty, never over 1024 chars."""
|
||||||
|
if not value:
|
||||||
|
return _EMPTY_FIELD_SENTINEL
|
||||||
|
if len(value) > _FIELD_VALUE_MAX:
|
||||||
|
keep = _FIELD_VALUE_MAX - len(_TRUNCATION_MARKER)
|
||||||
|
return value[:keep] + _TRUNCATION_MARKER
|
||||||
|
return value
|
||||||
|
|
||||||
|
|
||||||
def build_ping_embed(
|
def build_ping_embed(
|
||||||
*, modality: str, sender_id: str | None, snippet: str, raw_content: str,
|
*, modality: str, sender_id: str | None, snippet: str, raw_content: str,
|
||||||
|
|
@ -18,21 +38,35 @@ def build_ping_embed(
|
||||||
) -> discord.Embed:
|
) -> discord.Embed:
|
||||||
label = MODALITY_LABELS.get(modality, modality)
|
label = MODALITY_LABELS.get(modality, modality)
|
||||||
embed = discord.Embed(
|
embed = discord.Embed(
|
||||||
title=f"{label}" + (f" — {sender_id}" if sender_id else ""),
|
title=f"{label}" + (f" - {sender_id}" if sender_id else ""),
|
||||||
description=snippet,
|
description=snippet,
|
||||||
)
|
)
|
||||||
embed.add_field(name="modality", value=modality, inline=True)
|
embed.add_field(name="modality", value=_safe_field_value(modality), inline=True)
|
||||||
embed.add_field(name="sender_id", value=sender_id or "", inline=True)
|
embed.add_field(name="sender_id", value=_safe_field_value(sender_id or ""), inline=True)
|
||||||
embed.add_field(name="captured_at", value=captured_at.isoformat(), inline=True)
|
embed.add_field(
|
||||||
embed.add_field(name="raw_content", value=raw_content, inline=False)
|
name="captured_at", value=_safe_field_value(captured_at.isoformat()), inline=True,
|
||||||
|
)
|
||||||
|
embed.add_field(
|
||||||
|
name="raw_content", value=_safe_field_value(raw_content), inline=False,
|
||||||
|
)
|
||||||
return embed
|
return embed
|
||||||
|
|
||||||
|
|
||||||
def parse_ping_embed(embed: discord.Embed) -> dict:
|
def parse_ping_embed(embed: discord.Embed) -> dict:
|
||||||
values = {field.name: field.value for field in embed.fields}
|
values = {field.name: field.value for field in embed.fields}
|
||||||
|
sender_id = values.get("sender_id")
|
||||||
|
if sender_id == _EMPTY_FIELD_SENTINEL:
|
||||||
|
sender_id = None
|
||||||
|
else:
|
||||||
|
sender_id = sender_id or None
|
||||||
|
|
||||||
|
raw_content = values["raw_content"]
|
||||||
|
if raw_content == _EMPTY_FIELD_SENTINEL:
|
||||||
|
raw_content = ""
|
||||||
|
|
||||||
return {
|
return {
|
||||||
"modality": values["modality"],
|
"modality": values["modality"],
|
||||||
"sender_id": values.get("sender_id") or None,
|
"sender_id": sender_id,
|
||||||
"raw_content": values["raw_content"],
|
"raw_content": raw_content,
|
||||||
"captured_at": datetime.fromisoformat(values["captured_at"]),
|
"captured_at": datetime.fromisoformat(values["captured_at"]),
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -45,3 +45,48 @@ def test_build_and_parse_empty_raw_content():
|
||||||
)
|
)
|
||||||
parsed = ping.parse_ping_embed(embed)
|
parsed = ping.parse_ping_embed(embed)
|
||||||
assert parsed["raw_content"] == "", "Empty raw_content should round-trip as empty string, not '(empty)'"
|
assert parsed["raw_content"] == "", "Empty raw_content should round-trip as empty string, not '(empty)'"
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_ping_embed_never_produces_empty_field_value_for_empty_raw_content():
|
||||||
|
"""Regression guard for C1: Discord's API rejects embed field values with
|
||||||
|
length 0 (HTTP 400). Every field value must have len() >= 1 after
|
||||||
|
build_ping_embed runs, even when raw_content is ''."""
|
||||||
|
embed = ping.build_ping_embed(
|
||||||
|
modality="bh_email",
|
||||||
|
sender_id="test@example.com",
|
||||||
|
snippet="Empty message",
|
||||||
|
raw_content="",
|
||||||
|
captured_at=datetime(2026, 7, 13, 9, 0, 0, tzinfo=timezone.utc),
|
||||||
|
)
|
||||||
|
for field in embed.fields:
|
||||||
|
assert len(field.value) >= 1, f"field {field.name!r} has an empty value"
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_ping_embed_never_produces_empty_field_value_for_sender_id_none():
|
||||||
|
"""Regression guard for C1: manual_share.py always calls build_ping_embed
|
||||||
|
with sender_id=None. Every field value must still have len() >= 1."""
|
||||||
|
embed = ping.build_ping_embed(
|
||||||
|
modality="voice",
|
||||||
|
sender_id=None,
|
||||||
|
snippet="call about donation",
|
||||||
|
raw_content="call about donation",
|
||||||
|
captured_at=datetime(2026, 7, 13, 8, 0, 0, tzinfo=timezone.utc),
|
||||||
|
)
|
||||||
|
for field in embed.fields:
|
||||||
|
assert len(field.value) >= 1, f"field {field.name!r} has an empty value"
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_ping_embed_truncates_long_raw_content():
|
||||||
|
"""Regression guard for C2: embed field values over 1024 chars are
|
||||||
|
rejected by Discord. build_ping_embed must truncate defensively."""
|
||||||
|
long_body = "x" * 2000
|
||||||
|
embed = ping.build_ping_embed(
|
||||||
|
modality="bh_email",
|
||||||
|
sender_id="jane@example.com",
|
||||||
|
snippet="long email",
|
||||||
|
raw_content=long_body,
|
||||||
|
captured_at=datetime(2026, 7, 13, 9, 0, 0, tzinfo=timezone.utc),
|
||||||
|
)
|
||||||
|
raw_content_field = next(f for f in embed.fields if f.name == "raw_content")
|
||||||
|
assert len(raw_content_field.value) <= 1024
|
||||||
|
assert "truncated" in raw_content_field.value
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue