From d0d48bdf6917383d85be8ba07bd5178bdb7e8074 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Thu, 30 Apr 2026 17:16:57 -0400 Subject: [PATCH 1/2] fix: unify MCP and settings pipelines in cache build Two independent pipelines (build_merged_settings and install_mcps) wrote to the same agent config files without coordination, causing status to show false-positive diffs for amp, codex, and gemini. The cache now represents the complete expected file state by merging managed MCPs during build_merged_settings via a _merge_managed_mcps hook. Also fixes: MCP install output missing per-agent labels, amp settings.json formatting mismatch, tool install output appearing before profile header, Claude Code UI settings (skipAutoPermissionPrompt) lost on cache rebuild, and work profile model variant (opus vs opus[1m]). --- src/ai_rules/agents/amp.py | 4 + src/ai_rules/agents/base.py | 46 ++++++++- src/ai_rules/agents/gemini.py | 2 +- src/ai_rules/cli.py | 40 ++++---- src/ai_rules/config/amp/settings.json | 53 +++++++++- src/ai_rules/config/claude/settings.json | 1 + src/ai_rules/config/profiles/work.yaml | 2 +- src/ai_rules/mcp.py | 56 +++++++++-- src/ai_rules/symlinks.py | 17 ++++ src/ai_rules/targets/base.py | 9 ++ tests/unit/test_config.py | 121 +++++++++++++++++++++++ tests/unit/test_mcp.py | 48 +++++++++ tests/unit/test_symlinks.py | 39 ++++++++ 13 files changed, 403 insertions(+), 35 deletions(-) diff --git a/src/ai_rules/agents/amp.py b/src/ai_rules/agents/amp.py index 7be396e..aa964df 100644 --- a/src/ai_rules/agents/amp.py +++ b/src/ai_rules/agents/amp.py @@ -29,6 +29,10 @@ def config_file_name(self) -> str: def config_file_format(self) -> str: return "json" + @property + def preserved_fields(self) -> list[str]: + return ["amp.mcpServers"] + @cached_property def symlinks(self) -> list[tuple[Path, Path]]: """Cached list of all Amp symlinks.""" diff --git a/src/ai_rules/agents/base.py b/src/ai_rules/agents/base.py index a41bf6e..9433fa1 100644 --- a/src/ai_rules/agents/base.py +++ b/src/ai_rules/agents/base.py @@ -3,7 +3,7 @@ from __future__ import annotations from abc import abstractmethod -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from ai_rules.targets.base import ConfigTarget @@ -29,6 +29,50 @@ def get_mcp_manager(self) -> MCPManager | None: """Return the agent-specific MCPManager, or None if MCP is unsupported.""" return None + def _merge_managed_mcps(self, merged: dict[str, Any]) -> None: + """Merge managed MCPs into the settings cache. + + Reconciles managed entries (add/update/remove) while preserving + unmanaged entries that the user added directly. + """ + from ai_rules.mcp import is_managed_value + + mgr = self.get_mcp_manager() + if mgr is None or mgr.mcp_settings_key is None: + return + + mcp_key = mgr.mcp_settings_key + native_mcps = mgr.get_native_mcps(self.config_dir, self.config) + + current = merged.get(mcp_key, {}) + if not isinstance(current, dict): + current = {} + + if mgr.mcp_tracking_key: + tracking = merged.get(mgr.mcp_tracking_key, {}) + tracked = set(tracking.get("names", [])) + else: + tracked = { + n + for n, c in current.items() + if is_managed_value(c.get(mgr._marker_field)) + } + for name in tracked - set(native_mcps.keys()): + current.pop(name, None) + + current.update(native_mcps) + + if current: + merged[mcp_key] = current + else: + merged.pop(mcp_key, None) + + if mgr.mcp_tracking_key: + if native_mcps: + merged[mgr.mcp_tracking_key] = {"names": sorted(native_mcps.keys())} + else: + merged.pop(mgr.mcp_tracking_key, None) + def install_mcps( self, force: bool = False, dry_run: bool = False ) -> tuple[OperationResult, str, list[str]]: diff --git a/src/ai_rules/agents/gemini.py b/src/ai_rules/agents/gemini.py index f032909..15db337 100644 --- a/src/ai_rules/agents/gemini.py +++ b/src/ai_rules/agents/gemini.py @@ -31,7 +31,7 @@ def config_file_format(self) -> str: @property def preserved_fields(self) -> list[str]: - return ["ide"] + return ["ide", "mcpServers"] @cached_property def symlinks(self) -> list[tuple[Path, Path]]: diff --git a/src/ai_rules/cli.py b/src/ai_rules/cli.py index c1765c9..256fadc 100644 --- a/src/ai_rules/cli.py +++ b/src/ai_rules/cli.py @@ -1067,22 +1067,6 @@ def install( console = Console() - sl_source, sl_local_path = get_effective_install_source("statusline") - statusline_result, statusline_message = ensure_statusline_installed( - dry_run=dry_run, - from_github=sl_source == ToolSource.GITHUB, - local_path=sl_local_path, - ) - if statusline_result == "installed": - if dry_run and statusline_message: - console.print(f"[dim]{statusline_message}[/dim]\n") - else: - console.print("[green]✓[/green] Installed claude-statusline\n") - elif statusline_result == "failed": - console.print( - "[yellow]⚠[/yellow] Failed to install claude-statusline (continuing anyway)\n" - ) - if config_dir_override: config_dir = Path(config_dir_override) if not config_dir.exists(): @@ -1123,6 +1107,22 @@ def install( if profile and profile != "default": console.print(f"[dim]Using profile: {profile}[/dim]\n") + sl_source, sl_local_path = get_effective_install_source("statusline") + statusline_result, statusline_message = ensure_statusline_installed( + dry_run=dry_run, + from_github=sl_source == ToolSource.GITHUB, + local_path=sl_local_path, + ) + if statusline_result == "installed": + if dry_run and statusline_message: + console.print(f"[dim]{statusline_message}[/dim]\n") + else: + console.print("[green]✓[/green] Installed claude-statusline\n") + elif statusline_result == "failed": + console.print( + "[yellow]⚠[/yellow] Failed to install claude-statusline (continuing anyway)\n" + ) + all_targets = get_targets(config_dir, config) selected_targets = select_targets(all_targets, agents) @@ -1224,13 +1224,13 @@ def install( console.print("[yellow]Skipped MCP installation[/yellow]") else: result, message, _ = target.install_mcps(force=True, dry_run=dry_run) - console.print(f"[green]✓[/green] {message}") + console.print(f"[green]✓[/green] {target.name}: {message}") elif result == OperationResult.UPDATED: - console.print(f"[green]✓[/green] {message}") + console.print(f"[green]✓[/green] {target.name}: {message}") elif result == OperationResult.ALREADY_INSTALLED: - console.print(f"[dim]○[/dim] {message}") + pass elif result != OperationResult.NOT_FOUND: - console.print(f"[yellow]⚠[/yellow] {message}") + console.print(f"[yellow]⚠[/yellow] {target.name}: {message}") claude_agent = next((a for a in selected_targets if a.target_id == "claude"), None) if claude_agent is not None: diff --git a/src/ai_rules/config/amp/settings.json b/src/ai_rules/config/amp/settings.json index de3d7cf..7801f4f 100644 --- a/src/ai_rules/config/amp/settings.json +++ b/src/ai_rules/config/amp/settings.json @@ -8,10 +8,53 @@ "amp.notifications.enabled": true, "amp.notifications.system.enabled": true, "amp.permissions": [ - {"tool": "Bash", "action": "reject", "matches": {"cmd": ["rm -rf *", "rm -rf*", "rm -r *"]}}, - {"tool": "Bash", "action": "reject", "matches": {"cmd": ["git push --force*", "git push -f *"]}}, - {"tool": "Bash", "action": "reject", "matches": {"cmd": ["git reset --hard*"]}}, - {"tool": "Bash", "action": "reject", "matches": {"cmd": ["git stash drop*"]}}, - {"tool": "Bash", "action": "reject", "matches": {"cmd": ["gh pr merge*"]}} + { + "tool": "Bash", + "action": "reject", + "matches": { + "cmd": [ + "rm -rf *", + "rm -rf*", + "rm -r *" + ] + } + }, + { + "tool": "Bash", + "action": "reject", + "matches": { + "cmd": [ + "git push --force*", + "git push -f *" + ] + } + }, + { + "tool": "Bash", + "action": "reject", + "matches": { + "cmd": [ + "git reset --hard*" + ] + } + }, + { + "tool": "Bash", + "action": "reject", + "matches": { + "cmd": [ + "git stash drop*" + ] + } + }, + { + "tool": "Bash", + "action": "reject", + "matches": { + "cmd": [ + "gh pr merge*" + ] + } + } ] } diff --git a/src/ai_rules/config/claude/settings.json b/src/ai_rules/config/claude/settings.json index 6cac6b9..f898a5d 100644 --- a/src/ai_rules/config/claude/settings.json +++ b/src/ai_rules/config/claude/settings.json @@ -138,5 +138,6 @@ "alwaysThinkingEnabled": true, "showThinkingSummaries": true, "skipDangerousModePermissionPrompt": true, + "skipAutoPermissionPrompt": true, "viewMode": "verbose" } diff --git a/src/ai_rules/config/profiles/work.yaml b/src/ai_rules/config/profiles/work.yaml index 0606f67..f893bb7 100644 --- a/src/ai_rules/config/profiles/work.yaml +++ b/src/ai_rules/config/profiles/work.yaml @@ -11,7 +11,7 @@ settings_overrides: ANTHROPIC_DEFAULT_SONNET_MODEL: "claude-sonnet-4-6[1m]" CLAUDE_CODE_SUBAGENT_MODEL: "claude-sonnet-4-6[1m]" ANTHROPIC_DEFAULT_OPUS_MODEL: "claude-opus-4-6[1m]" - model: opus + model: "opus[1m]" codex: service_tier: "fast" features: diff --git a/src/ai_rules/mcp.py b/src/ai_rules/mcp.py index b7889c1..9e2cfcb 100644 --- a/src/ai_rules/mcp.py +++ b/src/ai_rules/mcp.py @@ -69,6 +69,30 @@ def _write_installed(self, mcps: dict[str, Any]) -> None: def _translate(self, shared_config: dict[str, Any]) -> dict[str, Any]: """Convert shared MCP format to agent-native format.""" + @property + def mcp_settings_key(self) -> str | None: + """Key in the settings dict where MCPs are stored. + + Returns None if this manager stores MCPs in a separate file + (e.g. Claude stores MCPs in ~/.claude.json, not in settings.json). + """ + return None + + @property + def mcp_tracking_key(self) -> str | None: + """Key for the managed-names tracking section, if any.""" + return None + + def get_native_mcps(self, config_dir: Path, config: Config) -> dict[str, Any]: + """Load managed MCPs, translate to native format, and stamp marker.""" + managed = self.load_managed_mcps(config_dir, config) + result: dict[str, Any] = {} + for name, shared_cfg in managed.items(): + native = self._translate(shared_cfg) + native[self._marker_field] = MANAGED_BY_VALUE + result[name] = native + return result + # --- shared logic -------------------------------------------------------- def load_managed_mcps(self, config_dir: Path, config: Config) -> dict[str, Any]: @@ -174,15 +198,10 @@ def install_mcps( dry_run: bool = False, ) -> tuple[OperationResult, str, list[str]]: """Install managed MCPs into the agent's config file.""" - managed_mcps = self.load_managed_mcps(config_dir, config) - - native_mcps: dict[str, Any] = {} - for name, shared_cfg in managed_mcps.items(): - translated = self._translate(shared_cfg) - translated[self._marker_field] = MANAGED_BY_VALUE - native_mcps[name] = translated + native_mcps = self.get_native_mcps(config_dir, config) current_mcps = self._read_installed() + original_mcps = copy.deepcopy(current_mcps) tracked_mcps = { name @@ -212,6 +231,9 @@ def install_mcps( current_mcps.update(native_mcps) + if current_mcps == original_mcps: + return (OperationResult.ALREADY_INSTALLED, "MCPs already up to date", []) + self._write_installed(current_mcps) parts = [] @@ -401,6 +423,10 @@ class GooseMCPManager(MCPManager): def _marker_field(self) -> str: return "_managed_by" + @property + def mcp_settings_key(self) -> str | None: + return "extensions" + @property def _config_path(self) -> Path: return Path.home() / ".config" / "goose" / "config.yaml" @@ -467,6 +493,14 @@ class CodexMCPManager(MCPManager): def _marker_field(self) -> str: return "_ai_agent_rules_managed_entry" + @property + def mcp_settings_key(self) -> str | None: + return "mcp_servers" + + @property + def mcp_tracking_key(self) -> str | None: + return self._MANAGED_SECTION + @property def _config_path(self) -> Path: return Path.home() / ".codex" / "config.toml" @@ -585,6 +619,10 @@ class GeminiMCPManager(MCPManager): def _marker_field(self) -> str: return "_managedBy" + @property + def mcp_settings_key(self) -> str | None: + return "mcpServers" + @property def _config_path(self) -> Path: return Path.home() / ".gemini" / "settings.json" @@ -647,6 +685,10 @@ class AmpMCPManager(MCPManager): def _marker_field(self) -> str: return "_managedBy" + @property + def mcp_settings_key(self) -> str | None: + return "amp.mcpServers" + @property def _config_path(self) -> Path: return Path.home() / ".config" / "amp" / "settings.json" diff --git a/src/ai_rules/symlinks.py b/src/ai_rules/symlinks.py index a38b161..4778941 100644 --- a/src/ai_rules/symlinks.py +++ b/src/ai_rules/symlinks.py @@ -269,6 +269,23 @@ def get_content_diff(actual_path: Path, expected_path: Path) -> str | None: except (OSError, UnicodeDecodeError): return None + if str(actual_path).endswith(".json") and str(expected_path).endswith(".json"): + try: + import json + + actual_parsed = json.loads("".join(actual_lines)) + expected_parsed = json.loads("".join(expected_lines)) + if actual_parsed == expected_parsed: + return None + actual_lines = (json.dumps(actual_parsed, indent=2) + "\n").splitlines( + keepends=True + ) + expected_lines = (json.dumps(expected_parsed, indent=2) + "\n").splitlines( + keepends=True + ) + except (json.JSONDecodeError, ValueError): + pass + diff = difflib.unified_diff( actual_lines, expected_lines, diff --git a/src/ai_rules/targets/base.py b/src/ai_rules/targets/base.py index 1d08d79..b58c82d 100644 --- a/src/ai_rules/targets/base.py +++ b/src/ai_rules/targets/base.py @@ -72,6 +72,13 @@ def symlinks(self) -> list[tuple[Path, Path]]: """ pass + def _merge_managed_mcps(self, merged: dict[str, Any]) -> None: # noqa: B027 + """Hook for subclasses to merge managed MCPs into the settings cache. + + Called during build_merged_settings() after preserved_fields are applied. + Override in Agent to reconcile managed MCP entries. + """ + # --- settings cache methods ------------------------------------------- @property @@ -146,6 +153,8 @@ def build_merged_settings( except CONFIG_PARSE_ERRORS: pass + self._merge_managed_mcps(merged) + if tracker and preserved: for field in preserved: merged_value = merged.get(field) diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index fd10a0b..61e6959 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -390,6 +390,127 @@ def test_build_merged_settings_preserves_enabled_plugins( assert result["model"] == "new" assert result["enabledPlugins"] == {"my-plugin@marketplace": True} + def test_build_merged_settings_includes_managed_mcps(self, tmp_path, monkeypatch): + """build_merged_settings merges managed MCPs into the cache for agents that store MCPs in settings.""" + import json + + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + + config_dir = tmp_path / "config" + amp_dir = config_dir / "amp" + amp_dir.mkdir(parents=True) + (amp_dir / "settings.json").write_text(json.dumps({"amp.showCosts": True})) + + mcps_data = {"test-mcp": {"type": "stdio", "command": "test", "args": []}} + (config_dir / "mcps.json").write_text(json.dumps(mcps_data)) + + from ai_rules.agents.amp import AmpAgent + + config = Config( + mcp_overrides={ + "my-mcp": {"type": "stdio", "command": "my-cmd", "args": ["--flag"]} + } + ) + agent = AmpAgent(config_dir, config) + + cache_path = agent.build_merged_settings(force_rebuild=True) + assert cache_path is not None + + with open(cache_path) as f: + cached = json.load(f) + + assert "amp.mcpServers" in cached + assert "test-mcp" in cached["amp.mcpServers"] + assert "my-mcp" in cached["amp.mcpServers"] + assert cached["amp.mcpServers"]["test-mcp"]["_managedBy"] == "ai-agent-rules" + assert cached["amp.showCosts"] is True + + def test_build_merged_settings_reconciles_stale_mcps(self, tmp_path, monkeypatch): + """build_merged_settings removes stale managed MCPs when config changes.""" + import json + + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + + config_dir = tmp_path / "config" + amp_dir = config_dir / "amp" + amp_dir.mkdir(parents=True) + (amp_dir / "settings.json").write_text(json.dumps({})) + (config_dir / "mcps.json").write_text( + json.dumps({"keep-mcp": {"type": "stdio", "command": "keep", "args": []}}) + ) + + from ai_rules.agents.amp import AmpAgent + + config = Config() + agent = AmpAgent(config_dir, config) + cache_path = agent.build_merged_settings(force_rebuild=True) + assert cache_path is not None + + with open(cache_path) as f: + cached = json.load(f) + cached["amp.mcpServers"]["stale-mcp"] = { + "command": "old", + "args": [], + "_managedBy": "ai-agent-rules", + } + with open(cache_path, "w") as f: + json.dump(cached, f) + + cache_path2 = agent.build_merged_settings(force_rebuild=True) + assert cache_path2 is not None + with open(cache_path2) as f: + result = json.load(f) + + assert "keep-mcp" in result["amp.mcpServers"] + assert "stale-mcp" not in result["amp.mcpServers"] + + def test_build_merged_settings_removes_stale_codex_mcps_via_tracking_key( + self, tmp_path, monkeypatch + ): + """Codex uses a tracking section instead of inline markers. Stale MCPs must still be removed.""" + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + + config_dir = tmp_path / "config" + codex_dir = config_dir / "codex" + codex_dir.mkdir(parents=True) + + import tomli_w + + base_toml = {"model": "o3"} + with open(codex_dir / "config.toml", "wb") as f: + tomli_w.dump(base_toml, f) + + (config_dir / "mcps.json").write_text("{}") + + config = Config() + agent = CodexAgent(config_dir, config) + + cache_path = agent.build_merged_settings(force_rebuild=True) + assert cache_path is not None + + import tomllib + + with open(cache_path, "rb") as f: + cached = tomllib.load(f) + cached["mcp_servers"] = {"stale-mcp": {"command": "old", "args": []}} + cached["_ai_agent_rules_managed"] = {"names": ["stale-mcp"]} + with open(cache_path, "wb") as f: + tomli_w.dump(cached, f) + + cache_path2 = agent.build_merged_settings(force_rebuild=True) + assert cache_path2 is not None + with open(cache_path2, "rb") as f: + result = tomllib.load(f) + + assert "stale-mcp" not in result.get("mcp_servers", {}) + assert result.get("_ai_agent_rules_managed", {}).get("names", []) == [] + def test_get_settings_file_for_symlink_returns_cache_when_exists( self, tmp_path, monkeypatch ): diff --git a/tests/unit/test_mcp.py b/tests/unit/test_mcp.py index 738af88..69f04b2 100644 --- a/tests/unit/test_mcp.py +++ b/tests/unit/test_mcp.py @@ -677,3 +677,51 @@ def test_amp_preserves_non_mcp_keys(mock_home, test_repo): assert data["amp.showCosts"] is True assert data["amp.anthropic.thinking.enabled"] is True assert "mcp-a" in data["amp.mcpServers"] + + +def test_install_mcps_noop_returns_already_installed(manager, mock_home, test_repo): + """install_mcps returns ALREADY_INSTALLED when MCPs haven't changed.""" + config = Config() + result1, _, _ = manager.install_mcps(test_repo, config, force=True) + assert result1 == OperationResult.UPDATED + + result2, message2, _ = manager.install_mcps(test_repo, config, force=True) + assert result2 == OperationResult.ALREADY_INSTALLED + assert "already up to date" in message2 + + +def test_get_native_mcps(manager, test_repo): + """get_native_mcps returns translated MCPs with managed-by marker.""" + config = Config() + native = manager.get_native_mcps(test_repo, config) + + assert "test-mcp" in native + assert native["test-mcp"]["_managedBy"] == "ai-agent-rules" + assert native["test-mcp"]["command"] == "test" + + +def test_mcp_settings_key_claude_is_none(): + """Claude stores MCPs in a separate file, so mcp_settings_key is None.""" + assert ClaudeMCPManager().mcp_settings_key is None + + +def test_mcp_settings_key_amp(): + """Amp stores MCPs in the settings file under amp.mcpServers.""" + assert AmpMCPManager().mcp_settings_key == "amp.mcpServers" + + +def test_mcp_settings_key_gemini(): + """Gemini stores MCPs in the settings file under mcpServers.""" + assert GeminiMCPManager().mcp_settings_key == "mcpServers" + + +def test_mcp_settings_key_codex(): + """Codex stores MCPs in config.toml under mcp_servers.""" + mgr = CodexMCPManager() + assert mgr.mcp_settings_key == "mcp_servers" + assert mgr.mcp_tracking_key == "_ai_agent_rules_managed" + + +def test_mcp_settings_key_goose(): + """Goose stores MCPs in config.yaml under extensions.""" + assert GooseMCPManager().mcp_settings_key == "extensions" diff --git a/tests/unit/test_symlinks.py b/tests/unit/test_symlinks.py index 0326f8f..c8aa175 100644 --- a/tests/unit/test_symlinks.py +++ b/tests/unit/test_symlinks.py @@ -9,6 +9,7 @@ SymlinkResult, check_symlink, create_symlink, + get_content_diff, remove_symlink, ) @@ -325,3 +326,41 @@ def mock_expanduser(self): assert removed == 1 assert deprecated_path.exists() + + +class TestGetContentDiffJsonNormalization: + """Test JSON normalization in get_content_diff.""" + + def test_formatting_only_diff_returns_none(self, tmp_path): + """Semantically identical JSON with different formatting returns None.""" + compact = tmp_path / "compact.json" + compact.write_text('{"a": 1, "b": [1, 2]}') + + expanded = tmp_path / "expanded.json" + expanded.write_text('{\n "a": 1,\n "b": [\n 1,\n 2\n ]\n}\n') + + assert get_content_diff(compact, expanded) is None + + def test_semantic_diff_still_shown(self, tmp_path): + """JSON files with actual content differences produce a diff.""" + file_a = tmp_path / "a.json" + file_a.write_text('{"key": "value1"}') + + file_b = tmp_path / "b.json" + file_b.write_text('{"key": "value2"}') + + result = get_content_diff(file_a, file_b) + assert result is not None + assert "value1" in result + assert "value2" in result + + def test_non_json_files_not_normalized(self, tmp_path): + """Non-JSON files are compared as raw text.""" + file_a = tmp_path / "a.yaml" + file_a.write_text("key: value\n") + + file_b = tmp_path / "b.yaml" + file_b.write_text("key: value\n") + + result = get_content_diff(file_a, file_b) + assert result is not None From 7f06845ab9c2b4c7e737b22bf89446d85156ba12 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Thu, 30 Apr 2026 17:47:47 -0400 Subject: [PATCH 2/2] fix: derive MCP preserved fields dynamically from MCPManager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's mcp_settings_key ("mcp_servers") and mcp_tracking_key ("_ai_agent_rules_managed") weren't in preserved_fields, so get_cache_diff still produced false-positive diffs after the MCP/cache unification — the same class of bug fixed for Amp and Gemini. Rather than manually adding MCP keys to each agent's preserved_fields (which is fragile and caused the Codex omission), adds an _effective_preserved_fields property that dynamically extends the static list with MCP keys from the agent's MCPManager. Also guards _merge_managed_mcps against non-dict MCP entries, and restores dim ALREADY_INSTALLED output with per-agent labels. --- src/ai_rules/agents/amp.py | 6 +--- src/ai_rules/agents/base.py | 13 +++++++- src/ai_rules/agents/claude.py | 2 +- src/ai_rules/agents/codex.py | 2 +- src/ai_rules/agents/gemini.py | 4 +-- src/ai_rules/agents/goose.py | 2 +- src/ai_rules/cli.py | 2 +- src/ai_rules/targets/base.py | 15 +++++++-- src/ai_rules/tools/statusline.py | 2 +- tests/unit/test_config.py | 54 ++++++++++++++++++++++++++++++++ tests/unit/test_mcp.py | 28 +++++++++++++++++ 11 files changed, 114 insertions(+), 16 deletions(-) diff --git a/src/ai_rules/agents/amp.py b/src/ai_rules/agents/amp.py index aa964df..414e95a 100644 --- a/src/ai_rules/agents/amp.py +++ b/src/ai_rules/agents/amp.py @@ -29,10 +29,6 @@ def config_file_name(self) -> str: def config_file_format(self) -> str: return "json" - @property - def preserved_fields(self) -> list[str]: - return ["amp.mcpServers"] - @cached_property def symlinks(self) -> list[tuple[Path, Path]]: """Cached list of all Amp symlinks.""" @@ -48,7 +44,7 @@ def symlinks(self) -> list[tuple[Path, Path]]: config_file = self.config_dir / "amp" / "settings.json" if config_file.exists(): target_file = self.config.get_settings_file_for_symlink( - "amp", config_file, force=bool(self.preserved_fields) + "amp", config_file, force=bool(self._effective_preserved_fields) ) result.append((Path("~/.config/amp/settings.json"), target_file)) diff --git a/src/ai_rules/agents/base.py b/src/ai_rules/agents/base.py index 9433fa1..cf81f84 100644 --- a/src/ai_rules/agents/base.py +++ b/src/ai_rules/agents/base.py @@ -29,6 +29,17 @@ def get_mcp_manager(self) -> MCPManager | None: """Return the agent-specific MCPManager, or None if MCP is unsupported.""" return None + @property + def _effective_preserved_fields(self) -> list[str]: + fields = list(self.preserved_fields) + mgr = self.get_mcp_manager() + if mgr is not None and mgr.mcp_settings_key is not None: + if mgr.mcp_settings_key not in fields: + fields.append(mgr.mcp_settings_key) + if mgr.mcp_tracking_key and mgr.mcp_tracking_key not in fields: + fields.append(mgr.mcp_tracking_key) + return fields + def _merge_managed_mcps(self, merged: dict[str, Any]) -> None: """Merge managed MCPs into the settings cache. @@ -55,7 +66,7 @@ def _merge_managed_mcps(self, merged: dict[str, Any]) -> None: tracked = { n for n, c in current.items() - if is_managed_value(c.get(mgr._marker_field)) + if isinstance(c, dict) and is_managed_value(c.get(mgr._marker_field)) } for name in tracked - set(native_mcps.keys()): current.pop(name, None) diff --git a/src/ai_rules/agents/claude.py b/src/ai_rules/agents/claude.py index c44b82b..a549e10 100644 --- a/src/ai_rules/agents/claude.py +++ b/src/ai_rules/agents/claude.py @@ -51,7 +51,7 @@ def symlinks(self) -> list[tuple[Path, Path]]: settings_file = self.config_dir / "claude" / "settings.json" if settings_file.exists(): target_file = self.config.get_settings_file_for_symlink( - "claude", settings_file, force=bool(self.preserved_fields) + "claude", settings_file, force=bool(self._effective_preserved_fields) ) result.append((Path("~/.claude/settings.json"), target_file)) diff --git a/src/ai_rules/agents/codex.py b/src/ai_rules/agents/codex.py index e213a28..3cdaf4e 100644 --- a/src/ai_rules/agents/codex.py +++ b/src/ai_rules/agents/codex.py @@ -48,7 +48,7 @@ def symlinks(self) -> list[tuple[Path, Path]]: config_file = self.config_dir / "codex" / "config.toml" if config_file.exists(): target_file = self.config.get_settings_file_for_symlink( - "codex", config_file, force=bool(self.preserved_fields) + "codex", config_file, force=bool(self._effective_preserved_fields) ) result.append((Path("~/.codex/config.toml"), target_file)) diff --git a/src/ai_rules/agents/gemini.py b/src/ai_rules/agents/gemini.py index 15db337..809f88c 100644 --- a/src/ai_rules/agents/gemini.py +++ b/src/ai_rules/agents/gemini.py @@ -31,7 +31,7 @@ def config_file_format(self) -> str: @property def preserved_fields(self) -> list[str]: - return ["ide", "mcpServers"] + return ["ide"] @cached_property def symlinks(self) -> list[tuple[Path, Path]]: @@ -48,7 +48,7 @@ def symlinks(self) -> list[tuple[Path, Path]]: config_file = self.config_dir / "gemini" / "settings.json" if config_file.exists(): target_file = self.config.get_settings_file_for_symlink( - "gemini", config_file, force=bool(self.preserved_fields) + "gemini", config_file, force=bool(self._effective_preserved_fields) ) result.append((Path("~/.gemini/settings.json"), target_file)) diff --git a/src/ai_rules/agents/goose.py b/src/ai_rules/agents/goose.py index 7182232..d91e8fd 100644 --- a/src/ai_rules/agents/goose.py +++ b/src/ai_rules/agents/goose.py @@ -48,7 +48,7 @@ def symlinks(self) -> list[tuple[Path, Path]]: config_file = self.config_dir / "goose" / "config.yaml" if config_file.exists(): target_file = self.config.get_settings_file_for_symlink( - "goose", config_file, force=bool(self.preserved_fields) + "goose", config_file, force=bool(self._effective_preserved_fields) ) result.append((Path("~/.config/goose/config.yaml"), target_file)) diff --git a/src/ai_rules/cli.py b/src/ai_rules/cli.py index 256fadc..866954e 100644 --- a/src/ai_rules/cli.py +++ b/src/ai_rules/cli.py @@ -1228,7 +1228,7 @@ def install( elif result == OperationResult.UPDATED: console.print(f"[green]✓[/green] {target.name}: {message}") elif result == OperationResult.ALREADY_INSTALLED: - pass + console.print(f"[dim]○ {target.name}: {message}[/dim]") elif result != OperationResult.NOT_FOUND: console.print(f"[yellow]⚠[/yellow] {target.name}: {message}") diff --git a/src/ai_rules/targets/base.py b/src/ai_rules/targets/base.py index b58c82d..a2adb53 100644 --- a/src/ai_rules/targets/base.py +++ b/src/ai_rules/targets/base.py @@ -53,11 +53,20 @@ def preserved_fields(self) -> list[str]: """ return [] + @property + def _effective_preserved_fields(self) -> list[str]: + """All fields that should be preserved across cache rebuilds. + + Includes static preserved_fields plus any dynamically derived fields. + Override in subclasses (e.g. Agent) to extend with additional keys. + """ + return self.preserved_fields + @property def needs_cache(self) -> bool: """Whether this target needs a cache file (has overrides or preserved fields).""" return self.target_id in self.config.settings_overrides or bool( - self.preserved_fields + self._effective_preserved_fields ) @cached_property @@ -132,7 +141,7 @@ def build_merged_settings( if cache_path: cache_path.parent.mkdir(parents=True, exist_ok=True) - preserved = self.preserved_fields + preserved = self._effective_preserved_fields # JSON targets: use ManagedFieldsTracker for granular cleanup on # profile switch (e.g. removing stale hook entries) @@ -262,7 +271,7 @@ def get_cache_diff(self) -> str | None: current_copy = copy.deepcopy(current_settings) expected_copy = copy.deepcopy(expected_settings) - for field in self.preserved_fields: + for field in self._effective_preserved_fields: current_copy.pop(field, None) expected_copy.pop(field, None) diff --git a/src/ai_rules/tools/statusline.py b/src/ai_rules/tools/statusline.py index e4f8471..165ff39 100644 --- a/src/ai_rules/tools/statusline.py +++ b/src/ai_rules/tools/statusline.py @@ -37,6 +37,6 @@ def symlinks(self) -> list[tuple[Path, Path]]: if not config_file.exists(): return [] target_file = self.config.get_settings_file_for_symlink( - "statusline", config_file, force=bool(self.preserved_fields) + "statusline", config_file, force=bool(self._effective_preserved_fields) ) return [(Path("~/.config/claude-statusline/config.yaml"), target_file)] diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 61e6959..4172878 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -511,6 +511,60 @@ def test_build_merged_settings_removes_stale_codex_mcps_via_tracking_key( assert "stale-mcp" not in result.get("mcp_servers", {}) assert result.get("_ai_agent_rules_managed", {}).get("names", []) == [] + def test_effective_preserved_fields_includes_mcp_keys(self, tmp_path): + """_effective_preserved_fields dynamically includes MCP settings/tracking keys.""" + from ai_rules.agents.amp import AmpAgent + from ai_rules.agents.codex import CodexAgent as CodexAgentLocal + from ai_rules.agents.gemini import GeminiAgent + + config_dir = tmp_path / "config" + config_dir.mkdir() + config = Config() + + amp = AmpAgent(config_dir, config) + assert "amp.mcpServers" in amp._effective_preserved_fields + + gemini = GeminiAgent(config_dir, config) + assert "ide" in gemini._effective_preserved_fields + assert "mcpServers" in gemini._effective_preserved_fields + + codex = CodexAgentLocal(config_dir, config) + assert "projects" in codex._effective_preserved_fields + assert "mcp_servers" in codex._effective_preserved_fields + assert "_ai_agent_rules_managed" in codex._effective_preserved_fields + + claude = ClaudeAgent(config_dir, config) + assert "enabledPlugins" in claude._effective_preserved_fields + assert claude._effective_preserved_fields == claude.preserved_fields + + def test_codex_cache_diff_none_after_build_with_mcps(self, tmp_path, monkeypatch): + """get_cache_diff returns None for Codex after build_merged_settings with MCPs.""" + import json + + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + + config_dir = tmp_path / "config" + codex_dir = config_dir / "codex" + codex_dir.mkdir(parents=True) + + import tomli_w + + with open(codex_dir / "config.toml", "wb") as f: + tomli_w.dump({"model": "o3"}, f) + + mcps_data = {"test-mcp": {"type": "stdio", "command": "test", "args": []}} + (config_dir / "mcps.json").write_text(json.dumps(mcps_data)) + + config = Config() + agent = CodexAgent(config_dir, config) + + cache_path = agent.build_merged_settings(force_rebuild=True) + assert cache_path is not None + + assert agent.get_cache_diff() is None + def test_get_settings_file_for_symlink_returns_cache_when_exists( self, tmp_path, monkeypatch ): diff --git a/tests/unit/test_mcp.py b/tests/unit/test_mcp.py index 69f04b2..98f816a 100644 --- a/tests/unit/test_mcp.py +++ b/tests/unit/test_mcp.py @@ -725,3 +725,31 @@ def test_mcp_settings_key_codex(): def test_mcp_settings_key_goose(): """Goose stores MCPs in config.yaml under extensions.""" assert GooseMCPManager().mcp_settings_key == "extensions" + + +def test_merge_managed_mcps_handles_non_dict_entries(tmp_path, monkeypatch): + """_merge_managed_mcps doesn't crash on malformed non-dict MCP entries.""" + import json + + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + + config_dir = tmp_path / "config" + amp_dir = config_dir / "amp" + amp_dir.mkdir(parents=True) + (amp_dir / "settings.json").write_text(json.dumps({})) + (config_dir / "mcps.json").write_text( + json.dumps({"good-mcp": {"type": "stdio", "command": "test", "args": []}}) + ) + + from ai_rules.agents.amp import AmpAgent + + config = Config() + agent = AmpAgent(config_dir, config) + + merged: dict = {"amp.mcpServers": {"bad-entry": "not-a-dict"}} + agent._merge_managed_mcps(merged) + + assert "good-mcp" in merged["amp.mcpServers"] + assert "bad-entry" in merged["amp.mcpServers"]