diff --git a/crates/stella-mcp/src/toolset.rs b/crates/stella-mcp/src/toolset.rs index e8dc8c8b8..e20cd5ca8 100644 --- a/crates/stella-mcp/src/toolset.rs +++ b/crates/stella-mcp/src/toolset.rs @@ -485,12 +485,29 @@ impl McpToolSet { #[async_trait] impl ToolExecutor for McpToolSet { + /// The advertised toolset, as two segments in a fixed order: the native + /// tools, then this set's MCP tools. + /// + /// **Both the segment order and the order within each segment are a + /// cross-process contract, not a presentation choice.** This list is + /// serialized verbatim at position 0 of the prompt prefix, and prompt + /// caching is a byte-level prefix match — so two stella processes in the + /// same workspace inside the cache TTL share the tools+system entry only + /// if they emit the same bytes. `ToolRegistry::schemas` sorts the native + /// segment for exactly that reason; this decorator used to concatenate + /// after it without re-sorting, which put the whole guarantee back at the + /// mercy of `self.clients` order and of which server finished connecting + /// first (#1848). + /// + /// Sorting the MCP segment by its namespaced name — rather than trusting + /// client order — is what makes the answer independent of both. fn schemas(&self) -> Vec { let mut schemas = Vec::new(); // Native tools first — they are the base layer the MCP set augments. if let Some(native) = &self.native { schemas.extend(native.schemas()); } + let mut mcp = Vec::new(); for (idx, client) in self.clients.iter().enumerate() { // A disabled server advertises nothing this session — the engine // re-reads schemas each model call, so the model stops seeing its @@ -503,7 +520,7 @@ impl ToolExecutor for McpToolSet { // Only advertise tools that actually route back to this client // (defends against any skipped/collided entry). if self.routes.get(&namespaced).map(|(i, _)| *i) == Some(idx) { - schemas.push(ToolSchema { + mcp.push(ToolSchema { name: namespaced, description: tool.description.clone(), input_schema: tool.input_schema.clone(), @@ -517,6 +534,11 @@ impl ToolExecutor for McpToolSet { } } } + // Namespaced names are unique by construction (`routes` is keyed on + // them), so this is a total order — no tie for the sort to resolve + // arbitrarily and reintroduce the nondeterminism. + mcp.sort_by(|a, b| a.name.cmp(&b.name)); + schemas.extend(mcp); schemas } @@ -784,6 +806,60 @@ mod tests { assert!(set.failed_servers().is_empty()); } + /// The advertised toolset must be byte-identical however the servers + /// happened to be ordered (#1848). + /// + /// This list is serialized verbatim at position 0 of the prompt prefix and + /// prompt caching is a byte-level prefix match, so the question is not + /// "does it look the same" but "do two processes emit the same bytes". + /// `ToolRegistry::schemas` sorts its own tools for that reason; this + /// decorator appended after it without re-sorting, so the guarantee held + /// only as long as `self.clients` order and connection-completion order + /// happened to match between processes — which across a restart, or two + /// stella processes in one workspace inside the cache TTL, they need not. + /// + /// Serialized rather than compared field by field: bytes are what the + /// cache matches on, so bytes are what the assertion should be about. + #[tokio::test] + async fn schemas_are_byte_identical_whatever_order_the_servers_connected_in() { + let forward = McpToolSet::from_clients(vec![ + connected_client("alpha", "one").await, + connected_client("zeta", "two").await, + ]) + .wrapping(Arc::new(FakeNative)); + let reverse = McpToolSet::from_clients(vec![ + connected_client("zeta", "two").await, + connected_client("alpha", "one").await, + ]) + .wrapping(Arc::new(FakeNative)); + + let bytes = |set: &McpToolSet| serde_json::to_string(&set.schemas()).expect("serialize"); + assert_eq!( + bytes(&forward), + bytes(&reverse), + "two processes that connected the same servers in different orders must \ + advertise the same bytes, or they cannot share a prompt-cache entry" + ); + + // And the segment contract the doc comment states: native first, then + // the MCP tools. A sort applied to the WHOLE list would also make the + // assertion above pass while silently moving the native tools. + let names: Vec = forward.schemas().into_iter().map(|s| s.name).collect(); + let first_mcp = names + .iter() + .position(|n| n.starts_with(NS_PREFIX)) + .expect("the mcp segment exists"); + assert!( + names[..first_mcp].iter().all(|n| !n.starts_with(NS_PREFIX)), + "native tools must all precede the mcp segment: {names:?}" + ); + assert_eq!( + names[first_mcp..], + ["mcp__alpha__one".to_string(), "mcp__zeta__two".to_string()], + "the mcp segment is sorted by namespaced name" + ); + } + #[tokio::test] async fn schemas_namespace_mcp_tools_and_include_native() { let client = connected_client("files", "read").await;