Conversation
alt-glitch
added
type/bug
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
`/reload-mcp` built its added/removed/reconnected sets purely from the live
connection registry on either side of the reconnect:
added = connected_servers - old_servers
removed = old_servers - connected_servers
reconnected = connected_servers & old_servers
So "removed" answered "was connected, isn't now" — not what the label
claims. Any server still in `config.yaml` but not connected at the instant
the diff runs is reported as `➖ Removed`: a connect still in flight past
`mcp_discovery_timeout` (1.5s by default), a server set `enabled: false`, or
one that failed this round. On the reported shape:
♻️ Reconnected: docs, search
➖ Removed: calendar, maps <- both still configured, both connect
seconds later via late binding
The label is not the only casualty. The same set is injected into
conversation history ("Removed servers: calendar, maps"), so the model is
told its tools were deleted while they are on their way in.
`classify_reload_diff()` in tools/mcp_tool.py owns the classification for
both surfaces: `removed` is now "no longer in the config", and a configured
server that simply is not connected yet comes back separately as
`not_connected`. `_load_mcp_config` deliberately does not apply the
`enabled` filter, so a server switched off in config stays out of `removed`
too — matching what the label promises. A config read that fails degrades to
the old connection-only answer rather than raising through a reload path.
Both call sites move onto it — the CLI (`_reload_mcp`) and the gateway
(`/reload-mcp` handler), which had the identical expression. The CLI gains an
explicit "Still configured, not connected" line; the gateway renders through
i18n, so reporting that set there needs a new translation key and is left
out rather than smuggled in. Its false `Removed` claim — the actual bug, in
output and in injected history alike — is fixed either way.
Tests cover the reported shape, a genuinely deleted server, a disabled one,
a fresh add, bucket disjointness/coverage, and the config-unreadable
fallback.
Refs NousResearch#80771
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0xGr1mm
force-pushed
the
fix/reload-mcp-diff-reads-config
branch
from
August 9, 2026 08:57
c452387 to
7c36b60
Compare
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
3 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.
What does this PR do?
/reload-mcpbuilt its diff purely from the live connection registry on either side of the reconnect:So
removedanswers "was connected, isn't now" — which is not what the label claims. Any server still inconfig.yamlbut not connected at the instant the diff runs is reported as➖ Removed: a connect still in flight pastmcp_discovery_timeout(1.5s by default), a server setenabled: false, or one that failed this round.On the reported shape:
The label is not the only casualty. The same set is injected into conversation history as
Removed servers: calendar, maps, so the model is told its tools were deleted while they are on their way in.The fix
classify_reload_diff()intools/mcp_tool.pyowns the classification for both surfaces:removednow means no longer in the config.not_connected._load_mcp_configdeliberately does not apply theenabledfilter, so a server switched off in config stays out ofremovedtoo — matching what the label promises.Old vs new on the reported inputs (
before={docs,search,calendar,maps},after={docs,search}, all four configured):Related Issue
Refs #80771 (P2,
comp/cli,tool/mcp,area/config). Searched open and merged PRs first, per CONTRIBUTING's search-first section:Update: #80855 (open) also fixes #80771 and is the broader of the two. It adds
summarize_mcp_reload()intools/mcp_tool.py, wires it intocli.pyandgateway/run.py, and adds thegateway.reload_mcp.pendingkey across all 17 locale catalogs, which is exactly the gateway i18n this PR's reviewer note leaves out by design. A reviewer should see both before picking one; if #80855 is preferred, this PR can be closed in its favour.Nothing else addresses the diff's source of truth. The other open
/reload-mcpPRs are about different things: #77074 / #77643 pointmcp addsuccess messages at the command, #72864 reloads args on config changes, #46520 restarts the MCP loop. The issue also distinguishes itself from #78426, where the servers genuinely failed andRemovedwas the correct label.Type of Change
Bug fix (non-breaking).
Changes Made
tools/mcp_tool.py— newclassify_reload_diff(before, after)returningadded/removed/reconnected/not_connected, with the config read as the source of truth for "removed" and a documented fallback.cli.py—_reload_mcpclassifies through the helper; gains an explicit⏳ Still configured, not connected:line. The history injection a few lines below reads the same correctedremoved, so it is fixed by the same change.gateway/run.py— the/reload-mcphandler had the identical expression; moved onto the helper.tests/tools/test_mcp_reload_diff.py— new, 6 tests.How to Test
6 passed: the reported shape, a genuinely deleted server, a disabled-but-configured server, a fresh add, bucket disjointness/coverage, and the config-unreadable fallback.
450 passed — the existing MCP suite is unaffected.
Because the classifier is new code rather than an edited branch, the before/after proof is the direct comparison above: the pre-fix expression (copied verbatim from
cli.pyandgateway/run.py) against the shipped helper on the issue's own inputs.Regression checks across all three touched surfaces:
Full
tests/tools/suite, this branch vsmain, same environment, run serially:Comparing the failing sets rather than the counts: zero regressions — the two sets are identical, nothing added and nothing removed. The +6 is this PR's new tests. Those 71 are pre-existing in a non-hermetic single-process run (that suite carries browser/docker/network-dependent cases); CI's per-file isolation via
run_tests_parallel.pyis the supported path.Checklist
Code
fix(mcp): …)Documentation & Housekeeping
removedcli-config.yaml.example— N/A, no config keysCONTRIBUTING.md/AGENTS.md— N/ANotes for the reviewer
One deliberate asymmetry: the CLI prints raw strings, so it gained the
Still configured, not connectedline directly. The gateway renders throught(), so surfacing that set there needs a new i18n key across locales — I left it out rather than add translations as a side effect of a bug fix. The gateway's falseRemovedclaim is fixed either way, in both its output and its injected history; if you want the extra line there too, say so and I will add the key.