fix(stella-cli): install the sub-agent pool ceiling that was documented but dead (#1849) - #1879
Open
macanderson wants to merge 2 commits into
Open
fix(stella-cli): install the sub-agent pool ceiling that was documented but dead (#1849)#1879macanderson wants to merge 2 commits into
macanderson wants to merge 2 commits into
Conversation
added 2 commits
August 6, 2026 04:06
…t is byte-stable
`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
…ed but dead
`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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Reviewer's GuideFixes the sub-agent pool ceiling so unbudgeted sessions actually install the documented DEFAULT_POOL_LIMIT_USD, clarifies pool-limit semantics, and makes MCP tool schema advertisement deterministic across server connection order with tests documenting both contracts. Sequence diagram for installing sub-agent pool ceiling in sessionssequenceDiagram
actor User
participant Cli
participant SessionSubAgents
User->>Cli: start_session
Cli->>Cli: install_for_session(cfg, registry, BudgetMode::Observed, session_pool_limit_usd())
Cli->>Cli: session_pool_limit_usd()
Cli-->>Cli: Some(DEFAULT_POOL_LIMIT_USD)
Cli->>SessionSubAgents: install(cfg, registry, BudgetMode::Observed, Some(DEFAULT_POOL_LIMIT_USD))
SessionSubAgents->>SessionSubAgents: with_pool_limit(Some(DEFAULT_POOL_LIMIT_USD))
SessionSubAgents-->>Cli: session with bounded sub-agent pool
Flow diagram for deterministic MCP tool schema advertisementflowchart TD
A[McpToolSet.schemas] --> B[collect native.schemas]
B --> C[collect mcp ToolSchema into mcp]
C --> D[mcp.sort_by name]
D --> E[schemas.extend mcp]
E --> F[return schemas]
subgraph Segments
B
C
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DEFAULT_POOL_LIMIT_USD = 2.0is documented as the bound that stops "a model looping ontask" from quietly spending a session's budget on research. It bound nothing.SessionSubAgents::newinstalls it — andinstallthen callswith_pool_limit(pool_limit_usd), which replaces the guard wholesale. Every production installer reaches that throughinstall_for_session, which passedNone, andNonemeans unlimited, not "nothing to override".The failure compounds downward:
carve(None, None)against an unlimited pool yieldsceiling: None, so the children inherited nothing either. A session without--budgetwhose model wedged on delegation ran every child tomax_stepswith no dollar bound at any layer.Change
One call site, routed through a named function:
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 redefiningNoneas "keep the default" would remove that while leaving the identical trap one level up. The fix belongs at the call site — which now names its choice, because an unexplained literal there is exactly what went wrong.Warn or stop — the maintainer's call, stated
The issue asks for both options on the record.
It stays
Observed: crossing $2 warns, 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--budgetalready stops. Making the pool itself enforcing would add a second wall no caller asked for and no flag can raise.Both readings are written into
session_pool_limit_usd's doc comment, and switching is a one-line change to the modeinstall_for_sessionpasses.Witness
an_unbudgeted_session_still_installs_a_sub_agent_pool_ceilingasserts both halves, because the first alone is satisfiable by a ceiling that never reaches a child:The second is the half that made this invisible: an unlimited pool carves an unlimited child, so a ceiling nothing inherits is the same as no ceiling at all.
On the old behaviour:
The dispatcher is built exactly as
install_for_sessionbuilds it — same mode, same limit. Only the provider differs, because constructing the real one needs credentials a unit test has no business holding; that is the one gap and it is named rather than papered over.Urgency
#1836 makes this worse if it lands first: concurrent siblings can carve in parallel, so an unbounded pool becomes an unbounded overshoot rather than a bounded one.
Closes #1849
Summary by Sourcery
Ensure deterministic tool schema ordering across MCP clients and reinstate the documented default sub-agent pool ceiling for unbudgeted sessions.
Bug Fixes:
Enhancements:
Tests: