fix(discord): canonicalize PluralKit inbound ids#55413
Conversation
Greptile SummaryThis PR fixes a bug where PluralKit-proxied webhook messages were excluded from the PluralKit API lookup (due to a Key changes:
One consequence worth monitoring: removing the Confidence Score: 5/5Safe to merge — the fix is narrowly scoped, logically correct, and well-covered by new tests. No correctness issues found. The removal of the !webhookId guard is the intended fix; the 404 → null path for non-PK messages is already properly handled. canonicalMessageId propagation is clean, the fallback to message.id is correct, and the new MessageSidFull field is additive. The increased PK API call volume for non-PK webhook messages is an acknowledged, documented trade-off, not a bug. No files require special attention.
|
| Filename | Overview |
|---|---|
| extensions/discord/src/monitor/message-handler.preflight.ts | Removes !webhookId guard from shouldCheckPluralKit and adds canonicalMessageId to the returned context from pluralkitInfo.original |
| extensions/discord/src/monitor/message-handler.preflight.types.ts | Adds optional canonicalMessageId field to DiscordMessagePreflightContext |
| extensions/discord/src/monitor/message-handler.process.ts | Destructures canonicalMessageId from ctx, uses it for MessageSid with message.id fallback, always sets MessageSidFull to message.id |
| extensions/discord/src/monitor/message-handler.preflight.test.ts | Adds mock for fetchPluralKitMessageInfo and a new test verifying canonical original id is preserved for webhook-authored PK messages |
| extensions/discord/src/monitor/message-handler.process.test.ts | Expands getLastDispatchCtx to track MessageSid/MessageSidFull and adds test verifying PK original id is used as MessageSid |
| extensions/discord/src/monitor/message-handler.preflight.test-helpers.ts | Adds optional webhookId param to createDiscordMessage helper |
Reviews (1): Last reviewed commit: "fix(discord): canonicalize PluralKit inb..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e953587cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const shouldCheckPluralKit = Boolean(pluralkitConfig?.enabled); | ||
| let pluralkitInfo: Awaited<ReturnType<typeof fetchPluralKitMessageInfo>> = null; | ||
| if (shouldCheckPluralKit) { | ||
| try { |
There was a problem hiding this comment.
Avoid PK lookup before webhook-echo suppression
When discord.pluralkit.enabled is true, shouldCheckPluralKit now ignores webhookId, so webhook-authored messages always call fetchPluralKitMessageInfo before preflight reaches the later bound-thread webhook suppression (shouldIgnoreBoundThreadWebhookMessage). In active bound-thread sessions this causes OpenClaw’s own webhook echo messages (which are dropped anyway) to still incur an external PluralKit API request, adding avoidable latency and rate-limit/log noise for no functional gain.
Useful? React with 👍 / 👎.
|
Addressed the Codex feedback about unnecessary PK polling on our own bound-thread webhook echoes. What changed in the latest push:
Local validation after the change: corepack pnpm exec vitest run \
extensions/discord/src/monitor/message-handler.preflight.test.ts \
extensions/discord/src/monitor/message-handler.process.test.ts \
extensions/discord/src/monitor/thread-bindings.lifecycle.test.ts \
extensions/discord/src/monitor/reply-delivery.test.ts \
extensions/discord/src/monitor/provider.test.tsResult: 5 files passed, 116 tests passed. |
e211232 to
043c625
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 043c625c63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pluralkitConfig?.enabled) { | ||
| try { | ||
| pluralkitInfo = await fetchPluralKitMessageInfo({ | ||
| messageId: message.id, | ||
| config: pluralkitConfig, | ||
| }); |
There was a problem hiding this comment.
Run PluralKit identity lookup before bot-drop gate
The PluralKit lookup now happens only after the early bot gate, so allowBots=false messages can be discarded before sender.isPluralKit is ever resolved. In practice, if a webhook-authored PK message arrives without webhookId in the event payload (or hydration fails and we keep the partial payload), this path drops the message at the bot check and never reaches the lookup that would classify it as PK. This is a regression from the previous ordering, where lookup could still rescue bot-authored PK traffic in that degraded-payload scenario.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Summary Reproducibility: yes. A focused current-main check shows webhook-authored messages return no PluralKit info because Next step before merge Security Review findings
Review detailsBest possible solution: Land a rebased Discord fix that filters known OpenClaw bound-thread webhook echoes before external PK calls, performs PK lookup for every other PK-enabled inbound message before bot gates, carries a trimmed Do we have a high-confidence way to reproduce the issue? Yes. A focused current-main check shows webhook-authored messages return no PluralKit info because Is this the best way to solve the issue? No. The provenance-driven canonical id approach is the right narrow fix, but this PR needs the lookup ordering corrected and the mapping rebased into the current Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against b8ddb8a494b2. |
|
Thanks @acgh213. I landed the equivalent fix on main in 70dcd81f03 after rechecking the PluralKit API contract and current inbound dedupe/reply paths. Closing this PR as superseded. |
Summary
Canonicalize Discord inbound message ids for PluralKit-routed messages by using the PluralKit
originalmessage id when available.What changed
canonicalMessageIdthrough Discord preflight contextMessageSidto the PluralKitoriginalid when presentMessageSidFullWhy
The earlier withdrawn approach tried to reduce duplicate turn pressure with broader webhook suppression and heuristic clone dedupe. This replacement takes a narrower provenance-driven path instead:
Validation
Ran:
corepack pnpm exec vitest run \ extensions/discord/src/pluralkit.test.ts \ extensions/discord/src/monitor/message-handler.preflight.test.ts \ extensions/discord/src/monitor/message-handler.process.test.ts \ extensions/discord/src/monitor/thread-bindings.lifecycle.test.ts \ extensions/discord/src/monitor/reply-delivery.test.tsAll passed.
Notes
MessageSidFullstill preserves the actual provider message id for downstream consumers that need the concrete Discord message id