fix(agent-core-v2): write refresh results in one atomic config transition - #2410
Conversation
…tion - add IConfigService.replaceSections: applies several domains in a single state transition — one disk write, one effective rebuild, change events fire only after all domains took effect - rework ProviderDiscoveryService to absorb the orchestrator's two-phase removeProvider/setConfig host contract into one replaceSections write, so the kosong registries never pass through a halfway-removed catalog - stop writing the env-synthesized __kimi_env__ slice to config; the bridge's event-driven sync carries it into the registries on its own - fixes sporadic "model is not configured" errors when starting kimi web, caused by the background refresh transiently clearing the model catalog while the first session was being created
🦋 Changeset detectedLatest commit: 8fe7883 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fa85e8bbd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (stripped === undefined) { | ||
| delete this.raw[domain]; | ||
| } else { | ||
| this.raw[domain] = this.registry.validate(domain, stripped); |
There was a problem hiding this comment.
When a multi-section write has a valid earlier domain followed by a later domain that fails stripEnv/validation, this assignment has already changed this.raw, but the method exits before persistDomains() and rebuildEffective(). Callers see a rejected atomic write while inspect() and future merges now use a partially applied user layer; stage all stripped/validated values in a temporary raw copy first, then swap it in only after the whole batch succeeds.
Useful? React with 👍 / 👎.
| /** | ||
| * 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 |
There was a problem hiding this comment.
This adds method-level documentation below the top-of-file block, but the agent-core-v2 guide requires comments to live solely in the header and not beside functions or methods. Please move the replaceSections contract details into the file header or another approved reference so this patch does not add a new exception to the local convention.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L11-L15
Useful? React with 👍 / 👎.
…config Validate and strip every domain into a staged copy of the raw/memory layer first, then swap it in only after the whole batch succeeds — previously a later domain failing validation left earlier domains already applied to this.raw/this.memory while the call reported failure, exposing a partially applied user layer to inspect() and future merges.
Conflict resolutions (6 files): - agent-core-v2/docs/state-manifest.d.ts: took upstream, regenerated via gen:state-manifest. - agent-core/test/harness/runtime.test.ts: kept all 3 added tests (fork stale plugin_session_start disabled-skill case + 2 upstream plugin agent layer cases). - agent-core-v2/test/agent/loop/loop.test.ts + test/tool/tool.test.ts: tools-hash inline snapshots collided (fork Agent tool exact-alias model param vs upstream MoonshotAI#2379 non-blocking TaskOutput contract); kept fork side, regenerated snapshots from merged behavior — exact-alias model contract preserved. - agent-core-v2/test/app/config/config.test.ts: kept both import additions (fork PERSIST_DEFAULT_MODEL_SECTION + upstream PROVIDERS_SECTION, atomic config refresh MoonshotAI#2410). - kap-server/test/skills.test.ts: restartWithDisabledSkills helper now passes TEST_HOST_IDENTITY (upstream made StartServerOptions.hostIdentity required). Verification: - agent-core-v2: 4265/4265 pass; gen:state-manifest --check clean. - kap-server: 927/927 pass. - agent-core: 4147 pass; 4 failures in test/harness/coder-subagent-tools.test.ts are pre-existing spawn /bin/bash ENOENT on NixOS (not merge-related). - kimi-code TUI: 2487 pass. typecheck: agent-core, agent-core-v2, kap-server clean. - lint: 2 errors in packages/oauth/src/managed-userinfo.ts are pre-existing on upstream (no-redundant-type-constituents, MoonshotAI#2363), unchanged by this merge. Closes the sync tracked in #23.
…configService.ts
…onfig/discoveryService.ts
…Config/discovery.test.ts
1 participant
Related Issue
No linked issue — the problem is explained below.
Problem
Starting kimi web (or creating the first session) could sporadically fail with "model is not configured" errors. Root cause: the background provider-model refresh wrote its results through the orchestrator's two-phase host contract (
removeProvider, thensetConfig), and each phase applied directly to kosong's in-memory registries plus separate config writes. Between the phases the model catalog was transiently empty/partially removed, so a session created concurrently (e.g. while the profile was binding its model) observed a halfway-removed catalog and failed.What changed
1.
IConfigService.replaceSections— atomic multi-domain config writesProblem:
ConfigServicecould only replace one domain per transition, so a multi-section update (providers + models + defaultModel + thinking) produced several intermediate effective states, each visible to readers and each firing its own change events.What was done:
replaceSections(sections, target?): updates every domain's raw value first, persists the document with a single disk write (persistDomains), and rebuilds the effective view once — change events fire only after all domains have taken effect, so no reader can observe a half-applied multi-section write. A domain mapped toundefinedis cleared, same asreplace(domain, undefined).persist(domain)now delegates topersistDomains([domain]).2.
ProviderDiscoveryService— single atomic refresh writeProblem: The two-phase host contract removed a provider from the runtime registries before the new shape landed, which is exactly the transiently-cleared catalog behind the startup race.
What was done:
removeProviderno longer touches the registries:shapeWithoutProvidercomputes the removal in memory only, because the host's patch always carries the FULL providers/models records under replace semantics — the removal is already expressed by the patch itself.applyRefreshPatchnow lands all four sections (providers, models, defaultModel, thinking) in ONEreplaceSectionstransition; the bridge's synchronous event sync pushes the records into the kosong registries atomically.__kimi_env__slice is no longer written to config at all (it lives in the effective overlay; the bridge carries it into the registries on its own), and the previous "preserve synthetic entries across registry replace" logic was dropped together with the directreplaceAllwrites.IModelServiceis no longer a dependency of the service.config.test.tscoversreplaceSections(atomic persist, clear-on-undefined, memory target, event timing),discovery.test.tscovers the single-transition refresh behavior.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.