fix(stella-mcp): bound one server's contribution to the advertised toolset (#1856) - #1898
Merged
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
…olset The issue reports MCP schemas as forwarded "uncapped". That is not quite right, and the correction is the fix. `crate::client::ingest` already caps every tool INDIVIDUALLY — 256 tools per server, 2,000 description characters, a per-tool `inputSchema` byte budget. What nothing bounds is the AGGREGATE. 256 tools times a 2,000-character description is ~512 KB of third-party prose from a single server, at position 0 of every request, before schemas are counted — and every per-item cap holds the whole way. The caps are per-item; the cost is the sum. Measured, through the real `tools/list` path: 40 tools with 2 KB descriptions (every ingest cap satisfied) advertise 81,440 bytes from one server. Adds `MAX_SERVER_SCHEMA_BYTES` (32 KB, ~9k tokens) applied per server. The first tool of a server is always admitted however large — a server whose one tool exceeds the budget should advertise that tool, not vanish; ingest's per-tool caps already bound it. Applied to the SORTED segment, which matters for more than tidiness: sorting by namespaced name groups each server's tools into a contiguous run AND fixes the order inside it, so the truncation point is a function of the tool names rather than of client order or connection timing. Budgeting an unsorted list would have re-lost the cross-process byte stability the sort exists to buy. `over_budget_servers()` reports what was cut, deliberately separate from `over_advertising_servers()`: those are two different walls and a reader needs to know which one was hit — 300 tools trips the count cap, twelve verbose ones trip this. One `budget_segment` fold serves both callers, and one `mcp_segment` builds the input for both, rather than a second copy of either for reporting (the drift #1613 was filed for). Witness: `one_chatty_server_cannot_spend_the_whole_prefix` drives a 40-tool server through the real transport and asserts the block is bounded, that the server is trimmed rather than silenced, that every tool is either advertised or counted as dropped, and that the survivors are the lexicographic prefix (so the cut is deterministic). It fails on the old code at 81,440 bytes. `an_ordinary_server_is_not_trimmed` is the other direction — the budget must be invisible for the servers people actually run. ## Not done: the lean-catalog default The issue's other half asks to flip `STELLA_LEAN_TOOLS` on by default, and its own fix direction says to "measure lean-mode's activation rate on the bench corpus before flipping any default" — each activation mutates the schema block and costs a prefix invalidation, so the trade is a ~70% smaller prefix against ~4-5 invalidations per session. That is a bench measurement, not a code change, and flipping it unmeasured is exactly what the issue warns against. Left open. `cargo test -p stella-mcp` — 130 passed, 0 failed. Clippy and fmt clean. Refs #1856, #1848
![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 GuideIntroduce a per-server byte budget for MCP tool schemas, enforce it deterministically on the sorted MCP segment, and expose reporting for servers trimmed by this budget while adding tests to guarantee byte-stable schema ordering and correct budget behavior. Flow diagram for MCP schema budgeting in McpToolSet.schemasflowchart LR
subgraph McpToolSet
A[schemas]
B[mcp_segment]
C[over_budget_servers]
end
A --> B
C --> B
B --> D[sorted MCP ToolSchema list]
D --> E[budget_segment]
E --> F[kept ToolSchema list]
E --> G[(server, dropped_count list)]
F --> H[final advertised schemas]
G --> I[over_budget_servers result]
subgraph helpers
J[server_of]
K[schema_cost]
end
E -. uses .-> J
E -. uses .-> K
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
11 tasks
macanderson
added a commit
that referenced
this pull request
Aug 6, 2026
…ter the #1875/#1898 growth (#1906) ## What & why Unbreaks main's `file-size` gate. `crates/stella-mcp/src/toolset.rs` reached **1756 lines** after three stacked merges — #1875 (sort the MCP schema segment for byte-stability, #1848), #1898 (bound one server's contribution to the advertised toolset, #1856), and the parked-waits work (#1860) — crossing the hard 1500-line limit for files not in `scripts/file-size-baseline.txt`. **What moved where:** the `#[cfg(test)] mod tests` block (~945 lines, more than half the file) moved out to a sibling submodule, `crates/stella-mcp/src/toolset/tests.rs`, declared as `#[cfg(test)] mod tests;`. This is the pattern AGENTS.md names (`crates/stella-core/src/driver/settlement.rs`, split out of `driver.rs`) and the exact shape `stella-model` already uses for `src/zai/tests.rs` and `src/anthropic/tests.rs`. Purely mechanical: the block is de-indented one level (plus the rustfmt repack that de-indent allows) — **no visibility changes** (a child module reaches the parent's private items via `use super::*`), no renames, no behavior change. The new file carries a `//!` module doc per crate idiom, and the README Layout row for `toolset.rs` now names the tests file (matching `stella-model`'s README style). - `src/toolset.rs`: 1756 → **812** lines - `src/toolset/tests.rs` (new): **948** lines **Why no baseline entry:** the guard's own policy — `scripts/file-size-baseline.txt` accepts no new entries; a file over the limit gets split, not grandfathered (AGENTS.md § God files). **Two more main unbreaks riding along:** - main's `cargo doc -D warnings` fails with *public documentation for `MAX_SERVER_SCHEMA_BYTES` links to private item `crate::client::ingest`*. The intra-doc link in that const's doc block (same file, `toolset.rs`) is now plain code font — same fix shape as #1823/#1830. - main's `Cargo.lock` is stale: `stella-diff`'s manifest says 0.6.127 but the lock records 0.6.126, so any build rewrites the lock and CI's `Cargo.lock` sync check fails. One-line resync, generated by cargo itself. **Heads-up for reviewers:** this PR alone does not green the gate — the `stella-pipeline` compile break and the Makefile duplicate target are being fixed in a separate PR by the coordinator. ## The witness - [x] No witness needed (pure refactor / docs / CI) — because: this is a mechanical module split plus a doc-link defuse and a lockfile resync; behavior is proven unchanged by the existing suite, not by a new test. All 122 `stella-mcp` unit tests (the relocated `toolset::tests` among them) and every integration suite pass unchanged. ## The gate GitHub Actions is in a major outage, so CI will not run on this PR — local verification is the evidence, run in this branch's worktree: - [x] `./scripts/check-file-size.sh` — OK, none over 1500 except the 32 grandfathered (none grew) - [x] `./scripts/check-god-files.sh` — OK; `stella-mcp`'s "no god files" README claim stays true - [x] `cargo fmt -p stella-mcp -- --check` — clean - [x] `cargo check -p stella-mcp` — clean - [x] `cargo clippy -p stella-mcp --all-targets -- -D warnings` — clean - [x] `cargo test -p stella-mcp` — 122 unit + 6/3/3/15/1 integration (wiremock + fixture-server suites included), all pass - [x] `RUSTDOCFLAGS="-D warnings" cargo doc -p stella-mcp --no-deps` — clean (verifies the private-link fix) - [x] Docs updated where behavior changed (README Layout row) - [x] CLA signed ## Nothing left behind - [x] There is nothing: everything I noticed is fixed in this PR (the remaining main breaks — `stella-pipeline` compile, Makefile duplicate target — are already owned by the coordinator's separate PR) ## Anything reviewers should know? Do not merge on green checks alone — Actions is down; the checklist above is the verification record.
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.
A correction to the premise, which is the fix
The issue reports MCP schemas as forwarded "uncapped". That is not quite right — and the correction is where the real defect is.
crate::client::ingestalready caps every tool individually:What nothing bounds is the aggregate. 256 tools × a 2,000-character description is ~512 KB of third-party prose from a single server, at position 0 of every request, before schemas are counted — and every per-item cap holds the whole way. The caps are per-item; the cost is the sum.
Measured through the real
tools/listpath: 40 tools with 2 KB descriptions — every ingest cap satisfied — advertise 81,440 bytes from one server.Change
MAX_SERVER_SCHEMA_BYTES(32 KB, ~9k tokens), applied per server.The first tool of a server is always admitted however large: a server whose single tool exceeds the budget should advertise that tool, not vanish, and ingest's per-tool caps already bound it.
Applied to the sorted segment, which matters for more than tidiness. Sorting by namespaced name groups each server's tools into a contiguous run and fixes the order inside it, so the truncation point is a function of the tool names rather than of client order or connection timing. Budgeting an unsorted list would have re-lost the cross-process byte stability the sort (#1848, this PR's base) exists to buy.
over_budget_servers()reports what was cut — deliberately separate fromover_advertising_servers(), because those are two different walls and a reader needs to know which one was hit: 300 tools trips the count cap, twelve verbose ones trip this.One
budget_segmentfold serves both callers, and onemcp_segmentbuilds the input for both. No second copy of either rule for reporting — that is the drift #1613 was filed for, and I would rather not re-file it.Witness
one_chatty_server_cannot_spend_the_whole_prefixdrives a 40-tool server through the real transport and asserts four things, because the first alone is satisfiable by simply dropping the server:On the old code:
an_ordinary_server_is_not_trimmedis the other direction — the budget must be invisible for the servers people actually run, so a fix that trimmed everything cannot pass.Clippy and
cargo fmt --checkclean.Not done, deliberately: the lean-catalog default
The issue's other half asks to flip
STELLA_LEAN_TOOLSon by default. Its own fix direction says to measure first:That is a bench measurement, not a code change, and flipping it unmeasured is exactly what the issue warns against. Left open — hence
Refs, notCloses.Base
Stacked on #1875 (#1848, the schema sort). The sort is a precondition here: the budget is applied to the sorted segment so the truncation point is deterministic.
Refs #1856, #1848
Summary by Sourcery
Enforce a per-server byte budget on MCP tool schemas while preserving deterministic tool ordering and improving diagnostics for over-budget servers.
New Features:
over_budget_serversAPI that reports which servers had tools trimmed and how many were dropped.Enhancements:
mcp_segmentand apply budgeting via a sharedbudget_segmenthelper used by both schema generation and diagnostics.Tests: