From e99e8e4a6bc2959266d4dd34c34e2b84e472ac52 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Wed, 11 Mar 2026 23:59:50 -0700 Subject: [PATCH] fix: follow up on linux sandbox review nits (#14440) ## Summary - address the follow-up review nits from #13996 in a separate PR - make the approvals test command a raw string and keep the managed-network path using env proxy routing - inline `--apply-seccomp-then-exec` in the Linux sandbox inner command builder - remove the bubblewrap-specific sandbox metric tag path and drop the `use_legacy_landlock` shim from `sandbox_tag`/`TurnMetadataState::new` - restore the `Feature` import that `origin/main` currently still needs in `connectors.rs` ## Testing - `cargo test -p codex-linux-sandbox` - focused `codex-core` tests were rerun/started, but the final verification pass was interrupted when I pushed at request --- codex-rs/core/src/codex.rs | 2 - codex-rs/core/src/connectors.rs | 1 + codex-rs/core/src/sandbox_tags.rs | 23 +++------ codex-rs/core/src/tools/registry.rs | 9 +--- codex-rs/core/src/turn_metadata.rs | 49 ++++---------------- codex-rs/core/tests/suite/approvals.rs | 5 +- codex-rs/linux-sandbox/src/linux_run_main.rs | 2 +- 7 files changed, 20 insertions(+), 71 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index fcc00fdf4..ef6d6b8be 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1290,7 +1290,6 @@ impl Session { cwd.clone(), session_configuration.sandbox_policy.get(), session_configuration.windows_sandbox_level, - per_turn_config.features.use_legacy_landlock(), )); let (current_date, timezone) = local_time_context(); TurnContext { @@ -5211,7 +5210,6 @@ async fn spawn_review_thread( parent_turn_context.cwd.clone(), parent_turn_context.sandbox_policy.get(), parent_turn_context.windows_sandbox_level, - parent_turn_context.features.use_legacy_landlock(), )); let review_turn_context = TurnContext { diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 92cc910c4..55318bc3d 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -31,6 +31,7 @@ use crate::config::types::AppsConfigToml; use crate::default_client::create_client; use crate::default_client::is_first_party_chat_originator; use crate::default_client::originator; +use crate::features::Feature; use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; use crate::mcp::McpManager; use crate::mcp::ToolPluginProvenance; diff --git a/codex-rs/core/src/sandbox_tags.rs b/codex-rs/core/src/sandbox_tags.rs index 4910d85f1..767d2bbf8 100644 --- a/codex-rs/core/src/sandbox_tags.rs +++ b/codex-rs/core/src/sandbox_tags.rs @@ -6,7 +6,6 @@ use codex_protocol::config_types::WindowsSandboxLevel; pub(crate) fn sandbox_tag( policy: &SandboxPolicy, windows_sandbox_level: WindowsSandboxLevel, - use_legacy_landlock: bool, ) -> &'static str { if matches!(policy, SandboxPolicy::DangerFullAccess) { return "none"; @@ -18,9 +17,6 @@ pub(crate) fn sandbox_tag( { return "windows_elevated"; } - if cfg!(target_os = "linux") && !use_legacy_landlock { - return "linux_bubblewrap"; - } get_platform_sandbox(windows_sandbox_level != WindowsSandboxLevel::Disabled) .map(SandboxType::as_metric_tag) @@ -38,41 +34,34 @@ mod tests { use pretty_assertions::assert_eq; #[test] - fn danger_full_access_is_untagged_even_when_bubblewrap_is_default() { + fn danger_full_access_is_untagged_even_when_linux_sandbox_defaults_apply() { let actual = sandbox_tag( &SandboxPolicy::DangerFullAccess, WindowsSandboxLevel::Disabled, - false, ); assert_eq!(actual, "none"); } #[test] - fn external_sandbox_keeps_external_tag_when_bubblewrap_is_default() { + fn external_sandbox_keeps_external_tag_when_linux_sandbox_defaults_apply() { let actual = sandbox_tag( &SandboxPolicy::ExternalSandbox { network_access: NetworkAccess::Enabled, }, WindowsSandboxLevel::Disabled, - false, ); assert_eq!(actual, "external"); } #[test] - fn bubblewrap_default_sets_distinct_linux_tag() { + fn default_linux_sandbox_uses_platform_sandbox_tag() { let actual = sandbox_tag( &SandboxPolicy::new_read_only_policy(), WindowsSandboxLevel::Disabled, - false, ); - let expected = if cfg!(target_os = "linux") { - "linux_bubblewrap" - } else { - get_platform_sandbox(false) - .map(SandboxType::as_metric_tag) - .unwrap_or("none") - }; + let expected = get_platform_sandbox(false) + .map(SandboxType::as_metric_tag) + .unwrap_or("none"); assert_eq!(actual, expected); } } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 088e3c836..fcf492192 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -173,7 +173,6 @@ impl ToolRegistry { sandbox_tag( &invocation.turn.sandbox_policy, invocation.turn.windows_sandbox_level, - invocation.turn.features.use_legacy_landlock(), ), ), ( @@ -498,12 +497,8 @@ async fn dispatch_after_tool_use_hook( success: dispatch.success, duration_ms: u64::try_from(dispatch.duration.as_millis()).unwrap_or(u64::MAX), mutating: dispatch.mutating, - sandbox: sandbox_tag( - &turn.sandbox_policy, - turn.windows_sandbox_level, - turn.features.use_legacy_landlock(), - ) - .to_string(), + sandbox: sandbox_tag(&turn.sandbox_policy, turn.windows_sandbox_level) + .to_string(), sandbox_policy: sandbox_policy_tag(&turn.sandbox_policy).to_string(), output_preview: dispatch.output_preview.clone(), }, diff --git a/codex-rs/core/src/turn_metadata.rs b/codex-rs/core/src/turn_metadata.rs index 0ddaad918..cb09e093b 100644 --- a/codex-rs/core/src/turn_metadata.rs +++ b/codex-rs/core/src/turn_metadata.rs @@ -132,12 +132,9 @@ impl TurnMetadataState { cwd: PathBuf, sandbox_policy: &SandboxPolicy, windows_sandbox_level: WindowsSandboxLevel, - use_legacy_landlock: bool, ) -> Self { let repo_root = get_git_repo_root(&cwd).map(|root| root.to_string_lossy().into_owned()); - let sandbox = Some( - sandbox_tag(sandbox_policy, windows_sandbox_level, use_legacy_landlock).to_string(), - ); + let sandbox = Some(sandbox_tag(sandbox_policy, windows_sandbox_level).to_string()); let base_metadata = build_turn_metadata_bag(Some(turn_id), sandbox, None, None); let base_header = base_metadata .to_header_value() @@ -295,53 +292,23 @@ mod tests { } #[test] - fn turn_metadata_state_respects_legacy_landlock_flag() { + fn turn_metadata_state_uses_platform_sandbox_tag() { let temp_dir = TempDir::new().expect("temp dir"); let cwd = temp_dir.path().to_path_buf(); let sandbox_policy = SandboxPolicy::new_read_only_policy(); - let default_bubblewrap = TurnMetadataState::new( + let state = TurnMetadataState::new( "turn-a".to_string(), - cwd.clone(), - &sandbox_policy, - WindowsSandboxLevel::Disabled, - false, - ); - let legacy_landlock = TurnMetadataState::new( - "turn-b".to_string(), cwd, &sandbox_policy, WindowsSandboxLevel::Disabled, - true, ); - let default_bubblewrap_header = default_bubblewrap - .current_header_value() - .expect("default_bubblewrap_header"); - let legacy_landlock_header = legacy_landlock - .current_header_value() - .expect("legacy_landlock_header"); + let header = state.current_header_value().expect("header"); + let json: Value = serde_json::from_str(&header).expect("json"); + let sandbox_name = json.get("sandbox").and_then(Value::as_str); - let default_bubblewrap_json: Value = - serde_json::from_str(&default_bubblewrap_header).expect("default_bubblewrap_json"); - let legacy_landlock_json: Value = - serde_json::from_str(&legacy_landlock_header).expect("legacy_landlock_json"); - - let default_bubblewrap_sandbox = default_bubblewrap_json - .get("sandbox") - .and_then(Value::as_str); - let legacy_landlock_sandbox = legacy_landlock_json.get("sandbox").and_then(Value::as_str); - - let expected_default_bubblewrap = - sandbox_tag(&sandbox_policy, WindowsSandboxLevel::Disabled, false); - assert_eq!( - default_bubblewrap_sandbox, - Some(expected_default_bubblewrap) - ); - - if cfg!(target_os = "linux") { - assert_eq!(default_bubblewrap_sandbox, Some("linux_bubblewrap")); - assert_ne!(default_bubblewrap_sandbox, legacy_landlock_sandbox); - } + let expected_sandbox = sandbox_tag(&sandbox_policy, WindowsSandboxLevel::Disabled); + assert_eq!(sandbox_name, Some(expected_sandbox)); } } diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 66c30f614..dc4f1b090 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -2298,9 +2298,8 @@ allow_local_binding = true let call_id_first = "allow-network-first"; // Use urllib without overriding proxy settings so managed-network sessions // continue to exercise the env-based proxy routing path under bubblewrap. - let fetch_command = - "python3 -c \"import urllib.request; opener = urllib.request.build_opener(urllib.request.ProxyHandler()); print('OK:' + opener.open('http://codex-network-test.invalid', timeout=30).read().decode(errors='replace'))\"" - .to_string(); + let fetch_command = r#"python3 -c "import urllib.request; opener = urllib.request.build_opener(urllib.request.ProxyHandler()); print('OK:' + opener.open('http://codex-network-test.invalid', timeout=30).read().decode(errors='replace'))""# + .to_string(); let first_event = shell_event( call_id_first, &fetch_command, diff --git a/codex-rs/linux-sandbox/src/linux_run_main.rs b/codex-rs/linux-sandbox/src/linux_run_main.rs index 9a5a4c738..a86e0128e 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -514,8 +514,8 @@ fn build_inner_seccomp_command(args: InnerSeccompCommandArgs<'_>) -> Vec file_system_policy_json, "--network-sandbox-policy".to_string(), network_policy_json, + "--apply-seccomp-then-exec".to_string(), ]; - inner.push("--apply-seccomp-then-exec".to_string()); if allow_network_for_proxy { inner.push("--allow-network-for-proxy".to_string()); let proxy_route_spec = proxy_route_spec