From a5cce8895a1400f94eb0a71771275284027fff17 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 30 Sep 2026 10:54:49 +0000 Subject: [PATCH] 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 --- codex-rs/core/src/session/session.rs | 6 + codex-rs/core/src/session/turn_context.rs | 1 + .../suite/guardian_environments_tests.rs | 168 ++++++++++++++++++ codex-rs/core/tests/suite/remote_env.rs | 53 +++++- 4 files changed, 221 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index e92aaeef8a..68a9fef1c0 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -619,7 +619,12 @@ async fn warm_plugins_and_skills_for_session_init( skills_service: Arc, turn_environments: &TurnEnvironmentSnapshot, extensions: &codex_extension_api::ExtensionRegistry, + session_source: &SessionSource, ) -> Vec { + // 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", diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 0d7b59d77e..777da7411b 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -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) diff --git a/codex-rs/core/tests/suite/guardian_environments_tests.rs b/codex-rs/core/tests/suite/guardian_environments_tests.rs index 2fceca3f12..452387a53e 100644 --- a/codex-rs/core/tests/suite/guardian_environments_tests.rs +++ b/codex-rs/core/tests/suite/guardian_environments_tests.rs @@ -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::::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(())); diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 4b8c919240..622a031e52 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -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,