mirror of
https://github.com/openai/codex.git
synced 2026-11-06 20:20:34 +00:00
Use permission profiles for protected-model startup downgrades (#59933)
## What changed Check the effective permission profile directly when applying startup restrictions for models that require automatic review, instead of converting it to a legacy sandbox policy. Preserve the downgrade to `workspace-write` for profiles with full disk write access and enabled network access, excluding external enforcement and writable paths that cannot be converted to local paths. Share writable-path conversion with the legacy sandbox adapter. ## Testing Add startup regression coverage for profiles that are downgraded, preserved, or rejected, including read-only carveouts and writable paths that cannot be converted locally. Verify restored permissions, automatic review selection, and workspace settings. GitOrigin-RevId: 256c949f23d84dfba185a45a01b638ff5f1df52e
This commit is contained in:
@@ -823,11 +823,17 @@ impl Session {
|
||||
.auto_review_required_for_model(&model)
|
||||
&& !trusted_guardian_reviewer
|
||||
{
|
||||
let config = Arc::make_mut(&mut config);
|
||||
if matches!(
|
||||
config.legacy_sandbox_policy(),
|
||||
SandboxPolicy::DangerFullAccess
|
||||
) {
|
||||
let permission_profile = config.permissions.effective_permission_profile();
|
||||
if permission_profile.enforcement() != SandboxEnforcement::External
|
||||
&& permission_profile.network_sandbox_policy().is_enabled()
|
||||
&& {
|
||||
let file_system = permission_profile.file_system_sandbox_policy();
|
||||
file_system.has_full_disk_write_access()
|
||||
// Preserve the legacy fallback for writable URIs that cannot be projected locally.
|
||||
&& file_system.entries.iter().all(|entry| entry.legacy_writable_path().is_ok())
|
||||
}
|
||||
{
|
||||
let config = Arc::make_mut(&mut config);
|
||||
let permission_profile = PermissionProfile::workspace_write();
|
||||
config
|
||||
.permissions
|
||||
|
||||
@@ -8,10 +8,17 @@ use codex_features::Feature;
|
||||
use codex_models_manager::model_info::model_info_from_slug;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::error::CodexErrorDetails;
|
||||
use codex_protocol::models::ManagedFileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::openai_models::ApprovalMessages;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::openai_models::PermissionMessages;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::FileSystemSpecialPath;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::CodexErrorInfo;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
@@ -19,6 +26,8 @@ use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ThreadSettingsOverrides;
|
||||
use codex_protocol::protocol::ThreadSettingsSnapshot;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
@@ -33,6 +42,173 @@ const PROTECTED_MODEL: &str = "gpt-5.2";
|
||||
const REVIEW_INSTRUCTIONS: &str = "Protected-model automatic review instructions.";
|
||||
const PERMISSION_INSTRUCTIONS: &str = "Protected-model read-only instructions.";
|
||||
|
||||
#[tokio::test]
|
||||
async fn protected_model_startup_preserves_legacy_permission_behavior() -> Result<()> {
|
||||
enum StartupExpectation {
|
||||
Preserve,
|
||||
Downgrade,
|
||||
Reject,
|
||||
}
|
||||
|
||||
skip_if_no_network!(Ok(()));
|
||||
let server = responses::start_mock_server().await;
|
||||
let root_write = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new(
|
||||
FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::Root,
|
||||
},
|
||||
FileSystemAccessMode::Write,
|
||||
)]);
|
||||
let mut carveout = root_write.clone();
|
||||
carveout.entries.push(FileSystemSandboxEntry::new(
|
||||
FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::Tmpdir,
|
||||
},
|
||||
FileSystemAccessMode::Read,
|
||||
));
|
||||
let mut unprojectable = root_write.clone();
|
||||
unprojectable.entries.push(FileSystemSandboxEntry::new(
|
||||
PathUri::parse("file:///%00/bad/path/YQ")?.into(),
|
||||
FileSystemAccessMode::Write,
|
||||
));
|
||||
let mut projectable = root_write.clone();
|
||||
let local_path = AbsolutePathBuf::try_from(std::env::current_dir()?)?;
|
||||
projectable.entries.push(FileSystemSandboxEntry::new(
|
||||
local_path.into(),
|
||||
FileSystemAccessMode::Write,
|
||||
));
|
||||
for (profile, expectation) in [
|
||||
(PermissionProfile::Disabled, StartupExpectation::Downgrade),
|
||||
(
|
||||
PermissionProfile::Managed {
|
||||
file_system: ManagedFileSystemPermissions::Unrestricted,
|
||||
network: NetworkSandboxPolicy::Enabled,
|
||||
},
|
||||
StartupExpectation::Downgrade,
|
||||
),
|
||||
(
|
||||
PermissionProfile::from_runtime_permissions(&root_write, NetworkSandboxPolicy::Enabled),
|
||||
StartupExpectation::Downgrade,
|
||||
),
|
||||
(
|
||||
PermissionProfile::from_runtime_permissions(
|
||||
&projectable,
|
||||
NetworkSandboxPolicy::Enabled,
|
||||
),
|
||||
StartupExpectation::Downgrade,
|
||||
),
|
||||
(
|
||||
PermissionProfile::from_runtime_permissions(
|
||||
&root_write,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
),
|
||||
StartupExpectation::Reject,
|
||||
),
|
||||
(
|
||||
PermissionProfile::External {
|
||||
network: NetworkSandboxPolicy::Enabled,
|
||||
},
|
||||
StartupExpectation::Reject,
|
||||
),
|
||||
(
|
||||
PermissionProfile::External {
|
||||
network: NetworkSandboxPolicy::Restricted,
|
||||
},
|
||||
StartupExpectation::Reject,
|
||||
),
|
||||
(PermissionProfile::read_only(), StartupExpectation::Preserve),
|
||||
(
|
||||
PermissionProfile::workspace_write(),
|
||||
StartupExpectation::Preserve,
|
||||
),
|
||||
(
|
||||
PermissionProfile::from_runtime_permissions(&carveout, NetworkSandboxPolicy::Enabled),
|
||||
StartupExpectation::Preserve,
|
||||
),
|
||||
(
|
||||
PermissionProfile::from_runtime_permissions(
|
||||
&unprojectable,
|
||||
NetworkSandboxPolicy::Enabled,
|
||||
),
|
||||
StartupExpectation::Reject,
|
||||
),
|
||||
] {
|
||||
let expected = match expectation {
|
||||
StartupExpectation::Preserve => Some(profile.clone()),
|
||||
StartupExpectation::Downgrade => Some(PermissionProfile::workspace_write()),
|
||||
StartupExpectation::Reject => None,
|
||||
};
|
||||
let case = format!("{profile:?}");
|
||||
let result = test_codex()
|
||||
.with_model(PROTECTED_MODEL)
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
"[auto_review]\nrequired_on_models = [\"gpt-5.2\"]\n",
|
||||
),
|
||||
)
|
||||
.with_config(move |config| {
|
||||
config.workspace_roots = vec![config.cwd.clone()];
|
||||
config
|
||||
.permissions
|
||||
.set_workspace_roots(config.workspace_roots.clone());
|
||||
config.permissions.approval_policy =
|
||||
Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile(profile)
|
||||
.expect("set permission profile");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::GuardianApproval)
|
||||
.expect("enable Guardian");
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await;
|
||||
if let Some(expected) = expected {
|
||||
let test = result?;
|
||||
let settings = test.codex.thread_settings_snapshot().await;
|
||||
let restored = test.codex.restorable_thread_settings().await;
|
||||
let environments = restored.environments.expect("captured environments");
|
||||
let environment = environments
|
||||
.environment_requests
|
||||
.first()
|
||||
.expect("selected executor environment");
|
||||
let actual = restored
|
||||
.permission_profile
|
||||
.expect("captured permission profile")
|
||||
.materialize_project_roots_with_path_uris(&environment.workspace_roots);
|
||||
// Keep foreign executor roots intact and derive the expectation from the fixture.
|
||||
let expected = expected.materialize_project_roots_with_path_uris(
|
||||
&test.executor_environment().selection().workspace_roots,
|
||||
);
|
||||
assert_eq!(
|
||||
(
|
||||
actual,
|
||||
settings.approvals_reviewer,
|
||||
settings.cwd,
|
||||
settings.runtime_workspace_roots
|
||||
),
|
||||
(
|
||||
expected,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
test.config.cwd.clone(),
|
||||
Some(test.config.workspace_roots.clone())
|
||||
),
|
||||
"{case}",
|
||||
);
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
} else {
|
||||
let error = result
|
||||
.err()
|
||||
.expect("protected model must reject full disk writes");
|
||||
assert!(
|
||||
error.to_string().contains("you need to use auto review"),
|
||||
"{case}: {error}"
|
||||
);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum SettingsOperation {
|
||||
Standalone,
|
||||
|
||||
@@ -226,6 +226,19 @@ impl FileSystemSandboxEntry {
|
||||
pub fn skips_missing_path(&self) -> bool {
|
||||
self.missing_path_behavior == Some(FileSystemSandboxEntryMissingPathBehavior::Skip)
|
||||
}
|
||||
|
||||
/// Projects an explicit writable path for the legacy native-path sandbox adapter.
|
||||
/// Special paths and glob patterns are handled separately by that adapter.
|
||||
pub fn legacy_writable_path(&self) -> io::Result<Option<AbsolutePathBuf>> {
|
||||
match &self.path {
|
||||
FileSystemPath::Path { path } if self.access.can_write() => {
|
||||
path.to_abs_path().map(Some)
|
||||
}
|
||||
FileSystemPath::Path { .. }
|
||||
| FileSystemPath::Special { .. }
|
||||
| FileSystemPath::GlobPattern { .. } => Ok(None),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(
|
||||
@@ -1880,9 +1893,8 @@ impl FileSystemSandboxPolicy {
|
||||
for entry in &self.entries {
|
||||
match &entry.path {
|
||||
FileSystemPath::GlobPattern { .. } => {}
|
||||
FileSystemPath::Path { path } => {
|
||||
if entry.access.can_write() {
|
||||
let path = path.to_abs_path()?;
|
||||
FileSystemPath::Path { .. } => {
|
||||
if let Some(path) = entry.legacy_writable_path()? {
|
||||
if cwd_absolute.as_ref().is_some_and(|cwd| cwd == &path) {
|
||||
workspace_root_writable = true;
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user