-
Notifications
You must be signed in to change notification settings - Fork 210
feat: add CLI arguments support for better MCP client compatibility #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import base64 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import requests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -30,12 +31,27 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from minimax_mcp.exceptions import MinimaxAPIError, MinimaxRequestError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from minimax_mcp.client import MinimaxAPIClient | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Parse CLI arguments | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cli_args = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args = sys.argv[1:] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i in range(len(args)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if args[i].startswith('--'): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| key_value = args[i][2:].split('=', 1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(key_value) == 2: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cli_args[key_value[0]] = key_value[1] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif i + 1 < len(args) and not args[i + 1].startswith('--'): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cli_args[key_value[0]] = args[i + 1] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+37
to
+43
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i in range(len(args)): | |
| if args[i].startswith('--'): | |
| key_value = args[i][2:].split('=', 1) | |
| if len(key_value) == 2: | |
| cli_args[key_value[0]] = key_value[1] | |
| elif i + 1 < len(args) and not args[i + 1].startswith('--'): | |
| cli_args[key_value[0]] = args[i + 1] | |
| i = 0 | |
| while i < len(args): | |
| arg = args[i] | |
| if arg.startswith('--'): | |
| key_value = arg[2:].split('=', 1) | |
| key = key_value[0] | |
| if len(key_value) == 2: | |
| # Handle --key=value form | |
| cli_args[key] = key_value[1] | |
| elif i + 1 < len(args) and not args[i + 1].startswith('-'): | |
| # Handle --key value form, ensuring the next token is not another flag | |
| cli_args[key] = args[i + 1] | |
| i += 1 # Skip the value we just consumed | |
| i += 1 |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI argument parsing code (lines 35-43) executes at module import time, which makes it difficult to test and could cause issues if this module is imported elsewhere. Consider wrapping this logic in a function (e.g., 'parse_cli_args') that can be called explicitly and tested independently.
| cli_args = {} | |
| args = sys.argv[1:] | |
| for i in range(len(args)): | |
| if args[i].startswith('--'): | |
| key_value = args[i][2:].split('=', 1) | |
| if len(key_value) == 2: | |
| cli_args[key_value[0]] = key_value[1] | |
| elif i + 1 < len(args) and not args[i + 1].startswith('--'): | |
| cli_args[key_value[0]] = args[i + 1] | |
| def parse_cli_args(argv=None): | |
| """Parse command-line arguments into a dictionary. | |
| Supports flags in the form --key=value or --key value. | |
| """ | |
| if argv is None: | |
| argv = sys.argv[1:] | |
| parsed = {} | |
| for i in range(len(argv)): | |
| if argv[i].startswith('--'): | |
| key_value = argv[i][2:].split('=', 1) | |
| if len(key_value) == 2: | |
| parsed[key_value[0]] = key_value[1] | |
| elif i + 1 < len(argv) and not argv[i + 1].startswith('--'): | |
| parsed[key_value[0]] = argv[i + 1] | |
| return parsed | |
| cli_args = parse_cli_args() |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added CLI argument parsing logic (lines 34-43) and the get_config function (lines 45-47) lack test coverage. Since the repository has existing test infrastructure in the tests directory, these new functions should have corresponding unit tests to verify correct behavior, especially for edge cases like missing values, empty strings, and various argument formats.
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_config function uses the 'or' operator for fallback logic, which means empty strings will be treated as falsy and fall through to the next option. If a user explicitly sets a CLI argument to an empty string (e.g., '--api-key='), it will fall back to the environment variable or default value instead of using the empty string. This behavior should be intentional, but if it's not, consider using 'is None' checks instead.
| return cli_args.get(cli_key) or os.getenv(env_key) or default | |
| cli_value = cli_args.get(cli_key) | |
| if cli_value is not None: | |
| return cli_value | |
| env_value = os.getenv(env_key) | |
| if env_value is not None: | |
| return env_value | |
| return default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path '/User/xxx/Desktop' is incorrect for macOS/Unix systems. The correct path should be '/Users/xxx/Desktop' (with an 's'). This appears in both the CLI argument example and should be corrected to avoid confusing users.