fix(stella-cli): install the sub-agent pool ceiling that was documented but dead (#1849)

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<f64>) -> 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<f64> {
+    Some(DEFAULT_POOL_LIMIT_USD)
+}
+
 #[async_trait]
 impl SubAgentDispatcher for SessionSubAgents {
     async fn dispatch(&self, spec: SubAgentSpec) -> SubAgentOutcome {

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),
+        &registry,
+        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();

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<ToolSchema> {
         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<String> = 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;