From 28127105767bcbc050b8f4f94a3bb35c539b4bb3 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 6 Aug 2026 10:54:50 +0200 Subject: [PATCH] feat(mcp): add custom tool profile config models The declarative surface for custom tool profiles: what an author may put in a `custom_tools:` YAML entry, and every way that entry can be wrong. A profile is `search-records` with some arguments frozen by the author and the rest still exposed. `lock` holds what the author decides;`params` holds the exposure and cap policy for what the model may still pass. Anything unlisted stays exposed, so a profile that only locks a filter keeps the rest of the built-in's contract. A locked projection is the one implicit case: locking it means the model cannot also choose one. Nothing consumes these models yet -- registration and execution land in the next commit in this stack. That ordering is deliberate: this is the complete authoring contract and it is fully testable on its own, so it reads before the machine that honors it. It must not reach main by itself, though. A `custom_tools:` block would validate here and then register nothing, with no warning -- exactly the silent-misconfiguration failure these models otherwise guard against. Validation is front-loaded to config load wherever the information exists there, since the alternative is a tool that looks locked and enforces nothing. Covered: a name that collides with a built-in, uses a reserved `redisvl-`/`redisvl_` prefix (both separators, since the pattern permits either), or falls outside the character set MCP hosts commonly accept; a duplicate name; a missing `index` when several bindings exist, or one naming an unknown binding; a `params` key that is not a real argument; `max` on anything but `limit`; a cap above the bound index's own `runtime.max_limit`, which could never be satisfied; hiding `query`, which a search profile needs; and locking `return_fields` while also exposing them. Field checks that need the inspected schema stay at startup. All three models set `extra="forbid"`. A misspelled key would otherwise be dropped in silence, which is the worst outcome available here: `lock: {return_field: [...]}` would read as a locked projection while enforcing nothing at all. --- redisvl/mcp/config.py | 225 ++++++++++++++++++++ tests/unit/test_mcp/test_config.py | 324 +++++++++++++++++++++++++++++ 2 files changed, 549 insertions(+) diff --git a/redisvl/mcp/config.py b/redisvl/mcp/config.py index f40d7f10..e459a0a9 100644 --- a/redisvl/mcp/config.py +++ b/redisvl/mcp/config.py @@ -27,6 +27,20 @@ _BUILTIN_TOOL_NAMES = frozenset({"list-indexes", "search-records", "upsert-records"}) +# Both separators are reserved because the name pattern permits either, so +# `redisvl_search` must be refused exactly like `redisvl-search`. +_RESERVED_TOOL_NAME_PREFIXES = ("redisvl-", "redisvl_") +# MCP clients commonly constrain tool names to this character set, so names that +# would be unusable on some hosts (dots, spaces, uppercase) are rejected here +# rather than at invocation time. Hyphens match the built-ins by convention. +_TOOL_NAME_PATTERN = re.compile(r"^[a-z][a-z0-9_-]{0,63}$") +# Caller-facing arguments a profile may govern. The built-in search tool also +# exposes `index`, which is deliberately absent here: a profile pins its binding +# through the top-level `index:` key rather than offering it to the model. Search +# mode and tuning are binding config and never reach the model at all. +_PROFILE_PARAM_NAMES = frozenset( + {"query", "limit", "offset", "filter", "return_fields"} +) def reserved_score_metadata_field_names() -> frozenset[str]: @@ -581,6 +595,162 @@ def validate_search( ) +class MCPProfileParamConfig(BaseModel): + """Exposure policy for one model-facing argument of a profile tool. + + ``max`` caps the argument when it is exposed. When it is hidden, the cap + becomes the fixed value instead -- see ``register_profile_tool``. + """ + + # Extras are forbidden across the profile models because a misspelled key + # would otherwise be dropped in silence, leaving a tool that reads as locked + # in config while enforcing nothing. + model_config = ConfigDict(extra="forbid") + + expose: bool = True + max: int | None = None + + @model_validator(mode="after") + def _validate_max(self) -> "MCPProfileParamConfig": + """Reject a non-positive cap.""" + if self.max is not None and self.max <= 0: + raise ValueError("custom_tools params max must be greater than 0") + return self + + +class MCPProfileLockConfig(BaseModel): + """Author-locked arguments that the model cannot override or remove.""" + + model_config = ConfigDict(extra="forbid") + + return_fields: list[str] | None = None + filter: dict[str, Any] | None = None + + @model_validator(mode="after") + def _validate_lock(self) -> "MCPProfileLockConfig": + """Reject an empty or blank projection. + + The annotations already give the rest: pydantic enforces ``list[str]`` + and ``dict``, so a locked filter is necessarily the structured DSL form + rather than a raw string, which could not be safely AND-combined with a + caller filter. + """ + if self.return_fields is not None: + if not self.return_fields: + raise ValueError( + "custom_tools lock.return_fields must contain at least one field" + ) + if any(not field_name.strip() for field_name in self.return_fields): + raise ValueError( + "custom_tools lock.return_fields must contain non-empty strings" + ) + return self + + +class MCPCustomToolConfig(BaseModel): + """A declarative custom tool that specializes a built-in. + + A profile is the built-in named by ``based_on`` with some arguments frozen by + the author and the rest still exposed to the model. Because it resolves to a + built-in call and nothing more, it inherits that built-in's concurrency cap, + timeout, read-only policy, auth scoping, and error mapping. + """ + + model_config = ConfigDict(extra="forbid") + + name: str = Field(..., min_length=1) + kind: Literal["profile"] = "profile" + based_on: Literal["search-records"] = "search-records" + index: str | None = None + description: str = Field(..., min_length=1) + suppress_schema_hints: bool = False + lock: MCPProfileLockConfig = Field(default_factory=MCPProfileLockConfig) + params: dict[str, MCPProfileParamConfig] = Field(default_factory=dict) + + @model_validator(mode="after") + def _validate_profile(self) -> "MCPCustomToolConfig": + """Validate naming, parameter policy, and lock/expose coherence.""" + if not self.description.strip(): + # min_length alone lets whitespace through, which would publish a + # tool whose description is blank to the model. + raise ValueError( + f"custom_tools '{self.name}' description must not be blank" + ) + if not _TOOL_NAME_PATTERN.match(self.name): + raise ValueError( + f"custom_tools name '{self.name}' is invalid; names must start with a " + "lowercase letter and contain only lowercase letters, digits, " + "hyphens, or underscores (max 64 characters)" + ) + if self.name in builtin_tool_names(): + raise ValueError( + f"custom_tools name '{self.name}' collides with a built-in tool; " + "built-in names are reserved" + ) + if self.name.startswith(_RESERVED_TOOL_NAME_PREFIXES): + raise ValueError( + f"custom_tools name '{self.name}' uses a reserved prefix " + f"({', '.join(_RESERVED_TOOL_NAME_PREFIXES)})" + ) + + unknown_params = sorted(set(self.params) - _PROFILE_PARAM_NAMES) + if unknown_params: + raise ValueError( + f"custom_tools params contains unknown arguments: " + f"{', '.join(unknown_params)}; allowed: " + f"{', '.join(sorted(_PROFILE_PARAM_NAMES))}" + ) + + # Only `limit` is a bounded numeric argument, so a cap is meaningless + # anywhere else and is more likely a config mistake than an intent. + for param_name, policy in self.params.items(): + if policy.max is not None and param_name != "limit": + raise ValueError( + f"custom_tools params.{param_name} does not support 'max'; " + "only params.limit can be capped" + ) + + query_policy = self.params.get("query") + if query_policy is not None and not query_policy.expose: + raise ValueError( + "custom_tools params.query cannot be hidden; a search profile " + "needs query text from the caller" + ) + + # Locking a projection and letting the model choose one are mutually + # exclusive. A filter is the deliberate exception: locked plus exposed is + # the narrowing case, where the caller's filter AND-combines with the + # locked one. + return_fields_policy = self.params.get("return_fields") + if self.lock.return_fields is not None and ( + return_fields_policy is not None and return_fields_policy.expose + ): + raise ValueError( + "custom_tools cannot both lock return_fields and expose them; " + "set params.return_fields.expose to false or drop the lock" + ) + return self + + def param_exposed(self, param_name: str) -> bool: + """Report whether the model may supply an argument. + + Unlisted arguments stay exposed, so a profile that only locks a filter + keeps the rest of the built-in's contract. A locked projection is the + exception: locking it implies the model cannot also pass one. + """ + policy = self.params.get(param_name) + if policy is not None: + return policy.expose + if param_name == "return_fields" and self.lock.return_fields is not None: + return False + return True + + def param_max(self, param_name: str) -> int | None: + """Return the author-declared cap for an argument, if any.""" + policy = self.params.get(param_name) + return None if policy is None else policy.max + + class MCPConfig(BaseModel): """Validated MCP server configuration loaded from YAML. @@ -591,6 +761,7 @@ class MCPConfig(BaseModel): server: MCPServerConfig indexes: dict[str, MCPIndexBindingConfig] + custom_tools: list[MCPCustomToolConfig] = Field(default_factory=list) @model_validator(mode="after") def _validate_bindings(self) -> "MCPConfig": @@ -603,6 +774,60 @@ def _validate_bindings(self) -> "MCPConfig": raise ValueError("indexes binding id must be non-blank") return self + @model_validator(mode="after") + def _validate_custom_tools(self) -> "MCPConfig": + """Validate custom tool names and index pinning against the bindings. + + Field-level checks that need the inspected schema (locked filter fields, + locked projections) run at startup once each binding's schema is known. + """ + seen: set[str] = set() + for profile in self.custom_tools: + if profile.name in seen: + raise ValueError( + f"custom_tools contains duplicate tool name '{profile.name}'" + ) + seen.add(profile.name) + + if profile.index is None: + # A profile freezes its index, so it can only default when there + # is exactly one binding to default to. + if len(self.indexes) > 1: + available = ", ".join(sorted(self.indexes)) + raise ValueError( + f"custom_tools '{profile.name}' must set 'index' when " + f"multiple indexes are configured; available: {available}" + ) + elif profile.index not in self.indexes: + available = ", ".join(sorted(self.indexes)) + raise ValueError( + f"custom_tools '{profile.name}' references unknown index " + f"'{profile.index}'; available: {available}" + ) + + # A cap above the binding's own ceiling can never be satisfied. With + # `limit` hidden the cap becomes the fixed request size, so every + # call would fail; with it exposed the cap is simply unreachable. + # Either way it is a config mistake, catchable before startup. + limit_cap = profile.param_max("limit") + if limit_cap is not None: + binding_max = self.indexes[ + self.resolved_profile_index(profile) + ].runtime.max_limit + if limit_cap > binding_max: + raise ValueError( + f"custom_tools '{profile.name}' params.limit.max " + f"({limit_cap}) exceeds the bound index's " + f"runtime.max_limit ({binding_max})" + ) + return self + + def resolved_profile_index(self, profile: MCPCustomToolConfig) -> str: + """Return the binding id a profile is pinned to.""" + if profile.index is not None: + return profile.index + return next(iter(self.indexes)) + def _substitute_env(value: Any) -> Any: """Recursively resolve `${VAR}` and `${VAR:-default}` placeholders.""" diff --git a/tests/unit/test_mcp/test_config.py b/tests/unit/test_mcp/test_config.py index 57cdd3bf..9bd178fb 100644 --- a/tests/unit/test_mcp/test_config.py +++ b/tests/unit/test_mcp/test_config.py @@ -26,6 +26,25 @@ def _valid_config() -> dict: } +def _profile_dict(**overrides) -> dict: + """Build one *unvalidated* profile dict. + + Named for its return type on purpose: this file's job is to feed raw + payloads to the validator, so it must not be confused with a helper that + returns an already-validated MCPCustomToolConfig. + """ + profile = {"name": "resolved-search", "description": "Search resolved records."} + profile.update(overrides) + return profile + + +def _raw_config_with_profiles(*profiles: dict) -> dict: + """Build an unvalidated config dict carrying the given raw profile dicts.""" + config = _valid_config() + config["custom_tools"] = list(profiles) + return config + + def _inspected_schema() -> dict: return { "index": { @@ -618,3 +637,308 @@ def test_load_mcp_config_parses_builtin_tools_from_yaml(tmp_path: Path): assert config.server.builtin_tool_enabled("upsert-records") is False assert config.server.builtin_tool_enabled("search-records") is True + + +def test_mcp_config_defaults_to_no_custom_tools(): + config = MCPConfig.model_validate(_valid_config()) + + assert config.custom_tools == [] + + +def test_mcp_config_custom_tool_defaults(): + config = MCPConfig.model_validate(_raw_config_with_profiles(_profile_dict())) + + profile = config.custom_tools[0] + assert profile.name == "resolved-search" + assert profile.kind == "profile" + assert profile.based_on == "search-records" + # An unpinned profile is legal with one binding and resolves to it. + assert profile.index is None + assert profile.suppress_schema_hints is False + assert profile.lock.return_fields is None + assert profile.lock.filter is None + assert profile.params == {} + assert config.resolved_profile_index(profile) == "knowledge" + + +@pytest.mark.parametrize( + "name", + [ + # One violation per position the pattern constrains: the anchored first + # character, and the body character class. + "1foo", + "shop.foo", + ], +) +def test_mcp_config_rejects_invalid_custom_tool_names(name): + with pytest.raises(ValueError, match="is invalid"): + MCPConfig.model_validate(_raw_config_with_profiles(_profile_dict(name=name))) + + +@pytest.mark.parametrize( + ("name", "valid"), + [ + # The pattern is `^[a-z][a-z0-9_-]{0,63}$`, so 64 characters is the + # inclusive ceiling and 65 is the first rejection. Nothing else pins this + # bound, and an off-by-one in the quantifier is otherwise invisible. + ("a" + "b" * 63, True), + ("a" + "b" * 64, False), + ], +) +def test_mcp_config_bounds_custom_tool_name_length_at_64_characters(name, valid): + config = _raw_config_with_profiles(_profile_dict(name=name)) + + if valid: + assert MCPConfig.model_validate(config).custom_tools[0].name == name + else: + with pytest.raises(ValueError, match="is invalid"): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_custom_tool_names_that_collide_with_builtins(): + # One built-in stands in for the rest: the check is a membership test against + # builtin_tool_names(), not per-name logic. + name = "search-records" + assert name in builtin_tool_names() + + with pytest.raises(ValueError, match="collides with a built-in tool"): + MCPConfig.model_validate(_raw_config_with_profiles(_profile_dict(name=name))) + + +def test_mcp_config_rejects_unknown_custom_tool_params(): + config = _raw_config_with_profiles( + _profile_dict(params={"search_type": {"expose": False}}) + ) + + with pytest.raises(ValueError, match="params contains unknown arguments"): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_max_on_params_other_than_limit(): + # `limit` is the sole param with a cap; every other name takes the same + # rejection branch, so one stands in for all of them. + config = _raw_config_with_profiles(_profile_dict(params={"offset": {"max": 5}})) + + with pytest.raises(ValueError, match="does not support 'max'"): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_non_positive_param_max(): + # 0 is the boundary: it is the largest value the `> 0` check must still refuse. + config = _raw_config_with_profiles(_profile_dict(params={"limit": {"max": 0}})) + + with pytest.raises(ValueError, match="max must be greater than 0"): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_hiding_the_query_param(): + config = _raw_config_with_profiles( + _profile_dict(params={"query": {"expose": False}}) + ) + + with pytest.raises(ValueError, match="params.query cannot be hidden"): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_locking_return_fields_while_exposing_them(): + config = _raw_config_with_profiles( + _profile_dict( + lock={"return_fields": ["content"]}, + params={"return_fields": {"expose": True}}, + ) + ) + + with pytest.raises(ValueError, match="cannot both lock return_fields"): + MCPConfig.model_validate(config) + + +def test_mcp_config_allows_locking_return_fields_when_they_are_explicitly_hidden(): + config = _raw_config_with_profiles( + _profile_dict( + lock={"return_fields": ["content"]}, + params={"return_fields": {"expose": False}}, + ) + ) + + profile = MCPConfig.model_validate(config).custom_tools[0] + + assert profile.lock.return_fields == ["content"] + assert profile.param_exposed("return_fields") is False + + +def test_mcp_config_allows_locking_a_filter_while_the_caller_filter_stays_exposed(): + config = _raw_config_with_profiles( + _profile_dict( + lock={"filter": {"field": "content", "op": "like", "value": "jam*"}}, + params={"filter": {"expose": True}}, + ) + ) + + profile = MCPConfig.model_validate(config).custom_tools[0] + + # Unlike return_fields, locked-plus-exposed is the intended narrowing case. + assert profile.param_exposed("filter") is True + + +def test_mcp_config_rejects_empty_locked_return_fields(): + config = _raw_config_with_profiles(_profile_dict(lock={"return_fields": []})) + + with pytest.raises(ValueError, match="must contain at least one field"): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_blank_locked_return_field_names(): + # Whitespace rather than "": the empty string is already falsy, so only this + # one requires the check to `.strip()` before testing emptiness. + config = _raw_config_with_profiles(_profile_dict(lock={"return_fields": [" "]})) + + with pytest.raises(ValueError, match="must contain non-empty strings"): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_duplicate_custom_tool_names(): + config = _raw_config_with_profiles( + _profile_dict(), _profile_dict(description="Another description.") + ) + + with pytest.raises(ValueError, match="contains duplicate tool name"): + MCPConfig.model_validate(config) + + +def test_mcp_config_requires_custom_tool_index_when_multiple_bindings_exist(): + config = _raw_config_with_profiles(_profile_dict()) + config["indexes"]["tickets"] = deepcopy(config["indexes"]["knowledge"]) + + with pytest.raises( + ValueError, match="must set 'index' when multiple indexes are configured" + ): + MCPConfig.model_validate(config) + + +def test_mcp_config_rejects_custom_tool_index_naming_an_unknown_binding(): + config = _raw_config_with_profiles(_profile_dict(index="missing")) + + with pytest.raises(ValueError, match="references unknown index"): + MCPConfig.model_validate(config) + + +def test_mcp_config_resolves_a_pinned_custom_tool_index(): + config = _raw_config_with_profiles(_profile_dict(index="tickets")) + config["indexes"]["tickets"] = deepcopy(config["indexes"]["knowledge"]) + + loaded = MCPConfig.model_validate(config) + + assert loaded.resolved_profile_index(loaded.custom_tools[0]) == "tickets" + + +def test_mcp_config_param_exposed_hides_return_fields_implicitly_when_locked(): + config = _raw_config_with_profiles( + _profile_dict(lock={"return_fields": ["content"]}) + ) + + profile = MCPConfig.model_validate(config).custom_tools[0] + + # Locking implies hiding without the author having to say so twice. + assert profile.param_exposed("return_fields") is False + assert profile.param_exposed("filter") is True + + +@pytest.mark.parametrize( + ("label", "profile_patch"), + [ + # One case per model that declares `extra="forbid"` -- the profile itself, + # its `lock`, and a per-param policy. There are exactly three, and within + # any one of them every misspelling takes the identical pydantic branch. + ("profile", {"basedon": "search-records"}), + ("lock", {"lock": {"return_field": ["content"]}}), + ("params policy", {"params": {"limit": {"exposed": True}}}), + ], +) +def test_custom_tools_rejects_misspelled_keys(label, profile_patch): + del label + config = _valid_config() + profile = {"name": "resolved-search", "description": "Search resolved records."} + profile.update(profile_patch) + config["custom_tools"] = [profile] + + # A dropped key would leave a tool that reads as locked in config while + # enforcing nothing, so unrecognized keys must fail rather than be ignored. + with pytest.raises(ValueError, match="Extra inputs are not permitted"): + MCPConfig.model_validate(config) + + +def test_custom_tools_rejects_a_limit_cap_above_the_bindings_max_limit(): + config = _valid_config() + config["indexes"]["knowledge"]["runtime"]["default_limit"] = 5 + config["indexes"]["knowledge"]["runtime"]["max_limit"] = 5 + config["custom_tools"] = [ + { + "name": "capped-search", + "description": "Search records.", + "params": {"limit": {"expose": False, "max": 10}}, + } + ] + + # With `limit` hidden the cap becomes the request size, so a cap the binding + # can never satisfy would make every call fail. Catch it before startup. + with pytest.raises(ValueError, match="exceeds the bound index's"): + MCPConfig.model_validate(config) + + +@pytest.mark.parametrize("name", ["redisvl-search", "redisvl_search"]) +def test_custom_tools_rejects_both_reserved_prefix_separators(name): + config = _valid_config() + config["custom_tools"] = [{"name": name, "description": "Search records."}] + + # The name pattern permits either separator, so both spellings of the + # reserved prefix have to be refused. + with pytest.raises(ValueError, match="reserved prefix"): + MCPConfig.model_validate(config) + + +def test_load_mcp_config_parses_custom_tools_from_yaml(tmp_path: Path): + """The whole point is YAML authoring, so cover the real load path once.""" + config_path = tmp_path / "mcp.yaml" + config_path.write_text( + """ +server: + redis_url: redis://localhost:6379 + builtin_tools: + search-records: disabled +indexes: + knowledge: + redis_name: docs-index + search: + type: fulltext + runtime: + text_field_name: content +custom_tools: + - name: resolved-search + description: Search resolved records. + lock: + return_fields: [content] + filter: + field: category + op: eq + value: resolved + params: + limit: + max: 5 +""".strip(), + encoding="utf-8", + ) + + config = load_mcp_config(str(config_path)) + + profile = config.custom_tools[0] + assert profile.name == "resolved-search" + assert profile.lock.return_fields == ["content"] + assert profile.lock.filter == { + "field": "category", + "op": "eq", + "value": "resolved", + } + assert profile.param_max("limit") == 5 + # Disabling the built-in a profile supersedes is the motivating pairing, so + # confirm both halves survive one load. + assert config.server.builtin_tool_enabled("search-records") is False