mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Reconnect MCP servers when authentication mode changes (#40634)
## What changed - Include the configured authentication mode in the MCP connection identity so runtime reconciliation replaces connections instead of reusing them after an authentication-mode change. - Add a regression test covering a switch from OAuth to ChatGPT authentication for an executor-owned MCP server. GitOrigin-RevId: 18cd9df62a9a5ee44594dcde6a844ad7d5e67ee2
This commit is contained in:
committed by
copyberry
parent
304c8de4c6
commit
f5853cbe90
Generated
+1
@@ -3578,6 +3578,7 @@ version = "0.0.0"
|
||||
dependencies = [
|
||||
"anyhow",
|
||||
"arc-swap",
|
||||
"assert_matches",
|
||||
"async-channel",
|
||||
"codex-api",
|
||||
"codex-async-utils",
|
||||
|
||||
@@ -43,6 +43,7 @@ tracing = { workspace = true }
|
||||
url = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
assert_matches = { workspace = true }
|
||||
codex-exec-server-test-support = { workspace = true }
|
||||
codex-plugin = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
|
||||
@@ -22,6 +22,7 @@ use crate::tools::ToolFilter;
|
||||
use crate::tools::ToolInfo;
|
||||
use crate::tools::filter_tools;
|
||||
use crate::tools::normalize_tools_for_model_with_prefix;
|
||||
use assert_matches::assert_matches;
|
||||
use codex_config::AppToolApproval;
|
||||
use codex_config::Constrained;
|
||||
use codex_config::McpServerAuth;
|
||||
@@ -4656,6 +4657,68 @@ async fn reconciliation_reuses_legacy_stdio_server_with_existing_protocol_marker
|
||||
assert!(previous.shares_test_connection_with(&reconciled, "docs"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn reconciliation_replaces_connection_when_auth_mode_changes() -> anyhow::Result<()> {
|
||||
let environment_manager = Arc::new(environment_manager_without_environments());
|
||||
environment_manager.upsert_environment(
|
||||
"customer-executor".to_string(),
|
||||
"ws://127.0.0.1:1".to_string(),
|
||||
/*connect_timeout*/ None,
|
||||
)?;
|
||||
let runtime_context = McpRuntimeContext::new(environment_manager, PathBuf::from("/tmp"));
|
||||
let codex_home = tempdir()?;
|
||||
let mcp_config = crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf());
|
||||
let [config, refreshed_config] = [McpServerAuth::OAuth, McpServerAuth::ChatGpt].map(|auth| {
|
||||
let mut config = reusable_server_config("https://chatgpt.com/backend-api/ps/mcp");
|
||||
config.environment_id = "customer-executor".to_string();
|
||||
config.auth = auth;
|
||||
crate::effective_mcp_servers_from_configured(
|
||||
HashMap::from([("docs".to_string(), config)]),
|
||||
&mcp_config,
|
||||
/*auth*/ None,
|
||||
)
|
||||
.remove("docs")
|
||||
.expect("configured server should survive auth projection")
|
||||
.config()
|
||||
.clone()
|
||||
});
|
||||
let previous = manager_with_reusable_ready_server(
|
||||
&config,
|
||||
&runtime_context,
|
||||
vec![create_test_tool("docs", "search")],
|
||||
)
|
||||
.await;
|
||||
|
||||
let reconciled = reconcile_reusable_server(&previous, refreshed_config, runtime_context).await;
|
||||
let outcome = reconciled
|
||||
.servers
|
||||
.get("docs")
|
||||
.expect("refreshed server should exist")
|
||||
.connection
|
||||
.client()
|
||||
.await;
|
||||
assert_matches!(
|
||||
outcome.err().expect("changed auth mode must be validated"),
|
||||
StartupOutcomeError::Failed {
|
||||
error,
|
||||
is_authentication_required,
|
||||
} => {
|
||||
assert_eq!(
|
||||
(error.as_str(), is_authentication_required),
|
||||
(
|
||||
"executor-owned MCP server `docs` cannot use hosted ChatGPT authentication; configure executor-owned credentials instead",
|
||||
false,
|
||||
)
|
||||
);
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
model_tool_names(&reconciled.list_all_tools().await),
|
||||
HashSet::new()
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn reconciliation_replaces_connection_when_protocol_mode_changes() {
|
||||
let runtime_context = reusable_server_runtime_context();
|
||||
|
||||
@@ -93,6 +93,7 @@ pub(crate) fn has_explicit_http_authorization(config: &McpServerConfig) -> bool
|
||||
/// those belong to a publication and can change without reconnecting.
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct McpServerConnectionIdentity {
|
||||
auth: McpServerAuth,
|
||||
transport: McpServerTransportConfig,
|
||||
environment_id: String,
|
||||
oauth_store: Option<(OAuthCredentialsStoreMode, AuthKeyringBackendKind)>,
|
||||
@@ -206,6 +207,7 @@ impl McpServerConnectionIdentity {
|
||||
.is_some_and(StoredOAuthCredentialSnapshot::store_was_contended);
|
||||
|
||||
Self {
|
||||
auth: config.auth.clone(),
|
||||
transport: config.transport.clone(),
|
||||
environment_id: config.environment_id.clone(),
|
||||
oauth_store: stored_oauth_url
|
||||
@@ -239,7 +241,8 @@ impl McpServerConnectionIdentity {
|
||||
(None, None) => true,
|
||||
(Some(_), None) | (None, Some(_)) => false,
|
||||
};
|
||||
self.transport == other.transport
|
||||
self.auth == other.auth
|
||||
&& self.transport == other.transport
|
||||
&& self.environment_id == other.environment_id
|
||||
&& self.oauth_store == other.oauth_store
|
||||
&& same_resolved_environment(&self.resolved_environment, &other.resolved_environment)
|
||||
|
||||
Reference in New Issue
Block a user