From 73f32840c6efc86fead6b5af16031680d66f7241 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Wed, 4 Feb 2026 19:03:14 -0800 Subject: [PATCH] chore(core) personality migration tests (#10650) ## Summary Adds additional tests for personality edge cases ## Testing - [x] These are tests --- .../app-server/tests/suite/v2/turn_start.rs | 94 ++++++++++ .../core/tests/suite/personality_migration.rs | 176 ++++++++++++++++++ 2 files changed, 270 insertions(+) diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 2fc68e0be..bbf94f1bd 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -2,6 +2,7 @@ use anyhow::Result; use app_test_support::McpProcess; use app_test_support::create_apply_patch_sse_response; use app_test_support::create_exec_command_sse_response; +use app_test_support::create_fake_rollout; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_mock_responses_server_sequence_unchecked; @@ -34,8 +35,10 @@ use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::TurnStartedNotification; use codex_app_server_protocol::TurnStatus; use codex_app_server_protocol::UserInput as V2UserInput; +use codex_core::config::ConfigToml; use codex_core::features::FEATURES; use codex_core::features::Feature; +use codex_core::personality_migration::PERSONALITY_MIGRATION_FILENAME; use codex_core::protocol_config_types::ReasoningSummary; use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ModeKind; @@ -52,6 +55,7 @@ use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); const TEST_ORIGINATOR: &str = "codex_vscode"; +const LOCAL_PRAGMATIC_TEMPLATE: &str = "You are a deeply pragmatic, effective software engineer."; #[tokio::test] async fn turn_start_sends_originator_header() -> Result<()> { @@ -595,6 +599,96 @@ async fn turn_start_change_personality_mid_thread_v2() -> Result<()> { Ok(()) } +#[tokio::test] +async fn turn_start_uses_migrated_pragmatic_personality_without_override_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let body = responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_assistant_message("msg-1", "Done"), + responses::ev_completed("resp-1"), + ]); + let response_mock = responses::mount_sse_once(&server, body).await; + + let codex_home = TempDir::new()?; + create_config_toml( + codex_home.path(), + &server.uri(), + "never", + &BTreeMap::from([(Feature::Personality, true)]), + )?; + create_fake_rollout( + codex_home.path(), + "2025-01-01T00-00-00", + "2025-01-01T00:00:00Z", + "history user message", + Some("mock_provider"), + None, + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let persisted_toml: ConfigToml = toml::from_str(&std::fs::read_to_string( + codex_home.path().join("config.toml"), + )?)?; + assert_eq!(persisted_toml.personality, Some(Personality::Pragmatic)); + assert!( + codex_home + .path() + .join(PERSONALITY_MIGRATION_FILENAME) + .exists(), + "expected personality migration marker to be written on startup" + ); + + let thread_req = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("gpt-5.2-codex".to_string()), + ..Default::default() + }) + .await?; + let thread_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(thread_resp)?; + + let turn_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id, + input: vec![V2UserInput::Text { + text: "Hello".to_string(), + text_elements: Vec::new(), + }], + personality: None, + ..Default::default() + }) + .await?; + let turn_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_req)), + ) + .await??; + let _turn: TurnStartResponse = to_response::(turn_resp)?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let request = response_mock.single_request(); + let instructions_text = request.instructions_text(); + assert!( + instructions_text.contains(LOCAL_PRAGMATIC_TEMPLATE), + "expected startup-migrated pragmatic personality in model instructions, got: {instructions_text:?}" + ); + + Ok(()) +} + #[tokio::test] async fn turn_start_accepts_local_image_input() -> Result<()> { // Two Codex turns hit the mock model (session start + turn/start). diff --git a/codex-rs/core/tests/suite/personality_migration.rs b/codex-rs/core/tests/suite/personality_migration.rs index 492b4fdd2..2ca7aeeef 100644 --- a/codex-rs/core/tests/suite/personality_migration.rs +++ b/codex-rs/core/tests/suite/personality_migration.rs @@ -42,6 +42,16 @@ async fn write_archived_session_with_user_event(codex_home: &Path) -> io::Result write_rollout_with_user_event(&dir, thread_id).await } +async fn write_session_with_meta_only(codex_home: &Path) -> io::Result<()> { + let thread_id = ThreadId::new(); + let dir = codex_home + .join(SESSIONS_SUBDIR) + .join("2025") + .join("01") + .join("01"); + write_rollout_with_meta_only(&dir, thread_id).await +} + async fn write_rollout_with_user_event(dir: &Path, thread_id: ThreadId) -> io::Result<()> { tokio::fs::create_dir_all(&dir).await?; let file_path = dir.join(format!("rollout-{TEST_TIMESTAMP}-{thread_id}.jsonl")); @@ -83,6 +93,40 @@ async fn write_rollout_with_user_event(dir: &Path, thread_id: ThreadId) -> io::R Ok(()) } +async fn write_rollout_with_meta_only(dir: &Path, thread_id: ThreadId) -> io::Result<()> { + tokio::fs::create_dir_all(&dir).await?; + let file_path = dir.join(format!("rollout-{TEST_TIMESTAMP}-{thread_id}.jsonl")); + let mut file = tokio::fs::File::create(&file_path).await?; + + let session_meta = SessionMetaLine { + meta: SessionMeta { + id: thread_id, + forked_from_id: None, + timestamp: TEST_TIMESTAMP.to_string(), + cwd: std::path::PathBuf::from("."), + originator: "test_originator".to_string(), + cli_version: "test_version".to_string(), + source: SessionSource::Cli, + model_provider: None, + base_instructions: None, + dynamic_tools: None, + }, + git: None, + }; + let meta_line = RolloutLine { + timestamp: TEST_TIMESTAMP.to_string(), + item: RolloutItem::SessionMeta(session_meta), + }; + + let meta_json = serde_json::to_string(&meta_line)?; + file.write_all(format!("{meta_json}\n").as_bytes()).await?; + Ok(()) +} + +fn parse_config_toml(contents: &str) -> io::Result { + toml::from_str(contents).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)) +} + #[tokio::test] async fn migration_marker_exists_no_sessions_no_change() -> io::Result<()> { let temp = TempDir::new()?; @@ -135,6 +179,138 @@ async fn no_marker_sessions_sets_personality() -> io::Result<()> { Ok(()) } +#[tokio::test] +async fn no_marker_sessions_preserves_existing_config_fields() -> io::Result<()> { + let temp = TempDir::new()?; + write_session_with_user_event(temp.path()).await?; + tokio::fs::write(temp.path().join("config.toml"), "model = \"gpt-5-codex\"\n").await?; + let config_toml = read_config_toml(temp.path()).await?; + + let status = maybe_migrate_personality(temp.path(), &config_toml).await?; + + assert_eq!(status, PersonalityMigrationStatus::Applied); + let persisted = read_config_toml(temp.path()).await?; + assert_eq!(persisted.model, Some("gpt-5-codex".to_string())); + assert_eq!(persisted.personality, Some(Personality::Pragmatic)); + Ok(()) +} + +#[tokio::test] +async fn no_marker_meta_only_rollout_is_treated_as_no_sessions() -> io::Result<()> { + let temp = TempDir::new()?; + write_session_with_meta_only(temp.path()).await?; + + let status = maybe_migrate_personality(temp.path(), &ConfigToml::default()).await?; + + assert_eq!(status, PersonalityMigrationStatus::SkippedNoSessions); + assert_eq!( + tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?, + true + ); + assert_eq!( + tokio::fs::try_exists(temp.path().join("config.toml")).await?, + false + ); + Ok(()) +} + +#[tokio::test] +async fn no_marker_explicit_global_personality_skips_migration() -> io::Result<()> { + let temp = TempDir::new()?; + write_session_with_user_event(temp.path()).await?; + let config_toml = parse_config_toml("personality = \"friendly\"\n")?; + + let status = maybe_migrate_personality(temp.path(), &config_toml).await?; + + assert_eq!( + status, + PersonalityMigrationStatus::SkippedExplicitPersonality + ); + assert_eq!( + tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?, + true + ); + assert_eq!( + tokio::fs::try_exists(temp.path().join("config.toml")).await?, + false + ); + Ok(()) +} + +#[tokio::test] +async fn no_marker_profile_personality_skips_migration() -> io::Result<()> { + let temp = TempDir::new()?; + write_session_with_user_event(temp.path()).await?; + let config_toml = parse_config_toml( + r#" +profile = "work" + +[profiles.work] +personality = "friendly" +"#, + )?; + + let status = maybe_migrate_personality(temp.path(), &config_toml).await?; + + assert_eq!( + status, + PersonalityMigrationStatus::SkippedExplicitPersonality + ); + assert_eq!( + tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?, + true + ); + assert_eq!( + tokio::fs::try_exists(temp.path().join("config.toml")).await?, + false + ); + Ok(()) +} + +#[tokio::test] +async fn marker_short_circuits_invalid_profile_resolution() -> io::Result<()> { + let temp = TempDir::new()?; + tokio::fs::write(temp.path().join(PERSONALITY_MIGRATION_FILENAME), "v1\n").await?; + let config_toml = parse_config_toml("profile = \"missing\"\n")?; + + let status = maybe_migrate_personality(temp.path(), &config_toml).await?; + + assert_eq!(status, PersonalityMigrationStatus::SkippedMarker); + Ok(()) +} + +#[tokio::test] +async fn invalid_selected_profile_returns_error_and_does_not_write_marker() -> io::Result<()> { + let temp = TempDir::new()?; + let config_toml = parse_config_toml("profile = \"missing\"\n")?; + + let err = maybe_migrate_personality(temp.path(), &config_toml) + .await + .expect_err("missing profile should fail"); + + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert_eq!( + tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?, + false + ); + Ok(()) +} + +#[tokio::test] +async fn applied_migration_is_idempotent_on_second_run() -> io::Result<()> { + let temp = TempDir::new()?; + write_session_with_user_event(temp.path()).await?; + + let first_status = maybe_migrate_personality(temp.path(), &ConfigToml::default()).await?; + let second_status = maybe_migrate_personality(temp.path(), &ConfigToml::default()).await?; + + assert_eq!(first_status, PersonalityMigrationStatus::Applied); + assert_eq!(second_status, PersonalityMigrationStatus::SkippedMarker); + let persisted = read_config_toml(temp.path()).await?; + assert_eq!(persisted.personality, Some(Personality::Pragmatic)); + Ok(()) +} + #[tokio::test] async fn no_marker_archived_sessions_sets_personality() -> io::Result<()> { let temp = TempDir::new()?;