feat(validation): reject shell-string command in MCP stdio entries

[danielmeppiel]

Summary

Reject self-defined stdio MCP entries with whitespace-containing command and no args at parse time. Surfaces with a fix-it error pointing at the canonical shape.

Why

Surfaced via #122 (thanks @lirantal). Users coming from the universal mcp.json / Claude Desktop / Cursor mental model write:

- name: nodejs-api-docs
  registry: false
  transport: stdio
  command: "npx mcp-server-nodejs-api-docs"   # WRONG: shell line in `command`

and get confused when nothing works. APM never whitespace-split command; per schema, command is a single binary path and args is the list of arguments. Silently accepting the loose shape was a UX trap.

What changes

MCPDependency.validate() now raises ValueError when:

• registry: false, AND
• transport == "stdio", AND
• command is a string containing whitespace, AND
• args is empty/missing

Error message includes a fix-it that splits the offending command at the first whitespace boundary and shows the corrected command: / args: shape.

Self-defined MCP dependency 'nodejs-api-docs': 'command' must be a single binary
path, not a shell line. APM does not split 'command' on whitespace.
Got: command='npx -y mcp-server-nodejs-api-docs'.
Did you mean: command: npx, args: ["-y", "mcp-server-nodejs-api-docs"] ?
See https://microsoft.github.io/apm/guides/mcp-servers/ for the canonical stdio shape.

Why error, not warn

Per maintainer steer (issue #806 thread): "move fast, breaking changes OK if they make sense, adoption base small enough." Panel ratified:

• devx-ux: npm/cargo error on bad manifests; warn-then-mis-execute is worse UX.
• supply-chain-security: Silently passing a whitespace string to a runtime that doesn't split = broken or surprising. Fail closed.
• CEO: The loose shape was never specified; CHANGELOG (BREAKING) entry covers the comms.

If user feedback shows broader migration friction, downgrading to a warn is a one-line change.

Edge cases covered (tests)

• Canonical shape (command: npx, args: [...]) keeps working
• Whitespace command + empty args -> rejected with fix-it
• Whitespace command + explicit args -> accepted (paths with spaces, e.g. /opt/My App/server, are legal when author has owned shape)
• Tabs in command -> also rejected (covers \t, not just spaces)
• Error message includes the suggested command: and args: tokens

Tests

tests/unit/test_mcp_overlays.py: 46 passed
tests/unit tests/test_console.py: 4069 passed

CHANGELOG

Added under ## [Unreleased] / Changed (BREAKING).

Followups

• feat(cli): extend 'apm install --mcp' for declarative MCP server addition #807 -- apm install --mcp will use this validator path; entries built from -- boundary on the CLI cannot trigger this error by construction
• docs: consolidate MCP coverage into a single canonical guide #808 -- new guides/mcp-servers.md will be the canonical home of the URL referenced in the error message; the link is already correct (URL pattern matches the existing site routing)

Closes #806
Refs #122

[Copilot]

Pull request overview

Adds stricter parse-time validation for self-defined stdio MCP dependencies to prevent the common “shell string in command” misconfiguration, and also updates package-type detection/observability to address plugin-vs-hooks classification edge cases.

Changes:

• Reject self-defined stdio MCP entries where command contains whitespace and args is missing/empty, with a fix-it suggestion in the error.
• Introduce richer package-type detection evidence and reorder the detection cascade to classify plugin-shaped packages before hook-only packages.
• Add/extend unit tests and update CHANGELOG.md entries for the behavior changes.

Show a summary per file

File	Description
src/apm_cli/models/dependency/mcp.py	Adds the new stdio shell-string command validation and fix-it error text.
tests/unit/test_mcp_overlays.py	Adds unit tests covering the new MCP validation behavior and error contents.
src/apm_cli/models/validation.py	Adds DetectionEvidence + gather_detection_evidence() and changes detection order to prefer plugin evidence over hooks.
tests/test_apm_package_models.py	Adds regression tests for plugin-vs-hooks classification and new evidence-gathering helper.
src/apm_cli/install/sources.py	Centralizes package-type label formatting and ensures hook packages get a label.
tests/unit/install/test_sources_classification.py	New tests ensuring every classifiable PackageType has a human-readable label.
CHANGELOG.md	Adds entries describing the new MCP validation breaking change and the plugin classification fix.

Copilot's findings

Comments suppressed due to low confidence (1)

src/apm_cli/models/dependency/mcp.py:142

• This validation rejects any whitespace in command when args is empty, but per the schema command is a binary path and valid paths can contain spaces (notably on Windows, e.g. under Program Files). Consider narrowing the check to the specific "shell line" anti-pattern (e.g. multiple tokens and command doesn't look like a path) so you don't block legitimate single-binary paths that contain spaces but have no arguments.

            if (
                self.transport == 'stdio'
                and isinstance(self.command, str)
                and any(ch.isspace() for ch in self.command)
                and not self.args
            ):

• Files reviewed: 7/7 changed files
• Comments generated: 7

[Copilot]

This changelog entry ends with (#122), but #122 is an issue (already referenced via Closes #806/Refs #122) and cannot also be the PR number. Per the repo changelog convention, the trailing parenthetical should be the PR number for this change.

[Copilot]

This entry appears to be a normal code change but does not end with a PR number, and (#780) reads like an issue reference rather than the required (#PR_NUMBER) suffix. Please adjust to Keep-a-Changelog format: one line per PR, ending with this PR's number (issue refs can stay in the sentence body).

[Copilot]

This PR's title/description are focused on MCP stdio validation (#806/#122), but this diff also adds new package-type detection behavior (gather_detection_evidence, .claude-plugin evidence, and a reordered detection cascade) tied to #780. If this extra scope is intentional, it should be called out explicitly in the PR description; otherwise consider splitting to keep review/rollback risk isolated.

[Copilot]

The fix-it suggestion splits command using .partition(' '), which only handles a literal space. For inputs containing other whitespace (e.g. tabs, multiple whitespace), the validation still triggers but the suggested command/args shape will be incorrect (e.g. npx\t-y\tpkg produces command: npx\t-y\tpkg, args: []). Split on any whitespace with a single pass (e.g. split(maxsplit=1) / regex \s+) so the suggestion matches the validation condition (isspace()).

This issue also appears on line 137 of the same file.

Suggested change

	first, _, rest = self.command.strip().partition(' ')
	command_parts = self.command.strip().split(maxsplit=1)
	first = command_parts[0] if command_parts else ''
	rest = command_parts[1] if len(command_parts) > 1 else ''

[Copilot]

The error message links to https://microsoft.github.io/apm/guides/mcp-servers/, but there is no mcp-servers guide in the current docs tree, so users will hit a 404 from the fix-it error. Either add the page in this PR or point to an existing canonical section (e.g. the current MCP dependency docs) until the new guide lands.

Suggested change

	f"See https://microsoft.github.io/apm/guides/mcp-servers/ "
	f"See https://microsoft.github.io/apm/reference/dependencies/ "

[Copilot]

These tests mix src.apm_cli... and apm_cli... imports for the same module. That can import the same source file twice under different module names, leading to duplicated class/enum singletons (e.g. PackageType identity mismatches) and flaky isinstance / equality behavior. Please standardize on a single import style within this file (preferably the existing src.apm_cli... used at the top).

[Copilot]

MARKETPLACE_PLUGIN classification now also triggers on a bare .claude-plugin/ directory (see detect_package_type), but this label only mentions plugin.json or agents/skills/commands. That makes the install-time classification log misleading for the manifest-less plugin case. Update the wording (or make it more generic) so it matches the actual detection signals.

[danielmeppiel]

Review: Approve with required changes

Strong change. Closes a real footgun (#806, refs #122) with the right call (error, not warn) and good test coverage. Two items must be addressed before merge; three should be addressed in this PR.

This synthesis comes from the APM Review Panel (Python Architect, CLI Logging Expert, DevX UX Expert, Supply-Chain Security Expert, OSS Growth Hacker), arbitrated by the CEO persona.

Must fix before merge

M1. Credential leak in error message (mcp.py line 225)

f"Got: command={self.command!r}" echoes the raw command verbatim. If a user writes command: "npx --token=SECRET mcp-server", the token appears in CLI stderr and CI scrollback. The class already redacts env and headers in __repr__ -- same caution must apply to error messages.

Fix: use the already-computed first variable:

f"Got: command={first!r} ({len(rest_tokens)} additional args). "

M2. not self.args rejects explicit args: [] (mcp.py line 212)

args: [] is a valid "no extra arguments" signal. not [] evaluates True, so the whitespace check fires incorrectly. This rejects valid input in a BREAKING change -- we cannot ship that.

Fix: change and not self.args to and self.args is None. Add a regression test asserting command: "/opt/My App/server" with args: [] validates cleanly.

Should fix in this PR

S1. Whitespace-only command produces broken fix-it (mcp.py lines 217-218)

command: " " produces Did you mean: command: , args: [] (nonsensical). After command_parts = self.command.strip().split(maxsplit=1), guard the empty case with a dedicated "command is empty/whitespace-only" error.

S2. Type gate for non-str command (mcp.py ~line 170)

YAML command: ["npx", "-y", "evil"] (list) bypasses the isinstance(self.command, str) guard silently, then crashes in validate_path_segments with an unhandled AttributeError. Add early in the universal hardening section:

if self.command is not None and not isinstance(self.command, str):
    raise ValueError(
        f"MCP dependency '{self.name}': 'command' must be a string, "
        f"got {type(self.command).__name__}"
    )

S3. Update manifest schema spec (docs/src/content/docs/reference/manifest-schema.md section 4.2.3)

Add rule 4 to the validation rules:

4. If transport is stdio, command MUST be a single binary path with no embedded whitespace. Use args for additional arguments.

Spec and code must ship together -- project convention.

Follow-up tickets (file after merge, do not block)

• fix(mcp): redact command in MCPDependency.__repr__ (credential leak surface) -- pre-existing in __repr__ line 121.
• fix(plugins): forward MCPDependency validation errors to DiagnosticCollector -- plugin-parser path uses stdlib logging.warning, invisible without --verbose.
• improve(mcp): use multi-line Cargo-style error format for validation messages -- the 350-char single-line ValueError defeats terminal URL detection.
• feat(mcp): warn on shell metacharacters in stdio command (defense-in-depth) -- command: "npx|curl..." (no whitespace) passes validation. No real risk (execve-style, no shell=True), but a warning would help.

Declined / out of scope

• Triple-nested error prefix in consumer re-raise (pre-existing wrapping pattern).
• Exit code 2 vs 1 divergence between UsageError and ValueError (pre-existing, broader concern).
• NBSP "shell line" wording (pathological input; wording is accurate enough).

---

Closes the #122 footgun with the right severity (error, not warn), good test matrix, and a clear CHANGELOG entry. The fix-it suggestion is genuinely helpful DX -- the kind of detail that earns trust. Address M1+M2+S1-S3 and this ships.

[danielmeppiel]

Follow-up tickets folded into this PR (commit 50858ee)

Per maintainer direction, all four CEO-classified follow-up items landed in this PR rather than being deferred to separate issues.

FU1 — MCPDependency.__repr__ credential leak (sec MEDIUM, pre-existing)

__repr__ echoed command={self.command!r} verbatim while carefully redacting env and headers. Now shows only the first whitespace-separated token, mirroring the M1 fix. Regression test: test_repr_redacts_command_to_avoid_leaking_credentials.

FU2 — Surface plugin-parser MCP validation warnings (cli-log IMPORTANT)

The apm stdlib logger has no handlers configured, so logger.warning calls in plugin_parser.py were silently dropped. Added _surface_warning helper that routes through both stdlib logger AND _rich_warning so invalid MCP servers from a plugin are visible without --verbose. Applied to both the validation-error catch site and the no-command/no-url skip.

FU3 — Multi-line Cargo-style error format (cli-log IMPORTANT / devx NIT)

The original 350-char single-line ValueError defeated terminal URL detection and the newspaper test. Restructured to the field/rule/got/fix/see pattern:

'command' contains whitespace in MCP dependency '<name>'.
  Rule: 'command' must be a single binary path -- APM does not split on whitespace. Use 'args' for additional arguments.
  Got:  command='npx' (2 additional args)
  Fix:  command: npx
        args: ["-y", "pkg"]
  See:  https://microsoft.github.io/apm/guides/mcp-servers/

URL now sits on its own line for click-through. Regression test: test_validate_stdio_error_uses_multiline_cargo_style_format.

FU4 — Shell-metachar warning for stdio command (sec LOW, defense-in-depth)

Extended _warn_shell_metachars(env, logger) to optionally check command as well, so command: "npx|curl evil.com" (no whitespace, passes the rejection guard) still triggers a warning that MCP stdio servers run via execve with no shell. Hooked into the --mcp install path via entry.get("command").

Architecture (python-architecture skill)

Adding the command-checking branch pushed commands/install.py over the 1525 LOC architectural invariant. Per the skill's guidance ("don't trim cosmetically — modularize"), extracted the F5 SSRF helper, F7 shell-metachar helper, _is_internal_or_metadata_host, _SHELL_METACHAR_TOKENS, and _METADATA_HOSTS into a new dedicated module: src/apm_cli/install/mcp_warnings.py. install.py back-binds the symbols at module scope so existing test patches against apm_cli.commands.install._warn_* keep working unchanged.

install.py: 1530 → 1441 LOC (84 under budget, room to breathe).

Verification

• 4715/4715 unit + console tests pass (excludes the known pre-existing test_user_scope_skips_workspace_runtimes failure on main, unrelated to this PR).
• Net PR diff vs main now: mcp.py +47, mcp_warnings.py +122 (new), plugin_parser.py +30, test_mcp_overlays.py +151, manifest-schema.md +1, install.py net unchanged (extracted code = imported back).