fix(rag): forward retrieval_filter from retrieval_config to vector store search - #34427
Conversation
…B search Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR forwards RAG retrieval filters to vector-store searches and preserves upstream error status codes.
Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain within the scope of this follow-up review. Important Files Changed
Reviews (3): Last reviewed commit: "fix(proxy): surface the upstream status ..." | Re-trigger Greptile |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…in search Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptile the P1 (top-level |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…etrieval_filter # Conflicts: # litellm/rag/main.py # tests/test_litellm/rag/test_main.py
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b4f9e31. Configure here.
TLDR
Problem this solves:
/v1/rag/querydroppedretrieval_config.retrieval_filteron the way to the vector storeHow it solves it:
filtersargumentretrieval_config.filtersand the SDK's top-levelfilterskeyword too,retrieval_filterwinningUser Flow
Before: a developer who scopes a RAG query to one document partition gets an answer built from documents outside it
retrieval_config: {"vector_store_id": "<knowledge base id>", "custom_llm_provider": "bedrock", "top_k": 5, "retrieval_filter": {"andAll": [{"equals": {"key": "department", "value": "billing"}}, {"equals": {"key": "doc_type", "value": "manual"}}]}}choices[0].message.provider_specific_fields.search_results.datalists chunks whoseattributes.departmentissupportorsales, and the answer quotes themAfter: the same request only retrieves chunks from the filtered partition, so the answer is built from the right documents
choices[0].message.provider_specific_fields.search_results.datacarriesattributes.department: "billing"andattributes.doc_type: "manual"; the other partitions no longer appearretrieval_config.filtersworks as an alias ofretrieval_filter, and when both are setretrieval_filterwinsandAllwith a single clause, say) comes back as a 400 carrying Bedrock's validation messageRelevant issues
Affected release
Linear ticket
Resolves LIT-4754
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Recording: Slack thread. Before was 226b1e1, after was b4f9e31, and current-main merge candidate d03556e710 was revalidated
Shared setup for both legs: one config, real Bedrock Knowledge Base
L7INRFMVQT(us-west-2, five ingested text files,x-amz-bedrock-kb-source-urimetadata on each chunk), realbedrock/us.anthropic.claude-sonnet-5completions, no mocks. Each leg is its own worktree and proxy booted with--num_workers 2on a random port (Before 59140 at the merge base, After 39860 at the PR tip). The three cases are the same on both sides: the customer'sretrieval_filtershape (anandAllof two clauses that onlytest_auth_v2.txtsatisfies), thefiltersalias (a plainequalsontest_bedrock_71f3a050.txt), a no-filter control, and a filter Bedrock rejects (anandAllwith one clause)Before (226b1e1)
retrieval_filter (customer shape, andAll of two clauses)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails){ "files": [ "basic_ingest_a0a725e7.txt", "ingest_query_c500aeea.txt", "test_bedrock_a9a4a586.txt", "test_bedrock_71f3a050.txt", "test_bedrock_61f947df.txt" ], "answer": "Looking at the context provided, there are **five different test documents**, each associated with a different ingestion" }filters alias (plain equals)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails){ "files": [ "basic_ingest_a0a725e7.txt", "ingest_query_c500aeea.txt", "test_bedrock_a9a4a586.txt", "test_bedrock_71f3a050.txt", "test_bedrock_61f947df.txt" ], "answer": "I don't see a specific document ID indicated in your question, and the context contains multiple test documents with dif" }no filter (control)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails){ "files": [ "basic_ingest_a0a725e7.txt", "ingest_query_c500aeea.txt", "test_bedrock_a9a4a586.txt", "test_bedrock_71f3a050.txt", "test_bedrock_61f947df.txt" ], "answer": "Based on the context provided, there isn't a single definitive answer since multiple test documents are listed with diff" }malformed filter (andAll with one clause, which Bedrock rejects)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails)After (b4f9e31)
retrieval_filter (customer shape, andAll of two clauses)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails){ "files": [ "test_auth_v2.txt" ], "answer": "This document was ingested using **BaseAWSLLM authentication**.\n\nAccording to the context provided, LiteLLM supports mul" }filters alias (plain equals)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails){ "files": [ "test_bedrock_71f3a050.txt" ], "answer": "Based on the context provided, this document (Test document 71f3a050) was ingested for **Bedrock Knowledge Base** purpos" }no filter (control)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails){ "files": [ "basic_ingest_a0a725e7.txt", "ingest_query_c500aeea.txt", "test_bedrock_a9a4a586.txt", "test_bedrock_71f3a050.txt", "test_bedrock_61f947df.txt" ], "answer": "Based on the context provided, there are multiple documents mentioned, each indicating a different ingestion method:\n\n- " }malformed filter (andAll with one clause, which Bedrock rejects)
Run the query against the live proxy
Observed output (files =
search_resultschunks the proxy retrieved, answer = first 120 characters of the completion, or the status and error body when the query fails)Observations from the run:
Type
🐛 Bug Fix
Caveats (if any)
Low
/v1/rag/querynow carries the upstream status code (404 for an unknown knowledge base, say) where it used to be a 500; the{"detail": {"error": ...}}body is unchangedfiltersremains SDK-only; raw/v1/rag/queryrequests must place filters insideretrieval_configFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/359598ee8147428892f5f8da6f9492c1
Note
Medium Risk
Changes RAG retrieval semantics (invalid filters now fail instead of being ignored) and alters HTTP status codes on
/v1/rag/queryerrors, which may affect clients that assumed 500 for all failures.Overview
Fixes RAG queries that ignored metadata filters, so scoped retrieval (e.g. Bedrock Knowledge Base
retrieval_filter) now reachesvector_stores.asearchasfilters. Resolution order isretrieval_config.retrieval_filter, thenretrieval_config.filters, then the SDKfilterskwarg; when several are set,retrieval_filterwins.The proxy
/v1/rag/queryerror path no longer maps every failure to 500—it uses_upstream_status_codeso provider errors like 400 (bad filter) or 404 (unknown KB) propagate while keeping the same{"detail": {"error": ...}}body.RAGRetrievalConfigtypes now documentretrieval_filterand the filter fields.Reviewed by Cursor Bugbot for commit b4f9e31. Bugbot is set up for automated code reviews on this repo. Configure here.
Open in Devin Desktop: https://app.devin.ai/desktop/session/359598ee8147428892f5f8da6f9492c1?variant=devin