fix(mcp): stabilize tool ordering for prefix-cache stability (#1319)
Sort discovered tools by name in three places so the prompt prefix the model sees is deterministic across runs regardless of server pagination order: - McpConnection::discover_tools — after all pages collected - McpPool::all_tools — after iterating connections - McpPool::to_api_tools — final block sent to the model Adds a regression test that exercises a 2-page paginated discovery with reverse-ordered tools and asserts the result is sorted. Adapts the production sort idea from @hxy91819's PR; the test infrastructure here uses the existing ScriptedValueTransport rather than introducing a parallel MockTransport with a different trait signature. Co-Authored-By: hxy91819 <hxy91819@gmail.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -23,6 +23,11 @@ published.
|
||||
|
||||
### Fixed
|
||||
|
||||
- **MCP tool ordering is deterministic** — discovered tools and the
|
||||
resulting API tool block are now sorted by name so the prompt
|
||||
prefix the model sees is stable across runs, regardless of
|
||||
server-side pagination order. Improves prompt-cache hit rates with
|
||||
multi-server MCP setups. Thanks **@hxy91819**.
|
||||
- **Error cells render as plain text** so env-var names (`API_KEY_FOO`)
|
||||
in error messages keep their underscores instead of being parsed as
|
||||
markdown emphasis. Thanks **@douglarek**.
|
||||
|
||||
@@ -1108,6 +1108,10 @@ impl McpConnection {
|
||||
break;
|
||||
}
|
||||
}
|
||||
// Sort by tool name so the order the model sees doesn't depend on
|
||||
// server-side pagination ordering — keeps the prompt prefix stable
|
||||
// for cache-hit purposes (#1319).
|
||||
self.tools.sort_by(|a, b| a.name.cmp(&b.name));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1541,6 +1545,9 @@ impl McpPool {
|
||||
tools.push((format!("mcp_{}_{}", server, tool.name), tool));
|
||||
}
|
||||
}
|
||||
// Sort by prefixed name so iteration order across servers is
|
||||
// deterministic for prefix-cache stability (#1319).
|
||||
tools.sort_by(|a, b| a.0.cmp(&b.0));
|
||||
tools
|
||||
}
|
||||
|
||||
@@ -1816,6 +1823,9 @@ impl McpPool {
|
||||
});
|
||||
}
|
||||
|
||||
// Sort by name for prefix-cache stability — the tool block sent to
|
||||
// the model needs to be deterministic across runs (#1319).
|
||||
api_tools.sort_by(|a, b| a.name.cmp(&b.name));
|
||||
api_tools
|
||||
}
|
||||
|
||||
@@ -2620,6 +2630,49 @@ mod tests {
|
||||
assert!(pool.all_tools().is_empty());
|
||||
}
|
||||
|
||||
/// #1319: discovered tools must be sorted by name so the prompt prefix
|
||||
/// is stable across runs (cache-hit stability), even when the server
|
||||
/// returns them in arbitrary or paginated order.
|
||||
#[tokio::test]
|
||||
async fn discover_tools_sorts_by_name_for_cache_stability() {
|
||||
let sent = Arc::new(Mutex::new(Vec::new()));
|
||||
let transport = ScriptedValueTransport {
|
||||
sent: Arc::clone(&sent),
|
||||
responses: VecDeque::from([
|
||||
json_frame(serde_json::json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"result": {
|
||||
"tools": [
|
||||
{ "name": "zeta", "inputSchema": {} },
|
||||
{ "name": "alpha", "inputSchema": {} }
|
||||
],
|
||||
"nextCursor": "page-2"
|
||||
}
|
||||
})),
|
||||
json_frame(serde_json::json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 2,
|
||||
"result": {
|
||||
"tools": [
|
||||
{ "name": "mu", "inputSchema": {} },
|
||||
{ "name": "beta", "inputSchema": {} }
|
||||
]
|
||||
}
|
||||
})),
|
||||
]),
|
||||
};
|
||||
let mut conn = test_connection(Box::new(transport));
|
||||
conn.discover_tools().await.expect("discover");
|
||||
|
||||
let names: Vec<&str> = conn.tools.iter().map(|t| t.name.as_str()).collect();
|
||||
assert_eq!(
|
||||
names,
|
||||
vec!["alpha", "beta", "mu", "zeta"],
|
||||
"tools must be sorted by name regardless of server order or pagination"
|
||||
);
|
||||
}
|
||||
|
||||
/// #1244: when an MCP stdio server fails to spawn, the underlying OS
|
||||
/// error (e.g. ENOENT for a missing binary) must reach the user via the
|
||||
/// snapshot.error string. Regression test for `err.to_string()` dropping
|
||||
|
||||
Reference in New Issue
Block a user