chore: use zod for arg-parser MCP-298 #2589
Conversation
gagik
changed the title
This is preparation for its usage of MCP and further changes in #2589
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the CLI argument parsing implementation to use Zod for schema validation and type safety. The changes introduce a more maintainable and type-safe approach to handling command-line arguments while preserving backward compatibility with deprecated arguments.
Key Changes:
- Migrated from
yargs-parserwith manual validation to a Zod-based schema approach - Added proper type definitions and exports for the arg-parser package
- Fixed previously broken boolean argument parsing (e.g.,
--browser=true)
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/arg-parser/src/arg-parser.ts | New implementation of argument parsing using Zod schemas with yargs-parser integration |
| packages/arg-parser/src/cli-options.ts | Refactored CLI options from interface to Zod schema with metadata for deprecation handling |
| packages/arg-parser/src/arg-metadata.ts | New metadata system for argument aliases, deprecation, and validation rules |
| packages/cli-repl/src/parse-mongosh-args.ts | New wrapper function that uses the refactored arg-parser with mongosh-specific error handling |
| packages/cli-repl/src/arg-parser.ts | Removed legacy argument parser implementation |
| packages/snippet-manager/package.json | Updated zod dependency version |
| packages/build/package.json | Updated @mongodb-js/dl-center dependency |
| packages/browser-repl/src/components/editor.tsx | Added type annotations to callback parameters |
| configs/tsconfig-mongosh/tsconfig.common.json | Updated module resolution to nodenext |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gagik
added
the
no-title-validation
| const processed = processPositionalCliOptions({ | ||
| parsed, | ||
| positional, | ||
| }); |
There was a problem hiding this comment.
This bit is problematic for MCP CLI because nodb used in processPositionalCliOptions is of no significance for MCP which is also why we ignore it in the current parsing. This bit was refactored and undone in our refactor PR - https://github.com/mongodb-js/mongodb-mcp-server/pull/729/files#diff-9d5ba1a74fa3c1ca2e89c9b7a2c356807802c8b5be7473377ffd4d9ab348aef1L298-L303
There was a problem hiding this comment.
we can mark nodb as unsupported
| }: { | ||
| schema: z.ZodObject; | ||
| configuration?: YargsOptions['configuration']; | ||
| }): YargsOptions { |
There was a problem hiding this comment.
It sucks that we can't easily do unwrapType() typesafely (right?), but the fact that we have to rely on casting to unknown and on instanceof here means we have very little guarantee that an incorrect input type would be properly handled.
Can we make an assertion here that schema instanceof z.ZodObject? Otherwise this would just silently fail if we get an object created from a different zod library, which I don't think we'd want.
There was a problem hiding this comment.
z.ZodType is the best we got as far as asserting it I think, but still better than unknown yeah.
Haven't found a good candidate beyond this unwrapping though.
A couple things here (though these specific commits are quite outdated now)
--browser=truehave not been parsed correctly until now (concerning?) and the only real options have been the string-based ones. So this adds a) tests relating to this (which fail without the coercion change) and b) conditional coercing to boolean value when it istrueorfalse. This seems to be intended behavior but let me know if I'm overlooking it.