From 5a2a8b217612a21f9626091ba6c2cb9a6bc738f3 Mon Sep 17 00:00:00 2001 From: bradAGI <46579244+bradAGI@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:07:36 -0400 Subject: [PATCH] test(rules): mirror and cover MCP-025, MCP-026 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#88, 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-026 pairs description_length_lt with has_docstring so an absent docstring stays MCP-001's finding instead of double-reporting — an empty description is length 0, which is also under the threshold — and the fifth case is what pins that guard. --- internal/rules/policies_test.go | 25 ++++++- .../rules-fixture/mcp/tool_definition.yaml | 65 +++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/internal/rules/policies_test.go b/internal/rules/policies_test.go index 65af6c0a..66f02d8a 100644 --- a/internal/rules/policies_test.go +++ b/internal/rules/policies_test.go @@ -1986,6 +1986,30 @@ def fetch_data(x: str) -> dict: return {} `, toolConfig: nil, wantFires: false}, + {name: "MCP-025 fires on placeholder description", ruleID: "MCP-025", kind: models.KindMCPTool, src: ` +def lookup_order(order_id: str) -> str: + """TODO: describe this tool.""" + return order_id +`, wantFires: true}, + {name: "MCP-025 silent on a real description", ruleID: "MCP-025", kind: models.KindMCPTool, src: ` +def lookup_order(order_id: str) -> str: + """Look up a single order by its identifier and return its current status.""" + return order_id +`, wantFires: false}, + {name: "MCP-026 fires on a too-short description", ruleID: "MCP-026", kind: models.KindMCPTool, src: ` +def list_orders(customer_id: str) -> str: + """Gets data.""" + return customer_id +`, wantFires: true}, + {name: "MCP-026 silent on a full description", ruleID: "MCP-026", kind: models.KindMCPTool, src: ` +def list_orders(customer_id: str) -> str: + """List every order belonging to one customer, most recent first.""" + return customer_id +`, wantFires: false}, + {name: "MCP-026 silent when the docstring is absent (MCP-001's case)", ruleID: "MCP-026", kind: models.KindMCPTool, src: ` +def list_orders(customer_id: str) -> str: + return customer_id +`, wantFires: false}, } // policyRepoRuleCases covers repo-scoped rules. @@ -2246,7 +2270,6 @@ var policyRepoRuleCases = []policyRepoCase{ }, models.RepoInventory{SDKsDetected: []models.SDK{models.SDKOpenAIAgents}}, false}, - } // optionsWithPermissionMode builds a ClaudeAgentOptionsDef whose captured diff --git a/testdata/rules-fixture/mcp/tool_definition.yaml b/testdata/rules-fixture/mcp/tool_definition.yaml index 1e843121..d7d27306 100644 --- a/testdata/rules-fixture/mcp/tool_definition.yaml +++ b/testdata/rules-fixture/mcp/tool_definition.yaml @@ -309,3 +309,68 @@ rules: fix: > Rename the method (or set the `#[tool]` `name = "..."` argument) to a verb-object form, e.g. `summarize_invoice`, `fetch_weather`. + + - id: MCP-025 + title: MCP tool description is a placeholder + severity: low + confidence: 0.85 + language: python + applies_to: + - mcp_tool + scope: tool + match: + has_description_text: + - todo + - tbd + - fixme + - placeholder + - no description + - does stuff + explanation: > + The docstring passes the MCP-001 has-a-description check but carries a + placeholder marker instead of real content. The server advertises this + string 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 a stub like "TODO: describe this tool" is functionally + indistinguishable from no description at all. A placeholder is worse here + than in an in-process SDK for two reasons. The consumer has no other + context to fall back on: it cannot read the source, the neighboring tools, + or the project's docs the way a developer working in the repo can. And the + server's authors never see the consequence, because mis-selection shows up + in someone else's client session as a wrong answer, not 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-026 + title: MCP tool description is too short to guide model selection + severity: low + confidence: 0.8 + language: python + 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 ambiguity is not confined to + this server's own tools, either: a client typically connects several + servers at once and the model chooses 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.