fix(claude): honor CLAUDE_CONFIG_DIR for user MCP config#2096
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Claude Code user-scope MCP config routing so global MCP installs honor CLAUDE_CONFIG_DIR (when set to a non-blank value) and otherwise fall back to ~/.claude.json, aligning APM behavior with the file Claude Code reads (issue #2060).
Changes:
- Update
ClaudeClientAdapteruser-scope config path resolution to consultCLAUDE_CONFIG_DIR(trimmed) before falling back toPath.home() / ".claude.json". - Add a regression test ensuring
get_config_path()respectsCLAUDE_CONFIG_DIRand isolate the default-path tests from the ambient environment. - Update docs and bundled usage guidance to document the Claude MCP destination and fallback behavior.
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/adapters/client/claude.py | Resolve the user-scope Claude MCP config path via CLAUDE_CONFIG_DIR when set. |
| tests/unit/test_claude_mcp.py | Add regression coverage for CLAUDE_CONFIG_DIR path routing and stabilize tests against caller env. |
| docs/src/content/docs/consumer/install-mcp-servers.md | Document Claude global MCP destination honoring CLAUDE_CONFIG_DIR with fallback. |
| docs/src/content/docs/reference/environment-variables.md | Extend CLAUDE_CONFIG_DIR docs to include user-scope MCP config behavior. |
| packages/apm-guide/.apm/skills/apm-usage/dependencies.md | Mirror the Claude MCP destination guidance in the bundled usage skill. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
| config_dir = os.environ.get("CLAUDE_CONFIG_DIR", "").strip() | ||
| if config_dir: | ||
| return Path(config_dir).expanduser() / ".claude.json" |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Clean path override; add integration proof and isolate ambient env. |
| CLI Logging Expert | 0 | 1 | 1 | Surface the resolved destination in diagnostics. |
| DevX UX Expert | 0 | 1 | 1 | Reject ambiguous relative config roots. |
| Supply Chain Security Expert | 0 | 1 | 1 | Normalize the configured root before writing. |
| OSS Growth Hacker | 0 | 0 | 2 | Accurate docs; only minor discoverability polish remains. |
| Doc Writer | 1 | 0 | 0 | The documented install-path promise needs empirical proof. |
| Test Coverage Expert | 0 | 1 | 1 | Unit proof is below the install-pipeline tier floor. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 4 follow-ups
- [Test Coverage Expert] (blocking-severity) Add an integration-with-fixtures test that performs the user-scope write, asserts
$CLAUDE_CONFIG_DIR/.claude.json, and asserts$HOME/.claude.jsonis absent; prove it with mutation-break. - [DevX UX Expert] Reject relative
CLAUDE_CONFIG_DIRvalues with a clear error, matching the sibling Codex adapter. - [Supply Chain Security Expert] Normalize the configured root with
resolve(strict=False)before use. - [CLI Logging Expert] Add a diagnostic breadcrumb containing the resolved user-scope config path.
Architecture
classDiagram
MCPClientAdapter <|-- CopilotClientAdapter
CopilotClientAdapter <|-- ClaudeClientAdapter
ClaudeClientAdapter : +_user_claude_json_path() Path
ClaudeClientAdapter : +_merge_user_mcp(config_updates) bool
ClaudeClientAdapter ..> atomic_write_text : user-scope writes
flowchart TD
A[apm install -g --mcp] --> B[ClaudeClientAdapter user scope]
B --> C{CLAUDE_CONFIG_DIR nonblank}
C -->|yes| D[configured root .claude.json]
C -->|no| E[HOME .claude.json]
D --> F[atomic write 0600]
E --> F
Recommendation
Add the empirical integration test and mutation-break proof, then fold the bounded path-hardening and diagnostic items. Re-run the panel on the resulting SHA.
Full per-persona findings
Python Architect
- [recommended] Missing integration-with-fixtures proof for relocated user-scope MCP writes at
tests/integration/test_claude_mcp_schema_fidelity.py. - [nit] The user-scope integration fixture does not clear ambient
CLAUDE_CONFIG_DIR.
CLI Logging Expert
- [recommended] The new destination choice lacks a resolved-path diagnostic breadcrumb.
- [nit] Direct
_rich_successusage is pre-existing and outside the essential fix.
DevX UX Expert
- [recommended] Relative
CLAUDE_CONFIG_DIRvalues resolve against CWD and should be rejected. - [nit] The Claude table cell is long but remains accurate.
Supply Chain Security Expert
- [recommended] Normalize the configured root consistently with the target resolver.
- [nit] The unit test does not exercise a real write.
OSS Growth Hacker
- [nit] Consider a future footnote pattern if runtime path cells grow further.
- [nit] A reference cross-link could improve discovery, but is not needed for correctness.
Auth Expert -- inactive
No authentication surface is affected by this path-routing fix.
Doc Writer
- [blocking] The documented global Claude MCP destination has no integration-level proof.
Test Coverage Expert
- [recommended] The install-pipeline promise needs integration-with-fixtures evidence.
- [nit] The new unit test passes and correctly proves helper-level path selection.
Performance Expert -- inactive
No package-manager performance surface is affected.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Exercise the real user-scope MCP install path, harden the configured root, and surface its resolved destination in diagnostics. Addresses the review panel follow-ups for PR #2096. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Clear ambient CLAUDE_CONFIG_DIR in schema-fidelity fixtures so default-path integration tests remain hermetic. Addresses the Python architect panel finding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Distinguish blank fallback from relative-path rejection and make the validation error actionable. Addresses the final panel documentation and recovery findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Minimal adapter extension; no architectural concerns remain. |
| CLI Logging Expert | 0 | 0 | 0 | Diagnostic breadcrumb and recovery message are clear. |
| DevX UX Expert | 0 | 0 | 0 | Override, fallback, and rejection behavior are predictable. |
| Supply Chain Security Expert | 0 | 0 | 0 | Normalized path and atomic restricted write preserve safety. |
| OSS Growth Hacker | 0 | 0 | 0 | Documentation stays accurate and scannable. |
| Doc Writer | 0 | 0 | 0 | Code, reference docs, and bundled guidance agree. |
| Test Coverage Expert | 0 | 0 | 0 | Integration and unit evidence defend every changed behavior. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
MCPClientAdapter <|-- CopilotClientAdapter
CopilotClientAdapter <|-- ClaudeClientAdapter
ClaudeClientAdapter : +_user_claude_json_path() Path
ClaudeClientAdapter : +_merge_user_mcp(config_updates) bool
ClaudeClientAdapter ..> PathTraversalError : rejects relative roots
ClaudeClientAdapter ..> atomic_write_text : writes user config
flowchart TD
A[apm install -g --mcp] --> B[Claude user-scope adapter]
B --> C{CLAUDE_CONFIG_DIR nonblank}
C -->|yes| D[expand validate and normalize]
C -->|no| E[HOME .claude.json]
D --> F[configured root .claude.json]
E --> G[atomic write 0600]
F --> G
Recommendation
The scoped behavior is empirically proven, mutation-protected, documented, and green on the final SHA. Ready for maintainer review.
Folded in this run
- (panel) Added real user-scope MCP install coverage for relocated output and absent home fallback -- resolved in
4a5e94b2c777d6c6b40df3e6942143fe3d392b2b. - (panel) Rejected relative
CLAUDE_CONFIG_DIRvalues and normalized accepted paths -- resolved in4a5e94b2c777d6c6b40df3e6942143fe3d392b2b. - (panel) Added a debug breadcrumb for the resolved Claude user config destination -- resolved in
4a5e94b2c777d6c6b40df3e6942143fe3d392b2b. - (panel) Isolated schema-fidelity tests from ambient
CLAUDE_CONFIG_DIR-- resolved in59bd501820db98cd04e99996c64233a87b35b31f. - (panel) Made relative-path recovery guidance actionable -- resolved in
b2926baa41b2d4dd23ece869eb7ec63369c82d27. - (panel) Clarified blank fallback versus relative-path rejection across docs and bundled guidance -- resolved in
b2926baa41b2d4dd23ece869eb7ec63369c82d27. - (panel) Shortened the Claude destination matrix entry while preserving the full contract in prose -- resolved in
b2926baa41b2d4dd23ece869eb7ec63369c82d27.
Regression-trap evidence (mutation-break gate)
tests/integration/test_mcp_targets_gating_e2e.py::TestMCPTargetsGatingE2E::test_user_scope_claude_install_honors_claude_config_dir-- replaced theCLAUDE_CONFIG_DIRlookup withCLAUDE_CONFIG_DIR_MUTANT; the test FAILED because the relocated file was absent; the guard was restored and the test passed.
Lint contract
The canonical ruff pair, YAML I/O guard, file-length guard, portable-relative-path guard, pylint R0801 guard, and auth-signals guard all passed before the final push.
CI
All checks passed on exact SHA b2926baa41b2d4dd23ece869eb7ec63369c82d27: https://github.com/microsoft/apm/actions/runs/29061703296 (0 CI recovery iterations).
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2096 | b2926ba |
ship_now | 2 | 7 | 0 | 2 | green | MERGEABLE | BLOCKED | awaiting required review |
Convergence
2 outer iterations; 2 Copilot rounds with zero inline findings. Final panel verdict: ship_now.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No authentication or credential surface is touched.
Doc Writer
No findings.
Test Coverage Expert
No findings.
Performance Expert -- inactive
No cache, transport, materialization, parallelism, or performance surface is touched.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
fix(claude): honor CLAUDE_CONFIG_DIR for user MCP config
TL;DR
Claude user-scope MCP writes now follow
CLAUDE_CONFIG_DIRwhen it is set and non-blank, falling back to~/.claude.jsonotherwise. This keepsapm install -g --mcpaligned with the file Claude Code actually reads and closes #2060.Note
Project-scope behavior remains unchanged: Claude MCP entries still go to
.mcp.json.Problem (WHY)
ClaudeClientAdapter(user_scope=True)hardcoded~/.claude.json, even when Claude Code was configured to read$CLAUDE_CONFIG_DIR/.claude.json.CLAUDE_CONFIG_DIR, so MCP routing diverged from the target's established behavior.Issue #2060 captures the observed contract: user-scope MCP servers should be written to
$CLAUDE_CONFIG_DIR/.claude.jsonwhen configured, with the existing home-directory fallback preserved.Approach (WHAT)
CLAUDE_CONFIG_DIR.~/.claude.jsonfor unset or whitespace-only values.Implementation (HOW)
src/apm_cli/adapters/client/claude.py_user_claude_json_path()now requires an absolute non-blankCLAUDE_CONFIG_DIR, normalizes it, and logs the resolved write path; all existing read, write, and merge paths continue through this helper.tests/unit/test_claude_mcp.pytests/integration/test_mcp_targets_gating_e2e.pydocs/src/content/docs/consumer/install-mcp-servers.mddocs/src/content/docs/reference/environment-variables.mdpackages/apm-guide/.apm/skills/apm-usage/dependencies.mdDiagrams
Legend: The dashed node is the new environment-aware destination selected before the existing Claude MCP write path runs.
flowchart LR E[CLAUDE_CONFIG_DIR] --> Q{Non-blank value} Q -->|yes| C[Config directory .claude.json] Q -->|no| H[Home directory .claude.json] C --> W[Existing Claude MCP read and write path] H --> W classDef new stroke-dasharray: 5 5; class C new;Trade-offs
Benefits
CLAUDE_CONFIG_DIRproduces exactly one expected user MCP destination:$CLAUDE_CONFIG_DIR/.claude.json.~/.claude.jsondestination.Validation
uv run --extra dev pytest tests/unit/test_claude_mcp.py -q:uv run --extra dev pytest tests/unit/test_claude_mcp.py tests/integration/test_claude_mcp_schema_fidelity.py tests/integration/test_mcp_targets_gating_e2e.py -q:Canonical lint mirror
Mutation-break proof replaced
CLAUDE_CONFIG_DIRwithCLAUDE_CONFIG_DIR_MUTANT;test_user_scope_claude_install_honors_claude_config_dirfailed because the relocated file was absent, then passed after restoration.Scenario Evidence
CLAUDE_CONFIG_DIR, then install Claude MCP servers globally; APM writes that directory's.claude.jsonand not the home fallback.tests/integration/test_mcp_targets_gating_e2e.py::TestMCPTargetsGatingE2E::test_user_scope_claude_install_honors_claude_config_dir(regression-trap for #2060)How to test
CLAUDE_CONFIG_DIRto a scratch directory and instantiateClaudeClientAdapter(user_scope=True);get_config_path()should end in that directory's.claude.json.CLAUDE_CONFIG_DIR; the same call should return~/.claude.json.Closes #2060
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com