feat(discord): searchable /model autocomplete - #65931
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Looks Good
- Discord
/modelautocomplete is now searchable (fuzzy filter on model names) - Clean addition to
cron/jobs.pywith corresponding test files - No security or performance concerns
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for bringing searchable Discord /model selection forward; remote main still exposes /model only as a free-text field at plugins/platforms/discord/adapter.py:4247-4250, so the feature is still useful.
Problems
plugins/platforms/discord/adapter.py:4392truncatesChoice.value. That changes the/modelargument beforegateway/slash_commands.py:1480-1487parses--provider, so a long model ID or foreign-provider selection can no longer identify the intended target.- The PR bundles unrelated cron, voice PCM, LoginFailure, and liveness changes. In particular, the new broad handler at
plugins/platforms/discord/adapter.py:1345precedes an existing broad handler at line 1359, leaving the latter unreachable.
Suggested changes
- Skip choices whose value cannot be represented intact; do not truncate them. Add tests for long values and for complete foreign-provider values.
- Isolate the
/modelautocomplete change from the unrelated commits, and consolidate the duplicate exception handling if the LoginFailure work is retained.
Automated hermes-sweeper review.
d6f9535 to
ee4a3c0
Compare
|
Thanks for the review — both points addressed, and the branch has been rebuilt on current Truncated Bundled changes — removed. The PR is now only the Two of them have been reopened standalone, addressing your point directly rather than dropping them:
The cron and liveness changes were local-only and have been dropped rather than upstreamed. One correction to the description, for the record: the autocomplete callback offloads the catalog build via |
SummaryFour PRs are grouped with this complex, but only #54338, #61593, and #77380 address Discord category policy; #65931 is unrelated searchable Related pull requests
DuplicatesIssues #38539 and #38540 are duplicates, as confirmed by contributor discussion. No exact PR duplicates remain: #54338 overlaps through overloaded channel scopes, #61593 is a contributor-confirmed separate category-API alternative, #77380 is the broader three-rule implementation, and #65931 is unrelated. Suggested consolidationKeep #77380 open with a salvage path centered on its complete three-rule contract and tests, because it is the recorded best fix for both duplicate issues; treat #38539 as canonical and close #38540 as its duplicate. Do not close #54338 or #61593 as duplicates without resolving the documented API choice, and keep #65931 outside this complex under its existing keep-open review. Cross-PR triage: Reviewed 4 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 114 kB of PR diffs, 26 kB of issue/PR text, 15 kB of discussion (16 comments), 8 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
/model is currently a free-text field, so switching models means recalling an exact id. Add an autocomplete callback that live-filters every authenticated provider's models as the user types, ranking the current provider first so an empty query still surfaces the likely target. The catalog is sourced from the same authenticated-provider list the interactive picker uses, but uncapped, so all models are searchable rather than the picker's first 50. It is cached briefly because the callback fires on every keystroke, and the build is offloaded with asyncio.to_thread since it reads the on-disk provider-model cache -- doing that inline stalls the gateway on a cold or stale cache. Choice.value is the literal /model argument and is later parsed for --provider, so values that exceed Discord's 100-character cap are skipped rather than truncated: a trimmed value silently becomes a different or unresolvable switch target. Labels are display-only and are still ellipsised to fit. Authorization is pre-checked so the catalog is not leaked to unauthorized users in the picker, and the callback never raises -- any failure yields an empty suggestion list instead of a typing-time error popup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ee4a3c0 to
548f3d6
Compare
|
Rebased onto current |
Supersedes #57796 (auto-closed when the head repo was deleted during my username migration). Addresses the review there: autocomplete catalog read offloaded via asyncio.to_thread, plus regression coverage in the connect/liveness suites.