feat(codex): add project-scoped MCP and user target support#803
feat(codex): add project-scoped MCP and user target support#803Nickolaus wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates APM's Codex integration to support project-scoped MCP configuration (writing to .codex/config.toml for project installs) and adds Codex participation in the existing user-scope target/scope model.
Changes:
- Make Codex MCP config scope-aware (project
.codex/config.tomlvs user~/.codex/config.toml) and avoid configuring Codex MCP unless Codex is an active project target. - Add scope-aware
project_root/user_scopeplumbing through MCP client creation and MCP install/uninstall flows. - Expand unit/integration tests and update Starlight docs to reflect the new Codex MCP scoping behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/integration/mcp_integrator.py |
Adds scope parameters and filters Codex MCP configuration to active project targets. |
src/apm_cli/adapters/client/base.py |
Introduces project_root/user_scope context and placeholder normalization helper. |
src/apm_cli/adapters/client/codex.py |
Resolves Codex config path by scope and normalizes project placeholders in args. |
src/apm_cli/factory.py |
Passes project_root/user_scope into client adapter construction. |
src/apm_cli/core/safe_installer.py |
Creates adapters with scope context for conflict-aware installs. |
src/apm_cli/core/operations.py |
Threads scope context through configure/install/uninstall helpers. |
src/apm_cli/registry/operations.py |
Checks installed server IDs using scope-aware adapter config resolution. |
src/apm_cli/integration/targets.py |
Marks Codex as partially user-scope supported. |
src/apm_cli/commands/install.py |
Passes scope context into MCP install + stale cleanup. |
src/apm_cli/commands/uninstall/cli.py / engine.py |
Passes scope context into MCP stale cleanup on uninstall. |
src/apm_cli/adapters/client/{vscode,cursor,opencode,copilot}.py |
Switches workspace-root resolution to self.project_root. |
tests/unit/test_transitive_mcp.py |
Adds Codex project-scoped MCP behavior tests (install + stale cleanup). |
tests/unit/test_mcp_client_factory.py |
Validates Codex config-path scoping + placeholder normalization. |
tests/unit/integration/test_scope_*.py |
Updates/extends target resolution and install/uninstall scope tests for Codex. |
tests/unit/integration/test_hook_integrator.py |
Verifies Codex hooks merge target respects scope-resolved root dir. |
docs/src/content/docs/integrations/ide-tool-integration.md |
Documents Codex MCP config paths and project-target gating. |
docs/src/content/docs/reference/cli-commands.md |
Clarifies global vs project Codex configuration behavior. |
Comments suppressed due to low confidence (4)
src/apm_cli/registry/operations.py:36
- The method signature now accepts project_root and user_scope, but the docstring's Args section doesn't document them. Please update the docstring to describe how scope/root affect which config files are inspected when checking installation status.
def check_servers_needing_installation(self, target_runtimes: List[str], server_references: List[str], project_root=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
are already installed by comparing server IDs (UUIDs), not names.
Args:
target_runtimes: List of target runtimes to check
server_references: List of MCP server references (names or IDs)
src/apm_cli/core/operations.py:13
- The function signature now includes project_root and user_scope, but the docstring doesn't document these parameters. Please update the Args section so callers understand which config scope/path will be written.
def configure_client(client_type, config_updates, project_root=None, user_scope=False):
"""Configure an MCP client.
Args:
client_type (str): Type of client to configure.
config_updates (dict): Configuration updates to apply.
src/apm_cli/core/operations.py:40
- The function signature now includes project_root and user_scope, but these parameters are missing from the docstring Args section. Document them so it's clear which project/user config is being targeted during installation.
def install_package(client_type, package_name, version=None, shared_env_vars=None, server_info_cache=None, shared_runtime_vars=None, project_root=None, user_scope=False):
"""Install an MCP package for a specific client type.
Args:
client_type (str): Type of client to configure.
package_name (str): Name of the package to install.
version (str, optional): Version of the package to install.
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.
src/apm_cli/core/operations.py:86
- The function signature now includes project_root and user_scope, but the docstring Args section doesn't mention them. Please document how they affect which config file is edited during uninstall.
def uninstall_package(client_type, package_name, project_root=None, user_scope=False):
"""Uninstall an MCP package.
Args:
client_type (str): Type of client to configure.
package_name (str): Name of the package to uninstall.
1070d52 to
0d0328b
Compare
0d0328b to
8df3621
Compare
8df3621 to
b31b2af
Compare
b31b2af to
84b5115
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/apm_cli/registry/operations.py:116
- _get_installed_server_ids() currently extracts IDs only for runtimes 'copilot', 'codex', and 'vscode'. MCPIntegrator.install() can pass 'cursor' (and potentially other runtimes) into check_servers_needing_installation(), and Cursor uses the same
mcpServersschema as Copilot (includingid). Without a 'cursor' branch here, Cursor will always appear to have zero installed servers, causing unnecessary reinstalls. Consider handling 'cursor' by reading IDs fromconfig.get("mcpServers", {})like the Copilot branch (and similarly address any other MCP-compatible runtimes you intend to support).
)
config = client.get_current_config()
if isinstance(config, dict):
if runtime == 'copilot':
84b5115 to
4863999
Compare
4863999 to
729180a
Compare
729180a to
f5178f5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/apm_cli/registry/operations.py:105
_get_installed_server_ids()now takesproject_rootanduser_scope, but the docstring does not describe what these parameters do (or when to pass them). Please update the docstring so callers understand which scope/path is being inspected when determining installed server IDs.
"""Get all installed server IDs across target runtimes.
Args:
target_runtimes: List of runtimes to check
f5178f5 to
e7c3558
Compare
There was a problem hiding this comment.
Thank you for your efforts towards improving our Codex support @Nickolaus
Consolidated review -- project-scoped Codex MCP
Strong direction. Project-scoped .codex/config.toml is the right evolution
for Codex support, and this PR does the hard plumbing work to thread scope
awareness through the adapter/factory/integrator chain cleanly. The
_get_installed_server_ids() pre-computation is a genuine performance win,
the em-dash to ASCII fix is correct, and the test coverage for scope
resolution is solid. Thank you for the iterations.
Two items need fixing before merge. Both are small.
BLOCKING
B1. TOML parse error silently destroys user config (see inline on codex.py)
In codex.py, get_current_config() catches TomlDecodeError and returns
{}. When update_config() then merges into that empty dict, it writes
back to disk -- silently deleting the user's entire existing config. A single
typo in config.toml + apm install = data loss.
B2. Silent Codex gating -- no output when excluded from targets (see inline on mcp_integrator.py)
When Codex is installed but not in active_targets(), it's silently removed
from target_runtimes. Every other filter path (--exclude, scope filtering,
"no runtimes installed") tells the user what happened. This is the only
silent one. If Codex was the only detected runtime, install() returns 0
with zero output and no way to diagnose why.
SHOULD-FIX (recommended in this PR, small effort)
S1. scope silently overrides user_scope in install() (see inline on
mcp_integrator.py). Two params controlling one behaviour is a maintenance
trap. At minimum, add a comment at the override site explaining the precedence.
S2. Log which Codex config path was resolved (project .codex/config.toml
vs user ~/.codex/config.toml) at verbose level. Currently the user cannot
tell which file was written.
S3. The doc table in ide-tool-integration.md crams two paths into one
cell for Codex. Split into two rows or add a Scope column for readability.
S4. Add a doc note about .gitignore for .codex/: if MCP server configs
use inline credentials instead of env-var references, users should gitignore
the directory.
FOLLOW-UP (separate PRs -- happy to file issues)
- Add regression test ensuring all
MCPClientAdaptersubclasses set
project_root/user_scopeafter construction (inheritance guard). - Move
normalize_project_arg()toCodexClientAdapter(only consumer). - Unify
remove_stale()to use adapter-resolved paths for all runtimes
(currently uses adapter for Codex but hardcoded paths for VS Code/Cursor). - Freeze
project_rootat construction time (avoid dynamicos.getcwd()
fallback in the property). - Path containment validation on
project_rootbeforemkdir(parents=True). - Update skill resource files for Codex scope awareness.
Once B1 and B2 are addressed, happy to approve. The SHOULD-FIX items are
small enough to land in the same push. Thank you for driving Codex support
forward -- this is a big step for the project.
Review produced with input from the APM Expert Review Panel: Python Architect, CLI Logging Expert, DevX UX Expert, Supply Chain Security Expert, with CEO synthesis.
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Inline comments for B1, B2, and S1 above.
There was a problem hiding this comment.
[B1] TOML parse error silently wipes user config
When get_current_config() catches TomlDecodeError and returns {},
update_config() merges the new server into that empty dict and writes it
back -- destroying the user's entire existing config.
Suggested fix: warn and return None on decode failure; have
update_config() bail out (or require --force) when the existing
config could not be parsed:
except toml.TomlDecodeError as exc:
from ..utils.console import _rich_warning
_rich_warning(
f"Could not parse {config_path}: {exc} -- "
"skipping config write to avoid data loss",
symbol="warning",
)
return None| ) | ||
| active = {t.name for t in active_targets(root, config_target)} | ||
| if "codex" not in active: | ||
| target_runtimes = [r for r in target_runtimes if r != "codex"] |
There was a problem hiding this comment.
[B2] Silent Codex gating -- add a log line here
This is the only runtime-filter path with no user output. Every other
filter (--exclude, scope, "no runtimes installed") logs something.
Suggested fix:
if "codex" not in active:
target_runtimes = [r for r in target_runtimes if r != "codex"]
_log.debug("Codex gated out: active_targets=%s", active)
if logger:
logger.progress(
"Codex not an active project target -- skipping MCP config "
"(create .codex/ or set target: codex in apm.yml)"
)| if scope is InstallScope.USER: | ||
| user_scope = True | ||
| elif scope is InstallScope.PROJECT: | ||
| user_scope = False |
There was a problem hiding this comment.
[S1] scope silently overrides user_scope -- document or unify
A caller passing scope=PROJECT, user_scope=True gets silently corrected
to user_scope=False. If both params must coexist, at minimum add a
comment explaining the precedence:
# scope enum takes precedence over the raw user_scope bool to prevent
# callers from accidentally mixing scopes.Longer-term, consider deprecating user_scope in favour of scope as
the single source of truth.
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Inline comments for S2, S3, and S4.
There was a problem hiding this comment.
[S2] Log which config path was resolved
Now that Codex config can go to either .codex/config.toml (project) or
~/.codex/config.toml (user), the success message should include the
resolved path so the user knows where to look. Currently only the server
name is shown.
Suggested addition after a successful write:
_log.debug("Codex config written to %s", config_path)And at verbose level:
print(f"[i] Codex config: {config_path}")|
|
||
| **Runtime targeting**: APM detects which runtimes are installed and configures MCP servers for all of them. Use `--runtime <name>` or `--exclude <name>` to control which clients receive configuration. | ||
| **Runtime targeting**: APM detects which runtimes are installed and configures MCP servers for all of them. Codex MCP is project-scoped during project installs, so it is written to `.codex/config.toml` only when Codex is an active project target. Use `--runtime <name>` or `--exclude <name>` to control which clients receive configuration. | ||
|
|
There was a problem hiding this comment.
[S4] Add .gitignore guidance for .codex/
Project-scoped .codex/config.toml lives inside the repo. If MCP server
configs use inline credentials instead of env-var references, a
git add . will commit them. Consider adding a note here (or in the
getting-started guide):
"Review
.codex/config.tomlbefore committing -- if your MCP servers
use inline credentials instead of env-var references, add.codex/to
.gitignore."
| | 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` User scope: `~/.codex/config.toml` | TOML `mcp_servers` section | |
There was a problem hiding this comment.
[S3] Doc table -- split Codex into two rows for readability
Two paths crammed into one cell is hard to scan. Every other row has a
single path. Suggested:
| Codex CLI (project) | `.codex/config.toml` | TOML `mcp_servers` section |
| Codex CLI (`--global`) | `~/.codex/config.toml` | TOML `mcp_servers` section |
Description
Make Codex MCP config project-scoped for repo installs by writing to
.codex/config.tomlinstead of~/.codex/config.toml, and only configure Codex MCP when Codex is an active project target.Also add Codex user-scope support for installed primitives so agents, hooks, and skills follow the existing target and scope model used by the other supported tools.
Fixes #502
Type of change
Testing