fix: enable only synced ModelScope MCP servers#9084
Open
VectorPeak wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves the sync_modelscope_mcp_servers method in func_tool_manager.py by ensuring only successfully synced MCP servers are enabled, and adds a corresponding unit test. Feedback was provided regarding a potential side effect where mutating local_mcp_config directly could modify a shared global default configuration in memory, with a suggestion to use copy.deepcopy to prevent this.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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
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.
This PR fixes ModelScope MCP sync aborting when the provider response contains unavailable or malformed server entries.
Modifications / 改动点
sync_modelscope_mcp_servers()has two separate steps after receiving the ModelScope provider response:Before this PR, those two steps used different source lists. The config-building step effectively filtered the provider response, because entries without
operational_urlsor without a usableurlwere skipped and never written tolocal_mcp_config["mcpServers"]. The later enable step then iterated over the originalmcp_server_listagain and looked each name up in the filtered local config.That mismatch allowed one unusable provider entry to abort the whole sync even when other valid servers had already been written:
Changes
This change makes the saved-config list and the enabled-server list the same list. While building the local config, the sync code now skips entries without a server name, skips entries without
operational_urls, skips entries whose first operational URL has no usableurl, and records each successfully written(name, config)pair insynced_servers.The enable step then iterates over
synced_serversdirectly, so only configs that were actually saved can be enabled. There is also a small state-isolation fix in the same path:load_mcp_config()is deep-copied before mutation, which prevents a fallback module-levelDEFAULT_MCP_CONFIGobject from being modified in memory when the local config file is missing or unreadable.The successful ModelScope sync behavior stays unchanged: valid MCP servers are still saved with
transport: "sse",active: true, andprovider: "modelscope". The change only aligns enablement with the same filtering rules already used when writing the local config.Evidence
Focused reproduction covered by
test_modelscope_sync_enables_only_synced_servers:The focused test also covers the mutable-default case by making
load_mcp_config()return a shared default config object and asserting it remains unchanged after sync.Local focused pytest evidence:
Validation run locally after addressing review comments:
GitHub CI is green for unit tests, format check, CodeQL, dashboard build, and smoke tests across Linux, macOS, and Windows.
Affected call chain / impact
The affected path is the Dashboard ModelScope MCP provider sync flow:
A mixed ModelScope response can therefore no longer make an unavailable skipped server block valid synchronized servers. The change is limited to the ModelScope provider sync path. It does not change manual local MCP config loading, non-ModelScope providers, the saved config shape for valid ModelScope servers, or the SSE transport selection used for those valid entries.
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ No new feature is added; this is a bug fix.
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ Focused pytest, Ruff check, Ruff format check,
git diff --check, and GitHub CI were verified.🧐 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ No new dependencies are introduced.
😇 My changes do not introduce malicious code.