diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index fe929e0a59..21d0038872 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -17,6 +17,7 @@ use codex_extension_api::ExtensionWarning; use codex_features::Feature; use codex_login::CodexAuth; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; +use codex_protocol::openai_models::TruncationPolicyConfig; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; use codex_protocol::user_input::UserInput; @@ -825,6 +826,8 @@ async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Resu const REFERENCED_RESOURCE: &str = "skill://demo/explicit-only/references/guide.md"; const REFERENCED_CONTENTS: &str = "# Referenced guide"; const READ_CALL_ID: &str = "read-explicit-only-resource"; + const LIST_CALL_ID: &str = "list-model-visible-skills"; + const CODE_MODE_LIST_CALL_ID: &str = "list-skills-through-code-mode"; const MAIN_READ_CALL_ID: &str = "read-explicit-only-main"; const REPEATED_MAIN_READ_CALL_ID: &str = "read-explicit-only-main-again"; const INVALID_CURSOR_CALL_ID: &str = "read-explicit-only-invalid-cursor"; @@ -850,6 +853,18 @@ async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Resu }) .to_string(), ), + responses::ev_function_call_with_namespace( + LIST_CALL_ID, + "skills", + "list", + &json!({ "authority": { "kind": "orchestrator" } }).to_string(), + ), + responses::ev_custom_tool_call( + CODE_MODE_LIST_CALL_ID, + "exec", + r#"const result = await tools.skills__list({ authority: { kind: "orchestrator" } }); +text({ names: result.skills.map(skill => skill.name), warnings: result.warnings, next_cursor: result.next_cursor });"#, + ), ev_completed("resp-1"), ]), sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), @@ -972,9 +987,16 @@ async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Resu // Local executors disable orchestrator skill discovery. .with_exec_server_url("none") .with_extensions(Arc::new(extensions.build())) + .with_model_info_override("gpt-5.5", |model_info| { + model_info.truncation_policy = TruncationPolicyConfig::bytes(/*limit*/ 250); + }) .with_config(|config| { config.include_skill_instructions = true; config.orchestrator_skills_enabled = true; + config + .features + .enable(Feature::CodeMode) + .expect("code mode should be configurable in tests"); }); let test = builder.build_with_auto_env(&server).await?; wait_for_mcp_server(&test.codex, CODEX_APPS_MCP_SERVER_NAME).await?; @@ -1034,6 +1056,25 @@ async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Resu "next_cursor": null, }) ); + let mut list_output = requests[1] + .function_call_output_text(LIST_CALL_ID) + .expect("skills.list should return the model-visible catalog"); + let code_mode_output = requests[1].custom_tool_call_output(CODE_MODE_LIST_CALL_ID); + let code_mode_text = code_mode_output["output"] + .as_array() + .and_then(|items| items.last()) + .and_then(|item| item["text"].as_str()) + .ok_or_else(|| { + anyhow::anyhow!("Code Mode should return its skills.list result: {code_mode_output}") + })?; + assert_eq!( + serde_json::from_str::(code_mode_text)?, + json!({ + "names": ["demo:visible", "demo:missing-policy", "demo:non-boolean-policy"], + "warnings": [], + "next_cursor": null, + }) + ); let events = wait_for_analytics_events(&server, "skill_invocation", /*expected_count*/ 1).await; assert_eq!(events.len(), 1); assert_eq!(events[0]["skill_name"], "demo:explicit-only"); @@ -1068,6 +1109,59 @@ async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Resu ); assert_eq!(events[1]["event_params"]["invoke_type"], "implicit"); + for (name, has_more) in [ + ("visible", true), + ("missing-policy", true), + ("non-boolean-policy", false), + ] { + assert!(list_output.len() <= 300); + let list_response = serde_json::from_str::(&list_output)?; + let next_cursor = list_response["next_cursor"].as_str(); + assert_eq!(next_cursor.is_some(), has_more); + assert_eq!( + list_response, + json!({ + "skills": [{ + "authority": {"kind": "orchestrator"}, + "package": format!("skill://demo/{name}"), + "name": format!("demo:{name}"), + "description": "", + "main_resource": format!("skill://demo/{name}/SKILL.md"), + }], + "warnings": [], + "next_cursor": next_cursor, + }) + ); + if let Some(cursor) = next_cursor { + let call_id = format!("list-after-{name}"); + let page = responses::mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created(&call_id), + responses::ev_function_call_with_namespace( + &call_id, + "skills", + "list", + &json!({ + "authority": { "kind": "orchestrator" }, + "cursor": cursor, + }) + .to_string(), + ), + ev_completed(&call_id), + ]), + sse(vec![ev_response_created("listed"), ev_completed("listed")]), + ], + ) + .await; + test.submit_turn("Continue listing skills.").await?; + list_output = page.requests()[1] + .function_call_output_text(&call_id) + .expect("skills.list should return the next page"); + } + } + Ok(()) } diff --git a/codex-rs/ext/skills/src/tools/list.rs b/codex-rs/ext/skills/src/tools/list.rs index 3885716621..bb79b9e17d 100644 --- a/codex-rs/ext/skills/src/tools/list.rs +++ b/codex-rs/ext/skills/src/tools/list.rs @@ -20,13 +20,13 @@ use super::SkillToolAuthority; use super::SkillToolAuthoritySelector; use super::SkillToolContext; use super::is_bounded_handle; -use super::pagination_cursor; use super::parse_args; use super::parse_pagination_cursor; use super::serialized_len; use super::skill_function_tool; use super::skill_json_output; use super::skill_tool_name; +use super::value_fingerprint; const TOOL_NAME: &str = "list"; const MAX_SKILLS_PER_PAGE: usize = 20; @@ -78,49 +78,114 @@ impl ToolExecutor for ListTool { fn handle(&self, call: ToolCall) -> ToolExecutorFuture<'_> { Box::pin(async move { let args: ListArgs = parse_args(&call)?; + let response_byte_budget = call.response_byte_budget(MAX_SKILL_RESPONSE_BYTES); let catalog = self.context.catalog(&call.turn_id, args.authority).await; let mut omitted_oversized_entry = false; - let skills = catalog + let canonical_skills = catalog .entries .into_iter() .filter(|entry| { entry.is_model_visible() && args.authority.matches(&entry.authority) }) .filter_map(|entry| { - let listed = listed_skill(entry).filter(single_entry_response_is_bounded); + let listed = listed_skill(entry); omitted_oversized_entry |= listed.is_none(); listed }) .collect::>(); - let start = parse_pagination_cursor(args.cursor.as_deref(), &skills, "skills.list")?; - if start > skills.len() { + // Older cursors contain only the catalog fingerprint and offset. New cursors + // also record the budget whose omissions have already been reported. + let (cursor, previous_byte_budget) = match args.cursor.as_deref() { + Some(cursor) => match cursor.rsplit_once(':') { + Some((canonical_cursor, byte_budget)) => { + if canonical_cursor.contains(':') { + let byte_budget = byte_budget.parse::().map_err(|_| { + FunctionCallError::RespondToModel( + "skills.list cursor is invalid".to_string(), + ) + })?; + (Some(canonical_cursor), Some(byte_budget)) + } else { + (Some(cursor), None) + } + } + None => (Some(cursor), None), + }, + None => (None, None), + }; + let start = parse_pagination_cursor(cursor, &canonical_skills, "skills.list")?; + if start > canonical_skills.len() { return Err(FunctionCallError::RespondToModel( "skills.list cursor is invalid".to_string(), )); } - let mut warnings = if start == 0 { - let mut warnings = catalog.warnings; - if omitted_oversized_entry { - warnings.push(OVERSIZED_ENTRY_WARNING.to_string()); + let cursor_fingerprint = value_fingerprint(&canonical_skills); + let cursor_at = + |offset| format!("{cursor_fingerprint:016x}:{offset}:{response_byte_budget}"); + let mut skills = Vec::with_capacity(canonical_skills.len().saturating_sub(start)); + // The final retained skill needs no cursor, even if later entries are oversized. + for (index, skill) in canonical_skills.iter().enumerate().skip(start).rev() { + let next_cursor = (!skills.is_empty()).then(|| cursor_at(index.saturating_add(1))); + if single_entry_response_is_bounded(skill, response_byte_budget, next_cursor) { + skills.push((index, skill)); + } else { + omitted_oversized_entry = true; } - bounded_warnings(&warnings) + } + skills.reverse(); + let mut provider_warnings = if args.cursor.is_none() { + bounded_warnings(&catalog.warnings) } else { Vec::new() }; - let mut end = (start + MAX_SKILLS_PER_PAGE).min(skills.len()); + let omission_warning = (omitted_oversized_entry + && previous_byte_budget != Some(response_byte_budget)) + .then_some(OVERSIZED_ENTRY_WARNING); + let mut end = MAX_SKILLS_PER_PAGE.min(skills.len()); loop { - let response = ListResponse { - skills: skills[start..end].to_vec(), - warnings: warnings.clone(), - next_cursor: (end < skills.len()).then(|| pagination_cursor(&skills, end)), + let mut response = ListResponse { + skills: skills[..end] + .iter() + .map(|(_, skill)| (*skill).clone()) + .collect(), + warnings: provider_warnings + .iter() + .cloned() + .chain(omission_warning.map(str::to_string)) + .collect(), + next_cursor: (end < skills.len()) + .then(|| cursor_at(skills[end.saturating_sub(1)].0.saturating_add(1))), }; - if serialized_len(&response)? <= MAX_SKILL_RESPONSE_BYTES { + if serialized_len(&response)? <= response_byte_budget { return skill_json_output(&response, args.authority); } - if end.saturating_sub(start) > 1 { - end -= 1; - } else if !warnings.is_empty() { - warnings.clear(); + if end > 1 { + end = end.saturating_sub(1); + } else if provider_warnings.len() > 1 { + provider_warnings.pop(); + } else if !response.warnings.is_empty() { + response.skills.clear(); + // Recording this budget acknowledges the omission warning, even when + // the next call resumes at the same canonical skill offset. + response.next_cursor = Some(cursor_at(start)); + if serialized_len(&response)? <= response_byte_budget { + return skill_json_output(&response, args.authority); + } + if omission_warning.is_some() && !provider_warnings.is_empty() { + response.warnings = provider_warnings; + // Report discovery first without acknowledging an unshown omission. + // Reusing this offset lets the next call report it before advancing. + response.next_cursor = Some(format!("{cursor_fingerprint:016x}:{start}")); + if serialized_len(&response)? <= response_byte_budget { + return skill_json_output(&response, args.authority); + } + } + // FunctionCallError cannot carry external-context metadata, so do not + // expose provider warnings through this path. + return Err(FunctionCallError::RespondToModel( + "skills.list response budget leaves no room for discovery warnings" + .to_string(), + )); } else { return Err(FunctionCallError::RespondToModel( "skill metadata is too large to list".to_string(), @@ -131,13 +196,17 @@ impl ToolExecutor for ListTool { } } -fn single_entry_response_is_bounded(skill: &ListedSkill) -> bool { +fn single_entry_response_is_bounded( + skill: &ListedSkill, + response_byte_budget: usize, + next_cursor: Option, +) -> bool { serialized_len(&ListResponse { skills: vec![skill.clone()], warnings: Vec::new(), - next_cursor: Some(pagination_cursor(skill, usize::MAX)), + next_cursor, }) - .is_ok_and(|size| size <= MAX_SKILL_RESPONSE_BYTES) + .is_ok_and(|size| size <= response_byte_budget) } fn listed_skill(entry: SkillCatalogEntry) -> Option { diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 33f8a07f9e..960afe882e 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -17,6 +17,7 @@ use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionMetrics; use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ExtensionWarning; +use codex_extension_api::FunctionCallError; use codex_extension_api::NoopTurnItemEmitter; use codex_extension_api::PreviousWorldStateSection; use codex_extension_api::RenderedWorldStateFragment; @@ -1541,7 +1542,10 @@ async fn extreme_budget_pressure_removes_descriptions_before_omitting_entries() #[tokio::test] async fn skills_list_only_returns_model_visible_bounded_metadata() -> TestResult { - let description = "x".repeat(1_025); + const OVERSIZED_WARNING: &str = "Some skills were omitted because their metadata is too large."; + const PROVIDER_WARNING: &str = "Some orchestrator skills could not be discovered because the provider disconnected before returning the complete catalog."; + + let supplementary_warning = "w".repeat(256); let opaque_suffix = "\\".repeat(1_500); let mut entry = test_entry( SkillSourceKind::Orchestrator, @@ -1549,12 +1553,25 @@ async fn skills_list_only_returns_model_visible_bounded_metadata() -> TestResult &format!("orchestrator/{opaque_suffix}"), &format!("skill://orchestrator/{opaque_suffix}/SKILL.md"), ); - entry.description = description.clone(); + entry.description = "x".repeat(1_025); + let [first_entry, mut middle_entry, mut final_entry] = ["p0", "p1", "p2"].map(|name| { + test_entry( + SkillSourceKind::Orchestrator, + "codex_apps", + name, + &format!("skill://{name}/SKILL.md"), + ) + }); + middle_entry.description = "d".repeat(110); + final_entry.description = "d".repeat(50); let providers = SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { entries: vec![ entry, + first_entry, + middle_entry, + final_entry, test_entry( SkillSourceKind::Orchestrator, "codex_apps", @@ -1563,7 +1580,12 @@ async fn skills_list_only_returns_model_visible_bounded_metadata() -> TestResult ) .hidden_from_prompt(), ], - warnings: vec!["w".repeat(256); 4], + warnings: vec![ + PROVIDER_WARNING.to_string(), + supplementary_warning.clone(), + supplementary_warning.clone(), + supplementary_warning.clone(), + ], }, read_requests: Arc::new(Mutex::new(Vec::new())), list_calls: None, @@ -1597,39 +1619,162 @@ async fn skills_list_only_returns_model_visible_bounded_metadata() -> TestResult let payload = ToolPayload::Function { arguments: serde_json::json!({"authority": {"kind": "orchestrator"}}).to_string(), }; - let output = list_tool - .handle(ToolCall { - turn_id: "turn-1".to_string(), - call_id: "call-1".to_string(), - tool_name: list_tool.tool_name(), - model: "gpt-test".to_string(), - codex_turn_metadata: None, - truncation_policy: TruncationPolicy::Bytes(1_024), - source: ToolCallSource::Direct, - conversation_history: ConversationHistory::default(), - turn_item_emitter: Arc::new(NoopTurnItemEmitter), - environments: Vec::new(), - payload: payload.clone(), - }) - .await?; - let response = output + let call = ToolCall { + turn_id: "turn-1".to_string(), + call_id: "call-1".to_string(), + tool_name: list_tool.tool_name(), + model: "gpt-test".to_string(), + codex_turn_metadata: None, + truncation_policy: TruncationPolicy::Bytes(10_000), + source: ToolCallSource::Direct, + conversation_history: ConversationHistory::default(), + turn_item_emitter: Arc::new(NoopTurnItemEmitter), + environments: Vec::new(), + payload: payload.clone(), + }; + let output = list_tool.handle(call.clone()).await?; + let complete_response = output .post_tool_use_response("call-1", &payload) .ok_or("skills.list should expose structured output")?; - let rendered_description = response["skills"][0]["description"] - .as_str() - .ok_or("skills.list response should include a description")?; + let complete_skills = complete_response["skills"] + .as_array() + .ok_or("skills.list should return skills")?; + assert_eq!(complete_skills.len(), 4); + assert_eq!(complete_skills[0]["description"], "x".repeat(1_021) + "..."); + assert_eq!( + complete_response["warnings"].as_array().map(Vec::len), + Some(4) + ); + assert_eq!(complete_response["next_cursor"], serde_json::Value::Null); - assert_eq!(response["skills"].as_array().map(Vec::len), Some(1)); - assert_eq!(response["warnings"].as_array().map(Vec::len), Some(4)); - assert_eq!(response["next_cursor"], serde_json::Value::Null); - assert_eq!(rendered_description, "x".repeat(1_021) + "..."); - assert_ne!(rendered_description, description); + let mut cursor = None; + for ( + index, + (truncation_limit, byte_budget, expected_indices, expected_warnings, cursor_offset), + ) in [ + // The escaped entry fits alone, but not alongside even one provider warning. + (6_458, 7_750, vec![], vec![PROVIDER_WARNING], Some("0")), + (6_458, 7_750, vec![0], vec![], Some("1")), + (266, 320, vec![1], vec![], Some("2")), + // A smaller budget omits the middle entry and must report the new omission. + (183, 220, vec![], vec![OVERSIZED_WARNING], Some("2")), + (183, 220, vec![3], vec![], None), + // Neither report may hide the other when they need separate pages. + (183, 220, vec![], vec![PROVIDER_WARNING], Some("0")), + (183, 220, vec![], vec![OVERSIZED_WARNING], Some("0")), + // The last retained entry fits without a cursor, despite an oversized suffix. + (151, 182, vec![], vec![OVERSIZED_WARNING], Some("0")), + (151, 182, vec![1], vec![], None), + // An omission notice must not displace an affordable provider warning. + ( + 2_000, + 2_400, + vec![1, 2, 3], + vec![ + PROVIDER_WARNING, + &supplementary_warning, + &supplementary_warning, + &supplementary_warning, + OVERSIZED_WARNING, + ], + None, + ), + ] + .into_iter() + .enumerate() + { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ + "authority": {"kind": "orchestrator"}, + "cursor": cursor, + }) + .to_string(), + }; + let call_id = format!("bounded-page-{index}"); + let output = list_tool + .handle(ToolCall { + call_id: call_id.clone(), + truncation_policy: TruncationPolicy::Bytes(truncation_limit), + payload: payload.clone(), + ..call.clone() + }) + .await?; + assert!(output.contains_external_context()); + let response = output + .post_tool_use_response(&call_id, &payload) + .ok_or("skills.list should expose structured output")?; + assert!(serde_json::to_vec(&response)?.len() <= byte_budget); + let next_cursor = response["next_cursor"].as_str().map(str::to_owned); + assert_eq!( + next_cursor + .as_deref() + .and_then(|value| value.split(':').nth(1)), + cursor_offset + ); + let expected_skills = expected_indices + .into_iter() + .map(|index| &complete_skills[index]) + .collect::>(); + assert_eq!( + response, + serde_json::json!({ + "skills": expected_skills, + "warnings": expected_warnings, + "next_cursor": next_cursor, + }) + ); + if index == 2 { + let (legacy_cursor, _) = next_cursor + .as_deref() + .and_then(|cursor| cursor.rsplit_once(':')) + .ok_or("cursor should include its budget")?; + let legacy_payload = ToolPayload::Function { + arguments: serde_json::json!({ + "authority": {"kind": "orchestrator"}, + "cursor": legacy_cursor, + }) + .to_string(), + }; + let output = list_tool + .handle(ToolCall { + payload: legacy_payload.clone(), + ..call.clone() + }) + .await?; + assert_eq!( + output.post_tool_use_response(&call.call_id, &legacy_payload), + Some(serde_json::json!({ + "skills": &complete_skills[2..], + "warnings": [], + "next_cursor": null, + })) + ); + } + cursor = next_cursor; + } + + assert_eq!( + list_tool + .handle(ToolCall { + call_id: "omitted-call".to_string(), + truncation_policy: TruncationPolicy::Bytes(64), + ..call + }) + .await + .err(), + Some(FunctionCallError::RespondToModel( + "skills.list response budget leaves no room for discovery warnings".to_string() + )) + ); Ok(()) } #[tokio::test] async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult { + const PROVIDER_WARNING: &str = + "orchestrator skills unavailable: temporary orchestrator failure"; + let list_calls = Arc::new(AtomicUsize::new(0)); let providers = SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { @@ -1695,11 +1840,38 @@ async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult { 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!(warning.message, PROVIDER_WARNING); } + + let tools = registry.tool_contributors()[0].tools(&session_store, &thread_store); + let list_tool = tools + .iter() + .find(|tool| tool.tool_name().name == "list") + .ok_or("skills.list tool should be registered")?; + assert_eq!( + list_tool + .handle(ToolCall { + turn_id: "turn-1".to_string(), + call_id: "unavailable-skills".to_string(), + tool_name: list_tool.tool_name(), + model: "gpt-test".to_string(), + codex_turn_metadata: None, + truncation_policy: TruncationPolicy::Bytes(64), + source: ToolCallSource::Direct, + conversation_history: ConversationHistory::default(), + turn_item_emitter: Arc::new(NoopTurnItemEmitter), + environments: Vec::new(), + payload: ToolPayload::Function { + arguments: serde_json::json!({"authority": {"kind": "orchestrator"}}) + .to_string(), + }, + }) + .await + .err(), + Some(FunctionCallError::RespondToModel( + "skills.list response budget leaves no room for discovery warnings".to_string() + )) + ); assert_eq!(1, list_calls.load(Ordering::Relaxed)); Ok(())