fix(proxy): stop /{provider}/v1/files and /v1/batches from shadowing custom pass_through_endpoints - #38017
Conversation
…custom pass_through_endpoints
Custom pass_through_endpoints entries from config.yaml are registered
during proxy startup, strictly after every built-in router (including
the generic /{provider}/v1/files and /v1/batches routes) is mounted at
module-import time. Since they're always appended to app.routes, the
generic native-provider routes always match first regardless of the
configured prefix, misinterpreting it as a provider name.
SafeRouteAdder now repositions a newly-added route immediately before
the first route whose path template contains "{provider}" -- the
shared marker for every such generic route, present and future -- so
a custom pass-through path always wins the match instead.
Fixes BerriAI#37925
Greptile SummaryThe PR reorders newly registered custom pass-through routes so they take precedence over generic provider file and batch routes.
Confidence Score: 4/5The PR appears safe to merge after addressing the non-blocking shared-route-list mutation concern. The intended custom-route precedence is covered without an established runtime regression, but the implementation directly mutates FastAPI’s shared route collection contrary to the repository’s source-code convention. Files Needing Attention: litellm/proxy/pass_through_endpoints/pass_through_endpoints.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/pass_through_endpoints/pass_through_endpoints.py | Adds custom-route precedence handling; the behavior is focused, but it directly mutates the shared FastAPI route list contrary to repository guidance. |
| tests/test_litellm/proxy/pass_through_endpoints/test_llm_pass_through_endpoints.py | Adds isolated route-resolution regression coverage without weakening existing assertions or making network calls. |
Reviews (1): Last reviewed commit: "fix(proxy): stop /{provider}/v1/files an..." | Re-trigger Greptile
| routes.pop() | ||
| routes.insert(index, new_route) |
There was a problem hiding this comment.
The helper directly mutates the shared app.routes collection with pop() and insert(), making route ownership and future registration changes harder to reason about and maintain.
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!
There was a problem hiding this comment.
Fixed — reassign app.router.routes wholesale (built via one unpacking expression) instead of pop()/insert() on the existing list, per your CLAUDE.md's no-in-place-mutation convention. See commit a099e80.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…)/insert() Addresses Greptile review feedback on BerriAI#38017: avoid in-place mutation of the shared FastAPI route list. Builds the reordered route list via unpacking and reassigns app.router.routes wholesale, rather than popping the appended route and inserting it back in place.
…ric_provider_routes Addresses Codecov patch-coverage gap on BerriAI#38017: the documented safe no-op when no "/{provider}/..." route exists (e.g. a minimal deployment) was previously untested, since the real production app always has one registered.
|
Added a focused unit test for the no-generic-route fallback branch (commit 6f2b7bd) — that path was previously untested since the real production app always has a generic /{provider}/... route registered. |
… fix - routes/new_route: Final, closing the LIT010 rebind-openness gap - # mutable-ok / # rebind-ok on the app.router.routes reassignment: it necessarily constructs a list literal and mutates state reachable from the app parameter, since Starlette's own Router.routes must stay a real, appendable list for the framework's own route registration to keep working -- an immutable rewrite is not possible here, per CLAUDE.md's own last-resort carve-out for this case CI failure: LIT002 total exceeded budget by the 1 new violation this PR added (https://github.com/BerriAI/litellm/pull/38017/checks).
TLDR
Problem this solves:
pass_through_endpointsprefix is always shadowed by/{provider}/v1/filesand/v1/batchesHow it solves it:
SafeRouteAddermoves a newly-registered custom route to sit before the first generic/{provider}/...routeUser Flow
Before: an operator running a pass_through_endpoints entry under a custom prefix (e.g. a self-hosted Anthropic-compatible endpoint reached via
/claude-aws) cannot upload documents or create batches through the gateway at all.pass_through_endpointsentry forpath: /claude-aws/v1/filesinconfig.yaml, targeting their own hostPOST https://litellm-domain/claude-aws/v1/fileswith a file, nopurposefield (the Anthropic SDK never sends one)422 {"detail":[{"type":"missing","loc":["body","purpose"],"msg":"Field required"}]}purpose=user_dataand retry the same request500 {"error":{"message":"files_settings is not set, set it on your config.yaml file."}}POST https://litellm-domain/claude-aws/v1/batcheswith a batch payload400 {"error":{"message":"/batches: Missing required parameter: 'input_file_id'."}}-- the request never reached their configured target at allAfter: the same requests reach the operator's configured target instead of being intercepted locally.
config.yamlentryPOST https://litellm-domain/claude-aws/v1/fileswith a file, no extra fieldsPOST https://litellm-domain/claude-aws/v1/batcheswith a batch payloadRelevant issues
Fixes #37925
Linear ticket
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
Config used for both runs (
config.yaml), against a self-hosted echo target so the proof needs no LLM provider credentials or real spend -- the bug is pure route-matching, upstream of any provider call:Run:
litellm --config config.yaml --port 4010Before (f005afa)
files
curl -X POST http://127.0.0.1:4010/claude-aws/v1/files -H "Authorization: Bearer sk-local-repro-master" -F "file=@probe.txt;type=text/plain"-F "purpose=user_data"added:batches
curl -X POST http://127.0.0.1:4010/claude-aws/v1/batches -H "Authorization: Bearer sk-local-repro-master" -H "Content-Type: application/json" -d '{"probe":"37925"}'After (26bcafc)
files
curl -X POST http://127.0.0.1:4010/claude-aws/v1/files -H "Authorization: Bearer sk-local-repro-master" -F "file=@probe.txt;type=text/plain"x-repro-headerand the base64-decoded file content (probe file for litellm passthrough repro) both round-tripped correctly.batches
curl -X POST http://127.0.0.1:4010/claude-aws/v1/batches -H "Authorization: Bearer sk-local-repro-master" -H "Content-Type: application/json" -d '{"probe":"37925"}'x-repro-headerand the echoed body both round-tripped correctly.Type
🐛 Bug Fix
✅ Test
Caveats (if any)
app.routesmay shift their relative order in/docs(OpenAPI UI); purely cosmetic, no functional impactbatchesshadowing symptom differs fromfiles: it 400s on a missing OpenAI-specificinput_file_idrather than 500ing onfiles_settings, since batch creation has no equivalent required-field gate before the provider mismatch would otherwise surface -- same root cause, different downstream error shape/anthropicprefix; this PR generalizes to any custompass_through_endpointsprefix and is orthogonal to that fix (neither conflicts with the other regardless of merge order)Final Attestation