feat: expand native MCP SDK integration#9277
Draft
Last-emo-boy wants to merge 8 commits into
Draft
Conversation
dosubot
Bot
added
the
size:XXL
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
dosubot
Bot
added
area:core
5 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The pagination compatibility handling for MCP resources relies on matching exact error message strings (
_MCP_RESOURCE_PAGINATION_ERRORSvs theRuntimeErrortext inMCPClient); consider centralizing these messages as shared constants or using a dedicated exception type to avoid fragile string coupling. - In the MCP resource pagination flow, both the backend (
list_mcp_resources/list_mcp_resource_templates) and frontend (mcpApi.listResources/listResourceTemplatesandloadResources/loadTemplates) treatcursoras falsy (using...(cursor ? { cursor } : {})andcursor || undefined), which will drop valid cursors like an empty string; if empty or non-string cursors are possible, switch to an explicitcursor !== undefinedcheck.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pagination compatibility handling for MCP resources relies on matching exact error message strings (`_MCP_RESOURCE_PAGINATION_ERRORS` vs the `RuntimeError` text in `MCPClient`); consider centralizing these messages as shared constants or using a dedicated exception type to avoid fragile string coupling.
- In the MCP resource pagination flow, both the backend (`list_mcp_resources` / `list_mcp_resource_templates`) and frontend (`mcpApi.listResources` / `listResourceTemplates` and `loadResources` / `loadTemplates`) treat `cursor` as falsy (using `...(cursor ? { cursor } : {})` and `cursor || undefined`), which will drop valid cursors like an empty string; if empty or non-string cursors are possible, switch to an explicit `cursor !== undefined` check.
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/services/tools_service.py" line_range="100-109" />
<code_context>
+def _serialize_mcp_resource_content(
</code_context>
<issue_to_address>
**issue (bug_risk):** Resource content serialization omits annotations, diverging from frontend types and other MCP serialization helpers.
For resources and templates you serialize `annotations`, but `_serialize_mcp_resource_content` does not. Since `McpTextResourceContent` / `McpBlobResourceContent` include `annotations`, any content-level annotations from the MCP server will be dropped and the frontend types won’t match reality. Either add `annotations` to the serialized content (mirroring `_serialize_mcp_resource` and `_serialize_mcp_resource_template`) or remove it from the frontend type if content-level annotations aren’t supported.
</issue_to_address>
### Comment 2
<location path="dashboard/src/components/extension/McpResourceBrowserDialog.vue" line_range="462-378" />
<code_context>
+ async readResource(resource) {
</code_context>
<issue_to_address>
**suggestion:** Selecting a resource immediately clears existing contents before the new preview is available, causing a brief empty state even on fast responses.
`readResource` currently sets `selectedResource`, immediately clears `contents`, then shows the loader. For fast responses this causes the preview to briefly disappear and reappear. Consider either keeping the previous contents until the new data arrives, or only clearing `contents` after a successful response. Since you already use `generation` and `readRequestId` to handle races, adjusting when `this.contents = []` runs should be safe.
Suggested implementation:
```
async readResource(resource) {
const generation = this.generation;
const requestId = ++this.readRequestId;
this.selectedResource = resource;
this.readError = "";
this.readLoading = true;
try {
const response = await mcpApi.readResource(
this.serverName,
resource.uri,
```
To fully implement the behavior where previous contents remain visible until the new data arrives, ensure that:
1. Inside the `readResource` success branch (after `await mcpApi.readResource(...)` resolves and *before* assigning new contents to `this.contents`), you explicitly clear or overwrite `this.contents` with the new data in one step. For example:
- `if (generation === this.generation && requestId === this.readRequestId) { this.contents = /* mapped response contents */ }`
2. Do **not** introduce any intermediate `this.contents = []` before the new contents assignment; that would reintroduce the brief empty state.
3. If there is an error branch, leave `this.contents` untouched there so the last successful preview remains visible when a read fails.
</issue_to_address>
### Comment 3
<location path="tests/test_fastapi_v1_dashboard.py" line_range="3097-3106" />
<code_context>
+ assert "SECRET_RESOURCE_PAYLOAD" not in str(log_error.call_args)
+
+
+@pytest.mark.asyncio
+async def test_mcp_resource_pagination_compatibility_error_remains_actionable():
+ client = _make_client()
+ client.list_resources.side_effect = RuntimeError(
+ "The installed MCP SDK does not support resource pagination."
+ )
+ service = _make_service(
+ runtimes={"demo": SimpleNamespace(client=client)},
+ )
+
+ with pytest.raises(ToolsServiceError, match="does not support resource pagination"):
+ await service.list_mcp_resources("demo", "next-page")
</code_context>
<issue_to_address>
**suggestion (testing):** Extend MCP scope tests to cover templates and read endpoints, not just list-resources
Since `/api/v1/mcp/resource-templates` and `/api/v1/mcp/resources/read` are also guarded by `require_mcp_scope`, please add equivalent scope-enforcement tests for these endpoints (forbidden with `tool` scope, allowed with `mcp` scope), reusing the existing test setup where possible. This will keep all MCP resource routes covered and catch any future regressions in auth behavior.
</issue_to_address>
### Comment 4
<location path="astrbot/core/agent/mcp_client.py" line_range="676" />
<code_context>
+ """Whether the connected server advertises MCP resources support."""
+ return bool(self._server_capabilities and self._server_capabilities.resources)
+
+ async def list_resources(
+ self,
+ cursor: str | None = None,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for capability checks, pagination handling, and tools support so the public MCP methods stay short, linear, and easier to reason about.
You can keep the new behavior but reduce cognitive load by:
1. Centralizing capability checks
2. Centralizing pagination compatibility handling
3. Making `list_tools_and_save` independent of `_server_capabilities` branching
### 1. Centralize capability checks
Instead of inlining the same session/resources checks in each method, extract a small helper. This keeps public methods short and focused:
```python
def _ensure_session_and_resources(self, operation: str) -> None:
if not self.session:
raise ValueError(f"MCP session is not available for {operation}.")
if not self.supports_resources:
raise RuntimeError("MCP server does not advertise resources support.")
```
Then your methods shrink to:
```python
async def list_resources(
self, cursor: str | None = None
) -> mcp.types.ListResourcesResult:
self._ensure_session_and_resources("resource listing")
return await self._list_with_optional_cursor(
self.session.list_resources,
cursor,
"resource pagination",
)
```
```python
async def list_resource_templates(
self, cursor: str | None = None
) -> mcp.types.ListResourceTemplatesResult:
self._ensure_session_and_resources("resource template listing")
return await self._list_with_optional_cursor(
self.session.list_resource_templates,
cursor,
"resource template pagination",
)
```
```python
async def read_resource(self, uri: str) -> mcp.types.ReadResourceResult:
self._ensure_session_and_resources("resource reading")
return await self.session.read_resource(uri=uri)
```
### 2. Centralize pagination compatibility handling
The `TypeError`/string-inspection logic appears twice and mixes SDK-compat concerns into each method. You can isolate it in a small helper to make the public methods linear:
```python
async def _list_with_optional_cursor(
self,
list_fn: Callable[..., Awaitable[Any]],
cursor: str | None,
feature_name: str,
):
if cursor is None:
return await list_fn()
try:
return await list_fn(cursor=cursor)
except TypeError as exc:
if "unexpected keyword argument 'cursor'" not in str(exc):
raise
raise RuntimeError(
f"The installed MCP SDK does not support {feature_name}."
) from exc
```
This preserves all current behavior (including the error messages) but keeps the resource methods free from duplicated branching.
### 3. Simplify `list_tools_and_save` semantics
To avoid coupling callers to `_server_capabilities` state and to make the method easier to reason about, move the capability logic behind a `supports_tools` helper and keep `list_tools_and_save` focused on listing + saving:
```python
@property
def supports_tools(self) -> bool:
return bool(self._server_capabilities and self._server_capabilities.tools)
```
Then:
```python
async def list_tools_and_save(self) -> mcp.ListToolsResult:
if not self.session:
raise Exception("MCP Client is not initialized")
if not self.supports_tools:
# keep your "no tools" behavior, but the decision is now centralized
response = mcp.types.ListToolsResult(tools=[])
else:
response = await self.session.list_tools()
self.tools = response.tools
return response
```
This keeps existing functionality (empty tools list when tools capability is absent) while:
- Removing direct capability introspection from the method
- Making the control flow simpler: one early guard, one capability decision, one assignment, one return
If you decide later to surface a more explicit error instead of an empty list, that change would be localized to `supports_tools` or this single `if` branch.
</issue_to_address>
### Comment 5
<location path="astrbot/dashboard/services/tools_service.py" line_range="79" />
<code_context>
+ }
+
+
+def _bounded_mcp_text_preview(
+ text: str,
+ max_preview_bytes: int,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the text preview helper and centralizing resource/annotation serialization mappings, with an optional dedicated pagination error type to reduce brittleness and ease maintenance.
You can trim a noticeable amount of complexity without changing behavior, mainly by:
1. simplifying the text preview logic, and
2. de-duplicating the resource/annotation serialization.
### 1. Simplify `_bounded_mcp_text_preview` and budget handling
You don’t actually need the chunked loop + `_MCP_RESOURCE_TEXT_SIZE_CHUNK_CHARS` to get (a) the total UTF‑8 byte size and (b) a bounded preview. A single `encode()` is enough and keeps the same semantics:
```python
def _bounded_mcp_text_preview(
text: str,
max_preview_bytes: int,
) -> tuple[str, int, bool]:
encoded = text.encode("utf-8")
size = len(encoded)
if size <= max_preview_bytes:
return text, size, False
preview = encoded[:max_preview_bytes].decode("utf-8", errors="ignore")
return preview, size, True
```
The `read_mcp_resource` logic still works as-is:
```python
text, size, truncated = _bounded_mcp_text_preview(text, text_preview_budget)
serialized = {
"type": "text",
"uri": uri,
"mime_type": mime_type,
"text": text,
"size": size,
"truncated": truncated,
}
# unchanged:
text_preview_budget -= len(serialized["text"].encode("utf-8"))
```
This removes the chunking constant and loop while preserving:
- total size in bytes (`size`),
- preview based on a strict byte budget,
- UTF‑8 boundary correctness via `errors="ignore"`,
- and the preview budget semantics.
If large `text` values are a concern, the extra pass is already inherent in the current implementation (through repeated `encode` on chunks), so this is not materially worse.
### 2. Reduce duplication in resource/template serialization
`_serialize_mcp_resource` and `_serialize_mcp_resource_template` are very similar and both use `getattr` with string literals. You can centralize the mapping to make them more declarative and less error‑prone:
```python
def _serialize_mcp_annotations(annotations: object) -> dict[str, Any] | None:
if annotations is None:
return None
if isinstance(annotations, dict):
audience = annotations.get("audience")
priority = annotations.get("priority")
else:
audience = getattr(annotations, "audience", None)
priority = getattr(annotations, "priority", None)
def _normalize_audience(audience: object | None) -> list[str] | None:
if audience is None:
return None
return [str(getattr(item, "value", item)) for item in audience]
serialized: dict[str, Any] = {}
norm_audience = _normalize_audience(audience)
if norm_audience is not None:
serialized["audience"] = norm_audience
if priority is not None:
serialized["priority"] = float(priority)
return serialized
def _serialize_obj_with_annotations(obj: object, mapping: dict[str, str]) -> dict[str, Any]:
data: dict[str, Any] = {}
for target_key, attr_name in mapping.items():
if target_key in ("uri", "name"):
data[target_key] = str(getattr(obj, attr_name))
else:
value = getattr(obj, attr_name, None)
if value is not None:
data[target_key] = value
annotations = getattr(obj, "annotations", None)
data["annotations"] = _serialize_mcp_annotations(annotations)
return data
def _serialize_mcp_resource(resource: object) -> dict[str, Any]:
return _serialize_obj_with_annotations(
resource,
{
"uri": "uri",
"name": "name",
"title": "title",
"description": "description",
"mime_type": "mimeType",
"size": "size",
},
)
def _serialize_mcp_resource_template(template: object) -> dict[str, Any]:
return _serialize_obj_with_annotations(
template,
{
"uri": "uriTemplate",
"name": "name",
"title": "title",
"description": "description",
"mime_type": "mimeType",
},
)
```
This keeps the current duck-typed behavior but:
- removes repeated `getattr` blocks,
- makes the schema differences explicit via the small `mapping` dicts,
- and isolates the “annotations are special” behavior.
### 3. Make error classification less brittle (optional but helpful)
If you can adjust `MCPClient`, you can replace the string‑matching in `_raise_mcp_resource_operation_error` with a dedicated exception type while keeping the helper’s API and call sites intact:
```python
# in mcp_client.py (or similar)
class MCPPaginationNotSupportedError(Exception):
pass
# wherever currently raising those string-matched errors:
raise MCPPaginationNotSupportedError(
"The installed MCP SDK does not support resource pagination."
)
```
Then:
```python
from astrbot.core.agent.mcp_client import MCPPaginationNotSupportedError
def _raise_mcp_resource_operation_error(
operation: str,
server_name: str,
exc: Exception,
) -> NoReturn:
if isinstance(exc, MCPPaginationNotSupportedError):
raise ToolsServiceError(str(exc)) from None
logger.error(
f"Failed to {operation} for MCP server {server_name} ({type(exc).__name__})"
)
raise ToolsServiceError(
f"Failed to {operation} for MCP server {server_name}"
) from None
```
This keeps the outward behavior the same, but removes the coupling to exact error strings and makes the helper easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Last-emo-boy
marked this pull request as draft
July 14, 2026 05:23
Last-emo-boy
changed the title
Contributor
Author
MCP SDK checkpoint: protocol logging reliabilityCommit range: This checkpoint fixes one SDK contract violation without adding a protocol-logging product feature:
Compatibility and risk:
Verification:
The next checkpoint will be Tools pagination and will remain a separate commit. |
added 2 commits
July 15, 2026 04:50
Contributor
Author
MCP SDK checkpoint: Tools pagination and minimum-version CICommits: What changed
Compatibility and scope
Verification
|
Contributor
Author
MCP SDK checkpoint: Prompts core client bridgeCommit: What changed
Control boundary and scope
Verification
|
Contributor
Author
Checkpoint M2b complete: Prompts metadata catalog APICommit: This checkpoint adds one bounded, read-only capability:
Deliberately outside this checkpoint:
Verification:
The branch is based on the latest |
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
1 participant
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.
Supersedes #6136 with a clean, incremental implementation based on the current
masterbranch.This PR is the maintained integration branch for AstrBot's native MCP Python SDK client support. Each milestone is kept independently reviewable, committed separately, and verified before the next one starts. It may be merged at any stable checkpoint; unfinished roadmap items can continue in a follow-up branch without blocking a safe merge.
Integration rules / 接入原则
mcp>=1.8.0,<2); MCP SDK v2 migration is a separate future change.Milestones / 里程碑
resources/list,resources/templates/list, andresources/readclient calls._metafields.tools/listresponses with SDK-version feature detection.mcp==1.8.0in CI.prompts/listandprompts/getclient operations without caching or model exposure.GET /api/v1/mcp/promptsendpoint.POST /api/v1/mcp/prompts/previewwith strict string arguments.user/assistantmessages and known content types; omit binary payloads, annotations, descriptions, icons, and arbitrary metadata.ResourceLinkandstructuredContent; evaluate Audio separately.Later architecture work / 后续独立设计
listChangedcatalog invalidation and safe tool-registry snapshots.Current delivered scope / 当前已交付范围
Protocol-compliant resources-only servers can connect without receiving an unadvertised
tools/listrequest.Resource and template catalogs support pagination, metadata, plain-text preview, and stale-request protection.
Tool catalogs consume all available pages on capable SDKs, with bounded fail-soft fallback to the legacy first page when continuation is unsupported or invalid.
Prompt-capable servers have explicit core client operations for discovery and retrieval; neither operation is called automatically, cached, transformed, or registered as a Tool.
MCP-scope clients can explicitly list Prompt metadata through
/api/v1/mcp/promptswithout callingprompts/get.MCP-scope clients can explicitly preview a selected Prompt with arguments through
/api/v1/mcp/prompts/preview; untrusted content is returned only as a bounded, allowlisted DTO and is never auto-sent.Preview state is cleared when the resource dialog closes; resource reads avoid auto-reconnect races.
Server error paths are sanitized so malformed MCP payloads do not leak through API errors or logs.
OpenAPI sources, generated clients, public API docs, MCP user docs, and en-US/zh-CN/ru-RU translations are updated for their delivered milestones.
This is NOT a breaking change. / 这不是一个破坏性变更。
Verification / 验证结果
Completed milestones through
c35261866:bash ./scripts/run_pytests_ci.sh ./tests→1856 passed, 5 warnings.39 passed.mcp==1.8.0→72 passed; the lockedmcp==1.28.1environment was restored afterward.MCPClient.get_prompt()→ authenticated FastAPI route →200 OK.uv run --no-sync ruff check .→ passed.uv run --no-sync ruff format --check .→480 files already formatted.vue-tsc --noEmit→ passed.vite build→ passed.0commits behind currentorigin/masterbefore the checkpoint commit.c35261866: all 20 checks passed.Checklist / 检查清单