mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Use the host skill provider catalog in world state (#36309)
## What changed - Load and cache the host skill provider catalog during world-state contribution. - Render host skill listings from that catalog with Core-compatible budgeting, while preserving Core's full prompt injection for selected host skills. - Reuse the cached host catalog for shadow selection, including when skill listings are disabled. - Preserve an empty Core-compatible skills fragment when every entry exceeds the metadata budget, and make budget warnings independent of the configured percentage. ## Testing - Cover provider-backed host listings, selected-skill prompt injection, shadow selection, disabled listings, oversized prompts, empty fragments, metrics, and warnings. GitOrigin-RevId: b6b2d8ea14386356094cff5783d121d74657e9e3
This commit is contained in:
Generated
+1
@@ -4099,6 +4099,7 @@ dependencies = [
|
||||
"codex-utils-path-uri",
|
||||
"codex-utils-string",
|
||||
"futures",
|
||||
"opentelemetry_sdk",
|
||||
"pretty_assertions",
|
||||
"schemars 0.8.22",
|
||||
"serde",
|
||||
|
||||
@@ -567,7 +567,7 @@ async fn turn_start_emits_thread_scoped_warning_notification_for_trimmed_skills(
|
||||
assert_eq!(warning.thread_id.as_deref(), Some(thread.id.as_str()));
|
||||
assert_eq!(
|
||||
warning.message,
|
||||
"Exceeded skills context budget of 2%. All skill descriptions were removed and 7 additional skills were not included in the model-visible skills list."
|
||||
"Exceeded skills context budget. All skill descriptions were removed and 7 additional skills were not included in the model-visible skills list."
|
||||
);
|
||||
|
||||
timeout(
|
||||
|
||||
@@ -11,6 +11,7 @@ use codex_extension_api::ExtensionEventSink;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_extension_api::ExtensionWarning;
|
||||
use codex_features::Feature;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_protocol::protocol::Event;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
@@ -35,18 +36,24 @@ use codex_skills_extension::provider::SkillProviderFuture;
|
||||
use codex_skills_extension::provider::SkillReadRequest;
|
||||
use codex_skills_extension::provider::SkillSearchRequest;
|
||||
use codex_utils_string::approx_token_count;
|
||||
use core_test_support::apps_test_server::AppsTestServer;
|
||||
use core_test_support::apps_test_server::apps_enabled_builder;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::responses::ev_completed;
|
||||
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::test_codex::test_codex;
|
||||
use core_test_support::wait_for_mcp_server;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::Duration;
|
||||
use tokio::time::Instant;
|
||||
use toml::toml;
|
||||
|
||||
struct StaticSkillProvider {
|
||||
catalog: SkillCatalog,
|
||||
main_prompt_contents: Option<String>,
|
||||
}
|
||||
|
||||
struct ExecutorSkillProvider {
|
||||
@@ -92,12 +99,18 @@ impl SkillProvider for StaticSkillProvider {
|
||||
Box::pin(async move { Ok(catalog) })
|
||||
}
|
||||
|
||||
fn read(&self, _request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> {
|
||||
Box::pin(async {
|
||||
Err(SkillProviderError::new(
|
||||
"production-flow catalog test does not read skills",
|
||||
))
|
||||
})
|
||||
fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> {
|
||||
let result = self
|
||||
.main_prompt_contents
|
||||
.clone()
|
||||
.map(|contents| SkillReadResult {
|
||||
resource: request.resource,
|
||||
contents,
|
||||
})
|
||||
.ok_or_else(|| {
|
||||
SkillProviderError::new("production-flow catalog test does not read skills")
|
||||
});
|
||||
Box::pin(async move { result })
|
||||
}
|
||||
|
||||
fn search(&self, _request: SkillSearchRequest) -> SkillProviderFuture<'_, SkillSearchResult> {
|
||||
@@ -461,7 +474,10 @@ async fn production_turn_scales_extension_catalog_from_resolved_model_window() -
|
||||
SkillProviders::new().with_provider(SkillProviderSource::new(
|
||||
source_kind,
|
||||
"test",
|
||||
Arc::new(StaticSkillProvider { catalog }),
|
||||
Arc::new(StaticSkillProvider {
|
||||
catalog,
|
||||
main_prompt_contents: None,
|
||||
}),
|
||||
)),
|
||||
|config: &Config| SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
@@ -576,13 +592,212 @@ async fn production_turn_shares_catalog_budget_across_host_and_executor_sections
|
||||
async fn production_turn_keeps_full_host_only_catalog_when_it_fits() -> Result<()> {
|
||||
let (developer_texts, _) =
|
||||
rendered_catalogs(&HOST_CATALOG, &[], FULL_CATALOG_CONTEXT_WINDOW).await?;
|
||||
let host_lines = skill_lines(catalog_text(&developer_texts, "host"), "host");
|
||||
let host_lines = developer_texts
|
||||
.iter()
|
||||
.flat_map(|text| skill_lines(text, "host"))
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_full_descriptions(&host_lines, &HOST_CATALOG);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn production_turn_uses_provider_host_catalog_and_core_snapshot_injection() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount(&server).await?;
|
||||
let response = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
|
||||
)
|
||||
.await;
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let skill_name = "snapshot-backed";
|
||||
let snapshot_description = "This description comes from Core's host skills snapshot.";
|
||||
write_host_skills(codex_home.path(), &[(skill_name, snapshot_description)])?;
|
||||
let skill_path = codex_home
|
||||
.path()
|
||||
.join("skills")
|
||||
.join(skill_name)
|
||||
.join("SKILL.md");
|
||||
let snapshot_contents = format!(
|
||||
"---\nname: {skill_name}\ndescription: {snapshot_description}\n---\n\nUse $calendar.\n"
|
||||
);
|
||||
std::fs::write(&skill_path, &snapshot_contents)?;
|
||||
let skill_resource = skill_path.to_string_lossy().into_owned();
|
||||
let provider_description = "This skill comes from the extension host provider.";
|
||||
let provider_contents = "# Provider instructions that Core must not inject.";
|
||||
let provider_catalog = SkillCatalog {
|
||||
entries: vec![
|
||||
SkillCatalogEntry::new(
|
||||
SkillPackageId(skill_resource.clone()),
|
||||
SkillAuthority::new(SkillSourceKind::Host, "host"),
|
||||
skill_name,
|
||||
provider_description,
|
||||
SkillResourceId::new(skill_resource.clone()),
|
||||
)
|
||||
.with_display_path(skill_resource.replace('\\', "/")),
|
||||
],
|
||||
warnings: Vec::new(),
|
||||
};
|
||||
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
|
||||
install_with_providers(
|
||||
&mut extensions,
|
||||
SkillProviders::new().with_host_provider(Arc::new(StaticSkillProvider {
|
||||
catalog: provider_catalog,
|
||||
main_prompt_contents: Some(provider_contents.to_string()),
|
||||
})),
|
||||
|config: &Config| SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
bundled_skills_enabled: false,
|
||||
orchestrator_skills_enabled: false,
|
||||
shadow_selection_enabled: false,
|
||||
},
|
||||
);
|
||||
let mut builder = apps_enabled_builder(apps_server.chatgpt_base_url)
|
||||
.with_home(Arc::clone(&codex_home))
|
||||
.with_extensions(Arc::new(extensions.build()))
|
||||
.with_config(configure_catalog_test);
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
wait_for_mcp_server(&test.codex, CODEX_APPS_MCP_SERVER_NAME).await?;
|
||||
|
||||
test.submit_turn(&format!("Use ${skill_name}.")).await?;
|
||||
let request = response.single_request();
|
||||
let developer_texts = request.message_input_texts("developer");
|
||||
let cutover_skill_lines = developer_texts
|
||||
.iter()
|
||||
.flat_map(|text| text.lines())
|
||||
.filter(|line| line.contains(skill_name))
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_eq!(
|
||||
cutover_skill_lines,
|
||||
vec![format!(
|
||||
"- {skill_name}: {provider_description} (file: {})",
|
||||
skill_resource.replace('\\', "/")
|
||||
)]
|
||||
);
|
||||
let user_text = request.message_input_texts("user").join("\n");
|
||||
assert!(user_text.contains(&snapshot_contents));
|
||||
assert!(!user_text.contains(provider_contents));
|
||||
let deadline = Instant::now() + Duration::from_secs(10);
|
||||
let app_mentioned_event = loop {
|
||||
let requests = server.received_requests().await.unwrap_or_default();
|
||||
if let Some(event) = requests
|
||||
.into_iter()
|
||||
.filter(|request| request.url.path() == "/codex/analytics-events/events")
|
||||
.find_map(|request| {
|
||||
let payload: serde_json::Value = serde_json::from_slice(&request.body).ok()?;
|
||||
payload["events"].as_array().and_then(|events| {
|
||||
events
|
||||
.iter()
|
||||
.find(|event| event["event_type"] == "codex_app_mentioned")
|
||||
.cloned()
|
||||
})
|
||||
})
|
||||
{
|
||||
break event;
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
panic!("timed out waiting for app mentioned analytics");
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(50)).await;
|
||||
};
|
||||
assert_eq!(
|
||||
app_mentioned_event["event_params"]["connector_id"],
|
||||
"calendar"
|
||||
);
|
||||
assert_eq!(
|
||||
app_mentioned_event["event_params"]["invoke_type"],
|
||||
"explicit"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn production_turn_keeps_full_snapshot_host_skill_prompt() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
let response = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
|
||||
)
|
||||
.await;
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let skill_dir = codex_home.path().join("skills").join("long-host");
|
||||
std::fs::create_dir_all(&skill_dir)?;
|
||||
let prompt_tail = "full host prompt tail";
|
||||
let skill_contents = format!(
|
||||
"---\nname: long-host\ndescription: Long host skill.\n---\n\n# Long host skill\n\n{}\n{prompt_tail}\n",
|
||||
"x".repeat(8_000)
|
||||
);
|
||||
std::fs::write(skill_dir.join("SKILL.md"), &skill_contents)?;
|
||||
let (extensions, _) =
|
||||
catalog_extensions(SkillCatalog::default(), /*include_host_provider*/ true);
|
||||
let mut builder = test_codex()
|
||||
.with_home(Arc::clone(&codex_home))
|
||||
.with_extensions(extensions)
|
||||
.with_config(configure_catalog_test);
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
test.submit_turn("Use $long-host.").await?;
|
||||
let user_text = response
|
||||
.single_request()
|
||||
.message_input_texts("user")
|
||||
.join("\n");
|
||||
|
||||
assert!(user_text.contains(&skill_contents));
|
||||
assert_eq!(
|
||||
user_text.matches("<skill>\n<name>long-host</name>").count(),
|
||||
1
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn production_turn_keeps_core_host_injection_when_catalog_listings_are_disabled() -> Result<()>
|
||||
{
|
||||
let server = responses::start_mock_server().await;
|
||||
let response = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
|
||||
)
|
||||
.await;
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let skill_dir = codex_home.path().join("skills").join("long-host");
|
||||
std::fs::create_dir_all(&skill_dir)?;
|
||||
let prompt_tail = "full host prompt tail";
|
||||
let skill_contents = format!(
|
||||
"---\nname: long-host\ndescription: Long host skill.\n---\n\n# Long host skill\n\n{}\n{prompt_tail}\n",
|
||||
"x".repeat(8_000)
|
||||
);
|
||||
std::fs::write(skill_dir.join("SKILL.md"), &skill_contents)?;
|
||||
let (extensions, _) =
|
||||
catalog_extensions(SkillCatalog::default(), /*include_host_provider*/ true);
|
||||
let mut builder = test_codex()
|
||||
.with_home(Arc::clone(&codex_home))
|
||||
.with_extensions(extensions)
|
||||
.with_config(configure_catalog_test)
|
||||
.with_config(|config| {
|
||||
config.include_skill_instructions = false;
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
test.submit_turn("Use $long-host.").await?;
|
||||
let user_text = response
|
||||
.single_request()
|
||||
.message_input_texts("user")
|
||||
.join("\n");
|
||||
|
||||
assert!(user_text.contains(&skill_contents));
|
||||
assert_eq!(
|
||||
user_text.matches("<skill>\n<name>long-host</name>").count(),
|
||||
1
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn production_turn_keeps_full_executor_only_catalog_when_it_fits() -> Result<()> {
|
||||
let (developer_texts, _) =
|
||||
@@ -621,7 +836,7 @@ async fn production_turn_omits_host_skills_under_extreme_host_only_pressure() ->
|
||||
skill_names(&host_lines),
|
||||
vec!["host-alpha", "host-beta", "host-delta"]
|
||||
);
|
||||
let expected_warning = "Exceeded skills context budget of 2%. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list.";
|
||||
let expected_warning = "Exceeded skills context budget. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list.";
|
||||
assert_eq!(
|
||||
warning_messages
|
||||
.iter()
|
||||
@@ -633,6 +848,36 @@ async fn production_turn_omits_host_skills_under_extreme_host_only_pressure() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn production_turn_preserves_empty_core_compatible_host_fragment() -> Result<()> {
|
||||
let host_skills = [(
|
||||
"host-a-b-c-d-e-f-g-h-i-j-k-l-m-n-o-p-q-r-s-t-u-v-w-x-y-z-a-b",
|
||||
"Host-only skill.",
|
||||
)];
|
||||
let (developer_texts, warning_messages) =
|
||||
rendered_catalogs(&host_skills, &[], /*context_window*/ 1_000).await?;
|
||||
let host_fragment = developer_texts
|
||||
.iter()
|
||||
.find(|text| text.contains("## Skills"))
|
||||
.unwrap_or_else(|| {
|
||||
panic!(
|
||||
"production request should preserve the empty host skills fragment, got {developer_texts:?}"
|
||||
)
|
||||
});
|
||||
|
||||
assert!(!host_fragment.contains(&format!("- {}:", host_skills[0].0)));
|
||||
assert!(!host_fragment.contains("## Host skills update"));
|
||||
assert_eq!(
|
||||
warning_messages,
|
||||
vec![
|
||||
"Exceeded skills context budget. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list."
|
||||
.to_string()
|
||||
]
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn successive_turns_do_not_repeat_unchanged_host_budget_warning() -> Result<()> {
|
||||
let (_, warning_messages) = rendered_catalogs_for_turns(
|
||||
@@ -642,7 +887,7 @@ async fn successive_turns_do_not_repeat_unchanged_host_budget_warning() -> Resul
|
||||
/*turn_count*/ 2,
|
||||
)
|
||||
.await?;
|
||||
let expected_warning = "Exceeded skills context budget of 2%. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list.";
|
||||
let expected_warning = "Exceeded skills context budget. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list.";
|
||||
|
||||
assert_eq!(
|
||||
warning_messages
|
||||
@@ -757,7 +1002,10 @@ async fn production_turn_fairly_shortens_extension_catalog_descriptions() -> Res
|
||||
SkillProviders::new().with_provider(SkillProviderSource::new(
|
||||
source_kind,
|
||||
"test",
|
||||
Arc::new(StaticSkillProvider { catalog }),
|
||||
Arc::new(StaticSkillProvider {
|
||||
catalog,
|
||||
main_prompt_contents: None,
|
||||
}),
|
||||
)),
|
||||
|config: &Config| SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
|
||||
@@ -34,5 +34,6 @@ url = { workspace = true }
|
||||
[dev-dependencies]
|
||||
codex-models-manager = { workspace = true }
|
||||
codex-utils-absolute-path = { workspace = true }
|
||||
opentelemetry_sdk = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }
|
||||
|
||||
@@ -64,6 +64,7 @@ use crate::shadow_selection_experiment::ShadowSelectionExperiment;
|
||||
use crate::sources::SkillProviders;
|
||||
use crate::state::EmittedCatalogBudgetWarnings;
|
||||
use crate::state::ExecutorSkillsStepState;
|
||||
use crate::state::HostSkillsStepState;
|
||||
use crate::state::SkillsSessionState;
|
||||
use crate::state::SkillsThreadState;
|
||||
use crate::state::SkillsTurnState;
|
||||
@@ -72,7 +73,6 @@ use crate::warnings::bounded_warnings;
|
||||
use crate::world_state::HostSkillsWarningEmitter;
|
||||
use crate::world_state::executor_skills_world_state_section;
|
||||
use crate::world_state::host_skills_world_state_section;
|
||||
use crate::world_state::rendered_host_skills_world_state_section;
|
||||
|
||||
struct SkillsExtension<C> {
|
||||
providers: SkillProviders,
|
||||
@@ -283,42 +283,50 @@ where
|
||||
let metadata_budget = skill_metadata_budget(context_window);
|
||||
let extension_metrics = input.extension_metrics.clone();
|
||||
let host_snapshot = input.turn_store.get::<HostSkillsSnapshot>();
|
||||
let shared_rendered = if config.include_instructions
|
||||
let needs_host_catalog = config.include_instructions || config.shadow_selection_enabled;
|
||||
let host_catalog = if needs_host_catalog
|
||||
&& let Some(host_snapshot) = host_snapshot.as_ref()
|
||||
&& self.providers.has_host_provider()
|
||||
{
|
||||
let host_catalog = self
|
||||
.providers
|
||||
.list_host_for_turn(SkillListQuery {
|
||||
turn_id: input.turn_id.to_string(),
|
||||
executor_roots: Vec::new(),
|
||||
resolved_executor_roots: Vec::new(),
|
||||
host_snapshot: Some(Arc::clone(host_snapshot)),
|
||||
include_host_skills: true,
|
||||
include_bundled_skills: false,
|
||||
include_orchestrator_skills: false,
|
||||
mcp_resources: None,
|
||||
executor_capability_discovery: None,
|
||||
})
|
||||
.await;
|
||||
let (host_rendered, executor_rendered) =
|
||||
render_combined_available_skills(&host_catalog, &catalog, metadata_budget);
|
||||
if host_catalog
|
||||
Some(
|
||||
self.providers
|
||||
.list_host_for_turn(SkillListQuery {
|
||||
turn_id: input.turn_id.to_string(),
|
||||
executor_roots: Vec::new(),
|
||||
resolved_executor_roots: Vec::new(),
|
||||
host_snapshot: Some(Arc::clone(host_snapshot)),
|
||||
include_host_skills: true,
|
||||
include_bundled_skills: false,
|
||||
include_orchestrator_skills: false,
|
||||
mcp_resources: None,
|
||||
executor_capability_discovery: None,
|
||||
})
|
||||
.await,
|
||||
)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
if let Some(host_catalog) = host_catalog.as_ref() {
|
||||
input
|
||||
.turn_store
|
||||
.insert(HostSkillsStepState(host_catalog.clone()));
|
||||
}
|
||||
let host_catalog_for_rendering = if config.include_instructions {
|
||||
host_catalog.as_ref()
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let shared_rendered = host_catalog_for_rendering.and_then(|host_catalog| {
|
||||
(host_catalog
|
||||
.entries
|
||||
.iter()
|
||||
.any(SkillCatalogEntry::is_model_visible)
|
||||
&& catalog
|
||||
.entries
|
||||
.iter()
|
||||
.any(SkillCatalogEntry::is_model_visible)
|
||||
{
|
||||
Some((host_rendered, executor_rendered))
|
||||
} else {
|
||||
None
|
||||
}
|
||||
} else {
|
||||
None
|
||||
};
|
||||
.any(SkillCatalogEntry::is_model_visible))
|
||||
.then(|| render_combined_available_skills(host_catalog, &catalog, metadata_budget))
|
||||
});
|
||||
let (rendered, rendered_host) =
|
||||
if let Some((host_rendered, executor_rendered)) = shared_rendered {
|
||||
if let Some(host_rendered) = host_rendered.as_ref() {
|
||||
@@ -335,6 +343,16 @@ where
|
||||
host_rendered,
|
||||
)
|
||||
} else {
|
||||
let rendered_host = host_catalog_for_rendering.and_then(|host_catalog| {
|
||||
render_available_skills(
|
||||
host_catalog,
|
||||
SkillCatalogRenderPolicy::CoreCompatible,
|
||||
metadata_budget,
|
||||
)
|
||||
});
|
||||
if let Some(rendered_host) = rendered_host.as_ref() {
|
||||
trace_catalog_budget_pressure(metadata_budget, &rendered_host.report);
|
||||
}
|
||||
(
|
||||
if config.include_instructions {
|
||||
render_catalog(
|
||||
@@ -348,12 +366,29 @@ where
|
||||
} else {
|
||||
RenderedCatalog::default()
|
||||
},
|
||||
None,
|
||||
rendered_host,
|
||||
)
|
||||
};
|
||||
let host_warning_message = rendered_host
|
||||
.as_ref()
|
||||
.and_then(|rendered_host| rendered_host.report.warning_message());
|
||||
let host_render_metrics = config.include_instructions.then(|| {
|
||||
rendered_host
|
||||
.as_ref()
|
||||
.map(|rendered_host| {
|
||||
let report = &rendered_host.report;
|
||||
(
|
||||
report.total_count,
|
||||
report.included_count,
|
||||
report.omitted_count,
|
||||
report.truncated_description_chars,
|
||||
)
|
||||
})
|
||||
.unwrap_or_default()
|
||||
});
|
||||
let host_body = rendered_host
|
||||
.and_then(|rendered_host| rendered_host.into_fragment(include_usage))
|
||||
.map(|fragment| fragment.body());
|
||||
let executor_body = rendered.fragment.map(|fragment| fragment.body());
|
||||
let mut sections = vec![executor_skills_world_state_section(
|
||||
executor_body,
|
||||
@@ -362,9 +397,7 @@ where
|
||||
let emitted_warnings = input
|
||||
.turn_store
|
||||
.get_or_init(EmittedCatalogBudgetWarnings::default);
|
||||
if let Some(host_snapshot) = host_snapshot
|
||||
&& self.providers.has_host_provider()
|
||||
{
|
||||
if host_snapshot.is_some() && self.providers.has_host_provider() {
|
||||
input.turn_store.insert(HostSkillsCatalogInWorldState);
|
||||
let event_sink = Arc::clone(&self.event_sink);
|
||||
let emitted_warnings = Arc::clone(&emitted_warnings);
|
||||
@@ -379,35 +412,14 @@ where
|
||||
});
|
||||
}
|
||||
});
|
||||
if let Some(rendered_host) = rendered_host {
|
||||
let report = &rendered_host.report;
|
||||
let render_metrics = Some((
|
||||
report.total_count,
|
||||
report.included_count,
|
||||
report.omitted_count,
|
||||
report.truncated_description_chars,
|
||||
));
|
||||
let body = rendered_host
|
||||
.into_fragment(include_usage)
|
||||
.map(|fragment| fragment.body());
|
||||
sections.push(rendered_host_skills_world_state_section(
|
||||
body,
|
||||
config.include_instructions,
|
||||
render_metrics,
|
||||
extension_metrics,
|
||||
host_warning_message,
|
||||
warning_emitter,
|
||||
));
|
||||
} else {
|
||||
sections.push(host_skills_world_state_section(
|
||||
&host_snapshot,
|
||||
config.include_instructions,
|
||||
include_usage,
|
||||
metadata_budget,
|
||||
extension_metrics,
|
||||
warning_emitter,
|
||||
));
|
||||
}
|
||||
sections.push(host_skills_world_state_section(
|
||||
host_body,
|
||||
config.include_instructions,
|
||||
host_render_metrics,
|
||||
extension_metrics,
|
||||
host_warning_message,
|
||||
warning_emitter,
|
||||
));
|
||||
}
|
||||
if let Some(message) = rendered.warning_message
|
||||
&& emitted_warnings.insert(&message)
|
||||
@@ -521,18 +533,18 @@ where
|
||||
let host_snapshot = turn_store.get::<HostSkillsSnapshot>();
|
||||
let host_catalog_in_world_state =
|
||||
turn_store.get::<HostSkillsCatalogInWorldState>().is_some();
|
||||
let host_skills = turn_store.get::<HostSkillsStepState>();
|
||||
let query = SkillListQuery {
|
||||
turn_id: input.turn_id.clone(),
|
||||
executor_roots: Vec::new(),
|
||||
resolved_executor_roots: Vec::new(),
|
||||
host_snapshot: host_snapshot.clone(),
|
||||
include_host_skills: !host_catalog_in_world_state,
|
||||
include_host_skills: host_skills.is_none() && !host_catalog_in_world_state,
|
||||
include_bundled_skills: config.bundled_skills_enabled,
|
||||
include_orchestrator_skills: thread_state.orchestrator_skills_enabled(),
|
||||
mcp_resources: mcp_resources.clone(),
|
||||
executor_capability_discovery: None,
|
||||
};
|
||||
let host_query = query.clone();
|
||||
let mut catalog = turn_store
|
||||
.get::<ExecutorSkillsStepState>()
|
||||
.map(|executor_skills| executor_skills.0.clone())
|
||||
@@ -545,8 +557,8 @@ where
|
||||
let selected_entries = collect_explicit_skill_mentions(&input.user_input, &catalog);
|
||||
let shadow_selection_turn = if config.shadow_selection_enabled {
|
||||
let mut shadow_catalog = catalog.clone();
|
||||
if host_catalog_in_world_state && host_snapshot.is_some() {
|
||||
shadow_catalog.extend(self.providers.list_host_for_turn(host_query).await);
|
||||
if let Some(host_skills) = host_skills {
|
||||
shadow_catalog.extend(host_skills.0.clone());
|
||||
}
|
||||
let shadow_selected_entries =
|
||||
collect_explicit_skill_mentions(&input.user_input, &shadow_catalog);
|
||||
@@ -562,9 +574,7 @@ where
|
||||
thread_state
|
||||
.replace_shadow_selection_turn(input.turn_id.clone(), shadow_selection_turn);
|
||||
let mut fragments: Vec<Box<dyn ContextualUserFragment + Send>> = Vec::new();
|
||||
if config.include_instructions
|
||||
&& turn_store.get::<HostSkillsCatalogInWorldState>().is_none()
|
||||
{
|
||||
if config.include_instructions && !host_catalog_in_world_state {
|
||||
let mut turn_catalog = catalog.clone();
|
||||
turn_catalog.entries.retain(|entry| {
|
||||
entry.authority.kind != SkillSourceKind::Executor
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
use codex_core_skills::AvailableSkills;
|
||||
use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS;
|
||||
use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ALIASES;
|
||||
use codex_core_skills::render_available_skills_body;
|
||||
@@ -32,26 +31,6 @@ impl AvailableSkillsInstructions {
|
||||
skill_lines,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn from_available_skills(
|
||||
available: AvailableSkills,
|
||||
include_skills_usage_instructions: bool,
|
||||
) -> Self {
|
||||
let mut skill_lines = available.skill_lines;
|
||||
if include_skills_usage_instructions {
|
||||
skill_lines.push("### How to use skills".to_string());
|
||||
let instructions = if available.skill_root_lines.is_empty() {
|
||||
SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS
|
||||
} else {
|
||||
SKILLS_HOW_TO_USE_WITH_ALIASES
|
||||
};
|
||||
skill_lines.push(instructions.to_string());
|
||||
}
|
||||
Self {
|
||||
skill_root_lines: available.skill_root_lines,
|
||||
skill_lines,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for AvailableSkillsInstructions {
|
||||
|
||||
@@ -440,6 +440,7 @@ struct RenderedCatalog {
|
||||
pub(crate) struct AvailableSkillsRender {
|
||||
skill_root_lines: Vec<String>,
|
||||
skill_lines: Vec<String>,
|
||||
preserve_empty_fragment: bool,
|
||||
pub(crate) report: SkillRenderReport,
|
||||
}
|
||||
|
||||
@@ -448,7 +449,7 @@ impl AvailableSkillsRender {
|
||||
self,
|
||||
include_skills_usage_instructions: bool,
|
||||
) -> Option<AvailableSkillsInstructions> {
|
||||
(!self.skill_lines.is_empty()).then(|| {
|
||||
(self.preserve_empty_fragment || !self.skill_lines.is_empty()).then(|| {
|
||||
AvailableSkillsInstructions::from_skill_lines(
|
||||
self.skill_root_lines,
|
||||
self.skill_lines,
|
||||
@@ -501,6 +502,7 @@ pub(crate) fn render_available_skills(
|
||||
Some(AvailableSkillsRender {
|
||||
skill_root_lines: selected.skill_root_lines,
|
||||
skill_lines: selected.skill_lines,
|
||||
preserve_empty_fragment: policy == SkillCatalogRenderPolicy::CoreCompatible,
|
||||
report: selected.report,
|
||||
})
|
||||
}
|
||||
@@ -615,6 +617,7 @@ fn render_combined_group(
|
||||
AvailableSkillsRender {
|
||||
skill_root_lines,
|
||||
skill_lines: lines.into_iter().map(|rendered| rendered.line).collect(),
|
||||
preserve_empty_fragment: false,
|
||||
report: SkillRenderReport {
|
||||
total_count: skill_lines.len(),
|
||||
included_count: skill_lines.len().saturating_sub(omitted_count),
|
||||
|
||||
@@ -387,6 +387,10 @@ fn mixed_catalog_reserves_executor_omission_marker_by_omitting_host_first() {
|
||||
"- 1 additional skill omitted from this bounded skills list.".to_string(),
|
||||
]
|
||||
);
|
||||
assert!(
|
||||
host.into_fragment(/*include_skills_usage_instructions*/ false)
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -630,10 +634,11 @@ fn catalog_emits_omission_marker_when_every_minimum_skill_line_exceeds_budget()
|
||||
)
|
||||
.expect("core-compatible report should render");
|
||||
assert_eq!(core_render.report, expected_report);
|
||||
assert_eq!(
|
||||
core_render.into_fragment(/*include_skills_usage_instructions*/ false),
|
||||
None
|
||||
);
|
||||
let core_fragment = core_render
|
||||
.into_fragment(/*include_skills_usage_instructions*/ false)
|
||||
.expect("core-compatible rendering should preserve an empty skills fragment");
|
||||
assert!(core_fragment.body().contains("## Skills"));
|
||||
assert!(!core_fragment.body().contains("- oversized:"));
|
||||
let render = render_available_skills(
|
||||
&catalog,
|
||||
SkillCatalogRenderPolicy::ExtensionCompatible,
|
||||
|
||||
@@ -375,3 +375,6 @@ pub(crate) struct SkillsTurnState {
|
||||
|
||||
#[derive(Clone, Debug, Default)]
|
||||
pub(crate) struct ExecutorSkillsStepState(pub(crate) SkillCatalog);
|
||||
|
||||
#[derive(Clone, Debug, Default)]
|
||||
pub(crate) struct HostSkillsStepState(pub(crate) SkillCatalog);
|
||||
|
||||
@@ -1,9 +1,5 @@
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_core_skills::HostSkillsSnapshot;
|
||||
use codex_core_skills::build_available_skills;
|
||||
use codex_core_skills::render::SkillRenderSideEffects;
|
||||
use codex_extension_api::ContextualUserFragment;
|
||||
use codex_extension_api::ExtensionMetrics;
|
||||
use codex_extension_api::PreviousWorldStateSection;
|
||||
use codex_extension_api::RenderedWorldStateFragment;
|
||||
@@ -12,8 +8,6 @@ use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG;
|
||||
use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG;
|
||||
use serde_json::json;
|
||||
|
||||
use crate::fragments::AvailableSkillsInstructions;
|
||||
use crate::render::SkillMetadataBudget;
|
||||
use crate::render_observability::CatalogSurface;
|
||||
use crate::render_observability::record_catalog_metrics;
|
||||
|
||||
@@ -82,60 +76,6 @@ pub(crate) fn executor_skills_world_state_section(
|
||||
}
|
||||
|
||||
pub(crate) fn host_skills_world_state_section(
|
||||
host_snapshot: &HostSkillsSnapshot,
|
||||
include_instructions: bool,
|
||||
include_skills_usage_instructions: bool,
|
||||
metadata_budget: SkillMetadataBudget,
|
||||
extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
|
||||
warning_emitter: HostSkillsWarningEmitter,
|
||||
) -> WorldStateSectionContribution {
|
||||
let outcome = host_snapshot.outcome();
|
||||
let metadata_budget = match metadata_budget {
|
||||
SkillMetadataBudget::Tokens(limit) => codex_core_skills::SkillMetadataBudget::Tokens(limit),
|
||||
SkillMetadataBudget::Characters(limit) => {
|
||||
codex_core_skills::SkillMetadataBudget::Characters(limit)
|
||||
}
|
||||
};
|
||||
let available = if include_instructions {
|
||||
build_available_skills(outcome, metadata_budget, SkillRenderSideEffects::None)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let warning_message = available
|
||||
.as_ref()
|
||||
.and_then(|available| available.warning_message.clone());
|
||||
let render_metrics = include_instructions.then(|| {
|
||||
available
|
||||
.as_ref()
|
||||
.map(|available| {
|
||||
let report = &available.report;
|
||||
(
|
||||
report.total_count,
|
||||
report.included_count,
|
||||
report.omitted_count,
|
||||
report.truncated_description_chars,
|
||||
)
|
||||
})
|
||||
.unwrap_or_default()
|
||||
});
|
||||
let body = available.map(|available| {
|
||||
AvailableSkillsInstructions::from_available_skills(
|
||||
available,
|
||||
include_skills_usage_instructions,
|
||||
)
|
||||
.body()
|
||||
});
|
||||
rendered_host_skills_world_state_section(
|
||||
body,
|
||||
include_instructions,
|
||||
render_metrics,
|
||||
extension_metrics,
|
||||
warning_message,
|
||||
warning_emitter,
|
||||
)
|
||||
}
|
||||
|
||||
pub(crate) fn rendered_host_skills_world_state_section(
|
||||
body: Option<String>,
|
||||
include_instructions: bool,
|
||||
render_metrics: Option<(usize, usize, usize, usize)>,
|
||||
|
||||
@@ -26,6 +26,8 @@ use codex_extension_api::ToolPayload;
|
||||
use codex_extension_api::TurnInputContext;
|
||||
use codex_extension_api::WorldStateContributionInput;
|
||||
use codex_models_manager::model_info::model_info_from_slug;
|
||||
use codex_otel::MetricsClient;
|
||||
use codex_otel::MetricsConfig;
|
||||
use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC;
|
||||
use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC;
|
||||
use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC;
|
||||
@@ -55,6 +57,7 @@ use codex_skills_extension::catalog::SkillSearchResult;
|
||||
use codex_skills_extension::catalog::SkillSourceKind;
|
||||
use codex_skills_extension::install;
|
||||
use codex_skills_extension::install_with_providers;
|
||||
use codex_skills_extension::install_with_providers_and_metrics;
|
||||
use codex_skills_extension::provider::SkillListQuery;
|
||||
use codex_skills_extension::provider::SkillProvider;
|
||||
use codex_skills_extension::provider::SkillProviderFuture;
|
||||
@@ -62,6 +65,9 @@ use codex_skills_extension::provider::SkillReadRequest;
|
||||
use codex_skills_extension::provider::SkillSearchRequest;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use opentelemetry_sdk::metrics::InMemoryMetricExporter;
|
||||
use opentelemetry_sdk::metrics::data::AggregatedMetrics;
|
||||
use opentelemetry_sdk::metrics::data::MetricData;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tokio::sync::Semaphore;
|
||||
|
||||
@@ -492,7 +498,190 @@ async fn nonempty_executor_empty_host_records_catalog_metrics() -> TestResult {
|
||||
/*count*/ 0,
|
||||
));
|
||||
assert_eq!(metrics.samples(), expected);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn host_world_state_uses_provider_catalog_with_core_compatible_rendering() -> TestResult {
|
||||
let list_calls = Arc::new(AtomicUsize::new(0));
|
||||
let provider = Arc::new(StaticSkillProvider {
|
||||
catalog: SkillCatalog {
|
||||
entries: vec![
|
||||
test_entry(
|
||||
SkillSourceKind::Host,
|
||||
"host",
|
||||
"host/provider-skill",
|
||||
"provider-skill/SKILL.md",
|
||||
)
|
||||
.with_short_description(Some("Short description.".to_string())),
|
||||
],
|
||||
warnings: Vec::new(),
|
||||
},
|
||||
read_requests: Arc::new(Mutex::new(Vec::new())),
|
||||
list_calls: Some(Arc::clone(&list_calls)),
|
||||
fail_first_list: false,
|
||||
});
|
||||
let mut builder = ExtensionRegistryBuilder::new();
|
||||
install_with_providers(
|
||||
&mut builder,
|
||||
SkillProviders::new().with_host_provider(provider),
|
||||
skills_extension_config,
|
||||
);
|
||||
let registry = builder.build();
|
||||
let session_store = ExtensionData::new("session");
|
||||
let thread_store = ExtensionData::new("thread");
|
||||
let config = default_config();
|
||||
registry.thread_lifecycle_contributors()[0]
|
||||
.on_thread_start(ThreadStartInput {
|
||||
config: &config,
|
||||
session_source: &SessionSource::Cli,
|
||||
persistent_thread_state_available: true,
|
||||
environments: &[],
|
||||
mcp_resource_client: None,
|
||||
extension_metrics: None,
|
||||
session_store: &session_store,
|
||||
thread_store: &thread_store,
|
||||
})
|
||||
.await;
|
||||
let turn_store = ExtensionData::new("turn-1");
|
||||
turn_store.insert(HostSkillsSnapshot::new(Arc::new(
|
||||
SkillLoadOutcome::default(),
|
||||
)));
|
||||
|
||||
let sections = registry.context_contributors()[0]
|
||||
.contribute_world_state(WorldStateContributionInput {
|
||||
thread_id: codex_protocol::ThreadId::new(),
|
||||
turn_id: "turn-1",
|
||||
environments: &[],
|
||||
ready_selected_capability_roots: &[],
|
||||
executor_capability_discovery: None,
|
||||
extension_metrics: None,
|
||||
session_store: &session_store,
|
||||
thread_store: &thread_store,
|
||||
turn_store: &turn_store,
|
||||
})
|
||||
.await;
|
||||
let host_fragment = sections[1]
|
||||
.render_diff(PreviousWorldStateSection::Absent)
|
||||
.ok_or("host provider catalog should render")?;
|
||||
|
||||
assert!(host_fragment.body().contains("Fix lint errors."));
|
||||
assert!(!host_fragment.body().contains("Short description."));
|
||||
assert_eq!(1, list_calls.load(Ordering::Relaxed));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn shadow_selection_uses_host_catalog_when_instructions_are_disabled() -> TestResult {
|
||||
let list_calls = Arc::new(AtomicUsize::new(0));
|
||||
let provider = Arc::new(StaticSkillProvider {
|
||||
catalog: SkillCatalog {
|
||||
entries: vec![test_entry(
|
||||
SkillSourceKind::Host,
|
||||
"host",
|
||||
"host/lint-fix",
|
||||
"lint-fix/SKILL.md",
|
||||
)],
|
||||
warnings: Vec::new(),
|
||||
},
|
||||
read_requests: Arc::new(Mutex::new(Vec::new())),
|
||||
list_calls: Some(Arc::clone(&list_calls)),
|
||||
fail_first_list: false,
|
||||
});
|
||||
let metrics = MetricsClient::new(
|
||||
MetricsConfig::in_memory(
|
||||
"test",
|
||||
"codex-skills-extension",
|
||||
env!("CARGO_PKG_VERSION"),
|
||||
InMemoryMetricExporter::default(),
|
||||
)
|
||||
.with_runtime_reader(),
|
||||
)?;
|
||||
let mut builder = ExtensionRegistryBuilder::new();
|
||||
install_with_providers_and_metrics(
|
||||
&mut builder,
|
||||
SkillProviders::new().with_host_provider(provider),
|
||||
Some(metrics.clone()),
|
||||
skills_extension_config,
|
||||
);
|
||||
let registry = builder.build();
|
||||
let session_store = ExtensionData::new("session");
|
||||
let thread_store = ExtensionData::new("thread");
|
||||
let mut config = default_config();
|
||||
config.include_instructions = false;
|
||||
config.shadow_selection_enabled = true;
|
||||
registry.thread_lifecycle_contributors()[0]
|
||||
.on_thread_start(ThreadStartInput {
|
||||
config: &config,
|
||||
session_source: &SessionSource::Cli,
|
||||
persistent_thread_state_available: true,
|
||||
environments: &[],
|
||||
mcp_resource_client: None,
|
||||
extension_metrics: None,
|
||||
session_store: &session_store,
|
||||
thread_store: &thread_store,
|
||||
})
|
||||
.await;
|
||||
let turn_store = ExtensionData::new("turn-1");
|
||||
turn_store.insert(HostSkillsSnapshot::new(Arc::new(
|
||||
SkillLoadOutcome::default(),
|
||||
)));
|
||||
|
||||
let sections = registry.context_contributors()[0]
|
||||
.contribute_world_state(WorldStateContributionInput {
|
||||
thread_id: codex_protocol::ThreadId::new(),
|
||||
turn_id: "turn-1",
|
||||
environments: &[],
|
||||
ready_selected_capability_roots: &[],
|
||||
executor_capability_discovery: None,
|
||||
extension_metrics: None,
|
||||
session_store: &session_store,
|
||||
thread_store: &thread_store,
|
||||
turn_store: &turn_store,
|
||||
})
|
||||
.await;
|
||||
let fragments = registry.turn_input_contributors()[0]
|
||||
.contribute(
|
||||
TurnInputContext {
|
||||
turn_id: "turn-1".to_string(),
|
||||
user_input: vec![UserInput::Text {
|
||||
text: "Fix lint errors.".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
environments: Vec::new(),
|
||||
},
|
||||
/*extension_metrics*/ None,
|
||||
&session_store,
|
||||
&thread_store,
|
||||
&turn_store,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
sections[1]
|
||||
.render_diff(PreviousWorldStateSection::Absent)
|
||||
.is_none()
|
||||
);
|
||||
assert!(fragments.is_empty());
|
||||
let snapshot = metrics.snapshot()?;
|
||||
let catalog_entry_counts = snapshot
|
||||
.scope_metrics()
|
||||
.flat_map(opentelemetry_sdk::metrics::data::ScopeMetrics::metrics)
|
||||
.find(|metric| metric.name() == "codex.skills.shadow_selection.catalog_entries")
|
||||
.map(|metric| match metric.data() {
|
||||
AggregatedMetrics::F64(MetricData::Histogram(histogram)) => histogram
|
||||
.data_points()
|
||||
.map(opentelemetry_sdk::metrics::data::HistogramDataPoint::sum)
|
||||
.collect::<Vec<_>>(),
|
||||
data => panic!("unexpected shadow catalog metric data: {data:?}"),
|
||||
})
|
||||
.ok_or("shadow catalog metric should be recorded")?;
|
||||
|
||||
assert!(
|
||||
catalog_entry_counts.iter().all(|count| *count == 1.0),
|
||||
"every shadow selector should see the cached host skill: {catalog_entry_counts:?}"
|
||||
);
|
||||
assert_eq!(1, list_calls.load(Ordering::Relaxed));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user