Merge pull request 'release: v1.2.1 — first shipped build with icon fix, audit fixes #32–#40, Windows tree-kill (#13)' (#51) from release/v1.2.1 into main

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

.github/workflows/ci.yml

@@ -29,21 +29,37 @@ jobs:
         run: ruff format --check .
 
   test:
-    runs-on: ubuntu-latest
-    name: Tests (py${{ matrix.python }})
+    runs-on: ${{ matrix.os }}
+    name: Tests (py${{ matrix.python }} / ${{ matrix.os }})
     strategy:
       fail-fast: false
       matrix:
-        python: ["3.10", "3.12"]
+        os: [ubuntu-latest]
+        python: ["3.10", "3.12", "3.13"]
+        include:
+          # Windows tests on 3.12 only — the version the release binaries ship
+          # with. The self-hosted Windows runner blocks setup-python's install
+          # script (PowerShell execution policy), so it uses the host's `py`
+          # launcher + venv, same as release.yml.
+          - os: windows-latest
+            python: "3.12"
     steps:
       - name: Checkout
         uses: actions/checkout@v4
 
-      - name: Set up Python ${{ matrix.python }}
+      - name: Set up Python ${{ matrix.python }} (Linux)
+        if: runner.os == 'Linux'
         uses: actions/setup-python@v5
         with:
           python-version: ${{ matrix.python }}
 
+      - name: Set up Python venv (Windows)
+        if: runner.os == 'Windows'
+        shell: pwsh
+        run: |
+          py -${{ matrix.python }} -m venv .venv
+          Add-Content -Path $env:GITHUB_PATH -Value "$env:GITHUB_WORKSPACE\.venv\Scripts"
+
       # bcc_core has no GUI imports, so the test suite needs no PySide6 —
       # keeps CI fast and avoids Qt system-library headaches on the runner.
       - name: Install test dependencies

bcc.py

@@ -19,6 +19,7 @@ from PySide6.QtCore import QRect, QSettings, QSize, Qt, QThread, QTimer, QUrl, S
 from PySide6.QtGui import (
     QAction,
     QColor,
+    QCursor,
     QDesktopServices,
     QGuiApplication,
     QIcon,
@@ -51,12 +52,17 @@ from PySide6.QtWidgets import (
     QStyledItemDelegate,
     QTableWidget,
     QTableWidgetItem,
+    QToolTip,
     QVBoxLayout,
     QWidget,
 )
 
 import bcc_core as core
 
+# A full ~/.claude.json with history can be huge; parsing happens on the UI
+# thread during drag-and-drop import, so skip anything larger than this.
+MAX_DROP_IMPORT_BYTES = 5 * 1024 * 1024  # 5 MB
+
 # --- One-line rebrand: change this to recolor the whole app --------------- #
 ACCENT = "#f97316"  # warm orange
 ACCENT_DIM = "#c2570b"
@@ -121,6 +127,7 @@ QScrollBar:vertical {{ background: transparent; width: 10px; margin: 2px; }}
 QScrollBar::handle:vertical {{ background: {BORDER}; border-radius: 5px; min-height: 24px; }}
 QScrollBar::add-line, QScrollBar::sub-line {{ height: 0; }}
 QLabel#statusbar {{ color: {MUTED}; padding: 4px 2px; }}
+QLabel#warnBanner {{ color: #1a1205; background: {WARN}; border-radius: 8px; padding: 8px 10px; font-weight: 600; }}
 QLabel#section {{ color: {MUTED}; font-weight: 600; font-size: 12px; padding: 2px 2px; }}
 QLabel#sectionDisabled {{ color: {MUTED}; font-weight: 600; font-size: 12px; padding: 2px 2px; }}
 QLabel#placeholder {{ color: {MUTED}; padding: 12px; background: {PANEL_2}; border: 1px dashed {BORDER}; border-radius: 8px; }}
@@ -266,10 +273,31 @@ class KeyValueTable(QWidget):
         self.reveal_btn.setText("Hide secrets" if on else "Show secrets")
         self.table.viewport().update()
 
-    def _changed(self, *_):
+    def _changed(self, item=None, *_):
+        if item is not None and item.column() == 0:
+            new_key = item.text().strip()
+            row = item.row()
+            if new_key and self._is_duplicate_key(new_key, row):
+                prev_key = item.data(Qt.ItemDataRole.UserRole)
+                prev_key = prev_key if prev_key is not None else ""
+                self.table.blockSignals(True)
+                item.setText(prev_key)
+                self.table.blockSignals(False)
+                QToolTip.showText(QCursor.pos(), f"Duplicate key '{new_key}' — reverted.")
+                return
+            item.setData(Qt.ItemDataRole.UserRole, new_key)
         if self._on_change:
             self._on_change()
 
+    def _is_duplicate_key(self, key: str, ignore_row: int) -> bool:
+        for r in range(self.table.rowCount()):
+            if r == ignore_row:
+                continue
+            other = self.table.item(r, 0)
+            if other and other.text().strip() == key:
+                return True
+        return False
+
     def _add_row(self):
         dlg = QDialog(self.window())
         dlg.setWindowTitle(f"Add {self._key_label}")
@@ -296,6 +324,13 @@ class KeyValueTable(QWidget):
         grid.addWidget(val_edit, 1, 1)
         v.addLayout(grid)
 
+        # Shown when the typed key already exists in the table.
+        dup_warn = QLabel("")
+        dup_warn.setStyleSheet(f"color: {WARN};")
+        dup_warn.setWordWrap(True)
+        dup_warn.hide()
+        v.addWidget(dup_warn)
+
         # Type an API_KEY/TOKEN-style name and the value field masks itself.
         def _sync_echo(text):
             secret = core.is_secret_key(text)
@@ -311,7 +346,15 @@ class KeyValueTable(QWidget):
         ok_btn = btns.button(QDialogButtonBox.StandardButton.Ok)
         ok_btn.setObjectName("primary")
         ok_btn.setEnabled(False)
-        key_edit.textChanged.connect(lambda t: ok_btn.setEnabled(bool(t.strip())))
+
+        def _validate(text):
+            k = text.strip()
+            dup = bool(k) and self._is_duplicate_key(k, ignore_row=-1)
+            ok_btn.setEnabled(bool(k) and not dup)
+            dup_warn.setText(f"'{k}' already exists" if dup else "")
+            dup_warn.setVisible(dup)
+
+        key_edit.textChanged.connect(_validate)
         btns.accepted.connect(dlg.accept)
         btns.rejected.connect(dlg.reject)
         v.addWidget(btns)
@@ -325,13 +368,15 @@ class KeyValueTable(QWidget):
             return
         k = key_edit.text().strip()
         val = val_edit.text()
-        if not k:
+        if not k or self._is_duplicate_key(k, ignore_row=-1):
             return
         if self._before_change:
             self._before_change()
         r = self.table.rowCount()
         self.table.insertRow(r)
-        self.table.setItem(r, 0, QTableWidgetItem(k))
+        key_item = QTableWidgetItem(k)
+        key_item.setData(Qt.ItemDataRole.UserRole, k)
+        self.table.setItem(r, 0, key_item)
         self.table.setItem(r, 1, QTableWidgetItem(val))
         self._changed()
 
@@ -350,7 +395,9 @@ class KeyValueTable(QWidget):
         for k, v in (d or {}).items():
             r = self.table.rowCount()
             self.table.insertRow(r)
-            self.table.setItem(r, 0, QTableWidgetItem(str(k)))
+            key_item = QTableWidgetItem(str(k))
+            key_item.setData(Qt.ItemDataRole.UserRole, str(k))
+            self.table.setItem(r, 0, key_item)
             self.table.setItem(r, 1, QTableWidgetItem(str(v)))
         self.table.blockSignals(False)
 
@@ -1492,6 +1539,15 @@ class MainWindow(QMainWindow):
 
         root.addLayout(self._build_topbar())
 
+        # Persistent warning banner (MSIX-virtualized config, etc.). Lives in
+        # its own widget because the status bar is rewritten on every action,
+        # which used to wipe the warning before the user could read it.
+        self.warn_banner = QLabel("")
+        self.warn_banner.setObjectName("warnBanner")
+        self.warn_banner.setWordWrap(True)
+        self.warn_banner.hide()
+        root.addWidget(self.warn_banner)
+
         # User-draggable divider between the server list and the editor.
         split = QSplitter(Qt.Orientation.Horizontal)
         split.setChildrenCollapsible(False)
@@ -1543,14 +1599,16 @@ class MainWindow(QMainWindow):
         last = float(st.value("update/lastCheck", 0.0, type=float) or 0.0)
         if (time.time() - last) < 86400:  # at most once/day
             return
-        st.setValue("update/lastCheck", time.time())
         self._startup_update_worker = UpdateCheckWorker()
         self._startup_update_worker.done.connect(self._on_startup_update_checked)
         self._startup_update_worker.start()
 
     def _on_startup_update_checked(self, release: dict | None):
         self._startup_update_worker = None
-        if release and core.is_newer_version(core.__version__, release["version"]):
+        if release is None:
+            return  # offline/failed check: don't advance lastCheck, allow retry
+        QSettings("BCC", "BetterClaudeConfig").setValue("update/lastCheck", time.time())
+        if core.is_newer_version(core.__version__, release["version"]):
             self.status.setText(
                 f"Update available: {release['version']}  ·  Help ▸ About to view it."
             )
@@ -1764,8 +1822,9 @@ class MainWindow(QMainWindow):
     def _maybe_warn_msix(self):
         """
         Windows-only, no-op everywhere else: if Claude Desktop looks like an
-        MSIX/Store install with a virtualized config, append a warning to the
-        status bar so edits to the plain %APPDATA% path aren't silently lost.
+        MSIX/Store install with a virtualized config, show a persistent banner
+        so edits to the plain %APPDATA% path aren't silently lost. (The status
+        bar is the wrong home for this: it's rewritten on every action.)
         Defensive on purpose -- this must never block startup or profile load.
         """
         try:
@@ -1773,8 +1832,11 @@ class MainWindow(QMainWindow):
         except Exception:
             return
         if warning:
-            self.status.setText(f"{self.status.text()}  ⚠ {warning}")
-            self.status.setToolTip(warning)
+            self.warn_banner.setText(f"⚠  {warning}")
+            self.warn_banner.setToolTip(warning)
+            self.warn_banner.show()
+        else:
+            self.warn_banner.hide()
 
     def add_custom_config(self):
         start = str(core.app_support_base())
@@ -2077,12 +2139,17 @@ class MainWindow(QMainWindow):
         if not (0 <= idx < len(self.servers)):
             return
         entry = self.servers[idx]
+        old_name = entry.name
         entry.name = self.editor.current_name()
         entry.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
-        # was spawn-tested no longer matches what's on disk once saved.
+        # was spawn-tested no longer matches what's on disk once saved. Pop
+        # both names: the old one (so a rename doesn't leave a stale entry
+        # for whoever takes that name next) and the new one (so we don't
+        # inherit a stale result cached under the name being renamed to).
+        self._health.pop(old_name, None)
         self._health.pop(entry.name, None)
         loc = self._row_of_index.get(idx)
         if loc:
@@ -2400,29 +2467,44 @@ class MainWindow(QMainWindow):
             e.acceptProposedAction()
 
     def dropEvent(self, e):
+        total_added, total_replaced, files_imported = 0, 0, 0
+        undo_pushed = False
         for u in e.mimeData().urls():
             path = u.toLocalFile()
             if not path.endswith(".json"):
                 continue
-            text = Path(path).read_text(encoding="utf-8")
+            p = Path(path)
+            if p.stat().st_size > MAX_DROP_IMPORT_BYTES:
+                QMessageBox.warning(
+                    self,
+                    "File too large",
+                    f"{p.name}:\nFile exceeds the 5 MB import limit and was skipped.",
+                )
+                continue
+            text = p.read_text(encoding="utf-8")
             try:
                 servers = core.parse_pasted_json(text)
             except Exception as ex:
-                QMessageBox.warning(self, "Couldn't import", f"{Path(path).name}:\n{ex}")
+                QMessageBox.warning(self, "Couldn't import", f"{p.name}:\n{ex}")
                 continue
+            if not undo_pushed:
                 self._push_undo()
+                undo_pushed = True
             added, replaced = 0, 0
             for name, data in servers.items():
                 a, r = self._import_server(name, data)
                 added += int(a)
                 replaced += int(r)
+            total_added += added
+            total_replaced += replaced
+            files_imported += 1
+        if files_imported:
             self._refresh_tables(select_index=len(self.servers) - 1)
             self._mark_dirty()
             self.status.setText(
-                f"Imported {added} added, {replaced} replaced from {Path(path).name}. "
-                "Review and Save."
+                f"Imported {total_added} added, {total_replaced} replaced from "
+                f"{files_imported} file(s). Review and Save."
             )
-            break
 
     def closeEvent(self, e):
         if self.dirty and not self._confirm_discard():

bcc.spec

@@ -78,8 +78,8 @@ if sys.platform == "darwin":
         info_plist={
             "CFBundleName": "Better Claude Config",
             "CFBundleDisplayName": "Better Claude Config",
-            "CFBundleShortVersionString": "1.0.0",
-            "CFBundleVersion": "1.0.0",
+            "CFBundleShortVersionString": "1.2.1",
+            "CFBundleVersion": "1.2.1",
             "NSHighResolutionCapable": True,
             "NSRequiresAquaSystemAppearance": False,  # supports dark mode
             "LSMinimumSystemVersion": "11.0",

bcc_core.py

@@ -59,7 +59,7 @@ KNOWN_FIELDS = {"command", "args", "env", "url", "type", "headers"}
 # binary. All network I/O here is fail-quiet (returns None on any problem)
 # so it's safe to run unattended, off the UI thread, at startup.
 # --------------------------------------------------------------------------- #
-__version__ = "1.2.0"
+__version__ = "1.2.1"
 
 REPO_URL = "https://git.avezzano.io/the_og/better-claude-config"
 ISSUES_URL = f"{REPO_URL}/issues"
@@ -1581,6 +1581,23 @@ def server_log_path(name: str) -> Path | None:
 _STDERR_CAP = 4096  # bytes
 
 
+def _kill_process_tree_windows(pid: int) -> None:
+    """
+    Kill `pid` and its whole descendant tree via `taskkill /T /F` (issue #13).
+
+    Popen.kill() only terminates the direct child; runner-style commands
+    (npx → node → server, cmd → real process) leave the actual server alive,
+    leaking a process on every Windows spawn test. taskkill walks the tree.
+    """
+    flags = getattr(subprocess, "CREATE_NO_WINDOW", 0)  # no console flash from the GUI exe
+    with contextlib.suppress(OSError):
+        subprocess.run(
+            ["taskkill", "/PID", str(pid), "/T", "/F"],
+            capture_output=True,
+            creationflags=flags,
+        )
+
+
 def spawn_test(data: dict, timeout: float = 3.0) -> dict:
     """
     Attempt to start a stdio server and observe it for `timeout` seconds.
@@ -1675,6 +1692,10 @@ def _spawn_test_impl(data: dict, timeout: float) -> dict:
     )
     if os.name != "nt":
         popen_kwargs["start_new_session"] = True  # own process group → clean kill
+    else:
+        # The packaged app is windowed (console=False); without this every
+        # spawn test of a console server flashes a console window.
+        popen_kwargs["creationflags"] = getattr(subprocess, "CREATE_NO_WINDOW", 0)
 
     try:
         proc = subprocess.Popen(args_list, **popen_kwargs)
@@ -1696,7 +1717,7 @@ def _spawn_test_impl(data: dict, timeout: float) -> dict:
             if os.name != "nt":
                 os.killpg(os.getpgid(proc.pid), _signal.SIGKILL)
             else:
-                proc.kill()  # best-effort on Windows
+                _kill_process_tree_windows(proc.pid)
         except OSError:
             pass
         with contextlib.suppress(subprocess.TimeoutExpired):

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "better-claude-config"
-version = "1.2.0"
+version = "1.2.1"
 description = "Cross-platform GUI for editing the mcpServers block of Claude Desktop and Claude Code configs"
 readme = "README.md"
 license = { file = "LICENSE" }

tests/test_core.py

@@ -3,9 +3,11 @@ proper test functions with tmp_path/monkeypatch fixtures)."""
 
 import json
 import os
+import re
 import sys
 import urllib.error
 import urllib.request
+from pathlib import Path
 
 import pytest
 
@@ -227,8 +229,11 @@ def test_diagnostics_redacts_token_args():
 # --------------------------------------------------------------------------- #
 # 6. Dependency / PATH checking
 # --------------------------------------------------------------------------- #
-def test_dep_check_finds_python3():
-    assert c.check_dependency({"command": "python3"})["status"] == "ok"
+def test_dep_check_finds_python():
+    # "python3" does not exist on a stock Windows install; "warn" (found only
+    # on an augmented PATH) still proves resolution works on a real machine.
+    cmd = "python" if os.name == "nt" else "python3"
+    assert c.check_dependency({"command": cmd})["status"] in ("ok", "warn")
 
 
 def test_dep_check_flags_missing():
@@ -326,6 +331,55 @@ def test_spawn_test_large_stderr_does_not_deadlock():
     assert len(r["stderr"]) <= c._STDERR_CAP
 
 
+def test_windows_tree_kill_uses_taskkill(monkeypatch):
+    """The Windows kill path must walk the process tree (taskkill /T /F);
+    Popen.kill() alone orphans grandchildren (issue #13)."""
+    calls = []
+
+    def fake_run(cmd, **kwargs):
+        calls.append(cmd)
+        return _FakeRC()
+
+    class _FakeRC:
+        returncode = 0
+
+    monkeypatch.setattr(c.subprocess, "run", fake_run)
+    c._kill_process_tree_windows(1234)
+    assert calls == [["taskkill", "/PID", "1234", "/T", "/F"]]
+
+
+@pytest.mark.skipif(os.name != "nt", reason="Windows-only: real process-tree kill")
+def test_spawn_test_windows_kills_child_process_tree(tmp_path):
+    """A spawn-tested parent that has its own child (npx -> node style) must
+    not leave the child running after the timeout kill (issue #13)."""
+    import subprocess as sp
+    import time as _t
+
+    pid_file = tmp_path / "child.pid"
+    parent_script = (
+        "import subprocess, sys, time\n"
+        "p = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(60)'])\n"
+        f"open({str(pid_file)!r}, 'w').write(str(p.pid))\n"
+        "time.sleep(60)\n"
+    )
+    r = c.spawn_test({"command": sys.executable, "args": ["-c", parent_script]}, timeout=2.0)
+    assert r["outcome"] == "ok"  # parent was still running at timeout, then tree-killed
+    assert pid_file.is_file(), "parent never spawned its child"
+    child_pid = int(pid_file.read_text())
+
+    deadline = _t.time() + 5.0
+    alive = True
+    while _t.time() < deadline:
+        out = sp.run(
+            ["tasklist", "/FI", f"PID eq {child_pid}"], capture_output=True, text=True
+        ).stdout
+        alive = str(child_pid) in out
+        if not alive:
+            break
+        _t.sleep(0.25)
+    assert not alive, f"child pid {child_pid} survived the spawn-test kill"
+
+
 def test_spawn_test_non_string_env_value():
     # Pasted JSON can carry numeric env values ("env": {"PORT": 8080}) that never
     # round-trip through the editor. Popen rejects non-str env; spawn_test must
@@ -1135,8 +1189,12 @@ def test_restart_claude_desktop_linux_refuses_without_touching_processes(monkeyp
 # --------------------------------------------------------------------------- #
 def test_dunder_version_matches_pyproject():
     # Guards against the version drifting out of sync between the two places
-    # a human might bump it.
-    assert c.__version__ == "1.2.0"
+    # a human might bump it. Reads pyproject.toml rather than hard-coding a
+    # version here (which would make this a third place to bump).
+    text = (Path(__file__).parent.parent / "pyproject.toml").read_text(encoding="utf-8")
+    m = re.search(r'^version\s*=\s*"([^"]+)"', text, re.MULTILINE)
+    assert m, "no [project] version found in pyproject.toml"
+    assert c.__version__ == m.group(1)
 
 
 def test_parse_version_basic():