fix: clear config defaults via delete-capable replace writes - #1830
fix: clear config defaults via delete-capable replace writes#1830liruifengv wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: b2e2f44 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2e2f4455d
ℹ️ 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".
| // `set()` cannot delete: its deepMerge resolves an undefined patch back | ||
| // to the existing base value. `replace()` removes the section instead. |
There was a problem hiding this comment.
Remove inline comments from agent-core-v2 code
packages/agent-core-v2/AGENTS.md requires comments in this subtree to live only in the top-of-file header block and says to never place comments beside functions, methods, or statements. This newly added explanatory inline comment violates that scoped rule (the same pattern is repeated in the auth and test changes), so the diff won't meet the package's review standards; move the context to the file header if it is still needed, or delete it.
Useful? React with 👍 / 👎.
Related Issue
None — the problem is explained below. (Found while reviewing #1824.)
Problem
In the agent-core-v2 daemon, removing the default provider or logging out of the managed OAuth account is supposed to clear the dangling defaults (
default_provider, ordefault_model+thinking). The code calledIConfigService.set(section, undefined)for those clears — butset()deep-merges, and for a scalar section itsdeepMergeresolves anundefinedpatch back to the existing base value (patch ?? base), so the "cleared" value is written straight back. In production the stale default survives forever; the unit tests passed only because their config stub assignsundefineddirectly.What changed
ProviderService.delete: cleardefaultProviderviaconfig.replace()instead ofconfig.set().AuthServiceOAuth-logout cleanup: cleardefaultModelandthinkingviaconfig.replace()instead ofconfig.set().replace()is the delete-capable write path: an explicitundefinedremoves the section from the persisted config (stripEnv → undefined → delete), which is exactly the semantic these call sites need.set→replace), extended the auth test'sconfigReplacestub to the two cleared sections, and added a live-ConfigServicetest pinning the contract —set(undefined)keeps the value,replace(undefined)deletes it — so the real merge semantics are covered, not just the stub behavior.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.