diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 2a36d66c1d..9270af85e7 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -68,7 +68,7 @@ "properties": { "extends": { "default": null, - "description": "Parent profile identifier once permissions profiles support inheritance. This is currently always `null`.", + "description": "Parent profile identifier from the selected permissions profile's `extends` setting, when present.", "type": [ "string", "null" 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 ee48d1e259..b0158d0a19 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 @@ -5666,7 +5666,7 @@ "properties": { "extends": { "default": null, - "description": "Parent profile identifier once permissions profiles support inheritance. This is currently always `null`.", + "description": "Parent profile identifier from the selected permissions profile's `extends` setting, when present.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 871cd81777..7e0053dcf2 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -134,7 +134,7 @@ "properties": { "extends": { "default": null, - "description": "Parent profile identifier once permissions profiles support inheritance. This is currently always `null`.", + "description": "Parent profile identifier from the selected permissions profile's `extends` setting, when present.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json index 3d66152a60..cccc7e5b8d 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json @@ -9,7 +9,7 @@ "properties": { "extends": { "default": null, - "description": "Parent profile identifier once permissions profiles support inheritance. This is currently always `null`.", + "description": "Parent profile identifier from the selected permissions profile's `extends` setting, when present.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json index f379455b59..f57257f94c 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json @@ -9,7 +9,7 @@ "properties": { "extends": { "default": null, - "description": "Parent profile identifier once permissions profiles support inheritance. This is currently always `null`.", + "description": "Parent profile identifier from the selected permissions profile's `extends` setting, when present.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadSettingsUpdatedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadSettingsUpdatedNotification.json index d016e28311..42a76ae364 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadSettingsUpdatedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadSettingsUpdatedNotification.json @@ -9,7 +9,7 @@ "properties": { "extends": { "default": null, - "description": "Parent profile identifier once permissions profiles support inheritance. This is currently always `null`.", + "description": "Parent profile identifier from the selected permissions profile's `extends` setting, when present.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json index 7d31c52f1c..02d156d7a6 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json @@ -9,7 +9,7 @@ "properties": { "extends": { "default": null, - "description": "Parent profile identifier once permissions profiles support inheritance. This is currently always `null`.", + "description": "Parent profile identifier from the selected permissions profile's `extends` setting, when present.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ActivePermissionProfile.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ActivePermissionProfile.ts index 73f9efcab5..ee9026b543 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ActivePermissionProfile.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ActivePermissionProfile.ts @@ -9,7 +9,7 @@ export type ActivePermissionProfile = { */ id: string, /** - * Parent profile identifier once permissions profiles support - * inheritance. This is currently always `null`. + * Parent profile identifier from the selected permissions profile's + * `extends` setting, when present. */ extends: string | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs b/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs index 99924ca1e5..dfc1942276 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs @@ -329,8 +329,8 @@ pub struct ActivePermissionProfile { /// Identifier from `default_permissions` or the implicit built-in default, /// such as `:workspace` or a user-defined `[permissions.]` profile. pub id: String, - /// Parent profile identifier once permissions profiles support - /// inheritance. This is currently always `null`. + /// Parent profile identifier from the selected permissions profile's + /// `extends` setting, when present. #[serde(default)] pub extends: Option, } diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index f4127c437c..95873191b9 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -478,6 +478,10 @@ struct AppServerCommand { #[derive(Debug, Parser)] struct ExecServerCommand { + /// Error out when config.toml contains fields that are not recognized by this version of Codex. + #[arg(long = "strict-config", default_value_t = false)] + strict_config: bool, + /// Transport endpoint URL. Supported values: `ws://IP:PORT` (default), `stdio`, `stdio://`. #[arg(long = "listen", value_name = "URL", conflicts_with = "remote")] listen: Option, @@ -1376,11 +1380,13 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { root_remote_auth_token_env.as_deref(), "exec-server", )?; + let strict_config = cmd.strict_config || root_strict_config; run_exec_server_command( cmd, &arg0_paths, &root_config_overrides, interactive.config_profile.clone(), + strict_config, ) .await?; } @@ -1481,6 +1487,7 @@ async fn run_exec_server_command( arg0_paths: &Arg0DispatchPaths, root_config_overrides: &CliConfigOverrides, config_profile: Option, + strict_config: bool, ) -> anyhow::Result<()> { let codex_self_exe = arg0_paths .codex_self_exe @@ -1494,12 +1501,10 @@ async fn run_exec_server_command( let environment_id = cmd .environment_id .ok_or_else(|| anyhow::anyhow!("--environment-id is required when --remote is set"))?; - let auth_provider = load_exec_server_remote_auth_provider( - root_config_overrides, - config_profile, - cmd.use_agent_identity_auth, - ) - .await?; + let config = + load_exec_server_config(root_config_overrides, config_profile, strict_config).await?; + let auth_provider = + load_exec_server_remote_auth_provider(&config, cmd.use_agent_identity_auth).await?; let mut remote_config = codex_exec_server::RemoteEnvironmentConfig::new( base_url, environment_id, @@ -1509,23 +1514,29 @@ async fn run_exec_server_command( remote_config.name = name; } codex_exec_server::run_remote_environment(remote_config, runtime_paths).await?; - return Ok(()); + Ok(()) + } else { + if strict_config { + // Local exec-server startup does not consume Config, but strict + // mode should still reject unknown fields before opening a listener. + let _validated_config = + load_exec_server_config(root_config_overrides, config_profile, strict_config) + .await?; + } + let listen_url = cmd + .listen + .as_deref() + .unwrap_or(codex_exec_server::DEFAULT_LISTEN_URL); + codex_exec_server::run_main(listen_url, runtime_paths) + .await + .map_err(anyhow::Error::from_boxed) } - let listen_url = cmd - .listen - .as_deref() - .unwrap_or(codex_exec_server::DEFAULT_LISTEN_URL); - codex_exec_server::run_main(listen_url, runtime_paths) - .await - .map_err(anyhow::Error::from_boxed) } async fn load_exec_server_remote_auth_provider( - root_config_overrides: &CliConfigOverrides, - config_profile: Option, + config: &codex_core::config::Config, use_agent_identity_auth: bool, ) -> anyhow::Result { - let config = load_exec_server_remote_config(root_config_overrides, config_profile).await?; if use_agent_identity_auth { let agent_identity_jwt = read_codex_access_token_from_env().ok_or_else(|| { anyhow::anyhow!("CODEX_ACCESS_TOKEN is required when --use-agent-identity-auth is set") @@ -1537,7 +1548,7 @@ async fn load_exec_server_remote_auth_provider( } let auth = load_exec_server_remote_auth( - &config, + config, "remote exec-server registration requires ChatGPT authentication; run `codex login` first", ) .await?; @@ -1551,9 +1562,10 @@ async fn load_exec_server_remote_auth_provider( Ok(codex_model_provider::auth_provider_from_auth(&auth)) } -async fn load_exec_server_remote_config( +async fn load_exec_server_config( root_config_overrides: &CliConfigOverrides, config_profile: Option, + strict_config: bool, ) -> anyhow::Result { let cli_kv_overrides = root_config_overrides .parse_overrides() @@ -1564,6 +1576,7 @@ async fn load_exec_server_remote_config( config_profile, ..Default::default() }) + .strict_config(strict_config) .build() .await?) } @@ -1869,6 +1882,7 @@ fn unsupported_subcommand_name_for_strict_config( | Some(Subcommand::Exec(_)) | Some(Subcommand::Review(_)) | Some(Subcommand::McpServer(_)) + | Some(Subcommand::ExecServer(_)) | Some(Subcommand::Resume(_)) | Some(Subcommand::Fork(_)) | Some(Subcommand::Doctor(_)) => None, @@ -1892,7 +1906,6 @@ fn unsupported_subcommand_name_for_strict_config( Some(Subcommand::Apply(_)) => Some("apply"), Some(Subcommand::ResponsesApiProxy(_)) => Some("responses-api-proxy"), Some(Subcommand::StdioToUds(_)) => Some("stdio-to-uds"), - Some(Subcommand::ExecServer(_)) => Some("exec-server"), Some(Subcommand::Features(_)) => Some("features"), } } @@ -2897,6 +2910,25 @@ mod tests { .. })) ); + + let cli = MultitoolCli::try_parse_from(["codex", "exec-server", "--strict-config"]) + .expect("parse"); + assert_matches!( + cli.subcommand, + Some(Subcommand::ExecServer(ExecServerCommand { + strict_config: true, + .. + })) + ); + } + + #[test] + fn root_strict_config_is_supported_for_exec_server() { + let cli = MultitoolCli::try_parse_from(["codex", "--strict-config", "exec-server"]) + .expect("parse"); + + reject_root_strict_config_for_subcommand(cli.interactive.strict_config, &cli.subcommand) + .expect("exec-server should support root --strict-config"); } #[test] diff --git a/codex-rs/cli/tests/exec_server.rs b/codex-rs/cli/tests/exec_server.rs new file mode 100644 index 0000000000..0afe0d37b5 --- /dev/null +++ b/codex-rs/cli/tests/exec_server.rs @@ -0,0 +1,35 @@ +use std::path::Path; + +use anyhow::Result; +use predicates::str::contains; +use tempfile::TempDir; + +fn codex_command(codex_home: &Path) -> Result { + let mut cmd = assert_cmd::Command::new(codex_utils_cargo_bin::cargo_bin("codex")?); + cmd.env("CODEX_HOME", codex_home); + Ok(cmd) +} + +#[test] +fn strict_config_rejects_unknown_config_fields_for_exec_server() -> Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join("config.toml"), + r#" +foo = "bar" +"#, + )?; + + let mut cmd = codex_command(codex_home.path())?; + cmd.args([ + "exec-server", + "--strict-config", + "--listen", + "http://127.0.0.1:0", + ]) + .assert() + .failure() + .stderr(contains("unknown configuration field")); + + Ok(()) +} diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 020adb0313..94d6701546 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -1,5 +1,6 @@ use crate::key_aliases::normalize_key_aliases; use crate::key_aliases::normalized_with_key_aliases; +use codex_network_proxy::normalize_host; use toml::Value as TomlValue; /// Merge config `overlay` into `base`, giving `overlay` precedence. @@ -14,6 +15,10 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m normalize_key_aliases(path, base_table); let mut overlay_table = overlay_table.clone(); normalize_key_aliases(path, &mut overlay_table); + if is_permission_network_domains_path(path) { + normalize_network_domain_keys(base_table); + normalize_network_domain_keys(&mut overlay_table); + } for (key, value) in overlay_table { path.push(key.clone()); @@ -29,6 +34,21 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } +fn is_permission_network_domains_path(path: &[String]) -> bool { + matches!( + path, + [permissions, _, network, domains] + if permissions == "permissions" && network == "network" && domains == "domains" + ) +} + +fn normalize_network_domain_keys(table: &mut toml::map::Map) { + let entries = std::mem::take(table); + for (pattern, value) in entries { + table.insert(normalize_host(&pattern), value); + } +} + #[cfg(test)] #[path = "merge_tests.rs"] mod tests; diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index 0bb92c0015..f9da6e7ebc 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -98,3 +98,29 @@ disable_on_external_context = true ); assert_eq!(base, expected); } + +#[test] +fn merge_toml_values_normalizes_permission_network_domains_before_overlaying() { + let mut base = parse_toml( + r#" +[permissions.dev.network.domains] +"example.com" = "deny" +"#, + ); + let overlay = parse_toml( + r#" +[permissions.dev.network.domains] +"EXAMPLE.COM" = "allow" +"#, + ); + + merge_toml_values(&mut base, &overlay); + + let expected = parse_toml( + r#" +[permissions.dev.network.domains] +"example.com" = "allow" +"#, + ); + assert_eq!(base, expected); +} diff --git a/codex-rs/config/src/permissions_toml.rs b/codex-rs/config/src/permissions_toml.rs index 4d533776ad..60ec9e128d 100644 --- a/codex-rs/config/src/permissions_toml.rs +++ b/codex-rs/config/src/permissions_toml.rs @@ -1,5 +1,6 @@ use std::collections::BTreeMap; +use crate::merge::merge_toml_values; use codex_network_proxy::NetworkDomainPermission as ProxyNetworkDomainPermission; use codex_network_proxy::NetworkMode; use codex_network_proxy::NetworkProxyConfig; @@ -9,6 +10,8 @@ use codex_protocol::permissions::FileSystemAccessMode; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; +use thiserror::Error; +use toml::Value as TomlValue; #[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)] pub struct PermissionsToml { @@ -20,17 +23,190 @@ impl PermissionsToml { pub fn is_empty(&self) -> bool { self.entries.is_empty() } + + pub fn resolve_profile( + &self, + profile_name: &str, + mut parent_profile: F, + ) -> Result + where + F: FnMut(&str) -> Option, + { + let mut profile_names = Vec::new(); + let mut profiles = Vec::new(); + let mut next_profile_name = profile_name.to_string(); + let mut referenced_by: Option = None; + + loop { + if let Some(cycle_start) = profile_names + .iter() + .position(|name| name == &next_profile_name) + { + let cycle = profile_names[cycle_start..] + .iter() + .cloned() + .chain(std::iter::once(next_profile_name)) + .collect::>(); + return Err(PermissionProfileResolutionError::Cycle { cycle }); + } + + let profile = self + .entries + .get(&next_profile_name) + .cloned() + .or_else(|| parent_profile(&next_profile_name)) + .ok_or_else(|| { + referenced_by.as_deref().map_or_else( + || PermissionProfileResolutionError::UndefinedProfile { + profile_name: next_profile_name.clone(), + }, + |referenced_by| { + if next_profile_name.starts_with(':') { + PermissionProfileResolutionError::UnsupportedBuiltInParent { + profile_name: referenced_by.to_string(), + parent_profile_name: next_profile_name.clone(), + } + } else { + PermissionProfileResolutionError::UndefinedParent { + profile_name: referenced_by.to_string(), + parent_profile_name: next_profile_name.clone(), + } + } + }, + ) + })?; + let parent_profile_name = profile.extends.clone(); + + profile_names.push(next_profile_name.clone()); + + if let Some(parent_profile_name) = parent_profile_name { + profiles.push(profile); + referenced_by = Some(next_profile_name); + next_profile_name = parent_profile_name; + continue; + } + + let profile = profiles + .into_iter() + .rev() + .try_fold(profile, merge_permission_profiles)?; + return Ok(ResolvedPermissionProfileToml { + profile, + inherited_profile_names: profile_names.into_iter().skip(1).collect(), + }); + } + } } #[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct PermissionProfileToml { pub description: Option, + pub extends: Option, pub workspace_roots: Option, pub filesystem: Option, pub network: Option, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedPermissionProfileToml { + pub profile: PermissionProfileToml, + /// Names of profiles inherited while resolving `profile`, ordered from the + /// selected profile's direct parent to the farthest ancestor. + /// + /// Callers use this to preserve which built-in baseline contributed the + /// resolved permissions after the parent profiles have been merged away. + pub inherited_profile_names: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq, Error)] +pub enum PermissionProfileResolutionError { + #[error("default_permissions refers to undefined profile `{profile_name}`")] + UndefinedProfile { profile_name: String }, + #[error( + "permissions profile `{profile_name}` extends undefined profile `{parent_profile_name}`" + )] + UndefinedParent { + profile_name: String, + parent_profile_name: String, + }, + #[error( + "permissions profile `{profile_name}` cannot extend unsupported built-in profile `{parent_profile_name}`" + )] + UnsupportedBuiltInParent { + profile_name: String, + parent_profile_name: String, + }, + #[error( + "permissions profile inheritance cycle detected: {}", + cycle.join(" -> ") + )] + Cycle { cycle: Vec }, + #[error("failed to serialize permissions profile while resolving inheritance: {source}")] + SerializeProfileToml { + #[source] + source: toml::ser::Error, + }, + #[error( + "failed to deserialize merged permissions profile while resolving inheritance: {source}" + )] + DeserializeProfileToml { + #[source] + source: toml::de::Error, + }, +} + +fn merge_permission_profiles( + mut parent: PermissionProfileToml, + mut child: PermissionProfileToml, +) -> Result { + let merges_network_domains = parent + .network + .as_ref() + .and_then(|network| network.domains.as_ref()) + .is_some() + && child + .network + .as_ref() + .and_then(|network| network.domains.as_ref()) + .is_some(); + + // Description and inheritance metadata belong to the selected profile + // declaration, so an inherited profile must not fill those gaps. + parent.description = None; + parent.extends = None; + + if merges_network_domains { + normalize_profile_network_domains(&mut parent); + normalize_profile_network_domains(&mut child); + } + + let mut merged = TomlValue::try_from(parent) + .map_err(|source| PermissionProfileResolutionError::SerializeProfileToml { source })?; + let child = TomlValue::try_from(child) + .map_err(|source| PermissionProfileResolutionError::SerializeProfileToml { source })?; + merge_toml_values(&mut merged, &child); + merged + .try_into() + .map_err(|source| PermissionProfileResolutionError::DeserializeProfileToml { source }) +} + +fn normalize_profile_network_domains(profile: &mut PermissionProfileToml) { + let Some(domains) = profile + .network + .as_mut() + .and_then(|network| network.domains.as_mut()) + else { + return; + }; + + let entries = std::mem::take(&mut domains.entries); + domains.entries = entries + .into_iter() + .map(|(pattern, permission)| (normalize_host(&pattern), permission)) + .collect(); +} + #[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)] pub struct WorkspaceRootsToml { #[serde(flatten)] diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 0e811e9017..636b1fcf3e 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2005,6 +2005,9 @@ "description": { "type": "string" }, + "extends": { + "type": "string" + }, "filesystem": { "$ref": "#/definitions/FilesystemPermissionsToml" }, @@ -4873,4 +4876,4 @@ }, "title": "ConfigToml", "type": "object" -} \ No newline at end of file +} diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 1eb55c4562..19fd762da9 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -733,44 +733,45 @@ async fn runtime_config_uses_tui_raw_output_mode() { #[test] fn config_toml_deserializes_permission_profiles() { let toml = r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace] +[permissions.dev] description = "Day-to-day workspace access." -[permissions.workspace.workspace_roots] +[permissions.dev.workspace_roots] "~/code/openai" = true "~/code/ignored" = false -[permissions.workspace.filesystem] +[permissions.dev.filesystem] ":minimal" = "read" "/tmp/secret.env" = "deny" -[permissions.workspace.filesystem.":workspace_roots"] +[permissions.dev.filesystem.":workspace_roots"] "." = "write" "docs" = "read" -[permissions.workspace.network] +[permissions.dev.network] enabled = true proxy_url = "http://127.0.0.1:43128" enable_socks5 = false allow_upstream_proxy = false mode = "full" -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "openai.com" = "allow" "#; let cfg: ConfigToml = toml::from_str(toml).expect("TOML deserialization should succeed for permissions profiles"); - assert_eq!(cfg.default_permissions.as_deref(), Some("workspace")); + assert_eq!(cfg.default_permissions.as_deref(), Some("dev")); assert_eq!( cfg.permissions.expect("[permissions] should deserialize"), PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: Some("Day-to-day workspace access.".to_string()), + extends: None, workspace_roots: Some(WorkspaceRootsToml { entries: BTreeMap::from([ ("~/code/ignored".to_string(), false), @@ -831,12 +832,63 @@ async fn permissions_profiles_proxy_policy_does_not_start_managed_network_proxy_ let config = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, + workspace_roots: None, + filesystem: Some(FilesystemPermissionsToml { + glob_scan_max_depth: None, + entries: BTreeMap::from([( + ":minimal".to_string(), + FilesystemPermissionToml::Access(FileSystemAccessMode::Read), + )]), + }), + network: Some(NetworkToml { + enabled: Some(true), + ..Default::default() + }), + }, + )]), + }), + ..Default::default() + }, + ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }, + codex_home.abs(), + ) + .await?; + assert_eq!( + config.permissions.network_sandbox_policy(), + NetworkSandboxPolicy::Enabled + ); + assert!( + config.permissions.network.is_none(), + "bare profile network.enabled should not start the managed network proxy" + ); + Ok(()) +} + +#[tokio::test] +async fn permissions_profiles_proxy_policy_starts_managed_network_proxy() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + std::fs::write(cwd.path().join(".git"), "gitdir: nowhere")?; + + let config = Config::load_from_base_config_with_overrides( + ConfigToml { + default_permissions: Some("dev".to_string()), + permissions: Some(PermissionsToml { + entries: BTreeMap::from([( + "dev".to_string(), + PermissionProfileToml { + description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -986,12 +1038,13 @@ async fn network_proxy_feature_matrix_preserves_sandbox_network_semantics() -> s .then(|| toml::from_str("network_proxy = true").expect("valid features")); let base_config = match case.surface { Surface::PermissionProfile => ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -1138,12 +1191,13 @@ async fn network_proxy_feature_uses_profile_network_proxy_settings() -> std::io: let config = Config::load_from_base_config_with_overrides( ConfigToml { features: Some(toml::from_str("network_proxy = true").expect("valid features")), - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -1242,12 +1296,13 @@ enabled = false ) .expect("valid features"), ), - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -1292,12 +1347,13 @@ async fn permissions_profiles_network_disabled_by_default_does_not_start_proxy() let config = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -1340,12 +1396,13 @@ async fn default_permissions_profile_populates_runtime_sandbox_policy() -> std:: std::fs::write(cwd.path().join(".git"), "gitdir: nowhere")?; let cfg = ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -1436,7 +1493,66 @@ async fn default_permissions_profile_populates_runtime_sandbox_policy() -> std:: .active_permission_profile() .as_ref() .map(|active| active.id.as_str()), - Some("workspace") + Some("dev") + ); + Ok(()) +} + +#[tokio::test] +async fn default_permissions_extended_profile_preserves_parent_metadata() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + std::fs::write(cwd.path().join(".git"), "gitdir: nowhere")?; + + let config = Config::load_from_base_config_with_overrides( + ConfigToml { + default_permissions: Some("dev".to_string()), + permissions: Some(PermissionsToml { + entries: BTreeMap::from([ + ( + "base".to_string(), + PermissionProfileToml { + description: None, + extends: None, + workspace_roots: None, + filesystem: Some(FilesystemPermissionsToml { + glob_scan_max_depth: None, + entries: BTreeMap::from([( + ":minimal".to_string(), + FilesystemPermissionToml::Access(FileSystemAccessMode::Read), + )]), + }), + network: None, + }, + ), + ( + "dev".to_string(), + PermissionProfileToml { + description: None, + extends: Some("base".to_string()), + workspace_roots: None, + filesystem: None, + network: None, + }, + ), + ]), + }), + ..Default::default() + }, + ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }, + codex_home.abs(), + ) + .await?; + + assert_eq!( + config.permissions.active_permission_profile(), + Some(ActivePermissionProfile { + id: "dev".to_string(), + extends: Some("base".to_string()), + }) ); Ok(()) } @@ -1645,12 +1761,13 @@ async fn permission_profile_override_preserves_configured_network_policy_without let config = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -1706,12 +1823,13 @@ async fn workspace_root_glob_none_compiles_to_filesystem_pattern_entry() -> std: let config = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: Some(2), @@ -1789,9 +1907,10 @@ async fn permissions_profiles_require_default_permissions() -> std::io::Result<( ConfigToml { permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -1920,6 +2039,7 @@ async fn workspace_profile_applies_rules_to_runtime_and_profile_workspace_roots( "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: Some(WorkspaceRootsToml { entries: BTreeMap::from([( profile_root.to_string_lossy().into_owned(), @@ -2040,6 +2160,118 @@ async fn explicit_builtin_workspace_profile_ignores_legacy_workspace_write_setti Ok(()) } +#[tokio::test] +async fn default_permissions_profile_can_extend_builtin_workspace() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + + let config = Config::load_from_base_config_with_overrides( + ConfigToml { + default_permissions: Some("workspace-with-network".to_string()), + permissions: Some(PermissionsToml { + entries: BTreeMap::from([( + "workspace-with-network".to_string(), + PermissionProfileToml { + description: None, + extends: Some(BUILT_IN_PERMISSION_PROFILE_WORKSPACE.to_string()), + workspace_roots: None, + filesystem: None, + network: Some(NetworkToml { + enabled: Some(true), + ..Default::default() + }), + }, + )]), + }), + ..Default::default() + }, + ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }, + codex_home.abs(), + ) + .await?; + + let policy = config.permissions.file_system_sandbox_policy(); + assert!( + policy.can_write_path_with_cwd(cwd.path(), cwd.path()), + "expected profile extending :workspace to keep project-root writes, policy: {policy:?}" + ); + assert!( + !policy.can_write_path_with_cwd(&cwd.path().join(".git"), cwd.path()), + "expected profile extending :workspace to keep metadata carveouts, policy: {policy:?}" + ); + assert_eq!( + config.permissions.network_sandbox_policy(), + NetworkSandboxPolicy::Enabled + ); + assert_eq!( + config.permissions.active_permission_profile(), + Some(ActivePermissionProfile { + id: "workspace-with-network".to_string(), + extends: Some(BUILT_IN_PERMISSION_PROFILE_WORKSPACE.to_string()), + }) + ); + Ok(()) +} + +#[tokio::test] +async fn default_permissions_profile_can_extend_builtin_read_only() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + + let config = Config::load_from_base_config_with_overrides( + ConfigToml { + default_permissions: Some("read-only-with-network".to_string()), + permissions: Some(PermissionsToml { + entries: BTreeMap::from([( + "read-only-with-network".to_string(), + PermissionProfileToml { + description: None, + extends: Some(BUILT_IN_PERMISSION_PROFILE_READ_ONLY.to_string()), + workspace_roots: None, + filesystem: None, + network: Some(NetworkToml { + enabled: Some(true), + ..Default::default() + }), + }, + )]), + }), + ..Default::default() + }, + ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }, + codex_home.abs(), + ) + .await?; + + let policy = config.permissions.file_system_sandbox_policy(); + assert!( + policy.can_read_path_with_cwd(cwd.path(), cwd.path()), + "expected profile extending :read-only to keep read access, policy: {policy:?}" + ); + assert!( + !policy.can_write_path_with_cwd(cwd.path(), cwd.path()), + "expected profile extending :read-only to stay non-writable, policy: {policy:?}" + ); + assert_eq!( + config.permissions.network_sandbox_policy(), + NetworkSandboxPolicy::Enabled + ); + assert_eq!( + config.permissions.active_permission_profile(), + Some(ActivePermissionProfile { + id: "read-only-with-network".to_string(), + extends: Some(BUILT_IN_PERMISSION_PROFILE_READ_ONLY.to_string()), + }) + ); + Ok(()) +} + #[tokio::test] async fn empty_config_defaults_to_builtin_profile_for_trusted_project() -> std::io::Result<()> { let codex_home = TempDir::new()?; @@ -2373,12 +2605,13 @@ async fn permissions_profiles_allow_direct_write_roots_outside_workspace_root() let config = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -2403,7 +2636,7 @@ async fn permissions_profiles_allow_direct_write_roots_outside_workspace_root() assert_eq!( config.custom_permission_profile_ids, - vec!["workspace".to_string()] + vec!["dev".to_string()] ); let memories_root = AbsolutePathBuf::from_absolute_path(std::fs::canonicalize( codex_home.path().join("memories"), @@ -2435,12 +2668,13 @@ async fn permissions_profiles_reject_nested_entries_for_non_workspace_roots() -> let err = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -2484,9 +2718,9 @@ async fn load_workspace_permission_profile( Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { - entries: BTreeMap::from([("workspace".to_string(), profile)]), + entries: BTreeMap::from([("dev".to_string(), profile)]), }), ..Default::default() }, @@ -2503,6 +2737,7 @@ async fn load_workspace_permission_profile( async fn permissions_profiles_allow_unknown_special_paths() -> std::io::Result<()> { let config = load_workspace_permission_profile(PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -2548,6 +2783,7 @@ async fn permissions_profiles_allow_unknown_special_paths_with_nested_entries() -> std::io::Result<()> { let config = load_workspace_permission_profile(PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -2586,6 +2822,7 @@ async fn permissions_profiles_allow_unknown_special_paths_with_nested_entries() async fn permissions_profiles_allow_missing_filesystem_with_warning() -> std::io::Result<()> { let config = load_workspace_permission_profile(PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: None, network: None, @@ -2604,7 +2841,7 @@ async fn permissions_profiles_allow_missing_filesystem_with_warning() -> std::io ); assert!( config.startup_warnings.iter().any(|warning| warning.contains( - "Permissions profile `workspace` does not define any recognized filesystem entries for this version of Codex." + "Permissions profile `dev` does not define any recognized filesystem entries for this version of Codex." )), "{:?}", config.startup_warnings @@ -2616,6 +2853,7 @@ async fn permissions_profiles_allow_missing_filesystem_with_warning() -> std::io async fn permissions_profiles_allow_empty_filesystem_with_warning() -> std::io::Result<()> { let config = load_workspace_permission_profile(PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -2631,7 +2869,7 @@ async fn permissions_profiles_allow_empty_filesystem_with_warning() -> std::io:: ); assert!( config.startup_warnings.iter().any(|warning| warning.contains( - "Permissions profile `workspace` does not define any recognized filesystem entries for this version of Codex." + "Permissions profile `dev` does not define any recognized filesystem entries for this version of Codex." )), "{:?}", config.startup_warnings @@ -2647,12 +2885,13 @@ async fn permissions_profiles_reject_workspace_root_parent_traversal() -> std::i let err = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -2695,12 +2934,13 @@ async fn permissions_profiles_allow_network_enablement() -> std::io::Result<()> let config = Config::load_from_base_config_with_overrides( ConfigToml { - default_permissions: Some("workspace".to_string()), + default_permissions: Some("dev".to_string()), permissions: Some(PermissionsToml { entries: BTreeMap::from([( - "workspace".to_string(), + "dev".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 51afdf0122..434969e2c5 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -150,6 +150,8 @@ pub use codex_sandboxing::system_bwrap_warning; pub use managed_features::ManagedFeatures; pub use network_proxy_spec::NetworkProxySpec; pub use network_proxy_spec::StartedNetworkProxy; +pub(crate) use permissions::is_builtin_permission_profile_name; +pub(crate) use permissions::reject_unknown_builtin_permission_profile; pub(crate) use permissions::resolve_permission_profile; pub use resolved_permission_profile::PermissionProfileSnapshot; pub(crate) use resolved_permission_profile::PermissionProfileState; @@ -2807,7 +2809,15 @@ impl Config { // when doing so would lose roots, network, or tmp settings. None } else { - Some(ActivePermissionProfile::new(default_permissions)) + let selected_profile_extends = cfg + .permissions + .as_ref() + .and_then(|permissions| permissions.entries.get(default_permissions)) + .and_then(|profile| profile.extends.clone()); + Some(ActivePermissionProfile { + id: default_permissions.to_string(), + extends: selected_profile_extends, + }) }; ( configured_network_proxy_config, diff --git a/codex-rs/core/src/config/permissions.rs b/codex-rs/core/src/config/permissions.rs index 4235cd0f65..7c757d386a 100644 --- a/codex-rs/core/src/config/permissions.rs +++ b/codex-rs/core/src/config/permissions.rs @@ -13,6 +13,7 @@ use codex_config::permissions_toml::NetworkUnixSocketPermissionToml; use codex_config::permissions_toml::NetworkUnixSocketPermissionsToml; use codex_config::permissions_toml::PermissionProfileToml; use codex_config::permissions_toml::PermissionsToml; +use codex_config::permissions_toml::ResolvedPermissionProfileToml; use codex_config::permissions_toml::WorkspaceRootsToml; use codex_config::types::SandboxWorkspaceWrite; use codex_features::NetworkProxyConfigToml; @@ -189,15 +190,29 @@ pub(crate) fn apply_network_proxy_feature_config( .apply_to_network_proxy_config(config); } -pub(crate) fn resolve_permission_profile<'a>( - permissions: &'a PermissionsToml, +pub(crate) fn resolve_permission_profile( + permissions: &PermissionsToml, profile_name: &str, -) -> io::Result<&'a PermissionProfileToml> { - permissions.entries.get(profile_name).ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidInput, - format!("default_permissions refers to undefined profile `{profile_name}`"), - ) +) -> io::Result { + permissions + .resolve_profile(profile_name, extensible_builtin_parent_profile_marker) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err.to_string())) +} + +/// Built-in parents provide their runtime permissions below. Resolution only +/// needs an empty profile marker so inheritance can terminate while preserving +/// the built-in parent id in `inherited_profile_names`. +fn extensible_builtin_parent_profile_marker(profile_name: &str) -> Option { + matches!( + profile_name, + BUILT_IN_READ_ONLY_PROFILE | BUILT_IN_WORKSPACE_PROFILE + ) + .then_some(PermissionProfileToml { + description: None, + extends: None, + workspace_roots: None, + filesystem: None, + network: None, }) } @@ -216,7 +231,7 @@ pub(crate) fn network_proxy_config_for_profile_selection( "default_permissions requires a `[permissions]` table", ) })?; - let profile = resolve_permission_profile(permissions, profile_name)?; + let profile = resolve_permission_profile(permissions, profile_name)?.profile; Ok(network_proxy_config_from_profile_network( profile.network.as_ref(), )) @@ -228,11 +243,27 @@ pub(crate) fn compile_permission_profile( policy_cwd: &Path, startup_warnings: &mut Vec, ) -> io::Result<(FileSystemSandboxPolicy, NetworkSandboxPolicy)> { - let profile = resolve_permission_profile(permissions, profile_name)?; - - let mut entries = Vec::new(); + let ResolvedPermissionProfileToml { + profile, + inherited_profile_names, + } = resolve_permission_profile(permissions, profile_name)?; + let base_permissions = inherited_profile_names.iter().find_map(|name| { + match name.as_str() { + BUILT_IN_READ_ONLY_PROFILE => Some(PermissionProfile::read_only()), + BUILT_IN_WORKSPACE_PROFILE => Some(PermissionProfile::workspace_write()), + _ => None, + } + .map(|profile| profile.to_runtime_permissions()) + }); + let (mut file_system_sandbox_policy, base_network_sandbox_policy) = base_permissions + .unwrap_or_else(|| { + ( + FileSystemSandboxPolicy::restricted(Vec::new()), + NetworkSandboxPolicy::Restricted, + ) + }); if let Some(filesystem) = profile.filesystem.as_ref() { - if filesystem.is_empty() { + if filesystem.is_empty() && file_system_sandbox_policy.entries.is_empty() { push_warning( startup_warnings, missing_filesystem_entries_warning(profile_name), @@ -257,15 +288,17 @@ pub(crate) fn compile_permission_profile( } } for (path, permission) in &filesystem.entries { - entries.extend(compile_filesystem_permission( - path, - permission, - policy_cwd, - startup_warnings, - )?); + file_system_sandbox_policy + .entries + .extend(compile_filesystem_permission( + path, + permission, + policy_cwd, + startup_warnings, + )?); } } - } else { + } else if file_system_sandbox_policy.entries.is_empty() { push_warning( startup_warnings, missing_filesystem_entries_warning(profile_name), @@ -277,10 +310,11 @@ pub(crate) fn compile_permission_profile( .as_ref() .and_then(|filesystem| filesystem.glob_scan_max_depth), )?; - - let network_sandbox_policy = compile_network_sandbox_policy(profile.network.as_ref()); - let mut file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(entries); - file_system_sandbox_policy.glob_scan_max_depth = glob_scan_max_depth; + if let Some(glob_scan_max_depth) = glob_scan_max_depth { + file_system_sandbox_policy.glob_scan_max_depth = Some(glob_scan_max_depth); + } + let network_sandbox_policy = + compile_network_sandbox_policy(profile.network.as_ref(), base_network_sandbox_policy); Ok((file_system_sandbox_policy, network_sandbox_policy)) } @@ -323,7 +357,7 @@ pub(crate) fn compile_permission_profile_workspace_roots( })?; let profile = resolve_permission_profile(permissions, profile_name)?; Ok(compile_workspace_roots( - profile.workspace_roots.as_ref(), + profile.profile.workspace_roots.as_ref(), policy_cwd, )) } @@ -340,7 +374,7 @@ fn compile_workspace_roots( }) } -fn reject_unknown_builtin_permission_profile(profile_name: &str) -> io::Result<()> { +pub(crate) fn reject_unknown_builtin_permission_profile(profile_name: &str) -> io::Result<()> { if profile_name.starts_with(':') { return Err(io::Error::new( io::ErrorKind::InvalidInput, @@ -382,14 +416,18 @@ pub(crate) fn get_readable_roots_required_for_codex_runtime( readable_roots } -fn compile_network_sandbox_policy(network: Option<&NetworkToml>) -> NetworkSandboxPolicy { +fn compile_network_sandbox_policy( + network: Option<&NetworkToml>, + base_network_sandbox_policy: NetworkSandboxPolicy, +) -> NetworkSandboxPolicy { let Some(network) = network else { - return NetworkSandboxPolicy::Restricted; + return base_network_sandbox_policy; }; match network.enabled { Some(true) => NetworkSandboxPolicy::Enabled, - _ => NetworkSandboxPolicy::Restricted, + Some(false) => NetworkSandboxPolicy::Restricted, + None => base_network_sandbox_policy, } } diff --git a/codex-rs/core/src/config/permissions_tests.rs b/codex-rs/core/src/config/permissions_tests.rs index 5e1fcf3525..09aa7606aa 100644 --- a/codex-rs/core/src/config/permissions_tests.rs +++ b/codex-rs/core/src/config/permissions_tests.rs @@ -68,6 +68,7 @@ async fn restricted_read_implicitly_allows_helper_executables() -> std::io::Resu "workspace".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, @@ -239,6 +240,157 @@ fn network_toml_overlays_unix_socket_permissions_by_path() { ); } +#[test] +fn permissions_profiles_resolve_extends_parent_first_with_child_overrides() { + let permissions = toml::from_str::( + r#" +[base] +description = "Base profile" + +[base.filesystem] +glob_scan_max_depth = 1 +"/tmp/base" = "read" +"/tmp/shared" = "read" + +[base.filesystem.":project_roots"] +"**/*.env" = "deny" +docs = "read" + +[base.network] +enabled = true + +[base.network.domains] +"base.example.com" = "allow" +"SHARED.EXAMPLE.COM." = "deny" + +[base.network.unix_sockets] +"/tmp/base.sock" = "allow" + +[child] +extends = "base" + +[child.filesystem] +glob_scan_max_depth = 3 +"/tmp/shared" = "write" + +[child.filesystem.":project_roots"] +docs = "write" +src = "read" + +[child.network] +enabled = false +allow_local_binding = true + +[child.network.domains] +"child.example.com" = "allow" +"shared.example.com" = "allow" + +[child.network.unix_sockets] +"/tmp/child.sock" = "allow" +"#, + ) + .expect("permissions should deserialize"); + + let resolved = permissions + .resolve_profile("child", |_| None) + .expect("child profile should resolve"); + let expected_profile = toml::from_str::( + r#" +extends = "base" + +[filesystem] +glob_scan_max_depth = 3 +"/tmp/base" = "read" +"/tmp/shared" = "write" + +[filesystem.":project_roots"] +"**/*.env" = "deny" +docs = "write" +src = "read" + +[network] +enabled = false +allow_local_binding = true + +[network.domains] +"base.example.com" = "allow" +"child.example.com" = "allow" +"shared.example.com" = "allow" + +[network.unix_sockets] +"/tmp/base.sock" = "allow" +"/tmp/child.sock" = "allow" +"#, + ) + .expect("expected profile should deserialize"); + + assert_eq!(resolved.profile, expected_profile); + assert_eq!(resolved.inherited_profile_names, vec!["base".to_string()]); +} + +#[test] +fn permissions_profiles_reject_undefined_extends_parent() { + let permissions = toml::from_str::( + r#" +[child] +extends = "base" +"#, + ) + .expect("permissions should deserialize"); + + let err = permissions + .resolve_profile("child", |_| None) + .expect_err("missing parent should be rejected"); + + assert_eq!( + err.to_string(), + "permissions profile `child` extends undefined profile `base`" + ); +} + +#[test] +fn permissions_profiles_reject_unsupported_builtin_extends_parent() { + let permissions = toml::from_str::( + r#" +[child] +extends = ":danger-full-access" +"#, + ) + .expect("permissions should deserialize"); + + let err = permissions + .resolve_profile("child", |_| None) + .expect_err("unsupported built-in parent should be rejected"); + + assert_eq!( + err.to_string(), + "permissions profile `child` cannot extend unsupported built-in profile `:danger-full-access`" + ); +} + +#[test] +fn permissions_profiles_reject_extends_cycles() { + let permissions = toml::from_str::( + r#" +[alpha] +extends = "beta" + +[beta] +extends = "alpha" +"#, + ) + .expect("permissions should deserialize"); + + let err = permissions + .resolve_profile("alpha", |_| None) + .expect_err("cycle should be rejected"); + + assert_eq!( + err.to_string(), + "permissions profile inheritance cycle detected: alpha -> beta -> alpha" + ); +} + #[test] fn profile_network_proxy_config_keeps_proxy_disabled_for_bare_network_access() { let config = network_proxy_config_from_profile_network(Some(&NetworkToml { @@ -287,6 +439,7 @@ fn compile_permission_profile_workspace_roots_resolves_enabled_entries() -> std: "workspace".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: Some(WorkspaceRootsToml { entries: BTreeMap::from([ ("backend".to_string(), true), @@ -397,6 +550,7 @@ fn read_write_trailing_glob_suffix_compiles_as_subpath() -> std::io::Result<()> "workspace".to_string(), PermissionProfileToml { description: None, + extends: None, workspace_roots: None, filesystem: Some(FilesystemPermissionsToml { glob_scan_max_depth: None, diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 72bfc1de1c..3708ea6089 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -99,54 +99,57 @@ pub(crate) async fn run_pending_session_start_hooks( sess: &Arc, turn_context: &Arc, ) -> bool { - let Some(session_start_source) = sess.take_pending_session_start_source().await else { - return false; - }; - - // Pending session-start hooks are reused to dispatch thread-spawn subagent - // starts. Other subagent sessions are internal/system work and do not run - // start hooks. - let target = match &turn_context.session_source { - SessionSource::SubAgent(SubAgentSource::ThreadSpawn { agent_role, .. }) - if matches!( - session_start_source, - codex_hooks::SessionStartSource::Startup - ) => - { - let agent_type = agent_role - .clone() - .unwrap_or_else(|| crate::agent::role::DEFAULT_ROLE_NAME.to_string()); - StartHookTarget::SubagentStart { - turn_id: turn_context.sub_id.clone(), - agent_id: sess.thread_id().to_string(), - agent_type, + while let Some(session_start_source) = sess.take_pending_session_start_source().await { + // Pending session-start hooks are reused to dispatch thread-spawn subagent + // starts. Other subagent sessions are internal/system work and do not run + // start hooks. + let target = match &turn_context.session_source { + SessionSource::SubAgent(SubAgentSource::ThreadSpawn { agent_role, .. }) + if matches!( + session_start_source, + codex_hooks::SessionStartSource::Startup + ) => + { + let agent_type = agent_role + .clone() + .unwrap_or_else(|| crate::agent::role::DEFAULT_ROLE_NAME.to_string()); + StartHookTarget::SubagentStart { + turn_id: turn_context.sub_id.clone(), + agent_id: sess.thread_id().to_string(), + agent_type, + } } + SessionSource::SubAgent(_) => return false, + _ => StartHookTarget::SessionStart { + source: session_start_source, + }, + }; + let request = codex_hooks::SessionStartRequest { + session_id: sess.session_id().into(), + #[allow(deprecated)] + cwd: turn_context.cwd.clone(), + transcript_path: sess.hook_transcript_path().await, + model: turn_context.model_info.slug.clone(), + permission_mode: hook_permission_mode(turn_context), + target, + }; + let hooks = sess.hooks(); + let preview_runs = hooks.preview_session_start(&request); + if run_context_injecting_hook( + sess, + turn_context, + preview_runs, + hooks.run_session_start(request, Some(turn_context.sub_id.clone())), + ) + .await + .record_additional_contexts(sess, turn_context) + .await + { + return true; } - SessionSource::SubAgent(_) => return false, - _ => StartHookTarget::SessionStart { - source: session_start_source, - }, - }; - let request = codex_hooks::SessionStartRequest { - session_id: sess.session_id().into(), - #[allow(deprecated)] - cwd: turn_context.cwd.clone(), - transcript_path: sess.hook_transcript_path().await, - model: turn_context.model_info.slug.clone(), - permission_mode: hook_permission_mode(turn_context), - target, - }; - let hooks = sess.hooks(); - let preview_runs = hooks.preview_session_start(&request); - run_context_injecting_hook( - sess, - turn_context, - preview_runs, - hooks.run_session_start(request, Some(turn_context.sub_id.clone())), - ) - .await - .record_additional_contexts(sess, turn_context) - .await + } + + false } /// Runs matching `PreToolUse` hooks before a tool executes. diff --git a/codex-rs/core/src/network_proxy_loader.rs b/codex-rs/core/src/network_proxy_loader.rs index 1c49be2daf..dc4b5b6b16 100644 --- a/codex-rs/core/src/network_proxy_loader.rs +++ b/codex-rs/core/src/network_proxy_loader.rs @@ -1,4 +1,6 @@ use crate::config::find_codex_home; +use crate::config::is_builtin_permission_profile_name; +use crate::config::reject_unknown_builtin_permission_profile; use crate::config::resolve_permission_profile; use crate::exec_policy::ExecPolicyError; use crate::exec_policy::format_exec_policy_error_with_source; @@ -13,6 +15,7 @@ use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; use codex_config::LoaderOverrides; use codex_config::loader::load_config_layers_state; +use codex_config::merge_toml_values; use codex_config::permissions_toml::NetworkToml; use codex_config::permissions_toml::PermissionsToml; use codex_config::permissions_toml::overlay_network_domain_permissions; @@ -117,6 +120,7 @@ fn network_constraints_from_trusted_layers( layers: &ConfigLayerStack, ) -> Result { let mut constraints = NetworkProxyConstraints::default(); + let mut merged = toml::Value::Table(toml::map::Map::new()); for layer in layers.get_layers( ConfigLayerStackOrdering::LowestPrecedenceFirst, /*include_disabled*/ false, @@ -125,10 +129,12 @@ fn network_constraints_from_trusted_layers( continue; } - let parsed = network_tables_from_toml(&layer.config)?; - if let Some(network) = selected_network_from_tables(parsed)? { - apply_network_constraints(network, &mut constraints); - } + merge_toml_values(&mut merged, &layer.config); + } + + let parsed = network_tables_from_toml(&merged)?; + if let Some(network) = selected_network_from_tables(parsed)? { + apply_network_constraints(network, &mut constraints); } Ok(constraints) } @@ -189,13 +195,17 @@ fn selected_network_from_tables(parsed: NetworkTablesToml) -> Result Result<()> { @@ -210,13 +220,15 @@ fn config_from_layers( exec_policy: &codex_execpolicy::Policy, ) -> Result { let mut config = NetworkProxyConfig::default(); + let mut merged = toml::Value::Table(toml::map::Map::new()); for layer in layers.get_layers( ConfigLayerStackOrdering::LowestPrecedenceFirst, /*include_disabled*/ false, ) { - let parsed = network_tables_from_toml(&layer.config)?; - apply_network_tables(&mut config, parsed)?; + merge_toml_values(&mut merged, &layer.config); } + let parsed = network_tables_from_toml(&merged)?; + apply_network_tables(&mut config, parsed)?; apply_exec_policy_network_rules(&mut config, exec_policy); Ok(config) } diff --git a/codex-rs/core/src/network_proxy_loader_tests.rs b/codex-rs/core/src/network_proxy_loader_tests.rs index b5adb935ec..e2f3e7b98b 100644 --- a/codex-rs/core/src/network_proxy_loader_tests.rs +++ b/codex-rs/core/src/network_proxy_loader_tests.rs @@ -1,19 +1,27 @@ use super::*; +use codex_app_server_protocol::ConfigLayerSource; +use codex_config::ConfigLayerEntry; +use codex_config::ConfigLayerStack; +use codex_config::ConfigRequirements; +use codex_config::ConfigRequirementsToml; +use codex_config::permissions_toml::NetworkDomainPermissionToml; +use codex_config::permissions_toml::NetworkDomainPermissionsToml; use codex_execpolicy::Decision; use codex_execpolicy::NetworkRuleProtocol; use codex_execpolicy::Policy; use pretty_assertions::assert_eq; +use std::collections::BTreeMap; #[test] fn higher_precedence_profile_network_overlays_domain_entries() { let lower_network: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "lower.example.com" = "allow" "blocked.example.com" = "deny" "#, @@ -21,11 +29,11 @@ default_permissions = "workspace" .expect("lower layer should parse"); let higher_network: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "higher.example.com" = "allow" "#, ) @@ -60,11 +68,11 @@ default_permissions = "workspace" fn higher_precedence_profile_network_overrides_matching_domain_entries() { let lower_network: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "shared.example.com" = "deny" "other.example.com" = "allow" "#, @@ -72,11 +80,11 @@ default_permissions = "workspace" .expect("lower layer should parse"); let higher_network: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "shared.example.com" = "allow" "#, ) @@ -151,9 +159,9 @@ fn execpolicy_network_rules_overlay_network_lists() { fn apply_network_constraints_includes_allow_all_unix_sockets_flag() { let config: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] dangerously_allow_all_unix_sockets = true "#, ) @@ -170,15 +178,345 @@ dangerously_allow_all_unix_sockets = true assert_eq!(constraints.dangerously_allow_all_unix_sockets, Some(true)); } +#[test] +fn selected_network_from_tables_ignores_builtin_profile_without_permissions_table() { + let config: toml::Value = toml::from_str( + r#" +default_permissions = ":workspace" +"#, + ) + .expect("built-in profile config should parse"); + + let network = selected_network_from_tables( + network_tables_from_toml(&config).expect("built-in profile config should deserialize"), + ) + .expect("built-in profile selection should not require permissions tables"); + + assert_eq!(network, None); +} + +#[test] +fn selected_network_from_tables_rejects_unknown_builtin_profile_without_permissions_table() { + let config: toml::Value = toml::from_str( + r#" +default_permissions = ":unknown" +"#, + ) + .expect("unknown built-in config should parse"); + + let err = selected_network_from_tables( + network_tables_from_toml(&config).expect("unknown built-in config should deserialize"), + ) + .expect_err("unknown built-in profile should be rejected"); + + assert_eq!( + err.to_string(), + "default_permissions refers to unknown built-in profile `:unknown`" + ); +} + +#[test] +fn selected_network_from_tables_resolves_builtin_workspace_parent() { + let config: toml::Value = toml::from_str( + r#" +default_permissions = "dev" + +[permissions.dev] +extends = ":workspace" + +[permissions.dev.network] +enabled = true + +[permissions.dev.network.domains] +"child.example.com" = "allow" +"#, + ) + .expect("dev extension config should parse"); + + let network = selected_network_from_tables( + network_tables_from_toml(&config).expect("dev extension config should deserialize"), + ) + .expect("dev extension should resolve") + .expect("dev extension should expose child network config"); + + assert_eq!( + network, + NetworkToml { + enabled: Some(true), + domains: Some(NetworkDomainPermissionsToml { + entries: BTreeMap::from([( + "child.example.com".to_string(), + NetworkDomainPermissionToml::Allow, + )]), + }), + ..Default::default() + } + ); +} + +#[test] +fn selected_network_from_tables_resolves_permission_profile_inheritance() { + let config: toml::Value = toml::from_str( + r#" +default_permissions = "dev" + +[permissions.base.network] +enabled = true +dangerously_allow_all_unix_sockets = true + +[permissions.base.network.domains] +"base.example.com" = "allow" +"shared.example.com" = "deny" + +[permissions.dev] +extends = "base" + +[permissions.dev.network] +allow_local_binding = true + +[permissions.dev.network.domains] +"child.example.com" = "allow" +"shared.example.com" = "allow" +"#, + ) + .expect("permissions profiles should parse"); + + let network = selected_network_from_tables( + network_tables_from_toml(&config).expect("permissions profiles should deserialize"), + ) + .expect("permissions profiles should select a network table") + .expect("network table should be present"); + + assert_eq!( + network, + NetworkToml { + enabled: Some(true), + dangerously_allow_all_unix_sockets: Some(true), + allow_local_binding: Some(true), + domains: Some(NetworkDomainPermissionsToml { + entries: BTreeMap::from([ + ( + "base.example.com".to_string(), + NetworkDomainPermissionToml::Allow, + ), + ( + "child.example.com".to_string(), + NetworkDomainPermissionToml::Allow, + ), + ( + "shared.example.com".to_string(), + NetworkDomainPermissionToml::Allow, + ), + ]), + }), + ..Default::default() + } + ); +} + +#[test] +fn config_from_layers_resolves_inherited_profiles_across_layers() { + let lower_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::toml! { + [permissions.base.network.domains] + "base.example.com" = "allow" + } + .into(), + ); + let higher_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::toml! { + default_permissions = "dev" + + [permissions.dev] + extends = "base" + + [permissions.dev.network.domains] + "child.example.com" = "allow" + } + .into(), + ); + let layers = ConfigLayerStack::new( + vec![lower_layer, higher_layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("layer stack should be valid"); + + let config = + config_from_layers(&layers, &Policy::empty()).expect("inherited profiles should load"); + + assert_eq!( + config.network.allowed_domains(), + Some(vec![ + "base.example.com".to_string(), + "child.example.com".to_string(), + ]) + ); +} + +#[test] +fn config_from_layers_normalizes_profile_network_domains_before_merging_layers() { + let lower_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::toml! { + default_permissions = "dev" + + [permissions.dev.network.domains] + "example.com" = "deny" + } + .into(), + ); + let higher_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::toml! { + [permissions.dev.network.domains] + "EXAMPLE.COM" = "allow" + } + .into(), + ); + let layers = ConfigLayerStack::new( + vec![lower_layer, higher_layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("layer stack should be valid"); + + let config = config_from_layers(&layers, &Policy::empty()) + .expect("network domain layer precedence should load"); + + assert_eq!( + config.network.allowed_domains(), + Some(vec!["example.com".to_string()]) + ); + assert_eq!(config.network.denied_domains(), None); +} + +#[test] +fn config_from_layers_uses_only_the_final_selected_profile_network() { + let lower_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::toml! { + default_permissions = "dev" + + [permissions.dev.network.domains] + "lower.example.com" = "allow" + } + .into(), + ); + let higher_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::toml! { + default_permissions = ":workspace" + } + .into(), + ); + let layers = ConfigLayerStack::new( + vec![lower_layer, higher_layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("layer stack should be valid"); + + let config = config_from_layers(&layers, &Policy::empty()) + .expect("final built-in profile selection should load"); + + assert_eq!(config.network.allowed_domains(), None); + assert_eq!(config.network.denied_domains(), None); +} + +#[test] +fn trusted_constraints_use_only_the_final_selected_profile_network() { + let lower_layer = ConfigLayerEntry::new( + ConfigLayerSource::System { + file: AbsolutePathBuf::try_from(std::path::PathBuf::from("/tmp/system.toml")) + .expect("system config path should be absolute"), + }, + toml::toml! { + default_permissions = "dev" + + [permissions.dev.network.domains] + "managed.example.com" = "allow" + } + .into(), + ); + let higher_layer = ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: AbsolutePathBuf::try_from(std::path::PathBuf::from("/tmp/managed.toml")) + .expect("managed config path should be absolute"), + }, + toml::toml! { + default_permissions = ":workspace" + } + .into(), + ); + let layers = ConfigLayerStack::new( + vec![lower_layer, higher_layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("layer stack should be valid"); + + let constraints = network_constraints_from_trusted_layers(&layers) + .expect("final built-in trusted selection should load"); + + assert_eq!(constraints.allowed_domains, None); + assert_eq!(constraints.denied_domains, None); +} + +#[test] +fn trusted_constraints_normalize_profile_network_domains_before_merging_layers() { + let lower_layer = ConfigLayerEntry::new( + ConfigLayerSource::System { + file: AbsolutePathBuf::try_from(std::path::PathBuf::from("/tmp/system.toml")) + .expect("system config path should be absolute"), + }, + toml::toml! { + default_permissions = "dev" + + [permissions.dev.network.domains] + "example.com" = "deny" + } + .into(), + ); + let higher_layer = ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: AbsolutePathBuf::try_from(std::path::PathBuf::from("/tmp/managed.toml")) + .expect("managed config path should be absolute"), + }, + toml::toml! { + [permissions.dev.network.domains] + "EXAMPLE.COM" = "allow" + } + .into(), + ); + let layers = ConfigLayerStack::new( + vec![lower_layer, higher_layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("layer stack should be valid"); + + let constraints = network_constraints_from_trusted_layers(&layers) + .expect("trusted network domain layer precedence should load"); + + assert_eq!( + constraints.allowed_domains, + Some(vec!["example.com".to_string()]) + ); + assert_eq!(constraints.denied_domains, None); +} + #[test] fn apply_network_constraints_skips_empty_domain_sides() { let config: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "managed.example.com" = "allow" "#, ) @@ -203,22 +541,22 @@ default_permissions = "workspace" fn apply_network_constraints_overlay_domain_entries() { let lower_network: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "blocked.example.com" = "deny" "#, ) .expect("lower layer should parse"); let higher_network: toml::Value = toml::from_str( r#" -default_permissions = "workspace" +default_permissions = "dev" -[permissions.workspace.network] +[permissions.dev.network] -[permissions.workspace.network.domains] +[permissions.dev.network.domains] "api.example.com" = "allow" "#, ) diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index bdffd4e3ca..b710e095ef 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2609,6 +2609,10 @@ impl Session { self.persist_rollout_items(&[RolloutItem::TurnContext(turn_context_item)]) .await; } + { + let mut state = self.state.lock().await; + state.queue_pending_session_start_source(codex_hooks::SessionStartSource::Compact); + } self.services.model_client.advance_window_generation(); } diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 8945880c33..88958b39ef 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1184,7 +1184,7 @@ impl Session { Box::pin(sess.record_initial_history(initial_history)).await; { let mut state = sess.state.lock().await; - state.set_pending_session_start_source(Some(session_start_source)); + state.queue_pending_session_start_source(session_start_source); } Ok(sess) diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index 0903d0d073..baeaddbbf9 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -4,6 +4,7 @@ use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::ResponseItem; use codex_sandboxing::policy_transforms::merge_permission_profiles; use std::collections::HashSet; +use std::collections::VecDeque; use super::auto_compact_window::AutoCompactWindow; use super::auto_compact_window::AutoCompactWindowSnapshot; @@ -33,7 +34,7 @@ pub(crate) struct SessionState { /// Startup prewarmed session prepared during session initialization. pub(crate) startup_prewarm: Option, pub(crate) active_connector_selection: HashSet, - pub(crate) pending_session_start_source: Option, + pub(crate) pending_session_start_sources: VecDeque, granted_permissions: Option, next_turn_is_first: bool, } @@ -52,7 +53,7 @@ impl SessionState { auto_compact_window: AutoCompactWindow::new(), startup_prewarm: None, active_connector_selection: HashSet::new(), - pending_session_start_source: None, + pending_session_start_sources: VecDeque::new(), granted_permissions: None, next_turn_is_first: true, } @@ -218,17 +219,17 @@ impl SessionState { self.active_connector_selection.clear(); } - pub(crate) fn set_pending_session_start_source( + pub(crate) fn queue_pending_session_start_source( &mut self, - value: Option, + value: codex_hooks::SessionStartSource, ) { - self.pending_session_start_source = value; + self.pending_session_start_sources.push_back(value); } pub(crate) fn take_pending_session_start_source( &mut self, ) -> Option { - self.pending_session_start_source.take() + self.pending_session_start_sources.pop_front() } pub(crate) fn record_granted_permissions(&mut self, permissions: AdditionalPermissionProfile) { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 4f376573b3..e9067b8a43 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -6,6 +6,8 @@ use anyhow::Result; use codex_core::config::Config; use codex_core::config::Constrained; use codex_features::Feature; +use codex_model_provider_info::ModelProviderInfo; +use codex_model_provider_info::built_in_model_providers; use codex_plugin::PluginHookSource; use codex_plugin::PluginId; use codex_protocol::items::parse_hook_prompt_fragment; @@ -26,11 +28,13 @@ use core_test_support::managed_network_requirements_loader; use core_test_support::responses::ev_apply_patch_custom_tool_call; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_completed_with_tokens; use core_test_support::responses::ev_custom_tool_call; use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_message_item_added; use core_test_support::responses::ev_output_text_delta; use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_compact_json_once; use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; @@ -74,6 +78,15 @@ fn network_workspace_write_profile() -> PermissionProfile { ) } +fn non_openai_model_provider(server: &wiremock::MockServer) -> ModelProviderInfo { + let mut provider = + built_in_model_providers(/* openai_base_url */ /*openai_base_url*/ None)["openai"].clone(); + provider.name = "OpenAI (test)".into(); + provider.base_url = Some(format!("{}/v1", server.uri())); + provider.supports_websockets = false; + provider +} + fn trust_plugin_hooks(config: &mut Config, plugin_hook_sources: Vec) { if let Err(err) = config.features.enable(Feature::CodexHooks) { panic!("test config should allow feature update: {err}"); @@ -229,6 +242,66 @@ if payload.get("prompt") == {blocked_prompt_json}: Ok(()) } +fn write_session_start_and_user_prompt_submit_order_hooks(home: &Path) -> Result<()> { + let session_start_script_path = home.join("session_start_order_hook.py"); + let user_prompt_submit_script_path = home.join("user_prompt_submit_order_hook.py"); + let log_path = home.join("hook_order_log.jsonl"); + + let session_start_script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps({{ + "hook_event_name": payload.get("hook_event_name"), + "source": payload.get("source"), + }}) + "\n") +"#, + log_path = log_path.display(), + ); + let user_prompt_submit_script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps({{ + "hook_event_name": payload.get("hook_event_name"), + "prompt": payload.get("prompt"), + }}) + "\n") +"#, + log_path = log_path.display(), + ); + let hooks = serde_json::json!({ + "hooks": { + "SessionStart": [{ + "hooks": [{ + "type": "command", + "command": format!("python3 {}", session_start_script_path.display()), + "statusMessage": "running session start order hook", + }] + }], + "UserPromptSubmit": [{ + "hooks": [{ + "type": "command", + "command": format!("python3 {}", user_prompt_submit_script_path.display()), + "statusMessage": "running user prompt submit order hook", + }] + }] + } + }); + + fs::write(&session_start_script_path, session_start_script) + .context("write session start order hook script")?; + fs::write(&user_prompt_submit_script_path, user_prompt_submit_script) + .context("write user prompt submit order hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn write_pre_tool_use_hook( home: &Path, matcher: Option<&str>, @@ -705,6 +778,109 @@ print(json.dumps({{ Ok(()) } +fn write_compact_session_start_hook_with_context( + home: &Path, + additional_context: &str, +) -> Result<()> { + let script_path = home.join("compact_session_start_hook.py"); + let log_path = home.join("session_start_hook_log.jsonl"); + let additional_context_json = serde_json::to_string(additional_context) + .context("serialize compact session start additional context for test")?; + let script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "SessionStart", + "additionalContext": {additional_context_json} + }} +}})) +"#, + log_path = log_path.display(), + ); + let hooks = serde_json::json!({ + "hooks": { + "SessionStart": [{ + "matcher": "compact", + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running compact session start hook", + }] + }] + } + }); + + fs::write(&script_path, script).context("write compact session start hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + +fn write_resume_and_compact_session_start_hook_with_context( + home: &Path, + resume_context: &str, + compact_context: &str, +) -> Result<()> { + let script_path = home.join("resume_and_compact_session_start_hook.py"); + let log_path = home.join("session_start_hook_log.jsonl"); + let resume_context_json = serde_json::to_string(resume_context) + .context("serialize resume session start additional context for test")?; + let compact_context_json = serde_json::to_string(compact_context) + .context("serialize compact session start additional context for test")?; + let script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +contexts = {{ + "resume": {resume_context_json}, + "compact": {compact_context_json}, +}} +print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "SessionStart", + "additionalContext": contexts[payload["source"]] + }} +}})) +"#, + log_path = log_path.display(), + ); + let hooks = serde_json::json!({ + "hooks": { + "SessionStart": [{ + "matcher": "resume", + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running resume session start hook", + }] + }, { + "matcher": "compact", + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running compact session start hook", + }] + }] + } + }); + + fs::write(&script_path, script) + .context("write resume and compact session start hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn rollout_hook_prompt_texts(text: &str) -> Result> { let mut texts = Vec::new(); for line in text.lines() { @@ -837,6 +1013,10 @@ fn read_user_prompt_submit_hook_inputs(home: &Path) -> Result Result> { + read_hook_inputs_from_log(home.join("hook_order_log.jsonl").as_path()) +} + fn ev_message_item_done(id: &str, text: &str) -> Value { serde_json::json!({ "type": "response.output_item.done", @@ -1020,6 +1200,54 @@ async fn session_start_hook_sees_materialized_transcript_path() -> Result<()> { Ok(()) } +#[tokio::test] +async fn session_start_runs_before_user_prompt_submit_on_first_turn() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let _response = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "hello after hooks"), + ev_completed("resp-1"), + ]), + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = write_session_start_and_user_prompt_submit_order_hooks(home) { + panic!("failed to write hook ordering fixtures: {error}"); + } + }) + .with_config(trust_discovered_hooks); + let test = builder.build(&server).await?; + + test.submit_turn("hello").await?; + + let hook_inputs = read_hook_order_inputs(test.codex_home_path())?; + assert_eq!( + hook_inputs + .iter() + .map(|input| input["hook_event_name"] + .as_str() + .expect("hook input event name")) + .collect::>(), + vec!["SessionStart", "UserPromptSubmit"], + ); + assert_eq!( + hook_inputs[0].get("source").and_then(Value::as_str), + Some("startup") + ); + assert_eq!( + hook_inputs[1].get("prompt").and_then(Value::as_str), + Some("hello") + ); + + Ok(()) +} + #[tokio::test] async fn session_start_hook_spills_large_additional_context() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1125,6 +1353,188 @@ async fn pre_tool_use_hook_spills_large_additional_context() -> Result<()> { Ok(()) } +#[tokio::test] +async fn compact_session_start_hook_records_additional_context_for_next_turn() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "hello before compact"), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", "summary after compact"), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-3", "hello after compact"), + ev_completed("resp-3"), + ]), + ], + ) + .await; + let additional_context = "remember the compacted reef"; + let model_provider = non_openai_model_provider(&server); + + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = + write_compact_session_start_hook_with_context(home, additional_context) + { + panic!("failed to write compact session start hook fixture: {error}"); + } + }) + .with_config(move |config| { + config.model_provider = model_provider; + trust_discovered_hooks(config); + }); + let test = builder.build(&server).await?; + + test.submit_turn("hello before compact").await?; + test.codex.submit(Op::Compact).await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + test.submit_turn("hello after compact").await?; + + let requests = request_log.requests(); + assert_eq!(requests.len(), 3); + assert!( + !requests[0] + .message_input_texts("developer") + .iter() + .any(|message| message == additional_context), + "compact matcher should not run for initial startup", + ); + assert!( + requests[2] + .message_input_texts("developer") + .iter() + .any(|message| message == additional_context), + "compact matcher should inject additional context before the next model turn", + ); + + let hook_inputs = read_session_start_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + hook_inputs[0].get("source").and_then(Value::as_str), + Some("compact") + ); + + Ok(()) +} + +#[tokio::test] +async fn resumed_thread_runs_resume_then_compact_session_start_hooks() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let limit = 200_000; + let over_limit_tokens = 250_000; + let remote_summary = "remote compact summary"; + let resume_context = "remember the resumed reef"; + let compact_context = "remember the compacted reef"; + let compacted_history = vec![ + ResponseItem::Message { + id: None, + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: remote_summary.to_string(), + }], + phase: None, + }, + ResponseItem::Compaction { + encrypted_content: "encrypted compact summary".to_string(), + }, + ]; + let compact_mock = + mount_compact_json_once(&server, serde_json::json!({ "output": compacted_history })).await; + + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = write_resume_and_compact_session_start_hook_with_context( + home, + resume_context, + compact_context, + ) { + panic!("failed to write resume/compact session start hook fixture: {error}"); + } + }) + .with_config(move |config| { + config.model_auto_compact_token_limit = Some(limit); + trust_discovered_hooks(config); + }); + let initial = builder.build(&server).await?; + let home = initial.home.clone(); + let rollout_path = initial + .session_configured + .rollout_path + .clone() + .context("rollout path")?; + + mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "hello before resume"), + ev_completed_with_tokens("resp-1", over_limit_tokens), + ]), + ) + .await; + initial.submit_turn("hello before resume").await?; + assert!(compact_mock.requests().is_empty()); + + let mut resume_builder = test_codex().with_config(move |config| { + config.model_auto_compact_token_limit = Some(limit); + trust_discovered_hooks(config); + }); + let resumed = resume_builder.resume(&server, home, rollout_path).await?; + let follow_up = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", "hello after resume"), + ev_completed("resp-2"), + ]), + ) + .await; + + resumed.submit_turn("hello after resume").await?; + + assert_eq!(compact_mock.requests().len(), 1); + let developer_messages = follow_up.single_request().message_input_texts("developer"); + assert!( + developer_messages + .iter() + .any(|message| message == resume_context), + "resume matcher should inject additional context before the next model turn", + ); + assert!( + developer_messages + .iter() + .any(|message| message == compact_context), + "compact matcher should inject additional context before the next model turn", + ); + + let hook_inputs = read_session_start_hook_inputs(resumed.codex_home_path())?; + assert_eq!( + hook_inputs + .iter() + .filter_map(|input| input.get("source").and_then(Value::as_str)) + .collect::>(), + vec!["resume", "compact"], + ); + + Ok(()) +} + #[tokio::test] async fn stop_hook_spills_large_continuation_prompt() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/windows_sandbox.rs b/codex-rs/core/tests/suite/windows_sandbox.rs index 0be8be3bbb..7b91a4f3cd 100644 --- a/codex-rs/core/tests/suite/windows_sandbox.rs +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -18,6 +18,7 @@ use serial_test::serial; use std::collections::HashMap; use std::ffi::OsString; use std::path::Path; +use std::path::PathBuf; use tempfile::TempDir; struct EnvVarGuard { @@ -46,13 +47,49 @@ impl Drop for EnvVarGuard { } } +enum TestCodexHome { + Persistent(PathBuf), + Temporary(TempDir), +} + +impl TestCodexHome { + fn path(&self) -> &Path { + match self { + Self::Persistent(path) => path.as_path(), + Self::Temporary(temp_dir) => temp_dir.path(), + } + } +} + +fn codex_home_for_windows_sandbox_test(name: &str) -> anyhow::Result { + if let Some(test_tmpdir) = std::env::var_os("TEST_TMPDIR") { + // The elevated backend provisions machine-local sandbox users. Bazel + // retries run in the same Windows VM, so keep CODEX_HOME stable within + // the test temp root and let setup reconcile its persisted ACL state. + let codex_home = PathBuf::from(test_tmpdir).join(name); + std::fs::create_dir_all(&codex_home) + .with_context(|| format!("create stable test CODEX_HOME {}", codex_home.display()))?; + return Ok(TestCodexHome::Persistent(codex_home)); + } + + Ok(TestCodexHome::Temporary(TempDir::new()?)) +} + fn stage_windows_sandbox_helpers() -> anyhow::Result<()> { let test_exe = std::env::current_exe().context("resolve current Windows test executable")?; let test_exe_dir = test_exe .parent() .context("Windows test executable should have a parent directory")?; let resources_dir = test_exe_dir.join("codex-resources"); - std::fs::create_dir_all(&resources_dir)?; + match std::fs::create_dir_all(&resources_dir) { + Ok(()) => {} + Err(err) + if err.kind() == std::io::ErrorKind::PermissionDenied && resources_dir.is_dir() => {} + Err(err) => { + return Err(err) + .with_context(|| format!("create resources dir {}", resources_dir.display())); + } + } for helper_name in ["codex-windows-sandbox-setup", "codex-command-runner"] { let helper = codex_utils_cargo_bin::cargo_bin(helper_name)?; let file_name = Path::new(helper_name).with_extension("exe"); @@ -79,8 +116,9 @@ fn stage_windows_sandbox_helpers() -> anyhow::Result<()> { #[tokio::test] #[serial(codex_home)] async fn windows_restricted_token_rejects_exact_and_glob_deny_read_policy() -> anyhow::Result<()> { - let temp_home = TempDir::new()?; - let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", temp_home.path().as_os_str()); + let codex_home = + codex_home_for_windows_sandbox_test("windows-restricted-token-deny-read-codex-home")?; + let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", codex_home.path().as_os_str()); let workspace = TempDir::new()?; let cwd = dunce::canonicalize(workspace.path())?.abs(); let secret = cwd.join("secret.env"); @@ -159,8 +197,8 @@ async fn windows_restricted_token_rejects_exact_and_glob_deny_read_policy() -> a #[tokio::test] #[serial(codex_home)] async fn windows_elevated_enforces_exact_and_glob_deny_read_policy() -> anyhow::Result<()> { - let temp_home = TempDir::new()?; - let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", temp_home.path().as_os_str()); + let codex_home = codex_home_for_windows_sandbox_test("windows-elevated-deny-read-codex-home")?; + let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", codex_home.path().as_os_str()); stage_windows_sandbox_helpers()?; let workspace = TempDir::new()?; let cwd = dunce::canonicalize(workspace.path())?.abs(); diff --git a/codex-rs/hooks/schema/generated/session-start.command.input.schema.json b/codex-rs/hooks/schema/generated/session-start.command.input.schema.json index 385932c364..d460fdb583 100644 --- a/codex-rs/hooks/schema/generated/session-start.command.input.schema.json +++ b/codex-rs/hooks/schema/generated/session-start.command.input.schema.json @@ -37,7 +37,8 @@ "enum": [ "startup", "resume", - "clear" + "clear", + "compact" ], "type": "string" }, diff --git a/codex-rs/hooks/src/events/session_start.rs b/codex-rs/hooks/src/events/session_start.rs index 2904497b40..bd1aa2096f 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -24,6 +24,7 @@ pub enum SessionStartSource { Startup, Resume, Clear, + Compact, } impl SessionStartSource { @@ -32,6 +33,7 @@ impl SessionStartSource { Self::Startup => "startup", Self::Resume => "resume", Self::Clear => "clear", + Self::Compact => "compact", } } } diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 96d5f790be..15a655c976 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -673,7 +673,7 @@ fn permission_mode_schema(_gen: &mut SchemaGenerator) -> Schema { } fn session_start_source_schema(_gen: &mut SchemaGenerator) -> Schema { - string_enum_schema(&["startup", "resume", "clear"]) + string_enum_schema(&["startup", "resume", "clear", "compact"]) } fn compaction_trigger_schema(_gen: &mut SchemaGenerator) -> Schema { diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 2927cb1cd1..28a7d6522c 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -339,8 +339,8 @@ pub struct ActivePermissionProfile { /// profile. pub id: String, - /// Optional parent profile identifier once permissions profiles support - /// inheritance. This is always `None` until that config feature exists. + /// Optional parent profile identifier from the selected permissions + /// profile's `extends` setting. #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] pub extends: Option, diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/SKILL.md b/codex-rs/skills/src/assets/samples/plugin-creator/SKILL.md index dde5779e6c..af960ec50c 100644 --- a/codex-rs/skills/src/assets/samples/plugin-creator/SKILL.md +++ b/codex-rs/skills/src/assets/samples/plugin-creator/SKILL.md @@ -1,6 +1,6 @@ --- name: plugin-creator -description: Create and scaffold plugin directories for Codex with a required `.codex-plugin/plugin.json`, optional plugin folders/files, valid manifest defaults, and personal-marketplace entries by default. Use when Codex needs to create a new personal plugin, add optional plugin structure, or generate or update marketplace entries for plugin ordering and availability metadata. +description: Create and scaffold plugin directories for Codex with a required `.codex-plugin/plugin.json`, optional plugin folders/files, valid manifest defaults, and personal-marketplace entries by default. Use when Codex needs to create a new personal plugin, add optional plugin structure, generate or update marketplace entries for plugin ordering and availability metadata, or update an existing local plugin during development with the CLI-driven cachebuster and reinstall flow. --- # Plugin Creator @@ -10,11 +10,11 @@ description: Create and scaffold plugin directories for Codex with a required `. 1. Run the scaffold script: ```bash - # Plugin names are normalized to lower-case hyphen-case and must be <= 64 chars. - # The generated folder and plugin.json name are always the same. -# Run from repo root (or replace .agents/... with the absolute path to this SKILL). -# By default creates in ~/plugins/. -python3 .agents/skills/plugin-creator/scripts/create_basic_plugin.py +# Plugin names are normalized to lower-case hyphen-case and must be <= 64 chars. +# The generated folder and plugin.json name are always the same. +# Run from the skill root (the directory containing this `SKILL.md`). +# By default creates in `~/plugins/`. +python3 scripts/create_basic_plugin.py ``` 2. Edit `/.codex-plugin/plugin.json` when the request gives specific metadata. @@ -23,40 +23,66 @@ python3 .agents/skills/plugin-creator/scripts/create_basic_plugin.py ` when the default `personal` marketplace name is already +taken or installed and you need to seed a different new marketplace file: + +```bash +python3 scripts/create_basic_plugin.py my-plugin \ + --with-marketplace \ + --marketplace-name team-local ``` Only use a repo/team marketplace when the user specifically asks for that destination: ```bash -python3 .agents/skills/plugin-creator/scripts/create_basic_plugin.py my-plugin \ +python3 scripts/create_basic_plugin.py my-plugin \ --path /plugins \ --marketplace-path /.agents/plugins/marketplace.json \ --with-marketplace ``` +When the user specifies a marketplace path, make sure that marketplace is actually installed before +telling the user to reinstall from it. The default personal marketplace file at +`~/.agents/plugins/marketplace.json` is discovered implicitly, but other marketplace paths are not. +On Windows, use the equivalent path under the user profile. + 4. Generate/adjust optional companion folders as needed: ```bash -python3 .agents/skills/plugin-creator/scripts/create_basic_plugin.py my-plugin \ +python3 scripts/create_basic_plugin.py my-plugin \ --path \ --marketplace-path \ --with-skills --with-hooks --with-scripts --with-assets --with-mcp --with-apps --with-marketplace ``` -`` is the directory where the plugin folder `` will be created (for example `~/code/plugins`). +`` is the directory where the plugin folder `` will be +created (for example `~/plugins`). 5. Before handing back a generated plugin, run: ```bash -python3 .agents/skills/plugin-creator/scripts/validate_plugin.py +python3 scripts/validate_plugin.py ``` +For updates to an existing local plugin during development, keep the scaffold flow as-is and use the +reference instead of hand-editing marketplace files: + +```bash +python3 scripts/update_plugin_cachebuster.py +``` + +Prefer the helper default cachebuster unless the user explicitly asks for a specific override. +See `references/installing-and-updating.md` for the expected cachebuster and reinstall flow while iterating on an existing local plugin. + ## What this skill creates -- Default marketplace-backed scaffolds are personal: `~/plugins//` plus - `~/.agents/plugins/marketplace.json`. +- Default marketplace-backed scaffolds use the personal marketplace file at + `~/.agents/plugins/marketplace.json`, with plugins generally being stored in + `~/plugins//`. - Creates plugin root at `///`. - Always creates `///.codex-plugin/plugin.json`. - Fills the manifest with the validated schema shape that the ingestion path accepts. @@ -77,9 +103,18 @@ python3 .agents/skills/plugin-creator/scripts/validate_plugin.py ## Marketplace workflow -- Personal creation defaults to `~/.agents/plugins/marketplace.json`. +- Personal-marketplace creation defaults to `~/.agents/plugins/marketplace.json`. Here, + "personal marketplace" means the marketplace whose file is at that path. - Repo/team marketplace creation is opt-in through both `--path` and `--marketplace-path`, only when the user specifically requests it. +- `--marketplace-name` is an exception path. Use it only when the default `personal` marketplace + name is already taken and you need to seed a different new marketplace file. +- Do not use `--marketplace-name` to rename an existing marketplace file in place. If the file + already exists, its top-level `name` must already match. +- If the user specifies a different marketplace path, treat that marketplace as needing explicit installation via `codex plugin marketplace add`. +- Prefer `scripts/read_marketplace_name.py` when you need the marketplace name from any + `marketplace.json` file. With no argument it reads the default personal marketplace; with an + explicit path it works for repo/team marketplaces too. - In either location, the generated source path remains `./plugins/`. - Marketplace root metadata supports top-level `name` plus optional `interface.displayName`. - Treat plugin order in `plugins[]` as render order in Codex. Append new entries unless a user explicitly asks to reorder the list. @@ -157,6 +192,21 @@ python3 .agents/skills/plugin-creator/scripts/validate_plugin.py - When generating marketplace entries, always write `policy.installation`, `policy.authentication`, and `category` even if their values are defaults. - Add `policy.products` only when the user explicitly asks for that override. - Keep marketplace `source.path` relative to the selected marketplace root as `./plugins/`. +- Only use `--marketplace-name` when creating a new marketplace file whose name should not be + `personal` because that name is already taken or installed elsewhere. +- If Codex would need approval to write the marketplace file, ask for that approval before + proceeding. If the user prefers to run the write themselves, provide the exact scaffold command + and then continue from validation or subsequent plugin edits instead of leaving the workflow + vague. +- For updates to an existing local plugin during development, do not hand-edit marketplace config + or `marketplace.json`. Use the update flow documented in + `references/installing-and-updating.md` and `scripts/update_plugin_cachebuster.py`. +- Do not tell the user to run `codex plugin marketplace add` for the default personal-marketplace + flow. That command is for explicit non-default marketplace configuration, not for the standard + `~/.agents/plugins/marketplace.json` path. +- If the user provided a non-default `--marketplace-path`, make sure that marketplace is installed + before giving reinstall instructions. Use `codex plugin marketplace add ` + when that explicit marketplace has not been configured yet. - When the workflow created or updated a marketplace-backed plugin, end the final user-facing response with a short Codex app handoff. Say `To view this in the Codex app:` and write `View ` and `Share ` as Markdown links, not raw @@ -175,17 +225,19 @@ python3 .agents/skills/plugin-creator/scripts/validate_plugin.py For the exact canonical sample JSON for both plugin manifests and marketplace entries, use: - `references/plugin-json-spec.md` +- `references/installing-and-updating.md` for update/reinstall guidance while + iterating on an existing local plugin, plus the new-thread pickup behavior after reinstall ## Validation After editing `SKILL.md`, run: ```bash -python3 /scripts/quick_validate.py .agents/skills/plugin-creator +python3 ../skill-creator/scripts/quick_validate.py . ``` Before handing back a generated plugin, run: ```bash -python3 .agents/skills/plugin-creator/scripts/validate_plugin.py +python3 scripts/validate_plugin.py ``` diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/references/installing-and-updating.md b/codex-rs/skills/src/assets/samples/plugin-creator/references/installing-and-updating.md new file mode 100644 index 0000000000..28b3b88908 --- /dev/null +++ b/codex-rs/skills/src/assets/samples/plugin-creator/references/installing-and-updating.md @@ -0,0 +1,143 @@ +# Updating Existing Local Plugins + +Use this reference when a plugin already exists and the request is about updating the plugin during +local development. + +All scripts here are specified relative to the skill root. Update the path for running the scripts +depending on your current working directory. + +## When To Use This Flow + +Use this flow when all of the following are true: + +- the plugin already exists locally +- the marketplace entry already points at the plugin source you are editing +- the user wants Codex to see the updated plugin without manually editing marketplace files + +If the user still needs the initial plugin entry or marketplace structure created, use the scaffold +flow first and only then switch to this reinstall flow. + +## Update Loop + +1. Update the plugin manifest to a single Codex cachebuster suffix: + +```bash +python3 scripts/update_plugin_cachebuster.py \ + +``` + +Prefer the default helper behavior here. If you omit `--cachebuster`, the helper uses a UTC +timestamp down to seconds, which is the recommended path for routine local iteration. + +Only use a manual cachebuster override when the user explicitly asks for one or when a workflow +outside Codex depends on a specific token: + +```bash +python3 scripts/update_plugin_cachebuster.py \ + \ + --cachebuster local-20260519-184516 +``` + +2. For the default scaffolded flow, read the marketplace name from the personal marketplace file: + +```bash +python3 scripts/read_marketplace_name.py +``` + +Here, "personal marketplace" means the marketplace whose file is at +`~/.agents/plugins/marketplace.json`. On Windows, use the equivalent path under the user profile. +The helper uses Python's home-directory resolution and prints the marketplace name to use when +constructing the install command. + +To read the name from a different marketplace file, pass the path directly: + +```bash +python3 scripts/read_marketplace_name.py --marketplace-path +``` + +3. Reinstall from that marketplace name: + +```bash +codex plugin add @ +``` + +The default personal marketplace is discovered implicitly from +`~/.agents/plugins/marketplace.json`. You do not need `codex plugin marketplace add` for that +path, and `codex plugin marketplace list` is not the right check for whether that default +marketplace exists. + +4. If the plugin is not using the personal marketplace file, check which configured local + marketplace is actually surfacing that plugin: + +```bash +codex plugin list +``` + +If the plugin is not in the personal marketplace file, confirm which marketplace entry points at +the plugin source you are editing and make sure that marketplace is still local. If it is a +different local marketplace, reinstall from that marketplace name instead of forcing the personal +marketplace flow. If it is not local, stop and help the user resolve the mismatch before +continuing. + +5. If the plugin lives in a different confirmed local marketplace, substitute that marketplace + name: + +```bash +codex plugin add @ +``` + +6. Prompt the user to use a new thread to try the updated plugin, so that Codex picks up new skills + and tools. + +## Cachebuster Policy + +- Preserve the existing version prefix and replace only the suffix. +- Treat the preserved prefix as everything before `+`. +- Use the format: + +```text ++codex. +``` + +Examples: + +- `0.1.0` → `0.1.0+codex.local-20260519-184516` +- `0.1.0+codex.old-token` → `0.1.0+codex.local-20260519-184516` +- `1.2.3-beta.1+codex.prev` → `1.2.3-beta.1+codex.local-20260519-184516` +- `dev-build+other-tag` → `dev-build+codex.local-20260519-184516` + +Replace the existing Codex cachebuster instead of appending another one. Do not keep incrementing +numeric version components just to trigger reinstall behavior. + +## Marketplace Rules + +- Marketplace manipulation should happen through commands, not by hand-editing `marketplace.json` + or `config.toml` during this update/reinstall flow. +- Prefer the personal marketplace file for the default scaffolded flow. +- Read the personal marketplace name with + `python3 scripts/read_marketplace_name.py` and use the printed value when constructing + `codex plugin add @`. +- For non-default marketplace files, use + `python3 scripts/read_marketplace_name.py --marketplace-path ` to read + the name before constructing reinstall commands. +- Do not tell the user to run `codex plugin marketplace add` for the default personal-marketplace + flow. That marketplace is discovered implicitly by Codex. +- If the user specified a different marketplace path, make sure that marketplace is installed + before giving install or reinstall instructions. Non-default marketplace paths are not + discovered implicitly. +- Use `codex plugin list` when the plugin lives in a different configured marketplace and you need + to confirm which marketplace is surfacing that plugin. +- If a non-default local marketplace has not been configured yet, install it with + `codex plugin marketplace add ` before telling the user to run + `codex plugin add @`. +- If the plugin is not in the personal marketplace file, confirm that the selected marketplace is + local before telling the user to reinstall from it. +- If the selected marketplace is not local, stop and help the user resolve that mismatch rather + than pretending the normal local reinstall flow applies. +- If the plugin source is not already the source referenced by the chosen marketplace entry, stop + and fix that first. This update flow does not rewrite marketplace entries. + +## After Reinstall + +After reinstalling, prompt the user to start a new thread for testing. That is the safe boundary for +picking up the updated plugin and its MCP tools. diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md index 0fd444e64b..5eb2251a0e 100644 --- a/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md +++ b/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md @@ -147,7 +147,8 @@ personal marketplace unless the caller explicitly requests a repo-local destinat - Personal plugin in `~/.agents/plugins/marketplace.json`: `./plugins/` - Repo/team plugin: `./plugins/` - The same relative path convention is used for both personal and repo/team marketplaces. - - Example: with `~/.agents/plugins/marketplace.json`, `./plugins/` resolves to `~/plugins/`. + - Example: with `~/.agents/plugins/marketplace.json`, `./plugins/` resolves to + `~/plugins/`. - `policy` (`object`): Marketplace policy block. Always include it. - `installation` (`string`): Availability policy. - Allowed values: `NOT_AVAILABLE`, `AVAILABLE`, `INSTALLED_BY_DEFAULT` @@ -168,6 +169,8 @@ personal marketplace unless the caller explicitly requests a repo-local destinat - Replace an existing entry for the same plugin only when overwrite is intentional. - Default new plugin creation to the personal marketplace. - Use a repo/team marketplace only when the user specifically requests that destination. +- Only override the marketplace `name` when the default `personal` name is already taken or + installed and you need to seed a different new marketplace file. - Choose marketplace location to match the selected destination: - Personal plugin: `~/.agents/plugins/marketplace.json` - Repo/team plugin: `/.agents/plugins/marketplace.json` diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/create_basic_plugin.py b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/create_basic_plugin.py index fb882ce99a..78b9fca8fc 100755 --- a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/create_basic_plugin.py +++ b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/create_basic_plugin.py @@ -11,15 +11,14 @@ from typing import Any MAX_PLUGIN_NAME_LENGTH = 64 -DEFAULT_PLUGIN_PARENT = Path.home() / "plugins" -DEFAULT_MARKETPLACE_PATH = Path.home() / ".agents" / "plugins" / "marketplace.json" DEFAULT_INSTALL_POLICY = "AVAILABLE" DEFAULT_AUTH_POLICY = "ON_INSTALL" DEFAULT_CATEGORY = "Productivity" DEFAULT_MARKETPLACE_NAME = "personal" -DEFAULT_MARKETPLACE_DISPLAY_NAME = "Personal" VALID_INSTALL_POLICIES = {"NOT_AVAILABLE", "AVAILABLE", "INSTALLED_BY_DEFAULT"} VALID_AUTH_POLICIES = {"ON_INSTALL", "ON_USE"} +DEFAULT_PLUGIN_PARENT = Path.home() / "plugins" +DEFAULT_MARKETPLACE_PATH = Path.home() / ".agents" / "plugins" / "marketplace.json" def normalize_plugin_name(plugin_name: str) -> str: @@ -41,8 +40,17 @@ def validate_plugin_name(plugin_name: str) -> None: ) +def validate_marketplace_name(marketplace_name: str) -> None: + if not marketplace_name: + raise ValueError("Marketplace name must include at least one letter or digit.") + if re.fullmatch(r"[A-Za-z0-9_-]+", marketplace_name) is None: + raise ValueError( + "Marketplace name may only contain ASCII letters, digits, `_`, and `-`." + ) + + def display_name_from_plugin_name(plugin_name: str) -> str: - return " ".join(part.capitalize() for part in plugin_name.split("-")) + return " ".join(part.capitalize() for part in re.split(r"[-_]+", plugin_name)) def build_plugin_json(plugin_name: str, *, with_mcp: bool, with_apps: bool) -> dict[str, Any]: @@ -97,11 +105,11 @@ def load_json(path: Path) -> dict[str, Any]: return json.load(handle) -def build_default_marketplace() -> dict[str, Any]: +def build_default_marketplace(marketplace_name: str) -> dict[str, Any]: return { - "name": DEFAULT_MARKETPLACE_NAME, + "name": marketplace_name, "interface": { - "displayName": DEFAULT_MARKETPLACE_DISPLAY_NAME, + "displayName": display_name_from_plugin_name(marketplace_name), }, "plugins": [], } @@ -115,6 +123,7 @@ def validate_marketplace_interface(payload: dict[str, Any]) -> None: def update_marketplace_json( marketplace_path: Path, + marketplace_name: str | None, plugin_name: str, install_policy: str, auth_policy: str, @@ -124,13 +133,24 @@ def update_marketplace_json( if marketplace_path.exists(): payload = load_json(marketplace_path) else: - payload = build_default_marketplace() + payload = build_default_marketplace(marketplace_name or DEFAULT_MARKETPLACE_NAME) if not isinstance(payload, dict): raise ValueError(f"{marketplace_path} must contain a JSON object.") validate_marketplace_interface(payload) + existing_marketplace_name = payload.get("name") + if marketplace_name is not None: + if not isinstance(existing_marketplace_name, str) or not existing_marketplace_name.strip(): + raise ValueError(f"{marketplace_path} must contain a non-empty string 'name'.") + if existing_marketplace_name != marketplace_name: + raise ValueError( + f"{marketplace_path} already uses marketplace name " + f"'{existing_marketplace_name}'. Create a new marketplace file to use " + f"'{marketplace_name}' instead." + ) + plugins = payload.setdefault("plugins", []) if not isinstance(plugins, list): raise ValueError(f"{marketplace_path} field 'plugins' must be an array.") @@ -206,6 +226,13 @@ def parse_args() -> argparse.Namespace: "Pass a repo-rooted marketplace path only when a repo/team plugin is intended." ), ) + parser.add_argument( + "--marketplace-name", + help=( + "Marketplace name to seed into a new marketplace.json. Use this only when the default " + "'personal' marketplace name is already taken and you need a different new marketplace." + ), + ) parser.add_argument( "--install-policy", default=DEFAULT_INSTALL_POLICY, @@ -234,6 +261,10 @@ def main() -> None: if plugin_name != raw_plugin_name: print(f"Note: Normalized plugin name from '{raw_plugin_name}' to '{plugin_name}'.") validate_plugin_name(plugin_name) + marketplace_name = None + if args.marketplace_name is not None: + marketplace_name = args.marketplace_name.strip() + validate_marketplace_name(marketplace_name) plugin_root = (Path(args.path).expanduser().resolve() / plugin_name) plugin_root.mkdir(parents=True, exist_ok=True) @@ -275,6 +306,7 @@ def main() -> None: marketplace_path = Path(args.marketplace_path).expanduser().resolve() update_marketplace_json( marketplace_path, + marketplace_name, plugin_name, args.install_policy, args.auth_policy, diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/read_marketplace_name.py b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/read_marketplace_name.py new file mode 100644 index 0000000000..597e9f7a42 --- /dev/null +++ b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/read_marketplace_name.py @@ -0,0 +1,48 @@ +#!/usr/bin/env python3 +"""Print the top-level marketplace name from any marketplace.json file.""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path + + +def default_marketplace_path() -> Path: + return Path.home() / ".agents" / "plugins" / "marketplace.json" + + +def parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description=( + "Print the top-level marketplace name from marketplace.json. Defaults to the personal " + "marketplace path under the current home directory." + ) + ) + parser.add_argument( + "--marketplace-path", + default=str(default_marketplace_path()), + help="Path to marketplace.json", + ) + return parser.parse_args() + + +def main() -> None: + args = parse_args() + marketplace_path = Path(args.marketplace_path).expanduser().resolve() + payload = json.loads(marketplace_path.read_text(encoding="utf-8")) + if not isinstance(payload, dict): + raise ValueError(f"{marketplace_path} must contain a JSON object.") + name = payload.get("name") + if not isinstance(name, str) or not name.strip(): + raise ValueError(f"{marketplace_path} must contain a non-empty string 'name'.") + print(name.strip()) + + +if __name__ == "__main__": + try: + main() + except Exception as err: # noqa: BLE001 - CLI should surface a single clear message. + print(str(err), file=sys.stderr) + raise SystemExit(1) from err diff --git a/codex-rs/skills/src/assets/samples/plugin-creator/scripts/update_plugin_cachebuster.py b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/update_plugin_cachebuster.py new file mode 100644 index 0000000000..82d19e5dfd --- /dev/null +++ b/codex-rs/skills/src/assets/samples/plugin-creator/scripts/update_plugin_cachebuster.py @@ -0,0 +1,78 @@ +#!/usr/bin/env python3 +"""Rewrite a local plugin version to a single Codex cachebuster suffix.""" + +from __future__ import annotations + +import argparse +import json +import re +import sys +from datetime import datetime, timezone +from pathlib import Path + + +CACHEBUSTER_PREFIX = "codex" + + +def parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description=( + "Rewrite a local plugin's version so it preserves everything before '+' and uses " + "a single +codex. suffix." + ) + ) + parser.add_argument("plugin_path", help="Path to the plugin root directory") + parser.add_argument( + "--cachebuster", + help="Optional cachebuster token to embed in the plugin version", + ) + return parser.parse_args() + + +def main() -> None: + args = parse_args() + plugin_root = Path(args.plugin_path).expanduser().resolve() + manifest_path = plugin_root / ".codex-plugin" / "plugin.json" + manifest = load_manifest(manifest_path) + + version = manifest.get("version") + if not isinstance(version, str) or not version.strip(): + raise ValueError(f"{manifest_path} must contain a non-empty string 'version'.") + cachebuster = sanitize_cachebuster(args.cachebuster or default_cachebuster()) + next_version = with_cachebuster(version, cachebuster) + manifest["version"] = next_version + manifest_path.write_text(json.dumps(manifest, indent=2) + "\n", encoding="utf-8") + + print(f"Updated plugin version: {version} -> {next_version}") + + +def load_manifest(manifest_path: Path) -> dict[str, object]: + if not manifest_path.is_file(): + raise FileNotFoundError(f"missing manifest: {manifest_path}") + payload = json.loads(manifest_path.read_text(encoding="utf-8")) + if not isinstance(payload, dict): + raise ValueError(f"{manifest_path} must contain a JSON object.") + return payload +def sanitize_cachebuster(value: str) -> str: + sanitized = re.sub(r"[^a-z0-9-]+", "-", value.strip().lower()) + sanitized = re.sub(r"-{2,}", "-", sanitized).strip("-") + if not sanitized: + raise ValueError("Cachebuster must contain at least one letter or digit.") + return sanitized + + +def default_cachebuster() -> str: + return datetime.now(timezone.utc).strftime("%Y%m%d%H%M%S") + + +def with_cachebuster(version: str, cachebuster: str) -> str: + version_prefix = version.split("+", 1)[0] + return f"{version_prefix}+{CACHEBUSTER_PREFIX}.{cachebuster}" + + +if __name__ == "__main__": + try: + main() + except Exception as err: # noqa: BLE001 - CLI should surface a single clear message. + print(str(err), file=sys.stderr) + raise SystemExit(1) from err