feat: detect stale config and prompt merge-or-overwrite on save
Records the file mtime at load time; before write_config() fires, re-checks it. If it changed (e.g. `claude mcp add`, a second BCC window, or Claude itself writing ~/.claude.json), StaleDialog prompts with the changed top-level key names and a masked server-section diff. "Merge & save" applies the user's in-memory server edits on top of the current on-disk file (preserving external non-server changes); "Overwrite anyway" proceeds as before. - bcc_core: config_mtime(), external_change_summary(), _server_sections() helper extracted from backup_diff for reuse - bcc.py: StaleDialog, MainWindow._loaded_mtime tracked through load/save - tests: 7 new tests (config_mtime, external_change_summary variants, AC merge test) Closes #4 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -926,6 +926,80 @@ class RestoreDialog(QDialog):
|
|||||||
return item.data(Qt.ItemDataRole.UserRole)
|
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 & 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
|
# Paste-JSON dialog
|
||||||
# --------------------------------------------------------------------------- #
|
# --------------------------------------------------------------------------- #
|
||||||
@@ -1016,6 +1090,7 @@ class MainWindow(QMainWindow):
|
|||||||
self.full_config: dict = {}
|
self.full_config: dict = {}
|
||||||
self.servers: list[core.ServerEntry] = []
|
self.servers: list[core.ServerEntry] = []
|
||||||
self.current_profile: core.Profile | None = None
|
self.current_profile: core.Profile | None = None
|
||||||
|
self._loaded_mtime: float | None = None
|
||||||
self.dirty = False
|
self.dirty = False
|
||||||
self._suppress_table = False
|
self._suppress_table = False
|
||||||
self._suppress_sel = False
|
self._suppress_sel = False
|
||||||
@@ -1291,6 +1366,7 @@ class MainWindow(QMainWindow):
|
|||||||
return
|
return
|
||||||
self.full_config = cfg
|
self.full_config = cfg
|
||||||
repaired = True
|
repaired = True
|
||||||
|
self._loaded_mtime = core.config_mtime(profile.path)
|
||||||
self.current_profile = profile
|
self.current_profile = profile
|
||||||
self.servers = core.extract_servers(self.full_config)
|
self.servers = core.extract_servers(self.full_config)
|
||||||
self.dirty = False
|
self.dirty = False
|
||||||
@@ -1627,12 +1703,52 @@ class MainWindow(QMainWindow):
|
|||||||
if not self._validate():
|
if not self._validate():
|
||||||
QMessageBox.warning(self, "Can't save yet", "Fix the highlighted problem first.")
|
QMessageBox.warning(self, "Can't save yet", "Fix the highlighted problem first.")
|
||||||
return
|
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)
|
core.apply_servers(self.full_config, self.servers)
|
||||||
try:
|
try:
|
||||||
backup = core.write_config(self.current_profile.path, self.full_config)
|
backup = core.write_config(self.current_profile.path, self.full_config)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
QMessageBox.critical(self, "Save failed", str(e))
|
QMessageBox.critical(self, "Save failed", str(e))
|
||||||
return
|
return
|
||||||
|
self._loaded_mtime = core.config_mtime(self.current_profile.path)
|
||||||
self.current_profile.config_exists = True
|
self.current_profile.config_exists = True
|
||||||
self.dirty = False
|
self.dirty = False
|
||||||
self.save_btn.setEnabled(False)
|
self.save_btn.setEnabled(False)
|
||||||
|
|||||||
+48
-6
@@ -290,6 +290,14 @@ def _redact_servers_block(block: dict | None) -> dict:
|
|||||||
return {name: _redact_server_data(data) for name, data in block.items()}
|
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(
|
def backup_diff(
|
||||||
config_path: Path | str,
|
config_path: Path | str,
|
||||||
backup_path: Path | str,
|
backup_path: Path | str,
|
||||||
@@ -318,12 +326,6 @@ def backup_diff(
|
|||||||
after_cfg = dict(current_cfg)
|
after_cfg = dict(current_cfg)
|
||||||
apply_servers(after_cfg, backup_servers)
|
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 = (
|
before_lines = (
|
||||||
json.dumps(_server_sections(current_cfg), indent=2, ensure_ascii=False) + "\n"
|
json.dumps(_server_sections(current_cfg), indent=2, ensure_ascii=False) + "\n"
|
||||||
).splitlines(keepends=True)
|
).splitlines(keepends=True)
|
||||||
@@ -370,6 +372,46 @@ def restore_backup(
|
|||||||
return write_config(p, cfg)
|
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
|
# Paste / import parsing
|
||||||
# --------------------------------------------------------------------------- #
|
# --------------------------------------------------------------------------- #
|
||||||
|
|||||||
@@ -500,3 +500,86 @@ def test_restore_backup_restores_disabled_servers(tmp_path):
|
|||||||
assert "parked" in restored.get(c.DISABLED_KEY, {})
|
assert "parked" in restored.get(c.DISABLED_KEY, {})
|
||||||
assert "active" in restored.get("mcpServers", {})
|
assert "active" in restored.get("mcpServers", {})
|
||||||
assert "newcomer" not 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
|
||||||
|
|||||||
Reference in New Issue
Block a user