mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix(core): preserve tool_params for elicitations (#14769)
- [x] Preserve tool_params keys.
This commit is contained in:
@@ -26,6 +26,7 @@ pub(crate) struct RenderedMcpToolApprovalTemplate {
|
|||||||
pub(crate) struct RenderedMcpToolApprovalParam {
|
pub(crate) struct RenderedMcpToolApprovalParam {
|
||||||
pub(crate) name: String,
|
pub(crate) name: String,
|
||||||
pub(crate) value: Value,
|
pub(crate) value: Value,
|
||||||
|
pub(crate) display_name: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, Deserialize)]
|
#[derive(Debug, Deserialize)]
|
||||||
@@ -142,8 +143,8 @@ fn render_tool_params(
|
|||||||
tool_params: &Map<String, Value>,
|
tool_params: &Map<String, Value>,
|
||||||
template_params: &[ConsequentialToolTemplateParam],
|
template_params: &[ConsequentialToolTemplateParam],
|
||||||
) -> Option<(Option<Value>, Vec<RenderedMcpToolApprovalParam>)> {
|
) -> Option<(Option<Value>, Vec<RenderedMcpToolApprovalParam>)> {
|
||||||
let mut relabeled = Map::new();
|
|
||||||
let mut display_params = Vec::new();
|
let mut display_params = Vec::new();
|
||||||
|
let mut display_names = HashSet::new();
|
||||||
let mut handled_names = HashSet::new();
|
let mut handled_names = HashSet::new();
|
||||||
|
|
||||||
for template_param in template_params {
|
for template_param in template_params {
|
||||||
@@ -154,12 +155,13 @@ fn render_tool_params(
|
|||||||
let Some(value) = tool_params.get(&template_param.name) else {
|
let Some(value) = tool_params.get(&template_param.name) else {
|
||||||
continue;
|
continue;
|
||||||
};
|
};
|
||||||
if relabeled.insert(label.to_string(), value.clone()).is_some() {
|
if !display_names.insert(label.to_string()) {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
display_params.push(RenderedMcpToolApprovalParam {
|
display_params.push(RenderedMcpToolApprovalParam {
|
||||||
name: label.to_string(),
|
name: template_param.name.clone(),
|
||||||
value: value.clone(),
|
value: value.clone(),
|
||||||
|
display_name: label.to_string(),
|
||||||
});
|
});
|
||||||
handled_names.insert(template_param.name.as_str());
|
handled_names.insert(template_param.name.as_str());
|
||||||
}
|
}
|
||||||
@@ -174,16 +176,17 @@ fn render_tool_params(
|
|||||||
if handled_names.contains(name.as_str()) {
|
if handled_names.contains(name.as_str()) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
if relabeled.insert(name.clone(), value.clone()).is_some() {
|
if !display_names.insert(name.clone()) {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
display_params.push(RenderedMcpToolApprovalParam {
|
display_params.push(RenderedMcpToolApprovalParam {
|
||||||
name: name.clone(),
|
name: name.clone(),
|
||||||
value: value.clone(),
|
value: value.clone(),
|
||||||
|
display_name: name.clone(),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
Some((Some(Value::Object(relabeled)), display_params))
|
Some((Some(Value::Object(tool_params.clone())), display_params))
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
@@ -231,22 +234,25 @@ mod tests {
|
|||||||
question: "Allow Calendar to create an event?".to_string(),
|
question: "Allow Calendar to create an event?".to_string(),
|
||||||
elicitation_message: "Allow Calendar to create an event?".to_string(),
|
elicitation_message: "Allow Calendar to create an event?".to_string(),
|
||||||
tool_params: Some(json!({
|
tool_params: Some(json!({
|
||||||
"Calendar": "primary",
|
"title": "Roadmap review",
|
||||||
"Title": "Roadmap review",
|
"calendar_id": "primary",
|
||||||
"timezone": "UTC",
|
"timezone": "UTC",
|
||||||
})),
|
})),
|
||||||
tool_params_display: vec![
|
tool_params_display: vec![
|
||||||
RenderedMcpToolApprovalParam {
|
RenderedMcpToolApprovalParam {
|
||||||
name: "Calendar".to_string(),
|
name: "calendar_id".to_string(),
|
||||||
value: json!("primary"),
|
value: json!("primary"),
|
||||||
|
display_name: "Calendar".to_string(),
|
||||||
},
|
},
|
||||||
RenderedMcpToolApprovalParam {
|
RenderedMcpToolApprovalParam {
|
||||||
name: "Title".to_string(),
|
name: "title".to_string(),
|
||||||
value: json!("Roadmap review"),
|
value: json!("Roadmap review"),
|
||||||
|
display_name: "Title".to_string(),
|
||||||
},
|
},
|
||||||
RenderedMcpToolApprovalParam {
|
RenderedMcpToolApprovalParam {
|
||||||
name: "timezone".to_string(),
|
name: "timezone".to_string(),
|
||||||
value: json!("UTC"),
|
value: json!("UTC"),
|
||||||
|
display_name: "timezone".to_string(),
|
||||||
},
|
},
|
||||||
],
|
],
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -1039,6 +1039,7 @@ fn build_mcp_tool_approval_display_params(
|
|||||||
|(name, value)| crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam {
|
|(name, value)| crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam {
|
||||||
name: name.clone(),
|
name: name.clone(),
|
||||||
value: value.clone(),
|
value: value.clone(),
|
||||||
|
display_name: name.clone(),
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
.collect::<Vec<_>>();
|
.collect::<Vec<_>>();
|
||||||
|
|||||||
@@ -109,7 +109,7 @@ fn approval_question_text_prepends_safety_reason() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn approval_elicitation_request_uses_message_override_and_readable_tool_params() {
|
async fn approval_elicitation_request_uses_message_override_and_preserves_tool_params_keys() {
|
||||||
let (session, turn_context) = make_session_and_context().await;
|
let (session, turn_context) = make_session_and_context().await;
|
||||||
let question = build_mcp_tool_approval_question(
|
let question = build_mcp_tool_approval_question(
|
||||||
"q".to_string(),
|
"q".to_string(),
|
||||||
@@ -133,10 +133,21 @@ async fn approval_elicitation_request_uses_message_override_and_readable_tool_pa
|
|||||||
Some("Create a calendar event."),
|
Some("Create a calendar event."),
|
||||||
)),
|
)),
|
||||||
tool_params: Some(&serde_json::json!({
|
tool_params: Some(&serde_json::json!({
|
||||||
"Calendar": "primary",
|
"calendar_id": "primary",
|
||||||
"Title": "Roadmap review",
|
"title": "Roadmap review",
|
||||||
})),
|
})),
|
||||||
tool_params_display: None,
|
tool_params_display: Some(&[
|
||||||
|
RenderedMcpToolApprovalParam {
|
||||||
|
name: "calendar_id".to_string(),
|
||||||
|
value: serde_json::json!("primary"),
|
||||||
|
display_name: "Calendar".to_string(),
|
||||||
|
},
|
||||||
|
RenderedMcpToolApprovalParam {
|
||||||
|
name: "title".to_string(),
|
||||||
|
value: serde_json::json!("Roadmap review"),
|
||||||
|
display_name: "Title".to_string(),
|
||||||
|
},
|
||||||
|
]),
|
||||||
question,
|
question,
|
||||||
message_override: Some("Allow Calendar to create an event?"),
|
message_override: Some("Allow Calendar to create an event?"),
|
||||||
prompt_options: prompt_options(true, true),
|
prompt_options: prompt_options(true, true),
|
||||||
@@ -163,9 +174,21 @@ async fn approval_elicitation_request_uses_message_override_and_readable_tool_pa
|
|||||||
MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: "Create Event",
|
MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: "Create Event",
|
||||||
MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: "Create a calendar event.",
|
MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: "Create a calendar event.",
|
||||||
MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: {
|
MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: {
|
||||||
"Calendar": "primary",
|
"calendar_id": "primary",
|
||||||
"Title": "Roadmap review",
|
"title": "Roadmap review",
|
||||||
},
|
},
|
||||||
|
MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY: [
|
||||||
|
{
|
||||||
|
"name": "calendar_id",
|
||||||
|
"value": "primary",
|
||||||
|
"display_name": "Calendar",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"name": "title",
|
||||||
|
"value": "Roadmap review",
|
||||||
|
"display_name": "Title",
|
||||||
|
},
|
||||||
|
],
|
||||||
})),
|
})),
|
||||||
message: "Allow Calendar to create an event?".to_string(),
|
message: "Allow Calendar to create an event?".to_string(),
|
||||||
requested_schema: McpElicitationSchema {
|
requested_schema: McpElicitationSchema {
|
||||||
|
|||||||
@@ -156,6 +156,7 @@ pub(crate) struct ToolSuggestionRequest {
|
|||||||
struct McpToolApprovalDisplayParam {
|
struct McpToolApprovalDisplayParam {
|
||||||
name: String,
|
name: String,
|
||||||
value: Value,
|
value: Value,
|
||||||
|
display_name: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Clone, Debug, PartialEq)]
|
#[derive(Clone, Debug, PartialEq)]
|
||||||
@@ -423,6 +424,7 @@ fn parse_tool_approval_display_params(meta: Option<&Value>) -> Vec<McpToolApprov
|
|||||||
.map(|(name, value)| McpToolApprovalDisplayParam {
|
.map(|(name, value)| McpToolApprovalDisplayParam {
|
||||||
name: name.clone(),
|
name: name.clone(),
|
||||||
value: value.clone(),
|
value: value.clone(),
|
||||||
|
display_name: name.clone(),
|
||||||
})
|
})
|
||||||
.collect::<Vec<_>>()
|
.collect::<Vec<_>>()
|
||||||
})
|
})
|
||||||
@@ -437,9 +439,18 @@ fn parse_tool_approval_display_param(value: &Value) -> Option<McpToolApprovalDis
|
|||||||
if name.is_empty() {
|
if name.is_empty() {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
|
let display_name = value
|
||||||
|
.get("display_name")
|
||||||
|
.and_then(Value::as_str)
|
||||||
|
.unwrap_or(name)
|
||||||
|
.trim();
|
||||||
|
if display_name.is_empty() {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
Some(McpToolApprovalDisplayParam {
|
Some(McpToolApprovalDisplayParam {
|
||||||
name: name.to_string(),
|
name: name.to_string(),
|
||||||
value: value.get("value")?.clone(),
|
value: value.get("value")?.clone(),
|
||||||
|
display_name: display_name.to_string(),
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -472,7 +483,7 @@ fn format_tool_approval_display_message(
|
|||||||
fn format_tool_approval_display_param_line(param: &McpToolApprovalDisplayParam) -> String {
|
fn format_tool_approval_display_param_line(param: &McpToolApprovalDisplayParam) -> String {
|
||||||
format!(
|
format!(
|
||||||
"{}: {}",
|
"{}: {}",
|
||||||
param.name,
|
param.display_name,
|
||||||
format_tool_approval_display_param_value(¶m.value)
|
format_tool_approval_display_param_value(¶m.value)
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
@@ -1668,7 +1679,7 @@ mod tests {
|
|||||||
fn tool_approval_meta(
|
fn tool_approval_meta(
|
||||||
persist_modes: &[&str],
|
persist_modes: &[&str],
|
||||||
tool_params: Option<Value>,
|
tool_params: Option<Value>,
|
||||||
tool_params_display: Option<Vec<(&str, Value)>>,
|
tool_params_display: Option<Vec<(&str, Value, &str)>>,
|
||||||
) -> Option<Value> {
|
) -> Option<Value> {
|
||||||
let mut meta = serde_json::Map::from_iter([(
|
let mut meta = serde_json::Map::from_iter([(
|
||||||
APPROVAL_META_KIND_KEY.to_string(),
|
APPROVAL_META_KIND_KEY.to_string(),
|
||||||
@@ -1694,10 +1705,11 @@ mod tests {
|
|||||||
Value::Array(
|
Value::Array(
|
||||||
tool_params_display
|
tool_params_display
|
||||||
.into_iter()
|
.into_iter()
|
||||||
.map(|(name, value)| {
|
.map(|(name, value, display_name)| {
|
||||||
serde_json::json!({
|
serde_json::json!({
|
||||||
"name": name,
|
"name": name,
|
||||||
"value": value,
|
"value": value,
|
||||||
|
"display_name": display_name,
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
.collect(),
|
.collect(),
|
||||||
@@ -1961,8 +1973,16 @@ mod tests {
|
|||||||
"alpha": 1,
|
"alpha": 1,
|
||||||
})),
|
})),
|
||||||
Some(vec![
|
Some(vec![
|
||||||
("Calendar", Value::String("primary".to_string())),
|
(
|
||||||
("Title", Value::String("Roadmap review".to_string())),
|
"calendar_id",
|
||||||
|
Value::String("primary".to_string()),
|
||||||
|
"Calendar",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"title",
|
||||||
|
Value::String("Roadmap review".to_string()),
|
||||||
|
"Title",
|
||||||
|
),
|
||||||
]),
|
]),
|
||||||
),
|
),
|
||||||
),
|
),
|
||||||
@@ -1973,12 +1993,14 @@ mod tests {
|
|||||||
request.approval_display_params,
|
request.approval_display_params,
|
||||||
vec![
|
vec![
|
||||||
McpToolApprovalDisplayParam {
|
McpToolApprovalDisplayParam {
|
||||||
name: "Calendar".to_string(),
|
name: "calendar_id".to_string(),
|
||||||
value: Value::String("primary".to_string()),
|
value: Value::String("primary".to_string()),
|
||||||
|
display_name: "Calendar".to_string(),
|
||||||
},
|
},
|
||||||
McpToolApprovalDisplayParam {
|
McpToolApprovalDisplayParam {
|
||||||
name: "Title".to_string(),
|
name: "title".to_string(),
|
||||||
value: Value::String("Roadmap review".to_string()),
|
value: Value::String("Roadmap review".to_string()),
|
||||||
|
display_name: "Title".to_string(),
|
||||||
},
|
},
|
||||||
]
|
]
|
||||||
);
|
);
|
||||||
@@ -2348,13 +2370,26 @@ mod tests {
|
|||||||
"ignored_after_limit": "fourth param",
|
"ignored_after_limit": "fourth param",
|
||||||
})),
|
})),
|
||||||
Some(vec![
|
Some(vec![
|
||||||
("Calendar", Value::String("primary".to_string())),
|
|
||||||
("Title", Value::String("Roadmap review".to_string())),
|
|
||||||
(
|
(
|
||||||
|
"calendar_id",
|
||||||
|
Value::String("primary".to_string()),
|
||||||
|
"Calendar",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"title",
|
||||||
|
Value::String("Roadmap review".to_string()),
|
||||||
|
"Title",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"notes",
|
||||||
|
Value::String("This is a deliberately long note that should truncate before it turns the approval body into a giant wall of text in the TUI overlay.".to_string()),
|
||||||
"Notes",
|
"Notes",
|
||||||
Value::String("This is a deliberately long note that should truncate before it turns the approval body into a giant wall of text in the TUI overlay.".to_string()),
|
),
|
||||||
|
(
|
||||||
|
"ignored_after_limit",
|
||||||
|
Value::String("fourth param".to_string()),
|
||||||
|
"Ignored",
|
||||||
),
|
),
|
||||||
("Ignored", Value::String("fourth param".to_string())),
|
|
||||||
]),
|
]),
|
||||||
),
|
),
|
||||||
),
|
),
|
||||||
|
|||||||
Reference in New Issue
Block a user