Repository navigation
Python: fix(redis): let an injected redis_client take precedence in RedisHistoryProvider - #10041
Fish (wxhking) wants to merge 2 commits into
Conversation
RedisContextProvider passed the user's message straight into TextQuery / AggregateHybridQuery as the search text. RediSearch reads several punctuation marks as query syntax there — '|' unions terms, '?' is a fuzzy marker, backticks quote exact phrases — so ordinary messages like 'ps aux | grep python' or 'Why ?' reached the engine as malformed grammar (e.g. '@content:(ps | aux | | | grep | python)') and raised a syntax error that failed the whole agent run; the model was never called. Escape the search text with a TokenEscaper configured for the RediSearch syntax characters before building either query. Spaces stay unescaped, so the terms keep tokenizing and BM25 scoring works as before. Tag filter values were already escaped through _vector_store's escaper; only the free-text path was exposed. Fixes microsoft#10002
…oryProvider RedisHistoryProvider treated redis_client, redis_url and credential_provider as strictly mutually exclusive, so a working caller-owned client passed together with a stray redis_url (or credential provider) raised at construction instead of using the injected client. The Redis vector-store constructors already follow the intended behavior: they bypass URL and .env connection settings whenever redis_client is supplied. Give redis_client the same precedence in the history provider: the mutual-exclusion and host checks only apply to provider-created clients, so the injected client stays caller-owned and the other settings are never validated. Fixes microsoft#9966
|
Fish (@wxhking) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
🟡 Changes recommended
Query escaping also alters the text passed to aembed(), potentially changing hybrid rankings; embeddings should use the original stripped text, with regression coverage asserting that input.
1 open issue
What changed in this PR
This PR aligns RedisHistoryProvider with Redis vector-store behavior by prioritizing caller-owned clients, addressing #9966. It also adds query-syntax escaping to Redis context searches.
Changes:
- Prefer injected history clients over other connection settings while preserving ownership and compatibility checks.
- Escape user text before constructing text and hybrid search queries.
- Add regression tests for client precedence and query escaping.
| File | Description |
|---|---|
python/packages/redis/tests/test_providers.py |
Tests injected-client precedence and caller ownership. |
python/packages/redis/tests/test_context_provider_edges.py |
Tests punctuation escaping in text and hybrid searches. |
python/packages/redis/agent_framework_redis/_history_provider.py |
Updates connection validation and documents client precedence. |
python/packages/redis/agent_framework_redis/_context_provider.py |
Adds escaping for Redis search text. |
Review effort: Balanced
| self._validate_filters() | ||
|
|
||
| q = (text or "").strip() | ||
| q = _SEARCH_TEXT_ESCAPER.escape((text or "").strip()) |

Fixes #9966
Summary
RedisHistoryProvidertreatedredis_client,redis_urlandcredential_provideras strictly mutually exclusive, so a working caller-owned client passed together with a strayredis_url(or credential provider) raisedValueError: redis_client, redis_url, and credential_provider are mutually exclusiveat construction instead of using the injected client.The Redis vector-store constructors already follow the intended behavior: they bypass URL, credential and host settings whenever
redis_clientis supplied. This gives the history provider the same precedence:host is requiredcheck only applies when the provider will build a client itself_owns_client=False) and nothing about it is validated beyond the existing type /decode_responseschecksTest plan
test_borrowed_client_takes_precedence_over_other_connection_sources(parametrized overredis_url/credential_provider+host): the injected client is used and no mutual-exclusion error is raised — replaces the test that pinned the old rejectiontest_url_and_credential_stay_mutually_exclusive_without_clientkeeps the no-client case covered (already existed astest_both_url_and_credential_raises)pytest tests -m 'not integration'inpython/packages/redis— 337 passed, 58 deselected (integration), no new failures