feat(mcp): let operators disable individual built-in tools - #668
Open
vishal-bala wants to merge 4 commits into
Open
feat(mcp): let operators disable individual built-in tools#668vishal-bala wants to merge 4 commits into
vishal-bala wants to merge 4 commits into
Conversation
vishal-bala
force-pushed
the
feat/mcp-builtin-tool-toggle
branch
from
August 6, 2026 13:42
264f13e to
20dc663
Compare
vishal-bala
marked this pull request as ready for review
August 7, 2026 15:36
`list-indexes`, `search-records`, and `upsert-records` all registered unconditionally, so an operator who wanted a narrower tool surface had no way to get one -- a deployment that should never advertise writes still published `upsert-records` whenever any binding was writable, and a single-purpose server still published discovery. `server.builtin_tools` maps a built-in name to enabled/disabled. Omitted names stay enabled, so existing configs are unaffected. Only the three real names are accepted: `search_records` with underscores fails at startup rather than silently disabling nothing while reading as though it had. Two tool-set shapes are valid config but unusable in practice, and both would otherwise be silent. Disabling everything leaves a server that connects and offers nothing. Disabling discovery on a multi-index server leaves `search-records` demanding a logical index id that clients have no way to learn, since its own description tells them to call `list-indexes` first. Both now log a warning at startup; neither is fatal, because an operator may be mid-rollout. The empty-surface warning deliberately does not name a cause, since `upsert-records` can also be absent because every binding is read-only. The multi-index discovery case had no coverage, so this adds it along with the negative case: a sole binding makes the index argument default, so disabling discovery there is legitimate and stays quiet.
vishal-bala
force-pushed
the
feat/mcp-builtin-tool-toggle
branch
from
August 7, 2026 15:45
20dc663 to
04fbfad
Compare
This was referenced Aug 7, 2026
Three review findings, all consequences of making the built-in tool set configurable: the rest of the surface still described the old, always-on one. **`list-indexes` misreported upsert availability.** `upsert_available` came from the binding's read-only state alone, so a writable binding on a server with `upsert-records` disabled reported `true` for a tool the client cannot call. It is a client-facing claim about what is callable, so it now also requires the tool to be published. Before this change the tool was registered whenever any binding was writable, which is why the old derivation was correct and is not any more. **The search description pointed at a missing tool.** On a multi-index server with `list-indexes` disabled, `search-records` still told clients to call it first -- while `index` remains required and the ids are otherwise unlearnable, so the contract was not just wrong but unsatisfiable. The description now names the ids inline in that case. The startup warning stays, because inlining ids is a fallback rather than an endorsement of the shape. **An edited config looked applied when it was not.** Tools register once per process and `_tools_registered` deliberately survives teardown, since re-registering the same names on the FastMCP object is invalid. That was harmless while the tool set derived only from binding state, which is re-derived at startup. Now it depends on config, and `startup()` re-reads that file -- so a stop/start against an edited config silently keeps the old tools. The server fingerprints the config its tools were built from and warns on a mismatch. The dangerous direction is an operator disabling a tool, restarting, and believing it is gone. Each fix has a test that fails when only that fix is reverted, plus a control for the enabled path. Docs updated so the advertised behavior matches.
`register_search_tool` gained a keyword-only `index_ids` in the previous commit, but `test_read_only_mode_excludes_upsert_tool` still patched it with a two-argument lambda. Startup raised `TypeError: unexpected keyword argument 'index_ids'` before any read-only assertion ran, so the failure was in the stub rather than the behavior under test. I fixed the two unit-test stubs when changing the signature and did not grep the integration tests, which is the actual mistake -- monkeypatched stubs only fail at call time, so nothing flagged it locally. All five patched call sites are now checked; `register_list_indexes_tool` and `register_upsert_tool` are unchanged, so their stubs still match. Verified by reverting the stub to reproduce the exact CI TypeError, then running the full MCP integration suite: 56 passed, 2 skipped.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c4d3485. Configure here.
I fixed the unsatisfiable-contract problem for `search-records` and left `upsert-records` with the same defect. Both require an `index` once several bindings exist, so both are equally stranded when `list-indexes` is withheld -- and upsert was arguably worse off, since its description never mentioned discovery at all, leaving a client no route to the ids. `register_upsert_tool` now takes the same `index_ids` and appends them to its description. The condition is computed once in `_register_tools` and passed to both, rather than derived separately at each call, so the two cannot drift apart again the way they just did. The unusable-surface warning was also keyed on `search-records` alone, so a write-only surface with discovery disabled stayed silent. It now reports whichever index-requiring tools are actually published, and names them. Each half has a test that fails when only that half is reverted: one asserting upsert receives the same ids as search, one asserting the warning fires and names `upsert-records` on a surface where search is disabled. Verified against the full MCP integration suite as well this time (56 passed, 2 skipped), since the last signature change broke a stub that only integration exercised.
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
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.

list-indexes,search-records, andupsert-recordsall registered unconditionally, so an operator who wanted a narrower tool surface had no way to get one. A deployment that should never advertise writes still publishedupsert-recordswhenever any binding was writable, and a single-purpose server still published discovery.The config
Omitted names stay enabled, so existing configs are unaffected. Only the three real names are accepted:
search_recordswith underscores fails at startup rather than silently disabling nothing while reading as though it had.Two unusable-but-valid tool surfaces now warn
Both shapes are legal config that presents to a client as a server that simply does not work, and both were previously silent:
upsert-recordscan also be absent because every binding is read-only.search-recordsdemanding a logical index id that clients have no way to learn — its own description tells them to calllist-indexesfirst.Neither is fatal, because an operator may be mid-rollout.
The multi-index discovery case had no coverage anywhere, so this adds it, along with the negative case: with a sole binding the
indexargument defaults, so disabling discovery there is legitimate and stays quiet. I verified by mutation that the new test fails when the warning condition is neutered.Verification
make check-types: cleanNote on sequencing
This is independently useful and mergeable on its own. It is also a prerequisite for the custom tool profiles work that follows, since a profile name must be checked against the built-in names this PR introduces.
Note
Medium Risk
Changes which MCP tools clients see and discovery/write advertising; misconfiguration can yield empty or hard-to-use tool surfaces, though defaults preserve existing behavior and validation plus warnings reduce silent failures.
Overview
Adds
server.builtin_toolsso operators can disable any oflist-indexes,search-records, orupsert-recordsindividually (omitted tools stay enabled). Unknown names fail at startup validation instead of being ignored.Registration gates each built-in on config;
list-indexessetsupsert_availableto false when upsert is disabled even if bindings are writable. On multi-index servers with discovery off, search and upsert tool descriptions inline logical index ids. Startup warnings cover empty tool lists, discovery disabled with multiple indexes, and config changes after tools were already registered (process restart required to change the published surface).Docs in
docs/concepts/mcp.mddescribe the config and behavior.Reviewed by Cursor Bugbot for commit f323df5. Bugbot is set up for automated code reviews on this repo. Configure here.