fix(proxy): keep the model-group alias through vector store credential merges - #36104
fix(proxy): keep the model-group alias through vector store credential merges#36104mateo-berri wants to merge 1 commit into
Conversation
…l merges Since #24548, get_deployment_credentials_with_provider unconditionally injects the deployment's litellm_params.model into the returned credentials. Router-bound vector store call sites merged that dict over data, so the request left the endpoint carrying the underlying provider model instead of the alias the caller named. The router then resolved the deployment via the specific-deployment path, which skips access-group filtering, and when several model groups share one provider model the request could be served under the wrong group. Add a routing_model parameter to prepare_data_with_credentials and pass the resolved alias at the three router-bound merge sites in the vector store files endpoints. Direct-to-SDK call sites (batches, files) are unchanged and keep receiving the underlying model, preserving the #25104 fix. Fixes #36103 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile SummaryThe PR adds an optional routing-model override to credential merging so router-bound vector-store requests retain their model-group alias while direct SDK paths keep the provider model.
Confidence Score: 3/5The PR should not merge until alias preservation also covers the legacy simple encoded-file path; the newly added test comments are non-blocking cleanup. Router-bound vector-store operations using legacy encoded file IDs still merge credentials without restoring the resolved alias, allowing dispatch to proceed under the underlying deployment identity instead. Files Needing Attention: litellm/proxy/vector_store_files_endpoints/endpoints.py and the modified vector-store endpoint tests
|
| Filename | Overview |
|---|---|
| litellm/proxy/openai_files_endpoints/common_utils.py | Adds a backward-compatible routing_model override while retaining the existing provider-model behavior for callers that omit it. |
| litellm/proxy/vector_store_files_endpoints/endpoints.py | Preserves aliases at three merge sites but misses the router-bound simple encoded-file branch. |
| tests/test_litellm/proxy/test_model_based_routing_files_batches.py | Covers both alias-preserving and legacy provider-model credential merges without real network calls. |
| tests/test_litellm/proxy/vector_store_endpoints/test_vector_store_endpoints.py | Updates model-hint and team-fallback expectations correctly, but adds comments prohibited by repository guidance. |
| tests/test_litellm/proxy/vector_store_endpoints/test_vector_store_tenant_guard.py | Updates the unified managed-vector-store assertion correctly, but adds a prohibited explanatory comment. |
Reviews (1): Last reviewed commit: "fix(proxy): keep the model-group alias t..." | Re-trigger Greptile
| data=data, | ||
| credentials=credentials, | ||
| file_id=llm_output_file_id, # Use the actual provider file ID | ||
| routing_model=routing_model, |
There was a problem hiding this comment.
When a vector-store operation uses a simple encoded file ID containing a model-group alias, the sibling branch merges credentials without passing model_used as routing_model. This replaces the alias with the underlying deployment model, causing incorrect group routing and attribution or a model-not-found response. How this was verified: The encoded-ID branch flows into router dispatch without any later assignment restoring model_used after the credential merge.
Rule Used: What: For security fix PRs, NEVER mention what sec... (source)
Knowledge Base Used: Proxy Server Request Flow
| assert result["api_key"] == "sk-team-openai" | ||
| assert result["api_base"] == "https://api.openai.com/v1" | ||
| assert result["model"] == "openai/gpt-4o-mini" | ||
| # routing stays on the model-group alias, not the deployment's provider model (#36103) |
There was a problem hiding this comment.
New comments violate repository guidance
This change adds explanatory inline comments here and at the other modified alias-routing assertions, despite the repository instruction prohibiting new comments unless explicitly requested. Remove the four redundant comments while retaining the assertions.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
If the description isn't updated in the next 24 hours, I'll auto-close this PR. That's not us saying we don't care about the change; we want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later," not a rejection. Take your time; everything below still works after the close. During the grace period: just update the PR description with the missing pieces. No need to ping me; I'll re-check on the next sweep and skip the auto-close if it now passes. See what counts as QA proof for the full rubric (a linked issue alone isn't enough; it covers context, not proof). If the PR does get auto-closed in 24 hours, you still have easy recovery paths:
Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, ping a maintainer; they'll override me.) |
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
Closing this PR isn't a rejection of the change. We want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later"; your work is still here, the diff is still here, and getting it reopened is one comment away. Take your time. To bring this PR back:
What "end-to-end QA proof" means, since it's the most common gap: at least one of a short before/after screen recording / video (the bug reproducing, then the fix working; for a brand-new feature, a recording of it working end-to-end), a screenshot (or before/after screenshots) of it working, or the exact commands you ran paired with their real output against the real system. Running Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, comment |
Relevant issues
Fixes #36103
What this does
Since #24548,
get_deployment_credentials_with_providerunconditionally injects the deployment'slitellm_params.modelinto the returned credentials. The router-bound merge sites in the vector store files endpoints copied that dict overdata, so requests left the endpoint carrying the underlying provider model instead of the alias the caller named. Inroute_requestthe underlying model matchesllm_router.deployment_names, which dispatches withspecific_deployment=Trueand skips access-group filtering entirely, so when several model groups share onelitellm_params.model(different access groups, team scoping, per-group settings) the request can be served under the wrong group and spend/logs attribute to it.The fix adds a
routing_modelparameter toprepare_data_with_credentialsand passes the resolved alias at the three router-bound merge sites inlitellm/proxy/vector_store_files_endpoints/endpoints.py(unified vector store id path, model-hint path, single-OpenAI-team-deployment fallback). Direct-to-SDK call sites (batches, files) don't pass it and keep receiving the underlying model, so the #25104 fix from #24548 is preserved; a test pins each behavior.Test coverage
TestPrepareDataWithCredentials::test_routing_model_keeps_alias_over_credentials_model— alias survives the merge when two groups share one provider modelTestPrepareDataWithCredentials::test_credentials_model_lands_in_data_without_routing_model— direct-to-SDK sites still get the deployment model (Bedrock batch inference broken via proxy: credential fields and model silently dropped #25104)uv run pytest tests/test_litellm/proxy/vector_store_endpoints/ tests/test_litellm/proxy/test_model_based_routing_files_batches.py tests/test_litellm/proxy/batches_endpoints/ tests/test_litellm/proxy/test_batch_x_litellm_model_encoding.py tests/test_litellm/proxy/openai_files_endpoint/— 244 passed🤖 Generated with Claude Code