mirror of
https://github.com/openai/codex.git
synced 2026-10-21 05:22:21 +00:00
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
This commit is contained in:
@@ -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
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<Vec<String>>,
|
||||
}
|
||||
|
||||
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<Vec<String>>;
|
||||
|
||||
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<Box<dyn ContextualUserFragment>> {
|
||||
let added_prefixes = match previous {
|
||||
PreviousSectionState::Known(previous) => self
|
||||
.prefixes
|
||||
.difference(previous)
|
||||
.cloned()
|
||||
.collect::<Vec<_>>(),
|
||||
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;
|
||||
@@ -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()
|
||||
);
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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<Vec<String>>,
|
||||
},
|
||||
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<Box<dyn ContextualUserFragment>> {
|
||||
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()))
|
||||
|
||||
@@ -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("<permissions instructions>"));
|
||||
|
||||
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<_>>(),
|
||||
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,
|
||||
)
|
||||
}
|
||||
|
||||
+16
@@ -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)
|
||||
<permissions instructions>
|
||||
Read only.
|
||||
Ask for approval.
|
||||
</permissions instructions>
|
||||
|
||||
AFTER APPROVAL — WORLD-STATE DIFF APPENDS
|
||||
Approved command prefix saved:
|
||||
- ["touch", "allow-prefix.txt"]
|
||||
|
||||
FULL PERMISSIONS BLOCK APPENDED AFTER APPROVAL
|
||||
None
|
||||
+5
-5
@@ -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)
|
||||
<permissions instructions>
|
||||
Read only.
|
||||
Ask for approval.
|
||||
</permissions instructions>
|
||||
|
||||
"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)
|
||||
<permissions instructions>
|
||||
Full access.
|
||||
Ask for approval.
|
||||
</permissions instructions>
|
||||
|
||||
"0ccde536dd8b4cfebb1df573f679dfe86b5718e3" -> "d96f228104cb899a8d3788b08e1d64b3bbb6ac84" (role - developer)
|
||||
{"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} -> {"approved_command_prefixes":[],"instructions":"d96f228104cb899a8d3788b08e1d64b3bbb6ac84"} (role - developer)
|
||||
<permissions instructions>
|
||||
Read only.
|
||||
Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected.
|
||||
</permissions instructions>
|
||||
|
||||
Unknown -> "0ccde536dd8b4cfebb1df573f679dfe86b5718e3" (role - developer)
|
||||
Unknown -> {"approved_command_prefixes":[],"instructions":"0ccde536dd8b4cfebb1df573f679dfe86b5718e3"} (role - developer)
|
||||
<permissions instructions>
|
||||
Read only.
|
||||
Ask for approval.
|
||||
|
||||
@@ -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![
|
||||
|
||||
@@ -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] = &[
|
||||
"<permissions instructions>",
|
||||
APPROVED_COMMAND_PREFIX_SAVED_MESSAGE_PREFIX,
|
||||
"<model_switch>",
|
||||
APPS_INSTRUCTIONS_OPEN_TAG,
|
||||
COLLABORATION_MODE_OPEN_TAG,
|
||||
|
||||
@@ -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 => {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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("<permissions instructions>").count())
|
||||
.sum::<usize>();
|
||||
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<()> {
|
||||
|
||||
Reference in New Issue
Block a user