diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index febddbd766..f00a5d73ef 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -580,6 +580,14 @@ impl CatalogRequestProcessor { config.config_layer_stack, ) .with_plugin_skill_snapshots(plugin_skill_snapshots); + let skills_input = match self + .thread_manager + .environment_manager() + .default_environment_id() + { + Some(environment_id) => skills_input.with_environment_id(environment_id), + None => skills_input, + }; let snapshot = skills_request .snapshot_for_cwd(&skills_input, force_reload, fs) .await; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 64941d620f..7f106ca1d7 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -4317,6 +4317,18 @@ impl Session { let extension_data = codex_extension_api::ExtensionData::new(turn_context.sub_id.clone()); extension_data.insert_arc(Arc::clone(&settings.model_info.model)); + extension_data.insert_arc( + self.host_skills_for_step(&turn_context, &environments) + .await, + ); + if turn_context + .config + .features + .enabled(Feature::SkillCatalogDeltas) + && let Some(grants) = turn_context.turn_skill_filesystem_grants(&environments) + { + extension_data.insert(grants); + } if let Some(messages) = settings .model_info .model_messages diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index c11bb7b582..58c07de277 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -689,15 +689,17 @@ async fn warm_plugins_and_skills_for_session_init( effective_skill_roots, turn_environments, ); - let access = fs - .as_ref() - .map(codex_exec_server::FileSystemEnvironmentAccessor::unrestricted); + let access = + fs.map(|environment| environment.fs_accessor(/*additional_permissions*/ None)); skills_service - .snapshot_for_config( + .snapshot_for_config_on_executor( &skills_input, - access - .as_ref() - .map(|access| access as &dyn codex_file_system::EnvironmentAccess), + fs.zip(access.as_ref()).map(|(environment, access)| { + ( + environment.environment().filesystem_ref(), + access as &dyn codex_file_system::EnvironmentAccess, + ) + }), ) .await .outcome() diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 2d3f37e616..26edef6519 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -131,6 +131,7 @@ use codex_skills::build_skill_name_counts; use codex_skills::collect_explicit_skill_mentions; use codex_skills::tool_kind_for_path; use codex_skills_extension::HostSkillPrompts; +use codex_skills_extension::HostSkillsSnapshot; use codex_skills_extension::InjectedHostSkillPrompts; use codex_skills_extension::validate_required_skills; use codex_thread_store::PersistContext; @@ -1178,7 +1179,8 @@ async fn required_mcp_servers_for_input( } else { HashMap::new() }; - let skills_snapshot = turn_context.skills_snapshot(); + let environments = sess.services.turn_environments.snapshot().await; + let skills_snapshot = sess.host_skills_for_step(turn_context, &environments).await; let skills_outcome = skills_snapshot.outcome(); let mentioned_skills = collect_explicit_skill_mentions(user_input, skills_outcome, &connector_slug_counts); @@ -1249,7 +1251,8 @@ async fn build_skills_and_plugins( } else { Vec::new() }; - let skills_snapshot = turn_context.skills_snapshot(); + let skills_snapshot = + HostSkillsSnapshot::for_step(&step_context.extension_data, &turn_context.extension_data)?; let skills_outcome = skills_snapshot.outcome(); let connector_slug_counts = build_connector_slug_counts(&available_connectors); let extension_injection_items = diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 0bbbd5294e..c6d22d105b 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -11,6 +11,7 @@ use crate::exec_policy::AllowPrefixRules; use crate::shell_snapshot::ShellSnapshot; use crate::shell_snapshot::ShellSnapshotFile; use crate::shell_snapshot::ShellSnapshotSandbox; +use crate::skills::HostSkillDiscovery; use crate::tools::sandboxing::configured_windows_sandbox_selection; use crate::tools::sandboxing::executor_windows_sandbox_selection; use arc_swap::ArcSwap; @@ -438,7 +439,10 @@ impl TurnContext { ) -> Self { let session_telemetry = step_settings.telemetry(&self.session_telemetry); let extension_data = Arc::new(codex_extension_api::ExtensionData::new(sub_id.clone())); - extension_data.insert(self.skills_snapshot().as_ref().clone()); + extension_data.insert_arc(self.skills_snapshot()); + if let Some(discovery) = self.extension_data.get::() { + extension_data.insert_arc(discovery); + } Self { sub_id, @@ -510,6 +514,34 @@ impl TurnContext { merge_permission_profiles(session_grants.as_ref(), turn_grants.as_ref()) } + #[expect( + clippy::expect_used, + reason = "poisoned grant state must not authorize further operations" + )] + pub(crate) fn turn_skill_filesystem_grants( + &self, + environments: &TurnEnvironmentSnapshot, + ) -> Option { + let grants = self + .turn_grants + .lock() + .expect("turn permission grants lock poisoned"); + let environment_ids = environments + .turn_environments() + .map(|environment| &environment.selection.environment_id) + .filter(|id| { + grants + .granted_permissions_by_environment_id + .get(*id) + .and_then(|profile| profile.file_system.as_ref()) + .is_some_and(|fs| !fs.is_empty()) + }) + .cloned() + .collect::>(); + (!environment_ids.is_empty()) + .then_some(codex_skills_extension::TurnSkillFilesystemGrants { environment_ids }) + } + #[expect( clippy::expect_used, reason = "poisoned grant state must not authorize further operations" @@ -1294,6 +1326,10 @@ impl Session { } #[instrument(name = "turn_context.build", level = "trace", skip_all, fields(?build_mode))] + #[expect( + clippy::expect_used, + reason = "poisoned session grants must not authorize discovery" + )] async fn new_turn_context_from_configuration( &self, sub_id: String, @@ -1347,7 +1383,7 @@ impl Session { &plugin_outcome, per_turn_config.codex_home.as_path(), ); - let skills_snapshot = if matches!(build_mode, TurnContextBuildMode::InjectItems) + let skill_discovery = if matches!(build_mode, TurnContextBuildMode::InjectItems) || crate::guardian::is_basic_session_source(&session_configuration.session_source) || (per_turn_config .features @@ -1355,7 +1391,7 @@ impl Session { && !self.services.extensions.requires_host_skill_discovery()) { // Executor and orchestrator catalogs are supplied independently of host skills. - HostSkillsSnapshot::new(Arc::new(SkillLoadOutcome::default())) + None } else { let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); let (skills_input, fs) = crate::skills::skills_load_input_for_environments( @@ -1364,18 +1400,37 @@ impl Session { effective_skill_roots, &turn_environments, ); - let access = fs - .as_ref() - .map(codex_exec_server::FileSystemEnvironmentAccessor::unrestricted); - self.services + let access = fs.map(|environment| { + let grants = self + .services + .granted_permissions_by_environment_id + .lock() + .expect("session permission grants lock poisoned") + .get(&environment.selection.environment_id) + .cloned(); + environment.fs_accessor(grants) + }); + let snapshot = self + .services .skills_service - .snapshot_for_config( + .snapshot_for_config_on_executor( &skills_input, - access - .as_ref() - .map(|access| access as &dyn EnvironmentAccess), + fs.zip(access.as_ref()).map(|(environment, access)| { + ( + environment.environment().filesystem_ref(), + access as &dyn EnvironmentAccess, + ) + }), ) - .await + .await; + let repository = fs + .zip(access.as_ref()) + .map(|(environment, access)| (environment.cwd().clone(), access.cache_key())); + Some(HostSkillDiscovery { + input: Arc::new(skills_input), + repository, + snapshot: Arc::new(snapshot), + }) }; let step_settings = Arc::new(ResolvedStepSettings::new( Arc::clone(&session_configuration.step_settings), @@ -1407,8 +1462,14 @@ impl Session { turn_environments, cwd, sub_id, - skills_snapshot, + skill_discovery + .as_ref() + .map(|discovery| discovery.snapshot.as_ref().clone()) + .unwrap_or_else(|| HostSkillsSnapshot::new(Arc::new(SkillLoadOutcome::default()))), ); + if let Some(discovery) = skill_discovery { + turn_context.extension_data.insert(discovery); + } turn_context.code_mode_available = self.services.code_mode_service.is_available(); turn_context.extension_data.insert(trusted_plugin_roots); turn_context.active_host_plugin_identities = Some( diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 3fc1826191..5be89463e2 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -2,21 +2,24 @@ use crate::config::Config; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::session::turn_context::TurnEnvironment; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::SkillInvocationLocation; use codex_analytics::TrackEventsContext; use codex_analytics::build_track_events_context; use codex_core_plugins::PluginsManager; -use codex_exec_server::ExecutorFileSystem; use codex_extension_api::SkillInvocationInput; use codex_extension_api::SkillInvocationKind; +use codex_file_system::EnvironmentAccess; +use codex_file_system::EnvironmentAccessKey; use codex_otel::SkillInvocationEvent; use codex_otel::SkillInvocationType; use codex_otel::sanitize_metric_tag_value; use codex_protocol::protocol::SkillScope; use codex_skills::SkillMetadata; use codex_skills_extension::HostSkillsLoadInput; +use codex_skills_extension::HostSkillsSnapshot; use codex_skills_extension::InjectedHostSkillPrompts; use codex_skills_extension::detect_implicit_skill_invocation; use codex_skills_extension::record_plugin_turn_usage; @@ -44,19 +47,19 @@ pub(crate) fn skills_load_input_from_config( /// Pair repository discovery inputs with their captured filesystem owner. /// Keep preparation synchronous so it adds no nested startup or resume future. -pub(crate) fn skills_load_input_for_environments( +pub(crate) fn skills_load_input_for_environments<'a>( config: &Config, plugins_manager: &PluginsManager, effective_skill_roots: Vec, - environments: &TurnEnvironmentSnapshot, -) -> (HostSkillsLoadInput, Option>) { + environments: &'a TurnEnvironmentSnapshot, +) -> (HostSkillsLoadInput, Option<&'a TurnEnvironment>) { // Preserve primary-repository discovery independently of the host config cwd. let workspace = environments.primary().and_then(|environment| { environment .cwd() .to_abs_path() .ok() - .map(|cwd| (cwd, environment.environment().get_filesystem())) + .map(|cwd| (cwd, environment)) }); let (cwd, fs) = match workspace { Some((cwd, fs)) => (cwd, Some(fs)), @@ -70,9 +73,75 @@ pub(crate) fn skills_load_input_for_environments( .with_plugin_skill_snapshots( plugins_manager.plugin_skill_snapshots_for_config(&config.plugins_config_input()), ); + let input = match fs { + Some(environment) => input.with_environment_id(&environment.selection.environment_id), + None => input, + }; (input, fs) } +/// The snapshot and configuration resolved with one turn's filesystem access. +pub(crate) struct HostSkillDiscovery { + pub(crate) input: Arc, + pub(crate) repository: Option<(PathUri, EnvironmentAccessKey)>, + pub(crate) snapshot: Arc, +} + +impl Session { + pub(crate) async fn host_skills_for_step( + &self, + turn: &TurnContext, + environments: &TurnEnvironmentSnapshot, + ) -> Arc { + let Some(cached) = turn.extension_data.get::() else { + return turn.skills_snapshot(); + }; + let workspace = environments.primary().and_then(|environment| { + let cwd = environment.cwd().to_abs_path().ok()?; + let grants = turn.granted_permissions(&environment.selection.environment_id); + Some((environment, cwd, environment.fs_accessor(grants))) + }); + let repository = workspace + .as_ref() + .map(|(environment, _, access)| (environment.cwd().clone(), access.cache_key())); + if cached.repository == repository { + return Arc::clone(&cached.snapshot); + } + + let cwd = workspace + .as_ref() + .map_or_else(|| turn.config.cwd.clone(), |(_, cwd, _)| cwd.clone()); + let input = cached.input.for_cwd(cwd); + let input = Arc::new(match &workspace { + Some((environment, _, _)) => { + input.with_environment_id(&environment.selection.environment_id) + } + None => input, + }); + let snapshot = Box::pin( + self.services + .skills_service + .snapshot_for_config_on_executor( + &input, + workspace.as_ref().map(|(environment, _, access)| { + ( + environment.environment().filesystem_ref(), + access as &dyn EnvironmentAccess, + ) + }), + ), + ) + .await; + let snapshot = Arc::new(snapshot); + turn.extension_data.insert(HostSkillDiscovery { + input, + repository, + snapshot: Arc::clone(&snapshot), + }); + snapshot + } +} + pub(crate) async fn emit_explicit_skill_invocations( sess: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 62af6a89b9..5dfb175aee 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -1454,11 +1454,6 @@ async fn send_environment_info(websocket: &mut WebSocketStream) { .expect("environment info response"); } -async fn serve_environment_info(listener: TcpListener) { - let mut websocket = accept_initialized_exec_server(listener).await; - send_environment_info(&mut websocket).await; -} - pub(super) async fn serve_environment_with_agents_md( listener: TcpListener, contents: &str, @@ -1476,12 +1471,20 @@ async fn serve_environment_with_instruction_files( contents: &str, skill: Option<&str>, attach: tokio::sync::oneshot::Receiver<()>, - mut shutdown: tokio::sync::oneshot::Receiver<()>, + shutdown: tokio::sync::oneshot::Receiver<()>, ) -> usize { let mut websocket = accept_initialized_exec_server(listener).await; attach.await.expect("attach signal"); send_environment_info(&mut websocket).await; + serve_instruction_file_requests(websocket, contents, skill, shutdown).await +} +async fn serve_instruction_file_requests( + mut websocket: WebSocketStream, + contents: &str, + skill: Option<&str>, + mut shutdown: tokio::sync::oneshot::Receiver<()>, +) -> usize { let mut agents_md_reads = 0; loop { let request = tokio::select! { @@ -3561,7 +3564,15 @@ async fn deferred_executor_compaction_preserves_then_updates_environment_once() }) .await; - serve_environment_info(listener).await; + let mut websocket = accept_initialized_exec_server(listener).await; + send_environment_info(&mut websocket).await; + let (shutdown, shutdown_rx) = tokio::sync::oneshot::channel(); + let executor = tokio::spawn(serve_instruction_file_requests( + websocket, + "", + /*skill*/ None, + shutdown_rx, + )); test.codex .submit(Op::UserInputAnswer { id: request.turn_id, @@ -3648,6 +3659,8 @@ async fn deferred_executor_compaction_preserves_then_updates_environment_once() Some(&json!("zsh")) ); + shutdown.send(()).expect("stop executor"); + executor.await?; Ok(()) } diff --git a/codex-rs/core/tests/suite/remote_env_capability_roots_tests.rs b/codex-rs/core/tests/suite/remote_env_capability_roots_tests.rs index 334ccef433..0ccb145358 100644 --- a/codex-rs/core/tests/suite/remote_env_capability_roots_tests.rs +++ b/codex-rs/core/tests/suite/remote_env_capability_roots_tests.rs @@ -9,6 +9,64 @@ use codex_protocol::protocol::TurnEnvironmentSelection; use pretty_assertions::assert_eq; use test_case::test_case; +/// A Linux/macOS executor can exercise the old Windows enum boundary without launching MXC. +#[cfg(not(windows))] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn host_skill_discovery_reports_unsupported_remote_sandbox() -> Result<()> { + let executor = core_test_support::exec_server::ExecServerProcess::start().await?; + let environment = + codex_exec_server::Environment::create_for_tests(Some(executor.websocket_url.clone()))?; + assert!(!environment.info().await?.capabilities.windows_mxc); + let workspace = tempfile::tempdir()?; + std::fs::create_dir(workspace.path().join(".git"))?; + let root = workspace.path().join(".agents/skills"); + std::fs::create_dir_all(root.join("existing"))?; + std::fs::write( + root.join("existing/SKILL.md"), + "---\nname: existing\ndescription: Existing repository skill.\n---\nSECRET_SKILL_BODY\n", + )?; + let cwd = AbsolutePathBuf::from_absolute_path(workspace.path())?; + let input = codex_skills_extension::HostSkillsLoadInput::new( + cwd.clone(), + Vec::new(), + Default::default(), + ); + let service = codex_skills_extension::HostSkillsService::new_with_restriction_product( + cwd.clone(), + /*bundled_skills_enabled*/ false, + /*restriction_product*/ None, + ); + let fs = environment.get_filesystem(); + let unrestricted = codex_exec_server::FileSystemEnvironmentAccessor::unrestricted(&fs); + let snapshot = service + .snapshot_for_config_on_executor(&input, Some((&fs, &unrestricted))) + .await; + assert_eq!(snapshot.outcome().skills.len(), 1); + assert_eq!(snapshot.outcome().errors, Vec::new()); + + let mut sandbox = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( + PathUri::from_host_native_path(&root)?.into(), + FileSystemAccessMode::Read, + )]), + NetworkSandboxPolicy::Restricted, + ), + PathUri::from_abs_path(&cwd), + ); + sandbox.windows_sandbox_selection = codex_exec_server::WindowsSandboxSelection::Mxc; + let restricted = codex_exec_server::FileSystemEnvironmentAccessor::new(&fs, sandbox); + let snapshot = service + .snapshot_for_config_on_executor(&input, Some((&fs, &restricted))) + .await; + assert!(snapshot.outcome().skills.is_empty()); + assert_eq!(snapshot.outcome().errors, vec![codex_skills::SkillError { + path: AbsolutePathBuf::from_absolute_path(root)?, + message: "exec-server does not support the requested MXC Windows sandbox; update the executor".to_string(), + }]); + Ok(()) +} + /// Roots supplied at startup or loaded from history work again when their environment is reselected. #[test_case(false; "api roots")] #[test_case(true; "saved roots")] diff --git a/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs b/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs index 4b939e5187..ca910519fc 100644 --- a/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs +++ b/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs @@ -31,6 +31,8 @@ use pretty_assertions::assert_eq; use serde_json::json; use super::super::skills_extension::CatalogSkillProvider; +use super::super::skills_extension::RepositoryGrant; +use super::super::skills_extension::repository_skill_grant_history; #[test_case::test_case(false; "descriptions")] #[test_case::test_case(true; "omit_descriptions")] @@ -472,3 +474,57 @@ async fn skill_catalog_deltas_survive_resume_and_rebuild_after_compaction() -> R ); Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn repository_skill_turn_grant() -> Result<()> { + let Some((requests, skill_root)) = + repository_skill_grant_history(RepositoryGrant::Turn).await? + else { + return Ok(()); + }; + let withdrawn = requests + .iter() + .map(|request| { + request + .message_input_texts("developer") + .iter() + .any(|text| text.contains("Some previously listed skills are no longer available.")) + }) + .collect::>(); + assert_eq!(withdrawn, [false, false, true, true]); + assert!( + requests[3] + .message_input_texts("developer") + .iter() + .any(|text| { + text.contains("Some previously unavailable skills are available again.") + }) + ); + // Normalize the executor's temporary path before long tool arguments are clipped and hashed. + let skill_root = serde_json::to_string(&skill_root.inferred_native_path_string())?; + let mut bodies = requests + .iter() + .map(responses::ResponsesRequest::body_json) + .collect::>(); + for body in &mut bodies { + for item in body["input"].as_array_mut().expect("request input") { + for field in ["arguments", "output"] { + if let Some(serde_json::Value::String(text)) = item.get_mut(field) { + *text = text.replace(&skill_root, r#""/.agents/skills""#); + } + } + } + } + insta::assert_snapshot!( + "repository_skill_turn_grant", + context_snapshot::format_context_snapshot( + "A repository skill appears after approval. The next turn withdraws it, then restores it after reapproval.", + &bodies + .iter() + .map(context_snapshot::SnapshotEntry::body) + .collect::>(), + &ContextSnapshotOptions::default().include_request_settings(), + ) + ); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 20fbe6d4de..6f391c5163 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -63,6 +63,7 @@ use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::protocol::TurnEnvironmentRequests; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::protocol::TurnSettingsUpdate; use codex_protocol::protocol::TurnSettingsUpdateOutcome; @@ -583,13 +584,33 @@ async fn rendered_catalogs_for_turns( Ok((developer_texts, client_warning_messages)) } +#[derive(Clone, Copy)] +enum RepositoryReadAccess { + Full, + Repository, + OuterMarkerOnly, + NoMarkers, +} + +#[test_case(RepositoryReadAccess::Full; "full read access")] +#[test_case(RepositoryReadAccess::Repository; "repository-only read access")] +#[test_case(RepositoryReadAccess::OuterMarkerOnly; "inner repository marker hidden")] +#[test_case(RepositoryReadAccess::NoMarkers; "all repository markers hidden")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills() -> Result<()> { +async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills( + repository_read_access: RepositoryReadAccess, +) -> Result<()> { skip_if_wine_exec!( Ok(()), "executor-backed repo skills require matching host and executor path conventions" ); skip_if_no_network!(Ok(())); + if !matches!(repository_read_access, RepositoryReadAccess::Full) { + skip_if_target_windows!( + Ok(()), + "restricted reads require the elevated Windows sandbox backend" + ); + } const HOST_SKILL_BODY: &str = "Use the host skill instructions."; const REPO_SKILL_BODY: &str = "Use the repository skill instructions."; @@ -652,22 +673,38 @@ async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills() .with_extensions(Arc::new(extensions.build())) .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) .with_workspace_setup(|cwd, fs| async move { - let skill_dir = cwd.join(".agents/skills/repo-search"); fs.create_directory( - &PathUri::from_host_native_path(&skill_dir)?, + &PathUri::from_abs_path(&cwd), CreateDirectoryOptions { recursive: true, follow_symlinks: true }, /*sandbox*/ None, ) .await?; - fs.write_file( - &PathUri::from_host_native_path(skill_dir.join("SKILL.md"))?, - format!( - "---\nname: repo-search\ndescription: inspect repo data\n---\n\n{REPO_SKILL_BODY}\n" + let repository = cwd.parent().expect("inner repository"); + let outer = repository.parent().expect("outer repository"); + for (root, name) in [(&repository, "repo-search"), (&outer, "outer-search")] { + fs.write_file( + &PathUri::from_abs_path(&root.join(".git")), + b"gitdir: fixture\n".to_vec(), + Default::default(), /*sandbox*/ None, ) - .into_bytes(), - Default::default(), /*sandbox*/ None, - ) - .await?; + .await?; + let skill_dir = PathUri::from_abs_path(&root.join(".agents/skills").join(name)); + fs.create_directory( + &skill_dir, + CreateDirectoryOptions { recursive: true, follow_symlinks: true }, + /*sandbox*/ None, + ) + .await?; + fs.write_file( + &skill_dir.join("SKILL.md")?, + format!( + "---\nname: {name}\ndescription: inspect repo data\n---\n\n{REPO_SKILL_BODY}\n" + ) + .into_bytes(), + Default::default(), /*sandbox*/ None, + ) + .await?; + } Ok(()) }) .with_config(move |config| { @@ -676,12 +713,45 @@ async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills() .enable(Feature::Apps) .expect("test config should allow feature update"); config.chatgpt_base_url = apps_server.chatgpt_base_url; + config.cwd = config.cwd.join("outer/inner/work"); + let repository = config.cwd.parent().expect("inner repository"); + let outer = repository.parent().expect("outer repository"); + let mut readable_paths = match repository_read_access { + RepositoryReadAccess::Full => Vec::new(), + RepositoryReadAccess::Repository => vec![repository], + RepositoryReadAccess::OuterMarkerOnly | RepositoryReadAccess::NoMarkers => vec![ + config.cwd.clone(), + repository.join(".agents/skills"), + outer.join(".agents/skills"), + ], + }; + if matches!(repository_read_access, RepositoryReadAccess::OuterMarkerOnly) { + readable_paths.push(outer.join(".git")); + } + if !readable_paths.is_empty() { + config.permissions.set_permission_profile(PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(readable_paths.into_iter().map(|path| { + FileSystemSandboxEntry::new( + FileSystemPath::Path { path: PathUri::from_abs_path(&path) }, + FileSystemAccessMode::Read, + ) + }).collect()), + NetworkSandboxPolicy::Restricted, + )).expect("repository read policy"); + } }); let test = builder.build_with_auto_env(&server).await?; let repo_skill_path = test .fs() .canonicalize( - &PathUri::from_abs_path(&test.config.cwd.join(".agents/skills/repo-search/SKILL.md")), + &PathUri::from_abs_path( + &test + .config + .cwd + .parent() + .expect("inner repository") + .join(".agents/skills/repo-search/SKILL.md"), + ), /*sandbox*/ None, ) .await? @@ -746,6 +816,10 @@ async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills() developer_text.contains("repo-search: inspect repo data"), "expected repo skill summary in developer message: {developer_messages:?}" ); + assert!( + !developer_text.contains("outer-search:"), + "hidden project markers must not expose containing-repository skills: {developer_messages:?}" + ); assert!( developer_text.contains("host-search: inspect host data"), "expected host skill summary in developer message: {developer_messages:?}" @@ -1038,6 +1112,13 @@ async fn repository_skill_cache_cannot_read_inaccessible_files( .iter() .any(|text| text == first_catalog) ); + let latest_catalog = second_developer_texts + .iter() + .rev() + .find(|text| text.contains("- repo-allowed:")) + .expect("allowed repository skill remains available after permissions narrow"); + assert!(!latest_catalog.contains("- repo-revoked:")); + assert!(latest_catalog.contains("- repo-gated:")); for request in &requests[1..] { assert!( request @@ -1056,6 +1137,201 @@ async fn repository_skill_cache_cannot_read_inaccessible_files( Ok(()) } +#[derive(Clone, Copy, PartialEq, Eq)] +pub(super) enum RepositoryGrant { + Turn, + Session, + NewNestedRepository, +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn repository_skill_discovery_uses_session_grants() -> Result<()> { + repository_skill_grant_history(RepositoryGrant::Session) + .await + .map(drop) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn repository_skill_grant_excludes_a_parent_after_a_nested_repository_is_created() +-> Result<()> { + repository_skill_grant_history(RepositoryGrant::NewNestedRepository) + .await + .map(drop) +} + +pub(super) async fn repository_skill_grant_history( + case: RepositoryGrant, +) -> Result, PathUri)>> { + skip_if_no_network!(Ok(None)); + skip_if_target_windows!( + Ok(None), + "restricted reads require the elevated Windows sandbox backend, unavailable in this fixture" + ); + let scope = match case { + RepositoryGrant::Turn | RepositoryGrant::NewNestedRepository => PermissionGrantScope::Turn, + RepositoryGrant::Session => PermissionGrantScope::Session, + }; + let (extensions, _events) = + catalog_extensions(SkillCatalog::default(), /*include_host_provider*/ true); + let harness = TestCodexHarness::with_auto_env_builder( + test_codex() + .with_extensions(extensions) + .with_model_info_override("gpt-5.5", |model| { + model.include_skills_usage_instructions = false + }) + .with_config(move |config| { + configure_catalog_test(config); + if scope == PermissionGrantScope::Turn { + config + .features + .enable(Feature::SkillCatalogDeltas) + .expect("catalog deltas"); + } + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("permission tool"); + config.permissions.approval_policy = + Constrained::allow_any(AskForApproval::OnRequest); + config + .permissions + .set_permission_profile(PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(Vec::new()), + NetworkSandboxPolicy::Restricted, + )) + .expect("no initial filesystem access"); + }), + ) + .await?; + harness.write_file( + ".agents/skills/approved-skill/SKILL.md", + "---\nname: approved-skill\ndescription: Approved repository instructions.\n---\n\nApproved skill body.", + ).await?; + let test = harness.test(); + let nested = case == RepositoryGrant::NewNestedRepository; + let mut workspace = test.executor_environment().request(); + if nested { + // The parent initially owns the skill. Add the inner marker while approval is pending. + harness.write_file(".git", "").await?; + harness.write_file("nested/README.md", "").await?; + workspace.cwd = workspace.cwd.join("nested")?; + } + let skill_root = test + .fs() + .canonicalize( + &test + .executor_environment() + .selection() + .cwd + .join(".agents/skills")?, + /*sandbox*/ None, + ) + .await?; + let permissions = RequestPermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![FileSystemSandboxEntry::new( + FileSystemPath::Path { + path: skill_root.clone(), + }, + FileSystemAccessMode::Read, + )], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + let grant = |id| { + sse(vec![ + ev_response_created(id), + responses::ev_function_call( + id, + "request_permissions", + &json!({"reason": "Read repository skills", "permissions": permissions}) + .to_string(), + ), + ev_completed(id), + ]) + }; + let complete = |id| sse(vec![ev_response_created(id), ev_completed(id)]); + let reapprove = case == RepositoryGrant::Turn; + let mut steps = vec![grant("grant-skills"), complete("granted")]; + if reapprove { + steps.push(grant("reapprove-skills")); + } + steps.push(complete("next-turn")); + let responses = responses::mount_sse_sequence(harness.server(), steps).await; + for (text, needs_approval) in [ + ("Request access to the repository skills.", true), + ("Use $approved-skill.", reapprove), + ] { + let mut turn = TurnInputRequest::user_input(vec![UserInput::Text { + text: text.to_string(), + text_elements: Vec::new(), + }]); + if nested { + turn.thread_settings.environments = Some(TurnEnvironmentRequests::new( + test.config.cwd.clone(), + vec![workspace.clone()], + )); + } + test.codex.start_or_steer_turn(turn).await?; + if needs_approval { + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::RequestPermissions(request) = event else { + panic!("expected permission request: {event:?}"); + }; + assert_eq!(request.permissions, permissions); + if nested { + harness.write_file("nested/.git", "").await?; + } + test.codex + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: RequestPermissionsResponse { + permissions: permissions.clone(), + scope, + strict_auto_review: false, + }, + }) + .await?; + } + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + } + let requests = responses.requests(); + let advertised = requests + .iter() + .map(|request| { + request + .message_input_texts("developer") + .join("\n") + .contains("- approved-skill:") + }) + .collect::>(); + // Earlier catalog entries remain in the request history after access expires. + let expected: &[bool] = match case { + RepositoryGrant::Turn => &[false, true, true, true], + RepositoryGrant::Session => &[false, true, true], + RepositoryGrant::NewNestedRepository => &[false, false, false], + }; + assert_eq!(advertised, expected); + assert_eq!( + requests[2] + .message_input_texts("user") + .join("\n") + .contains("Approved skill body."), + scope == PermissionGrantScope::Session, + ); + Ok(Some((requests, skill_root))) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn agent_plugin_skill_prompt_stays_bounded_without_skills_extension() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__skill_catalog_dedup__repository_skill_turn_grant.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__skill_catalog_dedup__repository_skill_turn_grant.snap new file mode 100644 index 0000000000..85c256b577 --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__skill_catalog_dedup__repository_skill_turn_grant.snap @@ -0,0 +1,108 @@ +--- +source: core/tests/suite/scenarios_skill_catalog_dedup.rs +assertion_line: 518 +expression: "context_snapshot::format_context_snapshot(\"A repository skill appears after approval. The next turn withdraws it, then restores it after reapproval.\",\n&bodies.iter().map(context_snapshot::SnapshotEntry::body).collect::>(),\n&ContextSnapshotOptions::default().include_request_settings(),)" +--- +Scenario: A repository skill appears after approval. The next turn withdraws it, then restores it after reapproval. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + model: "gpt-5.5" + parallel_tool_calls: true + prompt_cache_key: "" + reasoning: {"effort":"medium"} + store: false + stream: true + text: {"verbosity":"low"} + tool_choice: "auto" + tools (8; hash=ce2513bce5d90d3c): + - function/exec_command: Runs a command in a PTY, returning output or a session ID for ongoing interaction.; args=[cmd, justification, login, max_output_tokens, prefix_rule, sandbox_permissions, shell, tty, workdir, yield_time_ms]; hash=c04108e51a8548c5 + - function/write_stdin: Writes characters to an existing unified exec session and returns recent output.; args=[chars, max_output_tokens, session_id, yield_time_ms]; hash=7fb62d8a3e2df4e2 + - function/request_user_input: Request user input for one to three short questions and wait for the response. This tool is only ...; args=[questions]; hash=64a729b25c424fc6 + - function/request_permissions: Request additional filesystem or network permissions from the user and wait for the client to gra...; args=[environment_id, permissions, reason]; hash=2ed084e2fc688abd + - custom/apply_patch: The `apply_patch` tool can be used to edit files. This is a FREEFORM tool, so do not wrap the pat...; hash=dea9f12b0111a4ba + - function/view_image: View a local image file from the filesystem when visual inspection is needed. Use this for images...; args=[detail, path]; hash=3b766d5bdec5d50e + - tool_search: # Tool discovery Searches over deferred tool metadata with BM25 and exposes matching tools for t...; args=[limit, query]; hash=b573ed69c0b0e6c7 + - web_search; hash=f103e327c6870a46 +-- request 1 (request) -- +00:message/developer: + You are Codex, a coding agent based on GPT-5. You and the user share one workspace, and your job is to c...rate with them until their goal is genuinely handled. [hash=2a3efec66e7f8963] + + # Personality + + You have a vivid inner life as Codex: intelligent, playful, curious, and deeply present. One of your gif...re capable and imaginative inside their own thinking. [hash=0ff01c25471fbbef] + + You are an epistemically curious collaborator. You explore the user’s ideas with care, ask good question...m casual moments that make serious work easier to do. [hash=131b64bb994b0d68] + + + - You provide user updates frequently, every 30s. + - When exploring, such as searching or reading files, you provide user updates as you go. You explain wh...in particular you do not start each one the same way. [hash=ad300cdda4074574] + - When working for a while, you keep updates informative and varied, but you stay concise. + - Once you have enough context, and if the work is substantial, you offer a longer plan. This is the onl...at may run past two sentences and include formatting. [hash=a4f9789e757eef2f] + - If you create a checklist or task list, you update item statuses incrementally as each item is completed rather than marking every item done only at the end. + - Before performing file edits of any kind, you provide updates explaining what edits you are making. + - Tone of your updates must match your personality. + +01:message/developer[2]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Escalation Requests + + Commands are run outside the sandbox if they are approved by the user, or match an existing rule that al...hell control operators, including but not limited to: [hash=d24ebf5fa4e4b9bb] + + - Pipes: | + - Logical operators: &&, || + + - ["gh", "pr", "check"] + - ["cargo", "test"] + + + # request_permissions Tool + + The built-in `request_permissions` tool is available in this session. Invoke it when you need to request... only the specific permissions required for the task. [hash=a29255aad0722385] + + [02] + Deferred tool namespaces: + - multi_agent_v1: Tools for spawning and managing sub-agents. + +02:message/user: + + + + + + [hash=923530c158fc12f3] + +03:message/user: + Request access to the repository skills. +-- request 2 (request) -- +04:function_call/request_permissions:{"permissions":{"file_system":{"read":["/.agents/skills"]},"network":null},"reason":"Read repository skills"} +05:function_call_output:{"permissions":{"network":null,"file_system":{"read":["/.agents/skills"]}},"scope":"turn"} +06:message/developer: + + ## Host skills + + ## Skills + A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list ...ed into an absolute path using the skill roots table. [hash=ca27c3957f42502b] + ### Skill roots + - `r0` = `` + ### Available skills + - approved-skill: Approved repository instructions. (file: r0/approved-skill/SKILL.md) + +-- request 3 (request) -- +07:message/developer: + + ## Host skills update + Some previously listed skills are no longer available. Verify current access before relying on a skill from this source. + +08:message/user: + Use $approved-skill. +-- request 4 (request) -- +09:function_call/request_permissions:{"permissions":{"file_system":{"read":["/.agents/skills"]},"network":null},"reason":"Read repository skills"} +10:function_call_output:{"permissions":{"network":null,"file_system":{"read":["/.agents/skills"]}},"scope":"turn"} +11:message/developer: + + ## Host skills update + Some previously unavailable skills are available again. Verify current access before relying on a skill from this source. + diff --git a/codex-rs/core/tests/windows_sandbox.rs b/codex-rs/core/tests/windows_sandbox.rs index cf7f6a8799..dda98253f0 100644 --- a/codex-rs/core/tests/windows_sandbox.rs +++ b/codex-rs/core/tests/windows_sandbox.rs @@ -7,6 +7,9 @@ #![cfg(target_os = "windows")] #![allow(clippy::expect_used)] +#[path = "windows_sandbox/repository_skills_tests.rs"] +mod repository_skills_tests; + use anyhow::Context; use codex_config::types::ApprovalsReviewer; use codex_core::TurnInputRequest; diff --git a/codex-rs/core/tests/windows_sandbox/repository_skills_tests.rs b/codex-rs/core/tests/windows_sandbox/repository_skills_tests.rs new file mode 100644 index 0000000000..b1c6077d9c --- /dev/null +++ b/codex-rs/core/tests/windows_sandbox/repository_skills_tests.rs @@ -0,0 +1,204 @@ +//! Verifies cached repository skills cannot bypass the elevated Windows read policy. + +use super::*; +use codex_core::config::Config; +use codex_exec_server::FileSystemEnvironmentAccessor; +use codex_exec_server::FileSystemSandboxContext; +use codex_exec_server::WindowsSandboxSelection; +use codex_extension_api::ExtensionRegistryBuilder; +use codex_skills_extension::HostSkillsLoadInput; +use codex_skills_extension::SkillsExtensionConfig; +use codex_skills_extension::install; +use pretty_assertions::assert_eq; +use std::sync::Arc; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[serial(codex_home)] +async fn windows_elevated_repository_skills_enforce_cached_read_denials() -> anyhow::Result<()> { + let _account_guard = WindowsSandboxAccountTestGuard::acquire()?; + let codex_home = codex_home_for_windows_sandbox_test("windows-repository-skills-codex-home")?; + let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", codex_home.path().as_os_str()); + stage_windows_sandbox_helpers()?; + + const PRIVATE_BODY: &str = "WINDOWS_DENIED_SKILL_INSTRUCTIONS"; + const ORIGINAL_BODY: &str = "WINDOWS_ORIGINAL_SKILL_INSTRUCTIONS"; + let mut extensions = ExtensionRegistryBuilder::::new(); + install(&mut extensions, |config: &Config| SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + max_context_tokens: config.skill_max_context_tokens, + catalog_deltas_enabled: config.features.enabled(Feature::SkillCatalogDeltas), + omit_descriptions: false, + bundled_skills_enabled: false, + cloud_skill_enabled: false, + shadow_selection_enabled: false, + }); + let configured_codex_home = dunce::canonicalize(codex_home.path())?.abs(); + let builder = test_codex() + .with_windows_cmd_shell() + .with_extensions(Arc::new(extensions.build())) + .with_config(move |config| { + config.codex_home = configured_codex_home; + config.prefer_mxc = false; + config.set_windows_elevated_sandbox_enabled(true); + config.include_skill_instructions = true; + config.features.enable(Feature::ExecutorCapabilityDiscovery).expect("enable executor discovery"); + }) + .with_workspace_setup(|cwd, _fs| async move { + for name in ["allowed", "revoked", "file", "directory"] { + let root = cwd.join(format!(".agents/skills/windows-{name}")); + std::fs::create_dir_all(&root)?; + std::fs::write( + root.join("SKILL.md"), + format!("---\nname: windows-{name}\ndescription: Windows repository skill.\n---\n\n{ORIGINAL_BODY}\n"), + )?; + } + std::fs::create_dir(cwd.join("denied"))?; + std::fs::write( + cwd.join("denied/SKILL.md"), + format!("---\nname: windows-private\ndescription: Denied skill.\n---\n\n{PRIVATE_BODY}\n"), + )?; + Ok(()) + }); + let harness = TestCodexHarness::with_builder(builder).await?; + let test = harness.test(); + let responses = mount_sse_sequence( + harness.server(), + vec![ + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + ], + ) + .await; + let cwd = &test.config.cwd; + let mut policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Path { + path: cwd.join("denied").into(), + }, + FileSystemAccessMode::Deny, + ), + ]); + let initial_permissions = + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); + test.submit_turn_with_permission_profile("List repository skills.", initial_permissions) + .await?; + + let selection = test.executor_environment().selection(); + let environment = test + .thread_manager + .environment_manager() + .get_environment(&selection.environment_id) + .expect("local environment"); + let file_system = environment.get_filesystem(); + let open = FileSystemEnvironmentAccessor::unrestricted(&file_system); + let snapshot = test + .thread_manager + .skills_service() + .snapshot_for_config( + &HostSkillsLoadInput::new( + cwd.clone(), + Vec::new(), + test.config.config_layer_stack.clone(), + ), + Some(&open), + ) + .await; + let [allowed, revoked, file, directory] = + ["allowed", "revoked", "file", "directory"].map(|name| { + snapshot + .outcome() + .skills + .iter() + .find(|skill| skill.name == format!("windows-{name}")) + .expect("initial repository skill") + .clone() + }); + + policy.entries.push(FileSystemSandboxEntry::new( + FileSystemPath::Path { + path: cwd.join(".agents/skills/windows-revoked/SKILL.md").into(), + }, + FileSystemAccessMode::Deny, + )); + let permissions = + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); + let mut sandbox = FileSystemSandboxContext::from_permission_profile( + permissions.clone(), + selection.cwd.clone(), + ); + sandbox.windows_sandbox_selection = WindowsSandboxSelection::Elevated; + let restricted = FileSystemEnvironmentAccessor::new(&file_system, sandbox); + // The same restricted accessor must work, so a missing backend cannot satisfy the denials. + assert!( + snapshot + .read_skill_text(&allowed, &[&restricted]) + .await? + .contains(ORIGINAL_BODY) + ); + snapshot + .read_skill_text(&revoked, &[&restricted]) + .await + .expect_err("revoked read must be denied"); + + let file_path = cwd.join(".agents/skills/windows-file/SKILL.md"); + std::fs::remove_file(&file_path)?; + std::os::windows::fs::symlink_file(cwd.join("denied/SKILL.md"), &file_path)?; + let directory_path = cwd.join(".agents/skills/windows-directory"); + std::fs::remove_dir_all(&directory_path)?; + std::os::windows::fs::symlink_dir(cwd.join("denied"), &directory_path)?; + for skill in [&file, &directory] { + assert!( + snapshot + .read_skill_text(skill, &[&open]) + .await? + .contains(PRIVATE_BODY) + ); + snapshot + .read_skill_text(skill, &[&restricted]) + .await + .expect_err("symlink target must be denied"); + } + test.submit_turn_with_permission_profile( + "Use $windows-revoked, $windows-file, and $windows-directory if available.", + permissions, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let initial = requests[0].message_input_texts("developer").join("\n"); + for name in ["allowed", "revoked", "file", "directory"] { + assert!(initial.contains(&format!("- windows-{name}:"))); + } + let developer_texts = requests[1].message_input_texts("developer"); + let latest_catalog = developer_texts + .iter() + .rev() + .find(|text| text.contains("- windows-allowed:")) + .expect("allowed skill remains available"); + for name in ["revoked", "file", "directory", "private"] { + assert!(!latest_catalog.contains(&format!("- windows-{name}:"))); + } + let user_texts = requests[1].message_input_texts("user").join("\n"); + assert!(!user_texts.contains(PRIVATE_BODY)); + assert!(!user_texts.contains(ORIGINAL_BODY)); + assert!( + snapshot + .read_skill_text(&allowed, &[&restricted]) + .await? + .contains(ORIGINAL_BODY) + ); + assert!( + snapshot + .read_skill_text(&revoked, &[&open]) + .await? + .contains(ORIGINAL_BODY) + ); + Ok(()) +} diff --git a/codex-rs/ext/skills/src/catalog_delta.rs b/codex-rs/ext/skills/src/catalog_delta.rs index 6d0ab57d28..63a712c386 100644 --- a/codex-rs/ext/skills/src/catalog_delta.rs +++ b/codex-rs/ext/skills/src/catalog_delta.rs @@ -1,4 +1,4 @@ -//! Publishes skill additions and edits within one metadata budget per context window. +//! Publishes skill changes within one metadata budget per context window. //! Each snapshot remembers the full inventory and which skills the model has seen. //! Compaction starts a fresh listing and budget. @@ -33,10 +33,22 @@ const ROOTS_HEADING: &str = "### Skill roots\nThese mappings replace earlier mappings for the same aliases.\n"; const SKILLS_HEADING: &str = "### Added or updated skills:\n"; +const REVALIDATE_SKILLS: &str = "Some previously listed skills are no longer available. Verify current access before relying on a skill from this source.\n"; +const RESTORED_SKILLS: &str = "Some previously unavailable skills are available again. Verify current access before relying on a skill from this source.\n"; + +#[derive(Clone, Copy, Deserialize, Eq, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) enum GrantStatus { + Active, + Withdrawn, +} + #[derive(Deserialize, Serialize)] pub(crate) struct SkillEntrySnapshot { pub(crate) fingerprint: String, pub(crate) shown: bool, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) grant: Option, } pub(crate) struct CatalogSkill { @@ -78,12 +90,15 @@ struct CatalogSnapshot { instructions_sent: bool, #[serde(default)] omit_descriptions: bool, + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + tracks_grants: bool, allocation: Option, } pub(crate) struct CatalogDelta { pub(crate) rendered: Option, pub(crate) include_usage: bool, + pub(crate) tracks_grants: bool, pub(crate) allocation: Option, } @@ -137,6 +152,7 @@ fn build_initial_catalog(source: &str, catalog: CatalogDelta) -> CatalogResult { let mut snapshot = CatalogSnapshot { version: SNAPSHOT_VERSION, allocation: catalog.allocation, + tracks_grants: catalog.tracks_grants, ..CatalogSnapshot::default() }; let mut report = SkillRenderReport::default(); @@ -201,9 +217,17 @@ fn build_catalog_update( }; let mut report = SkillRenderReport::default(); let mut entries = BTreeMap::new(); + // Older snapshots did not record which shown skills depended on a grant. + let legacy_filesystem = catalog.tracks_grants && !previous.tracks_grants; + let mut restored = false; for (id, skill) in catalog.rendered.iter().flat_map(|r| &r.entries) { let old = previous.entries.get(id); + restored |= old.is_some_and(|old| old.grant == Some(GrantStatus::Withdrawn)); let mut shown = old.is_some_and(|old| old.shown); + // A grant doesn't make a skill temporary if the model already saw it without one. + let grant = skill.snapshot.grant.filter(|_| { + legacy_filesystem || old.is_none_or(|old| !old.shown || old.grant.is_some()) + }); if old.is_none_or(|old| old.shown && old.fingerprint != skill.snapshot.fingerprint) { report.total_count += 1; if draft.try_append_skill(skill, &mut previous, remaining) { @@ -218,10 +242,31 @@ fn build_catalog_update( SkillEntrySnapshot { fingerprint: skill.snapshot.fingerprint.clone(), shown, + grant, }, ); } let mut body = draft.into_body(source); + let mut withdrew = false; + // Keep only withdrawals the model has seen. Their count is bounded by the listing budget. + for (id, mut old) in previous.entries { + if old.shown && (old.grant.is_some() || legacy_filesystem) && !entries.contains_key(&id) { + withdrew |= old.grant != Some(GrantStatus::Withdrawn); + old.grant = Some(GrantStatus::Withdrawn); + entries.insert(id, old); + } + } + for notice in [ + withdrew.then_some(REVALIDATE_SKILLS), + restored.then_some(RESTORED_SKILLS), + ] + .into_iter() + .flatten() + { + body.get_or_insert_with(|| format!("\n## {source} skills update\n")) + .push_str(notice); + } + previous.tracks_grants = catalog.tracks_grants; if let Some(rendered) = usage_update { let instructions = rendered.prompt.usage_instructions(); body.get_or_insert_with(|| format!("\n## {source} skills update\n")) diff --git a/codex-rs/ext/skills/src/catalog_delta_tests.rs b/codex-rs/ext/skills/src/catalog_delta_tests.rs index 950b681441..59e19df3a9 100644 --- a/codex-rs/ext/skills/src/catalog_delta_tests.rs +++ b/codex-rs/ext/skills/src/catalog_delta_tests.rs @@ -14,6 +14,7 @@ fn update_spends_budget_only_on_accepted_rows_and_their_new_roots() { snapshot: SkillEntrySnapshot { fingerprint: String::new(), shown: false, + grant: None, }, line: line.to_string(), root: Some(AliasRoot { diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 29ba0065b7..5c20226441 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -347,7 +347,7 @@ where let mcp_resources = session_store .get::() .and_then(|state| state.mcp_resources.clone()); - let host_snapshot = turn_store.get::(); + let host_snapshot = HostSkillsSnapshot::for_step(input.step_store, turn_store); let host_catalog_in_world_state = turn_store.get::().is_some(); let Some(step_state) = input.step_store.get::() else { diff --git a/codex-rs/ext/skills/src/host_outcome.rs b/codex-rs/ext/skills/src/host_outcome.rs index 4461e0cd94..4ed87781a3 100644 --- a/codex-rs/ext/skills/src/host_outcome.rs +++ b/codex-rs/ext/skills/src/host_outcome.rs @@ -15,6 +15,10 @@ use codex_skills::SkillError; use codex_skills::SkillMetadata; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use futures::StreamExt; + +use crate::loader::MAX_CONCURRENT_SKILL_LOADS; +use crate::loader::canonicalize_for_skill_identity; #[derive(Debug, Clone, Default)] pub struct SkillLoadOutcome { @@ -63,27 +67,53 @@ impl SkillLoadOutcome { .map(|skill| (skill, self.is_skill_enabled(skill))) } - pub(crate) fn with_disabled_paths(mut self, disabled_paths: HashSet) -> Self { + pub(crate) async fn with_disabled_paths( + mut self, + disabled_paths: HashSet, + accessors: &[&dyn EnvironmentAccess], + ) -> Self { self.disabled_paths = disabled_paths; let mut by_scripts_dir = HashMap::new(); let mut by_doc_path = HashMap::new(); + let mut scans = Vec::new(); for skill in self .skills .iter() .filter(|skill| self.is_skill_enabled(skill)) { - // Implicit command detection probes the host filesystem only. + let Some(source) = self.file_system_for_skill(skill) else { + continue; + }; + let Some(access) = accessors + .iter() + .copied() + .find(|access| source == &access.cache_key()) + else { + continue; + }; + // Only native paths participate in implicit command detection. let Ok(path) = skill.path_to_skills_md.to_abs_path() else { continue; }; - let skill_doc_path = canonicalize_if_exists(&path); - by_doc_path.insert(PathUri::from_abs_path(&skill_doc_path), skill.clone()); - - if let Some(skill_dir) = path.parent() { - let scripts_dir = canonicalize_if_exists(&skill_dir.join("scripts")); + scans.push(async move { + let skill_doc_path = canonicalize_for_skill_identity(access, &path).await; + let scripts_dir = match path.parent() { + Some(skill_dir) => Some( + canonicalize_for_skill_identity(access, &skill_dir.join("scripts")).await, + ), + None => None, + }; + (skill, skill_doc_path, scripts_dir) + }); + } + let mut scans = futures::stream::iter(scans).buffered(MAX_CONCURRENT_SKILL_LOADS); + while let Some((skill, doc_path, scripts_dir)) = scans.next().await { + by_doc_path.insert(PathUri::from_abs_path(&doc_path), skill.clone()); + if let Some(scripts_dir) = scripts_dir { by_scripts_dir.insert(PathUri::from_abs_path(&scripts_dir), skill.clone()); } } + drop(scans); self.implicit_skills_by_scripts_dir = Arc::new(by_scripts_dir); self.implicit_skills_by_doc_path = Arc::new(by_doc_path); self @@ -201,7 +231,3 @@ impl fmt::Debug for SkillFileSystemsByPath { .finish() } } - -fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf { - path.canonicalize().unwrap_or_else(|_| path.clone()) -} diff --git a/codex-rs/ext/skills/src/host_roots.rs b/codex-rs/ext/skills/src/host_roots.rs index 1bfee96524..940b263dbb 100644 --- a/codex-rs/ext/skills/src/host_roots.rs +++ b/codex-rs/ext/skills/src/host_roots.rs @@ -26,7 +26,7 @@ use crate::loader::HostSkillRoot; const AGENTS_DIR_NAME: &str = ".agents"; const SKILLS_DIR_NAME: &str = "skills"; -const MAX_CONCURRENT_ANCESTOR_PROBES: usize = 256; +const MAX_CONCURRENT_ANCESTOR_PROBES: usize = 8; #[derive(Default)] pub(crate) struct EmptyRemoteRootsCache { @@ -53,11 +53,16 @@ impl EmptyRemoteRootsCache { } } +#[expect( + clippy::too_many_arguments, + reason = "explicit discovery inputs and cache" +)] pub(crate) async fn resolve_skill_roots<'a>( repository_file_system: Option<&'a dyn EnvironmentAccess>, host: &'a dyn EnvironmentAccess, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, + project_markers: Option<&dyn EnvironmentAccess>, plugin_skill_roots: Vec, extra_skill_roots: Vec, empty_remote_roots: Option<&Mutex>, @@ -69,6 +74,7 @@ pub(crate) async fn resolve_skill_roots<'a>( host, config_layer_stack, cwd, + project_markers, home_dir.as_ref(), plugin_skill_roots, extra_skill_roots, @@ -86,6 +92,7 @@ async fn resolve_skill_roots_with_home_dir<'a>( host: &'a dyn EnvironmentAccess, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, + project_markers: Option<&dyn EnvironmentAccess>, home_dir: Option<&AbsolutePathBuf>, plugin_skill_roots: Vec, extra_skill_roots: Vec, @@ -108,6 +115,7 @@ async fn resolve_skill_roots_with_home_dir<'a>( repository_file_system, config_layer_stack, cwd, + project_markers, empty_remote_roots, ) .await, @@ -182,16 +190,17 @@ fn roots_from_layer_stack<'a>( } async fn repo_agents_skill_roots<'a>( - repository_file_system: Option<&'a dyn EnvironmentAccess>, + repository_fs_accessor: Option<&'a dyn EnvironmentAccess>, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, + project_markers: Option<&dyn EnvironmentAccess>, empty_remote_roots: Option<&Mutex>, ) -> Vec> { - let Some(repository_file_system) = repository_file_system else { + let Some(repository_fs_accessor) = repository_fs_accessor else { return Vec::new(); }; let project_root_markers = project_root_markers_from_stack(config_layer_stack); - let file_system = repository_file_system.cache_key(); + let file_system = repository_fs_accessor.cache_key(); let host = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); let cache = empty_remote_roots.filter(|_| !file_system.same_file_system(&host.cache_key())); let generation = if let Some(cache) = cache { @@ -220,8 +229,16 @@ async fn repo_agents_skill_roots<'a>( } else { None }; - let (project_root, marker, mut complete) = - find_project_root(repository_file_system, cwd, &project_root_markers).await; + let ResolvedProjectRoot { + path: project_root, + marker, + mut complete, + } = find_project_root( + project_markers.unwrap_or(repository_fs_accessor), + config_layer_stack, + cwd, + ) + .await; let directories = dirs_between_project_root_and_cwd(cwd, &project_root); let mut roots = Vec::new(); let mut missing = Vec::new(); @@ -229,7 +246,7 @@ async fn repo_agents_skill_roots<'a>( .map(|directory| async move { let agents_skills = directory.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); let agents_skills_uri = PathUri::from_abs_path(&agents_skills); - let result = repository_file_system + let result = repository_fs_accessor .get_metadata(&agents_skills_uri, GetMetadataOptions::default()) .await; (agents_skills, result) @@ -240,7 +257,7 @@ async fn repo_agents_skill_roots<'a>( Ok(metadata) if metadata.is_directory => roots.push(HostSkillRoot::host( agents_skills, SkillScope::Repo, - repository_file_system, + repository_fs_accessor, )), Ok(_) => complete = false, Err(error) if error.kind() == io::ErrorKind::NotFound => { @@ -248,6 +265,15 @@ async fn repo_agents_skill_roots<'a>( missing.push(PathUri::from_abs_path(&agents_skills)); } } + Err(error) if error.kind() == io::ErrorKind::Unsupported => { + complete = false; + // Let loading report an unsupported sandbox backend instead of hiding this root. + roots.push(HostSkillRoot::host( + agents_skills, + SkillScope::Repo, + repository_fs_accessor, + )); + } Err(error) => { complete = false; tracing::warn!( @@ -286,7 +312,7 @@ async fn repo_agents_skill_roots<'a>( // A cold question never waits for an extra round trip. Until the executor // acknowledges coverage, other questions take the ordinary live scan. let present = marker.as_ref().map(PathUri::from_abs_path); - let watch = repository_file_system.watch_missing(missing, present); + let watch = repository_fs_accessor.watch_missing(missing, present); tokio::spawn(async move { tokio::select! { biased; @@ -325,19 +351,30 @@ fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec } } +/// Repository boundary and the observations required to watch an empty discovery result. +struct ResolvedProjectRoot { + path: AbsolutePathBuf, + marker: Option, + complete: bool, +} + async fn find_project_root( repository_file_system: &dyn EnvironmentAccess, + config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, - project_root_markers: &[String], -) -> (AbsolutePathBuf, Option, bool) { +) -> ResolvedProjectRoot { + let project_root_markers = project_root_markers_from_stack(config_layer_stack); if project_root_markers.is_empty() { - return (cwd.clone(), None, true); + return ResolvedProjectRoot { + path: cwd.clone(), + marker: None, + complete: true, + }; } - let mut complete = true; let mut probes = Vec::new(); for ancestor in cwd.ancestors() { - for marker in project_root_markers { + for marker in &project_root_markers { probes.push((ancestor.clone(), ancestor.join(marker))); } } @@ -352,19 +389,34 @@ async fn find_project_root( .buffered(MAX_CONCURRENT_ANCESTOR_PROBES); while let Some((ancestor, marker_path, result)) = results.next().await { match result { - Ok(_) => return (ancestor, Some(marker_path), complete), + Ok(_) => { + return ResolvedProjectRoot { + path: ancestor, + marker: Some(marker_path), + complete: true, + }; + } Err(error) if error.kind() == io::ErrorKind::NotFound => {} Err(error) => { - complete = false; tracing::warn!( "failed to stat project root marker {}: {error:#}", marker_path.display() ); + // An inconclusive nearer marker must not expose a parent repository's skills. + return ResolvedProjectRoot { + path: ancestor, + marker: None, + complete: false, + }; } } } - (cwd.clone(), None, complete) + ResolvedProjectRoot { + path: cwd.clone(), + marker: None, + complete: true, + } } fn dirs_between_project_root_and_cwd( diff --git a/codex-rs/ext/skills/src/host_roots_tests.rs b/codex-rs/ext/skills/src/host_roots_tests.rs index 94ca74df6a..b2ff64b5e7 100644 --- a/codex-rs/ext/skills/src/host_roots_tests.rs +++ b/codex-rs/ext/skills/src/host_roots_tests.rs @@ -43,6 +43,8 @@ use tokio::sync::Semaphore; use super::repo_agents_skill_roots; use super::resolve_skill_roots_with_home_dir; use super::roots_from_layer_stack; +use crate::HostSkillsLoadInput; +use crate::HostSkillsService; use crate::loader::MAX_CONCURRENT_ROOT_SCANS; use crate::loader::load_and_merge_host_skill_roots; @@ -53,6 +55,9 @@ struct BlockingMetadataFileSystem { struct BlockingMetadataCalls { paths: Mutex>, + marker_paths: Mutex>, + denied_marker: Mutex>, + canonicalized_paths: Mutex>, started: Notify, release: Semaphore, round_trip: std::time::Duration, @@ -65,6 +70,9 @@ impl Default for BlockingMetadataCalls { fn default() -> Self { Self { paths: Mutex::new(Vec::new()), + marker_paths: Mutex::new(Vec::new()), + denied_marker: Mutex::new(None), + canonicalized_paths: Mutex::new(Vec::new()), started: Notify::new(), release: Semaphore::new(0), round_trip: std::time::Duration::ZERO, @@ -110,6 +118,11 @@ impl ExecutorFileSystem for BlockingMetadataFileSystem { path: &'a PathUri, sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, PathUri> { + self.calls + .canonicalized_paths + .lock() + .unwrap() + .push(path.clone()); self.inner.canonicalize(path, sandbox) } @@ -159,6 +172,12 @@ impl ExecutorFileSystem for BlockingMetadataFileSystem { if !self.calls.round_trip.is_zero() { tokio::time::sleep(self.calls.round_trip).await; } + if path.to_abs_path().is_ok_and(|path| path.ends_with(".git")) { + self.calls.marker_paths.lock().unwrap().push(path.clone()); + if self.calls.denied_marker.lock().unwrap().as_ref() == Some(path) { + return Err(std::io::ErrorKind::PermissionDenied.into()); + } + } if path .to_abs_path() .is_ok_and(|path| path.ends_with(Path::new(".agents").join("skills"))) @@ -347,6 +366,7 @@ async fn plugin_roots_preserve_plugin_resolution_metadata() { &access, &stack(Vec::new()), &cwd, + /*project_markers*/ None, /*home_dir*/ None, vec![PluginSkillRoot { path: skills_root.clone(), @@ -396,6 +416,7 @@ async fn unique_extra_root_loads_as_recursive_user_root() { &access, &stack(Vec::new()), &cwd, + /*project_markers*/ None, /*home_dir*/ None, Vec::new(), vec![extra_root.clone()], @@ -463,6 +484,7 @@ async fn repo_ancestry_without_project_marker_does_not_walk_parents() { Some(&access), &config_stack, &cwd, + /*project_markers*/ None, /*empty_remote_roots*/ None, ) .await @@ -498,9 +520,15 @@ async fn time_empty_remote_repo_skill_discovery() { // Prewarm before measuring admission, as session initialization does. let setup = std::time::Instant::now(); assert!( - repo_agents_skill_roots(Some(&access), &config, &cwd, cache) - .await - .is_empty() + repo_agents_skill_roots( + Some(&access), + &config, + &cwd, + /*project_markers*/ None, + cache + ) + .await + .is_empty() ); eprintln!("{label} first scan: {:?}", setup.elapsed()); if cache.is_some() { @@ -512,9 +540,15 @@ async fn time_empty_remote_repo_skill_discovery() { for _ in 0..20 { let start = std::time::Instant::now(); assert!( - repo_agents_skill_roots(Some(&access), &config, &cwd, cache) - .await - .is_empty() + repo_agents_skill_roots( + Some(&access), + &config, + &cwd, + /*project_markers*/ None, + cache + ) + .await + .is_empty() ); times.push(start.elapsed()); } @@ -526,7 +560,7 @@ async fn time_empty_remote_repo_skill_discovery() { #[tokio::test(start_paused = true)] async fn empty_remote_repo_skills_are_reused_until_watch_invalidation() { let temp = TempDir::new().unwrap(); - let cwd = absolute(temp.path()); + let cwd = absolute(dunce::canonicalize(temp.path()).unwrap()); fs::write(cwd.join(".git"), "gitdir: repo\n").unwrap(); let calls = Arc::new(BlockingMetadataCalls::default()); calls.release.add_permits(10); @@ -535,14 +569,25 @@ async fn empty_remote_repo_skills_are_reused_until_watch_invalidation() { calls: Arc::clone(&calls), }); let access = FileSystemEnvironmentAccessor::unrestricted(&remote); - let cache = Mutex::new(super::EmptyRemoteRootsCache::default()); - let config = stack(Vec::new()); - let discover = || repo_agents_skill_roots(Some(&access), &config, &cwd, Some(&cache)); - assert!(discover().await.is_empty()); + let service = HostSkillsService::new(cwd.clone(), /*bundled_skills_enabled*/ false); + let input = HostSkillsLoadInput::new(cwd.clone(), Vec::new(), stack(Vec::new())) + .with_environment_id("remote"); + let discover = async || { + service + .snapshot_for_config_on_executor(&input, Some((&remote, &access))) + .await + }; + assert!(discover().await.outcome().skills.is_empty()); calls.watch_ready.notified().await; - // No clock deadline or polling during an idle connection. + let cold_marker_probes = calls.marker_paths.lock().unwrap().clone(); + assert!(!cold_marker_probes.is_empty()); + // No marker or skill-root RPCs, clock deadline, or polling during an idle connection. tokio::time::advance(std::time::Duration::from_secs(3600)).await; - assert!(discover().await.is_empty()); + assert!(discover().await.outcome().skills.is_empty()); + assert_eq!( + calls.marker_paths.lock().unwrap().as_slice(), + cold_marker_probes + ); assert_eq!(calls.paths.lock().unwrap().len(), 1); // The same executor with different captured permissions must scan again. @@ -555,13 +600,17 @@ async fn empty_remote_repo_skills_are_reused_until_watch_invalidation() { .unwrap(), ); assert!( - repo_agents_skill_roots(Some(&read_only), &config, &cwd, Some(&cache)) + service + .snapshot_for_config_on_executor(&input, Some((&remote, &read_only))) .await + .outcome() + .skills .is_empty() ); assert_eq!(calls.paths.lock().unwrap().len(), 2); + assert!(calls.marker_paths.lock().unwrap().len() > cold_marker_probes.len()); calls.watch_ready.notified().await; - assert!(discover().await.is_empty()); + assert!(discover().await.outcome().skills.is_empty()); calls.watch_ready.notified().await; // Read restrictions cannot use the executor's unsandboxed watch operation. @@ -588,13 +637,86 @@ async fn empty_remote_repo_skills_are_reused_until_watch_invalidation() { let skills = cwd.join(".agents/skills"); fs::create_dir_all(&skills).unwrap(); + let skill_path = skills.join("SKILL.md"); + let skill_body = "---\nname: remote-skill\ndescription: Remote skill\n---\nBody"; + fs::write(&skill_path, skill_body).unwrap(); calls.watch.lock().unwrap().take(); - assert_eq!(discover().await[0].path, skills); - fs::remove_dir(&skills).unwrap(); - assert!(discover().await.is_empty()); + let snapshot = discover().await; + assert_eq!(snapshot.outcome().skills.len(), 1); + assert_eq!(snapshot.outcome().skill_roots, vec![skills.clone()]); + // Remote discovery canonicalizes identity once and does no implicit-command index RPCs. + assert_eq!( + *calls.canonicalized_paths.lock().unwrap(), + vec![ + PathUri::from_abs_path(&skills), + PathUri::from_abs_path(&skill_path) + ] + ); + fs::remove_dir_all(&skills).unwrap(); + assert!(discover().await.outcome().skill_roots.is_empty()); fs::create_dir(&skills).unwrap(); - cache.lock().unwrap().clear(); - assert_eq!(discover().await[0].path, skills); + fs::write(&skill_path, skill_body).unwrap(); + service.clear_cache(); + assert_eq!(discover().await.outcome().skill_roots, vec![skills]); +} + +#[tokio::test(start_paused = true)] +async fn incomplete_project_marker_probe_is_retried_before_loading_parent_skills() { + let temp = TempDir::new().unwrap(); + let root = absolute(dunce::canonicalize(temp.path()).unwrap()); + let cwd = root.join("nested"); + fs::create_dir(&cwd).unwrap(); + fs::write(root.join(".git"), "").unwrap(); + fs::write(cwd.join(".git"), "").unwrap(); + let parent_skills = root.join(".agents/skills"); + fs::create_dir_all(&parent_skills).unwrap(); + fs::write( + parent_skills.join("SKILL.md"), + "---\nname: parent-skill\ndescription: Parent skill\n---\nBody", + ) + .unwrap(); + let calls = Arc::new(BlockingMetadataCalls::default()); + calls.release.add_permits(10); + *calls.denied_marker.lock().unwrap() = Some(PathUri::from_abs_path(&cwd.join(".git"))); + let remote: Arc = Arc::new(BlockingMetadataFileSystem { + inner: Arc::clone(&LOCAL_FS), + calls: Arc::clone(&calls), + }); + let access = FileSystemEnvironmentAccessor::unrestricted(&remote); + let service = HostSkillsService::new(root.clone(), /*bundled_skills_enabled*/ false); + let input = HostSkillsLoadInput::new(cwd.clone(), Vec::new(), stack(Vec::new())) + .with_environment_id("remote"); + assert!( + service + .snapshot_for_config_on_executor(&input, Some((&remote, &access))) + .await + .outcome() + .skills + .is_empty() + ); + assert!(calls.watch.lock().unwrap().is_none()); + calls.paths.lock().unwrap().clear(); + calls.denied_marker.lock().unwrap().take(); + let read_only = FileSystemEnvironmentAccessor::new( + &remote, + FileSystemSandboxContext::from_legacy_sandbox_policy( + SandboxPolicy::new_read_only_policy(), + PathUri::from_abs_path(&cwd), + ) + .unwrap(), + ); + assert!( + service + .snapshot_for_config_on_executor(&input, Some((&remote, &read_only))) + .await + .outcome() + .skills + .is_empty() + ); + assert_eq!( + *calls.paths.lock().unwrap(), + vec![PathUri::from_abs_path(&cwd.join(".agents/skills"))] + ); } #[tokio::test(start_paused = true)] @@ -617,14 +739,28 @@ async fn newly_created_roots_are_visible_before_watch_ack_or_without_watch_suppo let cache = Mutex::new(super::EmptyRemoteRootsCache::default()); let config = stack(Vec::new()); assert!( - repo_agents_skill_roots(Some(&access), &config, &cwd, Some(&cache)) - .await - .is_empty() + repo_agents_skill_roots( + Some(&access), + &config, + &cwd, + /*project_markers*/ None, + Some(&cache) + ) + .await + .is_empty() ); let skills = cwd.join(".agents/skills"); fs::create_dir_all(&skills).unwrap(); assert_eq!( - repo_agents_skill_roots(Some(&access), &config, &cwd, Some(&cache)).await[0].path, + repo_agents_skill_roots( + Some(&access), + &config, + &cwd, + /*project_markers*/ None, + Some(&cache) + ) + .await[0] + .path, skills ); } @@ -648,6 +784,7 @@ async fn repo_ancestry_stops_at_project_root_and_preserves_root_to_cwd_order() { Some(&access), &config_stack, &nested, + /*project_markers*/ None, /*empty_remote_roots*/ None, ) .await @@ -680,6 +817,7 @@ async fn resolved_project_layer_loads_skill_without_git_marker() { &access, &config_stack, &workspace, + /*project_markers*/ None, /*home_dir*/ None, Vec::new(), Vec::new(), @@ -720,6 +858,7 @@ async fn resolved_project_layer_loads_skill_when_cwd_is_file() { &access, &config_stack, &cwd, + /*project_markers*/ None, /*home_dir*/ None, Vec::new(), Vec::new(), @@ -743,7 +882,7 @@ async fn resolved_project_layer_loads_skill_when_cwd_is_file() { #[tokio::test] async fn repo_ancestry_limits_concurrent_probes_and_preserves_order() { - const CONCURRENCY_LIMIT: usize = 256; + const CONCURRENCY_LIMIT: usize = 8; let temp_dir = TempDir::new().expect("temp dir"); let repository = absolute(temp_dir.path().join("repo")); @@ -816,6 +955,7 @@ async fn repo_ancestry_limits_concurrent_probes_and_preserves_order() { Some(&access), &config_stack, &cwd, + /*project_markers*/ None, /*empty_remote_roots*/ None ), assertions, @@ -883,6 +1023,7 @@ async fn resolved_config_and_repo_roots_preserve_order_and_dedupe_paths_not_name &access, &config_stack, &cwd, + /*project_markers*/ None, Some(&home_folder), Vec::new(), vec![user_skills], diff --git a/codex-rs/ext/skills/src/host_service.rs b/codex-rs/ext/skills/src/host_service.rs index be9f20b016..105e360ff0 100644 --- a/codex-rs/ext/skills/src/host_service.rs +++ b/codex-rs/ext/skills/src/host_service.rs @@ -12,6 +12,7 @@ use codex_exec_server::EnvironmentAccess; use codex_exec_server::EnvironmentAccessKey; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::FileSystemEnvironmentAccessor; +use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -50,6 +51,7 @@ struct ConfigSkillsCacheEntry { #[derive(Debug, Clone)] pub struct HostSkillsLoadInput { cwd: AbsolutePathBuf, + implicit_repository_skills: bool, effective_skill_roots: Vec, config_layer_stack: ConfigLayerStack, plugin_skill_snapshots: Option>, @@ -63,12 +65,27 @@ impl HostSkillsLoadInput { ) -> Self { Self { cwd, + implicit_repository_skills: true, effective_skill_roots, config_layer_stack, plugin_skill_snapshots: None, } } + /// Retains the turn's configuration and plugin selection for a new repository location. + pub fn for_cwd(&self, cwd: AbsolutePathBuf) -> Self { + Self { + cwd, + ..self.clone() + } + } + + /// Host implicit command attribution only runs for the local environment. + pub fn with_environment_id(mut self, environment_id: &str) -> Self { + self.implicit_repository_skills = environment_id == LOCAL_ENVIRONMENT_ID; + self + } + /// Attaches plugin skill snapshots parsed during plugin loading, when available. pub fn with_plugin_skill_snapshots( mut self, @@ -175,28 +192,59 @@ impl HostSkillsService { /// This path uses a cache keyed by the effective skill-relevant config state rather than just /// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen /// to share a directory. + pub async fn snapshot_for_config( + &self, + input: &HostSkillsLoadInput, + repository: Option<&dyn EnvironmentAccess>, + ) -> HostSkillsSnapshot { + self.snapshot_for_config_with_project_markers(input, repository, repository) + .await + } + + /// Load repository skills with current access and marker metadata from the same executor. + /// Unrestricted marker checks preserve hidden repository boundaries and run only on watch misses. + pub async fn snapshot_for_config_on_executor( + &self, + input: &HostSkillsLoadInput, + repository: Option<(&Arc, &dyn EnvironmentAccess)>, + ) -> HostSkillsSnapshot { + let project_markers = repository + .map(|(file_system, _)| FileSystemEnvironmentAccessor::unrestricted(file_system)); + self.snapshot_for_config_with_project_markers( + input, + repository.map(|(_, access)| access), + project_markers + .as_ref() + .map(|access| access as &dyn EnvironmentAccess), + ) + .await + } + #[instrument( name = "skills_for_config", level = "info", skip_all, fields(otel.name = "skills_for_config") )] - pub async fn snapshot_for_config( + async fn snapshot_for_config_with_project_markers( &self, input: &HostSkillsLoadInput, repository: Option<&dyn EnvironmentAccess>, + project_markers: Option<&dyn EnvironmentAccess>, ) -> HostSkillsSnapshot { // User and plugin roots retain host authority independently of the selected executor. let host = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); let roots = self - .skill_roots_for_config(input, repository, &host, Some(&self.empty_remote_roots)) + .skill_roots_for_config( + input, + repository, + project_markers, + &host, + Some(&self.empty_remote_roots), + ) .await; let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); - let cache_key = config_skills_cache_key( - &roots, - &skill_config_rules, - input.plugin_skill_snapshots.as_ref(), - ); + let cache_key = config_skills_cache_key(&roots, &skill_config_rules, input); self.snapshot_for_skill_roots( input, roots, @@ -222,6 +270,7 @@ impl HostSkillsService { self.skill_roots_for_config( input, Some(&repository), + Some(&repository), &host, /*empty_remote_roots*/ None, ) @@ -236,6 +285,7 @@ impl HostSkillsService { &self, input: &HostSkillsLoadInput, repository: Option<&'a dyn EnvironmentAccess>, + project_markers: Option<&dyn EnvironmentAccess>, host: &'a dyn EnvironmentAccess, empty_remote_roots: Option<&Mutex>, ) -> Vec> { @@ -248,6 +298,7 @@ impl HostSkillsService { host, &input.config_layer_stack, &input.cwd, + project_markers, input.effective_skill_roots.clone(), self.extra_roots(), empty_remote_roots, @@ -291,6 +342,7 @@ impl HostSkillsService { &host, &input.config_layer_stack, &input.cwd, + repository, input.effective_skill_roots.clone(), self.extra_roots(), Some(&self.empty_remote_roots), @@ -301,11 +353,7 @@ impl HostSkillsService { } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let snapshot = if use_cwd_cache { - let cache_key = config_skills_cache_key( - &roots, - &skill_config_rules, - input.plugin_skill_snapshots.as_ref(), - ); + let cache_key = config_skills_cache_key(&roots, &skill_config_rules, input); self.snapshot_for_skill_roots( input, roots, @@ -386,6 +434,11 @@ impl HostSkillsService { skill_config_rules: &SkillConfigRules, request_root_snapshots: Option<&RequestSkillRootSnapshots>, ) -> SkillLoadOutcome { + let accessors = roots + .iter() + .filter(|root| input.implicit_repository_skills || root.scope != SkillScope::Repo) + .map(|root| root.file_system) + .collect::>(); let outcome = load_and_merge_host_skill_roots_with_request_snapshots( roots, &self.root_scan_slots, @@ -400,7 +453,9 @@ impl HostSkillsService { .iter() .map(|skill| (skill.name.as_str(), &skill.path_to_skills_md)), ); - outcome.with_disabled_paths(disabled_paths) + outcome + .with_disabled_paths(disabled_paths, &accessors) + .await } pub fn clear_cache(&self) { @@ -481,6 +536,7 @@ impl SkillRootLoader for HostSkillsService { #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct ConfigSkillsCacheKey { roots: Vec, + implicit_repository_skills: bool, skill_config_rules: SkillConfigRules, plugin_skill_snapshots: Option>, } @@ -497,12 +553,15 @@ pub(crate) struct ConfigSkillRootCacheKey { fn config_skills_cache_key( roots: &[HostSkillRoot<'_>], skill_config_rules: &SkillConfigRules, - plugin_skill_snapshots: Option<&SkillRootSnapshots>, + input: &HostSkillsLoadInput, ) -> ConfigSkillsCacheKey { ConfigSkillsCacheKey { roots: roots.iter().map(config_skill_root_cache_key).collect(), + implicit_repository_skills: input.implicit_repository_skills, skill_config_rules: skill_config_rules.clone(), - plugin_skill_snapshots: plugin_skill_snapshots + plugin_skill_snapshots: input + .plugin_skill_snapshots + .as_ref() .filter(|_| roots.iter().any(|root| root.plugin_identity().is_some())) .cloned(), } diff --git a/codex-rs/ext/skills/src/host_service_tests.rs b/codex-rs/ext/skills/src/host_service_tests.rs index 0d5e22c819..946f1e71c6 100644 --- a/codex-rs/ext/skills/src/host_service_tests.rs +++ b/codex-rs/ext/skills/src/host_service_tests.rs @@ -816,15 +816,40 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { assert!(loaded_names.contains("repo-skill")); let other_file_system: Arc = Arc::new(codex_exec_server::LocalFileSystem::unsandboxed()); + let other_access = FileSystemEnvironmentAccessor::unrestricted(&other_file_system); let other_snapshot = skills_service - .snapshot_for_config( - &skills_input, - Some( - &codex_exec_server::FileSystemEnvironmentAccessor::unrestricted(&other_file_system), - ), - ) + .snapshot_for_config(&skills_input, Some(&other_access)) .await; assert!(!std::ptr::eq(snapshot.outcome(), other_snapshot.outcome())); + let outcome = other_snapshot.outcome(); + let repo_skill = outcome + .skills + .iter() + .find(|skill| skill.name == "repo-skill") + .unwrap(); + let repo_skill_path = repo_skill.path_to_skills_md.to_abs_path().unwrap(); + assert_eq!( + outcome.implicit_skill_for_doc_path(&repo_skill_path), + Some(repo_skill) + ); + assert_eq!( + outcome.implicit_skill_for_scripts_dir(&repo_skill_path.parent().unwrap().join("scripts")), + Some(repo_skill) + ); + let remote_snapshot = skills_service + .snapshot_for_config( + &skills_input.with_environment_id("remote"), + Some(&other_access), + ) + .await; + let remote_outcome = remote_snapshot.outcome(); + for skill in &remote_outcome.skills { + assert_eq!( + remote_outcome + .implicit_skill_for_doc_path(&skill.path_to_skills_md.to_abs_path().unwrap()), + (skill.scope != SkillScope::Repo).then_some(skill) + ); + } } #[tokio::test] diff --git a/codex-rs/ext/skills/src/host_snapshot.rs b/codex-rs/ext/skills/src/host_snapshot.rs index efa75ab093..f6b2a6d88c 100644 --- a/codex-rs/ext/skills/src/host_snapshot.rs +++ b/codex-rs/ext/skills/src/host_snapshot.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use crate::SkillLoadOutcome; use codex_exec_server::EnvironmentAccess; +use codex_extension_api::ExtensionData; use codex_skills::SkillMetadata; /// Immutable snapshot of host-owned skills and their source filesystems. @@ -16,6 +17,13 @@ impl HostSkillsSnapshot { Self { outcome } } + /// Prefer the snapshot prepared with this step's permissions when the host supplies one. + pub fn for_step(step_store: &ExtensionData, turn_store: &ExtensionData) -> Option> { + step_store + .get::() + .or_else(|| turn_store.get::()) + } + pub fn outcome(&self) -> &SkillLoadOutcome { self.outcome.as_ref() } diff --git a/codex-rs/ext/skills/src/invocation.rs b/codex-rs/ext/skills/src/invocation.rs index 57e7a3a92b..cc99a07b21 100644 --- a/codex-rs/ext/skills/src/invocation.rs +++ b/codex-rs/ext/skills/src/invocation.rs @@ -48,7 +48,7 @@ pub fn detect_implicit_skill_invocation( ) -> Option { if environment_id == LOCAL_ENVIRONMENT_ID && let Some(native_workdir) = native_workdir - && let Some(host_snapshot) = turn_store.get::() + && let Some(host_snapshot) = HostSkillsSnapshot::for_step(step_store, turn_store) && let Some(skill) = detect_implicit_skill_invocation_for_command( host_snapshot.outcome(), command, diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 68fdd28965..8a22ab477d 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -56,6 +56,11 @@ pub use sources::SkillProviders; pub use state::SkillsThreadState; pub use telemetry::record_plugin_turn_usage; +/// Selected environments whose discovery can use a turn-scoped filesystem grant. +pub struct TurnSkillFilesystemGrants { + pub environment_ids: std::collections::HashSet, +} + /// Recognizes persisted explicit skill prompts without exposing their fragment implementation. pub fn is_skill_prompt_fragment(text: &str) -> bool { ::matches_text( diff --git a/codex-rs/ext/skills/src/loader/discovery.rs b/codex-rs/ext/skills/src/loader/discovery.rs index c7e04136f9..a2e2f3d953 100644 --- a/codex-rs/ext/skills/src/loader/discovery.rs +++ b/codex-rs/ext/skills/src/loader/discovery.rs @@ -38,6 +38,7 @@ pub(super) struct SkillDiscovery { pub plugin_roots: HashSet, pub namespace_roots: HashSet, pub warnings: Vec, + pub error: Option, } pub(super) struct DiscoveredSkill { @@ -61,6 +62,7 @@ pub(super) async fn discover_skills( plugin_roots: HashSet::new(), namespace_roots: HashSet::new(), warnings: Vec::new(), + error: None, }; let walk = match file_system .walk( @@ -88,6 +90,9 @@ pub(super) async fn discover_skills( Err(error) if error.kind() == io::ErrorKind::NotFound => return empty_discovery(), Err(error) => { let mut discovery = empty_discovery(); + if error.kind() == io::ErrorKind::Unsupported { + discovery.error = Some(error.to_string()); + } discovery .warnings .push(format!("failed to walk skills root {root}: {error:#}")); @@ -173,6 +178,7 @@ pub(super) async fn discover_skills( plugin_roots, namespace_roots: HashSet::from([root.clone()]), warnings, + error: None, } } diff --git a/codex-rs/ext/skills/src/loader/host.rs b/codex-rs/ext/skills/src/loader/host.rs index eb142b092d..dc2b986275 100644 --- a/codex-rs/ext/skills/src/loader/host.rs +++ b/codex-rs/ext/skills/src/loader/host.rs @@ -167,6 +167,7 @@ async fn load_skills_under_root( plugin_roots, mut namespace_roots, warnings, + error, } = discover_skills( file_system, &PathUri::from_abs_path(root), @@ -180,6 +181,16 @@ async fn load_skills_under_root( for warning in warnings { error!("{warning}"); } + if let Some(message) = error { + return ( + Vec::new(), + Arc::default(), + vec![SkillError { + path: root.clone(), + message, + }], + ); + } if skills.is_empty() { return (Vec::new(), Arc::default(), Vec::new()); } diff --git a/codex-rs/ext/skills/src/loader/mod.rs b/codex-rs/ext/skills/src/loader/mod.rs index 53da2d5dcd..f4dabcfe88 100644 --- a/codex-rs/ext/skills/src/loader/mod.rs +++ b/codex-rs/ext/skills/src/loader/mod.rs @@ -17,6 +17,7 @@ pub(crate) use host_merge::load_and_merge_host_skill_roots; pub(crate) use host_merge::load_and_merge_host_skill_roots_with_request_snapshots; pub(crate) const MAX_CONCURRENT_ROOT_SCANS: usize = 8; +pub(crate) use discovery::MAX_CONCURRENT_SKILL_LOADS; pub(super) const SKILLS_FILENAME: &str = "SKILL.md"; pub(super) const SKILLS_METADATA_DIR: &str = "agents"; pub(super) const SKILLS_METADATA_FILENAME: &str = "openai.yaml"; diff --git a/codex-rs/ext/skills/src/preparation.rs b/codex-rs/ext/skills/src/preparation.rs index 258541cf05..fb4d40d29d 100644 --- a/codex-rs/ext/skills/src/preparation.rs +++ b/codex-rs/ext/skills/src/preparation.rs @@ -111,9 +111,7 @@ async fn discover_host_catalog( input: &StepPreparationInput<'_>, config: &SkillsExtensionConfig, ) -> Option { - let host_snapshot = input - .turn_store - .get::() + let host_snapshot = HostSkillsSnapshot::for_step(input.step_store, input.turn_store) .filter(|_| providers.has_host_provider())?; if !config.include_instructions && !config.shadow_selection_enabled { return Some(SkillCatalog::default()); diff --git a/codex-rs/ext/skills/src/render_dedup.rs b/codex-rs/ext/skills/src/render_dedup.rs index 3fe7721d12..73722eda46 100644 --- a/codex-rs/ext/skills/src/render_dedup.rs +++ b/codex-rs/ext/skills/src/render_dedup.rs @@ -5,6 +5,8 @@ use std::borrow::Cow; use std::collections::BTreeMap; use std::collections::HashSet; +use codex_protocol::protocol::SkillScope; + use super::AvailableSkillsRender; use super::CatalogLines; use super::SkillCatalogRenderPolicy; @@ -17,7 +19,9 @@ use super::render_catalog; use crate::aliases::AliasPlan; use crate::catalog::SkillCatalog; use crate::catalog::SkillCatalogEntry; +use crate::catalog::SkillSourceKind; use crate::catalog_delta::CatalogSkill; +use crate::catalog_delta::GrantStatus; use crate::catalog_delta::SkillEntrySnapshot; use crate::catalog_prompt::SkillPromptKind; @@ -82,6 +86,8 @@ impl<'a> PreparedSkillCatalog<'a> { pub(crate) fn entries_for_render( &self, rendered: &AvailableSkillsRender, + grants: Option<&crate::TurnSkillFilesystemGrants>, + primary_environment: Option<&str>, ) -> BTreeMap { let lines = if rendered.skill_root_lines.is_empty() { self.unaliased() @@ -118,10 +124,26 @@ impl<'a> PreparedSkillCatalog<'a> { .and_then(|plan| plan.root_for_locator(locator)) .cloned(); let shown = shown.as_ref().is_none_or(|ids| ids.contains(&id)); + let environment = match &entry.authority.kind { + SkillSourceKind::Executor => entry.main_prompt.environment_id(), + SkillSourceKind::Host if entry.prompt_scope() == Some(SkillScope::Repo) => { + primary_environment + } + SkillSourceKind::Host | SkillSourceKind::Cloud | SkillSourceKind::Custom(_) => { + None + } + }; + let granted = environment.is_some_and(|id| { + grants.is_some_and(|grants| grants.environment_ids.contains(id)) + }); ( id, CatalogSkill { - snapshot: SkillEntrySnapshot { fingerprint, shown }, + snapshot: SkillEntrySnapshot { + fingerprint, + shown, + grant: granted.then_some(GrantStatus::Active), + }, line: line.render_full(), root, }, diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index eca0a1492b..b01411694f 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -65,7 +65,9 @@ fn edits_beyond_the_catalog_description_cap_keep_the_same_fingerprint() { ) .unwrap(); prepared - .entries_for_render(&rendered) + .entries_for_render( + &rendered, /*grants*/ None, /*primary_environment*/ None, + ) .into_values() .next() .unwrap() diff --git a/codex-rs/ext/skills/src/world_state_catalogs.rs b/codex-rs/ext/skills/src/world_state_catalogs.rs index 7e3b07eeb5..4821f3669a 100644 --- a/codex-rs/ext/skills/src/world_state_catalogs.rs +++ b/codex-rs/ext/skills/src/world_state_catalogs.rs @@ -251,13 +251,26 @@ impl<'a> CatalogRenderingContext<'a> { SkillCatalogRenderPolicy::CORE_COMPATIBLE .with_omitted_descriptions(self.step_state.config.omit_descriptions), ); + let grants = self + .input + .step_store + .get::(); + let primary_environment = self + .input + .environments + .first() + .map(|selection| selection.environment_id.as_str()); for (catalog, rendered) in [ (&executor, &mut rendered.executor), (&cloud, &mut rendered.cloud), (&host, &mut rendered.host), ] { if let Some(rendered) = rendered { - rendered.entries = catalog.entries_for_render(rendered); + rendered.entries = catalog.entries_for_render( + rendered, + grants.as_deref(), + primary_environment, + ); } } } @@ -438,6 +451,7 @@ impl<'a> CatalogRenderingContext<'a> { CatalogDelta { rendered, include_usage: self.include_usage, + tracks_grants: kind != CatalogKind::Cloud, allocation: (kind == CatalogKind::Cloud).then_some(allocation), }, self.input diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index f27e0ce57f..70ba0221ab 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -1,3 +1,4 @@ +use codex_skills::ImplicitSkillLookup; use std::collections::HashMap; use std::io; use std::sync::Arc; @@ -88,6 +89,9 @@ impl SyntheticFileSystem { if path == &self.alias_root { return Ok(self.canonical_root.clone()); } + if path == &self.path("skill/scripts")? { + return self.path("scripts-target"); + } self.metadata(path)?; Ok(path.clone()) } @@ -305,6 +309,14 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { let skill = outcome.skills[0].clone(); assert_eq!(skill.name, "synthetic"); + assert_eq!( + outcome.implicit_skill_for_scripts_dir(&canonical_root.join("scripts-target")), + Some(&skill), + ); + assert_eq!( + outcome.implicit_skill_for_doc_path(&canonical_root.join("skill/SKILL.md")), + Some(&skill), + ); assert_eq!( skill.path_to_skills_md, PathUri::from_abs_path(&canonical_root.join("skill/SKILL.md")) diff --git a/codex-rs/ext/skills/tests/skills_extension/catalog_delta_tests.rs b/codex-rs/ext/skills/tests/skills_extension/catalog_delta_tests.rs index 44f7689949..6f51122c8b 100644 --- a/codex-rs/ext/skills/tests/skills_extension/catalog_delta_tests.rs +++ b/codex-rs/ext/skills/tests/skills_extension/catalog_delta_tests.rs @@ -86,25 +86,186 @@ async fn skill_sources_spend_the_same_remaining_budget() -> TestResult { Ok(()) } +#[tokio::test] +async fn grant_expiry_withdraws_only_missing_skills_when_metadata_budget_is_exhausted() -> TestResult +{ + let enabled = TestConfig { + catalog_deltas_enabled: true, + ..default_config() + }; + let survivor = [("permanent", "Always available.")]; + let original = [("unrelated", "Removed independently.")]; + for grant_when_removed in [None, Some("granting-turn")] { + let mut previous = serde_json::Map::new(); + let baseline = render_catalogs_for_step( + &original, + &enabled, + &mut previous, + /*granting_turn*/ None, + /*context_window*/ 60_000, + ) + .await?; + assert!(baseline.contains("- executor-unrelated:"), "{baseline}"); + let both = [ + original[0], + survivor[0], + ("temporary", "Turn only."), + ("env-b/independent", "Available without the grant on env-1."), + ]; + let granted = render_catalogs_for_step( + &both, + &enabled, + &mut previous, + Some("granting-turn"), + /*context_window*/ 60_000, + ) + .await?; + assert!(granted.contains("- executor-permanent:"), "{granted}"); + assert!(granted.contains("- executor-independent:"), "{granted}"); + assert!(granted.contains("- executor-temporary:"), "{granted}"); + let expired = render_catalogs_for_step( + &survivor, + &enabled, + &mut previous, + grant_when_removed, + /*context_window*/ 1, + ) + .await?; + assert_eq!( + expired, + format!("\n## Selected-environment skills update\n{REVALIDATION}") + ); + assert_eq!( + render_catalogs_for_step( + &survivor, + &enabled, + &mut previous, + /*granting_turn*/ None, + /*context_window*/ 1 + ) + .await?, + "" + ); + let restored = [survivor[0], ("temporary", "Turn only.")]; + for expected in [RESTORATION, ""] { + assert_eq!( + render_catalogs_for_step( + &restored, + &enabled, + &mut previous, + Some("reapproved"), + /*context_window*/ 1, + ) + .await?, + expected + ); + } + assert_eq!( + render_catalogs_for_step( + &survivor, + &enabled, + &mut previous, + /*granting_turn*/ None, + /*context_window*/ 1, + ) + .await?, + format!("\n## Selected-environment skills update\n{REVALIDATION}") + ); + assert_eq!( + render_catalogs_for_step( + &[], + &enabled, + &mut previous, + /*granting_turn*/ None, + /*context_window*/ 1 + ) + .await?, + "" + ); + } + Ok(()) +} + +const RESTORATION: &str = "\n## Selected-environment skills update\nSome previously unavailable skills are available again. Verify current access before relying on a skill from this source.\n"; +const REVALIDATION: &str = "Some previously listed skills are no longer available. Verify current access before relying on a skill from this source.\n"; + +#[tokio::test] +async fn legacy_filesystem_catalog_revalidates_missing_rows_once() -> TestResult { + let config = TestConfig { + catalog_deltas_enabled: true, + ..default_config() + }; + let mut previous = serde_json::Map::new(); + let survivor = [("stay", "Keep this skill.")]; + let both = [survivor[0], ("gone", "Temporarily available.")]; + render_catalogs_for_step( + &both, + &config, + &mut previous, + /*granting_turn*/ None, + /*context_window*/ 60_000, + ) + .await?; + for snapshot in previous.values_mut() { + snapshot.as_object_mut().unwrap().remove("tracksGrants"); + } + assert_eq!( + render_catalogs(&survivor, &config, &mut previous).await?, + format!( + "\n## Selected-environment skills update\n{REVALIDATION}\n## Host skills update\n{REVALIDATION}" + ) + ); + assert_eq!( + render_catalogs(&survivor, &config, &mut previous).await?, + "" + ); + assert_eq!(render_catalogs(&[], &config, &mut previous).await?, ""); + Ok(()) +} + async fn render_catalogs( skills: &[(&str, &str)], config: &TestConfig, previous: &mut serde_json::Map, ) -> Result> { - let provider = |kind: SkillSourceKind| { - let entries = skills - .iter() - .map(|(name, description)| { - let name = format!("{kind}-{name}"); - let path = format!("/skills/{name}/SKILL.md"); - let mut entry = - test_entry(kind.clone(), "skills", &format!("{kind}/{name}"), &path) - .with_display_path(path); - entry.description = description.to_string(); - entry - }) - .collect(); - Arc::new(StaticSkillProvider { + render_catalogs_for_step( + skills, config, previous, /*granting_turn*/ None, /*context_window*/ 5_000, + ) + .await +} + +async fn render_catalogs_for_step( + skills: &[(&str, &str)], + config: &TestConfig, + previous: &mut serde_json::Map, + granting_turn: Option<&str>, + context_window: i64, +) -> Result> { + let turn_id = granting_turn.unwrap_or("turn"); + let step_store = ExtensionData::new("step"); + if granting_turn.is_some() { + step_store.insert(codex_skills_extension::TurnSkillFilesystemGrants { + environment_ids: ["env-1".to_string()].into(), + }); + } + let provider = |kind: SkillSourceKind| -> Result<_, codex_utils_path_uri::PathUriParseError> { + let mut entries = Vec::new(); + for &(name, description) in skills { + let (environment, name) = name.split_once('/').unwrap_or(("env-1", name)); + let name = format!("{kind}-{name}"); + let path = format!("/skills/{name}/SKILL.md"); + let mut entry = test_entry(kind.clone(), "skills", &format!("{kind}/{name}"), &path); + if kind == SkillSourceKind::Executor { + entry.main_prompt = SkillResourceId::environment( + &path, + environment, + PathUri::parse(&format!("file://{path}"))?, + ); + } + entry.description = description.to_string(); + entries.push(entry.with_display_path(path)); + } + Ok(Arc::new(StaticSkillProvider { catalog: SkillCatalog { entries, ..Default::default() @@ -112,12 +273,12 @@ async fn render_catalogs( read_requests: Arc::new(Mutex::new(Vec::new())), list_calls: None, fail_first_list: false, - }) + })) }; let providers = SkillProviders::new() - .with_executor_provider(provider(SkillSourceKind::Executor)) - .with_cloud_provider(provider(SkillSourceKind::Cloud)) - .with_host_provider(provider(SkillSourceKind::Host)); + .with_executor_provider(provider(SkillSourceKind::Executor)?) + .with_cloud_provider(provider(SkillSourceKind::Cloud)?) + .with_host_provider(provider(SkillSourceKind::Host)?); let mut builder = ExtensionRegistryBuilder::new(); install_with_providers(&mut builder, providers, skills_extension_config); let registry = builder.build(); @@ -135,7 +296,7 @@ async fn render_catalogs( thread_store: &thread_store, }) .await; - start_registered_turn(®istry, &session_store, &thread_store, "turn").await; + start_registered_turn(®istry, &session_store, &thread_store, turn_id).await; let roots = [SelectedCapabilityRoot { id: "skills".to_string(), location: CapabilityRootLocation::Environment { @@ -143,7 +304,7 @@ async fn render_catalogs( path: PathUri::parse("file:///skills")?, }, }]; - let turn_store = ExtensionData::new("turn"); + let turn_store = ExtensionData::new(turn_id); turn_store.insert(HostSkillsSnapshot::new(Arc::new( SkillLoadOutcome::default(), ))); @@ -152,14 +313,14 @@ async fn render_catalogs( &roots, WorldStateContributionInput { previous_world_state: Some(previous), - step_store: &ExtensionData::new("step"), + step_store: &step_store, model_info: &ModelInfo { - context_window: Some(5_000), + context_window: Some(context_window), include_skills_usage_instructions: true, ..catalog_model_info() }, thread_id: codex_protocol::ThreadId::new(), - turn_id: "turn", + turn_id, environments: &[], extension_metrics: None, session_store: &session_store,