fix: add argument validation - MCP-188 #542
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive argument validation for Atlas tools using Zod schemas to improve type safety and input validation. The changes replace direct Zod imports with a centralized args system that provides validation for common MongoDB Atlas resource types like ObjectIds, cluster names, usernames, and IP addresses.
- Centralizes argument validation into reusable schema definitions in
src/tools/args.ts - Updates all Atlas tools to use the new validation system instead of direct Zod imports
- Adds comprehensive unit tests covering various validation scenarios and edge cases
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/args.test.ts | Comprehensive test suite for argument validation schemas |
| src/tools/args.ts | Central argument validation definitions using Zod schemas |
| src/tools/atlas/read/*.ts | Updated to use centralized argument validation |
| src/tools/atlas/create/*.ts | Updated to use centralized argument validation |
| src/tools/atlas/connect/connectCluster.ts | Updated to use centralized argument validation |
| src/tools/atlas/atlasTool.ts | Import reorganization for tool base class |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
nirinchev
left a comment
nirinchev
left a comment
There was a problem hiding this comment.
Looks good, minor suggestions/questions from me. I haven't verified that the validations we've added match the ones in Atlas, so might be a good idea to double check we're not being overly restrictive with any of them.
| import { AtlasArgs } from "../../args.js"; | ||
|
|
||
| export const InspectClusterArgs = { | ||
| projectId: AtlasArgs.projectId().describe("Atlas project ID"), |
There was a problem hiding this comment.
The descriptions of projectId and clusterName seem to be the same everywhere - should we just include them in the AtlasArgs helpers?
There was a problem hiding this comment.
Unfortunately they differ with some extra tiny context per tool, I added this as a future improvement to keep the scope of the task
|
|
||
| const ALLOWED_PROJECT_NAME_CHARACTERS_REGEX = /^[a-zA-Z0-9\s()@&+:._',-]+$/; | ||
| export const ALLOWED_PROJECT_NAME_CHARACTERS_ERROR = | ||
| "Project names can't be longer than 64 characters and can only contain letters, numbers, spaces, and the following symbols: ( ) @ & + : . _ - ' ,"; |
There was a problem hiding this comment.
went into atlas UI and copied the exact same validation text
nirinchev
left a comment
nirinchev
left a comment
There was a problem hiding this comment.
Looks good - mostly minor nits from me.
Pull Request Test Coverage Report for Build 17649734141Details
💛 - Coveralls |
Proposed changes
MCP-188
Future improvements
validateThrowsForInvalidArguments, a PR is in progress but separated from this oneTest
Checklist