codex: address PR review feedback (#28582)

This commit is contained in:
Steven Lee
2026-06-16 20:58:28 +00:00
parent 7db1786573
commit e750db2ccb
16 changed files with 199 additions and 13 deletions

1
codex-rs/Cargo.lock generated
View File

@@ -2300,6 +2300,7 @@ dependencies = [
"codex-plugin",
"codex-utils-cargo-bin",
"codex-utils-cli",
"codex-utils-plugins",
"pretty_assertions",
"serde",
"serde_json",

View File

@@ -18,6 +18,7 @@ codex-login = { workspace = true }
codex-model-provider = { workspace = true }
codex-plugin = { workspace = true }
codex-utils-cli = { workspace = true }
codex-utils-plugins = { workspace = true }
serde = { workspace = true, features = ["derive"] }
tokio = { workspace = true, features = ["full"] }

View File

@@ -1,6 +1,9 @@
use codex_core::config::Config;
use codex_login::AuthManager;
use codex_login::default_client::chatgpt_cloudflare_cookie_header;
use codex_login::default_client::create_client;
use codex_utils_plugins::plugin_service_routing::plugin_service_preview_enabled;
use codex_utils_plugins::plugin_service_routing::plugin_service_routing_cookie;
use anyhow::Context;
use serde::de::DeserializeOwned;
@@ -9,18 +12,54 @@ use std::time::Duration;
const OAI_PRODUCT_SKU_HEADER: &str = "OAI-Product-Sku";
const CODEX_PRODUCT_SKU: &str = "codex";
#[derive(Clone, Copy)]
enum ChatgptRequestRouting {
Default,
PluginService,
}
/// Make a GET request to the ChatGPT backend API.
pub(crate) async fn chatgpt_get_request<T: DeserializeOwned>(
config: &Config,
path: String,
) -> anyhow::Result<T> {
chatgpt_get_request_with_timeout(config, path, /*timeout*/ None).await
chatgpt_get_request_with_timeout_inner(
config,
path,
/*timeout*/ None,
ChatgptRequestRouting::Default,
)
.await
}
pub(crate) async fn chatgpt_get_request_with_timeout<T: DeserializeOwned>(
config: &Config,
path: String,
timeout: Option<Duration>,
) -> anyhow::Result<T> {
chatgpt_get_request_with_timeout_inner(config, path, timeout, ChatgptRequestRouting::Default)
.await
}
pub(crate) async fn chatgpt_get_plugin_service_request_with_timeout<T: DeserializeOwned>(
config: &Config,
path: String,
timeout: Option<Duration>,
) -> anyhow::Result<T> {
chatgpt_get_request_with_timeout_inner(
config,
path,
timeout,
ChatgptRequestRouting::PluginService,
)
.await
}
async fn chatgpt_get_request_with_timeout_inner<T: DeserializeOwned>(
config: &Config,
path: String,
timeout: Option<Duration>,
routing: ChatgptRequestRouting,
) -> anyhow::Result<T> {
let chatgpt_base_url = &config.chatgpt_base_url;
let auth_manager =
@@ -52,6 +91,16 @@ pub(crate) async fn chatgpt_get_request_with_timeout<T: DeserializeOwned>(
.header(OAI_PRODUCT_SKU_HEADER, CODEX_PRODUCT_SKU)
.header("Content-Type", "application/json");
if matches!(routing, ChatgptRequestRouting::PluginService) && plugin_service_preview_enabled() {
let cloudflare_cookie = chatgpt_cloudflare_cookie_header(&url);
let existing_cookie_headers = cloudflare_cookie.as_deref().into_iter().collect::<Vec<_>>();
if let Some(routing_cookie) =
plugin_service_routing_cookie(&existing_cookie_headers, /*preview_enabled*/ true)
{
request = request.header("Cookie", routing_cookie);
}
}
if let Some(timeout) = timeout {
request = request.timeout(timeout);
}

View File

@@ -2,7 +2,7 @@ use std::collections::HashMap;
use std::collections::HashSet;
use std::time::Duration;
use crate::chatgpt_client::chatgpt_get_request_with_timeout;
use crate::chatgpt_client::chatgpt_get_plugin_service_request_with_timeout;
use codex_app_server_protocol::AppInfo;
use codex_connectors::ConnectorDirectoryCacheContext;
@@ -21,6 +21,7 @@ pub use codex_core::connectors::with_app_enabled_state;
use codex_login::AuthManager;
use codex_login::CodexAuth;
use codex_plugin::AppConnectorId;
use codex_utils_plugins::plugin_service_routing::plugin_service_preview_enabled;
const DIRECTORY_CONNECTORS_TIMEOUT: Duration = Duration::from_secs(60);
@@ -101,7 +102,7 @@ pub async fn list_all_connectors_with_options(
auth.is_workspace_account(),
force_refetch,
|path| async move {
chatgpt_get_request_with_timeout::<DirectoryListResponse>(
chatgpt_get_plugin_service_request_with_timeout::<DirectoryListResponse>(
config,
path,
Some(DIRECTORY_CONNECTORS_TIMEOUT),
@@ -127,7 +128,8 @@ fn connector_directory_cache_context(
auth.get_account_id(),
auth.get_chatgpt_user_id(),
auth.is_workspace_account(),
),
)
.plugin_service_preview(plugin_service_preview_enabled()),
)
}

View File

@@ -55,6 +55,18 @@ pub fn with_chatgpt_cloudflare_cookie_store(
builder.cookie_provider(Arc::clone(&SHARED_CHATGPT_CLOUDFLARE_COOKIE_STORE))
}
/// Returns the Cloudflare infrastructure cookies that the shared Codex cookie store would attach
/// to `url`.
///
/// Use this only when a caller must construct an explicit `Cookie` header. Reqwest does not add
/// cookie-provider values when that header is already present.
pub fn chatgpt_cloudflare_cookie_header(url: &str) -> Option<Vec<u8>> {
let url = reqwest::Url::parse(url).ok()?;
SHARED_CHATGPT_CLOUDFLARE_COOKIE_STORE
.cookies(&url)
.map(|value| value.as_bytes().to_vec())
}
fn is_chatgpt_cookie_url(url: &reqwest::Url) -> bool {
match url.scheme() {
"https" => {}

View File

@@ -10,6 +10,7 @@ mod sse;
mod telemetry;
mod transport;
pub use crate::chatgpt_cloudflare_cookies::chatgpt_cloudflare_cookie_header;
pub use crate::chatgpt_cloudflare_cookies::with_chatgpt_cloudflare_cookie_store;
pub use crate::chatgpt_hosts::is_allowed_chatgpt_host;
pub use crate::custom_ca::BuildCustomCaTransportError;

View File

@@ -16,6 +16,7 @@ use anyhow::Context;
use codex_login::CodexAuth;
use codex_protocol::mcp::McpServerInfo;
use codex_utils_plugins::mcp_connector::sanitize_name;
use codex_utils_plugins::plugin_service_routing::plugin_service_preview_enabled;
use serde::Deserialize;
use serde::Serialize;
use sha1::Digest;
@@ -26,6 +27,8 @@ pub struct CodexAppsToolsCacheKey {
pub(crate) account_id: Option<String>,
pub(crate) chatgpt_user_id: Option<String>,
pub(crate) is_workspace_account: bool,
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
pub(crate) plugin_service_preview: bool,
}
pub fn codex_apps_tools_cache_key(auth: Option<&CodexAuth>) -> CodexAppsToolsCacheKey {
@@ -33,6 +36,7 @@ pub fn codex_apps_tools_cache_key(auth: Option<&CodexAuth>) -> CodexAppsToolsCac
account_id: auth.and_then(CodexAuth::get_account_id),
chatgpt_user_id: auth.and_then(CodexAuth::get_chatgpt_user_id),
is_workspace_account: auth.is_some_and(CodexAuth::is_workspace_account),
plugin_service_preview: plugin_service_preview_enabled(),
}
}

View File

@@ -86,6 +86,7 @@ fn create_codex_apps_tools_cache_context(
account_id: account_id.map(ToOwned::to_owned),
chatgpt_user_id: chatgpt_user_id.map(ToOwned::to_owned),
is_workspace_account: false,
plugin_service_preview: false,
},
}
}
@@ -611,6 +612,24 @@ fn codex_apps_tools_cache_is_scoped_per_user() {
);
}
#[test]
fn codex_apps_tools_cache_is_scoped_by_plugin_service_preview() {
let codex_home = tempdir().expect("tempdir");
let regular = create_codex_apps_tools_cache_context(
codex_home.path().to_path_buf(),
Some("account-one"),
Some("user-one"),
);
let mut preview = regular.clone();
preview.user_key.plugin_service_preview = true;
assert_ne!(regular.tools_cache_path(), preview.tools_cache_path());
assert_ne!(
regular.server_info_cache_path(),
preview.server_info_cache_path()
);
}
#[test]
fn codex_apps_tools_cache_preserves_formerly_disallowed_connectors() {
let codex_home = tempdir().expect("tempdir");
@@ -1263,6 +1282,7 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() {
account_id: None,
chatgpt_user_id: None,
is_workspace_account: false,
plugin_service_preview: false,
},
/*prefix_mcp_tool_names*/ true,
ElicitationCapability::default(),

View File

@@ -33,6 +33,8 @@ pub struct ConnectorDirectoryCacheKey {
account_id: Option<String>,
chatgpt_user_id: Option<String>,
is_workspace_account: bool,
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
plugin_service_preview: bool,
}
impl ConnectorDirectoryCacheKey {
@@ -47,8 +49,14 @@ impl ConnectorDirectoryCacheKey {
account_id,
chatgpt_user_id,
is_workspace_account,
plugin_service_preview: false,
}
}
pub fn plugin_service_preview(mut self, plugin_service_preview: bool) -> Self {
self.plugin_service_preview = plugin_service_preview;
self
}
}
#[derive(Clone)]
@@ -492,6 +500,18 @@ mod tests {
ConnectorDirectoryCacheContext::new(codex_home.path().to_path_buf(), cache_key(id))
}
#[test]
fn directory_cache_is_scoped_by_plugin_service_preview() {
let codex_home = tempfile::tempdir().expect("tempdir");
let regular = cache_context(&codex_home, "user");
let preview = ConnectorDirectoryCacheContext::new(
codex_home.path().to_path_buf(),
cache_key("user").plugin_service_preview(true),
);
assert_ne!(regular.cache_path(), preview.cache_path());
}
fn clear_directory_memory_cache() {
let mut cache_guard = CONNECTOR_DIRECTORY_CACHE
.lock()

View File

@@ -10,6 +10,7 @@ use codex_app_server_protocol::PluginInterface;
use codex_app_server_protocol::SkillInterface;
use codex_login::CodexAuth;
use codex_login::default_client::build_reqwest_client;
use codex_login::default_client::chatgpt_cloudflare_cookie_header;
use codex_plugin::AppConnectorId;
use codex_plugin::AppDeclaration;
use codex_plugin::PluginId;
@@ -1878,12 +1879,31 @@ async fn send_plugin_service_request_with_preview(
) -> Result<reqwest::Response, reqwest::Error> {
let (client, request) = request.build_split();
let mut request = request?;
let headers = request.headers_mut();
let existing_cookie_headers = headers
let cloudflare_cookie = (preview_enabled && !request.headers().contains_key(COOKIE))
.then(|| chatgpt_cloudflare_cookie_header(request.url().as_str()))
.flatten();
apply_plugin_service_routing_cookie(
request.headers_mut(),
preview_enabled,
cloudflare_cookie.as_deref(),
);
client.execute(request).await
}
fn apply_plugin_service_routing_cookie(
headers: &mut reqwest::header::HeaderMap,
preview_enabled: bool,
cloudflare_cookie: Option<&[u8]>,
) {
let mut existing_cookie_headers = headers
.get_all(COOKIE)
.iter()
.map(|value| value.as_bytes().to_vec())
.collect::<Vec<_>>();
if let Some(cloudflare_cookie) = cloudflare_cookie {
existing_cookie_headers.push(cloudflare_cookie.to_vec());
}
let existing_cookie_headers = existing_cookie_headers
.iter()
.map(Vec::as_slice)
@@ -1896,6 +1916,4 @@ async fn send_plugin_service_request_with_preview(
{
headers.insert(COOKIE, routing_cookie);
}
client.execute(request).await
}

View File

@@ -1,6 +1,7 @@
use super::RemotePluginDirectoryItem;
use super::RemotePluginServiceConfig;
use codex_login::CodexAuth;
use codex_utils_plugins::plugin_service_routing::plugin_service_preview_enabled;
use serde::Deserialize;
use serde::Serialize;
use std::path::Path;
@@ -16,6 +17,8 @@ struct RemotePluginCatalogCacheKey {
account_id: Option<String>,
chatgpt_user_id: Option<String>,
is_workspace_account: bool,
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
plugin_service_preview: bool,
}
impl RemotePluginCatalogCacheKey {
@@ -25,6 +28,7 @@ impl RemotePluginCatalogCacheKey {
account_id: auth.get_account_id(),
chatgpt_user_id: auth.get_chatgpt_user_id(),
is_workspace_account: auth.is_workspace_account(),
plugin_service_preview: plugin_service_preview_enabled(),
}
}
}
@@ -109,3 +113,7 @@ fn cache_path(codex_home: &Path, cache_key: &RemotePluginCatalogCacheKey) -> Pat
.join(REMOTE_PLUGIN_CATALOG_DISK_CACHE_DIR)
.join(format!("{cache_key_hash:016x}.json"))
}
#[cfg(test)]
#[path = "catalog_cache_tests.rs"]
mod tests;

View File

@@ -0,0 +1,21 @@
use super::*;
#[test]
fn remote_plugin_catalog_cache_is_scoped_by_plugin_service_preview() {
let regular = RemotePluginCatalogCacheKey {
chatgpt_base_url: "https://chatgpt.com/backend-api".to_string(),
account_id: Some("account".to_string()),
chatgpt_user_id: Some("user".to_string()),
is_workspace_account: true,
plugin_service_preview: false,
};
let preview = RemotePluginCatalogCacheKey {
plugin_service_preview: true,
..regular.clone()
};
assert_ne!(
cache_path(Path::new("/codex-home"), &regular),
cache_path(Path::new("/codex-home"), &preview)
);
}

View File

@@ -299,6 +299,23 @@ fn recommended_plugins_ignore_invalid_remote_plugin_ids() {
);
}
#[test]
fn plugin_service_routing_preserves_cloudflare_cookie_jar_values() {
let mut headers = reqwest::header::HeaderMap::new();
let cloudflare_cookie = HeaderValue::from_static("cf_clearance=clearance; _cfuvid=visitor");
apply_plugin_service_routing_cookie(
&mut headers,
/*preview_enabled*/ true,
Some(cloudflare_cookie.as_bytes()),
);
assert_eq!(
headers.get(COOKIE).and_then(|value| value.to_str().ok()),
Some("cf_clearance=clearance; _cfuvid=visitor; oai-chat-plugin-service-preview=true"),
);
}
#[tokio::test]
async fn plugin_service_request_does_not_add_preview_cookie_when_disabled() {
let server = MockServer::start().await;

View File

@@ -41,6 +41,7 @@ use codex_mcp::codex_apps_tools_cache_key;
use codex_mcp::compute_auth_statuses;
use codex_mcp::effective_mcp_servers;
use codex_mcp::tool_plugin_provenance;
use codex_utils_plugins::plugin_service_routing::plugin_service_preview_enabled;
const CONNECTORS_READY_TIMEOUT_ON_EMPTY_TOOLS: Duration = Duration::from_secs(30);
@@ -460,7 +461,8 @@ async fn cached_directory_connectors_for_tool_suggest_with_auth(
Some(account_id),
auth.get_chatgpt_user_id(),
is_workspace_account,
),
)
.plugin_service_preview(plugin_service_preview_enabled()),
);
codex_connectors::cached_directory_connectors(&cache_context).unwrap_or_default()

View File

@@ -11,6 +11,7 @@ use codex_client::CodexHttpClient;
pub use codex_client::CodexRequestBuilder;
use codex_client::build_reqwest_client_for_route;
use codex_client::build_reqwest_client_with_custom_ca;
pub use codex_client::chatgpt_cloudflare_cookie_header;
use codex_client::with_chatgpt_cloudflare_cookie_store;
use codex_terminal_detection::user_agent;
use reqwest::header::HeaderMap;

View File

@@ -22,10 +22,19 @@ fn preview_signal_requires_exact_enabled_value() {
#[test]
fn routing_cookie_is_disabled_by_default_and_cannot_be_enabled_by_caller() {
assert_eq!(plugin_service_routing_cookie(&[], false), None);
assert_eq!(
plugin_service_routing_cookie(&[b"oai-chat-plugin-service-preview=true".as_slice()], false,),
None,
plugin_service_routing_cookie(&[], /*preview_enabled*/ false),
None
);
assert_eq!(
plugin_service_routing_cookie(
&[
b"session=abc; oai-chat-plugin-service-preview=true".as_slice(),
b"theme=dark".as_slice(),
],
/*preview_enabled*/ false,
),
Some(b"session=abc; theme=dark".to_vec()),
);
}
@@ -37,7 +46,7 @@ fn routing_cookie_preserves_unrelated_cookies_and_replaces_caller_value() {
b"session=abc; oai-chat-plugin-service-preview=false".as_slice(),
b"theme=dark; oai-chat-plugin-service-preview=true".as_slice(),
],
true,
/*preview_enabled*/ true,
),
Some(b"session=abc; theme=dark; oai-chat-plugin-service-preview=true".to_vec()),
);