mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Use step settings for MCP request metadata (#40807)
## Why MCP tool calls can be issued by a model step whose settings differ from the turn's initial settings. Request metadata must describe the issuing step. ## What changed - Build MCP tool-call metadata from the captured `StepContext` model and its effective reasoning effort. - Populate `node_repl_disabled` from the issuing step while leaving turn-owned Responses metadata unchanged. - Apply the same step-scoped values to MCP requests emitted by stop hooks. ## Testing Add coverage for configured and model-default reasoning effort, model changes, Node REPL restrictions, and preservation of the original step and turn metadata. GitOrigin-RevId: f9cce52fbe559f15437bd19d699b657229c0d145
This commit is contained in:
1 parent
3e4707b34b
commit
04907ab95a
8 files changed
+98
-14
No files matched your search
@@ -367,7 +367,8 @@ pub(crate) async fn run_turn_stop_hooks(
|
|||||||
.turn_metadata_state
|
.turn_metadata_state
|
||||||
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
||||||
model: step_context.settings.model_info.slug.as_str(),
|
model: step_context.settings.model_info.slug.as_str(),
|
||||||
reasoning_effort: step_context.settings.reasoning_effort().cloned(),
|
reasoning_effort: step_context.settings.effective_reasoning_effort(),
|
||||||
|
node_repl_disabled: step_context.settings.model_info.node_repl_disabled,
|
||||||
})
|
})
|
||||||
.map(|turn_metadata| {
|
.map(|turn_metadata| {
|
||||||
serde_json::Map::from_iter([(
|
serde_json::Map::from_iter([(
|
||||||
|
|||||||
@@ -466,7 +466,7 @@ async fn handle_approved_mcp_tool_call(
|
|||||||
tool_input = rewritten_arguments.clone();
|
tool_input = rewritten_arguments.clone();
|
||||||
}
|
}
|
||||||
let request_meta = build_mcp_tool_call_request_meta(
|
let request_meta = build_mcp_tool_call_request_meta(
|
||||||
turn_context,
|
step_context,
|
||||||
&server,
|
&server,
|
||||||
call_id,
|
call_id,
|
||||||
Some(&metadata),
|
Some(&metadata),
|
||||||
@@ -1180,7 +1180,7 @@ async fn custom_mcp_tool_approval_mode(
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn build_mcp_tool_call_request_meta(
|
fn build_mcp_tool_call_request_meta(
|
||||||
turn_context: &TurnContext,
|
step_context: &StepContext,
|
||||||
server: &str,
|
server: &str,
|
||||||
call_id: &str,
|
call_id: &str,
|
||||||
metadata: Option<&McpToolApprovalMetadata>,
|
metadata: Option<&McpToolApprovalMetadata>,
|
||||||
@@ -1191,11 +1191,13 @@ fn build_mcp_tool_call_request_meta(
|
|||||||
serde_json::Value::String(call_id.to_string()),
|
serde_json::Value::String(call_id.to_string()),
|
||||||
);
|
);
|
||||||
|
|
||||||
if let Some(turn_metadata) = turn_context
|
if let Some(turn_metadata) = step_context
|
||||||
|
.turn
|
||||||
.turn_metadata_state
|
.turn_metadata_state
|
||||||
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
||||||
model: turn_context.model_info().slug.as_str(),
|
model: step_context.settings.model_info.slug.as_str(),
|
||||||
reasoning_effort: turn_context.effective_reasoning_effort(),
|
reasoning_effort: step_context.settings.effective_reasoning_effort(),
|
||||||
|
node_repl_disabled: step_context.settings.model_info.node_repl_disabled,
|
||||||
})
|
})
|
||||||
{
|
{
|
||||||
request_meta.insert(
|
request_meta.insert(
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ use crate::session::step_context::StepContext;
|
|||||||
use crate::session::tests::make_session_and_context;
|
use crate::session::tests::make_session_and_context;
|
||||||
use crate::session::tests::make_session_and_context_with_rx;
|
use crate::session::tests::make_session_and_context_with_rx;
|
||||||
use crate::session::tests::mcp_config_for_test;
|
use crate::session::tests::mcp_config_for_test;
|
||||||
|
use crate::session::tests::update_selected_settings_for_test;
|
||||||
use crate::session::turn_context::TurnEnvironment;
|
use crate::session::turn_context::TurnEnvironment;
|
||||||
use crate::state::ActiveTurn;
|
use crate::state::ActiveTurn;
|
||||||
use crate::test_support::models_manager_with_provider;
|
use crate::test_support::models_manager_with_provider;
|
||||||
@@ -25,6 +26,7 @@ use codex_features::Features;
|
|||||||
use codex_hooks::HooksConfig;
|
use codex_hooks::HooksConfig;
|
||||||
use codex_model_provider::create_model_provider;
|
use codex_model_provider::create_model_provider;
|
||||||
use codex_protocol::models::PermissionProfile;
|
use codex_protocol::models::PermissionProfile;
|
||||||
|
use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig;
|
||||||
use codex_protocol::protocol::AskForApproval;
|
use codex_protocol::protocol::AskForApproval;
|
||||||
use codex_protocol::protocol::EnvironmentConfig;
|
use codex_protocol::protocol::EnvironmentConfig;
|
||||||
use codex_protocol::protocol::EnvironmentConfigState;
|
use codex_protocol::protocol::EnvironmentConfigState;
|
||||||
@@ -95,6 +97,7 @@ fn mcp_turn_metadata_context(turn_context: &TurnContext) -> McpTurnMetadataConte
|
|||||||
McpTurnMetadataContext {
|
McpTurnMetadataContext {
|
||||||
model: turn_context.model_info().slug.as_str(),
|
model: turn_context.model_info().slug.as_str(),
|
||||||
reasoning_effort: turn_context.effective_reasoning_effort(),
|
reasoning_effort: turn_context.effective_reasoning_effort(),
|
||||||
|
node_repl_disabled: turn_context.model_info().node_repl_disabled,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1012,6 +1015,8 @@ fn truncate_mcp_tool_result_for_event_bounds_large_error() {
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() {
|
async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() {
|
||||||
let (_, turn_context) = make_session_and_context().await;
|
let (_, turn_context) = make_session_and_context().await;
|
||||||
|
let turn_context = Arc::new(turn_context);
|
||||||
|
let step_context = StepContext::for_test(Arc::clone(&turn_context));
|
||||||
turn_context
|
turn_context
|
||||||
.turn_metadata_state
|
.turn_metadata_state
|
||||||
.set_responsesapi_client_metadata(HashMap::from([
|
.set_responsesapi_client_metadata(HashMap::from([
|
||||||
@@ -1024,7 +1029,7 @@ async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() {
|
|||||||
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
||||||
|
|
||||||
let meta = build_mcp_tool_call_request_meta(
|
let meta = build_mcp_tool_call_request_meta(
|
||||||
&turn_context,
|
&step_context,
|
||||||
"custom_server",
|
"custom_server",
|
||||||
"call-custom",
|
"call-custom",
|
||||||
/*metadata*/ None,
|
/*metadata*/ None,
|
||||||
@@ -1067,15 +1072,75 @@ async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test_case::test_case(Some(ReasoningEffortConfig::Low), "low"; "configured effort")]
|
||||||
|
#[test_case::test_case(None, "high"; "pinned model default")]
|
||||||
|
#[tokio::test]
|
||||||
|
async fn mcp_tool_call_request_meta_uses_the_issuing_step(
|
||||||
|
effort: Option<ReasoningEffortConfig>,
|
||||||
|
expected_effort: &str,
|
||||||
|
) {
|
||||||
|
let (_, turn_context) = make_session_and_context().await;
|
||||||
|
let turn_context = Arc::new(turn_context);
|
||||||
|
let step_a = StepContext::for_test(Arc::clone(&turn_context));
|
||||||
|
let original_meta =
|
||||||
|
build_mcp_tool_call_request_meta(&step_a, "node_repl", "call-a", /*metadata*/ None);
|
||||||
|
let mut step_b = StepContext::for_test(Arc::clone(&turn_context));
|
||||||
|
let settings = Arc::make_mut(&mut Arc::get_mut(&mut step_b).expect("unique step").settings);
|
||||||
|
update_selected_settings_for_test(settings, |selected| {
|
||||||
|
selected.collaboration_mode = selected.collaboration_mode.with_updates(
|
||||||
|
Some("model-b".to_string()),
|
||||||
|
Some(effort),
|
||||||
|
/*developer_instructions*/ None,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
let model = Arc::make_mut(&mut settings.model_info);
|
||||||
|
model.slug = "model-b".to_string();
|
||||||
|
model.default_reasoning_level = Some(ReasoningEffortConfig::High);
|
||||||
|
model.node_repl_disabled = true;
|
||||||
|
// This separate policy remains turn-owned in this migration.
|
||||||
|
model.node_repl_auto_review_required =
|
||||||
|
!turn_context.model_info().node_repl_auto_review_required;
|
||||||
|
|
||||||
|
let mut expected = expected_mcp_turn_metadata(&turn_context);
|
||||||
|
expected["model"] = serde_json::json!("model-b");
|
||||||
|
expected["reasoning_effort"] = serde_json::json!(expected_effort);
|
||||||
|
expected["node_repl_disabled"] = serde_json::json!(true);
|
||||||
|
assert_eq!(
|
||||||
|
build_mcp_tool_call_request_meta(&step_b, "node_repl", "call-b", /*metadata*/ None),
|
||||||
|
Some(serde_json::json!({
|
||||||
|
"callId": "call-b",
|
||||||
|
crate::X_CODEX_TURN_METADATA_HEADER: expected,
|
||||||
|
})),
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
build_mcp_tool_call_request_meta(&step_a, "node_repl", "call-a", /*metadata*/ None),
|
||||||
|
original_meta,
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
turn_context
|
||||||
|
.turn_metadata_state
|
||||||
|
.to_responses_metadata(
|
||||||
|
"installation".to_string(),
|
||||||
|
"window".to_string(),
|
||||||
|
crate::responses_metadata::CodexResponsesRequestKind::Turn,
|
||||||
|
)
|
||||||
|
.turn_metadata_value()
|
||||||
|
.expect("Responses turn metadata")["node_repl_disabled"],
|
||||||
|
serde_json::json!(false),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn mcp_tool_call_request_meta_includes_turn_started_at_unix_ms() {
|
async fn mcp_tool_call_request_meta_includes_turn_started_at_unix_ms() {
|
||||||
let (_, turn_context) = make_session_and_context().await;
|
let (_, turn_context) = make_session_and_context().await;
|
||||||
|
let turn_context = Arc::new(turn_context);
|
||||||
|
let step_context = StepContext::for_test(Arc::clone(&turn_context));
|
||||||
turn_context
|
turn_context
|
||||||
.turn_metadata_state
|
.turn_metadata_state
|
||||||
.set_turn_started_at_unix_ms(/*turn_started_at_unix_ms*/ 1_700_000_000_123);
|
.set_turn_started_at_unix_ms(/*turn_started_at_unix_ms*/ 1_700_000_000_123);
|
||||||
|
|
||||||
let meta = build_mcp_tool_call_request_meta(
|
let meta = build_mcp_tool_call_request_meta(
|
||||||
&turn_context,
|
&step_context,
|
||||||
"custom_server",
|
"custom_server",
|
||||||
"call-custom",
|
"call-custom",
|
||||||
/*metadata*/ None,
|
/*metadata*/ None,
|
||||||
@@ -1157,6 +1222,8 @@ async fn mcp_sandbox_cwd_is_none_for_unselected_server_environment() -> anyhow::
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() {
|
async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() {
|
||||||
let (_, turn_context) = make_session_and_context().await;
|
let (_, turn_context) = make_session_and_context().await;
|
||||||
|
let turn_context = Arc::new(turn_context);
|
||||||
|
let step_context = StepContext::for_test(Arc::clone(&turn_context));
|
||||||
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
||||||
let mut metadata = approval_metadata(
|
let mut metadata = approval_metadata(
|
||||||
/*connector_id*/ None, /*connector_name*/ None,
|
/*connector_id*/ None, /*connector_name*/ None,
|
||||||
@@ -1166,7 +1233,7 @@ async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() {
|
|||||||
metadata.plugin_id = Some("sample@test".to_string());
|
metadata.plugin_id = Some("sample@test".to_string());
|
||||||
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
build_mcp_tool_call_request_meta(&turn_context, "sample", "call-plugin", Some(&metadata),),
|
build_mcp_tool_call_request_meta(&step_context, "sample", "call-plugin", Some(&metadata),),
|
||||||
Some(serde_json::json!({
|
Some(serde_json::json!({
|
||||||
"callId": "call-plugin",
|
"callId": "call-plugin",
|
||||||
crate::X_CODEX_TURN_METADATA_HEADER: expected_turn_metadata,
|
crate::X_CODEX_TURN_METADATA_HEADER: expected_turn_metadata,
|
||||||
@@ -1278,6 +1345,8 @@ async fn mcp_tool_call_item_includes_app_identity() {
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps_meta() {
|
async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps_meta() {
|
||||||
let (_, turn_context) = make_session_and_context().await;
|
let (_, turn_context) = make_session_and_context().await;
|
||||||
|
let turn_context = Arc::new(turn_context);
|
||||||
|
let step_context = StepContext::for_test(Arc::clone(&turn_context));
|
||||||
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
||||||
let metadata = McpToolApprovalMetadata {
|
let metadata = McpToolApprovalMetadata {
|
||||||
annotations: None,
|
annotations: None,
|
||||||
@@ -1305,7 +1374,7 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps
|
|||||||
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
build_mcp_tool_call_request_meta(
|
build_mcp_tool_call_request_meta(
|
||||||
&turn_context,
|
&step_context,
|
||||||
CODEX_APPS_MCP_SERVER_NAME,
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
"call_abc123xyz789",
|
"call_abc123xyz789",
|
||||||
Some(&metadata),
|
Some(&metadata),
|
||||||
@@ -1326,11 +1395,13 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn codex_apps_tool_call_request_meta_includes_call_id_without_existing_codex_apps_meta() {
|
async fn codex_apps_tool_call_request_meta_includes_call_id_without_existing_codex_apps_meta() {
|
||||||
let (_, turn_context) = make_session_and_context().await;
|
let (_, turn_context) = make_session_and_context().await;
|
||||||
|
let turn_context = Arc::new(turn_context);
|
||||||
|
let step_context = StepContext::for_test(Arc::clone(&turn_context));
|
||||||
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
let expected_turn_metadata = expected_mcp_turn_metadata(&turn_context);
|
||||||
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
build_mcp_tool_call_request_meta(
|
build_mcp_tool_call_request_meta(
|
||||||
&turn_context,
|
&step_context,
|
||||||
CODEX_APPS_MCP_SERVER_NAME,
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
"call_abc123xyz789",
|
"call_abc123xyz789",
|
||||||
/*metadata*/ None,
|
/*metadata*/ None,
|
||||||
|
|||||||
@@ -80,6 +80,12 @@ impl ResolvedStepSettings {
|
|||||||
.as_ref()
|
.as_ref()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub(crate) fn effective_reasoning_effort(&self) -> Option<ReasoningEffort> {
|
||||||
|
self.reasoning_effort()
|
||||||
|
.or(self.model_info.default_reasoning_level.as_ref())
|
||||||
|
.cloned()
|
||||||
|
}
|
||||||
|
|
||||||
pub(crate) fn approval_policy(&self) -> AskForApproval {
|
pub(crate) fn approval_policy(&self) -> AskForApproval {
|
||||||
self.selected.approval_policy.value()
|
self.selected.approval_policy.value()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -410,9 +410,7 @@ impl TurnContext {
|
|||||||
/// Legacy: returns the frozen initial-turn reasoning effort, including the initial model default.
|
/// Legacy: returns the frozen initial-turn reasoning effort, including the initial model default.
|
||||||
/// Step-scoped consumers should use their captured `StepContext::settings`.
|
/// Step-scoped consumers should use their captured `StepContext::settings`.
|
||||||
pub(crate) fn effective_reasoning_effort(&self) -> Option<ReasoningEffortConfig> {
|
pub(crate) fn effective_reasoning_effort(&self) -> Option<ReasoningEffortConfig> {
|
||||||
self.reasoning_effort()
|
self.initial_settings.effective_reasoning_effort()
|
||||||
.or(self.model_info().default_reasoning_level.as_ref())
|
|
||||||
.cloned()
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Legacy: returns the frozen initial-turn reasoning-effort label for tracing.
|
/// Legacy: returns the frozen initial-turn reasoning-effort label for tracing.
|
||||||
|
|||||||
@@ -164,6 +164,7 @@ async fn to_extension_call(invocation: &ToolInvocation) -> ExtensionToolCall {
|
|||||||
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
||||||
model: invocation.turn.model_info().slug.as_str(),
|
model: invocation.turn.model_info().slug.as_str(),
|
||||||
reasoning_effort: invocation.turn.effective_reasoning_effort(),
|
reasoning_effort: invocation.turn.effective_reasoning_effort(),
|
||||||
|
node_repl_disabled: invocation.turn.model_info().node_repl_disabled,
|
||||||
})
|
})
|
||||||
.and_then(|metadata| to_ascii_json_string(&metadata).ok());
|
.and_then(|metadata| to_ascii_json_string(&metadata).ok());
|
||||||
let mut environments = Vec::new();
|
let mut environments = Vec::new();
|
||||||
|
|||||||
@@ -48,6 +48,7 @@ const WORKSPACE_KIND_KEY: &str = "workspace_kind";
|
|||||||
pub(crate) struct McpTurnMetadataContext<'a> {
|
pub(crate) struct McpTurnMetadataContext<'a> {
|
||||||
pub(crate) model: &'a str,
|
pub(crate) model: &'a str,
|
||||||
pub(crate) reasoning_effort: Option<ReasoningEffortConfig>,
|
pub(crate) reasoning_effort: Option<ReasoningEffortConfig>,
|
||||||
|
pub(crate) node_repl_disabled: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Clone, Debug, Default)]
|
#[derive(Clone, Debug, Default)]
|
||||||
@@ -210,6 +211,8 @@ impl TurnMetadataState {
|
|||||||
context: McpTurnMetadataContext<'_>,
|
context: McpTurnMetadataContext<'_>,
|
||||||
) -> Option<serde_json::Value> {
|
) -> Option<serde_json::Value> {
|
||||||
let mut responses_metadata = self.mcp_metadata_template();
|
let mut responses_metadata = self.mcp_metadata_template();
|
||||||
|
// Use the issuing step's Node REPL restriction.
|
||||||
|
responses_metadata.node_repl_disabled = Some(context.node_repl_disabled);
|
||||||
// Never serialize harness-owned tool inventory for external MCP servers.
|
// Never serialize harness-owned tool inventory for external MCP servers.
|
||||||
responses_metadata.tool_namespaces_info = None;
|
responses_metadata.tool_namespaces_info = None;
|
||||||
let Value::Object(mut metadata) = responses_metadata.turn_metadata_value()? else {
|
let Value::Object(mut metadata) = responses_metadata.turn_metadata_value()? else {
|
||||||
|
|||||||
@@ -45,6 +45,7 @@ fn test_mcp_turn_metadata_context() -> McpTurnMetadataContext<'static> {
|
|||||||
McpTurnMetadataContext {
|
McpTurnMetadataContext {
|
||||||
model: "gpt-5.4",
|
model: "gpt-5.4",
|
||||||
reasoning_effort: Some(ReasoningEffortConfig::High),
|
reasoning_effort: Some(ReasoningEffortConfig::High),
|
||||||
|
node_repl_disabled: false,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -501,6 +502,7 @@ fn turn_metadata_state_includes_model_and_reasoning_effort_only_in_request_meta(
|
|||||||
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
.current_meta_value_for_mcp_request(McpTurnMetadataContext {
|
||||||
model: "gpt-5.4",
|
model: "gpt-5.4",
|
||||||
reasoning_effort: None,
|
reasoning_effort: None,
|
||||||
|
node_repl_disabled: false,
|
||||||
})
|
})
|
||||||
.expect("turn metadata should be present");
|
.expect("turn metadata should be present");
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
|
|||||||
Reference in new issue
Block a user