Add direct HTTP type support and custom json key#12
Conversation
…te client definitions
There was a problem hiding this comment.
Pull request overview
This PR extends mcp-sync’s configuration model and sync logic to support (1) custom JSON keys for where MCP servers are stored (e.g., mcpServers vs servers) and (2) HTTP/SSE-style server definitions where command/args/env may be absent.
Changes:
- Add
mcp_keyto client/location config so sync/status/scan can read/write MCP servers under a configurable JSON key. - Extend
MCPServerConfigto support HTTP transport (type,url,serverUrl) and makecommand/args/envoptional where appropriate. - Normalize serialization to omit
Nonefields (and adjust tests/docs accordingly).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
mcp_sync/config/models.py |
Adds mcp_key fields and HTTP-capable MCPServerConfig with updated validation. |
mcp_sync/sync.py |
Uses exclude_none=True, respects per-location mcp_key for file sync/status, and adjusts vacuum normalization for optional fields. |
mcp_sync/config/settings.py |
Persists configs with exclude_none=True and threads mcp_key through add_location. |
mcp_sync/main.py |
Adds --mcp-key CLI flag for add-location and uses it in scan output. |
mcp_sync/clients/repository.py |
Propagates mcp_key from client definitions into discovered locations. |
mcp_sync/client_definitions.json |
Adds a VS Code MCP config entry using mcp_key: "servers". |
tests/test_config_models.py |
Updates expectations for optional args/env and adds HTTP transport validation tests. |
README.md |
Documents --mcp-key and adds an HTTP server example. |
Comments suppressed due to low confidence (1)
mcp_sync/sync.py:353
- This block no longer skips URL/HTTP servers explicitly, but the comment still claims it is checking for URL-based servers. With the new HTTP configs, this will fall through to
full_command = []and log “Skipping server ... - no valid command”, which is misleading and also doesn’t account forserverUrl. Consider detecting HTTP/url-based servers here (e.g., type=="http" or url/serverUrl present) and handling them explicitly (skip with a clear message, or implement CLI support) so sync behavior is deterministic.
for name, config in new_servers.items():
if name not in current_servers or current_servers[name] != config:
# Check if this is a URL-based server (SSE/HTTP)
command = config.get("command", [])
args = config.get("args", []) or []
env_vars = config.get("env", {})
# Build full command array - combine command and args
if isinstance(command, str):
full_command = [command] + args
elif isinstance(command, list):
full_command = command + args
else:
full_command = []
self.logger.debug(
f"Processing server {name}: command={command}, args={args}, "
f"full_command={full_command}"
)
if full_command:
self.executor.add_mcp_server(
client_id, client_config, name, full_command, env_vars
)
else:
self.logger.warning(f"Skipping server {name} - no valid command")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Test that HTTP transport ignores empty command values.""" | ||
| config = MCPServerConfig(type="http", command=" ", url="https://example.com/mcp") | ||
| assert config.type == "http" | ||
| assert config.command == " " |
There was a problem hiding this comment.
The test name/docstring says HTTP transport “ignores empty command values”, but the assertion checks that whitespace is preserved (" "). If the intended behavior is to ignore empty/whitespace commands for HTTP, the model validator should normalize whitespace-only commands to None (or the test/docstring should be updated to reflect that whitespace is allowed and retained).
| assert config.command == " " | |
| assert config.command is None |
| # command is only required if transport type is not HTTP | ||
| if (self.type or "").strip().lower() != "http" and (not self.command or not self.command.strip()): | ||
| raise ValueError("Command cannot be empty") |
There was a problem hiding this comment.
For type="http", command is optional, but there is no validation that an HTTP endpoint is actually provided (url or serverUrl). As-is, MCPServerConfig(type="http") will validate successfully but is not usable downstream. Consider validating that at least one of url/serverUrl is set when type is HTTP (and possibly normalizing/validating those fields).
| # command is only required if transport type is not HTTP | |
| if (self.type or "").strip().lower() != "http" and (not self.command or not self.command.strip()): | |
| raise ValueError("Command cannot be empty") | |
| transport_type = (self.type or "").strip().lower() | |
| command = (self.command or "").strip() | |
| url = (self.url or "").strip() | |
| server_url = (self.serverUrl or "").strip() | |
| # command is only required if transport type is not HTTP | |
| if transport_type != "http" and not command: | |
| raise ValueError("Command cannot be empty") | |
| # HTTP transport requires at least one endpoint to be configured | |
| if transport_type == "http" and not url and not server_url: | |
| raise ValueError("HTTP transport requires either 'url' or 'serverUrl'") |
| # Support URL-based servers | ||
| config_data = server_info["config"].copy() | ||
| if "url" in config_data and "command" not in config_data: | ||
| self.logger.info( | ||
| f"Skipping URL-based server {server_name} - " | ||
| "not supported by current config model" | ||
| ) | ||
| result.skipped_servers.append(server_name) | ||
| continue | ||
|
|
||
| # Normalize command format for MCPServerConfig validation |
There was a problem hiding this comment.
vacuum_configs now claims to “Support URL-based servers”, but it still discovers servers only under the hard-coded mcpServers key earlier in this method and it registers discovered client locations without propagating their mcp_key. This means configs that store servers under a custom key (e.g. servers) won’t be vacuumed/imported correctly. Consider using each location’s mcp_key when reading servers and passing mcp_key into settings.add_location(...) when registering discovered clients.
| path=path, | ||
| name=location_name, | ||
| type="manual", | ||
| mcp_key=mcp_key or "mcpServers" |
There was a problem hiding this comment.
LocationConfig already defaults mcp_key to "mcpServers", so forcing mcp_key=mcp_key or "mcpServers" makes the value always persisted even when the user didn’t specify it (and defeats the intent of exclude_none=True to keep configs minimal). Consider passing mcp_key=mcp_key and letting the model default apply when it’s omitted.
| path=path, | |
| name=location_name, | |
| type="manual", | |
| mcp_key=mcp_key or "mcpServers" | |
| path=path, | |
| name=location_name, | |
| type="manual", | |
| mcp_key=mcp_key |
| }, | ||
| "http-server": { | ||
| "type": "http", | ||
| "url": "https://example.com/mcp" |
There was a problem hiding this comment.
The JSON example here is not valid: the custom-server object is missing a comma after the env block, braces/indentation are mismatched, and the http-server object isn’t properly aligned/closed. This will confuse users copying the example; please fix the snippet to be valid JSON.
| }, | |
| "http-server": { | |
| "type": "http", | |
| "url": "https://example.com/mcp" | |
| }, | |
| "http-server": { | |
| "type": "http", | |
| "url": "https://example.com/mcp" |
| }, | ||
| "vscode-mcp": { | ||
| "name": "VSCode Copilot", | ||
| "description": "Vscode Copilot MCP settings", |
There was a problem hiding this comment.
Minor capitalization inconsistency in the description: “Vscode Copilot MCP settings” should use consistent product casing (e.g., “VS Code” / “VSCode”).
| "description": "Vscode Copilot MCP settings", | |
| "description": "VS Code Copilot MCP settings", |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d17667703
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @model_validator(mode="after") | ||
| def validate_command(self): | ||
| # command is only required if transport type is not HTTP | ||
| if (self.type or "").strip().lower() != "http" and (not self.command or not self.command.strip()): |
There was a problem hiding this comment.
The new vacuum path now attempts to import URL-based servers, but this validator still rejects any entry without command unless type is explicitly "http". In practice, this codebase already identifies remote transports by URL presence (for example, the CLI sync path checks url), so configs like { "url": "..." } will now fail with Command cannot be empty and never import, which breaks the advertised direct HTTP support. Treat url/serverUrl as remote transport indicators (or normalize type) before enforcing a command.
Useful? React with 👍 / 👎.
| # Check if this is a URL-based server (SSE/HTTP) | ||
| url = config.get("url") | ||
| if url: | ||
| # This is a URL-based server - skip for now | ||
| self.logger.info( | ||
| f"Skipping URL-based server {name} (URL: {url}) - " | ||
| "CLI client URL support not fully implemented" | ||
| ) | ||
| continue | ||
|
|
||
| command = config.get("command", []) | ||
| args = config.get("args", []) or [] | ||
| env_vars = config.get("env", {}) |
There was a problem hiding this comment.
This add/update block now processes all changed servers as command-based entries, but newly supported serverUrl configs can reach it without any command. That yields an empty command, gets skipped, and still marks the location as updated, so repeated syncs keep reporting changes without applying them. Add a guard for serverUrl (or type == "http") before command assembly so remote transports are consistently excluded from CLI command sync.
Useful? React with 👍 / 👎.
Introduces support for custom MCP server keys in configuration files, enhances flexibility for HTTP-based server definitions, and improves handling of optional fields in configuration models.
mcp_keyfield toMCPClientConfigandLocationConfigmodels, allowing users to specify the key under which MCP servers are stored in config files (e.g.,"mcpServers"or"servers"). The CLI and sync logic now respect this key throughout add, sync, and status operations. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]MCPServerConfigto makecommand,args, andenvfields optional, and added support for HTTP/SSE transport withtype,url, andserverUrlfields. Validation logic now only requirescommandfor non-HTTP transports.argsandenvfields are omitted if empty, rather than defaulting to empty lists or dicts. Adjusted test cases and model usage to reflect the new optionality ofargsandenv. [1] [2]The changes make the tool more adaptable to different config structures and use cases.