Conversation
Document the breaking changes for the @mongodb-js/mcp-tools-mongodb package extraction, including:
- Fix merge conflicts in package.json and src/web.ts - Recreate missing packages/tools-mongodb/package.json and tsconfig.json - Update pnpm-lock.yaml with new dependencies All checks pass.
# Conflicts: # api-extractor/reports/mongodb-mcp-server.public.api.md # api-extractor/reports/tools.public.api.md # api-extractor/reports/web.public.api.md # packages/atlas-telemetry/src/types.ts # src/tools/atlas/tools.ts # src/web.ts
# Conflicts: # src/setup/setupMcpServer.ts # src/setup/setupTelemetry.ts
2bc1b03 to
fd7d4e6
Compare
addaleax
left a comment
There was a problem hiding this comment.
No major concerns, and I imagine a lot of my comments aren't specific to this PR, but I've still done a full review here
| const NO_UNICODE_REGEX = /^[\x20-\x7E]*$/; | ||
| export const NO_UNICODE_ERROR = "String cannot contain special characters or Unicode symbols"; | ||
|
|
||
| export const CommonArgs = { | ||
| string: (): ZodString => z.string().regex(NO_UNICODE_REGEX, NO_UNICODE_ERROR), |
There was a problem hiding this comment.
Don't really like the naming or the error message here – if I see something like CommonArgs.string(), I would expect that to really correspond to a regular string. Also, saying "cannot contain Unicode symbols" is fairly confusing, since it's really not obvious which actual restriction that corresponds to (it can't be interpreted literally, since strings are by definition sequences of Unicode characters).
| const NO_UNICODE_REGEX = /^[\x20-\x7E]*$/; | |
| export const NO_UNICODE_ERROR = "String cannot contain special characters or Unicode symbols"; | |
| export const CommonArgs = { | |
| string: (): ZodString => z.string().regex(NO_UNICODE_REGEX, NO_UNICODE_ERROR), | |
| const ASCII_ONLY_NON_CC_REGEX = /^[\x20-\x7E]*$/; | |
| export const ASCII_ONLY_NON_CC_ERROR = "String cannot contain control characters or non-ASCII characters"; | |
| export const CommonArgs = { | |
| asciiOnlyString: (): ZodString => z.string().regex(ASCII_ONLY_NON_CC_REGEX, ASCII_ONLY_NON_CC_ERROR), |
| resolve(); | ||
| }, | ||
| { once: true } | ||
| ); |
There was a problem hiding this comment.
Probably fine, but it's good keeping in mind (esp. since this is a generic utility) that not removing the event listener if the signal does not abort can lead to memory leaks
There was a problem hiding this comment.
added cleanup to timeout scenario
| @@ -0,0 +1 @@ | |||
| {"version":3,"file":"vitest.config.js","sourceRoot":"","sources":["vitest.config.ts"],"names":[],"mappings":"AAAA,OAAO,EAAE,YAAY,EAAE,MAAM,eAAe,CAAC;AAE7C,eAAe,YAAY,CAAC;IACxB,IAAI,EAAE;QACF,UAAU,EAAE,CAAC,4BAA4B,CAAC;KAC7C;CACJ,CAAC,CAAC"} | |||
There was a problem hiding this comment.
Should these files that are generated from vitest.config.ts be commited?
| } | ||
| } | ||
|
|
||
| enableFipsIfRequested(); |
There was a problem hiding this comment.
We may want to be careful with ESM and the relative timing of execution here.
This seems to be inspired by similar code in mongosh; mongosh compiles to CJS, and does so in a slightly non-spec-compliant way, where imports get translated to require() calls in place. In "real" ESM (and the presence of import.meta handling in this PR makes me assume that we're targeting ESM now), imported modules evaluate before any code in the calling script runs.
This is a good reminder to also fix this in mongosh: mongodb-js/mongosh#2731
There was a problem hiding this comment.
hm yeah. We were already doing this with ESM as our default and I suppose haven't had FIPS users to report anything. I'd like to defer it to a seperate PR if that's fine as this matches current state
| { | ||
| "name": "mongodb-mcp-server", | ||
| "description": "MongoDB Model Context Protocol Server", | ||
| "version": "1.11.0-prerelease.3", |
There was a problem hiding this comment.
Probably an unnecessary comment, but let's make sure this version is what it should be when we merge this PR?
There was a problem hiding this comment.
The way our release workflow is setup, we'll trigger a release when we see an unreleased version. So we should bump it only if we want to also do a release.
| delete (process.env as Record<string, unknown>)[variable]; | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
useClearEnvironment() restores previous values of environment variables, should this one do the same?
There was a problem hiding this comment.
yeah.. generally there's a lot of redundancy here (more obvious now that they're been batched together); I'd love to adopt a hook-based approach across the board eventually. I'd add this for now
| import { z } from "zod"; | ||
| import { EJSON } from "bson"; | ||
|
|
||
| export function toEJSON<T extends object | undefined>(value: T): T { |
There was a problem hiding this comment.
This does the exact opposite of what it says it does 🙂
| export function toEJSON<T extends object | undefined>(value: T): T { | |
| export function fromEJSON<T extends object | undefined>(value: T): T { |
There was a problem hiding this comment.
I will call this deserializeEJSON to make the Zod chain below easier to read
There was a problem hiding this comment.
Should this file be checked in to the repo?
There was a problem hiding this comment.
It should be committed (we don't always build this, only on releases as part of generation script) but we could hide it in attributions. This can be done in a different PR though as this matches current state.
It's something worth improving for sure
There was a problem hiding this comment.
Hmm I ran into this question for my mcp apps skunkworks too. I copied what the mcp ui package does for now, but I don't like it.
|
Converting this to draft as we want to do a v1 release first before moving to v2. |
Only merge once the last v1 release is complete
It's many PRs merged into one along with many patches on top; I have reviewed every file but it's obviously a lot to expect that, so if you're curious about the major changes, here's the important files:
api-extractor/reports/tools.public.api.md, api-extractor/reports/mongodb-mcp-server.public.api.md- I kept these around purely to have an easier visualization of changes. It provides a good overview of what was removed & updates.packages/cli- This is the closest to the original index.ts / consumer layer with UserConfig we're used to. Everything around has been decoupled from UserConfigpackages/mongodb-mcp-server- This is an extremely minimal consumer of packages/cli . It also currently has a bunch of re-exporting for the sake of the v2 report comparisons above but this will be removed afterwards.