diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 647c0e7899..d9d4adedee 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -6,6 +6,7 @@ use crate::session::SessionIo; use crate::session::SessionSettingsUpdate; use crate::session::new_submission_id; use crate::session::session::Session; +use crate::session::step_settings::StepSettingsUpdate; use codex_diagnostics::Gauge; use codex_diagnostics::GaugeGuard; use codex_exec_server::SelectedCapabilityRootsStatus; @@ -567,18 +568,20 @@ impl CodexThread { }; SessionSettingsUpdate { + step_settings: StepSettingsUpdate { + collaboration_mode: Some(collaboration_mode), + reasoning_summary: summary, + service_tier, + personality, + approval_policy, + approvals_reviewer, + }, environments, profile_workspace_roots, - approval_policy, - approvals_reviewer, sandbox_policy, permission_profile, active_permission_profile, windows_sandbox_level, - collaboration_mode: Some(collaboration_mode), - reasoning_summary: summary, - service_tier, - personality, ..Default::default() } } diff --git a/codex-rs/core/src/session/environment.rs b/codex-rs/core/src/session/environment.rs index 106be766fb..1be2fda061 100644 --- a/codex-rs/core/src/session/environment.rs +++ b/codex-rs/core/src/session/environment.rs @@ -20,14 +20,21 @@ use crate::session::session::SessionSettingsUpdate; pub(super) fn validate_environment_selections( selections: &[TurnEnvironmentSelection], -) -> CodexResult<()> { +) -> ConstraintResult<()> { for selection in selections { match &selection.config { EnvironmentConfigState::FromThread | EnvironmentConfigState::Pending | EnvironmentConfigState::Failed(_) => {} EnvironmentConfigState::Ready(config) => { - validate_environment_config(selection, config)?; + validate_environment_config(selection, config).map_err(|error| { + ConstraintError::InvalidValue { + field_name: "environments", + candidate: "environment configuration".to_string(), + allowed: format!("valid environment configuration ({error})"), + requirement_source: codex_config::RequirementSource::Unknown, + } + })?; } } } diff --git a/codex-rs/core/src/session/mcp_runtime.rs b/codex-rs/core/src/session/mcp_runtime.rs index 50b26a762f..10029b8a8f 100644 --- a/codex-rs/core/src/session/mcp_runtime.rs +++ b/codex-rs/core/src/session/mcp_runtime.rs @@ -41,8 +41,9 @@ impl Session { // This gap predates that API: an internal Windows-level-only settings update // can leave the published MCP configuration stale. current.cwd() != next.cwd() - || current.approval_policy.value() != next.approval_policy.value() - || current.approvals_reviewer != next.approvals_reviewer + || current.step_settings.approval_policy.value() + != next.step_settings.approval_policy.value() + || current.step_settings.approvals_reviewer != next.step_settings.approvals_reviewer || current.permission_profile() != next.permission_profile() || updates.environments.as_ref().is_some_and(|environments| { environments.environments != self.services.turn_environments.selections() diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index f0250debe3..6ced24ca68 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -41,6 +41,7 @@ use crate::image_preparation::unified_image_budget_enabled; use crate::parse_turn_item; use crate::realtime_conversation::RealtimeConversationManager; use crate::session::step_context::StepContext; +use crate::session::step_settings::StepSettings; use crate::session::turn_context::TurnEnvironment; use crate::session_prefix::format_inter_agent_completion_message; use crate::skills_load_input_from_config; @@ -193,7 +194,6 @@ use crate::codex_thread::ThreadConfigSnapshot; #[cfg(test)] use crate::compact::collect_user_messages; use crate::config::Config; -use crate::config::Constrained; use crate::config::ConstraintResult; use crate::config::PermissionProfileSnapshot; use crate::config::PermissionProfileState; @@ -230,6 +230,7 @@ mod rollout_reconstruction; #[allow(clippy::module_inception)] pub(crate) mod session; pub(crate) mod step_context; +pub(crate) mod step_settings; mod thread_settings; pub(crate) mod time_reminder; mod token_budget; @@ -705,14 +706,17 @@ impl Session { config.model_provider.clone(), Some(Arc::clone(&auth_manager)), ), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, - service_tier, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - personality: config.personality, base_instructions, - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: session_permission_profile_state_from_config(&config)?, allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -3517,7 +3521,11 @@ impl Session { pub(crate) async fn collaboration_mode(&self) -> CollaborationMode { let state = self.state.lock().await; - state.session_configuration.collaboration_mode.clone() + state + .session_configuration + .step_settings + .collaboration_mode + .clone() } pub(crate) fn multi_agent_version(&self) -> Option { diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 320213eda2..ca2853e198 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -93,6 +93,7 @@ pub(super) async fn spawn_review_thread( state.session_configuration.thread_source.clone(), state .session_configuration + .step_settings .service_tier .clone() .or_else(|| config.service_tier.clone()), diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 11afc57169..53c35644e0 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1,5 +1,9 @@ use super::input_queue::InputQueue; use super::mcp_refresh::McpRefresh; +use super::step_settings::ModelInfoOverrides; +use super::step_settings::StepSettings; +use super::step_settings::StepSettingsConstraints; +use super::step_settings::StepSettingsUpdate; use super::*; use crate::agents_md_manager::AgentsMdManager; use crate::config::ConstraintError; @@ -17,8 +21,6 @@ use codex_login::auth::AgentIdentityAuthPolicy; use codex_model_provider::SharedModelProvider; use codex_protocol::SessionId; use codex_protocol::capabilities::SelectedCapabilityRoot; -use codex_protocol::config_types::SERVICE_TIER_DEFAULT_REQUEST_VALUE; -use codex_protocol::config_types::ServiceTier; use codex_protocol::config_types::ShellEnvironmentPolicy; use codex_protocol::mcp::ClientMcpExtensions; use codex_protocol::permissions::FileSystemPath; @@ -75,22 +77,17 @@ pub(crate) struct SessionConfiguration { /// Runtime provider and its provider-specific execution policy. pub(super) provider: SharedModelProvider, - pub(super) collaboration_mode: CollaborationMode, - pub(super) model_reasoning_summary: Option, - pub(super) service_tier: Option, + /// Desired configured inputs inherited by future turns. + pub(super) step_settings: Arc, + /// Explicit startup overrides used when resolving effective model metadata. + pub(super) model_info_overrides: ModelInfoOverrides, /// Developer instructions that supplement the base instructions. pub(super) developer_instructions: Option, - /// Personality preference for the model. - pub(super) personality: Option, - /// Base instructions for the session. pub(super) base_instructions: String, - /// When to escalate for approval for execution - pub(super) approval_policy: Constrained, - pub(super) approvals_reviewer: ApprovalsReviewer, /// Permission profile state for the session. Keep the constrained profile, /// active profile id, and profile-defined workspace roots in sync by using /// the methods below instead of mutating the fields independently. @@ -226,11 +223,11 @@ impl SessionConfiguration { .map(|config| config.permission_profile.clone()) .unwrap_or_else(|| self.permission_profile_state.snapshot()); ThreadConfigSnapshot { - model: self.collaboration_mode.model().to_string(), + model: self.step_settings.collaboration_mode.model().to_string(), model_provider_id: self.original_config_do_not_use.model_provider_id.clone(), - service_tier: self.service_tier.clone(), - approval_policy: self.approval_policy.value(), - approvals_reviewer: self.approvals_reviewer, + service_tier: self.step_settings.service_tier.clone(), + approval_policy: self.step_settings.approval_policy.value(), + approvals_reviewer: self.step_settings.approvals_reviewer, permission_profile: permission_profile .permission_profile() .clone() @@ -243,10 +240,10 @@ impl SessionConfiguration { workspace_roots, profile_workspace_roots: permission_profile.profile_workspace_roots().to_vec(), ephemeral: self.original_config_do_not_use.ephemeral, - reasoning_effort: self.collaboration_mode.reasoning_effort(), - reasoning_summary: self.model_reasoning_summary, - personality: self.personality, - collaboration_mode: self.collaboration_mode.clone(), + reasoning_effort: self.step_settings.collaboration_mode.reasoning_effort(), + reasoning_summary: self.step_settings.reasoning_summary, + personality: self.step_settings.personality, + collaboration_mode: self.step_settings.collaboration_mode.clone(), session_source: self.session_source.clone(), history_mode: self.history_mode, forked_from_thread_id: self.forked_from_thread_id, @@ -262,18 +259,18 @@ impl SessionConfiguration { environment_selections: &[TurnEnvironmentSelection], ) -> ThreadSettingsSnapshot { ThreadSettingsSnapshot { - model: self.collaboration_mode.model().to_string(), + model: self.step_settings.collaboration_mode.model().to_string(), model_provider_id: self.original_config_do_not_use.model_provider_id.clone(), - service_tier: self.service_tier.clone(), - approval_policy: self.approval_policy.value(), - approvals_reviewer: self.approvals_reviewer, + service_tier: self.step_settings.service_tier.clone(), + approval_policy: self.step_settings.approval_policy.value(), + approvals_reviewer: self.step_settings.approvals_reviewer, permission_profile: self.materialized_permission_profile(environment_selections), active_permission_profile: self.active_permission_profile(), cwd: self.legacy_fallback_cwd.clone(), - reasoning_effort: self.collaboration_mode.reasoning_effort(), - reasoning_summary: self.model_reasoning_summary, - personality: self.personality, - collaboration_mode: self.collaboration_mode.clone(), + reasoning_effort: self.step_settings.collaboration_mode.reasoning_effort(), + reasoning_summary: self.step_settings.reasoning_summary, + personality: self.step_settings.personality, + collaboration_mode: self.step_settings.collaboration_mode.clone(), } } @@ -292,14 +289,14 @@ impl SessionConfiguration { .profile_workspace_roots() .to_vec(), ), - approval_policy: Some(self.approval_policy.value()), - approvals_reviewer: Some(self.approvals_reviewer), + approval_policy: Some(self.step_settings.approval_policy.value()), + approvals_reviewer: Some(self.step_settings.approvals_reviewer), permission_profile: Some(self.permission_profile()), active_permission_profile: self.active_permission_profile(), - summary: self.model_reasoning_summary, - service_tier: Some(self.service_tier.clone()), - collaboration_mode: Some(self.collaboration_mode.clone()), - personality: self.personality, + summary: self.step_settings.reasoning_summary, + service_tier: Some(self.step_settings.service_tier.clone()), + collaboration_mode: Some(self.step_settings.collaboration_mode.clone()), + personality: self.step_settings.personality, // TODO(anp): Include the runtime Windows sandbox override in restored settings. // This omission predates TurnEnvironment::sandbox_context; rebuilding FromThread // environments falls back to Config instead of retaining that override. @@ -311,34 +308,15 @@ impl SessionConfiguration { &self, environments: &[TurnEnvironmentSelection], ) -> ConstraintResult<()> { - self.validate_auto_review_requirement(environments)?; - super::environment::validate_environment_selections(environments).map_err(|error| { - ConstraintError::InvalidValue { - field_name: "environments", - candidate: "environment configuration".to_string(), - allowed: format!("valid environment configuration ({error})"), - requirement_source: codex_config::RequirementSource::Unknown, - } - }) + self.step_settings + .validate(&self.step_settings_constraints(environments))?; + super::environment::validate_environment_selections(environments) } - fn validate_auto_review_requirement( + pub(super) fn step_settings_constraints( &self, environments: &[TurnEnvironmentSelection], - ) -> ConstraintResult<()> { - if self.trusted_guardian_reviewer { - return Ok(()); - } - - let requirements = self - .original_config_do_not_use - .config_layer_stack - .requirements(); - let model = self.collaboration_mode.model(); - if !requirements.auto_review_required_for_model(model) { - return Ok(()); - } - + ) -> StepSettingsConstraints<'_> { let permission_profile = ThreadEnvironments::primary_config_for(environments) .map(|config| config.permission_profile.permission_profile()) .unwrap_or_else(|| self.permission_profile_state.permission_profile()) @@ -346,21 +324,20 @@ impl SessionConfiguration { .materialize_project_roots_with_workspace_roots( &ThreadEnvironments::primary_workspace_roots_for(environments), ); - if self.approvals_reviewer == ApprovalsReviewer::AutoReview - && !permission_profile - .file_system_sandbox_policy() - .has_full_disk_write_access() - && self + StepSettingsConstraints { + requirements: self + .original_config_do_not_use + .config_layer_stack + .requirements(), + guardian_approval_enabled: self .original_config_do_not_use .features - .enabled(Feature::GuardianApproval) - { - return Ok(()); + .enabled(Feature::GuardianApproval), + trusted_guardian_reviewer: self.trusted_guardian_reviewer, + has_full_disk_write_access: permission_profile + .file_system_sandbox_policy() + .has_full_disk_write_access(), } - - Err(ConstraintError::AutoReviewRequired { - model: model.to_string(), - }) } pub(super) fn apply( @@ -386,57 +363,6 @@ impl SessionConfiguration { } ) }); - if let Some(collaboration_mode) = updates.collaboration_mode.clone() { - next_configuration.collaboration_mode = collaboration_mode; - } - if let Some(summary) = updates.reasoning_summary { - next_configuration.model_reasoning_summary = Some(summary); - } - if let Some(service_tier) = updates.service_tier.clone() { - // TODO(aibrahim): Remove once v2 clients no longer send the legacy - // "fast" service tier value. - next_configuration.service_tier = match service_tier { - Some(service_tier) => Some( - ServiceTier::from_request_value(&service_tier) - .map_or(service_tier, |service_tier| { - service_tier.request_value().to_string() - }), - ), - None => Some(SERVICE_TIER_DEFAULT_REQUEST_VALUE.to_string()), - }; - } - if let Some(personality) = updates.personality { - next_configuration.personality = Some(personality); - } - if let Some(approval_policy) = updates.approval_policy { - next_configuration.approval_policy.set(approval_policy)?; - } - if let Some(approvals_reviewer) = updates.approvals_reviewer { - next_configuration - .original_config_do_not_use - .config_layer_stack - .requirements() - .approvals_reviewer - .can_set(&approvals_reviewer)?; - next_configuration.approvals_reviewer = approvals_reviewer; - } - if !next_configuration.trusted_guardian_reviewer - && self.collaboration_mode.model() != next_configuration.collaboration_mode.model() - && next_configuration - .original_config_do_not_use - .config_layer_stack - .requirements() - .auto_review_required_for_model(next_configuration.collaboration_mode.model()) - && updates.approvals_reviewer.is_none() - { - next_configuration - .original_config_do_not_use - .config_layer_stack - .requirements() - .approvals_reviewer - .can_set(&ApprovalsReviewer::AutoReview)?; - next_configuration.approvals_reviewer = ApprovalsReviewer::AutoReview; - } if let Some(windows_sandbox_level) = updates.windows_sandbox_level { next_configuration.windows_sandbox_level = windows_sandbox_level; } @@ -548,7 +474,13 @@ impl SessionConfiguration { .map_or(current_environments, |environments| { environments.environments.as_slice() }); - next_configuration.validate(next_environments)?; + super::environment::validate_environment_selections(next_environments)?; + // Apply step settings last: the proposed permissions and environment + // selections must be complete before deriving their validation constraints. + next_configuration.step_settings = Arc::new(self.step_settings.apply( + &updates.step_settings, + &next_configuration.step_settings_constraints(next_environments), + )?); Ok(next_configuration) } @@ -597,20 +529,15 @@ pub(crate) struct SessionSettingsCommit { #[derive(Default, Clone)] pub(crate) struct SessionSettingsUpdate { + pub(crate) step_settings: StepSettingsUpdate, pub(crate) environments: Option, pub(crate) profile_workspace_roots: Option>, - pub(crate) approval_policy: Option, - pub(crate) approvals_reviewer: Option, pub(crate) sandbox_policy: Option, pub(crate) permission_profile: Option, pub(crate) active_permission_profile: Option, pub(crate) windows_sandbox_level: Option, - pub(crate) collaboration_mode: Option, - pub(crate) reasoning_summary: Option, - pub(crate) service_tier: Option>, pub(crate) service_tier_for_turn: Option, pub(crate) final_output_json_schema: Option>, - pub(crate) personality: Option, pub(crate) app_server_client_name: Option, pub(crate) app_server_client_version: Option, } @@ -719,7 +646,10 @@ impl Session { ) -> anyhow::Result> { debug!( "Configuring session: model={}; provider={:?}", - session_configuration.collaboration_mode.model(), + session_configuration + .step_settings + .collaboration_mode + .model(), session_configuration.provider ); let base_instructions_provenance = if config.base_instructions.is_some() { @@ -1031,9 +961,9 @@ impl Session { session_source: session_configuration.session_source.clone(), cwd: session_configuration.cwd().to_path_buf(), rollout_path: rollout_path.clone(), - model: session_configuration.collaboration_mode.model().to_string(), + model: session_configuration.step_settings.collaboration_mode.model().to_string(), provider_name: config.model_provider_id.clone(), - approval_policy: session_configuration.approval_policy.value().to_string(), + approval_policy: session_configuration.step_settings.approval_policy.value().to_string(), sandbox_policy: format!( "{:?}", session_configuration.sandbox_policy(environment_selections) @@ -1091,7 +1021,7 @@ impl Session { let account_email = telemetry_auth.and_then(CodexAuth::get_account_email); let originator = session_configuration.originator.clone(); let terminal_type = user_agent(); - let session_model = session_configuration.collaboration_mode.model().to_string(); + let session_model = session_configuration.step_settings.collaboration_mode.model().to_string(); let auth_env_telemetry = collect_auth_env_telemetry( session_configuration.provider.info(), auth_manager.codex_api_key_env_enabled(), @@ -1148,7 +1078,7 @@ impl Session { .collect::>(); session_telemetry.conversation_starts( config.model_provider.name.as_str(), - session_configuration.collaboration_mode.reasoning_effort(), + session_configuration.step_settings.collaboration_mode.reasoning_effort(), config .model_reasoning_summary .unwrap_or(ReasoningSummaryConfig::Auto), diff --git a/codex-rs/core/src/session/step_settings.rs b/codex-rs/core/src/session/step_settings.rs new file mode 100644 index 0000000000..e05bc7d472 --- /dev/null +++ b/codex-rs/core/src/session/step_settings.rs @@ -0,0 +1,174 @@ +//! Configured inputs used to derive effective step settings. + +use crate::config::Constrained; +use crate::config::ConstraintError; +use crate::config::ConstraintResult; +use codex_config::ConfigRequirements; +use codex_models_manager::ModelsManagerConfig; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::config_types::CollaborationMode; +use codex_protocol::config_types::Personality; +use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::config_types::SERVICE_TIER_DEFAULT_REQUEST_VALUE; +use codex_protocol::config_types::ServiceTier; +use codex_protocol::protocol::AskForApproval; + +/// Model and execution settings selected for an individual model step within +/// a turn. A turn may contain several steps, each using its own captured settings. +#[derive(Clone, Debug, PartialEq)] +pub(crate) struct StepSettings { + pub(crate) collaboration_mode: CollaborationMode, + pub(crate) reasoning_summary: Option, + pub(crate) service_tier: Option, + pub(crate) personality: Option, + pub(crate) approval_policy: Constrained, + pub(crate) approvals_reviewer: ApprovalsReviewer, +} + +/// Explicit startup overrides applied to catalog-derived model metadata. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(crate) struct ModelInfoOverrides { + pub(crate) context_window: Option, + pub(crate) auto_compact_token_limit: Option, + pub(crate) tool_output_token_limit: Option, + pub(crate) base_instructions: Option, +} + +impl From for ModelInfoOverrides { + fn from(config: ModelsManagerConfig) -> Self { + Self { + context_window: config.model_context_window, + auto_compact_token_limit: config.model_auto_compact_token_limit, + tool_output_token_limit: config.tool_output_token_limit, + base_instructions: config.base_instructions, + } + } +} + +impl ModelInfoOverrides { + pub(crate) fn models_manager_config( + &self, + personality: Option, + personality_enabled: bool, + ) -> ModelsManagerConfig { + ModelsManagerConfig { + model_context_window: self.context_window, + model_auto_compact_token_limit: self.auto_compact_token_limit, + tool_output_token_limit: self.tool_output_token_limit, + base_instructions: self.base_instructions.clone(), + personality, + personality_enabled, + // The models manager already owns its catalog. + model_catalog: None, + } + } +} + +/// Partial edits to configured step settings. +#[derive(Clone, Debug, Default, PartialEq)] +pub(crate) struct StepSettingsUpdate { + pub(crate) collaboration_mode: Option, + pub(crate) reasoning_summary: Option, + pub(crate) service_tier: Option>, + pub(crate) personality: Option, + pub(crate) approval_policy: Option, + pub(crate) approvals_reviewer: Option, +} + +/// Constraints used when applying and validating a candidate settings version. +pub(crate) struct StepSettingsConstraints<'a> { + pub(crate) requirements: &'a ConfigRequirements, + pub(crate) guardian_approval_enabled: bool, + pub(crate) trusted_guardian_reviewer: bool, + pub(crate) has_full_disk_write_access: bool, +} + +impl StepSettings { + /// Applies edits and validates the result against the supplied constraints. + /// Callers must supply the constraints of the proposed target environment. + pub(crate) fn apply( + &self, + update: &StepSettingsUpdate, + constraints: &StepSettingsConstraints<'_>, + ) -> ConstraintResult { + let mut next = self.clone(); + if let Some(collaboration_mode) = update.collaboration_mode.clone() { + next.collaboration_mode = collaboration_mode; + } + if let Some(summary) = update.reasoning_summary { + next.reasoning_summary = Some(summary); + } + if let Some(service_tier) = update.service_tier.clone() { + // TODO(aibrahim): Remove once v2 clients no longer send the legacy + // "fast" service tier value. + next.service_tier = Some(match service_tier { + Some(service_tier) => ServiceTier::from_request_value(&service_tier) + .map_or(service_tier, |service_tier| { + service_tier.request_value().to_string() + }), + None => SERVICE_TIER_DEFAULT_REQUEST_VALUE.to_string(), + }); + } + if let Some(personality) = update.personality { + next.personality = Some(personality); + } + if let Some(approval_policy) = update.approval_policy { + next.approval_policy.set(approval_policy)?; + } + if let Some(approvals_reviewer) = update.approvals_reviewer { + constraints + .requirements + .approvals_reviewer + .can_set(&approvals_reviewer)?; + next.approvals_reviewer = approvals_reviewer; + } + if !constraints.trusted_guardian_reviewer + && self.collaboration_mode.model() != next.collaboration_mode.model() + && constraints + .requirements + .auto_review_required_for_model(next.collaboration_mode.model()) + && update.approvals_reviewer.is_none() + { + constraints + .requirements + .approvals_reviewer + .can_set(&ApprovalsReviewer::AutoReview)?; + next.approvals_reviewer = ApprovalsReviewer::AutoReview; + } + next.validate(constraints)?; + Ok(next) + } + + /// Checks directly constructed settings or rechecks changed constraints. + pub(super) fn validate( + &self, + constraints: &StepSettingsConstraints<'_>, + ) -> ConstraintResult<()> { + if constraints.trusted_guardian_reviewer { + return Ok(()); + } + + let model = self.collaboration_mode.model(); + if !constraints + .requirements + .auto_review_required_for_model(model) + { + return Ok(()); + } + + if self.approvals_reviewer == ApprovalsReviewer::AutoReview + && !constraints.has_full_disk_write_access + && constraints.guardian_approval_enabled + { + return Ok(()); + } + + Err(ConstraintError::AutoReviewRequired { + model: model.to_string(), + }) + } +} + +#[cfg(test)] +#[path = "step_settings_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/session/step_settings_tests.rs b/codex-rs/core/src/session/step_settings_tests.rs new file mode 100644 index 0000000000..b169b9219d --- /dev/null +++ b/codex-rs/core/src/session/step_settings_tests.rs @@ -0,0 +1,272 @@ +use super::*; +use crate::config::PermissionProfileState; +use crate::session::session::SessionConfiguration; +use crate::session::session::SessionSettingsUpdate; +use crate::session::tests::make_session_configuration_for_tests; +use codex_config::ConfigLayerStack; +use codex_config::RequirementSource; +use codex_config::Sourced; +use codex_features::Feature; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxKind; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::TurnEnvironmentSelections; +use core_test_support::test_codex::local; +use pretty_assertions::assert_eq; +use std::collections::BTreeSet; +use std::sync::Arc; + +fn set_requirements(configuration: &mut SessionConfiguration, requirements: ConfigRequirements) { + let config = Arc::make_mut(&mut configuration.original_config_do_not_use); + config.config_layer_stack = ConfigLayerStack::new( + config + .config_layer_stack + .all_layers_low_to_high() + .cloned() + .collect(), + requirements, + config.config_layer_stack.requirements_toml().clone(), + ) + .expect("replace test requirements"); +} + +#[tokio::test] +async fn proposed_permission_profile_is_checked_before_step_settings() { + let mut configuration = make_session_configuration_for_tests().await; + let permission = Constrained::allow_only(PermissionProfile::read_only()); + let permission_error = permission + .can_set(&PermissionProfile::Disabled) + .unwrap_err(); + configuration.permission_profile_state = + PermissionProfileState::from_constrained_legacy(permission).unwrap(); + let approval = Constrained::allow_only(AskForApproval::OnRequest); + let approval_error = approval.can_set(&AskForApproval::Never).unwrap_err(); + Arc::make_mut(&mut configuration.step_settings).approval_policy = approval; + let mut requirements = configuration + .original_config_do_not_use + .config_layer_stack + .requirements() + .clone(); + requirements.approvals_reviewer.value = Constrained::allow_only(ApprovalsReviewer::User); + let reviewer_error = requirements + .approvals_reviewer + .can_set(&ApprovalsReviewer::AutoReview) + .unwrap_err(); + requirements.auto_review_required_models = Some(Sourced::new( + BTreeSet::from(["protected-model".to_string()]), + RequirementSource::Unknown, + )); + set_requirements(&mut configuration, requirements); + let protected_mode = configuration.step_settings.collaboration_mode.with_updates( + Some("protected-model".to_string()), + /*effort*/ None, + /*developer_instructions*/ None, + ); + + let invalid_profile = SessionSettingsUpdate { + permission_profile: Some(PermissionProfile::Disabled), + ..Default::default() + }; + assert_eq!( + configuration.apply(&invalid_profile, &[]).err().as_ref(), + Some(&permission_error) + ); + for (step_settings, expected) in [ + ( + StepSettingsUpdate { + approval_policy: Some(AskForApproval::Never), + ..Default::default() + }, + &approval_error, + ), + ( + StepSettingsUpdate { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + ..Default::default() + }, + &reviewer_error, + ), + ( + StepSettingsUpdate { + collaboration_mode: Some(protected_mode), + ..Default::default() + }, + &reviewer_error, + ), + ] { + assert_eq!( + configuration + .step_settings + .apply( + &step_settings, + &configuration.step_settings_constraints(&[]), + ) + .err() + .as_ref(), + Some(expected), + ); + assert_eq!( + configuration + .apply( + &SessionSettingsUpdate { + step_settings, + ..invalid_profile.clone() + }, + &[] + ) + .err() + .as_ref(), + Some(&permission_error), + ); + } +} + +#[tokio::test] +async fn model_review_requirement_uses_the_proposed_permission_profile() { + let mut configuration = make_session_configuration_for_tests().await; + configuration.permission_profile_state = PermissionProfileState::from_constrained_legacy( + Constrained::allow_any(PermissionProfile::Disabled), + ) + .unwrap(); + Arc::make_mut(&mut configuration.step_settings).approvals_reviewer = ApprovalsReviewer::User; + Arc::make_mut(&mut configuration.original_config_do_not_use) + .features + .enable(Feature::GuardianApproval) + .unwrap(); + let mut requirements = configuration + .original_config_do_not_use + .config_layer_stack + .requirements() + .clone(); + requirements.auto_review_required_models = Some(Sourced::new( + BTreeSet::from(["protected-model".to_string()]), + RequirementSource::Unknown, + )); + set_requirements(&mut configuration, requirements); + let collaboration_mode = configuration.step_settings.collaboration_mode.with_updates( + Some("protected-model".to_string()), + /*effort*/ None, + /*developer_instructions*/ None, + ); + let updates = SessionSettingsUpdate { + step_settings: StepSettingsUpdate { + collaboration_mode: Some(collaboration_mode.clone()), + ..Default::default() + }, + ..Default::default() + }; + assert_eq!( + configuration.apply(&updates, &[]).err(), + Some(ConstraintError::AutoReviewRequired { + model: "protected-model".to_string() + }), + ); + let updated = configuration + .apply( + &SessionSettingsUpdate { + permission_profile: Some(PermissionProfile::read_only()), + ..updates + }, + &[], + ) + .expect("the proposed restricted profile permits the protected model"); + assert_eq!( + updated.step_settings.as_ref(), + &StepSettings { + collaboration_mode, + approvals_reviewer: ApprovalsReviewer::AutoReview, + ..configuration.step_settings.as_ref().clone() + } + ); + assert_eq!(updated.permission_profile(), PermissionProfile::read_only()); + assert_eq!( + updated + .apply( + &SessionSettingsUpdate { + permission_profile: Some(PermissionProfile::Disabled), + ..Default::default() + }, + &[], + ) + .err(), + Some(ConstraintError::AutoReviewRequired { + model: "protected-model".to_string(), + }), + ); +} + +#[tokio::test] +async fn environment_only_update_revalidates_existing_step_settings() { + let mut configuration = make_session_configuration_for_tests().await; + let profile = PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy { + kind: FileSystemSandboxKind::Restricted, + glob_scan_max_depth: None, + entries: vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Write, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + FileSystemAccessMode::Read, + ), + ], + }, + NetworkSandboxPolicy::Restricted, + ); + configuration.permission_profile_state = + PermissionProfileState::from_constrained_legacy(Constrained::allow_any(profile)).unwrap(); + let settings = Arc::make_mut(&mut configuration.step_settings); + settings.collaboration_mode = settings.collaboration_mode.with_updates( + Some("protected-model".to_string()), + /*effort*/ None, + /*developer_instructions*/ None, + ); + settings.approvals_reviewer = ApprovalsReviewer::AutoReview; + Arc::make_mut(&mut configuration.original_config_do_not_use) + .features + .enable(Feature::GuardianApproval) + .unwrap(); + let mut requirements = configuration + .original_config_do_not_use + .config_layer_stack + .requirements() + .clone(); + requirements.auto_review_required_models = Some(Sourced::new( + BTreeSet::from(["protected-model".to_string()]), + RequirementSource::Unknown, + )); + set_requirements(&mut configuration, requirements); + + let environments = vec![local(configuration.cwd().clone())]; + assert_eq!(configuration.validate(&environments), Ok(())); + // Removing the selected workspace also removes its read-only carveout, + // leaving this profile with full-disk write access. + assert_eq!( + configuration + .apply( + &SessionSettingsUpdate { + environments: Some(TurnEnvironmentSelections::new( + configuration.cwd().clone(), + Vec::new(), + )), + ..Default::default() + }, + &environments, + ) + .err(), + Some(ConstraintError::AutoReviewRequired { + model: "protected-model".to_string(), + }), + ); +} diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index c0299fdaf6..8f29343d14 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1,4 +1,6 @@ use super::mcp_refresh::McpRefresh; +use super::step_settings::StepSettings; +use super::step_settings::StepSettingsUpdate; use super::turn_context::TurnEnvironment; use super::*; use crate::agents_md_manager::AgentsMdManager; @@ -3290,7 +3292,10 @@ async fn config_change_contributor_observes_effective_config_changes() { ); session .update_settings(SessionSettingsUpdate { - collaboration_mode: Some(collaboration_mode), + step_settings: StepSettingsUpdate { + collaboration_mode: Some(collaboration_mode), + ..Default::default() + }, ..Default::default() }) .await @@ -4327,17 +4332,20 @@ async fn set_rate_limits_retains_previous_credits() { }; let session_configuration = SessionConfiguration { provider: create_model_provider(config.model_provider.clone(), /*auth_manager*/ None), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -4440,17 +4448,20 @@ async fn set_rate_limits_updates_plan_type_when_present() { }; let session_configuration = SessionConfiguration { provider: create_model_provider(config.model_provider.clone(), /*auth_manager*/ None), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -4946,7 +4957,10 @@ async fn session_settings_null_service_tier_update_uses_default_service_tier() { let updated = session_configuration .apply( &SessionSettingsUpdate { - service_tier: Some(None), + step_settings: StepSettingsUpdate { + service_tier: Some(None), + ..Default::default() + }, ..Default::default() }, &[], @@ -4954,7 +4968,7 @@ async fn session_settings_null_service_tier_update_uses_default_service_tier() { .expect("null service tier update should apply"); assert_eq!( - updated.service_tier, + updated.step_settings.service_tier, Some(SERVICE_TIER_DEFAULT_REQUEST_VALUE.to_string()) ); } @@ -4966,7 +4980,10 @@ async fn session_settings_legacy_fast_service_tier_update_uses_priority_request_ let updated = session_configuration .apply( &SessionSettingsUpdate { - service_tier: Some(Some("fast".to_string())), + step_settings: StepSettingsUpdate { + service_tier: Some(Some("fast".to_string())), + ..Default::default() + }, ..Default::default() }, &[], @@ -4974,7 +4991,7 @@ async fn session_settings_legacy_fast_service_tier_update_uses_priority_request_ .expect("legacy fast service tier update should apply"); assert_eq!( - updated.service_tier, + updated.step_settings.service_tier, Some(ServiceTier::Fast.request_value().to_string()) ); } @@ -4998,17 +5015,20 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati SessionConfiguration { provider: create_model_provider(config.model_provider.clone(), /*auth_manager*/ None), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -5589,8 +5609,11 @@ async fn session_settings_commit_keeps_snapshot_across_postcommit_wait() { .expect("network refresh lock"); let mut first_update = Box::pin(tokio::task::unconstrained(session.update_settings( SessionSettingsUpdate { + step_settings: StepSettingsUpdate { + service_tier: Some(Some(ServiceTier::Fast.request_value().to_string())), + ..Default::default() + }, permission_profile: Some(PermissionProfile::read_only()), - service_tier: Some(Some(ServiceTier::Fast.request_value().to_string())), ..Default::default() }, ))); @@ -5603,7 +5626,10 @@ async fn session_settings_commit_keeps_snapshot_across_postcommit_wait() { let expected = session.thread_settings_snapshot().await; let later_commit = session .update_settings(SessionSettingsUpdate { - service_tier: Some(None), + step_settings: StepSettingsUpdate { + service_tier: Some(None), + ..Default::default() + }, ..Default::default() }) .await @@ -5838,17 +5864,20 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { config.model_provider.clone(), Some(Arc::clone(&auth_manager)), ), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -5988,17 +6017,20 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { config.model_provider.clone(), Some(Arc::clone(&auth_manager)), ), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -6278,17 +6310,20 @@ async fn make_session_with_config_and_rx( config.model_provider.clone(), Some(Arc::clone(&auth_manager)), ), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -6401,17 +6436,20 @@ async fn make_session_with_history_source_and_agent_control_and_rx( config.model_provider.clone(), Some(Arc::clone(&auth_manager)), ), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -8205,17 +8243,20 @@ where config.model_provider.clone(), Some(Arc::clone(&auth_manager)), ), - collaboration_mode, - model_reasoning_summary: config.model_reasoning_summary, + step_settings: Arc::new(StepSettings { + collaboration_mode, + reasoning_summary: config.model_reasoning_summary, + service_tier: None, + personality: config.personality, + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + }), + model_info_overrides: config.to_models_manager_config().into(), developer_instructions: config.developer_instructions.clone(), - service_tier: None, - personality: config.personality, base_instructions: config .base_instructions .clone() .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), - approval_policy: config.permissions.approval_policy.clone(), - approvals_reviewer: config.approvals_reviewer, permission_profile_state: config.permissions.permission_profile_state().clone(), allow_login_shell: config.permissions.allow_login_shell, shell_environment_policy: config.permissions.shell_environment_policy.clone(), @@ -8574,8 +8615,11 @@ async fn refreshed_mcp_binding_captures_current_approval_authority() { session .update_settings(SessionSettingsUpdate { - approval_policy: Some(AskForApproval::Never), - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + step_settings: StepSettingsUpdate { + approval_policy: Some(AskForApproval::Never), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + ..Default::default() + }, permission_profile: Some(PermissionProfile::Disabled), ..Default::default() }) @@ -8645,7 +8689,10 @@ async fn mcp_elicitation_reviewer_uses_latest_runtime_authority() { session .update_settings(SessionSettingsUpdate { - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + step_settings: StepSettingsUpdate { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + ..Default::default() + }, ..Default::default() }) .await @@ -8687,7 +8734,10 @@ async fn mcp_elicitation_reviewer_uses_latest_runtime_authority() { session .update_settings(SessionSettingsUpdate { - approval_policy: Some(AskForApproval::Never), + step_settings: StepSettingsUpdate { + approval_policy: Some(AskForApproval::Never), + ..Default::default() + }, ..Default::default() }) .await @@ -8781,7 +8831,10 @@ async fn mcp_policy_changes_schedule_runtime_refresh() { .new_turn_with_sub_id( "policy-change".to_string(), SessionSettingsUpdate { - approval_policy: Some(AskForApproval::Never), + step_settings: StepSettingsUpdate { + approval_policy: Some(AskForApproval::Never), + ..Default::default() + }, ..Default::default() }, ) diff --git a/codex-rs/core/src/session/thread_settings.rs b/codex-rs/core/src/session/thread_settings.rs index 66f2c3e424..181ab427d7 100644 --- a/codex-rs/core/src/session/thread_settings.rs +++ b/codex-rs/core/src/session/thread_settings.rs @@ -3,6 +3,7 @@ use super::session::Session; use super::session::SessionSettingsUpdate; +use super::step_settings::StepSettingsUpdate; use crate::config::ConstraintResult; use codex_protocol::protocol::CodexErrorInfo; use codex_protocol::protocol::ErrorEvent; @@ -63,23 +64,26 @@ pub(super) async fn prepare_update( // partial thread-settings updates refresh those fields on the active mode. state .session_configuration + .step_settings .collaboration_mode .with_updates(model, effort, /*developer_instructions*/ None) } }; SessionSettingsUpdate { + step_settings: StepSettingsUpdate { + collaboration_mode: Some(collaboration_mode), + reasoning_summary: summary, + service_tier, + personality, + approval_policy, + approvals_reviewer, + }, environments, profile_workspace_roots, - approval_policy, - approvals_reviewer, sandbox_policy, permission_profile, active_permission_profile, windows_sandbox_level, - collaboration_mode: Some(collaboration_mode), - reasoning_summary: summary, - service_tier, - personality, ..Default::default() } } diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 1b815875a4..ba506f2dc2 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -572,18 +572,22 @@ impl Session { let config = session_configuration.original_config_do_not_use.clone(); let mut per_turn_config = (*config).clone(); per_turn_config.cwd = cwd; - per_turn_config.permissions.approval_policy = session_configuration.approval_policy.clone(); + per_turn_config.permissions.approval_policy = + session_configuration.step_settings.approval_policy.clone(); let workspace_roots = self.services.turn_environments.primary_workspace_roots(); per_turn_config.workspace_roots = workspace_roots.clone(); per_turn_config .permissions .set_workspace_roots(workspace_roots); - per_turn_config.model_reasoning_effort = - session_configuration.collaboration_mode.reasoning_effort(); - per_turn_config.model_reasoning_summary = session_configuration.model_reasoning_summary; - per_turn_config.service_tier = session_configuration.service_tier.clone(); - per_turn_config.personality = session_configuration.personality; - per_turn_config.approvals_reviewer = session_configuration.approvals_reviewer; + per_turn_config.model_reasoning_effort = session_configuration + .step_settings + .collaboration_mode + .reasoning_effort(); + per_turn_config.model_reasoning_summary = + session_configuration.step_settings.reasoning_summary; + per_turn_config.service_tier = session_configuration.step_settings.service_tier.clone(); + per_turn_config.personality = session_configuration.step_settings.personality; + per_turn_config.approvals_reviewer = session_configuration.step_settings.approvals_reviewer; session_configuration .apply_permission_profile_to_permissions(&mut per_turn_config.permissions); let permission_profile = session_configuration.permission_profile(); @@ -614,7 +618,13 @@ impl Session { ) -> Config { let mut config = self.build_per_turn_config(session_configuration, session_configuration.cwd().clone()); - config.model = Some(session_configuration.collaboration_mode.model().to_string()); + config.model = Some( + session_configuration + .step_settings + .collaboration_mode + .model() + .to_string(), + ); config } @@ -640,13 +650,17 @@ impl Session { sub_id: String, skills_snapshot: HostSkillsSnapshot, ) -> TurnContext { - let collaboration_mode = &session_configuration.collaboration_mode; + let collaboration_mode = &session_configuration.step_settings.collaboration_mode; let reasoning_effort = collaboration_mode.reasoning_effort(); let reasoning_summary = session_configuration - .model_reasoning_summary + .step_settings + .reasoning_summary .unwrap_or(model_info.default_reasoning_summary); let session_telemetry = session_telemetry.clone().with_model( - session_configuration.collaboration_mode.model(), + session_configuration + .step_settings + .collaboration_mode + .model(), model_info.slug.as_str(), ); let session_source = session_configuration.session_source.clone(); @@ -723,7 +737,7 @@ impl Session { .developer_instructions .clone(), multi_agent_version, - personality: session_configuration.personality, + personality: session_configuration.step_settings.personality, network, windows_sandbox_level: session_configuration.windows_sandbox_level, available_models, @@ -768,7 +782,7 @@ impl Session { let mut configuration = commit.configuration; // Apply the override only to the turn's copy, after persisting thread settings. if let Some(service_tier) = service_tier_for_turn { - configuration.service_tier = Some(service_tier); + Arc::make_mut(&mut configuration.step_settings).service_tier = Some(service_tier); } let turn_context = self .new_turn_from_configuration(sub_id, configuration, final_output_json_schema) @@ -833,8 +847,16 @@ impl Session { .services .models_manager .get_model_info( - session_configuration.collaboration_mode.model(), - &per_turn_config.to_models_manager_config(), + session_configuration + .step_settings + .collaboration_mode + .model(), + &session_configuration + .model_info_overrides + .models_manager_config( + session_configuration.step_settings.personality, + self.features.enabled(Feature::Personality), + ), ) .await; self.services diff --git a/codex-rs/core/src/session/turn_input.rs b/codex-rs/core/src/session/turn_input.rs index 3e49c20b4a..704af6c1ff 100644 --- a/codex-rs/core/src/session/turn_input.rs +++ b/codex-rs/core/src/session/turn_input.rs @@ -85,7 +85,7 @@ impl PreparedTurnInputSettings { fn would_enter_plan_mode(&self) -> bool { self.thread_settings_update .as_ref() - .and_then(|updates| updates.collaboration_mode.as_ref()) + .and_then(|updates| updates.step_settings.collaboration_mode.as_ref()) .is_some_and(|collaboration_mode| collaboration_mode.mode == ModeKind::Plan) } diff --git a/codex-rs/core/src/session/turn_input_tests.rs b/codex-rs/core/src/session/turn_input_tests.rs index b1eb5f07fe..b16d8be2d6 100644 --- a/codex-rs/core/src/session/turn_input_tests.rs +++ b/codex-rs/core/src/session/turn_input_tests.rs @@ -133,7 +133,7 @@ async fn accepted_input_applies_thread_settings() { let state = session.state.lock().await; assert_eq!( - state.session_configuration.approvals_reviewer, + state.session_configuration.step_settings.approvals_reviewer, codex_config::types::ApprovalsReviewer::AutoReview ); assert!( @@ -240,7 +240,8 @@ async fn start_only_rejects_plan_mode_without_injecting() { collaboration_mode.mode = ModeKind::Plan; { let mut state = session.state.lock().await; - state.session_configuration.collaboration_mode = collaboration_mode; + Arc::make_mut(&mut state.session_configuration.step_settings).collaboration_mode = + collaboration_mode; } let submission = submit_start_only( @@ -272,7 +273,8 @@ async fn start_only_accepts_user_input_in_plan_mode() { collaboration_mode.mode = ModeKind::Plan; { let mut state = session.state.lock().await; - state.session_configuration.collaboration_mode = collaboration_mode; + Arc::make_mut(&mut state.session_configuration.step_settings).collaboration_mode = + collaboration_mode; state.merge_connector_selection(["calendar".to_string()]); } @@ -307,7 +309,8 @@ async fn start_only_rejects_empty_user_input_in_plan_mode() { collaboration_mode.mode = ModeKind::Plan; { let mut state = session.state.lock().await; - state.session_configuration.collaboration_mode = collaboration_mode; + Arc::make_mut(&mut state.session_configuration.step_settings).collaboration_mode = + collaboration_mode; } let submission = submit_start_only( diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index fec77eaef7..ba0f21285b 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -146,6 +146,7 @@ mod safety_check_downgrade; mod search_tool; mod send_user_message_async; mod settings_commits; +mod settings_constraints; mod shell_snapshot; mod skill_approval; mod skills; diff --git a/codex-rs/core/tests/suite/settings_constraints.rs b/codex-rs/core/tests/suite/settings_constraints.rs new file mode 100644 index 0000000000..ecdca6461c --- /dev/null +++ b/codex-rs/core/tests/suite/settings_constraints.rs @@ -0,0 +1,217 @@ +use anyhow::Result; +use codex_config::test_support::CloudConfigBundleFixture; +use codex_core::CodexThread; +use codex_core::TurnInputRequest; +use codex_core::TurnInputSubmission; +use codex_core::config::Constrained; +use codex_features::Feature; +use codex_models_manager::model_info::model_info_from_slug; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::models::PermissionProfile; +use codex_protocol::openai_models::ApprovalMessages; +use codex_protocol::openai_models::ModelsResponse; +use codex_protocol::openai_models::PermissionMessages; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::CodexErrorInfo; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::protocol::ThreadSettingsSnapshot; +use codex_protocol::user_input::UserInput; +use core_test_support::responses; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use std::time::Duration; +use test_case::test_case; +use tokio::time::timeout; + +const INITIAL_MODEL: &str = "gpt-5.4"; +const PROTECTED_MODEL: &str = "gpt-5.2"; +const REVIEW_INSTRUCTIONS: &str = "Protected-model automatic review instructions."; +const PERMISSION_INSTRUCTIONS: &str = "Protected-model read-only instructions."; + +#[derive(Clone, Copy)] +enum SettingsOperation { + Standalone, + TurnStart, +} + +impl SettingsOperation { + async fn submit( + self, + codex: &CodexThread, + thread_settings: ThreadSettingsOverrides, + ) -> Result { + let id = match self { + Self::Standalone => codex.submit(Op::ThreadSettings { thread_settings }).await?, + Self::TurnStart => { + let result = codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "use the protected model".to_string(), + text_elements: Vec::new(), + }]) + .with_thread_settings(thread_settings), + ) + .await?; + let TurnInputSubmission::Started { turn_id } = result else { + anyhow::bail!("expected a new turn, got {result:?}"); + }; + turn_id + } + }; + timeout(Duration::from_secs(10), async { + loop { + let event = codex.next_event().await?; + if event.id != id { + continue; + } + match event.msg { + EventMsg::ThreadSettingsApplied(applied) => return Ok(applied.thread_settings), + EventMsg::Error(error) => { + assert_eq!(error.codex_error_info, Some(CodexErrorInfo::BadRequest)); + anyhow::bail!(error.message); + } + _ => {} + } + } + }) + .await? + } +} + +#[test_case(SettingsOperation::Standalone; "standalone settings")] +#[test_case(SettingsOperation::TurnStart; "turn-start settings")] +#[tokio::test] +async fn protected_model_settings_use_the_proposed_permissions( + operation: SettingsOperation, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let response = responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_response_created("response"), + responses::ev_completed("response"), + ]), + ) + .await; + let mut protected = model_info_from_slug(PROTECTED_MODEL); + let messages = protected.model_messages.as_mut().expect("model messages"); + messages.approvals = Some(ApprovalMessages { + on_request: None, + on_request_auto_review: Some(REVIEW_INSTRUCTIONS.to_string()), + never: None, + unless_trusted: None, + }); + messages.permissions = Some(PermissionMessages { + danger_full_access: None, + workspace_write: None, + read_only: Some(PERMISSION_INSTRUCTIONS.to_string()), + }); + let test = test_codex() + .with_model(INITIAL_MODEL) + .with_cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement( + "[auto_review]\nrequired_on_models = [\"gpt-5.2\"]\n", + ), + ) + .with_config(move |config| { + config.model_catalog = Some(ModelsResponse { + models: vec![model_info_from_slug(INITIAL_MODEL), protected], + }); + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::User; + config + .permissions + .set_permission_profile(PermissionProfile::Disabled) + .expect("full-access profile should be allowed"); + config + .features + .enable(Feature::GuardianApproval) + .expect("test config should allow Guardian"); + }) + .build_with_auto_env(&server) + .await?; + let initial = test.codex.thread_settings_snapshot().await; + assert_eq!(initial.permission_profile, PermissionProfile::Disabled); + let model_update = ThreadSettingsOverrides { + model: Some(PROTECTED_MODEL.to_string()), + ..Default::default() + }; + + let error = operation + .submit(&test.codex, model_update.clone()) + .await + .expect_err("the protected model requires restricted permissions"); + assert!( + error.to_string().contains("you need to use auto review"), + "{error}" + ); + assert_eq!(test.codex.thread_settings_snapshot().await, initial); + assert!(response.requests().is_empty()); + + let expected = ThreadSettingsSnapshot { + model: PROTECTED_MODEL.to_string(), + collaboration_mode: initial.collaboration_mode.with_updates( + Some(PROTECTED_MODEL.to_string()), + /*effort*/ None, + /*developer_instructions*/ None, + ), + approvals_reviewer: ApprovalsReviewer::AutoReview, + permission_profile: PermissionProfile::read_only(), + active_permission_profile: None, + ..initial + }; + let applied = operation + .submit( + &test.codex, + ThreadSettingsOverrides { + permission_profile: Some(PermissionProfile::read_only()), + ..model_update + }, + ) + .await?; + assert_eq!(applied, expected); + assert_eq!(test.codex.thread_settings_snapshot().await, expected); + if let SettingsOperation::TurnStart = operation { + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + } + + // Even an update with no step-settings edits must revalidate the existing + // protected model against the proposed permissions. + let error = operation + .submit( + &test.codex, + ThreadSettingsOverrides { + permission_profile: Some(PermissionProfile::Disabled), + ..Default::default() + }, + ) + .await + .expect_err("full access must invalidate the protected model settings"); + assert!( + error.to_string().contains("you need to use auto review"), + "{error}" + ); + assert_eq!(test.codex.thread_settings_snapshot().await, expected); + + if let SettingsOperation::Standalone = operation { + test.submit_text_turn("use the committed settings").await?; + } + let request = response.single_request(); + assert_eq!(request.body_json()["model"], PROTECTED_MODEL); + let instructions = request.message_input_texts("developer").join("\n"); + assert!(instructions.contains(REVIEW_INSTRUCTIONS), "{instructions}"); + assert!( + instructions.contains(PERMISSION_INSTRUCTIONS), + "{instructions}" + ); + Ok(()) +}