feat(#91): sidecar config detection + precedence + verified paths

ssh-mcp v2 reads a TOML sidecar and only falls back to CLI args when that file is ABSENT — so BCC's managed --host/--user args can be completely inert while the real config lives in a file BCC never looks at. This adds read-only truth-telling for that (no sidecar writing). Core (pure, fully injectable platform/environ/home/exists/read for testing): - sidecar_path(spec): VERIFIED per-platform TOML location from the package source, NOT the README (macOS → ~/Library/Application Support/ssh-mcp, Windows → %APPDATA%, else → ${XDG_CONFIG_HOME:-~/.config}). sidecar_doc_path() is the README path. - sidecar_status(): resolves exists / has_managed_args / args_inert / wrong_path. - sidecar_warnings(): mirrors removed_flag_warnings' shape. Reports: * precedence — "these arguments are inert; the server reads <real path>" * wrong-path — a TOML at the README path the server never actually reads * #11 credential scoping — an unprefixed SSH_MCP_PASSWORD shared across 2+ profiles in a multi-profile sidecar (count_toml_profiles is a documented 3.10-safe heuristic; unscoped_credential_warning gates on it). GUI: a read-only advisory label in the stdio editor (mirrors the removed-flag label; no fix button — editing the sidecar is a separate deliberate action). Uses the real platform/env/filesystem so it reflects this machine. pytest green, ruff + format clean. Closes #91. Part of epic #94. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-12 02:59:49 -04:00
parent e087107710
commit 8c51c25211
3 changed files with 385 additions and 0 deletions
@@ -2535,6 +2535,248 @@ def drift_warnings(data: dict) -> list[str]:
    return out


+# --------------------------------------------------------------------------- #
+# Sidecar config detection + precedence (issue #91, epic #94)
+#
+# Some MCP servers read a config *sidecar* (ssh-mcp v2 reads a TOML file) and
+# only fall back to CLI args when that file is ABSENT. So BCC's carefully-managed
+# --host/--user args can be completely inert while the real config lives in a file
+# BCC never looks at. Two traps this surfaces (read-only — no sidecar writing):
+#
+#   1. Precedence: when the sidecar exists, the args are inert. Tell the user
+#      where the file the server actually reads is.
+#   2. Verified vs documented paths: ssh-mcp's README says ~/.config/ssh-mcp/…,
+#      but the code resolves the OS-native app-data dir on macOS/Windows. A user
+#      following the README writes a file the server never reads, with no error.
+#
+# Plus the #11 credential-scoping trap: an unprefixed SSH_MCP_PASSWORD is offered
+# to *every* profile in a multi-profile TOML — detect that and suggest scoping.
+#
+# All resolution is injectable (platform / environ / home / exists / read) so the
+# logic is unit-testable with fixtures and never touches the real filesystem in CI.
+# --------------------------------------------------------------------------- #
+# ${VAR} and ${VAR:-default} expansion inside a sidecar path template.
+_SIDECAR_VAR_RE = re.compile(r"\$\{([A-Z_][A-Z0-9_]*)(?::-([^}]*))?\}")
+
+# The connection-defining args a sidecar would override (rendering them inert).
+_SIDECAR_MANAGED_FLAGS = ("--host", "--user", "--port", "--identity", "--key")
+
+
+def _sidecar_platform_key(platform: str) -> str:
+    """Map a sys.platform-style string to a ServerSpec.sidecar_paths key."""
+    if platform == "darwin":
+        return "darwin"
+    if platform.startswith("win"):
+        return "win32"
+    return "posix"
+
+
+def _expand_sidecar_template(tmpl: str, environ: dict, home: Path) -> str:
+    """Expand ${VAR}/${VAR:-default} and a leading ~ in a path template.
+
+    Injectable so tests drive it with a fixture environ + home rather than the
+    real process environment. An unset ${VAR} with no default expands to "".
+    """
+
+    def sub(m: re.Match) -> str:
+        var, default = m.group(1), m.group(2)
+        val = environ.get(var)
+        if val:
+            return val
+        return default if default is not None else ""
+
+    s = _SIDECAR_VAR_RE.sub(sub, tmpl)
+    # A leading ~ (either literal, or introduced by a ${XDG:-~/.config} default).
+    if s.startswith("~"):
+        s = str(home) + s[1:]
+    return s
+
+
+def sidecar_path(
+    spec: ServerSpec | None,
+    *,
+    platform: str | None = None,
+    environ: dict | None = None,
+    home: str | os.PathLike | None = None,
+) -> Path | None:
+    """The VERIFIED sidecar config path for `spec` on `platform`, or None.
+
+    None when the package has no sidecar, or the platform has no entry. Uses the
+    package-source-derived paths in ServerSpec.sidecar_paths — NOT the README,
+    which is wrong on macOS/Windows. All inputs are injectable for testing.
+    """
+    if spec is None or not spec.sidecar_paths:
+        return None
+    platform = platform if platform is not None else sys.platform
+    environ = environ if environ is not None else dict(os.environ)
+    home = Path(home) if home is not None else Path.home()
+    tmpl = spec.sidecar_paths.get(_sidecar_platform_key(platform))
+    if not tmpl:
+        return None
+    return Path(_expand_sidecar_template(tmpl, environ, home))
+
+
+def sidecar_doc_path(
+    spec: ServerSpec | None,
+    *,
+    home: str | os.PathLike | None = None,
+) -> Path | None:
+    """The path the package's README documents (but may not actually read)."""
+    if spec is None or not spec.sidecar_doc_path:
+        return None
+    home = Path(home) if home is not None else Path.home()
+    return Path(_expand_sidecar_template(spec.sidecar_doc_path, {}, home))
+
+
+def _has_managed_connection_args(data: dict) -> bool:
+    """True if the server carries connection args a sidecar would make inert."""
+    return any(str(a).split("=", 1)[0] in _SIDECAR_MANAGED_FLAGS for a in data.get("args") or [])
+
+
+def sidecar_status(
+    data: dict,
+    *,
+    platform: str | None = None,
+    environ: dict | None = None,
+    home: str | os.PathLike | None = None,
+    exists=None,
+) -> dict | None:
+    """Resolve sidecar presence + precedence for one server. Pure/injectable.
+
+    Returns None when the server isn't a package with a sidecar. Otherwise a
+    dict: ``{package, path, doc_path, exists, doc_exists, has_managed_args,
+    args_inert, wrong_path}``. ``exists`` is a callable ``Path -> bool`` so tests
+    inject a virtual filesystem; it defaults to a real ``Path.is_file`` check.
+    """
+    spec = resolve_server_spec(data)
+    if spec is None:
+        return None
+    path = sidecar_path(spec, platform=platform, environ=environ, home=home)
+    if path is None:
+        return None
+    doc = sidecar_doc_path(spec, home=home)
+    exists = exists if exists is not None else (lambda p: Path(p).is_file())
+    file_exists = bool(exists(path))
+    # Only count the doc path as a separate "wrong place" when it's genuinely a
+    # different location from the real one (on Linux they coincide).
+    doc_exists = bool(doc is not None and doc != path and exists(doc))
+    has_managed = _has_managed_connection_args(data)
+    return {
+        "package": spec.package,
+        "path": path,
+        "doc_path": doc,
+        "exists": file_exists,
+        "doc_exists": doc_exists,
+        "has_managed_args": has_managed,
+        # Precedence: the sidecar wins, so managed args are inert only when it exists.
+        "args_inert": file_exists and has_managed,
+        # A file sits where the README says but not where the server actually reads.
+        "wrong_path": doc_exists and not file_exists,
+    }
+
+
+def count_toml_profiles(text: str) -> int:
+    """Best-effort count of profile sections in a TOML sidecar.
+
+    ssh-mcp's multi-profile mode defines several named tables; an unprefixed
+    credential is shared across all of them (the #11 trap). Without a TOML parser
+    on the 3.10 baseline, this counts top-level ``[table]`` and ``[[array]]``
+    headers (ignoring comments and dotted sub-keys) as a conservative proxy for
+    "how many profiles are defined". Heuristic — see #91 notes; the exact ssh-mcp
+    schema should be confirmed before this drives anything destructive.
+    """
+    seen: set[str] = set()
+    count = 0
+    for raw in (text or "").splitlines():
+        line = raw.strip()
+        if not line or line.startswith("#"):
+            continue
+        m = re.match(r"\[\[?\s*([^\]]+?)\s*\]\]?", line)
+        if not m:
+            continue
+        # Top-level section name (first dotted component), deduped so repeated
+        # sub-tables of one profile don't inflate the count.
+        top = m.group(1).split(".", 1)[0].strip().strip("'\"")
+        if top and top not in seen:
+            seen.add(top)
+            count += 1
+    return count
+
+
+def unscoped_credential_warning(data: dict, profile_count: int | None = None) -> str | None:
+    """Advisory when a bare credential is shared across multiple TOML profiles (#11).
+
+    ssh-mcp offers an unprefixed ``SSH_MCP_PASSWORD`` to every profile, so a
+    single stored password is silently handed to every host defined in a
+    multi-profile sidecar. Fires only when a bare credential is set AND the
+    sidecar defines 2+ profiles. ``profile_count`` is injected by the aggregator
+    (which reads the file); None means "unknown", so nothing is claimed.
+    """
+    spec = resolve_server_spec(data)
+    if spec is None or profile_count is None or profile_count < 2:
+        return None
+    env = data.get("env") or {}
+    bare = [k for k in ("SSH_MCP_PASSWORD", "SSH_MCP_SUDO_PASSWORD") if k in env]
+    if not bare:
+        return None
+    names = ", ".join(sorted(bare))
+    return (
+        f"{spec.package}: {names} is unprefixed, so it is offered to every one of "
+        f"the {profile_count} profiles in the sidecar. Scope the credential "
+        f"per-profile so one host's password isn't shared with the others."
+    )
+
+
+def sidecar_warnings(
+    data: dict,
+    *,
+    platform: str | None = None,
+    environ: dict | None = None,
+    home: str | os.PathLike | None = None,
+    exists=None,
+    read_text=None,
+) -> list[str]:
+    """Human-readable sidecar advisories for a server. Empty list == nothing to flag.
+
+    Mirrors ``removed_flag_warnings``' shape so the GUI wiring is identical. Read-only:
+    reports precedence (args inert), the wrong-path trap, and the #11 unscoped-credential
+    trap. ``read_text`` is an injectable ``Path -> str`` for the sidecar body (used only
+    for the profile count); it defaults to a safe real read that degrades to "".
+    """
+    status = sidecar_status(data, platform=platform, environ=environ, home=home, exists=exists)
+    if status is None:
+        return []
+    out: list[str] = []
+    if status["args_inert"]:
+        out.append(
+            f"{status['package']}: these arguments are currently inert — this server "
+            f"reads its config from {status['path']}, which already exists, and only "
+            f"falls back to CLI args when that file is absent. Edit the file instead."
+        )
+    if status["wrong_path"]:
+        out.append(
+            f"{status['package']}: a config file exists at {status['doc_path']} (the "
+            f"path the README documents) but this server actually reads "
+            f"{status['path']} — the file you wrote is never loaded. Move it there."
+        )
+    # #11: read the sidecar (if present) to count profiles for the credential check.
+    if status["exists"]:
+        if read_text is None:
+
+            def read_text(p):
+                try:
+                    return Path(p).read_text(encoding="utf-8", errors="replace")
+                except OSError:
+                    return ""
+
+        cred = unscoped_credential_warning(
+            data, profile_count=count_toml_profiles(read_text(status["path"]))
+        )
+        if cred:
+            out.append(cred)
+    return out
+
+
 # --------------------------------------------------------------------------- #
 # Validation
 # --------------------------------------------------------------------------- #