Merge pull request 'feat: stale-file protection — detect concurrent edits on save' (#15) from feat/4-stale-file-protection into main

Merge feat/4-stale-file-protection: stale-file protection (closes #4)

bcc.py

@@ -926,6 +926,80 @@ class RestoreDialog(QDialog):
         return item.data(Qt.ItemDataRole.UserRole)
 
 
+# --------------------------------------------------------------------------- #
+# Stale-file conflict dialog
+# Shown when the config file was modified on disk between load and save.
+# --------------------------------------------------------------------------- #
+class StaleDialog(QDialog):
+    MERGE = 0
+    OVERWRITE = 1
+
+    def __init__(
+        self,
+        parent,
+        profile_path: str,
+        changed_keys: list[str],
+        server_diff: str,
+    ):
+        super().__init__(parent)
+        self.setWindowTitle("Config changed on disk")
+        self.resize(680, 420)
+        self._choice = self.MERGE
+
+        v = QVBoxLayout(self)
+        v.setSpacing(8)
+
+        intro = QLabel(f"<b>{profile_path}</b> was modified on disk since you loaded it.")
+        intro.setWordWrap(True)
+        v.addWidget(intro)
+
+        if changed_keys:
+            kl = QLabel("Changed keys: " + ", ".join(f"<code>{k}</code>" for k in changed_keys))
+            kl.setObjectName("muted")
+            kl.setWordWrap(True)
+            v.addWidget(kl)
+
+        if server_diff:
+            v.addWidget(QLabel("Server section changes (on disk vs loaded):"))
+            dv = QPlainTextEdit()
+            dv.setObjectName("diag")
+            dv.setReadOnly(True)
+            dv.setPlainText(server_diff)
+            v.addWidget(dv, 1)
+        else:
+            note = QLabel("(Server sections unchanged — the external edit is in other keys.)")
+            note.setObjectName("muted")
+            v.addWidget(note)
+
+        desc = QLabel(
+            "<b>Merge &amp; save</b> applies your server edits on top of the current file "
+            "on disk — both sets of changes are kept.  "
+            "<b>Overwrite</b> saves your in-memory state, discarding external changes."
+        )
+        desc.setObjectName("muted")
+        desc.setWordWrap(True)
+        v.addWidget(desc)
+
+        btns = QDialogButtonBox()
+        merge_btn = btns.addButton("Merge && save", QDialogButtonBox.ButtonRole.AcceptRole)
+        merge_btn.setObjectName("primary")
+        overwrite_btn = btns.addButton(
+            "Overwrite anyway", QDialogButtonBox.ButtonRole.DestructiveRole
+        )
+        btns.addButton(QDialogButtonBox.StandardButton.Cancel)
+        merge_btn.clicked.connect(lambda: self._choose(self.MERGE))
+        overwrite_btn.clicked.connect(lambda: self._choose(self.OVERWRITE))
+        btns.rejected.connect(self.reject)
+        v.addWidget(btns)
+
+    def _choose(self, choice: int) -> None:
+        self._choice = choice
+        self.accept()
+
+    def choice(self) -> int:
+        return self._choice
+
+
 # --------------------------------------------------------------------------- #
 # Paste-JSON dialog
 # --------------------------------------------------------------------------- #
@@ -1016,6 +1090,7 @@ class MainWindow(QMainWindow):
         self.full_config: dict = {}
         self.servers: list[core.ServerEntry] = []
         self.current_profile: core.Profile | None = None
+        self._loaded_mtime: float | None = None
         self.dirty = False
         self._suppress_table = False
         self._suppress_sel = False
@@ -1291,6 +1366,7 @@ class MainWindow(QMainWindow):
                 return
             self.full_config = cfg
             repaired = True
+        self._loaded_mtime = core.config_mtime(profile.path)
         self.current_profile = profile
         self.servers = core.extract_servers(self.full_config)
         self.dirty = False
@@ -1627,12 +1703,52 @@ class MainWindow(QMainWindow):
         if not self._validate():
             QMessageBox.warning(self, "Can't save yet", "Fix the highlighted problem first.")
             return
+
+        # Stale-file check: if the file changed on disk since we loaded it, prompt.
+        disk_mtime = core.config_mtime(self.current_profile.path)
+        if (
+            disk_mtime is not None
+            and self._loaded_mtime is not None
+            and disk_mtime != self._loaded_mtime
+        ):
+            changed_keys, server_diff = core.external_change_summary(
+                self.full_config, self.current_profile.path
+            )
+            dlg = StaleDialog(self, str(self.current_profile.path), changed_keys, server_diff)
+            if not dlg.exec():
+                return  # user cancelled
+            if dlg.choice() == StaleDialog.MERGE:
+                try:
+                    fresh = core.load_config(self.current_profile.path)
+                except Exception as e:
+                    QMessageBox.critical(self, "Reload failed", str(e))
+                    return
+                core.apply_servers(fresh, self.servers)
+                try:
+                    backup = core.write_config(self.current_profile.path, fresh)
+                except Exception as e:
+                    QMessageBox.critical(self, "Save failed", str(e))
+                    return
+                self.full_config = fresh
+                self._loaded_mtime = core.config_mtime(self.current_profile.path)
+                self.current_profile.config_exists = True
+                self.dirty = False
+                self.save_btn.setEnabled(False)
+                bnote = f"  ·  backup: {backup.name}" if backup else "  ·  (new file)"
+                self.status.setText(
+                    f"Merged & saved {self.current_profile.path}{bnote}"
+                    f"  ·  Restart {self.current_profile.label} to apply."
+                )
+                return
+            # else OVERWRITE: fall through to normal write
+
         core.apply_servers(self.full_config, self.servers)
         try:
             backup = core.write_config(self.current_profile.path, self.full_config)
         except Exception as e:
             QMessageBox.critical(self, "Save failed", str(e))
             return
+        self._loaded_mtime = core.config_mtime(self.current_profile.path)
         self.current_profile.config_exists = True
         self.dirty = False
         self.save_btn.setEnabled(False)

bcc_core.py

@@ -290,6 +290,14 @@ def _redact_servers_block(block: dict | None) -> dict:
     return {name: _redact_server_data(data) for name, data in block.items()}
 
 
+def _server_sections(cfg: dict) -> dict:
+    """Return the masked server sections of a config dict, safe for diff display."""
+    out: dict = {"mcpServers": _redact_servers_block(cfg.get("mcpServers"))}
+    if DISABLED_KEY in cfg:
+        out[DISABLED_KEY] = _redact_servers_block(cfg.get(DISABLED_KEY))
+    return out
+
+
 def backup_diff(
     config_path: Path | str,
     backup_path: Path | str,
@@ -318,12 +326,6 @@ def backup_diff(
     after_cfg = dict(current_cfg)
     apply_servers(after_cfg, backup_servers)
 
-    def _server_sections(cfg: dict) -> dict:
-        out: dict = {"mcpServers": _redact_servers_block(cfg.get("mcpServers"))}
-        if DISABLED_KEY in cfg:
-            out[DISABLED_KEY] = _redact_servers_block(cfg.get(DISABLED_KEY))
-        return out
-
     before_lines = (
         json.dumps(_server_sections(current_cfg), indent=2, ensure_ascii=False) + "\n"
     ).splitlines(keepends=True)
@@ -370,6 +372,46 @@ def restore_backup(
     return write_config(p, cfg)
 
 
+def config_mtime(path: Path | str) -> float | None:
+    """Return the file's mtime, or None if the file does not exist."""
+    try:
+        return Path(path).stat().st_mtime
+    except OSError:
+        return None
+
+
+def external_change_summary(original_cfg: dict, path: Path | str) -> tuple[list[str], str]:
+    """
+    Compare original_cfg (what BCC loaded) with the current on-disk state.
+
+    Returns:
+      changed_keys — sorted list of top-level keys whose values differ.
+      server_diff  — masked unified diff of server sections (empty if unchanged
+                     or the file cannot be read).
+    """
+    try:
+        disk_cfg = load_config(Path(path))
+    except Exception:
+        return [], ""
+
+    all_keys = set(original_cfg) | set(disk_cfg)
+    changed_keys = sorted(k for k in all_keys if original_cfg.get(k) != disk_cfg.get(k))
+
+    server_diff = ""
+    if any(k in {"mcpServers", DISABLED_KEY} for k in changed_keys):
+        before_lines = (
+            json.dumps(_server_sections(original_cfg), indent=2, ensure_ascii=False) + "\n"
+        ).splitlines(keepends=True)
+        after_lines = (
+            json.dumps(_server_sections(disk_cfg), indent=2, ensure_ascii=False) + "\n"
+        ).splitlines(keepends=True)
+        server_diff = "".join(
+            difflib.unified_diff(before_lines, after_lines, fromfile="loaded", tofile="on disk now")
+        )
+
+    return changed_keys, server_diff
+
+
 # --------------------------------------------------------------------------- #
 # Paste / import parsing
 # --------------------------------------------------------------------------- #

tests/test_core.py

@@ -500,3 +500,86 @@ def test_restore_backup_restores_disabled_servers(tmp_path):
     assert "parked" in restored.get(c.DISABLED_KEY, {})
     assert "active" in restored.get("mcpServers", {})
     assert "newcomer" not in restored.get("mcpServers", {})
+
+
+# --------------------------------------------------------------------------- #
+# 9 — Stale-file protection
+# --------------------------------------------------------------------------- #
+
+
+def test_config_mtime_returns_float_for_existing_file(tmp_path):
+    f = tmp_path / "cfg.json"
+    f.write_text("{}")
+    assert isinstance(c.config_mtime(f), float)
+
+
+def test_config_mtime_returns_none_for_missing_file(tmp_path):
+    assert c.config_mtime(tmp_path / "nonexistent.json") is None
+
+
+def test_external_change_summary_detects_non_server_key_change(tmp_path):
+    cfgpath = tmp_path / "cfg.json"
+    original = {"numStartups": 1, "mcpServers": {"s": {"command": "node"}}}
+    cfgpath.write_text(json.dumps({"numStartups": 2, "mcpServers": {"s": {"command": "node"}}}))
+    changed_keys, server_diff = c.external_change_summary(original, cfgpath)
+    assert "numStartups" in changed_keys
+    assert server_diff == ""  # server sections unchanged
+
+
+def test_external_change_summary_detects_server_section_change(tmp_path):
+    cfgpath = tmp_path / "cfg.json"
+    original = {"mcpServers": {"s1": {"command": "node"}}}
+    cfgpath.write_text(json.dumps({"mcpServers": {"s2": {"command": "python"}}}))
+    changed_keys, server_diff = c.external_change_summary(original, cfgpath)
+    assert "mcpServers" in changed_keys
+    assert "s1" in server_diff or "s2" in server_diff
+
+
+def test_external_change_summary_server_diff_masks_secrets(tmp_path):
+    cfgpath = tmp_path / "cfg.json"
+    original = {"mcpServers": {"a": {"command": "node", "env": {"SECRET_KEY": "s3cr3t"}}}}
+    # command changes (visible in diff) AND secret value changes (both sides masked →
+    # MASK appears in context lines, raw secret never appears)
+    cfgpath.write_text(
+        json.dumps(
+            {"mcpServers": {"a": {"command": "python", "env": {"SECRET_KEY": "n3w_s3cr3t"}}}}
+        )
+    )
+    changed_keys, server_diff = c.external_change_summary(original, cfgpath)
+    assert "mcpServers" in changed_keys
+    assert "s3cr3t" not in server_diff
+    assert "n3w_s3cr3t" not in server_diff
+    assert c.MASK in server_diff  # appears in context for the masked env value
+
+
+def test_external_change_summary_empty_when_unchanged(tmp_path):
+    cfgpath = tmp_path / "cfg.json"
+    cfg = {"mcpServers": {"s": {"command": "node"}}}
+    cfgpath.write_text(json.dumps(cfg))
+    changed_keys, server_diff = c.external_change_summary(cfg, cfgpath)
+    assert changed_keys == []
+    assert server_diff == ""
+
+
+def test_merge_and_reapply_preserves_both_edits(tmp_path):
+    """AC test: external non-server change + in-memory server edit both survive merge."""
+    cfgpath = tmp_path / "cfg.json"
+    original = {"numStartups": 1, "mcpServers": {"s1": {"command": "node"}}}
+    cfgpath.write_text(json.dumps(original, indent=2))
+
+    user_servers = [c.ServerEntry("s2", {"command": "python"}, True)]
+
+    # External process bumps numStartups without touching servers
+    disk_changed = dict(original)
+    disk_changed["numStartups"] = 42
+    cfgpath.write_text(json.dumps(disk_changed, indent=2))
+
+    # Merge: load fresh from disk, apply user's server edits, write back
+    fresh = c.load_config(cfgpath)
+    c.apply_servers(fresh, user_servers)
+    c.write_config(cfgpath, fresh)
+
+    result = json.loads(cfgpath.read_text())
+    assert result["numStartups"] == 42  # external change preserved
+    assert "s2" in result["mcpServers"]  # user's server edit preserved
+    assert "s1" not in result["mcpServers"]  # old server replaced