Repository navigation
fix(bedrock): forward userContext in Knowledge Base Retrieve requests - #41475
Conversation
The Bedrock vector store search only lifted retrievalConfiguration out of extra_body, so the caller's userContext (the Retrieve API's ACL identity) never reached Bedrock and ACL-enabled data sources answered with zero results. The transform now forwards userContext, taken from extra_body first and then from the top-level params where the OpenAI SDK's extra_body merge lands, as the caller sent it.
🤖 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 caller-provided Bedrock Knowledge Base
Confidence Score: 5/5The PR appears safe to merge, with no outstanding correctness, security, or repository-rule failures identified. The forwarding logic handles both supported field spellings, preserves the intended precedence, omits absent context, and is covered through both transformation and public search paths. The previous typing finding was fully addressed and its thread is resolved. Important Files Changed
Reviews (2): Last reviewed commit: "test(bedrock): type the vector store sea..." | Re-trigger Greptile |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
VANDRANKI
left a comment
There was a problem hiding this comment.
Community review, not a merge-gate approval.
I traced _user_context() in litellm/llms/bedrock/vector_stores/transformation.py. It builds a tuple of (extra_body, litellm_params) filtered to actual Mapping instances (so a None extra_body is dropped, not an error), then does a lazy next() scan over sources -> ("userContext", "user_context") and returns the first key whose value is not None. That gives priority: extra_body["userContext"] > extra_body["user_context"] > litellm_params["userContext"] > litellm_params["user_context"], and it's a real short-circuit since next() stops at the first match rather than evaluating the whole generator.
I checked this against the three new tests in test_bedrock_vector_store_transformation.py:
- extra_body-only userContext forwards correctly, and
retrievalConfigurationfrom the other optional param still comes through unaffected. - litellm_params-only
user_context(snake_case, noextra_bodyat all) forwards correctly, which confirmsNoneextra_body doesn't blow up theisinstance(source, Mapping)filter. - when both extra_body and litellm_params set a (conflicting)
userContext, extra_body wins, matching the priority order I traced above.
The end-to-end test in test_main.py (test_search_forwards_top_level_user_context_to_bedrock_retrieve) goes one level up through vector_stores/main.py's search() and asserts the posted JSON body actually contains userContext, not just that the transformation function's return value has it, so this isn't just a unit-level check in isolation.
One thing I did not verify: whether userId is the only field Bedrock's real KB Retrieve API accepts under userContext, or whether BedrockKBUserContext (a TypedDict with just userId: ReadOnly[str]) is a deliberately narrow first cut. If AWS's Retrieve API supports other userContext fields, this type would need extending later, but that's a scope question, not a bug in what's here.
The rest of the diff (existing test now asserting "userContext" not in body when neither source sets it) is a reasonable regression guard for the negative case too.
This reads as a real, well-scoped, well-tested fix, and I didn't find a logic error tracing it.
|
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 884087f. Configure here.
TLDR
Problem this solves:
userContextextra_bodykey and the OpenAI SDK's top-level shape were lostHow it solves it:
userContext(oruser_context) into the Bedrock Retrieve request bodyextra_bodyfirst, then the top-level key, asretrievalConfigurationalready doesretrievalConfigurationuserIdis personal dataUser Flow
Before: a developer searching an ACL-enabled Bedrock Knowledge Base through the proxy gets no documents back, because the identity they attached never reaches Bedrock
client.vector_stores.search(vector_store_id="T37J8R4WTM", query="What is LiteLLM?", max_num_results=2, extra_body={"userContext": {"userId": "alice@example.com"}})with the OpenAI Python SDK pointed at the proxyPOST https://litellm-domain/v1/vector_stores/T37J8R4WTM/searchwith{"query": "What is LiteLLM?", "max_num_results": 2, "userContext": {"userId": "alice@example.com"}}200with"object": "vector_store.search_results.page", but it was produced without alice's identity: on an ACL-enabled data source such as SharePointdatais empty, and on a store without ACLs everyuserIdgets the same resultsaws bedrock-agent-runtime retrieve ... --user-context 'userId=alice@example.com'returns the documents alice is allowed to seeuserIdthey sendAfter: the same request carries alice's identity to Bedrock, so she gets the documents her ACL grants
client.vector_stores.search(vector_store_id="T37J8R4WTM", query="What is LiteLLM?", max_num_results=2, extra_body={"userContext": {"userId": "alice@example.com"}})with the OpenAI Python SDK pointed at the proxyPOST https://litellm-domain/v1/vector_stores/T37J8R4WTM/searchwith{"query": "What is LiteLLM?", "max_num_results": 2, "userContext": {"userId": "alice@example.com"}}200with"object": "vector_store.search_results.page"and the documents alice is permitted to see, the same set the direct Bedrock call returns{"query": ..., "extra_body": {"userContext": {"userId": "alice@example.com"}}}, gets the same resultuserIdit sends, the same trust Bedrock gives a direct caller; the proxy does not derive theuserIdfrom the key's own userRelevant issues
None on GitHub; reported by a customer through support
Affected release
Linear ticket
Resolves LIT-4415
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
Both legs boot the proxy from a detached worktree at the named commit with 2 uvicorn workers and no database, against the real Bedrock Knowledge Base
T37J8R4WTMin us-west-2. That store has no ACLs, so a valid identity returns the same page whether or not it reaches Bedrock. What separates the legs is Bedrock's own validation of the field: Bedrock rejects an emptyuserContext(and a non-stringuserId) with a400, so a400through the proxy proves the field arrived and a200on the same request proves the proxy dropped itConfig (
config.yaml, keys come from the environment):Boot, same on both legs apart from the checkout (
.envcarries the two AWS keys andLITELLM_MASTER_KEY, noDATABASE_URL):What Bedrock itself says per shape, calling Retrieve directly with the same keys (the AWS CLI validates
userContextclient-side, so{}never leaves the laptop through it; the signedcurlshows the server's verdict):The page every
200below returns is the same two chunks from https://www.litellm.ai with scores0.6238149and0.5865586; itstextfields are shortened with...for length only.$KEYis the proxy master key.openaiSDK 2.33.0Before (4e99640)
OpenAI SDK, valid userContext
OpenAI SDK, empty userContext
extra_body={"userContext": {}}extra_body={"userContext": {"userId": 12345}}curl, userContext nested under extra_body, valid
curl, userContext nested under extra_body, empty
"userContext":{"userId":12345}curl, top-level userContext, valid
curl, top-level userContext, empty
"userContext":{"userId":12345}Every shape got the same
200page, including the two Bedrock rejects outright, so nothing the caller put inuserContextreached BedrockAfter (033aa8b)
The tip 884087f only annotates a test helper on top of this hash; no runtime file changed since, so this leg stands
OpenAI SDK, valid userContext
OpenAI SDK, empty userContext
extra_body={"userContext": {}}extra_body={"userContext": {"userId": 12345}}curl, userContext nested under extra_body, valid
curl, userContext nested under extra_body, empty
"userContext":{"userId":12345}curl, top-level userContext, valid
curl, top-level userContext, empty
"userContext":{"userId":12345}Every proxy status now equals Bedrock's own status for the same shape on all three client paths: the valid identity still returns the page, and the two shapes Bedrock rejects come back as its
400, which is only possible ifuserContextreached itObservations from the legs:
{"userId": ""}returns200on both legs, Bedrock accepts it{}before sending, Bedrock rejects it tooType
🐛 Bug Fix
Caveats (if any)
Low
userIdit sends: the proxy forwards the identity as sent and does not derive it from the key's own user, the same trust Bedrock gives a direct caller and the same pass-through theretrievalConfigurationfilters already get. Kept as is: mapping the key's user ontouserContextwould be a new opt-in feature with its own identity-format decisions (a SharePoint ACL wants the user principal name, not a proxy user id) and would break the shape the ticket describes, an app passingextra_body.userContextfor end users who are not proxy users; the docs state the trust boundaryuserContextis forwarded as sent, no local validation, so a bad shape is Bedrock's400where it used to be silently ignored{}, a non-stringuserId, and a bare string all come back400, while{"userId": ""}is acceptedretrievalConfigurationis not validated locallyuser_contextset on a store invector_store_registryis forwarded too, the same way itsretrievalConfigurationalready isuserContextoverrides it, while a caller'suser_contextloses to it, because the endpoint lays the store'slitellm_paramsover the request body before either spelling is read; kept as is since that merge predates this PR and applies to every keyfile_searchpath the store-level value is the only way to set it, since the tool block has no per-request field; the ticket scopes the fix to the vector store search API--detailed_debugthe shared HTTP handler logs the outbound Retrieve body,userIdincluded, as it does for every request field; this PR adds no logging of its own, and redacting one field there is a change to the shared handler outside this fixretrievalConfigurationon the proxy path is still dropped, pre-existing and out of scopemainsince the merge base touches them: 22 Together AI tests fail withUnable to access modelin 7 of the last 10 pipelines on other branches with no green run anywhere, and the sumologic NDJSON, Responses streaming iterator, router cooldown, andtest_openai_endpoints::test_chat_completiontests fail in 1 or 2 of those 10 with 5 green jobs each; none touches a file this PR changes, and CircleCI is not a required checkFinal Attestation
The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR
033aa8b passes /live-pr-risk