From 2fafe4058140aa5b3fa383a78cf5eee560984c9f Mon Sep 17 00:00:00 2001 From: raymondginger Date: Wed, 23 Sep 2026 20:26:45 +0800 Subject: [PATCH] feat(mcp): name per-server concerns and default posture in `mcp list` `mcp list` showed configuration/auth/runtime state and left the operator to cross-reference `--json` for anything that should give them pause. CONCERNS states only what is explicitly configured or actually broken: `disabled`, `missing-env`, `sends-credential`. The two permissive readings that hold for every server which never made the choice -- an absent `enabledTools` resolves to every tool (core/config.py substitutes `["*"]`), and `auto`/`approve` add no MCP approval gate (core/harness/permissions.py) -- are stated once in a footer line instead of repeated on every row. Measured on a six-server config, the per-row form lit up 4 of 5 enabled rows with the same two defaults, which is how an operator learns to ignore the column. Only key names are read, never credential values. The NAME column is now width-aware, so a long server name cannot skew the table. Rows whose configuration did not parse stay out of the footer: the posture fields on those rows are defaults filled in by the invalid-row builder in mcp_service.py, not choices anyone made, so counting them would report a posture that was never configured. Rendered in cli/mcp_cli.py only: every reading is a pure function of fields that already exist on the wire, so McpServerInfo keeps its 33 fields and the desktop fixtures are untouched. Tests assert the rendered stdout -- the table cells and the footer counts -- so re-adding a default-triggered label to every row fails loudly, and so does counting an unparsed row. --- cli/mcp_cli.py | 75 +++++++++++++++++++++++++++++++++++++++++-- tests/test_mcp_cli.py | 72 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 2 deletions(-) diff --git a/cli/mcp_cli.py b/cli/mcp_cli.py index 94c9a4bf5..1d0f887a7 100644 --- a/cli/mcp_cli.py +++ b/cli/mcp_cli.py @@ -335,17 +335,88 @@ def _pairs(values: list[str], *, option: str) -> dict[str, str]: return result +#: Operator-facing labels for the CONCERNS column, in the order they are emitted. +_MCP_CONCERN_LABELS = { + "off": "disabled", + "env": "missing-env", + "cred": "sends-credential", +} + + +def _mcp_concerns(server: dict[str, Any]) -> tuple[str, ...]: + """Posture facts an operator should look at before trusting a server. + + Only facts about explicit configuration or actual breakage are reported. + The remaining two readings -- an absent ``enabledTools`` (every tool + exposed) and an ``approvalMode`` that adds no gate -- are the documented + defaults, so they hold for every server that never made the choice; + :func:`_posture_summary` states them once instead of repeating them on + every row. Only key *names* are ever read, never credential values (see + ``core.config._resolve_env_refs``). + """ + concerns: list[str] = [] + if not server["enabled"]: + concerns.append("off") + if server["missingEnvKeys"]: + concerns.append("env") + if server["credentialEnvKeys"]: + concerns.append("cred") + return tuple(concerns) + + +def _posture_summary(servers: list[dict[str, Any]]) -> str | None: + """Name how many enabled servers still sit on the permissive defaults. + + A disabled server exposes nothing and is left out of both counts, and so is + one whose configuration could not be parsed: ``_invalid_server_info`` fills + such a row's ``enabledTools``/``approvalMode`` with defaults nobody chose, so + counting them would state a posture that was never configured. An empty + ``enabledTools`` resolves to every tool (``core.config`` substitutes + ``["*"]``), and ``auto``/``approve`` both leave the global permission + decision untouched (``core.harness.permissions``); those are the defaults + an operator inherits by not choosing. + """ + readable = [ + server + for server in servers + if server["enabled"] and server["configurationState"] != "invalid" + ] + if not readable: + return None + all_tools = sum(1 for server in readable if not server["enabledTools"]) + no_gate = sum( + 1 for server in readable if server["approvalMode"] in {"auto", "approve"} + ) + if not all_tools and not no_gate: + return None + return ( + f"Default posture among {len(readable)} enabled servers: " + f"{all_tools} expose all tools, {no_gate} add no MCP approval gate." + ) + + def _print_inventory(result: dict[str, Any]) -> None: servers = result["servers"] if not servers: print("No MCP client servers configured for this workspace.") return - print(f"{'CONFIG':<20} {'AUTH':<16} {'RUNTIME':<12} {'SCOPE':<9} NAME") + name_width = min(max(len(server["name"]) for server in servers), 32) + print( + f"{'CONFIG':<20} {'AUTH':<16} {'RUNTIME':<12} {'SCOPE':<9} " + f"{'NAME':<{name_width}} CONCERNS" + ) for server in servers: + concerns = ", ".join( + _MCP_CONCERN_LABELS[concern] for concern in _mcp_concerns(server) + ) print( f"{server['configurationState']:<20} {server['authState']:<16} " - f"{server['runtimeState']:<12} {server['source']:<9} {server['name']}" + f"{server['runtimeState']:<12} {server['source']:<9} " + f"{server['name']:<{name_width}} {concerns or '-'}" ) + summary = _posture_summary(servers) + if summary is not None: + print(summary) def _print_presets(result: dict[str, Any]) -> None: diff --git a/tests/test_mcp_cli.py b/tests/test_mcp_cli.py index 4894716d1..65d023ed5 100644 --- a/tests/test_mcp_cli.py +++ b/tests/test_mcp_cli.py @@ -200,3 +200,75 @@ def test_mcp_cli_returns_failure_for_a_failed_real_probe( assert run(["test", "broken", "--json"]) == 1 assert _json_output(capsys)["ok"] is False + + +def test_mcp_list_names_concerns_and_states_default_posture_once( + tmp_path: Path, + monkeypatch, + capsys, +) -> None: + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("DEEPCODE_HOME", str(home)) + monkeypatch.delenv("DEEPCODE_TEST_UNSET_TOKEN", raising=False) + (home / "deepcode_config.json").write_text( + json.dumps( + { + "mcpServers": { + "plain": { + "type": "stdio", + "command": "python3", + "args": ["plain.py"], + }, + "scoped": { + "type": "stdio", + "command": "python3", + "args": ["scoped.py"], + "enabledTools": ["search"], + "approvalMode": "prompt", + }, + "credentialed": { + "type": "stdio", + "command": "python3", + "args": ["cred.py"], + "credentialEnv": { + "DEMO_API_KEY": {"credentialRef": "provider:openrouter"} + }, + "requiredEnvVars": ["DEEPCODE_TEST_UNSET_TOKEN"], + }, + "retired": { + "type": "stdio", + "command": "python3", + "args": ["old.py"], + "enabled": False, + }, + # ``type`` is required, so this row never parses. + "broken": {"command": "python3", "args": ["broken.py"]}, + } + } + ), + encoding="utf-8", + ) + + assert run(["list"]) == 0 + header, *rows, summary = capsys.readouterr().out.splitlines() + assert header.split() == ["CONFIG", "AUTH", "RUNTIME", "SCOPE", "NAME", "CONCERNS"] + concerns: dict[str, str] = {} + for row in rows: + fields = row.split(maxsplit=5) + concerns[fields[4]] = fields[5] + assert concerns == { + "broken": "-", + "credentialed": "missing-env, sends-credential", + "plain": "-", + "retired": "disabled", + "scoped": "-", + } + # ``plain`` inherits both permissive defaults while ``scoped`` opts out of + # both, so neither adds a per-row label and the defaults are counted once. + # ``broken`` never parsed: the defaults on its row were never configured, so + # it stays out of the counts. + assert summary == ( + "Default posture among 3 enabled servers: " + "2 expose all tools, 2 add no MCP approval gate." + )