diff --git a/crates/stella-cli/src/subagent.rs b/crates/stella-cli/src/subagent.rs index c64cf867f..c11c2c45b 100644 --- a/crates/stella-cli/src/subagent.rs +++ b/crates/stella-cli/src/subagent.rs @@ -255,6 +255,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(); @@ -306,11 +314,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 14fa495a7..ef44f34f5 100644 --- a/crates/stella-cli/src/subagent/tests.rs +++ b/crates/stella-cli/src/subagent/tests.rs @@ -216,6 +216,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(); 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;