From 7a25665f8be4e5aad6660fc2d8c1d57d5593819f Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 6 Aug 2026 10:37:19 +0200 Subject: [PATCH 1/4] fix(mcp): escape text filter values and reject an empty projection Two independent holes in the `search-records` request boundary. Text filter values reached the query string unescaped. `Text`'s operator templates are `@field:("value")` for eq/ne and `@field:(value)` for like, so a value containing a quote or a paren could close its own clause and have the remainder parsed as RediSearch syntax. Tag and numeric values are already escaped or type-checked upstream; text was not. This escapes text at the MCP filter boundary rather than in `Text.__str__`, so library users who pass raw RediSearch syntax deliberately are unaffected. `like` keeps `*` and `?` live, since it is the pattern-matching operator. Note this is a behavior change on a surface that has shipped since v0.17.0: a structured DSL value that previously smuggled query syntax through now matches literally. That passthrough was never documented, and the raw-string `filter` form still serves callers who want it. Separately, `return_fields: []` was accepted and reached Redis as no RETURN clause at all -- which returns every field, including the vector that the same function refuses a few lines down. An empty list now raises, with the error pointing at omitting the argument instead. Also moves the shared `_schema()` builder into conftest, since the filter and search unit tests were carrying identical copies. --- redisvl/mcp/filters.py | 28 ++++++- redisvl/mcp/tools/search.py | 11 +++ tests/unit/test_mcp/conftest.py | 35 +++++++++ tests/unit/test_mcp/test_filters.py | 77 +++++++++++++------- tests/unit/test_mcp/test_search_tool_unit.py | 42 ++++------- 5 files changed, 134 insertions(+), 59 deletions(-) diff --git a/redisvl/mcp/filters.py b/redisvl/mcp/filters.py index 270d1423d..5ca858abf 100644 --- a/redisvl/mcp/filters.py +++ b/redisvl/mcp/filters.py @@ -3,6 +3,15 @@ from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.query.filter import FilterExpression, Num, Tag, Text from redisvl.schema import IndexSchema +from redisvl.utils.token_escaper import TokenEscaper + +# Text values reach the query string unescaped: `Text`'s operator templates are +# `@field:("value")` for eq/ne and `@field:(value)` for like, so a caller value +# containing a quote or a paren can close its own clause and inject arbitrary +# RediSearch syntax after it -- including a `|` that escapes an enclosing AND. +# Tag and numeric values are already escaped or type-checked upstream; text is +# not, so this boundary escapes it before building the expression. +_TEXT_ESCAPER = TokenEscaper() def parse_filter( @@ -132,17 +141,28 @@ def _parse_tag_expression(field_name: str, op: str, operand: Any) -> FilterExpre ) +def _escape_text(value: str, *, preserve_wildcards: bool = False) -> str: + """Escape a caller-supplied text value so it cannot leave its own clause.""" + return _TEXT_ESCAPER.escape(value, preserve_wildcards=preserve_wildcards) + + def _parse_text_expression(field_name: str, op: str, operand: Any) -> FilterExpression: field = Text(field_name) if op == "eq": - return field == _require_string(operand, field_name, op) + return field == _escape_text(_require_string(operand, field_name, op)) if op == "ne": - return field != _require_string(operand, field_name, op) + return field != _escape_text(_require_string(operand, field_name, op)) if op == "like": - return field % _require_string(operand, field_name, op) + # `like` is the pattern-matching operator, so `*` and `?` stay live. + return field % _escape_text( + _require_string(operand, field_name, op), preserve_wildcards=True + ) if op == "in": return _combine_or( - [field == item for item in _require_string_list(operand, field_name, op)] + [ + field == _escape_text(item) + for item in _require_string_list(operand, field_name, op) + ] ) raise RedisVLMCPError( f"Unsupported operator '{op}' for text field '{field_name}'", diff --git a/redisvl/mcp/tools/search.py b/redisvl/mcp/tools/search.py index adbeab752..3334b3471 100644 --- a/redisvl/mcp/tools/search.py +++ b/redisvl/mcp/tools/search.py @@ -153,6 +153,17 @@ def _validate_request( code=MCPErrorCode.INVALID_REQUEST, retryable=False, ) + if not return_fields: + # An empty projection reaches Redis as no RETURN clause at all, which + # returns *every* field -- including the vector this function refuses + # a few lines down. "Omit the argument" is the way to ask for the + # default projection. + raise RedisVLMCPError( + "return_fields must not be empty; omit it to use the default " + "projection", + code=MCPErrorCode.INVALID_REQUEST, + retryable=False, + ) fields = [] for field_name in return_fields: if not isinstance(field_name, str) or not field_name: diff --git a/tests/unit/test_mcp/conftest.py b/tests/unit/test_mcp/conftest.py index f5d7e2bc9..2a625b278 100644 --- a/tests/unit/test_mcp/conftest.py +++ b/tests/unit/test_mcp/conftest.py @@ -1,8 +1,43 @@ import pytest +from redisvl.schema import IndexSchema + @pytest.fixture(scope="session", autouse=True) def redis_container(): # Shadow the repo-wide autouse Redis container fixture so MCP unit tests stay # pure-unit and do not require Docker; Redis coverage lives in integration tests. yield None + + +def _schema() -> IndexSchema: + """The one index shape the filter, search, and profile unit tests all build against. + + A plain function rather than a fixture: it is a stateless data builder that + module-level parametrization and helper functions both need to call, and the + convention in these files is per-module fakes rather than shared fixtures. + """ + return IndexSchema.from_dict( + { + "index": { + "name": "docs-index", + "prefix": "doc", + "storage_type": "hash", + }, + "fields": [ + {"name": "content", "type": "text"}, + {"name": "category", "type": "tag"}, + {"name": "rating", "type": "numeric"}, + { + "name": "embedding", + "type": "vector", + "attrs": { + "algorithm": "flat", + "dims": 3, + "distance_metric": "cosine", + "datatype": "float32", + }, + }, + ], + } + ) diff --git a/tests/unit/test_mcp/test_filters.py b/tests/unit/test_mcp/test_filters.py index 4fb43b6af..c2d12e3e7 100644 --- a/tests/unit/test_mcp/test_filters.py +++ b/tests/unit/test_mcp/test_filters.py @@ -1,36 +1,9 @@ import pytest +from conftest import _schema from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.mcp.filters import parse_filter from redisvl.query.filter import FilterExpression -from redisvl.schema import IndexSchema - - -def _schema() -> IndexSchema: - return IndexSchema.from_dict( - { - "index": { - "name": "docs-index", - "prefix": "doc", - "storage_type": "hash", - }, - "fields": [ - {"name": "content", "type": "text"}, - {"name": "category", "type": "tag"}, - {"name": "rating", "type": "numeric"}, - { - "name": "embedding", - "type": "vector", - "attrs": { - "algorithm": "flat", - "dims": 3, - "distance_metric": "cosine", - "datatype": "float32", - }, - }, - ], - } - ) def _render_filter(value): @@ -39,6 +12,18 @@ def _render_filter(value): return value +def _strip_escapes(rendered: str) -> str: + """Drop backslash-escaped pairs, leaving only unescaped query syntax.""" + out, index = [], 0 + while index < len(rendered): + if rendered[index] == "\\": + index += 2 + continue + out.append(rendered[index]) + index += 1 + return "".join(out) + + def test_parse_filter_passes_through_raw_string(): raw = "@category:{science} @rating:[4 +inf]" @@ -134,3 +119,39 @@ def test_parse_filter_rejects_malformed_payload(): parse_filter({"field": "category", "value": "science"}, _schema()) assert exc_info.value.code == MCPErrorCode.INVALID_FILTER + + +@pytest.mark.parametrize( + ("op", "operand"), + [ + ("eq", 'alpha") | (-@category:{secret}'), + ("ne", 'alpha") | (-@category:{secret}'), + ("like", "alpha) | (-@category:{secret}"), + ("in", ['alpha") | (-@category:{secret}']), + ], +) +def test_parse_filter_escapes_text_values_so_they_cannot_leave_their_clause( + op, operand +): + parsed = parse_filter({"field": "content", "op": op, "value": operand}, _schema()) + + # Text operator templates interpolate the value into `@field:("...")` or + # `@field:(...)`, so an unescaped quote or paren would close the clause and + # let the rest of the value inject query syntax -- including a `|` that + # escapes an enclosing AND. Stripping escape pairs leaves the structural + # skeleton: the payload must contribute no syntax to it. + skeleton = _strip_escapes(_render_filter(parsed)) + # The clause boundary survives: quotes and parens from the payload are + # escaped, so its `|` stays scoped inside this field's own query instead of + # splitting the whole expression into a union. + assert skeleton.count("(") == skeleton.count(")") + assert skeleton.count('"') % 2 == 0 + + +def test_parse_filter_preserves_wildcards_in_text_like_patterns(): + parsed = parse_filter( + {"field": "content", "op": "like", "value": "quant*"}, _schema() + ) + + # `like` is the pattern operator, so escaping must not neuter `*`. + assert _render_filter(parsed) == "@content:(quant*)" diff --git a/tests/unit/test_mcp/test_search_tool_unit.py b/tests/unit/test_mcp/test_search_tool_unit.py index 60a887f5c..6afbef041 100644 --- a/tests/unit/test_mcp/test_search_tool_unit.py +++ b/tests/unit/test_mcp/test_search_tool_unit.py @@ -2,6 +2,7 @@ from typing import Any import pytest +from conftest import _schema from redisvl.mcp.config import MCPConfig from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError @@ -16,33 +17,6 @@ from redisvl.schema import IndexSchema -def _schema() -> IndexSchema: - return IndexSchema.from_dict( - { - "index": { - "name": "docs-index", - "prefix": "doc", - "storage_type": "hash", - }, - "fields": [ - {"name": "content", "type": "text"}, - {"name": "category", "type": "tag"}, - {"name": "rating", "type": "numeric"}, - { - "name": "embedding", - "type": "vector", - "attrs": { - "algorithm": "flat", - "dims": 3, - "distance_metric": "cosine", - "datatype": "float32", - }, - }, - ], - } - ) - - def _config_with_search( search_type: str, params: dict[str, Any] | None = None, @@ -244,6 +218,20 @@ async def test_search_records_rejects_unknown_or_vector_return_fields(): assert vector_exc.value.code == MCPErrorCode.INVALID_REQUEST +@pytest.mark.asyncio +async def test_search_records_rejects_an_empty_return_fields_list(): + server = FakeServer() + + # An empty list is not "no projection" -- it reaches Redis as an absent + # RETURN clause, which widens the response to every field including the + # vector. Rejecting it keeps `[]` from quietly meaning the opposite of what + # it reads like. + with pytest.raises(RedisVLMCPError, match="return_fields must not be empty") as exc: + await search_records(server, query="science", return_fields=[]) + + assert exc.value.code == MCPErrorCode.INVALID_REQUEST + + @pytest.mark.asyncio @pytest.mark.parametrize( ("result", "message"), From 8e7a87f0cf8af362aa8a5daf57938a82f9407810 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 6 Aug 2026 16:02:57 +0200 Subject: [PATCH 2/4] fix(mcp): reject a text_field_name that points at a vector field Follows up review feedback on the empty-projection guard: the guard rejects an explicit `return_fields: []`, but the *default* projection could still come out empty, and nothing caught that. The default projection is every non-vector field. `validate_runtime_mapping` only checked that `text_field_name` appeared in the schema, not that it was text-searchable, so a fulltext or hybrid binding could name the vector field on an index with no other fields. The projection then resolved to `[]`, and an empty `return_fields` is falsy -- RedisVL omits the RETURN clause entirely, so Redis returns every stored field, including the embedding this tool refuses to return when a caller names it explicitly. `vector_field_name` was already type-checked a few lines below, so this restores the symmetry that was missing. Worth recording why this is the whole fix. `VectorQuery` adds `vector_distance` to its own return fields, so the vector path emits a RETURN clause even from an empty projection; the hybrid paths prepend `__key` for the same reason. `TextQuery` does neither. With `text_field_name` now guaranteed to be a non-vector field, the fulltext and hybrid projections always contain at least that field, which makes "every search path emits a RETURN clause" true by construction rather than by coincidence. A test pins that invariant so a future search mode cannot quietly reintroduce the hole. --- redisvl/mcp/config.py | 20 +++++++++--- tests/unit/test_mcp/test_config.py | 33 +++++++++++++++++++ tests/unit/test_mcp/test_search_tool_unit.py | 34 ++++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/redisvl/mcp/config.py b/redisvl/mcp/config.py index 90fb475a0..8bafcc5d7 100644 --- a/redisvl/mcp/config.py +++ b/redisvl/mcp/config.py @@ -472,10 +472,22 @@ def validate_runtime_mapping(self, schema: IndexSchema) -> None: """Ensure runtime mappings point at explicit fields in the effective schema.""" field_names = set(schema.field_names) - if self.uses_text_search and self.runtime.text_field_name not in field_names: - raise ValueError( - f"runtime.text_field_name '{self.runtime.text_field_name}' not found in schema" - ) + if self.uses_text_search: + if self.runtime.text_field_name not in field_names: + raise ValueError( + f"runtime.text_field_name '{self.runtime.text_field_name}' not found in schema" + ) + # Membership alone is not enough: a vector field cannot be searched as + # text, and pointing at one leaves the default projection empty (it is + # every *non-vector* field). An empty projection reaches Redis as no + # RETURN clause at all, which returns every stored field -- including + # the embedding this tool refuses to return when asked for by name. + text_field = schema.fields.get(self.runtime.text_field_name) + if text_field is not None and text_field.type == "vector": + raise ValueError( + f"runtime.text_field_name '{self.runtime.text_field_name}' is a " + "vector field; text search requires a text or tag field" + ) if ( self.supports_server_side_embedding diff --git a/tests/unit/test_mcp/test_config.py b/tests/unit/test_mcp/test_config.py index 512af828a..2a738d875 100644 --- a/tests/unit/test_mcp/test_config.py +++ b/tests/unit/test_mcp/test_config.py @@ -525,3 +525,36 @@ def test_mcp_config_allows_linear_hybrid_fallback_params(): schema=schema, supports_native_hybrid_search=False, ) + + +@pytest.mark.parametrize("search_type", ["fulltext", "hybrid"]) +def test_mcp_config_rejects_a_text_field_that_is_actually_the_vector_field(search_type): + """Membership in the schema is not enough -- the field must be text-searchable. + + Pointing `text_field_name` at the vector field passes a name-only check while + leaving the default projection (every *non-vector* field) empty. An empty + projection reaches Redis as no RETURN clause, which returns every stored + field including the embedding, so this has to fail at startup. + """ + config = _valid_config() + config["indexes"]["knowledge"]["search"] = {"type": search_type} + config["indexes"]["knowledge"]["runtime"]["text_field_name"] = "embedding" + + binding = MCPConfig.model_validate(config).indexes["knowledge"] + + # `to_index_schema` validates the runtime mapping against the effective + # schema, so startup fails here rather than at the first request. + with pytest.raises(ValueError, match="is a vector field"): + binding.to_index_schema(_inspected_schema()) + + +def test_mcp_config_still_accepts_a_real_text_field(): + """The control: a genuine text field must keep validating.""" + config = _valid_config() + config["indexes"]["knowledge"]["search"] = {"type": "fulltext"} + config["indexes"]["knowledge"]["runtime"]["text_field_name"] = "content" + + binding = MCPConfig.model_validate(config).indexes["knowledge"] + schema = binding.to_index_schema(_inspected_schema()) + + binding.validate_runtime_mapping(schema) diff --git a/tests/unit/test_mcp/test_search_tool_unit.py b/tests/unit/test_mcp/test_search_tool_unit.py index 6afbef041..8dbeb352c 100644 --- a/tests/unit/test_mcp/test_search_tool_unit.py +++ b/tests/unit/test_mcp/test_search_tool_unit.py @@ -850,3 +850,37 @@ def test_build_search_tool_description_distinguishes_typed_and_exists_support(): in description ) assert "Allowed return_fields: content, category, rating, location." in description + + +@pytest.mark.asyncio +@pytest.mark.parametrize("search_type", ["vector", "fulltext"]) +async def test_default_projection_always_produces_a_return_clause( + monkeypatch, search_type +): + """A query with no RETURN clause returns every stored field, vector included. + + The default projection is every *non-vector* field, so on an index with no + other fields it is empty -- and an empty `return_fields` is falsy, which makes + RedisVL omit RETURN entirely. `VectorQuery` self-adds `vector_distance` so it + is safe; `TextQuery` does not, which is why config validation refuses to point + `text_field_name` at a vector field. This pins the resulting invariant: no + search path may build a query without a RETURN clause. + """ + server = FakeServer(search_type=search_type) + built = [] + + monkeypatch.setattr( + "redisvl.mcp.tools.search.VectorQuery", + lambda **kw: built.append(kw) or FakeQuery(**kw), + ) + monkeypatch.setattr( + "redisvl.mcp.tools.search.TextQuery", + lambda **kw: built.append(kw) or FakeQuery(**kw), + ) + + await search_records(server, query="science") + + assert built, "no query was built" + # Non-empty is what makes RedisVL emit RETURN; empty would silently widen the + # response to every field. + assert built[0]["return_fields"], "empty projection would omit RETURN entirely" From e2b310b881c4a3553ad2bf0e642180b459a6e209 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 6 Aug 2026 17:37:24 +0200 Subject: [PATCH 3/4] fix(mcp): stop over-escaping `like` pattern metacharacters The escaping change used `preserve_wildcards=True` for `like`, which keeps `*` and `?` but still escapes a space and `%`. Both carry meaning in a `like` pattern, so two documented behaviors broke, and neither broke loudly -- an over-escaped pattern is a valid query that matches nothing: like "foo bar" -> @content:(foo\ bar) AND between terms lost like "%foo%" -> @content:(\%foo\%) fuzzy matching lost `like` now uses its own character set: the shared no-wildcard set minus `%` and space. Exact-match values keep the stricter escaping, since an `eq` value is a literal while a `like` value is a pattern -- the same reason `*` was already exempt. Containment is unaffected, because it never came from these characters. It comes from the delimiters: with `(` and `)` escaped, a value cannot close its own `@field:(...)`, so anything it carries stays scoped to that one field. The injection payload from the escaping tests still renders fully contained, and its `|` -- which no escaper in RedisVL touches -- unions within `content` rather than reaching the surrounding expression. Tests pin each metacharacter separately, assert parity with `Text.__mod__` for ordinary patterns, and keep a containment case so a future widening of the preserved set cannot quietly reopen the hole. --- redisvl/mcp/filters.py | 31 ++++++++++++---- tests/unit/test_mcp/test_filters.py | 56 ++++++++++++++++++++++++++--- 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/redisvl/mcp/filters.py b/redisvl/mcp/filters.py index 5ca858abf..862e6d4bc 100644 --- a/redisvl/mcp/filters.py +++ b/redisvl/mcp/filters.py @@ -1,3 +1,4 @@ +import re from typing import Any, Iterable from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError @@ -13,6 +14,20 @@ # not, so this boundary escapes it before building the expression. _TEXT_ESCAPER = TokenEscaper() +# `like` is the pattern operator, so the metacharacters that give a pattern its +# meaning have to stay live: `*` and `?` for wildcards, `%` for fuzzy matching, +# and a space for the implicit AND between terms. Escaping those does not fail +# loudly -- it silently turns a documented pattern into a literal that matches +# nothing -- so this set is the shared no-wildcard set minus `%` and space. +# +# Dropping them costs nothing in containment, because containment comes from the +# delimiters rather than from these. With `(` and `)` escaped, the value cannot +# close its own `@field:(...)`, so anything it carries -- including a `|`, which +# no escaper in RedisVL touches -- stays scoped to this one field instead of +# reaching the surrounding expression. +_LIKE_ESCAPED_CHARS = re.compile(r"[,.<>{}\[\]\\\"\':;!@#$^&()\-+=~\/]") +_LIKE_ESCAPER = TokenEscaper(escape_chars_re=_LIKE_ESCAPED_CHARS) + def parse_filter( value: str | dict[str, Any] | None, schema: IndexSchema @@ -141,9 +156,14 @@ def _parse_tag_expression(field_name: str, op: str, operand: Any) -> FilterExpre ) -def _escape_text(value: str, *, preserve_wildcards: bool = False) -> str: +def _escape_text(value: str) -> str: """Escape a caller-supplied text value so it cannot leave its own clause.""" - return _TEXT_ESCAPER.escape(value, preserve_wildcards=preserve_wildcards) + return _TEXT_ESCAPER.escape(value) + + +def _escape_like_pattern(value: str) -> str: + """Escape a `like` pattern, leaving its pattern metacharacters intact.""" + return _LIKE_ESCAPER.escape(value) def _parse_text_expression(field_name: str, op: str, operand: Any) -> FilterExpression: @@ -153,10 +173,9 @@ def _parse_text_expression(field_name: str, op: str, operand: Any) -> FilterExpr if op == "ne": return field != _escape_text(_require_string(operand, field_name, op)) if op == "like": - # `like` is the pattern-matching operator, so `*` and `?` stay live. - return field % _escape_text( - _require_string(operand, field_name, op), preserve_wildcards=True - ) + # An exact-match value is a literal, but a `like` value is a pattern, so + # the two need different escaping -- see `_LIKE_ESCAPED_CHARS`. + return field % _escape_like_pattern(_require_string(operand, field_name, op)) if op == "in": return _combine_or( [ diff --git a/tests/unit/test_mcp/test_filters.py b/tests/unit/test_mcp/test_filters.py index c2d12e3e7..f3cd44990 100644 --- a/tests/unit/test_mcp/test_filters.py +++ b/tests/unit/test_mcp/test_filters.py @@ -3,7 +3,7 @@ from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.mcp.filters import parse_filter -from redisvl.query.filter import FilterExpression +from redisvl.query.filter import FilterExpression, Text def _render_filter(value): @@ -148,10 +148,56 @@ def test_parse_filter_escapes_text_values_so_they_cannot_leave_their_clause( assert skeleton.count('"') % 2 == 0 -def test_parse_filter_preserves_wildcards_in_text_like_patterns(): +@pytest.mark.parametrize( + ("value", "rendered"), + [ + # Each metacharacter that gives a `like` pattern its meaning. Escaping any + # of these does not fail loudly -- it turns the pattern into a literal that + # matches nothing -- so each is pinned separately. + ("quant*", "@content:(quant*)"), + ("qu?nt", "@content:(qu?nt)"), + # A space is the implicit AND between terms, not padding. + ("foo bar", "@content:(foo bar)"), + # `%` is fuzzy matching. + ("%foo%", "@content:(%foo%)"), + ], +) +def test_parse_filter_preserves_like_pattern_metacharacters(value, rendered): + parsed = parse_filter({"field": "content", "op": "like", "value": value}, _schema()) + + assert _render_filter(parsed) == rendered + + +def test_parse_filter_like_matches_library_semantics_for_patterns(): + """MCP `like` must not diverge from `Text.__mod__` for ordinary patterns.""" + for value in ["quant*", "foo bar", "%foo%", "qu?nt"]: + mcp = _render_filter( + parse_filter({"field": "content", "op": "like", "value": value}, _schema()) + ) + assert mcp == str(Text("content") % value), f"diverged for {value!r}" + + +def test_parse_filter_like_still_escapes_clause_delimiters(): + """Preserving pattern metacharacters must not cost containment. + + Containment comes from the delimiters, not from `%`/space: with `(` and `)` + escaped the value cannot close its own `@field:(...)`, so a `|` it carries + stays scoped to this field. `|` is deliberately checked because no escaper in + RedisVL touches it. + """ parsed = parse_filter( - {"field": "content", "op": "like", "value": "quant*"}, _schema() + {"field": "content", "op": "like", "value": "alpha) | (-@category:{secret}"}, + _schema(), ) + rendered = _render_filter(parsed) - # `like` is the pattern operator, so escaping must not neuter `*`. - assert _render_filter(parsed) == "@content:(quant*)" + # The payload's own parens are escaped... + assert "\\)" in rendered and "\\(" in rendered + # ...so stripping escape pairs leaves a balanced skeleton: nothing the payload + # contributed can terminate the clause early. + skeleton = _strip_escapes(rendered) + assert skeleton.count("(") == skeleton.count(")") + # The `|` survives unescaped but is inside the field clause, so it unions + # within `content` rather than splitting the whole expression. + assert skeleton.index("|") > skeleton.index("(") + assert skeleton.index("|") < skeleton.rindex(")") From 1a1c0785ed2c3021c4645908c2f43bc7e565d661 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 7 Aug 2026 15:50:41 +0200 Subject: [PATCH 4/4] test(mcp): pin the one case where a falsy projection reaches a query Review question: can `return_fields` be read two ways, empty versus omitted? At the request boundary, no -- the branch is `is None`, so `[]` takes the else and is rejected rather than treated as absent. Weakening that check to `if not return_fields` makes `test_search_records_rejects_an_empty_return_fields_list` fail, so the distinction is guarded rather than incidental. The instinct is right one layer down, though. The *default* projection is every non-vector field, so an index whose only field is the vector resolves it to `[]` -- and RedisVL does read a falsy `return_fields` as "no projection" and omits RETURN. That path stays safe only because `VectorQuery` adds `vector_distance` to its own return fields, so a RETURN clause is emitted regardless. This adds a test for exactly that case: it asserts the default projection really is empty there, that the built query still carries RETURN, and that what comes back is the score rather than the embedding. The dependency on `VectorQuery` was previously implicit, so nothing would have caught it changing. Fulltext and hybrid cannot reach this state at all now that `text_field_name` must be a non-vector field, which puts it in the projection by construction. --- tests/unit/test_mcp/test_search_tool_unit.py | 64 ++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/unit/test_mcp/test_search_tool_unit.py b/tests/unit/test_mcp/test_search_tool_unit.py index 8dbeb352c..328aa2e27 100644 --- a/tests/unit/test_mcp/test_search_tool_unit.py +++ b/tests/unit/test_mcp/test_search_tool_unit.py @@ -11,9 +11,11 @@ _build_fallback_hybrid_kwargs, _build_search_tool_description, _embed_query, + _validate_request, register_search_tool, search_records, ) +from redisvl.query import VectorQuery from redisvl.schema import IndexSchema @@ -884,3 +886,65 @@ async def test_default_projection_always_produces_a_return_clause( # Non-empty is what makes RedisVL emit RETURN; empty would silently widen the # response to every field. assert built[0]["return_fields"], "empty projection would omit RETURN entirely" + + +def test_empty_default_projection_still_yields_a_return_clause(): + """The one case where a falsy `return_fields` legitimately reaches a query. + + `None` and `[]` are kept distinct at the request boundary -- the branch is + `is None`, so an explicit `[]` is rejected rather than read as "omitted". But + the *default* projection is every non-vector field, so an index with only a + vector field resolves it to `[]`, and RedisVL reads a falsy `return_fields` + as "no projection" and omits RETURN. + + That is safe here only because `VectorQuery` adds `vector_distance` to its own + return fields, so a RETURN clause is emitted anyway. This pins that + dependency: if it ever stops holding, an index like this would start + returning every stored field, embedding included. Fulltext and hybrid cannot + reach this state at all, because `text_field_name` is required to be a + non-vector field and therefore always lands in the projection. + """ + vector_only = IndexSchema.from_dict( + { + "index": {"name": "vec-only", "prefix": "v", "storage_type": "hash"}, + "fields": [ + { + "name": "embedding", + "type": "vector", + "attrs": { + "algorithm": "flat", + "dims": 3, + "distance_metric": "cosine", + "datatype": "float32", + }, + } + ], + } + ) + runtime = SimpleNamespace(default_limit=2, max_limit=5, max_result_window=100) + + _, fields = _validate_request( + query="science", + limit=None, + offset=0, + return_fields=None, + runtime=runtime, + schema=vector_only, + ) + + # The default projection really is empty here, so this is not passing for + # some other reason. + assert fields == [] + + rendered = str( + VectorQuery( + vector=[0.1, 0.2, 0.3], + vector_field_name="embedding", + return_fields=fields, + num_results=2, + ) + ) + + assert "RETURN" in rendered + # And what it returns is the score, not the embedding. + assert "embedding" not in rendered.split("RETURN", 1)[1]