Skip to content

fix(discord): clamp limit to [1, 100] in _search_members and _fetch_messages - #19749

Open
sprmn24 wants to merge 1 commit into
NousResearch:mainfrom
sprmn24:fix/discord-limit-clamp
Open

fix(discord): clamp limit to [1, 100] in _search_members and _fetch_messages#19749
sprmn24 wants to merge 1 commit into
NousResearch:mainfrom
sprmn24:fix/discord-limit-clamp

Conversation

@sprmn24

@sprmn24 sprmn24 commented May 4, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Clamps the limit parameter to a valid range [1, 100] in two Discord tool functions. Previously only an upper bound of 100 was enforced via min(limit, 100), allowing zero or negative values to be sent to the Discord API.

Type of Change

  • Bug fix

Changes Made

  • _search_members: min(limit, 100)max(1, min(limit, 100))
  • _fetch_messages: min(limit, 100)max(1, min(limit, 100))

How to Test

Pass limit=0 or limit=-5 to either function — previously would send invalid value to Discord API, now clamps to 1.

Checklist

  • Follows existing code style
  • No secrets or sensitive data
  • Minimal, focused change

@alt-glitch alt-glitch added type/bug Something isn't working platform/discord Discord bot adapter comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have labels May 4, 2026
@sprmn24
sprmn24 force-pushed the fix/discord-limit-clamp branch from 815ef0c to 01965bd Compare May 25, 2026 15:09

@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 the focused fix. The premise is confirmed on current main: tools/discord_tool.py:495 and tools/discord_tool.py:521 still pass zero or negative limits through after applying only the upper cap. The proposed clamp correctly addresses both sites.

Problems

  • Regression coverage is missing for the new lower bound. tests/tools/test_discord_tool.py:390-395 covers only the upper cap for member search, and tests/tools/test_discord_tool.py:423-429 covers normal message pagination.

Suggested changes

  • Add tests through the public Discord handlers for limit=0 and a negative limit on both actions, asserting the outbound request receives "limit": "1".

This is an automated hermes-sweeper review.

Comment thread tools/discord_tool.py
@@ -332,7 +332,7 @@ def _search_members(token: str, guild_id: str, query: str, limit: int = 20, **_k
limit = int(limit)

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 add regression coverage for this lower-bound branch. Current tests cover only the upper cap for search_members (tests/tools/test_discord_tool.py:390-395) and do not exercise zero or negative values for either action.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have platform/discord Discord bot adapter sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants