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