mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
permissions: derive snapshot sandbox projections (#19775)
## Why `ThreadConfigSnapshot` is used by app-server and thread metadata code as a stable view of active runtime settings. Keeping both `sandbox_policy` and `permission_profile` in the snapshot duplicates permission state and makes it possible for the legacy projection to drift from the canonical profile. The legacy `sandbox` value is still needed at app-server compatibility boundaries, so this PR derives it on demand from the snapshot profile and cwd instead of storing it. ## What Changed - Removes `ThreadConfigSnapshot.sandbox_policy`. - Adds `ThreadConfigSnapshot::sandbox_policy()` as a compatibility projection from `permission_profile` plus `cwd`. - Updates app-server response/metadata code and tests to call the projection only where legacy fields still exist. - Keeps snapshot construction profile-only so split filesystem rules, disabled enforcement, and external enforcement remain represented by the canonical profile. ## Verification - `cargo test -p codex-app-server thread_response_permission_profile_preserves_enforcement --lib` - `cargo test -p codex-core dispatch_reclaims_stale_global_lock_and_starts_consolidation --lib` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19775). * #19900 * #19899 * #19776 * __->__ #19775
This commit is contained in:
committed by
GitHub
Unverified
parent
bf38def44e
commit
fc2a69107c
@@ -2712,6 +2712,10 @@ impl CodexMessageProcessor {
|
||||
/*has_in_progress_turn*/ false,
|
||||
);
|
||||
|
||||
let sandbox = thread_response_sandbox_policy(
|
||||
&config_snapshot.permission_profile,
|
||||
config_snapshot.cwd.as_path(),
|
||||
);
|
||||
let permission_profile =
|
||||
thread_response_permission_profile(config_snapshot.permission_profile);
|
||||
|
||||
@@ -2724,7 +2728,7 @@ impl CodexMessageProcessor {
|
||||
instruction_sources,
|
||||
approval_policy: config_snapshot.approval_policy.into(),
|
||||
approvals_reviewer: config_snapshot.approvals_reviewer.into(),
|
||||
sandbox: config_snapshot.sandbox_policy.into(),
|
||||
sandbox,
|
||||
permission_profile,
|
||||
reasoning_effort: config_snapshot.reasoning_effort,
|
||||
};
|
||||
@@ -3284,7 +3288,7 @@ impl CodexMessageProcessor {
|
||||
builder.model_provider = Some(model_provider.clone());
|
||||
builder.cwd = config_snapshot.cwd.to_path_buf();
|
||||
builder.cli_version = Some(env!("CARGO_PKG_VERSION").to_string());
|
||||
builder.sandbox_policy = config_snapshot.sandbox_policy.clone();
|
||||
builder.sandbox_policy = config_snapshot.sandbox_policy();
|
||||
builder.approval_mode = config_snapshot.approval_policy;
|
||||
let metadata = builder.build(model_provider.as_str());
|
||||
if let Err(err) = state_db_ctx.insert_thread_if_absent(&metadata).await {
|
||||
@@ -8099,7 +8103,6 @@ async fn handle_pending_thread_resume_request(
|
||||
service_tier,
|
||||
approval_policy,
|
||||
approvals_reviewer,
|
||||
sandbox_policy: _,
|
||||
permission_profile,
|
||||
cwd,
|
||||
reasoning_effort,
|
||||
@@ -8323,8 +8326,9 @@ fn collect_resume_override_mismatches(
|
||||
}
|
||||
}
|
||||
if let Some(requested_sandbox) = request.sandbox.as_ref() {
|
||||
let active_sandbox = config_snapshot.sandbox_policy();
|
||||
let sandbox_matches = matches!(
|
||||
(requested_sandbox, &config_snapshot.sandbox_policy),
|
||||
(requested_sandbox, &active_sandbox),
|
||||
(
|
||||
SandboxMode::ReadOnly,
|
||||
codex_protocol::protocol::SandboxPolicy::ReadOnly { .. }
|
||||
@@ -8341,8 +8345,7 @@ fn collect_resume_override_mismatches(
|
||||
);
|
||||
if !sandbox_matches {
|
||||
mismatch_details.push(format!(
|
||||
"sandbox requested={requested_sandbox:?} active={:?}",
|
||||
config_snapshot.sandbox_policy
|
||||
"sandbox requested={requested_sandbox:?} active={active_sandbox:?}"
|
||||
));
|
||||
}
|
||||
}
|
||||
@@ -10042,7 +10045,6 @@ mod tests {
|
||||
service_tier: Some(codex_protocol::config_types::ServiceTier::Flex),
|
||||
approval_policy: codex_protocol::protocol::AskForApproval::OnRequest,
|
||||
approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer::User,
|
||||
sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess,
|
||||
permission_profile: codex_protocol::models::PermissionProfile::Disabled,
|
||||
cwd,
|
||||
ephemeral: false,
|
||||
|
||||
@@ -47,7 +47,6 @@ pub struct ThreadConfigSnapshot {
|
||||
pub service_tier: Option<ServiceTier>,
|
||||
pub approval_policy: AskForApproval,
|
||||
pub approvals_reviewer: ApprovalsReviewer,
|
||||
pub sandbox_policy: SandboxPolicy,
|
||||
pub permission_profile: PermissionProfile,
|
||||
pub cwd: AbsolutePathBuf,
|
||||
pub ephemeral: bool,
|
||||
@@ -56,6 +55,18 @@ pub struct ThreadConfigSnapshot {
|
||||
pub session_source: SessionSource,
|
||||
}
|
||||
|
||||
impl ThreadConfigSnapshot {
|
||||
pub fn sandbox_policy(&self) -> SandboxPolicy {
|
||||
let file_system_sandbox_policy = self.permission_profile.file_system_sandbox_policy();
|
||||
codex_sandboxing::compatibility_sandbox_policy_for_permission_profile(
|
||||
&self.permission_profile,
|
||||
&file_system_sandbox_policy,
|
||||
self.permission_profile.network_sandbox_policy(),
|
||||
self.cwd.as_path(),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/// Turn context overrides that app-server validates before starting a turn.
|
||||
#[derive(Clone, Default)]
|
||||
pub struct CodexThreadTurnContextOverrides {
|
||||
|
||||
@@ -883,11 +883,11 @@ mod phase2 {
|
||||
config_snapshot.cwd.as_path(),
|
||||
memory_root(&harness.config.codex_home).as_path()
|
||||
);
|
||||
match &config_snapshot.sandbox_policy {
|
||||
let sandbox_policy = config_snapshot.sandbox_policy();
|
||||
match &sandbox_policy {
|
||||
SandboxPolicy::WorkspaceWrite { network_access, .. } => {
|
||||
assert!(!*network_access);
|
||||
let effective_writable_roots: Vec<_> = config_snapshot
|
||||
.sandbox_policy
|
||||
let effective_writable_roots: Vec<_> = sandbox_policy
|
||||
.get_writable_roots_with_cwd(config_snapshot.cwd.as_path())
|
||||
.into_iter()
|
||||
.map(|root| root.root)
|
||||
@@ -917,7 +917,7 @@ mod phase2 {
|
||||
let file_system_sandbox_policy = turn_context.file_system_sandbox_policy();
|
||||
let legacy_file_system_sandbox_policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(
|
||||
&config_snapshot.sandbox_policy,
|
||||
&sandbox_policy,
|
||||
config_snapshot.cwd.as_path(),
|
||||
);
|
||||
assert!(
|
||||
|
||||
@@ -126,7 +126,6 @@ impl SessionConfiguration {
|
||||
service_tier: self.service_tier,
|
||||
approval_policy: self.approval_policy.value(),
|
||||
approvals_reviewer: self.approvals_reviewer,
|
||||
sandbox_policy: self.sandbox_policy(),
|
||||
permission_profile: self.permission_profile(),
|
||||
cwd: self.cwd.clone(),
|
||||
ephemeral: self.original_config_do_not_use.ephemeral,
|
||||
|
||||
@@ -2143,7 +2143,7 @@ async fn spawn_agent_reapplies_runtime_sandbox_after_role_config() {
|
||||
.expect("spawned agent thread should exist")
|
||||
.config_snapshot()
|
||||
.await;
|
||||
assert_eq!(snapshot.sandbox_policy, expected_sandbox);
|
||||
assert_eq!(snapshot.sandbox_policy(), expected_sandbox);
|
||||
assert_eq!(snapshot.approval_policy, AskForApproval::OnRequest);
|
||||
assert_eq!(snapshot.permission_profile, expected_permission_profile);
|
||||
let child_thread = manager
|
||||
|
||||
Reference in New Issue
Block a user