fix(stella-mcp): sort the MCP schema segment so the advertised toolset is byte-stable (#1848) - #1875
Merged
Merged
Conversation
…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
![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 GuideEnsures the advertised MCP tool schemas are emitted in a deterministic, cross-process-stable order by sorting the MCP segment by namespaced tool name while preserving the native-vs-MCP segment contract, and adds a test that asserts the serialized schema list is byte-identical regardless of MCP server connection order. Sequence diagram for deterministic MCP schemas emission ordersequenceDiagram
participant Caller
participant McpToolSet
participant NativeToolExecutor as Native
Caller->>McpToolSet: schemas()
activate McpToolSet
alt native segment present
McpToolSet->>Native: schemas()
Native-->>McpToolSet: Vec<ToolSchema> (native)
McpToolSet->>McpToolSet: extend(native_schemas)
end
McpToolSet->>McpToolSet: collect MCP ToolSchema into mcp
McpToolSet->>McpToolSet: mcp.sort_by(name)
McpToolSet->>McpToolSet: extend(mcp)
McpToolSet-->>Caller: Vec<ToolSchema> (native segment, then sorted MCP segment)
deactivate McpToolSet
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
macanderson
added a commit
that referenced
this pull request
Aug 6, 2026
…olset (#1856) (#1898) ## 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::ingest` **already caps every tool individually**: ```rust pub const MAX_TOOLS_PER_SERVER: usize = 256; pub(crate) const MAX_TOOL_DESCRIPTION_CHARS: usize = 2_000; // plus a per-tool inputSchema byte budget ``` 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/list` path: 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 from `over_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_segment` fold serves both callers, and one `mcp_segment` builds 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_prefix` drives a 40-tool server through the real transport and asserts four things, because the first alone is satisfiable by simply dropping the server: 1. the block is **bounded**; 2. the server is **trimmed, not silenced**; 3. every tool is **either advertised or counted as dropped** (the count is the whole diagnostic — an operator wondering why a tool is missing needs to be told); 4. the survivors are the **lexicographic prefix**, so the cut is deterministic. On the old code: ``` one server advertised 81440 bytes, over the 32768-byte budget test result: FAILED. 0 passed; 1 failed ``` `an_ordinary_server_is_not_trimmed` is the other direction — the budget must be invisible for the servers people actually run, so a fix that trimmed everything cannot pass. ``` $ cargo test -p stella-mcp test result: ok. 130 passed; 0 failed ``` Clippy and `cargo fmt --check` clean. ## Not done, deliberately: the lean-catalog default The issue's other half asks to flip `STELLA_LEAN_TOOLS` on by default. **Its own fix direction says to measure first**: > Measure lean-mode's activation rate on the bench corpus before flipping any default (each activation mutates the schema block = one prefix invalidation; the trade is ~70% smaller prefix vs ~4-5 invalidations/session). That is a bench measurement, not a code change, and flipping it unmeasured is exactly what the issue warns against. Left open — hence `Refs`, not `Closes`. ## 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: - Introduce a per-server schema byte budget limiting how much a single MCP server can contribute to the advertised toolset. - Expose an `over_budget_servers` API that reports which servers had tools trimmed and how many were dropped. Enhancements: - Refactor MCP tool collection into a sorted `mcp_segment` and apply budgeting via a shared `budget_segment` helper used by both schema generation and diagnostics. - Ensure the combined native and MCP tool schema list is byte-stable across processes regardless of server connection order, maintaining native-first ordering. Tests: - Add tests to verify schema byte stability across different server connection orders. - Add tests to ensure chatty servers are trimmed but not silenced and that ordinary servers are unaffected by the new budget. --------- Co-authored-by: Stella Test <test@stella.local>
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.
Problem
ToolRegistry::schemassorts by name, and its comment says exactly why:McpToolSet::schemasthen concatenated after that sorted list without re-sorting, handing the guarantee straight back toself.clientsorder 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 precisely the case the registry's sort exists for (invariant 7).
Change
The MCP segment is sorted by its namespaced name. That 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 (
routesis keyed on them), so the order is total — no tie for the sort to break arbitrarily and reintroduce the nondeterminism.Segments are preserved, not flattened. Native tools first, then MCP — that order is a deliberate contract ("the base layer the MCP set augments"), and a sort over the whole list would have made the new test pass while silently moving the native tools. The witness asserts the segment boundary too, so that shortcut cannot pass.
The two siblings the issue named
Both checked; neither needs a change, and the PR says why rather than leaving it to a reader:
CandidateMcpView::schemasconcatenates a sorted native list with a filtered view ofinner.schemas(). Filtering preserves relative order, so it inherits this fix.DiscoveryToolSet::discovery_schemasbuilds a literalvec![…]— ordered by construction, not by iteration.Witness
schemas_are_byte_identical_whatever_order_the_servers_connected_inbuilds two sets from the same two servers registered in opposite orders and compares the serialized schemas. Bytes, not fields — bytes are what the cache matches on, so bytes are what the assertion should be about.On the old code:
With the sort: passes.
Cost, stated
This changes the advertised order once, which is a one-time prompt-cache invalidation for any session live across the upgrade. That is the price of having the property at all, and it is paid once rather than on every restart — which is what the old behaviour risked.
Verification
cargo clippy -p stella-mcp --all-targets -- -D warningsandcargo fmt --checkclean.Closes #1848
Summary by Sourcery
Ensure MCP tool schemas are advertised in a deterministic, cross-process-stable order to keep prompt cache entries shareable across processes.
Bug Fixes:
Tests: