mirror of
https://github.com/openai/codex.git
synced 2026-10-21 09:54:56 +00:00
Skip host skill discovery for Guardian reviews (#49584)
## Why Guardian does not consume skills, but host skill discovery can block its startup or turn creation when the primary executor is offline. ## What changed Skip plugin and skill warmup during Guardian session initialization, and use an empty host skills snapshot when building Guardian turn contexts. ## Testing Add a regression test that discovers skills for the parent session, disconnects its primary executor, and verifies that Guardian still reviews and denies a network permission request. GitOrigin-RevId: 3c12129721c82cb78a3fa618d6b05faea631966a
This commit is contained in:
@@ -619,7 +619,12 @@ async fn warm_plugins_and_skills_for_session_init(
|
||||
skills_service: Arc<HostSkillsService>,
|
||||
turn_environments: &TurnEnvironmentSnapshot,
|
||||
extensions: &codex_extension_api::ExtensionRegistry<Config>,
|
||||
session_source: &SessionSource,
|
||||
) -> Vec<SkillError> {
|
||||
// Guardian does not consume skills, including the legacy empty-registry catalog.
|
||||
if crate::guardian::is_basic_session_source(session_source) {
|
||||
return Vec::new();
|
||||
}
|
||||
let plugins_input = config.plugins_config_input();
|
||||
let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await;
|
||||
if config.features.enabled(Feature::SkipHostSkillDiscovery)
|
||||
@@ -1450,6 +1455,7 @@ impl Session {
|
||||
Arc::clone(&skills_service),
|
||||
&resolved_environments,
|
||||
extensions.as_ref(),
|
||||
&session_configuration.session_source,
|
||||
)
|
||||
.instrument(info_span!(
|
||||
"session_init.plugin_skill_warmup",
|
||||
|
||||
@@ -1205,6 +1205,7 @@ impl Session {
|
||||
per_turn_config.codex_home.as_path(),
|
||||
);
|
||||
let skills_snapshot = if matches!(build_mode, TurnContextBuildMode::InjectItems)
|
||||
|| crate::guardian::is_basic_session_source(&session_configuration.session_source)
|
||||
|| (per_turn_config
|
||||
.features
|
||||
.enabled(Feature::SkipHostSkillDiscovery)
|
||||
|
||||
@@ -291,6 +291,174 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn guardian_reviews_with_offline_primary_executor() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
let listener = TcpListener::bind("127.0.0.1:0").await?;
|
||||
let executor_url = format!("ws://{}", listener.local_addr()?);
|
||||
let (attach, connection) = tokio::sync::oneshot::channel();
|
||||
let (disconnect, stop) = tokio::sync::oneshot::channel();
|
||||
let executor = AbortOnDropHandle::new(tokio::spawn(serve_environment_with_instruction_files(
|
||||
listener,
|
||||
"",
|
||||
Some(
|
||||
"---\nname: guardian-fixture-skill\ndescription: Source executor skill catalog entry.\n---\nUse the source skill.\n",
|
||||
),
|
||||
connection,
|
||||
stop,
|
||||
)));
|
||||
attach.send(()).expect("attach primary executor");
|
||||
|
||||
let environment_id = "guardian-primary";
|
||||
let (request_permission, pending_permission) = tokio::sync::oneshot::channel();
|
||||
let (model, _) = start_streaming_sse_server(vec![
|
||||
vec![StreamingSseChunk {
|
||||
gate: Some(pending_permission),
|
||||
body: sse(vec![
|
||||
ev_function_call(
|
||||
"permission",
|
||||
"request_permissions",
|
||||
&json!({
|
||||
"environment_id": environment_id,
|
||||
"permissions": {"network": {"enabled": true}},
|
||||
"reason": "Review permission while the executor is offline.",
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("parent"),
|
||||
]),
|
||||
}],
|
||||
vec![StreamingSseChunk {
|
||||
gate: None,
|
||||
body: sse(vec![
|
||||
ev_assistant_message("deny", r#"{"outcome":"deny"}"#),
|
||||
ev_completed("review"),
|
||||
]),
|
||||
}],
|
||||
vec![StreamingSseChunk {
|
||||
gate: None,
|
||||
body: sse(vec![ev_completed("done")]),
|
||||
}],
|
||||
])
|
||||
.await;
|
||||
|
||||
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
|
||||
codex_skills_extension::install(&mut extensions, |config: &Config| {
|
||||
codex_skills_extension::SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
max_context_tokens: config.skill_max_context_tokens,
|
||||
bundled_skills_enabled: false,
|
||||
cloud_skill_enabled: false,
|
||||
shadow_selection_enabled: false,
|
||||
}
|
||||
});
|
||||
let test = test_codex()
|
||||
.with_extensions(Arc::new(extensions.build()))
|
||||
.with_model_info_override("guardian-offline-skills-parent", |info| {
|
||||
info.guardian = None;
|
||||
info.auto_review_model_override = Some(info.slug.clone());
|
||||
})
|
||||
.with_config(|config| {
|
||||
// Isolate skill discovery from the unchanged repository-instruction discovery path.
|
||||
config.project_doc_max_bytes = 0;
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
// Start the reviewer only after the executor goes offline, not during prewarm.
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile(PermissionProfile::read_only())
|
||||
.expect("set read-only permissions");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::RequestPermissionsTool)
|
||||
.expect("enable permission requests");
|
||||
config
|
||||
.features
|
||||
.disable(Feature::SkipHostSkillDiscovery)
|
||||
.expect("keep host skill discovery enabled for the parent");
|
||||
config
|
||||
.features
|
||||
.disable(Feature::DeferredExecutor)
|
||||
.expect("disable deferred executor");
|
||||
})
|
||||
// This test supplies its own executor mock, using host-native paths.
|
||||
.build_with_streaming_server(&model)
|
||||
.await?;
|
||||
let manager = test.thread_manager.environment_manager();
|
||||
manager.upsert_environment(
|
||||
environment_id.to_owned(),
|
||||
executor_url,
|
||||
/*connect_timeout*/ None,
|
||||
)?;
|
||||
manager
|
||||
.get_environment(environment_id)
|
||||
.context("primary executor")?
|
||||
.wait_until_ready()
|
||||
.await?;
|
||||
let mut primary = test.executor_environment().selection().clone();
|
||||
primary.environment_id = environment_id.to_owned();
|
||||
primary.config =
|
||||
EnvironmentConfigState::Ready(environment_config_for_selection(&test.config, &primary));
|
||||
|
||||
test.codex
|
||||
.start_or_steer_turn(
|
||||
TurnInputRequest::user_input(vec![UserInput::Text {
|
||||
text: "Do not grant network permission.".to_owned(),
|
||||
text_elements: Vec::new(),
|
||||
}])
|
||||
.with_thread_settings(ThreadSettingsOverrides {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
environments: Some(TurnEnvironmentSelections::new(
|
||||
test.config.cwd.clone(),
|
||||
vec![primary],
|
||||
)),
|
||||
..Default::default()
|
||||
}),
|
||||
)
|
||||
.await?;
|
||||
timeout(
|
||||
Duration::from_secs(/*secs*/ 10),
|
||||
model.wait_for_request_count(/*count*/ 1),
|
||||
)
|
||||
.await
|
||||
.context("parent must discover skills and reach the model while online")?;
|
||||
disconnect.send(()).expect("disconnect primary executor");
|
||||
executor.await?;
|
||||
request_permission.send(()).expect("request permission");
|
||||
|
||||
timeout(
|
||||
Duration::from_secs(/*secs*/ 10),
|
||||
model.wait_for_request_count(/*count*/ 2),
|
||||
)
|
||||
.await
|
||||
.context("Guardian startup and turn creation must not wait for host skill discovery")?;
|
||||
let requests = model.requests().await;
|
||||
let parent: Value = serde_json::from_slice(&requests[0])?;
|
||||
let review: Value = serde_json::from_slice(&requests[1])?;
|
||||
assert!(
|
||||
parent["input"]
|
||||
.to_string()
|
||||
.contains("guardian-fixture-skill"),
|
||||
"parent must advertise the skill discovered on its executor"
|
||||
);
|
||||
assert_eq!(
|
||||
review["client_metadata"]["x-openai-subagent"],
|
||||
json!("guardian")
|
||||
);
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::GuardianAssessment(assessment)
|
||||
if assessment.status == GuardianAssessmentStatus::Denied)
|
||||
})
|
||||
.await;
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
model.shutdown().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn guardian_revalidates_allow_with_offline_secondary_executor() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -1446,6 +1446,19 @@ async fn serve_environment_with_agents_md(
|
||||
listener: TcpListener,
|
||||
contents: &str,
|
||||
attach: tokio::sync::oneshot::Receiver<()>,
|
||||
shutdown: tokio::sync::oneshot::Receiver<()>,
|
||||
) -> usize {
|
||||
serve_environment_with_instruction_files(
|
||||
listener, contents, /*skill*/ None, attach, shutdown,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn serve_environment_with_instruction_files(
|
||||
listener: TcpListener,
|
||||
contents: &str,
|
||||
skill: Option<&str>,
|
||||
attach: tokio::sync::oneshot::Receiver<()>,
|
||||
mut shutdown: tokio::sync::oneshot::Receiver<()>,
|
||||
) -> usize {
|
||||
let mut websocket = accept_initialized_exec_server(listener).await;
|
||||
@@ -1461,6 +1474,10 @@ async fn serve_environment_with_agents_md(
|
||||
let is_agents_md = request["params"]["path"]
|
||||
.as_str()
|
||||
.is_some_and(|path| path.ends_with("/AGENTS.md"));
|
||||
let is_skill_root = skill.is_some()
|
||||
&& request["params"]["path"]
|
||||
.as_str()
|
||||
.is_some_and(|path| path.ends_with("/.agents/skills"));
|
||||
let response = match request["method"].as_str() {
|
||||
Some("environment/info") => json!({
|
||||
"id": request["id"],
|
||||
@@ -1470,16 +1487,38 @@ async fn serve_environment_with_agents_md(
|
||||
"id": request["id"],
|
||||
"result": { "path": request["params"]["path"] }
|
||||
}),
|
||||
Some("fs/walk") => json!({
|
||||
"id": request["id"],
|
||||
"result": { "entries": [], "errors": [], "truncated": false }
|
||||
}),
|
||||
Some("fs/getMetadata") if is_agents_md => {
|
||||
Some("fs/walk") => {
|
||||
let root = request["params"]["path"].as_str().expect("walk root");
|
||||
let entries = if is_skill_root {
|
||||
vec![json!({
|
||||
"path": format!("{root}/guardian-fixture-skill/SKILL.md"),
|
||||
"kind": "file",
|
||||
})]
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
json!({
|
||||
"id": request["id"],
|
||||
"result": { "entries": entries, "errors": [], "truncated": false }
|
||||
})
|
||||
}
|
||||
Some("fs/readFile")
|
||||
if skill.is_some()
|
||||
&& request["params"]["path"]
|
||||
.as_str()
|
||||
.is_some_and(|path| path.ends_with("/guardian-fixture-skill/SKILL.md")) =>
|
||||
{
|
||||
json!({
|
||||
"id": request["id"],
|
||||
"result": { "dataBase64": BASE64_STANDARD.encode(skill.expect("skill contents")) }
|
||||
})
|
||||
}
|
||||
Some("fs/getMetadata") if is_agents_md || is_skill_root => {
|
||||
json!({
|
||||
"id": request["id"],
|
||||
"result": {
|
||||
"isDirectory": false,
|
||||
"isFile": true,
|
||||
"isDirectory": is_skill_root,
|
||||
"isFile": is_agents_md,
|
||||
"isSymlink": false,
|
||||
"size": contents.len(),
|
||||
"createdAtMs": 0,
|
||||
|
||||
Reference in New Issue
Block a user