mirror of
https://github.com/openai/codex.git
synced 2026-11-03 16:23:51 +00:00
Include call IDs in MCP requests and clarify metadata config (#37477)
## What changed - Add the tool call ID to `_meta.callId` for every MCP tool request. - Rename `features.tool_registry.include_tool_metadata` to `features.tool_registry.turn_metadata_includes_tool_info` to clarify that the setting controls authoritative tool information in per-turn metadata. - Update config parsing, schema generation, and session config locking for the renamed setting. ## Testing - Cover `callId` metadata for custom, plugin, and Codex Apps MCP calls. - Update strict-config, config-loading, feature-config, and config-lock tests for the renamed setting. GitOrigin-RevId: ac0e58c489b03bd9b55a356f545de86c8ecaf865
This commit is contained in:
1 parent
5729546839
commit
248d8c0e22
10 files changed
+37
-23
No files matched your search
@@ -95,8 +95,8 @@ fn strict_config_accepts_tool_registry_config() {
|
||||
for contents in [
|
||||
"[features.tool_registry]\nerror_on_tool_collisions = true\n",
|
||||
"[profiles.work.features.tool_registry]\nerror_on_tool_collisions = true\n",
|
||||
"[features.tool_registry]\ninclude_tool_metadata = true\n",
|
||||
"[profiles.work.features.tool_registry]\ninclude_tool_metadata = true\n",
|
||||
"[features.tool_registry]\nturn_metadata_includes_tool_info = true\n",
|
||||
"[profiles.work.features.tool_registry]\nturn_metadata_includes_tool_info = true\n",
|
||||
] {
|
||||
assert_eq!(
|
||||
config_error_from_ignored_toml_fields::<ConfigToml>(path, contents),
|
||||
|
||||
@@ -3219,8 +3219,8 @@
|
||||
"description": "Fail the turn when multiple tools share the same effective name.",
|
||||
"type": "boolean"
|
||||
},
|
||||
"include_tool_metadata": {
|
||||
"description": "Include authoritative tool information in request metadata.",
|
||||
"turn_metadata_includes_tool_info": {
|
||||
"description": "Include authoritative tool information in per-turn request metadata.",
|
||||
"type": "boolean"
|
||||
}
|
||||
},
|
||||
@@ -5895,4 +5895,4 @@
|
||||
},
|
||||
"title": "ConfigToml",
|
||||
"type": "object"
|
||||
}
|
||||
}
|
||||
@@ -574,7 +574,7 @@ disable_in_process_fallback = true
|
||||
async fn load_config_resolves_tool_registry_config() -> std::io::Result<()> {
|
||||
let codex_home = tempdir()?;
|
||||
|
||||
for (config_toml, error_on_tool_collisions, include_tool_metadata) in [
|
||||
for (config_toml, error_on_tool_collisions, turn_metadata_includes_tool_info) in [
|
||||
("", false, false),
|
||||
(
|
||||
"[features.tool_registry]\nerror_on_tool_collisions = true\n",
|
||||
@@ -582,7 +582,7 @@ async fn load_config_resolves_tool_registry_config() -> std::io::Result<()> {
|
||||
false,
|
||||
),
|
||||
(
|
||||
"[features.tool_registry]\ninclude_tool_metadata = true\n",
|
||||
"[features.tool_registry]\nturn_metadata_includes_tool_info = true\n",
|
||||
false,
|
||||
true,
|
||||
),
|
||||
@@ -601,8 +601,8 @@ async fn load_config_resolves_tool_registry_config() -> std::io::Result<()> {
|
||||
error_on_tool_collisions
|
||||
);
|
||||
assert_eq!(
|
||||
config.tool_registry.include_tool_metadata,
|
||||
include_tool_metadata
|
||||
config.tool_registry.turn_metadata_includes_tool_info,
|
||||
turn_metadata_includes_tool_info
|
||||
);
|
||||
assert!(!config.features.enabled(Feature::CodeMode));
|
||||
}
|
||||
|
||||
@@ -1119,8 +1119,8 @@ pub struct Config {
|
||||
pub struct ToolRegistryConfig {
|
||||
/// Fail the turn when multiple tools share the same effective name.
|
||||
pub error_on_tool_collisions: bool,
|
||||
/// Include authoritative tool information in request metadata.
|
||||
pub include_tool_metadata: bool,
|
||||
/// Include authoritative tool information in per-turn request metadata.
|
||||
pub turn_metadata_includes_tool_info: bool,
|
||||
}
|
||||
|
||||
const DEFAULT_CODE_MODE_EXEC_YIELD_TIME_MS: u64 = 30_000;
|
||||
@@ -3731,11 +3731,11 @@ impl Config {
|
||||
.and_then(|features| features.tool_registry.as_ref())
|
||||
.and_then(|config| config.error_on_tool_collisions)
|
||||
.unwrap_or_default(),
|
||||
include_tool_metadata: cfg
|
||||
turn_metadata_includes_tool_info: cfg
|
||||
.features
|
||||
.as_ref()
|
||||
.and_then(|features| features.tool_registry.as_ref())
|
||||
.and_then(|config| config.include_tool_metadata)
|
||||
.and_then(|config| config.turn_metadata_includes_tool_info)
|
||||
.unwrap_or_default(),
|
||||
};
|
||||
let code_mode = resolve_code_mode_config(&cfg);
|
||||
|
||||
@@ -1161,6 +1161,10 @@ fn build_mcp_tool_call_request_meta(
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
) -> Option<serde_json::Value> {
|
||||
let mut request_meta = serde_json::Map::new();
|
||||
request_meta.insert(
|
||||
"callId".to_string(),
|
||||
serde_json::Value::String(call_id.to_string()),
|
||||
);
|
||||
|
||||
if let Some(turn_metadata) = turn_context
|
||||
.turn_metadata_state
|
||||
|
||||
@@ -1052,6 +1052,7 @@ async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() {
|
||||
assert_eq!(
|
||||
meta,
|
||||
serde_json::json!({
|
||||
"callId": "call-custom",
|
||||
crate::X_CODEX_TURN_METADATA_HEADER: expected_turn_metadata,
|
||||
})
|
||||
);
|
||||
@@ -1147,6 +1148,7 @@ async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() {
|
||||
assert_eq!(
|
||||
build_mcp_tool_call_request_meta(&turn_context, "sample", "call-plugin", Some(&metadata),),
|
||||
Some(serde_json::json!({
|
||||
"callId": "call-plugin",
|
||||
crate::X_CODEX_TURN_METADATA_HEADER: expected_turn_metadata,
|
||||
MCP_TOOL_PLUGIN_ID_META_KEY: "sample@test",
|
||||
}))
|
||||
@@ -1292,6 +1294,7 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps
|
||||
Some(&metadata),
|
||||
),
|
||||
Some(serde_json::json!({
|
||||
"callId": "call_abc123xyz789",
|
||||
crate::X_CODEX_TURN_METADATA_HEADER: expected_turn_metadata,
|
||||
MCP_TOOL_CODEX_APPS_META_KEY: {
|
||||
"call_id": "call_abc123xyz789",
|
||||
@@ -1319,6 +1322,7 @@ async fn codex_apps_tool_call_request_meta_includes_call_id_without_existing_cod
|
||||
/*metadata*/ None,
|
||||
),
|
||||
Some(serde_json::json!({
|
||||
"callId": "call_abc123xyz789",
|
||||
crate::X_CODEX_TURN_METADATA_HEADER: expected_turn_metadata,
|
||||
MCP_TOOL_CODEX_APPS_META_KEY: {
|
||||
"call_id": "call_abc123xyz789",
|
||||
|
||||
@@ -164,12 +164,14 @@ fn save_config_resolved_fields(
|
||||
.get_or_insert_with(FeaturesToml::default);
|
||||
features.materialize_resolved_enabled(config.features.get());
|
||||
if config.tool_registry.error_on_tool_collisions
|
||||
|| config.tool_registry.include_tool_metadata
|
||||
|| config.tool_registry.turn_metadata_includes_tool_info
|
||||
|| features.tool_registry.is_some()
|
||||
{
|
||||
features.tool_registry = Some(ToolRegistryConfigToml {
|
||||
error_on_tool_collisions: Some(config.tool_registry.error_on_tool_collisions),
|
||||
include_tool_metadata: Some(config.tool_registry.include_tool_metadata),
|
||||
turn_metadata_includes_tool_info: Some(
|
||||
config.tool_registry.turn_metadata_includes_tool_info,
|
||||
),
|
||||
});
|
||||
}
|
||||
let mut multi_agent_v2: MultiAgentV2ConfigToml =
|
||||
@@ -282,7 +284,7 @@ mod tests {
|
||||
let mut sc = crate::session::tests::make_session_configuration_for_tests().await;
|
||||
let mut config = (*sc.original_config_do_not_use).clone();
|
||||
config.tool_registry.error_on_tool_collisions = true;
|
||||
config.tool_registry.include_tool_metadata = true;
|
||||
config.tool_registry.turn_metadata_includes_tool_info = true;
|
||||
config.multi_agent_v2.subagent_developer_instructions =
|
||||
Some("Locked subagent developer instructions.".to_string());
|
||||
config.token_budget = Some(crate::config::TokenBudgetConfig {
|
||||
@@ -343,7 +345,7 @@ mod tests {
|
||||
features.tool_registry,
|
||||
Some(ToolRegistryConfigToml {
|
||||
error_on_tool_collisions: Some(true),
|
||||
include_tool_metadata: Some(true),
|
||||
turn_metadata_includes_tool_info: Some(true),
|
||||
})
|
||||
);
|
||||
let feature_entries = features.entries();
|
||||
|
||||
@@ -247,6 +247,10 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R
|
||||
assert_eq!(mock.requests().len(), 2);
|
||||
let apps_tool_call = recorded_apps_tool_call_by_call_id(&server, call_id).await;
|
||||
|
||||
assert_eq!(
|
||||
apps_tool_call.pointer("/params/_meta/callId"),
|
||||
Some(&json!(call_id))
|
||||
);
|
||||
assert_eq!(
|
||||
apps_tool_call
|
||||
.pointer("/params/_meta/x-codex-turn-metadata/user_input_requested_during_turn"),
|
||||
|
||||
@@ -10,9 +10,9 @@ pub struct ToolRegistryConfigToml {
|
||||
/// Fail the turn when multiple tools share the same effective name.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub error_on_tool_collisions: Option<bool>,
|
||||
/// Include authoritative tool information in request metadata.
|
||||
/// Include authoritative tool information in per-turn request metadata.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub include_tool_metadata: Option<bool>,
|
||||
pub turn_metadata_includes_tool_info: Option<bool>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)]
|
||||
|
||||
@@ -30,7 +30,7 @@ fn under_development_features_are_disabled_by_default() {
|
||||
#[test]
|
||||
fn tool_registry_config_is_not_a_feature_toggle() {
|
||||
let features: FeaturesToml = toml::from_str(
|
||||
"[tool_registry]\nerror_on_tool_collisions = true\ninclude_tool_metadata = true\n",
|
||||
"[tool_registry]\nerror_on_tool_collisions = true\nturn_metadata_includes_tool_info = true\n",
|
||||
)
|
||||
.expect("tool registry settings should deserialize");
|
||||
|
||||
@@ -38,7 +38,7 @@ fn tool_registry_config_is_not_a_feature_toggle() {
|
||||
features.tool_registry,
|
||||
Some(crate::ToolRegistryConfigToml {
|
||||
error_on_tool_collisions: Some(true),
|
||||
include_tool_metadata: Some(true),
|
||||
turn_metadata_includes_tool_info: Some(true),
|
||||
})
|
||||
);
|
||||
assert!(features.entries().is_empty());
|
||||
@@ -554,7 +554,7 @@ fn materialize_resolved_enabled_writes_all_features_and_preserves_custom_config(
|
||||
let mut features_toml = FeaturesToml {
|
||||
tool_registry: Some(crate::ToolRegistryConfigToml {
|
||||
error_on_tool_collisions: Some(true),
|
||||
include_tool_metadata: Some(true),
|
||||
turn_metadata_includes_tool_info: Some(true),
|
||||
}),
|
||||
code_mode_host: Some(FeatureToml::Config(crate::CodeModeHostConfigToml {
|
||||
enabled: Some(false),
|
||||
@@ -587,7 +587,7 @@ fn materialize_resolved_enabled_writes_all_features_and_preserves_custom_config(
|
||||
features_toml.tool_registry,
|
||||
Some(crate::ToolRegistryConfigToml {
|
||||
error_on_tool_collisions: Some(true),
|
||||
include_tool_metadata: Some(true),
|
||||
turn_metadata_includes_tool_info: Some(true),
|
||||
})
|
||||
);
|
||||
let entries = features_toml.entries();
|
||||
|
||||
Reference in new issue
Block a user