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).