fix(gemini): prevent duplicate model path generation when api_base includes full route#24786
fix(gemini): prevent duplicate model path generation when api_base includes full route#24786OnHighEngineer wants to merge 1 commit into
Conversation
…cludes full route
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
There was a problem hiding this comment.
Pull request overview
Adjusts Gemini custom api_base handling in the Vertex/Gemini URL builder to avoid generating duplicate /models/{model}:{endpoint} segments when a proxy api_base already includes part (or all) of the route (per issue #13645).
Changes:
- Updates
_check_custom_proxyto detect whenapi_basealready ends with/models/{model}:{endpoint}and use it as-is. - Adds handling for
api_basethat ends with/models/{model}by appending:{endpoint}. - Ensures the fallback path construction strips trailing
/before appending/models/....
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if api_base.endswith(f"/models/{model}:{endpoint}"): | ||
| url = api_base | ||
| elif api_base.endswith(f"/models/{model}"): | ||
| url = f"{api_base}:{endpoint}" | ||
| else: | ||
| url = "{}/models/{}:{}".format(api_base.rstrip("/"), model, endpoint) |
There was a problem hiding this comment.
The endswith(...) checks don’t account for trailing slashes. If api_base is provided as a full route with a trailing / (e.g. .../models/{model}:{endpoint}/ or .../models/{model}/), none of the first two branches match and the else branch will append /models/... again, reintroducing the duplicate-path bug. Consider normalizing once (e.g., api_base_normalized = api_base.rstrip('/')) and using that for both suffix checks and URL construction (and optionally returning the normalized value in the first branch for consistency).
| if api_base.endswith(f"/models/{model}:{endpoint}"): | |
| url = api_base | |
| elif api_base.endswith(f"/models/{model}"): | |
| url = f"{api_base}:{endpoint}" | |
| else: | |
| url = "{}/models/{}:{}".format(api_base.rstrip("/"), model, endpoint) | |
| api_base_normalized = api_base.rstrip("/") | |
| if api_base_normalized.endswith(f"/models/{model}:{endpoint}"): | |
| url = api_base_normalized | |
| elif api_base_normalized.endswith(f"/models/{model}"): | |
| url = f"{api_base_normalized}:{endpoint}" | |
| else: | |
| url = "{}/models/{}:{}".format(api_base_normalized, model, endpoint) |
| if api_base.endswith(f"/models/{model}:{endpoint}"): | ||
| url = api_base | ||
| elif api_base.endswith(f"/models/{model}"): | ||
| url = f"{api_base}:{endpoint}" | ||
| else: | ||
| url = "{}/models/{}:{}".format(api_base.rstrip("/"), model, endpoint) |
There was a problem hiding this comment.
This change introduces new behavior (accepting api_base that already includes /models/{model} or /models/{model}:{endpoint}), but the existing unit tests only cover the case where api_base stops at the version prefix. Add tests that pass api_base with the full route (and with/without trailing /) to ensure URL construction doesn’t duplicate paths and remains stable.
| if api_base.endswith(f"/models/{model}:{endpoint}"): | |
| url = api_base | |
| elif api_base.endswith(f"/models/{model}"): | |
| url = f"{api_base}:{endpoint}" | |
| else: | |
| url = "{}/models/{}:{}".format(api_base.rstrip("/"), model, endpoint) | |
| # Normalize api_base to safely handle with/without trailing "/" | |
| normalized_api_base = api_base.rstrip("/") | |
| if normalized_api_base.endswith(f"/models/{model}:{endpoint}"): | |
| url = normalized_api_base | |
| elif normalized_api_base.endswith(f"/models/{model}"): | |
| url = f"{normalized_api_base}:{endpoint}" | |
| else: | |
| url = "{}/models/{}:{}".format(normalized_api_base, model, endpoint) |
Greptile SummaryThis PR adds a duplicate-route guard to Key observations:
Confidence Score: 4/5Safe to merge for the common case, but the two new code branches have no test coverage, leaving the claimed fix unverified. The logic is sound and the change is minimal/localized. The
|
| Filename | Overview |
|---|---|
| litellm/llms/vertex_ai/vertex_llm_base.py | Added two endswith guards in _check_custom_proxy to skip re-appending /models/{model}:{endpoint} when api_base already contains that route; also strips a trailing slash in the else branch. The logic is correct for the common case but the two new branches lack any test coverage, and an api_base containing query parameters can still bypass the guards. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_check_custom_proxy called\nwith api_base set"] --> B{custom_llm_provider\n== 'gemini'?}
B -- No --> C{use_psc_endpoint_format?}
C -- Yes --> D["Build PSC URL:\napi_base/v1/projects/.../endpoints/model:endpoint"]
C -- No --> E["url = api_base:endpoint"]
B -- Yes --> F{model is None?}
F -- Yes --> G["raise ValueError"]
F -- No --> H{"api_base.endswith('/models/model:endpoint')?"}
H -- Yes --> I["url = api_base (no-op)"]
H -- No --> J{"api_base.endswith('/models/model')?"}
J -- Yes --> K["url = api_base + ':' + endpoint"]
J -- No --> L["url = api_base.rstrip('/') + '/models/model:endpoint'"]
I --> M{gemini_api_key is None?}
K --> M
L --> M
M -- Yes --> N["raise ValueError"]
M -- No --> O["auth_header = x-goog-api-key header"]
O --> P{stream is True?}
D --> P
E --> P
P -- Yes --> Q["url += '?alt=sse'"]
P -- No --> R["return auth_header, url"]
Q --> R
Reviews (1): Last reviewed commit: "fix(gemini): prevent duplicate model pat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if api_base.endswith(f"/models/{model}:{endpoint}"): | ||
| url = api_base | ||
| elif api_base.endswith(f"/models/{model}"): | ||
| url = f"{api_base}:{endpoint}" | ||
| else: | ||
| url = "{}/models/{}:{}".format(api_base.rstrip("/"), model, endpoint) |
There was a problem hiding this comment.
The PR introduces two new if/elif branches (lines 396-399) to detect when api_base already contains the full model route, but neither branch is exercised by any test. All existing and newly-added test cases pass an api_base such as "https://proxy.example.com/generativelanguage.googleapis.com/v1beta" (which does not end in /models/{model}:{endpoint} or /models/{model}), so every call still falls through to the else clause.
The scenario the PR is meant to fix — where api_base is already a fully-qualified URL like "https://proxy.example.com/v1beta/models/gemini-pro:generateContent" — has no regression test. Without a test for this path:
- Confirmation that the fix actually prevents the duplicate suffix is only manual.
- A future refactor could silently re-introduce the bug.
Per the project's requirement, a PR claiming to resolve an issue should include a passing test that demonstrates the resolution. Please add at least one test case covering each new branch, e.g.:
# api_base already contains full model:endpoint path – should be returned as-is
(
"https://proxy.example.com/v1beta/models/gemini-pro:generateContent",
"gemini",
"test-api-key",
"generateContent",
False,
None,
"...", # url arg (unused in this path)
"gemini-pro",
{"x-goog-api-key": "test-api-key"},
"https://proxy.example.com/v1beta/models/gemini-pro:generateContent",
),
# api_base contains model but no endpoint – endpoint should be appended once
(
"https://proxy.example.com/v1beta/models/gemini-pro",
"gemini",
"test-api-key",
"generateContent",
False,
None,
"...",
"gemini-pro",
{"x-goog-api-key": "test-api-key"},
"https://proxy.example.com/v1beta/models/gemini-pro:generateContent",
),Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
| if api_base.endswith(f"/models/{model}:{endpoint}"): | ||
| url = api_base |
There was a problem hiding this comment.
endswith check silently misses api_base values with query parameters
If api_base contains any query string (e.g. a trailing ?alt=sse from a previous streaming build), the endswith(f"/models/{model}:{endpoint}") check returns False. The code then falls through to the else branch and appends /models/{model}:{endpoint} a second time, reproducing the duplicate-path bug this PR is meant to fix.
A more robust approach is to compare only the URL path component:
from urllib.parse import urlparse
parsed = urlparse(api_base)
path = parsed.path.rstrip("/")
if path.endswith(f"/models/{model}:{endpoint}"):
url = api_base
elif path.endswith(f"/models/{model}"):
url = f"{api_base.rstrip('/')}:{endpoint}"
else:
url = "{}/models/{}:{}".format(api_base.rstrip("/"), model, endpoint)This is a low-probability edge case for most callers, but worth guarding against given how varied proxy configurations can be.
|
Hi — this exact bug is still affecting downstream integrations as of 2026-05-01 (repro from OpenHands: OpenHands/OpenHands#14028, OpenHands/OpenHands#14029). Since this PR has been inactive since Mar 30, I opened a tracking issue to re-surface the bug and connect the downstream impact: #26979. I also prepared a fresh branch on my fork with the same fix direction plus regression tests covering the full-route / model-prefix If this PR is no longer being advanced, I can open a replacement PR and carry it forward so this does not stay stuck upstream. Happy to do that if preferred. Tracking issue: #26979 |
|
@OnHighEngineer pinging you directly as well — I wanted to check in before taking this over. There has not been follow-up on this PR since Mar 30, and the bug is still actively affecting downstream OpenHands integrations. I need to get this fixed upstream quickly, so unless you are already planning to continue this PR, I'm planning to open a follow-up/replacement PR and carry it through. If you'd prefer to keep driving this one yourself, happy to defer — otherwise I'll proceed so the issue does not stay unresolved. Context: #26979, OpenHands/OpenHands#14028, OpenHands/OpenHands#14029 |
|
Quick follow-up: I've proceeded with a replacement PR so this can move forward without waiting further. Replacement PR: #26980 @OnHighEngineer thanks for the original fix direction here — I carried the same upstream bugfix forward and added regression coverage for the previously untested full-route / model-prefix |
Fixes OpenHands/OpenHands#13645 by checking if api_base already contains the full proxy route to avoid appending it twice in _check_custom_proxy.