Merge 9c7f19ce5c into sapling-pr-archive-bolinfest

This commit is contained in:
Michael Bolin
2026-04-29 15:35:32 -07:00
committed by GitHub
13 changed files with 282 additions and 16 deletions
+2 -1
View File
@@ -49,7 +49,8 @@ jobs:
needs: tag-check
name: Build - ${{ matrix.runner }} - ${{ matrix.target }} - ${{ matrix.bundle }}
runs-on: ${{ matrix.runs_on || matrix.runner }}
timeout-minutes: 60
# Codex binaries can take a long time to build, particularly on Windows.
timeout-minutes: 90
permissions:
contents: read
id-token: write
+6
View File
@@ -19,6 +19,12 @@ In the codex-rs folder where the rust code lives:
- You can run `just argument-comment-lint` to run the lint check locally. This is powered by Bazel, so running it the first time can be slow if Bazel is not warmed up, though incremental invocations should take <15s. Most of the time, it is best to update the PR and let CI take responsibility for checking this (or run it asynchronously in the background after submitting the PR). Note CI checks all three platforms, which the local run does not.
- When possible, make `match` statements exhaustive and avoid wildcard arms.
- Newly added traits should include doc comments that explain their role and how implementations are expected to use them.
- Discourage both `#[async_trait]` and `#[allow(async_fn_in_trait)]` in Rust traits.
- Prefer native RPITIT trait methods with explicit `Send` bounds on the returned future, as in `3c7f013f9735` / `#16630`.
- Preferred trait shape:
`fn foo(&self, ...) -> impl std::future::Future<Output = T> + Send;`
- Implementations may still use `async fn foo(&self, ...) -> T` when they satisfy that contract.
- Do not use `#[allow(async_fn_in_trait)]` as a shortcut around spelling the future contract explicitly.
- When writing tests, prefer comparing the equality of entire objects over fields one by one.
- When making a change that adds or changes an API, ensure that the documentation in the `docs/` folder is up to date if applicable.
- Prefer private modules and explicitly exported public crate API.
+16 -7
View File
@@ -93,6 +93,8 @@ pub fn mcp_permission_prompt_is_auto_approved(
pub struct McpConfig {
/// Base URL for ChatGPT-hosted app MCP servers, copied from the root config.
pub chatgpt_base_url: String,
/// Optional path override for the built-in apps MCP server.
pub apps_mcp_path_override: Option<String>,
/// Codex home directory used for MCP OAuth state and app-tool cache files.
pub codex_home: PathBuf,
/// Preferred credential store for MCP OAuth tokens.
@@ -333,7 +335,10 @@ pub async fn collect_mcp_snapshot_from_manager(
}
pub(crate) fn codex_apps_mcp_url(config: &McpConfig) -> String {
codex_apps_mcp_url_for_base_url(&config.chatgpt_base_url)
codex_apps_mcp_url_for_base_url(
&config.chatgpt_base_url,
config.apps_mcp_path_override.as_deref(),
)
}
/// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`.
@@ -376,15 +381,19 @@ fn normalize_codex_apps_base_url(base_url: &str) -> String {
base_url
}
fn codex_apps_mcp_url_for_base_url(base_url: &str) -> String {
fn codex_apps_mcp_url_for_base_url(base_url: &str, apps_mcp_path_override: Option<&str>) -> String {
let base_url = normalize_codex_apps_base_url(base_url);
if base_url.contains("/backend-api") {
format!("{base_url}/wham/apps")
let (base_url, default_path) = if base_url.contains("/backend-api") {
(base_url, "wham/apps")
} else if base_url.contains("/api/codex") {
format!("{base_url}/apps")
(base_url, "apps")
} else {
format!("{base_url}/api/codex/apps")
}
(format!("{base_url}/api/codex"), "apps")
};
let path = apps_mcp_path_override
.unwrap_or(default_path)
.trim_start_matches('/');
format!("{base_url}/{path}")
}
fn codex_apps_mcp_server_config(config: &McpConfig) -> McpServerConfig {
+36 -4
View File
@@ -14,6 +14,7 @@ use std::path::PathBuf;
fn test_mcp_config(codex_home: PathBuf) -> McpConfig {
McpConfig {
chatgpt_base_url: "https://chatgpt.com".to_string(),
apps_mcp_path_override: None,
codex_home,
mcp_oauth_credentials_store_mode: OAuthCredentialsStoreMode::default(),
mcp_oauth_callback_port: None,
@@ -109,19 +110,31 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() {
#[test]
fn codex_apps_mcp_url_for_base_url_keeps_existing_paths() {
assert_eq!(
codex_apps_mcp_url_for_base_url("https://chatgpt.com/backend-api"),
codex_apps_mcp_url_for_base_url(
"https://chatgpt.com/backend-api",
/*apps_mcp_path_override*/ None,
),
"https://chatgpt.com/backend-api/wham/apps"
);
assert_eq!(
codex_apps_mcp_url_for_base_url("https://chat.openai.com"),
codex_apps_mcp_url_for_base_url(
"https://chat.openai.com",
/*apps_mcp_path_override*/ None,
),
"https://chat.openai.com/backend-api/wham/apps"
);
assert_eq!(
codex_apps_mcp_url_for_base_url("http://localhost:8080/api/codex"),
codex_apps_mcp_url_for_base_url(
"http://localhost:8080/api/codex",
/*apps_mcp_path_override*/ None,
),
"http://localhost:8080/api/codex/apps"
);
assert_eq!(
codex_apps_mcp_url_for_base_url("http://localhost:8080"),
codex_apps_mcp_url_for_base_url(
"http://localhost:8080",
/*apps_mcp_path_override*/ None,
),
"http://localhost:8080/api/codex/apps"
);
}
@@ -158,6 +171,25 @@ fn codex_apps_server_config_uses_legacy_codex_apps_path() {
assert_eq!(url, "https://chatgpt.com/backend-api/wham/apps");
}
#[test]
fn codex_apps_server_config_uses_configured_apps_mcp_path_override() {
let mut config = test_mcp_config(PathBuf::from("/tmp"));
config.apps_mcp_path_override = Some("/custom/mcp".to_string());
config.apps_enabled = true;
let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing();
let servers = with_codex_apps_mcp(HashMap::new(), Some(&auth), &config);
let server = servers
.get(CODEX_APPS_MCP_SERVER_NAME)
.expect("codex apps should be present when apps is enabled");
let url = match &server.transport {
McpServerTransportConfig::StreamableHttp { url, .. } => url,
_ => panic!("expected streamable http transport for codex apps"),
};
assert_eq!(url, "https://chatgpt.com/backend-api/custom/mcp");
}
#[tokio::test]
async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() {
let codex_home = tempfile::tempdir().expect("tempdir");
+9
View File
@@ -34,6 +34,15 @@ pub fn features_schema(schema_gen: &mut SchemaGenerator) -> Schema {
);
continue;
}
if feature.id == codex_features::Feature::AppsMcpPathOverride {
validation.properties.insert(
feature.key.to_string(),
schema_gen.subschema_for::<codex_features::FeatureToml<
codex_features::AppsMcpPathOverrideConfigToml,
>>(),
);
continue;
}
validation
.properties
.insert(feature.key.to_string(), schema_gen.subschema_for::<bool>());
+28
View File
@@ -218,6 +218,18 @@
},
"type": "object"
},
"AppsMcpPathOverrideConfigToml": {
"additionalProperties": false,
"properties": {
"enabled": {
"type": "boolean"
},
"path": {
"type": "string"
}
},
"type": "object"
},
"AskForApproval": {
"description": "Determines the conditions under which the user is consulted to approve running the command proposed by Codex.",
"oneOf": [
@@ -355,6 +367,9 @@
"apps": {
"type": "boolean"
},
"apps_mcp_path_override": {
"$ref": "#/definitions/FeatureToml_for_AppsMcpPathOverrideConfigToml"
},
"browser_use": {
"type": "boolean"
},
@@ -705,6 +720,16 @@
},
"type": "object"
},
"FeatureToml_for_AppsMcpPathOverrideConfigToml": {
"anyOf": [
{
"type": "boolean"
},
{
"$ref": "#/definitions/AppsMcpPathOverrideConfigToml"
}
]
},
"FeatureToml_for_MultiAgentV2ConfigToml": {
"anyOf": [
{
@@ -3297,6 +3322,9 @@
"apps": {
"type": "boolean"
},
"apps_mcp_path_override": {
"$ref": "#/definitions/FeatureToml_for_AppsMcpPathOverrideConfigToml"
},
"browser_use": {
"type": "boolean"
},
+35
View File
@@ -3366,8 +3366,13 @@ async fn to_mcp_config_preserves_apps_feature_from_config() -> std::io::Result<(
.await?;
let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf());
config.apps_mcp_path_override = Some("/custom/mcp".to_string());
let mcp_config = config.to_mcp_config(&plugins_manager).await;
assert!(mcp_config.apps_enabled);
assert_eq!(
mcp_config.apps_mcp_path_override.as_deref(),
Some("/custom/mcp")
);
let _ = config.features.disable(Feature::Apps);
let mcp_config = config.to_mcp_config(&plugins_manager).await;
@@ -6071,6 +6076,7 @@ async fn test_precedence_fixture_with_o3_profile() -> std::io::Result<()> {
model_verbosity: None,
personality: Some(Personality::Pragmatic),
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
apps_mcp_path_override: None,
realtime_audio: RealtimeAudioConfig::default(),
experimental_realtime_start_instructions: None,
experimental_realtime_ws_base_url: None,
@@ -6266,6 +6272,7 @@ async fn test_precedence_fixture_with_gpt3_profile() -> std::io::Result<()> {
model_verbosity: None,
personality: Some(Personality::Pragmatic),
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
apps_mcp_path_override: None,
realtime_audio: RealtimeAudioConfig::default(),
experimental_realtime_start_instructions: None,
experimental_realtime_ws_base_url: None,
@@ -6415,6 +6422,7 @@ async fn test_precedence_fixture_with_zdr_profile() -> std::io::Result<()> {
model_verbosity: None,
personality: Some(Personality::Pragmatic),
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
apps_mcp_path_override: None,
realtime_audio: RealtimeAudioConfig::default(),
experimental_realtime_start_instructions: None,
experimental_realtime_ws_base_url: None,
@@ -6549,6 +6557,7 @@ async fn test_precedence_fixture_with_gpt5_profile() -> std::io::Result<()> {
model_verbosity: Some(Verbosity::High),
personality: Some(Personality::Pragmatic),
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
apps_mcp_path_override: None,
realtime_audio: RealtimeAudioConfig::default(),
experimental_realtime_start_instructions: None,
experimental_realtime_ws_base_url: None,
@@ -7187,6 +7196,32 @@ allow_login_shell = false
Ok(())
}
#[tokio::test]
async fn config_loads_apps_mcp_path_override_from_feature_config() -> std::io::Result<()> {
let codex_home = TempDir::new()?;
let toml = r#"
model = "gpt-5.4"
[features.apps_mcp_path_override]
path = "/custom/mcp"
"#;
let cfg: ConfigToml =
toml::from_str(toml).expect("TOML deserialization should succeed for apps MCP feature");
let config = Config::load_from_base_config_with_overrides(
cfg,
ConfigOverrides::default(),
codex_home.abs(),
)
.await?;
assert_eq!(
config.apps_mcp_path_override.as_deref(),
Some("/custom/mcp")
);
Ok(())
}
#[tokio::test]
async fn config_loads_mcp_oauth_callback_url_from_toml() -> std::io::Result<()> {
let codex_home = TempDir::new()?;
+25
View File
@@ -55,6 +55,7 @@ use codex_config::types::UriBasedFileOpener;
use codex_config::types::WindowsSandboxModeToml;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::LOCAL_FS;
use codex_features::AppsMcpPathOverrideConfigToml;
use codex_features::Feature;
use codex_features::FeatureConfigSource;
use codex_features::FeatureOverrides;
@@ -673,6 +674,9 @@ pub struct Config {
/// Base URL for requests to ChatGPT (as opposed to the OpenAI API).
pub chatgpt_base_url: String,
/// Optional path override for the built-in apps MCP server.
pub apps_mcp_path_override: Option<String>,
/// Machine-local realtime audio device preferences used by realtime voice.
pub realtime_audio: RealtimeAudioConfig,
@@ -1003,6 +1007,7 @@ impl Config {
McpConfig {
chatgpt_base_url: self.chatgpt_base_url.clone(),
apps_mcp_path_override: self.apps_mcp_path_override.clone(),
codex_home: self.codex_home.to_path_buf(),
mcp_oauth_credentials_store_mode: self.mcp_oauth_credentials_store_mode,
mcp_oauth_callback_port: self.mcp_oauth_callback_port,
@@ -1814,6 +1819,15 @@ fn multi_agent_v2_toml_config(features: Option<&FeaturesToml>) -> Option<&MultiA
}
}
fn apps_mcp_path_override_toml_config(
features: Option<&FeaturesToml>,
) -> Option<&AppsMcpPathOverrideConfigToml> {
match features?.apps_mcp_path_override.as_ref()? {
FeatureToml::Enabled(_) => None,
FeatureToml::Config(config) => Some(config),
}
}
pub(crate) fn resolve_web_search_mode_for_turn(
web_search_mode: &Constrained<WebSearchMode>,
permission_profile: &PermissionProfile,
@@ -2333,6 +2347,16 @@ impl Config {
.unwrap_or(WebSearchMode::Cached);
let web_search_config = resolve_web_search_config(&cfg, &config_profile);
let multi_agent_v2 = resolve_multi_agent_v2_config(&cfg, &config_profile);
let apps_mcp_path_override = if features.enabled(Feature::AppsMcpPathOverride) {
let base = apps_mcp_path_override_toml_config(cfg.features.as_ref());
let profile = apps_mcp_path_override_toml_config(config_profile.features.as_ref());
profile
.and_then(|config| config.path.as_ref())
.or_else(|| base.and_then(|config| config.path.as_ref()))
.cloned()
} else {
None
};
let terminal_resize_reflow = resolve_terminal_resize_reflow_config(&cfg);
let agent_roles =
@@ -2840,6 +2864,7 @@ impl Config {
.chatgpt_base_url
.or(cfg.chatgpt_base_url)
.unwrap_or("https://chatgpt.com/backend-api/".to_string()),
apps_mcp_path_override,
realtime_audio: cfg
.audio
.map_or_else(RealtimeAudioConfig::default, |audio| RealtimeAudioConfig {
+15
View File
@@ -31,3 +31,18 @@ impl FeatureConfig for MultiAgentV2ConfigToml {
self.enabled
}
}
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)]
#[serde(deny_unknown_fields)]
pub struct AppsMcpPathOverrideConfigToml {
#[serde(skip_serializing_if = "Option::is_none")]
pub enabled: Option<bool>,
#[serde(skip_serializing_if = "Option::is_none")]
pub path: Option<String>,
}
impl FeatureConfig for AppsMcpPathOverrideConfigToml {
fn enabled(&self) -> Option<bool> {
self.enabled.or(self.path.as_ref().map(|_| true))
}
}
+18
View File
@@ -16,6 +16,7 @@ use toml::Table;
mod feature_configs;
mod legacy;
pub use feature_configs::AppsMcpPathOverrideConfigToml;
pub use feature_configs::MultiAgentV2ConfigToml;
use legacy::LegacyFeatureToggles;
pub use legacy::legacy_feature_keys;
@@ -149,6 +150,8 @@ pub enum Feature {
Apps,
/// Enable MCP apps.
EnableMcpApps,
/// Use the new path for the built-in apps MCP server.
AppsMcpPathOverride,
/// Enable the tool_search tool for apps.
ToolSearch,
/// Always defer MCP tools behind tool_search instead of exposing small sets directly.
@@ -557,6 +560,8 @@ pub fn is_known_feature_key(key: &str) -> bool {
pub struct FeaturesToml {
#[serde(default, skip_serializing_if = "Option::is_none")]
pub multi_agent_v2: Option<FeatureToml<MultiAgentV2ConfigToml>>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub apps_mcp_path_override: Option<FeatureToml<AppsMcpPathOverrideConfigToml>>,
/// Boolean feature toggles keyed by canonical or legacy feature name.
#[serde(flatten)]
entries: BTreeMap<String, bool>,
@@ -575,6 +580,13 @@ impl FeaturesToml {
if let Some(enabled) = self.multi_agent_v2.as_ref().and_then(FeatureToml::enabled) {
entries.insert(Feature::MultiAgentV2.key().to_string(), enabled);
}
if let Some(enabled) = self
.apps_mcp_path_override
.as_ref()
.and_then(FeatureToml::enabled)
{
entries.insert(Feature::AppsMcpPathOverride.key().to_string(), enabled);
}
entries
}
}
@@ -848,6 +860,12 @@ pub const FEATURES: &[FeatureSpec] = &[
stage: Stage::UnderDevelopment,
default_enabled: false,
},
FeatureSpec {
id: Feature::AppsMcpPathOverride,
key: "apps_mcp_path_override",
stage: Stage::UnderDevelopment,
default_enabled: false,
},
FeatureSpec {
id: Feature::ToolSearch,
key: "tool_search",
+21 -4
View File
@@ -50,6 +50,8 @@ use tracing::warn;
const DEFAULT_ISSUER: &str = "https://auth.openai.com";
const DEFAULT_PORT: u16 = 1455;
// Keep in sync with the Codex CLI Hydra redirect URI allow-list.
const FALLBACK_PORT: u16 = 1457;
static LOGIN_ERROR_PAGE_TEMPLATE: LazyLock<Template> = LazyLock::new(|| {
Template::parse(include_str!("assets/error.html"))
.unwrap_or_else(|err| panic!("login error page template must parse: {err}"))
@@ -527,9 +529,12 @@ fn send_cancel_request(port: u16) -> io::Result<()> {
}
fn bind_server(port: u16) -> io::Result<Server> {
let bind_address = format!("127.0.0.1:{port}");
let preferred_bind_address = format!("127.0.0.1:{port}");
let fallback_bind_address = format!("127.0.0.1:{FALLBACK_PORT}");
let mut bind_address = preferred_bind_address.clone();
let mut cancel_attempted = false;
let mut attempts = 0;
let mut using_fallback_port = false;
const MAX_ATTEMPTS: u32 = 10;
const RETRY_DELAY: Duration = Duration::from_millis(200);
@@ -543,10 +548,10 @@ fn bind_server(port: u16) -> io::Result<Server> {
.map(|io_err| io_err.kind() == io::ErrorKind::AddrInUse)
.unwrap_or(false);
// If the address is in use, there is probably another instance of the login server
// running. Attempt to cancel it and retry.
// If the address is in use, there may be another instance of the login server
// running. Attempt to cancel it and retry before falling back.
if is_addr_in_use {
if !cancel_attempted {
if !cancel_attempted && !using_fallback_port {
cancel_attempted = true;
if let Err(cancel_err) = send_cancel_request(port) {
eprintln!("Failed to cancel previous login server: {cancel_err}");
@@ -556,6 +561,18 @@ fn bind_server(port: u16) -> io::Result<Server> {
thread::sleep(RETRY_DELAY);
if attempts >= MAX_ATTEMPTS {
if port == DEFAULT_PORT && !using_fallback_port {
warn!(
%preferred_bind_address,
%fallback_bind_address,
"default login callback port is unavailable; falling back to the registered fallback port"
);
bind_address = fallback_bind_address.clone();
attempts = 0;
using_fallback_port = true;
continue;
}
return Err(io::Error::new(
io::ErrorKind::AddrInUse,
format!("Port {bind_address} is already in use"),
@@ -2,6 +2,7 @@
use std::io;
use std::net::SocketAddr;
use std::net::TcpListener;
use std::sync::Arc;
use std::thread;
use std::time::Duration;
@@ -13,6 +14,9 @@ use codex_login::run_login_server;
use core_test_support::skip_if_no_network;
use tempfile::tempdir;
const DEFAULT_LOGIN_PORT: u16 = 1455;
const FALLBACK_LOGIN_PORT: u16 = 1457;
// See spawn.rs for details
fn start_mock_issuer(chatgpt_account_id: &str) -> (SocketAddr, thread::JoinHandle<()>) {
@@ -401,6 +405,72 @@ async fn oauth_access_denied_unknown_reason_uses_generic_error_page() -> Result<
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
async fn falls_back_to_registered_fallback_port_when_default_port_is_in_use() -> Result<()> {
skip_if_no_network!(Ok(()));
match TcpListener::bind(("127.0.0.1", FALLBACK_LOGIN_PORT)) {
Ok(listener) => drop(listener),
Err(err) if err.kind() == io::ErrorKind::AddrInUse => {
eprintln!("Skipping test because 127.0.0.1:{FALLBACK_LOGIN_PORT} is already in use");
return Ok(());
}
Err(err) => return Err(err.into()),
}
let default_port_listener = match TcpListener::bind(("127.0.0.1", DEFAULT_LOGIN_PORT)) {
Ok(listener) => listener,
Err(err) if err.kind() == io::ErrorKind::AddrInUse => {
eprintln!("Skipping test because 127.0.0.1:{DEFAULT_LOGIN_PORT} is already in use");
return Ok(());
}
Err(err) => return Err(err.into()),
};
let default_port_server =
Arc::new(tiny_http::Server::from_listener(default_port_listener, None).unwrap());
let default_port_server_handle = {
let server = default_port_server.clone();
thread::spawn(move || {
while let Ok(req) = server.recv() {
let _ = req.respond(tiny_http::Response::from_string("not codex"));
}
})
};
let (issuer_addr, _issuer_handle) = start_mock_issuer("org-123");
let issuer = format!("http://{}:{}", issuer_addr.ip(), issuer_addr.port());
let tmp = tempdir()?;
let mut opts = ServerOptions::new(
tmp.path().to_path_buf(),
codex_login::CLIENT_ID.to_string(),
/*forced_chatgpt_workspace_id*/ None,
AuthCredentialsStoreMode::File,
);
opts.issuer = issuer;
opts.open_browser = false;
opts.force_state = Some("fallback_state".to_string());
let server_result = run_login_server(opts);
default_port_server.unblock();
let _ = default_port_server_handle.join();
let server = server_result?;
let actual_port = server.actual_port;
let auth_url = server.auth_url.clone();
server.cancel();
let _ = tokio::time::timeout(Duration::from_secs(2), server.block_until_done())
.await
.expect("login server should shut down after cancel");
assert_eq!(actual_port, FALLBACK_LOGIN_PORT);
assert!(auth_url.contains(&format!(
"redirect_uri=http%3A%2F%2Flocalhost%3A{FALLBACK_LOGIN_PORT}%2Fauth%2Fcallback"
)));
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
async fn cancels_previous_login_server_when_port_is_in_use() -> Result<()> {
skip_if_no_network!(Ok(()));
@@ -237,6 +237,7 @@ fn new_config(model: Option<String>, arg0_paths: Arg0DispatchPaths) -> anyhow::R
model_catalog: None,
model_verbosity: None,
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
apps_mcp_path_override: None,
realtime_audio: RealtimeAudioConfig::default(),
experimental_realtime_ws_base_url: None,
experimental_realtime_ws_model: None,