Skip to content

Commit eead542

Browse files
gpealHolovkat
authored andcommitted
[MCP] Dedicated error message for GitHub MCPs missing a personal access token (#5393)
Because the GitHub MCP is one of the most popular MCPs and it confusingly doesn't support OAuth, we should make it more clear how to make it work so people don't think Codex is broken.
1 parent 10245b4 commit eead542

File tree

3 files changed

+133
-24
lines changed

3 files changed

+133
-24
lines changed

codex-rs/cli/src/mcp_cmd.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
402402
.map(|(name, cfg)| {
403403
let auth_status = auth_statuses
404404
.get(name.as_str())
405-
.copied()
405+
.map(|entry| entry.auth_status)
406406
.unwrap_or(McpAuthStatus::Unsupported);
407407
let transport = match &cfg.transport {
408408
McpServerTransportConfig::Stdio {
@@ -489,7 +489,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
489489
};
490490
let auth_status = auth_statuses
491491
.get(name.as_str())
492-
.copied()
492+
.map(|entry| entry.auth_status)
493493
.unwrap_or(McpAuthStatus::Unsupported)
494494
.to_string();
495495
stdio_rows.push([
@@ -514,7 +514,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
514514
};
515515
let auth_status = auth_statuses
516516
.get(name.as_str())
517-
.copied()
517+
.map(|entry| entry.auth_status)
518518
.unwrap_or(McpAuthStatus::Unsupported)
519519
.to_string();
520520
http_rows.push([

codex-rs/core/src/codex.rs

Lines changed: 117 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use std::borrow::Cow;
2-
use std::collections::HashSet;
2+
use std::collections::{HashMap, HashSet};
33
use std::fmt::Debug;
44
use std::path::PathBuf;
55
use std::sync::Arc;
@@ -9,6 +9,7 @@ use crate::AuthManager;
99
use crate::client_common::REVIEW_PROMPT;
1010
use crate::event_mapping::map_response_item_to_event_messages;
1111
use crate::function_tool::FunctionCallError;
12+
use crate::mcp::auth::McpAuthStatusEntry;
1213
use crate::parse_command::parse_command;
1314
use crate::review_format::format_review_findings_block;
1415
use crate::state::ItemCollector;
@@ -58,6 +59,7 @@ use crate::client::ModelClient;
5859
use crate::client_common::Prompt;
5960
use crate::client_common::ResponseEvent;
6061
use crate::config::Config;
62+
use crate::config_types::McpServerTransportConfig;
6163
use crate::config_types::ShellEnvironmentPolicy;
6264
use crate::conversation_history::ConversationHistory;
6365
use crate::environment_context::EnvironmentContext;
@@ -525,22 +527,9 @@ impl Session {
525527
// Surface individual client start-up failures to the user.
526528
if !failed_clients.is_empty() {
527529
for (server_name, err) in failed_clients {
528-
let auth_status = auth_statuses.get(&server_name);
529-
let requires_login = match auth_status {
530-
Some(McpAuthStatus::NotLoggedIn) => true,
531-
Some(McpAuthStatus::OAuth) => is_mcp_client_auth_required_error(&err),
532-
_ => false,
533-
};
534-
let log_message =
535-
format!("MCP client for `{server_name}` failed to start: {err:#}");
536-
error!("{log_message}");
537-
let display_message = if requires_login {
538-
format!(
539-
"The {server_name} MCP server is not logged in. Run `codex mcp login {server_name}` to log in."
540-
)
541-
} else {
542-
log_message
543-
};
530+
let auth_entry = auth_statuses.get(&server_name);
531+
let display_message = mcp_init_error_display(&server_name, auth_entry, &err);
532+
warn!("MCP client for `{server_name}` failed to start: {err:#}");
544533
post_session_configured_error_events.push(Event {
545534
id: INITIAL_SUBMIT_ID.to_owned(),
546535
msg: EventMsg::Error(ErrorEvent {
@@ -1357,7 +1346,7 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
13571346

13581347
// This is a cheap lookup from the connection manager's cache.
13591348
let tools = sess.services.mcp_connection_manager.list_all_tools();
1360-
let (auth_statuses, resources, resource_templates) = tokio::join!(
1349+
let (auth_status_entries, resources, resource_templates) = tokio::join!(
13611350
compute_auth_statuses(
13621351
config.mcp_servers.iter(),
13631352
config.mcp_oauth_credentials_store_mode,
@@ -1367,6 +1356,10 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
13671356
.mcp_connection_manager
13681357
.list_all_resource_templates()
13691358
);
1359+
let auth_statuses = auth_status_entries
1360+
.iter()
1361+
.map(|(name, entry)| (name.clone(), entry.auth_status))
1362+
.collect();
13701363
let event = Event {
13711364
id: sub_id,
13721365
msg: EventMsg::McpListToolsResponse(
@@ -2530,6 +2523,36 @@ pub(crate) async fn exit_review_mode(
25302523
.await;
25312524
}
25322525

2526+
fn mcp_init_error_display(
2527+
server_name: &str,
2528+
entry: Option<&McpAuthStatusEntry>,
2529+
err: &anyhow::Error,
2530+
) -> String {
2531+
if let Some(McpServerTransportConfig::StreamableHttp {
2532+
url,
2533+
bearer_token_env_var,
2534+
http_headers,
2535+
..
2536+
}) = &entry.map(|entry| &entry.config.transport)
2537+
&& url == "https://api.githubcopilot.com/mcp/"
2538+
&& bearer_token_env_var.is_none()
2539+
&& http_headers.as_ref().map(HashMap::is_empty).unwrap_or(true)
2540+
{
2541+
// GitHub only supports OAUth for first party MCP clients.
2542+
// That means that the user has to specify a personal access token either via bearer_token_env_var or http_headers.
2543+
// https://github.com/github/github-mcp-server/issues/921#issuecomment-3221026448
2544+
format!(
2545+
"GitHub MCP does not support OAuth. Log in by adding `bearer_token_env_var = CODEX_GITHUB_PAT` in the `mcp_servers.{server_name}` section of your config.toml"
2546+
)
2547+
} else if is_mcp_client_auth_required_error(err) {
2548+
format!(
2549+
"The {server_name} MCP server is not logged in. Run `codex mcp login {server_name}`."
2550+
)
2551+
} else {
2552+
format!("MCP client for `{server_name}` failed to start: {err:#}")
2553+
}
2554+
}
2555+
25332556
fn is_mcp_client_auth_required_error(error: &anyhow::Error) -> bool {
25342557
// StreamableHttpError::AuthRequired from the MCP SDK.
25352558
error.to_string().contains("Auth required")
@@ -2543,7 +2566,10 @@ mod tests {
25432566
use super::*;
25442567
use crate::config::ConfigOverrides;
25452568
use crate::config::ConfigToml;
2569+
use crate::config_types::McpServerConfig;
2570+
use crate::config_types::McpServerTransportConfig;
25462571
use crate::exec::ExecToolCallOutput;
2572+
use crate::mcp::auth::McpAuthStatusEntry;
25472573
use crate::tools::format_exec_output_str;
25482574

25492575
use crate::protocol::CompactedItem;
@@ -2562,6 +2588,7 @@ mod tests {
25622588
use codex_app_server_protocol::AuthMode;
25632589
use codex_protocol::models::ContentItem;
25642590
use codex_protocol::models::ResponseItem;
2591+
use codex_protocol::protocol::McpAuthStatus;
25652592
use std::time::Duration;
25662593
use tokio::time::sleep;
25672594

@@ -3351,4 +3378,76 @@ mod tests {
33513378
pretty_assertions::assert_eq!(exec_output.metadata, ResponseExecMetadata { exit_code: 0 });
33523379
assert!(exec_output.output.contains("hi"));
33533380
}
3381+
3382+
#[test]
3383+
fn mcp_init_error_display_prompts_for_github_pat() {
3384+
let server_name = "github";
3385+
let entry = McpAuthStatusEntry {
3386+
config: McpServerConfig {
3387+
transport: McpServerTransportConfig::StreamableHttp {
3388+
url: "https://api.githubcopilot.com/mcp/".to_string(),
3389+
bearer_token_env_var: None,
3390+
http_headers: None,
3391+
env_http_headers: None,
3392+
},
3393+
enabled: true,
3394+
startup_timeout_sec: None,
3395+
tool_timeout_sec: None,
3396+
enabled_tools: None,
3397+
disabled_tools: None,
3398+
},
3399+
auth_status: McpAuthStatus::Unsupported,
3400+
};
3401+
let err = anyhow::anyhow!("OAuth is unsupported");
3402+
3403+
let display = mcp_init_error_display(server_name, Some(&entry), &err);
3404+
3405+
let expected = format!(
3406+
"GitHub MCP does not support OAuth. Log in by adding `bearer_token_env_var = CODEX_GITHUB_PAT` in the `mcp_servers.{server_name}` section of your config.toml"
3407+
);
3408+
3409+
assert_eq!(expected, display);
3410+
}
3411+
3412+
#[test]
3413+
fn mcp_init_error_display_prompts_for_login_when_auth_required() {
3414+
let server_name = "example";
3415+
let err = anyhow::anyhow!("Auth required for server");
3416+
3417+
let display = mcp_init_error_display(server_name, None, &err);
3418+
3419+
let expected = format!(
3420+
"The {server_name} MCP server is not logged in. Run `codex mcp login {server_name}`."
3421+
);
3422+
3423+
assert_eq!(expected, display);
3424+
}
3425+
3426+
#[test]
3427+
fn mcp_init_error_display_reports_generic_errors() {
3428+
let server_name = "custom";
3429+
let entry = McpAuthStatusEntry {
3430+
config: McpServerConfig {
3431+
transport: McpServerTransportConfig::StreamableHttp {
3432+
url: "https://example.com".to_string(),
3433+
bearer_token_env_var: Some("TOKEN".to_string()),
3434+
http_headers: None,
3435+
env_http_headers: None,
3436+
},
3437+
enabled: true,
3438+
startup_timeout_sec: None,
3439+
tool_timeout_sec: None,
3440+
enabled_tools: None,
3441+
disabled_tools: None,
3442+
},
3443+
auth_status: McpAuthStatus::Unsupported,
3444+
};
3445+
let err = anyhow::anyhow!("boom");
3446+
3447+
let display = mcp_init_error_display(server_name, Some(&entry), &err);
3448+
3449+
let expected = format!("MCP client for `{server_name}` failed to start: {err:#}");
3450+
3451+
assert_eq!(expected, display);
3452+
}
33543453
}

codex-rs/core/src/mcp/auth.rs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,25 +10,35 @@ use tracing::warn;
1010
use crate::config_types::McpServerConfig;
1111
use crate::config_types::McpServerTransportConfig;
1212

13+
#[derive(Debug, Clone)]
14+
pub struct McpAuthStatusEntry {
15+
pub config: McpServerConfig,
16+
pub auth_status: McpAuthStatus,
17+
}
18+
1319
pub async fn compute_auth_statuses<'a, I>(
1420
servers: I,
1521
store_mode: OAuthCredentialsStoreMode,
16-
) -> HashMap<String, McpAuthStatus>
22+
) -> HashMap<String, McpAuthStatusEntry>
1723
where
1824
I: IntoIterator<Item = (&'a String, &'a McpServerConfig)>,
1925
{
2026
let futures = servers.into_iter().map(|(name, config)| {
2127
let name = name.clone();
2228
let config = config.clone();
2329
async move {
24-
let status = match compute_auth_status(&name, &config, store_mode).await {
30+
let auth_status = match compute_auth_status(&name, &config, store_mode).await {
2531
Ok(status) => status,
2632
Err(error) => {
2733
warn!("failed to determine auth status for MCP server `{name}`: {error:?}");
2834
McpAuthStatus::Unsupported
2935
}
3036
};
31-
(name, status)
37+
let entry = McpAuthStatusEntry {
38+
config,
39+
auth_status,
40+
};
41+
(name, entry)
3242
}
3343
});
3444

0 commit comments

Comments
 (0)