mirror of
https://github.com/openai/codex.git
synced 2026-11-04 08:25:29 +00:00
Move skill catalog rendering out of core (#36364)
## What changed - Make the skills extension own the catalog prompt templates and rendering path. - Remove the duplicate core fallback that injected available skills into initial context. - Update core integration tests to install the skills extension explicitly when they expect model-visible skill context. GitOrigin-RevId: bbe6b15c4e776a09c98b7f4166426fe4e58434ba
This commit is contained in:
1 parent
da2c7ca8d1
commit
0d109f097c
21 files changed
+161
-2167
No files matched your search
@@ -1,4 +1,3 @@
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_config::types::AuthCredentialsStoreMode;
|
||||
use codex_core::ModelClient;
|
||||
use codex_core::NewThread;
|
||||
@@ -54,7 +53,6 @@ use codex_protocol::protocol::SessionMeta;
|
||||
use codex_protocol::protocol::SessionMetaLine;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::TestCodexResponsesRequestKind;
|
||||
use core_test_support::apps_test_server::AppsTestServer;
|
||||
use core_test_support::load_default_config_for_test;
|
||||
@@ -79,7 +77,6 @@ use core_test_support::test_codex::TestCodex;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_event;
|
||||
use dunce::canonicalize as normalize_path;
|
||||
use futures::StreamExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
@@ -87,7 +84,6 @@ use std::io::Write;
|
||||
use std::num::NonZeroU64;
|
||||
use std::sync::Arc;
|
||||
use tempfile::TempDir;
|
||||
use toml::toml;
|
||||
use uuid::Uuid;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
@@ -2204,172 +2200,6 @@ async fn omits_environment_context_when_configured_off() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn skills_append_to_developer_message() {
|
||||
skip_if_no_network!();
|
||||
let server = MockServer::start().await;
|
||||
|
||||
let resp_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp1"), ev_completed("resp1")]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let codex_home = Arc::new(TempDir::new().unwrap());
|
||||
let skill_dir = codex_home.path().join("skills/demo");
|
||||
std::fs::create_dir_all(&skill_dir).expect("create skill dir");
|
||||
std::fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
"---\nname: demo\ndescription: build charts\n---\n\n# body\n",
|
||||
)
|
||||
.expect("write skill");
|
||||
|
||||
let codex_home_path = codex_home.path().to_path_buf();
|
||||
let mut builder = test_codex()
|
||||
.with_home(codex_home.clone())
|
||||
.with_auth(CodexAuth::from_api_key("Test API Key"))
|
||||
.with_config(move |config| {
|
||||
config.cwd = codex_home_path.abs();
|
||||
});
|
||||
let codex = builder
|
||||
.build(&server)
|
||||
.await
|
||||
.expect("create new conversation")
|
||||
.codex;
|
||||
|
||||
codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "hello".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: Default::default(),
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
|
||||
|
||||
let request = resp_mock.single_request();
|
||||
let developer_messages = request.message_input_texts("developer");
|
||||
let developer_text = developer_messages.join("\n\n");
|
||||
assert!(
|
||||
developer_text.contains("## Skills"),
|
||||
"expected skills section present: {developer_messages:?}"
|
||||
);
|
||||
assert!(
|
||||
developer_text.contains("demo: build charts"),
|
||||
"expected skill summary: {developer_messages:?}"
|
||||
);
|
||||
assert!(
|
||||
developer_text.find("demo: build charts")
|
||||
< developer_text.find("<permissions instructions>"),
|
||||
"host skills should precede permissions: {developer_messages:?}"
|
||||
);
|
||||
let expected_path = normalize_path(skill_dir.join("SKILL.md")).unwrap();
|
||||
let expected_path_str = expected_path.to_string_lossy().replace('\\', "/");
|
||||
assert!(
|
||||
developer_text.contains(&expected_path_str),
|
||||
"expected path {expected_path_str} in developer message: {developer_messages:?}"
|
||||
);
|
||||
let _codex_home_guard = codex_home;
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn skills_use_aliases_in_developer_message_under_budget_pressure() {
|
||||
skip_if_no_network!();
|
||||
let server = MockServer::start().await;
|
||||
|
||||
let resp_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp1"), ev_completed("resp1")]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let codex_home_parent = TempDir::new().unwrap();
|
||||
let long_home_parent = codex_home_parent
|
||||
.path()
|
||||
.join("codex-home-with-long-shared-prefix-for-skill-alias-budget-test");
|
||||
std::fs::create_dir_all(&long_home_parent).expect("create long home parent");
|
||||
let codex_home = Arc::new(TempDir::new_in(long_home_parent).unwrap());
|
||||
let skill_root = codex_home.path().join("skills");
|
||||
for index in 0..12 {
|
||||
let skill_dir = skill_root.join(format!("s{index:02}"));
|
||||
std::fs::create_dir_all(&skill_dir).expect("create skill dir");
|
||||
std::fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
format!("---\nname: s{index:02}\ndescription: d\n---\n\n# body\n"),
|
||||
)
|
||||
.expect("write skill");
|
||||
}
|
||||
|
||||
let codex_home_path = codex_home.path().to_path_buf();
|
||||
let mut builder = test_codex()
|
||||
.with_home(codex_home.clone())
|
||||
.with_auth(CodexAuth::from_api_key("Test API Key"))
|
||||
.with_config(move |config| {
|
||||
config.cwd = codex_home_path.abs();
|
||||
let user_config_path = codex_home_path.join("config.toml").abs();
|
||||
config.config_layer_stack = ConfigLayerStack::default()
|
||||
.with_user_config(
|
||||
&user_config_path,
|
||||
toml! { skills = { bundled = { enabled = false } } }.into(),
|
||||
)
|
||||
.expect("skills user config should be valid");
|
||||
config.model_context_window = Some(12_000);
|
||||
});
|
||||
let codex = builder
|
||||
.build(&server)
|
||||
.await
|
||||
.expect("create new conversation")
|
||||
.codex;
|
||||
|
||||
codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "hello".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: Default::default(),
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
|
||||
|
||||
let request = resp_mock.single_request();
|
||||
let developer_messages = request.message_input_texts("developer");
|
||||
let developer_text = developer_messages.join("\n\n");
|
||||
let expected_root = normalize_path(skill_root).unwrap();
|
||||
let expected_root_str = expected_root.to_string_lossy().replace('\\', "/");
|
||||
assert!(
|
||||
developer_text.contains("### Skill roots"),
|
||||
"expected aliased skills root section: {developer_messages:?}"
|
||||
);
|
||||
assert!(
|
||||
developer_text.contains(&format!("- `r0` = `{expected_root_str}`")),
|
||||
"expected root alias for {expected_root_str}: {developer_messages:?}"
|
||||
);
|
||||
assert!(
|
||||
developer_text.contains("- s00: d (file: r0/s00/SKILL.md)"),
|
||||
"expected skill path to use root alias: {developer_messages:?}"
|
||||
);
|
||||
assert!(
|
||||
developer_text.contains(
|
||||
"expand the listed short `path` with the matching alias from `### Skill roots`"
|
||||
),
|
||||
"expected alias-specific skill instructions: {developer_messages:?}"
|
||||
);
|
||||
let _codex_home_guard = codex_home;
|
||||
let _codex_home_parent_guard = codex_home_parent;
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn includes_configured_max_effort_in_request() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -4,7 +4,10 @@ use std::sync::Arc;
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_config::types::Personality;
|
||||
use codex_core::config::Config;
|
||||
use codex_exec_server::LOCAL_ENVIRONMENT_ID;
|
||||
use codex_extension_api::ExtensionRegistry;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
@@ -13,6 +16,8 @@ use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelections;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_skills_extension::SkillsExtensionConfig;
|
||||
use codex_skills_extension::install;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::context_snapshot;
|
||||
@@ -34,6 +39,17 @@ use serde_json::json;
|
||||
|
||||
const PRETURN_CONTEXT_DIFF_CWD: &str = "PRETURN_CONTEXT_DIFF_CWD";
|
||||
|
||||
fn skills_extensions() -> Arc<ExtensionRegistry<Config>> {
|
||||
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
|
||||
install(&mut extensions, |config: &Config| SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
bundled_skills_enabled: config.bundled_skills_enabled(),
|
||||
orchestrator_skills_enabled: config.orchestrator_skills_enabled,
|
||||
shadow_selection_enabled: config.features.enabled(Feature::SkillSearch),
|
||||
});
|
||||
Arc::new(extensions.build())
|
||||
}
|
||||
|
||||
fn context_snapshot_options() -> ContextSnapshotOptions {
|
||||
ContextSnapshotOptions::default()
|
||||
.render_mode(ContextSnapshotRenderMode::KindWithTextPrefix { max_chars: 96 })
|
||||
@@ -162,13 +178,16 @@ async fn snapshot_model_visible_layout_turn_overrides() -> Result<()> {
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex().with_model("gpt-5.4").with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Personality)
|
||||
.expect("test config should allow feature update");
|
||||
config.personality = Some(Personality::Pragmatic);
|
||||
});
|
||||
let mut builder = test_codex()
|
||||
.with_extensions(skills_extensions())
|
||||
.with_model("gpt-5.4")
|
||||
.with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Personality)
|
||||
.expect("test config should allow feature update");
|
||||
config.personality = Some(Personality::Pragmatic);
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let preturn_context_diff_cwd = test.cwd_path().join(PRETURN_CONTEXT_DIFF_CWD);
|
||||
fs::create_dir_all(&preturn_context_diff_cwd)?;
|
||||
@@ -282,7 +301,9 @@ async fn snapshot_model_visible_layout_cwd_change_refreshes_agents() -> Result<(
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex().with_model("gpt-5.4");
|
||||
let mut builder = test_codex()
|
||||
.with_extensions(skills_extensions())
|
||||
.with_model("gpt-5.4");
|
||||
let test = builder.build(&server).await?;
|
||||
let cwd_one = test.cwd_path().join("agents_one");
|
||||
let cwd_two = test.cwd_path().join("agents_two");
|
||||
@@ -396,9 +417,11 @@ async fn snapshot_model_visible_layout_resume_with_personality_change() -> Resul
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let mut initial_builder = test_codex().with_config(|config| {
|
||||
config.model = Some("gpt-5.2".to_string());
|
||||
});
|
||||
let mut initial_builder = test_codex()
|
||||
.with_extensions(skills_extensions())
|
||||
.with_config(|config| {
|
||||
config.model = Some("gpt-5.2".to_string());
|
||||
});
|
||||
let initial = initial_builder.build(&server).await?;
|
||||
let codex = Arc::clone(&initial.codex);
|
||||
let home = initial.home.clone();
|
||||
@@ -442,14 +465,16 @@ async fn snapshot_model_visible_layout_resume_with_personality_change() -> Resul
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut resume_builder = test_codex().with_config(|config| {
|
||||
config.model = Some("gpt-5.4".to_string());
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Personality)
|
||||
.expect("test config should allow feature update");
|
||||
config.personality = Some(Personality::Pragmatic);
|
||||
});
|
||||
let mut resume_builder = test_codex()
|
||||
.with_extensions(skills_extensions())
|
||||
.with_config(|config| {
|
||||
config.model = Some("gpt-5.4".to_string());
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Personality)
|
||||
.expect("test config should allow feature update");
|
||||
config.personality = Some(Personality::Pragmatic);
|
||||
});
|
||||
let resumed = resume_builder.resume(&server, home, rollout_path).await?;
|
||||
let resume_override_cwd = resumed.cwd_path().join(PRETURN_CONTEXT_DIFF_CWD);
|
||||
fs::create_dir_all(&resume_override_cwd)?;
|
||||
@@ -511,9 +536,11 @@ async fn snapshot_model_visible_layout_resume_override_matches_rollout_model() -
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let mut initial_builder = test_codex().with_config(|config| {
|
||||
config.model = Some("gpt-5.2".to_string());
|
||||
});
|
||||
let mut initial_builder = test_codex()
|
||||
.with_extensions(skills_extensions())
|
||||
.with_config(|config| {
|
||||
config.model = Some("gpt-5.2".to_string());
|
||||
});
|
||||
let initial = initial_builder.build(&server).await?;
|
||||
let codex = Arc::clone(&initial.codex);
|
||||
let home = initial.home.clone();
|
||||
@@ -557,9 +584,11 @@ async fn snapshot_model_visible_layout_resume_override_matches_rollout_model() -
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut resume_builder = test_codex().with_config(|config| {
|
||||
config.model = Some("gpt-5.4".to_string());
|
||||
});
|
||||
let mut resume_builder = test_codex()
|
||||
.with_extensions(skills_extensions())
|
||||
.with_config(|config| {
|
||||
config.model = Some("gpt-5.4".to_string());
|
||||
});
|
||||
let resumed = resume_builder.resume(&server, home, rollout_path).await?;
|
||||
let resume_override_cwd = resumed.cwd_path().join(PRETURN_CONTEXT_DIFF_CWD);
|
||||
fs::create_dir_all(&resume_override_cwd)?;
|
||||
|
||||
@@ -6,7 +6,10 @@ use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_core::config::Config;
|
||||
use codex_core_plugins::store::PluginStore;
|
||||
use codex_extension_api::ExtensionRegistry;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_features::Feature;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
@@ -19,6 +22,8 @@ use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_skills_extension::SkillsExtensionConfig;
|
||||
use codex_skills_extension::install;
|
||||
use core_test_support::apps_test_server::AppsTestServer;
|
||||
use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL;
|
||||
use core_test_support::responses::ResponseMock;
|
||||
@@ -59,6 +64,17 @@ const PLUGIN_APP_SEARCH_CALL_ID: &str = "plugin-app-search";
|
||||
const PLUGIN_MCP_SEARCH_CALL_ID: &str = "plugin-mcp-search";
|
||||
const REMOTE_PLUGIN_CONFIG_NAME: &str = "sample@openai-curated-remote";
|
||||
|
||||
fn skills_extensions() -> Arc<ExtensionRegistry<Config>> {
|
||||
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
|
||||
install(&mut extensions, |config: &Config| SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
bundled_skills_enabled: config.bundled_skills_enabled(),
|
||||
orchestrator_skills_enabled: config.orchestrator_skills_enabled,
|
||||
shadow_selection_enabled: config.features.enabled(Feature::SkillSearch),
|
||||
});
|
||||
Arc::new(extensions.build())
|
||||
}
|
||||
|
||||
fn sample_plugin_root(home: &TempDir) -> std::path::PathBuf {
|
||||
home.path().join("plugins/cache/test/sample/local")
|
||||
}
|
||||
@@ -408,12 +424,18 @@ async fn capability_sections_render_in_developer_message_in_order() -> Result<()
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
write_plugin_skill_plugin(codex_home.as_ref());
|
||||
write_plugin_app_plugin(codex_home.as_ref());
|
||||
let test_codex = build_apps_enabled_plugin_test_codex(
|
||||
&server,
|
||||
Arc::clone(&codex_home),
|
||||
apps_server.chatgpt_base_url,
|
||||
)
|
||||
.await?;
|
||||
let mut builder = test_codex()
|
||||
.with_home(Arc::clone(&codex_home))
|
||||
.with_extensions(skills_extensions())
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Apps)
|
||||
.expect("test config should allow feature update");
|
||||
config.chatgpt_base_url = apps_server.chatgpt_base_url;
|
||||
});
|
||||
let test_codex = builder.build(&server).await?;
|
||||
let codex = Arc::clone(&test_codex.codex);
|
||||
|
||||
codex
|
||||
@@ -645,6 +667,7 @@ enabled = true
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_home(Arc::clone(&codex_home))
|
||||
.with_extensions(skills_extensions())
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing());
|
||||
let test_codex = builder.build_with_auto_env(&server).await?;
|
||||
let plugins_manager = test_codex.thread_manager.plugins_manager();
|
||||
|
||||
+2
-2
@@ -6,8 +6,8 @@ Scenario: New context window tool installs fresh full context before the next fo
|
||||
|
||||
## Final Follow-Up Request
|
||||
00:message/developer[3]:
|
||||
[01] <SKILLS_INSTRUCTIONS>
|
||||
[02] <context_window>\nThread id: <THREAD_ID>\nFirst context window id: <FIRST_WINDOW_ID>\nCurrent context window id: <WINDOW_ID>\nPrevious context window id: <FIRST_WINDOW_ID>\n</context_window>
|
||||
[01] <context_window>\nThread id: <THREAD_ID>\nFirst context window id: <FIRST_WINDOW_ID>\nCurrent context window id: <WINDOW_ID>\nPrevious context window id: <FIRST_WINDOW_ID>\n</context_window>
|
||||
[02] <SKILLS_INSTRUCTIONS>
|
||||
[03] <PERMISSIONS_INSTRUCTIONS>
|
||||
01:message/user:<ENVIRONMENT_CONTEXT:cwd=<CWD>>
|
||||
02:function_call/update_plan
|
||||
|
||||
@@ -2,7 +2,9 @@ use anyhow::Result;
|
||||
use codex_config::config_toml::ConfigLockfileToml;
|
||||
use codex_config::types::McpServerConfig;
|
||||
use codex_config::types::McpServerTransportConfig;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::TokenBudgetConfig;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_features::Feature;
|
||||
use codex_features::FeatureToml;
|
||||
use codex_features::TokenBudgetConfigToml;
|
||||
@@ -22,6 +24,8 @@ use codex_protocol::protocol::ItemStartedEvent;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ThreadSettingsOverrides;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_skills_extension::SkillsExtensionConfig;
|
||||
use codex_skills_extension::install;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::assert_regex_match;
|
||||
use core_test_support::context_snapshot;
|
||||
@@ -1483,7 +1487,15 @@ async fn new_context_tool_skips_auto_compact_fallback() -> Result<()> {
|
||||
],
|
||||
)
|
||||
.await;
|
||||
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
|
||||
install(&mut extensions, |config: &Config| SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
bundled_skills_enabled: config.bundled_skills_enabled(),
|
||||
orchestrator_skills_enabled: config.orchestrator_skills_enabled,
|
||||
shadow_selection_enabled: config.features.enabled(Feature::SkillSearch),
|
||||
});
|
||||
let test = test_codex()
|
||||
.with_extensions(Arc::new(extensions.build()))
|
||||
.with_config(|config| {
|
||||
config.model_context_window = Some(10_000);
|
||||
config.token_budget = Some(TokenBudgetConfig {
|
||||
|
||||
Reference in new issue
Block a user