mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(mcp): isolate ClaudeRegistrar CLI config env (#1888)
## Description `ClaudeRegistrar` accepts `home_dir` and `config_dir` overrides so isolated callers can keep Claude config reads and file fallback writes away from the real user profile. The CLI path did not carry that resolved config location into the `claude` subprocess, so `claude mcp add` and `claude mcp remove` could still inherit the caller's real Claude environment while the registrar's file paths pointed somewhere else. This changes the CLI-backed register and unregister paths to pass a narrow `CLAUDE_CONFIG_DIR` environment only when constructor overrides request isolation. Normal user sessions keep the ambient subprocess environment, server `-e KEY=VALUE` arguments remain unchanged, and isolated registrars make the Claude CLI see the same config directory as Headroom's file-backed paths. Closes #1861. ## 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 - Added a narrow `ClaudeRegistrar` subprocess-env helper that returns an isolated `CLAUDE_CONFIG_DIR` only when `home_dir` or `config_dir` is supplied. - Passed the isolated env into both `claude mcp add` and `claude mcp remove`. - Kept server env values in `ServerSpec.env` as existing `claude mcp add -e KEY=VALUE` arguments. - Added regression coverage for CLI add and remove with `home_dir`, plus explicit `config_dir` precedence over an ambient `CLAUDE_CONFIG_DIR`. ## Testing - [x] Unit tests pass (`uv run pytest tests/test_mcp_registry/test_claude_registrar.py -q`) - [x] Linting passes (`uv run ruff check headroom/mcp_registry/claude.py tests/test_mcp_registry/test_claude_registrar.py`) - [ ] Type checking passes (`uv run mypy headroom`) - [x] New tests added for new functionality when applicable - [x] Manual testing performed ### Test Output ```text uv run pytest tests/test_mcp_registry/test_claude_registrar.py -q ======================== 26 passed, 1 warning in 0.17s ======================== uv run ruff check headroom/mcp_registry/claude.py tests/test_mcp_registry/test_claude_registrar.py All checks passed! uv run ruff format tests/test_mcp_registry/test_claude_registrar.py --check 1 file already formatted ``` ## Real Behavior Proof - Environment: local test runner with mocked Claude CLI subprocess calls; no real user Claude config touched. - Exact command / steps: Construct `ClaudeRegistrar(claude_cli="/usr/local/bin/claude", home_dir=tmp_path)` and an explicit `config_dir` variant, then exercise `register_server(...)` and `unregister_server(...)` through the existing mocked subprocess path. - Observed result: CLI add and remove calls receive `env["CLAUDE_CONFIG_DIR"]` matching the registrar's resolved config directory when isolation is requested. Existing CLI command shape, server `-e` argument behavior, and file fallback behavior remain intact. - Not tested: live Claude Code CLI file writes; the PR proves Headroom's child-process environment handoff without mutating a real Claude installation. ## 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 ## Additional Notes Documentation and changelog updates are N/A because this fixes `ClaudeRegistrar` override isolation rather than adding a user-facing command or config option. Live Claude CLI config writes are intentionally left out of local validation to avoid touching real user configuration.
This commit is contained in:
parent
9d42ebaa1a
commit
1c947b1103
2 changed files with 48 additions and 7 deletions
|
|
@ -47,6 +47,7 @@ class ClaudeRegistrar(MCPRegistrar):
|
|||
"""
|
||||
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)
|
||||
self._isolated_cli_env = home_dir is not None or config_dir is not None
|
||||
self._modern_config = self._claude_dir / ".claude.json"
|
||||
self._legacy_config = self._claude_dir / "mcp.json"
|
||||
if claude_cli is ...:
|
||||
|
|
@ -96,6 +97,7 @@ class ClaudeRegistrar(MCPRegistrar):
|
|||
[str(self._claude_cli), "mcp", "remove", server_name, "-s", "user"],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
env=self._claude_cli_env(),
|
||||
)
|
||||
if result.returncode == 0:
|
||||
return True
|
||||
|
|
@ -121,6 +123,7 @@ class ClaudeRegistrar(MCPRegistrar):
|
|||
cmd,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
env=self._claude_cli_env(),
|
||||
)
|
||||
if result.returncode == 0:
|
||||
return RegisterResult(RegisterStatus.REGISTERED, "via `claude mcp add` (scope: user)")
|
||||
|
|
@ -187,10 +190,16 @@ class ClaudeRegistrar(MCPRegistrar):
|
|||
return None
|
||||
return _entry_to_spec(server_name, entry)
|
||||
|
||||
# ----------------------------------------------------------------------
|
||||
# Helpers
|
||||
# ----------------------------------------------------------------------
|
||||
|
||||
# ----------------------------------------------------------------------
|
||||
# Helpers
|
||||
# ----------------------------------------------------------------------
|
||||
def _claude_cli_env(self) -> dict[str, str] | None:
|
||||
if not self._isolated_cli_env:
|
||||
return None
|
||||
env = os.environ.copy()
|
||||
env["CLAUDE_CONFIG_DIR"] = str(self._claude_dir)
|
||||
return env
|
||||
|
||||
|
||||
def _resolve_claude_config_dir(
|
||||
|
|
|
|||
|
|
@ -158,8 +158,9 @@ def test_register_via_cli_calls_claude_mcp_add(
|
|||
with patch("subprocess.run", return_value=fake_result) as run_mock:
|
||||
result = reg.register_server(_install_spec(monkeypatch))
|
||||
assert result.status == RegisterStatus.REGISTERED
|
||||
cmds = [call.args[0] for call in run_mock.call_args_list]
|
||||
add_cmd = next(c for c in cmds if "add" in c)
|
||||
add_call = run_mock.call_args
|
||||
assert add_call is not None
|
||||
add_cmd = add_call.args[0]
|
||||
assert add_cmd[:6] == [
|
||||
"/usr/local/bin/claude",
|
||||
"mcp",
|
||||
|
|
@ -173,6 +174,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")
|
||||
|
||||
|
||||
def test_register_via_cli_includes_env(tmp_path: Path) -> None:
|
||||
|
|
@ -186,10 +188,38 @@ def test_register_via_cli_includes_env(tmp_path: Path) -> None:
|
|||
fake_result = subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
|
||||
with patch("subprocess.run", return_value=fake_result) as run_mock:
|
||||
reg.register_server(spec)
|
||||
add_cmd = next(c for c in [call.args[0] for call in run_mock.call_args_list] if "add" in c)
|
||||
add_call = run_mock.call_args
|
||||
assert add_call is not None
|
||||
add_cmd = add_call.args[0]
|
||||
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")
|
||||
|
||||
|
||||
def test_register_via_cli_without_overrides_keeps_ambient_env(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
monkeypatch.setenv("CLAUDE_CONFIG_DIR", "ambient")
|
||||
reg = ClaudeRegistrar(claude_cli="/usr/local/bin/claude")
|
||||
fake_result = subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
|
||||
with patch("subprocess.run", return_value=fake_result) as run_mock:
|
||||
reg.register_server(_spec())
|
||||
assert run_mock.call_args is not None
|
||||
assert run_mock.call_args.kwargs["env"] is None
|
||||
|
||||
|
||||
def test_register_via_cli_prefers_explicit_config_dir_over_ambient_env(
|
||||
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
|
||||
) -> None:
|
||||
config_dir = tmp_path / "explicit-config"
|
||||
monkeypatch.setenv("CLAUDE_CONFIG_DIR", "ambient")
|
||||
reg = ClaudeRegistrar(claude_cli="/usr/local/bin/claude", config_dir=config_dir)
|
||||
fake_result = subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
|
||||
with patch("subprocess.run", return_value=fake_result) as run_mock:
|
||||
reg.register_server(_spec())
|
||||
assert run_mock.call_args is not None
|
||||
assert run_mock.call_args.kwargs["env"]["CLAUDE_CONFIG_DIR"] == str(config_dir)
|
||||
|
||||
|
||||
def test_register_writes_file_when_no_cli(tmp_path: Path) -> None:
|
||||
|
|
@ -333,9 +363,11 @@ def test_unregister_via_cli(tmp_path: Path) -> None:
|
|||
ok = subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
|
||||
with patch("subprocess.run", return_value=ok) as run_mock:
|
||||
assert reg.unregister_server("headroom") is True
|
||||
cmd = run_mock.call_args_list[0].args[0]
|
||||
assert run_mock.call_args is not 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")
|
||||
|
||||
|
||||
def test_unregister_via_file_when_no_cli(tmp_path: Path) -> None:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue