Merge PR #77: tolerate non-object server values; keep named sets across a merge (#72, #73)

Two silent-failure bugs found auditing the v1.3.0 features.

#72 -- extract_servers called dict() on every server value, so a config
that was valid JSON but held a non-object server ("foo": "oops", a
number, a list, null) raised on load. The strict parse succeeded, so the
repair path never saw it, and the call sat outside the load try/except:
an unhandled traceback with the window half-swapped to the new profile.
It also made the #54 schema lint unreachable for the most likely
hand-edit mistake -- the load died before the linter ran.

Malformed values are now preserved verbatim on ServerEntry.raw (behind a
NO_RAW sentinel, since a literal JSON null is itself a malformed entry
worth keeping) and written back untouched on Save, so nothing is silently
deleted. lint_servers names the offending entry instead.

#73 -- the stale-file "Merge & save" path reloaded the file from disk and
re-applied the user's servers, but apply_servers only writes mcpServers
and _disabledMcpServers. Named server sets live under _bccServerSets in
the same file, so a set saved that session was dropped from disk and then
from memory, with no warning, on the path the user picks because it
sounds like the safe one.

BCC-owned keys are now declared in BCC_OWNED_KEYS and carried across by
carry_owned_keys, which reports genuinely contested keys so the status
line can say so. Deliberately one-directional: a key absent locally is
left alone on disk, because 'user deleted their last set' and 'another
machine just added sets' are indistinguishable and deleting someone
else's data is the worse failure.

Closes #72
Closes #73

bcc.py

@@ -1939,9 +1939,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()
@@ -2264,7 +2276,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
@@ -2358,7 +2370,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))
@@ -2478,6 +2490,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)
@@ -2490,8 +2507,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()

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>"
+
+
+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
+
 
 # --------------------------------------------------------------------------- #
 # Discovery
@@ -452,13 +506,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
 
 
@@ -550,8 +621,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:
@@ -564,6 +635,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)
@@ -1380,9 +1479,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
 

tests/test_core.py

@@ -2370,3 +2370,143 @@ def test_config_has_unfilled_placeholders_false_after_fill():
 def test_config_has_unfilled_placeholders_checks_env_too():
     cfg = {"command": "uvx", "args": ["mcp-grafana"], "env": {"GRAFANA_URL": "<GRAFANA_URL>"}}
     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