Skip to content

fix(client): separate REST and Gateway lifecycles#25

Open
doggy8088 wants to merge 2 commits into
DevRohit06:mainfrom
doggy8088:agent/fix-rest-gateway-separation
Open

fix(client): separate REST and Gateway lifecycles#25
doggy8088 wants to merge 2 commits into
DevRohit06:mainfrom
doggy8088:agent/fix-rest-gateway-separation

Conversation

@doggy8088

Copy link
Copy Markdown

Summary

  • route one-shot commands through an HTTP-only Client.login() lifecycle
  • reserve Gateway sessions for voice and persistent event consumers
  • derive Gateway intents from selected features instead of requesting Intents.all()
  • convert shared resource resolvers to async cache-first lookups with REST fallbacks
  • consolidate duplicated member, role, user, and thread lookup logic
  • close clients after action failures and Gateway startup failures
  • document the connection model and privileged-intent tradeoffs

Root cause

One-shot commands inherited the Gateway lifecycle originally built for serve. As a result, REST-only operations sent a Gateway IDENTIFY with every privileged intent before making their HTTP request. ID-based commands therefore failed when unrelated privileged intents were disabled and consumed unnecessary session-start capacity.

Impact

REST-only commands no longer open a Gateway WebSocket. Numeric resource IDs resolve directly over HTTP without requiring unrelated privileged intents. Name-based member and user lookup retains cache-first behavior and reports when Discord's Server Members intent is required for the REST fallback.

listen, serve, and voice commands continue to use Gateway connections, but now request only the intents required by their selected features. serve keeps Voice States enabled because it accepts voice actions dynamically.

Validation

  • uv run --frozen pytest tests/ -v — 62 passed, 4 skipped
  • uv build — source distribution and wheel built successfully
  • git diff upstream/main --check — no whitespace errors

Fixes #24

Run one-shot commands through Client.login() so REST operations no
longer open a Gateway connection or request unrelated privileged
intents. Reserve Gateway startup for voice and persistent event
consumers, and derive intents from the selected event features.

Convert resource resolvers to asynchronous cache-first lookups with
HTTP fallbacks. Consolidate duplicated member, role, user, and thread
resolution, preserve ID-based access without privileged intents, and
explain when name lookup requires the Server Members intent.

Close clients after actions and Gateway startup failures. Update
listen and serve to use scoped intents and REST fallbacks for missing
cache entries, and document the revised connection model.

Add fake-client coverage for lifecycle cleanup, resolver behavior,
and reaction list execution without a Gateway session.

Refs: DevRohit06#24
@doggy8088
doggy8088 marked this pull request as ready for review July 20, 2026 18:37
Copilot AI review requested due to automatic review settings July 20, 2026 18:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors discli’s Discord client lifecycle so one-shot CLI commands authenticate and use the Discord REST API without opening a Gateway WebSocket, while long-lived / real-time features (listen, serve, voice) keep using Gateway sessions with a minimal, feature-derived intent set. This addresses privileged-intent failures and avoids unnecessary IDENTIFY usage for REST-only operations.

Changes:

  • Introduces separate REST vs Gateway execution paths (run_rest* vs run_gateway*) and derives Gateway intents from selected features instead of Intents.all().
  • Converts shared resolvers to async, cache-first lookups with REST fallbacks (guild/channel/thread/member/user/role), and updates commands to await them.
  • Adds/expands tests to verify REST commands do not start Gateway and that resolver cache/HTTP fallback behavior works as intended.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_utils.py Adds async tests for new cache-first + REST-fallback resolver behavior.
tests/test_rest_commands.py Adds a CLI-level test asserting reaction list uses REST login and never starts Gateway.
tests/test_client.py Adds unit tests for minimal intent selection and REST/Gateway runners (including close-on-failure).
src/discli/utils.py Implements async cache-first resource resolvers plus fetch_guilds() REST fallback helper.
src/discli/commands/webhook.py Moves webhook commands to REST runner and awaits async channel resolution.
src/discli/commands/voice.py Routes voice commands through Gateway runner and adds a cached-guild resolver for voice.
src/discli/commands/typing_cmd.py Moves typing command to REST runner and awaits channel resolution.
src/discli/commands/thread.py Removes duplicated thread resolver and uses shared async resolvers + REST runner.
src/discli/commands/server.py Moves server commands to REST runner and uses REST fetches for channels/roles/member counts.
src/discli/commands/serve.py Derives intents from selected events/features, adds REST fallbacks for ID resolvers, and ensures client closes on failures.
src/discli/commands/role.py Moves role commands to REST runner and uses shared async member/role resolvers.
src/discli/commands/reaction.py Moves reaction commands to REST runner and awaits channel resolution.
src/discli/commands/poll.py Moves poll commands to REST runner and awaits channel resolution.
src/discli/commands/message.py Moves message commands to REST runner and awaits channel resolution.
src/discli/commands/member.py Moves member commands to REST runner and uses shared async member resolver.
src/discli/commands/listen.py Derives intents from selected events and ensures client closes on startup failures.
src/discli/commands/interact.py Moves interact commands to REST runner and awaits channel resolution.
src/discli/commands/event.py Moves event commands to REST runner; uses REST scheduled-event fetch APIs.
src/discli/commands/dm.py Moves DM commands to REST runner and uses shared async user resolver.
src/discli/commands/channel.py Moves channel commands to REST runner; uses shared async resolvers and REST fetches.
src/discli/client.py Adds run_rest* / run_gateway*, minimal-intent builder, permission check split, and backward-compatible run_discord = run_rest.
README.md Documents the new connection model and intent tradeoffs.
docs/guides/building-agents.mdx Updates agent-building guidance to reflect HTTP-only standalone CLI vs persistent Gateway serve.
docs/getting-started/your-first-bot.mdx Clarifies Message Content intent requirements for live Gateway features vs REST commands.
docs/architecture/overview.mdx Updates architecture docs to reflect REST vs Gateway lifecycles and resolver behavior.
CLAUDE.md Updates repository guidance to reflect run_rest() vs run_gateway() patterns.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/discli/commands/serve.py Outdated
Comment on lines +1357 to +1358
except discord.Forbidden:
pass

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid catch — thanks. The except discord.Forbidden: pass did leave member_counts as the zero-initialised dict, so a 403 reported members: 0 for every role instead of marking counts as unavailable. The serve action was also inconsistent with the standalone role list command, which already reset to None.

Fixed in 7c915ee:

  • The Forbidden handler now sets member_counts = None, so counts surface as null / "unavailable" instead of 0.
  • Member-count computation is now opt-in (gated behind a new with_member_counts field on the serve role_list action, default false), so the Forbidden branch only runs when counts are actually requested — which also avoids the full fetch_members(limit=None) scan on every call.
  • Added a regression test asserting a 403 reports unavailable for every role and never 0.

uv run --frozen pytest tests/ -v → 65 passed, 4 skipped.

Comment thread src/discli/commands/role.py Outdated
Comment on lines +24 to +28
try:
member_counts = {role.id: 0 for role in fetched_roles}
async for member in guild.fetch_members(limit=None):
for member_role in member.roles:
if member_role.id in member_counts:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid concern — agreed. Iterating the entire member list (fetch_members(limit=None)) on every role list is too expensive as a default for a REST-only command: it is slow and rate-limit-prone on large servers and requires the privileged Server Members intent.

Fixed in 7c915ee by making member counts opt-in:

  • New --with-member-counts flag on role list (default off). By default fetch_members is not called at all and members is reported as unavailable / null; passing the flag computes per-role counts and reports unavailable (not 0) on a 403.
  • Applied the same opt-in pattern to the serve role_list action via a with_member_counts request field (default false) so the bot protocol stays consistent and avoids the same whole-member-list scan on every call.
  • Documented in docs/reference/cli-commands.mdx, docs/reference/serve-actions.mdx, and agents/discord-agent.md.

Tests cover all three paths: default (no fetch), --with-member-counts (counts computed), and --with-member-counts under 403 (unavailable, never 0). uv run --frozen pytest tests/ -v → 65 passed, 4 skipped.

Addresses two Copilot code-review comments on PR DevRohit06#25 that both touch
the per-role member-count logic moved onto the REST-only path.

1) serve.py `_action_role_list` — incorrect counts on Forbidden
   The `except discord.Forbidden: pass` handler left `member_counts` as
   the dict of zeros that was initialised before `fetch_members()` ran.
   When the bot lacked the Server Members intent (403), every role was
   reported with `members: 0` instead of being marked unavailable. The
   standalone `role.py` command already reset to `None` on Forbidden, so
   the serve action was inconsistent and silently wrong. Fixed by setting
   `member_counts = None` in the handler so counts surface as `null` /
   "unavailable", matching the CLI command.

2) role.py `role list` — expensive by default
   `guild.fetch_members(limit=None)` iterates the entire member list to
   compute per-role counts. On large servers this is slow, rate-limit
   prone, and requires the privileged Server Members intent — yet it ran
   on every `role list` invocation of a REST-only command. Made member
   counts opt-in via a new `--with-member-counts` flag (default off). By
   default `fetch_members` is not called at all and `members` is reported
   as `unavailable`/`null`; passing the flag computes counts and, on
   Forbidden, reports them as unavailable rather than 0.

   Applied the same opt-in pattern to the serve `role_list` action via a
   new `with_member_counts` request field (default false) so the bot
   protocol stays consistent with the CLI and avoids the same
   whole-member-list scan on every call. The Forbidden fix above now
   lives inside this opt-in branch, so it only runs when counts are
   actually requested.

Docs:
- docs/reference/cli-commands.mdx: document `--with-member-counts` and
  the new default (counts omitted).
- docs/reference/serve-actions.mdx: document `with_member_counts` and
  that `members` is `null` by default.
- agents/discord-agent.md: show both default and `--with-member-counts`
  invocations.

Tests (tests/test_rest_commands.py):
- `role list` without the flag does not call `fetch_members` and reports
  `members: unavailable`.
- `role list --with-member-counts` computes counts (Admin: 2, Mod: 1).
- `role list --with-member-counts` against a 403 reports `unavailable`
  for every role and never `0`.

Validation:
- `uv run --frozen pytest tests/ -v` — 65 passed, 4 skipped
  (3 new tests added).
- `uv build` — source distribution and wheel built successfully.
- `git diff --check` — no whitespace errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One-shot REST commands unnecessarily open Gateway sessions and require privileged intents

2 participants