Skip to content

fix: enable only synced ModelScope MCP servers#9084

Open
VectorPeak wants to merge 2 commits into
AstrBotDevs:masterfrom
VectorPeak:fix/modelscope-mcp-skipped-server
Open

fix: enable only synced ModelScope MCP servers#9084
VectorPeak wants to merge 2 commits into
AstrBotDevs:masterfrom
VectorPeak:fix/modelscope-mcp-skipped-server

Conversation

@VectorPeak

@VectorPeak VectorPeak commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

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:

  1. build and save the local MCP config from provider entries that have enough data to run locally
  2. enable the MCP servers that were just synchronized

Before this PR, those two steps used different source lists. The config-building step effectively filtered the provider response, because entries without operational_urls or without a usable url were skipped and never written to local_mcp_config["mcpServers"]. The later enable step then iterated over the original mcp_server_list again 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:

POST /api/v1/mcp/providers/modelscope/sync
  -> ToolsService.sync_provider(...)
    -> FunctionToolManager.sync_modelscope_mcp_servers(...)
      -> receive mixed ModelScope response: valid server + unavailable server
      -> write only the valid server into local_mcp_config["mcpServers"]
      -> save the filtered local MCP config
      -> iterate the original provider response during enablement
      -> look up local_mcp_config["mcpServers"]["missing-url"]
      -> KeyError before the valid synchronized server can be enabled reliably

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 usable url, and records each successfully written (name, config) pair in synced_servers.

The enable step then iterates over synced_servers directly, 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-level DEFAULT_MCP_CONFIG object 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, and provider: "modelscope". The change only aligns enablement with the same filtering rules already used when writing the local config.

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Evidence

Focused reproduction covered by test_modelscope_sync_enables_only_synced_servers:

Fake ModelScope response:
- valid: operational_urls: [{"url": "https://example.com/mcp"}]
- missing-url: operational_urls: []
- empty-url: operational_urls: [{}]
- no-name: operational_urls: [{"url": "https://example.com/no-name"}]

Before:
- only valid was written into local_mcp_config["mcpServers"]
- the enable step could still iterate over missing-url / empty-url / no-name
- skipped entries could raise KeyError and abort sync

After:
- only successfully synchronized server configs are recorded
- only valid is enabled
- skipped entries no longer abort the sync

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:

Focused pytest evidence for PR 9084

Validation run locally after addressing review comments:

uv run pytest tests/unit/test_func_tool_manager.py -q
18 passed

uv run ruff check astrbot/core/provider/func_tool_manager.py tests/unit/test_func_tool_manager.py
passed

uv run ruff format --check astrbot/core/provider/func_tool_manager.py tests/unit/test_func_tool_manager.py
passed

git diff --check
passed

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:

Dashboard MCP provider sync action
  -> POST /api/v1/mcp/providers/modelscope/sync
  -> astrbot/dashboard/api/tools.py::sync_modelscope_mcp_servers(...)
  -> _sync_modelscope_mcp_servers(...)
  -> ToolsService.sync_provider({"name": "modelscope", ...})
  -> FunctionToolManager.sync_modelscope_mcp_servers(access_token)
  -> fetch ModelScope mcp_server_list
  -> filter usable entries into local_mcp_config["mcpServers"]
  -> save_mcp_config(local_mcp_config)
  -> enable only the saved entries from synced_servers

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.txt and pyproject.toml.
    / No new dependencies are introduced.

  • 😇 My changes do not introduce malicious code.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. labels Jun 30, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread astrbot/core/provider/func_tool_manager.py

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant