fix: thread_auto_close bug.

[lorenzo132]

This resolves an issue introduced in: #3423
Autocloses would fail after a while.

[Copilot]

Pull request overview

This PR fixes a bug in the thread auto-close functionality that was introduced in PR #3423. The fix addresses an issue where auto-closes would fail after a period of time by improving how closure cancellations are tracked and handled.

Key Changes:

• Enhanced cancel_closure method with granular control over which closures to cancel and whether to mark auto-close as explicitly cancelled
• Fixed thread auto-close logic to properly distinguish between user-initiated cancellations and automatic resets when users reply
• Added safeguards for handling None messages in menu interactions and send operations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
core/thread.py	Implements the core auto-close bug fix with improved closure cancellation logic, adds ready event signaling when threads are cancelled, and includes None message handling for menu interactions
core/models.py	Updates DummyMessage to properly handle None wrapped messages by raising AttributeError
core/clients.py	Adds fallback handling for None messages in append_log to prevent crashes

Comments suppressed due to low confidence (1)

core/thread.py:1086

• The comment on line 1841 states "Only restart auto-close if it wasn't explicitly cancelled", but the logic on line 1061 sets mark_auto_close_cancelled to True when auto_close is False (i.e., when doing a manual close). This means a manual close will mark auto-close as cancelled, which seems correct. However, on line 1086, auto_close_cancelled is reset to False when doing a manual close. This creates inconsistent behavior where mark_auto_close_cancelled is set one way in cancel_closure but then immediately overridden. Consider reviewing this logic for consistency.

        await self.cancel_closure(
            auto_close,
            mark_auto_close_cancelled=not auto_close,
        )

        if after > 0:
            # TODO: Add somewhere to clean up broken closures
            #  (when channel is already deleted)
            now = discord.utils.utcnow()
            items = {
                # 'initiation_time': now.isoformat(),
                "time": (now + timedelta(seconds=after)).isoformat(),
                "closer_id": closer.id,
                "silent": silent,
                "delete_channel": delete_channel,
                "message": message,
                "auto_close": auto_close,
            }
            self.bot.config["closures"][str(self.id)] = items
            await self.bot.config.update()

            task = asyncio.create_task(self._close_after(after, closer, silent, delete_channel, message))

            if auto_close:
                self.auto_close_task = task
            else:
                self.close_task = task
                self.auto_close_cancelled = False  # Reset flag when manually closing

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The unittest.mock module is imported inside a nested callback function. This import should be moved to the top of the file with other imports for better code organization and to avoid repeated imports on every execution of this code path.

[Copilot]

Using MagicMock in production code is not recommended. MagicMock is intended for testing purposes only. Consider creating a proper minimal message class or data structure instead of using a mock object for command invocation context.

Suggested change

	from unittest.mock import MagicMock
	# Create a mock message strictly for command invocation context
	mock_msg = MagicMock(spec=discord.Message)
	mock_msg.id = 0
	mock_msg.channel = ch
	mock_msg.guild = self.outer_thread.bot.modmail_guild
	mock_msg.content = self.outer_thread.bot.prefix + al
	mock_msg.author = self.outer_thread.bot.user
	synthetic = DummyMessage(mock_msg)
	# Create a minimal message-like object strictly for command invocation context
	ns_msg = SimpleNamespace(
	id=0,
	channel=ch,
	guild=self.outer_thread.bot.modmail_guild,
	content=self.outer_thread.bot.prefix + al,
	author=self.outer_thread.bot.user,
	)
	synthetic = DummyMessage(ns_msg)

[Copilot]

The fallback data when message is None creates a system log entry with hardcoded values. However, this may not be useful in practice since the content is always "System Message (No Content)". Consider whether this fallback should be logging actual useful information or if calling append_log with None should be prevented at the call site instead.

Suggested change

	"content": "System Message (No Content)",
	"content": (
	f"System Message (No Content) "
	f"[channel_id={channel_id}, message_id={message_id or 'N/A'}, type={type_}]"
	),

[Copilot]

The None check returns early only when all of note, from_mod, and thread_creation are False. However, if note is True and message is None, the code proceeds to lines 1809-1838 which will fail when trying to access message.content, message.author.name, message.author.display_avatar.url, and message.created_at. This will cause an AttributeError crash. The guard should either explicitly check that message is not None when note is True, or the note block should have its own None handling.