Skip to content

Commit ee2ccb5

Browse files
authored
Fix cache hit rate by making MCP tools order deterministic (#2611)
Fixes #2610 This PR sorts the tools in `get_openai_tools` by name to ensure a consistent MCP tool order. Currently, MCP servers are stored in a HashMap, which does not guarantee ordering. As a result, the tool order changes across turns, effectively breaking prompt caching in multi-turn sessions. An alternative solution would be to replace the HashMap with an ordered structure, but that would require a much larger code change. Given that it is unrealistic to have so many MCP tools that sorting would cause performance issues, this lightweight fix is chosen instead. By ensuring deterministic tool order, this change should significantly improve cache hit rates and prevent users from hitting usage limits too quickly. (For reference, my own sessions last week reached the limit unusually fast, with cache hit rates falling below 1%.) ## Result After this fix, sessions with MCP servers now show caching behavior almost identical to sessions without MCP servers. Without MCP | With MCP :-------------------------:|:-------------------------: <img width="1368" height="1634" alt="image" src="https://github.com/user-attachments/assets/26edab45-7be8-4d6a-b471-558016615fc8" /> | <img width="1356" height="1632" alt="image" src="https://github.com/user-attachments/assets/5f3634e0-3888-420b-9aaf-deefd9397b40" />
1 parent 8b49346 commit ee2ccb5

File tree

1 file changed

+80
-1
lines changed

1 file changed

+80
-1
lines changed

codex-rs/core/src/openai_tools.rs

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,12 @@ pub(crate) fn get_openai_tools(
531531
}
532532

533533
if let Some(mcp_tools) = mcp_tools {
534-
for (name, tool) in mcp_tools {
534+
// Ensure deterministic ordering to maximize prompt cache hits.
535+
// HashMap iteration order is non-deterministic, so sort by fully-qualified tool name.
536+
let mut entries: Vec<(String, mcp_types::Tool)> = mcp_tools.into_iter().collect();
537+
entries.sort_by(|a, b| a.0.cmp(&b.0));
538+
539+
for (name, tool) in entries.into_iter() {
535540
match mcp_tool_to_openai_tool(name.clone(), tool.clone()) {
536541
Ok(converted_tool) => tools.push(OpenAiTool::Function(converted_tool)),
537542
Err(e) => {
@@ -710,6 +715,80 @@ mod tests {
710715
);
711716
}
712717

718+
#[test]
719+
fn test_get_openai_tools_mcp_tools_sorted_by_name() {
720+
let model_family = find_family_for_model("o3").expect("o3 should be a valid model family");
721+
let config = ToolsConfig::new(
722+
&model_family,
723+
AskForApproval::Never,
724+
SandboxPolicy::ReadOnly,
725+
false,
726+
false,
727+
/*use_experimental_streamable_shell_tool*/ false,
728+
);
729+
730+
// Intentionally construct a map with keys that would sort alphabetically.
731+
let tools_map: HashMap<String, mcp_types::Tool> = HashMap::from([
732+
(
733+
"test_server/do".to_string(),
734+
mcp_types::Tool {
735+
name: "a".to_string(),
736+
input_schema: ToolInputSchema {
737+
properties: Some(serde_json::json!({})),
738+
required: None,
739+
r#type: "object".to_string(),
740+
},
741+
output_schema: None,
742+
title: None,
743+
annotations: None,
744+
description: Some("a".to_string()),
745+
},
746+
),
747+
(
748+
"test_server/something".to_string(),
749+
mcp_types::Tool {
750+
name: "b".to_string(),
751+
input_schema: ToolInputSchema {
752+
properties: Some(serde_json::json!({})),
753+
required: None,
754+
r#type: "object".to_string(),
755+
},
756+
output_schema: None,
757+
title: None,
758+
annotations: None,
759+
description: Some("b".to_string()),
760+
},
761+
),
762+
(
763+
"test_server/cool".to_string(),
764+
mcp_types::Tool {
765+
name: "c".to_string(),
766+
input_schema: ToolInputSchema {
767+
properties: Some(serde_json::json!({})),
768+
required: None,
769+
r#type: "object".to_string(),
770+
},
771+
output_schema: None,
772+
title: None,
773+
annotations: None,
774+
description: Some("c".to_string()),
775+
},
776+
),
777+
]);
778+
779+
let tools = get_openai_tools(&config, Some(tools_map));
780+
// Expect shell first, followed by MCP tools sorted by fully-qualified name.
781+
assert_eq_tool_names(
782+
&tools,
783+
&[
784+
"shell",
785+
"test_server/cool",
786+
"test_server/do",
787+
"test_server/something",
788+
],
789+
);
790+
}
791+
713792
#[test]
714793
fn test_mcp_tool_property_missing_type_defaults_to_string() {
715794
let model_family = find_family_for_model("o3").expect("o3 should be a valid model family");

0 commit comments

Comments
 (0)