Merge 72d41e5db2 into sapling-pr-archive-bolinfest

This commit is contained in:
Michael Bolin
2026-05-15 16:33:04 -07:00
committed by GitHub
15 changed files with 224 additions and 132 deletions
+24
View File
@@ -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<()> {
+8 -38
View File
@@ -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;
@@ -311,30 +312,13 @@ 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
@@ -343,25 +327,11 @@ impl Permissions {
pub fn replace_permission_profile_from_session_snapshot(
&mut self,
permission_profile: Constrained<PermissionProfile>,
active_permission_profile: Option<ActivePermissionProfile>,
snapshot: PermissionProfileSnapshot,
) -> ConstraintResult<()> {
self.replace_permission_profile_from_session_snapshot_with_profile_workspace_roots(
self.permission_profile_state = PermissionProfileState::from_constrained_resolved(
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>,
) -> ConstraintResult<()> {
self.permission_profile_state = PermissionProfileState::from_constrained_active_profile(
permission_profile,
active_permission_profile,
profile_workspace_roots,
snapshot.into_resolved_permission_profile(),
)?;
Ok(())
}
@@ -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,
@@ -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>,
@@ -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
View File
@@ -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;
+14 -5
View File
@@ -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)
}
}
+1
View File
@@ -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")]
+7 -5
View File
@@ -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
@@ -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!(
+4 -2
View File
@@ -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");
+5 -6
View File
@@ -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;
@@ -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;
@@ -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);
+4 -3
View File
@@ -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;
@@ -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
+1
View File
@@ -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;
+6 -5
View File
@@ -49,13 +49,14 @@ impl ChatWidget {
self.config.permissions.approval_policy =
Constrained::allow_only(session.approval_policy.to_core());
}
let permission_snapshot = PermissionProfileSnapshot::from_session_snapshot(
session.permission_profile.clone(),
session.active_permission_profile.clone(),
);
let permission_sync = self
.config
.permissions
.set_permission_profile_from_session_snapshot(
session.permission_profile.clone(),
session.active_permission_profile.clone(),
);
.set_permission_profile_from_session_snapshot(permission_snapshot.clone());
if let Err(err) = permission_sync {
tracing::warn!(%err, "failed to sync permissions from SessionConfigured");
if let Err(replace_err) = self
@@ -63,7 +64,7 @@ impl ChatWidget {
.permissions
.replace_permission_profile_from_session_snapshot(
Constrained::allow_only(session.permission_profile.clone()),
session.active_permission_profile.clone(),
permission_snapshot,
)
{
tracing::error!(
+2 -3
View File
@@ -20,12 +20,11 @@ impl ChatWidget {
#[cfg_attr(not(target_os = "windows"), allow(dead_code))]
pub(crate) fn set_permission_profile_from_session_snapshot(
&mut self,
profile: PermissionProfile,
active_profile: Option<ActivePermissionProfile>,
snapshot: PermissionProfileSnapshot,
) -> ConstraintResult<()> {
self.config
.permissions
.set_permission_profile_from_session_snapshot(profile, active_profile)?;
.set_permission_profile_from_session_snapshot(snapshot)?;
self.refresh_status_surfaces();
Ok(())
}
@@ -292,10 +292,9 @@ 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_from_session_snapshot(
chat.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy(
updated_profile.clone(),
/*active_profile*/ None,
)
))
.expect("set permission profile");
assert_eq!(
chat.config_ref().permissions.permission_profile(),
+41 -52
View File
@@ -5,6 +5,7 @@ use super::rate_limit_snapshot_display;
use crate::history_cell::HistoryCell;
use crate::legacy_core::config::Config;
use crate::legacy_core::config::ConfigBuilder;
use crate::legacy_core::config::PermissionProfileSnapshot;
use crate::status::StatusAccountDisplay;
use crate::test_support::PathBufExt;
use crate::test_support::test_path_buf;
@@ -305,12 +306,10 @@ async fn status_permissions_named_read_only_profile_shows_builtin_label() {
.expect("set approval policy");
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::read_only(),
Some(ActivePermissionProfile::new(
BUILT_IN_PERMISSION_PROFILE_READ_ONLY,
)),
)
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_READ_ONLY),
))
.expect("set permission profile");
assert_eq!(
@@ -334,15 +333,13 @@ async fn status_permissions_read_only_profile_shows_additional_writable_roots()
.with_additional_writable_roots(config.cwd.as_path(), std::slice::from_ref(&extra_root));
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::from_runtime_permissions(
&file_system_policy,
NetworkSandboxPolicy::Restricted,
),
Some(ActivePermissionProfile::new(
BUILT_IN_PERMISSION_PROFILE_READ_ONLY,
)),
)
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_READ_ONLY),
))
.expect("set permission profile");
assert_eq!(
@@ -362,12 +359,10 @@ async fn status_permissions_named_workspace_profile_shows_builtin_label() {
.expect("set approval policy");
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::workspace_write(),
Some(ActivePermissionProfile::new(
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
)),
)
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
))
.expect("set permission profile");
assert_eq!(
@@ -388,12 +383,10 @@ async fn status_permissions_workspace_auto_review_shows_reviewer_label() {
.expect("set approval policy");
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::workspace_write(),
Some(ActivePermissionProfile::new(
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
)),
)
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
))
.expect("set permission profile");
assert_eq!(
@@ -414,17 +407,15 @@ async fn status_permissions_named_profile_shows_additional_writable_roots() {
let extra_root = test_path_buf("/workspace/extra").abs();
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::workspace_write_with(
std::slice::from_ref(&extra_root),
NetworkSandboxPolicy::Restricted,
/*exclude_tmpdir_env_var*/ false,
/*exclude_slash_tmp*/ false,
),
Some(ActivePermissionProfile::new(
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
)),
)
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
))
.expect("set permission profile");
assert_eq!(
@@ -450,10 +441,10 @@ async fn status_permissions_workspace_roots_show_additional_directories() {
.set_workspace_roots(config.workspace_roots.clone());
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::workspace_write(),
Some(ActivePermissionProfile::new(":workspace")),
)
ActivePermissionProfile::new(":workspace"),
))
.expect("set permission profile");
assert_eq!(
@@ -475,15 +466,17 @@ async fn status_permissions_workspace_roots_include_profile_defined_directories(
let profile_root = test_path_buf("/workspace/shared").abs();
config
.permissions
.set_permission_profile_from_session_snapshot_with_profile_workspace_roots(
PermissionProfile::workspace_write_with(
std::slice::from_ref(&profile_root),
NetworkSandboxPolicy::Restricted,
/*exclude_tmpdir_env_var*/ false,
/*exclude_slash_tmp*/ false,
.set_permission_profile_from_session_snapshot(
PermissionProfileSnapshot::active_with_profile_workspace_roots(
PermissionProfile::workspace_write_with(
std::slice::from_ref(&profile_root),
NetworkSandboxPolicy::Restricted,
/*exclude_tmpdir_env_var*/ false,
/*exclude_slash_tmp*/ false,
),
ActivePermissionProfile::new(":workspace"),
vec![profile_root.clone()],
),
Some(ActivePermissionProfile::new(":workspace")),
vec![profile_root.clone()],
)
.expect("set permission profile");
@@ -507,17 +500,15 @@ async fn status_permissions_broadened_workspace_profile_shows_builtin_label() {
.expect("set approval policy");
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::workspace_write_with(
&[],
NetworkSandboxPolicy::Enabled,
/*exclude_tmpdir_env_var*/ false,
/*exclude_slash_tmp*/ false,
),
Some(ActivePermissionProfile::new(
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
)),
)
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
))
.expect("set permission profile");
assert_eq!(
@@ -532,10 +523,10 @@ async fn status_permissions_user_defined_profile_shows_name() {
let mut config = test_config(&temp_home).await;
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::read_only(),
Some(ActivePermissionProfile::new("locked")),
)
ActivePermissionProfile::new("locked"),
))
.expect("set permission profile");
assert_eq!(
@@ -552,10 +543,10 @@ async fn status_snapshot_shows_active_user_defined_profile() {
set_workspace_cwd(&mut config, test_path_buf("/workspace/tests").abs());
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::read_only(),
Some(ActivePermissionProfile::new("locked")),
)
ActivePermissionProfile::new("locked"),
))
.expect("set permission profile");
let usage = TokenUsage::default();
@@ -650,12 +641,10 @@ async fn status_snapshot_shows_auto_review_permissions() {
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config
.permissions
.set_permission_profile_from_session_snapshot(
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
PermissionProfile::workspace_write(),
Some(ActivePermissionProfile::new(
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
)),
)
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
))
.expect("set permission profile");
let usage = TokenUsage::default();