mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Keep MCP resource helpers available in code mode (#50546)
## Why In `CodeModeOnly`, MCP resource helper definitions changed when the first server was added or the last server was removed. ## What changed Always register `list_mcp_resources`, `list_mcp_resource_templates`, and `read_mcp_resource` in `CodeModeOnly`, even without configured MCP servers. This keeps their definitions stable across server configuration changes. ## Testing Add a regression scenario that calls all three helpers before adding a server, with a server registered, and after removing it. Assert identical tool definitions across requests, empty resource and template lists, and the expected error when reading from an unknown server. GitOrigin-RevId: c5777efac250b83dd4bb1c9100a6b6169470556f
This commit is contained in:
@@ -1113,7 +1113,9 @@ fn unified_exec_should_include_shell_parameter(
|
||||
|
||||
#[instrument(level = "trace", skip_all)]
|
||||
fn add_mcp_resource_tools(context: &CoreToolPlanContext<'_>, registry: &mut ToolRegistry) {
|
||||
if context.mcp.has_servers() {
|
||||
if context.mcp.has_servers()
|
||||
|| effective_tool_mode(context.turn_context, context.model_info) == ToolMode::CodeModeOnly
|
||||
{
|
||||
let messages = ResolvedModelMessages::from_model(context.model_info).mcp_resources();
|
||||
registry.add(ListMcpResourcesHandler::new(
|
||||
messages.and_then(|messages| messages.list_mcp_resources.as_ref()),
|
||||
|
||||
@@ -3,18 +3,25 @@
|
||||
use super::super::rmcp_client::remote_aware_environment_id;
|
||||
use super::super::rmcp_client::remote_aware_stdio_server_bin;
|
||||
use anyhow::Result;
|
||||
use codex_core::ConfigRefreshOutcome;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::openai_models::ToolMode;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::McpStartupStatus;
|
||||
use core_test_support::apps_test_server::AppsTestServer;
|
||||
use core_test_support::context_snapshot;
|
||||
use core_test_support::context_snapshot::ContextSnapshotOptions;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::skip_if_wine_exec;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_event_match;
|
||||
use core_test_support::wait_for_mcp_server;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use test_case::test_case;
|
||||
|
||||
#[test_case(ToolMode::Direct; "direct")]
|
||||
@@ -160,3 +167,98 @@ async fn mcp_resource_messages(tool_mode: ToolMode) -> Result<()> {
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn resource_helpers_across_server_changes() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
let mcp = responses::start_mock_server().await;
|
||||
// Empty tools isolate the resource helper definitions from the separate
|
||||
// deferred and MCP type guidance when the first server comes and goes.
|
||||
let empty_server =
|
||||
AppsTestServer::mount_with_tools(&mcp, Arc::new(Mutex::new(Vec::new()))).await?;
|
||||
let test = test_codex()
|
||||
.with_config(move |config| {
|
||||
super::configure_scenario_catalog(config);
|
||||
config.code_mode.disable_in_process_fallback = true;
|
||||
config.features.enable(Feature::CodeModeHost).unwrap();
|
||||
})
|
||||
.with_model_info_override("gpt-6-astra", |model| {
|
||||
model.tool_mode = Some(ToolMode::CodeModeOnly);
|
||||
model.use_responses_lite = false;
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
let script = r#"text(await tools.list_mcp_resources({}));
|
||||
text(await tools.list_mcp_resource_templates({}));
|
||||
try {
|
||||
await tools.read_mcp_resource({ server: "missing", uri: "memo://missing" });
|
||||
} catch (error) {
|
||||
text(String(error));
|
||||
}"#;
|
||||
let mut events = Vec::new();
|
||||
for phase in ["before", "registered", "removed"] {
|
||||
events.push(responses::sse(vec![
|
||||
responses::ev_custom_tool_call(phase, "exec", script),
|
||||
responses::ev_completed(phase),
|
||||
]));
|
||||
events.push(responses::sse_completed(&format!("{phase}-done")));
|
||||
}
|
||||
let mock = responses::mount_sse_sequence(&server, events).await;
|
||||
test.submit_turn("Inspect resources without any configured server.")
|
||||
.await?;
|
||||
let current = test.codex.config().await;
|
||||
let mut next = current.as_ref().clone();
|
||||
next.mcp_servers.set(HashMap::from([(
|
||||
"resources".to_string(),
|
||||
serde_json::from_value(json!({
|
||||
"url": format!("{}/api/codex/ps/mcp", empty_server.chatgpt_base_url),
|
||||
}))?,
|
||||
)]))?;
|
||||
assert_eq!(
|
||||
test.codex.refresh_mcp_config(current, next).await,
|
||||
ConfigRefreshOutcome::Published,
|
||||
);
|
||||
let startup = wait_for_event_match(&test.codex, |event| match event {
|
||||
EventMsg::McpStartupUpdate(update)
|
||||
if update.server == "resources"
|
||||
&& !matches!(update.status, McpStartupStatus::Starting) =>
|
||||
{
|
||||
Some(update.status.clone())
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
assert!(matches!(startup, McpStartupStatus::Ready), "{startup:?}");
|
||||
test.submit_turn("Inspect resources with one server.")
|
||||
.await?;
|
||||
let current = test.codex.config().await;
|
||||
let mut next = current.as_ref().clone();
|
||||
next.mcp_servers.set(HashMap::new())?;
|
||||
assert_eq!(
|
||||
test.codex.refresh_mcp_config(current, next).await,
|
||||
ConfigRefreshOutcome::Published,
|
||||
);
|
||||
test.submit_turn("Inspect resources after removing the last server.")
|
||||
.await?;
|
||||
let requests = mock.requests();
|
||||
let tools = requests
|
||||
.iter()
|
||||
.map(|request| request.body_json()["tools"].clone())
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(requests.len(), 6);
|
||||
assert_eq!(tools, vec![tools[0].clone(); requests.len()]);
|
||||
for (request, phase) in
|
||||
requests
|
||||
.iter()
|
||||
.skip(1)
|
||||
.step_by(2)
|
||||
.zip(["before", "registered", "removed"])
|
||||
{
|
||||
let output = request.custom_tool_call_output(phase).to_string();
|
||||
assert!(output.contains(r#"{\"resources\":[]}"#), "{output}");
|
||||
assert!(output.contains(r#"{\"resourceTemplates\":[]}"#), "{output}");
|
||||
assert!(output.contains("unknown MCP server 'missing'"), "{output}");
|
||||
}
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
+2
-2
@@ -6,8 +6,8 @@ Scenario: Astra starts in one workspace, switches for the next step of the activ
|
||||
|
||||
## Window 1
|
||||
-- request 1 (turn) --
|
||||
00:additional_tools/developer (3; hash=5c3ec9cbf2000b64):
|
||||
- namespace/functions; hash=e6c4aaa15e6d6f30
|
||||
00:additional_tools/developer (3; hash=34dffd584d55c8e8):
|
||||
- namespace/functions; hash=93657a45ae4a5a60
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -7,8 +7,8 @@ Scenario: Astra uses the catalog question schema to ask who a launch update is f
|
||||
|
||||
## Window 1
|
||||
-- request 1 (request) --
|
||||
00:additional_tools/developer (3; hash=f19fc129e930a86e):
|
||||
- namespace/functions; hash=b5c27b6b8551a052
|
||||
00:additional_tools/developer (3; hash=2351b38cd1e61302):
|
||||
- namespace/functions; hash=27bd0dde5ddb5182
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -6,8 +6,8 @@ Scenario: Astra receives host and handler timings on completed and failed code-m
|
||||
|
||||
## Window 1
|
||||
-- request 1 (turn) --
|
||||
00:additional_tools/developer (3; hash=10256ca2e2118704):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=fe0dd2d6b97e53b8):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -16,8 +16,8 @@ Settings:
|
||||
text: {"verbosity":"low"}
|
||||
tool_choice: "auto"
|
||||
-- request 1 (request) --
|
||||
00:additional_tools/developer (3; hash=1b07ea833720903d):
|
||||
- namespace/functions; hash=986d556577441f79
|
||||
00:additional_tools/developer (3; hash=e22abab074c3c061):
|
||||
- namespace/functions; hash=fd198a707305c309
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -6,8 +6,8 @@ Scenario: Astra receives new user input after its unfinished response is interru
|
||||
|
||||
## Window 1
|
||||
-- request 1 (request) --
|
||||
00:additional_tools/developer (3; hash=10256ca2e2118704):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=fe0dd2d6b97e53b8):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -6,8 +6,8 @@ Scenario: Astra continues after new user input yields a running code cell, then
|
||||
|
||||
## Window 1
|
||||
-- request 1 (turn) --
|
||||
00:additional_tools/developer (3; hash=7125a91d9e3d997f):
|
||||
- namespace/functions; hash=5cf23dadab8c4adb
|
||||
00:additional_tools/developer (3; hash=c42aa1aa511db58f):
|
||||
- namespace/functions; hash=b655cdad519813cb
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+4
-4
@@ -16,8 +16,8 @@ Settings:
|
||||
text: {"verbosity":"low"}
|
||||
tool_choice: "auto"
|
||||
-- request 1 (turn) --
|
||||
00:additional_tools/developer (3; hash=10256ca2e2118704):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=fe0dd2d6b97e53b8):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
@@ -187,8 +187,8 @@ Settings:
|
||||
## Window 2 (after request 3: input diverged at item 02)
|
||||
Settings: same as window 1
|
||||
-- request 4 (turn) --
|
||||
00:additional_tools/developer (3; hash=10256ca2e2118704):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=fe0dd2d6b97e53b8):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -16,8 +16,8 @@ Settings:
|
||||
text: {"verbosity":"low"}
|
||||
tool_choice: "auto"
|
||||
-- request 1 (turn) --
|
||||
00:additional_tools/developer (3; hash=10256ca2e2118704):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=fe0dd2d6b97e53b8):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+4
-4
@@ -16,8 +16,8 @@ Settings:
|
||||
text: {"verbosity":"low"}
|
||||
tool_choice: "auto"
|
||||
-- request 1 (turn; Before /compact) --
|
||||
00:additional_tools/developer (3; hash=10256ca2e2118704):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=fe0dd2d6b97e53b8):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
@@ -115,8 +115,8 @@ Settings: unavailable (items only)
|
||||
## Window 3 (after request 3: input diverged at item 00)
|
||||
Settings: same as window 1
|
||||
-- request 4 (turn; First turn after /compact) --
|
||||
00:additional_tools/developer (3; hash=10256ca2e2118704):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=fe0dd2d6b97e53b8):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+8
-8
@@ -50,8 +50,8 @@ Settings: relative to window 1
|
||||
reasoning: {"context":"all_turns","effort":"medium"}
|
||||
tools: <removed>
|
||||
-- request 4 (turn) --
|
||||
00:additional_tools/developer (1; hash=803cb4ba2a73d531):
|
||||
- namespace/functions; hash=4b4ed79c9bae9f27
|
||||
00:additional_tools/developer (1; hash=77ff9904ddb4aedd):
|
||||
- namespace/functions; hash=99a6a9ccd0de2a13
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
@@ -187,8 +187,8 @@ Settings: relative to window 2
|
||||
## Window 4 (after request 6: input diverged at item 00)
|
||||
Settings: same as window 2
|
||||
-- request 7 (turn) --
|
||||
00:additional_tools/developer (1; hash=803cb4ba2a73d531):
|
||||
- namespace/functions; hash=4b4ed79c9bae9f27
|
||||
00:additional_tools/developer (1; hash=77ff9904ddb4aedd):
|
||||
- namespace/functions; hash=99a6a9ccd0de2a13
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
@@ -216,8 +216,8 @@ Settings: same as window 2
|
||||
## Window 5 (after request 8: input diverged at item 04)
|
||||
Settings: same as window 2
|
||||
-- request 9 (turn) --
|
||||
00:additional_tools/developer (1; hash=803cb4ba2a73d531):
|
||||
- namespace/functions; hash=4b4ed79c9bae9f27
|
||||
00:additional_tools/developer (1; hash=77ff9904ddb4aedd):
|
||||
- namespace/functions; hash=99a6a9ccd0de2a13
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
@@ -340,8 +340,8 @@ Settings: same as window 3
|
||||
## Window 7 (after request 10: input diverged at item 00)
|
||||
Settings: same as window 2
|
||||
-- request 11 (turn) --
|
||||
00:additional_tools/developer (1; hash=803cb4ba2a73d531):
|
||||
- namespace/functions; hash=4b4ed79c9bae9f27
|
||||
00:additional_tools/developer (1; hash=77ff9904ddb4aedd):
|
||||
- namespace/functions; hash=99a6a9ccd0de2a13
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -16,8 +16,8 @@ Settings:
|
||||
text: {"verbosity":"low"}
|
||||
tool_choice: "auto"
|
||||
-- request 1 (turn) --
|
||||
00:additional_tools/developer (3; hash=954d592d9c1e75d8):
|
||||
- namespace/functions; hash=f01b11ec77d41982
|
||||
00:additional_tools/developer (3; hash=2c396104084747ac):
|
||||
- namespace/functions; hash=3a795ca1f0e0f332
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
+2
-2
@@ -16,8 +16,8 @@ Settings:
|
||||
text: {"verbosity":"low"}
|
||||
tool_choice: "auto"
|
||||
-- request 1 (turn) --
|
||||
00:additional_tools/developer (3; hash=a1c4ca6659851eb8):
|
||||
- namespace/functions; hash=8741ef7a7ba4ed8c
|
||||
00:additional_tools/developer (3; hash=5551438133509eec):
|
||||
- namespace/functions; hash=c60996290e7ef438
|
||||
- custom/exec
|
||||
- function/wait
|
||||
- function/request_user_input
|
||||
|
||||
Reference in New Issue
Block a user