feat: config aliases#18140
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97efe279c3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # codex-rs/core/src/stream_events_utils.rs # codex-rs/core/src/stream_events_utils_tests.rs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/core/src/config/service.rs
Line 690 in 2f1b1f4
compute_override_metadata compares segments literally against effective. After this change, effective config canonicalizes memories.no_memories_if_mcp_or_web_search to memories.disable_on_external_context, so writes using the legacy key can be misreported as overridden with effective_value = null even when no higher layer overrides them.
codex/codex-rs/core/src/config/service.rs
Line 345 in 2f1b1f4
User-layer writes validate user_config directly, but this value is not alias-normalized first. If a user file contains both legacy and canonical memory keys, writes to unrelated paths fail with config validation errors despite merge logic intending to canonicalize aliases. This blocks configuration updates for otherwise loadable configs.
codex/codex-rs/core/src/config/service.rs
Line 690 in 2f1b1f4
Override detection uses the raw edited key path against effective, which is now canonicalized. Editing via legacy path can produce effective_value = null and mark the write as overridden even when no higher-precedence layer overrides it. This yields incorrect status/metadata in write responses.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /// Merge config `overlay` into `base`, giving `overlay` precedence. | ||
| pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { | ||
| merge_toml_values_at_path(base, overlay, &mut Vec::new()); |
There was a problem hiding this comment.
Preserve merge path when normalizing aliased keys
merge_toml_values always initializes with an empty path, so alias normalization only runs correctly when merging from config root. Subtree merges (e.g. upserting memories in config write APIs) skip alias rewriting, allowing legacy and canonical keys to coexist and later fail typed deserialization as duplicate fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
all of this is the same comment... not applicable here
Rename
no_memories_if_mcp_or_web_search→disable_on_external_contextwith backward compatibilityWhile doing so, we add a key alias system on our layer merging system. What we try to avoid is a case where a company managed config use an old name while the user has a new name in it's local config (which would make the deserialization fail)