diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 96f4373290..abe8f46674 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -11,9 +11,10 @@ use codex_extension_api::SkillInvocationKind; use codex_otel::sanitize_metric_tag_value; use codex_protocol::protocol::SkillScope; use codex_skills::SkillMetadata; -use codex_skills::detect_implicit_skill_invocation_for_command; use codex_skills_extension::HostSkillsLoadInput; +use codex_skills_extension::detect_implicit_skill_invocation; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use codex_utils_plugins::PluginSkillRoot; use std::collections::HashSet; use tokio::sync::Mutex; @@ -83,34 +84,34 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( sess: &Session, turn_context: &TurnContext, command: &str, - workdir: &AbsolutePathBuf, + workdir: &PathUri, + native_workdir: Option<&AbsolutePathBuf>, + environment_id: &str, ) { - let Some(candidate) = detect_implicit_skill_invocation_for_command( - turn_context.skills_snapshot().outcome(), + let Some(invocation) = detect_implicit_skill_invocation( + turn_context.extension_data.as_ref(), + environment_id, command, workdir, + native_workdir, ) else { return; }; - let skill_scope = match candidate.scope { - SkillScope::User => "user", - SkillScope::Repo => "repo", - SkillScope::System => "system", - SkillScope::Admin => "admin", - }; - let skill_path = candidate.path_to_skills_md.to_string_lossy().into_owned(); - let invocation = SkillInvocation { - skill_name: candidate.name, - location: SkillInvocationLocation::Host { - path: candidate.path_to_skills_md.to_path_buf(), - scope: candidate.scope, - }, - plugin_id: candidate.plugin_id, - remote_plugin_id: candidate.remote_plugin_id, - invocation_type: InvocationType::Implicit, - }; let skill_name = invocation.skill_name.clone(); - let seen_key = format!("{skill_scope}:{skill_path}:{skill_name}"); + let (skill_resource, seen_key) = match &invocation.location { + SkillInvocationLocation::Host { path, scope } => { + let skill_scope = match scope { + SkillScope::User => "user", + SkillScope::Repo => "repo", + SkillScope::System => "system", + SkillScope::Admin => "admin", + }; + let skill_path = path.to_string_lossy().into_owned(); + let seen_key = format!("{skill_scope}:{skill_path}:{skill_name}"); + (skill_path, seen_key) + } + SkillInvocationLocation::Resource { id, .. } => (id.clone(), format!("resource:{id}")), + }; let inserted = { let skill_invocations = turn_context .extension_data @@ -130,7 +131,7 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( thread_store: &sess.services.thread_extension_data, turn_store: turn_context.extension_data.as_ref(), turn_id: turn_context.sub_id.as_str(), - skill_resource: skill_path.as_str(), + skill_resource: skill_resource.as_str(), kind: SkillInvocationKind::Implicit, }) .await; diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index 18605c1545..f517007082 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -1,7 +1,9 @@ +use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_protocol::models::ShellCommandToolCallParams; use codex_tools::ShellCommandBackendConfig; use codex_tools::ToolName; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use crate::exec::ExecCapturePolicy; use crate::exec::ExecParams; @@ -208,7 +210,9 @@ impl ShellCommandHandler { session.as_ref(), turn.as_ref(), ¶ms.command, - &cwd, + &PathUri::from_abs_path(&cwd), + Some(&cwd), + LOCAL_ENVIRONMENT_ID, ) .await; let prefix_rule = params.prefix_rule.clone(); diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 0829e5f07e..08aacf9340 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -191,16 +191,15 @@ impl ExecCommandHandler { let sandbox_permissions = resolve_sandbox_permissions(args.sandbox_permissions, args.justification.as_deref())?; let hook_command = args.cmd.clone(); - // TODO(anp) wire PathUri through implicit skills instead of skipping on foreign paths - if let Some(native_cwd) = native_cwd.as_ref() { - maybe_emit_implicit_skill_invocation( - session.as_ref(), - context.step_context.turn.as_ref(), - &hook_command, - native_cwd, - ) - .await; - } + maybe_emit_implicit_skill_invocation( + session.as_ref(), + context.step_context.turn.as_ref(), + &hook_command, + &cwd, + native_cwd.as_ref(), + &turn_environment.environment_id, + ) + .await; let shell_mode = shell_mode_for_environment(&turn.unified_exec_shell_mode, environment.as_ref()); // Remote environments may use a different OS and must build commands with their native diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 3c0131ede4..d1a8d27884 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -9,6 +9,7 @@ use codex_config::ConfigRequirementsToml; use codex_core::StartThreadOptions; use codex_core::config::Config; use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ExtensionWarning; @@ -51,6 +52,8 @@ use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; use core_test_support::responses::sse; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_remote; +use core_test_support::skip_if_target_windows; use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_mcp_server; @@ -1257,6 +1260,106 @@ async fn production_turn_aliases_executor_skill_roots() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn executor_skill_invocation_is_environment_scoped_and_deduplicated() -> Result<()> { + skip_if_target_windows!(Ok(()), "executes a POSIX cat command"); + skip_if_remote!(Ok(()), "executor fixture uses a host-local skill path"); + skip_if_no_network!(Ok(())); + + const SELECTED_RESOURCE: &str = "skill://selected-root/demo/SKILL.md"; + + let server = responses::start_mock_server().await; + let codex_home = Arc::new(TempDir::new()?); + let skill_path = codex_home.path().join("executor-skill/SKILL.md"); + std::fs::create_dir_all( + skill_path + .parent() + .expect("skill path should have a parent"), + )?; + std::fs::write(&skill_path, "executor skill contents\n")?; + let skill_uri = PathUri::from_host_native_path(&skill_path)?; + let catalog = SkillCatalog { + entries: vec![ + SkillCatalogEntry::new( + SkillPackageId("skill://other-root/demo".to_string()), + SkillAuthority::new(SkillSourceKind::Executor, "other-root"), + "other-environment-skill", + "Skill from another environment.", + SkillResourceId::environment( + "skill://other-root/demo/SKILL.md", + "other-environment", + skill_uri.clone(), + ), + ), + SkillCatalogEntry::new( + SkillPackageId("skill://selected-root/demo".to_string()), + SkillAuthority::new(SkillSourceKind::Executor, "selected-root"), + "selected-environment-skill", + "Skill from the selected environment.", + SkillResourceId::environment(SELECTED_RESOURCE, LOCAL_ENVIRONMENT_ID, skill_uri), + ), + ], + warnings: Vec::new(), + }; + let command = json!({ + "cmd": format!("cat {}", skill_path.display()), + "login": false, + }) + .to_string(); + let response = responses::mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + responses::ev_function_call("executor-skill-first", "exec_command", &command), + responses::ev_function_call("executor-skill-again", "exec_command", &command), + ev_completed("resp-1"), + ]), + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + ], + ) + .await; + + let (extensions, _) = catalog_extensions(catalog, /*include_host_provider*/ false); + let chatgpt_base_url = server.uri(); + let mut builder = test_codex() + .with_home(codex_home) + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_extensions(extensions) + .with_config(move |config| { + configure_catalog_test(config); + config.chatgpt_base_url = chatgpt_base_url; + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::UnifiedExec) + .expect("unified exec should be configurable in tests"); + }); + let test = builder.build_with_auto_env(&server).await?; + test.submit_turn("Read the executor skill twice.").await?; + + for call_id in ["executor-skill-first", "executor-skill-again"] { + let output = response + .function_call_output_text(call_id) + .expect("executor skill command should return output"); + assert!( + output.contains("executor skill contents"), + "command output: {output}" + ); + } + + let events = wait_for_analytics_events(&server, "skill_invocation", /*expected_count*/ 1).await; + assert_eq!(events.len(), 1, "executor skill should be counted once"); + assert_eq!(events[0]["skill_name"], "selected-environment-skill"); + assert_eq!( + events[0]["skill_id"], + format!("{:x}", sha1::Sha1::digest(SELECTED_RESOURCE.as_bytes())) + ); + assert_eq!(events[0]["event_params"]["invoke_type"], "implicit"); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn production_turn_aliases_combined_skill_catalogs_under_shared_budget() -> Result<()> { const EXECUTOR_ROOT: &str = diff --git a/codex-rs/ext/skills/src/invocation.rs b/codex-rs/ext/skills/src/invocation.rs new file mode 100644 index 0000000000..8f97cca02f --- /dev/null +++ b/codex-rs/ext/skills/src/invocation.rs @@ -0,0 +1,87 @@ +use codex_analytics::InvocationType; +use codex_analytics::SkillInvocation; +use codex_analytics::SkillInvocationLocation; +use codex_exec_server::LOCAL_ENVIRONMENT_ID; +use codex_extension_api::ExtensionData; +use codex_skills::ImplicitSkillAccess; +use codex_skills::detect_implicit_skill_invocation_for_command; +use codex_skills::implicit_skill_accesses_for_command; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; + +use crate::HostSkillsSnapshot; +use crate::catalog::SkillSourceKind; +use crate::state::ExecutorSkillsStepState; + +/// Identifies the executor-owned or host-owned skill referenced by a command. +pub fn detect_implicit_skill_invocation( + turn_store: &ExtensionData, + environment_id: &str, + command: &str, + workdir: &PathUri, + native_workdir: Option<&AbsolutePathBuf>, +) -> Option { + if environment_id == LOCAL_ENVIRONMENT_ID + && let Some(native_workdir) = native_workdir + && let Some(host_snapshot) = turn_store.get::() + && let Some(skill) = detect_implicit_skill_invocation_for_command( + host_snapshot.outcome(), + command, + native_workdir, + ) + { + return Some(SkillInvocation { + skill_name: skill.name, + location: SkillInvocationLocation::Host { + path: skill.path_to_skills_md.to_path_buf(), + scope: skill.scope, + }, + plugin_id: skill.plugin_id, + remote_plugin_id: skill.remote_plugin_id, + invocation_type: InvocationType::Implicit, + }); + } + + let catalog = turn_store.get::()?; + for access in implicit_skill_accesses_for_command(command, workdir) { + for entry in &catalog.0.entries { + let Some((skill_environment_id, skill_path)) = entry.main_prompt.environment_path() + else { + continue; + }; + if !entry.enabled + || entry.authority.kind != SkillSourceKind::Executor + || skill_environment_id != environment_id + { + continue; + } + + let matches = match &access { + ImplicitSkillAccess::Document(path) => path == skill_path, + ImplicitSkillAccess::Script(path) => skill_path + .parent() + .and_then(|skill_dir| skill_dir.join("scripts").ok()) + .is_some_and(|scripts_dir| path.starts_with(&scripts_dir)), + }; + if matches { + return Some(SkillInvocation { + skill_name: entry.name.clone(), + location: SkillInvocationLocation::Resource { + id: entry.main_prompt.as_str().to_owned(), + skill_id: entry.canonical_skill_id.clone(), + scope: entry.analytics_scope, + }, + plugin_id: None, + remote_plugin_id: None, + invocation_type: InvocationType::Implicit, + }); + } + } + } + + None +} + +#[cfg(test)] +#[path = "invocation_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/invocation_tests.rs b/codex-rs/ext/skills/src/invocation_tests.rs new file mode 100644 index 0000000000..30a88bc117 --- /dev/null +++ b/codex-rs/ext/skills/src/invocation_tests.rs @@ -0,0 +1,69 @@ +use super::*; +use crate::catalog::SkillAuthority; +use crate::catalog::SkillCatalog; +use crate::catalog::SkillCatalogEntry; +use crate::catalog::SkillPackageId; +use crate::catalog::SkillResourceId; + +fn detect_executor_skill(command: &str, environments: &[&str]) -> Option { + let skill_path = PathUri::parse("file:///skills/demo/SKILL.md").expect("valid skill URI"); + let catalog = SkillCatalog { + entries: environments + .iter() + .map(|environment| { + SkillCatalogEntry::new( + SkillPackageId(format!("skill://{environment}/demo")), + SkillAuthority::new(SkillSourceKind::Executor, *environment), + format!("{environment}-skill"), + "test skill", + SkillResourceId::environment( + format!("skill://{environment}/demo/SKILL.md"), + *environment, + skill_path.clone(), + ), + ) + }) + .collect(), + warnings: Vec::new(), + }; + let turn_store = ExtensionData::new("test-turn"); + turn_store.insert(ExecutorSkillsStepState(catalog)); + + detect_implicit_skill_invocation( + &turn_store, + "executor-a", + command, + &PathUri::parse("file:///skills").expect("valid workdir URI"), + /*native_workdir*/ None, + ) +} + +#[test] +fn detects_executor_skill_document_reads() { + let invocation = detect_executor_skill("cat demo/SKILL.md", &["executor-a"]) + .expect("document read should identify the executor skill"); + + assert_eq!(invocation.skill_name, "executor-a-skill"); + assert!(matches!( + invocation.location, + SkillInvocationLocation::Resource { id, .. } + if id == "skill://executor-a/demo/SKILL.md" + )); +} + +#[test] +fn detects_executor_skill_script_runs() { + let invocation = detect_executor_skill("python demo/scripts/run.py", &["executor-a"]) + .expect("script execution should identify the executor skill"); + + assert_eq!(invocation.skill_name, "executor-a-skill"); +} + +#[test] +fn ignores_matching_paths_from_other_environments() { + let invocation = detect_executor_skill("cat demo/SKILL.md", &["executor-b", "executor-a"]) + .expect("matching path should resolve to the execution environment"); + + assert_eq!(invocation.skill_name, "executor-a-skill"); + assert!(detect_executor_skill("cat demo/SKILL.md", &["executor-b"]).is_none()); +} diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 1495ac69c3..940aa9657c 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -11,6 +11,7 @@ mod host_prompt; mod host_roots; mod host_service; mod host_snapshot; +mod invocation; mod loader; pub mod provider; mod render; @@ -35,6 +36,7 @@ pub use host_service::HostSkillsLoadInput; pub use host_service::HostSkillsRequest; pub use host_service::HostSkillsService; pub use host_snapshot::HostSkillsSnapshot; +pub use invocation::detect_implicit_skill_invocation; pub use provider::ExecutorSkillProvider; pub use provider::HostSkillProvider; pub use provider::OrchestratorSkillProvider; diff --git a/codex-rs/skills/src/invocation.rs b/codex-rs/skills/src/invocation.rs index ca686cf86e..b762fa24d5 100644 --- a/codex-rs/skills/src/invocation.rs +++ b/codex-rs/skills/src/invocation.rs @@ -3,9 +3,18 @@ use std::path::Path; use codex_protocol::parse_command::ParsedCommand; use codex_shell_command::parse_command::parse_command_impl; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathConvention; +use codex_utils_path_uri::PathUri; use crate::SkillMetadata; +/// A skill document read or script execution identified in a shell command. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ImplicitSkillAccess { + Document(PathUri), + Script(PathUri), +} + /// Provides the indexed skill lookups used to recognize implicit invocations. pub trait ImplicitSkillLookup { fn implicit_skill_for_scripts_dir(&self, path: &AbsolutePathBuf) -> Option<&SkillMetadata>; @@ -28,6 +37,47 @@ pub fn detect_implicit_skill_invocation_for_command( detect_skill_doc_read(outcome, tokens.as_slice(), &workdir) } +/// Resolves statically recognizable skill accesses without consulting the host filesystem. +pub fn implicit_skill_accesses_for_command( + command: &str, + workdir: &PathUri, +) -> Vec { + // Normalize Windows paths and recognize PowerShell reads using existing cat parsing. + let tokens = if workdir.infer_path_convention() == Some(PathConvention::Windows) { + let mut tokens = tokenize_command(&command.replace('\\', "/")); + + if let Some(executable) = tokens.first_mut() + && matches!( + executable.to_ascii_lowercase().as_str(), + "get-content" | "gc" | "type" + ) + { + *executable = "cat".to_owned(); + } + + tokens + } else { + tokenize_command(command) + }; + let mut accesses = Vec::new(); + if let Some(script) = script_run_token(&tokens) + && let Ok(path) = workdir.join(script) + { + accesses.push(ImplicitSkillAccess::Script(path)); + } + + for parsed in parse_command_impl(&tokens) { + if let ParsedCommand::Read { path, .. } = parsed + && let Some(path) = path.to_str() + && let Ok(path) = workdir.join(path) + { + accesses.push(ImplicitSkillAccess::Document(path)); + } + } + + accesses +} + fn tokenize_command(command: &str) -> Vec { shlex::split(command) .unwrap_or_else(|| command.split_whitespace().map(str::to_string).collect()) diff --git a/codex-rs/skills/src/lib.rs b/codex-rs/skills/src/lib.rs index df739fa3dd..7b19d32788 100644 --- a/codex-rs/skills/src/lib.rs +++ b/codex-rs/skills/src/lib.rs @@ -10,8 +10,10 @@ mod selection; pub use interface::SkillInterfaceAssetPolicy; pub use interface::SkillInterfaceFile; pub use interface::resolve_skill_interface; +pub use invocation::ImplicitSkillAccess; pub use invocation::ImplicitSkillLookup; pub use invocation::detect_implicit_skill_invocation_for_command; +pub use invocation::implicit_skill_accesses_for_command; pub use loading::LoadedSkillRoot; pub use loading::LoadedSkills; pub use loading::SkillError;