Fix Discord feedback message selection

Author: zardus

tools/feedback/discord-feedback

@@ -41,6 +41,10 @@ PWN_COLLEGE_GITHUB_ORG = "pwncollege"
 TEXT_CHANNEL_TYPES = {0, 5}
 PUBLIC_THREAD_TYPES = {10, 11}
 THREAD_PARENT_TYPES = {0, 5, 15, 16}
+# Keep ordinary posts, replies, application-command responses, thread starters, and context
+# menu responses. Other Discord message types are generated system events rather than
+# learner-authored discussion.
+FEEDBACK_MESSAGE_TYPES = {0, 19, 20, 21, 23}
 MAX_DISCORD_PAGE_SIZE = 100
 MAX_DISCORD_SERVER_ERROR_RETRIES = 5
 # Image attachments are downloaded so the analysis agent can view screenshots that carry
@@ -640,11 +644,10 @@ def fetch_messages_since(
     api: DiscordAPI,
     channel: dict[str, Any],
     cutoff: datetime.datetime,
-    max_messages: int,
 ) -> list[dict[str, Any]]:
     messages: list[dict[str, Any]] = []
     params: dict[str, Any] = {"limit": MAX_DISCORD_PAGE_SIZE}
-    while len(messages) < max_messages:
+    while True:
         try:
             page = api.request(f"/channels/{channel['id']}/messages", params)
         except (DiscordPermissionError, DiscordServerError) as error:
@@ -667,13 +670,51 @@ def fetch_messages_since(
             oldest_time = (
                 message_time if oldest_time is None else min(oldest_time, message_time)
             )
-            if message_time >= cutoff:
+            if message_time >= cutoff and message_has_feedback_content(message):
                 messages.append(message)
 
         if oldest_id is None or oldest_time is None or oldest_time < cutoff:
-            return messages[:max_messages]
+            return messages
         params["before"] = str(oldest_id)
-    return messages[:max_messages]
+
+
+def message_has_feedback_content(message: dict[str, Any]) -> bool:
+    """Whether a Discord message carries content useful to the feedback analysis.
+
+    Discord system events such as member joins have an author and timestamp but no actual
+    message payload. Do not let those empty events crowd learner feedback out of the global
+    retention limit.
+    """
+    if message.get("type", 0) not in FEEDBACK_MESSAGE_TYPES:
+        return False
+    return bool(
+        (message.get("content") or "").strip()
+        or message.get("attachments")
+        or message.get("embeds")
+    )
+
+
+def retain_most_recent_messages(
+    messages_by_channel: list[tuple[dict[str, Any], list[dict[str, Any]]]],
+    max_messages: int,
+) -> list[tuple[dict[str, Any], list[dict[str, Any]]]]:
+    """Apply the message limit globally after every channel has been scanned."""
+    candidates = [
+        (channel, message)
+        for channel, messages in messages_by_channel
+        for message in messages
+    ]
+    candidates.sort(
+        key=lambda item: (parse_iso8601(item[1]["timestamp"]), int(item[1]["id"])),
+        reverse=True,
+    )
+
+    retained_by_channel: OrderedDict[
+        str, tuple[dict[str, Any], list[dict[str, Any]]]
+    ] = OrderedDict()
+    for channel, message in candidates[:max_messages]:
+        retained_by_channel.setdefault(channel["id"], (channel, []))[1].append(message)
+    return list(retained_by_channel.values())
 
 
 def sanitize_content(content: str, channels_by_id: dict[str, dict[str, Any]]) -> str:
@@ -4432,14 +4473,10 @@ def feedback_command(
         channels, channels_by_id = discover_channels(
             api, guild_id, selected_channel_ids, since, include_threads
         )
-        click.echo(f"Discovered {len(channels)} readable channel(s)/thread(s)")
+        click.echo(f"Discovered {len(channels)} channel(s)/thread(s) to scan")
         messages_by_channel: list[tuple[dict[str, Any], list[dict[str, Any]]]] = []
-        remaining = max_messages
         for channel in channels:
-            if remaining <= 0:
-                break
-            messages = fetch_messages_since(api, channel, since, remaining)
-            remaining -= len(messages)
+            messages = fetch_messages_since(api, channel, since)
             if messages:
                 messages_by_channel.append((channel, messages))
                 click.echo(
@@ -4453,6 +4490,17 @@ def feedback_command(
                     channel_display_name(channel, channels_by_id),
                 )
 
+        matching_message_count = sum(
+            len(channel_messages) for _channel, channel_messages in messages_by_channel
+        )
+        messages_by_channel = retain_most_recent_messages(
+            messages_by_channel, max_messages
+        )
+        click.echo(
+            f"Scanned all {len(channels)} channel(s)/thread(s); retaining the most "
+            f"recent {min(matching_message_count, max_messages)} of "
+            f"{matching_message_count} non-empty message(s)"
+        )
         messages = sanitize_messages(
             messages_by_channel,
             channels_by_id,
@@ -4513,6 +4561,8 @@ def feedback_command(
                     "since": since.isoformat(),
                     "until": until.isoformat(),
                     "messages": len(messages),
+                    "matching_messages": matching_message_count,
+                    "max_messages": max_messages,
                     "image_attachments": image_count,
                     "channels": len(channels),
                     "include_threads": include_threads,

tools/feedback/tests/test_discord_feedback.py

@@ -210,6 +210,147 @@ def test_completed_scrape_is_checkpointed_before_fetch_only_returns(self):
             self.assertTrue((artifact_dir / "run.json").is_file())
 
 
+class DiscordScrapeTests(unittest.TestCase):
+    @staticmethod
+    def message(message_id, timestamp, content="", **overrides):
+        message = {
+            "id": str(message_id),
+            "timestamp": timestamp,
+            "content": content,
+            "attachments": [],
+            "embeds": [],
+            "author": {"id": f"author-{message_id}"},
+        }
+        message.update(overrides)
+        return message
+
+    def test_fetch_scans_to_cutoff_and_discards_empty_system_events(self):
+        cutoff = datetime.datetime(2026, 7, 24, tzinfo=datetime.timezone.utc)
+        channel = {"id": "123", "name": "help"}
+        api = mock.Mock()
+        api.request.side_effect = [
+            [
+                self.message(
+                    5,
+                    "2026-07-24T00:05:00+00:00",
+                    type=7,
+                ),
+                self.message(4, "2026-07-24T00:04:00+00:00", "new feedback"),
+            ],
+            [
+                self.message(3, "2026-07-24T00:03:00+00:00", "more feedback"),
+                self.message(2, "2026-07-23T23:59:00+00:00", "before cutoff"),
+            ],
+        ]
+
+        messages = discord_feedback.fetch_messages_since(api, channel, cutoff)
+
+        self.assertEqual([message["id"] for message in messages], ["4", "3"])
+        self.assertEqual(api.request.call_count, 2)
+        self.assertEqual(api.request.call_args_list[1].args[1]["before"], "4")
+
+    def test_feedback_content_includes_text_attachments_and_embeds(self):
+        timestamp = "2026-07-24T00:00:00+00:00"
+
+        self.assertFalse(
+            discord_feedback.message_has_feedback_content(
+                self.message(1, timestamp, "  ", type=7)
+            )
+        )
+        self.assertFalse(
+            discord_feedback.message_has_feedback_content(
+                self.message(5, timestamp, "rendered system text", type=6)
+            )
+        )
+        self.assertTrue(
+            discord_feedback.message_has_feedback_content(
+                self.message(2, timestamp, "learner feedback")
+            )
+        )
+        self.assertTrue(
+            discord_feedback.message_has_feedback_content(
+                self.message(3, timestamp, attachments=[{"id": "attachment"}])
+            )
+        )
+        self.assertTrue(
+            discord_feedback.message_has_feedback_content(
+                self.message(4, timestamp, embeds=[{"title": "feedback"}])
+            )
+        )
+
+    def test_cli_scans_every_channel_then_keeps_global_newest_messages(self):
+        with tempfile.TemporaryDirectory() as temporary_directory:
+            repo = pathlib.Path(temporary_directory)
+            now = datetime.datetime(
+                2026, 7, 24, 1, 0, 0, tzinfo=datetime.timezone.utc
+            )
+            run_id = now.strftime("%Y%m%d-%H%M%S")
+            artifact_dir = repo / ".discord-feedback" / run_id
+            channels = [
+                {"id": "channel-a", "name": "a"},
+                {"id": "channel-b", "name": "b"},
+                {"id": "channel-c", "name": "c"},
+            ]
+            messages = {
+                "channel-a": [self.message(1, "2026-07-24T00:10:00+00:00", "oldest")],
+                "channel-b": [self.message(4, "2026-07-24T00:40:00+00:00", "newest")],
+                "channel-c": [
+                    self.message(2, "2026-07-24T00:20:00+00:00", "older"),
+                    self.message(3, "2026-07-24T00:30:00+00:00", "newer"),
+                ],
+            }
+
+            with (
+                mock.patch.object(discord_feedback, "git_root", return_value=repo),
+                mock.patch.object(discord_feedback, "utc_now", return_value=now),
+                mock.patch.object(
+                    discord_feedback,
+                    "resolve_scrape_since",
+                    return_value=now - datetime.timedelta(hours=1),
+                ),
+                mock.patch.object(
+                    discord_feedback,
+                    "discover_channels",
+                    return_value=(channels, {item["id"]: item for item in channels}),
+                ),
+                mock.patch.object(
+                    discord_feedback,
+                    "fetch_messages_since",
+                    side_effect=lambda _api, channel, _since: messages[channel["id"]],
+                ) as fetch_messages_since,
+                mock.patch.dict(os.environ, {"DISCORD_BOT_TOKEN": "token"}),
+            ):
+                result = CliRunner().invoke(
+                    discord_feedback.feedback_command,
+                    [
+                        "--fetch-only",
+                        "--no-pr-feedback",
+                        "--max-messages",
+                        "2",
+                    ],
+                )
+
+            self.assertEqual(result.exit_code, 0, result.output)
+            self.assertEqual(fetch_messages_since.call_count, len(channels))
+            self.assertEqual(
+                [call.args[1]["id"] for call in fetch_messages_since.call_args_list],
+                ["channel-a", "channel-b", "channel-c"],
+            )
+            retained = [
+                json.loads(line)
+                for line in (artifact_dir / "messages.jsonl").read_text().splitlines()
+            ]
+            self.assertEqual([message["id"] for message in retained], ["3", "4"])
+            run = json.loads((artifact_dir / "run.json").read_text())
+            self.assertEqual(run["matching_messages"], 4)
+            self.assertEqual(run["messages"], 2)
+            self.assertEqual(run["max_messages"], 2)
+            self.assertIn(
+                "Scanned all 3 channel(s)/thread(s); retaining the most recent 2 of 4",
+                result.output,
+            )
+
+
 class ValidationTests(unittest.TestCase):
     def test_primary_and_serial_validation_have_inner_and_outer_timeouts(self):
         with tempfile.TemporaryDirectory() as temporary_directory: