From 23b5ca00541dda67c75371b7757acb4040cd0d73 Mon Sep 17 00:00:00 2001 From: Logan Yang Date: Wed, 10 Jun 2026 22:21:47 -0700 Subject: [PATCH] fix: normalize provider base URL per consumer (legacy chat vs opencode) (#2589) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One stored Provider.baseUrl feeds two SDK stacks with opposite conventions: the legacy-chat LangChain clients for Google and Groq append their own version path (/v1beta, /openai/v1) and need the host-only form, while opencode's AI SDK treats baseURL as the complete prefix and needs the versioned models.dev form. Whichever form was stored, exactly one chat mode 404'd — a versioned URL doubled the path in legacy chat, a host-only URL dropped the version segment in agent chat (surfacing as "opencode finished the turn without returning any assistant text"). Normalize at each consumer boundary instead of fighting over the stored value: - chatModelManager strips a trailing version suffix before handing the URL to ChatGoogleGenerativeAI / ChatGroq (mirrors what listGoogleModels already did for verification). - buildOpencodeConfig drops the baseURL when it is just the provider's canonical endpoint in any spelling, letting opencode's registry default (always the correct versioned form) apply; genuine proxy/gateway overrides are still forwarded verbatim. Fixes logancyang/obsidian-copilot-preview#152 Co-authored-by: Claude Opus 4.8 --- .../chatModelManager.bridge.test.ts | 53 +++++++- src/LLMProviders/chatModelManager.ts | 9 +- .../backends/opencode/OpencodeBackend.test.ts | 39 ++++++ .../backends/opencode/OpencodeBackend.ts | 21 +++- src/utils/providerBaseUrl.test.ts | 114 ++++++++++++++++++ src/utils/providerBaseUrl.ts | 90 ++++++++++++++ 6 files changed, 320 insertions(+), 6 deletions(-) create mode 100644 src/utils/providerBaseUrl.test.ts create mode 100644 src/utils/providerBaseUrl.ts diff --git a/src/LLMProviders/chatModelManager.bridge.test.ts b/src/LLMProviders/chatModelManager.bridge.test.ts index 6c17c6ac..d8fea5f6 100644 --- a/src/LLMProviders/chatModelManager.bridge.test.ts +++ b/src/LLMProviders/chatModelManager.bridge.test.ts @@ -6,7 +6,26 @@ import { getSettings, setSettings } from "@/settings/model"; import ChatModelManager from "./chatModelManager"; jest.mock("@langchain/anthropic", () => ({ ChatAnthropic: class {} })); -jest.mock("@langchain/groq", () => ({ ChatGroq: class {} })); +// Capture constructor configs so tests can assert what the manager actually +// hands the LangChain clients (e.g. base-URL normalization). +jest.mock("@langchain/groq", () => { + class ChatGroq { + static configs: unknown[] = []; + constructor(config: unknown) { + ChatGroq.configs.push(config); + } + } + return { ChatGroq }; +}); +jest.mock("@langchain/google-genai", () => { + class ChatGoogleGenerativeAI { + static configs: unknown[] = []; + constructor(config: unknown) { + ChatGoogleGenerativeAI.configs.push(config); + } + } + return { ChatGoogleGenerativeAI }; +}); function bridgedModel(overrides: Partial = {}): CustomModel { return { @@ -41,4 +60,36 @@ describe("ChatModelManager bridged models", () => { expect(manager.getActiveModel()).toBe(model); await expect(manager.getChatModelWithTemperature(0)).resolves.toBeDefined(); }); + + it("strips a versioned Google base URL — ChatGoogleGenerativeAI appends /v1beta itself", async () => { + const GoogleMock = jest.requireMock("@langchain/google-genai").ChatGoogleGenerativeAI as { + configs: Array<{ baseUrl?: string }>; + }; + const model = bridgedModel({ + name: "gemini-2.5-flash", + provider: ChatModelProviders.GOOGLE, + apiKey: "g-key", + baseUrl: "https://generativelanguage.googleapis.com/v1beta", + }); + + await ChatModelManager.getInstance().createModelInstanceFromBridged(model); + + expect(GoogleMock.configs.at(-1)?.baseUrl).toBe("https://generativelanguage.googleapis.com"); + }); + + it("strips a versioned Groq base URL — groq-sdk appends /openai/v1 itself", async () => { + const GroqMock = jest.requireMock("@langchain/groq").ChatGroq as { + configs: Array<{ baseUrl?: string }>; + }; + const model = bridgedModel({ + name: "llama-3.3-70b-versatile", + provider: ChatModelProviders.GROQ, + apiKey: "gq-key", + baseUrl: "https://api.groq.com/openai/v1", + }); + + await ChatModelManager.getInstance().createModelInstanceFromBridged(model); + + expect(GroqMock.configs.at(-1)?.baseUrl).toBe("https://api.groq.com"); + }); }); diff --git a/src/LLMProviders/chatModelManager.ts b/src/LLMProviders/chatModelManager.ts index b7e078bc..7ccbce0b 100644 --- a/src/LLMProviders/chatModelManager.ts +++ b/src/LLMProviders/chatModelManager.ts @@ -25,6 +25,7 @@ import { safeFetchNoThrow, shouldUseGitHubCopilotResponsesApi, } from "@/utils"; +import { googleHostBaseUrl, groqHostBaseUrl } from "@/utils/providerBaseUrl"; import { ChatAnthropic } from "@langchain/anthropic"; import { BaseChatModel } from "@langchain/core/language_models/chat_models"; import { BaseLanguageModel } from "@langchain/core/language_models/base"; @@ -332,7 +333,9 @@ export default class ChatModelManager { ), model: modelName, safetySettings: GOOGLE_SAFETY_SETTINGS_BLOCK_NONE, - baseUrl: customModel.baseUrl, + // ChatGoogleGenerativeAI appends `/v1beta` itself; a stored versioned + // base URL would double the segment (`/v1beta/v1beta/…` → 404). + baseUrl: googleHostBaseUrl(customModel.baseUrl), }, [ChatModelProviders.XAI]: { apiKey: await this.resolveApiKey( @@ -376,7 +379,9 @@ export default class ChatModelManager { allowLegacyCredentialFallback ), model: modelName, - baseUrl: customModel.baseUrl, + // groq-sdk appends `/openai/v1` itself; the stored URL is usually the + // versioned models.dev form, which would double the segment. + baseUrl: groqHostBaseUrl(customModel.baseUrl), }, [ChatModelProviders.OLLAMA]: { // ChatOllama has `model` instead of `modelName`!! diff --git a/src/agentMode/backends/opencode/OpencodeBackend.test.ts b/src/agentMode/backends/opencode/OpencodeBackend.test.ts index 9f10814b..d3ed69b2 100644 --- a/src/agentMode/backends/opencode/OpencodeBackend.test.ts +++ b/src/agentMode/backends/opencode/OpencodeBackend.test.ts @@ -466,6 +466,45 @@ describe("buildOpencodeConfig — provider/model injection", () => { }); }); + it("omits the baseURL when a catalog provider's URL is its own host-only endpoint (Google dialog seed)", async () => { + // The dialog seeds Google with the host-only endpoint, but the AI SDK + // treats baseURL as the complete prefix — forwarding it would drop the + // `/v1beta` segment and 404 every call. Recognize the canonical endpoint + // and let opencode's registry default (the versioned form) apply. + const provider = makeProvider( + "p-google", + { kind: "byok", catalogProviderId: "google" }, + { + providerType: "google" as ProviderType, + baseUrl: "https://generativelanguage.googleapis.com", + } + ); + const deps = makeDeps({ + resolved: [okEntry(provider, makeModel("p-google", "gemini-2.5-flash"))], + keys: { "p-google": "g-123" }, + }); + const cfg = (await buildOpencodeConfig(getSettings(), deps)) as { + provider: Record; + }; + expect(cfg.provider.google.options).toEqual({ apiKey: "g-123" }); + }); + + it("omits the baseURL when a catalog provider's URL is its versioned endpoint (Groq models.dev seed)", async () => { + const provider = makeProvider( + "p-groq", + { kind: "byok", catalogProviderId: "groq" }, + { providerType: "openai-compatible", baseUrl: "https://api.groq.com/openai/v1" } + ); + const deps = makeDeps({ + resolved: [okEntry(provider, makeModel("p-groq", "llama-3.3-70b-versatile"))], + keys: { "p-groq": "gq-123" }, + }); + const cfg = (await buildOpencodeConfig(getSettings(), deps)) as { + provider: Record; + }; + expect(cfg.provider.groq.options).toEqual({ apiKey: "gq-123" }); + }); + it("registers Copilot Plus as a custom openai-compatible provider from its own fields", async () => { // Plus has no catalog identity, so it's registered like a custom endpoint — // npm/name/baseURL all read off the provider row (seeded by CopilotPlusSetupApi). diff --git a/src/agentMode/backends/opencode/OpencodeBackend.ts b/src/agentMode/backends/opencode/OpencodeBackend.ts index 26e408ac..9360d8b7 100644 --- a/src/agentMode/backends/opencode/OpencodeBackend.ts +++ b/src/agentMode/backends/opencode/OpencodeBackend.ts @@ -3,6 +3,7 @@ import { logInfo } from "@/logger"; import { getSettings } from "@/settings/model"; import type { CopilotSettings } from "@/settings/model"; import { isSelfHostedProvider } from "@/modelManagement"; +import { isCatalogProviderDefaultEndpoint } from "@/utils/providerBaseUrl"; import type { BackendConfigRegistry, ProviderRegistry } from "@/modelManagement"; import { AcpBackend, AcpSpawnDescriptor } from "@/agentMode/acp/types"; import type { CopilotMode } from "@/agentMode/session/types"; @@ -143,7 +144,8 @@ export async function buildOpencodeConfig( // identity and must be registered explicitly as `@ai-sdk/openai-compatible` // pointed at its own baseURL. const origin = entry.provider.origin; - const hasCatalogIdentity = origin.kind === "byok" && !!origin.catalogProviderId; + const catalogProviderId = origin.kind === "byok" ? origin.catalogProviderId : undefined; + const hasCatalogIdentity = !!catalogProviderId; // Whether a missing key should drop the provider is a separate question: // self-hosted endpoints commonly run key-less. Detect that from the baseUrl // host so it stays correct even if a local runner gains a catalog id. @@ -160,15 +162,28 @@ export async function buildOpencodeConfig( ); continue; } - const baseURL = entry.provider.baseUrl; + const rawBaseURL = entry.provider.baseUrl; // A non-catalog provider with no baseURL is unroutable — opencode has no // registry default to fall back on. - if (!hasCatalogIdentity && !baseURL) { + if (!hasCatalogIdentity && !rawBaseURL) { logInfo( `[AgentMode] skipping ${mapping.id}/${entry.configuredModel.info.id}: ${origin.kind} provider has no baseUrl` ); continue; } + // The AI SDK treats `baseURL` as the complete prefix (the versioned + // models.dev form), but stored values are often host-only — the + // configure dialog's Google seed, or a user trimming `/v1beta` to fix + // legacy chat (#152). When the value is just the provider's canonical + // endpoint in some spelling, drop it and let opencode's registry + // default apply: that is always the correct form. Anything else is a + // genuine proxy/gateway override and is forwarded verbatim. + const baseURL = + hasCatalogIdentity && + rawBaseURL && + isCatalogProviderDefaultEndpoint(catalogProviderId, rawBaseURL) + ? undefined + : rawBaseURL; // Omit apiKey/baseURL when falsy: an empty-string key reaches // `@ai-sdk/openai-compatible` as `Authorization: Bearer ` (silent 401), // and an empty baseURL would clobber opencode's registry default. diff --git a/src/utils/providerBaseUrl.test.ts b/src/utils/providerBaseUrl.test.ts new file mode 100644 index 00000000..fb888751 --- /dev/null +++ b/src/utils/providerBaseUrl.test.ts @@ -0,0 +1,114 @@ +import { + googleHostBaseUrl, + groqHostBaseUrl, + isCatalogProviderDefaultEndpoint, +} from "./providerBaseUrl"; + +describe("googleHostBaseUrl", () => { + it("strips a trailing /v1beta (Google's documented endpoint form)", () => { + expect(googleHostBaseUrl("https://generativelanguage.googleapis.com/v1beta")).toBe( + "https://generativelanguage.googleapis.com" + ); + }); + + it("strips a trailing /v1", () => { + expect(googleHostBaseUrl("https://generativelanguage.googleapis.com/v1")).toBe( + "https://generativelanguage.googleapis.com" + ); + }); + + it("leaves a host-only URL unchanged", () => { + expect(googleHostBaseUrl("https://generativelanguage.googleapis.com")).toBe( + "https://generativelanguage.googleapis.com" + ); + }); + + it("tolerates trailing slashes and surrounding whitespace", () => { + expect(googleHostBaseUrl(" https://generativelanguage.googleapis.com/v1beta/ ")).toBe( + "https://generativelanguage.googleapis.com" + ); + }); + + it("strips the suffix from a proxy origin too (proxies mirror the path shape)", () => { + expect(googleHostBaseUrl("https://my-gateway.example.com/v1beta")).toBe( + "https://my-gateway.example.com" + ); + }); + + it("does not touch mid-path version segments", () => { + expect(googleHostBaseUrl("https://gw.example.com/v1/acct/google-ai-studio")).toBe( + "https://gw.example.com/v1/acct/google-ai-studio" + ); + }); + + it("returns undefined for blank input", () => { + expect(googleHostBaseUrl(undefined)).toBeUndefined(); + expect(googleHostBaseUrl("")).toBeUndefined(); + expect(googleHostBaseUrl(" ")).toBeUndefined(); + }); +}); + +describe("groqHostBaseUrl", () => { + it("strips a trailing /openai/v1 (the models.dev form the dialog seeds)", () => { + expect(groqHostBaseUrl("https://api.groq.com/openai/v1")).toBe("https://api.groq.com"); + }); + + it("strips a trailing /openai", () => { + expect(groqHostBaseUrl("https://api.groq.com/openai")).toBe("https://api.groq.com"); + }); + + it("strips a trailing /v1", () => { + expect(groqHostBaseUrl("https://api.groq.com/v1")).toBe("https://api.groq.com"); + }); + + it("leaves a host-only URL unchanged", () => { + expect(groqHostBaseUrl("https://api.groq.com")).toBe("https://api.groq.com"); + }); + + it("returns undefined for blank input", () => { + expect(groqHostBaseUrl(undefined)).toBeUndefined(); + expect(groqHostBaseUrl("")).toBeUndefined(); + }); +}); + +describe("isCatalogProviderDefaultEndpoint", () => { + it("recognizes the versioned Google endpoint", () => { + expect( + isCatalogProviderDefaultEndpoint("google", "https://generativelanguage.googleapis.com/v1beta") + ).toBe(true); + }); + + it("recognizes the host-only Google endpoint (the configure-dialog seed)", () => { + expect( + isCatalogProviderDefaultEndpoint("google", "https://generativelanguage.googleapis.com") + ).toBe(true); + }); + + it("recognizes the versioned Groq endpoint with a trailing slash", () => { + expect(isCatalogProviderDefaultEndpoint("groq", "https://api.groq.com/openai/v1/")).toBe(true); + }); + + it("rejects a proxy origin", () => { + expect(isCatalogProviderDefaultEndpoint("openai", "https://my-proxy.example.com/v1")).toBe( + false + ); + }); + + it("rejects a non-default path on the canonical origin", () => { + expect( + isCatalogProviderDefaultEndpoint( + "google", + "https://generativelanguage.googleapis.com/v1alpha" + ) + ).toBe(false); + }); + + it("rejects unknown catalog ids", () => { + expect(isCatalogProviderDefaultEndpoint("mistral", "https://api.mistral.ai/v1")).toBe(false); + expect(isCatalogProviderDefaultEndpoint(undefined, "https://api.groq.com")).toBe(false); + }); + + it("rejects unparseable URLs", () => { + expect(isCatalogProviderDefaultEndpoint("google", "not a url")).toBe(false); + }); +}); diff --git a/src/utils/providerBaseUrl.ts b/src/utils/providerBaseUrl.ts new file mode 100644 index 00000000..56f553c0 --- /dev/null +++ b/src/utils/providerBaseUrl.ts @@ -0,0 +1,90 @@ +/** + * Per-consumer normalization for a stored provider base URL. + * + * One persisted `Provider.baseUrl` feeds two SDK stacks with opposite + * conventions (logancyang/obsidian-copilot-preview#152): + * + * - The legacy-chat LangChain clients for Google and Groq append their own + * version path (`ChatGoogleGenerativeAI` adds `/v1beta`, groq-sdk adds + * `/openai/v1`), so they need the HOST-ONLY form — a versioned base URL + * doubles the path and 404s. + * - opencode's AI SDK providers treat `baseURL` as the complete prefix and + * append only the route, so they need the VERSIONED form (the models.dev + * `api` convention) — a host-only base URL drops the version segment. + * + * Both forms reach persistence: the configure dialog seeds models.dev's + * versioned `api` URL when the catalog carries one (Groq) and a host-only + * known default when it doesn't (Google), and users paste either form from + * provider docs. Persisted rows are left untouched; each consumer normalizes + * through these helpers instead. + */ + +/** Trim and drop trailing slashes; blank input becomes `undefined`. */ +function trimBaseUrl(baseUrl: string | undefined): string | undefined { + const trimmed = (baseUrl ?? "").trim().replace(/\/+$/, ""); + return trimmed || undefined; +} + +/** + * Host form for `ChatGoogleGenerativeAI`, which appends `/${apiVersion}` + * (default `v1beta`) itself. Strips a trailing `/v1beta` or `/v1` regardless + * of origin: a Google-compatible proxy must mirror the `/v1beta/models/…` + * path shape, so the same stripping holds for it. + */ +export function googleHostBaseUrl(baseUrl: string | undefined): string | undefined { + const trimmed = trimBaseUrl(baseUrl); + if (!trimmed) return undefined; + return trimmed.replace(/\/v1(beta)?$/i, "") || undefined; +} + +/** + * Host form for `ChatGroq`, whose groq-sdk client appends `/openai/v1` + * itself. Strips a trailing `/openai/v1`, `/openai`, or `/v1`. + */ +export function groqHostBaseUrl(baseUrl: string | undefined): string | undefined { + const trimmed = trimBaseUrl(baseUrl); + if (!trimmed) return undefined; + return trimmed.replace(/(\/openai\/v1|\/openai|\/v1)$/i, "") || undefined; +} + +/** + * Canonical API origins for catalog providers whose endpoints we know. + * Used to recognize "this base URL is the provider's own endpoint in some + * spelling" as opposed to a genuine proxy/gateway override. + */ +const CATALOG_DEFAULT_ORIGINS: Record = { + anthropic: "https://api.anthropic.com", + google: "https://generativelanguage.googleapis.com", + groq: "https://api.groq.com", + openai: "https://api.openai.com", +}; + +/** + * Path spellings that still mean "the default endpoint": bare host plus any + * known version/prefix segment a user (or our own seeding) might include. + */ +const DEFAULT_ENDPOINT_PATHS = new Set(["", "/v1", "/v1beta", "/openai", "/openai/v1"]); + +/** + * `true` when `baseUrl` points at the catalog provider's own canonical API + * endpoint (any version-suffix / trailing-slash / case spelling). Callers + * with a more reliable default for that provider — opencode's models.dev + * registry — can then drop the stored value rather than guess which path + * form it needs. Unknown catalog ids, non-default paths, and unparseable + * URLs return `false` so genuine overrides are always forwarded. + */ +export function isCatalogProviderDefaultEndpoint( + catalogProviderId: string | undefined, + baseUrl: string +): boolean { + if (!catalogProviderId) return false; + const defaultOrigin = CATALOG_DEFAULT_ORIGINS[catalogProviderId]; + if (!defaultOrigin) return false; + try { + const url = new URL(baseUrl.trim()); + const path = url.pathname.replace(/\/+$/, "").toLowerCase(); + return url.origin === defaultOrigin && DEFAULT_ENDPOINT_PATHS.has(path); + } catch { + return false; + } +}