fix(install): handle bare-string MCP entry in shell-metachar warning

[edenfunf]

Description

apm install --mcp <registry-ref> without --transport, --url,
--mcp-version, --registry, or a post--- stdio command aborts with:

'str' object has no attribute 'get'

The pure builder _build_mcp_entry returns a bare str for the
documented bare-string registry shorthand path (preserving the
mcp: [foo] apm.yml UX contract), but the F7 shell-metachar warning
callsite in _run_mcp_install read entry.get("command") unguarded.
The exception fires before apm.yml is written, so the user is left
with no manifest update and only a workaround (--transport stdio).

Root cause

_build_mcp_entry returns str | dict by design — bare strings for
shorthand-with-no-overlays, dicts otherwise. Two later callsites in
_run_mcp_install already guard with isinstance(entry, dict); the
F7 warning callsite did not, and silently broke the bare-string
happy path.

Fix

Source the stdio command from command_argv (the CLI input that
populates entry["command"] in the dict-entry path) rather than
introspecting the post-build entry. The two values are identical in
the stdio path and both yield None elsewhere, so warning behaviour
is preserved bit-for-bit while the str-vs-dict dispatch disappears
from this callsite. The other isinstance-guarded callsites are
left untouched to keep the diff minimal.

Fixes #938

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

Manual verification

Path	Command	Result
Issue reproduce	apm install --mcp io.github.github/github-mcp-server	exit 0; bare-string entry persisted to apm.yml
Workaround (regression check)	... --transport stdio --dry-run	exit 0; dry-run preview
Self-defined stdio dict path	apm install --mcp myfetch -- npx -y @modelcontextprotocol/server-fetch	exit 0; dict entry persisted; integrators configured
F7 warning still fires	apm install --mcp evil --force -- 'echo;rm' arg	shell-metachar warning emitted with the correct command value

Automated tests

• New regression test test_mcp_registry_shorthand_no_overlays_persists_bare_string
  in tests/unit/test_install_command.py reproduces the issue path and
  asserts exit_code == 0 plus bare-string serialization.
• Verified the test fails on the unpatched code with the exact
  'str' object has no attribute 'get' message and passes after the fix.
• Full test_install_command.py (100 tests) and the broader MCP
  builder / overlay / yml-writer suites (137 tests) remain green.

[github-actions]

APM Review Panel Verdict

Disposition: APPROVE (with one lightweight pre-merge required action: add CHANGELOG entry)

---

Per-persona findings

Python Architect: This PR is purely procedural -- _run_mcp_install is a module-level function, not a class method. No new classes are introduced. The root cause is that _build_mcp_entry returns str | dict by design, and three of four downstream callsites already guard with isinstance(entry, dict). The F7 warning callsite was the only unguarded one. The fix avoids a fourth isinstance guard by sourcing command from the canonical upstream input (command_argv) whose value is semantically identical to entry["command"] in the dict path, and None for all non-stdio paths.

OO / class diagram (routine PR -- one function touched):

classDiagram
    direction LR
    class InstallModule {
        <<Pure>>
        +_build_mcp_entry(name, ...) str_or_dict
        +_run_mcp_install(...) void
    }
    class MCPWarningsModule {
        <<Pure>>
        +warn_shell_metachars(env, logger, command) void
        +warn_ssrf_url(url, logger) void
    }
    class MCPDependency {
        <<Model>>
        +from_string(name) MCPDependency
        +from_dict(entry) MCPDependency
    }
    class MCPIntegrator {
        <<Integrator>>
        +install(deps, ...) void
    }
    InstallModule ..> MCPWarningsModule : F5 and F7 warnings
    InstallModule ..> MCPDependency : validates entry
    InstallModule ..> MCPIntegrator : deploys
    note for InstallModule "_build_mcp_entry returns str or dict\n_run_mcp_install must not assume dict\nat warning callsite -- 3 of 4 callsites\nalready guarded with isinstance check"
    class InstallModule:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

Execution flow (Before / After -- before path crashes before any FS write):

Before:

flowchart TD
    A["User: apm install --mcp name\nno --transport, no double-dash"] --> B["_run_mcp_install\ninstall.py:809"]
    B --> C["_build_mcp_entry\ninstall.py:513\nreturns bare str"]
    C --> D["_warn_ssrf_url url=None\nmcp_warnings.py:75"]
    D --> E["entry.get command\ninstall.py:852 BEFORE FIX"]
    E --> F["AttributeError: str object has no attribute get\nEXIT 1 -- no apm.yml written"]

Loading

After:

flowchart TD
    A["User: apm install --mcp name\nno --transport, no double-dash"] --> B["_run_mcp_install\ninstall.py:809"]
    B --> C["_build_mcp_entry\ninstall.py:513\nreturns bare str"]
    C --> D["_warn_ssrf_url url=None\nmcp_warnings.py:75"]
    D --> E["stdio_command = command_argv[0] if command_argv else None\n-> None for bare shorthand\ninstall.py:857 AFTER FIX"]
    E --> F["_warn_shell_metachars env, logger, command=None\nmcp_warnings.py:93 -- guard exits cleanly on None"]
    F --> G["_add_mcp_to_apm_yml name, str_entry\ninstall.py:861 FS write"]
    G --> H["MCPDependency.from_string entry\ninstall.py:876 validates"]
    H --> I["MCPIntegrator.install deps\ninstall.py:894"]
    I --> J["EXIT 0 -- bare str persisted to apm.yml"]

Loading

Design patterns

• Used in this PR: none -- straight-line procedural fix; sources value from the upstream CLI input rather than re-extracting from the downstream transform, which is pragmatic and correct given the str | dict return shape.
• Pragmatic suggestion: Add a return type annotation -> tuple[str | dict, bool] to _build_mcp_entry (install.py:513). This costs one line and lets mypy/pyright flag any future unguarded .get() or .items() call at new callsites before they ship.

---

CLI Logging Expert: No logging regressions. The fix passes None to warn_shell_metachars in the bare-string path; the function guards if command and isinstance(command, str) so it exits cleanly with zero output, which is correct (a bare registry name has no stdio command to metachar-check). In the stdio dict path, command_argv[0] equals what entry["command"] would have been, so the F7 warning fires identically. All messages still route through the logger instance using logger.warning(...). No direct _rich_* calls introduced. The test assertion assert "'str' object has no attribute" not in result.output correctly validates the log path.

---

DevX UX Expert: Before this fix, apm install --mcp io.github.github/github-mcp-server -- the simplest, most discoverable form -- crashed with a raw Python traceback and left apm.yml unchanged. The workaround (--transport stdio) was non-obvious and contradicted the documented mcp: [foo] bare-string shorthand contract. Post-fix, the documented happy path works as expected: bare string in, bare string persisted. This matches the npm/pip mental model: install pkg-name works without extra flags. No CLI surface changes, no help text changes, no docs update required (restoring documented behavior, not adding new behavior).

---

Supply Chain Security Expert: The F7 warning is informational only -- warns, does not gate installation. In the stdio path, command_argv[0] is the CLI argument supplied directly by the user after --; no external package content reaches this parameter. In the bare-string path, None is passed, correctly suppressing the metachar check for a registry-managed name that has no stdio command. No new attack surface: no new subprocess calls, no path joins, no token exposure, no change to lockfile/download/path-security paths. The fix is security-neutral and fails closed identically to before.

---

Auth Expert: Not activated -- the PR modifies only src/apm_cli/commands/install.py (F7 warning callsite) and the test suite; no auth, token, credential, host-classification, or HTTP authorization header logic is touched.

---

OSS Growth Hacker: The bare-string path (apm install --mcp io.github.github/github-mcp-server) is the exact command a new user runs after encountering GitHub's own MCP server in any blog post, quickstart, or docs page. A first-run crash here -- before apm.yml is even written -- is a silent conversion killer: the user sees a Python traceback and concludes APM is broken. This fix unblocks the highest-traffic first-use MCP install pattern. Worth a clear CHANGELOG line and a brief mention in release notes. Side-channel to CEO: this is conversion-critical; shipping before any launch content referencing the GitHub MCP server is correct timing.

---

CEO arbitration

All specialists agree: clean, minimal, correct fix for a conversion-critical bug on the documented happy path. No disagreements to resolve. One gap requires attention before merge: the PR does not include a CHANGELOG entry, which is required by repo convention for every code-changing PR. The action is lightweight -- one line under [Unreleased] > Fixed. The Growth Hacker's note is taken: this fix unblocks the most prominent first-use MCP install pattern, and the release note should say so plainly.

---

Required actions before merge

1. Add a CHANGELOG.md entry under ## [Unreleased] > ### Fixed: e.g., Fix AttributeError crash when running apm install --mcp <name> without --transport or stdio command args -- bare-string registry shorthand now exits 0 and persists correctly (#951).

---

Optional follow-ups

• Add return type annotation -> tuple[str | dict, bool] to _build_mcp_entry (install.py:513) so static analysis catches future unguarded dict-method calls on its return value before they ship.
• Consider a brief integration test (or extension of the existing smoke suite) that runs apm install --mcp <name> against the registry in CI-runtime to catch regressions on the bare-string path in the live registry path, since this is the highest-traffic first-use pattern.

Generated by PR Review Panel for issue #951 · ● 763.2K · ◷

Failing CI

[danielmeppiel]

commands/install.py grew to 1704 LOC (budget 1700). Do NOT trim cosmetically -- engage the python-architecture skill (.github/skills/python-architecture/SKILL.md) and propose an extraction into apm_cli/install/.

[edenfunf]

@danielmeppiel Thanks for the review! Both points addressed and pushed.

Changes

• LOC budget: engaged the python-architecture skill and extracted the --mcp install path into five sibling modules under apm_cli/install/:

  • mcp_args.py
  • mcp_entry.py
  • mcp_writer.py
  • mcp_conflicts.py
  • mcp_command.py

  commands/install.py is now ==1317 LOC== (383 lines of headroom). Behaviour is bit-exact — the original _xxx symbol names are re-exported from commands/install.py so existing test patches (@patch("apm_cli.commands.install._run_mcp_install") etc.) and direct imports keep working without modification.

• CHANGELOG: added an entry under [Unreleased] > Fixed.

Status

Also merged the latest main to clear the conflict against the 0.9.4 release cut — PR is now MERGEABLE. Full unit suite (==5574 tests==) passes, including the #938 regression test.

Will be more mindful of the LOC invariant on future install.py PRs. Thanks again for the careful review!

[danielmeppiel]

consider grouping under a mcp folder and stripping mcp_ file prefixes

[edenfunf]

@danielmeppiel done — moved install/mcp_*.py (×7) into install/mcp/ and stripped the mcp_ prefix from each. Lines up with the unprefixed-noun naming the rest of install/ already uses (pipeline.py, context.py, service.py, ...).

Pure refactor — behaviour and public symbol names unchanged; the back-compat _xxx re-bind aliases in commands/install.py still resolve. All install/ unit tests, the #938 regression, and the LOC budget invariant stay green; commands/install.py is at 1187 LOC. Smoke-tested the bare-string registry shorthand, the stdio-dict path, and the F7 metachar warning live too.

[github-actions]

APM Review Panel Verdict

Disposition: REQUEST_CHANGES (two required pre-merge fixes; MCP subpackage extraction is excellent)

---

Per-persona findings

Python Architect:

This PR is a major architectural change across four concern areas. Diagrams cover the two most impactful structural shifts.

1. OO / class diagram -- Before / After: MCP extraction

Before (god-method in commands/install.py):

classDiagram
    direction LR
    class InstallCommand {
        <<GodClass>>
        +install_cmd(...)
        +_parse_kv_pairs(...)
        +_parse_env_pairs(...)
        +_parse_header_pairs(...)
        +_build_mcp_entry(...)
        +_diff_entry(...)
        +_add_mcp_to_apm_yml(...)
        +_validate_mcp_conflicts(...)
    }
    class MCPIntegrator {
        <<Integrator>>
        +install(...)
        +update_lockfile(...)
    }
    class MCPDependency {
        <<ValueObject>>
        +from_dict(entry)
        +from_string(name)
    }
    class mcp_registry {
        <<Module>>
        +validate_registry_url()
        +resolve_registry_url()
        +registry_env_override()
    }
    class mcp_warnings {
        <<Module>>
        +warn_ssrf_url()
        +warn_shell_metachars()
    }
    InstallCommand ..> mcp_registry : imports
    InstallCommand ..> mcp_warnings : imports
    InstallCommand ..> MCPIntegrator : calls
    InstallCommand ..> MCPDependency : validates through

Loading

After (this PR):

classDiagram
    direction LR
    class InstallCommand {
        <<Coordinator>>
        +install_cmd(...)
    }
    class run_mcp_install {
        <<Orchestrator>>
        +run_mcp_install(...)
    }
    class args {
        <<Pure>>
        +parse_env_pairs()
        +parse_header_pairs()
    }
    class entry {
        <<Pure>>
        +build_mcp_entry()
    }
    class conflicts {
        <<Pure>>
        +validate_mcp_conflicts()
    }
    class registry {
        <<IO/EnvManager>>
        +validate_registry_url()
        +resolve_registry_url()
        +registry_env_override()
    }
    class warnings {
        <<Module>>
        +warn_ssrf_url()
        +warn_shell_metachars()
    }
    class writer {
        <<IO>>
        +add_mcp_to_apm_yml()
    }
    class MCPIntegrator {
        <<Integrator>>
        +install(...)
    }
    class MCPDependency {
        <<ValueObject>>
        +from_dict()
        +from_string()
    }
    InstallCommand ..> run_mcp_install : delegates
    run_mcp_install *-- args : composes
    run_mcp_install *-- entry : composes
    run_mcp_install *-- conflicts : composes
    run_mcp_install *-- registry : composes
    run_mcp_install *-- warnings : composes
    run_mcp_install *-- writer : composes
    run_mcp_install ..> MCPIntegrator : calls
    entry ..> MCPDependency : validates through

    class run_mcp_install:::touched
    class args:::touched
    class entry:::touched
    class conflicts:::touched
    class registry:::touched
    class warnings:::touched
    class writer:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

1b. OO / class diagram -- Before / After: target field gatekeeper

Before:

classDiagram
    direction LR
    class parse_target_field {
        <<SharedGatekeeper>>
        +parse_target_field(value, source_path) str|list|None
    }
    class TargetParamType {
        <<ClickParamType>>
        +convert(value, param, ctx)
    }
    class APMPackage {
        <<ValueObject>>
        +from_apm_yml(path) APMPackage
        +target str|list|None
    }
    TargetParamType ..> parse_target_field : delegates (--target CLI)
    APMPackage ..> parse_target_field : delegates (apm.yml target:)
    note for parse_target_field "Single shared gatekeeper:\nCSV split, alias resolution,\nVALID_TARGET_VALUES check"

Loading

After (this PR -- regression):

classDiagram
    direction LR
    class TargetParamType {
        <<ClickParamType>>
        +convert(value, param, ctx)
    }
    class APMPackage {
        <<ValueObject>>
        +from_apm_yml(path) APMPackage
        +target raw_value_no_normalization
    }
    note for APMPackage "Bug: data.get('target') stored raw.\nCSV strings not split.\nNo VALID_TARGET_VALUES check.\nSilent fallback to copilot."
    class APMPackage:::touched
    class TargetParamType:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

2. Execution flow diagram -- MCP install path (After)

flowchart TD
    A["apm install --mcp NAME [flags]"] --> B["install_cmd() -- commands/install.py"]
    B --> C{"--mcp set?"}
    C -- yes --> D["validate_mcp_conflicts() -- conflicts.py [Pure]"]
    D --> E{"conflict hit?"}
    E -- yes --> F["click.UsageError exit 2"]
    E -- no --> G["resolve_registry_url() -- registry.py [I/O env]"]
    G --> H["run_mcp_install() -- command.py"]
    H --> I["parse_env_pairs() / parse_header_pairs() -- args.py [Pure]"]
    I --> J["build_mcp_entry() -- entry.py [Pure]"]
    J --> K{"ValueError?"}
    K -- yes --> L["click.UsageError exit 2"]
    K -- no --> M["warn_ssrf_url() + warn_shell_metachars() -- warnings.py"]
    M --> N["add_mcp_to_apm_yml() -- writer.py [FS write]"]
    N --> O{"status?"}
    O -- skipped --> P["logger.progress 'unchanged'"]
    O -- added/replaced --> Q["registry_env_override() -- registry.py [env]"]
    Q --> R["MCPIntegrator.install() [NET/FS/LOCK]"]
    R --> S{"Exception?"}
    S -- yes --> T["logger.warning -- torn state risk"]
    S -- no --> U["MCPIntegrator.update_lockfile() [LOCK]"]
    U --> V["logger.success"]

Loading

3. Design patterns

Pattern	Location	Note
Coordinator/Delegate	InstallCommand -> run_mcp_install	install.py sheds MCP responsibility
Pure Functions	args.py, entry.py, conflicts.py	No I/O; trivially testable
Template Method (implicit)	writer.py::add_mcp_to_apm_yml	idempotency policy W3 R3 / F8
Context Manager	registry_env_override()	env save/restore; correct
Shared Gatekeeper (REMOVED)	parse_target_field()	Bug: apm.yml target: no longer normalized

Verdict: MCP extraction is excellent architecture. The parse_target_field() removal is a silent behavioral regression (see Required Actions #1).

---

CLI Logging Expert:

1. command.py: Routes all output through logger.progress(), logger.verbose_detail(), logger.success(), logger.tree_item(), logger.warning(). Correct pattern. ✓
2. writer.py (lines 94-103): Calls _rich_echo() and _rich_warning() directly instead of routing through the injected logger parameter. This is the defined anti-pattern -- utility modules must delegate to the passed logger, not call _rich_* directly. Minor but should be fixed.
3. install/phases/targets.py: The zero-target warning was removed. The deleted block emitted: "No {scope} targets resolved -- nothing will be deployed. Check 'target:' in apm.yml or use --target." This was explicitly noted as defending against the worst-case silent DX (see #820 comment). Its removal means a user with a misconfigured target: value in apm.yml gets silence. This is a UX regression.
4. registry.py: Correctly surfaces MCP_REGISTRY_URL override at non-verbose level ("overrides are visible, defaults are quiet"). ✓

---

DevX UX Expert:

1. _resolve_url_source() in marketplace/resolver.py now rejects any URL that is not https://github.com/... or (github.com/redacted) Previously, it delegated to DependencyReference.parse()which handled GHES, GitLab, and SSH URLs. Users with a marketplacesource.url:pointing to a GHES or GitLab host will get:"Cannot resolve URL source '...' to a Git coordinate."` -- a breaking change with no migration path offered in the error message.
2. The parse_target_field() removal: a user who reads the apm.yml docs and writes target: "claude,copilot" will silently get copilot-only installs. No error, no warning. This is the most damaging silent regression in the PR.
3. CLI flags and help text: unchanged. ✓
4. The E1-E15 conflict matrix in conflicts.py is well-structured and the error messages follow the "name what failed, why, one next action" rule. ✓

---

Supply Chain Security Expert:

1. _redact_url_credentials() in registry.py: correct; strips user:password@ before logging. Handles parse errors safely. ✓
2. _is_local_or_metadata_host(): handles decimal-encoded IPs (obfuscation of 127.0.0.1 as 2130706433). Solid defensive check. ✓
3. registry_env_override(): saves and restores both MCP_REGISTRY_URL and MCP_REGISTRY_ALLOW_HTTP in finally. No env mutation leaks to parent process. ✓
4. command.py lines ~115-126: except Exception after apm.yml has been written. A failed MCPIntegrator.install() leaves the manifest modified but the server not integrated -- a torn state. The # pragma: no cover signals this is accepted. Consider a rollback of the apm.yml write on integration failure, or at minimum a clearer warning naming the recovery action (apm install --mcp NAME --force).
5. No new path traversal surfaces. All mcp/ modules use validated inputs from the Click layer. ✓

---

Auth Expert:

marketplace/builder.py changes introduce a host-classification regression:

1. self._host = default_host() or "github.com" and self._host_info removed. The dynamic host was used to choose the right auth context AND the right metadata URL strategy.
2. resolver.resolve(self._host) --> resolver.resolve("github.com"). For GHES users with GITHUB_HOST set, the resolved token is now always the github.com context, not the GHES context. If the user has a GHES-scoped PAT in GITHUB_APM_PAT and no github.com token, this silently returns no token and falls back to unauthenticated.
3. _fetch_remote_metadata() always constructs raw.githubusercontent.com URLs, which are only accessible for public github.com repos. GHES repos silently return None (metadata fetch fails, error is caught and logged at debug).
4. RefResolver.__init__(): host and token params removed. git ls-remote in RefResolver now always runs without an embedded HTTPS credential. Private github.com repos that rely on the token-in-URL will fail unless the user has a credential helper configured.

The marketplace was likely always primarily github.com-targeted, so this may be an acceptable simplification -- but it requires a CHANGELOG entry documenting the narrowing of scope.

---

OSS Growth Hacker:

The MCP subpackage extraction is an excellent contributor-experience improvement -- smaller, focused modules lower the barrier for community PRs on the --mcp path. This is invisible to end users but valuable for project health.

Side-channel to CEO: Two concerns for the release narrative. (1) The parse_target_field() silent regression is exactly the kind of invisible breakage that generates "it was working before" community issues and erodes trust. It should be the highest-priority fix. (2) The marketplace scope narrowing to github.com-only could generate a negative GitHub Enterprise account report if an enterprise user notices the breakage. A short CHANGELOG line ("Marketplace url: sources now require github.com URLs") lets that user find the change rather than file a confused issue.

---

CEO arbitration

The MCP subpackage extraction is first-rate -- a clean application of APM's LOC-budget invariant, with well-scoped pure modules, correct idempotency semantics, and good security hygiene in the registry helper. That part should merge. Two required fixes block merge:

First, the parse_target_field() removal introduced a silent regression: apm.yml's target: "claude,copilot" (CSV string) is no longer split and alias-resolved at manifest load time. Downstream, the raw string hits the KNOWN_TARGETS lookup as a literal key, fails to match, and silently falls back to [copilot]. A user who had multi-target installations working before this PR will get a different result with no warning. Fix is surgical: restore parse_target_field() (or equivalent inline normalization) in APMPackage.from_apm_yml().

Second, the marketplace narrowing (hardcoded github.com, removed GHES host branching, RefResolver token removal) needs a CHANGELOG entry. The change itself may be intentional and correct, but it's a silent scope reduction that will confuse enterprise users. Per operating principle 1: "Breaking changes are allowed; silent breaking changes are not."

The zero-target warning removal and the writer.py direct _rich_* calls are minor and can be addressed in a follow-up.

Disposition: REQUEST_CHANGES on the two items above; approve the rest.

---

Required actions before merge

1. Restore apm.yml target normalization (src/apm_cli/models/apm_package.py line ~206, src/apm_cli/core/target_detection.py). APMPackage.from_apm_yml() must normalize the raw target: value the same way TargetParamType.convert() does -- CSV split, alias resolution, VALID_TARGET_VALUES check -- before storing it. The simplest fix is to re-add parse_target_field() (or re-export TargetParamType-equivalent logic) and call it in from_apm_yml(). Without this, target: "claude,copilot" in apm.yml silently falls back to copilot-only with no warning.
2. Add CHANGELOG entry for marketplace scope narrowing. Under [Unreleased] > Changed, add a line documenting that marketplace url: sources now require https://github.com/ or `(github.com/redacted) URLs (GHES and non-GitHub URLs are no longer resolved). One line is sufficient.

---

Optional follow-ups

• writer.py calls _rich_echo() and _rich_warning() directly; these should route through the passed logger parameter to stay consistent with the CommandLogger pattern.
• Restore the zero-target install warning in install/phases/targets.py (or open a tracking issue -- the comment cited #820 as the motivating rationale for that warning).
• The unified loop in active_targets() / active_targets_user_scope() was replaced with duplicated isinstance(list) / str branches; consider re-unifying to a single normalization step at the top of each function.
• Track GHES marketplace support as a future capability (open issue referencing #951) so enterprise interest can be gauged before committing to re-adding it.
• Add a recovery hint to the command.py torn-state warning: "Use 'apm install --mcp NAME --force' to retry integration." so users know what to do if the exceptional path fires.

Generated by PR Review Panel for issue #951 · ● 1.8M · ◷

[danielmeppiel]

Required actions before merge

Restore apm.yml target normalization (src/apm_cli/models/apm_package.py line ~206, src/apm_cli/core/target_detection.py). APMPackage.from_apm_yml() must normalize the raw target: value the same way TargetParamType.convert() does -- CSV split, alias resolution, VALID_TARGET_VALUES check -- before storing it. The simplest fix is to re-add parse_target_field() (or re-export TargetParamType-equivalent logic) and call it in from_apm_yml(). Without this, target: "claude,copilot" in apm.yml silently falls back to copilot-only with no warning.

Add CHANGELOG entry for marketplace scope narrowing. Under [Unreleased] > Changed, add a line documenting that marketplace url: sources now require https://github.com/ or `(github.com/redacted) URLs (GHES and non-GitHub URLs are no longer resolved). One line is sufficient.

writer.py calls _rich_echo() and _rich_warning() directly; these should route through the passed logger parameter to stay consistent with the CommandLogger pattern.

Restore the zero-target install warning in install/phases/targets.py (or open a tracking issue -- the comment cited #820 as the motivating rationale for that warning).

The unified loop in active_targets() / active_targets_user_scope() was replaced with duplicated isinstance(list) / str branches; consider re-unifying to a single normalization step at the top of each function.

[edenfunf]

Pushed bf3ae07 addressing the panel verdict's required and optional actions:

• apm_package.from_apm_yml() now normalizes target: through parse_target_field() (CSV split, alias resolution, VALID_TARGET_VALUES check), matching the --target CLI behaviour.
• CHANGELOG.md [Unreleased] > Changed documents the marketplace url: scope narrowing to https://github.com/ / http://github.com/.
• install/mcp/writer.py routes the replacement-diff output through the injected logger (defaulting to NullCommandLogger); the direct _rich_echo / _rich_warning calls are gone.
• install/phases/targets.py restores the zero-target install warning.
• integration/targets.py re-unifies active_targets() / active_targets_user_scope() via a single normalization step at the top of each function.

Test status: 6645 unit tests pass; the 5 remaining failures are pre-existing Windows path-separator / absolute-path quirks unrelated to this change (tests/unit/test_config_command.py cowork skills dir, tests/unit/test_script_formatters.py). The #938 regression test still passes.

Re-requesting review when convenient — happy to iterate further if anything still looks off.

[Copilot]

Pull request overview

Fixes a crash in the apm install --mcp <registry-ref> “bare-string registry shorthand” path by avoiding dict-only access in the shell-metachar warning logic, and refactors the MCP install helpers into a dedicated install/mcp/ subpackage. The PR also introduces broader target-field parsing/normalization changes that affect --target and apm.yml target: handling.

Changes:

• Fix AttributeError: 'str' object has no attribute 'get' in --mcp installs by sourcing the stdio command from command_argv rather than the built entry.
• Extract MCP install logic into src/apm_cli/install/mcp/ modules (args/conflicts/entry/registry/warnings/writer/command) and re-bind legacy helper names from commands/install.py.
• Add a shared parse_target_field validator and update target resolution/tests accordingly.

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/apm_cli/install/mcp/command.py	New orchestrator for --mcp install flow; fixes the bare-string warning crash.
src/apm_cli/install/mcp/entry.py	Extracted MCP entry builder returning `str
src/apm_cli/install/mcp/warnings.py	Extracted F5/F7 warnings used during MCP install.
src/apm_cli/install/mcp/writer.py	Extracted idempotent apm.yml mutation logic for MCP entries.
src/apm_cli/install/mcp/conflicts.py	Extracted --mcp flag conflict validation matrix.
src/apm_cli/install/mcp/args.py	Extracted --env / --header repetition parsing.
src/apm_cli/install/mcp/registry.py	Updated registry helpers to use the extracted entry builder.
src/apm_cli/commands/install.py	Swaps in extracted MCP helper modules while keeping legacy patch points via aliases.
tests/unit/test_install_command.py	Adds regression test for bare-string shorthand persistence.
src/apm_cli/core/target_detection.py	Adds parse_target_field and routes Click target parsing through it.
src/apm_cli/models/apm_package.py	Validates/normalizes apm.yml target: via parse_target_field.
src/apm_cli/integration/targets.py	Adjusts explicit-target handling assumptions based on upstream parsing guarantees.
tests/unit/core/test_target_detection.py	Updates tests for list input validation + alias collapse behavior.
tests/unit/test_apm_package.py	Updates target parsing expectations (alias resolution + list collapsing).
tests/unit/integration/test_targets.py	Updates behavior expectations around unknown targets.
src/apm_cli/install/phases/targets.py	Adds defensive warning when zero targets resolve.
CHANGELOG.md	Adds Unreleased entries for MCP crash fix and MCP module refactor (and a marketplace note).
.github/instructions/tests.instructions.md / .apm/instructions/tests.instructions.md	Updates doc references to new MCP registry module path.

Comments suppressed due to low confidence (1)

CHANGELOG.md:14

• The Unreleased changelog includes a Marketplace URL-resolution behavior change, but this PR's description focuses on the --mcp crash/refactor. If the marketplace restriction is not part of this PR, it should be removed from this entry (or moved to the correct PR/version) to keep the changelog accurate; otherwise, ensure the PR description also calls out the marketplace behavior change.

- **Dev Container Feature** `ghcr.io/microsoft/apm/apm-cli` -- one-line install of the APM CLI into any `devcontainer.json`, GitHub Codespace, or JetBrains Gateway workspace. Supports a `version` option (`latest` or pinned semver), declares `installsAfter` for the official Python feature, handles PEP 668 on Ubuntu 24.04+. Ships with 37 bats unit tests and a 6-distro Docker integration matrix (Ubuntu 24.04, Ubuntu 22.04, Debian 12, Alpine 3.20, Fedora 41, plus Python-feature combo). (#861)
- `shared/apm.md` gh-aw workflow gains an `apps:` array input for cross-org private packages: each entry mints its own GitHub App installation token via `actions/create-github-app-token` and packs only its declared packages, with a matrix fan-out one replica per credential group. The single-app top-level form (`app-id`, `private-key`, `owner`, `repositories`) shipped earlier in this cycle is preserved as the canonical shorthand for one-org users; `apps[]` is purely additive. Multi-bundle restore uses the `bundles-file:` input from `microsoft/apm-action@v1.5.0` (microsoft/apm-action#30, microsoft/apm-action#29).

[github-actions]

APM Review Panel Verdict: REJECT

Five specialists converged on a CHANGELOG entry that promises a security-relevant github.com-only marketplace restriction with zero corresponding code implementation; four additional required findings on output consistency, exit codes, and credential leakage are independent blockers.

Required before merge (11 items)

• [python-architect] CHANGELOG documents a github.com-only restriction on marketplace url: sources that is not implemented in code

  • Why: CHANGELOG.md states "Marketplace url: sources now require https://github.com/ or (github.com/redacted) URLs" but resolver.py is byte-for-byte identical to main -- _resolve_url_source still accepts any host. Shipping a changelog that describes a security-relevant breaking change that was either never written or reverted without removing the entry.
  • Suggested fix: Either implement the restriction in _resolve_url_source (add a github.com netloc check and raise ValueError for non-github hosts) and update its docstring, or remove the CHANGELOG entry entirely. Do not ship a changelog that contradicts the running code.

• [python-architect] validate_mcp_dry_run_entry missing type annotation on name parameter -- violates public-API type-hints rule at src/apm_cli/install/mcp/registry.py:240

  • Why: All other extracted public functions in this PR (build_mcp_entry, run_mcp_install, add_mcp_to_apm_yml, validate_mcp_conflicts) carry full type hints. validate_mcp_dry_run_entry(name, **kwargs) has no annotation and **kwargs silently swallows unknown keyword arguments.
  • Suggested fix: Mirror build_mcp_entry's keyword-only signature with explicit typed parameters (name: str, *, transport: Optional[str] = None, url: Optional[str] = None, ...).

• [cli-logging-expert] Diff lines gated behind verbose_detail before a blocking confirm prompt at src/apm_cli/install/mcp/writer.py:101

  • Why: writer.py shows the replacement diff via logger.verbose_detail(), which is suppressed unless --verbose is passed. The user is then asked "Replace MCP server X?" with no visible diff. Asking for confirmation while hiding the information needed to answer it defeats the purpose of the prompt entirely.
  • Suggested fix: Use logger.info(line) or equivalent unconditional output for the diff lines. Reserve verbose_detail for supplementary detail, not the primary content of an interactive decision.

• [cli-logging-expert] Raw exception message from integration layer leaked into user-visible warning at src/apm_cli/install/mcp/command.py:130

  • Why: logger.warning(f"MCP server written to apm.yml but integration failed: {exc}") forwards an unredacted exception string. MCPIntegrator exceptions may contain internal file paths, registry credentials, or stack-trace fragments. Users cannot act on them and operators should not see them at warning level.
  • Suggested fix: Log exc at debug/verbose_detail level and emit a fixed actionable string: "MCP server written to apm.yml but tool integration failed. Run with --verbose for details.".

• [cli-logging-expert] Partial-failure path (integration error) exits 0 after emitting a warning at src/apm_cli/install/mcp/command.py:130

  • Why: When MCPIntegrator.install raises, the code logs a warning and returns normally from run_mcp_install, so the CLI exits 0. apm.yml was mutated but tool integration did not complete. A CI pipeline will see green while the MCP server is not actually integrated.
  • Suggested fix: Re-raise as click.ClickException (exit 1) after logging, or document and test that this is intentional soft-failure and update the docstring accordingly.

• [devx-ux-expert] CHANGELOG claims a new github.com-only restriction that has no corresponding code change at CHANGELOG.md:38

  • Why: resolver.py is not in the PR diff at all. Shipping this entry (1) alarms GHES/GitLab marketplace users who had nothing actually change for them, (2) drives unnecessary migration work, (3) sets a false expectation that github.com is enforced when the code does not enforce it.
  • Suggested fix: If the restriction is intentional, add the netloc check to _resolve_url_source. If documenting pre-existing behavior, reclassify away from "Changed" so it does not imply a new breakage.

• [devx-ux-expert] Error message for rejected non-github.com url: source is not actionable at src/apm_cli/marketplace/resolver.py:111

  • Why: If the github.com restriction is real (per CHANGELOG), the error a user sees when their GHES URL fails is a generic parse error with no guidance on why the host is disallowed or what to do next. Per the DevX contract, errors must tell users what to do, not just what went wrong.
  • Suggested fix: After any enforcement check, raise ValueError with an explicit message: "URL source '<url>' host '<host>' is not supported; only https://github.com/ URLs are resolved. GHES and GitLab resolution is tracked in #1010.".

• [supply-chain-security-expert] validate_registry_url echoes raw (unredacted) URL, including embedded credentials, in UsageError messages at src/apm_cli/install/mcp/registry.py:123

  • Why: If a user passes --registry (user/redacted):ghp_TOKEN@registry.internal with an unsupported scheme or missing netloc, the error message embeds the raw URL string. _redact_url_credentials() is defined in the same file and used in log paths, but is not applied to the three UsageError format strings. CI log aggregators and shell history capture this output.
  • Suggested fix: Replace '{value}' with '{_redact_url_credentials(value)}' in all three UsageError format strings (lines ~112, ~124, ~130). The redactor is already present and tested.

• [supply-chain-security-expert] CHANGELOG claims marketplace url: sources restricted to github.com, but _resolve_url_source silently accepts and silently drops any host at src/apm_cli/marketplace/resolver.py:96

  • Why: A user who pins a supply-chain source to (gitlab.example.com/redacted) receives no error but APM silently fetches from GITHUB_HOST` instead. This is a provenance integrity gap: the install is not bit-identical to what the URL asserts, and auditors cannot verify which host was actually queried.
  • Suggested fix: Either enforce the claimed restriction by checking dep.host after parse and raising ValueError for non-github.com hosts, or revert the CHANGELOG claim and document the actual silent host-normalization behavior.

• [oss-growth-hacker] Breaking change for GHES/GitLab marketplace url: users has no migration path at CHANGELOG.md:21

  • Why: The CHANGELOG states GHES, GitLab, and other non-GitHub hosts "are no longer resolved" with no guidance on what to do next. Contrast with the target: breaking change in the same release, which has an explicit Migration: block. Enterprise operators cannot self-serve a fix.
  • Suggested fix: Add a Migration: note: "GHES users: switch to type: github and set GITHUB_HOST to your enterprise host. GitLab/Bitbucket: use type: git-subdir with an explicit repo: owner/repo field, or migrate to a GitHub-hosted mirror."

• [oss-growth-hacker] No actionable error surfaced when a non-github.com url: source silently fails at src/apm_cli/marketplace/resolver.py:96

  • Why: _resolve_url_source silently discards the URL host and resolves against GITHUB_HOST. Users who land on a confusing "repository not found on github.com" failure deep in git ls-remote cannot connect it to their GHES URL. The CHANGELOG promises enforcement; the code does not enforce it.
  • Suggested fix: Add a netloc check at the top of _resolve_url_source with an actionable error message including GITHUB_HOST guidance and a docs link.

Nits (14 items, skip if you want)

• [python-architect] MCPEntry type alias is defined only in writer.py but entry.py inlines the same Union[str, Dict[str, Any]] -- move to mcp/__init__.py for a shared, centralized contract
• [python-architect] _ALLOWED_URL_SCHEMES is a private name imported across package boundaries from models/dependency/mcp.py -- expose a public constant instead
• [python-architect] registry_env_override mutates os.environ without a module-level lock -- add _registry_env_lock = threading.Lock() against future concurrent installs
• [python-architect] _diff_entry is re-exported from commands/install.py without aliasing, leaking a private name across the boundary -- either make it public in writer.py or add an explicit alias
• [cli-logging-expert] logger.progress() used for the terminal "MCP server X unchanged" outcome -- progress() implies in-flight work; a logger.info() or logger.skip() better conveys completion
• [cli-logging-expert] logger.tree_item(f" transport: ...") passes two leading spaces -- if tree_item already applies its own indent, output will have four spaces; strip the leading spaces
• [cli-logging-expert] chosen_transport falls back to "registry" for URL-based entries with no explicit transport key, printing transport: registry which is misleading
• [devx-ux-expert] logger.tree_item(f" transport: registry") in the success output is imprecise -- "registry" is a resolution mechanism, not a transport protocol; consider "resolved via: registry" or omit for bare-string shorthand
• [devx-ux-expert] Back-compat re-exports in commands/install.py use private-prefixed aliases with no comment explaining intent -- add # Legacy aliases for test patches; do not use in new code.
• [supply-chain-security-expert] warn_shell_metachars receives only command_argv[0]; metacharacters in args at index 1+ (e.g. npx server '$(evil)') are not warned on
• [supply-chain-security-expert] _is_local_or_metadata_host is warn-only for cloud metadata IPs including 169.254.169.254 -- consider an opt-out flag (e.g. MCP_REGISTRY_ALLOW_METADATA_HOST=1) that defaults to blocking
• [supply-chain-security-expert] registry_env_override mutates os.environ globally -- not thread-safe; document the single-threaded assumption in the docstring
• [oss-growth-hacker] The bare-string apm install --mcp (name) crash fix (Bug: apm install --mcp without --transport fails with AttributeError #938) is under-celebrated in the CHANGELOG -- it unblocks every new user on the MCP path; consider bolding it as **apm install --mcp (name) crash on first use**
• [oss-growth-hacker] Internal refactor (mcp/ subpackage rename) listed in "Changed" alongside breaking changes trains readers to skim -- consider moving internal-only items to a separate section

CEO arbitration

The dominant and unambiguous finding across all five active panelists is a CHANGELOG entry that documents a security-relevant, breaking behavioral change -- restricting marketplace url: sources to github.com only -- that has zero corresponding code implementation. resolver.py is byte-for-byte identical to main. This is not a matter of interpretation: the code does not enforce what the changelog promises, and the changelog does not reflect what the code does. The consequences are tripled: external operators (GHES, GitLab) are alarmed into unnecessary migration work by a restriction that does not exist; security auditors cannot verify supply-chain provenance because the host is silently discarded rather than rejected; and CI pipelines see exit 0 on a partial-failure path while apm.yml has been mutated. The PR must either implement the restriction with an actionable error and a migration note, or remove the CHANGELOG entry entirely before merge.

Beyond the dominant finding, cli-logging-expert surfaces three additional required issues of independent severity: confirmation prompts are shown after hiding the diff behind --verbose, making informed consent impossible; raw exception strings including potential internal paths or credentials are forwarded to user-visible warning output; and partial integration failures exit 0, breaking CI contracts. supply-chain-security-expert adds a fourth orthogonal required finding: validate_registry_url echoes unredacted URLs including embedded tokens in UsageError messages, despite a _redact_url_credentials helper existing in the same file. These four findings are independent of the CHANGELOG issue and each independently blocks a clean merge. python-architect flags a missing type annotation on a public function that violates the project's own API contract. No substantive dissent exists between panelists -- all active voices converge on the same core defect with complementary framings.

The structural refactor itself (7 flat modules into install/mcp/ subpackage) and the bare-string registry shorthand crash fix are uncontested positives. The refactor is clean, the boundary design is sound, and the crash fix addresses a real first-run failure mode. The PR is architecturally sound in its non-changelog scope and would be approvable if the five required-finding clusters were resolved. The CHANGELOG ghost entry is the critical blocker: ship it as written and the project owns a false security promise in a public release note.

Growth/positioning note: The first-run bug fix (#938, bare-string registry shorthand crash) is under-celebrated relative to its user impact. A user's very first apm install --mcp (name) invocation was crashing with AttributeError; fixing that is a high-leverage retention moment. Elevating it from a terse bullet to a named fix with a before/after example in release notes would reinforce the project's responsiveness signal to new adopters evaluating APM for the first time. Additionally, explicitly noting that GHES is supported via the GITHUB_HOST env var in any github.com restriction messaging would neutralize the read that APM is abandoning enterprise support.

---

Per-persona findings (full)

Python Architect

classDiagram
    class install_py ["commands/install.py (BEFORE)"] {
        +install() Click handler
        -_parse_kv_pairs()
        -_parse_env_pairs()
        -_parse_header_pairs()
        -_build_mcp_entry()
        -_add_mcp_to_apm_yml()
        -_diff_entry()
        -_validate_mcp_conflicts()
        -_MCP_REQUIRED_FLAGS
        -_run_mcp_install()
    }
    class mcp_registry_py ["install/mcp_registry.py"] {
        +validate_registry_url()
        +resolve_registry_url()
        +registry_env_override()
        +validate_mcp_dry_run_entry()
    }
    class mcp_warnings_py ["install/mcp_warnings.py"] {
        +warn_ssrf_url()
        +warn_shell_metachars()
    }
    install_py --> mcp_registry_py : imports
    install_py --> mcp_warnings_py : imports
    mcp_registry_py ..> install_py : circular dep (validate_mcp_dry_run_entry)

Loading

classDiagram
    class install_py ["commands/install.py (AFTER)"] {
        +install() Click handler
        back-compat re-exports
    }
    class args_py ["mcp/args.py"] {
        +parse_kv_pairs()
        +parse_env_pairs()
        +parse_header_pairs()
    }
    class entry_py ["mcp/entry.py"] {
        +build_mcp_entry() str|dict
    }
    class writer_py ["mcp/writer.py"] {
        +add_mcp_to_apm_yml()
        -_diff_entry()
        MCPEntry
    }
    class conflicts_py ["mcp/conflicts.py"] {
        +validate_mcp_conflicts()
        +MCP_REQUIRED_FLAGS
    }
    class command_py ["mcp/command.py"] {
        +run_mcp_install()
    }
    class registry_py ["mcp/registry.py"] {
        +validate_registry_url()
        +resolve_registry_url()
        +registry_env_override()
        +validate_mcp_dry_run_entry()
    }
    class warnings_py ["mcp/warnings.py"] {
        +warn_ssrf_url()
        +warn_shell_metachars()
    }
    install_py --> command_py : delegates run_mcp_install
    install_py --> registry_py : validate + resolve
    install_py --> conflicts_py : validate_mcp_conflicts
    command_py --> args_py : parse_env/header_pairs
    command_py --> entry_py : build_mcp_entry
    command_py --> writer_py : add_mcp_to_apm_yml
    command_py --> warnings_py : warn_ssrf_url, warn_shell_metachars
    command_py --> registry_py : registry_env_override
    registry_py --> entry_py : build_mcp_entry (lazy, no cycle)

Loading

flowchart TD
    A[install Click handler] --> B[parse_env_pairs inline]
    B --> C[_build_mcp_entry inline]
    C --> D{entry.get command}
    D -->|entry is str| E[AttributeError CRASH]
    D -->|entry is dict| F[warn_ssrf_url]
    F --> G[_add_mcp_to_apm_yml inline]
    G --> H{status?}
    H -->|skipped| I[return]
    H -->|added/replaced| J[MCPIntegrator.install]

Loading

flowchart TD
    A[install Click handler] --> B[validate_mcp_conflicts]
    B --> C[validate_registry_url]
    C --> D[run_mcp_install -- mcp/command.py]
    D --> E[parse_env_pairs / parse_header_pairs]
    E --> F[build_mcp_entry -- mcp/entry.py]
    F --> G{ValueError?}
    G -->|yes| H[raise UsageError exit 2]
    G -->|no| I[warn_ssrf_url]
    I --> J[stdio_command = command_argv 0 if command_argv]
    J --> K[warn_shell_metachars]
    K --> L[add_mcp_to_apm_yml -- mcp/writer.py]
    L --> M{status?}
    M -->|skipped| N[return]
    M -->|added/replaced| O{isinstance entry str?}
    O -->|str| P[MCPDependency.from_string]
    O -->|dict| Q[MCPDependency.from_dict]
    P --> R[registry_env_override]
    Q --> R
    R --> S[MCPIntegrator.install]
    S --> T[logger.success]

Loading

Required:

1. CHANGELOG documents github.com-only restriction not implemented in code -- CHANGELOG.md
2. validate_mcp_dry_run_entry missing type annotation -- src/apm_cli/install/mcp/registry.py:240

Nits:

• MCPEntry type alias duplicated between writer.py and entry.py
• _ALLOWED_URL_SCHEMES private name imported across package boundaries
• registry_env_override mutates os.environ without a lock
• _diff_entry re-exported without aliasing from commands/install.py

CLI Logging Expert

Required:

1. Diff lines gated behind verbose_detail before blocking confirm prompt -- src/apm_cli/install/mcp/writer.py:101
2. Raw exception message leaked into user-visible warning -- src/apm_cli/install/mcp/command.py:130
3. Partial-failure (integration error) exits 0 -- src/apm_cli/install/mcp/command.py:130

Nits:

• logger.progress() used for terminal "unchanged" outcome
• Double indentation in tree_item calls
• chosen_transport falls back to "registry" for URL-based entries

DevX UX Expert

Required:

1. CHANGELOG claims github.com restriction with no corresponding code -- CHANGELOG.md:38
2. Error message for rejected non-github.com URL source is not actionable -- src/apm_cli/marketplace/resolver.py:111

Nits:

• Success output shows transport: registry which is imprecise
• Back-compat re-exports in commands/install.py use private-prefixed aliases without comment

Supply Chain Security Expert

Required:

1. validate_registry_url echoes raw URL with embedded credentials in UsageError messages -- src/apm_cli/install/mcp/registry.py:123
2. CHANGELOG claims github.com restriction; _resolve_url_source silently accepts and drops any host -- src/apm_cli/marketplace/resolver.py:96

Nits:

• warn_shell_metachars only checks command_argv[0]; args at index 1+ not checked
• _is_local_or_metadata_host is warn-only for cloud metadata IPs (169.254.169.254)
• registry_env_override mutates os.environ globally -- not thread-safe

Auth Expert

Inactive -- No auth-related files changed; mcp/registry.py host classification is SSRF-guard-only, not AuthResolver-related; marketplace URL restriction is a business-logic change with no impact on credential resolution or token management.

OSS Growth Hacker

Required:

1. Breaking change for GHES/GitLab url: users has no migration path -- CHANGELOG.md:21
2. No actionable error when non-github.com url: source silently fails -- src/apm_cli/marketplace/resolver.py:96

Nits:

• First-run bug fix (Bug: apm install --mcp without --transport fails with AttributeError #938) is under-celebrated in the CHANGELOG
• Internal refactor entry in "Changed" adds noise for external readers

_{Verdict computed deterministically: 11 required findings across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically.}

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• #951 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• fix(install): handle bare-string MCP entry in shell-metachar warning #951 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #951 · ● 10.6M · ◷