Skip to content

Commit 0b9568d

Browse files
mzeng-openaihanzo-dev
mzeng-openai
authored and
hanzo-dev
committed
fix(core): preserve tool_params for elicitations (openai#14769)
- [x] Preserve tool_params keys.
1 parent 5bc1398 commit 0b9568d

4 files changed

Lines changed: 91 additions & 26 deletions

File tree

‎codex-rs/core/src/mcp_tool_approval_templates.rs‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ pub(crate) struct RenderedMcpToolApprovalTemplate {
2626
pub(crate) struct RenderedMcpToolApprovalParam {
2727
pub(crate) name: String,
2828
pub(crate) value: Value,
29+
pub(crate) display_name: String,
2930
}
3031

3132
#[derive(Debug, Deserialize)]
@@ -142,8 +143,8 @@ fn render_tool_params(
142143
tool_params: &Map<String, Value>,
143144
template_params: &[ConsequentialToolTemplateParam],
144145
) -> Option<(Option<Value>, Vec<RenderedMcpToolApprovalParam>)> {
145-
let mut relabeled = Map::new();
146146
let mut display_params = Vec::new();
147+
let mut display_names = HashSet::new();
147148
let mut handled_names = HashSet::new();
148149

149150
for template_param in template_params {
@@ -154,12 +155,13 @@ fn render_tool_params(
154155
let Some(value) = tool_params.get(&template_param.name) else {
155156
continue;
156157
};
157-
if relabeled.insert(label.to_string(), value.clone()).is_some() {
158+
if !display_names.insert(label.to_string()) {
158159
return None;
159160
}
160161
display_params.push(RenderedMcpToolApprovalParam {
161-
name: label.to_string(),
162+
name: template_param.name.clone(),
162163
value: value.clone(),
164+
display_name: label.to_string(),
163165
});
164166
handled_names.insert(template_param.name.as_str());
165167
}
@@ -174,16 +176,17 @@ fn render_tool_params(
174176
if handled_names.contains(name.as_str()) {
175177
continue;
176178
}
177-
if relabeled.insert(name.clone(), value.clone()).is_some() {
179+
if !display_names.insert(name.clone()) {
178180
return None;
179181
}
180182
display_params.push(RenderedMcpToolApprovalParam {
181183
name: name.clone(),
182184
value: value.clone(),
185+
display_name: name.clone(),
183186
});
184187
}
185188

186-
Some((Some(Value::Object(relabeled)), display_params))
189+
Some((Some(Value::Object(tool_params.clone())), display_params))
187190
}
188191

189192
#[cfg(test)]
@@ -231,22 +234,25 @@ mod tests {
231234
question: "Allow Calendar to create an event?".to_string(),
232235
elicitation_message: "Allow Calendar to create an event?".to_string(),
233236
tool_params: Some(json!({
234-
"Calendar": "primary",
235-
"Title": "Roadmap review",
237+
"title": "Roadmap review",
238+
"calendar_id": "primary",
236239
"timezone": "UTC",
237240
})),
238241
tool_params_display: vec![
239242
RenderedMcpToolApprovalParam {
240-
name: "Calendar".to_string(),
243+
name: "calendar_id".to_string(),
241244
value: json!("primary"),
245+
display_name: "Calendar".to_string(),
242246
},
243247
RenderedMcpToolApprovalParam {
244-
name: "Title".to_string(),
248+
name: "title".to_string(),
245249
value: json!("Roadmap review"),
250+
display_name: "Title".to_string(),
246251
},
247252
RenderedMcpToolApprovalParam {
248253
name: "timezone".to_string(),
249254
value: json!("UTC"),
255+
display_name: "timezone".to_string(),
250256
},
251257
],
252258
})

‎codex-rs/core/src/mcp_tool_call.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1039,6 +1039,7 @@ fn build_mcp_tool_approval_display_params(
10391039
|(name, value)| crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam {
10401040
name: name.clone(),
10411041
value: value.clone(),
1042+
display_name: name.clone(),
10421043
},
10431044
)
10441045
.collect::<Vec<_>>();

‎codex-rs/core/src/mcp_tool_call_tests.rs‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ fn approval_question_text_prepends_safety_reason() {
109109
}
110110

111111
#[tokio::test]
112-
async fn approval_elicitation_request_uses_message_override_and_readable_tool_params() {
112+
async fn approval_elicitation_request_uses_message_override_and_preserves_tool_params_keys() {
113113
let (session, turn_context) = make_session_and_context().await;
114114
let question = build_mcp_tool_approval_question(
115115
"q".to_string(),
@@ -133,10 +133,21 @@ async fn approval_elicitation_request_uses_message_override_and_readable_tool_pa
133133
Some("Create a calendar event."),
134134
)),
135135
tool_params: Some(&serde_json::json!({
136-
"Calendar": "primary",
137-
"Title": "Roadmap review",
136+
"calendar_id": "primary",
137+
"title": "Roadmap review",
138138
})),
139-
tool_params_display: None,
139+
tool_params_display: Some(&[
140+
RenderedMcpToolApprovalParam {
141+
name: "calendar_id".to_string(),
142+
value: serde_json::json!("primary"),
143+
display_name: "Calendar".to_string(),
144+
},
145+
RenderedMcpToolApprovalParam {
146+
name: "title".to_string(),
147+
value: serde_json::json!("Roadmap review"),
148+
display_name: "Title".to_string(),
149+
},
150+
]),
140151
question,
141152
message_override: Some("Allow Calendar to create an event?"),
142153
prompt_options: prompt_options(true, true),
@@ -163,9 +174,21 @@ async fn approval_elicitation_request_uses_message_override_and_readable_tool_pa
163174
MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: "Create Event",
164175
MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: "Create a calendar event.",
165176
MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: {
166-
"Calendar": "primary",
167-
"Title": "Roadmap review",
177+
"calendar_id": "primary",
178+
"title": "Roadmap review",
168179
},
180+
MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY: [
181+
{
182+
"name": "calendar_id",
183+
"value": "primary",
184+
"display_name": "Calendar",
185+
},
186+
{
187+
"name": "title",
188+
"value": "Roadmap review",
189+
"display_name": "Title",
190+
},
191+
],
169192
})),
170193
message: "Allow Calendar to create an event?".to_string(),
171194
requested_schema: McpElicitationSchema {

‎codex-rs/tui/src/bottom_pane/mcp_server_elicitation.rs‎

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ pub(crate) struct ToolSuggestionRequest {
156156
struct McpToolApprovalDisplayParam {
157157
name: String,
158158
value: Value,
159+
display_name: String,
159160
}
160161

161162
#[derive(Clone, Debug, PartialEq)]
@@ -423,6 +424,7 @@ fn parse_tool_approval_display_params(meta: Option<&Value>) -> Vec<McpToolApprov
423424
.map(|(name, value)| McpToolApprovalDisplayParam {
424425
name: name.clone(),
425426
value: value.clone(),
427+
display_name: name.clone(),
426428
})
427429
.collect::<Vec<_>>()
428430
})
@@ -437,9 +439,18 @@ fn parse_tool_approval_display_param(value: &Value) -> Option<McpToolApprovalDis
437439
if name.is_empty() {
438440
return None;
439441
}
442+
let display_name = value
443+
.get("display_name")
444+
.and_then(Value::as_str)
445+
.unwrap_or(name)
446+
.trim();
447+
if display_name.is_empty() {
448+
return None;
449+
}
440450
Some(McpToolApprovalDisplayParam {
441451
name: name.to_string(),
442452
value: value.get("value")?.clone(),
453+
display_name: display_name.to_string(),
443454
})
444455
}
445456

@@ -472,7 +483,7 @@ fn format_tool_approval_display_message(
472483
fn format_tool_approval_display_param_line(param: &McpToolApprovalDisplayParam) -> String {
473484
format!(
474485
"{}: {}",
475-
param.name,
486+
param.display_name,
476487
format_tool_approval_display_param_value(&param.value)
477488
)
478489
}
@@ -1668,7 +1679,7 @@ mod tests {
16681679
fn tool_approval_meta(
16691680
persist_modes: &[&str],
16701681
tool_params: Option<Value>,
1671-
tool_params_display: Option<Vec<(&str, Value)>>,
1682+
tool_params_display: Option<Vec<(&str, Value, &str)>>,
16721683
) -> Option<Value> {
16731684
let mut meta = serde_json::Map::from_iter([(
16741685
APPROVAL_META_KIND_KEY.to_string(),
@@ -1694,10 +1705,11 @@ mod tests {
16941705
Value::Array(
16951706
tool_params_display
16961707
.into_iter()
1697-
.map(|(name, value)| {
1708+
.map(|(name, value, display_name)| {
16981709
serde_json::json!({
16991710
"name": name,
17001711
"value": value,
1712+
"display_name": display_name,
17011713
})
17021714
})
17031715
.collect(),
@@ -1961,8 +1973,16 @@ mod tests {
19611973
"alpha": 1,
19621974
})),
19631975
Some(vec![
1964-
("Calendar", Value::String("primary".to_string())),
1965-
("Title", Value::String("Roadmap review".to_string())),
1976+
(
1977+
"calendar_id",
1978+
Value::String("primary".to_string()),
1979+
"Calendar",
1980+
),
1981+
(
1982+
"title",
1983+
Value::String("Roadmap review".to_string()),
1984+
"Title",
1985+
),
19661986
]),
19671987
),
19681988
),
@@ -1973,12 +1993,14 @@ mod tests {
19731993
request.approval_display_params,
19741994
vec![
19751995
McpToolApprovalDisplayParam {
1976-
name: "Calendar".to_string(),
1996+
name: "calendar_id".to_string(),
19771997
value: Value::String("primary".to_string()),
1998+
display_name: "Calendar".to_string(),
19781999
},
19792000
McpToolApprovalDisplayParam {
1980-
name: "Title".to_string(),
2001+
name: "title".to_string(),
19812002
value: Value::String("Roadmap review".to_string()),
2003+
display_name: "Title".to_string(),
19822004
},
19832005
]
19842006
);
@@ -2348,13 +2370,26 @@ mod tests {
23482370
"ignored_after_limit": "fourth param",
23492371
})),
23502372
Some(vec![
2351-
("Calendar", Value::String("primary".to_string())),
2352-
("Title", Value::String("Roadmap review".to_string())),
23532373
(
2354-
"Notes",
2374+
"calendar_id",
2375+
Value::String("primary".to_string()),
2376+
"Calendar",
2377+
),
2378+
(
2379+
"title",
2380+
Value::String("Roadmap review".to_string()),
2381+
"Title",
2382+
),
2383+
(
2384+
"notes",
23552385
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()),
2386+
"Notes",
2387+
),
2388+
(
2389+
"ignored_after_limit",
2390+
Value::String("fourth param".to_string()),
2391+
"Ignored",
23562392
),
2357-
("Ignored", Value::String("fourth param".to_string())),
23582393
]),
23592394
),
23602395
),

0 commit comments

Comments
 (0)