diff --git a/bcc.py b/bcc.py index 97dd989..5e2dbbd 100644 --- a/bcc.py +++ b/bcc.py @@ -2010,9 +2010,21 @@ class MainWindow(QMainWindow): return self.full_config = cfg repaired = True + # extract_servers tolerates malformed entries rather than raising (#72), + # but keep it inside the guard: a load failure must leave the previously + # loaded profile intact instead of half-swapping the window's state. + try: + servers = core.extract_servers(self.full_config) + except Exception as exc: # pragma: no cover - defence in depth + QMessageBox.critical( + self, + "Could not read config", + f"{profile.path}\n\nThe server list couldn't be read: {exc}", + ) + return self._loaded_stat = core.config_fingerprint(profile.path) self.current_profile = profile - self.servers = core.extract_servers(self.full_config) + self.servers = servers self.dirty = False self.restart_btn.hide() self._undo_stack.clear() @@ -2335,7 +2347,7 @@ class MainWindow(QMainWindow): entry = self.servers[idx] old_name = entry.name entry.name = self.editor.current_name() - entry.data = self.editor.dump_data() + entry.set_data(self.editor.dump_data()) # The server stays in its section (enable state unchanged), so update # its existing row in place rather than re-rendering. # An edit invalidates any cached "Test all" result -- the server that @@ -2429,7 +2441,7 @@ class MainWindow(QMainWindow): QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, ) if ans == QMessageBox.StandardButton.Yes: - self.servers[existing[name]].data = data + self.servers[existing[name]].set_data(data) return False, True name = core.resolve_name_collision(name, {s.name for s in self.servers}) self.servers.append(core.ServerEntry(name, data, True)) @@ -2549,6 +2561,11 @@ class MainWindow(QMainWindow): except Exception as e: QMessageBox.critical(self, "Reload failed", str(e)) return + # The reload above is the on-disk truth for everything the user + # didn't touch -- but it also wipes BCC-authored keys the user + # changed in this session (named sets), which apply_servers + # doesn't write. Carry them over before saving (#73). + contested = core.carry_owned_keys(self.full_config, fresh) core.apply_servers(fresh, self.servers) try: backup = core.write_config(self.current_profile.path, fresh) @@ -2561,8 +2578,13 @@ class MainWindow(QMainWindow): self.dirty = False self.save_btn.setEnabled(False) bnote = f" · backup: {backup.name}" if backup else " · (new file)" + cnote = ( + f" · kept your {', '.join(contested)} (the file on disk had a different copy)" + if contested + else "" + ) self.status.setText( - f"Merged & saved {self.current_profile.path}{bnote}" + f"Merged & saved {self.current_profile.path}{bnote}{cnote}" f" · Restart {self.current_profile.label} to apply." ) self._offer_restart_button() diff --git a/bcc_core.py b/bcc_core.py index a7735d6..03ab61f 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -15,6 +15,7 @@ from __future__ import annotations import base64 import contextlib +import copy import difflib import functools import glob @@ -49,6 +50,12 @@ DISABLED_KEY = "_disabledMcpServers" # parks the rest under DISABLED_KEY. SETS_KEY = "_bccServerSets" +# Top-level keys BCC itself authors. They live in the client's config file, but +# BCC is their owner, so on a stale-file merge the in-memory copy wins over the +# on-disk one (see `carry_owned_keys`). Any future BCC-authored key belongs +# here -- forgetting to add one is exactly how #73 happened. +BCC_OWNED_KEYS = (SETS_KEY,) + BACKUP_DIRNAME = ".bcc_backups" MAX_BACKUPS = 15 @@ -178,16 +185,63 @@ class Profile: self.path = Path(self.path) +class _NoRaw: + """Sentinel for ServerEntry.raw. + + `None` can't do this job: `{"mcpServers": {"foo": null}}` is legal JSON and + a real malformed-entry case, so None has to mean "the config said null", + not "there was nothing here". + """ + + __slots__ = () + + def __repr__(self) -> str: # keeps ServerEntry reprs readable in test output + return "" + + +NO_RAW = _NoRaw() + + @dataclass class ServerEntry: + """One server definition. + + `data` is always a dict so every consumer can treat it as one. When the + config held something that wasn't a JSON object for this server (a string, + a number, a list -- all legal JSON, all wrong here), `data` is empty and + the original value is preserved verbatim in `raw` so Save round-trips it + instead of silently deleting the user's line. `lint_servers` surfaces it. + `raw` defaults to the NO_RAW sentinel rather than None, because a config + value of literal `null` is itself a malformed entry worth preserving. + + Assigning `data` means the user replaced the definition through the editor, + which retires `raw` -- use `set_data` so that can't be forgotten. + """ + name: str data: dict enabled: bool = True + raw: object = NO_RAW @property def kind(self) -> str: return "remote" if "url" in self.data and "command" not in self.data else "stdio" + @property + def malformed(self) -> bool: + """True when the config value for this server wasn't a JSON object.""" + return self.raw is not NO_RAW + + def set_data(self, data: dict) -> None: + """Replace the definition from the editor, clearing any malformed original.""" + self.data = data + self.raw = NO_RAW + + def config_value(self): + """What to write back to the config: the edited dict, or the untouched + malformed original when the user never edited it.""" + return self.data if self.raw is NO_RAW else self.raw + # --------------------------------------------------------------------------- # # Theming (issue #75) @@ -584,13 +638,30 @@ def repair_config_file(path: str | os.PathLike) -> tuple[dict, list[str], str]: return obj, notes, pretty +def _server_entry(name: str, data, enabled: bool) -> ServerEntry: + """Build a ServerEntry, tolerating a value that isn't a JSON object. + + A hand-edited config can legally hold `{"mcpServers": {"foo": "oops"}}` -- + valid JSON, wrong shape. Calling dict() on that raises, which used to take + the whole load down before the linter ever got a look at it (#72). Keep the + original instead and let the linter report it. + """ + if isinstance(data, dict): + return ServerEntry(name=name, data=dict(data), enabled=enabled) + return ServerEntry(name=name, data={}, enabled=enabled, raw=data) + + def extract_servers(cfg: dict) -> list[ServerEntry]: - """Pull enabled (`mcpServers`) and disabled (`_disabledMcpServers`) servers.""" + """Pull enabled (`mcpServers`) and disabled (`_disabledMcpServers`) servers. + + Never raises on a structurally-odd config -- malformed entries come back as + empty-data entries carrying their original value (see `_server_entry`). + """ out: list[ServerEntry] = [] for name, data in (cfg.get("mcpServers") or {}).items(): - out.append(ServerEntry(name=name, data=dict(data), enabled=True)) + out.append(_server_entry(name, data, True)) for name, data in (cfg.get(DISABLED_KEY) or {}).items(): - out.append(ServerEntry(name=name, data=dict(data), enabled=False)) + out.append(_server_entry(name, data, False)) return out @@ -682,8 +753,8 @@ def apply_servers(cfg: dict, servers: list[ServerEntry]) -> dict: Write the server list back into `cfg` in place, preserving every other key and the position of `mcpServers`. Returns the same dict for convenience. """ - enabled = {s.name: s.data for s in servers if s.enabled} - disabled = {s.name: s.data for s in servers if not s.enabled} + enabled = {s.name: s.config_value() for s in servers if s.enabled} + disabled = {s.name: s.config_value() for s in servers if not s.enabled} cfg["mcpServers"] = enabled # replaces value if key existed; appends otherwise if disabled: @@ -696,6 +767,34 @@ def apply_servers(cfg: dict, servers: list[ServerEntry]) -> dict: # --------------------------------------------------------------------------- # # Write (atomic, with rotating backups) # --------------------------------------------------------------------------- # +def carry_owned_keys(local_cfg: dict, fresh_cfg: dict) -> list[str]: + """Carry BCC-authored top-level keys from `local_cfg` onto `fresh_cfg`. + + Used by the stale-file "Merge & save" path, which reloads the file from + disk and re-applies the user's server edits. That reload used to drop + anything BCC owns but `apply_servers` doesn't write -- named server sets + vanished without a word (#73). BCC owns these keys, so the in-memory copy + wins; mutates `fresh_cfg` in place. + + Returns the keys where the on-disk copy differed and was overwritten, so + the caller can tell the user something was actually contested rather than + merely carried across. + + Deliberately one-directional: a key absent locally is left alone on disk. + We can't tell "user deleted their last set" from "user never had sets and + another machine just added some", and silently deleting someone else's + data is the worse of the two failures. + """ + conflicts: list[str] = [] + for key in BCC_OWNED_KEYS: + if key not in local_cfg: + continue + if key in fresh_cfg and fresh_cfg[key] != local_cfg[key]: + conflicts.append(key) + fresh_cfg[key] = copy.deepcopy(local_cfg[key]) + return conflicts + + def _make_backup(path: Path) -> Path: bdir = path.parent / BACKUP_DIRNAME bdir.mkdir(exist_ok=True) @@ -1512,9 +1611,21 @@ def lint_server(name: str, data: dict) -> list[str]: def lint_servers(servers: list[ServerEntry]) -> list[str]: - """Concatenate lint_server warnings across every entry, in order.""" + """Concatenate lint_server warnings across every entry, in order. + + Entries whose config value wasn't a JSON object at all are reported here + rather than in lint_server, which takes an already-dict `data` (#72). + """ out: list[str] = [] for s in servers: + if s.malformed: + nm = s.name.strip() or "(unnamed)" + out.append( + f"'{nm}': server definition is not an object " + f"(found {type(s.raw).__name__}) -- it is preserved as-is; " + f"edit it to replace it with a proper definition" + ) + continue out.extend(lint_server(s.name, s.data)) return out diff --git a/tests/test_core.py b/tests/test_core.py index 2f62d63..55a1bb2 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2373,6 +2373,146 @@ def test_config_has_unfilled_placeholders_checks_env_too(): assert c.config_has_unfilled_placeholders(cfg) is True +# --------------------------------------------------------------------------- # +# #72 -- a server value that isn't a JSON object must not take the load down +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize("bad", ["not-a-dict", 123, ["a", "b"], None, True, 1.5]) +def test_extract_servers_survives_non_dict_server_value(bad): + entries = c.extract_servers({"mcpServers": {"foo": bad}}) + assert len(entries) == 1 + assert entries[0].name == "foo" + assert entries[0].data == {} + assert entries[0].malformed is True + assert entries[0].raw == bad + + +def test_extract_servers_marks_only_the_bad_entry(): + cfg = {"mcpServers": {"good": {"command": "npx"}, "bad": "oops"}} + by_name = {e.name: e for e in c.extract_servers(cfg)} + assert by_name["good"].malformed is False + assert by_name["good"].data == {"command": "npx"} + assert by_name["bad"].malformed is True + + +def test_extract_servers_handles_malformed_disabled_entry(): + entries = c.extract_servers({c.DISABLED_KEY: {"parked": ["nope"]}}) + assert entries[0].enabled is False + assert entries[0].malformed is True + + +def test_malformed_entry_round_trips_through_save_unchanged(): + """The cardinal rule: never silently delete what the user had on disk.""" + cfg = {"mcpServers": {"good": {"command": "npx"}, "bad": "oops"}} + servers = c.extract_servers(cfg) + out = c.apply_servers(dict(cfg), servers) + assert out["mcpServers"]["bad"] == "oops" + assert out["mcpServers"]["good"] == {"command": "npx"} + + +def test_editing_a_malformed_entry_retires_the_raw_value(): + entry = c.extract_servers({"mcpServers": {"bad": "oops"}})[0] + entry.set_data({"command": "npx"}) + assert entry.malformed is False + assert entry.config_value() == {"command": "npx"} + assert c.apply_servers({}, [entry])["mcpServers"]["bad"] == {"command": "npx"} + + +def test_lint_reports_the_malformed_entry_by_name(): + servers = c.extract_servers({"mcpServers": {"bad": "oops"}}) + warnings = c.lint_servers(servers) + assert len(warnings) == 1 + assert "'bad'" in warnings[0] + assert "not an object" in warnings[0] + assert "str" in warnings[0] + + +def test_lint_still_reports_normal_warnings_alongside_malformed(): + cfg = {"mcpServers": {"bad": "oops", "sloppy": {"command": "npx", "args": "one two"}}} + warnings = c.lint_servers(c.extract_servers(cfg)) + assert any("not an object" in w for w in warnings) + assert any("'args' should be a list" in w for w in warnings) + + +# --------------------------------------------------------------------------- # +# #73 -- the stale-file merge must not discard BCC-authored keys +# --------------------------------------------------------------------------- # +def test_carry_owned_keys_moves_sets_onto_the_reloaded_config(): + local = {"mcpServers": {}, c.SETS_KEY: {"work": ["a", "b"]}} + fresh = {"mcpServers": {"external": {"command": "npx"}}} + contested = c.carry_owned_keys(local, fresh) + assert contested == [] + assert fresh[c.SETS_KEY] == {"work": ["a", "b"]} + assert fresh["mcpServers"] == {"external": {"command": "npx"}} + + +def test_carry_owned_keys_reports_a_genuine_conflict(): + local = {c.SETS_KEY: {"work": ["a"]}} + fresh = {c.SETS_KEY: {"work": ["a", "b"]}} + assert c.carry_owned_keys(local, fresh) == [c.SETS_KEY] + assert fresh[c.SETS_KEY] == {"work": ["a"]} # local wins: BCC owns the key + + +def test_carry_owned_keys_is_quiet_when_both_sides_agree(): + local = {c.SETS_KEY: {"work": ["a"]}} + fresh = {c.SETS_KEY: {"work": ["a"]}} + assert c.carry_owned_keys(local, fresh) == [] + + +def test_carry_owned_keys_leaves_disk_alone_when_absent_locally(): + """Can't distinguish 'deleted my last set' from 'never had sets'; keep theirs.""" + fresh = {c.SETS_KEY: {"remote": ["a"]}} + assert c.carry_owned_keys({}, fresh) == [] + assert fresh[c.SETS_KEY] == {"remote": ["a"]} + + +def test_carry_owned_keys_deep_copies_so_later_edits_do_not_leak(): + local = {c.SETS_KEY: {"work": ["a"]}} + fresh = {} + c.carry_owned_keys(local, fresh) + local[c.SETS_KEY]["work"].append("b") + assert fresh[c.SETS_KEY] == {"work": ["a"]} + + +def test_merge_flow_preserves_sets_and_external_servers(tmp_path): + """End-to-end shape of the Merge & save path that lost sets in #73.""" + path = tmp_path / "claude.json" + path.write_text(json.dumps({"mcpServers": {"old": {"command": "old"}}})) + + # BCC loads, user saves a named set and edits servers in memory. + local = c.load_config(path) + servers = c.extract_servers(local) + c.save_server_set(local, "work", servers) + + # Something else rewrites the file underneath us. + path.write_text(json.dumps({"mcpServers": {"external": {"command": "new"}}, "other": 1})) + + # Merge & save: reload disk, carry BCC keys, re-apply the user's servers. + fresh = c.load_config(path) + c.carry_owned_keys(local, fresh) + c.apply_servers(fresh, servers) + c.write_config(path, fresh) + + saved = c.load_config(path) + assert saved[c.SETS_KEY] == {"work": ["old"]} # the set survived + assert saved["other"] == 1 # unrelated external key preserved + assert "old" in saved["mcpServers"] # user's servers re-applied + + +def test_null_server_value_is_malformed_not_mistaken_for_absent(): + """`{"mcpServers": {"foo": null}}` is legal JSON and a real malformed case, + so None must not double as the 'nothing here' sentinel.""" + entry = c.extract_servers({"mcpServers": {"foo": None}})[0] + assert entry.malformed is True + assert entry.raw is None + assert c.apply_servers({}, [entry])["mcpServers"]["foo"] is None + + +def test_a_normal_entry_is_not_malformed(): + entry = c.extract_servers({"mcpServers": {"foo": {"command": "npx"}}})[0] + assert entry.malformed is False + assert entry.raw is c.NO_RAW + + # --------------------------------------------------------------------------- # # #75 -- theming # --------------------------------------------------------------------------- #