Skip to content

feat: Constrain values for approval_policy #7778

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gt-oai wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Constrain values for approval_policy #7778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codex-rs/common/src/config_summary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub fn create_config_summary_entries(config: &Config) -> Vec<(&'static str, Stri
("workdir", config.cwd.display().to_string()),
("model", config.model.clone()),
("provider", config.model_provider_id.clone()),
("approval", config.approval_policy.to_string()),
("approval", config.approval_policy.value().to_string()),
("sandbox", summarize_sandbox_policy(&config.sandbox_policy)),
];
if config.model_provider.wire_api == WireApi::Responses {
Expand Down
151 changes: 109 additions & 42 deletions codex-rs/core/src/codex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ use crate::client_common::Prompt;
use crate::client_common::ResponseEvent;
use crate::compact::collect_user_messages;
use crate::config::Config;
use crate::config::Constrained;
use crate::config::ConstraintError;
use crate::config::ConstraintResult;
use crate::config::types::ShellEnvironmentPolicy;
use crate::context_manager::ContextManager;
use crate::environment_context::EnvironmentContext;
Expand All @@ -87,6 +90,7 @@ use crate::protocol::ApplyPatchApprovalRequestEvent;
use crate::protocol::AskForApproval;
use crate::protocol::BackgroundEventEvent;
use crate::protocol::DeprecationNoticeEvent;
use crate::protocol::ErrorEvent;
use crate::protocol::Event;
use crate::protocol::EventMsg;
use crate::protocol::ExecApprovalRequestEvent;
Expand Down Expand Up @@ -191,7 +195,7 @@ impl Codex {
user_instructions,
base_instructions: config.base_instructions.clone(),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.approval_policy,
approval_policy: config.approval_policy.clone(),
Copy link
Collaborator

@jif-oai jif-oai Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent this we could implement the copy Trait... not 100% sure this is what we want to do but I guess the validator will always be pretty small

Copy link
Author

@gt-oai gt-oai Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would we be trying to prevent here?

sandbox_policy: config.sandbox_policy.clone(),
cwd: config.cwd.clone(),
original_config_do_not_use: Arc::clone(&config),
Expand Down Expand Up @@ -339,7 +343,7 @@ pub(crate) struct SessionConfiguration {
compact_prompt: Option<String>,

/// When to escalate for approval for execution
approval_policy: AskForApproval,
approval_policy: Constrained<AskForApproval>,
/// How to sandbox commands executed in the system
sandbox_policy: SandboxPolicy,

Expand All @@ -362,7 +366,7 @@ pub(crate) struct SessionConfiguration {
}

impl SessionConfiguration {
pub(crate) fn apply(&self, updates: &SessionSettingsUpdate) -> Self {
pub(crate) fn apply(&self, updates: &SessionSettingsUpdate) -> ConstraintResult<Self> {
let mut next_configuration = self.clone();
if let Some(model) = updates.model.clone() {
next_configuration.model = model;
Expand All @@ -374,15 +378,15 @@ impl SessionConfiguration {
next_configuration.model_reasoning_summary = summary;
}
if let Some(approval_policy) = updates.approval_policy {
next_configuration.approval_policy = approval_policy;
next_configuration.approval_policy.set(approval_policy)?;
}
if let Some(sandbox_policy) = updates.sandbox_policy.clone() {
next_configuration.sandbox_policy = sandbox_policy;
}
if let Some(cwd) = updates.cwd.clone() {
next_configuration.cwd = cwd;
}
next_configuration
Ok(next_configuration)
}
}

Expand Down Expand Up @@ -450,7 +454,7 @@ impl Session {
base_instructions: session_configuration.base_instructions.clone(),
compact_prompt: session_configuration.compact_prompt.clone(),
user_instructions: session_configuration.user_instructions.clone(),
approval_policy: session_configuration.approval_policy,
approval_policy: session_configuration.approval_policy.value(),
sandbox_policy: session_configuration.sandbox_policy.clone(),
shell_environment_policy: per_turn_config.shell_environment_policy.clone(),
tools_config,
Expand Down Expand Up @@ -566,7 +570,7 @@ impl Session {
config.model_reasoning_summary,
config.model_context_window,
config.model_auto_compact_token_limit,
config.approval_policy,
config.approval_policy.value(),
config.sandbox_policy.clone(),
config.mcp_servers.keys().map(String::as_str).collect(),
config.active_profile.clone(),
Expand Down Expand Up @@ -609,7 +613,7 @@ impl Session {
session_id: conversation_id,
model: session_configuration.model.clone(),
model_provider_id: config.model_provider_id.clone(),
approval_policy: session_configuration.approval_policy,
approval_policy: session_configuration.approval_policy.value(),
sandbox_policy: session_configuration.sandbox_policy.clone(),
cwd: session_configuration.cwd.clone(),
reasoning_effort: session_configuration.model_reasoning_effort,
Expand Down Expand Up @@ -688,7 +692,7 @@ impl Session {
}

async fn record_initial_history(&self, conversation_history: InitialHistory) {
let turn_context = self.new_turn(SessionSettingsUpdate::default()).await;
let turn_context = self.new_default_turn().await;
match conversation_history {
InitialHistory::New => {
// Build and record initial items (user instructions + environment context)
Expand Down Expand Up @@ -747,29 +751,71 @@ impl Session {
}
}

pub(crate) async fn update_settings(&self, updates: SessionSettingsUpdate) {
pub(crate) async fn update_settings(
&self,
updates: SessionSettingsUpdate,
) -> ConstraintResult<()> {
let mut state = self.state.lock().await;

state.session_configuration = state.session_configuration.apply(&updates);
}

pub(crate) async fn new_turn(&self, updates: SessionSettingsUpdate) -> Arc<TurnContext> {
let sub_id = self.next_internal_sub_id();
self.new_turn_with_sub_id(sub_id, updates).await
match state.session_configuration.apply(&updates) {
Ok(updated) => {
state.session_configuration = updated;
Ok(())
}
Err(err) => {
let wrapped = ConstraintError {
message: format!("Could not update config: {err}"),
};
warn!(%wrapped, "rejected session settings update");
Err(wrapped)
}
}
}

pub(crate) async fn new_turn_with_sub_id(
&self,
sub_id: String,
updates: SessionSettingsUpdate,
) -> Arc<TurnContext> {
) -> ConstraintResult<Arc<TurnContext>> {
let session_configuration = {
let mut state = self.state.lock().await;
let session_configuration = state.session_configuration.clone().apply(&updates);
state.session_configuration = session_configuration.clone();
session_configuration
match state.session_configuration.clone().apply(&updates) {
Ok(next) => {
state.session_configuration = next.clone();
next
}
Err(err) => {
let wrapped = ConstraintError {
message: format!("Could not update config: {err}"),
};
self.send_event_raw(Event {
id: sub_id.clone(),
msg: EventMsg::Error(ErrorEvent {
message: wrapped.to_string(),
codex_error_info: Some(CodexErrorInfo::BadRequest),
}),
})
.await;
return Err(wrapped);
}
}
};

Ok(self
.new_turn_from_configuration(
sub_id,
session_configuration,
updates.final_output_json_schema,
)
.await)
}

async fn new_turn_from_configuration(
&self,
sub_id: String,
session_configuration: SessionConfiguration,
final_output_json_schema: Option<Option<Value>>,
) -> Arc<TurnContext> {
let per_turn_config = Self::build_per_turn_config(&session_configuration);
let model_family = self
.services
Expand All @@ -786,12 +832,26 @@ impl Session {
self.conversation_id,
sub_id,
);
if let Some(final_schema) = updates.final_output_json_schema {
if let Some(final_schema) = final_output_json_schema {
turn_context.final_output_json_schema = final_schema;
}
Arc::new(turn_context)
}

pub(crate) async fn new_default_turn(&self) -> Arc<TurnContext> {
self.new_default_turn_with_sub_id(self.next_internal_sub_id())
.await
}

pub(crate) async fn new_default_turn_with_sub_id(&self, sub_id: String) -> Arc<TurnContext> {
let session_configuration = {
let state = self.state.lock().await;
state.session_configuration.clone()
};
self.new_turn_from_configuration(sub_id, session_configuration, None)
.await
}

fn build_environment_update_item(
&self,
previous: Option<&Arc<TurnContext>>,
Expand Down Expand Up @@ -1462,8 +1522,7 @@ impl Session {

async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiver<Submission>) {
// Seed with context in case there is an OverrideTurnContext first.
let mut previous_context: Option<Arc<TurnContext>> =
Some(sess.new_turn(SessionSettingsUpdate::default()).await);
let mut previous_context: Option<Arc<TurnContext>> = Some(sess.new_default_turn().await);

if config.features.enabled(Feature::RemoteModels)
&& let Err(err) = sess
Expand Down Expand Up @@ -1492,6 +1551,7 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
} => {
handlers::override_turn_context(
&sess,
sub.id.clone(),
SessionSettingsUpdate {
cwd,
approval_policy,
Expand Down Expand Up @@ -1602,8 +1662,21 @@ mod handlers {
sess.interrupt_task().await;
}

pub async fn override_turn_context(sess: &Session, updates: SessionSettingsUpdate) {
sess.update_settings(updates).await;
pub async fn override_turn_context(
sess: &Session,
sub_id: String,
updates: SessionSettingsUpdate,
) {
if let Err(err) = sess.update_settings(updates).await {
sess.send_event_raw(Event {
id: sub_id,
msg: EventMsg::Error(ErrorEvent {
message: err.to_string(),
codex_error_info: Some(CodexErrorInfo::BadRequest),
}),
})
.await;
}
}

pub async fn user_input_or_turn(
Expand Down Expand Up @@ -1638,7 +1711,9 @@ mod handlers {
_ => unreachable!(),
};

let current_context = sess.new_turn_with_sub_id(sub_id, updates).await;
let Ok(current_context) = sess.new_turn_with_sub_id(sub_id, updates).await else {
return;
};
current_context
.client
.get_otel_event_manager()
Expand All @@ -1665,9 +1740,7 @@ mod handlers {
command: String,
previous_context: &mut Option<Arc<TurnContext>>,
) {
let turn_context = sess
.new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default())
.await;
let turn_context = sess.new_default_turn_with_sub_id(sub_id).await;
sess.spawn_task(
Arc::clone(&turn_context),
Vec::new(),
Expand Down Expand Up @@ -1822,17 +1895,13 @@ mod handlers {
}

pub async fn undo(sess: &Arc<Session>, sub_id: String) {
let turn_context = sess
.new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default())
.await;
let turn_context = sess.new_default_turn_with_sub_id(sub_id).await;
sess.spawn_task(turn_context, Vec::new(), UndoTask::new())
.await;
}

pub async fn compact(sess: &Arc<Session>, sub_id: String) {
let turn_context = sess
.new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default())
.await;
let turn_context = sess.new_default_turn_with_sub_id(sub_id).await;

sess.spawn_task(
Arc::clone(&turn_context),
Expand Down Expand Up @@ -1886,9 +1955,7 @@ mod handlers {
sub_id: String,
review_request: ReviewRequest,
) {
let turn_context = sess
.new_turn_with_sub_id(sub_id.clone(), SessionSettingsUpdate::default())
.await;
let turn_context = sess.new_default_turn_with_sub_id(sub_id.clone()).await;
match resolve_review_request(review_request, config.cwd.as_path()) {
Ok(resolved) => {
spawn_review_thread(
Expand Down Expand Up @@ -2595,7 +2662,7 @@ mod tests {
user_instructions: config.user_instructions.clone(),
base_instructions: config.base_instructions.clone(),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.approval_policy,
approval_policy: config.approval_policy.clone(),
sandbox_policy: config.sandbox_policy.clone(),
cwd: config.cwd.clone(),
original_config_do_not_use: Arc::clone(&config),
Expand Down Expand Up @@ -2666,7 +2733,7 @@ mod tests {
user_instructions: config.user_instructions.clone(),
base_instructions: config.base_instructions.clone(),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.approval_policy,
approval_policy: config.approval_policy.clone(),
sandbox_policy: config.sandbox_policy.clone(),
cwd: config.cwd.clone(),
original_config_do_not_use: Arc::clone(&config),
Expand Down Expand Up @@ -2867,7 +2934,7 @@ mod tests {
user_instructions: config.user_instructions.clone(),
base_instructions: config.base_instructions.clone(),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.approval_policy,
approval_policy: config.approval_policy.clone(),
sandbox_policy: config.sandbox_policy.clone(),
cwd: config.cwd.clone(),
original_config_do_not_use: Arc::clone(&config),
Expand Down Expand Up @@ -2949,7 +3016,7 @@ mod tests {
user_instructions: config.user_instructions.clone(),
base_instructions: config.base_instructions.clone(),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.approval_policy,
approval_policy: config.approval_policy.clone(),
sandbox_policy: config.sandbox_policy.clone(),
cwd: config.cwd.clone(),
original_config_do_not_use: Arc::clone(&config),
Expand Down
Loading