Skip to content

Commit c1c4d35

Browse files
authored
Merge pull request #867 from dsfaccini/fix/mcp-registry-sync-plugin-dir-writes
Keep MCP registry and user-plugin tree in sync with what actually loads
2 parents fbdfe15 + 289f90c commit c1c4d35

12 files changed

Lines changed: 338 additions & 34 deletions

code_puppy/command_line/mcp/trust_command.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,11 +150,14 @@ def _accept(self, config_file, group_id: str) -> None:
150150

151151
def _revoke(self, group_id: str) -> None:
152152
if revoke_project_mcp():
153+
try:
154+
self.manager.sync_from_config()
155+
except Exception as exc: # pragma: no cover - defensive
156+
logger.warning("Post-revoke registry sync failed: %s", exc)
153157
emit_info(
154158
Text.from_markup(
155159
"[green]\u2713 Revoked[/green] trust for this project's MCP "
156-
"config. Its servers will no longer load. Restart or re-sync "
157-
"to drop already-registered ones."
160+
"config. Its servers will no longer load."
158161
),
159162
message_group=group_id,
160163
)

code_puppy/config.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -595,7 +595,7 @@ def _parse_mcp_servers_mapping(raw_text: str) -> dict:
595595
return servers
596596

597597

598-
def load_mcp_server_configs():
598+
def load_mcp_server_configs(*, raise_on_error: bool = False):
599599
"""Load MCP server configs, merging user-level and trusted project-level.
600600
601601
Sources, in ascending order of precedence:
@@ -610,6 +610,11 @@ def load_mcp_server_configs():
610610
Project entries win on name collision, matching how project agents, skills,
611611
and plugins override their user-level counterparts. Returns an empty dict
612612
when nothing is configured.
613+
614+
When *raise_on_error* is true, a parse/IO failure of an existing user-level
615+
file (or a failure of the project loader) is re-raised after the error is
616+
emitted, so callers that unregister missing names can skip that drop
617+
instead of treating ``{}`` as "configure nothing".
613618
"""
614619
from code_puppy.messaging.message_queue import emit_error
615620

@@ -622,6 +627,8 @@ def load_mcp_server_configs():
622627
configs.update(_parse_mcp_servers_mapping(f.read()))
623628
except Exception as e:
624629
emit_error(f"Failed to load MCP servers - {str(e)}")
630+
if raise_on_error:
631+
raise
625632

626633
# 2. Project-level config (opt-in, trust-gated). A broken or untrusted
627634
# project file must never break user-level loading.
@@ -633,6 +640,8 @@ def load_mcp_server_configs():
633640
configs.update(project_configs)
634641
except Exception as e:
635642
emit_error(f"Failed to load project MCP servers - {str(e)}")
643+
if raise_on_error:
644+
raise
636645

637646
return configs
638647

code_puppy/mcp_/manager.py

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,7 @@ def sync_from_config(self) -> None:
161161
try:
162162
from code_puppy.config import load_mcp_server_configs
163163

164-
configs = load_mcp_server_configs()
165-
if not configs:
166-
logger.debug("No servers found in mcp_servers.json")
167-
return
164+
configs = load_mcp_server_configs(raise_on_error=True) or {}
168165

169166
synced_count = 0
170167
updated_count = 0
@@ -209,9 +206,23 @@ def sync_from_config(self) -> None:
209206
logger.warning(f"Failed to sync server '{name}' from config: {e}")
210207
continue
211208

212-
if synced_count > 0 or updated_count > 0:
209+
configured_names = {
210+
name for name, conf in configs.items() if isinstance(conf, dict)
211+
}
212+
dropped_count = 0
213+
for existing in list(self.registry.list_all()):
214+
if existing.name not in configured_names:
215+
if self.remove_server(existing.id):
216+
dropped_count += 1
217+
logger.debug(
218+
"Dropped server no longer in mcp_servers.json: %s",
219+
existing.name,
220+
)
221+
222+
if synced_count > 0 or updated_count > 0 or dropped_count > 0:
213223
logger.info(
214-
f"Synced {synced_count} new and updated {updated_count} servers from mcp_servers.json"
224+
f"Synced {synced_count} new, updated {updated_count}, "
225+
f"dropped {dropped_count} servers from mcp_servers.json"
215226
)
216227

217228
except Exception as e:

code_puppy/plugins/__init__.py

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,18 @@ def _install_project_plugin_finder() -> None:
117117

118118
PLUGIN_ENTRY_POINT_GROUP = "code_puppy.plugins"
119119

120+
# shell_safety implements the safety_permission_level threshold. Skip it only
121+
# when the user opted into high/critical autonomy; the default (medium) must
122+
# load it or the setting is a no-op.
123+
_SHELL_SAFETY_SKIP_LEVELS = frozenset({"high", "critical"})
124+
125+
126+
def _skip_shell_safety_plugin() -> bool:
127+
from code_puppy.config import get_safety_permission_level
128+
129+
return get_safety_permission_level() in _SHELL_SAFETY_SKIP_LEVELS
130+
131+
120132
# Track if plugins have already been loaded to prevent duplicate registration
121133
_PLUGINS_LOADED = False
122134

@@ -135,18 +147,13 @@ def _load_installed_plugins() -> list[str]:
135147
project plugins, but remain physically independent from the core package.
136148
Entry points are sorted for deterministic startup and test behavior.
137149
"""
138-
from code_puppy.config import get_safety_permission_level
139-
140150
loaded: list[str] = []
141151
discovered = sorted(
142152
entry_points(group=PLUGIN_ENTRY_POINT_GROUP), key=lambda item: item.name
143153
)
144154
for entry_point in discovered:
145155
plugin_name = entry_point.name
146-
if plugin_name == "shell_safety" and get_safety_permission_level() not in (
147-
"none",
148-
"low",
149-
):
156+
if plugin_name == "shell_safety" and _skip_shell_safety_plugin():
150157
logger.debug("Skipping shell_safety plugin due to safety permission level")
151158
continue
152159
try:
@@ -191,14 +198,12 @@ def _load_builtin_plugins(
191198
continue
192199

193200
if callbacks_file.exists():
194-
# Skip shell_safety plugin unless safety_permission_level is "low" or "none"
195-
if plugin_name == "shell_safety":
196-
safety_level = get_safety_permission_level()
197-
if safety_level not in ("none", "low"):
198-
logger.debug(
199-
f"Skipping shell_safety plugin - safety_permission_level is '{safety_level}' (needs 'low' or 'none')"
200-
)
201-
continue
201+
if plugin_name == "shell_safety" and _skip_shell_safety_plugin():
202+
logger.debug(
203+
"Skipping shell_safety plugin - safety_permission_level is %s",
204+
get_safety_permission_level(),
205+
)
206+
continue
202207

203208
try:
204209
module_name = f"code_puppy.plugins.{plugin_name}.register_callbacks"

code_puppy/session_surrogate_unpickler.py

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,9 @@ def _tz_from_utc_offset(seconds: float) -> timezone:
7272
("pydantic_core", "TzInfo"): _tz_from_utc_offset,
7373
}
7474

75-
# Genuine timezone libraries: unpickle their classes for real when the
76-
# library is installed; otherwise fall back to surrogates as usual.
77-
_TZ_LIBRARY_PREFIXES = ("pytz", "dateutil.tz")
75+
# pytz / dateutil.tz must NOT go through super().find_class: those packages
76+
# re-export os/sys the same way uuid and collections did. Known tzinfo
77+
# shapes are rebuilt via _TZINFO_EQUIVALENTS; everything else is a surrogate.
7878

7979

8080
class SurrogateBase:
@@ -145,14 +145,6 @@ def find_class(self, module: str, name: str) -> Any: # noqa: D102
145145
equivalent = _TZINFO_EQUIVALENTS.get((module, name))
146146
if equivalent is not None:
147147
return equivalent
148-
if any(
149-
module == prefix or module.startswith(prefix + ".")
150-
for prefix in _TZ_LIBRARY_PREFIXES
151-
):
152-
try:
153-
return super().find_class(module, name)
154-
except Exception: # noqa: BLE001 - library absent/renamed
155-
pass
156148
return self._surrogate_for(module, name)
157149

158150
def _tz_tolerant(self, obj: Any) -> Any:

code_puppy/tools/file_modifications.py

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import json
1515
import os
1616
import traceback
17+
from pathlib import Path
1718
from code_puppy.undo_manager import UndoManager
1819
import warnings
1920
from typing import Annotated, Any, Dict, List, Union
@@ -45,6 +46,75 @@
4546
)
4647

4748

49+
def _split_existing(path: Path) -> tuple[Path, tuple[str, ...]]:
50+
"""Deepest existing ancestor of *path*, plus the missing trailing names."""
51+
missing: list[str] = []
52+
current = path
53+
while True:
54+
if current.exists():
55+
return current.resolve(), tuple(reversed(missing))
56+
parent = current.parent
57+
if parent == current:
58+
return current, tuple(reversed(missing))
59+
missing.append(current.name)
60+
current = parent
61+
62+
63+
def _casefold_has_prefix(parts: tuple[str, ...], prefix: tuple[str, ...]) -> bool:
64+
if len(parts) < len(prefix):
65+
return False
66+
return all(a.casefold() == b.casefold() for a, b in zip(prefix, parts))
67+
68+
69+
def _is_inside_user_plugin_root(target: Path, root: Path) -> bool:
70+
"""Containment that survives APFS case-folding. ``Path.resolve`` does not."""
71+
target_existing, target_rest = _split_existing(target)
72+
root_existing, root_rest = _split_existing(root)
73+
74+
if os.path.samefile(target_existing, root_existing):
75+
return _casefold_has_prefix(target_rest, root_rest)
76+
77+
current = target_existing
78+
while True:
79+
if os.path.samefile(current, root_existing):
80+
return not root_rest
81+
parent = current.parent
82+
if parent == current:
83+
return False
84+
current = parent
85+
86+
87+
def _is_user_plugin_tree_path(file_path: str) -> bool:
88+
"""True if *file_path* is inside ``~/.code_puppy/plugins``.
89+
90+
That tree is imported at the next process start with no trust ceremony.
91+
File tools must not plant ``register_callbacks.py`` there.
92+
Canonicalization errors fail closed (treated as inside).
93+
"""
94+
from code_puppy.plugins import USER_PLUGINS_DIR
95+
96+
try:
97+
resolved = Path(resolve_path(file_path)).resolve()
98+
root = Path(USER_PLUGINS_DIR).expanduser().resolve()
99+
return _is_inside_user_plugin_root(resolved, root)
100+
except (OSError, RuntimeError, ValueError):
101+
return True
102+
103+
104+
def _refuse_user_plugin_tree(file_path: str) -> Dict[str, Any] | None:
105+
if not _is_user_plugin_tree_path(file_path):
106+
return None
107+
return {
108+
"success": False,
109+
"path": file_path,
110+
"message": (
111+
"Refused: file tools cannot modify ~/.code_puppy/plugins. "
112+
"That directory is imported at startup."
113+
),
114+
"changed": False,
115+
}
116+
117+
48118
def _permission_denied(permission_results: List[Any]) -> bool:
49119
"""Return True when any permission callback explicitly denies.
50120
@@ -424,6 +494,9 @@ def _write_to_file(
424494
def delete_snippet_from_file(
425495
context: RunContext, file_path: str, snippet: str, message_group: str | None = None
426496
) -> Dict[str, Any]:
497+
refused = _refuse_user_plugin_tree(file_path)
498+
if refused is not None:
499+
return refused
427500
# Use the plugin system for permission handling with operation data
428501
from code_puppy.callbacks import on_file_permission
429502

@@ -452,6 +525,9 @@ def write_to_file(
452525
overwrite: bool,
453526
message_group: str | None = None,
454527
) -> Dict[str, Any]:
528+
refused = _refuse_user_plugin_tree(path)
529+
if refused is not None:
530+
return refused
455531
# Use the plugin system for permission handling with operation data
456532
from code_puppy.callbacks import on_file_permission
457533

@@ -481,6 +557,9 @@ def replace_in_file(
481557
replacements: List[Dict[str, str]],
482558
message_group: str | None = None,
483559
) -> Dict[str, Any]:
560+
refused = _refuse_user_plugin_tree(path)
561+
if refused is not None:
562+
return refused
484563
# Use the plugin system for permission handling with operation data
485564
from code_puppy.callbacks import on_file_permission
486565

@@ -504,6 +583,9 @@ async def delete_snippet_from_file_async(
504583
context: RunContext, file_path: str, snippet: str, message_group: str | None = None
505584
) -> Dict[str, Any]:
506585
"""Async permission-aware variant of ``delete_snippet_from_file``."""
586+
refused = _refuse_user_plugin_tree(file_path)
587+
if refused is not None:
588+
return refused
507589
from code_puppy.callbacks import on_file_permission_async
508590

509591
operation_data = {"snippet": snippet}
@@ -530,6 +612,9 @@ async def write_to_file_async(
530612
message_group: str | None = None,
531613
) -> Dict[str, Any]:
532614
"""Async permission-aware variant of ``write_to_file``."""
615+
refused = _refuse_user_plugin_tree(path)
616+
if refused is not None:
617+
return refused
533618
from code_puppy.callbacks import on_file_permission_async
534619

535620
operation_data = {"content": content, "overwrite": overwrite}
@@ -556,6 +641,9 @@ async def replace_in_file_async(
556641
message_group: str | None = None,
557642
) -> Dict[str, Any]:
558643
"""Async permission-aware variant of ``replace_in_file``."""
644+
refused = _refuse_user_plugin_tree(path)
645+
if refused is not None:
646+
return refused
559647
from code_puppy.callbacks import on_file_permission_async
560648

561649
operation_data = {"replacements": replacements}
@@ -726,6 +814,9 @@ async def _edit_file_async(
726814
def _delete_file(
727815
context: RunContext, file_path: str, message_group: str | None = None
728816
) -> Dict[str, Any]:
817+
refused = _refuse_user_plugin_tree(file_path)
818+
if refused is not None:
819+
return refused
729820
UndoManager().record_change(file_path, "delete_file")
730821
file_path = resolve_path(file_path)
731822

@@ -786,6 +877,9 @@ async def _delete_file_async(
786877
context: RunContext, file_path: str, message_group: str | None = None
787878
) -> Dict[str, Any]:
788879
"""Async permission-aware variant of ``_delete_file``."""
880+
refused = _refuse_user_plugin_tree(file_path)
881+
if refused is not None:
882+
return refused
789883
file_path = resolve_path(file_path)
790884

791885
from code_puppy.callbacks import on_file_permission_async

tests/mcp/test_manager_extended.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,11 +646,21 @@ def test_initialization_loads_existing_servers(self):
646646
with patch("code_puppy.mcp_.manager.ServerRegistry") as mock_registry_class:
647647
mock_registry = Mock()
648648
mock_registry.list_all.return_value = configs
649+
mock_registry.get_by_name.side_effect = lambda name: next(
650+
(c for c in configs if c.name == name), None
651+
)
649652
mock_registry_class.return_value = mock_registry
650653

651654
with (
652655
patch("code_puppy.mcp_.manager.ManagedMCPServer") as mock_managed_class,
653656
patch("code_puppy.mcp_.manager.ServerStatusTracker"),
657+
patch(
658+
"code_puppy.config.load_mcp_server_configs",
659+
return_value={
660+
"server1": {"type": "stdio", "command": "echo"},
661+
"server2": {"type": "sse", "url": "http://localhost:8080"},
662+
},
663+
),
654664
):
655665
manager = MCPManager()
656666

@@ -687,6 +697,9 @@ def test_initialization_handles_server_creation_failures(self):
687697
with patch("code_puppy.mcp_.manager.ServerRegistry") as mock_registry_class:
688698
mock_registry = Mock()
689699
mock_registry.list_all.return_value = configs
700+
mock_registry.get_by_name.side_effect = lambda name: next(
701+
(c for c in configs if c.name == name), None
702+
)
690703
mock_registry_class.return_value = mock_registry
691704

692705
# Make second server creation fail
@@ -702,6 +715,13 @@ def side_effect(config):
702715
patch(
703716
"code_puppy.mcp_.manager.ServerStatusTracker"
704717
) as mock_tracker_class,
718+
patch(
719+
"code_puppy.config.load_mcp_server_configs",
720+
return_value={
721+
"good-server": {"type": "stdio", "command": "echo"},
722+
"bad-server": {"type": "stdio", "command": "bad"},
723+
},
724+
),
705725
):
706726
mock_tracker = Mock()
707727
mock_tracker_class.return_value = mock_tracker

0 commit comments

Comments
 (0)