feat: stale-file protection — detect concurrent edits on save #15
@@ -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 & 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)
|
||||
|
||||
+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()}
|
||||
|
||||
|
||||
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
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user