fix(llm): atomic route replacement, whole-snapshot requests, and loud credential misses
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.
This commit is contained in:
@@ -27,7 +27,7 @@ async function harness(baseURL: string, overrides: Record<string, unknown> = {})
|
||||
function adapterOf(providers: Record<string, LlmPiAi.PiAiProviderProfile>): 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', () => {
|
||||
|
||||
@@ -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<never> {
|
||||
throw new Error('stub adapter must never stream')
|
||||
}
|
||||
}
|
||||
|
||||
const cleanups: Array<() => Promise<void>> = []
|
||||
|
||||
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)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user