From 305ba8ae5c9d036d5fa009f677ae0fe3d2f04afb Mon Sep 17 00:00:00 2001 From: Stella Test Date: Thu, 6 Aug 2026 04:06:38 -0700 Subject: [PATCH] fix(stella-mcp): sort the MCP schema segment so the advertised toolset is byte-stable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ToolRegistry::schemas` sorts by name for a stated reason: the list is serialized verbatim at position 0 of the prompt prefix, prompt caching is a byte-level prefix match, and HashMap iteration is per-process randomized. So two processes share the tools+system cache entry only if they emit the same bytes. `McpToolSet::schemas` then concatenated after that sorted list without re-sorting, which handed the guarantee straight back to `self.clients` order and to which server finished connecting first. Within one process the order is stable, so this is a CROSS-process miss — a restart inside the cache TTL, or two stella processes in one workspace, is exactly the case the registry's sort comment says it exists for. The MCP segment is now sorted by its namespaced name, which makes the answer independent of both client order and connection-completion order rather than merely stable within a run. Namespaced names are unique by construction (`routes` is keyed on them), so the order is total and no tie is left for the sort to break arbitrarily. Segments are preserved rather than flattened: native tools first, then MCP. That order is a deliberate contract ("the base layer the MCP set augments"), and sorting the whole list would have made the new test pass while silently moving the native tools — so the witness asserts the segment boundary too. The two sibling decorators were checked and need no change. `CandidateMcpView::schemas` concatenates a sorted native list with a filtered view of `inner.schemas()`, and filtering preserves relative order, so it inherits this fix. `DiscoveryToolSet::discovery_schemas` builds a literal `vec![]`, which is ordered by construction. Witness: `schemas_are_byte_identical_whatever_order_the_servers_connected_in` builds two sets from the same two servers registered in opposite orders and compares the SERIALIZED schemas — bytes, because bytes are what the cache matches on. It fails on the old code ("two processes that connected the same servers in different orders must advertise the same bytes") and passes with the sort. This changes the advertised order once, which is a one-time prompt-cache invalidation for sessions live across the upgrade. That is the cost of having the property at all, and it is paid once rather than on every restart. `cargo test -p stella-mcp` — 146 passed, 0 failed. The prompt-cache golden fixtures (`cargo test -p stella-pipeline --test cache_correctness`) are unaffected: 5 passed. Closes #1848 --- crates/stella-mcp/src/toolset.rs | 78 +++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/crates/stella-mcp/src/toolset.rs b/crates/stella-mcp/src/toolset.rs index 5d1821eb2..819115fe8 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 } @@ -739,6 +761,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;