Repository navigation
feat(core): keep the MCP tool display title reachable - #3330
King52HerTz wants to merge 2 commits into
Conversation
MCP defines Tool.title as the human-readable name for display purposes, with ToolAnnotations.title as an alternative. McpClientManager built each McpTool from name, description, inputSchema, outputSchema and readOnlyHint, so the title was dropped at registration and no caller could recover it without a second listTools round trip through framework internals. Surface it as AgentTool.getTitle(), defaulting to null the way getStrict() and getOutputSchema() already do, carry it on ToolBase, and resolve title-or-annotation-fallback during registration. The existing McpTool constructors delegate with a null title, so servers that send none and callers that pre-date this are unchanged.
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
PR #3330 — feat(core): keep the MCP tool display title reachable. Nice catch: the MCP tool title was parsed but never surfaced, so permission prompts fall back to the namespaced id. Two things to address: the one production line that wires the title is untested, and the title is an untrusted server-provided string that is about to be shown to a human approving a tool call.
Findings
- [Warning]
agentscope-core/src/main/java/io/agentscope/core/tool/McpClientManager.java:326— the only line that actually delivers the title has no coverage; registration tests mocktitle()unstubbed. - [Warning]
agentscope-core/src/main/java/io/agentscope/core/tool/mcp/McpTool.java:408— server-controlled display label passed through verbatim; needs control-character stripping + length cap before it reaches a permission prompt. - [Info]
agentscope-core/src/test/java/io/agentscope/core/tool/mcp/McpToolTitleTest.java:51— non-English fixture literals against the repo's code-review convention.
CLA note
license/cla is pending on this head commit, so this is a comment-only review (no approval) — @King52HerTz please sign the CLA.
Automated review by github-manager-bot
| mcpClientWrapper.getName(), | ||
| readOnly); | ||
| readOnly, | ||
| McpTool.resolveDisplayTitle(mcpTool)); |
There was a problem hiding this comment.
[Warning] This is the only production line that actually delivers the title, but no test covers it: McpToolTitleTest exercises resolveDisplayTitle(...) and the new constructor directly, while the existing registration tests mock McpSchema.Tool with title()/annotations() unstubbed. A regression here (title dropped, or passed into the wrong positional parameter) would leave getTitle() null with the whole suite green. Suggest stubbing tool.title() in McpNamespacedToolTest, which already registers through this exact path (toolkit.registration().mcpClient(client).mcpToolNamePrefix(...).apply()), and asserting toolkit.getTool("crm__search").getTitle().
| } | ||
| String title = tool.title(); | ||
| if (title != null && !title.isBlank()) { | ||
| return title; |
There was a problem hiding this comment.
[Warning] resolveDisplayTitle returns the server-supplied string verbatim. Per the javadoc above it, this value is rendered to an end user (e.g. as the label on a permission confirmation), and MCP servers are third-party/untrusted: ANSI/terminal control sequences or bidi overrides in a title can spoof or hide the operation being approved, and the MCP spec sets no length limit, so an oversized title can flood the prompt. Please harden at this single trust boundary — strip Character.isISOControl characters, collapse whitespace, and cap the length (~120 chars) — so AgentTool.getTitle() only ever hands out a display-safe label.
| McpSchema.Tool tool = | ||
| new McpSchema.Tool( | ||
| "create_ticket", | ||
| "提交报修工单", |
There was a problem hiding this comment.
[Info] Minor repo-convention nit: the new fixture hardcodes non-English literals (提交报修工单), and the project's code-review instructions disallow non-English content outside i18n resources. The behaviour under test (prefer Tool.title, fall back to ToolAnnotations.title, reject blanks) is charset-independent, so ASCII literals would keep the test aligned with the convention. If non-ASCII pass-through is genuinely part of the contract, please say so in the test name/comment so the exception is explicit.
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @King52HerTz please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
…path Review follow-up for agentscope-ai#3330. The title is a string the MCP server chose and we show it to a human who is about to approve an action, so it now passes one normalisation step wherever it enters a McpTool: zero-width and directional-format characters are dropped, whitespace and C0/C1 controls collapse to a single space, and the value is capped at 120 characters (the width already used for one-line tool labels in AgentTraceMiddleware) without ever cutting a surrogate pair in half. Nothing printable left means null, so callers fall back to getName() rather than showing an empty label. Both entry points are guarded, not just registration, because extensions construct McpTool directly. The registration tests mocked McpSchema.Tool and stubbed only name(), description() and inputSchema(), so title() sat at null and the line that actually delivers the title never ran with a value under test. Four cases now drive a real tool definition through registerMcpClient and assert on the registered AgentTool: tool title, annotation fallback, hostile title, and a server that sends none. Verified by mutation: dropping resolveDisplayTitle from registration fails three of them, and removing either sanitise call site fails the ones that guard it.
|
Both points are addressed in 1b6c235, plus the 1. You were right about the cause: every registration test mocked
2.
The guard sits in both entry points — Also took the Evidence. Mutation, not just green-on-first-run: replacing the registration call with |
This branch has not been deployed
3 participants
Problem
MCP defines
Tool.titleas the human-readable name for display purposes, and allows the same value underToolAnnotations.title.McpClientManagerbuilt eachMcpToolfromname,description,inputSchema,outputSchemaandannotations.readOnlyHint— the title was dropped at registration and is not reachable fromAgentTool/Toolkitafterwards.name(repair__create_ticket) is a programmatic identifier anddescriptionis model-facing guidance, so a caller rendering a permission confirmation has nothing suitable to show an end user.Change
AgentTool.getTitle()— a default method returningnull, following the same shape as the existinggetStrict()andgetOutputSchema()defaults, so it is additive and no implementer must change.ToolBasecarries the value;ToolBase.Builder.title(...)sets it.McpTool.resolveDisplayTitle(McpSchema.Tool)prefersTool.titleand falls back toToolAnnotations.title, ignoring blank values.McpClientManagerresolves it at registration.The existing
McpToolconstructors stay and delegate with anulltitle, so a new 10-argument constructor carries it instead — servers that send no title, and callers that pre-date this, behave exactly as before.Deliberately not included
The issue also floats adding
toolTitleto the AG-UI permission interrupt next totoolName/toolInput. That is a second wire format and a second set of consumers, so I left it out to keep this reviewable; oncegetTitle()exists on the tool the AG-UI side is a one-line addition if maintainers want it.Local
@Toolmethods returnnullfor the title, as the issue suggested they may.Testing
McpToolTitleTestcovers title precedence, the annotation fallback, blank values, anulltool definition, reachability through theAgentToolinterface (which is the widthToolkithands to callers), and that a title-less registration does not throw.Partially addresses #3316