mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat: Budget skill metadata and surface trimming as a warning (#18298)
Cap the model-visible skills section to a small share of the context window, with a fallback character budget, and keep only as many implicit skills as fit within that budget. Emit a non-fatal warning when enabled skills are omitted, and add a new app-server warning notification Record thread-start skill metrics for total enabled skills, kept skills, and whether truncation happened --------- Co-authored-by: Matthew Zeng <mzeng@openai.com> Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
a58a0f083d
commit
3f7222ec76
@@ -20,6 +20,7 @@ use crate::commit_attribution::commit_message_trailer_instruction;
|
||||
use crate::compact;
|
||||
use crate::config::ManagedFeatures;
|
||||
use crate::connectors;
|
||||
use crate::default_skill_metadata_budget;
|
||||
use crate::exec_policy::ExecPolicyManager;
|
||||
use crate::installation_id::resolve_installation_id;
|
||||
use crate::parse_turn_item;
|
||||
@@ -28,6 +29,7 @@ use crate::realtime_conversation::RealtimeConversationManager;
|
||||
use crate::render_skills_section;
|
||||
use crate::rollout::find_thread_name_by_id;
|
||||
use crate::session_prefix::format_subagent_notification_message;
|
||||
use crate::skills::SkillRenderSideEffects;
|
||||
use crate::skills_load_input_from_config;
|
||||
use crate::turn_metadata::TurnMetadataState;
|
||||
use async_channel::Receiver;
|
||||
@@ -362,9 +364,10 @@ pub struct Codex {
|
||||
|
||||
pub(crate) type SessionLoopTermination = Shared<BoxFuture<'static, ()>>;
|
||||
|
||||
/// Wrapper returned by [`Codex::spawn`] containing the spawned [`Codex`],
|
||||
/// the submission id for the initial `ConfigureSession` request and the
|
||||
/// unique session id.
|
||||
pub(crate) const THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE: &str = "Some enabled skills were not included in the model-visible skills list for this session. Mention a skill by name or path if you need it.";
|
||||
|
||||
/// Wrapper returned by [`Codex::spawn`] containing the spawned [`Codex`] and
|
||||
/// the unique session id.
|
||||
pub struct CodexSpawnOk {
|
||||
pub codex: Codex,
|
||||
pub thread_id: ThreadId,
|
||||
@@ -396,6 +399,7 @@ pub(crate) const INITIAL_SUBMIT_ID: &str = "";
|
||||
pub(crate) const SUBMISSION_CHANNEL_CAPACITY: usize = 512;
|
||||
const CYBER_VERIFY_URL: &str = "https://chatgpt.com/cyber";
|
||||
const CYBER_SAFETY_URL: &str = "https://developers.openai.com/codex/concepts/cyber-safety";
|
||||
|
||||
impl Codex {
|
||||
/// Spawn a new [`Codex`] and initialize the session.
|
||||
pub(crate) async fn spawn(args: CodexSpawnArgs) -> CodexResult<CodexSpawnOk> {
|
||||
@@ -2422,8 +2426,24 @@ impl Session {
|
||||
.turn_skills
|
||||
.outcome
|
||||
.allowed_skills_for_implicit_invocation();
|
||||
if let Some(skills_section) = render_skills_section(&implicit_skills) {
|
||||
developer_sections.push(skills_section);
|
||||
let rendered_skills = render_skills_section(
|
||||
&implicit_skills,
|
||||
default_skill_metadata_budget(turn_context.model_info.context_window),
|
||||
SkillRenderSideEffects::ThreadStart {
|
||||
session_telemetry: &self.services.session_telemetry,
|
||||
},
|
||||
);
|
||||
if let Some(rendered_skills) = rendered_skills {
|
||||
if rendered_skills.emit_warning {
|
||||
self.send_event_raw(Event {
|
||||
id: String::new(),
|
||||
msg: EventMsg::Warning(WarningEvent {
|
||||
message: THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE.to_string(),
|
||||
}),
|
||||
})
|
||||
.await;
|
||||
}
|
||||
developer_sections.push(rendered_skills.text);
|
||||
}
|
||||
let loaded_plugins = self
|
||||
.services
|
||||
|
||||
@@ -12,6 +12,8 @@ use crate::config_loader::project_trust_key;
|
||||
use crate::exec::ExecCapturePolicy;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::shell::default_user_shell;
|
||||
use crate::skills::SkillRenderSideEffects;
|
||||
use crate::skills::render::SkillMetadataBudget;
|
||||
use crate::tools::format_exec_output_str;
|
||||
|
||||
use codex_features::Feature;
|
||||
@@ -61,6 +63,11 @@ use codex_execpolicy::Decision;
|
||||
use codex_execpolicy::NetworkRuleProtocol;
|
||||
use codex_execpolicy::Policy;
|
||||
use codex_network_proxy::NetworkProxyConfig;
|
||||
use codex_otel::MetricsClient;
|
||||
use codex_otel::MetricsConfig;
|
||||
use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC;
|
||||
use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC;
|
||||
use codex_otel::THREAD_SKILLS_TRUNCATED_METRIC;
|
||||
use codex_otel::TelemetryAuthMode;
|
||||
use codex_protocol::config_types::CollaborationMode;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
@@ -86,6 +93,7 @@ use codex_protocol::protocol::RealtimeVoice;
|
||||
use codex_protocol::protocol::RealtimeVoicesList;
|
||||
use codex_protocol::protocol::ResumedHistory;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_protocol::protocol::Submission;
|
||||
use codex_protocol::protocol::ThreadRolledBackEvent;
|
||||
use codex_protocol::protocol::TokenCountEvent;
|
||||
@@ -106,10 +114,16 @@ use core_test_support::responses::mount_sse_once;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::test_path_buf;
|
||||
use core_test_support::tracing::install_test_tracing;
|
||||
use core_test_support::wait_for_event;
|
||||
use opentelemetry::trace::TraceContextExt;
|
||||
use opentelemetry::trace::TraceId;
|
||||
use opentelemetry_sdk::metrics::InMemoryMetricExporter;
|
||||
use opentelemetry_sdk::metrics::data::AggregatedMetrics;
|
||||
use opentelemetry_sdk::metrics::data::Metric;
|
||||
use opentelemetry_sdk::metrics::data::MetricData;
|
||||
use opentelemetry_sdk::metrics::data::ResourceMetrics;
|
||||
use std::path::Path;
|
||||
use std::time::Duration;
|
||||
use tokio::time::sleep;
|
||||
@@ -155,6 +169,54 @@ fn assistant_message(text: &str) -> ResponseItem {
|
||||
}
|
||||
}
|
||||
|
||||
fn test_session_telemetry_without_metadata() -> SessionTelemetry {
|
||||
let exporter = InMemoryMetricExporter::default();
|
||||
let metrics = MetricsClient::new(
|
||||
MetricsConfig::in_memory("test", "codex-core", env!("CARGO_PKG_VERSION"), exporter)
|
||||
.with_runtime_reader(),
|
||||
)
|
||||
.expect("in-memory metrics client");
|
||||
SessionTelemetry::new(
|
||||
ThreadId::new(),
|
||||
"gpt-5.1",
|
||||
"gpt-5.1",
|
||||
/*account_id*/ None,
|
||||
/*account_email*/ None,
|
||||
/*auth_mode*/ None,
|
||||
"test_originator".to_string(),
|
||||
/*log_user_prompts*/ false,
|
||||
"tty".to_string(),
|
||||
SessionSource::Cli,
|
||||
)
|
||||
.with_metrics_without_metadata_tags(metrics)
|
||||
}
|
||||
|
||||
fn find_metric<'a>(resource_metrics: &'a ResourceMetrics, name: &str) -> &'a Metric {
|
||||
for scope_metrics in resource_metrics.scope_metrics() {
|
||||
for metric in scope_metrics.metrics() {
|
||||
if metric.name() == name {
|
||||
return metric;
|
||||
}
|
||||
}
|
||||
}
|
||||
panic!("metric {name} missing");
|
||||
}
|
||||
|
||||
fn histogram_sum(resource_metrics: &ResourceMetrics, name: &str) -> u64 {
|
||||
let metric = find_metric(resource_metrics, name);
|
||||
match metric.data() {
|
||||
AggregatedMetrics::F64(data) => match data {
|
||||
MetricData::Histogram(histogram) => {
|
||||
let points: Vec<_> = histogram.data_points().collect();
|
||||
assert_eq!(points.len(), 1);
|
||||
points[0].sum().round() as u64
|
||||
}
|
||||
_ => panic!("unexpected histogram aggregation"),
|
||||
},
|
||||
_ => panic!("unexpected metric data type"),
|
||||
}
|
||||
}
|
||||
|
||||
fn skill_message(text: &str) -> ResponseItem {
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
@@ -4482,6 +4544,138 @@ async fn build_initial_context_omits_default_image_save_location_without_image_h
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_initial_context_trims_skill_metadata_from_context_window_budget() {
|
||||
let (session, mut turn_context) = make_session_and_context().await;
|
||||
let mut outcome = SkillLoadOutcome::default();
|
||||
outcome.skills = vec![
|
||||
SkillMetadata {
|
||||
name: "admin-skill".to_string(),
|
||||
description: "desc".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Admin,
|
||||
},
|
||||
SkillMetadata {
|
||||
name: "repo-skill".to_string(),
|
||||
description: "desc".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
];
|
||||
turn_context.model_info.context_window = Some(100);
|
||||
turn_context.turn_skills = TurnSkillsContext::new(Arc::new(outcome));
|
||||
|
||||
let initial_context = session.build_initial_context(&turn_context).await;
|
||||
let developer_texts = developer_input_texts(&initial_context);
|
||||
|
||||
assert!(
|
||||
developer_texts
|
||||
.iter()
|
||||
.all(|text| !text.contains(THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE)),
|
||||
"expected skill budget warning to stay out of the initial context, got {developer_texts:?}"
|
||||
);
|
||||
assert!(
|
||||
developer_texts
|
||||
.iter()
|
||||
.all(|text| !text.contains("- admin-skill:") && !text.contains("- repo-skill:")),
|
||||
"expected no skill metadata entries to fit the tiny budget, got {developer_texts:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() {
|
||||
let session_telemetry = test_session_telemetry_without_metadata();
|
||||
let rendered = render_skills_section(
|
||||
&[SkillMetadata {
|
||||
name: "repo-skill".to_string(),
|
||||
description: "desc".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
}],
|
||||
SkillMetadataBudget::Characters(1),
|
||||
SkillRenderSideEffects::ThreadStart {
|
||||
session_telemetry: &session_telemetry,
|
||||
},
|
||||
)
|
||||
.expect("skills should render");
|
||||
|
||||
assert!(rendered.emit_warning);
|
||||
let snapshot = session_telemetry
|
||||
.snapshot_metrics()
|
||||
.expect("runtime metrics snapshot");
|
||||
assert_eq!(
|
||||
histogram_sum(&snapshot, THREAD_SKILLS_ENABLED_TOTAL_METRIC),
|
||||
1
|
||||
);
|
||||
assert_eq!(histogram_sum(&snapshot, THREAD_SKILLS_KEPT_TOTAL_METRIC), 0);
|
||||
assert_eq!(histogram_sum(&snapshot, THREAD_SKILLS_TRUNCATED_METRIC), 1);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_builds() {
|
||||
let (session, turn_context, rx) = make_session_and_context_with_rx().await;
|
||||
let mut turn_context = Arc::into_inner(turn_context).expect("sole turn context owner");
|
||||
let mut outcome = SkillLoadOutcome::default();
|
||||
outcome.skills = vec![
|
||||
SkillMetadata {
|
||||
name: "admin-skill".to_string(),
|
||||
description: "desc".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Admin,
|
||||
},
|
||||
SkillMetadata {
|
||||
name: "repo-skill".to_string(),
|
||||
description: "desc".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
];
|
||||
turn_context.model_info.context_window = Some(100);
|
||||
turn_context.turn_skills = TurnSkillsContext::new(Arc::new(outcome));
|
||||
|
||||
let _ = session.build_initial_context(&turn_context).await;
|
||||
let warning_event = timeout(Duration::from_secs(1), rx.recv())
|
||||
.await
|
||||
.expect("warning event should arrive")
|
||||
.expect("warning event should be readable");
|
||||
assert!(matches!(
|
||||
warning_event.msg,
|
||||
EventMsg::Warning(WarningEvent { message })
|
||||
if message == THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE
|
||||
));
|
||||
|
||||
let _ = session.build_initial_context(&turn_context).await;
|
||||
let warning_event = timeout(Duration::from_secs(1), rx.recv())
|
||||
.await
|
||||
.expect("warning event should arrive on repeated build")
|
||||
.expect("warning event should be readable");
|
||||
assert!(matches!(
|
||||
warning_event.msg,
|
||||
EventMsg::Warning(WarningEvent { message })
|
||||
if message == THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn handle_output_item_done_records_image_save_history_message() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
|
||||
Reference in New Issue
Block a user