mirror of
https://github.com/openai/codex.git
synced 2026-11-08 20:40:33 +00:00
Fix multi-env skill identity handling
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
use super::*;
|
||||
use crate::SkillsManager;
|
||||
use crate::config::ConfigBuilder;
|
||||
use crate::skills_load_input_from_config;
|
||||
use crate::skills_load_input;
|
||||
use codex_config::ConfigLayerStackOrdering;
|
||||
use codex_core_plugins::PluginsManager;
|
||||
use codex_protocol::config_types::ServiceTier;
|
||||
@@ -425,10 +425,12 @@ enabled = false
|
||||
Arc::clone(&codex_exec_server::LOCAL_FS),
|
||||
config.cwd.clone(),
|
||||
);
|
||||
let skills_input = skills_load_input_from_config(
|
||||
let skills_input = skills_load_input(
|
||||
&config,
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
cwd,
|
||||
vec![codex_core_skills::loader::SkillEnvironment {
|
||||
environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
path: cwd,
|
||||
}],
|
||||
Some(Arc::clone(&codex_exec_server::LOCAL_FS)),
|
||||
effective_skill_roots,
|
||||
);
|
||||
|
||||
@@ -95,7 +95,7 @@ pub(crate) use skills::default_skill_metadata_budget;
|
||||
pub(crate) use skills::injection;
|
||||
pub(crate) use skills::manager;
|
||||
pub(crate) use skills::maybe_emit_implicit_skill_invocation;
|
||||
pub(crate) use skills::skills_load_input_from_config;
|
||||
pub(crate) use skills::skills_load_input;
|
||||
mod stream_events_utils;
|
||||
pub mod test_support;
|
||||
mod unified_exec;
|
||||
|
||||
@@ -36,7 +36,6 @@ use crate::path_utils::normalize_for_native_workdir;
|
||||
use crate::realtime_conversation::RealtimeConversationManager;
|
||||
use crate::session_prefix::format_subagent_notification_message;
|
||||
use crate::skills::SkillRenderSideEffects;
|
||||
use crate::skills_load_input_from_config;
|
||||
use crate::turn_metadata::TurnMetadataState;
|
||||
use crate::turn_timing::now_unix_timestamp_ms;
|
||||
use async_channel::Receiver;
|
||||
@@ -2744,7 +2743,9 @@ impl Session {
|
||||
let available_skills = build_available_skills(
|
||||
&turn_context.turn_skills.outcome,
|
||||
default_skill_metadata_budget(turn_context.model_info.context_window),
|
||||
if turn_context.environments.turn_environments.len() > 1 {
|
||||
if turn_context.environments.turn_environments.len() > 1
|
||||
|| turn_context.turn_skills.outcome.has_multiple_environments()
|
||||
{
|
||||
crate::skills::render::SkillEnvironmentRender::Include
|
||||
} else {
|
||||
crate::skills::render::SkillEnvironmentRender::Omit
|
||||
|
||||
@@ -3,6 +3,7 @@ use super::*;
|
||||
use crate::config::ConstraintError;
|
||||
use crate::goals::GoalRuntimeState;
|
||||
use crate::skills::SkillError;
|
||||
use crate::skills_load_input;
|
||||
use crate::state::ActiveTurn;
|
||||
use codex_protocol::SessionId;
|
||||
use codex_protocol::config_types::SERVICE_TIER_DEFAULT_REQUEST_VALUE;
|
||||
@@ -473,10 +474,12 @@ async fn warm_plugins_and_skills_for_session_init(
|
||||
let plugins_input = config.plugins_config_input();
|
||||
let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await;
|
||||
let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots();
|
||||
let skills_input = skills_load_input_from_config(
|
||||
let skills_input = skills_load_input(
|
||||
config.as_ref(),
|
||||
environment_id,
|
||||
path_ref,
|
||||
vec![codex_core_skills::loader::SkillEnvironment {
|
||||
environment_id,
|
||||
path: path_ref,
|
||||
}],
|
||||
local_file_system,
|
||||
effective_skill_roots,
|
||||
);
|
||||
|
||||
@@ -8,6 +8,7 @@ use crate::context::TurnAborted;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::shell::default_user_shell;
|
||||
use crate::skills::SkillRenderSideEffects;
|
||||
use crate::skills::render::SkillEnvironmentRender;
|
||||
use crate::skills::render::SkillMetadataBudget;
|
||||
use crate::test_support::models_manager_with_provider;
|
||||
use crate::tools::format_exec_output_str;
|
||||
@@ -4127,13 +4128,15 @@ async fn new_default_turn_uses_config_aware_skills_for_role_overrides() {
|
||||
.services
|
||||
.skills_manager
|
||||
.skills_for_cwd(
|
||||
&crate::skills_load_input_from_config(
|
||||
&crate::skills_load_input(
|
||||
&parent_config,
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
crate::skills::EnvironmentPathRef::new(
|
||||
Arc::clone(&skill_fs),
|
||||
parent_config.cwd.clone(),
|
||||
),
|
||||
vec![codex_core_skills::loader::SkillEnvironment {
|
||||
environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
path: crate::skills::EnvironmentPathRef::new(
|
||||
Arc::clone(&skill_fs),
|
||||
parent_config.cwd.clone(),
|
||||
),
|
||||
}],
|
||||
session
|
||||
.services
|
||||
.environment_manager
|
||||
@@ -4708,10 +4711,12 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
environment.get_filesystem(),
|
||||
per_turn_config.cwd.clone(),
|
||||
);
|
||||
let skills_input = crate::skills_load_input_from_config(
|
||||
let skills_input = crate::skills_load_input(
|
||||
&per_turn_config,
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
cwd,
|
||||
vec![codex_core_skills::loader::SkillEnvironment {
|
||||
environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
path: cwd,
|
||||
}],
|
||||
services
|
||||
.environment_manager
|
||||
.try_local_environment()
|
||||
@@ -6562,10 +6567,12 @@ where
|
||||
environment.get_filesystem(),
|
||||
per_turn_config.cwd.clone(),
|
||||
);
|
||||
let skills_input = crate::skills_load_input_from_config(
|
||||
let skills_input = crate::skills_load_input(
|
||||
&per_turn_config,
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
cwd,
|
||||
vec![codex_core_skills::loader::SkillEnvironment {
|
||||
environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
path: cwd,
|
||||
}],
|
||||
services
|
||||
.environment_manager
|
||||
.try_local_environment()
|
||||
@@ -7302,6 +7309,68 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_initial_context_qualifies_skills_only_when_skill_envs_are_ambiguous() {
|
||||
let (session, mut turn_context) = make_session_and_context().await;
|
||||
let make_skill = |name: &str, environment_id: &str| SkillMetadata {
|
||||
name: name.to_string(),
|
||||
description: "desc".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: test_path_buf(&format!("/tmp/{name}/SKILL.md"))
|
||||
.abs()
|
||||
.clone(),
|
||||
source_path: codex_exec_server::EnvironmentPathRef::local(
|
||||
test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(),
|
||||
),
|
||||
environment_id: environment_id.to_string(),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
};
|
||||
|
||||
let mut single_env_outcome = SkillLoadOutcome::default();
|
||||
single_env_outcome.skills = vec![make_skill("repo-skill", "local")];
|
||||
turn_context.turn_skills = TurnSkillsContext::new(Arc::new(single_env_outcome));
|
||||
let single_env_context = session.build_initial_context(&turn_context).await;
|
||||
let single_env_texts = developer_input_texts(&single_env_context);
|
||||
assert!(
|
||||
single_env_texts
|
||||
.iter()
|
||||
.any(|text| text.contains("- repo-skill: desc (file: /tmp/repo-skill/SKILL.md)"))
|
||||
);
|
||||
assert!(single_env_texts.iter().all(|text| !text.contains("env:")));
|
||||
|
||||
let mut secondary_environment = turn_context.environments.turn_environments[0].clone();
|
||||
secondary_environment.environment_id = "devbox".to_string();
|
||||
turn_context
|
||||
.environments
|
||||
.turn_environments
|
||||
.push(secondary_environment);
|
||||
let multi_turn_env_context = session.build_initial_context(&turn_context).await;
|
||||
let multi_turn_env_texts = developer_input_texts(&multi_turn_env_context);
|
||||
assert!(multi_turn_env_texts.iter().any(|text| {
|
||||
text.contains("- repo-skill: desc (env: local, file: /tmp/repo-skill/SKILL.md)")
|
||||
}));
|
||||
turn_context.environments.turn_environments.truncate(1);
|
||||
|
||||
let mut mixed_env_outcome = SkillLoadOutcome::default();
|
||||
mixed_env_outcome.skills = vec![
|
||||
make_skill("repo-skill", "devbox"),
|
||||
make_skill("user-skill", "local"),
|
||||
];
|
||||
turn_context.turn_skills = TurnSkillsContext::new(Arc::new(mixed_env_outcome));
|
||||
let mixed_env_context = session.build_initial_context(&turn_context).await;
|
||||
let mixed_env_texts = developer_input_texts(&mixed_env_context);
|
||||
assert!(mixed_env_texts.iter().any(|text| {
|
||||
text.contains("- repo-skill: desc (env: devbox, file: /tmp/repo-skill/SKILL.md)")
|
||||
}));
|
||||
assert!(mixed_env_texts.iter().any(|text| {
|
||||
text.contains("- user-skill: desc (env: local, file: /tmp/user-skill/SKILL.md)")
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() {
|
||||
let session_telemetry = test_session_telemetry_without_metadata();
|
||||
|
||||
@@ -533,7 +533,6 @@ async fn build_skills_and_plugins(
|
||||
warnings: skill_warnings,
|
||||
} = build_skill_injections(
|
||||
&mentioned_skills,
|
||||
Some(skills_outcome),
|
||||
Some(&turn_context.session_telemetry),
|
||||
&sess.services.analytics_events_client,
|
||||
tracking.clone(),
|
||||
|
||||
@@ -706,7 +706,7 @@ impl Session {
|
||||
.plugins_for_config(&plugins_input)
|
||||
.await;
|
||||
let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots();
|
||||
let skills_input = crate::skills::skills_load_input_from_environments(
|
||||
let skills_input = crate::skills::skills_load_input(
|
||||
&per_turn_config,
|
||||
skill_env_paths,
|
||||
local_file_system,
|
||||
|
||||
+10
-22
@@ -36,26 +36,7 @@ pub use codex_core_skills::render;
|
||||
pub use codex_core_skills::render::SkillRenderSideEffects;
|
||||
pub use codex_core_skills::system;
|
||||
|
||||
pub(crate) fn skills_load_input_from_config(
|
||||
config: &Config,
|
||||
environment_id: String,
|
||||
env_path: EnvironmentPathRef,
|
||||
local_file_system: Option<Arc<dyn ExecutorFileSystem>>,
|
||||
effective_skill_roots: Vec<PluginSkillRoot>,
|
||||
) -> SkillsLoadInput {
|
||||
SkillsLoadInput::new(
|
||||
vec![codex_core_skills::loader::SkillEnvironment {
|
||||
environment_id,
|
||||
path: env_path,
|
||||
}],
|
||||
local_file_system,
|
||||
effective_skill_roots,
|
||||
config.config_layer_stack.clone(),
|
||||
config.bundled_skills_enabled(),
|
||||
)
|
||||
}
|
||||
|
||||
pub(crate) fn skills_load_input_from_environments(
|
||||
pub(crate) fn skills_load_input(
|
||||
config: &Config,
|
||||
env_paths: Vec<codex_core_skills::loader::SkillEnvironment>,
|
||||
local_file_system: Option<Arc<dyn ExecutorFileSystem>>,
|
||||
@@ -76,13 +57,20 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
|
||||
command: &str,
|
||||
workdir: &AbsolutePathBuf,
|
||||
) {
|
||||
let Some(turn_environment) = turn_context.environments.primary() else {
|
||||
return;
|
||||
};
|
||||
let Some(candidate) = detect_implicit_skill_invocation_for_command(
|
||||
turn_context.turn_skills.outcome.as_ref(),
|
||||
command,
|
||||
workdir,
|
||||
&EnvironmentPathRef::new(
|
||||
turn_environment.environment.get_filesystem(),
|
||||
workdir.clone(),
|
||||
),
|
||||
) else {
|
||||
return;
|
||||
};
|
||||
let environment_id = candidate.environment_id.clone();
|
||||
let invocation = SkillInvocation {
|
||||
skill_name: candidate.name,
|
||||
skill_scope: candidate.scope,
|
||||
@@ -98,7 +86,7 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
|
||||
};
|
||||
let skill_path = invocation.skill_path.to_string_lossy();
|
||||
let skill_name = invocation.skill_name.clone();
|
||||
let seen_key = format!("{skill_scope}:{skill_path}:{skill_name}");
|
||||
let seen_key = format!("{environment_id}:{skill_scope}:{skill_path}:{skill_name}");
|
||||
let inserted = {
|
||||
let mut seen_skills = turn_context
|
||||
.turn_skills
|
||||
|
||||
Reference in New Issue
Block a user