mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Default fresh memory profiles to v2 (#56211)
## Why Profiles without a v1 memory workspace have no legacy context to preserve or consolidate before using v2. ## What changed - Select v2 when memories are enabled and the `memories` directory is absent. Preserve explicit `memories.version` settings, the `memories.dual_write = false` opt-out, and the v1 default for existing workspaces or filesystem check errors. - Report `v2_ready` immediately for profiles without a v1 workspace. Keep the consolidation count and valid-summary requirements for profiles with a v1 workspace. - Make resetting an absent memory workspace a no-op so it does not create an unused v1 directory. ## Testing Extend coverage for default and explicit version selection, opt-outs, version-specific memory reads, fresh-profile readiness and reset behavior, and running only the v2 pipeline for fresh profiles. GitOrigin-RevId: a8fb0844962ae9266ffbe88af97e8dc4a99ac15f
This commit is contained in:
@@ -35,10 +35,19 @@ impl ThreadRequestProcessor {
|
||||
let summary = tokio::fs::read_to_string(root.join("memory_summary.md"))
|
||||
.await
|
||||
.ok();
|
||||
let ready = count >= minimum
|
||||
&& summary
|
||||
.as_deref()
|
||||
.is_some_and(codex_memories_write::workspace::is_valid_v2_summary);
|
||||
let has_v1_workspace = tokio::fs::try_exists(
|
||||
self.config
|
||||
.codex_home
|
||||
.join(MemoryVersion::V1.directory_name()),
|
||||
)
|
||||
.await
|
||||
.map_err(|error| internal_error(format!("failed to check v1 memory workspace: {error}")))?;
|
||||
// Fresh profiles can start on V2 without consolidating any V1 history first.
|
||||
let ready = !has_v1_workspace
|
||||
|| count >= minimum
|
||||
&& summary
|
||||
.as_deref()
|
||||
.is_some_and(codex_memories_write::workspace::is_valid_v2_summary);
|
||||
Ok(Some(
|
||||
MemoryStatusResponse {
|
||||
v2_consolidated_threads: count,
|
||||
|
||||
@@ -22,9 +22,11 @@ use tempfile::TempDir;
|
||||
|
||||
#[tokio::test]
|
||||
async fn memory_read_version_selects_only_its_own_context() -> Result<()> {
|
||||
for (version, expected, excluded) in [
|
||||
("v1", "legacy-only-marker", "v2-only-marker"),
|
||||
("v2", "v2-only-marker", "legacy-only-marker"),
|
||||
for (version, has_v1_workspace, expected, excluded) in [
|
||||
(Some("v1"), true, "legacy-only-marker", "v2-only-marker"),
|
||||
(Some("v2"), true, "v2-only-marker", "legacy-only-marker"),
|
||||
(None, true, "legacy-only-marker", "v2-only-marker"),
|
||||
(None, false, "v2-only-marker", "legacy-only-marker"),
|
||||
] {
|
||||
let server = start_mock_server().await;
|
||||
let home = TempDir::new()?;
|
||||
@@ -35,6 +37,9 @@ async fn memory_read_version_selects_only_its_own_context() -> Result<()> {
|
||||
("memories", "legacy-only-marker"),
|
||||
("memories_v2", "v2-only-marker"),
|
||||
] {
|
||||
if directory == "memories" && !has_v1_workspace {
|
||||
continue;
|
||||
}
|
||||
tokio::fs::create_dir_all(home.path().join(directory)).await?;
|
||||
tokio::fs::write(
|
||||
home.path().join(directory).join("memory_summary.md"),
|
||||
@@ -55,17 +60,18 @@ async fn memory_read_version_selects_only_its_own_context() -> Result<()> {
|
||||
.with_codex_home(home.path())
|
||||
.build_initialized()
|
||||
.await?;
|
||||
let mut config = std::collections::HashMap::from([(
|
||||
"memories.generate_memories".to_string(),
|
||||
json!(false),
|
||||
)]);
|
||||
if let Some(version) = version {
|
||||
config.insert("memories.version".to_string(), json!(version));
|
||||
}
|
||||
let start = app
|
||||
.send_thread_start_request_with_auto_env(ThreadStartParams {
|
||||
// Skip background consolidation so it cannot consume the read test's response.
|
||||
ephemeral: Some(true),
|
||||
config: Some(
|
||||
[
|
||||
("memories.version".to_string(), json!(version)),
|
||||
("memories.generate_memories".to_string(), json!(false)),
|
||||
]
|
||||
.into(),
|
||||
),
|
||||
config: Some(config),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -155,15 +155,34 @@ async fn memory_status_requires_successful_v2_consolidation_and_resets() -> Resu
|
||||
.enable_feature(Feature::Sqlite)
|
||||
.write(home.path())?;
|
||||
let db = init_state_db(home.path()).await?;
|
||||
seed_stage1_output(&db, home.path(), MemoryVersion::V1).await?;
|
||||
let source = seed_stage1_output(&db, home.path(), MemoryVersion::V2).await?;
|
||||
seed_stage1_output(&db, home.path(), MemoryVersion::V2).await?;
|
||||
let mut server = TestAppServer::builder()
|
||||
.with_codex_home(home.path())
|
||||
.without_auto_env()
|
||||
.build_initialized_with_timeout(DEFAULT_READ_TIMEOUT)
|
||||
.await?;
|
||||
let params = serde_json::json!({"minConsolidatedThreads": 2});
|
||||
// Resetting a fresh profile must not create an unused V1 workspace.
|
||||
let request = server
|
||||
.send_raw_request("memory/reset", /*params*/ None)
|
||||
.await?;
|
||||
let _: MemoryResetResponse = server.read_response(request).await?;
|
||||
assert!(!home.path().join("memories").exists());
|
||||
let request = server
|
||||
.send_raw_request("memory/status", Some(params.clone()))
|
||||
.await?;
|
||||
let status: MemoryStatusResponse = server.read_response(request).await?;
|
||||
assert_eq!(
|
||||
status,
|
||||
MemoryStatusResponse {
|
||||
v2_consolidated_threads: 0,
|
||||
v2_ready: true
|
||||
}
|
||||
);
|
||||
// Even an empty V1 workspace retains the existing readiness requirements.
|
||||
tokio::fs::create_dir(home.path().join("memories")).await?;
|
||||
seed_stage1_output(&db, home.path(), MemoryVersion::V1).await?;
|
||||
let source = seed_stage1_output(&db, home.path(), MemoryVersion::V2).await?;
|
||||
seed_stage1_output(&db, home.path(), MemoryVersion::V2).await?;
|
||||
let request = server
|
||||
.send_raw_request("memory/status", Some(params.clone()))
|
||||
.await?;
|
||||
|
||||
@@ -320,7 +320,8 @@ pub use codex_protocol::MemoryVersion;
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct MemoriesToml {
|
||||
/// Selects the memory pipeline; v1 remains the default.
|
||||
/// Selects the memory pipeline. Fresh profiles default to v2 when memories are enabled,
|
||||
/// unless dual-write is explicitly disabled; existing v1 profiles default to v1.
|
||||
pub version: Option<MemoryVersion>,
|
||||
/// Generate both versions while v1 is selected; v2 runs alone after cutover.
|
||||
pub dual_write: Option<bool>,
|
||||
|
||||
@@ -2752,7 +2752,7 @@
|
||||
"$ref": "#/definitions/MemoryVersion"
|
||||
}
|
||||
],
|
||||
"description": "Selects the memory pipeline; v1 remains the default."
|
||||
"description": "Selects the memory pipeline. Fresh profiles default to v2 when memories are enabled, unless dual-write is explicitly disabled; existing v1 profiles default to v1."
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
|
||||
@@ -397,6 +397,34 @@ consolidation_model = "gpt-5.2"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn fresh_memory_profiles_preserve_explicit_version_and_transition_opt_out() {
|
||||
for (settings, expected) in [
|
||||
("", codex_protocol::MemoryVersion::V2),
|
||||
("version = 'v1'", codex_protocol::MemoryVersion::V1),
|
||||
("dual_write = false", codex_protocol::MemoryVersion::V1),
|
||||
(
|
||||
"version = 'v2'\ndual_write = false",
|
||||
codex_protocol::MemoryVersion::V2,
|
||||
),
|
||||
] {
|
||||
let home = tempdir().expect("tempdir");
|
||||
let cfg = toml::from_str::<ConfigToml>(&format!(
|
||||
"[features]\nmemories = true\n[memories]\n{settings}"
|
||||
))
|
||||
.expect("memory config");
|
||||
let config = Config::load_from_base_config_with_overrides(
|
||||
cfg,
|
||||
ConfigOverrides::default(),
|
||||
home.abs(),
|
||||
)
|
||||
.await
|
||||
.expect("load config");
|
||||
assert_eq!(config.memories.version, expected, "{settings}");
|
||||
assert!(!home.path().join("memories").exists());
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn goal_max_token_budget_requires_positive_integer() {
|
||||
let config_toml = toml::from_str::<ConfigToml>("[goals]\nmax_goal_token_budget = 25000\n")
|
||||
@@ -6301,7 +6329,7 @@ async fn memory_tool_makes_memories_root_readable_without_creating_or_widening_w
|
||||
-> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let cwd = TempDir::new()?;
|
||||
let memories_root = codex_home.path().join("memories");
|
||||
let memories_root = codex_home.path().join("memories_v2");
|
||||
let memories_root_abs = memories_root.abs();
|
||||
|
||||
let config = Config::load_from_base_config_with_overrides(
|
||||
|
||||
@@ -97,6 +97,7 @@ use codex_model_provider_info::built_in_model_providers;
|
||||
use codex_model_provider_info::merge_configured_model_providers;
|
||||
use codex_models_manager::ModelsManagerConfig;
|
||||
use codex_prompts::ResolvedModelMessages;
|
||||
use codex_protocol::MemoryVersion;
|
||||
use codex_protocol::config_types::AltScreenMode;
|
||||
use codex_protocol::config_types::AutoCompactTokenLimitScope;
|
||||
use codex_protocol::config_types::ForcedLoginMethod;
|
||||
@@ -3646,7 +3647,20 @@ impl Config {
|
||||
));
|
||||
}
|
||||
|
||||
let memories_config: MemoriesConfig = cfg.memories.clone().unwrap_or_default().into();
|
||||
let mut memories = cfg.memories.clone().unwrap_or_default();
|
||||
// Fresh profiles have no V1 context to preserve or warm up before using V2.
|
||||
// Keep explicit selections and transition opt-outs, and fail closed on IO errors.
|
||||
if features.enabled(Feature::MemoryTool)
|
||||
&& memories.version.is_none()
|
||||
&& memories.dual_write != Some(false)
|
||||
&& matches!(
|
||||
tokio::fs::try_exists(codex_home.join(MemoryVersion::V1.directory_name())).await,
|
||||
Ok(false)
|
||||
)
|
||||
{
|
||||
memories.version = Some(MemoryVersion::V2);
|
||||
}
|
||||
let memories_config: MemoriesConfig = memories.into();
|
||||
let memories_root = codex_home.join(memories_config.version.directory_name());
|
||||
|
||||
let profiles_are_active = effective_permission_selection.profiles_are_active(
|
||||
|
||||
@@ -24,7 +24,7 @@ pub(crate) async fn clear_memory_root_contents(memory_root: &Path) -> std::io::R
|
||||
));
|
||||
}
|
||||
Ok(_) => {}
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()),
|
||||
Err(err) => return Err(err),
|
||||
}
|
||||
|
||||
|
||||
@@ -10,14 +10,34 @@ use wiremock::matchers::path_regex;
|
||||
|
||||
#[tokio::test]
|
||||
async fn dual_write_runs_both_versions_until_v2_is_selected() -> anyhow::Result<()> {
|
||||
for (selected_version, expected_pipelines) in [(MemoryVersion::V1, 2), (MemoryVersion::V2, 1)] {
|
||||
for (version, expected_pipelines) in [(Some("v1"), 2), (Some("v2"), 1), (None, 1)] {
|
||||
let server = start_mock_server().await;
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
let mut memories = startup_test_memories_config();
|
||||
memories.dual_write = true;
|
||||
memories.version = selected_version;
|
||||
let test =
|
||||
build_test_codex_with_memories_config(&server, Arc::clone(&home), memories).await?;
|
||||
let version_config = version
|
||||
.map(|version| format!("version = {version:?}"))
|
||||
.unwrap_or_default();
|
||||
tokio::fs::write(
|
||||
home.path().join("config.toml"),
|
||||
format!(
|
||||
"[features]\nmemories = true\n\
|
||||
[memories]\ndual_write = true\nmax_raw_memories_for_consolidation = 1\n\
|
||||
min_rollout_idle_hours = 0\n{version_config}\n"
|
||||
),
|
||||
)
|
||||
.await?;
|
||||
let test = test_codex()
|
||||
.with_home(Arc::clone(&home))
|
||||
.build(&server)
|
||||
.await?;
|
||||
let selected_version = test.config.memories.version;
|
||||
assert_eq!(
|
||||
selected_version,
|
||||
if version == Some("v1") {
|
||||
MemoryVersion::V1
|
||||
} else {
|
||||
MemoryVersion::V2
|
||||
}
|
||||
);
|
||||
let db = test.codex.state_db().expect("memory state");
|
||||
let source = seed_stage1_candidate(
|
||||
&db,
|
||||
@@ -28,7 +48,9 @@ async fn dual_write_runs_both_versions_until_v2_is_selected() -> anyhow::Result<
|
||||
.await?;
|
||||
let v1_root = home.path().join("memories");
|
||||
let v2_root = home.path().join("memories_v2");
|
||||
seed_required_memory_artifacts(&v1_root).await?;
|
||||
if version.is_some() {
|
||||
seed_required_memory_artifacts(&v1_root).await?;
|
||||
}
|
||||
tokio::fs::create_dir_all(&v2_root).await?;
|
||||
tokio::fs::write(
|
||||
v2_root.join("memory_summary.md"),
|
||||
@@ -119,6 +141,9 @@ async fn dual_write_runs_both_versions_until_v2_is_selected() -> anyhow::Result<
|
||||
);
|
||||
assert!(!v2_root.join("MEMORY.md").exists());
|
||||
assert!(!v2_root.join("raw_memories.md").exists());
|
||||
if version.is_none() {
|
||||
assert!(!v1_root.exists());
|
||||
}
|
||||
shutdown_test_codex(&test).await?;
|
||||
}
|
||||
Ok(())
|
||||
|
||||
Reference in New Issue
Block a user