Search code, repositories, users, issues, pull requests...


 feat: Implements atlasLocal tool atlas-local-connect-deployment

c7cf37b


 Adds integration tests

0d0581d


 adds accuracy tests

0c53f48


 Adds hints to llm on which connect tool to use and fixes atlas test

ba884f8

cveticm

cveticm commented

tests/integration/tools/atlas/clusters.test.ts Outdated

);

expect(elements[1]?.text).toContain(

'Please use one of the following tools: "atlas-connect-cluster", "connect" to connect to a MongoDB instance'

'Please use one of the following tools: "atlas-connect-cluster", "atlas-local-connect-deployment", "connect" to connect to a MongoDB instance'

Copy link

Collaborator Author

@cveticm cveticm Oct 6, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to fix test fail

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... should this be the case across the board? I.e. on macOS GHA runners, I'd expect this tool to be unavailable, no?

Copy link

Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @nirinchev , i was only considering how it runs in cicd. Changing it to expect either depending on testing environment

@cveticm cveticm changed the title ~~(削除) feat: Implements atlasLocal tool atlas-local-connect-deployment (削除ここまで)~~ (追記) feat(atlas-local): Adds Atlas Local Connect Deployment tool (追記ここまで)

@cveticm cveticm marked this pull request as ready for review

October 6, 2025 15:22

@cveticm cveticm requested a review from a team as a code owner

October 6, 2025 15:22

kmruiz

kmruiz requested changes

Copy link

Collaborator

@kmruiz kmruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself looks fine, but there are some things to consider before merging.

Also I think this feature should be feature toggled and disabled by default until we implement atlas-local-list-deployments.

@blva, WDYT?

tests/accuracy/connectDeployment.test.ts Show resolved Hide resolved

src/tools/atlasLocal/connect/connectDeployment.ts Outdated Show resolved Hide resolved

@Luke-Sanderson

Copy link

Collaborator

Luke-Sanderson commented Oct 6, 2025

The code itself looks fine, but there are some things to consider before merging.

Also I think this feature should be feature toggled and disabled by default until we implement atlas-local-list-deployments.

@blva, WDYT?

I believe the atlas-list-deployments tool was added here #520

@kmruiz

Copy link

Collaborator

kmruiz commented Oct 6, 2025

Didn't see that the base wasn't main. I see the tool now. Resolving the comment!

kmruiz

kmruiz approved these changes

@himanshusinghs himanshusinghs added the accuracy-tests label

@github-actions GitHub Actions

Copy link

Contributor

github-actions bot commented Oct 6, 2025

📊 Accuracy Test Results

📈 Summary

Metric	Value
Commit SHA	`5095c7f7df943309ce8cbe82f14d755c58232518`
Run ID	`ce3d82f9-8dac-4f82-8169-f402af59a399`
Status	done
Total Prompts Evaluated	78
Models Tested	1
Average Accuracy	99.0%
Responses with 0% Accuracy	0
Responses with 75% Accuracy	3
Responses with 100% Accuracy	75

📎 Download Full HTML Report - Look for the accuracy-test-summary artifact for detailed results.

Report generated on: 10/6/2025, 4:09:12 PM

himanshusinghs

himanshusinghs requested changes

src/tools/atlasLocal/connect/connectDeployment.ts Outdated Show resolved Hide resolved

tests/integration/tools/atlas-local/connectDeployment.test.ts Outdated Show resolved Hide resolved


 Moves cleanup to afterEach block

975909e

@cveticm cveticm requested a review from himanshusinghs

October 6, 2025 16:55

nirinchev

nirinchev approved these changes

Copy link

Collaborator

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid - I have mostly nitty comments/suggestions.

src/common/connectionErrorHandler.ts Outdated

if (atlasConnectTool) {

llmConnectHint = `Note to LLM: prefer using the "${atlasConnectTool.name}" tool to connect to an Atlas cluster over using a connection string. Make sure to ask the user to specify a cluster name they want to connect to or ask them if they want to use the "list-clusters" tool to list all their clusters. Do not invent cluster names or connection strings unless the user has explicitly specified them. If they've previously connected to MongoDB using MCP, you can ask them if they want to reconnect using the same cluster/connection.`;

} else if (atlasLocalConnectTool) {

llmConnectHint = `Note to LLM: prefer using the "${atlasLocalConnectTool.name}" tool for connecting to local MongoDB deployments. Ask the user to specify a deployment name or use "atlas-local-list-deployments" to show available local deployments. Do not invent deployment names unless the user has explicitly specified them. If they've previously connected to a local MongoDB deployment using MCP, you can ask them if they want to reconnect using the same deployment.`;

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I wonder if we want to be that prescriptive - my understanding is that this tool would be available as long as the user has docker installed, regardless of whether they're connecting to a local deployment or not. I guess we'd need to vibe check this in the real world.

Copy link

Collaborator Author

@cveticm cveticm Oct 7, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nirinchev Thoughts on these actions?:

Change the logic such that both atlasConnectTool and atlasLocalConnectTool hints can be provided if both tools are available. (currently only atlasConnectTool hint is printed if the tool is avb)
Change atlasLocalConnectTool hint to not have preference for it - since the connectTool can also be used to connect to local deployments if given the connection string.

Suggested change

llmConnectHint = `Note to LLM: For Atlas Local deployments, ask the user to either provide a connection string, specify a deployment name, or use "atlas-local-list-deployments" to show available local deployments. If a deployment name is provided, prefer using the "${atlasLocalConnectTool.name}" tool. If a connection string is provided, prefer using the "${connectTool.name}" tool. Do not invent deployment names or connection strings unless the user has explicitly specified them. If they've previously connected to an Atlas Local deployment using MCP, you can ask them if they want to reconnect using the same deployment.`;

Copy link

Collaborator

@nirinchev nirinchev Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me!

src/tools/atlasLocal/connect/connectDeployment.ts Outdated

protected description = "Connect to a MongoDB Atlas Local deployment";

public operationType: OperationType = "connect";

protected argsShape = {

deploymentIdOrName: z.string().describe("Name or ID of the deployment to connect to"),

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this take id if we're not surfacing the id in list-deployments?

Copy link

Collaborator Author

@cveticm cveticm Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool can handle both id and name but I agree, since we don't expose the container id (even in cli) I changed the input name and description to avoid confusion

tests/integration/tools/atlas-local/connectDeployment.test.ts Outdated

// Docker is not available on macOS in GitHub Actions

// That's why we skip the tests on macOS in GitHub Actions

describe("atlas-local-connect-deployment", () => {

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this .skipIf here?

tests/integration/tools/atlas-local/connectDeployment.test.ts Outdated

Comment on lines 57 to 65

it.skipIf(isMacOSInGitHubActions)("should have correct metadata", async ({ signal }) => {

await waitUntilMcpClientIsSet(integration.mcpServer(), signal);

const { tools } = await integration.mcpClient().listTools();

const connectDeployment = tools.find((tool) => tool.name === "atlas-local-connect-deployment");

expectDefined(connectDeployment);

expect(connectDeployment.inputSchema.type).toBe("object");

expectDefined(connectDeployment.inputSchema.properties);

expect(connectDeployment.inputSchema.properties).toHaveProperty("deploymentIdOrName");

});

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using the validateToolMetadata helper to simplify tests like this.

tests/integration/tools/atlas-local/connectDeployment.test.ts Outdated

const elements = getResponseElements(response.content);

expect(elements.length).toBeGreaterThanOrEqual(1);

expect(elements[0]?.text).toContain(

"Docker responded with status code 404: No such container: non-existent"

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: should we be translating these errors? I don't imagine customers care too deeply about docker containers.

Copy link

Collaborator Author

@cveticm cveticm Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, added error handling to atlas local tools. This can be expanded in future to translate more errors

tests/integration/tools/atlas-local/connectDeployment.test.ts Outdated

const elements = getResponseElements(response.content);

expect(elements.length).toBeGreaterThanOrEqual(1);

expect(elements[0]?.text).toContain(

'Successfully connected to Atlas Local deployment "' + deploymentName + '".'

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

'Successfully connected to Atlas Local deployment "'+deploymentName+'".'

`Successfully connected to Atlas Local deployment "${deploymentName}".`

tests/integration/tools/atlas/clusters.test.ts Outdated

);

expect(elements[1]?.text).toContain(

'Please use one of the following tools: "atlas-connect-cluster", "connect" to connect to a MongoDB instance'

'Please use one of the following tools: "atlas-connect-cluster", "atlas-local-connect-deployment", "connect" to connect to a MongoDB instance'

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... should this be the case across the board? I.e. on macOS GHA runners, I'd expect this tool to be unavailable, no?

nirinchev

nirinchev reviewed

tests/integration/tools/atlas-local/connectDeployment.test.ts Outdated

}

);

it.skipIf(isMacOSInGitHubActions)("should connect to a deployment when calling the tool", async ({ signal }) => {

Copy link

Collaborator

@nirinchev nirinchev Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider creating a separate suite for this case with its own setup/cleanup. Something like:

describe("when local deployments are available", () => {
 beforeEach(() => {
 // create deployment
 });
 afterEach(() => {
 // cleanup deployment
 });
 it("should connect to a deployment...", () => {
 // test without the setup
 });
});

Also, might be a good idea to test when there are more than one deployments that we connect to the correct one. As things stand today, all tests will pass if we disregard the deployment id/name argument and instead connect to the first available deployment.

jeroenvervaeke

jeroenvervaeke approved these changes

himanshusinghs

himanshusinghs approved these changes

cveticm added 5 commits

October 7, 2025 13:28


 adds error handling for when container non-existent

9ad0257


 tool input doesnt take deployment id

fa6d616


 cluster integ test to handle macos in github runner

a169d63


 restructures test

6bf04ce


 Improves llmConntectHint

74fe9fe

nirinchev

nirinchev approved these changes

Copy link

Collaborator

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Feel free to update the connect tool hints per your suggestion - as long as they pass a vibe check when testing manually, I'm happy shipping it.

@cveticm cveticm merged commit cb5d335 into feat-MCP-40