fix(mcp): correct default Claude Code config path in ClaudeRegistrar (#1859)

## Description

<!-- Briefly explain the change and why it is needed. -->

`ClaudeRegistrar` originally assumed Claude Code's modern per-user MCP
config lives at `~/.claude/.claude.json`. On a real Claude Code 2.1.202
install with `CLAUDE_CONFIG_DIR` unset, the actual file is
`~/.claude.json`, directly under the home directory. Whenever `claude
mcp add` failed for any transient reason and the registrar fell back to
writing the config file directly, it wrote to a path Claude Code never
reads — registration reported success with no error, but the server
silently never became available, and once that wrong file existed,
`get_server()` kept reading it back as already-registered, so the
registrar never retried.

While fixing the path, several related correctness and test-isolation
issues in the same file were found and fixed:

- Three tests instantiated `ClaudeRegistrar(claude_cli=None)` without
`home_dir`, so the legacy config path resolved to the developer's real
`~/.claude/mcp.json` — one test was actually deleting a `headroom` entry
from it.
- `detect()` only checked the legacy `~/.claude` directory, so installs
where the `claude` CLI is absent from `PATH` and only the modern
`~/.claude.json` exists were treated as not detected, skipping
registration entirely.
- `unregister_server()` returned early on CLI success without cleaning
the legacy config file, so a stale legacy entry could survive a
successful `claude mcp remove` and `get_server()` would keep reporting
the server as registered.
- `_read_server_entry`, `_remove_from_file`, and `_register_via_file`
assumed `mcpServers` was always a dict once present; a hand-edited or
corrupted config with `mcpServers` set to `null`, a list, or a string
crashed with an unhandled `AttributeError`/`TypeError`.

Closes #

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Code refactoring (no functional changes)

## Changes Made

- `_resolve_claude_config_dir` now defaults to `home` (the modern config
is `~/.claude.json`), keeping the `CLAUDE_CONFIG_DIR` and explicit
`config_dir` overrides intact; the legacy `.claude` directory is pinned
to `home / ".claude"` independently of where the modern config resolves.
- `detect()` also recognizes an install via
`self._modern_config.exists()`, not just the legacy directory.
- `unregister_server()` always cleans both the modern and legacy config
files, even after a successful CLI removal.
- `_read_server_entry`, `_remove_from_file`, and `_register_via_file`
now validate `mcpServers` is a dict before indexing into it, degrading
gracefully instead of crashing on malformed config.
- Module and constructor docstrings now state the current config-path
facts plainly (paths, and what `CLAUDE_CONFIG_DIR` relocates); the
constructor docstring also clarifies that `home_dir`/`config_dir`
isolate file-based reads/writes but not CLI subprocess calls.
- `tests/test_mcp_registry/test_claude_registrar.py`: corrected path
expectations, isolated the three previously-unisolated tests from the
real home directory, and added coverage for the modern-config-only
detect case, CLI-success-with-stale-legacy-entry, and non-dict
`mcpServers` values.
- Filed #1861 for a related, currently-unexercised gap: CLI subprocess
calls don't honor `home_dir`/`config_dir` overrides.

## Testing

- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [x] Manual testing performed

### Test Output

```text
$ pytest tests/test_mcp_registry/test_claude_registrar.py
============================== 35 passed in 0.08s ==============================

$ ruff check headroom/mcp_registry/claude.py tests/test_mcp_registry/test_claude_registrar.py
All checks passed!

$ mypy headroom/mcp_registry/claude.py
Success: no issues found in 1 source file
```

## Real Behavior Proof

- Environment: Claude Code 2.1.202 (macOS, Darwin 25.5.0),
`CLAUDE_CONFIG_DIR` unset.
- Exact command / steps: Injected a uniquely-named probe server into
**only** `~/.claude/.claude.json` (old assumed path) and ran `claude mcp
list`; then injected a different probe into **only** `~/.claude.json`
(corrected path) and ran `claude mcp list` again; restored both files
afterward.
1. `ZZZ_nested_probe` written to `~/.claude/.claude.json` only → `claude
mcp list`.
2. `ZZZ_flat_probe` written to `~/.claude.json` only → `claude mcp
list`.
- Observed result: The nested-path probe (`ZZZ_nested_probe`) was
**not** recognized by `claude mcp list` — Claude Code ignores
`~/.claude/.claude.json`. The flat-path probe (`ZZZ_flat_probe`) **was**
recognized and listed. This confirms `~/.claude.json` is the file Claude
Code actually reads. Both config files were restored to their original
state after the test.
- Not tested: older Claude Code versions (< 2.1.202); Windows/Linux path
resolution (logic is platform-agnostic via `pathlib`, but only macOS was
exercised); the CLI-subprocess env-isolation gap tracked in #1861.

## Review Readiness

- [x] I have performed a self-review
- [x] This PR is ready for human review

## Checklist

- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] I have updated the CHANGELOG.md if applicable

## Screenshots (if applicable)

N/A — CLI/config-path change with no UI surface.

## Additional Notes

- The `CLAUDE_CONFIG_DIR=~/.claude` configuration still correctly
resolves the modern config to `~/.claude/.claude.json` via the unchanged
env override — only the default (env unset) changed.
- Any machine that hit the original bug may have a stale
`~/.claude/.claude.json` written by the old fallback; it is harmless and
can be deleted.
- #1861 tracks a related gap (CLI subprocess env isolation) that isn't
exercised by any current production call site.
This commit is contained in:
Sakamoto, Kazunori 2026-07-10 06:58:20 +09:00 committed by GitHub
parent 33c7f6cd3a
commit c85731dc23
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 104 additions and 38 deletions

View file

@ -1,10 +1,12 @@
"""Claude Code MCP registrar.
Claude Code 2.x stores MCP server configuration in ``~/.claude/.claude.json``
and ships a CLI (``claude mcp add/remove/list/get``) that owns the file.
Claude Code 2.x stores user-scope MCP server configuration in
``~/.claude.json``, directly under the home directory. The ``claude`` CLI
(``claude mcp add/remove/list/get``) owns this file; setting
``CLAUDE_CONFIG_DIR`` relocates it to ``$CLAUDE_CONFIG_DIR/.claude.json``.
Older Claude Code releases (and the Claude Desktop app) read
``~/.claude/mcp.json``. This registrar prefers the CLI for writes when
available, and reads the underlying JSON files directly for compare /
``~/.claude/mcp.json`` instead. This registrar prefers the CLI for writes
when available, and reads the underlying JSON files directly for compare /
``get_server`` so it is robust to CLI output format changes.
"""
@ -42,13 +44,21 @@ class ClaudeRegistrar(MCPRegistrar):
``claude_cli`` defaults to :func:`shutil.which` lookup. Pass
``None`` to force the file-based fallback path. Pass an explicit
path to point at a specific binary. ``CLAUDE_CONFIG_DIR`` is honored
for real user sessions; ``home_dir`` keeps tests isolated from the
caller's environment unless ``config_dir`` is passed explicitly.
for real user sessions; ``home_dir`` isolates file-based reads and
writes from the caller's real home directory, unless ``config_dir``
is passed explicitly. It does not isolate CLI subprocess calls (see
``claude_cli``) pass ``claude_cli=None`` alongside ``home_dir`` to
keep a test fully off the real ``claude`` binary.
"""
home = home_dir if home_dir is not None else Path.home()
self._claude_dir = _resolve_claude_config_dir(home, config_dir, honor_env=home_dir is None)
modern_dir = _resolve_claude_config_dir(home, config_dir, honor_env=home_dir is None)
# Legacy config lives under the real ``.claude`` directory regardless
# of where the modern config resolved to (CLAUDE_CONFIG_DIR only
# relocates the modern file, per Claude Code's own behavior).
self._claude_dir = home / ".claude"
self._modern_dir = modern_dir
self._isolated_cli_env = home_dir is not None or config_dir is not None
self._modern_config = self._claude_dir / ".claude.json"
self._modern_config = modern_dir / ".claude.json"
self._legacy_config = self._claude_dir / "mcp.json"
if claude_cli is ...:
self._claude_cli = shutil.which("claude")
@ -63,7 +73,7 @@ class ClaudeRegistrar(MCPRegistrar):
def detect(self) -> bool:
if self._claude_cli:
return True
return self._claude_dir.is_dir()
return self._claude_dir.is_dir() or self._modern_config.exists()
def get_server(self, server_name: str) -> ServerSpec | None:
# Read from disk regardless of whether the CLI is present — the file
@ -92,6 +102,7 @@ class ClaudeRegistrar(MCPRegistrar):
return self._register_via_file(spec)
def unregister_server(self, server_name: str) -> bool:
removed = False
if self._claude_cli:
result = run(
[str(self._claude_cli), "mcp", "remove", server_name, "-s", "user"],
@ -100,11 +111,11 @@ class ClaudeRegistrar(MCPRegistrar):
env=self._claude_cli_env(),
)
if result.returncode == 0:
return True
logger.debug("claude mcp remove failed: %s", result.stderr.strip())
# Fall through to file-based removal in case CLI didn't know
# about the user-scope entry but the file still has it.
removed = False
removed = True
else:
logger.debug("claude mcp remove failed: %s", result.stderr.strip())
# Always clean up both files too — the CLI only touches the modern
# config, so a legacy entry (or one it didn't know about) can remain.
for config_path in (self._modern_config, self._legacy_config):
removed = self._remove_from_file(config_path, server_name) or removed
return removed
@ -154,7 +165,9 @@ class ClaudeRegistrar(MCPRegistrar):
try:
target.parent.mkdir(parents=True, exist_ok=True)
config = _read_json(target)
servers = config.setdefault("mcpServers", {})
servers = config.get("mcpServers")
if not isinstance(servers, dict):
config["mcpServers"] = servers = {}
servers[spec.name] = _spec_to_entry(spec)
_write_json(target, config)
except OSError as exc:
@ -168,8 +181,8 @@ class ClaudeRegistrar(MCPRegistrar):
config = _read_json(path)
except OSError:
return False
servers = config.get("mcpServers", {})
if server_name not in servers:
servers = config.get("mcpServers")
if not isinstance(servers, dict) or server_name not in servers:
return False
del servers[server_name]
try:
@ -185,7 +198,10 @@ class ClaudeRegistrar(MCPRegistrar):
config = _read_json(path)
except OSError:
return None
entry = config.get("mcpServers", {}).get(server_name)
servers = config.get("mcpServers")
if not isinstance(servers, dict):
return None
entry = servers.get(server_name)
if not isinstance(entry, dict):
return None
return _entry_to_spec(server_name, entry)
@ -198,7 +214,9 @@ class ClaudeRegistrar(MCPRegistrar):
if not self._isolated_cli_env:
return None
env = os.environ.copy()
env["CLAUDE_CONFIG_DIR"] = str(self._claude_dir)
# Point the CLI at the directory holding the modern ``.claude.json``
# (CLAUDE_CONFIG_DIR relocates that file), not the legacy ``.claude`` dir.
env["CLAUDE_CONFIG_DIR"] = str(self._modern_dir)
return env
@ -208,13 +226,19 @@ def _resolve_claude_config_dir(
*,
honor_env: bool,
) -> Path:
"""Resolve the directory holding the *modern* ``.claude.json`` config.
Defaults to ``home`` itself, since Claude Code's modern config lives at
``~/.claude.json`` directly under the home directory. ``CLAUDE_CONFIG_DIR``,
when set, relocates it to ``$CLAUDE_CONFIG_DIR/.claude.json``.
"""
if config_dir is not None:
return config_dir
if honor_env:
env_dir = os.environ.get("CLAUDE_CONFIG_DIR", "").strip()
if env_dir:
return Path(env_dir).expanduser()
return home / ".claude"
return home
def _read_json(path: Path) -> dict[str, Any]:

View file

@ -59,6 +59,12 @@ def test_detect_true_when_only_claude_dir_exists(tmp_path: Path) -> None:
assert reg.detect() is True
def test_detect_true_when_only_modern_config_exists(tmp_path: Path) -> None:
(tmp_path / ".claude.json").write_text("{}")
reg = _make_registrar(tmp_path, cli=None)
assert reg.detect() is True
def test_detect_false_when_neither_present(tmp_path: Path) -> None:
reg = _make_registrar(tmp_path, cli=None)
assert reg.detect() is False
@ -75,8 +81,7 @@ def test_get_server_returns_none_when_unregistered(tmp_path: Path) -> None:
def test_get_server_reads_modern_config(tmp_path: Path) -> None:
cfg = tmp_path / ".claude" / ".claude.json"
cfg.parent.mkdir()
cfg = tmp_path / ".claude.json"
cfg.write_text(
json.dumps(
{
@ -138,6 +143,7 @@ def test_get_server_reads_claude_config_dir(
)
)
monkeypatch.setenv("CLAUDE_CONFIG_DIR", str(tmp_path))
monkeypatch.setattr(Path, "home", lambda: tmp_path)
reg = ClaudeRegistrar(claude_cli=None)
got = reg.get_server("headroom")
assert got is not None
@ -174,7 +180,7 @@ def test_register_via_cli_calls_claude_mcp_add(
_RESOLVED_COMMAND[0],
*_RESOLVED_ARGS,
]
assert add_call.kwargs["env"]["CLAUDE_CONFIG_DIR"] == str(tmp_path / ".claude")
assert add_call.kwargs["env"]["CLAUDE_CONFIG_DIR"] == str(tmp_path)
def test_register_via_cli_includes_env(tmp_path: Path) -> None:
@ -194,7 +200,7 @@ def test_register_via_cli_includes_env(tmp_path: Path) -> None:
assert "-e" in add_cmd
e_idx = add_cmd.index("-e")
assert add_cmd[e_idx + 1] == "HEADROOM_PROXY_URL=http://127.0.0.1:9000"
assert add_call.kwargs["env"]["CLAUDE_CONFIG_DIR"] == str(tmp_path / ".claude")
assert add_call.kwargs["env"]["CLAUDE_CONFIG_DIR"] == str(tmp_path)
def test_register_via_cli_without_overrides_keeps_ambient_env(
@ -226,7 +232,7 @@ def test_register_writes_file_when_no_cli(tmp_path: Path) -> None:
reg = _make_registrar(tmp_path, cli=None)
result = reg.register_server(_spec())
assert result.status == RegisterStatus.REGISTERED
cfg = tmp_path / ".claude" / ".claude.json"
cfg = tmp_path / ".claude.json"
data = json.loads(cfg.read_text())
assert "headroom" in data["mcpServers"]
assert data["mcpServers"]["headroom"]["command"] == _RESOLVED_COMMAND[0]
@ -243,13 +249,14 @@ def test_register_writes_to_legacy_when_only_legacy_exists(tmp_path: Path) -> No
data = json.loads(legacy.read_text())
assert "headroom" in data["mcpServers"]
# Modern config should NOT have been created.
assert not (tmp_path / ".claude" / ".claude.json").exists()
assert not (tmp_path / ".claude.json").exists()
def test_register_writes_to_claude_config_dir(
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
) -> None:
monkeypatch.setenv("CLAUDE_CONFIG_DIR", str(tmp_path))
monkeypatch.setattr(Path, "home", lambda: tmp_path)
reg = ClaudeRegistrar(claude_cli=None)
result = reg.register_server(_spec())
assert result.status == RegisterStatus.REGISTERED
@ -265,8 +272,7 @@ def test_register_writes_to_claude_config_dir(
def test_register_already_when_spec_matches(tmp_path: Path) -> None:
cfg = tmp_path / ".claude" / ".claude.json"
cfg.parent.mkdir()
cfg = tmp_path / ".claude.json"
cfg.write_text(
json.dumps(
{
@ -287,8 +293,7 @@ def test_register_already_when_spec_matches(tmp_path: Path) -> None:
def test_register_mismatch_when_spec_differs_no_force(tmp_path: Path) -> None:
cfg = tmp_path / ".claude" / ".claude.json"
cfg.parent.mkdir()
cfg = tmp_path / ".claude.json"
cfg.write_text(
json.dumps(
{
@ -311,8 +316,7 @@ def test_register_mismatch_when_spec_differs_no_force(tmp_path: Path) -> None:
def test_register_force_overwrites_mismatch(tmp_path: Path) -> None:
cfg = tmp_path / ".claude" / ".claude.json"
cfg.parent.mkdir()
cfg = tmp_path / ".claude.json"
cfg.write_text(
json.dumps(
{
@ -347,7 +351,7 @@ def test_register_cli_failure_falls_back_to_file(tmp_path: Path) -> None:
result = reg.register_server(_spec())
# Even though CLI failed, we wrote the config file as a fallback.
assert result.status == RegisterStatus.REGISTERED
cfg = tmp_path / ".claude" / ".claude.json"
cfg = tmp_path / ".claude.json"
assert cfg.exists()
data = json.loads(cfg.read_text())
assert "headroom" in data["mcpServers"]
@ -367,12 +371,11 @@ def test_unregister_via_cli(tmp_path: Path) -> None:
cmd = run_mock.call_args.args[0]
assert cmd[:5] == ["/usr/local/bin/claude", "mcp", "remove", "headroom", "-s"]
assert cmd[5] == "user"
assert run_mock.call_args.kwargs["env"]["CLAUDE_CONFIG_DIR"] == str(tmp_path / ".claude")
assert run_mock.call_args.kwargs["env"]["CLAUDE_CONFIG_DIR"] == str(tmp_path)
def test_unregister_via_file_when_no_cli(tmp_path: Path) -> None:
cfg = tmp_path / ".claude" / ".claude.json"
cfg.parent.mkdir()
cfg = tmp_path / ".claude.json"
cfg.write_text(
json.dumps(
{
@ -390,6 +393,18 @@ def test_unregister_via_file_when_no_cli(tmp_path: Path) -> None:
assert "other" in data["mcpServers"]
def test_unregister_via_cli_also_removes_stale_legacy_entry(tmp_path: Path) -> None:
legacy = tmp_path / ".claude" / "mcp.json"
legacy.parent.mkdir()
legacy.write_text(json.dumps({"mcpServers": {"headroom": {"command": "old"}}}))
reg = _make_registrar(tmp_path, cli="/usr/local/bin/claude")
ok = subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
with patch("subprocess.run", return_value=ok):
assert reg.unregister_server("headroom") is True
data = json.loads(legacy.read_text())
assert "headroom" not in data["mcpServers"]
def test_unregister_returns_false_when_absent(tmp_path: Path) -> None:
reg = _make_registrar(tmp_path, cli=None)
assert reg.unregister_server("headroom") is False
@ -410,6 +425,7 @@ def test_unregister_removes_from_claude_config_dir(
)
)
monkeypatch.setenv("CLAUDE_CONFIG_DIR", str(tmp_path))
monkeypatch.setattr(Path, "home", lambda: tmp_path)
reg = ClaudeRegistrar(claude_cli=None)
assert reg.unregister_server("headroom") is True
data = json.loads(cfg.read_text())
@ -424,8 +440,34 @@ def test_unregister_removes_from_claude_config_dir(
@pytest.mark.parametrize("contents", ["", "not json", "{", "[]"])
def test_get_server_robust_to_bad_json(tmp_path: Path, contents: str) -> None:
cfg = tmp_path / ".claude" / ".claude.json"
cfg.parent.mkdir()
cfg = tmp_path / ".claude.json"
cfg.write_text(contents)
reg = _make_registrar(tmp_path, cli=None)
assert reg.get_server("headroom") is None
@pytest.mark.parametrize("mcp_servers", ["null", "[]", '"oops"'])
def test_get_server_robust_to_non_dict_mcp_servers(tmp_path: Path, mcp_servers: str) -> None:
cfg = tmp_path / ".claude.json"
cfg.write_text(f'{{"mcpServers": {mcp_servers}}}')
reg = _make_registrar(tmp_path, cli=None)
assert reg.get_server("headroom") is None
@pytest.mark.parametrize("mcp_servers", ["null", "[]", '"oops"'])
def test_unregister_robust_to_non_dict_mcp_servers(tmp_path: Path, mcp_servers: str) -> None:
cfg = tmp_path / ".claude.json"
cfg.write_text(f'{{"mcpServers": {mcp_servers}}}')
reg = _make_registrar(tmp_path, cli=None)
assert reg.unregister_server("headroom") is False
@pytest.mark.parametrize("mcp_servers", ["null", "[]", '"oops"'])
def test_register_robust_to_non_dict_mcp_servers(tmp_path: Path, mcp_servers: str) -> None:
cfg = tmp_path / ".claude.json"
cfg.write_text(f'{{"mcpServers": {mcp_servers}}}')
reg = _make_registrar(tmp_path, cli=None)
result = reg.register_server(_spec())
assert result.status == RegisterStatus.REGISTERED
data = json.loads(cfg.read_text())
assert data["mcpServers"]["headroom"]["command"] == _RESOLVED_COMMAND[0]