From 258ba436f1f784aa058625aec6b6653df83fe9c6 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 30 Mar 2026 10:48:49 -0700 Subject: [PATCH 1/5] codex-tools: extract discoverable tool models (#16254) ## Why `#16193` moved the pure `tool_search` and `tool_suggest` spec builders into `codex-tools`, but `codex-core` still owned the shared discoverable-tool model that those builders and the `tool_suggest` runtime both depend on. This change continues the migration by moving that reusable model boundary out of `codex-core` as well, so the discovery/suggestion stack uses one shared set of types and `core/src/tools` no longer needs its own `discoverable.rs` module. ## What changed - Moved `DiscoverableTool`, `DiscoverablePluginInfo`, and `filter_tool_suggest_discoverable_tools_for_client()` into `codex-rs/tools/src/tool_discovery.rs` alongside the extracted discovery/suggestion spec builders. - Added `codex-app-server-protocol` as a `codex-tools` dependency so the shared discoverable-tool model can own the connector-side `AppInfo` variant directly. - Updated `core/src/tools/handlers/tool_suggest.rs`, `core/src/tools/spec.rs`, `core/src/tools/router.rs`, `core/src/connectors.rs`, and `core/src/codex.rs` to consume the shared `codex-tools` model instead of the old core-local declarations. - Changed `core/src/plugins/discoverable.rs` to return `DiscoverablePluginInfo` directly, moved the pure client-filter coverage into `tool_discovery_tests.rs`, and deleted the old `core/src/tools/discoverable.rs` module. - Updated `codex-rs/tools/README.md` so the crate boundary documents that `codex-tools` now owns the discoverable-tool models in addition to the discovery/suggestion spec builders. ## Test plan - `cargo test -p codex-tools` - `CARGO_TARGET_DIR=/tmp/codex-core-discoverable-model cargo test -p codex-core --lib tools::handlers::tool_suggest::` - `CARGO_TARGET_DIR=/tmp/codex-core-discoverable-model cargo test -p codex-core --lib tools::spec::` - `CARGO_TARGET_DIR=/tmp/codex-core-discoverable-model cargo test -p codex-core --lib plugins::discoverable::` - `just bazel-lock-check` - `just argument-comment-lint` ## References - #16193 - #16154 - #15923 - #15928 - #15944 - #15953 - #16031 - #16047 - #16129 - #16132 - #16138 - #16141 --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/codex.rs | 3 +- codex-rs/core/src/connectors.rs | 4 +- codex-rs/core/src/plugins/discoverable.rs | 27 ++++-- .../core/src/plugins/discoverable_tests.rs | 32 ++----- codex-rs/core/src/tools/discoverable.rs | 94 ------------------- .../core/src/tools/handlers/tool_suggest.rs | 4 +- .../src/tools/handlers/tool_suggest_tests.rs | 52 +--------- codex-rs/core/src/tools/mod.rs | 1 - codex-rs/core/src/tools/router.rs | 2 +- codex-rs/core/src/tools/spec.rs | 2 +- codex-rs/core/src/tools/spec_tests.rs | 3 +- codex-rs/tools/Cargo.toml | 1 + codex-rs/tools/README.md | 2 +- codex-rs/tools/src/lib.rs | 3 + codex-rs/tools/src/tool_discovery.rs | 75 +++++++++++++++ codex-rs/tools/src/tool_discovery_tests.rs | 49 ++++++++++ 17 files changed, 168 insertions(+), 187 deletions(-) delete mode 100644 codex-rs/core/src/tools/discoverable.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 3dd569c6b0..f8d1de44fc 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2613,6 +2613,7 @@ dependencies = [ name = "codex-tools" version = "0.0.0" dependencies = [ + "codex-app-server-protocol", "codex-code-mode", "codex-protocol", "pretty_assertions", diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 9a8cc9cea7..2c70aad7a7 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -121,6 +121,7 @@ use codex_protocol::request_user_input::RequestUserInputResponse; use codex_rmcp_client::ElicitationResponse; use codex_rmcp_client::OAuthCredentialsStoreMode; use codex_terminal_detection::user_agent; +use codex_tools::filter_tool_suggest_discoverable_tools_for_client; use codex_utils_output_truncation::TruncationPolicy; use codex_utils_stream_parser::AssistantTextChunk; use codex_utils_stream_parser::AssistantTextStreamParser; @@ -6546,7 +6547,7 @@ pub(crate) async fn built_tools( ) .await .map(|discoverable_tools| { - crate::tools::discoverable::filter_tool_suggest_discoverable_tools_for_client( + filter_tool_suggest_discoverable_tools_for_client( discoverable_tools, turn_context.app_server_client_name.as_deref(), ) diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index e4999b9b41..a15138f6fb 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -18,6 +18,7 @@ use codex_connectors::AllConnectorsCacheKey; use codex_connectors::DirectoryListResponse; use codex_login::token_data::TokenData; use codex_protocol::protocol::SandboxPolicy; +use codex_tools::DiscoverableTool; use rmcp::model::ToolAnnotations; use serde::Deserialize; use serde::de::DeserializeOwned; @@ -44,8 +45,6 @@ use crate::mcp_connection_manager::codex_apps_tools_cache_key; use crate::plugins::AppConnectorId; use crate::plugins::PluginsManager; use crate::plugins::list_tool_suggest_discoverable_plugins; -use crate::tools::discoverable::DiscoverablePluginInfo; -use crate::tools::discoverable::DiscoverableTool; use codex_features::Feature; pub use codex_connectors::CONNECTORS_CACHE_TTL; @@ -133,7 +132,6 @@ pub(crate) async fn list_tool_suggest_discoverable_tools_with_auth( .map(DiscoverableTool::from); let discoverable_plugins = list_tool_suggest_discoverable_plugins(config)? .into_iter() - .map(DiscoverablePluginInfo::from) .map(DiscoverableTool::from); Ok(discoverable_connectors .chain(discoverable_plugins) diff --git a/codex-rs/core/src/plugins/discoverable.rs b/codex-rs/core/src/plugins/discoverable.rs index 427706235f..f7006185bf 100644 --- a/codex-rs/core/src/plugins/discoverable.rs +++ b/codex-rs/core/src/plugins/discoverable.rs @@ -9,6 +9,7 @@ use super::PluginsManager; use crate::config::Config; use crate::config::types::ToolSuggestDiscoverableType; use codex_features::Feature; +use codex_tools::DiscoverablePluginInfo; const TOOL_SUGGEST_DISCOVERABLE_PLUGIN_ALLOWLIST: &[&str] = &[ "github@openai-curated", @@ -23,7 +24,7 @@ const TOOL_SUGGEST_DISCOVERABLE_PLUGIN_ALLOWLIST: &[&str] = &[ pub(crate) fn list_tool_suggest_discoverable_plugins( config: &Config, -) -> anyhow::Result> { +) -> anyhow::Result> { if !config.features.enabled(Feature::Plugins) { return Ok(Vec::new()); } @@ -47,7 +48,7 @@ pub(crate) fn list_tool_suggest_discoverable_plugins( return Ok(Vec::new()); }; - let mut discoverable_plugins = Vec::::new(); + let mut discoverable_plugins = Vec::::new(); for plugin in curated_marketplace.plugins { if plugin.installed || (!TOOL_SUGGEST_DISCOVERABLE_PLUGIN_ALLOWLIST.contains(&plugin.id.as_str()) @@ -66,14 +67,28 @@ pub(crate) fn list_tool_suggest_discoverable_plugins( marketplace_path: curated_marketplace.path.clone(), }, ) { - Ok(plugin) => discoverable_plugins.push(plugin.plugin.into()), + Ok(plugin) => { + let plugin: PluginCapabilitySummary = plugin.plugin.into(); + discoverable_plugins.push(DiscoverablePluginInfo { + id: plugin.config_name, + name: plugin.display_name, + description: plugin.description, + has_skills: plugin.has_skills, + mcp_server_names: plugin.mcp_server_names, + app_connector_ids: plugin + .app_connector_ids + .into_iter() + .map(|connector_id| connector_id.0) + .collect(), + }); + } Err(err) => warn!("failed to load discoverable plugin suggestion {plugin_id}: {err:#}"), } } discoverable_plugins.sort_by(|left, right| { - left.display_name - .cmp(&right.display_name) - .then_with(|| left.config_name.cmp(&right.config_name)) + left.name + .cmp(&right.name) + .then_with(|| left.id.cmp(&right.id)) }); Ok(discoverable_plugins) } diff --git a/codex-rs/core/src/plugins/discoverable_tests.rs b/codex-rs/core/src/plugins/discoverable_tests.rs index fd9df9ddf6..70ac887cb6 100644 --- a/codex-rs/core/src/plugins/discoverable_tests.rs +++ b/codex-rs/core/src/plugins/discoverable_tests.rs @@ -5,7 +5,7 @@ use crate::plugins::test_support::write_curated_plugin_sha; use crate::plugins::test_support::write_file; use crate::plugins::test_support::write_openai_curated_marketplace; use crate::plugins::test_support::write_plugins_feature_config; -use crate::tools::discoverable::DiscoverablePluginInfo; +use codex_tools::DiscoverablePluginInfo; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -18,11 +18,7 @@ async fn list_tool_suggest_discoverable_plugins_returns_uninstalled_curated_plug write_plugins_feature_config(codex_home.path()); let config = load_plugins_config(codex_home.path()).await; - let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config) - .unwrap() - .into_iter() - .map(DiscoverablePluginInfo::from) - .collect::>(); + let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config).unwrap(); assert_eq!( discoverable_plugins, @@ -52,11 +48,7 @@ plugins = false ); let config = load_plugins_config(codex_home.path()).await; - let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config) - .unwrap() - .into_iter() - .map(DiscoverablePluginInfo::from) - .collect::>(); + let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config).unwrap(); assert_eq!(discoverable_plugins, Vec::::new()); } @@ -76,11 +68,7 @@ async fn list_tool_suggest_discoverable_plugins_normalizes_description() { ); let config = load_plugins_config(codex_home.path()).await; - let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config) - .unwrap() - .into_iter() - .map(DiscoverablePluginInfo::from) - .collect::>(); + let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config).unwrap(); assert_eq!( discoverable_plugins, @@ -115,11 +103,7 @@ async fn list_tool_suggest_discoverable_plugins_omits_installed_curated_plugins( .expect("plugin should install"); let refreshed_config = load_plugins_config(codex_home.path()).await; - let discoverable_plugins = list_tool_suggest_discoverable_plugins(&refreshed_config) - .unwrap() - .into_iter() - .map(DiscoverablePluginInfo::from) - .collect::>(); + let discoverable_plugins = list_tool_suggest_discoverable_plugins(&refreshed_config).unwrap(); assert_eq!(discoverable_plugins, Vec::::new()); } @@ -140,11 +124,7 @@ discoverables = [{ type = "plugin", id = "sample@openai-curated" }] ); let config = load_plugins_config(codex_home.path()).await; - let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config) - .unwrap() - .into_iter() - .map(DiscoverablePluginInfo::from) - .collect::>(); + let discoverable_plugins = list_tool_suggest_discoverable_plugins(&config).unwrap(); assert_eq!( discoverable_plugins, diff --git a/codex-rs/core/src/tools/discoverable.rs b/codex-rs/core/src/tools/discoverable.rs deleted file mode 100644 index 78d8935aa7..0000000000 --- a/codex-rs/core/src/tools/discoverable.rs +++ /dev/null @@ -1,94 +0,0 @@ -use crate::plugins::PluginCapabilitySummary; -use codex_app_server_protocol::AppInfo; -use codex_tools::DiscoverableToolType; - -const TUI_CLIENT_NAME: &str = "codex-tui"; - -#[derive(Clone, Debug, PartialEq)] -pub(crate) enum DiscoverableTool { - Connector(Box), - Plugin(Box), -} - -impl DiscoverableTool { - pub(crate) fn tool_type(&self) -> DiscoverableToolType { - match self { - Self::Connector(_) => DiscoverableToolType::Connector, - Self::Plugin(_) => DiscoverableToolType::Plugin, - } - } - - pub(crate) fn id(&self) -> &str { - match self { - Self::Connector(connector) => connector.id.as_str(), - Self::Plugin(plugin) => plugin.id.as_str(), - } - } - - pub(crate) fn name(&self) -> &str { - match self { - Self::Connector(connector) => connector.name.as_str(), - Self::Plugin(plugin) => plugin.name.as_str(), - } - } - - pub(crate) fn install_url(&self) -> Option<&str> { - match self { - Self::Connector(connector) => connector.install_url.as_deref(), - Self::Plugin(_) => None, - } - } -} - -impl From for DiscoverableTool { - fn from(value: AppInfo) -> Self { - Self::Connector(Box::new(value)) - } -} - -impl From for DiscoverableTool { - fn from(value: DiscoverablePluginInfo) -> Self { - Self::Plugin(Box::new(value)) - } -} - -pub(crate) fn filter_tool_suggest_discoverable_tools_for_client( - discoverable_tools: Vec, - app_server_client_name: Option<&str>, -) -> Vec { - if app_server_client_name != Some(TUI_CLIENT_NAME) { - return discoverable_tools; - } - - discoverable_tools - .into_iter() - .filter(|tool| !matches!(tool, DiscoverableTool::Plugin(_))) - .collect() -} - -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) struct DiscoverablePluginInfo { - pub(crate) id: String, - pub(crate) name: String, - pub(crate) description: Option, - pub(crate) has_skills: bool, - pub(crate) mcp_server_names: Vec, - pub(crate) app_connector_ids: Vec, -} - -impl From for DiscoverablePluginInfo { - fn from(value: PluginCapabilitySummary) -> Self { - Self { - id: value.config_name, - name: value.display_name, - description: value.description, - has_skills: value.has_skills, - mcp_server_names: value.mcp_server_names, - app_connector_ids: value - .app_connector_ids - .into_iter() - .map(|connector_id| connector_id.0) - .collect(), - } - } -} diff --git a/codex-rs/core/src/tools/handlers/tool_suggest.rs b/codex-rs/core/src/tools/handlers/tool_suggest.rs index 478bd2c837..c68ac8589c 100644 --- a/codex-rs/core/src/tools/handlers/tool_suggest.rs +++ b/codex-rs/core/src/tools/handlers/tool_suggest.rs @@ -8,8 +8,10 @@ use codex_app_server_protocol::McpElicitationSchema; use codex_app_server_protocol::McpServerElicitationRequest; use codex_app_server_protocol::McpServerElicitationRequestParams; use codex_rmcp_client::ElicitationAction; +use codex_tools::DiscoverableTool; use codex_tools::DiscoverableToolAction; use codex_tools::DiscoverableToolType; +use codex_tools::filter_tool_suggest_discoverable_tools_for_client; use rmcp::model::RequestId; use serde::Deserialize; use serde::Serialize; @@ -22,8 +24,6 @@ use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; -use crate::tools::discoverable::DiscoverableTool; -use crate::tools::discoverable::filter_tool_suggest_discoverable_tools_for_client; use crate::tools::handlers::parse_arguments; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; diff --git a/codex-rs/core/src/tools/handlers/tool_suggest_tests.rs b/codex-rs/core/src/tools/handlers/tool_suggest_tests.rs index c932e03145..56089a1aa9 100644 --- a/codex-rs/core/src/tools/handlers/tool_suggest_tests.rs +++ b/codex-rs/core/src/tools/handlers/tool_suggest_tests.rs @@ -5,9 +5,9 @@ use crate::plugins::test_support::load_plugins_config; use crate::plugins::test_support::write_curated_plugin_sha; use crate::plugins::test_support::write_openai_curated_marketplace; use crate::plugins::test_support::write_plugins_feature_config; -use crate::tools::discoverable::DiscoverablePluginInfo; -use crate::tools::discoverable::filter_tool_suggest_discoverable_tools_for_client; use codex_app_server_protocol::AppInfo; +use codex_tools::DiscoverablePluginInfo; +use codex_tools::DiscoverableTool; use codex_tools::DiscoverableToolAction; use codex_tools::DiscoverableToolType; use codex_utils_absolute_path::AbsolutePathBuf; @@ -161,54 +161,6 @@ fn build_tool_suggestion_meta_uses_expected_shape() { ); } -#[test] -fn filter_tool_suggest_discoverable_tools_for_codex_tui_omits_plugins() { - let discoverable_tools = vec![ - DiscoverableTool::Connector(Box::new(AppInfo { - id: "connector_google_calendar".to_string(), - name: "Google Calendar".to_string(), - description: Some("Plan events and schedules.".to_string()), - logo_url: None, - logo_url_dark: None, - distribution_channel: None, - branding: None, - app_metadata: None, - labels: None, - install_url: Some("https://example.test/google-calendar".to_string()), - is_accessible: false, - is_enabled: true, - plugin_display_names: Vec::new(), - })), - DiscoverableTool::Plugin(Box::new(DiscoverablePluginInfo { - id: "slack@openai-curated".to_string(), - name: "Slack".to_string(), - description: Some("Search Slack messages".to_string()), - has_skills: true, - mcp_server_names: vec!["slack".to_string()], - app_connector_ids: vec!["connector_slack".to_string()], - })), - ]; - - assert_eq!( - filter_tool_suggest_discoverable_tools_for_client(discoverable_tools, Some("codex-tui"),), - vec![DiscoverableTool::Connector(Box::new(AppInfo { - id: "connector_google_calendar".to_string(), - name: "Google Calendar".to_string(), - description: Some("Plan events and schedules.".to_string()), - logo_url: None, - logo_url_dark: None, - distribution_channel: None, - branding: None, - app_metadata: None, - labels: None, - install_url: Some("https://example.test/google-calendar".to_string()), - is_accessible: false, - is_enabled: true, - plugin_display_names: Vec::new(), - }))] - ); -} - #[test] fn verified_connector_suggestion_completed_requires_accessible_connector() { let accessible_connectors = vec![AppInfo { diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 57859bba42..e866d1f5f7 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -1,6 +1,5 @@ pub mod code_mode; pub mod context; -pub(crate) mod discoverable; pub mod events; pub(crate) mod handlers; pub mod js_repl; diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 1145e3a8f2..646c1d1387 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -7,7 +7,6 @@ use crate::sandboxing::SandboxPermissions; use crate::tools::context::SharedTurnDiffTracker; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; -use crate::tools::discoverable::DiscoverableTool; use crate::tools::registry::AnyToolResult; use crate::tools::registry::ToolRegistry; use crate::tools::spec::ToolsConfig; @@ -18,6 +17,7 @@ use codex_protocol::models::ResponseItem; use codex_protocol::models::SearchToolCallParams; use codex_protocol::models::ShellToolCallParams; use codex_tools::ConfiguredToolSpec; +use codex_tools::DiscoverableTool; use rmcp::model::Tool; use std::collections::HashMap; use std::sync::Arc; diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 6852671576..d0614fc35b 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -7,7 +7,6 @@ use crate::shell::Shell; use crate::shell::ShellType; use crate::tools::code_mode::PUBLIC_TOOL_NAME; use crate::tools::code_mode::WAIT_TOOL_NAME; -use crate::tools::discoverable::DiscoverableTool; use crate::tools::handlers::PLAN_TOOL; use crate::tools::handlers::TOOL_SEARCH_DEFAULT_LIMIT; use crate::tools::handlers::TOOL_SEARCH_TOOL_NAME; @@ -38,6 +37,7 @@ use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use codex_tools::CommandToolOptions; +use codex_tools::DiscoverableTool; use codex_tools::DiscoverableToolType; use codex_tools::ShellToolOptions; use codex_tools::SpawnAgentToolOptions; diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index 283bcd7161..16f87ca50e 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -4,7 +4,6 @@ use crate::models_manager::model_info::with_config_overrides; use crate::shell::Shell; use crate::shell::ShellType; use crate::tools::ToolRouter; -use crate::tools::discoverable::DiscoverablePluginInfo; use crate::tools::router::ToolRouterParams; use codex_app_server_protocol::AppInfo; use codex_protocol::models::VIEW_IMAGE_TOOL_NAME; @@ -14,6 +13,8 @@ use codex_protocol::openai_models::ModelsResponse; use codex_tools::AdditionalProperties; use codex_tools::CommandToolOptions; use codex_tools::ConfiguredToolSpec; +use codex_tools::DiscoverablePluginInfo; +use codex_tools::DiscoverableTool; use codex_tools::FreeformTool; use codex_tools::ResponsesApiTool; use codex_tools::ResponsesApiWebSearchFilters; diff --git a/codex-rs/tools/Cargo.toml b/codex-rs/tools/Cargo.toml index 9e2d23fb58..3d61dab7d8 100644 --- a/codex-rs/tools/Cargo.toml +++ b/codex-rs/tools/Cargo.toml @@ -8,6 +8,7 @@ version.workspace = true workspace = true [dependencies] +codex-app-server-protocol = { workspace = true } codex-code-mode = { workspace = true } codex-protocol = { workspace = true } rmcp = { workspace = true, default-features = false, features = [ diff --git a/codex-rs/tools/README.md b/codex-rs/tools/README.md index 59533f459d..cafb4b89ac 100644 --- a/codex-rs/tools/README.md +++ b/codex-rs/tools/README.md @@ -27,7 +27,7 @@ schema and Responses API tool primitives that no longer need to live in - local host tool spec builders for shell/exec/request-permissions/view-image - collaboration and agent-job `ToolSpec` builders for spawn/send/wait/close, `request_user_input`, and CSV fanout/reporting -- tool discovery and suggestion models / `ToolSpec` builders for +- discoverable-tool models, client filtering, and `ToolSpec` builders for `tool_search` and `tool_suggest` - `parse_tool_input_schema()` - `parse_dynamic_tool()` diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index 9dd629f916..9ae6031ccd 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -67,12 +67,15 @@ pub use responses_api::mcp_tool_to_deferred_responses_api_tool; pub use responses_api::mcp_tool_to_responses_api_tool; pub use responses_api::tool_definition_to_responses_api_tool; pub use tool_definition::ToolDefinition; +pub use tool_discovery::DiscoverablePluginInfo; +pub use tool_discovery::DiscoverableTool; pub use tool_discovery::DiscoverableToolAction; pub use tool_discovery::DiscoverableToolType; pub use tool_discovery::ToolSearchAppInfo; pub use tool_discovery::ToolSuggestEntry; pub use tool_discovery::create_tool_search_tool; pub use tool_discovery::create_tool_suggest_tool; +pub use tool_discovery::filter_tool_suggest_discoverable_tools_for_client; pub use tool_spec::ConfiguredToolSpec; pub use tool_spec::ResponsesApiWebSearchFilters; pub use tool_spec::ResponsesApiWebSearchUserLocation; diff --git a/codex-rs/tools/src/tool_discovery.rs b/codex-rs/tools/src/tool_discovery.rs index f2b9b4f934..0216cc1703 100644 --- a/codex-rs/tools/src/tool_discovery.rs +++ b/codex-rs/tools/src/tool_discovery.rs @@ -1,10 +1,13 @@ use crate::JsonSchema; use crate::ResponsesApiTool; use crate::ToolSpec; +use codex_app_server_protocol::AppInfo; use serde::Deserialize; use serde::Serialize; use std::collections::BTreeMap; +const TUI_CLIENT_NAME: &str = "codex-tui"; + #[derive(Clone, Debug, PartialEq, Eq)] pub struct ToolSearchAppInfo { pub name: String, @@ -34,6 +37,78 @@ pub enum DiscoverableToolAction { Enable, } +#[derive(Clone, Debug, PartialEq)] +pub enum DiscoverableTool { + Connector(Box), + Plugin(Box), +} + +impl DiscoverableTool { + pub fn tool_type(&self) -> DiscoverableToolType { + match self { + Self::Connector(_) => DiscoverableToolType::Connector, + Self::Plugin(_) => DiscoverableToolType::Plugin, + } + } + + pub fn id(&self) -> &str { + match self { + Self::Connector(connector) => connector.id.as_str(), + Self::Plugin(plugin) => plugin.id.as_str(), + } + } + + pub fn name(&self) -> &str { + match self { + Self::Connector(connector) => connector.name.as_str(), + Self::Plugin(plugin) => plugin.name.as_str(), + } + } + + pub fn install_url(&self) -> Option<&str> { + match self { + Self::Connector(connector) => connector.install_url.as_deref(), + Self::Plugin(_) => None, + } + } +} + +impl From for DiscoverableTool { + fn from(value: AppInfo) -> Self { + Self::Connector(Box::new(value)) + } +} + +impl From for DiscoverableTool { + fn from(value: DiscoverablePluginInfo) -> Self { + Self::Plugin(Box::new(value)) + } +} + +pub fn filter_tool_suggest_discoverable_tools_for_client( + discoverable_tools: Vec, + app_server_client_name: Option<&str>, +) -> Vec { + if app_server_client_name != Some(TUI_CLIENT_NAME) { + return discoverable_tools; + } + + discoverable_tools + .into_iter() + .filter(|tool| !matches!(tool, DiscoverableTool::Plugin(_))) + .collect() +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DiscoverablePluginInfo { + pub id: String, + pub name: String, + pub description: Option, + pub has_skills: bool, + pub mcp_server_names: Vec, + pub app_connector_ids: Vec, +} + #[derive(Clone, Debug, PartialEq, Eq)] pub struct ToolSuggestEntry { pub id: String, diff --git a/codex-rs/tools/src/tool_discovery_tests.rs b/codex-rs/tools/src/tool_discovery_tests.rs index 7be6bedd2c..1b241c5844 100644 --- a/codex-rs/tools/src/tool_discovery_tests.rs +++ b/codex-rs/tools/src/tool_discovery_tests.rs @@ -1,4 +1,5 @@ use super::*; +use codex_app_server_protocol::AppInfo; use pretty_assertions::assert_eq; use serde_json::json; use std::collections::BTreeMap; @@ -147,3 +148,51 @@ fn discoverable_tool_enums_use_expected_wire_names() { }) ); } + +#[test] +fn filter_tool_suggest_discoverable_tools_for_codex_tui_omits_plugins() { + let discoverable_tools = vec![ + DiscoverableTool::Connector(Box::new(AppInfo { + id: "connector_google_calendar".to_string(), + name: "Google Calendar".to_string(), + description: Some("Plan events and schedules.".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/google-calendar".to_string()), + is_accessible: false, + is_enabled: true, + plugin_display_names: Vec::new(), + })), + DiscoverableTool::Plugin(Box::new(DiscoverablePluginInfo { + id: "slack@openai-curated".to_string(), + name: "Slack".to_string(), + description: Some("Search Slack messages".to_string()), + has_skills: true, + mcp_server_names: vec!["slack".to_string()], + app_connector_ids: vec!["connector_slack".to_string()], + })), + ]; + + assert_eq!( + filter_tool_suggest_discoverable_tools_for_client(discoverable_tools, Some("codex-tui"),), + vec![DiscoverableTool::Connector(Box::new(AppInfo { + id: "connector_google_calendar".to_string(), + name: "Google Calendar".to_string(), + description: Some("Plan events and schedules.".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/google-calendar".to_string()), + is_accessible: false, + is_enabled: true, + plugin_display_names: Vec::new(), + }))] + ); +} From 9313c49e4c0480189070a149da03b7b6164ebdce Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 30 Mar 2026 11:59:50 -0700 Subject: [PATCH 2/5] fix: close Bazel argument-comment-lint CI gaps (#16253) ## Why The Bazel-backed `argument-comment-lint` CI path had two gaps: - Bazel wildcard target expansion skipped inline unit-test crates from `src/` modules because the generated `*-unit-tests-bin` `rust_test` targets are tagged `manual`. - `argument-comment-mismatch` was still only a warning in the Bazel and packaged-wrapper entrypoints, so a typoed `/*param_name*/` comment could still pass CI even when the lint detected it. That left CI blind to real linux-sandbox examples, including the missing `/*local_port*/` comment in `codex-rs/linux-sandbox/src/proxy_routing.rs` and typoed argument comments in `codex-rs/linux-sandbox/src/landlock.rs`. ## What Changed - Added `tools/argument-comment-lint/list-bazel-targets.sh` so Bazel lint runs cover `//codex-rs/...` plus the manual `rust_test` `*-unit-tests-bin` targets. - Updated `just argument-comment-lint`, `rust-ci.yml`, and `rust-ci-full.yml` to use that helper. - Promoted both `argument-comment-mismatch` and `uncommented-anonymous-literal-argument` to errors in every strict entrypoint: - `tools/argument-comment-lint/lint_aspect.bzl` - `tools/argument-comment-lint/src/bin/argument-comment-lint.rs` - `tools/argument-comment-lint/wrapper_common.py` - Added wrapper/bin coverage for the stricter lint flags and documented the behavior in `tools/argument-comment-lint/README.md`. - Fixed the now-covered callsites in `codex-rs/linux-sandbox/src/proxy_routing.rs`, `codex-rs/linux-sandbox/src/landlock.rs`, and `codex-rs/core/src/shell_snapshot_tests.rs`. This keeps the Bazel target expansion narrow while making the Bazel and prebuilt-linter paths enforce the same strict lint set. ## Verification - `python3 -m unittest discover -s tools/argument-comment-lint -p 'test_*.py'` - `cargo +nightly-2025-09-18 test --manifest-path tools/argument-comment-lint/Cargo.toml` - `just argument-comment-lint` --- .github/workflows/rust-ci-full.yml | 3 +- .github/workflows/rust-ci.yml | 3 +- codex-rs/core/src/shell_snapshot_tests.rs | 12 +++++-- codex-rs/linux-sandbox/src/landlock.rs | 32 +++++++++++++++---- codex-rs/linux-sandbox/src/proxy_routing.rs | 3 +- justfile | 4 +-- tools/argument-comment-lint/README.md | 14 +++++--- tools/argument-comment-lint/lint_aspect.bzl | 1 + .../list-bazel-targets.sh | 13 ++++++++ .../src/bin/argument-comment-lint.rs | 32 ++++++++++++++++--- .../test_wrapper_common.py | 13 ++++++++ tools/argument-comment-lint/wrapper_common.py | 8 +++-- 12 files changed, 111 insertions(+), 27 deletions(-) create mode 100755 tools/argument-comment-lint/list-bazel-targets.sh diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index a09c7fd433..cdedfb49e3 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -116,6 +116,7 @@ jobs: BUILDBUDDY_API_KEY: ${{ secrets.BUILDBUDDY_API_KEY }} shell: bash run: | + bazel_targets="$(./tools/argument-comment-lint/list-bazel-targets.sh)" ./.github/scripts/run-bazel-ci.sh \ -- \ build \ @@ -123,7 +124,7 @@ jobs: --keep_going \ --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ - //codex-rs/... + ${bazel_targets} - name: Run argument comment lint on codex-rs via packaged wrapper if: ${{ runner.os == 'Windows' }} shell: bash diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index a655b3071e..82591d251b 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -176,6 +176,7 @@ jobs: BUILDBUDDY_API_KEY: ${{ secrets.BUILDBUDDY_API_KEY }} shell: bash run: | + bazel_targets="$(./tools/argument-comment-lint/list-bazel-targets.sh)" ./.github/scripts/run-bazel-ci.sh \ -- \ build \ @@ -183,7 +184,7 @@ jobs: --keep_going \ --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ - //codex-rs/... + ${bazel_targets} - name: Run argument comment lint on codex-rs via packaged wrapper if: ${{ runner.os == 'Windows' }} shell: bash diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index 90288300ff..ff700ff7a6 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/codex-rs/core/src/shell_snapshot_tests.rs @@ -313,9 +313,15 @@ async fn timed_out_snapshot_shell_is_terminated() -> Result<()> { shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), }; - let err = run_script_with_timeout(&shell, &script, Duration::from_secs(1), true, dir.path()) - .await - .expect_err("snapshot shell should time out"); + let err = run_script_with_timeout( + &shell, + &script, + Duration::from_secs(1), + /*use_login_shell*/ true, + dir.path(), + ) + .await + .expect_err("snapshot shell should time out"); assert!( err.to_string().contains("timed out"), "expected timeout error, got {err:?}" diff --git a/codex-rs/linux-sandbox/src/landlock.rs b/codex-rs/linux-sandbox/src/landlock.rs index 307f956a4d..d257edf933 100644 --- a/codex-rs/linux-sandbox/src/landlock.rs +++ b/codex-rs/linux-sandbox/src/landlock.rs @@ -274,7 +274,10 @@ mod tests { #[test] fn managed_network_enforces_seccomp_even_for_full_network_policy() { assert_eq!( - should_install_network_seccomp(NetworkSandboxPolicy::Enabled, true), + should_install_network_seccomp( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ true + ), true ); } @@ -282,7 +285,10 @@ mod tests { #[test] fn full_network_policy_without_managed_network_skips_seccomp() { assert_eq!( - should_install_network_seccomp(NetworkSandboxPolicy::Enabled, false), + should_install_network_seccomp( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ false + ), false ); } @@ -291,18 +297,22 @@ mod tests { fn restricted_network_policy_always_installs_seccomp() { assert!(should_install_network_seccomp( NetworkSandboxPolicy::Restricted, - false + /*allow_network_for_proxy*/ false )); assert!(should_install_network_seccomp( NetworkSandboxPolicy::Restricted, - true + /*allow_network_for_proxy*/ true )); } #[test] fn managed_proxy_routes_use_proxy_routed_seccomp_mode() { assert_eq!( - network_seccomp_mode(NetworkSandboxPolicy::Enabled, true, true), + network_seccomp_mode( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ true, + /*proxy_routed_network*/ true + ), Some(NetworkSeccompMode::ProxyRouted) ); } @@ -310,7 +320,11 @@ mod tests { #[test] fn restricted_network_without_proxy_routing_uses_restricted_mode() { assert_eq!( - network_seccomp_mode(NetworkSandboxPolicy::Restricted, false, false), + network_seccomp_mode( + NetworkSandboxPolicy::Restricted, + /*allow_network_for_proxy*/ false, + /*proxy_routed_network*/ false + ), Some(NetworkSeccompMode::Restricted) ); } @@ -318,7 +332,11 @@ mod tests { #[test] fn full_network_without_managed_proxy_skips_network_seccomp_mode() { assert_eq!( - network_seccomp_mode(NetworkSandboxPolicy::Enabled, false, false), + network_seccomp_mode( + NetworkSandboxPolicy::Enabled, + /*allow_network_for_proxy*/ false, + /*proxy_routed_network*/ false + ), None ); } diff --git a/codex-rs/linux-sandbox/src/proxy_routing.rs b/codex-rs/linux-sandbox/src/proxy_routing.rs index f57472c8a1..07e1893ee3 100644 --- a/codex-rs/linux-sandbox/src/proxy_routing.rs +++ b/codex-rs/linux-sandbox/src/proxy_routing.rs @@ -718,7 +718,8 @@ mod tests { #[test] fn rewrites_proxy_url_to_local_loopback_port() { let rewritten = - rewrite_proxy_env_value("socks5h://127.0.0.1:8081", 43210).expect("rewritten value"); + rewrite_proxy_env_value("socks5h://127.0.0.1:8081", /*local_port*/ 43210) + .expect("rewritten value"); assert_eq!(rewritten, "socks5h://127.0.0.1:43210"); } diff --git a/justfile b/justfile index a39d2dd6ba..1a1295020f 100644 --- a/justfile +++ b/justfile @@ -74,7 +74,7 @@ bazel-clippy: [no-cd] bazel-argument-comment-lint: - bazel build --config=argument-comment-lint -- //codex-rs/... + bazel build --config=argument-comment-lint -- $(./tools/argument-comment-lint/list-bazel-targets.sh) bazel-remote-test: bazel test --test_tag_filters=-argument-comment-lint //... --config=remote --platforms=//:rbe --keep_going @@ -102,7 +102,7 @@ write-hooks-schema: [no-cd] argument-comment-lint *args: if [ "$#" -eq 0 ]; then \ - bazel build --config=argument-comment-lint -- //codex-rs/...; \ + bazel build --config=argument-comment-lint -- $(./tools/argument-comment-lint/list-bazel-targets.sh); \ else \ ./tools/argument-comment-lint/run-prebuilt-linter.py "$@"; \ fi diff --git a/tools/argument-comment-lint/README.md b/tools/argument-comment-lint/README.md index 78e28f308f..1b4895e325 100644 --- a/tools/argument-comment-lint/README.md +++ b/tools/argument-comment-lint/README.md @@ -129,7 +129,8 @@ Run the lint against `codex-rs` from the repo root: ```bash just argument-comment-lint -bazel build --config=argument-comment-lint -- //codex-rs/... +bazel build --config=argument-comment-lint -- \ + $(./tools/argument-comment-lint/list-bazel-targets.sh) ./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core just argument-comment-lint -p codex-core ``` @@ -138,10 +139,13 @@ If no package selection is provided, `just argument-comment-lint` now defaults to the Bazel aspect path over `//codex-rs/...`. The Python wrappers remain the package-scoped escape hatch and still default the underlying Cargo invocation to `--all-targets` unless you explicitly narrow the target set, so targeted -wrapper runs cover test-only call sites by default. +wrapper runs cover test-only call sites by default. The Bazel entrypoints use +`tools/argument-comment-lint/list-bazel-targets.sh` to add the internal +manual `*-unit-tests-bin` Rust targets explicitly, so inline `#[cfg(test)]` +call sites are covered without pulling in unrelated manual release targets. -Repo runs also promote `uncommented_anonymous_literal_argument` to an error by -default: +Repo runs also promote `argument_comment_mismatch` and +`uncommented_anonymous_literal_argument` to errors by default: ```bash ./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core @@ -154,7 +158,7 @@ rustc incremental compilation ICE locally. To override that behavior for an ad hoc run: ```bash -DYLINT_RUSTFLAGS="-A uncommented-anonymous-literal-argument" \ +DYLINT_RUSTFLAGS="-A argument-comment-mismatch -A uncommented-anonymous-literal-argument" \ CARGO_INCREMENTAL=1 \ ./tools/argument-comment-lint/run.py -p codex-core ``` diff --git a/tools/argument-comment-lint/lint_aspect.bzl b/tools/argument-comment-lint/lint_aspect.bzl index 2f0878d9f8..b3de768af1 100644 --- a/tools/argument-comment-lint/lint_aspect.bzl +++ b/tools/argument-comment-lint/lint_aspect.bzl @@ -16,6 +16,7 @@ load( ) _STRICT_LINT_FLAGS = [ + "-Dargument-comment-mismatch", "-Duncommented-anonymous-literal-argument", "-Aunknown-lints", ] diff --git a/tools/argument-comment-lint/list-bazel-targets.sh b/tools/argument-comment-lint/list-bazel-targets.sh new file mode 100755 index 0000000000..cba07f6080 --- /dev/null +++ b/tools/argument-comment-lint/list-bazel-targets.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash + +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +cd "${repo_root}" + +# Bazel wildcard builds skip manual targets, which misses the internal +# `*-unit-tests-bin` rust_test targets generated by `codex_rust_crate()`. +# Add only those manual rust_test targets explicitly so inline `#[cfg(test)]` +# call sites are linted without pulling in unrelated manual release targets. +printf '%s\n' "//codex-rs/..." +bazel query 'kind("rust_test rule", attr(tags, "manual", //codex-rs/...))' diff --git a/tools/argument-comment-lint/src/bin/argument-comment-lint.rs b/tools/argument-comment-lint/src/bin/argument-comment-lint.rs index 5a40d0efb3..9dfe83b0fb 100644 --- a/tools/argument-comment-lint/src/bin/argument-comment-lint.rs +++ b/tools/argument-comment-lint/src/bin/argument-comment-lint.rs @@ -8,6 +8,11 @@ use std::process::ExitCode; use std::time::SystemTime; use std::time::UNIX_EPOCH; +const STRICT_LINTS: [&str; 2] = [ + "argument-comment-mismatch", + "uncommented-anonymous-literal-argument", +]; + fn main() -> ExitCode { match run() { Ok(code) => code, @@ -85,14 +90,13 @@ fn has_library_selection(args: &[OsString]) -> bool { fn set_default_env(command: &mut Command) -> Result<(), String> { if let Some(flags) = env::var_os("DYLINT_RUSTFLAGS") { let mut flags = flags.to_string_lossy().to_string(); - append_flag_if_missing(&mut flags, "-D uncommented-anonymous-literal-argument"); + for strict_lint in STRICT_LINTS { + append_flag_if_missing(&mut flags, &format!("-D {strict_lint}")); + } append_flag_if_missing(&mut flags, "-A unknown_lints"); command.env("DYLINT_RUSTFLAGS", flags); } else { - command.env( - "DYLINT_RUSTFLAGS", - "-D uncommented-anonymous-literal-argument -A unknown_lints", - ); + command.env("DYLINT_RUSTFLAGS", strict_rustflags()); } if env::var_os("CARGO_INCREMENTAL").is_none() { @@ -108,6 +112,15 @@ fn set_default_env(command: &mut Command) -> Result<(), String> { Ok(()) } +fn strict_rustflags() -> String { + let strict_flags = STRICT_LINTS + .iter() + .map(|lint| format!("-D {lint}")) + .collect::>() + .join(" "); + format!("{strict_flags} -A unknown_lints") +} + fn append_flag_if_missing(flags: &mut String, flag: &str) { if flags.contains(flag) { return; @@ -253,6 +266,7 @@ fn exit_code_from_status(code: Option) -> ExitCode { #[cfg(test)] mod tests { use super::normalize_nightly_library_filename; + use super::strict_rustflags; use std::path::Path; #[test] @@ -276,4 +290,12 @@ mod tests { None ); } + + #[test] + fn strict_rustflags_promotes_both_enforced_lints() { + assert_eq!( + strict_rustflags(), + "-D argument-comment-mismatch -D uncommented-anonymous-literal-argument -A unknown_lints" + ); + } } diff --git a/tools/argument-comment-lint/test_wrapper_common.py b/tools/argument-comment-lint/test_wrapper_common.py index 3da6da604b..dbea24b534 100644 --- a/tools/argument-comment-lint/test_wrapper_common.py +++ b/tools/argument-comment-lint/test_wrapper_common.py @@ -103,6 +103,19 @@ class WrapperCommonTest(unittest.TestCase): ], ) + def test_default_lint_env_promotes_both_strict_lints(self) -> None: + env: dict[str, str] = {} + + wrapper_common.set_default_lint_env(env) + + self.assertEqual( + env["DYLINT_RUSTFLAGS"], + "-D argument-comment-mismatch " + "-D uncommented-anonymous-literal-argument " + "-A unknown_lints", + ) + self.assertEqual(env["CARGO_INCREMENTAL"], "0") + if __name__ == "__main__": unittest.main() diff --git a/tools/argument-comment-lint/wrapper_common.py b/tools/argument-comment-lint/wrapper_common.py index 75e03ecfbe..2473029b8e 100644 --- a/tools/argument-comment-lint/wrapper_common.py +++ b/tools/argument-comment-lint/wrapper_common.py @@ -13,7 +13,10 @@ import sys import tempfile from typing import MutableMapping, Sequence -STRICT_LINT = "uncommented-anonymous-literal-argument" +STRICT_LINTS = [ + "argument-comment-mismatch", + "uncommented-anonymous-literal-argument", +] NOISE_LINT = "unknown_lints" TOOLCHAIN_CHANNEL = "nightly-2025-09-18" @@ -129,7 +132,8 @@ def append_env_flag(env: MutableMapping[str, str], key: str, flag: str) -> None: def set_default_lint_env(env: MutableMapping[str, str]) -> None: - append_env_flag(env, "DYLINT_RUSTFLAGS", f"-D {STRICT_LINT}") + for strict_lint in STRICT_LINTS: + append_env_flag(env, "DYLINT_RUSTFLAGS", f"-D {strict_lint}") append_env_flag(env, "DYLINT_RUSTFLAGS", f"-A {NOISE_LINT}") if not env.get("CARGO_INCREMENTAL"): env["CARGO_INCREMENTAL"] = "0" From 28a9807f8403befd318fc6022872b087b2931ddb Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Mon, 30 Mar 2026 14:27:12 -0700 Subject: [PATCH 3/5] [codex-analytics] refactor analytics to use reducer architecture (#16225) - rework codex analytics crate to use reducer / publish architecture - in anticipation of extensive codex analytics --- codex-rs/Cargo.lock | 1 + codex-rs/analytics/Cargo.toml | 1 + codex-rs/analytics/src/analytics_client.rs | 648 +++++++----------- .../analytics/src/analytics_client_tests.rs | 149 ++++ codex-rs/analytics/src/lib.rs | 9 + 5 files changed, 421 insertions(+), 387 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index f8d1de44fc..2edc559c1d 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1337,6 +1337,7 @@ checksum = "e9b18233253483ce2f65329a24072ec414db782531bdbb7d0bbc4bd2ce6b7e21" name = "codex-analytics" version = "0.0.0" dependencies = [ + "codex-app-server-protocol", "codex-git-utils", "codex-login", "codex-plugin", diff --git a/codex-rs/analytics/Cargo.toml b/codex-rs/analytics/Cargo.toml index 9633eebb14..63bf63c911 100644 --- a/codex-rs/analytics/Cargo.toml +++ b/codex-rs/analytics/Cargo.toml @@ -13,6 +13,7 @@ path = "src/lib.rs" workspace = true [dependencies] +codex-app-server-protocol = { workspace = true } codex-git-utils = { workspace = true } codex-login = { workspace = true } codex-plugin = { workspace = true } diff --git a/codex-rs/analytics/src/analytics_client.rs b/codex-rs/analytics/src/analytics_client.rs index ac8d10b6c1..24618168cc 100644 --- a/codex-rs/analytics/src/analytics_client.rs +++ b/codex-rs/analytics/src/analytics_client.rs @@ -1,3 +1,8 @@ +use codex_app_server_protocol::ClientRequest; +use codex_app_server_protocol::ClientResponse; +use codex_app_server_protocol::InitializeParams; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ServerNotification; use codex_git_utils::collect_git_info; use codex_git_utils::get_git_repo_root; use codex_login::AuthManager; @@ -56,9 +61,73 @@ pub struct AppInvocation { pub invocation_type: Option, } +pub enum AnalyticsFact { + Initialize { + connection_id: u64, + params: InitializeParams, + }, + Request { + connection_id: u64, + request_id: RequestId, + request: Box, + }, + Response { + connection_id: u64, + response: Box, + }, + Notification(Box), + // Facts that do not naturally exist on the app-server protocol surface, or + // would require non-trivial protocol reshaping on this branch. + Custom(CustomAnalyticsFact), +} + +pub enum CustomAnalyticsFact { + SkillInvoked(SkillInvokedInput), + AppMentioned(AppMentionedInput), + AppUsed(AppUsedInput), + PluginUsed(PluginUsedInput), + PluginStateChanged(PluginStateChangedInput), +} + +pub struct SkillInvokedInput { + pub tracking: TrackEventsContext, + pub invocations: Vec, +} + +pub struct AppMentionedInput { + pub tracking: TrackEventsContext, + pub mentions: Vec, +} + +pub struct AppUsedInput { + pub tracking: TrackEventsContext, + pub app: AppInvocation, +} + +pub struct PluginUsedInput { + pub tracking: TrackEventsContext, + pub plugin: PluginTelemetryMetadata, +} + +pub struct PluginStateChangedInput { + pub plugin: PluginTelemetryMetadata, + pub state: PluginState, +} + +#[derive(Clone, Copy)] +pub enum PluginState { + Installed, + Uninstalled, + Enabled, + Disabled, +} + +#[derive(Default)] +pub struct AnalyticsReducer; + #[derive(Clone)] pub(crate) struct AnalyticsEventsQueue { - sender: mpsc::Sender, + sender: mpsc::Sender, app_used_emitted_keys: Arc>>, plugin_used_emitted_keys: Arc>>, } @@ -73,33 +142,11 @@ impl AnalyticsEventsQueue { pub(crate) fn new(auth_manager: Arc, base_url: String) -> Self { let (sender, mut receiver) = mpsc::channel(ANALYTICS_EVENTS_QUEUE_SIZE); tokio::spawn(async move { - while let Some(job) = receiver.recv().await { - match job { - TrackEventsJob::SkillInvocations(job) => { - send_track_skill_invocations(&auth_manager, &base_url, job).await; - } - TrackEventsJob::AppMentioned(job) => { - send_track_app_mentioned(&auth_manager, &base_url, job).await; - } - TrackEventsJob::AppUsed(job) => { - send_track_app_used(&auth_manager, &base_url, job).await; - } - TrackEventsJob::PluginUsed(job) => { - send_track_plugin_used(&auth_manager, &base_url, job).await; - } - TrackEventsJob::PluginInstalled(job) => { - send_track_plugin_installed(&auth_manager, &base_url, job).await; - } - TrackEventsJob::PluginUninstalled(job) => { - send_track_plugin_uninstalled(&auth_manager, &base_url, job).await; - } - TrackEventsJob::PluginEnabled(job) => { - send_track_plugin_enabled(&auth_manager, &base_url, job).await; - } - TrackEventsJob::PluginDisabled(job) => { - send_track_plugin_disabled(&auth_manager, &base_url, job).await; - } - } + let mut reducer = AnalyticsReducer; + while let Some(input) = receiver.recv().await { + let mut events = Vec::new(); + reducer.ingest(input, &mut events).await; + send_track_events(&auth_manager, &base_url, events).await; } }); Self { @@ -109,8 +156,8 @@ impl AnalyticsEventsQueue { } } - fn try_send(&self, job: TrackEventsJob) { - if self.sender.try_send(job).is_err() { + fn try_send(&self, input: AnalyticsFact) { + if self.sender.try_send(input).is_err() { //TODO: add a metric for this tracing::warn!("dropping analytics events: queue is full"); } @@ -163,114 +210,86 @@ impl AnalyticsEventsClient { tracking: TrackEventsContext, invocations: Vec, ) { - track_skill_invocations( - &self.queue, - self.analytics_enabled, - Some(tracking), - invocations, - ); + if invocations.is_empty() { + return; + } + self.record_fact(AnalyticsFact::Custom(CustomAnalyticsFact::SkillInvoked( + SkillInvokedInput { + tracking, + invocations, + }, + ))); } pub fn track_app_mentioned(&self, tracking: TrackEventsContext, mentions: Vec) { - track_app_mentioned( - &self.queue, - self.analytics_enabled, - Some(tracking), - mentions, - ); + if mentions.is_empty() { + return; + } + self.record_fact(AnalyticsFact::Custom(CustomAnalyticsFact::AppMentioned( + AppMentionedInput { tracking, mentions }, + ))); } pub fn track_app_used(&self, tracking: TrackEventsContext, app: AppInvocation) { - track_app_used(&self.queue, self.analytics_enabled, Some(tracking), app); + if !self.queue.should_enqueue_app_used(&tracking, &app) { + return; + } + self.record_fact(AnalyticsFact::Custom(CustomAnalyticsFact::AppUsed( + AppUsedInput { tracking, app }, + ))); } pub fn track_plugin_used(&self, tracking: TrackEventsContext, plugin: PluginTelemetryMetadata) { - track_plugin_used(&self.queue, self.analytics_enabled, Some(tracking), plugin); + if !self.queue.should_enqueue_plugin_used(&tracking, &plugin) { + return; + } + self.record_fact(AnalyticsFact::Custom(CustomAnalyticsFact::PluginUsed( + PluginUsedInput { tracking, plugin }, + ))); } pub fn track_plugin_installed(&self, plugin: PluginTelemetryMetadata) { - track_plugin_management( - &self.queue, - self.analytics_enabled, - PluginManagementEventType::Installed, - plugin, - ); + self.record_fact(AnalyticsFact::Custom( + CustomAnalyticsFact::PluginStateChanged(PluginStateChangedInput { + plugin, + state: PluginState::Installed, + }), + )); } pub fn track_plugin_uninstalled(&self, plugin: PluginTelemetryMetadata) { - track_plugin_management( - &self.queue, - self.analytics_enabled, - PluginManagementEventType::Uninstalled, - plugin, - ); + self.record_fact(AnalyticsFact::Custom( + CustomAnalyticsFact::PluginStateChanged(PluginStateChangedInput { + plugin, + state: PluginState::Uninstalled, + }), + )); } pub fn track_plugin_enabled(&self, plugin: PluginTelemetryMetadata) { - track_plugin_management( - &self.queue, - self.analytics_enabled, - PluginManagementEventType::Enabled, - plugin, - ); + self.record_fact(AnalyticsFact::Custom( + CustomAnalyticsFact::PluginStateChanged(PluginStateChangedInput { + plugin, + state: PluginState::Enabled, + }), + )); } pub fn track_plugin_disabled(&self, plugin: PluginTelemetryMetadata) { - track_plugin_management( - &self.queue, - self.analytics_enabled, - PluginManagementEventType::Disabled, - plugin, - ); + self.record_fact(AnalyticsFact::Custom( + CustomAnalyticsFact::PluginStateChanged(PluginStateChangedInput { + plugin, + state: PluginState::Disabled, + }), + )); } -} -enum TrackEventsJob { - SkillInvocations(TrackSkillInvocationsJob), - AppMentioned(TrackAppMentionedJob), - AppUsed(TrackAppUsedJob), - PluginUsed(TrackPluginUsedJob), - PluginInstalled(TrackPluginManagementJob), - PluginUninstalled(TrackPluginManagementJob), - PluginEnabled(TrackPluginManagementJob), - PluginDisabled(TrackPluginManagementJob), -} - -struct TrackSkillInvocationsJob { - analytics_enabled: Option, - tracking: TrackEventsContext, - invocations: Vec, -} - -struct TrackAppMentionedJob { - analytics_enabled: Option, - tracking: TrackEventsContext, - mentions: Vec, -} - -struct TrackAppUsedJob { - analytics_enabled: Option, - tracking: TrackEventsContext, - app: AppInvocation, -} - -struct TrackPluginUsedJob { - analytics_enabled: Option, - tracking: TrackEventsContext, - plugin: PluginTelemetryMetadata, -} - -struct TrackPluginManagementJob { - analytics_enabled: Option, - plugin: PluginTelemetryMetadata, -} - -#[derive(Clone, Copy)] -enum PluginManagementEventType { - Installed, - Uninstalled, - Enabled, - Disabled, + fn record_fact(&self, input: AnalyticsFact) { + if self.analytics_enabled == Some(false) { + return; + } + self.queue.try_send(input); + } } const ANALYTICS_EVENTS_QUEUE_SIZE: usize = 256; @@ -368,286 +387,145 @@ struct CodexPluginUsedEventRequest { event_params: CodexPluginUsedMetadata, } -pub(crate) fn track_skill_invocations( - queue: &AnalyticsEventsQueue, - analytics_enabled: Option, - tracking: Option, - invocations: Vec, -) { - if analytics_enabled == Some(false) { - return; - } - let Some(tracking) = tracking else { - return; - }; - if invocations.is_empty() { - return; - } - let job = TrackEventsJob::SkillInvocations(TrackSkillInvocationsJob { - analytics_enabled, - tracking, - invocations, - }); - queue.try_send(job); -} - -pub(crate) fn track_app_mentioned( - queue: &AnalyticsEventsQueue, - analytics_enabled: Option, - tracking: Option, - mentions: Vec, -) { - if analytics_enabled == Some(false) { - return; - } - let Some(tracking) = tracking else { - return; - }; - if mentions.is_empty() { - return; - } - let job = TrackEventsJob::AppMentioned(TrackAppMentionedJob { - analytics_enabled, - tracking, - mentions, - }); - queue.try_send(job); -} - -pub(crate) fn track_app_used( - queue: &AnalyticsEventsQueue, - analytics_enabled: Option, - tracking: Option, - app: AppInvocation, -) { - if analytics_enabled == Some(false) { - return; - } - let Some(tracking) = tracking else { - return; - }; - if !queue.should_enqueue_app_used(&tracking, &app) { - return; - } - let job = TrackEventsJob::AppUsed(TrackAppUsedJob { - analytics_enabled, - tracking, - app, - }); - queue.try_send(job); -} - -pub(crate) fn track_plugin_used( - queue: &AnalyticsEventsQueue, - analytics_enabled: Option, - tracking: Option, - plugin: PluginTelemetryMetadata, -) { - if analytics_enabled == Some(false) { - return; - } - let Some(tracking) = tracking else { - return; - }; - if !queue.should_enqueue_plugin_used(&tracking, &plugin) { - return; - } - let job = TrackEventsJob::PluginUsed(TrackPluginUsedJob { - analytics_enabled, - tracking, - plugin, - }); - queue.try_send(job); -} - -fn track_plugin_management( - queue: &AnalyticsEventsQueue, - analytics_enabled: Option, - event_type: PluginManagementEventType, - plugin: PluginTelemetryMetadata, -) { - if analytics_enabled == Some(false) { - return; - } - let job = TrackPluginManagementJob { - analytics_enabled, - plugin, - }; - let job = match event_type { - PluginManagementEventType::Installed => TrackEventsJob::PluginInstalled(job), - PluginManagementEventType::Uninstalled => TrackEventsJob::PluginUninstalled(job), - PluginManagementEventType::Enabled => TrackEventsJob::PluginEnabled(job), - PluginManagementEventType::Disabled => TrackEventsJob::PluginDisabled(job), - }; - queue.try_send(job); -} - -async fn send_track_skill_invocations( - auth_manager: &AuthManager, - base_url: &str, - job: TrackSkillInvocationsJob, -) { - let TrackSkillInvocationsJob { - analytics_enabled, - tracking, - invocations, - } = job; - let mut events = Vec::with_capacity(invocations.len()); - for invocation in invocations { - let skill_scope = match invocation.skill_scope { - SkillScope::User => "user", - SkillScope::Repo => "repo", - SkillScope::System => "system", - SkillScope::Admin => "admin", - }; - let repo_root = get_git_repo_root(invocation.skill_path.as_path()); - let repo_url = if let Some(root) = repo_root.as_ref() { - collect_git_info(root) - .await - .and_then(|info| info.repository_url) - } else { - None - }; - let skill_id = skill_id_for_local_skill( - repo_url.as_deref(), - repo_root.as_deref(), - invocation.skill_path.as_path(), - invocation.skill_name.as_str(), - ); - events.push(TrackEventRequest::SkillInvocation( - SkillInvocationEventRequest { - event_type: "skill_invocation", - skill_id, - skill_name: invocation.skill_name.clone(), - event_params: SkillInvocationEventParams { - thread_id: Some(tracking.thread_id.clone()), - invoke_type: Some(invocation.invocation_type), - model_slug: Some(tracking.model_slug.clone()), - product_client_id: Some(originator().value), - repo_url, - skill_scope: Some(skill_scope.to_string()), - }, +impl AnalyticsReducer { + async fn ingest(&mut self, input: AnalyticsFact, out: &mut Vec) { + match input { + AnalyticsFact::Initialize { + connection_id: _connection_id, + params: _params, + } => {} + AnalyticsFact::Request { + connection_id: _connection_id, + request_id: _request_id, + request: _request, + } => {} + AnalyticsFact::Response { + connection_id: _connection_id, + response: _response, + } => {} + AnalyticsFact::Notification(_notification) => {} + AnalyticsFact::Custom(input) => match input { + CustomAnalyticsFact::SkillInvoked(input) => { + self.ingest_skill_invoked(input, out).await; + } + CustomAnalyticsFact::AppMentioned(input) => { + self.ingest_app_mentioned(input, out); + } + CustomAnalyticsFact::AppUsed(input) => { + self.ingest_app_used(input, out); + } + CustomAnalyticsFact::PluginUsed(input) => { + self.ingest_plugin_used(input, out); + } + CustomAnalyticsFact::PluginStateChanged(input) => { + self.ingest_plugin_state_changed(input, out); + } }, - )); + } } - send_track_events(auth_manager, analytics_enabled, base_url, events).await; -} + async fn ingest_skill_invoked( + &mut self, + input: SkillInvokedInput, + out: &mut Vec, + ) { + let SkillInvokedInput { + tracking, + invocations, + } = input; + for invocation in invocations { + let skill_scope = match invocation.skill_scope { + SkillScope::User => "user", + SkillScope::Repo => "repo", + SkillScope::System => "system", + SkillScope::Admin => "admin", + }; + let repo_root = get_git_repo_root(invocation.skill_path.as_path()); + let repo_url = if let Some(root) = repo_root.as_ref() { + collect_git_info(root) + .await + .and_then(|info| info.repository_url) + } else { + None + }; + let skill_id = skill_id_for_local_skill( + repo_url.as_deref(), + repo_root.as_deref(), + invocation.skill_path.as_path(), + invocation.skill_name.as_str(), + ); + out.push(TrackEventRequest::SkillInvocation( + SkillInvocationEventRequest { + event_type: "skill_invocation", + skill_id, + skill_name: invocation.skill_name.clone(), + event_params: SkillInvocationEventParams { + thread_id: Some(tracking.thread_id.clone()), + invoke_type: Some(invocation.invocation_type), + model_slug: Some(tracking.model_slug.clone()), + product_client_id: Some(originator().value), + repo_url, + skill_scope: Some(skill_scope.to_string()), + }, + }, + )); + } + } -async fn send_track_app_mentioned( - auth_manager: &AuthManager, - base_url: &str, - job: TrackAppMentionedJob, -) { - let TrackAppMentionedJob { - analytics_enabled, - tracking, - mentions, - } = job; - let events = mentions - .into_iter() - .map(|mention| { + fn ingest_app_mentioned(&mut self, input: AppMentionedInput, out: &mut Vec) { + let AppMentionedInput { tracking, mentions } = input; + out.extend(mentions.into_iter().map(|mention| { let event_params = codex_app_metadata(&tracking, mention); TrackEventRequest::AppMentioned(CodexAppMentionedEventRequest { event_type: "codex_app_mentioned", event_params, }) - }) - .collect::>(); + })); + } - send_track_events(auth_manager, analytics_enabled, base_url, events).await; + fn ingest_app_used(&mut self, input: AppUsedInput, out: &mut Vec) { + let AppUsedInput { tracking, app } = input; + let event_params = codex_app_metadata(&tracking, app); + out.push(TrackEventRequest::AppUsed(CodexAppUsedEventRequest { + event_type: "codex_app_used", + event_params, + })); + } + + fn ingest_plugin_used(&mut self, input: PluginUsedInput, out: &mut Vec) { + let PluginUsedInput { tracking, plugin } = input; + out.push(TrackEventRequest::PluginUsed(CodexPluginUsedEventRequest { + event_type: "codex_plugin_used", + event_params: codex_plugin_used_metadata(&tracking, plugin), + })); + } + + fn ingest_plugin_state_changed( + &mut self, + input: PluginStateChangedInput, + out: &mut Vec, + ) { + let PluginStateChangedInput { plugin, state } = input; + let event = CodexPluginEventRequest { + event_type: plugin_state_event_type(state), + event_params: codex_plugin_metadata(plugin), + }; + out.push(match state { + PluginState::Installed => TrackEventRequest::PluginInstalled(event), + PluginState::Uninstalled => TrackEventRequest::PluginUninstalled(event), + PluginState::Enabled => TrackEventRequest::PluginEnabled(event), + PluginState::Disabled => TrackEventRequest::PluginDisabled(event), + }); + } } -async fn send_track_app_used(auth_manager: &AuthManager, base_url: &str, job: TrackAppUsedJob) { - let TrackAppUsedJob { - analytics_enabled, - tracking, - app, - } = job; - let event_params = codex_app_metadata(&tracking, app); - let events = vec![TrackEventRequest::AppUsed(CodexAppUsedEventRequest { - event_type: "codex_app_used", - event_params, - })]; - - send_track_events(auth_manager, analytics_enabled, base_url, events).await; -} - -async fn send_track_plugin_used( - auth_manager: &AuthManager, - base_url: &str, - job: TrackPluginUsedJob, -) { - let TrackPluginUsedJob { - analytics_enabled, - tracking, - plugin, - } = job; - let events = vec![TrackEventRequest::PluginUsed(CodexPluginUsedEventRequest { - event_type: "codex_plugin_used", - event_params: codex_plugin_used_metadata(&tracking, plugin), - })]; - - send_track_events(auth_manager, analytics_enabled, base_url, events).await; -} - -async fn send_track_plugin_installed( - auth_manager: &AuthManager, - base_url: &str, - job: TrackPluginManagementJob, -) { - send_track_plugin_management_event(auth_manager, base_url, job, "codex_plugin_installed").await; -} - -async fn send_track_plugin_uninstalled( - auth_manager: &AuthManager, - base_url: &str, - job: TrackPluginManagementJob, -) { - send_track_plugin_management_event(auth_manager, base_url, job, "codex_plugin_uninstalled") - .await; -} - -async fn send_track_plugin_enabled( - auth_manager: &AuthManager, - base_url: &str, - job: TrackPluginManagementJob, -) { - send_track_plugin_management_event(auth_manager, base_url, job, "codex_plugin_enabled").await; -} - -async fn send_track_plugin_disabled( - auth_manager: &AuthManager, - base_url: &str, - job: TrackPluginManagementJob, -) { - send_track_plugin_management_event(auth_manager, base_url, job, "codex_plugin_disabled").await; -} - -async fn send_track_plugin_management_event( - auth_manager: &AuthManager, - base_url: &str, - job: TrackPluginManagementJob, - event_type: &'static str, -) { - let TrackPluginManagementJob { - analytics_enabled, - plugin, - } = job; - let event_params = codex_plugin_metadata(plugin); - let event = CodexPluginEventRequest { - event_type, - event_params, - }; - let events = vec![match event_type { - "codex_plugin_installed" => TrackEventRequest::PluginInstalled(event), - "codex_plugin_uninstalled" => TrackEventRequest::PluginUninstalled(event), - "codex_plugin_enabled" => TrackEventRequest::PluginEnabled(event), - "codex_plugin_disabled" => TrackEventRequest::PluginDisabled(event), - _ => unreachable!("unknown plugin management event type"), - }]; - - send_track_events(auth_manager, analytics_enabled, base_url, events).await; +fn plugin_state_event_type(state: PluginState) -> &'static str { + match state { + PluginState::Installed => "codex_plugin_installed", + PluginState::Uninstalled => "codex_plugin_uninstalled", + PluginState::Enabled => "codex_plugin_enabled", + PluginState::Disabled => "codex_plugin_disabled", + } } fn codex_app_metadata(tracking: &TrackEventsContext, app: AppInvocation) -> CodexAppMetadata { @@ -699,13 +577,9 @@ fn codex_plugin_used_metadata( async fn send_track_events( auth_manager: &AuthManager, - analytics_enabled: Option, base_url: &str, events: Vec, ) { - if analytics_enabled == Some(false) { - return; - } if events.is_empty() { return; } diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index 4f49a7c713..991f977160 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -1,10 +1,20 @@ use super::AnalyticsEventsQueue; +use super::AnalyticsFact; +use super::AnalyticsReducer; use super::AppInvocation; +use super::AppMentionedInput; +use super::AppUsedInput; use super::CodexAppMentionedEventRequest; use super::CodexAppUsedEventRequest; use super::CodexPluginEventRequest; use super::CodexPluginUsedEventRequest; +use super::CustomAnalyticsFact; use super::InvocationType; +use super::PluginState; +use super::PluginStateChangedInput; +use super::PluginUsedInput; +use super::SkillInvocation; +use super::SkillInvokedInput; use super::TrackEventRequest; use super::TrackEventsContext; use super::codex_app_metadata; @@ -280,6 +290,145 @@ fn plugin_used_dedupe_is_keyed_by_turn_and_plugin() { assert_eq!(queue.should_enqueue_plugin_used(&turn_2, &plugin), true); } +#[tokio::test] +async fn reducer_ingests_skill_invoked_fact() { + let mut reducer = AnalyticsReducer; + let mut events = Vec::new(); + let tracking = TrackEventsContext { + model_slug: "gpt-5".to_string(), + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + }; + let skill_path = PathBuf::from("/Users/abc/.codex/skills/doc/SKILL.md"); + let expected_skill_id = super::skill_id_for_local_skill( + /*repo_url*/ None, + /*repo_root*/ None, + skill_path.as_path(), + "doc", + ); + + reducer + .ingest( + AnalyticsFact::Custom(CustomAnalyticsFact::SkillInvoked(SkillInvokedInput { + tracking, + invocations: vec![SkillInvocation { + skill_name: "doc".to_string(), + skill_scope: codex_protocol::protocol::SkillScope::User, + skill_path, + invocation_type: InvocationType::Explicit, + }], + })), + &mut events, + ) + .await; + + let payload = serde_json::to_value(&events).expect("serialize events"); + assert_eq!( + payload, + json!([{ + "event_type": "skill_invocation", + "skill_id": expected_skill_id, + "skill_name": "doc", + "event_params": { + "product_client_id": originator().value, + "skill_scope": "user", + "repo_url": null, + "thread_id": "thread-1", + "invoke_type": "explicit", + "model_slug": "gpt-5" + } + }]) + ); +} + +#[tokio::test] +async fn reducer_ingests_app_and_plugin_facts() { + let mut reducer = AnalyticsReducer; + let mut events = Vec::new(); + let tracking = TrackEventsContext { + model_slug: "gpt-5".to_string(), + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + }; + + reducer + .ingest( + AnalyticsFact::Custom(CustomAnalyticsFact::AppMentioned(AppMentionedInput { + tracking: tracking.clone(), + mentions: vec![AppInvocation { + connector_id: Some("calendar".to_string()), + app_name: Some("Calendar".to_string()), + invocation_type: Some(InvocationType::Explicit), + }], + })), + &mut events, + ) + .await; + reducer + .ingest( + AnalyticsFact::Custom(CustomAnalyticsFact::AppUsed(AppUsedInput { + tracking: tracking.clone(), + app: AppInvocation { + connector_id: Some("drive".to_string()), + app_name: Some("Drive".to_string()), + invocation_type: Some(InvocationType::Implicit), + }, + })), + &mut events, + ) + .await; + reducer + .ingest( + AnalyticsFact::Custom(CustomAnalyticsFact::PluginUsed(PluginUsedInput { + tracking, + plugin: sample_plugin_metadata(), + })), + &mut events, + ) + .await; + + let payload = serde_json::to_value(&events).expect("serialize events"); + assert_eq!(payload.as_array().expect("events array").len(), 3); + assert_eq!(payload[0]["event_type"], "codex_app_mentioned"); + assert_eq!(payload[1]["event_type"], "codex_app_used"); + assert_eq!(payload[2]["event_type"], "codex_plugin_used"); +} + +#[tokio::test] +async fn reducer_ingests_plugin_state_changed_fact() { + let mut reducer = AnalyticsReducer; + let mut events = Vec::new(); + + reducer + .ingest( + AnalyticsFact::Custom(CustomAnalyticsFact::PluginStateChanged( + PluginStateChangedInput { + plugin: sample_plugin_metadata(), + state: PluginState::Disabled, + }, + )), + &mut events, + ) + .await; + + let payload = serde_json::to_value(&events).expect("serialize events"); + assert_eq!( + payload, + json!([{ + "event_type": "codex_plugin_disabled", + "event_params": { + "plugin_id": "sample@test", + "plugin_name": "sample", + "marketplace_name": "test", + "has_skills": true, + "mcp_server_count": 2, + "connector_ids": ["calendar", "drive"], + "product_client_id": originator().value + } + }]) + ); +} + fn sample_plugin_metadata() -> PluginTelemetryMetadata { PluginTelemetryMetadata { plugin_id: PluginId::parse("sample@test").expect("valid plugin id"), diff --git a/codex-rs/analytics/src/lib.rs b/codex-rs/analytics/src/lib.rs index e36f5d71b5..5b15d2830a 100644 --- a/codex-rs/analytics/src/lib.rs +++ b/codex-rs/analytics/src/lib.rs @@ -1,8 +1,17 @@ mod analytics_client; pub use analytics_client::AnalyticsEventsClient; +pub use analytics_client::AnalyticsFact; +pub use analytics_client::AnalyticsReducer; pub use analytics_client::AppInvocation; +pub use analytics_client::AppMentionedInput; +pub use analytics_client::AppUsedInput; +pub use analytics_client::CustomAnalyticsFact; pub use analytics_client::InvocationType; +pub use analytics_client::PluginState; +pub use analytics_client::PluginStateChangedInput; +pub use analytics_client::PluginUsedInput; pub use analytics_client::SkillInvocation; +pub use analytics_client::SkillInvokedInput; pub use analytics_client::TrackEventsContext; pub use analytics_client::build_track_events_context; From 390b644b212602ee58689e9d305ea50ee4a89d5a Mon Sep 17 00:00:00 2001 From: Andrey Mishchenko Date: Mon, 30 Mar 2026 12:31:13 -1000 Subject: [PATCH 4/5] Update code mode exec() instructions (#16279) --- codex-rs/code-mode/src/description.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/codex-rs/code-mode/src/description.rs b/codex-rs/code-mode/src/description.rs index 22639b3246..3a87fef092 100644 --- a/codex-rs/code-mode/src/description.rs +++ b/codex-rs/code-mode/src/description.rs @@ -7,15 +7,17 @@ use crate::PUBLIC_TOOL_NAME; const MAX_JS_SAFE_INTEGER: u64 = (1_u64 << 53) - 1; const CODE_MODE_ONLY_PREFACE: &str = "Use `exec/wait` tool to run all other tools, do not attempt to use any other tools directly"; -const EXEC_DESCRIPTION_TEMPLATE: &str = r#"## exec -- Runs raw JavaScript in an isolated context (no Node, no file system, or network access, no console). -- Send raw JavaScript source text, not JSON, quoted strings, or markdown code fences. +const EXEC_DESCRIPTION_TEMPLATE: &str = r#"Run JavaScript code to orchestrate/compose tool calls +- Evaluates the provided JavaScript code in a fresh V8 isolate as an async module. +- All nested tools are available on the global `tools` object, for example `await tools.exec_command(...)`. Tool names are exposed as normalized JavaScript identifiers, for example `await tools.mcp__ologs__get_profile(...)`. +- Nested tool methods take either a string or an object as their input argument. +- Nested tools return either an object or a string, based on the description. +- Runs raw JavaScript -- no Node, no file system, no network access, no console. +- Accepts raw JavaScript source text, not JSON, quoted strings, or markdown code fences. - You may optionally start the tool input with a first-line pragma like `// @exec: {"yield_time_ms": 10000, "max_output_tokens": 1000}`. - `yield_time_ms` asks `exec` to yield early after that many milliseconds if the script is still running. - `max_output_tokens` sets the token budget for direct `exec` results. By default the result is truncated to 10000 tokens. -- All nested tools are available on the global `tools` object, for example `await tools.exec_command(...)`. Tool names are exposed as normalized JavaScript identifiers, for example `await tools.mcp__ologs__get_profile(...)`. -- Tool methods take either string or object as parameter. -- They return either a structured value or a string based on the description above. +- When the JS code is fully evaluated, the isolate's lifetime ends and unawaited promises are silently discarded. - Global helpers: - `exit()`: Immediately ends the current script successfully (like an early return from the top level). From 19f0d196d11dcf31e02a4d022d099e6d3bcfad6e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 30 Mar 2026 15:32:04 -0700 Subject: [PATCH 5/5] ci: run Windows argument-comment-lint via native Bazel (#16120) ## Why Follow-up to #16106. `argument-comment-lint` already runs as a native Bazel aspect on Linux and macOS, but Windows is still the long pole in `rust-ci`. To move Windows onto the same native Bazel lane, the toolchain split has to let exec-side helper binaries build in an MSVC environment while still linting repo crates as `windows-gnullvm`. Pushing the Windows lane onto the native Bazel path exposed a second round of Windows-only issues in the mixed exec-toolchain plumbing after the initial wrapper/target fixes landed. ## What Changed - keep the Windows lint lanes on the native Bazel/aspect path in `rust-ci.yml` and `rust-ci-full.yml` - add a dedicated `local_windows_msvc` platform for exec-side helper binaries while keeping `local_windows` as the `windows-gnullvm` target platform - patch `rules_rust` so `repository_set(...)` preserves explicit exec-platform constraints for the generated toolchains, keep the Windows-specific bootstrap/direct-link fixes needed for the nightly lint driver, and expose exec-side `rustc-dev` `.rlib`s to the MSVC sysroot - register the custom Windows nightly toolchain set with MSVC exec constraints while still exposing both `x86_64-pc-windows-msvc` and `x86_64-pc-windows-gnullvm` targets - enable `dev_components` on the custom Windows nightly repository set so the MSVC exec helper toolchain actually downloads the compiler-internal crates that `clippy_utils` needs - teach `run-argument-comment-lint-bazel.sh` to enumerate concrete Windows Rust rules, normalize the resulting labels, and skip explicitly requested incompatible targets instead of failing before the lint run starts - patch `rules_rust` build-script env propagation so exec-side `windows-msvc` helper crates drop forwarded MinGW include and linker search paths as whole flag/path pairs instead of emitting malformed `CFLAGS`, `CXXFLAGS`, and `LDFLAGS` - export the Windows VS/MSVC SDK environment in `setup-bazel-ci` and pass the relevant variables through `run-bazel-ci.sh` via `--action_env` / `--host_action_env` so Bazel build scripts can see the MSVC and UCRT headers on native Windows runs - add inline comments to the Windows `setup-bazel-ci` MSVC environment export step so it is easier to audit how `vswhere`, `VsDevCmd.bat`, and the filtered `GITHUB_ENV` export fit together - patch `aws-lc-sys` to skip its standalone `memcmp` probe under Bazel `windows-msvc` build-script environments, which avoids a Windows-native toolchain mismatch that blocked the lint lane before it reached the aspect execution - patch `aws-lc-sys` to prefer its bundled `prebuilt-nasm` objects for Bazel `windows-msvc` build-script runs, which avoids missing `generated-src/win-x86_64/*.asm` runfiles in the exec-side helper toolchain - annotate the Linux test-only callsites in `codex-rs/linux-sandbox` and `codex-rs/core` that the wider native lint coverage surfaced ## Patches This PR introduces a large patch stack because the Windows Bazel lint lane currently depends on behavior that upstream dependencies do not provide out of the box in the mixed `windows-gnullvm` target / `windows-msvc` exec-toolchain setup. - Most of the `rules_rust` patches look like upstream candidates rather than OpenAI-only policy. Preserving explicit exec-platform constraints, forwarding the right MSVC/UCRT environment into exec-side build scripts, exposing exec-side `rustc-dev` artifacts, and keeping the Windows bootstrap/linker behavior coherent all look like fixes to the Bazel/Rust integration layer itself. - The two `aws-lc-sys` patches are more tactical. They special-case Bazel `windows-msvc` build-script environments to avoid a `memcmp` probe mismatch and missing NASM runfiles. Those may be harder to upstream as-is because they rely on Bazel-specific detection instead of a general Cargo/build-script contract. - Short term, carrying these patches in-tree is reasonable because they unblock a real CI lane and are still narrow enough to audit. Long term, the goal should not be to keep growing a permanent local fork of either dependency. - My current expectation is that the `rules_rust` patches are less controversial and should be broken out into focused upstream proposals, while the `aws-lc-sys` patches are more likely to be temporary escape hatches unless that crate wants a more general hook for hermetic build systems. Suggested follow-up plan: 1. Split the `rules_rust` deltas into upstream-sized PRs or issues with minimized repros. 2. Revisit the `aws-lc-sys` patches during the next dependency bump and see whether they can be replaced by an upstream fix, a crate upgrade, or a cleaner opt-in mechanism. 3. Treat each dependency update as a chance to delete patches one by one so the local patch set only contains still-needed deltas. ## Verification - `./.github/scripts/run-argument-comment-lint-bazel.sh --config=argument-comment-lint --keep_going` - `RUNNER_OS=Windows ./.github/scripts/run-argument-comment-lint-bazel.sh --nobuild --config=argument-comment-lint --platforms=//:local_windows --keep_going` - `cargo test -p codex-linux-sandbox` - `cargo test -p codex-core shell_snapshot_tests` - `just argument-comment-lint` ## References - #16106 --- .github/actions/setup-bazel-ci/action.yml | 66 ++++++- .../run-argument-comment-lint-bazel.sh | 115 +++++++++++ .github/scripts/run-bazel-ci.sh | 50 ++++- .github/workflows/rust-ci-full.yml | 22 +-- .github/workflows/rust-ci.yml | 22 +-- BUILD.bazel | 9 +- MODULE.bazel | 38 ++++ codex-rs/linux-sandbox/src/landlock.rs | 14 +- patches/BUILD.bazel | 7 + ...aws-lc-sys_windows_msvc_memcmp_probe.patch | 38 ++++ ...ws-lc-sys_windows_msvc_prebuilt_nasm.patch | 48 +++++ ...rust_repository_set_exec_constraints.patch | 26 +++ ...ows_bootstrap_process_wrapper_linker.patch | 23 +++ .../rules_rust_windows_exec_bin_target.patch | 71 +++++++ ...t_windows_exec_msvc_build_script_env.patch | 111 +++++++++++ ...les_rust_windows_exec_rustc_dev_rlib.patch | 11 ++ patches/rules_rust_windows_exec_std.patch | 181 ++++++++++++++++++ ...es_rust_windows_gnullvm_build_script.patch | 44 ++++- ...s_rust_windows_msvc_direct_link_args.patch | 64 +++++++ 19 files changed, 920 insertions(+), 40 deletions(-) create mode 100755 .github/scripts/run-argument-comment-lint-bazel.sh create mode 100644 patches/aws-lc-sys_windows_msvc_memcmp_probe.patch create mode 100644 patches/aws-lc-sys_windows_msvc_prebuilt_nasm.patch create mode 100644 patches/rules_rust_repository_set_exec_constraints.patch create mode 100644 patches/rules_rust_windows_bootstrap_process_wrapper_linker.patch create mode 100644 patches/rules_rust_windows_exec_bin_target.patch create mode 100644 patches/rules_rust_windows_exec_msvc_build_script_env.patch create mode 100644 patches/rules_rust_windows_exec_rustc_dev_rlib.patch create mode 100644 patches/rules_rust_windows_exec_std.patch create mode 100644 patches/rules_rust_windows_msvc_direct_link_args.patch diff --git a/.github/actions/setup-bazel-ci/action.yml b/.github/actions/setup-bazel-ci/action.yml index 34bbd40b19..a1e46d2d04 100644 --- a/.github/actions/setup-bazel-ci/action.yml +++ b/.github/actions/setup-bazel-ci/action.yml @@ -60,8 +60,72 @@ runs: # Use the shortest available drive to reduce argv/path length issues, # but avoid the drive root because some Windows test launchers mis-handle # MANIFEST paths there. - $bazelOutputUserRoot = if (Test-Path 'D:\') { 'D:\b' } else { 'C:\b' } + $hasDDrive = Test-Path 'D:\' + $bazelOutputUserRoot = if ($hasDDrive) { 'D:\b' } else { 'C:\b' } + $repoContentsCache = Join-Path $env:RUNNER_TEMP "bazel-repo-contents-cache-$env:GITHUB_RUN_ID-$env:GITHUB_JOB" "BAZEL_OUTPUT_USER_ROOT=$bazelOutputUserRoot" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append + "BAZEL_REPO_CONTENTS_CACHE=$repoContentsCache" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append + if (-not $hasDDrive) { + $repositoryCache = Join-Path $env:USERPROFILE '.cache\bazel-repo-cache' + "BAZEL_REPOSITORY_CACHE=$repositoryCache" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append + } + + - name: Expose MSVC SDK environment (Windows) + if: runner.os == 'Windows' + shell: pwsh + run: | + # Bazel exec-side Rust build scripts do not reliably inherit the MSVC developer + # shell on GitHub-hosted Windows runners, so discover the latest VS install and + # ask `VsDevCmd.bat` to materialize the x64/x64 compiler + SDK environment. + $vswhere = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe" + if (-not (Test-Path $vswhere)) { + throw "vswhere.exe not found" + } + + $installPath = & $vswhere -latest -products * -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath 2>$null + if (-not $installPath) { + throw "Could not locate a Visual Studio installation with VC tools" + } + + $vsDevCmd = Join-Path $installPath 'Common7\Tools\VsDevCmd.bat' + if (-not (Test-Path $vsDevCmd)) { + throw "VsDevCmd.bat not found at $vsDevCmd" + } + + # Keep the export surface explicit: these are the paths and SDK roots that the + # MSVC toolchain probes need later when Bazel runs Windows exec-platform build + # scripts such as `aws-lc-sys`. + $varsToExport = @( + 'INCLUDE', + 'LIB', + 'LIBPATH', + 'PATH', + 'UCRTVersion', + 'UniversalCRTSdkDir', + 'VCINSTALLDIR', + 'VCToolsInstallDir', + 'WindowsLibPath', + 'WindowsSdkBinPath', + 'WindowsSdkDir', + 'WindowsSDKLibVersion', + 'WindowsSDKVersion' + ) + + # `VsDevCmd.bat` is a batch file, so invoke it under `cmd.exe`, suppress its + # banner, then dump the resulting environment with `set`. Re-export only the + # approved keys into `GITHUB_ENV` so later steps inherit the same MSVC context. + $envLines = & cmd.exe /c ('"{0}" -no_logo -arch=x64 -host_arch=x64 >nul && set' -f $vsDevCmd) + foreach ($line in $envLines) { + if ($line -notmatch '^(.*?)=(.*)$') { + continue + } + + $name = $matches[1] + $value = $matches[2] + if ($varsToExport -contains $name) { + "$name=$value" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append + } + } - name: Enable Git long paths (Windows) if: runner.os == 'Windows' diff --git a/.github/scripts/run-argument-comment-lint-bazel.sh b/.github/scripts/run-argument-comment-lint-bazel.sh new file mode 100755 index 0000000000..e2f494d620 --- /dev/null +++ b/.github/scripts/run-argument-comment-lint-bazel.sh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash + +set -euo pipefail + +ci_config=ci-linux +case "${RUNNER_OS:-}" in + macOS) + ci_config=ci-macos + ;; + Windows) + ci_config=ci-windows + ;; +esac + +bazel_lint_args=("$@") +if [[ "${RUNNER_OS:-}" == "Windows" ]]; then + has_host_platform_override=0 + for arg in "${bazel_lint_args[@]}"; do + if [[ "$arg" == --host_platform=* ]]; then + has_host_platform_override=1 + break + fi + done + + if [[ $has_host_platform_override -eq 0 ]]; then + # The nightly Windows lint toolchain is registered with an MSVC exec + # platform even though the lint target platform stays on `windows-gnullvm`. + # Override the host platform here so the exec-side helper binaries actually + # match the registered toolchain set. + bazel_lint_args+=("--host_platform=//:local_windows_msvc") + fi + + # Native Windows lint runs need exec-side Rust helper binaries and proc-macros + # to use rust-lld instead of the C++ linker path. The default `none` + # preference resolves to `cc` when a cc_toolchain is present, which currently + # routes these exec actions through clang++ with an argument shape it cannot + # consume. + bazel_lint_args+=("--@rules_rust//rust/settings:toolchain_linker_preference=rust") + + # Some Rust top-level targets are still intentionally incompatible with the + # local Windows MSVC exec platform. Skip those explicit targets so the native + # lint aspect can run across the compatible crate graph instead of failing the + # whole build after analysis. + bazel_lint_args+=("--skip_incompatible_explicit_targets") +fi + +bazel_startup_args=() +if [[ -n "${BAZEL_OUTPUT_USER_ROOT:-}" ]]; then + bazel_startup_args+=("--output_user_root=${BAZEL_OUTPUT_USER_ROOT}") +fi + +run_bazel() { + if [[ "${RUNNER_OS:-}" == "Windows" ]]; then + MSYS2_ARG_CONV_EXCL='*' bazel "$@" + return + fi + + bazel "$@" +} + +run_bazel_with_startup_args() { + if [[ ${#bazel_startup_args[@]} -gt 0 ]]; then + run_bazel "${bazel_startup_args[@]}" "$@" + return + fi + + run_bazel "$@" +} + +read_query_labels() { + local query="$1" + local query_stdout + local query_stderr + query_stdout="$(mktemp)" + query_stderr="$(mktemp)" + + if ! run_bazel_with_startup_args \ + --noexperimental_remote_repo_contents_cache \ + query \ + --keep_going \ + --output=label \ + "$query" >"$query_stdout" 2>"$query_stderr"; then + cat "$query_stderr" >&2 + rm -f "$query_stdout" "$query_stderr" + exit 1 + fi + + cat "$query_stdout" + rm -f "$query_stdout" "$query_stderr" +} + +final_build_targets=(//codex-rs/...) +if [[ "${RUNNER_OS:-}" == "Windows" ]]; then + # Bazel's local Windows platform currently lacks a default test toolchain for + # `rust_test`, so target the concrete Rust crate rules directly. The lint + # aspect still walks their crate graph, which preserves incremental reuse for + # non-test code while avoiding non-Rust wrapper targets such as platform_data. + final_build_targets=() + while IFS= read -r label; do + [[ -n "$label" ]] || continue + final_build_targets+=("$label") + done < <(read_query_labels 'kind("rust_(library|binary|proc_macro) rule", //codex-rs/...)') + + if [[ ${#final_build_targets[@]} -eq 0 ]]; then + echo "Failed to discover Windows Bazel lint targets." >&2 + exit 1 + fi +fi + +./.github/scripts/run-bazel-ci.sh \ + -- \ + build \ + "${bazel_lint_args[@]}" \ + -- \ + "${final_build_targets[@]}" diff --git a/.github/scripts/run-bazel-ci.sh b/.github/scripts/run-bazel-ci.sh index cf50135c2e..d08b32e3d4 100755 --- a/.github/scripts/run-bazel-ci.sh +++ b/.github/scripts/run-bazel-ci.sh @@ -41,6 +41,15 @@ if [[ -n "${BAZEL_OUTPUT_USER_ROOT:-}" ]]; then bazel_startup_args+=("--output_user_root=${BAZEL_OUTPUT_USER_ROOT}") fi +run_bazel() { + if [[ "${RUNNER_OS:-}" == "Windows" ]]; then + MSYS2_ARG_CONV_EXCL='*' bazel "$@" + return + fi + + bazel "$@" +} + ci_config=ci-linux case "${RUNNER_OS:-}" in macOS) @@ -60,7 +69,7 @@ print_bazel_test_log_tails() { bazel_info_cmd+=("${bazel_startup_args[@]}") fi - testlogs_dir="$("${bazel_info_cmd[@]}" info bazel-testlogs 2>/dev/null || echo bazel-testlogs)" + testlogs_dir="$(run_bazel "${bazel_info_cmd[@]:1}" info bazel-testlogs 2>/dev/null || echo bazel-testlogs)" local failed_targets=() while IFS= read -r target; do @@ -126,6 +135,41 @@ if [[ $remote_download_toplevel -eq 1 ]]; then post_config_bazel_args+=(--remote_download_toplevel) fi +if [[ -n "${BAZEL_REPO_CONTENTS_CACHE:-}" ]]; then + # Windows self-hosted runners can run multiple Bazel jobs concurrently. Give + # each job its own repo contents cache so they do not fight over the shared + # path configured in `ci-windows`. + post_config_bazel_args+=("--repo_contents_cache=${BAZEL_REPO_CONTENTS_CACHE}") +fi + +if [[ -n "${BAZEL_REPOSITORY_CACHE:-}" ]]; then + post_config_bazel_args+=("--repository_cache=${BAZEL_REPOSITORY_CACHE}") +fi + +if [[ "${RUNNER_OS:-}" == "Windows" ]]; then + windows_action_env_vars=( + INCLUDE + LIB + LIBPATH + PATH + UCRTVersion + UniversalCRTSdkDir + VCINSTALLDIR + VCToolsInstallDir + WindowsLibPath + WindowsSdkBinPath + WindowsSdkDir + WindowsSDKLibVersion + WindowsSDKVersion + ) + + for env_var in "${windows_action_env_vars[@]}"; do + if [[ -n "${!env_var:-}" ]]; then + post_config_bazel_args+=("--action_env=${env_var}" "--host_action_env=${env_var}") + fi + done +fi + bazel_console_log="$(mktemp)" trap 'rm -f "$bazel_console_log"' EXIT @@ -149,7 +193,7 @@ if [[ -n "${BUILDBUDDY_API_KEY:-}" ]]; then bazel_run_args+=("${post_config_bazel_args[@]}") fi set +e - "${bazel_cmd[@]}" \ + run_bazel "${bazel_cmd[@]:1}" \ --noexperimental_remote_repo_contents_cache \ "${bazel_run_args[@]}" \ -- \ @@ -184,7 +228,7 @@ else bazel_run_args+=("${post_config_bazel_args[@]}") fi set +e - "${bazel_cmd[@]}" \ + run_bazel "${bazel_cmd[@]:1}" \ --noexperimental_remote_repo_contents_cache \ "${bazel_run_args[@]}" \ -- \ diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index cdedfb49e3..6d36b5f2eb 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -99,17 +99,6 @@ jobs: run: | sudo DEBIAN_FRONTEND=noninteractive apt-get update sudo DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends pkg-config libcap-dev - - name: Install nightly argument-comment-lint toolchain - if: ${{ runner.os == 'Windows' }} - shell: bash - run: | - rustup toolchain install nightly-2025-09-18 \ - --profile minimal \ - --component llvm-tools-preview \ - --component rustc-dev \ - --component rust-src \ - --no-self-update - rustup default nightly-2025-09-18 - name: Run argument comment lint on codex-rs via Bazel if: ${{ runner.os != 'Windows' }} env: @@ -125,10 +114,17 @@ jobs: --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ ${bazel_targets} - - name: Run argument comment lint on codex-rs via packaged wrapper + - name: Run argument comment lint on codex-rs via Bazel if: ${{ runner.os == 'Windows' }} + env: + BUILDBUDDY_API_KEY: ${{ secrets.BUILDBUDDY_API_KEY }} shell: bash - run: python3 ./tools/argument-comment-lint/run-prebuilt-linter.py + run: | + ./.github/scripts/run-argument-comment-lint-bazel.sh \ + --config=argument-comment-lint \ + --platforms=//:local_windows \ + --keep_going \ + --build_metadata=COMMIT_SHA=${GITHUB_SHA} # --- CI to validate on different os/targets -------------------------------- lint_build: diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index 82591d251b..3a9eadc8be 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -159,17 +159,6 @@ jobs: run: | sudo DEBIAN_FRONTEND=noninteractive apt-get update sudo DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends pkg-config libcap-dev - - name: Install nightly argument-comment-lint toolchain - if: ${{ runner.os == 'Windows' }} - shell: bash - run: | - rustup toolchain install nightly-2025-09-18 \ - --profile minimal \ - --component llvm-tools-preview \ - --component rustc-dev \ - --component rust-src \ - --no-self-update - rustup default nightly-2025-09-18 - name: Run argument comment lint on codex-rs via Bazel if: ${{ runner.os != 'Windows' }} env: @@ -185,10 +174,17 @@ jobs: --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ ${bazel_targets} - - name: Run argument comment lint on codex-rs via packaged wrapper + - name: Run argument comment lint on codex-rs via Bazel if: ${{ runner.os == 'Windows' }} + env: + BUILDBUDDY_API_KEY: ${{ secrets.BUILDBUDDY_API_KEY }} shell: bash - run: python3 ./tools/argument-comment-lint/run-prebuilt-linter.py + run: | + ./.github/scripts/run-argument-comment-lint-bazel.sh \ + --config=argument-comment-lint \ + --platforms=//:local_windows \ + --keep_going \ + --build_metadata=COMMIT_SHA=${GITHUB_SHA} # --- Gatherer job that you mark as the ONLY required status ----------------- results: diff --git a/BUILD.bazel b/BUILD.bazel index 0be4b711e6..3f59ff1160 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -17,12 +17,19 @@ platform( platform( name = "local_windows", constraint_values = [ - # We just need to pick one of the ABIs. Do the same one we target. "@rules_rs//rs/experimental/platforms/constraints:windows_gnullvm", ], parents = ["@platforms//host"], ) +platform( + name = "local_windows_msvc", + constraint_values = [ + "@rules_rs//rs/experimental/platforms/constraints:windows_msvc", + ], + parents = ["@platforms//host"], +) + alias( name = "rbe", actual = "@rbe_platform", diff --git a/MODULE.bazel b/MODULE.bazel index e71f92ca77..d1a32df5f3 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -82,6 +82,13 @@ rules_rust = use_extension("@rules_rs//rs/experimental:rules_rust.bzl", "rules_r rules_rust.patch( patches = [ "//patches:rules_rust_windows_gnullvm_build_script.patch", + "//patches:rules_rust_windows_exec_msvc_build_script_env.patch", + "//patches:rules_rust_windows_bootstrap_process_wrapper_linker.patch", + "//patches:rules_rust_windows_msvc_direct_link_args.patch", + "//patches:rules_rust_windows_exec_bin_target.patch", + "//patches:rules_rust_windows_exec_std.patch", + "//patches:rules_rust_windows_exec_rustc_dev_rlib.patch", + "//patches:rules_rust_repository_set_exec_constraints.patch", ], strip = 1, ) @@ -96,6 +103,35 @@ nightly_rust.toolchain( dev_components = True, edition = "2024", ) +# Keep Windows exec tools on MSVC so Bazel helper binaries link correctly, but +# lint crate targets as `windows-gnullvm` to preserve the repo's actual cfgs. +nightly_rust.repository_set( + name = "rust_windows_x86_64", + dev_components = True, + edition = "2024", + exec_triple = "x86_64-pc-windows-msvc", + exec_compatible_with = [ + "@platforms//cpu:x86_64", + "@platforms//os:windows", + "@rules_rs//rs/experimental/platforms/constraints:windows_msvc", + ], + target_compatible_with = [ + "@platforms//cpu:x86_64", + "@platforms//os:windows", + "@rules_rs//rs/experimental/platforms/constraints:windows_msvc", + ], + target_triple = "x86_64-pc-windows-msvc", + versions = ["nightly/2025-09-18"], +) +nightly_rust.repository_set( + name = "rust_windows_x86_64", + target_compatible_with = [ + "@platforms//cpu:x86_64", + "@platforms//os:windows", + "@rules_rs//rs/experimental/platforms/constraints:windows_gnullvm", + ], + target_triple = "x86_64-pc-windows-gnullvm", +) use_repo(nightly_rust, "rust_toolchains") toolchains = use_extension("@rules_rs//rs/experimental/toolchains:module_extension.bzl", "toolchains") @@ -163,6 +199,8 @@ crate.annotation( patch_args = ["-p1"], patches = [ "//patches:aws-lc-sys_memcmp_check.patch", + "//patches:aws-lc-sys_windows_msvc_prebuilt_nasm.patch", + "//patches:aws-lc-sys_windows_msvc_memcmp_probe.patch", ], ) diff --git a/codex-rs/linux-sandbox/src/landlock.rs b/codex-rs/linux-sandbox/src/landlock.rs index d257edf933..9a0e2c100c 100644 --- a/codex-rs/linux-sandbox/src/landlock.rs +++ b/codex-rs/linux-sandbox/src/landlock.rs @@ -276,7 +276,7 @@ mod tests { assert_eq!( should_install_network_seccomp( NetworkSandboxPolicy::Enabled, - /*allow_network_for_proxy*/ true + /*allow_network_for_proxy*/ true, ), true ); @@ -287,7 +287,7 @@ mod tests { assert_eq!( should_install_network_seccomp( NetworkSandboxPolicy::Enabled, - /*allow_network_for_proxy*/ false + /*allow_network_for_proxy*/ false, ), false ); @@ -297,11 +297,11 @@ mod tests { fn restricted_network_policy_always_installs_seccomp() { assert!(should_install_network_seccomp( NetworkSandboxPolicy::Restricted, - /*allow_network_for_proxy*/ false + /*allow_network_for_proxy*/ false, )); assert!(should_install_network_seccomp( NetworkSandboxPolicy::Restricted, - /*allow_network_for_proxy*/ true + /*allow_network_for_proxy*/ true, )); } @@ -311,7 +311,7 @@ mod tests { network_seccomp_mode( NetworkSandboxPolicy::Enabled, /*allow_network_for_proxy*/ true, - /*proxy_routed_network*/ true + /*proxy_routed_network*/ true, ), Some(NetworkSeccompMode::ProxyRouted) ); @@ -323,7 +323,7 @@ mod tests { network_seccomp_mode( NetworkSandboxPolicy::Restricted, /*allow_network_for_proxy*/ false, - /*proxy_routed_network*/ false + /*proxy_routed_network*/ false, ), Some(NetworkSeccompMode::Restricted) ); @@ -335,7 +335,7 @@ mod tests { network_seccomp_mode( NetworkSandboxPolicy::Enabled, /*allow_network_for_proxy*/ false, - /*proxy_routed_network*/ false + /*proxy_routed_network*/ false, ), None ); diff --git a/patches/BUILD.bazel b/patches/BUILD.bazel index 8d4acbbb91..b6e96df523 100644 --- a/patches/BUILD.bazel +++ b/patches/BUILD.bazel @@ -1,7 +1,14 @@ exports_files([ "abseil_windows_gnullvm_thread_identity.patch", "aws-lc-sys_memcmp_check.patch", + "aws-lc-sys_windows_msvc_prebuilt_nasm.patch", + "aws-lc-sys_windows_msvc_memcmp_probe.patch", "llvm_windows_symlink_extract.patch", + "rules_rust_windows_bootstrap_process_wrapper_linker.patch", + "rules_rust_windows_exec_bin_target.patch", + "rules_rust_windows_exec_std.patch", + "rules_rust_repository_set_exec_constraints.patch", + "rules_rust_windows_msvc_direct_link_args.patch", "rules_rust_windows_gnullvm_build_script.patch", "rules_rs_windows_gnullvm_exec.patch", "rusty_v8_prebuilt_out_dir.patch", diff --git a/patches/aws-lc-sys_windows_msvc_memcmp_probe.patch b/patches/aws-lc-sys_windows_msvc_memcmp_probe.patch new file mode 100644 index 0000000000..d244e9418f --- /dev/null +++ b/patches/aws-lc-sys_windows_msvc_memcmp_probe.patch @@ -0,0 +1,38 @@ +diff --git a/builder/cc_builder.rs b/builder/cc_builder.rs +--- a/builder/cc_builder.rs ++++ b/builder/cc_builder.rs +@@ -667,12 +667,24 @@ + if cargo_env("HOST") != target() { + return; + } ++ ++ let bazel_execroot = Self::bazel_execroot(self.manifest_dir.as_path()); ++ if bazel_execroot.is_some() && target().ends_with("windows-msvc") { ++ // This probe guards a GCC memcmp miscompile. Under Bazel's ++ // hermetic Windows/MSVC build-script toolchain we drive clang with ++ // MinGW-flavored CFLAGS, which is good enough for compiling ++ // aws-lc but not for linking and executing this standalone test ++ // binary. Skip the probe in that configuration instead of failing ++ // the whole build script. ++ emit_warning("Skipping memcmp probe for Bazel windows-msvc build scripts."); ++ return; ++ } + + let basename = "memcmp_invalid_stripped_check"; + let exec_path = out_dir().join(basename); + let memcmp_build = cc::Build::default(); + let memcmp_compiler = memcmp_build.get_compiler(); + if !memcmp_compiler.is_like_clang() && !memcmp_compiler.is_like_gnu() { + // The logic below assumes a Clang or GCC compiler is in use + return; + } +@@ -701,7 +713,7 @@ + memcmp_compile_args.push(flag.into()); + } + } + +- if let Some(execroot) = Self::bazel_execroot(self.manifest_dir.as_path()) { ++ if let Some(execroot) = bazel_execroot { + // In Bazel build-script sandboxes, `cc` can pass `bazel-out/...` args + // relative to the execroot while the process runs from elsewhere. + // Normalize those args to absolute paths so this check can still link. diff --git a/patches/aws-lc-sys_windows_msvc_prebuilt_nasm.patch b/patches/aws-lc-sys_windows_msvc_prebuilt_nasm.patch new file mode 100644 index 0000000000..37f334b8bd --- /dev/null +++ b/patches/aws-lc-sys_windows_msvc_prebuilt_nasm.patch @@ -0,0 +1,48 @@ +diff --git a/builder/main.rs b/builder/main.rs +--- a/builder/main.rs ++++ b/builder/main.rs +@@ -721,16 +721,29 @@ + fn get_crate_cflags() -> Option { + optional_env_optional_crate_target("TARGET_CFLAGS") + .or(optional_env_optional_crate_target("CFLAGS")) ++} ++ ++fn is_bazel_windows_msvc_build_script() -> bool { ++ if !target().ends_with("windows-msvc") { ++ return false; ++ } ++ ++ let cargo_manifest_dir = cargo_env("CARGO_MANIFEST_DIR"); ++ let manifest_dir = Path::new(&cargo_manifest_dir); ++ manifest_dir ++ .components() ++ .any(|component| component.as_os_str() == "bazel-out") + } + + fn use_prebuilt_nasm() -> bool { ++ let use_prebuilt_for_bazel_windows_msvc = is_bazel_windows_msvc_build_script(); + target_os() == "windows" + && target_arch() == "x86_64" + && !is_no_asm() +- && !test_nasm_command() // NASM not found in environment + && Some(false) != allow_prebuilt_nasm() // not prevented by environment + && !is_disable_prebuilt_nasm() // not prevented by feature + // permitted by environment or by feature ++ && (use_prebuilt_for_bazel_windows_msvc || !test_nasm_command()) + && (Some(true) == allow_prebuilt_nasm() || is_prebuilt_nasm()) + } + + fn allow_prebuilt_nasm() -> Option { +diff --git a/builder/nasm_builder.rs b/builder/nasm_builder.rs +--- a/builder/nasm_builder.rs ++++ b/builder/nasm_builder.rs +@@ -57,7 +57,7 @@ + if self.files.is_empty() { + return vec![]; + } + +- if test_nasm_command() { ++ if test_nasm_command() && !use_prebuilt_nasm() { + for src in &self.files { + let obj_name = src + .file_name() diff --git a/patches/rules_rust_repository_set_exec_constraints.patch b/patches/rules_rust_repository_set_exec_constraints.patch new file mode 100644 index 0000000000..31afae4f7a --- /dev/null +++ b/patches/rules_rust_repository_set_exec_constraints.patch @@ -0,0 +1,26 @@ +# What: let `rules_rust` repository_set entries specify an explicit exec-platform +# constraint set. +# Why: codex needs Windows nightly lint toolchains to run helper binaries on an +# MSVC exec platform while still targeting `windows-gnullvm` crates. + +diff --git a/rust/extensions.bzl b/rust/extensions.bzl +--- a/rust/extensions.bzl ++++ b/rust/extensions.bzl +@@ -52,6 +52,7 @@ def _rust_impl(module_ctx): + "allocator_library": repository_set.allocator_library, + "dev_components": repository_set.dev_components, + "edition": repository_set.edition, ++ "exec_compatible_with": [str(v) for v in repository_set.exec_compatible_with] if repository_set.exec_compatible_with else None, + "exec_triple": repository_set.exec_triple, + "extra_target_triples": {repository_set.target_triple: [str(v) for v in repository_set.target_compatible_with]}, + "name": repository_set.name, +@@ -166,6 +167,9 @@ _COMMON_TAG_KWARGS = { + + _RUST_REPOSITORY_SET_TAG_ATTRS = { ++ "exec_compatible_with": attr.label_list( ++ doc = "Execution platform constraints for this repository_set.", ++ ), + "exec_triple": attr.string( + doc = "Exec triple for this repository_set.", + ), + "name": attr.string( diff --git a/patches/rules_rust_windows_bootstrap_process_wrapper_linker.patch b/patches/rules_rust_windows_bootstrap_process_wrapper_linker.patch new file mode 100644 index 0000000000..9a978b8be6 --- /dev/null +++ b/patches/rules_rust_windows_bootstrap_process_wrapper_linker.patch @@ -0,0 +1,23 @@ +--- a/rust/private/rustc.bzl ++++ b/rust/private/rustc.bzl +@@ -472,7 +472,19 @@ + ) + ld_is_direct_driver = False + +- if not ld or toolchain.linker_preference == "rust": ++ # The bootstrap process wrapper is built without the normal rules_rust ++ # process wrapper. On Windows nightly toolchains that expose rust-lld, the ++ # C++ toolchain path currently resolves to clang++ while still emitting ++ # MSVC-style arguments, so prefer rust-lld for this one bootstrap binary ++ # instead of switching all Rust actions over. ++ use_bootstrap_rust_linker = ( ++ toolchain.target_os.startswith("windows") and ++ toolchain.linker != None and ++ hasattr(ctx.executable, "_bootstrap_process_wrapper") and ++ not ctx.executable._process_wrapper ++ ) ++ ++ if not ld or toolchain.linker_preference == "rust" or use_bootstrap_rust_linker: + ld = toolchain.linker.path + ld_is_direct_driver = toolchain.linker_type == "direct" + diff --git a/patches/rules_rust_windows_exec_bin_target.patch b/patches/rules_rust_windows_exec_bin_target.patch new file mode 100644 index 0000000000..e4cf306dff --- /dev/null +++ b/patches/rules_rust_windows_exec_bin_target.patch @@ -0,0 +1,71 @@ +# What: compile exec-side Rust binaries against the exec Windows triple instead +# of the lint target triple. +# Why: Windows native argument-comment-lint keeps the repo target platform on +# `windows-gnullvm` to preserve cfg coverage, but exec-side helper binaries +# (build.rs, runners, bootstrap tools) must link as host tools. With +# `toolchain_linker_preference=rust`, rules_rust was still feeding those exec +# binaries the `windows-gnullvm` target/std path, which broke linking under the +# native Bazel lint lane. + +diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl +--- a/rust/private/rustc.bzl ++++ b/rust/private/rustc.bzl +@@ -129,6 +129,20 @@ + build_setting = config.bool(flag = True), + ) + +-def _get_rustc_env(attr, toolchain, crate_name): ++def _effective_target_arch(toolchain, use_exec_target): ++ return toolchain.exec_triple.arch if use_exec_target else toolchain.target_arch ++ ++def _effective_target_os(toolchain, use_exec_target): ++ return toolchain.exec_triple.system if use_exec_target else toolchain.target_os ++ ++def _effective_target_flag_value(toolchain, use_exec_target): ++ return toolchain.exec_triple.str if use_exec_target else toolchain.target_flag_value ++ ++def _effective_rust_std_paths(toolchain, use_exec_target): ++ if use_exec_target: ++ return ["{}/lib/rustlib/{}/lib".format(toolchain.sysroot, toolchain.exec_triple.str)] ++ return toolchain.rust_std_paths ++ ++def _get_rustc_env(attr, toolchain, crate_name, use_exec_target = False): + """Gathers rustc environment variables + +@@ -147,6 +161,6 @@ + + result = { +- "CARGO_CFG_TARGET_ARCH": "" if toolchain.target_arch == None else toolchain.target_arch, +- "CARGO_CFG_TARGET_OS": "" if toolchain.target_os == None else toolchain.target_os, ++ "CARGO_CFG_TARGET_ARCH": "" if _effective_target_arch(toolchain, use_exec_target) == None else _effective_target_arch(toolchain, use_exec_target), ++ "CARGO_CFG_TARGET_OS": "" if _effective_target_os(toolchain, use_exec_target) == None else _effective_target_os(toolchain, use_exec_target), + "CARGO_CRATE_NAME": crate_name, + "CARGO_PKG_AUTHORS": "", +@@ -997,9 +1011,11 @@ + if build_metadata and not use_json_output: + fail("build_metadata requires parse_json_output") + ++ use_exec_target = is_exec_configuration(ctx) and crate_info.type == "bin" ++ + output_dir = getattr(crate_info.output, "dirname", None) + linker_script = getattr(file, "linker_script", None) + +- env = _get_rustc_env(attr, toolchain, crate_info.name) ++ env = _get_rustc_env(attr, toolchain, crate_info.name, use_exec_target) + + # Wrapper args first +@@ -1138,5 +1154,5 @@ + if error_format != "json": + # Color is not compatible with json output. + rustc_flags.add("--color=always") +- rustc_flags.add(toolchain.target_flag_value, format = "--target=%s") ++ rustc_flags.add(_effective_target_flag_value(toolchain, use_exec_target), format = "--target=%s") + if hasattr(attr, "crate_features"): +@@ -1144,6 +1160,6 @@ + if linker_script: + rustc_flags.add(linker_script, format = "--codegen=link-arg=-T%s") + + # Tell Rustc where to find the standard library (or libcore) +- rustc_flags.add_all(toolchain.rust_std_paths, before_each = "-L", format_each = "%s") ++ rustc_flags.add_all(_effective_rust_std_paths(toolchain, use_exec_target), before_each = "-L", format_each = "%s") + rustc_flags.add_all(rust_flags, map_each = map_flag) diff --git a/patches/rules_rust_windows_exec_msvc_build_script_env.patch b/patches/rules_rust_windows_exec_msvc_build_script_env.patch new file mode 100644 index 0000000000..c78dba43cc --- /dev/null +++ b/patches/rules_rust_windows_exec_msvc_build_script_env.patch @@ -0,0 +1,111 @@ +diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl +--- a/cargo/private/cargo_build_script.bzl ++++ b/cargo/private/cargo_build_script.bzl +@@ -142,40 +142,82 @@ def _strip_stack_protector_for_windows_llvm_mingw(toolchain, args): + def _rewrite_windows_exec_msvc_cc_args(toolchain, args): + """Translate GNU-flavored cc args when exec-side build scripts target Windows MSVC.""" + if toolchain.target_flag_value != toolchain.exec_triple.str or not toolchain.exec_triple.str.endswith("-pc-windows-msvc"): + return args + +- rewritten = [] +- skip_next = False +- for arg in args: +- if skip_next: +- skip_next = False +- continue ++ rewritten = [ ++ "-target", ++ toolchain.target_flag_value, ++ ] ++ skip_next = False ++ for index in range(len(args)): ++ arg = args[index] ++ ++ if skip_next: ++ skip_next = False ++ continue + + if arg == "-target": +- skip_next = True ++ skip_next = True + continue + + if arg.startswith("-target=") or arg.startswith("--target="): + continue + + if arg == "-nostdlibinc" or arg.startswith("--sysroot"): + continue + +- if "mingw-w64-" in arg or "mingw_import_libraries_directory" in arg or "mingw_crt_library_search_directory" in arg: ++ if arg.startswith("-fstack-protector") or arg.startswith("-D_FORTIFY_SOURCE="): + continue + +- if arg.startswith("-fstack-protector"): +- continue +- +- if arg.startswith("-D_FORTIFY_SOURCE="): +- continue ++ if arg == "-isystem" and index + 1 < len(args): ++ path = args[index + 1] ++ if "mingw-w64-" in path or "mingw_import_libraries_directory" in path or "mingw_crt_library_search_directory" in path: ++ skip_next = True ++ continue + + rewritten.append(arg) + +- return [ +- "-target", +- toolchain.target_flag_value, +- ] + rewritten ++ return rewritten ++ ++def _rewrite_windows_exec_msvc_link_args(toolchain, args): ++ """Translate GNU-flavored link args when exec-side build scripts target Windows MSVC.""" ++ if toolchain.target_flag_value != toolchain.exec_triple.str or not toolchain.exec_triple.str.endswith("-pc-windows-msvc"): ++ return args ++ ++ rewritten = [] ++ skip_next = False ++ for index in range(len(args)): ++ arg = args[index] ++ ++ if skip_next: ++ skip_next = False ++ continue ++ ++ if arg == "--sysroot": ++ skip_next = True ++ continue ++ ++ if arg.startswith("--sysroot="): ++ continue ++ ++ if arg == "-L" and index + 1 < len(args): ++ path = args[index + 1] ++ if "mingw_import_libraries_directory" in path or "mingw_crt_library_search_directory" in path: ++ skip_next = True ++ continue ++ rewritten.extend([arg, path]) ++ skip_next = True ++ continue ++ ++ if arg.startswith("-L") and ( ++ "mingw_import_libraries_directory" in arg or ++ "mingw_crt_library_search_directory" in arg ++ ): ++ continue ++ ++ rewritten.append(arg) ++ ++ return rewritten + + def get_cc_compile_args_and_env(cc_toolchain, feature_configuration): + """Gather cc environment variables from the given `cc_toolchain` +@@ -509,6 +550,7 @@ def _construct_build_script_env( + linker, _, link_args, linker_env = get_linker_and_args(ctx, "bin", toolchain, cc_toolchain, feature_configuration, None) + env.update(**linker_env) + env["LD"] = linker ++ link_args = _rewrite_windows_exec_msvc_link_args(toolchain, link_args) + env["LDFLAGS"] = " ".join(_pwd_flags(link_args)) + + # Defaults for cxx flags. diff --git a/patches/rules_rust_windows_exec_rustc_dev_rlib.patch b/patches/rules_rust_windows_exec_rustc_dev_rlib.patch new file mode 100644 index 0000000000..44c54f4b81 --- /dev/null +++ b/patches/rules_rust_windows_exec_rustc_dev_rlib.patch @@ -0,0 +1,11 @@ +diff --git a/rust/private/repository_utils.bzl b/rust/private/repository_utils.bzl +--- a/rust/private/repository_utils.bzl ++++ b/rust/private/repository_utils.bzl +@@ -53,6 +53,7 @@ filegroup( + "lib/*{dylib_ext}*", + "lib/rustlib/{target_triple}/codegen-backends/*{dylib_ext}", + "lib/rustlib/{target_triple}/lib/*{dylib_ext}*", ++ "lib/rustlib/{target_triple}/lib/*.rlib", + "lib/rustlib/{target_triple}/lib/*.rmeta", + ], + allow_empty = True, diff --git a/patches/rules_rust_windows_exec_std.patch b/patches/rules_rust_windows_exec_std.patch new file mode 100644 index 0000000000..aa20ee0e51 --- /dev/null +++ b/patches/rules_rust_windows_exec_std.patch @@ -0,0 +1,181 @@ +# What: expose an exec-side Rust standard library alongside the target stdlib. +# Why: mixed Windows toolchains compile repo crates for `windows-gnullvm`, but +# exec-side helper binaries (build.rs, runners) may need the host MSVC stdlib. +# The toolchain sysroot must therefore carry both stdlib trees so rustc can +# resolve the correct one for each `--target`. + +diff --git a/rust/toolchain.bzl b/rust/toolchain.bzl +--- a/rust/toolchain.bzl ++++ b/rust/toolchain.bzl +@@ -209,6 +209,7 @@ def _generate_sysroot( + clippy = None, + cargo_clippy = None, + llvm_tools = None, ++ exec_rust_std = None, + rust_std = None, + rustfmt = None, + linker = None): +@@ -312,7 +313,15 @@ def _generate_sysroot( + + # Made available to support $(location) expansion in stdlib_linkflags and extra_rustc_flags. + transitive_file_sets.append(depset(ctx.files.rust_std)) ++ ++ sysroot_exec_rust_std = None ++ if exec_rust_std: ++ sysroot_exec_rust_std = _symlink_sysroot_tree(ctx, name, exec_rust_std) ++ transitive_file_sets.extend([sysroot_exec_rust_std]) + ++ # Made available to support $(location) expansion in extra_exec_rustc_flags. ++ transitive_file_sets.append(depset(ctx.files.exec_rust_std)) ++ + # Declare a file in the root of the sysroot to make locating the sysroot easy + sysroot_anchor = ctx.actions.declare_file("{}/rust.sysroot".format(name)) + ctx.actions.write( +@@ -323,6 +332,7 @@ def _generate_sysroot( + "cargo-clippy: {}".format(cargo_clippy), + "linker: {}".format(linker), + "llvm_tools: {}".format(llvm_tools), ++ "exec_rust_std: {}".format(exec_rust_std), + "rust_std: {}".format(rust_std), + "rustc_lib: {}".format(rustc_lib), + "rustc: {}".format(rustc), +@@ -340,6 +350,7 @@ def _generate_sysroot( + cargo_clippy = sysroot_cargo_clippy, + clippy = sysroot_clippy, + linker = sysroot_linker, ++ exec_rust_std = sysroot_exec_rust_std, + rust_std = sysroot_rust_std, + rustc = sysroot_rustc, + rustc_lib = sysroot_rustc_lib, +@@ -410,12 +421,14 @@ def _rust_toolchain_impl(ctx): + ) + + rust_std = ctx.attr.rust_std ++ exec_rust_std = ctx.attr.exec_rust_std if ctx.attr.exec_rust_std else rust_std + + sysroot = _generate_sysroot( + ctx = ctx, + rustc = ctx.file.rustc, + rustdoc = ctx.file.rust_doc, + rustc_lib = ctx.attr.rustc_lib, ++ exec_rust_std = exec_rust_std, + rust_std = rust_std, + rustfmt = ctx.file.rustfmt, + clippy = ctx.file.clippy_driver, +@@ -452,7 +465,7 @@ def _rust_toolchain_impl(ctx): + + expanded_stdlib_linkflags = _expand_flags(ctx, "stdlib_linkflags", rust_std[rust_common.stdlib_info].srcs, make_variables) + expanded_extra_rustc_flags = _expand_flags(ctx, "extra_rustc_flags", rust_std[rust_common.stdlib_info].srcs, make_variables) +- expanded_extra_exec_rustc_flags = _expand_flags(ctx, "extra_exec_rustc_flags", rust_std[rust_common.stdlib_info].srcs, make_variables) ++ expanded_extra_exec_rustc_flags = _expand_flags(ctx, "extra_exec_rustc_flags", exec_rust_std[rust_common.stdlib_info].srcs, make_variables) + + linking_context = cc_common.create_linking_context( + linker_inputs = depset([ +@@ -793,6 +806,10 @@ rust_toolchain = rule( + doc = "The Rust standard library.", + mandatory = True, + ), ++ "exec_rust_std": attr.label( ++ doc = "Optional Rust standard library for exec-configuration Rust tools. Defaults to rust_std.", ++ mandatory = False, ++ ), + "rustc": attr.label( + doc = "The location of the `rustc` binary. Can be a direct source or a filegroup containing one item.", + allow_single_file = True, +diff --git a/rust/private/repository_utils.bzl b/rust/private/repository_utils.bzl +--- a/rust/private/repository_utils.bzl ++++ b/rust/private/repository_utils.bzl +@@ -341,6 +341,7 @@ rust_toolchain( + name = "{toolchain_name}", + rust_doc = "//:rustdoc", + rust_std = "//:rust_std-{target_triple}", ++ exec_rust_std = {exec_rust_std_label}, + rustc = "//:rustc", + linker = {linker_label}, + linker_type = {linker_type}, +@@ -384,6 +385,7 @@ def BUILD_for_rust_toolchain( + include_llvm_tools, + include_linker, + include_objcopy = False, ++ exec_rust_std_label = None, + stdlib_linkflags = None, + extra_rustc_flags = None, + extra_exec_rustc_flags = None, +@@ -405,6 +407,7 @@ def BUILD_for_rust_toolchain( + include_llvm_tools (bool): Whether llvm-tools are present in the toolchain. + include_linker (bool): Whether a linker is available in the toolchain. + include_objcopy (bool): Whether rust-objcopy is available in the toolchain. ++ exec_rust_std_label (str, optional): Label for an exec-side stdlib when it differs from rust_std. + stdlib_linkflags (list, optional): Overridden flags needed for linking to rust + stdlib, akin to BAZEL_LINKLIBS. Defaults to + None. +@@ -453,6 +456,7 @@ def BUILD_for_rust_toolchain( + staticlib_ext = system_to_staticlib_ext(target_triple.system), + dylib_ext = system_to_dylib_ext(target_triple.system), + allocator_library = repr(allocator_library_label), ++ exec_rust_std_label = repr(exec_rust_std_label), + global_allocator_library = repr(global_allocator_library_label), + stdlib_linkflags = stdlib_linkflags, + default_edition = default_edition, +diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl +--- a/rust/private/rustc.bzl ++++ b/rust/private/rustc.bzl +@@ -1011,7 +1011,10 @@ def construct_arguments( + if build_metadata and not use_json_output: + fail("build_metadata requires parse_json_output") + +- use_exec_target = is_exec_configuration(ctx) and crate_info.type == "bin" ++ # Exec-configuration crates (build scripts, proc-macros, and their ++ # dependencies) must all target the exec triple so they can link against ++ # each other and the exec-side standard library. ++ use_exec_target = is_exec_configuration(ctx) + + output_dir = getattr(crate_info.output, "dirname", None) + linker_script = getattr(file, "linker_script", None) +diff --git a/rust/repositories.bzl b/rust/repositories.bzl +--- a/rust/repositories.bzl ++++ b/rust/repositories.bzl +@@ -536,6 +536,18 @@ def _rust_toolchain_tools_repository_impl(ctx): + build_components.append(rust_stdlib_content) + sha256s.update(rust_stdlib_sha256) + ++ exec_rust_std_label = None ++ if exec_triple.str != target_triple.str: ++ exec_rust_stdlib_content, exec_rust_stdlib_sha256 = load_rust_stdlib( ++ ctx = ctx, ++ target_triple = exec_triple, ++ version = version, ++ iso_date = iso_date, ++ ) ++ build_components.append(exec_rust_stdlib_content) ++ sha256s.update(exec_rust_stdlib_sha256) ++ exec_rust_std_label = "//:rust_std-{}".format(exec_triple.str) ++ + stdlib_linkflags = None + if "BAZEL_RUST_STDLIB_LINKFLAGS" in ctx.os.environ: + stdlib_linkflags = ctx.os.environ["BAZEL_RUST_STDLIB_LINKFLAGS"].split(":") +@@ -552,6 +564,7 @@ def _rust_toolchain_tools_repository_impl(ctx): + include_llvm_tools = include_llvm_tools, + include_linker = include_linker, + include_objcopy = include_objcopy, ++ exec_rust_std_label = exec_rust_std_label, + extra_rustc_flags = ctx.attr.extra_rustc_flags, + extra_exec_rustc_flags = ctx.attr.extra_exec_rustc_flags, + opt_level = ctx.attr.opt_level if ctx.attr.opt_level else None, +@@ -575,8 +588,16 @@ def _rust_toolchain_tools_repository_impl(ctx): + if ctx.attr.dev_components: + rustc_dev_sha256 = load_rustc_dev_nightly( + ctx = ctx, + target_triple = target_triple, + version = version, + iso_date = iso_date, + ) + sha256s.update(rustc_dev_sha256) ++ if exec_triple.str != target_triple.str: ++ exec_rustc_dev_sha256 = load_rustc_dev_nightly( ++ ctx = ctx, ++ target_triple = exec_triple, ++ version = version, ++ iso_date = iso_date, ++ ) ++ sha256s.update(exec_rustc_dev_sha256) diff --git a/patches/rules_rust_windows_gnullvm_build_script.patch b/patches/rules_rust_windows_gnullvm_build_script.patch index ca7ac57402..a1ed9bf145 100644 --- a/patches/rules_rust_windows_gnullvm_build_script.patch +++ b/patches/rules_rust_windows_gnullvm_build_script.patch @@ -1,7 +1,7 @@ diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl --- a/cargo/private/cargo_build_script.bzl +++ b/cargo/private/cargo_build_script.bzl -@@ -120,6 +120,25 @@ +@@ -120,6 +120,63 @@ executable = True, ) @@ -23,16 +23,56 @@ diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_sc + # flags through CFLAGS/CXXFLAGS breaks build.rs probe binaries compiled via + # cc-rs. + return [arg for arg in args if not arg.startswith("-fstack-protector")] ++ ++def _rewrite_windows_exec_msvc_cc_args(toolchain, args): ++ """Translate GNU-flavored cc args when exec-side build scripts target Windows MSVC.""" ++ if toolchain.target_flag_value != toolchain.exec_triple.str or not toolchain.exec_triple.str.endswith("-pc-windows-msvc"): ++ return args ++ ++ rewritten = [] ++ skip_next = False ++ for arg in args: ++ if skip_next: ++ skip_next = False ++ continue ++ ++ if arg == "-target": ++ skip_next = True ++ continue ++ ++ if arg.startswith("-target=") or arg.startswith("--target="): ++ continue ++ ++ if arg == "-nostdlibinc" or arg.startswith("--sysroot"): ++ continue ++ ++ if "mingw-w64-" in arg or "mingw_import_libraries_directory" in arg or "mingw_crt_library_search_directory" in arg: ++ continue ++ ++ if arg.startswith("-fstack-protector"): ++ continue ++ ++ if arg.startswith("-D_FORTIFY_SOURCE="): ++ continue ++ ++ rewritten.append(arg) ++ ++ return [ ++ "-target", ++ toolchain.target_flag_value, ++ ] + rewritten + def get_cc_compile_args_and_env(cc_toolchain, feature_configuration): """Gather cc environment variables from the given `cc_toolchain` -@@ -503,6 +522,8 @@ +@@ -503,6 +560,10 @@ if not env["AR"]: env["AR"] = cc_toolchain.ar_executable + cc_c_args = _strip_stack_protector_for_windows_llvm_mingw(toolchain, cc_c_args) + cc_cxx_args = _strip_stack_protector_for_windows_llvm_mingw(toolchain, cc_cxx_args) ++ cc_c_args = _rewrite_windows_exec_msvc_cc_args(toolchain, cc_c_args) ++ cc_cxx_args = _rewrite_windows_exec_msvc_cc_args(toolchain, cc_cxx_args) # Populate CFLAGS and CXXFLAGS that cc-rs relies on when building from source, in particular # to determine the deployment target when building for apple platforms (`macosx-version-min` # for example, itself derived from the `macos_minimum_os` Bazel argument). diff --git a/patches/rules_rust_windows_msvc_direct_link_args.patch b/patches/rules_rust_windows_msvc_direct_link_args.patch new file mode 100644 index 0000000000..2f04f8ca89 --- /dev/null +++ b/patches/rules_rust_windows_msvc_direct_link_args.patch @@ -0,0 +1,64 @@ +--- a/rust/private/rustc.bzl ++++ b/rust/private/rustc.bzl +@@ -2305,7 +2305,7 @@ + return crate.metadata.dirname + return crate.output.dirname + +-def _portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name, for_windows = False, for_darwin = False, flavor_msvc = False): ++def _portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name, for_windows = False, for_darwin = False, flavor_msvc = False, use_direct_driver = False): + artifact = get_preferred_artifact(lib, use_pic) + if ambiguous_libs and artifact.path in ambiguous_libs: + artifact = ambiguous_libs[artifact.path] +@@ -2344,6 +2344,11 @@ + artifact.basename.startswith("test-") or artifact.basename.startswith("std-") + ): + return [] if for_darwin else ["-lstatic=%s" % get_lib_name(artifact)] ++ ++ if for_windows and use_direct_driver and not artifact.basename.endswith(".lib"): ++ return [ ++ "-Clink-arg={}".format(artifact.path), ++ ] + + if flavor_msvc: + return [ +@@ -2381,7 +2386,7 @@ + ]) + elif include_link_flags: + get_lib_name = get_lib_name_for_windows if flavor_msvc else get_lib_name_default +- ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name, flavor_msvc = flavor_msvc)) ++ ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name, flavor_msvc = flavor_msvc, use_direct_driver = use_direct_driver)) + + # Windows toolchains can inherit POSIX defaults like -pthread from C deps, + # which fails to link with the MinGW/LLD toolchain. Drop them here. +@@ -2558,17 +2563,25 @@ + else: + # For all other crate types we want to link C++ runtime library statically + # (for example libstdc++.a or libc++.a). ++ runtime_libs = cc_toolchain.static_runtime_lib(feature_configuration = feature_configuration) + args.add_all( +- cc_toolchain.static_runtime_lib(feature_configuration = feature_configuration), ++ runtime_libs, + map_each = _get_dirname, + format_each = "-Lnative=%s", + ) + if include_link_flags: +- args.add_all( +- cc_toolchain.static_runtime_lib(feature_configuration = feature_configuration), +- map_each = get_lib_name, +- format_each = "-lstatic=%s", +- ) ++ if toolchain.target_os == "windows" and use_direct_link_driver: ++ for runtime_lib in runtime_libs.to_list(): ++ if runtime_lib.basename.endswith(".lib"): ++ args.add(get_lib_name(runtime_lib), format = "-lstatic=%s") ++ else: ++ args.add(runtime_lib.path, format = "--codegen=link-arg=%s") ++ else: ++ args.add_all( ++ runtime_libs, ++ map_each = get_lib_name, ++ format_each = "-lstatic=%s", ++ ) + + def _get_dirname(file): + """A helper function for `_add_native_link_flags`.