diff --git a/bcc_core.py b/bcc_core.py index b3234a8..23804ca 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -30,6 +30,7 @@ import tempfile import threading import time from dataclasses import dataclass +from dataclasses import field as _field from pathlib import Path from typing import NamedTuple from urllib.parse import urlparse @@ -2218,46 +2219,116 @@ def _looks_like_multiple_args(a: str) -> bool: # --------------------------------------------------------------------------- # -# Removed-flag → env migration +# ServerSpec — the server-package axis (issue #90, epic #94) # -# Some MCP servers moved credential CLI flags into environment variables across -# a major version (secrets on the command line are visible to any local user in -# process listings). A config written for the old version then fails hard on the -# new one — e.g. ssh-mcp v2 exits with "These flags were removed in v2: -# --password". This registry lets BCC recognise that shape, warn about it, and -# offer a one-click migration that lifts the value out of `args` into `env`. +# BCC already models *which host reads a config* (ClientSpec). ServerSpec is the +# orthogonal axis: *which server package a definition runs*. A given server has +# both — Claude Desktop (client) running ssh-mcp (server), say. #89 shipped the +# seed of this axis as FLAG_ENV_MIGRATIONS (a per-npm-package registry of removed +# credential flags); #90 promotes it into a richer struct the sidecar (#91), +# version-pin (#92), permission (#93) and future schema/auth work all hang off. # -# Per package: -# "env" removed flag -> env var that now supplies it. BCC auto-migrates -# these: the flag (and its value) leave `args`, the value lands in -# `env` under the mapped name. -# "removed" flags that no longer exist but have no confirmed 1:1 env -# replacement (e.g. a boolean, or an env name not documented here). -# BCC only warns; the user resolves these by hand. -# -# Extending it: add a package key and its flag maps. Keep "env" limited to -# mappings whose env-var name is verified, so the auto-fix stays trustworthy. +# Non-reversing constraint (epic #94): the migration LOGIC below +# (migrate_removed_flags / detect_migratable_package / removed_flag_warnings) is +# unchanged — only the DATA grows. FLAG_ENV_MIGRATIONS is kept as a derived view +# so every existing reader keeps seeing the same shape. # --------------------------------------------------------------------------- # -FLAG_ENV_MIGRATIONS: dict[str, dict[str, dict[str, str]]] = { - "ssh-mcp": { - "env": { +@dataclass(frozen=True) +class ServerSpec: + """Everything BCC knows about one MCP *server package*. + + Frozen so the module-level registry entries are effectively singletons. + Every collection field defaults empty, so a spec need only fill in the axes + that apply to its package. Fields: + + * ``env_flags`` — removed CLI flags whose value now belongs in an env var. + Auto-migratable: ``migrate_removed_flags`` lifts the flag's value out of + ``args`` into ``env`` under the mapped name. Two flags may map to one var + (ssh-mcp's ``--sudoPassword``/``--suPassword`` → ``SSH_MCP_SUDO_PASSWORD``); + the no-clobber logic already handles that collision. + * ``removed_flags`` — removed flags with no confirmed 1:1 env replacement + (e.g. a boolean policy toggle). Warn-only; the user resolves by hand. + * ``drift_flags`` — flags/values whose *meaning* changed across a major + version without being removed. Keyed by the exact ``flag=value`` token; the + value is the human explanation. Not auto-fixed — meaning drift needs a human. + * ``sidecar_paths`` — per-platform sidecar config location, VERIFIED from the + package source (not its docs). Keys are ``"darwin"``/``"win32"``/``"posix"``; + values are templates expanded against ``$HOME`` + environment (see #91). + Empty == the package has no sidecar. Consumed by #91/#93. + * ``sidecar_doc_path`` — a path the package's README documents but does NOT + actually read on some platform. Used by #91 to flag a file a user placed by + following the (wrong) docs, which the server silently never reads. + * ``schema`` — validation enums shipped by the package (zod). Seeded as data + now; becomes editor pick-lists later (#7). + """ + + package: str + env_flags: dict[str, str] = _field(default_factory=dict) + removed_flags: dict[str, str] = _field(default_factory=dict) + drift_flags: dict[str, str] = _field(default_factory=dict) + sidecar_paths: dict[str, str] = _field(default_factory=dict) + sidecar_doc_path: str = "" + schema: dict = _field(default_factory=dict) + + +# The registry. Keyed by the normalised npm package name (see _normalize_pkg_token). +# Verified ssh-mcp facts (from the package source, NOT its README — the README's +# sidecar path is wrong on macOS/Windows): +SERVER_SPECS: dict[str, ServerSpec] = { + "ssh-mcp": ServerSpec( + package="ssh-mcp", + # Credential flags removed in v2 — secrets on argv are visible to any + # local user in process listings, so v2 reads them from the environment. + env_flags={ "--password": "SSH_MCP_PASSWORD", + "--sudoPassword": "SSH_MCP_SUDO_PASSWORD", + "--suPassword": "SSH_MCP_SUDO_PASSWORD", }, - "removed": { - "--sudoPassword": ( - "removed in ssh-mcp v2 — supply it via an environment variable " - "instead (see the ssh-mcp README's 'Migrating from v1' section)" - ), - "--suPassword": ( - "removed in ssh-mcp v2 — supply it via an environment variable " - "instead (see the ssh-mcp README's 'Migrating from v1' section)" - ), + # --disableSudo is gone with no env replacement: sudo is now a role/policy. + removed_flags={ "--disableSudo": ( "removed in ssh-mcp v2 — sudo is now governed by the server " - "config/policy rather than a CLI flag" + "config/policy (a role/permission) rather than a CLI flag" ), }, - }, + # --maxChars=none changed meaning: v1 silently parsed "none" to a 5000-char + # cap (parseInt("none") || 5000), so a config still carrying it doesn't mean + # "unlimited" the way a user reading the flag name would assume. + drift_flags={ + "--maxChars=none": ( + "changed meaning across ssh-mcp versions — v1 silently parsed " + "'none' as a 5000-character cap (parseInt('none') || 5000), not " + '"unlimited". Confirm the value still means what you intend on the ' + "version you run." + ), + }, + # Sidecar TOML path, verified from the package code (see #91). The README + # documents ~/.config on every platform, but the code resolves the OS's + # native app-data dir on macOS/Windows. + sidecar_paths={ + "darwin": "~/Library/Application Support/ssh-mcp/config.toml", + "win32": "${APPDATA}/ssh-mcp/config.toml", + "posix": "${XDG_CONFIG_HOME:-~/.config}/ssh-mcp/config.toml", + }, + sidecar_doc_path="~/.config/ssh-mcp/config.toml", + # zod enums shipped by ssh-mcp v2 (data now; pick-lists later — #7). + schema={ + "auth": ["agent", "key", "password", "keychain"], + "approvalMode": ["auto", "ask-destructive", "ask-all", "deny"], + "role": ["viewer", "operator", "admin"], + "port": {"min": 1, "max": 65535}, + }, + ), +} + + +# Backwards-compatible derived view of the registry: {pkg: {"env": ..., "removed": ...}}. +# SERVER_SPECS is the source of truth; this alias preserves the exact shape #89 +# shipped so every existing reader of FLAG_ENV_MIGRATIONS (and the migration +# functions below) keeps working with no logic change. +FLAG_ENV_MIGRATIONS: dict[str, dict[str, dict[str, str]]] = { + pkg: {"env": dict(spec.env_flags), "removed": dict(spec.removed_flags)} + for pkg, spec in SERVER_SPECS.items() } @@ -2282,11 +2353,13 @@ def _normalize_pkg_token(token: str) -> str: return t -def detect_migratable_package(data: dict) -> str | None: - """Return the FLAG_ENV_MIGRATIONS key this stdio server runs, or None. +def resolve_server_spec(data: dict) -> ServerSpec | None: + """Return the ServerSpec for the package a stdio server runs, or None. Scans the command and every arg (so ``npx -y ssh-mcp`` and a direct ``command: ssh-mcp`` both resolve), matching on the normalised package name. + This is the ServerSpec-axis entry point; ``detect_migratable_package`` is a + thin name-only wrapper kept for the existing migration callers. """ if not isinstance(data, dict): return None @@ -2294,11 +2367,21 @@ def detect_migratable_package(data: dict) -> str | None: tokens.extend(data.get("args") or []) for tok in tokens: name = _normalize_pkg_token(tok) - if name in FLAG_ENV_MIGRATIONS: - return name + if name in SERVER_SPECS: + return SERVER_SPECS[name] return None +def detect_migratable_package(data: dict) -> str | None: + """Return the registry key for the package this stdio server runs, or None. + + Scans the command and every arg (so ``npx -y ssh-mcp`` and a direct + ``command: ssh-mcp`` both resolve), matching on the normalised package name. + """ + spec = resolve_server_spec(data) + return spec.package if spec else None + + def migrate_removed_flags(data: dict) -> tuple[dict, list[str]]: """Move known removed credential flags out of `args` and into `env`. @@ -2416,6 +2499,42 @@ def removed_flag_warning(data: dict) -> str | None: return warnings[0] if warnings else None +def drift_warnings(data: dict) -> list[str]: + """Advisory lines for flags whose *meaning* changed across a major version. + + Distinct from ``removed_flag_warnings``: these flags still exist and still + parse, so nothing errors — but the value no longer means what the user set + it to mean (e.g. ssh-mcp's ``--maxChars=none``, which v1 silently capped at + 5000 chars). There is no safe auto-fix: only a human knows the intent, so + this is warn-only. Matches both the inline ``--flag=value`` form and the + separate ``--flag value`` form. Empty list == nothing to flag. + """ + spec = resolve_server_spec(data) + if spec is None or not spec.drift_flags: + return [] + # Reconstruct the set of "flag=value" tokens present, collapsing the two argv + # spellings into one so a drift key of the form "--flag=value" matches both. + args = [str(a) for a in (data.get("args") or [])] + present: set[str] = set() + i, n = 0, len(args) + while i < n: + a = args[i] + if a.startswith("-") and "=" in a: + present.add(a) # inline: --flag=value + i += 1 + continue + if a.startswith("-") and i + 1 < n and not args[i + 1].startswith("-"): + present.add(f"{a}={args[i + 1]}") # separate: --flag value + i += 2 + continue + i += 1 + out: list[str] = [] + for token in sorted(spec.drift_flags): + if token in present: + out.append(f"{spec.package}: {token} {spec.drift_flags[token]}") + return out + + # --------------------------------------------------------------------------- # # Validation # --------------------------------------------------------------------------- # diff --git a/tests/test_core.py b/tests/test_core.py index c7e9e52..31d9b5f 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -3169,13 +3169,38 @@ def test_removed_flag_warnings_covers_migratable_and_manual(): assert "--password" in text assert "sudoPassword" in text assert "disableSudo" in text - # migrate only touches the confirmed --password mapping + # #90: --password AND --sudoPassword are now both auto-migratable (the + # verified ssh-mcp facts map sudo/su → SSH_MCP_SUDO_PASSWORD). The migration + # LOGIC is unchanged — only the registry data grew — so both move into env. + # --disableSudo has no env replacement (sudo is now a role/policy) → warn-only. new, _ = c.migrate_removed_flags(data) - assert new["env"] == {"SSH_MCP_PASSWORD": "p"} - assert "--sudoPassword=s" in new["args"] + assert new["env"] == {"SSH_MCP_PASSWORD": "p", "SSH_MCP_SUDO_PASSWORD": "s"} + assert "--sudoPassword=s" not in new["args"] assert "--disableSudo" in new["args"] +def test_migrate_su_password_maps_to_sudo_env(): + # --suPassword is the second flag that maps to the same SSH_MCP_SUDO_PASSWORD var. + data = {"command": "ssh-mcp", "args": ["--suPassword", "rootpw"]} + new, notes = c.migrate_removed_flags(data) + assert new["env"] == {"SSH_MCP_SUDO_PASSWORD": "rootpw"} + assert "args" not in new + assert notes + + +def test_migrate_sudo_and_su_two_flags_one_var_no_clobber(): + # Both sudo flags map to one var; the no-clobber path keeps the first, drops + # the second (different value) with a note rather than silently overwriting. + data = { + "command": "ssh-mcp", + "args": ["--sudoPassword=first", "--suPassword=second"], + } + new, notes = c.migrate_removed_flags(data) + assert new["env"] == {"SSH_MCP_SUDO_PASSWORD": "first"} + assert "args" not in new + assert any("already set" in n for n in notes) + + def test_removed_flag_warnings_empty_when_clean(): assert c.removed_flag_warnings({"command": "ssh-mcp", "args": ["--host=h"]}) == [] assert c.removed_flag_warning({"command": "ssh-mcp", "args": ["--host=h"]}) is None @@ -3208,6 +3233,70 @@ def test_ssh_membermatters_end_to_end(): assert notes +# --------------------------------------------------------------------------- # +# ServerSpec spine (issue #90) +# --------------------------------------------------------------------------- # +def test_server_spec_registry_seeds_ssh_mcp(): + spec = c.SERVER_SPECS["ssh-mcp"] + assert spec.package == "ssh-mcp" + # sudo/su both map to the one sudo env var (verified facts). + assert spec.env_flags["--sudoPassword"] == "SSH_MCP_SUDO_PASSWORD" + assert spec.env_flags["--suPassword"] == "SSH_MCP_SUDO_PASSWORD" + assert spec.env_flags["--password"] == "SSH_MCP_PASSWORD" + # disableSudo stays warn-only (no env replacement). + assert "--disableSudo" in spec.removed_flags + assert "--disableSudo" not in spec.env_flags + # Verified sidecar paths seeded per platform (README path differs → doc_path). + assert "ssh-mcp/config.toml" in spec.sidecar_paths["darwin"] + assert "Application Support" in spec.sidecar_paths["darwin"] + assert "APPDATA" in spec.sidecar_paths["win32"] + assert spec.sidecar_doc_path == "~/.config/ssh-mcp/config.toml" + # zod enums seeded as data (pick-lists later — #7). + assert spec.schema["auth"] == ["agent", "key", "password", "keychain"] + assert spec.schema["approvalMode"] == ["auto", "ask-destructive", "ask-all", "deny"] + assert spec.schema["role"] == ["viewer", "operator", "admin"] + assert spec.schema["port"] == {"min": 1, "max": 65535} + + +def test_flag_env_migrations_is_derived_from_server_specs(): + # The legacy constant is now a derived view; it must mirror the registry. + assert set(c.FLAG_ENV_MIGRATIONS) == set(c.SERVER_SPECS) + assert c.FLAG_ENV_MIGRATIONS["ssh-mcp"]["env"] == c.SERVER_SPECS["ssh-mcp"].env_flags + assert c.FLAG_ENV_MIGRATIONS["ssh-mcp"]["removed"] == c.SERVER_SPECS["ssh-mcp"].removed_flags + + +def test_resolve_server_spec_matches_detect_migratable_package(): + for data in ( + {"command": "npx", "args": ["-y", "ssh-mcp"]}, + {"command": "ssh-mcp", "args": []}, + {"command": "/usr/local/bin/ssh-mcp", "args": []}, + {"command": "npx", "args": ["some-other"]}, + {}, + {"url": "https://x"}, + ): + spec = c.resolve_server_spec(data) + pkg = c.detect_migratable_package(data) + assert (spec.package if spec else None) == pkg + + +def test_drift_warning_maxchars_none_inline_and_separate(): + inline = {"command": "ssh-mcp", "args": ["--maxChars=none"]} + separate = {"command": "ssh-mcp", "args": ["--maxChars", "none"]} + for data in (inline, separate): + warnings = c.drift_warnings(data) + assert len(warnings) == 1 + assert "--maxChars=none" in warnings[0] + assert "5000" in warnings[0] + + +def test_drift_warning_quiet_for_other_values_and_unknown_pkg(): + # A real numeric cap is fine; only "none" drifts. + assert c.drift_warnings({"command": "ssh-mcp", "args": ["--maxChars=8000"]}) == [] + assert c.drift_warnings({"command": "ssh-mcp", "args": ["--host=h"]}) == [] + # Unknown package: nothing to say. + assert c.drift_warnings({"command": "npx", "args": ["other", "--maxChars=none"]}) == [] + + # --------------------------------------------------------------------------- # # Move to environment variable (issue #83) # --------------------------------------------------------------------------- #