-
Notifications
You must be signed in to change notification settings - Fork 128
feat(search): Add a new tool to list search and vector search indexes MCP-235 #610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📊 Accuracy Test Results📈 Summary
📊 Baseline Comparison
📎 Download Full HTML Report - Look for the Report generated on: 10/6/2025, 1:52:38 PM |
Pull Request Test Coverage Report for Build 18338555347Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new MongoDB MCP tool called "list-search-indexes" that allows users to list search and vector search indexes for a specific collection. The tool detects Atlas Search support by catching exceptions and provides appropriate error messages for unsupported clusters.
Key changes:
- New search indexes listing tool with comprehensive test coverage
- Enhanced test infrastructure to support Atlas Local containers via Docker
- Refactored shared utilities and improved test organization
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/tools/mongodb/search/listSearchIndexes.ts |
Core implementation of the new list-search-indexes tool |
tests/integration/tools/mongodb/search/listSearchIndexes.test.ts |
Integration tests for the new tool with Atlas Local support |
tests/integration/tools/mongodb/mongodbHelpers.ts |
Enhanced test helpers to support Docker containers and search clusters |
tests/accuracy/listSearchIndexes.test.ts |
Accuracy tests for the new tool |
src/tools/mongodb/tools.ts |
Registers the new tool in the MongoDB tools array |
tests/integration/transports/stdio.test.ts |
Updates tool count expectation |
tests/integration/helpers.ts |
Adds shared sleep utility function |
tests/integration/tools/atlas/clusters.test.ts |
Uses shared sleep utility |
package.json |
Adds testcontainers dependencies |
.github/workflows/code-health.yml |
Adds Docker setup for CI |
.github/workflows/code-health-fork.yml |
Adds Docker setup for fork CI |
tests/integration/tools/mongodb/search/listSearchIndexes.test.ts
Outdated
Show resolved
Hide resolved
tests/integration/tools/mongodb/search/listSearchIndexes.test.ts
Outdated
Show resolved
Hide resolved
tests/integration/tools/mongodb/search/listSearchIndexes.test.ts
Outdated
Show resolved
Hide resolved
tests/integration/tools/mongodb/search/listSearchIndexes.test.ts
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
de83f4d to
8f2f8cc
Compare
It detects if the cluster supports search indexes the same way we do in all other tooling: by catching the exception. In some old clusters it might still return an empty list for unsupported custers, but we can't really know for sure. As we depend on mongot, we need to use the Atlas local image, which is not supported by mongodb-runner. Instead, we use testcontainers, that allows to run any docker image, which is enough for our use case. For now only run docker tests in Ubuntu * macos-latest does not support nested virtualisation yet * we don't have windows containers for atlas images
3656451 to
a1c2b82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: Why is this a separate tool instead of bundling the response in the collection-indexes tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is that not all environments support search indexes, and in all other tools we have ordinary and search indexes separated, so it's more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can handle that internally though - it's awkward to ask for indexes and get only some of them, then have to ask again to get the search indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not only about being able to handle it, it's about consistency with other tools. The driver, the shell and Compass show Search Indexes and ordinary indexes in different places. They are indexes, yeah, but they serve different purposes. And that is on different tools I don't think will make the agent only call one of the tools, however I can validate it with an accuracy test:
"show me all the indexes on collection whatever"
and it should call both "list-indexes" and "list-search-indexes" tools (in any order).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a strong argument for consistency across what I see as vastly different tools - the API surface of drivers and shell is significantly larger than what we have in MCP because they have different consumers. Besides the consistency point, is there a reason why we'd want these to be different tools? While we eventually decided against doing a far-reaching consolidation, we should still be mindful of the number of tools the server exposes and try to minimize that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all users that have access to MongoDB Search want to use Search (either Vector or Text), so having 2 separate tools ensures that they can disable it. There are reasonable reasons to not use Search (from permissions to business reasons), so I don't see a strong argument to merge them either. IIRC, the consensus before doing the consolidation is to at least make sure users can disable specific tools if they don't require them. If we consolidate only these 2, users won't be able to disable from their IDE the ability to list search indexes.
If you have a strong opinion on whether this 2 tools should be one, they can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True - the tool disabling is a valid point, though I wonder if we're not being overly granular here. Let's have a team call to align on an approach - in my mind having the search functionality integrated into existing tools would be the preferred DX, but we should weigh the pros and cons as that decision would influence the design of multiple related tickets.
tests/integration/tools/mongodb/search/listSearchIndexes.test.ts
Outdated
Show resolved
Hide resolved
Now, there is a new class, MongoDBClusterProcess, that knows how to start and stop a cluster based on the provided configuration. This decouples the test setup and simplifies any future approaches to consolidate running strategies into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we end up keeping this as a separate tool, you probably want to override the verifyAllowed method, the same as here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should link search indexes to the voyageAPI key, as there are other reasons other than embeddings to enable this tool. I think the approach should be testing if Search is supported (by listing search indexes) when connecting, and then disable tools based on the connection status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yea the cluster based disabling certainly makes sense. I was discussing a concern with Nikola about the disconnected experience when disabling part of search related functionality, happy to pull it up in our call but its not blocking so nothing to worry about there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be return super.handleError(error)?
Also if we want to keep it should we annotate this with isError: true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair! Changing to super.handleError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would be inserting as many documents as the retries happening. Is there another way to figure out if search is ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, the only way to ensure it works is by inserting a document and creating a search index. Listing indexes might return an empty because the system is enabled, but it's still not active.
Proposed changes
It detects if the cluster supports search indexes the same way we do in all other tooling: by catching the exception. In some old clusters it might still return an empty list for unsupported clusters, but we can't really know for sure.
Checklist