From 3360f128f46f2aed392dea0d9557d0bb9536d2c2 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 26 Mar 2026 11:52:59 -0700 Subject: [PATCH 1/6] [plugins] Polish tool suggest prompts. (#15891) - [x] Polish tool suggest prompts to distinguish between missing connectors and discoverable plugins, and be very precise about the triggering conditions. --- codex-rs/core/src/tools/spec_tests.rs | 30 ++++++++++++++----- .../search_tool/tool_suggest_description.md | 7 +++-- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index bab44764f0..6f0e7496b8 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -2090,12 +2090,12 @@ fn tool_suggest_can_be_registered_without_search_tool() { let ToolSpec::Function(ResponsesApiTool { description, .. }) = &tool_suggest.spec else { panic!("expected function tool"); }; - assert!( - description.contains( - "You've already tried to find a matching available tool for the user's request" - ) - ); - assert!(description.contains("This includes `tool_search` (if available) and other means.")); + assert!(description.contains( + "Suggests a missing connector in an installed plugin, or in narrower cases a not installed but discoverable plugin" + )); + assert!(description.contains( + "You've already tried to find a matching available tool for the user's request but couldn't find a good match. This includes `tool_search` (if available) and other means." + )); } #[test] @@ -2343,6 +2343,9 @@ fn tool_suggest_description_lists_discoverable_tools() { else { panic!("expected function tool"); }; + assert!(description.contains( + "Suggests a missing connector in an installed plugin, or in narrower cases a not installed but discoverable plugin" + )); assert!(description.contains("Google Calendar")); assert!(description.contains("Gmail")); assert!(description.contains("Sample Plugin")); @@ -2355,10 +2358,21 @@ fn tool_suggest_description_lists_discoverable_tools() { ); assert!( description.contains( - "You've already tried to find a matching available tool for the user's request" + "You've already tried to find a matching available tool for the user's request but couldn't find a good match. This includes `tool_search` (if available) and other means." ) ); - assert!(description.contains("This includes `tool_search` (if available) and other means.")); + assert!(description.contains( + "For connectors/apps that are not installed but needed for an installed plugin, suggest to install them if the task requirements match precisely." + )); + assert!(description.contains( + "For plugins that are not installed but discoverable, only suggest discoverable and installable plugins when the user's intent very explicitly and unambiguously matches that plugin itself." + )); + assert!(description.contains( + "Do not suggest a plugin just because one of its connectors or capabilities seems relevant." + )); + assert!(description.contains( + "Apply the stricter explicit-and-unambiguous rule for *discoverable tools* like plugin install suggestions; *missing tools* like connector install suggestions continue to use the normal clear-fit standard." + )); assert!(description.contains("DO NOT explore or recommend tools that are not on this list.")); assert!(!description.contains("tool_search fails to find a good match")); let JsonSchema::Object { required, .. } = parameters else { diff --git a/codex-rs/core/templates/search_tool/tool_suggest_description.md b/codex-rs/core/templates/search_tool/tool_suggest_description.md index 75e58958b5..ad0a50fcc1 100644 --- a/codex-rs/core/templates/search_tool/tool_suggest_description.md +++ b/codex-rs/core/templates/search_tool/tool_suggest_description.md @@ -1,10 +1,11 @@ # Tool suggestion discovery -Suggests a discoverable connector or plugin when the user clearly wants a capability that is not currently available in the active `tools` list. +Suggests a missing connector in an installed plugin, or in narrower cases a not installed but discoverable plugin, when the user clearly wants a capability that is not currently available in the active `tools` list. Use this ONLY when: - You've already tried to find a matching available tool for the user's request but couldn't find a good match. This includes `tool_search` (if available) and other means. -- AND the user's request strongly matches one of the discoverable tools listed below. +- For connectors/apps that are not installed but needed for an installed plugin, suggest to install them if the task requirements match precisely. +- For plugins that are not installed but discoverable, only suggest discoverable and installable plugins when the user's intent very explicitly and unambiguously matches that plugin itself. Do not suggest a plugin just because one of its connectors or capabilities seems relevant. Tool suggestions should only use the discoverable tools listed here. DO NOT explore or recommend tools that are not on this list. @@ -14,7 +15,7 @@ Discoverable tools: Workflow: 1. Ensure all possible means have been exhausted to find an existing available tool but none of them matches the request intent. -2. Match the user's request against the discoverable tools list above. +2. Match the user's request against the discoverable tools list above. Apply the stricter explicit-and-unambiguous rule for *discoverable tools* like plugin install suggestions; *missing tools* like connector install suggestions continue to use the normal clear-fit standard. 3. If one tool clearly fits, call `tool_suggest` with: - `tool_type`: `connector` or `plugin` - `action_type`: `install` or `enable` From b6050b42ae7e13b1eb26e9732a1582bd3c055505 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 26 Mar 2026 12:13:51 -0700 Subject: [PATCH 2/6] fix: resolve bwrap from trusted PATH entry (#15791) ## Summary - resolve system bwrap from PATH instead of hardcoding /usr/bin/bwrap - skip PATH entries that resolve inside the current workspace before launching the sandbox helper - keep the vendored bubblewrap fallback when no trusted system bwrap is found ## Validation - cargo test -p codex-core bwrap --lib - cargo test -p codex-linux-sandbox - just fix -p codex-core - just fix -p codex-linux-sandbox - just fmt - just argument-comment-lint - cargo clean --- codex-rs/core/README.md | 14 ++--- codex-rs/core/src/config/config_tests.rs | 65 ++++++++++++++++++++++-- codex-rs/core/src/config/mod.rs | 43 ++++++++++++---- codex-rs/linux-sandbox/README.md | 30 ++++++----- codex-rs/linux-sandbox/src/launcher.rs | 7 +-- 5 files changed, 121 insertions(+), 38 deletions(-) diff --git a/codex-rs/core/README.md b/codex-rs/core/README.md index 4b8cb793b7..1605d28a07 100644 --- a/codex-rs/core/README.md +++ b/codex-rs/core/README.md @@ -59,13 +59,13 @@ only when the split filesystem policy round-trips through the legacy cases like `/repo = write`, `/repo/a = none`, `/repo/a/b = write`, where the more specific writable child must reopen under a denied parent. -The Linux sandbox helper prefers `/usr/bin/bwrap` whenever it is available. If -`/usr/bin/bwrap` is present but too old to support `--argv0`, the helper keeps -using system bubblewrap and switches to a no-`--argv0` compatibility path for -the inner re-exec. If `/usr/bin/bwrap` is missing, it falls back to the -vendored bubblewrap path compiled into the binary and Codex surfaces a startup -warning through its normal notification path instead of printing directly from -the sandbox helper. +The Linux sandbox helper prefers the first `bwrap` found on `PATH` outside the +current working directory whenever it is available. If `bwrap` is present but +too old to support `--argv0`, the helper keeps using system bubblewrap and +switches to a no-`--argv0` compatibility path for the inner re-exec. If +`bwrap` is missing, it falls back to the vendored bubblewrap path compiled into +the binary and Codex surfaces a startup warning through its normal notification +path instead of printing directly from the sandbox helper. ### Windows diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 949906ac41..421bfa6bd6 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -5695,8 +5695,8 @@ shell_tool = true #[cfg(target_os = "linux")] #[test] fn system_bwrap_warning_reports_missing_system_bwrap() { - let warning = system_bwrap_warning_for_path(Path::new("/definitely/not/a/bwrap")) - .expect("missing system bwrap should emit a warning"); + let warning = + system_bwrap_warning_for_lookup(None).expect("missing system bwrap should emit a warning"); assert!(warning.contains("could not find system bubblewrap")); } @@ -5716,12 +5716,48 @@ exit 1 let fake_bwrap_path: &Path = fake_bwrap.as_ref(); assert_eq!( - system_bwrap_warning_for_path(fake_bwrap_path), + system_bwrap_warning_for_lookup(Some(fake_bwrap_path.to_path_buf())), None, "Do not warn even if bwrap does not support `--argv0`", ); } +#[cfg(target_os = "linux")] +#[test] +fn finds_first_executable_bwrap_in_search_paths() { + let temp_dir = tempdir().expect("temp dir"); + let cwd = temp_dir.path().join("cwd"); + let first_dir = temp_dir.path().join("first"); + let second_dir = temp_dir.path().join("second"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + std::fs::create_dir_all(&first_dir).expect("create first dir"); + std::fs::create_dir_all(&second_dir).expect("create second dir"); + std::fs::write(first_dir.join("bwrap"), "not executable").expect("write non-executable bwrap"); + let expected_bwrap = write_named_fake_bwrap_in(&second_dir); + + assert_eq!( + find_system_bwrap_in_search_paths(vec![first_dir, second_dir], &cwd), + Some(expected_bwrap) + ); +} + +#[cfg(target_os = "linux")] +#[test] +fn skips_workspace_local_bwrap_in_search_paths() { + let temp_dir = tempdir().expect("temp dir"); + let cwd = temp_dir.path().join("cwd"); + let trusted_dir = temp_dir.path().join("trusted"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + std::fs::create_dir_all(&trusted_dir).expect("create trusted dir"); + let _workspace_bwrap = write_named_fake_bwrap_in(&cwd); + let expected_bwrap = write_named_fake_bwrap_in(&trusted_dir); + + assert_eq!( + find_system_bwrap_in_search_paths(vec![cwd.clone(), trusted_dir], &cwd), + Some(expected_bwrap) + ); +} + #[cfg(not(target_os = "linux"))] #[test] fn system_bwrap_warning_is_disabled_off_linux() { @@ -5730,6 +5766,14 @@ fn system_bwrap_warning_is_disabled_off_linux() { #[cfg(target_os = "linux")] fn write_fake_bwrap(contents: &str) -> tempfile::TempPath { + write_fake_bwrap_in( + &std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + contents, + ) +} + +#[cfg(target_os = "linux")] +fn write_fake_bwrap_in(dir: &Path, contents: &str) -> tempfile::TempPath { use std::fs; use std::os::unix::fs::PermissionsExt; use tempfile::NamedTempFile; @@ -5737,9 +5781,8 @@ fn write_fake_bwrap(contents: &str) -> tempfile::TempPath { // Bazel can mount the OS temp directory `noexec`, so prefer the current // working directory for fake executables and fall back to the default temp // dir outside that environment. - let temp_file = std::env::current_dir() + let temp_file = NamedTempFile::new_in(dir) .ok() - .and_then(|dir| NamedTempFile::new_in(dir).ok()) .unwrap_or_else(|| NamedTempFile::new().expect("temp file")); // Linux rejects exec-ing a file that is still open for writing. let path = temp_file.into_temp_path(); @@ -5749,6 +5792,18 @@ fn write_fake_bwrap(contents: &str) -> tempfile::TempPath { path } +#[cfg(target_os = "linux")] +fn write_named_fake_bwrap_in(dir: &Path) -> PathBuf { + use std::fs; + use std::os::unix::fs::PermissionsExt; + + let path = dir.join("bwrap"); + fs::write(&path, "#!/bin/sh\n").expect("write fake bwrap"); + let permissions = fs::Permissions::from_mode(0o755); + fs::set_permissions(&path, permissions).expect("chmod fake bwrap"); + path +} + #[tokio::test] async fn approvals_reviewer_defaults_to_manual_only_without_guardian_feature() -> std::io::Result<()> { diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index b2b55feeca..6db6f0e613 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -145,7 +145,7 @@ pub(crate) const DEFAULT_AGENT_JOB_MAX_RUNTIME_SECONDS: Option = None; pub const CONFIG_TOML_FILE: &str = "config.toml"; const OPENAI_BASE_URL_ENV_VAR: &str = "OPENAI_BASE_URL"; #[cfg(target_os = "linux")] -const SYSTEM_BWRAP_PATH: &str = "/usr/bin/bwrap"; +const SYSTEM_BWRAP_PROGRAM: &str = "bwrap"; const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [ OPENAI_PROVIDER_ID, OLLAMA_OSS_PROVIDER_ID, @@ -154,7 +154,7 @@ const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [ #[cfg(target_os = "linux")] pub fn system_bwrap_warning() -> Option { - system_bwrap_warning_for_path(Path::new(SYSTEM_BWRAP_PATH)) + system_bwrap_warning_for_lookup(find_system_bwrap_in_path()) } #[cfg(not(target_os = "linux"))] @@ -163,15 +163,40 @@ pub fn system_bwrap_warning() -> Option { } #[cfg(target_os = "linux")] -fn system_bwrap_warning_for_path(system_bwrap_path: &Path) -> Option { - if !system_bwrap_path.is_file() { - return Some(format!( - "Codex could not find system bubblewrap at {}. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime.", - system_bwrap_path.display() - )); +fn system_bwrap_warning_for_lookup(system_bwrap_path: Option) -> Option { + match system_bwrap_path { + Some(_) => None, + None => Some( + "Codex could not find system bubblewrap on PATH. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime." + .to_string(), + ), } +} - None +#[cfg(target_os = "linux")] +pub fn find_system_bwrap_in_path() -> Option { + let search_path = std::env::var_os("PATH")?; + let cwd = std::env::current_dir().ok()?; + find_system_bwrap_in_search_paths(std::iter::once(PathBuf::from(search_path)), &cwd) +} + +#[cfg(target_os = "linux")] +fn find_system_bwrap_in_search_paths( + search_paths: impl IntoIterator, + cwd: &Path, +) -> Option { + let search_path = std::env::join_paths(search_paths).ok()?; + let cwd = std::fs::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf()); + which::which_in_all(SYSTEM_BWRAP_PROGRAM, Some(search_path), &cwd) + .ok()? + .find_map(|path| { + let path = std::fs::canonicalize(path).ok()?; + if path.starts_with(&cwd) { + None + } else { + Some(path) + } + }) } fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option { diff --git a/codex-rs/linux-sandbox/README.md b/codex-rs/linux-sandbox/README.md index 5898a9c688..d8af1959a4 100644 --- a/codex-rs/linux-sandbox/README.md +++ b/codex-rs/linux-sandbox/README.md @@ -7,25 +7,27 @@ This crate is responsible for producing: - the `codex-exec` CLI can check if its arg0 is `codex-linux-sandbox` and, if so, execute as if it were `codex-linux-sandbox` - this should also be true of the `codex` multitool CLI -On Linux, the bubblewrap pipeline prefers the system `/usr/bin/bwrap` whenever -it is available. If `/usr/bin/bwrap` is present but too old to support +On Linux, the bubblewrap pipeline prefers the first `bwrap` found on `PATH` +outside the current working directory whenever it is available. If `bwrap` is +present but too old to support `--argv0`, the helper keeps using system bubblewrap and switches to a -no-`--argv0` compatibility path for the inner re-exec. If `/usr/bin/bwrap` is -missing, the helper falls back to the vendored bubblewrap path compiled into -this binary. -Codex also surfaces a startup warning when `/usr/bin/bwrap` is missing so users -know it is falling back to the vendored helper. +no-`--argv0` compatibility path for the inner re-exec. If `bwrap` is missing, +the helper falls back to the vendored bubblewrap path compiled into this +binary. +Codex also surfaces a startup warning when `bwrap` is missing so users know it +is falling back to the vendored helper. **Current Behavior** - Legacy `SandboxPolicy` / `sandbox_mode` configs remain supported. - Bubblewrap is the default filesystem sandbox pipeline. -- If `/usr/bin/bwrap` is present, the helper uses it. -- If `/usr/bin/bwrap` is present but too old to support `--argv0`, the helper - uses a no-`--argv0` compatibility path for the inner re-exec. -- If `/usr/bin/bwrap` is missing, the helper falls back to the vendored - bubblewrap path. -- If `/usr/bin/bwrap` is missing, Codex also surfaces a startup warning instead - of printing directly from the sandbox helper. +- If `bwrap` is present on `PATH` outside the current working directory, the + helper uses it. +- If `bwrap` is present but too old to support `--argv0`, the helper uses a + no-`--argv0` compatibility path for the inner re-exec. +- If `bwrap` is missing, the helper falls back to the vendored bubblewrap + path. +- If `bwrap` is missing, Codex also surfaces a startup warning instead of + printing directly from the sandbox helper. - Legacy Landlock + mount protections remain available as an explicit legacy fallback path. - Set `features.use_legacy_landlock = true` (or CLI `-c use_legacy_landlock=true`) diff --git a/codex-rs/linux-sandbox/src/launcher.rs b/codex-rs/linux-sandbox/src/launcher.rs index e68eea2725..c36f1c5ff1 100644 --- a/codex-rs/linux-sandbox/src/launcher.rs +++ b/codex-rs/linux-sandbox/src/launcher.rs @@ -10,8 +10,6 @@ use std::sync::OnceLock; use crate::vendored_bwrap::exec_vendored_bwrap; use codex_utils_absolute_path::AbsolutePathBuf; -const SYSTEM_BWRAP_PATH: &str = "/usr/bin/bwrap"; - #[derive(Debug, Clone, PartialEq, Eq)] enum BubblewrapLauncher { System(SystemBwrapLauncher), @@ -36,7 +34,10 @@ pub(crate) fn exec_bwrap(argv: Vec, preserved_files: Vec) -> ! { fn preferred_bwrap_launcher() -> BubblewrapLauncher { static LAUNCHER: OnceLock = OnceLock::new(); LAUNCHER - .get_or_init(|| preferred_bwrap_launcher_for_path(Path::new(SYSTEM_BWRAP_PATH))) + .get_or_init(|| match codex_core::config::find_system_bwrap_in_path() { + Some(path) => preferred_bwrap_launcher_for_path(&path), + None => BubblewrapLauncher::Vendored, + }) .clone() } From b3e069e8cbed9ef86a47335041632ddfb458b79f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 26 Mar 2026 12:33:23 -0700 Subject: [PATCH 3/6] skills: remove unused skill permission metadata (#15900) ## Why Skill metadata accepted a `permissions` block and stored the result on `SkillMetadata`, but that data was never consumed by runtime behavior. Leaving the dead parsing path in place makes it look like skills can widen or otherwise influence execution permissions when, in practice, declared skill permissions are ignored. This change removes that misleading surface area so the skill metadata model matches what the system actually uses. ## What changed - removed `permission_profile` and `managed_network_override` from `core-skills::SkillMetadata` - stopped parsing `permissions` from skill metadata in `core-skills/src/loader.rs` - deleted the loader tests that only exercised the removed permissions parsing path - cleaned up dependent `SkillMetadata` constructors in tests and TUI code that were only carrying `None` for those fields ## Testing - `cargo test -p codex-core-skills` - `cargo test -p codex-tui submission_prefers_selected_duplicate_skill_path` - `just argument-comment-lint` --- codex-rs/core-skills/src/injection_tests.rs | 2 - .../core-skills/src/invocation_utils_tests.rs | 2 - codex-rs/core-skills/src/loader.rs | 73 ---- codex-rs/core-skills/src/loader_tests.rs | 396 ------------------ codex-rs/core-skills/src/manager_tests.rs | 2 - codex-rs/core-skills/src/model.rs | 16 - .../core/src/mcp/skill_dependencies_tests.rs | 2 - codex-rs/tui/src/bottom_pane/chat_composer.rs | 4 - codex-rs/tui/src/bottom_pane/mod.rs | 2 - codex-rs/tui/src/chatwidget/skills.rs | 2 - codex-rs/tui/src/chatwidget/tests.rs | 4 - .../src/bottom_pane/chat_composer.rs | 4 - .../tui_app_server/src/bottom_pane/mod.rs | 2 - .../tui_app_server/src/chatwidget/skills.rs | 2 - .../tui_app_server/src/chatwidget/tests.rs | 4 - 15 files changed, 517 deletions(-) diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/core-skills/src/injection_tests.rs index 8d66a0af57..b8611de4ef 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -11,8 +11,6 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from(path), scope: codex_protocol::protocol::SkillScope::User, } diff --git a/codex-rs/core-skills/src/invocation_utils_tests.rs b/codex-rs/core-skills/src/invocation_utils_tests.rs index 657582b742..6d74dbe9a7 100644 --- a/codex-rs/core-skills/src/invocation_utils_tests.rs +++ b/codex-rs/core-skills/src/invocation_utils_tests.rs @@ -18,8 +18,6 @@ fn test_skill_metadata(skill_doc_path: PathBuf) -> SkillMetadata { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: skill_doc_path, scope: codex_protocol::protocol::SkillScope::User, } diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index b6ef000845..42de9fb288 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -2,7 +2,6 @@ use crate::model::SkillDependencies; use crate::model::SkillError; use crate::model::SkillInterface; use crate::model::SkillLoadOutcome; -use crate::model::SkillManagedNetworkOverride; use crate::model::SkillMetadata; use crate::model::SkillPolicy; use crate::model::SkillToolDependency; @@ -13,10 +12,6 @@ use codex_config::ConfigLayerStackOrdering; use codex_config::default_project_root_markers; use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; -use codex_protocol::models::FileSystemPermissions; -use codex_protocol::models::MacOsSeatbeltProfileExtensions; -use codex_protocol::models::NetworkPermissions; -use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBufGuard; @@ -59,8 +54,6 @@ struct SkillMetadataFile { dependencies: Option, #[serde(default)] policy: Option, - #[serde(default)] - permissions: Option, } #[derive(Default)] @@ -68,28 +61,6 @@ struct LoadedSkillMetadata { interface: Option, dependencies: Option, policy: Option, - permission_profile: Option, - managed_network_override: Option, -} - -#[derive(Debug, Default, Deserialize, PartialEq, Eq)] -struct SkillPermissionProfile { - #[serde(default)] - network: Option, - #[serde(default)] - file_system: Option, - #[serde(default)] - macos: Option, -} - -#[derive(Debug, Default, Deserialize, PartialEq, Eq)] -struct SkillNetworkPermissions { - #[serde(default)] - enabled: Option, - #[serde(default)] - allowed_domains: Option>, - #[serde(default)] - denied_domains: Option>, } #[derive(Debug, Default, Deserialize)] @@ -551,8 +522,6 @@ fn parse_skill_file(path: &Path, scope: SkillScope) -> Result Result LoadedSkillMetadata { interface, dependencies, policy, - permissions, } = parsed; - let (permission_profile, managed_network_override) = normalize_permissions(permissions); LoadedSkillMetadata { interface: resolve_interface(interface, skill_dir), dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), - permission_profile, - managed_network_override, } } -fn normalize_permissions( - permissions: Option, -) -> ( - Option, - Option, -) { - let Some(permissions) = permissions else { - return (None, None); - }; - let managed_network_override = permissions - .network - .as_ref() - .map(|network| SkillManagedNetworkOverride { - allowed_domains: network.allowed_domains.clone(), - denied_domains: network.denied_domains.clone(), - }) - .filter(SkillManagedNetworkOverride::has_domain_overrides); - let permission_profile = PermissionProfile { - network: permissions.network.and_then(|network| { - let network = NetworkPermissions { - enabled: network.enabled, - }; - (!network.is_empty()).then_some(network) - }), - file_system: permissions - .file_system - .filter(|file_system| !file_system.is_empty()), - macos: permissions.macos, - }; - - ( - (!permission_profile.is_empty()).then_some(permission_profile), - managed_network_override, - ) -} - fn resolve_interface(interface: Option, skill_dir: &Path) -> Option { let interface = interface?; let interface = SkillInterface { diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 5875b1a043..ad196df670 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,12 +4,6 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; -use codex_protocol::models::FileSystemPermissions; -use codex_protocol::models::MacOsAutomationPermission; -use codex_protocol::models::MacOsContactsPermission; -use codex_protocol::models::MacOsPreferencesPermission; -use codex_protocol::models::MacOsSeatbeltProfileExtensions; -use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -290,8 +284,6 @@ fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -442,8 +434,6 @@ async fn loads_skill_dependencies_metadata_from_yaml() { ], }), policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -499,8 +489,6 @@ interface: }), dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(skill_path.as_path()), scope: SkillScope::User, }] @@ -610,340 +598,6 @@ policy: ); } -#[tokio::test] -async fn loads_skill_permissions_from_yaml() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "permissions-skill", "from yaml"); - let skill_dir = skill_path.parent().expect("skill dir"); - fs::create_dir_all(skill_dir.join("data")).expect("create read path"); - fs::create_dir_all(skill_dir.join("output")).expect("create write path"); - - write_skill_metadata_at( - skill_dir, - r#" -permissions: - network: - enabled: true - file_system: - read: - - "./data" - write: - - "./output" -"#, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!( - outcome.skills[0].permission_profile, - Some(PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions { - read: Some(vec![ - AbsolutePathBuf::try_from(normalized(skill_dir.join("data").as_path())) - .expect("absolute data path"), - ]), - write: Some(vec![ - AbsolutePathBuf::try_from(normalized(skill_dir.join("output").as_path())) - .expect("absolute output path"), - ]), - }), - macos: None, - }) - ); - assert_eq!(outcome.skills[0].managed_network_override, None); -} - -#[tokio::test] -async fn empty_skill_permissions_do_not_create_profile() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "permissions-empty", "from yaml"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_metadata_at( - skill_dir, - r#" -permissions: {} -"#, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].permission_profile, None); -} - -#[test] -fn normalize_permissions_splits_managed_network_overrides() { - let (permission_profile, managed_network_override) = - normalize_permissions(Some(SkillPermissionProfile { - network: Some(SkillNetworkPermissions { - enabled: Some(true), - allowed_domains: Some(vec!["skill.example.com".to_string()]), - denied_domains: Some(vec!["blocked.skill.example.com".to_string()]), - }), - file_system: None, - macos: None, - })); - - assert_eq!( - permission_profile, - Some(PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: None, - macos: None, - }) - ); - assert_eq!( - managed_network_override, - Some(SkillManagedNetworkOverride { - allowed_domains: Some(vec!["skill.example.com".to_string()]), - denied_domains: Some(vec!["blocked.skill.example.com".to_string()]), - }) - ); -} - -#[test] -fn normalize_permissions_preserves_network_gate_separately_from_overrides() { - let (permission_profile, managed_network_override) = - normalize_permissions(Some(SkillPermissionProfile { - network: Some(SkillNetworkPermissions { - enabled: Some(false), - allowed_domains: Some(vec!["skill.example.com".to_string()]), - denied_domains: None, - }), - file_system: None, - macos: None, - })); - - assert_eq!( - permission_profile, - Some(PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(false), - }), - file_system: None, - macos: None, - }) - ); - assert_eq!( - managed_network_override, - Some(SkillManagedNetworkOverride { - allowed_domains: Some(vec!["skill.example.com".to_string()]), - denied_domains: None, - }) - ); -} - -#[test] -fn skill_metadata_parses_macos_permissions_yaml() { - let parsed = serde_yaml::from_str::( - r#" -permissions: - macos: - macos_preferences: "read_write" - macos_automation: - - "com.apple.Notes" - macos_launch_services: true - macos_accessibility: true - macos_calendar: true -"#, - ) - .expect("parse skill metadata"); - - assert_eq!( - parsed.permissions, - Some(SkillPermissionProfile { - network: None, - file_system: None, - macos: Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string(), - ]), - macos_launch_services: true, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }), - }) - ); -} - -#[test] -fn skill_metadata_parses_macos_reminders_permission_yaml() { - let parsed = serde_yaml::from_str::( - r#" -permissions: - macos: - macos_reminders: true -"#, - ) - .expect("parse reminders skill metadata"); - - assert_eq!( - parsed.permissions, - Some(SkillPermissionProfile { - network: None, - file_system: None, - macos: Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadOnly, - macos_automation: MacOsAutomationPermission::None, - macos_launch_services: false, - macos_accessibility: false, - macos_calendar: false, - macos_reminders: true, - macos_contacts: MacOsContactsPermission::None, - }), - }) - ); -} - -#[test] -fn skill_metadata_parses_network_domain_overrides_under_permissions() { - let parsed = serde_yaml::from_str::( - r#" -permissions: - network: - enabled: true - allowed_domains: - - "skill.example.com" - denied_domains: - - "blocked.skill.example.com" -"#, - ) - .expect("parse network skill metadata"); - - assert_eq!( - parsed.permissions, - Some(SkillPermissionProfile { - network: Some(SkillNetworkPermissions { - enabled: Some(true), - allowed_domains: Some(vec!["skill.example.com".to_string()]), - denied_domains: Some(vec!["blocked.skill.example.com".to_string()]), - }), - file_system: None, - macos: None, - }) - ); -} - -#[cfg(target_os = "macos")] -#[tokio::test] -async fn loads_skill_macos_permissions_from_yaml() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "permissions-macos", "from yaml"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_metadata_at( - skill_dir, - r#" -permissions: - macos: - macos_preferences: "read_write" - macos_automation: - - "com.apple.Notes" - macos_launch_services: true - macos_accessibility: true - macos_calendar: true -"#, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!( - outcome.skills[0].permission_profile, - Some(PermissionProfile { - macos: Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string() - ],), - macos_launch_services: true, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }), - ..Default::default() - }) - ); -} - -#[cfg(not(target_os = "macos"))] -#[tokio::test] -async fn loads_skill_macos_permissions_from_yaml_non_macos_does_not_create_profile() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "permissions-macos", "from yaml"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_metadata_at( - skill_dir, - r#" -permissions: - macos: - macos_preferences: "read_write" - macos_automation: - - "com.apple.Notes" - macos_launch_services: true - macos_accessibility: true - macos_calendar: true -"#, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!( - outcome.skills[0].permission_profile, - Some(PermissionProfile { - macos: Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string() - ],), - macos_launch_services: true, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }), - ..Default::default() - }) - ); -} - #[tokio::test] async fn accepts_icon_paths_under_assets_dir() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -988,8 +642,6 @@ async fn accepts_icon_paths_under_assets_dir() { }), dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1030,8 +682,6 @@ async fn ignores_invalid_brand_color() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1085,8 +735,6 @@ async fn ignores_default_prompt_over_max_length() { }), dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1128,8 +776,6 @@ async fn drops_interface_when_icons_are_invalid() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1174,8 +820,6 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::User, }] @@ -1235,8 +879,6 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1273,8 +915,6 @@ fn loads_skills_via_symlinked_subdir_for_admin_scope() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::Admin, }] @@ -1314,8 +954,6 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&linked_skill_path), scope: SkillScope::Repo, }] @@ -1383,8 +1021,6 @@ async fn respects_max_scan_depth_for_user_scope() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&within_depth_path), scope: SkillScope::User, }] @@ -1412,8 +1048,6 @@ async fn loads_valid_skill() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1446,8 +1080,6 @@ async fn falls_back_to_directory_name_when_skill_name_is_missing() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1489,8 +1121,6 @@ async fn namespaces_plugin_skills_using_plugin_name() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1522,8 +1152,6 @@ async fn loads_short_description_from_metadata() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1636,8 +1264,6 @@ async fn loads_skills_from_repo_root() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -1673,8 +1299,6 @@ async fn loads_skills_from_agents_dir_without_codex_dir() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -1728,8 +1352,6 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&nested_skill_path), scope: SkillScope::Repo, }, @@ -1740,8 +1362,6 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&root_skill_path), scope: SkillScope::Repo, }, @@ -1781,8 +1401,6 @@ async fn loads_skills_from_codex_dir_when_not_git_repo() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -1820,8 +1438,6 @@ async fn deduplicates_by_path_preferring_first_root() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -1863,8 +1479,6 @@ async fn keeps_duplicate_names_from_repo_and_user() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&repo_skill_path), scope: SkillScope::Repo, }, @@ -1875,8 +1489,6 @@ async fn keeps_duplicate_names_from_repo_and_user() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&user_skill_path), scope: SkillScope::User, }, @@ -1940,8 +1552,6 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: first_path, scope: SkillScope::Repo, }, @@ -1952,8 +1562,6 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: second_path, scope: SkillScope::Repo, }, @@ -2025,8 +1633,6 @@ async fn loads_skills_when_cwd_is_file_in_repo() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -2085,8 +1691,6 @@ async fn loads_skills_from_system_cache_when_present() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::System, }] diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 2507fc9823..9e6c60c0e6 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -57,8 +57,6 @@ fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: path, scope: SkillScope::User, } diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index d47904b9c7..319ca4e64e 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -3,22 +3,8 @@ use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; -use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; -use serde::Deserialize; - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Eq)] -pub struct SkillManagedNetworkOverride { - pub allowed_domains: Option>, - pub denied_domains: Option>, -} - -impl SkillManagedNetworkOverride { - pub fn has_domain_overrides(&self) -> bool { - self.allowed_domains.is_some() || self.denied_domains.is_some() - } -} #[derive(Debug, Clone, PartialEq)] pub struct SkillMetadata { @@ -28,8 +14,6 @@ pub struct SkillMetadata { pub interface: Option, pub dependencies: Option, pub policy: Option, - pub permission_profile: Option, - pub managed_network_override: Option, /// Path to the SKILLS.md file that declares this skill. pub path_to_skills_md: PathBuf, pub scope: SkillScope, diff --git a/codex-rs/core/src/mcp/skill_dependencies_tests.rs b/codex-rs/core/src/mcp/skill_dependencies_tests.rs index 34c6a767f5..7387ef4b31 100644 --- a/codex-rs/core/src/mcp/skill_dependencies_tests.rs +++ b/codex-rs/core/src/mcp/skill_dependencies_tests.rs @@ -12,8 +12,6 @@ fn skill_with_tools(tools: Vec) -> SkillMetadata { interface: None, dependencies: Some(SkillDependencies { tools }), policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from("skill"), scope: SkillScope::User, } diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 6d5c92d490..3e85d03f53 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -5409,8 +5409,6 @@ mod tests { }), dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from("/tmp/repo/google-calendar/SKILL.md"), scope: codex_protocol::protocol::SkillScope::Repo, }])); @@ -5501,8 +5499,6 @@ mod tests { }), dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from("/tmp/repo/google-calendar/SKILL.md"), scope: codex_protocol::protocol::SkillScope::Repo, }])); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index eda37fe9c4..a405a5837a 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -1708,8 +1708,6 @@ mod tests { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from("test-skill"), scope: SkillScope::User, }]), diff --git a/codex-rs/tui/src/chatwidget/skills.rs b/codex-rs/tui/src/chatwidget/skills.rs index 24273b6976..6e161087c3 100644 --- a/codex-rs/tui/src/chatwidget/skills.rs +++ b/codex-rs/tui/src/chatwidget/skills.rs @@ -191,8 +191,6 @@ fn protocol_skill_to_core(skill: &ProtocolSkillMetadata) -> SkillMetadata { .collect(), }), policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: skill.path.clone(), scope: skill.scope, } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index cba1b285e0..90fea87e7f 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1049,8 +1049,6 @@ async fn submission_prefers_selected_duplicate_skill_path() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: repo_skill_path, scope: SkillScope::Repo, }, @@ -1061,8 +1059,6 @@ async fn submission_prefers_selected_duplicate_skill_path() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: user_skill_path.clone(), scope: SkillScope::User, }, diff --git a/codex-rs/tui_app_server/src/bottom_pane/chat_composer.rs b/codex-rs/tui_app_server/src/bottom_pane/chat_composer.rs index fea3b8c2ad..e972750f53 100644 --- a/codex-rs/tui_app_server/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui_app_server/src/bottom_pane/chat_composer.rs @@ -5424,8 +5424,6 @@ mod tests { }), dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from("/tmp/repo/google-calendar/SKILL.md"), scope: codex_protocol::protocol::SkillScope::Repo, }])); @@ -5516,8 +5514,6 @@ mod tests { }), dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from("/tmp/repo/google-calendar/SKILL.md"), scope: codex_protocol::protocol::SkillScope::Repo, }])); diff --git a/codex-rs/tui_app_server/src/bottom_pane/mod.rs b/codex-rs/tui_app_server/src/bottom_pane/mod.rs index 78cf6a2359..d74f05b5d4 100644 --- a/codex-rs/tui_app_server/src/bottom_pane/mod.rs +++ b/codex-rs/tui_app_server/src/bottom_pane/mod.rs @@ -1701,8 +1701,6 @@ mod tests { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: PathBuf::from("test-skill"), scope: SkillScope::User, }]), diff --git a/codex-rs/tui_app_server/src/chatwidget/skills.rs b/codex-rs/tui_app_server/src/chatwidget/skills.rs index 24273b6976..6e161087c3 100644 --- a/codex-rs/tui_app_server/src/chatwidget/skills.rs +++ b/codex-rs/tui_app_server/src/chatwidget/skills.rs @@ -191,8 +191,6 @@ fn protocol_skill_to_core(skill: &ProtocolSkillMetadata) -> SkillMetadata { .collect(), }), policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: skill.path.clone(), scope: skill.scope, } diff --git a/codex-rs/tui_app_server/src/chatwidget/tests.rs b/codex-rs/tui_app_server/src/chatwidget/tests.rs index 8191acc3db..7dadb963d8 100644 --- a/codex-rs/tui_app_server/src/chatwidget/tests.rs +++ b/codex-rs/tui_app_server/src/chatwidget/tests.rs @@ -1078,8 +1078,6 @@ async fn submission_prefers_selected_duplicate_skill_path() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: repo_skill_path, scope: SkillScope::Repo, }, @@ -1090,8 +1088,6 @@ async fn submission_prefers_selected_duplicate_skill_path() { interface: None, dependencies: None, policy: None, - permission_profile: None, - managed_network_override: None, path_to_skills_md: user_skill_path.clone(), scope: SkillScope::User, }, From 9736fa5e3d5da08e20c533f07c30c2960475e19b Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Thu, 26 Mar 2026 13:01:35 -0700 Subject: [PATCH 4/6] app-server: Split transport module (#15811) `transport.rs` is getting pretty big, split individual transport implementations into separate files. --- .../src/{transport.rs => transport/mod.rs} | 374 +----------------- codex-rs/app-server/src/transport/stdio.rs | 88 +++++ .../app-server/src/transport/websocket.rs | 308 +++++++++++++++ 3 files changed, 400 insertions(+), 370 deletions(-) rename codex-rs/app-server/src/{transport.rs => transport/mod.rs} (74%) create mode 100644 codex-rs/app-server/src/transport/stdio.rs create mode 100644 codex-rs/app-server/src/transport/websocket.rs diff --git a/codex-rs/app-server/src/transport.rs b/codex-rs/app-server/src/transport/mod.rs similarity index 74% rename from codex-rs/app-server/src/transport.rs rename to codex-rs/app-server/src/transport/mod.rs index 020e441da5..2f80a009d3 100644 --- a/codex-rs/app-server/src/transport.rs +++ b/codex-rs/app-server/src/transport/mod.rs @@ -1,8 +1,5 @@ pub(crate) mod auth; -use self::auth::WebsocketAuthPolicy; -use self::auth::authorize_upgrade; -use self::auth::should_warn_about_unauthenticated_non_loopback_listener; use crate::error_code::OVERLOADED_ERROR_CODE; use crate::message_processor::ConnectionSessionState; use crate::outgoing_message::ConnectionId; @@ -10,53 +7,20 @@ use crate::outgoing_message::OutgoingEnvelope; use crate::outgoing_message::OutgoingError; use crate::outgoing_message::OutgoingMessage; use crate::outgoing_message::QueuedOutgoingMessage; -use axum::Router; -use axum::body::Body; -use axum::extract::ConnectInfo; -use axum::extract::State; -use axum::extract::ws::Message as WebSocketMessage; -use axum::extract::ws::WebSocket; -use axum::extract::ws::WebSocketUpgrade; -use axum::http::HeaderMap; -use axum::http::Request; -use axum::http::StatusCode; -use axum::http::header::ORIGIN; -use axum::middleware; -use axum::middleware::Next; -use axum::response::IntoResponse; -use axum::response::Response; -use axum::routing::any; -use axum::routing::get; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::ServerRequest; -use futures::SinkExt; -use futures::StreamExt; -use owo_colors::OwoColorize; -use owo_colors::Stream; -use owo_colors::Style; use std::collections::HashMap; use std::collections::HashSet; -use std::io::ErrorKind; -use std::io::Result as IoResult; use std::net::SocketAddr; use std::str::FromStr; use std::sync::Arc; use std::sync::RwLock; use std::sync::atomic::AtomicBool; -use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; -use tokio::io::AsyncBufReadExt; -use tokio::io::AsyncWriteExt; -use tokio::io::BufReader; -use tokio::io::{self}; -use tokio::net::TcpListener; use tokio::sync::mpsc; -use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; -use tracing::debug; use tracing::error; -use tracing::info; use tracing::warn; /// Size of the bounded channels used to communicate between tasks. The value @@ -64,85 +28,11 @@ use tracing::warn; /// plenty for an interactive CLI. pub(crate) const CHANNEL_CAPACITY: usize = 128; -fn colorize(text: &str, style: Style) -> String { - text.if_supports_color(Stream::Stderr, |value| value.style(style)) - .to_string() -} +mod stdio; +mod websocket; -#[allow(clippy::print_stderr)] -fn print_websocket_startup_banner(addr: SocketAddr) { - let title = colorize("codex app-server (WebSockets)", Style::new().bold().cyan()); - let listening_label = colorize("listening on:", Style::new().dimmed()); - let listen_url = colorize(&format!("ws://{addr}"), Style::new().green()); - let ready_label = colorize("readyz:", Style::new().dimmed()); - let ready_url = colorize(&format!("http://{addr}/readyz"), Style::new().green()); - let health_label = colorize("healthz:", Style::new().dimmed()); - let health_url = colorize(&format!("http://{addr}/healthz"), Style::new().green()); - let note_label = colorize("note:", Style::new().dimmed()); - eprintln!("{title}"); - eprintln!(" {listening_label} {listen_url}"); - eprintln!(" {ready_label} {ready_url}"); - eprintln!(" {health_label} {health_url}"); - if addr.ip().is_loopback() { - eprintln!( - " {note_label} binds localhost only (use SSH port-forwarding for remote access)" - ); - } else { - eprintln!( - " {note_label} websocket auth is opt-in in this build; configure `--ws-auth ...` before real remote use" - ); - } -} - -#[derive(Clone)] -struct WebSocketListenerState { - transport_event_tx: mpsc::Sender, - connection_counter: Arc, - auth_policy: Arc, -} - -async fn health_check_handler() -> StatusCode { - StatusCode::OK -} - -async fn reject_requests_with_origin_header( - request: Request, - next: Next, -) -> Result { - if request.headers().contains_key(ORIGIN) { - warn!( - method = %request.method(), - uri = %request.uri(), - "rejecting websocket listener request with Origin header" - ); - Err(StatusCode::FORBIDDEN) - } else { - Ok(next.run(request).await) - } -} - -async fn websocket_upgrade_handler( - websocket: WebSocketUpgrade, - ConnectInfo(peer_addr): ConnectInfo, - State(state): State, - headers: HeaderMap, -) -> impl IntoResponse { - if let Err(err) = authorize_upgrade(&headers, state.auth_policy.as_ref()) { - warn!( - %peer_addr, - message = err.message(), - "rejecting websocket client during upgrade" - ); - return (err.status_code(), err.message()).into_response(); - } - let connection_id = ConnectionId(state.connection_counter.fetch_add(1, Ordering::Relaxed)); - info!(%peer_addr, "websocket client connected"); - websocket - .on_upgrade(move |stream| async move { - run_websocket_connection(connection_id, stream, state.transport_event_tx).await; - }) - .into_response() -} +pub(crate) use stdio::start_stdio_connection; +pub(crate) use websocket::start_websocket_acceptor; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum AppServerTransport { @@ -276,262 +166,6 @@ impl OutboundConnectionState { } } -pub(crate) async fn start_stdio_connection( - transport_event_tx: mpsc::Sender, - stdio_handles: &mut Vec>, -) -> IoResult<()> { - let connection_id = ConnectionId(0); - let (writer_tx, mut writer_rx) = mpsc::channel::(CHANNEL_CAPACITY); - let writer_tx_for_reader = writer_tx.clone(); - transport_event_tx - .send(TransportEvent::ConnectionOpened { - connection_id, - writer: writer_tx, - disconnect_sender: None, - }) - .await - .map_err(|_| std::io::Error::new(ErrorKind::BrokenPipe, "processor unavailable"))?; - - let transport_event_tx_for_reader = transport_event_tx.clone(); - stdio_handles.push(tokio::spawn(async move { - let stdin = io::stdin(); - let reader = BufReader::new(stdin); - let mut lines = reader.lines(); - - loop { - match lines.next_line().await { - Ok(Some(line)) => { - if !forward_incoming_message( - &transport_event_tx_for_reader, - &writer_tx_for_reader, - connection_id, - &line, - ) - .await - { - break; - } - } - Ok(None) => break, - Err(err) => { - error!("Failed reading stdin: {err}"); - break; - } - } - } - - let _ = transport_event_tx_for_reader - .send(TransportEvent::ConnectionClosed { connection_id }) - .await; - debug!("stdin reader finished (EOF)"); - })); - - stdio_handles.push(tokio::spawn(async move { - let mut stdout = io::stdout(); - while let Some(queued_message) = writer_rx.recv().await { - let Some(mut json) = serialize_outgoing_message(queued_message.message) else { - continue; - }; - json.push('\n'); - if let Err(err) = stdout.write_all(json.as_bytes()).await { - error!("Failed to write to stdout: {err}"); - break; - } - if let Some(write_complete_tx) = queued_message.write_complete_tx { - let _ = write_complete_tx.send(()); - } - } - info!("stdout writer exited (channel closed)"); - })); - - Ok(()) -} - -pub(crate) async fn start_websocket_acceptor( - bind_address: SocketAddr, - transport_event_tx: mpsc::Sender, - shutdown_token: CancellationToken, - auth_policy: WebsocketAuthPolicy, -) -> IoResult> { - if should_warn_about_unauthenticated_non_loopback_listener(bind_address, &auth_policy) { - warn!( - %bind_address, - "starting non-loopback websocket listener without auth; websocket auth is opt-in for now and will become the default in a future release" - ); - } - let listener = TcpListener::bind(bind_address).await?; - let local_addr = listener.local_addr()?; - print_websocket_startup_banner(local_addr); - info!("app-server websocket listening on ws://{local_addr}"); - - let router = Router::new() - .route("/readyz", get(health_check_handler)) - .route("/healthz", get(health_check_handler)) - .fallback(any(websocket_upgrade_handler)) - .layer(middleware::from_fn(reject_requests_with_origin_header)) - .with_state(WebSocketListenerState { - transport_event_tx, - connection_counter: Arc::new(AtomicU64::new(1)), - auth_policy: Arc::new(auth_policy), - }); - let server = axum::serve( - listener, - router.into_make_service_with_connect_info::(), - ) - .with_graceful_shutdown(async move { - shutdown_token.cancelled().await; - }); - Ok(tokio::spawn(async move { - if let Err(err) = server.await { - error!("websocket acceptor failed: {err}"); - } - info!("websocket acceptor shutting down"); - })) -} - -async fn run_websocket_connection( - connection_id: ConnectionId, - websocket_stream: WebSocket, - transport_event_tx: mpsc::Sender, -) { - let (writer_tx, writer_rx) = mpsc::channel::(CHANNEL_CAPACITY); - let writer_tx_for_reader = writer_tx.clone(); - let disconnect_token = CancellationToken::new(); - if transport_event_tx - .send(TransportEvent::ConnectionOpened { - connection_id, - writer: writer_tx, - disconnect_sender: Some(disconnect_token.clone()), - }) - .await - .is_err() - { - return; - } - - let (websocket_writer, websocket_reader) = websocket_stream.split(); - let (writer_control_tx, writer_control_rx) = - mpsc::channel::(CHANNEL_CAPACITY); - let mut outbound_task = tokio::spawn(run_websocket_outbound_loop( - websocket_writer, - writer_rx, - writer_control_rx, - disconnect_token.clone(), - )); - let mut inbound_task = tokio::spawn(run_websocket_inbound_loop( - websocket_reader, - transport_event_tx.clone(), - writer_tx_for_reader, - writer_control_tx, - connection_id, - disconnect_token.clone(), - )); - - tokio::select! { - _ = &mut outbound_task => { - disconnect_token.cancel(); - inbound_task.abort(); - } - _ = &mut inbound_task => { - disconnect_token.cancel(); - outbound_task.abort(); - } - } - - let _ = transport_event_tx - .send(TransportEvent::ConnectionClosed { connection_id }) - .await; -} - -async fn run_websocket_outbound_loop( - mut websocket_writer: futures::stream::SplitSink, - mut writer_rx: mpsc::Receiver, - mut writer_control_rx: mpsc::Receiver, - disconnect_token: CancellationToken, -) { - loop { - tokio::select! { - _ = disconnect_token.cancelled() => { - break; - } - message = writer_control_rx.recv() => { - let Some(message) = message else { - break; - }; - if websocket_writer.send(message).await.is_err() { - break; - } - } - queued_message = writer_rx.recv() => { - let Some(queued_message) = queued_message else { - break; - }; - let Some(json) = serialize_outgoing_message(queued_message.message) else { - continue; - }; - if websocket_writer.send(WebSocketMessage::Text(json.into())).await.is_err() { - break; - } - if let Some(write_complete_tx) = queued_message.write_complete_tx { - let _ = write_complete_tx.send(()); - } - } - } - } -} - -async fn run_websocket_inbound_loop( - mut websocket_reader: futures::stream::SplitStream, - transport_event_tx: mpsc::Sender, - writer_tx_for_reader: mpsc::Sender, - writer_control_tx: mpsc::Sender, - connection_id: ConnectionId, - disconnect_token: CancellationToken, -) { - loop { - tokio::select! { - _ = disconnect_token.cancelled() => { - break; - } - incoming_message = websocket_reader.next() => { - match incoming_message { - Some(Ok(WebSocketMessage::Text(text))) => { - if !forward_incoming_message( - &transport_event_tx, - &writer_tx_for_reader, - connection_id, - text.as_ref(), - ) - .await - { - break; - } - } - Some(Ok(WebSocketMessage::Ping(payload))) => { - match writer_control_tx.try_send(WebSocketMessage::Pong(payload)) { - Ok(()) => {} - Err(tokio::sync::mpsc::error::TrySendError::Closed(_)) => break, - Err(tokio::sync::mpsc::error::TrySendError::Full(_)) => { - warn!("websocket control queue full while replying to ping; closing connection"); - break; - } - } - } - Some(Ok(WebSocketMessage::Pong(_))) => {} - Some(Ok(WebSocketMessage::Close(_))) | None => break, - Some(Ok(WebSocketMessage::Binary(_))) => { - warn!("dropping unsupported binary websocket message"); - } - Some(Err(err)) => { - warn!("websocket receive error: {err}"); - break; - } - } - } - } - } -} - async fn forward_incoming_message( transport_event_tx: &mpsc::Sender, writer: &mpsc::Sender, diff --git a/codex-rs/app-server/src/transport/stdio.rs b/codex-rs/app-server/src/transport/stdio.rs new file mode 100644 index 0000000000..4f2bf26745 --- /dev/null +++ b/codex-rs/app-server/src/transport/stdio.rs @@ -0,0 +1,88 @@ +use super::CHANNEL_CAPACITY; +use super::TransportEvent; +use super::forward_incoming_message; +use super::serialize_outgoing_message; +use crate::outgoing_message::ConnectionId; +use crate::outgoing_message::QueuedOutgoingMessage; +use std::io::ErrorKind; +use std::io::Result as IoResult; +use tokio::io; +use tokio::io::AsyncBufReadExt; +use tokio::io::AsyncWriteExt; +use tokio::io::BufReader; +use tokio::sync::mpsc; +use tokio::task::JoinHandle; +use tracing::debug; +use tracing::error; +use tracing::info; + +pub(crate) async fn start_stdio_connection( + transport_event_tx: mpsc::Sender, + stdio_handles: &mut Vec>, +) -> IoResult<()> { + let connection_id = ConnectionId(0); + let (writer_tx, mut writer_rx) = mpsc::channel::(CHANNEL_CAPACITY); + let writer_tx_for_reader = writer_tx.clone(); + transport_event_tx + .send(TransportEvent::ConnectionOpened { + connection_id, + writer: writer_tx, + disconnect_sender: None, + }) + .await + .map_err(|_| std::io::Error::new(ErrorKind::BrokenPipe, "processor unavailable"))?; + + let transport_event_tx_for_reader = transport_event_tx.clone(); + stdio_handles.push(tokio::spawn(async move { + let stdin = io::stdin(); + let reader = BufReader::new(stdin); + let mut lines = reader.lines(); + + loop { + match lines.next_line().await { + Ok(Some(line)) => { + if !forward_incoming_message( + &transport_event_tx_for_reader, + &writer_tx_for_reader, + connection_id, + &line, + ) + .await + { + break; + } + } + Ok(None) => break, + Err(err) => { + error!("Failed reading stdin: {err}"); + break; + } + } + } + + let _ = transport_event_tx_for_reader + .send(TransportEvent::ConnectionClosed { connection_id }) + .await; + debug!("stdin reader finished (EOF)"); + })); + + stdio_handles.push(tokio::spawn(async move { + let mut stdout = io::stdout(); + while let Some(queued_message) = writer_rx.recv().await { + let Some(mut json) = serialize_outgoing_message(queued_message.message) else { + continue; + }; + json.push('\n'); + if let Err(err) = stdout.write_all(json.as_bytes()).await { + error!("Failed to write to stdout: {err}"); + break; + } + if let Some(write_complete_tx) = queued_message.write_complete_tx { + let _ = write_complete_tx.send(()); + } + } + info!("stdout writer exited (channel closed)"); + })); + + Ok(()) +} diff --git a/codex-rs/app-server/src/transport/websocket.rs b/codex-rs/app-server/src/transport/websocket.rs new file mode 100644 index 0000000000..05dfe24b05 --- /dev/null +++ b/codex-rs/app-server/src/transport/websocket.rs @@ -0,0 +1,308 @@ +use super::CHANNEL_CAPACITY; +use super::TransportEvent; +use super::auth::WebsocketAuthPolicy; +use super::auth::authorize_upgrade; +use super::auth::should_warn_about_unauthenticated_non_loopback_listener; +use super::forward_incoming_message; +use super::serialize_outgoing_message; +use crate::outgoing_message::ConnectionId; +use crate::outgoing_message::QueuedOutgoingMessage; +use axum::Router; +use axum::body::Body; +use axum::extract::ConnectInfo; +use axum::extract::State; +use axum::extract::ws::Message as WebSocketMessage; +use axum::extract::ws::WebSocket; +use axum::extract::ws::WebSocketUpgrade; +use axum::http::HeaderMap; +use axum::http::Request; +use axum::http::StatusCode; +use axum::http::header::ORIGIN; +use axum::middleware; +use axum::middleware::Next; +use axum::response::IntoResponse; +use axum::response::Response; +use axum::routing::any; +use axum::routing::get; +use futures::SinkExt; +use futures::StreamExt; +use owo_colors::OwoColorize; +use owo_colors::Stream; +use owo_colors::Style; +use std::io::Result as IoResult; +use std::net::SocketAddr; +use std::sync::Arc; +use std::sync::atomic::AtomicU64; +use std::sync::atomic::Ordering; +use tokio::net::TcpListener; +use tokio::sync::mpsc; +use tokio::task::JoinHandle; +use tokio_util::sync::CancellationToken; +use tracing::error; +use tracing::info; +use tracing::warn; + +fn colorize(text: &str, style: Style) -> String { + text.if_supports_color(Stream::Stderr, |value| value.style(style)) + .to_string() +} + +#[allow(clippy::print_stderr)] +fn print_websocket_startup_banner(addr: SocketAddr) { + let title = colorize("codex app-server (WebSockets)", Style::new().bold().cyan()); + let listening_label = colorize("listening on:", Style::new().dimmed()); + let listen_url = colorize(&format!("ws://{addr}"), Style::new().green()); + let ready_label = colorize("readyz:", Style::new().dimmed()); + let ready_url = colorize(&format!("http://{addr}/readyz"), Style::new().green()); + let health_label = colorize("healthz:", Style::new().dimmed()); + let health_url = colorize(&format!("http://{addr}/healthz"), Style::new().green()); + let note_label = colorize("note:", Style::new().dimmed()); + eprintln!("{title}"); + eprintln!(" {listening_label} {listen_url}"); + eprintln!(" {ready_label} {ready_url}"); + eprintln!(" {health_label} {health_url}"); + if addr.ip().is_loopback() { + eprintln!( + " {note_label} binds localhost only (use SSH port-forwarding for remote access)" + ); + } else { + eprintln!( + " {note_label} websocket auth is opt-in in this build; configure `--ws-auth ...` before real remote use" + ); + } +} + +#[derive(Clone)] +struct WebSocketListenerState { + transport_event_tx: mpsc::Sender, + connection_counter: Arc, + auth_policy: Arc, +} + +async fn health_check_handler() -> StatusCode { + StatusCode::OK +} + +async fn reject_requests_with_origin_header( + request: Request, + next: Next, +) -> Result { + if request.headers().contains_key(ORIGIN) { + warn!( + method = %request.method(), + uri = %request.uri(), + "rejecting websocket listener request with Origin header" + ); + Err(StatusCode::FORBIDDEN) + } else { + Ok(next.run(request).await) + } +} + +async fn websocket_upgrade_handler( + websocket: WebSocketUpgrade, + ConnectInfo(peer_addr): ConnectInfo, + State(state): State, + headers: HeaderMap, +) -> impl IntoResponse { + if let Err(err) = authorize_upgrade(&headers, state.auth_policy.as_ref()) { + warn!( + %peer_addr, + message = err.message(), + "rejecting websocket client during upgrade" + ); + return (err.status_code(), err.message()).into_response(); + } + let connection_id = ConnectionId(state.connection_counter.fetch_add(1, Ordering::Relaxed)); + info!(%peer_addr, "websocket client connected"); + websocket + .on_upgrade(move |stream| async move { + run_websocket_connection(connection_id, stream, state.transport_event_tx).await; + }) + .into_response() +} + +pub(crate) async fn start_websocket_acceptor( + bind_address: SocketAddr, + transport_event_tx: mpsc::Sender, + shutdown_token: CancellationToken, + auth_policy: WebsocketAuthPolicy, +) -> IoResult> { + if should_warn_about_unauthenticated_non_loopback_listener(bind_address, &auth_policy) { + warn!( + %bind_address, + "starting non-loopback websocket listener without auth; websocket auth is opt-in for now and will become the default in a future release" + ); + } + let listener = TcpListener::bind(bind_address).await?; + let local_addr = listener.local_addr()?; + print_websocket_startup_banner(local_addr); + info!("app-server websocket listening on ws://{local_addr}"); + + let router = Router::new() + .route("/readyz", get(health_check_handler)) + .route("/healthz", get(health_check_handler)) + .fallback(any(websocket_upgrade_handler)) + .layer(middleware::from_fn(reject_requests_with_origin_header)) + .with_state(WebSocketListenerState { + transport_event_tx, + connection_counter: Arc::new(AtomicU64::new(1)), + auth_policy: Arc::new(auth_policy), + }); + let server = axum::serve( + listener, + router.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(async move { + shutdown_token.cancelled().await; + }); + Ok(tokio::spawn(async move { + if let Err(err) = server.await { + error!("websocket acceptor failed: {err}"); + } + info!("websocket acceptor shutting down"); + })) +} + +async fn run_websocket_connection( + connection_id: ConnectionId, + websocket_stream: WebSocket, + transport_event_tx: mpsc::Sender, +) { + let (writer_tx, writer_rx) = mpsc::channel::(CHANNEL_CAPACITY); + let writer_tx_for_reader = writer_tx.clone(); + let disconnect_token = CancellationToken::new(); + if transport_event_tx + .send(TransportEvent::ConnectionOpened { + connection_id, + writer: writer_tx, + disconnect_sender: Some(disconnect_token.clone()), + }) + .await + .is_err() + { + return; + } + + let (websocket_writer, websocket_reader) = websocket_stream.split(); + let (writer_control_tx, writer_control_rx) = + mpsc::channel::(CHANNEL_CAPACITY); + let mut outbound_task = tokio::spawn(run_websocket_outbound_loop( + websocket_writer, + writer_rx, + writer_control_rx, + disconnect_token.clone(), + )); + let mut inbound_task = tokio::spawn(run_websocket_inbound_loop( + websocket_reader, + transport_event_tx.clone(), + writer_tx_for_reader, + writer_control_tx, + connection_id, + disconnect_token.clone(), + )); + + tokio::select! { + _ = &mut outbound_task => { + disconnect_token.cancel(); + inbound_task.abort(); + } + _ = &mut inbound_task => { + disconnect_token.cancel(); + outbound_task.abort(); + } + } + + let _ = transport_event_tx + .send(TransportEvent::ConnectionClosed { connection_id }) + .await; +} + +async fn run_websocket_outbound_loop( + mut websocket_writer: futures::stream::SplitSink, + mut writer_rx: mpsc::Receiver, + mut writer_control_rx: mpsc::Receiver, + disconnect_token: CancellationToken, +) { + loop { + tokio::select! { + _ = disconnect_token.cancelled() => { + break; + } + message = writer_control_rx.recv() => { + let Some(message) = message else { + break; + }; + if websocket_writer.send(message).await.is_err() { + break; + } + } + queued_message = writer_rx.recv() => { + let Some(queued_message) = queued_message else { + break; + }; + let Some(json) = serialize_outgoing_message(queued_message.message) else { + continue; + }; + if websocket_writer.send(WebSocketMessage::Text(json.into())).await.is_err() { + break; + } + if let Some(write_complete_tx) = queued_message.write_complete_tx { + let _ = write_complete_tx.send(()); + } + } + } + } +} + +async fn run_websocket_inbound_loop( + mut websocket_reader: futures::stream::SplitStream, + transport_event_tx: mpsc::Sender, + writer_tx_for_reader: mpsc::Sender, + writer_control_tx: mpsc::Sender, + connection_id: ConnectionId, + disconnect_token: CancellationToken, +) { + loop { + tokio::select! { + _ = disconnect_token.cancelled() => { + break; + } + incoming_message = websocket_reader.next() => { + match incoming_message { + Some(Ok(WebSocketMessage::Text(text))) => { + if !forward_incoming_message( + &transport_event_tx, + &writer_tx_for_reader, + connection_id, + text.as_ref(), + ) + .await + { + break; + } + } + Some(Ok(WebSocketMessage::Ping(payload))) => { + match writer_control_tx.try_send(WebSocketMessage::Pong(payload)) { + Ok(()) => {} + Err(tokio::sync::mpsc::error::TrySendError::Closed(_)) => break, + Err(tokio::sync::mpsc::error::TrySendError::Full(_)) => { + warn!("websocket control queue full while replying to ping; closing connection"); + break; + } + } + } + Some(Ok(WebSocketMessage::Pong(_))) => {} + Some(Ok(WebSocketMessage::Close(_))) | None => break, + Some(Ok(WebSocketMessage::Binary(_))) => { + warn!("dropping unsupported binary websocket message"); + } + Some(Err(err)) => { + warn!("websocket receive error: {err}"); + break; + } + } + } + } + } +} From 86764af684e9091963085f361daa3c521649bb76 Mon Sep 17 00:00:00 2001 From: rreichel3-oai Date: Thu, 26 Mar 2026 16:06:53 -0400 Subject: [PATCH 5/6] Protect first-time project .codex creation across Linux and macOS sandboxes (#15067) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Codex already treated an existing top-level project `./.codex` directory as protected, but there was a gap on first creation. If `./.codex` did not exist yet, a turn could create files under it, such as `./.codex/config.toml`, without going through the same approval path as later modifications. That meant the initial write could bypass the intended protection for project-local Codex state. ## What this changes This PR closes that first-creation gap in the Unix enforcement layers: - `codex-protocol` - treat the top-level project `./.codex` path as a protected carveout even when it does not exist yet - avoid injecting the default carveout when the user already has an explicit rule for that exact path - macOS Seatbelt - deny writes to both the exact protected path and anything beneath it, so creating `./.codex` itself is blocked in addition to writes inside it - Linux bubblewrap - preserve the same protected-path behavior for first-time creation under `./.codex` - tests - add protocol regressions for missing `./.codex` and explicit-rule collisions - add Unix sandbox coverage for blocking first-time `./.codex` creation - tighten Seatbelt policy assertions around excluded subpaths ## Scope This change is intentionally scoped to protecting the top-level project `.codex` subtree from agent writes. It does not make `.codex` unreadable, and it does not change the product behavior around loading project skills from `.codex` when project config is untrusted. ## Why this shape The fix is pointed rather than broad: - it preserves the current model of “project `.codex` is protected from writes” - it closes the security-relevant first-write hole - it avoids folding a larger permissions-model redesign into this PR ## Validation - `cargo test -p codex-protocol` - `cargo test -p codex-sandboxing seatbelt` - `cargo test -p codex-exec --test all sandbox_blocks_first_time_dot_codex_creation -- --nocapture` --------- Co-authored-by: Michael Bolin --- codex-rs/core/src/codex_tests.rs | 33 +-- codex-rs/core/src/codex_thread.rs | 10 + codex-rs/core/src/exec_tests.rs | 18 +- codex-rs/core/src/safety_tests.rs | 34 +++ codex-rs/exec/tests/suite/sandbox.rs | 73 +++++++ codex-rs/linux-sandbox/src/bwrap.rs | 11 + codex-rs/protocol/src/permissions.rs | 248 ++++++++++++++++++++-- codex-rs/protocol/src/protocol.rs | 54 +++-- codex-rs/sandboxing/src/seatbelt.rs | 8 +- codex-rs/sandboxing/src/seatbelt_tests.rs | 194 ++++++++++++----- 10 files changed, 571 insertions(+), 112 deletions(-) diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 8bd6759202..5a556f2071 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -1162,35 +1162,10 @@ async fn fork_startup_context_then_first_turn_diff_snapshot() -> anyhow::Result< }) .await?; wait_for_event(&initial.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; - // The parent rollout writer drains asynchronously after turn completion. - // Wait until the persisted JSONL includes the source user turn before forking from it. - let mut source_history_persisted = false; - for _ in 0..100 { - let history = RolloutRecorder::get_rollout_history(&rollout_path).await; - source_history_persisted = history.ok().is_some_and(|history| { - history.get_rollout_items().into_iter().any(|item| { - matches!( - item, - RolloutItem::ResponseItem(ResponseItem::Message { role, content, .. }) - if role == "user" - && content.iter().any(|content_item| { - matches!( - content_item, - ContentItem::InputText { text } if text == "fork seed" - ) - }) - ) - }) - }); - if source_history_persisted { - break; - } - sleep(StdDuration::from_millis(10)).await; - } - assert!( - source_history_persisted, - "source rollout should contain the completed pre-fork user turn before forking" - ); + // Forking reads the persisted rollout JSONL, so force the completed source turn to disk + // before snapshotting from it. + initial.codex.ensure_rollout_materialized().await; + initial.codex.flush_rollout().await; let mut fork_config = initial.config.clone(); fork_config.permissions.approval_policy = diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 9fa7069118..0635718c9c 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -74,6 +74,16 @@ impl CodexThread { self.codex.shutdown_and_wait().await } + #[doc(hidden)] + pub async fn ensure_rollout_materialized(&self) { + self.codex.session.ensure_rollout_materialized().await; + } + + #[doc(hidden)] + pub async fn flush_rollout(&self) { + self.codex.session.flush_rollout().await; + } + pub async fn submit_with_trace( &self, op: Op, diff --git a/codex-rs/core/src/exec_tests.rs b/codex-rs/core/src/exec_tests.rs index 074db95e1c..07532e5a18 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -595,6 +595,19 @@ fn windows_restricted_token_supports_full_read_split_write_read_carveouts() { }, ]); + #[cfg(windows)] + let expected_deny_write_paths = vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(cwd.join(".codex")) + .expect("absolute .codex"), + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(&docs) + .expect("absolute docs"), + ]; + #[cfg(not(windows))] + let expected_deny_write_paths = vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(&docs) + .expect("absolute docs"), + ]; + assert_eq!( resolve_windows_restricted_token_filesystem_overlay( SandboxType::WindowsRestrictedToken, @@ -605,10 +618,7 @@ fn windows_restricted_token_supports_full_read_split_write_read_carveouts() { WindowsSandboxLevel::RestrictedToken, ), Ok(Some(WindowsRestrictedTokenFilesystemOverlay { - additional_deny_write_paths: vec![ - codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(&docs) - .expect("absolute docs"), - ], + additional_deny_write_paths: expected_deny_write_paths, })) ); } diff --git a/codex-rs/core/src/safety_tests.rs b/codex-rs/core/src/safety_tests.rs index 3d05664ba3..e7f55693d1 100644 --- a/codex-rs/core/src/safety_tests.rs +++ b/codex-rs/core/src/safety_tests.rs @@ -252,3 +252,37 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { SafetyCheck::AskUser, ); } + +#[test] +fn missing_project_dot_codex_config_requires_approval() { + let tmp = TempDir::new().unwrap(); + let cwd = tmp.path().to_path_buf(); + let action = + ApplyPatchAction::new_add_for_test(&cwd.join(".codex").join("config.toml"), "".to_string()); + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: Default::default(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let file_system_sandbox_policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, &cwd); + + assert!(!is_write_patch_constrained_to_writable_paths( + &action, + &file_system_sandbox_policy, + &cwd, + )); + assert_eq!( + assess_patch_safety( + &action, + AskForApproval::OnRequest, + &sandbox_policy, + &file_system_sandbox_policy, + &cwd, + WindowsSandboxLevel::Disabled, + ), + SafetyCheck::AskUser, + ); +} diff --git a/codex-rs/exec/tests/suite/sandbox.rs b/codex-rs/exec/tests/suite/sandbox.rs index 744a9998ff..196fc68e84 100644 --- a/codex-rs/exec/tests/suite/sandbox.rs +++ b/codex-rs/exec/tests/suite/sandbox.rs @@ -118,6 +118,7 @@ async fn python_multiprocessing_lock_works_under_sandbox() { #[cfg(target_os = "linux")] let sandbox_env = match linux_sandbox_test_env().await { Some(env) => env, + // Skip on Linux hosts where Landlock cannot actually be enforced. None => return, }; #[cfg(not(target_os = "linux"))] @@ -312,6 +313,78 @@ async fn sandbox_distinguishes_command_and_policy_cwds() { assert!(allowed_exists, "allowed path should exist"); } +#[tokio::test] +async fn sandbox_blocks_first_time_dot_codex_creation() { + core_test_support::skip_if_sandbox!(); + #[cfg(target_os = "linux")] + let sandbox_env = match linux_sandbox_test_env().await { + Some(env) => env, + None => return, + }; + #[cfg(not(target_os = "linux"))] + let sandbox_env = HashMap::new(); + + let temp = tempfile::tempdir().expect("should be able to create temp dir"); + let repo_root = temp.path().join("repo"); + create_dir_all(&repo_root).await.expect("mkdir repo"); + let dot_codex = repo_root.join(".codex"); + let config_toml = dot_codex.join("config.toml"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: Default::default(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let mut child = spawn_command_under_sandbox( + vec![ + "bash".to_string(), + "-lc".to_string(), + "mkdir -p .codex && echo 'sandbox_mode = \"danger-full-access\"' > .codex/config.toml" + .to_string(), + ], + repo_root.clone(), + &policy, + repo_root.as_path(), + StdioPolicy::RedirectForShellTool, + sandbox_env, + ) + .await + .expect("should spawn command creating .codex"); + + let status = child.wait().await.expect("should wait for .codex command"); + assert!( + !status.success(), + "sandbox unexpectedly allowed first-time .codex creation: {status:?}" + ); + let dot_codex_metadata = tokio::fs::symlink_metadata(&dot_codex).await; + if let Ok(metadata) = dot_codex_metadata { + assert!( + !metadata.is_dir(), + "{} should not be creatable as a directory", + dot_codex.display() + ); + } else if let Err(err) = &dot_codex_metadata { + assert_eq!( + err.kind(), + io::ErrorKind::NotFound, + "unexpected metadata error for {}: {err}", + dot_codex.display() + ); + } + let config_toml_exists = match tokio::fs::try_exists(&config_toml).await { + Ok(exists) => exists, + Err(err) if err.kind() == io::ErrorKind::NotADirectory => false, + Err(err) => panic!("try_exists {} failed: {err}", config_toml.display()), + }; + assert!( + !config_toml_exists, + "{} should not have been created", + config_toml.display() + ); +} + fn unix_sock_body() { unsafe { let mut fds = [0i32; 2]; diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 05e7b58e2c..dfc5b1282a 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -770,14 +770,25 @@ mod tests { assert_eq!( args.args, vec![ + // Start from a read-only view of the full filesystem. "--ro-bind".to_string(), "/".to_string(), "/".to_string(), + // Recreate a writable /dev inside the sandbox. "--dev".to_string(), "/dev".to_string(), + // Make the writable root itself writable again. "--bind".to_string(), "/".to_string(), "/".to_string(), + // Mask the default protected .codex subpath under that writable + // root. Because the root is `/` in this test, the carveout path + // appears as `/.codex`. + "--ro-bind".to_string(), + "/dev/null".to_string(), + "/.codex".to_string(), + // Rebind /dev after the root bind so device nodes remain + // writable/usable inside the writable root. "--bind".to_string(), "/dev".to_string(), "/dev".to_string(), diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 0a5b337029..978334f696 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -264,7 +264,7 @@ impl FileSystemSandboxPolicy { /// into split filesystem policy. pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Self { let mut file_system_policy = Self::from(sandbox_policy); - if matches!(sandbox_policy, SandboxPolicy::WorkspaceWrite { .. }) { + if let SandboxPolicy::WorkspaceWrite { writable_roots, .. } = sandbox_policy { let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); file_system_policy.entries.retain(|entry| { if entry.access != FileSystemAccessMode::Read { @@ -278,6 +278,28 @@ impl FileSystemSandboxPolicy { FileSystemPath::Special { .. } => true, } }); + + if let Ok(cwd_root) = AbsolutePathBuf::from_absolute_path(cwd) { + for protected_path in default_read_only_subpaths_for_writable_root( + &cwd_root, /*protect_missing_dot_codex*/ true, + ) { + append_default_read_only_path_if_no_explicit_rule( + &mut file_system_policy.entries, + protected_path, + ); + } + } + for writable_root in writable_roots { + for protected_path in default_read_only_subpaths_for_writable_root( + writable_root, + /*protect_missing_dot_codex*/ false, + ) { + append_default_read_only_path_if_no_explicit_rule( + &mut file_system_policy.entries, + protected_path, + ); + } + } } file_system_policy @@ -454,7 +476,14 @@ impl FileSystemSandboxPolicy { .iter() .filter(|path| normalize_effective_absolute_path((*path).clone()) == root) .collect(); - let mut read_only_subpaths = default_read_only_subpaths_for_writable_root(&root); + let protect_missing_dot_codex = AbsolutePathBuf::from_absolute_path(cwd) + .ok() + .is_some_and(|cwd| normalize_effective_absolute_path(cwd) == root); + let mut read_only_subpaths: Vec = + default_read_only_subpaths_for_writable_root(&root, protect_missing_dot_codex) + .into_iter() + .filter(|path| !has_explicit_resolved_path_entry(&resolved_entries, path)) + .collect(); // Narrower explicit non-write entries carve out broader writable roots. // More specific write entries still remain writable because they appear // as separate WritableRoot values and are checked independently. @@ -1068,6 +1097,7 @@ fn normalize_effective_absolute_path(path: AbsolutePathBuf) -> AbsolutePathBuf { fn default_read_only_subpaths_for_writable_root( writable_root: &AbsolutePathBuf, + protect_missing_dot_codex: bool, ) -> Vec { let mut subpaths: Vec = Vec::new(); #[allow(clippy::expect_used)] @@ -1089,19 +1119,69 @@ fn default_read_only_subpaths_for_writable_root( subpaths.push(top_level_git); } - // Make .agents/skills and .codex/config.toml and related files read-only - // to the agent, by default. - for subdir in &[".agents", ".codex"] { - #[allow(clippy::expect_used)] - let top_level_codex = writable_root.join(subdir).expect("valid relative path"); - if top_level_codex.as_path().is_dir() { - subpaths.push(top_level_codex); - } + #[allow(clippy::expect_used)] + let top_level_agents = writable_root.join(".agents").expect("valid relative path"); + if top_level_agents.as_path().is_dir() { + subpaths.push(top_level_agents); + } + + // Keep top-level project metadata under .codex read-only to the agent by + // default. For the workspace root itself, protect it even before the + // directory exists so first-time creation still goes through the + // protected-path approval flow. + #[allow(clippy::expect_used)] + let top_level_codex = writable_root.join(".codex").expect("valid relative path"); + if protect_missing_dot_codex || top_level_codex.as_path().is_dir() { + subpaths.push(top_level_codex); } dedup_absolute_paths(subpaths, /*normalize_effective_paths*/ false) } +fn append_path_entry_if_missing( + entries: &mut Vec, + path: AbsolutePathBuf, + access: FileSystemAccessMode, +) { + if entries.iter().any(|entry| { + entry.access == access + && matches!( + &entry.path, + FileSystemPath::Path { path: existing } if existing == &path + ) + }) { + return; + } + + entries.push(FileSystemSandboxEntry { + path: FileSystemPath::Path { path }, + access, + }); +} + +fn append_default_read_only_path_if_no_explicit_rule( + entries: &mut Vec, + path: AbsolutePathBuf, +) { + if entries.iter().any(|entry| { + matches!( + &entry.path, + FileSystemPath::Path { path: existing } if existing == &path + ) + }) { + return; + } + + append_path_entry_if_missing(entries, path, FileSystemAccessMode::Read); +} + +fn has_explicit_resolved_path_entry( + entries: &[ResolvedFileSystemEntry], + path: &AbsolutePathBuf, +) -> bool { + entries.iter().any(|entry| &entry.path == path) +} + fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool { path.as_path().is_file() && path.as_path().file_name() == Some(OsStr::new(".git")) } @@ -1211,6 +1291,148 @@ mod tests { Ok(()) } + #[cfg(unix)] + #[test] + fn writable_roots_proactively_protect_missing_dot_codex() { + let cwd = TempDir::new().expect("tempdir"); + let expected_root = AbsolutePathBuf::from_absolute_path( + cwd.path().canonicalize().expect("canonicalize cwd"), + ) + .expect("absolute canonical root"); + let expected_dot_codex = expected_root.join(".codex").expect("expected .codex path"); + + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!(writable_roots[0].root, expected_root); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_dot_codex) + ); + } + + #[cfg(unix)] + #[test] + fn writable_roots_skip_default_dot_codex_when_explicit_user_rule_exists() { + let cwd = TempDir::new().expect("tempdir"); + let expected_root = AbsolutePathBuf::from_absolute_path( + cwd.path().canonicalize().expect("canonicalize cwd"), + ) + .expect("absolute canonical root"); + let explicit_dot_codex = expected_root.join(".codex").expect("expected .codex path"); + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: explicit_dot_codex.clone(), + }, + access: FileSystemAccessMode::Write, + }, + ]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + let workspace_root = writable_roots + .iter() + .find(|root| root.root == expected_root) + .expect("workspace writable root"); + assert!( + !workspace_root + .read_only_subpaths + .contains(&explicit_dot_codex), + "explicit .codex rule should win over the default protected carveout" + ); + assert!( + policy.can_write_path_with_cwd( + explicit_dot_codex + .join("config.toml") + .expect("config.toml") + .as_path(), + cwd.path() + ) + ); + } + + #[test] + fn legacy_workspace_write_projection_blocks_missing_dot_codex_writes() { + let cwd = TempDir::new().expect("tempdir"); + let dot_codex_config = cwd.path().join(".codex").join("config.toml"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![], + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let file_system_policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd.path()); + + assert!(!file_system_policy.can_write_path_with_cwd(&dot_codex_config, cwd.path())); + } + + #[test] + fn legacy_workspace_write_projection_accepts_relative_cwd() { + let relative_cwd = Path::new("workspace"); + let expected_dot_codex = AbsolutePathBuf::from_absolute_path( + std::env::current_dir() + .expect("current dir") + .join(relative_cwd) + .join(".codex"), + ) + .expect("absolute dot codex"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![], + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let file_system_policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, relative_cwd); + + assert_eq!( + file_system_policy, + FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: expected_dot_codex, + }, + access: FileSystemAccessMode::Read, + }, + ]) + ); + assert!( + !file_system_policy + .can_write_path_with_cwd(Path::new(".codex/config.toml"), relative_cwd,) + ); + } + #[cfg(unix)] #[test] fn effective_runtime_roots_canonicalize_symlinked_paths() { @@ -1695,8 +1917,10 @@ mod tests { policy.needs_direct_runtime_enforcement(NetworkSandboxPolicy::Restricted, cwd.path(),) ); - let legacy_workspace_write = - FileSystemSandboxPolicy::from(&SandboxPolicy::new_workspace_write_policy()); + let legacy_workspace_write = FileSystemSandboxPolicy::from_legacy_sandbox_policy( + &SandboxPolicy::new_workspace_write_policy(), + cwd.path(), + ); assert!( !legacy_workspace_write .needs_direct_runtime_enforcement(NetworkSandboxPolicy::Restricted, cwd.path(),) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 2e67fa5d73..71c9cbda64 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1064,13 +1064,20 @@ impl SandboxPolicy { } // For each root, compute subpaths that should remain read-only. + let cwd_root = AbsolutePathBuf::from_absolute_path(cwd).ok(); roots .into_iter() - .map(|writable_root| WritableRoot { - read_only_subpaths: default_read_only_subpaths_for_writable_root( - &writable_root, - ), - root: writable_root, + .map(|writable_root| { + let protect_missing_dot_codex = cwd_root + .as_ref() + .is_some_and(|cwd_root| cwd_root == &writable_root); + WritableRoot { + read_only_subpaths: default_read_only_subpaths_for_writable_root( + &writable_root, + protect_missing_dot_codex, + ), + root: writable_root, + } }) .collect() } @@ -1080,6 +1087,7 @@ impl SandboxPolicy { fn default_read_only_subpaths_for_writable_root( writable_root: &AbsolutePathBuf, + protect_missing_dot_codex: bool, ) -> Vec { let mut subpaths: Vec = Vec::new(); #[allow(clippy::expect_used)] @@ -1101,14 +1109,20 @@ fn default_read_only_subpaths_for_writable_root( subpaths.push(top_level_git); } - // Make .agents/skills and .codex/config.toml and related files read-only - // to the agent, by default. - for subdir in &[".agents", ".codex"] { - #[allow(clippy::expect_used)] - let top_level_codex = writable_root.join(subdir).expect("valid relative path"); - if top_level_codex.as_path().is_dir() { - subpaths.push(top_level_codex); - } + #[allow(clippy::expect_used)] + let top_level_agents = writable_root.join(".agents").expect("valid relative path"); + if top_level_agents.as_path().is_dir() { + subpaths.push(top_level_agents); + } + + // Keep top-level project metadata under .codex read-only to the agent by + // default. For the workspace root itself, protect it even before the + // directory exists so first-time creation still goes through the + // protected-path approval flow. + #[allow(clippy::expect_used)] + let top_level_codex = writable_root.join(".codex").expect("valid relative path"); + if protect_missing_dot_codex || top_level_codex.as_path().is_dir() { + subpaths.push(top_level_codex); } let mut deduped = Vec::with_capacity(subpaths.len()); @@ -4095,6 +4109,8 @@ mod tests { let expected_docs_public = AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs/public")) .expect("canonical docs/public"); + let expected_dot_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex")) + .expect("canonical .codex"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { @@ -4116,7 +4132,13 @@ mod tests { assert_eq!( sorted_writable_roots(policy.get_writable_roots_with_cwd(cwd.path())), vec![ - (canonical_cwd, vec![expected_docs.to_path_buf()]), + ( + canonical_cwd, + vec![ + expected_dot_codex.to_path_buf(), + expected_docs.to_path_buf() + ], + ), (expected_docs_public.to_path_buf(), Vec::new()), ] ); @@ -4128,6 +4150,8 @@ mod tests { let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()).expect("resolve docs"); let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); + let expected_dot_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex")) + .expect("canonical .codex"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: ReadOnlyAccess::Restricted { @@ -4144,7 +4168,7 @@ mod tests { FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd.path()) .get_writable_roots_with_cwd(cwd.path()) ), - vec![(canonical_cwd, Vec::new())] + vec![(canonical_cwd, vec![expected_dot_codex.to_path_buf()])] ); } diff --git a/codex-rs/sandboxing/src/seatbelt.rs b/codex-rs/sandboxing/src/seatbelt.rs index e7cca3d2ac..e6ff71d0d3 100644 --- a/codex-rs/sandboxing/src/seatbelt.rs +++ b/codex-rs/sandboxing/src/seatbelt.rs @@ -330,8 +330,14 @@ fn build_seatbelt_access_policy( { let excluded_subpath = normalize_path_for_sandbox(excluded_subpath.as_path()).unwrap_or(excluded_subpath); - let excluded_param = format!("{param_prefix}_{index}_RO_{excluded_index}"); + let excluded_param = format!("{param_prefix}_{index}_EXCLUDED_{excluded_index}"); params.push((excluded_param.clone(), excluded_subpath.into_path_buf())); + // Exclude both the exact protected path and anything beneath it. + // `subpath` alone leaves a gap for first-time creation of the + // protected directory itself, such as `mkdir .codex`. + require_parts.push(format!( + "(require-not (literal (param \"{excluded_param}\")))" + )); require_parts.push(format!( "(require-not (subpath (param \"{excluded_param}\")))" )); diff --git a/codex-rs/sandboxing/src/seatbelt_tests.rs b/codex-rs/sandboxing/src/seatbelt_tests.rs index 925285763e..e508a028c2 100644 --- a/codex-rs/sandboxing/src/seatbelt_tests.rs +++ b/codex-rs/sandboxing/src/seatbelt_tests.rs @@ -129,22 +129,40 @@ fn explicit_unreadable_paths_are_excluded_from_full_disk_read_and_write_access() let unreadable_roots = file_system_policy.get_unreadable_roots_with_cwd(Path::new("/")); let unreadable_root = unreadable_roots.first().expect("expected unreadable root"); assert!( - policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_RO_0\")))"), + policy.contains("(require-not (literal (param \"READABLE_ROOT_0_EXCLUDED_0\")))"), + "expected exact read carveout in policy:\n{policy}" + ); + assert!( + policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_EXCLUDED_0\")))"), "expected read carveout in policy:\n{policy}" ); assert!( - policy.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_RO_0\")))"), + policy.contains("(require-not (literal (param \"WRITABLE_ROOT_0_EXCLUDED_0\")))"), + "expected exact write carveout in policy:\n{policy}" + ); + assert!( + policy.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_EXCLUDED_0\")))"), "expected write carveout in policy:\n{policy}" ); assert!( - args.iter() - .any(|arg| arg == &format!("-DREADABLE_ROOT_0_RO_0={}", unreadable_root.display())), + args.iter().any( + |arg| arg == &format!("-DREADABLE_ROOT_0_EXCLUDED_0={}", unreadable_root.display()) + ), "expected read carveout parameter in args: {args:#?}" ); - assert!( - args.iter() - .any(|arg| arg == &format!("-DWRITABLE_ROOT_0_RO_0={}", unreadable_root.display())), - "expected write carveout parameter in args: {args:#?}" + let writable_definitions: Vec = args + .iter() + .filter(|arg| arg.starts_with("-DWRITABLE_ROOT_")) + .cloned() + .collect(); + assert_eq!( + writable_definitions, + vec![ + "-DWRITABLE_ROOT_0=/".to_string(), + "-DWRITABLE_ROOT_0_EXCLUDED_0=/.codex".to_string(), + format!("-DWRITABLE_ROOT_0_EXCLUDED_1={}", unreadable_root.display()), + ], + "unexpected write carveout parameters in args: {args:#?}" ); } @@ -179,7 +197,11 @@ fn explicit_unreadable_paths_are_excluded_from_readable_roots() { let unreadable_roots = file_system_policy.get_unreadable_roots_with_cwd(Path::new("/")); let unreadable_root = unreadable_roots.first().expect("expected unreadable root"); assert!( - policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_RO_0\")))"), + policy.contains("(require-not (literal (param \"READABLE_ROOT_0_EXCLUDED_0\")))"), + "expected exact read carveout in policy:\n{policy}" + ); + assert!( + policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_EXCLUDED_0\")))"), "expected read carveout in policy:\n{policy}" ); assert!( @@ -188,8 +210,9 @@ fn explicit_unreadable_paths_are_excluded_from_readable_roots() { "expected readable root parameter in args: {args:#?}" ); assert!( - args.iter() - .any(|arg| arg == &format!("-DREADABLE_ROOT_0_RO_0={}", unreadable_root.display())), + args.iter().any( + |arg| arg == &format!("-DREADABLE_ROOT_0_EXCLUDED_0={}", unreadable_root.display()) + ), "expected read carveout parameter in args: {args:#?}" ); } @@ -314,14 +337,17 @@ fn seatbelt_legacy_workspace_write_nested_readable_root_stays_writable() { /*extensions*/ None, ); - let docs_param = format!("-DWRITABLE_ROOT_0_RO_0={}", docs.as_path().display()); assert!( - !seatbelt_policy_arg(&args).contains("WRITABLE_ROOT_0_RO_0"), - "legacy workspace-write readable roots under cwd should not become seatbelt carveouts:\n{args:#?}" + !args + .iter() + .any(|arg| arg.ends_with(&format!("={}", docs.as_path().display()))), + "legacy workspace-write readable roots under cwd should not become seatbelt carveouts:\n{args:#?}", ); assert!( - !args.iter().any(|arg| arg == &docs_param), - "unexpected seatbelt carveout parameter for redundant legacy readable root: {args:#?}" + args.iter() + .any(|arg| arg.starts_with("-DWRITABLE_ROOT_0_EXCLUDED_") + && arg.ends_with("/workspace/.codex")), + "expected proactive .codex carveout for cwd root: {args:#?}", ); } @@ -647,30 +673,24 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { /*extensions*/ None, ); - // Build the expected policy text using a raw string for readability. - // Note that the policy includes: - // - the base policy, - // - read-only access to the filesystem, - // - write access to WRITABLE_ROOT_0 (but not its .git or .codex), WRITABLE_ROOT_1, and cwd as WRITABLE_ROOT_2. - let expected_policy = format!( - r#"{MACOS_SEATBELT_BASE_POLICY} -; allow read-only file operations -(allow file-read*) -(allow file-write* -(subpath (param "WRITABLE_ROOT_0")) (require-all (subpath (param "WRITABLE_ROOT_1")) (require-not (subpath (param "WRITABLE_ROOT_1_RO_0"))) (require-not (subpath (param "WRITABLE_ROOT_1_RO_1"))) ) (subpath (param "WRITABLE_ROOT_2")) -) - -; macOS permission profile extensions -(allow ipc-posix-shm-read* (ipc-posix-name-prefix "apple.cfprefs.")) -(allow mach-lookup - (global-name "com.apple.cfprefsd.daemon") - (global-name "com.apple.cfprefsd.agent") - (local-name "com.apple.cfprefsd.agent")) -(allow user-preference-read) -"#, + let policy_text = seatbelt_policy_arg(&args); + assert!( + policy_text.contains("(require-all (subpath (param \"WRITABLE_ROOT_0\"))"), + "expected cwd writable root to carry protected carveouts:\n{policy_text}", + ); + assert!( + policy_text.contains("WRITABLE_ROOT_0_EXCLUDED_0"), + "expected cwd .codex carveout in policy:\n{policy_text}", + ); + assert!( + policy_text.contains("WRITABLE_ROOT_1_EXCLUDED_0") + && policy_text.contains("WRITABLE_ROOT_1_EXCLUDED_1"), + "expected explicit writable root .git/.codex carveouts in policy:\n{policy_text}", + ); + assert!( + policy_text.contains("(subpath (param \"WRITABLE_ROOT_2\"))"), + "expected second explicit writable root grant in policy:\n{policy_text}", ); - - assert_eq!(seatbelt_policy_arg(&args), expected_policy); let expected_definitions = [ format!( @@ -679,16 +699,23 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { .expect("canonicalize cwd") .to_string_lossy() ), + format!( + "-DWRITABLE_ROOT_0_EXCLUDED_0={}", + cwd.canonicalize() + .expect("canonicalize cwd") + .join(".codex") + .display() + ), format!( "-DWRITABLE_ROOT_1={}", vulnerable_root_canonical.to_string_lossy() ), format!( - "-DWRITABLE_ROOT_1_RO_0={}", + "-DWRITABLE_ROOT_1_EXCLUDED_0={}", dot_git_canonical.to_string_lossy() ), format!( - "-DWRITABLE_ROOT_1_RO_1={}", + "-DWRITABLE_ROOT_1_EXCLUDED_1={}", dot_codex_canonical.to_string_lossy() ), format!( @@ -696,12 +723,15 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { empty_root_canonical.to_string_lossy() ), ]; - for expected_definition in expected_definitions { - assert!( - args.contains(&expected_definition), - "expected definition arg `{expected_definition}` in {args:#?}" - ); - } + let writable_definitions: Vec = args + .iter() + .filter(|arg| arg.starts_with("-DWRITABLE_ROOT_")) + .cloned() + .collect(); + assert_eq!( + writable_definitions, expected_definitions, + "unexpected writable-root parameter definitions in {args:#?}" + ); for (key, value) in macos_dir_params() { let expected_definition = format!("-D{key}={}", value.to_string_lossy()); assert!( @@ -818,6 +848,60 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { ); } +#[test] +fn create_seatbelt_args_block_first_time_dot_codex_creation_with_exact_and_descendant_carveouts() { + let tmp = TempDir::new().expect("tempdir"); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(&repo_root).expect("create repo root"); + + Command::new("git") + .arg("init") + .arg(".") + .current_dir(&repo_root) + .output() + .expect("git init ."); + + let dot_codex = repo_root.join(".codex"); + let config_toml = dot_codex.join("config.toml"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![repo_root.as_path().try_into().expect("absolute repo root")], + read_only_access: Default::default(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let shell_command: Vec = [ + "bash", + "-c", + "mkdir -p \"$1\" && echo 'sandbox_mode = \"danger-full-access\"' > \"$2\"", + "bash", + dot_codex.to_string_lossy().as_ref(), + config_toml.to_string_lossy().as_ref(), + ] + .iter() + .map(std::string::ToString::to_string) + .collect(); + let args = create_seatbelt_command_args_with_extensions( + shell_command, + &policy, + repo_root.as_path(), + /*enforce_managed_network*/ false, + /*network*/ None, + /*extensions*/ None, + ); + + let policy_text = seatbelt_policy_arg(&args); + assert!( + policy_text.contains("(require-not (literal (param \"WRITABLE_ROOT_0_EXCLUDED_1\")))"), + "expected exact .codex carveout in policy:\n{policy_text}" + ); + assert!( + policy_text.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_EXCLUDED_1\")))"), + "expected descendant .codex carveout in policy:\n{policy_text}" + ); +} + #[test] fn create_seatbelt_args_with_read_only_git_pointer_file() { let tmp = TempDir::new().expect("tempdir"); @@ -971,7 +1055,7 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { .map(|p| p.to_string_lossy().to_string()); let tempdir_policy_entry = if tmpdir_env_var.is_some() { - r#" (subpath (param "WRITABLE_ROOT_2"))"# + r#" (require-all (subpath (param "WRITABLE_ROOT_2")) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_0"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_0"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_1"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_1"))) )"# } else { "" }; @@ -986,7 +1070,7 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { ; allow read-only file operations (allow file-read*) (allow file-write* -(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) (require-not (subpath (param "WRITABLE_ROOT_0_RO_1"))) ) (subpath (param "WRITABLE_ROOT_1")){tempdir_policy_entry} +(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (literal (param "WRITABLE_ROOT_0_EXCLUDED_0"))) (require-not (subpath (param "WRITABLE_ROOT_0_EXCLUDED_0"))) (require-not (literal (param "WRITABLE_ROOT_0_EXCLUDED_1"))) (require-not (subpath (param "WRITABLE_ROOT_0_EXCLUDED_1"))) ) (subpath (param "WRITABLE_ROOT_1")){tempdir_policy_entry} ) ; macOS permission profile extensions @@ -1007,11 +1091,11 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { vulnerable_root_canonical.to_string_lossy() ), format!( - "-DWRITABLE_ROOT_0_RO_0={}", + "-DWRITABLE_ROOT_0_EXCLUDED_0={}", dot_git_canonical.to_string_lossy() ), format!( - "-DWRITABLE_ROOT_0_RO_1={}", + "-DWRITABLE_ROOT_0_EXCLUDED_1={}", dot_codex_canonical.to_string_lossy() ), format!( @@ -1025,6 +1109,14 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { if let Some(p) = tmpdir_env_var { expected_args.push(format!("-DWRITABLE_ROOT_2={p}")); + expected_args.push(format!( + "-DWRITABLE_ROOT_2_EXCLUDED_0={}", + dot_git_canonical.to_string_lossy() + )); + expected_args.push(format!( + "-DWRITABLE_ROOT_2_EXCLUDED_1={}", + dot_codex_canonical.to_string_lossy() + )); } expected_args.extend( From fcad39156d62c9892d94f45639636f2dd3ae4a6d Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 26 Mar 2026 13:27:21 -0700 Subject: [PATCH 6/6] chore: remove skill_metadata field --- ...CommandExecutionRequestApprovalParams.json | 11 ---- .../schema/json/ServerRequest.json | 11 ---- .../codex_app_server_protocol.schemas.json | 11 ---- .../CommandExecutionRequestApprovalParams.ts | 5 -- ...ndExecutionRequestApprovalSkillMetadata.ts | 5 -- .../schema/typescript/v2/index.ts | 1 - codex-rs/app-server-protocol/src/export.rs | 10 ---- .../src/protocol/common.rs | 28 ---------- .../app-server-protocol/src/protocol/v2.rs | 54 ------------------- codex-rs/app-server-test-client/src/lib.rs | 4 -- .../app-server/src/bespoke_event_handling.rs | 5 -- codex-rs/app-server/src/transport/mod.rs | 20 ++----- codex-rs/core/src/codex.rs | 3 -- codex-rs/core/src/codex_delegate.rs | 2 - codex-rs/core/src/codex_delegate_tests.rs | 1 - codex-rs/core/src/tools/network_approval.rs | 1 - codex-rs/core/src/tools/runtimes/shell.rs | 1 - .../tools/runtimes/shell/unix_escalation.rs | 1 - .../core/src/tools/runtimes/unified_exec.rs | 1 - codex-rs/mcp-server/src/codex_tool_runner.rs | 1 - codex-rs/protocol/src/approvals.rs | 16 +----- codex-rs/protocol/src/protocol.rs | 1 - codex-rs/tui/src/app.rs | 3 -- .../tui/src/app/pending_interactive_replay.rs | 3 -- codex-rs/tui/src/chatwidget/tests.rs | 8 --- codex-rs/tui_app_server/src/app.rs | 1 - .../src/app/app_server_requests.rs | 1 - .../src/app/pending_interactive_replay.rs | 1 - codex-rs/tui_app_server/src/chatwidget.rs | 5 -- .../tui_app_server/src/chatwidget/tests.rs | 9 ---- 30 files changed, 5 insertions(+), 219 deletions(-) delete mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalSkillMetadata.ts diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 2c146b9522..66dccf15e0 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -298,17 +298,6 @@ } ] }, - "CommandExecutionRequestApprovalSkillMetadata": { - "properties": { - "pathToSkillsMd": { - "type": "string" - } - }, - "required": [ - "pathToSkillsMd" - ], - "type": "object" - }, "MacOsAutomationPermission": { "oneOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 1fbbfb1b0a..6c63d36a37 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -452,17 +452,6 @@ ], "type": "object" }, - "CommandExecutionRequestApprovalSkillMetadata": { - "properties": { - "pathToSkillsMd": { - "type": "string" - } - }, - "required": [ - "pathToSkillsMd" - ], - "type": "object" - }, "DynamicToolCallParams": { "properties": { "arguments": true, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 6253c39297..28be3e5b35 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -1864,17 +1864,6 @@ "title": "CommandExecutionRequestApprovalResponse", "type": "object" }, - "CommandExecutionRequestApprovalSkillMetadata": { - "properties": { - "pathToSkillsMd": { - "type": "string" - } - }, - "required": [ - "pathToSkillsMd" - ], - "type": "object" - }, "DynamicToolCallParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts index 623fb971c1..8fb6375e69 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts @@ -4,7 +4,6 @@ import type { AdditionalPermissionProfile } from "./AdditionalPermissionProfile"; import type { CommandAction } from "./CommandAction"; import type { CommandExecutionApprovalDecision } from "./CommandExecutionApprovalDecision"; -import type { CommandExecutionRequestApprovalSkillMetadata } from "./CommandExecutionRequestApprovalSkillMetadata"; import type { ExecPolicyAmendment } from "./ExecPolicyAmendment"; import type { NetworkApprovalContext } from "./NetworkApprovalContext"; import type { NetworkPolicyAmendment } from "./NetworkPolicyAmendment"; @@ -44,10 +43,6 @@ commandActions?: Array | null, * Optional additional permissions requested for this command. */ additionalPermissions?: AdditionalPermissionProfile | null, -/** - * Optional skill metadata when the approval was triggered by a skill script. - */ -skillMetadata?: CommandExecutionRequestApprovalSkillMetadata | null, /** * Optional proposed execpolicy amendment to allow similar commands without prompting. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalSkillMetadata.ts b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalSkillMetadata.ts deleted file mode 100644 index dcfd7c2916..0000000000 --- a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalSkillMetadata.ts +++ /dev/null @@ -1,5 +0,0 @@ -// GENERATED CODE! DO NOT MODIFY BY HAND! - -// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. - -export type CommandExecutionRequestApprovalSkillMetadata = { pathToSkillsMd: string, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index d0b1b88198..b545f7986f 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -54,7 +54,6 @@ export type { CommandExecutionApprovalDecision } from "./CommandExecutionApprova export type { CommandExecutionOutputDeltaNotification } from "./CommandExecutionOutputDeltaNotification"; export type { CommandExecutionRequestApprovalParams } from "./CommandExecutionRequestApprovalParams"; export type { CommandExecutionRequestApprovalResponse } from "./CommandExecutionRequestApprovalResponse"; -export type { CommandExecutionRequestApprovalSkillMetadata } from "./CommandExecutionRequestApprovalSkillMetadata"; export type { CommandExecutionSource } from "./CommandExecutionSource"; export type { CommandExecutionStatus } from "./CommandExecutionStatus"; export type { Config } from "./Config"; diff --git a/codex-rs/app-server-protocol/src/export.rs b/codex-rs/app-server-protocol/src/export.rs index b89f23c666..033f605973 100644 --- a/codex-rs/app-server-protocol/src/export.rs +++ b/codex-rs/app-server-protocol/src/export.rs @@ -2298,10 +2298,6 @@ mod tests { command_execution_request_approval_ts.contains("additionalPermissions"), true ); - assert_eq!( - command_execution_request_approval_ts.contains("skillMetadata"), - true - ); Ok(()) } @@ -2705,10 +2701,6 @@ export type Config = { stableField: Keep, unstableField: string | null } & ({ [k command_execution_request_approval_json.contains("additionalPermissions"), false ); - assert_eq!( - command_execution_request_approval_json.contains("skillMetadata"), - false - ); let client_request_json = fs::read_to_string(output_dir.join("ClientRequest.json"))?; assert_eq!( @@ -2721,7 +2713,6 @@ export type Config = { stableField: Keep, unstableField: string | null } & ({ [k fs::read_to_string(output_dir.join("codex_app_server_protocol.schemas.json"))?; assert_eq!(bundle_json.contains("mockExperimentalField"), false); assert_eq!(bundle_json.contains("additionalPermissions"), false); - assert_eq!(bundle_json.contains("skillMetadata"), false); assert_eq!(bundle_json.contains("MockExperimentalMethodParams"), false); assert_eq!( bundle_json.contains("MockExperimentalMethodResponse"), @@ -2731,7 +2722,6 @@ export type Config = { stableField: Keep, unstableField: string | null } & ({ [k fs::read_to_string(output_dir.join("codex_app_server_protocol.v2.schemas.json"))?; assert_eq!(flat_v2_bundle_json.contains("mockExperimentalField"), false); assert_eq!(flat_v2_bundle_json.contains("additionalPermissions"), false); - assert_eq!(flat_v2_bundle_json.contains("skillMetadata"), false); assert_eq!( flat_v2_bundle_json.contains("MockExperimentalMethodParams"), false diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 5a9215f2ee..4492a14944 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -1705,7 +1705,6 @@ mod tests { }), macos: None, }), - skill_metadata: None, proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, available_decisions: None, @@ -1716,31 +1715,4 @@ mod tests { Some("item/commandExecution/requestApproval.additionalPermissions") ); } - - #[test] - fn command_execution_request_approval_skill_metadata_is_marked_experimental() { - let params = v2::CommandExecutionRequestApprovalParams { - thread_id: "thr_123".to_string(), - turn_id: "turn_123".to_string(), - item_id: "call_123".to_string(), - approval_id: None, - reason: None, - network_approval_context: None, - command: Some("cat file".to_string()), - cwd: None, - command_actions: None, - additional_permissions: None, - skill_metadata: Some(v2::CommandExecutionRequestApprovalSkillMetadata { - path_to_skills_md: PathBuf::from("/tmp/SKILLS.md"), - }), - proposed_execpolicy_amendment: None, - proposed_network_policy_amendments: None, - available_decisions: None, - }; - let reason = crate::experimental_api::ExperimentalApi::experimental_reason(¶ms); - assert_eq!( - reason, - Some("item/commandExecution/requestApproval.skillMetadata") - ); - } } diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 147c91db2c..559d825aa5 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -7,7 +7,6 @@ use crate::protocol::common::AuthMode; use codex_experimental_api_macros::ExperimentalApi; use codex_protocol::account::PlanType; use codex_protocol::approvals::ElicitationRequest as CoreElicitationRequest; -use codex_protocol::approvals::ExecApprovalRequestSkillMetadata as CoreExecApprovalRequestSkillMetadata; use codex_protocol::approvals::ExecPolicyAmendment as CoreExecPolicyAmendment; use codex_protocol::approvals::NetworkApprovalContext as CoreNetworkApprovalContext; use codex_protocol::approvals::NetworkApprovalProtocol as CoreNetworkApprovalProtocol; @@ -3519,14 +3518,6 @@ impl From for SkillMetadata { } } -impl From for CommandExecutionRequestApprovalSkillMetadata { - fn from(value: CoreExecApprovalRequestSkillMetadata) -> Self { - Self { - path_to_skills_md: value.path_to_skills_md, - } - } -} - impl From for SkillInterface { fn from(value: CoreSkillInterface) -> Self { Self { @@ -5240,11 +5231,6 @@ pub struct CommandExecutionRequestApprovalParams { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] pub additional_permissions: Option, - /// Optional skill metadata when the approval was triggered by a skill script. - #[experimental("item/commandExecution/requestApproval.skillMetadata")] - #[serde(default, skip_serializing_if = "Option::is_none")] - #[ts(optional = nullable)] - pub skill_metadata: Option, /// Optional proposed execpolicy amendment to allow similar commands without prompting. #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] @@ -5266,17 +5252,9 @@ impl CommandExecutionRequestApprovalParams { // We need a generic outbound compatibility design for stripping or // otherwise handling experimental server->client payloads. self.additional_permissions = None; - self.skill_metadata = None; } } -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] -#[serde(rename_all = "camelCase")] -#[ts(export_to = "v2/")] -pub struct CommandExecutionRequestApprovalSkillMetadata { - pub path_to_skills_md: PathBuf, -} - #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -6100,7 +6078,6 @@ mod tests { }, "macos": null }, - "skillMetadata": null, "proposedExecpolicyAmendment": null, "proposedNetworkPolicyAmendments": null, "availableDecisions": null @@ -6139,7 +6116,6 @@ mod tests { "contacts": "read_only" } }, - "skillMetadata": null, "proposedExecpolicyAmendment": null, "proposedNetworkPolicyAmendments": null, "availableDecisions": null @@ -6183,7 +6159,6 @@ mod tests { "contacts": "none" } }, - "skillMetadata": null, "proposedExecpolicyAmendment": null, "proposedNetworkPolicyAmendments": null, "availableDecisions": null @@ -6199,35 +6174,6 @@ mod tests { ); } - #[test] - fn command_execution_request_approval_accepts_skill_metadata() { - let params = serde_json::from_value::(json!({ - "threadId": "thr_123", - "turnId": "turn_123", - "itemId": "call_123", - "command": "cat file", - "cwd": "/tmp", - "commandActions": null, - "reason": null, - "networkApprovalContext": null, - "additionalPermissions": null, - "skillMetadata": { - "pathToSkillsMd": "/tmp/SKILLS.md" - }, - "proposedExecpolicyAmendment": null, - "proposedNetworkPolicyAmendments": null, - "availableDecisions": null - })) - .expect("skill metadata should deserialize"); - - assert_eq!( - params.skill_metadata, - Some(CommandExecutionRequestApprovalSkillMetadata { - path_to_skills_md: PathBuf::from("/tmp/SKILLS.md"), - }) - ); - } - #[test] fn permissions_request_approval_uses_request_permission_profile() { let read_only_path = if cfg!(windows) { diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index a479f96529..82954d3cb6 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -1917,7 +1917,6 @@ impl CodexClient { cwd, command_actions, additional_permissions, - skill_metadata, proposed_execpolicy_amendment, proposed_network_policy_amendments, available_decisions, @@ -1952,9 +1951,6 @@ impl CodexClient { if let Some(additional_permissions) = additional_permissions.as_ref() { println!("< additional permissions: {additional_permissions:?}"); } - if let Some(skill_metadata) = skill_metadata.as_ref() { - println!("< skill metadata: {skill_metadata:?}"); - } if let Some(execpolicy_amendment) = proposed_execpolicy_amendment.as_ref() { println!("< proposed execpolicy amendment: {execpolicy_amendment:?}"); } diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 15484d3d5f..9a850c0b1e 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -26,7 +26,6 @@ use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionOutputDeltaNotification; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; -use codex_app_server_protocol::CommandExecutionRequestApprovalSkillMetadata; use codex_app_server_protocol::CommandExecutionSource; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::ContextCompactedNotification; @@ -608,7 +607,6 @@ pub(crate) async fn apply_bespoke_event_handling( proposed_execpolicy_amendment, proposed_network_policy_amendments, additional_permissions, - skill_metadata, parsed_cmd, .. } = ev; @@ -680,8 +678,6 @@ pub(crate) async fn apply_bespoke_event_handling( }); let additional_permissions = additional_permissions.map(V2AdditionalPermissionProfile::from); - let skill_metadata = - skill_metadata.map(CommandExecutionRequestApprovalSkillMetadata::from); let params = CommandExecutionRequestApprovalParams { thread_id: conversation_id.to_string(), @@ -694,7 +690,6 @@ pub(crate) async fn apply_bespoke_event_handling( cwd, command_actions, additional_permissions, - skill_metadata, proposed_execpolicy_amendment: proposed_execpolicy_amendment_v2, proposed_network_policy_amendments: proposed_network_policy_amendments_v2, available_decisions: Some(available_decisions), diff --git a/codex-rs/app-server/src/transport/mod.rs b/codex-rs/app-server/src/transport/mod.rs index 2f80a009d3..9e02e77239 100644 --- a/codex-rs/app-server/src/transport/mod.rs +++ b/codex-rs/app-server/src/transport/mod.rs @@ -379,7 +379,6 @@ pub(crate) async fn route_outgoing_envelope( mod tests { use super::*; use crate::error_code::OVERLOADED_ERROR_CODE; - use codex_app_server_protocol::CommandExecutionRequestApprovalSkillMetadata; use codex_app_server_protocol::ConfigWarningNotification; use codex_app_server_protocol::ServerNotification; use codex_utils_absolute_path::AbsolutePathBuf; @@ -761,7 +760,7 @@ mod tests { } #[tokio::test] - async fn command_execution_request_approval_strips_experimental_fields_without_capability() { + async fn command_execution_request_approval_strips_additional_permissions_without_capability() { let connection_id = ConnectionId(8); let (writer_tx, mut writer_rx) = mpsc::channel(1); @@ -805,9 +804,6 @@ mod tests { macos: None, }, ), - skill_metadata: Some(CommandExecutionRequestApprovalSkillMetadata { - path_to_skills_md: PathBuf::from("/tmp/SKILLS.md"), - }), proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, available_decisions: None, @@ -824,11 +820,10 @@ mod tests { .expect("request should be delivered to the connection"); let json = serde_json::to_value(message.message).expect("request should serialize"); assert_eq!(json["params"].get("additionalPermissions"), None); - assert_eq!(json["params"].get("skillMetadata"), None); } #[tokio::test] - async fn command_execution_request_approval_keeps_experimental_fields_with_capability() { + async fn command_execution_request_approval_keeps_additional_permissions_with_capability() { let connection_id = ConnectionId(9); let (writer_tx, mut writer_rx) = mpsc::channel(1); @@ -872,9 +867,6 @@ mod tests { macos: None, }, ), - skill_metadata: Some(CommandExecutionRequestApprovalSkillMetadata { - path_to_skills_md: PathBuf::from("/tmp/SKILLS.md"), - }), proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, available_decisions: None, @@ -897,17 +889,11 @@ mod tests { "network": null, "fileSystem": { "read": [allowed_path], - "write": null, + "write": null, }, "macos": null, }) ); - assert_eq!( - json["params"]["skillMetadata"], - json!({ - "pathToSkillsMd": "/tmp/SKILLS.md", - }) - ); } #[tokio::test] diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3a27a8839b..ed24d28466 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -75,7 +75,6 @@ use codex_otel::current_span_w3c_trace_context; use codex_otel::set_parent_from_w3c_trace_context; use codex_protocol::ThreadId; use codex_protocol::approvals::ElicitationRequestEvent; -use codex_protocol::approvals::ExecApprovalRequestSkillMetadata; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::approvals::NetworkPolicyAmendment; use codex_protocol::approvals::NetworkPolicyRuleAction; @@ -2897,7 +2896,6 @@ impl Session { network_approval_context: Option, proposed_execpolicy_amendment: Option, additional_permissions: Option, - skill_metadata: Option, available_decisions: Option>, ) -> ReviewDecision { // command-level approvals use `call_id`. @@ -2951,7 +2949,6 @@ impl Session { proposed_execpolicy_amendment, proposed_network_policy_amendments, additional_permissions, - skill_metadata, available_decisions: Some(available_decisions), parsed_cmd, }); diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 9f491bbcb8..3f1508cca1 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -436,7 +436,6 @@ async fn handle_exec_approval( network_approval_context, proposed_execpolicy_amendment, additional_permissions, - skill_metadata, available_decisions, .. } = event; @@ -480,7 +479,6 @@ async fn handle_exec_approval( network_approval_context, proposed_execpolicy_amendment, additional_permissions, - skill_metadata, available_decisions, ), parent_session, diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 8201424d8e..29efd92d71 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -286,7 +286,6 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: Some(vec![ ReviewDecision::Approved, ReviewDecision::Abort, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 02b6bbe601..d3c526e99e 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -384,7 +384,6 @@ impl NetworkApprovalService { Some(network_approval_context.clone()), /*proposed_execpolicy_amendment*/ None, /*additional_permissions*/ None, - /*skill_metadata*/ None, available_decisions, ) .await diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index ecb5822124..3916a86301 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -184,7 +184,6 @@ impl Approvable for ShellRuntime { .proposed_execpolicy_amendment() .cloned(), req.additional_permissions.clone(), - /*skill_metadata*/ None, available_decisions, ) .await diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index baef00d339..9146743007 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -426,7 +426,6 @@ impl CoreShellActionProvider { /*network_approval_context*/ None, /*proposed_execpolicy_amendment*/ None, additional_permissions, - /*skill_metadata*/ None, Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), ) .await diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 96a3e09fb8..9211fb08a7 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -161,7 +161,6 @@ impl Approvable for UnifiedExecRuntime<'_> { .proposed_execpolicy_amendment() .cloned(), req.additional_permissions.clone(), - /*skill_metadata*/ None, available_decisions, ) .await diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 780a808038..ae9d26e99e 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -228,7 +228,6 @@ async fn run_codex_tool_session_inner( parsed_cmd, network_approval_context: _, additional_permissions: _, - skill_metadata: _, available_decisions: _, } = ev; handle_exec_approval_request( diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 848ea31c02..95ccdd624c 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -1,6 +1,3 @@ -use std::collections::HashMap; -use std::path::PathBuf; - use crate::mcp::RequestId; use crate::models::MacOsSeatbeltProfileExtensions; use crate::models::PermissionProfile; @@ -14,6 +11,8 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; use serde_json::Value as JsonValue; +use std::collections::HashMap; +use std::path::PathBuf; use ts_rs::TS; #[derive(Debug, Clone, PartialEq, Eq)] @@ -107,13 +106,6 @@ pub struct NetworkPolicyAmendment { pub action: NetworkPolicyRuleAction, } -#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] -#[serde(rename_all = "snake_case")] -#[ts(rename_all = "snake_case")] -pub struct ExecApprovalRequestSkillMetadata { - pub path_to_skills_md: PathBuf, -} - #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)] pub struct GuardianAssessmentEvent { /// Stable identifier for this guardian review lifecycle. @@ -181,10 +173,6 @@ pub struct ExecApprovalRequestEvent { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] pub additional_permissions: Option, - /// Optional skill metadata when the approval was triggered by a skill script. - #[serde(default, skip_serializing_if = "Option::is_none")] - #[ts(optional)] - pub skill_metadata: Option, /// Ordered list of decisions the client may present for this prompt. /// /// When absent, clients should derive the legacy default set from the diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 71c9cbda64..9a4f2b90b2 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -64,7 +64,6 @@ use ts_rs::TS; pub use crate::approvals::ApplyPatchApprovalRequestEvent; pub use crate::approvals::ElicitationAction; pub use crate::approvals::ExecApprovalRequestEvent; -pub use crate::approvals::ExecApprovalRequestSkillMetadata; pub use crate::approvals::ExecPolicyAmendment; pub use crate::approvals::GuardianAssessmentEvent; pub use crate::approvals::GuardianAssessmentStatus; diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index fe1cb6ae0c..ff5d6d47dd 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -4979,7 +4979,6 @@ mod tests { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: Vec::new(), }, @@ -6626,7 +6625,6 @@ guardian_approval = true proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: Vec::new(), }, @@ -6715,7 +6713,6 @@ guardian_approval = true proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: Vec::new(), }, diff --git a/codex-rs/tui/src/app/pending_interactive_replay.rs b/codex-rs/tui/src/app/pending_interactive_replay.rs index 02efb2ec1b..dba5f2f018 100644 --- a/codex-rs/tui/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui/src/app/pending_interactive_replay.rs @@ -453,7 +453,6 @@ mod tests { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: Vec::new(), }, @@ -597,7 +596,6 @@ mod tests { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: Vec::new(), }, @@ -687,7 +685,6 @@ mod tests { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: Vec::new(), }, diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 90fea87e7f..8e3485daa7 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -3456,7 +3456,6 @@ async fn exec_approval_emits_proposed_command_and_decision_history() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -3507,7 +3506,6 @@ async fn exec_approval_uses_approval_id_when_present() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }), @@ -3549,7 +3547,6 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -3605,7 +3602,6 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -9966,7 +9962,6 @@ async fn approval_modal_exec_snapshot() -> anyhow::Result<()> { ])), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -10028,7 +10023,6 @@ async fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> { ])), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -10077,7 +10071,6 @@ async fn approval_modal_exec_multiline_prefix_hides_execpolicy_option_snapshot() proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -10445,7 +10438,6 @@ async fn status_widget_and_approval_modal_snapshot() { ])), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; diff --git a/codex-rs/tui_app_server/src/app.rs b/codex-rs/tui_app_server/src/app.rs index 95e231b970..a86a8c9388 100644 --- a/codex-rs/tui_app_server/src/app.rs +++ b/codex-rs/tui_app_server/src/app.rs @@ -8371,7 +8371,6 @@ guardian_approval = true cwd: Some(PathBuf::from("/tmp/project")), command_actions: None, additional_permissions: None, - skill_metadata: None, proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, available_decisions: None, diff --git a/codex-rs/tui_app_server/src/app/app_server_requests.rs b/codex-rs/tui_app_server/src/app/app_server_requests.rs index 4381e883c0..29f0032b65 100644 --- a/codex-rs/tui_app_server/src/app/app_server_requests.rs +++ b/codex-rs/tui_app_server/src/app/app_server_requests.rs @@ -318,7 +318,6 @@ mod tests { cwd: None, command_actions: None, additional_permissions: None, - skill_metadata: None, proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, available_decisions: None, diff --git a/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs b/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs index 67c88d5f90..0110c8795b 100644 --- a/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs @@ -627,7 +627,6 @@ mod tests { cwd: Some(PathBuf::from("/tmp")), command_actions: None, additional_permissions: None, - skill_metadata: None, proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, available_decisions: None, diff --git a/codex-rs/tui_app_server/src/chatwidget.rs b/codex-rs/tui_app_server/src/chatwidget.rs index 636349aae5..5799cc4dd3 100644 --- a/codex-rs/tui_app_server/src/chatwidget.rs +++ b/codex-rs/tui_app_server/src/chatwidget.rs @@ -1317,11 +1317,6 @@ fn exec_approval_request_from_params( .collect() }, ), - skill_metadata: params.skill_metadata.map(|metadata| { - codex_protocol::approvals::ExecApprovalRequestSkillMetadata { - path_to_skills_md: metadata.path_to_skills_md, - } - }), available_decisions: params.available_decisions.map(|decisions| { decisions .into_iter() diff --git a/codex-rs/tui_app_server/src/chatwidget/tests.rs b/codex-rs/tui_app_server/src/chatwidget/tests.rs index 7dadb963d8..58ff60eb03 100644 --- a/codex-rs/tui_app_server/src/chatwidget/tests.rs +++ b/codex-rs/tui_app_server/src/chatwidget/tests.rs @@ -3479,7 +3479,6 @@ async fn exec_approval_emits_proposed_command_and_decision_history() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -3529,7 +3528,6 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() { cwd: Some(PathBuf::from("/tmp")), command_actions: None, additional_permissions: None, - skill_metadata: None, proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, available_decisions: None, @@ -3564,7 +3562,6 @@ async fn exec_approval_uses_approval_id_when_present() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }), @@ -3606,7 +3603,6 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -3662,7 +3658,6 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { proposed_execpolicy_amendment: None, proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -10680,7 +10675,6 @@ async fn approval_modal_exec_snapshot() -> anyhow::Result<()> { ])), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -10742,7 +10736,6 @@ async fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> { ])), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -10791,7 +10784,6 @@ async fn approval_modal_exec_multiline_prefix_hides_execpolicy_option_snapshot() proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], }; @@ -11175,7 +11167,6 @@ async fn status_widget_and_approval_modal_snapshot() { ])), proposed_network_policy_amendments: None, additional_permissions: None, - skill_metadata: None, available_decisions: None, parsed_cmd: vec![], };