Skip to content

feat(discord-bridge): treat owner DM edits within 5min as new tasks - #824

Merged
qingyun-wu merged 2 commits into
sonichi:mainfrom
jayjaybeeuk:feat/discord-edit-as-followup-task
May 18, 2026
Merged

qingyun-wu merged 2 commits into
sonichi:mainfrom
jayjaybeeuk:feat/discord-edit-as-followup-task

Conversation

@jayjaybeeuk

Copy link
Copy Markdown
Contributor

Summary

  • Extends on_message_edit with Case 2 (issue Discord-edit-as-follow-up-task: treat MESSAGE_UPDATE on prior requests as a new task #795): when the owner edits their own DM within 5 minutes, the edited message is reprocessed via _handle_discord_message(after, force=True) so corrections ("actually do X instead") reach the bridge as a replacement task.
  • Restricted to DMs only — channel edits fire on embed unfurls / link previews and don't represent user intent.
  • Skips events where after.content == before.content (attachment updates, embed unfurls with no text delta).
  • Owner verified via existing load_allowed() before reprocessing.
  • Case 1 (edit adds a bot @-mention) is preserved unchanged; now has an explicit return so it doesn't fall through into Case 2 guards.

Closes #795.

Test plan

  • Owner edits a DM within 5 min → bridge logs [edit] owner edited DM … — reprocessing as new task and a new task file is written
  • Owner edits a DM after >5 min → no reprocessing (age_sec > 300 guard)
  • Non-owner edits a DM → no reprocessing (load_allowed() guard)
  • Owner edits a channel message → no reprocessing (isinstance(DMChannel) guard)
  • Owner edits DM with no text change (e.g. embed unfurl) → no reprocessing (content == before.content guard)
  • Edit that adds a bot @-mention → Case 1 still fires correctly; no double-processing

🤖 Generated with Claude Code

Extends on_message_edit with a second case (issue sonichi#795): when the owner
edits their own DM within 5 minutes, reprocess it via _handle_discord_message
with force=True so corrections ("actually do X instead") reach the bridge as a
replacement task.

Restricted to DMs only — channel edits fire on embed unfurls / link previews
which don't represent user intent. Skips no-text-change events for the same
reason. Owner-check via load_allowed() ensures non-owner edits are not
reprocessed.
@CLAassistant

CLAassistant commented May 18, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Cold review — LGTM with one real edge case to flag.

Verified

  • Case 1 (mention added) preserved with return so Case 2 doesn't double-fire on a mention edit
  • Case 2 guards are good: DMChannel only (avoids channel embed-unfurl noise), content must change, sender must be allowed, age ≤ 300s
  • force=True reuses the existing dispatch path — no parallel code surface

1 real edge case

`load_allowed()` can return `None` after PR #808's tri-state work (file missing → None vs file empty → set()). The check on the new line:

```python
if sender_id not in load_allowed():
```

If access.json is missing (TOFU-eligible state), `load_allowed()` returns `None` and `sender_id not in None` raises `TypeError: argument of type 'NoneType' is not iterable`. Bridge would crash on the first edit-in-a-fresh-install scenario.

Fix:
```python
allowed = load_allowed()
if not allowed or sender_id not in allowed:
return
```

This also has a nice side-effect of treating "file empty (admin lockdown)" same as "file missing" — both → no edit reprocess, which is the right policy.

Non-blocking nits

  • The 300s threshold is hardcoded; not a config. Fine for now.
  • If owner edits the same DM 3 times in 4 minutes, fires the handler 3 times → 3 task dispatches. Existing dedup machinery should catch duplicates downstream, but worth a one-line "may dispatch multiple times for rapid-fire edits — downstream dedups" comment.

Ship after the None-guard fix.

@qingyun-wu
qingyun-wu merged commit 5395312 into sonichi:main May 18, 2026
1 check passed

@vidhuUC vidhuUC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One bug: wrong timestamp for the 5-min window

age_sec = time.time() - after.created_at.timestamp()

This uses message creation time, not edit time. A message created 10+ minutes ago and then edited will be silently ignored even though the edit just happened.

Fix:

edit_ts = after.edited_at or after.created_at
age_sec = time.time() - edit_ts.timestamp()

after.edited_at is populated by discord.py on on_message_edit events (it's the edited_timestamp field from the Discord API). The or after.created_at fallback handles the rare case where edited_at is None (e.g. embed-only edits that slip through).

Everything else looks good — the owner-ID guard, content-change check, and force=True delegation are all correct.

liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…onichi#824)

Extends on_message_edit with a second case (issue sonichi#795): when the owner
edits their own DM within 5 minutes, reprocess it via _handle_discord_message
with force=True so corrections ("actually do X instead") reach the bridge as a
replacement task.

Restricted to DMs only — channel edits fire on embed unfurls / link previews
which don't represent user intent. Skips no-text-change events for the same
reason. Owner-check via load_allowed() ensures non-owner edits are not
reprocessed.

Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discord-edit-as-follow-up-task: treat MESSAGE_UPDATE on prior requests as a new task

4 participants