From cc31e374fc15cd616fca19c07d55f9fa7e6d7e31 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 1 Oct 2026 16:59:31 +0000 Subject: [PATCH] Follow MCP tool pagination in legacy protocol mode (#50035) ## Why Legacy MCP tool discovery discarded pagination cursors, leaving tools on later pages unavailable. ## What changed Pass `tools/list` pagination cursors to the existing collector in both legacy and modern protocol modes. Legacy discovery now follows subsequent pages and applies the same pagination limits and cursor validation as modern discovery. ## Testing Extend integration coverage to both protocol modes, checking that tools from later pages reach the model and can be called successfully, while catalogs with oversized cursors are rejected. Update the legacy discovery test to expect an error for a repeated cursor. GitOrigin-RevId: a6daa8034d2a403c1f2a5846e385789d8a620af7 --- .../codex-mcp/src/connection_manager_tests.rs | 15 +++--- codex-rs/codex-mcp/src/rmcp_client.rs | 7 +-- codex-rs/core/tests/suite/rmcp_client.rs | 54 +++++++++++-------- 3 files changed, 43 insertions(+), 33 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 015240507b..52be4fe101 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -412,7 +412,7 @@ impl InProcessTransportFactory for DisconnectingToolsTransportFactory { } #[tokio::test] -async fn legacy_tool_catalog_does_not_follow_pagination_cursor() -> anyhow::Result<()> { +async fn legacy_tool_catalog_rejects_repeated_pagination_cursor() -> anyhow::Result<()> { let requests = Arc::new(AtomicUsize::new(0)); let client = Arc::new( RmcpClient::new_in_process_client(Arc::new(RefreshTestTransportFactory { @@ -436,7 +436,7 @@ async fn legacy_tool_catalog_does_not_follow_pagination_cursor() -> anyhow::Resu ) .await?; - let tools = list_tools_for_client_uncached( + let error = list_tools_for_client_uncached( "legacy", /*is_codex_apps_mcp_server*/ false, "test", @@ -445,11 +445,14 @@ async fn legacy_tool_catalog_does_not_follow_pagination_cursor() -> anyhow::Resu crate::pagination::MAX_MCP_CATALOG_ITEMS, /*server_instructions*/ None, ) - .await?; + .await + .expect_err("repeated pagination cursor must fail discovery"); - assert_eq!(tools.len(), 1); - assert_eq!(tools[0].tool.name.as_ref(), "first-page"); - assert_eq!(requests.load(std::sync::atomic::Ordering::SeqCst), 1); + assert_eq!( + error.to_string(), + "tools/list returned a repeated pagination cursor" + ); + assert_eq!(requests.load(std::sync::atomic::Ordering::SeqCst), 2); client.shutdown().await; Ok(()) } diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index 6d1d90b74c..37089a380a 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -667,18 +667,13 @@ pub(crate) async fn list_tools_for_client_uncached( server_instructions: Option<&str>, ) -> Result> { let fetch_start = Instant::now(); - let protocol_mode = client.protocol_mode(); let tools = collect_paginated_with_limit("tools/list", timeout, catalog_item_limit, |params| { let client = Arc::clone(client); async move { let response = client .list_tools_with_connector_ids(params, timeout) .await?; - let next_cursor = match protocol_mode { - McpProtocolMode::Legacy => None, - McpProtocolMode::V20260728 => response.next_cursor, - }; - Ok((response.tools, next_cursor)) + Ok((response.tools, response.next_cursor)) } }) .await? diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 686a781f00..577b819806 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -38,6 +38,7 @@ use codex_http_client::HttpClientBuilder; use codex_login::CodexAuth; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::MCP_SANDBOX_STATE_META_CAPABILITY; +use codex_mcp::McpProtocolMode; use codex_mcp::SandboxState; use codex_models_manager::manager::RefreshStrategy; use codex_utils_path_uri::LegacyAppPathString; @@ -1367,9 +1368,12 @@ server_names = ["history", "notes"] Ok(()) } +#[test_case(McpProtocolMode::Legacy; "legacy")] +#[test_case(McpProtocolMode::V20260728; "modern")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn modern_mcp_pagination_preserves_valid_tools_and_rejects_oversized_cursors() --> anyhow::Result<()> { +async fn mcp_pagination_preserves_valid_tools_and_rejects_oversized_cursors( + protocol_mode: McpProtocolMode, +) -> anyhow::Result<()> { skip_if_wine_exec!( Ok(()), "requires a Windows test_stdio_server in the Wine-exec environment" @@ -1377,43 +1381,45 @@ async fn modern_mcp_pagination_preserves_valid_tools_and_rejects_oversized_curso skip_if_no_network!(Ok(())); let server = responses::start_mock_server().await; + let call_id = "call-second-page-tool"; let response = mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_assistant_message("msg-1", "done"), + responses::ev_function_call_with_namespace(call_id, "mcp__paginated", "sync", "{}"), responses::ev_completed("resp-1"), ]), ) .await; + let final_mock = mount_sse_once(&server, responses::sse_completed("resp-2")).await; let command = remote_aware_stdio_server_bin()?; let fixture = test_codex() .with_model_info_override("gpt-5.4", |model| model.supports_search_tool = false) .with_config(move |config| { config .features - .enable(Feature::Mcp20260728) - .expect("test config should allow modern MCP"); + .set_enabled( + Feature::Mcp20260728, + protocol_mode == McpProtocolMode::V20260728, + ) + .expect("test config should allow MCP protocol selection"); for (server_name, pagination) in [("paginated", "two-pages"), ("rejected", "oversized-cursor")] { + let mut env = HashMap::from([( + "MCP_TEST_TOOL_PAGINATION".to_string(), + pagination.to_string(), + )]); + if protocol_mode == McpProtocolMode::V20260728 { + env.insert( + "CODEX_MCP_PROTOCOL_VERSION".to_string(), + "2026-07-28".to_string(), + ); + } insert_mcp_server( config, server_name, - stdio_transport( - command.clone(), - Some(HashMap::from([ - ( - "CODEX_MCP_PROTOCOL_VERSION".to_string(), - "2026-07-28".to_string(), - ), - ( - "MCP_TEST_TOOL_PAGINATION".to_string(), - pagination.to_string(), - ), - ])), - Vec::new(), - ), + stdio_transport(command.clone(), Some(env), Vec::new()), TestMcpServerOptions { environment_id: remote_aware_environment_id(), ..Default::default() @@ -1446,9 +1452,9 @@ async fn modern_mcp_pagination_preserves_valid_tools_and_rejects_oversized_curso fixture .codex - .start_or_steer_turn(read_only_user_turn( + .start_or_steer_turn(auto_approved_user_turn( &fixture, - "show the paginated MCP tools", + "call the paginated sync tool", )) .await?; wait_for_event(&fixture.codex, |event| { @@ -1467,6 +1473,12 @@ async fn modern_mcp_pagination_preserves_valid_tools_and_rejects_oversized_curso responses::namespace_child_tool(&body, "mcp__rejected", "echo").is_none(), "a rejected MCP catalog must not reach the model" ); + let output = final_mock.single_request().function_call_output(call_id); + let output_text = output["output"] + .as_str() + .unwrap_or_else(|| panic!("expected successful MCP tool output, got {output}")); + let output_json: Value = serde_json::from_str(split_wall_time_wrapped_output(output_text))?; + assert_eq!(output_json, json!({"result": "ok"})); Ok(()) }