-
Notifications
You must be signed in to change notification settings - Fork 128
feat(atlas-local): Adds Atlas Local Connect Deployment tool #612
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
); | ||
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' |
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.
added to fix test fail
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.
Hm... should this be the case across the board? I.e. on macOS GHA runners, I'd expect this tool to be unavailable, no?
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.
Good point @nirinchev , i was only considering how it runs in cicd. Changing it to expect either depending on testing environment
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 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?
Didn't see that the base wasn't main. I see the tool now. Resolving the comment! |
📊 Accuracy Test Results📈 Summary
📎 Download Full HTML Report - Look for the Report generated on: 10/6/2025, 4:09:12 PM |
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 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.`; |
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.
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.
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.
@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.
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.`; | |
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.`; |
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.
Sounds reasonable to me!
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"), |
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.
Why does this take id if we're not surfacing the id in list-deployments?
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 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
|
||
// 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", () => { |
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 we move this .skipIf
here?
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"); | ||
}); |
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.
Consider using the validateToolMetadata
helper to simplify tests like this.
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" |
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: should we be translating these errors? I don't imagine customers care too deeply about docker containers.
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.
sounds good, added error handling to atlas local tools. This can be expanded in future to translate more errors
const elements = getResponseElements(response.content); | ||
expect(elements.length).toBeGreaterThanOrEqual(1); | ||
expect(elements[0]?.text).toContain( | ||
'Successfully connected to Atlas Local deployment "' + deploymentName + '".' |
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.
'Successfully connected to Atlas Local deployment "' + deploymentName + '".' | |
`Successfully connected to Atlas Local deployment "${deploymentName}".` |
); | ||
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' |
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.
Hm... should this be the case across the board? I.e. on macOS GHA runners, I'd expect this tool to be unavailable, no?
} | ||
); | ||
|
||
it.skipIf(isMacOSInGitHubActions)("should connect to a deployment when calling the tool", async ({ signal }) => { |
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.
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.
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! 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.
Proposed changes
Implements tool atlas-local-connect-deployment
Adds integration tests
Adds accuracy tests
Adds hint for connecting for local deployment
Checklist