From 572adb5721fda194a8274119774dba2e63834267 Mon Sep 17 00:00:00 2001 From: Christian Hessel Date: Tue, 21 Apr 2026 09:46:51 +0200 Subject: [PATCH] feat(codex): add project-scoped MCP and user target support --- CHANGELOG.md | 2 +- .../docs/integrations/ide-tool-integration.md | 7 +- .../content/docs/reference/cli-commands.md | 2 +- src/apm_cli/adapters/client/base.py | 46 +++- src/apm_cli/adapters/client/codex.py | 78 ++++--- src/apm_cli/adapters/client/copilot.py | 15 +- src/apm_cli/adapters/client/cursor.py | 4 +- src/apm_cli/adapters/client/opencode.py | 6 +- src/apm_cli/adapters/client/vscode.py | 16 +- src/apm_cli/commands/install.py | 28 ++- src/apm_cli/commands/uninstall/cli.py | 12 +- src/apm_cli/commands/uninstall/engine.py | 18 +- src/apm_cli/core/operations.py | 57 ++++- src/apm_cli/core/safe_installer.py | 19 +- src/apm_cli/factory.py | 16 +- src/apm_cli/integration/mcp_integrator.py | 139 +++++++++--- src/apm_cli/integration/targets.py | 1 + src/apm_cli/registry/operations.py | 69 ++++-- .../install/test_architecture_invariants.py | 10 +- .../integration/test_copilot_cowork_target.py | 9 +- .../integration/test_data_driven_dispatch.py | 15 +- .../unit/integration/test_hook_integrator.py | 14 ++ tests/unit/integration/test_mcp_integrator.py | 54 +++++ .../test_scope_install_uninstall.py | 50 ++++- .../integration/test_scope_integration.py | 25 ++- tests/unit/test_cursor_mcp.py | 22 ++ tests/unit/test_global_mcp_scope.py | 23 ++ tests/unit/test_mcp_client_factory.py | 71 ++++++- tests/unit/test_opencode_mcp.py | 23 ++ tests/unit/test_registry_integration.py | 59 +++++- tests/unit/test_runtime_detection.py | 14 +- tests/unit/test_transitive_mcp.py | 198 +++++++++++++++++- tests/unit/test_uninstall_engine_helpers.py | 14 +- 33 files changed, 993 insertions(+), 143 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d81384ae..ce9e03b0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ### Added +- Codex CLI MCP config is now project-local (`.codex/config.toml`) during project installs and gated to active project targets; Codex user-scope primitive deployment is also supported. (#803) - **Dev Container Feature** `ghcr.io/microsoft/apm/apm-cli` -- one-line install of the APM CLI into any `devcontainer.json`, GitHub Codespace, or JetBrains Gateway workspace. Supports a `version` option (`latest` or pinned semver), declares `installsAfter` for the official Python feature, handles PEP 668 on Ubuntu 24.04+. Ships with 37 bats unit tests and a 6-distro Docker integration matrix (Ubuntu 24.04, Ubuntu 22.04, Debian 12, Alpine 3.20, Fedora 41, plus Python-feature combo). (#861) - `shared/apm.md` gh-aw workflow gains an `apps:` array input for cross-org private packages: each entry mints its own GitHub App installation token via `actions/create-github-app-token` and packs only its declared packages, with a matrix fan-out one replica per credential group. The single-app top-level form (`app-id`, `private-key`, `owner`, `repositories`) shipped earlier in this cycle is preserved as the canonical shorthand for one-org users; `apps[]` is purely additive. Multi-bundle restore uses the `bundles-file:` input from `microsoft/apm-action@v1.5.0` (microsoft/apm-action#30, microsoft/apm-action#29). - `shared/apm.md` gh-aw workflow now accepts `app-id`, `private-key`, `owner`, and `repositories` inputs to mint a GitHub App installation token for fetching cross-org private APM packages, restoring parity with the deprecated `dependencies.github-app` form. The default `GH_AW_PLUGINS_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN` cascade still applies when no app-id is supplied. diff --git a/docs/src/content/docs/integrations/ide-tool-integration.md b/docs/src/content/docs/integrations/ide-tool-integration.md index 2add1fd12..26f4bdca6 100644 --- a/docs/src/content/docs/integrations/ide-tool-integration.md +++ b/docs/src/content/docs/integrations/ide-tool-integration.md @@ -489,14 +489,17 @@ APM configures MCP servers in the native config format for each supported client |--------|----------------|--------| | VS Code | `.vscode/mcp.json` | JSON `servers` object | | GitHub Copilot CLI | `~/.copilot/mcp-config.json` | JSON `mcpServers` object | -| Codex CLI | `~/.codex/config.toml` | TOML `mcp_servers` section | +| Codex CLI (project) | `.codex/config.toml` | TOML `mcp_servers` section | +| Codex CLI (`--global`) | `~/.codex/config.toml` | TOML `mcp_servers` section | | Claude | `.claude/settings.json` | JSON `mcpServers` object | | Cursor | `.cursor/mcp.json` | JSON `mcpServers` object | | Gemini CLI | `.gemini/settings.json` | JSON `mcpServers` object | **Runtime targeting**: APM detects which runtimes are installed and configures MCP servers for all of them. Use `--runtime ` or `--exclude ` to control which clients receive configuration. -> **VS Code detection**: APM considers VS Code available when either the `code` CLI command is on PATH **or** a `.vscode/` directory exists in the current working directory. This means VS Code MCP configuration works even when `code` is not on PATH — common on macOS and Linux when "Install 'code' command in PATH" has not been run from the VS Code command palette, or when VS Code was installed via a method that doesn't register the CLI (e.g. `.tar.gz`, Flatpak, or a non-standard macOS install location). +**Codex CLI**: Project installs write MCP configuration to `.codex/config.toml` only when Codex is an active project target. `--global` installs write to `~/.codex/config.toml`. + +> **VS Code detection**: APM considers VS Code available when either the `code` CLI command is on PATH **or** a `.vscode/` directory exists in the resolved project root (defaulting to the current working directory when no explicit project root is provided). This means VS Code MCP configuration works even when `code` is not on PATH — common on macOS and Linux when "Install 'code' command in PATH" has not been run from the VS Code command palette, or when VS Code was installed via a method that doesn't register the CLI (e.g. `.tar.gz`, Flatpak, or a non-standard macOS install location). ```bash # Install MCP dependencies for all detected runtimes diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index de0df2f78..aa7617c87 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -1976,7 +1976,7 @@ apm runtime setup llm **Default Behavior:** - Installs runtime binary from official sources - Configures with GitHub Models (free) as APM default -- Creates configuration file at `~/.codex/config.toml` or similar +- Creates Codex runtime configuration (global `~/.codex/config.toml`; project MCP config is managed separately in `.codex/config.toml`) - Provides clear logging about what's being configured **Vanilla Behavior (`--vanilla` flag):** diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index d5fae39d2..74a0b816b 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -1,6 +1,8 @@ """Base adapter interface for MCP clients.""" +import os import re +from pathlib import Path from abc import ABC, abstractmethod _INPUT_VAR_RE = re.compile(r"\$\{input:([^}]+)\}") @@ -15,14 +17,42 @@ class MCPClientAdapter(ABC): # so that ``apm install --global`` can install MCP servers to them. supports_user_scope: bool = False + def __init__( + self, + project_root: Path | str | None = None, + user_scope: bool = False, + ): + """Initialize the adapter with optional scope-aware path context. + + Args: + project_root: Project root used to resolve project-local config paths. + When not provided, adapters fall back to the current working + directory for project-scoped paths. + user_scope: Whether the adapter should resolve user-scope config + paths instead of project-local paths when supported. + """ + self._project_root = Path(project_root) if project_root is not None else None + self.user_scope = user_scope + + @property + def project_root(self) -> Path: + """Return the explicit project root or the current working directory.""" + if self._project_root is not None: + return self._project_root + return Path(os.getcwd()) + @abstractmethod def get_config_path(self): """Get the path to the MCP configuration file.""" pass @abstractmethod - def update_config(self, config_updates): - """Update the MCP configuration.""" + def update_config(self, config_updates) -> bool | None: + """Update the MCP configuration. + + Returns ``False`` or ``None`` when the config write was skipped + (for example because the existing file could not be parsed safely). + """ pass @abstractmethod @@ -120,6 +150,16 @@ def _warn_input_variables(mapping, server_name, runtime_label): seen.add(var_id) print( f"[!] Warning: ${{input:{var_id}}} in server " - f"'{server_name}' will not be resolved \u2014 " + f"'{server_name}' will not be resolved -- " f"{runtime_label} does not support input variable prompts" ) + + def normalize_project_arg(self, value): + """Normalize workspace placeholders for project-local runtimes.""" + if ( + not self.user_scope + and isinstance(value, str) + and value in {"${workspaceFolder}", "${projectRoot}", "${workspaceRoot}"} + ): + return "." + return value diff --git a/src/apm_cli/adapters/client/codex.py b/src/apm_cli/adapters/client/codex.py index 3c158d4ff..4894b1d70 100644 --- a/src/apm_cli/adapters/client/codex.py +++ b/src/apm_cli/adapters/client/codex.py @@ -1,47 +1,61 @@ -"""OpenAI Codex CLI implementation of MCP client adapter. - -This adapter implements the Codex CLI-specific handling of MCP server configuration, -targeting the global ~/.codex/config.toml file as specified in the MCP installation -architecture specification. -""" +"""OpenAI Codex CLI implementation of MCP client adapter.""" +import logging import os import toml from pathlib import Path from .base import MCPClientAdapter from ...registry.client import SimpleRegistryClient from ...registry.integration import RegistryIntegration +from ...utils.console import _rich_warning + + +_log = logging.getLogger(__name__) class CodexClientAdapter(MCPClientAdapter): """Codex CLI implementation of MCP client adapter. This adapter handles Codex CLI-specific configuration for MCP servers using - a global ~/.codex/config.toml file, following the TOML format for - MCP server configuration. + a scope-resolved config.toml file, following the TOML format for MCP + server configuration. """ - supports_user_scope: bool = True - def __init__(self, registry_url=None): + def __init__( + self, + registry_url=None, + project_root: Path | str | None = None, + user_scope: bool = False, + ): """Initialize the Codex CLI client adapter. Args: registry_url (str, optional): URL of the MCP registry. If not provided, uses the MCP_REGISTRY_URL environment variable or falls back to the default GitHub registry. + project_root: Project root used to resolve project-local Codex + config paths. + user_scope: Whether the adapter should resolve user-scope Codex + config paths instead of project-local paths. """ + super().__init__(project_root=project_root, user_scope=user_scope) self.registry_client = SimpleRegistryClient(registry_url) self.registry_integration = RegistryIntegration(registry_url) + + def _get_codex_dir(self): + """Return the root directory used for Codex config in the current scope.""" + if self.user_scope: + return Path.home() / ".codex" + return self.project_root / ".codex" def get_config_path(self): """Get the path to the Codex CLI MCP configuration file. Returns: - str: Path to ~/.codex/config.toml + str: Path to the scope-resolved Codex config.toml """ - codex_dir = Path.home() / ".codex" - return str(codex_dir / "config.toml") + return str(self._get_codex_dir() / "config.toml") def update_config(self, config_updates): """Update the Codex CLI MCP configuration. @@ -49,7 +63,10 @@ def update_config(self, config_updates): Args: config_updates (dict): Configuration updates to apply. """ + config_path = Path(self.get_config_path()) current_config = self.get_current_config() + if current_config is None: + return False # Ensure mcp_servers section exists if "mcp_servers" not in current_config: @@ -57,21 +74,21 @@ def update_config(self, config_updates): # Apply updates to mcp_servers section current_config["mcp_servers"].update(config_updates) - - # Write back to file - config_path = Path(self.get_config_path()) - + # Ensure directory exists config_path.parent.mkdir(parents=True, exist_ok=True) - - with open(config_path, 'w') as f: + + with open(config_path, 'w', encoding='utf-8') as f: toml.dump(current_config, f) + _log.debug("Codex config written to %s", config_path) + return True def get_current_config(self): """Get the current Codex CLI MCP configuration. Returns: - dict: Current configuration, or empty dict if file doesn't exist. + dict | None: Current configuration, empty dict if file doesn't + exist, or None when an existing config cannot be parsed safely. """ config_path = self.get_config_path() @@ -79,10 +96,19 @@ def get_current_config(self): return {} try: - with open(config_path, 'r') as f: + with open(config_path, 'r', encoding='utf-8') as f: return toml.load(f) - except (toml.TomlDecodeError, IOError): - return {} + except toml.TomlDecodeError as exc: + _log.debug("Failed to parse Codex config at %s", config_path, exc_info=True) + _rich_warning( + f"Could not parse {config_path}: {exc} -- " + "skipping config write to avoid data loss", + symbol="warning", + ) + return None + except IOError: + _log.debug("Failed to read Codex config at %s", config_path, exc_info=True) + return None def configure_mcp_server(self, server_url, server_name=None, enabled=True, env_overrides=None, server_info_cache=None, runtime_vars=None): """Configure an MCP server in Codex CLI configuration. @@ -148,7 +174,8 @@ def configure_mcp_server(self, server_url, server_name=None, enabled=True, env_o server_config = self._format_server_config(server_info, env_overrides, runtime_vars) # Update configuration using the chosen key - self.update_config({config_key: server_config}) + if not self.update_config({config_key: server_config}): + return False print(f"Successfully configured MCP server '{config_key}' for Codex CLI") return True @@ -180,7 +207,7 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No raw = server_info.get("_raw_stdio") if raw: config["command"] = raw["command"] - config["args"] = raw["args"] + config["args"] = [self.normalize_project_arg(arg) for arg in raw["args"]] if raw.get("env"): config["env"] = raw["env"] self._warn_input_variables(raw["env"], server_info.get("name", ""), "Codex CLI") @@ -555,4 +582,3 @@ def _select_best_package(self, packages): # If no priority package found, return the first one return packages[0] if packages else None - diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 9c1077c9f..a331e8914 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -23,17 +23,26 @@ class CopilotClientAdapter(MCPClientAdapter): a global ~/.copilot/mcp-config.json file, following the JSON format for MCP server configuration. """ - supports_user_scope: bool = True - def __init__(self, registry_url=None): + def __init__( + self, + registry_url=None, + project_root: Path | str | None = None, + user_scope: bool = False, + ): """Initialize the Copilot CLI client adapter. Args: registry_url (str, optional): URL of the MCP registry. If not provided, uses the MCP_REGISTRY_URL environment variable or falls back to the default GitHub registry. + project_root: Project root context passed through to the base + adapter for scope-aware operations. + user_scope: Whether the adapter should resolve user-scope config + paths instead of project-local paths when supported. """ + super().__init__(project_root=project_root, user_scope=user_scope) self.registry_client = SimpleRegistryClient(registry_url) self.registry_integration = RegistryIntegration(registry_url) @@ -749,4 +758,4 @@ def _is_github_server(self, server_name, url): # If URL parsing fails, assume it's not a GitHub server return False - return False \ No newline at end of file + return False diff --git a/src/apm_cli/adapters/client/cursor.py b/src/apm_cli/adapters/client/cursor.py index 92462ec0f..8390a2dc8 100644 --- a/src/apm_cli/adapters/client/cursor.py +++ b/src/apm_cli/adapters/client/cursor.py @@ -38,7 +38,7 @@ def get_config_path(self): ``.cursor/`` directory is *not* created automatically — APM only writes here when the directory already exists. """ - cursor_dir = Path(os.getcwd()) / ".cursor" + cursor_dir = self.project_root / ".cursor" return str(cursor_dir / "mcp.json") # ------------------------------------------------------------------ # @@ -102,7 +102,7 @@ def configure_mcp_server( return False # Opt-in: skip silently when .cursor/ does not exist - cursor_dir = Path(os.getcwd()) / ".cursor" + cursor_dir = self.project_root / ".cursor" if not cursor_dir.exists(): return True # nothing to do, not an error diff --git a/src/apm_cli/adapters/client/opencode.py b/src/apm_cli/adapters/client/opencode.py index 2ab434e7c..6adbcb63d 100644 --- a/src/apm_cli/adapters/client/opencode.py +++ b/src/apm_cli/adapters/client/opencode.py @@ -44,7 +44,7 @@ class OpenCodeClientAdapter(CopilotClientAdapter): def get_config_path(self): """Return the path to ``opencode.json`` in the repository root.""" - return str(Path(os.getcwd()) / "opencode.json") + return str(self.project_root / "opencode.json") def update_config(self, config_updates, enabled=True): """Merge *config_updates* into the ``mcp`` section of ``opencode.json``. @@ -55,7 +55,7 @@ def update_config(self, config_updates, enabled=True): Translates Copilot-format entries (``command``/``args``/``env``) into OpenCode format (``command`` array / ``environment``). """ - opencode_dir = Path(os.getcwd()) / ".opencode" + opencode_dir = self.project_root / ".opencode" if not opencode_dir.is_dir(): return @@ -99,7 +99,7 @@ def configure_mcp_server( print("Error: server_url cannot be empty") return False - opencode_dir = Path(os.getcwd()) / ".opencode" + opencode_dir = self.project_root / ".opencode" if not opencode_dir.is_dir(): return False diff --git a/src/apm_cli/adapters/client/vscode.py b/src/apm_cli/adapters/client/vscode.py index 3698004b8..d0849c082 100644 --- a/src/apm_cli/adapters/client/vscode.py +++ b/src/apm_cli/adapters/client/vscode.py @@ -21,14 +21,24 @@ class VSCodeClientAdapter(MCPClientAdapter): in the VSCode documentation. """ - def __init__(self, registry_url=None): + def __init__( + self, + registry_url=None, + project_root: Path | str | None = None, + user_scope: bool = False, + ): """Initialize the VSCode client adapter. Args: registry_url (str, optional): URL of the MCP registry. If not provided, uses the MCP_REGISTRY_URL environment variable or falls back to the default demo registry. + project_root: Project root used to resolve the repository-local + `.vscode/mcp.json` path. + user_scope: Whether to resolve user-scope config paths instead of + project-local paths when supported. """ + super().__init__(project_root=project_root, user_scope=user_scope) self.registry_client = SimpleRegistryClient(registry_url) self.registry_integration = RegistryIntegration(registry_url) @@ -38,8 +48,8 @@ def get_config_path(self, logger=None): Returns: str: Path to the .vscode/mcp.json file. """ - # Use the current working directory as the repository root - repo_root = Path(os.getcwd()) + # Use the resolved project root, which may be explicitly provided + repo_root = self.project_root # Path to .vscode/mcp.json in the repository vscode_dir = repo_root / ".vscode" diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index b8e9c3333..4e88bfb16 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1679,7 +1679,7 @@ def _install_apm_packages(ctx, outcome): # Also enter the APM install path when the project root has local .apm/ # primitives, even if there are no external APM dependencies (#714). - from apm_cli.core.scope import get_deploy_root as _get_deploy_root + from apm_cli.core.scope import InstallScope, get_deploy_root as _get_deploy_root _cli_project_root = _get_deploy_root(ctx.scope) apm_diagnostics = None @@ -1782,10 +1782,18 @@ def _install_apm_packages(ctx, outcome): # Continue with MCP installation (existing logic) mcp_count = 0 new_mcp_servers: builtins.set = builtins.set() + mcp_apm_config = { + "target": apm_package.target, + "scripts": apm_package.scripts or {}, + } if should_install_mcp and mcp_deps: mcp_count = MCPIntegrator.install( mcp_deps, ctx.runtime, ctx.exclude, ctx.verbose, stored_mcp_configs=old_mcp_configs, + apm_config=mcp_apm_config, + project_root=ctx.project_root, + user_scope=(ctx.scope is InstallScope.USER), + explicit_target=ctx.target, diagnostics=apm_diagnostics, scope=ctx.scope, ) @@ -1795,14 +1803,28 @@ def _install_apm_packages(ctx, outcome): # Remove stale MCP servers that are no longer needed stale_servers = old_mcp_servers - new_mcp_servers if stale_servers: - MCPIntegrator.remove_stale(stale_servers, ctx.runtime, ctx.exclude, scope=ctx.scope) + MCPIntegrator.remove_stale( + stale_servers, + ctx.runtime, + ctx.exclude, + project_root=ctx.project_root, + user_scope=(ctx.scope is InstallScope.USER), + scope=ctx.scope, + ) # Persist the new MCP server set and configs in the lockfile MCPIntegrator.update_lockfile(new_mcp_servers, mcp_configs=new_mcp_configs) elif should_install_mcp and not mcp_deps: # No MCP deps at all -- remove any old APM-managed servers if old_mcp_servers: - MCPIntegrator.remove_stale(old_mcp_servers, ctx.runtime, ctx.exclude, scope=ctx.scope) + MCPIntegrator.remove_stale( + old_mcp_servers, + ctx.runtime, + ctx.exclude, + project_root=ctx.project_root, + user_scope=(ctx.scope is InstallScope.USER), + scope=ctx.scope, + ) MCPIntegrator.update_lockfile(builtins.set(), mcp_configs={}) logger.verbose_detail("No MCP dependencies found in apm.yml") elif not should_install_mcp and old_mcp_servers: diff --git a/src/apm_cli/commands/uninstall/cli.py b/src/apm_cli/commands/uninstall/cli.py index fc2784284..a507be349 100644 --- a/src/apm_cli/commands/uninstall/cli.py +++ b/src/apm_cli/commands/uninstall/cli.py @@ -194,8 +194,16 @@ def uninstall(ctx, packages, dry_run, verbose, global_): # Step 10: MCP cleanup try: apm_package = APMPackage.from_apm_yml(manifest_path) - _cleanup_stale_mcp(apm_package, lockfile, lockfile_path, _pre_uninstall_mcp_servers, - modules_dir=get_modules_dir(scope), scope=scope) + _cleanup_stale_mcp( + apm_package, + lockfile, + lockfile_path, + _pre_uninstall_mcp_servers, + modules_dir=get_modules_dir(scope), + project_root=deploy_root, + user_scope=scope is InstallScope.USER, + scope=scope, + ) except Exception: logger.warning("MCP cleanup during uninstall failed") diff --git a/src/apm_cli/commands/uninstall/engine.py b/src/apm_cli/commands/uninstall/engine.py index 8fce28493..4137b147a 100644 --- a/src/apm_cli/commands/uninstall/engine.py +++ b/src/apm_cli/commands/uninstall/engine.py @@ -402,7 +402,16 @@ def _sync_integrations_after_uninstall(apm_package, project_root, all_deployed_f return counts -def _cleanup_stale_mcp(apm_package, lockfile, lockfile_path, old_mcp_servers, modules_dir=None, scope=None): +def _cleanup_stale_mcp( + apm_package, + lockfile, + lockfile_path, + old_mcp_servers, + modules_dir=None, + project_root=None, + user_scope: bool = False, + scope=None, +): """Remove MCP servers that are no longer needed after uninstall.""" if not old_mcp_servers: return @@ -416,5 +425,10 @@ def _cleanup_stale_mcp(apm_package, lockfile, lockfile_path, old_mcp_servers, mo new_mcp_servers = MCPIntegrator.get_server_names(all_remaining_mcp) stale_servers = old_mcp_servers - new_mcp_servers if stale_servers: - MCPIntegrator.remove_stale(stale_servers, scope=scope) + MCPIntegrator.remove_stale( + stale_servers, + project_root=project_root, + user_scope=user_scope, + scope=scope, + ) MCPIntegrator.update_lockfile(new_mcp_servers, lockfile_path) diff --git a/src/apm_cli/core/operations.py b/src/apm_cli/core/operations.py index 0b432482d..341595edc 100644 --- a/src/apm_cli/core/operations.py +++ b/src/apm_cli/core/operations.py @@ -1,21 +1,36 @@ """Core operations for APM.""" +from pathlib import Path + from ..factory import ClientFactory, PackageManagerFactory from .safe_installer import SafeMCPInstaller -def configure_client(client_type, config_updates): +def configure_client( + client_type, + config_updates, + project_root: Path | str | None = None, + user_scope: bool = False, +): """Configure an MCP client. Args: client_type (str): Type of client to configure. config_updates (dict): Configuration updates to apply. + project_root (str | Path | None): Project root used to resolve + project-local client config paths. + user_scope (bool): Whether to update user-scope config instead of + project-local config for runtimes that support it. Returns: bool: True if successful, False otherwise. """ try: - client = ClientFactory.create_client(client_type) + client = ClientFactory.create_client( + client_type, + project_root=project_root, + user_scope=user_scope, + ) client.update_config(config_updates) return True except Exception as e: @@ -23,7 +38,16 @@ def configure_client(client_type, config_updates): return False -def install_package(client_type, package_name, version=None, shared_env_vars=None, server_info_cache=None, shared_runtime_vars=None): +def install_package( + client_type, + package_name, + version=None, + shared_env_vars=None, + server_info_cache=None, + shared_runtime_vars=None, + project_root: Path | str | None = None, + user_scope: bool = False, +): """Install an MCP package for a specific client type. Args: @@ -33,13 +57,21 @@ def install_package(client_type, package_name, version=None, shared_env_vars=Non shared_env_vars (dict, optional): Pre-collected environment variables to use. server_info_cache (dict, optional): Pre-fetched server info to avoid duplicate registry calls. shared_runtime_vars (dict, optional): Pre-collected runtime variables to use. + project_root (str | Path | None): Project root used to resolve + project-local client config paths during installation. + user_scope (bool): Whether to install into user-scope config instead of + project-local config for runtimes that support it. Returns: dict: Result with 'success' (bool), 'installed' (bool), 'skipped' (bool) keys. """ try: # Use safe installer with conflict detection - safe_installer = SafeMCPInstaller(client_type) + safe_installer = SafeMCPInstaller( + client_type, + project_root=project_root, + user_scope=user_scope, + ) # Pass shared environment and runtime variables and server info cache if available if shared_env_vars is not None or server_info_cache is not None or shared_runtime_vars is not None: @@ -69,18 +101,31 @@ def install_package(client_type, package_name, version=None, shared_env_vars=Non } -def uninstall_package(client_type, package_name): +def uninstall_package( + client_type, + package_name, + project_root: Path | str | None = None, + user_scope: bool = False, +): """Uninstall an MCP package. Args: client_type (str): Type of client to configure. package_name (str): Name of the package to uninstall. + project_root (str | Path | None): Project root used to resolve + project-local client config paths during uninstall. + user_scope (bool): Whether to uninstall from user-scope config instead of + project-local config for runtimes that support it. Returns: bool: True if successful, False otherwise. """ try: - client = ClientFactory.create_client(client_type) + client = ClientFactory.create_client( + client_type, + project_root=project_root, + user_scope=user_scope, + ) package_manager = PackageManagerFactory.create_package_manager() # Uninstall the package diff --git a/src/apm_cli/core/safe_installer.py b/src/apm_cli/core/safe_installer.py index 6ce93af40..2ae63b380 100644 --- a/src/apm_cli/core/safe_installer.py +++ b/src/apm_cli/core/safe_installer.py @@ -2,6 +2,7 @@ from typing import List, Dict, Any, Optional from dataclasses import dataclass +from pathlib import Path from ..factory import ClientFactory from .conflict_detector import MCPConflictDetector from ..utils.console import _rich_warning, _rich_success, _rich_error @@ -58,15 +59,27 @@ def log_summary(self, logger=None): class SafeMCPInstaller: """Safe MCP server installation with conflict detection.""" - def __init__(self, runtime: str, logger=None): + def __init__( + self, + runtime: str, + logger=None, + project_root: Path | str | None = None, + user_scope: bool = False, + ): """Initialize the safe installer. Args: runtime: Target runtime (copilot, codex, vscode). logger: Optional CommandLogger for structured output. + project_root: Optional project root for repo-local runtime configs. + user_scope: Whether runtime config should resolve in user scope. """ self.runtime = runtime - self.adapter = ClientFactory.create_client(runtime) + self.adapter = ClientFactory.create_client( + runtime, + project_root=project_root, + user_scope=user_scope, + ) self.conflict_detector = MCPConflictDetector(self.adapter) self.logger = logger @@ -156,4 +169,4 @@ def check_conflicts_only(self, server_references: List[str]) -> Dict[str, Any]: for server_ref in server_references: conflicts[server_ref] = self.conflict_detector.get_conflict_summary(server_ref) - return conflicts \ No newline at end of file + return conflicts diff --git a/src/apm_cli/factory.py b/src/apm_cli/factory.py index 650750a7d..4c560b8a5 100644 --- a/src/apm_cli/factory.py +++ b/src/apm_cli/factory.py @@ -1,5 +1,7 @@ """Factory classes for creating adapters.""" +from pathlib import Path + from .adapters.client.vscode import VSCodeClientAdapter from .adapters.client.codex import CodexClientAdapter from .adapters.client.copilot import CopilotClientAdapter @@ -13,11 +15,18 @@ class ClientFactory: """Factory for creating MCP client adapters.""" @staticmethod - def create_client(client_type): + def create_client( + client_type, + project_root: Path | str | None = None, + user_scope: bool = False, + ): """Create a client adapter based on the specified type. Args: client_type (str): Type of client adapter to create. + project_root: Project root used to resolve repo-local config paths. + user_scope: Whether the adapter should use user-scope paths instead + of project-local paths when supported. Returns: MCPClientAdapter: An instance of the specified client adapter. @@ -38,7 +47,10 @@ def create_client(client_type): if client_type.lower() not in clients: raise ValueError(f"Unsupported client type: {client_type}") - return clients[client_type.lower()]() + return clients[client_type.lower()]( + project_root=project_root, + user_scope=user_scope, + ) class PackageManagerFactory: diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index e2b49f0cc..6fc74aff8 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -32,16 +32,21 @@ _log = logging.getLogger(__name__) -def _is_vscode_available() -> bool: +def _is_vscode_available(project_root: Path | str | None = None) -> bool: """Return True when VS Code can be targeted for MCP configuration. VS Code is considered available when either: - the ``code`` CLI command is on PATH (the standard case), or - - a ``.vscode/`` directory exists in the current working directory + - a ``.vscode/`` directory exists in the resolved project root (common on macOS where the user hasn't run "Install 'code' command in PATH" from the VS Code command palette). + + Args: + project_root: Project root to inspect for a `.vscode/` directory when + explicit project context is provided. Falls back to CWD when unset. """ - return shutil.which("code") is not None or (Path.cwd() / ".vscode").is_dir() + root = Path(project_root) if project_root is not None else Path.cwd() + return shutil.which("code") is not None or (root / ".vscode").is_dir() class MCPIntegrator: @@ -388,6 +393,8 @@ def _detect_mcp_config_drift( def _check_self_defined_servers_needing_installation( dep_names: list, target_runtimes: list, + project_root=None, + user_scope: bool = False, ) -> list: """Return self-defined MCP servers missing from at least one runtime. @@ -405,7 +412,11 @@ def _check_self_defined_servers_needing_installation( runtime_failures = [] for runtime in target_runtimes: try: - client = ClientFactory.create_client(runtime) + client = ClientFactory.create_client( + runtime, + project_root=project_root, + user_scope=user_scope, + ) detector = MCPConflictDetector(client) runtime_existing[runtime] = detector.get_existing_server_configs() except Exception: @@ -432,6 +443,8 @@ def remove_stale( stale_names: builtins.set, runtime: str = None, exclude: str = None, + project_root=None, + user_scope: bool = False, logger=None, scope=None, ) -> None: @@ -484,9 +497,11 @@ def remove_stale( if "/" in n: expanded_stale.add(n.rsplit("/", 1)[-1]) + project_root_path = Path(project_root) if project_root is not None else Path.cwd() + # Clean .vscode/mcp.json if "vscode" in target_runtimes: - vscode_mcp = Path.cwd() / ".vscode" / "mcp.json" + vscode_mcp = project_root_path / ".vscode" / "mcp.json" if vscode_mcp.exists(): try: import json as _json @@ -537,9 +552,17 @@ def remove_stale( exc_info=True, ) - # Clean ~/.codex/config.toml (mcp_servers section) + # Clean the scope-resolved Codex config.toml (mcp_servers section) if "codex" in target_runtimes: - codex_cfg = Path.home() / ".codex" / "config.toml" + from apm_cli.factory import ClientFactory + + codex_cfg = Path( + ClientFactory.create_client( + "codex", + project_root=project_root, + user_scope=user_scope, + ).get_config_path() + ) if codex_cfg.exists(): try: import toml as _toml @@ -564,7 +587,7 @@ def remove_stale( # Clean .cursor/mcp.json (only if .cursor/ directory exists) if "cursor" in target_runtimes: - cursor_mcp = Path.cwd() / ".cursor" / "mcp.json" + cursor_mcp = project_root_path / ".cursor" / "mcp.json" if cursor_mcp.exists(): try: import json as _json @@ -591,8 +614,8 @@ def remove_stale( # Clean opencode.json (only if .opencode/ directory exists) if "opencode" in target_runtimes: - opencode_cfg = Path.cwd() / "opencode.json" - if opencode_cfg.exists() and (Path.cwd() / ".opencode").is_dir(): + opencode_cfg = project_root_path / "opencode.json" + if opencode_cfg.exists() and (project_root_path / ".opencode").is_dir(): try: import json as _json @@ -740,7 +763,9 @@ def _filter_runtimes(detected_runtimes: List[str]) -> List[str]: except ImportError: mcp_compatible = [ - rt for rt in detected_runtimes if rt in ["vscode", "copilot", "cursor", "opencode", "gemini"] + rt + for rt in detected_runtimes + if rt in ["vscode", "copilot", "codex", "cursor", "opencode", "gemini"] ] return [rt for rt in mcp_compatible if shutil.which(rt)] @@ -755,6 +780,8 @@ def _install_for_runtime( shared_env_vars: dict = None, server_info_cache: dict = None, shared_runtime_vars: dict = None, + project_root=None, + user_scope: bool = False, logger=None, ) -> bool: """Install MCP dependencies for a specific runtime. @@ -765,10 +792,6 @@ def _install_for_runtime( logger = NullCommandLogger() try: from apm_cli.core.operations import install_package - from apm_cli.factory import ClientFactory - - # Get the appropriate client for the runtime - client = ClientFactory.create_client(runtime) all_ok = True for dep in mcp_deps: @@ -780,10 +803,22 @@ def _install_for_runtime( shared_env_vars=shared_env_vars, server_info_cache=server_info_cache, shared_runtime_vars=shared_runtime_vars, + project_root=project_root, + user_scope=user_scope, ) if result["failed"]: logger.error(f" Failed to install {dep}") all_ok = False + elif logger and runtime == "codex": + from apm_cli.factory import ClientFactory + + config_path = ClientFactory.create_client( + runtime, + project_root=project_root, + user_scope=user_scope, + ).get_config_path() + _log.debug("Codex config written to %s", config_path) + logger.verbose_detail(f" Codex config: {config_path}") except Exception as install_error: _log.debug( "Failed to install MCP dep %s for runtime %s", @@ -822,6 +857,9 @@ def install( verbose: bool = False, apm_config: dict = None, stored_mcp_configs: dict = None, + project_root=None, + user_scope: bool = False, + explicit_target: str | None = None, logger=None, diagnostics=None, scope=None, @@ -839,7 +877,10 @@ def install( stored_mcp_configs: Previously stored MCP configs from lockfile for diff-aware installation. When provided, servers whose manifest config has changed are re-applied automatically. - scope: InstallScope (PROJECT or USER). When USER, only + project_root: Project root for repo-local runtime configs. + user_scope: Whether runtime configuration is being resolved at user scope. + explicit_target: Explicit target selected by CLI or manifest. + scope: InstallScope (PROJECT or USER). When USER, only runtimes whose adapter declares ``supports_user_scope`` are targeted; workspace-only runtimes are skipped. @@ -852,6 +893,16 @@ def install( logger.warning("No MCP dependencies found in apm.yml") return 0 + from apm_cli.core.scope import InstallScope + + # The explicit scope enum takes precedence over the raw user_scope bool + # so callers cannot accidentally mix user-scope runtime filtering with + # project-scope config writes (or the inverse). + if scope is InstallScope.USER: + user_scope = True + elif scope is InstallScope.PROJECT: + user_scope = False + # Split into registry-resolved and self-defined deps # Backward compat: plain strings are treated as registry deps registry_deps = [ @@ -902,10 +953,12 @@ def install( target_runtimes = [runtime] logger.progress(f"Targeting specific runtime: {runtime}") else: + project_root_path = Path(project_root) if project_root is not None else Path.cwd() + if apm_config is None: # Lazy load -- only when the caller doesn't provide it try: - apm_yml = Path("apm.yml") + apm_yml = project_root_path / "apm.yml" if apm_yml.exists(): from apm_cli.utils.yaml_io import load_yaml apm_config = load_yaml(apm_yml) @@ -923,17 +976,17 @@ def install( for runtime_name in ["copilot", "codex", "vscode", "cursor", "opencode", "gemini"]: try: if runtime_name == "vscode": - if _is_vscode_available(): + if _is_vscode_available(project_root=project_root_path): ClientFactory.create_client(runtime_name) installed_runtimes.append(runtime_name) elif runtime_name == "cursor": # Cursor is opt-in: only target when .cursor/ exists - if (Path.cwd() / ".cursor").is_dir(): + if (project_root_path / ".cursor").is_dir(): ClientFactory.create_client(runtime_name) installed_runtimes.append(runtime_name) elif runtime_name == "opencode": # OpenCode is opt-in: only target when .opencode/ exists - if (Path.cwd() / ".opencode").is_dir(): + if (project_root_path / ".opencode").is_dir(): ClientFactory.create_client(runtime_name) installed_runtimes.append(runtime_name) elif runtime_name == "gemini": @@ -954,13 +1007,13 @@ def install( if shutil.which(rt) is not None ] # VS Code: check binary on PATH or .vscode/ directory presence - if _is_vscode_available(): + if _is_vscode_available(project_root=project_root_path): installed_runtimes.append("vscode") # Cursor is directory-presence based, not binary-based - if (Path.cwd() / ".cursor").is_dir(): + if (project_root_path / ".cursor").is_dir(): installed_runtimes.append("cursor") # OpenCode is directory-presence based - if (Path.cwd() / ".opencode").is_dir(): + if (project_root_path / ".opencode").is_dir(): installed_runtimes.append("opencode") # Gemini CLI is directory-presence based if (Path.cwd() / ".gemini").is_dir(): @@ -1040,10 +1093,35 @@ def install( target_runtimes = ["vscode"] logger.progress("No runtimes installed, using VS Code as fallback") + # Codex MCP is project-scoped: only configure it when Codex is an + # active project target, mirroring Cursor/OpenCode opt-in behavior. + if not user_scope and "codex" in target_runtimes: + from apm_cli.integration.targets import active_targets + + root = project_root or Path.cwd() + config_target = ( + explicit_target + or (apm_config.get("target") if apm_config else None) + ) + active = {t.name for t in active_targets(root, config_target)} + if "codex" not in active: + _log.debug("Codex gated out: active_targets=%s", sorted(active)) + target_runtimes = [r for r in target_runtimes if r != "codex"] + message = ( + "Codex not an active project target -- skipping MCP config " + "(create .codex/ or set target: codex in apm.yml)" + ) + if logger: + logger.progress(message) + else: + _rich_info(message, symbol="info") + + # Explicit runtime/exclusion/gating can leave nothing to configure. + if not target_runtimes: + return 0 + # Scope filtering: at USER scope, keep only global-capable runtimes. # Applied after both explicit --runtime and auto-discovery paths. - from apm_cli.core.scope import InstallScope - if scope is InstallScope.USER: from apm_cli.factory import ClientFactory as _CF @@ -1105,7 +1183,10 @@ def install( if valid_servers: servers_to_install = ( operations.check_servers_needing_installation( - target_runtimes, valid_servers + target_runtimes, + valid_servers, + project_root=project_root, + user_scope=user_scope, ) ) already_configured_candidates = [ @@ -1212,6 +1293,8 @@ def install( shared_env_vars, server_info_cache, shared_runtime_vars, + project_root=project_root, + user_scope=user_scope, logger=logger, ): any_ok = True @@ -1249,6 +1332,8 @@ def install( MCPIntegrator._check_self_defined_servers_needing_installation( self_defined_names, target_runtimes, + project_root=project_root, + user_scope=user_scope, ) ) already_configured_candidates_sd = [ @@ -1320,6 +1405,8 @@ def install( [dep.name], self_defined_env, self_defined_cache, + project_root=project_root, + user_scope=user_scope, logger=logger, ): any_ok = True diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index 9c746dad8..fccaae115 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -383,6 +383,7 @@ def for_scope(self, user_scope: bool = False) -> "TargetProfile | None": }, auto_create=False, detect_by_dir=True, + user_supported="partial", ), # Microsoft 365 Copilot (Cowork) -- experimental, user-scope only. # Skills are deployed to /Documents/Cowork/skills/. diff --git a/src/apm_cli/registry/operations.py b/src/apm_cli/registry/operations.py index 237d18daf..b5df7462f 100644 --- a/src/apm_cli/registry/operations.py +++ b/src/apm_cli/registry/operations.py @@ -24,7 +24,13 @@ def __init__(self, registry_url: Optional[str] = None): """ self.registry_client = SimpleRegistryClient(registry_url) - def check_servers_needing_installation(self, target_runtimes: List[str], server_references: List[str]) -> List[str]: + def check_servers_needing_installation( + self, + target_runtimes: List[str], + server_references: List[str], + project_root: Path | str | None = None, + user_scope: bool = False, + ) -> List[str]: """Check which MCP servers actually need installation across target runtimes. This method checks the actual MCP configuration files to see which servers @@ -33,16 +39,24 @@ def check_servers_needing_installation(self, target_runtimes: List[str], server_ Args: target_runtimes: List of target runtimes to check server_references: List of MCP server references (names or IDs) + project_root: Project root used to resolve project-local client config + paths when checking install status. + user_scope: Whether to inspect user-scope config instead of + project-local config for runtimes that support it. Returns: List of server references that need installation in at least one runtime """ servers_needing_installation = set() - # Pre-load installed IDs per runtime (O(R) reads instead of O(S*R)) - installed_by_runtime: Dict[str, Set[str]] = {} - for runtime in target_runtimes: - installed_by_runtime[runtime] = self._get_installed_server_ids([runtime]) + installed_by_runtime: Dict[str, Set[str]] = { + runtime: self._get_installed_server_ids( + [runtime], + project_root=project_root, + user_scope=user_scope, + ) + for runtime in target_runtimes + } # Check each server reference for server_ref in server_references: @@ -78,11 +92,20 @@ def check_servers_needing_installation(self, target_runtimes: List[str], server_ return list(servers_needing_installation) - def _get_installed_server_ids(self, target_runtimes: List[str]) -> Set[str]: + def _get_installed_server_ids( + self, + target_runtimes: List[str], + project_root: Path | str | None = None, + user_scope: bool = False, + ) -> Set[str]: """Get all installed server IDs across target runtimes. Args: target_runtimes: List of runtimes to check + project_root: Project root used to resolve project-local client config + paths while inspecting installed server IDs. + user_scope: Whether to inspect user-scope config instead of + project-local config for runtimes that support it. Returns: Set of server IDs that are currently installed @@ -97,7 +120,11 @@ def _get_installed_server_ids(self, target_runtimes: List[str]) -> Set[str]: for runtime in target_runtimes: try: - client = ClientFactory.create_client(runtime) + client = ClientFactory.create_client( + runtime, + project_root=project_root, + user_scope=user_scope, + ) config = client.get_current_config() if isinstance(config, dict): @@ -120,18 +147,20 @@ def _get_installed_server_ids(self, target_runtimes: List[str]) -> Set[str]: installed_ids.add(server_id) elif runtime == 'vscode': - # VS Code stores servers in settings.json with different structure - # Check both mcpServers and any nested structure - mcp_servers = config.get("mcpServers", {}) - for server_name, server_config in mcp_servers.items(): - if isinstance(server_config, dict): - server_id = ( - server_config.get("id") or - server_config.get("serverId") or - server_config.get("server_id") - ) - if server_id: - installed_ids.add(server_id) + # VS Code stores project-local MCP config in .vscode/mcp.json + # under the top-level "servers" key. Keep legacy fallbacks for + # older settings.json-style structures when present. + for key in ("servers", "mcpServers"): + mcp_servers = config.get(key, {}) + for server_name, server_config in mcp_servers.items(): + if isinstance(server_config, dict): + server_id = ( + server_config.get("id") or + server_config.get("serverId") or + server_config.get("server_id") + ) + if server_id: + installed_ids.add(server_id) except Exception: # If we can't read a runtime's config, skip it @@ -430,4 +459,4 @@ def _prompt_for_environment_variables(self, required_vars: Dict[str, Dict]) -> D click.echo() - return env_vars \ No newline at end of file + return env_vars diff --git a/tests/unit/install/test_architecture_invariants.py b/tests/unit/install/test_architecture_invariants.py index 5c0e16138..c17ffca17 100644 --- a/tests/unit/install/test_architecture_invariants.py +++ b/tests/unit/install/test_architecture_invariants.py @@ -145,12 +145,18 @@ def test_install_py_under_legacy_budget(): ``_validate_and_add_packages_to_apm_yml()`` from ~50 to ~10. This is a structural improvement, not feature growth -- the follow-up file-split into ``apm_cli/install/`` will recover the budget. + + PR #803 rebase follow-up raised 1950 -> 1980 to keep the + scope-aware Codex MCP arguments threaded through the extracted + ``_install_apm_packages()`` helper after upstream rebases. This is + still helper overhead inside the same pending file-split work, not + new install surface area. """ install_py = Path(__file__).resolve().parents[3] / "src" / "apm_cli" / "commands" / "install.py" assert install_py.is_file() n = _line_count(install_py) - assert n <= 1950, ( - f"commands/install.py grew to {n} LOC (budget 1950). " + assert n <= 1980, ( + f"commands/install.py grew to {n} LOC (budget 1980). " "Do NOT trim cosmetically -- engage the python-architecture skill " "(.github/skills/python-architecture/SKILL.md) and propose an " "extraction into apm_cli/install/." diff --git a/tests/unit/integration/test_copilot_cowork_target.py b/tests/unit/integration/test_copilot_cowork_target.py index 4f2747ce3..89905d01d 100644 --- a/tests/unit/integration/test_copilot_cowork_target.py +++ b/tests/unit/integration/test_copilot_cowork_target.py @@ -97,8 +97,13 @@ def test_for_scope_non_resolver_user_supported_returns_profile( def test_for_scope_non_resolver_user_unsupported_returns_none( self, ) -> None: - codex = KNOWN_TARGETS["codex"] - result = codex.for_scope(user_scope=True) + unsupported = TargetProfile( + name="dummy", + root_dir=".dummy", + primitives={}, + user_supported=False, + ) + result = unsupported.for_scope(user_scope=True) assert result is None diff --git a/tests/unit/integration/test_data_driven_dispatch.py b/tests/unit/integration/test_data_driven_dispatch.py index 20a61c971..b9274211b 100644 --- a/tests/unit/integration/test_data_driven_dispatch.py +++ b/tests/unit/integration/test_data_driven_dispatch.py @@ -670,12 +670,14 @@ def test_project_scope_returns_self(self): copilot = KNOWN_TARGETS["copilot"] assert copilot.for_scope(user_scope=False) is copilot - def test_unsupported_target_returns_none(self): - """for_scope returns None for targets that don't support user scope.""" + def test_codex_is_supported_at_user_scope(self): + """Codex resolves cleanly at user scope.""" from apm_cli.integration.targets import KNOWN_TARGETS codex = KNOWN_TARGETS["codex"] - assert codex.user_supported is False - assert codex.for_scope(user_scope=True) is None + assert codex.user_supported == "partial" + resolved = codex.for_scope(user_scope=True) + assert resolved is not None + assert resolved.root_dir == ".codex" def test_resolves_root_dir_to_user_root(self): """for_scope replaces root_dir with user_root_dir.""" @@ -747,13 +749,12 @@ def test_resolve_targets_project_scope(self): assert targets[0].root_dir == ".github" def test_resolve_targets_filters_unsupported(self): - """resolve_targets at user scope excludes unsupported targets.""" + """resolve_targets at user scope includes all user-capable targets.""" from apm_cli.integration.targets import resolve_targets, KNOWN_TARGETS from pathlib import Path targets = resolve_targets(Path.home(), user_scope=True, explicit_target="all") target_names = {t.name for t in targets} - # Codex doesn't support user scope - assert "codex" not in target_names + assert "codex" in target_names # =================================================================== diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 4e2f6ffa3..972b83ed5 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -2416,6 +2416,20 @@ def test_merged_hooks_use_target_root_dir(self): assert result.files_integrated > 0 assert (self.root / ".claude" / "settings.json").exists() + def test_codex_hooks_use_scope_resolved_root_dir(self): + """Codex hooks at user scope merge into .codex/hooks.json.""" + codex_target = self._make_target("codex", ".codex") + (self.root / ".codex").mkdir() + pi = _make_package_info(self.pkg_dir, "scope-pkg") + integrator = HookIntegrator() + + result = integrator.integrate_hooks_for_target( + codex_target, pi, self.root, + ) + + assert result.files_integrated > 0 + assert (self.root / ".codex" / "hooks.json").exists() + def test_script_paths_rewritten_with_scope_root(self): """Script paths in hook commands use the scope-resolved root_dir.""" # Create a hook with a script reference diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index 3fd489151..26578d826 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -616,6 +616,60 @@ def test_target_restricted_to_requested_runtime(self, tmp_path): copilot_remaining = json.loads(copilot_mcp.read_text()) assert "stale" in copilot_remaining["mcpServers"] + def test_removes_stale_server_from_vscode_with_explicit_project_root(self, tmp_path): + nested = tmp_path / "nested-project" + vscode_dir = nested / ".vscode" + self._write_vscode_mcp(vscode_dir, {"old-server": {}, "keep-server": {}}) + + with patch("apm_cli.integration.mcp_integrator.Path.cwd", return_value=tmp_path), \ + patch("pathlib.Path.home", return_value=tmp_path): + MCPIntegrator.remove_stale( + {"old-server"}, + runtime="vscode", + project_root=nested, + ) + + remaining = json.loads((vscode_dir / "mcp.json").read_text()) + assert "old-server" not in remaining["servers"] + assert "keep-server" in remaining["servers"] + + +class TestInstallProjectRootDetection: + @patch("apm_cli.registry.operations.MCPServerOperations") + @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime") + @patch("apm_cli.runtime.manager.RuntimeManager") + @patch("apm_cli.integration.mcp_integrator.shutil.which", return_value=None) + def test_install_uses_explicit_project_root_for_workspace_runtime_detection( + self, _which, mock_mgr_cls, mock_install_rt, mock_ops_cls, tmp_path + ): + nested = tmp_path / "nested-project" + (nested / ".cursor").mkdir(parents=True) + (nested / ".opencode").mkdir() + (nested / ".vscode").mkdir() + + mock_mgr = mock_mgr_cls.return_value + mock_mgr.is_runtime_available.return_value = False + mock_install_rt.return_value = True + + mock_ops = mock_ops_cls.return_value + mock_ops.validate_servers_exist.return_value = (["test/server"], []) + mock_ops.check_servers_needing_installation.return_value = ["test/server"] + mock_ops.batch_fetch_server_info.return_value = {"test/server": {}} + mock_ops.collect_environment_variables.return_value = {} + mock_ops.collect_runtime_variables.return_value = {} + + with patch("apm_cli.integration.mcp_integrator.Path.cwd", return_value=tmp_path): + MCPIntegrator.install( + mcp_deps=["test/server"], + project_root=nested, + apm_config={}, + ) + + called_runtimes = {call.args[0] for call in mock_install_rt.call_args_list} + assert "vscode" in called_runtimes + assert "cursor" in called_runtimes + assert "opencode" in called_runtimes + # =========================================================================== # MCPIntegrator.remove_stale - copilot diff --git a/tests/unit/integration/test_scope_install_uninstall.py b/tests/unit/integration/test_scope_install_uninstall.py index fd110d31b..fce6ac459 100644 --- a/tests/unit/integration/test_scope_install_uninstall.py +++ b/tests/unit/integration/test_scope_install_uninstall.py @@ -693,10 +693,29 @@ def test_project_scope(self): for p in deployed: assert not (self.project_root / p).exists() - def test_user_scope_returns_none(self): - """Codex does not support user scope -- for_scope returns None.""" - result = KNOWN_TARGETS["codex"].for_scope(user_scope=True) - assert result is None + def test_user_scope(self): + """Codex agents deploy to .codex/agents/ at user scope as well.""" + target = KNOWN_TARGETS["codex"].for_scope(user_scope=True) + assert target is not None + (self.project_root / ".codex").mkdir() + + pkg_info = _make_pkg( + self.project_root, + instructions=False, + agents=True, + commands=False, + ) + + agent_integrator = AgentIntegrator() + agent_result = agent_integrator.integrate_agents_for_target( + target, pkg_info, self.project_root + ) + + assert agent_result.files_integrated >= 1 + + deployed = _posix_relpaths(self.project_root, agent_result.target_paths) + assert any(p.startswith(".codex/agents/") for p in deployed) + assert any(p.endswith(".toml") for p in deployed) # --------------------------------------------------------------------------- @@ -879,6 +898,29 @@ def test_codex_project_scope(self): for p in deployed: assert not (self.project_root / p).exists() + def test_codex_user_scope(self): + """Codex skills keep using .agents/skills/ at user scope.""" + target = KNOWN_TARGETS["codex"].for_scope(user_scope=True) + assert target is not None + (self.project_root / ".codex").mkdir() + + pkg_info = _make_pkg( + self.project_root, + name="test-skill", + instructions=False, + agents=False, + skills=True, + ) + + integrator = SkillIntegrator() + result = integrator.integrate_package_skill( + pkg_info, self.project_root, targets=[target] + ) + + assert result.skill_created or result.skill_updated + deployed = _posix_relpaths(self.project_root, result.target_paths) + assert any(p.startswith(".agents/skills/") for p in deployed) + def test_claude_project_scope(self): """Skill deploys to .claude/skills/ at project scope.""" target = KNOWN_TARGETS["claude"].for_scope(user_scope=False) diff --git a/tests/unit/integration/test_scope_integration.py b/tests/unit/integration/test_scope_integration.py index 5cdfab060..bfd0da506 100644 --- a/tests/unit/integration/test_scope_integration.py +++ b/tests/unit/integration/test_scope_integration.py @@ -196,23 +196,28 @@ def test_project_scope_agents_deploy_to_opencode(self): ).exists() -# -- Codex exclusion at user scope ------------------------------------------- +# -- Codex user-scope behavior ---------------------------------------------- -class TestCodexScopeExclusion: - """Verify Codex is excluded at user scope.""" +class TestCodexUserScope: + """Verify Codex participates in user-scope target resolution.""" - def test_for_scope_returns_none(self): + def test_for_scope_returns_profile(self): codex = KNOWN_TARGETS["codex"] - assert codex.user_supported is False - assert codex.for_scope(user_scope=True) is None + assert codex.user_supported == "partial" + resolved = codex.for_scope(user_scope=True) + assert resolved is not None + assert resolved.root_dir == ".codex" + assert "agents" in resolved.primitives + assert "skills" in resolved.primitives + assert "hooks" in resolved.primitives - def test_resolve_targets_excludes_codex_at_user_scope(self): + def test_resolve_targets_includes_codex_at_user_scope(self): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp) targets = resolve_targets(root, user_scope=True, explicit_target="all") names = {t.name for t in targets} - assert "codex" not in names + assert "codex" in names # -- Claude same-root behavior ----------------------------------------------- @@ -248,8 +253,8 @@ def test_all_targets_at_user_scope_have_correct_roots(self): Path(tmp), user_scope=True, explicit_target="all" ) root_map = {t.name: t.root_dir for t in targets} - # Codex should be excluded - assert "codex" not in root_map + # Codex keeps .codex at user scope + assert root_map["codex"] == ".codex" # Copilot should use .copilot if "copilot" in root_map: assert root_map["copilot"] == ".copilot" diff --git a/tests/unit/test_cursor_mcp.py b/tests/unit/test_cursor_mcp.py index 153253c11..3c5fd8e8c 100644 --- a/tests/unit/test_cursor_mcp.py +++ b/tests/unit/test_cursor_mcp.py @@ -159,6 +159,28 @@ def test_remove_stale_cursor_noop_when_no_file(self): MCPIntegrator.remove_stale({"stale"}, runtime="cursor") # No exception is the assertion + def test_remove_stale_cursor_uses_explicit_project_root(self): + from apm_cli.integration.mcp_integrator import MCPIntegrator + + other_root = Path(self.tmp.name) / "nested-project" + cursor_dir = other_root / ".cursor" + cursor_dir.mkdir(parents=True) + mcp_json = cursor_dir / "mcp.json" + mcp_json.write_text( + json.dumps({"mcpServers": {"keep": {"command": "k"}, "stale": {"command": "s"}}}), + encoding="utf-8", + ) + + MCPIntegrator.remove_stale( + {"stale"}, + runtime="cursor", + project_root=other_root, + ) + + data = json.loads(mcp_json.read_text(encoding="utf-8")) + self.assertIn("keep", data["mcpServers"]) + self.assertNotIn("stale", data["mcpServers"]) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_global_mcp_scope.py b/tests/unit/test_global_mcp_scope.py index 17025d311..00e83065f 100644 --- a/tests/unit/test_global_mcp_scope.py +++ b/tests/unit/test_global_mcp_scope.py @@ -210,6 +210,29 @@ def test_user_scope_explicit_global_runtime_proceeds(self): self.assertTrue(mock_install.called) self.assertEqual(mock_install.call_args_list[0].args[0], "copilot") + def test_scope_user_overrides_false_user_scope_flag(self): + """USER scope should force user-scope path resolution even if the boolean disagrees.""" + from apm_cli.integration.mcp_integrator import MCPIntegrator + + with patch.object( + MCPIntegrator, "_install_for_runtime", return_value=True + ) as mock_install, patch( + "apm_cli.registry.operations.MCPServerOperations" + ) as mock_ops_cls: + mock_ops = MagicMock() + mock_ops.validate_servers_exist.return_value = (["test/server"], []) + mock_ops.check_servers_needing_installation.return_value = ["test/server"] + mock_ops_cls.return_value = mock_ops + + MCPIntegrator.install( + mcp_deps=["test/server"], + runtime="copilot", + scope=InstallScope.USER, + user_scope=False, + ) + + assert mock_install.call_args.kwargs["user_scope"] is True + def test_scope_none_treated_as_project(self): """When scope is None, all runtimes are eligible (backward compat).""" from apm_cli.integration.mcp_integrator import MCPIntegrator diff --git a/tests/unit/test_mcp_client_factory.py b/tests/unit/test_mcp_client_factory.py index f1522e1c3..9270c5a48 100644 --- a/tests/unit/test_mcp_client_factory.py +++ b/tests/unit/test_mcp_client_factory.py @@ -78,8 +78,15 @@ def tearDown(self): self.temp_dir.cleanup() def test_get_config_path_default(self): - """Test default config path for Codex CLI.""" - adapter = CodexClientAdapter() + """Test project-scope config path for Codex CLI.""" + project_root = Path(self.temp_dir.name) / "workspace" + adapter = CodexClientAdapter(project_root=project_root) + expected_path = str(project_root / ".codex" / "config.toml") + self.assertEqual(adapter.get_config_path(), expected_path) + + def test_get_config_path_user_scope(self): + """Test user-scope config path for Codex CLI.""" + adapter = CodexClientAdapter(user_scope=True) expected_path = str(Path.home() / ".codex" / "config.toml") self.assertEqual(adapter.get_config_path(), expected_path) @@ -89,6 +96,39 @@ def test_get_current_config_existing(self): self.assertEqual(config["model_provider"], "github-models") self.assertEqual(config["model"], "gpt-4o-mini") + + def test_get_current_config_invalid_toml_returns_none(self): + """Invalid existing TOML should not be treated as an empty config.""" + Path(self.config_path).write_text('invalid = "unterminated', encoding="utf-8") + + with patch("apm_cli.adapters.client.codex._rich_warning") as mock_warn: + config = self.adapter.get_current_config() + + self.assertIsNone(config) + mock_warn.assert_called_once() + + @patch('apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference') + def test_configure_mcp_server_does_not_overwrite_invalid_toml(self, mock_find_server): + """Parse failures should skip writes to avoid destroying existing config.""" + Path(self.config_path).write_text('invalid = "unterminated', encoding="utf-8") + mock_find_server.return_value = { + "id": "test-id", + "name": "test-server", + "packages": [{ + "registry_name": "npm", + "name": "test-package", + "arguments": [] + }], + "environment_variables": [] + } + + original = Path(self.config_path).read_text(encoding="utf-8") + with patch("apm_cli.adapters.client.codex._rich_warning") as mock_warn: + result = self.adapter.configure_mcp_server("test-server", "my_server") + + self.assertFalse(result) + self.assertEqual(Path(self.config_path).read_text(encoding="utf-8"), original) + self.assertTrue(mock_warn.called) @patch('apm_cli.registry.client.SimpleRegistryClient.find_server_by_reference') def test_configure_mcp_server_basic(self, mock_find_server): @@ -212,6 +252,31 @@ def test_configure_mcp_server_name_extraction(self, mock_find_server): self.assertIn("azure-devops-mcp", config["mcp_servers"]) # Should extract name after slash self.assertNotIn("microsoft/azure-devops-mcp", config["mcp_servers"]) # Should NOT use full path + def test_self_defined_stdio_normalizes_project_placeholders(self): + """Project-local Codex configs normalize VS Code placeholders to '.'.""" + adapter = CodexClientAdapter(project_root=Path(self.temp_dir.name)) + server_info = { + "id": "stdio-id", + "name": "local-filesystem", + "_raw_stdio": { + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-filesystem", + "${workspaceFolder}", + "${projectRoot}", + ], + "env": {}, + }, + } + + config = adapter._format_server_config(server_info) + + self.assertEqual( + config["args"], + ["-y", "@modelcontextprotocol/server-filesystem", ".", "."], + ) + if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() diff --git a/tests/unit/test_opencode_mcp.py b/tests/unit/test_opencode_mcp.py index 9e34a8fb4..516d754f1 100644 --- a/tests/unit/test_opencode_mcp.py +++ b/tests/unit/test_opencode_mcp.py @@ -363,6 +363,29 @@ def test_remove_stale_opencode_noop_when_no_file(self): MCPIntegrator.remove_stale({"stale"}, runtime="opencode") # No exception is the assertion + def test_remove_stale_opencode_uses_explicit_project_root(self): + from apm_cli.integration.mcp_integrator import MCPIntegrator + + other_root = Path(self.tmp.name) / "nested-project" + (other_root / ".opencode").mkdir(parents=True) + opencode_json = other_root / "opencode.json" + opencode_json.write_text( + json.dumps( + {"mcp": {"keep": {"type": "local"}, "stale": {"type": "remote"}}} + ), + encoding="utf-8", + ) + + MCPIntegrator.remove_stale( + {"stale"}, + runtime="opencode", + project_root=other_root, + ) + + data = json.loads(opencode_json.read_text(encoding="utf-8")) + self.assertIn("keep", data["mcp"]) + self.assertNotIn("stale", data["mcp"]) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_registry_integration.py b/tests/unit/test_registry_integration.py index e468b6aa8..8ad44028d 100644 --- a/tests/unit/test_registry_integration.py +++ b/tests/unit/test_registry_integration.py @@ -250,6 +250,51 @@ def side_effect(ref): self.assertEqual(sorted(valid), ["flaky", "found"]) self.assertEqual(invalid, ["missing"]) + def test_check_servers_needing_installation_reads_each_runtime_once(self): + """Installed server IDs are cached per runtime across server checks.""" + ops = self._make_ops() + + def find_server(ref): + return {"id": f"id-{ref}", "name": ref} + + ops.registry_client.find_server_by_reference.side_effect = find_server + ops._get_installed_server_ids = mock.MagicMock( + side_effect=[ + {"id-s1"}, + set(), + ] + ) + + result = ops.check_servers_needing_installation( + ["codex", "cursor"], + ["s1", "s2"], + ) + + self.assertEqual(sorted(result), ["s1", "s2"]) + self.assertEqual(ops._get_installed_server_ids.call_count, 2) + self.assertEqual( + [call.args[0] for call in ops._get_installed_server_ids.call_args_list], + [["codex"], ["cursor"]], + ) + + @mock.patch("apm_cli.factory.ClientFactory.create_client") + def test_get_installed_server_ids_reads_vscode_servers_key(self, mock_create_client): + """VS Code installed IDs should be read from .vscode/mcp.json's servers key.""" + from apm_cli.registry.operations import MCPServerOperations + + ops = MCPServerOperations.__new__(MCPServerOperations) + mock_client = mock.MagicMock() + mock_client.get_current_config.return_value = { + "servers": { + "example": {"id": "server-123"}, + } + } + mock_create_client.return_value = mock_client + + installed = ops._get_installed_server_ids(["vscode"]) + + self.assertEqual(installed, {"server-123"}) + class TestCheckServersNeedingInstallation(unittest.TestCase): """Tests for MCPServerOperations.check_servers_needing_installation caching.""" @@ -283,8 +328,16 @@ def test_caches_runtime_lookups(self): # _get_installed_server_ids called exactly once per runtime self.assertEqual(ops._get_installed_server_ids.call_count, 2) - ops._get_installed_server_ids.assert_any_call(["r1"]) - ops._get_installed_server_ids.assert_any_call(["r2"]) + ops._get_installed_server_ids.assert_any_call( + ["r1"], + project_root=None, + user_scope=False, + ) + ops._get_installed_server_ids.assert_any_call( + ["r2"], + project_root=None, + user_scope=False, + ) # All three need installation because none are installed in *all* runtimes: # srv-a: installed in r1 but missing from r2 → needs install @@ -336,4 +389,4 @@ def test_registry_error_flags_for_installation(self): if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() diff --git a/tests/unit/test_runtime_detection.py b/tests/unit/test_runtime_detection.py index 386becbe8..4b5e215cd 100644 --- a/tests/unit/test_runtime_detection.py +++ b/tests/unit/test_runtime_detection.py @@ -214,6 +214,18 @@ def _run(self, code_on_path: bool, vscode_dir_exists: bool) -> bool: mock_cwd.return_value = mock_vscode return _is_vscode_available() + def test_vscode_detected_via_explicit_project_root(self): + """Explicit project_root should be used instead of CWD for .vscode detection.""" + from apm_cli.integration.mcp_integrator import _is_vscode_available + + root = Path("/tmp/project-root") + + with patch(f"{self.MODULE}.shutil.which", return_value=None), \ + patch(f"{self.MODULE}.Path.cwd", return_value=Path("/tmp/other-cwd")), \ + patch.object(Path, "is_dir", autospec=True) as mock_is_dir: + mock_is_dir.side_effect = lambda path: path == (root / ".vscode") + self.assertTrue(_is_vscode_available(project_root=root)) + def test_vscode_detected_via_code_binary(self): """`code` binary on PATH is sufficient to detect VS Code.""" self.assertTrue(self._run(code_on_path=True, vscode_dir_exists=False)) @@ -232,4 +244,4 @@ def test_vscode_detected_when_both_binary_and_dir_present(self): if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() diff --git a/tests/unit/test_transitive_mcp.py b/tests/unit/test_transitive_mcp.py index 822f35017..17ac67c05 100644 --- a/tests/unit/test_transitive_mcp.py +++ b/tests/unit/test_transitive_mcp.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock, patch +import toml import yaml from apm_cli.integration.mcp_integrator import MCPIntegrator @@ -952,11 +953,12 @@ def test_drift_shows_updated_label( ) assert "updated" in printed_lines + @patch("apm_cli.core.null_logger._rich_success") @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._check_self_defined_servers_needing_installation") @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime") @patch("apm_cli.integration.mcp_integrator._get_console", return_value=None) def test_no_stored_configs_preserves_existing_behavior( - self, _console, mock_install_runtime, mock_check, + self, _console, mock_install_runtime, mock_check, mock_rich_success, ): """Without stored configs (first install), behavior unchanged.""" mock_check.return_value = [] @@ -970,3 +972,197 @@ def test_no_stored_configs_preserves_existing_behavior( assert count == 0 mock_install_runtime.assert_not_called() + mock_rich_success.assert_called_once() + assert "already configured" in mock_rich_success.call_args.args[0] + + +class TestCodexProjectScopedMCP: + """Tests for project-local Codex MCP activation and cleanup.""" + + @patch("apm_cli.integration.mcp_integrator._get_console", return_value=None) + @patch("apm_cli.integration.mcp_integrator._is_vscode_available", return_value=False) + @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime") + @patch("apm_cli.registry.operations.MCPServerOperations") + @patch("apm_cli.factory.ClientFactory.create_client") + @patch("apm_cli.runtime.manager.RuntimeManager") + def test_codex_skipped_when_not_active_project_target( + self, + mock_manager_cls, + mock_create_client, + mock_ops_cls, + mock_install_runtime, + _vscode, + _console, + tmp_path, + ): + """Installed Codex should not receive MCP config unless the project targets Codex.""" + mock_manager = mock_manager_cls.return_value + mock_manager.is_runtime_available.side_effect = lambda runtime: runtime == "codex" + mock_create_client.return_value = MagicMock() + + mock_ops = mock_ops_cls.return_value + mock_ops.validate_servers_exist.return_value = (["ghcr.io/org/new"], []) + mock_ops.check_servers_needing_installation.return_value = ["ghcr.io/org/new"] + mock_ops.batch_fetch_server_info.return_value = {"ghcr.io/org/new": {}} + mock_ops.collect_environment_variables.return_value = {} + mock_ops.collect_runtime_variables.return_value = {} + + count = MCPIntegrator.install( + ["ghcr.io/org/new"], + project_root=tmp_path, + apm_config={}, + ) + + assert count == 0 + mock_install_runtime.assert_not_called() + + @patch("apm_cli.integration.mcp_integrator._get_console", return_value=None) + @patch("apm_cli.integration.mcp_integrator._is_vscode_available", return_value=False) + @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime") + @patch("apm_cli.registry.operations.MCPServerOperations") + @patch("apm_cli.factory.ClientFactory.create_client") + @patch("apm_cli.runtime.manager.RuntimeManager") + def test_codex_installed_when_explicit_project_target( + self, + mock_manager_cls, + mock_create_client, + mock_ops_cls, + mock_install_runtime, + _vscode, + _console, + tmp_path, + ): + """Explicit Codex targeting should install MCP config into the project scope.""" + mock_manager = mock_manager_cls.return_value + mock_manager.is_runtime_available.side_effect = lambda runtime: runtime == "codex" + mock_create_client.return_value = MagicMock() + mock_install_runtime.return_value = True + + mock_ops = mock_ops_cls.return_value + mock_ops.validate_servers_exist.return_value = (["ghcr.io/org/new"], []) + mock_ops.check_servers_needing_installation.return_value = ["ghcr.io/org/new"] + mock_ops.batch_fetch_server_info.return_value = {"ghcr.io/org/new": {}} + mock_ops.collect_environment_variables.return_value = {} + mock_ops.collect_runtime_variables.return_value = {} + + count = MCPIntegrator.install( + ["ghcr.io/org/new"], + project_root=tmp_path, + apm_config={"target": "codex"}, + explicit_target="codex", + ) + + assert count == 1 + mock_install_runtime.assert_called_once() + assert mock_install_runtime.call_args.kwargs["project_root"] == tmp_path + assert mock_install_runtime.call_args.kwargs["user_scope"] is False + + @patch("apm_cli.integration.mcp_integrator._get_console", return_value=None) + @patch("apm_cli.integration.mcp_integrator._is_vscode_available", return_value=False) + @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime") + @patch("apm_cli.registry.operations.MCPServerOperations") + @patch("apm_cli.factory.ClientFactory.create_client") + @patch("apm_cli.runtime.manager.RuntimeManager") + def test_explicit_codex_runtime_still_requires_active_project_target( + self, + mock_manager_cls, + mock_create_client, + mock_ops_cls, + mock_install_runtime, + _vscode, + _console, + tmp_path, + ): + """Explicit runtime selection should not bypass Codex project gating.""" + mock_manager = mock_manager_cls.return_value + mock_manager.is_runtime_available.side_effect = lambda runtime: runtime == "codex" + mock_create_client.return_value = MagicMock() + + mock_ops = mock_ops_cls.return_value + mock_ops.validate_servers_exist.return_value = (["ghcr.io/org/new"], []) + mock_ops.check_servers_needing_installation.return_value = ["ghcr.io/org/new"] + mock_ops.batch_fetch_server_info.return_value = {"ghcr.io/org/new": {}} + mock_ops.collect_environment_variables.return_value = {} + mock_ops.collect_runtime_variables.return_value = {} + + count = MCPIntegrator.install( + ["ghcr.io/org/new"], + runtime="codex", + project_root=tmp_path, + apm_config={}, + ) + + assert count == 0 + mock_install_runtime.assert_not_called() + + @patch("apm_cli.core.null_logger._rich_info") + @patch("apm_cli.integration.mcp_integrator._get_console", return_value=None) + @patch("apm_cli.integration.mcp_integrator._is_vscode_available", return_value=False) + @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime") + @patch("apm_cli.registry.operations.MCPServerOperations") + @patch("apm_cli.factory.ClientFactory.create_client") + @patch("apm_cli.runtime.manager.RuntimeManager") + def test_codex_gating_reports_why_it_was_skipped( + self, + mock_manager_cls, + mock_create_client, + mock_ops_cls, + mock_install_runtime, + _vscode, + _console, + mock_info, + tmp_path, + ): + """Codex gating should tell the user why project MCP config was skipped.""" + mock_manager = mock_manager_cls.return_value + mock_manager.is_runtime_available.side_effect = lambda runtime: runtime == "codex" + mock_create_client.return_value = MagicMock() + + mock_ops = mock_ops_cls.return_value + mock_ops.validate_servers_exist.return_value = (["ghcr.io/org/new"], []) + mock_ops.check_servers_needing_installation.return_value = ["ghcr.io/org/new"] + mock_ops.batch_fetch_server_info.return_value = {"ghcr.io/org/new": {}} + mock_ops.collect_environment_variables.return_value = {} + mock_ops.collect_runtime_variables.return_value = {} + + count = MCPIntegrator.install( + ["ghcr.io/org/new"], + runtime="codex", + project_root=tmp_path, + apm_config={}, + ) + + assert count == 0 + mock_install_runtime.assert_not_called() + mock_info.assert_any_call( + "Codex not an active project target -- skipping MCP config " + "(create .codex/ or set target: codex in apm.yml)", + symbol="info", + ) + + def test_remove_stale_codex_uses_project_config(self, tmp_path): + """Stale cleanup should edit the project .codex/config.toml.""" + codex_dir = tmp_path / ".codex" + codex_dir.mkdir() + config_path = codex_dir / "config.toml" + config_path.write_text( + toml.dumps( + { + "mcp_servers": { + "keep": {"command": "npx"}, + "stale": {"command": "npx"}, + } + } + ), + encoding="utf-8", + ) + + MCPIntegrator.remove_stale( + {"stale"}, + runtime="codex", + project_root=tmp_path, + ) + + data = toml.loads(config_path.read_text(encoding="utf-8")) + assert "keep" in data["mcp_servers"] + assert "stale" not in data["mcp_servers"] diff --git a/tests/unit/test_uninstall_engine_helpers.py b/tests/unit/test_uninstall_engine_helpers.py index c094b393d..b99895be9 100644 --- a/tests/unit/test_uninstall_engine_helpers.py +++ b/tests/unit/test_uninstall_engine_helpers.py @@ -374,7 +374,12 @@ def test_stale_servers_removed(self, tmp_path): modules_dir=tmp_path / "apm_modules", ) - mock_mcp.remove_stale.assert_called_once_with({"stale-server"}, scope=None) + mock_mcp.remove_stale.assert_called_once_with( + {"stale-server"}, + project_root=None, + user_scope=False, + scope=None, + ) mock_mcp.update_lockfile.assert_called_once() def test_non_stale_server_not_removed(self, tmp_path): @@ -423,7 +428,12 @@ def test_scope_passed_to_remove_stale(self, tmp_path): scope="user", ) - mock_mcp.remove_stale.assert_called_once_with({"stale"}, scope="user") + mock_mcp.remove_stale.assert_called_once_with( + {"stale"}, + project_root=None, + user_scope=False, + scope="user", + ) def test_get_mcp_dependencies_exception_handled(self, tmp_path): """Exception from apm_package.get_mcp_dependencies is swallowed."""