skills: decouple the skills extension from core (#27413)

## Why

`ext/skills` currently depends on `codex-core` for two host concerns:
reading the concrete `Config` type and borrowing core-owned
model-context fragment types. That coupling prevents the extension from
being assembled independently above core and leaves context that belongs
to the skills feature owned by core.

This stacked PR introduces the host boundary needed for the broader
extension migration while intentionally preserving existing skills
behavior. It is stacked on #27404.

## What changed

- Adds a small public `SkillsExtensionConfig` view and makes skills
installation generic over the host config type.
- Requires the host to map its config into that view; app-server
supplies the current `Config` values.
- Moves the available-skills and selected-skill context fragment
implementations into `ext/skills`, preserving their roles, markers, and
rendered bytes.
- Removes the direct `codex-core` dependency from
`codex-skills-extension`.
- Keeps local discovery, invocation, side effects, and the
`codex-core-skills` compatibility types unchanged for later staged PRs.

## Behavior

This adds no capability and is intended to have no user-visible or
model-visible behavior change. The install API and ownership boundary
change internally; emitted skills context remains byte-for-byte
compatible.

## Validation

- Updates the skills extension integration coverage to use a host-owned
test config.
- Asserts the complete rendered catalog and selected-skill fragments,
including their roles and markers.
- `just bazel-lock-check`
- Rust tests and Clippy were not run locally per request; CI will run
them.
This commit is contained in:
jif
2026-06-11 13:03:53 +01:00
committed by GitHub
Unverified
parent 4322e01d2b
commit 7c2394808e
10 changed files with 191 additions and 103 deletions
-1
View File
@@ -3815,7 +3815,6 @@ name = "codex-skills-extension"
version = "0.0.0"
dependencies = [
"async-trait",
"codex-core",
"codex-core-skills",
"codex-exec-server",
"codex-extension-api",
+4
View File
@@ -80,6 +80,10 @@ where
.with_orchestrator_provider(Arc::new(
codex_skills_extension::OrchestratorSkillProvider::new(),
)),
|config: &Config| codex_skills_extension::SkillsExtensionConfig {
include_instructions: config.include_skill_instructions,
bundled_skills_enabled: config.bundled_skills_enabled(),
},
);
Arc::new(builder.build())
}
-1
View File
@@ -14,7 +14,6 @@ doctest = false
workspace = true
[dependencies]
codex-core = { workspace = true }
codex-core-skills = { workspace = true }
codex-exec-server = { workspace = true }
codex-extension-api = { workspace = true }
+8
View File
@@ -0,0 +1,8 @@
/// Host-supplied configuration used by the skills extension.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct SkillsExtensionConfig {
/// Whether the available-skills catalog is included in model context.
pub include_instructions: bool,
/// Whether bundled skills are eligible for discovery.
pub bundled_skills_enabled: bool,
}
+46 -26
View File
@@ -1,10 +1,7 @@
use std::sync::Arc;
use codex_core::config::Config;
use codex_core_skills::HostLoadedSkills;
use codex_core_skills::SkillInstructions;
use codex_core_skills::injection::InjectedHostSkillPrompts;
use codex_core_skills::injection::SkillInjection;
use codex_extension_api::ConfigContributor;
use codex_extension_api::ContextContributor;
use codex_extension_api::ContextualUserFragment;
@@ -26,10 +23,12 @@ use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::WarningEvent;
use crate::SkillsExtensionConfig;
use crate::catalog::SkillCatalog;
use crate::catalog::SkillCatalogEntry;
use crate::catalog::SkillReadResult;
use crate::catalog::SkillSourceKind;
use crate::fragments::SkillInstructions;
use crate::provider::HostSkillProvider;
use crate::provider::SkillListQuery;
use crate::provider::SkillReadRequest;
@@ -40,22 +39,21 @@ use crate::render::truncate_main_prompt_contents;
use crate::render::truncate_utf8_to_bytes;
use crate::selection::collect_explicit_skill_mentions;
use crate::sources::SkillProviders;
use crate::state::SkillsExtensionConfig;
use crate::state::SkillsThreadState;
use crate::state::SkillsTurnState;
use crate::tools::skill_tools;
#[derive(Clone)]
struct SkillsExtension {
struct SkillsExtension<C> {
providers: SkillProviders,
event_sink: Arc<dyn ExtensionEventSink>,
config_from_host: Arc<dyn Fn(&C) -> SkillsExtensionConfig + Send + Sync>,
}
impl ThreadLifecycleContributor<Config> for SkillsExtension {
fn on_thread_start<'a>(
&'a self,
input: ThreadStartInput<'a, Config>,
) -> ExtensionFuture<'a, ()> {
impl<C> ThreadLifecycleContributor<C> for SkillsExtension<C>
where
C: Send + Sync + 'static,
{
fn on_thread_start<'a>(&'a self, input: ThreadStartInput<'a, C>) -> ExtensionFuture<'a, ()> {
Box::pin(async move {
let selected_roots = input
.thread_store
@@ -63,22 +61,25 @@ impl ThreadLifecycleContributor<Config> for SkillsExtension {
.map(|selected_roots| selected_roots.as_ref().clone())
.unwrap_or_default();
input.thread_store.insert(SkillsThreadState::new(
SkillsExtensionConfig::from_config(input.config),
(self.config_from_host)(input.config),
selected_roots,
));
})
}
}
impl ConfigContributor<Config> for SkillsExtension {
impl<C> ConfigContributor<C> for SkillsExtension<C>
where
C: Send + Sync + 'static,
{
fn on_config_changed(
&self,
_session_store: &ExtensionData,
thread_store: &ExtensionData,
_previous_config: &Config,
new_config: &Config,
_previous_config: &C,
new_config: &C,
) {
let next_config = SkillsExtensionConfig::from_config(new_config);
let next_config = (self.config_from_host)(new_config);
if let Some(state) = thread_store.get::<SkillsThreadState>() {
state.set_config(next_config);
} else {
@@ -87,7 +88,10 @@ impl ConfigContributor<Config> for SkillsExtension {
}
}
impl ContextContributor for SkillsExtension {
impl<C> ContextContributor for SkillsExtension<C>
where
C: Send + Sync + 'static,
{
fn contribute<'a>(
&'a self,
session_store: &'a ExtensionData,
@@ -126,7 +130,10 @@ impl ContextContributor for SkillsExtension {
}
}
impl ToolContributor for SkillsExtension {
impl<C> ToolContributor for SkillsExtension<C>
where
C: Send + Sync + 'static,
{
fn tools(
&self,
session_store: &ExtensionData,
@@ -143,7 +150,10 @@ impl ToolContributor for SkillsExtension {
}
}
impl TurnInputContributor for SkillsExtension {
impl<C> TurnInputContributor for SkillsExtension<C>
where
C: Send + Sync + 'static,
{
fn contribute<'a>(
&'a self,
input: TurnInputContext,
@@ -204,7 +214,7 @@ impl TurnInputContributor for SkillsExtension {
self.emit_warning(&input.turn_id, warning.clone());
warnings.push(warning);
}
let injection = SkillInjection {
let fragment = SkillInstructions {
name: truncate_utf8_to_bytes(&entry.name, MAX_SKILL_NAME_BYTES).0,
path: truncate_utf8_to_bytes(
entry.rendered_path(),
@@ -213,7 +223,7 @@ impl TurnInputContributor for SkillsExtension {
.0,
contents,
};
fragments.push(Box::new(SkillInstructions::from(&injection)));
fragments.push(Box::new(fragment));
main_prompts_injected = true;
if entry.authority.kind == SkillSourceKind::Host {
injected_host_skill_prompts.insert_path(entry.main_prompt.as_str());
@@ -259,7 +269,7 @@ impl TurnInputContributor for SkillsExtension {
}
}
impl SkillsExtension {
impl<C> SkillsExtension<C> {
async fn list_skills(
&self,
mut query: SkillListQuery,
@@ -308,20 +318,30 @@ impl SkillsExtension {
}
}
pub fn install(registry: &mut ExtensionRegistryBuilder<Config>) {
pub fn install<C>(
registry: &mut ExtensionRegistryBuilder<C>,
config_from_host: impl Fn(&C) -> SkillsExtensionConfig + Send + Sync + 'static,
) where
C: Send + Sync + 'static,
{
install_with_providers(
registry,
SkillProviders::new().with_host_provider(Arc::new(HostSkillProvider::new())),
config_from_host,
);
}
pub fn install_with_providers(
registry: &mut ExtensionRegistryBuilder<Config>,
pub fn install_with_providers<C>(
registry: &mut ExtensionRegistryBuilder<C>,
providers: SkillProviders,
) {
config_from_host: impl Fn(&C) -> SkillsExtensionConfig + Send + Sync + 'static,
) where
C: Send + Sync + 'static,
{
let extension = Arc::new(SkillsExtension {
providers,
event_sink: registry.event_sink(),
config_from_host: Arc::new(config_from_host),
});
registry.thread_lifecycle_contributor(extension.clone());
registry.config_contributor(extension.clone());
+61
View File
@@ -0,0 +1,61 @@
use codex_core_skills::render_available_skills_body;
use codex_extension_api::ContextualUserFragment;
use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG;
use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG;
#[derive(Clone, Debug, Eq, PartialEq)]
pub(crate) struct AvailableSkillsInstructions {
skill_lines: Vec<String>,
}
impl AvailableSkillsInstructions {
pub(crate) fn from_skill_lines(skill_lines: Vec<String>) -> Self {
Self { skill_lines }
}
}
impl ContextualUserFragment for AvailableSkillsInstructions {
fn role(&self) -> &'static str {
"developer"
}
fn markers(&self) -> (&'static str, &'static str) {
Self::type_markers()
}
fn type_markers() -> (&'static str, &'static str) {
(SKILLS_INSTRUCTIONS_OPEN_TAG, SKILLS_INSTRUCTIONS_CLOSE_TAG)
}
fn body(&self) -> String {
render_available_skills_body(&[], &self.skill_lines)
}
}
#[derive(Clone, Debug, Eq, PartialEq)]
pub(crate) struct SkillInstructions {
pub(crate) name: String,
pub(crate) path: String,
pub(crate) contents: String,
}
impl ContextualUserFragment for SkillInstructions {
fn role(&self) -> &'static str {
"user"
}
fn markers(&self) -> (&'static str, &'static str) {
Self::type_markers()
}
fn type_markers() -> (&'static str, &'static str) {
("<skill>", "</skill>")
}
fn body(&self) -> String {
let name = &self.name;
let path = &self.path;
let contents = &self.contents;
format!("\n<name>{name}</name>\n<path>{path}</path>\n{contents}\n")
}
}
+3
View File
@@ -1,5 +1,7 @@
pub mod catalog;
mod config;
mod extension;
mod fragments;
pub mod provider;
mod render;
mod selection;
@@ -7,6 +9,7 @@ mod sources;
mod state;
mod tools;
pub use config::SkillsExtensionConfig;
pub use extension::install;
pub use extension::install_with_providers;
pub use provider::ExecutorSkillProvider;
+1 -1
View File
@@ -1,9 +1,9 @@
use codex_core::context::AvailableSkillsInstructions;
use codex_utils_string::take_bytes_at_char_boundary;
use crate::catalog::SkillCatalog;
use crate::catalog::SkillCatalogEntry;
use crate::catalog::SkillSourceKind;
use crate::fragments::AvailableSkillsInstructions;
const MAX_AVAILABLE_SKILLS_BYTES: usize = 8_000;
const MAX_MAIN_PROMPT_BYTES: usize = 8_000;
+1 -16
View File
@@ -1,28 +1,13 @@
use codex_core::config::Config;
use codex_protocol::capabilities::SelectedCapabilityRoot;
use std::future::Future;
use std::sync::Mutex;
use tokio::sync::OnceCell;
use crate::SkillsExtensionConfig;
use crate::catalog::SkillCatalog;
use crate::catalog::SkillCatalogEntry;
use crate::catalog::SkillProviderError;
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) struct SkillsExtensionConfig {
pub(crate) include_instructions: bool,
pub(crate) bundled_skills_enabled: bool,
}
impl SkillsExtensionConfig {
pub(crate) fn from_config(config: &Config) -> Self {
Self {
include_instructions: config.include_skill_instructions,
bundled_skills_enabled: config.bundled_skills_enabled(),
}
}
}
#[derive(Debug)]
pub(crate) struct SkillsThreadState {
config: Mutex<SkillsExtensionConfig>,
+67 -58
View File
@@ -4,11 +4,11 @@ use std::sync::Mutex;
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering;
use codex_core::config::Config;
use codex_core::config::ConfigBuilder;
use codex_core_skills::HostLoadedSkills;
use codex_core_skills::SkillsLoadInput;
use codex_core_skills::SkillsManager;
use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS;
use codex_core_skills::SKILLS_INTRO_WITH_ABSOLUTE_PATHS;
use codex_core_skills::SkillLoadOutcome;
use codex_core_skills::SkillMetadata;
use codex_core_skills::injection::InjectedHostSkillPrompts;
use codex_extension_api::ExtensionData;
use codex_extension_api::ExtensionEventSink;
@@ -19,10 +19,13 @@ use codex_protocol::capabilities::CapabilityRootLocation;
use codex_protocol::capabilities::SelectedCapabilityRoot;
use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG;
use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG;
use codex_protocol::protocol::SessionSource;
use codex_protocol::protocol::SkillScope;
use codex_protocol::user_input::UserInput;
use codex_skills_extension::SkillProviders;
use codex_skills_extension::SkillsExtensionConfig;
use codex_skills_extension::catalog::SkillAuthority;
use codex_skills_extension::catalog::SkillCatalog;
use codex_skills_extension::catalog::SkillCatalogEntry;
@@ -39,14 +42,17 @@ use codex_skills_extension::provider::SkillProvider;
use codex_skills_extension::provider::SkillProviderFuture;
use codex_skills_extension::provider::SkillReadRequest;
use codex_skills_extension::provider::SkillSearchRequest;
use codex_utils_absolute_path::AbsolutePathBuf;
use pretty_assertions::assert_eq;
type TestResult = Result<(), Box<dyn std::error::Error>>;
static NEXT_CODEX_HOME_ID: AtomicUsize = AtomicUsize::new(0);
const DEMO_SKILL_CONTENTS: &str =
"---\nname: demo\ndescription: Demo skill.\n---\n# Demo\n\nUse the demo skill.\n";
#[tokio::test]
async fn installed_extension_loads_host_skills_from_legacy_roots() -> TestResult {
async fn installed_extension_uses_host_loaded_skills() -> TestResult {
let codex_home = test_codex_home();
let skill_path = codex_home.join("skills").join("demo").join("SKILL.md");
std::fs::create_dir_all(
@@ -54,18 +60,11 @@ async fn installed_extension_loads_host_skills_from_legacy_roots() -> TestResult
.parent()
.ok_or("skill path should have a parent")?,
)?;
std::fs::write(
&skill_path,
"---\nname: demo\ndescription: Demo skill.\n---\n# Demo\n\nUse the demo skill.\n",
)?;
let config = ConfigBuilder::default()
.codex_home(codex_home.clone())
.fallback_cwd(Some(codex_home.clone()))
.build()
.await?;
std::fs::write(&skill_path, DEMO_SKILL_CONTENTS)?;
let config = default_config();
let mut builder = ExtensionRegistryBuilder::new();
install(&mut builder);
install(&mut builder, skills_extension_config);
let registry = builder.build();
let session_store = ExtensionData::new("session");
let thread_store = ExtensionData::new("thread");
@@ -80,22 +79,21 @@ async fn installed_extension_loads_host_skills_from_legacy_roots() -> TestResult
})
.await;
let manager = SkillsManager::new(config.codex_home.clone(), config.bundled_skills_enabled());
let input = SkillsLoadInput::new(
config.cwd.clone(),
Vec::new(),
config.config_layer_stack.clone(),
config.bundled_skills_enabled(),
);
let loaded_skills = Arc::new(manager.skills_for_config(&input, /*fs*/ None).await);
let skill_path_string = loaded_skills
.skills
.iter()
.find(|skill| skill.name == "demo")
.ok_or("demo skill should load")?
.path_to_skills_md
.to_string_lossy()
.into_owned();
let skill_path = AbsolutePathBuf::try_from(skill_path)?;
let skill_path_string = skill_path.to_string_lossy().into_owned();
let mut outcome = SkillLoadOutcome::default();
outcome.skills.push(SkillMetadata {
name: "demo".to_string(),
description: "Demo skill.".to_string(),
short_description: None,
interface: None,
dependencies: None,
policy: None,
path_to_skills_md: skill_path,
scope: SkillScope::User,
plugin_id: None,
});
let loaded_skills = Arc::new(outcome);
let skill_prompt_path = skill_path_string.replace('\\', "/");
let turn_store = ExtensionData::new("turn-1");
turn_store.insert(HostLoadedSkills::new(Arc::clone(&loaded_skills)));
@@ -116,19 +114,19 @@ async fn installed_extension_loads_host_skills_from_legacy_roots() -> TestResult
)
.await;
assert_eq!(2, fragments.len());
assert_eq!("developer", fragments[0].role());
assert!(fragments[0].render().contains("demo"));
assert!(fragments[0].render().contains(&skill_prompt_path));
assert!(
fragments[0]
.render()
.contains(&format!("(file: {skill_prompt_path})"))
let expected_catalog = format!(
"{SKILLS_INSTRUCTIONS_OPEN_TAG}\n## Skills\n{SKILLS_INTRO_WITH_ABSOLUTE_PATHS}\n### Available skills\n- demo: Demo skill. (file: {skill_prompt_path})\n### How to use skills\n{SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS}\n{SKILLS_INSTRUCTIONS_CLOSE_TAG}"
);
let expected_skill = format!(
"<skill>\n<name>demo</name>\n<path>{skill_prompt_path}</path>\n{DEMO_SKILL_CONTENTS}\n</skill>"
);
assert_eq!(
vec![("developer", expected_catalog), ("user", expected_skill),],
fragments
.iter()
.map(|fragment| (fragment.role(), fragment.render()))
.collect::<Vec<_>>()
);
assert_eq!("user", fragments[1].role());
assert!(fragments[1].render().contains("<name>demo</name>"));
assert!(fragments[1].render().contains("# Demo"));
assert!(fragments[1].render().contains(&skill_prompt_path));
let injected_host_skill_prompts = turn_store
.get::<InjectedHostSkillPrompts>()
.ok_or("host skill prompt marker should be set")?;
@@ -158,7 +156,7 @@ async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_in
});
let providers = SkillProviders::new().with_executor_provider(executor_provider);
let mut builder = ExtensionRegistryBuilder::new();
install_with_providers(&mut builder, providers);
install_with_providers(&mut builder, providers, skills_extension_config);
let registry = builder.build();
let session_store = ExtensionData::new("session");
@@ -171,7 +169,7 @@ async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_in
},
}]);
let session_source = SessionSource::Cli;
let config = default_config().await?;
let config = default_config();
registry.thread_lifecycle_contributors()[0]
.on_thread_start(ThreadStartInput {
config: &config,
@@ -276,12 +274,12 @@ async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult {
let (event_tx, event_rx) = std::sync::mpsc::channel();
let mut builder =
ExtensionRegistryBuilder::with_event_sink(Arc::new(ChannelEventSink(event_tx)));
install_with_providers(&mut builder, providers);
install_with_providers(&mut builder, providers, skills_extension_config);
let registry = builder.build();
let session_store = ExtensionData::new("session");
let thread_store = ExtensionData::new("thread");
let session_source = SessionSource::Cli;
let config = default_config().await?;
let config = default_config();
registry.thread_lifecycle_contributors()[0]
.on_thread_start(ThreadStartInput {
config: &config,
@@ -355,7 +353,7 @@ async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> Te
});
let providers = SkillProviders::new().with_executor_provider(executor_provider);
let mut builder = ExtensionRegistryBuilder::new();
install_with_providers(&mut builder, providers);
install_with_providers(&mut builder, providers, skills_extension_config);
let registry = builder.build();
let session_store = ExtensionData::new("session");
let thread_store = ExtensionData::new("thread");
@@ -372,7 +370,7 @@ async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> Te
.collect::<Vec<_>>(),
);
let session_source = SessionSource::Cli;
let config = default_config().await?;
let config = default_config();
registry.thread_lifecycle_contributors()[0]
.on_thread_start(ThreadStartInput {
config: &config,
@@ -441,12 +439,12 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult {
});
let providers = SkillProviders::new().with_host_provider(provider);
let mut builder = ExtensionRegistryBuilder::new();
install_with_providers(&mut builder, providers);
install_with_providers(&mut builder, providers, skills_extension_config);
let registry = builder.build();
let session_store = ExtensionData::new("session");
let thread_store = ExtensionData::new("thread");
let session_source = SessionSource::Cli;
let config = default_config().await?;
let config = default_config();
registry.thread_lifecycle_contributors()[0]
.on_thread_start(ThreadStartInput {
config: &config,
@@ -558,13 +556,24 @@ fn test_entry(
.with_display_path(format!("skill://{package_id}/SKILL.md"))
}
async fn default_config() -> std::io::Result<Config> {
let codex_home = test_codex_home();
std::fs::create_dir_all(&codex_home)?;
let config =
Config::load_default_with_cli_overrides_for_codex_home(codex_home.clone(), vec![]).await?;
std::fs::remove_dir_all(codex_home)?;
Ok(config)
#[derive(Clone, Debug, Eq, PartialEq)]
struct TestConfig {
include_instructions: bool,
bundled_skills_enabled: bool,
}
fn default_config() -> TestConfig {
TestConfig {
include_instructions: true,
bundled_skills_enabled: true,
}
}
fn skills_extension_config(config: &TestConfig) -> SkillsExtensionConfig {
SkillsExtensionConfig {
include_instructions: config.include_instructions,
bundled_skills_enabled: config.bundled_skills_enabled,
}
}
fn test_codex_home() -> PathBuf {