mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
chore(multiagent) skills instructions toggle (#18596)
## Summary Support toggling the skills message off. ## Test Plan - [x] Updated unit tests
This commit is contained in:
committed by
GitHub
Unverified
parent
d58d3ccfec
commit
49403e3676
@@ -27,6 +27,10 @@ pub struct SkillsConfig {
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub bundled: Option<BundledSkillsConfig>,
|
||||
|
||||
/// Whether turns receive the automatic skills instructions block.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub include_instructions: Option<bool>,
|
||||
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub config: Vec<SkillConfig>,
|
||||
}
|
||||
|
||||
@@ -1902,6 +1902,10 @@
|
||||
"$ref": "#/definitions/SkillConfig"
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"include_instructions": {
|
||||
"description": "Whether turns receive the automatic skills instructions block.",
|
||||
"type": "boolean"
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
|
||||
@@ -292,6 +292,9 @@ consolidation_model = "gpt-5"
|
||||
fn parses_bundled_skills_config() {
|
||||
let cfg: ConfigToml = toml::from_str(
|
||||
r#"
|
||||
[skills]
|
||||
include_instructions = false
|
||||
|
||||
[skills.bundled]
|
||||
enabled = false
|
||||
"#,
|
||||
@@ -302,6 +305,7 @@ enabled = false
|
||||
cfg.skills,
|
||||
Some(SkillsConfig {
|
||||
bundled: Some(BundledSkillsConfig { enabled: false }),
|
||||
include_instructions: Some(false),
|
||||
config: Vec::new(),
|
||||
})
|
||||
);
|
||||
@@ -4888,6 +4892,7 @@ async fn test_precedence_fixture_with_o3_profile() -> std::io::Result<()> {
|
||||
guardian_policy_config: None,
|
||||
include_permissions_instructions: true,
|
||||
include_apps_instructions: true,
|
||||
include_skill_instructions: true,
|
||||
include_environment_context: true,
|
||||
compact_prompt: None,
|
||||
commit_attribution: None,
|
||||
@@ -5038,6 +5043,7 @@ async fn test_precedence_fixture_with_gpt3_profile() -> std::io::Result<()> {
|
||||
guardian_policy_config: None,
|
||||
include_permissions_instructions: true,
|
||||
include_apps_instructions: true,
|
||||
include_skill_instructions: true,
|
||||
include_environment_context: true,
|
||||
compact_prompt: None,
|
||||
commit_attribution: None,
|
||||
@@ -5186,6 +5192,7 @@ async fn test_precedence_fixture_with_zdr_profile() -> std::io::Result<()> {
|
||||
guardian_policy_config: None,
|
||||
include_permissions_instructions: true,
|
||||
include_apps_instructions: true,
|
||||
include_skill_instructions: true,
|
||||
include_environment_context: true,
|
||||
compact_prompt: None,
|
||||
commit_attribution: None,
|
||||
@@ -5319,6 +5326,7 @@ async fn test_precedence_fixture_with_gpt5_profile() -> std::io::Result<()> {
|
||||
guardian_policy_config: None,
|
||||
include_permissions_instructions: true,
|
||||
include_apps_instructions: true,
|
||||
include_skill_instructions: true,
|
||||
include_environment_context: true,
|
||||
compact_prompt: None,
|
||||
commit_attribution: None,
|
||||
@@ -6270,6 +6278,9 @@ include_apps_instructions = false
|
||||
include_environment_context = false
|
||||
profile = "chatty"
|
||||
|
||||
[skills]
|
||||
include_instructions = false
|
||||
|
||||
[profiles.chatty]
|
||||
include_permissions_instructions = true
|
||||
include_environment_context = true
|
||||
@@ -6284,6 +6295,7 @@ include_environment_context = true
|
||||
|
||||
assert!(config.include_permissions_instructions);
|
||||
assert!(!config.include_apps_instructions);
|
||||
assert!(!config.include_skill_instructions);
|
||||
assert!(config.include_environment_context);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -292,6 +292,9 @@ pub struct Config {
|
||||
/// Whether to inject the `<apps_instructions>` developer block.
|
||||
pub include_apps_instructions: bool,
|
||||
|
||||
/// Whether to inject the `<skills_instructions>` developer block.
|
||||
pub include_skill_instructions: bool,
|
||||
|
||||
/// Whether to inject the `<environment_context>` user block.
|
||||
pub include_environment_context: bool,
|
||||
|
||||
@@ -1929,6 +1932,11 @@ impl Config {
|
||||
.include_apps_instructions
|
||||
.or(cfg.include_apps_instructions)
|
||||
.unwrap_or(true);
|
||||
let include_skill_instructions = cfg
|
||||
.skills
|
||||
.as_ref()
|
||||
.and_then(|skills| skills.include_instructions)
|
||||
.unwrap_or(true);
|
||||
let include_environment_context = config_profile
|
||||
.include_environment_context
|
||||
.or(cfg.include_environment_context)
|
||||
@@ -2143,6 +2151,7 @@ impl Config {
|
||||
commit_attribution,
|
||||
include_permissions_instructions,
|
||||
include_apps_instructions,
|
||||
include_skill_instructions,
|
||||
include_environment_context,
|
||||
// The config.toml omits "_mode" because it's a config file. However, "_mode"
|
||||
// is important in code to differentiate the mode from the store implementation.
|
||||
|
||||
@@ -2343,28 +2343,30 @@ impl Session {
|
||||
developer_sections.push(apps_section);
|
||||
}
|
||||
}
|
||||
let implicit_skills = turn_context
|
||||
.turn_skills
|
||||
.outcome
|
||||
.allowed_skills_for_implicit_invocation();
|
||||
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;
|
||||
if turn_context.config.include_skill_instructions {
|
||||
let implicit_skills = turn_context
|
||||
.turn_skills
|
||||
.outcome
|
||||
.allowed_skills_for_implicit_invocation();
|
||||
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);
|
||||
}
|
||||
developer_sections.push(rendered_skills.text);
|
||||
}
|
||||
let loaded_plugins = self
|
||||
.services
|
||||
|
||||
@@ -19,6 +19,8 @@ use core_test_support::test_codex::TestCodex;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use std::time::Duration;
|
||||
use tokio::time::Instant;
|
||||
use tokio::time::sleep;
|
||||
@@ -101,6 +103,14 @@ fn role_block(description: &str, role_name: &str) -> Option<String> {
|
||||
Some(block.join("\n"))
|
||||
}
|
||||
|
||||
fn write_home_skill(codex_home: &Path, dir: &str, name: &str, description: &str) -> Result<()> {
|
||||
let skill_dir = codex_home.join("skills").join(dir);
|
||||
fs::create_dir_all(&skill_dir)?;
|
||||
let contents = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n");
|
||||
fs::write(skill_dir.join("SKILL.md"), contents)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn wait_for_spawned_thread_id(test: &TestCodex) -> Result<String> {
|
||||
let deadline = Instant::now() + Duration::from_secs(2);
|
||||
loop {
|
||||
@@ -507,6 +517,99 @@ async fn spawned_multi_agent_v2_child_receives_xml_tagged_developer_context() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn skills_toggle_skips_instructions_for_parent_and_spawned_child() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let spawn_args = serde_json::to_string(&json!({
|
||||
"message": CHILD_PROMPT,
|
||||
"task_name": "worker",
|
||||
}))?;
|
||||
let spawn_turn = mount_sse_once_match(
|
||||
&server,
|
||||
|req: &wiremock::Request| body_contains(req, TURN_1_PROMPT),
|
||||
sse(vec![
|
||||
ev_response_created("resp-turn1-1"),
|
||||
ev_function_call(SPAWN_CALL_ID, "spawn_agent", &spawn_args),
|
||||
ev_completed("resp-turn1-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let _child_request_log = mount_sse_once_match(
|
||||
&server,
|
||||
|req: &wiremock::Request| {
|
||||
body_contains(req, CHILD_PROMPT) && !body_contains(req, SPAWN_CALL_ID)
|
||||
},
|
||||
sse(vec![
|
||||
ev_response_created("resp-child-1"),
|
||||
ev_completed("resp-child-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let _turn1_followup = mount_sse_once_match(
|
||||
&server,
|
||||
|req: &wiremock::Request| body_contains(req, SPAWN_CALL_ID),
|
||||
sse(vec![
|
||||
ev_response_created("resp-turn1-2"),
|
||||
ev_assistant_message("msg-turn1-2", "parent done"),
|
||||
ev_completed("resp-turn1-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(err) = write_home_skill(home, "demo", "demo-skill", "demo skill") {
|
||||
panic!("write home skill: {err}");
|
||||
}
|
||||
})
|
||||
.with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Collab)
|
||||
.expect("test config should allow feature update");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::MultiAgentV2)
|
||||
.expect("test config should allow feature update");
|
||||
config.include_skill_instructions = false;
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn(TURN_1_PROMPT).await?;
|
||||
let parent_request = spawn_turn.single_request();
|
||||
assert!(!parent_request.body_contains_text("<skills_instructions>"));
|
||||
assert!(!parent_request.body_contains_text("demo-skill"));
|
||||
|
||||
let deadline = Instant::now() + Duration::from_secs(2);
|
||||
let child_request = loop {
|
||||
if let Some(request) = server
|
||||
.received_requests()
|
||||
.await
|
||||
.unwrap_or_default()
|
||||
.into_iter()
|
||||
.find(|request| {
|
||||
body_contains(request, CHILD_PROMPT)
|
||||
&& body_contains(request, "<spawned_agent_context>")
|
||||
&& !body_contains(request, SPAWN_CALL_ID)
|
||||
})
|
||||
{
|
||||
break request;
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
anyhow::bail!("timed out waiting for spawned child request");
|
||||
}
|
||||
sleep(Duration::from_millis(10)).await;
|
||||
};
|
||||
assert!(!body_contains(&child_request, "<skills_instructions>"));
|
||||
assert!(!body_contains(&child_request, "demo-skill"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn spawn_agent_role_overrides_requested_model_and_reasoning_settings() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
Reference in New Issue
Block a user