fix(update): reconcile MCP/LSP servers after apm update

[cffnpwr]

Description

apm update never ran the MCP/LSP reconciliation that apm install performs: it calls _install_apm_dependencies directly, while MCPIntegrator / run_lsp_integration were only invoked from _install_apm_packages. Because the lockfile phase intentionally carries mcp_servers / mcp_configs forward unreconciled (install/phases/lockfile.py::_preserve_existing_mcp_state), MCP servers that dependencies no longer declare survived apm update indefinitely -- in apm.lock.yaml and in runtime configs (.mcp.json etc.). LSP had the same gap in the opposite direction: lsp_servers are not carried forward, so LSP servers never reached the lockfile via apm update at all.

Changes:

• Extract the MCP integration block from commands/install.py::_install_apm_packages into install/mcp/integration.py::run_mcp_integration(), mirroring the existing install/lsp/integration.py::run_lsp_integration(). The transitive-policy preflight now raises PolicyBlockError instead of calling sys.exit(1); both callers catch it, report, and exit non-zero (no behaviour change for apm install).
• apm update now runs MCP and LSP integration after the consent gate -- only when the user accepted the plan (--yes or interactive confirm). It snapshots the old lockfile state before the pipeline regenerates it, re-parses the on-disk manifest after revision pins are applied, then reconciles (install -> remove stale -> update lockfile).
• docs/reference/lockfile-spec.md: mcp_servers / lsp_servers are now described as reflecting the manifest "as of the last install or update".

Security notes:

• apm update now writes and prunes runtime MCP/LSP configs the same way apm install does; remove_stale runs as a trust-hygiene step.
• The merged (direct + transitive) MCP set goes through the same policy preflight. apm update has no --no-policy / --trust-transitive-mcp flags, so both default to the stricter side, and allowExecutables gating now applies on update too.
• Dry-run, declined, and non-TTY runs touch nothing (covered by tests).

Known limitation: reconciliation only runs when a plan is applied. An apm update where nothing changed early-returns and leaves a pre-existing orphan in place until the next applied change (or an apm install); pruning without consent would contradict the command's "no changes applied" promise.

Out of scope: flag parity for --runtime / --exclude / --trust-transitive-mcp / --no-policy on apm update; apm lock (by design it resolves packages only).

Fixes #2077

Type of change

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

Testing

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

uv run pytest tests/unit tests/test_console.py: 18153 passed. New tests: tests/integration/test_update_mcp_lsp_prune.py (update prunes an orphaned MCP server from the lockfile; dry-run leaves state untouched; a declared LSP server is recorded via update) and tests/unit/install/test_mcp_integration_extraction.py (extraction parity for every branch, including PolicyBlockError propagation). ruff check and ruff format --check pass.

Spec conformance (OpenAPM v0.1)

If this PR changes behaviour that an OpenAPM v0.1 req-XXX covers,
confirm the three-step ritual (see CONTRIBUTING.md "Adding or
changing a normative requirement"):

• Spec edit: docs/src/content/docs/specs/openapm-v0.1.md updated
  (new/changed <a id="req-XXX"></a> anchor + prose + Appendix C
  row).
• Manifest edit: docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.yml
  updated.
• Test edit: a @pytest.mark.req("req-XXX") test under
  tests/spec_conformance/ added or extended.
• CONFORMANCE.{md,json} regenerated via
  uv run --extra dev python -m tests.spec_conformance.gen_statement
  and committed.
• N/A -- this PR does not change OpenAPM-observable behaviour.

apm-spec-waiver: install/mcp/integration.py is a behaviour-preserving extraction of the MCP integration block from commands/install.py (apm install semantics unchanged); the update-side fix alters no req-covered behaviour (req-rs-011/012 untouched)

[cffnpwr]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

This PR fixes a behavioral gap where apm update bypassed the post-install reconciliation steps that apm install performs, causing MCP servers to linger as orphans in apm.lock.yaml (and runtime configs) and preventing LSP servers from being recorded via update. It extracts MCP reconciliation into a shared helper and invokes both MCP and LSP integration from the update flow after the consent gate, aligning apm update with the install pipeline’s integration semantics.

Changes:

• Extract MCP reconciliation logic into run_mcp_integration() and wire it into apm install and apm update.
• Add apm update integration glue to snapshot prior lockfile MCP/LSP state, re-parse the manifest after pin application, and reconcile servers/configs.
• Add unit + integration tests for extraction parity and for update pruning/recording behavior; update lockfile spec docs to reflect “last install or update”.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/unit/install/test_mcp_integration_extraction.py	Verifies branch-by-branch parity for the extracted run_mcp_integration() behavior, including policy-block propagation.
tests/integration/test_update_mcp_lsp_prune.py	Regression coverage ensuring apm update --yes prunes orphaned MCP entries, preserves state on --dry-run, and records declared LSP servers.
src/apm_cli/install/mcp/integration.py	New shared MCP reconciliation helper used by both install and update flows.
src/apm_cli/install/mcp/init.py	Exposes run_mcp_integration from the MCP install subpackage.
src/apm_cli/commands/update.py	Adds MCP+LSP reconciliation to apm update after consent, using a snapshot of pre-update lockfile state.
src/apm_cli/commands/install.py	Replaces the inline MCP block with a call to run_mcp_integration() while preserving existing policy-failure behavior.
docs/src/content/docs/reference/lockfile-spec.md	Updates lockfile field descriptions to reflect install/update parity for MCP/LSP server recording.

[sergio-sisternes-epam]

APM Review Panel: ship_with_followups

Community-contributed fix closes silent MCP/LSP config rot in apm update; ship with follow-up issues for doc gaps and CHANGELOG entry.

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

This is a clean, well-scoped community contribution that closes a real correctness gap: apm update silently left orphaned MCP servers in the lockfile and never wrote LSP servers at all, violating the user promise that update and install produce equivalent runtime state. The fix follows the existing LSP integration pattern structurally, the security panelist gives a clean bill, and the test-coverage-expert confirms the critical regression surface is covered by three passing CliRunner integration tests. No specialist disagreements rise to the level of blocking.

The substantive findings cluster into three tiers. First, the CHANGELOG entry (oss-growth-hacker) and doc gaps (doc-writer: lifecycle table, update.md Behavior section, flag parity documentation) are real but post-merge fixable -- they do not change shipped behavior. Second, the logging parity issue (cli-logging-expert: _rich_error vs logger.error) is a style inconsistency inherited from how the code was extracted, not a user-visible correctness regression; it should be harmonized but does not block. Third, the flag asymmetry between install and update (devx-ux-expert: --no-policy, --trust-transitive-mcp, --runtime absent from update) is the most strategically important finding -- it means a user who hits a PolicyBlockError during update has no escape hatch, unlike during install. This is a real UX gap but it is pre-existing scope: this PR did not introduce the flags on install, and adding them to update is a design decision that deserves its own issue rather than scope-creeping a bug fix.

From a positioning standpoint, this PR reinforces APM's 'behavioral parity across commands' promise and demonstrates healthy community contribution patterns. The author identified a genuine gap, wrote integration tests that prove the fix, and followed the existing architectural patterns.

Dissent. The python-architect flags the outer except Exception as a recommended finding, noting it could mask future refactors if PolicyBlockError handling changes from sys.exit to raising. The test-coverage-expert independently confirms no test exercises this handler (evidence=missing). These reinforce each other: the handler is untested AND fragile to future change. I side with treating this as a follow-up issue rather than a blocker because (a) the current behavior is correct (SystemExit inherits BaseException, not Exception), and (b) the fix is mechanical -- add a test and narrow the except clause.

Aligned with: Portable by manifest (lockfile now reflects declared LSP servers after update, closing manifest-to-runtime drift); Secure by default (policy enforcement and allowExecutables gates apply on update identically to install); Governed by policy (MCP/LSP reconciliation on update now enforces same policy checks, closing a governance bypass where orphaned servers persisted); OSS community driven (external contributor @cffnpwr identified, reported, and fixed a real behavioral gap with integration tests).

Growth signal. The oss-growth-hacker's release narrative is strong: 'apm update now fully reconciles MCP/LSP servers -- behavioral parity with apm install, contributed by the community.' Credit @cffnpwr by name in the CHANGELOG and release notes. The silent-config-rot angle reinforces APM's positioning as the package manager that takes runtime correctness seriously.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Clean extraction following the existing LSP integration pattern. Outer except Exception and untyped params are minor.
CLI Logging Expert	0	2	1	PolicyBlockError in update.py uses _rich_error instead of logger.error, breaking logging parity with install.py.
DevX UX Expert	0	2	2	MCP/LSP reconciliation in update is a correct fix; policy-block error lacks recovery guidance and flag-parity gap should be documented.
Supply Chain Security Expert	0	0	2	MCP/LSP reconciliation on update path correctly enforces policy, allowExecutables gate, and consent; no security regressions found.
OSS Growth Hacker	0	1	2	Community-contributed bug fix closes a silent config-rot gap; needs CHANGELOG entry and is great release-narrative material.
Doc Writer	0	3	1	Lockfile field descriptions are accurate; three gaps: lifecycle table, update.md Behavior section, flag parity gap undocumented.
Test Coverage Expert	0	2	1	Core regression for #2077 is well-covered at integration tier (3 CliRunner tests, all passing); two recommended gaps on error/failure paths.

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

Top 5 follow-ups

1. [OSS Growth Hacker] Add CHANGELOG entry under [Unreleased] crediting @cffnpwr for the fix. -- User-facing behavior change requires a CHANGELOG line per project convention; community credit reinforces contributor flywheel.
2. [Test Coverage Expert] Add integration test exercising the except Exception handler around _run_mcp_lsp_integration in update.py. -- Evidence=missing on an error path that the python-architect independently flagged as fragile to future refactors. Failed-test-equivalent gap on a user-facing error surface.
3. [Doc Writer] Add apm update to lockfile-spec.md lifecycle table and document MCP/LSP reconciliation in update.md Behavior section. -- Lockfile spec now has a new writer (update) but the lifecycle table doesn't reflect it; update.md Behavior section is the canonical place users look for what update does.
4. [DevX UX Expert] File issue for flag parity: --no-policy, --trust-transitive-mcp, --runtime on apm update. -- Users who hit PolicyBlockError during update have no escape hatch. Pre-existing gap but now more visible since update exercises MCP policy checks.
5. [CLI Logging Expert] Harmonize PolicyBlockError handler in update.py to use logger.error instead of _rich_error, matching install.py. -- Logging parity between install and update for identical error paths; affects structured log consumers and --quiet mode behavior.

Architecture

classDiagram
    direction LR
    class MCPIntegrator {
        <<Facade>>
        +collect_transitive() list
        +deduplicate(deps) list
        +install(deps, ...) int
        +remove_stale(servers, ...)
        +update_lockfile(servers, path)
        +get_server_names(deps) set
        +get_server_configs(deps) dict
    }
    class LSPIntegrator {
        <<Facade>>
        +collect_transitive() list
        +deduplicate(deps) list
        +install(deps, ...) int
        +remove_stale(servers, ...)
        +update_lockfile(servers, path)
        +resolve_target_runtimes() list
    }
    class run_mcp_integration {
        <<Function -- install/mcp/integration.py>>
        +run_mcp_integration(**kwargs) tuple
    }
    class run_lsp_integration {
        <<Function -- install/lsp/integration.py>>
        +run_lsp_integration(**kwargs) int
    }
    class install_command {
        <<Caller -- commands/install.py>>
        +_install_apm_packages(ctx, outcome)
    }
    class update_command {
        <<Caller -- commands/update.py>>
        +_run_dep_update()
        +_run_mcp_lsp_integration()
    }
    class PolicyBlockError {
        <<Exception>>
    }
    class LockFile {
        <<ValueObject>>
        +mcp_servers set
        +mcp_configs dict
        +lsp_servers set
        +lsp_configs dict
        +read(path) LockFile
    }
    class APMPackage {
        <<ValueObject>>
        +get_all_mcp_dependencies() list
        +get_lsp_dependencies() list
        +from_apm_yml(path) APMPackage
    }
    run_mcp_integration ..> MCPIntegrator : delegates
    run_mcp_integration ..> PolicyBlockError : raises
    run_lsp_integration ..> LSPIntegrator : delegates
    install_command ..> run_mcp_integration : calls
    install_command ..> run_lsp_integration : calls
    update_command ..> run_mcp_integration : calls
    update_command ..> run_lsp_integration : calls
    update_command ..> LockFile : snapshots old state
    update_command ..> APMPackage : re-parses after pin

Loading

flowchart TD
    A["apm update CLI"] --> B["_run_dep_update()"]
    B --> C["LockFile.read() -- snapshot old MCP/LSP state"]
    C --> D["_install_apm_dependencies() -- resolve + materialize"]
    D --> E{"plan accepted?"}
    E -->|No| F["return early"]
    E -->|Yes| G["_annotate_lockfile_revision_tags()"]
    G --> H["_run_mcp_lsp_integration()"]
    H --> I["clear_apm_yml_cache()"]
    I --> J["APMPackage.from_apm_yml -- re-parse after pins"]
    J --> K["run_mcp_integration() -- install/mcp/integration.py"]
    K --> L["MCPIntegrator.collect_transitive"]
    L --> M["filter_mcp_by_allow_executables"]
    M --> N{"mcp_deps?"}
    N -->|Yes| O["run_policy_preflight -- raises PolicyBlockError"]
    O --> P["MCPIntegrator.install"]
    P --> Q["MCPIntegrator.remove_stale (old - new)"]
    Q --> R["MCPIntegrator.update_lockfile"]
    N -->|No + old servers| S["remove_stale(all old) + update_lockfile(empty)"]
    R --> T["run_lsp_integration()"]
    S --> T
    T --> U["LSPIntegrator install/prune/lockfile"]
    U --> V["render summary + exit"]

Loading

Recommendation

Ship this PR. The core fix is correct, well-tested at the integration tier, security-clean, and architecturally sound. The five follow-ups (CHANGELOG entry, error-path test, two doc gaps, flag-parity issue) are all post-merge work that does not affect shipped correctness. The CHANGELOG entry should ideally land before or with the next release cut, not necessarily before this PR merges. This is a community contribution -- optimize for merge velocity and contributor experience.

---

Full per-persona findings

Python Architect

• [recommended] Outer except Exception in _run_dep_update masks future refactors at src/apm_cli/commands/update.py:694
  NOT a current bug (SystemExit inherits BaseException not Exception). But if _run_mcp_lsp_integration ever changes to raise instead of sys.exit, the outer handler would mask it with a generic message. The dual-exit pattern is fragile.
  Suggested: Let PolicyBlockError propagate out of _run_mcp_lsp_integration and add a dedicated except PolicyBlockError clause at the call site in _run_dep_update, before the generic except Exception.

• [nit] run_mcp_integration has 19 keyword parameters -- consider a config dataclass at src/apm_cli/install/mcp/integration.py:15
  The noqa: PLR0913 is honest about it. A frozen MCPIntegrationConfig dataclass would make the two call sites more uniform.

• [nit] logger/diagnostics params untyped at src/apm_cli/install/mcp/integration.py:26
  Adding TYPE_CHECKING-guarded annotations would improve IDE support.

CLI Logging Expert

• [recommended] PolicyBlockError handler in _run_mcp_lsp_integration uses _rich_error instead of logger.error at src/apm_cli/commands/update.py
  install.py catches the identical PolicyBlockError with logger.error + logger.render_summary(). update.py uses _rich_error for the same message, bypassing CommandLogger lifecycle.
  Suggested: Replace _rich_error(...) with logger.error(...) to match install.py line 1882.

• [recommended] Outer MCP/LSP catch-all also uses _rich_error/_rich_info instead of logger at src/apm_cli/commands/update.py
  Compare with _resolve_and_stage_revision_pin_updates (same file) which correctly uses logger.error and logger.info for structurally identical error handling.
  Suggested: Use logger.error(f"Error reconciling MCP/LSP servers: {e}") and logger.info(...).

• [nit] Verbose detail uses '(ies)' pluralization instead of count-aware noun at src/apm_cli/install/mcp/integration.py
  Suggested: noun = "dependency" if len(transitive_mcp) == 1 else "dependencies"

DevX UX Expert

• [recommended] PolicyBlockError in update gives no recovery action
  When apm install hits a policy block, users can pass --no-policy. In apm update, the same block fires with no escape hatch and no hint of what to do.
  Suggested: Append hint: 'To skip policy checks, use apm install --no-policy after updating.' or add --no-policy to apm update.

• [recommended] Flag asymmetry between install and update is invisible to the user
  apm install exposes --runtime, --exclude, --trust-transitive-mcp, --no-policy. apm update silently uses hardcoded defaults for all of these.
  Suggested: Add a note in apm update --help or CLI reference: 'MCP policy and runtime flags are not yet supported; use apm install for full control.'

• [nit] Success message grammar issue when installed=0
  'No dependency changes were applied.' reads like failure when user explicitly accepted.

• [nit] Consumer guide (update-and-refresh.mdx) doesn't mention MCP reconciliation
  A user reading the consumer guide won't know update cleans up orphaned MCP configs.

Supply Chain Security Expert

• [nit] Lifecycle script failures silently swallowed in _fire_update_scripts (pre-existing)
  contextlib.suppress(Exception) swallows all errors from pre-update scripts. If a lifecycle script enforces a security constraint, the update proceeds regardless.

• [nit] Pre-update lifecycle scripts fire before user consent (pre-existing, consistent with install)

OSS Growth Hacker

• [recommended] Missing CHANGELOG entry under [Unreleased] for this user-facing bug fix
  Suggested: Add to [Unreleased] ### Fixed: apm update now reconciles MCP and LSP servers against apm.yml, removing orphaned MCP server entries from runtime configs and writing LSP servers to the lockfile -- matching apm install behavior. (by @cffnpwr, [BUG] apm update does not prune orphaned MCP servers from apm.lock.yaml #2077)

• [nit] Lockfile spec doc change is accurate but misses story-shaped opportunity

• [nit] Community contributor pattern worth amplifying (thank-you comment, release note credit)

Auth Expert -- inactive

No auth/token/credential surfaces touched; PR changes MCP/LSP reconciliation in apm update.

Doc Writer

• [recommended] apm update missing from lockfile-spec.md lifecycle table at docs/src/content/docs/reference/lockfile-spec.md
  apm update now writes mcp_servers/mcp_configs and lsp_servers/lsp_configs but the lifecycle table doesn't list it as a writer.
  Suggested: Add row: apm update | existing lockfile (MCP/LSP baseline) | rewrites mcp_servers, mcp_configs, lsp_servers, lsp_configs after applying plan

• [recommended] update.md Behavior section does not mention MCP/LSP reconciliation at docs/src/content/docs/reference/cli/update.md
  Users running apm update with an MCP server removed from apm.yml will see reconciliation output with no explanation in the reference docs.
  Suggested: Add a bullet: MCP/LSP reconciliation: after applying the dependency plan, apm update reconciles MCP and LSP servers against the updated apm.yml, mirroring apm install behavior.

• [recommended] Flag parity gap (--trust-transitive-mcp, --no-policy not on update) undocumented at docs/src/content/docs/reference/cli/update.md
  Users migrating workflows from install to update will hit this silently.
  Suggested: Add a Limitations note: apm update does not yet support --trust-transitive-mcp or --no-policy.

• [nit] Two-word change is accurate and consistent with existing voice

Test Coverage Expert

• [recommended] Integration tests for update MCP/LSP prune pass -- regression [BUG] apm update does not prune orphaned MCP servers from apm.lock.yaml #2077 is well-defended at tests/integration/test_update_mcp_lsp_prune.py
  Proof (passed): tests/integration/test_update_mcp_lsp_prune.py::TestUpdatePrunesOrphanedMCPServers::test_apm_update_prunes_orphaned_mcp_server
  assert updated_lock.mcp_servers == [], f"Expected orphaned MCP server to be pruned, got: {updated_lock.mcp_servers}"

• [recommended] No test exercises the except Exception error handler around _run_mcp_lsp_integration in update.py at src/apm_cli/commands/update.py
  Proof (missing): tests/integration/test_update_mcp_lsp_prune.py::test_apm_update_exits_nonzero_on_mcp_reconciliation_failure

• [nit] No integration-tier test for PolicyBlockError exit path in apm update (unit tier covers propagation)
  Proof (missing): tests/integration/test_update_mcp_lsp_prune.py::test_apm_update_policy_block_exits_nonzero

Performance Expert -- inactive

No hot-path surface touched. Overhead is bounded by dep count (single-digit typically), justified for correctness.

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

[sergio-sisternes-epam]

Spec conformance gate -- how to fix

The Spec conformance gate job is failing because the Mode B detector sees 112 substantive new lines under src/apm_cli/install/mcp/ (a normative critical path) and no corresponding spec citation.

This PR is a refactor + bug fix, not a new normative behaviour -- the extracted run_mcp_integration() implements exactly what apm install already did; it just makes it reachable from apm update. The spec already covers MCP integration; this PR closes an implementation gap, not a spec gap.

The fix is a single line. Add this to the PR description body:

apm-spec-waiver: refactor extracts existing MCP integration logic to a shared function; apm update bug fix closes implementation gap, no new normative behaviour

That line must start at column 1 (no leading spaces) in the PR body. The detector reads it, echoes the rationale to the CI log, and exits 0. The waiver is reviewer-auditable -- maintainers can see exactly what was claimed and verify it matches the diff.

How to add it:

1. Edit the PR description (pencil icon top-right of the PR body)
2. Add the apm-spec-waiver: line anywhere in the body (e.g. after the "Out of scope" paragraph)
3. Save -- CI will re-run automatically

No code changes needed.

[cffnpwr]

@sergio-sisternes-epam Thanks for the pointer. The waiver line is now in the PR body:

apm-spec-waiver: install/mcp/integration.py is a behaviour-preserving extraction of the MCP integration block from commands/install.py (apm install semantics unchanged); the update-side fix alters no req-covered behaviour (req-rs-011/012 untouched)

One note: the body edit alone did not re-run the gate — the Spec conformance workflow's pull_request trigger uses the default activity types (opened/synchronize/reopened), which do not include edited. I pushed a no-op update (author timestamp only, identical tree) to fire a synchronize event instead. The re-triggered workflows are currently awaiting maintainer approval to run.