fix: respect targets: whitelist for all runtimes during MCP install

[danielmeppiel]

fix: respect targets: whitelist for all runtimes during MCP install

TL;DR

apm install --mcp now mirrors the same targets-engine UX apm install already uses for skills, agents, and instructions: the active target set (--target > apm.yml targets: > directory auto-detection) is the whitelist for every runtime, and the user-facing logging follows the same [i] / [x] symbol contract. Two findings drove the final scope: (1) a paired CLI-logging + DevX-UX advisory review caught a hidden call-site bypass that was silently breaking the PR's own promise, and (2) a UX-parity sweep against apm install caught a remaining greenfield asymmetry (the MCP path silently fell back to [copilot] while apm install raised NoHarnessError). Both are closed in this PR. apm install -g --mcp NAME is a deliberate carve-out — user-scope writes are not project-bound and bypass the gate by design. Fixes #1335.

Important

This is a behavior change. With no targets: declared, directory detection is now a hard whitelist for every runtime (not just Codex / Claude). And a greenfield project with no targets:, no --target, and no detected harness signals fails closed with NoHarnessError — the same outcome apm install already produces for that input. See "Behavior delta" in Trade-offs.

Problem (WHY)

• targets: [copilot] did not bind MCP writes. The plural targets: form was parsed for skill compilation but never threaded into the MCP gate, so a Codex binary on PATH triggered a ~/.codex/config.toml write the user had explicitly opted out of ([BUG] targets: in apm.yml is not respected for MCP server installation #1335). Reproducible against main with the steps in "How to test".
• Audit-caught: hidden call-site bypass. Even after the gate was rewritten to honor targets:, the call site at commands/install.py:1795 was still building mcp_apm_config = {"target": apm_package.target, ...} from the singular legacy field only. APMPackage.from_apm_yml never read targets: plural, so for any user on the canonical syntax apm_package.target = None, the gate saw an empty config dict, and fell back to permissive directory detection. The whitelist was silently bypassed all over again.
• UX-parity gap: greenfield asymmetry. apm install for skills already raises NoHarnessError when the user provides no targets:, no --target, and the project has no detectable harness signals (no .github/copilot-instructions.md, no .cursor/, etc.). The MCP gate, in the same input shape, silently fell back to [copilot] and wrote ~/.copilot/mcp-config.json. Two surfaces, two contracts, one user — and the silent path was the dangerous one (a greenfield project shouldn't get user-scope writes the user never asked for).
• Asymmetric BC path (legacy). The previous gate only checked Codex and Claude when no targets: was declared, on the theory that other runtimes self-gate by directory. That carve-out lived in a _PROJECT_SCOPED_RUNTIMES = ("codex", "claude") constant — a footnote that future maintainers had to internalize.
• [!] CLI-logging divergence. The malformed-targets: branch glued multi-line EmptyTargetsListError body mid-sentence into a single yellow [!] warning, breaking layout. The drop branch omitted the symbol= kwarg so its line had no [i] prefix and didn't say what the active set actually was — the user had to grep apm.yml to confirm what the gate did.
• [!] CSV regression in bundle wiring. _wire_bundle_mcp_servers passed target_csv = "claude,copilot" straight into active_targets(), which treated the comma string as one unknown token and dropped every runtime silently.

Why these matter: the targets engine is the single contract that promises "one manifest, every harness". When MCP install diverges from it, users learn that targets: "kind of" works — and silent writes to global config files are a security-relevant trust break, not just an annoyance. The audit-caught bypass and the greenfield asymmetry are the worst kinds of divergence: the gate logic was correct, but the call site never gave it the data, and the no-data fallback was permissive instead of failing closed like the rest of apm install.

Approach (WHAT)

#	Fix
1	Replace the gate body with a thin wrapper around resolve_targets(). The active set IS the whitelist for every runtime, every install path.
2	Audit fix: thread targets: plural end-to-end. APMPackage now carries a targets: list[str] | None field; commands/install.py forwards only the key the user actually declared (avoids tripping the gate's target / targets mutex check with a None placeholder).
3	UX-parity fix: greenfield (no targets:, no --target, no detected signals) now lets resolve_targets raise NoHarnessError; the gate catches and renders [x] Skipping all MCP config writes -- could not resolve active targets. Same red [x] voice and same fail-closed semantics as apm install.
4	Drop the _PROJECT_SCOPED_RUNTIMES constant and its conditional branch entirely.
5	Alias the vscode runtime to the copilot canonical target inside the gate (mirrors targets.py:776).
6	Fail closed on ConflictingTargetsError / EmptyTargetsListError / UnknownTargetError from the resolver — write nothing, render the same [x] red error voice + remediation block as apm install.
7	Normalize CSV strings ("claude,copilot") to lists before they reach the resolver.
8	Drop line: [i] Skipped MCP config for X (active targets: Y) — same double-space shape as the canonical Targets: X (source: Y) provenance line.
9	--mcp help text and docs/consumer/install-mcp-servers.md cross-link to install-packages "Where files land" instead of duplicating the rule, and explicitly frame the -g carve-out (user-scope writes don't consult the project-scope gate).

Implementation (HOW)

• src/apm_cli/integration/mcp_integrator.py — _gate_project_scoped_runtimes is a thin wrapper around resolve_targets(). Catches NoHarnessError (greenfield), AmbiguousHarnessError (multi-signal collision), ConflictingTargetsError, EmptyTargetsListError, UnknownTargetError. Each renders a red [x] line and returns [] (write nothing). Pre-canonicalizes runtime aliases (vscode/agents) via RUNTIME_TO_CANONICAL_TARGET before the resolver call so the strict canonical validator accepts them.
• src/apm_cli/integration/targets.py — Exports the shared RUNTIME_TO_CANONICAL_TARGET dict reused by the gate.
• src/apm_cli/models/apm_package.py — Added targets: list[str] \| None = None field; from_apm_yml parses it from the raw YAML.
• src/apm_cli/commands/install.py — Line 1795 builds mcp_apm_config conditionally, forwarding only the key the user declared. --mcp NAME help wordsmithed to land the gate contract in one line.
• src/apm_cli/install/local_bundle_handler.py — _wire_bundle_mcp_servers builds a plural targets: list (was a CSV string — pre-empted dependency-shape drift) and forwards it to the gate via the same key the user declares. Log lines reference the joined name list.
• tests/unit/integration/test_mcp_integrator.py — TestGateProjectScopedRuntimes is now 17 tests (89 total in the file). Locks in the audit findings, the strictness extension, the alias-canonicalization step, and the new [i] + (active targets: ...) shape.
• tests/integration/test_mcp_targets_gating_e2e.py — NEW. End-to-end gating tests: whitelist-suppresses-foreign-writes, multi-target-allows-listed-runtimes, greenfield-fails-closed.
• docs/src/content/docs/consumer/install-mcp-servers.md — Consolidates per-runtime opt-in + project-gate paragraphs into one rule; mentions greenfield NoHarnessError parity; explicit -g carve-out framing.
• docs/src/content/docs/reference/targets-matrix.md — MCP section no longer claims unconditional writes "for every target with an adapter" — the active target set governs the write.
• packages/apm-guide/.apm/skills/apm-usage/commands.md — --mcp NAME row spells out the gate behavior + the -g carve-out.
• CHANGELOG.md — Leads with the user-visible delta, then audit detail, then the strictness extension and -g carve-out.

Diagrams

Legend: how a single apm install call decides which runtimes get an MCP write, end-to-end. The shaded "audit fix" nodes are where the call-site bypass lived before this PR. The shaded "UX-parity fix" node is where the greenfield asymmetry was closed.

flowchart TD
    A["apm install --mcp NAME"] --> B{user_scope -g?}
    B -- "yes (~/.config writes)" --> Z["bypass project-scope gate, write user-scope only<br/>(by design -- not project-bound)"]
    B -- "no (project scope)" --> AUD["APMPackage.from_apm_yml<br/>parses BOTH target: and targets:"]
    AUD --> CALL["commands/install.py builds<br/>mcp_apm_config conditionally<br/>(only the key user declared)"]
    CALL --> D["resolve_targets(root, explicit)"]
    D --> E{"resolve outcome"}
    E -- "NoHarnessError<br/>(greenfield)" --> X1["fail closed:<br/>[x] 'Skipping all MCP writes -- could not resolve active targets'"]
    E -- "ConflictingTargetsError /<br/>EmptyTargetsListError /<br/>UnknownTargetError /<br/>AmbiguousHarnessError" --> X2["fail closed:<br/>[x] red error + remediation"]
    E -- "ok" --> J["alias vscode -> copilot,<br/>intersect with target_runtimes"]
    J --> K{"any runtime dropped?"}
    K -- "yes" --> L["[i] Skipped MCP config for X<br/>  (active targets: Y)"]
    K -- "no" --> M["write MCP configs"]
    L --> M
    classDef audit fill:#fff3cd,stroke:#856404
    classDef parity fill:#d1ecf1,stroke:#0c5460
    class AUD,CALL audit
    class X1 parity

Loading

Trade-offs

• Behavior delta vs main (project scope). With no targets: declared, directory detection is now a hard whitelist for every runtime. Concretely: a project with copilot on PATH and only a .cursor/ directory previously wrote MCP for both Copilot and Cursor; it now writes Cursor only. Same rule apm install already enforces for skills/agents/instructions in that project.
• Behavior delta vs main (greenfield). A project with no targets:, no --target, AND no detected signals previously fell back to [copilot] and wrote ~/.copilot/mcp-config.json silently. It now raises NoHarnessError with the same red [x] voice and remediation block as apm install. Pin a target with --target or declare one in apm.yml to opt in.
• -g carve-out preserved. apm install -g --mcp NAME writes user-scope (Copilot CLI to ~/.copilot/mcp-config.json, Codex CLI to ~/.codex/config.toml, Gemini CLI to ~/.gemini/settings.json) and bypasses the project-scope targets: gate by design. User-scope writes are not project-bound, so a project's targets: whitelist does not apply. Workspace-only runtimes (VS Code, Cursor, OpenCode) are skipped at user scope.
• Chose uniform gate over per-integrator self-gating. Pushing the directory check down into the codex/claude integrators would have preserved today's "binary-installed = configured" semantics symmetrically. Rejected because it kept two contracts (the gate AND the integrator checks) for one user-visible question.
• Conditional mcp_apm_config construction. Building the dict with a None placeholder for the unused key would be one line shorter, but the resolver's mutex check ("targets" in yaml_data and "target" in yaml_data) would falsely trip ConflictingTargetsError. Forwarding only the key the user declared keeps the mutex check honest.
• _rich_error without SystemExit(2) for malformed-targets. Mirrors the canonical voice at install/phases/targets.py:213 (red [x]) but skips the exit because _gate_project_scoped_runtimes runs mid-bundle in local_bundle_handler callers. Fail-closed-continue: the gate writes nothing for the malformed config, the bundle moves on.
• Fail closed, not fall through. A malformed or unresolvable targets: field could plausibly mean "I am mid-edit, please be permissive". Rejected: the user typed (or did not type) a whitelist; honoring an unresolvable whitelist as "write everything" is the opposite of intent.

Benefits

1. One contract for apm install, regardless of whether the dependency is a skill, an agent, an instruction, or an MCP server. UX parity is now demonstrable end-to-end.
2. Audit-caught: the call-site bypass is closed end-to-end with a regression test that exercises the full apm.yml -> APMPackage -> mcp_apm_config -> gate path.
3. Greenfield no longer silently routes user-scope writes. Users who never declared an opinion get the same fail-closed treatment they'd get from apm install.
4. CLI-logging UX is symmetric: [i] for drops, [x] red for malformed and unresolvable, with the same (active targets: ...) shape as the canonical provenance line.
5. _PROJECT_SCOPED_RUNTIMES constant deleted — one fewer footnote for future maintainers.
6. Bundle MCP wiring (_wire_bundle_mcp_servers) no longer silently drops every runtime when a CSV string sneaks in.

Validation

$ uv run --extra dev pytest tests/unit -n auto --dist worksteal -q
  ........................................................................ [100%]
  8480 passed, 1 warning, 28 subtests passed in 13.87s

$ uv run --extra dev pytest tests/integration/test_mcp_targets_gating_e2e.py -v
  test_targets_whitelist_copilot_suppresses_foreign_writes        PASSED
  test_targets_whitelist_multi_allows_listed_runtimes             PASSED
  test_greenfield_no_targets_no_signals_no_flag_writes_nothing    PASSED
  3 passed in 1.13s

$ uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/
  All checks passed!
  764 files already formatted

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	A repo declaring targets: [copilot] does not get MCP servers written to ~/.codex/config.toml even when a Codex binary sits on PATH	Secure by default, Governed by policy, Portability by manifest	tests/integration/test_mcp_targets_gating_e2e.py::test_targets_whitelist_copilot_suppresses_foreign_writes (regression-trap for #1335)	integration
2	The plural targets: form survives the apm.yml -> APMPackage -> mcp_apm_config -> gate plumbing end-to-end (audit-caught call-site bypass)	Secure by default, DevX	test_apm_package_targets_plural_forwards_through_call_site	unit
3	A repo with targets: [copilot, cursor] gets .cursor/mcp.json written and .codex/config.toml skipped	Multi-harness support, DevX	test_targets_whitelist_multi_allows_listed_runtimes	integration
4	A greenfield project (no targets:, no --target, no detectable harness signals) fails closed with NoHarnessError -- the same outcome apm install produces for that input. No silent [copilot] fallback, no user-scope writes the user never asked for	Secure by default, DevX, UX parity	test_greenfield_no_targets_no_signals_no_flag_writes_nothing	integration
5	A malformed targets: field (conflict, empty list, unknown target) fails closed: no MCP files are written and the user sees the same [x] red error voice as apm install	Secure by default, DevX	test_conflicting_targets_field_fails_closed test_empty_targets_list_fails_closed test_unknown_target_in_explicit_flag_fails_closed	unit
6	When the gate drops a runtime, the user sees [i] Skipped MCP config for X (active targets: Y) — same shape as the canonical provenance line	DevX	test_dropped_runtime_message_includes_active_targets	unit
7	Multi-target bundles whose internal CSV is "claude,copilot" are normalized correctly instead of silently dropping every runtime	Multi-harness support, Vendor-neutral	test_explicit_target_csv_string_normalized	unit
8	apm install -g --mcp NAME is unaffected by the project-scope gate -- user-scope writes are not project-bound	DevX, opt-in surface	test_user_scope_install_bypasses_project_gate	unit

How to test

• Check out main, scaffold a fresh apm.yml with targets: [copilot] and one MCP dependency. Ensure codex is on PATH. Run apm install. Observe: ~/.codex/config.toml gets a new entry (the bug).
• Check out this branch. Repeat the same steps. Observe: ~/.codex/config.toml is not touched, and the install log includes [i] Skipped MCP config for codex (active targets: copilot).
• On this branch, scaffold a project with no targets: field, only a .cursor/ directory, and copilot on PATH. Run apm install --mcp NAME. Observe: only .cursor/mcp.json is written; the log names copilot as skipped with the active-set parenthetical.
• On this branch, scaffold a greenfield project (no apm.yml targets:, no --target, no .github/copilot-instructions.md, no .cursor/, etc.) and run apm install --mcp NAME. Observe: red [x] Skipping all MCP config writes -- could not resolve active targets., no MCP files written anywhere — same fail-closed behavior as apm install on a greenfield.
• On this branch, run apm install -g --mcp NAME from a project that declares targets: [cursor]. Observe: user-scope MCP config is written for runtimes that support user scope (Copilot CLI, Codex CLI, Gemini CLI); the project-scope targets: whitelist does not apply.
• On this branch, set apm.yml to declare both target: copilot AND targets: [claude]. Run apm install. Observe: a red [x] error naming the conflict with the canonical remediation block, no MCP files written.
• Run uv run --extra dev pytest tests/unit/integration/test_mcp_integrator.py tests/integration/test_mcp_targets_gating_e2e.py -q. All tests pass (89 unit + 3 integration).

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This PR fixes MCP server installation to respect the project's targets:/target: whitelist from apm.yml, aligning MCP runtime selection with the existing target-resolution contract (Fixes #1335).

Changes:

• Updates MCPIntegrator._gate_project_scoped_runtimes() to use parse_targets_field() and (when targets are explicit) filter all runtimes against the resolved active target set.
• Adds unit tests covering plural/singular targets, explicit CLI override, and backward-compatible auto-detect behavior.
• Adds a changelog entry under ## [Unreleased] -> Fixed.

Show a summary per file

File	Description
src/apm_cli/integration/mcp_integrator.py	Implements explicit-target parsing/gating logic for MCP runtime selection.
tests/unit/integration/test_mcp_integrator.py	Adds unit tests for _gate_project_scoped_runtimes behavior across target configurations.
CHANGELOG.md	Documents the MCP install target-whitelist fix under Unreleased/Fixed.

Copilot's findings

Comments suppressed due to low confidence (3)

tests/unit/integration/test_mcp_integrator.py:817

• This comment includes a non-ASCII em dash character ("—"). The repo's encoding rules require printable ASCII in Python source; please replace with '-' or similar ASCII punctuation.

    def test_no_targets_gates_only_codex_claude(self, mock_at, tmp_path):
        # active_targets returns copilot only — codex/claude should be gated
        mock_at.side_effect = _fake_active_targets(["copilot"])

src/apm_cli/integration/mcp_integrator.py:980

• This comment line uses a non-ASCII em dash character ("—" in "EmptyTargetsListError — validation"). The repo's encoding rules require printable ASCII in Python source; please replace with '-' or similar ASCII punctuation.

            except Exception:
                # ConflictingTargetsError / EmptyTargetsListError — validation
                # should have caught this earlier; fall through to auto-detect.

src/apm_cli/integration/mcp_integrator.py:1003

• This comment uses a non-ASCII em dash character ("—" in "No explicit targets — backward-compat"). The repo's encoding rules require printable ASCII in Python source; please replace with '-' or similar ASCII punctuation.

        # No explicit targets — backward-compat: only gate project-scoped
        # runtimes whose directory marker was auto-detected.
        gated = [rt for rt in MCPIntegrator._PROJECT_SCOPED_RUNTIMES if rt in target_runtimes]

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

[danielmeppiel]

Fixed in 430bd9f (em dashes replaced with ASCII -- across the file).

[danielmeppiel]

Declined -- repo convention is to reference the issue number in CHANGELOG entries, not the PR number. Every recent entry follows this: (#1313), (#1289), (#1222), (#1299), (#1317). Keeping (#1335) for consistency.

[danielmeppiel]

Fixed in 430bd9f (right-arrow replaced with ->).

[danielmeppiel]

Stale -- this was already addressed in commit 82fe862 (one commit before your review landed). The except is now narrowed to (ConflictingTargetsError, EmptyTargetsListError) and fails closed (returns [] with a _rich_warning naming the field) rather than falling back to auto-detect. Two new tests assert the fail-closed behavior.

[danielmeppiel]

Fixed in 430bd9f (em/en dashes replaced with ASCII -- across the docstring and inline comments).

[danielmeppiel]

Fixed in 430bd9f (annotation widened to str | list[str] | None and CSV-string acceptance documented in the docstring).

[danielmeppiel]

Good catch -- legit regression. Fixed in 430bd9f by normalizing CSV strings to a list before calling active_targets(). Added test_explicit_target_csv_string_normalized as a regression guard.

[danielmeppiel]

APM Review Panel: ship_with_followups

PR #1336 makes targets: the single source of truth for MCP install gating across all runtimes, closing a silent-ignore bug (#1335) with a clean dual-mode gate and 10 regression-trap tests.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

Three independent personas (cli-logging, devx-ux, supply-chain) converged on the same code region (lines 978-994) with complementary but non-contradictory findings: the explicit-whitelist path logs gated-out runtimes at debug-only, and the exception fallback silently widens the write surface. This convergence is the strongest signal in the panel. The supply-chain concern is the most strategically significant: when a user declares targets: but the field is malformed, falling back to auto-detect writes MCP config to MORE runtimes than intended -- violating fail-closed semantics on a security-adjacent surface. The python-architect independently flagged the same bare-except as a code-health nit, reinforcing the supply-chain finding from a different angle.

The test-coverage expert confirms 10 unit tests pass (evidence: uv run pytest -k TestGateProjectScopedRuntimes -q -> 10 passed), proving the happy-path contract holds on this commit. However, the exception-fallback path has NO test (outcome: missing). On a critical-promise surface like install-pipeline gating, a missing regression trap on the degradation path is a real gap -- if the except clause is accidentally narrowed or removed in a future refactor, nothing breaks red. This elevates above a typical 'recommended' finding.

Architecture is clean and ships as-is (python-architect: no concerns beyond style nits). The doc-writer correctly identifies that the canonical MCP install page still describes only auto-detect semantics; this is a doc-drift gap that should land before or alongside the next release cut but does not block merge of the code fix itself.

Dissent. No true dissent between panelists. The only tension is severity calibration: supply-chain rates the silent-fallback-on-exception as 'recommended' while the fail-closed principle would normally warrant 'blocking'. I side with supply-chain's 'recommended' here because the malformed-targets scenario requires user error in apm.yml AND the fallback direction (auto-detect) matches pre-PR behavior -- it is not a regression, just a missed opportunity to tighten. The follow-up test + warning log should land promptly.

Aligned with: Portable by manifest (targets: field is now enforced as law for MCP writes -- manifest declares intent, CLI respects it everywhere); Secure by default (Partial: explicit whitelist correctly narrows scope, but exception fallback widens it silently. Follow-up to fail-closed would complete the principle); Multi-harness multi-host (All runtimes now participate in the same gating logic; no more special-casing of Codex/Claude only); OSS community-driven (Clean regression-trap suite models contribution bar; linked issue from community member resolved).

Growth signal. The oss-growth-hacker's story angle is strong: 'APM now treats your targets: field as law -- declare once, enforce across every harness.' This reinforces the 'one source of truth' narrative that differentiates APM from incumbent tools where config scattering is the norm. Worth echoing in release notes verbatim.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Clean dual-mode gating with correct falsy-list semantics; no architectural concerns. Ship.
CLI Logging Expert	0	1	1	Gated-out runtimes should surface at info/summary level, not debug; silent drops violate progressive-disclosure contract.
DevX UX Expert	0	1	1	targets: whitelist semantics align with npm/pip mental models; silent gating warrants an info-level hint in install summary.
Supply Chain Security Expert	0	1	1	Tightens MCP config surface correctly; one silent-fallback concern on malformed targets field.
OSS Growth Hacker	0	0	2	CHANGELOG entry is crisp and user-facing; minor framing tweak would sharpen the repostable hook for release notes.
Doc Writer	0	1	1	CHANGELOG entry is accurate and links #1335; the apm install MCP page still describes only auto-detect/directory gating and needs a sentence on targets: whitelist filtering.
Test Coverage Expert	0	1	0	10 unit tests cover the fix well; one recommended gap: no test exercises the parse_targets_field exception fallback path.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Test Coverage Expert] Add test for parse_targets_field exception fallback path in _gate_project_scoped_runtimes -- Missing regression trap on a critical-promise surface (install gating). If the except clause is narrowed or removed, no test fails. Evidence: outcome=missing on the degradation contract.
2. [Supply Chain Security Expert] Narrow except to known exceptions and consider fail-closed (return []) on unparseable targets -- Malformed manifest currently widens write surface via silent fallback to auto-detect; fail-closed is the safer default on a declared-intent surface.
3. [CLI Logging Expert] Promote explicit-whitelist gating message from debug to info/_rich_info -- User declared targets: and install silently skipped runtimes with no feedback. Violates progressive-disclosure contract; user should see confirmation their whitelist took effect.
4. [Doc Writer] Update docs/src/content/docs/consumer/install-mcp-servers.md with targets: whitelist semantics -- Canonical MCP install page still describes only auto-detect gating; the new whitelist behavior is a user-visible contract change that needs documentation before next release.
5. [OSS Growth Hacker] Polish CHANGELOG entry to lead with user promise rather than yaml key name -- Growth-optimized phrasing improves release-note repostability and reinforces the 'manifest as truth' narrative.

Architecture

classDiagram
    direction LR
    class MCPIntegrator {
        <<Namespace>>
        +_PROJECT_SCOPED_RUNTIMES tuple
        +_gate_project_scoped_runtimes(target_runtimes, ...) list~str~
    }
    class parse_targets_field {
        <<Pure Function>>
        +__call__(yaml_data dict) list~str~
    }
    class active_targets {
        <<IOBoundary>>
        +__call__(root, explicit_target) list~TargetProfile~
    }
    class TargetProfile {
        <<ValueObject>>
        +name str
        +root_dir str
        +auto_create bool
    }
    MCPIntegrator ..> parse_targets_field : calls (lazy import)
    MCPIntegrator ..> active_targets : calls (lazy import)
    active_targets ..> TargetProfile : returns list of
    class MCPIntegrator:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["_gate_project_scoped_runtimes(target_runtimes, ...)"] --> B{user_scope?}
    B -- Yes --> C[return target_runtimes unchanged]
    B -- No --> D["parse_targets_field(apm_config)"]
    D --> E{has_explicit_targets?}
    E -- Yes --> F["active_targets(root, config_target)"]
    F --> G["out = [rt for rt in target_runtimes if rt in active]"]
    G --> H{dropped any?}
    H -- Yes --> I["[I/O] _log.debug gated-out list"]
    I --> J[return out]
    H -- No --> J
    E -- No --> K{"any _PROJECT_SCOPED_RUNTIMES in target_runtimes?"}
    K -- No --> L[return target_runtimes unchanged]
    K -- Yes --> M["active_targets(root, None)"]
    M --> N{"gated rt in active?"}
    N -- Yes --> O[keep rt]
    N -- No --> P["[I/O] _log.debug rt gated out"]
    P --> Q[remove rt from out]
    O --> R[return out]
    Q --> R

Loading

Recommendation

Merge as-is -- the fix correctly closes #1335, architecture is clean, and 10 tests prove the happy path. Track the exception-path test + fail-closed narrowing as immediate follow-ups (same sprint); the info-level logging and doc update should land before the next release cut. None of these block the bug fix from reaching users.

---

Full per-persona findings

Python Architect

• [nit] Bare except Exception swallows unexpected errors from parse_targets_field at src/apm_cli/integration/mcp_integrator.py:978
  The catch at line 978 catches ConflictingTargetsError and EmptyTargetsListError but also any unexpected bug (KeyError, TypeError, AttributeError). Narrowing to the two known exceptions makes debugging easier if parse_targets_field evolves.
  Suggested: except (ConflictingTargetsError, EmptyTargetsListError):
• [nit] config_target relies on empty-list falsiness -- a brief comment would aid future readers at src/apm_cli/integration/mcp_integrator.py:983
  The expression explicit_target or explicit_from_config or None silently falls through when explicit_from_config is [] (returned by parse_targets_field for 'neither key present'). Correct Python but intent is non-obvious.

CLI Logging Expert

• [recommended] Targets-whitelist gating emits debug-only; user gets no explanation for skipped runtimes at src/apm_cli/integration/mcp_integrator.py:994
  Traffic-light rule: info (blue) is for things the user 'should know'. When targets: [cursor] and install of MCP for vscode+cursor+windsurf drops 2 of 3 silently, this violates 'lead with the outcome' and 'name the thing'. Debug is for verbose; gating is a user-visible behavioral outcome that passes 'So What?' (user can edit targets or use --target). The NEW explicit-whitelist path (line 994) is the user's own declaration taking effect -- they deserve confirmation.
  Suggested: Promote to _rich_info or logger.info at default verbosity: [i] Skipped MCP config for vscode, windsurf -- not in targets whitelist. Or surface via DiagnosticCollector in the install summary block.
• [nit] Debug message on parse_targets_field exception swallows context at src/apm_cli/integration/mcp_integrator.py:981
  Line 981 logs without exc_info=True. For verbose/agent consumers, the actual exception type is valuable diagnostic signal. Other exception-path debugs in this file (line 937) use exc_info=True.
  Suggested: _log.debug("parse_targets_field failed; falling back to auto-detect", exc_info=True)

DevX UX Expert

• [recommended] Gated runtimes are only logged at DEBUG; user gets no feedback when targets: silently drops harnesses at src/apm_cli/integration/mcp_integrator.py:988
  Package-manager mental model (Rule 4 - Recovery): when a user sets targets: [claude] and runs apm install, they should see confirmation that the whitelist was honored. Without this, a user migrating from old behavior will not understand why their copilot config stopped updating. npm logs which workspace it installs into; pip --target echoes the path.
  Suggested: Promote the 'Targets whitelist gated out' message from _log.debug to a user-facing info line on the happy path.
• [nit] CHANGELOG entry could note the backward-compat split more explicitly at CHANGELOG.md
  Entry says 'All runtimes are now filtered when an explicit whitelist is present' but does not say 'repos without a targets: field keep the previous auto-detect behavior'.
  Suggested: Append: 'Repos without a targets: field retain previous auto-detect semantics (no behavior change).'

Supply Chain Security Expert

• [recommended] Bare except on parse_targets_field silently widens write surface on malformed manifest at src/apm_cli/integration/mcp_integrator.py:978
  If apm.yml contains a malformed targets field, parse_targets_field raises, the except catches it, has_explicit_targets stays false, and the code falls through to auto-detect -- writing MCP config to MORE runtimes than intended. Violates fail-closed: when the user declared targets but the declaration is unparseable, the safe default is to write NOTHING (empty list), not to fall back to permissive auto-detect.
  Suggested: except (ConflictingTargetsError, EmptyTargetsListError): _log.warning('...'); return []
• [nit] user_scope bypass is undocumented from a security-contract perspective at src/apm_cli/integration/mcp_integrator.py:967
  user_scope=True short-circuits all gating. Likely correct (user-global config is the user's own scope), but security model doc does not state this. A one-line in-code comment would prevent future reviewers from questioning it.
  Suggested: Add: # Security note: user-scope writes target ~/.config paths the user owns globally; gating applies only to project-scoped writes that could affect shared repos.

OSS Growth Hacker

• [nit] CHANGELOG entry buries the user promise behind implementation detail at CHANGELOG.md:12
  Entry leads with the yaml key name. Growth-optimized phrasing leads with the user promise.
  Suggested: Reword: 'MCP server installation now respects the explicit targets: whitelist for all runtimes; previously only Codex and Claude Code were gated, and only when the singular target: key was used. ([BUG] targets: in apm.yml is not respected for MCP server installation #1335)'
• [nit] Test class is exemplary contribution-pattern material worth calling out in release notes at tests/unit/integration/test_mcp_integrator.py:734
  TestGateProjectScopedRuntimes is a textbook regression-trap suite. Highlighting models the contribution bar.

Auth Expert -- inactive

No auth fast-path file touched; change is a runtime-whitelist gate in MCPIntegrator with no impact on token resolution, host classification, or credential flows.

Doc Writer

• [recommended] docs/src/content/docs/consumer/install-mcp-servers.md does not document the new targets: whitelist gating for MCP install. at docs/src/content/docs/consumer/install-mcp-servers.md:89
  The 'What apm install writes to disk' section tells the reader APM writes per-runtime MCP config 'For every harness APM detects'. After this PR, an explicit targets: / target: field in apm.yml (or --target on the CLI) is also a hard whitelist for ALL runtimes during MCP install. That is a user-visible contract change (the explicit reason [BUG] targets: in apm.yml is not respected for MCP server installation #1335 was filed). Page is the canonical source for MCP install behavior under the 'state once, reference elsewhere' rule.
  Suggested: After the 'Cursor, Gemini, and OpenCode are opt-in by directory' paragraph, add: 'When apm.yml declares targets: (or --target is passed), MCP install treats that list as a hard whitelist -- runtimes outside the list are skipped even if their harness directory is present. With no targets: field, only project-scoped runtimes (Codex, Claude Code) are gated for backward compatibility ([BUG] targets: in apm.yml is not respected for MCP server installation #1335).'
• [nit] CHANGELOG entry could name the user-facing symptom alongside the fix. at CHANGELOG.md:12
  As polish, a one-clause symptom ('e.g. targets: [copilot] no longer leaks an MCP write to ~/.codex/config.toml') would let readers self-diagnose without opening the issue.

Test Coverage Expert

• [recommended] No test exercises parse_targets_field exception fallback in _gate_project_scoped_runtimes at tests/unit/integration/test_mcp_integrator.py
  Lines 974-977 catch any exception from parse_targets_field and degrade to auto-detect. No test in TestGateProjectScopedRuntimes supplies a config dict triggering this path (e.g., {'target':'claude','targets':['copilot']} raising ConflictingTargetsError). Verified via grep on tests/unit/integration/test_mcp_integrator.py for ConflictingTargetsError/EmptyTargetsListError/exception/fallback -- only hit is unrelated test_plain_string_fallback. The exception tests in test_target_resolution_v2.py cover parse_targets_field in isolation but not the degrade-to-auto-detect behavior in the MCP gate. If the except clause is accidentally deleted or narrowed, no test fails.
  Suggested: Add test_conflicting_targets_falls_back_to_auto_detect that patches parse_targets_field to raise ConflictingTargetsError and asserts the method returns the legacy auto-detect result.
  Proof (passed): tests/unit/integration/test_mcp_integrator.py::TestGateProjectScopedRuntimes::test_targets_plural_filters_unlisted_runtimes -- proves: targets: [claude] in apm.yml gates all unlisted runtimes out of MCP install
  Proof (missing at): ::TestGateProjectScopedRuntimes::test_conflicting_targets_falls_back_to_auto_detect -- proves: When parse_targets_field raises, install degrades gracefully to auto-detect rather than crashing

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

[danielmeppiel]

Pushed 3a5bc4e -- per maintainer guidance, the MCP gate now mirrors the apm install targets engine instead of carrying an asymmetric BC path for codex/claude.

What changed

• Dropped _PROJECT_SCOPED_RUNTIMES = ("codex", "claude") and the conditional branch.
• The active set returned by active_targets() (explicit targets: / --target > directory detection > [copilot] fallback) is now the whitelist for every runtime, no matter whether targets: is declared.
• Aliased the vscode runtime to the copilot canonical target inside the gate -- mirrors the alias active_targets honors for explicit_target (targets.py:776).
• Updated docs (install-mcp-servers.md) and CHANGELOG to describe the unified UX.

Behavior delta vs main

Scenario	Before this PR	After this PR
targets: [copilot], codex binary on PATH	Wrote ~/.codex/config.toml (bug #1335)	Skipped (whitelist)
No targets:, copilot binary, no .codex/	Skipped (BC special case)	Skipped (uniform gate)
No targets:, cursor binary, no .cursor/, no .github/	Wrote .cursor/mcp.json	Falls back to copilot (greenfield default)
No targets:, copilot binary AND .cursor/ only	Wrote both copilot + cursor MCP	Cursor only (directory detection is the whitelist)

The last row is the one user-visible behavior shift. It's the same contract apm install already enforces for skills/agents/instructions in the same project, so the surfaces are now consistent.

Verification: pytest tests/unit tests/test_console.py -n auto 8476/8476 green; ruff check + ruff format --check silent.

[danielmeppiel]

APM Review Panel: ship_with_followups

Second-pass review confirms PR #1336 closes the #1335 trust break cleanly: 0 blocking, 3 deferrable recommended items, all six prior-pass recommendations applied; the targets: contract now means what it says across every MCP install entry point.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

The panel converges. Supply-chain-security returns clean (a single chokepoint now governs three MCP install entry points, with the -g carve-out intentional and documented). Python-architect confirms the first-pass recommendations landed cleanly -- shared RUNTIME_TO_CANONICAL_TARGET table, plural targets: threaded end-to-end, gate reduced to thin delegation over resolve_targets(). DevX-UX, cli-logging, and doc-writer each return a single nit on otherwise-shipped surfaces. Test-coverage's two affirmations carry the strongest evidentiary weight in the pack: tests/integration/test_mcp_targets_gating_e2e.py runs the exact #1335 bug shape against real on-disk layouts with no gate or resolver mocks, and test_apm_package_targets_plural_forwards_through_call_site is a load-bearing regression test at the precise call-site layer where the audit-caught bypass lived. Per the test-evidence weighting rule, those passing integration tests are the proof the user-facing promise holds on this commit.

The three recommended-tier items are all deferrable polish: (a) growth's CHANGELOG split is a release-cut convenience, not a correctness issue; (b) python-architect's gate-function-size note is explicitly flagged as "defer until a third axis of variation appears"; (c) python-architect's APMPackage.__post_init__ mutex guard is a defense-in-depth hardening for a code path that the canonical builder (from_apm_yml) and the gate already cover. None of the three change the verdict on this PR; all three are worth tracking as follow-ups. No cross-persona dissent to arbitrate.

Aligned with: portable-by-manifest (targets: in apm.yml now governs MCP server writes for all runtimes the same way it already governed skills and agents -- one manifest, one rule, every harness); secure-by-default (greenfield projects with no targets: and no detected harness now fail closed with [x] instead of silently writing copilot config); governed-by-policy (project-scope writes are gated through a single _gate_project_scoped_runtimes chokepoint covering all three MCP install entry points); pragmatic-as-npm (the -g user-scope carve-out preserves the npm-style escape hatch -- explicit, documented, and tested).

Growth signal. The launch beat is "apm install now enforces the same targets: contract for MCP servers that it already enforces for skills and agents -- one manifest, one rule, every harness." The greenfield fail-closed behavior is the proof point; the -g carve-out shows the design is intentional. Worth amplifying in the next release notes.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	First-pass architectural recommendations applied cleanly: shared RUNTIME_TO_CANONICAL_TARGET dict (3 call sites, no drift), plural targets: threaded end-to-end. The gate is now a thin delegation over resolve_targets().
CLI Logging Expert	0	0	1	CLI output contract holds end-to-end: drop-line double-space lock, [i]/[x] symbol contract, and voice parity with canonical install phase all confirmed. Ship.
DevX UX Expert	0	0	1	MCP install now matches apm install end-to-end: same gate, same drop-line shape, same greenfield fail-closed voice, -g carve-out documented. Ship.
Supply Chain Security	0	0	0	Security hardening: closes silent user-scope config-write trust break (#1335) at a single chokepoint covering all three MCP install entry points; -g carve-out is intentional and documented.
OSS Growth Hacker	0	1	2	Behavior-change comms are solid; CHANGELOG paragraph is too dense for release scanning -- recommend splitting before the release cut.
Doc Writer	0	0	2	Docs accurate against the gating code; one nit on the -g carve-out paragraph omitting Claude/Windsurf from the user-scope examples.
Test Coverage Expert	0	2	2	Coverage is honest: 17 unit + 3 integration tests cover every changed branch; integration tier run-verified. Two PR-body test-name labels drift from the actual symbols.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 4 follow-ups

1. [OSS Growth Hacker] Split the CHANGELOG entry into 2-3 sub-bullets before the release cut: (1) targets: whitelist now governs MCP writes for all runtimes, (2) greenfield fail-closed behavior change with remediation one-liner, (3) drop-line observability shape. -- Single 12-line paragraph buries the greenfield behavior change -- the one users most need to act on -- in the middle of audit detail. Splitting now means the release author writes the announcement in zero edits instead of re-triaging.
2. [Python Architect] Add __post_init__ mutex check to APMPackage rejecting simultaneous target + targets. -- Same class of silent-bypass bug as [BUG] targets: in apm.yml is not respected for MCP server installation #1335: the gate enforces the mutex correctly, but any caller constructing APMPackage(...) directly (not via from_apm_yml) can produce a wrong-shape instance that bypasses the gate's check. Defense-in-depth at the dataclass boundary surfaces the bug at the call site rather than three layers deep.
3. [Test Coverage Expert] Lock the [x] symbol prefix on greenfield + malformed-targets tests, and assert the user-visible voice on the AmbiguousHarnessError branch. -- Two small assertion additions close silent-drift gaps on the secure-by-default fail-closed surface: a future contributor swapping symbol='error' for symbol='warning' on these branches would change the traffic-light color without failing any test.
4. [Doc Writer] Fix the -g carve-out paragraph in install-mcp-servers.md (Claude + Windsurf are user-scope runtimes too) and drop the trailing (#1335) annotation. -- Two small docs corrections that prevent users from concluding the user-scope routing is more limited than it actually is.

Architecture

classDiagram
    direction LR
    class MCPIntegrator {
      <<Service>>
      +install(...)
      -_gate_project_scoped_runtimes(target_runtimes, user_scope, project_root, apm_config, explicit_target) list~str~
    }
    class APMPackage {
      <<Aggregate>>
      +name str
      +target str|list|None
      +targets list~str~|None
      +scripts dict
      +from_apm_yml(path) APMPackage
    }
    class parse_targets_field {
      <<PureFn>>
    }
    class resolve_targets {
      <<PureFn>>
      +flag list~str~|None
      +yaml_targets list~str~|None
    }
    class RUNTIME_TO_CANONICAL_TARGET {
      <<SharedTable>>
    }
    class ConflictingTargetsError
    class EmptyTargetsListError
    class UnknownTargetError
    class NoHarnessError
    class AmbiguousHarnessError
    class CommandsInstall {
      <<EntryPoint>>
      -_install_apm_packages(ctx, outcome)
    }
    class LocalBundleHandler {
      <<EntryPoint>>
      -_wire_bundle_mcp_servers(...)
    }
    CommandsInstall ..> APMPackage : reads
    CommandsInstall ..> MCPIntegrator : invokes install
    LocalBundleHandler ..> MCPIntegrator : invokes install
    MCPIntegrator ..> parse_targets_field : delegates step 1
    MCPIntegrator ..> resolve_targets : delegates step 3
    MCPIntegrator ..> RUNTIME_TO_CANONICAL_TARGET : reads alias map
    MCPIntegrator ..> ConflictingTargetsError : catches
    MCPIntegrator ..> EmptyTargetsListError : catches
    MCPIntegrator ..> UnknownTargetError : catches
    MCPIntegrator ..> NoHarnessError : catches
    MCPIntegrator ..> AmbiguousHarnessError : catches
    APMPackage ..> parse_targets_field : validates targets
    class MCPIntegrator:::touched
    class APMPackage:::touched
    class RUNTIME_TO_CANONICAL_TARGET:::touched
    class CommandsInstall:::touched
    class LocalBundleHandler:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install --mcp NAME<br/>(commands/install.py)"] --> B{"ctx.scope == USER?"}
    B -- yes --> Z["user_scope=True<br/>bypass gate, write ~/.config"]
    B -- no --> C["APMPackage.from_apm_yml<br/>parses target AND targets"]
    C --> D["build mcp_apm_config conditionally<br/>only the key user declared"]
    D --> E["MCPIntegrator.install"]
    E --> F["_gate_project_scoped_runtimes"]
    F --> G{"step 1: parse_targets_field(apm_config)"}
    G -- ConflictingTargetsError --> X1["_rich_error red [x]<br/>return []"]
    G -- EmptyTargetsListError --> X1
    G -- UnknownTargetError --> X1
    G -- ok or empty --> H["step 2: normalize CSV explicit_target<br/>apply RUNTIME_TO_CANONICAL_TARGET to flag"]
    H --> I["step 3: resolve_targets(root, flag, yaml_targets)"]
    I -- NoHarnessError --> X2["_rich_error red [x]<br/>'Skipping all MCP config writes'<br/>return []"]
    I -- AmbiguousHarnessError --> X2
    I -- ok --> J["intersect target_runtimes<br/>with active via RUNTIME_TO_CANONICAL_TARGET"]
    J --> K{"any dropped?"}
    K -- yes --> L["_rich_info [i] Skipped MCP config for X<br/>(active targets Y)"]
    K -- no --> M["return filtered runtime list"]
    L --> M
    M --> N["per-runtime adapters write configs"]
    classDef failclosed fill:#f8d7da,stroke:#842029
    classDef auditfix fill:#fff3cd,stroke:#856404
    class X1 failclosed
    class X2 failclosed
    class C auditfix
    class D auditfix

Loading

sequenceDiagram
    participant User
    participant CLI as commands/install.py
    participant Pkg as APMPackage
    participant Gate as MCPIntegrator gate
    participant Parse as parse_targets_field
    participant Resolver as resolve_targets
    User->>CLI: apm install --mcp NAME
    CLI->>Pkg: from_apm_yml(apm.yml)
    Pkg-->>CLI: APMPackage targets=copilot,claude
    CLI->>CLI: build mcp_apm_config conditional plural
    CLI->>Gate: install with apm_config=targets list
    Gate->>Parse: parse_targets_field(apm_config)
    Parse-->>Gate: copilot,claude
    Gate->>Gate: normalize and canonicalize flag
    Gate->>Resolver: resolve_targets(root, flag, yaml_targets)
    Resolver-->>Gate: ResolvedTargets active=copilot,claude
    Gate->>Gate: intersect runtimes with active
    Gate-->>CLI: filtered runtimes
    CLI->>User: write only whitelisted MCP configs

Loading

Recommendation

Ship. Every first-pass recommendation has been applied; second-pass surfaces no blocking issues and no cross-persona dissent. The integration test at tests/integration/test_mcp_targets_gating_e2e.py is the load-bearing proof that the #1335 bug shape cannot regress silently. Track the CHANGELOG split as the highest-signal post-merge follow-up so the release announcement writes itself; the APMPackage.__post_init__ guard and the test-coverage tightening can land in a small follow-up PR.

---

Full per-persona findings

Python Architect

• [recommended] Gate function size at the extract threshold (~120 lines, 3 logical phases) at src/apm_cli/integration/mcp_integrator.py:943
_gate_project_scoped_runtimes now does five things in one body: parse YAML targets, normalize CSV/list explicit_target and apply alias mapping, delegate to resolve_targets, render error voice on three exception families, intersect+canonicalize+log dropped runtimes. Step comments keep it readable but it's at the threshold where extracting helpers would aid testability. Not blocking -- the function is correct. Flagged because the next strictness extension will push past the boundary where step-comments stop helping.
  Suggested: Defer until a third axis of variation appears. At that point, extract _parse_yaml_targets, _normalize_explicit_target, and _intersect_runtimes_with_active so each branch can be unit-tested without setting up the full tmp_path + signal-seeding rig.
• [recommended] APMPackage allows simultaneous target + targets -- mutex only enforced at gate-time at src/apm_cli/models/apm_package.py:92
  The dataclass exposes both target: str | list[str] | None AND targets: list[str] | None. The docstring on target notes "never both populated" but the constructor has no __post_init__ enforcing this. from_apm_yml is the canonical builder and does the right thing, but any code path that constructs APMPackage(...) directly can produce a wrong-shape instance that passes silently until the MCP gate re-runs parse_targets_field. This is the same class of silent-bypass bug as [BUG] targets: in apm.yml is not respected for MCP server installation #1335 -- the gate IS correct, but a malformed dataclass slipping through means the gate's mutex check never fires for that code path.
• [nit] explicit_target: str | list[str] | None union is now wider than any production caller needs at src/apm_cli/integration/mcp_integrator.py:951
  Bundle wiring was migrated to forward list[str] (local_bundle_handler.py:380). install.py forwards ctx.target (already canonical). The CSV-string normalization branch is now defensive-only -- there is no production caller that takes the str-with-comma path.

CLI Logging Expert

• [nit] Greenfield + malformed-targets tests assert message text but not the [x] symbol prefix at tests/unit/integration/test_mcp_integrator.py
test_dropped_runtime_message_includes_active_targets locks the [i] symbol with assert "[i]" in out, but test_no_signal_no_targets_no_flag_fails_closed and test_unknown_target_in_yaml_fails_closed only assert the message strings. A future contributor swapping symbol="error" for symbol="warning" (yellow [!]) on these calls would silently change the traffic-light color from red to yellow without failing any test.
  Suggested: Add assert "[x]" in out (and ideally assert "[i]" not in out) to test_no_signal_no_targets_no_flag_fails_closed and test_unknown_target_in_yaml_fails_closed.

DevX UX Expert

• [nit] --mcp NAME help text names the project-scope gate but not the -g carve-out at src/apm_cli/commands/install.py:982
  A user reading apm install --help who types apm install -g --mcp NAME only learns from the docs page that the project-scope targets: whitelist does not apply at user scope. Surfacing it inline (~12 chars: e.g. "with -g writes user-scope and bypasses the gate") would close the last gap between the package-manager mental model and the help surface.
  Suggested: Append a short clause: "...skips others with [i]; with -g writes user-scope and bypasses the project-scope gate."

Supply Chain Security Expert

No findings.

OSS Growth Hacker

• [recommended] CHANGELOG entry is a single 12-line paragraph; unreadable in release notes without re-editing. at CHANGELOG.md:12
  The CHANGELOG is raw material for release narratives. A single dense paragraph forces the release author to re-triage what matters. Users scan changelogs for "does this affect me?" -- the current wall buries the greenfield behavior change (the one most likely to surprise someone) mid-paragraph after the audit detail.
  Suggested: Break the single bullet into sub-bullets: (1) targets: whitelist now governs MCP writes for all runtimes (the fix), (2) Greenfield projects with no targets: and no detected signals now fail closed with [x] instead of silently writing copilot config -- add --target copilot or declare targets: in apm.yml to restore the previous behavior (the behavior change users need to act on), (3) Drop lines now show [i] Skipped MCP config for X (active targets: Y) (observability).
• [nit] Docs remediation for greenfield is accurate but could be a copyable one-liner. at docs/src/content/docs/consumer/install-mcp-servers.md:119
  The page says "Pin a target with --target or declare one in apm.yml" -- correct, but for a user hitting the new [x] error in a greenfield project, a single copy-paste line like apm install --target copilot --mcp NAME or a 2-line apm.yml snippet would convert confusion into action faster.
• [nit] Release story angle: "one contract for all runtimes" is the repostable hook.
  Side-channel for the CEO: when this ships, the launch beat is "apm install now enforces the same targets: contract for MCP servers that it already enforces for skills and agents -- one manifest, one rule, every harness." The greenfield strictness is the proof point (fail-closed is the secure default). The -g carve-out is the escape hatch that shows the design is intentional, not accidental.

Auth Expert -- inactive

no auth surface touched -- diff is gate logic + plural targets plumbing + docs/tests; none of auth.py, token_manager.py, azure_cli.py, github_downloader.py, marketplace/client.py, github_host.py, install/validation.py, install/pipeline.py, or deps/registry_proxy.py appear in the changed files

Doc Writer

• [nit] -g carve-out paragraph names only 3 of 5 user-scope runtimes; Claude and Windsurf are omitted from the example list. at docs/src/content/docs/consumer/install-mcp-servers.md:124
install-mcp-servers.md lines 122-128 illustrate the user-scope routing with Copilot CLI, Codex CLI, and Gemini CLI, then explicitly enumerates VS Code/Cursor/OpenCode as workspace-only. Claude (writes ~/.claude.json with -g) and Windsurf (writes ~/.codeium/windsurf/mcp_config.json) also receive -g writes per the adapter source.
  Suggested: Either drop the inline runtime list and write "routes the write to each runtime user-scope MCP config (see the harness table above)", or extend the list to all five user-scope runtimes.
• [nit] Issue ref (#1335) embedded mid-prose at end of greenfield/malformed paragraph reads as a stray annotation. at docs/src/content/docs/consumer/install-mcp-servers.md:120
  Line 120 ends the greenfield paragraph with "... declare one in apm.yml. ([BUG] targets: in apm.yml is not respected for MCP server installation #1335)". Trailing (#1335) reads like residual changelog drift rather than a deliberate citation.
  Suggested: Remove the trailing (#1335) or relocate it to a single see-also footnote.

Test Coverage Expert

• [nit] PR body Scenario Evidence rows 5 and 8 cite test names that don't exist in the diff at tests/unit/integration/test_mcp_integrator.py
  Row 5 cites test_unknown_target_in_explicit_flag_fails_closed; the actual symbol is test_unknown_target_in_yaml_fails_closed. Row 8 cites test_user_scope_install_bypasses_project_gate; the actual symbol is test_user_scope_bypasses_all_gating. Both real tests do prove what the rows claim, but the labels mislead anyone clicking through to verify.
  Suggested: Update PR body Scenario Evidence row 5 to test_unknown_target_in_yaml_fails_closed and row 8 to test_user_scope_bypasses_all_gating.
  Proof (passed): tests/unit/integration/test_mcp_integrator.py::TestGateProjectScopedRuntimes::test_unknown_target_in_yaml_fails_closed -- proves: Malformed targets: with unknown canonical name fails closed via the yaml-parse path; explicit-flag-with-unknown path is NOT directly covered. [secure-by-default,devx]
• [nit] AmbiguousHarnessError is caught but tested only via the no-flag/no-yaml/multi-signal path at tests/unit/integration/test_mcp_integrator.py:870
  The except clause except (NoHarnessError, AmbiguousHarnessError) is exercised for AmbiguousHarnessError only via test_no_targets_with_ambiguous_signals_fails_closed. The error voice/output is not asserted on the Ambiguous branch (only result == [] is). If a future refactor split the two exceptions into different handlers with different messages, no test would notice the AmbiguousHarnessError message regressing.
  Suggested: Add out = capsys.readouterr().out; assert 'Skipping all MCP config writes' in out so the ambiguous branch's user-visible voice is locked in symmetrically with the greenfield branch.
• [recommended] Affirmation: integration coverage of the [BUG] targets: in apm.yml is not respected for MCP server installation #1335 regression is run-verified at the install-pipeline floor tier at tests/integration/test_mcp_targets_gating_e2e.py
tests/integration/test_mcp_targets_gating_e2e.py runs against real on-disk project layouts with real MCPIntegrator.install (no gate or resolver mocks) and asserts the negative case. This is the exact bug shape from [BUG] targets: in apm.yml is not respected for MCP server installation #1335 and the test would fail loudly if any future refactor reintroduced permissive behavior.
• [recommended] Affirmation: call-site bypass (devx-ux B1 audit) has a load-bearing regression test at tests/unit/integration/test_mcp_integrator.py
test_apm_package_targets_plural_forwards_through_call_site exercises the actual code path: real apm.yml with targets: plural -> APMPackage.from_apm_yml -> exact dict-construction logic from commands/install.py:1797 -> assert parse_targets_field sees the plural list. Catches a regression at the layer where the bug actually was.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}