From 305ba8ae5c9d036d5fa009f677ae0fe3d2f04afb Mon Sep 17 00:00:00 2001 From: Stella Test Date: Thu, 6 Aug 2026 04:06:38 -0700 Subject: [PATCH 1/2] 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; From 57b515230fa1f51fd78428221fb85fb1f7992cdf Mon Sep 17 00:00:00 2001 From: Stella Test Date: Thu, 6 Aug 2026 04:12:49 -0700 Subject: [PATCH 2/2] fix(stella-cli): install the sub-agent pool ceiling that was documented but dead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DEFAULT_POOL_LIMIT_USD = 2.0` is documented as the bound that stops "a model looping on `task`" from quietly spending a session's budget on research. It bound nothing. `SessionSubAgents::new` installs it, and `install` then calls `with_pool_limit(pool_limit_usd)` — which REPLACES the guard wholesale. Every production installer reached that through `install_for_session`, which passed `None`, and `None` means unlimited rather than "nothing to override". So a session without `--budget` whose model wedged on delegation ran every child to `max_steps` with no dollar bound at any layer: the pool was unlimited, and `carve(None, None)` against an unlimited pool yields `ceiling: None`, so the children inherited nothing either. `with_pool_limit`'s semantics are left alone rather than reinterpreted. A caller that genuinely wants no pool ceiling needs a way to say so, and making `None` mean "keep the default" would take that away while leaving the same trap one level up. The fix belongs at the call site, which now names its choice through `session_pool_limit_usd()` — a named function rather than a literal, because a literal at the call site is exactly what went wrong. It stays `Observed`, so crossing $2 warns and the children keep running. That is this repository's standing posture — degradation warns, never disables — and the enforcing bound is elsewhere and unchanged: the parent's guard is the hard ceiling, via the spend ledger the engine drains at each step boundary, so a session that passed `--budget` already stops. Making the pool itself enforcing would add a second wall no caller asked for and no flag can raise. The issue asks for both options to be stated; they are, in the function's own doc comment, and switching is a one-line change to the mode `install_for_session` passes. Witness: `an_unbudgeted_session_still_installs_a_sub_agent_pool_ceiling` asserts both halves, because the first alone is satisfiable by a ceiling that never reaches a child — the pool carries the ceiling, AND a carve against it hands the child finite headroom and the session's mode. It fails on the old behaviour ("the documented default must be what a session actually installs"). The dispatcher is built exactly as `install_for_session` builds it, same mode and same limit; only the provider differs, because constructing the real one needs credentials a unit test has no business holding. `cargo test -p stella-cli --bin stella` — 1428 passed, 0 failed. Closes #1849 --- crates/stella-cli/src/subagent.rs | 38 ++++++++++++++++- crates/stella-cli/src/subagent/tests.rs | 56 +++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/crates/stella-cli/src/subagent.rs b/crates/stella-cli/src/subagent.rs index 77492597b..b1baec04d 100644 --- a/crates/stella-cli/src/subagent.rs +++ b/crates/stella-cli/src/subagent.rs @@ -253,6 +253,14 @@ impl SessionSubAgents { } /// Override the session pool ceiling. + /// + /// `None` means **unlimited**, and replaces the ceiling + /// [`Self::new`] installed — it does not mean "leave the default alone". + /// That reading is what made [`DEFAULT_POOL_LIMIT_USD`] dead code: + /// `install_for_session` passed `None` meaning "nothing to override" and + /// got an unbounded pool (#1849). Kept as-is rather than reinterpreted, + /// because a caller that genuinely wants no pool ceiling needs a way to + /// say so; the fix belongs at the call site, which now names its choice. #[must_use] pub fn with_pool_limit(self, limit_usd: Option) -> Self { let mode = self.pool.lock().unwrap_or_else(|p| p.into_inner()).mode(); @@ -304,11 +312,39 @@ pub fn install_for_session( registry, crate::agent::engine_config_for(cfg), stella_protocol::BudgetMode::Observed, - None, + session_pool_limit_usd(), ); Ok(()) } +/// The sub-agent pool ceiling a session installs when nothing overrides it. +/// +/// A named function rather than a literal at the call site, because a literal +/// there is exactly what went wrong: every production installer passed `None` +/// — which [`SessionSubAgents::with_pool_limit`] reads as *unlimited*, not as +/// "keep the default" — so [`DEFAULT_POOL_LIMIT_USD`] was documented as the +/// bound that stops "a model looping on `task`" while binding nothing. A +/// session without `--budget` whose model wedged on delegation ran every child +/// to `max_steps` with no dollar bound at any layer (#1849). +/// +/// # Why it warns rather than stops +/// +/// The pool is installed `Observed`, so crossing $2 produces a warning and the +/// children keep running. That is this repository's standing posture — +/// degradation warns, never disables — and the enforcing bound is elsewhere +/// and unchanged: the *parent's* guard is the hard ceiling, via the spend +/// ledger the engine drains at each step boundary, so a session that passed +/// `--budget` already stops. Making the pool itself enforcing would add a +/// second wall that a caller never asked for and that no flag can raise. +/// +/// The alternative — enforce at the pool — is a maintainer's call, not this +/// function's: say so and this becomes a one-line change to the mode passed by +/// [`install_for_session`]. +#[must_use] +pub fn session_pool_limit_usd() -> Option { + Some(DEFAULT_POOL_LIMIT_USD) +} + #[async_trait] impl SubAgentDispatcher for SessionSubAgents { async fn dispatch(&self, spec: SubAgentSpec) -> SubAgentOutcome { diff --git a/crates/stella-cli/src/subagent/tests.rs b/crates/stella-cli/src/subagent/tests.rs index 75cc9f42d..89d29db7a 100644 --- a/crates/stella-cli/src/subagent/tests.rs +++ b/crates/stella-cli/src/subagent/tests.rs @@ -149,6 +149,62 @@ async fn the_pool_binds_and_a_failed_child_charges_nothing() { ); } +/// A session that passed no `--budget` must still install a pool ceiling +/// (#1849). +/// +/// `DEFAULT_POOL_LIMIT_USD` was documented as the bound that stops "a model +/// looping on `task`" and bound nothing: every production installer passed +/// `None` to `with_pool_limit`, which means *unlimited*, not "keep the +/// default". So an unbudgeted session whose model wedged on delegation ran +/// every child to `max_steps` with no dollar bound at any layer. +/// +/// Both halves are asserted, because the first alone is satisfiable by a +/// ceiling that never reaches a child: the pool binds, AND a carve against it +/// hands the child finite headroom. `carve(None, None)` on an unlimited pool +/// yields `ceiling: None`, which is the shape that made this invisible. +#[test] +fn an_unbudgeted_session_still_installs_a_sub_agent_pool_ceiling() { + assert_eq!( + crate::subagent::session_pool_limit_usd(), + Some(crate::subagent::DEFAULT_POOL_LIMIT_USD), + "the documented default must be what a session actually installs" + ); + + // Constructed exactly as `install_for_session` constructs it — same mode, + // same limit. Only the provider differs, because building the real one + // needs credentials a unit test has no business holding. + let registry = registry(); + let dispatcher = SessionSubAgents::new( + Arc::new(NeverProvider), + ®istry, + EngineConfig::default(), + stella_protocol::BudgetMode::Observed, + ) + .with_pool_limit(crate::subagent::session_pool_limit_usd()); + + let pool = *dispatcher.pool.lock().unwrap(); + assert_eq!( + pool.session_limit_usd(), + Some(crate::subagent::DEFAULT_POOL_LIMIT_USD), + "the pool must carry the ceiling, not an unbounded guard" + ); + + // The half that reaches the child: an unlimited pool carves an unlimited + // child, so a ceiling nothing inherits is the same as no ceiling. + let child = pool.carve(None); + assert_eq!( + child.session_limit_usd(), + Some(crate::subagent::DEFAULT_POOL_LIMIT_USD), + "a child carved against the pool must inherit finite headroom" + ); + assert_eq!( + child.mode(), + stella_protocol::BudgetMode::Observed, + "and the session's mode — the pool warns, it does not stop (the \ + parent's guard is the enforcing bound)" + ); +} + #[test] fn the_task_tool_is_always_advertised_and_never_read_only() { let registry = registry();