feat: update the connect tool based on connectivity status #118
Conversation
nirinchev
requested review from
gagik and
fmenezes
and removed request for
gagik
| @@ -21,7 +21,7 @@ export class Server { | |||
| public readonly session: Session; | |||
| private readonly mcpServer: McpServer; | |||
| private readonly telemetry: Telemetry; | |||
| private readonly userConfig: UserConfig; | |||
| public readonly userConfig: UserConfig; | |||
There was a problem hiding this comment.
Still not super happy with where we are at wrt configs, but this was a stopgap - there were a bunch of places in the tests where we were updating fields in the global config expecting this to be reflected in the server config, which only worked by accident.
| telemetry: this.userConfig.telemetry, | ||
| logPath: this.userConfig.logPath, | ||
| connectionString: this.userConfig.connectionString | ||
| ? "set; no explicit connect needed, use switch-connection tool to connect to a different connection if necessary" | ||
| : "not set; before using any mongodb tool, you need to call the connect tool with a connection string", | ||
| connectOptions: this.userConfig.connectOptions, |
There was a problem hiding this comment.
These are the config options we're exposing as a resource
tests/integration/helpers.ts
Outdated
| @@ -22,13 +22,14 @@ export interface IntegrationTest { | |||
| mcpServer: () => Server; | |||
| } | |||
|
|
|||
| export function setupIntegrationTest(userConfig: UserConfig = config): IntegrationTest { | |||
| export function setupIntegrationTest(userConfigGetter: () => UserConfig = () => config): IntegrationTest { | |||
There was a problem hiding this comment.
This was necessary to allow the user config to be constructed as part of the test initialization rather than statically.
| randomDbName = new ObjectId().toString(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| if (mcpServer) { |
There was a problem hiding this comment.
Moved this from the mongodb helpers because the session cleanup seemed like something common.
Pull Request Test Coverage Report for Build 14670404051Warning: 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 |
tests/integration/helpers.ts
Outdated
| @@ -22,13 +22,14 @@ export interface IntegrationTest { | |||
| mcpServer: () => Server; | |||
| } | |||
|
|
|||
| export function setupIntegrationTest(userConfig: UserConfig = config): IntegrationTest { | |||
| export function setupIntegrationTest(userConfigGetter: () => UserConfig = () => config): IntegrationTest { | |||
There was a problem hiding this comment.
| export function setupIntegrationTest(userConfigGetter: () => UserConfig = () => config): IntegrationTest { | |
| export function setupIntegrationTest(getUserConfig: () => UserConfig = () => config): IntegrationTest { |
nit
tests/integration/helpers.ts
Outdated
| let mcpClient: Client | undefined; | ||
| let mcpServer: Server | undefined; | ||
|
|
||
| let randomDbName: string; | ||
|
|
||
| beforeAll(async () => { | ||
| const userConfig = userConfigGetter(); |
There was a problem hiding this comment.
| const userConfig = userConfigGetter(); | |
| const userConfig = getUserConfig(); |
|
I'm going to go ahead and merge this, even though we're going to rework it once more. |
This turned out to be a lot more involved than anticipated and revealed a handful of upstream bugs, which we've worked around.
There are two main changes in this PR:
configresource, listing a redacted subset of the UserConfig options. Right now it lists the following:connecttool. Now it updates its metadata based on whether we have a connection string configured and whether it has run before. If we have connected already or have a connection string, it changes its name toswitch-connectionand updates its metadata to more clearly hint at the model in which situations it should be invoked.Based on my testing with Claude, Cursor, and Windsurf, this works pretty solidly, but would appreciate if others could give it a go.
TODO
Fixes #92
Fixes #95