diff --git a/.changeset/fuzzy-pandas-refresh.md b/.changeset/fuzzy-pandas-refresh.md new file mode 100644 index 0000000000..ecd821e6f2 --- /dev/null +++ b/.changeset/fuzzy-pandas-refresh.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix sporadic "model is not configured" errors when starting kimi web, caused by the background provider-model refresh transiently clearing the model catalog while the first session was being created. diff --git a/packages/agent-core-v2/src/app/config/config.ts b/packages/agent-core-v2/src/app/config/config.ts index 6c51b63a76..d2d9a8a299 100644 --- a/packages/agent-core-v2/src/app/config/config.ts +++ b/packages/agent-core-v2/src/app/config/config.ts @@ -203,6 +203,19 @@ export interface IConfigService { getAll(): ResolvedConfig; set(domain: string, patch: unknown, target?: ConfigTarget): Promise; replace(domain: string, value: unknown, target?: ConfigTarget): Promise; + /** + * Replaces several sections in ONE state transition: every domain's raw + * value is updated first, the document is persisted with a single disk + * write, and the effective view is rebuilt once — change events fire only + * after all domains have taken effect, so a reader can never observe a + * half-applied multi-section write. A domain mapped to `undefined` is + * cleared (same as `replace(domain, undefined)`). Domain application order + * follows the `sections` key order. + */ + replaceSections( + sections: Readonly>, + target?: ConfigTarget, + ): Promise; reload(): Promise; diagnostics(): readonly ConfigDiagnostic[]; } diff --git a/packages/agent-core-v2/src/app/config/configService.ts b/packages/agent-core-v2/src/app/config/configService.ts index 43675a718a..77a46b2c4d 100644 --- a/packages/agent-core-v2/src/app/config/configService.ts +++ b/packages/agent-core-v2/src/app/config/configService.ts @@ -354,6 +354,43 @@ export class ConfigService extends Disposable implements IConfigService { }); } + async replaceSections( + sections: Readonly>, + target: ConfigTarget = ConfigTarget.User, + ): Promise { + await this.ready; + const domains = Object.keys(sections); + if (domains.length === 0) return; + if (target === ConfigTarget.Memory) { + const staged: ResolvedConfig = { ...this.memory }; + for (const domain of domains) { + const value = sections[domain]; + if (value === undefined) { + delete staged[domain]; + } else { + staged[domain] = this.registry.validate(domain, value); + } + } + this.memory = staged; + this.commit('set', domains); + return; + } + await this.enqueueStateTransition(async () => { + const staged: ResolvedConfig = { ...this.raw }; + for (const domain of domains) { + const stripped = this.stripEnv(domain, sections[domain]); + if (stripped === undefined) { + delete staged[domain]; + } else { + staged[domain] = this.registry.validate(domain, stripped); + } + } + this.raw = staged; + await this.persistDomains(domains); + this.rebuildEffective('set', domains); + }); + } + private stripEnv(domain: string, value: unknown): unknown { let result = value; const section = this.registry.getSection(domain); @@ -565,7 +602,13 @@ export class ConfigService extends Disposable implements IConfigService { } private async persist(domain: string): Promise { - applySectionToToml(this.rawSnake, domain, this.raw[domain], this.registry); + await this.persistDomains([domain]); + } + + private async persistDomains(domains: readonly string[]): Promise { + for (const domain of domains) { + applySectionToToml(this.rawSnake, domain, this.raw[domain], this.registry); + } await this.documentStore.set(CONFIG_SCOPE, this.configKey, this.rawSnake); } } diff --git a/packages/agent-core-v2/src/app/kosongConfig/discoveryService.ts b/packages/agent-core-v2/src/app/kosongConfig/discoveryService.ts index 3414f97636..f41041de89 100644 --- a/packages/agent-core-v2/src/app/kosongConfig/discoveryService.ts +++ b/packages/agent-core-v2/src/app/kosongConfig/discoveryService.ts @@ -3,10 +3,11 @@ * * Owns the all-provider model refresh: delegates to the shared * `@moonshot-ai/kimi-code-oauth` orchestrator (managed OAuth + open - * platforms + custom registries), applies the discovered providers/models - * to kosong's in-memory registries (the persistence bridge writes them back - * to config), and publishes `event.model_catalog.changed` on change. Bound - * at App scope. + * platforms + custom registries), writes the discovered providers/models + * into config through ONE atomic `replaceSections` transition (the + * persistence bridge then syncs them into kosong's in-memory registries), + * and publishes `event.model_catalog.changed` on change. Bound at App + * scope. * * `modelSource: 'static'` short-circuits refresh: a provider whose effective * model source is `static` (config-declared, or declared by its vendor @@ -18,14 +19,18 @@ * refresh them nor drop them (or a default model pointing at them). * * Two write-path details preserve the legacy semantics exactly: - * - Registry replaces preserve the entries the orchestrator could not see: - * the static exclusion AND the config-file-external entries (the - * env-synthesized `__kimi_env__` slice), which the orchestrator's - * user-value view does not contain. - * - `defaultModel` / `thinking` stay direct `config.replace` writes (like - * the OAuth flows): the env overlay may pin the runtime default to the - * env-synthesized model, and only the config effective view knows that — - * the bridge then syncs the effective pointer into the registry. + * - The orchestrator's two-phase host contract (removeProvider, then + * setConfig) is absorbed into a single atomic write: the removal is + * computed in memory only (`shapeWithoutProvider`), because the patch's + * full providers/models records already express it. The runtime + * registries therefore never pass through a halfway-removed state — that + * intermediate state was the source of the "provider/model not + * configured" startup race against profile binding. + * - The env-synthesized `__kimi_env__` slice is never written to config: + * it lives in the effective overlay, and the bridge's event-driven sync + * carries it into the registries on its own. `defaultModel` / `thinking` + * also go through config (like the OAuth flows), since the env overlay + * may pin the runtime default and only the config effective view knows. * * Credential detection goes through the provider-definition registry * (`resolveProviderEndpoint` against the provider's config env bag), not a @@ -47,7 +52,7 @@ import { IConfigService } from '#/app/config/config'; import { IEventService } from '#/app/event/event'; import { ModelCatalogErrors } from '#/kosong/model/errors'; import { IHostRequestHeaders } from '#/kosong/model/hostRequestHeaders'; -import { IModelService, type ModelRecord } from '#/kosong/model/model'; +import { type ModelRecord } from '#/kosong/model/model'; import { IProviderService, type ModelSource, @@ -88,7 +93,6 @@ export class ProviderDiscoveryService implements IProviderDiscoveryService { private refreshChain: Promise = Promise.resolve(); constructor( - @IModelService private readonly modelService: IModelService, @IProviderService private readonly providerService: IProviderService, @IConfigService private readonly config: IConfigService, @IOAuthService private readonly oauth: IOAuthService, @@ -188,7 +192,7 @@ export class ProviderDiscoveryService implements IProviderDiscoveryService { private buildRefreshHost(exclusion: StaticExclusion): RefreshProviderHost { return { getConfig: async () => this.readUserConfigShape(exclusion), - removeProvider: (providerId) => this.removeProviderForRefresh(providerId), + removeProvider: (providerId) => this.shapeWithoutProvider(providerId), setConfig: (patch) => this.applyRefreshPatch(patch, exclusion), resolveOAuthToken: (providerName, oauthRef) => this.resolveOAuthToken(providerName, oauthRef), userAgent: this.hostRequestHeaders.headers['User-Agent'], @@ -212,23 +216,16 @@ export class ProviderDiscoveryService implements IProviderDiscoveryService { } /** - * The registry entries the orchestrator's user-value view cannot see (the - * env-synthesized slice): preserved verbatim across every registry - * replace, or a refresh would drop the runtime env model/provider. + * The orchestrator's host contract is two-phase (removeProvider, then + * setConfig) because its original merge-semantics host could not delete + * keys through a patch. This host writes with replace semantics and the + * patch always carries the FULL providers/models records, so the removal + * is already expressed by the patch itself — computing it here in memory + * keeps the runtime registries untouched until the single atomic write in + * {@link applyRefreshPatch} (a halfway-removed catalog is what used to + * produce the "provider/model not configured" startup race). */ - private syntheticProviders( - userProviders: Readonly>, - ): Record { - return withoutKeys(this.providerService.list(), userProviders); - } - - private syntheticModels( - userModels: Readonly>, - ): Record { - return withoutKeys(this.modelService.list(), userModels); - } - - private async removeProviderForRefresh(providerId: string): Promise { + private shapeWithoutProvider(providerId: string): Promise { const current = this.readUserConfigShape(); const providers = current.providers as Record; const restProviders = Object.fromEntries( @@ -238,16 +235,11 @@ export class ProviderDiscoveryService implements IProviderDiscoveryService { const restModels = Object.fromEntries( Object.entries(models).filter(([, record]) => record.provider !== providerId), ); - await this.providerService.replaceAll({ - ...this.syntheticProviders(providers), - ...restProviders, - }); - await this.modelService.replaceAll({ ...this.syntheticModels(models), ...restModels }); - return { + return Promise.resolve({ ...current, providers: restProviders, models: restModels, - } as ManagedKimiConfigShape; + } as ManagedKimiConfigShape); } private async applyRefreshPatch( @@ -258,56 +250,50 @@ export class ProviderDiscoveryService implements IProviderDiscoveryService { this.config.inspect>(PROVIDERS_SECTION).userValue ?? {}; const userModels = this.config.inspect>(MODELS_SECTION).userValue ?? {}; + // All four sections land in ONE config transition: a single disk write, + // then one effective rebuild whose change events synchronously push the + // new records into the kosong registries — no reader can observe a + // half-applied refresh. The env-synthesized slice (`__kimi_env__` etc.) + // is NOT written here: it lives in the effective overlay, and the + // bridge's event-driven sync carries it into the registries on its own. + const sections: Record = {}; if (patch.providers !== undefined) { - await this.providerService.replaceAll({ - ...this.syntheticProviders(userProviders), + sections[PROVIDERS_SECTION] = { ...exclusion.providers, ...patch.providers, - }); + }; } if (patch.models !== undefined) { - await this.modelService.replaceAll({ - ...this.syntheticModels(userModels), + sections[MODELS_SECTION] = { ...exclusion.models, // The orchestrator's alias shape is a structural superset of // ModelRecord at runtime (its protocol union additionally allows // vendor spellings the records never actually carry); the legacy // config.write path took `unknown`, so cast here. ...(patch.models as Record), - }); + }; } // The refresh orchestrator always sends all four keys, so key presence is // the write intent and an explicit `undefined` means CLEAR, not "leave - // alone". `set()` cannot express that — its deepMerge resolves an - // undefined patch back to the base value — so these go through `replace`, - // which deletes the section on undefined. Otherwise a default model (and - // its thinking setting) whose alias the upstream dropped would dangle in - // the user config forever. + // alone" — `replaceSections` deletes the section on undefined. Otherwise + // a default model (and its thinking setting) whose alias the upstream + // dropped would dangle in the user config forever. // // Exception: when the user's default points at a statically-sourced model // the orchestrator could not see, its clamp/restore logic would silently // clear or re-point the selection (and its thinking) — restore both. - // - // `defaultModel` / `thinking` go through config directly (not the - // registry): the env overlay may pin the runtime default, and only the - // config effective view knows — the bridge syncs the effective pointer - // into the registry afterwards. const restoreDefault = exclusion.defaultModel !== undefined; if ('defaultModel' in patch) { - await this.config.replace( - DEFAULT_MODEL_SECTION, - restoreDefault ? exclusion.defaultModel : patch.defaultModel, - ); + sections[DEFAULT_MODEL_SECTION] = restoreDefault + ? exclusion.defaultModel + : patch.defaultModel; } if ('thinking' in patch) { - await this.config.replace( - THINKING_SECTION, - restoreDefault ? exclusion.thinking : patch.thinking, - ); + sections[THINKING_SECTION] = restoreDefault ? exclusion.thinking : patch.thinking; } - // The writes above landed in the registries / config; compute the - // post-patch shape in memory (re-reading config would race the bridge's - // asynchronous persist of the registry changes). + await this.config.replaceSections(sections); + // The write above landed in config (and, through the bridge's synchronous + // event sync, the registries); compute the post-patch shape in memory. return { providers: patch.providers !== undefined diff --git a/packages/agent-core-v2/test/app/config/config.test.ts b/packages/agent-core-v2/test/app/config/config.test.ts index fe51e6af4b..f6842a525e 100644 --- a/packages/agent-core-v2/test/app/config/config.test.ts +++ b/packages/agent-core-v2/test/app/config/config.test.ts @@ -12,7 +12,7 @@ import type { ToolCall } from '#/kosong/contract/message'; import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'pathe'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { IAgentProfileService, type ResolvedAgentProfile } from '#/agent/profile/profile'; import { Error2, ErrorCodes, toErrorPayload } from '#/errors'; @@ -24,7 +24,7 @@ import { SyncDescriptor } from '#/_base/di/descriptors'; import { DisposableStore } from '#/_base/di/lifecycle'; import { TestInstantiationService } from '#/_base/di/test'; import { IBootstrapService } from '#/app/bootstrap/bootstrap'; -import { IConfigRegistry, IConfigService } from '#/app/config/config'; +import { ConfigTarget, IConfigRegistry, IConfigService } from '#/app/config/config'; import { ConfigRegistry, ConfigService } from '#/app/config/configService'; import { SECONDARY_MODEL_FLAG_ID } from '#/session/subagent/flag'; import '#/app/cron/configSection'; @@ -46,7 +46,9 @@ import { type LoopControl, } from '#/agent/loop/configSection'; import { + DEFAULT_MODEL_SECTION, MODELS_SECTION, + PROVIDERS_SECTION, SECONDARY_MODEL_EFFORT_ENV, SECONDARY_MODEL_ENV, SECONDARY_MODEL_SECTION, @@ -1889,3 +1891,153 @@ describe('ConfigService thinking effort max migration', () => { disposables.dispose(); }); }); + +describe('ConfigService replaceSections', () => { + // Top-level keys must precede every [table] header in TOML. + const SEED_TOML = [ + 'default_model = "acme/m1"', + '', + '[providers.acme]', + 'type = "openai"', + 'api_key = "sk-acme"', + '', + '[models."acme/m1"]', + 'provider = "acme"', + 'model = "m1"', + 'max_context_size = 1000', + '', + '[thinking]', + 'enabled = true', + '', + ].join('\n'); + + async function createSectionsConfig(toml = SEED_TOML) { + const disposables = new DisposableStore(); + const ix = disposables.add(new TestInstantiationService()); + const storage = new InMemoryStorageService(); + await storage.write('', 'config.toml', new TextEncoder().encode(toml)); + ix.stub(ILogService, stubLog()); + ix.stub(IBootstrapService, stubBootstrap('/tmp/kimi-cfg-replace-sections')); + ix.stub(IFileSystemStorageService, storage); + ix.set(IAtomicTomlDocumentStore, new SyncDescriptor(TomlAtomicDocumentStore)); + ix.set(IConfigRegistry, new SyncDescriptor(ConfigRegistry)); + ix.set(IConfigService, new SyncDescriptor(ConfigService)); + const config = ix.get(IConfigService); + await config.ready; + const store = ix.get(IAtomicTomlDocumentStore); + return { config, disposables, store, storage }; + } + + it('applies every domain in one transition with a single disk write, clearing undefined domains', async () => { + const { config, disposables, store } = await createSectionsConfig(); + const setSpy = vi.spyOn(store, 'set'); + + await config.replaceSections({ + [PROVIDERS_SECTION]: { acme: { type: 'openai', apiKey: 'sk-acme-2' } }, + [MODELS_SECTION]: { 'acme/m2': { provider: 'acme', model: 'm2', maxContextSize: 2000 } }, + [DEFAULT_MODEL_SECTION]: undefined, + [THINKING_SECTION]: undefined, + }); + + expect(setSpy).toHaveBeenCalledTimes(1); + expect(config.get>(PROVIDERS_SECTION)).toEqual({ + acme: { type: 'openai', apiKey: 'sk-acme-2' }, + }); + expect(config.get>(MODELS_SECTION)).toEqual({ + 'acme/m2': { provider: 'acme', model: 'm2', maxContextSize: 2000 }, + }); + expect(config.get(DEFAULT_MODEL_SECTION)).toBeUndefined(); + expect(config.get(THINKING_SECTION)).toEqual({}); + expect(config.inspect(DEFAULT_MODEL_SECTION).userValue).toBeUndefined(); + // `stripThinkingEnv` maps a clear to `{}` (`{...undefined}`), so the user + // layer collapses to an empty object instead of disappearing — the + // long-standing `replace(domain, undefined)` behavior, unchanged here. + expect(config.inspect(THINKING_SECTION).userValue).toEqual({}); + + disposables.dispose(); + }); + + it('fires change events only after all domains have taken effect', async () => { + const { config, disposables } = await createSectionsConfig(); + const domains: string[] = []; + let snapshotDuringFirstEvent: + | { providers: unknown; models: unknown; defaultModel: unknown; thinking: unknown } + | undefined; + config.onDidSectionChange((e) => { + domains.push(e.domain); + snapshotDuringFirstEvent ??= { + providers: config.get(PROVIDERS_SECTION), + models: config.get(MODELS_SECTION), + defaultModel: config.get(DEFAULT_MODEL_SECTION), + thinking: config.get(THINKING_SECTION), + }; + }); + + await config.replaceSections({ + [PROVIDERS_SECTION]: { acme: { type: 'openai', apiKey: 'sk-acme-2' } }, + [MODELS_SECTION]: { 'acme/m2': { provider: 'acme', model: 'm2', maxContextSize: 2000 } }, + [DEFAULT_MODEL_SECTION]: undefined, + [THINKING_SECTION]: undefined, + }); + + // Every event — including the very first one — already observes the fully + // applied state; no listener can catch the write half-applied. (The + // cleared thinking section still resolves to its schema default `{}`.) + expect(snapshotDuringFirstEvent).toEqual({ + providers: { acme: { type: 'openai', apiKey: 'sk-acme-2' } }, + models: { 'acme/m2': { provider: 'acme', model: 'm2', maxContextSize: 2000 } }, + defaultModel: undefined, + thinking: {}, + }); + expect([...domains].sort()).toEqual( + [PROVIDERS_SECTION, MODELS_SECTION, DEFAULT_MODEL_SECTION, THINKING_SECTION].sort(), + ); + + disposables.dispose(); + }); + + it('supports the memory target without touching the persisted user layer', async () => { + const { config, disposables, store } = await createSectionsConfig(); + const setSpy = vi.spyOn(store, 'set'); + + await config.replaceSections( + { [THINKING_SECTION]: { enabled: false, effort: 'low' } }, + ConfigTarget.Memory, + ); + + expect(setSpy).not.toHaveBeenCalled(); + expect(config.get(THINKING_SECTION)).toEqual({ + enabled: false, + effort: 'low', + }); + expect(config.inspect(THINKING_SECTION).userValue).toEqual({ enabled: true }); + + disposables.dispose(); + }); + + it('leaves the user layer untouched when a later domain fails validation', async () => { + const { config, disposables, store } = await createSectionsConfig(); + const setSpy = vi.spyOn(store, 'set'); + + // Providers is applied first in key order and validates fine; thinking + // then fails schema validation (`enabled` must be a boolean). The batch + // must reject with NO observable partial application. + await expect( + config.replaceSections({ + [PROVIDERS_SECTION]: { acme: { type: 'openai', apiKey: 'sk-acme-2' } }, + [THINKING_SECTION]: { enabled: 'yes' }, + }), + ).rejects.toThrow(); + + expect(setSpy).not.toHaveBeenCalled(); + expect(config.inspect>(PROVIDERS_SECTION).userValue).toEqual({ + acme: { type: 'openai', apiKey: 'sk-acme' }, + }); + expect(config.get>(PROVIDERS_SECTION)).toEqual({ + acme: { type: 'openai', apiKey: 'sk-acme' }, + }); + expect(config.inspect(THINKING_SECTION).userValue).toEqual({ enabled: true }); + + disposables.dispose(); + }); +}); diff --git a/packages/agent-core-v2/test/app/kosongConfig/discovery.test.ts b/packages/agent-core-v2/test/app/kosongConfig/discovery.test.ts index 7b2430ee4a..7d4d69a91e 100644 --- a/packages/agent-core-v2/test/app/kosongConfig/discovery.test.ts +++ b/packages/agent-core-v2/test/app/kosongConfig/discovery.test.ts @@ -8,12 +8,16 @@ * at them all survive; * - concurrent refreshes serialize (never overlap); * - custom-registry fetches carry the host `User-Agent`; - * - provider/model patches land in kosong's in-memory registries via - * `replaceAll` (the persistence bridge writes them back to config), - * merging discovered aliases into user-owned provider records, while - * `defaultModel` / `thinking` still go through `config.replace` directly — - * restoring a surviving default selection and CLEARing a default (plus its - * thinking) whose alias the upstream dropped, never a dangling `set`; + * - provider/model patches land in config through ONE atomic + * `replaceSections` transition (the persistence bridge then syncs them + * into the registries), merging discovered aliases into user-owned + * provider records, while `defaultModel` / `thinking` ride the same + * transition — restoring a surviving default selection and CLEARing a + * default (plus its thinking) whose alias the upstream dropped, never a + * dangling `set`; + * - the two-phase orchestrator host contract (removeProvider, then + * setConfig) never exposes a halfway-removed catalog: the registries + * stay untouched until the single atomic write; * - the `[modelCatalog]` config section self-registers and validates. */ @@ -22,6 +26,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { createScopedTestHost } from '#/_base/di/test'; import { isError2 } from '#/_base/errors/errors'; +import { ILogService, type LogPayload } from '#/_base/log/log'; import { IOAuthService } from '#/app/auth/auth'; import { IConfigService } from '#/app/config/config'; import { ConfigRegistry } from '#/app/config/configService'; @@ -29,18 +34,18 @@ import { IEventService } from '#/app/event/event'; import { IProviderDiscoveryService } from '#/app/kosongConfig/discovery'; import '#/app/kosongConfig/discoveryService'; import { MODEL_CATALOG_SECTION } from '#/app/kosongConfig/configSection'; +import { IKosongConfigService } from '#/app/kosongConfig/kosongConfig'; +import '#/app/kosongConfig/kosongConfigService'; import '#/kosong/model/errors'; import { HostRequestHeaders, IHostRequestHeaders } from '#/kosong/model/hostRequestHeaders'; import { IModelService, type ModelRecord, - type ModelsSection, } from '#/kosong/model/model'; import '#/kosong/model/modelService'; import { IProviderService, type ProviderConfig, - type ProvidersSection, } from '#/kosong/provider/provider'; import '#/kosong/provider/providerService'; import '#/kosong/provider/providers/kimi/kimi.contrib'; @@ -61,37 +66,50 @@ function stubEvents(): IEventService & { published: Array<{ type: string; payloa } as unknown as IEventService & { published: Array<{ type: string; payload: unknown }> }; } -function createHost( +function stubLogService(): ILogService { + return { + _serviceBrand: undefined, + level: 'debug', + setLevel: () => {}, + flush: async () => {}, + error: () => {}, + warn: () => {}, + info: () => {}, + debug: () => {}, + child: () => { + throw new Error('child loggers are not used by KosongConfigService'); + }, + } satisfies ILogService; +} + +async function createHost( sections: Record = {}, oauth: IOAuthService = stubOAuthService(), -): { +): Promise<{ host: ReturnType; config: StubConfigService; events: ReturnType; discovery: IProviderDiscoveryService; providers: IProviderService; models: IModelService; -} { +}> { const config = new StubConfigService(sections); const events = stubEvents(); const host = createScopedTestHost([ [IConfigService, config], [IOAuthService, oauth], [IEventService, events], + [ILogService, stubLogService()], [IHostRequestHeaders, new HostRequestHeaders({ 'User-Agent': 'kimi-test/1.0' })], ]); - // Seed the in-memory registries from the fixture sections (the persistence - // bridge that normally does this is not instantiated here). const providers = host.app.accessor.get(IProviderService); - providers.loadAll( - (sections['providers'] ?? {}) as ProvidersSection, - sections['defaultProvider'] as string | undefined, - ); const models = host.app.accessor.get(IModelService); - models.loadAll( - (sections['models'] ?? {}) as ModelsSection, - sections['defaultModel'] as string | undefined, - ); + // The real persistence bridge (DI-activated): refresh writes land in + // config, and the bridge's config → kosong sync is what carries them into + // the registries (exactly like production). Await its initialization so + // the event subscriptions are in place before the test refreshes. + const bridge = host.app.accessor.get(IKosongConfigService); + await bridge.ready; return { host, config, @@ -125,7 +143,7 @@ describe('refreshProviderModels modelSource short-circuit', () => { it('answers scoped refreshes of static providers with unchanged and no I/O', async () => { const fetchMock = vi.fn(); vi.stubGlobal('fetch', fetchMock); - const { host, discovery } = createHost(staticSections); + const { host, discovery } = await createHost(staticSections); try { const result = await discovery.refreshProviderModels({ providerId: 'static-p' }); expect(result).toEqual({ changed: [], unchanged: ['static-p'], failed: [] }); @@ -136,7 +154,7 @@ describe('refreshProviderModels modelSource short-circuit', () => { }); it('returns an empty result when nothing is refreshable', async () => { - const { host, discovery, events } = createHost(staticSections); + const { host, discovery, events } = await createHost(staticSections); try { const result = await discovery.refreshProviderModels({ scope: 'all' }); expect(result).toEqual({ changed: [], unchanged: [], failed: [] }); @@ -164,7 +182,7 @@ describe('refreshProviderModels modelSource short-circuit', () => { ); vi.stubGlobal('fetch', fetchMock); - const { host, config, discovery, events, providers, models } = createHost({ + const { host, config, discovery, events, providers, models } = await createHost({ providers: { ...staticProviders, acme: { @@ -207,7 +225,7 @@ describe('refreshProviderModels modelSource short-circuit', () => { }); it('throws provider.not_found for an unknown scoped provider', async () => { - const { host, discovery } = createHost(staticSections); + const { host, discovery } = await createHost(staticSections); try { await expect(discovery.refreshProviderModels({ providerId: 'missing' })).rejects.toSatisfy( (error) => isError2(error) && error.code === 'provider.not_found', @@ -220,7 +238,7 @@ describe('refreshProviderModels modelSource short-circuit', () => { describe('refreshProviderModels write behavior', () => { it('serializes concurrent runs so they never overlap', async () => { - const { host, discovery } = createHost( + const { host, discovery } = await createHost( { providers: { [KIMI_CODE_PROVIDER_NAME]: { @@ -287,7 +305,7 @@ describe('refreshProviderModels write behavior', () => { ); vi.stubGlobal('fetch', fetchMock); - const { host, discovery } = createHost({ + const { host, discovery } = await createHost({ providers: { acme: { type: 'openai', @@ -337,7 +355,7 @@ describe('refreshProviderModels write behavior', () => { ); vi.stubGlobal('fetch', fetchMock); - const { host, config, discovery, events, providers, models } = createHost({ + const { host, config, discovery, events, providers, models } = await createHost({ providers: { 'my-kimi': { type: 'kimi', baseUrl, apiKey: 'sk-distributed-key' }, }, @@ -397,7 +415,7 @@ describe('refreshProviderModels write behavior', () => { ); vi.stubGlobal('fetch', fetchMock); - const { host, config, discovery, models } = createHost({ + const { host, config, discovery, models } = await createHost({ providers: { 'my-kimi': { type: 'kimi', baseUrl, apiKey: 'sk-distributed-key' }, }, @@ -421,8 +439,8 @@ describe('refreshProviderModels write behavior', () => { ]); // The dropped alias was the default: an explicit undefined in the patch // must clear the section instead of leaving the default dangling. It has - // to go through `replace` — `set()`'s deepMerge would resolve undefined - // back to the stale base value. + // to go through a replacing write — `set()`'s deepMerge would resolve + // undefined back to the stale base value. expect(config.get('defaultModel')).toBeUndefined(); expect(config.get('thinking')).toBeUndefined(); const modelRecords = models.list(); @@ -432,6 +450,73 @@ describe('refreshProviderModels write behavior', () => { host.dispose(); } }); + + it('never exposes a halfway-removed catalog: the registries stay untouched until the single atomic write', async () => { + const fetchMock = vi.fn( + async () => + new Response( + JSON.stringify({ + acme: { + id: 'acme', + name: 'Acme', + api: 'https://acme.example.test/v1', + type: 'openai', + models: { m2: { id: 'm2', name: 'M2' } }, + }, + }), + { headers: { 'Content-Type': 'application/json' } }, + ), + ); + vi.stubGlobal('fetch', fetchMock); + + const { host, config, discovery, providers, models } = await createHost({ + providers: { + acme: { + type: 'openai', + apiKey: 'sk-acme', + source: { + kind: 'apiJson', + url: 'https://registry.example.test/api.json', + apiKey: 'sk-registry', + }, + }, + }, + models: { + 'acme/m1': { provider: 'acme', model: 'm1', maxContextSize: 1000 }, + }, + defaultModel: 'acme/m1', + }); + try { + // The orchestrator's host contract is two-phase (removeProvider, then + // setConfig). By the time the atomic write happens, the removal phase + // must NOT have touched the runtime registries — a reader (e.g. a + // profile bind racing the refresh) only ever sees the old catalog or + // the new one, never a half-removed one. + let seenDuringWrite: { providers: readonly string[]; models: readonly string[] } | undefined; + const originalReplaceSections = config.replaceSections.bind(config); + vi.spyOn(config, 'replaceSections').mockImplementation(async (sections) => { + seenDuringWrite = { + providers: Object.keys(providers.list()), + models: Object.keys(models.list()), + }; + await originalReplaceSections(sections); + }); + + const result = await discovery.refreshProviderModels({ scope: 'all' }); + + expect(result.failed).toEqual([]); + expect(seenDuringWrite).toEqual({ providers: ['acme'], models: ['acme/m1'] }); + expect(vi.mocked(config.replaceSections).mock.calls.length).toBe(1); + // After the write, config and the registries converge on the new state: + // the dropped alias (and its default selection) is gone everywhere. + expect(providers.list()['acme']).toBeDefined(); + expect(models.list()['acme/m2']).toBeDefined(); + expect(models.list()['acme/m1']).toBeUndefined(); + expect(config.get('defaultModel')).toBeUndefined(); + } finally { + host.dispose(); + } + }); }); describe('modelCatalog config section', () => { diff --git a/packages/agent-core-v2/test/kosong/stubs.ts b/packages/agent-core-v2/test/kosong/stubs.ts index 7ab26a4a06..50b6262c40 100644 --- a/packages/agent-core-v2/test/kosong/stubs.ts +++ b/packages/agent-core-v2/test/kosong/stubs.ts @@ -69,6 +69,19 @@ export class StubConfigService implements IConfigService { return Promise.resolve(); } + replaceSections(sections: Readonly>): Promise { + for (const [domain, value] of Object.entries(sections)) { + const previousValue = this._values.get(domain); + if (value === undefined) { + this._values.delete(domain); + } else { + this._values.set(domain, value); + } + this._onDidChange.fire({ domain, source: 'set', value, previousValue }); + } + return Promise.resolve(); + } + /** * Mutate a section WITHOUT firing the change event — simulates a config * write that bypasses the services' change events (the cache-invalidation