fix(llm): capture an immutable snapshot per pi-ai operation
Review found four defects in the declared-provider work. `PiAiAdapter` reused one `Models` collection and mutated it whenever the configuration changed. `Models.streamSimple()` resolves its provider lazily — when the stream is first consumed, which is after the adapter awaits the route's credential — so a configuration change landing in that window let an in-flight request finish under a configuration it never resolved against, or fail on a provider that no longer existed. Each resolution now produces an immutable snapshot and every operation captures one before its first await, which is what makes the seam's per-step freeze (`llm.prepareCall()`) hold end to end: switching models mid-reply takes effect on the next step, never inside the one in flight. `defaultMaxTokens` was materialized from the catalog's `Model.maxTokens`. The two answer different questions: pi-ai requires that field as the model's output capability, while the seam's is a cap the deployment chose to send on requests naming none, so every request had started carrying a number nobody picked. Only an explicitly configured cap reaches the seam now. The configurable-provider directory was refreshed by disposing its registration and making a new one. A candidate set the registry refuses — a profile keyed `deepseek-official`, which llm-deepseek declares — left the whole directory withdrawn and the Models page empty, silently, because the settings callback contains the failure. The seam's registration handle now carries `replace()` with the same validate-first atomicity `registerAdapter` has. The protocol table offered every pi-ai streaming API, including four whose authentication a profile cannot express: Bedrock signs with SigV4 over AWS credentials and a region, Vertex needs a project, a location, and ADC, Azure needs provider environment plus an api-version, and Codex uses OAuth. Offering them handed back routes that cannot authenticate. Catalog routes still reach them through their own provider.
This commit is contained in:
@@ -1,10 +1,17 @@
|
||||
/**
|
||||
* Generic pi-ai-backed implementation of the Harness LLM seam.
|
||||
*
|
||||
* The adapter owns one pi-ai `Models` collection and keeps it in step with the
|
||||
* resolved profiles: each route contributes the `Provider` its resolution built,
|
||||
* so model lookup, protocol dispatch, and request auth all reach pi-ai through
|
||||
* its supported runtime rather than the deprecated global compatibility entry.
|
||||
* Each resolution produces one **immutable** snapshot — the profiles plus a
|
||||
* `Models` collection holding the `Provider` each route built — and an
|
||||
* operation captures a whole snapshot before its first `await`. A
|
||||
* configuration change builds a *new* collection rather than mutating the one
|
||||
* in use, because `Models.streamSimple()` is lazy: it resolves the provider
|
||||
* when the stream is first consumed, which is after the credential await, so a
|
||||
* mutated collection would let a request that started under one configuration
|
||||
* finish under another — or fail with a provider that no longer exists. This is
|
||||
* what makes the seam's per-step call freeze (`llm.prepareCall()`) hold all the
|
||||
* way down: switching models mid-reply takes effect on the next step, never
|
||||
* inside the one in flight.
|
||||
*
|
||||
* Credentials stay outside that collection. The harness resolves a route's key
|
||||
* through its own seam and passes it as the request's `apiKey` option, which
|
||||
@@ -18,6 +25,7 @@ import { createModels, getSupportedThinkingLevels } from '@earendil-works/pi-ai'
|
||||
import type {
|
||||
Api,
|
||||
Model,
|
||||
Models,
|
||||
ModelThinkingLevel,
|
||||
MutableModels,
|
||||
SimpleStreamOptions,
|
||||
@@ -42,6 +50,14 @@ import type { ResolvedPiAiProviderProfile } from './config.ts'
|
||||
import { toPiContext } from './context.ts'
|
||||
import { toStreamChunks } from './stream.ts'
|
||||
|
||||
/** One resolution's frozen view: the profiles and the collection built from them. */
|
||||
interface PiAiSnapshot {
|
||||
/** The resolved profiles this collection was built from, used as its identity. */
|
||||
profiles: ReadonlyMap<string, ResolvedPiAiProviderProfile>
|
||||
/** Providers for exactly those profiles; never mutated once published. */
|
||||
models: Models
|
||||
}
|
||||
|
||||
/** Constructor options for {@link PiAiAdapter}: the two resolution seams the plugin owns. */
|
||||
export interface PiAiAdapterOptions {
|
||||
/** Current validated profiles by provider route; called once per operation. */
|
||||
@@ -107,41 +123,40 @@ function requestHeaders(headers: Readonly<Record<string, string>> | undefined):
|
||||
* restart; model descriptors come from the collection those profiles built.
|
||||
*/
|
||||
export class PiAiAdapter extends LlmAdapter {
|
||||
private readonly models: MutableModels = createModels()
|
||||
private registered: ReadonlyMap<string, ResolvedPiAiProviderProfile> | undefined
|
||||
private snapshot: PiAiSnapshot | undefined
|
||||
|
||||
constructor(private readonly config: PiAiAdapterOptions) {
|
||||
super()
|
||||
}
|
||||
|
||||
/**
|
||||
* The `Models` collection for the current profiles. Resolution memoizes its
|
||||
* result, so an unchanged configuration is recognized by identity and the
|
||||
* collection is rebuilt only when the route set or any profile actually
|
||||
* changes.
|
||||
* The snapshot for the current profiles. Resolution memoizes its result, so
|
||||
* an unchanged configuration is recognized by identity; a changed one gets a
|
||||
* brand-new collection, leaving any snapshot an operation already captured
|
||||
* untouched for as long as that operation holds it.
|
||||
*/
|
||||
private collection(): MutableModels {
|
||||
private current(): PiAiSnapshot {
|
||||
const profiles = this.config.profiles()
|
||||
if (profiles === this.registered) return this.models
|
||||
this.models.clearProviders()
|
||||
for (const profile of profiles.values()) this.models.setProvider(profile.piProvider)
|
||||
this.registered = profiles
|
||||
return this.models
|
||||
if (this.snapshot?.profiles === profiles) return this.snapshot
|
||||
const models: MutableModels = createModels()
|
||||
for (const profile of profiles.values()) models.setProvider(profile.piProvider)
|
||||
this.snapshot = { profiles, models }
|
||||
return this.snapshot
|
||||
}
|
||||
|
||||
/** The profile for one route, or the seam's own not-owned failure. */
|
||||
private profileOf(provider: string): ResolvedPiAiProviderProfile {
|
||||
const profile = this.config.profiles().get(provider)
|
||||
/** The profile for one route within one snapshot, or the not-owned failure. */
|
||||
private profileOf(snapshot: PiAiSnapshot, provider: string): ResolvedPiAiProviderProfile {
|
||||
const profile = snapshot.profiles.get(provider)
|
||||
if (profile === undefined) {
|
||||
throw new LlmError(`pi-ai adapter does not own provider "${provider}"`, 'NO_ADAPTER')
|
||||
}
|
||||
return profile
|
||||
}
|
||||
|
||||
/** The configured descriptor for one exact route/model pair. */
|
||||
private modelOf(provider: string, model: string): Model<Api> {
|
||||
this.profileOf(provider)
|
||||
const resolved = this.collection().getModel(provider, model)
|
||||
/** The configured descriptor for one exact route/model pair within one snapshot. */
|
||||
private modelOf(snapshot: PiAiSnapshot, provider: string, model: string): Model<Api> {
|
||||
this.profileOf(snapshot, provider)
|
||||
const resolved = snapshot.models.getModel(provider, model)
|
||||
if (resolved === undefined) {
|
||||
throw new LlmError(`pi-ai provider "${provider}" has no configured model "${model}"`, 'UNKNOWN_MODEL')
|
||||
}
|
||||
@@ -149,13 +164,14 @@ export class PiAiAdapter extends LlmAdapter {
|
||||
}
|
||||
|
||||
override providerRetryPolicy(provider: string): ResolvedRetryPolicy | undefined {
|
||||
return this.config.profiles().get(provider)?.retryPolicy
|
||||
return this.current().profiles.get(provider)?.retryPolicy
|
||||
}
|
||||
|
||||
override listModels(provider: string): Promise<readonly LlmModelInfo[]> {
|
||||
return Promise.resolve().then(() => {
|
||||
this.profileOf(provider)
|
||||
return this.collection().getModels(provider).map(model => ({
|
||||
const snapshot = this.current()
|
||||
this.profileOf(snapshot, provider)
|
||||
return snapshot.models.getModels(provider).map(model => ({
|
||||
provider,
|
||||
id: model.id,
|
||||
name: model.name,
|
||||
@@ -169,16 +185,20 @@ export class PiAiAdapter extends LlmAdapter {
|
||||
_signal?: AbortSignal,
|
||||
): Promise<LlmResolvedModelInfo> {
|
||||
return Promise.resolve().then(() => {
|
||||
const profile = this.profileOf(provider)
|
||||
const resolvedModel = this.modelOf(provider, model)
|
||||
const snapshot = this.current()
|
||||
const profile = this.profileOf(snapshot, provider)
|
||||
const resolvedModel = this.modelOf(snapshot, provider, model)
|
||||
const levels = getSupportedThinkingLevels(resolvedModel)
|
||||
const defaultLevel = resolveReasoningLevel(resolvedModel, profile.reasoning)
|
||||
// Only a cap the deployment configured is a request default; the
|
||||
// catalog's `maxTokens` sizes the model and stops there.
|
||||
const configuredMaxTokens = profile.configuredMaxTokens.get(model)
|
||||
return {
|
||||
provider,
|
||||
id: model,
|
||||
name: resolvedModel.name,
|
||||
context: { contextWindow: resolvedModel.contextWindow },
|
||||
defaultMaxTokens: resolvedModel.maxTokens,
|
||||
...configuredMaxTokens === undefined ? {} : { defaultMaxTokens: configuredMaxTokens },
|
||||
reasoning: {
|
||||
efforts: levels.map(level => ({
|
||||
id: ReasoningEffortId(level),
|
||||
@@ -196,13 +216,14 @@ export class PiAiAdapter extends LlmAdapter {
|
||||
if (options.stop !== undefined) {
|
||||
throw new LlmError('llm-pi-ai does not support GenerateOptions.stop', 'UNSUPPORTED_OPTION')
|
||||
}
|
||||
// One resolution per stream call: the profile snapshot, the model
|
||||
// descriptor, 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.
|
||||
const profile = this.profileOf(options.provider)
|
||||
const collection = this.collection()
|
||||
const model = this.modelOf(options.provider, options.model)
|
||||
// One capture per stream call, taken before any await: the profile, the
|
||||
// model descriptor, and the collection all come from the same immutable
|
||||
// snapshot, and the credential freezes with them. A configuration change
|
||||
// mid-request builds a separate snapshot, so this request finishes under
|
||||
// the one it started with and the next call picks up the new one.
|
||||
const snapshot = this.current()
|
||||
const profile = this.profileOf(snapshot, options.provider)
|
||||
const model = this.modelOf(snapshot, options.provider, options.model)
|
||||
const reasoning = resolveReasoningLevel(
|
||||
model,
|
||||
options.reasoningEffort ?? profile.reasoning,
|
||||
@@ -217,7 +238,7 @@ export class PiAiAdapter extends LlmAdapter {
|
||||
using watchdog = idleWatchdog(upstream, streamIdleTimeoutMs, 'LLM_STREAM_IDLE_TIMEOUT')
|
||||
|
||||
try {
|
||||
const events = collection.streamSimple(model, toPiContext(options), {
|
||||
const events = snapshot.models.streamSimple(model, toPiContext(options), {
|
||||
...profileOptions(profile, reasoning, apiKey),
|
||||
...options.temperature === undefined ? {} : { temperature: options.temperature },
|
||||
...options.maxTokens === undefined ? {} : { maxTokens: options.maxTokens },
|
||||
|
||||
@@ -79,7 +79,11 @@ export interface PiAiModelProfile {
|
||||
name?: string
|
||||
/** Maximum combined request and response context in tokens. */
|
||||
contextWindow?: number
|
||||
/** Per-request output cap materialized when a caller omits one. */
|
||||
/**
|
||||
* Maximum output tokens. Configuring one also makes it this model's
|
||||
* per-request default; the value inherited from the installed catalog is the
|
||||
* model's capability and never becomes a request default on its own.
|
||||
*/
|
||||
maxTokens?: number
|
||||
/** Whether the model exposes reasoning; defaults to the catalog capability. */
|
||||
reasoning?: boolean
|
||||
@@ -116,15 +120,32 @@ function sharedCatalogApi(defaults: ReadonlyMap<string, Model<Api>>): string | u
|
||||
return apis.size === 1 ? [...apis][0] : undefined
|
||||
}
|
||||
|
||||
/** One route's materialized catalog, plus the request caps its profile chose. */
|
||||
export interface RouteCatalog {
|
||||
/** The materialized models in configuration order. */
|
||||
models: readonly Model<Api>[]
|
||||
/**
|
||||
* Per-request output caps this profile explicitly configured, by model id.
|
||||
*
|
||||
* Separate from `Model.maxTokens` because the two answer different
|
||||
* questions: pi-ai requires `maxTokens` as the model's output *capability*,
|
||||
* while the harness seam's `defaultMaxTokens` is a cap the deployment chose
|
||||
* to send on requests that name none. Materializing a catalog capability as
|
||||
* a request default would start capping every request at a number nobody
|
||||
* picked, so only an explicit configuration lands here.
|
||||
*/
|
||||
configuredMaxTokens: ReadonlyMap<string, number>
|
||||
}
|
||||
|
||||
/**
|
||||
* Materialize one route's catalog by merging the installed catalog defaults
|
||||
* under the configured entries. A route with no configured `models` serves the
|
||||
* installed catalog unchanged, which is what keeps an existing
|
||||
* `providers: { deepseek: { apiKeyEnv: … } }` profile working untouched.
|
||||
* @param request - the route-level catalog facts.
|
||||
* @returns the materialized models in configuration order.
|
||||
* @returns the materialized models and the explicitly configured request caps.
|
||||
*/
|
||||
export function resolveRouteModels(request: RouteCatalogRequest): readonly Model<Api>[] {
|
||||
export function resolveRouteModels(request: RouteCatalogRequest): RouteCatalog {
|
||||
const { provider } = request
|
||||
const defaults = catalogModels(provider)
|
||||
const providerBaseUrl = catalogProvider(provider)?.baseUrl
|
||||
@@ -141,7 +162,8 @@ export function resolveRouteModels(request: RouteCatalogRequest): readonly Model
|
||||
}
|
||||
const routeApi = sharedCatalogApi(defaults)
|
||||
const seen = new Set<string>()
|
||||
return entries.map((entry) => {
|
||||
const configuredMaxTokens = new Map<string, number>()
|
||||
const models = entries.map((entry) => {
|
||||
if (entry.id.length === 0) invalid(provider, 'has a model with an empty id')
|
||||
if (seen.has(entry.id)) invalid(provider, `lists model "${entry.id}" more than once`)
|
||||
seen.add(entry.id)
|
||||
@@ -171,6 +193,9 @@ export function resolveRouteModels(request: RouteCatalogRequest): readonly Model
|
||||
if (!Number.isInteger(maxTokens) || maxTokens <= 0) {
|
||||
invalid(provider, `model "${entry.id}" maxTokens must be a positive integer`)
|
||||
}
|
||||
// Only a value the profile named is a deployment choice; the catalog's is
|
||||
// the model's capability and stays out of request defaults.
|
||||
if (entry.maxTokens !== undefined) configuredMaxTokens.set(entry.id, entry.maxTokens)
|
||||
return {
|
||||
id: entry.id,
|
||||
name: entry.name ?? base?.name ?? entry.id,
|
||||
@@ -190,4 +215,5 @@ export function resolveRouteModels(request: RouteCatalogRequest): readonly Model
|
||||
...base?.headers === undefined ? {} : { headers: base.headers },
|
||||
}
|
||||
})
|
||||
return { models, configuredMaxTokens }
|
||||
}
|
||||
|
||||
@@ -92,6 +92,12 @@ export interface ResolvedPiAiProviderProfile
|
||||
* serving requests.
|
||||
*/
|
||||
piProvider: Provider
|
||||
/**
|
||||
* Per-request output caps this profile explicitly configured, by model id.
|
||||
* The seam materializes one only into a request that names no cap of its
|
||||
* own, so a catalog capability must not appear here.
|
||||
*/
|
||||
configuredMaxTokens: ReadonlyMap<string, number>
|
||||
}
|
||||
|
||||
/** Plugin configuration: the provider routes this instance owns. */
|
||||
@@ -200,7 +206,7 @@ export function resolveProfiles(
|
||||
// always shown route keys, and a catalog route must not silently rename
|
||||
// itself on every configuration surface just because it gained a profile.
|
||||
const displayName = source.displayName ?? provider
|
||||
const models = resolveRouteModels({
|
||||
const catalog = resolveRouteModels({
|
||||
provider,
|
||||
...source.api === undefined ? {} : { api: source.api },
|
||||
...source.baseURL === undefined ? {} : { baseURL: source.baseURL },
|
||||
@@ -216,12 +222,13 @@ export function resolveProfiles(
|
||||
retryPolicy: resolveRetryPolicy(retryPolicy, `llm-pi-ai: provider "${provider}" retryPolicy`),
|
||||
...rest.headers === undefined ? {} : { headers: { ...rest.headers } },
|
||||
...rest.thinkingBudgets === undefined ? {} : { thinkingBudgets: { ...rest.thinkingBudgets } },
|
||||
configuredMaxTokens: catalog.configuredMaxTokens,
|
||||
piProvider: buildProvider({
|
||||
provider,
|
||||
displayName,
|
||||
...source.api === undefined ? {} : { api: source.api },
|
||||
...source.baseURL === undefined ? {} : { baseURL: source.baseURL },
|
||||
models,
|
||||
models: catalog.models,
|
||||
}),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -44,7 +44,7 @@
|
||||
|
||||
import type { Context } from 'cordis'
|
||||
import { LlmError } from '@deepseek-ai/dsh-llm'
|
||||
import type { AdapterRegistrationHandle, LlmConfigurableProvider } from '@deepseek-ai/dsh-llm'
|
||||
import type { AdapterRegistrationHandle, DirectoryRegistrationHandle, LlmConfigurableProvider } from '@deepseek-ai/dsh-llm'
|
||||
import { deepEqualJson, installSettingsSection, settingsNamespace } from '@deepseek-ai/dsh-settings'
|
||||
import { PiAiAdapter } from './adapter.ts'
|
||||
import { catalogProviderIds } from './catalog.ts'
|
||||
@@ -151,13 +151,21 @@ export function apply(ctx: Context, config: Config): void {
|
||||
// mounts — dormant or not — so configuration surfaces can offer every
|
||||
// pi-ai provider before any route exists. Hand-declared routes join it as
|
||||
// profiles appear, and leave with them.
|
||||
let directory: (() => void) | undefined
|
||||
let directory: DirectoryRegistrationHandle | undefined
|
||||
let directoryFacts: unknown
|
||||
const ensureDirectory = (): void => {
|
||||
const entries = directoryEntries(profiles())
|
||||
if (deepEqualJson(entries, directoryFacts)) return
|
||||
directory?.()
|
||||
directory = ctx.llm.registerConfigurableProviders(entries)
|
||||
// Atomic replace, never dispose-then-register: a route another adapter
|
||||
// family already declares (a profile keyed `deepseek-official`) would
|
||||
// otherwise leave this plugin's whole directory withdrawn and the Models
|
||||
// page empty. The candidate set is validated first, so a collision keeps
|
||||
// the previous entries serving and only costs a diagnostic.
|
||||
if (directory === undefined) {
|
||||
directory = ctx.llm.registerConfigurableProviders(entries)
|
||||
} else {
|
||||
directory.replace(entries)
|
||||
}
|
||||
directoryFacts = entries
|
||||
}
|
||||
ensureDirectory()
|
||||
@@ -199,8 +207,16 @@ export function apply(ctx: Context, config: Config): void {
|
||||
onChange: () => {
|
||||
ensureRegistrationFacts()
|
||||
// The directory follows the profiles the registry accepted, so a route
|
||||
// that failed to register is not advertised as configurable.
|
||||
ensureDirectory()
|
||||
// that failed to register is not advertised as configurable. A refused
|
||||
// directory swap is contained here for the same reason the registry's
|
||||
// is: the previous entries keep serving, and `directoryFacts` stays put
|
||||
// so returning to a working configuration re-applies.
|
||||
try {
|
||||
ensureDirectory()
|
||||
} catch (error) {
|
||||
ctx.logger.error('llm-pi-ai: keeping the previous configurable-provider directory after a refused update')
|
||||
ctx.logger.error(error)
|
||||
}
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
@@ -22,12 +22,8 @@
|
||||
import { createProvider } from '@earendil-works/pi-ai'
|
||||
import type { Api, ApiKeyAuth, Model, Provider, ProviderStreams } from '@earendil-works/pi-ai'
|
||||
import { anthropicMessagesApi } from '@earendil-works/pi-ai/api/anthropic-messages.lazy'
|
||||
import { azureOpenAIResponsesApi } from '@earendil-works/pi-ai/api/azure-openai-responses.lazy'
|
||||
import { bedrockConverseStreamApi } from '@earendil-works/pi-ai/api/bedrock-converse-stream.lazy'
|
||||
import { googleGenerativeAIApi } from '@earendil-works/pi-ai/api/google-generative-ai.lazy'
|
||||
import { googleVertexApi } from '@earendil-works/pi-ai/api/google-vertex.lazy'
|
||||
import { mistralConversationsApi } from '@earendil-works/pi-ai/api/mistral-conversations.lazy'
|
||||
import { openAICodexResponsesApi } from '@earendil-works/pi-ai/api/openai-codex-responses.lazy'
|
||||
import { openAICompletionsApi } from '@earendil-works/pi-ai/api/openai-completions.lazy'
|
||||
import { openAIResponsesApi } from '@earendil-works/pi-ai/api/openai-responses.lazy'
|
||||
import { piMessagesApi } from '@earendil-works/pi-ai/api/pi-messages.lazy'
|
||||
@@ -35,18 +31,24 @@ import { catalogProvider } from './catalog.ts'
|
||||
|
||||
/**
|
||||
* Wire protocols a configured route may name, mapped to pi-ai's lazily loaded
|
||||
* implementations. The table is pi-ai's own streaming API set: each entry is
|
||||
* the factory that pi-ai's matching provider factory uses, so a hand-declared
|
||||
* route reaches exactly the implementation a catalog route would.
|
||||
* implementations. Each entry is the factory that pi-ai's matching provider
|
||||
* factory uses, so a hand-declared route reaches exactly the implementation a
|
||||
* catalog route would.
|
||||
*
|
||||
* The table is deliberately narrower than pi-ai's full streaming API set: it
|
||||
* holds only the protocols a profile can *completely* describe with a key, an
|
||||
* endpoint, and headers. Bedrock signs with SigV4 over AWS credentials and a
|
||||
* region, Vertex needs a project, a location, and application-default
|
||||
* credentials, Azure needs provider environment plus an api-version, and
|
||||
* Codex authenticates through OAuth — none of which this configuration shape
|
||||
* can express, so offering them would hand back a provider that cannot
|
||||
* authenticate. Catalog routes still reach those protocols through their own
|
||||
* provider; only an explicit override is refused.
|
||||
*/
|
||||
const PROTOCOLS: Readonly<Record<string, () => ProviderStreams>> = {
|
||||
'anthropic-messages': anthropicMessagesApi,
|
||||
'azure-openai-responses': azureOpenAIResponsesApi,
|
||||
'bedrock-converse-stream': bedrockConverseStreamApi,
|
||||
'google-generative-ai': googleGenerativeAIApi,
|
||||
'google-vertex': googleVertexApi,
|
||||
'mistral-conversations': mistralConversationsApi,
|
||||
'openai-codex-responses': openAICodexResponsesApi,
|
||||
'openai-completions': openAICompletionsApi,
|
||||
'openai-responses': openAIResponsesApi,
|
||||
'pi-messages': piMessagesApi,
|
||||
|
||||
Reference in New Issue
Block a user