Fix/issue 844 cursor mcp json schema#869
Conversation
When load_policy() receives a YAML string that exceeds ~1023 bytes, the is_file() syscall fails on macOS (PATH_MAX ≈ 1024) with OSError [Errno 63] File name too long. Wrap the call in try/except to gracefully fall back to treating the input as a YAML string. Fixes microsoft#848
- Override _format_server_config() to emit correct 'type' field: - stdio transports -> 'type: stdio' (not 'type: local') - http/sse transports -> 'type: http' - Remove Copilot-specific 'tools' and 'id' fields that cause Cursor's MCP loader to silently reject servers Fixes microsoft#844
chaobo8484
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
sergio-sisternes-epam
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
This PR fixes Cursor MCP configuration generation so that apm writes a .cursor/mcp.json that matches Cursor's expected schema (instead of inheriting Copilot CLI formatting).
Changes:
- Add Cursor-specific
_format_server_config()that emits Cursor-compatibletypevalues and omits Copilot-only fields. - Add unit tests covering Cursor MCP config formatting for stdio, http remotes, and package-based servers.
- Harden
load_policy()to avoid crashing whenPath.is_file()raises on non-path inputs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/apm_cli/adapters/client/cursor.py |
Overrides server-config formatting to match Cursor MCP JSON expectations. |
tests/unit/test_cursor_mcp.py |
Adds coverage to ensure Cursor config output uses correct type and excludes Copilot-only fields. |
src/apm_cli/policy/parser.py |
Wraps Path.is_file() to tolerate OS-level path errors when source is not a real file path. |
| def _format_server_config(self, server_info, env_overrides=None, runtime_vars=None): | ||
| """Format server info into Cursor-compatible ``.cursor/mcp.json`` format. | ||
|
|
||
| Cursor uses ``"type": "stdio"`` or ``"type": "http"`` (NOT ``"local"``) | ||
| and does NOT support the ``tools`` or ``id`` fields that Copilot CLI uses. | ||
|
|
||
| Args: | ||
| server_info (dict): Server information from registry. | ||
| env_overrides (dict, optional): Pre-collected environment variable overrides. | ||
| runtime_vars (dict, optional): Pre-collected runtime variable values. | ||
|
|
||
| Returns: | ||
| dict: Cursor-compatible server configuration. | ||
| """ |
There was a problem hiding this comment.
New/modified method is missing type hints for parameters and return value, which makes the adapter contract harder to follow and conflicts with the repo guideline requiring type hints. Please add appropriate annotations (e.g., dict[str, Any] / Optional[dict[str, str]] / dict[str, Any]).
| remotes = server_info.get("remotes", []) | ||
| if remotes: | ||
| remote = remotes[0] | ||
| transport = (remote.get("transport_type") or "http").strip() | ||
| if transport in ("sse", "streamable-http"): | ||
| transport = "http" | ||
| config = { | ||
| "type": "http", | ||
| "url": remote.get("url", ""), | ||
| } | ||
| headers = remote.get("headers", []) | ||
| if headers: | ||
| if isinstance(headers, list): | ||
| config["headers"] = { | ||
| h["name"]: h["value"] for h in headers if "name" in h and "value" in h | ||
| } | ||
| else: | ||
| config["headers"] = headers | ||
| return config |
There was a problem hiding this comment.
Remote (http) server formatting dropped the Copilot behavior of resolving header placeholders via _resolve_env_variable and adding GitHub auth headers when applicable. This is a functional regression vs the previously-inherited Copilot formatter and can leave Cursor configs with unusable headers (e.g., values containing '' placeholders) or missing Authorization for GitHub-hosted remotes. Please resolve/normalize headers the same way as Copilot (including _warn_input_variables for ${input:...} in headers) while still omitting Cursor-incompatible fields (tools/id).
| if path.is_file(): | ||
| try: | ||
| is_file = path.is_file() | ||
| except OSError: |
There was a problem hiding this comment.
load_policy() now guards Path.is_file() with an OSError handler, but Path/is_file can also raise ValueError for invalid path strings (e.g., embedded NUL bytes). Elsewhere in the repo path operations commonly catch (OSError, ValueError). Consider catching ValueError here as well so YAML-string inputs can't crash with an unhandled exception.
| except OSError: | |
| except (OSError, ValueError): |
| - ``type`` must be ``"stdio"`` or ``"http"`` (NOT ``"local"``). | ||
| - ``tools`` and ``id`` fields must **never** be emitted — they are | ||
| Copilot-CLI-specific and cause Cursor's MCP loader to silently | ||
| reject the server. |
There was a problem hiding this comment.
This docstring introduces a Unicode em dash character (�> ASCII). Repo policy requires source + CLI strings to be printable ASCII only; please replace the em dash with ASCII (e.g., "-" or "--").
| # ------------------------------------------------------------------ # | ||
| # _format_server_config — MUST override; do NOT silently inherit Copilot | ||
| # ------------------------------------------------------------------ # |
There was a problem hiding this comment.
This comment header uses a Unicode em dash character (�> ASCII). The repo's encoding rule requires source files to be printable ASCII only; please replace it with ASCII punctuation (e.g., "--").
APM Expert Review Panel
[Python Architect] — Architecture / Code StructureFixes are structurally sound. Three non-blocking concerns worth tracking:
[CLI Logging Expert] — Output / Logging PathsNo logging regressions introduced. The pre-existing [DevX UX Expert] — Command Surface / Error ErgonomicsHigh-impact UX fix. Cursor was silently rejecting every server APM configured because the This fix unblocks all Cursor users from a silent failure. No new UX regressions observed. The policy parser fix ( [Supply Chain Security Expert] — Headers, Auth, Token ScopeOne functional gap worth a follow-up issue: In the Copilot adapter's remote path, each registry-defined header value goes through # cursor.py (new code)
config["headers"] = {
h["name"]: h["value"] for h in headers if "name" in h and "value" in h
}
# vs copilot.py:
resolved_value = self._resolve_env_variable(header_name, header_value, env_overrides)
config["headers"][header_name] = resolved_valueFor MCP servers that require auth headers with env-var placeholders, Cursor users will get unexpanded No new attack surfaces, no path traversal risk, no credential leakage. [OSS Growth Hacker] — Adoption ImpactCursor has significant market share among AI developers — this fix directly improves conversion for a large segment of APM's potential user base. Silent MCP rejection was the worst possible experience for a first-timer on Cursor. Recommend calling this out explicitly in the [APM CEO] — Strategic Rationale / Final VerdictBoth fixes address real reported issues (#844, #848) with clear reproduction paths and test coverage. The contributor shipped tests for the new behavior which is exactly what we want from external contributors — accept the pattern encouragingly. Missing CHANGELOG entries — required per our conventions. Please add before merge: ### Fixed
- Cursor MCP adapter now emits `type: "stdio"` / `type: "http"` instead of `type: "local"`, and omits Copilot-specific `tools` and `id` fields that caused Cursor to silently reject configured servers. Closes #844 (#869)
- `load_policy()` no longer raises `OSError [Errno 63] File name too long` on macOS when given a YAML string longer than PATH_MAX. Closes #848 (#869)Action items (non-blocking — maintainer discretion):
Merge when CHANGELOG is added. Everything else can be tracked separately.
|
sergio-sisternes-epam
left a comment
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Review -- PR #869 Fix/issue 844 cursor MCP JSON schema
Thanks for tackling #844 -- the diagnosis is correct, the type mapping is exactly what Cursor needs, and the tests cover the key happy paths. Good work identifying the root cause.
However, the implementation strategy (full method fork) introduces functional regressions that recreate the same class of silent-failure bug that #844 reported. Three independent reviewers (architecture, security, DX) converge on the same recommendation.
Blocking findings
1. super() + transform instead of ~100-line fork
The override duplicates the parent but silently drops: GitHub MCP auto-auth, header env-var resolution, _warn_input_variables, and _apm_tools_override. OpenCodeClientAdapter already proves the delegate-then-transform pattern works in this codebase (~8 lines vs ~100).
2. GitHub MCP server auth silently dropped (Security: HIGH)
The parent detects GitHub servers via _is_github_server() and injects a Bearer token. This override skips it. apm install github-mcp-server --client cursor will produce an unauthenticated config -- the server returns 401 and Cursor shows nothing useful. Same class of silent-failure as #844.
3. Header env-var resolution missing (Security: MEDIUM)
The parent resolves ${input:...} and env-var references in header values and warns about unresolvable ones. This override copies raw values as literal strings.
4. Docstring endorses copy-paste overrides
The new docstring says _format_server_config "must be explicitly overridden in each subclass". This codifies tech debt as policy. The correct lesson (proven by OpenCode) is to call super() and transform.
Non-blocking
- Unrelated
policy/parser.pychange -- reasonable, but should be a separate PR. Also consider catching(OSError, ValueError). - Module docstring should note Cursor's schema differs from Copilot's.
- Missing test for GitHub auth -- once the auth gap is fixed, add a test mocking
GitHubTokenManager+_is_github_server. - Edge-case test gaps -- empty env, docker path, ValueError on missing packages, SSE/streamable-http transport normalisation.
- Missing CHANGELOG entry under
[Unreleased]. - PR body is still template placeholder -- please fill in linking to #844.
- Merge conflicts need resolving (rebase onto
main).
Happy to re-review once updated. The core fix is right -- it just needs the cleaner implementation strategy to avoid trading one silent-failure bug for another.
| schema differs from Copilot CLI's in two critical ways: | ||
|
|
||
| - ``type`` must be ``"stdio"`` or ``"http"`` (NOT ``"local"``). | ||
| - ``tools`` and ``id`` fields must **never** be emitted — they are | ||
| Copilot-CLI-specific and cause Cursor's MCP loader to silently | ||
| reject the server. | ||
|
|
||
| .. note:: | ||
|
|
||
| This inheritance design is a known fragility. ``_format_server_config`` | ||
| **must** be explicitly overridden in each subclass; silently inheriting | ||
| the Copilot version will produce invalid configs for the target runtime. |
There was a problem hiding this comment.
Docstring codifies the wrong architectural lesson.
"This inheritance design is a known fragility.
_format_server_configmust be explicitly overridden in each subclass"
This endorses the copy-paste approach as policy. The correct lesson (proven by OpenCodeClientAdapter) is that subclasses should call super() and transform the result. Please update this to document the delegate-then-transform pattern instead.
| def _format_server_config(self, server_info, env_overrides=None, runtime_vars=None): | ||
| """Format server info into Cursor-compatible ``.cursor/mcp.json`` format. | ||
|
|
||
| Cursor uses ``"type": "stdio"`` or ``"type": "http"`` (NOT ``"local"``) | ||
| and does NOT support the ``tools`` or ``id`` fields that Copilot CLI uses. | ||
|
|
||
| Args: | ||
| server_info (dict): Server information from registry. | ||
| env_overrides (dict, optional): Pre-collected environment variable overrides. | ||
| runtime_vars (dict, optional): Pre-collected runtime variable values. | ||
|
|
||
| Returns: | ||
| dict: Cursor-compatible server configuration. | ||
| """ | ||
| if runtime_vars is None: | ||
| runtime_vars = {} | ||
|
|
||
| raw = server_info.get("_raw_stdio") | ||
| if raw: | ||
| config = { | ||
| "type": "stdio", | ||
| "command": raw["command"], | ||
| "args": raw["args"], | ||
| } | ||
| if raw.get("env"): | ||
| config["env"] = raw["env"] | ||
| self._warn_input_variables(raw["env"], server_info.get("name", ""), "Cursor") | ||
| return config | ||
|
|
||
| remotes = server_info.get("remotes", []) | ||
| if remotes: | ||
| remote = remotes[0] | ||
| transport = (remote.get("transport_type") or "http").strip() | ||
| if transport in ("sse", "streamable-http"): | ||
| transport = "http" | ||
| config = { | ||
| "type": "http", | ||
| "url": remote.get("url", ""), | ||
| } | ||
| headers = remote.get("headers", []) | ||
| if headers: | ||
| if isinstance(headers, list): | ||
| config["headers"] = { | ||
| h["name"]: h["value"] for h in headers if "name" in h and "value" in h | ||
| } | ||
| else: | ||
| config["headers"] = headers | ||
| return config | ||
|
|
||
| packages = server_info.get("packages", []) | ||
| if not packages: | ||
| raise ValueError( | ||
| f"MCP server has incomplete configuration in registry - no package " | ||
| f"information or remote endpoints available. " | ||
| f"Server: {server_info.get('name', 'unknown')}" | ||
| ) | ||
|
|
||
| package = self._select_best_package(packages) | ||
| if not package: | ||
| raise ValueError( | ||
| f"No suitable package found for MCP server " | ||
| f"'{server_info.get('name', 'unknown')}'" | ||
| ) | ||
|
|
||
| registry_name = self._infer_registry_name(package) | ||
| package_name = package.get("name", "") | ||
| runtime_hint = package.get("runtime_hint", "") | ||
| runtime_arguments = package.get("runtime_arguments", []) | ||
| package_arguments = package.get("package_arguments", []) | ||
| env_vars = package.get("environment_variables", []) | ||
|
|
||
| resolved_env = self._resolve_environment_variables(env_vars, env_overrides) | ||
| processed_runtime_args = self._process_arguments(runtime_arguments, resolved_env, runtime_vars) | ||
| processed_package_args = self._process_arguments(package_arguments, resolved_env, runtime_vars) | ||
|
|
||
| config = {"type": "stdio"} | ||
|
|
||
| if registry_name == "npm": | ||
| config["command"] = runtime_hint or "npx" | ||
| config["args"] = ["-y", package_name] + processed_runtime_args + processed_package_args | ||
| elif registry_name == "docker": | ||
| config["command"] = "docker" | ||
| if processed_runtime_args: | ||
| config["args"] = self._inject_env_vars_into_docker_args( | ||
| processed_runtime_args, resolved_env | ||
| ) | ||
| else: | ||
| from ...core.docker_args import DockerArgsProcessor | ||
| config["args"] = DockerArgsProcessor.process_docker_args( | ||
| ["run", "-i", "--rm", package_name], | ||
| resolved_env | ||
| ) | ||
| elif registry_name == "pypi": | ||
| config["command"] = runtime_hint or "uvx" | ||
| config["args"] = [package_name] + processed_runtime_args + processed_package_args | ||
| elif registry_name == "homebrew": | ||
| config["command"] = package_name.split("/")[-1] if "/" in package_name else package_name | ||
| config["args"] = processed_runtime_args + processed_package_args | ||
| else: | ||
| config["command"] = runtime_hint or package_name | ||
| config["args"] = processed_runtime_args + processed_package_args | ||
|
|
||
| if resolved_env: | ||
| config["env"] = resolved_env | ||
|
|
||
| return config |
There was a problem hiding this comment.
Replace this ~100-line override with super() + transform (~8 lines).
All three reviewers (architecture, security, DX) converge on this. The override duplicates the parent but silently drops: GitHub MCP auto-auth, header env-var resolution, _warn_input_variables, and _apm_tools_override. OpenCodeClientAdapter already proves the delegate-then-transform pattern works in this codebase:
def _format_server_config(self, server_info, env_overrides=None, runtime_vars=None):
config = super()._format_server_config(server_info, env_overrides, runtime_vars)
config.pop("tools", None)
config.pop("id", None)
if config.get("type") == "local":
config["type"] = "stdio"
return configThis fixes every functional regression flagged below in one go and automatically inherits future parent improvements.
| remotes = server_info.get("remotes", []) | ||
| if remotes: | ||
| remote = remotes[0] | ||
| transport = (remote.get("transport_type") or "http").strip() | ||
| if transport in ("sse", "streamable-http"): | ||
| transport = "http" | ||
| config = { | ||
| "type": "http", | ||
| "url": remote.get("url", ""), | ||
| } | ||
| headers = remote.get("headers", []) | ||
| if headers: | ||
| if isinstance(headers, list): | ||
| config["headers"] = { | ||
| h["name"]: h["value"] for h in headers if "name" in h and "value" in h | ||
| } | ||
| else: | ||
| config["headers"] = headers | ||
| return config |
There was a problem hiding this comment.
Security: GitHub MCP server authentication silently dropped.
The parent (copilot.py L198-210) detects GitHub servers via _is_github_server() and injects Authorization: Bearer <token> using GitHubTokenManager. This override skips that entirely.
User impact: apm install github-mcp-server --client cursor produces a config with no auth header. The server returns 401, Cursor shows nothing useful. This is the same class of silent-failure bug that #844 reports -- just on a different code path.
This is a fail-open pattern that violates APM's fail-closed security posture. Inherited for free with super() + transform.
| headers = remote.get("headers", []) | ||
| if headers: | ||
| if isinstance(headers, list): | ||
| config["headers"] = { | ||
| h["name"]: h["value"] for h in headers if "name" in h and "value" in h | ||
| } | ||
| else: | ||
| config["headers"] = headers |
There was a problem hiding this comment.
Header env-var references written as literal strings.
The parent resolves ${input:...} and env-var references via _resolve_env_variable() (L222) and warns about unresolvable ones via _warn_input_variables() (L227). This override copies raw values, so headers containing placeholders like ${input:API_KEY} are written literally to .cursor/mcp.json.
Also inherited for free with super() + transform.
| class CursorClientAdapter(CopilotClientAdapter): | ||
| """Cursor IDE MCP client adapter. | ||
|
|
||
| Inherits all config formatting from :class:`CopilotClientAdapter` | ||
| (``mcpServers`` JSON with ``command``/``args``/``env``). Only the | ||
| config-file location differs: repo-local ``.cursor/mcp.json`` instead | ||
| of global ``~/.copilot/mcp-config.json``. | ||
| Inherits config path and read/write logic from this class, but | ||
| **must** override :meth:`_format_server_config` because Cursor's JSON | ||
| schema differs from Copilot CLI's in two critical ways: | ||
|
|
||
| - ``type`` must be ``"stdio"`` or ``"http"`` (NOT ``"local"``). | ||
| - ``tools`` and ``id`` fields must **never** be emitted — they are | ||
| Copilot-CLI-specific and cause Cursor's MCP loader to silently | ||
| reject the server. | ||
|
|
||
| .. note:: | ||
|
|
||
| This inheritance design is a known fragility. ``_format_server_config`` | ||
| **must** be explicitly overridden in each subclass; silently inheriting | ||
| the Copilot version will produce invalid configs for the target runtime. |
There was a problem hiding this comment.
Nit: the class docstring should note that the config schema differs from Copilot's.
After this PR, a developer reading the source will see the module-level docstring (L1-17) still implies the config schema is identical to Copilot's. Worth a one-line clarification that Cursor uses stdio/http instead of local and omits tools/id.
|
|
||
| if path.is_file(): | ||
| try: | ||
| is_file = path.is_file() | ||
| except OSError: |
There was a problem hiding this comment.
Scope: this change is unrelated to issue #844.
The OSError guard is reasonable defensive coding (and security-acceptable per review). However, mixing it into a Cursor MCP fix makes the PR harder to review and cherry-pick. Please split into its own PR or at minimum its own commit with a clear message.
Also consider catching (OSError, ValueError) -- ValueError is raised for paths containing NUL bytes.
| @patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference") | ||
| def test_http_server_outputs_type_http(self, mock_find): | ||
| """Remote servers must emit type=http, not type=local.""" | ||
| mock_find.return_value = { | ||
| "id": "remote-uuid", | ||
| "name": "remote-srv", | ||
| "packages": [], | ||
| "remotes": [ | ||
| { | ||
| "url": "https://example.com/mcp", | ||
| "transport_type": "http", | ||
| "headers": [{"name": "Authorization", "value": "Bearer token"}], | ||
| } | ||
| ], | ||
| } | ||
| ok = self.adapter.configure_mcp_server("remote-srv", "remote-srv") | ||
| self.assertTrue(ok) | ||
| data = json.loads(self.mcp_json.read_text(encoding="utf-8")) | ||
| self.assertEqual(data["mcpServers"]["remote-srv"]["type"], "http") | ||
| self.assertNotIn("tools", data["mcpServers"]["remote-srv"]) | ||
| self.assertNotIn("id", data["mcpServers"]["remote-srv"]) |
There was a problem hiding this comment.
Missing test: GitHub MCP server auth headers.
This test verifies type: "http" and absence of tools/id but doesn't assert that a GitHub MCP server gets an Authorization: Bearer ... header. Once the auth gap is fixed (ideally via super() + transform), add a test that mocks GitHubTokenManager.get_token_for_purpose and _is_github_server and asserts the header is present.
| @patch("apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference") | ||
| def test_stdio_with_packages_outputs_type_stdio(self, mock_find): | ||
| """NPM/docker packages must also emit type=stdio, not type=local.""" | ||
| mock_find.return_value = { | ||
| "id": "pkg-uuid", | ||
| "name": "npm-pkg", | ||
| "packages": [ | ||
| { | ||
| "registry_name": "npm", | ||
| "name": "some-npm-pkg", | ||
| "runtime_hint": "npx", | ||
| "arguments": [], | ||
| "environment_variables": [], | ||
| } | ||
| ], | ||
| } | ||
| ok = self.adapter.configure_mcp_server("npm-pkg", "npm-pkg") | ||
| self.assertTrue(ok) | ||
| data = json.loads(self.mcp_json.read_text(encoding="utf-8")) | ||
| self.assertEqual(data["mcpServers"]["npm-pkg"]["type"], "stdio") | ||
| self.assertNotIn("tools", data["mcpServers"]["npm-pkg"]) | ||
| self.assertNotIn("id", data["mcpServers"]["npm-pkg"]) |
There was a problem hiding this comment.
Suggestion: a few more edge-case tests would strengthen coverage.
Consider adding:
- Empty
env: {}to verify it's omitted from output - Docker package path (has its own branch in the implementation)
- Negative test where
packagesis empty and noremotesexist (should raiseValueError) sseorstreamable-httptransport type to verify the normalisation to"http"
Cursor adapter previously forked ~106 lines of Copilot's _format_server_config and silently dropped GitHub MCP auth, header env-var resolution, _warn_input_variables, and _apm_tools_override -- recreating the same class of silent-failure bug as microsoft#844. - Replace copy-paste fork with super() delegate-then-transform (~20 lines) - Restore GitHub Bearer token injection for github-mcp-server remotes - Restore header env-var resolution via _resolve_env_variable - Restore _warn_input_variables for \ references - Normalize sse/streamable-http transport types to 'http' for Cursor - Strip Copilot-only 'tools' and 'id' fields that cause Cursor to reject - Update docstrings to document delegate-then-transform pattern - Add Optional[dict] type hints to _format_server_config - Fix non-ASCII em dashes -> ASCII '--' - Catch (OSError, ValueError) in parser.py load_policy() for invalid paths - Add 7 new unit tests (GitHub auth, SSE normalization, edge cases) Fixes microsoft#844 Fixes microsoft#848
|
@sergio-sisternes-epam I have made adjustments based on your and Copilot's suggestions. Wish you a wonderful day! |
danielmeppiel
added
panel-review
CLI Logging / Output UX Review — PR #869SummaryThe
FINDINGSF1. Raw
|
| File | Raw print() count |
|---|---|
copilot.py |
5 |
cursor.py |
4 |
codex.py |
7 |
opencode.py |
4 |
vscode.py |
6 |
base.py |
1 |
The VSCode adapter is the only one that partially supports a logger parameter (configure_mcp_server(..., logger=None)), showing the intended migration direction. Cursor should follow that lead.
F2. _warn_input_variables emits an em-dash (\u2014) — ASCII violation 🔴
# base.py:123
f"'{server_name}' will not be resolved \u2014 "This is — (U+2014 EM DASH). On Windows cp1252 terminals and some CI log viewers this renders as â€" or ?. The entire purpose of STATUS_SYMBOLS being ASCII-safe is to prevent exactly this. When _format_server_config → super() → _warn_input_variables fires during Cursor config, this broken character hits the user's terminal.
This is the only blocking issue. It's in base.py, not cursor.py, but Cursor inherits and actively triggers this code path.
F3. Silent return on missing .cursor/ directory — acceptable, but verbose-worthy
# cursor.py:116-117
if not cursor_dir.exists():
return True # nothing to do, not an error
```
Returning `True` silently is correct for opt-in adapters (OpenCode does the same). But in `--verbose` mode, an agent or power user troubleshooting "why didn't Cursor get configured?" has zero signal. A verbose-only message would help:
```
[i] Cursor -- skipping (.cursor/ directory not found)F4. Bare except Exception swallows stack traces
# cursor.py:148-149
except Exception as e:
print(f"Error configuring MCP server: {e}")
return FalseIn non-verbose mode, hiding the traceback is fine. But in --verbose mode, the traceback is the single most useful debugging artifact. Every other part of APM's logging design follows progressive disclosure — default gives outcome, verbose gives diagnostics. This except block gives the same minimal output regardless of mode.
F5. Error messages lack STATUS_SYMBOLS prefix
Compare cursor.py output with what CommandLogger.error() would produce:
| Current (cursor.py) | Expected (APM convention) |
|---|---|
Error: server_url cannot be empty |
[x] Error: server_url cannot be empty |
Error: MCP server 'X' not found in registry |
[x] MCP server 'X' not found in registry |
Successfully configured MCP server 'X' for Cursor |
[*] Configured MCP server 'X' for Cursor |
Error configuring MCP server: ... |
[x] Error configuring MCP server: ... |
Also note: "Successfully configured" is wordier than APM's usual outcome-first style. Should be "Configured" — the [*] symbol already conveys success.
F6. No color differentiation
Raw print() outputs everything in the terminal's default color. APM's traffic-light convention:
- Red (
_rich_error) = errors the user must act on - Green (
_rich_success) = success confirmation - Yellow (
_rich_warning) = should-know warnings - Dim (
verbose_detail) = debug-level detail
Currently, all four cursor.py messages — errors, success, and any inherited warnings — render in the same undifferentiated color. This makes scanning output significantly harder, especially when installing across multiple clients simultaneously.
VIOLATIONS (specific lines)
| # | File:Line | Code | Violation |
|---|---|---|---|
| V1 | base.py:123 |
\u2014 (em-dash) |
ASCII compliance — will corrupt on Windows cp1252 terminals |
| V2 | cursor.py:111 |
print("Error: server_url cannot be empty") |
Raw print, no [x] symbol, no red color |
| V3 | cursor.py:127 |
print(f"Error: MCP server '{server_url}' not found...") |
Raw print, no [x] symbol, no red color |
| V4 | cursor.py:143-145 |
print(f"Successfully configured...") |
Raw print, no [*] symbol, no green color, verbose wording |
| V5 | cursor.py:149 |
print(f"Error configuring MCP server: {e}") |
Raw print, no [x] symbol, no traceback in verbose mode |
| V6 | cursor.py:116-117 |
Silent return True |
No verbose-mode signal for skipped adapter |
REQUIRED CHANGES
Blocking (must fix before merge):
-
Fix the em-dash in
base.py:123— replace\u2014with--(ASCII double-dash). This affects all adapters inheriting_warn_input_variables, and Cursor actively triggers it via the super() delegation pattern this PR introduces.# base.py:121-125 — before print( f"[!] Warning: ${{input:{var_id}}} in server " f"'{server_name}' will not be resolved \u2014 " f"{runtime_label} does not support input variable prompts" ) # after print( f"[!] Warning: ${{input:{var_id}}} in server " f"'{server_name}' will not be resolved -- " f"{runtime_label} does not support input variable prompts" )
Recommended (should fix in this PR since configure_mcp_server is already being modified):
-
Migrate cursor.py's four
print()calls to_rich_*helpers. This is a 4-line change that doesn't require plumbing a logger parameter — just import and call directly:from ...utils.console import _rich_error, _rich_success, _rich_info # Line 111 _rich_error("server_url cannot be empty", symbol="error") # Line 127 _rich_error(f"MCP server '{server_url}' not found in registry", symbol="error") # Line 143-145 _rich_success(f"Configured MCP server '{config_key}' for Cursor", symbol="success") # Line 149 _rich_error(f"Error configuring MCP server: {e}", symbol="error")
-
Add a verbose-mode message for the silent
.cursor/skip (cursor.py:116-117). Not blocking, but completes the diagnostic picture for--verboseusers.
Future (track separately):
- Migrate all adapters to accept an optional
logger: CommandLoggerparameter (following VSCode's existing pattern). This is a cross-cutting refactor and shouldn't block this PR.
VERDICT: Request Changes
- Blocking: V1 (em-dash
\u2014inbase.py) — ASCII compliance violation that actively fires through the code path this PR introduces - Strongly recommended: V2–V5 — since
configure_mcp_serveris being overridden in this PR, migrating its 4print()calls to_rich_*helpers is minimal effort and prevents the anti-pattern from calcifying - Nice to have: V6 — verbose-mode diagnostic for skipped adapter
Warning
⚠️ Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
astral.shpypi.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "astral.sh"
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #869 · ● 20.3M · ◷
🔒 Supply Chain Security Review — PR #869FINDINGS🔴 F1 — CRITICAL: Bearer token written to repo-local
|
| # | Severity | Requirement |
|---|---|---|
| R1 | 🔴 BLOCKER | Do not write resolved Bearer tokens to .cursor/mcp.json. The Cursor adapter must strip or redact Authorization headers from the config dict before it reaches update_config() → json.dump(). Options: (a) post-transform, replace the Authorization value with a ${env:GITHUB_TOKEN} placeholder that Cursor resolves at runtime, or (b) pop the Authorization header entirely and document that users must configure auth separately, or (c) use Cursor's native env-var-in-header support if available. |
| R2 | 🟡 SHOULD | Audit all resolved header values for secrets before disk write. Any header whose value came from _resolve_env_variable() should NOT be written as plaintext to a repo-local file. Consider writing placeholders or using Cursor's env block with variable references. |
| R3 | 🟢 NICE-TO-HAVE | Override _warn_input_variables calls to use "Cursor" as the runtime_label, or have the parent accept it as a class attribute. |
| R4 | 🟢 NICE-TO-HAVE | Consider adding .cursor/mcp.json to the project's .gitignore template as a defense-in-depth measure. |
VERDICT
🚫 Request Changes (for supply-chain security area)
R1 is a blocking credential leakage regression. The old Cursor code deliberately avoided injecting tokens; the super() delegation undoes that safety property. The refactor to delegate-then-transform is architecturally sound, but the transform step must also sanitize credentials before they reach the repo-local disk path. Until R1 is addressed, this PR should not merge.
Warning
⚠️ Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
astral.shpypi.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "astral.sh"
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #869 · ● 20.3M · ◷
🏗️ Python Architecture Review — PR #869Reviewer: Python Architect FINDINGSF1 ✅ Delegate-then-transform pattern is architecturally soundThe refactor from ~100 LOC copy-paste fork to
Key win: Cursor now automatically inherits all parent features — GitHub auth, header env-var resolution, F2
|
| Parent code path | Returns type = |
Cursor elif hit? |
|---|---|---|
_raw_stdio branch (L174-184) |
"local" |
No — hits the if "local" branch |
remotes branch (L188-236) |
"http" |
Yes — enters elif |
packages branch (L248-318) |
"local" |
No — hits the if "local" branch |
When the elif fires (parent returned "http" for remotes), the inner body does:
transport_type = server_info.get("remotes", [{}])[0].get("transport_type", "")
if transport_type in ("sse", "streamable-http"):
config["type"] = "http" # ← already "http", this is a no-opThe parent already emits "http" unconditionally for all remote types (L193: "type": "http"). The elif branch cannot change anything — it's vestigial from the old copy-paste implementation where Cursor had its own remote-handling logic with transport-type switching. There is no else clause that would alter the type, so "http" stays "http" regardless.
Impact: Not a bug (it's inert), but it's misleading. A reader will assume there's a reason for the transport_type check, and future maintainers may try to "fix" it.
F3 ✅ Signature compatibility is fine
The override adds Optional type hints that the parent lacks:
# Parent
def _format_server_config(self, server_info, env_overrides=None, runtime_vars=None):
# Override
def _format_server_config(self, server_info: dict, env_overrides: Optional[dict] = None, runtime_vars: Optional[dict] = None) -> dict:Python doesn't enforce signature covariance at runtime. The defaults match. The added hints are strictly additive — they improve tooling (mypy, IDE autocomplete) without breaking LSP. Good.
F4 i️ Pattern differs from OpenCode, but appropriately so
OpenCodeClientAdapter does not override _format_server_config. Instead, it delegates at the configure_mcp_server level and uses a static _to_opencode_format() to transform the Copilot-format dict into OpenCode schema (different key names: environment vs env, command array vs split).
Cursor's transformation is simpler (type rename + field removal), so overriding _format_server_config directly is the cleaner choice — it keeps the transform co-located with the formatting concern. The two adapters make different design tradeoffs appropriate to their schema distance from Copilot. No consistency issue here.
F5 ⚠️ Hardcoded "Copilot CLI" in _warn_input_variables calls
When Cursor's super()._format_server_config() calls _warn_input_variables(...), the parent hardcodes "Copilot CLI" as the runtime_label (copilot.py L179, L229):
self._warn_input_variables(raw["env"], server_info.get("name", ""), "Copilot CLI")This means a Cursor user will see: "Warning: ${input:...} will not be resolved — Copilot CLI does not support input variable prompts" — referring to the wrong runtime. This is a pre-existing issue (not introduced by this PR) but is now more visible because the delegate pattern actually exercises this code path for Cursor.
F6 ✅ Test coverage is good, with one gap
The test file covers:
- ✅
type: "local"→"stdio"(stdio, packages) - ✅
type: "http"preserved for remotes - ✅
toolsandidstripped - ✅ GitHub auth headers flow through
- ✅
_warn_input_variablesexercised - ✅ Header env-var resolution
⚠️ Missing: No test for_apm_tools_overridebeing stripped. Whenserver_infoincludes"_apm_tools_override": ["tool1"], the parent setsconfig["tools"] = ["tool1"], and Cursor should strip it. This path works (thepop("tools", None)is unconditional), but it's not explicitly tested.
REQUIRED CHANGES
R1. Remove the dead elif branch (F2)
The entire elif block on lines 172-177 is unreachable dead code that cannot change any output. It should be removed to prevent maintenance confusion:
def _format_server_config(self, server_info: dict, ...) -> dict:
config = super()._format_server_config(server_info, env_overrides, runtime_vars)
# Cursor uses "stdio" where Copilot uses "local"
if config.get("type") == "local":
config["type"] = "stdio"
# Parent already emits "http" for all remote types — no transform needed.
# Remove Copilot-only fields
config.pop("tools", None)
config.pop("id", None)
return configThis is a required change because dead code with a plausible-looking comment is an active maintainability hazard — someone will try to extend that branch thinking it does something.
OPTIONAL IMPROVEMENTS
O1. Add a test for _apm_tools_override stripping (F6)
def test_tools_override_stripped_for_cursor(self):
"""_apm_tools_override should be applied by parent but stripped for Cursor."""
server_info = {
"name": "my-cli",
"_raw_stdio": {"command": "./cli", "args": ["mcp"]},
"_apm_tools_override": ["specific-tool"],
}
config = self.adapter._format_server_config(server_info)
self.assertNotIn("tools", config)O2. Track the "Copilot CLI" label issue (F5)
Not a blocker for this PR, but the hardcoded "Copilot CLI" in parent's _warn_input_variables calls will show the wrong runtime label when invoked via Cursor or OpenCode. A follow-up could add a runtime_label class attribute to MCPClientAdapter so each adapter reports its own name. File an issue if not already tracked.
O3. Docstring: remove reference to SSE normalization
The class docstring (cursor.py L34) mentions sse/streamable-http transports normalized to "http" — if the elif is removed per R1, this line should be updated to simply note that the parent handles all remote types as "http".
VERDICT: Request Changes (one required change)
The architectural pattern is excellent — this is exactly how child adapters should relate to CopilotClientAdapter. The refactor eliminates ~80 LOC of duplicated logic and restores silently-dropped features. The single required change (R1: remove dead elif branch) is surgical and risk-free. Once that's addressed, this is a clean approve from the architecture perspective.
Warning
⚠️ Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
astral.shpypi.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "astral.sh"
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #869 · ● 20.3M · ◷
CEO / Strategic Owner — Final Verdict on PR #869Verdict: Request Changes — 1 security blocker + 3 required fixes, then this merges. To the contributor
Finding Classification
F1 — Token Leakage (Blocker): Detailed RationaleThis is the one finding I cannot defer. Here is the evidence chain:
The fix is small. In # After config = super()._format_server_config(...)
# Strip resolved bearer tokens — Cursor is repo-local, tokens must not hit disk
headers = config.get("headers", {})
if headers.get("Authorization", "").startswith("Bearer "):
del headers["Authorization"]
if not headers:
config.pop("headers", None)A follow-up issue can implement proper Cursor-compatible auth (env-var references like F2 — Dead
|
| Action | Effort |
|---|---|
| Strip resolved Bearer token from config before disk write | ~5 lines |
Remove dead elif branch |
~5 lines removed |
| Fix runtime label to say "Cursor" not "Copilot CLI" | ~3-10 lines |
| Update CHANGELOG to remove "restores auto-auth" claim | ~1 line |
Total: roughly 15 lines changed. The architectural pattern stays exactly as-is — these are surgical fixes on top of your excellent refactor. Once addressed, I'm happy to approve.
Thank you for your patience and for the high quality of this contribution. 🙏
Warning
⚠️ Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
astral.shpypi.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "astral.sh"
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #869 · ● 20.3M · ◷
APM Review Panel — PR #869
What this PR actually changesThe large diff is misleading — the branch has three upstream merge commits folded in. The real changes are six files:
The PR has two commits that tell the full story. The first commit ( [Python Architect]Verdict: APPROVE — with one optional simplification The delegate-then-transform pattern in the final state is correct. Call Finding: Dead code in the SSE normalization block (low severity, non-blocking) # cursor.py lines ~172-177
elif config.get("type") == "http":
# For remote endpoints, ensure type remains http (as opposed to sse, etc.)
# and normalize sse/streamable-http to http for Cursor
transport_type = server_info.get("remotes", [{}])[0].get("transport_type", "")
if transport_type in ("sse", "streamable-http"):
config["type"] = "http" # <-- no-op
The tests ( # After: all that's needed
if config.get("type") == "local":
config["type"] = "stdio"
config.pop("tools", None)
config.pop("id", None)Finding: The commit message says "Restore Observation on [Supply Chain Security + Auth Expert]Verdict: APPROVE — no regressions, GitHub auth correctly restored The GitHub Bearer token injection path is correctly restored. The parent's remote-handling code calls
self.assertEqual(server_cfg["headers"]["Authorization"], "Bearer ghp_test_token_12345")
self.assertNotIn("tools", server_cfg)
self.assertNotIn("id", server_cfg)parser.py fix is correct. On Windows, No new credential exposure. The [CLI Logging / UX Expert]Verdict: APPROVE Existing [DevX / UX Expert]Verdict: APPROVE — real user-facing fix Before this PR, installing any MCP server for Cursor via
All three are fixed at HEAD. The behavior is now correct for all server types: stdio packages, raw stdio, and HTTP remotes. The seven new tests cover the critical paths including GitHub Bearer auth injection and SSE/streamable-http normalization. Good test coverage for a bug fix. [OSS Growth / CEO] — ArbitrationVerdict: APPROVE and MERGE This is a quality external contribution. The two-commit story is exemplary: the author identified their own regression in the initial fix and proactively refactored to a better pattern. The final state is correct, well-tested, and meaningfully smaller than what it replaced. The two findings from the Architect (SSE dead code, The Ship it. Summary
Warning
|
APM Expert Review Panel — PR #869PR: VERDICT: REQUEST CHANGES (4 required items, then merge)The architectural refactor is exactly right and appreciated. The delegate-then-transform pattern is clean, the Findings by AreaPython Architect
Supply Chain Security
DevX / UX
CLI Logging
CEO ArbitrationToken leakage vs DevX: Security wins. The old code never injected PATs into Pre-existing parent-class issues (em-dash,
CHANGELOG "restores" wording: Since the token injection is being stripped (security fix), remove the "restores GitHub MCP auto-auth" claim. Net behavior: type normalization + label fix. Required Action Items Before Merge1. [Security -- Blocker] Strip # In _format_server_config, after config.pop("id", None):
if "headers" in config and "Authorization" in config.get("headers", {}):
del config["headers"]["Authorization"]
if not config.get("headers"):
config.pop("headers", None)2. [Architecture -- Required] Remove the dead 3. [UX -- Required] Fix the 4. [CHANGELOG -- Required] Remove the "restores GitHub MCP auto-auth" claim (the feature is being stripped per item 1). Accurately describe: type normalization ( Optional Follow-ups (Post-Merge Issues)
Review conducted by the APM Expert Panel (Python Architect, Supply Chain Security, DevX UX, CLI Logging, APM CEO). Estimated ~15 lines of additional changes needed. Warning
|
Cursor adapter previously forked ~106 lines of Copilot's _format_server_config and silently dropped GitHub MCP auth, header env-var resolution, _warn_input_variables, and _apm_tools_override -- recreating the same class of silent-failure bug as microsoft#844. - Replace copy-paste fork with super() delegate-then-transform (~20 lines) - Restore GitHub Bearer token injection for github-mcp-server remotes - Security: replace literal tokens with \ references so secrets never touch disk in .cursor/mcp.json - Restore header env-var resolution via _resolve_env_variable - Restore _warn_input_variables for \ references - Normalize sse/streamable-http transport types to 'http' for Cursor - Strip Copilot-only 'tools' and 'id' fields that cause Cursor to reject - Update docstrings to document delegate-then-transform pattern - Add Optional[dict] type hints to _format_server_config - Fix non-ASCII em dashes -> ASCII '--' - Catch (OSError, ValueError) in parser.py load_policy() for invalid paths - Add 7 new unit tests (GitHub auth, SSE normalization, edge cases) Fixes microsoft#844 Fixes microsoft#848
Resolved conflicts in: - CHANGELOG.md: combined PR microsoft#844/microsoft#848 changes with upstream microsoft#918 changes - src/apm_cli/policy/parser.py: kept _looks_like_yaml_content pattern from upstream, added (OSError, ValueError) handling - uv.lock: updated to version 0.10.0
|
I have updated the code again based on the Bot's suggestions and optimized the implementation.Please let me know if there are any further issues. Have a wonderful day! |
danielmeppiel
added
panel-review
APM Review Panel VerdictDisposition: REQUEST_CHANGES (cursor fix and policy-parser fix are clean; three blocking issues in the target-detection reversion and logging regressions must be addressed first) Per-persona findingsPython Architect: This PR bundles three architectural threads across 31 changed files. Thread 1 (cursor adapter) is clean. Thread 2 (target detection) is a major architectural reversion with two new bugs introduced. Thread 3 (marketplace GHES removal) is a deliberate scope reduction. Before: target-validation class structureclassDiagram
direction LR
class parse_target_field {
<<SharedValidator>>
+parse(value, source_path) Union[str,List,None]
}
class TargetParamType {
<<ClickParamType>>
+convert(value, param, ctx)
}
class APMPackage {
+from_apm_yml(path) APMPackage
}
class active_targets {
<<IOBoundary>>
+__call__(root, explicit_target) List
}
TargetParamType ..> parse_target_field : delegates
APMPackage ..> parse_target_field : delegates
active_targets ..> APMPackage : consumes target
class parse_target_field:::touched
classDef touched fill:#fff3b0,stroke:#d47600
After: target-validation class structureclassDiagram
direction LR
class TargetParamType {
<<ClickParamType>>
+convert(value, param, ctx)
-- inline validation (duplicated)
}
class APMPackage {
+from_apm_yml(path) APMPackage
-- raw data.get(target) passthrough
}
class CopilotClientAdapter {
+_format_server_config(server_info) dict
}
class CursorClientAdapter {
<<Adapter>>
+_format_server_config(server_info) dict
-- super()+transform pattern
}
class RefResolver {
-- no host/token params
+list_remote_refs(owner_repo) List
-- hardcoded github.com URL
}
class MarketplaceBuilder {
-- no _host, _host_info, _auth_resolved
+_fetch_remote_metadata(pkg) Optional
-- hardcoded raw.githubusercontent.com
}
CopilotClientAdapter <|-- CursorClientAdapter
MarketplaceBuilder *-- RefResolver
class CursorClientAdapter:::touched
class APMPackage:::touched
class RefResolver:::touched
class MarketplaceBuilder:::touched
classDef touched fill:#fff3b0,stroke:#d47600
Execution flow: apm.yml CSV target -> active_targets (AFTER, showing silent failure)flowchart TD
A["[FS] APMPackage.from_apm_yml(apm.yml)"] --> B["data.get('target') = 'claude,copilot'"]
B --> C["[FS] APMPackage(target='claude,copilot') raw string stored"]
C --> D["[FS] active_targets(root, explicit_target='claude,copilot')"]
D --> E{isinstance str?}
E --> F["KNOWN_TARGETS.get('claude,copilot') = None"]
F --> G["return []"]
G --> H["phases/targets.py: if ctx.logger and _targets: FALSE"]
H --> I["[SILENT] No warning emitted, exit 0"]
style G fill:#ffcccc
style I fill:#ffcccc
Design patternsDesign patterns
CLI Logging Expert: Two signal-level regressions in this PR:
Minor: DevX UX Expert: Three UX violations, one critical:
The Cursor MCP fix and policy-parser fix are excellent UX improvements on their own. Supply Chain Security Expert: Positives: The Concerns:
Auth Expert: Activated: This PR removes token injection from
Cursor adapter: OSS Growth Hacker: Side-channel to CEO: Cursor fix = concrete conversion win. Cursor is the fastest-growing AI IDE. A broken Silent zero-deployment = conversion killer. A new user follows a blog post, writes Recommendation to CEO: ship the Cursor fix and policy-parser fix now (high adoption value, zero risk). Gate the target-detection reversion behind explicit CHANGELOG communication and restore the zero-target warning before it lands. CEO arbitrationThe Cursor adapter fix (#844) and the policy-parser fix (#848) are clean, well-tested, and ready to merge. They represent real adoption wins -- especially the Cursor fix, which closes a silent schema incompatibility for a fast-growing IDE segment. The supply-chain specialists and growth hacker agree: ship these. The target-detection changes are a different story. The PR reverts #820 (shared validator, strict target parsing) without replacing the user-facing safety nets that #820 introduced. The three concrete regressions -- silent zero-deployment from CSV target strings in apm.yml, swallowed apm.yml parse errors in compile, and always-success message when zero files are written -- are not acceptable at the current APM adoption stage. Each one turns a misconfigured-user into a churned user with no recovery path. The removed "No targets resolved" warning was not defensive ceremony; it was the primary first-run recovery surface. The GHES marketplace removal is a defensible scope decision (github.com-first is a legitimate strategic choice) and is properly documented. The RefResolver token-auth loss is a follow-up concern the team should track. Strategic call: Restore the three safety nets (zero-target warning, zero-output compile warning, loud apm.yml parse error) before merging. These are 15-20 lines of code and do not conflict with the stated goal of this PR. The Cursor fix stands on its own and should not be held waiting. Required actions before merge
Optional follow-ups
|
danielmeppiel
added
panel-review
Description
Brief description of changes and motivation.
Fixes # (issue)
Type of change
Testing