mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Surface MCP reauthentication-required startup failures (#29877)
## Summary - distinguish expired, non-refreshable stored MCP OAuth credentials from first-time missing credentials - carry a typed `failureReason: "reauthenticationRequired"` on the existing `mcpServer/startupStatus/updated` notification only when user action is required - keep the public MCP auth-status API unchanged and regenerate the app-server protocol schemas and documentation ## Why An MCP server with an expired access token and no usable refresh token currently fails startup without giving clients a reliable, typed recovery signal. The existing startup-status notification is the natural place to carry this state. Its nullable `failureReason` keeps the recovery reason attached to the failed startup transition without adding a one-off notification. Internally, Codex distinguishes first-time login from reauthentication and emits the reason only when the startup error itself requires authentication. ## User impact App clients can prompt an existing user to reconnect an MCP server when automatic recovery is impossible by handling a failed `mcpServer/startupStatus/updated` notification whose `failureReason` is `reauthenticationRequired`. Starting, ready, cancelled, unrelated failures, and first-time setup carry no reauthentication reason. ## Companion app PR - openai/openai#1069582 ## Validation - `just test -p codex-app-server-protocol` — 248 passed; schema fixture tests passed - `cargo check -p codex-app-server -p codex-tui` - `just test -p codex-rmcp-client -p codex-mcp` — 184 passed, 2 skipped - `just test -p codex-protocol -p codex-app-server-protocol -p codex-mcp` — 579 passed - `just write-app-server-schema` - `just fmt`
This commit is contained in:
committed by
GitHub
Unverified
parent
b80fbb70cd
commit
a6d20ed297
@@ -2384,6 +2384,12 @@
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"McpServerStartupFailureReason": {
|
||||
"enum": [
|
||||
"reauthenticationRequired"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"McpServerStartupState": {
|
||||
"enum": [
|
||||
"starting",
|
||||
@@ -2401,6 +2407,16 @@
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"failureReason": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/McpServerStartupFailureReason"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"name": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
+16
@@ -12094,6 +12094,12 @@
|
||||
"title": "McpServerRefreshResponse",
|
||||
"type": "object"
|
||||
},
|
||||
"McpServerStartupFailureReason": {
|
||||
"enum": [
|
||||
"reauthenticationRequired"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"McpServerStartupState": {
|
||||
"enum": [
|
||||
"starting",
|
||||
@@ -12165,6 +12171,16 @@
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"failureReason": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/v2/McpServerStartupFailureReason"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"name": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
+16
@@ -8498,6 +8498,12 @@
|
||||
"title": "McpServerRefreshResponse",
|
||||
"type": "object"
|
||||
},
|
||||
"McpServerStartupFailureReason": {
|
||||
"enum": [
|
||||
"reauthenticationRequired"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"McpServerStartupState": {
|
||||
"enum": [
|
||||
"starting",
|
||||
@@ -8569,6 +8575,16 @@
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"failureReason": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/McpServerStartupFailureReason"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"name": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
+16
@@ -1,6 +1,12 @@
|
||||
{
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"definitions": {
|
||||
"McpServerStartupFailureReason": {
|
||||
"enum": [
|
||||
"reauthenticationRequired"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"McpServerStartupState": {
|
||||
"enum": [
|
||||
"starting",
|
||||
@@ -18,6 +24,16 @@
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"failureReason": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/McpServerStartupFailureReason"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"name": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
+5
@@ -0,0 +1,5 @@
|
||||
// GENERATED CODE! DO NOT MODIFY BY HAND!
|
||||
|
||||
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
|
||||
|
||||
export type McpServerStartupFailureReason = "reauthenticationRequired";
|
||||
+2
-1
@@ -1,6 +1,7 @@
|
||||
// GENERATED CODE! DO NOT MODIFY BY HAND!
|
||||
|
||||
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
|
||||
import type { McpServerStartupFailureReason } from "./McpServerStartupFailureReason";
|
||||
import type { McpServerStartupState } from "./McpServerStartupState";
|
||||
|
||||
export type McpServerStatusUpdatedNotification = { threadId: string | null, name: string, status: McpServerStartupState, error: string | null, };
|
||||
export type McpServerStatusUpdatedNotification = { threadId: string | null, name: string, status: McpServerStartupState, error: string | null, failureReason: McpServerStartupFailureReason | null, };
|
||||
|
||||
@@ -238,6 +238,7 @@ export type { McpServerOauthLoginCompletedNotification } from "./McpServerOauthL
|
||||
export type { McpServerOauthLoginParams } from "./McpServerOauthLoginParams";
|
||||
export type { McpServerOauthLoginResponse } from "./McpServerOauthLoginResponse";
|
||||
export type { McpServerRefreshResponse } from "./McpServerRefreshResponse";
|
||||
export type { McpServerStartupFailureReason } from "./McpServerStartupFailureReason";
|
||||
export type { McpServerStartupState } from "./McpServerStartupState";
|
||||
export type { McpServerStatus } from "./McpServerStatus";
|
||||
export type { McpServerStatusDetail } from "./McpServerStatusDetail";
|
||||
|
||||
@@ -23,6 +23,12 @@ v2_enum_from_core!(
|
||||
}
|
||||
);
|
||||
|
||||
v2_enum_from_core!(
|
||||
pub enum McpServerStartupFailureReason from codex_protocol::protocol::McpStartupFailureReason {
|
||||
ReauthenticationRequired
|
||||
}
|
||||
);
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
@@ -242,6 +248,7 @@ pub struct McpServerStatusUpdatedNotification {
|
||||
pub name: String,
|
||||
pub status: McpServerStartupState,
|
||||
pub error: Option<String>,
|
||||
pub failure_reason: Option<McpServerStartupFailureReason>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)]
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use super::*;
|
||||
use crate::ServerNotification;
|
||||
use codex_protocol::approvals::ElicitationRequest as CoreElicitationRequest;
|
||||
use codex_protocol::config_types::MultiAgentMode;
|
||||
use codex_protocol::items::AgentMessageContent;
|
||||
@@ -2155,6 +2156,7 @@ fn mcp_server_status_updated_accepts_missing_thread_id() {
|
||||
name: "optional_broken".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("handshake failed".to_string()),
|
||||
failure_reason: None,
|
||||
};
|
||||
assert_eq!(notification, expected);
|
||||
assert_eq!(
|
||||
@@ -2164,6 +2166,33 @@ fn mcp_server_status_updated_accepts_missing_thread_id() {
|
||||
"name": "optional_broken",
|
||||
"status": "failed",
|
||||
"error": "handshake failed",
|
||||
"failureReason": null,
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_server_status_updated_serializes_failure_reason() {
|
||||
let notification =
|
||||
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
|
||||
thread_id: Some("thread-1".to_string()),
|
||||
name: "expired-oauth".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("OAuth credentials expired".to_string()),
|
||||
failure_reason: Some(McpServerStartupFailureReason::ReauthenticationRequired),
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
serde_json::to_value(notification).expect("notification should serialize"),
|
||||
json!({
|
||||
"method": "mcpServer/startupStatus/updated",
|
||||
"params": {
|
||||
"threadId": "thread-1",
|
||||
"name": "expired-oauth",
|
||||
"status": "failed",
|
||||
"error": "OAuth credentials expired",
|
||||
"failureReason": "reauthenticationRequired",
|
||||
},
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1345,7 +1345,7 @@ Because audio is intentionally separate from `ThreadItem`, clients can opt out o
|
||||
|
||||
### MCP server startup events
|
||||
|
||||
- `mcpServer/startupStatus/updated` — `{ threadId, name, status, error }` when app-server observes an MCP server startup transition. `threadId` identifies the owning thread when startup is thread-scoped and is `null` when startup is app-scoped. `status` is one of `starting`, `ready`, `failed`, or `cancelled`. `error` is `null` except for `failed`.
|
||||
- `mcpServer/startupStatus/updated` — `{ threadId, name, status, error, failureReason }` when app-server observes an MCP server startup transition. `threadId` identifies the owning thread when startup is thread-scoped and is `null` when startup is app-scoped. `status` is one of `starting`, `ready`, `failed`, or `cancelled`. `error` and `failureReason` are `null` except for `failed`; `failureReason` is `reauthenticationRequired` when stored OAuth credentials have expired and cannot be refreshed, so clients can prompt the user to reconnect the named server.
|
||||
|
||||
### Turn events
|
||||
|
||||
@@ -1935,7 +1935,7 @@ Codex supports these authentication modes. The current mode is surfaced in `acco
|
||||
- `account/rateLimits/updated` (notify) — emitted whenever a user's ChatGPT rate limits change. This is a sparse rolling update; merge available values into the most recent `account/rateLimits/read` response or refetch that snapshot.
|
||||
- `account/sendAddCreditsNudgeEmail` — ask ChatGPT to email the workspace owner about depleted credits or a reached usage limit.
|
||||
- `mcpServer/oauthLogin/completed` (notify) — emitted after a `mcpServer/oauth/login` flow finishes for a server; payload includes `{ name, threadId, success, error? }`.
|
||||
- `mcpServer/startupStatus/updated` (notify) — emitted when a configured MCP server's startup status changes; payload includes `{ threadId, name, status, error }`, where `threadId` is the owning thread when startup is thread-scoped and `null` when it is app-scoped, and `status` is `starting`, `ready`, `failed`, or `cancelled`.
|
||||
- `mcpServer/startupStatus/updated` (notify) — emitted when a configured MCP server's startup status changes; payload includes `{ threadId, name, status, error, failureReason }`, where `threadId` is the owning thread when startup is thread-scoped and `null` when it is app-scoped, and `status` is `starting`, `ready`, `failed`, or `cancelled`. `failureReason` is `reauthenticationRequired` when stored OAuth credentials have expired and cannot be refreshed, so clients can prompt the user to reconnect the named server.
|
||||
|
||||
### 1) Check auth state
|
||||
|
||||
|
||||
@@ -200,18 +200,20 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
.await;
|
||||
}
|
||||
EventMsg::McpStartupUpdate(update) => {
|
||||
let (status, error) = match update.status {
|
||||
let (status, error, failure_reason) = match update.status {
|
||||
codex_protocol::protocol::McpStartupStatus::Starting => {
|
||||
(McpServerStartupState::Starting, None)
|
||||
(McpServerStartupState::Starting, None, None)
|
||||
}
|
||||
codex_protocol::protocol::McpStartupStatus::Ready => {
|
||||
(McpServerStartupState::Ready, None)
|
||||
}
|
||||
codex_protocol::protocol::McpStartupStatus::Failed { error } => {
|
||||
(McpServerStartupState::Failed, Some(error))
|
||||
(McpServerStartupState::Ready, None, None)
|
||||
}
|
||||
codex_protocol::protocol::McpStartupStatus::Failed { error, reason } => (
|
||||
McpServerStartupState::Failed,
|
||||
Some(error),
|
||||
reason.map(Into::into),
|
||||
),
|
||||
codex_protocol::protocol::McpStartupStatus::Cancelled => {
|
||||
(McpServerStartupState::Cancelled, None)
|
||||
(McpServerStartupState::Cancelled, None, None)
|
||||
}
|
||||
};
|
||||
let notification = McpServerStatusUpdatedNotification {
|
||||
@@ -219,6 +221,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
name: update.server,
|
||||
status,
|
||||
error,
|
||||
failure_reason,
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::McpServerStatusUpdated(notification))
|
||||
|
||||
@@ -945,6 +945,7 @@ async fn thread_start_emits_mcp_server_status_updated_notifications() -> Result<
|
||||
name: "optional_broken".to_string(),
|
||||
status: McpServerStartupState::Starting,
|
||||
error: None,
|
||||
failure_reason: None,
|
||||
}
|
||||
);
|
||||
|
||||
@@ -977,6 +978,7 @@ async fn thread_start_emits_mcp_server_status_updated_notifications() -> Result<
|
||||
assert_eq!(failed.thread_id, Some(start_response.thread.id));
|
||||
assert_eq!(failed.name, "optional_broken");
|
||||
assert_eq!(failed.status, McpServerStartupState::Failed);
|
||||
assert_eq!(failed.failure_reason, None);
|
||||
assert!(
|
||||
failed
|
||||
.error
|
||||
|
||||
@@ -573,7 +573,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
|
||||
.map(|(name, cfg)| {
|
||||
let auth_status = auth_statuses
|
||||
.get(name.as_str())
|
||||
.map(|entry| entry.auth_status)
|
||||
.map(|entry| McpAuthStatus::from(entry.auth_state))
|
||||
.unwrap_or(McpAuthStatus::Unsupported);
|
||||
let transport = match &cfg.transport {
|
||||
McpServerTransportConfig::Stdio {
|
||||
@@ -657,7 +657,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
|
||||
let status = format_mcp_status(cfg);
|
||||
let auth_status = auth_statuses
|
||||
.get(name.as_str())
|
||||
.map(|entry| entry.auth_status)
|
||||
.map(|entry| McpAuthStatus::from(entry.auth_state))
|
||||
.unwrap_or(McpAuthStatus::Unsupported)
|
||||
.to_string();
|
||||
stdio_rows.push([
|
||||
@@ -678,7 +678,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
|
||||
let status = format_mcp_status(cfg);
|
||||
let auth_status = auth_statuses
|
||||
.get(name.as_str())
|
||||
.map(|entry| entry.auth_status)
|
||||
.map(|entry| McpAuthStatus::from(entry.auth_state))
|
||||
.unwrap_or(McpAuthStatus::Unsupported)
|
||||
.to_string();
|
||||
let bearer_token_display =
|
||||
|
||||
@@ -55,9 +55,12 @@ use codex_protocol::protocol::Event;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::McpStartupCompleteEvent;
|
||||
use codex_protocol::protocol::McpStartupFailure;
|
||||
use codex_protocol::protocol::McpStartupFailureReason;
|
||||
use codex_protocol::protocol::McpStartupStatus;
|
||||
use codex_protocol::protocol::McpStartupUpdateEvent;
|
||||
use codex_rmcp_client::ElicitationResponse;
|
||||
use codex_rmcp_client::McpAuthState;
|
||||
use codex_rmcp_client::McpLoginRequirement;
|
||||
use rmcp::model::ElicitationCapability;
|
||||
use rmcp::model::ListResourceTemplatesResult;
|
||||
use rmcp::model::ListResourcesResult;
|
||||
@@ -228,12 +231,16 @@ impl McpConnectionManager {
|
||||
Ok(_) => McpStartupStatus::Ready,
|
||||
Err(StartupOutcomeError::Cancelled) => McpStartupStatus::Cancelled,
|
||||
Err(error) => {
|
||||
let reason = mcp_startup_failure_reason(auth_entry.as_ref(), error);
|
||||
let error_str = mcp_init_error_display(
|
||||
server_name.as_str(),
|
||||
auth_entry.as_ref(),
|
||||
error,
|
||||
);
|
||||
McpStartupStatus::Failed { error: error_str }
|
||||
McpStartupStatus::Failed {
|
||||
error: error_str,
|
||||
reason,
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
@@ -266,7 +273,7 @@ impl McpConnectionManager {
|
||||
match outcome {
|
||||
Ok(_) => summary.ready.push(server_name),
|
||||
Err(StartupOutcomeError::Cancelled) => summary.cancelled.push(server_name),
|
||||
Err(StartupOutcomeError::Failed { error }) => {
|
||||
Err(StartupOutcomeError::Failed { error, .. }) => {
|
||||
summary.failed.push(McpStartupFailure {
|
||||
server: server_name,
|
||||
error,
|
||||
@@ -909,6 +916,28 @@ async fn emit_update(
|
||||
.await
|
||||
}
|
||||
|
||||
fn mcp_startup_failure_reason(
|
||||
entry: Option<&McpAuthStatusEntry>,
|
||||
error: &StartupOutcomeError,
|
||||
) -> Option<McpStartupFailureReason> {
|
||||
if !error.is_authentication_required() {
|
||||
return None;
|
||||
}
|
||||
|
||||
match entry.map(|entry| entry.auth_state) {
|
||||
Some(McpAuthState::LoggedOut(McpLoginRequirement::Reauthentication)) => {
|
||||
Some(McpStartupFailureReason::ReauthenticationRequired)
|
||||
}
|
||||
Some(
|
||||
McpAuthState::Unsupported
|
||||
| McpAuthState::LoggedOut(McpLoginRequirement::Login)
|
||||
| McpAuthState::BearerToken
|
||||
| McpAuthState::OAuth,
|
||||
)
|
||||
| None => None,
|
||||
}
|
||||
}
|
||||
|
||||
fn mcp_init_error_display(
|
||||
server_name: &str,
|
||||
entry: Option<&McpAuthStatusEntry>,
|
||||
@@ -955,20 +984,20 @@ fn mcp_init_error_display(
|
||||
fn startup_outcome_error_message(error: StartupOutcomeError) -> String {
|
||||
match error {
|
||||
StartupOutcomeError::Cancelled => "MCP startup cancelled".to_string(),
|
||||
StartupOutcomeError::Failed { error } => error,
|
||||
StartupOutcomeError::Failed { error, .. } => error,
|
||||
}
|
||||
}
|
||||
|
||||
fn is_mcp_client_auth_required_error(error: &StartupOutcomeError) -> bool {
|
||||
match error {
|
||||
StartupOutcomeError::Failed { error } => error.contains("Auth required"),
|
||||
StartupOutcomeError::Failed { error, .. } => error.contains("Auth required"),
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
fn is_mcp_client_startup_timeout_error(error: &StartupOutcomeError) -> bool {
|
||||
match error {
|
||||
StartupOutcomeError::Failed { error } => {
|
||||
StartupOutcomeError::Failed { error, .. } => {
|
||||
error.contains("request timed out")
|
||||
|| error.contains("timed out handshaking with MCP server")
|
||||
}
|
||||
|
||||
@@ -25,7 +25,6 @@ use codex_protocol::ToolName;
|
||||
use codex_protocol::mcp::McpServerInfo;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use codex_protocol::protocol::McpAuthStatus;
|
||||
use codex_rmcp_client::InProcessTransportFactory;
|
||||
use codex_rmcp_client::RmcpClient;
|
||||
use futures::FutureExt;
|
||||
@@ -927,6 +926,7 @@ async fn list_all_tools_uses_shared_codex_apps_cache_when_client_startup_fails()
|
||||
let failed_client = futures::future::ready::<Result<ManagedClient, StartupOutcomeError>>(Err(
|
||||
StartupOutcomeError::Failed {
|
||||
error: "startup failed".to_string(),
|
||||
is_authentication_required: false,
|
||||
},
|
||||
))
|
||||
.boxed()
|
||||
@@ -1236,7 +1236,7 @@ fn mcp_init_error_display_prompts_for_github_pat() {
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
}),
|
||||
auth_status: McpAuthStatus::Unsupported,
|
||||
auth_state: McpAuthState::Unsupported,
|
||||
};
|
||||
let err: StartupOutcomeError = anyhow::anyhow!("OAuth is unsupported").into();
|
||||
|
||||
@@ -1263,6 +1263,50 @@ fn mcp_init_error_display_prompts_for_login_when_auth_required() {
|
||||
assert_eq!(expected, display);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_startup_failure_reason_requires_existing_oauth_and_auth_failure() {
|
||||
for (auth_state, is_authentication_required, expected) in [
|
||||
(
|
||||
Some(McpAuthState::LoggedOut(
|
||||
McpLoginRequirement::Reauthentication,
|
||||
)),
|
||||
true,
|
||||
Some(McpStartupFailureReason::ReauthenticationRequired),
|
||||
),
|
||||
(
|
||||
Some(McpAuthState::LoggedOut(
|
||||
McpLoginRequirement::Reauthentication,
|
||||
)),
|
||||
false,
|
||||
None,
|
||||
),
|
||||
(
|
||||
Some(McpAuthState::LoggedOut(McpLoginRequirement::Login)),
|
||||
true,
|
||||
None,
|
||||
),
|
||||
(Some(McpAuthState::Unsupported), true, None),
|
||||
(Some(McpAuthState::BearerToken), true, None),
|
||||
(Some(McpAuthState::OAuth), true, None),
|
||||
(None, true, None),
|
||||
] {
|
||||
let entry = auth_state.map(|auth_state| McpAuthStatusEntry {
|
||||
config: None,
|
||||
auth_state,
|
||||
});
|
||||
let error = StartupOutcomeError::Failed {
|
||||
error: "startup failed".to_string(),
|
||||
is_authentication_required,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
mcp_startup_failure_reason(entry.as_ref(), &error),
|
||||
expected,
|
||||
"auth_state={auth_state:?}, is_authentication_required={is_authentication_required}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_init_error_display_reports_generic_errors() {
|
||||
let server_name = "custom";
|
||||
@@ -1290,7 +1334,7 @@ fn mcp_init_error_display_reports_generic_errors() {
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
}),
|
||||
auth_status: McpAuthStatus::Unsupported,
|
||||
auth_state: McpAuthState::Unsupported,
|
||||
};
|
||||
let err: StartupOutcomeError = anyhow::anyhow!("boom").into();
|
||||
|
||||
|
||||
@@ -9,7 +9,7 @@ use codex_config::types::AuthKeyringBackendKind;
|
||||
use codex_config::types::OAuthCredentialsStoreMode;
|
||||
use codex_exec_server::HttpClient;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_protocol::protocol::McpAuthStatus;
|
||||
use codex_rmcp_client::McpAuthState;
|
||||
use codex_rmcp_client::OAuthProviderError;
|
||||
use codex_rmcp_client::determine_streamable_http_auth_status;
|
||||
use codex_rmcp_client::determine_streamable_http_auth_status_with_http_client;
|
||||
@@ -54,7 +54,7 @@ pub struct ResolvedMcpOAuthScopes {
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct McpAuthStatusEntry {
|
||||
pub config: Option<McpServerConfig>,
|
||||
pub auth_status: McpAuthStatus,
|
||||
pub auth_state: McpAuthState,
|
||||
}
|
||||
|
||||
pub async fn oauth_login_support(transport: &McpServerTransportConfig) -> McpOAuthLoginSupport {
|
||||
@@ -208,7 +208,7 @@ where
|
||||
)
|
||||
});
|
||||
async move {
|
||||
let auth_status = match config.as_ref() {
|
||||
let auth_state = match config.as_ref() {
|
||||
Some(config) => {
|
||||
match compute_auth_status(
|
||||
&name,
|
||||
@@ -225,16 +225,13 @@ where
|
||||
warn!(
|
||||
"failed to determine auth status for MCP server `{name}`: {error:?}"
|
||||
);
|
||||
McpAuthStatus::Unsupported
|
||||
McpAuthState::Unsupported
|
||||
}
|
||||
}
|
||||
}
|
||||
None => McpAuthStatus::Unsupported,
|
||||
};
|
||||
let entry = McpAuthStatusEntry {
|
||||
config,
|
||||
auth_status,
|
||||
None => McpAuthState::Unsupported,
|
||||
};
|
||||
let entry = McpAuthStatusEntry { config, auth_state };
|
||||
(name, entry)
|
||||
}
|
||||
});
|
||||
@@ -249,17 +246,17 @@ async fn compute_auth_status(
|
||||
keyring_backend_kind: AuthKeyringBackendKind,
|
||||
has_runtime_auth: bool,
|
||||
runtime_context: &McpRuntimeContext,
|
||||
) -> Result<McpAuthStatus> {
|
||||
) -> Result<McpAuthState> {
|
||||
if !config.enabled {
|
||||
return Ok(McpAuthStatus::Unsupported);
|
||||
return Ok(McpAuthState::Unsupported);
|
||||
}
|
||||
|
||||
if has_runtime_auth {
|
||||
return Ok(McpAuthStatus::BearerToken);
|
||||
return Ok(McpAuthState::BearerToken);
|
||||
}
|
||||
|
||||
match &config.transport {
|
||||
McpServerTransportConfig::Stdio { .. } => Ok(McpAuthStatus::Unsupported),
|
||||
McpServerTransportConfig::Stdio { .. } => Ok(McpAuthState::Unsupported),
|
||||
McpServerTransportConfig::StreamableHttp {
|
||||
url,
|
||||
bearer_token_env_var,
|
||||
|
||||
@@ -567,7 +567,7 @@ fn auth_statuses_from_entries(
|
||||
) -> HashMap<String, McpAuthStatus> {
|
||||
auth_status_entries
|
||||
.iter()
|
||||
.map(|(name, entry)| (name.clone(), entry.auth_status))
|
||||
.map(|(name, entry)| (name.clone(), McpAuthStatus::from(entry.auth_state)))
|
||||
.collect::<HashMap<_, _>>()
|
||||
}
|
||||
|
||||
|
||||
@@ -63,6 +63,7 @@ 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_util::sync::CancellationToken;
|
||||
use tracing::Instrument;
|
||||
use tracing::instrument;
|
||||
@@ -304,13 +305,39 @@ pub(crate) enum StartupOutcomeError {
|
||||
// We can't store the original error here because anyhow::Error doesn't implement
|
||||
// `Clone`.
|
||||
#[error("MCP startup failed: {error}")]
|
||||
Failed { error: String },
|
||||
Failed {
|
||||
error: String,
|
||||
is_authentication_required: bool,
|
||||
},
|
||||
}
|
||||
|
||||
impl StartupOutcomeError {
|
||||
pub(crate) fn is_authentication_required(&self) -> bool {
|
||||
match self {
|
||||
Self::Cancelled => false,
|
||||
Self::Failed {
|
||||
is_authentication_required,
|
||||
..
|
||||
} => *is_authentication_required,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
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
|
||||
)
|
||||
})
|
||||
});
|
||||
Self::Failed {
|
||||
error: error.to_string(),
|
||||
is_authentication_required,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -765,6 +792,17 @@ mod tests {
|
||||
use pretty_assertions::assert_eq;
|
||||
use rmcp::model::JsonObject;
|
||||
use rmcp::model::Meta;
|
||||
use rmcp::transport::auth::AuthError;
|
||||
|
||||
#[test]
|
||||
fn startup_outcome_error_identifies_authentication_required() {
|
||||
let error = anyhow::Error::new(AuthError::AuthorizationRequired)
|
||||
.context("failed to initialize MCP server");
|
||||
|
||||
let error = StartupOutcomeError::from(error);
|
||||
|
||||
assert!(error.is_authentication_required());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_initialize_advertises_openai_form_only_when_supported() {
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::collections::HashMap;
|
||||
|
||||
fn stdio_server(command: &str, args: &[&str]) -> McpServerConfig {
|
||||
McpServerConfig {
|
||||
auth: Default::default(),
|
||||
transport: McpServerTransportConfig::Stdio {
|
||||
command: command.to_string(),
|
||||
args: args.iter().map(ToString::to_string).collect(),
|
||||
|
||||
@@ -3621,10 +3621,22 @@ pub struct McpStartupUpdateEvent {
|
||||
pub enum McpStartupStatus {
|
||||
Starting,
|
||||
Ready,
|
||||
Failed { error: String },
|
||||
Failed {
|
||||
error: String,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional = nullable)]
|
||||
reason: Option<McpStartupFailureReason>,
|
||||
},
|
||||
Cancelled,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize, JsonSchema, TS)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
#[ts(rename_all = "snake_case")]
|
||||
pub enum McpStartupFailureReason {
|
||||
ReauthenticationRequired,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS, Default)]
|
||||
pub struct McpStartupCompleteEvent {
|
||||
pub ready: Vec<String>,
|
||||
@@ -5749,6 +5761,7 @@ mod tests {
|
||||
server: "srv".to_string(),
|
||||
status: McpStartupStatus::Failed {
|
||||
error: "boom".to_string(),
|
||||
reason: Some(McpStartupFailureReason::ReauthenticationRequired),
|
||||
},
|
||||
}),
|
||||
};
|
||||
@@ -5758,6 +5771,10 @@ mod tests {
|
||||
assert_eq!(value["msg"]["server"], "srv");
|
||||
assert_eq!(value["msg"]["status"]["state"], "failed");
|
||||
assert_eq!(value["msg"]["status"]["error"], "boom");
|
||||
assert_eq!(
|
||||
value["msg"]["status"]["reason"],
|
||||
"reauthentication_required"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -28,8 +28,33 @@ pub struct StreamableHttpOAuthDiscovery {
|
||||
pub scopes_supported: Option<Vec<String>>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum McpLoginRequirement {
|
||||
Login,
|
||||
Reauthentication,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum McpAuthState {
|
||||
Unsupported,
|
||||
LoggedOut(McpLoginRequirement),
|
||||
BearerToken,
|
||||
OAuth,
|
||||
}
|
||||
|
||||
impl From<McpAuthState> for McpAuthStatus {
|
||||
fn from(value: McpAuthState) -> Self {
|
||||
match value {
|
||||
McpAuthState::Unsupported => Self::Unsupported,
|
||||
McpAuthState::LoggedOut(_) => Self::NotLoggedIn,
|
||||
McpAuthState::BearerToken => Self::BearerToken,
|
||||
McpAuthState::OAuth => Self::OAuth,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
enum AuthStatusCheck {
|
||||
Complete(McpAuthStatus),
|
||||
Complete(McpAuthState),
|
||||
Discover(HeaderMap),
|
||||
}
|
||||
|
||||
@@ -42,7 +67,7 @@ pub async fn determine_streamable_http_auth_status(
|
||||
env_http_headers: Option<HashMap<String, String>>,
|
||||
store_mode: OAuthCredentialsStoreMode,
|
||||
keyring_backend_kind: AuthKeyringBackendKind,
|
||||
) -> Result<McpAuthStatus> {
|
||||
) -> Result<McpAuthState> {
|
||||
let default_headers = match auth_status_before_discovery(
|
||||
server_name,
|
||||
url,
|
||||
@@ -75,7 +100,7 @@ pub async fn determine_streamable_http_auth_status_with_http_client(
|
||||
store_mode: OAuthCredentialsStoreMode,
|
||||
keyring_backend_kind: AuthKeyringBackendKind,
|
||||
http_client: Arc<dyn HttpClient>,
|
||||
) -> Result<McpAuthStatus> {
|
||||
) -> Result<McpAuthState> {
|
||||
let default_headers = match auth_status_before_discovery(
|
||||
server_name,
|
||||
url,
|
||||
@@ -110,20 +135,22 @@ fn auth_status_before_discovery(
|
||||
keyring_backend_kind: AuthKeyringBackendKind,
|
||||
) -> Result<AuthStatusCheck> {
|
||||
if bearer_token_env_var.is_some() {
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthStatus::BearerToken));
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthState::BearerToken));
|
||||
}
|
||||
|
||||
let default_headers = build_default_headers(http_headers, env_http_headers)?;
|
||||
if default_headers.contains_key(AUTHORIZATION) {
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthStatus::BearerToken));
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthState::BearerToken));
|
||||
}
|
||||
|
||||
match oauth_token_status(server_name, url, store_mode, keyring_backend_kind)? {
|
||||
StoredOAuthTokenStatus::Usable => {
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthStatus::OAuth));
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthState::OAuth));
|
||||
}
|
||||
StoredOAuthTokenStatus::AuthorizationRequired => {
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthStatus::NotLoggedIn));
|
||||
return Ok(AuthStatusCheck::Complete(McpAuthState::LoggedOut(
|
||||
McpLoginRequirement::Reauthentication,
|
||||
)));
|
||||
}
|
||||
StoredOAuthTokenStatus::Missing => {}
|
||||
}
|
||||
@@ -135,15 +162,15 @@ fn determine_auth_status_from_discovery(
|
||||
server_name: &str,
|
||||
url: &str,
|
||||
discovery: Result<Option<StreamableHttpOAuthDiscovery>>,
|
||||
) -> Result<McpAuthStatus> {
|
||||
) -> Result<McpAuthState> {
|
||||
match discovery {
|
||||
Ok(Some(_)) => Ok(McpAuthStatus::NotLoggedIn),
|
||||
Ok(None) => Ok(McpAuthStatus::Unsupported),
|
||||
Ok(Some(_)) => Ok(McpAuthState::LoggedOut(McpLoginRequirement::Login)),
|
||||
Ok(None) => Ok(McpAuthState::Unsupported),
|
||||
Err(error) => {
|
||||
debug!(
|
||||
"failed to detect OAuth support for MCP server `{server_name}` at {url}: {error:?}"
|
||||
);
|
||||
Ok(McpAuthStatus::Unsupported)
|
||||
Ok(McpAuthState::Unsupported)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -336,7 +363,7 @@ mod tests {
|
||||
.await
|
||||
.expect("status should compute");
|
||||
|
||||
assert_eq!(status, McpAuthStatus::BearerToken);
|
||||
assert_eq!(status, McpAuthState::BearerToken);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -358,7 +385,7 @@ mod tests {
|
||||
.await
|
||||
.expect("status should compute");
|
||||
|
||||
assert_eq!(status, McpAuthStatus::BearerToken);
|
||||
assert_eq!(status, McpAuthState::BearerToken);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -12,6 +12,8 @@ mod rmcp_client;
|
||||
mod stdio_server_launcher;
|
||||
mod utils;
|
||||
|
||||
pub use auth_status::McpAuthState;
|
||||
pub use auth_status::McpLoginRequirement;
|
||||
pub use auth_status::StreamableHttpOAuthDiscovery;
|
||||
pub use auth_status::determine_streamable_http_auth_status;
|
||||
pub use auth_status::determine_streamable_http_auth_status_with_http_client;
|
||||
|
||||
@@ -7,7 +7,8 @@ use std::time::UNIX_EPOCH;
|
||||
use codex_config::types::AuthKeyringBackendKind;
|
||||
use codex_config::types::OAuthCredentialsStoreMode;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_rmcp_client::McpAuthStatus;
|
||||
use codex_rmcp_client::McpAuthState;
|
||||
use codex_rmcp_client::McpLoginRequirement;
|
||||
use codex_rmcp_client::RmcpClient;
|
||||
use codex_rmcp_client::StoredOAuthTokens;
|
||||
use codex_rmcp_client::WrappedOAuthTokenResponse;
|
||||
@@ -146,6 +147,21 @@ async fn reports_auth_status_for_persisted_credentials() -> anyhow::Result<()> {
|
||||
#[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<()> {
|
||||
let first_login_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", first_login_server.uri()),
|
||||
"token_endpoint": format!("{}/oauth/token", first_login_server.uri()),
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&first_login_server)
|
||||
.await;
|
||||
|
||||
let status = auth_status(&format!("{}/mcp", first_login_server.uri())).await?;
|
||||
assert_eq!(status, McpAuthState::LoggedOut(McpLoginRequirement::Login));
|
||||
first_login_server.verify().await;
|
||||
|
||||
let response = OAuthTokenResponse::new(
|
||||
AccessToken::new(EXPIRED_ACCESS_TOKEN.to_string()),
|
||||
BasicTokenType::Bearer,
|
||||
@@ -166,7 +182,10 @@ async fn persisted_credentials_auth_status_child() -> anyhow::Result<()> {
|
||||
)?;
|
||||
|
||||
let status = auth_status(UNREFRESHABLE_SERVER_URL).await?;
|
||||
assert_eq!(status, McpAuthStatus::NotLoggedIn);
|
||||
assert_eq!(
|
||||
status,
|
||||
McpAuthState::LoggedOut(McpLoginRequirement::Reauthentication)
|
||||
);
|
||||
|
||||
let response = OAuthTokenResponse::new(
|
||||
AccessToken::new("unexpired-access-token".to_string()),
|
||||
@@ -192,7 +211,7 @@ async fn persisted_credentials_auth_status_child() -> anyhow::Result<()> {
|
||||
)?;
|
||||
|
||||
let status = auth_status(UNEXPIRED_SERVER_URL).await?;
|
||||
assert_eq!(status, McpAuthStatus::OAuth);
|
||||
assert_eq!(status, McpAuthState::OAuth);
|
||||
|
||||
let mut response = OAuthTokenResponse::new(
|
||||
AccessToken::new(EXPIRED_ACCESS_TOKEN.to_string()),
|
||||
@@ -215,11 +234,11 @@ async fn persisted_credentials_auth_status_child() -> anyhow::Result<()> {
|
||||
)?;
|
||||
|
||||
let status = auth_status(REFRESHABLE_SERVER_URL).await?;
|
||||
assert_eq!(status, McpAuthStatus::OAuth);
|
||||
assert_eq!(status, McpAuthState::OAuth);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn auth_status(server_url: &str) -> anyhow::Result<McpAuthStatus> {
|
||||
async fn auth_status(server_url: &str) -> anyhow::Result<McpAuthState> {
|
||||
determine_streamable_http_auth_status(
|
||||
SERVER_NAME,
|
||||
server_url,
|
||||
|
||||
@@ -285,6 +285,7 @@ mod tests {
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
failure_reason: None,
|
||||
});
|
||||
|
||||
let target = server_notification_thread_target(¬ification);
|
||||
@@ -300,6 +301,7 @@ mod tests {
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
failure_reason: None,
|
||||
});
|
||||
|
||||
let target = server_notification_thread_target(¬ification);
|
||||
|
||||
@@ -3496,6 +3496,7 @@ async fn primary_thread_ignores_child_mcp_startup_notifications() {
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
failure_reason: None,
|
||||
}),
|
||||
),
|
||||
)
|
||||
@@ -3567,6 +3568,7 @@ async fn app_scoped_mcp_startup_notifications_do_not_render_in_active_thread() {
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
failure_reason: None,
|
||||
}),
|
||||
),
|
||||
)
|
||||
@@ -3631,6 +3633,7 @@ async fn active_side_thread_renders_live_mcp_startup_notifications() {
|
||||
status,
|
||||
error: matches!(status, McpServerStartupState::Failed)
|
||||
.then(|| "sentry is not logged in".to_string()),
|
||||
failure_reason: None,
|
||||
}),
|
||||
),
|
||||
)
|
||||
|
||||
@@ -619,6 +619,7 @@ mod tests {
|
||||
name: "sentry".to_string(),
|
||||
status: codex_app_server_protocol::McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
failure_reason: None,
|
||||
},
|
||||
);
|
||||
let mut store = ThreadEventStore::new(/*capacity*/ 8);
|
||||
|
||||
@@ -8,6 +8,7 @@ fn notify_mcp_status(chat: &mut ChatWidget, name: &str, status: McpServerStartup
|
||||
name: name.to_string(),
|
||||
status,
|
||||
error: None,
|
||||
failure_reason: None,
|
||||
}),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
@@ -20,6 +21,7 @@ fn notify_mcp_status_error(chat: &mut ChatWidget, name: &str, error: &str) {
|
||||
name: name.to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some(error.to_string()),
|
||||
failure_reason: None,
|
||||
}),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
@@ -51,6 +53,7 @@ async fn mcp_startup_ignores_status_for_other_thread() {
|
||||
status,
|
||||
error: matches!(status, McpServerStartupState::Failed)
|
||||
.then(|| "sentry is not logged in".to_string()),
|
||||
failure_reason: None,
|
||||
}),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user