mirror of
https://github.com/openai/codex.git
synced 2026-11-05 16:29:31 +00:00
Route extension warnings to app-server threads (#35168)
## Why Extension warnings need a stable thread target even when they are emitted outside an active turn. ## What changed - Add `ExtensionWarning` and a dedicated `ExtensionEventSink::emit_warning` path with an explicit thread ID and optional turn ID. - Deliver extension warnings as thread-scoped app-server `warning` notifications, preserving listener ordering and waiting briefly for a subscriber when no listener is active. - Move skills warnings to the new path, limiting provider catalog warnings to four messages of 256 bytes each. ## Testing - Cover warning routing, ordering, UTF-8-safe truncation, subscriber fallback, and invalid thread IDs. - Verify skills warning bounds and app-server delivery of executor catalog budget warnings. GitOrigin-RevId: 4dcb1ba7f17c0edb1a9d548f9568e0c734bffc15
This commit is contained in:
1 parent
c8957bbf0f
commit
5dd992acd3
15 files changed
+592
-65
No files matched your search
@@ -11,6 +11,7 @@ use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionEventSink;
|
||||
use codex_extension_api::ExtensionFuture;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_extension_api::ExtensionWarning;
|
||||
use codex_extension_api::PromptFragment;
|
||||
use codex_extension_api::SkillInvocationContributor;
|
||||
use codex_extension_api::SkillInvocationInput;
|
||||
@@ -27,9 +28,6 @@ use codex_extension_api::WorldStateSectionContribution;
|
||||
use codex_mcp::McpResourceClient;
|
||||
use codex_otel::MetricsClient;
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::protocol::Event;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::WarningEvent;
|
||||
|
||||
use crate::SkillsExtensionConfig;
|
||||
use crate::catalog::SkillCatalog;
|
||||
@@ -58,6 +56,7 @@ use crate::state::SkillsSessionState;
|
||||
use crate::state::SkillsThreadState;
|
||||
use crate::state::SkillsTurnState;
|
||||
use crate::tools::skill_tools;
|
||||
use crate::warnings::bounded_warnings;
|
||||
use crate::world_state::executor_skills_world_state_section;
|
||||
use crate::world_state::host_skills_world_state_section;
|
||||
|
||||
@@ -170,8 +169,8 @@ where
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
for warning in &catalog.warnings {
|
||||
self.emit_warning(thread_store.level_id(), warning.clone());
|
||||
for warning in bounded_warnings(&catalog.warnings) {
|
||||
self.emit_warning(thread_store.level_id(), /*turn_id*/ None, warning);
|
||||
}
|
||||
let include_usage = thread_store
|
||||
.get::<ModelInfo>()
|
||||
@@ -183,7 +182,7 @@ where
|
||||
capped_skill_metadata_budget(/*context_window*/ None),
|
||||
);
|
||||
if let Some(message) = rendered.warning_message {
|
||||
self.emit_warning(thread_store.level_id(), message);
|
||||
self.emit_warning(thread_store.level_id(), /*turn_id*/ None, message);
|
||||
}
|
||||
rendered
|
||||
.fragment
|
||||
@@ -247,7 +246,7 @@ where
|
||||
.get_or_init(EmittedCatalogBudgetWarnings::default)
|
||||
.insert(&message)
|
||||
{
|
||||
self.emit_warning(input.turn_id, message);
|
||||
self.emit_warning(input.thread_store.level_id(), Some(input.turn_id), message);
|
||||
}
|
||||
let executor_body = rendered.fragment.map(|fragment| fragment.body());
|
||||
let mut sections = vec![executor_skills_world_state_section(
|
||||
@@ -364,8 +363,8 @@ where
|
||||
.map(|executor_skills| executor_skills.0.clone())
|
||||
.unwrap_or_default();
|
||||
catalog.extend(self.list_skills(query, &thread_state).await);
|
||||
for warning in &catalog.warnings {
|
||||
self.emit_warning(&input.turn_id, warning.clone());
|
||||
for warning in bounded_warnings(&catalog.warnings) {
|
||||
self.emit_warning(thread_store.level_id(), Some(&input.turn_id), warning);
|
||||
}
|
||||
|
||||
let selected_entries = collect_explicit_skill_mentions(&input.user_input, &catalog);
|
||||
@@ -407,7 +406,7 @@ where
|
||||
metadata_budget,
|
||||
);
|
||||
if let Some(message) = rendered.warning_message {
|
||||
self.emit_warning(&input.turn_id, message);
|
||||
self.emit_warning(thread_store.level_id(), Some(&input.turn_id), message);
|
||||
}
|
||||
if let Some(fragment) = rendered.fragment {
|
||||
fragments.push(Box::new(fragment));
|
||||
@@ -435,7 +434,11 @@ where
|
||||
"Skill `{}` exceeded the main prompt context limit and was truncated.",
|
||||
entry.name
|
||||
);
|
||||
self.emit_warning(&input.turn_id, warning.clone());
|
||||
self.emit_warning(
|
||||
thread_store.level_id(),
|
||||
Some(&input.turn_id),
|
||||
warning.clone(),
|
||||
);
|
||||
warnings.push(warning);
|
||||
}
|
||||
let fragment = SkillInstructions {
|
||||
@@ -455,7 +458,11 @@ where
|
||||
}
|
||||
Err(message) => {
|
||||
let warning = format!("Failed to load skill `{}`: {message}", entry.name);
|
||||
self.emit_warning(&input.turn_id, warning.clone());
|
||||
self.emit_warning(
|
||||
thread_store.level_id(),
|
||||
Some(&input.turn_id),
|
||||
warning.clone(),
|
||||
);
|
||||
warnings.push(warning);
|
||||
}
|
||||
}
|
||||
@@ -542,10 +549,11 @@ impl<C> SkillsExtension<C> {
|
||||
.map_err(|err| err.message)
|
||||
}
|
||||
|
||||
fn emit_warning(&self, turn_id: &str, message: String) {
|
||||
self.event_sink.emit(Event {
|
||||
id: turn_id.to_string(),
|
||||
msg: EventMsg::Warning(WarningEvent { message }),
|
||||
fn emit_warning(&self, thread_id: &str, turn_id: Option<&str>, message: String) {
|
||||
self.event_sink.emit_warning(ExtensionWarning {
|
||||
thread_id: thread_id.to_string(),
|
||||
turn_id: turn_id.map(str::to_string),
|
||||
message,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -10,6 +10,7 @@ mod shadow_selection_experiment;
|
||||
mod sources;
|
||||
mod state;
|
||||
mod tools;
|
||||
mod warnings;
|
||||
mod world_state;
|
||||
|
||||
pub use config::SkillsExtensionConfig;
|
||||
|
||||
@@ -9,7 +9,7 @@ use serde::Serialize;
|
||||
|
||||
use crate::catalog::SkillCatalogEntry;
|
||||
use crate::render::truncate_catalog_skill_description;
|
||||
use crate::render::truncate_utf8_to_bytes;
|
||||
use crate::warnings::bounded_warnings;
|
||||
|
||||
use super::MAX_HANDLE_BYTES;
|
||||
use super::SkillToolAuthority;
|
||||
@@ -21,8 +21,6 @@ use super::skill_function_tool;
|
||||
use super::skill_tool_name;
|
||||
|
||||
const TOOL_NAME: &str = "list";
|
||||
const MAX_WARNINGS: usize = 4;
|
||||
const MAX_WARNING_BYTES: usize = 256;
|
||||
|
||||
#[derive(Deserialize, JsonSchema)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
@@ -76,7 +74,7 @@ impl ToolExecutor<ToolCall> for ListTool {
|
||||
.filter(|entry| entry.enabled && entry.authority == authority)
|
||||
.filter_map(listed_skill)
|
||||
.collect(),
|
||||
warnings: bounded_warnings(catalog.warnings),
|
||||
warnings: bounded_warnings(&catalog.warnings),
|
||||
};
|
||||
|
||||
external_json_output(&response)
|
||||
@@ -100,14 +98,3 @@ fn listed_skill(entry: SkillCatalogEntry) -> Option<ListedSkill> {
|
||||
main_resource: entry.main_prompt.as_str().to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
fn bounded_warnings(warnings: Vec<String>) -> Vec<String> {
|
||||
warnings
|
||||
.into_iter()
|
||||
.take(MAX_WARNINGS)
|
||||
.map(|warning| {
|
||||
let (warning, _) = truncate_utf8_to_bytes(&warning, MAX_WARNING_BYTES);
|
||||
warning
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
@@ -0,0 +1,12 @@
|
||||
use crate::render::truncate_utf8_to_bytes;
|
||||
|
||||
const MAX_WARNINGS: usize = 4;
|
||||
const MAX_WARNING_BYTES: usize = 256;
|
||||
|
||||
pub(crate) fn bounded_warnings(warnings: &[String]) -> Vec<String> {
|
||||
warnings
|
||||
.iter()
|
||||
.take(MAX_WARNINGS)
|
||||
.map(|warning| truncate_utf8_to_bytes(warning, MAX_WARNING_BYTES).0)
|
||||
.collect()
|
||||
}
|
||||
@@ -12,6 +12,7 @@ use codex_extension_api::ConversationHistory;
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionEventSink;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_extension_api::ExtensionWarning;
|
||||
use codex_extension_api::NoopTurnItemEmitter;
|
||||
use codex_extension_api::PreviousWorldStateSection;
|
||||
use codex_extension_api::ThreadStartInput;
|
||||
@@ -23,7 +24,6 @@ use codex_models_manager::model_info::model_info_from_slug;
|
||||
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;
|
||||
@@ -540,11 +540,9 @@ async fn extreme_budget_pressure_removes_descriptions_before_omitting_entries()
|
||||
assert!(!rendered.contains("description-"));
|
||||
assert!(rendered.contains("additional skills omitted from this bounded skills list"));
|
||||
let omitted_count = 200 - included_count;
|
||||
let event = event_rx.try_recv()?;
|
||||
assert_eq!("thread", event.id);
|
||||
let EventMsg::Warning(warning) = event.msg else {
|
||||
panic!("expected catalog budget warning");
|
||||
};
|
||||
let warning = event_rx.try_recv()?.into_warning();
|
||||
assert_eq!(warning.thread_id, "thread");
|
||||
assert_eq!(warning.turn_id, None);
|
||||
assert_eq!(
|
||||
warning.message,
|
||||
format!(
|
||||
@@ -673,9 +671,9 @@ async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult {
|
||||
.contribute_thread_context(&session_store, &thread_store)
|
||||
.await;
|
||||
assert!(initial_fragments.is_empty());
|
||||
let EventMsg::Warning(warning) = event_rx.try_recv()?.msg else {
|
||||
panic!("expected warning event");
|
||||
};
|
||||
let warning = event_rx.try_recv()?.into_warning();
|
||||
assert_eq!(warning.thread_id, thread_store.level_id());
|
||||
assert_eq!(warning.turn_id, None);
|
||||
assert_eq!(
|
||||
warning.message,
|
||||
"orchestrator skills unavailable: temporary orchestrator failure"
|
||||
@@ -698,6 +696,13 @@ async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult {
|
||||
)
|
||||
.await;
|
||||
assert!(fragments.is_empty());
|
||||
let warning = event_rx.try_recv()?.into_warning();
|
||||
assert_eq!(warning.thread_id, thread_store.level_id());
|
||||
assert_eq!(warning.turn_id.as_deref(), Some(turn_id));
|
||||
assert_eq!(
|
||||
warning.message,
|
||||
"orchestrator skills unavailable: temporary orchestrator failure"
|
||||
);
|
||||
}
|
||||
assert_eq!(1, list_calls.load(Ordering::Relaxed));
|
||||
|
||||
@@ -916,11 +921,9 @@ async fn model_context_window_scales_executor_catalog_but_not_thread_catalog() -
|
||||
turn_store: &turn_store,
|
||||
})
|
||||
.await;
|
||||
let event = event_rx.try_recv()?;
|
||||
assert_eq!("turn-1", event.id);
|
||||
let EventMsg::Warning(warning) = event.msg else {
|
||||
panic!("expected catalog budget warning");
|
||||
};
|
||||
let warning = event_rx.try_recv()?.into_warning();
|
||||
assert_eq!(warning.thread_id, thread_store.level_id());
|
||||
assert_eq!(warning.turn_id.as_deref(), Some("turn-1"));
|
||||
assert!(
|
||||
warning
|
||||
.message
|
||||
@@ -947,6 +950,84 @@ async fn model_context_window_scales_executor_catalog_but_not_thread_catalog() -
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn executor_catalog_emits_at_most_four_warnings() -> TestResult {
|
||||
let executor_provider = Arc::new(StaticSkillProvider {
|
||||
catalog: SkillCatalog {
|
||||
entries: Vec::new(),
|
||||
warnings: (0..6).map(|index| format!("warning-{index}")).collect(),
|
||||
},
|
||||
read_requests: Arc::new(Mutex::new(Vec::new())),
|
||||
list_calls: None,
|
||||
fail_first_list: false,
|
||||
});
|
||||
let providers = SkillProviders::new().with_executor_provider(executor_provider);
|
||||
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, 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.bundled_skills_enabled = false;
|
||||
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,
|
||||
session_store: &session_store,
|
||||
thread_store: &thread_store,
|
||||
})
|
||||
.await;
|
||||
let selected_roots = vec![SelectedCapabilityRoot {
|
||||
id: "skills".to_string(),
|
||||
location: CapabilityRootLocation::Environment {
|
||||
environment_id: "env-1".to_string(),
|
||||
path: PathUri::parse("file:///skills").expect("skill root URI"),
|
||||
},
|
||||
}];
|
||||
let turn_store = ExtensionData::new("turn-1");
|
||||
|
||||
registry.context_contributors()[0]
|
||||
.contribute_world_state(WorldStateContributionInput {
|
||||
thread_id: codex_protocol::ThreadId::new(),
|
||||
turn_id: "turn-1",
|
||||
environments: &[],
|
||||
ready_selected_capability_roots: &selected_roots,
|
||||
executor_capability_discovery: None,
|
||||
session_store: &session_store,
|
||||
thread_store: &thread_store,
|
||||
turn_store: &turn_store,
|
||||
})
|
||||
.await;
|
||||
registry.turn_input_contributors()[0]
|
||||
.contribute(
|
||||
TurnInputContext {
|
||||
turn_id: "turn-1".to_string(),
|
||||
user_input: Vec::new(),
|
||||
environments: Vec::new(),
|
||||
},
|
||||
&session_store,
|
||||
&thread_store,
|
||||
&turn_store,
|
||||
)
|
||||
.await;
|
||||
|
||||
let messages = event_rx
|
||||
.try_iter()
|
||||
.map(|event| event.into_warning().message)
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
messages,
|
||||
vec!["warning-0", "warning-1", "warning-2", "warning-3"]
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult {
|
||||
let read_requests = Arc::new(Mutex::new(Vec::new()));
|
||||
@@ -1033,11 +1114,30 @@ struct StaticSkillProvider {
|
||||
fail_first_list: bool,
|
||||
}
|
||||
|
||||
struct ChannelEventSink(std::sync::mpsc::Sender<Event>);
|
||||
#[derive(Debug)]
|
||||
enum CapturedExtensionEvent {
|
||||
Event(Box<Event>),
|
||||
Warning(ExtensionWarning),
|
||||
}
|
||||
|
||||
impl CapturedExtensionEvent {
|
||||
fn into_warning(self) -> ExtensionWarning {
|
||||
match self {
|
||||
Self::Warning(warning) => warning,
|
||||
Self::Event(event) => panic!("expected extension warning, got {event:?}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
struct ChannelEventSink(std::sync::mpsc::Sender<CapturedExtensionEvent>);
|
||||
|
||||
impl ExtensionEventSink for ChannelEventSink {
|
||||
fn emit(&self, event: Event) {
|
||||
let _ = self.0.send(event);
|
||||
let _ = self.0.send(CapturedExtensionEvent::Event(Box::new(event)));
|
||||
}
|
||||
|
||||
fn emit_warning(&self, warning: ExtensionWarning) {
|
||||
let _ = self.0.send(CapturedExtensionEvent::Warning(warning));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user