mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Propagate updated permissions to review threads (#36901)
## What changed - Apply the session's current approval policy whenever per-turn configuration is built. - Have review threads inherit the parent turn's full permission settings and approval reviewer. - Add coverage for settings updated after session startup, including the approval policy, permission profile, and approval reviewer. GitOrigin-RevId: bff050c6de157dd65d62f7b49651bef3f426ee3f
This commit is contained in:
1 parent
18f03c1eb7
commit
f93109615f
5 files changed
+116
-10
No files matched your search
@@ -67,8 +67,7 @@ impl Session {
|
|||||||
.primary()
|
.primary()
|
||||||
.and_then(|environment| environment.cwd().to_abs_path().ok())
|
.and_then(|environment| environment.cwd().to_abs_path().ok())
|
||||||
.unwrap_or_else(|| session_configuration.cwd().clone());
|
.unwrap_or_else(|| session_configuration.cwd().clone());
|
||||||
let mut config = Self::build_per_turn_config(&session_configuration, cwd);
|
let config = Self::build_per_turn_config(&session_configuration, cwd);
|
||||||
config.permissions.approval_policy = session_configuration.approval_policy.clone();
|
|
||||||
|
|
||||||
McpDesiredState {
|
McpDesiredState {
|
||||||
config: Arc::new(config),
|
config: Arc::new(config),
|
||||||
@@ -90,8 +89,7 @@ impl Session {
|
|||||||
) -> anyhow::Result<()> {
|
) -> anyhow::Result<()> {
|
||||||
let cwd = AbsolutePathBuf::from_absolute_path(local_stdio_fallback_cwd)
|
let cwd = AbsolutePathBuf::from_absolute_path(local_stdio_fallback_cwd)
|
||||||
.unwrap_or_else(|_| session_configuration.cwd().clone());
|
.unwrap_or_else(|_| session_configuration.cwd().clone());
|
||||||
let mut config = Self::build_per_turn_config(session_configuration, cwd);
|
let config = Self::build_per_turn_config(session_configuration, cwd);
|
||||||
config.permissions.approval_policy = session_configuration.approval_policy.clone();
|
|
||||||
let desired = McpDesiredState {
|
let desired = McpDesiredState {
|
||||||
config: Arc::new(config),
|
config: Arc::new(config),
|
||||||
auth,
|
auth,
|
||||||
|
|||||||
@@ -48,11 +48,8 @@ pub(super) async fn spawn_review_thread(
|
|||||||
let mut per_turn_config = (*config).clone();
|
let mut per_turn_config = (*config).clone();
|
||||||
per_turn_config.model = Some(model.clone());
|
per_turn_config.model = Some(model.clone());
|
||||||
per_turn_config.features = review_features.clone();
|
per_turn_config.features = review_features.clone();
|
||||||
per_turn_config.permissions.shell_environment_policy = parent_turn_context
|
per_turn_config.permissions = parent_turn_context.config.permissions.clone();
|
||||||
.config
|
per_turn_config.approvals_reviewer = parent_turn_context.config.approvals_reviewer;
|
||||||
.permissions
|
|
||||||
.shell_environment_policy
|
|
||||||
.clone();
|
|
||||||
per_turn_config.codex_linux_sandbox_exe =
|
per_turn_config.codex_linux_sandbox_exe =
|
||||||
parent_turn_context.config.codex_linux_sandbox_exe.clone();
|
parent_turn_context.config.codex_linux_sandbox_exe.clone();
|
||||||
per_turn_config.compact_prompt = parent_turn_context.config.compact_prompt.clone();
|
per_turn_config.compact_prompt = parent_turn_context.config.compact_prompt.clone();
|
||||||
|
|||||||
@@ -8148,6 +8148,11 @@ async fn refreshed_mcp_binding_captures_current_approval_authority() {
|
|||||||
let (session, old_turn) = make_session_and_context().await;
|
let (session, old_turn) = make_session_and_context().await;
|
||||||
let session = Arc::new(session);
|
let session = Arc::new(session);
|
||||||
let previous_policy = old_turn.approval_policy.value();
|
let previous_policy = old_turn.approval_policy.value();
|
||||||
|
assert_ne!(previous_policy, AskForApproval::Never);
|
||||||
|
assert_eq!(
|
||||||
|
old_turn.config.permissions.approval_policy.value(),
|
||||||
|
previous_policy
|
||||||
|
);
|
||||||
|
|
||||||
session
|
session
|
||||||
.update_settings(SessionSettingsUpdate {
|
.update_settings(SessionSettingsUpdate {
|
||||||
@@ -8180,6 +8185,17 @@ async fn refreshed_mcp_binding_captures_current_approval_authority() {
|
|||||||
)
|
)
|
||||||
);
|
);
|
||||||
assert_eq!(old_turn.approval_policy.value(), previous_policy);
|
assert_eq!(old_turn.approval_policy.value(), previous_policy);
|
||||||
|
assert_eq!(
|
||||||
|
old_turn.config.permissions.approval_policy.value(),
|
||||||
|
previous_policy
|
||||||
|
);
|
||||||
|
|
||||||
|
let new_turn = session.new_default_turn().await;
|
||||||
|
assert_eq!(new_turn.approval_policy.value(), AskForApproval::Never);
|
||||||
|
assert_eq!(
|
||||||
|
new_turn.config.permissions.approval_policy.value(),
|
||||||
|
AskForApproval::Never
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
|
|||||||
@@ -440,6 +440,7 @@ impl Session {
|
|||||||
let config = session_configuration.original_config_do_not_use.clone();
|
let config = session_configuration.original_config_do_not_use.clone();
|
||||||
let mut per_turn_config = (*config).clone();
|
let mut per_turn_config = (*config).clone();
|
||||||
per_turn_config.cwd = cwd;
|
per_turn_config.cwd = cwd;
|
||||||
|
per_turn_config.permissions.approval_policy = session_configuration.approval_policy.clone();
|
||||||
let workspace_roots = session_configuration.primary_workspace_roots();
|
let workspace_roots = session_configuration.primary_workspace_roots();
|
||||||
per_turn_config.workspace_roots = workspace_roots.clone();
|
per_turn_config.workspace_roots = workspace_roots.clone();
|
||||||
per_turn_config
|
per_turn_config
|
||||||
@@ -478,7 +479,6 @@ impl Session {
|
|||||||
let mut config =
|
let mut config =
|
||||||
Self::build_per_turn_config(session_configuration, session_configuration.cwd().clone());
|
Self::build_per_turn_config(session_configuration, session_configuration.cwd().clone());
|
||||||
config.model = Some(session_configuration.collaboration_mode.model().to_string());
|
config.model = Some(session_configuration.collaboration_mode.model().to_string());
|
||||||
config.permissions.approval_policy = session_configuration.approval_policy.clone();
|
|
||||||
config
|
config
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,9 +1,14 @@
|
|||||||
use codex_core::CodexThread;
|
use codex_core::CodexThread;
|
||||||
use codex_core::REVIEW_PROMPT;
|
use codex_core::REVIEW_PROMPT;
|
||||||
use codex_core::config::Config;
|
use codex_core::config::Config;
|
||||||
|
use codex_core::config::Constrained;
|
||||||
|
use codex_core::find_thread_path_by_id_str;
|
||||||
|
use codex_protocol::config_types::ApprovalsReviewer;
|
||||||
use codex_protocol::items::TurnItem;
|
use codex_protocol::items::TurnItem;
|
||||||
use codex_protocol::models::ContentItem;
|
use codex_protocol::models::ContentItem;
|
||||||
|
use codex_protocol::models::PermissionProfile;
|
||||||
use codex_protocol::models::ResponseItem;
|
use codex_protocol::models::ResponseItem;
|
||||||
|
use codex_protocol::protocol::AskForApproval;
|
||||||
use codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG;
|
use codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG;
|
||||||
use codex_protocol::protocol::EventMsg;
|
use codex_protocol::protocol::EventMsg;
|
||||||
use codex_protocol::protocol::ExitedReviewModeEvent;
|
use codex_protocol::protocol::ExitedReviewModeEvent;
|
||||||
@@ -16,6 +21,7 @@ use codex_protocol::protocol::ReviewRequest;
|
|||||||
use codex_protocol::protocol::ReviewTarget;
|
use codex_protocol::protocol::ReviewTarget;
|
||||||
use codex_protocol::protocol::RolloutItem;
|
use codex_protocol::protocol::RolloutItem;
|
||||||
use codex_protocol::protocol::RolloutLine;
|
use codex_protocol::protocol::RolloutLine;
|
||||||
|
use codex_protocol::protocol::ThreadSettingsOverrides;
|
||||||
use codex_protocol::review_format::render_review_output_text;
|
use codex_protocol::review_format::render_review_output_text;
|
||||||
use codex_protocol::user_input::UserInput;
|
use codex_protocol::user_input::UserInput;
|
||||||
use core_test_support::PathBufExt;
|
use core_test_support::PathBufExt;
|
||||||
@@ -549,6 +555,95 @@ async fn review_does_not_emit_agent_message_on_structured_output() {
|
|||||||
server.verify().await;
|
server.verify().await;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Review requests must inherit permissions and reviewer settings updated after session startup.
|
||||||
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
|
async fn review_uses_updated_turn_permissions_and_approval_policy() {
|
||||||
|
skip_if_no_network!();
|
||||||
|
|
||||||
|
let (server, request_log) =
|
||||||
|
start_responses_server_with_sse(completed_sse(), /*expected_requests*/ 1).await;
|
||||||
|
let codex_home = Arc::new(TempDir::new().unwrap());
|
||||||
|
let codex = new_conversation_for_server(&server, codex_home.clone(), |config| {
|
||||||
|
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||||
|
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||||
|
config
|
||||||
|
.permissions
|
||||||
|
.set_permission_profile(PermissionProfile::read_only())
|
||||||
|
.expect("initial permission profile should be valid");
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
|
||||||
|
core_test_support::submit_thread_settings(
|
||||||
|
&codex,
|
||||||
|
ThreadSettingsOverrides {
|
||||||
|
approval_policy: Some(AskForApproval::Never),
|
||||||
|
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||||
|
permission_profile: Some(PermissionProfile::Disabled),
|
||||||
|
..Default::default()
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.expect("updated thread permissions should be accepted");
|
||||||
|
|
||||||
|
codex
|
||||||
|
.submit(Op::Review {
|
||||||
|
review_request: ReviewRequest {
|
||||||
|
target: ReviewTarget::Custom {
|
||||||
|
instructions: "review current permissions".to_string(),
|
||||||
|
},
|
||||||
|
user_facing_hint: None,
|
||||||
|
},
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.expect("review should start");
|
||||||
|
wait_for_event(&codex, |event| matches!(event, EventMsg::TurnComplete(_))).await;
|
||||||
|
|
||||||
|
let request = request_log.single_request();
|
||||||
|
assert!(
|
||||||
|
request
|
||||||
|
.message_input_texts("developer")
|
||||||
|
.iter()
|
||||||
|
.any(|text| text.contains("Approval policy is currently never")),
|
||||||
|
"review should use the updated approval policy"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
request
|
||||||
|
.message_input_texts("user")
|
||||||
|
.iter()
|
||||||
|
.any(|text| text.contains("<permission_profile type=\"disabled\">")),
|
||||||
|
"review should use the updated permission profile"
|
||||||
|
);
|
||||||
|
let review_thread_id = request.body_json()["client_metadata"]["thread_id"]
|
||||||
|
.as_str()
|
||||||
|
.expect("review request should include its thread ID")
|
||||||
|
.to_string();
|
||||||
|
let review_rollout_path = find_thread_path_by_id_str(
|
||||||
|
codex_home.path(),
|
||||||
|
&review_thread_id,
|
||||||
|
/*state_db_ctx*/ None,
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.expect("review rollout lookup should succeed")
|
||||||
|
.expect("review thread should have a rollout");
|
||||||
|
let review_rollout =
|
||||||
|
std::fs::read_to_string(review_rollout_path).expect("review rollout should be readable");
|
||||||
|
let review_approvals_reviewer = review_rollout
|
||||||
|
.lines()
|
||||||
|
.filter_map(|line| {
|
||||||
|
let rollout_line: RolloutLine =
|
||||||
|
serde_json::from_str(line).expect("review rollout line should be valid");
|
||||||
|
match rollout_line.item {
|
||||||
|
RolloutItem::TurnContext(turn_context) => turn_context.approvals_reviewer,
|
||||||
|
_ => None,
|
||||||
|
}
|
||||||
|
})
|
||||||
|
.next_back();
|
||||||
|
assert_eq!(review_approvals_reviewer, Some(ApprovalsReviewer::User));
|
||||||
|
|
||||||
|
let _codex_home_guard = codex_home;
|
||||||
|
server.verify().await;
|
||||||
|
}
|
||||||
|
|
||||||
/// Ensure that when a custom `review_model` is set in the config, the review
|
/// Ensure that when a custom `review_model` is set in the config, the review
|
||||||
/// request uses that model (and not the main chat model).
|
/// request uses that model (and not the main chat model).
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
|
|||||||
Reference in new issue
Block a user