Skip to content

fix(discord): PluralKit DM pairing identity + direct peer regex (#86332)#86397

Merged
steipete merged 4 commits into
openclaw:mainfrom
Sanjays2402:fix/discord-pluralkit-dm-pairing
May 31, 2026
Merged

fix(discord): PluralKit DM pairing identity + direct peer regex (#86332)#86397
steipete merged 4 commits into
openclaw:mainfrom
Sanjays2402:fix/discord-pluralkit-dm-pairing

Conversation

@Sanjays2402

@Sanjays2402 Sanjays2402 commented May 25, 2026

Copy link
Copy Markdown
Contributor

Fixes

Two related Discord DM-handling bugs from #86332.

Bug 1 — PluralKit DM pairing infinite loop

resolveDiscordDmPreflightAccess calls the access check with params.sender (the resolved, PK-aware identity) but then calls handleDiscordDmCommandDecision with params.author (the raw gateway author). For non-PluralKit users these are identical, so the bug is invisible. For PluralKit-proxied users:

  • resolveDiscordDmCommandAccess → createDiscordDmIngressSubject(params.sender) keys lookups under the PK member UUID.
  • handleDiscordDmCommandDecision used params.author.id (the Discord user/webhook id) when storing the pairing record.

The next inbound DM from the same PK user looked up the PK member UUID, found nothing, and issued a new pairing challenge — forever.

Fix: pass params.sender to the pairing handler so the stored key matches the lookup key. Fall back to gateway author for tag/name only.

Bug 2 — extractDiscordSessionKind regex missing direct

Normalized DM session keys look like agent:<id>:discord:direct:<userId>. The regex matched channel|group|dm but not direct. Today every caller compares against \"dm\" and treats null the same way, so there is no user-visible breakage — but the function silently lies about the session kind.

Fix: add direct to the regex and map it to dm in the return value, preserving the existing union type and downstream comparisons.

Verification

node scripts/run-vitest.mjs run \
  extensions/discord/src/approval-native.ts \
  extensions/discord/src/monitor/dm-command-decision.test.ts \
  extensions/discord/src/monitor/message-handler.preflight.test.ts

Test Files  3 passed (3)
     Tests  52 passed (52)

Closes #86332

@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 25, 2026
@clawsweeper

clawsweeper Bot commented May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 25, 2026, 4:45 AM ET / 08:45 UTC.

Summary
The PR changes Discord DM preflight pairing to store records under the resolved sender identity and updates native approval session-kind parsing so normalized discord:direct: keys behave as logical DMs.

PR surface: Source +12. Total +12 across 2 files.

Reproducibility: yes. by source inspection: PluralKit makes sender.id the PK member id, DM access uses that id for lookup, and current main stores the pairing request under author.id. I did not run a live Discord/PluralKit scenario in this read-only review.

Review metrics: 1 noteworthy metric.

  • Discord runtime files changed: 2 files, +17/-5. The diff is small, but both files sit on Discord DM authorization/session routing paths where green CI alone does not settle identity semantics.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Add redacted real Discord/PluralKit after-fix proof showing a paired PK DM is recognized on a later inbound message.
  • Add focused regression coverage for the PluralKit pairing identity handoff and discord:direct: session-kind parsing.
  • Get maintainer confirmation that pairing should persist the PK member UUID as the authorization principal.

Proof guidance:
Needs real behavior proof before merge: The PR body only shows targeted Vitest output; the contributor should add redacted real Discord/PluralKit DM pairing proof in the PR body, after which ClawSweeper should re-review automatically or a maintainer can comment @clawsweeper re-review.

Risk before merge

  • The PR changes which identity gets persisted as the Discord DM pairing principal for PluralKit-proxied users; maintainers should confirm PK member id is the intended authorization boundary and whether any dual-id or migration behavior is needed.
  • The PR body only reports targeted Vitest output, so external real behavior proof for a Discord plus PluralKit DM pairing flow is still missing before merge.

Maintainer options:

  1. Confirm the paired principal before merge
    Have a maintainer approve that PluralKit member UUID, not raw Discord user id or both ids, is the intended persistent pairing principal for proxied DMs.
  2. Require proof and regression coverage (recommended)
    Add focused coverage for PluralKit pairing identity and discord:direct: parsing, then attach redacted after-fix Discord/PluralKit runtime proof before landing.
  3. Pause if the identity boundary is undecided
    If maintainers are unsure whether pairing should authorize the PK member, raw Discord account, or both, pause this PR until that policy is settled.

Next step before merge
This PR should stay in maintainer review because the code fix is plausible, but the remaining blockers are real behavior proof and authorization-boundary approval rather than an automated code repair.

Security
Needs attention: The diff is narrow and does not add dependencies or secret handling, but it changes a Discord DM pairing authorization principal and needs maintainer security review before merge.

Review details

Best possible solution:

Land the narrow Discord plugin fix after maintainer approval of the PK member-id pairing boundary, focused regression coverage, and redacted real Discord/PluralKit proof; avoid changing core pairing APIs unless that review finds a broader contract issue.

Do we have a high-confidence way to reproduce the issue?

Yes, by source inspection: PluralKit makes sender.id the PK member id, DM access uses that id for lookup, and current main stores the pairing request under author.id. I did not run a live Discord/PluralKit scenario in this read-only review.

Is this the best way to solve the issue?

Yes, the proposed code is the narrow owner-boundary fix because it makes the stored pairing key match the existing sender-based lookup and preserves direct as logical dm. The merge should still wait for real behavior proof, regression coverage, and maintainer confirmation of the authorization principal.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against c791e4242bc8.

Label changes

Label changes:

  • add P2: This is a normal-priority Discord plugin bug fix with limited blast radius but real impact for PluralKit DM pairing users.
  • add merge-risk: 🚨 security-boundary: Merging changes the persisted identity used for Discord DM pairing authorization, so maintainers need to approve the intended PluralKit principal boundary.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body only shows targeted Vitest output; the contributor should add redacted real Discord/PluralKit DM pairing proof in the PR body, after which ClawSweeper should re-review automatically or a maintainer can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a normal-priority Discord plugin bug fix with limited blast radius but real impact for PluralKit DM pairing users.
  • merge-risk: 🚨 security-boundary: Merging changes the persisted identity used for Discord DM pairing authorization, so maintainers need to approve the intended PluralKit principal boundary.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body only shows targeted Vitest output; the contributor should add redacted real Discord/PluralKit DM pairing proof in the PR body, after which ClawSweeper should re-review automatically or a maintainer can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +12. Total +12 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 2 17 5 +12
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 17 5 +12

Security concerns:

  • [medium] Confirm the paired identity boundary — extensions/discord/src/monitor/message-handler.dm-preflight.ts:83
    This line changes the persistent pairing principal from the raw Discord author id to the resolved PluralKit member id. That appears aligned with lookup behavior, but maintainers should confirm this is the intended authorization boundary and whether any dual-id compatibility is needed.
    Confidence: 0.76

What I checked:

Likely related people:

  • steipete: Recent GitHub history on the central Discord DM access files includes channel ingress access, DM access groups, and PluralKit inbound-id work relevant to this pairing path. (role: recent Discord access contributor; confidence: high; commits: a0fb7fb04547, b217cd09726f, 70dcd81f03be; files: extensions/discord/src/monitor/message-handler.dm-preflight.ts, extensions/discord/src/monitor/dm-command-auth.ts, extensions/discord/src/monitor/sender-identity.ts)
  • thewilloftheshadow: Commit 8e2b17e added Discord PluralKit sender identity resolution, the feature whose member-id identity exposes the pairing key mismatch. (role: feature introducer; confidence: high; commits: 8e2b17e0c519; files: extensions/discord/src/monitor/sender-identity.ts, extensions/discord/src/pluralkit.ts, extensions/discord/src/monitor/message-handler.preflight.ts)
  • vincentkoc: GitHub history for approval-native shows adjacent Discord native approval and session-routing work, which is the surface touched by the direct session-kind parser change. (role: adjacent approval runtime contributor; confidence: medium; commits: c3d3cf23bcd2, b201589ae11c; files: extensions/discord/src/approval-native.ts)
  • scoootscooob: Commit 5682ec3 moved the Discord channel implementation into extensions/discord, including the files involved in this PR. (role: refactor author; confidence: medium; commits: 5682ec37fada; files: extensions/discord/src/monitor/message-handler.preflight.ts, extensions/discord/src/monitor/sender-identity.ts, extensions/discord/src/approval-native.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels May 25, 2026
@clawsweeper

clawsweeper Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@omarshahine

omarshahine commented May 25, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR. Before this can move forward, please add live proof from the affected surface, not just unit tests, mocked tests, or source inspection.

A useful proof update should include:

  • the exact build/SHA tested
  • the real environment used
  • the command, UI flow, channel flow, provider request, or other live path exercised
  • before/after symptom evidence where applicable
  • the observed result after the patch
  • any remaining proof gaps

Please redact secrets, tokens, phone numbers, and private message content from logs or screenshots.

@RomneyDa

RomneyDa commented May 29, 2026

Copy link
Copy Markdown
Member

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

@steipete steipete self-assigned this May 31, 2026
Sanjays2402 and others added 4 commits May 31, 2026 16:25
@Sanjays2402 @steipete
fix(discord): PluralKit DM pairing identity + direct peer regex (open…
7c528a0 
…claw#86332)

Bug 1: resolveDiscordDmPreflightAccess passed the raw gateway author
to handleDiscordDmCommandDecision while the access check used the
resolved (PK-aware) sender identity. The two diverge for PluralKit
users — the pairing record was keyed by the Discord user id while
the ingress resolver looked it up by PK member UUID. Result: every
inbound DM from a PK user triggered a fresh pairing challenge.

Bug 2: extractDiscordSessionKind regex matched 'dm' but not 'direct',
which is the canonical peer kind used for normalized DM session keys.
No visible breakage today because all callers compare against 'dm',
but a latent fragility for any future caller that distinguishes
'direct' from null.

- Pass params.sender (resolved identity, includes PK UUID) to the
  pairing decision so the record key matches the lookup key.
- Extend the kind regex to accept 'direct' and map it to 'dm'.

Closes openclaw#86332
@steipete
test(discord): cover PluralKit pairing identity
c0c4cdf
@steipete
fix(discord): normalize PluralKit pairing ids
ebe6cb7
@steipete
fix(discord): detect account-scoped DM sessions
0db617d
@steipete steipete force-pushed the fix/discord-pluralkit-dm-pairing branch from 9a602f7 to 0db617d Compare May 31, 2026 15:26
@steipete steipete added the proof: override Maintainer override for the external PR real behavior proof gate. label May 31, 2026
@steipete

steipete commented May 31, 2026

Copy link
Copy Markdown
Contributor

Maintainer update before landing:

  • Rebased the PR branch onto current origin/main and pushed the fixups to the contributor branch.
  • Added regression coverage for the PluralKit DM pairing identity handoff, the pk: pairing-store allowlist lifecycle, canonical discord:direct: approval sessions, and account-scoped discord:<account>:direct: approval sessions.
  • Fixed the pairing write to store PluralKit member approvals as pk:<member-id>, which matches the existing Discord allowlist normalization path while keeping lookup subjects as the bare PK member id.
  • Applied proof: override for this external PR because maintainer review is landing it with source-level proof and focused regression coverage; no live Discord + PluralKit setup was available in this lane.

Verification run locally on head 0db617df7a877bdc6fb4c5b826d22dde4b3e66ba after rebase:

node scripts/run-vitest.mjs extensions/discord/src/approval-native.test.ts extensions/discord/src/monitor/dm-command-auth.test.ts extensions/discord/src/monitor/dm-command-decision.test.ts extensions/discord/src/monitor/message-handler.preflight.test.ts

Result: 4 test files passed, 82 tests passed.

Autoreview:

.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main

Result: clean after addressing two accepted findings.

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 31, 2026
@steipete steipete removed the status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. label May 31, 2026
@steipete steipete merged commit e0e7bae into openclaw:main May 31, 2026
156 of 163 checks passed
@steipete steipete mentioned this pull request May 31, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

channel: discord Channel integration: discord merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: override Maintainer override for the external PR real behavior proof gate. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Discord DM pairing identity mismatch breaks PluralKit users; extractDiscordSessionKind regex missing "direct" peer kind

4 participants

@Sanjays2402 @omarshahine @RomneyDa @steipete