From 9a0f0bcf4070bedcf3f1f94ed1d976052fc5beaa Mon Sep 17 00:00:00 2001 From: bradAGI <46579244+bradAGI@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:09:14 -0400 Subject: [PATCH] test(rules): mirror and cover MCP-027, MCP-028 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Engine half of a coordinated pair with trustabl/trustabl-rules#91, on a branch of the same name so the rules-sync job resolves the matching pack rather than main. Neither half should merge alone — check-rules-sync.sh fails if they do. Mirrors mcp/tool_definition.yaml into testdata/rules-fixture and adds cases to policyRuleCases, as TestPolicyRules_AllRulesCovered requires. Five cases rather than four. MCP-028 pairs description_length_lt with has_docstring so an empty description stays MCP-011's finding instead of double-reporting, and the fifth case pins that guard. Note for sequencing: rules#88 (MCP-025/026, the Python pair) appends to the same file, so whichever of the two pairs lands second needs a trivial rebase in both repos — the rule blocks and the test cases are independent and the resolution is "keep both". --- internal/rules/policies_test.go | 37 +++++++++++ .../rules-fixture/mcp/tool_definition.yaml | 64 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/internal/rules/policies_test.go b/internal/rules/policies_test.go index d8df59f5..9a42aea7 100644 --- a/internal/rules/policies_test.go +++ b/internal/rules/policies_test.go @@ -2233,6 +2233,43 @@ def get_note_meta(note_id: str) -> dict: " return \"n1\";\n" + "}, { name: \"save_note\", description: \"Save a note.\", schema: {} });\n", }, + + // ─── MCP-027 / MCP-028: TS MCP description quality ────────────────────── + { + name: "MCP-027 fires on placeholder description", ruleID: "MCP-027", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: true, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const s = new McpServer({ name: \"orders\", version: \"1.0.0\" });\n" + + "s.tool(\"lookup_order\", \"TODO: describe this tool.\", {}, async () => ({ content: [] }));\n", + }, + { + name: "MCP-027 silent on a real description", ruleID: "MCP-027", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: false, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const s = new McpServer({ name: \"orders\", version: \"1.0.0\" });\n" + + "s.tool(\"lookup_order\", \"Look up one order by its number and return its fulfillment status.\", {}, async () => ({ content: [] }));\n", + }, + { + name: "MCP-028 fires on a too-short description", ruleID: "MCP-028", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: true, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const s = new McpServer({ name: \"orders\", version: \"1.0.0\" });\n" + + "s.tool(\"list_orders\", \"Gets data.\", {}, async () => ({ content: [] }));\n", + }, + { + name: "MCP-028 silent on a full description", ruleID: "MCP-028", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: false, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const s = new McpServer({ name: \"orders\", version: \"1.0.0\" });\n" + + "s.tool(\"list_orders\", \"List every order belonging to one customer, most recent first.\", {}, async () => ({ content: [] }));\n", + }, + { + name: "MCP-028 silent when the description is absent (MCP-011's case)", ruleID: "MCP-028", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: false, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const s = new McpServer({ name: \"orders\", version: \"1.0.0\" });\n" + + "s.tool(\"list_orders\", \"\", {}, async () => ({ content: [] }));\n", + }, } // policyRepoRuleCases covers repo-scoped rules. diff --git a/testdata/rules-fixture/mcp/tool_definition.yaml b/testdata/rules-fixture/mcp/tool_definition.yaml index 1e843121..aef3bbbb 100644 --- a/testdata/rules-fixture/mcp/tool_definition.yaml +++ b/testdata/rules-fixture/mcp/tool_definition.yaml @@ -309,3 +309,67 @@ rules: fix: > Rename the method (or set the `#[tool]` `name = "..."` argument) to a verb-object form, e.g. `summarize_invoice`, `fetch_weather`. + + - id: MCP-027 + title: TypeScript MCP tool description is a placeholder + severity: low + confidence: 0.85 + language: typescript + applies_to: + - mcp_tool + scope: tool + match: + has_description_text: + - todo + - tbd + - fixme + - placeholder + - no description + - does stuff + explanation: > + The tool registration sets a description, so it passes MCP-011, but the + string is a placeholder rather than real content. The server publishes it + across the protocol boundary in its tools/list response, so it is the + entire account of the tool that every connecting client and model + receives, and "TODO: describe this tool" tells them nothing the tool name + did not. The consumer has no fallback: it cannot read this repo's source, + the neighboring tools, or the project's docs. The server's authors do not + see the result either, because mis-selection surfaces in someone else's + client session as a wrong answer rather than as an error on this side of + the connection. + fix: > + Replace the placeholder with a real description covering what the tool + does, what it returns, and when a model should call it rather than a + neighboring tool on this server. Write it for a reader with no other + knowledge of the server, since that is what a connecting client is. + + - id: MCP-028 + title: TypeScript MCP tool description is too short to guide model selection + severity: low + confidence: 0.8 + language: typescript + applies_to: + - mcp_tool + scope: tool + match: + all: + - has_docstring: true + - description_length_lt: 40 + explanation: > + A description under 40 characters is rarely enough to convey what a tool + does, what it returns, and when to call it rather than a similarly named + neighbor. It is the whole of what the server publishes about the tool in + tools/list, so a stub like "Gets data." leaves scope and preconditions to + guesswork for every client that connects. The Zod input schema does not + close the gap — it constrains the arguments once the model has chosen this + tool, not whether choosing it was right. The ambiguity also is not + confined to this server: a client typically connects several at once and + the model picks across all of them from these strings alone, so a thin + description competes badly against a well-described tool from an unrelated + server that only approximately fits the request. + fix: > + Expand the description to at least a full sentence covering inputs, + outputs, and the situation in which this tool should be used over the + alternatives. Name the domain the tool acts on rather than assuming the + server name conveys it, since the model sees the tool alongside those of + every other connected server.