Skip to content

fix: plain replies deletion and edits #3416

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
lorenzo132 merged 8 commits into from
Jan 16, 2026
+116 −101
Merged

fix: plain replies deletion and edits #3416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f33b06c
support to edit and delete plain reply messages
lorenzo132 Dec 13, 2025
20af225
fix linting
lorenzo132 Dec 13, 2025
e620a03
fix: not rely on mod_color as originally was made.
lorenzo132 Dec 13, 2025
4e38eaa
fix: typeerror / refactor
lorenzo132 Dec 13, 2025
420ece0
fix linting
lorenzo132 Dec 13, 2025
2992eb0
Merge branch 'development' into users/lorenzo132/plainmsg-edit-deletion
lorenzo132 Dec 17, 2025
2758afe
silent unneeded noise
lorenzo132 Dec 18, 2025
9e549a4
Merge branch 'development' into users/lorenzo132/plainmsg-edit-deletion
lorenzo132 Jan 16, 2026
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
2 changes: 2 additions & 0 deletions bot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1913,6 +1913,8 @@ async def on_message_delete(self, message):
"DM message not found.",
"Malformed thread message.",
"Thread message not found.",
"Linked DM message not found.",
"Thread message is an internal message, not a note.",
}:
logger.debug("Failed to find linked message to delete: %s", e)
embed = discord.Embed(description="Failed to delete message.", color=self.error_color)
Expand Down
6 changes: 3 additions & 3 deletions cogs/modmail.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1724,11 +1724,11 @@ async def edit(self, ctx, message_id: Optional[int] = None, *, message: str):

try:
await thread.edit_message(message_id, message)
except ValueError:
except ValueError as e:
return await ctx.send(
embed=discord.Embed(
title="Failed",
description="Cannot find a message to edit. Plain messages are not supported.",
description=str(e),
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Returning the raw exception message str(e) directly to users in error messages can expose internal implementation details or confusing technical messages. Consider mapping specific ValueError messages to more user-friendly error descriptions, or ensure that all ValueError messages raised in the thread module are written with end-users in mind.

Copilot uses AI. Check for mistakes.
color=self.bot.error_color,
)
)
Expand Down Expand Up @@ -2274,7 +2274,7 @@ async def delete(self, ctx, message_id: int = None):
return await ctx.send(
embed=discord.Embed(
title="Failed",
description="Cannot find a message to delete. Plain messages are not supported.",
description=str(e),
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Returning the raw exception message str(e) directly to users in error messages can expose internal implementation details or confusing technical messages. Consider mapping specific ValueError messages to more user-friendly error descriptions, or ensure that all ValueError messages raised in the thread module are written with end-users in mind.

Suggested change
description=str(e),
description=(
"I couldn't delete that message. It may not exist, may not have been "
"sent via Modmail, or cannot be deleted."
),

Copilot uses AI. Check for mistakes.
color=self.bot.error_color,
)
)
Expand Down
209 changes: 111 additions & 98 deletions core/thread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,117 +1334,118 @@ async def find_linked_messages(
message1: discord.Message = None,
note: bool = True,
) -> typing.Tuple[discord.Message, typing.List[typing.Optional[discord.Message]]]:
if message1 is not None:
if note:
# For notes, don't require author.url; rely on footer/author.name markers
if not message1.embeds or message1.author != self.bot.user:
logger.warning(
f"Malformed note for deletion: embeds={bool(message1.embeds)}, author={message1.author}"
)
raise ValueError("Malformed note message.")
if message1 is None:
if message_id is not None:
try:
message1 = await self.channel.fetch_message(message_id)
except discord.NotFound:
logger.warning(f"Message ID {message_id} not found in channel history.")
raise ValueError("Thread message not found.")
else:
if (
not message1.embeds
or not message1.embeds[0].author.url
or message1.author != self.bot.user
):
logger.debug(
f"Malformed thread message for deletion: embeds={bool(message1.embeds)}, author_url={getattr(message1.embeds[0], 'author', None) and message1.embeds[0].author.url}, author={message1.author}"
)
# Keep original error string to avoid extra failure embeds in on_message_delete
raise ValueError("Malformed thread message.")
# No ID provided - find last message sent by bot
async for msg in self.channel.history():
if msg.author != self.bot.user:
continue
if not msg.embeds:
continue

elif message_id is not None:
try:
message1 = await self.channel.fetch_message(message_id)
except discord.NotFound:
logger.warning(f"Message ID {message_id} not found in channel history.")
raise ValueError("Thread message not found.")
is_valid_candidate = False
if (
msg.embeds[0].footer
and msg.embeds[0].footer.text
and msg.embeds[0].footer.text.startswith("[PLAIN]")
):
is_valid_candidate = True
elif msg.embeds[0].author.url and msg.embeds[0].author.url.split("#")[-1].isdigit():
is_valid_candidate = True
Comment on lines +1359 to +1360
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

There's a missing check for whether msg.embeds[0].author exists before accessing msg.embeds[0].author.url. If the embed exists but doesn't have an author field, this will raise an AttributeError. The code should verify that msg.embeds[0].author is not None before attempting to access its url attribute.

Suggested change
elif msg.embeds[0].author.url and msg.embeds[0].author.url.split("#")[-1].isdigit():
is_valid_candidate = True
else:
author = msg.embeds[0].author
if author and author.url and author.url.split("#")[-1].isdigit():
is_valid_candidate = True

Copilot uses AI. Check for mistakes.

if is_valid_candidate:
message1 = msg
break

if note:
# Try to treat as note/persistent note first
if message1.embeds and message1.author == self.bot.user:
footer_text = (message1.embeds[0].footer and message1.embeds[0].footer.text) or ""
author_name = getattr(message1.embeds[0].author, "name", "") or ""
is_note = (
"internal note" in footer_text.lower()
or "persistent internal note" in footer_text.lower()
or author_name.startswith("📝 Note")
or author_name.startswith("📝 Persistent Note")
)
if is_note:
# Notes have no linked DM counterpart; keep None sentinel
return message1, None
# else: fall through to relay checks below

# Non-note path (regular relayed messages): require author.url and colors
if not (
message1.embeds
and message1.embeds[0].author.url
and message1.embeds[0].color
and message1.author == self.bot.user
):
logger.warning(
f"Message {message_id} is not a valid modmail relay message. embeds={bool(message1.embeds)}, author_url={getattr(message1.embeds[0], 'author', None) and message1.embeds[0].author.url}, color={getattr(message1.embeds[0], 'color', None)}, author={message1.author}"
)
raise ValueError("Thread message not found.")
if message1 is None:
raise ValueError("No editable thread message found.")

is_note = False
if message1.embeds and message1.author == self.bot.user:
footer_text = (message1.embeds[0].footer and message1.embeds[0].footer.text) or ""
author_name = getattr(message1.embeds[0].author, "name", "") or ""
is_note = (
"internal note" in footer_text.lower()
or "persistent internal note" in footer_text.lower()
or author_name.startswith("📝 Note")
or author_name.startswith("📝 Persistent Note")
)
Comment on lines +1372 to +1378
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The logic for checking if a message is a note uses both footer and author fields. However, if message1.embeds[0] doesn't have an author attribute, the getattr call on line 1372 will return an empty string, but this doesn't protect against AttributeError when checking author_name.startswith. While getattr provides a default, the issue is that if embeds[0].author is None, getattr returns the default correctly. However, consider adding an explicit check for embeds[0].author existence for clarity.

Copilot uses AI. Check for mistakes.

if message1.embeds[0].footer and "Internal Message" in message1.embeds[0].footer.text:
if not note:
logger.warning(
f"Message {message_id} is an internal message, but note deletion not requested."
)
raise ValueError("Thread message is an internal message, not a note.")
# Internal bot-only message treated similarly; keep None sentinel
return message1, None
if note and is_note:
return message1, None

if message1.embeds[0].color.value != self.bot.mod_color and not (
either_direction and message1.embeds[0].color.value == self.bot.recipient_color
):
logger.warning("Message color does not match mod/recipient colors.")
raise ValueError("Thread message not found.")
else:
async for message1 in self.channel.history():
if (
message1.embeds
and message1.embeds[0].author.url
and message1.embeds[0].color
and (
message1.embeds[0].color.value == self.bot.mod_color
or (either_direction and message1.embeds[0].color.value == self.bot.recipient_color)
)
and message1.embeds[0].author.url.split("#")[-1].isdigit()
and message1.author == self.bot.user
):
break
else:
if not note and is_note:
raise ValueError("Thread message is an internal message, not a note.")

if is_note:
return message1, None
Comment on lines +1381 to +1387
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The return type annotation indicates the method returns a Tuple containing a discord.Message and a List of Optional discord.Messages. However, when returning for notes (line 1381 and 1387), the method returns (message1, None) where None is not a List. This violates the type contract. The return should be (message1, []) or the type annotation should be updated to allow None as the second element.

Copilot uses AI. Check for mistakes.

Comment on lines +1386 to +1388
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

This statement is unreachable.

Suggested change
if is_note:
return message1, None

Copilot uses AI. Check for mistakes.
is_plain = False
if message1.embeds and message1.embeds[0].footer and message1.embeds[0].footer.text:
if message1.embeds[0].footer.text.startswith("[PLAIN]"):
is_plain = True

if not is_plain:
# Relaxed mod_color check: only ensure author is bot and has url (which implies it's a relay)
# We rely on author.url existing for Joint ID
if not (message1.embeds and message1.embeds[0].author.url and message1.author == self.bot.user):
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The check for msg.embeds[0].author.url should first verify that msg.embeds[0].author exists. If the embed has no author attribute or author is None, accessing msg.embeds[0].author.url will raise an AttributeError.

Copilot uses AI. Check for mistakes.
raise ValueError("Thread message not found.")

try:
joint_id = int(message1.embeds[0].author.url.split("#")[-1])
except ValueError:
raise ValueError("Malformed thread message.")
try:
joint_id = int(message1.embeds[0].author.url.split("#")[-1])
except (ValueError, AttributeError, IndexError):
raise ValueError("Malformed thread message.")
else:
joint_id = None
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Variable joint_id is not used.

Suggested change
joint_id = None

Copilot uses AI. Check for mistakes.
mod_tag = message1.embeds[0].footer.text.replace("[PLAIN]", "", 1).strip()
author_name = message1.embeds[0].author.name
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Similar to the issue in find_linked_messages, accessing embed1.author.name without first checking if embed1.author exists can cause an AttributeError. Verify that embed1.author is not None before accessing its name attribute.

Copilot uses AI. Check for mistakes.
desc = message1.embeds[0].description or ""
prefix = f"**{mod_tag} " if mod_tag else "**"
plain_content_expected = f"{prefix}{author_name}:** {desc}"
creation_time = message1.created_at

messages = [message1]
for user in self.recipients:
async for msg in user.history():
if either_direction:
if msg.id == joint_id:
return message1, msg

if not (msg.embeds and msg.embeds[0].author.url):
continue
try:
if int(msg.embeds[0].author.url.split("#")[-1]) == joint_id:
if is_plain:
for user in self.recipients:
async for msg in user.history(limit=50, around=creation_time):
if abs((msg.created_at - creation_time).total_seconds()) > 15:
Comment on lines +1416 to +1418
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The time window check of 15 seconds may be too restrictive for matching plain messages. If there's any network delay, message processing delay, or if the bot is under load, the DM message could be created slightly more than 15 seconds after the thread message, causing the match to fail. Consider increasing this tolerance to at least 30-60 seconds to account for potential delays.

Suggested change
for user in self.recipients:
async for msg in user.history(limit=50, around=creation_time):
if abs((msg.created_at - creation_time).total_seconds()) > 15:
tolerance_seconds = 60
for user in self.recipients:
async for msg in user.history(limit=50, around=creation_time):
if abs((msg.created_at - creation_time).total_seconds()) > tolerance_seconds:

Copilot uses AI. Check for mistakes.
continue
if msg.author != self.bot.user:
continue
if msg.embeds:
continue

if msg.content == plain_content_expected:
Comment on lines +1406 to +1425
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The code constructs the expected plain content to match against, but this logic is brittle. If the mod_tag contains special characters or if there are any formatting differences in how the message was originally constructed versus how it's being reconstructed here, the match will fail. Consider a more robust matching strategy, such as checking for key components separately or using fuzzy matching for the content.

Copilot uses AI. Check for mistakes.
messages.append(msg)
break
except ValueError:
continue
else:
for user in self.recipients:
async for msg in user.history():
if either_direction:
if msg.id == joint_id:
messages.append(msg)
break

if not (msg.embeds and msg.embeds[0].author.url):
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The code checks msg.embeds[0].author.url without first verifying that msg.embeds[0].author exists. If an embed doesn't have an author field, this will raise an AttributeError. Add a check to ensure msg.embeds[0].author is not None before accessing its url attribute.

Copilot uses AI. Check for mistakes.
continue
try:
if int(msg.embeds[0].author.url.split("#")[-1]) == joint_id:
messages.append(msg)
break
except (ValueError, IndexError, AttributeError):
continue

if len(messages) > 1:
return messages
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The return statement at line 1446 returns messages (a list), not a tuple as specified by the return type annotation. According to the type annotation, the method should return typing.Tuple[discord.Message, typing.List[typing.Optional[discord.Message]]], but here it's returning just the list. This should be return (messages[0], messages[1:]) to match the type signature and maintain consistency with how the result is unpacked in callers like line 1452.

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The error "Linked DM message not found." is raised when len(messages) <= 1, but this doesn't distinguish between the case where no linked messages were found versus when we're dealing with a note that doesn't have linked messages. For notes that return early with (message1, None), this is fine, but for regular messages, this error assumes that a linked message should always exist. Consider whether this is the intended behavior for all message types, particularly for plain messages where finding the linked DM might legitimately fail.

Suggested change
if is_plain:
# For plain messages, it's acceptable that no linked DM is found;
# in that case, just return the original message.
return messages

Copilot uses AI. Check for mistakes.
raise ValueError("DM message not found.")
raise ValueError("Linked DM message not found.")

async def edit_message(self, message_id: typing.Optional[int], message: str) -> None:
try:
Expand All @@ -1456,6 +1457,10 @@ async def edit_message(self, message_id: typing.Optional[int], message: str) ->
embed1 = message1.embeds[0]
embed1.description = message

is_plain = False
if embed1.footer and embed1.footer.text and embed1.footer.text.startswith("[PLAIN]"):
is_plain = True

tasks = [
self.bot.api.edit_message(message1.id, message),
message1.edit(embed=embed1),
Expand All @@ -1465,9 +1470,17 @@ async def edit_message(self, message_id: typing.Optional[int], message: str) ->
else:
for m2 in message2:
if m2 is not None:
embed2 = m2.embeds[0]
embed2.description = message
tasks += [m2.edit(embed=embed2)]
if is_plain:
# Reconstruct the plain message format to preserve matching capability
mod_tag = embed1.footer.text.replace("[PLAIN]", "", 1).strip()
author_name = embed1.author.name
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

When reconstructing the plain message content for editing, the code directly accesses embed1.author.name without checking if embed1.author exists. If the embed doesn't have an author field, this will raise an AttributeError. Add a check to ensure embed1.author is not None before accessing its name attribute.

Suggested change
author_name = embed1.author.name
author_name = embed1.author.name if embed1.author and embed1.author.name else ""

Copilot uses AI. Check for mistakes.
prefix = f"**{mod_tag} " if mod_tag else "**"
new_content = f"{prefix}{author_name}:** {message}"
tasks += [m2.edit(content=new_content)]
else:
embed2 = m2.embeds[0]
embed2.description = message
tasks += [m2.edit(embed=embed2)]

await asyncio.gather(*tasks)

Expand Down
Loading