mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Track implicit executor skill invocations (#38074)
## What changed - Detect executor-owned skill document reads and script executions across native and URI-based working directories. - Match skills only within the active execution environment while preserving host skill detection for local commands. - Emit resource-backed implicit invocation events and deduplicate repeated accesses within a turn. ## Testing - Add unit coverage for document reads, script executions, and environment-scoped matching. - Add an integration test for environment selection, resource-backed analytics, and repeated-access deduplication. GitOrigin-RevId: f78a48c3a4766fe8775bd88926b45a76761f6af8
This commit is contained in:
committed by
copyberry
parent
ba2fb48319
commit
a817d9424d
+24
-23
@@ -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;
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 =
|
||||
|
||||
@@ -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<SkillInvocation> {
|
||||
if environment_id == LOCAL_ENVIRONMENT_ID
|
||||
&& let Some(native_workdir) = native_workdir
|
||||
&& let Some(host_snapshot) = turn_store.get::<HostSkillsSnapshot>()
|
||||
&& 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::<ExecutorSkillsStepState>()?;
|
||||
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;
|
||||
@@ -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<SkillInvocation> {
|
||||
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());
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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<ImplicitSkillAccess> {
|
||||
// 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<String> {
|
||||
shlex::split(command)
|
||||
.unwrap_or_else(|| command.split_whitespace().map(str::to_string).collect())
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user