From 1bbfb5cfada8e56280adcd397b23d0c301423894 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Mon, 3 Aug 2026 21:31:57 +0000 Subject: [PATCH] Avoid reinjecting permissions after command approvals (#36800) ## What changed - Track approved command prefixes separately from the stable permissions instructions in world-state snapshots. - Emit only newly approved prefixes after an exec-policy amendment instead of appending the full permissions block again. - Preserve prefix updates when full permissions instructions are disabled and remain compatible with legacy world-state snapshots. ## Testing - Cover incremental prefix additions, removals, legacy snapshots, history rollback, and approval flows with permissions instructions enabled or disabled. GitOrigin-RevId: e6f68c6a91be82750e70e28456f2b7c58607fbd8 --- .../context/approved_command_prefix_saved.rs | 8 +- codex-rs/core/src/context/mod.rs | 1 + .../world_state/compact_permissions.rs | 59 +++++++ .../world_state/compact_permissions_tests.rs | 64 +++++++ codex-rs/core/src/context/world_state/mod.rs | 2 + .../src/context/world_state/permissions.rs | 77 +++++++-- .../context/world_state/permissions_tests.rs | 157 ++++++++++++++++++ ...dered_without_reinjecting_permissions.snap | 16 ++ ..._state__permissions__tests__snapshots.snap | 10 +- .../core/src/context_manager/history_tests.rs | 28 ++++ codex-rs/core/src/event_mapping.rs | 2 + codex-rs/core/src/session/handlers.rs | 31 ++-- codex-rs/core/src/session/mod.rs | 18 -- codex-rs/core/src/session/world_state.rs | 4 + codex-rs/core/tests/suite/approvals.rs | 121 ++++++++++++++ 15 files changed, 539 insertions(+), 59 deletions(-) create mode 100644 codex-rs/core/src/context/world_state/compact_permissions.rs create mode 100644 codex-rs/core/src/context/world_state/compact_permissions_tests.rs create mode 100644 codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__approved_prefix_is_rendered_without_reinjecting_permissions.snap diff --git a/codex-rs/core/src/context/approved_command_prefix_saved.rs b/codex-rs/core/src/context/approved_command_prefix_saved.rs index 3686741d1c..176df83035 100644 --- a/codex-rs/core/src/context/approved_command_prefix_saved.rs +++ b/codex-rs/core/src/context/approved_command_prefix_saved.rs @@ -1,5 +1,8 @@ use super::ContextualUserFragment; +pub(crate) const APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX: &str = + "Approved command prefix saved:"; + #[derive(Debug, Clone, PartialEq)] pub(crate) struct ApprovedCommandPrefixSaved { prefixes: String, @@ -27,6 +30,9 @@ impl ContextualUserFragment for ApprovedCommandPrefixSaved { } fn body(&self) -> String { - format!("Approved command prefix saved:\n{}", self.prefixes) + format!( + "{APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX}\n{}", + self.prefixes + ) } } diff --git a/codex-rs/core/src/context/mod.rs b/codex-rs/core/src/context/mod.rs index fc874cd6d2..2e69331475 100644 --- a/codex-rs/core/src/context/mod.rs +++ b/codex-rs/core/src/context/mod.rs @@ -34,6 +34,7 @@ mod user_instructions; mod user_shell_command; pub(crate) mod world_state; +pub(crate) use approved_command_prefix_saved::APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX; pub(crate) use approved_command_prefix_saved::ApprovedCommandPrefixSaved; pub(crate) use apps_instructions::AppsInstructions; pub(crate) use available_plugins_instructions::AvailablePluginsInstructions; diff --git a/codex-rs/core/src/context/world_state/compact_permissions.rs b/codex-rs/core/src/context/world_state/compact_permissions.rs new file mode 100644 index 0000000000..f6822ef7bc --- /dev/null +++ b/codex-rs/core/src/context/world_state/compact_permissions.rs @@ -0,0 +1,59 @@ +use super::PreviousSectionState; +use super::WorldStateSection; +use crate::context::APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX; +use crate::context::ApprovedCommandPrefixSaved; +use crate::context::ContextualUserFragment; +use codex_execpolicy::Policy; +use codex_protocol::models::format_allow_prefixes; +use std::collections::BTreeSet; + +/// Newly approved command prefixes visible without the full permissions instructions. +#[derive(Clone, Debug)] +pub(crate) struct CompactPermissionsState { + prefixes: BTreeSet>, +} + +impl CompactPermissionsState { + pub(crate) fn new(exec_policy: &Policy) -> Self { + Self { + prefixes: exec_policy.get_allowed_prefixes().into_iter().collect(), + } + } +} + +impl WorldStateSection for CompactPermissionsState { + const ID: &'static str = "approved_command_prefixes"; + type Snapshot = BTreeSet>; + + fn snapshot(&self) -> Self::Snapshot { + self.prefixes.clone() + } + + fn matches_legacy_fragment(role: &str, text: &str) -> bool { + role == "developer" + && text + .trim_start() + .starts_with(APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX) + } + + fn render_diff( + &self, + previous: PreviousSectionState<'_, Self::Snapshot>, + ) -> Option> { + let added_prefixes = match previous { + PreviousSectionState::Known(previous) => self + .prefixes + .difference(previous) + .cloned() + .collect::>(), + PreviousSectionState::Absent | PreviousSectionState::Unknown => return None, + }; + format_allow_prefixes(added_prefixes) + .filter(|prefixes| !prefixes.is_empty()) + .map(|prefixes| Box::new(ApprovedCommandPrefixSaved::new(prefixes)) as _) + } +} + +#[cfg(test)] +#[path = "compact_permissions_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/context/world_state/compact_permissions_tests.rs b/codex-rs/core/src/context/world_state/compact_permissions_tests.rs new file mode 100644 index 0000000000..8f777cb1b1 --- /dev/null +++ b/codex-rs/core/src/context/world_state/compact_permissions_tests.rs @@ -0,0 +1,64 @@ +use super::*; +use codex_execpolicy::Decision; +use pretty_assertions::assert_eq; + +#[test] +fn renders_only_newly_approved_prefixes() { + use PreviousSectionState::Known; + + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule(&["git".to_string(), "pull".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + let with_existing_prefix = CompactPermissionsState::new(&exec_policy); + exec_policy + .add_prefix_rule(&["cargo".to_string(), "test".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + let with_new_prefix = CompactPermissionsState::new(&exec_policy); + let existing_snapshot = with_existing_prefix.snapshot(); + let current_snapshot = with_new_prefix.snapshot(); + + assert_eq!( + with_new_prefix + .render_diff(Known(&existing_snapshot)) + .map(|fragment| fragment.render()), + Some("Approved command prefix saved:\n- [\"cargo\", \"test\"]".to_string()) + ); + assert!( + with_new_prefix + .render_diff(Known(¤t_snapshot)) + .is_none() + ); +} + +#[test] +fn does_not_duplicate_a_retained_legacy_update() { + use PreviousSectionState::Unknown; + + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule(&["touch".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + let state = CompactPermissionsState::new(&exec_policy); + + assert_eq!( + state.render_diff(Unknown).map(|fragment| fragment.render()), + None + ); +} + +#[test] +fn does_not_render_existing_prefixes_without_a_previous_snapshot() { + use PreviousSectionState::Absent; + + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule(&["touch".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + + assert!( + CompactPermissionsState::new(&exec_policy) + .render_diff(Absent) + .is_none() + ); +} diff --git a/codex-rs/core/src/context/world_state/mod.rs b/codex-rs/core/src/context/world_state/mod.rs index 38611f29bb..28cb48c882 100644 --- a/codex-rs/core/src/context/world_state/mod.rs +++ b/codex-rs/core/src/context/world_state/mod.rs @@ -1,6 +1,7 @@ mod agents_md; mod apps_instructions; mod collaboration_mode; +mod compact_permissions; mod context_window_guidance; mod environment; mod environments_instructions; @@ -33,6 +34,7 @@ use std::fmt; pub(crate) use agents_md::AgentsMdState; pub(crate) use apps_instructions::AppsInstructionsState; pub(crate) use collaboration_mode::CollaborationModeState; +pub(crate) use compact_permissions::CompactPermissionsState; pub(crate) use context_window_guidance::ContextWindowGuidanceState; pub(crate) use environment::EnvironmentsState; pub(crate) use environments_instructions::EnvironmentsInstructionsState; diff --git a/codex-rs/core/src/context/world_state/permissions.rs b/codex-rs/core/src/context/world_state/permissions.rs index ae21469a7f..00a690fc60 100644 --- a/codex-rs/core/src/context/world_state/permissions.rs +++ b/codex-rs/core/src/context/world_state/permissions.rs @@ -2,20 +2,35 @@ use super::PreviousSectionState; use super::WorldStateHash; use super::WorldStateSection; use crate::context::ApprovalPromptContext; +use crate::context::ApprovedCommandPrefixSaved; use crate::context::ContextualUserFragment; use crate::context::PermissionsInstructions; use codex_execpolicy::Policy; use codex_protocol::models::PermissionProfile; +use codex_protocol::models::format_allow_prefixes; use codex_protocol::protocol::AskForApproval; +use serde::Deserialize; +use serde::Serialize; +use std::collections::BTreeSet; use std::path::Path; /// Permission instructions currently visible to the model. #[derive(Clone, Debug)] pub(crate) struct PermissionsState { - snapshot: WorldStateHash, + snapshot: PermissionsSnapshot, instructions: PermissionsInstructions, } +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(untagged)] +pub(crate) enum PermissionsSnapshot { + Current { + instructions: WorldStateHash, + approved_command_prefixes: BTreeSet>, + }, + Legacy(WorldStateHash), +} + impl PermissionsState { pub(crate) fn new( permission_profile: &PermissionProfile, @@ -26,16 +41,23 @@ impl PermissionsState { exec_permission_approvals_enabled: bool, request_permissions_tool_enabled: bool, ) -> Self { - let instructions = PermissionsInstructions::from_permission_profile( - permission_profile, - approval_policy, - approval_context, - exec_policy, - cwd, - exec_permission_approvals_enabled, - request_permissions_tool_enabled, - ); - let snapshot = WorldStateHash::from_fragment(&instructions); + let build_instructions = |exec_policy| { + PermissionsInstructions::from_permission_profile( + permission_profile, + approval_policy, + approval_context, + exec_policy, + cwd, + exec_permission_approvals_enabled, + request_permissions_tool_enabled, + ) + }; + let instructions = build_instructions(exec_policy); + let instructions_without_approved_prefixes = build_instructions(&Policy::empty()); + let snapshot = PermissionsSnapshot::Current { + instructions: WorldStateHash::from_fragment(&instructions_without_approved_prefixes), + approved_command_prefixes: exec_policy.get_allowed_prefixes().into_iter().collect(), + }; Self { snapshot, instructions, @@ -45,7 +67,7 @@ impl PermissionsState { impl WorldStateSection for PermissionsState { const ID: &'static str = "permissions"; - type Snapshot = WorldStateHash; + type Snapshot = PermissionsSnapshot; fn snapshot(&self) -> Self::Snapshot { self.snapshot.clone() @@ -67,8 +89,35 @@ impl WorldStateSection for PermissionsState { &self, previous: PreviousSectionState<'_, Self::Snapshot>, ) -> Option> { - if matches!(previous, PreviousSectionState::Known(previous) if previous == &self.snapshot) { - return None; + match (previous, &self.snapshot) { + ( + PreviousSectionState::Known(PermissionsSnapshot::Current { + instructions: previous_instructions, + approved_command_prefixes: previous_prefixes, + }), + PermissionsSnapshot::Current { + instructions, + approved_command_prefixes, + }, + ) if previous_instructions == instructions => { + if previous_prefixes == approved_command_prefixes { + return None; + } + if previous_prefixes.is_subset(approved_command_prefixes) { + let added_prefixes = approved_command_prefixes + .difference(previous_prefixes) + .cloned() + .collect(); + if let Some(prefixes) = format_allow_prefixes(added_prefixes) { + return Some(Box::new(ApprovedCommandPrefixSaved::new(prefixes))); + } + } + } + ( + PreviousSectionState::Known(PermissionsSnapshot::Legacy(previous)), + PermissionsSnapshot::Current { .. }, + ) if previous == &WorldStateHash::from_fragment(&self.instructions) => return None, + _ => {} } Some(Box::new(self.instructions.clone())) diff --git a/codex-rs/core/src/context/world_state/permissions_tests.rs b/codex-rs/core/src/context/world_state/permissions_tests.rs index dedeee0903..3f76ed934d 100644 --- a/codex-rs/core/src/context/world_state/permissions_tests.rs +++ b/codex-rs/core/src/context/world_state/permissions_tests.rs @@ -1,5 +1,6 @@ use super::*; use crate::context::world_state::test_support::render_section_cases; +use codex_execpolicy::Decision; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; @@ -8,6 +9,7 @@ use codex_protocol::openai_models::ApprovalMessages; use codex_protocol::openai_models::PermissionMessages; use codex_protocol::protocol::AskForApproval; use pretty_assertions::assert_eq; +use serde_json::json; use std::path::Path; #[test] @@ -30,6 +32,145 @@ fn snapshots() { ])); } +#[test] +fn approved_prefix_is_rendered_without_reinjecting_permissions() { + use PreviousSectionState::Known; + + let without_approved_prefix = permissions_state_with_default_messages(&Policy::empty()); + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule( + &["touch".to_string(), "allow-prefix.txt".to_string()], + Decision::Allow, + ) + .expect("test prefix should be valid"); + let with_approved_prefix = permissions_state_with_default_messages(&exec_policy); + let approved_prefix = r#"["touch", "allow-prefix.txt"]"#; + let without_snapshot = without_approved_prefix.snapshot(); + let with_snapshot = with_approved_prefix.snapshot(); + let rendered_update = with_approved_prefix + .render_diff(Known(&without_snapshot)) + .expect("approving a prefix should render a world-state update") + .render(); + + assert_ne!(without_snapshot, with_snapshot); + assert_ne!( + without_approved_prefix.instructions, + with_approved_prefix.instructions + ); + assert!( + !without_approved_prefix + .instructions + .body() + .contains(approved_prefix) + ); + assert!( + with_approved_prefix + .instructions + .body() + .contains(approved_prefix) + ); + assert_eq!( + rendered_update, + "Approved command prefix saved:\n- [\"touch\", \"allow-prefix.txt\"]" + ); + assert!(!rendered_update.contains("")); + + let before_approval_permissions = + permissions_state(PermissionProfile::read_only(), AskForApproval::OnRequest) + .instructions + .render(); + let model_visible_before_and_after = format!( + "BEFORE APPROVAL — PERMISSIONS ALREADY IN CONTEXT (CONDENSED)\n{before_approval_permissions}\n\nAFTER APPROVAL — WORLD-STATE DIFF APPENDS\n{rendered_update}\n\nFULL PERMISSIONS BLOCK APPENDED AFTER APPROVAL\nNone", + ); + insta::assert_snapshot!( + "approved_prefix_is_rendered_without_reinjecting_permissions", + model_visible_before_and_after + ); +} + +#[test] +fn renders_only_newly_approved_prefixes() { + use PreviousSectionState::Known; + + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule(&["git".to_string(), "pull".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + let with_existing_prefix = permissions_state_with_default_messages(&exec_policy); + exec_policy + .add_prefix_rule(&["cargo".to_string(), "test".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + let with_new_prefix = permissions_state_with_default_messages(&exec_policy); + let existing_snapshot = with_existing_prefix.snapshot(); + let current_snapshot = with_new_prefix.snapshot(); + + assert_eq!( + with_new_prefix + .render_diff(Known(&existing_snapshot)) + .map(|fragment| fragment.render()), + Some("Approved command prefix saved:\n- [\"cargo\", \"test\"]".to_string()) + ); + assert!( + with_new_prefix + .render_diff(Known(¤t_snapshot)) + .is_none() + ); +} + +#[test] +fn legacy_snapshot_deserializes_and_only_suppresses_matching_full_permissions() { + use super::super::WorldState; + use super::super::WorldStateSnapshot; + + let without_approved_prefix = permissions_state_with_default_messages(&Policy::empty()); + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule(&["touch".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + let with_approved_prefix = permissions_state_with_default_messages(&exec_policy); + let matching_legacy: WorldStateSnapshot = serde_json::from_value(json!({ + "permissions": WorldStateHash::from_fragment(&with_approved_prefix.instructions), + })) + .expect("legacy world-state snapshot should deserialize"); + let stale_legacy: WorldStateSnapshot = serde_json::from_value(json!({ + "permissions": WorldStateHash::from_fragment(&without_approved_prefix.instructions), + })) + .expect("legacy world-state snapshot should deserialize"); + let expected_permissions = with_approved_prefix.instructions.render(); + let mut world_state = WorldState::default(); + world_state.add_section(with_approved_prefix); + + assert!(world_state.render_diff(&matching_legacy).is_empty()); + assert_eq!( + world_state + .render_diff(&stale_legacy) + .into_iter() + .map(|fragment| fragment.render()) + .collect::>(), + vec![expected_permissions] + ); +} + +#[test] +fn removing_an_approved_prefix_renders_full_permissions() { + use PreviousSectionState::Known; + + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule(&["touch".to_string()], Decision::Allow) + .expect("test prefix should be valid"); + let with_approved_prefix = permissions_state_with_default_messages(&exec_policy); + let without_approved_prefix = permissions_state_with_default_messages(&Policy::empty()); + + let rendered = without_approved_prefix + .render_diff(Known(&with_approved_prefix.snapshot())) + .expect("removing a prefix should refresh permissions") + .render(); + + assert_eq!(rendered, without_approved_prefix.instructions.render()); +} + #[test] fn persisted_permissions_are_detected_inside_bundled_developer_messages() { let state = permissions_state(PermissionProfile::read_only(), AskForApproval::OnRequest); @@ -94,3 +235,19 @@ fn permissions_state( /*request_permissions_tool_enabled*/ false, ) } + +fn permissions_state_with_default_messages(exec_policy: &Policy) -> PermissionsState { + PermissionsState::new( + &PermissionProfile::read_only(), + AskForApproval::OnRequest, + ApprovalPromptContext::new( + ApprovalsReviewer::User, + /*messages*/ None, + /*permission_messages*/ None, + ), + exec_policy, + Path::new("/workspace"), + /*exec_permission_approvals_enabled*/ false, + /*request_permissions_tool_enabled*/ false, + ) +} diff --git a/codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__approved_prefix_is_rendered_without_reinjecting_permissions.snap b/codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__approved_prefix_is_rendered_without_reinjecting_permissions.snap new file mode 100644 index 0000000000..0d4eff502f --- /dev/null +++ b/codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__approved_prefix_is_rendered_without_reinjecting_permissions.snap @@ -0,0 +1,16 @@ +--- +source: core/src/context/world_state/permissions_tests.rs +expression: model_visible_before_and_after +--- +BEFORE APPROVAL — PERMISSIONS ALREADY IN CONTEXT (CONDENSED) + +Read only. +Ask for approval. + + +AFTER APPROVAL — WORLD-STATE DIFF APPENDS +Approved command prefix saved: +- ["touch", "allow-prefix.txt"] + +FULL PERMISSIONS BLOCK APPENDED AFTER APPROVAL +None diff --git a/codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__snapshots.snap b/codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__snapshots.snap index caa912ecce..efe6728413 100644 --- a/codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__snapshots.snap +++ b/codex-rs/core/src/context/world_state/snapshots/codex_core__context__world_state__permissions__tests__snapshots.snap @@ -5,28 +5,28 @@ expression: "render_section_cases(&[(Absent, Absent), (Absent, Known(&read_only) Absent -> Absent None -Absent -> "0ccde536dd8b4cfebb1df573f679dfe86b5718e3" (role - developer) +Absent -> {"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} (role - developer) Read only. Ask for approval. -"0ccde536dd8b4cfebb1df573f679dfe86b5718e3" -> "0ccde536dd8b4cfebb1df573f679dfe86b5718e3" +{"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} -> {"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} None -"0ccde536dd8b4cfebb1df573f679dfe86b5718e3" -> "7e17d9a0df61e5f0c032ed7f0bf1dda58c9d5e85" (role - developer) +{"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} -> {"approved_command_prefixes":[],"instructions":"7e17d9a0df61e5f0c032ed7f0bf1dda58c9d5e85"} (role - developer) Full access. Ask for approval. -"0ccde536dd8b4cfebb1df573f679dfe86b5718e3" -> "d96f228104cb899a8d3788b08e1d64b3bbb6ac84" (role - developer) +{"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} -> {"approved_command_prefixes":[],"instructions":"d96f228104cb899a8d3788b08e1d64b3bbb6ac84"} (role - developer) Read only. Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. -Unknown -> "0ccde536dd8b4cfebb1df573f679dfe86b5718e3" (role - developer) +Unknown -> {"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} (role - developer) Read only. Ask for approval. diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index d1dac7d312..2f3ea18237 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::context::APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX; use crate::context::UserInstructions; use crate::context::world_state::WorldState; use crate::context::world_state::WorldStateSection; @@ -1102,6 +1103,33 @@ fn drop_last_n_user_turns_trims_context_updates_above_rolled_back_turn() { ); } +#[test] +fn drop_last_n_user_turns_trims_saved_prefix_update_above_rolled_back_turn() { + let items = vec![ + assistant_msg("session prefix item"), + user_input_text_msg("turn 1 user"), + assistant_msg("turn 1 assistant"), + developer_msg(&format!( + "{APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX}\n- [\"touch\"]" + )), + user_input_text_msg("turn 2 user"), + assistant_msg("turn 2 assistant"), + ]; + + let modalities = default_input_modalities(); + let mut history = create_history_with_items(items); + history.drop_last_n_user_turns(/*num_turns*/ 1); + + assert_eq!( + history.for_prompt(&modalities), + vec![ + assistant_msg("session prefix item"), + user_input_text_msg("turn 1 user"), + assistant_msg("turn 1 assistant"), + ] + ); +} + #[test] fn drop_last_n_user_turns_clears_reference_context_for_mixed_developer_context_bundles() { let items = vec![ diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index 7af4e5f99a..42bbcc2df6 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -32,12 +32,14 @@ use codex_protocol::user_input::UserInput; use tracing::warn; use uuid::Uuid; +use crate::context::APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX; use crate::context::is_contextual_user_fragment; use crate::context::parse_visible_hook_prompt_message; use crate::web_search::web_search_action_detail; const CONTEXTUAL_DEVELOPER_PREFIXES: &[&str] = &[ "", + APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX, "", APPS_INSTRUCTIONS_OPEN_TAG, COLLABORATION_MODE_OPEN_TAG, diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index dc8b06f02e..8ed7a7badd 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -381,29 +381,18 @@ pub async fn exec_approval( if let ReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment, } = &decision - { - match sess + && let Err(err) = sess .persist_execpolicy_amendment(proposed_execpolicy_amendment) .await - { - Ok(()) => { - sess.record_execpolicy_amendment_message( - &event_turn_id, - proposed_execpolicy_amendment, - ) - .await; - } - Err(err) => { - let message = format!("Failed to apply execpolicy amendment: {err}"); - tracing::warn!("{message}"); - let warning = EventMsg::Warning(WarningEvent { message }); - sess.send_event_raw(Event { - id: event_turn_id.clone(), - msg: warning, - }) - .await; - } - } + { + let message = format!("Failed to apply execpolicy amendment: {err}"); + tracing::warn!("{message}"); + let warning = EventMsg::Warning(WarningEvent { message }); + sess.send_event_raw(Event { + id: event_turn_id.clone(), + msg: warning, + }) + .await; } match decision { ReviewDecision::Abort => { diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 36d311255d..ec9c9cb1ea 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -22,7 +22,6 @@ use crate::compact; use crate::compact::CompactedHistoryMetadata; use crate::config::ManagedFeatures; use crate::config::resolve_tool_suggest_config_from_layer_stack; -use crate::context::ApprovedCommandPrefixSaved; use crate::context::ContextualUserFragment; use crate::context::ModelSwitchInstructions; use crate::context::NetworkRuleSaved; @@ -104,7 +103,6 @@ use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::BaseInstructions; use codex_protocol::models::PermissionProfile; use codex_protocol::models::SandboxEnforcement; -use codex_protocol::models::format_allow_prefixes; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; use codex_protocol::permissions::FileSystemSandboxPolicy; @@ -2164,22 +2162,6 @@ impl Session { )) } - pub(crate) async fn record_execpolicy_amendment_message( - &self, - sub_id: &str, - amendment: &ExecPolicyAmendment, - ) { - let Some(prefixes) = format_allow_prefixes(vec![amendment.command.clone()]) else { - warn!("execpolicy amendment for {sub_id} had no command prefix"); - return; - }; - let message: ResponseItem = - ContextualUserFragment::into(ApprovedCommandPrefixSaved::new(prefixes)); - let turn_context = self.turn_context_for_sub_id(sub_id).await; - self.inject_no_new_turn(vec![message], turn_context.as_deref()) - .await; - } - pub(crate) async fn persist_network_policy_amendment( &self, amendment: &NetworkPolicyAmendment, diff --git a/codex-rs/core/src/session/world_state.rs b/codex-rs/core/src/session/world_state.rs index 073dba8aec..f56b8baecd 100644 --- a/codex-rs/core/src/session/world_state.rs +++ b/codex-rs/core/src/session/world_state.rs @@ -7,6 +7,7 @@ use crate::context::ApprovalPromptContext; use crate::context::world_state::AgentsMdState; use crate::context::world_state::AppsInstructionsState; use crate::context::world_state::CollaborationModeState; +use crate::context::world_state::CompactPermissionsState; use crate::context::world_state::ContextWindowGuidanceState; use crate::context::world_state::EnvironmentsInstructionsState; use crate::context::world_state::EnvironmentsState; @@ -135,6 +136,9 @@ impl Session { .features .enabled(Feature::RequestPermissionsTool), )); + } else { + let exec_policy = self.services.exec_policy.current(); + world_state.add_section(CompactPermissionsState::new(exec_policy.as_ref())); } if turn_context.config.include_collaboration_mode_instructions { world_state.add_section(CollaborationModeState::from_collaboration_mode( diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 9e8ac16142..75a9fabc76 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -2139,6 +2139,127 @@ async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() Ok(()) } +#[tokio::test(flavor = "current_thread")] +#[cfg(unix)] +async fn approving_execpolicy_amendment_does_not_reinject_permissions_instructions() -> Result<()> { + assert_execpolicy_amendment_context(PermissionsInstructionsExpectation::Included).await +} + +#[tokio::test(flavor = "current_thread")] +#[cfg(unix)] +async fn approving_execpolicy_amendment_is_reported_when_permissions_are_disabled() -> Result<()> { + assert_execpolicy_amendment_context(PermissionsInstructionsExpectation::Omitted).await +} + +#[derive(Clone, Copy)] +enum PermissionsInstructionsExpectation { + Included, + Omitted, +} + +async fn assert_execpolicy_amendment_context( + expectation: PermissionsInstructionsExpectation, +) -> Result<()> { + let (include_permissions_instructions, expected_permissions_block_count) = match expectation { + PermissionsInstructionsExpectation::Included => (true, 1), + PermissionsInstructionsExpectation::Omitted => (false, 0), + }; + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let sandbox_policy_for_config = sandbox_policy.clone(); + let mut builder = test_codex() + .with_model_info_override("gpt-5.4", |model_info| { + model_info.model_messages = None; + }) + .with_config(move |config| { + config.include_permissions_instructions = include_permissions_instructions; + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config + .features + .disable(Feature::DeferredExecutor) + .expect("test config should allow feature update"); + config + .set_legacy_sandbox_policy(sandbox_policy_for_config) + .expect("set sandbox policy"); + }); + let test = builder.build(&server).await?; + + let call_id = "permissions-prefix"; + let (event, expected_command) = ActionKind::RunCommand { + command: "touch permissions-prefix.txt", + } + .prepare( + &test, + &server, + call_id, + SandboxPermissions::RequireEscalated, + ) + .await?; + let expected_command = + expected_command.expect("execpolicy amendment scenario should produce a shell command"); + let expected_execpolicy_amendment = ExecPolicyAmendment::new(vec![ + "touch".to_string(), + "permissions-prefix.txt".to_string(), + ]); + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-permissions-prefix-1"), + event, + ev_completed("resp-permissions-prefix-1"), + ]), + ) + .await; + let follow_up = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-permissions-prefix", "done"), + ev_completed("resp-permissions-prefix-2"), + ]), + ) + .await; + + submit_turn(&test, "permissions-prefix", approval_policy, sandbox_policy).await?; + + let approval = expect_exec_approval(&test, expected_command.as_str()).await; + assert_eq!( + approval.proposed_execpolicy_amendment, + Some(expected_execpolicy_amendment.clone()) + ); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment: expected_execpolicy_amendment, + }, + }) + .await?; + wait_for_completion(&test).await; + + let developer_messages = follow_up.single_request().message_input_texts("developer"); + let permissions_block_count = developer_messages + .iter() + .map(|message| message.matches("").count()) + .sum::(); + assert_eq!( + permissions_block_count, expected_permissions_block_count, + "saving an execpolicy amendment must respect the full permissions rendering mode: \ + {developer_messages:#?}" + ); + assert!( + developer_messages + .iter() + .any(|message| message.contains("Approved command prefix saved:") + && message.contains(r#"["touch", "permissions-prefix.txt"]"#)), + "expected developer message documenting saved rule, got: {developer_messages:?}" + ); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] #[cfg(unix)] async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts() -> Result<()> {