feat(discord): resolve trusted principals via identity links#70944
feat(discord): resolve trusted principals via identity links#70944ericberic wants to merge 6 commits into
Conversation
Greptile SummaryThis PR extends the Discord channel to resolve a sender's Discord user ID to a canonical Two minor items worth noting:
Confidence Score: 4/5Safe to merge; no correctness or security issues found. One displaced import and an undocumented PluralKit behaviour worth addressing before or after merge. All findings are P2 (style and documentation). The core resolution logic is correct, well-tested across 5+ unit-test cases, and the refactoring of resolveCanonicalIdentityFromLinks preserves the existing internal call-site cleanly. Score is 4 rather than 5 only because the PluralKit trust semantics are non-obvious and currently undocumented in the code. extensions/discord/src/monitor/agent-components.ts (misplaced import), extensions/discord/src/monitor/sender-identity.ts (PluralKit trust comment) Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/discord/src/monitor/agent-components.ts
Line: 119
Comment:
**Import placed mid-file after function definitions**
The static import of `resolveDiscordTrustedPrincipalFromUserId` is placed at line 119, inside a block of function definitions, rather than at the top of the file with all other imports. While static imports are hoisted by the JS runtime, placing them here breaks the file's import-at-top convention and is likely a minor merge artifact.
Move this line to the top-level import block alongside the other imports.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/discord/src/monitor/sender-identity.ts
Line: 62-65
Comment:
**PluralKit trust lookup uses relay-bot ID, not the proxied member ID**
`trustedPrincipal` is always derived from `params.author.id` (the PluralKit relay bot's Discord snowflake), even though for PK messages `sender.id` is set to `pkMember.id`. Operators configuring identity links must map the relay bot's ID, which grants the same principal to *all* members of that PK system. If per-member granularity is ever needed, the lookup would need to be repeated for `memberId` after the PK branch is entered.
This appears intentional (the test asserts exactly this behaviour with `system_owner: ["discord:444555666"]`), but a brief code comment clarifying the relay-bot-as-trust-anchor semantics would help future maintainers.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Discord: resolve trusted principals via ..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1277ec4541
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…uting blocked on it Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
Codex review: found issues before merge. Reviewed July 3, 2026, 3:08 AM ET / 07:08 UTC. Summary PR surface: Source +53, Tests +195, Docs +1, Generated 0. Total +249 across 21 files. Reproducibility: not applicable. as a bug reproduction; this is a feature PR. Source inspection confirms current main lacks the proposed field and resolver, and the PR body provides after-fix terminal output for the changed resolver and metadata builder. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep this PR open as a Discord implementation candidate, but land it only after maintainer/security approval of the trusted identity and SDK contracts and after removing the release-owned changelog edit. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction; this is a feature PR. Source inspection confirms current main lacks the proposed field and resolver, and the PR body provides after-fix terminal output for the changed resolver and metadata builder. Is this the best way to solve the issue? Mostly yes: Discord-specific principal resolution belongs in the Discord plugin, and reusing the existing identity-link resolver is cleaner than duplicating lookup logic. The unresolved part is maintainer approval of the trusted metadata and public SDK contracts. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ed6c78b7891. Label changesLabel justifications:
Evidence reviewedPR surface: Source +53, Tests +195, Docs +1, Generated 0. Total +249 across 21 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
1eca439 to
b2ea219
Compare
b2ea219 to
b69f66b
Compare
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Gilded Lint Imp Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
|
This pull request has been automatically marked as stale due to inactivity. |
…uting blocked on it Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
b69f66b to
2f97030
Compare
…uting blocked on it Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…uting blocked on it Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…uting blocked on it Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Summary
Completes identity-links support for the Discord plugin. When a Discord message, component interaction, or native slash-command context arrives, the sender's stable Discord user ID is resolved through the agent's
session.identityLinksconfig and passed into trusted inbound metadata assender_principal.Changes
resolveDiscordTrustedPrincipalFromUserId().sender_principalin the trusted inbound metadata prompt only when an operator-configured identity link matches.resolveCanonicalIdentityFromLinks()helper throughopenclaw/plugin-sdk/routinginstead of duplicating identity-link lookup logic.Real behavior proof
Behavior addressed: Discord senders mapped in
session.identityLinksnow produce trusted prompt metadata withsender_principal, while raw sender IDs and spoofable display names remain excluded from the trusted metadata block.Real environment tested: Local OpenClaw source checkout on macOS/Darwin, branch
feat/discord-resolve-trusted-principals-via-identity-linksat2f97030d0a9c77e569cf1a852fee19e63cb135cd, running the changed Discord sender identity resolver and inbound metadata builder through Node + tsx from the working tree.Exact steps or command run after this patch:
Evidence after fix:
Observed result after fix: The trusted metadata payload emitted
sender_principal = "alice"from the configured stablediscord:111111111111111111identity link, using PluralKitpkInfo.senderrather than the relay/webhook author ID or display metadata.What was not tested: End-to-end delivery through a live Discord bot/account was not tested in this local pass. GitHub CI is expected to provide broad lane proof for the rebased SHA after the branch push.
Verification
pnpm docs:listnode scripts/run-vitest.mjs extensions/discord/src/monitor/sender-identity.test.ts extensions/discord/src/monitor/message-handler.preflight.test.ts extensions/discord/src/monitor/message-handler.process.test.ts extensions/discord/src/monitor/native-command-context.test.ts src/routing/session-key.test.ts src/auto-reply/reply/inbound-meta.test.ts- passed 3 Vitest shards, 6 files, 287 tests.node --max-old-space-size=8192 --import tsx scripts/generate-plugin-sdk-api-baseline.ts --checkgit diff --checkSafety notes
session.identityLinksand stable Discord snowflake IDs.pkInfo.senderwhen available so identity links target the real Discord sender instead of spoofable display metadata.Disclosure
AI-assisted, per the repository contribution policy.