From 54f95d7669af550c7c8f986bca223b908a7f1ef6 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Thu, 30 Jul 2026 15:51:35 +0800 Subject: [PATCH] fix(llm): atomic route replacement, whole-snapshot requests, and loud credential misses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings across the seam and both adapters. registerAdapter now returns a handle carrying replace(providers): the candidate route set is validated in full before anything moves, so a route another adapter owns leaves the previous registration intact, and the swap itself is one synchronous section with no observable gap. pi-ai uses it instead of dispose-then-register — the old shape dropped every route when the new set conflicted, and its facts cache could then equal the registry's, so reverting to a working configuration never re-applied. Its registration facts are also sorted by provider, so a settings document that merely reorders keys no longer triggers a swap. DeepSeek's per-request snapshot now carries the credential facts, and resolveApiKey receives it instead of re-reading the raw config: a settings generation the resolver rejects can no longer contribute its literal key to a request the previous generation's endpoint serves. pi-ai only defers to the SDK's provider-native discovery when a profile names no credential at all; a configured apiKeyEnv that misses now fails with MISSING_CREDENTIAL naming the route and the reference, instead of handing pi-ai undefined and letting it authenticate with an unrelated ambient key. The eager boot-time credential probe is gone: it could run before the credentials service mounted and reported every failure as a missing key. The route stays registered and browsable; the first request gives the accurate error, whose guidance now leads with the credential store and mentions a literal apiKey last. --- packages/llm/llm-deepseek/src/adapter.ts | 22 +++- packages/llm/llm-deepseek/src/index.ts | 41 ++++--- .../llm/llm-deepseek/tests/adapter.spec.ts | 4 +- .../llm-deepseek/tests/dynamic-config.spec.ts | 23 ++++ packages/llm/llm-pi-ai/src/adapter.ts | 8 +- packages/llm/llm-pi-ai/src/index.ts | 64 ++++++++--- packages/llm/llm-pi-ai/tests/adapter.spec.ts | 14 ++- .../llm-pi-ai/tests/dynamic-config.spec.ts | 51 ++++++++- packages/llm/llm/src/index.ts | 101 +++++++++++++----- 9 files changed, 252 insertions(+), 76 deletions(-) diff --git a/packages/llm/llm-deepseek/src/adapter.ts b/packages/llm/llm-deepseek/src/adapter.ts index 0163dd3cab..a4b02a3e39 100644 --- a/packages/llm/llm-deepseek/src/adapter.ts +++ b/packages/llm/llm-deepseek/src/adapter.ts @@ -17,6 +17,7 @@ import type { ResolvedRetryPolicy, StreamChunk, } from '@deepseek-ai/dsh-llm' +import type { CredentialRef } from '@deepseek-ai/dsh-credentials' import { idleWatchdog, timeoutOf } from '@deepseek-ai/dsh-timeout' import { serializeRequest } from './serialize.ts' import type { RequestDefaults } from './serialize.ts' @@ -45,6 +46,14 @@ export interface DeepSeekCatalogModel { export interface DeepSeekConnectionOptions { /** Endpoint base; `/chat/completions` is appended. */ baseURL: string + /** + * Literal API key of this same resolution, when the configuration carried + * one. Travelling with the endpoint is the point: a request can never pair + * one generation's URL with another generation's secret. + */ + apiKey?: string + /** Credential reference of this same resolution, resolved per request when no literal key exists. */ + apiKeyEnv: CredentialRef /** Request defaults applied to every call (thinking mode, effort). */ defaults: RequestDefaults /** Positive context capacity used when the selected model has no exact value. */ @@ -62,11 +71,12 @@ export interface DeepSeekAdapterOptions { /** Current validated connection facts; called once per operation. */ options: () => DeepSeekConnectionOptions /** - * Resolve the bearer token for one request; called once per stream call and - * frozen for that call. Throws `LlmError` `MISSING_CREDENTIAL` when no key - * is available anywhere. + * Resolve the bearer token for the connection facts of one request. The + * snapshot is passed in — never re-read — so the key can only ever come + * from the same resolution as the endpoint it is sent to. Throws `LlmError` + * `MISSING_CREDENTIAL` when no key is available anywhere. */ - resolveApiKey: () => Promise + resolveApiKey: (connection: DeepSeekConnectionOptions) => Promise } /** Default maximum idle interval while an adapter stream read is outstanding. */ @@ -189,8 +199,10 @@ export class DeepSeekAdapter extends LlmAdapter { // One resolution per stream call: connection facts and the credential // freeze here and hold for this whole request, so an in-flight stream // never observes a configuration change and the next call re-resolves. + // The key resolves *from this snapshot*, so an endpoint and the secret + // sent to it can never come from different configuration generations. const connection = this.config.options() - const apiKey = await this.config.resolveApiKey() + const apiKey = await this.config.resolveApiKey(connection) const consumer = new AbortController() const upstream = options.signal === undefined ? consumer.signal diff --git a/packages/llm/llm-deepseek/src/index.ts b/packages/llm/llm-deepseek/src/index.ts index bb2ccdaa11..6623351774 100644 --- a/packages/llm/llm-deepseek/src/index.ts +++ b/packages/llm/llm-deepseek/src/index.ts @@ -16,7 +16,6 @@ import z from 'schemastery' import { LlmError, resolveRetryPolicy, RetryPolicySchema } from '@deepseek-ai/dsh-llm' import type { RetryPolicyConfig } from '@deepseek-ai/dsh-llm' import { credentialRef } from '@deepseek-ai/dsh-credentials' -import type { CredentialRef } from '@deepseek-ai/dsh-credentials' import { deepEqualJson, installSettingsSection, settingsNamespace } from '@deepseek-ai/dsh-settings' import { MAX_TIMER_DELAY_MS } from '@deepseek-ai/dsh-timeout' import { DEFAULT_STREAM_IDLE_TIMEOUT_MS, DeepSeekAdapter } from './adapter.ts' @@ -32,6 +31,8 @@ export const inject = ['llm'] const NS = settingsNamespace('llm-deepseek') const DEFAULT_API_KEY_ENV = 'DEEPSEEK_API_KEY' +/** The single provider route this plugin owns. */ +const PROVIDER = 'deepseek' const DEFAULT_MODELS: DeepSeekCatalogModel[] = [ { id: 'deepseek-v4-flash', name: 'DeepSeek-V4-Flash', contextWindow: 256_000 }, @@ -89,11 +90,13 @@ export const Config: z = z.object({ /** Public API default; the internal endpoint comes from $DEEPSEEK_BASE_URL. */ export const PUBLIC_BASE_URL = 'https://api.deepseek.com' -/** Connection facts plus the plugin-consumed credential reference. */ -export interface ResolvedDeepSeekOptions extends DeepSeekConnectionOptions { - /** Reference resolved per request when no literal key is configured. */ - apiKeyEnv: CredentialRef -} +/** + * One resolution's complete request facts. Connection and credential facts + * are one value on purpose: a snapshot the resolver rejects keeps the whole + * previous generation, so a request can never pair a stale endpoint with a + * newer key. + */ +export type ResolvedDeepSeekOptions = DeepSeekConnectionOptions /** Resolve, validate, and detach the advisory model catalog. */ function resolveModels(models: readonly DeepSeekCatalogModel[] | undefined): DeepSeekCatalogModel[] { @@ -147,6 +150,7 @@ export function resolveAdapterOptions(config: Config): ResolvedDeepSeekOptions { ) } return { + ...config.apiKey !== undefined && config.apiKey.length > 0 ? { apiKey: config.apiKey } : {}, apiKeyEnv: credentialRef(config.apiKeyEnv ?? DEFAULT_API_KEY_ENV), baseURL: config.baseURL ?? process.env.DEEPSEEK_BASE_URL ?? PUBLIC_BASE_URL, defaults: { @@ -187,10 +191,11 @@ export function apply(ctx: Context, config: Config): void { } options() - const resolveApiKey = async (): Promise => { - const raw = current() - if (raw.apiKey !== undefined && raw.apiKey.length > 0) return raw.apiKey - const ref = options().apiKeyEnv + const resolveApiKey = async (connection: ResolvedDeepSeekOptions): Promise => { + // Every credential fact comes from the caller's snapshot, so a rejected + // settings generation cannot leak its key onto the previous endpoint. + if (connection.apiKey !== undefined) return connection.apiKey + const ref = connection.apiKeyEnv const credentials = ctx.get('credentials') if (credentials !== undefined) { const hit = await credentials.resolve(ref) @@ -202,8 +207,9 @@ export function apply(ctx: Context, config: Config): void { if (ambient !== undefined && ambient.length > 0) return ambient } throw new LlmError( - 'llm-deepseek: no API key for provider route "deepseek"; set the llm-deepseek "apiKey" setting,' - + ` store ${ref} with the credentials service, or export ${ref}`, + `llm-deepseek: no API key for provider route "${PROVIDER}"; store ${ref} through the credentials` + + ` service (the web Models page writes it), export ${ref} in the launching environment, or — as a` + + ' last resort — set a literal "apiKey" in the llm-deepseek settings section', 'MISSING_CREDENTIAL', ) } @@ -211,7 +217,7 @@ export function apply(ctx: Context, config: Config): void { const adapter = new DeepSeekAdapter({ options, resolveApiKey }) // Route effects bind to this apply fiber via the stable `ctx` reference, // even when a swap runs inside the scoped settings callback below. - let disposeRoute = ctx.llm.registerAdapter(['deepseek'], adapter) + let disposeRoute = ctx.llm.registerAdapter([PROVIDER], adapter) let registeredPolicy = options().retryPolicy const ensureRegistrationFacts = (): void => { const policy = options().retryPolicy @@ -220,17 +226,10 @@ export function apply(ctx: Context, config: Config): void { // fact per-request resolution cannot refresh: swap the registration in one // synchronous section (same adapter instance, no NO_ADAPTER window). disposeRoute() - disposeRoute = ctx.llm.registerAdapter(['deepseek'], adapter) + disposeRoute = ctx.llm.registerAdapter([PROVIDER], adapter) registeredPolicy = policy } - void resolveApiKey().then(() => undefined, () => { - // Expected on a first boot with dynamic sources: the route stays - // registered (the catalog is browsable) and each request fails with the - // actionable MISSING_CREDENTIAL message until a key arrives. - ctx.logger.warn('llm-deepseek: no API key resolved yet for route "deepseek"; requests will fail until one is configured') - }) - installSettingsSection(ctx, NS, Config, config, { setSource: (source) => { current = source diff --git a/packages/llm/llm-deepseek/tests/adapter.spec.ts b/packages/llm/llm-deepseek/tests/adapter.spec.ts index 935235d825..b5a4bc9ff4 100644 --- a/packages/llm/llm-deepseek/tests/adapter.spec.ts +++ b/packages/llm/llm-deepseek/tests/adapter.spec.ts @@ -817,8 +817,10 @@ describe('plugin registration and config', () => { await expect(ctx.llm.listModels('deepseek')).resolves.toHaveLength(2) await expect(assemble(ctx, { model: 'deepseek-v4-flash', messages: [] })) .rejects.toMatchObject({ code: 'MISSING_CREDENTIAL' }) + // The guidance leads with the credential store — the path that keeps the + // secret out of configuration files — and mentions a literal key last. await expect(assemble(ctx, { model: 'deepseek-v4-flash', messages: [] })) - .rejects.toThrow(/store DEEPSEEK_API_KEY with the credentials service, or export DEEPSEEK_API_KEY/) + .rejects.toThrow(/store DEEPSEEK_API_KEY through the credentials service.*as a last resort.*"apiKey"/s) }) it('prefers explicit config over env for key and base URL', async () => { diff --git a/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts b/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts index 79a8afb671..3cd430ec14 100644 --- a/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts +++ b/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts @@ -143,6 +143,29 @@ describe('request-level dynamic configuration', () => { ]) }) + it('sends the whole last-good snapshot when a rejected one changed both the key and the URL', async () => { + vi.stubEnv('DEEPSEEK_API_KEY', '') + const dir = await home() + const good = await mockServer([{ kind: 'sse', events: textEvents }]) + const rejected = await mockServer([{ kind: 'sse', events: textEvents }]) + const { ctx } = await boot(dir, { apiKey: 'good-key', baseURL: good.url }) + + // One snapshot moves the endpoint AND the literal key, and fails the + // resolve step beyond the schema (duplicate catalog ids). + await ctx.settings.update(NS, { + apiKey: 'rejected-key', + baseURL: rejected.url, + models: [{ id: 'dup' }, { id: 'dup' }], + }) + + await prompt(ctx) + // The rejected generation contributes nothing: not its endpoint, and — the + // regression this pins — not its key either. + expect(rejected.requests).toHaveLength(0) + expect(good.requests).toHaveLength(1) + expect(good.headers[0]?.authorization).toBe('Bearer good-key') + }) + it('falls back to the composition entry when settings detach', async () => { vi.stubEnv('DEEPSEEK_API_KEY', '') const dir = await home() diff --git a/packages/llm/llm-pi-ai/src/adapter.ts b/packages/llm/llm-pi-ai/src/adapter.ts index fd40c79c73..030592f74c 100644 --- a/packages/llm/llm-pi-ai/src/adapter.ts +++ b/packages/llm/llm-pi-ai/src/adapter.ts @@ -41,9 +41,11 @@ export interface PiAiAdapterOptions { /** * Resolve the credential for one already-resolved profile; called once per * stream call and frozen for that call. `undefined` defers to pi-ai's - * provider-native ambient discovery. + * provider-native ambient discovery, which the plugin allows only for a + * profile naming no credential at all; a named reference that misses throws + * `LlmError` `MISSING_CREDENTIAL` rather than falling back. */ - resolveApiKey: (profile: ResolvedPiAiProviderProfile) => Promise + resolveApiKey: (provider: string, profile: ResolvedPiAiProviderProfile) => Promise } /** @@ -180,7 +182,7 @@ export class PiAiAdapter extends LlmAdapter { model, options.reasoningEffort ?? profile.reasoning, ) - const apiKey = await this.config.resolveApiKey(profile) + const apiKey = await this.config.resolveApiKey(options.provider, profile) const consumer = new AbortController() const upstream = options.signal === undefined diff --git a/packages/llm/llm-pi-ai/src/index.ts b/packages/llm/llm-pi-ai/src/index.ts index 6140b2456d..7ff3825bf2 100644 --- a/packages/llm/llm-pi-ai/src/index.ts +++ b/packages/llm/llm-pi-ai/src/index.ts @@ -29,7 +29,8 @@ */ import type { Context } from 'cordis' -import type {} from '@deepseek-ai/dsh-llm' +import { LlmError } from '@deepseek-ai/dsh-llm' +import type { AdapterRegistrationHandle } from '@deepseek-ai/dsh-llm' import { deepEqualJson, installSettingsSection, settingsNamespace } from '@deepseek-ai/dsh-settings' import { PiAiAdapter } from './adapter.ts' import { Config, resolveProfiles } from './config.ts' @@ -45,9 +46,15 @@ export const inject = ['llm'] const NS = settingsNamespace('llm-pi-ai') -/** The registry captures these per route; a change here must re-register. */ +/** + * The registry captures these per route; a change here must re-register. + * Sorted by provider so a settings document that merely reorders its keys is + * not mistaken for a route change. + */ function registrationFacts(profiles: ReadonlyMap): unknown { - return [...profiles.entries()].map(([provider, profile]) => ({ provider, retryPolicy: profile.retryPolicy })) + return [...profiles.entries()] + .map(([provider, profile]) => ({ provider, retryPolicy: profile.retryPolicy })) + .sort((left, right) => left.provider < right.provider ? -1 : left.provider > right.provider ? 1 : 0) } /** Register one generic pi-ai adapter for all configured provider routes. */ @@ -76,17 +83,31 @@ export function apply(ctx: Context, config: Config): void { } profiles() - const resolveApiKey = async (profile: ResolvedPiAiProviderProfile): Promise => { + const resolveApiKey = async ( + provider: string, + profile: ResolvedPiAiProviderProfile, + ): Promise => { if (profile.apiKey !== undefined) return profile.apiKey const ref = profile.apiKeyEnv + // Only a profile that names no credential at all defers to pi-ai's + // provider-native discovery. Once one is named, a miss must fail loud: + // handing pi-ai `undefined` would let it pick up an unrelated ambient key + // (OPENAI_API_KEY and friends), billing another tenant for a request the + // deployment meant to authenticate differently. if (ref === undefined) return undefined const credentials = ctx.get('credentials') - if (credentials !== undefined) return (await credentials.resolve(ref))?.value - // Without the seam, keep an ambient fallback so a plain cordis.yml - // composition works from the environment alone; an empty variable defers - // to pi-ai's own provider-native discovery like an absent one. - const ambient = process.env[ref] - return ambient !== undefined && ambient.length > 0 ? ambient : undefined + const hit = credentials !== undefined + ? (await credentials.resolve(ref))?.value + // Without the seam, read exactly the named variable so a plain + // cordis.yml composition works from the environment alone. + : process.env[ref] + if (hit !== undefined && hit.length > 0) return hit + throw new LlmError( + `llm-pi-ai: no credential for provider route "${provider}"; its profile resolves ${ref}, which is not` + + ` set — store ${ref} through the credentials service (the web Models page writes it) or export it,` + + ' and remove apiKeyEnv only if this provider should authenticate from pi-ai\'s own environment discovery', + 'MISSING_CREDENTIAL', + ) } const adapter = new PiAiAdapter({ profiles, resolveApiKey }) @@ -94,18 +115,29 @@ export function apply(ctx: Context, config: Config): void { // even when a swap runs inside the scoped settings callback below. A bare // mount (zero routes) is the dormant posture: nothing registers until a // settings section supplies profiles, and routes drop when it empties. - let disposeRoutes: (() => void) | undefined + let registration: AdapterRegistrationHandle | undefined let registeredFacts: unknown const ensureRegistrationFacts = (): void => { const facts = registrationFacts(profiles()) if (deepEqualJson(facts, registeredFacts)) return // The registry captures the route set and each route's retry policy at - // registration: swap the registration in one synchronous section (same - // adapter instance, no NO_ADAPTER window). - disposeRoutes?.() - disposeRoutes = undefined + // registration, so a change to either must re-register. The swap is + // atomic (same adapter instance, validated before anything moves): a + // conflicting route leaves the previous routes serving requests, and + // `registeredFacts` only advances once the registry actually holds the + // new set — so returning to a working configuration always re-applies. const routes = [...profiles().keys()] - if (routes.length > 0) disposeRoutes = ctx.llm.registerAdapter(routes, adapter) + if (registration === undefined) { + // Dormant bare mount: nothing is registered until a section supplies + // profiles, and an empty section keeps it that way. + if (routes.length === 0) { + registeredFacts = facts + return + } + registration = ctx.llm.registerAdapter(routes, adapter) + } else { + registration.replace(routes) + } registeredFacts = facts } ensureRegistrationFacts() diff --git a/packages/llm/llm-pi-ai/tests/adapter.spec.ts b/packages/llm/llm-pi-ai/tests/adapter.spec.ts index 4b97e7b2a7..a0826b3571 100644 --- a/packages/llm/llm-pi-ai/tests/adapter.spec.ts +++ b/packages/llm/llm-pi-ai/tests/adapter.spec.ts @@ -27,7 +27,7 @@ async function harness(baseURL: string, overrides: Record = {}) function adapterOf(providers: Record): PiAiAdapter { return new PiAiAdapter({ profiles: () => resolveProfiles(providers), - resolveApiKey: profile => Promise.resolve(profile.apiKey), + resolveApiKey: (_provider, profile) => Promise.resolve(profile.apiKey), }) } @@ -385,13 +385,19 @@ describe('provider profile lifecycle', () => { expect(server.headers[0]?.authorization).toBe('Bearer custom-ref-key') }) - it('treats an empty apiKeyEnv variable as absent and defers to SDK ambient discovery', async () => { + it('fails a named-but-missing apiKeyEnv instead of using another ambient key', async () => { + // The exact confusion this guards: the named reference is empty while an + // unrelated provider key sits in the environment. Deferring to pi-ai's own + // discovery here would authenticate as another tenant. vi.stubEnv('PI_CUSTOM_REF_KEY', '') vi.stubEnv('DEEPSEEK_API_KEY', 'ambient-key') const server = await mockServer([{ events: textEvents }]) const ctx = await harness(server.url, { apiKey: undefined, apiKeyEnv: 'PI_CUSTOM_REF_KEY' }) - await assemble(ctx, { model: 'deepseek-v4-flash', messages: [] }) - expect(server.headers[0]?.authorization).toBe('Bearer ambient-key') + await expect(assemble(ctx, { model: 'deepseek-v4-flash', messages: [] })) + .rejects.toMatchObject({ code: 'MISSING_CREDENTIAL' }) + await expect(assemble(ctx, { model: 'deepseek-v4-flash', messages: [] })) + .rejects.toThrow(/provider route "deepseek".*PI_CUSTOM_REF_KEY/s) + expect(server.requests).toHaveLength(0) }) it('validates empty, unknown, legacy-shaped, and explicitly blank profiles', () => { diff --git a/packages/llm/llm-pi-ai/tests/dynamic-config.spec.ts b/packages/llm/llm-pi-ai/tests/dynamic-config.spec.ts index 4bf4d6425a..598d2aa2a9 100644 --- a/packages/llm/llm-pi-ai/tests/dynamic-config.spec.ts +++ b/packages/llm/llm-pi-ai/tests/dynamic-config.spec.ts @@ -3,7 +3,7 @@ import { Context } from 'cordis' import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import LlmService from '@deepseek-ai/dsh-llm' +import LlmService, { LlmAdapter } from '@deepseek-ai/dsh-llm' import { credentialRef } from '@deepseek-ai/dsh-credentials' import { CredentialsLocal } from '@deepseek-ai/dsh-credentials-local' import { settingsNamespace } from '@deepseek-ai/dsh-settings' @@ -14,6 +14,14 @@ import { closeMockServers, mockServer, textEvents } from './mock-server.ts' const NS = settingsNamespace('llm-pi-ai') +/** Minimal foreign adapter: only needs to own a route the pi-ai plugin then wants. */ +class StubAdapter extends LlmAdapter { + + override async * stream(): AsyncIterable { + throw new Error('stub adapter must never stream') + } +} + const cleanups: Array<() => Promise> = [] afterEach(async () => { @@ -137,4 +145,45 @@ describe('request-level dynamic profiles', () => { await ctx.settings.update(NS, { providers: { 'not-a-real-provider': {} } }) expect(ctx.llm.listProviders().map(provider => provider.id)).toEqual(['openai']) }) + + it('keeps serving its routes when a settings-born route collides with another adapter', async () => { + const dir = await home() + const server = await mockServer([{ events: textEvents }, { events: textEvents }]) + const ctx = await boot(dir, { providers: { openai: { apiKey: 'pk', baseURL: `${server.url}/v1` } } }) + // Another adapter owns `anthropic`; the registry must refuse to hand it over. + ctx.llm.registerAdapter(['anthropic'], new StubAdapter()) + + await ctx.settings.update(NS, { + providers: { + openai: { apiKey: 'pk', baseURL: `${server.url}/v1` }, + anthropic: { apiKey: 'other' }, + }, + }) + + // The conflicting swap was refused whole: the previous route set still + // owns openai (an eager dispose would have dropped it), and anthropic + // still belongs to its original adapter. + expect(ctx.llm.listProviders().map(provider => provider.id).sort()).toEqual(['anthropic', 'openai']) + const result = await assemble(ctx, { provider: 'openai', model: 'gpt-4.1', messages: [] }) + expect(result.finish.kind).toBe('error') + expect(server.paths).toEqual(['/v1/responses']) + + // Reverting to the working configuration re-applies, even though its + // facts equal the ones the registry already holds. + await ctx.settings.replace(NS, {}) + expect(ctx.llm.listProviders().map(provider => provider.id).sort()).toEqual(['anthropic', 'openai']) + await assemble(ctx, { provider: 'openai', model: 'gpt-4.1', messages: [] }) + expect(server.paths).toEqual(['/v1/responses', '/v1/responses']) + }) + + it('ignores a settings document that merely reorders its provider keys', async () => { + const dir = await home() + const ctx = await boot(dir, { providers: { openai: {}, anthropic: {} } }) + const before = ctx.llm.listProviders().map(provider => provider.id) + + // Same routes, different YAML key order: nothing about the registration + // changed, so no swap should happen at all. + await ctx.settings.update(NS, { providers: { anthropic: {}, openai: {} } }) + expect(ctx.llm.listProviders().map(provider => provider.id)).toEqual(before) + }) }) diff --git a/packages/llm/llm/src/index.ts b/packages/llm/llm/src/index.ts index 1fb4443af0..73ac6ed900 100644 --- a/packages/llm/llm/src/index.ts +++ b/packages/llm/llm/src/index.ts @@ -184,6 +184,26 @@ export abstract class LlmAdapter { abstract stream(options: GenerateOptions): AsyncIterable } +/** + * What {@link LlmService.registerAdapter} returns: the disposer, plus an + * atomic route replacement for the same adapter instance. + */ +export interface AdapterRegistrationHandle { + /** Release every route this registration currently holds. */ + (): void + /** + * Replace this registration's routes with `providers`, keeping the same + * adapter instance. The candidate set is validated in full first — a + * conflict with another adapter, an invalid name, or bad provider metadata + * throws and leaves the current routes untouched — and the swap itself is + * one synchronous section, so no request can observe a gap. An empty array + * is legal here (a settings section that emptied holds zero routes while + * staying registered), unlike an empty initial registration. + * @param providers - the complete next route set for this registration. + */ + replace(providers: string[]): void +} + /** * The abstract `llm` service: an adapter registry plus a streaming model-call * surface, interceptable via the `llm/stream` waterfall. @@ -201,39 +221,70 @@ export class LlmService extends Service { * Disposed with the fiber. * @param providers - every provider route this adapter should serve. * @param adapter - the adapter that streams calls for those providers. - * @returns the disposer that unregisters all of them. + * @returns the disposer, carrying {@link AdapterRegistrationHandle.replace}. */ - registerAdapter(providers: string[], adapter: LlmAdapter): () => void { + registerAdapter(providers: string[], adapter: LlmAdapter): AdapterRegistrationHandle { + // The routes this registration currently holds; `replace` rewrites it, and + // the disposer releases whatever it holds at disposal time. + const owned = new Set() const dispose = this.ctx.effect(function* (this: LlmService) { if (providers.length === 0) throw new LlmError('an adapter must register at least one provider', 'INVALID_ADAPTER') - const unique = new Set() - const registrations: AdapterRegistration[] = [] - for (const provider of providers) { - if (provider.length === 0) throw new LlmError('adapter provider names must be non-empty', 'INVALID_ADAPTER') - if (unique.has(provider) || this.adapters.has(provider)) { - throw new LlmError(`an adapter for provider "${provider}" is already registered`, 'DUPLICATE_ADAPTER') - } - const info = adapter.providerInfo(provider) - if (typeof info.id !== 'string' || info.id !== provider || typeof info.name !== 'string' || info.name.length === 0) { - throw new LlmError(`adapter metadata for provider "${provider}" must preserve its id and have a non-empty name`, 'INVALID_ADAPTER') - } - unique.add(provider) - const retryPolicy = adapter.providerRetryPolicy(provider) - ?? resolveRetryPolicy(undefined, `llm: provider "${provider}" retryPolicy`) - registrations.push({ - adapter, - provider: { id: info.id, name: info.name }, - retryPolicy, - }) - } - for (const registration of registrations) this.adapters.set(registration.provider.id, registration) + this.commitRoutes(owned, this.prepareRoutes(providers, adapter, owned)) yield () => { - for (const provider of providers) this.adapters.delete(provider) + for (const provider of owned) this.adapters.delete(provider) + owned.clear() } }.bind(this), 'llm.registerAdapter()') // ctx.effect's disposer returns Promise; our disposer API is // synchronous fire-and-forget — discard the (always-resolved) promise. - return () => void dispose() + const handle = (() => void dispose()) as AdapterRegistrationHandle + handle.replace = (next: string[]): void => { + this.commitRoutes(owned, this.prepareRoutes(next, adapter, owned)) + } + return handle + } + + /** + * Validate one candidate route set for `adapter`, treating routes this + * registration already holds as available. Nothing is mutated: a rejected + * candidate leaves the registry exactly as it was. + */ + private prepareRoutes(providers: string[], adapter: LlmAdapter, owned: ReadonlySet): AdapterRegistration[] { + const unique = new Set() + const registrations: AdapterRegistration[] = [] + for (const provider of providers) { + if (provider.length === 0) throw new LlmError('adapter provider names must be non-empty', 'INVALID_ADAPTER') + if (unique.has(provider) || (this.adapters.has(provider) && !owned.has(provider))) { + throw new LlmError(`an adapter for provider "${provider}" is already registered`, 'DUPLICATE_ADAPTER') + } + const info = adapter.providerInfo(provider) + if (typeof info.id !== 'string' || info.id !== provider || typeof info.name !== 'string' || info.name.length === 0) { + throw new LlmError(`adapter metadata for provider "${provider}" must preserve its id and have a non-empty name`, 'INVALID_ADAPTER') + } + unique.add(provider) + const retryPolicy = adapter.providerRetryPolicy(provider) + ?? resolveRetryPolicy(undefined, `llm: provider "${provider}" retryPolicy`) + registrations.push({ + adapter, + provider: { id: info.id, name: info.name }, + retryPolicy, + }) + } + return registrations + } + + /** + * Swap this registration's routes for the prepared ones in one synchronous + * section, so no observer can see the registry between the release and the + * re-registration. + */ + private commitRoutes(owned: Set, registrations: readonly AdapterRegistration[]): void { + for (const provider of owned) this.adapters.delete(provider) + owned.clear() + for (const registration of registrations) { + this.adapters.set(registration.provider.id, registration) + owned.add(registration.provider.id) + } } /**