Fix crash in gptel-mcp--activate-tools when requested servers return no tools#1321
Open
benthamite wants to merge 1 commit intokarthink:masterfrom
Open
Fix crash in gptel-mcp--activate-tools when requested servers return no tools#1321benthamite wants to merge 1 commit intokarthink:masterfrom
benthamite wants to merge 1 commit intokarthink:masterfrom
Conversation
Two bugs in the `add-all-tools` callback inside `gptel-mcp-connect`: 1. When called for specific servers that fail to start or return no tools, the callback passes nil to `gptel-mcp--activate-tools`. Its `(unless tools ...)` fallback fetches tools from ALL connected MCP servers and tries to look them up via `gptel-get-tool`, crashing on tools never registered with `gptel-make-tool`. Fix: guard the call with `when` so nil means "nothing to activate". 2. When there is a mix of active-but-unregistered and inactive servers, only the inactive servers' tools are registered. The callback's fallback `(mapcar #'car inactive-servers)` skips the already-active servers entirely, so their tools are never passed to `gptel-make-tool`. Fix: use `servers` (all unregistered) instead of `inactive-servers` in the fallback, so the callback registers tools from every server that needs it.
benthamite
force-pushed
the
fix/mcp-activate-tools-nil-crash
branch
from
5a53975 to
e6a6764
Compare
Owner
|
Thanks for the PR @benthamite. I took a brief look but didn't understand the logic. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
gptel-mcp-connecthas two bugs in itsadd-all-toolscallback that surface when calling it programmatically for specific servers:Bug 1: Crash on nil tools list
When the requested servers fail to start or return no tools, the callback passes nil to
gptel-mcp--activate-tools. Its(unless tools ...)fallback fetches tools from all connected MCP servers and tries to look them up viagptel-get-tool, crashing on tools from servers that were never registered withgptel-make-tool:even though home-assistant was never requested.
Bug 2: Active-but-unregistered servers skipped
When there is a mix of active-but-unregistered and inactive-but-unregistered servers, the callback's fallback
(mapcar #'car inactive-servers)only processes the inactive ones. Tools from already-active servers that still need registration are silently dropped.For example, if brave-search (active, connected) and slack (inactive, init) are both unregistered:
mcp-hub-start-all-serverstarts only slackinactive-servers→ only slackFix
Two one-line changes in the
add-all-toolslambda:gptel-mcp--activate-toolswithwhenso nil means "nothing to activate" rather than "activate everything".inactive-serverstoservers(all unregistered), so the callback registers tools from every server that needs it—both those just started and those already active.