From 8309e42dd81fb1a67be4c7bc21fb8ee527beb721 Mon Sep 17 00:00:00 2001 From: Cowork Supervisor Date: Sun, 12 Jul 2026 12:48:03 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20spawn=5Ftest=20never=20raises=20?= =?UTF-8?q?=E2=80=94=20coerce=20env/command=20to=20str,=20wrap=20unexpecte?= =?UTF-8?q?d=20errors=20(#34)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #34 Co-Authored-By: Claude Fable 5 --- ROADMAP-POST-1.2-COWORK.md | 112 +++++++++++++++++++++++++++++++++++++ bcc_core.py | 24 +++++++- tests/test_core.py | 36 ++++++++++++ 3 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 ROADMAP-POST-1.2-COWORK.md diff --git a/ROADMAP-POST-1.2-COWORK.md b/ROADMAP-POST-1.2-COWORK.md new file mode 100644 index 0000000..13a235b --- /dev/null +++ b/ROADMAP-POST-1.2-COWORK.md @@ -0,0 +1,112 @@ +# BCC Roadmap — v1.2.0 readiness + what's next (Cowork planning, 2026-07-07) + +Written by the Cowork supervisor session. Captures (1) what's ready for the next +release, (2) the remaining tracked backlog, and (3) genuinely-missing features +worth planning. Priorities are suggestions for AJ, not commitments. + +--- + +## 1. v1.2.0 — ready to ship + +Five feature branches landed as green, isolated, reviewed PRs (all approved by the +supervisor session; each touches only `bcc.py` / `bcc_core.py` / `tests/test_core.py` +and passes CI): + +| PR | Issue | Feature | CI | +|----|-------|---------|----| +| #25 | #18 + #19 | About dialog (Help menu, repo/issues/license links) + notify-only update checker (`__version__`, numeric version-compare, fail-quiet Gitea releases fetch, throttled once/day startup check) | run 160 ✓ | +| #24 | #9 | Restart Claude Desktop button (post-save, Desktop-profile-only, platform-abstracted `restart_claude_desktop()`) | run 159 ✓ | +| #23 | #6 | In-app MCP server log viewer (`server_log_path()`, read-only auto-tailing panel per server) | run 158 ✓ | +| #22 | #17 | Stale-file detection hardened with size (`ConfigStat` mtime+size fingerprint) | run 157 ✓ | +| #21 | #8 | Duplicate-name collision fix — closes a real silent-overwrite on drag-and-drop import | run 164 ✓ | + +Combined they take the suite from 92 tests toward ~130+ (each PR adds 3–20 tests). + +### Merge / release checklist (needs AJ — this is the open decision) + +The five branches all edit overlapping regions of `bcc.py`/`bcc_core.py`, so they +will **not** merge cleanly in sequence without conflict resolution: + +- **Shared conflict:** #22/#23/#24/#25 each rewrote the one `_normalize_unicode` + non-breaking-space line (all behavior-identical). Merge one, then take that line + from any branch for the rest. +- **Save-flow region:** #21 (import prompt), #22 (stale fingerprint), #24 (restart + button) all touch the save path in `bcc.py` — review the merged result once. +- **Version bump:** at cut, set both `__version__` (bcc_core.py) and + `pyproject.toml` to `1.2.0` and update `test_dunder_version_matches_pyproject`. +- **Release is tag-triggered:** push a `v1.2.0` tag → `.github/workflows/release.yml` + builds macOS/Windows/Linux and publishes. Don't hand-create the release. + +Suggested merge order (least- to most-conflict-prone): **#21 → #22 → #23 → #24 → #25**. +Recommend AJ does the merges + conflict resolution locally (cleaner than the Gitea +API for overlapping diffs), or greenlights the supervisor to merge sequentially and +patch conflicts on each branch via the API. + +--- + +## 2. Remaining tracked backlog (open issues) + +**Windows-specific — deferred (can't build/test in the Linux/mac sandbox):** + +- **#20** Windows release shows no app icon (P?) — build-config fix in `bcc.spec` / + `release.yml`; needs a Windows build to verify. Low risk, cosmetic. +- **#7** Windows MSIX virtualized-path detection (P1) — config edits silently ignored + on MSIX installs. Core path-detection is unit-testable; the runtime behavior needs + a real MSIX Windows box. Genuinely valuable (silent failure class). +- **#13** Windows process-tree kill is best-effort (P1) — follow-up refinement to #9; + hard to verify without Windows. + +**Deferred by design:** + +- **#5** Cross-client support (Cursor `.cursor/mcp.json`, Windsurf + `~/.codeium/windsurf/mcp_config.json`, VS Code `servers` key) — P1 but a behemoth; + its own branch, revisit after single-client UX is solid. Not a v1.2 item. +- **#10** Server catalog / one-click add — P2, design-first. See §3 overlap with search. + +--- + +## 3. Genuinely-missing features (not yet tracked) — planning candidates + +Grounded against what already exists (enable/disable toggle, duplicate-server, +`_disabledMcpServers` round-trip, backup/restore, spawn-test, secret masking, JSON +repair pipeline — all shipped, so not repeated here). + +**P1 candidates** + +1. **Server search / filter box.** Once a user has many servers (and especially once + #10's catalog lands), there's no way to filter the list. Small, high-utility: + a `QLineEdit` above the table filtering by name/command. Pure-ish; testable filter + fn in core. +2. **"Test all" / health column.** Extends spawn-test (#2) + log viewer (#6): a status + dot per server (untested / ok / crashed) and a one-click "test all". Turns three + separate features into an at-a-glance dashboard — the single biggest "is my config + actually working?" answer. +3. **Project-level `.mcp.json` discovery (Claude Code).** Claude Code reads project + `.mcp.json` files; BCC only sees the global config. Detect and offer to edit the + project file when relevant. Previously listed "not in backlog" — worth reconsidering + now that Claude Code support is solid. + +**P2 candidates** + +4. **`${VAR}` / `.env` environment-variable expansion.** Previously declined (AJ's + call). Revisit only if secret-handling demand grows — pairs with keychain below. +5. **OS keychain integration for secrets.** Previously declined. Bigger lift; would + let env secrets live outside the JSON entirely. Design-first if pursued. +6. **Config schema/lint for well-known servers.** Warn on obvious mistakes (missing + `command`, `args` not a list, unknown top-level keys) beyond the JSON-repair layer. +7. **Theming / dark-mode follow-through.** Style constants (`MUTED`/`ACCENT`/`GOOD`) + already exist; a proper light/dark toggle is a polish item. + +**Explicitly out (recorded decisions):** renaming BCC (deferred until cross-client +actually ships — do not raise before then). + +--- + +## 4. Suggested next sprint (after v1.2.0 ships) + +1. Merge + release v1.2.0 (decision above). +2. **#7 MSIX detection** — highest-value untracked-runtime P1; land the testable core + path-detection now, gate the runtime behavior behind a Windows verification. +3. **Server search/filter (§3.1)** + **Test-all health column (§3.2)** — small, high + daily-utility, and they compound with the log viewer/spawn-test already in 1.2. +4. Design pass on **#10 catalog** and **#5 cross-client** (both need design before code). diff --git a/bcc_core.py b/bcc_core.py index 7a8574f..8f89be6 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -1591,12 +1591,29 @@ def spawn_test(data: dict, timeout: float = 3.0) -> dict: "crashed" — exited with a non-zero code before timeout "not_found" — command could not be resolved to an executable "not_applicable" — remote server or no command; nothing to spawn + "error" — unexpected internal failure while spawning/observing returncode: int | None stderr: str (first ~4 KB) detail: str + Never raises: the GUI threads (Test launch / Test all) re-enable their + buttons only when a result arrives, so an escaping exception would leave + the UI stuck. Anything unexpected comes back as outcome "error". + Run this off the UI thread — it blocks for up to `timeout` seconds. """ + try: + return _spawn_test_impl(data, timeout) + except Exception as e: + return { + "outcome": "error", + "returncode": None, + "stderr": "", + "detail": f"unexpected error: {e!r}", + } + + +def _spawn_test_impl(data: dict, timeout: float) -> dict: if "url" in data and "command" not in data: return { "outcome": "not_applicable", @@ -1605,7 +1622,9 @@ def spawn_test(data: dict, timeout: float = 3.0) -> dict: "detail": "remote server", } - cmd = (data.get("command") or "").strip() + # str() first: pasted JSON can legally carry a non-string here and the + # value never round-trips through the editor before a Test all run. + cmd = str(data.get("command") or "").strip() if not cmd: return { "outcome": "not_applicable", @@ -1627,7 +1646,8 @@ def spawn_test(data: dict, timeout: float = 3.0) -> dict: args_list = [resolved_cmd] + [str(a) for a in (data.get("args") or [])] merged_env = {**os.environ, "PATH": augmented_path()} - merged_env.update(data.get("env") or {}) + # Popen rejects non-string env values; pasted JSON may carry numbers. + merged_env.update({str(k): str(v) for k, v in (data.get("env") or {}).items()}) stderr_chunks: list[bytes] = [] diff --git a/tests/test_core.py b/tests/test_core.py index b07db51..dfca4e7 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -326,6 +326,42 @@ def test_spawn_test_large_stderr_does_not_deadlock(): assert len(r["stderr"]) <= c._STDERR_CAP +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 + # coerce instead of letting TypeError escape (which would leave the GUI's + # Test launch / Test all buttons stuck disabled). + r = c.spawn_test( + { + "command": sys.executable, + "args": ["-c", "import os; raise SystemExit(0 if os.environ['PORT'] == '8080' else 1)"], + "env": {"PORT": 8080}, + }, + timeout=2.0, + ) + assert r["outcome"] == "exited" + assert r["returncode"] == 0 + + +def test_spawn_test_non_string_command(): + # A non-string command must classify, not raise (str(123) resolves to nothing). + r = c.spawn_test({"command": 123}, timeout=0.3) + assert r["outcome"] == "not_found" + + +def test_spawn_test_internal_error_yields_result(monkeypatch): + # Any unexpected exception inside the spawn path must come back as a result + # dict (outcome "error"), never escape — the UI only re-enables its buttons + # when a result arrives. + def boom(*a, **k): + raise RuntimeError("simulated internal failure") + + monkeypatch.setattr(c.shutil, "which", boom) + r = c.spawn_test({"command": "python3"}, timeout=0.3) + assert r["outcome"] == "error" + assert "simulated internal failure" in r["detail"] + + # --------------------------------------------------------------------------- # # 8. Backup restore # --------------------------------------------------------------------------- #