openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-21 17:48:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

Attribute nested REPL reviews to their tool calls (#42082)

Browse Source 
## What changed

- Retain MCP invocation metadata for Node-backed REPL servers so nested
  elicitations can resolve the enclosing tool call.
- Use a valid, same-server `callId` from the current turn as the Guardian
  assessment target while continuing to review each nested action and connector
  independently.
- Fall back to an elicitation-specific target when `callId` is missing,
  malformed, unknown, from another server, or from an earlier turn.

## Testing

- Cover ordinary and strict reviews for `node_repl` and `cua_repl`, multiple
  nested decisions, unrelated servers, and invalid or stale call IDs.

GitOrigin-RevId: adea4b5cff96726a540ff3b3ea902b0f5dd99299
This commit is contained in:
Ankush Gupta
2026-09-01 15:34:03 +00:00
committed by copyberry
parent 2350823caa
commit 67cc3c318d
4 changed files with 366 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/mcp_tool_call.rs
+3 -1
View File
@@ -1154,7 +1154,9 @@ impl Session {
};
turn_state.lock().await.insert_mcp_tool_approval_metadata(
call_id.to_string(),
(invocation.server == CODEX_APPS_MCP_SERVER_NAME).then(|| invocation.clone()),
(invocation.server == CODEX_APPS_MCP_SERVER_NAME
|| is_node_repl_backed_server(&invocation.server))
.then(|| invocation.clone()),
metadata,
);
}
codex-rs/core/src/session/mcp.rs
+30 -7
View File
@@ -13,6 +13,7 @@ use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY;
use codex_protocol::capabilities::CapabilityRootLocation;
use codex_protocol::capabilities::SelectedCapabilityRoot;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::mcp::is_node_repl_backed_server;
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY as MCP_ELICITATION_APPROVAL_KIND_KEY;
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_MCP_TOOL_CALL as MCP_ELICITATION_APPROVAL_KIND_MCP_TOOL_CALL;
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_TOOL_SUGGESTION as MCP_ELICITATION_APPROVAL_KIND_TOOL_SUGGESTION;
@@ -724,6 +725,24 @@ async fn review_guardian_mcp_elicitation(
return Ok(None);
};
// The invocation identifies the tool event, but a nested elicitation can
// review a different action and connector than the enclosing JavaScript.
let originating_call_id = if is_node_repl_backed_server(&request.server_name)
&& let Some(call_id) = request
.elicitation
.meta()
.and_then(|meta| meta.get("callId"))
.and_then(Value::as_str)
&& let Some((Some(invocation), _)) = session
.mcp_tool_approval_metadata(&turn_context.sub_id, call_id)
.await
&& invocation.server == request.server_name
{
Some(call_id)
} else {
None
};
let require_synchronous_review = matches!(
request
.elicitation
@@ -804,7 +823,7 @@ async fn review_guardian_mcp_elicitation(
}
let GuardianElicitationReview::ApprovalRequest(guardian_request) =
guardian_elicitation_review_request(&request)
guardian_elicitation_review_request(&request, originating_call_id)
else {
return Ok(None);
};
@@ -884,7 +903,8 @@ async fn review_guardian_mcp_elicitation(
return Ok(None);
}
let guardian_request = match guardian_elicitation_review_request(&request) {
let guardian_request = match guardian_elicitation_review_request(&request, originating_call_id)
{
GuardianElicitationReview::NotRequested => return Ok(None),
GuardianElicitationReview::Decline(reason) => {
warn!(
@@ -921,6 +941,7 @@ async fn review_guardian_mcp_elicitation(
fn guardian_elicitation_review_request(
request: &ElicitationReviewRequest,
originating_call_id: Option<&str>,
) -> GuardianElicitationReview {
let (meta, requested_schema) = match &request.elicitation {
Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams {
@@ -987,11 +1008,13 @@ fn guardian_elicitation_review_request(
GuardianElicitationReview::ApprovalRequest(Box::new(
crate::guardian::GuardianApprovalRequest::McpToolCall {
id: format!(
"mcp_elicitation:{}:{}",
request.server_name,
mcp_elicitation_request_id(&request.request_id)
),
id: originating_call_id.map(str::to_owned).unwrap_or_else(|| {
format!(
"mcp_elicitation:{}:{}",
request.server_name,
mcp_elicitation_request_id(&request.request_id)
)
}),
server: request.server_name.clone(),
tool_name,
arguments,
codex-rs/core/src/session/mcp_tests.rs
+12 -6
View File
@@ -47,7 +47,7 @@ fn guardian_elicitation_review_request_builds_mcp_tool_call() {
}))));
let GuardianElicitationReview::ApprovalRequest(guardian_request) =
guardian_elicitation_review_request(&request)
guardian_elicitation_review_request(&request, /*originating_call_id*/ None)
else {
panic!("expected Guardian MCP tool call request");
};
@@ -86,7 +86,7 @@ fn guardian_elicitation_review_request_defaults_missing_tool_params() {
let request = form_request(guardian_meta(/*tool_params*/ None));
let GuardianElicitationReview::ApprovalRequest(guardian_request) =
guardian_elicitation_review_request(&request)
guardian_elicitation_review_request(&request, /*originating_call_id*/ None)
else {
panic!("expected Guardian MCP tool call request");
};
@@ -163,7 +163,7 @@ fn guardian_elicitation_review_request_requires_opt_in() {
})));
assert_eq!(
guardian_elicitation_review_request(&request),
guardian_elicitation_review_request(&request, /*originating_call_id*/ None),
GuardianElicitationReview::NotRequested
);
}
@@ -181,7 +181,7 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() {
}),
};
assert!(matches!(
guardian_elicitation_review_request(&url_request),
guardian_elicitation_review_request(&url_request, /*originating_call_id*/ None),
GuardianElicitationReview::Decline(_)
));
@@ -201,7 +201,10 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() {
}),
};
assert!(matches!(
guardian_elicitation_review_request(&non_empty_schema_request),
guardian_elicitation_review_request(
&non_empty_schema_request,
/*originating_call_id*/ None
),
GuardianElicitationReview::Decline(_)
));
@@ -210,7 +213,10 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() {
"codex_request_type": "approval_request",
})));
assert!(matches!(
guardian_elicitation_review_request(&missing_tool_name_request),
guardian_elicitation_review_request(
&missing_tool_name_request,
/*originating_call_id*/ None
),
GuardianElicitationReview::Decline(_)
));
}
codex-rs/core/tests/suite/guardian_mcp_elicitation.rs
+321 -10
View File
@@ -3,9 +3,12 @@ use anyhow::Result;
use codex_core::TurnInputRequest;
use codex_core::config::Constrained;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::items::McpToolCallStatus;
use codex_protocol::items::TurnItem;
use codex_protocol::openai_models::AutoReviewMessages;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::GuardianAssessmentAction;
use codex_protocol::protocol::GuardianAssessmentStatus;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::ReviewDecision;
@@ -36,6 +39,10 @@ def send(message):
pending_call = None
approval_meta = json.loads(sys.argv[1]) if len(sys.argv) > 1 else {}
multiple = isinstance(approval_meta, list)
approval_metas = approval_meta if multiple else [approval_meta]
pending_elicitations = []
approval_results = {}
for line in sys.stdin:
request = json.loads(line)
method = request.get("method")
@@ -44,21 +51,37 @@ for line in sys.stdin:
"capabilities": {"tools": {}},
"serverInfo": {"name": "guardian-elicitation-test", "version": "1"}}
elif method == "tools/list":
result = {"tools": [{"name": "request_approval",
result = {"tools": [{"name": "js" if multiple else "request_approval",
"inputSchema": {"type": "object", "properties": {}},
"annotations": {"readOnlyHint": True}}]}
elif method == "tools/call":
if request["params"].get("arguments", {}).get("seed"):
send({"id": request["id"], "result": {"content": [{"type": "text", "text": "seeded"}]}})
continue
pending_call = request["id"]
send({"id": "server-approval", "method": "elicitation/create", "params": {
"message": "Approve the server-side action?",
"requestedSchema": {"type": "object", "properties": {}},
"_meta": {"codex_request_type": "approval_request",
"codex_approval_kind": "mcp_tool_call", "tool_name": "write_record",
**approval_meta}}})
approval_results = {}
for index, meta in enumerate(approval_metas):
invocation_meta = {}
if len(sys.argv) > 2 and sys.argv[2] == "forward":
invocation_meta["callId"] = request["params"]["_meta"]["callId"]
pending_elicitations.append({
"id": f"server-approval-{index}" if multiple else "server-approval",
"method": "elicitation/create", "params": {
"message": "Approve the server-side action?",
"requestedSchema": {"type": "object", "properties": {}},
"_meta": {"codex_request_type": "approval_request",
"codex_approval_kind": "mcp_tool_call", "tool_name": "write_record",
**meta, **invocation_meta}}})
send(pending_elicitations.pop(0))
continue
elif method is None and request.get("id") == "server-approval":
send({"id": pending_call, "result": {"content": [
{"type": "text", "text": json.dumps(request.get("result"))}]}})
elif method is None and str(request.get("id", "")).startswith("server-approval"):
approval_results[request["id"]] = request.get("result")
if pending_elicitations:
send(pending_elicitations.pop(0))
else:
result = approval_results if multiple else request.get("result")
send({"id": pending_call, "result": {"content": [
{"type": "text", "text": json.dumps(result)}]}})
continue
elif method == "resources/list":
result = {"resources": []}
@@ -342,3 +365,291 @@ async fn server_initiated_mcp_elicitation_can_require_synchronous_auto_review(
test.codex.shutdown_and_wait().await?;
Ok(())
}
#[derive(Clone, Copy, PartialEq, Eq)]
enum CallIdSource {
Host,
Missing,
NonString,
Unknown,
WrongServer,
PreviousTurn,
}
#[test_case("node_repl", false, CallIdSource::Host; "ordinary_node_repl")]
#[test_case("node_repl", true, CallIdSource::Host; "strict_node_repl")]
#[test_case("cua_repl", false, CallIdSource::Host; "ordinary_cua_repl")]
#[test_case("cua_repl", true, CallIdSource::Host; "strict_cua_repl")]
#[test_case("node_repl", false, CallIdSource::Missing; "missing_call_id")]
#[test_case("node_repl", true, CallIdSource::Missing; "strict_missing_call_id")]
#[test_case("node_repl", false, CallIdSource::NonString; "non_string_call_id")]
#[test_case("node_repl", false, CallIdSource::Unknown; "unknown_call_id")]
#[test_case("node_repl", false, CallIdSource::WrongServer; "wrong_server_call_id")]
#[test_case("node_repl", false, CallIdSource::PreviousTurn; "previous_turn_call_id")]
#[test_case("elicitation", false, CallIdSource::Host; "unrelated_server")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn node_elicitations_attribute_independent_reviews_without_changing_actions(
server_name: &str,
strict_auto_review: bool,
call_id_source: CallIdSource,
) -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_wine_exec!(Ok(()), "the MCP fixture requires a host Python interpreter");
let actions = [
(
"access_browser_origin",
json!({"origin": "https://example.com"}),
),
("webmcp:write_record", json!({"record": {"value": 42}})),
];
let meta = actions.map(|(tool_name, arguments)| {
let mut meta = json!({
"codex_strict_auto_review": strict_auto_review,
"codex_sensitive_action": true,
"tool_name": tool_name,
"tool_params": arguments,
"connector_id": "inner-connector",
"connector_name": "Inner Connector",
"connector_description": "Connector for the reviewed inner action",
"tool_title": "Inner action",
"tool_description": "Review this action independently from JavaScript",
});
match call_id_source {
CallIdSource::Host | CallIdSource::Missing => {}
CallIdSource::NonString => meta["callId"] = json!(42),
CallIdSource::Unknown => meta["callId"] = json!("unknown-invocation"),
CallIdSource::WrongServer | CallIdSource::PreviousTurn => {
meta["callId"] = json!("seed-invocation");
}
}
meta
});
let server = responses::start_mock_server().await;
let fixture_config = json!({
"command": if cfg!(windows) { "python" } else { "python3" },
"args": ["-u", "-c", ELICITATION_SERVER, serde_json::to_string(&meta)?,
if call_id_source == CallIdSource::Host { "forward" } else { "omit" }],
"default_tools_approval_mode": "approve",
});
let mut mcp_servers = json!({(server_name): fixture_config.clone()});
if call_id_source == CallIdSource::WrongServer {
mcp_servers["cua_repl"] = fixture_config;
}
let mcp_servers = serde_json::from_value(mcp_servers)?;
let mut builder = test_codex().with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
config.approvals_reviewer = if strict_auto_review {
ApprovalsReviewer::User
} else {
ApprovalsReviewer::AutoReview
};
config
.mcp_servers
.set(mcp_servers)
.expect("set MCP fixture");
});
let test = builder.build_with_auto_env(&server).await?;
wait_for_mcp_server(&test.codex, server_name).await?;
if matches!(
call_id_source,
CallIdSource::WrongServer | CallIdSource::PreviousTurn
) {
let seed_server = if call_id_source == CallIdSource::WrongServer {
"cua_repl"
} else {
server_name
};
responses::mount_sse_once(
&server,
responses::sse(vec![
responses::ev_function_call_with_namespace(
"seed-invocation",
&format!("mcp__{seed_server}"),
"js",
r#"{"seed":true}"#,
),
responses::ev_completed("seed-call"),
]),
)
.await;
if call_id_source == CallIdSource::PreviousTurn {
responses::mount_sse_once(
&server,
responses::sse(vec![responses::ev_completed("seed-complete")]),
)
.await;
test.submit_text_turn("Prepare the earlier invocation.")
.await?;
}
}
let parent_arguments = json!({"code": "await browser.doTwoActions()"});
responses::mount_sse_once(
&server,
responses::sse(vec![
responses::ev_function_call_with_namespace(
"eliciting-tool",
&format!("mcp__{server_name}"),
"js",
&parent_arguments.to_string(),
),
responses::ev_completed("parent-tool"),
]),
)
.await;
let mut guardian = Vec::new();
for outcome in ["allow", "deny"] {
guardian.push(
responses::mount_sse_once_match(
&server,
body_partial_json(json!({"client_metadata": {"x-openai-subagent": "guardian"}})),
responses::sse(vec![
responses::ev_assistant_message(
"review-result",
&json!({
"risk_level": "low", "user_authorization": "high",
"outcome": outcome, "rationale": "Independent inner action decision.",
})
.to_string(),
),
responses::ev_completed(outcome),
]),
)
.await,
);
}
let parent = responses::mount_sse_once(
&server,
responses::sse(vec![responses::ev_completed("parent-complete")]),
)
.await;
test.codex
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
text: "Run the JavaScript invocation with two reviewed inner actions.".into(),
text_elements: Vec::new(),
}]))
.await?;
let mut assessments = Vec::new();
let mut tool_items = Vec::new();
loop {
let event = tokio::time::timeout(Duration::from_secs(30), test.codex.next_event())
.await
.context("timed out waiting for independent MCP Guardian reviews")??;
match event.msg {
EventMsg::GuardianAssessment(assessment)
if assessment.status != GuardianAssessmentStatus::InProgress =>
{
assessments.push(assessment)
}
EventMsg::ItemCompleted(event) => {
if let TurnItem::McpToolCall(item) = event.item
&& item.id == "eliciting-tool"
{
tool_items.push(item);
}
}
EventMsg::ElicitationRequest(_) => panic!("Guardian approval must not prompt the user"),
EventMsg::TurnComplete(_) => break,
_ => {}
}
}
assert_eq!(assessments.len(), 2);
assert_ne!(assessments[0].id, assessments[1].id);
let attributed = call_id_source == CallIdSource::Host && server_name != "elicitation";
for (index, (assessment, status)) in assessments
.iter()
.zip([
GuardianAssessmentStatus::Approved,
GuardianAssessmentStatus::Denied,
])
.enumerate()
{
let target = if attributed {
"eliciting-tool".to_string()
} else {
format!("mcp_elicitation:{server_name}:server-approval-{index}")
};
assert_eq!(
(&assessment.target_item_id, &assessment.status),
(&Some(target), &status)
);
assert_eq!(
assessment.action,
GuardianAssessmentAction::McpToolCall {
server: server_name.to_string(),
tool_name: meta[index]["tool_name"]
.as_str()
.expect("inner tool name")
.to_string(),
connector_id: Some("inner-connector".to_string()),
connector_name: Some("Inner Connector".to_string()),
tool_title: Some("Inner action".to_string()),
}
);
let request = guardian[index].single_request();
let prompt = request.message_input_texts("user").join("\n");
let action_text = prompt
.rsplit_once("Planned action JSON:\n")
.context("Guardian planned action")?
.1
.split_once("\n>>> APPROVAL REQUEST END")
.context("Guardian action end")?
.0;
assert_eq!(
serde_json::from_str::<Value>(action_text)?,
json!({
"tool": "mcp_tool_call", "server": server_name,
"tool_name": meta[index]["tool_name"], "arguments": meta[index]["tool_params"],
"connector_id": "inner-connector", "connector_name": "Inner Connector",
"connector_description": "Connector for the reviewed inner action",
"tool_title": "Inner action",
"tool_description": "Review this action independently from JavaScript",
})
);
}
let [tool_item] = tool_items.as_slice() else {
panic!("expected one completed enclosing tool item: {tool_items:?}");
};
assert_eq!(
(
tool_item.server.as_str(),
tool_item.tool.as_str(),
&tool_item.arguments,
tool_item.status
),
(
server_name,
"js",
&parent_arguments,
McpToolCallStatus::Completed
)
);
let output = parent
.single_request()
.function_call_output("eliciting-tool");
let replies: Value = serde_json::from_str(
output["output"][1]["text"]
.as_str()
.context("MCP fixture responses")?,
)?;
assert_eq!(replies.as_object().map(serde_json::Map::len), Some(2));
assert_eq!(
replies["server-approval-0"],
json!({
"action": "accept", "content": {}, "_meta": {"approvals_reviewer": "auto_review"},
})
);
assert_eq!(replies["server-approval-1"]["action"], "decline");
assert_eq!(
replies["server-approval-1"]["_meta"]["approvals_reviewer"],
"auto_review"
);
assert!(
replies["server-approval-1"]["_meta"]["message"]
.as_str()
.expect("Guardian rejection reason")
.contains("Independent inner action decision.")
);
test.codex.shutdown_and_wait().await?;
Ok(())
}