-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
🗺️ fix: Resolve Custom-Endpoint Providers for Summarization #12739
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
Changes from 1 commit
2528093
60f5ad6
e68e7c8
7bfd041
828126f
a39ab10
56ab4b5
ea64dd2
fff08e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| import type { AppConfig } from '@librechat/data-schemas'; | ||
| import type { SummarizationConfig } from 'librechat-data-provider'; | ||
| import { EModelEndpoint } from 'librechat-data-provider'; | ||
| import { createRun } from '~/agents/run'; | ||
|
|
||
| // Mock winston logger | ||
|
|
@@ -57,6 +59,7 @@ async function callAndCapture( | |
| agents?: ReturnType<typeof makeAgent>[]; | ||
| summarizationConfig?: SummarizationConfig; | ||
| initialSummary?: { text: string; tokenCount: number }; | ||
| appConfig?: AppConfig; | ||
| } = {}, | ||
| ) { | ||
| const agents = opts.agents ?? [makeAgent()]; | ||
|
|
@@ -67,6 +70,7 @@ async function callAndCapture( | |
| signal, | ||
| summarizationConfig: opts.summarizationConfig, | ||
| initialSummary: opts.initialSummary, | ||
| appConfig: opts.appConfig, | ||
| streaming: true, | ||
| streamUsage: true, | ||
| }); | ||
|
|
@@ -77,6 +81,17 @@ async function callAndCapture( | |
| return callArgs.graphConfig.agents as Array<Record<string, unknown>>; | ||
| } | ||
|
|
||
| /** Minimal AppConfig with a single custom endpoint for testing provider resolution. */ | ||
| function makeAppConfig( | ||
| customEndpoints: Array<{ name: string; baseURL: string; apiKey: string }>, | ||
| ): AppConfig { | ||
| return { | ||
| endpoints: { | ||
| [EModelEndpoint.custom]: customEndpoints, | ||
| }, | ||
| } as unknown as AppConfig; | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
@@ -297,3 +312,156 @@ describe('initialSummary passthrough', () => { | |
| expect(agents[0].initialSummary).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Suite 7: custom-endpoint provider resolution | ||
| // --------------------------------------------------------------------------- | ||
| describe('custom-endpoint provider resolution', () => { | ||
| it('remaps a custom endpoint name to openAI and injects baseURL/apiKey', async () => { | ||
| const appConfig = makeAppConfig([ | ||
| { name: 'Ollama', baseURL: 'http://localhost:11434/v1', apiKey: 'ollama-key' }, | ||
| ]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'Ollama', model: 'llama3' }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| expect(config.provider).toBe('openAI'); | ||
| expect(config.model).toBe('llama3'); | ||
|
|
||
| const parameters = config.parameters as Record<string, unknown>; | ||
| expect(parameters).toMatchObject({ | ||
| configuration: { baseURL: 'http://localhost:11434/v1' }, | ||
| apiKey: 'ollama-key', | ||
| }); | ||
| }); | ||
|
|
||
| it('is case-insensitive when matching custom endpoint names', async () => { | ||
| const appConfig = makeAppConfig([ | ||
| { name: 'Ollama', baseURL: 'http://localhost:11434/v1', apiKey: 'ollama-key' }, | ||
| ]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'ollama', model: 'llama3' }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| expect(config.provider).toBe('openAI'); | ||
| expect((config.parameters as Record<string, unknown>).apiKey).toBe('ollama-key'); | ||
| }); | ||
|
||
|
|
||
| it('leaves known SDK providers untouched', async () => { | ||
| const appConfig = makeAppConfig([]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'anthropic', model: 'claude' }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| expect(config.provider).toBe('anthropic'); | ||
| expect(config.parameters).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('preserves unknown provider names when appConfig is missing', async () => { | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'Ollama', model: 'llama3' }, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| expect(config.provider).toBe('Ollama'); | ||
| expect(config.parameters).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('leaves unrecognized names untouched when no matching custom endpoint exists', async () => { | ||
| const appConfig = makeAppConfig([ | ||
| { name: 'Ollama', baseURL: 'http://localhost:11434/v1', apiKey: 'ollama-key' }, | ||
| ]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'nonexistent', model: 'foo' }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| expect(config.provider).toBe('nonexistent'); | ||
| expect(config.parameters).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('extracts ${ENV_VAR} references in custom endpoint credentials', async () => { | ||
| process.env.TEST_OLLAMA_KEY = 'resolved-key-value'; | ||
| const appConfig = makeAppConfig([ | ||
| { | ||
| name: 'Ollama', | ||
| baseURL: 'http://localhost:11434/v1', | ||
| apiKey: '${TEST_OLLAMA_KEY}', | ||
| }, | ||
| ]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'Ollama', model: 'llama3' }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| const parameters = config.parameters as Record<string, unknown>; | ||
| expect(parameters.apiKey).toBe('resolved-key-value'); | ||
| delete process.env.TEST_OLLAMA_KEY; | ||
| }); | ||
|
|
||
| it('skips override when apiKey is marked user_provided', async () => { | ||
| const appConfig = makeAppConfig([ | ||
| { name: 'Ollama', baseURL: 'http://localhost:11434/v1', apiKey: 'user_provided' }, | ||
| ]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'Ollama', model: 'llama3' }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| // Provider still remapped to the SDK-recognized name... | ||
| expect(config.provider).toBe('openAI'); | ||
| // ...but credentials are not forwarded (async user lookup not supported here; | ||
| // SDK's self-summarize path will reuse the agent's clientOptions). | ||
| expect(config.parameters).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('skips override when env var reference cannot be resolved', async () => { | ||
| delete process.env.UNSET_TEST_KEY; | ||
| const appConfig = makeAppConfig([ | ||
| { | ||
| name: 'Ollama', | ||
| baseURL: 'http://localhost:11434/v1', | ||
| apiKey: '${UNSET_TEST_KEY}', | ||
| }, | ||
| ]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { provider: 'Ollama', model: 'llama3' }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| expect(config.provider).toBe('openAI'); | ||
| expect(config.parameters).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('merges overrides alongside user-supplied parameters', async () => { | ||
| const appConfig = makeAppConfig([ | ||
| { name: 'Ollama', baseURL: 'http://localhost:11434/v1', apiKey: 'ollama-key' }, | ||
| ]); | ||
| const agents = await callAndCapture({ | ||
| summarizationConfig: { | ||
| provider: 'Ollama', | ||
| model: 'llama3', | ||
| parameters: { temperature: 0.2 }, | ||
| }, | ||
| appConfig, | ||
| }); | ||
|
|
||
| const config = agents[0].summarizationConfig as Record<string, unknown>; | ||
| const parameters = config.parameters as Record<string, unknown>; | ||
| expect(parameters).toMatchObject({ | ||
| temperature: 0.2, | ||
| configuration: { baseURL: 'http://localhost:11434/v1' }, | ||
| apiKey: 'ollama-key', | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,10 @@ | ||
| import { Run, Providers, Constants } from '@librechat/agents'; | ||
| import { providerEndpointMap, KnownEndpoints } from 'librechat-data-provider'; | ||
| import { | ||
| envVarRegex, | ||
| KnownEndpoints, | ||
| extractEnvVariable, | ||
| providerEndpointMap, | ||
| } from 'librechat-data-provider'; | ||
| import type { | ||
| SummarizationConfig as AgentSummarizationConfig, | ||
| MultiAgentGraphConfig, | ||
|
|
@@ -15,9 +20,11 @@ import type { | |
| } from '@librechat/agents'; | ||
| import type { Agent, SummarizationConfig } from 'librechat-data-provider'; | ||
| import type { BaseMessage } from '@langchain/core/messages'; | ||
| import type { IUser } from '@librechat/data-schemas'; | ||
| import type { AppConfig, IUser } from '@librechat/data-schemas'; | ||
| import type * as t from '~/types'; | ||
| import { getProviderConfig } from '~/endpoints/config/providers'; | ||
| import { resolveHeaders, createSafeUser } from '~/utils/env'; | ||
| import { isUserProvided } from '~/utils/common'; | ||
|
|
||
| /** Expected shape of JSON tool search results */ | ||
| interface ToolSearchJsonResult { | ||
|
|
@@ -186,26 +193,102 @@ function isNonEmptyString(value: unknown): value is string { | |
| return typeof value === 'string' && value.trim().length > 0; | ||
| } | ||
|
|
||
| /** Client-option overrides for summarization models targeting custom endpoints. */ | ||
| interface SummarizationClientOverrides { | ||
| configuration?: { baseURL: string }; | ||
| apiKey?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Resolves a summarization provider string (which may be a custom-endpoint name | ||
| * like "Ollama") into the SDK-recognized provider and any baseURL/apiKey | ||
| * overrides required to talk to that endpoint. | ||
| * | ||
| * Without this step, a `summarization.provider: "Ollama"` entry in | ||
| * `librechat.yaml` flows verbatim to the agents SDK, which only knows a fixed | ||
| * set of provider names and throws "Unsupported LLM provider: Ollama". | ||
| */ | ||
| function resolveSummarizationProvider( | ||
| rawProvider: string, | ||
| appConfig: AppConfig | undefined, | ||
| ): { | ||
| provider: string; | ||
| clientOverrides?: SummarizationClientOverrides; | ||
| } { | ||
| if (!appConfig || !isNonEmptyString(rawProvider)) { | ||
| return { provider: rawProvider }; | ||
| } | ||
| try { | ||
| const { overrideProvider, customEndpointConfig } = getProviderConfig({ | ||
| provider: rawProvider, | ||
| appConfig, | ||
| }); | ||
|
Comment on lines
+258
to
+262
|
||
| if (!customEndpointConfig) { | ||
| return { provider: overrideProvider }; | ||
| } | ||
| const rawApiKey = customEndpointConfig.apiKey ?? ''; | ||
| const rawBaseURL = customEndpointConfig.baseURL ?? ''; | ||
|
Comment on lines
+266
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The override builder only reads Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| /** | ||
| * User-provided credentials require an async DB lookup and expiry checks | ||
| * that are out of scope here. Fall back to the remapped provider and let | ||
| * the SDK's self-summarize path reuse the agent's own client options. | ||
| */ | ||
| if (isUserProvided(rawApiKey) || isUserProvided(rawBaseURL)) { | ||
| return { provider: overrideProvider }; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Returning Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the flag. I traced the code paths carefully and this case is actually handled: By the time So in the user-provided branch of
The per-user credentials are preserved through the self-summarize baseOptions merge — we're not dropping them. Happy to wire up an explicit test for this path if you think it would help guard against regressions. |
||
| } | ||
|
Comment on lines
+276
to
+278
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a custom summarization provider uses Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — fixed in |
||
| const apiKey = extractEnvVariable(rawApiKey); | ||
| const baseURL = extractEnvVariable(rawBaseURL); | ||
| if (!apiKey || !baseURL || apiKey.match(envVarRegex) || baseURL.match(envVarRegex)) { | ||
| return { provider: overrideProvider }; | ||
|
||
| } | ||
| return { | ||
| provider: overrideProvider, | ||
| clientOverrides: { | ||
| configuration: { baseURL }, | ||
| apiKey, | ||
| }, | ||
| }; | ||
| } catch { | ||
| return { provider: rawProvider }; | ||
| } | ||
| } | ||
|
|
||
| /** Shapes a SummarizationConfig into the format expected by AgentInputs. */ | ||
| function shapeSummarizationConfig( | ||
| config: SummarizationConfig | undefined, | ||
| fallbackProvider: string, | ||
| fallbackModel: string | undefined, | ||
| appConfig: AppConfig | undefined, | ||
| ) { | ||
| const provider = config?.provider ?? fallbackProvider; | ||
| const rawProvider = config?.provider ?? fallbackProvider; | ||
| const { provider, clientOverrides } = resolveSummarizationProvider(rawProvider, appConfig); | ||
| const model = config?.model ?? fallbackModel; | ||
| const trigger = | ||
| config?.trigger?.type && config?.trigger?.value | ||
| ? { type: config.trigger.type, value: config.trigger.value } | ||
| : undefined; | ||
|
|
||
| /** | ||
| * Custom-endpoint overrides (baseURL/apiKey) are merged into `parameters` so | ||
| * that the SDK's `buildSummarizationClientConfig` spreads them onto the | ||
| * summarization client options. This is required when summarization targets | ||
| * a different custom endpoint than the main agent. | ||
| */ | ||
| const parameters = | ||
| clientOverrides != null | ||
| ? { | ||
| ...(config?.parameters ?? {}), | ||
| ...clientOverrides, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When custom-endpoint overrides are applied, this merge order makes Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Applying Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point — addressed in |
||
| : config?.parameters; | ||
|
|
||
| return { | ||
| enabled: config?.enabled !== false && isNonEmptyString(provider) && isNonEmptyString(model), | ||
| config: { | ||
| trigger, | ||
| provider, | ||
| model, | ||
| parameters: config?.parameters, | ||
| parameters, | ||
| prompt: config?.prompt, | ||
| updatePrompt: config?.updatePrompt, | ||
| reserveRatio: config?.reserveRatio, | ||
|
|
@@ -260,6 +343,7 @@ export async function createRun({ | |
| summarizationConfig, | ||
| initialSummary, | ||
| calibrationRatio, | ||
| appConfig, | ||
| streaming = true, | ||
| streamUsage = true, | ||
| }: { | ||
|
|
@@ -277,6 +361,11 @@ export async function createRun({ | |
| initialSummary?: { text: string; tokenCount: number }; | ||
| /** Calibration ratio from previous run's contextMeta, seeds the pruner EMA */ | ||
| calibrationRatio?: number; | ||
| /** | ||
| * Resolved app config. Used to translate custom-endpoint provider names | ||
| * (e.g. "Ollama") in the summarization config to SDK-recognized providers. | ||
| */ | ||
| appConfig?: AppConfig; | ||
| } & Pick<RunConfig, 'tokenCounter' | 'customHandlers' | 'indexTokenCountMap'>): Promise< | ||
| Run<IState> | ||
| > { | ||
|
|
@@ -307,6 +396,7 @@ export async function createRun({ | |
| agent.summarization ?? summarizationConfig, | ||
| provider as string, | ||
| selfModel, | ||
| appConfig, | ||
| ); | ||
|
|
||
| const llmConfig: t.RunLLMConfig = Object.assign( | ||
|
|
||
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.
This passes the module-level
appConfigintocreateRun, but in this filesetAppConfighas no callers in the repository, soappConfigremainsnullon both response run paths. In that stateresolveSummarizationProvidershort-circuits and leaves custom summarization providers unresolved, so/api/agents/v1/responsesstill hitsUnsupported LLM providerfor configs likesummarization.provider: "Ollama". Passreq.confighere (as already done forsummarizationConfig) so provider resolution can actually run.Useful? React with 👍 / 👎.
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.
Good catch —
setAppConfigis indeed dead code. Fixed ine68e7c8f7: bothcreateRuncalls inresponses.jsnow usereq.configdirectly (matching what the same controller already does forsummarizationConfigon line 283).