mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
tui: pass active permission profiles through app commands (#22891)
## Why This continues the permissions migration by keeping the TUI command boundary aligned with the app-server protocol direction from #22795: callers should select a permission profile by id instead of passing a concrete `PermissionProfile` value around as the turn configuration. `AppCommand` is internal to the TUI, but it is the path that eventually becomes `thread/turn/start`, so carrying concrete profile details there made it too easy for UI code to keep relying on the old whole-profile replacement model. ## What changed - `AppCommand::UserTurn` and `AppCommand::OverrideTurnContext` now carry `Option<ActivePermissionProfile>` instead of `PermissionProfile`. - Composer submissions copy the active permission profile id from the current session snapshot; legacy snapshots intentionally submit no active profile id. - Permission preset UI events now carry only the active built-in profile id. The app derives the concrete built-in `PermissionProfile` internally only when updating its local config/status snapshot. - Permission presets expose their built-in active profile id, and preset selection preserves that id in both the immediate turn override and the local TUI config snapshot. - Turn routing sends `TurnPermissionsOverride::ActiveProfile` when an active id is present, and only falls back to the legacy sandbox projection for the remaining runtime override path. ## How to review Start with `codex-rs/tui/src/app_command.rs` to verify the command shape no longer exposes `PermissionProfile`. Then read `codex-rs/tui/src/app/thread_routing.rs` to verify the app-server turn-start conversion: active ids go through as ids, while the legacy sandbox fallback is still constrained to the existing runtime override case. Finally, check `codex-rs/tui/src/chatwidget/permission_popups.rs`, `codex-rs/tui/src/app/event_dispatch.rs`, `codex-rs/tui/src/app/config_persistence.rs`, and `codex-rs/utils/approval-presets/src/lib.rs` to see how preset selections stay id-only across TUI events while the local display/config mirror still gets a concrete built-in profile. ## Verification Latest local verification after the id-only `AppEvent` cleanup: - `cargo check -p codex-tui --tests` - `cargo test -p codex-tui permissions_selection_sends_approvals_reviewer_in_override_turn_context` - `cargo test -p codex-tui update_feature_flags_enabling_guardian` - `cargo test -p codex-utils-approval-presets` - `just fmt` - `just fix -p codex-tui -p codex-utils-approval-presets` Earlier in the same PR, before the final event-shape cleanup: - `cargo test -p codex-tui turn_permissions_` - `cargo test -p codex-tui submission_` - `cargo test -p codex-tui session_configured_syncs_widget_config_permissions_and_cwd` - `RUST_MIN_STACK=16777216 cargo test -p codex-tui`
This commit is contained in:
@@ -336,12 +336,12 @@ impl ChatWidget {
|
||||
None if self.config.notices.fast_default_opt_out == Some(true) => Some(None),
|
||||
None => None,
|
||||
};
|
||||
let permission_profile = self.config.permissions.effective_permission_profile();
|
||||
let active_permission_profile = self.config.permissions.active_permission_profile();
|
||||
let op = AppCommand::user_turn(
|
||||
items,
|
||||
self.config.cwd.to_path_buf(),
|
||||
AskForApproval::from(self.config.permissions.approval_policy.value()),
|
||||
permission_profile,
|
||||
active_permission_profile,
|
||||
effective_mode.model().to_string(),
|
||||
effective_mode.reasoning_effort(),
|
||||
/*summary*/ None,
|
||||
|
||||
@@ -124,7 +124,7 @@ impl ChatWidget {
|
||||
} else {
|
||||
Self::approval_preset_actions(
|
||||
preset_approval,
|
||||
preset.permission_profile.clone(),
|
||||
preset.active_permission_profile.clone(),
|
||||
base_name.clone(),
|
||||
ApprovalsReviewer::User,
|
||||
)
|
||||
@@ -134,7 +134,7 @@ impl ChatWidget {
|
||||
{
|
||||
Self::approval_preset_actions(
|
||||
preset_approval,
|
||||
preset.permission_profile.clone(),
|
||||
preset.active_permission_profile.clone(),
|
||||
base_name.clone(),
|
||||
ApprovalsReviewer::User,
|
||||
)
|
||||
@@ -142,7 +142,7 @@ impl ChatWidget {
|
||||
} else {
|
||||
Self::approval_preset_actions(
|
||||
preset_approval,
|
||||
preset.permission_profile.clone(),
|
||||
preset.active_permission_profile.clone(),
|
||||
base_name.clone(),
|
||||
ApprovalsReviewer::User,
|
||||
)
|
||||
@@ -180,7 +180,7 @@ impl ChatWidget {
|
||||
),
|
||||
actions: Self::approval_preset_actions(
|
||||
preset_approval,
|
||||
preset.permission_profile.clone(),
|
||||
preset.active_permission_profile.clone(),
|
||||
"Auto-review".to_string(),
|
||||
ApprovalsReviewer::AutoReview,
|
||||
),
|
||||
@@ -308,17 +308,16 @@ impl ChatWidget {
|
||||
|
||||
pub(super) fn approval_preset_actions(
|
||||
approval: AskForApproval,
|
||||
permission_profile: PermissionProfile,
|
||||
active_permission_profile: ActivePermissionProfile,
|
||||
label: String,
|
||||
approvals_reviewer: ApprovalsReviewer,
|
||||
) -> Vec<SelectionAction> {
|
||||
vec![Box::new(move |tx| {
|
||||
let permission_profile_clone = permission_profile.clone();
|
||||
tx.send(AppEvent::CodexOp(AppCommand::override_turn_context(
|
||||
/*cwd*/ None,
|
||||
Some(approval),
|
||||
Some(approvals_reviewer),
|
||||
Some(permission_profile_clone.clone()),
|
||||
Some(active_permission_profile.clone()),
|
||||
/*windows_sandbox_level*/ None,
|
||||
/*model*/ None,
|
||||
/*effort*/ None,
|
||||
@@ -328,7 +327,9 @@ impl ChatWidget {
|
||||
/*personality*/ None,
|
||||
)));
|
||||
tx.send(AppEvent::UpdateAskForApprovalPolicy(approval));
|
||||
tx.send(AppEvent::UpdatePermissionProfile(permission_profile_clone));
|
||||
tx.send(AppEvent::UpdateActivePermissionProfile(
|
||||
active_permission_profile.clone(),
|
||||
));
|
||||
tx.send(AppEvent::UpdateApprovalsReviewer(approvals_reviewer));
|
||||
tx.send(AppEvent::InsertHistoryCell(Box::new(
|
||||
history_cell::new_info_event(
|
||||
@@ -385,7 +386,6 @@ impl ChatWidget {
|
||||
) {
|
||||
let selected_name = preset.label.to_string();
|
||||
let approval = AskForApproval::from(preset.approval);
|
||||
let permission_profile = preset.permission_profile;
|
||||
let mut header_children: Vec<Box<dyn Renderable>> = Vec::new();
|
||||
let title_line = Line::from("Enable full access?").bold();
|
||||
let info_line = Line::from(vec![
|
||||
@@ -402,7 +402,7 @@ impl ChatWidget {
|
||||
|
||||
let mut accept_actions = Self::approval_preset_actions(
|
||||
approval,
|
||||
permission_profile.clone(),
|
||||
preset.active_permission_profile.clone(),
|
||||
selected_name.clone(),
|
||||
ApprovalsReviewer::User,
|
||||
);
|
||||
@@ -412,7 +412,7 @@ impl ChatWidget {
|
||||
|
||||
let mut accept_and_remember_actions = Self::approval_preset_actions(
|
||||
approval,
|
||||
permission_profile,
|
||||
preset.active_permission_profile,
|
||||
selected_name,
|
||||
ApprovalsReviewer::User,
|
||||
);
|
||||
|
||||
@@ -285,7 +285,7 @@ impl ChatWidget {
|
||||
/*cwd*/ None,
|
||||
/*approval_policy*/ None,
|
||||
/*approvals_reviewer*/ None,
|
||||
/*permission_profile*/ None,
|
||||
/*active_permission_profile*/ None,
|
||||
/*windows_sandbox_level*/ None,
|
||||
Some(switch_model_for_events.clone()),
|
||||
Some(Some(default_effort)),
|
||||
|
||||
@@ -107,7 +107,7 @@ impl ChatWidget {
|
||||
/*cwd*/ None,
|
||||
/*approval_policy*/ None,
|
||||
/*approvals_reviewer*/ None,
|
||||
/*permission_profile*/ None,
|
||||
/*active_permission_profile*/ None,
|
||||
/*windows_sandbox_level*/ None,
|
||||
/*model*/ None,
|
||||
/*effort*/ None,
|
||||
|
||||
@@ -17,13 +17,15 @@ impl ChatWidget {
|
||||
}
|
||||
}
|
||||
|
||||
/// Set the permission profile in the widget's config copy.
|
||||
#[cfg_attr(not(target_os = "windows"), allow(dead_code))]
|
||||
pub(crate) fn set_permission_profile(
|
||||
pub(crate) fn set_permission_profile_from_session_snapshot(
|
||||
&mut self,
|
||||
profile: PermissionProfile,
|
||||
active_profile: Option<ActivePermissionProfile>,
|
||||
) -> ConstraintResult<()> {
|
||||
self.config.permissions.set_permission_profile(profile)?;
|
||||
self.config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(profile, active_profile)?;
|
||||
self.refresh_status_surfaces();
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -53,7 +53,7 @@ impl ChatWidget {
|
||||
/*cwd*/ None,
|
||||
/*approval_policy*/ None,
|
||||
/*approvals_reviewer*/ None,
|
||||
/*permission_profile*/ None,
|
||||
/*active_permission_profile*/ None,
|
||||
/*windows_sandbox_level*/ None,
|
||||
/*model*/ None,
|
||||
/*effort*/ None,
|
||||
|
||||
@@ -149,6 +149,8 @@ pub(super) use codex_protocol::config_types::CollaborationMode;
|
||||
pub(super) use codex_protocol::config_types::ModeKind;
|
||||
pub(super) use codex_protocol::config_types::Personality;
|
||||
pub(super) use codex_protocol::config_types::ServiceTier;
|
||||
pub(super) use codex_protocol::models::ActivePermissionProfile;
|
||||
pub(super) use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_WORKSPACE;
|
||||
pub(super) use codex_protocol::models::FileSystemPermissions;
|
||||
pub(super) use codex_protocol::models::MessagePhase;
|
||||
pub(super) use codex_protocol::models::NetworkPermissions;
|
||||
|
||||
@@ -94,7 +94,7 @@ async fn submission_preserves_text_elements_and_local_images() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn submission_includes_configured_permission_profile() {
|
||||
async fn submission_includes_configured_active_permission_profile() {
|
||||
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
let thread_id = ThreadId::new();
|
||||
@@ -120,6 +120,7 @@ async fn submission_includes_configured_permission_profile() {
|
||||
},
|
||||
}
|
||||
.into();
|
||||
let expected_active_permission_profile = ActivePermissionProfile::new("custom");
|
||||
let configured = crate::session_state::ThreadSessionState {
|
||||
thread_id,
|
||||
forked_from_id: None,
|
||||
@@ -130,8 +131,8 @@ async fn submission_includes_configured_permission_profile() {
|
||||
service_tier: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: ApprovalsReviewer::User,
|
||||
permission_profile: expected_permission_profile.clone(),
|
||||
active_permission_profile: None,
|
||||
permission_profile: expected_permission_profile,
|
||||
active_permission_profile: Some(expected_active_permission_profile.clone()),
|
||||
cwd: test_path_buf("/home/user/project").abs(),
|
||||
runtime_workspace_roots: Vec::new(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
@@ -150,17 +151,21 @@ async fn submission_includes_configured_permission_profile() {
|
||||
);
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
let permission_profile = match next_submit_op(&mut op_rx) {
|
||||
let active_permission_profile = match next_submit_op(&mut op_rx) {
|
||||
Op::UserTurn {
|
||||
permission_profile, ..
|
||||
} => permission_profile,
|
||||
active_permission_profile,
|
||||
..
|
||||
} => active_permission_profile,
|
||||
other => panic!("expected Op::UserTurn, got {other:?}"),
|
||||
};
|
||||
assert_eq!(permission_profile, expected_permission_profile);
|
||||
assert_eq!(
|
||||
active_permission_profile,
|
||||
Some(expected_active_permission_profile)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn submission_keeps_profile_when_legacy_projection_is_external() {
|
||||
async fn submission_omits_active_permission_profile_for_legacy_snapshot() {
|
||||
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
let thread_id = ThreadId::new();
|
||||
@@ -180,7 +185,7 @@ async fn submission_keeps_profile_when_legacy_projection_is_external() {
|
||||
service_tier: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: ApprovalsReviewer::User,
|
||||
permission_profile: expected_permission_profile.clone(),
|
||||
permission_profile: expected_permission_profile,
|
||||
active_permission_profile: None,
|
||||
cwd: test_path_buf("/home/user/project").abs(),
|
||||
runtime_workspace_roots: Vec::new(),
|
||||
@@ -197,13 +202,14 @@ async fn submission_keeps_profile_when_legacy_projection_is_external() {
|
||||
.set_composer_text("submit".to_string(), Vec::new(), Vec::new());
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
let permission_profile = match next_submit_op(&mut op_rx) {
|
||||
let active_permission_profile = match next_submit_op(&mut op_rx) {
|
||||
Op::UserTurn {
|
||||
permission_profile, ..
|
||||
} => permission_profile,
|
||||
active_permission_profile,
|
||||
..
|
||||
} => active_permission_profile,
|
||||
other => panic!("expected Op::UserTurn, got {other:?}"),
|
||||
};
|
||||
assert_eq!(permission_profile, expected_permission_profile);
|
||||
assert_eq!(active_permission_profile, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -296,8 +296,11 @@ async fn session_configured_syncs_widget_config_permissions_and_cwd() {
|
||||
assert_eq!(&chat.config_ref().cwd, &expected_cwd);
|
||||
|
||||
let updated_profile = PermissionProfile::workspace_write();
|
||||
chat.set_permission_profile(updated_profile.clone())
|
||||
.expect("set permission profile");
|
||||
chat.set_permission_profile_from_session_snapshot(
|
||||
updated_profile.clone(),
|
||||
/*active_profile*/ None,
|
||||
)
|
||||
.expect("set permission profile");
|
||||
assert_eq!(
|
||||
chat.config_ref().permissions.permission_profile(),
|
||||
&updated_profile,
|
||||
|
||||
@@ -742,7 +742,9 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context
|
||||
cwd: None,
|
||||
approval_policy: Some(AskForApproval::OnRequest),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
permission_profile: Some(PermissionProfile::workspace_write()),
|
||||
active_permission_profile: Some(ActivePermissionProfile::new(
|
||||
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
|
||||
)),
|
||||
windows_sandbox_level: None,
|
||||
model: None,
|
||||
effort: None,
|
||||
@@ -752,6 +754,20 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context
|
||||
personality: None,
|
||||
}
|
||||
);
|
||||
|
||||
let active_permission_profile_update = std::iter::from_fn(|| rx.try_recv().ok())
|
||||
.find_map(|event| match event {
|
||||
AppEvent::UpdateActivePermissionProfile(active_permission_profile) => {
|
||||
Some(active_permission_profile)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.expect("expected UpdateActivePermissionProfile event");
|
||||
|
||||
assert_eq!(
|
||||
active_permission_profile_update,
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -42,10 +42,10 @@ impl ChatWidget {
|
||||
extra_count: usize,
|
||||
failed_scan: bool,
|
||||
) {
|
||||
let (approval, permission_profile) = match &preset {
|
||||
let (approval, active_permission_profile) = match &preset {
|
||||
Some(p) => (
|
||||
Some(AskForApproval::from(p.approval)),
|
||||
Some(p.permission_profile.clone()),
|
||||
Some(p.active_permission_profile.clone()),
|
||||
),
|
||||
None => (None, None),
|
||||
};
|
||||
@@ -110,10 +110,12 @@ impl ChatWidget {
|
||||
tx.send(AppEvent::SkipNextWorldWritableScan);
|
||||
}));
|
||||
}
|
||||
if let (Some(approval), Some(permission_profile)) = (approval, permission_profile.clone()) {
|
||||
if let (Some(approval), Some(active_permission_profile)) =
|
||||
(approval, active_permission_profile.clone())
|
||||
{
|
||||
accept_actions.extend(Self::approval_preset_actions(
|
||||
approval,
|
||||
permission_profile,
|
||||
active_permission_profile,
|
||||
mode_label.to_string(),
|
||||
ApprovalsReviewer::User,
|
||||
));
|
||||
@@ -124,10 +126,12 @@ impl ChatWidget {
|
||||
tx.send(AppEvent::UpdateWorldWritableWarningAcknowledged(true));
|
||||
tx.send(AppEvent::PersistWorldWritableWarningAcknowledged);
|
||||
}));
|
||||
if let (Some(approval), Some(permission_profile)) = (approval, permission_profile) {
|
||||
if let (Some(approval), Some(active_permission_profile)) =
|
||||
(approval, active_permission_profile)
|
||||
{
|
||||
accept_and_remember_actions.extend(Self::approval_preset_actions(
|
||||
approval,
|
||||
permission_profile,
|
||||
active_permission_profile,
|
||||
mode_label.to_string(),
|
||||
ApprovalsReviewer::User,
|
||||
));
|
||||
|
||||
Reference in New Issue
Block a user