diff --git a/.changeset/config-persist-guard.md b/.changeset/config-persist-guard.md new file mode 100644 index 000000000..4f0dd2d4a --- /dev/null +++ b/.changeset/config-persist-guard.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix config.toml entries being lost when the file had a syntax error or was edited outside the app. diff --git a/packages/agent-core-v2/src/app/config/configService.ts b/packages/agent-core-v2/src/app/config/configService.ts index d71e79c2d..83c8e80c1 100644 --- a/packages/agent-core-v2/src/app/config/configService.ts +++ b/packages/agent-core-v2/src/app/config/configService.ts @@ -3,7 +3,7 @@ import { Disposable } from '#/_base/di/lifecycle'; import { LifecycleScope } from '#/app/scopes'; import { ScopeActivation, registerScopedService } from '#/_base/di/scope'; import { Emitter, type Event } from '#/_base/event'; -import { BugIndicatingError, onUnexpectedError } from '#/errors'; +import { BugIndicatingError, Error2, ErrorCodes, onUnexpectedError } from '#/errors'; import { IBootstrapService } from '#/app/bootstrap/bootstrap'; import { ILogService } from '#/_base/log/log'; import { @@ -309,6 +309,7 @@ export class ConfigService extends Disposable implements IConfigService { private readonly diagnosticsList: ConfigDiagnostic[] = []; private lastDiagnosticsSnapshot = '[]'; private readonly configKey: string; + private tainted = false; constructor( @IConfigRegistry private readonly registry: IConfigRegistry, @@ -402,17 +403,18 @@ export class ConfigService extends Disposable implements IConfigService { return; } await this.enqueueStateTransition(async () => { - const base = this.raw[domain]; - const next = this.registry.merge(domain, base, patch); - const validated = this.registry.validate(domain, next); - const stripped = this.stripEnv(domain, validated); - if (stripped === undefined) { - delete this.raw[domain]; - } else { - this.registry.validate(domain, stripped); - this.raw[domain] = stripped; - } - await this.persist(domain); + this.assertPersistable(); + await this.persist(domain, (stagedRaw, stagedRawSnake) => { + const next = this.registry.merge(domain, stagedRaw[domain], patch); + const validated = this.registry.validate(domain, next); + const stripped = this.stripEnv(domain, validated, stagedRaw, stagedRawSnake); + if (stripped === undefined) { + delete stagedRaw[domain]; + } else { + this.registry.validate(domain, stripped); + stagedRaw[domain] = stripped; + } + }); this.rebuildEffective('set', [domain]); }); } @@ -434,13 +436,15 @@ export class ConfigService extends Disposable implements IConfigService { return; } await this.enqueueStateTransition(async () => { - const stripped = this.stripEnv(domain, effectiveValue); - if (stripped === undefined) { - delete this.raw[domain]; - } else { - this.raw[domain] = this.registry.validate(domain, stripped); - } - await this.persist(domain); + this.assertPersistable(); + await this.persist(domain, (stagedRaw, stagedRawSnake) => { + const stripped = this.stripEnv(domain, effectiveValue, stagedRaw, stagedRawSnake); + if (stripped === undefined) { + delete stagedRaw[domain]; + } else { + stagedRaw[domain] = this.registry.validate(domain, stripped); + } + }); this.rebuildEffective('set', [domain]); }); } @@ -467,33 +471,38 @@ export class ConfigService extends Disposable implements IConfigService { return; } await this.enqueueStateTransition(async () => { - const staged: ResolvedConfig = { ...this.raw }; - for (const domain of domains) { - const value = sections[domain] === null ? undefined : sections[domain]; - const stripped = this.stripEnv(domain, value); - if (stripped === undefined) { - delete staged[domain]; - } else { - staged[domain] = this.registry.validate(domain, stripped); + this.assertPersistable(); + await this.persistDomains(domains, (stagedRaw, stagedRawSnake) => { + for (const domain of domains) { + const value = sections[domain] === null ? undefined : sections[domain]; + const stripped = this.stripEnv(domain, value, stagedRaw, stagedRawSnake); + if (stripped === undefined) { + delete stagedRaw[domain]; + } else { + stagedRaw[domain] = this.registry.validate(domain, stripped); + } } - } - this.raw = staged; - await this.persistDomains(domains); + }); this.rebuildEffective('set', domains); }); } - private stripEnv(domain: string, value: unknown): unknown { + private stripEnv( + domain: string, + value: unknown, + raw: ResolvedConfig, + rawSnake: ResolvedConfig, + ): unknown { let result = value; const section = this.registry.getSection(domain); if (section?.stripEnv !== undefined) { const getEnv = (name: string): string | undefined => this.bootstrap.getEnv(name); - result = section.stripEnv(result, this.raw[domain], getEnv); + result = section.stripEnv(result, raw[domain], getEnv); } if (result === undefined) return result; for (const overlay of this.registry.listEffectiveOverlays()) { if (overlay.strip === undefined) continue; - result = overlay.strip(domain, result, this.rawSnake); + result = overlay.strip(domain, result, rawSnake); if (result === undefined) return result; } return result; @@ -516,17 +525,25 @@ export class ConfigService extends Disposable implements IConfigService { private async load(source: ConfigChangeSource): Promise { this.diagnosticsList.length = 0; let fileData: ResolvedConfig = {}; + let failed = false; try { const data = await this.documentStore.get(CONFIG_SCOPE, this.configKey); fileData = data !== undefined && isPlainObject(data) ? data : {}; } catch (error) { + failed = true; const message = error instanceof TomlError ? `Failed to parse ${this.bootstrap.configPath}: ${describeTomlSyntaxError(error)}` : describeUnknownError(error); this.pushDiagnostic({ severity: 'error', message }); this.log.warn('config load failed', { error: describeUnknownError(error) }); + if (source !== 'load') { + this.tainted = true; + this.emitDiagnosticsIfChanged(); + return; + } } + this.tainted = failed; const nextRawSnake = cloneRecord(fileData); for (const diagnostic of collectKeyDeprecations(nextRawSnake, this.registry.listSections())) { this.pushDiagnostic(diagnostic); @@ -732,15 +749,56 @@ export class ConfigService extends Disposable implements IConfigService { this.commit('reload', [domain]); } - private async persist(domain: string): Promise { - await this.persistDomains([domain]); + private assertPersistable(): void { + if (!this.tainted) return; + throw new Error2( + ErrorCodes.CONFIG_PERSIST_BLOCKED, + `Refusing to persist config: ${this.bootstrap.configPath} could not be read; fix the file and reload before writing.`, + ); } - private async persistDomains(domains: readonly string[]): Promise { - for (const domain of domains) { - applySectionToToml(this.rawSnake, domain, this.raw[domain], this.registry); + private async persist( + domain: string, + rebase: (stagedRaw: ResolvedConfig, stagedRawSnake: ResolvedConfig) => void, + ): Promise { + await this.persistDomains([domain], rebase); + } + + private async persistDomains( + domains: readonly string[], + rebase: (stagedRaw: ResolvedConfig, stagedRawSnake: ResolvedConfig) => void, + ): Promise { + this.assertPersistable(); + let onDisk: ResolvedConfig = {}; + try { + const data = await this.documentStore.get(CONFIG_SCOPE, this.configKey); + onDisk = data !== undefined && isPlainObject(data) ? data : {}; + } catch (error) { + const message = + error instanceof TomlError + ? `Failed to parse ${this.bootstrap.configPath}: ${describeTomlSyntaxError(error)}` + : describeUnknownError(error); + this.pushDiagnostic({ severity: 'error', message }); + this.emitDiagnosticsIfChanged(); + this.log.warn('config persist aborted: re-read failed', { + error: describeUnknownError(error), + }); + this.tainted = true; + throw new Error2( + ErrorCodes.CONFIG_PERSIST_BLOCKED, + `Refusing to persist config: ${this.bootstrap.configPath} could not be read; fix the file and reload before writing.`, + { cause: error }, + ); } - await this.documentStore.set(CONFIG_SCOPE, this.configKey, this.rawSnake); + const stagedRawSnake = cloneRecord(onDisk); + const stagedRaw = transformTomlData(onDisk, this.registry); + rebase(stagedRaw, stagedRawSnake); + for (const domain of domains) { + applySectionToToml(stagedRawSnake, domain, stagedRaw[domain], this.registry); + } + await this.documentStore.set(CONFIG_SCOPE, this.configKey, stagedRawSnake); + this.rawSnake = stagedRawSnake; + this.raw = stagedRaw; } } diff --git a/packages/agent-core-v2/src/app/config/errors.ts b/packages/agent-core-v2/src/app/config/errors.ts index f0dd785f8..63c01cb81 100644 --- a/packages/agent-core-v2/src/app/config/errors.ts +++ b/packages/agent-core-v2/src/app/config/errors.ts @@ -4,6 +4,7 @@ import { CONFIG_INVALID_ERROR_CODE } from '#/kosong/contract/errors'; export const ConfigErrors = { codes: { CONFIG_INVALID: CONFIG_INVALID_ERROR_CODE, + CONFIG_PERSIST_BLOCKED: 'config.persist_blocked', }, } as const satisfies ErrorDomain; 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 20bd542f3..3271f44d6 100644 --- a/packages/agent-core-v2/test/app/config/config.test.ts +++ b/packages/agent-core-v2/test/app/config/config.test.ts @@ -24,6 +24,7 @@ import { createDecorator, type ProvideHandle } from '#/_base/di/instantiation'; import { DisposableStore } from '#/_base/di/lifecycle'; import { Service } from '#/_base/di/service'; import { TestInstantiationService } from '#/_base/di/test'; +import { Event } from '#/_base/event'; import { IBootstrapService } from '#/app/bootstrap/bootstrap'; import { type ConfigSchema, @@ -34,8 +35,7 @@ import { } from '#/app/config/config'; import { ConfigRegistry, ConfigService } from '#/app/config/configService'; import { ConfigSectionContribution } from '#/app/config/configSectionContributions'; -import '#/app/cron/configSection'; -import type { CronConfig } from '#/app/cron/configSection'; +import { CRON_SECTION, DEFAULT_CRON_CONFIG, type CronConfig } from '#/app/cron/configSection'; import '#/app/skillCatalog/configSection'; import { BUILTIN_PRODUCT_SKILLS_SECTION } from '#/app/skillCatalog/configSection'; import { @@ -66,6 +66,7 @@ import { PROVIDERS_SECTION, THINKING_SECTION, } from '#/app/kosongConfig/configSection'; +import '#/app/kosongConfig/envOverlay'; import { type ThinkingConfig } from '#/kosong/model/thinking'; import { KEEP_ALIVE_ON_EXIT_ENV, @@ -2596,3 +2597,196 @@ describe('ConfigService replaceSections', () => { disposables.dispose(); }); }); + +describe('ConfigService persistence guards', () => { + class SilentStorage extends InMemoryStorageService { + override watch(): Event { + return Event.None as Event; + } + } + + async function createGuardedConfig(toml: string, env: NodeJS.ProcessEnv = {}) { + const disposables = new DisposableStore(); + const ix = disposables.add(new TestInstantiationService()); + const storage = new SilentStorage(); + await storage.write('', 'config.toml', new TextEncoder().encode(toml)); + ix.stub(ILogService, stubLog()); + ix.stub(IBootstrapService, stubBootstrap('/tmp/kimi-cfg-guards', env)); + 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; + return { config, disposables, storage }; + } + + async function overwrite(storage: InMemoryStorageService, toml: string): Promise { + await storage.write('', 'config.toml', new TextEncoder().encode(toml)); + } + + async function stored(storage: InMemoryStorageService): Promise { + const bytes = await storage.read('', 'config.toml'); + return new TextDecoder().decode(bytes); + } + + async function expectPersistBlocked(promise: Promise): Promise { + const error = await promise.then( + () => undefined, + (e: unknown) => e, + ); + expect(isError2(error)).toBe(true); + expect((error as Error2).code).toBe(ErrorCodes.CONFIG_PERSIST_BLOCKED); + } + + it('refuses to persist when the initial load fails and keeps the file untouched', async () => { + const broken = '[providers\nbroken'; + const { config, disposables, storage } = await createGuardedConfig(broken); + + expect(config.diagnostics().some((d) => d.severity === 'error')).toBe(true); + expect(config.get(PROVIDERS_SECTION)).toEqual({}); + expect(config.get(CRON_SECTION)).toEqual(DEFAULT_CRON_CONFIG); + + await expectPersistBlocked(config.set(THINKING_SECTION, { enabled: true })); + await expectPersistBlocked(config.replace(THINKING_SECTION, { enabled: true })); + await expectPersistBlocked(config.replaceSections({ [THINKING_SECTION]: { enabled: true } })); + + expect(await stored(storage)).toBe(broken); + + await config.set(THINKING_SECTION, { enabled: true }, ConfigTarget.Memory); + expect(config.get(THINKING_SECTION)).toEqual({ enabled: true }); + + disposables.dispose(); + }); + + it('keeps last-known-good values when a reload hits a broken file, and recovers after the file is fixed', async () => { + const { config, disposables, storage } = await createGuardedConfig( + '[providers.acme]\ntype = "openai"\napi_key = "sk-acme"\n', + ); + expect(config.get>(PROVIDERS_SECTION)).toEqual({ + acme: { type: 'openai', apiKey: 'sk-acme' }, + }); + + await overwrite(storage, '= broken ='); + await config.reload(); + + expect(config.get>(PROVIDERS_SECTION)).toEqual({ + acme: { type: 'openai', apiKey: 'sk-acme' }, + }); + await expectPersistBlocked(config.set(THINKING_SECTION, { enabled: true })); + expect(await stored(storage)).toBe('= broken ='); + + await overwrite(storage, '[providers.beta]\ntype = "openai"\napi_key = "sk-beta"\n'); + await config.reload(); + + expect(config.get>(PROVIDERS_SECTION)).toEqual({ + beta: { type: 'openai', apiKey: 'sk-beta' }, + }); + await config.set(THINKING_SECTION, { enabled: true }); + expect(config.get(THINKING_SECTION)).toEqual({ enabled: true }); + + disposables.dispose(); + }); + + it('merges external edits observed at persist time instead of clobbering them', async () => { + const { config, disposables, storage } = await createGuardedConfig( + 'default_model = "acme/m1"\n\n[providers.acme]\ntype = "openai"\napi_key = "sk-acme"\n', + ); + + await overwrite( + storage, + 'default_model = "acme/m1"\n\n[providers.acme]\ntype = "openai"\napi_key = "sk-acme-2"\n\n[providers.beta]\ntype = "openai"\napi_key = "sk-beta"\n', + ); + + const changed: string[] = []; + config.onDidSectionChange((e) => changed.push(e.domain)); + await config.set(THINKING_SECTION, { enabled: true }); + + const doc = await stored(storage); + expect(doc).toContain('sk-acme-2'); + expect(doc).toContain('[providers.beta]'); + expect(doc).toContain('[thinking]'); + expect(config.get>(PROVIDERS_SECTION)).toEqual({ + acme: { type: 'openai', apiKey: 'sk-acme-2' }, + beta: { type: 'openai', apiKey: 'sk-beta' }, + }); + expect(config.get(THINKING_SECTION)).toEqual({ enabled: true }); + expect(changed).toContain(PROVIDERS_SECTION); + expect(changed).toContain(THINKING_SECTION); + + disposables.dispose(); + }); + + it('honors an external delete instead of resurrecting the in-memory copy', async () => { + const { config, disposables, storage } = await createGuardedConfig( + '[providers.acme]\ntype = "openai"\napi_key = "sk-acme"\n', + ); + + await storage.delete('', 'config.toml'); + await config.set(THINKING_SECTION, { enabled: true }); + + const doc = await stored(storage); + expect(doc).toContain('[thinking]'); + expect(doc).not.toContain('[providers.acme]'); + expect(config.inspect(PROVIDERS_SECTION).userValue).toBeUndefined(); + + disposables.dispose(); + }); + + it('rebases a set() merge onto external edits of the same section', async () => { + const { config, disposables, storage } = await createGuardedConfig( + '[providers.acme]\ntype = "openai"\napi_key = "sk-acme"\n', + ); + + await overwrite( + storage, + '[providers.acme]\ntype = "openai"\napi_key = "sk-acme"\n\n[providers.beta]\ntype = "openai"\napi_key = "sk-beta"\n', + ); + await config.set(PROVIDERS_SECTION, { gamma: { type: 'openai', apiKey: 'sk-gamma' } }); + + const doc = await stored(storage); + expect(doc).toContain('[providers.beta]'); + expect(doc).toContain('[providers.gamma]'); + expect(config.get>(PROVIDERS_SECTION)).toEqual({ + acme: { type: 'openai', apiKey: 'sk-acme' }, + beta: { type: 'openai', apiKey: 'sk-beta' }, + gamma: { type: 'openai', apiKey: 'sk-gamma' }, + }); + + disposables.dispose(); + }); + + it('restores env-masked values from the freshly re-read file instead of the stale snapshot', async () => { + const { config, disposables, storage } = await createGuardedConfig( + 'default_model = "acme/m1"\n\n[providers.acme]\ntype = "openai"\napi_key = "sk-acme"\n\n[models."acme/m1"]\nprovider = "acme"\nmodel = "m1"\n', + { KIMI_MODEL_NAME: 'env-model' }, + ); + expect(config.get(DEFAULT_MODEL_SECTION)).toBe('__kimi_env_model__'); + + await overwrite( + storage, + 'default_model = "acme/m2"\n\n[providers.acme]\ntype = "openai"\napi_key = "sk-acme"\n\n[models."acme/m2"]\nprovider = "acme"\nmodel = "m2"\n', + ); + await config.replace(DEFAULT_MODEL_SECTION, config.get(DEFAULT_MODEL_SECTION)); + + const doc = await stored(storage); + expect(doc).toContain('default_model = "acme/m2"'); + expect(doc).not.toContain('default_model = "acme/m1"'); + + disposables.dispose(); + }); + + it('keeps the in-memory snapshots untouched when a write fails validation', async () => { + const { config, disposables, storage } = await createGuardedConfig( + '[thinking]\nenabled = true\n', + ); + + await overwrite(storage, '[thinking]\nenabled = false\n'); + await expect(config.set(THINKING_SECTION, { enabled: 'yes' })).rejects.toThrow(); + + expect(config.inspect(THINKING_SECTION).userValue).toEqual({ enabled: true }); + expect(await stored(storage)).toBe('[thinking]\nenabled = false\n'); + + disposables.dispose(); + }); +}); diff --git a/packages/protocol/src/events.ts b/packages/protocol/src/events.ts index 57febf105..78cf6a3bc 100644 --- a/packages/protocol/src/events.ts +++ b/packages/protocol/src/events.ts @@ -229,6 +229,7 @@ export interface GoalChange { export type KimiErrorCode = | 'config.invalid' + | 'config.persist_blocked' | 'session.not_found' | 'session.already_exists' | 'session.id_invalid' @@ -1259,6 +1260,7 @@ export const goalChangeSchema = z.object({ export const kimiErrorCodeSchema = z.enum([ 'config.invalid', + 'config.persist_blocked', 'session.not_found', 'session.already_exists', 'session.id_invalid',