chore: add arg-parser and put the config under test MCP-86 #429

kmruiz · 2025-08-07T13:34:14Z

Proposed changes

Use mongosh argument parser to generate the connection string that is going to be used to connect to MongoDB, aligning with how mongosh does it and making sure we support additional authentication mechanisms. It removes a bunch of flags and types not necessary now with the consolidation.

There will be integration tests for each of the supported connection types when playing each of their tasks. Specific support is out of the scope of this PR.

Checklist

I have signed the MongoDB CLA

This commit is biggish because it adds a big chunk of tests that weren't there before. While it would be arguably that testing every flag might be too much, because I'm changing how arguments are parsed, I want to have this as a safeline.

For help we are refering to the README.md, if we want something more fancy we porobably want to have better documentation generation so we don't maintain two files (here and the README.md)

The driver options can not be sent by the agent, so we are going to use the ones specified in the CLI arguments / ENV Vars. This is relevant because new connections should use any inherited configuration like TLS settings or FIPS by default.

This is necessary when connecting for the first time.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

This PR refactors the configuration management system to use the @mongosh/arg-parser library for generating MongoDB connection strings. This change aligns with how mongosh handles connection arguments and ensures support for additional authentication mechanisms. The refactoring removes several deprecated flags and types that are no longer necessary with the consolidated approach.

Uses mongosh argument parser to standardize connection string generation and authentication support
Consolidates configuration by removing the separate ConnectOptions interface and moving connection options to driver-level configuration
Removes hardcoded connection options from test files and connection calls throughout the codebase

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/common/config.ts	Major refactor to use mongosh arg-parser with new configuration parsing and driver options setup
tests/unit/common/config.test.ts	Comprehensive test suite for the new configuration parsing functionality
src/index.ts	Added FIPS mode support, help/version modes, and system CA loading
tests/unit/common/session.test.ts	Removed deprecated connectOptions usage in tests
tests/integration/tools/mongodb/connect/connect.test.ts	Removed deprecated connectOptions usage in integration tests
tests/integration/common/connectionManager.test.ts	Removed deprecated connectOptions usage and import
src/tools/mongodb/mongodbTool.ts	Simplified connection call by removing connectOptions spread
src/tools/atlas/connect/connectCluster.ts	Simplified connection call by removing connectOptions spread
src/server.ts	Simplified connection call by removing connectOptions spread
src/resources/common/config.ts	Updated to use new driverOptions export instead of connectOptions
src/lib.ts	Removed ConnectOptions from exports
src/common/connectionManager.ts	Refactored to use new driverOptions configuration
package.json	Added @mongosh/arg-parser dependency

src/common/config.ts

himanshusinghs · 2025-08-08T11:24:39Z

src/common/config.ts

+// From: https://github.com/mongodb-js/mongosh/blob/main/packages/cli-repl/src/arg-parser.ts
+const OPTIONS = {


Would it be an option to import it from the args-parser package instead?

I don't think so:

args-parser does not expose it, we would need to update it, which is fine but

we don't support all arguments (eval for example) so we would still need to filter or do some post processing.

Yea but I still think its just a pretty long list to copy here instead of something like this?

Omit<Options, "eval" | "or" | "others">

that way you at-least have one source of truth.

While they are similar, we do support more options (for the legacy options and Atlas credentials) and I feel being explicit on what we do support additionally and what we don't support is better. If we depend on mongosh and we add more mongosh specific flags we would inherit them without validation.

src/common/config.ts

himanshusinghs · 2025-08-08T11:42:07Z

src/index.ts

+        console.log("For usage information refer to the README.md:");
+        console.log("https://github.com/mongodb-js/mongodb-mcp-server?tab=readme-ov-file#quick-start");


I think these should be console.error or console.warn. We shouldn't be logging to stdio as that is reserved for transport.

It's reserved once the transport is started, but this is done before the transport is started. This is for developers using --help to understand the arguments in their console, it's not expected to be used in a mcp.json file or similar.

Agreed, but then you might wanna verify the behaviour of transport on start when you already have pushed some non-json rpc messages.

These functions early exit, so there wouldn't be a situation where we issue a warning and then start the transport. That's why the signature of the function is void | never.

src/common/config.ts

coveralls · 2025-08-08T15:19:09Z

Pull Request Test Coverage Report for Build 16830919759

Details

195 of 254 (76.77%) changed or added relevant lines in 6 files are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.2%) to 81.516%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/resources/common/config.ts	1	2	50.0%
src/common/config.ts	190	198	95.96%
src/index.ts	0	50	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/index.ts	1	0.83%
src/common/session.ts	6	84.68%

Totals
Change from base Build 16826971263:	0.2%
Covered Lines:	3695
Relevant Lines:	4507

💛 - Coveralls

gagik · 2025-08-11T08:00:29Z

src/index.ts

@@ -1,11 +1,37 @@
 #!/usr/bin/env node

+let fipsError: Error | undefined;


why not just throw the error right away? would be good to avoid global variables if we can

Because in line 108 we do more checks that need to be done after this.

We first enable FIPS before anything.

Then after we load the flags, we check different configurations and then give a specific error message (for example, shared openssl vs bundled)

Then we show FIPS error details based on the actual configuration.

gagik · 2025-08-11T08:00:53Z

src/index.ts

@@ -78,3 +104,39 @@ main().catch((error: unknown) => {
    });
    process.exit(1);
 });
+
+function assertFIPSMode(): void | never {


Suggested change

function assertFIPSMode(): void | never {

function assertFIPSMode(): void {

nit: same below

Why? void | never makes explicit that this function either doesn't return anything or exits the process.

gagik · 2025-08-11T08:03:02Z

src/index.ts

@@ -78,3 +104,39 @@ main().catch((error: unknown) => {
    });
    process.exit(1);
 });
+
+function assertFIPSMode(): void | never {


would be good to keep the index.ts file as small as possible and move these to their own file(s)

I personally prefer to have all initialisation functions that can exit the process in the main file so it's clear that outside this file no one should do process.exit.

kmruiz added 3 commits August 8, 2025 11:37

chore: load the systemCA and check for FIPS, help and version

1164b91

For help we are refering to the README.md, if we want something more fancy we porobably want to have better documentation generation so we don't maintain two files (here and the README.md)

kmruiz force-pushed the chore/mcp-86 branch from c6536b0 to c79a479 Compare August 8, 2025 09:40

kmruiz added 2 commits August 8, 2025 11:50

chore: generate the connection string properly on first connection

8c2da63

This is necessary when connecting for the first time.

chore: some minor refactors and linter checks

ccd9d41

kmruiz marked this pull request as ready for review August 8, 2025 10:16

Copilot AI review requested due to automatic review settings August 8, 2025 10:16

kmruiz requested a review from a team as a code owner August 8, 2025 10:16

This comment was marked as outdated.

Sign in to view

kmruiz and others added 2 commits August 8, 2025 12:18

Update tests/unit/common/config.test.ts

09da3b8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

chore: enable FIPS before doing anything else

c95bf72

kmruiz requested a review from Copilot August 8, 2025 11:00

Copilot AI reviewed Aug 8, 2025

View reviewed changes

src/common/config.ts Show resolved Hide resolved

src/common/config.ts Show resolved Hide resolved

src/common/config.ts Show resolved Hide resolved

himanshusinghs requested changes Aug 8, 2025

View reviewed changes

himanshusinghs reviewed Aug 8, 2025

View reviewed changes

src/common/config.ts Show resolved Hide resolved

himanshusinghs reviewed Aug 8, 2025

View reviewed changes

src/common/config.ts Show resolved Hide resolved

kmruiz added 4 commits August 8, 2025 14:34

chore: move config validation to the actual config

dbd633e

chore: add tests for the moved validation

4c421c2

chore: add a comment to explain the shift

bc346cf

chore: make eslint happy for a happy weekend

3885aa7

kmruiz requested a review from himanshusinghs August 11, 2025 07:26

gagik reviewed Aug 11, 2025

View reviewed changes

		// From: https://github.com/mongodb-js/mongosh/blob/main/packages/cli-repl/src/arg-parser.ts
		const OPTIONS = {

		console.log("For usage information refer to the README.md:");
		console.log("https://github.com/mongodb-js/mongodb-mcp-server?tab=readme-ov-file#quick-start");

		@@ -1,11 +1,37 @@
		#!/usr/bin/env node

		let fipsError: Error \| undefined;

	function assertFIPSMode(): void \| never {
	function assertFIPSMode(): void {

chore: add arg-parser and put the config under test MCP-86 #429

Are you sure you want to change the base?

chore: add arg-parser and put the config under test MCP-86 #429

Conversation

kmruiz commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16830919759

Details

💛 - Coveralls

Uh oh!

gagik Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kmruiz commented Aug 7, 2025 •

edited

Loading

coveralls commented Aug 8, 2025 •

edited

Loading

gagik Aug 11, 2025 •

edited

Loading

gagik Aug 11, 2025 •

edited

Loading

gagik Aug 11, 2025 •

edited

Loading