mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Classify nested MCP authentication startup errors (#30257)
## Summary - classify authentication-required RMCP startup failures, including errors nested inside `ClientInitializeError::TransportError` - let `codex-mcp` consume that classification so the existing `reauthenticationRequired` startup failure reason is emitted - add a regression test that performs real startup with an expired persisted OAuth token and no refresh token ## Why Follow-up to #29877. RMCP stores streamable HTTP initialization failures inside a dynamic transport error whose payload is not exposed through the standard Rust error source chain. The original `anyhow::Error::chain()` check therefore missed the nested `AuthError::AuthorizationRequired` seen during real MCP startup and emitted `failureReason: null`. The transport-specific inspection now lives in `codex-rmcp-client`, while `codex-mcp` consumes only the domain-level authentication-required result. This classifier does not distinguish first-time login from reauthentication; the existing auth-state logic remains responsible for that distinction. ## User impact When stored MCP OAuth credentials are expired and cannot be refreshed, app clients now receive `failureReason: "reauthenticationRequired"` on the failed startup update and can show the reconnect action. First-time login and unrelated startup failures remain unchanged. ## Validation - `just test -p codex-rmcp-client --test streamable_http_oauth_startup identifies_expired_unrefreshable_token_startup_error` - `just test -p codex-mcp startup_outcome_error_identifies_authentication_required` - `just test -p codex-mcp mcp_startup_failure_reason_requires_existing_oauth_and_auth_failure` - `cargo build -p codex-cli --bin codex` - local app-server probe emitted `failureReason: "reauthenticationRequired"` - manual end-to-end reconnect flow confirmed - `just fmt`
This commit is contained in:
committed by
GitHub
Unverified
parent
c55ce3b51b
commit
526f495f3a
@@ -57,6 +57,7 @@ use codex_rmcp_client::LocalStdioServerLauncher;
|
||||
use codex_rmcp_client::RmcpClient;
|
||||
use codex_rmcp_client::StdioServerLauncher;
|
||||
use codex_rmcp_client::ToolWithConnectorId;
|
||||
use codex_rmcp_client::is_authentication_required_error;
|
||||
use futures::future::BoxFuture;
|
||||
use futures::future::FutureExt;
|
||||
use futures::future::Shared;
|
||||
@@ -67,7 +68,6 @@ use rmcp::model::InitializeRequestParams;
|
||||
use rmcp::model::JsonObject;
|
||||
use rmcp::model::ProtocolVersion;
|
||||
use rmcp::model::Tool as RmcpTool;
|
||||
use rmcp::transport::auth::AuthError;
|
||||
use tokio::time::Instant as TokioInstant;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
use tracing::Instrument;
|
||||
@@ -552,16 +552,7 @@ impl StartupOutcomeError {
|
||||
|
||||
impl From<anyhow::Error> for StartupOutcomeError {
|
||||
fn from(error: anyhow::Error) -> Self {
|
||||
let is_authentication_required = error.chain().any(|source| {
|
||||
source
|
||||
.downcast_ref::<AuthError>()
|
||||
.is_some_and(|auth_error| {
|
||||
matches!(
|
||||
auth_error,
|
||||
AuthError::AuthorizationRequired | AuthError::TokenExpired
|
||||
)
|
||||
})
|
||||
});
|
||||
let is_authentication_required = is_authentication_required_error(&error);
|
||||
Self::Failed {
|
||||
error: error.to_string(),
|
||||
is_authentication_required,
|
||||
|
||||
@@ -9,6 +9,7 @@ mod oauth_http_client;
|
||||
mod perform_oauth_login;
|
||||
mod program_resolver;
|
||||
mod rmcp_client;
|
||||
mod startup_error;
|
||||
mod stdio_server_launcher;
|
||||
mod utils;
|
||||
|
||||
@@ -40,6 +41,7 @@ pub use rmcp_client::ListToolsWithConnectorIdResult;
|
||||
pub use rmcp_client::RmcpClient;
|
||||
pub use rmcp_client::SendElicitation;
|
||||
pub use rmcp_client::ToolWithConnectorId;
|
||||
pub use startup_error::is_authentication_required_error;
|
||||
pub use stdio_server_launcher::ExecutorStdioServerLauncher;
|
||||
pub use stdio_server_launcher::LocalStdioServerLauncher;
|
||||
pub use stdio_server_launcher::StdioServerLauncher;
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
use anyhow::Error;
|
||||
use rmcp::service::ClientInitializeError;
|
||||
use rmcp::transport::auth::AuthError;
|
||||
use rmcp::transport::streamable_http_client::StreamableHttpError;
|
||||
|
||||
use crate::http_client_adapter::StreamableHttpClientAdapterError;
|
||||
|
||||
/// Returns whether an RMCP client error indicates that authentication is required.
|
||||
///
|
||||
/// This does not distinguish first-time login from reauthentication.
|
||||
/// Streamable HTTP initialization errors are stored inside RMCP's dynamic
|
||||
/// transport error, which is not part of the standard error source chain.
|
||||
pub fn is_authentication_required_error(error: &Error) -> bool {
|
||||
error.chain().any(|source| {
|
||||
source
|
||||
.downcast_ref::<AuthError>()
|
||||
.is_some_and(auth_error_requires_authentication)
|
||||
|| source
|
||||
.downcast_ref::<ClientInitializeError>()
|
||||
.is_some_and(client_initialize_error_requires_authentication)
|
||||
})
|
||||
}
|
||||
|
||||
fn client_initialize_error_requires_authentication(error: &ClientInitializeError) -> bool {
|
||||
let ClientInitializeError::TransportError { error, .. } = error else {
|
||||
return false;
|
||||
};
|
||||
|
||||
error
|
||||
.error
|
||||
.downcast_ref::<StreamableHttpError<StreamableHttpClientAdapterError>>()
|
||||
.is_some_and(|error| {
|
||||
matches!(
|
||||
error,
|
||||
StreamableHttpError::Auth(auth_error)
|
||||
if auth_error_requires_authentication(auth_error)
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
fn auth_error_requires_authentication(error: &AuthError) -> bool {
|
||||
matches!(
|
||||
error,
|
||||
AuthError::AuthorizationRequired | AuthError::TokenExpired
|
||||
)
|
||||
}
|
||||
@@ -13,6 +13,7 @@ use codex_rmcp_client::RmcpClient;
|
||||
use codex_rmcp_client::StoredOAuthTokens;
|
||||
use codex_rmcp_client::WrappedOAuthTokenResponse;
|
||||
use codex_rmcp_client::determine_streamable_http_auth_status;
|
||||
use codex_rmcp_client::is_authentication_required_error;
|
||||
use codex_rmcp_client::save_oauth_tokens;
|
||||
use oauth2::AccessToken;
|
||||
use oauth2::RefreshToken;
|
||||
@@ -144,6 +145,40 @@ async fn reports_auth_status_for_persisted_credentials() -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||
async fn identifies_expired_unrefreshable_token_startup_error() -> anyhow::Result<()> {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/.well-known/oauth-authorization-server/mcp"))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"authorization_endpoint": format!("{}/oauth/authorize", server.uri()),
|
||||
"token_endpoint": format!("{}/oauth/token", server.uri()),
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let codex_home = TempDir::new()?;
|
||||
let status = Command::new(std::env::current_exe()?)
|
||||
.args([
|
||||
"expired_unrefreshable_startup_child",
|
||||
"--exact",
|
||||
"--ignored",
|
||||
"--nocapture",
|
||||
])
|
||||
.env("CODEX_HOME", codex_home.path())
|
||||
.env(CHILD_SERVER_URL_ENV, format!("{}/mcp", server.uri()))
|
||||
.status()
|
||||
.await?;
|
||||
|
||||
assert!(
|
||||
status.success(),
|
||||
"expired OAuth startup child failed: {status}"
|
||||
);
|
||||
server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||
#[ignore = "spawned by reports_auth_status_for_persisted_credentials"]
|
||||
async fn persisted_credentials_auth_status_child() -> anyhow::Result<()> {
|
||||
@@ -298,3 +333,46 @@ async fn oauth_startup_child() -> anyhow::Result<()> {
|
||||
initialize_client(&client).await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||
#[ignore = "spawned by identifies_expired_unrefreshable_token_startup_error"]
|
||||
async fn expired_unrefreshable_startup_child() -> anyhow::Result<()> {
|
||||
let server_url = std::env::var(CHILD_SERVER_URL_ENV)?;
|
||||
let response = OAuthTokenResponse::new(
|
||||
AccessToken::new(EXPIRED_ACCESS_TOKEN.to_string()),
|
||||
BasicTokenType::Bearer,
|
||||
VendorExtraTokenFields::default(),
|
||||
);
|
||||
let tokens = StoredOAuthTokens {
|
||||
server_name: SERVER_NAME.to_string(),
|
||||
url: server_url.clone(),
|
||||
client_id: "test-client-id".to_string(),
|
||||
token_response: WrappedOAuthTokenResponse(response),
|
||||
expires_at: Some(0),
|
||||
};
|
||||
save_oauth_tokens(
|
||||
SERVER_NAME,
|
||||
&tokens,
|
||||
OAuthCredentialsStoreMode::File,
|
||||
AuthKeyringBackendKind::default(),
|
||||
)?;
|
||||
|
||||
let client = RmcpClient::new_streamable_http_client(
|
||||
SERVER_NAME,
|
||||
&server_url,
|
||||
/*bearer_token*/ None,
|
||||
/*http_headers*/ None,
|
||||
/*env_http_headers*/ None,
|
||||
OAuthCredentialsStoreMode::File,
|
||||
AuthKeyringBackendKind::default(),
|
||||
Environment::default_for_tests().get_http_client(),
|
||||
/*auth_provider*/ None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let error = initialize_client(&client)
|
||||
.await
|
||||
.expect_err("expired token without a refresh token should fail startup");
|
||||
assert!(is_authentication_required_error(&error));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user