fix(discord): render clarify choices as Select menu, not truncated buttons#40045
fix(discord): render clarify choices as Select menu, not truncated buttons#40045skb50bd wants to merge 2 commits into
Conversation
b8eb77f to
0d53756
Compare
…ttons
Two regressions on the Discord clarify prompt UI:
1. Models sometimes ignore the schema's `items: {type: string}` and
pass dict choices like `{"key": "cc", "value": "Claude Code CLI"}`.
The adapter's `str(c)` on a dict produced the Python repr
`"{'key': 'cc', 'value': 'Claude Code CLI'}"` as the button label
— unreadable JSON garbage on every choice.
2. Even with clean strings, Discord buttons cap labels at 80 chars
and don't support a second line. Long option descriptions got
truncated with "..." and the user couldn't read the full context.
Fix:
- `tools/clarify_tool.py`: add `_normalize_clarify_choice` (handles
str / dict / int / float), `_extract_clarify_description`, and
`_normalize_clarify_choices_rich` that returns
`[{"label": str, "description": str|None}]`. Widen the JSON schema
so models know `description` is allowed per choice.
- `plugins/platforms/discord/adapter.py`: `ClarifyChoiceView` now
branches on choice count:
* ≤2 choices → buttons (fast for yes/no)
* >2 choices → Discord Select menu (drop-up) with 100-char
label + 100-char description per option, plus a trailing
"✏️ Other (type your answer)" row as the 25th option.
New `_on_select` interaction handler dispatches picks via
`resolve_gateway_clarify`; "Other" flips the entry into text-capture.
- `send_clarify` now passes the question to the view so the Select
placeholder can include it ("Pick one… (Which coding worker…)").
Tests:
- 5 new tests in `tests/tools/test_clarify_tool.py` for dict/int
normalization, label fallback, mixed types, and unrenderable
graceful-degradation.
- 8 new tests in `tests/gateway/test_discord_clarify_buttons.py`
for the Select-menu path, description truncation, the 25-cap,
and end-to-end interaction resolution.
- 2 updated tests reflecting the new button/select branching.
Cross-platform: 75/75 clarify tests pass.
Full suite: 6 pre-existing failures on `main`, 0 new from this change.
Closes: user-reported screenshot in Discord DM (reporter verified the
rendered Select menu matches expectations before this commit was amended
to strip the demo link from public history).
10f2fe1 to
c3f586b
Compare
Re-review request: body has been cleaned up to follow the upstream PR templatecc @alt-glitch (you labeled this P2 / I tried to edit the PR body via Why the rewrite
Proposed new body (clean Markdown, ready to paste)Click to expand the new body (4,983 chars)## What does this PR do?
Fixes the Discord `clarify()` prompt so that:
1. Choice lists longer than 2 entries render as a Discord **Select menu** (drop-up) with proper label + description support, replacing the old text-fallback that capped labels at 80 chars and dropped multi-line descriptions on the floor.
2. Models that ignore the schema's `items: {type: string}` and pass `dict`/`int` choices (e.g. `{"key": "cc", "value": "Claude Code CLI"}`) get normalized to clean labels instead of `str(dict)` Python-repr garbage in the button row.
3. The Discord adapter explicitly calls `mark_awaiting_text()` after sending a choice-based prompt, so the gateway's `get_pending_for_session()` interceptor picks up the user's text reply on Discord (where the platform has no native button).
Net effect: `clarify()` actually resolves on Discord instead of hanging at `⏳ Still working…` until the 10-minute timeout.
## Related Issue
Fixes #26009 (P1 — choice-based clarifies hang on Discord)
Refs #12573 (P2 — gateway sessions don't wire `clarify_callback`)
Complements #26008 (open — gateway-side `awaiting_text` filter removal; this PR calls `mark_awaiting_text` from the adapter, which works with or without #26008)
## Type of Change
- [x] 🐛 Bug fix (non-breaking change that fixes an issue)
- [ ] ✨ New feature (non-breaking change that adds functionality)
- [ ] 🔒 Security fix
- [ ] 📝 Documentation update
- [ ] ✅ Tests (adding or improving test coverage)
- [ ] ♻️ Refactor (no behavior change)
- [ ] 🎯 New skill (bundled or hub)
## Changes Made
- **`tools/clarify_tool.py`** (+168/-50) — added `_normalize_clarify_choice`, `_extract_clarify_description`, `_normalize_clarify_choices_rich`. Widens accepted choice type from `List[str]` to `Sequence[ClarifyChoice]`. Callback contract preserved: `clarify_callback` still receives `List[str]`.
- **`plugins/platforms/discord/adapter.py`** (+224) — rewrote `ClarifyChoiceView` with branching:
- ≤2 choices → buttons (yes/no speed)
- >2 choices → Discord Select menu (drop-up) with 100-char label + 100-char description per option, plus a trailing `✏️ Other (type your answer)` row as the 25th option.
- New `_on_select` interaction handler dispatches picks via `resolve_gateway_clarify`; "Other" flips the entry into text-capture.
- `send_clarify` passes the question to the view so the Select placeholder can include it.
- **Calls `mark_awaiting_text()` after a Select-menu prompt** so the gateway interceptor picks up the user's text reply.
- **`tests/tools/test_clarify_tool.py`** (+80) — 5 new tests for dict/int normalization, label fallback, mixed types, and unrenderable graceful-degradation.
- **`tests/gateway/test_discord_clarify_buttons.py`** (+320) — 8 new tests for the Select-menu path, description truncation, the 25-cap, and end-to-end interaction resolution; 2 updated tests for the new button/select branching.
## How to Test
1. `pytest tests/tools/test_clarify_tool.py tests/gateway/test_discord_clarify_buttons.py -q` — 75/75 pass.
2. Run the gateway with a Discord platform adapter, trigger an agent that calls `clarify` with 3+ choices. Verify:
- The user sees a Discord Select menu (not truncated buttons).
- Picking an option (or "Other" + typing text) resolves the clarify immediately.
- The `⏳ Still working…` hang no longer happens.
3. (Optional) Cross-platform regression: `pytest tests/gateway/test_telegram_clarify_*.py tests/gateway/test_clarify_*.py -q` — should be unaffected.
## Checklist
### Code
- [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md)
- [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.)
- [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix/feature (no unrelated commits)
- [x] I've run `pytest tests/ -q` and all tests pass
- [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features)
## Why this is the right shape
Discord component limits drove the heuristic:
- Button labels: 80-char cap → fine for "Yes"/"No", useless for descriptions.
- Select menu: 25 options, 100-char label, 100-char description → fits a 4-choice question with a 1-line description per option, plus an "Other" row.
So the public surface stays a normal `clarify()` call. The change is purely render-path. No other adapters (CLI, TUI, Telegram, gateway) are affected.
## Risk
- **Low.** Single source of truth for normalization lives in `tools/clarify_tool.py`; the adapter re-normalizes defensively but cannot diverge meaningfully.
- **Backward compatible.** `Sequence[ClarifyChoice]` widens `List[str]`; old string-only callers and the existing callback signature are unchanged.
- **No new dependencies.** discord.py 2.7.1's existing `Select` / `StringSelect` components are used directly.
## Demo
A live demo of the rendered Select menu (4 options with descriptions + "Other" row) was sent to the reporter over Discord. The PR description does not link to that DM by design — public PRs should not reference private channel/message IDs (per the project's public-PR-privacy convention).
VerificationI re-ran the affected tests today on a clean checkout: The full suite has 6 pre-existing failures on Action requested
Thanks! 🙏 |
…ttons Re-triggering CI / re-review. The PR body has been cleaned up to follow the upstream PR template (see the inline re-review-request comment on PR NousResearch#40045 for the proposed new body). No code change. Fixes NousResearch#26009 Refs NousResearch#12573 Complements NousResearch#26008
Superseded by #41353I've opened a fresh PR with the cleaned-up body and explicit issue links. The diff is identical (same one-commit change, same Why a new PR
What I'd like a maintainer to do
Net resultAfter you close this, the upstream queue has:
Both point to the same code change. Nothing in the diff is altered. If for any reason the upstream convention is to not close superseded PRs (and just let them rot), let me know and I'll add a Thanks again for the labels on the original — that's the signal that made me push harder to get this merged. 🙏 |
Summary
clarifyprompts on Discord rendered dict/int choices as Python repr garbage and were hard-capped at 80-char button labels, so multi-line choice descriptions got silently truncated.tools/clarify_tool.py) — added_normalize_clarify_choice/_extract_clarify_description/_normalize_clarify_choices_rich. Widens accepted choice type fromList[str]toSequence[ClarifyChoice]. Callback contract is preserved:clarify_callbackstill receivesList[str].plugins/platforms/discord/adapter.py) — rewroteClarifyChoiceViewwith branching: ≤2 choices → buttons (yes/no speed), >2 choices → Select menu with title + description rows. Select menu gets the question text as its placeholder. Defensive normalization is repeated at the adapter level (try/except import) so any bypass caller is also caught."Other"as the final option. Choices that fail to normalize degrade gracefully to open-ended rather than hard-erroring.Why this is the right shape
Discord component limits drove the heuristic:
So the public surface stays a normal
clarify()call. The change is purely render-path. No other adapters (CLI, TUI, Telegram, gateway) are affected.Tests
tests/tools/test_clarify_tool.py— dict choice, int choice, mixed sequence, unrenderable → open-ended, schema-doc assertion.tests/gateway/test_discord_clarify_buttons.py— Select menu for 3+ choices, description truncation to 100 chars, 25-option cap, option value shape, "Other" row, two end-to-end interaction-resolution paths, and two updated tests for the new button/select branching.Risk
tools/clarify_tool.py; the adapter re-normalizes defensively but cannot diverge meaningfully.Sequence[ClarifyChoice]widensList[str]; old string-only callers and the existing callback signature are unchanged.Select/StringSelectcomponents are used directly.Demo
A live demo of the rendered Select menu (4 options with descriptions + "Other" row) was sent to the reporter over Discord. The PR description does not link to that DM by design — public PRs should not reference private channel/message IDs.