Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions codex-rs/core/src/config/config_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1453,6 +1453,30 @@ async fn permission_profile_override_populates_runtime_permissions() -> std::io:
Ok(())
}

#[test]
fn permission_snapshot_setter_preserves_permission_constraints() {
let initial_profile = PermissionProfile::read_only();
let mut permissions = Permissions::from_approval_and_profile(
Constrained::allow_any(AskForApproval::Never),
Constrained::allow_only(initial_profile.clone()),
)
.expect("initial permissions should satisfy constraints");

let err = permissions
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::workspace_write(),
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
))
.expect_err("workspace profile should violate read-only constraint");

assert_eq!(permissions.permission_profile(), &initial_profile);
assert_eq!(permissions.active_permission_profile(), None);
assert!(
matches!(err, ConstraintError::InvalidValue { .. }),
"expected invalid value constraint error, got {err:?}"
);
}

#[tokio::test]
async fn permission_profile_override_preserves_managed_unrestricted_filesystem()
-> std::io::Result<()> {
Expand Down
48 changes: 9 additions & 39 deletions codex-rs/core/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ pub use managed_features::ManagedFeatures;
pub use network_proxy_spec::NetworkProxySpec;
pub use network_proxy_spec::StartedNetworkProxy;
pub(crate) use permissions::resolve_permission_profile;
pub use resolved_permission_profile::PermissionProfileSnapshot;
pub(crate) use resolved_permission_profile::PermissionProfileState;

const DEFAULT_IGNORE_LARGE_UNTRACKED_DIRS: i64 = 200;
Expand Down Expand Up @@ -311,57 +312,26 @@ impl Permissions {
///
/// This is a trusted-state bridge for consumers of `SessionConfigured`.
/// Config loading and app-server selection should resolve named profiles
/// through config instead of constructing this pair directly.
/// through config instead of constructing a snapshot directly.
pub fn set_permission_profile_from_session_snapshot(
&mut self,
permission_profile: PermissionProfile,
active_permission_profile: Option<ActivePermissionProfile>,
snapshot: PermissionProfileSnapshot,
) -> ConstraintResult<()> {
self.set_permission_profile_from_session_snapshot_with_profile_workspace_roots(
permission_profile,
active_permission_profile,
Vec::new(),
)
}

pub fn set_permission_profile_from_session_snapshot_with_profile_workspace_roots(
&mut self,
permission_profile: PermissionProfile,
active_permission_profile: Option<ActivePermissionProfile>,
profile_workspace_roots: Vec<AbsolutePathBuf>,
) -> ConstraintResult<()> {
self.permission_profile_state.set_active_permission_profile(
permission_profile,
active_permission_profile,
profile_workspace_roots,
)
self.permission_profile_state
.set_permission_profile_snapshot(snapshot)
}

/// Replace the current permission constraints with a trusted session
/// snapshot. This is only for clients that must mirror core session state
/// after their local config constraints reject the snapshot.
pub fn replace_permission_profile_from_session_snapshot(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This still leaves the replacement path with two sources of truth: callers pass both a constrained PermissionProfile and a PermissionProfileSnapshot. Since the only current caller builds Constrained::allow_only(session.permission_profile.clone()), could this API take only the snapshot and derive that constraint internally? That would make the “snapshot is the coherent unit” story complete here too.

Suggested change
pub fn replace_permission_profile_from_session_snapshot(
pub fn replace_permission_profile_from_session_snapshot(
&mut self,
snapshot: PermissionProfileSnapshot,
) -> ConstraintResult<()> {
let permission_profile =
Constrained::allow_only(snapshot.permission_profile().clone());
self.permission_profile_state = PermissionProfileState::from_constrained_resolved(
permission_profile,
snapshot.into_resolved_permission_profile(),
)?;
Ok(())
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in the latest revision: replace_permission_profile_from_session_snapshot() now takes only PermissionProfileSnapshot and derives Constrained::allow_only(snapshot.permission_profile().clone()) internally, so the replacement path uses the snapshot as the single coherent source.

&mut self,
permission_profile: Constrained<PermissionProfile>,
active_permission_profile: Option<ActivePermissionProfile>,
) -> ConstraintResult<()> {
self.replace_permission_profile_from_session_snapshot_with_profile_workspace_roots(
permission_profile,
active_permission_profile,
Vec::new(),
)
}

pub fn replace_permission_profile_from_session_snapshot_with_profile_workspace_roots(
&mut self,
permission_profile: Constrained<PermissionProfile>,
active_permission_profile: Option<ActivePermissionProfile>,
profile_workspace_roots: Vec<AbsolutePathBuf>,
snapshot: PermissionProfileSnapshot,
) -> ConstraintResult<()> {
self.permission_profile_state = PermissionProfileState::from_constrained_active_profile(
let permission_profile = Constrained::allow_only(snapshot.permission_profile().clone());
self.permission_profile_state = PermissionProfileState::from_constrained_resolved(
permission_profile,
active_permission_profile,
profile_workspace_roots,
snapshot.into_resolved_permission_profile(),
)?;
Ok(())
}
Expand Down
114 changes: 104 additions & 10 deletions codex-rs/core/src/config/resolved_permission_profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,19 @@ pub(crate) enum ResolvedPermissionProfile {
Named(NamedPermissionProfile),
}

/// Trusted snapshot of a resolved permission profile.
///
/// This is a bridge for already-resolved session/config state. It keeps the
/// concrete `PermissionProfile`, optional active profile id, and
/// profile-defined workspace roots together so `Permissions` can validate and
/// install them atomically. It is not a resolver: callers that are handling
/// user-selected profile ids should resolve those ids through config instead
/// of constructing this type directly.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct PermissionProfileSnapshot {
resolved_permission_profile: ResolvedPermissionProfile,
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct LegacyPermissionProfile {
permission_profile: PermissionProfile,
Expand Down Expand Up @@ -124,6 +137,93 @@ impl ResolvedPermissionProfile {
}
}

impl PermissionProfileSnapshot {
/// Create a snapshot with no active profile id.
///
/// Prefer this only for legacy data or local overrides that genuinely do
/// not have a named/built-in profile identity. Using this for a built-in or
/// named profile will intentionally clear the active profile metadata.
pub fn legacy(permission_profile: PermissionProfile) -> Self {
Self {
resolved_permission_profile: ResolvedPermissionProfile::legacy(permission_profile),
}
}

/// Create a snapshot for a known active profile id.
///
/// Use this only after a trusted caller has already resolved the active id
/// to the supplied concrete `PermissionProfile`. This constructor does not
/// verify that the id and profile match; `Permissions` will still enforce
/// configured permission constraints when the snapshot is installed.
pub fn active(
permission_profile: PermissionProfile,
active_permission_profile: ActivePermissionProfile,
) -> Self {
Self::active_with_profile_workspace_roots(
permission_profile,
active_permission_profile,
Vec::new(),
)
}

/// Create a snapshot for a known active profile id with profile roots.
///
/// As with `active`, the caller is responsible for passing the concrete
/// profile and active id that were resolved together. Use this variant when
/// the selected profile declared workspace roots that should remain
/// distinct from turn-scoped runtime workspace roots.
pub fn active_with_profile_workspace_roots(
permission_profile: PermissionProfile,
active_permission_profile: ActivePermissionProfile,
profile_workspace_roots: Vec<AbsolutePathBuf>,
) -> Self {
Self {
resolved_permission_profile: ResolvedPermissionProfile::from_active_profile(
permission_profile,
Some(active_permission_profile),
profile_workspace_roots,
),
}
}

/// Reconstruct a trusted snapshot from session state.
///
/// This is intended for session responses emitted by core, where the
/// concrete profile and active profile id were captured together. Avoid
/// using this as a shortcut for arbitrary user input because mismatched
/// arguments can still misrepresent the active profile identity.
pub fn from_session_snapshot(
permission_profile: PermissionProfile,
active_permission_profile: Option<ActivePermissionProfile>,
) -> Self {
match active_permission_profile {
Some(active_permission_profile) => {
Self::active(permission_profile, active_permission_profile)
}
None => Self::legacy(permission_profile),
}
}

/// Borrow the concrete permission profile captured in this snapshot.
pub fn permission_profile(&self) -> &PermissionProfile {
self.resolved_permission_profile.permission_profile()
}

/// Return the active profile id captured in this snapshot, if any.
pub fn active_permission_profile(&self) -> Option<ActivePermissionProfile> {
self.resolved_permission_profile.active_permission_profile()
}

/// Borrow profile-declared workspace roots captured in this snapshot.
pub fn profile_workspace_roots(&self) -> &[AbsolutePathBuf] {
self.resolved_permission_profile.profile_workspace_roots()
}

pub(crate) fn into_resolved_permission_profile(self) -> ResolvedPermissionProfile {
self.resolved_permission_profile
}
}

#[derive(Debug, Clone, PartialEq)]
pub(crate) struct PermissionProfileState {
resolved_permission_profile: Constrained<ResolvedPermissionProfile>,
Expand Down Expand Up @@ -199,17 +299,11 @@ impl PermissionProfileState {
.set(ResolvedPermissionProfile::legacy(permission_profile))
}

pub(crate) fn set_active_permission_profile(
pub(crate) fn set_permission_profile_snapshot(
&mut self,
permission_profile: PermissionProfile,
active_permission_profile: Option<ActivePermissionProfile>,
profile_workspace_roots: Vec<AbsolutePathBuf>,
snapshot: PermissionProfileSnapshot,
) -> ConstraintResult<()> {
let candidate = ResolvedPermissionProfile::from_active_profile(
permission_profile,
active_permission_profile,
profile_workspace_roots,
);
self.resolved_permission_profile.set(candidate)
self.resolved_permission_profile
.set(snapshot.into_resolved_permission_profile())
}
}
1 change: 1 addition & 0 deletions codex-rs/core/src/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ use crate::compact::collect_user_messages;
use crate::config::Config;
use crate::config::Constrained;
use crate::config::ConstraintResult;
use crate::config::PermissionProfileSnapshot;
use crate::config::PermissionProfileState;
use crate::config::StartedNetworkProxy;
use crate::config::resolve_web_search_mode_for_turn;
Expand Down
19 changes: 14 additions & 5 deletions codex-rs/core/src/session/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,11 +357,20 @@ impl SessionConfiguration {
&file_system_sandbox_policy,
network_sandbox_policy,
);
self.permission_profile_state.set_active_permission_profile(
effective_permission_profile,
active_permission_profile,
profile_workspace_roots,
)

let permission_snapshot = match active_permission_profile {
Some(active_permission_profile) => {
PermissionProfileSnapshot::active_with_profile_workspace_roots(
effective_permission_profile,
active_permission_profile,
profile_workspace_roots,
)
}
None => PermissionProfileSnapshot::legacy(effective_permission_profile),
};

self.permission_profile_state
.set_permission_profile_snapshot(permission_snapshot)
}
}

Expand Down
1 change: 1 addition & 0 deletions codex-rs/tui/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ use crate::keymap::RuntimeKeymap;
use crate::legacy_core::config::Config;
use crate::legacy_core::config::ConfigBuilder;
use crate::legacy_core::config::ConfigOverrides;
use crate::legacy_core::config::PermissionProfileSnapshot;
use crate::legacy_core::config::edit::ConfigEdit;
use crate::legacy_core::config::edit::ConfigEditsBuilder;
#[cfg(target_os = "windows")]
Expand Down
12 changes: 7 additions & 5 deletions codex-rs/tui/src/app/config_persistence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,10 @@ impl App {

if let Err(err) = config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
permission_profile.clone(),
Some(active_permission_profile),
)
active_permission_profile,
))
{
tracing::warn!(error = %err, "{log_message}");
self.chat_widget
Expand Down Expand Up @@ -328,8 +328,10 @@ impl App {
&& let Err(err) = self
.chat_widget
.set_permission_profile_from_session_snapshot(
permission_profile.clone(),
active_permission_profile_override.clone(),
PermissionProfileSnapshot::from_session_snapshot(
permission_profile.clone(),
active_permission_profile_override.clone(),
),
)
{
tracing::error!(
Expand Down
6 changes: 4 additions & 2 deletions codex-rs/tui/src/app/event_dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1422,8 +1422,10 @@ impl App {
if let Err(err) = self
.chat_widget
.set_permission_profile_from_session_snapshot(
permission_profile_for_chat,
Some(active_permission_profile),
PermissionProfileSnapshot::active(
permission_profile_for_chat,
active_permission_profile,
),
)
{
tracing::warn!(%err, "failed to set permission profile on chat config");
Expand Down
11 changes: 5 additions & 6 deletions codex-rs/tui/src/app/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use crate::app_command::AppCommand as Op;
use crate::diff_model::FileChange;
use crate::legacy_core::config::ConfigBuilder;
use crate::legacy_core::config::ConfigOverrides;
use crate::legacy_core::config::PermissionProfileSnapshot;
use crate::legacy_core::config::TerminalResizeReflowMaxRows;
use codex_app_server_protocol::AdditionalFileSystemPermissions;
use codex_app_server_protocol::AdditionalNetworkPermissions;
Expand Down Expand Up @@ -1730,10 +1731,9 @@ async fn update_feature_flags_disabling_guardian_clears_review_policy_and_restor
app.chat_widget
.set_approval_policy(AskForApproval::OnRequest);
app.chat_widget
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy(
PermissionProfile::workspace_write(),
/*active_profile*/ None,
)?;
))?;

app.update_feature_flags(vec![(Feature::GuardianApproval, false)])
.await;
Expand Down Expand Up @@ -3153,10 +3153,9 @@ async fn side_fork_config_inherits_parent_thread_runtime_settings() {
app.chat_widget
.set_approval_policy(AskForApproval::OnRequest);
app.chat_widget
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy(
parent_permission_profile.clone(),
/*active_profile*/ None,
)
))
.expect("test permission profile should be accepted");
app.chat_widget
.set_approvals_reviewer(ApprovalsReviewer::AutoReview);
Expand Down
7 changes: 4 additions & 3 deletions codex-rs/tui/src/app/thread_session_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ mod tests {
use crate::app::side::SideThreadState;
use crate::app::test_support::make_test_app;
use crate::app::thread_events::ThreadEventChannel;
use crate::legacy_core::config::PermissionProfileSnapshot;
use crate::test_support::PathBufExt;
use crate::test_support::test_path_buf;
use codex_app_server_protocol::AskForApproval;
Expand Down Expand Up @@ -201,10 +202,10 @@ mod tests {
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE);
app.chat_widget.handle_thread_session(main_session.clone());
app.chat_widget
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
expected_permission_profile.clone(),
Some(expected_active_permission_profile.clone()),
)
expected_active_permission_profile.clone(),
))
.expect("set widget permission profile");
app.config
.permissions
Expand Down
1 change: 1 addition & 0 deletions codex-rs/tui/src/chatwidget.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ use crate::legacy_core::DEFAULT_AGENTS_MD_FILENAME;
use crate::legacy_core::config::Config;
use crate::legacy_core::config::Constrained;
use crate::legacy_core::config::ConstraintResult;
use crate::legacy_core::config::PermissionProfileSnapshot;
#[cfg(target_os = "windows")]
use crate::legacy_core::windows_sandbox::WindowsSandboxLevelExt;
use crate::mention_codec::LinkedMention;
Expand Down
Loading
Loading