mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat: config aliases (#18140)
Rename `no_memories_if_mcp_or_web_search` → `disable_on_external_context` with backward compatibility While 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)
This commit is contained in:
committed by
GitHub
Unverified
parent
af7b8d551c
commit
cfc23eee3d
@@ -0,0 +1,52 @@
|
||||
use toml::Value as TomlValue;
|
||||
use toml::map::Map as TomlMap;
|
||||
|
||||
#[derive(Debug, Clone, Copy)]
|
||||
struct ConfigKeyAlias {
|
||||
table_path: &'static [&'static str],
|
||||
legacy_key: &'static str,
|
||||
canonical_key: &'static str,
|
||||
}
|
||||
|
||||
const CONFIG_KEY_ALIASES: &[ConfigKeyAlias] = &[ConfigKeyAlias {
|
||||
table_path: &["memories"],
|
||||
legacy_key: "no_memories_if_mcp_or_web_search",
|
||||
canonical_key: "disable_on_external_context",
|
||||
}];
|
||||
|
||||
pub(crate) fn normalize_key_aliases(path: &[String], table: &mut TomlMap<String, TomlValue>) {
|
||||
for alias in CONFIG_KEY_ALIASES {
|
||||
if path
|
||||
.iter()
|
||||
.map(String::as_str)
|
||||
.eq(alias.table_path.iter().copied())
|
||||
&& let Some(value) = table.remove(alias.legacy_key)
|
||||
{
|
||||
table
|
||||
.entry(alias.canonical_key.to_string())
|
||||
.or_insert(value);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn normalized_with_key_aliases(value: &TomlValue, path: &[String]) -> TomlValue {
|
||||
match value {
|
||||
TomlValue::Table(table) => {
|
||||
let mut normalized = TomlMap::new();
|
||||
for (key, child) in table {
|
||||
let mut child_path = path.to_vec();
|
||||
child_path.push(key.clone());
|
||||
normalized.insert(key.clone(), normalized_with_key_aliases(child, &child_path));
|
||||
}
|
||||
normalize_key_aliases(path, &mut normalized);
|
||||
TomlValue::Table(normalized)
|
||||
}
|
||||
TomlValue::Array(items) => TomlValue::Array(
|
||||
items
|
||||
.iter()
|
||||
.map(|item| normalized_with_key_aliases(item, path))
|
||||
.collect(),
|
||||
),
|
||||
_ => value.clone(),
|
||||
}
|
||||
}
|
||||
@@ -4,6 +4,7 @@ pub mod config_toml;
|
||||
mod constraint;
|
||||
mod diagnostics;
|
||||
mod fingerprint;
|
||||
mod key_aliases;
|
||||
mod marketplace_edit;
|
||||
mod mcp_edit;
|
||||
mod mcp_types;
|
||||
|
||||
@@ -1,18 +1,34 @@
|
||||
use crate::key_aliases::normalize_key_aliases;
|
||||
use crate::key_aliases::normalized_with_key_aliases;
|
||||
use toml::Value as TomlValue;
|
||||
|
||||
/// 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());
|
||||
}
|
||||
|
||||
fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec<String>) {
|
||||
if let TomlValue::Table(overlay_table) = overlay
|
||||
&& let TomlValue::Table(base_table) = base
|
||||
{
|
||||
normalize_key_aliases(path, base_table);
|
||||
let mut overlay_table = overlay_table.clone();
|
||||
normalize_key_aliases(path, &mut overlay_table);
|
||||
|
||||
for (key, value) in overlay_table {
|
||||
if let Some(existing) = base_table.get_mut(key) {
|
||||
merge_toml_values(existing, value);
|
||||
path.push(key.clone());
|
||||
if let Some(existing) = base_table.get_mut(&key) {
|
||||
merge_toml_values_at_path(existing, &value, path);
|
||||
} else {
|
||||
base_table.insert(key.clone(), value.clone());
|
||||
base_table.insert(key, normalized_with_key_aliases(&value, path));
|
||||
}
|
||||
path.pop();
|
||||
}
|
||||
} else {
|
||||
*base = overlay.clone();
|
||||
*base = normalized_with_key_aliases(overlay, path);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "merge_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,100 @@
|
||||
use super::*;
|
||||
use crate::config_toml::ConfigToml;
|
||||
use crate::types::MemoriesToml;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn parse_toml(value: &str) -> TomlValue {
|
||||
toml::from_str(value).expect("TOML should parse")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_toml_values_normalizes_legacy_key_from_base_layer() {
|
||||
let mut base = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
no_memories_if_mcp_or_web_search = false
|
||||
"#,
|
||||
);
|
||||
let overlay = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
disable_on_external_context = true
|
||||
"#,
|
||||
);
|
||||
|
||||
merge_toml_values(&mut base, &overlay);
|
||||
|
||||
let expected = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
disable_on_external_context = true
|
||||
"#,
|
||||
);
|
||||
assert_eq!(base, expected);
|
||||
|
||||
let config: ConfigToml = base.try_into().expect("merged config should deserialize");
|
||||
assert_eq!(
|
||||
config.memories,
|
||||
Some(MemoriesToml {
|
||||
disable_on_external_context: Some(true),
|
||||
..Default::default()
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_toml_values_normalizes_legacy_key_from_overlay_layer() {
|
||||
let mut base = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
disable_on_external_context = false
|
||||
"#,
|
||||
);
|
||||
let overlay = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
no_memories_if_mcp_or_web_search = true
|
||||
"#,
|
||||
);
|
||||
|
||||
merge_toml_values(&mut base, &overlay);
|
||||
|
||||
let expected = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
disable_on_external_context = true
|
||||
"#,
|
||||
);
|
||||
assert_eq!(base, expected);
|
||||
|
||||
let config: ConfigToml = base.try_into().expect("merged config should deserialize");
|
||||
assert_eq!(
|
||||
config.memories,
|
||||
Some(MemoriesToml {
|
||||
disable_on_external_context: Some(true),
|
||||
..Default::default()
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_toml_values_prefers_canonical_key_when_one_layer_has_both_names() {
|
||||
let mut base = TomlValue::Table(toml::map::Map::new());
|
||||
let overlay = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
disable_on_external_context = true
|
||||
no_memories_if_mcp_or_web_search = false
|
||||
"#,
|
||||
);
|
||||
|
||||
merge_toml_values(&mut base, &overlay);
|
||||
|
||||
let expected = parse_toml(
|
||||
r#"
|
||||
[memories]
|
||||
disable_on_external_context = true
|
||||
"#,
|
||||
);
|
||||
assert_eq!(base, expected);
|
||||
}
|
||||
@@ -3,6 +3,7 @@ use crate::config_requirements::ConfigRequirementsToml;
|
||||
|
||||
use super::fingerprint::record_origins;
|
||||
use super::fingerprint::version_for_toml;
|
||||
use super::key_aliases::normalized_with_key_aliases;
|
||||
use super::merge::merge_toml_values;
|
||||
use codex_app_server_protocol::ConfigLayer;
|
||||
use codex_app_server_protocol::ConfigLayerMetadata;
|
||||
@@ -262,7 +263,8 @@ impl ConfigLayerStack {
|
||||
ConfigLayerStackOrdering::LowestPrecedenceFirst,
|
||||
/*include_disabled*/ false,
|
||||
) {
|
||||
record_origins(&layer.config, &layer.metadata(), &mut path, &mut origins);
|
||||
let config = normalized_with_key_aliases(&layer.config, &[]);
|
||||
record_origins(&config, &layer.metadata(), &mut path, &mut origins);
|
||||
}
|
||||
|
||||
origins
|
||||
@@ -354,3 +356,7 @@ fn verify_layer_ordering(layers: &[ConfigLayerEntry]) -> std::io::Result<Option<
|
||||
|
||||
Ok(user_layer_index)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "state_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,34 @@
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn origins_use_canonical_key_aliases() {
|
||||
let layer = ConfigLayerEntry::new(
|
||||
ConfigLayerSource::SessionFlags,
|
||||
toml::from_str(
|
||||
r#"
|
||||
[memories]
|
||||
no_memories_if_mcp_or_web_search = true
|
||||
"#,
|
||||
)
|
||||
.expect("config TOML should parse"),
|
||||
);
|
||||
let metadata = layer.metadata();
|
||||
let stack = ConfigLayerStack::new(
|
||||
vec![layer],
|
||||
ConfigRequirements::default(),
|
||||
ConfigRequirementsToml::default(),
|
||||
)
|
||||
.expect("single layer stack should be valid");
|
||||
|
||||
let origins = stack.origins();
|
||||
|
||||
assert_eq!(
|
||||
origins.get("memories.disable_on_external_context"),
|
||||
Some(&metadata)
|
||||
);
|
||||
assert!(
|
||||
!origins.contains_key("memories.no_memories_if_mcp_or_web_search"),
|
||||
"legacy key should be canonicalized before origin recording"
|
||||
);
|
||||
}
|
||||
@@ -182,8 +182,9 @@ pub struct ToolSuggestConfig {
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct MemoriesToml {
|
||||
/// When `true`, web searches and MCP tool calls mark the thread `memory_mode` as `"polluted"`.
|
||||
pub no_memories_if_mcp_or_web_search: Option<bool>,
|
||||
/// When `true`, external context sources mark the thread `memory_mode` as `"polluted"`.
|
||||
#[serde(alias = "no_memories_if_mcp_or_web_search")]
|
||||
pub disable_on_external_context: Option<bool>,
|
||||
/// When `false`, newly created threads are stored with `memory_mode = "disabled"` in the state DB.
|
||||
pub generate_memories: Option<bool>,
|
||||
/// When `false`, skip injecting memory usage instructions into developer prompts.
|
||||
@@ -209,7 +210,7 @@ pub struct MemoriesToml {
|
||||
/// Effective memories settings after defaults are applied.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct MemoriesConfig {
|
||||
pub no_memories_if_mcp_or_web_search: bool,
|
||||
pub disable_on_external_context: bool,
|
||||
pub generate_memories: bool,
|
||||
pub use_memories: bool,
|
||||
pub max_raw_memories_for_consolidation: usize,
|
||||
@@ -224,7 +225,7 @@ pub struct MemoriesConfig {
|
||||
impl Default for MemoriesConfig {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
no_memories_if_mcp_or_web_search: false,
|
||||
disable_on_external_context: false,
|
||||
generate_memories: true,
|
||||
use_memories: true,
|
||||
max_raw_memories_for_consolidation: DEFAULT_MEMORIES_MAX_RAW_MEMORIES_FOR_CONSOLIDATION,
|
||||
@@ -242,9 +243,9 @@ impl From<MemoriesToml> for MemoriesConfig {
|
||||
fn from(toml: MemoriesToml) -> Self {
|
||||
let defaults = Self::default();
|
||||
Self {
|
||||
no_memories_if_mcp_or_web_search: toml
|
||||
.no_memories_if_mcp_or_web_search
|
||||
.unwrap_or(defaults.no_memories_if_mcp_or_web_search),
|
||||
disable_on_external_context: toml
|
||||
.disable_on_external_context
|
||||
.unwrap_or(defaults.disable_on_external_context),
|
||||
generate_memories: toml.generate_memories.unwrap_or(defaults.generate_memories),
|
||||
use_memories: toml.use_memories.unwrap_or(defaults.use_memories),
|
||||
max_raw_memories_for_consolidation: toml
|
||||
|
||||
@@ -863,6 +863,10 @@
|
||||
"description": "Model used for memory consolidation.",
|
||||
"type": "string"
|
||||
},
|
||||
"disable_on_external_context": {
|
||||
"description": "When `true`, external context sources mark the thread `memory_mode` as `\"polluted\"`.",
|
||||
"type": "boolean"
|
||||
},
|
||||
"extract_model": {
|
||||
"description": "Model used for thread summarisation.",
|
||||
"type": "string"
|
||||
@@ -900,10 +904,6 @@
|
||||
"format": "int64",
|
||||
"type": "integer"
|
||||
},
|
||||
"no_memories_if_mcp_or_web_search": {
|
||||
"description": "When `true`, web searches and MCP tool calls mark the thread `memory_mode` as `\"polluted\"`.",
|
||||
"type": "boolean"
|
||||
},
|
||||
"use_memories": {
|
||||
"description": "When `false`, skip injecting memory usage instructions into developer prompts.",
|
||||
"type": "boolean"
|
||||
|
||||
@@ -221,7 +221,7 @@ persistence = "none"
|
||||
|
||||
let memories = r#"
|
||||
[memories]
|
||||
no_memories_if_mcp_or_web_search = true
|
||||
disable_on_external_context = true
|
||||
generate_memories = false
|
||||
use_memories = false
|
||||
max_raw_memories_for_consolidation = 512
|
||||
@@ -236,7 +236,7 @@ consolidation_model = "gpt-5"
|
||||
toml::from_str::<ConfigToml>(memories).expect("TOML deserialization should succeed");
|
||||
assert_eq!(
|
||||
Some(MemoriesToml {
|
||||
no_memories_if_mcp_or_web_search: Some(true),
|
||||
disable_on_external_context: Some(true),
|
||||
generate_memories: Some(false),
|
||||
use_memories: Some(false),
|
||||
max_raw_memories_for_consolidation: Some(512),
|
||||
@@ -260,7 +260,7 @@ consolidation_model = "gpt-5"
|
||||
assert_eq!(
|
||||
config.memories,
|
||||
MemoriesConfig {
|
||||
no_memories_if_mcp_or_web_search: true,
|
||||
disable_on_external_context: true,
|
||||
generate_memories: false,
|
||||
use_memories: false,
|
||||
max_raw_memories_for_consolidation: 512,
|
||||
@@ -272,6 +272,18 @@ consolidation_model = "gpt-5"
|
||||
consolidation_model: Some("gpt-5".to_string()),
|
||||
}
|
||||
);
|
||||
|
||||
let legacy_memories_cfg =
|
||||
toml::from_str::<ConfigToml>("[memories]\nno_memories_if_mcp_or_web_search = true\n")
|
||||
.expect("legacy memories TOML should deserialize");
|
||||
assert!(
|
||||
MemoriesConfig::from(
|
||||
legacy_memories_cfg
|
||||
.memories
|
||||
.expect("legacy memories config")
|
||||
)
|
||||
.disable_on_external_context
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -541,11 +541,7 @@ async fn augment_mcp_tool_request_meta_with_sandbox_state(
|
||||
}
|
||||
|
||||
async fn maybe_mark_thread_memory_mode_polluted(sess: &Session, turn_context: &TurnContext) {
|
||||
if !turn_context
|
||||
.config
|
||||
.memories
|
||||
.no_memories_if_mcp_or_web_search
|
||||
{
|
||||
if !turn_context.config.memories.disable_on_external_context {
|
||||
return;
|
||||
}
|
||||
state_db::mark_thread_memory_mode_polluted(
|
||||
|
||||
@@ -154,10 +154,7 @@ pub(crate) async fn mark_thread_memory_mode_polluted_if_external_context(
|
||||
turn_context: &TurnContext,
|
||||
item: &ResponseItem,
|
||||
) {
|
||||
if !turn_context
|
||||
.config
|
||||
.memories
|
||||
.no_memories_if_mcp_or_web_search
|
||||
if !turn_context.config.memories.disable_on_external_context
|
||||
|| !response_item_may_include_external_context(item)
|
||||
{
|
||||
return;
|
||||
|
||||
@@ -314,7 +314,7 @@ async fn web_search_pollution_moves_selected_thread_into_removed_phase2_inputs()
|
||||
.enable(Feature::MemoryTool)
|
||||
.expect("test config should allow feature update");
|
||||
config.memories.max_raw_memories_for_consolidation = 1;
|
||||
config.memories.no_memories_if_mcp_or_web_search = true;
|
||||
config.memories.disable_on_external_context = true;
|
||||
});
|
||||
let initial = initial_builder.build(&server).await?;
|
||||
mount_sse_once(
|
||||
@@ -386,7 +386,7 @@ async fn web_search_pollution_moves_selected_thread_into_removed_phase2_inputs()
|
||||
.enable(Feature::MemoryTool)
|
||||
.expect("test config should allow feature update");
|
||||
config.memories.max_raw_memories_for_consolidation = 1;
|
||||
config.memories.no_memories_if_mcp_or_web_search = true;
|
||||
config.memories.disable_on_external_context = true;
|
||||
});
|
||||
let resumed = resumed_builder
|
||||
.resume(&server, home.clone(), rollout_path.clone())
|
||||
|
||||
@@ -297,7 +297,7 @@ async fn web_search_marks_thread_memory_mode_polluted_when_configured() -> Resul
|
||||
.features
|
||||
.enable(Feature::Sqlite)
|
||||
.expect("test config should allow feature update");
|
||||
config.memories.no_memories_if_mcp_or_web_search = true;
|
||||
config.memories.disable_on_external_context = true;
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let db = test.codex.state_db().expect("state db enabled");
|
||||
@@ -355,7 +355,7 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result<
|
||||
.features
|
||||
.enable(Feature::Sqlite)
|
||||
.expect("test config should allow feature update");
|
||||
config.memories.no_memories_if_mcp_or_web_search = true;
|
||||
config.memories.disable_on_external_context = true;
|
||||
|
||||
let mut servers = config.mcp_servers.get().clone();
|
||||
servers.insert(
|
||||
|
||||
@@ -8274,8 +8274,9 @@ mod tests {
|
||||
Some(&TomlValue::Boolean(false))
|
||||
);
|
||||
assert!(
|
||||
!memories.contains_key("no_memories_if_mcp_or_web_search"),
|
||||
"the TUI menu should not write the MCP pollution setting"
|
||||
!memories.contains_key("disable_on_external_context")
|
||||
&& !memories.contains_key("no_memories_if_mcp_or_web_search"),
|
||||
"the TUI menu should not write the external-context memory setting"
|
||||
);
|
||||
app_server.shutdown().await?;
|
||||
Ok(())
|
||||
|
||||
Reference in New Issue
Block a user