feat(#88): detect & migrate removed CLI flags into env (ssh-mcp v2)
CI / Lint (ruff) (pull_request) Successful in 11s
CI / Tests (py3.12 / windows-latest) (pull_request) Successful in 38s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 12s
CI / Catalog signature (pull_request) Successful in 8s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 12s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 12s
CI / Lint (ruff) (pull_request) Successful in 11s
CI / Tests (py3.12 / windows-latest) (pull_request) Successful in 38s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 12s
CI / Catalog signature (pull_request) Successful in 8s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 12s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 12s
ssh-mcp v2 removed --password from the command line and reads
SSH_MCP_PASSWORD instead, so an old config crashes on startup. Add a
data-driven FLAG_ENV_MIGRATIONS registry plus detect_migratable_package,
migrate_removed_flags and removed_flag_warnings in bcc_core, and a
'Fix: move to environment variables' one-click action + warning in the
stdio server editor, with a matching main-window lint line. The literal
value lands in env{} (the only form Claude Desktop honours). 14 tests.
This commit is contained in:
@@ -602,6 +602,19 @@ class ServerEditor(QFrame):
|
||||
self.secret_warn.setWordWrap(True)
|
||||
self.secret_warn.hide()
|
||||
v.addWidget(self.secret_warn)
|
||||
# Shown when args carry a CLI flag a package removed in a major version
|
||||
# (e.g. ssh-mcp v2's --password), with a one-click move into env.
|
||||
self.removed_flag_warn = QLabel("")
|
||||
self.removed_flag_warn.setStyleSheet(f"color: {WARN};")
|
||||
self.removed_flag_warn.setWordWrap(True)
|
||||
self.removed_flag_warn.hide()
|
||||
self.removed_flag_fix_btn = QPushButton("Fix: move to environment variables")
|
||||
self.removed_flag_fix_btn.clicked.connect(self._fix_removed_flags)
|
||||
self.removed_flag_fix_btn.hide()
|
||||
rf_row = QHBoxLayout()
|
||||
rf_row.addWidget(self.removed_flag_warn, 1)
|
||||
rf_row.addWidget(self.removed_flag_fix_btn)
|
||||
v.addLayout(rf_row)
|
||||
v.addWidget(self._lbl("Environment variables"))
|
||||
self.env = KeyValueTable(
|
||||
"Variable", "Value", on_change=self._emit, before_change=self._before_change
|
||||
@@ -649,6 +662,8 @@ class ServerEditor(QFrame):
|
||||
self.args_warn.hide()
|
||||
self.args_fix_btn.hide()
|
||||
self.secret_warn.hide()
|
||||
self.removed_flag_warn.hide()
|
||||
self.removed_flag_fix_btn.hide()
|
||||
self._loading = False
|
||||
return
|
||||
self.setEnabled(True)
|
||||
@@ -736,6 +751,8 @@ class ServerEditor(QFrame):
|
||||
self.args_warn.hide()
|
||||
self.args_fix_btn.hide()
|
||||
self.secret_warn.hide()
|
||||
self.removed_flag_warn.hide()
|
||||
self.removed_flag_fix_btn.hide()
|
||||
return
|
||||
_, notes = core.split_suspicious_args(self._current_arg_lines())
|
||||
if notes:
|
||||
@@ -751,6 +768,23 @@ class ServerEditor(QFrame):
|
||||
self.secret_warn.show()
|
||||
else:
|
||||
self.secret_warn.hide()
|
||||
# Flags a package removed across a major version (e.g. ssh-mcp v2's
|
||||
# --password). Needs command + env too, so build from the live form.
|
||||
stdio = {
|
||||
"command": self.command.text().strip(),
|
||||
"args": self._current_arg_lines(),
|
||||
"env": self.env.dump(),
|
||||
}
|
||||
rf_warnings = core.removed_flag_warnings(stdio)
|
||||
if rf_warnings:
|
||||
self.removed_flag_warn.setText("⚠ " + "\n".join(rf_warnings))
|
||||
self.removed_flag_warn.show()
|
||||
# Only offer the button when something is actually auto-migratable.
|
||||
migrated, _ = core.migrate_removed_flags(stdio)
|
||||
self.removed_flag_fix_btn.setVisible(migrated is not stdio)
|
||||
else:
|
||||
self.removed_flag_warn.hide()
|
||||
self.removed_flag_fix_btn.hide()
|
||||
|
||||
def _fix_args(self):
|
||||
if self._before_change:
|
||||
@@ -758,6 +792,19 @@ class ServerEditor(QFrame):
|
||||
fixed, _ = core.split_suspicious_args(self._current_arg_lines())
|
||||
self.args.setPlainText("\n".join(fixed)) # triggers _emit -> recheck
|
||||
|
||||
def _fix_removed_flags(self):
|
||||
if self._before_change:
|
||||
self._before_change()
|
||||
stdio = {
|
||||
"command": self.command.text().strip(),
|
||||
"args": self._current_arg_lines(),
|
||||
"env": self.env.dump(),
|
||||
}
|
||||
migrated, _ = core.migrate_removed_flags(stdio)
|
||||
# Reload env first, then args; setting args text triggers _emit -> recheck.
|
||||
self.env.load(migrated.get("env", {}))
|
||||
self.args.setPlainText("\n".join(migrated.get("args", [])))
|
||||
|
||||
# --- dependency ------------------------------------------------------ #
|
||||
def refresh_dependency(self, auto_open=False):
|
||||
if not self.isEnabled():
|
||||
@@ -2633,6 +2680,8 @@ class MainWindow(QMainWindow):
|
||||
for entry in self.servers:
|
||||
for warning in core.env_ref_warnings(entry.data, self.current_profile):
|
||||
lint_warnings.append(f"'{entry.name}': {warning}")
|
||||
for warning in core.removed_flag_warnings(entry.data):
|
||||
lint_warnings.append(f"'{entry.name}': {warning}")
|
||||
if lint_warnings:
|
||||
self.validation_lbl.setText(f"⚠ {lint_warnings[0]}")
|
||||
self.validation_lbl.setStyleSheet(f"color: {WARN};")
|
||||
|
||||
+199
@@ -1964,6 +1964,205 @@ def _looks_like_multiple_args(a: str) -> bool:
|
||||
return len(toks) > 1 and any(t.startswith("-") for t in toks)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Removed-flag → env migration
|
||||
#
|
||||
# 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`.
|
||||
#
|
||||
# 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.
|
||||
# --------------------------------------------------------------------------- #
|
||||
FLAG_ENV_MIGRATIONS: dict[str, dict[str, dict[str, str]]] = {
|
||||
"ssh-mcp": {
|
||||
"env": {
|
||||
"--password": "SSH_MCP_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": (
|
||||
"removed in ssh-mcp v2 — sudo is now governed by the server "
|
||||
"config/policy rather than a CLI flag"
|
||||
),
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def _normalize_pkg_token(token: str) -> str:
|
||||
"""Reduce an argv token to a bare npm package name for registry lookup.
|
||||
|
||||
Strips any leading path (``/usr/local/bin/ssh-mcp`` -> ``ssh-mcp``) and a
|
||||
trailing ``@version`` (``ssh-mcp@2.0.1`` -> ``ssh-mcp``), while preserving a
|
||||
leading scope (``@acme/ssh-mcp`` stays intact).
|
||||
"""
|
||||
t = str(token or "").strip()
|
||||
if not t or t.startswith("-"):
|
||||
return ""
|
||||
# Drop a path prefix but keep an npm scope (leading '@' with no earlier '/').
|
||||
if "/" in t and not t.startswith("@"):
|
||||
t = t.rsplit("/", 1)[-1]
|
||||
# Strip a trailing @version. For scoped names, only the version '@' counts:
|
||||
# split on the LAST '@' when it isn't the scope's leading one.
|
||||
at = t.rfind("@")
|
||||
if at > 0: # >0 so a scope's leading '@' at index 0 is untouched
|
||||
t = t[:at]
|
||||
return t
|
||||
|
||||
|
||||
def detect_migratable_package(data: dict) -> str | None:
|
||||
"""Return the FLAG_ENV_MIGRATIONS key 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.
|
||||
"""
|
||||
if not isinstance(data, dict):
|
||||
return None
|
||||
tokens = [data.get("command", "")]
|
||||
tokens.extend(data.get("args") or [])
|
||||
for tok in tokens:
|
||||
name = _normalize_pkg_token(tok)
|
||||
if name in FLAG_ENV_MIGRATIONS:
|
||||
return name
|
||||
return None
|
||||
|
||||
|
||||
def migrate_removed_flags(data: dict) -> tuple[dict, list[str]]:
|
||||
"""Move known removed credential flags out of `args` and into `env`.
|
||||
|
||||
Returns ``(new_data, notes)``. When there is nothing to migrate the original
|
||||
dict is returned unchanged with an empty notes list, so callers can cheaply
|
||||
treat an empty notes list as "no change".
|
||||
|
||||
Only flags in the package's ``env`` map are rewritten. Both spellings are
|
||||
handled: ``--password secret`` (value on the next token) and
|
||||
``--password=secret`` (inline). An existing ``env`` value for the target
|
||||
name is never overwritten — the redundant flag is dropped and noted instead.
|
||||
A ``${VAR}`` reference is migrated verbatim (env is the right home for it).
|
||||
Flags listed under ``removed`` are left untouched here; see
|
||||
``removed_flag_warnings`` for those.
|
||||
"""
|
||||
pkg = detect_migratable_package(data)
|
||||
if pkg is None:
|
||||
return data, []
|
||||
env_map = FLAG_ENV_MIGRATIONS[pkg]["env"]
|
||||
args = [str(a) for a in (data.get("args") or [])]
|
||||
if not args:
|
||||
return data, []
|
||||
|
||||
new_args: list[str] = []
|
||||
new_env: dict = dict(data.get("env") or {})
|
||||
notes: list[str] = []
|
||||
i = 0
|
||||
n = len(args)
|
||||
changed = False
|
||||
while i < n:
|
||||
a = args[i]
|
||||
# Inline form: --flag=value
|
||||
if a.startswith("-") and "=" in a and a.split("=", 1)[0] in env_map:
|
||||
flag, value = a.split("=", 1)
|
||||
env_name = env_map[flag]
|
||||
changed = True
|
||||
if env_name in new_env and new_env[env_name] != value:
|
||||
notes.append(f"{flag} dropped from args ({env_name} is already set in env)")
|
||||
else:
|
||||
new_env[env_name] = value
|
||||
notes.append(f"moved {flag} into env as {env_name}")
|
||||
i += 1
|
||||
continue
|
||||
# Separate form: --flag value
|
||||
if a in env_map:
|
||||
flag = a
|
||||
env_name = env_map[flag]
|
||||
has_value = i + 1 < n and not args[i + 1].startswith("-")
|
||||
if not has_value:
|
||||
# No value to move (unexpected for a credential flag). Drop the
|
||||
# bare flag so the server can start, and say so.
|
||||
changed = True
|
||||
notes.append(f"removed {flag} from args (no value found to move)")
|
||||
i += 1
|
||||
continue
|
||||
value = args[i + 1]
|
||||
changed = True
|
||||
if env_name in new_env and new_env[env_name] != value:
|
||||
notes.append(f"{flag} dropped from args ({env_name} is already set in env)")
|
||||
else:
|
||||
new_env[env_name] = value
|
||||
notes.append(f"moved {flag} into env as {env_name}")
|
||||
i += 2
|
||||
continue
|
||||
new_args.append(a)
|
||||
i += 1
|
||||
|
||||
if not changed:
|
||||
return data, []
|
||||
|
||||
new_data = dict(data)
|
||||
if new_args:
|
||||
new_data["args"] = new_args
|
||||
else:
|
||||
new_data.pop("args", None)
|
||||
if new_env:
|
||||
new_data["env"] = new_env
|
||||
return new_data, notes
|
||||
|
||||
|
||||
def removed_flag_warnings(data: dict) -> list[str]:
|
||||
"""Advisory lines for every removed-in-a-major-version flag still present.
|
||||
|
||||
Covers both the auto-migratable flags (reported as a one-click fix being
|
||||
available) and the manual-only ones. Empty list == nothing to flag.
|
||||
"""
|
||||
pkg = detect_migratable_package(data)
|
||||
if pkg is None:
|
||||
return []
|
||||
spec = FLAG_ENV_MIGRATIONS[pkg]
|
||||
env_map = spec["env"]
|
||||
removed_map = spec.get("removed", {})
|
||||
present = set()
|
||||
for a in data.get("args") or []:
|
||||
s = str(a)
|
||||
present.add(s.split("=", 1)[0] if s.startswith("-") and "=" in s else s)
|
||||
|
||||
out: list[str] = []
|
||||
migratable = [f for f in env_map if f in present]
|
||||
if migratable:
|
||||
flags = ", ".join(sorted(migratable))
|
||||
out.append(
|
||||
f"{pkg}: {flags} was removed from the command line — its value should "
|
||||
f"live in an environment variable. Use “Fix” to move it into env."
|
||||
)
|
||||
for flag in sorted(removed_map):
|
||||
if flag in present:
|
||||
out.append(f"{pkg}: {flag} {removed_map[flag]}")
|
||||
return out
|
||||
|
||||
|
||||
def removed_flag_warning(data: dict) -> str | None:
|
||||
"""First removed-flag advisory for a single-line UI label, or None."""
|
||||
warnings = removed_flag_warnings(data)
|
||||
return warnings[0] if warnings else None
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Validation
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
@@ -3059,3 +3059,150 @@ def test_discover_project_configs_skips_non_object_and_garbage(tmp_path):
|
||||
assert str(array / ".mcp.json") not in paths
|
||||
assert str(garbage / ".mcp.json") not in paths
|
||||
assert str(missing / ".mcp.json") not in paths
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Removed-flag -> env migration (ssh-mcp v2 and the general mechanism)
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_detect_migratable_package_via_npx_args():
|
||||
data = {"command": "npx", "args": ["-y", "ssh-mcp", "--host=h", "--password=p"]}
|
||||
assert c.detect_migratable_package(data) == "ssh-mcp"
|
||||
|
||||
|
||||
def test_detect_migratable_package_direct_command_and_path_and_version():
|
||||
assert c.detect_migratable_package({"command": "ssh-mcp", "args": []}) == "ssh-mcp"
|
||||
assert (
|
||||
c.detect_migratable_package({"command": "/usr/local/bin/ssh-mcp", "args": []}) == "ssh-mcp"
|
||||
)
|
||||
assert (
|
||||
c.detect_migratable_package({"command": "npx", "args": ["-y", "ssh-mcp@2.0.1"]})
|
||||
== "ssh-mcp"
|
||||
)
|
||||
|
||||
|
||||
def test_detect_migratable_package_unknown_returns_none():
|
||||
assert c.detect_migratable_package({"command": "npx", "args": ["some-other"]}) is None
|
||||
assert c.detect_migratable_package({}) is None
|
||||
assert c.detect_migratable_package({"url": "https://x"}) is None
|
||||
|
||||
|
||||
def test_migrate_removed_flags_inline_form():
|
||||
data = {
|
||||
"command": "npx",
|
||||
"args": ["-y", "ssh-mcp", "--", "--host=1.2.3.4", "--password=hunter2"],
|
||||
}
|
||||
new, notes = c.migrate_removed_flags(data)
|
||||
assert new["args"] == ["-y", "ssh-mcp", "--", "--host=1.2.3.4"]
|
||||
assert new["env"] == {"SSH_MCP_PASSWORD": "hunter2"}
|
||||
assert any("SSH_MCP_PASSWORD" in n for n in notes)
|
||||
# Original untouched (pure function).
|
||||
assert "env" not in data
|
||||
|
||||
|
||||
def test_migrate_removed_flags_separate_form():
|
||||
data = {
|
||||
"command": "npx",
|
||||
"args": ["-y", "ssh-mcp", "--user", "root", "--password", "s3cret"],
|
||||
}
|
||||
new, _ = c.migrate_removed_flags(data)
|
||||
assert new["args"] == ["-y", "ssh-mcp", "--user", "root"]
|
||||
assert new["env"] == {"SSH_MCP_PASSWORD": "s3cret"}
|
||||
|
||||
|
||||
def test_migrate_removed_flags_preserves_existing_env_and_merges():
|
||||
data = {
|
||||
"command": "npx",
|
||||
"args": ["ssh-mcp", "--password=p"],
|
||||
"env": {"OTHER": "keep"},
|
||||
}
|
||||
new, _ = c.migrate_removed_flags(data)
|
||||
assert new["env"] == {"OTHER": "keep", "SSH_MCP_PASSWORD": "p"}
|
||||
|
||||
|
||||
def test_migrate_removed_flags_does_not_clobber_existing_secret():
|
||||
data = {
|
||||
"command": "npx",
|
||||
"args": ["ssh-mcp", "--password=fromargs"],
|
||||
"env": {"SSH_MCP_PASSWORD": "fromenv"},
|
||||
}
|
||||
new, notes = c.migrate_removed_flags(data)
|
||||
# env value wins; the redundant flag is still stripped so v2 can start.
|
||||
assert new["env"] == {"SSH_MCP_PASSWORD": "fromenv"}
|
||||
assert "--password=fromargs" not in new["args"]
|
||||
assert any("already set" in n for n in notes)
|
||||
|
||||
|
||||
def test_migrate_removed_flags_moves_env_ref_verbatim():
|
||||
data = {"command": "npx", "args": ["ssh-mcp", "--password", "${MY_PW}"]}
|
||||
new, _ = c.migrate_removed_flags(data)
|
||||
assert new["env"] == {"SSH_MCP_PASSWORD": "${MY_PW}"}
|
||||
assert "${MY_PW}" not in new["args"]
|
||||
|
||||
|
||||
def test_migrate_removed_flags_noop_returns_same_object():
|
||||
data = {"command": "npx", "args": ["ssh-mcp", "--host=h", "--user=u"]}
|
||||
new, notes = c.migrate_removed_flags(data)
|
||||
assert new is data
|
||||
assert notes == []
|
||||
|
||||
|
||||
def test_migrate_removed_flags_unknown_package_noop():
|
||||
data = {"command": "npx", "args": ["mystery", "--password=p"]}
|
||||
new, notes = c.migrate_removed_flags(data)
|
||||
assert new is data and notes == []
|
||||
|
||||
|
||||
def test_migrate_removed_flags_only_args_left_drops_args_key():
|
||||
data = {"command": "ssh-mcp", "args": ["--password=p"]}
|
||||
new, _ = c.migrate_removed_flags(data)
|
||||
assert "args" not in new
|
||||
assert new["env"] == {"SSH_MCP_PASSWORD": "p"}
|
||||
|
||||
|
||||
def test_removed_flag_warnings_covers_migratable_and_manual():
|
||||
data = {
|
||||
"command": "npx",
|
||||
"args": ["ssh-mcp", "--password=p", "--sudoPassword=s", "--disableSudo"],
|
||||
}
|
||||
warnings = c.removed_flag_warnings(data)
|
||||
text = " ".join(warnings)
|
||||
assert "--password" in text
|
||||
assert "sudoPassword" in text
|
||||
assert "disableSudo" in text
|
||||
# migrate only touches the confirmed --password mapping
|
||||
new, _ = c.migrate_removed_flags(data)
|
||||
assert new["env"] == {"SSH_MCP_PASSWORD": "p"}
|
||||
assert "--sudoPassword=s" in new["args"]
|
||||
assert "--disableSudo" in new["args"]
|
||||
|
||||
|
||||
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
|
||||
|
||||
|
||||
def test_ssh_membermatters_end_to_end():
|
||||
# The exact shape from AJ's failing server.
|
||||
data = {
|
||||
"command": "npx",
|
||||
"args": [
|
||||
"-y",
|
||||
"ssh-mcp",
|
||||
"--",
|
||||
"--host=member.example.io",
|
||||
"--user=deploy",
|
||||
"--port=22",
|
||||
"--password=topsecret",
|
||||
],
|
||||
}
|
||||
new, notes = c.migrate_removed_flags(data)
|
||||
assert new["args"] == [
|
||||
"-y",
|
||||
"ssh-mcp",
|
||||
"--",
|
||||
"--host=member.example.io",
|
||||
"--user=deploy",
|
||||
"--port=22",
|
||||
]
|
||||
assert new["env"] == {"SSH_MCP_PASSWORD": "topsecret"}
|
||||
assert notes
|
||||
|
||||
Reference in New Issue
Block a user