-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: append /v1 to OpenAI Compatible base URLs for llama.cpp compatibility #7066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ility - Automatically append /v1 to base URLs that are plain URLs without a path - This fixes compatibility with llama.cpp servers that expose OpenAI-compatible endpoints - Added tests to verify the URL handling works correctly Fixes #7065
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
try { | ||
const url = new URL(baseURL) | ||
// If the pathname is just '/', append /v1 | ||
if (url.pathname === "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation only appends /v1
when the pathname is exactly /
. What happens with URLs like http://localhost:8080/api
or http://localhost:8080/openai
? These might also be base URLs that need /v1
appended. Should we consider a more flexible approach or document the expected URL format clearly?
if (url.pathname === "/") { | ||
baseURL = baseURL.replace(/\/$/, "") + "/v1" | ||
} | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When URL parsing fails, the code silently continues with the original URL. Would it be helpful to log a warning so users know their URL might not be processed as expected?
} catch { | |
} catch (error) { | |
// If URL parsing fails, leave it as is but warn the user | |
console.warn('Failed to parse OpenAI base URL, using as-is:', baseURL, error); |
openAiNativeBaseUrl: "http://127.0.0.1:8080", | ||
}) | ||
expect(handler1).toBeInstanceOf(OpenAiNativeHandler) | ||
// The client should have the /v1 appended internally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests verify that the handler instantiates correctly, but they don't actually verify that the /v1
is appended to the client's baseURL. Since the OpenAI client is mocked, we can't directly inspect its configuration. Could we consider adding a way to verify the actual URL being used, perhaps by checking the mock calls or adding integration tests?
@@ -57,7 +57,23 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio | |||
this.options.enableGpt5ReasoningSummary = true | |||
} | |||
const apiKey = this.options.openAiNativeApiKey ?? "not-provided" | |||
this.client = new OpenAI({ baseURL: this.options.openAiNativeBaseUrl, apiKey }) | |||
|
|||
// Handle base URL - append /v1 if it's a plain URL without a path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment explaining why /v1
is appended and which servers (like llama.cpp) expect this format. This would help future maintainers understand the reasoning.
// Handle base URL - append /v1 if it's a plain URL without a path | |
// Handle base URL - append /v1 if it's a plain URL without a path | |
// This is needed for compatibility with llama.cpp and other OpenAI-compatible servers | |
// that expose their endpoints at /v1 but users typically provide just the base URL |
Summary
This PR fixes an issue where OpenAI Compatible mode was not working with llama.cpp servers. The problem was that llama.cpp exposes OpenAI-compatible endpoints at URLs like
http://127.0.0.1:8080/v1
, but users typically provide just the base URLhttp://127.0.0.1:8080
.Changes
/v1
to base URLs that are plain URLs without a path componentTesting
Added unit tests that verify:
/v1
appended (e.g.,http://127.0.0.1:8080
→http://127.0.0.1:8080/v1
)/v1
are left unchangedAll existing tests pass
Linting and type checking pass
Impact
This change makes Roo Code compatible with llama.cpp servers and other OpenAI-compatible servers that follow the standard OpenAI API URL structure. Users can now use llama.cpp with Roo Code by simply providing the server URL without needing to manually append
/v1
.Fixes #7065
Important
Fixes URL handling in
OpenAiNativeHandler
to append/v1
for compatibility with llama.cpp servers, with comprehensive tests added.OpenAiNativeHandler
, automatically appends/v1
toopenAiNativeBaseUrl
if it's a plain URL without a path.openai-native.spec.ts
to verify URL handling:/v1
to plain URLs./v1
or other paths unchanged.This description was created by
for 3c89c43. You can customize this summary. It will automatically update as commits are pushed.