Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions codex-rs/core/src/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ pub(crate) use legacy_unified_exec_process_limit_warning::LegacyUnifiedExecProce
pub(crate) use model_switch_instructions::ModelSwitchInstructions;
pub(crate) use multi_agent_mode_instructions::MultiAgentModeInstructions;
pub(crate) use network_rule_saved::NetworkRuleSaved;
pub use permissions_instructions::ApprovalPromptContext;
pub use permissions_instructions::PermissionsInstructions;
pub(crate) use personality_spec_instructions::PersonalitySpecInstructions;
pub(crate) use plugin_instructions::PluginInstructions;
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/context/permissions_instructions.rs
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
pub use codex_prompts::ApprovalPromptContext;
pub use codex_prompts::PermissionsInstructions;
10 changes: 9 additions & 1 deletion codex-rs/core/src/context_manager/updates.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::context::ApprovalPromptContext;
use crate::context::CollaborationModeInstructions;
use crate::context::ContextualUserFragment;
use crate::context::ModelSwitchInstructions;
Expand Down Expand Up @@ -30,6 +31,7 @@ fn build_permissions_update_item(
let prev = previous?;
if prev.permission_profile() == next.permission_profile()
&& prev.approval_policy == next.approval_policy.value()
&& prev.model == next.model_info.slug
{
Comment on lines 32 to 35

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Refresh approvals when same-slug catalog text changes

When the model catalog refreshes in-session for the same model slug, this early return treats permissions as unchanged even if the model's catalog approval text changed. The next turn then persists the new TurnContextItem without appending the new approval guidance, so the updated same-slug catalog message never becomes model-visible until some unrelated model/profile/policy change occurs; include a persisted approval-message version/hash, or compare a catalog compatibility hash if it covers these messages.

AGENTS.md reference: AGENTS.md:L91-L100

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sayan-oai i'll defer to you here for world state work

return None;
}
Expand All @@ -38,7 +40,13 @@ fn build_permissions_update_item(
PermissionsInstructions::from_permission_profile(
&next.permission_profile,
next.approval_policy.value(),
next.config.approvals_reviewer,
ApprovalPromptContext::new(
next.config.approvals_reviewer,
next.model_info
.model_messages
.as_ref()
.and_then(|messages| messages.approvals.as_ref()),
),
exec_policy,
#[allow(deprecated)]
&next.cwd,
Expand Down
10 changes: 9 additions & 1 deletion codex-rs/core/src/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use crate::build_available_skills;
use crate::compact;
use crate::config::ManagedFeatures;
use crate::config::resolve_tool_suggest_config_from_layer_stack;
use crate::context::ApprovalPromptContext;
use crate::context::ApprovedCommandPrefixSaved;
use crate::context::AvailableSkillsInstructions;
use crate::context::CollaborationModeInstructions;
Expand Down Expand Up @@ -3176,7 +3177,14 @@ impl Session {
PermissionsInstructions::from_permission_profile(
&turn_context.permission_profile,
turn_context.approval_policy.value(),
turn_context.config.approvals_reviewer,
ApprovalPromptContext::new(
turn_context.config.approvals_reviewer,
turn_context
.model_info
.model_messages
.as_ref()
.and_then(|messages| messages.approvals.as_ref()),
),
self.services.exec_policy.current().as_ref(),
#[allow(deprecated)]
&turn_context.cwd,
Expand Down
17 changes: 6 additions & 11 deletions codex-rs/core/src/session/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9027,18 +9027,13 @@ async fn record_context_updates_and_set_reference_context_item_reinjects_full_co
#[tokio::test]
async fn record_context_updates_and_set_reference_context_item_persists_baseline_without_emitting_diffs()
{
let (mut session, previous_context) = make_session_and_context().await;
let next_model = if previous_context.model_info.slug == "gpt-5.4" {
"gpt-5.2"
} else {
"gpt-5.4"
};
let turn_context = previous_context
.with_model(next_model.to_string(), &session.services.models_manager)
.await;
let previous_context_item = previous_context.to_turn_context_item();
let previous_context = Arc::new(previous_context);
let (mut session, turn_context) = make_session_and_context().await;
let previous_context_item = turn_context.to_turn_context_item();
let previous_context = Arc::new(turn_context);
let world_state = build_world_state_from_turn_context(&session, &previous_context).await;
let mut turn_context = Arc::try_unwrap(previous_context)
.unwrap_or_else(|_| panic!("previous turn context should have no remaining references"));
turn_context.sub_id = format!("{}-next", turn_context.sub_id);
{
let mut state = session.state.lock().await;
state.set_reference_context_item(Some(previous_context_item.clone()));
Expand Down
130 changes: 129 additions & 1 deletion codex-rs/core/tests/suite/permissions_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@ use anyhow::Result;
use codex_config::ConfigLayerStack;
use codex_core::ForkSnapshot;
use codex_core::config::Constrained;
use codex_core::context::ApprovalPromptContext;
use codex_core::context::ContextualUserFragment;
use codex_core::context::PermissionsInstructions;
use codex_core::load_exec_policy;
use codex_models_manager::model_info::model_info_from_slug;
use codex_protocol::models::PermissionProfile;
use codex_protocol::openai_models::ApprovalMessages;
use codex_protocol::openai_models::ModelMessages;
use codex_protocol::openai_models::ModelsResponse;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
Expand Down Expand Up @@ -33,6 +38,129 @@ fn permissions_texts(request: &ResponsesRequest) -> Vec<String> {
.collect()
}

fn model_with_approval_messages(
slug: &str,
on_request: &str,
on_request_auto_review: &str,
) -> codex_protocol::openai_models::ModelInfo {
let mut model = model_info_from_slug(slug);
model.model_messages = Some(ModelMessages {
instructions_template: None,
instructions_variables: None,
approvals: Some(ApprovalMessages {
on_request: Some(on_request.to_string()),
on_request_auto_review: Some(on_request_auto_review.to_string()),
}),
});
model
}

async fn submit_text_turn(
test: &core_test_support::test_codex::TestCodex,
text: &str,
) -> Result<()> {
test.codex
.submit(Op::UserInput {
items: vec![UserInput::Text {
text: text.to_string(),
text_elements: Vec::new(),
}],
final_output_json_schema: None,
responsesapi_client_metadata: None,
additional_context: Default::default(),
thread_settings: Default::default(),
})
.await?;
wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn catalog_approval_message_is_sent_in_initial_permissions() -> Result<()> {
skip_if_no_network!(Ok(()));

let server = start_mock_server().await;
let req = mount_sse_once(
&server,
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
)
.await;
let model_slug = "catalog-approvals-model";
let model = model_with_approval_messages(
model_slug,
"catalog user approval instructions",
"catalog auto-review approval instructions",
);
let mut builder = test_codex()
.with_model(model_slug)
.with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
config.model_catalog = Some(ModelsResponse {
models: vec![model],
});
});
let test = builder.build(&server).await?;

submit_text_turn(&test, "hello").await?;

let permissions = permissions_texts(&req.single_request());
assert_eq!(permissions.len(), 1);
assert!(permissions[0].contains("catalog user approval instructions"));
assert!(permissions[0].contains("Filesystem sandboxing defines"));
assert!(!permissions[0].contains("How to request escalation"));
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn model_change_appends_new_catalog_approval_message() -> Result<()> {
skip_if_no_network!(Ok(()));

let server = start_mock_server().await;
let _req1 = mount_sse_once(
&server,
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
)
.await;
let req2 = mount_sse_once(
&server,
sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]),
)
.await;
let first_slug = "catalog-approvals-model-a";
let second_slug = "catalog-approvals-model-b";
let first = model_with_approval_messages(first_slug, "model A approvals", "model A auto");
let second = model_with_approval_messages(second_slug, "model B approvals", "model B auto");
let mut builder = test_codex()
.with_model(first_slug)
.with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
config.model_catalog = Some(ModelsResponse {
models: vec![first, second],
});
});
let test = builder.build(&server).await?;
submit_text_turn(&test, "first").await?;

core_test_support::submit_thread_settings(
&test.codex,
codex_protocol::protocol::ThreadSettingsOverrides {
model: Some(second_slug.to_string()),
..Default::default()
},
)
.await?;
submit_text_turn(&test, "second").await?;

let permissions = permissions_texts(&req2.single_request());
assert_eq!(permissions.len(), 2);
assert!(
permissions
.last()
.is_some_and(|text| text.contains("model B approvals"))
);
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn permissions_message_sent_once_on_start() -> Result<()> {
skip_if_no_network!(Ok(()));
Expand Down Expand Up @@ -563,7 +691,7 @@ async fn permissions_message_includes_writable_roots() -> Result<()> {
let expected = PermissionsInstructions::from_permission_profile(
&permission_profile,
AskForApproval::OnRequest,
test.config.approvals_reviewer,
ApprovalPromptContext::new(test.config.approvals_reviewer, /*messages*/ None),
&exec_policy,
test.config.cwd.as_path(),
/*exec_permission_approvals_enabled*/ false,
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/core/tests/suite/personality.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ async fn remote_model_friendly_personality_instructions_with_feature() -> anyhow
personality_friendly: Some(friendly_personality_message.to_string()),
personality_pragmatic: Some("Pragmatic variant".to_string()),
}),
approvals: None,
}),
include_skills_usage_instructions: false,
supports_reasoning_summaries: false,
Expand Down Expand Up @@ -689,6 +690,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() -
personality_friendly: Some(remote_friendly_message.to_string()),
personality_pragmatic: Some(remote_pragmatic_message.to_string()),
}),
approvals: None,
}),
include_skills_usage_instructions: false,
supports_reasoning_summaries: false,
Expand Down
15 changes: 13 additions & 2 deletions codex-rs/models-manager/src/model_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,24 @@ pub fn with_config_overrides(mut model: ModelInfo, config: &ModelsManagerConfig)

if let Some(base_instructions) = &config.base_instructions {
model.base_instructions = base_instructions.clone();
model.model_messages = None;
clear_instruction_messages(&mut model);
} else if !config.personality_enabled {
model.model_messages = None;
clear_instruction_messages(&mut model);
}

model
}

fn clear_instruction_messages(model: &mut ModelInfo) {
if let Some(model_messages) = &mut model.model_messages {
model_messages.instructions_template = None;
model_messages.instructions_variables = None;
if model_messages.approvals.is_none() {
model.model_messages = None;
}
}
}

/// Build a minimal fallback model descriptor for missing/unknown slugs.
pub fn model_info_from_slug(slug: &str) -> ModelInfo {
warn!("Unknown model {slug} is used. This will use fallback model metadata.");
Expand Down Expand Up @@ -119,6 +129,7 @@ fn local_personality_messages_for_slug(slug: &str) -> Option<ModelMessages> {
personality_friendly: Some(LOCAL_FRIENDLY_TEMPLATE.to_string()),
personality_pragmatic: Some(LOCAL_PRAGMATIC_TEMPLATE.to_string()),
}),
approvals: None,
}),
_ => None,
}
Expand Down
63 changes: 63 additions & 0 deletions codex-rs/models-manager/src/model_info_tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::*;
use crate::ModelsManagerConfig;
use codex_protocol::openai_models::ApprovalMessages;
use pretty_assertions::assert_eq;

#[test]
Expand Down Expand Up @@ -44,6 +45,68 @@ fn reasoning_summaries_override_false_is_noop_when_model_is_false() {
assert_eq!(updated, model);
}

#[test]
fn base_instruction_override_preserves_catalog_approval_messages() {
let mut model = model_info_from_slug("unknown-model");
let approvals = ApprovalMessages {
on_request: Some("user approvals".to_string()),
on_request_auto_review: Some("auto approvals".to_string()),
};
model.model_messages = Some(ModelMessages {
instructions_template: Some("template".to_string()),
instructions_variables: Some(ModelInstructionsVariables {
personality_default: Some("default".to_string()),
personality_friendly: Some("friendly".to_string()),
personality_pragmatic: Some("pragmatic".to_string()),
}),
approvals: Some(approvals.clone()),
});
let config = ModelsManagerConfig {
base_instructions: Some("override".to_string()),
..Default::default()
};

let updated = with_config_overrides(model, &config);

assert_eq!(
updated.model_messages,
Some(ModelMessages {
instructions_template: None,
instructions_variables: None,
approvals: Some(approvals),
})
);
}

#[test]
fn disabled_personality_preserves_catalog_approval_messages() {
let mut model = model_info_from_slug("unknown-model");
let approvals = ApprovalMessages {
on_request: Some("user approvals".to_string()),
on_request_auto_review: None,
};
model.model_messages = Some(ModelMessages {
instructions_template: Some("template".to_string()),
instructions_variables: None,
approvals: Some(approvals.clone()),
});
let config = ModelsManagerConfig {
personality_enabled: false,
..Default::default()
};

let updated = with_config_overrides(model, &config);

assert_eq!(
updated.model_messages,
Some(ModelMessages {
instructions_template: None,
instructions_variables: None,
approvals: Some(approvals),
})
);
}

#[test]
fn model_context_window_override_clamps_to_max_context_window() {
let mut model = model_info_from_slug("unknown-model");
Expand Down
1 change: 1 addition & 0 deletions codex-rs/prompts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ pub use compact::SUMMARY_PREFIX;
pub use goals::budget_limit_prompt;
pub use goals::continuation_prompt;
pub use goals::objective_updated_prompt;
pub use permissions_instructions::ApprovalPromptContext;
pub use permissions_instructions::PermissionsInstructions;
pub use realtime::BACKEND_PROMPT;
pub use realtime::END_INSTRUCTIONS;
Expand Down
Loading
Loading