Skip to content

fix(discord): allow category-scoped channel policies - #54338

Open
KLIEBHAN wants to merge 1 commit into
NousResearch:mainfrom
KLIEBHAN:fix/discord-category-channel-policy
Open

KLIEBHAN wants to merge 1 commit into
NousResearch:mainfrom
KLIEBHAN:fix/discord-category-channel-policy

Conversation

@KLIEBHAN

@KLIEBHAN KLIEBHAN commented Jun 28, 2026

Copy link
Copy Markdown

Summary

  • Include Discord category IDs in the channel policy scope used by allowed, ignored, and free-response channel checks.
  • Reuse one helper across message handling, no-mention filtering, and slash-command authorization.
  • Add regression coverage for category-based free responses and slash-command allow/ignore policy.

Motivation

Discord text channels expose their containing category as category_id, while threads expose their parent channel through parent or parent_id. Existing channel policy checks only considered the concrete channel ID and, in some paths, a thread parent ID. That meant deployments could allow a thread parent or single channel, but could not reliably target a whole category with DISCORD_ALLOWED_CHANNELS or DISCORD_FREE_RESPONSE_CHANNELS.

Tests

python -m pytest tests/gateway/test_discord_free_response.py tests/gateway/test_discord_slash_auth.py

Result: 78 passed.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/discord Discord bot adapter labels Jun 28, 2026
@KLIEBHAN
KLIEBHAN force-pushed the fix/discord-category-channel-policy branch 3 times, most recently from a6b8e86 to 85611fc Compare June 29, 2026 05:57

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

Thanks for the focused Discord policy coverage. The category-ID gap is still present on current main: plugins/platforms/discord/adapter.py:4914-4945 builds the shared channel keys from channel ID/name/#name and parent scope, but not category_id.

Problems

  • The current shared key mechanism supersedes the PR's standalone _discord_channel_scope_ids approach. A salvage should preserve the current ID/name/#name/parent matching contract used by normal messages at plugins/platforms/discord/adapter.py:6170-6198 and slash authorization at :3444-3472.
  • Current fail-closed message authorization passes only channel and parent IDs into _is_allowed_user at plugins/platforms/discord/adapter.py:1151-1163; category-only DISCORD_ALLOWED_CHANNELS must be included there or normal messages are denied before _handle_message.
  • The new free-response test exercises _handle_message, not the earlier no-mention admission filter changed by the PR. Add an end-to-end category case for that gate, plus a thread-under-category case.

Suggested changes

  • Rework the patch against the shared channel-key resolver and cover all authorization paths above.
  • Update website/docs/user-guide/messaging/discord.md:283-289 if category IDs are intentionally accepted in the existing channel-list settings. #61593 proposes a different explicit-category API, so maintainers should select the intended configuration shape.

Automated hermes-sweeper review.

Comment thread plugins/platforms/discord/adapter.py Outdated
@@ -5130,6 +5119,31 @@ def _get_parent_channel_id(self, channel: Any) -> Optional[str]:
return str(parent_id)
return None

def _discord_channel_scope_ids(self, channel: Any) -> set[str]:

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.

Current main now centralizes channel policy matching in _discord_channel_keys_from_channel() (adapter.py:4914-4945) and includes channel names, #name, and parent names. Please salvage category scope through that shared resolver rather than introducing a parallel ID-only key set, so all existing policy forms retain parity.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@KLIEBHAN
KLIEBHAN force-pushed the fix/discord-category-channel-policy branch from 85611fc to c88fc22 Compare July 15, 2026 08:57
@KLIEBHAN

KLIEBHAN commented Jul 15, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed review. I rebased the PR onto current main and reworked the salvage around the shared channel-key resolver.

  • Category IDs are now added in _discord_channel_keys_from_channel() for both direct channels and threads via their parent channel; the existing ID/name/#name/parent keys remain unchanged.
  • The earlier fail-closed on_message authorization and the no-mention admission filter now both use the shared resolver.
  • Slash allow/ignore authorization continues to use that same resolver, including category scope.
  • Uncached discord.Thread instances now retain thread/parent policy keys instead of raising when Thread.category_id cannot resolve its parent from the guild cache.
  • Added event-admission tests for a category channel and a thread under that category, downstream free-response coverage for a thread under a category, shared-key coverage, and discriminating slash allow/ignore tests.
  • Updated both the environment-variable table and detailed config.yaml documentation for category IDs in the existing channel-list settings.

Targeted result: 138 passed (test_discord_connect.py, test_discord_free_response.py, test_discord_slash_auth.py, and test_discord_prompt_content_siblings.py). A real discord.py==2.7.1 uncached Thread smoke test also passes.

@KLIEBHAN
KLIEBHAN force-pushed the fix/discord-category-channel-policy branch from c88fc22 to 4877e4d Compare July 15, 2026 09:03

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

This was generated by AI during triage.

Summary

Two PRs are included in this complex, but their diffs address separate Discord gaps: #54338 adds category-scoped channel-policy resolution, while #65931 adds searchable /model autocomplete. Neither diff implements the other's reported functionality.

Related pull requests

  • #54338 related — (+291/-36) — keep open with a salvage path: The diff adds category IDs to the shared channel-key resolver and applies that resolver to early message authorization, no-mention admission, downstream message handling, and slash authorization, with direct-channel, thread-under-category, uncached-thread, allowlist, ignorelist, and documentation coverage. This preserves the salvage path identified by the contributor keep_open review on #54338 and directly addresses its stated concerns about the shared ID/name/#name/parent contract and the earlier admission gates.
  • #65931 related — (+269/-1) — keep open with a salvage path: The isolated diff adds authorized, thread-offloaded, searchable /model autocomplete, preserves representable model values verbatim, skips overlong values, and tests filtering, limits, authorization, and provider-qualified values. This follows the contributor keep_open review on #65931: the current diff no longer contains the cited unrelated changes, and it addresses the value-truncation objection with discriminating regression coverage.

Suggested consolidation

Keep both PRs open on separate salvage paths rather than consolidating them as duplicates: retain #54338 for category-scoped Discord channel policies and #65931 for searchable /model autocomplete. The diffs are functionally independent, both now address the blocking concerns recorded in their respective contributor keep_open reviews, and there is no evidence that either is already implemented on main or duplicates the other.

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

@andrexibiza

Copy link
Copy Markdown
Contributor

Scope note (dedup-campaign triage, 2026-08-03): this PR covers category scoping for allowed/ignored/free-response policies. The canonical feature request (#38539) also requires require_mention_categories — the key that makes "free everywhere except category C" expressible. The complete three-key implementation shipped in #77380; PR bodies reference Fixes #38539, so merging #77380 closes the issue. No action needed on this PR beyond a maintainer consolidation call.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/discord Discord bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants