Skip to content

feat(discord): outbound @Name → <@id> via config.yaml (re-scope of #69206) + forum path - #75633

Closed
mehmetkr-31 wants to merge 3 commits into
NousResearch:mainfrom
mehmetkr-31:feat/discord-outbound-mentions-config
Closed

feat(discord): outbound @Name → <@id> via config.yaml (re-scope of #69206) + forum path#75633
mehmetkr-31 wants to merge 3 commits into
NousResearch:mainfrom
mehmetkr-31:feat/discord-outbound-mentions-config

Conversation

@mehmetkr-31

Copy link
Copy Markdown
Contributor

Closes #69203. This is the re-scope #69206 was closed asking for:

A focused re-scope using a documented discord.resolve_outbound_mentions config.yaml setting (with any internal compatibility bridge kept private) would fit the project configuration model.

@tieubao's commits are carried unchanged and unsquashed, so the runtime work stays theirs — the triage review already called it "clean and does what the issue asks" and "a solid parity fix." Only the gating and one missed call path are mine.

What changed from #69206

1. The knob is config.yaml, not a new env var. The opt-in is now discord.resolve_outbound_mentions, documented in hermes_cli/config_defaults.py next to approval_mentions, and bridged to the internal DISCORD_RESOLVE_MENTIONS var by _apply_yaml_config. That is the same arrangement discord.approval_mentions already uses (adapter.py:6782 reads _env_bool("DISCORD_APPROVAL_MENTIONS"), bridged at adapter.py:9731-9736), so this adds no new user-facing environment variable — it plugs into the bridge the adapter already has.

2. Forum posts were being skipped. In #69206 the resolver ran after the forum-parent branch had already returned, so _send_to_forum() passed the raw content straight into create_thread(content=...) and a forum starter post kept the inert @Name. The call now sits above that branch, so an ordinary channel message and a forum post get identical treatment. Same bug class, second call path.

Verification — each break point verified to fail

what I removed result
the config→env bridge 2 faileddiscord.resolve_outbound_mentions: true did not reach the adapter
the send() call site 2 failedsend() delivered the raw @Name — the resolver is not on the send path
moved the call back below the forum branch 1 failedthe forum thread starter kept the raw @Name
nothing (restored) 16 passed

The TestSendCallSite class exists specifically because the unit tests call _resolve_outbound_mentions directly and would all still pass with the send() call site deleted — a resolver nothing calls is dead code, and I wanted a test that says so.

No test reads adapter source; @tieubao had already replaced the earlier ast.parse/exec version with one that imports DiscordAdapter and rides the real import path. I additionally moved the flag handling onto monkeypatch so it can't leak into unrelated tests.

tests/gateway/ + tests/hermes_cli/test_config*.py: 16 failures on this branch, 16 on a pristine origin/main worktree at the same base — identical set. Nothing introduced, nothing masked.

contributors/emails/nntruonghan@gmail.comtieubao added per contributors/README.md, since this PR carries their commits.

@teknium1 teknium1 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.

Thanks for carrying forward the prior runtime work and for covering the forum early-return path.

Problems

  • The resolver iterates guild.members at plugins/platforms/discord/adapter.py:5156, but startup requests the privileged member intent only for named allowlist entries or role authorization (adapter.py:1185-1193). Turning on the new setting alone does not ensure the member cache can resolve the target name.
  • Resolution is added to send() (adapter.py:2943) but not to edit_message(), which edits raw formatted content at adapter.py:3207-3241; overflow continuations also send raw chunks at adapter.py:3317-3347. Streaming/edit delivery remains outside the feature.
  • The re-scope says the env bridge is private, but the added default-config comment advertises DISCORD_RESOLVE_MENTIONS as an env override (hermes_cli/config_defaults.py:1884-1885) and the new test preserves that public override (tests/gateway/test_discord_outbound_mentions.py:121-128).

Suggested changes

  • Gate member-intent setup on this opt-in and document/test the portal prerequisite.
  • Cover edit_message() and overflow continuations.
  • Keep the compatibility bridge private or obtain maintainer direction for exposing the new env override.

Automated hermes-sweeper review.

return content
pairs = []
seen = set()
for member in (getattr(guild, "members", None) or []):

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.

guild.members is not reliably populated unless the privileged members intent is requested. The startup condition currently enables that intent only for named allowlist entries or role authorization (adapter.py:1185-1193); include this opt-in in that condition and document/test the Discord Portal requirement.

# a user or another bot by name; a bare "@Name" from an LLM is otherwise
# inert text. Done before the forum branch so a forum thread's starter
# post gets the same treatment as an ordinary channel message.
content = await self._resolve_outbound_mentions(content, channel)

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.

Please cover the corresponding outbound edit path as well: edit_message() formats and calls msg.edit() without resolving names (adapter.py:3207-3241), and its overflow helper sends continuation chunks directly. Streaming responses can otherwise still deliver inert @Name text.

Comment thread hermes_cli/config_defaults.py Outdated
# the guild's own members, so the person or bot is actually pinged. A
# model reliably writes "@Name" rather than the raw <@id> Discord needs,
# which otherwise renders as inert plain text. @everyone and roles stay
# governed by the existing allowed_mentions safe defaults. Env override:

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.

The predecessor review requested a config.yaml setting with any compatibility bridge kept private. Advertising DISCORD_RESOLVE_MENTIONS here, together with the new explicit-env-wins test, makes this a new user-facing non-secret env override rather than a private implementation bridge.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter area/config Config system, migrations, profiles sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #69206 is the closed predecessor. This re-scope keeps the resolver opt-in through discord.resolve_outbound_mentions and also covers the forum starter-post path.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 31, 2026
@mehmetkr-31
mehmetkr-31 force-pushed the feat/discord-outbound-mentions-config branch from 5a80aa6 to 1c7c1ce Compare July 31, 2026 21:09
@mehmetkr-31

Copy link
Copy Markdown
Contributor Author

All three are fair. Fixed in 1c7c1ce.

1. edit_message() and the overflow continuations

This one stings, because I fixed the forum early-return in this same PR and then missed the bigger path — edit_message() is how every streamed reply is delivered, so the feature was off for most of what the gateway actually sends.

The resolver now runs once in edit_message(), before format_message(), which also covers _edit_overflow_split() since it re-formats that same content.

Only on finalize=True, deliberately. Mid-stream the text is still partial: with members Alice and Al, the streamed prefix @Al matches the wrong person, and an edit can deliver that ping. Resolving once at finalize means the message the user is left with is the correct one. There's a test for that specific case (test_mid_stream_edit_is_left_alone) — moving the call out of the if finalize: guard fails it.

2. The members intent

Correct, and it would have been a silent no-op: guild.members stays empty, the resolver finds nothing, and nothing tells the operator why.

I pulled the condition out into a module-level _needs_members_intent(allowed_user_ids, allowed_role_ids) — following the same "extract it as a helper and import it" suggestion from the review on #69206 — so it now includes the opt-in and is testable without a live client. "*" still does not pull the intent in, so the migrate-from-OpenClaw path is unchanged, and there's a test pinning that.

My first attempt at this test re-implemented the condition inside the test instead of calling the real one, so deleting the production gate didn't fail anything. The helper is what makes the assertion real.

The Developer Portal prerequisite (Bot → Privileged Gateway Intents → Server Members Intent) is documented on the setting and in the helper's docstring.

3. The env bridge is private again

You're right that I'd re-advertised it. The Env override: DISCORD_RESOLVE_MENTIONS line is gone from config_defaults.py, and test_explicit_env_wins_over_config — which pinned the public override as contract — is deleted. The config comment now documents only discord.resolve_outbound_mentions plus the intent prerequisite. _apply_yaml_config still writes the var, purely as the internal plumbing the adapter's ~50 os.getenv() call sites read.

Verification

reverted result
edit_message() resolution 2 failed — final streamed edit keeps the raw @Name
resolution moved to every edit 1 failed — a partial name was resolved mid-stream
the members-intent clause 1 failed
nothing 22 passed

tests/gateway/: 16 failures on this branch, 16 on a pristine origin/main worktree at the same base — identical set. @tieubao's two commits are still carried unsquashed.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

One PR addresses #69203. #75633 adds opt-in outbound Discord @Name-to-<@id> resolution across ordinary sends, forum starter posts, finalized streaming edits, and overflow continuations, while wiring the required member intent and documenting its Discord Portal prerequisite.

Related pull requests

  • feat(discord): outbound @Name → <@id> via config.yaml (re-scope of #69206) + forum path #75633 best fix — (+447/-10) — fixes, keep open: The diff directly addresses the reported inert outbound mentions on standard, forum, and finalized streaming delivery paths, with targeted tests and the guild-members intent required for name lookup. It addresses the three problems raised in teknium1's visible keep_open review, but that contributor review has not been superseded by a re-review of the updated head.

Suggested consolidation

Keep #75633 open with a salvage path: preserve its opt-in resolver, standard/forum/final-edit/overflow call-site coverage, member-intent gating, documentation, and regression tests, then obtain contributor re-review confirming that the concerns in the existing keep_open review are resolved. There are no competing PRs to close as duplicates.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I69203(["issue #69203 (open)"])
    P75633["PR #75633 (open)"]
    P75633 -->|best fix| I69203
    class I69203 open
    class P75633 open
    class P75633 best
    class P75633 target
    click I69203 "https://github.com/NousResearch/hermes-agent/issues/69203"
    click P75633 "https://github.com/NousResearch/hermes-agent/pull/75633"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 27 kB of PR diffs, 5 kB of issue/PR text, 5 kB of discussion (6 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation and removed sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 4, 2026
tieubao added 2 commits August 4, 2026 21:01
An LLM writes @name rather than the raw <@id> Discord needs, so an agent's attempt to tag a user or hand off to another bot is inert plain text. The Feishu adapter already resolves mentions on outbound; this adds the same to Discord, reusing the guild-member name->id matching the adapter already has (_resolve_allowed_usernames).

Opt-in via DISCORD_RESOLVE_MENTIONS (unset/false = byte-identical behavior). @everyone/roles stay governed by the existing allowed_mentions safe defaults.

Fixes NousResearch#69203

Signed-off-by: Han Ngo <nntruonghan@gmail.com>
@mehmetkr-31
mehmetkr-31 force-pushed the feat/discord-outbound-mentions-config branch from 6b4650f to f6502e6 Compare August 4, 2026 18:02
… edit + forum paths

Re-scope of NousResearch#69206 (closed under the env-var-for-config policy) to the shape
the sweeper asked for: the opt-in is the documented
``discord.resolve_outbound_mentions`` setting in config.yaml, bridged to the
internal ``DISCORD_RESOLVE_MENTIONS`` var by ``_apply_yaml_config``, exactly as
the neighbouring ``discord.approval_mentions`` works. The bridge is kept
private -- the default-config comment documents only the config key, and no
test pins the env override.

Three delivery paths, not one. The original resolved mentions in send() only:

* the forum branch returned before the resolver ran, so a forum thread's
  starter post kept the inert @name;
* edit_message() -- which is how every streamed reply is delivered -- never
  resolved at all, and neither did its _edit_overflow_split() continuation
  chunks, which re-format the same content.

The resolver now runs above the forum branch in send(), and once in
edit_message() on the FINAL edit. Mid-stream is deliberately skipped: the text
is still partial there, so a member named "Al" matches while "@alice" is only
half-written, and an edit can deliver that ping. Resolving once at finalize
means the message the user keeps is the correct one, and it also covers the
overflow-split path.

guild.members is empty unless the privileged Server Members intent is
requested, which startup asked for only for named allowlist entries or role
authorization -- so turning the setting on resolved nothing and gave no clue
why. The condition moves to a module-level _needs_members_intent() helper that
includes this opt-in and is testable without a live client. The "*" open-mode
wildcard still does not pull the intent in, so the migrate-from-OpenClaw path
is unchanged.

The Developer Portal prerequisite is documented where someone setting this up
actually reads: the intent table and its explanation in the Discord setup
guide, the discord: config block next to the other settings, and voice-mode.md,
whose "only needed if your allowlist uses usernames" claim was now incomplete.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mehmetkr-31

Copy link
Copy Markdown
Contributor Author

Closing this to clear stale work — but flagging what goes with it, because it is not only mine.

This PR carries two commits by @tieubao / Han Ngo (nntruonghan@gmail.com) unsquashed, salvaged from #69206. That PR was reviewed as doing what its issue asked and was closed for exactly one reason — DISCORD_RESOLVE_MENTIONS being a non-secret behavioural env var — and the closing comment named the fix it wanted: a focused re-scope onto a documented discord.resolve_outbound_mentions config.yaml setting. That is what the third commit here does.

So the work is finished and matches the stated ask; it has simply been open three weeks, and measured against this repo's merge behaviour (157 external merges sampled across four weeks, none slower than 24h, median ~20 minutes) it is not waiting on a queue.

Nothing is lost: the branch and all three commits are intact, authorship included. If the re-scope is still wanted, say so and I will rebase it onto current main and reopen — @tieubao's commits stay unsquashed either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have platform/discord Discord bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discord adapter: no outbound @Name -> <@id> mention resolution (Feishu has it)

5 participants