mirror of
https://github.com/kevinveenbirkenbach/computer-playbook.git
synced 2026-11-06 10:59:59 +00:00
feat(mcp): offer no client a tool its provider calls mutating
Both agents registered every tool every provider served. Hermes documents that as its own default in tools/mcp_tool.py, "Neither set -> register all tools", and the role never set tools.include; OpenClaw's toolFilter was never rendered at all. A provider that grew a mutating tool would have reached both without anything declaring it. A serving role now names its mutating tools, the discovery snapshot carries that set beside the allowlist, and every client renders the allowlist minus it in whatever form its upstream enforces. The subtraction sits at the client and not in discovery because Flowise compares the declared set against what its registry actually discovered and needs the full served list. Where a provider permits mutations there is nothing to withhold, so the set renders empty. This is a client-side boundary, not a server-side one: whoever holds the endpoint key can still call a write tool directly. Baserow is the case that made it necessary, since its MCPEndpoint model carries no scope and its owner is a workspace administrator, so the declared mutating_tools_enabled bounds nothing there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
63a37dac45
commit
ec736bd4d7
9 files changed
+302
-22
No files matched your search
@@ -236,6 +236,8 @@ def build_mcp_discovery(
|
||||
"auth": auth,
|
||||
"auth_subject": server.get("auth_subject"),
|
||||
"owner": owner,
|
||||
"tools": list(server.get("tools") or []),
|
||||
"mutating": list(server.get("mutating") or []),
|
||||
"transport": transport.replace("_", "-"),
|
||||
}
|
||||
)
|
||||
|
||||
@@ -198,6 +198,14 @@ class LookupModule(LookupBase):
|
||||
"key": credential.get("key"),
|
||||
}
|
||||
entry["allowed_consumers"] = list(block.get("allowed_consumers") or [])
|
||||
tools = block.get("tools")
|
||||
tools = tools if isinstance(tools, dict) else {}
|
||||
entry["tools"] = list(tools.get("allowlist") or [])
|
||||
entry["mutating"] = (
|
||||
[]
|
||||
if tools.get("mutating_tools_enabled")
|
||||
else list(tools.get("mutating") or [])
|
||||
)
|
||||
entry["supported_transports"] = list(
|
||||
block.get("supported_transports") or []
|
||||
)
|
||||
|
||||
@@ -35,5 +35,9 @@ tools:
|
||||
- list_table_rows
|
||||
- list_tables
|
||||
- update_rows
|
||||
mutating:
|
||||
- create_rows
|
||||
- delete_rows
|
||||
- update_rows
|
||||
read_only_default: true
|
||||
mutating_tools_enabled: false
|
||||
@@ -12,4 +12,8 @@ mcp_servers:{{ ' {}' if renderable | length == 0 else '' }}
|
||||
tools:
|
||||
resources: true
|
||||
prompts: false
|
||||
{% set callable_tools = server.tools | reject('in', server.mutating | default([])) | list %}
|
||||
{% if callable_tools %}
|
||||
include: {{ callable_tools | to_json }}
|
||||
{% endif %}
|
||||
{% endfor %}
|
||||
@@ -4,14 +4,32 @@
|
||||
"controlUi": {
|
||||
"allowedOrigins": [{{ OPENCLAW_URL | to_json }}]
|
||||
}
|
||||
}{% set renderable = OPENCLAW_MCP_SERVERS | mcp_renderable_servers %}{% if renderable | length > 0 %},
|
||||
}{% if OPENCLAW_LITELLM_ENABLED | bool %},
|
||||
"agents": {
|
||||
"defaults": {
|
||||
"model": {"primary": {{ (OPENCLAW_LITELLM_PROVIDER ~ '/' ~ OPENCLAW_LITELLM_MODEL) | to_json }}}
|
||||
}
|
||||
},
|
||||
"models": {
|
||||
"providers": {
|
||||
{{ OPENCLAW_LITELLM_PROVIDER | to_json }}: {
|
||||
"baseUrl": {{ OPENCLAW_LITELLM_BASE_URL | to_json }},
|
||||
"apiKey": {{ OPENCLAW_LITELLM_API_KEY | to_json }},
|
||||
"api": "openai-completions",
|
||||
"models": [{"id": {{ OPENCLAW_LITELLM_MODEL | to_json }}}]
|
||||
}
|
||||
}
|
||||
}
|
||||
{% endif %}{% set renderable = OPENCLAW_MCP_SERVERS | mcp_renderable_servers %}{% if renderable | length > 0 %},
|
||||
"mcp": {
|
||||
"servers": {
|
||||
{% for server in renderable %}
|
||||
{% for server in renderable %}{% set callable_tools = server.tools | reject('in', server.mutating | default([])) | list %}
|
||||
{{ server.id | to_json }}: {
|
||||
"url": {{ server.url | to_json }},
|
||||
"transport": {{ server.transport | to_json }},
|
||||
"headers": {"Authorization": {{ server | mcp_authorization | to_json }}}
|
||||
"headers": {"Authorization": {{ server | mcp_authorization | to_json }}}{% if callable_tools %},
|
||||
"toolFilter": {"include": {{ callable_tools | to_json }}}{% endif %}
|
||||
|
||||
}{{ "," if not loop.last }}
|
||||
{% endfor %}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,94 @@
|
||||
"""A surface that serves mutating tools has to name them.
|
||||
|
||||
``mutating_tools_enabled: false`` is a claim, not a mechanism. Where the
|
||||
upstream cannot scope the credential it issues, nothing refuses the call, and
|
||||
the only remaining boundary is the include list every client renders. That list
|
||||
is built by subtracting ``tools.mutating`` from ``tools.allowlist``, so a
|
||||
mutating tool that is served but not named is offered to every client while the
|
||||
declaration says mutations are off.
|
||||
|
||||
Suppression (see ``docs/contributing/actions/testing/suppression.md``):
|
||||
|
||||
* ``# nocheck: mcp-mutating-tools-named`` in the head of the role's
|
||||
``meta/mcp.yml``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import unittest
|
||||
from collections.abc import Mapping
|
||||
|
||||
from utils.annotations.suppress import is_suppressed_in_head
|
||||
from utils.cache.files import read_text
|
||||
from utils.cache.yaml import load_yaml_any
|
||||
from utils.roles.mapping import ROLE_FILE_META_MCP
|
||||
|
||||
from . import PROJECT_ROOT
|
||||
|
||||
_RULE = "mcp-mutating-tools-named"
|
||||
_SERVING = {"server", "both"}
|
||||
|
||||
|
||||
def _surfaces() -> list[tuple[str, Mapping]]:
|
||||
"""Return ``(role, mcp block)`` for every role declaring an MCP surface."""
|
||||
found = []
|
||||
for path in sorted((PROJECT_ROOT / "roles").glob(f"*/{ROLE_FILE_META_MCP}")):
|
||||
if is_suppressed_in_head(read_text(str(path)).splitlines(), _RULE):
|
||||
continue
|
||||
block = load_yaml_any(str(path), default_if_missing={})
|
||||
if isinstance(block, Mapping):
|
||||
found.append((path.parent.parent.name, block))
|
||||
return found
|
||||
|
||||
|
||||
class TestMcpMutatingToolsNamed(unittest.TestCase):
|
||||
def test_named_mutating_tools_are_part_of_the_allowlist(self) -> None:
|
||||
stray = []
|
||||
for role, block in _surfaces():
|
||||
tools = block.get("tools")
|
||||
if not isinstance(tools, Mapping):
|
||||
continue
|
||||
allowlist = set(tools.get("allowlist") or [])
|
||||
outside = sorted(set(tools.get("mutating") or []) - allowlist)
|
||||
if outside:
|
||||
stray.append(
|
||||
f"{role}: names {outside} as mutating, but the allowlist "
|
||||
f"does not serve them, so the subtraction removes nothing "
|
||||
f"and the name is stale"
|
||||
)
|
||||
self.assertEqual(
|
||||
[],
|
||||
stray,
|
||||
"mutating name(s) outside the served set:\n"
|
||||
+ "\n".join(f" - {s}" for s in stray),
|
||||
)
|
||||
|
||||
def test_a_surface_that_permits_mutations_names_none(self) -> None:
|
||||
contradictory = []
|
||||
for role, block in _surfaces():
|
||||
tools = block.get("tools")
|
||||
if not isinstance(tools, Mapping):
|
||||
continue
|
||||
if tools.get("mutating_tools_enabled") and tools.get("mutating"):
|
||||
contradictory.append(
|
||||
f"{role}: permits mutations yet still names {sorted(tools['mutating'])} "
|
||||
f"as withheld; a reader cannot tell which half is meant"
|
||||
)
|
||||
self.assertEqual(
|
||||
[],
|
||||
contradictory,
|
||||
"surface(s) both permitting and withholding mutation:\n"
|
||||
+ "\n".join(f" - {c}" for c in contradictory),
|
||||
)
|
||||
|
||||
def test_the_scan_finds_surfaces(self) -> None:
|
||||
self.assertTrue(
|
||||
[role for role, block in _surfaces() if block.get("direction") in _SERVING],
|
||||
"no serving MCP surface was read, so every rule here would pass "
|
||||
"vacuously; check that the scan still reaches the roles' MCP "
|
||||
"declarations",
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -0,0 +1,3 @@
|
||||
from pathlib import Path
|
||||
|
||||
PROJECT_ROOT: Path = Path(__file__).resolve().parents[5]
|
||||
@@ -0,0 +1,165 @@
|
||||
"""The agent clients register only the tools their providers declare.
|
||||
|
||||
Both upstreams default to registering every tool an MCP server happens to
|
||||
serve. Hermes says so in ``tools/mcp_tool.py``: "Neither set -> register all
|
||||
tools (backward-compatible default)". OpenClaw's ``toolFilter`` is absent
|
||||
unless written. A provider that grows a mutating tool would therefore reach
|
||||
both agents without anyone declaring it, so the rendered configuration has to
|
||||
carry the allowlist rather than rely on the default.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib.util
|
||||
import json
|
||||
import unittest
|
||||
|
||||
import yaml
|
||||
from jinja2 import Environment, FileSystemLoader
|
||||
|
||||
from . import PROJECT_ROOT
|
||||
|
||||
HERMES = PROJECT_ROOT / "roles/web-app-hermes/templates"
|
||||
OPENCLAW = PROJECT_ROOT / "roles/web-app-openclaw/templates"
|
||||
FILTERS = PROJECT_ROOT / "plugins/filter/mcp/authorization.py"
|
||||
|
||||
SERVERS = [
|
||||
{
|
||||
"id": "svc-db-qdrant",
|
||||
"url": "http://qdrantmcp:8080/mcp",
|
||||
"token": "q" * 40,
|
||||
"auth": "bearer_token",
|
||||
"owner": "mcp-svc-db-qdrant",
|
||||
"tools": ["qdrant_list_collections", "qdrant_get_collection"],
|
||||
"transport": "streamable-http",
|
||||
},
|
||||
{
|
||||
"id": "web-app-prometheus",
|
||||
"url": "http://prometheusmcp:8080/mcp",
|
||||
"token": "p" * 40,
|
||||
"auth": "bearer_token",
|
||||
"owner": "mcp-web-app-prometheus",
|
||||
"tools": [],
|
||||
"mutating": [],
|
||||
"transport": "streamable-http",
|
||||
},
|
||||
{
|
||||
"id": "web-app-baserow",
|
||||
"url": "http://baserow:8080/mcp",
|
||||
"token": "b" * 40,
|
||||
"auth": "bearer_token",
|
||||
"owner": "mcp-web-app-baserow",
|
||||
"tools": ["list_databases", "list_tables", "create_rows", "delete_rows"],
|
||||
"mutating": ["create_rows", "delete_rows"],
|
||||
"transport": "streamable-http",
|
||||
},
|
||||
]
|
||||
|
||||
|
||||
def _filters() -> dict:
|
||||
spec = importlib.util.spec_from_file_location("mcp_authorization", FILTERS)
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(module)
|
||||
return {
|
||||
"mcp_authorization": module.mcp_authorization,
|
||||
"mcp_renderable_servers": module.mcp_renderable_servers,
|
||||
"to_json": json.dumps,
|
||||
"bool": bool,
|
||||
}
|
||||
|
||||
|
||||
def _yaml(text: str):
|
||||
"""Parse a rendered template.
|
||||
|
||||
Args:
|
||||
text: the rendered document.
|
||||
|
||||
The project's YAML cache is keyed by path; this input never becomes a file.
|
||||
"""
|
||||
return yaml.safe_load(text) # nocheck: direct-yaml a render, not a file
|
||||
|
||||
|
||||
def _render(directory, name, **context) -> str:
|
||||
env = Environment(loader=FileSystemLoader(str(directory)), autoescape=False) # noqa: S701 - renders configuration, not markup
|
||||
env.filters.update(_filters())
|
||||
return env.get_template(name).render(**context)
|
||||
|
||||
|
||||
class TestAgentToolFilter(unittest.TestCase):
|
||||
def test_hermes_pins_the_declared_allowlist(self) -> None:
|
||||
rendered = _yaml(_render(HERMES, "config.yaml.j2", HERMES_MCP_SERVERS=SERVERS))
|
||||
entry = rendered["mcp_servers"]["svc_db_qdrant"]
|
||||
self.assertEqual(
|
||||
["qdrant_list_collections", "qdrant_get_collection"],
|
||||
entry["tools"]["include"],
|
||||
)
|
||||
|
||||
def test_hermes_omits_the_key_when_no_allowlist_is_declared(self) -> None:
|
||||
rendered = _yaml(_render(HERMES, "config.yaml.j2", HERMES_MCP_SERVERS=SERVERS))
|
||||
entry = rendered["mcp_servers"]["web_app_prometheus"]
|
||||
self.assertNotIn(
|
||||
"include",
|
||||
entry["tools"],
|
||||
"an empty include list would register nothing at all, which reads "
|
||||
"as a working client serving no tools",
|
||||
)
|
||||
|
||||
def test_openclaw_pins_the_declared_allowlist(self) -> None:
|
||||
rendered = json.loads(
|
||||
_render(
|
||||
OPENCLAW,
|
||||
"openclaw.json.j2",
|
||||
OPENCLAW_URL="https://claw.example.org",
|
||||
OPENCLAW_LITELLM_ENABLED=False,
|
||||
OPENCLAW_MCP_SERVERS=SERVERS,
|
||||
)
|
||||
)
|
||||
entry = rendered["mcp"]["servers"]["svc-db-qdrant"]
|
||||
self.assertEqual(
|
||||
["qdrant_list_collections", "qdrant_get_collection"],
|
||||
entry["toolFilter"]["include"],
|
||||
)
|
||||
|
||||
def test_openclaw_stays_valid_json_with_and_without_the_filter(self) -> None:
|
||||
rendered = json.loads(
|
||||
_render(
|
||||
OPENCLAW,
|
||||
"openclaw.json.j2",
|
||||
OPENCLAW_URL="https://claw.example.org",
|
||||
OPENCLAW_LITELLM_ENABLED=False,
|
||||
OPENCLAW_MCP_SERVERS=SERVERS,
|
||||
)
|
||||
)
|
||||
self.assertNotIn("toolFilter", rendered["mcp"]["servers"]["web-app-prometheus"])
|
||||
self.assertEqual(
|
||||
{"svc-db-qdrant", "web-app-prometheus", "web-app-baserow"},
|
||||
set(rendered["mcp"]["servers"]),
|
||||
)
|
||||
|
||||
def test_hermes_never_offers_a_tool_the_provider_calls_mutating(self) -> None:
|
||||
rendered = _yaml(_render(HERMES, "config.yaml.j2", HERMES_MCP_SERVERS=SERVERS))
|
||||
include = rendered["mcp_servers"]["web_app_baserow"]["tools"]["include"]
|
||||
self.assertEqual(["list_databases", "list_tables"], include)
|
||||
|
||||
def test_openclaw_never_offers_a_tool_the_provider_calls_mutating(self) -> None:
|
||||
rendered = json.loads(
|
||||
_render(
|
||||
OPENCLAW,
|
||||
"openclaw.json.j2",
|
||||
OPENCLAW_URL="https://claw.example.org",
|
||||
OPENCLAW_LITELLM_ENABLED=False,
|
||||
OPENCLAW_MCP_SERVERS=SERVERS,
|
||||
)
|
||||
)
|
||||
entry = rendered["mcp"]["servers"]["web-app-baserow"]
|
||||
self.assertEqual(
|
||||
["list_databases", "list_tables"], entry["toolFilter"]["include"]
|
||||
)
|
||||
|
||||
def test_neither_agent_inlines_a_bearer_for_a_bearer_provider(self) -> None:
|
||||
hermes = _render(HERMES, "config.yaml.j2", HERMES_MCP_SERVERS=SERVERS)
|
||||
self.assertNotIn("q" * 40, hermes)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -136,25 +136,6 @@ MCP_SPECIFICATION_ADAPTER_TYPES = frozenset({"openapi_allowlist", "graphql_allow
|
||||
|
||||
MCP_CREDENTIAL_SOURCES = frozenset({"token_store", "credentials"})
|
||||
|
||||
MCP_SERVED_KEYS = frozenset(
|
||||
{
|
||||
"direction",
|
||||
"transport",
|
||||
"exposure",
|
||||
"auth",
|
||||
"auth_subject",
|
||||
"delegation",
|
||||
"credential",
|
||||
"allowed_consumers",
|
||||
"supported_transports",
|
||||
"supported_auths",
|
||||
"endpoint",
|
||||
"adapter",
|
||||
"limits",
|
||||
"tools",
|
||||
}
|
||||
)
|
||||
|
||||
MCP_KEYS = frozenset(
|
||||
{
|
||||
"enabled",
|
||||
@@ -236,6 +217,7 @@ MCP_LIMITS_KEYS = frozenset(
|
||||
MCP_TOOLS_KEYS = frozenset(
|
||||
{
|
||||
"allowlist",
|
||||
"mutating",
|
||||
"writer_allowlist",
|
||||
"upstream_serves",
|
||||
"categories",
|
||||
|
||||
Reference in new issue
Block a user