diff --git a/headroom/mcp_registry/claude.py b/headroom/mcp_registry/claude.py index 8dc82c846..b27823887 100644 --- a/headroom/mcp_registry/claude.py +++ b/headroom/mcp_registry/claude.py @@ -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]: diff --git a/tests/test_mcp_registry/test_claude_registrar.py b/tests/test_mcp_registry/test_claude_registrar.py index 985aeab47..f64fc87d8 100644 --- a/tests/test_mcp_registry/test_claude_registrar.py +++ b/tests/test_mcp_registry/test_claude_registrar.py @@ -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]