mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 18:18:37 +00:00
fix(providers): normalize Ollama api_base URL to include /v1 suffix
- Write-time normalization via normalizeOllamaAPIBase() on create/update - Read-time safety net in resolveAPIBase() for pre-existing DB records - Covers both ProviderOllama and ProviderOllamaCloud - Stop hardcoding +"/v1" in registerInMemory/startup registration Closes #654
This commit is contained in:
1 parent
9050426840
commit
80c2c0e0ee
3 files changed
+320
-8
No files matched your search
@@ -284,12 +284,13 @@ func registerProvidersFromDB(registry *providers.Registry, provStore store.Provi
|
||||
continue
|
||||
}
|
||||
// Local Ollama requires no API key — handle before the key guard (same pattern as ClaudeCLI).
|
||||
// api_base is stored with /v1 (normalized at write time), so no suffix appending needed.
|
||||
if p.ProviderType == store.ProviderOllama {
|
||||
host := p.APIBase
|
||||
if host == "" {
|
||||
host = "http://localhost:11434"
|
||||
host = "http://localhost:11434/v1"
|
||||
}
|
||||
registry.RegisterForTenant(p.TenantID, providers.NewOpenAIProvider(p.Name, "ollama", config.DockerLocalhost(host+"/v1"), "llama3.3"))
|
||||
registry.RegisterForTenant(p.TenantID, providers.NewOpenAIProvider(p.Name, "ollama", config.DockerLocalhost(host), "llama3.3"))
|
||||
slog.Info("registered provider from DB", "name", p.Name)
|
||||
continue
|
||||
}
|
||||
|
||||
@@ -76,14 +76,24 @@ func (h *ProvidersHandler) SetAgentStore(as store.AgentCRUDStore) {
|
||||
}
|
||||
|
||||
// resolveAPIBase returns the provider's api_base, falling back to config/env if empty.
|
||||
// For Ollama/OllamaCloud providers, applies a safety-net normalization: if the stored
|
||||
// value is missing the /v1 suffix (pre-existing record before write-time normalization),
|
||||
// the suffix is appended so all downstream call sites receive a ready-to-use URL.
|
||||
func (h *ProvidersHandler) resolveAPIBase(p *store.LLMProviderData) string {
|
||||
base := ""
|
||||
if p.APIBase != "" {
|
||||
return p.APIBase
|
||||
base = p.APIBase
|
||||
} else if h.apiBaseFallback != nil {
|
||||
base = h.apiBaseFallback(p.ProviderType)
|
||||
}
|
||||
if h.apiBaseFallback != nil {
|
||||
return h.apiBaseFallback(p.ProviderType)
|
||||
// Safety net: normalize Ollama URLs missing /v1 (pre-existing DB records).
|
||||
if base != "" && (p.ProviderType == store.ProviderOllama || p.ProviderType == store.ProviderOllamaCloud) {
|
||||
base = strings.TrimRight(base, "/")
|
||||
if !strings.HasSuffix(base, "/v1") {
|
||||
base += "/v1"
|
||||
}
|
||||
}
|
||||
return ""
|
||||
return base
|
||||
}
|
||||
|
||||
// emitProviderCacheInvalidate broadcasts a provider cache invalidation event.
|
||||
@@ -164,12 +174,13 @@ func (h *ProvidersHandler) registerInMemory(p *store.LLMProviderData) {
|
||||
}
|
||||
// Ollama doesn't need an API key — handle before the key guard (same as startup).
|
||||
// In Docker, swap localhost → host.docker.internal so the container can reach the host.
|
||||
// api_base is stored with /v1 (normalized at write time), so no suffix appending needed.
|
||||
if p.ProviderType == store.ProviderOllama {
|
||||
host := p.APIBase
|
||||
if host == "" {
|
||||
host = "http://localhost:11434"
|
||||
host = "http://localhost:11434/v1"
|
||||
}
|
||||
h.providerReg.RegisterForTenant(p.TenantID, providers.NewOpenAIProvider(p.Name, "ollama", config.DockerLocalhost(host+"/v1"), "llama3.3"))
|
||||
h.providerReg.RegisterForTenant(p.TenantID, providers.NewOpenAIProvider(p.Name, "ollama", config.DockerLocalhost(host), "llama3.3"))
|
||||
return
|
||||
}
|
||||
if p.APIKey == "" {
|
||||
@@ -210,6 +221,22 @@ func (h *ProvidersHandler) registerInMemory(p *store.LLMProviderData) {
|
||||
}
|
||||
}
|
||||
|
||||
// normalizeOllamaAPIBase ensures Ollama and OllamaCloud api_base values include the
|
||||
// /v1 suffix required for OpenAI-compatible endpoints. Normalizing at write time means
|
||||
// resolveAPIBase() always returns a ready-to-use base URL.
|
||||
func normalizeOllamaAPIBase(p *store.LLMProviderData) {
|
||||
if p.ProviderType != store.ProviderOllama && p.ProviderType != store.ProviderOllamaCloud {
|
||||
return
|
||||
}
|
||||
if p.APIBase == "" {
|
||||
return
|
||||
}
|
||||
p.APIBase = strings.TrimRight(p.APIBase, "/")
|
||||
if !strings.HasSuffix(p.APIBase, "/v1") {
|
||||
p.APIBase += "/v1"
|
||||
}
|
||||
}
|
||||
|
||||
// validateProviderURL rejects provider base URLs pointing to internal/private networks.
|
||||
// Defense-in-depth: prevents SSRF when providers are later used for API calls.
|
||||
func validateProviderURL(rawURL string, providerType string) error {
|
||||
@@ -321,6 +348,10 @@ func (h *ProvidersHandler) handleCreateProvider(w http.ResponseWriter, r *http.R
|
||||
return
|
||||
}
|
||||
|
||||
// Normalize Ollama base URL to include /v1 so all code paths
|
||||
// (chat, model listing, embedding verify) use the same value from DB.
|
||||
normalizeOllamaAPIBase(&p)
|
||||
|
||||
if err := h.store.CreateProvider(r.Context(), &p); err != nil {
|
||||
slog.Error("providers.create", "error", err)
|
||||
writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()})
|
||||
@@ -447,6 +478,12 @@ func (h *ProvidersHandler) handleUpdateProvider(w http.ResponseWriter, r *http.R
|
||||
}
|
||||
}
|
||||
|
||||
// Normalize Ollama base URL to include /v1 so all code paths use the same value.
|
||||
normalizeOllamaAPIBase(&candidate)
|
||||
if candidate.APIBase != currentProvider.APIBase {
|
||||
updates["api_base"] = candidate.APIBase
|
||||
}
|
||||
|
||||
if err := validateChatGPTOAuthProviderCandidate(r.Context(), h.store, id, &candidate); err != nil {
|
||||
writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()})
|
||||
return
|
||||
|
||||
@@ -0,0 +1,274 @@
|
||||
package http
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/providers"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/store"
|
||||
)
|
||||
|
||||
// TestNormalizeOllamaAPIBase validates write-time normalization applied before DB persist.
|
||||
// The function must append /v1, strip redundant trailing slashes, and leave non-Ollama
|
||||
// provider types untouched.
|
||||
func TestNormalizeOllamaAPIBase(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
providerType string
|
||||
input string
|
||||
want string
|
||||
}{
|
||||
// --- Ollama normalization cases ---
|
||||
{
|
||||
name: "bare host:port gets /v1 appended",
|
||||
providerType: store.ProviderOllama,
|
||||
input: "http://host:11434",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "trailing slash is stripped before /v1 is appended",
|
||||
providerType: store.ProviderOllama,
|
||||
input: "http://host:11434/",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "already-normalized /v1 is not doubled",
|
||||
providerType: store.ProviderOllama,
|
||||
input: "http://host:11434/v1",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "trailing slash after /v1 is stripped",
|
||||
providerType: store.ProviderOllama,
|
||||
input: "http://host:11434/v1/",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "empty api_base is left unchanged",
|
||||
providerType: store.ProviderOllama,
|
||||
input: "",
|
||||
want: "",
|
||||
},
|
||||
// --- OllamaCloud also normalized ---
|
||||
{
|
||||
name: "OllamaCloud bare URL gets /v1",
|
||||
providerType: store.ProviderOllamaCloud,
|
||||
input: "https://ollama.com",
|
||||
want: "https://ollama.com/v1",
|
||||
},
|
||||
{
|
||||
name: "OllamaCloud already-normalized /v1 unchanged",
|
||||
providerType: store.ProviderOllamaCloud,
|
||||
input: "https://ollama.com/v1",
|
||||
want: "https://ollama.com/v1",
|
||||
},
|
||||
// --- Non-Ollama types must NOT be touched ---
|
||||
{
|
||||
name: "OpenAI-compat is not modified",
|
||||
providerType: store.ProviderOpenAICompat,
|
||||
input: "http://host:11434",
|
||||
want: "http://host:11434",
|
||||
},
|
||||
{
|
||||
name: "Anthropic native is not modified",
|
||||
providerType: store.ProviderAnthropicNative,
|
||||
input: "https://api.anthropic.com",
|
||||
want: "https://api.anthropic.com",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
p := &store.LLMProviderData{
|
||||
ProviderType: tt.providerType,
|
||||
APIBase: tt.input,
|
||||
}
|
||||
normalizeOllamaAPIBase(p)
|
||||
if p.APIBase != tt.want {
|
||||
t.Fatalf("normalizeOllamaAPIBase() APIBase = %q, want %q", p.APIBase, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveAPIBaseSafetyNet validates the read-time safety net in resolveAPIBase.
|
||||
// Pre-existing DB records (written before write-time normalization) must receive /v1
|
||||
// on read; already-normalized values and non-Ollama types must be returned as-is.
|
||||
func TestResolveAPIBaseSafetyNet(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
providerType string
|
||||
apiBase string
|
||||
want string
|
||||
}{
|
||||
// Ollama missing /v1 — safety net fires
|
||||
{
|
||||
name: "Ollama bare host:port gets /v1 via safety net",
|
||||
providerType: store.ProviderOllama,
|
||||
apiBase: "http://host:11434",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "Ollama trailing slash gets /v1 via safety net",
|
||||
providerType: store.ProviderOllama,
|
||||
apiBase: "http://host:11434/",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
// Ollama already normalized — no double /v1
|
||||
{
|
||||
name: "Ollama already has /v1 — no double append",
|
||||
providerType: store.ProviderOllama,
|
||||
apiBase: "http://host:11434/v1",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "Ollama /v1 with trailing slash — trimmed, no double",
|
||||
providerType: store.ProviderOllama,
|
||||
apiBase: "http://host:11434/v1/",
|
||||
want: "http://host:11434/v1",
|
||||
},
|
||||
// Non-Ollama — must never have /v1 appended by safety net
|
||||
{
|
||||
name: "OpenAI-compat is returned unchanged",
|
||||
providerType: store.ProviderOpenAICompat,
|
||||
apiBase: "http://host:11434",
|
||||
want: "http://host:11434",
|
||||
},
|
||||
{
|
||||
name: "Anthropic native is returned unchanged",
|
||||
providerType: store.ProviderAnthropicNative,
|
||||
apiBase: "https://api.anthropic.com",
|
||||
want: "https://api.anthropic.com",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
h := &ProvidersHandler{}
|
||||
p := &store.LLMProviderData{
|
||||
ProviderType: tt.providerType,
|
||||
APIBase: tt.apiBase,
|
||||
}
|
||||
got := h.resolveAPIBase(p)
|
||||
if got != tt.want {
|
||||
t.Fatalf("resolveAPIBase() = %q, want %q", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolveAPIBaseUsesAPIBaseFallback checks the config/env fallback path: when
|
||||
// the DB value is empty and a fallback is configured, the fallback is used — and
|
||||
// the Ollama safety-net normalization is applied to fallback values too.
|
||||
func TestResolveAPIBaseUsesAPIBaseFallback(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
providerType string
|
||||
storedBase string
|
||||
fallback string
|
||||
want string
|
||||
}{
|
||||
{
|
||||
name: "empty DB, non-Ollama fallback returned as-is",
|
||||
providerType: store.ProviderOpenAICompat,
|
||||
storedBase: "",
|
||||
fallback: "https://api.example.com/v1",
|
||||
want: "https://api.example.com/v1",
|
||||
},
|
||||
{
|
||||
name: "empty DB, Ollama fallback missing /v1 gets safety net",
|
||||
providerType: store.ProviderOllama,
|
||||
storedBase: "",
|
||||
fallback: "http://localhost:11434",
|
||||
want: "http://localhost:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "empty DB, Ollama fallback already has /v1 — no double",
|
||||
providerType: store.ProviderOllama,
|
||||
storedBase: "",
|
||||
fallback: "http://localhost:11434/v1",
|
||||
want: "http://localhost:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "no fallback, empty DB, empty result",
|
||||
providerType: store.ProviderOllama,
|
||||
storedBase: "",
|
||||
fallback: "",
|
||||
want: "",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
h := &ProvidersHandler{
|
||||
apiBaseFallback: func(_ string) string { return tt.fallback },
|
||||
}
|
||||
p := &store.LLMProviderData{
|
||||
ProviderType: tt.providerType,
|
||||
APIBase: tt.storedBase,
|
||||
}
|
||||
got := h.resolveAPIBase(p)
|
||||
if got != tt.want {
|
||||
t.Fatalf("resolveAPIBase() = %q, want %q", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestRegisterInMemoryOllamaURLNormalization verifies that registerInMemory registers the
|
||||
// Ollama provider with a correct base URL (already normalized with /v1 at write time)
|
||||
// and does NOT produce a doubled /v1 in the registered provider.
|
||||
func TestRegisterInMemoryOllamaURLNormalization(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
apiBase string // value as stored in DB (post-normalization)
|
||||
wantURL string // expected URLin the registered provider
|
||||
}{
|
||||
{
|
||||
name: "normalized /v1 is registered unchanged",
|
||||
apiBase: "http://host:11434/v1",
|
||||
wantURL: "http://host:11434/v1",
|
||||
},
|
||||
{
|
||||
name: "empty api_base falls back to default with /v1",
|
||||
apiBase: "",
|
||||
wantURL: "http://localhost:11434/v1",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
providerReg := providers.NewRegistry(nil)
|
||||
handler := NewProvidersHandler(newMockProviderStore(), newMockSecretsStore(), providerReg, "")
|
||||
|
||||
p := &store.LLMProviderData{
|
||||
BaseModel: store.BaseModel{ID: uuid.New()},
|
||||
TenantID: uuid.New(),
|
||||
Name: "my-ollama",
|
||||
ProviderType: store.ProviderOllama,
|
||||
APIBase: tt.apiBase,
|
||||
Enabled: true,
|
||||
}
|
||||
|
||||
handler.registerInMemory(p)
|
||||
|
||||
runtimeProvider, err := providerReg.GetForTenant(p.TenantID, p.Name)
|
||||
if err != nil {
|
||||
t.Fatalf("GetForTenant() error = %v", err)
|
||||
}
|
||||
|
||||
type apiBaseProvider interface {
|
||||
APIBase() string
|
||||
}
|
||||
abp, ok := runtimeProvider.(apiBaseProvider)
|
||||
if !ok {
|
||||
t.Fatalf("registered provider does not implement APIBase(), type = %T", runtimeProvider)
|
||||
}
|
||||
|
||||
got := abp.APIBase()
|
||||
if got != tt.wantURL {
|
||||
t.Fatalf("registered APIBase = %q, want %q", got, tt.wantURL)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user